| name | code-review |
| description | Self-review, PR review, and review-response protocols. Invoke proactively before any commit, push, or PR action; when reviewing someone else's diff; and when responding to review feedback. |
Code Review
Three modes: self-review (before you commit/push), PR review (when reviewing someone else's diff), and receiving review (when feedback comes back on your own PR).
Self-Review Gate
Run before every commit or PR. The goal is catching the things that are obvious in retrospect but invisible when you're inside the problem.
Start by reading the full diff:
git diff --staged
git diff main...HEAD
Then work through these seven checks in order:
1. Correctness
- Does this actually solve the stated problem?
- What happens on the error path — handled or propagated?
- Any assumptions that aren't validated at the boundary?
2. Tests
- Does new behavior have a test?
- Would the test actually fail before this change and pass after?
- Edge cases covered: empty input, null/undefined, concurrent writes, large data?
- No test = undocumented assumption. Write it or open a tracking issue.
3. Security
- Any user input that reaches a database, shell, file path, or HTML? Validate it.
- Any new secrets or API keys? Must be in env, never in code, never in logs.
- Auth/authz: does this expose a resource to the wrong caller?
- If touching payment, PII, session tokens, or cryptography: flag for a dedicated security review.
4. Design
- Is this the simplest thing that could work?
- Any new abstraction with only one consumer? Probably premature.
- Any function longer than ~20 lines? Can it be named helpers?
- Any copy-paste from elsewhere? Extract it.
5. Naming and Readability
- Would a teammate understand this diff in 6 months without asking you?
- Functions named as verbs, variables as nouns?
- Any magic numbers without a named constant?
6. Side Effects
- Any global state mutation that could surprise callers?
- Any resource (file handle, DB connection, timer) opened but not closed?
- Any
console.log, debugger, or stray TODO comment left behind?
- Any async call without error handling?
7. Diff Size
- More than 400 lines changed? Is this actually one logical change?
- Refactoring mixed with feature work? Split them into separate commits or PRs.
- Large diffs get reviewed worse — splitting is a quality decision, not a bureaucratic one.
If a check surfaces a real issue: fix it before committing. If it's an intentional trade-off or known debt, document it inline with // DEBT: and a brief explanation.
Ship / Show / Ask
Before opening a PR, decide which track this change belongs on:
| Track | What | When |
|---|
| Ship | Merge directly to main (or self-approve) | Typo fix, trivial docs, routine chore with no logic change |
| Show | Open PR, merge immediately, notify team | New feature following accepted design; want async eyes but don't need a gate |
| Ask | Open PR, wait for approval before merging | Novel approach, architectural decision, breaking change, security-sensitive code |
When in doubt: Ask. The cost of a held PR is low; the cost of merging a subtle design mistake is high.
PR Description
A PR description answers three questions for the reviewer:
- What — what changed? (one sentence, not "see diff")
- Why — what problem does this solve? Link the issue.
- How to verify — what should the reviewer run or look at?
## What
<one-sentence summary>
## Why
Closes #<issue> / <brief motivation if no issue>
## How to verify
- [ ] <specific test command or manual step>
- [ ] <regression check, if relevant>
Add screenshots for UI changes. Add before/after numbers for performance changes. Add migration notes for schema or API changes.
Reviewing Someone Else's Code
Your job: help the author ship good code, not protect yourself from blame.
Read the PR description first. If it's missing or unclear, ask before diving into the diff — a vague description is a signal the change isn't ready.
Use the same 7 checks as self-review (above). Focus comments on things that:
- Could cause a bug in production
- Will be hard to change later (API surface, schema, public interface)
- A test would have caught
What NOT to block on
- Code style (that's for linters and formatters)
- Personal preference when both approaches are reasonable
- Hypothetical future requirements ("what if we need X later?")
- Anything you'd accept if you'd written it yourself
Comment tone
Use explicit labels so the author knows what's required:
- Blocking: "This will panic on nil input when the user hasn't set a config — must fix before merge"
- Suggestion: "Consider extracting this loop to
filterActiveUsers() — easier to test in isolation"
- Nitpick: "nit:
userList → users (shorter, idiomatic)"
Authors resolve nitpicks at their discretion. Blockers must be addressed. Suggestions are invitations, not mandates.
LGTM discipline
Don't approve to be polite. But don't hold up a PR over nitpicks either. Approve with comments for small things that don't block correctness.
Definition of "good enough to merge": is this better than what exists, and does it not make things measurably worse?
Receiving Review on Your Own Code
Feedback comes back — from a human reviewer, a subagent, or a linter bot. Your job now flips: not "defend the diff" and not "agree to every ask". Technical correctness beats social comfort.
Work the six phases in order. Do not skip to IMPLEMENT — doing so is how partial fixes, performative agreement, and regressions happen.
1. READ — absorb before reacting
Read every comment first. Don't start editing on comment #1 while #5 contradicts it. Group related comments so you see the shape of the feedback, not just its pieces.
2. UNDERSTAND — restate or ask
For each item, either:
- Restate the requirement in your own words (internally or in the reply), or
- Ask one clarifying question if you can't.
Do not guess. A guess implemented is a guess the reviewer has to catch again on round two.
3. VERIFY — check against the actual code
The reviewer may be wrong about what the code does. Before accepting:
- Open the file they referenced. Confirm the behavior they're describing.
- Check for context they didn't have (surrounding code, existing tests, related PR, prior decision).
If the reviewer is mistaken about what exists: that's push-back territory, not accept.
4. EVALUATE — accept / push back / defer
For each item, pick one:
| Decision | When |
|---|
| Accept | Feedback is technically correct for this codebase; fix doesn't break existing behavior; the code actually needs it (not speculative YAGNI) |
| Push back | Reviewer misunderstands the codebase, conflicts with a settled architectural decision, or adds unused abstractions — reply with technical reasoning and reference (file/line/decision doc) |
| Defer | Unclear, needs investigation to verify, or bundles with other items that must be understood together — mark, don't implement yet, return in step 2 |
Internal review (teammate, team convention) is trusted but still clarified. External review (bot, third-party) is verified more skeptically — it doesn't have the project's context.
5. RESPOND — batch before you touch code
Post replies for all items first. Structure: accepted (brief ack), pushed back (with reasoning), deferred (with specific question).
Never: You're absolutely right! Fixing now. followed by a half-wrong fix. Performative agreement is worse than disagreement — it hides the real state.
6. IMPLEMENT — one change at a time
Only now touch the code:
- Work items one at a time. Do not batch-apply a sweep of suggested edits.
- Run the relevant test after each fix, before moving to the next. A "small change" that breaks a test is the usual cause of round-three reviews.
- If a fix reveals the push-back was right after all, stop and say so — don't quietly un-do it.
- When all items are addressed, reply with what changed per thread (or resolve threads with the convention the reviewer uses).
Anti-patterns
| Anti-pattern | Why it fails |
|---|
You're absolutely right! + immediate edit | Performative agreement — often followed by a wrong fix because UNDERSTAND was skipped |
| Implementing comment-by-comment top-to-bottom | Later comments often override earlier ones; wastes work |
| Accepting every suggestion to end the review faster | Adds unused abstractions, bloats diff, invites round two |
| Pushing back without a reference | "I don't think so" ≠ "see docs/design-docs/X.md §2 — we decided against this in March" |
| Batched implementation, no per-fix test | One bad edit in a stack of ten is invisible until CI fails |
| Silent re-revert | Fix introduced a regression, you reverted it without telling the reviewer — they'll re-comment next round |
