feat: run review phases in parallel or in sequence behind the phases input

[aliasunder]

Why

Every review is one model call carrying all four dimensions, and that call is the whole wall clock of a review: 8m00s of an 8m28s job on this repo's own self-review, 3m29s of a ~4m job on a consumer's docs PR. Everything else in the pipeline takes about two seconds. Splitting the call was the candidate latency lever, and one 900s timeout on the single call loses the whole review. The live A/B below shows what splitting actually buys on the model in use.

What

• phases input gains parallel and sequential. The dimensions split into three focused phases: correctness + security (+ CI workflow checks), code quality + test quality, subtle bug patterns. parallel runs them at once; sequential runs them in order, each phase seeing the earlier findings through the existing prior_findings prompt section. combined stays the default and is unchanged. An empty value means the default, so a workflow can wire an unset repo variable.
• Stage dispatcher (review/run-stages.ts, pure): a stage's phases run concurrently, stages run in order. One phase failing does not discard its siblings. When no phase completes the dispatcher throws an error carrying every phase's outcome, so the failure check-run summary can still render the billed attempts. An auth/credit abort stops later stages instead of making doomed requests, and those phases are reported as not attempted.
• Cross-phase merge (review/merge-phase-findings.ts, pure): findings two phases reported on overlapping lines of one file collapse to the higher severity (earlier phase wins ties), category-agnostic, because two phases on the same lines are almost always one defect under two labels. A candidate overlapping several kept findings must outrank all of them and then evicts all of them. Only cross-phase pairs compare, so a one-phase run passes through unchanged. Runs after the non-finding and unknown-file filters so a non-finding can never out-rank a real finding. Non-findings are also dropped before findings are threaded to a later stage.
• Partial completion surfaces on the PR. The status comment names the phase(s) that did not complete; the check title carries (1 of 3 phases incomplete) and the summary carries the reason; the job summary lists completed and incomplete phases and a Duplicates (cross-phase) row. The conclusion stays success: the review ran and posted what it found.
• Cost table gains a phase column. Attempts from every phase are listed, including a failed phase's billed attempts, which ride on a ReviewRequestError thrown by the client; a run where every phase fails renders them into the failure summary. model_used joins the routed models when phases were served by different ones.
• Prompt: the it()-enumeration sentence moved from the shared proof-of-work section into the test dimension, so phases without that dimension do not enumerate tests they are not reviewing. Each split phase opens with a pass-scope line whose boundary keeps a bug from being dropped because "another pass owns it". The prior_findings note states the merge rule so a later phase is not told to report what the merge would collapse.
• Docs: action.yml, README (input, output, how it works, status), AGENTS.md tree line; self_review.yml wires UMM_PHASES so this repo can A/B the modes.

Tests

• 587 tests (+45): stage resolver shapes for all three modes, pass-scope line, merge rules (cross-category overlap, ties, multi-overlap eviction, same-phase pass-through, non-overlapping and different-file survivors), dispatcher (concurrency, prior-findings threading with non-findings removed, partial and total failure, abort short-circuit, log lines), pipeline end to end for parallel, sequential, partial failure, all-failed with and without billed attempts, sequential abort, and mixed routed models, cost table phase column, status comment and job summary wording, empty phases config.
• Mutation checks: removing the phase guard from the merge fails only the same-phase test; removing the prior-findings threading fails only the sequential test.
• npm test, npm run lint, npm run build green.

Live A/B

Same PR, DeepSeek v4 flash via OpenRouter, 80K context budget. Per-phase order is correctness-security / conventions-tests / subtle-bugs. Runs cancelled by a later push still logged their phases.

Mode	Wall clock (model calls)	Per phase	Raw findings	Completion tokens	Cost
combined	43s	43s	1 (a non-finding)	1,657	$0.0060
combined	8m48s	8m48s	2	14,136	$0.0041
parallel	9m33s	2m02 / 9m33 / 6m16	5	(cancelled before the summary)
parallel	9m08s	9m08 / 1m51 / 1m37	1	(cancelled)
parallel	over 13m (cancelled)	8m26 / 4m31 / unfinished	2+	(cancelled)
parallel	15m55s	7m49 / 12m00 / 15m55 (one schema-mismatch retry)	6	15,181 / 22,136 / 43,894 + 14,232	$0.0617
sequential	18m31s	3m49 + 6m06 + 8m36	2	8,263 / 20,785 / 25,549	$0.0297

What the numbers say:

• Output generation is where the time goes. A normal combined call and a single focused phase are the same size of work on this model: 8,000 to 26,000 completion tokens, roughly 4 to 12 minutes each. The 43-second combined run that stopped after one non-finding was the outlier.
• A focused phase therefore does not finish sooner than the combined call; parallel wall clock is the slowest of three independent draws (every sample 9 to 16 minutes), and sequential is their sum.
• The split modes read the PR three times over and found more: seven of the eight findings posted on this PR came from parallel or sequential runs, and five of those were valid and fixed.
• Cost per review in the split modes was seven to fifteen times the combined run.

Default stays combined. On this model parallel is a depth setting, not a speed one; the speed premise holds only for a provider whose per-call latency is dominated by prefill or queueing rather than by output tokens. Worth re-measuring when the primary model changes.

🤖 Generated with Claude Code

[umm-actually]

umm-actually re-reviewed at 003e998

2 new finding(s) posted (15 tracked finding(s) across all runs).

Context notes

• Priority docs already in context: README.md

---

umm-actually · deepseek/deepseek-v4-flash-0731

[aliasunder]

@CodeRabbit review

[coderabbitai]

⚠️ Action not completed

Head commit changed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/staged-phases

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[umm-actually]

New prior-findings note mis-states the merge and teaches later phases to suppress distinct same-line defects
Low severity · correctness · medium confidence

src/review/prompt.ts:2183 — beyond the diff's line ranges, in code the changes touch or depend on.

The new sentence in the prior-findings note ('a later report on the same lines is merged into the earlier one') is factually wrong — overflow: an overlapping lower-or-equal-severity later finding is dropped by a later report on merger rule (or the equal-severity entry kept in merge-phase-findings), it is not merged — and its practical effect is to instruct a later phase that reporting on territory an earlier phase already touched is futile. The stated principle elsewhere in this PR's own prompt text is that a genuinely different defect at the same location is still a finding. A model reading this new clause will self-suppress precisely that distinct finding before the merge ever runs, so the run silently loses a real defect at model time rather than at merge time.

Failure scenario: phases: sequential where correctness-security reports a high-severity null-deref at src/foo.ts:42. subtle-bugs traces the same diff and — per the pass-scope principle — identifies a genuinely different missing-bounds-check defect also at line 42. Reading 'a later report on the same lines is merged into the earlier one', subtle-bugs judges its report futile and omits it. The run posts only the first finding; the second real defect is never reported. Replacing the misleading claim with accurate merge semantics (higher severity wins, distinct defects still reported) avoids this. The fix for the phrasing: say reports on the same lines are collapsed keeping the higher-severity one, and a genuinely distinct defect at the same location is still a finding — report it.

Suggested fix

Reword to: 'A later report on the same lines is collapsed with the earlier one, keeping the higher-severity finding; a genuinely distinct defect at the same location is still a finding — report it.' Remove the false 'merged into the earlier one' claim, which teaches suppression. This also keeps the prompt consistent with the pass-scope principle the PR adds for sequential mode.

---

umm-actually · deepseek/deepseek-v4-flash-0731

[aliasunder]

Regarding the beyond-diff finding on the prior-findings note (the "merged into the earlier one" wording): the precision half is applied in 170b16f. The note now says "when a later report overlaps one of these lines, only the higher-severity finding of the two is kept", which is what the merge does. The other half, promising that a distinct defect at the same location is still reported, is declined on purpose: the cross-phase merge is category-agnostic by design, so that promise would contradict the code (the previous finding on this PR flagged exactly that contradiction), and a later phase told to report on collapsed territory would spend output tokens on findings that cannot post. The collapse is counted in the job summary as Duplicates (cross-phase).

---

🔍 ship-check · pr-monitor · claude-fable-5-1

[aliasunder]

@umm review

The self-review on 170b16f failed on two consecutive 900-second provider timeouts (both attempts, no response from DeepSeek), which is a provider failure rather than anything in the PR; the commit under review changed one line of prompt text. Re-running once. This run also exercised the new every-phase-failed path live: the check run concluded failure with both billed timeout attempts in its cost table.

---

🔍 ship-check · pr-monitor · claude-fable-5-1

[aliasunder]

The current wording at prompt.ts:218 is "when a later report overlaps one of these lines, only the higher-severity finding of the two is kept" — this accurately describes the behavior (lower-severity overlapping finding is dropped via the pre-sorted reduce in selectFindings). The quoted wording "a later report on the same lines is merged into the earlier one" does not appear in the current code.

---

🔍 ship-check · pr-monitor · claude-opus-4-6

[umm-actually]

umm-actually — review skipped

diff too large for context budget (40090 tokens, limit 40000 of 80000)

---

umm-actually