fix(tools): the provider selected in hermes tools is what runs — credential presence never reroutes - #90317
Conversation
…ols selection
Add read_selection()/selection_exists()/selection_error() to
tool_backend_helpers: one provider string per category ('nous' = managed
Nous Tool Gateway, vendor name = direct with the user's own credentials,
no key ever written = legacy credential autodetect). Legacy configs are
interpreted at read time only (use_gateway: true => nous); nothing is
migrated on disk, and the DEFAULT_CONFIG-seeded stt.provider: local is
treated as never-configured.
_resolve_managed_fal_gateway / _resolve_managed_fal_video_gateway now
switch on that string: 'nous' routes managed only (unentitled => error
naming the selection), a stored vendor routes direct only (missing
FAL_KEY => error naming FAL_KEY and the selection, no silent managed
reroute), and FAL_KEY presence no longer selects the route. Krea's
model-driven managed interception now requires no stored provider (or
the managed selection) instead of merely provider != krea, and the
image/video registries map the 'nous' selection to the FAL plugin.
…waps _get_backend returns the stored web.backend verbatim (mapping the managed 'nous' selection to the firecrawl provider) — unknown names surface the honest selection-naming error at dispatch instead of silently rerouting through the credential ladder, which now runs only on never-configured installs. _get_capability_backend no longer discards an explicit search/extract backend when its availability probe fails. The firecrawl client resolves strictly: 'nous' => managed gateway only (unavailable => selection-naming error), stored vendor => direct only (no FIRECRAWL key => error, never a silent managed fallback billed to Nous).
…redentials Both _resolve_openai_audio_client_config resolvers now switch on the stored provider string: 'nous' (or legacy use_gateway: true) => managed openai-audio gateway only, erroring by selection name when unentitled — the STT twin previously never read the stored gateway intent at all, so a direct OPENAI_API_KEY silently overrode the Nous Subscription pick; stored vendor => direct credentials only with a selection-naming error on missing keys (no silent managed fallback); never-configured keeps the legacy ladder. DEFAULT_CONFIG stops seeding stt.provider: local, and the seeded value on existing configs is treated as no-selection so autodetect keeps working for that installed base.
…ction An explicitly stored browser.cloud_provider that names no registered plugin now raises the honest selection-naming error instead of warning and silently auto-detecting; the auto-detect walk (including the managed gateway entitlement probe) runs only when no cloud_provider key was ever written. The 'nous' selection routes to the Browser Use provider, whose config resolver is now a strict switch: 'nous' => managed only, stored vendor => direct BROWSER_USE_API_KEY only with a selection-naming error when missing. Camofox is selected via browser.cloud_provider: camofox; CAMOFOX_URL stays the server ADDRESS only and can no longer override an explicit different selection (never-configured installs keep the legacy env-var activation).
…outing in status Every hermes tools row now writes exactly one selection value per category — managed 'Nous Subscription' rows write 'nous', BYOK rows the vendor name (including the historically-unset BYOK-FAL image row) — and use_gateway is no longer written; fresh picks drop any legacy key so the read-time shim cannot override them. The non-managed clear now resolves the category from the row's own markers, covering plugin-injected rows the TOOL_CATEGORIES loop missed. Setup-flow writers (managed defaults, gateway enablement) store 'nous', and the feature-state mirrors in nous_subscription.py compute per-category selections with the same legacy interpretation so hermes status matches runtime: a stored vendor selection pins direct (managed availability no longer lights it up) and an explicit non-camofox selection beats a stray CAMOFOX_URL.
New tests/tools/test_strict_provider_selection.py covers read_selection semantics (legacy use_gateway interpretation, seeded stt local, empty strings, browser.backend vs cloud_provider) and the three strict behaviors per category: managed 'nous' selection wins over present direct keys, a vendor selection with missing credentials raises the selection-naming error with NO managed call, and never-configured installs keep today's autodetect. Updated the tests that pinned the old credential-first precedence (TTS resolver gateway override, STT silent managed fallback, web invalid-backend reroute, video_gen picker writes). Sabotage-verified: reverting the image FAL strict switch makes the new managed-selection tests fail.
૮ >ﻌ< ა ci reviewran on be6a8bd — test(tools): repin selector/picker tests to the provider-str
|
alt-glitch
added
type/bug
…er provider-string migration
Two real gaps the CI-red sibling tests exposed:
- read_selection() treated EVERY raw stt.provider: local as the legacy
DEFAULT_CONFIG seed and reported no-selection — but the seed never
reached config.yaml (save_config strips schema defaults), so a
picker- or hand-written local pick was silently discarded and the
autodetect ladder could route an explicit local user to cloud STT.
A raw 'local' is now a genuine selection; the merged-view ambiguity
note replaces the over-broad shim (mirror comment updated in
nous_subscription._selected_provider and _get_provider).
- _reconfigure_provider was half-migrated: the tts/stt/browser/web
branches and the managed-category fallthrough still wrote
use_gateway flags and vendor names for managed rows. They now write
the single provider string ('nous' for managed rows) and pop the
legacy key, matching _write_provider_config.
Update the sibling tests that pinned the old use_gateway-writing
contract: image/video selector and reconfigure rows now assert the
single provider string ('nous' managed / 'fal' BYOK) plus legacy-key
popping, the stt/video picker writes drop the use_gateway expectation,
the web_server managed-browser select asserts the persisted 'nous'
cloud_provider, and explicit-local STT pins no-cloud-fallback against
a stored raw-config selection.
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head be6a8bdbf92207131eca58242e705550b9508a64 against base 7b25941b0ecd1a2d367edc7b6ef89a0958c10822. Current main is 9ec5750aca33bf6e977cfde468868b0f0d66e339, three unrelated commits ahead of the merge base; there is no direct file overlap with this tool-routing change. There were no prior human reviews or inline threads on this head. Exact-head CI, Docker, and Nix are all green.
The class-level direction is right: one persisted provider identity, a read-only legacy use_gateway compatibility shim, and no credential-presence rerouting after an explicit choice. The cross-category sweep and the selected-broken-provider error contract are materially stronger than the previous per-call-site fixes. I found three authority/transition blockers before this can safely become the canonical rule.
1. The new runtime authority bypasses the administrator-managed config layer
tools.tool_backend_helpers.read_selection() and selection_exists() read only hermes_cli.config.read_raw_config_readonly(). That API intentionally returns the user's raw ~/.hermes/config.yaml without defaults or the managed-scope overlay. But hermes_cli.managed_scope defines /etc/hermes/config.yaml as IT-owned, user-immutable policy that wins per leaf key, and normal load_config()/status paths apply it.
Concrete failure: an administrator pins image_gen.provider: nous, the user file contains image_gen.provider: fal, and FAL_KEY is present. Status/effective config says nous; the new runtime selector returns the user-owned fal and bills the personal account. If the raw user file has no selection, the same managed pin is reduced to “never configured” and ambient autodetect is allowed.
Please give selection its own explicit-effective read seam: raw user values plus the managed overlay, but still without schema defaults. It should copy before overlaying rather than mutate the identity-cached read-only raw dict. Add a real conflict witness for managed nous vs user fal + direct key and prove both runtime dispatch and status honor the managed owner.
2. “Never configured” is still conflated with “selection source unavailable”
read_raw_config() returns {} both when the file is absent and when it cannot be parsed, and read_selection() additionally catches every read exception and returns None. Every category then treats None as permission to enter the legacy credential/entitlement autodetect ladder.
That means a partial write, malformed YAML, transient read error, or permissions failure can silently change the account/provider used by the next tool call. This is the other side of the same wrong-provider class: the persisted choice did not change, but loss of evidence is interpreted as affirmative absence.
Please preserve a tri-state boundary—selected, genuinely absent, and unavailable/invalid—or retain the last known-good effective selection. unavailable/invalid must fail closed before any vendor/gateway call; it cannot be the same value as a never-configured install. Add corrupt/unreadable-config regressions with competing direct and managed credentials and assert that neither route is invoked.
3. A whole-web-provider selection does not supersede stale per-capability selections
Web dispatch deliberately resolves web.search_backend / web.extract_backend before shared web.backend. The desktop endpoint writes those capability keys when capability is supplied, while the whole-provider path calls apply_provider_selection(). _write_provider_config() then writes only web.backend; it never clears either capability override.
So a user can select split providers, later select a whole-category provider such as Nous Subscription, and continue running the old search/extract providers. Example: search_backend: tavily, extract_backend: firecrawl, then whole-provider nous leaves both old authorities intact; neither capability runs the provider the user just selected.
The whole-provider transition should atomically clear both capability overrides (or the UI/API must be explicitly redefined as “change shared fallback only,” which conflicts with this PR's promise and current picker copy). Add a split → whole-provider transition test covering both search and extract.
Provenance and merge topology
- Merged #86919 and #87562 by
@teknium1are the narrow image/FAL predecessors; this PR is a legitimate class-level superseder, not duplicate work. - Merged #35109 by
@alt-glitchis the first-install managed image/video config-writer precursor. Its setup-path contribution remains part of this lineage even though the persisted representation changes from vendor + boolean toprovider: nous. - Open #90313 by
@teknium1is complementary keyless-web fallback work on the sameconfig_defaults.py/tools_config.py/web_tools.pyseam. Its keyless tier must run only after a genuinely absent, valid selection—not after managed-policy loss or config-read failure. Whichever lands second needs a semantic composition rebase and transition tests, not just conflict resolution.
Re-review gate: an explicit-effective selection resolver that honors managed scope and preserves absence-vs-unavailable provenance; atomic reconciliation of web capability overrides on a whole-provider choice; the three regression families above; rebase onto current main; and a fresh exact-head matrix.
3 participants
Summary
The provider you pick in
hermes tools(or the desktop GUI) is now what runs — always. Runtime tool dispatch no longer chooses backends by sniffing which API keys or entitlements happen to exist: pick Nous Portal → FAL → gpt-image-2 and it uses the managed gateway even if aFAL_KEYsits in.env; pick BYOK FAL and it uses your key or fails with an error that names your selection — never a silent reroute to another route or account.This kills the class behind the repeated "wrong provider ran" incidents (most recently: an exhausted personal
FAL_KEYhijacking image gen from the managed subscription). A pre-PR audit found the sameif <key present> → direct, else → managedpattern at 10 call sites across all six tool categories; all are replaced.The model (one string, no booleans): each picker row writes one provider value —
provider: nousfor the managed Nous Subscription row, the vendor name (fal,openai,firecrawl,browser-use,camofox, …) for BYOK rows. Runtime is a switch on that string.use_gatewayis never written again; old configs are interpreted at read time (use_gateway: true⇒nous), nothing rewritten on disk. Never-configured categories keep today's autodetect, unchanged.Changes
tools/tool_backend_helpers.py:read_selection()/selection_exists()/selection_error()— the one shared resolver (folds in the legacyuse_gatewayshim; treats the previously-seededstt.provider: localas no-selection)._resolve_managed_fal_gateway, krea interception) + video FAL: strict switch on the stored selection; krea model-interception only fires with no stored provider._get_capability_backendno longer silently swaps an unavailable selected backend; firecrawl client no longer silently falls to the managed gateway.cloud_provider= honest error (was warn + autodetect);camofoxis now a real selection (CAMOFOX_URLis just the address);nousroutes via the managed browser-use gateway.hermes_cli/tools_config.py: every row persists its provider string (BYOK image FAL included), legacyuse_gatewaykeys popped on write, plugin-row clear-gap fixed.hermes_cli/nous_subscription.py: status/hermes statusmirrors + setup writers follow the same strict precedence.<category> is configured to use <selection> (set via hermes tools), but <failure>. Run 'hermes tools' to change it.Validation
provider: nous+FAL_KEY⇒nous ·provider: fal+no key⇒error naming fal/FAL_KEY/hermes tools · legacyuse_gateway: true⇒nous · never-configured⇒autodetectInfographic
Not included: FAL image account for this box is the exhausted direct key this PR exists to stop silently routing around. Will backfill via
gh pr edit.