Code Review

Systematic code review skill that validates implementation quality across six dimensions: service delegation, framework standards, ADR compliance, plan synchronization, coding standards, and general code quality.

Design Philosophy

This skill acts as a quality gate between implementation phases. It ensures code meets established standards before proceeding, catching issues early when they're cheapest to fix.

When to Use This Skill

Automatically invoked by implement-plan:

• After each phase completes its functional verification
• Before marking a phase as complete
• Before proceeding to the next phase

Manually invoked when:

• Reviewing a PR or set of changes
• Validating code before committing
• Checking implementation against architectural decisions

Review Dimensions

1. Service Delegation & Single Responsibility

Verify that implementation follows proper separation of concerns:

Check	What to Look For
Controller thin	Controllers only handle HTTP concerns, delegate to services
Service focused	Each service has one clear responsibility
No god objects	No classes/modules doing too many things
Dependency direction	Dependencies flow inward (controllers → services → repositories)
No business logic in wrong layer	Controllers don't contain business rules, repositories don't transform data

2. Framework Standards Compliance

Verify code follows the project's established patterns:

Check	What to Verify
Naming conventions	Files, classes, methods follow project conventions
Module structure	New code follows existing module organization
Error handling	Uses project's error handling patterns
Logging	Follows established logging conventions
Configuration	Uses project's config management approach
Testing patterns	Tests follow existing test structure and naming

3. ADR Compliance

Verify architectural decisions are followed and documented:

Check	What to Do
Read relevant ADRs	Check INDEX.md, identify ADRs related to this change
Verify compliance	Implementation follows documented decisions
New decisions documented	Any new architectural choices have corresponding ADRs
No contradictions	Changes don't violate existing accepted ADRs

4. Plan Synchronization

Verify the plan document reflects current state:

Check	What to Verify
Checkboxes updated	Completed tasks marked with [x]
Exit conditions recorded	All exit condition statuses updated
Deviations noted	Any plan deviations documented with rationale
ADR references added	New ADRs linked in plan where relevant
Phase status accurate	Current phase progress matches reality

5. General Code Quality

Standard code review concerns:

Check	What to Look For
No security issues	No injection vulnerabilities, proper auth checks
No hardcoded secrets	No API keys, passwords, or tokens in code
Error handling	Errors handled gracefully, not swallowed
Resource cleanup	Connections, streams, handlers properly closed
No dead code	No commented-out code, unused imports/variables
Test coverage	New code has appropriate test coverage
Documentation	Complex logic has explanatory comments

6. Coding Standards Compliance

Verify implementation follows project coding standards:

Check	What to Verify
File size limits	Services <500 lines, controllers <250 lines
Service delegation	Controllers thin, services focused
Interface usage	DTOs and response types have interfaces
Error handling	Domain exceptions, no swallowed errors
Logging	Project logger used, no console.log
Configuration	Via ConfigService, no hardcoded values
Forbidden patterns	No empty catch blocks, no unhandled promises

Reference: docs/standards/CODING_STANDARDS.md

Review Workflow

Step 1: Gather Context

1. READ the plan file to understand:
   - Current phase being reviewed
   - Expected deliverables
   - Exit conditions that should be met

2. READ docs/decisions/INDEX.md to identify:
   - ADRs relevant to this change
   - Recently added ADRs

3. IDENTIFY changed files using git:
   - git diff --name-only [base]...HEAD
   - Or use provided file list

Step 2: Framework Pattern Analysis

1. SPAWN codebase-pattern-finder to identify project conventions:
   - Service patterns
   - Controller patterns
   - Test patterns
   - Error handling patterns

2. COMPARE changed code against identified patterns

Step 3: Review Each Dimension

Execute reviews in parallel where possible:

Task (parallel): "Review service delegation in [files]"
Task (parallel): "Compare code against framework patterns found"
Task (parallel): "Check ADR compliance for [relevant ADRs]"
Task (parallel): "Verify plan file [path] is synchronized"
Task (parallel): "General code quality review of [files]"
Task (parallel): "Coding standards compliance check against docs/standards/CODING_STANDARDS.md"

Step 4: Generate Review Report

Output a structured review report:

# Code Review: [Phase/Feature Name]

**Date:** YYYY-MM-DD
**Files Reviewed:** [count]
**Verdict:** PASS | PASS_WITH_NOTES | NEEDS_CHANGES

## Summary

[1-2 sentence overall assessment]

## Review Results

### Service Delegation & Single Responsibility
Status: PASS | NEEDS_CHANGES

[Findings or "All checks passed"]

### Framework Standards Compliance
Status: PASS | NEEDS_CHANGES

[Findings or "All checks passed"]

### ADR Compliance
Status: PASS | NEEDS_CHANGES

[Findings or "All checks passed"]

### Plan Synchronization
Status: PASS | NEEDS_CHANGES

[Findings or "All checks passed"]

### General Code Quality
Status: PASS | NEEDS_CHANGES

[Findings or "All checks passed"]

### Coding Standards Compliance
Status: PASS | NEEDS_CHANGES

[Findings or "All checks passed"]

## Required Changes

[Numbered list of changes required before approval, or "None"]

## Recommendations

[Optional improvements that don't block approval]

## Files Reviewed

| File | Status | Notes |
|------|--------|-------|
| `path/to/file.ts` | OK | |
| `path/to/other.ts` | ISSUE | [Brief note] |

Integration with implement-phase

When invoked by implement-phase (Step 3 of the phase pipeline), this skill:

1. Receives context: Phase number, changed files, plan path
2. Executes review: All five dimensions
3. Returns structured result:

STATUS: PASS | NEEDS_CHANGES
VERDICT: [Brief summary]
CHANGES_REQUIRED: [Count or "None"]
BLOCKING_ISSUES:
- [List of blocking issues, or "None"]
RECOMMENDATIONS:
- [List of non-blocking suggestions]
REPORT: [Path to full review report, if written to disk]

Implement-Phase Integration Point

This skill is Step 3 in the implement-phase pipeline:

Step 1: Implementation (subagents)
Step 2: Exit condition verification
Step 3: CODE-REVIEW (this skill) ←
Step 4: ADR compliance check
Step 5: Plan synchronization
Step 6: Completion report

The implement-phase skill handles retry logic:

1. INVOKE code-review skill for the phase
2. IF code-review returns NEEDS_CHANGES:
   a. PRESENT blocking issues
   b. SPAWN fix subagents for each issue
   c. Re-run code-review
   d. REPEAT until PASS (max 3 retries)
3. PROCEED to Step 4 (ADR compliance)

Invoking the Skill

From implement-phase (automatic)

Skill(skill="code-review"): Review Phase 2 implementation.

Context:
- Plan: docs/plans/auth-implementation.md
- Phase: 2 (Authentication Service)
- Changed files: src/auth/auth.service.ts, src/auth/auth.guard.ts, src/auth/jwt.strategy.ts

Return structured result for implement-phase orchestrator.

Manual invocation

/code-review

Review the authentication changes in src/auth/.
Focus on: service delegation, NestJS patterns, and ADR-0012 compliance.

Review Checklists

Detailed checklists are available in references:

• service-delegation-checklist.md
• framework-standards-checklist.md
• adr-compliance-checklist.md
• general-quality-checklist.md
• coding-standards-checklist.md

Severity Levels

Level	Meaning	Action
BLOCKING	Must fix before proceeding	Phase cannot complete
WARNING	Should fix, but doesn't block	Note for follow-up
INFO	Suggestion for improvement	Optional enhancement

Critical Rule: All Errors Are New Errors

Every phase ends with a clean baseline (exit conditions pass). Therefore, ANY error present at code review time was introduced by this phase.

The Clean Baseline Principle

The implement-phase pipeline guarantees:

Previous Phase → Exit Conditions PASS → Clean State
                                            ↓
Current Phase Implementation
                                            ↓
Code Review ← ANY errors here are from THIS phase

Since each phase must pass exit conditions (build, lint, typecheck, tests) before completion:

• The previous phase left the codebase in a clean state
• Any errors now present were introduced by the current phase
• This applies to ALL files, not just files we directly modified

Why This Works

State	Implication
Lint passed before this phase	Any lint errors now = we caused them
Build passed before this phase	Any build errors now = we caused them
Tests passed before this phase	Any test failures now = we caused them
No type errors before this phase	Any type errors now = we caused them

There is no "pre-existing error" exception because:

• If errors existed before, the previous phase wouldn't have completed
• Exit conditions are blocking gates - phases cannot complete with errors
• The baseline is always clean

Errors in Unchanged Files

Even errors in files we didn't directly modify are our responsibility:

Example: We modify src/auth/types.ts
         This causes type errors in src/api/endpoints.ts (which imports our types)

         → The error in endpoints.ts is OUR error
         → We broke it by changing the types it depends on
         → This is BLOCKING

What This Means for Code Review

1. All build/lint/type errors are blocking - No exceptions
2. All test failures are blocking - No exceptions
3. No need to check git blame - If it's broken, fix it
4. No "tech debt" exceptions - We leave clean, we inherit clean responsibility

Inherited Codebases

When implementing on an existing codebase with pre-existing errors:

• Pre-existing errors are still blocking - We cannot complete a phase with errors
• Fixing them becomes part of our work - Add to the phase tasks if needed
• We are responsible for leaving clean - The next phase deserves a clean baseline

The principle is simple: every phase must end clean. If we inherit a mess, cleaning it up is our job before we can complete the phase.

Blocking Issues (always fail review)

• Security vulnerabilities (injection, auth bypass, etc.)
• Hardcoded secrets or credentials
• Violation of accepted ADR
• Missing test coverage for new functionality
• Build/lint/typecheck failures (in ANY file)
• Broken single responsibility (god classes/modules)
• Business logic in wrong layer

Warning Issues (review passes with notes)

• Minor pattern deviations
• Missing documentation on complex code
• Suboptimal but functional implementation
• Test coverage below project threshold (but not missing)

Info Issues (suggestions only)

• Code style preferences not enforced by linter
• Alternative implementation approaches
• Performance optimization opportunities
• Future refactoring candidates

Best Practices

For Effective Reviews

1. Read the plan first - Understand intent before judging implementation
2. Check ADRs - Don't flag violations of decisions that don't exist
3. Be specific - "Line 45: missing null check" not "error handling is bad"
4. Prioritize - Lead with blocking issues, don't bury them in warnings
5. Verify fixes - Re-run review after changes, don't assume

For Review Consumers

1. Address blocking issues first - Don't optimize while broken
2. Ask for clarification - If a review finding is unclear, ask
3. Don't argue with patterns - Follow established conventions, propose changes via ADR
4. Track warnings - Create follow-up tasks for non-blocking issues

Example Review Session

implement-phase: Step 2 (exit conditions) passed. Running Step 3: code review.

code-review: Reviewing Phase 2: Authentication Service

Files: src/auth/auth.service.ts, auth.guard.ts, jwt.strategy.ts

● Checking service delegation...
  - ✅ Controllers thin, delegate to AuthService
  - ✅ AuthService focused on authentication only
  - ✅ No business logic in guards

● Checking framework standards...
  - ✅ Follows NestJS injectable patterns
  - ✅ Uses project's config service
  - ⚠️ Logger not using project's custom logger

● Checking ADR compliance...
  - ✅ ADR-0012 (JWT auth): Compliant
  - ✅ No contradictions with existing ADRs

● Checking plan synchronization...
  - ✅ All Phase 2 tasks marked complete
  - ✅ Exit conditions recorded
  - ⚠️ Missing link to ADR-0012 in plan

● Checking general quality...
  - ✅ No security issues
  - ✅ Test coverage adequate
  - ✅ No dead code

STATUS: PASS_WITH_NOTES
VERDICT: Code meets standards with minor improvements suggested
CHANGES_REQUIRED: 0
BLOCKING_ISSUES: None
RECOMMENDATIONS:
- Use project's CustomLogger instead of NestJS Logger
- Add ADR-0012 reference to plan's Phase 2 section