/code-review

name: code-review
description: |
  Two-mode skill: (1) adversarial review — spec compliance + code quality + security,
  confidence-scored findings with file:line evidence; (2) receiving review — verify-before-
  agreeing discipline for acting on external/human review feedback.
allowed-tools: Read Grep Glob LSP Bash
user-invocable: false

Code Review (Adversarial + Receiving)

Reference Files

Read only what's needed:

• `references/review-order-and-checkpoints.md` — review order, checkpoint discipline; load when starting a review that spans multiple files or needs a checkpointed pass
• `references/code-review-heuristics.md` — heuristics, pattern recognition, false-positive prevention; load when the diff is non-trivial or before reporting CLEAN (Zero-Finding Halt re-scan)
• `references/security-review-checklist.md` — security review checklist; load whenever the diff touches auth, input handling, network, secrets, or data access

---

Run ADVERSARIAL when producing findings on a diff; run RECEIVING when acting on findings someone else produced. The Code Smells catalog, AI-Generated Anti-Patterns, Metric Honesty Rule, and Deferred Findings handling below apply in both modes.

Mode: ADVERSARIAL REVIEW

Only report issues with confidence ≥80 — below that, a finding is more likely noise than signal, and noise burns the fix loop's time and trust. Do not inflate a score to smuggle a hunch through; a genuine security hunch goes to the Summary as an open question (see the Security exception under Confidence Scoring). Every finding states category, impact, and why it matters. Present a recommendation, not a menu. Be opinionated.

**Signal quality rule:** One finding with `file:line` evidence and a fix is worth more than ten generic observations. Never report a pattern without showing where it lives.

Two-Stage Review

**Stage 1: Spec Compliance** — Does the code do what the plan/spec asked? Check: phase exit criteria met, interfaces match plan's Consumes/Produces, no scope drift, no missing scenarios.

**Stage 2: Code Quality** — Is the code well-built? Check: correctness, performance, security, clarity, test coverage.

Review Order

Top-down (spec → architecture → module → function → line) for first pass. Bottom-up (line → function → module) for detail pass. See `references/review-order-and-checkpoints.md`.

Severity Classification

| Severity | Criteria | | ---------- | ---------- | | CRITICAL | Data loss, security breach, silent data corruption | | HIGH | User-visible broken behavior | | MEDIUM | Suboptimal but functional | | LOW | Code smell, style |

Confidence Scoring

| Confidence | Meaning | | ----------- | --------- | | 90-100 | Verified: read the code, confirmed the issue, can cite file:line | | 80-89 | Strong: read surrounding context, pattern is clear | | <80 | Do not report — insufficient evidence |

**Security exception:** a security-category finding below 80 confidence is NOT silently dropped. Surface it as an explicit open question in the review Summary (e.g., "Possible auth bypass at `file:line` — could not confirm exploit path"), not as a finding. The <80 floor drops everything else.

Parallel Review + Router Merge

When `code-reviewer` and `failure-hunter` run in parallel (BUILD workflow):

• **code-reviewer** (Assessment A): correctness, performance, spec compliance. Forms opinion WITHOUT seeing the hunter's scan.
• **failure-hunter** (Assessment B): silent failure scan using red-flags table. Does NOT see the reviewer's findings.
• **Router-owned merge:** after both complete, the router writes a merged findings summary into the workflow artifact before verifier handoff. Where both agree → high confidence. Where the hunter caught what the reviewer missed → keep. Where the hunter finding is a false positive → drop with reason. Contradictory verdicts: stricter verdict wins, logged in `status_history`.

Zero-Finding Halt

Zero findings on a non-trivial change → insufficient depth, not perfect code. Re-scan against heuristics and security checklist before reporting CLEAN.

Code Smells (Fowler Catalog)

Scan for these 16 named smells during review. Each is actionable — not a style preference. "Messy" is not actionable; "Mysterious Name" is. Each smell is a labelled heuristic and always a judgement call ("possible Feature Envy"), never a hard violation:

| Smell | Signal | Fix | | ------ | ------ | ---- | | **Mysterious Name** | Function/variable name doesn't reveal intent | Rename to describe what it does | | **Long Method** | Method > 20 lines doing multiple things | Extract sub-methods | | **Long Parameter List** | > 4 parameters — consider parameter object | Extract into an object | | **Large Class** | Class with too many responsibilities | Split by responsibility | | **Data Class** | Holds data, no behavior — anemic domain model | Move behavior in, or inline the class | | **Duplicated Code** | Same logic in 3+ places | Extract shared function | | **Feature Envy** | Method reads more from another class than its own | Move method to the class it envies | | **Shotgun Surgery** | One change requires touching many files | Consolidate responsibility | | **Divergent Change** | One class changes for different reasons | Split into separate classes | | **Primitive Obsession** | Using primitives where a small value object would add meaning | Create a value object | | **Repeated Switches** | Same switch/if-else on a type across files | Replace with polymorphism | | **Speculative Generality** | Abstraction for future use that never comes | Delete it (YAGNI) | | **Message Chains** | `a.b().c().d()` — client knows the object graph | Hide the chain behind a method | | **Middle Man** | Class just delegates to another — adds no logic | Remove the middleman, use the real object | | **Refused Bequest** | Subclass doesn't use parent's methods | Replace inheritance with composition | | **Data Clumps** | 3+ values always passed together | Extract into an obje