| name | code-review |
| description | Use when reviewing a diff or pull request — check correctness, tests, readability, security, and server/client boundaries before approving. |
Code Review
Overview
Give a focused, constructive review of a change. Separate blocking issues from
optional suggestions. Review the work product (diff + tests), not the author's
process.
Core principle: Review early, review often — before merge, not after production.
When to Review
Mandatory:
- Before merging to the main branch
- After completing a major feature or multi-file change
- When security-sensitive paths change (auth, payments, user data)
Valuable:
- After fixing a non-trivial bug (regression check)
- Before a large refactor (baseline sanity check)
Process
- Read the change description and confirm it matches the diff.
- Identify scope: which files, routes, or modules changed and why.
- Check correctness: edge cases, error handling, and inputs that could break.
- Check tests: is new behavior covered? Do existing tests still pass?
- Check readability: clear names, small functions, no dead code.
- Check boundaries: server-only code must not leak to client bundles or logs.
- Note security concerns as blocking (see
project-security skill).
Review Checklist
| Area | Blocking if… |
|---|
| Correctness | Wrong logic, missing error handling, race conditions |
| Tests | New behavior untested; tests deleted without replacement |
| Security | Unvalidated input, secrets in code/logs, missing auth |
| API shape | Breaking changes without migration or docs |
| Performance | Obvious N+1, unbounded loops, sync I/O on hot paths |
Output Format
Use this structure every time:
## Blocking
- [file:line] Issue — concrete fix
## Suggestions
- [file:line] Optional improvement
## Verdict
approve | approve-with-nits | request-changes
- List blocking issues first, each with a concrete fix.
- Then non-blocking suggestions.
- End with an explicit verdict.
Integration with TDD
If the change adds behavior without a failing test first, that is a blocking
issue unless the author documents why (e.g. config-only change). Ask for tests
that prove the behavior.
Anti-patterns
- Approving without checking tests or security-sensitive paths.
- Nitpicking style while missing error handling.
- Vague feedback ("this feels wrong") without a concrete fix.
- Reviewing the conversation instead of the diff.
- Rubber-stamping because "it's small."
When You Disagree
Push back with technical reasoning. Cite failing tests, spec requirements, or
security impact. Request clarification when the intent is unclear.
