Skip to content

Commit 28dabfd

Browse files
cchenault4claude
andcommitted
code-review: make DOMAIN_CRITIC_CAP operator-tunable
The domain-critic cap was a hardcoded 3 with no settings path, and it drops by (priority asc, reviewer asc). A repo whose critic-gates.json 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. That deterministically cuts the critic the coverage critic proposed FOR the diff: on two symphony-alpha cost/metric PRs, logical-metric-reconciliation-auditor was accepted by consolidate and then dropped, while api-architect, caching-strategist and database-architect survived on their first letter. Adds domain_critic_cap to code-review.json (the file that already hosts bha_unified_threshold_loc and out_of_hunk_confidence_floor), plus an --domain-critic-cap namespace override and --settings for test isolation on arbitrate-budget. Precedence mirrors bha_unified_threshold_loc: flag > settings file > built-in default. 0 is a valid kill switch (no domain critics; source:"core" reviewers stay exempt); negative and non-int values fall back to the default rather than crashing the stage. Raising the cap is the blunt remedy — the precise one is migrating rules to the canonical coverage[] schema, which supports explicit priority and required. Both are now documented in README and start.md. Tests: 9 new (5 loader validation, 4 arbitrate-budget behavior incl. precedence, the 0 kill switch, and malformed-value fallback). The shared _run_arbitrate_budget harness now always points --settings at a test-local path so a code-review.json in the CWD cannot leak into a run. Full suite 1339 passed / 3 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 5c6b6ab commit 28dabfd

6 files changed

Lines changed: 249 additions & 12 deletions

File tree

plugins/code-review/README.md

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ Three tiers select which reviewer fleet runs:
190190
| `design_critic` (always-on at deep) ||||
191191
| `impact_analyzer` (FEA-1401, on exported-symbol change/deletion) ||| ✓ (on signal) |
192192

193-
**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.
193+
**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.
194194

195195
**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.).
196196

@@ -305,11 +305,15 @@ Sensitive-path policy. See `start.md` for the full glob syntax and the three sup
305305

306306
### `code-review.json` (PLN-774)
307307

308-
Operator-tunable reviewer behavior. Currently exposes the BHA conditional-partitioning threshold; future knobs will land here too.
308+
Operator-tunable reviewer behavior.
309309

310310
| Key | Default | Behavior |
311311
|---|---|---|
312312
| `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. |
313+
| `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+. |
314+
| `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`. |
315+
316+
**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).
313317

314318
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.
315319

plugins/code-review/commands/start.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ Run a multi-agent code review with partitioned deep review, deterministic hygien
3232
The `--depth` flag selects which reviewer fleet runs. Default `standard`. Bare `/start` invocations preserve historical behavior.
3333

3434
- **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.
35-
- **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.
35+
- **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.
3636
- **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.
3737

3838
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
346346

347347
## Operator Settings
348348

349-
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.
349+
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.
350350

351351
### `.closedloop-ai/settings/verdict-thresholds.json` (FEA-1401)
352352

plugins/code-review/tools/python/code_review_helpers.py

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3064,6 +3064,16 @@ def _load_code_review_settings(path: Path | None) -> dict[str, Any]:
30643064
legitimate companion-change findings). Default
30653065
:data:`OUT_OF_HUNK_CONFIDENCE_FLOOR` (0.80). Setting to 1.0 is a
30663066
kill switch (strict "in-hunk only" behavior).
3067+
- ``domain_critic_cap`` (int, ≥ 0): how many domain critics may
3068+
spawn across the required and best-effort buckets combined.
3069+
Default :data:`DOMAIN_CRITIC_CAP` (3). Setting the value to 0 is a
3070+
kill switch (no domain critic spawns; ``source: "core"``
3071+
reviewers stay exempt). Raise it when a repo's
3072+
``critic-gates.json`` legitimately resolves more relevant critics
3073+
than the default allows: the cap drops by ``(priority asc,
3074+
reviewer asc)``, so on a roster where every entry sits at the
3075+
default ``priority: 2`` the tiebreak is alphabetical, which
3076+
favors early reviewer names over relevance to the diff.
30673077

30683078
Unknown keys are ignored. Invalid entries (wrong type, out of range)
30693079
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]:
30723082
defaults: dict[str, Any] = {
30733083
"bha_unified_threshold_loc": BHA_UNIFIED_THRESHOLD_LOC,
30743084
"out_of_hunk_confidence_floor": OUT_OF_HUNK_CONFIDENCE_FLOOR,
3085+
"domain_critic_cap": DOMAIN_CRITIC_CAP,
30753086
}
30763087
data, out = _load_optional_settings_dict(path, defaults)
30773088
if data is None:
@@ -3092,6 +3103,15 @@ def _load_code_review_settings(path: Path | None) -> dict[str, Any]:
30923103
and 0.0 <= float(raw_floor) <= 1.0
30933104
):
30943105
out["out_of_hunk_confidence_floor"] = float(raw_floor)
3106+
raw_critic_cap = data.get("domain_critic_cap")
3107+
# Reject bool (an int subclass) so a stray `true` doesn't become 1.
3108+
# 0 is valid and meaningful: it is the no-domain-critics kill switch.
3109+
if (
3110+
isinstance(raw_critic_cap, int)
3111+
and not isinstance(raw_critic_cap, bool)
3112+
and raw_critic_cap >= 0
3113+
):
3114+
out["domain_critic_cap"] = raw_critic_cap
30953115
return out
30963116

30973117

@@ -11550,15 +11570,46 @@ def cmd_arbitrate_budget(args: argparse.Namespace) -> int:
1155011570

1155111571
# ``--depth`` is still validated (shared stage-arg hygiene; an invalid
1155211572
# tier should fail loud), but the per-source domain-critic cap is now
11553-
# tier-uniform — standard and deep both cap at DOMAIN_CRITIC_CAP. Deep's
11573+
# tier-uniform — standard and deep both cap at the same value. Deep's
1155411574
# extra breadth comes from the always-on conditional core reviewers
1155511575
# (Design Critic, Impact Analyzer), which are exempt from this cap.
1155611576
depth: str | None = getattr(args, "depth", None) or None
1155711577
ok, err = _validate_invocation_depth(depth)
1155811578
if not ok:
1155911579
print(err, file=sys.stderr)
1156011580
return 1
11561-
critic_cap = DOMAIN_CRITIC_CAP
11581+
11582+
# Domain-critic cap precedence, mirroring bha_unified_threshold_loc:
11583+
# 1. ``--domain-critic-cap`` — explicit namespace override (tests,
11584+
# one-off runs).
11585+
# 2. ``.closedloop-ai/settings/code-review.json`` →
11586+
# ``domain_critic_cap`` — operator-tunable settings file.
11587+
# 3. :data:`DOMAIN_CRITIC_CAP` (3) — built-in default.
11588+
#
11589+
# Operator-tunable because the cap drops by (priority asc, reviewer
11590+
# asc): a repo whose critic-gates.json uses the legacy moduleCritics[]
11591+
# schema gets every entry at the default priority 2, so the tiebreak
11592+
# degenerates to alphabetical and cuts the critic the coverage critic
11593+
# proposed FOR the diff. Raising the cap is the blunt fix; assigning
11594+
# explicit priorities on canonical coverage[] rules is the precise one.
11595+
cap_override = getattr(args, "domain_critic_cap", None)
11596+
if cap_override is not None:
11597+
critic_cap = int(cap_override)
11598+
else:
11599+
cap_settings_path = Path(
11600+
getattr(args, "settings", None) or _CODE_REVIEW_SETTINGS_DEFAULT_PATH,
11601+
)
11602+
critic_cap = int(
11603+
_load_code_review_settings(cap_settings_path).get(
11604+
"domain_critic_cap", DOMAIN_CRITIC_CAP,
11605+
),
11606+
)
11607+
if critic_cap < 0:
11608+
print(
11609+
f"Error: domain_critic_cap must be >= 0, got {critic_cap}",
11610+
file=sys.stderr,
11611+
)
11612+
return 1
1156211613

1156311614
def _persist_plan(plan: dict[str, Any]) -> int:
1156411615
try:

plugins/code-review/tools/python/config/cli.json

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -408,7 +408,11 @@
408408
"flags": [
409409
"--depth"
410410
],
411-
"choices": ["shallow", "standard", "deep"],
411+
"choices": [
412+
"shallow",
413+
"standard",
414+
"deep"
415+
],
412416
"help": "Invocation tier (FEA-1401). Gates COVERAGE_CORE_CONDITIONAL reviewers like the Impact Analyzer."
413417
}
414418
]
@@ -1437,6 +1441,20 @@
14371441
"deep"
14381442
],
14391443
"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)"
1444+
},
1445+
{
1446+
"flags": [
1447+
"--settings"
1448+
],
1449+
"default": null,
1450+
"help": "Path to code-review.json operator settings (default .closedloop-ai/settings/code-review.json); overridable for test isolation"
1451+
},
1452+
{
1453+
"flags": [
1454+
"--domain-critic-cap"
1455+
],
1456+
"type": "int",
1457+
"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"
14401458
}
14411459
]
14421460
},
@@ -1463,7 +1481,7 @@
14631481
},
14641482
{
14651483
"name": "derive-static-spec",
1466-
"help": "Emit a static spawn.json.spec for shallow-tier runs (BHA × N partitions + BHB + unified_auditor); arbitrate_status=\"static\"",
1484+
"help": "Emit a static spawn.json.spec for shallow-tier runs (BHA \u00d7 N partitions + BHB + unified_auditor); arbitrate_status=\"static\"",
14671485
"func": "cmd_derive_static_spec",
14681486
"args": [
14691487
{
@@ -1727,4 +1745,4 @@
17271745
]
17281746
}
17291747
]
1730-
}
1748+
}

plugins/code-review/tools/python/fixtures/run_plan_snapshots/cli_parser_resolved.json

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1554,6 +1554,21 @@
15541554
"standard",
15551555
"deep"
15561556
]
1557+
},
1558+
{
1559+
"flags": [
1560+
"--settings"
1561+
],
1562+
"dest": "settings",
1563+
"default": null
1564+
},
1565+
{
1566+
"flags": [
1567+
"--domain-critic-cap"
1568+
],
1569+
"dest": "domain_critic_cap",
1570+
"default": null,
1571+
"type": "int"
15571572
}
15581573
]
15591574
},

0 commit comments

Comments
 (0)