From 151a1744fb242e6f8fbafeb9992ab861aa1137fc Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 3 Sep 2026 16:37:47 -0500 Subject: [PATCH 1/2] fix(ci): read the reviewed label live in the review gate (BACKLOG #1423) The label-reading step took its verdict from the webhook payload, which is a snapshot frozen when the event fired. On PR 765 a `labeled` run created at 20:43:24Z started its job at 20:49:34Z and reported SUCCESS from that payload, four minutes after the queued `synchronize` run had removed the label. Nothing corrected the green: the removal runs under GITHUB_TOKEN, which raises no workflow run, so no `unlabeled` run ever fired. It now reads `gh pr view --json labels` at evaluation time and BLOCKS when that read fails. The `synchronize` removal and its unread-by-definition arm are unchanged, as is the job name that branch protection requires. The controls run the shipped shell against a `gh` stand-in that records its own argv, and the pre-fix form is run against the same planted fixture and observed PASSING, so the refusal is measured rather than asserted. Co-Authored-By: Claude Opus 5 --- .github/workflows/review-gate.yml | 79 ++++++++-- tests/negative_controls.toml | 53 +++++-- tests/test_merge_gate_controls.py | 252 +++++++++++++++++++++++++++--- 3 files changed, 337 insertions(+), 47 deletions(-) diff --git a/.github/workflows/review-gate.yml b/.github/workflows/review-gate.yml index 134980c47..08d6dce12 100644 --- a/.github/workflows/review-gate.yml +++ b/.github/workflows/review-gate.yml @@ -34,6 +34,32 @@ name: review gate # are unread again, even if the pull request was marked an hour ago. That is the one thing the # workflow writes, and it only ever writes in the blocking direction. # +# THE LABEL IS READ LIVE, NEVER FROM THE EVENT PAYLOAD (BACKLOG #1423, generalising #1417). A webhook +# payload is a SNAPSHOT taken when the event fired, and a run can sit queued for minutes before it +# executes. Measured on pull request 765, 2026-09-03, from the runs and timeline APIs: a reviewer +# labelled at 20:43:19Z; run 33803911587 was created by that `labeled` event at 20:43:24Z; the earlier +# `synchronize` run 33803677823, created 20:40:58Z and queued four minutes, finally executed and +# removed the label at 20:45:14Z; then the `labeled` run's job started at 20:49:34Z and reported +# SUCCESS at 20:49:37Z out of its own 20:43:24 payload -- six minutes stale, and four minutes after +# the label it was reading had been deleted. The pull request carried this context GREEN with zero +# labels until a human re-labelled at 20:52:55Z. So the step below asks the API what the labels ARE. +# +# AND NOTHING CORRECTS THAT GREEN, WHICH IS WHY READING LIVE IS THE FIX RATHER THAN AN IMPROVEMENT. +# GitHub does not dispatch a workflow run from an event raised by the repository's own GITHUB_TOKEN -- +# `workflow_dispatch` and `repository_dispatch` are the documented exceptions, and neither is a label +# event. The removal above runs `gh` under `github.token`, so its `unlabeled` event emits no run. +# Verified on the same head: the workflow-runs API reports exactly TWO review-gate runs on +# ee2e7ec2423a73fd385d2afa27733ca050058cba, the two named above, and none for the 20:45:14Z removal. +# +# THE LIVE READ NARROWS THE WINDOW, IT DOES NOT CLOSE IT TO ZERO, and saying otherwise would be the +# compensating control resting on a false premise. A `labeled` run can still read a label that a +# `synchronize` run removes moments later, and its green then stands on a state that has just changed. +# What the payload read cost was MINUTES of queue delay; what remains is the gap between this step's +# API call and the job finishing, which is seconds. So the operational rule survives the fix rather +# than expiring with it: for a BEHIND pull request, update-branch, WAIT for the resulting +# `synchronize` review-gate run to COMPLETE, then label, then merge. Labelling while that run is still +# queued is what produced the false green above, and it is still the shape to avoid. +# # NO CONCURRENCY BLOCK, DELIBERATELY. This job's name is intended to become a required status # context, and a cancelled required check can never go green. backlog-hygiene.yml carried # `cancel-in-progress` on a key that collapsed to one group on `merge_group`, and entries cancelled @@ -50,8 +76,11 @@ on: # without it the label emits no run, the red check-run stands, and the only trigger left that # re-runs this job is `synchronize` -- whose first step REMOVES the label. The pull request then # cannot be merged by any action at all, and `strict = true` means it cannot even be brought up to - # date past the block. `unlabeled` is the opposite half: without it, withdrawing the label leaves a - # GREEN context behind. Both are pinned by + # date past the block. `unlabeled` is the opposite half FOR A HUMAN WITHDRAWAL: without it, a + # reviewer taking the label back would leave a GREEN context behind. It does NOT cover the removal + # this workflow performs on `synchronize` -- that one is raised by GITHUB_TOKEN and emits no run at + # all, measured above -- so `unlabeled` is not what protects against a stale green; the live read + # in the last step is. Both are pinned by # tests/test_merge_gate_controls.py::test_the_review_gate_reruns_when_a_reviewer_adds_the_label, # because dropping `labeled` reddened nothing before that test existed. types: [opened, reopened, ready_for_review, synchronize, labeled, unlabeled] @@ -102,21 +131,43 @@ jobs: - name: Require the reviewed label if: github.event_name == 'pull_request' env: - LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + NUMBER: ${{ github.event.pull_request.number }} ACTION: ${{ github.event.action }} + # DIAGNOSTIC ONLY, AND NEVER THE VERDICT. This is the snapshot that produced the measured + # false green, kept so the log can SAY it went stale instead of leaving a reader to infer it + # from three API endpoints and two clocks. Nothing below branches on it. + PAYLOAD_LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} run: | - # On `synchronize` the label was just removed above, so the event payload is stale by one - # step. Treat that action as unreviewed by definition rather than reading a value that is - # already wrong -- reading the payload here would pass a pull request that was just - # invalidated, which is the exact failure this step exists to prevent. + # `synchronize` IS UNREAD BY DEFINITION, and it is answered BEFORE any read. The step above + # has just removed the label, so a live read would usually agree -- but "usually" is what the + # payload race already cost. Commits nobody has read are unread whatever any label says, so + # this arm is decided by the action and consults nothing it could race against. if [ "$ACTION" = "synchronize" ]; then echo "::error::New commits have not been read. Re-review, then: gh pr edit --add-label reviewed" exit 1 fi - case ",$LABELS," in - *,reviewed,*) - echo "reviewed label present. Gate satisfied." ;; - *) - echo "::error::Not yet read by a reviewer. When you have read it: gh pr edit --add-label reviewed" - exit 1 ;; - esac + # READ LIVE, AND FAIL CLOSED ON A FAILED READ. A gate whose safe state depends on a network + # call succeeding is not a gate, so the read is tested rather than trusted: `gh` absent, + # unauthenticated, rate-limited, or answering with an error all land here and BLOCK. The + # sibling shape to avoid is a checker that ignores a child's return code and reports clean. + if ! LABELS=$(gh pr view "$NUMBER" --json labels --jq '[.labels[].name] | join(",")'); then + echo "::error::Could not read the labels on pull request #$NUMBER, so this gate cannot say whether anybody has read it. Blocking rather than guessing -- re-run this job." + exit 1 + fi + # EXACT-ELEMENT MATCH against the comma-joined list, not a substring: `reviewed-by-bot` and + # `not-reviewed` are different labels, and one of them reads as the opposite. + case ",$PAYLOAD_LABELS," in *,reviewed,*) WAS=yes ;; *) WAS=no ;; esac + case ",$LABELS," in *,reviewed,*) NOW=yes ;; *) NOW=no ;; esac + # Printed only when the two DISAGREE ON THIS LABEL, so ordering differences between the two + # sources never make noise. A diagnostic that fires on every run is one nobody reads. + if [ "$WAS" != "$NOW" ]; then + echo "STALE PAYLOAD: the event payload recorded reviewed=$WAS, the live label set says reviewed=$NOW. The verdict below uses the LIVE set. Payload [$PAYLOAD_LABELS], live [$LABELS]." + fi + if [ "$NOW" = yes ]; then + echo "reviewed label present in the live label set [$LABELS]. Gate satisfied." + exit 0 + fi + echo "::error::Not yet read by a reviewer. When you have read it: gh pr edit --add-label reviewed" + exit 1 diff --git a/tests/negative_controls.toml b/tests/negative_controls.toml index b53c04592..2fb6d10b9 100644 --- a/tests/negative_controls.toml +++ b/tests/negative_controls.toml @@ -409,17 +409,26 @@ core.autocrlf, and PATH ordered so the WSL bash comes first). 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. +(`reviewed-by-bot`, `not-reviewed`, `Reviewed`, `re,viewed`) -- plus `synchronize`, which must be +refused even with the label still in place, because commits nobody has read are unread by definition. +THE STALE PAYLOAD IS PLANTED SEPARATELY AND IS THE MEASURED ONE (BACKLOG #1423): the webhook snapshot +says `reviewed` while the live label set is empty, which is PR 765 replayed at 20:49:37Z on +2026-09-03. It is planted on every action the gate can see, because the pre-fix form screened one +action and let the rest read the snapshot. Two more plants guard the read itself: `gh` exiting +non-zero must BLOCK, and the stand-in records its own argv so a gate that stopped calling the API +cannot be graded as though it had. 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_refuses_a_synchronize_even_when_the_label_is_still_there", + "tests/test_merge_gate_controls.py::test_the_review_gate_refuses_a_stale_payload_that_still_shows_the_label", + "tests/test_merge_gate_controls.py::test_the_review_gate_reads_the_live_labels_for_every_action_not_just_one", + "tests/test_merge_gate_controls.py::test_the_review_gate_fails_closed_when_the_live_label_read_fails", "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", @@ -428,15 +437,21 @@ 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. +blocked pull request cannot even be brought up to date past it. The live read has its own asymmetry +and it runs the OTHER way: a payload that UNDERSTATES the labels -- a run created before the reviewer +labelled -- must now clear, where the pre-fix form left it red until something re-ran the job. And the +pre-fix form itself is run against the identical planted fixture and must go GREEN; if both forms +refused, the fixture would be proving something other than the recorded defect. 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_clears_when_the_stale_payload_understates_the_live_label", + "tests/test_merge_gate_controls.py::test_the_payload_reading_form_of_the_gate_passes_the_same_planted_pull_request", "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", @@ -468,6 +483,16 @@ ONE test by name -- test_the_review_gate_reruns_when_a_reviewer_adds_the_label - 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. + +A FOURTH NEUTERING WAS FOUND IN PRODUCTION USE, 2026-09-03, and it was the gate itself rather than an +edit to it (BACKLOG #1423). Run 33803911587 on pull request 765 reported SUCCESS while the pull +request carried ZERO labels, because it read a webhook payload snapshotted six minutes before its own +job started and four minutes before `github-actions[bot]` deleted the label it was reading. Nothing +corrected the green: the removal is raised under GITHUB_TOKEN, GitHub dispatches no workflow run from +such an event, and the runs API confirms exactly two review-gate runs on that head with neither being +the removal. The fix reads the labels live; the control that proves it is the pre-fix form, run +against the identical planted fixture and observed PASSING, so the shipped refusal is measured +against a form known to accept -- not merely asserted. """ # --- 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..dbbe6de79 100644 --- a/tests/test_merge_gate_controls.py +++ b/tests/test_merge_gate_controls.py @@ -1033,6 +1033,11 @@ def test_the_absence_detector_fires_on_a_trigger_set_that_can_go_quiet() -> None _LABEL_STEP = "Require the reviewed label" +#: The live read the step must perform, verbatim. Substituted away by the pre-fix control below, so it +#: is written once here rather than twice. +_LIVE_READ = 'gh pr view "$NUMBER" --json labels --jq \'[.labels[].name] | join(",")\'' + + def _review_gate_script() -> str: """The label-reading step's shell, with the shape this control depends on asserted first.""" steps = jobs_of(_REVIEW_GATE)[_REVIEW_GATE_JOB]["steps"] @@ -1042,16 +1047,56 @@ def _review_gate_script() -> str: f"{len(matches)}; this control runs THAT step's shell and cannot pick between several" ) script = str(matches[0]["run"]) - assert "$LABELS" in script and "$ACTION" in script, ( - "the label-reading step no longer reads $LABELS and $ACTION, so this control would be feeding " - f"input to a script that ignores it and every verdict below would be about nothing: {script!r}" + assert _LIVE_READ in script, ( + "the label-reading step no longer reads the labels LIVE from the API, so this control would " + "be feeding a stub to a script that never calls it and every verdict below would be about " + f"nothing. Expected {_LIVE_READ!r} in: {script!r}" + ) + assert "$ACTION" in script, ( + f"the label-reading step no longer reads $ACTION, so the synchronize arm is gone: {script!r}" ) return script +#: A stand-in `gh` placed FIRST on PATH. It records its own argv, so a script that stopped calling the +#: API cannot be graded as though it had -- the instrument reports what it was actually asked. +_FAKE_GH = """#!/usr/bin/env bash +printf '%s\\n' "$*" >> "$FAKE_GH_LOG" +if [ "${FAKE_GH_EXIT:-0}" != "0" ]; then + echo "gh: could not resolve to a PullRequest" >&2 + exit "${FAKE_GH_EXIT}" +fi +printf '%s\\n' "$FAKE_GH_OUT" +""" + + +def _install_fake_gh(workdir: Path) -> Path: + """Write the stand-in and return the directory to prepend to PATH. + + STUBBED THROUGH PATH RATHER THAN THROUGH A HOOK IN THE WORKFLOW. The alternative -- teaching the + shipped step to call `${GH:-gh}` -- would put a test seam in the gate itself, and a seam that can + redirect the read is a seam that can disable it. The step keeps the literal command CI runs; only + what `gh` resolves to changes. + """ + bindir = workdir / "fakebin" + bindir.mkdir(exist_ok=True) + target = bindir / "gh" + target.write_text(_FAKE_GH, encoding="utf-8", newline="\n") + target.chmod(0o755) + return bindir + + def _run_review_gate( - bash: str, script: str, workdir: Path, env: dict[str, str], *, labels: str, action: str -) -> tuple[int, str]: + bash: str, + script: str, + workdir: Path, + env: dict[str, str], + *, + live_labels: str, + action: str, + payload_labels: str | None = None, + gh_exit: int = 0, +) -> tuple[int, str, list[str]]: """Run the step's shell UNDER THE FLAGS ACTIONS USES, and validate the invocation. GitHub runs a `run:` block as `bash --noprofile --norc -e -o pipefail {0}`. Reproducing those @@ -1061,18 +1106,38 @@ def _run_review_gate( 126/127 are a HARNESS fault, never a gate verdict. A caller comparing `code != 0` would otherwise read a broken invocation as "the gate refused this pull request" -- the shape BACKLOG #1216 records, where a mangled script path made six assertions vacuously green. + + ``live_labels`` is what the API answers; ``payload_labels`` is the frozen webhook snapshot and + defaults to the same thing. Passing them DIFFERENTLY is the whole point of BACKLOG #1423: the + verdict has to follow the first and ignore the second. Returns the gh argv log as a third value so + a caller can assert the read happened at all. """ (workdir / "review_gate.sh").write_text(script, encoding="utf-8", newline="\n") + bindir = _install_fake_gh(workdir) + log = workdir / "gh_argv.log" + log.write_text("", encoding="utf-8") proc = _run( [bash, "--noprofile", "--norc", "-e", "-o", "pipefail", "review_gate.sh"], workdir, - {**env, "LABELS": labels, "ACTION": action}, + { + **env, + "PATH": str(bindir) + os.pathsep + env.get("PATH", ""), + "ACTION": action, + "NUMBER": "765", + "GH_TOKEN": "not-a-real-token", + "GH_REPO": "MEFORORG/MessageFoundry", + "PAYLOAD_LABELS": live_labels if payload_labels is None else payload_labels, + "FAKE_GH_LOG": str(log), + "FAKE_GH_OUT": live_labels, + "FAKE_GH_EXIT": str(gh_exit), + }, ) out = _text(proc) assert proc.returncode not in (126, 127), ( f"{explain_returncode(proc.returncode, 'the review-gate step')} Output: {out.strip()[:300]}" ) - return proc.returncode, out + calls = [ln for ln in log.read_text(encoding="utf-8").splitlines() if ln.strip()] + return proc.returncode, out, calls @pytest.fixture @@ -1112,8 +1177,10 @@ def test_the_review_gate_refuses_a_pull_request_nobody_has_marked_read( """ bash, script, workdir, env = review_gate for labels, action, why in _UNREAD: - code, out = _run_review_gate(bash, script, workdir, env, labels=labels, action=action) - print(f"[#1000] review gate labels={labels!r} action={action} exit={code}") + code, out, calls = _run_review_gate( + bash, script, workdir, env, live_labels=labels, action=action + ) + print(f"[#1000] review gate live={labels!r} action={action} exit={code} gh={calls}") assert code != 0, ( f"the review gate PASSED a pull request nobody has read ({why}). labels={labels!r} " f"action={action!r}\n{_ascii(out)}" @@ -1124,19 +1191,19 @@ def test_the_review_gate_refuses_a_pull_request_nobody_has_marked_read( ) -def test_the_review_gate_refuses_a_synchronize_even_when_the_payload_shows_the_label( +def test_the_review_gate_refuses_a_synchronize_even_when_the_label_is_still_there( review_gate: tuple[str, str, Path, dict[str, str]], ) -> None: - """PLANTED: the STALE PAYLOAD, and the single most load-bearing line in this gate. + """PLANTED: `synchronize` must be unread BY DEFINITION, decided before any read. - On `synchronize` the previous step has just removed the label, so the event payload is one step - out of date. Reading it would pass a pull request that was invalidated moments earlier. This is - the one case where the gate must refuse a payload that says `reviewed`, and it is the whole - difference between "a reviewer read THESE commits" and "a reviewer read some earlier ones". + The previous step has just removed the label, so both the payload and a live read are racing the + removal. Answering from the action instead of from a value it could race is the whole difference + between "a reviewer read THESE commits" and "a reviewer read some earlier ones". Both sources are + fed `reviewed` here, so nothing but the action can be producing the refusal. """ bash, script, workdir, env = review_gate - code, out = _run_review_gate( - bash, script, workdir, env, labels="reviewed", action="synchronize" + code, out, _ = _run_review_gate( + bash, script, workdir, env, live_labels="reviewed", action="synchronize" ) assert code != 0, ( "the gate accepted a `synchronize` on the strength of a label the step before it had already " @@ -1155,12 +1222,159 @@ def test_the_review_gate_passes_a_pull_request_a_reviewer_has_marked_read( """ bash, script, workdir, env = review_gate for labels, action, why in _READ: - code, out = _run_review_gate(bash, script, workdir, env, labels=labels, action=action) - print(f"[#1000] review gate labels={labels!r} action={action} exit={code}") + code, out, calls = _run_review_gate( + bash, script, workdir, env, live_labels=labels, action=action + ) + print(f"[#1000] review gate live={labels!r} action={action} exit={code} gh={calls}") assert code == 0, ( f"the gate refused a pull request a reviewer HAD marked read ({why}). labels={labels!r} " f"action={action!r}\n{_ascii(out)}" ) + assert calls, ( + "the gate passed without calling `gh` at all, so it cannot have read the live label set. " + f"labels={labels!r}\n{_ascii(out)}" + ) + + +# --------------------------------------------------------------------------------------------------- +# THE STALE PAYLOAD THAT REPORTED SUCCESS ON AN UNLABELLED PULL REQUEST (BACKLOG #1423). +# --------------------------------------------------------------------------------------------------- +# +# MEASURED ON PULL REQUEST 765, 2026-09-03, head ee2e7ec. A reviewer labelled at 20:43:19Z; the +# `labeled` run 33803911587 was created five seconds later and its payload recorded the label as +# PRESENT; the earlier `synchronize` run 33803677823 sat queued four minutes, executed at 20:45:10Z +# and removed that label at 20:45:14Z; then the `labeled` run's own job started at 20:49:34Z and +# reported SUCCESS at 20:49:37Z from its six-minute-old payload. `a reviewer has read this` was green +# on a pull request carrying zero labels until 20:52:55Z. +# +# NOTHING CORRECTED IT, and that is why the payload read had to go rather than be narrowed. The +# removal is performed by `github-actions[bot]` under GITHUB_TOKEN, and GitHub does not dispatch a +# workflow run from an event raised by that token, so no `unlabeled` run ever fired. Verified against +# the workflow-runs API: exactly two review-gate runs exist on that head, and neither is the removal. +# +# THE OLD SHAPE WAS A ONE-ACTION SCREEN. The workflow already knew the payload went stale and +# hard-coded the single case its author had an instance of (`if [ "$ACTION" = "synchronize" ]`), +# leaving every other action reading the same snapshot. A screen built from one case finds one shape. + + +def test_the_review_gate_refuses_a_stale_payload_that_still_shows_the_label( + review_gate: tuple[str, str, Path, dict[str, str]], +) -> None: + """PLANTED: the measured defect. The payload says `reviewed`; the live label set is EMPTY. + + This is PR 765 replayed at 20:49:37Z. Without this control the fix is unproven -- the shipped + gate and a gate that never looks at the payload are the same green on every other input here. + """ + bash, script, workdir, env = review_gate + code, out, calls = _run_review_gate( + bash, script, workdir, env, live_labels="", payload_labels="reviewed", action="labeled" + ) + print(f"[#1423] stale-payload replay exit={code} gh={calls}\n{_ascii(out)}") + assert code != 0, ( + "the gate reported SUCCESS from a frozen payload while the pull request carried no `reviewed` " + f"label. That is the measured PR 765 false green.\n{_ascii(out)}" + ) + assert calls, "the gate refused without calling `gh`, so it did not read the live set either" + assert "STALE PAYLOAD" in out, ( + "the gate refused correctly but never said WHY, leaving the next reader to reconstruct the " + f"race from three API endpoints and two clocks.\n{_ascii(out)}" + ) + + +def test_the_payload_reading_form_of_the_gate_passes_the_same_planted_pull_request( + review_gate: tuple[str, str, Path, dict[str, str]], +) -> None: + """RUN AGAINST THE PRE-FIX GATE, which is what makes the control above evidence rather than a + claim. The identical fixture, with only the live read reverted to the payload, must go GREEN. + + If both forms refused, the fixture would be proving something else and the recorded defect would + be unreproduced -- the same reasoning as the two-dot control on the hygiene gate above. + """ + bash, script, workdir, env = review_gate + pre_fix = script.replace(_LIVE_READ, 'echo "$PAYLOAD_LABELS"') + assert pre_fix != script, ( + "the live-read substitution matched nothing, so this test compares the shipped gate with " + "itself and can only ever pass" + ) + code, out, _ = _run_review_gate( + bash, pre_fix, workdir, env, live_labels="", payload_labels="reviewed", action="labeled" + ) + print(f"[#1423] pre-fix payload-reading gate exit={code}\n{_ascii(out)}") + assert code == 0, ( + "the payload-reading form no longer reproduces the recorded defect, so the assertion above is " + f"not measuring what it says. exit={code}\n{_ascii(out)}" + ) + + +def test_the_review_gate_clears_when_the_stale_payload_understates_the_live_label( + review_gate: tuple[str, str, Path, dict[str, str]], +) -> None: + """THE OTHER DIRECTION, and it is not padding: the race runs both ways. + + A run created before the label was applied carries a payload with no `reviewed` in it. Under the + old form that pull request stayed red until something else re-ran the job. Reading live means the + verdict follows the label rather than the order two runs happened to be scheduled in. + """ + bash, script, workdir, env = review_gate + code, out, calls = _run_review_gate( + bash, script, workdir, env, live_labels="reviewed", payload_labels="", action="labeled" + ) + print(f"[#1423] understating payload exit={code} gh={calls}\n{_ascii(out)}") + assert code == 0, f"the gate refused a pull request that IS labelled right now\n{_ascii(out)}" + assert "STALE PAYLOAD" in out, "the disagreement went unreported" + + +def test_the_review_gate_fails_closed_when_the_live_label_read_fails( + review_gate: tuple[str, str, Path, dict[str, str]], +) -> None: + """PLANTED: `gh` exits non-zero. The gate must BLOCK, never pass. + + A gate whose safe state depends on a network call succeeding is not a gate, and this is the shape + to avoid rather than a hypothetical: a sibling checker in this repository ignores a child's return + code and reports clean, which is filed separately. Reading live buys nothing if an unreadable + answer is treated as an empty one -- or worse, as a green. + + THE PAYLOAD SAYS `reviewed` HERE ON PURPOSE. A fail-closed arm that only ever ran with an empty + payload could be satisfied by a script that had silently fallen back to the snapshot. + """ + bash, script, workdir, env = review_gate + code, out, calls = _run_review_gate( + bash, + script, + workdir, + env, + live_labels="reviewed", + payload_labels="reviewed", + action="labeled", + gh_exit=1, + ) + print(f"[#1423] unreadable label set exit={code} gh={calls}\n{_ascii(out)}") + assert calls, "the read never happened, so this says nothing about how a failed read is handled" + assert code != 0, ( + "the gate PASSED while it could not read the label set at all. It cannot know whether anybody " + f"read the pull request, so the only safe verdict is to block.\n{_ascii(out)}" + ) + + +def test_the_review_gate_reads_the_live_labels_for_every_action_not_just_one( + review_gate: tuple[str, str, Path, dict[str, str]], +) -> None: + """THE GENERALISATION, asserted rather than described. + + The pre-fix gate special-cased `synchronize` and let every other action read the snapshot. So the + stale payload has to be refused on each action that can carry one -- `synchronize` is excluded + because it is answered before any read, which the control above already pins. + """ + bash, script, workdir, env = review_gate + for action in ("opened", "reopened", "ready_for_review", "labeled", "unlabeled"): + code, out, calls = _run_review_gate( + bash, script, workdir, env, live_labels="", payload_labels="reviewed", action=action + ) + print(f"[#1423] action={action} exit={code} gh={calls}") + assert code != 0, ( + f"the gate passed a stale payload on action {action!r}. A screen built from one case " + f"finds one shape.\n{_ascii(out)}" + ) def test_the_review_gate_still_reports_under_the_required_context_string() -> None: From 03c6cfa7ce2b7b26e908731fea2f8060b3fa2717 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 3 Sep 2026 16:38:53 -0500 Subject: [PATCH 2/2] docs(backlog): file the review-gate false green and the ordering rule (BACKLOG #1423) Records the PR 765 measurement end to end, names the mechanism, and states the limit rather than claiming closure: reading live narrows the window from minutes of queue delay to the seconds between the API call and the job finishing, so the operational rule survives the fix. For a BEHIND pull request, update-branch, wait for the `synchronize` run to COMPLETE, then label, then merge. #1417 filed the same defect on PR 724 and worked out the detection rule; this is a second instance plus the fix, and adds the fact that no `unlabeled` run fires to correct the green. Whether #1417 closes with it is the Lander's call. The erratum records #1422 as a hole: it was allocated from a coordinating session's own worktree and handed to a builder in another, so both ownership keys failed and the ledger gate refused the commit. Re-filed at #1423 unchanged. Co-Authored-By: Claude Opus 5 --- docs/BACKLOG.md | 79 +++++++++++++++++++++++++++++++++++++++++++++++++ docs/CI.md | 5 ++++ 2 files changed, 84 insertions(+) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 4b71b3902..193be7a0e 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -54,6 +54,15 @@ worktree") and the item was re-filed at **#1298**. The gate did its job; the les records the claim against **whatever tree it runs in**, so run it from the worktree that will commit. Always allocate with `scripts/coord/alloc.ps1`; never pick a number by reading this file. +**#1422 is a hole of the same shape as #1297, and it earns a line because the cause is a DIFFERENT +tree rather than the wrong one.** On 2026-09-03 a coordinating session allocated it from its own +worktree and handed the number to a separate builder session, in another worktree and on another +branch. Ownership is keyed on the worktree, with the branch as a fallback that only becomes reachable +once that worktree is GONE โ€” and it was live โ€” so both keys failed and the ledger gate refused the +builder's commit, correctly. The item was re-filed at **#1423** with its text unchanged. `alloc.ps1` +has no transfer verb by design, because one would let a seat take a number another session is holding, +so **allocate in the tree that will commit, never on another session's behalf.** + **If you allocated a backlog number before 2026-07-31T00:31Z, re-check it โ€” the trigger is the timestamp, not the value.** That is when the floor fix landed. Any number issued before it came from a floor that could not see most of the namespace, so it is suspect **regardless of how low or high it @@ -19928,3 +19937,73 @@ That is the same `self._lock` the staged-pipeline handoffs take. On a first depl **PARTLY CLOSED ALREADY, AND THE CLOSURE SITS IN THE WRONG ARTIFACT.** The full record -- both questions, all eight options, both answers quoted -- is [comment 5515263760 on PR 749](https://github.com/MEFORORG/MessageFoundry/pull/749#issuecomment-5515263760), written 2026-09-02. A pull-request comment is a real improvement on a session transcript, which does not survive its session. It is still not the ADR, and the ADR is what a reader consults. **This limb differs from the first two in shape:** closing it needs no decision about the engine, only the record moved into the artifact people actually read. **THE GENERAL PROBLEM, stated once so it is not re-derived per incident.** A decision recorded as an outcome plus a delegation is not reviewable. The inputs -- the question, the options, the answer -- are what let a later reader tell a considered call from an arbitrary one, and they are exactly the part that lives in the least durable place. + +## 1423. The review gate passes green with no reviewed label: a queued synchronize run strips the label after the reviewer applies it, and the labeled run then reads a stale payload + +> ๐Ÿšง **Filed 2026-09-03 -- the fix is in the pull request that files this row.** `a reviewer has read this` is a required status check on `main`, and with `required_approving_review_count` at 0 it is this repository's entire automated review requirement. **It reported SUCCESS on pull request 765 while that pull request carried zero labels.** The workflow now reads the label set live from the API instead of out of the frozen webhook payload, and BLOCKS when that read fails. +> +> **Scored 2026-09-03 -> P2.** Value **7/10** ยท Difficulty **3/10** ยท _quick win_. Value 7 rather than #1417's 6, on one fact #1417 does not carry: the correction a reader would assume exists does not fire, so a false green stands until a person notices it. Difficulty 3, which is what #1417 predicted: the workflow edit is small and the cost was the control harness, welded to a `$LABELS` env contract and needing a `gh` stand-in on PATH. +> Verdict: build +> Research: none +> Closing-act: code + +**Cluster:** CI gates / merge protection. **Priority:** P2. **Verdict:** build. +**Severity:** no engine effect, no PHI axis, and **no deployment axis (sec. 0)** -- nothing here reaches shipped code. What it reaches is this repository's own merge control, so the cost is an unreviewed change landing on `main`. **It does not mean past merges were unreviewed.** The race needs a specific ordering, and #1417's replay over the last 25 merged pull requests found every deciding run created after its last `reviewed` event. + +**RELATIONSHIP TO #1417, STATED FIRST so the two are not read as one finding counted twice.** #1417 filed this defect on pull request 724 and worked out the detection rule; its remedy list already names reading the labels live as one of two options. This row is a second, independent instance on pull request 765, it takes that option, and it adds one fact #1417 does not have. **Whether #1417 closes alongside this is the Lander's call, not this row's.** + +### The measurement, pull request 765, 2026-09-03 + +Every timestamp below is a quoted API value from the runs, jobs and issue-timeline endpoints, on head `ee2e7ec2423a73fd385d2afa27733ca050058cba`. + +| time (UTC) | event | +|---|---| +| 20:40:58Z | run `33803677823` created by a Builder's `synchronize` push | +| 20:43:19Z | `wshallwshall` applies `reviewed` | +| 20:43:24Z | run `33803911587` created by that `labeled` event -- **its payload records the label as PRESENT** | +| 20:45:10Z | the `synchronize` run's job finally starts, four minutes after creation | +| 20:45:11-20:45:15Z | its step 3 removes the label; `github-actions[bot]` unlabels at 20:45:14Z | +| 20:45:16Z | that run concludes FAILURE (check-run `100808902697`) | +| 20:49:34Z | the `labeled` run's job starts, **six minutes** after creation | +| 20:49:37Z | it reads its own 20:43:24 payload, finds `reviewed`, and concludes **SUCCESS** (check-run `100809679433`) | +| 20:49:37-20:52:55Z | pull request 765 carries `a reviewer has read this` = success **with zero labels** | +| 20:52:55Z | a person re-applies the label by hand, which creates a run whose payload is honest | + +The success is the newer check-run on the head, so it is the one branch protection reads. + +### The mechanism + +1. A push fires `synchronize`. That run is queued, here for four minutes. +2. A reviewer, seeing a red gate, applies the label. That fires a `labeled` run whose payload captures the label as present. +3. The queued `synchronize` run starts and deletes the label. Correct in isolation -- that removal is the whole re-review mechanism and must stay. +4. The `labeled` run then evaluates its OWN payload, now stale by minutes, sees a label that no longer exists, and passes. + +**THE PAYLOAD IS A SNAPSHOT AND THE VERDICT IS NOT.** Those are two clocks, and the gate was reading the older one. + +### And nothing corrects it, which is why the payload read had to go + +The workflow header used to claim `unlabeled` was *"the opposite half"* that stops a withdrawn label leaving a green context. **That reasoning does not hold for this removal.** GitHub does not dispatch a workflow run from an event raised by the repository's own `GITHUB_TOKEN` -- `workflow_dispatch` and `repository_dispatch` are the documented exceptions, and a label event is neither. The removal runs `gh` under `github.token`, so its `unlabeled` event emits nothing. + +**Verified rather than inferred:** the workflow-runs API reports exactly **two** review-gate runs on that head, the two named above, and none for the 20:45:14Z removal. `unlabeled` therefore covers a **person** taking the label back, and nothing else. The header now says that. + +### What the fix does, and the three properties that constrain it + +The label-reading step now runs `gh pr view "$NUMBER" --json labels --jq '[.labels[].name] | join(",")'` and decides on the result. A gate must assert about the present. + +1. **It fails closed on a failed read.** `gh` absent, unauthenticated, rate-limited or answering with an error all BLOCK. A gate whose safe state depends on a network call succeeding is not a gate. +2. **The `synchronize` removal stays.** Deleting the label on new commits is correct. +3. **`synchronize` still fails by definition**, decided before any read rather than from a value it could race. Commits nobody has read are unread whatever a label says. + +The payload is still passed in, as a **diagnostic only**: when it and the live set disagree about `reviewed`, the log says so in words. Nothing branches on it. That line exists so the next reader is told the payload went stale instead of reconstructing it from three endpoints and two clocks. + +**THE OLD SHAPE WAS A ONE-ACTION SCREEN, and that is the generalisable lesson.** The workflow already knew the payload went stale -- its own comment said so -- and hard-coded a fix for the single action its author had an instance of, leaving every other action reading the same snapshot. A screen built from one case finds one shape. + +### What the fix does NOT do, said plainly + +**It narrows the window; it does not close it to zero.** A `labeled` run can still read a label that a `synchronize` run removes moments later, and its green then stands on a state that has just changed. The difference is the size: the payload read cost **minutes** of queue delay -- six, measured above -- while what remains is the gap between the API call and the job finishing, which is seconds. Claiming closure here would be the compensating control resting on a false premise that section 11 forbids. + +So the operational rule survives the fix rather than expiring with it. **For a BEHIND pull request: update-branch, WAIT for the resulting `synchronize` review-gate run to COMPLETE, then label, then merge.** Labelling while that run is still queued is what produced the false green above, and it is still the shape to avoid. Closing the residual entirely needs a different mechanism -- pinning the verdict to the head and the label event, which is #1417's rule (4) plus (5) -- and that is not built here. + +### Controls + +`tests/test_merge_gate_controls.py` lifts the step's own shell out of the workflow and runs it against a `gh` stand-in placed first on PATH, which records its own argv so a gate that stopped calling the API cannot be graded as though it had. Both directions are pinned: a stale payload showing `reviewed` over an empty live set must FAIL on every action the gate can see, a genuinely labelled pull request must PASS, and an unreadable label set must BLOCK. **The pre-fix form is run against the identical planted fixture and must go GREEN**, so the shipped refusal is measured against a form known to accept rather than merely asserted. `tests/negative_controls.toml` carries the registry rows. diff --git a/docs/CI.md b/docs/CI.md index 4eadadfd9..698a093c1 100644 --- a/docs/CI.md +++ b/docs/CI.md @@ -76,6 +76,11 @@ which makes this single context the repository's whole review requirement. A PR `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. +**Do not label a PR while a `synchronize` review-gate run is still queued.** That run removes the +label when it finally executes, and the ordering has produced a green context on an unlabelled PR +(BACKLOG #1423). For a PR that is BEHIND: update-branch, wait for that run to COMPLETE, then label, +then merge. + 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 things keep them advisory: they are absent from branch protection, and every analysis step is