ce-code-review

# Code Review Reviews code changes using dynamically selected reviewer personas. Spawns parallel sub-agents that return structured JSON, then merges and deduplicates findings into a single report. ## When to Use - Before creating a PR - After completing a task during iterative implementation - When feedback is needed on any code changes - Can be invoked standalone - Can run inside larger workflows; use `mode:agent` when the caller needs JSON instead of markdown tables ## Argument Parsing Parse `$ARGUMENTS` for optional tokens. Strip each recognized token before interpreting the remainder as a PR number, GitHub URL, or branch name. | Token | Example | Effect | |-------|---------|--------| | `mode:agent` | `mode:agent` | **Report-only**: return **JSON** instead of markdown tables and skip the Stage 5c apply (the caller applies). Does not change reviewer selection, merge logic, or scope rules (see Output format) | | `mode:headless` | `mode:headless` | **Deprecated alias** for `mode:agent` | | `mode:report-only` | `mode:report-only` | **Deprecated — ignored.** Former no-artifacts mode; default behavior is review-only without checkout | | `base:<sha-or-ref>` | `base:abc1234` or `base:origin/main` | Diff base on the **current checkout** (explicit; skips auto base detection) | | `plan:<path>` | `plan:docs/plans/2026-03-25-001-feat-foo-plan.md` | Plan file for requirements verification (explicit). Supports markdown and HTML unified plans. | | `depth:full` | `depth:full` | **Force the full reviewer roster** — skip the Stage 3c small-diff lite path so every always-on persona runs regardless of diff size. Use when a deep/thorough review is explicitly requested (the one escalation signal Stage 3c cannot infer from the diff). Does not change conditional selection, merge, or scope. | | `depth:auto` | `depth:auto` | **Default** — self-right-size via Stage 3c (lite roster for trivial, low-risk, code-only diffs; full roster otherwise). | | `grouping:auto` | `grouping:auto` | **Default** — build thematic triage groups when findings span distinct concerns (Stage 5 step 9b) | | `grouping:off` | `grouping:off` | Suppress triage groups: no Triage Groups section, empty `triage_groups` in JSON | | `grouping:always` | `grouping:always` | Always build triage groups, even for small reviews | **Grouping is presentation, not a mode.** The `grouping:` tokens change how the finding set is organized for triage — never reviewer selection, merge logic, scope rules, or the Stage 5c apply decision. **Mode alias:** `mode:headless` normalizes to `mode:agent`. `mode:agent` + `mode:headless` is not a conflict. **Conflicting arguments:** Stop without dispatching reviewers when: - Multiple incompatible scope selectors appear together (e.g. `base:` **and** a PR number/branch target — `base:` means "review the current checkout against this base") - Multiple distinct `mode:` tokens other than the `mode:agent`/`mode:headless` alias pair - Multiple distinct `grouping:` tokens (e.g. `grouping:off` **and** `grouping:always`) Deprecated `mode:autofix` is **not** a conflict — ignore the token and proceed with the normal flow (see below). Emit a one-line failure reason. In `mode:agent`, return JSON: `{"status":"failed","reason":"..."}`. ## Operating principles Same pipeline for default and `mode:agent`: - **Apply locally; never push.** Never push, open PRs, or file tickets in any mode — push is the outward step the user owns. In **default (interactive)** mode the review applies safe, verified fixes and commits them when the pre-review tree was clean (Stage 5c owns the full rule). In **`mode:agent`** it never mutates the tree — it reports and the caller applies. - **No blocking prompts.** Never use `AskUserQuestion`, `request_user_input`, `ask_user`, or other blocking question tools. Infer intent, plan, and scope from explicit tokens, git state, PR metadata, and conversation. Note uncertainty in Coverage or the verdict — do not stop to ask. - **Explicit mutations only.** Never run `gh pr checkout`, `git checkout`, `git switch`, or similar branch-switch commands. Passing a PR number, URL, or branch name selects **review scope**, not permission to mutate the working tree. To review local uncommitted work on a feature branch, check out that branch yourself (or stay on it) and pass `base:` or no target. - **Smart defaults.** Untracked files: review tracked changes only and list excluded paths in Coverage. Plan: use `plan:` when passed; otherwise discover conservatively from PR body or branch keywords. Weak advisory P2/P3 from testing/maintainability alone: demote to `testing_gaps` / `residual_risks` per Stage 5. - **Report outcomes, not machinery.** What you show the user is about the review: what's being examined (the PR/branch), which coverage is included and the one-line reason for each conditional lens, the independent cross-model pass and which model runs it, and the findings. Keep the skill's internals out of user-facing text — model-tier assignments, raw scope-mode codenames (`local-aligned`/`pr-remote`), staging the diff to disk, loading persona files, parallel-dispatch bookkeeping, and step-by-step narration of your own setup. Name what the user would recognize (a PR number, a reviewer's concern, a peer model), not the plumbing. This governs *what* you surface and suppress; it does not script the wording — use your own voice. ## Output format | Invocation | Deliverable | |------------|-------------| | **Default** | Markdown report (pipe-delimited finding tables) + Actionable Findings summary | | **`mode:agent`** | One JSON object (see ### JSON output format below) + the same `/tmp/.../ce-code-review/<run-id>/` artifacts | `mode:agent` is **report-only**: it skips the Stage 5c apply (the caller applies) and serializes findings as JSON instead of markdown. It does not change reviewer selection, merge logic, or scope rules — the JSON is the deterministic contract for programmatic and cross-harness callers (Codex, Gemini, etc.). The default markdown is the human view; keep it ASCII-safe (pipe tables, `->` not middot `·`, no box-drawing) so it degrades gracefully across terminals. ## Quick Review Short-Circuit If `$ARGUMENTS` indicates the user wants a quick, fast, or light code review — and **`mode:agent` is not active** — do not dispatch the multi-agent flow. **Announce the chosen path** before any other work (Quick review vs Multi-agent review). Skip this announcement when `mode:agent` is active. Sequence: 1. **Run the harness's built-in code review.** Forward any review target after stripping tokens. Then stop — do not dispatch the multi-agent pipeline. 2. **Exemption:** If no built-in review exists, continue into the full multi-agent review. 3. **`mode:agent` bypasses this short-circuit** — always run the full multi-agent review and return JSON. **Deprecated:** `mode:autofix` is no longer supported — there is no apply *mode*. If passed, ignore the token and proceed with the normal flow (default applies safe fixes via Stage 5c; `mode:agent` reports and the caller applies). ## Severity Scale All reviewers use P0-P3: | Level | Meaning | Action | |-------|---------|--------| | **P0** | Critical breakage, exploitable vulnerability, data loss/corruption | Must fix before merge | | **P1** | High-impact defect likely hit in normal usage, breaking contract | Should fix | | **P2** | Moderate issue with meaningful downside (edge case, perf regression, maintainability trap) | Fix if straightforward | | **P3** | Low-impact, narrow scope, minor improvement | User's discretion | ## Action Routing Severity answers **urgency**. `autofix_class` and `owner` are **signal** describing follow-up shape for callers — **not apply permission or an apply gate.** The apply decision is judgment (Stage 5c), not a function of `autofix_class`: default mode applies; in `mode:agent` this skill does not mutate the checkout — the caller applies. See `references/action-class-rubric.md` for persona guidance. | `autofix_class` | Default owner | Meaning | |-----------------|---------------|---------| | `gated_auto` | `downstream-resolver` or `human` | Concrete `suggested_fix` proposed; caller applies after judgment | | `manual` | `downstream-resolver` or `human` | Actionable work needing design input or handoff | | `advisory` | `human` or `release` | Report-only — learnings, rollout notes, residual risk | Routing rules: - **Synthesis owns the final route.** Persona-provided routing metadata is input, not the last word. - **Choose the more conservative route on disagreement.** A merged finding may move from `gated_auto` to `manual`, but never widen without stronger evidence. - **Reject `safe_auto` and `review-fixer` if present** — drop the finding or remap to `gated_auto` / `downstream-resolver` during synthesis. - **`requires_verification: true` means any caller-applied fix needs targeted tests or follow-up validation.** ## Reviewers 14 reviewer personas in layered conditionals, plus CE local prompt assets. Quick roster with one-line triggers below; the persona catalog included at the bottom has the full per-persona selection criteria and spawn gates. Each selected reviewer is a generic subagent seeded with a local prompt file from `references/personas/`; do not dispatch standalone agents by type/name. **Always-on (full review):** local prompt assets `correctness-reviewer`, `testing-reviewer`, `maintainability-reviewer`, `project-standards-reviewer`, plus CE local prompt assets `agent-native-reviewer` and `learnings-researcher`. (Stage 3c may reduce this set to a lite roster for trivial, low-risk diffs.) **Cross-cutting conditional (per diff):** - `security-reviewer` — auth, public endpoints, user input, permissions - `performance-reviewer` — DB queries, data transforms, caching, async - `api-contract-reviewer` — routes, serializers, type signatures, versioning - `data-migration-reviewer` — migration files / schema dumps / backfills (see spawn gate in Stage 3) - `reliability-reviewer` — error handling, retries, timeouts, background jobs - `adversarial-reviewer` — >=50 changed code lines, or auth / payments / data mutations / external APIs. When selected, a **cross-model adversarial pass** (Stage 4) additionally runs the same brief through a different model family via a peer CLI — additive, non-blocking - `previous-comments-reviewer` — PR with existing review comments (PR-only, comment-gated) **Stack-specific conditional (per diff):** `julik-frontend-races-reviewer` (Stimulus/Turbo, DOM events, async UI) and `swift-ios-reviewer` (Swift/SwiftUI/UIKit, entitlements, Core Data, `.pbxproj`). **CE conditional (migration-specific):** local prompt asset `deployment-verification-agent` — deployment checklist + rollback when the migration gate applies and the change is risky. ## Review Scope A full review spawns generic subagents for all 4 always-on personas plus the 2 CE always-on local prompt assets, then adds whichever cross-cutting and stack-specific conditionals fit the diff (Stage 3c can collapse this to a lite roster for trivial, low-risk diffs). The model naturally right-sizes: a small config change triggers 0 conditionals = 6 reviewers. A Rails auth feature might trigger security + reliability + adversarial = 9 reviewers. ## Protected Artifacts The following paths are compound-engineering pipeline artifacts and must never be flagged for deletion, removal, or gitignore by any reviewer: - `docs/brainstorms/*` -- legacy requirements documents created by older ce-brainstorm versions - `docs/plans/*.{md,html}` -- unified plan artifacts created by ce-brainstorm or ce-plan (decision artifacts; execution progress is derived from git, not stored in plan bodies) - `docs/solutions/*.md` -- solution documents created during the pipeline If a reviewer flags any file in these directories for cleanup or removal, discard that finding during synthesis. ## Plan Requirements Completeness When a plan is provided via `plan:<path>` or discovered from PR/branch context, classify readiness before checking completeness: - Unified artifact: metadata includes `artifact_contract: ce-unified-plan/v1`. - `artifact_readiness: requirements-only` can inform product intent, but it must not trigger implementation-unit completeness findings. Report that the artifact was not implementation-ready if the diff appears to implement it. - `artifact_readiness: implementation-ready` is eligible for full requirements and U-ID completeness checks. - Invalid progress-like readiness values (`active`, `in_progress`, `completed`, `done`) are contract errors. - Legacy plan: use the existing completeness checks. Extract requirements from these shapes, in order: 1. Unified `Product Contract` -> `### Requirements` 2. Legacy top-level `## Requirements` 3. Legacy `## Requirements Trace` For unified implementation-ready plans, also extract U-IDs from `## Implementation Units` and compare against PR body/branch context when available. Do not require every Product Contract R-ID to map one-to-one to a single U-ID; verify that implemented U-IDs cite the relevant R/F/AE/KTD IDs and that no claimed U-ID is missing from the plan. ## How to Run ### Stage 1: Determine scope Compute the diff range, file list, and diff. Minimize permission prompts by combining into as few commands as possible. **If `base:` argument is provided (fast path):** The caller already knows the diff base. Skip all base-branch detection, remote resolution, and merge-base computation. Use the provided value directly: ``` BASE_ARG="{base_arg}" BASE=$(git merge-base HEAD "$BASE_ARG" 2>/dev/null) || BASE="$BASE_ARG" ``` Then produce the same output as the other paths: ``` echo "BASE:$BASE" && echo "FILES:" && git diff --name-only $BASE && echo "DIFF:" && git diff -U10 $BASE && echo "UNTRACKED:" && git ls-files --others --exclude-standard ``` This path works with any ref — a SHA, `origin/main`, a branch name. Callers reviewing the current checkout should pass explicit `base:` when auto-detection is unnecessary. **Do not combine `base:` with a PR number or branch target.** If both are present, stop with an error: "Cannot use `base:` with a PR number or branch target — `base:` implies the current checkout is already the correct branch. Pass `base:` alone, or pass the target alone and let scope detection resolve the base." **If a PR number or GitHub URL is provided as an argument:** Do **not** check out the PR branch. Scope comes from GitHub read APIs plus optional local alignment when HEAD already matches the PR head branch. **Skip-condition pre-check.** Before scope detection, run a PR-state probe: ``` gh pr view <number-or-url> --json state,title,body,files ``` Apply skip rules in order: - `state` is `CLOSED` or `MERGED` -> stop with reason `PR is closed/merged; not reviewing.` - **Trivial-PR judgment**: spawn a lightweight sub-agent on the platform's cheapest capable model when a known override exists; otherwise omit the model override and inherit. Give it the PR title, body, and changed file paths. The agent's task: "Is this an automated or trivial PR that does not warrant a code review? Consider: dependency lock-file or manifest-only bumps, automated release commits, chore version increments with no substantive code changes. When in doubt, answer no — false negatives (skipped reviews that should have run) are more costly than false positives (unnecessary reviews)." If the judgment returns yes: stop with reason `PR appears to be a trivial automated PR; not reviewing. Run without a PR argument to review the current branch, or pass base:<ref> if review is intended.` When any skip rule fires, stop without dispatching reviewers. **Default mode:** emit the reason as plain text. **`mode:agent`:** emit JSON only — `{"status":"skipped","reason":"<same message>"}` — so programmatic callers can parse the outcome. **Standalone**, **`base:`**, and **branch-remote** paths are unaffected. **Draft PRs are reviewed normally.** If no skip rule fires, fetch PR metadata **without checkout**: ``` gh pr view <number-or-url> --json title,body,baseRefName,headRefName,headRefOid,isCrossRepository,url,files,reviews,comments --jq '{title, body, baseRefName, headRefName, headRefOid, isCrossRepository, url, files: [.files[].path], hasPriorComments: ((.reviews | map(select(.state != "APPROVED" or .body != "")) | length) > 0 or (.comments | length) > 0)}' ``` Set `BASE:` to `pr:<number-or-url>` (logical marker — not a git SHA). Set `UNTRACKED:` from `git ls-files --others --exclude-standard` on the **current** checkout (usually empty during PR-remote review). **PR scope mode.** Classify as **`local-aligned`** only when **all** of these hold; otherwise use **`pr-remote`**. A matching branch name alone is not enough — a fork PR or a stale local branch can share a name with the PR head while pointing at unrelated code, and trusting the name would diff and inspect the wrong tree.