Repository navigation
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #255 +/- ##
==========================================
+ Coverage 93.50% 93.58% +0.08%
==========================================
Files 273 277 +4
Lines 35569 35921 +352
Branches 6040 6086 +46
==========================================
+ Hits 33260 33618 +358
+ Misses 1492 1489 -3
+ Partials 817 814 -3
🚀 New features to boost your workflow:
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 563eed3. The routing, the isolation flags and the error classification in llm_cli.py hold up, and the API-provider path is unchanged. The problems are at the edges: an ordering bug in orchestrate eval, a novelty default that was missed, and test gaps that let real regressions through.
What I ran, on Linux at 563eed3:
make ci-local: 28 passed, 0 failed, 7 skipped. The changed-line coverage step passes.- Reproductions for the code findings, with the LLM transport mocked and no
claudebinary started. - A mutation run on
test_llm_cli.py: 122 mutants, 52 of which pass the full suite. Most are cosmetic; the ones that matter are in the two test comments.
Findings outside the diff:
-
orchestrate eval --judge-provider claude-cli --judge-temperature 0is refused only after personas, questions and answers have been generated.EvaluationPipeline.__init__(orchestrator/pipeline.py:376-380) only storesjudge_temperature;check_samplingruns whenJudge(...)is built in_evaluate_answers(pipeline.py:647-668). Withjudge_provider="claude-cli", judge_temperature=0.0the constructor raises nothing, andrun()printed "Discovered 0 personas", "Generated 1 questions", "Generated answers for 1 questions" before reaching the judge. A user pairing an API answerer with a CLI judge pays for the whole answer step, then gets aValueError. The commit message of f192c40 says the refusal is "before the run". Suggestion: callcheck_sampling(judge_provider, judge_temperature, None)in the pipeline constructor, or at the top ofcmd_evaluateincommands/orchestrate.py. -
novelty ... --judge-provider claude-cliwithout--judge-modelfails, although the docs and changelog list novelty as supported.defaults.default_judge_model("claude-cli")was extended (defaults.py:71), butdefault_judgeinnovelty/client.py:248-272was not: it only matches a candidate whose provider equals--judge-provideror Bedrock.default_judge("sonnet", "claude-cli")raises--judge-provider claude-cli: no default judge runs there other than the model under measurement (sonnet); pass --judge-model as well. The message is wrong for this case, since the judge is not the model under measurement. Suggestion: givedefault_judgethe same CLI branch asdefault_judge_model, and add a test next totest_the_default_judge_runs_on_the_cli.
Checked, no issues
- Provider lists: every
--providerchoice incommands/*now comes fromANSWER_PROVIDERSorJUDGE_PROVIDERS; no old tuple is left. - Sampling refusal:
arms,judge-validity,JudgeandJudgePanelrefuse before any model call. - Schema versioning: the spec change only raises
max_docsfrom 32 to 33, withinspec.schema.json's range; no version bump is needed. llm_json.py:extract_json_objectandextract_json_arrayare byte-identical to the base, andfrom dokimi.llm import ...still works.- Docs:
mkdocs buildis clean, the nav entry is present, and the flags insubscription-backends.mdmatch argparse.
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 43eafc9. The core of the claude-cli provider is done with care: the routing, the isolation flags, the error classes and cost_usd=None are right. Some things still need changes before merge; they are in the line comments, and two points from the earlier review are still open at this head.
What I ran, at 43eafc9:
make ci-local --offline: 17 passed, 0 failed, 13 skipped (the skips need the network or docker).- No live
claude -pcalls. Every repro ran through a fake CLI runner: the alias against full-id independence check,ArmRunnerwithbaseline,agentand with a session,mkdirunder a read-only parent,claude auth statuswith no JSON,OPENAI_API_KEY=file:with a missing file, andanswers generate --provider claude-cliwithout--model. _envinside a Claude Code 2.1.303 session, to see which variables reach the child.- A temporary merge with #155:
select_docsfails withgolden_manifest violation: matched 34 ... more than max_docs=33, since both PRs raisemax_docsfrom 32 to 33 for a new page.
Checked, no issues
- Earlier threads: the
tmp_paththread is fixed in e79c12d, and the "32 files" comment inspec.v1.yamlis fixed in 43eafc9. - Prompt size: the system prompt goes in argv, but the agent profile is cut at 12,000 characters, far below the argv limit.
detectwithout--setting-sources: right as it is, sinceclaude auth statusdoes not accept that flag.- Artifacts: no schema or stored-key changes, and the numbers for API providers do not change.
- Docs and changelog: the new page is in
nav:and indocs/README.md, and the Added/Changed entries fit. - Files: SPDX headers are in all new files, the tests run offline with a fake runner, and no run artifacts are in the PR.
| """ | ||
| if provider == DEFAULT_JUDGE_PROVIDER or (provider == "amazon-bedrock" | ||
| # claude-cli takes Anthropic's model ids, as the anthropic provider does. | ||
| if provider in (DEFAULT_JUDGE_PROVIDER, "claude-cli") or (provider == "amazon-bedrock" |
There was a problem hiding this comment.
Blocking: With --provider claude-cli the answerer usually gets an alias (--model sonnet, as in the docs), but the default judge here is the full id claude-sonnet-5-5. The independence check compares strings (normalize_model_ref), so claude-cli/sonnet != claude-cli/claude-sonnet-5-5: eval score prints no warning and writes evaluator_independence_warning: false, and novelty's judge.model == answerer.model refusal (novelty/measure.py:243) does not fire. Please treat a CLI alias as the same model as any claude-<alias>-* id in these checks, and warn when unsure.
There was a problem hiding this comment.
Fixed in 9147dfa. The independence checks use same_model (llm_cli.py), so a CLI alias and the full id of its family are one model: sonnet and claude-sonnet-5-5, including Bedrock or prefixed ids. same_model is used by the evaluate independence check (commands/evaluate.py) and the novelty judge check (novelty/client.py).
Tests: test_an_alias_and_its_full_id_are_one_model_for_the_identity_checks and test_the_independence_warning_fires_for_an_alias_against_its_full_id (9147dfa), and test_an_alias_and_a_bedrock_or_prefixed_full_id_are_one_model_for_the_identity_checks (2c491ec). Head is 1688652.
There was a problem hiding this comment.
A CLI alias and the full id of its family are one model in both identity checks: same_model matches sonnet with claude-sonnet-5-5, including Bedrock and prefixed ids. The evaluate independence warning and the novelty judge gate both use it. Pinned by test_an_alias_and_its_full_id_are_one_model_for_the_identity_checks and test_the_independence_warning_fires_for_an_alias_against_its_full_id. Fixed in 9147dfa.
| "repository: --questions-root /path/to/content-repo/questions") | ||
| eval_p.add_argument("--model", default=DEFAULT_ANSWER_MODEL, help="LLM model for generation/answering") | ||
| eval_p.add_argument("--provider", default=DEFAULT_ANSWER_PROVIDER, choices=["openai", "anthropic", "amazon-bedrock", "google-vertex", "openrouter", "openai-codex"]) | ||
| eval_p.add_argument("--provider", default=DEFAULT_ANSWER_PROVIDER, choices=ANSWER_PROVIDERS) |
There was a problem hiding this comment.
Following up on @antonwolfy's point 1 in the review: still open at 43eafc9. cmd_evaluate has no check_sampling call and EvaluationPipeline.__init__ only stores judge_temperature, so orchestrate eval --judge-provider claude-cli --judge-temperature 0 still pays for personas, questions and answers before the judge refuses.
There was a problem hiding this comment.
Fixed in 35c5219. cmd_evaluate (commands/orchestrate.py) calls check_sampling(args.judge_provider, getattr(args, "judge_temperature", None), None) before the pipeline starts. orchestrate eval --judge-provider claude-cli --judge-temperature 0 now prints the ValueError and exits 1 before any persona, question or answer is paid for. Head is 1688652.
The call is pinned by test_a_cli_judge_given_a_temperature_stops_the_full_pipeline_before_any_work, which fails if the pipeline is constructed.
There was a problem hiding this comment.
cmd_evaluate now checks the judge's sampling flags before the pipeline is built, so a claude-cli judge with a temperature stops before any persona, question or answer is paid for. test_a_cli_judge_given_a_temperature_stops_the_full_pipeline_before_any_work fails if the pipeline is constructed. Fixed in 35c5219.
| for m in request.messages if m.get("role") == "system"] | ||
| out = llm_cli.call(request.provider, model=request.model, prompt=prompt_text(request), | ||
| system="\n\n".join(system) or None) | ||
| return Reply(text=out.text, usage=out.usage, finish_reason=out.finish_reason) |
There was a problem hiding this comment.
_cli_reply drops out.model_id, so answers, the judge record and the response-cache key only say sonnet. The alias moves with CLI updates and with ANTHROPIC_DEFAULT_SONNET_MODEL, which is passed through from the caller's env. So two runs that both say claude-cli/sonnet can be different models, and a cached reply from one is replayed in the other. Please record the resolved id (in the usage record or the run meta). Also, next(iter(modelUsage)) can be a helper model; harnesses/claude_json._main_model picks the main one.
There was a problem hiding this comment.
Partly addressed, at 1688652. The resolved model is recorded. It is not part of the cache key.
- Recorded (4937a07): each usage row carries
resolved_model, the model the CLI resolved the alias to. Sums keep every name they mix, comma-separated. Answer records get it throughas_metrics_dict(shown only when set, so API rows are unchanged), and the judge record listsjudge_resolved_models. - Main model (2c491ec):
_main_modelinllm_cli.pytakes the requested family frommodelUsage, else the entry with the most output, so a helper model is not reported. - Cache key (4937a07): for a CLI provider the key takes the CLI version (
llm_cli.cache_extra), not the resolved id. A new CLI release gives a new key, so the old model's replies are not replayed. Test:test_a_new_cli_version_misses_the_response_cache. - Known gaps: (1) the resolved id is known only after the call, so the pre-call key cannot hold it; a server-side move of the alias with no CLI release is not caught; (2) the version is read once per process, so a CLI self-update during a run is not seen; (3) when the version line cannot be read, the key takes an empty version, so all such calls share one key.
Tests: test_the_resolved_model_is_on_the_usage_row_and_in_its_metrics, test_a_sum_of_usage_rows_keeps_every_resolved_model_it_mixes, test_a_cached_cli_reply_replays_the_resolved_model, test_the_judge_record_names_the_resolved_models_it_was_judged_by.
There was a problem hiding this comment.
The resolved model is on each usage row and in the judge record, and the main model is taken from the requested family rather than a helper model. The response-cache key takes the CLI version, and the ANTHROPIC_* variables that could move the alias are no longer passed to the CLI. Known gap, accepted for this PR: a server-side move of the alias within one CLI version is not caught, because the resolved id is only known after the call.
| arms_run_p.add_argument("--model", default=DEFAULT_ANSWER_MODEL, help="LLM for answering") | ||
| arms_run_p.add_argument("--provider", default=DEFAULT_ANSWER_PROVIDER, | ||
| choices=["openai", "anthropic", "amazon-bedrock", "google-vertex", "openrouter", "openai-codex"]) | ||
| choices=ANSWER_PROVIDERS) |
There was a problem hiding this comment.
arms run --provider claude-cli (and matrix-run, line 384) is accepted with agentic arms (agent, skill-agent:, skill-catalog:, tool plugins) and with --session-turns, but the CLI path refuses tools and multi-turn chat on every call. The run starts, pays for the plain arms, and records agent_failed for every agentic answer; a session stops after its first turn. Please refuse this in _build_arms_output before any answer, as tools does by leaving CLI providers out.
There was a problem hiding this comment.
Partly addressed, at 1688652. Fixed in 9147dfa for arms run, with the import fix in 2c491ec and the entry-point test in 4e33134.
arms runwith--provider claude-clirefuses agentic arms (MCP agent, skill agent, skill catalog, and tool plugins) and--session-turns.refuse_cli_arms(commands/arms_refusals.py) runs in_build_arms_outputbefore any answer. It reaches through plugin wrappers and combined arms.matrix-runcalls the same_build_arms_outputfor each cell, so the refusal applies there too, but per cell. Not done: a matrix whose earlier cells use another provider runs those cells before the first CLI cell is refused. Pre-checking every cell before the first answer is open for the author to decide.--session-turnsdefaults to None (test_the_session_turns_flag_defaults_to_none, d14a762).- The entry-point test patches
ArmRunnerto fail on start (RunnerMustNotStart).
There was a problem hiding this comment.
arms run refuses agentic arms and session turns on claude-cli in _build_arms_output, before any answer. matrix-run now applies the same refusal to every cell before the first cell is answered, so an API cell does not run ahead of a refused CLI cell. Pinned by test_a_matrix_with_a_cli_agentic_cell_is_refused_before_the_first_cell_is_answered and test_the_arms_entry_point_refuses_a_cli_agentic_arm_before_any_answer. Fixed in 090122c. Correction: this branch has no tool-plugin kind, only prompt_middleware and output_shaper, so my earlier mention of tool plugins was wrong.
| Fixed, not fresh per call: the CLI states its working directory in the prompt, and | ||
| a random path would make every request, and its response-cache key, differ. | ||
| """ | ||
| path = Path(tempfile.gettempdir()) / "dokimi-cli" / name |
There was a problem hiding this comment.
<tmp>/dokimi-cli/claude-cli is shared by every user of the machine. If another user ran it first, mkdir fails with PermissionError for you; that is not a CliCallError and is not retried, so every answer fails. Anyone can also pre-create the directory (for example as a git repo), and the CLI puts its git status into the hidden prompt. Please use a per-user directory (for example under the user cache dir, mode 0700) and turn an OSError here into a CliCallError.
There was a problem hiding this comment.
Fixed in 35c5219, with the per-user test in 9147dfa. Head is 1688652.
The work dir is now per user: $XDG_CACHE_HOME/dokimi/cli/claude-cli, default ~/.cache/dokimi/cli/claude-cli, created with mode 0700. An OSError while creating it is raised as a permanent CliCallError that names the path, so it is not retried.
Tests: test_the_work_dir_is_under_the_users_cache_not_the_shared_temp_dir and test_an_unusable_work_dir_is_a_permanent_cli_error.
There was a problem hiding this comment.
The CLI work directory is now under the user's cache dir, created with mode 0700, so no other account can pre-create it. An OSError while creating it becomes a non-retryable CliCallError that names the path. Pinned by test_the_work_dir_is_under_the_users_cache_not_the_shared_temp_dir and test_an_unusable_work_dir_is_a_permanent_cli_error. Fixed in 35c5219.
| CLAUDE_ENV_DROP = frozenset({ | ||
| "CLAUDECODE", "CLAUDE_CODE_SESSION_ID", "CLAUDE_CODE_CHILD_SESSION", | ||
| "CLAUDE_CODE_ENTRYPOINT", "CLAUDE_CODE_SSE_PORT", | ||
| # Set, it wins over the CLI's login: the run would bill the key, not the subscription. | ||
| "ANTHROPIC_API_KEY", | ||
| }) |
There was a problem hiding this comment.
The drop list misses variables that change who is billed or which session the child talks to. ANTHROPIC_AUTH_TOKEN also wins over the CLI login, for the same reason you drop ANTHROPIC_API_KEY. In a Claude Code 2.1.303 session the child still gets CLAUDE_CODE_MESSAGING_SOCKET, CLAUDE_CODE_MESSAGING_TOKEN, CLAUDE_PID, CLAUDE_EFFORT and CLAUDE_CODE_SESSION_ATTENDED. Could you drop by prefix (CLAUDECODE, CLAUDE_*) and keep an explicit list of the login and provider variables?
There was a problem hiding this comment.
Fixed in 9147dfa. The child process drops every variable whose name starts with ANTHROPIC_, CLAUDE_ or CLAUDECODE (CLAUDE_ENV_DROP_PREFIXES). That covers ANTHROPIC_AUTH_TOKEN, CLAUDE_CODE_MESSAGING_SOCKET, CLAUDE_CODE_MESSAGING_TOKEN, CLAUDE_PID, CLAUDE_EFFORT and CLAUDE_CODE_SESSION_ATTENDED.
The keep list is explicit: CLAUDE_CONFIG_DIR and CLAUDE_CODE_OAUTH_TOKEN (CLAUDE_ENV_KEEP). The login check (detect) uses the same reduced env. Head is 1688652.
The prefix rule also drops the Bedrock and Vertex switch variables (CLAUDE_CODE_USE_BEDROCK, CLAUDE_CODE_USE_VERTEX), so the CLI uses its own login. This is intended and is in the changelog.
There was a problem hiding this comment.
The child process now drops every variable with prefix ANTHROPIC_, CLAUDE_ or CLAUDECODE, except CLAUDE_CONFIG_DIR and CLAUDE_CODE_OAUTH_TOKEN, and the login check uses the same reduced environment. Pinned by test_the_child_env_drops_billing_and_session_variables_by_prefix. Fixed in 9147dfa.
| if status.authenticated is False: | ||
| return f"{provider}: '{status.binary}' is not signed in ({status.hint})" | ||
| return None |
There was a problem hiding this comment.
When claude auth status times out or prints no JSON, detect returns authenticated=None and this returns None, so preflight passes with no message. Please report the unknown state at least as a warning, e.g. claude-cli: could not check the login (claude auth status gave no answer).
There was a problem hiding this comment.
Fixed in 35c5219. When claude auth status gives no answer, preflight now reports a warning: claude-cli: could not check the login ('claude auth status' gave no answer) (commands/benchmark.py). It is a warning, not an error, so the preflight still passes. Head is 1688652.
Test: test_an_unanswered_login_check_is_a_warning_not_a_pass.
There was a problem hiding this comment.
An unanswered login check is now reported as a warning, 'claude-cli: could not check the login', and preflight still passes. Pinned by test_an_unanswered_login_check_is_a_warning_not_a_pass. Fixed in 35c5219.
| if llm_cli.backend(request.provider) is not None: | ||
| return _cli_reply(request) |
There was a problem hiding this comment.
A CLI call still goes through LLM._key(), and api_key_env('claude-cli') falls back to OPENAI_API_KEY. With OPENAI_API_KEY=file:~/keys/openai and the file missing, every --provider claude-cli call fails with Cannot read API key from file before the CLI runs. Please skip the key for CLI providers, in _key() or in api_key_env.
There was a problem hiding this comment.
There was a problem hiding this comment.
A CLI provider never reads an API key now, so OPENAI_API_KEY=file:... with a missing file no longer fails before the CLI runs. Pinned by test_a_cli_provider_never_reads_an_api_key_file. Fixed in 35c5219.
| gen_a_p.add_argument("--output", default=None, help="Output file (default: answers/{product}.json)") | ||
| gen_a_p.add_argument("--model", default=DEFAULT_ANSWER_MODEL, help="LLM model for answering") | ||
| gen_a_p.add_argument("--provider", default=DEFAULT_ANSWER_PROVIDER, choices=["openai", "anthropic", "amazon-bedrock", "google-vertex", "openrouter", "openai-codex"]) | ||
| gen_a_p.add_argument("--provider", default=DEFAULT_ANSWER_PROVIDER, choices=ANSWER_PROVIDERS) |
There was a problem hiding this comment.
--provider claude-cli keeps the default answer model gpt-6.1-sol, so answers generate --provider claude-cli without --model sends --model gpt-6.1-sol to claude and every answer fails with an unknown-model error. The same holds for questions, personas and subjects run, where --model is hidden. Please refuse a non-Claude model for a CLI provider in parse_args, or pick a Claude default as default_judge_model does.
There was a problem hiding this comment.
Fixed in 35c5219, with the coverage test in 9147dfa. Head is 1688652.
A non-Claude model with --provider claude-cli is refused at parse time (cli.py:107): "takes a Claude model (sonnet, haiku, opus or a claude-* id)". This covers answers generate (default gpt-6.1-sol), questions, personas and subjects run, where --model is hidden.
Test: test_every_command_with_a_hidden_or_default_model_refuses_the_cli_without_a_claude_model.
There was a problem hiding this comment.
parse_args refuses a non-Claude model with --provider claude-cli, so answers generate, questions, personas and subjects fail at parse time and ask for --model. The parse-time test now includes answers generate, which it did not before, so my earlier claim was not backed by a test. Pinned by test_every_command_with_a_hidden_or_default_model_refuses_the_cli_without_a_claude_model. Fixed in 090122c.
| DEFAULT_JUDGE_MODEL, | ||
| DEFAULT_JUDGE_PROVIDER, | ||
| ) | ||
| from dokimi.llm import ANSWER_PROVIDERS as PROVIDERS |
There was a problem hiding this comment.
On @antonwolfy's point 2 (novelty default_judge on claude-cli): when you add the CLI branch, please compare by model family. With --model sonnet the bare-name check sees sonnet != claude-sonnet-5-5 and would pick the answerer itself as the judge, and novelty/measure.py:243 would not refuse it either.
There was a problem hiding this comment.
Fixed in 9147dfa. default_judge (novelty/client.py) on claude-cli compares by family through same_model: it picks the first of opus, sonnet, haiku that is not the answerer's family. So --model sonnet picks opus, not the answerer.
Test: test_the_novelty_default_judge_on_the_cli_is_another_family. Head is 1688652.
There was a problem hiding this comment.
default_judge on claude-cli now compares by family, so the judge is the first of opus, sonnet, haiku that is not the answerer's family. The test now covers full-id answerers, such as claude-opus-5-1, not only the bare alias. Pinned by test_the_novelty_default_judge_on_the_cli_is_another_family. Fixed in 9147dfa; test extended in 090122c.
github-actions
Bot
added
the
cli-interface-change
CLI interface change detectedThis PR changes dokimi's user-facing CLI surface (commands and/or options): + added command `dokimi backends`
+ option `--help/-h`
+ option `--json`
~ `dokimi answers generate` parameters changed:
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi arms matrix-run` parameters changed:
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi arms run` parameters changed:
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi benchmark batch` parameters changed:
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi benchmark preflight` parameters changed:
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi benchmark run` parameters changed:
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi eval checklist` parameters changed:
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ `dokimi eval judge-validity` parameters changed:
~ changed option `--escalate-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ `dokimi eval judge-wording` parameters changed:
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ `dokimi eval pairwise` parameters changed:
~ changed option `--escalate-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ `dokimi eval panel-score` parameters changed:
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi eval reference` parameters changed:
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ `dokimi eval score` parameters changed:
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ `dokimi evaluate` parameters changed:
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi novelty analyze` parameters changed:
~ changed option `--extract-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi novelty atoms` parameters changed:
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi novelty probe` parameters changed:
~ changed option `--extract-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi novelty routing` parameters changed:
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi novelty shorten` parameters changed:
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi personas discover` parameters changed:
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi questions analyze` parameters changed:
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi questions generate` parameters changed:
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi questions panel-review` parameters changed:
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]
~ `dokimi subjects run` parameters changed:
~ changed option `--judge-provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google'", "'google-vertex'", "'openai'", "'openrouter'"]
~ changed option `--provider`: choices: ["'amazon-bedrock'", "'anthropic'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"] -> ["'amazon-bedrock'", "'anthropic'", "'claude-cli'", "'google-vertex'", "'openai'", "'openai-codex'", "'openrouter'"]Updated on every push; the |
… Claude Code CLI A provider in dokimi.llm_cli.BACKENDS runs one prompt through the CLI the user is signed in to instead of an API key. LiteLLMAdapter hands such a request to llm_cli, so retry, the response cache and usage totals are unchanged. Each call is isolated (no tools, no settings, no MCP, no session, never --bare), runs in one fixed empty directory and takes the prompt on stdin; the parent's Claude Code session variables are dropped. temperature/seed are reported as unsupported, cost_usd stays None (the CLI's figure is list price), and CliCallError says whether to retry. Also: cli.py backends (install/login status, no model call), benchmark preflight checks the CLI login instead of a key, claude-cli for the judge and get_llm, the --provider choices come from llm.ANSWER_PROVIDERS, and the JSON helpers move to llm_json so llm.py does not grow. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…pling, drops the API key Answerer, PersonaGenerator and QuestionGenerator kept their own provider lists, so answers generate / personas discover / questions generate --provider claude-cli raised "Unsupported provider". A judge built with --judge-temperature or a seed now fails instead of recording a value the CLI never applied. ANTHROPIC_API_KEY is no longer passed to the child: set, it wins over the login and the run bills the key. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ore the run The refusal was per call, so eval score exited 0 with an error row per answer and recorded a judge_temperature the CLI never applied. Tests now also pin --output-format json and the whole env drop list. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
JudgePanel builds its judges without Judge.__init__, so panel-score with --judge-temperature still ran and recorded the value. The check moves to llm_cli.check_sampling, called by both. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
…rning - A CLI provider never reads an API key: OPENAI_API_KEY (the fallback for an unknown provider) no longer fails a call with a missing key file. - The work dir moves from the shared temp dir to the user's cache dir (0700); a dir that cannot be made is a permanent CliCallError, not an OSError. - orchestrate evaluate refuses --judge-temperature for a CLI judge before any persona, question or answer is paid for. - parse_args refuses a non-Claude --model for --provider claude-cli (the default answer model is not a Claude model). - Benchmark preflight warns when `claude auth status` gives no answer, instead of passing silently. - The env drop list also takes ANTHROPIC_AUTH_TOKEN and the Claude Code messaging, pid, effort and attended variables. - Tests: each retryable status and word alone, is_error and the exit code each alone, a judge panel with explicit judges, the work-dir failure. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
43eafc9 to
35c5219
Compare
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 35c5219. The earlier fixes hold: orchestrate eval now refuses a CLI judge's temperature before any work, the key path no longer reads OPENAI_API_KEY for claude-cli, the default answer model is refused for a CLI provider, the work dir is per user, and an unknown login now warns in preflight. One point from my earlier review is still open, and the test gaps in two earlier threads are unchanged. Three of @vlad-perevezentsev's points are also still open at this head.
What I ran, on a Linux review host at 35c5219:
make ci-local: 28 passed, 0 failed, 7 skipped. The changed-line coverage step passes.- Reproductions for the code findings, with the LLM transport mocked and no
claudebinary started. - A mutation run on
test_llm_cli.py: 27 mutants, including every entry of_RETRYABLE_STATUSand_RETRYABLE_WORDSand the preflight andcmd_evaluatewiring.
Still open from my earlier review:
dokimi/tests/test_llm_cli.py:111: the per-status and per-word tests are parametrized overllm_cli._RETRYABLE_STATUSand_RETRYABLE_WORDSthemselves, so dropping a member also drops its test. Dropping each of 408, 429, 500, 502, 503, 504 and 529, and each of the five words, leaves the suite green. Write the expected values as literals. Also pinusage limiton its own, and see the point on it in the thread.dokimi/tests/test_llm_cli.py:247:test_benchmark_preflight_checks_the_login_not_a_keystill calls_credential_errordirectly. Four mutants ofcmd_benchmark_preflightpass the suite: errors not appended, judge provider not checked, warnings not appended, and the API-key branch returningNone. The newcheck_samplingcall incmd_evaluate(commands/orchestrate.py:33) is also unpinned: replacing it withpasspasses the suite. The panel test with explicitJudgeConfigjudges is in place.
Findings outside the diff:
novelty ... --judge-provider claude-cliwithout--judge-modelstill fails.default_judgeinnovelty/client.py:263has no CLI branch.default_judge("sonnet", "claude-cli")raisesno default judge runs there other than the model under measurement (sonnet); pass --judge-model as well, although the judge is not the model under measurement.defaults.default_judge_modelwas extended, this was not, and the docs list novelty as supported. Suggestion: givedefault_judgethe same CLI branch, compare by model family (an aliassonnetandclaude-sonnet-5-5are one model), and add a test next totest_the_default_judge_runs_on_the_cli.
I agree with @vlad-perevezentsev's three open points (alias vs full id, agentic arms on claude-cli, the dropped model id), and reproduced each.
Checked, no issues
- Sampling refusal:
orchestrate eval --judge-provider claude-cli --judge-temperature 0exits before any persona is generated. - Key path: with
OPENAI_API_KEY=file:/missing, aclaude-clicall no longer fails on the key. - Answer model:
answers generate --provider claude-cliwithout--modelexits inparse_argswith a message naming the flag. - Docs and bookkeeping:
mkdocs build --strictis clean, the growth ratchet and import-architecture tests pass, every commit has aSigned-off-by, andllm_json.pyis byte-identical to the base functions.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to fff2f19, which fix the nit from my review:
fff2f19:dokimi/docs/subscription-backends.md:66: the work dir is now named$XDG_CACHE_HOME/dokimi/cli/claude-cli, default~/.cache/dokimi/cli/claude-cli, asllm_cli.work_dirbuilds it.
make ci-local at fff2f19: 27 passed, 1 failed (host timing flake), 7 skipped.
GitHub CI has not run on these commits: the branch conflicts with main, and Actions runs no workflow on a conflicting PR. CI will check them once main is merged in.
Not approving yet. Still open: the bug from my review; the two earlier threads of mine above nit level; @vlad-perevezentsev on alias (sonnet) vs full id (claude-sonnet-5-5) treated as different models by the independence checks; reproduced; @vlad-perevezentsev on arms run/matrix-run accept agentic arms and --session-turns with claude-cli; each fails per answer; reproduced; @vlad-perevezentsev on resolved model id dropped by _cli_reply, not in usage or cache key; helper model can be picked from modelUsage; reproduced.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/scripts/growth_baseline.json # dokimi/tests/test_import_architecture.py
… itself - Agentic arms (mcp_agent, skill_agent, skill_catalog) and --session-turns are refused on a CLI provider in arms run/matrix-run before any answer. - The child env drops ANTHROPIC_*, CLAUDE_* and CLAUDECODE by prefix, keeping CLAUDE_CONFIG_DIR and CLAUDE_CODE_OAUTH_TOKEN. - An alias and its full id are one model for the independence warning and the novelty self-judge refusal; the novelty default judge on a CLI is another family. - A usage-limit error is not retried; the reply names the main model the CLI used. - Preflight checks the login through _provider_checks; tests pin the retry lists. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
- arms: import refuse_cli_arms (a NameError on every arms run before this). - Model identity strips a Bedrock or LiteLLM prefix before the family match, so an alias and its prefixed full id are one model. The main-model fallback is not taken for a model with no family. - default_judge takes the answerer's provider: a CLI answerer with no judge provider judges on the same CLI, another family. Drops the redundant alias check. - Tests: every agentic arm kind through the wrapper and the combined arm, the arms entry point refuses before any answer, the login check uses the reduced env, the model gate names --model, and the refusal cases pass required args. - Changelog: a Bedrock or Vertex route is dropped with the rest of ANTHROPIC_/CLAUDE_. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
- The entry-point test makes any arm-runner start fail, so the refusal is checked to come before any answer machinery. - A test pins --session-turns to None by default (parse_args on arms run). - The login check test pins the reduced env (already the case at head). - A comment on the claude-cli alias list for a second CLI backend. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The arms entry point test now fails with 'the arm runner started before the CLI refusal' when the refusal is removed; before, it failed on a missing Namespace field. The session default test is renamed to what it checks. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Drop an unused loop variable, and a literal temp path in the env-drop test (bandit B108), whose value does not matter to the drop rule. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…xit 2 check_sampling raised a plain ValueError that cli.main did not catch, so `eval score` and `eval panel-score` with --judge-provider claude-cli and a temperature printed a traceback. It now raises SamplingError (a ValueError, so the existing catches still work), which cli.main reports with exit 2. The message no longer names --seed, which the panel does not have. Thread: PRRT_kwDOVBg7g86xlz59 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The guide gave the valid --provider values as an exhaustive set and left out claude-cli. A test now checks it names every answer provider. Thread: PRR_kwDOVBg7g88AAAABU43URA.a Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Thread: PRR_kwDOVBg7g88AAAABUbOhiQ: already fixed in 35c5219 and 9147dfa Thread: PRR_kwDOVBg7g88AAAABUgBkVw: already fixed in 9147dfa, 4937a07 and 9ad3fe1 Thread: PRR_kwDOVBg7g88AAAABUp_h4A: already fixed in 35c5219, a3c2923 and 9147dfa Thread: PRR_kwDOVBg7g88AAAABUqQ6ag: already fixed in 9147dfa, 090122c and 4937a07 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Both findings were fixed on this branch before this round. (1) |
|
The points open at 43eafc9 are fixed: alias against full id ( |
|
The open points of the 35c5219 review are fixed: the retry tables as literals in 35c5219, the preflight and |
|
(a) |
|
Waiting on a decision before I change this: the one open point is the settings-file Bedrock login that |
|
A coverage report, not a claim. The report is for 925efd8 (patch coverage 93.4%). The gate CI holds is 80% of changed lines, and |
|
Informational. The new |
|
Waiting on a decision before I change this: what to report when |
|
A merge note, not a claim. |
|
The summary of the review run, not a claim. Its three findings are the threads answered above. No change. |
| The Q&A track (answers, judging, question generation, personas, novelty) can call | ||
| a model through a coding-agent CLI the machine is already signed in to, instead | ||
| of an API key. Today one backend exists, `claude-cli` (Claude Code's `claude -p`). | ||
| Pass it wherever a command takes `--provider` or `--judge-provider`. |
There was a problem hiding this comment.
Nit: overbroad. tools preference --provider is limited to the API providers (PROVIDERS in commands/tools.py, because it needs tool calling), so claude-cli is rejected there. The changelog says the same. Name the exception, e.g. "wherever a command takes --provider or --judge-provider, except tools".
Suggestion:
| Pass it wherever a command takes `--provider` or `--judge-provider`. | |
| Pass it wherever a command takes `--provider` or `--judge-provider`, except `tools`, which needs tool calling. |
| from dokimi.treatments import create_treatments | ||
|
|
||
| if llm_cli.backend(args.provider) is not None: | ||
| refuse_cli_arms(args, create_treatments(specs, top_k=args.top_k, rerank_threshold=args.rerank_threshold)) |
There was a problem hiding this comment.
Nit: this builds every arm again for each matrix cell on a CLI provider, and _build_arms_output builds them once more per cell. create_treatments loads skills and profiles, creates doc-source clients, and for skill-catalog: prints discovery notes to stderr. With N CLI cells the same notes are printed N+1 times, and each arm is loaded N+1 times. The arm set doesn't depend on the cell, so build it once, or check the arm specs without constructing them.
| for r in rows: | ||
| signed = {True: "yes", False: "no", None: "unknown"}[r.authenticated] if r.installed else "-" | ||
| models = ", ".join(llm_cli.BACKENDS[r.backend].aliases) | ||
| print(f"| {r.backend} | {r.binary} | {r.version or 'not installed'} | {signed} " |
There was a problem hiding this comment.
Must-fix (wrong output): the version column prints "not installed" whenever version is empty, not when installed is False. detect sets installed=True with version="" when claude --version prints nothing (empty stdout, or a timeout, since process.run returns timed_out instead of raising). The row then reads "not installed" next to a "yes"/"no" signed-in value. Key the text off r.installed.
Suggestion:
| print(f"| {r.backend} | {r.binary} | {r.version or 'not installed'} | {signed} " | |
| version = r.version or ("unknown" if r.installed else "not installed") | |
| print(f"| {r.backend} | {r.binary} | {version} | {signed} " |
| raise CliCallError(f"claude-cli: exit {done.returncode}, no JSON reply: " | ||
| f"{(done.stderr or done.stdout).strip()[:300]}", retryable=False) |
There was a problem hiding this comment.
Nit: a non-zero exit with no JSON on stdout is always retryable=False, and the transient-word check in _claude_error is skipped on this path. If the CLI is killed or prints only API Error: 503 ... overloaded or rate limit to stderr, the call fails at once and the run is not retried, even though the same text in a JSON result would be retried. Consider running the stderr text through the same _RETRYABLE_WORDS and status check.
There was a problem hiding this comment.
Fixed in 509867c. A non-zero exit with no JSON reply now checks the transient words (rate limit, overloaded, timed out) on stderr, or stdout when stderr is empty, as a JSON result is checked. API Error: 503 ... overloaded on stderr is retried. A failure without those words is still permanent. Pinned by a new case in test_a_failed_call_says_whether_another_attempt_can_help.
| assert not (tmp_path / "shared-tmp").exists() | ||
|
|
||
|
|
||
| def test_a_reply_names_the_main_model_the_cli_resolved(runner): |
There was a problem hiding this comment.
Every success payload comes from _ok() with non-empty result, usage and modelUsage. The parser has explicit branches for the empty shapes, and no test covers them. They are modelUsage: {} or missing (_main_model returns None, resolved_model becomes ""), usage missing or null, result null or "", and an empty --version stdout in detect. A regression that indexes payload["modelUsage"] or calls max() on an empty dict would still pass the suite. Add a case such as reply(_ok(modelUsage={}, usage=None, result=None)). It should assert model_id is None, zero tokens and text == "".
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at b03bd76. Every point from my review at 43eafc9 is fixed, and the replies in the threads match the code. The settings-file Bedrock login that @xaleryb raised is still open and waits on a decision. What I add is small and in the line comments: one variable the env prefix rule should keep on Windows, the [1m] model form, and a git repository above the work dir.
What I ran, at b03bd76:
make ci-local --offline: 17 passed, 0 failed, 13 skipped (the skips need the network or docker); pytest 10,555 passed.- The identity checks with a fake CLI runner:
_warn_judge_independencewarns forclaude-cli/sonnetagainstclaude-cli/claude-sonnet-5-5, and foranthropic/claude-sonnet-5-5againstclaude-cli/sonnet. Novelty'sdefault_judge("sonnet", "claude-cli")returnsopus.same_modelandaccepts_modelonsonnet[1m]both say no. select_docswith the spec's own globs: 36 files, matchingmax_docs: 36.- No live
claude -pcall.
Checked, no issues
- Self-judge checks:
same_modelis used in the evaluate independence warning, the novelty gate and the novelty default judge. Two full ids that differ stay two models, and Vertex@ids are now recognised. - Model id: each usage row has
resolved_model, picked by family rather than the firstmodelUsagekey. The eval and arms judge records listjudge_resolved_models. The cache key takes the CLI version only for CLI providers, so API cache keys do not change. - Arms: agentic arms and
--session-turnsare refused before any answer, andmatrix-runchecks every cell before the first runs. - Errors and preflight: a subscription usage limit is not retried, and the retry tables are pinned both ways. A CLI judge's temperature is an error line with exit 2. Preflight probes each CLI once and is tested end to end.
- Env and docs: the child drops
ANTHROPIC_*/CLAUDE_*/CLAUDECODE*except the config dir and the OAuth token, and the Isolation section andreviewing.mdnow say so.
| CLAUDE_ENV_DROP_PREFIXES = ("ANTHROPIC_", "CLAUDE_", "CLAUDECODE") | ||
| #: Kept despite a prefix: where the account's login lives, and the token ``claude setup-token`` | ||
| #: makes for a headless login. | ||
| CLAUDE_ENV_KEEP = frozenset({"CLAUDE_CONFIG_DIR", "CLAUDE_CODE_OAUTH_TOKEN"}) |
There was a problem hiding this comment.
The prefix rule also drops CLAUDE_CODE_GIT_BASH_PATH, which Claude Code on Windows reads to find Git Bash when it is not in the default place.
On such a machine claude -p cannot start, every call fails with "no JSON reply", and backends still shows the CLI as signed in (auth status does not need bash).
Suggestion: add CLAUDE_CODE_GIT_BASH_PATH to CLAUDE_ENV_KEEP; it changes neither billing nor the session.
| def accepts_model(provider: str, model: str) -> bool: | ||
| """Whether *provider*'s CLI takes *model*: one of its aliases, or a full id it names.""" | ||
| cli = BACKENDS[provider] | ||
| return model in cli.aliases or model.startswith(cli.model_prefixes) |
There was a problem hiding this comment.
accepts_model refuses sonnet[1m] and opus[1m], which claude --model takes (the 1M-context form).
answers generate --provider claude-cli --model 'sonnet[1m]' exits with a usage error. If you allow it, _family must strip the [...] suffix too. Today same_model('claude-cli', 'sonnet[1m]', 'claude-cli', 'claude-sonnet-5-5') is False, so the self-judge check would miss it again.
Suggestion: strip a trailing [...] in both accepts_model and _family, with a test for each.
| Under the user's cache dir, not the shared temp dir: another account there could | ||
| pre-create it, and the CLI reads the git status of its working directory into the prompt. | ||
| """ | ||
| base = Path(os.environ.get("XDG_CACHE_HOME") or Path.home() / ".cache") |
There was a problem hiding this comment.
The work dir can be inside a git repository, which is the case the move away from the temp dir was meant to avoid.
A home directory tracked with git (a dotfiles repo), or an XDG_CACHE_HOME inside a project, makes git find that repository from ~/.cache/dokimi/cli/claude-cli. Then the CLI puts its branch and status into the hidden prompt of every call.
Suggestion: pass GIT_CEILING_DIRECTORIES=<parent of the work dir> in the child's env, so git stops looking above the work dir.
…not 'not installed' Review of intel#255 (backends.py:26): detect() sets installed=True with an empty version when --version prints nothing or times out, and the version column then read 'not installed' beside 'yes'/'no'. The column now keys off installed. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…t words Review of intel#255 (llm_cli.py:190): a non-zero exit with no JSON on stdout was always non-retryable, so 'API Error: 503 ... overloaded' or 'rate limit' on stderr failed the run at once. The same words are now checked on that text, as they are on a JSON result. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Review of intel#255 (llm_cli.py:71): the ANTHROPIC_/CLAUDE_ prefix drop also removed CLAUDE_CODE_GIT_BASH_PATH, which Claude Code on Windows reads to find Git Bash. There, every call failed with 'no JSON reply'. It changes neither the billing nor the session, so it is kept with the login variables. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Review of intel#255 (llm_cli.py:256): claude --model takes the [1m] context-window suffix, but accepts_model refused it, so a matrix cell failed with a usage error. A trailing [...] is now stripped in accepts_model, _family and same_model, so sonnet[1m] is the sonnet family for the self-judge check as well. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
… not read Review of intel#255 (llm_cli.py:305): the work dir is under the user's cache dir, which can sit inside a git repository (a home directory under git). The CLI then put that repository's branch and status into every prompt. The child now gets GIT_CEILING_DIRECTORIES set to the work dir's parent, so git does not search above it. The test runs git with the call's own env and cwd. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Review of intel#255 (test_llm_cli.py:410): every success payload in the suite had a non-empty result, usage and modelUsage, so the empty branches had no test. A reply with modelUsage {}, a null usage and a null result, and one with no modelUsage key, now must give no model id, no tokens and no text. Each of those fails when its guard is removed. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…provider Review of intel#255 (subscription-backends.md:16): 'wherever a command takes --provider' is too broad, since tools preference takes only the API providers (it needs tool calling). The sentence names that exception, and a test pins it against tools.PROVIDERS. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
🔍 OpenCodeReview found 7 issue(s) in this PR. By severity: 1 medium, 6 low
Review details
|
| lines = done.stdout.strip().splitlines() | ||
| return lines[0] if lines else "" |
There was a problem hiding this comment.
process.run reports a timeout as Completed.timed_out, not as an exception, and a failed --version may exit non-zero. Both give an empty or garbage stdout, and lru_cache then keeps "" for the whole process. Every reply then shares the cache key cli_version: "", so a transient failure silently weakens the cache-invalidation guarantee. Don't cache an unreadable result: for example, check done.ok, and keep the cached value only on success.
| done = (runner or process.run)(cli.build_argv(model, system), timeout=timeout, env=_env(cli), | ||
| cwd=work_dir(cli.name), input=prompt, capture_limit=None) |
There was a problem hiding this comment.
The continuation line is misindented: it sits at an arbitrary column under the call. Reformat it so the arguments align.
Suggestion:
| done = (runner or process.run)(cli.build_argv(model, system), timeout=timeout, env=_env(cli), | |
| cwd=work_dir(cli.name), input=prompt, capture_limit=None) | |
| done = (runner or process.run)( | |
| cli.build_argv(model, system), timeout=timeout, env=_env(cli), | |
| cwd=work_dir(cli.name), input=prompt, capture_limit=None) |
| "no temperature or seed", retryable=False) | ||
| system = [prompt_text(replace(request, messages=[m])) | ||
| for m in request.messages if m.get("role") == "system"] | ||
| out = llm_cli.call(request.provider, model=request.model, prompt=prompt_text(request), |
There was a problem hiding this comment.
prompt_text returns the content of the last message, while the guard only checks that exactly one non-system turn exists. If a system message comes after the user turn ([user, system]), the system text is sent as the prompt and the user turn is dropped. Take the prompt from turns[0] so it doesn't depend on message order.
Suggestion:
| out = llm_cli.call(request.provider, model=request.model, prompt=prompt_text(request), | |
| out = llm_cli.call(request.provider, model=request.model, prompt=prompt_text(replace(request, messages=turns)), |
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The comment-block gate (scripts/check_comment_blocks.py, from intel#298) fails a PR that adds or edits a line of a block longer than two lines. After the merge of intel/main it flagged three blocks: the CLAUDE_ENV_DROP_PREFIXES and CLAUDE_ENV_KEEP comments in llm_cli.py, and the golden_manifest bounds comment in spec.v1.yaml, which this PR edits for the new doc count. Each block is cut to two lines that keep the reason. The bounds comment drops its "8 markdown files live outside docs/" backstory; the max_docs value says the count. Red: check_comment_blocks.py --base intel/main --head HEAD exits 1 with three errors. Green: it exits 0 after this commit. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| The Q&A track (answers, judging, question generation, personas, novelty) can call | ||
| a model through a coding-agent CLI the machine is already signed in to, instead | ||
| of an API key. Today one backend exists, `claude-cli` (Claude Code's `claude -p`). | ||
| Pass it wherever a command takes `--provider` or `--judge-provider`, except `tools`, which needs tool calling. |
There was a problem hiding this comment.
Overstated. arms run and arms matrix-run also refuse a CLI provider with an agentic arm (mcp_agent, skill_agent, skill_catalog, or a combined arm containing one) and with --session-turns (see commands/arms_refusals.py). A non-Claude --model is refused at parse time too. The changelog says this, but this page doesn't, and it's the one a user reads. Name these exceptions here, e.g. in the differences table next to "Tools".
| - **The CLI states its environment, and nothing turns this off.** It does so | ||
| with no tools, with its own system prompt replaced, and with | ||
| `--exclude-dynamic-system-prompt-sections`. Tested and not working: |
There was a problem hiding this comment.
--exclude-dynamic-system-prompt-sections is not in dokimi's argv (CLAUDE_ISOLATION / _claude_argv) or in the argv listed under Isolation. The sentence reads as if dokimi passes it. Say it was tried manually and is not used, or add the flag.
| Each call is `claude -p --input-format text --output-format json --model <m> | ||
| --tools "" --setting-sources "" --strict-mcp-config --no-session-persistence | ||
| --system-prompt <s>`. The prompt goes on stdin, so its size is not bounded by | ||
| the argument limit. Without these flags, a plain `claude -p` in a repository |
There was a problem hiding this comment.
"Not bounded by the argument limit" holds for the user prompt only. The system prompt is passed as the --system-prompt <s> argv entry (_claude_argv), and the PR text lists system prompts over 128 KiB as a follow-up. No size check exists in llm_cli.py, so such a prompt would fail with an OS argument-length error. State the limit here.
| - The child gets the caller's environment minus every variable starting with | ||
| `ANTHROPIC_`, `CLAUDE_` or `CLAUDECODE`, except `CLAUDE_CONFIG_DIR` and | ||
| `CLAUDE_CODE_OAUTH_TOKEN`. That drops the session variables, an API key or |
There was a problem hiding this comment.
Doc says only two variables are exempt from the prefix drop, but llm_cli.CLAUDE_ENV_KEEP also keeps CLAUDE_CODE_GIT_BASH_PATH (needed on Windows). The same two-item list is repeated in the changelog fragment. List all three in both places. (AWS_*/GOOGLE_* are kept, which matches the Bedrock/Vertex claim, since the switches are CLAUDE_CODE_USE_*.)
Suggestion:
| - The child gets the caller's environment minus every variable starting with | |
| `ANTHROPIC_`, `CLAUDE_` or `CLAUDECODE`, except `CLAUDE_CONFIG_DIR` and | |
| `CLAUDE_CODE_OAUTH_TOKEN`. That drops the session variables, an API key or | |
| `ANTHROPIC_`, `CLAUDE_` or `CLAUDECODE`, except `CLAUDE_CONFIG_DIR`, | |
| `CLAUDE_CODE_OAUTH_TOKEN` and `CLAUDE_CODE_GIT_BASH_PATH` (Windows). |
| Rate limits, overload, 5xx and a timeout are retried with the same backoff as | ||
| an API provider. An unknown model, a missing login or a missing binary fail on | ||
| the first attempt. |
There was a problem hiding this comment.
The code (llm_cli.py, "usage limit" branch) fails a subscription usage limit at once, and the PR text says so. This section and the changelog omit it. A reader could take it for a retryable rate limit. Also add the unusable work dir, which is non-retryable.
Suggestion:
| Rate limits, overload, 5xx and a timeout are retried with the same backoff as | |
| an API provider. An unknown model, a missing login or a missing binary fail on | |
| the first attempt. | |
| Rate limits, overload, 5xx and a timeout are retried with the same backoff as | |
| an API provider. An unknown model, a missing login, a missing binary, an unusable | |
| work directory and a subscription usage limit fail on the first attempt. |
| if provider is None and answerer_provider is not None and backend(answerer_provider) is not None: | ||
| provider = answerer_provider | ||
| # Hard-coded for claude-cli; a second CLI backend (codex-cli) needs its own alias list here. | ||
| if provider is not None and provider == "claude-cli": |
There was a problem hiding this comment.
Nit: provider is not None and is redundant, because provider == "claude-cli" is already False for None. The alias tuple below also repeats CLAUDE_CLI.aliases from llm_cli, in a different order. Deriving the list from the backend would keep the two in step when an alias is added.
Suggestion:
| if provider is not None and provider == "claude-cli": | |
| if provider == "claude-cli": |
| from dokimi.novelty.client import same_client | ||
|
|
||
| if same_client(judge, answerer) and not allow_self_judge: |
There was a problem hiding this comment.
Nit: from .client import LLMClient is already a module-level import, and client imports nothing from measure. So there is no cycle to avoid. Add same_client to the existing top-level import. The same applies to the function-local import in analyze.py, which also already has from .client import LLMClient, roll_up_cost at the top.
Suggestion:
| from dokimi.novelty.client import same_client | |
| if same_client(judge, answerer) and not allow_self_judge: | |
| if same_client(judge, answerer) and not allow_self_judge: |
feat(llm): claude-cli provider: run the Q&A track on a logged-in Claude Code CLI
Summary
New provider
claude-cli: the Q&A track (answers, judge, question generation, personas, novelty) runs on a logged-in Claude Code CLI instead of an API key.cost_usdisNone: the CLI'stotal_cost_usdis list price, not what a subscription costs.--temperature/--judge-temperature/ seed withclaude-clifail before the run (orchestrate evalincluded);max_tokensis ignored.--modelwith a CLI provider fails at parse time.arms run/matrix-runrefuse agentic arms and--session-turnsbefore any answer is paid for.ANTHROPIC_*,CLAUDE_*andCLAUDECODEare dropped by prefix, exceptCLAUDE_CONFIG_DIRandCLAUDE_CODE_OAUTH_TOKEN. That also drops Bedrock and Vertex switches, so the CLI uses its own login;backendsreports that login, not the route.resolved_model(the model the CLI resolved the alias to); the judge record listsjudge_resolved_models. The response-cache key takes the CLI version for a CLI provider, so a new release that movessonnetdoes not replay the old model's replies.Evidence
answers generate --provider claude-cli→ argparse error; with the choice added,AnswererraisesUnsupported provider: 'claude-cli'.After:
tests/test_llm_cli.py(60 collected items) andtests/test_judge_sampling.py(17): argv isolation, stdin prompt, env drop by prefix, retryability per error, missing binary, unusable work dir, LLM retry and metering, judge/panel/answerer/generators, sampling refused, alias vs full-id identity, default judge family, arms and session refusals (entry point, with the runner patched to fail on start), resolved model on usage rows, judge record and cache replay, cache miss on a new CLI version,detect,backends, preflight login check.TypeError/AttributeErrorbefore;'OLD' == 'NEW'before for the version key).make ci-localon the working tree before1688652d: pytest 9755 passed, 5 skipped; ruff, bandit and mypy pass. pip-audit fails on filelock advisories, which this diff does not touch (no dependency file changed).PONGwith token usage, judge call OK, bad model → non-retryable error, missing binary → "not installed") was taken before the Bedrock/Vertex drop. This box signs in through Bedrock, so that smoke cannot be reproduced here. Not tested: a subscription OAuth login, real rate limits.8d65d107..1688652d) reported no blocking findings; its nits were either fixed or declined with the reason in the review record.Known gaps
cli_version: "", shared by every such process.Follow-ups (next PRs):
codex-cli+--judge-providerper role;--provider auto/ matrix; CLI version and list cost in run metadata; one provider registry instead of per-class lists; share theclaude -pisolation recipe withharnesses/;--system-prompt-filefor system prompts over 128 KiB.Merge Danger
Door: two-way
New provider name and a new
backendscommand; nothing existing changes shape. Theclaude-cliname is a soft door (artifacts record it);claude-codeis the harness-side name for the same binary — worth settling before people store results.Blast Radius: additive
API providers take the same path as before; the only shared edits are the
--providerchoice lists (now one tuple, same members plusclaude-cli),is_retryablereading anexc.retryableattribute first, and the usage row / judge record gaining an optionalresolved_modelkey that is absent for API providers.🤖 Generated with Claude Code