fix(code-review): revert #178 parallel GitHub fleet → restore synchronous headless dispatch (FEA-3085)

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

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"

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 `<CR_DIR>/cache_result.json` (stats), `<CR_DIR>/agent_cached_bha.json` (cached BHA findings, glob-compatible with `agent_*`), `<CR_DIR>/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 `<CR_DIR>/coverage.json` (`final` section — post-arbitrate), `<CR_DIR>/partitions.json`, and `<CR_DIR>/spawn.json` (`route` section, written by Gate B's `cmd_route --cr-dir`) and writes `<CR_DIR>/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 `<CR_DIR>/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 `<CR_DIR>/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 `<CR_DIR>/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 `<CR_DIR>/spawn.json` (`spec` section) and globs `<CR_DIR>/agent_*.json`; for every descriptor with `bucket: "required"` that has no on-disk output, appends a coverage-gap finding to `<CR_DIR>/coverage_gaps.json` (reason `spawn_missing_required_agent`) and records the omission in `<CR_DIR>/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 `<CR_DIR>/findings_validated.json` via `> <CR_DIR>/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) `<CR_DIR>/verify_manifest.json` with `to_verify[]` + `skipped_no_verification[]` + `deferred_budget[]` + `cache_hits[]`, and (b) `<CR_DIR>/verifier_inputs/<finding_id>.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_<finding_id>.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`.