ci: retire the review gate, and record what its removal broke - #889
ci: retire the review gate, and record what its removal broke#889wshallwshall wants to merge 1 commit into
Conversation
Owner instruction 2026-09-04: "the reviewer requirement is retired", then "remove it now". The required context came off branch protection the same day -- 14 contexts to 13, verified live -- and this is the repository half. .github/workflows/review-gate.yml deleted .github/required-contexts.txt context line out, block kept as a retired record tests/test_required_contexts.py pin 14 -> 13 tests/test_merge_gate_controls.py 8 review-gate tests out, 1 general test added tests/negative_controls.toml plants block out, findings kept tests/test_security_posture.py count reconciled docs/CI.md, docs/BACKLOG.md, CLAUDE.md prose corrected, retracted in place SET-EQUALITY PROVED, NOT COUNTED. required-contexts.txt is equal to the server again, checked by diffing sorted sets through the module's own required_contexts() parser rather than comparing lengths: 13 and 13, no element on either side alone. Two sets of the same size can differ. THE GUARD IS BLIND AND TODAY IS THE PROOF. test_required_contexts.py parses the FILE and never the server, so when the context came off protection this morning the test STAYED GREEN at 14 over a server holding 13. It reddened only when a human arrived to edit the file. That is a guard which can only fail when somebody edits the thing it guards, which is the case it is least needed for. Recorded in the test, with scripts/ci/check_required_contexts_drift.py named as the instrument that does read the API. WHAT THE DELETION BROKE, AND IT IS NOT FIXED HERE. unread-signal.yml watched `review gate` so that labelling a pull request `reviewed` re-ran that gate, whose completion re-triggered the signal and withdrew the `unread` label. review-gate.yml was the ONLY watched workflow firing on a `labeled` event: ci.yml, security.yml and backlog-hygiene.yml declare no types and cla.yml declares [opened, synchronize]. So `unread` now clears on the next push rather than when somebody reads the pull request. A flag that survives being read is worse than no flag. The regression is written at the line where the entry was removed, and the test that used to pin the behaviour now guards the record: it fails if the entry returns without the mechanism, and it fails if the explanation is deleted. Restoring it needs an arm firing on `labeled`, and that file deliberately uses workflow_run rather than pull_request for stated reasons -- it runs from the default branch, never executes head code, and its header says NOT REQUIRED, AND MUST NOT BECOME REQUIRED. That is a security-surface change and it is the owner's, not this commit's. FINDINGS KEPT THROUGH THE DELETIONS, verified by count before and after: dropping `labeled` reddened NOTHING while 65 tests passed; two separate neuterings each reverted; the merge-commit reporting trap measured on PR 713 at head 5cb51e4; and merge_group declaration. Deleting the mechanism is right, deleting the evidence it was blind is not. VERIFICATION. 240 passed across all 11 affected modules. Scoped rather than the whole suite, which exceeds the runner limit here and is running separately. ruff clean, yaml parses, zero non-ASCII in the edited files. A parallel branch, claude/reconcile-required-contexts-13 at 9e9752b, reconciled the same records without deleting the workflow. Its docs/CI.md was taken as the base here and the deletion layered on top, so its reconciliation is not re-litigated. Its negative_controls.toml deletion dropped the 65-tests finding; this commit keeps it.
|
This pull request is green and nobody has marked it read. Every check that has reported is passing and the To clear it, IN THIS ORDER:
DO NOT TRIAGE THIS FROM A seat can find every pull request in this state in one call: The |
Owner instruction 2026-09-04: "the reviewer requirement is retired", then "remove it now". The required context came off branch protection the same day, 14 contexts to 13. This is the repository half.
What changed
.github/workflows/review-gate.yml.github/required-contexts.txttests/test_required_contexts.pytests/test_merge_gate_controls.pytests/negative_controls.tomldocs/CI.md,docs/BACKLOG.md,CLAUDE.mdSet-equality is proved, not counted
required-contexts.txtmatches the server again, checked by diffing sorted sets through the module's ownrequired_contexts()parser rather than comparing lengths: 13 and 13, no element on either side alone. Two sets of the same size can differ.The guard is blind, and today is the proof
test_required_contexts.pyparses the file and never the server. So when the context came off protection this morning, the test stayed green at 14 over a server holding 13. It reddened only when a human arrived to edit the file.That is a guard which can only fail when somebody edits the thing it guards, which is the case it is least needed for. It is now recorded in the test, with
scripts/ci/check_required_contexts_drift.pynamed as the instrument that does read the API.What the deletion broke, and this PR does not fix it
unread-signal.ymlwatchedreview gateso that labelling a pull requestreviewedre-ran that gate, whose completion re-triggered the signal and withdrew theunreadlabel.review-gate.ymlwas the only watched workflow firing on alabeledevent --ci.yml,security.ymlandbacklog-hygiene.ymldeclare notypes:, andcla.ymldeclares[opened, synchronize]. Sounreadnow clears on the next push rather than when somebody reads the pull request.A flag that survives being read is worse than no flag. The regression is written at the line where the entry was removed, and the test that used to pin the behaviour now guards the record: it fails if the entry returns without the mechanism, and it fails if the explanation is deleted.
Restoring it needs an arm firing on
labeled, and that file deliberately usesworkflow_runrather thanpull_request-- it runs from the default branch, never executes head code, and its header says NOT REQUIRED, AND MUST NOT BECOME REQUIRED. That is a security-surface change and it is the owner's call, not this PR's.Findings kept through the deletions
Verified by count before and after: dropping
labeledreddened NOTHING while 65 tests passed; two separate neuterings each reverted; the merge-commit reporting trap measured on PR 713 at head5cb51e46;merge_groupdeclaration.Deleting the mechanism is right. Deleting the evidence it was blind is not.
Verification
240 passed across all 11 affected modules. Scoped rather than the whole suite, which exceeds the local runner limit and is running separately.
ruffclean, yaml parses, zero non-ASCII in the edited files. All ten pre-commit hooks passed.Relationship to
claude/reconcile-required-contexts-13That branch (
9e9752b52) reconciled the same records without deleting the workflow. Itsdocs/CI.mdwas taken as the base here and the deletion layered on top, so its reconciliation is not re-litigated. One difference: itsnegative_controls.tomlchange dropped the 65-tests finding. This PR keeps it.