Repository navigation
tooling(agents): hand an agent the branch's own commits, PR body and PR comments - #350
Merged
Merged
Conversation
…ot be read Three scripts dated files with GNU `stat -c %Y`. BSD stat rejects `-c`, so on macOS every one of them read no mtime at all - and each had been written to treat that as "nothing to see": .claude/hooks/check-merge-outstanding-work.sh skipped the file bin/check-pr-ready.sh skipped the file .claude/hooks/remind-inflight-on-push.sh lost its throttle The first two are guards, and skipping is the wrong direction for a guard. The merge guard exists to refuse `gh pr merge` while a background task is still writing work that belongs in the PR - and on macOS it allowed every merge, silently, while reporting success. Its own header lists the fail-open cases it accepts deliberately (no session id, no scratch dir, no python3, unparseable JSON) and this was not among them: those all mean there is nothing to check, whereas this one found the evidence and discarded it. Both guards now FAIL CLOSED. A file that matched the session's tasks glob but cannot be dated counts as live, and the merge refusal says so rather than pretending to a reading it does not have. The platform is detected once by probing `stat -c` rather than by falling back from one to the other. A blind `stat -c %Y || stat -f %m` would be the same class of bug one level up: on Linux `stat -f` is --file-system and SUCCEEDS, so the fallback arm would return a number about the filesystem instead of the file - a wrong answer where the first arm gave none. Deliberately inlined in each script rather than shared through bin/lib. These are guards; sourcing a helper adds a failure mode where the helper cannot be found, and a guard that cannot load its own dependency fails open again. Four lines repeated beats a new way to silently pass.
`mapfile` is bash 4. macOS ships bash 3.2, where the line failed with "mapfile: command not found" and the script carried on to exit 0 - so a local run printed nothing alarming and reported success having examined no documents at all. That is precisely the failure this gate exists to catch one level up: a check whose green means "the check did not run". Replaced with a portable read loop. The gate's own header advertises running it locally before pushing. On macOS that advice returned a false green, which is how a self-reference reached CI as the first thing to notice it.
The suite aged its fixtures with `touch -d '@1000000000'`. BSD touch rejects that form, so on macOS the two cases that depend on an OLD file kept a current mtime, looked live, and failed - which then reads as a bug in the hook rather than in the harness. `touch -t 200109090146` is the same instant in the POSIX form both accept. This was the last of five sites in this change and the one that hid the other four: with the harness itself unable to run correctly, its failures were easy to attribute to the scripts under test. All self-tests now pass on macOS as well as on CI's ubuntu.
…ists on macOS The sweep behind this branch covered `stat -c`, `mapfile`, `readarray`, `grep -P`, `date -d`, `readlink -f`, `sed -i` and `touch -d`. `paste` was not on that list, and `.claude/hooks/inject-recorded-knowledge.sh` ends two of its lists with `paste -sd,` and no file operand. GNU paste reads stdin when no operand is given; BSD paste rejects it with a usage message. So on macOS the "Dated plans and investigations" and "Dated test-hardening audits" sections have printed `(none)` since they were added, while the hook wrote a usage line to stderr that nobody reads and exited 0. Same signature as the five already fixed here: the evidence was found and thrown away, and the result was indistinguishable from there being nothing to report. It is the same defect in the hook whose entire purpose is to stop prior art being invisible - so the two document sets an agent is most often told to check before forming a hypothesis were the two it could not see. Worked case: an adaptive-concurrency design was drafted and committed against `docs/plans/` prior art that this hook had been declining to list. Both sites now pass an explicit `-`, which GNU and BSD both accept, matching `bin/build-client.sh`'s existing `paste -sd' ' -`. Swept every tracked `*.sh` plus `bin/` and `.claude/` for stdin-reading `paste`: three sites in this hook (the third arrives with the ideation scanning on another branch), and `bin/build-client.sh`, which already had the operand and is correct. No others. Verified on macOS: both lists populate, stderr is empty, the hook exits 0, and `bin/test-check-agent-hooks.sh` still passes.
…l allowed every merge
The branch this sits on added a FAIL CLOSED arm to
`.claude/hooks/check-merge-outstanding-work.sh`: a task file that matched the
session's glob but could not be dated counts as live, and the merge is refused.
That arm was unreachable.
The script runs under `set -euo pipefail`, and `mtime="$(_mtime "$f")"` takes the
command substitution's exit status. When `stat` fails - the one condition the arm
exists for - the assignment fails, `set -e` kills the hook at that line, and the
`[ -z "$mtime" ]` test below is never evaluated. The hook exits 1 with no output,
which PreToolUse reads as a non-blocking error, so the merge is ALLOWED.
That is the same fail-open the branch set out to close, one level in: the guard
did not skip the file, it died holding it. `_mtime` now never fails - it prints
the mtime, or nothing - and the arm runs.
Found by giving the change a negative control it did not have, which is the other
half of this commit. CI is Linux, so the BSD arm shipped unexecuted and the
fail-closed arm is unreachable on ANY platform with a working `stat`: a file
`find` has just listed can always be dated. A stub `stat` earlier on PATH forces
each case, same file and same command throughout, only `stat` changing:
- a BSD stat (rejects `-c`, answers `-f %m`) must still refuse a live task and
still allow a stale one. The second half is what proves the probe rather than
a blind `-c || -f` fallback: on Linux `stat -f` is --file-system and
SUCCEEDS, so a fallback would date the mount and read the stale file as live.
- a stat that answers nothing must refuse.
Red controls, both run:
against the merge base da91f3f - 2 of the 4 fail, including the live-task
case, which is the macOS bug itself.
against this branch's head 5dddcb1 - the undateable case fails, which is the
defect fixed above.
bin/test-check-agent-hooks.sh: 158 assertions, exit 0.
DEFECT CLASS SWEPT. `set -e` plus an assignment from a substitution that is
EXPECTED to fail is the shape. The two sibling guards that gained the same
`_mtime` block, bin/check-pr-ready.sh and
.claude/hooks/remind-inflight-on-push.sh, both run under `set -uo pipefail` with
no `-e`, so neither can take this path - checked, not assumed. No other site
found.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… that had already drifted Simplification pass over this branch's portability fixes. No behaviour changes; the deliberate parts - the platform probe rather than a `-c || -f` fallback, and each guard keeping its own inlined copy rather than sourcing bin/lib - are left exactly as the earlier commits argued for. .claude/hooks/remind-inflight-on-push.sh: the probe and the `_mtime` function were defined unconditionally, several lines above the throttle block that is their only consumer. On a branch's first push there is no stamp file, so the common path forked a `stat` and defined a function it never called - on a hook that fires on every push-shaped command. Moved into the `[ -f "$stamp" ]` block and inlined; a function wrapping one call site at one call site is indirection, not reuse. The throttle's fail-open direction is now stated where it is chosen: an unreadable stamp reminds rather than staying silent, which is right for a reminder and the opposite of what the two guards must do. bin/check-pr-ready.sh: the copied comment's opening line sat at column 0 inside an `if` block while its own continuation lines were indented. This is the drift bin/lib/quarantine-common.sh's header predicts for copied blocks, arriving inside the commit that made the copies - worth noting even though the reasoning for keeping them separate still holds, because comments are where it will show up. bin/check-branch-self-reference.sh: the replacement read loop used `_line`, a name no loop in this tree uses, and `[ -n "$x" ] && arr+=(...)` where the same idiom in bin/check-inflight-tags.sh and bin/check-copyright-headers.sh is `|| continue`. Renamed to `doc` and matched the house form, so the portable idiom looks like the other three places it already appears. The bash-3.2 explanation was also running straight on from the paragraph above it about untracked files; separated. Verified: bin/test-check-agent-hooks.sh (158 assertions), plus bin/test-check-branch-self-reference.sh (31), bin/test-check-pr-ready.sh (8), bin/test-check-shell-sigpipe.sh, bin/check-branch-self-reference.sh, bin/check-shell-sigpipe.sh, bin/check-issue-refs.sh, bin/check-copyright-headers.sh, and `bin/check-pr-ready.sh 341` with a live session id so the changed loop ran against real task files. All as expected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ust test the value's shape
Two defects the review pass found, both in the mtime read this branch added.
1. THE FAIL-CLOSED ARM TESTED THE WRONG THING, and the branch's own justification
for it was factually wrong.
Measured on GNU coreutils 9.7, `stat -f %m FILE` does NOT return "a number about
the filesystem" as the earlier commit and the PR body both claim. It exits 1 while
printing six lines of prose to STDOUT:
File: "probe-file"
ID: 7f048f3f1526928f Namelen: 255 Type: tmpfs
Block size: 4096 Fundamental block size: 4096
...
The conclusion the branch drew from that belief - probe the platform, never fall
back - is still right, and is more right than the reasoning behind it: a blind
`-c || -f` hands back not a wrong number but a wrong STRING. What was wrong is the
guard downstream. `[ -z "$mtime" ]` catches absent and waves garbage through to
`age=$(( now - mtime ))`, where the arithmetic evaluates the text as an
expression, `File` is a variable name, and `set -u` aborts the hook: non-zero exit,
no verdict, and PreToolUse reads that as a non-blocking error and ALLOWS the merge.
The closed arm now tests the value's SHAPE - `case "$mtime" in ''|*[!0-9]*)` -
at all three sites, so anything that is not a timestamp counts as live in the two
guards and as "remind" in the reminder. The comments carry the measured behaviour
instead of the belief.
2. THE BSD SELF-TEST WAS RED ON BSD. The stub `stat` this branch added to force
the BSD arm delegated its mtime read to the host's `stat -c %Y` - which on macOS
IS BSD stat, and rejects `-c`. So the case asserting portability failed on the one
platform the branch exists for. Reproduced here by putting a BSD-shaped `stat`
first on PATH and running the previous commit's suite: FAIL, "BSD stat: a
long-stale task still does not block (expected ALLOW, got DENY)". The stub now
models BSD rather than borrowing from the host, reading the mtime through python3,
which this suite already requires for every payload.
RED CONTROLS, all run:
new "fails but still PRINTS" case vs 2d67247 (the `|| true` fix, `-z` guard)
-> FAIL, expected DENY got ALLOW. The defect above, pinned.
the four mtime cases vs merge base da91f3f -> 3 FAIL.
previous commit's suite under a simulated BSD userland -> 1 FAIL (defect 2).
this commit's suite under the same simulated BSD userland -> 159 assertions,
exit 0. The closest thing to a macOS run available on this box; a real one
is still owed, and the PR's macOS claims rest on it.
Verified: bin/test-check-agent-hooks.sh (159), test-check-branch-self-reference.sh,
test-check-pr-ready.sh, test-check-shell-sigpipe.sh, check-shell-sigpipe.sh,
check-branch-self-reference.sh, check-copyright-headers.sh, check-issue-refs.sh,
check-file-refs.sh - all exit 0; and `bin/check-pr-ready.sh 341` with a live
session id so the changed loop ran against real task files.
DEFECT CLASS SWEPT. "A command that fails while still writing to stdout, read by a
caller that only tests for empty" is the shape. `bin/check-pr-ready.sh` and
.claude/hooks/remind-inflight-on-push.sh are the two other sites that read a
`stat`; both fixed here. Every other `$(...)` in the three scripts reads `date`,
`basename`, `git rev-parse` or `find`, none of which print on failure in a way the
caller then does arithmetic on. No further sites.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The simplify-and-review pass surfaced a seventh portability site, a factually wrong rationale in the PR body, two fixes with no executing coverage, and five checks that need a real BSD userland. None of them are closed by this PR, and all of them lived only in an agent report - which is where findings go to be lost. The xargs -r finding is the one that matters most: under BSD xargs the prior-art index does not merely shrink, it relabels notes with a false explanation. A hook that exists to stop knowledge being invisible was measured inventing a reason for hiding it.
…PR comments WHAT CHANGED `.claude/hooks/inject-branch-context.sh` emits, for the branch of the working tree it runs in: the commits between the merge base and HEAD with each body's non-empty line count, the docs/inflight/ and docs/plans/ notes only this branch has, the .worktree-owner marker, and the open PR's number, title, body size, comment count PER AUTHOR and review count. Names, counts and pointers - never bodies. Registered in .claude/settings.json on SessionStart and on PreToolUse with matcher "*", and covered by 41 new cases in bin/test-check-agent-hooks.sh. THE DIAGNOSIS AGENTS.md's inherit rule fired on one trigger - your base moved under you. The other trigger was unwritten: you were HANDED a branch. On 2026-08-24 five agents were dispatched, one per open PR, to run a simplify-then-review pass, each given the PR's changed-file list and none of its commits, body or comments. Every one of the five had a decision in its body that a simplify pass reverses on sight - #341's body argues by name against the most extractable thing in its own diff, because a guard that cannot source its helper fails OPEN. In that PR's case the decisive text was a COMMENT posted after the body, which is why this counts comments by author rather than merely noting a body exists. THE EXPERIMENT - three registrations, all measured against Claude Code 2.1.231 SessionStart -> the session, BEFORE its first tool call PreToolUse, tool Agent/Task -> the dispatcher, alongside the dispatched agent's RESULT PreToolUse, payload has agent_type -> the SUBAGENT ITSELF, alongside its first tool result The middle one is a late signal by construction and says so in its own output: a PreToolUse hook cannot alter the call it fires on, because the model composed that tool call before the hook ran. It informs dispatch N+1 and the reading of dispatch N's report, nothing more. The third is what closes the incident, and it exists because of a NEGATIVE result: SessionStart does NOT fire for an agent spawned via the Task tool - a subagent borrows the dispatcher's session_id and gets no session of its own. Without that registration a subagent could receive branch context by no route at all. Verified reachable in the other direction too: a hook keyed on agent_type had its text quoted back by the subagent, while the dispatcher in the same run saw none of it. REJECTED ALTERNATIVES - Matcher "Task" alone. The real tool_name is "Agent" (Task, Agent, Task|Agent and * all match), and more importantly a Task-only matcher misses the subagent registration entirely. - UserPromptSubmit. Fires before any dispatch, which is the right instant - but it can only describe the DISPATCHER's tree, which for a coordinator on master is nothing worth reading. - Describing the dispatcher's own cwd on a dispatch. Noise, and noise gets scrolled past. The prompt is scanned for a path whose git toplevel DIFFERS from the dispatcher's, so an ordinary in-repo file path correctly loses and a named worktree wins. - GNU timeout(1) to bound the gh call. macOS does not ship it. python3's subprocess timeout does the bound, and python also does the cache mtime check that would otherwise be `stat -c %Y`. - Letting gh resolve its own repo. A bare gh here answers confluentinc/parallel-consumer; the slug is derived from `git remote get-url origin`, which is by definition the repo the branch pushes to and is still right in someone else's fork. DEGRADED READS ARE LOUD, NEVER SHORT This hook's correct output on a boring branch is silence, which is byte-identical to being broken. So a section that cannot be built says COULD NOT BE BUILT or UNKNOWN and names the reason, rather than being omitted - and a CONFIRMED absence is reported as a fact, since gh exits non-zero for "no PR on this branch" exactly as it does for offline, and an alarm on every fresh branch is an alarm nobody reads. The self-test's negative controls cover both halves: a hook stubbed to silence reds 28 cases, and one that drops a section instead of announcing it reds the 3 loudness cases. COSTS, MEASURED 6ms on the shell bail path (every tool call it has no business with), 27ms if python starts - which is why the once-per-agent_id stamp is named after the agent_id verbatim, so the shell prologue can test for it without shasum, whose name and output differ between GNU and BSD. The gh call is bounded at 5s (a warm call measures 0.5-1.4s) and cached 10 minutes per repo and branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cts about sub-agent hooks
AGENTS.md's "Read the commits you inherit" is now "Read the record you inherit", one rule with two
triggers rather than two rules: your BASE moved (unchanged), and you were HANDED a branch - read its
own commits, its handoff note, and its PR body AND COMMENTS before you change anything. Merged
rather than added, because they are the same rule and a second entry would compete with the first
for the attention AGENTS.md's own "How to write this file" section rations. Net +13 lines.
The rule is worth having with or without the hook, which is why it is stated as a rule and the
enforcer is only named. What the enforcer cannot reach is stated too: a hook injects context into an
agent's session, and nothing can put it into a PROMPT the dispatcher writes. That residue is the
2026-08-24 failure exactly - five agents dispatched with a changed-file list and nothing else.
Renaming that heading required repairing one citation, in a dated solutions write-up that may not be
rewritten to match today's code. Per docs/citations.md the ADDRESS is repaired and the CLAIM is left
alone: the entry now names the current heading and records what it was called when the document was
written, so a reader arriving from either name lands in the right place.
docs/agent-harness.md gains the "What is wired up today" entry with the reasoning for each of the
three registration points, the honest limitation on the middle one, and the loud-degradation rule -
plus five results for "Settled by testing", every one measured against Claude Code 2.1.231 rather
than read off the documentation:
- SessionStart does NOT fire for a sub-agent. It borrows the dispatcher's session_id and gets no
session of its own, so context an agent must have BEFORE it starts cannot reach a sub-agent that
way. This is the negative result the third registration exists for.
- A hook CAN inject into a sub-agent's own context, keyed on agent_type in the payload. Verified
by a marker the sub-agent quoted back. It arrives with the sub-agent's first tool result and
repeats on every call unless throttled per agent_id.
- The Task tool's real tool_name is "Agent", and matchers are regexes - Task, Agent, Task|Agent
and * all fired for the same dispatch.
- additionalContext reaches the CALLER, not the callee, and arrives WITH THE RESULT. Verified in
both directions in one run: the dispatcher quoted the marker, the sub-agent reported seeing
none. So a PreToolUse hook on a dispatch cannot pre-empt that dispatch.
- Injected text written as an instruction gets flagged as prompt injection - reliably, with an
unprompted warning. State provenance and keep the register factual.
Two "Worth adding" entries are reconciled rather than left to rot, which AGENTS.md asks for as part
of adding: the SessionStart repo-state entry is now PARTLY done (this branch's PR and worktree; the
cross-repo view is still open), and the UserPromptSubmit review-state entry is SUPERSEDED for the
counting half while the verdict half - is any review an APPROVAL - remains open and is the half that
matters where a human LGTM is required.
Also corrects the settings.json hook count, which said five while the file registered seven, and
says why the list is shorter than the count - two hooks carry their reasoning in their own headers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 |
`bin/check-issue-refs.sh` and `bin/check-file-refs.sh` guarded with
`command -v node`. That proves node is PRESENT. It proves nothing about
whether node can RUN, and the two are not the same thing:
NODE_OPTIONS=--require=/var/folders/.../T/cmux-claude-node-options/restore-node-options.cjs
Error: Cannot find module '.../restore-node-options.cjs'
requireStack: [ 'internal/preload' ]
An agent harness writes that preload into a temp directory; the temp
directory is later cleaned up; from then on every node process on the
machine dies during preload, before one line of the gate executes. node's
exit status for a failed preload is 1 - and 1 is what both scripts reserve
for "violations found". So a gate that had checked nothing reported a
policy violation.
That is not a hypothetical. It happened, and it cost a real debugging
detour: `bin/check-issue-refs.sh` exiting 1 reads as "you wrote a bare
#NN", not as "your environment is broken", so the search starts in the
diff and not in the environment. Worse, `.githooks/pre-commit` classifies
1 as a hard block and 2 as "could not run, warn" - so the misclassification
blocked commits over a violation nobody had committed.
Both scripts already documented the right answer. `check-issue-refs.sh`
says "2 = cannot run (node missing, ...)"; `check-file-refs.sh` says the
same. The contract was correct and simply unreachable: the guard tested
the wrong property. This is the same defect class the branch it is cut for
exists to remove - a guard that fails open, or fails into the wrong answer,
rather than failing closed.
WHAT CHANGED. A shared classifier, `bin/lib/node-gate.sh`, sourced by both
scripts. Each now lets a non-zero node status reach `node_gate_verdict`
instead of dying on it, and the verdict distinguishes "node reported a
finding" from "node never reached a verdict". The message names NODE_OPTIONS
explicitly when it is set, because the failure is otherwise baffling: the
stack trace names a temp path the reader has never heard of.
THE PROBE RUNS ON THE FAILURE PATH, NOT UPFRONT, and that was the one real
design choice here. An unconditional `node -e ''` before each gate is the
obvious shape, and it costs ~90ms of interpreter start apiece, measured -
against a `.githooks/pre-commit` budget of ~1.5s for its whole run, which
these two gates already very nearly spend between them (0.59s and 0.97s
measured). Probing only after a non-zero result costs nothing on the clean
path. It is also the stronger check: an upfront probe only ever proves node
worked a moment BEFORE the run. The probe LOADS the gate module rather than
merely starting node, so a module that cannot be parsed lands on 2 as well
instead of masquerading as a finding.
Rejected: having the node payload signal "finding" with a sentinel code
(3) that node itself never produces at startup. Zero cost and rigorous, but
it fails OPEN if anyone later writes `process.exit(1)` back into the
payload - findings would silently become soft "could not run" warnings in
the pre-commit hook. The probe has no such failure mode.
VERIFIED. Before, on master's scripts with a stale --require: both exit 1.
After: both exit 2, with a message naming NODE_OPTIONS. Clean environment
is unchanged, 0 either way, and a real finding is still 1.
The regression tests were run against the UNFIXED scripts and go red on
exactly the new cases and nothing else: test-check-issue-refs.sh fails 2 of
10, test-check-file-refs.sh fails 3 of 6, and every pre-existing case stays
green. `bin/test-check-file-refs.sh` is new - the file-reference mirror had
no self-test at all, so nothing pinned its exit-code contract in either
direction; it is wired into `PR Checklist` beside its sibling, per
bin/AGENTS.md ("CI runs them BEFORE the gate they protect").
OTHER INSTANCES OF THE CLASS. Every `node` shell-out in the repo was
checked. Only these two scripts have both a node dependency and a
"violation" exit code that a startup failure could be mistaken for.
`bin/check-docs-data.sh` matches on the word "node" only as a Python AST
variable and shells out to python3, whose guard already distinguishes
"no interpreter" and "no PyYAML" as 2. The four `node .github/scripts/
*-gate.test.js` steps in pr-checklist.yml have no violation code to be
confused with - a non-zero status there is a red job either way.
`bin/check-toolchain-versions.sh` and `bin/build-client.sh`, both named as
suspects, do not exist in this repo.
…concurrent runs failed at random
FOUND WHILE ADDING TO IT, FIXED WHERE FOUND. Two cases in bin/test-check-agent-hooks.sh went red
about one run in three on a pristine master worktree, on a different case each time - the signature
of shared state rather than a bug in the thing under test. Under six CONCURRENT runs, which is
ordinary on a box hosting several agent sessions, master fails EVERY run: 2-4 failures each, across
seven distinct assertions. This branch passes all six.
Two mechanisms, both the same class:
1. The push-reminder and branch-context hooks key their throttle stamps off ${TMPDIR:-/tmp}, and
these fixtures use FIXED names (one branch name, `bctxfixture01`). So run A's `rm` of the stamp
and run B's write of it interleave, and the reminder that should have fired is throttled away.
Each section now gives the hook under test a private TMPDIR.
2. The merge-outstanding note arm wrote docs/inflight/pr-90001-selftest.md into whatever checkout
the suite ran from, then deleted it. That races the same way - and it MUTATES a tree the suite
does not own, which is the worse half. It now runs in a scratch repository, the way the
push-reminder section already did.
My own new cases had defect (1) from birth; finding it in the push-reminder cases is what exposed
it in mine. Both are fixed here rather than noted, because a flake in the file this PR adds
43 cases to would be read as this PR's.
ALSO: a local-path `origin` is no longer turned into a repository slug. `git clone /path/to/repo` -
how a scratch or baseline checkout is made - yielded `git/parallel-consumer` from the last two path
segments, and `gh` was then asked about a repo that does not exist. It failed, so the block stayed
honest, but it named a plausible wrong repository while doing so. A scheme or an scp-style
`host:owner/repo` is now required, and the absence is reported as "origin is not a remote URL".
Verified against https, https with credentials, trailing slash, scp-style, ssh:// and a bare local
path; two self-test cases pin it, paired with the case proving a real slug still resolves.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rts on bin/test-check-issue-refs.sh inherited NODE_OPTIONS from whatever environment ran it. On a machine whose ambient NODE_OPTIONS carries a stale --require, every case expecting a real verdict got "cannot run" instead, and the suite reported 8 failures that had nothing to do with the gate. Found by running it on exactly such a machine while reviewing the fix that this suite covers. The irony is the point: a harness that misreads an environment fault as a result is the same defect the gate under test exists to prevent, one level up - and it is worse here, because a red test suite reads as "your fix is broken" rather than "your environment is". Every case now sets NODE_OPTIONS explicitly - env -u where it wants a real verdict, env NODE_OPTIONS=... where it is deliberately breaking node - so the outcome depends only on the code under test. bin/test-check-file-refs.sh already did this correctly and is unchanged; this brings its older sibling into line. Verified both ways round, which is the whole claim: with a stale --require in the ambient environment both suites now pass (they did not before), and with a clean environment both still pass, so nothing regressed to achieve it.
…er as a violation Measured, not reasoned: move bin/lib/node-gate.sh aside and both check-issue-refs.sh and check-file-refs.sh exit 1 - 'violations found' - because `source` returns 1 and set -e propagates it. Exactly the class the commit above fixes, one level up, and exactly the failure the PR body cites when it argues against sharing guard code through bin/lib. Recorded rather than fixed: fix/node-gates-preflight is being actively iterated, and a one-line fix committed here would race it.
The node preflight moved the verdict logic into bin/lib/node-gate.sh and sourced it as `source A 2>/dev/null || source B`. Under set -e, both attempts failing exits with source's own status - 1 - which both gates reserve for 'violations found'. Measured by moving the helper aside: check-issue-refs.sh and check-file-refs.sh each exited 1 and accused the tree, because a helper was missing. That is the same defect the helper exists to fix, one level up, and it is the exact failure this PR's description cites when it argues against sharing guard code through bin/lib/. Sharing is still right here - these are gates, not PreToolUse guards, so a load failure is visible rather than silently permissive - but it has to fail closed like everything else on this branch. Both gates now exit 2 and name the helper. A case in each self-test moves it aside and asserts that; both cases were run against the unfixed scripts first and reported 'expected exit 2, got 1', which is the whole claim. Drops the finding from the inflight note, since it is closed.
astubbs
added a commit
that referenced
this pull request
Aug 24, 2026
Pre-existing, and nothing to do with the disk warner: the push-reminder section
cleaned up throttle stamps with `rm -f "${TMPDIR:-/tmp}"/pc-push-reminder-*` and
let the hook write them to that same shared location. Two copies of the suite
running at once therefore deleted each other's stamps mid-run.
Measured on a pristine master worktree: run serially it is green - 154 ok, 0
failures, five runs out of five. Run four at a time in one directory and all four
fail. That is why the flake stayed invisible, and it is why this PR's own
description claims 14 failures "byte-identical to the set a pristine master
worktree reports": two contaminated runs were being compared to each other. The
description needs correcting before merge.
The hook already honours TMPDIR, so this is a test-only change - the section gets
its own directory and stops competing for a machine-global name. Four concurrent
runs: 4/4 failed before, 0/4 after.
A second concurrency cause remains, in the merge-outstanding section, which
writes a note into the real working tree. That one is fixed on
#350 and is deliberately left alone here rather than
have two branches edit the same lines.
5 tasks done
One conflict, in bin/test-check-agent-hooks.sh, and both sides were wanted rather than one winning. #341 replaced `touch -d '@1000000000'` with `touch -t 200109090146` - the fifth of its GNU-only sites, and the one that hid the other four, since a harness unable to age its own fixtures makes its failures read as bugs in the scripts under test. This branch rewrote the same arm to run in a scratch repository instead of the real working tree, because the guard resolves its note from `git rev-parse --show-toplevel` and the old form both mutated a tree the suite does not own and raced any concurrent run on a fixed shared path. The two are orthogonal, so the resolution keeps this branch's isolation running on #341's portable touch. Taking either side alone would have silently dropped the other's fix.
Owner
Author
|
@claude review this |
|
Claude finished @astubbs's task in 1m 14s —— View job Review complete
Sandbox note: in my checkout, Findings1. if "://" not in url and not re.match(r"^[^/]+@[^/:]+:", url):
return NoneThe PR's stated fix is "require a scheme or scp-style form" so a bare local-path origin ( Everything else checked out:
Overall: well-engineered and heavily self-tested. The one real finding ( |
Review finding on #350. `origin_slug` asked only whether the remote URL contained `://` before trusting it, so `file:///home/astubbs/git/parallel-consumer` satisfied it and the next regex produced `git/parallel-consumer` - precisely the plausible-but-wrong slug the guard was added to eliminate, reached through a scheme instead of a bare path. `gh` is then asked about a repository that does not exist. `git clone file://...` is the documented way to force a real transport against a local repository, so this is not a hypothetical spelling of the bare-path case the guard already caught. Allowlist the four schemes that can carry a hosting slug - https, http, ssh, git - rather than requiring that any scheme be present. That closes `ftp://` in the same move, and an unknown scheme gets the accurate answer either way: there is no repository to ask about. Verified against all six forms the branch already claimed - https, https-with-credentials, trailing slash, scp-style, ssh:// and a bare local path - none of which change. Two cases added; both were run against the pre-fix guard first and reported 'expected accurate, got plausible_wrong_repo' and 'expected clean, got invented', which is the claim. The bare-path case cannot catch this one, which is why it is its own case rather than another spelling of the same one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found while babysitting #350, after the review pass reported the BSD-clean claim as holding. It grepped the same construct list this file's own header carries - inherited from #341's sweep - and `mktemp` was on neither. GNU `mktemp` invents a template when given none; BSD/macOS `mktemp` requires a template operand (or `-t prefix`) and exits 1 on none. This hook writes its payload to a temp file as its first act, deliberately, because the payload arrives on stdin and a dispatch prompt clears Linux's MAX_ARG_STRLEN. So on macOS the first line failed and the hook exited 0 having emitted ZERO BYTES. That is the exact failure the file's own DEGRADED READS ARE LOUD rule exists to forbid, and it is worse here than in any other hook: an injection hook is silent when it is working, so total inertness is byte-identical to a branch with nothing to say. Nobody would ever have reported it. Templated, with six X's - the BSD minimum, and accepted by GNU. A case fires the hook under a modelled BSD `mktemp` (modelled rather than delegating, for the reason the BSD `stat` stub above it records: on macOS the host mktemp IS the behaviour under test) and asserts it still speaks and still carries the section it would have dropped. Run against the bare form first: 'expected spoke, got silent' and 'expected complete, got truncated'. The two file:// cases from the previous commit stayed green across both controls, so neither case is standing in for the other. The construct list is corrected where it is stated, and says why: a list is only as good as the last defect that taught it something. Swept the class rather than only this site. Three further hooks open with the same bare call - inject-merge-checklist.sh, pre-commit-gate.sh, check-squash-subject.sh - and under `set -euo pipefail` they abort before emitting a verdict. Measured on a `gh pr merge --subject` that check-squash-subject.sh denies on Linux: under a BSD-shaped mktemp it prints a usage error and exits 0 with no verdict, and PreToolUse reads that as a non-blocking error. A deny gate that stops denying, on every invocation, while still registered and still green on Linux CI. Roughly twenty more sites in bin/, all fixtures. Those are left where the class is owned rather than fixed here, and recorded with the measurement in docs/inflight/pr-341-review-findings.md so the sweep that missed `mktemp` picks it up with the evidence attached. Suite 204 -> 206 assertions, 0 failures, serially and under four concurrent runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Both addressed in Finding 1 — Finding 2 — the "BSD-clean claim holds" line does not hold, and the miss is instructive (
Class sweep, since a fix that removes today's instance invites tomorrow's. Three further hooks open with the same bare call and, under Accepted without change: the Suite 202 → 206 assertions, 0 failures, serially and under four concurrent runs; |
Owner
Author
|
@claude review this — the head moved after your last pass. Please review |
…gh prose Second review pass on #350 found no defect in the scheme allowlist but flagged what stood behind the claim: "verified against eight forms" rested on ad-hoc checks. Only two spellings - the default `https://` origin and the `file://` case - actually reached `origin_slug` through `git remote`, so an edit to that regex could regress credentialed, trailing-slash, scp-style, `ssh://`-with-a-port or `git://` remotes with nothing going red. A claim a suite does not execute is a claim the next edit gets to break quietly, which is the same argument this branch already makes about a degraded read that stays silent. Seven rows, each setting the SAME repository so the expected slug is a constant and any row resolving differently is the bug: credentialed https, trailing-slash https, scp-style, ssh://, ssh:// on a nonstandard port, and git:// must all resolve; ftp:// must be rejected, because without a reject row the allowlist could quietly become "any scheme" again. Both controls run. Narrowing the allowlist to https only reddens exactly the ssh://, ssh://-with-port and git:// rows - and correctly not scp-style, which the second regex handles and which the first therefore cannot break. Broadening it back to "any scheme" reddens the ftp:// row together with the two file:// cases, which is the regression that motivated the allowlist. Suite 206 -> 213 assertions, 0 failures, serially and under four concurrent runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Second pass addressed in "Verified against N forms" now runs. Seven table-driven rows drive each spelling through Both controls run, and they partition cleanly:
On the theoretical misses you listed — Suite 206 → 213 assertions, 0 failures, serially and under four concurrent runs. |
This branch's chaos lane went RED twice while babysitting its PR. Neither red is attributable to it - the branch touches no Java at all, only agent-harness shell and docs, so the code under test was byte-identical to master's on both sides of the failure. Diagnosing it anyway, because AGENTS.md forbids waving a chaos RED away and because the answer turned out to be about the runner, not the seed. Both reds: CLASS2_STALL/LAG_STAGNATION, peak lagStagnation 154194-154296ms against a 150000ms bound, on three different seeds, with the ambient probe recording ZERO violations across its own calibrated bounds. A 2.8% overshoot twice on different seeds is the bound meeting the load rather than a schedule crossing it. The runner slots say why. The note this extends describes `Performance` and `Chaos Pain Suite` sharing the self-hosted box - two jobs, roughly 27% inflation. Measured now, with several agent sessions each pushing to their own PR, `Chaos Pain Suite` was running on ALL SIX slots at once, highcpu-1 through highcpu-6, each starting twenty-plus PC instances against its own broker. Four of six chaos jobs red, two green, in each of two windows. That strengthens the note's existing lean - stop the jobs sharing the box rather than widen the bound - rather than changing it, and adds the practical consequence: a chaos RED on any PR while sibling agents are pushing is uninformative by default. Records the one `gh api` call that checks the slots, because it costs a second and the seed replay the note prescribes costs minutes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
The It went red twice, on Not attributable here, and unusually cleanly so: this branch touches no Java at all — agent-harness shell and docs only — so the code under test was byte-identical to master's on both sides of the failure. There is no path by which it could move a chaos bound. Both reds have the same shape, and it is not the family signature:
A 2.8% overshoot, twice, on three different seeds is the bound meeting the load, not a schedule crossing it — and The runner slots say why, and the existing note understates it by a factor of three. Recorded in that note in
|
astubbs
marked this pull request as draft
August 25, 2026 02:49
astubbs
marked this pull request as ready for review
August 25, 2026 03:16
…s them This branch merged #341 at a15421f and then wrote its own bare-`mktemp` measurement into `docs/inflight/pr-341-review-findings.md` - 87 lines that exist nowhere else. #341 has since renamed that file to `ci-bsd-portability-gaps.md`, because `pr-` means "context about an open PR" and the name would lie the moment it merged. Left alone, master would gain the note under the new name while this branch still modified the old one. Git resolves that as modify/delete at best, and the realistic outcome is the measurement silently stranded in a path master no longer has - which is the failure this note is itself a catalogue of. Renamed to match. The two now collide on one file with different bodies, which is a loud content conflict a human resolves in a minute: take #341's post-merge rewrite and append the `mktemp` section below it. A conflict that shows up is the cheap outcome; the one that does not is what costs a day. No citation anywhere in the tree referenced the old path, so nothing else moves.
4 tasks done
#341 merged to master as re-cut commits, so the copy of it this branch already carried arrived a second time with different ancestry - seven add/add conflicts, none of them a real disagreement. Five node-gate files (`bin/check-file-refs.sh`, `bin/check-issue-refs.sh`, `bin/lib/node-gate.sh` and their two self-tests) taken from master: this branch never edited them, it only inherited #341's pre-Mac versions, and master's carry the fixes the Mac run produced. `bin/test-check-agent-hooks.sh` keeps this branch's scratch-repository isolation for the merge-outstanding arm - master contributes nothing at that hunk, and the isolation is why four concurrent runs of this suite are green. `docs/inflight/ci-bsd-portability-gaps.md` taken from master wholesale. This branch's copy carried two extra sections, and the Mac run falsified both: macOS `xargs` accepts `-r` (its man page says the flag exists for GNU compatibility and does nothing), and bare `mktemp` works. Master's rewrite correctly drops them and moves the class knowledge to docs/solutions/workflow-issues/gnu-only-constructs-fail-silently-on-bsd-2026-08-25.md. That leaves a live problem this merge does NOT fix, recorded here so it is not lost: this branch still contains a BSD `mktemp` stub and self-test cases built on the falsified model, plus comments asserting it. A stub that models the platform wrongly asserts a fiction indefinitely - the same failure mode the solutions doc was written about, reached by the same method.
…m where the claim lives #348 landed four lines inside the dated plan doc's `One machine` caveat, citing #347's finding that a stress detection rate is machine-dependent by 3.4x. They were honestly labelled "Added after the fact" and flagged for a decision; the decision is to take them out. `docs/citations.md`'s test is whether an edit changes the **address** of what a document refers to, or **what the document says about it**. Naming the box in the verdict line was the first kind and stays - "on this machine" was a reference a reader could not resolve. This was the second: a claim sourced from work that did not exist when the plan was written, added to a record whose value is that it says what was known on 2026-08-25. A label does not convert one into the other, and the more useful an after-the-fact addition is, the more it invites the next one. Relocated rather than deleted. `docs/solutions/best-practices/a-stress-probe-is-an-instrument-you-built-not-a-test.md` already argues that a hand-built probe's zero, rate and copied code are each worth less than they look, so the measurement belongs there as the evidence for the rate half - with the two consequences a future probe author actually needs: a bound priced on the authoring machine is not a bound on the gating machine, and where the shape allows it the deterministic model checker needs no bound priced at all. The plan doc's arm64-versus-x86-64 caveat is untouched and still correct; the solutions entry says why it is necessary but not sufficient, architecture being one source of between-box variance rather than the only one.
Merged
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 25, 2026
… 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
Brings down 18 commits, including the ShellCheck lane (406dc7d), the two pre-review commands (10acec1), and the chaos serialisation (2ccd3c7, 637ea31) that retires the contention story this branch had been leaning on. TWO CONFLICTS. `.claude/settings.json` - both sides appended a new entry to the same `PreToolUse` array, so the conflict is positional rather than semantic and NEITHER side wins: master's `check-shallow-history.sh` (matcher `Bash`) and this branch's `inject-branch-context.sh` (matcher `*`) are unrelated hooks that must both fire. Kept both, with the shallow-history guard grouped alongside the other Bash guards and the injector last, so each side's ordering intent survives. The branch's second registration under `SessionStart` was never in conflict and is untouched. `docs/inflight/test-chaos-class2-red-was-runner-contention.md` - modify/delete. Took master's DELETE. The branch had added a 2026-08-25 section asserting the six-slot contention finding; master deleted the note in 2ccd3c7 precisely because its name "asserted a conclusion the replays have since narrowed", and its replacement, `ci-chaos-lane-serialised-confirm-no-coresidency.md`, already carries the same measurement - four of six red in each of two windows, six seeds, ~154s against a 150s bound. Restoring the branch's section would both resurrect a superseded conclusion and state that fact twice. Verified before deleting: no file in the merged tree links to the old name, and the diagnosis it supported is recorded in `bug-857-family.md`'s 2026-08-25 entry and in 637ea31, which demotes the Class 2 lag bound from gating to reporting. WHAT THIS CHANGES ABOUT THIS BRANCH'S CLAIMS. A `Chaos Pain Suite` red after this merge is no longer explained by runner contention - that lane is serialised and the bound no longer gates - so any future red here is a new fact and must be reported as one, not waved away with the old story. AUTO-MERGED BUT CHECKED BY HAND, since a silent auto-merge is where damage hides. `AGENTS.md` and `docs/agent-harness.md` were each edited by both sides in disjoint regions; every added block from both sides was asserted present in the merged text. The branch renames the AGENTS.md heading "Read the commits you inherit", so the whole tree was grepped for citations of the old label - master added none, and the one live citation already records the rename. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…measured false Two assertions here stubbed `mktemp` to reject a bare invocation, modelling BSD, and checked that the hook still spoke under it. Both passed. Both were asserting a fiction. Running the sweep on a real Mac settled it: bare `mktemp` works. docs/solutions/workflow-issues/gnu-only-constructs-fail-silently-on-bsd-2026-08-25.md records it alongside `xargs -r` as one of two findings that were argued carefully and were both wrong, while the four real defects that run found were on nobody's list. The stub was modelled rather than borrowed - deliberately, so it would not inherit the behaviour under test - which is precisely what let a wrong model go unchallenged. Removed rather than left green. A passing test that models a platform incorrectly does not merely fail to protect anything: it asserts the wrong model indefinitely, and the next person to touch this file reads it as evidence about macOS. Green is the worst state for it to be in. The template on `mktemp` stays - it costs nothing and gives the file a recognisable name - but the comments no longer hang a portability claim on it, and a note in place of the deleted arm says why, so the stub does not come back. 211 assertions, 0 failures, serially and under four concurrent runs.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
Only ONE textual conflict, and three things that merged CLEANLY while becoming false - which is the part worth reading, because nothing goes red for those. CONFLICT: `.claude/settings.json`. Both sides appended a `PreToolUse` hook into the same array slot - this branch's `warn-low-disk.sh` and master's `check-shallow-history.sh` (3d51667). Resolved by keeping BOTH, as two separate matcher entries, master's first so its slot is unmoved. Neither carries an `if` filter, which is deliberate on both sides. CLEANLY MERGED BUT WRONG, all in `docs/agent-harness.md`, all fixed here: - It said the file registers "eight hooks". Master's ninth landed in a region this branch never touched, so the count merged without a murmur. It is now "nine", and `bin/test-check-agent-hooks.sh` was already asserting that number against `settings.json` - so this one WOULD have gone red, which is exactly why that assertion exists. - It called `check-history-rewrite.sh` "the one guard here that refuses". `check-shallow-history.sh` also refuses, so the sentence became false on arrival. Reworded to name both. - Master registered its hook without adding it to that doc's hook list, leaving the list one short of the count it states. Documented, pointing at the hook's own header for the reasoning, per the convention the neighbouring entries use. THE MERGE FOUND A REAL GAP IN THIS BRANCH'S META-TEST. This PR added "every registered hook is self-tested", which read coverage as "has a `$HOOKS/<name>` section in `bin/test-check-agent-hooks.sh`". Master's hook is self-tested by a DEDICATED `bin/test-check-shallow-history.sh`, so a genuinely tested hook was reported untested. That is the assertion being narrower than the property: since 1078acd, `bin/check-all.sh --with-tests` globs `bin/test-*.sh`, so a standalone self-test runs in CI with no wiring and is equally real coverage. It now accepts both shapes - and deliberately NOT a leading-comment mention, because `# Self-test for .claude/hooks/foo.sh` is prose and accepting it would let a hook buy coverage with a sentence. Two red controls, both fire: registering a hook nothing tests reports it untested (and the count arm fires too), and pointing the standalone self-test at another path re-reds the exact case that started this. Both fixtures restored byte-identically, verified by sha256. MASTER'S NEW `check-shell-hazards.sh` GATE CAUGHT THIS BRANCH'S DELIVERABLE. `.claude/hooks/warn-low-disk.sh` uses `stat -f` and `stat -c` in a platform `case`, which the gate flags because `stat -f` on GNU exits 1 while PRINTING filesystem prose to stdout. Both uses are correct - the `case` has already dispatched on `REAL_UNAME`, which is the un-injectable reading kept separate from `OS_NAME` for precisely this - so they carry the `hazard-ok:` declaration the gate prescribes, on the immediately preceding line, which is the only place it reads. `bin/check-shell-lint.sh` (ShellCheck, 406dc7d) had never run against this branch. Clean at the gated `error` floor. At `warning` it found one SC2155 in the new hook, which `docs/inflight/static-shell-lint-severity-tiers.md` ranks #1 on its work order as the cheapest floor to raise next - so it is split here rather than added to that pile. The hook is now clean at `warning`. `docs/inflight/pr-339-review-findings.md` renamed to `ci-low-disk-warner-open-items.md`. It kept the `pr-` prefix on the stated grounds that `pr-323-docs-outstanding.md` and `pr-324-tooling-outstanding.md` sat on master with their PRs merged - and master has since deleted BOTH (65cf9ec), relocating their survivors to area prefixes for the reason recorded there. The precedent that decision rested on is gone, and the four sibling PRs all renamed, so this follows. Title now names the subject rather than the PR number, matching `test-lincheck-lane-open-items.md`. Nothing cited the old path. STILL OPEN AND UNCHANGED: the throttle stamp is keyed per-UID, so it is shared across sessions and only the first of eleven agents would have been warned in this hook's own motivating incident. Fixing it means keying on `session_id`, which is Antony's call, and the note still records it as open and blocked. VERIFICATION. `bin/check-all.sh --with-tests`: 38 ran, 38 passed, 0 failed, 0 could not run (5 PR-scoped gates skipped by design). Hook suite serially: 194 ok, 0 failures. The private-`TMPDIR` fix survived - master never touched `bin/test-check-agent-hooks.sh`, so there was no conflict - and it is re-proven against a pristine-master control worktree at f7e58d7, four concurrent copies each side. Master: 4/4 runs red, with push-reminder-section failures in three of them. This branch: ZERO push-reminder failures in 4/4 runs. The residual red on two of four is `the open item is quoted into the deny reason`, which is the merge-outstanding section writing its fixture into the real working tree - a different, pre-existing cause owned by #350, outside every hunk this branch wrote, and one master fails on too. Re-run serially it is green, which is how it was confirmed as contention rather than a defect. NOTE FOR WHOEVER MERGES NEXT: the shared clone re-shallowed itself three times during this work - the `shallow` file lives in `--git-common-dir`, so one sibling agent's depth-limited fetch re-shallows every worktree. While shallow, `git merge-base` returns EMPTY and ahead-counts read in the hundreds. All figures here were taken with `git rev-parse --is-shallow-repository` asserted false on both sides of the measurement, and match ground truth: 827 commits reachable from master, behind 22, ahead 6, merge-base cf0007d. Master's new `check-shallow-history.sh` is the guard for exactly this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
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>
No textual conflict, which is the case this repo has learned not to trust: the three files both sides touch - AGENTS.md, bin/test-check-agent-hooks.sh and docs/agent-harness.md - auto-merged, so each was audited in BOTH directions (`git diff origin/master -- <file>` and `git diff HEAD -- <file>`) to confirm neither side's intent was dropped. bin/test-check-agent-hooks.sh - master's #353 added 14 assertions to the pre-commit-gate section (keyword command position, multi-line payloads, the `function` spelling); this branch's additions are the inject-branch-context.sh section at the end and the private-TMPDIR isolation in the push-reminder section. Disjoint regions, both present. Verified by name: all 14 of master's new assertions are in the merged file, and the merge-outstanding arm still builds its scratch repository rather than writing docs/inflight/pr-90001-selftest.md into the real working tree. docs/agent-harness.md - master rewrote the `check-squash-subject.sh` bullet and added two Known-gaps entries about `if` filtering; this branch added the `inject-branch-context.sh` section and the settings.json count. Both present. This branch's claim that the hook self-filters "the same shape check-squash-subject.sh uses" still holds after master's edit: that edit corrects how the squash guard finds a merge, not how it is registered. AGENTS.md - master appended the never-put-prose-in-a-shell-string rule under Commits; this branch rewrote the inherit-rule heading and added its second trigger. Different sections, both present. Deliberately NOT taken from master: nothing. Deliberately preserved against reintroduction: the removed BSD `mktemp` arm stays removed - the note at bin/test-check-agent-hooks.sh "NO BSD `mktemp` ARM HERE, DELIBERATELY" survives and no stub came back with master. Reseal assertions re-checked on the merged tree, because master could have touched either file and did not: zero occurrences of "Added after the fact" in docs/plans/2026-08-25-002-test-jcstress-poc-plain-long-visibility.md, and exactly one "A rate is a claim about one machine" section in docs/solutions/best-practices/a-stress-probe-is-an-instrument-you-built-not-a-test.md. Verification on the merged tree: bin/test-check-agent-hooks.sh 225 ok / 0 fail serially, and 225 ok / 0 fail in each of four concurrent runs. The 211 baseline plus master's 14 new arms. bin/check-all.sh 13 passed, 0 failed, 1 could not run (check-shell-lint.sh - no local ShellCheck) bin/check-all.sh --tests-only 24 passed, 0 failed, same single cannot-run shell lint run through the cached koalaman/shellcheck image with the lane's own command over the 76 tracked scripts: clean at severity error, and bin/test-check-shell-lint.sh 7/7 with that same binary on PATH branch self-reference PR_NUMBER=350 GITHUB_HEAD_REF=... - green with the PR resolved, not the bare false green bash parse sweep 79 scripts, 0 failures - but under bash 5.2. Master's new `shell: macos` lane is bash 3.2, and this branch's shell has never met it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `.claude/settings.json` entry in docs/agent-harness.md says the count is
stated because it drifted - "this said five while the file registered seven" -
and then stated a wrong count of its own: eight hook scripts across nine
registrations, against a measured nine scripts across ten registrations.
The arithmetic: the file carried eight scripts in eight registrations before
this branch, and this branch adds one script - inject-branch-context.sh - in
two registrations, SessionStart and PreToolUse. Nine and ten.
Measured rather than counted by eye, which is how the previous two were got
wrong:
python3 - <<'PY'
import json
d = json.load(open('.claude/settings.json'))
regs = [h['command'] for arr in d['hooks'].values()
for m in arr for h in m['hooks']]
print(len(regs), len({c.split('/')[-1].rstrip('"') for c in regs}))
PY
Found while auditing the master merge, since master's incoming commits touch
several hooks; settings.json itself was untouched by master, so this is this
branch's own error and not merge fallout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
Eleven commits, merge-base f7e58d7. One textual conflict, plus two things master brought down that change what this branch can claim. CONFLICT: docs/agent-harness.md, in the settings.json hook list. Both sides rewrote the same bullet's tail. Master's #353 corrected a false claim about check-squash-subject.sh - the file said the hook matches `gh` in COMMAND POSITION, and running it showed it matches a regex over the raw command, so an `echo` naming a subject-overriding merge is denied. This branch had appended its own bullet for warn-low-disk.sh immediately after that tail. Resolved by taking BOTH: master's corrected paragraph replaces the wrong claim, and this branch's warn-low-disk.sh bullet follows it unchanged. Taking either side alone would have dropped the other's whole contribution. bin/test-check-agent-hooks.sh auto-merged, and the auto-merge is correct: master added 35 cases to the pre-commit-gate section (keywords, multi-line payloads, the `function` spelling), this branch added its cases further down, and the private-TMPDIR fix in the push-reminder section is untouched. Verified by running it rather than by reading the diff. The hook-count assertion did not fire. #353 registered no new hook, so settings.json still holds nine and the prose still says nine. WHAT MASTER CHANGED ABOUT THIS BRANCH'S CLAIMS 11c6551 added a macOS lane that runs every bin/test-*.sh under BSD userland and bash 3.2. This branch's deliverable is a shell hook with `stat -f`/`stat -c` platform branching whose BSD arm the inflight note records as never having run, so that gap now has a CI answer for the first time. Recorded in the note. 3a98b8d/957d08423 added a shared-git-state hazard row and taught check-shell-hazards.sh to join backslash continuations and to see wrapper command words. The two `hazard-ok:` declarations on warn-low-disk.sh's platform `case` still satisfy it - the gate passes. VERIFICATION ON THE MERGED TREE bin/check-all.sh --with-tests 39 ran, 37 passed, 0 failed, 2 could not run (check-shell-lint.sh and its self-test: ShellCheck is not installed here) shellcheck run through the cached koalaman/shellcheck image with the lane's own command over all 76 tracked scripts: clean at the `error` floor, and clean at `warning` for this branch's files check-branch-self-reference.sh green with PR_NUMBER=339 and GITHUB_HEAD_REF=ci/warn-low-disk-hook, which is the only way it checks anything hook suite, serial 208 ok, 0 failures (master alone: 173 ok, 0 failures - the same 35-case delta as before) CONCURRENCY, MEASURED AGAINST A PRISTINE-MASTER CONTROL Four concurrent copies, this branch: 3/4 red, all on one arm. Four concurrent copies of a detached origin/master worktree: 4/4 red, on that same arm AND on five push-reminder assertions. So the private-TMPDIR fix survived the merge and still works - zero push-reminder failures on this branch against five on master. The arm that remains red is check-merge-outstanding-work.sh's note fixture, which writes docs/inflight/pr-90001-selftest.md into the real working tree at a fixed path; it is pre-existing on master, untouched by this branch, and already fixed on #350, which this branch's own inflight note and PR body both name as its owner. Deliberately not fixed here: two branches editing those lines is the conflict the note exists to prevent.
…Command deadlock This branch's own chaos run tripped the correctness SLO "no instance may end the run with an unclassified failure cause" with a commit-response timeout - the third time that signature has appeared, but the first time the autopsy captured the broker-poll thread's STATE at the timeout. It was BLOCKED on a monitor, not running and not waiting on a socket, and the dump names both the lock and its holder. That is the experiment the two 2026-08-25 sections in the note pre-declared and could not run: each closed with "do NOT read either as identifying the open deadlock", because a poll thread merely slower than offsetCommitTimeout produces an identical error text, and nothing in either capture separated the two readings. This capture separates them, and lands on the side those sections called the deadlock. The frames resolve, at the head under test, to the exact AB-BA pair #29 replaces with ReentrantLock.tryLock(): the poll thread stopped ON the synchronized(commitCommand) line inside commitOffsetsThatAreReady, reached from onPartitionsRevoked, while the control thread held that monitor across committer.retrieveOffsetsAndCommit() - a call awaiting a response only the poll thread can produce. Every synchronized block in AbstractParallelEoSStreamProcessor takes commitCommand and it is the only AtomicBoolean among them, so the logged lock has one candidate. PT10S is what breaks the cycle rather than anything resolving it. Neither the branch nor the shared box is a suspect: at the observed head this branch carried hook script, shell and markdown only - no Java, pom or workflow - and other PRs' chaos jobs in the same window completed green, with the non-successes cancelled by the box-exclusive queue rather than failed. So the red is not master-state and not PR-introduced. Recorded with the replay seed before the CI log expires, as one observation rather than a rate. What is wanted next is a verification, not a discriminator: replay the seed with the #29 stack applied and read whether the poll thread is still found blocked on the same monitor. That would let #29 land on fleet-level evidence instead of a unit reproduction alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5LgTetnPkQciHFNaQWwSN
This branch renames the `AGENTS.md` section "Read the commits you inherit" to "Read the record you inherit", because the rule gained a second trigger. `AGENTS.md` requires a repo-wide grep before any labelled block moves, and names the incident that rule came from - a moved bold label left six files pointing at it. The grep found one citation here. Left alone deliberately: the `docs/solutions/` write-up keeps the old wording in its frontmatter title and H1. That is a dated record, which `docs/citations.md` forbids rewriting to match today's tree, and this branch already added the line there explaining the rename instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5LgTetnPkQciHFNaQWwSN
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
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.
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
Three conflicts, all from #350 landing the branch-context hook into the same three files this branch touches. Resolutions, and why: `.claude/settings.json` - both sides appended a PreToolUse entry at the same position. Kept both, so `warn-low-disk.sh` (matcher `Bash`) and `inject-branch-context.sh` (matcher `*`) are each registered. Validated as JSON, and the registration self-test confirms all ten scripts are wired and covered. `bin/test-check-agent-hooks.sh` - both sides had independently given the push-reminder cases a private TMPDIR for the same reason, under different names. Took master's `PUSH_TMPDIR`, which is what the surviving declaration outside the conflict uses; this branch's `push_tmp` would have been an undefined reference under `set -u`. That left master's block and this branch's near-identical explanatory comment both present - one fact stated twice, which AGENTS.md forbids because the copies drift - so the duplicate is deleted and the one datum only it carried is grafted into the survivor: the specific case that failed, "an immediate second push is throttled" reporting `repeated`. `docs/agent-harness.md` - a genuine cross-branch interaction rather than a textual clash. This branch added a gate pinning the doc's hook count; master then reworded that same sentence to distinguish hook SCRIPTS from REGISTRATIONS, which the gate's regex could not match. Kept master's wording, since it explains why the count is stated at all and the two numbers legitimately differ - one script can be registered against several events - and widened the gate to parse both and verify both against `settings.json`. Verified by making each number wrong in turn and watching the gate name it. The doc now reads ten scripts across eleven registrations, counted from the merged file rather than by incrementing master's nine and ten. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5LgTetnPkQciHFNaQWwSN
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…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>
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
…sk the wrong repository (#364) 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; or the lookup asked the WRONG REPOSITORY. `2>/dev/null || true` discarded both the error and the exit status, so absence and failure were the same value. The third cause is specific to this fork and the most damaging. None of these calls carried `-R`, and `gh` prefers `upstream` unless `remote.origin.gh-resolved` is set - a local, uncommitted config, so CI runners and fresh agent sandboxes all start without it. AGENTS.md has a section on exactly this, and calls the silent wrong-repo success the case that hurts. Not hypothetical: hit twice in one session on #356, from two different causes, both times asserting "nothing could be measured" without saying why. Each lookup now derives the repository slug from `origin`, runs bounded, and reports which of four states it is in: found; measured absence; failed, quoting what `gh` actually said; or did not run because the slug could not be derived - in which case `gh` is deliberately NOT called rather than called unqualified. Every guard stays fail-closed. This changes the message, never the verdict. The same defect is fixed in `remind-inflight-on-push.sh`, where it was live-dangerous - an upstream PR number would have matched this repo's `docs/inflight/pr-<n>-*.md` and quoted an unrelated note - and in `check-merge-outstanding-work.sh`, which keeps its deliberate fail-open and merely stops being fail-SILENT. A class sweep found a fourth instance in `bin/check-pr-ready.sh`, which could report a stranger's PR as this branch's. Checked and ruled out: `check-branch-self-reference.sh` and `check-pr-analysis-surfaces.sh` already qualify every call; `check-squash-subject.sh`, `pre-commit-gate.sh`, `inject-merge-checklist.sh` and `inject-recorded-knowledge.sh` make no `gh` calls at all. A simplification pass then found the branch's own defect one level down. The lookup contracts to answer found/failed/none, but two hooks dispatched on only the first two, so an EMPTY answer - interpreter killed, or a `BaseException` their `except Exception` cannot catch - matched neither test and fell through to silence, exactly where "none" falls through. `check-history-rewrite.sh` already carried the backstop the other two lacked. Both now route an unrecognised answer to their existing advisory. It also overlapped two independent `gh` calls that had been waiting on each other: measured 1.85s concurrent against 2.34s sequential, non-overlapping across three runs, the gap equal to one round trip. The lookup is now written four times, and the reasoning is narrower than "avoid shared helpers". #341 draws the line at guards versus gates: "Guards are inlined; gates share through `bin/lib/`", because sourcing a helper adds a failure mode where the helper cannot be found, and a guard that cannot load its dependency fails OPEN - four repeated lines beat a new way to pass silently. #357 landed `.claude/hooks/lib/hook-common.sh` while this branch was open, and drew that line in code rather than in prose, which moves it. Its two non-blocking reminders source the library behind `[ -r "$hook_lib" ] || exit 0` - fail-open, and harmless for something that only advises - while every hook that actually refuses a tool call still inlines. The split is therefore refusing-guard against advisory-reminder, not hook against gate, and all of these are `PreToolUse`, so the event type does not tell you which is which. Two of the four copies are free to move on that basis: `remind-inflight-on-push.sh`, which is advisory and already sources that library, and `bin/check-pr-ready.sh`, a gate where a load failure is visible rather than silently permissive. The two refusing guards stay inlined unless someone deliberately changes their failure mode with a fail-closed loader. `docs/inflight/ci-pr-lookup-is-copied-into-three-hooks.md` records that, along with the reason `bin/check-quarantine-owners.sh`'s older slug derivation must not be copied: it predates the `file://` fix and would ask GitHub about a repository that does not exist, reintroducing the very class this closes. The pattern was adopted from #350 rather than invented - origin-derived slug, bounded in python3 rather than GNU-only `timeout(1)`, no swallowing. One departure: these use `gh pr list --head`, which exits 0 printing nothing when there is no PR, so absence is read off the exit status and #350's stderr regex is unnecessary. An independent review raised three defects, all fixed, and overstated a fourth. It said an unparseable gate "never fails the sweep"; measured, `check-shell-lint.sh` runs ShellCheck over all of `bin/` at severity=error and an unterminated `if` is SC1072/SC1073, so the sweep normally does go red. The residual hole is narrower and real - with ShellCheck absent the linter itself exits 2, both skips agree, and the sweep reports zero failures - so `bash -n` now runs first and a parse failure is a FAIL naming the broken gate. The code carries the measured claim, not the review's. A second review then found the branch's remaining gap, and it was this same subject one level further out: `bin/check-pr-ready.sh` received the fix above and shipped with no behavioural coverage at all. Its self-test had eight cases and every one grepped the script's own source text, so they passed identically before and after a behavioural change - nothing exercised the new failure messaging, nothing checked that `-R` reached the `gh` argv, nothing proved `PR_READY_REPO` overrode anything. `bin/AGENTS.md` states the bar that missed: a regression test that has never failed proves nothing. Twelve cases now run the script against a stubbed `gh` that records its own argv, each in a throwaway repository on a named branch. Restoring master's pre-fix script fails 10 of them; the 2 that survive are invariants, so they are controls rather than coverage. The new self-test arms come in pairs, and the pairs are the point: only the arms that CAN flip are red controls, the rest are fail-open invariants that must pass either way by construction. Proven red against the pre-fix hooks rather than merely passing - restore the three hooks from master with the new arms in place and 12 fail, the 10 that pass being the invariants. Those figures are re-measured against this branch's final state after merging master; an earlier draft of this message carried numbers that had drifted from an earlier shape of the branch, which is this change's own subject turning up in its own commit message. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
depends on #341
#341 is merged into this branch, not cherry-picked: both touch
bin/test-check-agent-hooks.sh, and the one conflict needed both sides rather than a winner. #341 made the fixture-ageingtouchportable (touch -d '@…'is GNU-only, and it was the site that hid four others); this branch moved the same arm into a scratch repository, because it otherwise mutates the real working tree and races any concurrent run. The resolution keeps this branch's isolation running on #341's portabletouch.Verified after the merge and after review: 213 self-test cases green, four concurrent runs in one worktree all green, #341's own node-gate self-tests still passing here, and seven repository gates clean.
It serves no single issue, so it cites none.
Description
An agent handed a branch has no idea what that branch decided.
AGENTS.md's inherit rule fired onone trigger only - your base moved under you - and the other one was never written down: you were
handed a branch, read its own record.
On 2026-08-24 five agents were dispatched, one per open PR, to run a simplify-then-review pass. Each
got the PR's changed-file list and none of its commits, handoff note, body or comments. All five PRs
had a decision in the body that a simplify pass reverses on sight -
#341's body argues by name against extracting the most extractable thing
in its own diff, because a guard that cannot source its helper fails OPEN. In that case the decisive
text was a comment posted after the body, a ride-along scope addition; so this counts comments
per author rather than merely noting that a body exists.
.claude/hooks/inject-branch-context.shemits, for the branch of the working tree it runs in:HEAD, each with its body's non-empty line count (the count isthe signal - bodies are load-bearing here, since release notes are generated from the log);
docs/inflight/anddocs/plans/notes only this branch has - how a handoff doc announcesitself;
.worktree-ownermarker;author and state, and the
gh pr view --commentscommand to read them.Names, counts and pointers - never bodies. Same cheapness contract
inject-recorded-knowledge.shstates, and for the same reason: the failure being fixed is not knowing the record exists.
Three registrations, all measured against Claude Code 2.1.231
SessionStartPreToolUse, toolAgent/TaskPreToolUse, payload carriesagent_typeThe middle row is a late signal by construction, and the emitted block says so instead of
implying the dispatch was vetted: a
PreToolUsehook cannot alter the call it fires on, because themodel composed that tool call before the hook ran. It informs dispatch N+1 - four further agents, in
the incident - and the reading of dispatch N's report.
The third row is what actually closes the incident, and it exists because of a negative result:
SessionStartdoes not fire for an agent spawned via the Task tool. A subagent borrows thedispatcher's
session_idand gets no session of its own, so without this registration a subagentcould receive branch context by no route at all.
Both
PreToolUserows are one registration withmatcher: "*", self-filtering on the payload - thesame shape
check-squash-subject.shuses. ATask-only matcher would miss the subagent row.Five things measured, now in Settled by testing
Every one run against a live
claude -p, not read off documentation:SessionStartdoes NOT fire for a sub-agent - onestartupevent for the whole run, and thesub-agent's own tool calls carried the parent's
session_id.agent_type- the sub-agent quotedthe marker back.
tool_nameisAgent, and matchers are regexes:Task,Agent,Task|Agentand*all fired for the same dispatch.additionalContextreaches the caller, not the callee, and arrives with the RESULT - verifiedin both directions in one run.
unprompted. State provenance; keep the register factual.
Degraded reads are LOUD, never short
This hook's correct output on a boring branch is silence, which is byte-identical to being
broken, unregistered or absent. So a section that cannot be built says
COULD NOT BE BUILTorUNKNOWNand names the reason rather than being omitted - and a confirmed absence is reported asa fact, because
ghexits non-zero for "no PR on this branch" exactly as it does for offline, and analarm on every fresh branch is an alarm nobody reads.
That is a measured incident in this repo, not a hypothesis:
inject-recorded-knowledge.shusesGNU-only
xargs -r, and under a BSDxargsits Registers section drops from 13 entries to 4 whileclosed notes get relabelled as mis-tagged. That defect belongs to #341's
class and is deliberately not fixed here - flagged so it can be routed.
Costs and boundaries
agent_idstamp is named afterthe id verbatim so the shell prologue can test for it without
shasum, whose name and outputdiffer between GNU and BSD.
ghbounded at 5s (a warm call measures 0.5-1.4s), cached 10 minutes per repo and branch.The bound is python's
subprocesstimeout, nottimeout(1)- that is GNU coreutils and absenton macOS.
git remote get-url origin, never left togh: a bareghhereanswers confluentinc/parallel-consumer, and the damaging case is the command that succeeds
against the wrong repository. Hardcoding the fork would be wrong in somebody else's fork.
stat -c,mapfile,readarray,grep -P,date -d,readlink -f,touch -d,xargsortimeout(1). It was not clean from line one: see Two findings from reviewbelow. The construct list is corrected in the file's own header, where it is stated.
mktempisnot on that list, and the reason is a correction rather than an omission - see below.
Self-test
54 new cases in
bin/test-check-agent-hooks.sh, every fixture pinned - a self-test for abranch-context hook must not be a function of which branch it runs on, and CI checks PRs out
detached, which this hook deliberately stays silent for.
Most cases force the hook to speak; every asserted-silent case (on master, detached, outside a repo,
unparseable payload) is paired with a forced case proving the same call path can still talk.
Negative controls, both run:
exit 0(broken / unregistered / absent)://-only remote guardhttpsonlyghdrops the section instead of announcing itSuite: 154 → 213 assertions, 0 failures, serially and under six concurrent runs.
A pre-existing flake, found while adding to the suite and fixed where found
Two cases went red about one run in three on a pristine master worktree, on a different case
each time. Under six concurrent runs - ordinary on a box hosting several agent sessions - master
fails every run: 2-4 failures each, across seven distinct assertions. This branch passes all
six.
${TMPDIR:-/tmp}with fixed fixture names, so one run'srmand another's write interleaveTMPDIRdocs/inflight/pr-90001-selftest.mdinto the real working tree, then deleted itThe second is the worse half: it mutates a tree the suite does not own. My own new cases had the
first defect from birth - finding it in the push-reminder cases is what exposed it in mine. Fixed
here rather than noted, because a flake in the file this PR adds ~47 cases to reads as this PR's.
This is very likely what #339's body recorded as "~14 pre-existing
failures". A pristine
origin/masterclone atda91f3f61run serially is green: 154assertions, 0 failures. The failures appear only under concurrency.
One more defect, self-found - and its sibling, found in review
A local-path
origin-git clone /path/to/repo, how a scratch or baseline checkout is made -produced the slug
git/parallel-consumerfrom the last two path segments, andghwas then askedabout a repository that does not exist. It failed, so the block stayed honest, but it named a
plausible wrong repository while doing so. A scheme or scp-style
host:owner/repois now required - and, after review, an allowlistedscheme: see Two findings from review below. Verified against https, https-with-credentials,
trailing slash, scp-style,
ssh://,file://,ftp://and a bare local path.Two findings from review, both fixed here
A
file://origin walked through the remote guard (9e17b35c7). The guard below asked onlywhether the URL contained
://, andfile:///home/astubbs/git/parallel-consumersatisfies it - sothe very local clone it exists to reject produced the same
git/parallel-consumerslug, reachedthrough a scheme instead of a bare path.
git clone file://...is the documented way to force a realtransport against a local repo, so this is not a hypothetical spelling. Fixed by allowlisting the
four schemes that can carry a hosting slug (
https,http,ssh,git) rather than requiring thatsome scheme be present, which closes
ftp://in the same move. All six forms previously verifiedare unchanged.
A bare
mktempwas reported as making the hook silently inert on macOS, and that finding waswrong. It is kept in this description rather than quietly dropped, because the way it was wrong is
the more useful half.
The claim was that GNU
mktempinvents a template when given none while BSD/macOS requires one andexits 1, so
payload_file=$(mktemp)- line one of the real work - would leave the hook emitting zerobytes and exiting 0. It was measured against a modelled BSD
mktempstubbed on Linux, and themodel was false: running the sweep on a real Mac showed bare
mktempworks.docs/solutions/workflow-issues/gnu-only-constructs-fail-silently-on-bsd-2026-08-25.mdrecords italongside
xargs -ras two findings argued carefully and both wrong, while the four real BSD defectsthat run did find were on nobody's list.
So the two assertions built on that stub have been removed (
706253497), along with thecomments that hung a portability claim on the template. A passing test that models a platform
incorrectly does not merely protect nothing - it asserts the wrong model indefinitely and reads later
as evidence about macOS. The template itself stays, because it costs nothing and names the file
recognisably; no claim rests on it, and a note where the deleted arm was says so, so the stub does
not come back.
What survives from the episode is the review observation, which was sound: the BSD-clean claim had
been confirmed by grepping the construct list this file's own header carries, inherited from
#341's sweep - so an inherited checklist certified the gap it did not cover.
The class, swept rather than the site patched. Three further hooks open with the same bare call
and, under
set -euo pipefail, abort before emitting a verdict. Measured on agh pr merge --subjectthat
check-squash-subject.shdenies on Linux: under a BSD-shapedmktempit prints a usage errorand exits 0 with no verdict, which
PreToolUsereads as a non-blocking error - a deny gate thatstops denying, on every invocation, while still registered and still green on Linux CI.
pre-commit-gate.shandinject-merge-checklist.share the same shape; roughly twenty more sites inbin/, all fixtures. Left where the class is owned rather than fixed here, and recorded with themeasurement in
docs/inflight/pr-341-review-findings.md.A third finding, from the second review pass (
0a1a2b793): no defect in either fix, but the"verified against N forms" claim rested on ad-hoc checks - only the default
https://origin and thefile://case actually reachedorigin_slugthroughgit remote, so an edit to the scheme regexcould regress credentialed, trailing-slash, scp-style,
ssh://-with-a-port orgit://remotes withnothing going red. Seven table-driven rows now drive each spelling through the real path, plus an
ftp://reject row so the allowlist cannot quietly become "any scheme" again. Both controls run andpartition cleanly: narrowing to
httpsonly reddens thessh://,ssh://-with-port andgit://rows and correctly not scp-style; broadening back reddens
ftp://together with bothfile://cases.
Also here
AGENTS.md- "Read the commits you inherit" becomes "Read the record you inherit": one rule,two triggers, merged rather than added, net +13 lines. The rule is worth having with or without the
hook, and what the hook cannot reach is stated - nothing can put context into a prompt the
dispatcher writes, which is the 2026-08-24 residue exactly.
docs/solutions/write-up: perdocs/citations.mdthe addressis fixed and the claim left alone, and the old heading is recorded so either name lands right.
Worth addingentries reconciled rather than left to rot: theSessionStartrepo-state entryis now partly done, and the
UserPromptSubmitreview-state entry is superseded for thecounting half - the verdict half, is any review an approval, is still open and is the half that
matters where a human LGTM is required.
settings.jsonhook count indocs/agent-harness.md, which said five while the fileregistered seven.
Expected conflict
.claude/settings.jsonalso changes on #339. Cheap to resolve; it did notshape the design.
Three doc corrections that ride along
Outside the title's scope, and visible in the diff, so they belong here rather than surfacing as unexplained changes:
docs/plans/2026-08-25-002-test-jcstress-poc-plain-long-visibility.mdloses four lines. test(jcstress): a probe module - the plain-long residual is real on hardware, and the commit path errs safe #348 had added test(core): a Lincheck lane, calibrated by refinding four real races unaided #347's machine-dependence datum into that dated plan'sOne machinecaveat, honestly labelled "Added after the fact".docs/citations.md's test is whether an edit changes the address of what a document refers to or what it says: naming the machine in the verdict line was the first kind and stays, but importing a later finding is the second, and a label does not convert one into the other. Stripped, so the record says what was known on the day it was written.docs/solutions/best-practices/a-stress-probe-is-an-instrument-you-built-not-a-test.mdgains that datum instead, as "A rate is a claim about one machine" - 3.4x apart, 0.69% against 2.33% per iteration, likelihood-ratio 6.42 on 1 df, p = 0.011. That doc already argues a hand-built probe's zero, rate and copied code are each worth less than they look, so the measurement is the evidence for the rate half, with the two consequences a future probe author needs: a bound priced on the authoring machine is not a bound on the gating machine, and where the shape allows it the deterministic model checker needs no bound priced at all.docs/solutions/workflow-issues/read-the-commits-you-inherit-2026-08-10.mdhas a citation repaired, since this PR renames the heading it pointed at.Checklist
docs/agent-harness.md(wired-up entry, five Settled-by-testing results, twoWorth-adding entries reconciled) and
AGENTS.md(the rule).docs/features/- N/A - agent tooling, nouser-facing library behaviour changes.
ce-simplifyandce-code-reviewlocally - both, at the start of this work;ce-code-review's findings are thefile://-origin guard and the table-driven remote-spelling rows. A later@claude reviewround and a Codex round each found more, all addressed.inject-branch-context.shsection ofbin/test-check-agent-hooks.sh, with two negative controls; the suite as a whole reports 225 ok / 0 fail serially and under four concurrent runs (211 before this branch took master's 14 new pre-commit-gate arms). Counted from the suite's own output rather than by handrun and recorded above, plus a pre-existing concurrency flake diagnosed and fixed.