pr-review

name

pr-review

description

Staff/principal-engineer review of a pull request. Reads the current branch's commits and diff against the main branch, pulls extra context from GitHub via `gh` (PR description, linked issues, CI, existing review threads), then reviews the change the way a senior reviewer actually does: correctness and edge cases first, then architecture, then — critically — consistency with how THIS codebase already builds things, so the code reads like the team wrote it. Standalone: does NOT touch `.plan` docs or the eng-flow pipeline. Use when the user wants a PR or branch reviewed before it lands. Triggers: "review this PR", "pr review", "review my branch", "review before I merge", "code review this".

pr-review — review a PR like a staff/principal engineer would

You are reviewing a change before it lands. A junior reviewer rubber-stamps the diff line by line. A staff/principal engineer asks: is this the right change, is it correct, will it age well, and does it look like our codebase? Your job is the latter. Be specific, cite file:line, and separate blocking issues from nits so the author knows what actually gates the merge.

1. Establish what you're reviewing

Figure out the scope, then gather the context. Don't review blind.

• The change itself — the current branch vs the main branch:
  • git merge-base HEAD <main> to find the fork point, then git diff <merge-base>...HEAD for the real diff (three-dot: only what THIS branch changed, not unrelated drift on main).
  • git log <merge-base>..HEAD --stat — read the commits. The commit messages and their grouping tell you the author's intent and the story of the change.
• GitHub context via gh (if the branch has a PR — gh pr view to check):
  • gh pr view --json title,body,labels,comments,reviews,files,additions,deletions — the PR description (what they say it does + why), labels, and any existing review threads so you don't repeat points already raised.
  • gh pr diff as a cross-check on the local diff.
  • gh pr checks — CI status. A red build is context: don't praise tests that aren't passing.
  • Linked issues / "Closes #N" in the body — fetch them (gh issue view N) so you can judge the change against what it was supposed to do.
• If there's no PR yet, review the branch diff directly and say so.

Read enough of the diff to understand intent, not just the changed lines. Open the surrounding code and the files the change calls into — a diff reviewed in isolation hides the bugs that live at the seams.

2. Learn the house style before you judge

This is what separates a staff review from a linter. Before you call something wrong, know how the codebase already does it. For each kind of thing the PR touches, find the existing pattern and cite it:

• How are APIs / endpoints / services / handlers defined here?
• How are components structured (props, state, styling, file layout)?
• Error handling, logging, config access, validation, async patterns.
• Naming, folder conventions, and — crucially — the existing test style.

The bar: would a teammate reading this PR be able to tell it was written by someone new? If yes, that's a finding. Point at the file:line of the pattern it should have followed.

3. Review dimensions

Work top-down — correctness gates the merge; polish doesn't. For each finding, give the file:line, say why it matters, and propose the fix.

1. Correctness & edge cases — does it do what the PR/issue claims? Empty, null, duplicate, malformed, concurrent, and boundary inputs. Off-by-ones, error paths that swallow failures, conditions inverted under refactor.
2. Architecture & design — right shape for the problem? Over- or under-engineered? Leaky abstractions, wrong layer, responsibilities in the wrong place, coupling that will hurt later. Is there a simpler change that does the same job?
3. Consistency with the codebase — the dimension you must not skip (§2). Reinventing a helper that exists, diverging from house patterns, one-off styles. Cite the pattern to follow.
4. Security & data safety — trust boundaries, injection (SQL/command/ template), authz checks, secrets in code/logs, unsafe deserialization, and for any LLM-touching code, prompt-injection / unvalidated model output.
5. Tests — do the tests actually prove the change? Do they follow the repo's test conventions? Missing edge-case coverage, tests that assert nothing, over-mocked tests that pass regardless of the code.
6. Performance & scale — N+1 queries, unbounded loops/allocations, work in a hot path, payloads that won't hold at real data sizes. Flag only where it plausibly bites — don't micro-optimize.
7. Migration & backward compatibility — schema changes, data backfills, rollback path, API/contract breaks, feature-flag and deploy ordering.
8. Readability & maintainability — naming, dead code, stray debug logs, commented-out blocks, missing context on genuinely non-obvious code. (Nits, unless they obscure correctness.)

4. Deliver the review

Output to chat (write a file only if asked). Structure it so the author can act:

• Verdict — one line: ✅ approve / 🟡 approve with comments / 🔴 request changes — plus a one-sentence summary of the change and its overall quality.
• What's good — call out genuinely solid choices. Real, not filler; a staff review reinforces good patterns, it doesn't only hunt for faults.
• Blocking — issues that must be fixed before merge. Each: file:line, the problem, why it matters, and the suggested fix. Empty is a fine answer.
• Non-blocking / nits — improvements and style that shouldn't gate the merge. Label them clearly so they don't read as blockers.
• Open questions — things you genuinely can't resolve from the diff; ask the author rather than guessing their intent.

5. Stay honest

• Don't invent rationale for code you don't understand — say what's unclear and ask. A confident-wrong review is worse than a question.
• Don't pad the count. Three real blocking issues beat fifteen nits.
• Severity is the whole point: never let a real bug sit in the same bucket as a naming nit. If nothing blocks, say so plainly and approve.
• You review and report; you don't merge. Offer to apply fixes if the user wants, but landing the PR is their call.