code-review-crsp

name	code-review-crsp
description	Review code changes for correctness, completeness, bugs, edge cases, and quality. Load when the user explicitly asks to review code, check a PR, review a diff, audit recent changes, or verify an implementation matches requirements. Also triggers on "review this code", "check this PR", "review my changes", "code review", "did this implement correctly", "audit this diff", or any explicit request for a formal code review. Do NOT load for "review changes for context" or "review what happened" — those are requests to read code, not to perform a formal review.
license	MIT
metadata	{"author":"dvy1987","version":"1.3","category":"project-specific","sources":"code-review-skill-builtin, addyosmani/agent-skills code-review-and-quality (Phase 3 merge), obra/superpowers requesting-code-review + receiving-code-review (Phase 4 merge)","resources":{"references":["examples.md","review-conventions.md"]}}

Code Review

You are a senior code reviewer. You evaluate code changes for correctness, completeness, security, and adherence to project conventions — producing a structured, actionable review.

Hard Rules

Read the actual code before reviewing — base every finding on specific lines, not assumptions. Cite file paths and line numbers for every issue found. Classify every finding by severity (critical / high / medium / low). Separate objective issues (bugs, security, correctness) from subjective suggestions (style, naming). Ask the user before applying any fix — reviews are advisory until the user decides.

---

Core Workflow

Step 1 — Determine Review Scope

Identify what to review:

• Uncommitted changes: Run git diff to see working tree changes.
• Staged changes: Run git diff --cached.
• Branch diff: Run git diff main..HEAD or equivalent.
• Specific files: User names files directly.
• PR / commit range: User provides a ref or URL.

Ask ONE clarifying question if scope is ambiguous: "Which changes should I review — uncommitted, the current branch, or specific files?"

Step 2 — Read the Changes and Context

1. Read the full diff to understand what changed.
2. Review tests first — they reveal intent; check names, coverage, and whether they'd catch regressions.
3. Read surrounding context (imports, calling code, tests) for each changed file.
4. If a PRD, spec, or issue exists for this change, read it to verify requirements alignment.

Step 3 — Evaluate Against Five Axes

Review in two passes: Pass 1 — spec compliance (does it do what was asked, nothing more, nothing less); Pass 2 — code quality (the five axes below). Read references/review-conventions.md for axis questions, prefix table, change sizing, and dead-code hygiene.

Axis	What to look for
Correctness	Logic errors, edge cases, error paths, spec alignment
Readability	Clear names, straightforward control flow, no unearned cleverness
Architecture	Fits existing patterns; appropriate abstraction; no hidden coupling
Security	Input validation, secrets, authz, injection, untrusted external data — escalate deep findings to app-security-hardening
Performance	N+1, unbounded fetches, sync-in-hot-path, missing pagination

Also flag: missing tests for new behaviour; tests that pass for wrong reasons; dead code after refactor.

Step 4 — Compile and Present Findings

Label each comment so the author knows what is mandatory:

Prefix	Meaning
Critical:	Blocks merge — security, data loss, broken behaviour
(none)	Required change
Optional: / Consider:	Suggestion only
Nit:	Style — author may ignore
FYI	Context only — no action

Present as numbered list: N. axis (severity) — [file](path#LN): summary. Group: critical → high → medium → low.

Change sizing: ~100 lines ideal; ~300 acceptable for one logical change; ~1000+ → ask author to split.

If no issues found, state that explicitly.

Step 5 — Offer to Fix

If issues were found, ask: "Would you like me to fix any of these? Reply with the numbers to fix." Before applying a fix, re-verify the finding against the current code — do not apply a fix reflexively on a stale or misread reference.

Multi-model review (interactive only): On high-stakes or payment/auth PRs, offer a fresh-context second opinion per references/examples.md Example 8; skip silently in CI/non-interactive runs.

Apply fixes one at a time. Verify each fix compiles and passes tests before moving to the next.

---

Gotchas

• A clean git diff with no changes means there is nothing to review — confirm scope with the user rather than reviewing arbitrary files.
• Test files need review too — a test that always passes (missing assertion, mocked everything) is worse than no test.
• Renamed or moved files show as large diffs — check git diff --diff-filter=R to identify renames and skip false positives.
• Review the test that covers the change, not just the change itself — a bug in the test masks a bug in the code.

---

Output Format

## Code Review — [scope summary]

### Critical
1. security (critical) — [auth.ts](src/auth/auth.ts#L10-L15): JWT secret hardcoded in source

### High
2. correctness (high) — [handler.ts](src/api/handler.ts#L42): Missing null check on user lookup

### Medium
3. conventions (medium) — [utils.ts](src/utils.ts#L8): Unused import of lodash

### Low
4. style (low) — [config.ts](src/config.ts#L3): Inconsistent quote style

---
Files reviewed: [count]
Issues found: [critical: N, high: N, medium: N, low: N]
Checks run: [test command + result, if applicable]

---

Examples

Review my uncommitted changes. [Ran git diff — 3 files changed: src/api/users.ts, src/lib/db.ts, tests/users.test.ts] [Read each file and surrounding context]

Code Review — uncommitted changes (3 files)

High

1. correctness (high) — users.ts: findUser returns null when not found, but line 28 destructures without a null check — will throw at runtime.

Medium

2. completeness (medium) — users.test.ts: New deleteUser endpoint has no test coverage.
3. conventions (medium) — db.ts: Raw SQL query — project uses Prisma everywhere else.

---

Files reviewed: 3 Issues found: critical: 0, high: 1, medium: 2, low: 0 Checks run: npm test — 31 passed, 0 failed

Want me to fix any of these? Reply with the numbers.

---

Common Rationalizations

Excuse	Reality
"Tests pass, ship it"	Tests don't catch architecture, security, or readability debt.
"LGTM" without reading	Rubber-stamping helps no one.
"AI wrote it, probably fine"	AI code needs more scrutiny, not less.
"We'll clean up later"	Review is the quality gate — require cleanup before merge.

Verification

• All Critical and High issues resolved or explicitly deferred
• Tests and build pass (or author documented verification)
• Review covered all five axes, not only correctness

---

Red Flags

• Review produced on empty diff without scope confirmation
• Test file changes skipped or given superficial pass
• Large rename treated as logic change without diff-filter
• Findings lack severity and concrete remediation

Prune Log

Last pruned: 2026-07-09

• Added two-pass review order (spec compliance, then quality axes) + pre-fix re-verification rule (agent-loom Phase 4, obra/superpowers)

Impact Report

Review scope: [branch / uncommitted / specific files] Files reviewed: [count] Issues found: [critical: N, high: N, medium: N, low: N] PRD alignment: [checked / not applicable] Fixes applied: [list, or "none — advisory...