Skip to content

Commit f676570

Browse files
committed
fix(ci): make the review cap per-PR and require exhaustive findings [skip changelog]
Two problems with what #1290 shipped, both surfaced by that PR taking 13 review rounds. The 3-run cap was per-commit, not per-PR. The API query filtered on `head_sha`, so the counter reset on every push - a PR could take unlimited reviews at three per commit, which is the opposite of a cap. When the reviewer pointed this out I corrected the log message and the skip comment to say "for this commit", making the wording match the bug instead of fixing it. #1290 is the evidence: 15 successful review runs on one PR under a "3 run" cap. Now scoped with `branch=<head ref>`, which spans the PR's whole life, with `--paginate` and a summing awk so a PR with more than 100 runs still counts correctly. Verified against #1290's branch: returns 15, and an unknown branch returns 0 rather than empty. The prompt also never asked for completeness, which is why 13 rounds happened. Five of them were one class of bug - advice contradicting its own check - surfacing a corner at a time: crate direction, root direction, mixed case, per-file lines, all-misnamed. Each round found one instance, I fixed that instance, and the next round found the next. Under a real per-PR cap that pattern exhausts the budget without converging. The prompt now requires sweeping the whole class before reporting, with the specific generalizations that would have collapsed those five rounds into one: check parallel lists for the same omission, grep for other places repeating a contradicted claim, ask what the opposite direction does, check sibling files. Also asks for confidence and verification method per finding, and to say plainly when something could not be verified rather than presenting inference as measurement - the reviewer did this well unprompted and it is worth making explicit.
1 parent b976987 commit f676570

1 file changed

Lines changed: 46 additions & 14 deletions

File tree

.github/workflows/claude-code-review.yml

Lines changed: 46 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -56,16 +56,24 @@ jobs:
5656
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
5757
REPO: ${{ github.repository }}
5858
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
59+
HEAD_REF: ${{ github.event.pull_request.head.ref }}
5960
EVENT_ACTION: ${{ github.event.action }}
6061
PR_NUMBER: ${{ github.event.pull_request.number }}
6162
run: |
6263
WORKFLOW_FILE="claude-code-review.yml"
6364
65+
# Scoped to the PR's branch, not its head SHA. Filtering on head_sha
66+
# made this a per-commit cap that reset on every push - so a PR could
67+
# take unlimited reviews, three per commit, which is the opposite of
68+
# the intent. `branch` matches the PR's head ref and so spans the whole
69+
# PR.
6470
RUN_COUNT=$(gh api \
6571
-H "Accept: application/vnd.github+json" \
6672
-H "X-GitHub-Api-Version: 2022-11-28" \
67-
"repos/${REPO}/actions/workflows/${WORKFLOW_FILE}/runs?head_sha=${HEAD_SHA}&status=completed" \
68-
--jq '[.workflow_runs[] | select(.event == "pull_request" and .conclusion == "success")] | length' || echo "")
73+
--paginate \
74+
"repos/${REPO}/actions/workflows/${WORKFLOW_FILE}/runs?branch=${HEAD_REF}&status=completed&per_page=100" \
75+
--jq '[.workflow_runs[] | select(.event == "pull_request" and .conclusion == "success")] | length' \
76+
| awk '{total += $1} END {print total + 0}' || echo "")
6977
7078
RUN_COUNT=${RUN_COUNT:-0}
7179
if ! echo "$RUN_COUNT" | grep -qE '^[0-9]+$'; then
@@ -75,22 +83,19 @@ jobs:
7583
7684
RUN_COUNT=$((RUN_COUNT + 1))
7785
78-
# Note this counts SUCCESSFUL runs for THIS commit: the API query
79-
# filters on head_sha, so the cap resets on every push. Filtering on
80-
# `conclusion == "success"` matters because the concurrency group
81-
# cancels superseded runs, a cancelled run reports
82-
# `status=completed`, and non-push events (draft -> ready_for_review,
83-
# close/reopen) fire on the same sha - so cancellations would
84-
# otherwise burn budget slots without ever producing a review. In-flight
85-
# runs are still uncounted; the concurrency group prevents overlap.
86-
echo "This is run #$RUN_COUNT for commit ${HEAD_SHA:0:8} (event: $EVENT_ACTION)"
86+
# Counts SUCCESSFUL runs across the whole PR. `conclusion == "success"`
87+
# matters because the concurrency group cancels superseded runs and a
88+
# cancelled run still reports `status=completed`, so cancellations would
89+
# otherwise burn budget without producing a review. In-flight runs are
90+
# uncounted; the concurrency group prevents overlap.
91+
echo "This is review #$RUN_COUNT for PR #${PR_NUMBER} (${HEAD_REF}, event: $EVENT_ACTION)"
8792
8893
if [ "$RUN_COUNT" -le 3 ]; then
8994
echo "should_run=true" >> $GITHUB_OUTPUT
90-
echo "[OK] Will run Claude review (run $RUN_COUNT of 3 for this commit)"
95+
echo "[OK] Will run Claude review (review $RUN_COUNT of 3 for this PR)"
9196
else
9297
echo "should_run=false" >> $GITHUB_OUTPUT
93-
echo "[SKIP] Skipping Claude review (already ran 3 times for this commit)"
98+
echo "[SKIP] Skipping Claude review (already ran 3 times for this PR)"
9499
fi
95100
96101
# Toolchain and cache come after the run-count gate: both are pointless
@@ -190,6 +195,33 @@ jobs:
190195
clippy already gate those. Report only what a maintainer would act on,
191196
and say so plainly if the diff looks correct.
192197
198+
**Be exhaustive in one pass.** You get at most 3 reviews per PR, so
199+
this may be the only one. For every problem you find, before
200+
reporting it, search the diff and the files it touches for every
201+
other instance of the same class and report them together as one
202+
finding. Do not report one instance and stop.
203+
204+
Concretely, if you find:
205+
206+
- a list or allowlist missing an entry, check whether the parallel
207+
lists elsewhere are missing it too, and whether that list is
208+
missing anything else
209+
- a message, comment, or doc line that contradicts the code it
210+
describes, grep for the other places making the same claim
211+
- a check that handles one direction, ask what the opposite
212+
direction does
213+
- a fix applied in one file, check whether its sibling files need it
214+
215+
A finding that says "and the same applies at X, Y, Z" is worth far
216+
more than three rounds discovering X, then Y, then Z. If you fix one
217+
corner of a class and leave the others, the next push spends another
218+
of the three reviews on the same bug.
219+
220+
State your confidence and how you verified each finding. If you
221+
could not verify something - a tool was unavailable, a fixture was
222+
impractical - say so explicitly rather than presenting inference as
223+
measurement, and say what would close the gap.
224+
193225
Use inline comments for specific lines; one top-level comment for
194226
overall assessment.
195227
@@ -199,4 +231,4 @@ jobs:
199231
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
200232
PR_NUMBER: ${{ github.event.pull_request.number }}
201233
run: |
202-
gh pr comment "$PR_NUMBER" --body "[INFO] Claude Code review was skipped as it has already run 3 times for this commit."
234+
gh pr comment "$PR_NUMBER" --body "[INFO] Claude Code review was skipped as it has already run 3 times for this PR."

0 commit comments

Comments
 (0)