/review — Risk-Tiered Code Review

Performs a structured code review with four risk tiers. Produces an AC Trace Matrix.

Optional council deliberation

When the user includes --council <profile-path>, follow .claude/commands/council.md after collecting the diff, acceptance criteria, and context below. For a PR use the pr source; for a local branch supply the captured diff and context as text. Treat the council transcript as additional review evidence, preserve dissent and limitations, and still complete the AC Trace Matrix. Never silently substitute the key-free demo for a real review. Council findings do not authorize code changes or GitHub publication.

Usage

/review                      # Review current branch vs main
/review feat/T-42-my-feature # Review specific branch
/review #17                  # Review PR by number

---

Step 1 — Get the Diff

# For a branch
git diff main..<branch> --stat      # File overview
git diff main..<branch>             # Full diff

# For a PR
gh pr diff <number>
gh pr view <number> --json title,body,commits

Also fetch the task details (if a Beads ID is findable from branch name or PR body):

bd show <TASK-ID> 2>/dev/null || true

---

Step 2 — Read the Context

Before reviewing, understand intent:

1. Read the PR/commit description
2. Read the Beads task and its AC (if available)
3. Read the knowledge file for the affected repo, if it has one: cat knowledge/repos/<repo>.yaml 2>/dev/null || echo "no knowledge file for <repo>". A repo has one only after /add-repo or /sync-knowledge has been run for it, and knowledge/repos/ is empty in a fresh checkout; with none, review from the diff and the files around it.
4. Spot-check 2-3 key files to understand the full context (not just the diff)

---

Step 3 — Risk-Tiered Analysis

Evaluate the diff against all four tiers. Rate each finding.

For UI-facing changes, also apply .claude/protocols/evaluation-rubric.md (visual design / originality vs generic patterns) in addition to the tiers below — surface HIGH findings when polish or product depth misses stated intent, not only when code is broken.

Blocker — Must fix. Blocks PASS.

Category	What to look for
Security	Hardcoded secrets, SQL injection, path traversal, XSS, auth bypass, unsafe deserialization
Data loss	Destructive operations without confirmation, missing rollback, irreversible mutations
Credentials	API keys, passwords, tokens, private keys committed
Critical bugs	Off-by-one in auth/payment code, null deref crashes in critical paths

High — Must fix. Blocks PASS.

Category	What to look for
Error handling	Uncaught exceptions in async code, silent failures, missing error boundaries
Type safety	any without justification, unsafe casts, missing null checks
Test gaps	New code paths with zero test coverage
Breaking changes	Changed public interfaces without migration path
Performance	N+1 queries, unbounded loops, blocking the main thread

Medium — Does not block. Create follow-up task.

Category	What to look for
Code quality	Deeply nested logic, functions >50 lines, duplicated logic
Edge cases	Missing validation for empty arrays, zero, empty strings
Observability	Missing logs/metrics for significant operations
Naming	Misleading variable or function names

Low — Optional. Minor polish.

Category	What to look for
Style	Minor formatting inconsistencies
Docs	Missing JSDoc for public APIs
Naming nits	Slightly unclear names (not misleading, just verbose)

---

Step 4 — AC Trace Matrix

Map each acceptance criterion to how it is verified:

## AC Trace Matrix

| Criterion | Verified By | Status |
|-----------|-------------|--------|
| User can update display name | `useUserSettings.test.ts:42` | ✓ |
| Error shows if API fails | `UserSettingsPage.test.tsx:89` | ✓ |
| Settings persist after refresh | Manual: navigate away and back | ✓ |
| Cannot set empty display name | `useUserSettings.test.ts:67` | ✓ |

If a criterion has no test and no documented manual verification: flag as High finding.

---

Step 5 — Rework (if needed)

If Blocker or High findings exist:

1. List all Blocker and High findings clearly
2. Ask the worker/author to address them
3. Maximum 2 rework iterations

After each rework: re-run the review against the updated diff. Log the result: bd comments add <TASK-ID> "review: iteration <n> — <finding counts>"

If issues remain after 2 iterations:

• Revert the implementation
• File a Beads bug with all findings
• Close the review with FAIL status

---

Step 6 — Final Verdict

PASS

• Zero Blockers
• Zero High findings (or all fixed)
• AC Trace Matrix complete

bd comments add <TASK-ID> "review: PASS — 0B/0H/2M/1L"

FAIL

• Any unfixed Blockers or High findings

bd comments add <TASK-ID> "review: FAIL — 1B/2H/0M/0L — reverted"

---

Output Format

## Code Review: feat/T-42-refresh-tokens

Files reviewed: 4 (+187 / -23 lines)
Task: T-42 — Add refresh token rotation

### Findings

🔴 BLOCKER (0)

🟠 HIGH (1)
  - src/api/auth.ts:87 — Missing error handling on token refresh failure.
    Async function with no try/catch; unhandled promise rejection in production.
    Fix: wrap in try/catch, return error state to caller.

🟡 MEDIUM (2)
  - src/hooks/useAuth.ts:34 — refreshToken function is 60+ lines. Consider splitting.
  - src/api/auth.ts — No logging on token rotation for audit trail.

⚪ LOW (0)

### AC Trace Matrix
| Criterion | Verified By | Status |
|-----------|-------------|--------|
| Token rotates on use | useAuth.test.ts:112 | ✓ |
| Old token rejected after rotation | useAuth.test.ts:134 | ✓ |
| Error state on network failure | ✗ NOT COVERED | — |

### Verdict: FAIL — 0B/1H/2M/0L
AC criterion "Error state on network failure" is untested.
Address HIGH finding and add error state test, then re-review.

Rework iteration: 1/2