| 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.
- 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.
- 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?
- 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.
- 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.
- 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.
- 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.
- Migration & backward compatibility — schema changes, data backfills,
rollback path, API/contract breaks, feature-flag and deploy ordering.
- 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.
