Skip to content

fix(review): count review rounds by commit, not review state #19

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zfarrell merged 6 commits into from
Jul 29, 2026
Merged

fix(review): count review rounds by commit, not review state #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
380616e
test(review): cover review cycle counter with captured API fixtures
zfarrell Jul 29, 2026
b7d93e8
fix(review): count review rounds by commit, not review state
zfarrell Jul 29, 2026
55c52d2
fix(review): keep cycle counter from failing the review step
zfarrell Jul 29, 2026
02b2ac2
fix(review): guard prior-comment fetch and scope the drift warning
zfarrell Jul 29, 2026
791e9a6
refactor(review): simplify drift warning; warn on unparsable reviews
zfarrell Jul 29, 2026
a0fc644
test(review): assert the drift-warning predicate
zfarrell Jul 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 39 additions & 6 deletions .github/workflows/claude-pr-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,46 @@ jobs:
PR_NUMBER=${{ github.event.pull_request.number }}
REPO=${{ github.repository }}

CYCLE=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews" \
--paginate | jq -s '[.[][] | select(.user.type == "Bot" and (.state == "CHANGES_REQUESTED" or .state == "APPROVED"))] | length')
# Count distinct commits already reviewed, never review state: the org ruleset
# sets dismiss_stale_reviews_on_push, so a push flips a prior APPROVED to
# DISMISSED and a state filter stops matching it. Inline comments each create
# their own COMMENTED review sharing the round's commit_id, so unique commit_id
# == round count, +/-1 when a push lands mid-round and splits it across two SHAs.

@claude claude Bot Jul 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: the +/-1 note covers the mid-round push split, but the opposite-sign case in the same family isn't listed: rounds that share a SHA collapse into one. This workflow triggers on ready_for_review and reopened, neither of which moves the head SHA, and a manual re-run of the job doesn't either — so draft → review → ready_for_review → review counts as one round and the ladder stays put. Unlike the split case it isn't bounded at 1: N reviews against a single SHA count as 1, not N-1 too many.

In practice re-review is push-driven so the exposure is small, which is why this is worth a clause here (and in the limits list at tests/review-cycle-test.sh:66-70) rather than a fix — it reads as an oversight otherwise, since the trigger list at the top of this file makes both same-SHA events reachable. (not blocking)

@zfarrell zfarrell Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining, because I think collapsing is the correct semantics rather than a limitation to document.

The counter feeds a de-escalation ladder whose purpose is to detect "the author pushed changes and I am re-checking evolving code." A review against an unchanged head SHA is not another iteration of that loop — the code is byte-identical, so there is nothing new to de-escalate about.

Counting same-SHA events would also be exploitable in the direction that matters. ready_for_review and reopened do not move the head SHA, so toggling draft five times would walk the reviewer to Cycle 5+: Blocking issues only. Do not leave any nits. without a line of code changing. The split case errs toward reviewing more; this one would err toward silence. I would rather keep the counter tied to distinct reviewed code than to distinct review invocations.

# Coupled to the reviewer's login: if that ever changes the count silently drops
# to 0 and every round looks like the first, hence the warning below.
CYCLE_JQ='[.[][] | select(.user.login == "claude[bot]") | .commit_id] | unique | length'
# Only consulted when CYCLE is 0; see the warning below. Kept in its own variable
# so tests/review-cycle-test.sh can assert it against the fixtures.
DRIFT_JQ='any(.[][]; .user.type == "Bot")'
# Never fail the review over the cycle number; degrade to 1, but say so. gh
# writes its error body to stdout, so an unguarded pipe into jq aborts the step
# under `bash -e` and skips the failure-notification step below.
if ! REVIEWS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews" --paginate); then
echo "::warning::Could not read prior reviews; treating this as review cycle 1."
REVIEWS=''
fi
CYCLE=$(printf '%s' "$REVIEWS" | jq -s "$CYCLE_JQ" 2>/dev/null) || CYCLE=''
if [ -z "$CYCLE" ]; then
echo "::warning::Could not parse prior reviews; treating this as review cycle 1."
CYCLE=0

@claude claude Bot Jul 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the degrade-to-1 fallbacks put the reviewer in a state the prompt explicitly says cannot happen. docs/claude-pr-review-prompt.md:7 reads "If this is cycle 1, there will be no prior comments — skip straight to reviewing the code", and line 19 says "Skip this section entirely on cycle 1". But the two fetches are independent: when either guard here fires, review_cycle is 1 while steps.context.outputs.threads is fully populated from the comments call three lines down. The reviewer is handed prior feedback and told in the same prompt to ignore it — so nits the author declined get re-raised and prior blocking items get dropped, which is the failure the cycle ladder exists to prevent.

The annotation covers the human reading the run log afterward; nothing tells the model. Keeping the two signals independent would close it — e.g. emit a sentinel (review_cycle=unknown) on the fallback paths, and have the prompt key "skip prior-feedback handling" off whether the threads block is empty rather than off cycle == 1.

This round is a live instance of that combination, from a different cause: per README.md:22 the workflow is served from main by the org ruleset, so this run evaluated main's old expression and got cycle 1 — with six prior threads in the prompt. (not blocking)

@zfarrell zfarrell Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining for this PR, but keeping the structural point.

The supporting claim is wrong: this repo's pull_request runs use the workflow from the PR head branch, not main. Proof — .github/workflows/tests.yml does not exist on main yet it ran on this PR (run 30485721742), and the review run reports referenced_workflows: [], so it isn't a pinned-ref required-workflow invocation. Replaying both expressions against this PR's own review history: rounds 1-4 were told cycle 1, 2, 3, 4 under the new expression versus 1, 1, 1, 1 under the old one. No warning annotations fired on any round, so the happy path ran. Round 4 was told cycle 4, with the threads block populated and consistent.

The real half — review_cycle=1 alongside populated threads on the degrade path — stands, and it is the same contradiction that caused the bug this PR fixes. Two reasons it isn't fixed here. It only fires when the reviews API call fails, and the prior behaviour in that case was the step aborting under bash -e, which skipped the review step and left the PR with no review and no notification; degrading to 1 is strictly better than that. And the fix you describe is a change to docs/claude-pr-review-prompt.md:7 — keying "skip prior-feedback handling" off an empty threads block instead of off cycle == 1. That file needs a separate pass anyway (severity classification is miscalibrated: all 26 inline comments on hotdata-dev/monopoly#1560 were labelled nit:/super nit: while describing data-loss-grade bugs, which is what actually drove the 9 rounds). Splitting prompt line 7 across two PRs would fragment that work.

elif [ "$CYCLE" -eq 0 ] && printf '%s' "$REVIEWS" \
| jq -e -s "$DRIFT_JQ" >/dev/null 2>&1; then
# claude[bot] is the only bot that submits reviews across the org (598 of 598
# sampled), so bot reviews that the login filter did not count mean the
# reviewer's identity moved and the counter has silently pinned at 1.
echo "::warning::Bot reviews exist but none matched the reviewer login; the review cycle counter is stale."
fi
echo "review_cycle=$((CYCLE + 1))" >> $GITHUB_OUTPUT

THREADS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \
--paginate | jq -s -r '
add | sort_by(.created_at) |
# Same guard as the counter above: unguarded `gh api | jq` aborts the step, and a
# failure here *skips* the review step, so the notify step's failure check never
# fires and the PR gets no review and no explanation.
if ! COMMENTS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" --paginate); then
echo "::warning::Could not read prior review comments; reviewing without them."
COMMENTS=''
fi
THREADS=$(printf '%s' "$COMMENTS" | jq -s -r '
(add // []) | sort_by(.created_at) |
if length == 0 then "No prior review comments."
else .[] |
"---",
Expand All @@ -82,7 +115,7 @@ jobs:
"",
.body
end
')
') || THREADS='No prior review comments.'

DELIMITER="REVIEW_CONTEXT_$(openssl rand -hex 16)"
{
Expand Down
20 changes: 20 additions & 0 deletions .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
name: Tests

on:
pull_request:
push:
branches: [main]

permissions:
contents: read

jobs:
test:
runs-on: ubuntu-latest
steps:
Comment on lines +12 to +14

@claude claude Bot Jul 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: no permissions: block, so this job inherits the repo/org default token scopes. claude-pr-review.yml declares its permissions explicitly; the test job only needs to read the checkout. (not blocking)

Suggested change
test:
runs-on: ubuntu-latest
steps:
test:
runs-on: ubuntu-latest
permissions:
contents: read
steps:

- uses: actions/checkout@v6.0.2
with:
fetch-depth: 1

- name: Review cycle counter
run: tests/review-cycle-test.sh
Loading