diff --git a/.github/required-contexts.txt b/.github/required-contexts.txt index 5c6f553a4..68a3c86b7 100644 --- a/.github/required-contexts.txt +++ b/.github/required-contexts.txt @@ -202,9 +202,20 @@ a PR that implements BACKLOG #N must update BACKLOG.md # label fires no run, and the only trigger that still re-runs the job is `synchronize`, whose first # step REMOVES the label. Measured 2026-08-31 -- before the test above existed, dropping `labeled` # passed 65 tests. -a reviewer has read this +# +# RETIRED 2026-09-04 BY THE OWNER, and removed from branch protection the same day. The whole block +# above is kept as the record of what the gate did while it was armed; it is history, not a claim +# about the current set. The context now lives in the not-required list below. # DELIBERATELY NOT REQUIRED (do not add without the owner's decision; each has a recorded reason): +# review-gate.yml -- `a reviewer has read this` was REQUIRED from 2026-08-31 until the +# owner retired the reviewer requirement on 2026-09-04. Measured +# against branch protection that day: the required set went 14 -> 13 +# and this context is absent. The workflow still runs and still +# reports; it simply no longer gates a merge. Reinstating it is an +# owner decision -- the label it checks for is applied by hand and by +# the PR's own author, so the gate records that a step happened, not +# that an independent party looked. # codeql.yml, scorecard.yml -- their SARIF upload needs `security-events: write`, which fork-PR # tokens lack, so requiring them would block every fork PR. # `CodeQL (python)` and `CodeQL (javascript-typescript)` were read as diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index ede138941..9cad2cc4d 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -34,8 +34,11 @@ on: # .github/required-contexts.txt mirrors the server and is the single authority; restating the # required set in a second file is how the two drift. An earlier version of this comment asserted # "THE QUEUE IS LIVE AND THESE THREE CONTEXTS ARE REQUIRED". That was true when written and is - # false as of 2026-09-01, when the required set is 14 contexts and none of them is CodeQL - # (BACKLOG #1404 records the removal). The trigger stays either way: it costs nothing while they + # false as of 2026-09-01: none of the required contexts is CodeQL (BACKLOG #1404 records the + # removal). The COUNT that stood here has been dropped rather than updated -- restating it was the + # very drift the paragraph above warns about, and it went stale on 2026-09-04 when the required set + # moved. Read the count from .github/required-contexts.txt, which mirrors the server. + # The trigger stays either way: it costs nothing while they # are not required, and it is the prerequisite that has to be in place BEFORE they ever are. # # security.yml's own on: block predicted this in the PR that added merge_group to the other diff --git a/docs/CI.md b/docs/CI.md index 4eadadfd9..cd2286364 100644 --- a/docs/CI.md +++ b/docs/CI.md @@ -51,7 +51,6 @@ The stable contexts required on `main` are — mirroring - `forbidden-content (customer/PHI leak guard)` - `a PR that implements BACKLOG #N must update BACKLOG.md` - `cla` -- `a reviewer has read this` `cla` is the **job key** in `cla.yml`, whose job declares no `name:`. Branch protection matches the job name, never the workflow name — so the context is `cla`, not "CLA Assistant". Every @@ -69,12 +68,19 @@ the same reason and additionally **does not run on PRs at all** (`scorecard.yml` trigger — it runs on push-to-main, a schedule, and branch-protection changes). Nightly / path-gated legs (service-smoke, load, SQL/Postgres store) are deliberately **not** required. -`a reviewer has read this` (`review-gate.yml`) is the required check that is **not a test of the code**, -and the one with nothing behind it. `required_approving_review_count` is 0 and stays 0 — every session -pushes as one GitHub identity, so a human-approval rule would wedge every PR rather than review any — -which makes this single context the repository's whole review requirement. A PR clears it with -`gh pr edit --add-label reviewed`; a new commit removes the label, so re-review is automatic. It -proves a **step happened**, not that an independent party looked. +`a reviewer has read this` (`review-gate.yml`) was **retired by the owner on 2026-09-04** and removed +from branch protection the same day; the required set went 14 to 13. The workflow still runs and still +reports, but it no longer gates a merge, and a PR needs no `reviewed` label to land. + +**Read what that leaves, because the two halves were always separate.** `required_approving_review_count` +is 0 and stays 0 — every session pushes as one GitHub identity, so a human-approval rule would wedge +every PR rather than review any. With the label context retired as well, **no automated control now +requires that any change be read before it merges.** That is a deliberate owner decision, recorded here +rather than inferred; it is not a gap to be quietly closed by re-arming the context. Re-arming it is an +owner decision too. + +Worth keeping in view if it is ever reconsidered: the label is applied by hand, commonly by the PR's own +author, so the check proved a **step happened**, not that an independent party looked. The `quality-advisory.yml` jobs create **no code-scanning category** and **no _required_ check context** — they do report as ordinary advisory checks, and they **must never be added to the required list**. Two diff --git a/tests/negative_controls.toml b/tests/negative_controls.toml index b53c04592..3072d9f20 100644 --- a/tests/negative_controls.toml +++ b/tests/negative_controls.toml @@ -403,73 +403,6 @@ deliberately hostile ambient (PYTHONIOENCODING=cp1252, PYTHONUTF8=0, LC_ALL=tr_T core.autocrlf, and PATH ordered so the WSL bash comes first). """ -# --- review-gate.yml ------------------------------------------------------------------------------- - -[[control]] -context = "a reviewer has read this" -plants = """ -Six label sets that are not a review -- none at all, unrelated ones, and four NEAR-MISSES -(`reviewed-by-bot`, `not-reviewed`, `Reviewed`, `re,viewed`) -- plus the stale payload: a `synchronize` -whose event still carries `reviewed` because the removal step ran one step ago. The gate's OWN shell is -lifted out of the workflow and run under the flags Actions uses, not re-implemented; a second copy of -that `case` would be free to agree with itself. Four structural plants sit beside them, because the -quieter death of this gate is the context never arriving or never clearing: a renamed job, a job-level -`if:`, a trigger set that cannot report on a pull request or in the merge queue, and a `types:` list -that does not re-run the job when the reviewer adds the label. -""" -red = [ - "tests/test_merge_gate_controls.py::test_the_review_gate_refuses_a_pull_request_nobody_has_marked_read", - "tests/test_merge_gate_controls.py::test_the_review_gate_refuses_a_synchronize_even_when_the_payload_shows_the_label", - "tests/test_merge_gate_controls.py::test_the_review_gate_still_reports_under_the_required_context_string", - "tests/test_merge_gate_controls.py::test_nothing_in_the_review_gate_adds_the_label_it_checks_for", - "tests/test_merge_gate_controls.py::test_the_review_gate_reruns_when_a_reviewer_adds_the_label", -] -holds = """ -A labelled pull request must PASS, in every position the token can occupy in the joined label list -- -first, last, middle and alone. A gate that refused uniformly would satisfy every planted case above -while wedging every pull request in the repository, permanently, because `strict = true` means a -blocked pull request cannot even be brought up to date past it. The merge queue is the other half: a -merge_group entry carries no pull request and therefore no labels, so the label-reading step must stay -confined to pull_request events -- a required context that can never go green in the queue means -NOTHING MERGES, which codeql.yml's header records happening here. The trigger list has the same shape -of asymmetry: every action the gate depends on must be named individually when it is dropped, while a -list that only ADDS actions must stay clean. -""" -green = [ - "tests/test_merge_gate_controls.py::test_the_review_gate_passes_a_pull_request_a_reviewer_has_marked_read", - "tests/test_merge_gate_controls.py::test_the_review_gate_lets_a_merge_queue_entry_through", - "tests/test_merge_gate_controls.py::test_the_absence_detector_fires_on_a_trigger_set_that_can_go_quiet", - "tests/test_merge_gate_controls.py::test_the_label_rerun_detector_fires_on_a_types_list_that_ignores_the_label", -] -observed = """ -RUN AGAINST A NEUTERED GATE, 2026-08-31, and the result is SPECIFIC rather than uniform, which is the -part that makes it evidence. Two separate neuterings of review-gate.yml, each reverted from the same -run: - - * the `*,reviewed,*)` case arm widened to `*)`, so the gate can never refuse -- 1 of the 11 selected - controls red, 10 green. - * the synchronize comparison changed to a value that never occurs - (`[ "$ACTION" = "never-happens" ]`), which leaves the gate reading a payload that is stale by one - step -- 1 red, 10 green, and the one that reddened is the stale-payload control specifically. - -A THIRD SPELLING OF THE SAME NEUTERING IS REPORTED AS A HARNESS FAULT, DELIBERATELY. Replacing the -test with `if false` removes `$ACTION` from the script, and the fixture refuses to run a script that -ignores the input it is being fed -- three ERRORS rather than three passes. A control that fed input to -a script which cannot read it would be green about nothing (BACKLOG #1216). - -The gate has also been seen red in ordinary use: arming the context on 2026-08-31 turned every open -pull request red on this one check until a reviewer labelled it. - -A THIRD NEUTERING WAS FOUND BY IT SURVIVING, 2026-08-31, which is the only reason it is written down. -Deleting `labeled` from review-gate.yml's `types:` list reddened NOTHING -- 65 tests passed with the -gate made permanently unclearable, because the absence detector reads `pull_request`, `merge_group` and -`paths` and never looked at `types`. With the trigger-list control added, the same edit reds exactly -ONE test by name -- test_the_review_gate_reruns_when_a_reviewer_adds_the_label -- and 62 pass. Exactly -one, not two: the detector's own control uses literal trigger lists rather than subtracting from the -shipped file, so it does not move with the workflow it judges and cannot bury the naming red under a -second one that is only bookkeeping. -""" - # --- codeql.yml: NO ENTRY, BECAUSE ITS CONTEXTS ARE NOT REQUIRED ----------------------------------- # # `CodeQL (python)` and `CodeQL (javascript-typescript)` are advisory: the SARIF upload needs diff --git a/tests/test_merge_gate_controls.py b/tests/test_merge_gate_controls.py index bef3435ad..d3ffe831a 100644 --- a/tests/test_merge_gate_controls.py +++ b/tests/test_merge_gate_controls.py @@ -1172,9 +1172,21 @@ def test_the_review_gate_still_reports_under_the_required_context_string() -> No for the same reason -- where the context string is the surface, the string is what to pin. """ job = jobs_of(_REVIEW_GATE)[_REVIEW_GATE_JOB] - assert _REVIEW_GATE_CONTEXT in required_contexts(), ( - f"{_REVIEW_GATE_CONTEXT!r} left .github/required-contexts.txt. With approvals pinned at 0 " - "that does not weaken review, it removes it." + # THIS TEST USED TO ASSERT THE CONTEXT WAS REQUIRED, and that assertion has been REMOVED rather + # than weakened, because the owner retired the reviewer requirement on 2026-09-04 and the context + # left branch protection the same day. Its message was right about the consequence and is kept + # here so nobody re-derives it: with `required_approving_review_count` pinned at 0, dropping this + # context does not weaken the review requirement, it removes it. That is now the recorded state, + # not a regression -- docs/CI.md says so in the same words. + # + # What remains below still earns its keep while the workflow runs: the job must keep reporting + # under its own name, must not grow a job-level `if:`, and must not acquire a trigger set that + # goes quiet. Those are the properties that would have to hold on the day anyone re-arms it, and + # they rot silently in the meantime. + assert _REVIEW_GATE_CONTEXT not in required_contexts(), ( + f"{_REVIEW_GATE_CONTEXT!r} is back in .github/required-contexts.txt. Re-arming the review " + "gate is an owner decision; if that is what happened, restore the assertion this comment " + "replaced, its negative control in tests/negative_controls.toml, and the docs/CI.md entry." ) assert str(job.get("name")) == _REVIEW_GATE_CONTEXT, ( f"the job name is {job.get('name')!r}, so it reports under a different context string than " diff --git a/tests/test_required_contexts.py b/tests/test_required_contexts.py index ce9bb0454..9728720a1 100644 --- a/tests/test_required_contexts.py +++ b/tests/test_required_contexts.py @@ -107,7 +107,8 @@ def test_the_canonical_file_parses_and_names_the_live_set() -> None: ) # Pinned so that ADDING or REMOVING a required check is a deliberate, reviewed edit here rather # than a silent one. Verified against `gh api repos/MEFORORG/MessageFoundry/branches/main/protection` - # at 2026-08-31 20:57 CDT: 14 contexts, SET-EQUAL to the file with nothing extra on either side. + # at 2026-09-04 18:33 CDT: 13 contexts, SET-EQUAL to the file with nothing extra on either side. + # (Was 14 until the owner retired `a reviewer has read this` on 2026-09-04 and it left protection.) # Set-equal is the reading worth recording -- a count alone cannot tell a matching set from two # errors that cancel. # @@ -116,7 +117,7 @@ def test_the_canonical_file_parses_and_names_the_live_set() -> None: # one. A count that only ever fails when someone edits the FILE cannot notice the server moving # underneath it, so reconcile against the API, never against this number. The server moved four # times on 2026-07-29 and twice more on 2026-08-31. - assert len(contexts) == 14, ( + assert len(contexts) == 13, ( f"the canonical required set changed to {len(contexts)} contexts. If branch protection really " "changed, update this count AND every claim this suite checks; if it did not, revert the file." ) diff --git a/tests/test_security_posture.py b/tests/test_security_posture.py index e0e408d9f..544dea280 100644 --- a/tests/test_security_posture.py +++ b/tests/test_security_posture.py @@ -263,8 +263,9 @@ def test_required_jobs_carry_no_continue_on_error() -> None: # change in the collapse — a matrix split, or a context that quietly stops resolving — forces a # look here instead of passing on a self-consistent count. # - # 14/12 since 2026-08-31 (BACKLOG #1404), when `a reviewer has read this` was recorded. One - # collapse: ci.yml's `test` matrix reports 3 contexts from 1 job, so 14 - 2 = 12. + # 13/11 since 2026-09-04, when the owner retired `a reviewer has read this` and it left branch + # protection. One collapse: ci.yml's `test` matrix reports 3 contexts from 1 job, so 13 - 2 = 11. + # (Was 14/12 from 2026-08-31, BACKLOG #1404, while that context was recorded.) # # THIS MODULE READS THE CANONICAL FILE, NOT THE SERVER, so a context that reaches protection and # not the file is not examined here. That file's own header states the ordering rule — protection @@ -278,10 +279,10 @@ def test_required_jobs_carry_no_continue_on_error() -> None: f"[security-posture] examined {examined} distinct jobs backing " f"{len(required_contexts())} required contexts" ) - assert examined == 12, ( - f"expected the 14 required contexts to resolve to 12 distinct jobs (the 3 `test` legs share " - f"one matrix job); got {examined}. If the workflow layout genuinely changed, update this " - "count." + assert examined == 11, ( + f"expected {len(required_contexts())} required contexts to resolve to 11 distinct jobs (the 3 " + f"`test` legs share one matrix job); got {examined}. If the workflow layout genuinely changed, " + "update this count." ) assert not offenders, ( "a REQUIRED status check cannot fail, so it gates nothing:\n "