mhylle/code-review

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