code-review

--- name: code-review description: "Review code with structured findings and optional reviewer persona. Invoke when user says: review code, review PR, review my changes, review diff, or review as . Supports: PR id, current diff, or specific files. Produces: categorized findings table, severity ratings, and concrete fix plan. Load reviewer persona with --as ." allowed-tools: - Bash - Read - Grep - Glob metadata: token-cost-tier: instruction token-cost-estimate: 4000 --- # /code-review — Code Review with Persona Support ## Usage ``` /code-review [--pr ] [--as ] [--files ] [--mode quick|standard] ``` - `--pr ` — review a specific PR by number - `--as ` — load `.claude/personas/.md` and apply that lens - `--files ` — review specific files (comma-separated or glob) - `quick` — surface only HIGH/CRITICAL findings - `standard` — full review across all categories (default) --- ## Step 0 — Announce Plan Before doing anything, output: ``` Code Review Plan (~ tokens): 1. [cli] Fetch code target ~ tok 2. [cli] Load persona (if --as used) ~100 tok 3. [instruction] Analyze across N categories ~3000 tok 4. [instruction] Build findings table ~500 tok 5. [instruction] Create fix plan ~1000 tok Total: ~ tokens ``` --- ## Step 1 — Fetch Code Target **If `--pr `:** ```bash bash .claude/scripts/gh/pr-diff.sh ``` Also get PR metadata: ```bash gh pr view --json title,body,author,additions,deletions,changedFiles 2>/dev/null ``` **If `--files `:** Read each file with Read tool. Keep to relevant sections if > 200 lines. **If neither (review staged/current diff):** ```bash git diff --staged 2>/dev/null || git diff HEAD~1 2>/dev/null | head -300 ``` --- ## Step 2 — Load Persona (if `--as` specified) Read `.claude/personas/.md`. Extract from persona file: - Top concern categories - Red flags that always trigger review comments - Approval criteria - Communication style Apply this lens throughout the review. Flag what **this reviewer** would flag, in **their style**. If persona file does not exist: ``` Persona '' not found at .claude/personas/.md Run: /persona-builder --reviewer Proceeding with standard review lens. ``` --- ## Step 3 — Review Categories Analyze the code diff/files across these categories. Skip categories with no relevant code in the diff. ### CAT-A: Architecture & Design - Does this change fit the existing architecture patterns? - Are new abstractions justified? (YAGNI violation check) - Is there coupling that should be decoupled? - Does this add circular dependencies? ### CAT-B: Code Quality - Are functions doing more than one thing? - Is there duplicated logic that should be extracted? - Are names descriptive? (No `tmp`, `data`, `obj`, `thing`) - Files > 200 lines that could be split? - Magic numbers / strings without named constants? ### CAT-C: Error Handling & Resilience - Are all error paths handled? - Are errors logged with enough context to debug? - Can this crash on unexpected input? - Are async operations properly awaited / `.catch()`ed? ### CAT-D: Security ```bash # Quick scan of the diff for known patterns git diff HEAD~1 2>/dev/null | grep -E "eval\(|exec\(|innerHTML|dangerouslySetInnerHTML|shell=True|password\s*=|api_key\s*=" | head -10 ``` - User-controlled input going into SQL/shell/eval → CRITICAL - Hardcoded credentials → CRITICAL - Missing input validation at API boundaries → HIGH - `dangerouslySetInnerHTML` without sanitization → CRITICAL ### CAT-E: Performance - N+1 query patterns (loop containing DB call)? - Missing indexes implied by new query patterns? - Expensive operations on hot paths (inside loops, request handlers)? - Large payloads being serialized/deserialized unnecessarily? ### CAT-F: Testability & Tests - Is this change covered by tests? - Are new functions testable (no hard dependencies injected)? - Are tests testing behavior or implementation details? - Are edge cases covered (empty input, null, boundary values)? ### CAT-G: Backwards Compatibility - Are public interfaces/APIs changed in breaking ways? - Are database migrations additive (no column drops in active use)? - Are deprecated items properly marked before removal? --- ## Step 4 — Findings Table ``` ## Code Review Findings | # | Category | Severity | Location | Issue | Suggestion | |---|----------|----------|----------|-------|-----------| | 1 | Security | CRITICAL | `auth.ts:45` | SQL query uses string concat with user input | Use parameterized query | | 2 | Quality | HIGH | `UserService.ts:120` | Function >50 lines doing 4 things | Split into fetchUser, validateUser, updateUser | | 3 | Error Handling | MEDIUM | `api/routes.ts:33` | Async handler missing try/catch | Wrap in try/catch or use asyncHandler wrapper | ... Severity: CRITICAL (must fix before merge) · HIGH (strongly recommended) · MEDIUM (should fix) · LOW (optional improvement) ``` If persona was loaded: prefix persona-specific findings with `[]`. --- ## Step 5 — Fix Plan Based on findings, propose a concrete fix plan ordered by severity: ``` ## Fix Plan CRITICAL (fix before merge): 1. `auth.ts:45` — Replace string concat SQL with: `db.query('SELECT * FROM users WHERE id = ?', [userId])` 2. ... HIGH (strongly recommended): 3. `UserService.ts:120` — Extract into 3 focused functions... ```typescript // Suggested split: async function fetchUser(id: string): Promise async function validateUserData(data: UserInput): Promise async function persistUser(user: User): Promise ``` ... MEDIUM / LOW: [List briefly, no code needed unless quick to show] ``` --- ## Step 6 — Verdict ``` ## Verdict APPROVE — 0 critical, 0 high findings APPROVE_WITH_NOTES — 0 critical, N medium/low findings REQUEST_CHANGES — 1+ high findings BLOCK — 1+ critical findings Overall: ``` If blocking findings exist: offer to implement fixes. ``` Found N critical/high findings. Implement fixes now? (y to proceed, n to review manually) ```