pr-review-rubric

name	pr-review-rubric
description	Staff-level PR review rubric — correctness, contract, clarity, tests, blast radius. Use when reviewing code (yours or others') before merge.

PR review rubric

What a staff-plus engineer looks at when reviewing.

1. Understand before commenting

• Read the PR description. If there isn't one, that's the first problem.
• Skim commits. A clean story of atomic commits tells you what the author was thinking.
• Read the tests first — they describe the contract.
• Now read the code.

If you can't state what this PR does in one sentence in your own words, you can't review it yet.

2. Correctness

• Does the happy path do what the description claims?
• What are the boundary inputs? (empty, huge, zero, negative, unicode, null, undefined, missing)
• What about concurrency? Two callers at once. Retries. Cancellation.
• Error paths — is every throw handled, and is the handler the right place?
• Off-by-one, inclusive/exclusive range, integer overflow on counters.

3. Contract

• Public API change? Add-only? Breaking?
• Response shape change — who depends on the old shape?
• Database schema change — is the migration safe under traffic?
• Removed a function — who called it? git grep before approving the delete.

4. Clarity

• Names convey intent. getUser returns a user. getUserOrDefault returns a user or a default. resolve explains nothing.
• Control flow reads top-to-bottom with minimal branching surprise.
• The obvious case isn't buried under the edge case.
• Comments explain why, not what. If there's a non-obvious constraint, it's documented.

5. Tests

• Do they assert behavior the user cares about, or implementation detail?
• Would they catch the next regression of this code?
• Are the failing messages useful, or "expected true got false"?
• Are there tests for every new branch?
• Do the tests actually run? (Yes, this gets missed.)

6. Blast radius

• What else does this code touch or get touched by?
• Are those callers tested, or are we relying on this PR's tests alone?
• Is there a way to stage the rollout?

7. Security

• Only flag real risks. "You could add validation here" is noise unless there's an exploitable surface.
• If this touches authn/authz, data egress, or user-controlled strings → apply the security checklist.

8. Perf

• Loops inside loops on user data. Queries inside loops. New deps that balloon the bundle.
• Do not optimize on feel. If you flag perf, propose a measurement.

Severity

Every comment has a severity:

• Blocker — must fix before merge
• Important — should fix, call out if intentional
• Nit — optional polish
• Question — explain the choice

Pick the right severity. "Blocker" for whitespace is how people learn to ignore you.

Respect the author

• Point at the problem. Let them choose the fix.
• Don't rewrite the PR in the comments.
• Ask before assuming incompetence — sometimes there's context you don't have.
• Approve with nits is a valid verdict. Don't chain-block on trivia.