diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b08daf0..af179731 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ 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.14.0 + +#### Added +- `.closedloop-ai/settings/code-review.json` accepts `domain_critic_cap`, the number of domain critics that may spawn across the required and best-effort buckets combined (default 3; `source: "core"` reviewers stay exempt). `0` is a kill switch; invalid entries fall back to the default. `arbitrate-budget --domain-critic-cap` overrides it per run, and takes precedence over the settings file. + +#### Fixed +- On a PR review, `domain_critic_cap` is read from the settings file as committed on `origin/` rather than from the checked-out head. The reviewed tree is contributor-authored, so a head-only copy could set the budget deciding which reviewers inspect that same diff — `domain_critic_cap: 0` suppressed every rule and critic reviewer, and because cap drops emit no coverage gap, nothing downstream reported the absent coverage. A cap the operator committed on the base branch still applies; one present only on the head is ignored, and off a PR the working tree is read as before. `bha_unified_threshold_loc` and `out_of_hunk_confidence_floor` are still read from the working tree on every run. +- `domain_critic_cap` is clamped to the budget headroom left after BHA partitions and the non-critic required core are reserved. Above that headroom the surplus critics were selected and then dropped as `budget_exceeded` coverage gaps carrying `required: True`, which canonical verdict Rule 1 turns into CHANGES_REQUESTED — so raising the cap flipped the verdict on a run that would otherwise have been APPROVED. The surplus now defers under `defer_reason: "domain_critic_cap"`, and `budget.domain_critic_cap` reports the cap that actually governed selection. +- `README.md` and `commands/start.md` no longer state that required critics dropped by the domain-critic cap emit coverage-gap findings. They have not since cap drops began reporting only through `deferred_for_budget`, so an operator lowering the cap was pointed at a safety signal that never appears. Both now also record the cap's effective ceiling and the base-ref trust anchor. + ### code-review v3.13.0 #### Fixed diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index 1ea3741e..734c4061 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.13.0", + "version": "3.14.0", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code-review/.codex-plugin/plugin.json b/plugins/code-review/.codex-plugin/plugin.json index be52f0f2..939377ce 100644 --- a/plugins/code-review/.codex-plugin/plugin.json +++ b/plugins/code-review/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "code-review", - "version": "3.13.0", + "version": "3.14.0", "description": "Code review plugin for automated review workflows and inline GitHub PR comments.", "author": { "name": "ClosedLoop", diff --git a/plugins/code-review/README.md b/plugins/code-review/README.md index 9159bbf6..2d972e0f 100644 --- a/plugins/code-review/README.md +++ b/plugins/code-review/README.md @@ -194,7 +194,7 @@ Three tiers select which reviewer fleet runs: | `design_critic` (always-on at deep) | ✗ | ✗ | ✓ | | `impact_analyzer` (FEA-1401, on exported-symbol change/deletion or a deleted file) | ✗ | ✗ | ✓ (on signal or file deletion) | -**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. Cap-deferred entries — from either bucket — carry `defer_reason: "domain_critic_cap"` in `deferred_for_budget` and do **not** emit coverage-gap findings: a required gap carries `required: True`, which `_compute_canonical_verdict` Rule 1 turns into CHANGES_REQUESTED, so emitting one would block every repo whose `critic-gates.json` resolves more required critics than the cap. Lowering the cap therefore raises no safety signal — `deferred_for_budget` is the only record of what it dropped. 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.). @@ -310,11 +310,19 @@ 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 useful range tops out at the total `--cap` headroom.** The cap is a soft budget inside `--cap` (default 20), which reserves BHA partitions and the non-critic required core first, so the most critics that can actually spawn is roughly `--cap − bha_partitions − required core`. A value above that is clamped to the headroom rather than honored: the surplus defers under `defer_reason: "domain_critic_cap"`, and `budget.domain_critic_cap` in `coverage.json.final` reads back as the clamped value, not the one requested. Note that `--cap` itself is not exposed in this settings file, so raising the headroom is a per-run flag, not a setting. + +**On a PR review this setting is read from `origin/`, not from the checked-out head.** The tree under review is contributor-authored, so honoring a head copy would let the author of a diff set the budget that decides which reviewers inspect it — `domain_critic_cap: 0` suppresses every rule/critic reviewer, and because cap drops emit no coverage gap (see above) nothing downstream reports the missing coverage. A value the operator has committed on the base branch still applies; one that exists only on the head is ignored. Off a PR the working tree is the operator's own checkout and is read directly. The same anchoring is **not** yet applied to `bha_unified_threshold_loc` or `out_of_hunk_confidence_floor`, which are still read from the working tree on every run. 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 c1f8ddde..aeab86d2 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`). Critics dropped by the cap land in `deferred_for_budget` with `defer_reason: "domain_critic_cap"` and emit no coverage-gap finding, so lowering the cap is silent by design. On a PR review the cap is read from `origin/`, not the checked-out head, so the diff under review cannot set the budget that decides which reviewers inspect it. - **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 code-intelligence-aware (uses whichever indexing MCP server the operator's session provides for module layout and dependency direction, 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`, or when the diff deletes a file outright (`file_deletion`; at standard and shallow depth Bug Hunter B and the fast-path reviewer own deletions). The analyzer identifies changed exported symbols, finds external usages outside the diff (via whichever indexing MCP server the operator's session provides, 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. @@ -352,7 +352,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 ebf4cd2e..9ada5521 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -3833,17 +3833,38 @@ 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 not crash the pipeline on a typo. """ + return _code_review_settings_from_data(_read_optional_json(path, None) if path else None) + + +def _code_review_settings_from_data(data: Any) -> dict[str, Any]: + """Validate an already-read ``code-review.json`` object. + + Split from :func:`_load_code_review_settings` so a blob read from a git + ref (:func:`_load_code_review_settings_at_ref`) gets the identical + per-key validation rather than a second, driftable copy of it. + """ 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: + _, out = _load_optional_settings_dict(None, defaults) + if not isinstance(data, dict): return out raw_threshold = data.get("bha_unified_threshold_loc") if ( @@ -3861,9 +3882,57 @@ 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 +def _trusted_settings_ref(cr_dir: Path) -> str | None: + """Return the git ref whose ``code-review.json`` blob is trusted, or None. + + The plugin is trusted; the tree it reviews is not. On a PR review the + working tree is contributor-authored content, so a settings file + committed on the head may have been written by the author of the very + diff under review. ``origin/`` is the merge target — already + reviewed and operator-controlled — so it is the trust anchor for any + setting that steers the reviewer fleet. + + None means "no PR under review": a local run's working tree IS the + operator's own checkout, so there is nothing to anchor against. + """ + scope = _read_optional_json(cr_dir / "scope.json", {}) + if not isinstance(scope, dict) or not scope.get("pr_number"): + return None + base_ref = str(scope.get("base_ref") or "") + return f"origin/{base_ref}" if base_ref else None + + +def _load_code_review_settings_at_ref(ref: str, rel_path: Path) -> dict[str, Any]: + """Read ``code-review.json`` from a git ref. Unreadable → built-in defaults. + + Falls back to the built-in defaults rather than the working-tree file: + a missing blob on the base ref means the operator never committed the + setting, and reading the head copy instead is the exposure + :func:`_trusted_settings_ref` exists to close. + """ + try: + blob = _run_git(["show", f"{ref}:{rel_path.as_posix()}"]) + except (subprocess.CalledProcessError, OSError): + return _code_review_settings_from_data(None) + try: + data = json.loads(blob) + except json.JSONDecodeError: + return _code_review_settings_from_data(None) + return _code_review_settings_from_data(data if isinstance(data, dict) else None) + + def _load_verification_gates(path: Path | None) -> dict[str, list[str]]: """Read verification-gates.json. Absent or malformed → empty gates. @@ -12893,7 +12962,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 @@ -12901,7 +12970,51 @@ 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. The cap decides which reviewers run, so + # unlike bha_unified_threshold_loc its source has to be trustworthy: + # 1. ``--domain-critic-cap`` — passed on the stage's own command line + # by the plugin (tests, one-off operator runs), never by the tree + # under review. + # 2. An explicit ``--settings`` path — same provenance as (1). + # 3. On a PR review, ``domain_critic_cap`` as committed on + # ``origin/``. The head tree is contributor-authored: + # honoring a settings file from it would let the author of the + # diff under review set ``domain_critic_cap: 0`` and silently + # suppress every rule/critic reviewer (cap drops emit no + # coverage gap), or raise it to amplify review cost. + # 4. Off a PR, the working-tree + # ``.closedloop-ai/settings/code-review.json`` — a local run's + # checkout is the operator's own. + # 5. :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) + settings_arg = getattr(args, "settings", None) + if cap_override is not None: + critic_cap = int(cap_override) + else: + trusted_ref = None if settings_arg else _trusted_settings_ref(cr_dir) + if trusted_ref is not None: + cap_settings = _load_code_review_settings_at_ref( + trusted_ref, _CODE_REVIEW_SETTINGS_DEFAULT_PATH, + ) + else: + cap_settings = _load_code_review_settings( + Path(settings_arg or _CODE_REVIEW_SETTINGS_DEFAULT_PATH), + ) + critic_cap = int(cap_settings.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: @@ -13061,31 +13174,41 @@ def _plan_target_str() -> str: # required domain critics on a single PR. The BLOCKING branch already does # the right thing (deferred, no gap); this matches that behavior on # the PASS path. + # The cap is a soft budget, so it cannot exceed the capacity actually + # left after the reservations that outrank it (BHA partitions and the + # non-critic required core). Past that headroom the surplus critics + # would be selected here and then dropped by Step 3 below as + # ``budget_exceeded`` coverage gaps carrying ``required: True`` — + # which ``_compute_canonical_verdict`` Rule 1 turns into + # CHANGES_REQUESTED. Measured on the 3-critic default that was + # unreachable; once the cap became operator-tunable, raising it past + # the headroom silently flipped a run that would have been APPROVED. + # Clamping keeps the surplus in ``deferred_for_budget`` under + # ``defer_reason: "domain_critic_cap"``, which is how every other cap + # drop already reports itself. + critic_cap_headroom = max(0, cap - bha_target - len(required_non_critic)) + effective_critic_cap = min(critic_cap, critic_cap_headroom) sel_req_critics, sel_be_critics, def_req_critics, def_be_critics = ( _select_domain_critics( - required_critics_in, best_effort_critics_in, critic_cap, + required_critics_in, best_effort_critics_in, effective_critic_cap, ) ) # PLN-807 Phase 4 Step 3: required overflow against the reduced - # available_for_critics. Required-bucket critics already capped at - # DOMAIN_CRITIC_CAP, so this branch normally doesn't fire — but it - # remains the safety net for the cap=tiny edge case (a smaller - # ``--cap`` override could leave less than required_non_critic + - # bha_target slots). Drop selected critics FIRST, then non-critic - # required (which includes core reviewers — operators shouldn't - # lose BHB/auditor to budget overflow before they lose a - # non-essential critic). + # available_for_critics. The clamp above leaves no room for a critic + # to overflow, so only the non-critic tail can land here, and only + # when ``--cap`` is smaller than bha_target + the required core + # itself. Core reviewers are dropped last on purpose: operators + # shouldn't lose BHB/auditor to budget overflow. required = required_non_critic + sel_req_critics required_overflow = len(required) + bha_target - cap if required_overflow > 0: - # Drop selected required critics first. - drop_critics_n = min(required_overflow, len(sel_req_critics)) - if drop_critics_n: - dropped_critics = sel_req_critics[-drop_critics_n:] - sel_req_critics = sel_req_critics[:-drop_critics_n] + drop_core_n = min(required_overflow, len(required_non_critic)) + if drop_core_n: + dropped_core = required_non_critic[-drop_core_n:] + required_non_critic = required_non_critic[:-drop_core_n] for idx, entry in enumerate( - dropped_critics, start=len(coverage_gaps), + dropped_core, start=len(coverage_gaps), ): coverage_gaps.append( _make_coverage_gap_finding( @@ -13095,26 +13218,7 @@ def _plan_target_str() -> str: emitted_at=now_iso, ), ) - dropped_required.extend(dropped_critics) - required_overflow -= drop_critics_n - # If still over, drop non-critic required tail (rare). - if required_overflow > 0: - drop_core_n = min(required_overflow, len(required_non_critic)) - if drop_core_n: - dropped_core = required_non_critic[-drop_core_n:] - required_non_critic = required_non_critic[:-drop_core_n] - for idx, entry in enumerate( - dropped_core, start=len(coverage_gaps), - ): - coverage_gaps.append( - _make_coverage_gap_finding( - entry, - reason="budget_exceeded", - index=idx, - emitted_at=now_iso, - ), - ) - dropped_required.extend(dropped_core) + dropped_required.extend(dropped_core) required = required_non_critic + sel_req_critics # PLN-807 Phase 4 Step 4: best-effort prune against remaining cap. @@ -13178,7 +13282,10 @@ def _plan_target_str() -> str: "required_count": len(required), "best_effort_count": len(best_effort_final), "bha_partitions": bha_partitions, - "domain_critic_cap": critic_cap, + # The cap that actually governed selection, i.e. post-clamp. + # A requested cap above the headroom reads back as the + # headroom so the plan doesn't claim a budget it never had. + "domain_critic_cap": effective_critic_cap, "domain_critic_cap_fired": bool(def_req_critics or def_be_critics), }, "dropped_required": dropped_required, diff --git a/plugins/code-review/tools/python/config/cli.json b/plugins/code-review/tools/python/config/cli.json index 4be3133b..aea571e1 100644 --- a/plugins/code-review/tools/python/config/cli.json +++ b/plugins/code-review/tools/python/config/cli.json @@ -1429,6 +1429,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" } ] }, 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 cdda5552..f3bc49bc 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 @@ -1545,6 +1545,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 5c4fa7e5..0aa61312 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -9313,6 +9313,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. @@ -9349,6 +9351,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: @@ -9357,6 +9367,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) @@ -9506,6 +9518,119 @@ 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_cap_above_headroom_does_not_drop_required_critics( + self, tmp_path: Path, + ) -> None: + """A cap raised past the budget headroom must defer the surplus, + never drop it as a ``budget_exceeded`` coverage gap. + + Those gaps carry ``required: True``, which + ``_compute_canonical_verdict`` Rule 1 turns into + CHANGES_REQUESTED — so before the clamp, raising the knob flipped + the verdict on a run that would otherwise have been APPROVED. + Measured: ``--cap 20`` with 25 required rule critics and + ``domain_critic_cap: 25`` produced 6 dropped_required and 6 + required gaps. + """ + from code_review_helpers import _compute_canonical_verdict + diff = _make_diff_data(files=["src/app.ts"]) + _, plan, gaps = _run_arbitrate_budget( + tmp_path, self._critic_plan(25), diff, cap=20, depth="standard", + settings={"domain_critic_cap": 25}, + ) + assert plan["dropped_required"] == [] + assert gaps["findings"] == [] + assert _compute_canonical_verdict([], gaps["findings"])[0] == "APPROVED" + # The clamp is now the only thing holding the cap to the budget, so + # pin the invariant the deleted overflow branch used to defend: + # selected required + reserved BHA never exceeds ``--cap``. Without + # it, removing the clamp trades the verdict flip for a silent + # over-allocation and the assertions above still pass. + bha = plan["budget"]["bha_partitions"] + assert len(plan["required"]) + bha <= 20 + # The surplus is still accounted for, under the cap's own reason. + capped = [ + e for e in plan["deferred_for_budget"] + if e.get("defer_reason") == "domain_critic_cap" + ] + headroom = 20 - bha # this plan has no non-critic required entries + assert len(capped) == 25 - headroom + # The plan reports the cap that actually governed selection, not + # the unreachable 25 the operator asked for. + assert plan["budget"]["domain_critic_cap"] == headroom + def test_invalid_cap_returns_error(self, tmp_path: Path) -> None: diff = _make_diff_data(files=["src/app.ts"]) plan_in = {"required": [], "best_effort": []} @@ -9527,6 +9652,180 @@ def test_invalid_cap_returns_error(self, tmp_path: Path) -> None: assert rc == 1 +class TestDomainCriticCapTrustBoundary: + """ISS-5122: where the domain-critic cap may legitimately come from. + + Every test here drives ``arbitrate-budget`` through the REAL argparse + wiring (``_register_subparsers`` → ``parse_args`` → ``args.func``) with + no ``--settings`` flag, because that is how the production stage runs + it. The sibling ``TestArbitrateBudget`` cases hand-build a namespace + and always pass ``--settings``, so they cannot see a regression in the + parser default or in the default-path resolution this class covers. + """ + + _CRITICS = 5 + _SETTINGS_REL = Path(".closedloop-ai/settings/code-review.json") + + def _seed( + self, repo: Path, *, scope: dict[str, Any] | None = None, + ) -> Path: + """Seed a cr_dir inside *repo* with ``self._CRITICS`` rule critics.""" + from code_review_helpers import _write_coverage_section + cr_dir = repo / ".cr" + cr_dir.mkdir(parents=True, exist_ok=True) + _write_coverage_section(cr_dir, "final", { + "required": [ + {"reviewer": f"domain-{i}", "priority": 1, "source": "rule"} + for i in range(self._CRITICS) + ], + "best_effort": [], + }) + dd = cr_dir / "diff_data.json" + dd.write_text(json.dumps(_make_diff_data(files=["src/app.ts"]))) + if scope is not None: + (cr_dir / "scope.json").write_text(json.dumps(scope)) + return cr_dir + + def _write_settings(self, repo: Path, cap: int) -> None: + path = repo / self._SETTINGS_REL + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps({"domain_critic_cap": cap})) + + def _run(self, cr_dir: Path) -> dict[str, Any]: + """Invoke the stage exactly as ``main()`` does, minus ``--settings``.""" + import io + import sys as _sys + + from code_review_helpers import _read_coverage_state, _register_subparsers + parser = argparse.ArgumentParser() + sub = parser.add_subparsers(dest="command", required=True) + _register_subparsers(sub) + args = parser.parse_args([ + "arbitrate-budget", + "--cr-dir", str(cr_dir), + "--diff-data", str(cr_dir / "diff_data.json"), + "--cap", "20", + "--depth", "standard", + ]) + # Proves the parser default is what feeds the default-path branch; + # if a future change makes --settings required or non-None by + # default, the production path stops being the one under test. + assert args.settings is None + assert args.domain_critic_cap is None + old = _sys.stdout + _sys.stdout = io.StringIO() + try: + assert args.func(args) == 0 + finally: + _sys.stdout = old + plan = _read_coverage_state(cr_dir).get("final") + assert isinstance(plan, dict) + return plan + + @staticmethod + def _selected(plan: dict[str, Any]) -> int: + return len([r for r in plan["required"] if r.get("source") == "rule"]) + + def _init_repo(self, repo: Path, *, base_cap: int | None) -> None: + """A repo whose ``origin/main`` is the trusted base of a PR branch. + + When *base_cap* is None, ``main`` carries no settings file at all — + the shape that matters most, since that is every repo that has + never tuned the cap. + """ + repo.mkdir(parents=True, exist_ok=True) + git_fixture(repo, "init", "--quiet", "-b", "main") + if base_cap is not None: + self._write_settings(repo, base_cap) + _commit_file(repo, "base.txt", "base\n") + git_fixture(repo, "update-ref", "refs/remotes/origin/main", "main") + git_fixture(repo, "checkout", "--quiet", "-b", "contributor-branch") + + # --- off a PR: the working tree is the operator's own checkout ------- + + def test_default_path_settings_raise_the_cap_on_a_local_run( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, + ) -> None: + """The operator-tunable knob reached through the production path. + + No ``--settings``, no PR: the stage must find + ``.closedloop-ai/settings/code-review.json`` by its own default + and honor it. + """ + monkeypatch.chdir(tmp_path) + self._write_settings(tmp_path, 5) + plan = self._run(self._seed(tmp_path)) + assert self._selected(plan) == 5 + assert plan["budget"]["domain_critic_cap"] == 5 + + def test_default_path_zero_cap_is_honored_on_a_local_run( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, + ) -> None: + """0 is the kill switch, and it must survive the default-path + resolution rather than being read as "absent, use 3".""" + monkeypatch.chdir(tmp_path) + self._write_settings(tmp_path, 0) + plan = self._run(self._seed(tmp_path)) + assert self._selected(plan) == 0 + assert plan["budget"]["domain_critic_cap"] == 0 + + # --- on a PR: the working tree is contributor-authored -------------- + + def test_head_only_settings_cannot_suppress_the_fleet( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, + ) -> None: + """THE trust-boundary case: a settings file that exists only on the + reviewed head must not change that PR's fleet. + + Otherwise the author of the diff under review commits + ``domain_critic_cap: 0`` alongside it and every rule/critic + reviewer silently stops spawning — silently because cap drops + deliberately emit no coverage gap, so nothing downstream reports + the missing coverage. + """ + repo = tmp_path / "repo" + self._init_repo(repo, base_cap=None) + monkeypatch.chdir(repo) + self._write_settings(repo, 0) # uncommitted: head-only + cr_dir = self._seed(repo, scope={"pr_number": 42, "base_ref": "main"}) + + # Guard the fixture: the file really is there to be read, so a pass + # below is the trust check and not a missing-file no-op. + assert (repo / self._SETTINGS_REL).exists() + + plan = self._run(cr_dir) + assert self._selected(plan) == 3 + assert plan["budget"]["domain_critic_cap"] == 3 + + def test_head_only_settings_cannot_amplify_the_fleet( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, + ) -> None: + """The same boundary in the other direction: a contributor must not + be able to inflate review cost on their own PR either.""" + repo = tmp_path / "repo" + self._init_repo(repo, base_cap=None) + monkeypatch.chdir(repo) + self._write_settings(repo, self._CRITICS) + cr_dir = self._seed(repo, scope={"pr_number": 42, "base_ref": "main"}) + plan = self._run(cr_dir) + assert self._selected(plan) == 3 + + def test_base_committed_settings_are_honored_on_a_pr( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, + ) -> None: + """The knob is not merely disabled under review: a cap the operator + committed on the base ref still applies, and still wins over the + head's copy of the same file.""" + repo = tmp_path / "repo" + self._init_repo(repo, base_cap=5) + monkeypatch.chdir(repo) + self._write_settings(repo, 0) # head tries to override to the kill switch + cr_dir = self._seed(repo, scope={"pr_number": 42, "base_ref": "main"}) + plan = self._run(cr_dir) + assert self._selected(plan) == 5 + assert plan["budget"]["domain_critic_cap"] == 5 + + class TestArbitrateBudgetVerdict: """End-to-end: arbitrate-budget → finalize-result → CHANGES_REQUESTED.""" @@ -14113,17 +14412,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: @@ -14133,6 +14435,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."""