PR Review

Review a change for defects. Bugs first, structure second, style last.

1. Resolve the target

Three-dot is merge-base: A...B shows B's side. Point it the wrong way and you silently review the opposite branch. Run git branch --show-current before choosing the row.

Validate before reviewing:

• Ref (branch, commit, tag, range) → git rev-parse <ref>.
• PR → gh pr view <n>; a fork PR may have no local ref.
• File path → exists and readable; review it whether or not it changed.

Bad target or empty diff → stop and say so. Ambiguous input → ask; never review a guessed target.

Counts for the §7 header: git diff --shortstat <range>, or gh pr view <n> --json title,body,additions,deletions,changedFiles — also returns the PR intent.

2. Gather context

A diff alone misleads — code wrong in isolation is often right in context, and vice versa.

• Full file for up to ~5 key changed files (entry points, trust boundaries, shared modules); hunk plus enclosing function for the rest. Over ~15 files → state which you read in full.
• Repo standards if present: CLAUDE.md, .claude/rules/*.md, CONTRIBUTING.md, CODING_STANDARDS.md, AGENTS.md, CONVENTIONS.md. A documented standard overrides every default here and in references/.
• Spec: issue ref in commits (gh issue view <n>), ticket key in branch name, or a doc under docs//specs/. None → skip the spec axis, say so in the report. Never invent requirements.
• Input, auth, storage, network, rendering, or secrets touched → trace the trust boundary, not just the changed lines.

3. Review axes

Ordered by leverage; spend effort top-down.

Correctness

• Logic errors, off-by-one, inverted or missing conditionals, unreachable branches.
• Edge cases: null/empty, boundaries, overflow, concurrent access.
• Error paths: swallowed failures, wrong error type, silent fallback hiding an unclear invariant.
• Tests: exist, test behavior not implementation, would catch this regression.

Spec — only if found.

• Requirements missing, partial, or implemented wrong — quote the spec line.
• Behavior nobody asked for (scope creep).

Structure

• Near-duplicate bolted on instead of the existing pattern or canonical helper.
• New conditional grafted onto an unrelated flow → missing abstraction, not a nit.
• Feature-specific logic leaking into a shared module.
• Refactor that relocates complexity instead of removing it — same number of concepts to hold means not cleaner.
• Gratuitous any or cast over a type boundary. unknown plus narrowing is the fix, not the smell.

Security

• Changed code is reachable by hostile input unless verifiably not.
• Validation and authorization at the real boundary, not the UI or the caller.

Performance — only when obvious: N+1, unbounded loops or fetches, O(n²) on growing data, blocking I/O on a hot path, missing pagination. Quantify ("~50ms per item") or state the growth ("one query per line item; an order carries up to 50") — never "this could be slow".

4. References

One file per concern the diff warrants; a second only when a finding spans areas. Never read all.

5. Before flagging anything

• Be certain. Unsure → investigate. Still unsure → "I'm not sure about X", not a finding.
• Only what changed. Pre-existing code is out of scope unless the change makes it newly wrong. A user-named file is wholly in scope.
• No hypotheticals. Name the concrete input/state that triggers the failure, or drop it.
• Security needs an exploit path — attacker-controlled input, missing control, impact. Never "potential security concern".
• Skip what tooling enforces — formatting, lint, compiler errors.
• Style only against a documented convention. A let is fine when the alternative is convoluted.

6. Severity

Order by leverage, not file order. One structural problem plus ten nits → the structural problem is the review.

Verdict: Approve when the change improves code health, even if imperfect. Request changes on any Blocker, or Should-fixes that materially degrade the codebase. Never block over "I'd have written it differently".

7. Output format

Exactly this shape. Skip any severity section with no findings.

## Review — <target> (<N> files, +<add>/−<del>)

**Verdict:** <Approve | Request changes> — <n> blocker, <n> should-fix, <n> nits

### Blockers
**1. <one-line claim>** · `path/file.ts:LINE`
<Why it breaks — the concrete input or state, and the consequence. 1-3 lines.>
→ <Concrete fix.>

### Should fix
**2. <one-line claim>** · `path/file.ts:LINE`
<Why. 1-2 lines.>
→ <Fix.>

### Nits
**3.** <claim> · `path/file.ts:LINE`

• Number findings continuously across sections so they're citable.
• Blocker / Should fix: location + why + fix. Nit: one line, location + claim only.
• Closing lines, each only if applicable:
  • **Not flagged:** <thing> — <reason>. — considered and deliberately skipped.
  • **Needs a second look:** <thing> — <why>. — non-defect needing human sign-off. No severity. Covers:
    • Schema change.
    • API contract change of unclear blast radius. A confirmed breaking change is a defect — give it a severity.
    • New dependency or framework.
    • Change to a performance- or security-sensitive path.
    • Dead code left by the change — name each symbol and why it's dead, e.g. formatLegacyDate() in src/utils/date.ts (replaced by formatDate()) — safe to remove?. Never delete silently.
  • No spec found.
• Nothing found → **Verdict:** Approve — no findings. plus one line on what you checked.

Tone: matter-of-fact. No praise, no thanks, no summary restating findings.