Repository navigation
refactor(judge): one default-judge rule that takes the answerer into account - #621
Conversation
|
OpenCodeReview: Commit b625955 was not reviewed. This PR has reached its review limit (6 reviews). CI at `b625955`: 13 still running, 4 passing, 1 skipped
3 more not listed. To review this commit: re-request review from exai-bot in the Reviewers list. <details><summary>Last review (e6b09e5): no findings</summary> Open threads from the bot: 0 </details> |
There was a problem hiding this comment.
🟡 Changes recommended
Judge resolution prematurely validates an unused top-level model, causing valid explicit-subject and library matrix configurations to fail.
3 open issues
What changed in this PR
3 new findings: 2 must-fix issues and 1 nit.
This PR centralizes default-judge selection so judging commands account for the answerer rather than selecting a judge solely by provider.
Changes:
- Introduces a shared judge rule with Claude CLI alias selection and same-model conflict detection.
- Integrates the rule into CLI parsing, novelty, and arms configuration, with exceptions for non-judging runs and CLI matrices.
- Updates regression tests, subscription-backend documentation, and the changelog.
| File | Description |
|---|---|
dokimi/tests/test_same_model.py |
Uses the shared rule in model-identity tests. |
dokimi/tests/test_llm_cli.py |
Checks the Claude CLI alias default. |
dokimi/tests/test_judge_provider_default.py |
Covers provider defaults, conflicts, preflight, and matrices. |
dokimi/tests/test_input_devices.py |
Avoids a judge conflict in input-validation coverage. |
dokimi/tests/test_import_architecture.py |
Registers the shared rule in the import layer. |
dokimi/tests/test_arms_run_config.py |
Updates configuration-default expectations. |
dokimi/tests/test_arms_run_config_parity.py |
Adjusts panel tests for answerer-aware defaults. |
dokimi/dokimi/novelty/client.py |
Re-exports the shared rule instead of defining it locally. |
dokimi/dokimi/judge_rule.py |
Implements centralized default-judge selection. |
dokimi/dokimi/eval/arms_run.py |
Applies the rule during configuration construction. |
dokimi/dokimi/defaults.py |
Removes the old resolver and updates help text. |
dokimi/dokimi/commands/benchmark.py |
Marks preflight as non-judging. |
dokimi/dokimi/cli.py |
Resolves default judges with answerer and matrix context. |
dokimi/docs/subscription-backends.md |
Documents alias selection and conflict behavior. |
dokimi/changelog.d/20261111_100000_napetrov_default-judge-shared-rule.md |
Records behavior changes and the matrix limitation. |
Review effort: Balanced
| the answerer, and `novelty` took it from another that did not. They all take it from | ||
| `judge_rule.default_judge` now: the default judge on the judge's provider, unless it is the model | ||
| under measurement, then the default generator on that provider. Two visible effects: | ||
| - **`--provider claude-cli` judges with an alias.** With no `--judge-model`, the claude-cli |
There was a problem hiding this comment.
Not changed. With --provider claude-cli and no --judge-provider, the judge follows the answerer onto the CLI: arms run --provider claude-cli --model haiku --judge resolves to opus on claude-cli at this head. The bullet already says "no --judge-provider", so it stays.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #621 +/- ##
==========================================
- Coverage 95.73% 95.72% -0.01%
==========================================
Files 403 404 +1
Lines 54061 54139 +78
Branches 9308 9317 +9
==========================================
+ Hits 51755 51827 +72
- Misses 1421 1425 +4
- Partials 885 887 +2
🚀 New features to boost your workflow:
|
…account Three resolvers picked the default judge: defaults.default_judge_model (the CLI sentinel), novelty.client.default_judge and ArmsRunConfig. The arms path ignored the answerer, so a run could judge its own answers. dokimi.judge_rule.default_judge(answerer, provider, answerer_provider) is now the only one. The judge is the default on the judge's provider unless it is the answerer (same_model), then the default generator. On claude-cli the judge is the first alias of opus, sonnet, haiku not of the answerer's family. A judging run whose answerer is the only default on its provider exits before any answer and asks for --judge-model. Runs that do not judge are not refused. The rule lives in its own module, not in utils or defaults: defaults cannot import utils (cycle), and utils is at its length cap. Behaviour change, see changelog.d. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…s CLI argparse fills --judge-provider with the anthropic default, so the shared rule never saw that no provider was named and its answerer-provider swap did not fire from the CLI. `--provider claude-cli` alone was refused (the judge would be the answerer's model on anthropic) although the changelog says it judges with an alias. The rule now gets no provider when the flag is not named (abbreviations count) and the answerer runs on a CLI backend; the chosen provider is kept. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…laude-cli answerer judges on its CLI ArmsRunConfig resolved the default judge in __post_init__, so a matrix-shaped config (no model, judge_provider=openai) raised at construction, where base resolved gpt-6.1-sol. The default judge that is the answerer is now deferred (DeferredJudge, a str that survives replace()) and refused at run start by refuse_deferred_self_judge; an explicit --judge-model stays warn-only. A library claude-cli answerer with the default judge provider now judges on its CLI (opus), as the CLI does. docs/reviewing.md says the default-judge self-judge exits 2. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ell is warned The field default is now a sentinel equal to the provider name, so an explicit judge_provider=anthropic is kept for a claude-cli answerer (it was moved to the CLI as the default is). A matrix cell whose model is the default judge is warned, not refused, as the changelog and the CLI say; the run-start refusal stays for a single run. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
… to two lines The follow-the-CLI branch's variable reused the name judge_provider, which mypy typed as str. The judge-model check then read it under a new name. Also trims a three-line comment block that the static Comment blocks check rejects. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Thread: PRRT_kwDOVBg7g862VxAJ Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
An explicit-cells descriptor has no top-level answerer, so the refusal waits for the cells; implicit cells keep it. Thread: PRRT_kwDOVBg7g862S2_y Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…udge Thread: PRRT_kwDOVBg7g862Vw_7 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…udge edit Thread: PRRT_kwDOVBg7g862Vw_7 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Thread: PRRT_kwDOVBg7g862VxAJ Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 9ee2fe0. The shared rule is right: every judging surface now takes its default judge from judge_rule.default_judge, a single run whose default judge is the answerer is refused before any answer, and matrix cells are only warned. What is left is mostly docs and the changelog: the change to the claude-cli judge changes scores, and the fragment does not say so.
What I ran, at 9ee2fe0:
make ci-local --offline: 0 failed, 21 passed, 19 skipped (network-only steps).parse_argsoneval score pointwise,arms run,benchmark run|preflightwith--judge-provideranthropic,openai,amazon-bedrock,claude-cli, a claude-cli answerer,--matrix-cellsand an abbreviated--judge-pr;ArmsRunConfigfor a library matrix and a claude-cli answerer.
Checked, no issues
- Earlier threads: the subjects descriptor cells (07a59a7), the library matrix (3cc104e: cells carry
matrix_cell, sorefuse_deferred_self_judgeskips them), themodel=keyword ofnovelty.client.default_judgeand thereviewing.mdwording are fixed. On the changelog thread the author is right:--provider claude-cli --model haiku --judgedoes judge onclaude-cli. - Non-judging runs:
arms runwithout--judgekeeps the base default judge instead of exiting.
| def _names_judge_provider(token: str) -> bool: | ||
| """True for ``--judge-provider`` (``=value`` too) or an argparse abbreviation of it (``--judge-p``).""" | ||
| flag = token.split("=", 1)[0] | ||
| return flag.startswith("--judge-p") and "--judge-provider".startswith(flag) |
There was a problem hiding this comment.
Nit: _names_judge_provider re-implements argparse's prefix matching on raw argv, while arms_run answers the same question with the JUDGE_PROVIDER_DEFAULT identity sentinel.
Suggestion: use default=JUDGE_PROVIDER_DEFAULT on the --judge-provider options and test args.judge_provider is JUDGE_PROVIDER_DEFAULT; then _names_judge_provider and judge_provider_named can go.
There was a problem hiding this comment.
Not changed. The raw-argv check in cli.py covers every command in one place. A sentinel default would change the ~14 --judge-provider options for a nit with no behaviour difference.
--judge-model's help said claude-sonnet-5-5 (an alias) was the default judge on claude-cli. The rule picks opus there, and sonnet only for an opus answerer. Thread: PRRT_kwDOVBg7g862m0Vz Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Preflight took the judge as a warning only, so it printed openai/gpt-6.1-sol for flags `benchmark run` refuses at parse time. It now takes the same judge rule, so both commands agree on the plan. Thread: PRRT_kwDOVBg7g862m0V6 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
refuse_deferred_self_judge reads only the config, but ran after the arms, the sampling probe (a real call) and the library lookup (network). It now runs at the top of prepare_arms. Thread: PRRT_kwDOVBg7g862m0V8 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…es on its CLI With --judge-model given and --judge-provider left out, the judge stayed on the Anthropic API, so `--provider claude-cli --model haiku --judge` sent the alias `haiku` to the API and failed at the first judge call. The left-out model already followed the CLI answerer; the explicit one now does too, on the CLI path (parse_args) and the library path (ArmsRunConfig). A named --judge-provider is kept. Thread: PRRT_kwDOVBg7g862m0WD Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
… the refusal The fragment now names the explicit --judge-provider claude-cli case (opus, was claude-sonnet-5-5), says that runs judged on claude-cli are not comparable with earlier runs and need a re-baseline, and says that benchmark preflight refuses the flags benchmark run refuses. The "Default models" section of the benchmarking doc drops the old warn-only rule for the answerer's judge and states the rule that refuses it, with opus/sonnet on claude-cli. Thread: PRRT_kwDOVBg7g862m0Vo Thread: PRRT_kwDOVBg7g862m0Vv Review: PRR_kwDOVBg7g88AAAABXh-xkQ Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…s-621-tmp Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The comment still said preflight warns about the judge and does not refuse one, which the m0V6 fix reversed. The flag is kept (apply_default_judge reads it); the test test_preflight_refuses_the_judge_that_run_refuses pins the refusal. Thread: PRRT_kwDOVBg7g862m0V6 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Preflight now refuses a judge provider that is the answerer's without a judge model, as run does. Seven exit-code tests named --judge-provider openai with --provider openai and no judge model, so they now stopped at that refusal before the check they test. Each passes --judge-model judge-x. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Re: review by vlad-perevezentsev
|
…del move The defaults-parity test had swapped its --judge-provider openai case for claude-cli, so a non-judging run whose default is the answerer's provider went untested. Both cases run now. The changelog fragment says that an explicit --judge-model also moves the judge provider when no --judge-provider is given. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
vchamarthi
left a comment
vchamarthi
left a comment
There was a problem hiding this comment.
Review at e052d61. 2 new findings: 2 must-fix. In the line comments. The PR replaces three default-judge resolvers with one rule in dokimi/judge_rule.py, so a judging run no longer takes its own answerer as the judge and a claude-cli judge is an alias. The rule holds on the CLI and novelty paths the changed tests cover, but two paths still move the judge provider to claude-cli where they should not: subjects run at parse time and a library matrix cell.
Verified
- Changed tests: The ten changed test files pass on
e052d61: 539 passed, 0 failed, includingtests/test_novelty_default_judge_keyword.pyfor themodel=keyword ofnovelty.client.default_judge. - Changed tests fail on main: The changed tests fail on main:
tests/test_judge_provider_default.pycannot importrefuse_deferred_self_judge, so the new behaviour is pinned by them. - Merge: The PR merges onto main with no conflict.
- Named judge provider: Removing the named-provider check from
follow_answerer_clifailstest_a_named_judge_provider_is_kept_for_an_explicit_judge_model_on_a_claude_cli_answerer, so that test pins the check. - Matrix cell warning: Removing the matrix-cell guard from
refuse_deferred_self_judgefailstest_a_matrix_cell_whose_model_is_the_default_judge_is_warned_not_refused, so that test pins the warn-only path for cells. - Judge-provider abbreviation:
_names_judge_providercounts the argparse abbreviation--judge-pand the--judge-provider=valueform as naming the flag, and does not count--judge-panel.
parse_args moved --judge-provider to the CLI answerer for every command, subjects run included. Explicit descriptor cells are the answerers there, so the judge went to claude-cli while the cells ran on anthropic. The follow now waits for the descriptor, as the default judge already does, in resolve_judge, which also refuses a judge model the moved-to CLI judge cannot take (parse_args checked that before the move). Thread: PRRT_kwDOVBg7g862u8H5 Thread: PRRT_kwDOVBg7g86265cg Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
…s-621-tmp Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/dokimi/cli.py
cell_config copies the config with the cell's provider, which reruns the claude-cli branch of __post_init__ and moved the shared judge onto claude-cli for one cell. A matrix cell now keeps the judge its base config resolved. Thread: PRRT_kwDOVBg7g862u8IE Thread: PRRT_kwDOVBg7g86265cl Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…st say what they check Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
resolve_judge turns the default-judge refusal into an InputError before any cell runs; nothing tested it, so the run could judge itself silently. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
With --judge-model left out, apply_default_judge must not move a named judge provider onto the answerer's CLI; only the explicit-model case was tested. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
…un start refuse_deferred_self_judge refuses only the deferred default judge; nothing tested that an explicit --judge-model equal to the answerer still runs. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 1747bc4. Every point of my first round holds as fixed. The fragment now names the claude-cli judge change and the re-baseline, the docs and --help match the rule, preflight refuses what run refuses, the library refusal comes before the probe, and an explicit alias judge follows a claude-cli answerer. One small point is left: the refusal message after that move, in a line comment. On the argv scan in _names_judge_provider I accept the reason to keep it.
What I ran, at 1747bc4:
make ci-local --offline: 0 failed, 21 passed, 19 skipped (network-only steps).parse_argsforbenchmark preflight|runandarms run/--matrix-cellswith a claude-cli answerer and explicit--judge-modelhaiku,claude-opus-5-5,gpt-6.1-sol, with and without--judge-provider.subjects.resolve_judgewith explicit and implicit cells.cell_configof a library matrix into a claude-cli cell.
Checked, no issues
- Threads from @vchamarthi and exai-bot: e6b09e5 defers the CLI follow for
subjects run, so explicit cells keepanthropic/claude-sonnet-5-5and implicit ones judge onclaude-cli/opus. bd572e8 keeps the base judge in a claude-cli matrix cell, for both a left-out and an explicit judge model. - Matrix: with
--matrix-cellsthe top-level--provider claude-clidoes not move the judge, as before this PR.
| parser.error(f"--judge-provider {cur_judge_provider} takes a Claude model (sonnet, haiku, opus or a " | ||
| f"claude-* id), not {cur_judge_model!r}: pass --judge-model") |
There was a problem hiding this comment.
When the judge provider was moved to the answerer's CLI, the refusal names a flag the user did not pass and asks for one they did.
arms run ... --provider claude-cli --model sonnet --judge --judge-model gpt-6.1-sol exits 2 with --judge-provider claude-cli takes a Claude model ..., not 'gpt-6.1-sol': pass --judge-model. The same message comes from benchmark run and from resolve_judge in subjects.py:106. The changelog documents this message instead of fixing it.
Suggestion: when --judge-provider was not named, say that the judge follows --provider claude-cli and ask for --judge-provider (e.g. openai) for a non-Claude judge; then drop that sentence from the fragment.
There was a problem hiding this comment.
Fixed in fc6b2f1: With --judge-provider not passed, the refusal says the judge follows --provider claude-cli and asks for --judge-provider; the changelog sentence matches. Same message from benchmark run and resolve_judge. tests/test_judge_provider_default.py::test_a_judge_model_refusal_does_not_name_a_judge_provider_that_was_not_passed pins it and fails without the change.
…ge-model refusal names the flag to pass A library `ArmsRunConfig` with `provider="claude-cli"` and an explicit `judge_model` judged every matrix cell on `claude-cli`; `cell_config` now reverts the followed provider to the default. A `--judge-model` the CLI judge cannot take, with `--judge-provider` not passed, now says the judge follows `--provider` and asks for `--judge-provider` (`cli.py`, `resolve_judge`). Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com>
|
Pushed 1 commit to
Threads: 1 fixed, 0 answered as not a defect. |
…atrix does ArmsRunConfig(provider="claude-cli") with judge_model left out picked claude-cli/opus from the unused top-level model, and cell_config kept it for every cell; arms matrix-run judges anthropic/claude-sonnet-5-5 there, as main did. A cell now takes the default judge with no answerer. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
xaleryb
left a comment
xaleryb
left a comment
There was a problem hiding this comment.
Re-checked at bc23905 + d428307 merged with main (Python 3.13): full suite, ruff, mypy, bandit and docs check --strict pass, and every earlier thread's original input passes. A library matrix with a left-out judge now judges every cell with the default judge, as arms matrix-run and main do. Since then only main (#737) was merged in; the PR's own diff is unchanged, and CI on b625955 is green.
Summary
Three resolvers picked the default judge:
defaults.default_judge_model(CLI sentinel),novelty.client.default_judge, andArmsRunConfig.__post_init__. The arms path ignored the answerer, so a run could judge its own answers. Now one rule:The rule lives in
judge_rule.py, notdefaults.pyorutils.py: defaults cannot import utils (cycle), and utils is at its length cap.Evidence
benchmark preflight --judge-provider openaiexits 0;arms matrix-run --matrix-cells c.json --judge --judge-provider openairesolvesgpt-6.1-sol.After: the same preflight exits 2 with the same refusal, since it plans the run; the matrix run still resolves
gpt-6.1-sol;arms run --model claude-sonnet-5-5 --judge --judge-provider anthropicexits 2 asking for--judge-model.tests/test_import_architecture.py: 53 passed.064cf4234: 34 failed, 18954 passed, 20 skipped. Of the 34: 7 were the preflight exit-code tests that passed--provider openai --judge-provider openaiwith no judge model, now stopped by the refusal this PR adds; they pass--judge-model judge-xin64334b0f4. 19 also fail on base9ee2fe03f, so they predate this PR. 8 more (3 subprocess tests, 5 intest_catalog_unrestricted_bash.py) pass whenPYTHONPATHpoints at the checkout's package, so they were a local import artifact. Not re-run on64334b0f4as a full suite.make ci-local CI_LOCAL_ARGS="--offline --jobs static,typecheck": 0 failed, 14 passed.pr-review, four passes): pass 1 found two regressions (preflight refused with a judge provider; matrix runs refused on the top-level default model), both fixed with regression tests. Passes 2 to 4: no blocking finding; pass 3's nits on judge routing for an explicit--judge-modeland on the defaults-parity test are open.Merge Danger
Door: two-way (a revert restores the three resolvers).
Blast radius: CLI judge selection on every judging surface, and the arms matrix path.
--provider claude-cliwith no--judge-modelnow judges with an alias (opus/sonnet/haiku), notclaude-sonnet-5-5. Recorded judge ids change meaning if the CLI moves its aliases.arms run/subjects runwithout--judge) are not refused.Changelog fragment:
changelog.d/20261111_100000_napetrov_default-judge-shared-rule.md.🤖 Generated with Claude Code