Repository navigation
fix(hooks): tell "no PR" apart from a lookup that failed, and never ask the wrong repository - #364
Conversation
…sk the wrong repository
Three hooks looked a branch's PR up with
gh pr list --head "$branch" --json number --jq '.[0].number' 2>/dev/null || true
which is wrong twice over, and silently both times.
`2>/dev/null || true` throws away gh's exit status and its stderr together, so an
empty result and a failed request are the same value. `check-history-rewrite.sh`
then told the operator "No PR was found for this branch" whether the branch had no
PR, gh was missing, gh was unauthenticated, gh was rate-limited or the network was
down. Both of the first two happened on #356 in one day
and produced identical output.
There was also no `-R`. In this fork gh prefers the `upstream` remote, and the fix
(`gh repo set-default`) writes `remote.origin.gh-resolved` into a LOCAL, uncommitted
config - so a CI runner, another machine or a fresh agent sandbox asks
confluentinc/parallel-consumer instead. That failure mode is the one AGENTS.md calls
the damaging case, because it SUCCEEDS: in `remind-inflight-on-push.sh` a PR number
from upstream would have been matched against this repo's `docs/inflight/pr-<n>-*.md`
and quoted a completely unrelated note.
WHAT CHANGES. Each lookup now derives the repository from `origin`, runs bounded in
python3 (`timeout(1)` is GNU-only and absent on macOS), and reports three outcomes
apart from each other: found, a measured absence, and a failure that quotes what gh
actually said. When `origin` yields no hosting slug the lookup is NOT retried
unqualified - a wrong answer that resolves is worse than no answer - and the hook
says it did not run. The same treatment is applied to the two other gh calls in
`check-history-rewrite.sh`: `gh api repos/{owner}/{repo}/...` resolved the wrong way
too, and a comment count that FAILED was being rendered as "no open review comments
were found", which is the same defect one level down.
Fail-closed is unchanged. `check-history-rewrite.sh` refuses the rewrite in every one
of these cases, including the new ones, and three self-test arms pin that. The
reminder and the merge guard stay non-blocking by design; what changes there is that
a failed lookup now says so instead of producing the silence a clean result produces.
SHAPE FOLLOWED, NOT INVENTED. `.claude/hooks/inject-branch-context.sh` on
#350 already solved this correctly - origin-derived slug with
a scheme allowlist, no `2>/dev/null || true`, a timeout in python3, and an explicit
problem message. Its regex and its reasoning are reused here rather than re-derived,
so the two do not drift. The one departure: these hooks keep `gh pr list --head`,
which exits 0 printing nothing when there is no PR, so absence is read off the exit
status and no stderr regex (`NO_PR_RE`) is needed.
DEFECT-CLASS SWEEP, since the class is "a gh call that neither names its repo nor
keeps its exit status". Fixed in `check-history-rewrite.sh`,
`remind-inflight-on-push.sh` and `check-merge-outstanding-work.sh`. Checked and found
already correct: `bin/check-branch-self-reference.sh` (has `-R`),
`bin/check-pr-analysis-surfaces.sh` (`--repo "$REPO"` on every call),
`.claude/hooks/check-squash-subject.sh`, `.claude/hooks/pre-commit-gate.sh` and
`.claude/hooks/inject-merge-checklist.sh` (make no gh calls at all). One more instance
found in `bin/check-pr-ready.sh`, fixed in the following commit.
TESTS. `bin/test-check-agent-hooks.sh` gains a section covering all three hooks: a
failed lookup is named as a failure and quotes gh's own words; an empty answer is
reported as a measured absence and names the repository it asked; every gh call the
rewrite guard makes is asserted against the argv the stub recorded, because a
wrong-repo lookup is invisible in the message; an underivable slug is reported rather
than guessed at, with the argv log asserted EMPTY; and the rewrite is still refused in
all of those cases. Verified red against the pre-fix hooks: 10 of the 18 new
assertions fail, and the 8 that pass are the fail-closed invariants and the
near-miss guards, which must not change. The push fixture repo gains an `origin`
remote, now load-bearing rather than cosmetic.
`bin/check-all.sh`: 35 ran, 35 passed, 0 could not run, 5 skipped (the four PR-state
reporters and the one needing a maven log, skipped by design without `--pr`).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
…rting a failed lookup as no PR
The same defect the previous commit fixed in the hooks, found by sweeping for the
class rather than the symptom: `bin/check-pr-ready.sh` resolved the branch's PR with
an unqualified `gh pr list ... 2>/dev/null || true` and then read the PR with an
unqualified `gh pr view`.
Unqualified, both address confluentinc/parallel-consumer on any machine without the
local `remote.origin.gh-resolved` config, so the script could enumerate what is
outstanding on a stranger's PR and present it as this branch's - the wrong answer that
resolves, which is worse than an error. And a lookup that failed printed the same
usage line as a branch with genuinely no PR, on a script whose entire purpose is to
refuse to conclude readiness from what it did not measure.
The repository is now an env-overridable constant, the convention its neighbours
`bin/check-pr-analysis-surfaces.sh` and `bin/check-branch-self-reference.sh` already
use - deliberately not the hooks' origin-derived slug, because a gate runs against
this repo while a hook runs wherever the agent happens to be. Both exits stay 2
("cannot run", never a pass), and the failure path now says what gh said and that
nothing was measured.
Verified by hand on all three paths: no PR for this branch, a real PR number
(#350), and a PATH with no gh at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
|
… that waited on each other A ce-simplify-code pass over this branch. Three reviewers ran against the diff; what they found that was worth taking, and what was declined, is below. THE FOURTH ANSWER. The lookup contracts to print `found`, `failed` or `none`, and both `remind-inflight-on-push.sh` and `check-merge-outstanding-work.sh` dispatched on the first two with no arm for anything else. An empty answer - the interpreter killed for memory, or a BaseException that its `except Exception` cannot catch - therefore matched neither test and fell through to exactly where `none` falls through: silence. That is this branch's own defect one level down, an absence of measurement rendered as a measurement of absence, and `check-history-rewrite.sh` already carried the backstop the other two lacked. Both now treat an unrecognized answer as a failure, which routes it to the advisory each hook already has. Neither becomes a deny: the fail-open posture is deliberate and documented, and the fix is that it stops being fail-SILENT. Two self-test arms per hook cover it, and they were written first and confirmed red against the unfixed hooks - the lookup is the only python3 call in either hook whose first argument is `-`, so a stub that kills just that invocation leaves the token scan and the JSON emitter on the real interpreter, and the resulting silence is attributable to the dispatch rather than to a hook that never ran. TWO ROUND TRIPS THAT DID NOT NEED TO BE SERIAL. In `check-history-rewrite.sh` the inline-comment count and the in-progress-run count depend only on the slug, the PR number and the branch - never on each other - but ran one after the other, so their 10s bounds added on a path that already pays for a slug lookup and a PR lookup before it. Overlapped in a two-worker pool: `subprocess.run` is thread-safe, each call keeps its own timeout, and the results are read back in the same order, so the message is unchanged and only the waiting is shared. Measured rather than asserted, control arm being the committed sequential hook against the same branch, repo and network: 1.86/1.81/1.85s concurrent versus 2.34/2.45/2.23s sequential. Non-overlapping, and the gap is one gh round trip, which is what the change predicts. CITATIONS THAT RESOLVED NOWHERE. Three comments added by this branch cite `.claude/hooks/inject-branch-context.sh`, which is on #350 and not in this tree - so a reader who greps for it, as the anchor convention tells them to, finds nothing and cannot tell a moved file from an unlanded one. `bin/check-file-refs.sh` cannot catch this: it scans .md/.adoc/.txt/.html and never .sh. Each now names the PR the file arrives with. The inflight note gains that hook as a FOURTH copy of the lookup - and it is the copy where the `file://` local-clone case was found and fixed, so it is ahead of these three rather than behind them - plus `bin/check-quarantine-owners.sh`, which solved the confirmed-absence-versus-transient-failure half in bash. Whoever does the #357 fold-in needs both, and neither was reachable from the note. The note also carried a `file-refs: N/A` marker that protected nothing. The gate scopes a marker by scanning DOWNWARD from the citing line to the next blank line; this one sat alone between two blank lines, so it covered no line at all. Verified by deleting it and re-running the gate, which stayed green. Removed, and the real exemption is now inside the block it applies to. DECLINED, with the harm each would have done: - Extracting the triplicated lookup into a shared helper. A guard that cannot source its helper fails OPEN, which is the argument #341 makes by name, and the fold-in is already recorded as deferred until the shared hooks library from #357 is on master. Extracting now would mean inventing a second shared location for hooks. - Swapping the slug derivation for the one-line sed in `bin/check-quarantine-owners.sh`. That version predates the `file://` fix, so a `file:///home/astubbs/git/parallel-consumer` origin would yield the slug `git/parallel-consumer` and gh would be asked about a repository that does not exist - reintroducing the wrong-repo class this branch exists to close. Recorded in the note as prior art to read instead. - Sharing the gh-stub test helper with `bin/test-check-pr-analysis-surfaces.sh`. The two files test unrelated scripts and no shared test-helper library exists for `bin/`; inventing one for two call sites buys nothing. No fail-closed behaviour was removed, no diagnostic distinction was merged, and the four outcome states are intact. The self-test goes from 18 lookup assertions to 22 and stays green; `bin/check-all.sh` reports 35 ran, 35 passed, 0 could not run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
|
@claude review this Three commits, all agent tooling - no runtime code. A steer on where the risk actually is. What this changes. A PR lookup in three hooks used to collapse three unrelated situations into one sentence, "No PR was found for this branch": the branch really has no PR, the lookup failed, or the lookup asked the wrong repository (no Be hardest on these:
Known and not worth your time: this One thing outside this PR that you might confirm: |
|
Claude finished @astubbs's task in 7m 13s —— View job Review complete
A harness note before the review itself, so nothing below reads as "review skipped it": in this 1. Fail-open/fail-closed - no regression found. 2. Slug derivation. Traced the regex by hand against the shapes you asked about:
3. The two-worker pool. 4. Self-test honesty. Your characterization of the four newest assertions checks out exactly: One gap this angle surfaces that isn't mentioned anywhere in the PR: 5. The duplication. Checked whether "four places" means four copies of one thing: it doesn't, Confirming the file-ref-gate question, with the source rather than a guess: Bottom line: no P0 found - every guard's deny/advisory posture is unchanged in strength across |
Ten commits of master, including the shell-hazard and shallow-clone guards that land on the same three files this branch rewrote. No textual conflict: `git merge` resolved every file on its own, and the three overlapping ones were verified rather than trusted. NOTHING HERE IS SUPERSEDED, which was the thing worth checking. `ci(hooks): two guards for answers that are wrong rather than absent` edits `.claude/hooks/check-merge-outstanding-work.sh`, `.claude/hooks/remind-inflight-on-push.sh` and `bin/check-pr-ready.sh`, which is exactly this branch's file set, but it edits a different defect in them: it annotates the `stat -c` / `stat -f` platform probes with `hazard-ok:` markers so `bin/check-shell-hazards.sh` can tell a deliberate GNU/BSD split from an accidental one. This branch's subject is the PR lookup underneath, which tells "found", "measured absence", "the lookup failed" and "the repository could not be derived" apart instead of collapsing all four into silence. The two changes are adjacent in the file and disjoint in what they fix, so both survive whole. VERIFIED PER FILE AGAINST BOTH PARENTS, not by reading the merge and finding it plausible. For each of the three, the merge-versus-branch-tip diff was compared against master's own contribution and the merge-versus-master-tip diff against this branch's, and the only differences are hunk-header line offsets: neither side lost a hunk. `bin/test-check-agent-hooks.sh` was expected to conflict and did not - master never touched it - so all 181 arms, including the 24 the lookup work added, are carried across unchanged. The two notes this branch's code points forward to are still accurate after the merge: neither #350 (`inject-branch-context.sh`) nor #357 (the shared hooks library that `docs/inflight/ci-pr-lookup-is-copied-into-three-hooks.md` waits on) has landed on master, so `.claude/hooks/lib/` still does not exist and the deferred fold is still deferred. A DISCREPANCY WORTH RECORDING RATHER THAN SHRUGGING OFF. The first post-merge `bin/check-all.sh --with-tests` reported `test-check-agent-hooks.sh` FAILED at 74s while the same script run directly, moments earlier and moments later, passed all 181 arms. It has not reproduced since: five standalone runs, three `--tests-only` sweeps, three instrumented `--with-tests` sweeps and four sweeps under ten-way CPU load, all green. One failure in sixteen, and `--tests-only` and `--with-tests` run the self-test phase identically, so the evidence does not support "depends on how it is invoked" - it supports an intermittent race. What the hunt did establish is where such a race would live, and it is new. Before 1078acd the sweep in `bin/check-all.sh` was sequential; it now scatters all 24 self-tests at once, and four of them mutate the REAL working tree rather than a scratch directory: `bin/test-check-file-refs.sh` and `bin/test-check-issue-refs.sh` both write `docs/note.md`, the same path, one of them following it with `git checkout` in the real repo; `bin/test-todo-index.sh` regenerates `docs/todo-index.md` and restores it from a trap; `bin/test-check-agent-hooks.sh` writes `docs/inflight/pr-90001-selftest.md`. Separately, the push-reminder throttle stamp is `${TMPDIR}/pc-push-reminder-<branch>` in a user-global directory shared by every worktree and session, and the self-test clears it with a `pc-push-reminder-*` glob that does not distinguish its own fixture from a live session's. A control-arm run of just those four scripts, six iterations concurrent against six sequential, did not fire either, so the hazard is real by inspection and unproven by experiment. AND THE CLONE WENT SHALLOW UNDER THIS WORKTREE MID-SESSION, which is the same failure class arriving from outside. It was full at the merge and full through every check above; a sibling agent's depth-limited fetch re-shallowed the shared git dir afterwards, and `git merge-base` immediately started returning nothing while `git rev-list --count origin/master..HEAD` reported 820 against a true 3. `bin/check-issue-refs.sh` went from CANNOT RUN back to a pass across `git fetch --unshallow` with nothing else changed, so a gate reporting differently between two runs minutes apart is a measured fact here and not always the gate's fault. This is exactly what `check-shallow-history.sh` arrived to catch, and it cannot catch it in a session that started before the hook existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One conflict, in bin/test-check-agent-hooks.sh: both sides appended a self-contained section of arms at the same point - this branch's PR-lookup arms and #350's inject-branch-context arms. Neither replaces the other and the helper namespaces do not collide (lk_ against bctx_), so both sections are kept. Verified as a strict superset of each parent's assert cases rather than by eye, and the suite passes. #350 landing also settles this branch's three citations of .claude/hooks/inject-branch-context.sh, which until now named a file that existed only on that branch.
…escribing them as future #350 and #357 both merged while this branch was open, which falsifies three claims it was carrying. `.claude/hooks/remind-inflight-on-push.sh` told the reader to grep `.claude/hooks/inject-branch-context.sh` "on that branch" because the file was not in this tree. It is now, and the anchor it names resolves here. The inflight note was deferred on the shared hooks library from #357. That library is on master, so the note loses its deferred state and becomes ordinary open work. More usefully, #357 settled in code a question the note could only argue about. Its two non-blocking reminders DO source `hook-common.sh`, behind a fail-open `[ -r "$hook_lib" ] || exit 0`; every hook that actually refuses a tool call still inlines. So the line #341 draws is refusing-guard against advisory-reminder, not hook against gate - all of these are `PreToolUse`, so the event type does not tell you which is which. That reclassifies the copies. `remind-inflight-on-push.sh` is advisory and already sources the library, so the fail-open objection never applied to it; it can move today, alongside `bin/check-pr-ready.sh`. The two refusing guards stay inlined unless someone deliberately swaps in a fail-closed loader, which is a change to their failure mode rather than a refactor, and the note now says so instead of lumping all four together. One citation in the note was never greppable: it cited `origin_slug()` where the hook defines `def origin_slug(target)` in python. Corrected to a string that greps, and every other anchor in the note was re-grepped rather than assumed. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…criminator, on the cooperative arm The chaos lane went red on this PR's CI. The failure is not this PR's - it changes shell and markdown only, so it cannot reach the chaos engine - but the house rule is that a sighting gets its ledger entry before the PR merges, because the replay seed dies when the log expires. `ChaosRevokeUnderWorkCooperativeIT.revokeUnderWorkStaysProtocolHonestWithCooperativeAssignor` failed the same correctness SLO the family's other captures trip, and the automatic thread dump shows the same thing the 2026-08-26 section above identified: the poll thread BLOCKED acquiring the `commitCommand` monitor at `AbstractParallelEoSStreamProcessor:1585`, reached through `onPartitionsRevoked` at `:548`, with the monitor held by that instance's own control thread. Frame for frame and holder for holder it is that capture, so the reasoning there is cited rather than repeated. What it adds is that the cycle is not a property of the key-order arm. The first capture was key-order under the eager assignor; this is the cooperative assignor in the plain revoke-under-work scenario. Both enter through `onPartitionsRevoked`, so the pair is reachable across the arm split rather than through one scenario's interleaving. Two captures is not a rate and the entry does not claim one - neither seed has been replayed - but it removes "seen once" as a reason to defer the verification the previous section asks for. The same-run control arm is unusually strong and is recorded: every sibling scenario passed in the same JVM on the same runner, including `ChaosRevokeUnderWorkCooperativeDrainIT`, which is the same cooperative assignor differing only in stop-mode. The box is ruled out too - three other chaos jobs went green in the same half hour, and the other non-successes were `cancelled` by the box-exclusive queue rather than failed. Also recorded: the run-logs archive downloaded short the first time, a zip with no central directory, and `gh api` exited 0 both times. Only `unzip -t` distinguished the truncated download from the complete one. That is the `gh run view` truncation class arriving by a different route, and a grep over the short archive would have come back quiet and looked like an answer. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…est that can fail
The automated review found the one real gap in this branch, and it is the branch's
own subject: `bin/check-pr-ready.sh` got the same four-outcome lookup fix as the
three hooks and shipped with no behavioural coverage at all.
Its self-test had eight cases and every one of them grepped the script's own source
text - "does the body contain this phrase". Prose assertions pass identically before
and after a behavioural change, so nothing exercised the new `lookup_problem`
messaging, nothing checked that `-R "$REPO"` reaches the `gh` argv, and nothing
proved `PR_READY_REPO` overrides anything. `bin/AGENTS.md` states the bar this
missed: when you fix a bug in a checker, add the case to its self-test and verify it
goes red against the old code, because a regression test that has never failed
proves nothing.
Twelve cases added. They run the script against a stubbed `gh` that records its own
argv, each in a throwaway repository on a named branch, with the stub first on PATH
so the answer under test is chosen rather than ambient:
- a failed lookup is named as a failure, quotes what gh actually said, and states
that nothing was measured
- an empty answer - gh exiting 0 with no output, which is what a real absence looks
like for `gh pr list --head` - reads differently from that failure
- the lookup carries `-R`, and the message names the repository it asked
- `PR_READY_REPO` changes the repository actually queried, not just a string in a
comment
- a detached HEAD is reported as having no branch, and gh is not asked about a
branch literally named `HEAD`
Proven red rather than merely passing: restore master's pre-fix script with these
cases in place and 10 of the 12 fail. The 2 that pass either way are invariants that
must hold in both versions by construction, so they are controls, not coverage.
The review's other four points were traced and found sound, and it agreed with the
deferred fold-in already recorded in
docs/inflight/ci-pr-lookup-is-copied-into-three-hooks.md. It also confirmed, from
`.github/scripts/file-ref-gate.js` rather than by inference, that `CITING_FILE` does
not include `.sh`, so path citations inside shell comments are unenforced. That is a
real gap in the gate and outside this change; it is not fixed here.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixed in a97c27f, and you were right about why it was missable: the eight existing cases all grepped the script's own source text, so they pass identically before and after a behavioural change. Nothing exercised the new Twelve cases added. They run the script against a stubbed
Held to the bar you cited rather than just added: restoring master's pre-fix script with these cases in place fails 10 of the 12. The other two are invariants that must hold in both versions by construction, so they are controls rather than coverage, and I would rather say so than count them. On your other points, briefly, since three of them changed nothing and that is worth stating explicitly:
One update since you reviewed. Both PRs this branch waited on have merged, which settled the duplication argument in code rather than in prose: #357's two non-blocking reminders do source the shared hooks library, behind a fail-open |
1 participant
Serves no single issue.
depends on #350
depends on #357
Not a functional dependency - this branch is cut from master, stands alone, and its self-tests pass
without #350. Two things couple them. Three comments here cite
.claude/hooks/inject-branch-context.sh, which only #350 provides, andbin/check-file-refs.shdoes not scan
.shcomments so nothing would catch them dangling. And both editbin/test-check-agent-hooks.sh, which conflicts; the new arms here are one contiguous append sowhoever lands second resolves it cheaply.
Description
A hook that could not look up a PR said "No PR was found for this branch". Three unrelated
situations produced that one sentence, and the operator could not tell them apart: the branch really
has no PR; the lookup failed (no
gh, no auth, rate limit, offline); or the lookup asked the wrongrepository.
2>/dev/null || truediscarded both the error and the exit status.The third is the worst and is specific to this fork. None of these calls carried
-R, andghprefers
upstreamunlessremote.origin.gh-resolvedis set - a local, uncommitted config, so CIrunners and fresh agent sandboxes all start without it. AGENTS.md has a section on exactly this.
Hit live twice on #356, from two different causes.
Each lookup now derives the slug from
origin, runs bounded, and reports which of four states it isin: found; measured absence; failed, quoting what
ghsaid; or did not run because the slug couldnot be derived - in which case
ghis deliberately not called at all rather than calledunqualified. Every guard stays fail-closed; this changes the message, never the verdict.
Same defect fixed in
remind-inflight-on-push.sh, where it was live-dangerous - an upstream PRnumber would have matched this repo's
docs/inflight/pr-<n>-*.mdand quoted an unrelated note - andin
check-merge-outstanding-work.sh, which kept its deliberate fail-open and merely stopped beingsilent. A class sweep also found it in
bin/check-pr-ready.sh(separate commit).Checked and ruled out:
check-branch-self-reference.shandcheck-pr-analysis-surfaces.shalready qualify every call;
check-squash-subject.sh,pre-commit-gate.sh,inject-merge-checklist.shandinject-recorded-knowledge.shmake noghcalls.The lookup is now written three times. Deliberate: #350's header cites #341 arguing against
extracting shared hook logic, because a guard that cannot source its helper fails open. Folding it
into
hook-common.shonce #357 lands is recorded indocs/inflight/ci-pr-lookup-is-copied-into-three-hooks.md.Pattern adopted from #350 rather than invented: origin-derived slug, bound in python3 rather than
GNU-only
timeout(1), no swallowing. One departure - these usegh pr list --head, which exits 0printing nothing when there is no PR, so absence is read off the exit status and #350's stderr regex
is unnecessary.
Checklist
docs/inflight/ci-pr-lookup-is-copied-into-three-hooks.mdrecords theduplication and what retires it.
docs/features/- N/A - agent toolingonly, no runtime behaviour changes.
bin/test-check-agent-hooks.sh. Proven red against thepre-fix hooks: restore the three from master with the new arms in place and 10 of 18 fail;
the 8 that pass are the fail-closed invariants, which must not change. Verified independently
of the agent that wrote them.