C3D-2204: fix the review gates head-blindness - #269
Conversation
Adversarial review — Tier A, 6 opinions (full write-up on c3d-mcp-server#201)The shipped workflow is byte-identical across all four rollout PRs ( Verdict: merge is fine; promotion to a required check is not. This is a strict improvement on the head-blind gate it replaces and introduces no regression. But the gate can be turned green by anyone able to post a comment (24 characters, invisible in the rendered UI, no write access needed on the two PUBLIC repos) — pre-existing, and the thing that must close before enforcement mode. All five legs answered the planted question YES independently. |
Rolls out the head-drift fix for the review gate (C3D-2204). Workflow file only — no application code.
The defect
The gate passed on label presence alone and never read the marker's
head=. It fires onsynchronize— the one event that invalidates a marker — so it re-ran and confidently passed green exactly when it was most likely wrong.The fix
When a review is recorded, compare the marker's
head=<40-char sha>against the PR's current head:HEAD DRIFT, naming both SHAs and the delta to reviewhead=review-exemptSemantics are deliberately identical to
~/reviews/tools/review-gate.sh, including lasthead=wins so a supplemental pass re-anchors the marker. Two answers to "is this reviewed?" that disagree would be its own defect.Break-test evidence — observed, not asserted
Extracted the
run:block and executed it against a mockedgh, 10 scenarios:Then sabotaged the comparison itself (
if [ "$MARKER_HEAD" = "$PR_HEAD_SHA" ]→if true): the drift case flips torc=0, confirming that comparison is what produces the red rather than something incidental.The break test earned its keep — it caught a real defect in the fix.
grep -ocolourised the match, so the captured SHA carried ANSI escapes and never string-equalled the PR head: the gate reportedHEAD DRIFTon every correctly-reviewed PR. Fails closed and loud, but wrong, and it only reproduces where grep chooses to colourise. Fixed with--color=never, which the sibling shell script already had for exactly this reason.Review status — stated plainly
This has NOT had a multi-leg adversarial review. It carries the break-test evidence above and a single (orchestrator) opinion. Deliberately left unlabeled so the gate flags it rather than self-exempting — a workflow whose job is to enforce review should not wave itself through.