Code Review Checklist

A priority-ordered review of a diff. Catch the things that hurt in production; let the linter catch the rest. Save the reviewer's attention for what only a human can see.

When to use

• Reviewing someone else's PR
• Self-reviewing before submitting your own PR (one pass before the team sees it catches half the embarrassment)
• Building a team's code review standard

Skip for: trivial changes (typo fixes, dependency bumps) — review checklists are overkill; merge it and move on. Pure stylistic preferences — that's what auto-formatters are for.

Workflow (priority-ordered)

1. Correctness first. Before anything else: does this code do what the PR description claims? Read the diff with the PR title as the lens. Find one test or one example that exercises the change. If the test is missing AND the code path is non-trivial, that's the first comment.

2. Boundary conditions. Five questions, in order:

   • Empty case: what happens when the input is [] / "" / None / 0?
   • Single-element: any branch that assumes "at least 2"?
   • Max case: pagination, timeouts, large inputs?
   • Concurrent case: if two requests/processes hit this at once, what happens?
   • Error path: when the underlying call fails, what does the user see?

3. Security touch. Three quick checks:

   • Input → trust boundary: does user input flow into a SQL query (parameterized?), a shell command (escaped?), a file path (path-traversal-safe?), an HTML template (escaped or sanitized?)
   • Auth: is there a new endpoint? Does it require auth? Does the route docstring match the actual gate?
   • Logging: are we logging secrets, tokens, or PII? print(api_key) belongs in no PR.

4. Testability. Two questions:

   • Is the new code covered by ANY test? If not, what change would make it testable?
   • Are the existing tests still meaningful, or did the diff change the code under them in a way that makes them tautological?

5. Reversibility. Some changes are hard to undo: migrations, public API shapes, externally-visible URLs, deprecation notices. Treat those with extra scrutiny — once they ship, they stay.

6. Performance, only if relevant. If the changed code is on a hot path (request handler, data pipeline, render loop), check for N+1 patterns and unbounded loops. If it's not a hot path, skip this step.

7. Trivia, LAST and OPTIONAL. Naming, comments, slight readability. Cap yourself at 2-3 trivia notes — beyond that, you're nitpicking, not reviewing.

Output shape

Comments go on the diff. The summary, if written, should follow this shape:

**Recommendation**: approve / request changes / blocking concern

**Load-bearing comments** (must address before merge)
- <comment>

**Suggestions** (think about, not blocking)
- <comment>

**Trivia** (optional, not blocking)
- <comment>

The three-tier shape signals priority. A 30-comment review with no signal of which matter erodes trust — the next time the reviewee sees a 30-comment review they'll skim, even when it matters.

Definition of done

• Correctness check made (does the code do what the PR claims)
• Boundary cases reviewed (at least empty + error path)
• Security touch made (no new auth gap, no secret-logging)
• Test coverage assessed (named gap if any)
• Tier-labeled comments (load-bearing / suggestion / trivia)

Gotchas

1. Don't review style. The formatter reviews style. Every minute spent on "missing trailing newline" is a minute not spent on the SQL injection three lines down. If your team doesn't have an auto-formatter, install one — the review savings pay for themselves in a week.

2. Don't review the design in a PR. If the design is wrong, that's a design conversation BEFORE the PR. Reviewing a 800-line PR with "I think we should have approached this differently" is too late and disrespectful. Catch design earlier; review code at code time.

3. "Could this be simpler?" needs an example. A reviewer who says "this could be simpler" without offering the simpler version is making the reviewee re-invent. Suggest the simpler form OR move on.

4. Comments without solutions are demoralizing. "This is bad" → bad comment. "This breaks when X is empty; can we add a guard?" → useful comment. Specify the failure case AND a path to fix.

5. A reviewer who agrees with everything has stopped reviewing. If you find nothing to push on, double-check. Some PRs are genuinely clean, but most have one thing worth flagging.

6. A reviewer who pushes on everything has lost calibration. If every PR has 20 comments, the reviewee will start skimming. Trade-off: catch the right things, not all the things.

7. Beware "I would have written this differently." Sometimes that's worth saying; usually it isn't. The reviewer's job is to verify the change works, not to rewrite it.

8. Don't review what the diff doesn't change. If the existing code is bad and the PR doesn't make it worse, leave it. Drive-by demands ("while you're here, can you also fix X") expand scope unfairly.