-
Notifications
You must be signed in to change notification settings - Fork 10
code-review: make DOMAIN_CRITIC_CAP operator-tunable #184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | | ||
| |---|---|---| | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The repo marks this README as generated and requires |
||
| | `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. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This adds shipped plugin behavior, but |
||
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes review policy controllable by the PR being reviewed. In GitHub mode this settings path is read from the checked-out PR tree, so a contributor can add |
||
| ): | ||
| out["domain_critic_cap"] = raw_critic_cap | ||
| return out | ||
|
|
||
|
|
||
|
|
@@ -11550,15 +11570,46 @@ 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 | ||
| ok, err = _validate_invocation_depth(depth) | ||
| 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: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still says required critics dropped by this cap emit coverage-gap findings, but the new zero-cap test explicitly asserts
gaps["findings"] == [], and arbitration records them only indeferred_for_budget. An operator lowering the cap could rely on a safety signal that never appears. Please make this line andcommands/start.mdmatch the implemented no-gap behavior, or change the implementation if gaps are intended.