code-review

---
name: code-review
description: Code review a pull request, commit, file, or local git changes. Use when the user says "review", "code review", "/code-review", "review local changes", "review PR", "review commit", or asks for a review of changes.
version: 1.0.0
allowed-tools: Bash(git *), Bash(gh *), Read, Grep, Glob, Edit, Write, Agent, Workflow, ToolSearch, AskUserQuestion, mcp__claude_ai_Figma__get_screenshot, mcp__claude_ai_Figma__get_design_context, mcp__claude_ai_Figma__get_variable_defs, mcp__claude_ai_Figma__get_metadata, mcp__claude_ai_Figma__get_code_connect_map, mcp__claude_ai_ClickUp__clickup_get_task, mcp__claude_ai_ClickUp__clickup_get_task_comments
---

# Code Review Skill

Run a careful, line-level review of a change set and record the outcome in `REVIEWED.md` at the project root (or wherever the user prefers to keep review notes — confirm if the destination is ambiguous).

## Gather context first

This skill ships standalone, so it can't assume your project's conventions, house style, or preferences the way it could for its author. Before doing the main work:

1. **Auto-detect what you safely can** from the repo — language/stack, base branch, build/test commands, existing config and docs. Never ask for something you can read for yourself.
2. **Ask, don't assume, for the rest.** Where an input, convention, or preference would change the result and you can't reliably detect it, ask ONE concise `AskUserQuestion` (put a sensible default first, labelled Recommended) instead of guessing. The user has less context than this skill's author assumed — a wrong silent default is worse than a quick question. Don't ask about things you can detect, and don't ask more than you need.

For this skill, confirm up front (only the items you can't already detect):
- **Output destination** — write `REVIEWED.md` at the project root (the default), append to an existing one, report inline, or use a path the user keeps review notes in.
- **Base branch to diff against** — for any branch-level or structural pass; auto-detect the repo's default branch (`git symbolic-ref refs/remotes/origin/HEAD`) and confirm only when it's ambiguous (multiple long-lived branches, a fork, a non-`main` trunk).
- **How strict the severity bar is** — the `MUST FIX` / `SHOULD FIX` / `FOLLOW-UP` ladder and verdict mapping, including whether unfixed sibling-branch code paths should escalate to `MUST FIX` / `REQUEST_CHANGES`.
- **Review depth** — line-level only, or also the structural (pre-merge-review-style) specialist pass (and, per *Subagents & parallelism*, inline vs parallel).

## Subagents & parallelism (opt-in)

By default this skill runs **inline in a single context** — no subagents, no Workflow fan-out — to keep it cheap to run.

Parallel subagents can be faster on large jobs (big diffs, many files, the multi-lens structural pass). **Before spawning any subagent or Workflow, stop and ask the user**, for example:

> This change is large ([N files / big diff], plus the structural specialist panel). I can review it inline here (cheaper, slower) or fan out [M] parallel subagents — one per area or specialist lens (faster, more tokens). Which do you want?

Spawn subagents/workflows only after an explicit yes. If the user declines or doesn't answer, do the whole job inline.

## Determining What to Review

Inspect `$ARGUMENTS`:
- Empty → review uncommitted local work: `git diff HEAD`
- A PR number (e.g. `123`) → that GitHub PR: `gh pr diff 123`
- A commit ref (e.g. `abc1234`) → that commit: `git show <ref>`
- A file path → that file's changes: `git diff HEAD -- <path>`

If there are no arguments and the working tree is clean, say so and stop.

## Task Validation

When the PR or branch is tied to a task (ClickUp, Linear, a GitHub issue, or something the user names), confirm the change actually resolves the reported problem:

1. **Pull the task** — use the right tool (`clickup_get_task` for ClickUp IDs like `86d2cdpcr`, `gh issue view` for GitHub issues) to read the bug description, expected behavior, and repro steps.
2. **Trace the bug** — follow the flow end to end, from the user action in the report through handlers → services → adapters → external APIs → response, and pin down the root cause of the reported behavior.
3. **Diff it against the fix** — decide whether this PR addresses that root cause, or whether it patches something else / the wrong layer.
4. **Verdict** — add a `## Bug Validation` section to the output containing:
   - The reported bug (1–2 sentences)
   - The traced root cause (with file paths)
   - Whether the PR fixes it: **Yes** / **Partially** / **No**
   - If No/Partially: what the real fix should target (files and approach)

If the PR does **not** fix the linked task, add a `BLOCKER` row to the summary table at impact 10/10. This overrides every other verdict — a PR that doesn't solve its stated problem cannot be approved.

### Sibling-branch coverage check (MUST FIX trigger)

Once you've confirmed the fix works on the demonstrated repro path, enumerate the **sibling branches that share the same root pattern** — the other arms of the same `if/else`, other values of the same enum, other config flags (`required_card`, `is_admin`, locale variants, and so on), other call sites of the same helper. For each one ask: *under realistic data, would the originally reported symptom still show up here?*

If the answer is yes, that sibling is a **MUST FIX**, not a SHOULD FIX. A bug ticket is, by definition, a complaint about broken behavior; a partial fix that still ships the same user-visible symptom for a meaningful slice of traffic belongs at MUST FIX (impact 7–8/10 depending on the traffic share), and the verdict becomes `REQUEST_CHANGES`.

The reasoning: bug-fix PRs exist to *eliminate* the reported behavior. Filing a partial fix as SHOULD FIX understates the gap — the user re-opens the same ticket, and the next fix re-derives context the reviewer already had in hand. Close it now.

**Skip this section** when no task/ticket is linked or mentioned.

## Review Process

1. Run the right git command to obtain the diff.
2. For every changed file, read the surrounding context — not only the diff lines — so you understand intent.
3. Evaluate the change across:
   - **Bugs** — logic errors, nil/null dereferences, off-by-one, race conditions
   - **Security** — injection, auth bypass, sensitive-data exposure, missing input validation
   - **Correctness** — does it do what it claims? which edge cases are missed?
   - **Data integrity** — could this lose or corrupt data?
   - **Performance** — N+1 queries, DB calls inside loops that should be batched, unbounded loops, missing indexes, needless allocations
   - **Maintainability** — dead code, misleading names, missing error handling at boundaries
4. Assign each issue a severity:
   - `MUST FIX` — bug, data loss, security, broken behavior. **Includes any sibling code path where the originally reported bug still reproduces** (see the sibling-branch check above).
   - `SHOULD FIX` — correctness concern, UX regression, missing guard
   - `FOLLOW-UP` — dead code, style, leftover `console.log`
5. Rate impact 1–10:
   - 9–10: data loss, security, crashes
   - 7–8: incorrect behavior visible to users
   - 5–6: degraded UX, stale data, performance
   - 3–4: maintainability, dead code, naming
   - 1–2: style, whitespace, minor inconsistency

## Structural Escalation (pre-merge-review pass)

After the line-level pass, decide whether the change warrants a deeper **structural** review — the same specialist-lens analysis the `/pre-merge-review` skill performs. Run that structural pass when ANY of these hold:

- The diff touches **database queries, API integrations, or auth/security code**
- The diff adds **new enum values, status strings, or type constants**
- The line-level review surfaced **any MUST FIX tied to race conditions, data safety, or trust boundaries**
- The diff changes **>5 files with logic** (not just config/assets)
- The diff introduces **new user-facing data flows** (form submissions, API calls that write data)

**Skip the structural pass for:**
- Asset-only changes (images, fonts, config files)
- Pure style/CSS changes with no logic
- Documentation-only changes
- Diffs with fewer than 3 files and no data/security concerns

**By default, run the structural pass inline in this context:** pick the specialist lenses the diff calls for and walk each one's checklist sequentially yourself, then fold the findings into REVIEWED.md. This keeps everything in a single context and cheap. Run the structural pass here directly — don't invoke the `/pre-merge-review` skill as a separate step from inside this review.

If the user opted into parallelism (see *Subagents & parallelism*), you may instead fan the specialist panel out as a dynamic Workflow — one `agent()` per lens, in parallel:

1. **Select specialists** by what the diff touches — security, performance, testing, maintainability, data-migration, api-contract, design (skip design if the diff is < 50 lines). Same selection logic as the `/pre-merge-review` skill, Step 3.5.
2. **Launch the specialist panel as a Workflow** (one `agent()` per lens, in parallel). Pass `args: { base, stack, testFw, specialists: [{ key, domain, checklist }, ...] }` — detect `base` from the repo's default branch (`git symbolic-ref refs/remotes/origin/HEAD`, confirm if ambiguous) and `stack` / `testFw` from the repo — where each `checklist` is the matching specialist checklist from the `/pre-merge-review` skill (Step 3.5, "Specialist checklists"):

```js
export const meta = {
  name: 'code-review-specialists',
  description: 'Parallel specialist review army over the branch diff — one lens per agent',
  phases: [{ title: 'Specialist' }],
}
const FINDING_SCHEMA = {
  type: 'object',
  properties: {
    specialist: { type: 'string' },
    findings: {
      type: 'array',
      items: {
        type: 'object',
        properties: {
          severity: { type: 'string' },
          confidence: { type: 'number' },
          path: { type: 'string' },
          line: { type: 'number' },
          category: { type: 'string' },
          summary: { type: 'string' },
          fix: { type: 'string' },
          fingerprint: { type: 'string' },
          evidence: { type: 'string' },
          test_stub: { type: 'string' },
        },
        required: ['severity', 'confidence', 'path', 'category', 'summary'],
      },
    },
  },
  required: ['specialist', 'findings'],
}
const reviews = await parallel(args.specialists.map(s => () =>
  agent(
    `You are a specialist code reviewer for ${s.domain} on a ${args.stack} project.\n\n` +
    `Run \`git diff origin/${args.base}\` to get the full diff. Review against the checklist below.\n\n` +
    `For each finding return: severity (CRITICAL|INFORMATIONAL), confidence (1-10), path, line (optional), ` +
    `category, summary, fix, fingerprint (path:line:category), specialist ("${s.domain}"); optionally evidence ` +
    `and a test_stub (minimal ${args.testFw} skeleton if a test would catch it). Before flagging a commented ` +
    `line, walk up to the nearest block boundary; if the enclosing function is itself commented, it is dead — ` +
    `do not file it. If no findings, return an empty array. Findings only, no preamble.\n\n` +
    `CHECKLIST:\n${s.checklist}`,
    { label: `specialist:${s.key}`, phase: 'Specialist', model: 'opus', schema: FINDING_SCHEMA }
  )
))
return reviews.filter(Boolean)
```

3. **Merge + fix-first:** dedupe by fingerprint, then classify each finding as AUTO-FIX (mechanical — dead code, N+1, magic numbers, missing output validation, unused imports → fix directly) or ASK (security, race conditions, design decisions, >20-line fixes, behavior changes → one AskUserQuestion). Any finding carrying a `test_stub` reclassifies to ASK. The full classification table lives in the `/pre-merge-review` skill, Step 4.
4. **Write** the findings into REVIEWED.md under a `## Staff Review Findings` section.

Whether you run the structural pass inline or as a Workflow, the findings land in the same `## Staff Review Findings` section. If a Workflow fails or times out, fall back to walking the specialist checklists inline (or note that the pass was skipped).

## Figma Comparison

When the diff changes UI, compare the **committed code against the Figma design spec** for the components this PR touches. This is a thin, PR-scoped, **static** spec diff — read-only, no rendering, no 0–10 / A–F scores, no fixes. It answers exactly one question: *does the built code match the Figma spec for the components this PR changed?*

**Run automatically when BOTH are true:**
- The diff touches **UI** — `.tsx`/`.jsx`/`.css`/Tailwind/component/page files with visual changes (not pure logic, config, tests, or assets).
- A **Figma reference is discoverable** (see *Finding the Figma ref* below).

**Do NOT run for:**
- Backend-only / API / config / logic-only diffs with no rendered surface.
- Docs / tests / assets only.
- UI diffs where no Figma ref turns up after the discovery steps — **skip silently, never block** (emit one line: `**Figma ref:** none found (skipped)`).

### Finding the Figma ref

The Figma MCP has **no search-by-name** — you must supply a concrete `fileKey` + `nodeId`. Discover them in order; stop at the first hit:
1. **PR body / comments** — grep `gh pr view <n>` and `gh pr view <n> --comments` for a `figma.com/...` URL or a fileKey/nodeId.
2. **Linked task** — resolve the task from the PR/branch, then `clickup_get_task` (description) **and `clickup_get_task_comments`**. The Figma URL often lives in the task **comments**, not the description.
3. **Project memory** — fall back to `reference_*figma*.md` for known fileKey/node IDs when the PR is on a matching feature branch.
4. **Ask the user** — one `AskUserQuestion` for the Figma URL or node ID (plain labels, no preview), only when 1–3 turn up nothing.

Parse `figma.com/design/:fileKey/:name?node-id=14100-712660` into `fileKey` and `nodeId = "14100:712660"` (the `-` and `:` forms are interchangeable). For a branch URL, use the `branchKey` as the fileKey.

### Pulling the spec (read-only Figma MCP)

Load the tools via ToolSearch (`select:mcp__claude_ai_Figma__get_design_context,mcp__claude_ai_Figma__get_screenshot,mcp__claude_ai_Figma__get_variable_defs,mcp__claude_ai_Figma__get_metadata,mcp__claude_ai_Figma__get_code_connect_map`), then for each touched component's node:
1. `get_screenshot(fileKey, nodeId)` — the visual reference (the returned PNG URL is short-lived; fetch it immediately).
2. `get_design_context(fileKey, nodeId, clientFrameworks:"react")` — the structural/styling intent to diff JSX/Tailwind against. If it degrades on an oversized frame, run `get_metadata` first and drill into child node IDs.
3. `get_variable_defs(fileKey, nodeId)` — resolved design tokens (colors, spacing, type) to diff against the project's Tailwind/theme constants.
4. `get_code_connect_map(fileKey, nodeId)` *(optional)* — resolves each Figma node to its repo component file so you compare against the right source. Empty when Code Connect isn't set up.

**Read-only only** — never call `use_figma`, `create_*`, `upload_*`, or any write/sync tool from a review.

### What to diff (objective build-vs-spec only)

For the components in the diff, flag concrete deviations:
- **Design tokens** — color / spacing / radius / font-size / weight / line-height in the code vs `get_variable_defs` values (and the project's Tailwind tokens).
- **Type ramp** — heading/body sizes, weights, hierarchy order.
- **Structure & content** — element order, labels/microcopy, iconography vs the Figma frame.
- **States present in Figma but absent from the build** — empty / error / hover / disabled / loading variants the design frames show.

Report each as a standard code-review finding (`file:line` + the Figma node it deviates from), severity-mapped:
- A user-visible structural / content / missing-state mismatch → **SHOULD FIX** (UX regression, impact 5–6).
- A pixel/token nit (spacing, color shade, radius) → **FOLLOW-UP** (impact 1–2).
- **MUST FIX** only when a mismatch breaks behavior — e.g. a required CTA or field present in the Figma spec is absent from the build.

Findings go in `REVIEWED.md` under `## Figma Comparison` and as rows in the shared Summary Table.

### Hand off depth — don't reimplement the design skills

This step does a static spec diff only. Defer anything deeper:
- **→ `/design-review`** when the gap is a *missing design decision* (unspecified states, IA/hierarchy questions, responsive/a11y not in the Figma, or no `DESIGN.md`). It rates dimensions 0–10 and edits the plan; code-review only names the gap and points there.
- **→ `/ui-polish`** when the deviation needs *eyes on the running app* or *source fixes* (rendered pixel/spacing polish, cross-page consistency, motion, performance-as-design, AI-slop grading, "fix it in source"). It screenshots the rendered app and commits fixes; code-review does neither.

Don't re-inline their AI-Slop blacklist or Design Hard Rules, and don't claim the bare "design review" trigger phrase.

## Output Format

**Always** record the review using this exact format — by default create or append to `REVIEWED.md` in the project root, or use the destination confirmed up front (an existing file, an inline report, or a custom path):

```md
# Review — <title> (`<commit/PR ref>`)

**Date:** YYYY-MM-DD
**Author:** <author from git log, or "local changes">
**Files touched:** <list>
**Verdict:** APPROVE | APPROVE WITH FOLLOW-UPS | REQUEST_CHANGES | NEEDS_REVISION
**Linked task:** <task URL or ID, if any>

---

## Bug Validation

_(Include this section only when a task/ticket is linked — see Task Validation above)_

**Reported bug:** <1-2 sentence summary of the bug from the task>
**Root cause:** <traced root cause with file paths>
**Does this PR fix it:** Yes / Partially / No
**If No/Partially:** <what the actual fix should target>

---

## Issues

### [SEVERITY] N. <Short title>

**File:** `path/to/file.ts:LINE_START–LINE_END`
**Impact score:** X/10
**Reason:** <explanation of what is wrong and why it matters>

**Current code:**
```lang
// the problematic code
```

**Fix:**
```lang
// the corrected code
```

---

## Staff Review Findings

_(Include this section only when the structural pass was run)_

**Structural pass status:** Ran inline / Ran as Workflow / Skipped (reason)
**Auto-fixed:** N items
**Needs input:** N items

[Include the structural findings here]

---

## Figma Comparison

_(Include this section only when the Figma comparison ran — see Figma Comparison above)_

**Figma ref:** <figma.com URL / fileKey + nodeId> | none found (skipped) | N/A (no UI changes)
**Components compared:** <built component ↔ Figma node, one per line>

[Per-mismatch entries in the standard issue block — built component `file:line` vs the Figma node, severity, impact, the deviation, and the fix.]

---

## Summary Table

| # | Severity | File | Line(s) | Impact | Issue | Action |
|---|----------|------|---------|--------|-------|--------|
| 1 | MUST FIX | `file.ts` | 123–130 | 9/10 | Description | Delete / Change / Add |
```

## Large PR Strategy (>10 files)

When a PR touches more than 10 changed files:
1. **By default, work through the areas one at a time in this context** (e.g. engine logic, handlers/DTOs, tests/seed data). If the user opted into parallelism (see *Subagents & parallelism*), launch one subagent per area instead of going sequentially.
2. **Subagents must NOT write directly to REVIEWED.md** — they return findings to the parent.
3. The parent consolidates and deduplicates all findings into a SINGLE review section.
4. Write the consolidated review to REVIEWED.md once.

## Rules

- **Always record the review** — by default write `REVIEWED.md`; never skip recording, even for small reviews or when there are no issues. If the user asked for an inline report or a different path up front, honor that.
- **Only code-level reviews go in REVIEWED.md** — founder reviews, eng reviews, and other strategic reviews live in their own files (`SCOPE_REVIEW_*.md`, `PLAN_REVIEW_*.md`), never in REVIEWED.md.
- **Always include the exact file path and line range** — read the file to verify line numbers before writing.
- **Always include a concrete before/after code block** for every issue — not just a description.
- If `REVIEWED.md` already exists, **append** the new review below the existing content — never overwrite.
- If there are no issues, still write the file with verdict APPROVE and an empty issues section.
- Verdict rules:
  - Any `BLOCKER` (PR doesn't fix the linked task) → `REQUEST_CHANGES`
  - Any `MUST FIX` → `REQUEST_CHANGES`
  - Only `SHOULD FIX` → `NEEDS_REVISION`
  - Only `FOLLOW-UP` → `APPROVE WITH FOLLOW-UPS`
  - Nothing → `APPROVE`
- Structural-pass findings with CRITICAL severity map to `MUST FIX` in the summary table.
- Structural-pass findings with INFORMATIONAL severity map to `FOLLOW-UP` based on impact.
- **The Figma comparison runs only on UI diffs with a discoverable Figma ref** — a static build-vs-spec diff, read-only (never call Figma write/sync tools, never edit source from it). A missing Figma ref is never a `BLOCKER`; just skip the section. Map deviations on the standard severity ladder — `MUST FIX` only when a required element from the Figma spec is absent from the build; user-visible structural/state mismatches → `SHOULD FIX`; pixel/token nits → `FOLLOW-UP`. Defer dimension scoring to `/design-review` and rendered audits / source fixes to `/ui-polish`.