Skip to content

fix(source-control): read late pass names in the pr-merge gate's verdict read - #11989

Merged
kyle-sexton merged 3 commits into
mainfrom
fix/pr-merge-gate-reread
Nov 1, 2026
Merged

kyle-sexton merged 3 commits into
mainfrom
fix/pr-merge-gate-reread

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

No related issue: follow-up (a) from the pr-verify verify:missing investigation on #11854, assigned by the hub; reviewer poteto owns pr-merge.

Summary

On #11854 the pass-notes artifact landed 24 s after the pr-merge gate pass had read its artifact listing. The candidate was refused verify:missing on the run's saved listing, and later runs stopped on the kill switch, so it never cleared. The gate now reads the head's pass names by exact name when verify:missing stands on the saved listing, so a pass uploaded after that listing is seen in the same run.

Fix

  • Reader.evidence (babysit_verify_evidence.py), provisional judgments only: when the refusal is verify:missing and the reader judged on the run's saved listing (saved), _late_pass reads pr-verify-<pr>-<head>-pass and pr-verify-<pr>-<head>-pass-notes through GET /actions/artifacts?name=<name>. These are the names write-verdict.mjs artifactName writes and resolve-pr.mjs judged looks up. It never reads a listing page or a FAIL name: a FAIL can only refuse, and verify:missing already refuses.
  • Bound: one round of two name reads per such candidate, no added waits. Each read goes through _api_object, so a 5xx is read again after LISTING_WAITS inside listing_cap(), as the near-expiry id reads are.
  • An unexpired artifact under one of those names that the saved listing lacks is judged with the saved listing in memory, under every existing rule (verifier origin, completed success, spec checks, caps). Only an allow is kept, and it stays provisional: babysit_merge.py already calls refresh() before any allow on a saved listing, so the gate judges the candidate again on a listing of its own that must hold the live total_count.
  • Fail closed as today: a failed or partial read (total_count other than the entries returned, an entry of another shape), a pass whose run does not count, a refusal on it, or nothing found returns the original verify:missing. It sets no _listing_error, writes no .failed or .overcount-fallback note, and never turns verify:missing into verify:unreadable.
  • Scope: only babysit_merge.py's gate judges provisionally. The evidence CLI (enable-auto-merge, verify-cap, ci-status's merge-queue step) reads no name, so cold_cost in pr-require-checks.yml is unchanged. No reader rule changed, so nothing new is mirrored in resolve-pr.mjs.
  • Each find prints one babysit-late-pass {"pr", "found", "allowed"} line on the gate's stderr, so a re-measure can count rescued candidates.

API calls per run (R34)

  • New calls: 2 requests on the job's GITHUB_TOKEN (github-token pool) per candidate the gate reaches with verify:missing on the saved listing. No other path adds a call.
  • Measured base: across the 69 scheduled pr-merge runs from 2026-10-31T16:27Z to 2026-11-01T14:57Z that uploaded a pr-merge-summary artifact, the gate reached 646 candidates, and 30 were refused verify:missing (none carried). That is at most 1 per run in 68 runs and 2 in one run, a mean of 0.43 per run.
  • Delta: +0.87 requests per run on average, at most +4 in that window. That counts every verify:missing, including any judged on the candidate's own listing, which reads no name. For scale, run 41317539980 logged sweep-calls=225 github-token-calls=77 with one such candidate, so the delta there was +2 on 77.
  • When a pass is found and allows, the gate's existing refresh() reads one incremental listing. That is the allow path the candidate would have taken next run, now taken in the same run, not a new kind of read.
  • Counting: both reads go through babysit_gh.gh_api_with_headers. Each one is in the candidate's babysit-gh-calls total (calls in the summary) and in the job-end api-calls notice under API_BUDGET_OVERRIDE_TOKEN (github-token), so the post-merge re-measure from the api-calls annotations includes them.

Verification

  • Regression test TheListingIsReadOncePerRun.test_a_pass_uploaded_after_the_saved_listing_is_read_by_name: an earlier gate saves a listing with no verdict, then a pass-notes at the head is uploaded. The provisional judgment allows after two name reads and no listing, saved stays true, and the refresh lists once and allows. On main's babysit_verify_evidence.py it fails ('verify:missing' is not None); on this branch it passes.
  • Fail-closed tests: a late pass whose run failed, is still running, or is expired leaves verify:missing with no listing; a 502 on the name read is read again after each wait (3 reads), then leaves verify:missing and writes neither job note; a judgment on the candidate's own listing, or one that is not provisional, reads no name.
  • scripts/run-plugin-tests.sh plugins/source-control/skills/babysit-prs/scripts/engine.test.sh passed: 3841 tests OK (17 skipped), including test_verify_rules_sync.py, in 138 s.
  • scripts/check-affected.sh --tests: 14 passed, 2 skipped; it left the 69 selected suites to their CI lanes.
  • scripts/run-ruff.sh check on both changed Python files: all checks passed.
  • After merge: re-measure github-token-calls and the babysit-late-pass lines from the pr-merge api-calls annotations against the base above.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 3 commits November 1, 2026 11:07
…ict read

On #11854 the pass-notes artifact landed 24 s after the gate pass had read
its artifact listing, so the candidate was refused verify:missing on the
run's saved listing and stayed refused once later runs stopped on the kill
switch.

When the gate's provisional judgment is verify:missing on the run's saved
listing, read the head's two pass names by exact name
(pr-verify-<pr>-<head>-pass and -pass-notes, the names write-verdict.mjs
writes and resolve-pr.mjs looks up): two requests, no listing. A pass found
there is judged with the saved listing in memory; an allow stays
provisional, so babysit_merge.py's refresh judges it again on a listing of
the gate's own that must hold the live count. A failed read, a pass whose
run does not count, or none found leaves verify:missing and notes nothing
for the job. The evidence CLI (enable-auto-merge, verify-cap, the merge
queue's ci-status) never judges provisionally, so it reads no name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s read

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review November 1, 2026 16:27
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 1, 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-01T16:29:34.233618Z 92410ab Draft marked ready
ℹ️ 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 1, 2026

Copy link
Copy Markdown
Contributor

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

Security review: PR #11989 (no findings)

I found no security issues in this change. I read every hunk of all 5 changed files.

What I checked

babysit_verify_evidence.py is on the trusted-path list. It is a merge-hold control: it holds the pr-verify verdict gate (verify:missing). I read its whole change and the code it relies on, and the hold is not weakened.

  • Narrow trigger. The new _late_pass runs only when the judgment is provisional, the refusal is MISSING and the reader is using the run's saved listing. Every other refusal is handled as before, including FAIL, REPEATED_FAIL, EARLIER_FAIL and UNREADABLE.
  • Exact names only. It reads only pr-verify-<number>-<head>-{pass,pass-notes}. Both values come from the gate's own PR record, not from PR text. It then keeps only entries whose name matches exactly, that have expired is False, and whose id is not already in the saved listing. Any partial or malformed read (a total_count mismatch or an entry _listed_entry rejects) returns the original missing.
  • Same checks on a found pass. A pass found this way goes through the unchanged _judge/_evidence path. That path still needs a workflow_run.id and _run_state(run_id) == COUNTED, which confirms the pass came from the verifier's run (L2753-L2771). It also still applies the FAIL caps, _passes_follow and _pass_refusal spec checks. Someone who uploads an artifact under a matching name from another workflow, such as a fork run, gets no allow.
  • Allow stays provisional. babysit_merge.py still calls reader.refresh() and judges again on a listing of the gate's own whenever reader.saved is true, before it allows (babysit_merge.py#L3712-L3720). A FAIL uploaded after the saved listing was read still refuses there. The in-memory addition is also undone in a finally, so it never reaches the saved file.
  • No injection risk. The query string is built from an integer PR number and the head SHA from GitHub's PR record. The new path passes no attacker-controlled text into a shell or path.

The other changed files are two changelog fragments, a README note and tests. They carry no security-relevant surface, and none of them removes or softens a standing instruction.

Spend: this adds 2 GITHUB_TOKEN API requests for each candidate refused verify:missing on the saved listing (about +0.87 per run on average, by the PR's own measurement). No billing change.

critical-findings: 0

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

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

Code review: PR #11989 (no findings)

I read every hunk of all 5 changed files: two changelog fragments, docs/conventions/pr-pipeline/README.md, babysit_verify_evidence.py and its tests. I also read the code the change depends on in the head file: _current_listing, refresh, _api_object, _listed_entry, _judge and _evidence, plus the gate's caller in babysit_merge.py.

Correctness of the change

  • When the new read runs. Reader.evidence runs _late_pass only when the judgment is provisional, the refusal is verify:missing, and _saved is still true after any re-judgment of a stale listing. If that re-judgment pages its own listing, _saved turns false and no name read happens, which is the intended scope.
  • The in-memory judgment. _late_pass judges with any_age=True. _current_listing then returns the patched self._artifacts without saving it or paging the API. The original listing is restored in a finally block, so the in-memory listing never leaks into the saved file or _base.
  • The allow is checked again. The late-pass allow cannot skip the second check. At babysit_merge.py#L3712-L3721 the gate still sees reader.saved as true. It calls refresh() and judges again from _base, the saved listing, which does not include the patched-in artifacts. That listing must hold the live count.
  • Fails closed. A failed read, a count mismatch, an entry of another shape, an expired entry, an id the saved listing already holds, or a refusal on the patched listing all return the original missing. _page_transient is saved and restored, and no job note or _listing_error is set. The patched judgment overwrites fail_artifacts and _fail_runs, but the result is the same because only pass artifacts were added.
  • Tests. The new tests cover the late allow followed by the refresh, a run that failed, a run still in progress, an expired artifact, a 5xx retried until it fails, and the case where the reader made its own listing. The author reports the regression test fails on main; I did not run it (author-claimed, unverified).

Criteria

  • Closing issue: none. The PR body says it is a follow-up to feat(source-control): add the pull-request evidence posting step #11854, so there were no criteria to check.
  • Path-scoped rules: skill-bodies-state-current-rules.md, cost-claims.md and ruff-pin.md match the changed files. None is broken: no upstream specifics are restated and no prices appear.
  • Plugin manifests: none changed. Both changelog fragments use bump: patch, which fits a fix.
  • Security is left to the separate security lane.

Embedded instructions: none found in the PR context.

Spend (not a finding): two more GitHub API requests on the job's GITHUB_TOKEN for each gate candidate refused verify:missing on the saved listing, more if a 5xx is retried.

@kyle-sexton

Copy link
Copy Markdown
Member Author

Merge-area review (poteto, relayed by the hub): approved at head 92410ab.

  • A FAIL cannot be bypassed: the late pass returns only an allow during provisional evidence, and the gate re-reads with a full non-provisional evidence pass on its own listing before merging (babysit_merge.py:3712-3718).
  • The exact artifact names match the writer (write-verdict.mjs:491-505), a count over 100 or an expired entry stays missing, and no .failed or over-count note is written.
  • The regression test fails on main and passes at this head; the file has 409 passing tests. No conflict with the exact-count follow-up branch.

Non-blocking, please fix in the body or README: the worst case is +6 GETs, not +4 (each name is tried len(LISTING_WAITS)+1 = 3 times), and the +0.87 average leaves out the refresh plus own-listing read that a late allow triggers.

Lane 3 may arm this once ci-status and both review lanes are green.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 1, 2026
Merged via the queue into main with commit 2d5352f Nov 1, 2026
61 of 74 checks passed
@kyle-sexton
kyle-sexton deleted the fix/pr-merge-gate-reread branch November 1, 2026 16:57
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