ce-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 | | `run:<run-id>` | `run:2026-06-01-001-feat-foo` | Associate this invocation with an orchestrated `/lfg` or `/slfg` run so artifacts co-locate under the shared run id. Recognize and strip it; it does not change review behavior. Ignore any other unrecognized `key:value` tokens | 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, and emit residual downstream work when needed | | **Report-only** | `mode:report-only` in arguments | Strictly read-only. Review and report only, then stop with no edits, artifacts, todos, 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, skip todos, 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 `.context/compound-engineering/ce-review/<run-id>/` summarizing findings, applied fixes, residual actionable work, and advisory outputs. - **Create durable todo files only for unresolved actionable findings** whose final owner is `downstream-resolver`. Load the `todo-create` skill for the canonical directory path and naming convention. - **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 `.context/compound-engineering/ce-review/<run-id>/`, do not create todo files, 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 (`ask_user` in Copilot CLI) 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, evidence[], and pre_existing per finding. - **Write a run artifact** under `.context/compound-engineering/ce-review/<run-id>/` summarizing findings, applied fixes, and advisory outputs. Include the artifact path in the structured output. - **Do not create todo files.** 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". ## 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 17 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 | |-------|-------| | `compound-engineering:review:correctness-reviewer` | Logic errors, edge cases, state bugs, error propagation | | `compound-engineering:review:testing-reviewer` | Coverage gaps, weak assertions, brittle tests | | `compound-engineering:review:maintainability-reviewer` | Coupling, complexity, naming, dead code, abstraction debt | | `compound-engineering:review:project-standards-reviewer` | AGENTS.md compliance -- frontmatter, references, naming, portability | | `compound-engineering:review:agent-native-reviewer` | Verify new features are agent-accessible | | `compound-engineering:research:learnings-researcher` | Search docs/solutions/ for past issues related to this PR | **Cross-cutting conditional (selected per diff):** | Agent | Select when diff touches... | |-------|---------------------------| | `compound-engineering:review:security-reviewer` | Auth, public endpoints, user input, permissions | | `compound-engineering:review:performance-reviewer` | DB queries, data transforms, caching, async | | `compound-engineering:review:api-contract-reviewer` | Routes, serializers, type signatures, versioning | | `compound-engineering:review:data-migrations-reviewer` | Migrations, schema changes, backfills | | `compound-engineering:review:reliability-reviewer` | Error handling, retries, timeouts, background jobs | | `compound-engineering:review:adversarial-reviewer` | Diff >=50 changed non-test/non-generated/non-lockfile lines, or auth, payments, data mutations, external APIs | | `compound-engineering:review:cli-readiness-reviewer` | CLI command definitions, argument parsing, CLI framework usage, command handler implementations | | `compound-engineering:review:previous-comments-reviewer` | Reviewing a PR that has existing review comments or threads | **Stack-specific conditional (selected per diff):** | Agent | Select when diff touches... | |-------|---------------------------| | `compound-engineering:review:dhh-rails-reviewer` | Rails architecture, service objects, session/auth choices, or Hotwire-vs-SPA boundaries | | `compound-engineering:review:kieran-rails-reviewer` | Rails application code where conventions, naming, and maintainability are in play | | `compound-engineering:review:kieran-python-reviewer` | Python modules, endpoints, scripts, or services | | `compound-engineering:review:kieran-typescript-reviewer` | TypeScript components, services, hooks, utilities, or shared types | | `compound-engineering:review:julik-frontend-races-reviewer` | Stimulus/Turbo controllers, DOM events, timers, animations, or async UI flows | **CE conditional (migration-specific):** | Agent | Select when diff includes migration files | |-------|------------------------------------------| | `compound-engineering:review:schema-drift-detector` | Cross-references schema.rb against included migrations | | `compound-engineering:review: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 (living documents with progress checkboxes) - `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. First, verify the worktree is clean before switching branches: ``` git status --porcelain ``` If the output is non-empty, inform the user: "You have uncommitted changes on the current branch. Stash or commit them before reviewing a PR, or use standalone mode (no argument) to review the current branch as-is." Do not proceed with checkout until the worktree is clean. Then check out the PR branch so persona agents can read the actual code (not the current checkout): ``` gh pr checkout <number-or-url> ``` Then fetch PR metadata. Capture the base branch name and the PR base repository identity, not just the branch name: ``` gh pr view <number-or-url> --json title,body,baseRefName,headRefName,url ``` Use the repository portion of the returned PR URL as `<base-repo>` (for example, `EveryInc/compound-engineering-plugin` from `https://github.com/EveryInc/compound-engineering-plugin/pull/348`). Then compute a local diff against the PR's base branch so re-reviews also include local fix commits and uncommitted edits. Substitute the PR base branch from metadata (shown here as `<base>`) and the PR base repository identity derived from the PR URL (shown here as `<base-repo>`). Resolve the base ref from the PR's actual base repository, not by assuming `origin` points at that repo: ```