From 293e616eb79a45f2edadd22f606c77c9ca17bcf1 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 4 Sep 2026 19:06:03 -0500 Subject: [PATCH 1/2] fix(ci): follow the server after the reviewer context was de-required The owner removed `a reviewer has read this` from main's branch protection on 2026-09-04. The checked-in claim did not follow, so it overstated the required set by one context and read as blocking when nothing was. Re-measured the server at 2026-09-04 18:48 CDT: 13 contexts, strict true, enforce_admins true, approvals 0. - .github/required-contexts.txt: drop the context, and record the de-requiring and its cost under DELIBERATELY NOT REQUIRED. - tests/test_required_contexts.py: pin 13, not 14. - docs/CI.md: drop the bullet, rewrite the review-gate paragraph. - tests/test_security_posture.py: 13 contexts over 11 distinct jobs. - tests/negative_controls.toml: remove the entry, since the registry refuses a control for a context nobody requires. The tests it named are kept, because review-gate.yml still exists and still runs. review-gate.yml and branch protection are untouched. Watched fail: scripts/ci/check_required_contexts_drift.py exits 1 on the pre-fix file naming that context, and 0 on the corrected file, 13 against 13. Co-Authored-By: Claude Opus 5 --- .github/required-contexts.txt | 108 +++++++++++++----------------- docs/CI.md | 14 ++-- tests/negative_controls.toml | 89 +++++++----------------- tests/test_merge_gate_controls.py | 59 ++++++++++------ tests/test_required_contexts.py | 43 ++++++++---- tests/test_security_posture.py | 20 +++--- 6 files changed, 159 insertions(+), 174 deletions(-) diff --git a/.github/required-contexts.txt b/.github/required-contexts.txt index 5c6f553a4..b0e954f36 100644 --- a/.github/required-contexts.txt +++ b/.github/required-contexts.txt @@ -15,13 +15,14 @@ # "nothing pending" would have reported green without ever waiting for a test leg, because with one # required context there was nothing to wait for. Count what SHOULD gate the merge, not what does. # -# The set then moved four more times the same day (1 -> 13 -> 12 -> 15 -> 12), and it moved again on -# 2026-08-31: `a reviewer has read this` was armed, and the two CodeQL matrix contexts were seen -# required earlier that day and are no longer. THE LIST BELOW IS SET-EQUAL TO THE SERVER -- fourteen -# contexts, nothing extra on either side -- read from the API at 2026-08-31 20:57 CDT. A set-equal -# read is the only one worth writing down: a count alone cannot tell a matching set from two -# offsetting errors. Two non-context settings also moved and are recorded because they change merge -# semantics: +# The set then moved four more times the same day (1 -> 13 -> 12 -> 15 -> 12); it moved on 2026-08-31, +# when `a reviewer has read this` was armed and the two CodeQL matrix contexts stopped being required; +# and it moved again on 2026-09-04, when the owner DE-REQUIRED `a reviewer has read this`. That last +# one is recorded with its cost in the review-gate.yml entry at the foot of this file. THE LIST BELOW +# IS SET-EQUAL TO THE SERVER -- thirteen contexts, nothing extra on either side -- read from the API at +# 2026-09-04 18:48 CDT. A set-equal read is the only one worth writing down: a count alone cannot tell +# a matching set from two offsetting errors. Two non-context settings also moved and are recorded +# because they change merge semantics: # # strict = TRUE -- a PR must be up to date with `main` before it can merge. This closes the # stale-green hole: previously a PR validated against an older base could merge @@ -30,7 +31,8 @@ # enforce_admins = TRUE -- an admin's direct push to `main` is refused by the server, not merely by # the local hook. It was ON from 2026-07-28, turned OFF on 2026-07-29 through the # escape hatch in scripts/hooks/push_guard.py (for when a permanently red required -# check blocks every merge), and READ BACK AS TRUE on 2026-08-31. +# check blocks every merge), READ BACK AS TRUE on 2026-08-31, and TRUE again in +# the 2026-09-04 read above. `strict` re-read TRUE in that same call. # # THE HOOK ITSELF IS ALREADY CORRECTED. scripts/hooks/push_guard.py's module # docstring and its user-facing warning were rewritten in this same change, and @@ -152,59 +154,45 @@ a PR that implements BACKLOG #N must update BACKLOG.md # prevent. Adding to protection is all-or-nothing: the REST endpoint 422s with `already_exists` if any # context in the request is already required, and then adds NONE of them, so send only the missing ones. -# --- review-gate.yml -------------------------------------------------------------------------------- -# ARMED 2026-08-31 (BACKLOG #1404), and this line is the second half of that arming -- protection moved -# first, this file followed, which is the order the paragraph directly above prescribes. -# -# THIS ONE CONTEXT IS THE REPOSITORY'S ENTIRE REVIEW REQUIREMENT, AND THERE IS NOTHING BEHIND IT. -# `required_approving_review_count` is 0 and has to stay 0: every session on this machine pushes as ONE -# GitHub identity (`gh` auth lives at AppData\Roaming\GitHub CLI\hosts.yml, machine-wide rather than per -# config root), CODEOWNERS assigns every path to that same account, and GitHub refuses to let an author -# approve their own pull request -- so raising the count to 1 wedges every pull request permanently -# instead of reviewing any of them. review-gate.yml's header carries that measurement. With approvals -# pinned at 0, dropping this context from protection does not weaken review, it REMOVES it: nothing -# else anywhere reports that a green pull request was never read. -# -# What it enforces is a STEP, not an identity. `gh pr edit --add-label reviewed` is the whole -# protocol; nothing automated ever adds the label, and a `synchronize` event REMOVES it, so commits -# nobody has read are unread again. It does not establish that an independent party looked, and no -# prose here or anywhere else may describe it as if it does. -# -# SAFE TO REQUIRE, which for this file means one thing: the required-but-absent trap. A required check -# that never reports blocks every pull request forever, so three separate ways it could fail to report -# were checked before arming, and all three came back clean: -# -# * IT ALWAYS DISPATCHES. The trigger is `pull_request` with NO paths filter, and the `reviewed` job -# carries no `if:` of its own. The `if:`s in that workflow sit on STEPS, inside a job that always -# runs -- and a skipped step still leaves the job reporting a conclusion, which is what branch -# protection reads. -# * IT REPORTS IN THE QUEUE. review-gate.yml declares `merge_group:`. Omitting that is the total -# failure codeql.yml's own header records under BACKLOG #340: a required context whose workflow has -# no merge_group trigger never reports on a queue entry, so NOTHING MERGES. Measured there on the -# first entry the queue ever held -- 32 check-runs green and three contexts simply absent. -# * IT REPORTS ON PULL REQUESTS OLDER THAN ITSELF. GitHub runs a `pull_request` workflow from the -# MERGE commit, not from the head, so a branch that predates the workflow still gets the check. -# VERIFIED on PR 713: its head 5cb51e46 returns 404 for .github/workflows/review-gate.yml, and the -# context reported on it anyway. -# -# SO ARMING IT STRANDED NOTHING, and the remedy is not a rebase. Every open pull request goes red on -# this one check until a reviewer labels it; `labeled` is in the trigger list, so the label alone -# re-runs the job and clears it. That distinction is the one worth keeping: a reader who assumes a -# stranded check needs a rebase will rebase, which on `synchronize` STRIPS the label and moves the pull -# request further from green. -# -# THAT SENTENCE IS ONLY TRUE WHILE `labeled` IS IN THE WORKFLOW'S `types:` LIST, so it is held by a -# test rather than by this paragraph: -# -# tests/test_merge_gate_controls.py::test_the_review_gate_reruns_when_a_reviewer_adds_the_label -# -# Deleting that action reddens nothing on its own and leaves a gate no reviewer action can clear: the -# 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 - # DELIBERATELY NOT REQUIRED (do not add without the owner's decision; each has a recorded reason): +# review-gate.yml -- DE-REQUIRED 2026-09-04 by the owner, having been armed on 2026-08-31 +# (BACKLOG #1404). The workflow is untouched: it still dispatches, and +# it still reports `a reviewer has read this`. It no longer BLOCKS, so +# a red one on a pull request is not a merge problem. +# +# WHAT THAT COSTS, recorded because nothing else on the merge path now +# records it. `required_approving_review_count` is 0 and has to stay 0 +# -- every session here pushes as ONE GitHub identity and GitHub +# refuses to let an author approve their own pull request, so raising +# it to 1 wedges every pull request instead of reviewing any; +# review-gate.yml's header carries that measurement. This context was +# therefore the repository's ENTIRE review requirement. With it +# de-required, NOTHING on the merge path reports that a green pull +# request was never read. That is the owner's call, recorded here +# rather than argued. +# +# The `reviewed` label protocol is correspondingly unenforced. `gh pr +# edit --add-label reviewed` still works and still means what it +# meant -- a STEP happened, never that an independent party looked -- +# but no gate reads it any more. +# +# THE WORKFLOW'S OWN PROPERTIES ARE STILL HELD BY TESTS, in +# tests/test_merge_gate_controls.py: it reports under this exact +# context string, it declares `merge_group:`, and `labeled` is in its +# `types:` so a reviewer's label re-runs the job. Those tests stay. +# What they are no longer is NEGATIVE CONTROLS: the registry at +# tests/negative_controls.toml indexes controls by REQUIRED context +# and its reconciliation fails on a control naming a context nobody +# requires, so that entry came out in this same change and the tests +# themselves were kept. +# +# RE-ARMING IS AN OWNER DECISION, and the three ways it could have hit +# the required-but-absent trap were measured clean before 2026-08-31 +# and are unchanged: no paths filter and no job-level `if:`, a +# `merge_group:` trigger (whose absence is the total failure +# codeql.yml's header records under BACKLOG #340), and it reports on +# branches older than itself -- verified on PR 713, whose head +# 5cb51e46 404s for the workflow file and got the check anyway. # 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/docs/CI.md b/docs/CI.md index 4eadadfd9..161352104 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,13 @@ 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 **de-required by the owner on 2026-09-04**, having +been required since 2026-08-31. **The workflow still runs**, so a PR with no `reviewed` label still +shows that check red — it just does not block a merge any more, and it is not something to chase. +`required_approving_review_count` is 0 and stays 0, because every session pushes as one GitHub identity +and a human-approval rule would wedge every PR rather than review any. That made this one context the +repository's whole review requirement, so **nothing on the merge path now reports that a green PR was +never read**. `.github/required-contexts.txt` records that call and its cost; do not restate it here. 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..b52a19dfd 100644 --- a/tests/negative_controls.toml +++ b/tests/negative_controls.toml @@ -403,72 +403,29 @@ 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. -""" +# --- review-gate.yml: NO ENTRY, BECAUSE ITS CONTEXT IS NO LONGER REQUIRED --------------------------- +# +# `a reviewer has read this` was armed on 2026-08-31 (BACKLOG #1404) and DE-REQUIRED by the owner on +# 2026-09-04. Its control block came out here in that same change, for the reason the codeql note at +# the foot of this file gives: this registry reconciles against .github/required-contexts.txt, and an +# entry for a context that file does not list fails `every registered context is required`. +# +# THE TESTS THEMSELVES WERE KEPT, and that distinction is the whole point. review-gate.yml still exists +# and still runs; what ended is its power to block a merge. So the nine nodes this entry used to name +# still live in tests/test_merge_gate_controls.py and still execute inside the required `test` legs -- +# they are ordinary tests of a workflow now, not negative controls for a merge gate. Deleting them to +# satisfy a bookkeeping rule would have thrown away watched-fail evidence. +# +# WHAT THAT EVIDENCE WAS, kept here because it is not reproducible from the tests alone and would +# otherwise leave with this entry. Two neuterings of review-gate.yml on 2026-08-31, each reverted from +# the same run, reddened 1 of 11 selected controls and left 10 green -- SPECIFIC rather than uniform, +# which is what made them evidence rather than flattery. A third was found by SURVIVING: deleting +# `labeled` from the workflow's `types:` reddened NOTHING, 65 tests passing with the gate made +# permanently unclearable, because the absence detector read `pull_request`, `merge_group` and `paths` +# and never `types`. The trigger-list control added in response reds exactly one test, by name. +# +# RE-ARMING THE CONTEXT MEANS RESTORING AN ENTRY HERE, and the reconciliation says so on its own: a +# required context with no control is a failure, so nothing returns to the merge path unproven. # --- codeql.yml: NO ENTRY, BECAUSE ITS CONTEXTS ARE NOT REQUIRED ----------------------------------- # diff --git a/tests/test_merge_gate_controls.py b/tests/test_merge_gate_controls.py index bef3435ad..3e06598b9 100644 --- a/tests/test_merge_gate_controls.py +++ b/tests/test_merge_gate_controls.py @@ -2,17 +2,24 @@ # Copyright (C) 2026 MessageFoundry Organization and contributors """Negative controls for the required merge contexts that had none (BACKLOG #1000). -A GATE NOBODY HAS WATCHED FAIL IS AN ASSUMPTION WEARING A GREEN TICK. Fourteen contexts are the entire -merge gate as of 2026-08-31, and several of them were guarded only by the property that they exist. +A GATE NOBODY HAS WATCHED FAIL IS AN ASSUMPTION WEARING A GREEN TICK. Thirteen contexts are the entire +merge gate as of 2026-09-04, and several of them were guarded only by the property that they exist. This file supplies the missing plant-and-observe controls; ``tests/negative_controls.toml`` records which control belongs to which context and ``tests/test_negative_controls.py`` fails when a context has none. THE COUNT IS RECORDED HERE, NEVER DERIVED HERE. ``.github/required-contexts.txt`` is the checked-in -claim and the only thing this file reads. It said thirteen until ``a reviewer has read this`` was -reconciled onto it (BACKLOG #1404), and that is exactly how a context arrives here unproven: the -reconciliation is driven by the LIVE required set, so a newly-required context lands with zero controls -and SAYS SO, rather than quietly not being looked at. +claim and the only thing this file reads. It said thirteen, then fourteen when ``a reviewer has read +this`` was reconciled onto it (BACKLOG #1404), and thirteen again when the owner de-required that +context on 2026-09-04. The churn is the point: the reconciliation is driven by the LIVE required set, +so a newly-required context lands with zero controls and SAYS SO, rather than quietly not being looked +at. + +NOT EVERY TEST HERE BACKS A REQUIRED CONTEXT ANY MORE. The review-gate section at the foot is the case: +``review-gate.yml`` still exists and still runs, so those tests still hold its real behaviour, but they +stopped being NEGATIVE CONTROLS the moment its context left the required set -- the registry indexes by +required context and refuses an entry naming one nobody requires. They were kept rather than deleted, +because watched-fail evidence is not bookkeeping. EVERY CONTROL HERE IS ASYMMETRIC, and that is the part most likely to be skipped. It is not enough that neutering a rule turns a control red -- the control must fail for exactly the shapes that rule covers @@ -48,7 +55,7 @@ import pytest from _bash_resolver import CANNOT_RUN_CODES, bash_sees, explain_returncode, require_bash -from tests._workflow_contexts import ROOT, jobs_of, load_workflow, required_contexts +from tests._workflow_contexts import ROOT, jobs_of, load_workflow _TIMEOUT = 300 @@ -1019,11 +1026,16 @@ def test_the_absence_detector_fires_on_a_trigger_set_that_can_go_quiet() -> None # `a reviewer has read this` -- run as the SHIPPED SHELL against planted label sets. # =================================================================================================== # -# ARMED 2026-08-31 (BACKLOG #1404). With `required_approving_review_count` pinned at 0 -- every -# session on this machine pushes as one GitHub identity, so a human-approval rule would wedge every -# pull request rather than review any -- this single context is the repository's ENTIRE review -# requirement. Nothing else anywhere reports that a green pull request was never read, so a gate that -# could not go red here would not be a weakened control, it would be the absence of one. +# ARMED 2026-08-31 (BACKLOG #1404), DE-REQUIRED by the owner 2026-09-04. THE WORKFLOW IS UNCHANGED -- +# it still dispatches and still reports -- so everything below still describes live behaviour; what +# ended is the context's power to block a merge, which is why these are no longer registered in +# tests/negative_controls.toml. Expect a red `a reviewer has read this` on an unlabelled pull request: +# it gates nothing now. +# +# WHY THEY ARE KEPT ANYWAY. With `required_approving_review_count` pinned at 0 -- every session on this +# machine pushes as one GitHub identity, so a human-approval rule would wedge every pull request rather +# than review any -- this context WAS the repository's entire review requirement, and nothing replaced +# it. Re-arming is an owner decision; these tests are what would make it provable rather than hopeful. # # The gate's OWN shell is lifted out of the workflow and run, never re-implemented: a second copy of # that `case` would be free to agree with itself. @@ -1163,22 +1175,27 @@ def test_the_review_gate_passes_a_pull_request_a_reviewer_has_marked_read( ) -def test_the_review_gate_still_reports_under_the_required_context_string() -> None: +def test_the_review_gate_still_reports_under_its_declared_context_string() -> None: """The other way this gate dies, and the quieter one: the context stops arriving at all. Three surfaces, every one of them in-repo. The job NAME is the context string; a job-level `if:` - would let the job skip, and a skipped required check reports nothing; the trigger set decides - whether it reports on a pull request and in the merge queue. The `cla` control is this same shape - for the same reason -- where the context string is the surface, the string is what to pin. + would let the job skip, and a skipped check reports nothing; the trigger set decides whether it + reports on a pull request and in the merge queue. The `cla` control is this same shape for the + same reason -- where the context string is the surface, the string is what to pin. + + IT NO LONGER ASSERTS THAT THE CONTEXT IS REQUIRED, because since 2026-09-04 it is not. This test + kept its three structural assertions and dropped only that one, so what it now holds is that + re-arming would WORK: a job whose name drifted, or which could skip, or which could go absent in + the queue, is a context that cannot be put back on the merge path without wedging every pull + request. A re-arm is caught elsewhere and deliberately not duplicated here -- the registry + reconciliation in tests/test_negative_controls.py fails on a required context with no control, so + this gate cannot quietly return to blocking unproven. """ 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." - ) 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 " - f"branch protection requires. {_REVIEW_GATE_CONTEXT!r} would never report and every pull " + f"{_REVIEW_GATE_CONTEXT!r}. While that context is not required this reports nothing worse " + "than a renamed check; the moment it is re-armed, it would never report and every pull " "request would wedge." ) assert "if" not in job, ( diff --git a/tests/test_required_contexts.py b/tests/test_required_contexts.py index ce9bb0454..568c6c68d 100644 --- a/tests/test_required_contexts.py +++ b/tests/test_required_contexts.py @@ -12,19 +12,33 @@ and under a different string than its own line 18 gives tests/test_push_guard.py 12 -The live API said 12 when those five were counted. It says 14 at 2026-08-31 20:57 CDT. That question +The live API said 12 when those five were counted. It says 13 at 2026-09-04 18:48 CDT. That question is not trivia here: ``required_approving_review_count`` is 0 and auto-merge is armed, so required-set membership is the ONLY thing separating "reviewed" from "merged unread". A session reasoning from docs/CI.md would conclude that gitleaks, semgrep, npm-audit and crypto-inventory are advisory -- i.e. that four blocking security gates were safe to weaken. -THE SET NOW HOLDS A CONTEXT THAT DOES NOT TEST THE CODE, and it is the one with the least behind it. -``a reviewer has read this`` (review-gate.yml, armed 2026-08-31, BACKLOG #1404) is this repository's -ENTIRE review requirement. It is load-bearing BECAUSE approvals are pinned at 0 rather than in spite of -it: every session pushes as one GitHub identity, so a human-approval rule would wedge every pull -request instead of reviewing any. Drop that context from protection and review is not weakened, it is -gone -- nothing else anywhere reports that a green pull request was never read. The count below is what -makes removing it a deliberate edit rather than a quiet one. +EVERY CONTEXT IN THE SET NOW TESTS THE CODE. The one that did not, ``a reviewer has read this`` +(review-gate.yml, armed 2026-08-31 under BACKLOG #1404), was DE-REQUIRED by the owner on 2026-09-04. +With approvals pinned at 0 -- every session pushes as one GitHub identity, so a human-approval rule +would wedge every pull request instead of reviewing any -- that context was the repository's entire +review requirement, and removing it did not weaken review so much as end it: nothing on the merge path +now reports that a green pull request was never read. ``.github/required-contexts.txt`` records the +owner's call and its cost; this suite only holds the file honest about it. + +THIS SUITE CANNOT SEE THE SERVER, WHICH IS WHY THE DE-REQUIRING SAT HERE UNNOTICED. Everything in this +file reads the checked-in file, the workflows and the prose; the ``gh api`` line beside the count pin +is a COMMENT recording a manual read, not a call. So every test here stayed green while the file +overstated the required set by one context. A green run means the in-repo claims AGREE, never that they +are true. + +SOMETHING ELSE DOES READ THE SERVER, and it is the instrument to reach for when the answer matters: +``scripts/ci/check_required_contexts_drift.py`` reconciles the canonical file against live branch +protection and fails closed, run by the drift job in ``.github/workflows/required-workflow-state.yml``. +It is deliberately NOT one of the required contexts, so it reports without blocking -- it went red on +`main` for this very drift while this suite was green, which is the division of labour working, not a +gap. Watched fail 2026-09-04: against the pre-fix file it exited 1 naming ``a reviewer has read this``, +and against the corrected file it exited 0 on 13 versus 13. WHAT THIS PINS. ``.github/required-contexts.txt`` is the checked-in claim; these tests assert every in-repo statement agrees with it. The file is NOT the enforcement -- the server is -- so when branch @@ -107,16 +121,21 @@ 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:48 CDT: 13 contexts, SET-EQUAL to the file with nothing extra on either side. # Set-equal is the reading worth recording -- a count alone cannot tell a matching set from two # errors that cancel. # + # THE DATE IS NOT DECORATION AND MUST NOT BE COPIED FORWARD. Re-read the API and write down what + # THAT call returned; a stamp carried over from the last edit claims a measurement nobody took. + # # THE PIN GOES STALE IN THE DIRECTION THAT LOOKS FINE. It read 13 while the server held more, and # a context this file omits reads as NOT BLOCKING -- the reassuring answer rather than the true # 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, ( + # underneath it, so reconcile against the API, never against this number -- or let + # scripts/ci/check_required_contexts_drift.py do it, per the module docstring. The server moved + # four times on 2026-07-29, twice more on 2026-08-31, and again on 2026-09-04 when `a reviewer has + # read this` was de-required, which this pin over-reported as required until it was re-measured. + 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..0ce35482d 100644 --- a/tests/test_security_posture.py +++ b/tests/test_security_posture.py @@ -263,23 +263,27 @@ 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 DE-REQUIRED `a reviewer has read this` and review-gate's + # job left this module's scope with it. One collapse: ci.yml's `test` matrix reports 3 contexts + # from 1 job, so 13 - 2 = 11. (It was 14/12 from 2026-08-31, when that context was armed, #1404.) # # 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 # first, the file in the same pull request — and this pin is only as live as that discipline. Read - # at 2026-08-31 20:57 CDT the two were SET-EQUAL, so nothing is currently unexamined. + # at 2026-09-04 18:48 CDT the two were SET-EQUAL, so nothing is currently unexamined; the + # instrument that keeps them so is scripts/ci/check_required_contexts_drift.py, not this file. # - # Recording a context drags its job under every rule in this module for the first time, exactly as - # backlog-hygiene did on 2026-07-29 — and it paid immediately here: the review-gate job's - # `|| true` surfaced on the first CI run after that context was recorded. + # DE-RECORDING A CONTEXT TAKES ITS JOB BACK OUT OF EVERY RULE HERE, which is the direction that + # loses coverage quietly: review-gate.yml still runs, and its steps are no longer read by this + # module at all. Recording one does the reverse, exactly as backlog-hygiene did on 2026-07-29 — + # and that paid immediately here, the review-gate job's `|| true` surfacing on the first CI run + # after its context was recorded. print( 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 " + assert examined == 11, ( + f"expected the 13 required contexts to resolve to 11 distinct jobs (the 3 `test` legs share " f"one matrix job); got {examined}. If the workflow layout genuinely changed, update this " "count." ) From 2bac18567a630d646c2b714a04e117f30d85e031 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 4 Sep 2026 19:09:16 -0500 Subject: [PATCH 2/2] docs(backlog): file BACKLOG #1452 for the required-context drift fixed here Records the measurement, the five in-repo claims that had to move with the canonical file, and why the review-gate negative controls were de-registered rather than deleted. Also records the residue nobody has ruled on: CLAUDE.md still describes the Reviewer seat and the reviewed-label gate, and unread-signal.yml plus check_unread_prs.py exist to notify a reviewer about a gate that now blocks nothing. Two earlier allocations on this subject are named so a later reader does not read three numbers as three defects: 1449, allocated by a peer seat and left unfiled, and 1451, which this seat allocated from the primary checkout by mistake and cannot commit, because ledger ownership keys on the allocating worktree or its branch. Co-Authored-By: Claude Opus 5 --- docs/BACKLOG.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 5a070ff8b..7eb7762a6 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -22066,3 +22066,32 @@ So `test_the_script_prefers_its_own_repo_over_an_earlier_path_entry` supplies th **Verification:** 8 passed in `tests/test_webconsole_seam_snapshot.py`. Mutation check run rather than argued -- with the `sys.path.insert` line deleted, the decoy test reds naming the decoy import, and the by-path digest test **stays green**, which is the luck described above measured rather than predicted. Anchor restored, 8 passed again. **Adjacent and NOT fixed here, named rather than numbered.** `docs/WEBCONSOLE-PACKAGE.md`'s seam-refresh procedure is stale in three steps left behind by #1220: it says to bump `ENGINE_UI_SEAM` by hand (`1` to `2`) when the value is a derived digest, it says to update curated lists in this script that #1220 retired, and its step 5 prescribes `python scripts/webconsole_seam_snapshot.py > tests/golden/...`, the shell redirect this script's own docstring forbids because PowerShell's `>` writes UTF-16LE with a BOM into a file the test reads as UTF-8. That is doc drift with its own cause and it wants its own item; folding a documentation rewrite into a `sys.path` fix would make both harder to review. + +## 1452. required-contexts.txt and its pin still claim the retired reviewer context is required + +> 🔢 **Filed 2026-09-04 - FIXED IN THIS PULL REQUEST, not yet landed.** The owner removed `a reviewer has read this` from `main`'s branch protection earlier the same day. The checked-in claim did not follow it, so the repository's own answer to "does this check block a merge" over-reported the required set by exactly one context. + +**Cluster:** repository tooling. **Priority:** P3. **Verdict:** build. +**Severity:** no engine effect, no PHI axis, and **no deployment axis (sec. 0)** -- none of these files ship in a wheel. The cost is a merge-path claim that was wrong, in the one file whose entire purpose is to be that answer readable from a clone. + +**MEASURED 2026-09-04 18:48 CDT** against `gh api repos/MEFORORG/MessageFoundry/branches/main/protection`: the server requires **13** contexts, `.github/required-contexts.txt` named **14**, and the difference is exactly `a reviewer has read this`. The other twelve matched, both directions. `strict`, `enforce_admins` and `required_approving_review_count` re-read TRUE, TRUE and 0 in the same call, unchanged. + +**THE DIRECTION OF THE ERROR IS THE UNUSUAL PART, AND IT IS THE SAFER ONE.** The failure this file was built against is the file naming FEWER contexts than the server, because a context the file omits reads as not-blocking -- the reassuring answer rather than the true one. This drift ran the other way: the file claimed a gate that nothing enforced. That still misdirects, and it does so about review specifically, which is the one gate that never tested the code. + +**ONE INSTRUMENT SAW IT, AND IT IS DELIBERATELY NOT A REQUIRED ONE.** `tests/test_required_contexts.py` compares the file against prose, workflow job names and a hand-written count, so all nine of its tests stayed green throughout while the claim was false. That is by construction rather than a defect in it: the `gh api` line beside its count pin is a COMMENT recording a past manual read, not a call. `scripts/ci/check_required_contexts_drift.py` does read the server and fails closed, and it was RED on `main` for this drift -- it is not in the required 13, so it reported and blocked nothing. Watched fail 2026-09-04: exit 1 against the pre-fix file, naming that context, and exit 0 against the corrected file at 13 against 13. + +**WHAT THE CORRECTION HAD TO TOUCH, which is wider than the two lines it looks like.** Removing one context from the canonical file moves five in-repo claims that are reconciled against it: + +| File | What moved | +|---|---| +| `.github/required-contexts.txt` | the context line, the set-equal header read, and a new DELIBERATELY NOT REQUIRED entry recording the de-requiring and its cost | +| `tests/test_required_contexts.py` | the pinned count, 14 to 13, plus the module docstring | +| `docs/CI.md` | the required-checks bullet and the review-gate paragraph | +| `tests/test_security_posture.py` | 13 contexts resolving to 11 distinct jobs, was 14 over 12 | +| `tests/negative_controls.toml` | the review-gate control entry, because the registry refuses a control naming a context nobody requires | + +**THE NEGATIVE CONTROLS WERE DE-REGISTERED, NOT DELETED.** `review-gate.yml` still exists and still runs, so the nine test nodes that entry named still hold its real behaviour inside the required `test` legs. What ended is their status as controls for a merge gate. One assertion did have to go: `test_the_review_gate_still_reports_under_the_required_context_string` asserted the context was required, which is now false; it kept its three structural assertions, was renamed to `..._under_its_declared_context_string`, and its docstring records that a re-arm is caught by the registry reconciliation rather than duplicated there. + +**RESIDUE, NAMED RATHER THAN NUMBERED, because retiring any of it is an owner decision this seat cannot take.** `CLAUDE.md` section 5 still describes the Reviewer seat and still states that the `reviewed` label is what blocks a merge. `.github/workflows/unread-signal.yml` and `scripts/ci/check_unread_prs.py` exist to notify a reviewer that a pull request is waiting on a gate that now blocks nothing. `review-gate.yml` itself still reddens an unlabelled pull request, which is expected and harmless but reads as a failure. Whether any of that is retired, and whether the context is re-armed, is the owner's call. + +**TWO NUMBERS WERE BURNED ON THIS SUBJECT BEFORE THIS ONE, AND NEITHER IS FILED.** A peer seat allocated 1449 for the same drift and left it unfiled. This seat then allocated 1451 from the PRIMARY checkout by mistake: ledger ownership keys on the allocating worktree or its branch, both non-transferable, so 1451 is claimed by `main` in a tree that must never commit it and is unusable here. 1452 was re-allocated from this worktree and is the only one of the three that names this work. Recorded so a later reader does not read three numbers as three defects; `scripts/coord/alloc_strand_sweep.py` is the tool that already exists for stranded allocations.