review-code

What this skill does

# Code Review

Review code for design and architecture issues that linters and static analysis tools miss.

> [!IMPORTANT]
> Consult [REFERENCE.md](REFERENCE.md) for the expected output format and level of detail.

## Prerequisites

This review focuses on design issues. Standard tooling handles the mechanical checks and is assumed to run alongside (not before) this review:
- **Linters** (ESLint, golangci-lint, pylint) catch complexity, length, nesting, unused code
- **Formatters** (prettier, gofmt, black) handle style
- **Security scanners** (Semgrep, CodeQL, Bandit) catch injection, XSS, secrets
- **Type checkers** (TypeScript, mypy) catch type errors

If any of these aren't set up, mention it in the report so the team can add them — but proceed with the design review regardless. Skip mechanical findings that those tools would catch.

## Scope

Determine the review scope before discovering files:

- If `$ARGUMENTS` is non-empty, treat it as a path (file or directory) and run:
  ```bash
  ${CLAUDE_PLUGIN_ROOT}/scripts/discover-files.sh "$ARGUMENTS"
  ```
- If `$ARGUMENTS` is empty, scope to files added or modified on the current branch relative to the default branch:
  ```bash
  ${CLAUDE_PLUGIN_ROOT}/scripts/discover-files.sh
  ```

Handle the script's exit codes:
- **0 with output** — use the listed paths as input to the discovery step below.
- **0 with empty output** — branch has no diff vs the default branch. Tell the user and ask which path to review.
- **non-zero** — script prints a message to stderr (path not found, not a git repo, on the default branch with no path, detached HEAD, or default branch indeterminate). Relay the message and ask the user which path to review.

The script returns paths language-blind. The discovery step below filters to source files; if the filter excludes everything but the script's output was non-empty, the language may not be in the pattern list — apply judgment to identify source files in the output.

## Workflow

### Step 1 — Discover source files

From the script's output, filter to source files, excluding test files, vendored/generated code, and files that are purely configuration. Record the full file list and count.

### Step 2 — Choose execution strategy

- **1–4 files → Direct mode**: Read the files, evaluate against the Design Criteria below (skip mechanical checks tools handle), then proceed to Pattern Collapsing.
- **5+ files → Parallel mode**: Batch files, spawn subagents, collect results, merge, then proceed to Pattern Collapsing.

Adjust by file size when the count is on the boundary: if 5–6 files are small (under ~200 lines each), direct mode is fine; if 3–4 files are large (over ~400 lines each), prefer parallel mode. Use judgment — the goal is to avoid serially reading thousands of lines in one context, not to hit an exact threshold.

### Parallel Review Mode

Use this mode when there are enough files (or files are large enough) that reading them serially would meaningfully slow the review.

#### Batching

Group files into batches based on total file count:

| Total files | Files per batch | ~Subagents |
|-------------|-----------------|------------|
| 5–10        | 1               | 5–10       |
| 11–20       | 2               | 6–10       |
| 21+         | 3               | 7–10       |

#### Spawn subagents

For each batch, use `Agent(subagent_type="general-purpose")`. **Spawn all subagents in a single message** so they run in parallel.

Each subagent prompt MUST include:

1. The file paths in its batch (instruct the subagent to read them)
2. The **Design Criteria** section from this skill — copy it verbatim into the prompt
3. The **Severity** section from this skill — copy it verbatim into the prompt
4. The **Prerequisites** note — remind the subagent to skip mechanical checks that linters/formatters/scanners handle
5. The structured output format below
6. The explicit instruction: **"Do NOT use the Bash tool. Do NOT run any shell commands. Use only Read, Grep, and Glob tools. Return findings only."** — the review is static analysis of source files, so shell access adds latency and side-effect risk without enabling anything the read-only tools can't already do.
7. The explicit instruction: **"For every P2 and P3 finding, you MUST state a concrete consequence in the `explanation` field: name a specific extension, change, or maintenance scenario where a caller or maintainer would predictably go wrong because of this design (e.g., 'adding a CLI entry point would force re-implementing the discount logic that's currently embedded in the HTTP handler'). Omit findings that lack this claim. The single exception is P1 findings (security design flaws and tests-cannot-be-written designs carry their consequence implicitly)."**
8. The explicit instruction: **"For the `pattern` field, use a short, reusable label that names the underlying anti-pattern (e.g., 'module-scope side effects', 'mixed abstraction in handlers'). If two findings in your batch stem from the same root cause, they MUST use the same pattern label."**

Instruct each subagent to return findings in this exact delimited format (one block per finding):

```
---FINDING---
priority: P<1|2|3>
location: <file:line>
title: <short title>
category: <Single Responsibility|Abstraction Levels|Meaningful Naming|Testability|API Design|Error Handling Strategy>
pattern: <short label for the underlying anti-pattern, e.g. "module-scope side effects" or "mixed abstraction in handlers" — use the SAME label across findings that share the same root cause>
explanation: <what is wrong and why it matters>
fix: <concrete prescription>
done_when: <verifiable criterion>
---END---
```

If the subagent finds no issues for its batch, it should return `---NO-FINDINGS---`.

#### Collect and merge

After all subagents return:

1. Parse each subagent's structured findings
2. Combine into a single list, sorted by priority (P1 first)
3. Deduplicate: if two findings share the same `location` (file:line) AND the same `category`, keep only the one with the highest priority
4. Group findings by `pattern` label — findings from different subagents that used the same (or very similar) pattern label share a root cause and will be collapsed in the Pattern Collapsing step

#### Error fallback

If a subagent fails or returns unparseable output, review those files directly (as in direct mode) and include a note in the report: `Note: Files [list] were reviewed directly due to subagent failure.`

### Pattern Collapsing

Both direct mode and parallel mode flow into this step before producing the final report.

After merging all findings, look for findings that share the **same root cause** — i.e., the same design pattern repeated across multiple files. Examples:

- Multiple files flagged for "module-level side effect blocks testability" → one pattern: "codebase initialises services at module scope instead of using injection"
- Multiple files flagged for "mixed abstraction levels" where the same kind of mixing recurs → one pattern: "business logic is interleaved with infrastructure calls throughout"

When you identify a shared root cause:

1. **Collapse** the N per-file findings into **one finding** that names the pattern, lists all affected files, and prescribes the codebase-wide fix
2. **Set severity** to the highest severity among the collapsed findings
3. **Keep separate** any findings that happen to share a category but have genuinely different root causes

This is critical: N findings for N instances of the same pattern creates noise. One finding that names the pattern and lists the affected locations is actionable.

---

## Design Criteria

### Single Responsibility

Flag when a unit has multiple unrelated reasons to change:
- Functions that do X AND Y (validate AND save, fetch AND transform AND render)
- Classes with unrelated method groups (UserService with email sending and caching)
- Files with unrelated exports
- Modules mixing infrastructure with business logic

Ask: "If requir