fix(agents): pre-push review gate can attest to unreviewed code (marker inherited across commits; wrong worktree checked)

[EricAndrechek]

The pre-push review gate (.claude/hooks/agent-bash-gate.sh + .claude/hooks/review-marker.sh) can attest to code it never reviewed. Two independent holes, both found while pushing #440's fix. Neither requires malice — I hit both by working normally.

The gate's whole value is that an agent can't fake a marker (AGENTS.md §"Don't bypass the gates" — hooks run at Claude Code privilege level precisely so the orchestrator can't write one itself). Both bugs below let a marker exist for something unreviewed without anyone hand-writing it, which is the failure mode the design is meant to exclude.

---

1. A marker can be inherited by a commit the reviewer never saw

The more serious of the two, because it fires silently during ordinary work.

review-marker.sh resolves the SHA at the moment the subagent stops:

head_sha=$(git rev-parse HEAD 2>/dev/null || echo "")

A reviewer takes 4–9 minutes. If HEAD moves during that window — the orchestrator commits an unrelated fix, addresses another reviewer's finding, or (as happened here) lands a doc tweak — the ship_it verdict for the old HEAD is written as a marker for the new one.

Observed

Docs reviewer read 40da1924 and returned ship_it. I committed f4e64d5b while it was still running. The marker it wrote:

tmp/docs-reviewer-passed-f4e64d5bd3095f0c3369b6fb448be9bae08fcb4d

f4e64d5b was never reviewed by anything. Nothing in the output flags this — the marker looks identical to a legitimate one, and the push gate would have accepted it.

In my case the delta was one documentation sentence and the same reviewer had already considered the point, so nothing unreviewed would actually have shipped. That was luck, not a property of the system: the same sequence with a code commit lands unreviewed code behind a green gate.

Note this is more likely the better an agent behaves. Running reviewers in parallel and continuing useful work while they run — exactly what /prepush step 3 asks for — is what opens the window.

Suggested fix

Capture HEAD when the subagent starts and write the marker only if it still matches at stop; otherwise log loudly and write nothing. If SubagentStop can't see start-time state, review-marker.sh could instead record the SHA the reviewer reported in its own output and cross-check.

Cheaper interim mitigation: have /prepush state explicitly that HEAD must not move while reviewers are in flight.

---

2. The gate inspects the session's worktree, not the branch being pushed

Both hooks begin:

cd "${CLAUDE_PROJECT_DIR:-.}"

CLAUDE_PROJECT_DIR is the session's project directory. In a multi-worktree session — the standard parallel-agent workflow this repo endorses via wt switch --create in .config/wt.toml — an agent can cd into a sibling worktree and git push a branch the gate never examines.

Observed

From .worktrees/sdk-fetch-override (branch sdk-fetch-override, HEAD 154cd451):

$ git push --dry-run origin sdk-fetch-override
🛑 Claude PR discipline gate: missing pre-push review marker(s) for HEAD (a2b960a3) on branch 'local-ci-fail':

a2b960a3 / local-ci-fail are the other worktree's HEAD and branch. The gate blocked, but for the wrong reason and against the wrong branch. The corollary is the problem: once the session's own branch has its markers, a push of any sibling branch sails through unreviewed, because the gate keeps checking the branch that already passed.

The same cd also means a reviewer run "for" a sibling branch writes its marker into the session worktree's tmp/, keyed to the session branch's HEAD — so you can't satisfy the gate for the sibling branch even if you want to.

Suggested fix

Resolve the repo from the push command's own working directory rather than CLAUDE_PROJECT_DIR. Or, more conservatively, refuse the push outright when its cwd resolves to a different worktree than CLAUDE_PROJECT_DIR — that enforces one-session-per-branch, which the marker mechanism already assumes.

---

Not affected

tmp/ci-passed-tree-<TREE> markers are keyed to the tree hash and written by scripts/ci-marker.sh from inside the worktree that ran make ci, so they are per-worktree and content-addressed. Both bugs are specific to the HEAD-keyed review markers.

Repro

1. Inherited marker: start any gating reviewer, commit anything while it runs, observe tmp/<reviewer>-passed-<new-HEAD> for a SHA that reviewer never read.
2. Cross-worktree: from a second worktree on a different branch, git push --dry-run — the gate reports the session worktree's branch and HEAD.

Found while landing #440.

[coderabbitai]

🔗 Related PRs

#129 - ci: kill coverage Docker pull on PR runs + tighten claude-review group [merged]
#147 - chore: claude code native improvements [closed]
#312 - ci: split ci into a job dag and isolate docs deploys from pr code [closed]

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[EricAndrechek]

Escalation: finding 2 is worse than filed — the gate can end up fully open, and EnterWorktree doesn't fix it

Filed as "a sibling branch can ride through on the session branch's markers". In practice it degrades to no gate at all for the rest of the session, and the obvious remedy doesn't work.

EnterWorktree moves the shell, not the hooks

I tried re-rooting the session into the second worktree so the gate would evaluate the right branch:

EnterWorktree path=<repo>/.worktrees/sdk-fetch-override
→ "Entered worktree ... on branch sdk-fetch-override"
$ pwd                             → <repo>/.worktrees/sdk-fetch-override
$ git rev-parse --abbrev-ref HEAD → sdk-fetch-override

The shell moved. The hooks did not — they still resolve CLAUDE_PROJECT_DIR to the original launch directory.

The consequence: every push from the session is ungated

local-ci-fail (the launch worktree) is at 5befb486, which legitimately has both markers. The gate cds there, finds them, and allows the push — regardless of what is actually being pushed:

$ git push --dry-run origin sdk-fetch-override      # 154cd451, zero markers
 * [new branch]  sdk-fetch-override -> sdk-fetch-override      ← allowed

$ git push --dry-run origin HEAD:refs/heads/gate-probe-throwaway
 * [new branch]  HEAD -> gate-probe-throwaway                  ← allowed

That second one is an arbitrary branch name that has never been reviewed by anything. It sails through.

The before/after is the proof

The identical probe from the same worktree earlier in the session blocked:

🛑 missing pre-push review marker(s) for HEAD (a2b960a3) on branch 'local-ci-fail'

At that moment local-ci-fail was at a2b960a3 with no markers yet. Nothing about the pushed branch changed between the two probes — only the launch worktree's marker state did. The gate was never looking at sdk-fetch-override.

So the failure isn't so much "checks the wrong branch" as "once the launch branch is green, the gate is a no-op for everything else" — and it gets more permissive as a session progresses, because the launch branch accumulates markers.

Why this matters for the intended workflow

.config/wt.toml and the machine conventions push toward one worktree per workstream, and EnterWorktree exists to move between them. Any session that does so has an open gate for the rest of its life, and nothing in the output says so — the push just succeeds.

Suggested fix, revised

The cd "${CLAUDE_PROJECT_DIR:-.}" in both hooks should become "resolve the repo/worktree from the invoking command's cwd". Failing that, the conservative version is stronger than I first suggested: refuse the push when its cwd resolves to a different worktree than CLAUDE_PROJECT_DIR, rather than silently evaluating the wrong one. A hard failure is recoverable; a silent pass is not.

Worth also asserting the pushed ref: the gate knows head_sha but never checks it against the refspec being pushed, so even within one worktree a git push origin <other-sha>:<branch> isn't covered.

Practical note for agents until this is fixed

One worktree per session. If you need to work another branch, start a new session rooted there — EnterWorktree is not a substitute, and it silently disables the gate.

(No branches were created — both probes were --dry-run, confirmed via git ls-remote.)