diff --git a/hermes_cli/main.py b/hermes_cli/main.py index b5b95df893..a9d25a2a6a 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -3332,7 +3332,26 @@ def _model_flow_named_custom(config, provider_info): provider_entry = providers_cfg.get(provider_key) if isinstance(provider_entry, dict): provider_entry["default_model"] = model_name - if config_api_key and not str(provider_entry.get("api_key", "") or "").strip(): + # Only persist an inline api_key when the user originally had + # one (either a literal secret or a ``${VAR}`` template). When + # the entry relies on ``key_env``, do not synthesize a + # ``${key_env}`` api_key — the runtime already resolves the + # key from ``key_env`` directly, and writing the resolved + # secret (or even a synthesized template) would silently + # downgrade credential hygiene on entries that intentionally + # keep plaintext out of ``config.yaml``. See issue #15803. + original_api_key_ref = str( + provider_info.get("api_key_ref", "") or "" + ).strip() + original_api_key = str( + provider_info.get("api_key", "") or "" + ).strip() + had_inline_api_key = bool(original_api_key_ref or original_api_key) + if ( + had_inline_api_key + and config_api_key + and not str(provider_entry.get("api_key", "") or "").strip() + ): provider_entry["api_key"] = config_api_key if key_env and not str(provider_entry.get("key_env", "") or "").strip(): provider_entry["key_env"] = key_env diff --git a/tests/hermes_cli/test_custom_provider_model_switch.py b/tests/hermes_cli/test_custom_provider_model_switch.py index 57706f2172..454337592d 100644 --- a/tests/hermes_cli/test_custom_provider_model_switch.py +++ b/tests/hermes_cli/test_custom_provider_model_switch.py @@ -322,3 +322,129 @@ class TestCustomProviderModelSwitch: assert config["model"]["api_key"] == "${NEURALWATT_API_KEY}" assert config["custom_providers"][0]["api_key"] == "${NEURALWATT_API_KEY}" assert "sk-live-neuralwatt-secret" not in saved + + def test_key_env_providers_dict_entry_does_not_add_api_key( + self, config_home, monkeypatch + ): + """Regression for #15803: a ``providers:`` (keyed-schema) entry that + relies on ``key_env`` must not gain an ``api_key`` field after the + model picker runs. + + Before the fix, ``_model_flow_named_custom`` synthesized + ``api_key: ${KEY_ENV}`` from the resolved secret and wrote it to the + ``providers.`` entry, cluttering configs that intentionally keep + credentials out of ``config.yaml``. The entry already carries + ``key_env``; the runtime resolves it directly, so no inline + ``api_key`` belongs on disk. + """ + import yaml + from hermes_cli.main import _model_flow_named_custom + + config_path = config_home / "config.yaml" + config_path.write_text( + "providers:\n" + " crs-henkee:\n" + " name: CRS Henkee\n" + " base_url: http://127.0.0.1:3000/api/v1\n" + " key_env: HERMES_CRS_HENKEE_KEY\n" + " transport: anthropic_messages\n" + " model: claude-opus-4-7\n" + " default_model: claude-opus-4-7\n" + "custom_providers: []\n" + ) + monkeypatch.setenv("HERMES_CRS_HENKEE_KEY", "cr_live_secret_xyz") + + # provider_info as built by _named_custom_provider_map for a + # ``providers:`` entry that has key_env but no inline api_key. + provider_info = { + "name": "CRS Henkee", + "base_url": "http://127.0.0.1:3000/api/v1", + "api_key": "", + "key_env": "HERMES_CRS_HENKEE_KEY", + "model": "claude-opus-4-7", + "api_mode": "anthropic_messages", + "provider_key": "crs-henkee", + "api_key_ref": "", + } + + with patch( + "hermes_cli.models.fetch_api_models", + return_value=["claude-opus-4-7"], + ) as mock_fetch, \ + patch.dict("sys.modules", {"simple_term_menu": None}), \ + patch("builtins.input", return_value="1"), \ + patch("builtins.print"): + _model_flow_named_custom({}, provider_info) + + # The /models probe must resolve the secret from the env var. + mock_fetch.assert_called_once() + probe_args, _ = mock_fetch.call_args + assert probe_args[0] == "cr_live_secret_xyz" + + # The providers entry must NOT gain an api_key field — neither the + # plaintext secret nor a synthesized ${KEY_ENV} template. + saved_text = config_path.read_text() + saved = yaml.safe_load(saved_text) or {} + entry = saved["providers"]["crs-henkee"] + assert "api_key" not in entry, ( + f"providers.crs-henkee gained an api_key field: {entry.get('api_key')!r}" + ) + assert entry["key_env"] == "HERMES_CRS_HENKEE_KEY" + assert entry["default_model"] == "claude-opus-4-7" + + # And the plaintext secret must never appear anywhere on disk. + assert "cr_live_secret_xyz" not in saved_text + # The synthesized template is also redundant here — key_env owns it. + assert "${HERMES_CRS_HENKEE_KEY}" not in saved_text + + def test_key_env_providers_dict_preserves_existing_api_key( + self, config_home, monkeypatch + ): + """A ``providers:`` entry that already has an inline ``api_key`` + template must keep it untouched. Only entries that never declared + an ``api_key`` should skip the write.""" + import yaml + from hermes_cli.main import _model_flow_named_custom + + config_path = config_home / "config.yaml" + config_path.write_text( + "providers:\n" + " crs-henkee:\n" + " name: CRS Henkee\n" + " base_url: http://127.0.0.1:3000/api/v1\n" + " api_key: ${HERMES_CRS_HENKEE_KEY}\n" + " key_env: HERMES_CRS_HENKEE_KEY\n" + " transport: anthropic_messages\n" + " model: claude-opus-4-7\n" + " default_model: claude-opus-4-7\n" + "custom_providers: []\n" + ) + monkeypatch.setenv("HERMES_CRS_HENKEE_KEY", "cr_live_secret_xyz") + + provider_info = { + "name": "CRS Henkee", + "base_url": "http://127.0.0.1:3000/api/v1", + "api_key": "cr_live_secret_xyz", # expanded by load_config + "key_env": "HERMES_CRS_HENKEE_KEY", + "model": "claude-opus-4-7", + "api_mode": "anthropic_messages", + "provider_key": "crs-henkee", + "api_key_ref": "${HERMES_CRS_HENKEE_KEY}", # raw template preserved + } + + with patch( + "hermes_cli.models.fetch_api_models", + return_value=["claude-opus-4-7"], + ), \ + patch.dict("sys.modules", {"simple_term_menu": None}), \ + patch("builtins.input", return_value="1"), \ + patch("builtins.print"): + _model_flow_named_custom({}, provider_info) + + saved_text = config_path.read_text() + saved = yaml.safe_load(saved_text) or {} + entry = saved["providers"]["crs-henkee"] + # Existing api_key template must survive (the resolved secret must not + # clobber it via _preserve_env_ref_templates). + assert entry["api_key"] == "${HERMES_CRS_HENKEE_KEY}" + assert "cr_live_secret_xyz" not in saved_text