diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b47c01..ddd0f67 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.6.0 + +#### Fixed +- **Reverted the v3.5.0 parallel GitHub-mode reviewer fleet (#178); restored synchronous headless dispatch at `stage_20_spawn_reviewers`.** v3.5.0 unified both modes on `run_in_background: true` + blocking `TaskOutput`, betting that headless `claude -p` (Claude Code ≥ v2.1.182) always awaits background subagents as long as the orchestrator issues the blocking collection. In practice the orchestrator does not reliably issue it: in ~18% of `claude-code-review.yml` `pull_request` runs it launched the fleet (Bug Hunter A/B, Unified Auditor) as background agents and then ended its turn ("I'll wait for the harness to notify me") with no pending synchronous tool call, so `claude -p` exited (`terminal_reason: "completed"`) before `stage_21_collect_findings … stage_30_footer` ran — no findings collected, no `.closedloop-ai/code-review-*` artifacts written, and the workflow's artifact-validation guard failed the job. GitHub/headless standard flow again dispatches reviewers **synchronously** one-at-a-time (`run_in_background: false`, wait for each `DONE findings=N file=...` before the next), which keeps the turn alive by construction; local mode keeps parallel background dispatch + blocking `TaskOutput`. The `stage_23` verifier fleet is unchanged (background + blocking `TaskOutput` in both modes, as it was before v3.5.0); its exposure to the same headless turn-ending is tracked separately (FEA-3085 follow-up). Trade-off: GitHub review wall-clock returns to sum-of-reviewers — correctness over speed for a merge-adjacent gate. Restores the FEA-2162 synchronous-GitHub reviewer contract and its test class. + ### code-review v3.5.0 #### Changed diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index dbb922c..7d2c66e 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.5.0", + "version": "3.6.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 243f8d2..fc27177 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 **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_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_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` 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. +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. -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. +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. 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 36f9cfc..65e3a2e 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`, 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). +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. 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,17 +325,19 @@ When GRAPH_PROJECT is empty, grep imports instead. ### Spawn + Collection Contract (standard flow) -**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. +**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. -> **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). +**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. **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. -**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: +**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: 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. @@ -347,7 +349,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 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. +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. 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. @@ -362,7 +364,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; there is only one agent, so backgrounding buys no parallelism, see "Fast-Path Spawn + Collection" below) +- `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) - `AGENT_ID`: `"fast"` - ``: `{CR_DIR}/agent_fast.json` - ``: `{CR_DIR}/patches_all.txt` @@ -447,9 +449,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 (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. +- 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). - `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 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.) +**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." diff --git a/plugins/code-review/skills/verify-findings/SKILL.md b/plugins/code-review/skills/verify-findings/SKILL.md index 4b46a9d..100dd88 100644 --- a/plugins/code-review/skills/verify-findings/SKILL.md +++ b/plugins/code-review/skills/verify-findings/SKILL.md @@ -30,10 +30,6 @@ 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 43c6233..7e99d39 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -17728,101 +17728,126 @@ def test_collect_findings_depends_on_verify_spawn( assert "stage_20b_verify_spawn" in collect["depends_on"] -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. +class TestFEA2162SpawnReviewerContracts: + """FEA-2162 hardens GitHub-mode reviewer dispatch while preserving + local interactive parallelism. """ - 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. + def test_github_standard_flow_requires_synchronous_dispatch(self) -> None: + """GitHub headless standard flow must not leave background Tasks + outstanding when stage_20 completes. """ - contract = _extract_section( + 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( _spawn_reviewers_skill_text(), - "### Spawn + Collection Contract (standard flow)", - "### Agent Failure Recovery", + "**GitHub mode (`MODE=github`): dispatch synchronously.**", + "**Local mode (`MODE=local`): spawn ALL agents at once.**", ) - _assert_standard_flow_parallel(contract) - assert "Both `MODE=local` and `MODE=github`" in contract - assert "slowest** reviewer, not the sum" in contract + _assert_github_standard_flow_sync(github_section) - # 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) + 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) - 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. + def test_local_standard_flow_preserves_background_collection(self) -> None: + """Local mode keeps parallel reviewer dispatch and blocking + TaskOutput collection. """ skill = _spawn_reviewers_skill_text() - 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 - - 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", + 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 "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() + 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 - def test_retry_uses_background_taskoutput_contract(self) -> None: - """Recovery re-spawns background + collects with blocking TaskOutput.""" + 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_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. + """ recovery_section = _extract_section( _spawn_reviewers_skill_text(), "### Agent Failure Recovery", "**Coverage materialization (machine-readable contract).**", ) - assert "background-plus-`TaskOutput`" in recovery_section - assert "run_in_background: true" in recovery_section - - 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. - """ - 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() + 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 - 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. + 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. """ - note = _extract_from(_spawn_reviewers_skill_text(), "**Headless mode note") - _assert_headless_note_awaits_background(note) + 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 = note.replace("v2.1.182", "v-removed") + mutated = intro.replace( + "with mode-specific Task scheduling", + "fleet spawning is mode-agnostic", + ) with pytest.raises(AssertionError): - _assert_headless_note_awaits_background(mutated) + _assert_mode_specific_prompt_contract(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. + 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. """ - 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 + warning = _extract_from( + _spawn_reviewers_skill_text(), + "**Headless mode warning", + ) + _assert_headless_warning_mode_specific(warning) + + mutated = warning.replace( + "GitHub standard-flow synchronous reviewer Task calls", + "the blocking `TaskOutput` collection (standard flow)", + ) + with pytest.raises(AssertionError): + _assert_headless_warning_mode_specific(mutated) def test_stage_20b_still_precedes_collect_findings(self, tmp_path: Path) -> None: """Missing required reviewers must materialize before @@ -17843,37 +17868,47 @@ 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_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. + 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. """ stage_note = _extract_section( _start_command_text(), "- **stage_20_spawn_reviewers**:", "- **stage_20b_verify_spawn**", ) - _assert_start_md_stage_20_parallel_guard(stage_note) + _assert_start_md_github_stage_20_guard(stage_note) - mutated = stage_note.replace( - "before proceeding to `stage_21_collect_findings`", "whenever convenient", - ) + mutated = stage_note.replace("or fail before `stage_21_collect_findings`", "or continue later") with pytest.raises(AssertionError): - _assert_start_md_stage_20_parallel_guard(mutated) + _assert_start_md_github_stage_20_guard(mutated) - 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. - """ + 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.""" reviewer_fleet = _extract_section( _start_command_text(), "## Reviewer Fleet (stage_20_spawn_reviewers)", "## Verifier Fleet (stage_23_verify_findings)", ) - 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 + _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) def _spawn_reviewers_skill_text() -> str: @@ -17891,16 +17926,6 @@ 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) @@ -17914,42 +17939,51 @@ def _extract_from(text: str, start: str) -> str: return text[start_index:] -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_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_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``.""" +def _assert_start_md_github_stage_20_guard(section: str) -> None: + """Assert start.md pins the GitHub-mode walker guard at stage 20.""" 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 "stage_21_collect_findings" in section - assert "before proceeding to `stage_21_collect_findings`" in section + 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_headless_note_awaits_background(section: str) -> None: - """Assert the headless note reflects v2.1.182+ background-subagent await.""" +def _assert_mode_specific_prompt_contract(section: str) -> None: + """Assert prompt prose describes mode-aware, not mode-agnostic, dispatch.""" lowered = section.lower() - assert "v2.1.182" in section - assert "background subagent" in lowered - assert "`taskoutput`" in lowered + 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 + + +def _assert_headless_warning_mode_specific(section: str) -> None: + """Assert headless warning keeps GitHub standard flow synchronous.""" + 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 "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(