diff --git a/CHANGELOG.md b/CHANGELOG.md index 99c017c..2b47c01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ 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.5.0 + +#### Changed +- GitHub-mode reviews now dispatch the reviewer fleet (`stage_20`) as parallel background tasks collected with a blocking `TaskOutput`, the same as local mode, instead of running each reviewer synchronously one at a time. The verifier fleet (`stage_23`) already dispatched in the background in both modes; its contract is now stated explicitly as parallel with a matching fallback note so the two fleets are documented consistently. Headless `claude -p` waits for background subagents to finish before exiting (Claude Code v2.1.182+), so a code-review CI run's reviewer stage now takes the wall-clock of the slowest reviewer rather than the sum of all reviewers, cutting shallow-tier CI review time. The synchronous one-at-a-time fallback for Claude Code older than v2.1.182 is documented as an operator/environment concern (the orchestrating agent cannot detect its own CLI version at runtime, so it always takes the parallel path); the per-agent background wait can be raised past its 10-minute default via `CLAUDE_CODE_PRINT_BG_WAIT_CEILING_MS`, and runs that background reviewers must allow the `TaskOutput` tool. Updates the `spawn-reviewers` and `verify-findings` skills and the `start.md` walker contract. + ### code-review v3.4.1 #### Fixed diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index 9422b0e..dbb922c 100644 --- a/plugins/code-review/.claude-plugin/plugin.json +++ b/plugins/code-review/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "code-review", "description": "Code review plugin", - "version": "3.4.1", + "version": "3.5.0", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code-review/commands/start.md b/plugins/code-review/commands/start.md index fc27177..243f8d2 100644 --- a/plugins/code-review/commands/start.md +++ b/plugins/code-review/commands/start.md @@ -390,7 +390,7 @@ These notes annotate the run-plan stages with anything not obvious from the plan - **stage_17_partition**: positioned in the run plan array after `stage_19_cache_check` so Gate B's `route` invocation runs first and supplies `--max-bha-agents`. The stage id retains its `_17_` prefix as a stable label (stage ids are not strict ordinals; execution order follows array position). Reads `partitions.json` afterward; entries shape `{id, files, total_loc, is_test_only}` with `files[].file` (NOT `path`), `files[].loc`, `files[].is_test`, optional `files[].line_range`. **PLN-774**: top-level keys also carry `partition_mode` (`"unified"` | `"partitioned"`), `partition_count`, `total_changed_loc`, and `unified_threshold_loc`. When total changed LOC ≤ `BHA_UNIFIED_THRESHOLD_LOC` (default 5000, settable via `.closedloop-ai/settings/code-review.json:bha_unified_threshold_loc`; `0` = always partition), the partitioner emits a single unified partition holding every file so cross-region invariants stay visible to one BHA reviewer's context. `cmd_verify_prepare` propagates `partition_mode` + `partition_count` into `verify_manifest.json` for the presenter footer. The `stats.verification.by_reviewer` block naturally labels BHA by partition via the filename-derived `reviewer` field (`agent_bha_p0.json` → `reviewer='bha_p0'`) — no extra split logic is needed; under unified mode only a single `bha_p0` bucket exists because there is only one partition. - **stage_19_cache_check**: writes `/cache_result.json` (stats), `/agent_cached_bha.json` (cached BHA findings, glob-compatible with `agent_*`), `/uncached_diff_data.json` (filtered diff_data for uncached files). Do NOT print the cache status here — it is printed in Gate A (hygiene exit) or Gate B (after route). - **stage_19b_derive_spawn_spec** (PLN-725): runs `derive-spawn-spec`. Reads `/coverage.json` (`final` section — post-arbitrate), `/partitions.json`, and `/spawn.json` (`route` section, written by Gate B's `cmd_route --cr-dir`) and writes `/spawn.json` (`spec` section) — a flat list of agent descriptors keyed by `agent_id` (e.g. `bha_p0`, `bhb`, `auditor`, `domain_0`, `fast`) carrying `reviewer`, `model`, `partitioned`, `patches_file`, `source`, `bucket`, and (for BHA) `partition_id` + `is_test_only`. The fast-path branch from Gate B is honored (`fast_path: true` → single `fast` agent, bucket walk skipped). BHA descriptors are capped at `coverage_plan.budget.bha_partitions` (the post-arbitrate cap, which may be < the partitioner's output count); the excess partitions land in `skipped[]` with `reason: "budget_capped"`. A BLOCKING verify verdict (`budget.gated_by_verify: true`) drives **plan sanitization**: only `source: "core"` reviewers survive; every `rule` or `critic` entry is moved to `skipped[]` with `reason: "gated_by_verify"` (the canonical BLOCKING finding from stage_15c remains the operator-facing signal). Required-bucket skips with non-benign reasons (everything except `deferred_pln723`, `no_partitions`, `gated_by_verify`) generate coverage-gap findings appended to `/coverage_gaps.json` so finalize-result picks them up — the spec-driven dispatch never silently drops a required reviewer. `on_failure: continue` — a derive failure writes a sentinel spec with `arbitrate_status: "fallback"` (`fallback_reason` ∈ {`coverage_plan_missing_or_malformed`, `partitions_missing_or_malformed`}), which the stage_20 orchestrator interprets as "ignore the spec, use the static reviewer table fallback in the `code-review:spawn-reviewers` skill." Note: stage_19b depends only on `stage_16_arbitrate_budget`, NOT on `stage_17_partition`, so Gate B's fast-path branch (which skips stage_17) can still reach stage_20 with a fast descriptor. -- **stage_20_spawn_reviewers**: agent_fleet stage. Invoke the `code-review:spawn-reviewers` skill. The skill reads `/spawn.json` (`spec` section) first and dispatches one Task per agent descriptor (using the `agent_id`, `reviewer`, `model`, and `patches_file` from the spec). If `spawn.json` is missing, its `spec` section is absent, or it marks `arbitrate_status: "fallback"`, the skill walks its static reviewer table fallback instead — a derive failure must never block review. In `MODE=github`, the walker must follow the skill's synchronous standard-flow branch: do not use `TaskOutput`, watcher files, sleep loops, polling loops, or turn-ending waits as replacements for synchronous reviewer completion. `stage_20` must complete every GitHub synchronous reviewer and retry, leaving no reviewer task still running, or fail before `stage_21_collect_findings`. +- **stage_20_spawn_reviewers**: agent_fleet stage. Invoke the `code-review:spawn-reviewers` skill. The skill reads `/spawn.json` (`spec` section) first and dispatches one Task per agent descriptor (using the `agent_id`, `reviewer`, `model`, and `patches_file` from the spec). If `spawn.json` is missing, its `spec` section is absent, or it marks `arbitrate_status: "fallback"`, the skill walks its static reviewer table fallback instead — a derive failure must never block review. In **both** `MODE=local` and `MODE=github` the skill dispatches the fleet as parallel background tasks and collects every one with a blocking `TaskOutput` (headless `claude -p` awaits background subagents on Claude Code v2.1.182+). `stage_20` must collect every reviewer and its retries — leaving no reviewer task still running — before proceeding to `stage_21_collect_findings`; watcher files, sleep loops, polling loops, and turn-ending "wait" turns are forbidden substitutes for the blocking `TaskOutput` collection. - **stage_20b_verify_spawn** (PLN-725): runs `verify-spawn`. Reads `/spawn.json` (`spec` section) and globs `/agent_*.json`; for every descriptor with `bucket: "required"` that has no on-disk output, appends a coverage-gap finding to `/coverage_gaps.json` (reason `spawn_missing_required_agent`) and records the omission in `/spawn.json` (`verification` section). Missing best-effort descriptors are recorded for telemetry but emit no finding — best-effort omissions are budget-driven, not coverage gaps. No-ops cleanly when the spec is missing (`spec_missing`), marks fallback (`spec_fallback`), or contains no agents (`spec_empty`). `on_failure: continue` — a verification bug must never block review; worst case is missing telemetry, not a halted pipeline. Wired before `stage_21_collect_findings` so the gap findings land in `coverage_gaps.json` in time for `cmd_finalize_result` to merge them into the canonical envelope. - **stage_22_validate**: writes `/findings_validated.json` via `> /findings_validated.json` redirection. Validates finding scope and applies the out-of-hunk confidence gate. P2+ findings whose `line` falls outside the file's changed range survive when `confidence > out_of_hunk_confidence_floor` (default `0.80`, operator-tunable via `.closedloop-ai/settings/code-review.json:out_of_hunk_confidence_floor`, range `[0.0, 1.0]`) — this admits legitimate companion-change findings (e.g. a signature change in the diff window leaving stale sibling call sites just outside it) while still filtering low-confidence noise. Survivors get tagged `out_of_hunk_kept: true` so presenters can label them as companion-change without re-deriving hunk membership; the validate-stats block exposes `kept_out_of_hunk` and `discarded_out_of_hunk_low_confidence`. The comparison is strict `>`, so setting the floor to `1.0` is a kill switch (nothing can clear); setting it to `0.0` lets every out-of-hunk P2+ through (lean on the PLN-722 verifier downstream). Per-finding verification (stage_23) still applies on top, so noise that surfaces here gets a second-pass CONFIRMED/REJECTED verdict. - **stage_22b_verify_prepare** (PLN-722): tier-selects findings for verification per the canonical table — BLOCKING/HIGH always; MEDIUM with confidence < 0.85 yes; MEDIUM with confidence ≥ 0.85 no; LOW (P3) no; `category: "Hygiene"` no; `source: "injection-detector"` no. Ranks the eligible set by `severity_weight × confidence`, caps at `VERIFY_MAX_VERIFICATIONS = 50`, and writes (a) `/verify_manifest.json` with `to_verify[]` + `skipped_no_verification[]` + `deferred_budget[]` + `cache_hits[]`, and (b) `/verifier_inputs/.json` per eligible finding. When `--cache-dir` is set, fresh verifier outputs from a prior run for the same `(finding_id, code_snippet_hash, model, prompt_hash)` tuple are pre-materialized at `agent_verifier_.json` and skipped from `to_verify[]` (logged under `cache_hits[]`). `on_failure: continue` is intentional — verify-prepare failure degrades to "no verifier this run", not a pipeline abort. @@ -407,9 +407,9 @@ These notes annotate the run-plan stages with anything not obvious from the plan When the walker reaches `stage_20_spawn_reviewers`, invoke the `code-review:spawn-reviewers` skill. The skill owns the full reviewer-fleet dispatch: spawn-spec consumption (`spawn.json.spec`, the authoritative path), GRAPH_PROJECT resolution, the per-agent prompt template and role suffixes (Bug Hunter A/B, Unified Auditor, Domain Critics, Design Critic, Impact Analyzer), the context-budget constraints, the standard / fast-path / all-cached-BHA / gated-by-verify branches, the static-table fallback (`arbitrate_status: "fallback"`), the spawn + collection contract, and agent-failure recovery. -The skill is invoked for both `MODE=local` and `MODE=github`, but standard-flow Task scheduling is mode-specific: GitHub mode dispatches reviewers synchronously, while local mode preserves parallel background dispatch plus blocking collection. The verifier fleet (`stage_23`) and the PLN-725 single-agent dispatch (`stage_11` / `stage_15`) are **not** in this skill; they are owned by the `code-review:verify-findings` and `code-review:singleton-dispatch` skills respectively. +The skill is invoked for both `MODE=local` and `MODE=github` with the **same** parallel Task scheduling: spawn every standard-flow reviewer as a background task and collect them all with blocking `TaskOutput`, so `stage_20` wall-clock is the slowest reviewer rather than the sum. The verifier fleet (`stage_23`) and the PLN-725 single-agent dispatch (`stage_11` / `stage_15`) are **not** in this skill; they are owned by the `code-review:verify-findings` and `code-review:singleton-dispatch` skills respectively. -GitHub headless mode has a walker-level guard in addition to the skill contract: standard-flow reviewers and retries must be dispatched synchronously, and the walker must not proceed to `stage_21_collect_findings`, emit a final summary, mark the review complete, or end the assistant turn while any GitHub reviewer remains outstanding. Watcher files, sleep loops, polling loops, background `TaskOutput` waits, and "I'll continue when notified" turns are forbidden substitutes for completing the synchronous reviewer response. +GitHub headless mode carries one walker-level guard: the turn stays alive only while a pending synchronous tool call is outstanding, so the blocking `TaskOutput` collection is what keeps `claude -p` running until reviewers finish (headless awaits background subagents on Claude Code v2.1.182+). The walker must not proceed to `stage_21_collect_findings`, emit a final summary, mark the review complete, or end the assistant turn while any reviewer remains outstanding. Watcher files, sleep loops, polling loops, and "I'll continue when notified" turns are forbidden — they end the turn with no pending call and the process exits before the tail stages run. On Claude Code < v2.1.182 background subagents are not awaited — but the agent cannot self-detect its CLI version, so this is an operator/environment concern, not a runtime branch: `claude-code-action@v1` ships ≥ v2.1.182 (always parallel), and an operator forced onto an older CLI must switch the skill to its legacy synchronous fallback. Decomposition rationale: ~470 lines of reviewer-fleet dispatch content was extracted as a skill so the orchestration spine stays lean and the content no longer loads into orchestrator context during the deterministic prefix (stages 0-19) or on hygiene-only / full-cache-hit runs that never reach `stage_20`. diff --git a/plugins/code-review/skills/spawn-reviewers/SKILL.md b/plugins/code-review/skills/spawn-reviewers/SKILL.md index 65e3a2e..36f9cfc 100644 --- a/plugins/code-review/skills/spawn-reviewers/SKILL.md +++ b/plugins/code-review/skills/spawn-reviewers/SKILL.md @@ -5,7 +5,7 @@ description: Spawn and collect the reviewer fleet at stage_20_spawn_reviewers. C # Reviewer Fleet Dispatch (stage_20_spawn_reviewers) -This skill is the canonical reviewer-fleet dispatcher for `/code-review` at `stage_20_spawn_reviewers`. It is split out of `commands/start.md` so the orchestration spine stays lean; the orchestrator invokes it when the walker reaches `stage_20_spawn_reviewers`. The content below is authoritative for both `MODE=local` and `MODE=github`, with mode-specific Task scheduling: GitHub standard flow dispatches reviewers synchronously, while local standard flow preserves parallel background dispatch plus blocking collection. +This skill is the canonical reviewer-fleet dispatcher for `/code-review` at `stage_20_spawn_reviewers`. It is split out of `commands/start.md` so the orchestration spine stays lean; the orchestrator invokes it when the walker reaches `stage_20_spawn_reviewers`. The content below is authoritative for both `MODE=local` and `MODE=github`, which use the same Task scheduling: parallel background dispatch collected with blocking `TaskOutput` (headless `claude -p` awaits background subagents on Claude Code v2.1.182+, so the fleet runs concurrently in CI too). The verifier fleet (`stage_23`) and the PLN-725 single-agent dispatch (`stage_11` / `stage_15`) are **not** in this skill — they are owned by the `code-review:verify-findings` and `code-review:singleton-dispatch` skills respectively. @@ -325,19 +325,17 @@ When GRAPH_PROJECT is empty, grep imports instead. ### Spawn + Collection Contract (standard flow) -**First branch on `MODE`.** GitHub and local runs intentionally use different Task scheduling because GitHub headless mode cannot survive outstanding background reviewers after the assistant turn ends. +**Both `MODE=local` and `MODE=github` dispatch the fleet as parallel background tasks and collect them with blocking `TaskOutput`.** Spawn every standard-flow reviewer with `run_in_background: true` — all agents in a single message or across a few messages — then call `TaskOutput` (block: true) for every spawned agent before the walker proceeds past `stage_20`. The fleet runs concurrently, so `stage_20` wall-clock is the **slowest** reviewer, not the sum of all reviewers. -**GitHub mode (`MODE=github`): dispatch synchronously.** Spawn exactly one standard-flow reviewer at a time and wait for its Task response before spawning the next descriptor. Omit `run_in_background` or set `run_in_background: false`; never set it to `true` for GitHub standard-flow reviewers. Do not use `TaskOutput`, watcher files, sleep loops, polling loops, or "wait for background task" turns in GitHub standard flow. Each reviewer must finish, write `/agent_{AGENT_ID}.json` (or return the write-denied payload below), and return `DONE findings=N file=...` before the walker proceeds to the next reviewer. When `stage_20` completes in GitHub mode there must be no reviewer task still running. - -**Local mode (`MODE=local`): spawn ALL agents at once.** Use `run_in_background: true` on every standard-flow reviewer. You can spawn all agents in a single message or across a few messages. +> **Headless requirement (GitHub mode).** GitHub reviews run under headless `claude -p`, which **waits for background subagents to complete before exiting** (their output is part of the final result) as of **Claude Code v2.1.182+** — the version `anthropics/claude-code-action@v1` ships. A blocking `TaskOutput` is a pending synchronous tool call, so the assistant turn never "ends" with an outstanding reviewer and the run does not die. The per-agent wait is capped at **10 minutes** by default; when reviewers can run long, raise it with the `CLAUDE_CODE_PRINT_BG_WAIT_CEILING_MS` env var (or set `0` for no cap) so a slow reviewer is not orphaned, and add `TaskOutput` to the run's `--allowedTools`. +> +> **Legacy fallback (Claude Code < v2.1.182 only) — an operator concern, NOT a runtime branch the agent picks.** The orchestrating agent **cannot introspect its own CLI version from inside the prompt**, so it has no decision procedure to choose between the two paths at runtime — it always follows the parallel primary above. This fallback is therefore an environment/operator decision, not a branch the live agent resolves. `anthropics/claude-code-action@v1` ships ≥ v2.1.182, so standard GitHub CI is always on the parallel path and no action is needed. An operator who must run an older CLI — where headless does not await background subagents, so a backgrounded reviewer dies when the turn ends — must edit this skill to dispatch standard-flow reviewers synchronously one at a time (`run_in_background: false`, wait for each `DONE findings=N file=...` before spawning the next) so no reviewer is ever left outstanding. That pre-v2.1.182 path is correct but serial (wall-clock = sum of reviewers). **Agents write findings to files — NOT to their response.** Each agent writes its findings JSON to `/agent_{AGENT_ID}.json` and returns only a one-line status (`DONE findings=N file=...`). `TaskOutput` responses are ~50 tokens each instead of 2-5K tokens, so you can collect ALL agents at once without context overflow. **Write-denied fallback:** If an agent's Write tool is denied (restrictive project permissions), the agent outputs findings in `` tags in its response with `DONE findings=N file=WRITE_DENIED`. When collecting, if a response contains `WRITE_DENIED`, extract the JSON from `` tags and write it to `/agent_{AGENT_ID}.json` yourself. -**Local collection (MANDATORY for `MODE=local`):** Call `TaskOutput` (block: true) for every spawned local background agent. You MUST collect ALL agents before the walker proceeds past `stage_20`. Do NOT read disk files or start validation until every `TaskOutput` call has returned. In headless GitHub mode there is no asynchronous completion notification, so GitHub standard flow uses the synchronous branch above instead of backgrounding and collecting with `TaskOutput`. - -For local mode, call all `TaskOutput` calls in a **single message** (parallel) so they resolve together. For GitHub mode, check each synchronous Task response immediately. In either mode, handle each response: +**Collection (MANDATORY).** Call `TaskOutput` (block: true) for every spawned background agent. You MUST collect ALL agents before the walker proceeds past `stage_20`. Do NOT read disk files or start validation until every `TaskOutput` call has returned. Call all `TaskOutput` calls in a **single message** (parallel) so they resolve together, and do not substitute watcher files, sleep loops, polling loops, or "I'll continue when notified" turns for the blocking collection. For each response, handle: 1. `DONE findings=N file=...` (not WRITE_DENIED) — output file is on disk, nothing to do. 2. `DONE findings=N file=WRITE_DENIED` — extract JSON from `` tags and write to `/agent_{AGENT_ID}.json`. 3. Agent didn't report `DONE` — check if its output file exists on disk using Bash. @@ -349,7 +347,7 @@ If any agent failed (context overflow, subscription limits, timeout) or its outp 1. **Log the failure**: Record which agent failed and why (e.g., `"Bug Hunter A partition 2: context overflow"`). 2. **If failed agent is BHA (partitioned)**: halve the failed partition (LOC budget ÷ 2) and re-spawn with `model: "haiku"` and `subagent_type: "code-review:code-review-worker"`. The re-spawned agent writes to a new output file. 3. **If failed agent is non-partitioned (BHB / Impact Analyzer / Design Critic / Unified Auditor / Domain Critic)**: re-spawn the same role once with `model: "haiku"` and the same file assignment. Keep the role's worker type — BHB, the Impact Analyzer, and the Design Critic re-spawn as `code-review:code-review-worker-graph` (with the same `GRAPH_PROJECT`); Auditor/Domain Critic re-spawn as `code-review:code-review-worker`. -4. **Retry uses the same mode branch**: GitHub retries are synchronous and must finish before the next descriptor or downstream stage; local retries may use the local background-plus-`TaskOutput` collection contract. +4. **Retry uses the same background-plus-`TaskOutput` contract**: re-spawn the failed reviewer with `run_in_background: true` and collect it with a blocking `TaskOutput` before the walker proceeds past `stage_20` (in both modes). On Claude Code < v2.1.182 (legacy synchronous fallback), the retry is synchronous and must finish before any downstream stage. 5. **Second failure → skip with warning**: if the recovery attempt fails, log a warning (`"⚠️ {agent_name} skipped — {N} files not reviewed due to agent failures"`) and continue. Do NOT fall back to reviewing in the main conversation — this would load patches into the orchestrator's context and recreate the overflow problem on large PRs. Skipped scope must be listed in the output for manual follow-up. 6. **Continue collecting**: do not block the pipeline on a single agent failure. The walker's `on_failure: continue_with_coverage_gap` for `stage_20` ensures the run completes even if some partitions are unreviewed. @@ -364,7 +362,7 @@ The fast-path spawns a single agent that performs all review passes in one run. **Fast-Path Agent settings:** - `subagent_type`: `"code-review:code-review-worker-graph"` (the fast-path agent runs a BHB cross-file pass, so it gets the graph-aware worker; pass the resolved `GRAPH_PROJECT` into its prompt) - `model`: from `spawn.json.route -> models.fast_path_reviewer` (NOT hardcoded) -- `run_in_background`: `false` (spawn the single fast-path agent SYNCHRONOUSLY; backgrounding one agent buys no parallelism and is fatal in headless mode, see "Fast-Path Spawn + Collection" below) +- `run_in_background`: `false` (spawn the single fast-path agent SYNCHRONOUSLY; there is only one agent, so backgrounding buys no parallelism, see "Fast-Path Spawn + Collection" below) - `AGENT_ID`: `"fast"` - ``: `{CR_DIR}/agent_fast.json` - ``: `{CR_DIR}/patches_all.txt` @@ -449,9 +447,9 @@ Standard severity/priority rules apply. If `domain_critics` is empty, remove the `{DOMAIN_CRITIC_PASS}` placeholder entirely. **Fast-Path Spawn + Collection:** -- Spawn exactly ONE agent (`AGENT_ID: "fast"`) and collect it SYNCHRONOUSLY (MANDATORY). Either spawn the Task with `run_in_background: false` (omitted is fine), in which case the call blocks and returns the `DONE`/findings status directly, or, if you do background it, your VERY NEXT action MUST be a blocking `TaskOutput` for `AGENT_ID: "fast"`. Do NOT emit a final summary, mark a todo complete, or end your turn until the fast-path agent has returned and the remaining stages (`stage_21_collect_findings` through `stage_30_footer`) have run. Backgrounding one agent provides no parallelism and, in headless mode, lets the run exit before those stages execute (see the headless warning below). +- Spawn exactly ONE agent (`AGENT_ID: "fast"`) and collect it (MANDATORY). Because it is a single agent, the simplest form is `run_in_background: false` (omitted is fine), in which case the call blocks and returns the `DONE`/findings status directly; if you do background it, your VERY NEXT action MUST be a blocking `TaskOutput` for `AGENT_ID: "fast"`. Do NOT emit a final summary, mark a todo complete, or end your turn until the fast-path agent has returned and the remaining stages (`stage_21_collect_findings` through `stage_30_footer`) have run. - `DONE ... file=WRITE_DENIED` is a success path, not a failure. Extract `` from `TaskOutput` and write it to `/agent_fast.json`. Retry only when the task fails to return `DONE`, times out/crashes, or returns malformed findings with no usable output file. - On failure (not WRITE_DENIED): retry once with `model: "haiku"`, same `AGENT_ID: "fast"`, same output file `/agent_fast.json`. Delete any existing `agent_fast.json` before retrying. Do NOT create `agent_fast_retry.json`. - If retry also fails: warn and continue with zero fast-path findings. -**Headless mode warning (applies to BOTH the standard flow and the fast path).** In GitHub mode the review runs under headless `claude -p`, where there is NO asynchronous subagent-completion notification: when the orchestrator's assistant turn ends with no pending synchronous tool call, the process terminates immediately (`terminal_reason: "completed"`). If you background a reviewer and then end your turn to "wait" for it, the run dies before `stage_21_collect_findings` through `stage_30_footer` execute, so no `.closedloop-ai/code-review-*` artifacts are written and the workflow posts an empty fallback summary. The GitHub standard-flow synchronous reviewer Task calls, GitHub fast-path synchronous spawn, and local-mode blocking `TaskOutput` collection are the only supported ways to keep the turn alive until reviewers finish; never substitute any of them with watcher files, sleep loops, polling loops, or "I'll continue when notified." +**Headless mode note (applies to BOTH the standard flow and the fast path).** In GitHub mode the review runs under headless `claude -p`. The turn stays alive only while a **pending synchronous tool call** is outstanding — so the supported ways to wait for reviewers are a synchronous Task (fast path) or a **blocking `TaskOutput`** on backgrounded reviewers (standard flow). As of Claude Code **v2.1.182+**, headless `claude -p` also waits for background subagents to finish before exiting (their output is part of the final result), so a `TaskOutput`-collected background fleet completes and writes `stage_21_collect_findings` through `stage_30_footer` normally. What still breaks the run: ending the assistant turn with a reviewer outstanding and **no** pending synchronous call — e.g. watcher files, sleep loops, polling loops, or "I'll continue when notified" turns. Never use those; the process terminates (`terminal_reason: "completed"`) before the tail stages run and the workflow posts an empty fallback summary. (On Claude Code < v2.1.182, background subagents are not awaited — use the legacy synchronous fallback in the Spawn + Collection Contract above.) diff --git a/plugins/code-review/skills/verify-findings/SKILL.md b/plugins/code-review/skills/verify-findings/SKILL.md index 100dd88..4b46a9d 100644 --- a/plugins/code-review/skills/verify-findings/SKILL.md +++ b/plugins/code-review/skills/verify-findings/SKILL.md @@ -30,6 +30,10 @@ This stage runs when the walker reaches `stage_23`. It implements PLN-722's find ### Spawn contract +**Dispatch the whole verifier fleet as parallel background tasks in both `MODE=local` and `MODE=github`** — spawn all `to_verify[]` entries with `run_in_background: true` (in one message or a few), then collect them with blocking `TaskOutput` (below). Do NOT dispatch verifiers one-at-a-time; headless `claude -p` awaits background subagents on Claude Code v2.1.182+, so the fleet runs concurrently in CI too (`stage_23` wall-clock is the slowest verifier, not the sum). + +> **Legacy fallback (Claude Code < v2.1.182 only) — an operator concern, NOT a runtime branch the agent picks.** Same as the reviewer fleet (see `spawn-reviewers/SKILL.md`): the orchestrating agent cannot introspect its own CLI version, so it always takes the parallel path above, and `anthropics/claude-code-action@v1` ships ≥ v2.1.182. An operator forced onto an older CLI — where headless does not await background subagents, so a backgrounded verifier dies when the turn ends — must edit this skill to dispatch verifiers synchronously one at a time so no verifier is left outstanding. This mirrors the reviewer fleet so both stages degrade the same way on older CLIs. + For each entry in `verify_manifest.json.to_verify[]`: 1. Spawn one background `Task` with `subagent_type: "code-review:code-review-worker"`. The agent's tool allowlist (`Read`, `Write`, `Grep`, `Glob`) is identical to the Reviewer Fleet's — no permission changes needed. diff --git a/plugins/code-review/tools/python/test_code_review_helpers.py b/plugins/code-review/tools/python/test_code_review_helpers.py index 7e99d39..43c6233 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -17728,126 +17728,101 @@ def test_collect_findings_depends_on_verify_spawn( assert "stage_20b_verify_spawn" in collect["depends_on"] -class TestFEA2162SpawnReviewerContracts: - """FEA-2162 hardens GitHub-mode reviewer dispatch while preserving - local interactive parallelism. +class TestParallelFleetDispatchContracts: + """Reviewer and verifier fleets dispatch in parallel (background + + blocking ``TaskOutput``) in BOTH local and GitHub headless mode. + + Supersedes the FEA-2162 synchronous-GitHub contract: as of Claude + Code v2.1.182+, headless ``claude -p`` awaits background subagents + (their output is part of the final result), so GitHub CI no longer + needs serial reviewer dispatch. A blocking ``TaskOutput`` is a + pending synchronous call that keeps the headless turn alive. """ - def test_github_standard_flow_requires_synchronous_dispatch(self) -> None: - """GitHub headless standard flow must not leave background Tasks - outstanding when stage_20 completes. + def test_standard_flow_uses_parallel_background_dispatch_in_both_modes(self) -> None: + """Both modes spawn the fleet as background tasks and collect with + blocking TaskOutput — the fleet runs concurrently. """ - skill = _spawn_reviewers_skill_text() - github_section = _extract_section( - skill, - "**GitHub mode (`MODE=github`): dispatch synchronously.**", - "**Local mode (`MODE=local`): spawn ALL agents at once.**", - ) - assert "Spawn exactly one standard-flow reviewer at a time" in github_section - assert "wait for its Task response before spawning the next descriptor" in github_section - assert "run_in_background: false" in github_section - assert "never set it to `true`" in github_section - assert "Do not use `TaskOutput`, watcher files, sleep loops, polling loops" in github_section - assert "there must be no reviewer task still running" in github_section - - mutated = github_section.replace("never set it to `true`", "it may be true") - with pytest.raises(AssertionError): - _assert_github_standard_flow_sync(mutated) - - def test_github_standard_flow_bans_watchers_sleep_and_taskoutput_waiting(self) -> None: - """Background waiting alternatives are equally unsafe in - headless GitHub mode and should fail the source contract. - """ - github_section = _extract_section( + contract = _extract_section( _spawn_reviewers_skill_text(), - "**GitHub mode (`MODE=github`): dispatch synchronously.**", - "**Local mode (`MODE=local`): spawn ALL agents at once.**", + "### Spawn + Collection Contract (standard flow)", + "### Agent Failure Recovery", ) - _assert_github_standard_flow_sync(github_section) + _assert_standard_flow_parallel(contract) + assert "Both `MODE=local` and `MODE=github`" in contract + assert "slowest** reviewer, not the sum" in contract - for forbidden_replacement in ( - "Use `TaskOutput` to wait for background reviewers.", - "Use watcher files to wait for background reviewers.", - "Use sleep loops to wait for background reviewers.", - "Use polling loops to wait for background reviewers.", - ): - mutated = re.sub( - r"Do not use `TaskOutput`, watcher files, sleep loops, polling loops,[^\n]+", - forbidden_replacement, - github_section, - ) - with pytest.raises(AssertionError): - _assert_github_standard_flow_sync(mutated) + # Reverting to synchronous-only dispatch must fail the contract. + mutated = contract.replace("run_in_background: true", "run_in_background: false") + with pytest.raises(AssertionError): + _assert_standard_flow_parallel(mutated) - def test_local_standard_flow_preserves_background_collection(self) -> None: - """Local mode keeps parallel reviewer dispatch and blocking - TaskOutput collection. + def test_headless_requirement_documents_await_and_ceiling(self) -> None: + """The headless requirement must cite v2.1.182+, the wait-ceiling + env var, and the allowlist need for TaskOutput. """ skill = _spawn_reviewers_skill_text() - local_section = _extract_section( - skill, - "**Local mode (`MODE=local`): spawn ALL agents at once.**", - "**Agents write findings to files", - ) - local_collection = _extract_section( - skill, - "**Local collection (MANDATORY for `MODE=local`):**", - "For local mode, call all `TaskOutput` calls", - ) - assert "Use `run_in_background: true` on every standard-flow reviewer" in local_section - assert "Call `TaskOutput` (block: true) for every spawned local background agent" in local_collection + assert "Claude Code v2.1.182+" in skill + assert "CLAUDE_CODE_PRINT_BG_WAIT_CEILING_MS" in skill + assert "anthropics/claude-code-action@v1" in skill + assert "pending synchronous tool call" in skill + assert "add `TaskOutput` to the run's `--allowedTools`" in skill + + def test_legacy_synchronous_fallback_is_documented(self) -> None: + """A synchronous fallback stays documented for Claude Code < v2.1.182.""" + skill = _spawn_reviewers_skill_text() + assert "Legacy fallback (Claude Code < v2.1.182 only)" in skill + assert "synchronously one at a time" in skill - mutated = local_section.replace("run_in_background: true", "run_in_background: false") - with pytest.raises(AssertionError): - assert "run_in_background: true" in mutated + def test_collection_is_blocking_taskoutput_and_bans_wait_substitutes(self) -> None: + """Collection uses blocking TaskOutput; wait-loop substitutes stay banned.""" + contract = _extract_section( + _spawn_reviewers_skill_text(), + "### Spawn + Collection Contract (standard flow)", + "### Agent Failure Recovery", + ) + assert "Call `TaskOutput` (block: true) for every spawned background agent" in contract + for banned in ("watcher files", "sleep loops", "polling loops"): + assert banned in contract.lower() - def test_retry_uses_same_mode_specific_dispatch_rule(self) -> None: - """GitHub recovery attempts must be synchronous too, so a retry - cannot recreate the original outstanding-background failure. - """ + def test_retry_uses_background_taskoutput_contract(self) -> None: + """Recovery re-spawns background + collects with blocking TaskOutput.""" recovery_section = _extract_section( _spawn_reviewers_skill_text(), "### Agent Failure Recovery", "**Coverage materialization (machine-readable contract).**", ) - assert "Retry uses the same mode branch" in recovery_section - assert "GitHub retries are synchronous" in recovery_section - assert "local retries may use the local background-plus-`TaskOutput`" in recovery_section + assert "background-plus-`TaskOutput`" in recovery_section + assert "run_in_background: true" in recovery_section - def test_skill_intro_describes_mode_specific_task_scheduling(self) -> None: - """The skill introduction must not call stage-20 spawning - mode-agnostic after FEA-2162 split scheduling by mode. + def test_skill_intro_describes_unified_parallel_scheduling(self) -> None: + """The intro must describe the same parallel scheduling for both + modes — not mode-specific synchronous GitHub dispatch. """ - intro = _extract_section( - _spawn_reviewers_skill_text(), - "# Reviewer Fleet Dispatch (stage_20_spawn_reviewers)", - "## Reviewer Fleet (stage_20_spawn_reviewers)", - ) - _assert_mode_specific_prompt_contract(intro) - - mutated = intro.replace( - "with mode-specific Task scheduling", - "fleet spawning is mode-agnostic", - ) - with pytest.raises(AssertionError): - _assert_mode_specific_prompt_contract(mutated) + skill = _spawn_reviewers_skill_text() + assert "use the same Task scheduling" in skill + assert "parallel background dispatch collected with blocking `TaskOutput`" in skill + assert "dispatches reviewers synchronously" not in skill.lower() - def test_headless_warning_describes_github_sync_not_standard_taskoutput_collection(self) -> None: - """The headless warning must keep GitHub standard flow tied to - synchronous Task calls, not blocking TaskOutput collection. + def test_headless_note_reflects_v2_1_182_await(self) -> None: + """The headless note must reflect the v2.1.182+ background-await + semantics rather than the old 'backgrounding dies' rule. """ - warning = _extract_from( - _spawn_reviewers_skill_text(), - "**Headless mode warning", - ) - _assert_headless_warning_mode_specific(warning) + note = _extract_from(_spawn_reviewers_skill_text(), "**Headless mode note") + _assert_headless_note_awaits_background(note) - mutated = warning.replace( - "GitHub standard-flow synchronous reviewer Task calls", - "the blocking `TaskOutput` collection (standard flow)", - ) + mutated = note.replace("v2.1.182", "v-removed") with pytest.raises(AssertionError): - _assert_headless_warning_mode_specific(mutated) + _assert_headless_note_awaits_background(mutated) + + def test_verify_findings_dispatches_parallel_in_both_modes(self) -> None: + """The verifier fleet must also dispatch in parallel in both modes, + so the orchestrator does not serialize verifiers in GitHub CI. + """ + verify = _verify_findings_skill_text() + assert "parallel background tasks in both `MODE=local` and `MODE=github`" in verify + assert "run_in_background: true" in verify + assert "Do NOT dispatch verifiers one-at-a-time" in verify def test_stage_20b_still_precedes_collect_findings(self, tmp_path: Path) -> None: """Missing required reviewers must materialize before @@ -17868,47 +17843,37 @@ def test_stage_20b_still_precedes_collect_findings(self, tmp_path: Path) -> None in mutated_stages["stage_21_collect_findings"]["depends_on"] ) - def test_start_md_stage_20_walker_guard_requires_github_sync_completion_before_stage_21(self) -> None: - """The orchestration spine must pin the GitHub-mode stage-20 guard. - - The extracted spawn-reviewers skill owns detailed dispatch, but - start.md is still the walker surface that decides whether stage 20 - may finish and advance to stage 21. + def test_start_md_stage_20_guard_requires_taskoutput_collection_before_stage_21(self) -> None: + """The orchestration spine must pin the stage-20 guard: collect the + background fleet via blocking TaskOutput before stage 21. """ stage_note = _extract_section( _start_command_text(), "- **stage_20_spawn_reviewers**:", "- **stage_20b_verify_spawn**", ) - _assert_start_md_github_stage_20_guard(stage_note) + _assert_start_md_stage_20_parallel_guard(stage_note) - mutated = stage_note.replace("or fail before `stage_21_collect_findings`", "or continue later") + mutated = stage_note.replace( + "before proceeding to `stage_21_collect_findings`", "whenever convenient", + ) with pytest.raises(AssertionError): - _assert_start_md_github_stage_20_guard(mutated) + _assert_start_md_stage_20_parallel_guard(mutated) - def test_start_md_reviewer_fleet_bans_github_watcher_sleep_and_turn_end_waits(self) -> None: - """start.md must not leave room for headless wait-loop substitutes.""" + def test_start_md_reviewer_fleet_bans_wait_loop_substitutes(self) -> None: + """start.md must still forbid headless wait-loop substitutes even + though dispatch is now parallel background + TaskOutput. + """ reviewer_fleet = _extract_section( _start_command_text(), "## Reviewer Fleet (stage_20_spawn_reviewers)", "## Verifier Fleet (stage_23_verify_findings)", ) - _assert_start_md_github_stage_20_guard(reviewer_fleet) - _assert_mode_specific_prompt_contract(reviewer_fleet) - - for forbidden_replacement in ( - "Watcher files may replace synchronous reviewer completion.", - "Sleep loops may replace synchronous reviewer completion.", - "End the assistant turn and continue when notified.", - "Use background `TaskOutput` waits in GitHub mode.", - ): - mutated = re.sub( - r"Watcher files, sleep loops, polling loops, background `TaskOutput` waits,[^\n]+", - forbidden_replacement, - reviewer_fleet, - ) - with pytest.raises(AssertionError): - _assert_start_md_github_stage_20_guard(mutated) + lowered = reviewer_fleet.lower() + assert "`taskoutput`" in lowered + assert "background" in lowered + for banned in ("watcher files", "sleep loops", "polling loops"): + assert banned in lowered def _spawn_reviewers_skill_text() -> str: @@ -17926,6 +17891,16 @@ def _start_command_text() -> str: return (Path(__file__).parents[2] / "commands" / "start.md").read_text() +def _verify_findings_skill_text() -> str: + """Read the verifier-fleet dispatch contract under test.""" + return ( + Path(__file__).parents[2] + / "skills" + / "verify-findings" + / "SKILL.md" + ).read_text() + + def _extract_section(text: str, start: str, end: str) -> str: """Return a bounded prompt section so tests pin narrow contracts.""" start_index = text.index(start) @@ -17939,51 +17914,42 @@ def _extract_from(text: str, start: str) -> str: return text[start_index:] -def _assert_github_standard_flow_sync(section: str) -> None: - """Assert the GitHub-mode standard-flow source contract is sync-only.""" - assert "one standard-flow reviewer at a time" in section - assert "wait for its Task response before spawning the next descriptor" in section - assert "run_in_background: false" in section - assert "never set it to `true`" in section - assert "Do not use `TaskOutput`, watcher files, sleep loops, polling loops" in section - assert "there must be no reviewer task still running" in section +def _assert_standard_flow_parallel(section: str) -> None: + """Assert the standard-flow contract is parallel background dispatch + collected with a blocking ``TaskOutput`` (same for both modes).""" + lowered = section.lower() + assert "run_in_background: true" in section + assert "block: true" in section + assert "`taskoutput`" in lowered + assert "parallel background tasks" in lowered + # Wait-loop substitutes stay banned even though dispatch is now parallel. + assert "watcher files" in lowered + assert "sleep loops" in lowered + assert "polling loops" in lowered -def _assert_start_md_github_stage_20_guard(section: str) -> None: - """Assert start.md pins the GitHub-mode walker guard at stage 20.""" +def _assert_start_md_stage_20_parallel_guard(section: str) -> None: + """Assert start.md pins the stage-20 guard: blocking ``TaskOutput`` + collection of the background fleet before ``stage_21``.""" lowered = section.lower() - assert "MODE=github" in section - assert "synchronous" in section assert "`taskoutput`" in lowered + assert "background" in lowered assert "watcher" in lowered assert "sleep loop" in lowered assert "polling loop" in lowered - assert "turn-ending waits" in section or "end the assistant turn" in section - assert "no reviewer task" in section or "reviewer remains outstanding" in section - assert "fail before `stage_21_collect_findings`" in section or "must not proceed to `stage_21_collect_findings`" in section - - -def _assert_mode_specific_prompt_contract(section: str) -> None: - """Assert prompt prose describes mode-aware, not mode-agnostic, dispatch.""" - lowered = section.lower() - assert "mode-specific" in lowered - assert "github" in lowered - assert "synchronous" in lowered - assert "local" in lowered - assert "blocking" in lowered - assert "mode-agnostic" not in lowered + assert "stage_21_collect_findings" in section + assert "before proceeding to `stage_21_collect_findings`" in section -def _assert_headless_warning_mode_specific(section: str) -> None: - """Assert headless warning keeps GitHub standard flow synchronous.""" +def _assert_headless_note_awaits_background(section: str) -> None: + """Assert the headless note reflects v2.1.182+ background-subagent await.""" lowered = section.lower() - assert "github standard-flow synchronous reviewer task calls" in lowered - assert "github fast-path synchronous spawn" in lowered - assert "local-mode blocking `taskoutput` collection" in lowered + assert "v2.1.182" in section + assert "background subagent" in lowered + assert "`taskoutput`" in lowered assert "watcher files" in lowered assert "sleep loops" in lowered assert "polling loops" in lowered - assert "blocking `taskoutput` collection (standard flow)" not in lowered def _seed_phase9_inputs(