From c136400c9e70734959ecba930749894e214d396f Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Tue, 28 Jul 2026 09:37:57 -0700 Subject: [PATCH] =?UTF-8?q?fix(voice):=20single=20scoped=20resolver=20?= =?UTF-8?q?=E2=80=94=20STT/TTS=20keys=20fall=20back=20to=20the=20credentia?= =?UTF-8?q?l=20pool?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rework of #68509 per triage: hoist the duplicated per-tool _resolve_provider_key helpers into one owner, tools.tool_backend_helpers.resolve_provider_secret(), and migrate every STT/TTS key lookup site to it. Resolution order: explicit config.yaml value > profile secret scope / env / ~/.hermes/.env > credential pool (checks both '' and 'custom:' pool keys, so keys added via 'hermes auth add mistral' or declared under providers. both resolve). Under an active multiplex turn the profile scope stays authoritative — no pool or .env fallback that could borrow another profile's key (composes with the #69469 scope fix). Coverage now includes GROQ_API_KEY, MISTRAL_API_KEY, ELEVENLABS_API_KEY, DEEPINFRA_API_KEY, MINIMAX_API_KEY, GEMINI_API_KEY/GOOGLE_API_KEY, the XAI_API_KEY fallback in resolve_xai_http_credentials, and the OpenAI audio key (resolve_openai_audio_api_key now pool-aware for OPENAI_API_KEY via 'hermes auth add openai-api'). Unit tests: fake pool entry proves each provider resolves from the pool when env is empty; env still wins when set; config wins over both; a multiplex scope miss never borrows the pool; pool read failures never raise; tool-level wiring for STT, TTS, xAI, and OpenAI audio. Fixes #68003 --- .../test_voice_credential_pool_resolution.py | 239 ++++++++++++++++++ tools/tool_backend_helpers.py | 109 +++++++- tools/transcription_tools.py | 40 ++- tools/tts_tool.py | 47 ++-- tools/xai_http.py | 7 +- 5 files changed, 391 insertions(+), 51 deletions(-) create mode 100644 tests/tools/test_voice_credential_pool_resolution.py diff --git a/tests/tools/test_voice_credential_pool_resolution.py b/tests/tools/test_voice_credential_pool_resolution.py new file mode 100644 index 00000000000..a73be308b7b --- /dev/null +++ b/tests/tools/test_voice_credential_pool_resolution.py @@ -0,0 +1,239 @@ +"""Tests for ``resolve_provider_secret`` — the single owner of STT/TTS +provider key resolution (#68003). + +Keys added via ``hermes auth add `` live in the credential pool / +auth store and used to be invisible to the voice tools, which only read +``os.environ`` + ``~/.hermes/.env`` via ``get_env_value``. The shared +resolver falls back to the pool; env still wins when set; an explicit +config.yaml value wins over both; and under a multiplexed gateway turn the +profile secret scope stays authoritative (no pool borrow). +""" + +from types import SimpleNamespace +from unittest.mock import patch + +import pytest + +from tools.tool_backend_helpers import resolve_provider_secret + + +def _fake_pool(key: str = "", *, has: bool = True): + entry = SimpleNamespace(runtime_api_key=key, access_token=key) if key else None + return SimpleNamespace( + has_credentials=lambda: has and bool(key), + peek=lambda: entry, + ) + + +@pytest.fixture(autouse=True) +def _clean_env(monkeypatch): + for var in ( + "GROQ_API_KEY", "MISTRAL_API_KEY", "ELEVENLABS_API_KEY", + "DEEPINFRA_API_KEY", "MINIMAX_API_KEY", "GEMINI_API_KEY", + "GOOGLE_API_KEY", "XAI_API_KEY", "OPENAI_API_KEY", + "VOICE_TOOLS_OPENAI_KEY", + ): + monkeypatch.delenv(var, raising=False) + yield + + +@pytest.fixture(autouse=True) +def _no_dotenv(monkeypatch): + """Keep the developer's real ~/.hermes/.env out of these tests.""" + import hermes_cli.config as config_mod + + monkeypatch.setattr(config_mod, "load_env", lambda: {}) + yield + + +class TestPoolFallback: + """Each provider's key resolves from the credential pool when env is empty.""" + + @pytest.mark.parametrize( + "env_var,provider_id", + [ + ("GROQ_API_KEY", "groq"), + ("MISTRAL_API_KEY", "mistral"), + ("ELEVENLABS_API_KEY", "elevenlabs"), + ("DEEPINFRA_API_KEY", "deepinfra"), + ("MINIMAX_API_KEY", "minimax"), + ("GEMINI_API_KEY", "gemini"), + ("XAI_API_KEY", "xai"), + ("OPENAI_API_KEY", "openai-api"), + ], + ) + def test_pool_entry_resolves_when_env_empty(self, env_var, provider_id): + pool_key_seen = [] + + def fake_load_pool(pid): + pool_key_seen.append(pid) + if pid == provider_id: + return _fake_pool(f"pool-key-{provider_id}") + return _fake_pool("") + + with patch("agent.credential_pool.load_pool", side_effect=fake_load_pool): + assert resolve_provider_secret(env_var, provider_id) == ( + f"pool-key-{provider_id}" + ) + assert provider_id in pool_key_seen + + def test_custom_pool_key_fallback(self): + """A provider pooled under ``custom:`` (config.yaml providers) + is found when the plain pool id is empty — the issue's + ``custom:mistral`` scenario.""" + + def fake_load_pool(pid): + if pid == "custom:mistral": + return _fake_pool("custom-mistral-key") + return _fake_pool("") + + with patch("agent.credential_pool.load_pool", side_effect=fake_load_pool): + assert ( + resolve_provider_secret("MISTRAL_API_KEY", "mistral") + == "custom-mistral-key" + ) + + def test_no_key_anywhere_returns_empty(self): + with patch( + "agent.credential_pool.load_pool", return_value=_fake_pool("") + ): + assert resolve_provider_secret("MISTRAL_API_KEY", "mistral") == "" + + def test_pool_read_failure_never_raises(self): + with patch( + "agent.credential_pool.load_pool", side_effect=Exception("disk error") + ): + assert resolve_provider_secret("MISTRAL_API_KEY", "mistral") == "" + + +class TestEnvPrecedence: + """Env / .env still wins over the pool when set — unchanged behaviour.""" + + def test_env_wins_over_pool(self, monkeypatch): + monkeypatch.setenv("ELEVENLABS_API_KEY", "env-key") + with patch( + "agent.credential_pool.load_pool", + return_value=_fake_pool("pool-key"), + ) as lp: + assert ( + resolve_provider_secret("ELEVENLABS_API_KEY", "elevenlabs") + == "env-key" + ) + lp.assert_not_called() + + def test_env_getter_is_consulted(self): + """Callers can pass their module-level get_env_value wrapper.""" + with patch( + "agent.credential_pool.load_pool", return_value=_fake_pool("") + ): + assert ( + resolve_provider_secret( + "GROQ_API_KEY", + "groq", + env_getter=lambda name: "dotenv-key", + ) + == "dotenv-key" + ) + + +class TestConfigPrecedence: + def test_config_value_wins_over_env_and_pool(self, monkeypatch): + monkeypatch.setenv("MISTRAL_API_KEY", "env-key") + with patch( + "agent.credential_pool.load_pool", + return_value=_fake_pool("pool-key"), + ): + assert ( + resolve_provider_secret( + "MISTRAL_API_KEY", "mistral", config_value="cfg-key" + ) + == "cfg-key" + ) + + +class TestMultiplexScope: + """Under multiplexing the profile scope is authoritative — no pool borrow.""" + + @pytest.fixture(autouse=True) + def _reset_multiplex(self): + from agent import secret_scope as ss + + ss.set_multiplex_active(False) + yield + ss.set_multiplex_active(False) + + def test_scope_value_wins(self, monkeypatch): + from agent import secret_scope as ss + + monkeypatch.setenv("MISTRAL_API_KEY", "sk-other-profile") + ss.set_multiplex_active(True) + token = ss.set_secret_scope({"MISTRAL_API_KEY": "sk-this-profile"}) + try: + assert ( + resolve_provider_secret("MISTRAL_API_KEY", "mistral") + == "sk-this-profile" + ) + finally: + ss.reset_secret_scope(token) + + def test_scope_miss_does_not_fall_through_to_pool(self): + from agent import secret_scope as ss + + ss.set_multiplex_active(True) + token = ss.set_secret_scope({"UNRELATED": "x"}) + try: + with patch( + "agent.credential_pool.load_pool", + return_value=_fake_pool("pool-key"), + ) as lp: + assert resolve_provider_secret("MISTRAL_API_KEY", "mistral") == "" + lp.assert_not_called() + finally: + ss.reset_secret_scope(token) + + +class TestToolWiring: + """The tools' module-level helpers delegate to the shared resolver.""" + + def test_transcription_tools_delegates(self): + from tools import transcription_tools as tt + + def fake_load_pool(pid): + return _fake_pool("stt-pool-key" if pid == "groq" else "") + + with patch("agent.credential_pool.load_pool", side_effect=fake_load_pool): + assert tt._resolve_provider_key("GROQ_API_KEY", "groq") == "stt-pool-key" + + def test_tts_tool_delegates(self): + from tools import tts_tool + + def fake_load_pool(pid): + return _fake_pool("tts-pool-key" if pid == "minimax" else "") + + with patch("agent.credential_pool.load_pool", side_effect=fake_load_pool): + assert ( + tts_tool._resolve_provider_key("MINIMAX_API_KEY", "minimax") + == "tts-pool-key" + ) + + def test_xai_env_fallback_consults_pool(self): + from tools.xai_http import resolve_xai_http_credentials + + def fake_load_pool(pid): + # xai-oauth pool empty → OAuth path yields no token; + # manual `hermes auth add xai` pool has the key. + return _fake_pool("xai-pool-key" if pid == "xai" else "") + + with patch("agent.credential_pool.load_pool", side_effect=fake_load_pool): + creds = resolve_xai_http_credentials() + assert creds["api_key"] == "xai-pool-key" + assert creds["provider"] == "xai" + + def test_openai_audio_key_falls_back_to_pool(self): + from tools.tool_backend_helpers import resolve_openai_audio_api_key + + def fake_load_pool(pid): + return _fake_pool("oai-pool-key" if pid == "openai-api" else "") + + with patch("agent.credential_pool.load_pool", side_effect=fake_load_pool): + assert resolve_openai_audio_api_key() == "oai-pool-key" diff --git a/tools/tool_backend_helpers.py b/tools/tool_backend_helpers.py index c28a9141141..de1fec7e8c5 100644 --- a/tools/tool_backend_helpers.py +++ b/tools/tool_backend_helpers.py @@ -2,12 +2,15 @@ from __future__ import annotations +import logging import os from pathlib import Path from typing import Any, Dict from utils import is_truthy_value +logger = logging.getLogger(__name__) + _DEFAULT_BROWSER_PROVIDER = "local" _DEFAULT_MODAL_MODE = "auto" @@ -152,6 +155,103 @@ def _scoped_credential(name: str) -> str: return (os.getenv(name, "") or "").strip() +def resolve_provider_secret( + env_var: str, + provider_id: str, + config_value: str = "", + env_getter=None, +) -> str: + """Resolve a voice-provider API key. Single owner for STT/TTS key lookup. + + Resolution order (fixes #68003 — keys added via ``hermes auth add + `` were invisible to the voice tools, which only consulted + env/.env): + + 1. An explicit ``config_value`` from config.yaml, when the caller has one. + 2. The environment / ``~/.hermes/.env``. Under a multiplexed gateway turn + this reads the active profile's secret scope (authoritative — a scope + miss must NOT borrow another profile's ``os.environ``; see + ``agent/secret_scope.py``). Outside multiplexing it reads + ``hermes_cli.config.get_env_value`` (os.environ, then ``.env``), + matching the tools' historical behaviour exactly. + 3. The credential pool / auth store for ``provider_id`` (``hermes auth + add ``). Skipped under an active multiplex turn, where + only the profile scope is authoritative for credentials. + + Never raises — credential resolution must not hard-fail on a pool or + config read; returns ``""`` when no key is found anywhere. + + ``env_getter`` lets callers supply their module-level ``get_env_value`` + wrapper (transcription_tools / tts_tool expose one that tests patch); + when omitted, ``hermes_cli.config.get_env_value`` is used directly. + """ + value = str(config_value or "").strip() + if value: + return value + + # Scope-aware env read: under a multiplexed gateway turn this reads the + # active profile's secret scope (authoritative); otherwise it reads the + # scope overlay then os.environ (see ``agent.secret_scope.get_secret``). + key = _scoped_credential(env_var) + if key: + return key + + try: + from agent.secret_scope import is_multiplex_active + + if is_multiplex_active(): + # Under multiplexing the profile scope is authoritative: do not + # fall through to the process-global .env or credential pool, + # which may belong to a different profile than the current turn. + return "" + except Exception: # pragma: no cover — secret_scope is in-repo + pass + + if env_getter is not None: + key = str(env_getter(env_var) or "").strip() + else: + try: + from hermes_cli.config import get_env_value + + key = str(get_env_value(env_var) or "").strip() + except ImportError: # pragma: no cover — config is in-repo + key = "" + if key: + return key + + if not provider_id: + return "" + try: + from agent.credential_pool import load_pool + + # `hermes auth add ` keys a registry provider by its plain + # id, but a provider declared via config.yaml ``providers.`` / + # ``custom_providers`` is pooled under ``custom:`` (see + # agent/credential_pool.py CUSTOM_POOL_PREFIX). Check both. + for pool_key in (provider_id, f"custom:{provider_id}"): + pool = load_pool(pool_key) + if pool is None or not pool.has_credentials(): + continue + entry = pool.peek() + if entry is None: + continue + key = str( + getattr(entry, "runtime_api_key", "") + or getattr(entry, "access_token", "") + or "" + ).strip() + if key: + return key + except Exception as exc: + logger.debug( + "Could not read %s credential pool for %s: %s", + provider_id, + env_var, + exc, + ) + return "" + + def resolve_openai_audio_api_key() -> str: """Prefer the voice-tools key, but fall back to the normal OpenAI key. @@ -163,10 +263,15 @@ def resolve_openai_audio_api_key() -> str: and get billed against — a different profile's OpenAI account. Same routing the WeChat send path and ``agent/vertex_adapter`` already use; see ``agent/secret_scope.py``. + + Outside a multiplexed turn, ``OPENAI_API_KEY`` additionally falls back to + the credential pool (``hermes auth add openai-api``) via + ``resolve_provider_secret`` — same #68003 fix as the other voice + providers. The dedicated voice-tools override remains env/scope-only. """ return ( - _scoped_credential("VOICE_TOOLS_OPENAI_KEY") - or _scoped_credential("OPENAI_API_KEY") + resolve_provider_secret("VOICE_TOOLS_OPENAI_KEY", "") + or resolve_provider_secret("OPENAI_API_KEY", "openai-api") ) diff --git a/tools/transcription_tools.py b/tools/transcription_tools.py index 15cad62152c..3dbe09206be 100644 --- a/tools/transcription_tools.py +++ b/tools/transcription_tools.py @@ -64,27 +64,19 @@ def get_env_value(name, default=None): def _resolve_provider_key(env_var: str, provider_id: str) -> str: - """Resolve an API key from env, .env, or the credential pool. + """Resolve an STT provider API key via the shared voice-key resolver. - Used by TTS/STT providers (Mistral, ElevenLabs) that store keys - via ``hermes auth add ``. + Delegates to ``tools.tool_backend_helpers.resolve_provider_secret`` — + the single owner of STT/TTS key resolution (config > env/.env > the + credential pool populated by ``hermes auth add ``). + Resolved at call time so tests that reload the helpers module see the + live function. """ - key = get_env_value(env_var) - if key: - return key try: - from agent.credential_pool import load_pool - pool = load_pool(provider_id) - if pool and pool.has_credentials(): - entry = pool.peek() - if entry: - key = getattr(entry, "access_token", "") or getattr(entry, "runtime_api_key", "") - key = str(key).strip() - if key: - return key - except Exception: - pass - return "" + from tools.tool_backend_helpers import resolve_provider_secret + except ImportError: # pragma: no cover — helpers are in-repo + return str(get_env_value(env_var) or "").strip() + return resolve_provider_secret(env_var, provider_id, env_getter=get_env_value) # --------------------------------------------------------------------------- # Optional imports — graceful degradation @@ -851,7 +843,7 @@ def _get_provider(stt_config: dict) -> str: return "none" if provider == "groq": - if _HAS_OPENAI and get_env_value("GROQ_API_KEY"): + if _HAS_OPENAI and _resolve_provider_key("GROQ_API_KEY", "groq"): return "groq" logger.warning( "STT provider 'groq' configured but GROQ_API_KEY not set" @@ -894,7 +886,7 @@ def _get_provider(stt_config: dict) -> str: return "none" if provider == "deepinfra": - if _HAS_OPENAI and (get_env_value("DEEPINFRA_API_KEY") or "").strip(): + if _HAS_OPENAI and _resolve_provider_key("DEEPINFRA_API_KEY", "deepinfra"): return "deepinfra" logger.warning( "STT provider 'deepinfra' configured but DEEPINFRA_API_KEY not set " @@ -919,7 +911,7 @@ def _get_provider(stt_config: dict) -> str: # Try lazy-install before falling through to cloud providers if _try_lazy_install_stt(): return "local" - if _HAS_OPENAI and get_env_value("GROQ_API_KEY"): + if _HAS_OPENAI and _resolve_provider_key("GROQ_API_KEY", "groq"): logger.info("No local STT available, using Groq Whisper API") return "groq" if _HAS_OPENAI and _has_openai_audio_backend(): @@ -942,7 +934,7 @@ def _get_provider(stt_config: dict) -> str: if _resolve_provider_key("ELEVENLABS_API_KEY", "elevenlabs"): logger.info("No local STT available, using ElevenLabs Scribe STT API") return "elevenlabs" - if _HAS_OPENAI and (get_env_value("DEEPINFRA_API_KEY") or "").strip(): + if _HAS_OPENAI and _resolve_provider_key("DEEPINFRA_API_KEY", "deepinfra"): logger.info("No local STT available, using DeepInfra Whisper API") return "deepinfra" return "none" @@ -1362,7 +1354,7 @@ def _transcribe_groq(file_path: str, model_name: str) -> Dict[str, Any]: ``HERMES_LOCAL_STT_LANGUAGE`` (env). When none is set, Groq Whisper auto-detects. """ - api_key = get_env_value("GROQ_API_KEY") + api_key = _resolve_provider_key("GROQ_API_KEY", "groq") if not api_key: return {"success": False, "transcript": "", "error": "GROQ_API_KEY not set"} @@ -1752,7 +1744,7 @@ def _transcribe_deepinfra(file_path: str, model_name: str) -> Dict[str, Any]: ``hermes_cli.models`` helpers so every DeepInfra surface resolves the base URL and model ids identically. """ - api_key = (get_env_value("DEEPINFRA_API_KEY") or "").strip() + api_key = _resolve_provider_key("DEEPINFRA_API_KEY", "deepinfra") if not api_key: return {"success": False, "transcript": "", "error": "DEEPINFRA_API_KEY not set"} diff --git a/tools/tts_tool.py b/tools/tts_tool.py index 66ab9127f58..a7fddab7b07 100644 --- a/tools/tts_tool.py +++ b/tools/tts_tool.py @@ -72,27 +72,20 @@ def get_env_value(name, default=None): def _resolve_provider_key(env_var: str, provider_id: str) -> str: - """Resolve an API key from env, .env, or the credential pool. + """Resolve a TTS provider API key via the shared voice-key resolver. - Used by TTS providers (Mistral, ElevenLabs) that store keys - via ``hermes auth add ``. + Delegates to ``tools.tool_backend_helpers.resolve_provider_secret`` — + the single owner of STT/TTS key resolution (config > env/.env > the + credential pool populated by ``hermes auth add ``). + Resolved at call time so tests that reload the helpers module see the + live function. """ - key = get_env_value(env_var) - if key: - return key try: - from agent.credential_pool import load_pool - pool = load_pool(provider_id) - if pool and pool.has_credentials(): - entry = pool.peek() - if entry: - key = getattr(entry, "access_token", "") or getattr(entry, "runtime_api_key", "") - key = str(key).strip() - if key: - return key - except Exception: - pass - return "" + from tools.tool_backend_helpers import resolve_provider_secret + except ImportError: # pragma: no cover — helpers are in-repo + return str(get_env_value(env_var) or "").strip() + return resolve_provider_secret(env_var, provider_id, env_getter=get_env_value) + from tools.managed_tool_gateway import resolve_managed_tool_gateway from tools.tool_backend_helpers import ( managed_nous_tools_enabled, @@ -1285,7 +1278,7 @@ def _generate_deepinfra_tts(text: str, output_path: str, tts_config: Dict[str, A the shared ``hermes_cli.models`` helpers so every DeepInfra surface resolves them identically. """ - api_key = (get_env_value("DEEPINFRA_API_KEY") or "").strip() + api_key = _resolve_provider_key("DEEPINFRA_API_KEY", "deepinfra") if not api_key: raise ValueError( "DEEPINFRA_API_KEY not set. Run `hermes setup` to configure, " @@ -1566,7 +1559,7 @@ def _generate_minimax_tts(text: str, output_path: str, tts_config: Dict[str, Any """ import requests - api_key = (get_env_value("MINIMAX_API_KEY") or "") + api_key = (_resolve_provider_key("MINIMAX_API_KEY", "minimax") or "") if not api_key: raise ValueError("MINIMAX_API_KEY not set. Get one at https://platform.minimax.io/") @@ -1936,7 +1929,10 @@ def _generate_gemini_tts(text: str, output_path: str, tts_config: Dict[str, Any] """ import requests - api_key = (get_env_value("GEMINI_API_KEY") or get_env_value("GOOGLE_API_KEY") or "").strip() + api_key = ( + _resolve_provider_key("GEMINI_API_KEY", "gemini") + or _resolve_provider_key("GOOGLE_API_KEY", "gemini") + ) if not api_key: raise ValueError( "GEMINI_API_KEY not set. Get one at https://aistudio.google.com/app/apikey" @@ -2782,9 +2778,9 @@ def check_tts_requirements() -> bool: _import_openai_client() except ImportError: return False - return bool(get_env_value("DEEPINFRA_API_KEY")) + return bool(_resolve_provider_key("DEEPINFRA_API_KEY", "deepinfra")) if provider == "minimax": - return bool(get_env_value("MINIMAX_API_KEY")) + return bool(_resolve_provider_key("MINIMAX_API_KEY", "minimax")) if provider == "xai": try: from tools.xai_http import resolve_xai_http_credentials @@ -2793,7 +2789,10 @@ def check_tts_requirements() -> bool: except Exception: return False if provider == "gemini": - return bool(get_env_value("GEMINI_API_KEY") or get_env_value("GOOGLE_API_KEY")) + return bool( + _resolve_provider_key("GEMINI_API_KEY", "gemini") + or _resolve_provider_key("GOOGLE_API_KEY", "gemini") + ) if provider == "mistral": try: _import_mistral_client() diff --git a/tools/xai_http.py b/tools/xai_http.py index 81f2e264ca0..8ef0b856302 100644 --- a/tools/xai_http.py +++ b/tools/xai_http.py @@ -311,7 +311,12 @@ def resolve_xai_http_credentials( except Exception: pass - api_key = str(get_env_value("XAI_API_KEY") or "").strip() + try: + from tools.tool_backend_helpers import resolve_provider_secret + + api_key = resolve_provider_secret("XAI_API_KEY", "xai", env_getter=get_env_value) + except ImportError: # pragma: no cover — helpers are in-repo + api_key = str(get_env_value("XAI_API_KEY") or "").strip() base_url = str(get_env_value("XAI_BASE_URL") or "https://api.x.ai/v1").strip().rstrip("/") return { "provider": "xai",