fix(review): make re-reviews converge instead of accumulating nits

[zfarrell]

The Problem

Hotdata's PR reviews became slow and too detailed. The cause was a model change. The workflow did not set a model. Therefore the action moved us from Opus 4.8 to Opus 5 on 24 July. No commit shows this change.

The data shows the effect:

• Cost for each review went from $0.72 to $1.86.
• Rounds for each PR went from 1.71 to 2.49.
• Cost for each PR became about 4 times more.

Quality Analysis

I tried to show that review quality became worse. The data does not show this. Quality for each comment went up.

Three measurements show this:

Authors did not disagree more. I counted human replies that push back on a comment. These replies use words like "intentional", "false positive", or "disagree". This rate went down from 6% to 4%. Therefore the findings were correct.

Small problems did not stop merges. I found 39 rounds where all findings were non-blocking. The model approved all 39 rounds. It blocked none of them. The Decision Framework works correctly.

The late comments were good. I read the comments on monopoly#1560 from cycle 7 to cycle 13. They report real defects. Examples: a greedy match with no backtracking; a dry run that writes to the database; two database updates with no transaction.

Two more counts show where the value becomes low:

• At cycle 6 and later, the model made 31 comments. All 31 were small problems. None were blocking.
• At cycle 3 and later, 42% of comments were about old code comments and docstrings. At cycles 1 and 2, this figure was 22%.

Each comment is good. But each new round gives less value than the round before. This is the problem to correct. The model is not the problem.

Two Failures

The taper did not work. The prompt told the model to report only blocking problems after cycle 5. The model did not obey. At cycle 6 and later, it made 31 comments. None of them were blocking.

The loop fed itself. The author corrected a small problem. This made a docstring old. The next review then reported the old docstring. At cycle 3 and later, 42% of comments were of this type.

The Changes

There are three changes:

1. The workflow now sets the model. This stops more silent changes.
2. The workflow removes prompt sections. It does this before it sends the prompt. At cycle 3, the model gets no instructions about small problems. Therefore it cannot report them.
3. Late reviews look only at new changes. Before, each review read all of the diff again.

Cycle	Scope	Reports	Model
1	full diff	blocking + nits + docs	claude-opus-5[1m]
2	changes since last review	blocking + nits	claude-opus-5[1m]
3+	changes since last review	blocking only	claude-sonnet-5

Results

Both test files pass. There are 59 tests. The shellcheck tool finds no problems.

The tests found an error in the first version of the prompt. That version told the model not to use the words nit: and super nit:. But this teaches the words. They are now removed.

[claude]

LAST_SHA_JQ accepts any claude[bot] review as the incremental base, including ones from a round that never finished. That makes it possible for pushed code to never be reviewed by any cycle.

Concretely, with cancel-in-progress: true (line 9) and continue-on-error: true (line 176):

1. Cycle 2 starts at head B, posts 3 inline comments (each a COMMENTED review with commit_id == B), and is cancelled by the next push before it reaches gh pr review --request-changes.
2. Author's push creates C. The new run computes LAST_SHA = B (newest claude review by submitted_at) and builds the incremental diff as B...C.
3. The hunks the cancelled round never got to at B are now behind the base. Cycle 3+ only sees B...C, and the cycle-2+ prompt tells the model "Do not re-review code you already passed on, and do not mine unchanged hunks." Those changes are never reviewed at any cycle.

A job timeout (timeout-minutes: 15) produces the same partial state. This isn't theoretical for this repo — reviews-straddled-round.json is a captured real case of a round's reviews spanning two SHAs.

A completed round always ends with gh pr review --approve|--request-changes, so a terminal state is a usable "this round finished" signal — COMMENTED reviews are never dismissed, whereas both APPROVED and CHANGES_REQUESTED become DISMISSED on push. Something like restricting the base to reviews whose state != "COMMENTED" (and falling back to the full diff when none exists) would close the hole without reintroducing the state-filter bug that CYCLE_JQ deliberately avoids — the counter needs every round, but the base needs the last finished one.

Whichever way you cut it, please add a fixture for the partial-round case so the base can't silently regress.

[zfarrell]

Confirmed and fixed in 55ba278. Reproduced both variants as fixtures before changing anything.

Your terminal-state suggestion is right about cancelled rounds but does not cover the straddled case, which has the same consequence: when a push lands mid-review the inline comments sit on B while the terminal approve lands on C, so filtering to finished reviews picks C — a tree Claude never read. tests/fixtures/reviews-straddled-latest-round.json captures it.

Both are fixed by taking the round's first SHA rather than its last. A terminal state closes a round, so the window from the previous terminal state to this one is one round, and its oldest review is the tree that round actually started from. Cancelled rounds have no terminal state, so they never open a window and the base stays at the last finished round.

Fixtures added: reviews-cancelled-round.json (base stays at aaa, not the killed round's bbb), reviews-straddled-latest-round.json (base is bbb, the tree read, not ccc), reviews-only-partial-round.json (no finished round yet, so no base and the review reads the full diff). All six pre-existing base expectations are unchanged.

[claude]

nit: the render-failure fallback ships a prompt that contradicts itself (not blocking).

cat-ing the unrendered file sends the model every cycle block at once, which means both ## Decision Framework sections — the cycle≤2 one ("Only nits/super nits → approve") and the cycle≥3 one ("No blocking issues → approve. No inline comments.") — plus prompt line 12 asserting "This prompt has already been narrowed to the current review cycle by the workflow", which is false on this path. tests/render-prompt-test.sh guards "exactly one Decision Framework" precisely because two leaves the model choosing between contradictory approve rules; the fallback is the one path that violates the invariant.

Reachable cases are narrow (script missing from the sparse checkout, malformed markers in a prompt edit) but they're exactly the cases where a warning gets buried in the log. Failing the step instead would route to the existing Notify on review failure step, which is a clearer signal than a review conducted under conflicting instructions.

[zfarrell]

Agreed, fixed. The context step now exits 1 rather than falling back to the unrendered prompt.

One thing your suggestion depends on that was not true yet: a failed context step skips the review step, so steps.review.outcome is skipped and the existing notify condition would not have matched — failing closed would have produced silence. The notify step now carries always() and also checks steps.context.outcome == 'failure'. Verified both trigger paths (missing script, malformed marker) exit 1 with an ::error::.

[claude]

nit: PATCH_JQ is the one jq program here without test coverage (not blocking).

CYCLE_JQ, DRIFT_JQ and now LAST_SHA_JQ are all extracted from this file by extract_jq and asserted against fixtures — deliberately, per the comment on line 70. PATCH_JQ breaks that pattern, so the .files[]? guard and the .patch // "[no textual patch available]" fallback for binary/omitted patches are untested. It's the piece that decides what the model actually reviews on cycle 2+; a captured compare-API response as a fixture would put it on the same footing as the rest.

[zfarrell]

Fixed. PATCH_JQ now has fixture coverage on the same footing as the rest, via tests/fixtures/compare-incremental.json (a modified file with a patch, a binary file the API returns with no patch, and a removed file).

Note for anyone extending extract_jq: it could not be reused here. PATCH_JQ contains a literal \\n, and BSD sed expands that when writing a backreference, so the extracted program would carry a real newline instead of the two characters the workflow ships. It is pulled with awk instead, and there is an assertion on the output line count that fails if that escape ever degrades.

[claude]

super nit: the compare API caps files at 300 per page and this call has no --paginate (not blocking).

A push touching more than 300 files yields a non-empty, under-60000-char PATCH that passes both guards, so no warning fires and the model is told the block "holds everything pushed since your last review" when it holds a prefix. The 60000-char cap makes this unlikely to bite in practice, but the truncation is silent in a way the force-push and oversize paths aren't. Comparing .files | length against the response's changed_files would let you fall back with a warning instead.

[zfarrell]

Fixed, and I rated this higher than you did — an independent review pass called it high severity for the same reason: it is the only fallback that was silent, and the cycle 2+ prompt calls this block the entire review surface.

The compare response carries no total to check .files | length against (changed_files is on the PR object, not the comparison), so the guard trips on >= 300 — the cap itself — and falls back with a warning. TRUNCATED_JQ is extracted and asserted against a 300-file fixture. Every fallback on this path now emits a distinct warning; verified all three (404, file cap, oversize).

[claude]

Review

Blocking Issues

.github/workflows/claude-pr-review.yml:74 — LAST_SHA_JQ takes the newest reviewer-bot review by submitted_at regardless of whether that round completed.

With cancel-in-progress: true (line 9), continue-on-error: true (line 176), and a 15-minute job timeout, a round can die after posting some COMMENTED reviews at head B but before its terminal gh pr review call. The next run then uses B as the incremental base, so the hunks that round never reached fall behind the base — and the cycle-2+ prompt (docs/claude-pr-review-prompt.md:24) instructs the model not to look at them. Result: pushed code that no cycle ever reviews. The reviews-straddled-round.json fixture is captured evidence that rounds do span two SHAs in this org.

Action Required

Derive the incremental base only from a round that reached a decision, and fall back to the full diff when there is none. Filtering on a terminal review state works because APPROVED and CHANGES_REQUESTED persist as themselves or as DISMISSED, while COMMENTED is never dismissed. That does not reintroduce the state-filter bug CYCLE_JQ deliberately avoids: the counter needs every round, the base needs the last finished one. Add a fixture covering a partial round so this cannot regress silently.

Three non-blocking comments are inline. The rest of the change — mechanical marker stripping instead of advisory tapering, the pinned models, and a test suite that asserts the absence of the nit vocabulary rather than trusting a rule — is sound.

[claude]

Blocking issue on the incremental-diff base; see the summary comment and inline comments.

[claude]

nit: the "oldest review in the round" invariant only holds for rounds with ≥2 reviews (not blocking).

The comment above states it unconditionally — "its oldest review is the tree that round actually started from" — but when a round's only review is its terminal one, the oldest review is the terminal review, so .[0].commit_id is the head-at-submit-time again and the straddle hole reopens:

COMMENTED  @aaa
DISMISSED  @aaa     <- round 1 terminal
APPROVED   @ccc     <- round 2: read bbb, approved after a push to ccc

$done = [1,2], start = $done[-2]+1 = 2, slice = [APPROVED@ccc], base = ccc. bbb...ccc is reviewed by nobody.

Both halves of that conjunction are already in your own captured data. reviews-straddled-round.json has the straddle (comments on a89f19e8, approve 5s later on 951c8afa), and the same fixture has a single-terminal-review round at index 6 (DISMISSED@951c8afa at 19:07:06, a separate id from the APPROVED@951c8afa at 19:00:35 — a round that approved with no inline comments). So the shape needing both is rarer than what you fixed, not hypothetical — a clean approve is the common outcome per the Decision Framework, and the straddle window is the same seconds-wide one.

The fallback direction you chose makes this cheap to close: treat a last-finished round of length 1 as untrustworthy and step back to the previous round's window (or return "" and take the full diff). Either way it'd be worth a fixture, and worth softening the comment so the invariant reads as conditional on the round having a non-terminal review.

[claude]

nit: always() makes the cancelled branch live, so every push that lands mid-review now comments (not blocking).

Before this PR the condition had no status check function, so success() was applied automatically and the step was skipped whenever the job was cancelled — which made steps.review.outcome == 'cancelled' unreachable. always() returns true on cancellation too, so that branch now fires.

With cancel-in-progress: true (line 9) that is the common case, not an edge one: a push while the model is reviewing cancels the run, steps.review.outcome becomes cancelled, and the PR gets "Automated review unavailable (Claude step failed). Please review manually." even though the superseding run is already computing a real review. This repo's own data says pushes land mid-review often enough to have shaped LAST_SHA_JQ.

Dropping the branch gets the new behaviour you want without the noise:

Suggested change

	&& (steps.review.outcome == 'failure' || steps.review.outcome == 'cancelled'
	&& (steps.review.outcome == 'failure' || steps.context.outcome == 'failure')

That keeps the context-failure path this PR added, keeps genuine review failures, and returns cancellation to being silent — same as before, so nothing regresses. (!cancelled() would also work but suppresses the timeout-minutes: 15 case, which also surfaces as cancellation and is worth notifying about; dropping the branch and leaving always() treats the timeout the same way the old code did.)

[claude]

nit: still open from the last round — always() makes the cancelled branch live (not blocking).

Before this PR the condition had no status function, so success() was implied and the step was skipped whenever the job was cancelled, which made steps.review.outcome == 'cancelled' dead code. always() returns true on cancellation, so that branch now fires: with cancel-in-progress: true (line 9), every push that lands while the model is reviewing cancels the run and posts "Automated review unavailable (Claude step failed). Please review manually." even though the superseding run is already computing a real review. reviews-straddled-round.json and reviews-cancelled-round.json are this repo's own evidence that mid-review pushes are routine.

Dropping the branch keeps everything this PR added and returns cancellation to being silent, as it was before:

Suggested change

	&& (steps.review.outcome == 'failure' || steps.review.outcome == 'cancelled'
	&& (steps.review.outcome == 'failure' || steps.context.outcome == 'failure')

(timeout-minutes: 15 also surfaces as cancellation, so this does lose the timeout notification — but that was already the pre-PR behaviour, so nothing regresses.)

[claude]

nit: still open from the last round — the "oldest review in the round" invariant only holds when the round has a non-terminal review (not blocking).

Verified against your own fixture. Truncating reviews-straddled-round.json to the first seven reviews (round 1 = 5 × COMMENTED@a89f19e8 + APPROVED@951c8afa, round 2 = the single DISMISSED@951c8afa, a distinct review id) gives:

done   = [5, 6]
window = ["DISMISSED@951c8afa"]
base   = 951c8afa

When a round's only review is its terminal one, .[0] is the terminal review, so the base is head-at-submit-time again and the straddle hole this line closes reopens for that shape: a round that read bbb, approved with no inline comments after a push to ccc, and now hands ccc back as the base — so bbb...ccc is reviewed by no cycle. A clean approve with no comments is the common outcome per the cycle 3+ Decision Framework, so single-review rounds are not rare; only the straddle on top of one is.

Your own fallback direction closes it cheaply: treat a last-finished round of length 1 as untrustworthy and step back to the previous round's window, or return "" and take the full diff. Worth a fixture either way, and worth softening lines 83–85 so the invariant reads as conditional rather than unconditional.

[claude]

nit: LAST_SHA == HEAD_SHA falls through to the full-diff message, not to "nothing new" (not blocking).

The trigger list includes ready_for_review and reopened (lines 4–5), neither of which changes the head SHA. So converting a reviewed PR to draft and back, or closing and reopening it, runs at cycle N+1 with LAST_SHA == HEAD_SHA, the if is skipped, and INCREMENTAL keeps its default — "Incremental diff unavailable - review the full diff with gh pr diff." The cycle 2+ prompt honours that literally ("Fall back to gh pr diff only if that block reports the incremental diff is unavailable"), so the model re-reads the entire PR on a tree it already reviewed. That is the full-diff re-review this change exists to stop, arriving through the one path where the incremental diff is genuinely empty rather than unavailable.

The two cases want opposite messages: unavailable → read everything; empty → there is nothing new, approve. Splitting the equal-SHA case out (INCREMENTAL='No changes have been pushed since your last review.') costs one branch and removes the only path that still re-exposes old code at a late cycle.