Code Review

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
• A file path → that file's changes: git diff HEAD --

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

1. 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

1. 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 () =>

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 ` and `gh pr view  --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 —  (``)

**Date:** YYYY-MM-DD
**Author:** 
**Files touched:** 
**Verdict:** APPROVE | APPROVE WITH FOLLOW-UPS | REQUEST_CHANGES | NEEDS_REVISION
**Linked task:** 

---

## Bug Validation

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

**Reported bug:** 
**Root cause:** 
**Does this PR fix it:** Yes / Partially / No
**If No/Partially:** 

---

## Issues

### [SEVERITY] N. 

**File:** `path/to/file.ts:LINE_START–LINE_END`
**Impact score:** X/10
**Reason:** 

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

Fix:

// 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 ite

…

Source & license

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

• Author: napnap11
• Source: napnap11/claude-skills
• License: MIT

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