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 as a read-only or autofix review step inside larger workflows ## Argument Parsing Parse `$ARGUMENTS` for the following optional tokens. Strip each recognized token before interpreting the remainder as the PR number, GitHub URL, or branch name. | Token | Example | Effect | |-------|---------|--------| | `mode:autofix` | `mode:autofix` | Select autofix mode (see Mode Detection below) | | `mode:report-only` | `mode:report-only` | Select report-only mode | | `mode:headless` | `mode:headless` | Select headless mode for programmatic callers (see Mode Detection below) | | `base:<sha-or-ref>` | `base:abc1234` or `base:origin/main` | Skip scope detection — use this as the diff base directly | | `plan:<path>` | `plan:docs/plans/2026-03-25-001-feat-foo-plan.md` | Load this plan for requirements verification | All tokens are optional. Each one present means one less thing to infer. When absent, fall back to existing behavior for that stage. **Conflicting mode flags:** If multiple mode tokens appear in arguments, stop and do not dispatch agents. If `mode:headless` is one of the conflicting tokens, emit the headless error envelope: `Review failed (headless mode). Reason: conflicting mode flags — <mode_a> and <mode_b> cannot be combined.` Otherwise emit the generic form: `Review failed. Reason: conflicting mode flags — <mode_a> and <mode_b> cannot be combined.` ## Mode Detection | Mode | When | Behavior | |------|------|----------| | **Interactive** (default) | No mode token present | Review, apply safe_auto fixes automatically, present findings, ask for policy decisions on gated/manual findings, and optionally continue into fix/push/PR next steps | | **Autofix** | `mode:autofix` in arguments | No user interaction. Review, apply only policy-allowed `safe_auto` fixes, re-review in bounded rounds, write a run artifact capturing residual downstream work | | **Report-only** | `mode:report-only` in arguments | Strictly read-only. Review and report only, then stop with no edits, artifacts, commits, pushes, or PR actions | | **Headless** | `mode:headless` in arguments | Programmatic mode for skill-to-skill invocation. Apply `safe_auto` fixes silently (single pass), return all other findings as structured text output, write run artifacts, and return "Review complete" signal. No interactive prompts. | ### Autofix mode rules - **Skip all user questions.** Never pause for approval or clarification once scope has been established. - **Apply only `safe_auto -> review-fixer` findings.** Leave `gated_auto`, `manual`, `human`, and `release` work unresolved. - **Write a run artifact** under `/tmp/compound-engineering/ce-code-review/<run-id>/` summarizing findings, applied fixes, residual actionable work, and advisory outputs. Orchestrators read this artifact to route residual `downstream-resolver` findings; the skill itself does not file tickets or prompt the user in autofix. - **Emit a compact Residual Actionable Work summary in the autofix return** listing each residual `downstream-resolver` finding with severity, file:line, title, and autofix_class. Include the run-artifact path. Callers read this summary directly without parsing the artifact. When no residuals exist, state `Residual actionable work: none.` explicitly. - **Never commit, push, or create a PR** from autofix mode. Parent workflows own those decisions. ### Report-only mode rules - **Skip all user questions.** Infer intent conservatively if the diff metadata is thin. - **Never edit files or externalize work.** Do not write `/tmp/compound-engineering/ce-code-review/<run-id>/`, do not file tickets, and do not commit, push, or create a PR. - **Safe for parallel read-only verification.** `mode:report-only` is the only mode that is safe to run concurrently with browser testing on the same checkout. - **Do not switch the shared checkout.** If the caller passes an explicit PR or branch target, `mode:report-only` must run in an isolated checkout/worktree or stop instead of running `gh pr checkout` / `git checkout`. - **Do not overlap mutating review with browser testing on the same checkout.** If a future orchestrator wants fixes, run the mutating review phase after browser testing or in an isolated checkout/worktree. ### Headless mode rules - **Skip all user questions.** Never use the platform question tool (`AskUserQuestion` in Claude Code, `request_user_input` in Codex, `ask_user` in Gemini, `ask_user` in Pi (requires the `pi-ask-user` extension)) or other interactive prompts. Infer intent conservatively if the diff metadata is thin. - **Require a determinable diff scope.** If headless mode cannot determine a diff scope (no branch, PR, or `base:` ref determinable without user interaction), emit `Review failed (headless mode). Reason: no diff scope detected. Re-invoke with a branch name, PR number, or base:<ref>.` and stop without dispatching agents. - **Apply only `safe_auto -> review-fixer` findings in a single pass.** No bounded re-review rounds. Leave `gated_auto`, `manual`, `human`, and `release` work unresolved and return them in the structured output. - **Return all non-auto findings as structured text output.** Use the headless output envelope format (see Stage 6 below) preserving severity, autofix_class, owner, requires_verification, confidence, pre_existing, and suggested_fix per finding. Enrich with detail-tier fields (why_it_matters, evidence[]) from the per-agent artifact files on disk (see Detail enrichment in Stage 6). - **Write a run artifact** under `/tmp/compound-engineering/ce-code-review/<run-id>/` summarizing findings, applied fixes, and advisory outputs. Include the artifact path in the structured output. - **Do not file tickets or externalize work.** The caller receives structured findings and routes downstream work itself. - **Do not switch the shared checkout.** If the caller passes an explicit PR or branch target, `mode:headless` must run in an isolated checkout/worktree or stop instead of running `gh pr checkout` / `git checkout`. When stopping, emit `Review failed (headless mode). Reason: cannot switch shared checkout. Re-invoke with base:<ref> to review the current checkout, or run from an isolated worktree.` - **Not safe for concurrent use on a shared checkout.** Unlike `mode:report-only`, headless mutates files (applies `safe_auto` fixes). Callers must not run headless concurrently with other mutating operations on the same checkout. - **Never commit, push, or create a PR** from headless mode. The caller owns those decisions. - **End with "Review complete" as the terminal signal** so callers can detect completion. If all reviewers fail or time out, emit `Code review degraded (headless mode). Reason: 0 of N reviewers returned results.` followed by "Review complete". ### Interactive mode rules - **Pre-load the platform question tool before any question fires.** In Claude Code, `AskUserQuestion` is a deferred tool — its schema is not available at session start. At the start of Interactive-mode work (before Stage 2 intent-ambiguity questions, the After-Review routing question, walk-through per-finding questions, bulk-preview Proceed/Cancel, and tracker-defer failure sub-questions), call `ToolSearch` with query `select:AskUserQuestion` to load the schema. Load it **once, eagerly, at the top of the Interactive flow** — do not wait for the first question site and do not decide it on a per-site basis. On Codex, Gemini, and Pi this preload step does not apply. - **The numbered-list fallback only applies when the harness genuinely lacks a blocking question tool** — `ToolSearch` returns no match, the tool call explicitly fails, or the runtime mode does not expose it (e.g., Codex edit modes where `request_user_input` is unavailable). A pending schema load is not a fallback trigger; call `ToolSearch` first per the pre-load rule. Rendering a question as narrative text because the tool feels inconvenient, because the model is in report-formatting mode, or because the instruction was buried in a long skill is a bug. A question that calls for a user decision must either fire the tool or fall back loudly. ## 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**. Routing answers **who acts next** and **whether this skill may mutate the checkout**. | `autofix_class` | Default owner | Meaning | |-----------------|---------------|---------| | `safe_auto` | `review-fixer` | Local, deterministic fix suitable for the in-skill fixer when the current mode allows mutation | | `gated_auto` | `downstream-resolver` or `human` | Concrete fix exists, but it changes behavior, contracts, permissions, or another sensitive boundary that should not be auto-applied by default | | `manual` | `downstream-resolver` or `human` | Actionable work that should be handed off rather than fixed in-skill | | `advisory` | `human` or `release` | Report-only output such as learnings, rollout notes, or 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 `safe_auto` to `gated_auto` or `manual`, but never the other way without stronger evidence. - **Only `safe_auto -> review-fixer` enters the in-skill fixer queue automatically.** - **`requires_verification: true` means a fix is not complete without targeted tests, a focused re-review, or operational validation.** ## Reviewers 18 reviewer personas in layered conditionals, plus CE-specific agents. See the persona catalog included below for the full catalog. **Always-on (every review):** | Agent | Focus | |-------|-------| | `ce-correctness-reviewer` | Logic errors, edge cases, state bugs, error propagation | | `ce-testing-reviewer` | Coverage gaps, weak assertions, brittle tests | | `ce-maintainability-reviewer` | Coupling, complexity, naming, dead code, abstraction debt | | `ce-project-standards-reviewer` | CLAUDE.md and AGENTS.md compliance -- frontmatter, references, naming, portability | | `ce-agent-native-reviewer` | Verify new features are agent-accessible | | `ce-learnings-researcher` | Search docs/solutions/ for past issues related to this PR | **Cross-cutting conditional (selected per diff):** | Agent | Select when diff touches... | |-------|---------------------------| | `ce-security-reviewer` | Auth, public endpoints, user input, permissions | | `ce-performance-reviewer` | DB queries, data transforms, caching, async | | `ce-api-contract-reviewer` | Routes, serializers, type signatures, versioning | | `ce-data-migrations-reviewer` | Migrations, schema changes, backfills | | `ce-reliability-reviewer` | Error handling, retries, timeouts, background jobs | | `ce-adversarial-reviewer` | Diff >=50 changed non-test/non-generated/non-lockfile lines, or auth, payments, data mutations, external APIs | | `ce-cli-readiness-reviewer` | CLI command definitions, argument parsing, CLI framework usage, command handler implementations | | `ce-previous-comments-reviewer` | Reviewing a PR that has existing review comments or threads | **Stack-specific conditional (selected per diff):** | Agent | Select when diff touches... | |-------|---------------------------| | `ce-dhh-rails-reviewer` | Rails architecture, service objects, session/auth choices, or Hotwire-vs-SPA boundaries | | `ce-kieran-rails-reviewer` | Rails application code where conventions, naming, and maintainability are in play | | `ce-kieran-python-reviewer` | Python modules, endpoints, scripts, or services | | `ce-kieran-typescript-reviewer` | TypeScript components, services, hooks, utilities, or shared types | | `ce-julik-frontend-races-reviewer` | Stimulus/Turbo controllers, DOM events, timers, animations, or async UI flows | | `ce-swift-ios-reviewer` | Swift files, SwiftUI views, UIKit controllers, entitlements, privacy manifests, Core Data models, SPM manifests, storyboards/XIBs, or semantic build-setting/target/signing changes in .pbxproj | **CE conditional (migration-specific):** | Agent | Select when diff includes migration files | |-------|------------------------------------------| | `ce-schema-drift-detector` | Cross-references schema.rb against included migrations | | `ce-deployment-verification-agent` | Produces deployment checklist with SQL verification queries | ## Review Scope Every review spawns all 4 always-on personas plus the 2 CE always-on agents, then adds whichever cross-cutting and stack-specific conditionals fit the diff. The model naturally right-sizes: a small config change triggers 0 conditionals = 6 reviewers. A Rails auth feature might trigger security + reliability + kieran-rails + dhh-rails = 10 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/*` -- requirements documents created by ce-brainstorm - `docs/plans/*.md` -- plan files created by 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. ## 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. Automated callers (ce-work, lfg, slfg) should prefer this to avoid the detection overhead. **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." This avoids scope/intent mismatches where the diff base comes from one source but the code and metadata come from another. **If a PR number or GitHub URL is provided as an argument:** If `mode:report-only` or `mode:headless` is active, do **not** run `gh pr checkout <number-or-url>` on the shared checkout. For `mode:report-only`, tell the caller: "mode:report-only cannot switch the shared checkout to review a PR target. Run it from an isolated worktree/checkout for that PR, or run report-only with no target argument on the already checked out branch." For `mode:headless`, emit `Review failed (headless mode). Reason: cannot switch shared checkout. Re-invoke with base:<ref> to review the current checkout, or run from an isolated worktree.` Stop here unless the review is already running in an isolated checkout. **Skip-condition pre-check.** Before checkout or scope detection, run a PR-state probe to decide whether the review should proceed: ``` gh pr view <number-or-url> --json state,title,body,files ``` Apply skip rules in order: - `state` is `CLOSED` or `MERGED` -> stop with message `PR is closed/merged; not reviewing.`