---
name: code-review
description: Reviews code changes with a severity-ranked checklist and Google engineering review standards. Use for reviewing PRs, diffs, or staged changes, responding to review feedback, preparing a change for review, or splitting an oversized change.
license: MIT
metadata:
  author: lammanhhoang
  version: "1.0.0"
---

# Code Review

## When to use

- Reviewing a PR, diff, patch, or staged/uncommitted changes.
- Responding to review feedback on your own change (author side).
- Preparing a change for review (self-review pass).
- Deciding whether/how to split an oversized change.

Do NOT use for: design-doc/RFC review, dedicated security audits (use a security-review skill), CI/branch-protection configuration, or performance reviews of people.

## Core principle (Iron Law)

Approve once the change is in a state where it "definitely improves the overall code health of the system being worked on, even if the CL isn't perfect" (Google eng-practices, The Standard of Code Review). There is no perfect code — only better code.

Two failure modes, both forbidden:

- **Perfectionism hostage-taking**: blocking merge over polish you would merely prefer. That feedback is `Nit:` — say it and approve anyway.
- **Rubber stamps**: approving a diff you did not fully read. If you didn't read every line you were asked to review, you didn't review it.

## Workflow (reviewing a change)

1. **Isolate context.** Where the harness supports it, run the review in a fresh-context subagent that sees only the diff and the change description — never the author's reasoning from earlier in the session. Prior context makes you review what the author *meant*, not what they *wrote*.
2. **Fetch the change.** Default tool: `gh` for PRs, plain `git` locally.
   - PR: `gh pr view <n>` (description) then `gh pr diff <n>`
   - Branch: `git diff main...HEAD` (three dots = since merge-base)
   - Staged: `git diff --cached`
   - Escape hatch: no `gh` → review from `git fetch origin pull/<n>/head:pr-<n> && git diff main...pr-<n>`, or paste from the web UI.
3. **Size gate first**: `git diff --stat main...HEAD`. Over ~1000 changed lines (excluding generated files, lockfiles, mechanical renames): stop and request a split (see Size discipline) before reviewing further. ~100 lines is the ideal unit; 100–400 is normal.
4. **Read the description.** It must explain WHY. If you cannot tell what problem the change solves, that is your first `[blocker]` — on the description, before any code.
5. **Broad pass — Design (level 1).** Does this change belong in this codebase, now, at this layer? Does it integrate sanely with existing systems? Wrong design ⇒ stop the line: say so immediately and skip lower levels. Never polish naming on code that shouldn't exist.
6. **Logic pass — Functionality (2), Complexity (3), Tests (4).** Read the main files first (largest logical change), then everything else. Read EVERY line you were asked to review; skim only verified-generated code.
7. **Line pass — Naming (5), Comments (6), Style (7), Documentation (8).**
8. For concrete per-category check questions, read references/CHECKLIST.md (load whenever actually reviewing a diff; skip only when advising on process).
9. **Write findings** ranked most-severe first, each anchored to `file:line`, each carrying a severity tag. For phrasing, use assets/review-comment-templates.md.
10. **Verdict**: Approve / Approve with nits / Request changes. Approve the moment the Iron Law is satisfied — not later.

## Review order (stop-the-line priority)

| # | Level | Blocking question | Stop-the-line rule |
|---|-------|-------------------|--------------------|
| 1 | Design | Should this change exist, here, at this layer? Does it integrate sanely? | Wrong design ⇒ nothing below matters; return it now |
| 2 | Functionality | Does it do what the author intended, and is that good for users? Edge cases (empty, null, boundary, huge, unicode, clock/timezone)? Concurrency (shared state, races, TOCTOU, deadlock)? | Broken behavior ⇒ `[blocker]` before any style comment |
| 3 | Complexity | Could a simpler design do the job? Is any of this built for a future that may never come? | Speculative generality ("we might need it") ⇒ block it |
| 4 | Tests | Do tests exist for new logic? Would they fail if the code broke? Behavior asserted, not implementation? Failing-first evidence available? | New logic without meaningful tests ⇒ `[blocker]` |
| 5 | Naming | Does each name say what the thing is/does — long enough to communicate, short enough to read? | Comment, usually `[should]` or `Nit:` |
| 6 | Comments | Do comments explain WHY, not WHAT? Code-in-English comments are noise; decision rationale is gold. | `Nit:` unless a wrong comment misleads |
| 7 | Style | Matches the project's style guide? | Personal taste NEVER blocks. Prefix `Nit:` |
| 8 | Documentation | README / API docs / changelog updated if behavior changed? | `[should]` with issue link, or fix now |

## Severity protocol (numbered, citable)

1. Every finding MUST carry exactly one tag: `[blocker]`, `[should]`, or `Nit:`.
2. `[blocker]` = merging is wrong while this stands: bugs, data loss, security holes, wrong design, untested new logic. MUST include the technical reason and, where possible, a concrete failing scenario ("crashes when the list is empty because...").
3. `[should]` = fix now, OR file a tracked issue and link it in the review thread before merge. An untracked "later" is a "never".
4. `Nit:` = optional polish. The author MAY ignore it. The reviewer MUST NOT re-litigate an ignored nit.
5. Every comment MUST state a technical reason. "I don't like this" is an opinion, not a review comment.
6. Comment on the code, never the author. "This function re-reads the config on every iteration" — not "you keep re-reading the config."
7. Findings MUST be ranked most-severe first, each with `file:line`.
8. If the code confused you, the confusion IS the finding. "I couldn't follow this without reading the callee" is technical and valid — the next reader hits the same wall.
9. Deadlock between reviewer and author MUST escalate to a third maintainer. Never resolve a standoff by attrition (silence, or merging over objections).
10. Reviewers SHOULD respond within one business day, maximum. If you can't, say so and name who can.

## Size discipline

| Changed lines | Action |
|---------------|--------|
| ~100 | Ideal review unit |
| 100–400 | Normal; review as-is |
| 400–1000 | Acceptable only if one coherent concern |
| 1000+ | Request a split into stacked changes before reviewing |

Exclude generated code, lockfiles, and mechanical renames from the count — but verify they really are mechanical.

**Splitting recipe (stacked changes)**: stack by dependency — (a) pure refactor with zero behavior change, (b) the behavior change itself, (c) caller/consumer migration. Every link in the stack builds and passes tests independently, so each gets a real ~100–400-line review.

**Author obligation**: respond to EVERY comment — fix it or justify with a technical reason. Silence is neither. "Done" is a complete response to a nit.

## Receiving review (author side)

- No defensive litigation. Read the comment twice before replying once. Assume reviewer competence.
- If a comment shows the reviewer was confused, the code is unclear — fix the code, don't explain in the thread. A thread reply helps one reader once; clearer code helps every future reader.
- Verify before implementing. Reviewers can be wrong. If a comment is technically mistaken, push back with the technical reason — that is respect, not conflict.
- Address comments as separately reviewable commits or hunks so the reviewer can map each comment to its fix; don't bury ten fixes in one opaque "addressed feedback" commit.
- Never substitute performative agreement for action ("great catch!" with no code change).
- The review is of the code. Take nothing personally; extract the technical content from even a badly-worded comment.

## Self-review pass (before requesting review)

1. Read `git diff main...HEAD` top to bottom, every line, as if it were a stranger's code.
2. Remove debug leftovers: `git diff main...HEAD | grep -nE "console\.log|debugger|dbg!|binding\.pry|pdb\.set_trace|print\("` (adapt to language). Also: commented-out code, TODOs without issue links, formatting churn in untouched lines.
3. Run the tests. Paste the output (or CI run link) into the change description.
4. Description explains WHY: the problem, the approach, alternatives rejected — not a list of files touched.
5. `git diff --stat main...HEAD` — if 1000+, split before sending, not after the reviewer asks.

## Rationalization table

| Excuse | Counter |
|--------|---------|
| "It's small, I'll just skim it" | Small changes ship bugs too. Read every line or decline the review. |
| "The author is senior; it's fine" | Seniority is not a test suite. The Iron Law has no seniority clause. |
| "Approve now, they'll fix the bug in a follow-up" | A known bug at approval time is a `[blocker]`. Untracked later = never. |
| "Blocking on naming feels rude" | Nits don't block. Rude is letting confusing code ship. Say it, tag `Nit:`, approve. |
| "It works, so the design debate is moot" | Working code at the wrong layer is tomorrow's rewrite. Design is level 1 for a reason. |
| "We might need this abstraction later" | Speculative generality. Build it when 'later' arrives. Block it now. |
| "Tests would double the size of this PR" | Then untested logic ships silently broken. New logic without tests is a `[blocker]`. |
| "The reviewer just didn't get it" (author) | A competent reader didn't get it ⇒ the code failed at its one job. Fix the code. |
| "It's 1200 lines but it's all one feature" | Features split: refactor / core change / caller migration. Stacked changes exist for exactly this. |
| "I reviewed yesterday's version already" | Review the interdiff at minimum. New lines are unreviewed lines. |
| "CI is green, what else is there" | CI checks what tests assert. It does not check design, complexity, or what tests forgot. |

## Red flags — stop, that's rationalization

- You are about to approve a diff you did not fully read.
- Your comment starts with "I would have..." — that's preference. Tag it `Nit:` or delete it.
- You wrote five style comments and zero functionality comments on a 500-line change. You polished; you didn't review.
- The author answered your `[blocker]` with an explanation and no code change — and you're tempted to drop it to avoid friction.
- The change description is empty and you started reading code anyway.
- You're reviewing from memory of the author's intent ("I know why we did this") instead of the diff in front of you.
- (Author) You're typing a paragraph defending confusing code instead of spending 30 seconds renaming a variable.

## What NOT to do

- Never block on style or personal taste.
- Never rubber-stamp a large diff because CI is green or the author is trusted.
- Never review a 1000+ line change as-is; request the split.
- Never let a `[should]` dissolve — fix or linked issue, before merge.
- Never post a finding without a severity tag, a technical reason, and a `file:line` anchor.
- Never argue past two rounds — escalate to a third maintainer instead.

## Files

- `references/CHECKLIST.md` — full expanded per-category checklist with concrete check questions. Load when performing an actual review.
- `assets/review-comment-templates.md` — blocker/should/nit phrasing, before/after comment pairs, verdict and split-request templates. Load when writing review comments or responses.