Code Review

Review-only. Find problems; do not fix them. Every finding traces to a documented rule OR a real correctness / security / performance defect — never to taste.

Evidence-gate every finding — the false-positive guard. Before you flag something, prove it isn't intentional: read the callers (grep -rn the symbol) and the change's history (git log -p -- <file>), and check it isn't a sanctioned exception — an ADR / decision record, or a paths-matched rule that explicitly blesses the pattern. A pattern with a logged reason is intent, not a defect. No evidence, no finding.

Name the mechanism, not the feeling. "Cleaner", "more idiomatic", "too complex" precedes a finding: restate it as the property of this code that makes it hard and what that costs — a wrong call invited, a change broken, a reader misled — or drop it. One feeling is a finding: I cannot hold this in my head; rank it, and say what defeated you (files, hops, the invariant).

Phase 0 — Load profile

Tooling preflight — one call, before step 1. Some tools this skill relies on are deferred by the harness: the session lists them by name only and loads their schemas on demand, so calling one before it is fetched fails. Listing a tool in allowed-tools does not un-defer it. Issue a single ToolSearch up front covering the whole run — select:SendMessage,TaskOutput (continuing the same deep-analyzer / finding-verifier agent instead of respawning, collecting a backgrounded pass) — instead of one round-trip per discovery. A name already loaded costs nothing to include; a schema discovered missing mid-run costs a turn.

1. Read .claude/PROJECT.md — stack, architecture, dependency direction, commands. If CONTEXT.md exists, read it too — judge naming against the project's ubiquitous language.
2. Always read .claude/rules/_generic/: code-quality, exception-patterns, testing, comments, boundary-validation, external-api-integration.
3. After resolving the target (Phase 1), read every .claude/rules/* whose paths frontmatter matches a changed file. These stack/domain packs carry the framework- and library-specific checks — never hardcode such checks here. If PROJECT.md is missing or still TEMPLATE, fall back to the root CLAUDE.md (always in context) for stack/architecture/commands; note that you're running without a kit profile and that stack/library-specific checks are unavailable until /bootstrap runs, and review against the generic rules only. Only if neither carries the project's facts, say so.

Phase 1 — Resolve the target

From $ARGUMENTS, in order:

• A file or directory path → review those files.
• staged → git diff --cached --name-only.
• HEAD~N (or a commit/branch ref) → git diff <ref>...HEAD --name-only.
• Nothing → working-branch diff: git diff --name-only (uncommitted). If empty, fall back to git diff --cached --name-only, then to the branch's merge-base diff.

Skip binary, lock, and generated files. Read each surviving file in full and its diff hunks, so you judge both the change and its context. Note each file's layer/module from PROJECT.md → Architecture.

If the diff does not fit, stop and narrow — never truncate. A truncated read loses coverage silently while the report still looks complete. Narrow explicitly instead: git diff --stat for the map of changes, then whole files one at a time, and name in the summary what stayed unread (core.md → verdicts carry denominators).

Collect the change's decisions — the provenance block. List the decisions the diff embodies before judging any of them, each tagged:

• [AGENT] — decided by an agent while building. Every row of the plan's Deviation Report is one of these unless its Decided by cell carries a quote.
• [USER] — decided by the user, and only with the user's verbatim quote, in its original language, unparaphrased. In a fresh review session the build conversation is gone, so the quote comes from the artifact: the Deviation Report's Decided by cell, which /implement fills at the moment of the answer. No quote, no [USER] tag — it is [AGENT]. An agent that "remembers" a user decision is precisely the mechanism this tag exists to catch.

An empty block is a normal outcome (the plan held, nothing was quoted) — never invent decisions to fill it. The block is context for why the code looks like this, never established truth: press [AGENT] decisions hardest, because an agent's own choice re-labelled "the requirement" is how a review walks past its own author. A [USER] decision that looks wrong is not silently accepted either — raise it for discussion rather than reviewing around it. Legibility is judged before the explanation. Before collecting the block, write down where the code alone did not tell you what it does — an unnamed load-bearing order, a value whose origin you had to trace. Not a second pass: in Phase 2 a note lands on an anchor or is dropped.

Phase 2 — Review workflow

Examine every changed file in this focus order; stop chasing once a category is clean.

1. Correctness — does it do what it claims? Trace the happy path and one failure path. Edge cases (empty, boundary, null/NaN, off-by-one), error paths and their safety, type/conversion hazards, concurrency (races, shared mutable state), and side effects reaching outside the change.
2. Security — untrusted input reaching a sink, injection, unsafe deserialization, missing authorization/scoping, secrets or PII in code or logs, fail-open where it should fail-closed. Apply the paths-matched stack security rules.
3. Performance — repeated I/O or queries in loops, unbounded fetches, missing batching/streaming, needless allocation on hot paths. Apply stack rules for the framework's specific anti-patterns.
4. Readability / convention — naming, function size, duplication, dead code, wrong-layer logic and dependency-direction violations, magic values. Judge against code-quality.md and the matched stack packs. Four checks that list does not reach:
   • One name, two concepts — naming rules check a name in isolation, so a parameter reused for two concepts at two call sites passes all of them. Does it mean one thing everywhere?
   • Each conditional, classified — picking which object handles the case is fine; inspecting a value and supplying behaviour on its behalf means a type is missing.
   • An abstraction that does not pay — which error can the code no longer raise? If that list outweighs the code it simplifies, the duplication was cheaper.
   • A public entry point, misread — an ignored return value, or a flag assumed to raise: misreading that fails open is a finding, misreading that fails closed is not.

Mandatory when the change writes state: the write-path check. For every state-mutating path in the diff (persist, accumulate, finalize, publish), check idempotency and races before any verdict: can two paths reach the same write? does a retry or replay double-apply? is there a guard (status='open', unique constraint, upsert, processed-id set) or only the caller's discipline? The invariant is already a write-rule (.claude/rules/_generic/resilience.md → Idempotency); this is its read-side obligation. A double-apply is CRITICAL, not a smell. If the write-path is out of the diff and you did not open it, say so in the summary rather than letting the review imply coverage (core.md → verdicts carry denominators).

The verification question. One lens over the change, not a separate pass: if the behaviour this change exists to produce broke where it is actually used, would verification fail? Trace that behaviour to its consumers (grep -rn the symbol) instead of measuring the module's coverage, and state the concrete regression that would ship green — no demonstration, no finding, the same evidence gate as everything else. Tests that do not answer the question: ones that never executed (testing.md — unregistered, filtered out, skipped, disabled), assertions on source text rather than behaviour, no-throw and snapshot-only checks, and mock-only paths that never reach the changed code. Report a gap as missing tests for changed behaviour (STRUCTURAL).

Mandatory: comment-quality check. Audit every added/changed comment against .claude/rules/_generic/code.md — restating the code, commented-out code, debug/TODO leftovers, wrong language, multi-idea or unterminated comments. Report each as STYLE. A notice is not a mitigation. A risk answered with a comment, a NOTE:, a README line or a log warning is unmitigated — "it was documented" is no defence; a mitigation is a mechanism, or it sits where it cannot be walked past. Code that was hard to read is fixed by subtraction first, then by explaining only what survived the deletion.

For a complex or high-risk module (intricate logic, security-sensitive, large refactor), delegate a deeper pass to the deep-analyzer agent (or the security-reviewer agent when the risk is security-sensitive) and fold its findings in.

Phase 2.5 — Adversarial verification (CRITICAL findings)

Before reporting, run every CRITICAL candidate through the finding-verifier agent (batches of 3–4 in parallel; its job is to KILL the finding — ambiguity defaults to REFUTED). Then:

• CONFIRMED → stays CRITICAL.
• REFUTED → dropped, with a one-line reason in a "Dropped after verification" note.
• STALE / UNVERIFIED → downgraded to STRUCTURAL with the uncertainty named.

This is the same evidence gate /arch-health applies before its ranked table: a plausible-but-wrong CRITICAL costs the user more than a missed STYLE nit ever will. Skip this phase only when there are no CRITICAL candidates, or the target is a plan/epic doc (see below).

Phase 3 — Output

Group findings by severity. Each finding: file:line — the problem — the rule it violates or the concrete correctness/security/performance risk — a concrete fix. Do not apply any fix.

• CRITICAL — correctness bugs, security holes, data-integrity or missing-authz risks. Must fix before merge.
• STRUCTURAL — wrong-layer logic, dependency/convention violations, duplication, performance smells, missing tests for changed behaviour.
• STYLE — naming, comment-quality violations, minor readability nits.

CRITICAL findings carry their code; STRUCTURAL and STYLE carry path:line. For each CRITICAL, quote the 2–3 lines around the problem so the user decides with the code in front of them instead of blind. This applies only to this report — ephemeral output, read once, where a line anchor plus a snippet is the cheapest proof. It does not reach durable artifacts: plan files, decision records and the plan write-path below keep citing symbols, not line-anchored snippets (rules/_generic/code.md → durable text cites symbols, ephemeral output cites lines).

## Code Review — <target>
### CRITICAL
- `path:line` — <problem>. Violates <rule|risk>. Fix: <concrete change>.
  ```<lang>
  <the 2–3 lines around the problem>
  ```
### STRUCTURAL
- ...
### STYLE
- ...

End with one line: X critical, Y structural, Z style.

When the ask is architectural (a refactor, a new module, "is this safe to build on?"), you may additionally label the changeset with the shared quality verdict — SOUND / SHORTCUT / HACK as defined by the quality-auditor agent — mapping HACK→CRITICAL, SHORTCUT→STRUCTURAL. Same evidence gate applies: a verdict below SOUND needs the proof above — and SOUND on a module that writes state needs its write-path opened, else the label is bounded in words ("sound across the read-paths I opened; write-path unread").

When the target is a plan/epic (the one write exception)

Review-only forbids fixing the code — but when the review targets a plan or epic doc (not app code), the Artifact-Continuity Contract (rules/_generic/planning-artifacts.md) applies: fold the findings into the affected plan(s), cross-link this review from each plan's header (and the overview's "Reviews & decisions — READ FIRST" index), and sweep siblings the findings touch. This updates the plan DOC only — never the application code, which stays review-only.

Siblings

• To apply quality cleanups (style/structural fixes), point to /refactor.
• To debug a specific failure, point to /diagnose.
• To close test-coverage gaps, point to /test.