Code Review
Decision Tree
code
Review request → What scope?
├─ Full PR review → Follow all 6 steps below
├─ Security-focused → Steps 1, 3 + security-audit skill
├─ Quick sanity check → Steps 1, 2, 6 only
└─ Performance review → Steps 1, 4 + performance-audit skill
Process
- •Context - Read changed files, understand intent from PR description or commit messages
- •Correctness - Logic errors, edge cases, off-by-ones, null/undefined handling
- •Security - Injection, auth bypass, data exposure, OWASP top 10
- •Performance - N+1 queries, unnecessary allocations, algorithm complexity
- •Maintainability - Naming, structure, DRY, single responsibility
- •Tests - Coverage of changed code, edge cases, assertion quality
Severity Levels
| Level | Meaning | Action |
|---|---|---|
| CRITICAL | Security vulnerability, data loss, crash | Must fix before merge |
| MAJOR | Logic error, missing error handling, perf issue | Should fix before merge |
| MINOR | Style, naming, documentation gap | Fix or acknowledge |
| NITPICK | Preference, alternative approach | Optional |
Output Format
For each finding:
code
[SEVERITY] file:line - Description Why: Impact explanation Fix: Suggested code change
Checklist
- • No hardcoded secrets, tokens, or credentials
- • Error paths handled (not just happy path)
- • No unvalidated user input reaches DB/shell/HTML
- • New dependencies justified and audited
- • Breaking changes documented
- • Tests cover the changed behavior
- • No commented-out code committed
- • No debug logging left in
Anti-Patterns
- •Rubber-stamping - Approving without reading. Every file deserves attention.
- •Style wars - Debating formatting that a linter should handle. Automate it.
- •Rewrite requests - Asking for a total rewrite in review. That's a design discussion, not a review.
- •Scope creep - Requesting unrelated improvements. File separate issues instead.