fix(gate): bind certification to content, not to a SHA (#540) - #628
Conversation
The CERTIFIED lane keyed staleness on `commit_sha == HEAD`. PreToolUse runs before the commit, so at check time HEAD is still the certified commit and the gate allowed — then the commit landed carrying whatever was staged. The resulting commit was by construction never the one that was reviewed. Sol found it at file:line; Fable withdrew its own recommendation to extend the gate once shown the counter-example. The same key also taxed every commit with a re-pin call, ~10 wasted tool calls in one measured cycle, to protect nothing a content pin does not. Certification now names the tree it was issued over. The hook compares `git write-tree` against `candidate_tree`; the merge gate compares the remote head's tree, and the base it was read against, before merging. An earlier ruling chose `git patch-id --stable`. Both advisors amended it after measuring two failures: two diffs differing only in the indentation of an added Python line produce the identical patch-id, and whitespace is semantic in Python, YAML, Makefiles and string literals; and what merges is the resulting tree, not the diff, so a rebase onto moved upstream keeps patch-id identical while producing content nobody read. Message-only amend, squash and reorder with no upstream movement all produce the same tree, so certification survives them at zero cost. Only content that moves invalidates. The hook does not parse commit flags. Requiring a clean tracked worktree makes `git commit` and `git commit -a` commit the same tree, so the index speaks for both; modelling git's option grammar in a regex is what took #610 from 3 to 6 to 4 before round 4 deleted it. `base_tree` is checked only at the merge boundary for the same reason: at commit time the base is HEAD or HEAD^ depending on whether it is an amend, and that costs nothing to defer because a rebase changes the index tree anyway. Three suites encoded the old contract and were rewritten with it. The stale-SHA row became a moved-content row, because a new commit that changes nothing reviewable is now exactly the case the ruling preserves. Two fixtures used a made-up 40-char head with no git objects behind it, which cannot survive a check that resolves a tree; both now build real ones. Prior art read rather than derived, per the ruling: codex-sdlc-wizard#111's requireFrozenIndex + candidateTree. Each guard proven by mutating the file and re-running: hook 8/0 -> 6/2, and either merge-gate comparison neutered lets the fixture merge.
…540) Round 1 of the cross-model review found one blocker, independently, in both legs, and both constructed it rather than argued it. The base_tree check resolved `origin/$BASE_BRANCH^{tree}` — a local cache. Absent, the `[ -n "$CUR_BASE_TREE" ] &&` guard skipped the comparison outright: one `update-ref -d` merged a certification carrying a bogus base_tree at exit 0. Stale, the cached tree still satisfied the certification: a bare server repo with main advanced and the local ref left behind reached the same exit 0. The check was fail-open in exactly the situation it exists for, and its posture contradicted the fail-closed candidate_tree check ten lines above it. The base is now read from the PR's own baseRefOid, resolved with the same fetch-fallback as the remote head, and blocks when it cannot be resolved. Both reviewers prescribed this same fix. A claim in the round-1 rationale was also false, and it was load-bearing. The deferral of base_tree to the merge boundary was justified by "a rebase onto moved upstream changes the index tree, so the hook's check already refuses it". When upstream independently produces the candidate's final content, the rebase drops the redundant branch change and the index tree does not move. The deferral survives on the narrower ground that PreToolUse cannot observe post-command state and the base ref is well-defined only at the merge boundary — which is precisely why that boundary may not trust a cache. The test that asserted the exact merge command read .fixture-sha after deleting the fixture, so the substitution was empty and the grep passed on a prefix match without checking the SHA. Guards proven by mutation, reverted by inverse edit: neutering the fail-closed branch gives 84/1, and reverting the base read to the tracking ref gives 83/2 with the fixture merging at exit 0. The first mutation attempt reverted with `git checkout --`, which silently discarded the uncommitted fix along with the mutation and made the reading meaningless. The unreadable-base row first asserted only a non-zero exit and stayed green through its own mutation, because the mismatch check below rejects an empty tree anyway. Both new rows now assert the specific refusal.
…emembers (#540) Round 2 fixed the wrong thing correctly. It replaced a stale local tracking ref with `baseRefOid`, which both reviewers had independently prescribed — and which live data falsifies. That field is the base commit associated with the pull request, a snapshot that does not move when the branch does. On this repo PR #615 carries baseRefOid f8ba12b while main is at d0e1c7b, and the trees differ. So round 2 swapped an indefinitely stale local cache for an indefinitely stale server-side snapshot: the same hole, relocated. Both rounds failed the same way, and the shared mistake is the finding: read a cached value, treat it as current. The base tip now comes from `gh api repos/{owner}/{repo}/git/ref/heads/<base>` on every run, and `baseRefOid` is removed from the parsed field list rather than left sitting there for someone to reach for again. Everything downstream of the OID is unchanged — the fetch-fallback, the fail-closed refusal, the tree comparison were all reviewed and cleared. The round-2 regression test agreed with the bug. It hand-set the base OID to the advanced commit, which is precisely what GitHub does not do, so it could never have caught this. The stub now drives the two knobs separately: baseRefOid stays at the snapshot while the ref endpoint reports the moved tip. Reverting the source to the snapshot field is red permanently. A shipped comment in the commit hook asserted that the merge boundary reads baseRefOid. It did, and it was wrong; the correction keeps the falsification rather than quietly restating the conclusion. Removing the field regressed the clearance suite to 40/9 — eight stubs answered `pr view` and not the ref endpoint. All eight now answer both. Mutation: swapping the source back to the snapshot gives 83/2 with the fixture merging at exit 0, reproducing the round-2 defect. A first attempt broke the JSON parse instead of the source and went red for an unrelated reason, which proves nothing; it was redone as a clean swap. The check-to-merge race is recorded as an accepted limit. --match-head-commit binds the head atomically and GitHub offers no base equivalent, so a base move inside that window cannot be closed from the client. Suites: merge-gate 85/0, clearance-binds-to-tree 8/0, hooks 225/0, cross-model-clearance 49/0.
|
CROSS-MODEL-CLEARANCE {"confidence":99,"reviewer":"sol","sha":"912eb65651bf8956380f8cdd73f48f603c847fb4","verdict":"YES"}Round 3, GPT-5.6 Sol at Verified live rather than asserted: REST returned
Mutations reproduced independently: snapshot source 83/2 with two exit-0 merges, fail-closed branch neutered 84/1, tracking ref restored 83/2. Baselines merge-gate 85/0, clearance-binds-to-tree 8/0, hooks 225/0, cross-model 49/0, doc-consistency 137/0, workflow-triggers 169/0. Reverting the eight stub repairs reproduced 40/9, confirming they supply the newly required response rather than relaxing assertions. Non-blocking items filed as #632 (two comments describing removed mechanisms). The falsified "rebase changes the index tree" premise has been corrected in the PR body above. |
|
CROSS-MODEL-CLEARANCE {"confidence":98,"reviewer":"fable","sha":"912eb65651bf8956380f8cdd73f48f603c847fb4","verdict":"YES"}Round 3, Fable 5. Score 9/10. Concession recorded first. This leg CERTIFIED at 9/10 in round 2 on a false premise — it characterised the That round-2 certification was not used to merge anything. Independently reproduced on this head, mutations run in an isolated clone: faithful revert to parsing Attack (b): Filed non-blocking, now on #632: |
|
DUAL CROSS-MODEL CERTIFIED MERGE Cleared by: sol (posted by @BaseInfinity), fable (posted by @BaseInfinity) — both bound to Merge-evidence path(s) this authorised:
Verified: CI ATTESTED, NOT AUTHENTICATED. Both clearances were posted by the same Posted by |
1 participant
Closes #540. First item of #593 Rung 1's merged order (#540 → #521 → #563 → #617 → #504).
The hole
hooks/codex-gate-check.shkeyed staleness oncommit_sha == HEAD.PreToolUseruns before the commit, so at check time HEAD is still the certified commit and the gate allows — then the commit lands carrying whatever is staged. The resulting commit is by construction not the one that was reviewed. The issue's own stated test — "a fixture that certifies, commits, and asserts the gate refuses; currently it passes, which is the bug" — is row 1 of the new suite, and it failed before this change.The same key also taxed every commit with a re-pin call (~10 wasted tool calls in one measured cycle) while protecting nothing a content pin does not.
The mechanism, as ruled
Per #540's reconciled comment of 2026-08-11 (Fable 5 + Sol, independent agreement after one reconcile round), which amended the earlier patch-id ruling. Two measurements killed patch-id:
So certification names the tree. The hook compares
git write-treetocandidate_tree;scripts/merge-pr.shcompares the remote head's tree, and the base it was read against.Deliberately preserved: message-only amend, squash and reorder with no upstream movement all produce the same tree, so certification survives them at zero cost. Rows pin both directions — a guard that only ever refuses is indistinguishable from a broken one.
Two decisions worth reviewing
No flag parsing.
git commit -asweeps unstaged changes in, so the committed tree would not be the index's. The fix is not to look for-a: requiring a clean tracked worktree makesgit commitandgit commit -acommit the same tree. Modelling git's option grammar in a regex is what took #610 from 3 → 6 → 4 before round 4 deleted it.base_treeis enforced at the merge boundary only. At commit time the base is HEAD for a normal commit and HEAD^ for an amend, and telling them apart means reading--amendoff the command line — the same trap. Pinned as row 6, which asserts ALLOW with the reasoning inline.Contract change: three suites encoded the old one
test-hooks.sh's "blocks a stale certification after a new commit" row is now "blocks staged content the certification never saw" — under tree binding a new commit that changes nothing reviewable is exactly the preserved case.test-merge-gate.shandtest-cross-model-clearance.shused a made-up 40-char head with no git objects behind it, which cannot survive a check that resolves a tree. Both build real ones now.Verification
RED first: 3 passed / 5 failed against the unmodified hook. GREEN: 8/0.
Every guard proven by mutation, not by reading:
!= candidate_treehalfif falseif falseSuites green: test-hooks 225/0, test-merge-gate 83/0, test-cross-model-clearance 49/0 (baseline 49/0), new suite 8/0, doc-consistency 137/0, workflow-triggers.
Registered in both
ci.ymlandCONTRIBUTING.md— #610 ran eight review rounds with CI red because a suite reached one and not the other.Prior art read rather than derived, per the ruling:
codex-sdlc-wizard#111'srequireFrozenIndex+candidateTree.