Repository navigation
tooling(agents): make the repo's conventions fire instead of hoping they are read - #299
Conversation
… read Every rule this repo has was already written down, correctly, before this commit - and they were still missed, because a document only fires when somebody chooses to open it. The trigger was an agent adding a docs/inflight/ note describing work its own PR was landing, which docs/inflight/AGENTS.md forbids in its first rule. The rule existed, was correct, and was linked from root AGENTS.md. It was simply never read. The root cause is mechanical, and worth stating plainly because it was guessed at wrongly three times before being checked: Claude Code reads CLAUDE.md and NEVER reads AGENTS.md (verified against 2.1.223). So every AGENTS.md rule in this repo has been opt-in reading, and the routing tables pointing at them only help an agent that already decided to look. Four layers, cheapest first: - CLAUDE.md at the root imports @AGENTS.md, so the conventions load every session instead of when someone remembers. - bin/CLAUDE.md and docs/inflight/CLAUDE.md do the same for their nested AGENTS.md. These are the interesting half: Claude Code lazy-loads a nested CLAUDE.md when it touches a file in that directory, so the rules arrive at the moment you are about to break them. That is the right-time injection a routing table cannot do, and it is native - no hook needed. - .githooks/pre-commit runs the fast read-only gates (~1.5s): copyright, issue refs, docs data, shell sigpipe, quarantine registry, action versions. It distinguishes a gate that FAILED from one that COULD NOT RUN - check-issue-refs exits 2 without node, and blocking a commit for that would teach everyone to --no-verify, taking the real violations with it. Enable with `git config core.hooksPath .githooks`. - .claude/settings.json adds a PreToolUse hook on `git commit`, running the same script. Belt and braces for a clone where core.hooksPath was never set, which is the likely state of a fresh worktree. Note what is NOT here: an attempt to inject AGENTS.md via PreToolUse. That hook cannot inject context at all - its stdout never reaches the model, it can only allow, deny or ask. The nested CLAUDE.md does that job properly. docs/agent-harness.md documents the layer map, what each layer can and cannot enforce, the two gaps I could not close (sub-agent hook inheritance is undocumented; core.hooksPath cannot be committed), and an open list of what to add next. It is explicitly meant to be extended - the point is to have somewhere to put a rule when writing it into a document is not going to be enough.
…the shared harness config Follows the CLAUDE.md bridges in the previous commit with the two things they cannot do: fire on intent rather than on location, and be shared at all. Three findings drove the shape, and two of them contradicted the plan: - .claude/* is gitignored, so the hook config could not have been shared as written. The .gitignore comment anticipates this exactly - it ignores by CONTENTS "so that shared config can be un-ignored later with a negation, e.g. !/.claude/settings.json" - so this takes that door. settings.json and hooks/ are now tracked; settings.local.json stays ignored, which is where per-machine permission grants accumulate. - A .claude/settings.json already existed, with a permissions allow-list. The hooks are merged into it rather than over it. - The merge-strategy rule ALREADY existed, in AGENTS.md "PR Discipline" - recommend a strategy, write the squash message out in full, re-cut into atomic commits. A new doc restating it would have been the duplication AGENTS.md forbids in its own rules. So the detail MOVES to docs/merge-checklist.md, which now owns it, and AGENTS.md keeps the rule plus a pointer. That is the pattern AGENTS.md prescribes for itself when over length, and it comes out 18 lines shorter. The UserPromptSubmit hook prints that doc when a prompt looks like merge prep - squash, rebase, "ready to merge", "tidy up the commits". It never blocks; the point is to inject the thought at the decision rather than leave it in a file someone might open afterwards. Matching is broad on verbs and narrow on nouns: a false positive costs a few hundred tokens, a false negative costs the thing it exists to prevent. Verified against four matching prompts and three that must not match. Neutrality is preserved by keeping the words in the doc rather than in the hook: Codex and anything else reading AGENTS.md is routed to the same file, and only the delivery is Claude-specific. The CLAUDE.md bridges are also now PURE imports. They previously carried a few invariants of their own, which was the same duplication mistake one level down - content in a CLAUDE.md is invisible to every other agent and is free to drift from AGENTS.md. A stub containing only @AGENTS.md has nothing to drift with, which is also why it beats a symlink outright: identical zero-drift, without a symlink's silent breakage on Windows checkouts where core.symlinks defaults false and the file arrives as text reading "AGENTS.md".
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 |
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
🧪🔒 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/astubbs/parallel-consumer/blob/b12d5433e27b4ee7bbf6c4b01cb10acf35940c53/.claude/worktrees/review-ba70/docs/agent-harness.md#L38-L40
The commit advertises these three bridge files as the mechanism that makes the rules load, but a repo-wide git ls-tree -r --name-only HEAD finds no CLAUDE.md paths, and the existing CLAUDE.md ignore rule still covers all three. Consequently Claude Code still never receives the root, bin/, or docs/inflight/ instructions, leaving the motivating defect entirely unfixed.
https://github.com/astubbs/parallel-consumer/blob/b12d5433e27b4ee7bbf6c4b01cb10acf35940c53/.claude/worktrees/review-ba70/.githooks/pre-commit#L34
On a fresh machine without PyYAML, check-docs-data.sh explicitly exits 2 to mean it could not run, but this empty soft-code list classifies that result as a violation. Running the new pre-commit hook in such a checkout blocks every commit with check-docs-data: PyYAML is not installed; the Claude PreToolUse wrapper also turns this into an exit-2 denial, contradicting the hook's stated policy of warning rather than blocking when dependencies are absent.
https://github.com/astubbs/parallel-consumer/blob/b12d5433e27b4ee7bbf6c4b01cb10acf35940c53/.claude/worktrees/review-ba70/.githooks/pre-commit#L54
When changes are partially staged, these scripts inspect the working tree rather than the index—for example, the issue-reference gate uses git diff <base> without --cached, while the other gates directly read current files. If a staged violation is then corrected without restaging, the hook sees the clean working copy and permits the violating index to be committed; conversely, an unstaged violation can block an otherwise clean commit. The hook needs to run the gates against the snapshot that Git will actually commit.
https://github.com/astubbs/parallel-consumer/blob/b12d5433e27b4ee7bbf6c4b01cb10acf35940c53/.claude/worktrees/review-ba70/.claude/hooks/inject-merge-checklist.sh#L38
For prompts such as reorganise the commits or reorganize the commits, this hook emits no context: the alternatives stop before the final e, while the enclosing trailing \b requires a word boundary immediately after s or z. Thus one of the checklist's explicitly advertised merge-preparation requests is missed; include the complete spellings or move the boundary accordingly.
AGENTS.md reference: AGENTS.md:L85-L86
https://github.com/astubbs/parallel-consumer/blob/b12d5433e27b4ee7bbf6c4b01cb10acf35940c53/.claude/worktrees/review-ba70/.claude/settings.json#L17
When a gate fails and a Claude Code user runs git commit --no-verify for a legitimate exception, this PreToolUse command still executes the pre-commit script before Git sees the flag and converts its failure into the blocking exit code 2. The documented bypass therefore works for humans but not for the actor this wrapper targets; the matcher or hook needs to recognize --no-verify and allow that command through.
https://github.com/astubbs/parallel-consumer/blob/b12d5433e27b4ee7bbf6c4b01cb10acf35940c53/.claude/worktrees/review-ba70/.claude/hooks/inject-merge-checklist.sh#L54-L57
These strings duplicate the checklist's two standing rules inside the delivery mechanism, despite the script's claim that it contains no advice and the checklist's declaration that it is the single owner. When docs/merge-checklist.md changes, Claude can therefore receive a stale or contradictory summary immediately before the current document; keep only a neutral introduction here and let the injected file supply the rules.
AGENTS.md reference: AGENTS.md:L59-L61
https://github.com/astubbs/parallel-consumer/blob/b12d5433e27b4ee7bbf6c4b01cb10acf35940c53/.claude/worktrees/review-ba70/.claude/hooks/inject-merge-checklist.sh#L23-L24
When a Claude session starts at the main clone and then follows the repository's required workflow by cd-ing into a task worktree, CLAUDE_PROJECT_DIR remains the session's original root, so this selects the checklist from the mutable main checkout instead of the branch being worked on. Merge preparation can consequently inject rules from a different commit—or a concurrently moved HEAD—rather than the task's own docs/merge-checklist.md; use the hook invocation's current working directory/input to resolve the active worktree.
AGENTS.md reference: AGENTS.md:L436-L440
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…chat The rule said to write the suggested squash message out in full, and did not say where. Read literally that means the conversation, which is the one place it does not belong: it is long, the author is being asked for a decision rather than a proofread, and pasting it buries the question they actually have to answer. The purpose was never the printing - it was that GitHub's default message, the PR title plus every commit subject concatenated, is a log of how the work happened rather than an explanation of what changed and why. So the message still gets written, and written properly; it goes into the merge when the agent performs it, or into the PR body when the author is merging. Chat gets the strategy and the reason in a line or two, and an offer. Delivery rule, not a weakening: nothing about what the message must contain changed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b289e87ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…, and stop a gate reading other worktrees Two things the pre-commit hook shook out, one of them by failing. A PreToolUse hook now denies `gh pr merge --subject` when the subject has no `(#N)`. `gh pr merge --squash` normally lands `... (#265)` because GitHub appends the number - but only when the subject is NOT overridden. Pass `--subject` and the text is used verbatim, so the number silently never appears. That is a good hook candidate precisely because it is invisible at the point of failure: the merge succeeds, the message reads fine, and the omission only shows up later next to its neighbours in git log. It happened on #206 and cost a force-push to master to correct, which is not a fix anyone should need. It fires only when --subject is present AND lacks the suffix, so a correct merge and a merge that does not override the subject both pass silently. Tested against three malformed spellings (quoted, single-quoted, --subject=) and four legitimate cases. The same trap is written up in docs/merge-checklist.md for anyone merging by hand, since a Claude-only hook does not bind a human. bin/lib/quarantine-common.sh now excludes .claude from its scan. It was green in a worktree and RED on a clean master in the primary checkout, reporting drift from the ~60 sibling branches under .claude/worktrees/ - annotations that are not on the branch at all, judged against a registry that is. The bug is older than this PR and was hidden by the repo's own rule never to work in the primary checkout, so nobody ran the gate there. The pre-commit hook added here does, which is how it surfaced, and it would have failed spuriously for anyone committing from the primary checkout. Verified green both ways - worktree and primary checkout - and the four quarantine script self-tests still pass, 40 tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7062ac27fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…hook gating every command A CE review ran this branch against live Claude Code 2.1.223 and found the harness did not do what it documented. Three of its findings were the harness failing its own rule 3 - "give it a negative control, make it go red on purpose before you trust it" - which this branch applied to every gate except itself. Each fix below was re-confirmed by running it, before and after. THE HEADLINE DELIVERABLE WAS NOT IN THE PR. `.gitignore` carried a bare `CLAUDE.md` rule from when those were personal scratch files, so all three bridges - `CLAUDE.md`, `bin/CLAUDE.md`, `docs/inflight/CLAUDE.md` - were ignored and untracked: `git ls-files | grep -c CLAUDE.md` returned 0 while `docs/agent-harness.md` described them as wired. They existed only in the author's worktree, which is exactly why it looked correct locally, and on merge nobody would have got the bridge at all - including the nested lazy-load the PR body calls the interesting half. The three paths are now negated individually rather than with a blanket `!CLAUDE.md`: the default should stay "a CLAUDE.md is yours, not the repo's", a blanket negation invites the drifting second copy of the rules the design argues against, and requiring a deliberate line here is the only place the "did you add the bridge?" question currently gets asked at all. THE `if` KEY WAS SILENTLY DROPPED, AND IT COST EVERY BASH CALL. `if` is honoured on the hook object, not on the matcher group. On the group it is stripped with no warning, so both PreToolUse hooks ran on EVERY Bash call - and since the gate ended `|| exit 2`, one red gate took away every Bash command in the session, including the one that would fix the gate. Sub-agents inherit hooks (verified), so the blast radius was larger still. Verified both ways with a hook that always fails: `if` on the group, `claude -p "...echo MARKER_OK"` -> "the command did not run, a PreToolUse hook blocked it"; `if` on the hook object -> `MARKER_OK`. Positive control too: a `git commit` prompt still fires the gate, so this filtered the hook rather than disabling it. THE `--subject` GUARD LET THROUGH THE EXACT MISTAKE IT EXISTS FOR. It read the FIRST `--subject` with a regex over the whole command line; `gh` honours the LAST, and the whole line includes text that is not part of the merge. Both halves were wrong in both directions - it ALLOWED a second `--subject` overriding a good one, a decoy `--subject` in an earlier command, and a `(#N)` naming a different PR (the precise #206 shape it is named after, with the right number sitting in the same command); it DENIED an escaped apostrophe and a `--subject` mentioned inside `--body`. A PreToolUse deny is hard, so those last two stopped legitimate merges with no way to argue. Now: slice at each `gh pr merge`, `shlex.split` the slice, take the LAST `--subject`, and cross-check the `(#N)` against the PR being merged. Fails open on any parse error - a hook that blocks on its own bug is worse than no hook. THE GATE IGNORED `--no-verify`, WHILE ITS OWN HEADER MADE THE BYPASS LOAD-BEARING. The inline `pre-commit || exit 2` never read the payload, so it could not see the command it was gating: an agent facing a red gate had no escape hatch, and "a gate people cannot skip when they have a reason is a gate they disable permanently" was written three lines above it. Replaced with `.claude/hooks/pre-commit-gate.sh`, which parses the payload, honours a real `--no-verify` argument (shlex, so a commit message merely mentioning the flag is not a bypass), and exits 2 with the failing gate's own output on stderr - the inline form produced "hook error: No stderr output", telling the agent it was blocked and nothing about why. A COULD-NOT-RUN GATE HARD-BLOCKED THE COMMIT. `bin/check-docs-data.sh` exits 2 for "no Python 3" and "PyYAML not installed" and 1 for violations, but had no soft code, so a contributor without PyYAML - which nothing in this repo installs - was blocked from committing anything at all. Reading the other five gates' exit codes rather than guessing turned up a third instance the review did not catch: `bin/check-copyright-headers.sh` documents "2 = cannot run (shallow clone)" in its own header and was also missing one. Both added; a seeded real violation still blocks, so neither weakened anything. THE LAYER MAP'S CENTRAL CAPABILITY CLAIM WAS FALSE. The doc and both hook headers said "PreToolUse cannot inject context - it can allow, deny, or ask, nothing else", and used it as the reason `UserPromptSubmit` was chosen. Verified live: a PreToolUse hook returning `hookSpecificOutput.additionalContext` with a marker passphrase caused the model to report it verbatim as a `PreToolUse:Bash hook additional context` system-reminder. The narrow claim is true - raw stdout is discarded - but the conclusion is not. `UserPromptSubmit` is still right for the checklist, so the justification is fixed rather than the design: it fires when the human states the intent, where PreToolUse would staple the checklist to whatever command ran next, repeatedly, and never at the decision. Likewise "assume sub-agent hooks may not fire" is now settled the other way, verified with a payload log and a sub-agent told to run one echo. Also: report a gate that is present but not executable instead of skipping it silently like a gate a branch simply does not have - a lost exec bit is a silent miss in a tool built to stop silent misses. Drop the injection hook's own summary of the checklist's two standing asks, which contradicted the "this script holds no advice of its own" claim eleven lines above it and was a second copy in the one place nobody would check for drift. Source the shared quarantine lib for the audit listing in `bin/quarantined-test.sh`, which had its own copy of the grep without the worktree exclusions, so the bug fixed in `quarantine-common.sh` still reproduced there. De-duplicate the release-notes sentence between `AGENTS.md` and `docs/merge-checklist.md`, which owns it. Give `docs/agent-harness.md` the ownership statement every sibling doc has. NEW: `bin/test-check-agent-hooks.sh`, the negative control for all three hooks, wired into repo-hygiene. It covers all ten `--subject` cases from the review plus five boundaries, the `--no-verify` hatch, the stderr forwarding and both fail-open paths. It goes red against the code this branch previously shipped - 8 failures on the old parser, 3 on the old inline gate - which is the property that makes it worth having. LEFT ALONE, DELIBERATELY: the gates judge the working tree while the commit records the index, so selective staging can both block a commit for content it is not committing and let a staged violation through. Documented in the hook header and under "Known gaps" rather than fixed - the usual remedy, `git stash push --keep-index`, can destroy uncommitted work if the hook dies mid-run. Open decision for Antony.
Only AGENTS.md conflicted, and both sides were additive to the same routing table, so the resolution keeps all four rows rather than either side. master (#206) added the `docs/inflight/AGENTS.md` row and the rule above "Where work and knowledge are recorded" - *a directory with its own `AGENTS.md` owns the rules for what goes in it*. This branch added the `docs/agent-harness.md` and `docs/merge-checklist.md` rows. The two directory-owned `AGENTS.md` rows are now adjacent and last, so they sit immediately above the paragraph that is about them, matching the table's existing habit of putting `bin/AGENTS.md` at the end. That rule and this branch are the same idea from two directions: #206 found routing was complete and got missed anyway, and the nested `CLAUDE.md` bridges here are what make a directory's `AGENTS.md` arrive when a file in it is touched instead of waiting to be opened. The rule now says so and points at `docs/agent-harness.md`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhF637Ywr7MiKkxaR11Up3
…e repo's own rule demands Review round two. The largest finding is the one this branch keeps proving it needs: `bin/AGENTS.md` says a checker fix arrives with a self-test that goes red against the old code, and the `.claude`/`.git` exclusion added to `quarantine-common.sh` shipped without one. Nothing held it - deleting or misspelling `--exclude-dir=.claude` left every test green. bin/test-check-quarantine-registry.sh is that control, and it carries its own: `previous_implementation()` is the exact pre-fix grep, and the fixture is asserted against BOTH scans, so a refactor that stops the fixture reaching the defect fails loudly instead of passing vacuously. Against the pre-fix lib it is 3 red; against this one, 12 green. It runs in quarantine-lane.yml ahead of the two checks that read the scan. Tracking `.claude/settings.json` clobbers the local one it replaces, once, and silently. Git refuses to overwrite an untracked file and overwrites an IGNORED one without a word - and that file was ignored in every clone until this branch. Reproduced in a scratch clone: a fast-forward replaced local contents with no conflict and nothing recoverable, because it was never in git. Not fixable from inside the repo (git resolves the checkout before any hook here runs), so it is written where it will be read in time - the .gitignore comment and Known gaps - with the fix being to move local grants into settings.local.json first. Two findings judged and kept as they are, now pinned by tests or comments rather than left to be re-raised: - The `--subject` guard need not put `(#N)` in the trailing slot. What is unfixable after a merge is a number MISSING or naming the WRONG PR, and both are denied - including the reviewer's own example, verified. A correct number sitting mid-subject is visible and still links right; a hard PreToolUse deny for it would block unusual-but-fine subjects with no way to argue. Both directions asserted. - The Claude-side gate reads the session's repo. The worktree hazard raised against it cannot reach it: `if: Bash(git commit *)` matches the command as written, so `cd worktree && git commit` never fires the hook, and the git hook - which git runs inside the target repository - is what covers that. Stated in the hook header.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a5d99acd9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code reviewReviewed against the PR's exact head commit ( BlockingAll three findings below are in
Non-blockingA stale citation, already wrong within this same PR. Steer-requested live verification, done against the checked-out head commit:
Everything else checked out clean: copyright headers on all new files are correct ( |
… subject we cannot resolve Review round three, all three findings in the same 25-line parser two earlier rounds already worked on. Each was reproduced before being fixed and is asserted in both directions: 9 of the new cases go red against the parser this replaces. `-t` is gh's documented short form of --subject, and the parser never read it. So `gh pr merge 1299 --squash -t "no number"` fell through the "no override, GitHub appends the number itself" branch and was ALLOWED, while gh used the text verbatim - the #206 shape the hook is named after, reached through an unhandled spelling. A parser that reads one spelling of a flag protects against one spelling of the mistake. Now `--subject`, `--subject=X`, `-t X`, `-tX`, `-t=X` and `-t` inside a shorthand group are all read, following pflag's rule that the first value-taking letter in a group takes the rest of it - which is also what keeps `-bt` from being misread as a subject. An unexpanded `--subject "$SUBJECT"` was DENIED. shlex does not expand variables or command substitution, so the hook was judging a string that is not the one gh will send, and a `(#N)` may well be inside it. That contradicted the file's own header - fail open, always - and it is the expensive direction: a PreToolUse deny is hard and the agent cannot argue with it. Every other unresolvable case already allowed. The PR cross-check only fired on a bare number, so a URL selector switched it off entirely and any `(#N)`, including one naming a different PR, was accepted. URLs now yield their number; a branch-name selector still cannot be resolved without a network call, so it keeps the documented allow, now pinned by a test rather than left implicit. Flag values are consumed so one that looks like a PR number cannot be read as the selector - the old parser denied a correct merge on `--body 1206 ... 1299 --subject "... (#1299)"`. Also: the self-test itself measured the wrong tree. inject-merge-checklist resolves its checklist from CLAUDE_PROJECT_DIR or the cwd's toplevel, so running the suite from the primary checkout pointed the hook at master's docs/ and read the missing file as "not injected". The root is now pinned from $0, so the suite tests the branch it ships with from any cwd. That is this file's own failure mode, in this file. And a stale citation the review caught: docs/agent-harness.md cited .gitignore by line number, which AGENTS.md forbids, and this PR's own .gitignore edit had already moved it. Now cited by its comment text, which greps.
|
All three blocking findings were real, reproduced before being fixed, and are fixed in Every case below was run against the hook, before and after. 1.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdbb369d83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…op the copyright gate skipping in silence Review round four. Two findings, both reproduced first, both asserted red against the code they replace. Body text could disable the guard entirely. Command boundaries were found with a regex over the RAW line, so `--body "text gh pr merge here"` matched a second `gh pr merge` inside quoted text. The two slices it then cut each had unbalanced quotes, both fail-opened, and the real `--subject` was never judged at all - a silent allow, reached by writing a sentence. Segmentation now runs over `shlex` TOKENS, where a quoted body is a single token and cannot look like a command. Searching for the defect class rather than the instance found a second one the report did not name: the same phrase in the SUBJECT (`--subject "how to gh pr merge safely"`) switched it off the same way. Both are pinned, along with a correct-subject control and an absolute path to gh, which must still match. The copyright gate was skipping in silence. On a shallow clone `check-copyright-headers.sh` prints a warning and exits 0; this loop discards the output of anything that exits 0, so the gate stopped running with nobody told - and the `:2` soft code configured for exactly that case was unreachable. The hook now asks it to hard-fail (COPYRIGHT_CHECK_REQUIRE_FORK_POINT=1), which routes the case through the soft-skip machinery it was always meant to use: exit 2, classified soft, reported as `pre-commit: skipped check-copyright-headers.sh (exit 2 - could not run)`. The commit is not blocked either way; only the silence changes. Verified against a simulated unreachable fork point, before (silent) and after (reported), with the ordinary run unchanged. A gate that stops running with nobody told is the same silent miss the NOT-EXECUTABLE branch already guards against, arriving by another door.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5805e3a950
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e hooks dying on a large payload Review round five. Five findings, each reproduced before being fixed and each red against the code it replaces - 7 new cases go red, 0 green. A merge slice ran to the next `gh pr merge`, so it swallowed everything after `&&`, `;` or `|`. An unrelated later command's `--subject` became "the last one", in both directions: a decoy could vouch for a bad merge, and a trailing `echo --subject "no number"` condemned a good one. Slices now end at a shell operator, which needs `shlex.shlex(punctuation_chars= True)` rather than `shlex.split` - the latter leaves `a;b` as one token. `fix/pull/1299` is a legal branch name, and searching every non-numeric selector for `/pull/<digits>` read it as PR 1299 - denying a correct subject that named the real PR. URL detection is anchored to an actual URL now. False positives are the expensive direction here; a hard PreToolUse deny cannot be argued with. All three hooks passed the payload as argv, which Linux caps at ~128 KiB. A prompt carrying a pasted diff or log died with "Argument list too long" before python started, and because these hooks fail open the failure was silent - the checklist simply did not appear on exactly the long prompts a human is most likely to be mid-decision on. The payload now arrives by temp file. The self-test had the same ceiling in its own fixture builder, so it would have reported the hook broken for reasons unrelated to the hook; it uses stdin now. The quarantine scan excluded only one of the two worktree roots. `.gitignore` names both, calling `/.worktrees/` "the other root in use", and an annotated file there was still scanned - the same bug as the `.claude` one with a smaller blast radius, and review caught it precisely because no fixture covered that root. Both roots are excluded and both have a fixture plus a negative-control assertion. Finally, `if: Bash(gh pr merge *)` matches only a command PREFIX, so `/usr/local/bin/gh pr merge ...` and `echo x && gh pr merge ...` never reached the guard - while the self-test asserted they were denied. They were, by the script the harness was not going to invoke for them. A self-test can only prove what a script does; whether it is reached is a separate question and needs asking separately. The merge guard now runs on every Bash call and filters itself, fronted by a grep for `merge` that settles the common case without starting python: 6.5ms per ordinary call. The commit gate keeps its `if` - it can exit 2, and it must stay prefix-matched because it gates the session's repository.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7ee92981c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…the bypass to the commit asking for it Review round six - seven findings, and most of them are the bill for round five. Registering the merge guard for every Bash call widened what the parser has to get right, and it was not right yet. Reproduced first, 10 new cases red against the code they replace. `echo gh pr merge 1299 --subject bad` was HARD-DENIED. Printing a command is not running one, and with the guard now seeing every Bash call that is a false positive an agent cannot argue with - created by the very change that made the guard reachable. `gh` is matched in COMMAND POSITION now: the start of a line, or after a shell operator, skipping `VAR=value` prefixes. `gh -R owner/repo pr merge ...` was missed entirely - valid syntax the CLI accepts, and requiring `gh pr merge` to be adjacent walked straight past it. Global flags are skipped before looking for the subcommand. A newline is a command boundary and `#` still starts a comment. Lexing the whole payload at once with newlines as whitespace and comments disabled meant a decoy on the next line, or after a `#` Bash ignores, became "the last --subject" of the merge above it. Lexing line by line restores both; a subject's own `(#N)` is safe because it is quoted. `fix (#1206) (#1299)` was allowed because the right number was present somewhere. That is exactly the ambiguity AGENTS.md exists to remove - two bare numbers with no way to tell the issue from the squash-added PR number. Any parenthesised number that is not this PR is now refused, in either order, and the deny reason says where an issue reference belongs instead. `--subject 'reduce cost to $5'` failed open with no number at all: shlex strips quoting before the check, so a literal dollar was indistinguishable from `"$SUBJECT"`. Only real expansion syntax fails open now. A single-quoted `'$VAR'` is still read as an expansion; that is the fail-open direction and is stated as a residual rather than left to be rediscovered. Two in the commit gate. `git commit -m x && echo --no-verify` bypassed a red gate for a commit that never asked - the bypass search read the whole payload rather than the commit's own arguments, so an unrelated later command granted it. And with no python3 on PATH the payload could not be parsed at all, so the hook fell through and ran the gates: `git commit --no-verify` blocked at exit 2, the escape hatch the header calls load-bearing missing on exactly the machine with no other way out. Python is not among this repo's build requirements. Both fixed; the second fails open like every other limit here. Finally, "What is wired up today" still described the `if` this branch had already removed - the same fact in two places, drifting within one PR, which is the thing AGENTS.md warns about.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c615050e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…h as a merge
Review round seven - both findings are the bill for running on every Bash
call, and both are pinned red against the parser they replace.
`echo "(" gh pr merge 1299 --subject bad` was HARD-DENIED. posix lexing
strips quoting, so an ordinary string argument holding a paren was
indistinguishable from a subshell, reset command position, and made the
text after it look like an executed merge.
Recovering the quoting is not available here: a second, non-posix lex does
NOT align token-for-token with the posix one - an escaped apostrophe splits
differently - and aligning by index would resurrect the false positive an
earlier round fixed. Verified rather than assumed. So `(` is honoured only
where a command could actually begin, which `echo "("` is not, because
`echo` has already taken the command slot. A genuine `( gh pr merge ... )`
still matches, which the previous code did not manage either.
`command gh pr merge ...` was allowed - Bash's own `help command` documents
it as running the named command, and the scanner cleared command position
on the wrapper and never looked at the `gh` behind it. Execution wrappers
that pass command position through are recognised now: command, builtin,
exec, env, nohup, time, sudo, nice, stdbuf.
343 lines of hook and 400 of test, to guard a mistake that happened once and cost a five-minute amend. Review found the parser wrong in both directions - it allowed a (#N) naming the wrong PR, and denied a subject containing an escaped apostrophe - and the fix for that was a shlex rewrite. That was the wrong response to the wrong design. The flag has no good use here. Omit --subject and GitHub uses the PR title and appends the number itself, which is what the convention wants. If the title is wrong the fix is to fix the title, not to override it at merge: the title is what reviewers saw, and AGENTS.md already asks for it to be kept in step. --body-file alone does not touch the subject, so a hand-written message still works. So the guard is now "do not override the subject" rather than "override it correctly". No parser, so no last-occurrence semantics, no PR-number cross-check, no shell-quoting edge cases, and no false-positive class to defend against. It is also strictly stronger: the version it replaces would have allowed a hand-written wrong number. 58 lines and 8 cases, from 343 and ~30.
593 lines of checker and 855 of self-test, to answer "did Antony say lgtm". It parsed code fences, blockquotes, negation forms, glued repeats, CRLF line endings and typographic apostrophes - defences against an attacker who is also the only person the check protects. The last thing the previous round was doing was widening a bracket class because a typographic apostrophe is three UTF-8 bytes. The rule as stated: a review by the owner whose body contains lgtm, any case, anywhere. That is now what the code says. The marker machinery went with it, and that is the interesting part. It existed because the workflow streamed every review as flat text - marker line, body, marker line, body - so a body could forge a segment header and mint an owner LGTM out of a stranger's comment. Hence an unguessable token per run. Filtering on .user.login with jq BEFORE any text is looked at removes the attack, so the token defends nothing and is gone. The reviews endpoint gives "a review, not a comment" for free. Behaviour is unchanged where it matters: case-insensitive, anywhere in the body, submitted reviews only, and still not head-sensitive - review state is not consulted, because the ruleset dismisses stale reviews on push and consulting state would silently un-stamp a PR the owner had already stamped. Verified against the real data rather than fixtures alone: PR #206 reads LGTM-present, #298 and #299 read absent, which is correct in all three cases. The self-test keeps the two real spellings the repo's history contains - Lgtm on #84, and the mid-sentence form ending in a question mark on #73 - plus a negative control proving the check can fail at all. 15 lines and 11 cases, from 593 and ~55.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7ca605bc6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
All four verified before and after. Three are in code written an hour earlier, when the guard was cut down; the cut went too far in two places and not far enough in one. - The SHORT FORM walked straight past it. gh pr merge --help documents '-t, --subject text', and -t is the one a hand-typed merge reaches for. A guard that knows only the long spelling has a documented way around it. - A 150 KB command made it FAIL OPEN, silently. The payload went through argv, hit E2BIG, and the hook exited having printed nothing - which reads as allow. A guard that fails open on large inputs is worse than no guard, because it looks present. It now goes through a temp file, with only the name in argv. Note the obvious alternative does not work: stdin is where the heredoc delivers the program. - --body "why --subject matters" was hard-DENIED. Substring searching the raw command line cannot tell an option from a mention. It now reads the parsed argument list of the merge command itself. - pre-commit-gate honoured a bypass belonging to a DIFFERENT commit: 'git commit -m first; git commit --no-verify -m second' exempted both, so in a clone with no core.hooksPath the first landed ungated. The bypass now needs EVERY commit in the payload to ask for it. The rule is also back to what was asked for: an override must END with (#N), rather than the flag being refused outright. Refusing it was simpler but stricter than the ask, and the flag has legitimate uses. Reading the argument list is not a return to the 343-line version. That was long because it validated the subject's CONTENT - fences, negations, cross-checking the number against the PR. Tokenising to find whether a flag is present is four lines, and it is what removes both the short-form hole and the false positive at once. The self-test gains a case per hole. Its own verdict helper also had the E2BIG bug, so the 150 KB case was killing the harness rather than exercising the hook.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ded9e43482
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… a bypass Two more from Codex, both verified before and after, both in the fixes from an hour ago rather than in the original code. -tbad. A short flag can carry its value ATTACHED, so -tbad is -t bad - Cobra accepts it and gh reaches the API with it. The previous round added -t and -t=, which is two of the three spellings. There are five in total once the long forms are counted, and the guard now knows all of them. A quoted MULTILINE commit message containing --no-verify skipped the gate. The bypass detector lexed line by line, so a message split across lines left the first line with an unbalanced quote, shlex raised ValueError, and the fallback went looking for the flag in raw text - where it found it, in the message. The whole command is lexed at once now; shlex handles the newlines and the line split was never needed. The fallback no longer reads the flag out of text it could not lex: it fails open on unbalanced quoting without pretending to have parsed anything. That second one mattered more than it looks: in a clone with no core.hooksPath the commit proceeded with no gate at all, and the trigger was a perfectly ordinary commit message that happened to mention the flag.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23830e8cc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| expect() { # <expected> <name> <command> | ||
| local got; got=$(verdict "$3") | ||
| if [ "$got" = "$1" ]; then echo "ok: $2"; else echo "FAIL: $2 (expected $1, got $got)"; fails=$((fails + 1)); fi |
There was a problem hiding this comment.
Increment failures here rather than the separate fails variable: every squash-hook assertion records failures in fails, while the final exit decision checks only failures. Consequently, a regression in check-squash-subject.sh can print FAIL throughout this section yet the CI self-test still exits 0, defeating the required negative control.
AGENTS.md reference: bin/AGENTS.md:L23-L26
Useful? React with 👍 / 👎.
| lexer = shlex.shlex(line, posix=True, punctuation_chars=True) | ||
| lexer.whitespace_split = True | ||
| tokens = list(lexer) |
There was a problem hiding this comment.
Fresh evidence after the earlier multiple-commit fix is that an actual newline is still discarded as whitespace by this lexer: git commit -m first\ngit commit --no-verify -m second is treated as one commit whose argument slice contains --no-verify, so the hook exits 0. In a fresh clone without core.hooksPath, the first commit therefore lands without requesting a bypass; retain unquoted newlines as command operators while still allowing newlines inside quoted messages.
Useful? React with 👍 / 👎.
|
|
||
| expect() { # <expected> <name> <command> | ||
| local got; got=$(verdict "$3") | ||
| if [ "$got" = "$1" ]; then echo "ok: $2"; else echo "FAIL: $2 (expected $1, got $got)"; fails=$((fails + 1)); fi |
There was a problem hiding this comment.
Increment failures here rather than the separate fails variable: every squash-hook assertion records failures in fails, while the final exit decision checks only failures. Consequently, a regression in check-squash-subject.sh can print FAIL throughout this section yet the CI self-test still exits 0, defeating the required negative control.
AGENTS.md reference: bin/AGENTS.md:L23-L26
Useful? React with 👍 / 👎.
…298) Two merged PRs, #264 and #277, carry no LGTM in any form - no review, no comment. The requirement is that the owner reviews everything that goes in, and the gap was not a lapse of intent but of visibility: "have I read this one myself yet?" is a thing to carry across a dozen open PRs, and carrying it does not work. So it becomes a check with its own name. `bin/check-human-lgtm.sh` asserts one thing: the owner has left a REVIEW whose body contains "lgtm", any case, anywhere in the body. The reviews endpoint is the rule rather than an implementation detail - "lgtm" typed into the ordinary comment box is not the deliberate act being asked for - and the endpoint gives that distinction for free. The matching rule was settled by looking rather than by taste. All 50 owner LGTMs this repo has received, across 38 PRs from #63 to #292, are COMMENTED reviews; 49 are lower-case `lgtm` and one, on #84, is `Lgtm`. Four carry a trailing clause, one of them ending in a question mark. A case-sensitive or whole-word rule would have gone red on real history, so both of those real spellings are pinned as cases. NOT HEAD-SENSITIVE, deliberately: an LGTM on any commit counts for the whole PR, permanently, because the owner only stamps a PR once it is near merge. That is why review STATE is not consulted at all - the master ruleset sets dismiss_stale_reviews_on_push, so reading state would silently un-stamp a PR the owner had already stamped. A SEPARATE JOB, NOT A SECOND STEP IN claude-review, and that is the substance of this change rather than a packaging choice. Two checks say WHICH half is missing straight from the checks list - "claude-review" red means no automated review, "review: human LGTM" red means the owner has not read it yet - and one combined check is red either way, which is exactly the question the human half exists to answer. It also leaves claude-review byte-identical to master: no rename, so no transitional duplicate job and no ordered ruleset swap, and a required check matched by name cannot be left pointing at nothing. Making the new context required is additive, once it has merged and is reporting. NO BOT EXEMPTION, unlike the automated half. A Dependabot PR does not need an AUTOMATED review, but it is still a change going in. Having no guard also means there is no job to skip - and a skipped job satisfies a required check, so a guard keyed on github.event.sender would have let a human PR synchronized by a bot go green having asserted nothing. The checker is 15 lines. An earlier revision was 593, with 855 lines of self-test, parsing code fences, blockquotes, negation forms, glued repeats, CRLF and typographic apostrophes - defences against an attacker who is also the only person the check protects. Filtering the reviews with jq on .user.login before any text is read removed both the attack and the unguessable per-run marker token it needed, since a stranger's body can no longer be attributed to the owner. Verified against real data, not only fixtures: #206 reads LGTM-present, #298 and #299 read absent. The self-test carries a negative control proving the check can fail at all.
…r they need #322 carries ten unrelated workstreams over 66 commits and 206 files; the confluentinc#909 reproduction it is named for is 13 commits and 20 files of that. Records the feasibility MEASUREMENT rather than an estimate: each group cherry-picked onto master in a throwaway worktree. Four groups replay fully clean (CI gates 8/8, #209 4/4, the confluentinc#857 ledger 5/5, test logging 1/1); the harness+tracker group blocks after one commit. The governing constraint is that cherry-pick replays a commit's ORIGINAL diff. Master gained bin/test-check-agent-hooks.sh from #299 after these commits were written, and this branch's one merge already resolved that overlap - replay throws the resolution away and re-fights it. Merging master again cannot help, and there is nothing to merge: the branch is 0 behind. What the branch being current DOES buy is that `git diff master...HEAD` describes the resolved state and therefore applies to master by construction - verified, 35/35 exclusive files clean. Hence two methods (replay where clean, diff-extraction where not) and, more importantly, a STACK rather than ten independent branches: PR 2 adds tag headers to ~70 existing inflight notes while later groups edit those same notes' bodies, so extracting independently turns every one of those into a conflict, while extracting in order applies them on top. Also corrects a grouping error - "agent hooks" and "inflight tracker" are one workstream, not two; the merge guard and the session index are the same hook system. Adds a tenth PR that is new work rather than an extraction: a conflict-marker gate, prompted by finding 305 lines of docs/inflight/bug-857-family.md committed and pushed inside an unresolved conflict on the #29 branch, with no gate in bin/ looking for markers. No history is rewritten by this commit. Re-cutting #322 itself is a force-push to a pushed branch and is left as the owner's call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
All four are prose this branch wrote or should have updated. None touches the fix. The merge-guard note claimed bin/test-check-agent-hooks.sh is referenced by no workflow. It is: repo-hygiene.yml runs it on every push, wired by #299 and on master before this branch existed. The grep behind that claim was run in the repo's MAIN CHECKOUT, which was sitting on a different branch that lacks the wiring - the exact thing AGENTS.md forbids, and it turned a one-line check into a published falsehood. The note now makes the stronger and true point: the harness runs constantly and only ever on ubuntu-latest, so it is blind to the BSD `stat` path by construction. Coverage that reports green while unable to see the defect is worse than no coverage, not better. upstream-map still said this PR bundles the confluentinc#893 cherry-pick, two entries away from the entry that says it was split out - the file contradicting itself. Commit 2b4c9a3 fixed two records of that class and missed this third one in the same file. The leak write-up narrates a declined review nit about tryToEncodeOffsets, which lives in PartitionState.java and left with the split; a reader would chase it into a diff that no longer contains it. The shared-collections note listed the files fix/concurrent-collection-sweep collides with and omitted PCMetrics.java, which that branch does touch - and this PR converts the same field to a LinkedHashSet under metersLock, so that branch's hunk there is likely redundant rather than merely conflicting. Whoever opens it was not being warned. Refs: #120, #121, #299 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176D6PxG2rKUhz7ekCh9JMF
1 participant
Description
Every convention in this repo was already written down, correctly. They still got missed — most recently by an agent that added a
docs/inflight/note describing work its own PR was landing, whichdocs/inflight/AGENTS.mdforbids in its first rule. The rule existed, was correct, and was linked from the rootAGENTS.md. It was simply never read.A document only fires when somebody chooses to open it. This PR gives the conventions mechanisms that fire on their own.
The finding that drove the design
Claude Code reads
CLAUDE.mdand never readsAGENTS.md— verified against 2.1.223, not assumed. So everyAGENTS.mdrule in this repo has been opt-in reading, and the routing tables pointing at them only ever helped an agent that had already decided to look. This repo had noCLAUDE.mdat all.What lands
CLAUDE.mdbin/CLAUDE.md,docs/inflight/CLAUDE.md.githooks/pre-commitgit commit.claude/settings.json→PreToolUsegit commit, orgh pr merge.claude/settings.json→UserPromptSubmitThe nested bridges are the interesting half: Claude Code lazy-loads a nested
CLAUDE.mdwhen it touches a file in that directory, so the rules arrive at the moment you are about to break them. That is right-time injection a routing table cannot do, and it is native — no hook involved. It is also the direct fix for the failure above.All three
CLAUDE.mdfiles are pure imports (@AGENTS.md) plus a sentence on why the file exists. That is deliberate: a stub with no content of its own has nothing to drift with, which is also why it beats a symlink — identical zero-drift, without a symlink's silent breakage on Windows checkouts wherecore.symlinksdefaults false and the file arrives as text readingAGENTS.md. Rules written into aCLAUDE.mdwould also be invisible to Codex and anything else readingAGENTS.md..githooks/pre-commitruns six fast read-only gates in ~1.7s: copyright headers, issue references, docs data, shell sigpipe, quarantine registry, action versions. It distinguishes a gate that failed from one that could not run — three of the six exit 2 for a missing interpreter or a shallow clone while reserving 1 for a real finding, and blocking a commit for that would teach everyone to--no-verify, taking the real violations with it. Bypass is deliberately easy; CI remains the authority.The
UserPromptSubmithook printsdocs/merge-checklist.mdwhen a prompt looks like merge prep —squash,rebase, "ready to merge", "tidy up the commits". It never blocks.Three things that contradicted the plan
.claude/*is gitignored, so the hook config could not have been shared as first written. The.gitignorecomment anticipates exactly this — it ignores by contents "so that shared config can be un-ignored later with a negation, e.g.!/.claude/settings.json". This takes that door.settings.local.jsonstays ignored..claude/settings.jsonalready existed with a permissions allow-list. The hooks are merged into it, not over it.AGENTS.md"PR Discipline". A new doc restating it would have been the duplicationAGENTS.mdforbids in its own rules, so the detail moves todocs/merge-checklist.md, which now owns it, andAGENTS.mdkeeps rule-plus-pointer.Review round: the harness did not apply its own rule 3 to itself
A CE review ran this branch against live Claude Code 2.1.223 and found it did not do what it documented. The common cause is
docs/agent-harness.mdrule 3 — "give it a negative control, make it go red on purpose before you trust it" — which the branch applied to every gate except its own hooks. Fixed infd9079377; every claim below was re-confirmed by running it, before and after.The headline deliverable was not in the PR.
.gitignore:4carried a bareCLAUDE.mdrule from when those were personal scratch files, so all three bridges were ignored and untracked —git ls-files | grep -c CLAUDE.mdreturned 0 while the docs described them as wired. They existed only in the author's worktree, which is why it looked correct locally; on merge nobody would have got the bridge at all. The three paths are now negated individually (reasoning in.gitignoreitself), andgit ls-tree -r HEADconfirms all three are in the commit.The
ifkey was silently dropped, and it cost every Bash call.ifis honoured on the hook object, not the matcher group. On the group it is stripped with no warning, so bothPreToolUsehooks ran on every Bash call — and since the gate ended|| exit 2, one red gate took away every Bash command in the session, including the one that would fix the gate. Sub-agents inherit hooks, so the blast radius was larger still.ifpositionclaude -p "…run: echo MARKER_OK"MARKER_OKThe
--subjectguard let through the exact mistake it exists for. It read the first--subjectwith a regex over the whole command line;ghhonours the last. It allowed a second--subjectoverriding a good one, a decoy--subjectin an earlier command, and a(#N)naming a different PR — the precise #206 shape it is named after. It denied an escaped apostrophe and a--subjectmentioned inside--body; aPreToolUsedeny is hard, so those stopped legitimate merges. Now: slice at eachgh pr merge,shlex.split, take the last--subject, cross-check(#N)against the PR being merged, fail open on any parse error.The gate ignored
--no-verifywhile its own header made the bypass load-bearing. The inlinepre-commit || exit 2never read the payload. Replaced with.claude/hooks/pre-commit-gate.sh, which honours a real--no-verifyargument and exits 2 with the failing gate's output on stderr — the inline form produced "hook error: No stderr output".docs/agent-harness.md's central capability claim was false. It said "PreToolUse cannot inject context… it can allow, deny, or ask - nothing else", repeated in both hook headers as the reasonUserPromptSubmitwas chosen. Verified live: aPreToolUsehook returninghookSpecificOutput.additionalContextwith a marker passphrase had the model report it verbatim as aPreToolUse:Bash hook additional contextsystem-reminder. The narrow claim is true (raw stdout is discarded); the conclusion is not.UserPromptSubmitremains right for the checklist, so the justification is fixed rather than the design — it fires when the intent is stated, wherePreToolUsewould staple the checklist to whatever command ran next. Likewise "assume sub-agent hooks may not fire" is now settled the other way, verified with a payload log.Smaller:
check-docs-data.shandcheck-copyright-headers.shboth document exit 2 as "could not run" and both were missing their soft code, hard-blocking a contributor without PyYAML or with a shallow clone; a gate present but not executable is now reported rather than silently skipped; the injection hook no longer prepends its own summary of the checklist (it claimed to hold no advice of its own eleven lines above);bin/quarantined-test.shnow sources the shared quarantine lib instead of its own copy of the grep without the worktree exclusions.New:
bin/test-check-agent-hooks.shThe negative control for all three hooks, wired into
repo-hygiene.yml. All ten--subjectcases from the review plus five boundaries, the--no-verifyhatch, the stderr forwarding, both fail-open paths, and the injection hook's match/no-match set. It goes red against the code this branch previously shipped — 8 failures on the old parser, 3 on the old inline gate — which is the property that makes it worth having.Verification
docs/featuresviolation (rc 1); against a seeded missing copyright header (rc 1); withpython3shimmed to 127 (rc 0, skipped … could not run); with a shallow-clone fork point (rc 0, skipped); with a gate's exec bit removed (rc 0, present but NOT EXECUTABLE); with a gate absent (rc 0, silent).test-check-*.shself-tests green.settings.json: an unrelated Bash call runs while a gate is red;git commitblocks with the gate's reason;git commit --no-verifycommits;gh pr merge … --subject "… (#206)"on a different PR is denied with the reason delivered to the model.git ls-tree -r HEADshows all three bridges tracked;git check-ignore -vshows.claude/worktreesandsettings.local.jsonstill ignored.Known gaps, documented rather than hidden
git stash push --keep-indexcan destroy uncommitted work if the hook dies mid-run. Open decision.core.hooksPathcannot be committed — a fresh clone needsgit config core.hooksPath .githooksonce (covers all its worktrees). ThePreToolUsehook covers Claude Code in that window; nothing covers a human.Bash(git commit *)matches the command as written, so it does not fire oncd sub && git commit …. The git hook covers that.AGENTS.mdhas itsCLAUDE.mdbridge. Listed under Worth adding; until then the.gitignorenegation is the only place the question is asked.Review round two: the same rule missed the same way
A second review round found six things. Three were already fixed in
fd9079377and had been reviewed against an older commit; two were judged and kept as they are, now pinned by tests rather than left implicit; one was a real gap and is the most interesting finding on the branch.The quarantine-scan fix had no negative control - again.
bin/lib/quarantine-common.shgained--exclude-dir=.claudeso a sibling worktree's annotations stop polluting this branch's registry check, and nothing held it. There were no quarantine self-tests at all, so deleting or misspelling that flag left the whole repo green.bin/AGENTS.mdsays a checker fix arrives with a case in its self-test, verified red against the old code;docs/agent-harness.mdrule 3 says the same. This is a PR about making conventions fire on their own, and it is now the second time it skipped its own.bin/test-check-quarantine-registry.shis that control, and it carries its own:previous_implementation()is the exact pre-fix grep, and the same fixture is asserted against both scans - so a refactor that stops the fixture reaching the defect fails loudly instead of the suite passing vacuously. 3 red against the pre-fix lib, 12 green against this one. It runs inquarantine-lane.ymlahead of the two checks that read the scan.Tracking
.claude/settings.jsonsilently destroys the local one it replaces. Git refuses to overwrite an untracked file and overwrites an ignored one without a word - and that file was ignored in every clone until this branch. Reproduced in a scratch clone: a fast-forward replaced the local contents with no conflict, no warning, and nothing recoverable, because the file was never in git. No hook here can prevent it; git resolves the checkout before any repo code runs, and a genuinely staged migration needs two releases. So it is written where it will be read in time - the.gitignorecomment and Known gaps - with the actual mitigation: move local grants into.claude/settings.local.jsonbefore pulling. It is strictly one-time, and a fresh clone has nothing to lose.Two findings judged and kept. Reviewer feedback is judged on merits here, not complied with:
--subjectguard does not require(#N)in the trailing slot. What is unfixable after a merge is a number that is missing or names the wrong PR, and both are denied - including the reviewer's ownfix(core): detail (#206)example, verified by running the hook. A correct number sitting mid-subject is visible in the subject the author just typed and still links right. Requiring the slot would make"port (#N) to master"a hardPreToolUsedeny that an agent cannot argue with - the exact false-positive class the previous round found blocking legitimate merges. Now asserted in both directions.if: Bash(git commit *)matches the command as written, socd worktree && git commitnever fires the hook at all; every command that does fire it is a baregit commitin the session's own cwd. Thecd'd case is covered by.githooks/pre-commit, which git runs inside the target repository - which is why the git hook is the primary mechanism. The header now says which repository it gates.Left open for the owner: the pre-commit gates read the working tree while the commit records the index. The reviewer suggested an index-backed snapshot, which is the better of the two known fixes -
git stash push --keep-indexcan destroy uncommitted work if the hook dies mid-run - but two gates diff against the fork point and need real history, and the rest are whole-tree scans, so it is a design change against a ~1.5s budget whose own failure mode is people reaching for--no-verify. That trade-off is Antony's call. The thread is deliberately unresolved.Review round three: one spelling of a flag is one spelling of the mistake
A dispatched review found three blocking defects, all in
check-squash-subject.sh's parser - the same 25 lines two earlier rounds already fixed six defects in. Each was reproduced before being fixed, and 9 of the new test cases go red against the parser they replace.-twas not read at all. It isgh's documented short form of--subject, sogh pr merge 1299 --squash -t "no number"fell through the "no override, GitHub appends the number itself" branch and was allowed, whileghused the text verbatim. That is the #206 shape this hook is named after, reached through an unhandled spelling. The parser now reads--subject,--subject=X,-t X,-tX,-t=Xand-tinside a shorthand group, following pflag's rule that the first value-taking letter in a group takes the rest of it - which is also what stops-btbeing misread as a subject.An unresolvable subject was hard-denied.
shlexdoes not expand$VARor$(...), so the hook was judging a string that is not the oneghwill send - and denying it. That contradicted the file's own fail open, always header, and it is the expensive direction: aPreToolUsedeny is hard and the agent cannot argue with it.The PR cross-check only fired on a bare number, so a URL selector switched it off and any
(#N)- including one naming a different PR - was accepted. URLs now yield their number. A branch-name selector still cannot be resolved without a network call, so it keeps the documented allow, now pinned by a test.And the self-test was measuring the wrong tree.
inject-merge-checklist.shresolves its checklist fromCLAUDE_PROJECT_DIRor the cwd's toplevel, so running the suite from the primary checkout pointed the hook at master'sdocs/and read the missing file as "not injected". The root is now pinned from$0. This file's own failure mode, inside this file - the third time this branch has done that, and the reason rule 3 exists.Review rounds four and five: the tests were true and the system was not
Two more rounds, seven more defects, all in the harness itself. Each was reproduced before being fixed and each is asserted red against the code it replaces.
Body text could switch the merge guard off. Command boundaries were found with a regex over the raw line, so
--body "text gh pr merge here"matched a secondgh pr mergeinside quoted text; both resulting slices had unbalanced quotes, both fail-opened, and the real--subjectwas never judged. Segmentation runs overshlextokens now. Searching for the class found a second instance the report did not name - the same phrase in the subject did it too.A merge slice ran past
&&,;and|, so an unrelated later command's--subjectbecame "the last one" - a decoy could vouch for a bad merge, and a trailingecho --subject "no number"condemned a good one. Slices end at a shell operator now, which needsshlex.shlex(punctuation_chars=True); plainshlex.splitleavesa;bas one token.All three hooks died on a large payload, silently. They passed it as argv, which Linux caps at ~128 KiB, so a prompt carrying a pasted diff failed with Argument list too long before python started - and since the hooks fail open, the checklist simply did not appear on exactly the long prompts a human is most likely to be mid-decision on. The payload arrives by temp file now. The self-test had the same ceiling in its own fixture builder, so it would have reported the hook broken for reasons unrelated to the hook.
fix/pull/1299is a legal branch name, and searching every non-numeric selector for/pull/<digits>read it as PR 1299 - denying a correct subject. URL detection is anchored to a real URL.The quarantine scan excluded one of the two worktree roots.
.gitignorenames both, calling/.worktrees/"the other root in use". Same bug as the.claudeone, smaller blast radius - and review caught it precisely because no fixture covered that root. Both are excluded, both have a fixture and a negative-control assertion.And the sharpest one:
if: Bash(gh pr merge *)matches a command PREFIX. So/usr/local/bin/gh pr merge ...andecho x && gh pr merge ...never reached the guard - while the self-test asserted they were denied. They were, by a script the harness was never going to invoke for them. The test was true and the system was not. A self-test proves what a script does; whether it is reached is a separate question, and it had not been asked. The merge guard now runs on every Bash call and filters itself, fronted by agrepformergethat settles the common case without starting python - measured at 6.5ms per ordinary call, which is the only reason theifwas safe to drop. The commit gate keeps itsif: it canexit 2, and it must stay prefix-matched because it gates the session's repository.Checklist
docs/agent-harness.md(the layer map, corrected against a live session and now carrying an ownership statement),docs/merge-checklist.md(new owner of the merge-strategy rule),AGENTS.md(two routing rows, detail moved out)docs/features/-N/A - contributor tooling, no runtime or API surfacebin/test-check-agent-hooks.shcovers all three hooks and runs in repo-hygiene; verified red against the previously shipped versions