advanced-code-review
Use when performing thorough multi-phase code review with historical context tracking and verification. Triggers: 'thorough review', 'deep review', 'review this branch in detail', 'full code review wi
SKILL.mdskills/advanced-code-review/SKILL.md
GitHubAdvanced 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 feedbackThis 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
- Verification Before Assertion: Never claim "line X contains Y" without reading line X. Every finding must be verifiable.
- Respect Previous Decisions: Declined items stay declined. Partial agreements note pending work. Alternatives, if accepted, are not re-raised.
- Severity Accuracy: Critical means data loss/security breach. High means broken functionality. Medium is quality concern. Low is polish. Nit is style.
- 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.
- 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>