--- name: code-review description: "Pre-commit self-review checklist, structured review requests, and processing review findings into tasks. Triggers: \"review this\", \"self-review\", \"pre-commit check\", \"code review\", \"review my changes\", \"check before commit\"." license: Apache-2.0 metadata: author: spur version: "1.0" platforms: "claude-code,codex,openclaw,opencode,antigravity" category: execution interactions: - technique operations: - self-review - request-review - process-findings see_also: - sp:code-verification - sp:code-implementation --- # sp:code-review — Code Review Workflow Two workflows: **pre-commit self-review** (catch issues before they leave your machine) and **requesting/receiving agent review** (structured context for deeper review). This skill owns the review *workflow*; `sp:code-verification` owns post-implementation SECUA review within the pipeline. ## Workflow A — Pre-commit self-review Run before `git commit`. Catches 60-80% of issues that a reviewer would flag. 1. **Diff it:** `git diff --cached` (or `git diff` if unstaged). Read every changed line. 2. **Checklist:** Walk the [self-review-checklist.md](references/self-review-checklist.md) — type-safety, null-handling, error-propagation, test-coverage, security-surface. Each category has diagnostic questions. 3. **Fix:** Anything the checklist flags → fix before committing. 4. **Commit:** Only when the checklist is clean. ## Workflow B — Requesting agent review When you want a deeper review (SECUA, architectural, or second-opinion): 1. **Prepare a structured brief** and prefer a fresh reviewer context/subagent when available: ```markdown WHAT_WAS_IMPLEMENTED: PLAN_OR_REQUIREMENTS: DIFF_RANGE: .. or "working tree" FOCUS_HINTS: ``` 2. **Attach evidence:** include the relevant `git diff`, self-review results (Workflow A output), and any verification commands already run. 3. **Request:** Invoke `sp:code-review` directly or trigger via "review my changes", "review this diff". 4. **Receive:** The reviewer produces a P1–P4 findings table. ## Workflow C — Processing review findings When you receive review findings: 1. **Verify each finding against the codebase first.** A finding can be stale or based on a misread; cite the file/line that proves it before changing code. Reasoned pushback is allowed. 2. **Fix in priority order:** blockers first (P1), then simple P2 fixes, then complex P2 fixes. Keep unrelated refactors out of the review-response diff. 3. **Test each fix individually** with the narrowest command that proves it, then run the task gate. 4. **Re-review:** After all P1/P2 fixes, request a follow-up review to confirm resolution. 5. **File follow-up tasks** for deferred P3/P4 items via `spur task create --template review`. ## Common Rationalizations | Rationalization | Reality | |---|---| | "It looks good to me — approve." | "LGTM" with no specific findings is not a review; it's a rubber stamp. State what you checked and what you found, with anchors. | | "It's a small diff, skim it." | Small diffs ship real bugs. Size does not lower the bar; run the same lenses regardless. | | "The author knows this area better — trust them." | Deference is not review. Your job is the independent second pair of eyes; verify, don't assume. | | "I'll soften this finding so it doesn't sound harsh." | Softening a real finding is dishonest and lets the defect through. State severity plainly; the diff, not the author, is under review. | | "There's dead code — I'll just delete it." | Deleting code you don't fully understand is risky. Identify it, list it, and ask before removing (Chesterton's Fence). | | "A new dependency is fine for this one helper." | Prefer stdlib / existing utilities. Flag every new dependency; a one-function import is rarely worth the supply-chain cost. | ## Red Flags - An approval with no specific, anchored findings. - Findings phrased to spare feelings instead of stating severity and impact. - Rubber-stamping because the author is senior or the diff is small. - Proposing a rewrite far larger than the change under review (scope creep in the review itself). - Deleting flagged dead code without confirming it is truly unreachable. - Waving through a new third-party dependency without justification. ## When to use - Before committing changes. - Before creating a PR or merge request. - After receiving review feedback. - The operator says "review this" or "check my changes before commit." Do **not** use this skill for: - Pipeline verification — that is `sp:code-verification` (`/sp:dev-verify`). - SECUA review of a completed task — that is `sp:code-verification` (`/sp:dev-review`). - Architectural review alone — that is `sp:sys-architecture`. ## References | Reference | Covers | |-----------|--------| | [self-review-checklist.md](references/self-review-checklist.md) | Pre-commit checklist: 6 categories with diagnostic questions | | [review-lenses.md](references/review-lenses.md) | SECUA review lenses: correctness, security, efficiency, usability, architecture | ## See also - **`sp:code-implementation`** — the implement step that produces the changes being reviewed.