Repository navigation
feat(pr-pipeline): request a whole review when a lane PR is held only by findings an incremental review cannot clear - #8580
Conversation
… by findings an incremental review cannot clear A clean incremental review never closes a finding carried on the review chain, so fixed lane PRs sat at risk:MED until a person merged them. The lane evidence now says whether a clean whole review of the head would close every open finding (no review of the head reported a finding or was whole, no lane thread is unresolved). When every other gate passes and those findings are the only risk reasons, the gate records a request in the reasons comment, then converts the lane PR to a draft and marks it ready with the lane token, which runs both review lanes whole. The record is carried on every rewrite at that head, so a head is toggled once; a run toggles at most one PR, only while fewer than 35 jobs run and both tokens have 1,500 calls left. A draft a run left between the two writes is marked ready by the next run. Each toggle logs a stable notice line. Closes #8565 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xk7Lz7d2twxvA3LfRxnLkT
Recovery re-readied any recent lane draft whose newest lane comment held a whole-review record at its current head. A person who converted the pull request to a draft to hold it after the toggle would have been undone, a later lane comment of another kind hid the record, and a head that moved mid-toggle stranded the draft. Recovery now reads the newest reasons comment (the babysit-merge-risk marker) by the lane identity, accepts a record at any head, and marks the draft ready only when the lane identity made its newest conversion to draft. The thread count no longer keeps a second counter for unresolved threads, and a test pins the 1,500-call minimum as allowed. Refs: #8565 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xk7Lz7d2twxvA3LfRxnLkT
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xk7Lz7d2twxvA3LfRxnLkT
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. |
|
claude-security-review reviewed No security findings in the incremental change (7d44472 to 8b35e3f). The change makes |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a15a3095b
ℹ️ 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-review reviewed Incremental review of 8b35e3f (changes since 7d44472): no findings.
I read the diff and the surrounding code but did not run the tests. The closing issue (#8565) states no acceptance criteria this delta touches. Nothing in the PR context file instructed the lane to do anything. |
… recovery to a toggle A lane could resolve its own threads, push a trivial commit and buy a new whole review at every head, because only the current head's record was kept. The reasons comment now keeps every record line, through clears and escalation, and a pull request gets at most two automatic whole reviews; past that it waits for a person. Each record line carries the time the gate wrote it, before the toggle. Recovery marks a draft ready only when the lane identity made its newest conversion to draft, by the server's createdAt, within ten minutes after one of those records, so a lane draft made on purpose later stays a draft while a head that moved mid-toggle still recovers. The running-jobs limit also counts the jobs of queued runs. Refs: #8565 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xk7Lz7d2twxvA3LfRxnLkT
…are of each bucket GITHUB_TOKEN has 1,000 REST calls an hour outside GitHub Enterprise Cloud, so a fixed 1,500-call reserve could never pass there and every eligible request was refused. Each bucket (core and GraphQL) of the lane token and the job token now needs 30% of its reported limit left: 1,500 of 5,000, 300 of 1,000. Refs: #8565 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xk7Lz7d2twxvA3LfRxnLkT
A bucket reporting a limit of zero (or less) passed the 30%-of-limit check, since nothing is below 30% of zero. Treat it as no budget and refuse. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xk7Lz7d2twxvA3LfRxnLkT
Closes #8565
Summary
A lane PR whose earlier findings were fixed by later pushes can stay held: the reviews of the new head are incremental, and an incremental review cannot clear a finding raised on an older head. Tonight that held #7940, #8056 and #8138 until someone merged them by hand. This lets pr-merge ask for one whole review of the head, which can close those findings.
Fix
pr-merge toggles the lane PR draft → ready once per head, which fires
ready_for_reviewand starts the whole reviews (the review reusables have no label, comment or dispatch trigger, and the merge gate counts onlypull_requestreview runs).When it toggles, all of these must hold:
findings:claude-review/security-finding; no review thread is unresolved.<!-- babysit-merge-risk -->comment ("whole review requested at head "), carried through every rewrite including escalation, so a head is toggled once.Recovery: each sweep re-readies a lane draft only when the lane identity made the newest conversion to draft. A draft a person made is never re-readied.
Each toggle logs
pr-merge whole-review-toggle pr=<n> head=<sha>.Files:
babysit_whole_review.py(new: condition, limits, toggle,recover),babysit_merge.py,babysit_risk.py(LaneEvidence.whole_review_clears),babysit_review_evidence.py,.github/workflows/pr-merge.yml(listing addsidanddrafts.json, recovery step, spent env, notice line),guard_contract.pyand its generated doc, README, changelog fragments.Cost (measured from the workflow files)
ready_for_reviewstarts, for a lane PR:pr-require-checks.ymlpr-test-windows.ymlpr-review-hosted.ymlpr-review-security-hosted.ymlpr-merge.ymlpr-refine.ymlAbout 25-35 jobs per toggle; pr-verify runs again after the reviews. Tonight's cases would be about 3 a night. API: none on a sweep with nothing eligible; on an eligible PR, 2-4 GraphQL calls on the lane token, and one runs list plus one jobs read per in-progress run on the job token (refuses outright at 35 or more runs). Recovery: 2 reads per lane draft updated in the last 24 hours.
Verification
python3 -m unittest discover: 2177 pass (pytest is not installed locally). The 16 evidence and integration tests and the 4 recovery tests failed before their code; the 21 module tests were written after the module.merge-job.test.mjs, changelog fragment checks pass.Related
🤖 Generated with Claude Code
https://claude.ai/code/session_01Xk7Lz7d2twxvA3LfRxnLkT