Agent-Skills.md

Agent Skills: Code Review

Systematic code review for implementation phases verifying architectural principles, framework standards, ADR compliance, and code quality. This skill is invoked by implement-phase as part of its quality gate pipeline, or manually when reviewing code changes. Triggers on "review code", "code review", "/code-review", or automatically as Step 3 of implement-phase.

UncategorizedID: mhylle/claude-skills-collection/code-review
171

Install this agent skill to your local

pnpm dlx add-skill https://github.com/mhylle/claude-skills-collection/tree/HEAD/skills/code-review

Skill Files

Browse the full folder contents for code-review.

Download Skill

Loading file tree…

skills/code-review/SKILL.md

Skill Metadata

Name
code-review
Description
>-

Code Review

<!-- contents: generated by tests/skill_structure.py --write; edit headings, not this block -->

Contents

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
<!-- /contents -->

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:

| 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 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.

| Check | What to Verify | |-------|----------------| | Scope delivered | The phase's objective and every work item have matching code and tests | | No unplanned scope | Changes outside the phase's described scope are needed by the phase, not unrelated work | | Deviations surfaced | Each difference between plan and code is listed with its reason | | ADR references | ADRs this phase follows or creates are listed |

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:

| 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 (unless the standards file sets its own) | | Service delegation | Controllers thin, services focused | | Interface usage | DTOs and response types have interfaces (typed languages) | | Error handling | Domain exceptions, no swallowed errors | | Logging | Project logger used, no debug printing (console.log, print) in production code | | Configuration | Via the project's config mechanism (NestJS: ConfigService), no hardcoded values | | Forbidden patterns | No empty catch blocks, no unhandled promises |

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.

| Dimension | Context to pass | Checklist | |-----------|-----------------|-----------| | Service delegation | Changed files | service-delegation-checklist.md | | Framework standards | Changed files, stack and conventions from Step 2 | framework-standards-checklist.md | | ADR compliance | Relevant ADR paths from Step 1 | adr-compliance-checklist.md | | Plan synchronization | Plan path and phase, or the scope the caller passed | Dimension 4 table above | | General code quality | Changed files | general-quality-checklist.md | | Coding standards | Changed files, standards file or observed conventions | coding-standards-checklist.md |

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:

Severity Levels

| Level | Meaning | Action | |-------|---------|--------| | BLOCKING | Must fix before proceeding | STATUS is NEEDS_CHANGES; phase cannot complete | | RECOMMENDATION | Actionable fix at a specific location, within this phase's scope | STATUS is NEEDS_CHANGES; fixed in the same loop as blocking issues | | NOTE | Informational: future refactoring candidates, alternative approaches, deviations for the plan record | Reported only; never changes STATUS |

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

| 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

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.