Code Review

Code Review

When to Use This Skill

Load this skill when:

• Verifying a task's implementation against its acceptance criteria (Verifier agent)
• Conducting specification compliance checks on executor output
• Performing security review on high-risk task implementations
• Deciding the appropriate review depth for a task

Review Depth Tiers

The coordinator determines review depth before delegating to the verifier. Use this decision tree:

Is the task security-sensitive (auth, crypto, input validation, SQL)?
  → YES: review_depth = "full"
  → NO: Is the task high-priority OR complex (estimated_effort = "large")?
    → YES: review_depth = "full"
    → NO: Is the task documentation/config only?
      → YES: review_depth = "lightweight"
      → NO: review_depth = "standard"

Full Review

When: Security-sensitive tasks, high-priority tasks, large effort tasks.

Procedure:

1. Read every file in files_modified line-by-line
2. Verify each acceptance criterion with concrete evidence
3. Run all relevant tests and capture output
4. Check for regressions (other files not broken, existing tests still pass)
5. OWASP security scan (see OWASP Quick Reference below)
6. Specification compliance check (required patterns present, forbidden patterns absent)
7. Code quality assessment (naming, function size, error handling, DRY)

Evidence required: File content excerpts, test output transcripts, command results.

Standard Review

When: Typical tasks with moderate risk.

Procedure:

1. Check each acceptance criterion individually with evidence
2. Verify file changes match files_to_modify from the plan
3. Spot-check code quality (naming, obvious anti-patterns, error handling)
4. Verify no obvious regressions (quick scan of modified files' dependents)
5. Specification compliance check

Evidence required: Per-criterion evidence (file content or command output proving pass/fail).

Lightweight Review

When: Documentation changes, configuration updates, low-risk tasks.

Procedure:

1. Verify listed files exist
2. Check basic content correctness (file non-empty, expected structure present)
3. Verify no unintended changes to other files

Evidence required: File existence confirmation, basic content snippets.

Verification Methodology

Per-Criterion Evidence Gathering

For every acceptance criterion in the task definition:

1. IDENTIFY — Read the criterion text carefully
2. LOCATE — Find the relevant code/file/output
3. EXAMINE — Read the actual file content (never trust claims)
4. EVIDENCE — Capture concrete proof
5. JUDGE — Determine PASS or FAIL based on evidence
6. RECORD — Document the criterion, result, and evidence

Critical rule: NEVER trust the executor's self-assessment. Always verify independently.

Specification Compliance

Check that the implementation follows locked-in decisions:

required_patterns:
  For each pattern in specification_adherence.required_patterns:
    1. Search for the pattern in modified files
    2. Verify it is correctly applied (not just present, but correct)
    3. Record: pattern name, found in file(s), correctly applied (yes/no)

forbidden_patterns:
  For each pattern in specification_adherence.forbidden_patterns:
    1. Search for the pattern in modified files
    2. If found → FAIL with specific location
    3. Record: pattern name, found (yes/no), location if found

tech_choices:
  For each choice in specification_adherence.tech_choices:
    1. Verify the correct technology is used (not a substitute)
    2. Record: choice name, implemented correctly (yes/no)

Code Quality Assessment

For full and standard review depths:

Regression Check

1. Identify files that import from or depend on modified files
2. Check that modified exports haven't changed in breaking ways
3. If tests exist for modified files, verify they still pass
4. Check build/type-check status (run tsc or equivalent)
5. Record: what was checked, passed (yes/no), details

Evidence Types

File Content Evidence

Read the actual file and extract the relevant section:

criterion: "File exports rateLimitMiddleware function"
result: pass
evidence: |
  src/middleware/rateLimit.ts:15 —
  export function rateLimitMiddleware(options: RateLimitOptions): Middleware {
    // ... full implementation present (42 lines)
  }

Test Output Evidence

Run the test and capture results:

criterion: "All search tests pass"
result: pass
evidence: |
  $ npm test -- --grep "search"
  ✓ returns paginated results (23ms)
  ✓ handles empty query (5ms)
  ✓ validates page_size parameter (8ms)
  3 passing (36ms)

Command Result Evidence

Run a command and capture output:

criterion: "TypeScript compiles without errors"
result: pass
evidence: |
  $ npx tsc --noEmit
  (exit code 0, no output)

Structural Check Evidence

Verify file existence, exports, or configuration:

criterion: "Config file includes search endpoint"
result: pass
evidence: |
  File exists: src/config/routes.ts
  Line 24: { path: '/api/search', handler: searchHandler }

OWASP Quick Reference

For full review depth, check against the OWASP Top 10:

Common Verification Pitfalls

Avoid these mistakes that lead to false PASS verdicts:

Verification Output Format

The verifier must produce this exact output structure:

verifier_output:
  verdict: "PASS | FAIL"
  criteria_results:
    - criterion: "GET /api/search returns 200 with results array"
      result: "pass"
      evidence: "Response body: { results: [...], total: 42 }. Status: 200."
    - criterion: "Invalid query returns 400 with error message"
      result: "pass"
      evidence: "GET /api/search?page=-1 returns 400: { error: 'Invalid page number' }"
  specification_compliance:
    compliant: true
    violations: []
  code_quality:
    issues:
      - "Function searchHandler is 52 lines — consider extracting validation logic"
  regression_check:
    passed: true
    details: "Existing tests pass (npm test: 47 passing). TypeScript compiles (tsc --noEmit: clean)."
  overall_summary: "All 4 acceptance criteria verified with evidence. Code quality is good with one minor suggestion. No regressions detected."
  failure_details: null

On FAIL verdict, failure_details must include:

• Which criteria failed and why
• Specific file and line references
• Exact fix instructions for the executor retry

failure_details: |
  FAILED criterion: "Invalid query returns 400 with error message"
  Issue: GET /api/search?page=-1 returns 500 instead of 400.
  Location: src/api/search.ts:34 — missing input validation before query execution.
  Fix: Add parameter validation at the start of the handler function.
  Validate: page >= 1, page_size between 1-100, q is a non-empty string.

Verification

A thorough verification meets ALL of these criteria:

1. Every acceptance criterion checked: No criterion is skipped or assumed
2. Evidence is concrete: Every PASS/FAIL includes file content, test output, or command results
3. Independence verified: No evidence relies solely on executor's self-assessment
4. Specification compliance checked: Required patterns confirmed present, forbidden patterns confirmed absent
5. Appropriate depth applied: Review depth matches the task's risk profile
6. Regressions considered: For full/standard, existing functionality confirmed unbroken
7. Failure details are actionable: If FAIL, the executor can fix the issue from the details alone
8. Output format correct: verifier_output follows the exact schema above