Code Review
Decision Tree
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:
[SEVERITY] file:line - Description
Why: Impact explanation
Fix: Suggested code change
Checklist
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.
1---2name: code-review-53description: Systematic code review checklist covering correctness, security, performance, and maintainability. Use when reviewing pull requests, auditing changes, or checking code quality across any language.4---5
6# Code Review
7
8## Decision Tree
9
10```
11Review request → What scope?
12 ├─ Full PR review → Follow all 6 steps below
13 ├─ Security-focused → Steps 1, 3 + security-audit skill
14 ├─ Quick sanity check → Steps 1, 2, 6 only
15 └─ Performance review → Steps 1, 4 + performance-audit skill
16```
17
18## Process
19
201. **Context** - Read changed files, understand intent from PR description or commit messages
212. **Correctness** - Logic errors, edge cases, off-by-ones, null/undefined handling
223. **Security** - Injection, auth bypass, data exposure, OWASP top 10
234. **Performance** - N+1 queries, unnecessary allocations, algorithm complexity
245. **Maintainability** - Naming, structure, DRY, single responsibility
256. **Tests** - Coverage of changed code, edge cases, assertion quality
26
27## Severity Levels
28
29| Level | Meaning | Action |
30|-------|---------|--------|
31| **CRITICAL** | Security vulnerability, data loss, crash | Must fix before merge |
32| **MAJOR** | Logic error, missing error handling, perf issue | Should fix before merge |
33| **MINOR** | Style, naming, documentation gap | Fix or acknowledge |
34| **NITPICK** | Preference, alternative approach | Optional |
35
36## Output Format
37
38For each finding:
39```
40[SEVERITY] file:line - Description
41 Why: Impact explanation
42 Fix: Suggested code change
43```
44
45## Checklist
46
47- [ ] No hardcoded secrets, tokens, or credentials
48- [ ] Error paths handled (not just happy path)
49- [ ] No unvalidated user input reaches DB/shell/HTML
50- [ ] New dependencies justified and audited
51- [ ] Breaking changes documented
52- [ ] Tests cover the changed behavior
53- [ ] No commented-out code committed
54- [ ] No debug logging left in
55
56## Anti-Patterns
57
58- **Rubber-stamping** - Approving without reading. Every file deserves attention.
59- **Style wars** - Debating formatting that a linter should handle. Automate it.
60- **Rewrite requests** - Asking for a total rewrite in review. That's a design discussion, not a review.
61- **Scope creep** - Requesting unrelated improvements. File separate issues instead.
diegosouzapw