Agent Skills: Code Review Skill

Code Review Skill

Required Context

Pre-Flight Challenge

Before beginning work, address these operational feasibility questions:

1. What are the riskiest change areas in this implementation?
2. Which integration boundaries does this change cross?
3. What review focus areas does the spec's security or edge-case section highlight?

If any question cannot be answered from available context, surface it as a finding -- do not skip.

Purpose

Review implementation for quality issues before security review. Catch maintainability problems, style inconsistencies, and best practice violations. Produce pass/fail report with findings.

Key Input: Spec group at .claude/specs/groups/<spec-group-id>/

Usage

/code-review <spec-group-id>                   # Review all changes for spec group
/code-review <spec-group-id> <atomic-spec-id>  # Review changes for specific atomic spec

When to Use

Mandatory for:

• Any implementation completing the spec workflow
• Multi-file changes
• Public API additions or modifications
• Changes to core services

Optional for:

• Single-file bug fixes
• Documentation-only changes
• Test-only changes
• Configuration changes

Review Pipeline Position

Implementation → Unify → Code Review → Security Review → Merge
                            ↑
                        You are here

Code Review runs BEFORE Security Review because:

• Quality issues may mask security issues
• Consistent code is easier to security-review
• Catches different class of problems

Prerequisites

Before code review, verify:

1. Spec group exists at .claude/specs/groups/<spec-group-id>/
2. Convergence validated - /unify has passed
3. All atomic specs implemented - status is implemented
4. Tests passing - all tests pass

If prerequisites not met → STOP and run /unify first.

Code Review Process

Step 1: Load Review Context

# Load manifest
cat .claude/specs/groups/<spec-group-id>/manifest.json

# Verify convergence passed
# convergence.all_acs_implemented: true
# convergence.all_tests_passing: true

# Load spec and atomic specs
cat .claude/specs/groups/<spec-group-id>/spec.md
ls .claude/specs/groups/<spec-group-id>/atomic/

# What files changed (from atomic spec evidence)
# Read Implementation Evidence sections from each atomic spec

Step 2: Identify Files to Review

Build file list from atomic spec Implementation Evidence:

# For each atomic spec, extract Implementation Evidence
# These are the files that need review

# Alternatively, use git diff if on a feature branch
git diff --name-only main..HEAD

Step 3: Review Categories

Check each category systematically:

A. Code Style & Consistency

• Naming conventions (camelCase, PascalCase per project standard)
• File organization (imports, exports, structure)
• Formatting consistency
• Comment quality (useful vs obvious vs missing)
• AC references in comments (should reference atomic spec IDs)

B. Code Quality & Maintainability

• Function length (>50 lines is suspect)
• Cyclomatic complexity (>10 is suspect)
• Deep nesting (>3 levels is suspect)
• Code duplication
• Dead code
• Magic numbers/strings

C. TypeScript Best Practices

• any usage (should be rare and justified)
• Missing return types on public methods
• Proper null/undefined handling
• Generic usage appropriateness
• Type assertions (as) overuse

D. Error Handling

• Empty catch blocks
• Swallowed errors (catch and return null)
• Missing error types
• Inconsistent error handling patterns
• Error messages quality

E. API Design

• Inconsistent parameter ordering
• Missing or inconsistent return types
• Breaking changes to public API
• Undocumented public methods

F. Spec Conformance

• Implementation matches atomic spec ACs
• No undocumented features added
• Error handling per spec
• Edge cases per spec addressed

G. Testing Gaps

• Public methods without tests
• Edge cases not covered
• Test quality (meaningful assertions)
• Test isolation (no shared state)
• Tests reference correct atomic spec IDs

H. Assumption Review

• // TODO(assumption): comments identified and reviewed
• Each assumption's confidence level assessed for appropriateness
• No assumptions contradict explicit spec requirements
• No assumptions make behavioral decisions (should have been escalated)
• Atomic spec "Assumptions Made" section matches code TODOs
• Each assumption has clear resolution: Accept / Reject / Escalate

Step 4: Severity Classification

| Level | Meaning | Blocks Merge | | ------------ | ---------------------------------------------- | ------------ | | Critical | Will cause runtime failure | Yes | | High | Significant maintainability issue | Yes | | High | Unresolved assumptions (TODO(assumption) open) | Yes | | Medium | Should fix but not blocking | No | | Low | Suggestion for improvement | No |

Assumption Severity Rules:

• Unresolved // TODO(assumption): comments → High (blocking)
• Assumption contradicts spec → High (blocking)
• Assumption makes behavioral decision → High (blocking)
• Low-confidence assumption without justification → Medium
• Accepted assumption with documentation → No finding

Step 5: Generate Review Report

## Code Review Report

**Spec Group**: <spec-group-id>
**Files Reviewed**: 6
**Review Date**: 2026-01-14

### Summary

| Severity | Count |
| -------- | ----- |
| Critical | 0     |
| High     | 0     |
| Medium   | 2     |
| Low      | 3     |

**Verdict**: ✅ PASS

### Per Atomic Spec Review

#### as-001: Logout Button UI

- Files: src/components/UserMenu.tsx
- Quality: ✅ Clean
- Spec Conformance: ✅ Matches ACs
- Assumption Review: ✅ No assumptions

#### as-002: Token Clearing

- Files: src/services/auth-service.ts
- Quality: ⚠️ 1 Medium finding (M1)
- Spec Conformance: ✅ Matches ACs
- Assumption Review: ✅ 1 assumption (Accepted)

#### as-003: Post-Logout Redirect

- Files: src/router/auth-router.ts
- Quality: ✅ Clean
- Spec Conformance: ✅ Matches ACs
- Assumption Review: ✅ No assumptions

#### as-004: Error Handling

- Files: src/services/auth-service.ts
- Quality: ⚠️ 1 Medium finding (M2)
- Spec Conformance: ✅ Matches ACs
- Assumption Review: ✅ No assumptions

### Findings

#### Medium Severity

**M1: Function approaching length limit**

- **File**: src/services/auth-service.ts:45-90
- **Atomic Spec**: as-002
- **Issue**: `logout` function is 45 lines (approaching 50 line threshold)
- **Impact**: May become harder to maintain as feature grows
- **Suggestion**: Consider extracting token clearing logic

**M2: Missing return type**

- **File**: src/services/auth-service.ts:95
- **Atomic Spec**: as-004
- **Issue**: `handleLogoutError` has no return type annotation
- **Impact**: Type safety lost for consumers
- **Suggestion**: Add `Promise<void>` return type

#### Low Severity

[... suggestions ...]

### Positive Observations

- Good traceability - code comments reference atomic spec IDs
- Consistent use of Result type pattern
- Clear separation of concerns in handlers
- Test coverage matches Implementation Evidence

### Recommendations

1. Consider extracting token clearing logic (M1) in follow-up
2. Add return types to all public methods (M2)
3. Add JSDoc to public APIs (L1, L2) for better DX

Step 6: Handle Blocking Issues

If Critical or High severity issues found:

## Code Review Report

**Spec Group**: <spec-group-id>
**Verdict**: ❌ BLOCKED (2 High severity issues)

### Blocking Issues

**H1: Swallowed exception in error handling**

- **File**: src/services/auth-service.ts:78
- **Atomic Spec**: as-004
- **Issue**: Catch block returns null, hiding failure cause
- **AC Violation**: as-004 AC1 requires error message display
- **Impact**: Logout failures will be silent, hard to debug
- **Required Fix**: Throw AuthError with cause chain

**H2: Implementation deviates from spec**

- **File**: src/services/auth-service.ts:67
- **Atomic Spec**: as-002
- **Issue**: Clears ALL localStorage, spec says only auth_token
- **AC Violation**: as-002 AC1 specifies "clear authentication token"
- **Impact**: User data loss
- **Required Fix**: Change `localStorage.clear()` to `localStorage.removeItem('auth_token')`

**Action**: Fix blocking issues, then re-run code review.

Blocking on Unresolved Assumptions

When unresolved assumptions are found, the review produces verdict BLOCKED:

## Code Review Report

**Spec Group**: <spec-group-id>
**Verdict**: ❌ BLOCKED (1 unresolved assumption)

### Blocking Issues

**H3: Unresolved assumption - timeout duration**

- **File**: src/services/auth-service.ts:52
- **Atomic Spec**: as-002
- **TODO**: `// TODO(assumption): [HIGH] Assuming 5s timeout is sufficient - spec doesn't specify`
- **Issue**: Behavioral assumption made without escalation
- **Impact**: May cause timeout failures in slow network conditions
- **Resolution Required**: One of:
  - **Accept**: Add to atomic spec "Assumptions Made" section with justification
  - **Reject**: Remove assumption, use spec-defined value or escalate
  - **Escalate**: Propose spec amendment with timeout AC

### Assumption Summary

| File                         | Line | Assumption                  | Confidence | Status     |
| ---------------------------- | ---- | --------------------------- | ---------- | ---------- |
| src/services/auth-service.ts | 52   | 5s timeout sufficient       | HIGH       | Unresolved |
| src/services/auth-service.ts | 78   | Retry not needed on failure | MEDIUM     | Accepted   |

**Action**: Resolve all unresolved assumptions, then re-run code review.

Step 7: Update Manifest

Update manifest.json with code review status:

{
  "convergence": {
    "code_review_passed": true
  },
  "decision_log": [
    {
      "timestamp": "<ISO timestamp>",
      "actor": "agent",
      "action": "code_review_complete",
      "details": "0 critical, 0 high, 2 medium, 3 low - PASS"
    }
  ]
}

Convergence Loop

This gate runs under the Convergence Loop Protocol: check → fix → recheck until 2 consecutive clean passes or 5 iterations (escalate after 5).

• Check agent: code-reviewer
• Fix agent: implementer
• Gate name: code_review

After each clean pass, record via:

node .claude/scripts/session-checkpoint.mjs update-convergence code_review

Coercive enforcement: workflow-gate-enforcement.mjs blocks downstream dispatches when clean_pass_count < 2. See /challenge SKILL.md for full loop mechanics (state schema, fix agent input contract, escalation format).

Review Guidelines

Be Specific and Actionable

Bad:

Code quality could be better in auth.ts

Good:

**Quality: Function too long** (Medium)

- File: src/services/auth-service.ts:45-120
- Atomic Spec: as-002
- Issue: `validateSession` is 75 lines with 8 branches
- Impact: Hard to test, hard to modify safely
- Suggestion: Extract token parsing (L45-65) and permission check (L80-100)

Check Spec Conformance

Always verify implementation matches atomic spec:

**Spec Conformance: Extra feature added** (High)

- File: src/services/auth-service.ts:85
- Atomic Spec: as-002
- Issue: Auto-retry on failure added (not in spec)
- AC Violation: No AC mentions retry logic
- Impact: Undocumented behavior, may cause unexpected side effects
- Required Fix: Remove or add to spec via amendment

Reference Atomic Specs in Findings

Every finding should reference the atomic spec it relates to:

**H1: Missing error handling**

- **File**: src/router/auth-router.ts:45
- **Atomic Spec**: as-003 ← Always include this
- **Issue**: ...

Acknowledge Good Patterns

Include positive observations:

• Well-structured code
• Good test coverage
• Clear AC references in code
• Good traceability

Scope to Changes

Review what changed, not the entire codebase.

In scope: Files listed in atomic spec Implementation Evidence Out of scope: Pre-existing issues in unchanged files

Note pre-existing issues as "Pre-existing" - they don't block merge.

Review Checklist

For each atomic spec:

□ Implementation Evidence files reviewed
□ Code matches ACs in atomic spec
□ No undocumented features added
□ Naming follows project conventions
□ No obvious code duplication
□ Functions are reasonably sized (<50 lines)
□ Nesting depth acceptable (<4 levels)
□ Error handling matches spec
□ No `any` without justification
□ Public APIs have return types
□ Tests reference correct atomic spec IDs

## Assumption Review (Category H)

□ TODO(assumption) comments identified and reviewed
□ Each assumption's confidence level validated for appropriateness
□ Spec intent checked - no assumptions contradict requirements
□ Behavioral assumption contradictions flagged (should have been escalated)
□ Atomic spec "Assumptions Made" section matches code TODOs
□ Each assumption resolved: Accept / Reject / Escalate

Integration with Other Skills

Before code review:

• /unify to ensure spec-impl-test convergence

After code review:

• If PASS → Proceed to /security
• If BLOCKED → Use /implement to fix issues, then re-run /code-review

Full review chain after code-review:

1. /security - Security review (always)
2. /browser-test - UI validation (if UI changes)
3. /docs - Documentation generation (if public API)
4. Commit

Constraints

READ-ONLY

You report findings but do not modify code. Let Implementer fix issues.

Not Security Review

Focus on code quality. Security Reviewer handles:

• Injection vulnerabilities
• Authentication/authorization flaws
• Secrets exposure
• OWASP Top 10

Flag obvious security issues, but security review is the comprehensive check.

Examples

Example 1: Clean Pass

Input: Well-structured logout implementation (spec group sg-logout-button)

Review:

• as-001: ✅ Clean implementation
• as-002: ✅ Clean implementation
• as-003: ✅ Clean implementation
• as-004: ✅ Clean implementation

Output: ✅ PASS - No findings, proceed to security review

Example 2: Pass with Recommendations

Input: Feature implementation with minor issues

Review:

• as-001: ✅ Clean
• as-002: ⚠️ 1 Medium (long function)
• as-003: ✅ Clean
• as-004: ⚠️ 1 Medium (missing JSDoc)

Output: ✅ PASS with recommendations - Proceed, address in follow-up

Example 3: Blocked

Input: Implementation with spec deviation

Review:

• as-002: ❌ High (clears all localStorage instead of just auth_token)

Output: ❌ BLOCKED - Fix H1 before proceeding