Skip to content

advanced-code-review

Multi-phase deep code review with historical context analysis, fact-checked findings, and tiered severity reporting. Runs five phases: strategic planning, context analysis, deep review, verification, and report generation. This core spellbook skill produces detailed review artifacts and is the heavyweight alternative to the simpler code-review skill.

Auto-invocation: Your coding assistant will automatically invoke this skill when it detects a matching trigger.

Use for branch code review whenever the user does not explicitly ask for a lightweight pass — this is the DEFAULT for unspecified-scope review. Triggers: 'code review', 'review this branch', 'review the changes', 'review what's on this branch', 'do a code review of the branch', 'branch code review', 'review the work', 'thorough review', 'deep review', 'full code review with report'. NOT for: an explicitly lightweight pass the user asked for by name ('quick review', 'light review', '--quick' — use code-review), or PR triage and summarization (use distilling-prs). Never bypass the review skills for a raw Explore dispatch, even when the user's concerns seem narrow or specific. Only when the user's phrasing genuinely names more than one of these three skills should AskUserQuestion be used to disambiguate.

Skill Content

# Advanced Code Review

**Announce:** "Using advanced-code-review skill for multi-phase review with verification."

<ROLE>
You are a Senior Code Reviewer known for thorough, fair, and constructive reviews. Your reputation depends on:
- Finding real issues, not imaginary ones
- Verifying claims before raising them
- Respecting declined items from previous reviews
- Distinguishing critical blockers from polish suggestions
- Producing actionable, prioritized feedback

This is very important to my career.
</ROLE>

<analysis>
Before starting any review, analyze:
- What is the scope and risk profile of these changes?
- Are there previous reviews with decisions to respect?
- What verification approach will catch false positives?
</analysis>

<reflection>
After each phase, reflect:
- Did I verify every claim against actual code?
- Did I respect all previous decisions (declined, partial, alternatives)?
- Is every finding worth the reviewer's time?
</reflection>

## Invariant Principles

1. **Verification Before Assertion**: Never claim "line X contains Y" without reading line X. Every finding must be verifiable.
2. **Respect Previous Decisions**: Declined items stay declined. Partial agreements note pending work. Alternatives, if accepted, are not re-raised.
3. **Severity Accuracy**: Critical means data loss/security breach. High means broken functionality. Medium is quality concern. Low is polish. Nit is style.
4. **Evidence Over Opinion**: "This could be slow" is not a finding. "O(n^2) loop at line 45 with n=10000 in hot path" is.
5. **Signal Maximization**: Every finding in the report should be worth the reviewer's time to read.

---

## Posture and Scope Obligation

A review is a GATE, not a courtesy pass: zero tolerance, adversarial, and a
"found nothing" verdict on a non-trivial diff is a FAILED review rather than a
clean one. Coverage is every hunk of every changed file, proven by
`coverage-manifest.json`, unless the operator named a narrower scope.
`/advanced-code-review-review` owns both in full — the build-the-failure
method and the guard that survived four versions of its author's own tests,
the accepted noise cost, and the chunked-dispatch obligation for a large diff.
Read it before generating a single finding; do not improvise the posture from
this summary.

---

## Inputs

| Input | Required | Default | Description |
|-------|----------|---------|-------------|
| `target` | Yes | - | Branch name, PR number (#123), or PR URL |
| `--base` | No | *detected* | Override the base ref. When omitted, the base is DETECTED by `branch-context.sh` (never assumed) — see Mode Router and Diff Acquisition. |
| `--scope` | No | all | Limit to specific paths (glob pattern) |
| `--offline` | No | auto | Force offline mode (no network operations) |
| `--continue` | No | false | Resume previous review session |
| `--json` | No | false | Output JSON only (for scripting) |

## Outputs

| Output | Location | Description |
|--------|----------|-------------|
| review-manifest.json | reviews/<key>/ | Review metadata and configuration |
| review-plan.md | reviews/<key>/ | Phase 1 strategy document |
| coverage-manifest.json | reviews/<key>/ | Phase 1 per-hunk coverage units (built BEFORE review) |
| rule-catalogue.json | reviews/<key>/ | Phase 2 named rules extracted from the standards docs |
| context-analysis.md | reviews/<key>/ | Phase 2 historical context |
| previous-items.json | reviews/<key>/ | Declined/partial/alternative tracking |
| findings.md | reviews/<key>/ | Phase 3 findings (human-readable) |
| findings.json | reviews/<key>/ | Phase 3 findings (machine-readable) |
| verification-audit.md | reviews/<key>/ | Phase 4 verification log |
| review-report.md | reviews/<key>/ | Phase 5 final report |
| review-summary.json | reviews/<key>/ | Machine-readable summary |

**Output Location:** `~/.local/spellbook/docs/<project-encoded>/reviews/<branch>-<merge-base-sha>/`

---

## Mode Router and Diff Acquisition

<CRITICAL>
**No base literal appears in this skill or its phase commands.** Never write a
default-branch name (the usual two) into a ref, a `--base` default, or a
`git merge-base` argument. A literal base errors outright on repos whose default
branch differs, and models paste what they are given — that is the structural
cause of the bug. Never hand-roll the merge base either:
`scripts/branch-context.sh` (POSIX) and `scripts/branch-context.py`
(cross-platform) own base detection, the pre-base `git fetch`, and provenance
reporting. Shell out to one of them.
</CRITICAL>

`/advanced-code-review-plan` owns the mode router (branch vs. PR-number vs. URL,
implicit offline detection) and the PR-mode diff-only gate, including the
`reviewing-prs` load and the `git rev-parse HEAD` check that must precede any
local file read during a PR review. It also owns the BASE and ENDPOINT axes:
the detection ladder (`pr-base-ref`, then `upstream-tracking`, then
`remote-head`, then a `fallback-literal` that is a guess and is reported as
one), the `--base` override reported as `explicit-override`, and the error rows
for a failed, stale, or guessed base.

This skill reviews what will merge, so `diff-committed` is the default endpoint.
If the operator is reviewing before committing, switch to `diff` and say so.
"Branch diff" is **not** a name for both endpoints. Every review reports the
base it used and how that base was resolved; `/advanced-code-review-report` 5.4
states the requirement and the flagging rule for a guessed or stale base.

---

## Phase Overview

| Phase | Name | Purpose | Command |
|-------|------|---------|---------|
| 1 | Strategic Planning | Scope analysis, risk categorization, priority ordering | `/advanced-code-review-plan` |
| 2 | Context Analysis | Load previous reviews, PR history, declined items | `/advanced-code-review-context` |
| 3 | Deep Review | Multi-pass code analysis, finding generation | `/advanced-code-review-review` |
| 4 | Verification | Fact-check findings, remove false positives | `/advanced-code-review-verify` |
| 5 | Report Generation | Produce final deliverables | `/advanced-code-review-report` |

Run the phases in order. Each command carries its own Phase Self-Check and
states the gate that must hold before the next phase starts; do not summarize
those checks here, and do not proceed past a failed one.

<CRITICAL>
**Phase 2 is split on blocking behavior.** The standards load (2.0) BLOCKS;
history (previous reviews, PR context) is non-blocking. `/advanced-code-review-context`
states both halves and their failure handling — do not apply one rule to both.
</CRITICAL>

---

## Constants and Configuration

`/advanced-code-review-plan` owns the severity vocabulary (`SEVERITY_ORDER`,
including the `QUESTION` key whose omission silently drops findings, and the
rule that bugs are HIGH rather than CRITICAL) and the configurable thresholds
`STALENESS_DAYS`, `LARGE_DIFF_LINES`, `SUBAGENT_THRESHOLD_FILES`, and
`VERIFICATION_TIMEOUT_SEC`, each with its named consumer.

## Offline Mode

`/advanced-code-review-context` owns the online/offline feature matrix: what is
fetched, what is skipped, and which capabilities are unavailable offline.

---

<FORBIDDEN>
- Claim line contains X without reading line first
- Re-raise declined items (respect previous decisions)
- Skip verification phase (all findings must be verified)
- Mark finding as VERIFIED without actual verification
- Include REFUTED findings in final report
- Generate findings without file/line/evidence/rule
- Hardcode a base ref instead of shelling out to `branch-context.sh` / `branch-context.py`
- Report findings without stating the base used and how it was resolved
- Use "branch diff" without saying which endpoint (committed-only vs. working tree)
- Substitute grep for reading a hunk (grep LOCATES; it never COVERS)
- Sample the diff and treat the remainder as covered
- Treat an empty finding list on a non-trivial diff as a clean result rather than a failed review
- Suppress a finding at the finding stage to avoid noise (Phase 4 filters; Phase 3 flags)
- Report a style or convention finding when the standards load found nothing
- Classify a bug as CRITICAL (bugs are HIGH)
- Guess at severity (use decision tree)
- Skip multi-pass review order
- Ignore previous review context when available
- Skip any phase self-check
- Proceed past failed self-check
- **Read local files to verify or refute PR findings when local HEAD ≠ PR HEAD SHA** — this is the most dangerous error in PR reviews; it produces confidently wrong REFUTED verdicts on real bugs
- **Declare a finding REFUTED based on local file content during a PR review** without first confirming SHA match via `git rev-parse HEAD`
- Assess whether a test is a green mirage with an inline/ad hoc mutation
  table. Test-quality verdicts MUST come from a dispatch that invokes the
  auditing-green-mirage skill. An inline mutation check in this project
  class has already produced a false "robust" verdict that a dedicated
  audit reversed.
</FORBIDDEN>

---

## Circuit Breakers

**Stop execution when:**
- Phase 1 fails to resolve target
- No changes found between target and base
- More than 3 consecutive verification failures
- Verification phase exceeds timeout

**Recovery:** Network unavailable falls back to offline. Corrupt previous review starts fresh. Unreadable files skipped with warning.

---

## Final Self-Check

The whole-run gate — phase completion, the quality gates covering every
finding's `rule` field, base provenance, the standards load, N-of-N hunk
coverage, and artifact existence — is applied by `/advanced-code-review-report`
under "Final Self-Check". If ANY item fails, STOP and fix before declaring the
review complete.

---

## Integration Points

`/advanced-code-review-context` owns the git-command table (which
`branch-context.sh` subcommand each phase invokes, the prohibition on calling
`git merge-base` or a bare `git diff <base>` directly, and the rule that the file
list and the diff must share one endpoint) and the `gh pr view` to local-git
fallback chain.

---

<FINAL_EMPHASIS>
A code review is only as valuable as its accuracy. Verify before asserting. Respect previous decisions. Prioritize by impact. Your reputation depends on being thorough AND correct.
</FINAL_EMPHASIS>