Code review playbook

Code Review Playbook

Purpose

Review exists to catch what the author cannot see — not to prove the reviewer read the code. This skill is the reviewing discipline: understand the intent before the diff, check in risk order, classify every finding by severity, write comments the author can act on, and approve cleanly when nothing is genuinely wrong.

Metadata

• Prerequisites: codebase-onboarding if the repository is unfamiliar (a review without context produces confident noise).
• Related Skills: change-control (the standard the diff is held to), validation-and-testing (what the accompanying tests should look like), security-review-playbook (security-sensitive diffs), refactoring-playbook (verifying behavior-preservation claims), llm-integration-reliability (reviewing LLM call sites; also the origin of the silence rule).
• Owns: review methodology; severity taxonomy; review-comment craft; reviewing AI-authored code.

When to Use / When NOT to Use

Use when:

• Asked to review any change you did not write: PR, diff, patch, or an agent's output.
• Writing review comments, or deciding whether to approve or block.
• Reviewing AI/agent-generated code (a special section below).
• Calibrating how deep a review should go for a given change.

Do NOT use (load the sibling instead):

• You are the AUTHOR making the change → change-control (this skill is the mirror-image standard your diff will be held to).
• The review's focus is a trust boundary or hardening diff → security-review-playbook (co-load this one for process).
• Reviewing a design document rather than code → system-design's review checklist.
• Reviewing prompts/LLM behavior changes → evaluation-frameworks (demand the before/after score) plus prompt-and-context-engineering.

Definitions & Mental Model

• Intent: what the change claims to do — from the PR description, issue, or commit messages — established before reading the diff.
• Severity taxonomy: blocking (correctness, data loss, security, broken contract — cannot merge), important (should fix before or immediately after merge; reviewer judgment call, stated explicitly), nit (style/preference — author may ignore; always prefixed "nit:").
• The silence rule: a review that finds no genuine issues says so and approves. Inventing findings to demonstrate diligence is noise with authority (the same law as llm-integration-reliability step 12: a reviewer that cannot produce silence produces noise).
• Review depth: effort proportional to blast radius — a docs typo and a payment-path change do not deserve the same hour.

Mental model: the reviewer's job is to ask "what would have to be true for this change to be wrong?" — and then check exactly those things. That question orders everything: it starts from intent (a perfect diff implementing the wrong thing is wrong), moves through the risk surface (tests and contracts fail loudest), and ends at style (almost never what makes a change wrong). A reviewer who reads diffs top-to-bottom, line-by-line, in file order is doing what a diff tool does; a reviewer who hunts the specific ways this change could fail is doing what only a reviewer can do.

Core Methodology

1. Establish intent before opening the diff. Read the PR description, linked issue, and commit messages. Write (or state) one sentence: "This change claims to X." If you cannot, that is the first review comment — an unreviewable change is blocking on description, not on code. Then ask the altitude question: should this change exist, at this size, in this place? (A correct implementation of the wrong approach fails review at step 1, cheaply — see engineering-minimalism.)
2. Calibrate depth to blast radius (decision tree):

What does the change touch?
├─ Docs / comments / pure formatting → skim for correctness, approve.
│    Minutes, not hours.
├─ Internal logic with tests, no contract changes → standard review
│    (steps 3-6), focus on the logic and its tests.
├─ Public API / schema / config contract → standard review PLUS
│    api-and-interface-design's checklist (breaking-change analysis).
├─ Trust boundary, auth, secrets, deserialization, money path
│    → standard review PLUS security-review-playbook. Blocking bar
│      is lower: doubt = block.
└─ Generated/AI-authored code → standard review PLUS the AI-code
     protocol (step 7). Never lighter review because "the tool wrote it".

3. Review in risk order, not file order:
   1. Tests first. Do tests accompany the change (per validation-and-testing's at-least-one-check rule)? Do they test the change's actual risk, or just the happy path? Would they fail if the fix/feature were broken? A test that cannot fail is a blocking finding on a bugfix.
   2. Contracts second. Signatures, response shapes, schemas, error types, config keys: anything a consumer observes. Every contract delta must be classified additive/deprecating/breaking (api-and-interface-design step 7).
   3. Logic third. Now read the implementation — with the failure question in hand: boundaries (empty/one/many/maximum), error paths, concurrency, resource cleanup, off-by-ones. Trace one representative input through the change end to end.
   4. Style last, and lightly. Convention violations the repo actually enforces are fair; personal preference is a nit or nothing.
4. Apply the per-change-type checks:
   • Bugfix: Is the ROOT CAUSE fixed, or the symptom patched at one call site? (Ask: do sibling callers have the same bug? — change-control's decision tree.) Is there a test that reproduces the bug and now passes?
   • Feature: Are inputs validated at the trust boundary? Is the failure path designed, not just the happy path? Is anything speculative smuggled in ("while I was here...")?
   • Refactor: Is the commit behavior-pure? Do characterization/existing tests pass UNMODIFIED? A "refactor" that edits test expectations changed behavior — reclassify and re-review (refactoring-playbook).
   • Dependency change: Why this dep, why this constraint, is the lock file updated, does a clean install pass? (dependency-management's pre-merge checklist.)
   • Config/infra change: Precedence understood? Secrets kept out? Startup validation present? (configuration-management.)
5. Write comments the author can act on. Each comment: (a) quote or anchor the exact lines; (b) state the failure mode, not just the preference ("if items is empty this indexes past the end when...", not "this looks fragile"); (c) propose a concrete fix or ask a specific question; (d) carry its severity label. One issue per comment; no essays.
6. Verdict honestly:
   • Any blocking finding → request changes, with the blocking items enumerated at the top.
   • Only important findings → approve-with-comments or request-changes per your confidence; SAY which you chose and why.
   • Only nits or nothing → approve and say so plainly. "LGTM — I looked for X, Y, Z and found nothing" is a complete, high-value review. Never pad it.
7. Reviewing AI-authored code — the extra protocol. AI-generated changes fail differently from human ones: confidently, plausibly, and sometimes with invented APIs. Additional checks:
   • Verify claims, don't trust narration. If the description says "all tests pass," run them (or require CI evidence). If it says "fixed the root cause," apply the bugfix check yourself — AI descriptions and AI diffs can diverge.
   • Check every unfamiliar API/flag against reality (the docs, the dependency's actual version in the lockfile). Hallucinated methods compile in review and fail at runtime.
   • Look for over-delivery: unrequested abstractions, gratuitous refactors bundled with the fix, defensive code for impossible states. Scope creep is the signature AI failure — hold it to engineering-minimalism.
   • Watch for silent test weakening: assertions loosened, tests deleted or skipped to make the suite green. Diff the test files with extra care.
   • The severity bar does not move: AI authorship is not a mitigating factor for a blocking finding, nor a reason to block clean code.

Review checklist

• Intent stated in one sentence before reading the diff; altitude question asked
• Depth calibrated to blast radius (tree in step 2)
• Tests reviewed first: present, risk-targeting, capable of failing
• Contract deltas classified additive/deprecating/breaking
• One representative input traced through the logic; boundaries and error paths probed
• Per-change-type checks applied (bugfix root cause / refactor purity / etc.)
• Every comment: anchored, failure-mode-stated, fix-proposed, severity-labeled
• AI-authored code: claims verified, APIs reality-checked, scope creep and test-weakening hunted
• Verdict explicit; clean code approved without invented findings

Discovery & Audit Commands

# Establish intent and shape before the diff
git log --oneline main..HEAD          # what commits claim (adjust base branch)
git diff --stat main..HEAD            # blast radius at a glance
git diff main..HEAD -- '*test*'       # tests first: what changed in test files?

# Verify, don't trust: run the suite yourself
pytest tests/ -x                      # or: npm test / cargo test — the repo's command

# Contract-surface deltas
git diff main..HEAD -- '*.py' | grep -E "^[-+](def |class )" | head -20
git diff main..HEAD -- '*openapi*' '*schema*' '*.proto' 2>/dev/null

# Root-cause check on a bugfix: who else calls the patched function?
grep -rn "patched_function(" --include="*.py" --include="*.ts" . | grep -v node_modules

# AI-code reality checks
grep -n "the_unfamiliar_method" $(git diff --name-only main..HEAD) 2>/dev/null   # where did this API come from?
pip show the_dependency | grep -i version   # does the used API exist in THIS version? (npm ls for Node)

# Test-weakening hunt
git diff main..HEAD -- '*test*' | grep -E "^-.*assert|^\+.*skip|^\+.*xfail" | head

Failure Modes & Anti-patterns

Symptom	Mistake	Correction
40 nits, the actual bug shipped	Line-by-line reading instead of risk hunting	Risk order: tests → contracts → logic → style (step 3)
Perfect implementation of the wrong thing approved	Intent never established	One-sentence intent + altitude question before the diff (step 1)
"This looks wrong" comment war	Failure mode not stated	Anchor + failure mode + proposed fix + severity (step 5)
Author can't tell what blocks the merge	Unlabeled findings	Severity taxonomy on every comment; blocking items listed at top (steps 5-6)
Reviewer invents findings on clean code	Diligence theater	The silence rule: state what you checked, approve (step 6)
Bugfix approved, same bug in three sibling callers	Symptom patch not caught	Root-cause check: grep the patched function's callers (step 4)
"Refactor" merged with edited test expectations	Behavior change smuggled past review	Refactor purity check: tests pass UNMODIFIED (step 4)
AI PR merged with a hallucinated API call	Narration trusted over verification	Run the tests; reality-check unfamiliar APIs against the lockfile version (step 7)
Suite "fixed" by loosened assertions	Test diffs skimmed	Diff test files with extra care; deleted/weakened assertions are blocking (step 7)
Two-line docs fix waits three days for deep review	Depth not calibrated	Blast-radius tree; skim-and-approve is a valid review (step 2)

Worked Example

Task: review a PR titled "fix: handle missing user email in invoice sender".

1. Intent: "When a user has no email, invoice sending crashes; this makes it skip and log instead." Clear. Altitude: reasonable fix, right place — proceed.
2. Depth: internal logic + one log line; standard review.
3. Tests first: one new test: test_invoice_skips_user_without_email. Good — but it asserts only that no exception is raised, not that a skip was recorded. Comment (important): "Assert the skip is observable (log/counter), or a future refactor can silently drop these users — see observability-and-diagnostics on surfaced skips."
4. Contracts: none changed. ✓
5. Logic + root-cause check: the guard was added in send_invoice(). Grep shows send_receipt() and send_reminder() call the same user.email unguarded — the same crash exists there. Comment (blocking): "Root cause is shared: notify.deliver() is the common path all three route through; one guard there fixes the class. As written, two sibling crashes remain — change-control's fix-placement tree points at the producer."
6. Verdict: request changes; blocking item on top, important item second, zero nits (the code style was fine, so nothing was said about style).

The revised PR guards in notify.deliver(), tests the observable skip, and is approved with: "LGTM — checked sibling callers, error path, and test failure capability; all clean."

Repository Examples

Repo facts below are point-in-time illustrations (as of 2026-07-04) — examples, never assumptions about your system.

• prism (~/prism) — an automated reviewer built on this skill's core laws: it reviews only genuine bugs/security issues (scope constrained in the prompt), posts NOTHING when there is nothing to say (the silence rule as architecture), and is idempotent on re-review (deletes its own prior comments) — evidence that "no findings" is a designed, first-class outcome, not reviewer laziness.
• prism's own history — the reviewable-change pattern from the author's side: commit 51b92c9 (hardening) followed by b011239 (tests locking it in) is exactly what step 3 looks for — a change that arrives with the test that would fail if it regressed.

Validation Exercise (any repository): take the most recent merged PR you did not write; without reading its review thread, apply steps 1–6 and write your own review; then compare with what the actual reviewers caught. Divergence in blocking findings — theirs or yours — is the calibration signal.

Validation Criteria

You applied this skill correctly when:

1. Your review states the change's intent in one sentence, and your findings connect to it.
2. Every finding carries a severity label, an anchor, a failure mode, and a proposed fix.
3. For a bugfix you can name the sibling callers you checked; for a refactor, you verified tests passed unmodified.
4. You ran (or demanded evidence of) the test suite rather than trusting the description — always, for AI-authored changes.
5. Clean code got a plain approval listing what you checked — with zero invented findings.
6. The time spent was visibly proportional to the blast radius.

Provenance & Maintenance

• Sources: ~/prism — investigated 2026-07-04. The AI-code review protocol reflects observed failure patterns of 2026-era code-generation models (Hypothesis for future generations — recalibrate as models change). Skill authored 2026-07-06; methodology is repo-independent.
• Assumptions: the severity taxonomy (blocking/important/nit) is a common convention; teams with existing labels should map, not replace.
• Re-verification commands:

  git -C ~/prism log --oneline | head -5
  grep -rn "genuine\|no issues" ~/prism/prism 2>/dev/null | head   # the silence-rule prompt scope
  

• Likely to drift: AI-authored-code failure signatures (model-generation-dependent); the example repo's reviewer prompt.
• Maintenance checklist:
  • Re-run re-verification; re-stamp Repository Examples.
  • Re-validate the AI-code protocol against current model failure modes annually.
  • Confirm cross-referenced skills still exist under their directory names.