Code Review

/code-review — Systematic Code Review

Perform a structured, multi-phase code review. This skill produces actionable findings with severity, location, and concrete fixes — not style nits or praise.

Use memory-mcp's read tool to load the code-review-patterns memory (scope: global) before starting. It contains learned patterns from previous reviews that should inform what you look for. If the memory doesn't exist yet, proceed without it — findings from this review will seed it.

Argument handling

$ARGUMENTS determines the review scope. The default is stack, which uses the revset alias defined in [references/stacking-conventions.md](../references/stacking-conventions.md).

| Argument | Scope | |-----------------------------|------------------------------------------------------| | (empty) or stack | bookmark_base()..@ — the current stack from last published bookmark | | change | Single jj change (use the alphabetic change ID) | | range | Any jj revset, e.g. pr/layer-1..pr/layer-2 | | working | Working-copy diff only — @-..@ | | pr or pr | The change(s) backing the current branch's PR, or PR #N | | --since | Modifier on any scope: limit to descendants of `` |

If bookmark_base() resolves to nothing (no remote bookmarks in ancestry), fall back to trunk()..@ and tell the user this happened.

If the resolved revset is empty, report it and stop. The most common reason is that @ is sitting on a published bookmark with no work on top — the user needs to jj new first.

Incremental review mode (--since)

When --since is appended (e.g., stack --since abcdefg), the review operates in incremental mode for fix-round efficiency:

1. The primary diff is limited to the descendants of `` intersected

with the base scope. Sub-agents analyze only this subset.

1. Sub-agents also receive a prior findings summary — the findings from the

previous round, with their status (fixed, still-open, or deferred-with-rationale).

1. Sub-agents are instructed to:

• Verify each prior finding is addressed (fixed in the new commits or explicitly deferred)
• Flag new issues introduced by the fix commits
• Only cross-reference unchanged code when the new changes directly affect it
• Not re-review code that hasn't changed since the last review

1. The coordinator merges the incremental findings with the prior findings to produce a

cumulative status report.

Change IDs are stable across rebases in jj, so --since survives jj squash, jj rebase, and other history rewrites without breaking. This is materially better than git-SHA-based incremental review.

The /develop skill's Phase 4.5 uses this mode automatically when re-invoking /code-review after fixes.

Phase 1: Gather context

Before reviewing code, build understanding. This phase is silent — no output to user.

1. Resolve the scope. Run jj log -r to confirm the revset matches what

the user intended. For pr scope, fetch the PR's commits and resolve to their change IDs.

1. Identify changed files and hunks. Use jj diff -r for the cumulative

diff across the scope. For per-change context, jj diff -r .

1. Read project conventions — check for .claude/CLAUDE.md, memory-mcp project memories

(use list filtered by project scope, look for project-overview, conventions), and any linter/formatter configs.

1. Understand architecture — for non-trivial changes, use Serena's get_symbols_overview

on affected files to understand the surrounding code structure. Read symbol bodies only when needed to understand how changed code fits into the system.

1. Trace callers — for any function/method whose signature, behavior, or error handling

changed, use find_referencing_symbols to identify all call sites. This is critical for catching breakage that looks fine in isolation.

1. Load design artifacts — check for docs/design/ and docs/adr/ directories. If

the change has associated design docs (requirements, test plans, architecture, ADRs), read them. These are the contract for what should exist — sub-agents need them to identify missing coverage, not just bugs in code that's present. Also check for linked issues (gh issue view ) if the branch name or commit messages reference one. Pass relevant artifacts to sub-agents as "Design context."

Resolving design-vs-code divergence: when a sub-agent finds that the code doesn't match the design docs (or vice versa), don't mechanically pick a winner. Determine what the user intended this change to accomplish, then judge the divergence in that context:

1. Establish the change's intent. Read the PR description, linked issue, commit

messages, and any conversation context. What was the user trying to build? Is this a "implement the spec" change, or a "prototype and iterate" change?

1. Classify the divergence:

• Unimplemented requirement — design says X should exist, code doesn't have it.

But is it in scope? Check whether the issue or PR description scopes this to a subset of the design. Phased implementation is normal — missing requirements are only findings if they're in scope for this change.

• Contradicted requirement — code does Y where design says X. This could be:

(a) a bug in the code, (b) a discovery during implementation that invalidates the design, or (c) an intentional deviation the user hasn't documented yet.

• Stale design — the design describes an older architecture and hasn't been

updated to match intentional evolution. Common when design docs are written up-front and code legitimately outgrows them.

1. Choose severity based on confidence:

• If the divergence contradicts explicit in-scope requirements and there's no

signal the user intended to deviate → P2 finding.

• If the divergence might be intentional (code looks deliberate, or the design

is old and the code is clearly more evolved) → P3, framed as "design doc may be stale — verify intent."

• If you genuinely can't tell → surface both sides to the user without assuming

either is correct. Quote the specific requirement and the specific code, and ask which reflects current intent.

Timestamps are a useful signal (check with jj log -r 'ancestors(@)' --no-graph -T 'change_id ++ " " ++ committer.timestamp().local_format("%Y-%m-%d")' ) but they're not authority. A design doc committed yesterday can still be aspirational for a future phase. Code committed today can still be wrong.

Context budget: aim for roughly 1:1 ratio of changed code to surrounding context. More context than code means you're over-reading. Less means you're likely missing impact.

Large diff warning: review effectiveness drops sharply past 400 lines of changed code. If the cumulative diff exceeds ~500 lines, tell the user upfront and suggest either reviewing per-change (each commit in the stack reviewed independently) or splitting the stack. For stacks, per-change review is usually the right answer — that's what the stack structure is for.

Phase 2: Analyze

Launch all three sub-agents in a single message with three parallel Agent tool calls, each with run_in_background: true. This ensures true concurrent execution — launching them sequentially wastes time and defeats the purpose of independent analysis. Each agent gets the same diff and context but a different analytical lens. The separation ensures independent findings — a bug one agent normalizes, another catches. Use sonnet for sub-agents A and B (mechanical analysis), opus (4.6) for sub-agent C (judgment-heavy architectural review).

When reviewing a multi-change scope (a stack of N commits), there are two valid approaches:

• Cumulative: pass all sub-agents the cumulative diff. Faster, fewer tokens, but

loses per-change boundaries — a bug introduced in commit 1 and fixed in commit 3 doesn't surface as either a bug or a fix.

• Per-change: dispatch one set of sub-agents per change in the stack. Slower, more

tokens, but each commit is reviewed as the reviewer will see it on the PR.

Default to per-change for stacks of 2 or more, cumulative for single-change scopes. The user can override with --cumulative or --per-change.

Sub-agent A: Correctness & Safety

You are reviewing code changes for correctness and safety issues. Precision matters
more than count. Every genuine finding at any priority level (P1, P2, or P3) is
valuable and will be addressed. False positives waste verification time and erode
trust in the review process — a finding that isn't real is worse than a finding
you didn't report.

Review the following changes for:

**Logic errors**
- Off-by-one, wrong operator, inverted condition, missing early return
- Race conditions, TOCTOU, shared mutable state
- Error handling: swallowed errors, wrong error type, missing propagation

**Completeness gaps** (this is the #1 thing LLM reviewers miss — be thorough)
- For each branch/match arm: what cases exist in the domain that aren't covered?
- For each identifier used in dedup/lookup/fallback: what entity types share that
  format? (e.g., owner/repo#77 matches issues, PRs, AND discussions)
- For each pattern match on input: trace all callers — what inputs reach this code
  that DON'T match any handled pattern?
- "Inputs always look like X" is a red flag — verify by tracing actual data flow
- For each resource created (sessions, connections, handles, caches, temp files):
  what cleans it up? Timeout, eviction, explicit close, Drop impl? If nothing
  cleans it up, that's a finding.

**Data integrity**
- Mutations that could corrupt or lose data (wrong UPDATE scope, missing WHERE clause)
- LIKE/GLOB wildcards in user-supplied values without escaping
- Identifier collisions across entity types sharing the same format
- Upsert/dedup logic that collapses things that should remain distinct

For each finding, output EXACTLY this format:
**[P1|P2|P3] **
- File: `:`
- Change: ``
- Issue: 
- Impact: 
- Fix: 

Sub-agent B: Design & Maintainability

You are reviewing code changes for design and maintainability issues. Precision matters
more than count. Every genuine finding at any priority level (P1, P2, or P3) is
valuable and will be addressed. False positives waste verification time and erode
trust in the review process — a finding that isn't real is worse than a finding
you didn't report.

Review the following changes for:

**API contract issues**
- Breaking changes to public interfaces without migration
- Inconsistent naming, parameter ordering, or return types vs existing patterns
- Missing or misleading error messages that will confuse callers

**Dead code & redundancy**
- Code that the change made unreachable or unnecessary
- Duplicated logic that should be consolidated
- Imports, variables, enum variants, or parameters that are no longer used
- For `Option`-guarded features: does disabling the feature leave allocated-but-unused
  fields? If so, group the feature's state into a sub-struct and wrap in `Option`.

**Testing gaps**
- Changed behavior that has no corresponding test update
- Edge cases in new code that tests don't cover
- Test assertions that don't actually verify the behavior they claim to test

**Test quality**
- Do tests exercise the library's public API, or do they duplicate internal logic?
  Tests that reimplement the production code path instead of calling it prove nothing
  about the real code.
- Are assertions non-vacuous? A test should fail if its assertion is removed. Tests
  that compare single-element collections, assert `true`, or check trivially-true
  conditions waste CI time and give false confidence.
- For edge-case tests: is the edge case actually exercised? Trace the test input
  through the code — does it actually hit the branch/condition the test name claims?

**Stack hygiene** (when reviewing per-change in a stack)
- Does this change belong in this commit? If a fixup belongs in an earlier commit
  in the stack, it should be squashed there before landing.
- Are commits cohesive? Each commit should be one logical change. Mixing refactor
  + new feature in one commit makes review harder and revert riskier.

**Architectural fit**
- Does this change follow the project's established patterns?
- Are abstractions at the right level? (over-engineering is as bad as under-)
- Will this change make future work harder? (coupling, hidden dependencies)

**Design validation** (when design artifacts are provided)
- Cross-reference requirements against implementation: does each requirement (R-01,
  R-02, ...) have corresponding code? Flag requirements with no implementation.
- Cross-reference test plan against tests: does each test case (TC-01, TC-02a, ...)
  have a corresponding test? Flag test cases with no test, and tests that don't
  actually verify what the test case specifies.
- Cross-reference ADR decisions against implementation: does the code match the
  decision? Flag divergences (may be intentional — report, don't assume).
- Check behavioral parity requirements: if the design says "test double must behave
  like production for X," verify the double actually enforces X (e.g., dimension
  validation, error semantics).

For each finding, output EXACTLY this format:
**[P1|P2|P3] **
- File: `:`
- Change: ``
- Issue: 
- Impact: 
- Fix: 

Sub-agent C: Architecture & Security (model: opus 4.6)

You are reviewing code changes for architectural fitness and security. You did NOT
write this code and have NOT seen the implementation process — only the diff and
the project structure. This separation is intentional: you catch things the
implementer normalized away. Precision matters more than count. Every genuine
finding at any priority level (P1, P2, or P3) is valuable and will be addressed.
False positives waste verification time and erode trust in the review process — a
finding that isn't real is worse than a finding you didn't report.

Review the following changes for:

**API contracts**
- Are changes backward-compatible? If breaking: are ALL callers updated?
  Use `find_referencing_symbols` to verify.
- Are error types consistent with the project's conventions?
- Would a consumer of this API be surprised by the new behavior?

**Architectural fit**
- Does this follow established patterns in the codebase?
- If introducing a new pattern: is the old pattern being migrated, or will both coexist?
- Are dependencies flowing in the right direction? (no circular deps, no upward deps)
- Is the abstraction level appropriate? (not over-engineered, not under-abstracted)

**Completeness**
- For each match/branch: what cases exist in the domain that aren't handled?
- For each input path: trace what data can actually arrive — are all shapes covered?
- Are error paths tested? Is the happy path the only path tested?

**Design compliance** (when design artifacts are provided)
- Verify ADR decisions are faithfully implemented — if an ADR says "use X pattern,"
  confirm the code uses X, not a variation. Flag divergences as findings.
- Verify requirements coverage — does each requirement have corresponding code?
  Missing requirements are P2 findings.
- Verify architectural diagrams match the actual module/trait/type structure.
  Stale diagrams that don't match code are P3 findings.

**Security (STRIDE threat model)**
For each change that touches trust boundaries, data flows, or auth:
- Spoofing: can an attacker impersonate a user or system?
- Tampering: can data be modified in transit/at rest without detection?
- Repudiation: can actions occur without accountability/logging?
- Information

…

## Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

- **Author:** [butterflyskies](https://github.com/butterflyskies)
- **Source:** [butterflyskies/claude-skills](https://github.com/butterflyskies/claude-skills)
- **License:** Apache-2.0

Install and usage instructions live in the source repository linked above.