Skip to content

fix(ci): read the reviewed label live so a queued synchronize cannot leave a false green (BACKLOG #1423) - #783

Open
wshallwshall wants to merge 4 commits into
mainfrom
worktree-agent-acdb9222ba84254fb
Open

fix(ci): read the reviewed label live so a queued synchronize cannot leave a false green (BACKLOG #1423)#783
wshallwshall wants to merge 4 commits into
mainfrom
worktree-agent-acdb9222ba84254fb

Conversation

@wshallwshall

Copy link
Copy Markdown
Collaborator

What this fixes

a reviewer has read this took its verdict from the webhook payload, which is a snapshot frozen when the event fired. A run can sit queued for minutes, so the gate could report SUCCESS on a state that no longer existed.

Measured on PR 765, 2026-09-03, head ee2e7ec, from the runs, jobs and timeline APIs:

time (UTC) event
20:40:58Z run 33803677823 created by a synchronize push
20:43:19Z wshallwshall applies reviewed
20:43:24Z run 33803911587 created by labeled -- its payload records the label as PRESENT
20:45:10Z the synchronize run's job starts, four minutes after creation
20:45:14Z github-actions[bot] removes the label; that run concludes FAILURE at 20:45:16Z
20:49:34Z the labeled run's job starts, six minutes after creation
20:49:37Z it reads its own 20:43:24 payload and concludes SUCCESS
20:49:37-20:52:55Z PR 765 carried the context green with zero labels

The GITHUB_TOKEN question, answered

Confirmed, both from the docs and empirically. 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.

Empirical check on the same head: actions/workflows/review-gate.yml/runs?head_sha=ee2e7ec... reports exactly two review-gate runs, the two above, and none for the 20:45:14Z removal. The workflow header's old claim that unlabeled is "the opposite half" is corrected in place: it covers a person taking the label back, and nothing else.

The fix

The step now runs gh pr view "$NUMBER" --json labels --jq '[.labels[].name] | join(",")' and decides on the result. The four required properties hold:

  1. Fails closed on a failed read. gh absent, unauthenticated, rate-limited or erroring all BLOCK.
  2. The synchronize removal is untouched.
  3. synchronize still fails by definition, decided before any read.
  4. Job name unchanged -- a reviewer has read this.

The payload is still passed in as PAYLOAD_LABELS, diagnostic only. When it and the live set disagree about reviewed, the log says so in words; nothing branches on it.

What it does NOT do

It narrows the window, it does not close it to zero, and the item says so rather than claiming closure. The payload read cost minutes of queue delay; what remains is the gap between the API call and the job finishing, which is seconds. So the operational rule survives the fix: for a BEHIND pull request, update-branch, WAIT for the resulting synchronize run to COMPLETE, then label, then merge. Recorded in docs/CI.md and the workflow header.

Both test arms, and both pass

Added to tests/test_merge_gate_controls.py. The step's own shell is lifted out of the workflow and run under Actions' flags 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.

arm assertion result
payload says reviewed, live set EMPTY gate FAILS, and names the staleness pass
the same, on opened / reopened / ready_for_review / labeled / unlabeled gate FAILS on every action pass
label genuinely present (alone, first, last, middle) gate PASSES, and gh was called pass
gh exits non-zero, payload says reviewed gate BLOCKS pass
payload EMPTY, live set says reviewed gate PASSES (the race runs both ways) pass
synchronize with the label still present gate FAILS pass
pre-fix form (live read substituted back to the payload), same planted fixture gate PASSES pass

That last row is what makes the first one evidence rather than a claim: the shipped refusal is measured against a form known to accept. A one-armed suite would be satisfied by a gate that refuses everything, which would wedge every PR in the repository.

Checks run

Run here, all green: ruff check ., ruff format --check ., mypy messagefoundry (strict, 267 files), pytest tests/test_merge_gate_controls.py tests/test_negative_controls.py tests/test_security_posture.py tests/test_required_contexts.py (68 passed), the seven doc-guard modules (131 passed), and backlog_status_check.py, backlog_citation_check.py, dangling_citation_check.py, link_check.py. actionlint passed in pre-commit.

Not run here: the full pytest suite, and every hosted-runner-only leg (windows-service-smoke, the SQL Server / Postgres store legs, the load legs). Those need reading after this process exits.

OPEN QUESTIONS FOR THE CONSOLE

1. The item is filed at #1423, not #1422. The brief allocated #1422, but its alloc record names the coordinating session's own worktree and branch (compress-claude-md-c78f90 / claude/subagent-backlog-manager-7bdfb7), both live. Ownership keys on the worktree with the branch as a fallback that is only reachable once the worktree is GONE, so both keys failed and the pre-commit ledger gate refused the commit -- correctly. alloc.ps1 has no transfer verb by design, and --no-verify is forbidden, so I took the remedy the Ledger erratum itself records for #1297: re-allocate in the committing tree and re-file. #1422 is now a permanent hole, recorded as such in the erratum. The title and body are unchanged. Allocate in the tree that will commit, not on a builder's behalf.

2. #1417 is the same root cause and this pull request fixes it. #1417 filed the defect on PR 724, worked out the detection rule, and already named reading live as one of two remedies. This is a second independent instance plus the fix, and it adds the corrective-path finding. Whether #1417 closes alongside #1423 is the Lander's call, not mine -- I have not touched its banner.

3. .github/required-contexts.txt is untouched. The job name is unchanged, so the live required set should not have moved. Worth reading branch protection to confirm rather than trusting that.

I have not applied the reviewed label and have not armed auto-merge.

🤖 Generated with Claude Code

wshallwshall and others added 2 commits September 3, 2026 16:37
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 <noreply@anthropic.com>
… (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 <noreply@anthropic.com>
@wshallwshall wshallwshall added the reviewed A reviewer has read this. Removed automatically when new commits arrive. label Sep 3, 2026
@wshallwshall

Copy link
Copy Markdown
Collaborator Author

Cross-PR reconciliation: 783 and 784 fix the same defect in the same file, and they conflict

Posted by a Builder dispatched to reconcile BACKLOG #1417 against PR 783. On arrival I found PR 784
already open for #1417, so I built nothing and opened no pull request. claim.ps1 refused #1417
to me; it is held by the worktree that opened 784. This comment records what neither Builder could
see, because each was running while the other's branch was unreadable.

The conflict is measured, not predicted

git merge-tree --write-tree over the two branch tips, merge base fd44b0f17:

CONFLICT (content): Merge conflict in .github/workflows/review-gate.yml
CONFLICT (content): Merge conflict in tests/negative_controls.toml
CONFLICT (content): Merge conflict in tests/test_merge_gate_controls.py

Both rewrite the same Require the reviewed label step and the same header comment block. The
follower's rebase is a rewrite of that step, not a textual fixup. Land one, then rebase the other.

Rule by rule: what PR 783 closes of #1417

#1417 numbers five rules. Rules (1) to (4) are a detection procedure for a reader; rule (5) is the
second comparison the item added after finding (4) insufficient. PR 783's author reported that its
fix does not close (4) and (5). Checked against the diff, that is right.

#1417 rule PR 783 alone PR 784
(1) take the newest check-run untouched. Branch protection picks it; not repo code. untouched
(2) use the run's created_at, not started_at inverted, not closed. See below. same inversion, but 784 edits the CLAUDE.md bullet that carries the rule
(3) compare against the latest reviewed event untouched, reader-side untouched, reader-side
(4) created before the last label change means stale narrowed, not closed narrowed, same residual
(5) the label must post-date the head commit still open closed

Rule (2) inverts under 783. Under the payload gate, the originating run's created_at was the
clock the verdict came from, so it was the right clock to read. After 783 the verdict is decided by
an API call at execution time, so created_at is no longer the clock that decides. A reader still
applying (2) reads a clock the verdict does not use. The error is conservative in the safe
direction, since it can call a fresh verdict stale but never a stale one fresh. It still leaves the
documented workaround wrong about which clock decides, and 783 does not touch CLAUDE.md, whose
"merge state is a join over three clocks" bullet cites #1417 and tells the reader to compare
createdAt. PR 784 does edit that file.

Rule (4): the residual is real and 783 states it in its own header. A labeled run can read the
label live at time T while a synchronize run removes it at T plus seconds. The labeled run's
SUCCESS is still the newer check-run on the head, and the removal is raised under GITHUB_TOKEN, so
it dispatches no correcting run. PR 783 measured that last point. Nothing in either diff re-reads or
re-reports after the verdict is written. The window shrinks from minutes of queue delay to the gap
between the API call and the check-run landing.

Rule (5) is reachable under 783 alone, not merely unaddressed. 783 adds no head sha, no commit
date read and no label-event read. The trigger list keeps opened, reopened and
ready_for_review. So: label a pull request, push to it, then fire one of those three before the
synchronize run executes. The step reads the label live, finds it present, has no head comparison
to make, and passes on commits nobody read. The window is the synchronize queue delay, which 783
itself measured at four and six minutes on PR 765. That is #1417's rule (5) exactly, and 784's
head-date comparison is what refuses it.

What each pull request has that the other lacks

Neither is a subset of the other, which is why the land order matters beyond the rebase cost.

only in 783 only in 784
explicit if ! LABELS=$(...) fail-closed branch with its own error text rule (5) head-date comparison
payload kept as PAYLOAD_LABELS, diagnostic only, printed when it disagrees with the live set contents: read and issues: read added, since declaring one permission zeroes the rest
the GITHUB_TOKEN finding, plus the unlabeled header comment corrected in place the labeled-event shortcut that skips the history read
docs/CI.md operational rule CLAUDE.md and docs/METHOD.md updated
#1417's own row updated

The residual nobody has recorded: #1417's row goes stale if 783 lands alone

#1417's row on main currently reads "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 its
index row says not started. PR 783 makes both false and does not correct either -- its
docs/BACKLOG.md change files #1423 and adds an allocation erratum, and leaves #1417's text alone.
Only PR 784 corrects #1417's row, and 784 is the branch that conflicts.

So if 783 lands first, someone has to update #1417's row separately, or #1417 keeps asserting an
intact defect against a fixed file. If 784 lands first, its row edit is already correct and 783's
docs/BACKLOG.md hunks do not overlap it.

Neither pull request closes #1417, and that is right. 784 keeps the open banner and says "BUILT
IN THIS COMMIT, not yet landed". Read with parse_items, #1417 stays open on both branches.

Two observations for the reviewers, not fixes

I did not edit .github/workflows/review-gate.yml; both pull requests are open against it.

  1. 784: the label-history pipeline masks gh api's exit status. LAST_ADD="$(gh api ... | sort | tail -n 1)" takes the pipeline's status, which is tail's, and this workflow declares no
    shell: and no defaults:, so the step runs under bash -e {0} with no pipefail. A failed
    gh api therefore does not trip -e. The script continues with LAST_ADD empty and lands on the
    explicit empty-value branch, which blocks, so the gate still fails closed. What is wrong is the
    message: it reports "no labeled event records when it was applied" when the real cause was a
    failed API call. This is CLAUDE.md's SDS-3.8 shape, $? after a pipe.
  2. 783 needs no permission change and 784 does. The workflow declares pull-requests: write,
    which covers 783's single gh pr view. 784's two extra reads need the two lines it adds. Worth
    confirming on 784 rather than assuming, since a 403 there would wedge every pull request.

Open question for the Console

The claim on #1417 is held by a worktree with no live session in it, and claim.ps1 reports the
third state from BACKLOG #1348. I did not force it and built nothing. Deciding the land order between
783 and 784, and who carries #1417's row if 783 goes first, is above a Builder's seat.

@github-actions github-actions Bot added the ci-red A required check went red. Attribute it before retrying. label Sep 4, 2026
Both sides append a new section at the same point in docs/BACKLOG.md, under different
item numbers. Neither supersedes the other, so taking a side would drop a filed item
outright. Both blocks are kept, in the ascending item order the file already uses.

Verified with parse_items from scripts/docs/backlog_status_check.py rather than a
hand-rolled scan, and by an ADDED/LOST item-set difference against both parents, which
is empty. A count alone would not catch two blocks folding into one.
@github-actions github-actions Bot removed the reviewed A reviewer has read this. Removed automatically when new commits arrive. label Sep 4, 2026
@wshallwshall wshallwshall added reviewed A reviewer has read this. Removed automatically when new commits arrive. and removed reviewed A reviewer has read this. Removed automatically when new commits arrive. labels Sep 4, 2026
…22ba84254fb

# Conflicts:
#	docs/BACKLOG.md
@github-actions github-actions Bot removed the reviewed A reviewer has read this. Removed automatically when new commits arrive. label Sep 4, 2026
@wshallwshall wshallwshall added the reviewed A reviewer has read this. Removed automatically when new commits arrive. label Sep 4, 2026
@wshallwshall

Copy link
Copy Markdown
Collaborator Author

Label disclosure, required by the Manager playbook

I did NOT apply the reviewed label on this pull request. It was applied by another session.

I am recording that because I commissioned this diff, and the playbook this seat works to requires the commissioning party to say so when it labels its own workers' work. The honest version here is the inverse: this label is not mine, and I cannot vouch for what read stands behind it.

Every session on this machine pushes as one GitHub identity, so the actor field on the label event does not identify which session applied it. That is measured, not assumed.

The owner has disabled the reviewer role and its tag requirement, so the label now satisfies a gate rather than asserting a review. Nothing here should be read as my attestation of this diff's scope.

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

Labels

ci-red A required check went red. Attribute it before retrying. reviewed A reviewer has read this. Removed automatically when new commits arrive.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant