PR Review

A useful review finds the bugs that tests missed, checks that the change does what was asked, and says clearly whether it can merge. It does not restate the diff or nitpick formatting that a linter should own.

Step 1 — Gather context (five minutes, always)

gh pr view <num> --json title,body,baseRefName,headRefName,files,additions,deletions
gh pr diff <num>            # or: git diff origin/main...HEAD
gh pr checks <num>          # CI status
git log origin/main..HEAD --oneline

• Read the PR description and the linked issue or spec. Write down in one sentence what the change is supposed to do. If you cannot, ask before reviewing.
• Note the size. Over ~400 changed lines, review in logical chunks (by commit or by directory) and say so in the summary.
• Identify the risky files: auth, billing, migrations, public API, concurrency, anything with delete or rm.

Step 2 — Two-axis review

Review on two independent axes and label every comment with its axis.

Axis A — Does it do what was asked?

• Every requirement in the spec has corresponding code and a test.
• Nothing extra was added that the spec did not ask for (scope creep is a review finding).
• Edge cases from the spec (empty, max, unauthorised, concurrent) are handled.

Axis B — Is the code sound?

• Correctness: off-by-one, null/undefined paths, error handling that swallows failures, race conditions, timezone and encoding assumptions.
• Security: input validation at boundaries, injection (SQL, shell, HTML), authz checks on every new endpoint or action, secrets in code or logs, unsafe deserialisation, new dependencies and their versions.
• Data: migrations reversible and lock-safe, backwards compatibility of API shapes and serialised formats.
• Tests: do they fail if the behaviour breaks? Look for tests that only assert the mock was called, or that were skipped or loosened.
• Maintainability: naming, function size, duplication, dead code, comments that explain why.
• Performance: N+1 queries, unbounded loops over user data, work inside render paths, missing indexes for new queries.

For anything non-trivial, run it: check out the branch, run the suite, and exercise the change directly. Reviewing by reading alone misses runtime issues.

Step 3 — Rank and write comments

Use exactly these severities:

• blocker — incorrect behaviour, security issue, data loss risk, or breaks the spec. Must fix before merge.
• should — meaningful quality or maintainability problem; fix now or file a follow-up with a link.
• nit — stylistic; author may ignore. Prefix with nit:.
• question — you need information to judge.
• praise — something done well that others should copy. Use sparingly and specifically.

Each comment: file and line, the problem, why it matters, and a concrete suggestion (code when it is short). One issue per comment.

Step 4 — Verdict

End with one of:

• Approve — no blockers, at most a few shoulds you trust the author to handle.
• Approve with changes — list the required shoulds.
• Request changes — list the blockers.

Post with gh pr review <num> --approve|--request-changes --body-file review.md if asked; otherwise return the markdown.

Output format

## Summary
What the PR does (1–2 sentences). Size and risk level. Whether I ran it.

## Blockers
1. `src/billing/refund.ts:42` — Refund amount is not clamped to the original charge; a crafted request can refund more than paid. Suggest: `Math.min(amount, charge.amount)` plus a test.

## Should
...

## Nits
...

## Questions
...

## Verdict: Request changes

Checklist for the reviewer

• I read the spec/issue, not just the diff.
• I looked at test files as carefully as production files.
• I checked new endpoints/actions for authentication and authorisation.
• I ran the tests or explained why I could not.
• Every blocker has a suggested fix.
• I did not comment on things a formatter or linter enforces.

Pitfalls

• Do not approve because CI is green; CI proves the tests that exist pass, not that the right tests exist.
• Do not rewrite the author's design in the comments; if the approach is wrong, say so once at the top and ask for a conversation.
• Large diffs hide bugs. Ask for a split rather than skimming.
• Avoid "LGTM" with no evidence. Even an approval should say what you checked.
• Generated files, lockfiles and vendored code get a glance for surprises, not a line-by-line read.