Repository navigation
code-review: make DOMAIN_CRITIC_CAP operator-tunable - #184
Closed
cchenault4 wants to merge 6 commits into
Closed
cchenault4 wants to merge 6 commits into
cchenault4 wants to merge 6 commits into
Conversation
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>
Collaborator
|
Beginning a code-review. Please allow time for me to finish before merging the PR |
shafty023
reviewed
Aug 14, 2026
shafty023
reviewed
Aug 14, 2026
shafty023
reviewed
Aug 14, 2026
shafty023
reviewed
Aug 14, 2026
shafty023
reviewed
Aug 14, 2026
mikeangstadt
approved these changes
Sep 1, 2026
mikeangstadt
left a comment
Collaborator
There was a problem hiding this comment.
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.
# 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes
DOMAIN_CRITIC_CAPoperator-tunable via.closedloop-ai/settings/code-review.json, the file that already hostsbha_unified_threshold_locandout_of_hunk_confidence_floor.The problem. The cap was a hardcoded
3with no settings path, and it drops by(priority asc, reviewer asc). A repo whosecritic-gates.jsonuses the legacymoduleCritics[]schema gets every entry migrated by_migrate_module_criticsasrequired: Falsewith nopriority— so all of them sit at the default2and the tiebreak degenerates to alphabetical by reviewer name.That is not a theoretical edge. On two
symphony-alphacost/metric PRs (#4330, #4349), the coverage critic proposedlogical-metric-reconciliation-auditorspecifically because of the diff's tally/cost semantics, consolidate accepted it, and the cap then dropped it — whileapi-architect,caching-strategist, anddatabase-architectsurvived 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_settingsgainsdomain_critic_cap(int, ≥ 0) with the same validation discipline as its siblings: bool rejected (it is anintsubclass), negative and wrong-type fall back to the default rather than crashing the stage.cmd_arbitrate_budgetreads it with precedence mirroringbha_unified_threshold_loc:--domain-critic-capflag →code-review.json→ built-inDOMAIN_CRITIC_CAP.arbitrate-budgetgains--settings(test isolation, mirroringvalidate) and--domain-critic-cap.0is 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.mdno longer state the cap as a fixed3. 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 canonicalcoverage[]schema (which supports explicitpriorityandrequired) is the precise one.Test plan
0kill switch, negative → default, wrong type → default, bool rejected). Four coverarbitrate-budgetbehavior: the settings file actually raising the cap,--domain-critic-capbeating the settings file (precedence pinned so a refactor cannot silently invert it),0spawning 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._run_arbitrate_budgetharness now always points--settingsat a test-local path, so acode-review.jsonin the CWD can never leak into a run.cli_parser_resolved.jsonregenerated. The diff is exactly the two newarbitrate-budgetflags and nothing else — I diffed parser-by-parser before writing to confirm no collateral drift.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.HEADfailure 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 missingpyyaml, not the repo.)What could still break: the cap remains tier-uniform, so
--depth deepstill does not widen it — deep's extra breadth continues to come from the cap-exempt core reviewers. Settingdomain_critic_capabove the total reviewer--caphas no additional effect, since total-cap arbitration still runs first.🤖 Generated with Claude Code