Skip to content

ci: retire the review gate, and record what its removal broke - #889

Open
wshallwshall wants to merge 1 commit into
mainfrom
ci/retire-review-gate
Open

ci: retire the review gate, and record what its removal broke#889
wshallwshall wants to merge 1 commit into
mainfrom
ci/retire-review-gate

Conversation

@wshallwshall

Copy link
Copy Markdown
Collaborator

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

File Change
.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 to 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
docs/CI.md, docs/BACKLOG.md, CLAUDE.md prose corrected, retracted in place

Set-equality is proved, not counted

required-contexts.txt matches 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. It is now 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 this PR does not fix it

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 -- 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 labeled reddened NOTHING while 65 tests passed; two separate neuterings each reverted; the merge-commit reporting trap measured on PR 713 at head 5cb51e46; 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 local runner limit and is running separately. ruff clean, yaml parses, zero non-ASCII in the edited files. All ten pre-commit hooks passed.

Relationship to claude/reconcile-required-contexts-13

That branch (9e9752b52) 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. One difference: its negative_controls.toml change dropped the 65-tests finding. This PR keeps it.

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.
@github-actions github-actions Bot added the unread Green and mergeable, but not marked read. Read it, then add: reviewed label Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

@wshallwshall

This pull request is green and nobody has marked it read.

Every check that has reported is passing and the reviewed label is absent, so
a reviewer has read this is the only thing between it and a merge. Nothing else reports that, which is
why this comment exists (BACKLOG #1413).

To clear it, IN THIS ORDER:

  1. If the branch is behind main, update it FIRST: gh pr update-branch --repo MEFORORG/MessageFoundry 889.
    That push fires synchronize, and synchronize REMOVES the reviewed label. Labelling
    before updating throws the label away and costs a round trip.
  2. Read the diff.
  3. gh pr edit 889 --repo MEFORORG/MessageFoundry --add-label reviewed

DO NOT TRIAGE THIS FROM mergeStateStatus. It returns one value with precedence, so BEHIND,
DIRTY and UNSTABLE each mask the missing check -- the requirement is invisible until you act on
something else. This check never reads that field; see scripts/ci/check_unread_prs.py.

A seat can find every pull request in this state in one call: gh pr list --label unread.
The label is withdrawn automatically once this pull request leaves the state.

The reviewed label is a PROCESS gate. It records that a step happened. It does not
establish that an independent party looked, and nothing here should be read as saying it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

unread Green and mergeable, but not marked read. Read it, then add: reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant