fix(cli): clear stale api_key/api_mode when switching away from custom provider - #49362
Conversation
…m provider Three model-selection flows updated config.yaml with a new provider but did not remove model.api_key or model.api_mode left over from a previous custom-provider setup: • _model_flow_nous: _update_config_for_provider() correctly popped the key from the on-disk file, but save_config(config) was called immediately after with the stale in-memory config dict (passed in by the caller), reinstating the old api_key. Fix: pop api_key and api_mode from model_cfg before assigning it back to config["model"]. • _model_flow_openrouter: loaded fresh config with load_config() and set provider/base_url/api_mode, but never popped api_key. • _model_flow_api_key_provider: same omission as OpenRouter. The stale api_key would silently override the new provider's credential resolution chain (which reads from .env), causing authentication failures or unintended credential leakage in config.yaml.
Covers the three flows that failed to clear stale model.api_key when the user switches from a custom endpoint to a built-in provider: • TestNousFlowClearsApiKey: mocks the Nous login/credential chain and passes a stale in-memory config dict; asserts save_config is called without api_key or api_mode in the model section. • TestOpenRouterFlowClearsApiKey: starts with a real config.yaml that has model.api_key and verifies the on-disk file no longer contains it after switching to OpenRouter. • TestApiKeyProviderFlowClearsApiKey: same, for a generic api-key provider (DeepSeek).
alt-glitch
added
type/bug
|
Related: #49360 (concurrent competing fix for the same stale-credential bug, filed ~90s earlier by a different author — it fixes only the This PR fixes the same |
The previous version accidentally included a duplicate block that used hermes_cli.model_setup_flows.* as patch targets. Those symbols don't exist as module-level attributes (they are imported lazily inside each function body), so unittest.mock raised AttributeError. Correct targets: hermes_cli.auth.*, hermes_cli.models.*, hermes_cli.main.*, hermes_cli.config.* — the source modules where each symbol is defined.
|
Closing as superseded by #49380 (#49380), which covers the same bug class across all ~13 provider-switch sites via a shared helper. You correctly extended the original Nous fix to the three main CLI flows (openrouter, api_key_provider) and prompted the wider audit that surfaced the rest — credited in the merged PR. Thanks! |
3 participants
Problem
When a user switches from a named custom provider (which writes
model.api_keytoconfig.yaml) to a built-in provider like Nous Portal, OpenRouter, or any API-key provider, the oldapi_key(and optionallyapi_mode) entry was left behind inconfig.yaml. This silently overrode the new provider's credential resolution chain (which reads from.env, notconfig.yaml), causing authentication failures or unintended credential leakage.Reported by @Latipun in Discord.
Root cause
Three model-selection flows set
model.providerbut did not clear the staleapi_key:_model_flow_nous:_update_config_for_provider()correctly poppedapi_keyfrom the on-disk file, butsave_config(config)was called immediately after with the stale in-memoryconfigdict (passed in by the caller, still carrying the old key), reinstating the oldapi_key._model_flow_openrouter: loaded fresh config viaload_config()and updated provider/base_url/api_mode, but never poppedapi_key._model_flow_api_key_provider: same omission as OpenRouter.Fix
_model_flow_nous: popapi_keyandapi_modefrommodel_cfgbeforeconfig["model"] = model_cfg(so the stale values from the caller's dict are discarded before writing)._model_flow_openrouter: addmodel.pop("api_key", None)after setting the provider fields._model_flow_api_key_provider: same as OpenRouter.Tests
New
tests/hermes_cli/test_provider_switch_clears_api_key.pywith three test classes, one per affected flow. Each starts with aconfig.yamlthat hasmodel.api_keyset (simulating a prior custom-provider session) and asserts it is absent after switching providers.