Code Review

Contents

• Design Philosophy
• When to Use This Skill
• Review Dimensions
• Review Workflow
• Integration with implement-phase
• Invoking the Skill
• Review Checklists
• Severity Levels
• Critical Rule: All Errors Are New Errors
• Best Practices
• Example Review Session

Reference files (open one when a step points to it):

• references/adr-compliance-checklist.md: ADR Compliance Checklist
• references/coding-standards-checklist.md: Coding Standards Compliance Checklist
• references/framework-standards-checklist.md: Framework Standards Compliance Checklist
• references/general-quality-checklist.md: General Code Quality Checklist
• references/service-delegation-checklist.md: Service Delegation & Single Responsibility Checklist

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-phase (Step 4):

• After the phase passes exit conditions and integration testing (Steps 2–3)
• 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:

2. Framework Standards Compliance

Verify code follows the project's established patterns:

3. ADR Compliance

Verify architectural decisions are followed and documented:

4. Plan Synchronization

Verify the implementation matches the phase as the plan specifies it. Per ADR-0001 the plan is a specification and progress is tracked outside it, so don't expect ticked checkboxes or status edits in the plan.

The scope comes from the plan file's phase section, or from the phase description and requirements the caller passes. With no plan or phase scope given (a plain manual review), report this dimension as PASS with "No plan given".

A missing work item is BLOCKING. Unplanned work, or a deviation with no evident reason, is a RECOMMENDATION: bring the code back in line with the plan or state the reason. A deviation with a reason, and an ADR reference the plan lacks, are NOTES: implement-phase records both in the plan at Step 6, after this review, so they need not be there yet.

5. General Code Quality

Standard code review concerns:

6. Coding Standards Compliance

Verify implementation follows project coding standards:

Source of truth: docs/standards/CODING_STANDARDS.md when it exists. When it doesn't, the project's own conventions are the standard: observe them in neighbouring code (Review Workflow, Step 2) and flag only departures from what that code consistently does. Framework-specific checks in the checklists (marked NestJS, TypeScript, …) apply only when that framework is detected in the project, or when the standards file adopts them.

Review Workflow

Step 1: Gather Context

1. READ the plan file to understand:
   - Current phase being reviewed
   - Expected deliverables (the objective and work items dimension 4 checks against)
   - 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. DETECT the stack from manifest files (package.json, pyproject.toml, go.mod,
   *.csproj, pom.xml, ...). Framework-specific checklist items apply only to
   frameworks found here.

2. READ docs/standards/CODING_STANDARDS.md if it exists.

3. FIND the project's conventions for each kind of changed file
   (services, controllers, tests, error handling, logging, config):
   - Glob for siblings of the same kind (e.g. other *.service.ts in the module)
   - Read two or three of them; Grep for the idioms they share
   - In a large or unfamiliar codebase, if Agent is in your tool list, spawn an
     Explore subagent instead and ask for the conventions with file:line examples

4. COMPARE changed code against identified patterns

Step 3: Review Each Dimension

Each dimension produces the same block, however it runs:

DIMENSION: [name]
STATUS: PASS | NEEDS_CHANGES
FINDINGS:
- [BLOCKING|RECOMMENDATION|NOTE] path:line — issue — fix

If Agent is in your tool list and the change spans more than a few files, review the dimensions in parallel: send six Agent calls in one message, one per dimension, with subagent_type: "general-purpose". Subagents start without your context, so each prompt carries the changed files, the context from the table below, the path to its checklist in this skill's references/ directory, the Severity Levels table, an instruction not to edit files, and the block above as the required reply. Subagents run in the background; wait for all six completion notifications before writing the report.

Agent(
  description="Review: service delegation",
  subagent_type="general-purpose",
  prompt="Read-only code review; do not edit any file.
Dimension: service delegation & single responsibility.
Files: src/auth/auth.service.ts, src/auth/auth.guard.ts, src/auth/jwt.strategy.ts
Checklist: <absolute path to this skill>/references/service-delegation-checklist.md
Classify each finding BLOCKING, RECOMMENDATION or NOTE: <Severity Levels table>
Reply with only:
DIMENSION: service-delegation
STATUS: PASS | NEEDS_CHANGES
FINDINGS:
- [BLOCKING|RECOMMENDATION|NOTE] path:line — issue — fix"
)

Otherwise (no Agent in your tool list, or a change small enough to review in one pass), work through the six dimensions yourself, in order, as a checklist, and produce the same six blocks.

Step 4: Generate Review Report

Output a structured review report. A dimension's Status is NEEDS_CHANGES if it has any BLOCKING or RECOMMENDATION finding; NOTES alone leave it PASS.

# Code Review: [Phase/Feature Name]

**Date:** YYYY-MM-DD
**Files Reviewed:** [count]
**Verdict:** PASS | 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 every BLOCKING issue and RECOMMENDATION, each tagged with its class, file:line and the fix, or "None"]

## Notes

[Informational findings that don't affect the verdict, or "None"]

## 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 4 of the phase pipeline), this skill:

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

STATUS: PASS | NEEDS_CHANGES
VERDICT: [Brief summary]
CHANGES_REQUIRED: [Count of blocking issues + recommendations; 0 on PASS]
BLOCKING_ISSUES:
- [path:line — issue — fix, or "None"]
RECOMMENDATIONS:
- [path:line — issue — fix, or "None"]
NOTES:
- [Informational items, or "None"]
REPORT: [Path to full review report, if written to disk]

Implement-Phase Integration Point

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

Step 1: Implementation (subagents)
Step 2: Exit condition verification
Step 3: Integration testing
Step 4: CODE-REVIEW (this skill) ←
Step 5: ADR compliance check
Step 6: Plan synchronization
Step 7: Prompt archival
Step 8: 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 and recommendations
   b. SPAWN fix subagents for each of them
   c. Re-run code-review
   d. REPEAT until PASS (max 3 retries)
3. PROCEED to Step 5 (ADR compliance)

NOTES never start a fix loop; deviation notes feed Step 6 (plan synchronization).

Invoking the Skill

From implement-phase (automatic)

Skill(skill="devflow: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

/devflow: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

STATUS is PASS only with zero BLOCKING issues and zero RECOMMENDATIONS; otherwise it is NEEDS_CHANGES. The verdict is binary because implement-phase gates on it and fixes every recommendation before a phase completes, while NOTES keep useful observations visible without forcing a fix loop. So class a finding as a RECOMMENDATION only if it is worth fixing now, and never downgrade a real defect to a NOTE to reach PASS.

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

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

Recommendations (fail review until fixed)

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

Notes (never fail review)

• 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 notes
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. Keep notes visible - Notes don't block; record the ones worth acting on as follow-up work

Example Review Session

implement-phase: Step 3 (integration testing) passed. Running Step 4: 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 (NestJS detected in package.json)
  - ✅ Uses project's config service
  - ⚠️ RECOMMENDATION: auth.service.ts:12 uses NestJS Logger, not the project's CustomLogger

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

● Checking plan synchronization...
  - ✅ Phase 2 work items delivered: AuthService, AuthGuard, JwtStrategy, with tests
  - ℹ️ NOTE: refresh-token rotation pulled forward from Phase 3 because the guard needs it

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

● Checking coding standards...
  - ✅ No swallowed errors, no debug printing
  - ℹ️ NOTE: auth.service.ts is 280 lines, near the 300-line service threshold

STATUS: NEEDS_CHANGES
VERDICT: One recommendation to fix; two notes for the record
CHANGES_REQUIRED: 1
BLOCKING_ISSUES: None
RECOMMENDATIONS:
- src/auth/auth.service.ts:12 — NestJS Logger instead of the project's CustomLogger — inject CustomLogger
NOTES:
- Deviation: refresh-token rotation implemented in Phase 2 rather than Phase 3 (the guard needs it) — for Step 6 to record in the plan
- auth.service.ts at 280 lines — candidate for splitting if Phase 3 adds to it

implement-phase: NEEDS_CHANGES → spawning a fix subagent for 1 recommendation, then re-running code review.