Review Pull Request
Mode: $ARGUMENTS
If mode is one of the following, adjust the review:
- BUGS: Focus only on logical or other bugs
- SECURITY: Focus only on security issues
- PERFORMANCE: Focus only on performance issues
Approval standard
Approve when the change definitely improves overall code health, even if it isn't perfect. Perfect code doesn't exist — don't block a change because it isn't how you would have written it. If it improves the codebase and follows its conventions, approve.
If the change is too large to review well (~1000+ lines), asking the author to split it is a valid review outcome. Suggest a strategy: stack (small change, next one based on it), by file group, horizontal (shared code first, then consumers), or vertical (one end-to-end slice per PR).
Workflow
- Analyze the diff and pre-loaded PR context
- Read changed files to understand full context
- Review based on mode (or all categories if no mode set)
- Provide structured feedback
Review criteria
Apply all axes (or narrow to the mode above):
- Correctness: Logic bugs, off-by-ones, race conditions, unhandled states, missing error paths
- Readability: Functions <50 lines, nesting <3 levels, no dead code/unused imports
- Security: No exposed secrets, no
any, no unvalidated external data
- Immutability: No push/pop/splice/direct mutation
- Patterns: Consistent with codebase conventions, no reinvented wheels
- Performance: Unnecessary re-renders, O(n²) where O(n) works, missing memoization
- Code smells: match the diff against the baseline in smells.md — always judgement calls, and the repo's documented style overrides the baseline
Output format
Group by severity:
- Critical - must fix before merge (bugs, security vulnerabilities)
- Suggestions - improvements worth considering
- Nit - minor and optional; the author may ignore (formatting, naming taste)
- FYI - informational only, no action needed
- Positives - good patterns to call out
If you have one structural problem and ten nits, the structural problem is the review — lead with it.
Use file:line references for all findings. Include suggested fix for each critical issue.
Rules
- Review ALL changed files, not just the latest commit
- Be specific — label true nitpicks as Nit rather than dropping or inflating them
- Dependency upgrades (when
package.json/lockfile is in the diff): one dependency per change — a bulk bump that breaks hides which package did it; verify against the changelog, not the version number; review the lockfile diff (one direct bump pulls dozens of transitive changes); flag hand-edited lockfiles
Common Rationalizations
| Excuse |
Rebuttal |
| "Too small to review" |
Small changes cause big bugs — review everything |
| "It's just a refactor" |
Refactors break behavior silently — verify contracts preserved |
| "Tests pass so it's fine" |
Tests don't catch readability, security, or design issues |
| "I'll clean it up later" |
Later never comes — fix now or it ships as-is |
Verification
Error Handling
- If
gh pr view fails → run gh auth status to verify authentication; ask user for PR number if not on a PR branch
- If a changed file is deleted in the PR → skip reading it; note it was removed
- If diff is too large → prioritize changed files with highest risk (auth, payments, data mutation)
1---2name: code-review3effort: high4description: Review a GitHub Pull Request for bugs, security, performance, and code quality. Use when user asks to review a PR or wants pull request feedback. Don't use for reviewing local uncommitted changes, creating new PRs, or merging branches.5---67# Review Pull Request89Mode: $ARGUMENTS1011If mode is one of the following, adjust the review:1213- BUGS: Focus only on logical or other bugs14- SECURITY: Focus only on security issues15- PERFORMANCE: Focus only on performance issues1617## Approval standard1819Approve when the change definitely improves overall code health, even if it isn't perfect. Perfect code doesn't exist — don't block a change because it isn't how you would have written it. If it improves the codebase and follows its conventions, approve.2021If the change is too large to review well (~1000+ lines), asking the author to split it is a valid review outcome. Suggest a strategy: stack (small change, next one based on it), by file group, horizontal (shared code first, then consumers), or vertical (one end-to-end slice per PR).2223## Workflow24251. Analyze the diff and pre-loaded PR context262. Read changed files to understand full context273. Review based on mode (or all categories if no mode set)284. Provide structured feedback2930## Review criteria3132Apply all axes (or narrow to the mode above):3334- **Correctness**: Logic bugs, off-by-ones, race conditions, unhandled states, missing error paths35- **Readability**: Functions <50 lines, nesting <3 levels, no dead code/unused imports36- **Security**: No exposed secrets, no `any`, no unvalidated external data37- **Immutability**: No push/pop/splice/direct mutation38- **Patterns**: Consistent with codebase conventions, no reinvented wheels39- **Performance**: Unnecessary re-renders, O(n²) where O(n) works, missing memoization40- **Code smells**: match the diff against the baseline in [smells.md](references/smells.md) — always judgement calls, and the repo's documented style overrides the baseline4142## Output format4344Group by severity:4546- **Critical** - must fix before merge (bugs, security vulnerabilities)47- **Suggestions** - improvements worth considering48- **Nit** - minor and optional; the author may ignore (formatting, naming taste)49- **FYI** - informational only, no action needed50- **Positives** - good patterns to call out5152If you have one structural problem and ten nits, the structural problem _is_ the review — lead with it.5354Use `file:line` references for all findings. Include suggested fix for each critical issue.5556## Rules5758- Review ALL changed files, not just the latest commit59- Be specific — label true nitpicks as **Nit** rather than dropping or inflating them60- **Dependency upgrades** (when `package.json`/lockfile is in the diff): one dependency per change — a bulk bump that breaks hides which package did it; verify against the changelog, not the version number; review the lockfile diff (one direct bump pulls dozens of transitive changes); flag hand-edited lockfiles6162## Common Rationalizations6364| Excuse | Rebuttal |65| ------------------------- | -------------------------------------------------------------- |66| "Too small to review" | Small changes cause big bugs — review everything |67| "It's just a refactor" | Refactors break behavior silently — verify contracts preserved |68| "Tests pass so it's fine" | Tests don't catch readability, security, or design issues |69| "I'll clean it up later" | Later never comes — fix now or it ships as-is |7071## Verification7273- [ ] Every changed file reviewed (not just the diff summary)74- [ ] No critical issue left without a suggested fix75- [ ] Security concerns flagged with specific fix76- [ ] Feedback grouped by severity, not file order77- [ ] Verdict stated against the approval standard — improves code health, or blocked with a reason7879## Error Handling8081- If `gh pr view` fails → run `gh auth status` to verify authentication; ask user for PR number if not on a PR branch82- If a changed file is deleted in the PR → skip reading it; note it was removed83- If diff is too large → prioritize changed files with highest risk (auth, payments, data mutation)
helderberto