[kyle-sexton]

No related issue: follow-up to #12302's security review

Summary

Two non-blocking security follow-ups from #12302 (the exact-name FAIL index, squash commit 23dfa3f):

• legacy-fails refuses a FAIL uploaded before --since. The build lists runs from --since on and adds the rest through one whole artifact listing that carries no count check. A FAIL uploaded before --since means --since was set too late, so the build now refuses with exit 1 and writes nothing. The message names the earliest such artifact, its run and its upload time, and tells the operator to lower --since to at most that time, remove the checkpoint file and run again. A FAIL of a run created before --since but uploaded at or after it (a re-run or a late judgment) is still added through the listing.
• upload-fail-index retries its pre-read. The read of actions/runs/<run_id>/artifacts and its count check get three attempts, 5 s and then 10 s apart, before the step fails. Before this change, one transient API error failed the step, and the FAIL judgment was left with no verdict.

Fix

(a) babysit_verify_evidence.py, _PerRunBuild

• _listing_fails now keeps each listed FAIL's created_at.
• The new _uploaded_since runs after the listing is read and after the second running-status check, and before any listed run is read by id. It refuses on:
  • a listed FAIL with no readable created_at, which fails closed;
  • any listed FAIL created before --since, naming the earliest.
• Old checkpoints are re-listed, not refused. A checkpoint whose listed array has an item without a created_at key reads the listing again. Every finished window and run in it is kept, and LEGACY_CHECKPOINT_VERSION is unchanged.
• The refusal says to remove the checkpoint file, not to start a new --checkpoint path. The pacer keeps its per-window call count in <checkpoint>.window. A new path would start a fresh count and reset the cap inside the same GitHub reset window.

(b) .github/actions/upload-fail-index/action.yml

• The read and the total_count check sit in a loop of three attempts, with sleep 5 and sleep 10 between them. Each failed attempt logs a ::warning:: that names the reason. The last one logs ::error:: and exits 1.
• Every other check is unchanged: the retention range, the 3-13 name count, the name grammar and the held-name refusal.
• A run that really holds 100 or more artifacts still fails, now after about 15 s of retries instead of at once.

Verification

• pytest plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_index.py: 38 passed. Three tests are new:
  • Pre---since FAILs. Two listed FAILs uploaded before --since refuse, the message names the earliest (artifact 90 of run 9 at 2026-10-13T20:00:00Z), nothing is written, and run 9 is never read.
  • No creation time. A listed FAIL with no created_at refuses.
  • Old checkpoint. A checkpoint whose listing lacks created_at reads the listing again.
• The existing tests still hold. The test "a FAIL of a run from before --since is added through the listing" passes: run 8 was created before --since and uploaded its FAIL after it. The RunsGitHub fixtures now give each artifact an upload time inside its run's hour.
• The new tests fail on main's code. With the source change stashed, all three fail (3 failed, 35 passed).
• node --test .github/scripts/pr-verify/workflow.test.mjs: 32 pass. The upload-fail-index case, run with a stand-in gh and a stand-in sleep, checks four things:
  • A read that always fails makes 3 reads with waits of 5 and 10 and writes no slots. A read that misses its count does the same.
  • A read that fails twice and then succeeds uploads every index.
  • A held name still fails the step after a retried read.
• The action test fails on main's step. Run against main's action.yml, the case fails: it got 1 read and no waits, where the test expects 3 reads and waits of 5 and 10.
• scripts/run-ruff.sh check on both Python files: clean.

Related

• perf(source-control): find pr-verify FAILs by exact-name index instead of the whole artifact listing #12302: the exact-name FAIL index these changes follow up.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

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-02T14:08:31.422913Z	6464bcd	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]

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

Code review: no findings

I read every hunk in all 6 changed files. I also read the code around _PerRunBuild in babysit_verify_evidence.py (load, _listing_fails, _run, build, _when, _stamp) to check how the new guard fits in.

legacy-fails refuses a FAIL uploaded before --since (babysit_verify_evidence.py, a trusted path)

• _listing_fails now keeps the artifact's created_at, which is its upload time. _uploaded_since runs after the second _running_check and before any run is read by id. A missing or unparseable time refuses with _Unreadable, so it fails closed.
• The lower --since to at most <stamp> advice is sound. _stamp truncates to the second, so the suggested time is never later than the real upload time, and that FAIL then passes created >= since.
• The advice to remove the checkpoint file is correct. load() already refuses a checkpoint whose since differs, and keeping the <checkpoint>.window pacer file preserves the per-window call cap, as the PR body says.
• Old checkpoints: an item without created_at, or a listed that is not an array, triggers a fresh listing. An empty saved listing is reused, which is correct because it has no items to check.
• One side effect, not counted as a finding: a listed FAIL with no run id (which build ignores) can now also trigger the refusal if it was uploaded before --since. That fails closed, and lowering --since clears it.

upload-fail-index retries its pre-read (action.yml)

• The loop is correct under bash -eo pipefail. The gh and jq failures sit inside if and elif conditions, so errexit does not stop the step. body keeps the successful read on break. The final attempt prints ::error:: and exits 1. The checks after the loop are unchanged.
• The test's stand-in gh and sleep cover four cases: a read that always fails, a read that misses its count, two failures followed by success, and a retried read that then shows a held name. The test checks the read count and the waits (5, 10).

Tests: the new Python tests cover the refusal message (earliest artifact, its run and its time), a missing creation time, and re-listing an old checkpoint. I did not run any tests. The PR's pytest and node --test results are claimed by the author and I have not verified them.

Changelog fragments: both are bump: patch, which fits a fix. No plugin manifest changed, so there are no manifest lines to report.

Closing issue: none. The PR is a follow-up to #12302 and states no acceptance criteria to check. The PR context contains no embedded instructions.

Spend: when the pre-read keeps failing, the retry adds up to about 15 s of runner time per upload-fail-index step. This is not a hold.

[github-actions]

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

Security review: PR #12370

I found no security issues in the six changed files. I read every hunk of each one.

critical-findings: 0

Trusted path checked: plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py is a merge-hold control, so I read its whole change and what it gates.

• New _uploaded_since check (L5730): this only adds refusals. A listed FAIL with an unreadable created_at raises _Unreadable. _when (L4236) returns None for an unparseable or timezone-less value, so that case fails closed too. A FAIL created before --since also raises _Unreadable.
• Order of the check: it runs after both _running_check calls and before any listed run is read or added, so nothing new gets through.
• Checkpoint re-listing: the base re-listed only when the listed key was missing. The head also re-lists when listed is not an array or an item has no created_at. That means a fresh read from GitHub replaces stale or malformed local state. It does not trust that state more than before.
• Result: no merge hold is weakened.

.github/actions/upload-fail-index/action.yml retry loop: each attempt runs the same gh api read and the same total_count == length and < 100 check as before. body is only used after a break on an attempt that passed both checks, and the third failure still exits 1. why is a fixed string, and REPO and RUN_ID come in as environment variables, so no expression or command injection was added. The held-name, retention, name-count and name-grammar checks are unchanged, so the action lets through nothing the base stopped.

Other files: the two tests and the two changelog fragments have no security surface. The tests use stand-in gh and sleep scripts written into a temporary directory.

Other notes:

• No plugin manifest is changed.
• The PR text contains no embedded instructions.
• The round cap does not apply: 0 prior rounds, cap 3.
• Spend: the retry adds at most about 15 s of runner time and two extra artifact-list API calls, and only when the pre-read fails.