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.