Skip to content

code-review: make DOMAIN_CRITIC_CAP operator-tunable - #184

Closed
cchenault4 wants to merge 6 commits into
mainfrom
fix/iss-5122-domain-critic-cap-tunable
Closed

cchenault4 wants to merge 6 commits into
mainfrom
fix/iss-5122-domain-critic-cap-tunable

Conversation

@cchenault4

Copy link
Copy Markdown
Contributor

Summary

Makes DOMAIN_CRITIC_CAP operator-tunable via .closedloop-ai/settings/code-review.json, the file that already hosts bha_unified_threshold_loc and out_of_hunk_confidence_floor.

The problem. The 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 by _migrate_module_critics 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 is not a theoretical edge. On two symphony-alpha cost/metric PRs (#4330, #4349), the coverage critic proposed logical-metric-reconciliation-auditor specifically because of the diff's tally/cost semantics, consolidate accepted it, and the cap then dropped it — while api-architect, caching-strategist, and database-architect survived on their first letter. The reviewer best matched to the change was the one systematically excluded, on every such PR.

The change.

  • _load_code_review_settings gains domain_critic_cap (int, ≥ 0) with the same validation discipline as its siblings: bool rejected (it is an int subclass), negative and wrong-type fall back to the default rather than crashing the stage.
  • cmd_arbitrate_budget reads it with precedence mirroring bha_unified_threshold_loc: --domain-critic-cap flag → code-review.json → built-in DOMAIN_CRITIC_CAP.
  • arbitrate-budget gains --settings (test isolation, mirroring validate) and --domain-critic-cap.
  • 0 is a valid, documented kill switch — no domain critics spawn; source: "core" reviewers (Design Critic, Impact Analyzer) stay exempt as before.

Docs. README and commands/start.md no longer state the cap as a fixed 3. Both now explain when raising it is the right call — and note that raising the cap is the blunt remedy, while migrating rules to the canonical coverage[] schema (which supports explicit priority and required) is the precise one.

Test plan

  • 9 new tests. Five cover loader validation (override honored, 0 kill switch, negative → default, wrong type → default, bool rejected). Four cover arbitrate-budget behavior: the settings file actually raising the cap, --domain-critic-cap beating the settings file (precedence pinned so a refactor cannot silently invert it), 0 spawning zero domain critics with all of them cap-deferred and no coverage-gap findings, and a malformed value leaving the fleet at the default 3.
  • Test-isolation fix: the shared _run_arbitrate_budget harness now always points --settings at a test-local path, so a code-review.json in the CWD can never leak into a run.
  • Snapshot updated deliberately: cli_parser_resolved.json regenerated. The diff is exactly the two new arbitrate-budget flags and nothing else — I diffed parser-by-parser before writing to confirm no collateral drift.
  • Full suite: 1339 passed, 3 skipped, 0 failed across test_code_review_helpers.py, test_code_review_schema.py, test_golden_fixtures.py, test_prefix_golden.py — including the golden-fixture and prefix-parity harnesses.
  • Regression check against baseline: stashed the change, captured the clean-HEAD failure set, restored, and diffed. Exactly one new failure (the CLI snapshot, expected and then updated) and zero others. (An earlier run showed 95 failures on both sides — that was my venv missing pyyaml, not the repo.)

What could still break: the cap remains tier-uniform, so --depth deep still does not widen it — deep's extra breadth continues to come from the cap-exempt core reviewers. Setting domain_critic_cap above the total reviewer --cap has no additional effect, since total-cap arbitration still runs first.

🤖 Generated with Claude Code

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>
@shafty023

Copy link
Copy Markdown
Collaborator

Beginning a code-review. Please allow time for me to finish before merging the PR

Comment thread plugins/code-review/tools/python/code_review_helpers.py
Comment thread plugins/code-review/README.md Outdated
Comment thread plugins/code-review/tools/python/test_code_review_helpers.py
Comment thread plugins/code-review/tools/python/code_review_helpers.py
Comment thread plugins/code-review/README.md

@mikeangstadt mikeangstadt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right fix for a real problem, and the loader validation lines up with its siblings. One thing worth a look before this lands: raising the cap past the total --cap headroom doesn't no-op the way the PR body says, it drops required critics through the budget_exceeded path and auto-flips the verdict. Details inline. shafty's version-bump and settings-trust points still need answers too.

Comment thread plugins/code-review/tools/python/code_review_helpers.py Outdated
Comment thread plugins/code-review/README.md
Comment thread plugins/code-review/tools/python/config/cli.json Outdated
cchenault4 and others added 5 commits October 22, 2026 08:01
# Conflicts:
#	plugins/code-review/commands/start.md
Read domain_critic_cap from origin/<base_ref> on a PR review instead of
the checked-out head, so the author of the diff under review cannot set
the cap that decides which reviewers inspect it. Clamp the cap to the
budget headroom so raising it defers the surplus instead of dropping it
as required coverage gaps, which Rule 1 turned into CHANGES_REQUESTED.

Revert the cli.json reformat collateral (choices reflow, the ×
escape, the EOF newline) that regenerating the file introduced.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRK1nj8soNurp47ZRpLvwX
The docs said required critics dropped by the cap emit coverage-gap
findings. They do not, and have not since PLN-807 Phase 4 made cap
drops report only through deferred_for_budget -- deliberately, because
a required gap is required: True and Rule 1 would turn it into
CHANGES_REQUESTED on any repo with more required critics than the cap.
An operator lowering the cap was being pointed at a signal that never
appears. Also record the cap's effective ceiling and its trust anchor.
Both manifests move 3.13.0 -> 3.14.0 (minor: a backward-compatible new
setting and CLI flag). Computed from origin/main at
ca4c0ea, which carries 3.13.0 in both
.claude-plugin and .codex-plugin.
@cchenault4 cchenault4 closed this Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants