v2.0.0 to v2.0.0

34 added, 55 removed. Audit A to A.

---

name: review

description: Review a plan, diff, or roadmap slice; findings land on it.

version: 2.0.0

argument-hint: "--surface plan|diff|roadmap"

allowed-tools: ["Read","Write","Edit","Bash","Grep","Glob","Agent","Skill","AskUserQuestion","TaskCreate","TaskUpdate","TaskGet","TaskList"]

---

# coordinator:review

- **Trigger:** a reviewable artifact exists — a plan/design doc/RFC, a code change, or a roadmap spine/sprint slice — outgoing when nothing on it has been reviewed yet, incoming when a reviewer's findings have landed.

+ **Trigger:** a reviewable artifact exists (plan/design doc/RFC, code change, roadmap spine/sprint slice) — outgoing when unreviewed, incoming when a reviewer's findings have landed.

- **When NOT to use:** frozen weekly diff at `/workweek-complete` Step 7 → `coordinator:parallel-code-review`. Stuck/oscillating → self-monitor, don't dispatch a reviewer. Pure mechanical citation check, no Opus → `docs-checker` directly. `--surface plan` mid-drafting, `--surface diff` mid-implementation, or `--surface roadmap` before the PM has shape-approved the spine → keep working. A finding about how one stub will be BUILT is plan altitude, not roadmap altitude → route it to that stub's own plan review. `--surface diff` pure test-output classification → capture stdout/stderr to a file, dispatch `test-evidence-parser` on it.

+ **When NOT to use:** frozen weekly diff at `/workweek-complete` Step 7 → `coordinator:parallel-code-review`. Stuck/oscillating → self-monitor, don't dispatch a reviewer. Pure mechanical citation check, no Opus → `docs-checker` directly. `--surface plan` mid-drafting, `--surface diff` mid-implementation, or `--surface roadmap` before the PM has shape-approved the spine → keep working. A finding about how one stub will be BUILT is plan altitude, not roadmap altitude → route it to that stub's own plan review. `--surface diff` pure test-output classification → capture stdout/stderr to a file, run `bin/weekly-gate-tests.py --test-output` on it.

- `--surface` is resolved before this skill loads. Pre-flight checks, reviewer-tier precedence, sequencing exceptions, and prior-art mutability are retrieved by `review-assemble brief`, scoped to the resolved surface (segment set: `coordinator/skills/review/residue/`).

+ `--surface` is resolved before this skill loads. Surface-scoped pre-flight, tier precedence, sequencing exceptions and prior-art mutability come from `review-assemble brief` (segments: `coordinator/skills/review/residue/`).

**Dispatch authorization — invoking this skill IS the request.** The dispatches named below are constitutive steps of this skill, not a separate thing to get cleared: invoking a skill requests the actions that skill performs. A harness line permitting dispatch "unless the user requested it" is therefore **satisfied here, not overridden** — no precedence claim is needed and none is made. Re-asking spends the very context the dispatch exists to protect. The rule attaches to skill entry and dissolves no PM-authored gate: keyword-gated skills gate entry, and every gate a skill names for itself still binds — per-session cross-repo-commit assent, ask-before-external-action, and any other this skill's own body names. Tripwire: `UNATTRIBUTED-HARNESS-LINE-IS-NOT-PM`.

---

## Branch A — Outgoing

_A reviewable artifact exists for `--surface`, no reviewer invoked yet this iteration._

### A.1 — Pre-flight

- **Diff freeze:** only the caller knows the intended range — `code-reviewer` never selects its own. Freeze via `freeze-review-diff` with the caller-chosen range and slice-id before dispatch; never default a shared `work/*` branch to `origin/main...HEAD` (sweeps in sibling sessions' reviewed commits). Inject the frozen diff path as the primary dispatch artifact.

+ **Diff freeze:** the caller picks the range, never `code-reviewer`. Freeze via `freeze-review-diff` with the range and slice-id before dispatch; never default a shared `work/*` branch to `origin/main...HEAD`. Inject the frozen diff path as the primary artifact.

- **Reviewers don't execute.** Bash is read-only inspection — no interpreter, scratch files, or test runs. A runtime claim gets the EM running the probe before dispatch and pasting its output into the brief as evidence, never a task. The verdict's `executed: <yes|no>` discloses whether a WARN was empirically checked or hand-traced.

+ **Reviewers don't execute.** Bash is read-only. The EM runs any runtime probe before dispatch and pastes the output into the brief; the verdict's `executed: <yes|no>` discloses it.

### A.2 — Reviewer selection and dispatch

- Reviewer selection (routing-table match, tier precedence, effort) is a signal-lookup a program can compute; no producer AUTOMATES it yet — the plan carries the input, nothing yet merges it at dispatch time. Until then:

+ No producer automates reviewer selection yet. Until then:

- - **Routing table:** merge `coordinator/routing.md` with every enabled plugin's `routing.md` fragments into one composite table; match signals to identify Reviewer 1 (domain specialist) and Reviewer 2 (generalist, if needed) — same table both surfaces.

- - **Read `review_signals` off the plan first.** Resolve each id through `coordinator/contract/review-signals.json`. An id present in `review_signals` but ABSENT from the contract surfaces loudly (name it; do not proceed as if unset). Hand-matching the routing table against prose is the fallback only for a plan with **no** `review_signals` field.

- - **Tier precedence** (Sonnet-vs-Opus, single-vs-cross-domain) is surface-specific, assembled by `review-assemble brief`.

- - **Effort is PM-gated, not an EM dial.** `routing.md`'s Effort field is PM-facing reference only — never put a level in a dispatch prompt or narrate one unless the PM named it.

- - **the Director of Engineering:** cross-team/consumer-leak signal → dispatch standalone as primary (director-altitude posture in brief; no `mode` arg), skip the Staff Engineer. Chained-after-the Staff Engineer ("backstop") is the High-effort-architectural routing entry, not the Director of Engineering's only mode.

- - `--reviewers "name1,name2"` skips auto-detection; report "PM-directed review: [name1] then [name2]."

- - **Tier vs. complexity, not importance:** one reviewer suffices unless a second would likely *contradict*.

+ - **Routing table:** merge `coordinator/routing.md` with every enabled plugin's `routing.md` fragments; match signals to Reviewer 1 (domain specialist) and Reviewer 2 (generalist, if needed).

+ - **Start from the plan's `review_signals`**, resolving ids through `coordinator/contract/review-signals.json`. An id absent from the contract surfaces loudly. Hand-matching prose is the fallback only for a plan with **no** `review_signals` field.

+ - **Effort is PM-gated.** Never put a level in a dispatch prompt unless the PM named it.

+ - **the Director of Engineering:** cross-team/consumer-leak signal → standalone primary (director-altitude posture in brief; no `mode` arg), skip the Staff Engineer. "Backstop" is the High-effort-architectural entry.

+ - `--reviewers "a,b"` skips auto-detection; report "PM-directed review: [a] then [b]."

+ - One reviewer suffices unless a second would likely *contradict*.

- **Pipeline phases** (docs-checker, prior-art-checker/plan-coverage-checker, external-pattern-checker, backstop, report) aren't optional — walk them inline per the surface's assembled phase list.

+ Pipeline phases aren't optional — walk them per the assembled phase list.

- **Persist findings on the pre-provisioned sidecar** (`state/subagent-share/<session>/<provision_key>.md`, already in the dispatch brief — `staff-eng-review` for personas, `review-findings` for `code-reviewer`) via **Edit**, not a Bash redirect or hand-scaffold.

+ **Persist findings on the pre-provisioned sidecar** (`state/subagent-share/<session>/<provision_key>.md`) via **Edit**, not a Bash redirect.

- **Persona/Opus reviewer** applies every finding in place, writes the `## Findings Ledger` to its sidecar, runs `review-findings-ledger verify --sidecar <path>`, and returns `DONE: <sidecar-path> | verdict: <OK|WARN|BLOCKED> | findings: <N>`; the EM reads the applied diff and the verified ledger.

+ **Persona/Opus reviewer** applies every finding in place, writes the `## Findings Ledger` to its sidecar, runs `review-findings-ledger verify --sidecar <path>`, and returns `DONE: <sidecar-path> | verdict: <OK|WARN|BLOCKED> | findings: <N>`.

- **Multi-reviewer chain:** each reviewer gets its own provisioned sidecar and verifies its own ledger. A trivial/unfilled returned sidecar fails loud (BLOCKED) at intake.

+ **Multi-reviewer chain:** each reviewer gets its own sidecar and verifies its own ledger. An unfilled returned sidecar fails loud (BLOCKED) at intake.

### A.3 — Sequencing

- **Code review (a diff) is per executor group in plan runs; reviewers apply own findings.**

- Slice owners, Kira and the named personas all apply in place with exact-text `Edit`s, including

- findings outside their own slice. Kira (`overengineering-reviewer`) is the execute review's

- whole-diff editor of shipped code, never of plans; runs at L and above. Only a rebuild verdict goes back to the EM; there is no second review round. This

- runs inside the execute workflow's review stage; a

- diff reviewed ad hoc (`/review-code`) applies and verifies its own ledger before a second

- reviewer touches it.

+ **Code review (a diff): per executor group in plan runs; reviewers apply their own findings.** Slice owners, Kira and the named personas apply in place with exact-text `Edit`s. Kira (`overengineering-reviewer`) edits shipped code only, never plans, and runs only when the EM or PM names Kira. Only a rebuild verdict goes back to the EM; there is no second review round. `/review-code` verifies its own ledger before a second reviewer.

- **Plan review over one artifact:** a chatty run reviews in one parallel wave, regions claimed by mail (`CLAIM <region>`; review-integration-doctrine.md); otherwise Reviewer 1 applies first.

+ **Plan review:** one parallel wave, each reviewer applying its own findings; a chatty run claims regions by mail (`CLAIM <region>`; review-integration-doctrine.md); a post-wave ledger verify catches lost edits.

- **Pre-flight sidecars are consumed alongside the plan**, never inserted into that chain; on a non-XL plan the Sonnet pre-flights (`docs-checker`, `prior-art-checker`, `plan-coverage-checker`) gate before the Opus reviewer; on the accepted-XL lane a `plan-reviewer` pass runs ahead of them, so four Sonnet-tier passes precede the Opus persona. `plan-coverage-checker` has no EM opt-out.

+ **Pre-flight sidecars are consumed alongside the plan.** Non-XL: the Sonnet pre-flights (`docs-checker`, `prior-art-checker`, `plan-coverage-checker`) gate before the Opus reviewer; accepted-XL adds a `plan-reviewer` pass ahead, so four Sonnet-tier passes precede the Opus persona. `plan-coverage-checker` has no EM opt-out.

- **Angelique (apm) trigger — size-gated, plan-review-only.** plan-blitz's size ladder on `sizing_object` (`estimate.tshirt`): XS/S Sonnet `plan-reviewer`, M and L the Staff Engineer, XL+ adds Angelique and the Director of Engineering. XL is the APM floor; add no second threshold surface. Kira is on no rung and never gates: `blocking_verdicts` is `null`, like `coordinator:docs-checker`.

+ **Angelique (apm) trigger — size-gated, plan-review-only.** Ladder on `sizing_object` (`estimate.tshirt`): XS/S Sonnet `plan-reviewer`, M and L the Staff Engineer, XL+ adds Angelique and the Director of Engineering. XL is the APM floor; add no second threshold surface. Kira is on no rung and never gates (`blocking_verdicts` is `null`).

---

## Branch B — Incoming

- _A reviewer has returned output, having already applied every finding in place and verified its

- own findings ledger. The EM checks whether anything should come back out._

+ _A reviewer has returned output, having applied every finding in place and verified its own ledger._

- **The double-check is a disagreement scan, not a re-adjudication.** Findings reaching the EM are already applied and ledgered by the reviewer. Read the applied diff and the findings ledger and reject what you disagree with via `review-findings-ledger reject --sidecar <path> --finding <id> --reason "<one line>"` — only the EM may run `reject`.

+ **The double-check is a disagreement scan, not a re-adjudication.** Read the applied diff and the ledger; reject what you disagree with via `review-findings-ledger reject --sidecar <path> --finding <id> --reason "<one line>"` — only the EM may run `reject`.

- The reviewer applies everything under an `OK`/`WARN`/`BLOCKED` verdict; under `REJECTED`/`PIVOT`

- (premise failure) or a Kira `rebuild_recommended: true`, it applies nothing and logs every row

- `suspended`.

+ The reviewer applies everything under an `OK`/`WARN`/`BLOCKED` verdict; under `REJECTED`/`PIVOT` (premise failure) or a Kira `rebuild_recommended: true`, it applies nothing and logs every row `suspended`.

- **Forbidden:** defer-to-later, capture-for-backlog, time-estimate-as-rationale — surface to PM; the EM does not decide to defer. Rejecting needs a `reject` reason. Application is not measured by count; rejecting a disagreeable nit with a reason is a correct outcome.

+ **Default is apply once, then continue.** A plan is redone only on an `UNSAVEABLE` finding that names why, or after n attempts on one problem space, when the route is research or a spike, never another plan (`A-REVIEWER-RETURNS-UNSAVEABLE-ONLY-WHEN-THE-PLAN-CANNOT-BE-SAVED`).

- **Provenance gate — resolve before triage.** Reviewer (persona/`code-reviewer`) sidecar at

- `state/subagent-share/<session>/<key>.md`? → the ledger contract above applies. Pre-flight lens

- checker (`prior-art-checker`, `plan-coverage-checker`, `docs-checker`, `external-pattern-checker`)

- sidecar at `.coordinator-local/plan-sidecars/<plan-stem>.<lens>.md`? → dispatch `coordinator:enricher`

- with the lens sidecar path + adjudicated items instead; a pre-flight lens sidecar is never a

- findings-ledger sidecar. Never hand-author around this.

+ **Forbidden:** defer-to-later, capture-for-backlog, time-estimate-as-rationale — surface to PM; the EM does not decide to defer. Rejecting needs a reason.

- **An artifact-shape tradeoff** (architectural direction, scope, sequencing, file organization,

- abstraction boundary) the reviewer applied anyway is what the EM's double-check exists

- for: read the reasoning, and `reject` it with a reason if you disagree.

- YAGNI/scope-trim, refactor-over-patch, and build-vs-defer stay PM calls when the finding itself

- raises one — the reviewer applying the edit does not resolve who owns that call.

+ **Provenance gate — resolve before triage.** Sidecar at `state/subagent-share/<session>/<key>.md` (persona/`code-reviewer`) → the ledger contract applies. Pre-flight lens checker sidecar at `.coordinator-local/plan-sidecars/<plan-stem>.<lens>.md` → dispatch `coordinator:enricher` with that path + adjudicated items; a lens sidecar is never a findings-ledger sidecar.

+ **An artifact-shape tradeoff** the reviewer applied anyway: read the reasoning, `reject` with a reason if you disagree. YAGNI/scope-trim, refactor-over-patch and build-vs-defer stay PM calls when the finding raises one.

**Worker Dispatch Recommendations block present** → dispatch each named worker (reviewers name,

EM dispatches), feed output back into EM context. Surface-specific eligibility and the

- test-evidence-parser capture-before-dispatch rule are assembled by the op.

+ capture-before-classify rule are assembled by the op.

- **Rejecting a finding from a plan body invalidates its mise-prep stamp** as applying one did: any body edit makes an existing `mise_prepped_*` attest STALE, not absent. Say STALE and route to a re-gate (`<settings-home>/bin/mise-prep-gate <plan>`), never a re-stamp; the predicate is a recomputed `canonical_body_sha`, never `mise_prepped_by` presence. Tripwire: `A-PRESENT-MISE-PREPPED-STAMP-IS-NOT-A-CERTIFICATION`; four states, four repairs: `coordinator/docs/wiki/lesson-triage/mise-prepped-attest.md`.

+ **Rejecting a finding from a plan body makes an existing `mise_prepped_*` attest STALE**, as applying one does. Route to a re-gate (`<settings-home>/bin/mise-prep-gate <plan>`), never a re-stamp. Tripwire: `A-PRESENT-MISE-PREPPED-STAMP-IS-NOT-A-CERTIFICATION`; `coordinator/docs/wiki/lesson-triage/mise-prepped-attest.md`.

- **`/review` fires on exiting `/plan`, not after an announcement.** The EM invokes it in the same

- turn, without waiting to be asked.

+ **`/review` fires on exiting `/plan`**: the EM invokes it in the same turn, unprompted.

---

## Cross-reference exit

- Outside a chatty run, after Branch B for a multi-reviewer review and Reviewer 1's findings are applied, return to A.2 for Reviewer 2.

- Execution-authorization gate, stamp-op invocation, and prior-art mutability/reviewer-elevation (plan-only) are assembled by `review-assemble brief`.

+ Outside a chatty run, once Reviewer 1's findings are applied, return to A.2 for Reviewer 2.

- `status: reviewed` and the mise-prep attest are orthogonal: `/review` writes `reviewer:` +

- `status: reviewed`; nothing here stamps a plan mise-prepped.

+ Execution-authorization gate and stamp-op invocation come from `review-assemble brief`. `/review` writes `reviewer:` + `status: reviewed`; nothing here stamps a plan mise-prepped.

- After authorization, the EM owns the dispatch-gate graph before the first executor dispatch: enumerate touched files per task, mark file-overlap/output-consumption/contract-change gates only, size per-executor scope ~5-10 min (15 min ceiling), author parallel-wave prompts with explicit peer-scope prohibition. Procedure: `coordinator:execute-plan` Phase 1.5.

+ After authorization, the EM owns the dispatch-gate graph before the first executor dispatch: `coordinator:execute-plan` Phase 1.5.