From 9664723befae87b66fae965729969a4e7842f4e6 Mon Sep 17 00:00:00 2001 From: Kris Wong Date: Thu, 2 Jul 2026 11:35:26 -0500 Subject: [PATCH 1/3] feat(code-review): fold Gate B route + partition into run-prefix (PLN-1229 Phase 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend cmd_run_prefix to run the WHOLE deterministic prefix in one process, through Gate B route + stage_17_partition + derive-spawn-spec, stopping at the reviewer fleet. - After stage_19_cache_check the runner invokes `route` itself (writing spawn.json.route), caches fast_path / max_bha_agents on the context, and on fast-path deletes the cached-BHA replay artifact. - stage_17_partition is skipped entirely in fast-path (no partitions.json / patches_p.txt); otherwise its args are augmented with --loc-budget 500 --max-files 25 --max-bha-agents , swapping --diff-data to uncached_diff_data.json when a cache dir is active. - The terminal return is now `ready_for_reviewers`, carrying fast_path, max_bha_agents, and cache_status_message so the orchestrator prints the routing + cache notices without re-reading spawn.json. A route failure surfaces as error with failed_stage="route". Parity oracle: the subprocess A-side gains a subprocess `route` runner and the same partition skip/augmentation, and both sides now run through to the reviewer fleet — so the A/B byte-equality test covers the full prefix (incl. partitions.json / spawn.json) across the fast-path and partitioned branches on all seven fixtures. Adds route/partition contract tests; SCHEMA.md §7b documents the ready_for_reviewers result and its fast_path / max_bha_agents / cache_status_message fields. Zero orchestrator changes (start.md untouched). Full code-review suite 1305 passed / 3 skipped; ruff + pyright clean. Co-Authored-By: Claude Opus 4.8 --- .../code-review/.claude-plugin/plugin.json | 2 +- plugins/code-review/SCHEMA.md | 77 ++++---- .../tools/python/code_review_helpers.py | 176 ++++++++++++++---- .../tools/python/prefix_golden_harness.py | 56 ++++-- .../tools/python/test_code_review_helpers.py | 87 ++++++++- .../tools/python/test_prefix_golden.py | 44 +++-- 6 files changed, 338 insertions(+), 104 deletions(-) diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index 3c771b0..53c551a 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.2.0", + "version": "3.3.0", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code-review/SCHEMA.md b/plugins/code-review/SCHEMA.md index 135200b..0dd2b53 100644 --- a/plugins/code-review/SCHEMA.md +++ b/plugins/code-review/SCHEMA.md @@ -495,51 +495,56 @@ Stages from plans 01/03/05/06 are present in `run_plan.json` but marked ## 7b. `run-prefix` result contract (PLN-1229) -`run-prefix` runs the deterministic prefix (stages 01→`cache_check`) in ONE -process instead of one orchestrator turn per stage. It reads `run_plan.json` + -`setup.json` from `--cr-dir`, walks from `--resume-from` (default: the first -plan stage), and stops at the next genuine decision point — emitting a status -JSON (to stdout, or `--output `) that tells the orchestrator what to do -next. The runner is **resumable**: after handling a pause the orchestrator -re-invokes `run-prefix --resume-from `. Because each segment is a -fresh process, the `depends_on` `completed` set is reconstructed from artifacts -on disk (a prior stage counts as done iff its literal `expected_outputs` exist). +`run-prefix` runs the **entire** deterministic prefix (stages 01 through Gate B +`route` + `partition` + `derive-spawn-spec`) in ONE process instead of one +orchestrator turn per stage. It reads `run_plan.json` + `setup.json` from +`--cr-dir`, walks from `--resume-from` (default: the first plan stage), and stops +at the next genuine decision point — emitting a status JSON (to stdout, or +`--output `) that tells the orchestrator what to do next. The runner is +**resumable**: after handling a pause the orchestrator re-invokes +`run-prefix --resume-from `. Because each segment is a fresh +process, the `depends_on` `completed` set is reconstructed from artifacts on disk +(a prior stage counts as done iff its literal `expected_outputs` exist). **Result fields:** -| Field | Type | Meaning | -| --------------- | --------------- | ----------------------------------------------------------------------- | -| `next_action` | string (enum) | The pause reason — authoritative (read this, not the exit code). | -| `resume_stage` | string \| null | The stage id to pass as `--resume-from` on the next invocation. | -| `singleton` | string \| null | `"extract_signals"` \| `"coverage_critic"` when `needs_singleton`. | -| `failed_stage` | string \| null | The aborting stage id when `next_action == "error"`. | -| `ran_stages` | string[] | Stage ids executed (or `continue`-failed) this segment, in order. | -| `message` | string \| null | Short diagnostic on `error`, else null. | +| Field | Type | Meaning | +| ---------------------- | --------------- | ----------------------------------------------------------------------- | +| `next_action` | string (enum) | The pause reason — authoritative (read this, not the exit code). | +| `resume_stage` | string \| null | The stage id to pass as `--resume-from` on the next invocation. | +| `singleton` | string \| null | `"extract_signals"` \| `"coverage_critic"` when `needs_singleton`. | +| `failed_stage` | string \| null | The aborting stage id when `next_action == "error"`. | +| `ran_stages` | string[] | Stage ids executed (or `continue`-failed) this segment, in order. | +| `message` | string \| null | Short diagnostic on `error`, else null. | +| `fast_path` | bool | Gate B routing decision (on `ready_for_reviewers`). | +| `max_bha_agents` | int \| null | Gate B Bug-Hunter-A agent cap (on `ready_for_reviewers`). | +| `cache_status_message` | string \| null | `cache_result.json.status_message` to print (Gate A/B), else null. | **`next_action` values:** -| Value | Fires at | Orchestrator does next | -| ------------------- | ------------------------------------- | ---------------------------------------------------------------------------- | -| `needs_singleton` | `stage_11` / `stage_15` `needs_agent` | Spawn the `singleton` agent, write its output, re-invoke from `resume_stage`. | -| `hygiene_exit` | Gate A (`hygiene_only` after hygiene) | Present hygiene findings and stop (no verdict/footer). | -| `ready_for_route` | reaching `stage_17_partition` | Run Gate B (`route`) + partition + the rest of the walk. | -| `error` | a stage aborted / a gate failed | Fall back to the per-stage walk from `failed_stage`; partials are preserved. | - -`ready_for_route` distinguishes its two cases by `resume_stage`: a non-null -`resume_stage` (`stage_17_partition`) is the normal boundary — run Gate B + -partition from there. A **null** `resume_stage` means the walk reached the end -of the plan without a partition stage (e.g. a depth tier that filters partition -out); there is nothing left to route, so the orchestrator skips Gate B and -partition and proceeds directly to the reviewer fleet. +| Value | Fires at | Orchestrator does next | +| --------------------- | ------------------------------------- | ---------------------------------------------------------------------------- | +| `needs_singleton` | `stage_11` / `stage_15` `needs_agent` | Spawn the `singleton` agent, write its output, re-invoke from `resume_stage`. | +| `hygiene_exit` | Gate A (`hygiene_only` after hygiene) | Print `cache_status_message`, present hygiene findings, stop (no verdict). | +| `ready_for_reviewers` | the whole deterministic prefix is done | Print `cache_status_message` + the `fast_path` notice; spawn the reviewer fleet (`stage_20`). | +| `error` | a stage aborted / a gate failed | Fall back to the per-stage walk from `failed_stage`; partials are preserved. | + +On `ready_for_reviewers` the runner has already run Gate B `route` (writing +`spawn.json.route`) and — unless `fast_path` — `stage_17_partition` (with the +`--loc-budget 500 --max-files 25 --max-bha-agents ` augmentation, and the +`uncached_diff_data.json` swap when a cache dir is active). In `fast_path` mode +partition is skipped (no `partitions.json` / `patches_p.txt`) and any cached +BHA replay artifact is deleted. The `fast_path` / `max_bha_agents` / +`cache_status_message` fields let the orchestrator print the routing + cache +notices without re-reading `spawn.json`. The exit code is `0` for every well-formed result (including `error`) — the -`next_action` field is the contract. Route + partition (Segment 3) fold into the -runner in Phase 2, at which point `ready_for_route` becomes `ready_for_reviewers` -(carrying `fast_path` + `cache_status_message`). The `on_failure` policy of each -stage is honored exactly as the Walker Contract prescribes: `abort` → `error`; -`continue` → proceed; `continue_with_coverage_gap` → proceed after writing an +`next_action` field is the contract. The `on_failure` policy of each stage is +honored exactly as the Walker Contract prescribes: `abort` → `error`; `continue` +→ proceed; `continue_with_coverage_gap` → proceed after writing an `agent-failure` system finding to `agent_-failed.json` (collected by -`collect-findings`). +`collect-findings`). A `route` failure is surfaced as `error` with +`failed_stage: "route"`. --- diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index 29d4087..12511ae 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -10332,9 +10332,13 @@ def cmd_evaluate_gate(args: argparse.Namespace) -> int: # manifest is ``needs_agent`` → pause so the orchestrator can # spawn the one synchronous agent, then resume at the sibling # consolidate stage. -# * Gate B — reaching ``stage_17_partition``. Phase 1 stops here and -# returns ``ready_for_route``; folding route + partition into -# the runner (→ ``ready_for_reviewers``) is Phase 2. +# * Gate B — after ``stage_19_cache_check`` the runner invokes ``route`` +# (model routing) itself, then runs ``stage_17_partition`` +# (skipped in fast-path) and ``derive-spawn-spec`` before +# stopping at the reviewer fleet — returning +# ``ready_for_reviewers`` with ``fast_path`` + +# ``cache_status_message`` so the orchestrator prints them +# without re-reading ``spawn.json``. # # The runner is RESUMABLE: after handling a pause the orchestrator re-invokes # ``run-prefix --resume-from ``. Because each segment is a fresh process, @@ -10354,7 +10358,9 @@ def cmd_evaluate_gate(args: argparse.Namespace) -> int: _RP_SETUP_STAGE = "stage_01_setup" _RP_AUTO_INCREMENTAL_STAGE = "stage_07_auto_incremental" _RP_HYGIENE_STAGE = "stage_12_hygiene" -_RP_PARTITION_STAGE = "stage_17_partition" # Phase-1 terminal boundary +_RP_CACHE_CHECK_STAGE = "stage_19_cache_check" # Gate B (route) fires after this +_RP_PARTITION_STAGE = "stage_17_partition" # skipped in fast-path; else augmented +_RP_REVIEWER_FLEET_STAGE = "stage_20_spawn_reviewers" # terminal boundary (LLM fleet) # The two PLN-725 singleton *prepare* stages: the manifest each writes and the # path (within that manifest JSON) to the ``status`` field the walker reads to @@ -10393,6 +10399,9 @@ class _RunPrefixContext: #: Runtime token overrides keyed by literal token string. Currently only #: ```` is overridden (by stage_07 auto-incremental narrowing). overrides: dict[str, str] = field(default_factory=dict) + #: Gate B route decision, populated after stage_19_cache_check runs ``route``. + fast_path: bool = False + max_bha_agents: int | None = None def _rp_read_json(path: Path) -> dict[str, Any]: @@ -10649,6 +10658,11 @@ def _execute_stage_inprocess( # so dependents resolve. completed.add(stage_id) return "ran", None + if stage_id == _RP_PARTITION_STAGE and ctx.fast_path: + # Gate B fast-path (start.md Gate B step 5): partition is skipped + # entirely — the single fast-path reviewer consumes patches_all.txt + # directly, so no partitions.json / patches_p.txt are produced. + return "skipped_fast_path", None kind = stage.get("kind") if kind != "helper": @@ -10668,6 +10682,8 @@ def _execute_stage_inprocess( try: with contextlib.redirect_stderr(err_buf): resolved = _rp_resolve_args(stage.get("args", []) or [], ctx) + if stage_id == _RP_PARTITION_STAGE: + resolved = _rp_augment_partition_args(resolved, ctx) ns = parser.parse_args([stage["subcommand"], *resolved]) stdout_target = stage.get("stdout") rc = _rp_dispatch( @@ -10766,10 +10782,88 @@ def _rp_evaluate_gates( return True, None +def _rp_augment_partition_args( + resolved: list[str], ctx: _RunPrefixContext, +) -> list[str]: + """Apply Gate B's partition augmentation (start.md Gate B step 4). + + For the standard (non-fast-path) flow the walker passes + ``--loc-budget 500 --max-files 25 --max-bha-agents `` on top of the plan + args, and — when a cache dir is active — swaps ``--diff-data`` to + ``uncached_diff_data.json`` so partitions only contain files that missed the + cache. + """ + out = list(resolved) + cache_dir = _rp_resolve_token("", ctx) + uncached = ctx.cr_dir / "uncached_diff_data.json" + if cache_dir and uncached.exists(): + out = [ + str(uncached) if a.endswith("/diff_data.json") else a + for a in out + ] + out += ["--loc-budget", "500", "--max-files", "25"] + if ctx.max_bha_agents is not None: + out += ["--max-bha-agents", str(ctx.max_bha_agents)] + return out + + +def _rp_run_route(ctx: _RunPrefixContext, parser: argparse.ArgumentParser) -> int: + """Run Gate B ``route`` after ``stage_19_cache_check`` (start.md Gate B). + + ``route`` is not a canonical plan stage — the walker invokes it between + cache-check and partition to compute ``fast_path`` / ``max_bha_agents`` and + write the ``route`` section into ``spawn.json``. Caches the decision on + ``ctx``; on fast-path, deletes any cached-BHA replay artifact (the fast-path + reviewer bypasses the BHA cache). Returns the ``route`` return code. + """ + cr = ctx.cr_dir + intent = _rp_resolve_token("", ctx) + err_buf = io.StringIO() + try: + with contextlib.redirect_stderr(err_buf): + ns = parser.parse_args([ + "route", + "--diff-data", str(cr / "diff_data.json"), + "--critic-gates", ".closedloop-ai/settings/critic-gates.json", + "--intent", intent or "mixed", + "--cr-dir", str(cr), + ]) + rc = _rp_dispatch(ns.func, ns, None) + except (SystemExit, Exception): # noqa: BLE001 — a route crash is a run failure + rc = 1 + finally: + captured = err_buf.getvalue() + if captured: + sys.stderr.write(captured) + if rc != 0: + return rc + + route = _rp_read_json(cr / "spawn.json").get("route", {}) + ctx.fast_path = bool(route.get("fast_path", False)) if isinstance(route, dict) else False + mba = route.get("max_bha_agents") if isinstance(route, dict) else None + ctx.max_bha_agents = int(mba) if isinstance(mba, (int, float)) else None + if ctx.fast_path: + cached_bha = cr / "agent_cached_bha.json" + if cached_bha.exists(): + cached_bha.unlink() + return 0 + + +def _rp_cache_status_message(cr_dir: Path) -> str | None: + """The ``cache_result.json`` status message the orchestrator prints, or None. + + start.md Gate A/B print ``cache_result.json.status_message`` when a cache dir + is active; returning it lets the orchestrator print it without re-reading the + artifact. + """ + msg = _rp_read_json(cr_dir / "cache_result.json").get("status_message") + return str(msg) if msg else None + + def cmd_run_prefix(args: argparse.Namespace) -> int: """Run the deterministic prefix in-process until the next pause point. - PLN-1229 Phase 1. Consumes ``run_plan.json`` + ``setup.json`` from + PLN-1229 Phase 1-2. Consumes ``run_plan.json`` + ``setup.json`` from ``--cr-dir`` (both already written by the orchestrator's stage 0) and walks stages from ``--resume-from`` (default: the first plan stage) to the next genuine decision point, emitting a small status JSON telling the orchestrator @@ -10779,15 +10873,18 @@ def cmd_run_prefix(args: argparse.Namespace) -> int: Result ``next_action`` values (authoritative — read the JSON, not the exit code, which is always 0 for a well-formed run): - * ``needs_singleton`` — spawn the ``singleton`` agent, write its output, - then re-invoke ``--resume-from ``. - * ``hygiene_exit`` — Gate A: present hygiene findings and stop. - * ``ready_for_route`` — reached ``stage_17_partition``; run Gate B (route) - + the rest of the walk. (Phase 2 folds route + - partition in and returns ``ready_for_reviewers``.) - * ``error`` — a stage aborted; ``failed_stage`` is set. The - orchestrator falls back to the per-stage walk from - there. Partial artifacts are preserved. + * ``needs_singleton`` — spawn the ``singleton`` agent, write its output, + then re-invoke ``--resume-from ``. + * ``hygiene_exit`` — Gate A: present hygiene findings and stop. + * ``ready_for_reviewers``— the whole deterministic prefix (through Gate B + route + partition + derive-spawn-spec) is done; + spawn the reviewer fleet. Carries ``fast_path``, + ``max_bha_agents``, and ``cache_status_message`` + so the orchestrator prints them without + re-reading ``spawn.json``. + * ``error`` — a stage aborted; ``failed_stage`` is set. The + orchestrator falls back to the per-stage walk + from there. Partial artifacts are preserved. """ cr_dir = Path(args.cr_dir) run_plan = _rp_read_json(cr_dir / "run_plan.json") @@ -10819,6 +10916,18 @@ def emit(result: dict[str, Any]) -> int: print(text) return 0 + def reviewers_result() -> dict[str, Any]: + return { + "next_action": "ready_for_reviewers", + "resume_stage": None, + "singleton": None, + "failed_stage": None, + "fast_path": ctx.fast_path, + "max_bha_agents": ctx.max_bha_agents, + "cache_status_message": _rp_cache_status_message(cr_dir), + "message": None, + } + ran: list[str] = [] started = not resume_from for stage in stages: @@ -10828,16 +10937,10 @@ def emit(result: dict[str, Any]) -> int: started = True else: continue - # Phase-1 terminal boundary: Gate B (route) + partition are still the - # orchestrator's job. Stop BEFORE executing partition. - if sid == _RP_PARTITION_STAGE: - return emit({ - "next_action": "ready_for_route", - "resume_stage": sid, - "singleton": None, - "failed_stage": None, - "message": None, - }) + # Terminal boundary: the reviewer fleet is the first LLM (non- + # deterministic) stage. The whole deterministic prefix is done. + if sid == _RP_REVIEWER_FLEET_STAGE: + return emit(reviewers_result()) status, message = _execute_stage_inprocess(stage, ctx, parser, completed) if status == "failed_abort": @@ -10868,9 +10971,22 @@ def emit(result: dict[str, Any]) -> int: "resume_stage": None, "singleton": None, "failed_stage": None, + "cache_status_message": _rp_cache_status_message(cr_dir), "message": None, }) + # Gate B — after cache-check, run ``route`` to compute fast_path / + # max_bha_agents (writing spawn.json.route) before the partition stage. + if sid == _RP_CACHE_CHECK_STAGE and status != "skipped": + if _rp_run_route(ctx, parser) != 0: + return emit({ + "next_action": "error", + "resume_stage": _RP_PARTITION_STAGE, + "singleton": None, + "failed_stage": "route", + "message": "Gate B route failed", + }) + # PLN-725 singleton pause — a needs_agent prepare stage yields to the # orchestrator to spawn one synchronous agent, then resume at the sibling. if status == "ran" and sid in _RP_SINGLETONS and _rp_singleton_needs_agent(ctx, sid): @@ -10882,15 +10998,9 @@ def emit(result: dict[str, Any]) -> int: "message": None, }) - # Walked to the end without reaching the partition stage (e.g. a depth tier - # that filters partition out). No route needed; signal completion. - return emit({ - "next_action": "ready_for_route", - "resume_stage": None, - "singleton": None, - "failed_stage": None, - "message": None, - }) + # Walked to the end without hitting the reviewer fleet (e.g. a depth tier + # whose plan ends earlier). The deterministic prefix is complete either way. + return emit(reviewers_result()) def cmd_prepare_run(args: argparse.Namespace) -> int: diff --git a/plugins/code-review/tools/python/prefix_golden_harness.py b/plugins/code-review/tools/python/prefix_golden_harness.py index f6af581..2070b35 100644 --- a/plugins/code-review/tools/python/prefix_golden_harness.py +++ b/plugins/code-review/tools/python/prefix_golden_harness.py @@ -917,17 +917,14 @@ def run_prefix_fixture( # # A and B share the canonical stage/gate TABLES but implement the walk WRAPPER # independently (A here, B in code_review_helpers), so a wrapper bug shared by -# both cannot hide. Both stop at the Phase-1 boundary (before stage_17_partition -# — route + partition land in Phase 2). Determinism across the two independent -# stage-0 setups is provided by the same normalization the golden test relies on. +# both cannot hide. Both run the WHOLE deterministic prefix — including Gate B +# route + partition (Phase 2) — and stop at the reviewer fleet. Determinism +# across the two independent stage-0 setups is provided by the same +# normalization the golden test relies on. # The production helpers CLI both sides shell out to. HELPERS_PATH = Path(code_review_helpers.__file__).resolve() -# The A-side stops before the partition stage: Gate B (route) + partition are -# still the orchestrator's job in Phase 1, so run-prefix (B) never runs them. -PHASE1_STOP_STAGE = _PARTITION_STAGE_ID - def _make_fake_gh(bin_dir: Path) -> Path: """Write a ``gh`` stub that always exits non-zero and return its bin dir. @@ -976,6 +973,37 @@ def _run_stage_subprocess( return proc.returncode +def _run_route_gate_b_subprocess( + ctx: PrefixContext, repo: Path, env: dict[str, str], +) -> None: + """A-side Gate B: run ``route`` as a subprocess after ``stage_19_cache_check``. + + Subprocess twin of ``_run_route_gate_b``. Computes ``fast_path`` / + ``max_bha_agents`` (written into ``spawn.json.route``) and deletes the cached + BHA replay artifact on fast-path. + """ + cr = ctx.cr_dir + intent = _resolve_token("", ctx) + _run_stage_subprocess( + "route", + [ + "--diff-data", str(cr / "diff_data.json"), + "--critic-gates", ".closedloop-ai/settings/critic-gates.json", + "--intent", intent or "mixed", + "--cr-dir", str(cr), + ], + repo, None, env, + ) + route = _read_json(cr / "spawn.json").get("route", {}) + ctx.fast_path = bool(route.get("fast_path", False)) + mba = route.get("max_bha_agents") + ctx.max_bha_agents = int(mba) if isinstance(mba, (int, float)) else None + if ctx.fast_path: + cached_bha = cr / "agent_cached_bha.json" + if cached_bha.exists(): + cached_bha.unlink() + + def _execute_stage_subprocess( stage: dict[str, Any], ctx: PrefixContext, @@ -985,8 +1013,8 @@ def _execute_stage_subprocess( ) -> StageResult: """Subprocess twin of ``_execute_stage`` — identical control flow, real exec. - No partition augmentation: the A-side stops before ``stage_17_partition``, - so that branch never runs in Phase 1. + Applies the same Gate B partition handling: fast-path skip and the + non-fast-path arg augmentation. """ stage_id = stage["id"] if not stage.get("enabled", True): @@ -996,6 +1024,8 @@ def _execute_stage_subprocess( if stage_id == _SETUP_STAGE_ID: completed.add(stage_id) return StageResult(stage_id, "ran") + if stage_id == _PARTITION_STAGE_ID and ctx.fast_path: + return StageResult(stage_id, "skipped_fast_path") kind = stage.get("kind") if kind != "helper": @@ -1004,6 +1034,8 @@ def _execute_stage_subprocess( ) resolved = _resolve_args(stage.get("args", []), ctx) + if stage_id == _PARTITION_STAGE_ID: + resolved = _augment_partition_args(resolved, ctx) stdout_target = stage.get("stdout") rc = _run_stage_subprocess( stage["subcommand"], resolved, repo, @@ -1030,10 +1062,10 @@ def subprocess_walk_prefix( repo: Path, env: dict[str, str], *, - stop_before: str = PHASE1_STOP_STAGE, + stop_before: str = REVIEWER_FLEET_STAGE, singleton_stubs: dict[str, dict[str, Any]] | None = None, ) -> list[StageResult]: - """A-side walk: subprocess per stage, Gate A + singleton dispatch, no route.""" + """A-side walk: subprocess per stage, Gate A + singleton dispatch + Gate B.""" stubs = singleton_stubs or {} completed: set[str] = set() results: list[StageResult] = [] @@ -1046,6 +1078,8 @@ def subprocess_walk_prefix( _singleton_dispatch(stage_id, ctx, stubs) if stage_id == _HYGIENE_STAGE_ID and ctx.flags.get("hygiene_only"): break # Gate A + if stage_id == _CACHE_CHECK_STAGE_ID: + _run_route_gate_b_subprocess(ctx, repo, env) # Gate B return results 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 da88aba..ae3f69f 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -22751,31 +22751,36 @@ def test_error_on_aborting_stage( assert result["failed_stage"] == "stage_bad" assert result["message"] - def test_ready_for_route_at_partition_boundary( + def test_ready_for_reviewers_at_fleet_boundary( self, tmp_path: Path, capsys: pytest.CaptureFixture[str], ) -> None: - # Reaching the partition stage stops the walk BEFORE running it (Phase 1). + # Reaching the reviewer fleet stops the walk BEFORE running it and + # returns the Gate B routing fields the orchestrator prints. self._write_plan(tmp_path, [ {"id": "stage_01_setup", "kind": "helper", "subcommand": "setup", "enabled": True, "depends_on": [], "expected_outputs": []}, - {"id": "stage_17_partition", "kind": "helper", "subcommand": "partition", + {"id": "stage_20_spawn_reviewers", "kind": "agent_fleet", "enabled": True, "depends_on": [], "args": [], "expected_outputs": []}, ]) result = self._run(tmp_path, capsys) - assert result["next_action"] == "ready_for_route" - assert result["resume_stage"] == "stage_17_partition" + assert result["next_action"] == "ready_for_reviewers" + assert result["resume_stage"] is None assert result["failed_stage"] is None + # Gate B routing fields are always present on this terminal. + assert result["fast_path"] is False # no route ran (no cache-check stage) + assert "max_bha_agents" in result + assert "cache_status_message" in result - def test_ready_for_route_at_end_when_no_partition( + def test_ready_for_reviewers_at_end( self, tmp_path: Path, capsys: pytest.CaptureFixture[str], ) -> None: - # A plan that ends before any partition stage completes cleanly. + # A plan that ends before the reviewer fleet completes cleanly. self._write_plan(tmp_path, [ {"id": "stage_01_setup", "kind": "helper", "subcommand": "setup", "enabled": True, "depends_on": [], "expected_outputs": []}, ]) result = self._run(tmp_path, capsys) - assert result["next_action"] == "ready_for_route" + assert result["next_action"] == "ready_for_reviewers" assert result["resume_stage"] is None def test_output_flag_writes_status_to_file(self, tmp_path: Path) -> None: @@ -22789,4 +22794,68 @@ def test_output_flag_writes_status_to_file(self, tmp_path: Path) -> None: rc = cmd_run_prefix(self._ns(tmp_path, output=str(out_path))) assert rc == 0 result = json.loads(out_path.read_text()) - assert result["next_action"] == "ready_for_route" + assert result["next_action"] == "ready_for_reviewers" + + +class TestRunPrefixRoutePartition: + """Gate B route + partition folding (PLN-1229 Phase 2).""" + + def test_augment_partition_args_adds_budget_flags(self, tmp_path: Path) -> None: + from code_review_helpers import _rp_augment_partition_args + + ctx = _rp_ctx(tmp_path, max_bha_agents=7) + out = _rp_augment_partition_args(["--diff-data", "x/diff_data.json"], ctx) + assert "--loc-budget" in out and "500" in out + assert "--max-files" in out and "25" in out + # max_bha_agents from the route decision is threaded through. + assert out[out.index("--max-bha-agents") + 1] == "7" + + def test_augment_partition_swaps_to_uncached_when_cached( + self, tmp_path: Path, + ) -> None: + from code_review_helpers import _rp_augment_partition_args + + (tmp_path / "cache_config.json").write_text(json.dumps({"cache_dir": "/c"})) + (tmp_path / "uncached_diff_data.json").write_text("{}") + ctx = _rp_ctx(tmp_path) + out = _rp_augment_partition_args( + ["--diff-data", str(tmp_path / "diff_data.json")], ctx, + ) + # The plan's diff_data.json is swapped for the uncached subset. + assert str(tmp_path / "uncached_diff_data.json") in out + assert str(tmp_path / "diff_data.json") not in out + + def test_augment_partition_no_swap_without_cache(self, tmp_path: Path) -> None: + from code_review_helpers import _rp_augment_partition_args + + # No cache_config → no uncached swap; diff-data stays as-is. + ctx = _rp_ctx(tmp_path) + out = _rp_augment_partition_args( + ["--diff-data", str(tmp_path / "diff_data.json")], ctx, + ) + assert str(tmp_path / "diff_data.json") in out + + def test_fast_path_skips_partition_stage(self, tmp_path: Path) -> None: + from code_review_helpers import _execute_stage_inprocess + + ctx = _rp_ctx(tmp_path, fast_path=True) + + def _boom(_ns: argparse.Namespace) -> int: + raise AssertionError("partition must not run in fast-path") + + parser = _fake_stage_parser(_boom) + status, _msg = _execute_stage_inprocess( + {"id": "stage_17_partition", "kind": "helper", "subcommand": "fake", + "enabled": True, "depends_on": [], "args": [], "expected_outputs": []}, + ctx, parser, set(), + ) + assert status == "skipped_fast_path" + + def test_cache_status_message_read_from_artifact(self, tmp_path: Path) -> None: + from code_review_helpers import _rp_cache_status_message + + assert _rp_cache_status_message(tmp_path) is None # no artifact + (tmp_path / "cache_result.json").write_text( + json.dumps({"status_message": "1/2 files cached"}), + ) + assert _rp_cache_status_message(tmp_path) == "1/2 files cached" diff --git a/plugins/code-review/tools/python/test_prefix_golden.py b/plugins/code-review/tools/python/test_prefix_golden.py index d0d41c8..be78a73 100644 --- a/plugins/code-review/tools/python/test_prefix_golden.py +++ b/plugins/code-review/tools/python/test_prefix_golden.py @@ -253,28 +253,37 @@ def test_prefix_matches_golden(name: str, tmp_path: Path, update_golden: bool) - # The refactor guarantee: run the deterministic prefix two ways and assert the # artifacts are byte-identical (modulo review_id / timestamps / abs paths, which # normalization scrubs). A-side is the subprocess-per-stage walk (what start.md -# does today); B-side is production ``run-prefix``. Both stop at the Phase-1 -# boundary (before stage_17_partition — route + partition are Phase 2), so the -# compared artifact set runs 01→cache_check. A and B implement the walk WRAPPER -# independently, so a shared wrapper bug cannot hide. +# does today); B-side is production ``run-prefix``. Both run the WHOLE +# deterministic prefix — including Gate B route + partition — and stop at the +# reviewer fleet, so the compared artifact set runs 01→derive-spawn-spec. A and +# B implement the walk WRAPPER independently, so a shared wrapper bug can't hide. # The pause sequence each fixture drives run-prefix through, as # ``[:]`` per emitted segment. Pins the resumable -# 3-segment contract at the integration level (the two singletons almost always -# fire; hygiene-only is the one-segment Gate A exit). +# segment contract at the integration level (the two singletons almost always +# fire; hygiene-only is the one-segment Gate A exit; the terminal segment runs +# Gate B route + partition and returns ready_for_reviewers). _EXPECTED_SEGMENTS: dict[str, list[str]] = { - "golden_prefix_standard": ["needs_singleton:extract_signals", "ready_for_route"], - "golden_prefix_fast_path": ["needs_singleton:extract_signals", "ready_for_route"], + "golden_prefix_standard": [ + "needs_singleton:extract_signals", "ready_for_reviewers", + ], + "golden_prefix_fast_path": [ + "needs_singleton:extract_signals", "ready_for_reviewers", + ], "golden_prefix_hygiene_only": ["hygiene_exit"], - "golden_prefix_empty_diff": ["needs_singleton:extract_signals", "ready_for_route"], - "golden_prefix_cache_hit": ["needs_singleton:extract_signals", "ready_for_route"], + "golden_prefix_empty_diff": [ + "needs_singleton:extract_signals", "ready_for_reviewers", + ], + "golden_prefix_cache_hit": [ + "needs_singleton:extract_signals", "ready_for_reviewers", + ], "golden_prefix_since_last_review": [ - "needs_singleton:extract_signals", "ready_for_route", + "needs_singleton:extract_signals", "ready_for_reviewers", ], "golden_prefix_coverage_critic": [ "needs_singleton:extract_signals", "needs_singleton:coverage_critic", - "ready_for_route", + "ready_for_reviewers", ], } @@ -323,8 +332,15 @@ def test_run_prefix_pause_sequence(name: str, tmp_path: Path) -> None: _snaps, statuses = run_prefix_fixture_via_runner(tmp_path, _FIXTURE_FACTORIES[name]()) assert _segment_labels(statuses) == _EXPECTED_SEGMENTS[name] # The final segment resolves the pipeline (no dangling needs_singleton). - assert statuses[-1]["next_action"] in ("ready_for_route", "hygiene_exit") - assert statuses[-1]["failed_stage"] is None + terminal = statuses[-1] + assert terminal["next_action"] in ("ready_for_reviewers", "hygiene_exit") + assert terminal["failed_stage"] is None + # A ready_for_reviewers terminal carries the Gate B routing decision the + # orchestrator prints without re-reading spawn.json. + if terminal["next_action"] == "ready_for_reviewers": + assert isinstance(terminal["fast_path"], bool) + assert "cache_status_message" in terminal + assert "max_bha_agents" in terminal # Every needs_singleton names the sibling consolidate stage to resume at. for status in statuses: if status["next_action"] == "needs_singleton": From 7b89fde9fa1e08fadd168780bde5bd22fdd21a09 Mon Sep 17 00:00:00 2001 From: Kris Wong Date: Thu, 2 Jul 2026 11:36:22 -0500 Subject: [PATCH 2/3] docs(code-review): changelog + README for run-prefix route/partition folding (PLN-1229 Phase 2) Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 6 ++++++ plugins/code-review/README.md | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 918accf..7a7fbb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to the claude-plugins project will be documented in this fil The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Entries are listed newest-first; each plugin section is treated as released when merged to `main`. +### code-review v3.3.0 + +#### Changed +- `run-prefix` now runs the **entire** deterministic review prefix in one process, folding in the Gate B model-routing (`route`) and file partitioning that previously sat outside the runner. After the cache check it computes the routing decision itself (writing `spawn.json.route`), then — unless the fast path is selected — partitions the changed files (applying the reviewer-budget caps and, when a cache directory is active, restricting partitions to the files that missed the cache). In fast-path mode partitioning is skipped entirely and the cached Bug-Hunter-A replay artifact is removed. The terminal result is now `ready_for_reviewers`, which carries the `fast_path` decision, the Bug-Hunter-A agent cap, and the cache status message so the orchestrator can print the routing and cache notices without re-reading `spawn.json`; a routing failure is surfaced as an `error` result. Documented in `SCHEMA.md`. +- The subprocess A/B parity oracle now walks the whole prefix through partitioning and spawn-spec derivation on both sides, so its byte-identical-artifact guarantee covers the fast-path and partitioned branches (including `partitions.json` and `spawn.json`) across all seven fixtures. + ### code-review v3.2.0 #### Added diff --git a/plugins/code-review/README.md b/plugins/code-review/README.md index 75f2d78..c01fc6f 100644 --- a/plugins/code-review/README.md +++ b/plugins/code-review/README.md @@ -231,7 +231,7 @@ The helper script is a multi-subcommand Python CLI. The orchestrator invokes it | `finalize-result` | Consolidates validated findings + coverage state + verdict into the canonical `review_result.json` envelope; deep-merges `/telemetry.json` into the canonical `telemetry` block and populates `telemetry.cache_hit_rate["bha"]` from `cache_result.json` (PLN-719 Phase 7/9) | | `arbitrate-budget` | Applies the canonical reviewer cap policy; emits coverage gaps for required reviewers that overflow (PLN-719) | | `prepare-run` | Emits a declarative `run_plan.json` describing the 30-stage pipeline (PLN-719) | -| `run-prefix` | Runs the deterministic prefix (setup→cache-check) in one process, resolving tokens and honoring gates/`on_failure`; pauses at the hygiene-only exit, a singleton needing an agent, or the route/partition boundary, emitting a status JSON and resuming from a given stage (PLN-1229) | +| `run-prefix` | Runs the whole deterministic prefix (setup through Gate B route + partition + spawn-spec derivation) in one process, resolving tokens and honoring gates/`on_failure`; pauses at the hygiene-only exit or a singleton needing an agent, otherwise returns `ready_for_reviewers` with the fast-path/cache-status decision — emitting a status JSON and resuming from a given stage (PLN-1229) | ## GitHub CI Mode From 5909f7ecac9e49691a4d0c09320b28b1112f4974 Mon Sep 17 00:00:00 2001 From: Kris Wong Date: Thu, 2 Jul 2026 12:45:22 -0500 Subject: [PATCH 3/3] fix(code-review): resumable route-failure error + unconditional Gate B route (PLN-1229 Phase 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review of PR #174: - Route-failure error was non-resumable: it set failed_stage="route" (not a real plan stage) and resume_stage=stage_17_partition (which would skip route on retry). Anchor both on stage_19_cache_check so a per-stage fallback re-runs cache-check → route → partition, restoring the resume_stage == failed_stage == real-stage-id invariant every other error path holds. Adds a test that stubs route to fail and asserts the contract. - run-prefix gated the Gate B route call on `status != "skipped"`, diverging from start.md's Walker Contract (route reads the diff, not the cache result, so it fires unconditionally after cache-check) and from the parity oracle's A-side, which runs route unconditionally. Removed the guard so both sides — and the walker contract — agree; the A/B byte-equality oracle stays green. - SCHEMA.md §7b now states exactly which result fields (fast_path / max_bha_agents / cache_status_message) are present per next_action, and that they are omitted (not null) elsewhere, so consumers use .get(). Full code-review suite 1306 passed / 3 skipped; ruff + pyright clean. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 2 +- plugins/code-review/SCHEMA.md | 13 +++++++--- .../tools/python/code_review_helpers.py | 14 +++++++---- .../tools/python/test_code_review_helpers.py | 24 +++++++++++++++++++ 4 files changed, 45 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a7fbb3..c5d2456 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.3.0 #### Changed -- `run-prefix` now runs the **entire** deterministic review prefix in one process, folding in the Gate B model-routing (`route`) and file partitioning that previously sat outside the runner. After the cache check it computes the routing decision itself (writing `spawn.json.route`), then — unless the fast path is selected — partitions the changed files (applying the reviewer-budget caps and, when a cache directory is active, restricting partitions to the files that missed the cache). In fast-path mode partitioning is skipped entirely and the cached Bug-Hunter-A replay artifact is removed. The terminal result is now `ready_for_reviewers`, which carries the `fast_path` decision, the Bug-Hunter-A agent cap, and the cache status message so the orchestrator can print the routing and cache notices without re-reading `spawn.json`; a routing failure is surfaced as an `error` result. Documented in `SCHEMA.md`. +- `run-prefix` now runs the **entire** deterministic review prefix in one process, folding in the Gate B model-routing (`route`) and file partitioning that previously sat outside the runner. After the cache check it computes the routing decision itself (writing `spawn.json.route`), then — unless the fast path is selected — partitions the changed files (applying the reviewer-budget caps and, when a cache directory is active, restricting partitions to the files that missed the cache). In fast-path mode partitioning is skipped entirely and the cached Bug-Hunter-A replay artifact is removed. The terminal result is now `ready_for_reviewers`, which carries the `fast_path` decision, the Bug-Hunter-A agent cap, and the cache status message so the orchestrator can print the routing and cache notices without re-reading `spawn.json`. Routing runs unconditionally after the cache check (matching the walker contract, since it reads the diff rather than the cache result), and a routing failure is surfaced as an `error` result anchored on the cache-check stage so a per-stage fallback re-runs routing rather than skipping it. Documented in `SCHEMA.md`, which now also specifies exactly which result fields are present per outcome. - The subprocess A/B parity oracle now walks the whole prefix through partitioning and spawn-spec derivation on both sides, so its byte-identical-artifact guarantee covers the fast-path and partitioned branches (including `partitions.json` and `spawn.json`) across all seven fixtures. ### code-review v3.2.0 diff --git a/plugins/code-review/SCHEMA.md b/plugins/code-review/SCHEMA.md index 0dd2b53..727c761 100644 --- a/plugins/code-review/SCHEMA.md +++ b/plugins/code-review/SCHEMA.md @@ -516,9 +516,16 @@ process, the `depends_on` `completed` set is reconstructed from artifacts on dis | `failed_stage` | string \| null | The aborting stage id when `next_action == "error"`. | | `ran_stages` | string[] | Stage ids executed (or `continue`-failed) this segment, in order. | | `message` | string \| null | Short diagnostic on `error`, else null. | -| `fast_path` | bool | Gate B routing decision (on `ready_for_reviewers`). | -| `max_bha_agents` | int \| null | Gate B Bug-Hunter-A agent cap (on `ready_for_reviewers`). | -| `cache_status_message` | string \| null | `cache_result.json.status_message` to print (Gate A/B), else null. | +| `fast_path` | bool | Gate B routing decision. Present **only** on `ready_for_reviewers`. | +| `max_bha_agents` | int \| null | Gate B Bug-Hunter-A agent cap. Present **only** on `ready_for_reviewers`.| +| `cache_status_message` | string \| null | `cache_result.json.status_message` to print. Present on `ready_for_reviewers` and `hygiene_exit`. | + +`next_action`, `resume_stage`, `singleton`, `failed_stage`, `ran_stages`, and +`message` are present on every result. The three Gate-B fields above are +**omitted entirely** (not set to null) on the results that don't carry them — +`fast_path` / `max_bha_agents` appear only on `ready_for_reviewers`, and +`cache_status_message` only on `ready_for_reviewers` / `hygiene_exit`. Read them +with `.get()`, not direct indexing. **`next_action` values:** diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index 12511ae..b8a34f3 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -10977,14 +10977,20 @@ def reviewers_result() -> dict[str, Any]: # Gate B — after cache-check, run ``route`` to compute fast_path / # max_bha_agents (writing spawn.json.route) before the partition stage. - if sid == _RP_CACHE_CHECK_STAGE and status != "skipped": + # Fired unconditionally after cache-check, matching start.md's Walker + # Contract (route reads diff_data, not the cache result, so it does not + # depend on cache-check succeeding) and the parity harness's A-side. + if sid == _RP_CACHE_CHECK_STAGE: if _rp_run_route(ctx, parser) != 0: + # Route is not a plan stage, so anchor the resumable error on + # cache-check: a per-stage fallback resumes there and re-runs + # cache-check → route → partition (route gets retried). return emit({ "next_action": "error", - "resume_stage": _RP_PARTITION_STAGE, + "resume_stage": _RP_CACHE_CHECK_STAGE, "singleton": None, - "failed_stage": "route", - "message": "Gate B route failed", + "failed_stage": _RP_CACHE_CHECK_STAGE, + "message": "Gate B route failed after cache-check", }) # PLN-725 singleton pause — a needs_agent prepare stage yields to the 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 ae3f69f..7e99d39 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -22796,6 +22796,30 @@ def test_output_flag_writes_status_to_file(self, tmp_path: Path) -> None: result = json.loads(out_path.read_text()) assert result["next_action"] == "ready_for_reviewers" + def test_route_failure_emits_resumable_error( + self, tmp_path: Path, capsys: pytest.CaptureFixture[str], + ) -> None: + # Reaching cache-check fires Gate B route; a route failure must anchor the + # resumable error on a real, re-runnable stage (cache-check) so a per-stage + # fallback re-runs cache-check → route → partition — NOT on the synthetic + # "route" label or the partition stage (which would skip route on retry). + self._write_plan(tmp_path, [ + {"id": "stage_01_setup", "kind": "helper", "subcommand": "setup", + "enabled": True, "depends_on": [], "expected_outputs": []}, + {"id": "stage_19_cache_check", "kind": "helper", "subcommand": "setup", + "enabled": True, "depends_on": [], "args": [], "expected_outputs": [], + "on_failure": "continue"}, + {"id": "stage_20_spawn_reviewers", "kind": "agent_fleet", + "enabled": True, "depends_on": [], "args": [], "expected_outputs": []}, + ]) + with patch("code_review_helpers._rp_run_route", return_value=1): + result = self._run(tmp_path, capsys) + assert result["next_action"] == "error" + # failed_stage and resume_stage are the SAME real, resumable stage id. + assert result["failed_stage"] == "stage_19_cache_check" + assert result["resume_stage"] == "stage_19_cache_check" + assert result["message"] + class TestRunPrefixRoutePartition: """Gate B route + partition folding (PLN-1229 Phase 2)."""