Code Review

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.

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:

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.

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.

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.