Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ __pycache__/
.closedloop-ai/*
!.closedloop-ai/settings/
!.closedloop-ai/bootstrap-metadata.json
# Plugin-local runtime working dirs must never be committed (the patterns above
# are root-anchored; hook/review artifacts can otherwise land under plugins/**).
plugins/**/.closedloop-ai/

# Generated route knowledge files
.claude/docs/routes/*
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ All notable changes to the claude-plugins project will be documented in this fil

The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Entries are listed newest-first; each plugin section is treated as released when merged to `main`.

### code-review v3.4.0

#### Changed
- The `/code-review` orchestrator now runs the deterministic review prefix via the `run-prefix` helper loop instead of walking those stages one at a time. After session setup it invokes `run-prefix` and acts on the returned status: dispatching a signal-extraction or coverage-critic agent and resuming when one is needed, presenting hygiene findings and exiting on a hygiene-only run, or — once the whole prefix (including model routing and partitioning) is done — printing the cache/fast-path notices and handing off to the reviewer fleet. This collapses roughly nineteen helper stages and their serial model turns into a handful of orchestrator turns, the single biggest turn-count reduction in a review. The per-stage walk remains fully documented as the labeled fallback, used only when `run-prefix` reports an error or is unavailable (e.g. an older plugin cache), so no behavior is lost.
- The stage-by-stage "Walker Contract" is now scoped to the reviewer/verification/presentation tail (spawn-reviewers onward) plus that prefix fallback; the hygiene-only exit and the routing/partition gate are performed inside `run-prefix` and surfaced through its result. The single-agent dispatch skill and `SCHEMA.md` were updated to match (the skill returns control for `run-prefix` to resume in the default flow, and `SCHEMA.md` documents the routing-failure recovery anchor and the shallow-tier spawn-spec stage).

### code-review v3.3.0

#### Changed
Expand Down
2 changes: 1 addition & 1 deletion plugins/code-review/.claude-plugin/plugin.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "code-review",
"description": "Code review plugin",
"version": "3.3.0",
"version": "3.4.0",
"author": {
"name": "ClosedLoop",
"email": "support@closedloop.ai"
Expand Down
8 changes: 5 additions & 3 deletions plugins/code-review/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ canonical schema documented in [SCHEMA.md](SCHEMA.md). Key contracts:
(default cap=20, BHA floor=1 waived for docs-only PRs, required overflow
fails closed and emits coverage gaps).
- **`prepare-run`** emits a declarative `run_plan.json` describing the 30-stage
pipeline. (The orchestrator rewrite over this plan is a follow-up.)
pipeline; the orchestrator runs its deterministic prefix in one process via
`run-prefix` and walks the reviewer/verification/presentation tail
stage-by-stage (PLN-1229).
- **Canonical `prompt_hash`** folds in `schema_version`: a MAJOR schema bump
invalidates every cache namespace at once.

Expand All @@ -66,7 +68,7 @@ The terminal artifact of every review run is `review_result.json` (PLN-722 envel

| Component | Role |
|---|---|
| `start.md` | Orchestrator command. Parses flags, sets up the session, invokes the helper CLI subcommands in sequence, spawns reviewer sub-agents, collects results, and presents findings |
| `start.md` | Orchestrator command. Parses flags, sets up the session, runs the deterministic prefix in one process via `run-prefix` (then walks the reviewer tail stage-by-stage), spawns reviewer sub-agents, collects results, and presents findings |
| `github-review.md` | Loaded by the orchestrator only in GitHub mode. Contains PR metadata resolution, file-based handoff format for CI, and summary format |
| `code_review_helpers.py` | Python CLI that handles all deterministic work: git diff parsing, hygiene pattern matching, file partitioning, risk scoring/model routing, finding validation, cache management, and GitHub comment posting |
| `shared_prompt.txt` | Constraints injected into every reviewer agent prompt: file assignment rules, evidence standards, severity definitions, and output format |
Expand Down Expand Up @@ -196,7 +198,7 @@ The orchestrator executes these steps in order:
13. **Review state write** — persists the current diff tip so future `--since-last-review` runs can narrow the scope
14. **Footer** — prints elapsed time, token usage stats, and writes the deterministic verdict JSON to `<CR_DIR>/verdict.json` (consumed by the `code` plugin's `run-loop.sh`)

(Step numbers in this list are illustrative; the canonical 30-stage ordering lives in `prepare-run`'s `run_plan.json`.)
(Step numbers in this list are illustrative; the canonical 30-stage ordering lives in `prepare-run`'s `run_plan.json`. Steps 2–8 — the deterministic prefix through routing and partitioning — run in a single process via the `run-prefix` helper; the orchestrator walks the reviewer/validation/presentation tail from step 9 onward.)

## Helper CLI (`code_review_helpers.py`)

Expand Down
6 changes: 4 additions & 2 deletions plugins/code-review/SCHEMA.md
Original file line number Diff line number Diff line change
Expand Up @@ -550,8 +550,10 @@ The exit code is `0` for every well-formed result (including `error`) — the
honored exactly as the Walker Contract prescribes: `abort` → `error`; `continue`
→ proceed; `continue_with_coverage_gap` → proceed after writing an
`agent-failure` system finding to `agent_<stage>-failed.json` (collected by
`collect-findings`). A `route` failure is surfaced as `error` with
`failed_stage: "route"`.
`collect-findings`). A Gate B `route` failure is surfaced as `error` with
`failed_stage: "stage_19_cache_check"` (route is not a plan stage, so the error
anchors on the stage a per-stage fallback resumes from — re-running cache-check
→ route → partition).

---

Expand Down
36 changes: 33 additions & 3 deletions plugins/code-review/commands/start.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,10 @@ The walk is hybrid:

**Turn & context discipline (cost).** Cache cost scales with carried context × turn count, so keep both small:
- **Never read large artifacts into the orchestrator's context.** `diff_data.json`, `patches_*.txt`, and per-file diffs are passed to helpers and reviewers as **file-path arguments**, never `cat`/`Read` into the walk. Reviewers read patches themselves (see the spawn skill's anti-inline rule). The only large file the orchestrator reads is `review_result.json` at the present stage, once, with the per-section display caps the present skill already applies.
- **Batch deterministic helper stages.** As a turn-count optimization you MAY chain a run of consecutive `helper`-kind stages into one `Bash` call (`cmd1 && cmd2 && …`, each redirecting stdout per its `stdout` field) — but ONLY a run in which **every** stage declares `on_failure: abort`, **none** has a `GATES` entry firing after it, **none** is a branching-gate boundary (A/B/C/D) or an `agent_fleet`/`present`/singleton-dispatch stage, and no stage's args depend on a value an in-batch predecessor printed to stdout. Those constraints make recovery unambiguous: there is no gate to interleave, and no `continue` stage whose successors the `&&` short-circuit would wrongly skip. After the chain returns, confirm each chained stage's `expected_outputs`; if the chain exited non-zero, an `abort` stage failed, so **abort** (do not run any gate against the partial batch). Any stage with an associated gate, a non-`abort` `on_failure` (`continue` / `continue_with_coverage_gap`), or a stdout dependency runs solo under the normal one-stage-at-a-time walk. When in doubt, don't batch.
- **Batch the deterministic prefix with `run-prefix`.** The entire deterministic prefix (stages 01→19b) runs in ONE process via the `run-prefix` helper — see the **Deterministic Prefix — `run-prefix` loop** section below — collapsing ~19 helper stages (and their ~4 serial model turns each) into a handful of orchestrator turns. This is the default and the single biggest turn-count saving. Only in the per-stage **fallback walk** (when `run-prefix` errored or is unavailable) may you additionally chain a run of consecutive `helper`-kind stages into one `Bash` call (`cmd1 && cmd2 && …`, each redirecting stdout per its `stdout` field) — but ONLY a run in which **every** stage declares `on_failure: abort`, **none** has a `GATES` entry firing after it, **none** is a branching-gate boundary (A/B/C/D) or an `agent_fleet`/`present`/singleton-dispatch stage, and no stage's args depend on a value an in-batch predecessor printed to stdout. Those constraints make recovery unambiguous: there is no gate to interleave, and no `continue` stage whose successors the `&&` short-circuit would wrongly skip. After the chain returns, confirm each chained stage's `expected_outputs`; if the chain exited non-zero, an `abort` stage failed, so **abort** (do not run any gate against the partial batch). Any stage with an associated gate, a non-`abort` `on_failure` (`continue` / `continue_with_coverage_gap`), or a stdout dependency runs solo under the normal one-stage-at-a-time walk. When in doubt, don't batch.
- **Narrate sparingly.** Emit only the operator-essential lines the per-stage notes mark for printing (review-mode line, cache status, fast-path notice, verdict). Do not echo intermediate stage progress as prose.

Four runtime gates modify walker default behavior (they are runtime-driven and either replace the default walk or add a condition on top of a plan stage):
Four runtime gates modify walker default behavior (they are runtime-driven and either replace the default walk or add a condition on top of a plan stage). **Gate A and Gate B fire inside the prefix — `run-prefix` performs them and surfaces the result to you (as `hygiene_exit` / `ready_for_reviewers`); you only execute their mechanics in the per-stage fallback walk. Gate C and Gate D fire in the walked tail (after `stage_20`), so you always apply them.**
1. **Gate A** — after `stage_12_hygiene`, if `flags.hygiene_only` is true: present hygiene findings and **EXIT** (no further stages, no verdict, no footer).
2. **Gate B** — after `stage_19_cache_check`, invoke `route` (model routing) to compute `fast_path` and `max_bha_agents`. `fast_path == true` skips `stage_17_partition` entirely and drives a single fast-path reviewer in `stage_20`.
3. **Gate C** — before `stage_26_cache_update`, skip if `fast_path == true` OR `CACHE_DIR` is empty.
Expand Down Expand Up @@ -200,8 +200,36 @@ If MODE=github, also Read `${CLAUDE_PLUGIN_ROOT}/prompts/github-review.md` now.

---

## Deterministic Prefix — `run-prefix` loop

The deterministic prefix — every stage from `stage_01_setup` through Gate B (`route`), `stage_17_partition`, and `stage_19b_derive_spawn_spec` (or `stage_19c_derive_static_spec` in `--depth shallow`) — is run **in one process** by the `run-prefix` helper instead of one orchestrator turn per stage. This is the default path. Do **not** walk these stages one at a time (the per-stage **Walker Contract** below is the labeled fallback, used only when `run-prefix` returns `error` or is unavailable, e.g. an old marketplace cache with no `run-prefix` subcommand).

`run-prefix` reads `run_plan.json` + `setup.json` from `<CR_DIR>` (both written in stage 0), resolves each stage's placeholder tokens from prior-stage artifacts, honors every stage's `on_failure` policy and validation gate exactly as the Walker Contract prescribes, and pauses only at genuine decision points — emitting a status JSON. See `SCHEMA.md` §7b for the full result contract.

Invoke it, then dispatch on the result's `next_action` (authoritative — read the field, not the exit code, which is `0` for every well-formed result):

```bash
python3 <HELPERS> run-prefix --cr-dir <CR_DIR> --plugin-root <PLUGIN_ROOT>
```

Read the status JSON from stdout and act:

1. **`needs_singleton`** — a PLN-725 singleton needs an agent (`singleton` is `"extract_signals"` or `"coverage_critic"`). Invoke the `code-review:singleton-dispatch` skill for that stage — it reads the prepare manifest `run-prefix` just wrote and spawns one synchronous Task, writing `pln725_<singleton>.json`. Then **re-invoke** `run-prefix` with `--resume-from <resume_stage>` (the `resume_stage` from the result — the sibling consolidate stage) and dispatch on the new result. Both singletons fire on most runs, so expect up to two such pauses per review.

2. **`hygiene_exit`** — **Gate A** (hygiene-only). Mark the pre-review todos `run-prefix` completed (`Parse scope and get diff data`, `Run deterministic hygiene checks`) `completed`. If `cache_status_message` is non-null, print it. Render `<CR_DIR>/hygiene.json` using the **Hygiene Findings Format (Gate A render target)** section below. If `MODE=github`, do the Gate A GitHub write (`.closedloop-ai/code-review-summary.md` + `.closedloop-ai/code-review-findings.json`). Then mark "Present hygiene findings" `completed` and **EXIT** — no route, partition, agents, validate, finalize, verdict, or footer.

3. **`ready_for_reviewers`** — the whole deterministic prefix is done; `run-prefix` has already run Gate B `route`, partitioned (or skipped partition in fast-path), and derived the spawn spec. Mark the pre-review todos `run-prefix` completed (`Parse scope and get diff data`, `Run deterministic hygiene checks`, `Assess scope and route models`) `completed`. Cache `FAST_PATH` (`fast_path`) and `MAX_BHA_AGENTS` (`max_bha_agents`) from the result. **Read `CACHE_DIR` from `<CR_DIR>/cache_config.json` (`cache_dir`, empty when no cache)** — the run-prefix loop skipped the walk where the fallback would have cached it, and Gate C, Gate D, and the notices below all need it. If `cache_status_message` is non-null, print it. If `FAST_PATH` is true, read `<CR_DIR>/spawn.json` (`route.models.fast_path_reviewer`) and print `"Fast path selected: 1 reviewer (<fast_path_reviewer>)."` (matching the Gate B fallback notice) and — when `CACHE_DIR` is set — `"BHA Cache: bypassed in fast-path mode."`, and replace the "Spawn reviewer agents in parallel" todo with "Run fast-path review". Then continue with the **Walker Contract** below **starting at `stage_20_spawn_reviewers`** — the reviewer fleet and everything after it are still walked one stage at a time. (Any `<CACHE_DIR>` / `<REVIEW_ROOT>` / other tokens the tail stages need are resolved from the on-disk artifacts `run-prefix` wrote, per the token table.)

4. **`error`** — a stage aborted or a validation gate failed (`failed_stage` names the stage; `message` carries the diagnostic). Partial artifacts on disk are preserved. **Fall back** to the per-stage **Walker Contract** below, resuming the walk from `failed_stage` (re-run only that stage forward). If a downstream stage keeps failing, surface `message` to the operator.

The routing/cache notices (Gate A cache line, Gate B fast-path + cache line) are the operator-essential output of this loop — emit them and nothing else; do not narrate the individual prefix stages `run-prefix` ran.

---

## Walker Contract

**When this applies.** The Walker Contract governs the **reviewer/verify/present tail** — `stage_20_spawn_reviewers` onward — which is always walked one stage at a time. In the normal flow the `run-prefix` loop above has already run the deterministic prefix (stages 01→19b) and handed off at `stage_20_spawn_reviewers`, so **begin the walk there**. The Contract is ALSO the **per-stage fallback for the prefix**: if `run-prefix` returned `error` (or is unavailable), walk the prefix stages one at a time from `failed_stage`, applying the same steps 1-8 and the Branching Gates (A/B) below. Everything in this section — token resolution, `on_failure`, gates, singleton dispatch — is exactly what `run-prefix` reproduces internally; it is documented here as the canonical contract and the recovery path.

**Reading `<CR_DIR>/*.json` artifacts.** The walker reads run-plan output JSON to resolve placeholder tokens (`<DIFF_SCOPE>`, `<CACHE_DIR>`, etc.). If your session has a hook that intercepts the `Read` tool on generated artifacts (e.g. a code-discovery gate that demands codebase-memory-mcp lookups), fall back to `cat` via `Bash` — these are pipeline artifacts, not source code.

Walk `STAGES` in array order. For each stage:
Expand Down Expand Up @@ -263,6 +291,8 @@ If a token's source file does not exist yet (a prior stage that produces it was

Four runtime gates modify walker default behavior. Each is documented below with the exact stage boundary it fires at.

**Gate A and Gate B fire inside the prefix, so `run-prefix` performs them for you.** In the normal flow you never execute the mechanics below — `run-prefix` runs Gate A's hygiene-only exit (surfaced as `next_action: "hygiene_exit"`) and Gate B's `route` + partition (surfaced as `next_action: "ready_for_reviewers"` with `fast_path` / `max_bha_agents`), and the **Deterministic Prefix — `run-prefix` loop** section tells you what to print and where to hand off. The Gate A/B detail below is the canonical spec and the recipe for the **per-stage fallback walk** (when `run-prefix` errored). **Gate C and Gate D fire after `stage_20`, in the walked tail, so you always apply them yourself** as described.

### Gate A — After `stage_12_hygiene`: Hygiene-only early exit

If `FLAGS.hygiene_only` is true (or the equivalent `--hygiene-only` was passed):
Expand Down Expand Up @@ -397,7 +427,7 @@ When the walker reaches `stage_23_verify_findings`, invoke the `code-review:veri

## PLN-725 Single-Agent Dispatch

Walker-contract step 6 points here. When the stage just finished is `stage_11_extract_signals` or `stage_15_coverage_critic`, invoke the `code-review:singleton-dispatch` skill. The skill owns the full protocol: reading the prepare manifest's `status` (`cache_hit` / `skipped` → no dispatch; `needs_agent` → spawn one synchronous singleton Task), the by-convention `pln725_*.json` agent write target, and the fail-closed semantics the sibling consolidate stage relies on.
The `run-prefix` loop's `next_action: "needs_singleton"` handler points here (and, in the per-stage fallback walk, so does Walker-Contract step 6). When `run-prefix` reports a singleton — or, in the fallback walk, when the stage just finished is `stage_11_extract_signals` or `stage_15_coverage_critic`invoke the `code-review:singleton-dispatch` skill. The skill owns the full protocol: reading the prepare manifest's `status` (`cache_hit` / `skipped` → no dispatch; `needs_agent` → spawn one synchronous singleton Task), the by-convention `pln725_*.json` agent write target, and the fail-closed semantics the sibling consolidate stage relies on. In the `run-prefix` flow the manifest already exists on disk (the runner wrote it); after the skill writes `pln725_<singleton>.json`, re-invoke `run-prefix --resume-from <resume_stage>` per the loop.

<!-- replaced-by-skill: code-review:singleton-dispatch — DO NOT add inline singleton-dispatch content here -->

Expand Down
2 changes: 1 addition & 1 deletion plugins/code-review/skills/singleton-dispatch/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ Spawn one synchronous `Task` (do **not** set `run_in_background: true`). Unlike
| `{INPUT_PATH}` | `manifest.input_path` |
| `{OUTPUT_PATH}` | The by-convention agent write target from the table above — `<CR_DIR>/pln725_extract_signals.json` (stage_11) or `<CR_DIR>/pln725_coverage_critic.json` (stage_15). **NOT** `manifest.output_path`. |
| `{STAGE_LABEL}` | `"signal-extraction"` for stage_11, `"coverage-critic"` for stage_15. |
4. After the Task returns, advance the walker to the sibling consolidate stage. No `TaskOutput` call — that's for background tasks; synchronous Tasks complete before control returns to the walker.
4. After the Task returns, hand control back to the caller. **In the default `run-prefix` flow** (the caller is `start.md`'s Deterministic Prefix loop): do NOT advance the walker — return, and `start.md` re-invokes `run-prefix --resume-from <resume_stage>`, which runs the sibling consolidate stage. **In the per-stage fallback walk** (the caller is walking stages one at a time): advance the walker to the sibling consolidate stage yourself. Either way, no `TaskOutput` call — that's for background tasks; synchronous Tasks complete before control returns.

### Failure semantics

Expand Down
Loading