From 7ad4427a2c094fc8bcd7f83ae6107ef6c5135b9d Mon Sep 17 00:00:00 2001 From: Kris Wong Date: Fri, 10 Jul 2026 09:06:12 -0500 Subject: [PATCH 1/2] feat(code-review): parallelize GitHub-mode reviewer/verifier fleets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub/headless reviews dispatched the reviewer fleet (stage_20) and verifier fleet (stage_23) synchronously one agent at a time, so CI wall-clock was the SUM of every agent — a shallow review with 3 serial reviewers + 2 serial verifiers took ~30 min. The synchronous rule existed because headless `claude -p` used to terminate before background subagents finished. As of Claude Code v2.1.182+ (shipped by claude-code-action@v1), `claude -p` waits for background subagents to complete because their output is part of the final result, so a blocking TaskOutput keeps the turn alive until reviewers finish. Unify both modes on the local parallel pattern: spawn every standard-flow reviewer/verifier with run_in_background: true and collect all with blocking TaskOutput before advancing. Fleet stages now take the SLOWEST agent, not the sum — roughly halving shallow-tier CI review time. A synchronous one-at-a-time fallback stays documented for Claude Code < v2.1.182, and CLAUDE_CODE_PRINT_BG_WAIT_CEILING_MS can raise the 10-min per-agent wait cap for long reviewers. Updates spawn-reviewers + verify-findings skills and start.md; replaces the FEA-2162 synchronous-GitHub test class with the parallel-dispatch contract. Full code-review suite green (1308 passed), ruff + pyright clean. Bump 3.4.1 -> 3.5.0. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 5 + .../code-review/.claude-plugin/plugin.json | 2 +- plugins/code-review/commands/start.md | 6 +- .../skills/spawn-reviewers/SKILL.md | 22 +- .../skills/verify-findings/SKILL.md | 2 + .../tools/python/test_code_review_helpers.py | 280 ++++++++---------- 6 files changed, 144 insertions(+), 173 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99c017c..b5c51e2 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`) and verifier fleet (`stage_23`) as parallel background tasks collected with a blocking `TaskOutput`, the same as local mode, instead of running each agent synchronously one at a time. Headless `claude -p` waits for background subagents to finish before exiting (Claude Code v2.1.182+), so a code-review CI run's fleet stages now take the wall-clock of the slowest agent rather than the sum of all agents — roughly halving shallow-tier CI review time. A synchronous one-at-a-time fallback stays documented for Claude Code older than v2.1.182, 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..805565e 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; use the skill's legacy synchronous fallback there. 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..b1f8d8e 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).** On older CLIs headless does not await background subagents, so a backgrounded reviewer dies when the turn ends. There — and only there — 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. This is the pre-v2.1.182 behavior; it 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..e14f548 100644 --- a/plugins/code-review/skills/verify-findings/SKILL.md +++ b/plugins/code-review/skills/verify-findings/SKILL.md @@ -30,6 +30,8 @@ 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). + 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( From a9d6e5386b9eba40b1633f5ecd16933295e8e5f0 Mon Sep 17 00:00:00 2001 From: Kris Wong Date: Fri, 10 Jul 2026 10:12:57 -0500 Subject: [PATCH 2/2] docs(code-review): reframe pre-v2.1.182 fleet fallback as operator concern Address deep-review findings on the parallel-fleet contract: - The "Legacy fallback (Claude Code < v2.1.182)" branch was written as if the orchestrating agent picks between parallel and synchronous dispatch at runtime, but the agent cannot introspect its own CLI version from inside the prompt, so the fallback was an unreachable branch. Reframe it in spawn-reviewers/SKILL.md and start.md as an operator/environment concern (claude-code-action@v1 ships >= v2.1.182, so CI is always parallel; an operator on an older CLI must edit the skill), not a path the live agent resolves. - Add the matching legacy-fallback note to verify-findings/SKILL.md so the reviewer and verifier fleets document the same degradation on older CLIs. - Scope the CHANGELOG entry: only stage_20 removed a synchronous-GitHub branch; stage_23 was already background-dispatched in both modes and is now only documented explicitly. Drop the inflated "halving" estimate. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 2 +- plugins/code-review/commands/start.md | 2 +- plugins/code-review/skills/spawn-reviewers/SKILL.md | 2 +- plugins/code-review/skills/verify-findings/SKILL.md | 2 ++ 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5c51e2..2b47c01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### code-review v3.5.0 #### Changed -- GitHub-mode reviews now dispatch the reviewer fleet (`stage_20`) and verifier fleet (`stage_23`) as parallel background tasks collected with a blocking `TaskOutput`, the same as local mode, instead of running each agent synchronously one at a time. Headless `claude -p` waits for background subagents to finish before exiting (Claude Code v2.1.182+), so a code-review CI run's fleet stages now take the wall-clock of the slowest agent rather than the sum of all agents — roughly halving shallow-tier CI review time. A synchronous one-at-a-time fallback stays documented for Claude Code older than v2.1.182, 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. +- 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 diff --git a/plugins/code-review/commands/start.md b/plugins/code-review/commands/start.md index 805565e..243f8d2 100644 --- a/plugins/code-review/commands/start.md +++ b/plugins/code-review/commands/start.md @@ -409,7 +409,7 @@ When the walker reaches `stage_20_spawn_reviewers`, invoke the `code-review:spaw 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 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; use the skill's legacy synchronous fallback there. +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 b1f8d8e..36f9cfc 100644 --- a/plugins/code-review/skills/spawn-reviewers/SKILL.md +++ b/plugins/code-review/skills/spawn-reviewers/SKILL.md @@ -329,7 +329,7 @@ When GRAPH_PROJECT is empty, grep imports instead. > **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).** On older CLIs headless does not await background subagents, so a backgrounded reviewer dies when the turn ends. There — and only there — 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. This is the pre-v2.1.182 behavior; it is correct but serial (wall-clock = sum of reviewers). +> **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. diff --git a/plugins/code-review/skills/verify-findings/SKILL.md b/plugins/code-review/skills/verify-findings/SKILL.md index e14f548..4b46a9d 100644 --- a/plugins/code-review/skills/verify-findings/SKILL.md +++ b/plugins/code-review/skills/verify-findings/SKILL.md @@ -32,6 +32,8 @@ This stage runs when the walker reaches `stage_23`. It implements PLN-722's find **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.