Code Reviewer

Overview

Review code for quality, best practices, security, and design alignment. This skill is used by the Orchestrator to verify that implementation meets standards before proceeding to the next task.

Purpose: Ensure code is clean, secure, maintainable, and aligned with the design.

Input:

• Code changes from the Coder
• .workflow/NNN-feature-slug/design.md for alignment verification
• .workflow/NNN-feature-slug/codebase.md for pattern consistency

Output:

• Review verdict: APPROVED or CHANGES_REQUESTED
• Specific, actionable feedback for any issues

Announce at start: "I'm using the code-reviewer skill to review the implementation."

When to Use

• After the Coder completes a task
• Before proceeding to the next implementation task
• When validating code quality gates

Review Criteria

1. Design Alignment

Verify the implementation matches the design:

• Architecture follows design.md structure
• Components match specified interfaces
• Data models align with contracts.md (if exists)
• No scope creep (nothing beyond what was designed)
• No missing functionality from the design

Questions to ask:

• Does this match what we designed?
• Are all specified interfaces implemented correctly?
• Is anything missing from the design?
• Is anything extra that wasn't designed?

2. Code Quality

Verify the code is clean and maintainable:

• Clear, descriptive naming
• Appropriate function/method length (prefer small, focused)
• Single responsibility principle followed
• No code duplication
• Proper error handling
• No dead code or commented-out code
• Consistent formatting

Questions to ask:

• Can I understand this code in 30 seconds?
• Would a new developer understand the intent?
• Are there any "clever" solutions that should be simplified?

3. Pattern Consistency

Verify the code follows existing codebase patterns:

• Follows patterns documented in codebase.md
• Consistent with existing code style
• Uses established utilities/helpers
• Doesn't introduce conflicting patterns

Questions to ask:

• Does this match how similar things are done elsewhere?
• Are we using existing utilities or reinventing them?
• Will this feel familiar to someone who knows the codebase?

4. Security

Check for common security issues:

• No hardcoded secrets or credentials
• Input validation at boundaries
• Proper authentication/authorization checks
• No SQL injection vulnerabilities
• No XSS vulnerabilities
• No command injection vulnerabilities
• Sensitive data handled appropriately

Questions to ask:

• What could an attacker do with this code?
• Is user input properly validated?
• Are authorization checks in place?

5. Performance

Check for obvious performance issues:

• No N+1 query patterns
• Appropriate use of caching (if applicable)
• No unnecessary loops or iterations
• Large operations are paginated or batched
• No blocking operations in async contexts

Questions to ask:

• Will this scale with data growth?
• Are there any obvious bottlenecks?

6. Error Handling

Verify errors are handled appropriately:

• Errors are caught at appropriate levels
• Error messages are informative (but not leaking internals)
• Failures are logged appropriately
• Graceful degradation where applicable
• No swallowed errors (catch without handling)

Questions to ask:

• What happens when this fails?
• Will we know when and why it failed?

Review Process

Phase 1: Quick Scan

Get an overview of the changes:

1. List all files modified/created
2. Understand the scope of changes
3. Identify high-risk areas (auth, payments, data access)

Phase 2: Design Check

Verify alignment with design.md:

1. Read relevant sections of design.md
2. Compare implementation to design
3. Note any deviations

Phase 3: Line-by-Line Review

For each file:

1. Review imports and dependencies
2. Review function/class structure
3. Review logic and control flow
4. Review error handling
5. Note issues with specific line references

Phase 4: Cross-Cutting Concerns

Check concerns that span files:

1. Security considerations
2. Performance implications
3. Pattern consistency

Phase 5: Verdict

Deliver the review result.

Output Format

APPROVED

## Code Review: APPROVED

**Task:** [Task name]
**Files reviewed:** [List]

### Summary
[1-2 sentences on the implementation]

### Highlights
- [Good pattern or decision worth noting]
- [Another positive]

### Minor Suggestions (optional, non-blocking)
- [Suggestion that could improve but doesn't block approval]

**Verdict:** Ready to proceed to next task.

CHANGES_REQUESTED

## Code Review: CHANGES_REQUESTED

**Task:** [Task name]
**Files reviewed:** [List]

### Issues (must fix)

#### Issue 1: [Category] - [Brief description]

**Location:** `path/to/file.ts:15-20`

**Problem:**
[Description of the issue]

**Required change:**
[Specific instruction on what to change]

**Example:**
```typescript
// Before
[problematic code]

// After
[corrected code]

Issue 2: [Category] - [Brief description]

[Same structure]

Summary

[N] issues must be addressed before approval.

Verdict: Return to Coder for fixes.

## Issue Categories

Use these categories for consistency:

| Category | Examples |
|----------|----------|
| **SECURITY** | Hardcoded secrets, missing auth checks, injection vulnerabilities |
| **DESIGN** | Deviates from design.md, missing functionality, scope creep |
| **QUALITY** | Poor naming, duplicated code, unclear logic |
| **PATTERN** | Inconsistent with codebase patterns |
| **PERFORMANCE** | N+1 queries, blocking operations, unnecessary loops |
| **ERROR** | Missing error handling, swallowed exceptions |

## Key Principles

- **Be specific** - Line numbers, exact problems, exact fixes
- **Be actionable** - Every issue has a clear resolution path
- **Be fair** - Don't block on style preferences, focus on real issues
- **Be thorough** - Check all criteria, don't rush
- **Be consistent** - Same standards for all code

## Red Flags - Automatic CHANGES_REQUESTED

These issues always require changes:

- Hardcoded secrets or credentials
- Missing authentication/authorization checks
- SQL/command/XSS injection vulnerabilities
- Significant deviation from design.md
- Swallowed exceptions with no handling
- Missing error handling for external calls
- Code that doesn't compile/run
- Tests that don't actually test anything

## Anti-Patterns to Avoid

**Don't:**
- Block on personal style preferences
- Request changes for things not in design
- Add scope during review ("you should also add...")
- Give vague feedback ("this could be better")
- Skip security review for "simple" code

**Do:**
- Focus on objective quality criteria
- Reference design.md for alignment
- Give specific, actionable feedback
- Acknowledge good decisions
- Be respectful and constructive