code-review

---
name: code-review
description: Review a pull request the way a good open-source maintainer does. Use when reviewing an incoming PR, when the user asks "review this PR" or "is this ready to merge", when triaging a stale PR queue, or when writing review feedback to an external contributor. Covers what to check and in what order, severity levels, phrasing feedback so people come back, handling large or unwanted PRs, first-time contributors, AI-generated submissions, and closing PRs kindly. Complements the built-in /code-review command by adding the maintainer and community dimension.
---

# Maintainer-Grade Code Review

An open-source review does two things at once: it protects the codebase, and it
decides whether this person contributes again. Most reviews are competent at the first
and terrible at the second.

## Read in this order

Never start at the diff. Start at intent.

1. **The linked issue.** Was this problem agreed to be worth solving? A perfect
   implementation of an unwanted feature must be closed, and it is far kinder to
   discover that in the first minute than the fortieth.
2. **The PR description.** What does the author claim it does?
3. **The tests.** They tell you what the author believes the behavior is. Read them
   before the implementation — you will review the implementation more honestly.
4. **The public API diff.** New exports, changed signatures, changed defaults, changed
   error types. This is where irreversible mistakes live (see `api-design`).
5. **The implementation.**
6. **What's missing** — docs, changelog entry, migration note, the test for the error
   path they did not think about.

```bash
gh pr checkout 123
gh pr diff 123 --name-only          # scope at a glance
gh pr view 123 --json title,body,additions,deletions,files,author
make test                            # run it locally; CI green is necessary, not sufficient
```

## Severity, stated explicitly

Unlabeled feedback makes contributors guess which comments block the merge. Prefix
every comment:

| Prefix | Meaning | Author must |
|---|---|---|
| **blocking:** | Must change before merge | Fix it |
| **question:** | I don't understand yet | Answer; may resolve to nothing |
| **suggestion:** | Better, but your call | Decide |
| **nit:** | Style/taste, non-blocking | Ignore freely |
| **praise:** | This is good | Nothing — but say it anyway |

Reserve **blocking** for: correctness bugs, security issues, data loss, breaking
changes without a migration path, missing tests on new behavior, and public API
mistakes. Everything else is a suggestion. A review where every comment is blocking is
a rewrite request wearing a costume.

Use `praise:` genuinely at least once per review when it is warranted. It is not
decoration — it tells the contributor which instincts to repeat.

## What actually deserves scrutiny

Ranked by cost-if-wrong:

1. **Security** — injection, path traversal, deserialization, secrets in code, new
   network calls, new dependencies with install scripts. See `supply-chain-security`.
2. **Public API changes** — a mistake here is permanent for a major version.
3. **Data loss and corruption** — migrations, destructive defaults, cache invalidation.
4. **Correctness on boundaries** — empty, null, concurrent, unicode, huge.
5. **Backward compatibility** — will this break a user who upgrades without reading?
6. **Performance in hot paths only.** Do not micro-optimize cold code in review.
7. **Test quality** — do the tests fail if the fix is reverted? Check this literally
   for bug fixes; a surprising number of "regression tests" pass on the broken code.
8. **Readability** — the next reader is a stranger.

Delegate everything mechanical to tooling. Formatting, import order, lint rules and
typos should be enforced by CI, never by a human comment. If you find yourself typing
a style comment, the correct fix is a linter rule and a follow-up PR — otherwise
you will type it again next month, to someone else.

## Phrasing that keeps contributors

The same technical content lands completely differently depending on framing.

| Instead of | Write |
|---|---|
| "This is wrong." | "This breaks when `items` is empty — line 42 indexes `[0]`." |
| "Why did you do it this way?" | "What led you to the recursive approach here? I want to make sure I'm not missing a constraint." |
| "Use a map." | "suggestion: a map keyed by id would drop this to O(n). Not blocking." |
| "Needs tests." | "blocking: could you add a test for the empty-input case? `tests/parser_test.py` has a similar pattern at line 88." |
| "Rewrite this." | "This is a bigger change than I can review in one pass — could we split the refactor into its own PR and keep this one to the fix?" |

Principles:

- **Critique the code, never the person.** "This function does X" not "you did X".
- **Explain the why.** A rule without a reason will be violated again.
- **Point at the example.** Link to a file and line where the codebase already does it
  right. This converts a correction into a five-second fix.
- **Ask, don't assume.** They may know a constraint you don't.
- **Own project-specific weirdness.** "We do it this odd way because of #412 — sorry,
  it's not documented; I'll fix that."
- **One round of feedback, batched.** Drip-feeding comments over five days across
  three review rounds is the top cause of abandoned PRs. Read the whole thing, then
  submit once.

## Speed is a feature

Response latency matters more to contributor retention than review depth.

- **Acknowledge within 48 hours**, even if it is "thanks — I need a week to review
  this properly." Silence reads as rejection.
- **A first-time contributor's PR gets priority.** They are deciding whether this
  community is worth their time, and you get exactly one chance.
- **State your cadence** in `CONTRIBUTING.md` if you are slow. Honest expectations beat
  optimistic silence.
- **Merge small good PRs immediately.** Every day open is a day it can rot into a
  merge conflict, and a day the author's context evaporates.

## Hard cases

**The 3,000-line unsolicited PR.** Do not review it line by line — you will burn a
weekend on something you may close. Respond at the design level within two days:
thank them, state clearly whether the *direction* is acceptable, and if it is, ask for
a split into reviewable pieces. If it is not, say so immediately and explain why. The
real failure here was upstream — add "open an issue before large changes" to
`CONTRIBUTING.md` so the next person doesn't lose a weekend.

**Right change, wrong implementation.** Merge-and-fix beats a long review cycle when
the author is unlikely to iterate. Merge to a branch, do the cleanup yourself, credit
them with `Co-authored-by:`. Their name stays on the contribution, which is the part
that actually matters to them.

**Drive-by contributor who disappears.** Give it two weeks and a ping. Then either
finish it yourself with co-authorship, or close with "happy to reopen if you come
back." Do not let it sit for a year.

**AI-generated PRs.** Increasingly common, and the tell is code that is plausible but
untethered — invented APIs, tests that assert nothing, a description that restates the
diff. Review by the same standard as any other PR; do not accuse anyone. But do
require that the author can explain the change, and be firm that CI green plus a
passing test they cannot describe is not sufficient. State a policy in
`CONTRIBUTING.md` if volume becomes a problem: disclosure is fine, unreviewed
generation is not.

**Contributor pushes back on blocking feedback.** Restate the constraint, ask what you
are missing, and if you still disagree, decide — you are the maintainer. Then say so
plainly and without hedging: "I hear you, but I'm going to keep this as-is because X.
I appreciate the argument." Ambiguity here is crueler than a clear no.

## Closing a PR kindly

Some PRs must be closed. Doing it well preserves the contributor.

```markdown
Thanks for putting this together, and sorry for the outcome.

I'm going to close this — <specific reason: out of scope / conflicts with the
plugin design in #412 / adds a dependency we can't take on>.

That's a gap in our docs rather than a mistake on your part; I've opened #520 to
document the scope boundary so this is clearer next time.

If you're up for it, #445 is a close relative and I'd merge that quickly.
```

Elements that matter: thank them first, give a *specific* reason, take responsibility
for anything that was your project's fault, and offer a concrete next thing. Never
close silently, and never let it expire into a stale-bot close — that is the version
contributors remember and post about.

## The review checklist

```
[ ] Linked issue exists and the change was wanted
[ ] Scope matches the description — no unrelated drive-by changes
[ ] Tests added; they fail without the fix
[ ] Public API changes are intentional and documented
[ ] Backward compatible, or has a migration path
[ ] Error paths handled with actionable messages
[ ] No secrets, no new unvetted dependencies
[ ] Docs and changelog updated
[ ] CI green, and I ran it locally
[ ] Every comment is prefixed with a severity
[ ] Something specific praised
```

## Anti-patterns

- **Silence.** The single most common way projects lose contributors.
- **Nitpicking a first PR to death.** Merge it and fix the nits yourself.
- **Style comments a linter should make.**
- **"LGTM" on 800 lines.** Either review it or say you only skimmed it.
- **Requiring changes without saying which comments block.**
- **Rewriting their PR in review comments** instead of pairing or taking it over.
- **Reviewing only the diff**, never the surrounding code the diff assumes.
- **Stale-bot closing human PRs.** Automate reminders, not rejections.