Skip to content

perf(pr-merge): judge the gate's candidate on the run's listing at any age - #10558

Merged
kyle-sexton merged 5 commits into
mainfrom
perf/shared-artifact-listing
Oct 28, 2026
Merged

kyle-sexton merged 5 commits into
mainfrom
perf/shared-artifact-listing

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 28, 2026 •

Copy link
Copy Markdown
Member

No related issue: GITHUB_TOKEN spend follow-up to #10488

Summary

The pr-merge gate judges its own candidate's verifier verdict on the run's saved artifact listing at any age, instead of listing again whenever that listing is over two minutes old. A candidate that is refused, by its verdict or by anything after it such as the risk score, now lists nothing. Only a candidate the gate is about to allow, or a queued entry it keeps, still needs a recent listing.

This builds on #10488's carried, incremental listing and adds no listing path. The skip filter, the gate and clear-escalations already share one file per job (BABYSIT_VERIFY_LISTING). The skip filter's listing is the job's first, built on the carry.

Fix

  • One listing seam for pr-merge's verdict readers. pr-merge's verdict readers all read the repository artifact listing through babysit_verify_evidence.Reader.listing() / _current_listing: the skip filter (babysit_merge_skip.py filter), each gate candidate, queued holds, enable-auto-merge and clear-escalations (evidence --shared-listing). The seam builds on the run's saved listing or perf(pr-merge): list only new artifacts on top of a trusted carried listing #10488's carry and pages the whole listing only as a fallback. This PR adds no second helper, and a future strategy goes behind _current_listing.

  • Outside the seam, out of scope. Exact-name lookups of other artifacts remain elsewhere:

    • babysit_postmerge.py _lane_record (lane records);
    • resolve-pr.mjs judged;
    • reconcile.mjs passStarts.
  • INCREMENTAL_READS = 2: the incremental listing is read once more on the same base before the whole listing is paged.

  • babysit_verify_evidence.py: Reader.evidence(..., provisional=True) judges on the listing at hand at any age.

    • An allow stands on that listing, because babysit_merge.py still calls refresh() and judges the candidate again on a listing of its own before allowing it. That rule is unchanged.
    • A STANDING refusal (a FAIL at the head, the FAIL cap, verify:too-old) stands, because no later upload clears it.
    • Any other refusal is judged again on a listing under two minutes old, so a stale verify:missing never dequeues an entry.
  • Calls that are not provisional work as before: queued entries, enable-auto-merge's --shared-listing, and the evidence command.

  • babysit_merge.py: the candidate's first judgment passes provisional=True.

  • The trust rule is unchanged. Every artifact still counts only through Reader._run_state/_origin (verifier origin: .github/workflows/pr-verify.yml, event workflow_run, on the branch, in this repository), and a FAIL still counts under any number and head.

Cut 2 (exact-name verdict lookup before an allow): not built, on data

  • Coverage. The refresh before an allow exists to catch a FAIL uploaded since the listing, wherever _evidence counts one. Exact names find only the FAILs at this head (resolve-pr.mjs judged). They miss:

    • a FAIL at another head of the pull request (verify:earlier-fail);
    • an -i<issue> FAIL from another pull request (the cap);
    • an off-grammar FAIL-prefixed name.

    A name-only refresh would narrow the FAIL-anywhere rule.

  • Cost. The name set is this PR's pass, pass-notes and fail, each plain and with the digest, plus the -i FAIL and the commit's other pull requests: 8 or more GETs and a GraphQL read. The incremental refresh it would replace reads 2 pages live (perf(pr-merge): list only new artifacts on top of a trusted carried listing #10488's measurement) and 5-6 in the fixture below. It would cost the same or more and cover less.

Verification

Measured with a fixture of one pr-merge tick (APrMergeTickListsOnlyBeforeAnAllowOrAKeep). Each job prints a babysit-gh-calls line.

  • Repository shape:
    • 6,240 artifacts, uploaded 4 a minute.
    • A carried listing 15 minutes old.
    • Uploads continue during the tick.
  • Merge job: the skip filter lists, then the gate candidates run.
    • Fast job: 17 candidates, 20 s apart. 8 are refused by a FAIL, 7 have a verdict that allows and are then refused by risk, and 2 are allowed.
    • Slow job: 6 candidates, 2 min apart, as in run 40594967390.
  • enable-auto-merge: 5 pull requests on --shared-listing.

"Before" is origin/main at 23f7831, which includes #10488, so the table shows only this PR's increment. Most of the cut came from #10488: the coordinator measured babysit_merge.py's pages falling from 292 to 18 a tick. Do not compare the table with the pre-#10488 197 (merge) and 152 (enable-auto-merge) calls.

job listings before → after artifact pages before → after all calls before → after
merge, fast, empty queue 4 → 3 21 → 16 87 → 84
merge, fast, 1 queued entry 4 → 4 21 → 21 95 → 97
merge, slow, empty queue 4 → 2 24 → 12 56 → 46
merge, slow, 1 queued entry 4 → 4 24 → 23 62 → 63
enable-auto-merge 1 → 1 6 → 6 28 → 28
skip filter (inside the merge job) 1 → 1 6 → 6 —
  • A queued entry needs a listing under two minutes old to stay queued, because pr-verify is not a required check. That bound sets the queued rows, and this PR keeps it. The +2 calls in those rows come from a provisional allow being judged again after refresh(); it reads no extra listing pages.
  • The empty-queue rows match live ticks: run 40594967390's merge job had "queued": [] in every gate.
  • enable-auto-merge already lists once a run (--shared-listing), so it has nothing to cut here.

Skip filter, live. It already reads the job's one listing on #10488's carry. The 71-page full listings came from two other causes:

tick carry skip-filter listing pages
40596876680 22:42Z none: first tick after #10488 ("no earlier artifact listing") 5 + 71
40600822658 23:17Z 7,121 artifacts 8 + 71: the incremental read missed the live count, then paged everything
40602269640 23:37Z 7,122 artifacts 4
  • At 23:24Z I read the same carried base again (diagnostic, user token). It held the live count, 7,120, in 4 pages, and matched a full listing taken right after id for id.
  • So the 23:17Z miss was transient. The cause is UNVERIFIED: a page boundary moving during the read would explain it, as with fix(source-control): the verdict reader's full artifact listing skips live artifacts at page boundaries, FAILs included #10486.
  • This PR adds one more incremental read on the same base before a whole listing (INCREMENTAL_READS = 2). With it, the 23:17Z case costs about 4 more pages instead of 71.
  • Skip-filter calls: 71 full pages (before) → 4 pages (23:37Z tick, the carry working). A miss like 23:17Z's should now cost about 8 pages instead of 79; that figure is an estimate, not measured.
  • All three ticks ran main's code, not this PR's.

Does the skip filter earn its place? Measured over 44 scheduled merge jobs (05:50Z to 22:42Z on 2026-10-27, from job logs):

  • It skipped 7 of 556 candidates (1.3%), one in each of 7 ticks.
  • Before perf(pr-merge): list only new artifacts on top of a trusted carried listing #10488 it cost a full listing every tick, about 70 pages. Its listing is now the job's first and the gate reads it (BABYSIT_VERIFY_LISTING), so its own cost is about 1 GraphQL point and nothing extra in listing pages.
  • Each skipped candidate saves a gate run, which cost 68-189 calls per candidate in run 40594967390.
  • It roughly pays for itself, so it stays.

Tests:

  • plugins/source-control/skills/babysit-prs/scripts/tests: 3,265 passed serially (122 s). Under -n auto (xdist), the unrelated test_babysit_lane_room.py and test_babysit_postmerge_quarantine.py tests flake on git fixtures; they pass serially.
  • New tests:
    • A FAIL on an older saved listing refuses a provisional judgment without listing.
    • A provisional allow stands on an older saved listing (saved, so the gate refreshes).
    • A clearable refusal on an older saved listing is judged again on a fresh one, and a pass uploaded since allows.
    • Only the candidate is judged provisional; queued entries are not.
    • The tick fixture pins listing counts.

Related

  • perf(pr-merge): list only new artifacts on top of a trusted carried listing #10488: the incremental listing on a trusted carry.
  • Follow-ups, not built here:
    • enable-auto-merge could reuse the merge job's listing within the run, saving 1 listing a tick. It needs a needs: edge that changes job timing.
    • The merge-queue ci-status reader (pr-require-checks.yml) still lists every page with no carry, about 69 pages per lanes App merge_group run (run 40559885862). It is now the largest artifact-listing spender per run.

🤖 Generated with Claude Code

kyle-sexton and others added 3 commits October 27, 2026 19:06
…y age

The gate's own candidate is judged provisional on the run's saved artifact
listing at any age: an allow is judged again on a listing of its own
before the gate allows (refresh), and a FAIL, the FAIL cap or an old pull
request stands, since no later upload clears them. Any other refusal is
judged again on a listing under two minutes old, so none dequeues an entry
on an older listing. Queued entries keep the two-minute bound. A candidate
whose verdict allows and that the risk score then refuses lists nothing.

A fixture of a pr-merge tick prints each job's babysit-gh-calls line and
pins its listing count.

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

The first tick on a carried base read 4 incremental pages that missed the
live count and then paged all 71 in the skip filter; the same base read
again minutes later held the count. One more read on the same base costs
about 4 pages against 71, and each read is still checked against the live
total_count.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 28, 2026 01:02
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 28, 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-10-28T01:03:18.993167Z e6410a0 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 Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review reviewed 534f8ab9b0e4ff84dae0cc3d4e0d63d30dde0bf2: 0 finding(s), 0 as inline comments.

No security findings.

critical-findings: 0

Both changed code files, babysit_verify_evidence.py and babysit_merge.py, are merge-hold paths. I read their whole change and what it gates. I read the diff and PR context only; I ran no tests or code.

Stale allow before the gate allows

  • The gate-weakening risk is a candidate allowed on an old saved listing that lacks a later FAIL upload.
  • evidence(..., provisional=True) now takes the saved listing at any age (_current_listing, babysit_verify_evidence.py:907-915).
  • The candidate is only allowed after reader.refresh() and a second, non-provisional evidence() call at babysit_merge.py:3481-3490. That second call needs a listing under LISTING_MAX_AGE old. The refresh condition is reader.saved.
  • _saved is true exactly when the judged listing came from listing_path (babysit_verify_evidence.py:914). A listing the Reader read itself is fresh and sets _saved false (:935).
  • Reader is built fresh per candidate (babysit_merge.py:3361), so no in-memory listing can be old. No path allows on a stale listing without the refresh.
  • _any_age is reset in _judge's finally. The non-provisional callers (refresh() re-judge, queued holds at babysit_merge.py:4024, --shared-listing) keep the age bound.

Refusals on an older listing

  • A STANDING refusal (FAIL, REPEATED_FAIL, TOO_OLD) keeps its refusal on the older listing without listing again. The PR says no later upload clears these; I took that from the PR and the code comments and did not check it in _evidence.
  • Any other refusal is judged again on a recent listing (evidence() lines 233-241).
  • Both cases fail closed. The risk is a wrong dequeue, not a wrong allow.

Incremental re-read (INCREMENTAL_READS)

  • A re-read happens only when the pages read whole but missed the live total_count. The live-count check stays the integrity check, so a forged carry still has to match it.
  • A failed read, a missing base or a base of another shape still returns None and goes straight to the whole listing.
  • The _complete_listing top-up now handles COUNT_MISSED through isinstance(listing, list) (:1019).

No trust control widens. Verifier-origin checks (_run_state/_origin) are unchanged. There are no manifest, workflow or token-scope changes.

Spend: fewer GitHub API calls per pr-merge tick, plus one extra incremental read (about 4 pages) on a count miss. This is rate-limit spend on the GITHUB_TOKEN, not a billing change, and it is not a hold.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

claude-review: 1 inline finding(s) on e6410a08f5bd10a1ecec4bdc4e60f5b8d1101151; the summary is in this lane's pull request comment.

claude-lane: claude-review

@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

claude-review reviewed 534f8ab9b0e4ff84dae0cc3d4e0d63d30dde0bf2: 1 finding(s), 0 as inline comments.

Reviewed the whole diff (4 files) against the PR context. There is one advisory finding, below. I found no correctness problem in the provisional-judgment logic.

Checked in the code

  • STANDING is sound. _evidence returns FAIL whenever the head is in failed, whatever passes exist, and REPEATED_FAIL and TOO_OLD only grow with time. A later upload can't clear them.
  • The allow path is still guarded. babysit_merge.py:3481-3486 calls refresh() and judges again whenever reader.saved is true. A provisional allow on an old saved listing always sets _saved, so the existing recheck before the gate allows still runs.
  • A non-standing refusal on an old listing is judged again with any_age=False. That relists, and _current_listing clears _saved when it does. Queued entries, --shared-listing and the evidence command still use the recent-listing bound.
  • _any_age is reset in finally, so it can't leak between judgments.
  • The COUNT_MISSED sentinel is handled at both call sites. _complete_listing's top-up uses isinstance(listing, list), and _current_listing falls back to the whole listing on anything that isn't a list.
  • The manifest checks don't apply. No plugin.json or marketplace.json changed. The bump: patch fragment fits a perf change plus a retry fix.
  • Spend: the change cuts GITHUB_TOKEN and API calls and adds one extra incremental read in the count-miss case. That spends no paid credit, so there is no hold.

Verified: I only read the code. I did not run the tests. The test results, the live-run measurements and the fixture timings in the PR description are author-claimed and unverified.

Finding: the retry loop gives each incremental read its own timeout. See the inline comment.

No closing issue is named in the PR context ("No related issue"). The PR body contained no embedded instructions.

critical-findings: 1

Findings not posted inline (their path, line or side is malformed):

  • plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py:921: Each pass of this loop calls self._incremental(base) with deadline=None, so every read gets its own fresh LISTING_TIMEOUT_SECONDS (300 s). _incremental returns COUNT_MISSED only after the pages finished inside their deadline. Two slow reads that each end in a count miss can therefore take about 600 s, and _complete_listing then starts its own 300 s budget. The worst case for one _current_listing becomes about 900 s. Before this change it was about 600 s: one incremental read plus the whole listing. The module's own rule (_read_listing docs and the LISTING_READS comment) is that retries share one LISTING_TIMEOUT_SECONDS so a retry "adds no time to the run's worst" case. The gate's step budget is unchanged, so the extra time can push a slow tick past it.

To reproduce, make gh_api_with_headers take about 290 s per incremental pass and answer with a mismatched total_count on both reads.

Fix: compute one deadline before the loop and pass it to each read, e.g. deadline = _listing_clock() + LISTING_TIMEOUT_SECONDS and self._incremental(base, deadline). A retry then adds no time, and the whole listing keeps its own budget as before.

kyle-sexton and others added 2 commits October 27, 2026 21:23
_incremental now answers COUNT_MISSED when its pages read whole but miss
the live count, and None for a failed read. Only a count miss is read
again on the same base; a timeout, a 429 or a rate-limit 403 pages the
whole listing at once, as _read_listing's no-retry rule says.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 28, 2026
Merged via the queue into main with commit 2774a6f Oct 28, 2026
42 checks passed
@kyle-sexton
kyle-sexton deleted the perf/shared-artifact-listing branch October 28, 2026 03:38
@github-actions

Copy link
Copy Markdown
Contributor

Post-merge mutation testing

  • Range: 5d612cf..2774a6f
  • Merged pull request: #10558
  • Outcome: no result. The run's summary says why.

The run's summary has the full report.

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