From ff9db9b177132a76e438aadf2e926fb4a322076a Mon Sep 17 00:00:00 2001 From: Peter Ulsteen Date: Tue, 29 Sep 2026 05:54:42 -0500 Subject: [PATCH 1/3] fix(code-review): stop a docs-only BHA skip from blocking (ISS-10869) - arbitrate-budget writes budget.docs_only: true when it waives the BHA floor, so the zero bha_partitions carries its cause - derive-spawn-spec records that zero as skipped reason "docs_only", a benign skip that emits no coverage-gap finding - A zero cap without the marker, and partitions beyond bha_partitions, still record budget_capped and the required gap - docs_only added to SPAWN_SPEC_SKIP_REASONS, SCHEMA.md, start.md - Bump code-review to 3.10.3 Testing: pytest plugins/code-review (1401 passed, 3 skipped); ruff and pyright clean; arbitrate -> derive-spawn-spec -> finalize-result -> verdict chain run for docs-only, mixed and cap-pressure diffs Risks: None identified; the budget key is additive and absent unless the diff is docs-only Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 9 ++ .../code-review/.claude-plugin/plugin.json | 2 +- plugins/code-review/SCHEMA.md | 16 +- plugins/code-review/commands/start.md | 2 +- .../tools/python/code_review_helpers.py | 51 ++++-- .../tools/python/code_review_schema.py | 1 + .../tools/python/test_code_review_helpers.py | 148 +++++++++++++++++- 7 files changed, 199 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f9307ea..6652fc49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ 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.10.3 + +#### Fixed +- A docs-only diff no longer gets a `CHANGES_REQUESTED` verdict for not spawning `bug_hunter_a`. `arbitrate-budget` already set `bha_partitions` to 0 when every changed file is documentation, but `derive-spawn-spec` recorded that skip as `reason: "budget_capped"`. Because `bug_hunter_a` is a required reviewer, a `Required reviewer dropped: bug_hunter_a` coverage-gap finding followed, advising the operator to raise `--cap`. + - `arbitrate-budget` now writes `budget.docs_only: true` into `coverage.json.final` when it waives the BHA floor, on both the arbitrated and the `blocked_by_verify` paths. The key is absent for other diffs. + - `derive-spawn-spec` records a zero cap that carries that marker as `skipped[].reason: "docs_only"`. It is benign, like `no_partitions`, so it emits no coverage-gap finding. + - A zero cap without the marker, and partitions dropped because the partitioner produced more than `bha_partitions`, still record `budget_capped` and still produce the required-reviewer coverage gap. + - `docs_only` is added to `SPAWN_SPEC_SKIP_REASONS`, and SCHEMA.md and `start.md` list it with the other benign skip reasons. + ### code v1.15.0 #### Added diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index f5de735a..8b811a4f 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.10.2", + "version": "3.10.3", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code-review/SCHEMA.md b/plugins/code-review/SCHEMA.md index aaa839d5..6141868f 100644 --- a/plugins/code-review/SCHEMA.md +++ b/plugins/code-review/SCHEMA.md @@ -170,7 +170,8 @@ The terminal artifact of every review run. "total_cap": , "required_count": , "best_effort_count": , - "bha_partitions": + "bha_partitions": , + "docs_only": true // only when arbitrate-budget waived the BHA floor } }, "coverage_gaps": [, ...], @@ -347,7 +348,7 @@ ignored at spawn time. { "reviewer": "", "bucket": "required | best_effort", - "reason": "deferred_pln723 | no_partitions | unknown_reviewer | missing_reviewer_name | duplicate_agent_id | budget_capped | gated_by_verify", + "reason": "deferred_pln723 | no_partitions | docs_only | unknown_reviewer | missing_reviewer_name | duplicate_agent_id | budget_capped | gated_by_verify", "agent_id": "", // only on duplicate_agent_id "partition_id": 0, // only on budget_capped (BHA) "budget_cap": 0, // only on budget_capped @@ -376,7 +377,7 @@ constants in `code_review_schema.py`): | `arbitrate_status` | `ok`, `blocked_by_verify`, `fallback`, `static` | `ok` = normal arbitration ran; `blocked_by_verify` = Phase 7 BLOCKING gate fired upstream and the plan passed through unchanged; `fallback` = derive failed, orchestrator must walk the static reviewer table in the `code-review:spawn-reviewers` skill; `static` (PLN-807) = shallow tier — the spec was emitted by `derive-static-spec` (fixed BHA + BHB + unified_auditor fleet) without consulting a coverage plan, and `stage_20` treats it identically to `fallback` (use the spec verbatim, skip the bucket walk); the distinct status is a telemetry signal that distinguishes user intent (shallow) from upstream derive failure | | `source` | `core`, `rule`, `critic`, `fast_path` | Selects the prompt-suffix dispatch in the `code-review:spawn-reviewers` skill (`source: "core"` further branches on `reviewer`; `rule` and `critic` both map to the Domain Critic suffix — `rule` for deterministically matched critic-gates rules including migrated `moduleCritics[]`, `critic` for LLM-proposed additions) | | `bucket` | `required`, `best_effort`, `fast_path` | Mirrors the source bucket in `coverage_plan.json` | -| `skipped[].reason` | `deferred_pln723`, `no_partitions`, `unknown_reviewer`, `missing_reviewer_name`, `duplicate_agent_id`, `budget_capped`, `gated_by_verify` | Reasons surfaced so operators see why a reviewer was omitted | +| `skipped[].reason` | `deferred_pln723`, `no_partitions`, `docs_only`, `unknown_reviewer`, `missing_reviewer_name`, `duplicate_agent_id`, `budget_capped`, `gated_by_verify` | Reasons surfaced so operators see why a reviewer was omitted | | `fallback_reason` | `coverage_plan_missing_or_malformed`, `partitions_missing_or_malformed` | Only set when `arbitrate_status == "fallback"`; names the specific upstream-artifact failure | **Fallback sentinel invariant:** when `arbitrate_status == "fallback"`, @@ -403,12 +404,15 @@ does not duplicate it. Presenters use `gated_by_verify` to surface by the count of partitions in `partitions.json`. When the partitioner emitted more partitions than the budget reserved, the first `cap` partitions spawn and the rest land in `skipped[]` with -`reason: "budget_capped"`. A cap of 0 (docs-only post-arbitrate) -suppresses all BHA spawns regardless of partition count. +`reason: "budget_capped"`. A cap of 0 suppresses all BHA spawns +regardless of partition count. When arbitrate-budget set the cap to 0 +because the diff is docs-only (`coverage_plan.budget.docs_only: true`), +the single skipped entry carries `reason: "docs_only"`; a cap of 0 +without that marker keeps `reason: "budget_capped"`. **Required coverage-gap invariant:** every entry in `skipped[]` with `bucket == "required"` and a non-benign reason -(everything except `deferred_pln723`, `no_partitions`, +(everything except `deferred_pln723`, `no_partitions`, `docs_only`, `gated_by_verify`) produces a coverage-gap finding appended to `coverage_gaps.json`. `cmd_finalize_result` reads that file into the envelope's coverage-gap bucket where it contributes to the canonical diff --git a/plugins/code-review/commands/start.md b/plugins/code-review/commands/start.md index cd4a59a1..8fc9c51c 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_12_hygiene**: writes `/hygiene.json` with hygiene findings. Triggers **Gate A** (hygiene-only exit) immediately after. - **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." **Exit `3` is NOT that case** — it is a review-root refusal, and the static table would spawn the same agents against the same wrong tree, so it aborts the walk regardless of `on_failure`. 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_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"`. When the cap is 0 because the diff is docs-only (`coverage_plan.budget.docs_only: true`), BHA lands in `skipped[]` with `reason: "docs_only"` instead. 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`, `docs_only`, `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." **Exit `3` is NOT that case** — it is a review-root refusal, and the static table would spawn the same agents against the same wrong tree, so it aborts the walk regardless of `on_failure`. Note: stage_19b depends only on `stage_16_arbitrate_budget`, NOT on `stage_17_partition`, so Gate B's fast-path branch (which skips stage_17) can still reach stage_20 with a fast descriptor. - **stage_20_spawn_reviewers**: agent_fleet stage. Invoke the `code-review:spawn-reviewers` skill. The skill reads `/spawn.json` (`spec` section) first and dispatches one Task per agent descriptor (using the `agent_id`, `reviewer`, `model`, and `patches_file` from the spec). If `spawn.json` is missing, its `spec` section is absent, or it marks `arbitrate_status: "fallback"`, the skill walks its static reviewer table fallback instead — a derive failure must never block review. In `MODE=github`, the walker must follow the skill's synchronous standard-flow branch: do not use `TaskOutput`, watcher files, sleep loops, polling loops, or turn-ending waits as replacements for synchronous reviewer completion. `stage_20` must complete every GitHub synchronous reviewer and retry, leaving no reviewer task still running, or fail before `stage_21_collect_findings`. - **stage_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. diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index ddd6eeb1..b996584a 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -12238,6 +12238,7 @@ def cmd_arbitrate_budget(args: argparse.Namespace) -> int: return 1 diff_data = _read_optional_json(Path(args.diff_data), {}) or {} + docs_only = _is_docs_only(diff_data) cap: int = int(args.cap) if cap <= 0: print(f"Error: --cap must be > 0, got {cap}", file=sys.stderr) @@ -12325,7 +12326,7 @@ def _plan_target_str() -> str: # leftover capacity, so a BLOCKING verdict on a critic-heavy # plan does not crush BHA to its floor=1 the same way the # pre-PLN-807 PASS path did. Docs-only PRs still get 0. - if _is_docs_only(diff_data): + if docs_only: blocking_bha_partitions = 0 else: blocking_bha_partitions = max( @@ -12351,6 +12352,8 @@ def _plan_target_str() -> str: "arbitrate_status": "blocked_by_verify", "generated_at": now_iso_gate, } + if docs_only: + final_plan["budget"]["docs_only"] = True rc = _persist_plan(final_plan) if rc != 0: return rc @@ -12377,7 +12380,7 @@ def _plan_target_str() -> str: sys.stdout.write("\n") return 0 - bha_floor = 0 if _is_docs_only(diff_data) else BUDGET_BHA_FLOOR_DEFAULT + bha_floor = 0 if docs_only else BUDGET_BHA_FLOOR_DEFAULT max_bha = _max_bha_partitions_by_loc(diff_data) now_iso = datetime.now(timezone.utc).isoformat() @@ -12393,7 +12396,7 @@ def _plan_target_str() -> str: # coverage budget. ``_max_bha_partitions_by_loc`` already caps at # ``DEFAULT_MAX_BHA_AGENTS`` internally, so no second min() is # needed. - if _is_docs_only(diff_data): + if docs_only: bha_target = 0 else: bha_target = max(bha_floor, max_bha) @@ -12533,6 +12536,10 @@ def _plan_target_str() -> str: }, "dropped_required": dropped_required, } + # ISS-10869: records why ``bha_partitions`` is 0 so derive-spawn-spec + # can tell the deliberate docs-only skip from a budget-capped drop. + if docs_only: + final_plan["budget"]["docs_only"] = True rc = _persist_plan(final_plan) if rc != 0: @@ -12550,7 +12557,7 @@ def _plan_target_str() -> str: "deferred_count": len(deferred_for_budget), "dropped_required_count": len(dropped_required), "bha_partitions": bha_partitions, - "docs_only": _is_docs_only(diff_data), + "docs_only": docs_only, }, sys.stdout, indent=2, @@ -12685,6 +12692,7 @@ def _derive_spawn_agents_from_plan( models: dict[str, Any], *, bha_partitions_cap: int | None = None, + docs_only: bool = False, ) -> tuple[list[dict[str, Any]], list[dict[str, Any]]]: """Walk the post-arbitrate plan into a flat (agents, skipped) pair. @@ -12700,9 +12708,11 @@ def _derive_spawn_agents_from_plan( and the post-arbitrate cap are computed at different times and can diverge), the first K partitions are spawned and the rest land in ``skipped[]`` with ``reason: "budget_capped"``. A cap of 0 - suppresses all BHA spawns (docs-only post-arbitrate). ``None`` - means "no cap" — only used by callers that pre-date the cap - parameter. + suppresses all BHA spawns: with ``docs_only`` (arbitrate-budget + waived the BHA floor) the one skipped entry carries + ``reason: "docs_only"``; without it the zero is a cap and keeps + ``reason: "budget_capped"``. ``None`` means "no cap" — only used by + callers that pre-date the cap parameter. """ agents: list[dict[str, Any]] = [] skipped: list[dict[str, Any]] = [] @@ -12744,8 +12754,7 @@ def _emit_for_entry(entry: dict[str, Any], bucket: str) -> None: # partitions on disk (the route-level # ``max_bha_agents`` and the post-arbitrate cap are # computed at different times and can diverge). When - # the cap is 0, no BHA agents spawn at all - # (docs-only post-arbitrate). When the cap is positive + # the cap is 0, no BHA agents spawn at all. When the cap is positive # but < len(partitions), the first cap partitions # spawn (partitions are bin-packed by the partitioner # in roughly diff-LOC order, so prefix-take is the @@ -12754,8 +12763,14 @@ def _emit_for_entry(entry: dict[str, Any], bucket: str) -> None: dropped_for_cap: list[dict[str, Any]] = [] if bha_partitions_cap is not None: cap = max(0, int(bha_partitions_cap)) + if cap == 0 and docs_only: + skipped.append({ + "reviewer": reviewer, + "bucket": bucket, + "reason": "docs_only", + }) + return if cap == 0: - # Docs-only post-arbitrate; skip BHA entirely. skipped.append({ "reviewer": reviewer, "bucket": bucket, @@ -13080,6 +13095,7 @@ def cmd_derive_spawn_spec(args: argparse.Namespace) -> int: agents, skipped = _derive_spawn_agents_from_plan( plan_for_spawn, partitions, models, bha_partitions_cap=bha_cap, + docs_only=budget.get("docs_only") is True, ) skipped.extend(sanitized_extras) @@ -13089,12 +13105,13 @@ def cmd_derive_spawn_spec(args: argparse.Namespace) -> int: # missing required reviewer). The spawn-spec path now emits the # same canonical Coverage finding so finalize-result picks it up # via coverage_gaps.json. Benign reasons (test_quality deferral, - # all-cached/docs-only no_partitions, gated_by_verify suppression) - # are explicitly excluded — those are intentional omissions, not + # all-cached/docs-only no_partitions, the docs_only BHA floor waiver, + # gated_by_verify suppression) are explicitly excluded — those are intentional omissions, not # coverage gaps. _SPAWN_BENIGN_REQUIRED_SKIPS = { "deferred_pln723", # PLN-723 placeholder slot "no_partitions", # all-cached / docs-only + "docs_only", # arbitrate-budget waived the BHA floor "gated_by_verify", # BLOCKING sanitization } spawn_gap_findings = _build_spawn_required_gap_findings( @@ -13274,8 +13291,9 @@ def _build_spawn_required_gap_findings( Benign reasons (intentional omissions, not coverage gaps): the PLN-723 ``deferred_pln723`` placeholder, ``no_partitions`` (all-cached / docs-only — BHA legitimately has nothing to do), - and ``gated_by_verify`` (BLOCKING sanitization already surfaces - via agent_coverage-verify-blocking.json). + ``docs_only`` (arbitrate-budget waived the BHA floor for a + docs-only diff), and ``gated_by_verify`` (BLOCKING sanitization + already surfaces via agent_coverage-verify-blocking.json). """ findings: list[dict[str, Any]] = [] idx = 0 @@ -13808,11 +13826,12 @@ def _render_fleet_notes( # one note. Two emission shapes from _derive_spawn_agents_from_plan: # - cap > 0: one capped entry per dropped partition (each # carries ``partition_id``). Dropped count = len(entries). - # - cap == 0 (docs-only post-arbitrate): a single aggregate + # - cap == 0 without the docs-only marker: a single aggregate # entry covers ALL N suppressed partitions (no # ``partition_id``; ``partition_count`` reflects the total). # Dropped count = ``partition_count`` from the aggregate. - # Counting len(capped_entries) under-reports the docs-only case + # (A docs-only zero is ``reason: "docs_only"``, not counted here.) + # Counting len(capped_entries) under-reports the aggregate case # as "1 partition(s)" when N partitions were actually suppressed # via a single aggregate entry. capped_entries = [ diff --git a/plugins/code-review/tools/python/code_review_schema.py b/plugins/code-review/tools/python/code_review_schema.py index 0ef1a397..1bf42bd0 100644 --- a/plugins/code-review/tools/python/code_review_schema.py +++ b/plugins/code-review/tools/python/code_review_schema.py @@ -291,6 +291,7 @@ SPAWN_SPEC_SKIP_REASONS: frozenset[str] = frozenset({ "deferred_pln723", # test_quality slot reserved for PLN-723 "no_partitions", # all files cached or docs-only → no BHA + "docs_only", # arbitrate-budget waived the BHA floor (docs-only diff) "unknown_reviewer", # closed-vocab violation: not core, not critic "missing_reviewer_name", # plan entry with blank/missing reviewer "duplicate_agent_id", # same agent_id produced twice (defense-in-depth) 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 61646df2..67e4a620 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -18166,6 +18166,7 @@ def test_blocking_verdict_docs_only_diff_still_zero_bha( final = _read_coverage_section(cr_dir, "final") assert final["arbitrate_status"] == "blocked_by_verify" assert final["budget"]["bha_partitions"] == 0 + assert final["budget"]["docs_only"] is True def test_writes_coverage_gaps_alongside_aggregate(self, tmp_path: Path) -> None: """``coverage_gaps.json`` stays standalone (multi-writer). On @@ -19022,10 +19023,11 @@ def test_bha_capped_at_budget_partitions(self, tmp_path: Path) -> None: assert capped[0]["partition_count"] == 3 def test_bha_partitions_zero_suppresses_all_bha(self, tmp_path: Path) -> None: - """Docs-only post-arbitrate sets ``bha_partitions: 0``. No BHA - descriptors emit even when the partitioner produced - partitions (e.g. on a mixed docs+code diff where docs - dominate the LOC cap). + """``bha_partitions: 0`` suppresses every BHA descriptor even + when the partitioner produced partitions. Without the + ``budget.docs_only`` marker the zero is a cap, so the one + skipped entry stays ``budget_capped`` (the docs-only case is + ``TestISS10869DocsOnlyBhaSkip``). """ plan = { "required": [{"reviewer": "bug_hunter_a", "source": "core"}], @@ -19065,6 +19067,140 @@ def test_bha_cap_equals_partition_count_spawns_all( assert capped == [] +class TestISS10869DocsOnlyBhaSkip: + """ISS-10869: arbitrate-budget deliberately zeroes BHA on a docs-only + diff. derive-spawn-spec used to report that zero as a + ``budget_capped`` drop of a required reviewer, so finalize-result + carried a HIGH coverage gap and the verdict was CHANGES_REQUESTED. + Each case drives arbitrate-budget -> derive-spawn-spec -> + finalize-result -> verdict against a review root that holds the + diff's files. + """ + + _CORE_PLAN: dict[str, Any] = { + "required": [ + {"reviewer": r, "source": "core"} + for r in ( + "bug_hunter_a", "bug_hunter_b", "unified_auditor", + "test_quality", + ) + ], + "best_effort": [], + } + + @staticmethod + def _partitions(files: list[str]) -> dict[str, Any]: + return { + "partitions": [ + {"id": i, "files": [{"file": f}], "is_test_only": False} + for i, f in enumerate(files) + ], + "test_file_paths": [], + "force_merged_count": 0, + } + + def _run_chain( + self, tmp_path: Path, files: list[str], partition_files: list[str], + ) -> tuple[dict[str, Any], dict[str, Any], list[dict[str, Any]], dict[str, Any]]: + from code_review_helpers import cmd_finalize_result, cmd_verdict + from golden_fixture_harness import run_with_stdout_capture + + root = tmp_path / "review_root_repo" + resolved = _make_review_root(root, {f: "x\n" for f in files}) + (tmp_path / "scope.json").write_text(json.dumps({ + "review_root": resolved, + "review_root_sha": git_fixture(root, "rev-parse", "HEAD").strip(), + })) + _, final, _ = _run_arbitrate_budget( + tmp_path, self._CORE_PLAN, _make_diff_data(files=files), + ) + _, spec = _run_derive_spawn_spec( + tmp_path, final, self._partitions(partition_files), + {"fast_path": False, "models": {}}, + ) + validated = tmp_path / "findings_validated.json" + validated.write_text(json.dumps({"validated": [], "discarded": [], "stats": {}})) + run_with_stdout_capture(cmd_finalize_result, argparse.Namespace( + cr_dir=str(tmp_path), findings_validated=str(validated), + mode="local", diff_tip="abc1234", pr_number=None, + )) + verdict = json.loads(run_with_stdout_capture(cmd_verdict, argparse.Namespace( + findings_validated=str(validated), + review_result=str(tmp_path / "review_result.json"), + ))) + gaps = json.loads((tmp_path / "coverage_gaps.json").read_text())["findings"] + return final, spec, gaps, verdict + + def test_docs_only_diff_skips_bha_without_coverage_gap( + self, tmp_path: Path, + ) -> None: + doc = "docs/runbooks/production-branch-cutover-reset.md" + final, spec, gaps, verdict = self._run_chain(tmp_path, [doc], [doc]) + assert final["budget"]["bha_partitions"] == 0 + assert final["budget"]["docs_only"] is True + bha_skips = [s for s in spec["skipped"] if s["reviewer"] == "bug_hunter_a"] + assert bha_skips == [ + {"reviewer": "bug_hunter_a", "bucket": "required", "reason": "docs_only"}, + ] + assert spec["stats"]["required_coverage_gaps"] == 0 + assert gaps == [] + assert verdict["canonical_verdict"] == "APPROVED" + + def test_mixed_docs_and_code_diff_keeps_bha_floor( + self, tmp_path: Path, + ) -> None: + final, spec, gaps, verdict = self._run_chain( + tmp_path, ["docs/guide.md", "src/app.ts"], ["src/app.ts"], + ) + assert final["budget"]["bha_partitions"] >= 1 + assert "docs_only" not in final["budget"] + assert [a["agent_id"] for a in spec["agents"] if a["reviewer"] == "bug_hunter_a"] == ["bha_p0"] + assert [s for s in spec["skipped"] if s["reviewer"] == "bug_hunter_a"] == [] + assert gaps == [] + assert verdict["canonical_verdict"] == "APPROVED" + + def test_bha_partition_cap_still_reports_high_gap( + self, tmp_path: Path, + ) -> None: + """Counterfactual for the docs-only case: partitions the budget + could not fit are a real required-reviewer drop and still block. + """ + files = ["src/a.ts", "src/b.ts", "src/c.ts"] + final, spec, gaps, verdict = self._run_chain(tmp_path, files, files) + assert final["budget"]["bha_partitions"] == 1 + capped = [s for s in spec["skipped"] if s["reason"] == "budget_capped"] + assert [s["partition_id"] for s in capped] == [1, 2] + assert len(gaps) == 2 + for gap in gaps: + assert gap["severity"] == "HIGH" + assert gap["issue"] == "Required reviewer dropped: bug_hunter_a" + assert "budget capped" in gap["explanation"] + assert verdict["canonical_verdict"] == "CHANGES_REQUESTED" + + def test_zero_cap_without_docs_only_marker_still_reports_high_gap( + self, tmp_path: Path, + ) -> None: + """A zero ``bha_partitions`` is only benign when arbitrate-budget + marked the diff docs-only. A zero from anywhere else (the + pre-fix BLOCKING path hardcoded one on code PRs) stays a + ``budget_capped`` drop with a HIGH gap. + """ + plan = { + "required": [{"reviewer": "bug_hunter_a", "source": "core"}], + "best_effort": [], + "budget": {"total_cap": 20, "bha_partitions": 0}, + } + _, spec = _run_derive_spawn_spec( + tmp_path, plan, self._partitions(["src/a.ts"]), + {"fast_path": False, "models": {}}, + ) + assert [s["reason"] for s in spec["skipped"]] == ["budget_capped"] + gaps = json.loads((tmp_path / "coverage_gaps.json").read_text())["findings"] + assert [(g["severity"], g["issue"]) for g in gaps] == [ + ("HIGH", "Required reviewer dropped: bug_hunter_a"), + ] + + class TestPLN725Phase8DeriveSpawnSpecBlockingSanitization: """PLN-725 Phase 8 / v2.22.3 — under a BLOCKING verify verdict, the spawner sanitizes the plan to ``source: "core"`` only. @@ -20395,8 +20531,8 @@ def test_budget_capped_partitions_emits_warning( def test_budget_capped_docs_only_aggregate_uses_partition_count( self, tmp_path: Path, ) -> None: - """When the post-arbitrate cap is 0 (docs-only), - ``_derive_spawn_agents_from_plan`` emits ONE aggregate + """When the post-arbitrate cap is 0 without the docs-only + marker, ``_derive_spawn_agents_from_plan`` emits ONE aggregate ``budget_capped`` entry covering all N suppressed partitions (no ``partition_id``, ``partition_count`` = N). Pre-v2.23.3 the renderer counted From bcd0519b78f9c6222679376f5eae2d2fcb7db4b0 Mon Sep 17 00:00:00 2001 From: Peter Ulsteen Date: Tue, 29 Sep 2026 06:03:00 -0500 Subject: [PATCH 2/3] fix(code-review): correct no_partitions comments (ISS-10869) - The partitioner keeps docs files, so a docs-only diff has partitions; no_partitions means every file was cached - Rename the zero-cap fleet-summary test, which no longer describes the docs-only case Testing: ruff clean; pytest plugins/code-review Risks: None identified; comments and a test name only Co-Authored-By: Claude Opus 5.5 --- .../code-review/tools/python/code_review_helpers.py | 10 +++++----- plugins/code-review/tools/python/code_review_schema.py | 2 +- .../tools/python/test_code_review_helpers.py | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index b996584a..6d4af3dd 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -12741,7 +12741,7 @@ def _emit_for_entry(entry: dict[str, Any], bucket: str) -> None: if role_cfg is not None: if role_cfg["partitioned"]: # BHA expands per partition. When partitions is empty - # (all cached or docs-only post-arbitrate), no BHA spawn. + # (all files cached), no BHA spawn. if not partitions: skipped.append({ "reviewer": reviewer, @@ -13105,12 +13105,12 @@ def cmd_derive_spawn_spec(args: argparse.Namespace) -> int: # missing required reviewer). The spawn-spec path now emits the # same canonical Coverage finding so finalize-result picks it up # via coverage_gaps.json. Benign reasons (test_quality deferral, - # all-cached/docs-only no_partitions, the docs_only BHA floor waiver, + # all-cached no_partitions, the docs_only BHA floor waiver, # gated_by_verify suppression) are explicitly excluded — those are intentional omissions, not # coverage gaps. _SPAWN_BENIGN_REQUIRED_SKIPS = { "deferred_pln723", # PLN-723 placeholder slot - "no_partitions", # all-cached / docs-only + "no_partitions", # all files cached "docs_only", # arbitrate-budget waived the BHA floor "gated_by_verify", # BLOCKING sanitization } @@ -13235,7 +13235,7 @@ def cmd_derive_static_spec(args: argparse.Namespace) -> int: # Synthetic plan: shallow fleet is exactly BHA + BHB + unified_auditor. # Reusing _derive_spawn_agents_from_plan keeps BHA partition expansion, - # docs-only (no_partitions) handling, dedup, and patches-file naming + # all-cached (no_partitions) handling, dedup, and patches-file naming # in one place instead of duplicating per-tier logic. static_plan: dict[str, Any] = { "required": [ @@ -13290,7 +13290,7 @@ def _build_spawn_required_gap_findings( Benign reasons (intentional omissions, not coverage gaps): the PLN-723 ``deferred_pln723`` placeholder, ``no_partitions`` - (all-cached / docs-only — BHA legitimately has nothing to do), + (all files cached — BHA legitimately has nothing to do), ``docs_only`` (arbitrate-budget waived the BHA floor for a docs-only diff), and ``gated_by_verify`` (BLOCKING sanitization already surfaces via agent_coverage-verify-blocking.json). diff --git a/plugins/code-review/tools/python/code_review_schema.py b/plugins/code-review/tools/python/code_review_schema.py index 1bf42bd0..24e95800 100644 --- a/plugins/code-review/tools/python/code_review_schema.py +++ b/plugins/code-review/tools/python/code_review_schema.py @@ -290,7 +290,7 @@ # absent from the fleet. SPAWN_SPEC_SKIP_REASONS: frozenset[str] = frozenset({ "deferred_pln723", # test_quality slot reserved for PLN-723 - "no_partitions", # all files cached or docs-only → no BHA + "no_partitions", # all files cached → no BHA "docs_only", # arbitrate-budget waived the BHA floor (docs-only diff) "unknown_reviewer", # closed-vocab violation: not core, not critic "missing_reviewer_name", # plan entry with blank/missing reviewer 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 67e4a620..1bd88d5b 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -20528,7 +20528,7 @@ def test_budget_capped_partitions_emits_warning( assert "2 partition(s)" in out assert "(2/3)" in out - def test_budget_capped_docs_only_aggregate_uses_partition_count( + def test_budget_capped_zero_cap_aggregate_uses_partition_count( self, tmp_path: Path, ) -> None: """When the post-arbitrate cap is 0 without the docs-only From ca1ac39d8746fe31f3eb5aae5ee3934f7063d097 Mon Sep 17 00:00:00 2001 From: Peter Ulsteen Date: Thu, 1 Oct 2026 17:18:05 -0500 Subject: [PATCH 3/3] fix(code-review): note a docs-only BHA skip (ISS-10869) - _render_fleet_notes adds an informational "BHA skipped on a docs-only diff." note when spawn.json.spec.skipped[] holds a docs_only entry; before this the fleet summary said nothing - The docs_only skipped entry carries budget_cap: 0 and partition_count, as the zero-cap budget_capped entry does - SCHEMA.md and CHANGELOG.md (code-review v3.10.3) updated Testing: TestISS10869DocsOnlyBhaSkip docs-only case asserts the new fields and renders the fleet summary; removing the note line or the two fields turns it red. uv run pytest plugins/: 2179 passed, 3 skipped. ruff and pyright clean. Risks: None identified Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + plugins/code-review/SCHEMA.md | 4 ++-- plugins/code-review/tools/python/code_review_helpers.py | 8 ++++++++ .../code-review/tools/python/test_code_review_helpers.py | 4 +++- 4 files changed, 14 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a220f05..f2c2cd42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - `derive-spawn-spec` records a zero cap that carries that marker as `skipped[].reason: "docs_only"`. It is benign, like `no_partitions`, so it emits no coverage-gap finding. - A zero cap without the marker, and partitions dropped because the partitioner produced more than `bha_partitions`, still record `budget_capped` and still produce the required-reviewer coverage gap. - `docs_only` is added to `SPAWN_SPEC_SKIP_REASONS`, and SCHEMA.md and `start.md` list it with the other benign skip reasons. + - The `docs_only` skipped entry carries `budget_cap: 0` and `partition_count`, as the zero-cap `budget_capped` entry does, and `render-fleet-summary` adds an informational `ℹ️ BHA skipped on a docs-only diff.` note when that entry is present. ### code v1.16.3 diff --git a/plugins/code-review/SCHEMA.md b/plugins/code-review/SCHEMA.md index 6141868f..1f432bd1 100644 --- a/plugins/code-review/SCHEMA.md +++ b/plugins/code-review/SCHEMA.md @@ -351,8 +351,8 @@ ignored at spawn time. "reason": "deferred_pln723 | no_partitions | docs_only | unknown_reviewer | missing_reviewer_name | duplicate_agent_id | budget_capped | gated_by_verify", "agent_id": "", // only on duplicate_agent_id "partition_id": 0, // only on budget_capped (BHA) - "budget_cap": 0, // only on budget_capped - "partition_count": 0, // only on budget_capped + "budget_cap": 0, // only on budget_capped and docs_only + "partition_count": 0, // only on budget_capped and docs_only "source": "rule | critic" // only on gated_by_verify (preserved for presenters) } ], diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index 6d4af3dd..cae42641 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -12768,6 +12768,8 @@ def _emit_for_entry(entry: dict[str, Any], bucket: str) -> None: "reviewer": reviewer, "bucket": bucket, "reason": "docs_only", + "budget_cap": 0, + "partition_count": len(partitions), }) return if cap == 0: @@ -13860,6 +13862,12 @@ def _render_fleet_notes( ): notes.append("- ℹ️ `test_quality` slot reserved for PLN-723.") + if any( + isinstance(s, dict) and s.get("reason") == "docs_only" + for s in skipped + ): + notes.append("- ℹ️ BHA skipped on a docs-only diff.") + # Other skip reasons (unknown_reviewer / duplicate_agent_id / # missing_reviewer_name) → already produce coverage_gaps.json # findings via stage_19b. Surface them as a single line so 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 1bd88d5b..436ee970 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -19140,11 +19140,13 @@ def test_docs_only_diff_skips_bha_without_coverage_gap( assert final["budget"]["docs_only"] is True bha_skips = [s for s in spec["skipped"] if s["reviewer"] == "bug_hunter_a"] assert bha_skips == [ - {"reviewer": "bug_hunter_a", "bucket": "required", "reason": "docs_only"}, + {"reviewer": "bug_hunter_a", "bucket": "required", "reason": "docs_only", + "budget_cap": 0, "partition_count": 1}, ] assert spec["stats"]["required_coverage_gaps"] == 0 assert gaps == [] assert verdict["canonical_verdict"] == "APPROVED" + assert "- ℹ️ BHA skipped on a docs-only diff." in _run_render_fleet_summary(tmp_path) def test_mixed_docs_and_code_diff_keeps_bha_floor( self, tmp_path: Path,