diff --git a/agent/auxiliary_client.py b/agent/auxiliary_client.py index 9d26d1bf611b4..69b8cd969b1e8 100644 --- a/agent/auxiliary_client.py +++ b/agent/auxiliary_client.py @@ -6960,6 +6960,52 @@ def _get_cached_client( } +def _builtin_provider_present(name: str) -> bool: + """Look up *name* in the built-in provider registry, returning False + (not raising) when the catalog fails to load. + + Used by ``_preserve_provider_with_base_url`` so a built-in lookup + exception cannot suppress the parallel user-defined provider lookup + (#76602). + """ + try: + from hermes_cli.providers import get_provider + + return get_provider(name) is not None + except Exception: + # Keep the high-risk provider-backed routes safe even if provider + # catalog loading is unavailable during early import/test paths. + return name in { + "anthropic", + "copilot", + "copilot-acp", + "minimax-oauth", + "nous", + "openai-codex", + "qwen-oauth", + "xai-oauth", + } + + +def _named_custom_provider_present(name: str) -> bool: + """Look up *name* in the user-defined ``providers:`` section of + config.yaml, returning False when the config is unavailable or + fails to load. + + Used by ``_preserve_provider_with_base_url`` so a user-defined + provider remains preserved even when the built-in registry raises + (parallel lookup; each side fails independently — #76602). + """ + try: + from hermes_cli.runtime_provider import _get_named_custom_provider + + return _get_named_custom_provider(name) is not None + except Exception: + # Config not loaded yet (early import paths, tests) — fail closed: + # never widen True just because the import / load failed. + return False + + def _resolve_task_provider_model( task: str = None, provider: str = None, @@ -7077,23 +7123,18 @@ def _preserve_provider_with_base_url(prov: Optional[str]) -> bool: normalized = str(prov or "").strip().lower() if normalized in {"", "auto", "custom"} or normalized.startswith("custom:"): return False - try: - from hermes_cli.providers import get_provider - - return get_provider(normalized) is not None - except Exception: - # Keep the high-risk provider-backed routes safe even if provider - # catalog loading is unavailable during early import/test paths. - return normalized in { - "anthropic", - "copilot", - "copilot-acp", - "minimax-oauth", - "nous", - "openai-codex", - "qwen-oauth", - "xai-oauth", - } + # #76602 — two independent lookups, each guarded by its own + # try/except so a partial catalog-load failure in either path + # doesn't suppress the other. The previous shape nested the + # named-custom lookup inside the built-in try-block, so an + # exception from get_provider() jumped straight to the outer + # allowlist fallback and never consulted the user's configured + # named provider (the very bug this PR exists to fix). + if _builtin_provider_present(normalized): + return True + if _named_custom_provider_present(normalized): + return True + return False if provider: provider, base_url = _expand_direct_api_alias(provider, base_url) diff --git a/tests/agent/test_auxiliary_client.py b/tests/agent/test_auxiliary_client.py index 0e60ea3dd7064..076a75a1af6aa 100644 --- a/tests/agent/test_auxiliary_client.py +++ b/tests/agent/test_auxiliary_client.py @@ -4213,3 +4213,250 @@ def test_third_party_anthropic_fallback_keeps_message_markers_without_tool_marke for part in (message.get("content") if isinstance(message.get("content"), list) else []) ) + +# --------------------------------------------------------------------------- +# Regression coverage for #76602 — auxiliary vision with a custom provider +# defined in the ``providers:`` section of config.yaml plus an explicit +# ``base_url`` was being silently downgraded to ``"custom"`` because +# ``_preserve_provider_with_base_url`` only consulted the built-in +# provider registry (``hermes_cli.providers.get_provider``). The +# downgrade routed the call through the bare-custom branch in +# ``resolve_provider_client`` with no key, producing 401s from +# auth-required providers (e.g. agnes-ai.cn, nvidia-nim with key_env). +# The fix also checks ``_get_named_custom_provider`` so a named +# user-defined provider + explicit base_url stays named through to the +# named-custom-provider key-resolution branch. +# --------------------------------------------------------------------------- + + +class TestPreserveNamedCustomProviderWithBaseUrl: + """#76602 — _resolve_task_provider_model must keep a user-defined + provider named when the call site passes ``provider=`` + + ``base_url=...`` (the shape async_call_llm takes after resolving the + auxiliary.vision task config). The bare-custom downgrade to the + ``"custom"`` string loses the key and produces 401s. + """ + + def test_user_defined_provider_named_in_config_is_preserved(self, monkeypatch): + """A provider name from ``providers:`` survives explicit base_url. + + Before the fix this returned ``("custom", ...)`` → bare-custom + branch in ``resolve_provider_client`` → ``no-key-required`` → 401. + After the fix it returns ``("agnes-ai.cn", ...)`` so the + named-custom-provider branch picks up + ``providers..api_key`` (or the configured ``key_env``). + """ + import agent.auxiliary_client as ac + + fake_entry = { + "name": "AgnesAI", + "base_url": "https://api.agnes-ai.cn/v1", + "api_key": "sk-agnes-test", + "model": "agnes-2.5-flash", + } + with patch( + "hermes_cli.runtime_provider._get_named_custom_provider", + return_value=fake_entry, + ), patch( + "hermes_cli.providers.get_provider", return_value=None, + ): + resolved_provider, _model, base_url, _api_key, _api_mode = ( + ac._resolve_task_provider_model( + task="vision", + provider="agnes-ai.cn", + model="agnes-2.5-flash", + base_url="https://api.agnes-ai.cn/v1", + api_key=None, + ) + ) + + assert resolved_provider == "agnes-ai.cn", ( + "User-defined provider from providers: section must be preserved " + "instead of being downgraded to 'custom' (issue #76602)" + ) + assert base_url == "https://api.agnes-ai.cn/v1" + + def test_built_in_provider_with_base_url_still_preserved(self, monkeypatch): + """Built-in registry hit still wins — user-defined fallback only + fires when the built-in registry missed. This guards against + regressing the pre-existing built-in provider behavior. + """ + import agent.auxiliary_client as ac + + with patch( + "hermes_cli.providers.get_provider", + return_value={"name": "Anthropic", "api_key": "sk-anthropic"}, + ): + resolved_provider, _model, base_url, _api_key, _api_mode = ( + ac._resolve_task_provider_model( + task="moa_reference", + provider="anthropic", + model="claude-sonnet-4-6", + base_url="https://api.anthropic.com/v1", + api_key="sk-anthropic", + ) + ) + + assert resolved_provider == "anthropic" + assert base_url == "https://api.anthropic.com/v1" + + def test_unknown_provider_with_base_url_falls_back_to_custom_downgrade(self, monkeypatch): + """Provider name not in either registry → keep the existing + ``"custom"`` downgrade behavior. Nothing changes for truly + anonymous custom endpoints (no name → no key → bare-custom branch + handles ``no-key-required`` itself). + """ + import agent.auxiliary_client as ac + + with patch( + "hermes_cli.runtime_provider._get_named_custom_provider", + return_value=None, + ), patch( + "hermes_cli.providers.get_provider", return_value=None, + ): + resolved_provider, _model, base_url, _api_key, _api_mode = ( + ac._resolve_task_provider_model( + task="vision", + provider="some-unknown-gateway", + model="custom-model", + base_url="https://example.com/v1", + api_key="some-token", + ) + ) + + # Pre-existing behavior: unknown name + explicit base_url → + # downgrade to "custom" so the caller routes through the + # bare-custom branch (which uses the explicit api_key). + assert resolved_provider == "custom" + assert base_url == "https://example.com/v1" + + def test_hardcoded_allowlist_still_works_when_both_registries_unavailable(self, monkeypatch): + """If both the built-in registry and the user-defined config are + unavailable (e.g. early import path before config is loaded), the + existing hardcoded allowlist still returns True for known names — + so xai-oauth / qwen-oauth / etc. aren't regressed by the fix. + """ + import agent.auxiliary_client as ac + + def _boom(_name): + raise RuntimeError("catalog unavailable") + + with patch( + "hermes_cli.runtime_provider._get_named_custom_provider", + side_effect=_boom, + ), patch( + "hermes_cli.providers.get_provider", side_effect=_boom, + ): + resolved_provider, _model, _base_url, _api_key, _api_mode = ( + ac._resolve_task_provider_model( + task="moa_reference", + provider="xai-oauth", + model="grok-3", + base_url="https://api.x.ai/v1", + api_key="xai-token", + ) + ) + + assert resolved_provider == "xai-oauth" + + def test_builtin_registry_raises_does_not_suppress_named_custom_lookup(self, monkeypatch): + """#76602 (review feedback) — a partial catalog-load failure in the + built-in registry must not short-circuit the user-defined + provider lookup. The two lookups are now parallel and + independent; an exception in one does not suppress the other. + + Before the refactor, ``get_provider`` raising jumped the outer + ``except`` to the hardcoded allowlist fallback and never + consulted ``_get_named_custom_provider`` — so a configured + named provider was still downgraded to ``"custom"`` whenever the + built-in catalog failed to load. The user's repro path + (Hermes desktop on Windows with a partial / early-startup + catalog state) hits this branch. + """ + import agent.auxiliary_client as ac + + def _catalog_raises(_name): + raise RuntimeError("built-in catalog unavailable") + + fake_entry = { + "name": "AgnesAI", + "base_url": "https://api.agnes-ai.cn/v1", + "api_key": "sk-agnes-from-config", + "model": "agnes-2.5-flash", + } + with patch( + "hermes_cli.runtime_provider._get_named_custom_provider", + return_value=fake_entry, + ), patch( + "hermes_cli.providers.get_provider", side_effect=_catalog_raises, + ): + resolved_provider, _model, base_url, _api_key, _api_mode = ( + ac._resolve_task_provider_model( + task="vision", + provider="agnes-ai.cn", + model="agnes-2.5-flash", + base_url="https://api.agnes-ai.cn/v1", + api_key=None, + ) + ) + + assert resolved_provider == "agnes-ai.cn", ( + "Built-in registry raising must not suppress the named-custom " + "lookup; the user-defined provider must remain named (review " + "feedback on #76602 — partial-load failure path)" + ) + assert base_url == "https://api.agnes-ai.cn/v1" + + def test_resolve_vision_provider_client_preserves_named_provider_via_config(self, tmp_path): + """#76602 (review feedback) — integration test through the real + ``resolve_vision_provider_client`` entry point with a real + ``HERMES_HOME`` config.yaml, mirroring the pattern from + ``tests/agent/test_auxiliary_named_custom_providers.py``. + + Before the fix the repro in the issue body returned + ``('custom', 'no-key-required')`` for this exact config shape; + after the fix the named provider is preserved and the inline + ``api_key`` from the providers: entry reaches the client. + """ + import yaml + + hermes_home = tmp_path / ".hermes" + hermes_home.mkdir() + (hermes_home / "config.yaml").write_text(yaml.dump({ + "model": {"default": "test-model"}, + "providers": { + "agnes-ai.cn": { + "name": "AgnesAI", + "base_url": "https://api.agnes-ai.cn/v1", + "api_key": "sk-agnes-test", + "model": "agnes-2.5-flash", + }, + }, + })) + import os + old_home = os.environ.get("HERMES_HOME") + os.environ["HERMES_HOME"] = str(hermes_home) + try: + from agent.auxiliary_client import resolve_vision_provider_client + + resolved_provider, client, _model = resolve_vision_provider_client( + provider="agnes-ai.cn", + model="agnes-2.5-flash", + base_url="https://api.agnes-ai.cn/v1", + api_key=None, + ) + finally: + if old_home is None: + os.environ.pop("HERMES_HOME", None) + else: + os.environ["HERMES_HOME"] = old_home + + assert resolved_provider == "agnes-ai.cn", ( + "resolve_vision_provider_client must preserve a named " + "user-defined provider + explicit base_url rather than " + "downgrading to 'custom' (issue #76602 repro)" + ) + # The inline api_key from the providers: entry must reach the + # client — this is what the 'custom' downgrade was losing. + assert client.api_key == "sk-agnes-test" +