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

[zfarrell]

The REVIEW CYCLE value passed to the reviewer read 1 on every round of an approve-with-nits PR, so the prompt's cycle ladder and its Handling Prior Feedback (cycle 2+ only) section — explicitly skipped on cycle 1 — never engaged there. The counter filtered reviews to CHANGES_REQUESTED/APPROVED, but the org ruleset sets dismiss_stale_reviews_on_push, which flips a prior APPROVED to DISMISSED and leaves the filter matching nothing. CHANGES_REQUESTED survives a push, so request-changes PRs counted correctly and the bug stayed hidden since the feature landed in 42df4d8. Counting distinct commits already reviewed is state-independent, and filtering on claude[bot] stops the other allowed_bots approvals from inflating the count.

On hotdata-dev/monopoly#1560 — nine rounds, 26 inline comments, approved every round — the old expression yields cycle 2 and the new one yields 10. That payload is checked in as a fixture; the test extracts the jq program from the workflow rather than copying it, and fails against the old expression.

Also guards the step: gh writes its error body to stdout, so an unguarded pipe into jq aborted the whole review under bash -e and skipped the failure-notification step. The counter now degrades to 1 with a warning annotation instead, and warns separately if bot reviews exist that the reviewer-login filter didn't match — the shape that would silently reintroduce this bug.

Two known limits, asserted in fixtures rather than fixed: a push landing mid-round splits it across two SHAs and counts twice, and base-branch merge commits count as rounds. Both are bounded at ±1 against a 5-step ladder.

[claude]

nit: the hardcoded claude[bot] login couples this counter to the identity claude-code-action happens to post under, and the failure mode is silent — if that identity ever changes (different app/token, app rename, or a repo where the action falls back to github-actions[bot]), the select matches nothing and CYCLE pins at 1 forever, which is exactly the bug this PR fixes. The fixtures can't catch it either, since they hardcode the same string.

Worth a line in the comment above noting the coupling, e.g. "login must match the identity claude-code-action posts reviews under (Claude GitHub App)", so the next person who touches the token config knows to update it. (not blocking)

[claude]

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:

[claude]

Approved. Verified the extracted jq program against all three fixtures (cycles 1/10/3) and confirmed the old state-based expression yields 2 on the nine-round fixture, so the regression test bites. Two non-blocking notes inline.

[claude]

nit: the guard above stops the cycle counter from aborting the step, but this pipe is the same unguarded gh api | jq shape and it runs three lines later. On a 404/rate-limit/secondary-limit, gh exits non-zero and writes {"message":...} to stdout; the pipeline's status is jq's, and add | sort_by(.created_at) on an object exits 5, so the step still fails under bash -e.

That's the failure mode the comment at line 76 describes, and it's worse here than for CYCLE: when this step fails, the claude-code-action step is skipped, so steps.review.outcome == 'skipped' and the Notify on review failure condition (failure || cancelled) doesn't fire either. Net result is no review and no notification comment — the PR just looks unreviewed.

Same treatment as REVIEWS would close it:

if ! COMMENTS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" --paginate); then
  echo "::warning::Could not read prior review comments."
  COMMENTS=''
fi
THREADS=$(printf '%s' "$COMMENTS" | jq -s -r '...') || THREADS='No prior review comments.'

Pre-existing code, so not introduced here — but the diff makes the asymmetry stand out. (not blocking)

[claude]

nit: the "stale counter" warning misfires in both directions, which undercuts it as the runtime backstop for the login coupling.

False positive: CYCLE -eq 0 plus any bot review is also the shape of a legitimate cycle 1 on a PR another bot reviewed first — and allowed_bots names two (hotdata-automation[bot], aikido-autofix[bot]), so their reviews satisfy .user.type == "Bot" while claude[bot] has genuinely never reviewed. reviews-foreign-reviewer.json is exactly that fixture. Filtering the any() to logins that aren't the known third-party bots would separate the two cases.

False negative: if jq itself fails on line 83 (gh exits 0 but the body isn't an array of arrays), 2>/dev/null swallows it, CYCLE=0, and then the any() on line 85 fails on the same input — so >/dev/null 2>&1 eats that too and no annotation is emitted at all. The step degrades to cycle 1 silently, which is the outcome the comment on line 76 says it wants to avoid. Splitting the jq failure from the zero-count case would cover it:

if ! CYCLE=$(printf '%s' "$REVIEWS" | jq -s "$CYCLE_JQ"); then
  echo "::warning::Could not parse prior reviews; treating this as review cycle 1."
  CYCLE=0
fi

(not blocking)

[claude]

nit: the drift warning is suppressed for the exact scenario reviews-foreign-reviewer.json encodes, so the runtime backstop the test comment promises doesn't exist for that shape.

That fixture's reviews are all hotdata-automation[bot], which is in $known here. Verified against the fixture:

$ jq -s '[...] as $known | any(.[][]; .user.type == "Bot" and (.user.login | IN($known[]) | not))' \
    tests/fixtures/reviews-foreign-reviewer.json
false

So CYCLE=0 and no annotation — tests/review-cycle-test.sh:56-59 ("the workflow also emits a warning annotation when it sees this shape at runtime") doesn't hold for the login it actually asserts on.

It's also the least lucky login to pick: hotdata-automation[bot] is the app this workflow mints a token for (lines 32-36), so wiring that token into claude-code-action as github_token — a one-line change someone could plausibly make — is the drift path that lands on a suppressed login and reproduces the original bug with no warning at all.

Two ways out, either fine: use a login for the fixture that isn't in $known (github-actions[bot] is the realistic fallback identity), or drop hotdata-automation[bot] from $known and accept the false positive on PRs it reviews first. (not blocking)

[claude]

nit: half of thread #3677516935 is still open — the 2>/dev/null here swallows a jq failure, so CYCLE=0 with no annotation, and then the any() below fails on the same malformed input and >/dev/null 2>&1 eats that too. Net: cycle silently reads 1, which is the outcome the comment on line 76 says it wants to avoid. The false-positive half is fixed; this is the remaining false negative.

Suggested change

	CYCLE=$(printf '%s' "$REVIEWS" | jq -s "$CYCLE_JQ" 2>/dev/null) || CYCLE=0
	if ! CYCLE=$(printf '%s' "$REVIEWS" | jq -s "$CYCLE_JQ"); then
	echo "::warning::Could not parse prior reviews; treating this as review cycle 1."
	CYCLE=0
	fi

Dropping 2>/dev/null also puts jq's parse error in the log, which is what you'd want when diagnosing it. (not blocking)

[claude]

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]

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.

[claude]

nit: "the workflow also emits a warning annotation when it sees this shape at runtime" is asserted nowhere in this file. The harness extracts and evaluates CYCLE_JQ only, so the entire guard block — claude-pr-review.yml:78-93: the gh-failure fallback, the unparsable-reviews fallback, and the drift predicate itself — ships untested. Those are precisely the parts added to stop a silent degradation, and so the parts a future edit can break without CI noticing.

The drift predicate is extractable the same way CYCLE_JQ is. Hoisting it to its own single-quoted variable in the workflow and asserting it here would cover the claim:

DRIFT_JQ=$(sed -n "s/^ *DRIFT_JQ='\(.*\)'\$/\1/p" "$WORKFLOW")
# Fires only on count==0 with bot reviews present: the login moved.
jq -e -s "$DRIFT_JQ" tests/fixtures/reviews-foreign-reviewer.json >/dev/null   # expect exit 0
jq -e -s "$DRIFT_JQ" tests/fixtures/reviews-first-review.json  >/dev/null      # expect exit 1

I confirmed both hold against the current predicate (true on the foreign-reviewer fixture, false on the empty one), so this is pinning behaviour that already works rather than asking for a change. (not blocking)

[zfarrell]

Taken in a0fc644. DRIFT_JQ is hoisted to its own variable and tests/review-cycle-test.sh extracts it the same way as CYCLE_JQ, asserting it fires on reviews-foreign-reviewer.json and stays silent on reviews-first-review.json. The extraction is now a shared extract_jq helper so a missing assignment fails the build for either program.

Mutation-checked both ways: replacing the predicate with true fails the silent-on-genuine-cycle-1 assertion, and deleting the assignment fails extraction. Also corrected the stale comment above the fixture, which claimed a runtime warning the suite did not cover.

[claude]

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]

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.

[claude]

Verified the extracted jq programs against every fixture by hand: cycle counts 10 / 3 / 3 / 7 / 1 and the drift predicate fires on the foreign-reviewer fixture, silent on the empty one — all match the assertions. Empty-input paths (gh failure → REVIEWS='') yield cycle 0 with no spurious drift warning, and the comments pipe degrades to "No prior review comments." Prior rounds' items are addressed.