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. ```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.