Skip to content

C3D-2204: fix the review gates head-blindness - #14

Merged
matt-manuel merged 2 commits into
developfrom
matt/c3d-2204-review-gate-head-drift
Sep 2, 2026
Merged

matt-manuel merged 2 commits into
developfrom
matt/c3d-2204-review-gate-head-drift

Conversation

@matt-manuel

Copy link
Copy Markdown
Member

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 on synchronize — the one event that invalidates a marker — so it re-ran and confidently passed green exactly when it was most likely wrong.

That is not hypothetical: c3d-mcp-server#170 shipped a real feature to production fully unreviewed, carrying a valid adversarial-reviewed label and two human approvals the whole time. A head-blind required check is worse than an advisory one, so this had to land before enforcement mode is considered.

The fix

When a review is recorded, compare the marker's head=<40-char sha> against the PR's current head:

state result
marker head == PR head pass
marker head != PR head failHEAD DRIFT, naming both SHAs and the delta to review
marker carries no head= warn and pass — "cannot verify", not "drifted"
review-exempt pass, not head-sensitive (an exemption is about the kind of change)

Semantics are deliberately identical to ~/reviews/tools/review-gate.sh, including last head= 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 mocked gh, 10 scenarios:

label + marker head MATCHES current    rc=0 want=0  PASS
label + marker head STALE (drift)      rc=1 want=1  PASS
label + marker has NO head=            rc=0 want=0  PASS
supplemental: LAST marker matches      rc=0 want=0  PASS
supplemental: LAST marker stale        rc=1 want=1  PASS
no label, marker matches (warn+pass)   rc=0 want=0  PASS
no label, no marker                    rc=1 want=1  PASS
review-exempt ignores drift            rc=0 want=0  PASS
dependabot auto-C                      rc=0 want=0  PASS
chore(deps title auto-C                rc=0 want=0  PASS

Then sabotaged the comparison itself (if [ "$MARKER_HEAD" = "$PR_HEAD_SHA" ]if true): the drift case flips to rc=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 -o colourised the match, so the captured SHA carried ANSI escapes and never string-equalled the PR head: the gate reported HEAD DRIFT on 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. Your call whether it needs a cycle before merge.

@matt-manuel
matt-manuel requested a review from 000x999 as a code owner September 2, 2026 03:38
@matt-manuel matt-manuel added the agentic-engineering Work produced with agentic engineering assistance label Sep 2, 2026
@matt-manuel matt-manuel self-assigned this Sep 2, 2026
@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@matt-manuel

Copy link
Copy Markdown
Member Author

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 (sha256 13fc4c17…, verified on every head), so one cycle covers all four. The reconciled findings, false-positives section and recommendation live on https://github.com/CognitiveVR/c3d-mcp-server/pull/201 rather than being duplicated here.

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.

@matt-manuel matt-manuel added the adversarial-reviewed A reconciled multi-model adversarial review is posted on this PR label Sep 2, 2026
@matt-manuel
matt-manuel merged commit e76a126 into develop Sep 2, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

adversarial-reviewed A reconciled multi-model adversarial review is posted on this PR agentic-engineering Work produced with agentic engineering assistance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant