Repository navigation
feat(review): converge CI review lanes across rounds - #12011
Conversation
…he lane's own posts Rule 1 (review only what changed since the lane's last reviewed head) already ships in ci-workflows pr-review-cli.yml (incremental-review, default true), so this adds rules 2 and 3. plugins/review/lib/prior-findings.mjs lists a lane's own earlier findings for its wrapper to hand the model: inline review threads, reviews and PR comments, kept only when the github-actions bot wrote them (numeric id), no other account edited them, and they carry the lane's footer or reviewed-head marker. For each anchored finding on a thread still open it records whether git diff <posted head> HEAD touches the lines it names (old-side hunk math), and counts the lane's rounds against loop.review-rounds read from a default-branch pr-pipeline.yaml. A resolved thread, and anything it cannot judge, answers "changed: yes", so it never suppresses: babysit_review_evidence.py counts every open lane thread whatever later reviews report, but reads a clean review as clearing a resolved one. /review:code-review and /review:security-review read the PRIOR FINDINGS file: a finding already posted on an open thread whose lines are unchanged is not posted again, at any severity, and gets one uncounted summary line. Past the cap the code lane posts only blocking findings and the security lane only CRITICAL and IMPORTANT. With no such file both lanes review as before. The cap reuses loop.review-rounds (3), whose schema already defines it as the full review rounds before reviews narrow to high severity. Observed-stumble evidence for the new standing instructions: the 2026-11-01 survey on #7777 (issuecomment-6559771252) found 24 repeats of a lane finding on a byte-identical file across 597 same-lane same-path pairs, 15 of them before anyone replied, plus #10397, #9483 and #11820. Not live until ci-workflows pr-review-cli.yml runs the script in a no-model step and passes its note to the compose step. Refs: #7777 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. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
claude-review reviewed claude-review incremental review of The change reverses the "Prior findings" rule in both lane skills. A finding that was already posted on an open thread with unchanged lines is still not posted again inline. Its
The stated reason holds up. The open thread does not keep holding the merge by itself, because an agent-resolved thread closes once a clean whole review of the lane completes. So an uncounted repeat would let that clean review close the thread without anyone checking it. What I checked:
I ran nothing. The test counts in the PR's Verification section were reported by the author and are unverified here. Not counted as a finding: the PR description is now out of date. Its Fix section still says a repeat "gets one uncounted summary line", which contradicts this commit and the changelog fragment. Worth updating so the merge reviewer reads the current rule. No findings in this increment. |
![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: e173d95ffd
ℹ️ 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 (incremental: 15df0cc → 027fa92)No security issues found in this change. What I reviewed: every hunk in What changed: a repeat finding that the prior-findings rule suppresses is now counted as if it were posted. In both
Before this change, a suppressed repeat was uncounted and set no hold. Why this narrows the gate:
The other two files:
Embedded text in the PR context: none. Instruction-surface lens: not triggered, because the diff tightens a standing rule and removes none. critical-findings: 0 |
A round is now one distinct head before the head under review on which the lane posted at least one finding: an inline thread, a review, or a reviewed-head summary comment whose status line counts one or more findings (head taken from its marker). A rerun's posts on the head under review no longer count, and a skip or zero-finding summary does not count. The lane footer is read bare or in backticks, matching clear-escalations.mjs LANE_TAG and babysit_review_evidence.py LANE_TAG_RE, and stripped in either form. Refs: #7777 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A finding the lane already posted on an open thread whose lines are unchanged was given an uncounted `already posted` summary line, on the claim that the open thread keeps holding the merge until resolved. The gates do not work that way: babysit_review_evidence.py closes a thread an agent resolved once a clean whole review of the lane completed after it opened, judging clean by the record's `findings=` (from the model's own count), and clear-escalations reviewLanesClean reads the security marker's `critical-findings: 0`. A review that suppressed a repeat it still found therefore read clean, and a later agent resolution closed the thread with no review re-checking it. Both lane skills now count the suppressed repeat exactly as the posted finding would (findings count, `critical-findings:` when CRITICAL, and `hold`), and only skip the new inline comment, so every gate is as strict as when the lane re-posted it. The changelog fragment and one test title that repeated the false claim are corrected. Refs: #7777 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs: #7777
Summary
The CI review lanes re-post the same finding round after round on code that has not changed. The #7777 survey found 24 repeats of a lane finding on a byte-identical file across 597 same-lane, same-path pairs, 15 of them before anyone replied. Rule 1 of the issue (review only what changed since the lane's last reviewed head) already ships in ci-workflows
pr-review-cli.yml(incremental-review, default true). This PR adds rules 2 and 3: tell each lane what it already posted, and narrow it after the round cap.Fix
plugins/review/lib/prior-findings.mjslists a lane's own earlier findings (inline threads, reviews, PR comments). It keeps only posts the github-actions bot wrote (numeric id 41898282) that no other account edited and that carry the lane's footer or reviewed-head marker. For each anchored finding on an open thread it records whethergit diff <posted head> HEADtouches the named lines (old-side hunk math), and counts the lane's rounds againstloop.review-roundsread from the default branch'spr-pipeline.yaml. A resolved thread, or anything it cannot judge, reads "changed: yes", so the script never suppresses on doubt./review:code-reviewand/review:security-reviewread that PRIOR FINDINGS file. A finding already posted on an open thread whose lines are unchanged is not posted again, at any severity, and gets one uncounted summary line. Past the cap, the code lane posts only blocking findings and the security lane only CRITICAL and IMPORTANT. With no file, both lanes review as before.loop.review-rounds(3), whose schema already defines it as the full rounds before reviews narrow to high severity.reviewchangelog fragment.Verification
plugins/review/tests/prior-findings.test.sh: 13/13.ci-review-lanes.test.sh: 239/239.check-affected: 14 passed, 2 skipped (build worker's run; the pre-pushcheck-affectedhook passed again on this push).Related
pr-review-cli.ymlrunsprior-findings.mjsin a no-model step and hands its note to the compose step; that follow-up is ci-workflows#950.security-review/SKILL.md"Settled points" never drops a settled CRITICAL, so this build does not stop them. Whether to change that is an owner decision, asked in the session that opened this PR.🤖 Generated with Claude Code