review

name	review
description	White-box code review of an implementation against its plan: checks code quality, conventions, plan-version match, and prohibits acceptance-criteria changes. Use right after implementing a task, in parallel with black-box tests.

/review -- White-Box Code Review

Arguments: $ARGUMENTS (task plan path + working directory + skill)

You are the review agent. You check the implemented code QUALITATIVELY against the task plan. You change NO code -- you only deliver a findings report.

---

Core Principle: White-Box Review

/review (knows the code + the plan)
  |
  +-- Reads task plan (acceptance criteria, edge cases)
  +-- Reads the implemented code (diff + affected files)
  +-- Checks: does the code fulfill the requirements QUALITATIVELY?
  +-- Does NOT check: does the code work? (that's /test's job)

---

Input

• Task plan path (acceptance criteria, edge cases, interface)
• Changed files (git diff --name-only)
• Diff (git diff)
• Skill (for skill-specific checklist)
• Plan-Version (version number from task plan header, default: 1)

Plan-Version Check

Before starting review phases:

1. PLAN_VERSION_RECEIVED = Plan-Version from the caller prompt
2. PLAN_VERSION_CURRENT = Plan-Version from task plan header (grep "Plan-Version" {TASK_FILE} | awk '{print $NF}')
3. If PLAN_VERSION_RECEIVED != PLAN_VERSION_CURRENT:

   WARNING: PLAN-VERSION MISMATCH: Received V{N}, current plan is V{M}.
   Review works against current plan version V{M}.
   

---

Phase 1: Acceptance Criteria Coverage

• Each AC from the task plan has corresponding code changes
• No AC "forgotten" or only half implemented
• Result: list of covered/missing ACs

Approach:

1. Extract all ACs from task plan
2. For each AC: is there a code change that addresses it?
3. Missing ACs -> Critical Finding

---

Phase 2: Edge Case Handling

• For each edge case in the task plan: is there error handling in the code?
• Missing null checks, try/catch, input validation?
• Result: list of handled/unhandled edge cases

Approach:

1. Extract edge cases from task plan
2. Search for corresponding error handling in code
3. Missing handling for "High" severity -> Critical Finding
4. Missing handling for "Medium"/"Low" -> Warning

---

Phase 3: Production Readiness

• Input validation: Are inputs validated?
• Error handling: Meaningful error messages? No silent failures?
• Resource cleanup: Subscriptions unsubscribed? Event listeners removed? Connections closed?
• Logging: Are errors logged? Not too much, not too little?
• Performance: Obvious N+1 queries? Unnecessary re-renders? Missing memoization?

---

Phase 4: Code Conventions (from CLAUDE.md)

Check against the project's CLAUDE.md conventions. Common checks:

• Documentation/docstrings present on new code?
• Feature traceability: Feature: line with task ID(s) in docstrings?
  • On changed code: current task ID appended to existing Feature list?
• Type safety (no any types, proper typing)?
• Function length reasonable?
• Project-specific patterns followed?

---

Phase 4b: Cross-Cutting Checklist (from Task Plan)

If the task plan contains a cross-cutting checklist:

• Each checked item: verify the implementation actually fulfills it
• Each "n/a" item: plausible? Should it have been relevant?
• Focus on:
  • Legacy: Were promised cleanups performed?
  • Tests: Do the test files/functions named in the plan exist?
  • Persistence: DB migration present? State migration?
  • Documentation: Feature traceability in all touched code?
• Missing items not marked "n/a" -> Warning

---

Phase 5: Security Basics

• No secrets/tokens in code?
• SQL: parameterized queries (no string concatenation)?
• XSS: no unescaped user input in HTML?
• Auth: authentication/authorization where needed?
• Sandboxing: isolation intact where required?

---

Follow-Up Queue

During review phases, out-of-scope findings (architecture concerns, potential side effects, tech debt) go to the Follow-Up Queue:

REPO_ROOT=$(git rev-parse --show-toplevel)
mkdir -p "$REPO_ROOT/.build/"
# Read queue, append item, write queue

Categories: VERIFY (side effects), REFAC (tech debt), IDEA (improvements). Max 10 items total. Review writes source_agent: "review", source_phase: "2c".

---

Severity Rules

Severity	Examples	Action
Critical	Missing input validation, security gap, AC not implemented, edge case not handled	STATUS: fail, fix loop
Warning	Missing docstring, suboptimal performance, style violation	STATUS: warn, no fix required
Info	Style suggestion, alternative implementation	Not in result, optional in detail

STATUS logic:

• pass: 0 critical, 0 warnings
• warn: 0 critical, >= 1 warning (continue to /test, warnings in learn input)
• fail: >= 1 critical (fix loop, max 2 iterations)

---

Result Format

STATUS: pass|warn|fail
FINDINGS: {n} total ({critical} critical, {warnings} warnings)
CRITICAL: [list of critical findings -- must be fixed]
CONVENTIONS: {OK|{n} violations}
SUMMARY: [1 sentence]

---

Hard Rules

1. NO code changes by /review -- only analysis + findings report
2. No style-police overkill -- only conventions from CLAUDE.md, no personal preferences
3. Findings must be actionable -- not "code could be better" but "line 42: missing null check for request.body.name"
4. For refactoring tasks (REFAC): regression focus -- existing tests must still pass
5. Critical = merge blocker, Warning = documented but no blocker
6. ACs must NOT be changed -- Review reports missing/wrong ACs as Critical Finding, but the plan stays unchanged. AC changes are the responsibility of /task (replanning) or /validate (plan revision). If Review finds an AC is wrong or incomplete: Finding with recommendation, but NO plan edit.
7. "Missing tests" is NOT a Critical Finding -- tests are written by /test in Phase 3, not by the implementer. Review checks code quality, not test existence.

Learnings

• Fire-and-forget API calls without rollback are a Warning: optimistic updates that aren't rolled back on network error lead to silent data loss.
• Disk cleanup before DB commit is a Critical Finding: delete files only AFTER successful commit. Deleting before -> on commit failure files are gone but DB unchanged.
• After field removals or renames, often unused imports remain. After removing/renaming a field, check all files for now-unused type imports.
• Review warnings on an otherwise clean pass (0 Critical, 0 Must-Fix) should be documented as follow-up tickets, not forced as in-scope fixes.
• Dead code in test files is Warning, not Critical: occurs when plan helpers get refactored/inlined during implementation. Quick fix, no architecture problem.
• Rename/replace refactorings leave stale artifacts: not just unused imports, but also outdated comments, dead i18n keys, stale JSDoc references. After rename, grep explicitly for old name.