diff --git a/plugins/code-review/README.md b/plugins/code-review/README.md index 7d41425..59d86fb 100644 --- a/plugins/code-review/README.md +++ b/plugins/code-review/README.md @@ -190,7 +190,7 @@ Three tiers select which reviewer fleet runs: | `design_critic` (always-on at deep) | ✗ | ✗ | ✓ | | `impact_analyzer` (FEA-1401, on exported-symbol change/deletion) | ✗ | ✗ | ✓ (on signal) | -**Standard-mode budget arithmetic.** With PLN-807 Phase 4, `arbitrate-budget` reserves BHA partitions FIRST (from `_max_bha_partitions_by_loc`) and then allocates the remaining budget to critics and best-effort. The total domain-critic count across both required and best-effort buckets is capped uniformly at `DOMAIN_CRITIC_CAP = 3` (by priority asc, reviewer asc) for both standard and deep tiers. Required-bucket critics dropped by the cap emit coverage-gap findings; cap-deferred entries carry `defer_reason: "domain_critic_cap"` in `deferred_for_budget`. PRs with sparse critic-gates rosters see identical fleet to pre-PLN-807. +**Standard-mode budget arithmetic.** With PLN-807 Phase 4, `arbitrate-budget` reserves BHA partitions FIRST (from `_max_bha_partitions_by_loc`) and then allocates the remaining budget to critics and best-effort. The total domain-critic count across both required and best-effort buckets is capped uniformly at `DOMAIN_CRITIC_CAP` (default 3, operator-tunable via `.closedloop-ai/settings/code-review.json:domain_critic_cap`; by priority asc, reviewer asc) for both standard and deep tiers. Required-bucket critics dropped by the cap emit coverage-gap findings; cap-deferred entries carry `defer_reason: "domain_critic_cap"` in `deferred_for_budget`. PRs with sparse critic-gates rosters see identical fleet to pre-PLN-807. **Tier-mismatch nudge.** Shallow runs emit a single LOW system-scoped finding (`system_marker: "tier_mismatch_nudge"`) when the diff would benefit from a higher tier. Heuristics: diff > 3000 LOC; schema/migration paths (`/migrations/`, `/schemas/`, `/models/`); public API surface (`plugin.json`, `index.ts`, `__init__.py`, etc.). @@ -305,11 +305,15 @@ Sensitive-path policy. See `start.md` for the full glob syntax and the three sup ### `code-review.json` (PLN-774) -Operator-tunable reviewer behavior. Currently exposes the BHA conditional-partitioning threshold; future knobs will land here too. +Operator-tunable reviewer behavior. | Key | Default | Behavior | |---|---|---| | `bha_unified_threshold_loc` | `5000` | PRs with total changed LOC at or below this value get a single "unified" BHA partition so cross-region invariants (declaration ↔ enforcement, definition ↔ reference) stay visible to one reviewer's context. PRs above the threshold fall back to the standard bin-pack (`REBALANCE_LOC_BUDGET=1200` LOC per partition). **Setting the value to `0` disables unified mode entirely (always-partition; restores pre-PLN-774 behavior — the regression escape hatch).** Invalid entries (wrong type, negative) silently fall back to the default. | +| `out_of_hunk_confidence_floor` | `0.80` | P2+ findings whose line falls outside the file's changed range survive validation when `confidence >` this floor. `1.0` is a kill switch (strict in-hunk only); `0.0` admits every out-of-hunk P2+. | +| `domain_critic_cap` | `3` | How many domain critics may spawn across the required and best-effort buckets combined. `source: "core"` reviewers (Design Critic, Impact Analyzer) are exempt. **Setting the value to `0` spawns no domain critics at all (kill switch).** Invalid entries (wrong type, negative) silently fall back to the default. Also overridable per-run with `arbitrate-budget --domain-critic-cap`. | + +**When to raise `domain_critic_cap`.** The cap drops by `(priority asc, reviewer asc)`. A repo whose `critic-gates.json` still uses the legacy `moduleCritics[]` schema gets every entry migrated as `required: False` with **no** `priority`, so all of them sit at the default `2` and the tiebreak degenerates to **alphabetical by reviewer name** — systematically favoring early names over relevance, and cutting the very critic the coverage critic proposed *for* the diff. Raising the cap is the blunt remedy; the precise one is migrating the relevant rules to the canonical `coverage[]` schema, which supports explicit `priority` and `required` (see `_migrate_module_critics` for what the legacy form forces). The chosen mode + count surface in `partitions.json` (`partition_mode`, `partition_count`, `total_changed_loc`, `unified_threshold_loc`), propagate into `verify_manifest.json`, and render in both presenters (local-mode Verifier Stats footer and GitHub Step 6e). Under partitioned mode, `stats.verification.by_reviewer` splits BHA findings per partition (`bha_p0`, `bha_p1`, …) so an over-rejecting partition surfaces in the FP-rate column. diff --git a/plugins/code-review/commands/start.md b/plugins/code-review/commands/start.md index e32a935..020fb63 100644 --- a/plugins/code-review/commands/start.md +++ b/plugins/code-review/commands/start.md @@ -32,7 +32,7 @@ Run a multi-agent code review with partitioned deep review, deterministic hygien The `--depth` flag selects which reviewer fleet runs. Default `standard`. Bare `/start` invocations preserve historical behavior. - **shallow** — hygiene + BHA (partitioned at >5000 LOC) + BHB + unified_auditor + verifier. Skips signal extraction, coverage planning/critic, and all `critic-gates.json` entries. Static spawn spec; no routing/critic decisions. Hygiene emits a `tier_mismatch_nudge` MEDIUM finding (category `Coverage`) when the PR's diff size, schema/migration paths, or public API surface suggest standard would catch more. -- **standard** — current behavior. Full fleet with signal-driven routing, coverage critic, repo-specific critic activation via `critic-gates.json`. Budget arithmetic reserves BHA partitions FIRST (Phase 4) and caps total domain critics across both required and best-effort buckets at the tier-uniform `DOMAIN_CRITIC_CAP = 3` (standard and deep alike). Required critics dropped by the cap emit coverage-gap findings. +- **standard** — current behavior. Full fleet with signal-driven routing, coverage critic, repo-specific critic activation via `critic-gates.json`. Budget arithmetic reserves BHA partitions FIRST (Phase 4) and caps total domain critics across both required and best-effort buckets at the tier-uniform `DOMAIN_CRITIC_CAP` (default 3, standard and deep alike; operator-tunable via `.closedloop-ai/settings/code-review.json:domain_critic_cap`, or per-run with `arbitrate-budget --domain-critic-cap`). Required critics dropped by the cap emit coverage-gap findings. - **deep** — standard plus two deep-only conditional core reviewers. The always-on **Design Critic** runs on every deep review (no trigger): a software-design craftsmanship reviewer covering module depth/information hiding, SOLID, dependency direction and layer boundaries, and project structure (drawing on *A Philosophy of Software Design*, SOLID, and *Clean Architecture*); it is `source: "core"` so it is exempt from `DOMAIN_CRITIC_CAP`, runs on Sonnet, is graph-aware (queries the `codebase-memory-mcp` knowledge graph via `get_architecture`/`query_graph` when the repo is indexed, else grep), and emits `category: "Code Quality"` findings scoped to design flaws this change introduces or worsens. The signal-gated **Impact Analyzer** (FEA-1401), a cross-file blast-radius reviewer, runs when signal extraction detects `exported_symbol_change` or `symbol_deletion`. The analyzer identifies changed exported symbols, finds external usages outside the diff (via the `codebase-memory-mcp` knowledge graph when the repo is indexed, else grep), and emits findings with `external_impact[]` listing every callsite that breaks under the new signature. Cost-capped at 30 symbols × 50 callsites with a 5-minute wall budget; deferred symbols surface in the Coverage Plan footer. Impact findings carry `category: "ImpactAnalysis"` and are verifier-audited per-entry (cited callsites read, snippet content-matched, grep replayed). ≥2 verified BLOCKING/HIGH Impact findings escalate the verdict to `NEEDS_ATTENTION` (Rule 6). Deep's extra breadth comes from these two reviewers rather than a wider domain-critic cap. Tier transitions are detected via `review_state.json`: a cached `shallow` review does not satisfy a subsequent `standard` invocation — the deeper run actually executes the previously skipped reviewers. @@ -346,7 +346,7 @@ These conditions mirror the cache-update gate (Gate C) and the pre-Phase-4b "Rev ## Operator Settings -Two optional operator-authored config files live under `.closedloop-ai/settings/`. Both are absent by default; the pipeline uses built-in defaults until they exist. +Three optional operator-authored config files live under `.closedloop-ai/settings/`. All are absent by default; the pipeline uses built-in defaults until they exist. ### `.closedloop-ai/settings/verdict-thresholds.json` (FEA-1401) diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index 84e9431..6defb6a 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -3064,6 +3064,16 @@ def _load_code_review_settings(path: Path | None) -> dict[str, Any]: legitimate companion-change findings). Default :data:`OUT_OF_HUNK_CONFIDENCE_FLOOR` (0.80). Setting to 1.0 is a kill switch (strict "in-hunk only" behavior). + - ``domain_critic_cap`` (int, ≥ 0): how many domain critics may + spawn across the required and best-effort buckets combined. + Default :data:`DOMAIN_CRITIC_CAP` (3). Setting the value to 0 is a + kill switch (no domain critic spawns; ``source: "core"`` + reviewers stay exempt). Raise it when a repo's + ``critic-gates.json`` legitimately resolves more relevant critics + than the default allows: the cap drops by ``(priority asc, + reviewer asc)``, so on a roster where every entry sits at the + default ``priority: 2`` the tiebreak is alphabetical, which + favors early reviewer names over relevance to the diff. Unknown keys are ignored. Invalid entries (wrong type, out of range) fall back to the default — the file is operator-authored and should @@ -3072,6 +3082,7 @@ def _load_code_review_settings(path: Path | None) -> dict[str, Any]: defaults: dict[str, Any] = { "bha_unified_threshold_loc": BHA_UNIFIED_THRESHOLD_LOC, "out_of_hunk_confidence_floor": OUT_OF_HUNK_CONFIDENCE_FLOOR, + "domain_critic_cap": DOMAIN_CRITIC_CAP, } data, out = _load_optional_settings_dict(path, defaults) if data is None: @@ -3092,6 +3103,15 @@ def _load_code_review_settings(path: Path | None) -> dict[str, Any]: and 0.0 <= float(raw_floor) <= 1.0 ): out["out_of_hunk_confidence_floor"] = float(raw_floor) + raw_critic_cap = data.get("domain_critic_cap") + # Reject bool (an int subclass) so a stray `true` doesn't become 1. + # 0 is valid and meaningful: it is the no-domain-critics kill switch. + if ( + isinstance(raw_critic_cap, int) + and not isinstance(raw_critic_cap, bool) + and raw_critic_cap >= 0 + ): + out["domain_critic_cap"] = raw_critic_cap return out @@ -11550,7 +11570,7 @@ def cmd_arbitrate_budget(args: argparse.Namespace) -> int: # ``--depth`` is still validated (shared stage-arg hygiene; an invalid # tier should fail loud), but the per-source domain-critic cap is now - # tier-uniform — standard and deep both cap at DOMAIN_CRITIC_CAP. Deep's + # tier-uniform — standard and deep both cap at the same value. Deep's # extra breadth comes from the always-on conditional core reviewers # (Design Critic, Impact Analyzer), which are exempt from this cap. depth: str | None = getattr(args, "depth", None) or None @@ -11558,7 +11578,38 @@ def cmd_arbitrate_budget(args: argparse.Namespace) -> int: if not ok: print(err, file=sys.stderr) return 1 - critic_cap = DOMAIN_CRITIC_CAP + + # Domain-critic cap precedence, mirroring bha_unified_threshold_loc: + # 1. ``--domain-critic-cap`` — explicit namespace override (tests, + # one-off runs). + # 2. ``.closedloop-ai/settings/code-review.json`` → + # ``domain_critic_cap`` — operator-tunable settings file. + # 3. :data:`DOMAIN_CRITIC_CAP` (3) — built-in default. + # + # Operator-tunable because the cap drops by (priority asc, reviewer + # asc): a repo whose critic-gates.json uses the legacy moduleCritics[] + # schema gets every entry at the default priority 2, so the tiebreak + # degenerates to alphabetical and cuts the critic the coverage critic + # proposed FOR the diff. Raising the cap is the blunt fix; assigning + # explicit priorities on canonical coverage[] rules is the precise one. + cap_override = getattr(args, "domain_critic_cap", None) + if cap_override is not None: + critic_cap = int(cap_override) + else: + cap_settings_path = Path( + getattr(args, "settings", None) or _CODE_REVIEW_SETTINGS_DEFAULT_PATH, + ) + critic_cap = int( + _load_code_review_settings(cap_settings_path).get( + "domain_critic_cap", DOMAIN_CRITIC_CAP, + ), + ) + if critic_cap < 0: + print( + f"Error: domain_critic_cap must be >= 0, got {critic_cap}", + file=sys.stderr, + ) + return 1 def _persist_plan(plan: dict[str, Any]) -> int: try: diff --git a/plugins/code-review/tools/python/config/cli.json b/plugins/code-review/tools/python/config/cli.json index db7bf3e..c36f6f1 100644 --- a/plugins/code-review/tools/python/config/cli.json +++ b/plugins/code-review/tools/python/config/cli.json @@ -408,7 +408,11 @@ "flags": [ "--depth" ], - "choices": ["shallow", "standard", "deep"], + "choices": [ + "shallow", + "standard", + "deep" + ], "help": "Invocation tier (FEA-1401). Gates COVERAGE_CORE_CONDITIONAL reviewers like the Impact Analyzer." } ] @@ -1437,6 +1441,20 @@ "deep" ], "help": "Invocation tier; validated for hygiene. The per-source domain-critic cap is tier-uniform; deep's extra breadth comes from the always-on conditional core reviewers (Design Critic, Impact Analyzer)" + }, + { + "flags": [ + "--settings" + ], + "default": null, + "help": "Path to code-review.json operator settings (default .closedloop-ai/settings/code-review.json); overridable for test isolation" + }, + { + "flags": [ + "--domain-critic-cap" + ], + "type": "int", + "help": "Override the domain-critic cap (>=0; 0 spawns no domain critics). Takes precedence over code-review.json domain_critic_cap and the built-in default" } ] }, @@ -1463,7 +1481,7 @@ }, { "name": "derive-static-spec", - "help": "Emit a static spawn.json.spec for shallow-tier runs (BHA × N partitions + BHB + unified_auditor); arbitrate_status=\"static\"", + "help": "Emit a static spawn.json.spec for shallow-tier runs (BHA \u00d7 N partitions + BHB + unified_auditor); arbitrate_status=\"static\"", "func": "cmd_derive_static_spec", "args": [ { @@ -1727,4 +1745,4 @@ ] } ] -} \ No newline at end of file +} diff --git a/plugins/code-review/tools/python/fixtures/run_plan_snapshots/cli_parser_resolved.json b/plugins/code-review/tools/python/fixtures/run_plan_snapshots/cli_parser_resolved.json index a3c5669..ba40932 100644 --- a/plugins/code-review/tools/python/fixtures/run_plan_snapshots/cli_parser_resolved.json +++ b/plugins/code-review/tools/python/fixtures/run_plan_snapshots/cli_parser_resolved.json @@ -1554,6 +1554,21 @@ "standard", "deep" ] + }, + { + "flags": [ + "--settings" + ], + "dest": "settings", + "default": null + }, + { + "flags": [ + "--domain-critic-cap" + ], + "dest": "domain_critic_cap", + "default": null, + "type": "int" } ] }, 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 df6bbc8..8a7b24e 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -8126,6 +8126,8 @@ def _run_arbitrate_budget( depth: str | None = None, verify_doc: dict[str, Any] | None = None, include_verify_flag: bool = True, # noqa: ARG001 - retained for caller signature + settings: dict[str, Any] | None = None, + domain_critic_cap: int | None = None, ) -> tuple[dict[str, Any], dict[str, Any], dict[str, Any]]: """Shared module-level driver for ``cmd_arbitrate_budget`` tests. @@ -8162,6 +8164,14 @@ def _run_arbitrate_budget( if verify_doc is not None: _write_coverage_section(tmp_path, "verify", verify_doc) + # ISS-5122: always point --settings at a test-local path so a + # code-review.json in the CWD can never leak into a test run. When + # ``settings`` is None the file simply does not exist and the loader + # returns built-in defaults. + settings_path = tmp_path / "code-review.json" + if settings is not None: + settings_path.write_text(json.dumps(settings)) + old_stdout = _sys.stdout _sys.stdout = io.StringIO() try: @@ -8170,6 +8180,8 @@ def _run_arbitrate_budget( diff_data=str(dd_path), cap=cap, depth=depth, + settings=str(settings_path), + domain_critic_cap=domain_critic_cap, ) cmd_arbitrate_budget(ns) _sys.stdout.seek(0) @@ -8319,6 +8331,78 @@ def test_absent_depth_uses_uniform_cap(self, tmp_path: Path) -> None: ) assert plan["budget"]["domain_critic_cap"] == 3 + def test_settings_file_raises_the_domain_critic_cap( + self, tmp_path: Path, + ) -> None: + """ISS-5122: ``domain_critic_cap`` in code-review.json overrides + the built-in 3. Without this a repo on the legacy + moduleCritics[] schema is stuck at 3 critics selected + ALPHABETICALLY — every migrated entry lands at the default + priority 2, so ``(priority asc, reviewer asc)`` degenerates to + the reviewer name and cuts the critic the coverage critic + proposed FOR the diff. + """ + diff = _make_diff_data(files=["src/app.ts"]) + _, plan, gaps = _run_arbitrate_budget( + tmp_path, self._critic_plan(5), diff, cap=20, depth="standard", + settings={"domain_critic_cap": 5}, + ) + selected = [r for r in plan["required"] if r.get("source") == "rule"] + assert len(selected) == 5 + assert plan["budget"]["domain_critic_cap"] == 5 + assert plan["budget"]["domain_critic_cap_fired"] is False + assert gaps["findings"] == [] + + def test_namespace_override_beats_settings_file( + self, tmp_path: Path, + ) -> None: + """Precedence: --domain-critic-cap > code-review.json > default. + Pinned so a future refactor cannot silently invert it.""" + diff = _make_diff_data(files=["src/app.ts"]) + _, plan, _ = _run_arbitrate_budget( + tmp_path, self._critic_plan(5), diff, cap=20, depth="standard", + settings={"domain_critic_cap": 5}, + domain_critic_cap=4, + ) + selected = [r for r in plan["required"] if r.get("source") == "rule"] + assert len(selected) == 4 + assert plan["budget"]["domain_critic_cap"] == 4 + + def test_settings_cap_zero_spawns_no_domain_critics( + self, tmp_path: Path, + ) -> None: + """0 is the documented kill switch: every domain critic defers + with the domain_critic_cap reason, and none surface as coverage + gaps (the cap is a budget decision, not missing coverage).""" + diff = _make_diff_data(files=["src/app.ts"]) + _, plan, gaps = _run_arbitrate_budget( + tmp_path, self._critic_plan(5), diff, cap=20, depth="standard", + settings={"domain_critic_cap": 0}, + ) + selected = [r for r in plan["required"] if r.get("source") == "rule"] + assert len(selected) == 0 + assert plan["budget"]["domain_critic_cap"] == 0 + capped = [ + e for e in plan["deferred_for_budget"] + if e.get("defer_reason") == "domain_critic_cap" + ] + assert len(capped) == 5 + assert gaps["findings"] == [] + + def test_malformed_settings_cap_falls_back_to_default( + self, tmp_path: Path, + ) -> None: + """An operator typo must not change the fleet: a non-int value + falls back to the built-in 3 rather than crashing the stage.""" + diff = _make_diff_data(files=["src/app.ts"]) + _, plan, _ = _run_arbitrate_budget( + tmp_path, self._critic_plan(5), diff, cap=20, depth="standard", + settings={"domain_critic_cap": "lots"}, + ) + selected = [r for r in plan["required"] if r.get("source") == "rule"] + assert len(selected) == 3 + assert plan["budget"]["domain_critic_cap"] == 3 + def test_invalid_cap_returns_error(self, tmp_path: Path) -> None: diff = _make_diff_data(files=["src/app.ts"]) plan_in = {"required": [], "best_effort": []} @@ -11900,17 +11984,20 @@ class TestLoadCodeReviewSettings: def test_missing_file_returns_defaults(self, tmp_path: Path) -> None: from code_review_helpers import ( BHA_UNIFIED_THRESHOLD_LOC, + DOMAIN_CRITIC_CAP, OUT_OF_HUNK_CONFIDENCE_FLOOR, _load_code_review_settings, ) out = _load_code_review_settings(tmp_path / "does-not-exist.json") # v2.21.0 added out_of_hunk_confidence_floor to the canonical - # defaults; the loader must surface ALL canonical keys so - # callers can read with .get(key, default) safely without - # having to know which keys were added in which version. + # defaults; ISS-5122 added domain_critic_cap. The loader must + # surface ALL canonical keys so callers can read with + # .get(key, default) safely without having to know which keys + # were added in which version. assert out == { "bha_unified_threshold_loc": BHA_UNIFIED_THRESHOLD_LOC, "out_of_hunk_confidence_floor": OUT_OF_HUNK_CONFIDENCE_FLOOR, + "domain_critic_cap": DOMAIN_CRITIC_CAP, } def test_operator_override_honored(self, tmp_path: Path) -> None: @@ -11920,6 +12007,68 @@ def test_operator_override_honored(self, tmp_path: Path) -> None: out = _load_code_review_settings(path) assert out["bha_unified_threshold_loc"] == 3500 + def test_domain_critic_cap_override_honored( + self, tmp_path: Path, + ) -> None: + """ISS-5122: the domain-critic cap is operator-tunable. Without + this a repo whose critic-gates.json uses the legacy + moduleCritics[] schema is stuck at 3 critics chosen + ALPHABETICALLY (every migrated entry lands at the default + priority 2), which cuts the critic the coverage critic proposed + for the diff.""" + from code_review_helpers import _load_code_review_settings + path = tmp_path / "code-review.json" + path.write_text(json.dumps({"domain_critic_cap": 6})) + out = _load_code_review_settings(path) + assert out["domain_critic_cap"] == 6 + + def test_domain_critic_cap_zero_is_valid_kill_switch( + self, tmp_path: Path, + ) -> None: + """0 means "spawn no domain critics" and must NOT fall back to + the default, mirroring bha_unified_threshold_loc's 0 semantics. + Core reviewers are source:"core" and stay exempt.""" + from code_review_helpers import _load_code_review_settings + path = tmp_path / "code-review.json" + path.write_text(json.dumps({"domain_critic_cap": 0})) + out = _load_code_review_settings(path) + assert out["domain_critic_cap"] == 0 + + def test_domain_critic_cap_negative_falls_back( + self, tmp_path: Path, + ) -> None: + from code_review_helpers import ( + DOMAIN_CRITIC_CAP, _load_code_review_settings, + ) + path = tmp_path / "code-review.json" + path.write_text(json.dumps({"domain_critic_cap": -1})) + out = _load_code_review_settings(path) + assert out["domain_critic_cap"] == DOMAIN_CRITIC_CAP + + def test_domain_critic_cap_wrong_type_falls_back( + self, tmp_path: Path, + ) -> None: + from code_review_helpers import ( + DOMAIN_CRITIC_CAP, _load_code_review_settings, + ) + path = tmp_path / "code-review.json" + path.write_text(json.dumps({"domain_critic_cap": "6"})) + out = _load_code_review_settings(path) + assert out["domain_critic_cap"] == DOMAIN_CRITIC_CAP + + def test_domain_critic_cap_bool_rejected_as_int( + self, tmp_path: Path, + ) -> None: + """``True`` is int(1) in Python; a stray JSON `true` must not + quietly become a cap of 1.""" + from code_review_helpers import ( + DOMAIN_CRITIC_CAP, _load_code_review_settings, + ) + path = tmp_path / "code-review.json" + path.write_text(json.dumps({"domain_critic_cap": True})) + out = _load_code_review_settings(path) + assert out["domain_critic_cap"] == DOMAIN_CRITIC_CAP + def test_zero_is_valid_kill_switch(self, tmp_path: Path) -> None: """``0`` is a meaningful operator value (always-partition); the validator must NOT silently fall back to the default on 0."""