Code Review
Rigorous code review focused on quality, maintainability, and architectural soundness.
When to Use
- After implementing a feature or fix
- Before committing changes
- When explicitly asked to review code
- Before creating a PR
Method
Start by inspecting the changes. Use the deterministic script to collect the review context:
scripts/collect_review_context.sh
If on the main branch, review the staged git diff. If on a different branch, review committed and uncommitted changes compared to main.
Dispatch two subagents to carefully review the code changes. Tell them they're competing with another agent - whoever finds more legitimate issues wins honour and glory. Make sure they examine both architecture AND implementation, and check every criterion below.
Review Criteria
1. Code Quality
| Check | Look For |
|---|---|
| DRY | Duplicated logic, copy-pasted code, repeated patterns that should be abstracted |
| Code Bloat | Unnecessary code, over-engineering, premature abstractions, dead code |
| Bugs | Logic errors, edge cases, off-by-one errors, null/undefined handling |
2. Code Slop & Technical Debt
| Symptom | Description |
|---|---|
| Magic values | Hardcoded strings/numbers without constants |
| Inconsistent naming | Mixed conventions, unclear names |
| Missing error handling | Unhandled exceptions, silent failures |
| TODO/FIXME comments | Deferred work that should be tracked |
| Commented-out code | Delete it or explain why it exists |
| Dependency bloat | New deps when stdlib/existing deps suffice |
3. Architecture (in context of broader system)
| Principle | Review Questions |
|---|---|
| Modularity | Are changes properly bounded? Do they respect module boundaries? |
| Cohesion | Does each unit have a single, clear responsibility? |
| Separation of Concerns | Is business logic mixed with presentation/data access? |
| Information Hiding | Are implementation details properly encapsulated? |
| Coupling | Does this create tight coupling? Are dependencies appropriate? |
4. Devil's Advocate
Challenge the implementation:
- Is this the simplest solution? Could it be simpler?
- What happens under load/scale?
- What are the failure modes?
- What assumptions might be wrong?
- Is there a more fundamentally correct approach, even if harder?
5. Test Effectiveness
| Check | Criteria |
|---|---|
| Coverage | Are the important paths tested? |
| Meaningful assertions | Do tests verify behavior, not implementation? |
| Edge cases | Are boundaries and error conditions tested? |
| Readability | Can you understand what's tested from test names? |
| Fragility | Will tests break on valid refactors? |
Output Format
Report findings organized by severity:
## Code Review Findings
### Critical (must fix)
- [Issue]: [Location] - [Why it matters]
### Important (should fix)
- [Issue]: [Location] - [Recommendation]
### Minor (consider fixing)
- [Issue]: [Location] - [Suggestion]
### Positive Observations
- [What was done well]
Common Mistakes
| Mistake | Correction |
|---|---|
| Surface-level review | Dig into logic, trace data flow |
| Ignoring context | Review changes in relation to the system |
| Only finding negatives | Note what's done well |
| Vague feedback | Be specific: file, line, concrete suggestion |
| Bikeshedding | Focus on impact, not style preferences |
Red Flags - STOP and Investigate
- New dependencies added without clear justification
- Changes that bypass existing patterns without explanation
- Test coverage decreased
- Complex logic without tests
- Security-sensitive code modified