fix(codex-gate): the review gate made the review protocol uncommittable (#533) - #548
fix(codex-gate): the review gate made the review protocol uncommittable (#533)#548BaseInfinity wants to merge 2 commits into
Conversation
) The gate blocked every commit unless handoff.json said REVIEWED or CERTIFIED. The shipped protocol prescribes PENDING_REVIEW and PENDING_RECHECK for the whole duration of a round — which is exactly when work gets produced. So nothing could be saved while it was under review, reviewers were pushed onto a mutable working tree, and REVIEW UNIT's "review committed SHAs, not a mutable tree" was unreachable by construction. Not theoretical. During #520 round 18 a reviewer read the tree while a mutation harness was concurrently mutating it, measured 179 passed / 16 failed, and filed a correct, fully-diagnosed P1 against a defect that existed only in the mutation. A whole round spent on an artifact. The fix is a branch-scoped in-flight lane. An in-flight status may be saved, but only onto a branch the hook can positively prove is not the default one. The gate's purpose is stopping unreviewed work from reaching the default branch, never stopping work from being saved. Everything else blocks as before: no artifact blocks everywhere, an unrecognised status blocks everywhere, and #437's freshness check is untouched — including on feature branches. Round 1 counts as much as round 2. PENDING_REVIEW is what holds while the FIRST reviewer reads, which is precisely the configuration that produced the phantom P1. Resolution is offline and fails strict. refs/remotes/origin/HEAD is a local ref written by git clone, with main/master refs as fallback. Anything unresolvable — not a repo, detached HEAD, no trunk-shaped ref anywhere — is treated as the default branch. init.defaultBranch is deliberately not consulted: it names the branch for NEW repos, so it can name one this repo never had. The documented bypass was also unreachable from where you hit it. All four messages said to set CODEX_GATE_SKIP=1, but the hook reads that from its own process environment and runs BEFORE the command, so an inline prefix in a tool call can never produce it. The mechanism is right and stays — an agent must not be able to self-grant a bypass of a gate that exists to constrain it — so no sentinel file was added. The messages now name the route that works. Nine tests: three RED before the fix, six boundary assertions that pin the quarantine. The follow-up the design named — the Stop nag going silent once the tree stays clean — is filed as #547, not patched here. Design: Fable, 82% confidence, the 18% in the branch-resolution primitive.
…and at all Round 1 of the in-flight lane failed lenient in two directions Codex reproduced, and verifying those turned up two detection holes that predate the lane. P0-1, targeting. The hook read its own branch and vouched for a different repository. Relocation is now refused by shape rather than modelled: any relocation token, or any chain segment that is not a bare git invocation, makes a command ineligible. Ineligible is not a block; it falls through to the old rules, which block an in-flight status. P0-2, the resolver. A vestigial local ref and a stale symref were being read as proof. The positive-evidence fallback is deleted, not patched. The lane now needs a conjunction: inside a work tree, an attached HEAD, a branch outside the reserved-trunk denylist, and a remote whose HEAD symref resolves to a ref that exists and names something else. Anything unresolvable is "we do not know", and that is not a lane. P0-3, detection, pre-existing. A newline arrives as two characters, so the word boundary never fired after it and this gate has never seen a commit that was not on the first line. Verified on 435708a: exit 0 with no review artifact at all. P0-4, detection, pre-existing. A short option takes its value attached, so one spelling of the repo-relocation flag was a real invocation the old pattern could not match. Also exit 0 on 435708a with no artifact. 23 assertions, executed against the hook. 15 are red against the round-1 hook and 2 against main; the rest pin behaviour that must not move.
Round 2: NOT CERTIFIED. The design is refuted, not the implementation.Codex returned two more P0s at P0-A — masking destroys the evidence eligibility needsThe eligibility shape test runs against Masking turns this into Any quote character can split any token. Blacklisting tokens in a string whose quotes have been erased is unsound in principle, not just in this instance. P0-B — a cached symref is not authorityThe resolver requires a remote whose The hook grants the lane while standing on the repository's real trunk. I was wrong about one of my own findingsI claimed
The other self-found hole is real and confirmed pre-existing: a newline arrives as two characters, so the word boundary never fired after one, and this gate has never seen a commit that was not on the command's first line — exit 0 on Where this goesThis is the second repair round on the same component in one cycle, which by this repo's own rule means the design is wrong rather than the implementation. I've escalated the design question rather than writing a third patch. Likely outcomes include deleting most of what this branch added and splitting out the newline detection fix. What did verify clean
Round-1 and round-2 reviews are now committed at |
Design call after two refuted rounds: the question was wrong, not the answerTwo review rounds, four P0s, all four reproduced. Escalated to design rather than writing a third patch, per this repo's second-repair rule. The verdict: "Provably not the default branch" is unanswerable offlineDefault branch is remote, server-side state. An offline hook has only Both P0s were the same error: treating non-authoritative evidence as authority. A quote-masked string is not the command. A cached symref is not the remote's default. The feature never needed that questionThe requirement is reviewers get immutable SHAs of the round's work. The round already knows which branch it is about. Two rounds were spent answering something strictly harder than the feature asks for. The designWhen a round opens, the protocol writes a
Two strictness-only lines stay, documented in the hook as accident-catchers and explicitly not soundness claims: refuse on a literal Why this holds where the other two didn't. It adds no new trust assumption. Rejected as wrong, not merely worse:
Eligibility must never run on masked text. Masking exists so detection can ignore prose; it destroys exactly what eligibility needs. The masked shape test and segment counting are deleted, and are not rebuilt on the raw string. What diesMost of the ~500 lines on Splitting out the real bypassThe newline-decode fix ( It will not carry the Confidence88%. The residual sits in whether the reviewer accepts the self-attestation trust model — the prepared answer is that it is the same model |
fe0a326 to
4c621dc
Compare
1 participant
Closes #533.
The review gate blocked every save while
statuswasPENDING_REVIEWorPENDING_RECHECK— which is the status the shipped protocol prescribes for the entire duration of a review round. Work could not be saved exactly while it was being reviewed, so reviewers read a mutable working tree. During #520 round 18 a reviewer read a tree a mutation harness was concurrently mutating and filed a fully-diagnosed P1 against a defect that existed only in the mutation. A whole round spent on an artifact.The lane: an in-flight status may save, but only onto a branch the hook can positively prove is not the default branch. Everything else blocks exactly as before — no artifact, unrecognised status, and #437 staleness all still block, on every branch.
What round 1 got wrong
Codex reviewed the first implementation and returned NO with two P0s, both of which I reproduced. Both failed lenient, which is the only direction that matters: the lane turned a hard block into a silent allow.
P0-1 — targeting. The branch check consulted the hook's own cwd, never the repository actually being written to. Relocation is now refused by shape rather than modelled: any relocation token, or any chain segment that is not a bare
gitinvocation, makes a command ineligible for the lane. Ineligible is not a block — it falls through to the pre-existing rules, which block an in-flight status.P0-2 — the resolver. A leftover local ref from a branch rename, and a symref that was never repointed after a default-branch change, were both being read as proof. Codex was right that the one named hole was not the only one, so the positive-evidence fallback is deleted rather than patched. The lane now requires a conjunction of positive proofs: inside a work tree, an attached HEAD, a branch outside the reserved-trunk denylist, and at least one remote whose HEAD symref resolves to a ref that actually exists and names something else. Anything unresolvable is "we do not know", and "we do not know" is not a lane.
Two holes that predate this branch
Found while verifying the above. Neither is a regression of the lane; both are reproduced against
435708aonmain, and in both cases the gate returned exit 0 with no review artifact present at all.P0-3 — detection. A newline reaches the hook as two characters, so the word boundary in the detection pattern never fired after one. This gate has never seen a
git committhat was not on the first line of the command. Every multi-line invocation has walked past it since it was written.P0-4 — detection. A short option accepts its value attached, so one spelling of the repo-relocation flag was a real, valid invocation the old pattern could not match at all.
Evidence
The verification matrix was executed against three hooks:
d26eb28(the round-1 hook Codex reviewed)435708a(main)23 permanent assertions in
tests/test-hooks.sh, all of which execute the hook rather than grepping its source. 243 hook tests pass; all 65 non-E2E CI suites pass locally on macOS.Known limitation, on record: a bare
git commit -F - <<'MSG'loses the lane, because that heredoc body is unquoted and a prose line is indistinguishable from a command segment to a coarse shape test. The three forms that carry a body through a quoted span all keep it, so no commit is prevented — one spelling of it is. Pinned by a test so it stays a decision rather than a surprise.Dogfooded: the round-2 commit on this branch was made at
PENDING_RECHECK— the exact state that blocked this work twice during #531.Reviewer note
macOS is the only platform verified locally. The remaining flagged risk is BSD-vs-GNU divergence in
\b,\s, andsed's newline handling, all of which this change depends on. Linux CI green is part of the verification here, not a formality.