Status: stable, in daily use
History: see ../CHANGELOG.md
Supersedes: code-review (mattpocock), code-review-and-quality (addyosmani), and the ad-hoc use of /code-review max
Six overlapping code-review skills were installed. Each carried one or two solid ideas
and a lot of redundancy. Worse, the disk skill named code-review was shadowing the built-in
/code-review max (the effort-tiered, ~10-angle binary review), silently downgrading every
review to a 2-agent run.
This command absorbs the best of all six into one adaptive review, and frees the code-review
name so the built-in works again. /deep-review and /code-review max now coexist.
| Idea | Source |
|---|---|
| Confidence × impact progressive gate | review-local-changes |
| Applicability gating (skip angles that don't apply) | review-local-changes |
| False-positive catalogue | review-local-changes |
historical-context reviewer (git blame) |
review-local-changes |
| Adversarial refutation → CONFIRMED / PLAUSIBLE | built-in /code-review |
| Mandatory concrete failure scenario | built-in /code-review |
ReportFindings native UI |
built-in /code-review |
| Spec-conformance axis | code-review (mattpocock) |
| Anti-reranking (never merge axes) | code-review (mattpocock) |
| Fowler smell baseline | code-review (mattpocock) |
| P0–P3 severity semantics | code-review-expert |
| Removal candidates / dead code | code-review-expert |
| "Areas not covered" disclosure | code-review-expert |
| Context isolation (reviewers never get session history) | superpowers:requesting-code-review |
These came out of the design conversation and address failures I'd actually hit.
Vertical-slice completeness. A ticket says "add design reset" and the agent ships routes, controller, and tests, but no UI. The spec axis can't catch this, because the ticket never asked for the UI either. This angle checks whether the change delivers a usable capability, inferring the expected layers from how comparable features were built in this repo.
Collateral / untraceable hunks. Every hunk must trace back to the task. Deletions and value-reverts outside the task's blast radius are flagged loudly even at low computed impact; unrelated additions get a quiet mention. Asymmetric on purpose: a silent deletion is the one that reaches production.
Three-way spec checking. Not code-vs-spec, but:
- Does the code do what the ticket asked? (wrong feature, missing requirement)
- Did the ticket ask for enough to be usable? (routes with no UI)
- Does every hunk trace to the task at all? (the silently reverted field)
Dismissal persistence. A finding you reject stays rejected. Without it, every mid-flight increment re-litigates findings already triaged, attacking the noise problem from the opposite side to the confidence gate.
Sibling amplification. A confirmed finding is one instance of a rule, and the rule usually has other instances. One field is escaped and the other six on the same model are not; a validation rule is mirrored at a second layer whose semantics differ; a claim in a comment exists as four paraphrases in three other files. Every review pipeline I'd used reports the instance it happened to look at and stops there. This stage restates the defect as a rule and goes looking for the rest.
Report-only, never blocking. Mode B emits an advisory verdict. A false positive must never be able to block a good merge.
Mode A's floor is stricter than Mode B's, on purpose. Counter-intuitive at first glance: the final pre-merge gate is the looser one. Mode A runs often, mid-flight, so it has to stay quiet or it gets ignored; Mode B runs once, right before merge, so it can afford to surface more and let the operator triage it in one sitting. Amplification is gated on the same logic — default-on in B, opt-in everywhere else.
Reports live outside the repo. ~/.claude/projects/<project>/reviews/. A committed findings
history travels to GitHub and is a roadmap for anyone with read access. Durability is worth
having; putting it in the codebase is not. Local-only does not defend against a compromised
laptop; it defends against repo read access, which is the actual threat described.
Fable is never spawned. Not as a sub-agent, not at any tier, not under any flag. When a finding warrants a stronger independent read, the review stops and prints a ready-to-paste prompt so you run Fable in a fresh context you control.
Refuter is Opus, with independence by construction. With Fable excluded, finder and refuter share a model, so their errors correlate. Mitigated structurally: the refuter receives only the claim, never the finder's reasoning, runs in a fresh context, and is instructed to default to refuted when uncertain. Weaker than model diversity. Documented rather than hidden.
Fixed model table, no runtime overrides. One table at the top of the skill. Retuning happens in one place when models change.
Deletions, missing slice layers, and siblings bypass the impact floor. All three are findings
whose true cost is invisible in the diff. The bypass is against a mode's default floor only —
an explicit --min-impact binds on every category, exemptions included. A threshold flag that
silently fails to apply to three categories is worse than no flag, and the operator asking for
blockers only is a different statement from a mode picking a sensible default.
Amplification is finding-triggered, not diff-triggered. The alternative was expanding the review surface before fan-out — from the diff to every unchanged file that depends on it — and feeding that to all fourteen angles. That is a multiplier on diff size. Amplifying confirmed findings instead is an addend on a number the confidence gate has already made small. It also targets the case a careful human reader cannot cover: nobody re-greps for their own bug in three other columns.
The amplifier never searches the text of the finding it was given. Searching for the phrasing
already in hand matches only what has already been found — the method quietly confirms where it
started, which is the same failure shape as trusting a stale local main. So the stage restates
the defect as an abstract rule first, and searches the rule.
Siblings are verified like anything else. A sibling inherits the parent's rule, not the parent's verdict. Each is independently scored and independently refuted, and its impact is scored fresh — a sibling can easily be worse than its parent, since the parent is merely the instance that happened to be in the diff.
The amplifier is Opus, and must read every candidate before proposing it. Counter to the instinct that search is mechanical work for a cheap model: everything the amplifier proposes buys a Sonnet scorer and an Opus refuter downstream, so over-generation is more expensive than accuracy. A speculative "check these files" list is exactly the shape that over-generates.
Depth 1, capped at 5. The axis walk is already breadth-first over peers, so a second round mostly re-treads the same ground from a different starting point. Depth 1 with a per-finding cap makes the worst case bounded and knowable before you run: confirmed findings × 5.
Out-of-diff siblings are reportable; other pre-existing issues still are not. The carve-out is narrow on purpose — a sibling of a CONFIRMED finding under the same rule, and nothing else. The diff is the evidence that the defect class is live and someone is working in it, which is precisely what the "pre-existing issues in unchanged code" exclusion is otherwise right to filter. Siblings report on their own axis so they can never be re-ranked against, or bury, the other two.
Dismissals are typed, and the rule is stored beside them. "Dismissed" was conflating two
opposite things. Not real invalidates the rule: later runs refuse to amplify a matching rule even
when a different finding raises it. Won't fix means the rule is real and only this instance is
being let go, so the rule stays available. Recording a won't-fix as a not-real would silently hide
the same defect everywhere else it occurs. The distinction only does anything if the rule itself is
persisted, so dismissals.json stores it — a fingerprint is a hash and cannot be matched against a
rule a later run derives. Where no amplifier ran, the rule is null rather than invented; a
fabricated rule marked invalid would suppress legitimate amplification later.
A sibling keeps its defect category and wears a prefix. sibling:concurrency, not sibling.
A bare axis name would have thrown away what kind of defect it is, and every downstream consumer —
the floor exemption, the axis table, ReportFindings, which has no axis field of its own — can key
on one prefix instead of maintaining a parallel list. It also removes the trap where a stage
matching on category finds nothing, because no finding's category was ever literally sibling.
Out-of-diff siblings are reported, never fixed, and never blocking. --fix bounds itself to
the diff — auto-editing files the operator never put up for review is the blast radius that section
exists to prevent — and the same reasoning covers the Mode B verdict and the escalation trigger. A
defect in untouched code is worth knowing about and is not a reason to stamp
NOT RECOMMENDED FOR MERGE or halt the review. Widening the search must not widen what the review
can block.
But a sibling is not out-of-diff by construction, and assuming it was would have been a real
bug. Three of the six axes — peer fields, peer branches, peer callers — routinely land in the
parent's own file, and the other arm of a conditional the diff just added is a sibling like any
other. Had the containment rules keyed on "is a sibling" rather than on location, an
amplifier-found crash introduced by the PR would have been demoted to a non-blocking note purely
because of which agent found it. So the coordinator classifies by location: inside the diff, it is
an ordinary finding with a plain category and full standing; outside it, it takes the sibling:
prefix and the containment rules apply. The boundary is lines, not files: the operator put
specific hunks up for review, not everything else that happens to live in the same file, and the
file-level reading is the expensive error — it would let --fix edit untouched code and let an old
defect elsewhere in a large file block a clean PR.
Rule-level suppression matches strictly and announces itself. A stored not-real rule is an
exclusion, and the skill's own standard for exclusions is that over-matching one is a hole rather
than a false positive. A wrong non-match costs a single amplifier run; a wrong match costs every
sibling of a confirmed defect and reports nothing about having done so. Hence: match only at the
same defect shape and the same level of generality, prefer not matching under doubt, and disclose
every suppression under "Areas not covered". An undisclosed skip is indistinguishable from a search
that came back empty, which is the same failure as a green check that never ran.
--fix in Mode A amplifies without --amplify. A fix is the point where the sibling is most
likely to be missed, and the existing re-review step asks only whether the fix broke something
here. Scoped to the findings it fixed, so a plain /deep-review stays as cheap as it was, and
--no-amplify still wins — a negative flag that binds in only one of four modes is a trap.
Stack-specific content is quarantined, org-specific content is removed. Two different things. Technology-specific detectors (Laravel, Blade, Alpine) are useful as worked examples and are labelled as replaceable. Anything specific to one organisation — internal naming conventions, local environment quirks, private tooling — is deleted outright: a stranger cannot act on it, and a suppressor derived from someone else's machine actively degrades their review.
Hard-won failures from prior work, built into the skill rather than left to be rediscovered:
| Gotcha | How it's encoded |
|---|---|
| A fix for a review finding shipped a new defect | --fix re-reviews the fix diff; a fix that introduces a CONFIRMED finding is reverted |
| A fix landed correctly and the same defect one field over shipped beside it | --fix amplifies the finding it fixed, even in Mode A |
| Parallel review agents share one worktree | agents run read-only against a committed state |
Stale local main corrupts the diff base |
always resolves origin/main, cross-checked against gh pr view |
| Searching for the sentence you already have finds only what you already found | the amplifier restates the defect as a rule and is forbidden the parent's literal text |
Some tests fail on main before any change (not introduced by this diff) |
named suppressor |
| Hollow tests: green but prove nothing | dedicated detector angle (delete an arrangement line, confirm RED) |
Suppressors are verified before they silence anything. Memory records what was true when written; a genuinely new failure in a known-flaky file must not be waved through.
A Mode B run without amplification is roughly 40–60 agent calls: 14 angle agents (8 Opus, 6 Sonnet), ~30 Sonnet scorers, ~11 Opus refuters. Amplification is default-on in Mode B and adds one Opus amplifier per confirmed finding, plus a scorer and refuter for each sibling that survives inspection — roughly +25 to +35 calls, for a typical total of 65–95. The ceiling is bounded: confirmed findings × 5 candidates.
Mode A is materially lighter: gated angles, a higher floor, and no amplification unless asked. The scorer stage is the volume stage and the obvious place to drop to Haiku if cost matters.
- Blocking / hard verdicts: advisory only, by decision.
- PR write-back:
review-prcould post inline comments; declined in favour of advisory-only. - Per-stage model override flags: too much surface for something rarely changed.
- Terminal markdown report: the native UI is the surface; files are memory behind it.
- Diff-triggered reachability analysis: expanding the pre-fan-out review surface from the diff to its unchanged dependents. Declined in favour of finding-triggered amplification, which costs an addend rather than a multiplier. Revisit if confirmed findings turn out to under-sample the ways a change breaks its callers.
- Interactive tracker setup command: a
--setupflow that asks once and persists a chosen issue tracker to a config file. Declined in favour of MCP-tool detection (§3): check every connected tracker for a match, ask only if ambiguous. Simpler, no state to keep in sync, no first-run step. Revisit if MCP-tool detection proves unreliable in practice.