code-review

# Code Review **The review-agent panel is the primary quality gate** (Rec 5, `docs/experiments/RECOMMENDATIONS.md`). The review-agent lens — SRP, complexity, coupling, duplication — was the only quality axis that separated workflow arms in the experiment line. Coverage and mutation scores saturate near-identically across every workflow shape and must **never** be used to rank workflow quality: the losing big-batch and split arms posted *higher* mutation scores (0.93–0.98) than the two winners (0.80–0.86). A higher coverage or mutation number is not evidence that code — or the workflow that produced it — is better. (The deterministic static-analysis pre-pass below is a different, complementary axis: mechanical findings cleared before the semantic panel runs, not a metric competing with it.) Role: orchestrator. Route work to review agents; do not review code yourself. Pass each agent's `model:`/`effort:` frontmatter as declared when dispatching — the harness resolves both fields natively before dispatch, per Model/Effort Resolution in `agents/orchestrator.md` (ADR 0026). Output templates and JSON schemas: [`output-format.md`](output-format.md). Example report: [`examples/sample-report.md`](examples/sample-report.md). ## Orchestrator constraints **MUST — confirm agent-dispatch capability before anything else in this skill (issue #1461).** Before attempting to dispatch ANY review agent (Step 4), you MUST confirm the `Agent` (or `Task`) tool is actually present and available in your current toolset. If it is not present: **STOP.** Do not proceed with a self-applied, inline, or checklist-based review of any kind as a substitute for independent dispatch — an orchestrator applying the review agents' checklists itself is not a review, it is self-certification, and it defeats the entire purpose of this gate. Do not write `.pr-review-passed` under any circumstance in this state. Instead, report to the user/operator plainly: code review cannot run in this environment because no agent-dispatch capability (`Agent`/`Task` tool) is available; name exactly what's missing; and state that the PR gate cannot be satisfied until `/code-review` is re-run from a session that has that capability. This is a hard requirement, not a preference — "should dispatch agents" is not sufficient; a missing `Agent`/`Task` tool always halts this skill before Step 2. 1. **Do not review code yourself.** Delegate all semantic analysis to review agents. 2. **Minimize context per agent.** Pass only what each agent's `Context needs` field requires. 3. **Route to the right model.** Each agent's `model:`/`effort:` frontmatter declares its model alias and reasoning effort; the harness resolves both fields natively before dispatch, per `agents/orchestrator.md` → Model/Effort Resolution (ADR 0026). Do not override the frontmatter value. 4. **Run deterministic gates first.** Lint, type-check, secret scan are cheaper than AI. Stop if they fail. 5. **Return structured results.** Aggregate agent JSON; do not add your own findings. 6. **Be concise.** Tables and JSON, no preambles, no filler. ## Parse Arguments Arguments: $ARGUMENTS | Flag | Behavior | | --- | --- | | `--agent <name>` | Run only the named agent (delegates to `/review-agent`) | | `--since <ref>` | Review files changed since the ref — see step 1 for the exact command (the `-c diff.relative=false -c core.quotePath=false` overrides there are load-bearing, not cosmetic) | | `--path <dir>` | Review only files in this directory | | `--all` | Force full-repository review even when uncommitted changes exist | | `--slice <N>` | Engage sliced large-repo review explicitly, capping each slice at N files (module-aligned) at any repo size. `N` must be a positive integer. See [`sliced-mode.md`](sliced-mode.md). | | `--resume` | Resume a sliced run — skip slices whose section artifact already exists on disk. See [`sliced-mode.md`](sliced-mode.md). | | `--no-slice` | Escape hatch — force the legacy single-pass review even on a large full-repo scope that would otherwise auto-engage sliced mode. | | `--json` | Output aggregated JSON to **stdout** instead of prose. Contractually non-interactive (for CI): never prompts; defaults to report-only (no code modified). | | `--expand <finding-id>|all` | Prose-mode only (step 7): render Tier-2 (full message + suggested fix) for the named finding-id, or for every finding with `all`, after the Tier-1 report — see step 7. A no-op under `--json` (see step 7's `--json` branch). **Only meaningful within the SAME run that computed the ids** — pass it alongside `--since`/`--path`/etc. in one invocation once you already know a specific id, e.g. because the operating Claude session read the prior Tier-1 output and is now re-invoking this skill with the same scope plus `--expand <id>` still in the same conversation; that path never re-dispatches anything beyond what the scope would have dispatched anyway. A cold, separate `/code-review --expand <id>` run with no memory of where that id came from IS a full re-dispatch of the panel (steps 1-6 run in full, same as any other invocation) and the id is not guaranteed to still exist or mean the same finding — `render_tiered_findings.py`'s own docstring says ids are not stable across runs. `--expand` never triggers a SECOND panel dispatch on top of an already-running one; it only changes step 7's rendering of the one panel a given invocation already ran. | | `--pdf` | After the durable report is written, also render it to a sibling PDF via `hooks/lib/report_pdf.py`. See `knowledge/report-pdf-integration.md`. No-op with a message when no report file is written (`--json` or `--internal`); under `--json`, that status goes to **stderr** so stdout stays pure JSON. Additive: never changes the review's own output or exit status. | | `--internal` | This is an orchestrator-internal dispatch (`/build`'s Step 6 backstop review, `/test-improve`'s Phase 4/5 end-of-phase review loop) — skip the `.dev-team-reports/code-review.md` report write in step 7. Orthogonal to `--json`: `--internal` alone still runs the prose/fix-loop path; both sanctioned callers use `--internal` without `--json` specifically to keep the fix loop. `/build` and `/test-improve` are the only sanctioned callers of this flag today — see `knowledge/report-output-location.md` for `/ship`'s deliberate exception (writes the report by default, no `--internal`). | | `--init-risks` | Scaffold `ACCEPTED-RISKS.md` from `templates/ACCEPTED-RISKS.md.tmpl` if absent. Exits non-zero without overwriting if present. Schema: `knowledge/accepted-risks-schema.md`. | | `--force` | Skip pre-flight gates **and the documentation-only short-circuit** (forces a full review of doc-only changes). **Requires `--reason "<text>"`** — logged to `.claude/metrics/override-audit.jsonl`. | | `--reason "<text>"` | Override justification (required with `--force`) | | `--static-analysis` / `--no-static-analysis` | Force on/off the static analysis pre-pass (Semgrep, ESLint, TypeScript, Ruff, mypy). Auto-enabled when tools are detected. | | `--background` | Drift review mode — review default branch for documentation, naming, and structural drift. Runs doc-review, arch-review, naming-review, structure-review only. Skips pre-flight gates. | | (no flags) | **Auto-scope**: review uncommitted changes if any exist, otherwise full repository | ## Progress tracking ```text - [ ] Target files determined - [ ] Documentation-only check (short-circuit if all docs) - [ ] Pre-flight gates passed - [ ] Static analysis pre-pass (if enabled) - [ ] Agents loaded and filtered - [ ] All agents executed - [ ] Results aggregated - [ ] User asked: fix or report only? - [ ] Review-fix loop (if user chose fix, up to 5 iterations) - [ ] Report generated - [ ] Correction prompts saved - [ ] Pre-commit gate file written (if auto-scoped to uncommitted changes) ``` ## Steps ### 1. Determine target files Priority order: 1. `--path <dir>` — files in that directory (exclude node_modules, .git, dist, build, coverage) 2. `--since <ref>` — `git -c diff.relative=false -c core.quotePath=false diff --name-only <ref>...HEAD` 3. `--all` — all source files 4. **Auto-scope** (no flags): run `git -c diff.relative=false -c core.quotePath=false diff --name-only` + `git -c diff.relative=false -c core.quotePath=false diff --cached --name-only`, combine and dedupe. If non-empty, review those files. If empty, review the full repository. The explicit `-c diff.relative=false` matters here (#1461 fourth security re-review): a repo/global `diff.relative=true` config would otherwise silently scope this listing to the invocation's cwd, and `review_gate_hash()`/`_staged_names()` (which pin the same override) would then hash/gate a broader staged patch than what was actually reviewed. `-c core.quotePath=false` (#1733) keeps this listing byte-identical to step 3's `changed_file_list.py` input for the same ref/scope — without it, a non-ASCII path would arrive C-quoted here but raw there, and `select_lenses.py`'s `--added` membership test (an exact string comparison) would silently fail to match it. **Stage auto-scoped changes now, before anything else (#1461).** When the auto-scope path found a non-empty file set, `git add` those files immediately — before pre-flight gates, static analysis, or any agent dispatch — so the staged content's hash is fixed from this point through step 9's gate write. This is not cosmetic: `agent_dispatch_ledger.py` stamps each review-agent dispatch's `subject_hash` with `review_gate_hash()` at **dispatch time** (step 4). If staging happened only at step 9 (after dispatch) as previously documented, the dispatch-time hash and the gate-write-time hash would differ whenever the auto-scope target was unstaged — the common case — and every genuine dispatch would silently fail to corroborate the gate, forcing a hard block on a fully legitimate review. Staging here, before dispatch, is what makes step 9's hash and the dispatch ledger's `subject_hash` the same value. An unstaged working-tree edit after this point does **not** by itself change the staged hash (`review_gate_hash()` hashes `git diff --cached`, not the working tree) — step 6a's fix loop explicitly re-stages (`git add`) each iteration's fixes for exactly this reason; see that step for how corroboration is re-established after a fix loop runs. **Never `Read` a directory path directly to enumerate its contents** — `Read` on a directory throws `EISDIR` (the same hazard step 3 avoids for agent-roster enumeration). This applies to `--path <dir>`, `--all`, and the full-repository fallback alike: always list files with `Glob` (e.g. `Glob("<dir>/**/*")`), never a bare `Read` on the directory itself. See `${CLAUDE_PLUGIN_ROOT}/knowledge/directory-enumeration.md` for the shared rule. **Scope validation** (full-repo paths only): | File count | Action | | --- | --- | | ≤200 | Proceed | | 201–500 | Warn: "Reviewing {N} files — consider `--path` to narrow scope." Proceed. | | >500 | **Auto-engage sliced mode** (large-repo review) unless `--no-slice`. | **Sliced large-repo review.** On a full-repo scope exceeding the >500 tier (or whenever `--slice <N>` is passed), **auto-engage sliced mode**: run the sliced path in [`sliced-mode.md`](sliced-mode.md) instead of steps 4–9 below. That file owns the full activation precedence (via `scripts/activation.py`), partitioning, per-slice panels, persist-and-drop, `--resume`, and cross-slice consolidation — not restated here. `--no-slice` forces the legacy single-pass review (steps 2–9) even past the threshold; Exactly at 500 files does not auto-engage. **Non-full-repo scope** (`--path`, `--since`, auto-scoped uncommitted changes) **never** auto-engages, regardless of file count — the review proceeds exactly as before this feature. Sliced mode is **report-only** (no interactive fix loop). **Documentation-only short-circuit.** After the target set is known, classify each file. A file is **documentation** when it matches a doc type or path: - extension `.md`, `.mdx`, `.markdown`, `.rst`, `.txt`, `.adoc` - any path under a `docs/` directory - a root doc: `README*`, `CHANGELOG*`, `CONTRIBUTING*`, `LICENSE*`, `NOTICE*`, `AUTHORS*`, `CODE_OF_CONDUCT*` …**except functional Claude-config markdown, which is never documentation** (it drives agent/skill/command behavior and must be reviewed): any path containing a `.claude/` segment, or under `agents/`, `skills/`, `prompts/`, `knowledge/`, or `templates/agents/`. Treat `CLAUDE.md` and `AGENTS.md` as functional config too, not documentation. If **every** target file is documentation, short-circuit: 1. Emit: `Documentation-only changeset ({N} files) — skipping code review. Re-run with --force --reason "<text>" to review anyway.` 2. If the review was auto-scoped to uncommitted changes or scoped via `--since <base>` (issue #1904 Bug 2b — same extension as step 9's own gate condition, and for the same reason: `/pr`'s only path to `gh pr create` reviews via `--since <base>`), write the `.pr-review-passed` gate file (per step 9) so `hooks/pre_pr_review.py` allows the next `gh pr create`. **Contemporaneously** (before or immediately after that write), record the doc-only exemption as an explicit, auditable boundary event — the `.pr-review-passed` gate's dispatch-ledger corroboration (#1461, #1886) reads this event, bound to the gate's own hash, to let the doc-only path stay exempt from agent-dispatch evidence without being a silent, unaccountable code-path skip: ```bash HASH=$(python3 "${CLAUDE_PLUGIN_ROOT}/hooks/lib/review_gate_hash.py" --branch-diff) mkdir -p .claude/memory && echo "$HASH" > .claude/memory/.pr-review-passed python3 "${CLAUDE_PLUGIN_ROOT}/hooks/lib/boundary_events.py" --event doc-only --subject-hash "$HASH" ``` 3. In `--json` mode, emit `{"status": "skipped", "reason": "documentation-only", "files": [<list>]}` instead. 4. **Stop.** Do not run pre-flight gates, static analysis, or any agent. **Bypass:** the short-circuit does **not** apply with `--force` (with `--reason`), `--agent <name>`, or `--background` (drift review always inspects docs). ### 1b. Check for institutional context If `REVIEW-CONTEXT.md` exists at the repo root, read it and pass its contents to every agent in step 4, prefixed with: "Institutional context provided for this review:". This file is optional. ### 1c. Probe for optional MCP tools | Tool | Check | Use | | --- | --- | --- | | RoslynMCP | `get_code_metrics` / `search_symbols` available | C# metrics, compiler diagnostics | | CodeGraph | `.codegraph/` present / `mcp__codegraph__codegraph_explore` available | Verified structural skeletons, resolved callers/callees/impact | | Repowise | `get_context` / `get_symbol` / `search_codebase` / `get_risk` available | Verified file/symbol context + modification-risk lookups | | Documentation MCP | wiki/docs search available | Architecture docs | | Semgrep | `which semgrep` | SAST context for security-review | Pass availability info to each agent so they can use enhanced tools or fall back to Glob/Grep/Read. All read-only review agents grant these MCP tools; see [`knowledge/codegraph-vs-graphify.md`](../../knowledge/codegraph-vs-graphify.md) for tool selection and the fallback contract. Include availability in the final report per `knowledge/review-template.md`. ### 2. Pre-flight gates Skip entirely if `--background`. If `--force` without `--reason`, halt: ``` ERROR: --force requires --reason "<justification>". ``` If `--force` with `--reason`, append an entry to `.claude/metrics/override-audit.jsonl` per the schema in [`output-format.md`](output-format.md#override-audit-log-entry-step-2---force-path), then proceed to step 3. Otherwise run these in sequence (stop on first failure):