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
1---2name: code-review3description: Use when reviewing code changes before committing, after implementing features, or when asked to review. Triggers on staged changes, PR reviews, or explicit review requests.4---56# Code Review78Rigorous code review focused on quality, maintainability, and architectural soundness.910## When to Use1112- After implementing a feature or fix13- Before committing changes14- When explicitly asked to review code15- Before creating a PR1617## Method1819Start by inspecting the changes. Use the deterministic script to collect the review context:2021```bash22scripts/collect_review_context.sh23```2425If on the `main` branch, review the staged git diff. If on a different branch, review committed and uncommitted changes compared to main.2627Dispatch 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.2829## Review Criteria3031### 1. Code Quality3233| Check | Look For |34|-------|----------|35| **DRY** | Duplicated logic, copy-pasted code, repeated patterns that should be abstracted |36| **Code Bloat** | Unnecessary code, over-engineering, premature abstractions, dead code |37| **Bugs** | Logic errors, edge cases, off-by-one errors, null/undefined handling |3839### 2. Code Slop & Technical Debt4041| Symptom | Description |42|---------|-------------|43| **Magic values** | Hardcoded strings/numbers without constants |44| **Inconsistent naming** | Mixed conventions, unclear names |45| **Missing error handling** | Unhandled exceptions, silent failures |46| **TODO/FIXME comments** | Deferred work that should be tracked |47| **Commented-out code** | Delete it or explain why it exists |48| **Dependency bloat** | New deps when stdlib/existing deps suffice |4950### 3. Architecture (in context of broader system)5152| Principle | Review Questions |53|-----------|-----------------|54| **Modularity** | Are changes properly bounded? Do they respect module boundaries? |55| **Cohesion** | Does each unit have a single, clear responsibility? |56| **Separation of Concerns** | Is business logic mixed with presentation/data access? |57| **Information Hiding** | Are implementation details properly encapsulated? |58| **Coupling** | Does this create tight coupling? Are dependencies appropriate? |5960### 4. Devil's Advocate6162Challenge the implementation:63- Is this the simplest solution? Could it be simpler?64- What happens under load/scale?65- What are the failure modes?66- What assumptions might be wrong?67- Is there a more fundamentally correct approach, even if harder?6869### 5. Test Effectiveness7071| Check | Criteria |72|-------|----------|73| **Coverage** | Are the important paths tested? |74| **Meaningful assertions** | Do tests verify behavior, not implementation? |75| **Edge cases** | Are boundaries and error conditions tested? |76| **Readability** | Can you understand what's tested from test names? |77| **Fragility** | Will tests break on valid refactors? |7879## Output Format8081Report findings organized by severity:8283```markdown84## Code Review Findings8586### Critical (must fix)87- [Issue]: [Location] - [Why it matters]8889### Important (should fix)90- [Issue]: [Location] - [Recommendation]9192### Minor (consider fixing)93- [Issue]: [Location] - [Suggestion]9495### Positive Observations96- [What was done well]97```9899## Common Mistakes100101| Mistake | Correction |102|---------|------------|103| Surface-level review | Dig into logic, trace data flow |104| Ignoring context | Review changes in relation to the system |105| Only finding negatives | Note what's done well |106| Vague feedback | Be specific: file, line, concrete suggestion |107| Bikeshedding | Focus on impact, not style preferences |108109## Red Flags - STOP and Investigate110111- New dependencies added without clear justification112- Changes that bypass existing patterns without explanation113- Test coverage decreased114- Complex logic without tests115- Security-sensitive code modified
tools-only