Repository navigation
perf(source-control): find pr-verify FAILs by exact-name index instead of the whole artifact listing - #12302
Conversation
Each FAIL now uploads one small artifact per tie the merge gate counts it by: pr-verify-index-pr-<n>, -sha-<head>, -ref-<digest> and -issue-<i> (write-verdict.mjs failIndexNames, the upload-fail-index action). The verdict is scanned and uploads only when it is no FAIL or every index uploaded, so a FAIL never exists without its index. The API's name filter returns every artifact of an exact name across runs, which lets the reader find a FAIL without the whole listing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Once verify-legacy-fails.json (every FAIL from before the index) sits beside the reader, a judgment reads no artifact listing. It reads the index names of the pull request, its closers, its head and commits, its head branch and its live issues, each named run's artifacts, and the pass names at the head and at each FAILed head. Each read must hold its own total_count; any failed read refuses. The answer key, the skip filter's key (Reader.own_verdicts) and fail_findings read the same way. With no legacy file, or past INDEX_NAMES_CAP names, the whole listing is read as before. legacy-fails builds the file from a listing that holds the live count, after every verifier run from before the cutover has completed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A re-run of a pr-verify run from before the index runs its old workflow, which has no index step, on main's write-verdict.mjs. Only a job that sets FAIL_INDEX may now write a FAIL, so no FAIL appears without its index once this lands. The index carries the issues the FAIL's name carries, so a FAIL whose issues fit no name still uploads under its plain name, as before. ci-status's sparse checkout now takes verify-legacy-fails.json beside the reader, and the babysit test package pins no legacy file, so the suites keep faking the whole listing once the file is committed. The answer key's index branch gets a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
legacy-fails no longer depends on an artifact listing settling. It lists every pr-verify.yml run in one-day created windows (split at the 1,000-result cap), requiring each window's listed ids to equal its total_count, then reads each run's artifacts in one page that must hold its own count. A run not completed or any read that fails or miscounts refuses with nothing written. Every call is paced (0.5 a second by default), capped at 1,800 per rate-limit reset window, and stops on a rate-limit answer, an open breaker file or core remaining under 2,000, with a per-window call line on stderr. A stop exits 3 and resumes from the checkpoint file. The listing build stays only as --cross-check-listing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9babebd465
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
claude-security-review reviewed Security review: PR #12302, incremental (696ba56 → 44b849d)I read every hunk of the 4 in-scope files in critical-findings: 0 Prior findingMy earlier IMPORTANT finding (discussion_r4439678973) is addressed. A re-run's FAIL could outlive an index that had been skipped. Now, when this run already holds an index name, the step fails (action.yml#L78-L81). A FAIL upload in all three jobs requires Paths checked (no finding)
Tests
Spend (not counted as a finding)The verdict reader makes one extra prepaid GitHub API read per index run that holds no FAIL. The |
|
claude-review reviewed Code review (incremental,
|
…nion A FAIL can come from a run created before --since: a re-run keeps its created_at, and a three-job chain can upload hours after it. The build now also reads one whole artifact listing through the pacer, reads each run a listed FAIL names outside every window with the same exact per-run artifact read, and refuses, naming the runs, when a listed FAIL is still missing. The cutover comes from --merge-sha, the commit on main whose pr-verify.yml sets FAIL_INDEX, plus a one-hour margin; an earlier --cutover refuses. The last window now reads the cutover's own second. Also: index uploads overwrite a same-named artifact, so a re-run attempt does not fail on its earlier attempt's index; _own_fails drops expired legacy entries; a permission 403 refuses instead of stopping as a rate limit; the merge-skip docstring names index mode. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-flight runs overwrite deletes a same-named artifact before the new upload, so a failed upload on a re-run attempt would drop attempt 1's pointer. The index step now reads the run's artifacts once (holding its count) and skips every index name the run already holds, which already points to this run. legacy-fails now lists runs created in the 31 days before --since (GitHub allows a re-run up to 30 days after the initial run) with the same windowed count check, and reads each one updated since --since; the listing union stays as a second check. It again refuses on any verifier run created at or before the cutover that is still queued or running, read per status with its count, before and after the listing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A run seen completed with only its index is read again, since its FAIL may have uploaded between the two reads. - A legacy-fails window holding an unfinished run refuses before it is kept, so a resume lists that window again. - The pacer keeps its reset window and call count beside the checkpoint, so a resume inside the same window keeps its cap. - The index step fails when the run already holds one of its names from an earlier attempt, so a re-run attempt writes no FAIL that would outlive the index it kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No related issue: a measured API-budget fix for the verdict reader, asked for in session; no issue was filed.
Summary
The pr-verify verdict reader proves "no FAIL tied to this PR" by paging the whole-repo
actions/artifactslisting, about 135 pages, and accepting it only when its ids equal GitHub's livetotal_count. That check rarely settles under this repository's upload churn. Between 03:44 and 04:44Z on 2026-11-02, whole-listing pages made up 1,290 calls and drove aGITHUB_TOKENbreaker trip.This PR makes every FAIL findable by exact-name lookups:
pr-verify-index-pr-<n>,-sha-<head>,-ref-<head-ref digest>, and-issue-<i>for each issue the FAIL's name carries.actions/runs/<run_id>/artifactsunder the job'sactions: readtoken. If the run already holds one of its index names from an earlier attempt, the step fails, so a re-run attempt writes no FAIL (verify:missing). That attempt then neither deletes the earlier pointer, whichoverwritewould, nor leaves a new FAIL that outlives a kept index.write-verdict.mjs.total_count.verify-legacy-fails.json, built once by the newlegacy-failssubcommand.The merge-gate intent is unchanged. A counted FAIL still refuses, and any read that fails refuses (
verify:unreadable).Fix
Measurements
Basis: one whole listing taken 2026-11-02 05:01:15-05:03:32Z. That was 135 pages, saved in the session's
.work/measure/pages, and summarized with jq/python over that file.1a. Why
total_countruns above the listed idstotal_countwas 13,468 on the first page and 13,470 on the last. Over the read it ranged from 13,466 to 13,471.expired: true.expires_atbefore the first page'sDate.total_countstayed 1-8 above a top-up 88-110 s after a fresh listing, is not explained by this one read. The new design does not depend on it.1b. FAIL-named artifacts
1c. Listing by name prefix: count, retention days min-max
pr-verify-*-pass-notespr-merge-dispatchedskill-reply-work-resolve-issuepr-verify-*-failpr-verify-*-passmutation-rangepr-review-main-claude-reviewpr-review-main-claude-security-reviewAbout 50 more families hold 0-7 day artifacts. Workflow is inferred from each name's upload site, since the listing carries only
workflow_run.id.?name=mutation-range&page=6returnedtotal_count502 with 2 items on the page, across runs.actions/runs/41390871184/artifactsreturnedtotal_count1 and 1 item, carryingworkflow_run.id.Design
Legacy FAILs need a closed legacy set, not a self-ending fallback.
legacy-failsdoes.--merge-sha, the merge commit that brought FAIL_INDEX to main. That commit must be on main, and itspr-verify.ymlmust setFAIL_INDEX: "1". The cutover is its committer date plus a 1 h margin. An explicit--cutoverearlier than that refuses; a later one only reads more runs.--sinceto the cutover, the cutover's own second included, in one-daycreated=windows. A window is split while its count reaches GitHub's 1,000-result cap. Each window's listed run ids must equal itstotal_count; a past window is stable, since runs are not deleted inside retention.actions/runs/<id>/artifacts, one page whose ids must equal itstotal_count.[since - 31 days, since)with the same windowed count check, and reads each one whoseupdated_atis at or after--since. GitHub allows a re-run up to 30 days after the initial run (Re-running workflows and jobs, read 2026-11-02), and a re-run keeps itscreated_at. 31 days covers that limit with a day to spare.--rerun-lookback-dayssets it.total_count, and refuses on any verifier run created at or before the cutover.created_at, and a three-job chain can upload a FAIL hours after it. So the file is the union of the windowed runs and the listing.x-ratelimit-remaining: 0, 429, or retry-after. It also stops when the breaker file exists or core remaining is under 2,000, checked by arate_limitread at start and in each window. A stop exits 3 and resumes from the checkpoint file. Every finished window and run is kept there. It prints onelegacy-fails: window reset=... calls=... total=...line per window. All five limits are flags.Tie coverage. Each rule in the module docstring, and what finds it in index mode:
verify:fail,verify:earlier-fail, the cap)pr-verify-index-pr-<n>pr-verify-index-sha-<head>pulls/<n>/commits)pr-verify-index-sha-<c>per commit; pastINDEX_NAMES_CAP(100) names, the whole listingpr-verify-index-ref-<digest>, uploaded by the FAIL's own verifier run fromverdict.head_refpr-verify-index-pr-<p>for each closer from the GraphQL already readpr-verify-index-issue-<i>pr-verify-<n>-<sha>-passand-pass-noteshead_refand origin recordedTwo deliberate narrowings, both fail-closed or justified.
verify:unreadablefor every PR.head_ref. The build refuses when one cannot be read.artifactName), and resolve-pr reads only the plain names. A pass missed this way leavesverify:missing.Pointer safety.
INDEX_RUNS_CAP(120) runs refuses.Consumers moved. These all read in index mode:
_evidence_own_fails/fail_findings_answer_key(Reader.targeted, handed to the next judgment of the same head)Reader.own_verdicts)_late_passandrefresh()need no listing there. Each judgment reads fresh, andrefresh()drops a kept hand-off.Retention (step 3): no change.
pr-merge-dispatched.github/artifact-retention.json).pr-review-main-*pr-merge-rerun-*branch-cleanup-ledger-*Verdict artifacts are untouched. The new
pr-verify-index-*row keeps the FAIL's 90 days, so an index never expires before its FAIL.Per-run call delta per caller
Before the follow-up commits
verify-legacy-fails.json, every caller's delta is 0. The reader stays on the listing path.The listing grows meanwhile:
So the follow-up should land as soon as old runs drain.
After it lands, per judgment:
prrefThat comes to about 8-11 calls for a PR with no FAIL and about 15-20 with two FAILed heads. These are estimates (
judgmenton the formula), to be re-measured from the newbabysit-index-readsstderr line and the existingbabysit-gh-callsroute counts.verify:unreadableverify:unreadablemergejob 125086673975, babysit_merge_skip.py (5 candidates)own_verdictsat 4-8)mergejob 125086673975, babysit_merge.py (5 candidates; budget ran out after 3)Breakeven. One whole listing costs about 135 calls, so index mode costs more only when one process judges more than about 11 PRs. Past
INDEX_NAMES_CAPnames, one judgment falls back to a listing.Follow-up (not in this PR)
BABYSIT_VERIFY_HEAD_REFSto a head-ref map from a scheduled pr-merge run. Run it once, and run it again with the same arguments after any stop (exit 3):python3 plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py legacy-fails --repo melodic-software/claude-code-plugins --since 2026-10-17T00:00:00Z --merge-sha <this PR's squash commit on main> --checkpoint <file> --out plugins/source-control/skills/babysit-prs/scripts/verify-legacy-fails.json --breaker-file <hub breaker.txt>--sincecomes from the 05:01Z listing: its oldest unexpired FAIL was created 2026-10-17T05:11Z. A run created earlier with a FAIL still unexpired is caught by the build's listing union, not by--since.Expected calls (estimate,
judgmenton the measured counts): 2,500 runs created since 2026-10-17 as of 05:5xZ, plus about 150 a day up to the cutover. The total is about 2,550-2,800 run and window reads, one compare per distinct FAIL run head (at most about 550), up to about 550 verdict downloads without a head-ref map (few with one), onerate_limitread per window, 3 reads for the merge commit, 15 running-status reads (5 at each of three checks), about 31-62 re-run lookback window reads plus 2 per lookback run updated since--since, and one whole listing at about 135-250 pages (more as index artifacts accumulate), plus 2 per run the listing adds. That makes about 2,800-4,250 calls in all: 2-3 reset windows at the 1,800 cap, about 1.6-2.4 h of pacing at 0.5 a second.Verification
node --testpassed:.github/scripts/pr-verify/write-verdict.test.mjsandworkflow.test.mjs: 71 pass.reconcile,verify-notes,workflow-gatesandresolve-prtests: 369 pass.plugins/source-control/skills/babysit-prs/scripts/tests: 3,973 passed, 6,523 subtests.test_babysit_verify_index.py: index-mode allow with no listing read; FAIL at the head under another number; earlier head; head-branch tie; issue-named cap; legacy FAIL on recorded origin; expired legacy; still-uploading run; failed or short name read; the names cap falling back to the listing; the runs cap;own_verdicts; an expired legacy FAIL left out offail_findings.legacy-fails: the built file; a window or a run's artifacts that miss their count refuse with nothing written; a run not completed refuses; a rate-limit answer stops with exit 3 and the checkpoint resumes without re-reading; a rate limit inside a verdict download still stops; the window cap; the breaker and low-remaining stops before any read; pacing at 0.5 a second; a checkpoint for another range refuses; the last window reads the cutover's own second; a FAIL of a run created before--sinceis added through the listing; a listed FAIL that no run read holds refuses, naming the run; a cutover before the merge commit refuses; a merge commit without FAIL_INDEX refuses; a permission 403 refuses with exit 1 rather than stopping; a run created before--sinceand updated after it is read through the lookback windows, and one not updated since is not; a lookback window that misses its count refuses; a pre-cutover verifier run still in progress refuses, and a later one does not; a run unfinished at the first build is listed again on resume; a resume inside the same reset window keeps counting toward its cap; in index mode, a FAIL uploaded while its run completes still counts.upload-fail-index's name split, run with a stand-ingh: it fails on an index name the run already holds, uploads nothing on a failed or miscounted read, and sets nooverwrite.FAIL_INDEXrefusal.scripts/check-artifact-retention.shand its test: 32 pass.scripts/ci-lanes-pr-hold.test.sh: 300 pass.zizmor --no-online-auditsonpr-verify.yml,pr-require-checks.ymland the new action: no findings.scripts/run-ruff.sh check: clean.cache-modejob key, which its local version does not know.Related
🤖 Generated with Claude Code