From 51f11bb2fb09bd4ba36b6414fba782f3d230693d Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 3 Sep 2026 16:41:11 -0500 Subject: [PATCH 1/2] fix(ci): read the reviewed label live inside the job, not from the frozen event payload (BACKLOG #1417) `a reviewer has read this` is a required status check on `main` with `enforce_admins: true`, and it could report SUCCESS on a pull request carrying no `reviewed` label. The step read `join(github.event.pull_request.labels.*.name, ',')`, which is the label set as it stood when the webhook FIRED, while branch protection picks the newest check-run by EXECUTION time. Measured on PR 724 on 2026-09-01: a run created 13:24 executed at 13:43:46, twenty-odd seconds after the label was removed, and reported success from its 13:24 payload. That success stood for ten minutes with no label on the pull request; only `strict = true`, an unrelated control, kept it from merging. THE STEP NOW READS THE LABEL LIVE, with `gh pr view --json labels` from inside the running job. Reading the status context instead is not a fix -- it inherits the same staleness through the same snapshot -- and had already been adopted and refuted twice. AND THE LABEL MUST POST-DATE THE HEAD IT SITS ON. A live read answers "is the label there now", never "did anyone read THESE commits". So the step also compares the newest `reviewed` labeled event against the head commit's date and refuses a label older than the commit it sits on. Where the run was started by a reviewer applying the label, that comparison is answered by the event itself and no history read happens: a GitHub clock, and the one timestamp that cannot lag a write made seconds earlier. The history read uses `per_page=100` with `--paginate`, because every `gh api` list route defaults to 30. THE `synchronize` ARM STAYS, and it is no longer the fix. The removal step runs immediately before it and the labels endpoint is not guaranteed to have caught up; and the head-date comparison rests on a commit's committer date, which a client supplies, so a commit authored before the label and pushed after it would clear that comparison while being genuinely unread. What this removes is the other half: every action EXCEPT `synchronize` reading the snapshot. IT PASSES MORE OFTEN IN EXACTLY ONE DIRECTION, and that is stated rather than left to be found. Where the payload was stale-positive the gate now refuses. Where it was stale-negative -- a run queued before the label was applied, executing after -- it now passes, because at execution time the label is present and post-dates the head. That is the correct verdict on a state the old step could only get wrong. The head-date comparison only ever ADDS a refusal, so a spoofed commit date cannot open the gate, and every failure of the two API reads exits non-zero under `-e`, which blocks. PERMISSIONS. Declaring any permission sets the rest to `none`, so the two reads are named: `contents: read` for the head commit's date and `issues: read` for the label events. A 403 would fail closed, which is the right direction but would wedge every pull request. MEASURED BEFORE AND AFTER, one input, both shells run under the flags Actions uses. The pre-fix shell was lifted from `git show HEAD:.github/workflows/review-gate.yml` rather than retyped, and fed PR 724's state -- payload carries `reviewed`, live read returns nothing: before exit 0 "reviewed label present. Gate satisfied." after exit 1 "labels, read at execution time: (none)" plus the remedy CONTROLS. Four new planted violations and three new asymmetry arms in tests/test_merge_gate_controls.py, registered in tests/negative_controls.toml. The gate's own shell is still lifted from the workflow rather than re-implemented; its `gh` calls are answered by a shim whose unanswered call is a loud harness fault, not a silent refusal -- empty output reads as "no label" everywhere in this step, so a broken shim would have looked exactly like the gate working. The static arm's detector is asserted against its own false positive: the workflow's comment NAMES the banned expression, and a whole-file scan would have "fixed" that by deleting the sentence recording this item. NO DEPLOYMENT AXIS. This reaches the repository's own merge control, not shipped code. The cost is an unreviewed change landing on `main`. Co-Authored-By: Claude Opus 5 --- .github/workflows/review-gate.yml | 93 +++++++- CLAUDE.md | 5 +- docs/BACKLOG.md | 59 ++++- docs/METHOD.md | 7 +- tests/negative_controls.toml | 44 +++- tests/test_merge_gate_controls.py | 358 ++++++++++++++++++++++++++++-- 6 files changed, 525 insertions(+), 41 deletions(-) diff --git a/.github/workflows/review-gate.yml b/.github/workflows/review-gate.yml index 134980c47..ded7b9308 100644 --- a/.github/workflows/review-gate.yml +++ b/.github/workflows/review-gate.yml @@ -34,6 +34,25 @@ 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, FROM INSIDE THE JOB (BACKLOG #1417). This step used to read +# `join(github.event.pull_request.labels.*.name, ',')` -- the label set as it stood when the WEBHOOK +# FIRED. A queued run therefore reported the truth of its own CREATION time, while branch protection +# picks the newest check-run by EXECUTION time. Two clocks, and when they disagree the older one is +# what gets reported. Measured end to end on PR 724 on 2026-09-01: a run created 13:24 executed at +# 13:43:46, twenty-odd seconds after the label was removed, and reported SUCCESS from its 13:24 +# payload; that success stood for ten minutes on a pull request carrying no `reviewed` label. Nothing +# but `strict = true` -- an unrelated control -- kept it from merging. +# +# READING THE STATUS CONTEXT INSTEAD IS NOT A FIX, and was adopted by two sessions before being +# refuted: the context inherits the same staleness through the same snapshot. A lower-level proxy of +# a snapshotted value is still the snapshot. The only live reading is an API call made while the job +# is running, which is what the label step now does. +# +# AND THE LABEL HAS TO POST-DATE THE HEAD IT SITS ON. A live read answers "is the label there now"; +# it does not answer "did it ever cover THESE commits". Label a pull request, push to it, and read +# the state before the `synchronize` strip lands, and the label is live-present on a head nobody has +# read. So the step makes a second comparison, and refuses a label older than the head. +# # 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 @@ -58,6 +77,15 @@ on: merge_group: permissions: + # `pull-requests: write` is the label REMOVAL on `synchronize`, and the only write here. + # + # THE TWO READS ARE WHAT THE LIVE CHECK COSTS. Declaring any permission sets every undeclared one + # to `none`, so the label step's API calls would 403 without them: `contents: read` for the head + # commit's date, `issues: read` for the pull request's label events -- label history lives on the + # issue side of a pull request, not the pulls side. A 403 would fail the gate CLOSED, which is the + # right direction but would wedge every pull request, so both are named rather than assumed. + contents: read + issues: read pull-requests: write jobs: @@ -102,21 +130,72 @@ 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 }} + # THESE THREE ARE FACTS ABOUT THE EVENT, NOT STATE READ THROUGH IT, and the distinction is + # the whole of #1417. `ACTION` and `LABEL_NAME` describe what happened; an event cannot go + # stale about itself. `HEAD_SHA` is the commit this job's check-run ATTACHES to, so it is + # the subject of the verdict rather than a mutable property of it. Everything the verdict + # DEPENDS on -- whether the label is on the pull request, and when it was applied -- is + # fetched below, at execution time, and never taken from the payload. ACTION: ${{ github.event.action }} + LABEL_NAME: ${{ github.event.label.name }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} 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. + # A NEW COMMIT IS UNREAD BY DEFINITION, and this arm is kept as a belt over the live read + # rather than as the fix for staleness -- it used to be the fix, applied to the one action + # anyone had an instance of. Two reasons it stays. The step above has just removed the + # label, and the labels endpoint is not guaranteed to have caught up by the time this step + # reads it. And the head-date comparison at the bottom rests on a commit's COMMITTER DATE, + # which is client-supplied: a commit authored before the label and pushed after it would + # clear that comparison while being genuinely unread. if [ "$ACTION" = "synchronize" ]; then echo "::error::New commits have not been read. Re-review, then: gh pr edit --add-label reviewed" exit 1 fi + + # LIVE. Not `github.event.pull_request.labels`, which is the snapshot #1417 records. + LABELS="$(gh pr view "$NUMBER" --json labels --jq '[.labels[].name] | join(",")')" + echo "labels, read at execution time: ${LABELS:-(none)}" + + # EXACT ELEMENT, not substring: `reviewed-by-bot` and `not-reviewed` are not a review. case ",$LABELS," in - *,reviewed,*) - echo "reviewed label present. Gate satisfied." ;; + *,reviewed,*) ;; *) echo "::error::Not yet read by a reviewer. When you have read it: gh pr edit --add-label reviewed" exit 1 ;; esac + + # THE SECOND COMPARISON: the label must post-date the head it sits on. + # + # THE CHEAP ANSWER FIRST, and it is also the better-founded one. If a reviewer applied + # `reviewed` in the very event that started this run, the label was applied while this head + # already existed -- that is a GitHub clock, needs no API call, and cannot lag behind a + # write made moments earlier. + if [ "$ACTION" = "labeled" ] && [ "$LABEL_NAME" = "reviewed" ]; then + echo "the reviewed label was applied by the event that started this run. Gate satisfied." + exit 0 + fi + + # Otherwise ask when it was last applied. `per_page=100` AND `--paginate`: every `gh api` + # list route defaults to 30, and a label history read 30 at a time answers a question about + # a population it cannot see. `sort | tail -n 1` rather than trusting the page order. + LAST_ADD="$(gh api "repos/$GH_REPO/issues/$NUMBER/events?per_page=100" --paginate \ + --jq '.[] | select(.event == "labeled" and .label.name == "reviewed") | .created_at' \ + | sort | tail -n 1)" + HEAD_AT="$(gh api "repos/$GH_REPO/commits/$HEAD_SHA" --jq '.commit.committer.date')" + echo "reviewed last applied ${LAST_ADD:-(no recorded event)}; head $HEAD_SHA dated $HEAD_AT" + + if [ -z "$LAST_ADD" ]; then + echo "::error::The reviewed label is on this pull request but no labeled event records when it was applied, so it cannot be shown to cover this head. Re-apply it: gh pr edit --remove-label reviewed && gh pr edit --add-label reviewed" + exit 1 + fi + + # Both timestamps are fixed-width ISO-8601 in UTC, so a string compare is a time compare. + if [[ "$LAST_ADD" < "$HEAD_AT" ]]; then + echo "::error::The reviewed label was applied at $LAST_ADD, before this head was dated $HEAD_AT, so it never covered these commits. Re-review, then: gh pr edit --remove-label reviewed && gh pr edit --add-label reviewed" + exit 1 + fi + + echo "reviewed label present at execution time and applied after this head. Gate satisfied." diff --git a/CLAUDE.md b/CLAUDE.md index 23797a554..793d8162a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -411,7 +411,10 @@ went red, and no workflow reads that label back. So the Console finds both by as queued synchronize run has not stripped the label yet, so the label can be present and invalid at the same time. When no run is newer than the label event at all, the state is unknown: the Console keeps polling, and nobody inherits the last verdict. A Builder never evaluates this, because its - process exits before any run reports. Filed as BACKLOG #1417, open in PR 731 and not yet on main. + process exits before any run reports. BACKLOG #1417 was the gate's own half of the same staleness + and it is fixed: the gate reads the label live inside the job, so a SUCCESS is no longer a + snapshot. The join above still binds, because a run that has not reported yet is still not a + verdict. - Never write the required-context count into a document. `.github/required-contexts.txt` is a checked-in claim that can lag the server, so read branch protection for the live set. When the set moves, move that file and the pinned count in `tests/test_required_contexts.py` in the same PR, or diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 4b71b3902..7294d22f3 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -212,7 +212,7 @@ Ordered by value descending, then difficulty ascending (cheapest first at equal | 9 | **#1385** | three merge-queue attempts on one PR failed three DIFFERENT unrelated tests, and the PR gate cannot see any of it | 6 | 3 | _quick win_ | P2 | partly shipped | Partly shipped, and more has landed since filing than the first pass credited. The visibility half is in -- .github/workflows/failure-signal.yml:71 recovers the pull request number from a merge_group ref and :92 applies ci-red, so an ejection is recorded, though nothing in this tree reads that label (grep for ci-red returns the writing workflow plus prose at CLAUDE.md:302 and docs/METHOD.md:365). The windows-2025 hang limb is no longer undiagnosed either: .github/workflows/ci.yml:852 now passes --max-worker-restart=0, landed 2026-09-02 in 042ef7ff5, and the comment from :826 to :850 reads the mechanism out of the pinned pytest-xdist and records four hangs of 25 to 46 minutes with the inner watchdogs armed, which is this row's process-level deadlock below pytest. What is left is two tests -- tests/test_api_request_timeout.py:99 still runs a route against a 0.1 second deadline, and tests/test_sqlserver_store.py:4039 is unchanged and runs at ci.yml:1772 under a wrapper that retries only a native crash (ci.yml:1769), never exit 1; the hostile-disposition test the row names was already a ratio assertion before filing (tests/test_multipart.py:142). Difficulty falls to 3 because the hard limb landed and the seams exist -- pyproject.toml:195 already ships pytest-rerunfailures and tests/test_load_failover_sqlserver.py:71 marks a sibling flaky -- with the SQL Server arm provable only on the gated CI leg. | | 10 | **#1393** | four open rows say their work ALREADY SHIPPED and must not be rebuilt, and every dispatch screen passes them as buildable because the verb is REBUILD not BUILD | 6 | 3 | _quick win_ | P2 | not started | Not started, and I could find no body read anywhere on the dispatch path. judge() at scripts/coord/dispatch_gate.py:134 reads only item.fields at :152 to :154, the string MUST BE READ appears nowhere under scripts/, and the repo's only DO NOT REBUILD matcher is scripts/coord/claim-adjudicate.ps1:170, which adjudicates claim release rather than dispatch and so answers the opposite question. Re-running parse_items over docs/BACKLOG.md at HEAD, all four named rows (#1107, #1130, #1183, #1242) are still open, 36 open rows carry rebuild or already-shipped language on this row's own needle set, and #1020 still holds both its dead bar and the sentence retiring it. Value is a lane-window per occurrence with only the awkward workaround of reading every row, capped below 8 because this is fleet tooling with no product, PHI or deployment axis. The build is a body read, a needle set and a MUST BE READ level on one existing function plus must-fire and must-not-fire arms, and the unmerged #1334 retirement limb at eaf6d0940 already proves that seam. | | 11 | **#1398** | a row can be fully built with nothing in its text saying so, and no ledger-reading screen can detect it -- ask the tree, not the banner | 6 | 3 | _quick win_ | P2 | not started | Not started, and there is a live instance of the class. Nothing on the dispatch path asks the tree: scripts/coord/dispatch_gate.py:152 reads only banner fields, and a grep across scripts/ finds only scripts/hooks/claim_check.py:44, which matches a commit message rather than the tree, plus the citation checkers. The class is real -- #1328's remaining limb shipped at scripts/asvs/rescore_handoff_check.py:5 and both f6c96b3b1 and the f769316fa hardening are ancestors of main, while its banner still reads Filed 2026-08-22 - not started -- and I sized the population by grepping origin/main for the literal BACKLOG #N form across tests/, scripts/, messagefoundry/ and .github/: 118 of 275 open rows are cited by code that landed. Value is 6 rather than 7 because the row publishes and prices its own workaround, a per-candidate git grep costing four minutes for nine rows, which is awkward rather than absent, and because a 43 percent flag rate means the output has to be MUST BE READ rather than a verdict or it becomes the noise failure #1394 names. The build is that grep wired into the dispatch path with both controls, a known-unbuilt row returning zero and a known-built row returning nonzero. | -| 12 | **#1417** | the review gate can report success on a head nobody reviewed: it reads the label from a snapshotted event payload | 6 | 3 | _quick win_ | P2 | not started | Not started, and the defect is intact at HEAD: .github/workflows/review-gate.yml:105 still reads LABELS out of the frozen webhook payload, and line 112 still hard-codes the remedy for the single synchronize action, so any other action can report SUCCESS from a payload snapshotted before the label moved. That gate is the repository's entire automated review requirement, since .github/required-contexts.txt:160 to :166 pins approvals at 0 and states that nothing else reports that a green pull request was never read. VALUE LOWERED 7 to 6, because a workaround exists and is already written down: CLAUDE.md:407 to :414 makes the reader compare the gate run's originating createdAt against the newest reviewed label event, and CLAUDE.md:287 forbids the Lander from merging a pull request carrying no reviewed label, so a seat that reads the label live is not fooled by the stale green. It is awkward rather than clean, because the documented join is this item's rule (4) alone and returns a false clean when the label predates the head. Difficulty 3 holds: the workflow edit is small, but tests/test_merge_gate_controls.py:1044 to :1069 welds the existing control suite to the $LABELS and $ACTION env contract, so reading labels live forces a gh stub into that harness on top of the new staleness assertion. | +| 12 | **#1417** | the review gate can report success on a head nobody reviewed: it reads the label from a snapshotted event payload | 6 | 3 | _quick win_ | P2 | shipped | Shipped: the label step now reads the label with gh pr view at execution time and additionally refuses a label older than the head it sits on, and the pre-fix shell was measured passing the same input the fixed one refuses. Scored while the defect was still intact at HEAD, where .github/workflows/review-gate.yml:105 read LABELS out of the frozen webhook payload and line 112 hard-coded the remedy for the single synchronize action, so any other action could report SUCCESS from a payload snapshotted before the label moved. That gate is the repository's entire automated review requirement, since .github/required-contexts.txt:160 to :166 pins approvals at 0 and states that nothing else reports that a green pull request was never read. VALUE LOWERED 7 to 6, because a workaround exists and is already written down: CLAUDE.md:407 to :414 makes the reader compare the gate run's originating createdAt against the newest reviewed label event, and CLAUDE.md:287 forbids the Lander from merging a pull request carrying no reviewed label, so a seat that reads the label live is not fooled by the stale green. It is awkward rather than clean, because the documented join is this item's rule (4) alone and returns a false clean when the label predates the head. Difficulty 3 holds: the workflow edit is small, but tests/test_merge_gate_controls.py:1044 to :1069 welds the existing control suite to the $LABELS and $ACTION env contract, so reading labels live forces a gh stub into that harness on top of the new staleness assertion. | | 13 | **#1420** | the connscale final sample is taken after stop and drain while its docstrings call it in-hold | 6 | 3 | _quick win_ | P2 | not started | Not started -- the ordering is unchanged at harness/load/connscale/runner.py:441 (sampler_stop.set), :450 (driver.stop), :451 (await_drain), :454 (sleep _SETTLE, defined at :92) and :474 (samples.append(final)), _empty_claim_rates still reads samples[0] and samples[-1] at runner.py:1153, and all five sites still call that window in-hold (report.py:119, runner.py:922, runner.py:1150, runner.py:1166, tests/test_connscale_empty_claims_per_msg.py:12). Value holds at 6 on stronger evidence than the harvest argument: the same window feeds a LIVE gate, empty_claims_base_reading at runner.py:1441, switched on at tests/test_connscale_smoke.py:110 and asserted at :539, and the post-drain tail adds empty claims with no reads, so a sign test built to fire on a dead in-hold counter can be satisfied by the tail instead. Left to build is the measurement the row refuses to pre-empt, one run per sweep cell with the final sample in and out, then either narrowing the rate window inside _build_record (runner.py:867) or correcting all five sentences. That is a measurement run plus a small additive change on an existing seam, with tests, so difficulty 3. | | 14 | **#1349** | a rescue ref can silently hold an ancestor instead of the tip, and the population where that matters is the population where it cannot be checked | 6 | 4 | _quick win_ | P2 | partly shipped | Partly shipped, and I re-ran the audit rather than trust the claim. The write-time control is real -- scripts/coord/rescue.ps1:166 writes an annotated tag recording branch, commit, was-tip and instant, scripts/coord/rescue.ps1:276 reads it back after the branch is gone, and tests/test_rescue_ref_provenance.py holds 12 tests over it. What is left is the population it grades: `rescue.ps1 -Check` run in this checkout reports EXAMINED 1671, UNVERIFIABLE 1671, because zero refs under refs/rescue or refs/tags/rescue carry the mefor-rescue-v1 marker defined at scripts/coord/rescue.ps1:83. Three writer surfaces still bypass it -- the post-commit hook force-pushes a bare lightweight tag at scripts/hooks/durability_push.sh:102, scripts/coord/unbacked_check.ps1:459 prints that same un-provenanced push as its remediation, and the audit reads only two namespaces at scripts/coord/rescue.ps1:111 while 1455 refs sit under refs/remotes/private/rescuetags. Landing the remainder means a provenance design for an sh hook that must never fail a commit, pointing the printed remedy at -Anchor, widening the audit, and Windows-gated tests for each. | | 15 | **#1359** | the worktree gate hands off branch-switch detection to rule 3b by verb, not by whether the command names a branch | 6 | 4 | _quick win_ | P2 | not started | Not started, and I re-located every limb by symbol rather than by the filed line. Test-WorktreeHijack at scripts/hooks/worktree_gate.ps1:1411 still opens with a return unless the verb is checkout or switch (:1412), while the gated verb list at :2282 carries twelve, and Test-Governed at :1360 exempts nested .claude/worktrees paths at :1365 -- so rule 3 declines a linked worktree and rule 3b declines the other ten verbs, leaving reset --hard, a fast-forward merge and rebase onto an existing branch unevaluated as branch changes on another session's checkout. The gate states the gap in its own words at :341, and the pinned safe form a naive patch would red is at tests/test_worktree_gate.py:580. Value 6 for a hole in a guard believed to govern every session, matching the table's own precedent for a worktree-gate hole, with no workaround beyond noticing afterwards. Difficulty 4 because the work is deciding per verb which ones actually move a governed worktree's HEAD, measuring the fail-open axis as #1229 requires and not only the false denies, in a 2840-line file with a regression history and live claims on it. | @@ -19766,7 +19766,7 @@ Four things read those files, and none can interrupt anything: `usage.ps1` on de **Not in scope:** giving the watcher an account. It makes no model calls, so it cannot be exhausted by what it watches. The hazard runs the other way: the usage endpoint returns 429 PER ENDPOINT rather than per caller, proven inside a single process, so the design wants ONE reader and a dedicated account would add one. ## 1417. the review gate can report success on a head nobody reviewed: it reads the label from a snapshotted event payload -> 🔢 **Filed 2026-09-01 (lander-5eaa4e) -- not started.** `a reviewer has read this` is a required status check on `main` with `enforce_admins: true`, so it is the control that stands between an unread diff and `main`. **It can be satisfied by a pull request carrying no `reviewed` label at all.** Reproduced on PR 724, with the window measured end to end. +> 🔢 **Filed 2026-09-01 (lander-5eaa4e). BUILT IN THIS COMMIT, not yet landed** -- see "What landed" at the foot of this item; the scoring paragraph below is left verbatim as the dated measurement it was. `a reviewer has read this` is a required status check on `main` with `enforce_admins: true`, so it is the control that stands between an unread diff and `main`. **It can be satisfied by a pull request carrying no `reviewed` label at all.** Reproduced on PR 724, with the window measured end to end. > > **Scored 2026-09-03 -> P2.** Value **6/10** · Difficulty **3/10** · _quick win_. Not started, and the defect is intact at HEAD: .github/workflows/review-gate.yml:105 still reads LABELS out of the frozen webhook payload, and line 112 still hard-codes the remedy for the single synchronize action, so any other action can report SUCCESS from a payload snapshotted before the label moved. That gate is the repository's entire automated review requirement, since .github/required-contexts.txt:160 to :166 pins approvals at 0 and states that nothing else reports that a green pull request was never read. VALUE LOWERED 7 to 6, because a workaround exists and is already written down: CLAUDE.md:407 to :414 makes the reader compare the gate run's originating createdAt against the newest reviewed label event, and CLAUDE.md:287 forbids the Lander from merging a pull request carrying no reviewed label, so a seat that reads the label live is not fooled by the stale green. It is awkward rather than clean, because the documented join is this item's rule (4) alone and returns a false clean when the label predates the head. Difficulty 3 holds: the workflow edit is small, but tests/test_merge_gate_controls.py:1044 to :1069 welds the existing control suite to the $LABELS and $ACTION env contract, so reading labels live forces a gh stub into that harness on top of the new staleness assertion. > Verdict: build @@ -19857,6 +19857,61 @@ The author identified this failure mode precisely, named its consequence exactly **Related:** #1413 covers the absence of any trigger that notifies a reviewer; this is the complementary defect in what the gate *reports* once a label exists. +### What landed, 2026-09-03 + +**THE STEP READS THE LABEL LIVE.** `gh pr view "$NUMBER" --json labels` inside the running job, in +place of `join(github.event.pull_request.labels.*.name, ',')`. That is a reading at execution time, +which is the clock branch protection uses to pick a check-run, so the two clocks no longer disagree. +Reading the status context instead was not adopted, for the reason recorded above: it inherits the +same snapshot. + +**AND THE LABEL MUST POST-DATE THE HEAD, which is rule (5) rather than rule (4).** A live read +answers *"is the label there now"* and never *"did anyone read THESE commits"*, so the step also +compares the newest `reviewed` **labeled** event against the head commit's date and refuses a label +older than the commit it sits on. Where the run was started by a reviewer applying the label, that +comparison is answered by the event itself and no history read happens -- a GitHub clock, and the one +timestamp that cannot lag behind a write made seconds earlier. + +**THE `synchronize` ARM STAYED, and it is no longer the fix.** Two reasons it is a belt worth +keeping rather than a special case to delete. The removal step runs immediately before, and the +labels endpoint is not guaranteed to have caught up. And the head-date comparison rests on a +commit's **committer date**, which a client supplies: a commit authored before the label and pushed +after it clears that comparison while being genuinely unread. What the fix removed is the *other* +half -- every action except `synchronize` reading the snapshot. + +**A SPOOFED OR ODD COMMIT DATE CANNOT OPEN THE GATE.** The head-date comparison only ever adds a +refusal, so defeating it leaves the live label read standing; it cannot turn a refusal into a pass. +Every failure of the two API reads exits non-zero under `-e`, which blocks. + +**IT DOES PASS MORE OFTEN IN ONE DIRECTION, said plainly.** Reading live cuts both ways. Where the +payload was stale-**positive** the gate now refuses, which is the defect this item filed. Where it +was stale-**negative** -- a run queued before the label was applied, executing after -- the gate now +passes, because at execution time the label is there and it post-dates the head. That is the correct +verdict on a state the old step could only get wrong, and the sign convention above is why it is not +a relaxation: a stale FAILURE was safe but still wrong, and it wedged a pull request nothing else +would re-run. + +**MEASURED BEFORE AND AFTER, one input, both shells run under the flags Actions uses.** The pre-fix +shell was lifted from `git show HEAD:.github/workflows/review-gate.yml` rather than retyped, and fed +PR 724's 13:24-vs-13:43 state -- payload carries `reviewed`, live read returns nothing: + +| shell | exit | printed | +|---|---|---| +| before | 0 | `reviewed label present. Gate satisfied.` | +| after | 1 | `labels, read at execution time: (none)` and the remedy | + +**The controls live beside the existing ones** in `tests/negative_controls.toml` and +`tests/test_merge_gate_controls.py`: four new planted violations and three new asymmetry arms, with +the `gh` calls answered by a shim whose unanswered call is a loud harness fault rather than a silent +refusal -- empty output reads as "no label" everywhere in this step, so a broken shim would have +looked exactly like the gate working. + +**NOT FIXED HERE, named rather than left to be found.** The eight open pull requests with no gate +check-run on their head at all are a separate gap in this item's body and are untouched. So is the +absence of any artifact answering *"what was required when this merged"*, and PR 712's unresolved +merge with a `failure` verdict. And this changes nothing about what the gate MEANS: it enforces that +a step happened, not that an independent party looked. + ## 1421. Record what the #1277 grant-trail default costs: unbounded audit_log growth, a standalone commit per authenticated read, and a decision record outside the ADR > 🔢 **Filed 2026-09-02 -- not started. THIS ROW RECORDS COSTS AND DOES NOT PICK A FIX.** PR 749 landed #1277, so on the shipped default `[security].audit_all_authorization_decisions` and the `[diagnostics].audit_all_authz` field it desugars to are both `true`, and every authenticated request on a `require()`-gated route writes one `auth.permission_granted` row. **At least four costs came with it**, none of which changes #1277's verdict. They are filed here so they are tracked work rather than prose in a merged discussion. **The levers named so far -- a retention bound, a rate or sampling bound on read grants, or enrolling audit in the ADR 0055 group committer -- are NOT chosen here.** One of them turns on whether deleting an audit row breaks the tamper-evident chain, and cost 1 below records that two files on `main` give different answers to that question. diff --git a/docs/METHOD.md b/docs/METHOD.md index b0ec115ba..e2e39cae8 100644 --- a/docs/METHOD.md +++ b/docs/METHOD.md @@ -226,9 +226,10 @@ Settle it this way instead. 4. If no run at all is newer than the label event, the state is unknown and the Console keeps polling. -Never inherit the last verdict when the state is unknown. The gate's own version of this staleness is -BACKLOG #1417. That item is open in PR 731 and not yet on `main`, so it does not resolve on -`origin/main` today. +Never inherit the last verdict when the state is unknown. The gate's own version of this staleness +was BACKLOG #1417, and it is fixed: `review-gate.yml` reads the label live inside the running job +rather than from the snapshotted event payload. The steps above still bind, because they answer a +different question -- whether a verdict exists for this head yet, not whether the verdict is honest. Two more measured facts about PR state, so you do not re-derive them: diff --git a/tests/negative_controls.toml b/tests/negative_controls.toml index b53c04592..ac9b48cd0 100644 --- a/tests/negative_controls.toml +++ b/tests/negative_controls.toml @@ -409,17 +409,25 @@ 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 a `synchronize` whose label is +still present, because a new commit is unread whatever the label says. 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, and the `gh` calls it now makes are answered by a shim +rather than mocked away, so an unanswered call is a loud harness fault instead of a silent refusal. +Three plants sit on the SNAPSHOT defect (BACKLOG #1417): an event payload that still says `reviewed` +while the live read says nothing, a label applied twelve hours before the head it sits on, and a +label whose application nothing records. 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_present", + "tests/test_merge_gate_controls.py::test_the_review_gate_ignores_the_label_set_frozen_in_the_event_payload", + "tests/test_merge_gate_controls.py::test_the_review_gate_reads_the_label_live_rather_than_from_the_event_payload", + "tests/test_merge_gate_controls.py::test_the_review_gate_refuses_a_label_applied_before_the_head_it_sits_on", + "tests/test_merge_gate_controls.py::test_the_review_gate_refuses_when_nothing_records_the_label_being_applied", "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", @@ -433,10 +441,19 @@ merge_group entry carries no pull request and therefore no labels, so the label- 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. +list that only ADDS actions must stay clean. The BACKLOG #1417 arms carry the same obligation and it +is sharper there, because each of them is a new way to refuse: a head-date comparison written the +wrong way round would refuse every correctly-reviewed pull request, and a history read that could not +see a label a reviewer applied moments ago would refuse the very event that exists to clear the +check. Reading live also cuts the OTHER way -- a run queued before the label existed now passes on the +live reading rather than refusing on a snapshot that was already wrong -- and that direction is +pinned too, because it is the one place this fix makes the gate pass more often. """ 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_passes_a_label_applied_after_the_head_it_sits_on", + "tests/test_merge_gate_controls.py::test_the_review_gate_accepts_the_label_event_that_started_the_run", + "tests/test_merge_gate_controls.py::test_the_review_gate_clears_a_pull_request_labelled_after_the_run_was_queued", "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", @@ -452,6 +469,15 @@ run: (`[ "$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. +THE SNAPSHOT DEFECT WAS MEASURED BEFORE AND AFTER, 2026-09-03 (BACKLOG #1417), by running the PRE-fix +shell -- lifted from `git show HEAD:.github/workflows/review-gate.yml`, not retyped -- through the +post-fix control. One arm, one input: the event payload carries `reviewed` and the live read returns +nothing, which is PR 724's 13:24-vs-13:43 state. + + * BEFORE: exit 0, printing `reviewed label present. Gate satisfied.` -- the gate passing a pull + request that carried no `reviewed` label at execution time. + * AFTER: exit 1, printing `labels, read at execution time: (none)` and the remedy. + 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 diff --git a/tests/test_merge_gate_controls.py b/tests/test_merge_gate_controls.py index bef3435ad..ff38a1ec2 100644 --- a/tests/test_merge_gate_controls.py +++ b/tests/test_merge_gate_controls.py @@ -1033,24 +1033,78 @@ def test_the_absence_detector_fires_on_a_trigger_set_that_can_go_quiet() -> None _LABEL_STEP = "Require the reviewed label" -def _review_gate_script() -> str: - """The label-reading step's shell, with the shape this control depends on asserted first.""" +def _label_step() -> dict[str, Any]: + """The one step that decides the verdict, located by name rather than by position.""" steps = jobs_of(_REVIEW_GATE)[_REVIEW_GATE_JOB]["steps"] matches = [s for s in steps if _LABEL_STEP in str(s.get("name", ""))] assert len(matches) == 1, ( f"expected exactly ONE step named like {_LABEL_STEP!r} in {_REVIEW_GATE}, found " 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}" + return dict(matches[0]) + + +def _review_gate_script() -> str: + """The label-reading step's shell, with the shape this control depends on asserted first.""" + script = str(_label_step()["run"]) + assert "gh pr view" in script and "$ACTION" in script, ( + "the label-reading step no longer reads the label with `gh pr view` or no longer reads " + "$ACTION, so this control would be feeding input to a script that ignores it and every " + f"verdict below would be about nothing: {script!r}" ) return script +#: A `gh` shim, defined AHEAD of the shipped shell so the live reads can be answered without a +#: network, a token, or a pull request. +#: +#: THIS IS WHAT MAKES THE FIX TESTABLE AT ALL. Before BACKLOG #1417 the gate's inputs arrived as +#: environment variables, so a control could set them; now the gate FETCHES them, so a control has to +#: answer the fetch. The three answers come from `STUB_LABELS`, `STUB_EVENTS` and `STUB_HEAD_AT`. +#: +#: AN UNRECOGNISED CALL EXITS NONZERO AND SAYS SO, rather than printing nothing. A shim that answered +#: everything with silence would let a gate pass on a call it never really made -- and empty output +#: reads, at every point below, as "no label" or "no event", which is a REFUSAL. The failure would +#: therefore look like the gate working. +_GH_SHIM = """gh() { + case "$1" in + pr) printf '%s\\n' "$STUB_LABELS"; return 0 ;; + api) + for arg in "$@"; do + case "$arg" in + */events*) printf '%s' "$STUB_EVENTS"; return 0 ;; + */commits/*) printf '%s\\n' "$STUB_HEAD_AT"; return 0 ;; + esac + done + ;; + esac + echo "gh shim: unhandled call: $*" >&2 + return 90 +} +""" + +#: The shim's own exit code, kept clear of the gate's `exit 1` so a broken control cannot be read as +#: a refusal. Same reason 126/127 are separated out below. +_SHIM_FAULT = 90 + +#: Defaults that satisfy the head-date comparison, so the label-token cases below stay about the +#: token. A `reviewed` label applied an hour AFTER the head it sits on is the ordinary shape. +_HEAD_AT = "2026-09-01T11:00:00Z" +_LABEL_AT = "2026-09-01T12:00:00Z" + + def _run_review_gate( - bash: str, script: str, workdir: Path, env: dict[str, str], *, labels: str, action: str + bash: str, + script: str, + workdir: Path, + env: dict[str, str], + *, + labels: str, + action: str, + label_name: str = "", + payload_labels: str = "", + events: str | None = None, + head_at: str = _HEAD_AT, ) -> tuple[int, str]: """Run the step's shell UNDER THE FLAGS ACTIONS USES, and validate the invocation. @@ -1058,20 +1112,44 @@ def _run_review_gate( flags is not decoration: `-e` changes which line can end the script, and a control run under a friendlier shell would be measuring a step CI never executes. - 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. + `labels` is what the LIVE read returns; `payload_labels` is what the frozen event payload would + have said. They are separate arguments because the whole of BACKLOG #1417 is that they can + differ, and the gate must follow the first. + + 126/127 are a HARNESS fault, never a gate verdict, and `_SHIM_FAULT` is the same class one level + in. 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. """ - (workdir / "review_gate.sh").write_text(script, encoding="utf-8", newline="\n") + (workdir / "review_gate.sh").write_text(_GH_SHIM + script, encoding="utf-8", newline="\n") proc = _run( [bash, "--noprofile", "--norc", "-e", "-o", "pipefail", "review_gate.sh"], workdir, - {**env, "LABELS": labels, "ACTION": action}, + { + **env, + # The gate's own inputs. `LABELS` is set DELIBERATELY: it is the variable the pre-#1417 + # step read out of the frozen payload, and leaving it populated is how the control below + # can tell a live read from a snapshot. + "LABELS": payload_labels, + "ACTION": action, + "LABEL_NAME": label_name, + "NUMBER": "724", + "GH_REPO": "MEFORORG/MessageFoundry", + "HEAD_SHA": "0" * 40, + # The shim's answers. + "STUB_LABELS": labels, + "STUB_EVENTS": _LABEL_AT if events is None else events, + "STUB_HEAD_AT": head_at, + }, ) out = _text(proc) assert proc.returncode not in (126, 127), ( f"{explain_returncode(proc.returncode, 'the review-gate step')} Output: {out.strip()[:300]}" ) + assert proc.returncode != _SHIM_FAULT, ( + "the review-gate step made a `gh` call this control's shim does not answer, so the verdict " + f"below would be about a harness fault rather than the gate. Output: {out.strip()[:300]}" + ) return proc.returncode, out @@ -1124,19 +1202,30 @@ 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_present( 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: a new commit, with the label still readable both ways. + + The label set is planted as `reviewed` in the live read AND in the frozen payload, and the label + event is planted newer than the head, so every other arm of the gate would pass this. On + `synchronize` it must refuse anyway: new commits are unread by definition, and the removal step + that ran one step earlier is not guaranteed to be visible to the labels endpoint yet. - 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". + This arm survived BACKLOG #1417 rather than being replaced by it. The live read is the general + fix; this stays as the belt, because the head-date comparison rests on a commit's committer date, + which a client supplies -- a commit authored before the label and pushed after it would clear + that comparison while being genuinely unread. """ bash, script, workdir, env = review_gate code, out = _run_review_gate( - bash, script, workdir, env, labels="reviewed", action="synchronize" + bash, + script, + workdir, + env, + labels="reviewed", + payload_labels="reviewed", + action="synchronize", ) assert code != 0, ( "the gate accepted a `synchronize` on the strength of a label the step before it had already " @@ -1163,6 +1252,237 @@ def test_the_review_gate_passes_a_pull_request_a_reviewer_has_marked_read( ) +# --------------------------------------------------------------------------------------------------- +# THE SNAPSHOTTED PAYLOAD. BACKLOG #1417, measured on PR 724 on 2026-09-01. +# --------------------------------------------------------------------------------------------------- +# +# The gate read `join(github.event.pull_request.labels.*.name, ',')`, which is the label set as it +# stood when the WEBHOOK FIRED. Branch protection picks the newest check-run by EXECUTION time. Two +# clocks: a run created 13:24 executed at 13:43:46, twenty-odd seconds after the label was removed, +# and reported SUCCESS from its 13:24 payload. That success stood for ten minutes on a pull request +# carrying no `reviewed` label, and only `strict = true` -- an unrelated control -- kept it from +# merging. +# +# THE SIGN CONVENTION IS LOAD-BEARING: only a stale SUCCESS is dangerous. A stale FAILURE blocks, which +# is the safe direction. Two sessions misread a stale failure as a valid label going unhonoured before +# checking whether the label was there at all. + + +def test_the_review_gate_ignores_the_label_set_frozen_in_the_event_payload( + review_gate: tuple[str, str, Path, dict[str, str]], +) -> None: + """PLANTED: the two clocks disagreeing, which is the whole defect. + + `LABELS` is populated with `reviewed` -- exactly what the pre-#1417 step read out of the frozen + payload -- while the LIVE read returns nothing, because the label was withdrawn between the event + firing and this run executing. The gate must follow the live read and refuse. + + THIS IS THE ARM THAT FAILED BEFORE THE FIX. Run against the pre-#1417 shell the planted `LABELS` + is the only input the step consults, so it exits 0 and this assertion reddens. + """ + bash, script, workdir, env = review_gate + code, out = _run_review_gate( + bash, + script, + workdir, + env, + labels="", + payload_labels="reviewed", + action="labeled", + label_name="ci-red", + ) + assert code != 0, ( + "the gate reported success from the label set frozen in its event payload, on a pull request " + f"carrying no `reviewed` label at execution time. BACKLOG #1417.\n{_ascii(out)}" + ) + + +#: Every place an Actions EXPRESSION can enter a step: its `env:` values, and any `${{ }}` written +#: inline in the shell. Prose is deliberately not one of them. +#: +#: THE OBVIOUS SPELLING IS WRONG HERE, in the same way `_GH_ADD_LABEL` above is. A plain +#: `"github.event.pull_request.labels" in step` fires on the workflow's own comment, which names the +#: expression precisely because it is explaining why the step must not use it -- and the "fix" that +#: invites is deleting the sentence that records BACKLOG #1417. +def _payload_label_reads(step: dict[str, Any]) -> list[str]: + exprs = [str(v) for v in dict(step.get("env", {})).values()] + exprs += re.findall(r"\$\{\{.*?\}\}", str(step.get("run", "")), flags=re.DOTALL) + return [e for e in exprs if "github.event.pull_request.labels" in e] + + +def test_the_review_gate_reads_the_label_live_rather_than_from_the_event_payload() -> None: + """The static half, and the one that cannot be satisfied by luck. + + A behavioural control can be passed by a step that happens to agree with the shim. The property + is narrower than that: no EXPRESSION in this step may carry the LABEL SET out of the event + payload, because the payload is the snapshot. The head sha and the action are different -- they + are facts about the event rather than state read through it -- so they stay allowed by name. + + THE DETECTOR'S OWN ASYMMETRY IS ASSERTED IN THE SAME CALL, because a detector that matches + nothing would pass this test on any workflow at all. + """ + step = _label_step() + assert _payload_label_reads( + {"env": {"LABELS": "${{ join(github.event.pull_request.labels.*.name, ',') }}"}} + ), "the detector cannot see the pre-#1417 mapping it exists to catch" + assert ( + _payload_label_reads( + {"run": "# not github.event.pull_request.labels, which is the snapshot"} + ) + == [] + ), "the detector flagged a comment naming the expression rather than an expression using it" + found = _payload_label_reads(step) + assert not found, ( + "the label-reading step is back on the frozen webhook payload. A queued run then reports the " + "label state of its own creation time while branch protection picks the newest run by " + f"execution time, which is BACKLOG #1417 exactly: {found}" + ) + assert "gh pr view" in str(step.get("run", "")), ( + "the step no longer reads the label with a live API call, so whatever it reads instead is a " + "snapshot. Reading the status context is not a fix either -- it inherits the same staleness " + "through the same snapshot, which two sessions adopted before it was refuted." + ) + + +def test_the_review_gate_refuses_a_label_applied_before_the_head_it_sits_on( + review_gate: tuple[str, str, Path, dict[str, str]], +) -> None: + """PLANTED: a live-present label that never covered these commits. + + A live read answers "is the label there now". It does not answer "did anyone read THIS head". + Label a pull request, push to it, and read the state before the `synchronize` strip lands, and + the label is genuinely present on commits nobody has seen. Measured on PR 723: label event + 03:22:44Z, head commit 15:30:17Z, deciding run created 15:30:44Z -- a comparison against the RUN + reports fresh while the label predates the head by twelve hours. + """ + bash, script, workdir, env = review_gate + code, out = _run_review_gate( + bash, + script, + workdir, + env, + labels="reviewed", + action="reopened", + events="2026-09-01T03:22:44Z", + head_at="2026-09-01T15:30:17Z", + ) + assert code != 0, ( + "the gate accepted a `reviewed` label applied twelve hours before the head it sits on. The " + f"label is real; it just never covered these commits.\n{_ascii(out)}" + ) + + +def test_the_review_gate_refuses_when_nothing_records_the_label_being_applied( + review_gate: tuple[str, str, Path, dict[str, str]], +) -> None: + """PLANTED: the label is present and its history is empty, so it cannot be shown to cover the head. + + The safe reading of "I cannot tell" is a refusal, and the remedy is cheap -- re-applying the label + writes an event and fires a fresh run. Passing here instead would turn every unreadable history + into a green. + """ + bash, script, workdir, env = review_gate + code, out = _run_review_gate( + bash, + script, + workdir, + env, + labels="reviewed", + action="opened", + events="", + ) + assert code != 0, ( + "the gate passed a label whose application it could not date, so it could not know the label " + f"covered this head.\n{_ascii(out)}" + ) + assert "add-label reviewed" in out, ( + f"the gate refused but did not name the remedy, which is to re-apply the label.\n{_ascii(out)}" + ) + + +def test_the_review_gate_passes_a_label_applied_after_the_head_it_sits_on( + review_gate: tuple[str, str, Path, dict[str, str]], +) -> None: + """THE ASYMMETRY for the head-date comparison, and it is not decoration. + + A comparison written the wrong way round, or one that refused whenever it had two timestamps, + would satisfy the planted case above while wedging every correctly-reviewed pull request. Same + inputs as that case with the two timestamps swapped into their ordinary order. + """ + bash, script, workdir, env = review_gate + code, out = _run_review_gate( + bash, + script, + workdir, + env, + labels="reviewed", + action="reopened", + events="2026-09-01T15:31:02Z", + head_at="2026-09-01T15:30:17Z", + ) + assert code == 0, ( + "the gate refused a pull request whose `reviewed` label was applied 45 seconds AFTER the head " + f"it sits on, which is the ordinary shape of a reviewed pull request.\n{_ascii(out)}" + ) + + +def test_the_review_gate_accepts_the_label_event_that_started_the_run( + review_gate: tuple[str, str, Path, dict[str, str]], +) -> None: + """THE ASYMMETRY for the history read, on a GitHub clock rather than a client-supplied one. + + When the run was started by a reviewer applying `reviewed`, the label was applied while this head + already existed, so the head-date comparison is answered by construction and no history read is + needed. That matters beyond tidiness: the label event a reviewer just wrote is the one most + likely not to have reached the events endpoint yet, and refusing there would leave the pull + request red with nothing further to re-run it. + + Planted with an EMPTY history and a head dated in the far future, so the fallback path could only + refuse. A gate that reached it would redden here. + """ + bash, script, workdir, env = review_gate + code, out = _run_review_gate( + bash, + script, + workdir, + env, + labels="reviewed", + action="labeled", + label_name="reviewed", + events="", + head_at="2099-01-01T00:00:00Z", + ) + assert code == 0, ( + "the gate refused the very event a reviewer creates by marking a pull request read, so the " + f"label could never clear the check that the label exists to clear.\n{_ascii(out)}" + ) + + +def test_the_review_gate_clears_a_pull_request_labelled_after_the_run_was_queued( + review_gate: tuple[str, str, Path, dict[str, str]], +) -> None: + """THE OTHER DIRECTION OF THE LIVE READ, recorded because it is a real behaviour change. + + Reading live cuts both ways: where the payload is stale-POSITIVE the gate now refuses, and where + it is stale-NEGATIVE the gate now passes. This is the second case -- a run queued before the + label existed, executing after it was applied. The old step would have refused it on a snapshot + that was already wrong. + + That is the gate passing more often than it used to, and it is the correct verdict rather than a + relaxation: the label is on the pull request at execution time and it post-dates the head. A + stale refusal is safe but it is still wrong, and here it wedges a pull request that nothing else + will re-run. + """ + bash, script, workdir, env = review_gate + code, out = _run_review_gate( + bash, script, workdir, env, labels="reviewed", payload_labels="", action="ready_for_review" + ) + assert code == 0, ( + "the gate refused a pull request that carries the `reviewed` label at execution time, on the " + f"strength of a payload snapshotted before the reviewer applied it.\n{_ascii(out)}" + ) + + def test_the_review_gate_still_reports_under_the_required_context_string() -> None: """The other way this gate dies, and the quieter one: the context stops arriving at all. From edd6dc85736d3d768cec915ea651e5881122fb44 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 3 Sep 2026 18:33:04 -0500 Subject: [PATCH 2/2] fix(ci): close the review gate's empty-HEAD_AT fail-open and give the step the shell CI runs (BACKLOG #1417) Review of this branch found a fail-open in the head-date comparison it added, plus three claims describing controls by mechanisms that were not the ones operating. THE FAIL-OPEN. `$LAST_ADD` was guarded for emptiness and `$HEAD_AT` was not. `gh api --jq` on a path that does not resolve prints an EMPTY LINE and exits 0 -- not `null`, and not non-zero -- so nothing in the shell objects. Nothing sorts before the empty string, so `[[ "$LAST_ADD" < "" ]]` is FALSE and the step printed `Gate satisfied` on a head it had not managed to date. A `null` would have sorted after any `2026-...` timestamp and refused, so the safe-looking failure mode is the one that does not happen. Measured against the shipped shell: exit 0 before the guard, exit 1 after. `shell: bash` ON THE STEP. With no `shell:` key Actions runs `bash -e {0}`, which carries no `pipefail`, and the label-history read is a pipeline whose status is `tail`'s. Declaring bash supplies it. This changes no verdict -- a failed history read already refused, through the `-z` guard -- it changes which mechanism reports one, and it makes the controls faithful: both harnesses run the step under `-e -o pipefail`, which was a friendlier shell than CI actually used. TWO NEW PINS, each measured red before and green after. * An empty `$HEAD_AT` was uncovered. Removing the new guard now reddens exactly 1 of the 15 review-gate controls. * The `-z "$LAST_ADD"` guard was unpinned: deleting it left all 14 controls green, because the head-date branch refused instead and its message names `add-label reviewed` too. That guard changes no verdict on any input, only the diagnosis, so the control now asserts its own wording. `_review_gate_script()` additionally refuses to lift a step that does not declare `shell: bash`. Dropping the declaration turns 10 behavioural controls into loud errors rather than silent passes. CORRECTED IN PLACE, NOT DELETED. "The head-date comparison only ever adds a refusal" -- the `labeled`-event shortcut is a pass path that returns before either API read. "Every failure of the two API reads exits non-zero under `-e`, which blocks" -- false for the pipeline; the gate blocked through the `-z` guard instead. And the header's claim that `unlabeled` covers the removal case: it does not cover the bot's own removal, because GitHub dispatches no run from an event raised with `GITHUB_TOKEN`. Measured on PR 765: exactly two review-gate runs on that head, and none for the bot's 20:45:14Z removal. The `synchronize` arm is what covers it. Co-Authored-By: Claude Opus 5 --- .github/workflows/review-gate.yml | 51 ++++++++++++++++++ docs/BACKLOG.md | 44 +++++++++++++-- tests/negative_controls.toml | 31 +++++++++-- tests/test_merge_gate_controls.py | 90 +++++++++++++++++++++++++++++-- 4 files changed, 206 insertions(+), 10 deletions(-) diff --git a/.github/workflows/review-gate.yml b/.github/workflows/review-gate.yml index ded7b9308..048ee9352 100644 --- a/.github/workflows/review-gate.yml +++ b/.github/workflows/review-gate.yml @@ -73,6 +73,15 @@ on: # GREEN context behind. 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. + # + # `unlabeled` COVERS LESS THAN IT LOOKS LIKE IT COVERS, and this comment used to imply otherwise. + # It catches a HUMAN withdrawing the label. It does NOT catch the strip step below removing it: + # GitHub does not dispatch a run from an event raised by the repository's own `GITHUB_TOKEN`, and + # that step runs `gh` under `github.token`. Measured on PR 765: exactly TWO review-gate runs on + # that head, and none for the bot's 20:45:14Z removal. Nothing is wedged by that, because the + # `synchronize` that caused the strip dispatches its own run and that run ends red -- but the + # coverage comes from `synchronize`, not from `unlabeled`, and a reader who believed otherwise + # would think deleting the `synchronize` arm was safe. types: [opened, reopened, ready_for_review, synchronize, labeled, unlabeled] merge_group: @@ -84,6 +93,10 @@ permissions: # commit's date, `issues: read` for the pull request's label events -- label history lives on the # issue side of a pull request, not the pulls side. A 403 would fail the gate CLOSED, which is the # right direction but would wedge every pull request, so both are named rather than assumed. + # + # THAT "FAILS CLOSED" HOLDS BECAUSE THE STEP DECLARES `shell: bash`, and it did not before. Under + # the default shell one of the two reads swallowed its own 403. The mechanism is recorded once, at + # that declaration, rather than restated here. contents: read issues: read pull-requests: write @@ -142,6 +155,21 @@ jobs: ACTION: ${{ github.event.action }} LABEL_NAME: ${{ github.event.label.name }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} + # `shell: bash` IS LOAD-BEARING HERE, NOT TIDINESS. A `run:` block with no `shell:` key runs + # under `bash -e {0}`; naming bash gets `bash --noprofile --norc -e -o pipefail {0}`. The + # option that goes missing is `pipefail`, and one of this step's two API reads is a PIPELINE + # (`gh api ... | sort | tail -n 1`). A pipeline's status is its LAST command's, so `tail` + # exits 0 over a `gh` that 403'd, `-e` never sees the failure, and the step carries on with an + # empty result. Measured 2026-09-03 under both flag sets: without `pipefail` the script ran + # past a failing history read; with it the script ends at the assignment. + # + # It also settles the bashism. The comparison at the bottom is `[[ ]]`, which the `sh -e {0}` + # fallback -- what a runner without bash gets -- would reject outright. + # + # THE TESTS IN tests/test_merge_gate_controls.py RUN THIS STEP UNDER `-e -o pipefail`, and + # before this line that was a friendlier shell than CI used, so they were measuring a step + # Actions never executed. The declaration is what makes those runs faithful. + shell: bash run: | # A NEW COMMIT IS UNREAD BY DEFINITION, and this arm is kept as a belt over the live read # rather than as the fix for staleness -- it used to be the fix, applied to the one action @@ -173,6 +201,13 @@ jobs: # `reviewed` in the very event that started this run, the label was applied while this head # already existed -- that is a GitHub clock, needs no API call, and cannot lag behind a # write made moments earlier. + # + # SAY WHAT THIS ARM IS: a PASS that returns before either API read. So the head-date + # machinery does NOT "only ever add a refusal", which is how the first draft of this change + # described it. The shortcut is sound -- a `labeled` event necessarily post-dates the head + # named in its own payload -- and it stays. But it is a new way to exit 0, and a control + # described as refusal-only while it carries a pass path is a compensating control resting + # on a false premise. if [ "$ACTION" = "labeled" ] && [ "$LABEL_NAME" = "reviewed" ]; then echo "the reviewed label was applied by the event that started this run. Gate satisfied." exit 0 @@ -192,6 +227,22 @@ jobs: exit 1 fi + # BOTH TIMESTAMPS ARE GUARDED, and the first draft guarded only the one above. An empty + # `$HEAD_AT` FAILS OPEN: every string sorts at or after the empty string, so + # `[[ "$LAST_ADD" < "" ]]` is FALSE and the comparison below falls straight through to + # `Gate satisfied` on a head this step could not date at all. + # + # AND IT IS REACHABLE, which is the part that reads as impossible until you check what `gh` + # actually prints. `gh api --jq` on a path that does not resolve prints an EMPTY LINE and + # exits 0 -- not `null`, and not non-zero -- so neither `-e` nor `pipefail` sees anything + # wrong. Measured 2026-09-03 against this repository: one byte of output, a newline, exit 0. + # A `null` would have sorted AFTER any `2026-...` timestamp and refused, so the safe-looking + # failure mode is the one that never happens. + if [ -z "$HEAD_AT" ]; then + echo "::error::The head commit $HEAD_SHA could not be dated, so the reviewed label cannot be shown to cover it. Re-run this check; if it persists, re-apply the label: gh pr edit --remove-label reviewed && gh pr edit --add-label reviewed" + exit 1 + fi + # Both timestamps are fixed-width ISO-8601 in UTC, so a string compare is a time compare. if [[ "$LAST_ADD" < "$HEAD_AT" ]]; then echo "::error::The reviewed label was applied at $LAST_ADD, before this head was dated $HEAD_AT, so it never covered these commits. Re-review, then: gh pr edit --remove-label reviewed && gh pr edit --add-label reviewed" diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 3941437e0..692afbb73 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -19891,9 +19891,47 @@ commit's **committer date**, which a client supplies: a commit authored before t after it clears that comparison while being genuinely unread. What the fix removed is the *other* half -- every action except `synchronize` reading the snapshot. -**A SPOOFED OR ODD COMMIT DATE CANNOT OPEN THE GATE.** The head-date comparison only ever adds a -refusal, so defeating it leaves the live label read standing; it cannot turn a refusal into a pass. -Every failure of the two API reads exits non-zero under `-e`, which blocks. +**A SPOOFED OR ODD COMMIT DATE CANNOT OPEN THE GATE.** Defeating the head-date *comparison* leaves +the live label read standing, so it cannot turn a refusal into a pass. + +**A FAIL-OPEN AND TWO FALSE CLAIMS, FOUND BY REVIEW BEFORE THIS MERGED.** They are corrected here +rather than deleted, because each described a control by a mechanism that was not the one operating, +which is the defect CLAUDE.md section 11 forbids. + +* **THE FAIL-OPEN. `$LAST_ADD` was guarded for emptiness and `$HEAD_AT` was not.** `gh api --jq` on + a path that does not resolve prints an **empty line and exits 0** -- not `null`, and not non-zero + -- so neither `-e` nor `pipefail` sees anything wrong. Measured 2026-09-03 against this + repository: one byte of output, a newline, exit 0. Nothing sorts before the empty string, so + `[[ "$LAST_ADD" < "" ]]` is **false** and the step printed `Gate satisfied` on a head it had not + managed to date. A `null` would have sorted after any `2026-...` timestamp and refused, so the + safe-looking failure mode is the one that does not happen. `$HEAD_AT` now carries the same `-z` + guard, measured exit 0 before it and exit 1 after, and it is pinned by + `test_the_review_gate_refuses_a_head_commit_it_cannot_date`. +* ***"The head-date comparison only ever adds a refusal"* -- it does not.** The `labeled`-event + shortcut is a **pass** that returns before either API read. It is sound, because a `labeled` event + necessarily post-dates the head named in its own payload, and it stays; but it is a new way to + exit 0 and must be described as one. +* ***"Every failure of the two API reads exits non-zero under `-e`, which blocks"* -- false under + the shell the step actually ran.** A `run:` block with no `shell:` key gets `bash -e {0}`, which + has no `pipefail`. The label-history read is a **pipeline**, and a pipeline's status is its last + command's, so `tail` exits 0 over a `gh` that failed and `-e` never sees it. The gate still + blocked, but through the `-z "$LAST_ADD"` guard rather than the mechanism claimed. The step now + declares `shell: bash`, which supplies `pipefail` and makes the sentence true. + +**THE `-z "$LAST_ADD"` GUARD WAS UNPINNED, and that is a separate finding.** Deleting it left all +fourteen review-gate controls green: with `LAST_ADD` empty the head-date comparison refused instead, +and its message names `add-label reviewed` too, so both assertions matched the wrong branch. The +guard is not verdict-load-bearing -- no input makes the step pass without it -- it is +**diagnosis**-load-bearing, so the control now asserts the guard's own wording. Without it a reader +gets `applied at , before this head was dated ...`, which reads as a clock problem and points at the +wrong remedy. + +**THE TESTS WERE MEASURING A SHELL CI DOES NOT USE.** Both harnesses run the step under +`bash --noprofile --norc -e -o pipefail`, and their comments said a friendlier shell "would be +measuring a step CI never executes" -- while the real shell was the friendlier one. `shell: bash` is +what makes those runs faithful, so `_review_gate_script()` now refuses to lift a step that does not +declare it: dropping the declaration turns ten behavioural controls into loud errors rather than +silent passes. **IT DOES PASS MORE OFTEN IN ONE DIRECTION, said plainly.** Reading live cuts both ways. Where the payload was stale-**positive** the gate now refuses, which is the defect this item filed. Where it diff --git a/tests/negative_controls.toml b/tests/negative_controls.toml index ac9b48cd0..a5c2b4d93 100644 --- a/tests/negative_controls.toml +++ b/tests/negative_controls.toml @@ -414,9 +414,13 @@ still present, because a new commit is unread whatever the label says. The gate' 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, and the `gh` calls it now makes are answered by a shim rather than mocked away, so an unanswered call is a loud harness fault instead of a silent refusal. -Three plants sit on the SNAPSHOT defect (BACKLOG #1417): an event payload that still says `reviewed` -while the live read says nothing, a label applied twelve hours before the head it sits on, and a -label whose application nothing records. Four structural plants sit beside them, because the quieter +Four plants sit on the SNAPSHOT defect (BACKLOG #1417): an event payload that still says `reviewed` +while the live read says nothing, a label applied twelve hours before the head it sits on, a label +whose application nothing records, and a head commit the step CANNOT DATE -- an empty `$HEAD_AT`, +which made the head-date comparison false and printed `Gate satisfied` on a head nothing had checked. +The undatable-label plant asserts the guard's OWN wording rather than only the exit code, because the +two refusals are indistinguishable by verdict and deleting that guard once left every control green. +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. @@ -428,6 +432,7 @@ red = [ "tests/test_merge_gate_controls.py::test_the_review_gate_reads_the_label_live_rather_than_from_the_event_payload", "tests/test_merge_gate_controls.py::test_the_review_gate_refuses_a_label_applied_before_the_head_it_sits_on", "tests/test_merge_gate_controls.py::test_the_review_gate_refuses_when_nothing_records_the_label_being_applied", + "tests/test_merge_gate_controls.py::test_the_review_gate_refuses_a_head_commit_it_cannot_date", "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", @@ -478,6 +483,26 @@ nothing, which is PR 724's 13:24-vs-13:43 state. request that carried no `reviewed` label at execution time. * AFTER: exit 1, printing `labels, read at execution time: (none)` and the remedy. +A FAIL-OPEN IN THE #1417 CHANGE ITSELF WAS FOUND BY REVIEW AND MEASURED BOTH WAYS, 2026-09-03. The +step guarded `$LAST_ADD` for emptiness and left `$HEAD_AT` unguarded, and `gh api --jq` on a path +that does not resolve prints an EMPTY LINE and exits 0 -- not `null`, not non-zero -- so nothing in +the shell objects. Nothing sorts before the empty string, so the comparison went FALSE and the step +passed a head it could not date. Three mutations run against the shipped shell, each reverted from +the same run: + + * the `-z "$HEAD_AT"` guard removed -- exit 0 printing `Gate satisfied`, and exactly 1 of the 15 + selected controls red (test_the_review_gate_refuses_a_head_commit_it_cannot_date), 14 green. + * the `-z "$LAST_ADD"` guard removed -- 1 red, 14 green, and the red is the undatable-label control + reporting that the HEAD-DATE branch answered instead. Before that control asserted the guard's + own wording this same edit reddened NOTHING: both refusals exit 1 and both name `add-label + reviewed`, so the assertions matched whichever branch ran. That guard changes no verdict on any + input; it changes the DIAGNOSIS, and pinning the diagnosis is the only thing that can redden. + * `shell: bash` removed from the step -- 10 ERRORS, not 10 passes. The harness lifts the step's + shell and runs it under `-e -o pipefail`, which Actions supplies only when the step declares + bash; with no `shell:` key CI runs `bash -e {0}` with no pipefail, and one of the two API reads + is a pipeline that swallows its own failure there. A control measuring the friendlier shell would + be green about a step CI never executes, so `_review_gate_script()` refuses to lift it at all. + 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 diff --git a/tests/test_merge_gate_controls.py b/tests/test_merge_gate_controls.py index ff38a1ec2..f401223f6 100644 --- a/tests/test_merge_gate_controls.py +++ b/tests/test_merge_gate_controls.py @@ -1046,12 +1046,26 @@ def _label_step() -> dict[str, Any]: def _review_gate_script() -> str: """The label-reading step's shell, with the shape this control depends on asserted first.""" - script = str(_label_step()["run"]) + step = _label_step() + script = str(step["run"]) assert "gh pr view" in script and "$ACTION" in script, ( "the label-reading step no longer reads the label with `gh pr view` or no longer reads " "$ACTION, so this control would be feeding input to a script that ignores it and every " f"verdict below would be about nothing: {script!r}" ) + # THE HARNESS MUST RUN THE SHELL CI RUNS, and this is the line that makes that true rather than + # assumed. `_run_review_gate` runs the script under `-e -o pipefail`. Actions gives a `run:` + # block those flags ONLY when the step declares `shell: bash`; with no `shell:` key it gets + # `bash -e {0}`, which has no `pipefail`. That difference is not cosmetic here -- the step's + # label-history read is a pipeline, and without `pipefail` a failed `gh` inside it exits 0 and + # the script carries on. So a step that drops the declaration silently turns every behavioural + # control below into a measurement of a shell CI never executes, and this assertion is what + # stops that from being invisible (the BACKLOG #1216 shape: green about nothing). + assert str(step.get("shell", "")) == "bash", ( + "the label-reading step no longer declares `shell: bash`, so Actions runs it under " + f"`bash -e {{0}}` with NO pipefail while this control runs it under `-e -o pipefail`. Every " + f"verdict below would then be about a friendlier shell than CI uses. shell={step.get('shell')!r}" + ) return script @@ -1092,6 +1106,13 @@ def _review_gate_script() -> str: _HEAD_AT = "2026-09-01T11:00:00Z" _LABEL_AT = "2026-09-01T12:00:00Z" +#: The `-z "$LAST_ADD"` guard's own diagnosis, quoted so a control can tell WHICH branch refused. +#: +#: Needed because the two refusals are not distinguishable by exit code. Both exit 1 and both name +#: `add-label reviewed`, so a control asserting only those two things passes whichever branch runs -- +#: which is how deleting the guard once left every review-gate test green. +_NO_HISTORY_REASON = "no labeled event records when it was applied" + def _run_review_gate( bash: str, @@ -1108,9 +1129,16 @@ def _run_review_gate( ) -> tuple[int, 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 - flags is not decoration: `-e` changes which line can end the script, and a control run under a - friendlier shell would be measuring a step CI never executes. + GitHub runs a `run:` block as `bash --noprofile --norc -e -o pipefail {0}` WHEN THE STEP DECLARES + `shell: bash`, and the label step does. That qualifier is not pedantry, and this docstring shipped + without it: with no `shell:` key Actions runs `bash -e {0}`, which has NO `pipefail`. So the + sentence "a friendlier shell would be measuring a step CI never executes" was true of the wrong + shell -- these flags were the friendlier ones, and the step CI ran was the stricter question. The + sibling harness in tests/test_release_pipeline.py has always said this correctly, because the step + IT lifts genuinely declares no shell and it uses `bash -e` to match. + + `_review_gate_script()` is what keeps the two in step now: it refuses to lift a label step that + does not declare `shell: bash`, so the flags below cannot silently drift away from CI's. `labels` is what the LIVE read returns; `payload_labels` is what the frozen event payload would have said. They are separate arguments because the whole of BACKLOG #1417 is that they can @@ -1398,6 +1426,60 @@ def test_the_review_gate_refuses_when_nothing_records_the_label_being_applied( assert "add-label reviewed" in out, ( f"the gate refused but did not name the remedy, which is to re-apply the label.\n{_ascii(out)}" ) + # PINNED ON THE REASON, BECAUSE THE VERDICT CANNOT PIN IT, and the test above shipped without + # this line and was therefore green about nothing. Deleting the step's `-z "$LAST_ADD"` guard + # left all fourteen review-gate controls passing: with `LAST_ADD` empty, `[[ "" < "$HEAD_AT" ]]` + # is TRUE for every non-empty head date -- nothing sorts before the empty string -- so the + # head-date comparison refused instead, and its message names `add-label reviewed` too. Both + # assertions above matched the wrong branch. + # + # So the guard is not verdict-load-bearing; it is DIAGNOSIS-load-bearing, and that is what has to + # be asserted. Without it the reader gets `applied at , before this head was dated ...`, which + # reads as a clock problem rather than as missing history and points at the wrong remedy. + assert _NO_HISTORY_REASON in out, ( + 'the step refused, but not through its `-z "$LAST_ADD"` guard -- the head-date comparison ' + "caught this input instead and reported it as a stale label rather than as an undatable one. " + f"Expected the guard's own diagnosis, {_NO_HISTORY_REASON!r}.\n{_ascii(out)}" + ) + + +def test_the_review_gate_refuses_a_head_commit_it_cannot_date( + review_gate: tuple[str, str, Path, dict[str, str]], +) -> None: + """PLANTED: an empty `$HEAD_AT`, which is the fail-open this step shipped with. + + THE COMPARISON IS THE WHOLE PROBLEM. `[[ "$LAST_ADD" < "$HEAD_AT" ]]` with an empty right-hand + side is FALSE for every real timestamp, because nothing sorts before the empty string. The step + therefore fell through to `Gate satisfied` on a head it had not managed to date at all, and it + did so with a real `reviewed` label present, so nothing else in the step objected. + + AND IT IS REACHABLE, which is the half that reads as impossible. `gh api --jq` on a path that + does not resolve prints an EMPTY LINE and exits 0 -- not `null`, and not non-zero -- so neither + `-e` nor `pipefail` catches it. Measured 2026-09-03 against this repository: one byte of output, + a newline, exit 0. Had it printed `null` the gate would have failed CLOSED, because `null` sorts + after any `2026-...` timestamp. The safe-looking failure mode is the one that does not happen. + + Measured against the shipped shell before and after the guard: exit 0, then exit 1. + """ + bash, script, workdir, env = review_gate + code, out = _run_review_gate( + bash, + script, + workdir, + env, + labels="reviewed", + action="reopened", + events=_LABEL_AT, + head_at="", + ) + assert code != 0, ( + "the gate PASSED a pull request whose head it could not date. An empty `$HEAD_AT` makes the " + "head-date comparison false, so the step reports `Gate satisfied` having established nothing " + f"about whether the label covers these commits.\n{_ascii(out)}" + ) + assert "add-label reviewed" in out, ( + f"the gate refused but did not name the remedy.\n{_ascii(out)}" + ) def test_the_review_gate_passes_a_label_applied_after_the_head_it_sits_on(