Skip to content

fix(source-control): keep a queued lanes App entry whose verdict the sweep cannot read - #12472

Merged
kyle-sexton merged 2 commits into
mainfrom
fix/sweep-unreadable-keeps-entry
Nov 2, 2026
Merged

kyle-sexton merged 2 commits into
mainfrom
fix/sweep-unreadable-keeps-entry

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

No related issue: a merge-queue loop the hub found on #12432 and #12435; no new issue is filed while open issues are driven to zero.

Summary

pr-merge's merge job dequeued lanes App PRs that enable-auto-merge had just armed on a pass verdict, so #12432 and #12435 cycled in and out of the merge queue about every 10 minutes:

  • added at 17:51Z, removed at 18:13Z, re-added at 18:25Z, removed at 18:35Z;
  • every re-add built a merge group that was thrown away.

The cause was in the merge job's queued-entry sweep:

  • In schedule run 41496022715, the job's whole artifact listing failed: repository artifacts: 1 whole listing never held GitHub's live artifact count.
  • The sweep then read every queued entry's verdict as verify:unreadable and dequeued them: "dequeued": {"12432": "verify:unreadable", "12435": "verify:unreadable"}.
  • enable-auto-merge in the next run read the same verdicts as passing (verify-cache shadow: ... fresh=allow) and armed them again.

Fix

  • babysit_verify_evidence.py Reader.queued_hold: when the verdict read is verify:unreadable and the pull request is the lanes App's, it now answers verify:queue-reads. The App is matched by id and type == "Bot", the same way ci-status matches it. A lanes App id that cannot be read keeps verify:unreadable.
  • babysit_merge.py unattended_writes: a verify:queue-reads entry stays in the queue and is not recorded as kept, so the next sweep judges it again.
  • Every other hold still dequeues: verify FAIL classes, an unreadable entry by any other author (an adopted owner or App pull request included), a crash, label and body holds (entry_hold), the run's own refusal, and its risk score.
  • Why this is safe:
    • For a lanes App pull request, the merge queue's ci-status runs its own verdict reader on the merged head. It holds fail-closed on verify:unreadable (pr-require-checks.yml, the "AND NO FAIL PR-MERGE'S READER COUNTS" step; merge-holds.mjs evidenceHold).
    • The same step exits before any read for every other author ("is not the lanes App's; pr-merge's gate or a person reads its holds"), so those entries keep the sweep's dequeue.
    • The independent security review found that gap in this PR's first version, which kept every unreadable entry.
  • The lasting cure for the listing count mismatch is the FAIL index mode. That waits on the legacy-FAIL file, whose build is running.

Per-run call delta: none. The sweep makes the same reads and dequeues fewer entries. Each avoided cycle saves a merge-group CI run and an arm.

Verification

  • New test AnUnreadableQueuedLanesEntryIsLeftToTheQueue (test_babysit_verify_evidence.py): only a lanes App author gets verify:queue-reads. An owner, a non-Bot with the same id, another App, a missing App id and an unreadable App id all keep verify:unreadable. Answering yes for every author fails 5 of its 6 cases.
  • New test AnUnreadableVerdictKeepsAQueuedEntry (test_babysit_merge_unattended.py): FAIL and unreadable entries dequeue, a verify:queue-reads entry stays, and only the passing entry is stored as kept. It fails with the sweep change reverted.
  • Test suites: test_babysit_verify_evidence.py 429 passed, test_babysit_merge_unattended.py 298, test_babysit_merge.py 143, test_babysit_merge_skip.py 22. Pinned ruff check is clean.
  • Left out: a live run. The next schedule run whose whole listing fails will show whether queued entries stay.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 2 commits November 2, 2026 13:57
…ot read

The merge job's queued-entry sweep read verify:unreadable as a hold and
dequeued the entry. When the job's whole artifact listing failed, it
removed lanes App PRs that enable-auto-merge had just armed on a pass
verdict, and they cycled in and out of the queue (#12432, #12435 in run
41496022715). The merge queue's own reader refuses an unreadable
verdict fail-closed, so the sweep now leaves the entry and does not
count it as kept.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The merge queue's ci-status reads the verdict only for lanes App pull
requests and exits before any read for every other author, so keeping
every unreadable entry let an adopted pull request merge unread.
queued_hold now answers verify:queue-reads only for an unreadable verdict
on a lanes App entry (id and Bot type, as ci-status matches it), and the
sweep keeps only that one; every other unreadable entry and a crash still
dequeue.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-02T19:14:53.803076Z cca22b9 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

claude-review reviewed cca22b9e65dd06c8fc99e503599fc56aeb268be3: read 5 of 5 changed file(s); 0 finding(s), 0 as inline comments.

Code review: no findings.

I read every hunk of all 5 changed files: the changelog fragment, babysit_merge.py, babysit_verify_evidence.py and the two test files. I also read the surrounding code in unattended_writes, _queued_verify_hold, Reader.queued_hold and Reader._lane_app_id.

What I checked:

  • Single caller. Reader.queued_hold has one caller, _queued_verify_hold at babysit_merge.py#L4256. No other consumer can mistake the new verify:queue-reads value for a hold.
  • Narrow scope. Only a verify:unreadable verdict becomes QUEUE_READS. A crash in queued_hold is caught by _queued_verify_hold and still returns UNREADABLE, so that entry is still dequeued. A failed pull-request read also still dequeues.
  • Identity check fails closed. _queue_reads matches the author by App id and type == "Bot", the same rule _split_child uses. When the App id is missing, or reading it raises, it answers no, so the entry keeps its hold.
  • No caching. A QUEUE_READS entry returns None before kept_at is set. It is not stored in the sweep's kept cache, so the next sweep judges it again. The pull request's own entry (n == number) never reaches the verifier, so its own_refusal and risk paths are unchanged.
  • Tests. Both new tests cover the rule. The verify-evidence test covers the lanes App, owner, same id but not a Bot, another App, no App id, and an unreadable App id. The unattended-sweep test checks that FAIL and unreadable entries are dequeued, that the QUEUE_READS entry stays, and that only the passing entry is cached as kept. I did not run them: the test counts in the PR description are the author's claim and unverified.

Other checks:

  • Closing issue: none. The PR says it has no related issue, so I had no acceptance criteria to check.
  • Path-scoped rules: skill-bodies-state-current-rules.md and cost-claims.md match these paths by glob. The changed files are scripts and tests with no restated upstream details and no cost claims, so neither rule is broken.
  • Plugin manifests: none changed. The changelog fragment's bump: patch fits a bug fix.
  • Security: left to the separate security lane. The safety argument that ci-status refuses the entry fail-closed rests on pr-require-checks.yml and merge-holds.mjs, which this PR does not change. I did not verify it here.
  • Embedded instructions: none found in the PR context or the prior-findings file.
  • Spend: none. The PR says the sweep makes the same reads and dequeues fewer entries.

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

claude-security-review reviewed cca22b9e65dd06c8fc99e503599fc56aeb268be3: read 5 of 5 changed file(s); 0 finding(s), 0 as inline comments.

Security review: no security issues found

Scope. I read every hunk of all 5 changed files: the changelog fragment, babysit_merge.py, babysit_verify_evidence.py and their two test files. Two of them are trusted paths and merge-hold controls, babysit_merge.py and babysit_verify_evidence.py, so I read their whole change and the gate the change relies on.

What changes. pr-merge's queued-entry sweep normally dequeues a queued entry whose pr-verify verdict it cannot read (verify:unreadable). It now skips that dequeue for one case:

  • Reader.queued_hold returns verify:queue-reads instead of verify:unreadable when the pull request's author is the lanes App.
  • The author must match by both user.id == _lane_app_id() and user.type == "Bot".
  • unattended_writes returns None for that value, so the entry stays in the queue and is not stamped as kept.

This loosens a merge hold, so I judged it as a possible weakening of a merge hold.

Does anything still stop that path? Checked, and yes. The narrowed hold is backed by the merge queue's own check, which fails closed:

  • pr-require-checks.yml, step "Fail a lanes App pull request its body or a member holds" (L6248-L6249):
    • It runs on merge_group, because github.event.pull_request.draft != true is true when there is no pull request payload.
    • For a lanes App author (L6351) in the merge queue, it runs babysit_verify_evidence.py evidence on the live head (L6536).
    • If the reader fails or produces no listing, the step exits 1.
    • Any refusal outside the three that a pass settles reaches merge-holds.mjs evidenceHold (L1421-L1437) and holds. That includes verify:unreadable.
  • Every other author keeps the sweep's dequeue, because ci-status exits early for them (L6341-L6354). The new _queue_reads matches that check by id and type, so it is not wider than what the queue reads.
  • Failure handling stays fail-closed:
    • If the App id cannot be read, or the trusted list names none, the result is False, so the entry stays verify:unreadable and is dequeued.
    • If the pull request itself cannot be read, queued_hold still returns UNREADABLE before the author test.
  • The author's user.id and type come from GitHub's API and cannot be set by whoever opens a pull request. No new actor, token scope, credential or untrusted text reaches a lane.

So a lanes App entry with an unreadable verdict can no longer be dequeued early, but it still cannot merge without a verdict the queue's reader accepts (or the same trusted head approval that lifted the hold before). The only effect is that the queue may build a merge group that ci-status then fails. That costs CI time, not security.

Embedded text: the PR context contains no instructions aimed at the review lane.

Spend: none. The PR says the sweep makes the same reads and dequeues fewer entries, which may save merge-group CI runs.

critical-findings: 0

@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 2, 2026
Merged via the queue into main with commit 1784017 Nov 2, 2026
47 checks passed
@kyle-sexton
kyle-sexton deleted the fix/sweep-unreadable-keeps-entry branch November 2, 2026 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant