From 759b2a98cf819abbca2ee8399e80e871ecf97005 Mon Sep 17 00:00:00 2001 From: kangxl <230263957+zh-xl-kang@users.noreply.github.com> Date: Thu, 2 Jul 2026 15:00:18 +0800 Subject: [PATCH] fix: preserve provider identity when auxiliary task has base_url MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an auxiliary task (e.g. vision) has a base_url configured, _resolve_task_provider_model was collapsing known providers to 'custom' because the base_url triggered the 'unknown endpoint' heuristic. This broke credential resolution — provider 'custom' looks for OPENAI_API_KEY instead of the provider-specific key (e.g. alibaba → DASHSCOPE_API_KEY). Fix: when the caller explicitly passes a known provider name, preserve it regardless of base_url. The base_url only indicates a proxy/mirror, not an unknown vendor. - agent/auxiliary_client.py: keep provider string when it matches a known provider, even with custom base_url - tests/agent/test_vision_provider_base_url.py: 135 + 86 lines of regression tests covering direct-argument, config-driven, and end-to-end credential resolution paths --- agent/auxiliary_client.py | 7 + tests/agent/test_vision_provider_base_url.py | 221 +++++++++++++++++++ 2 files changed, 228 insertions(+) create mode 100644 tests/agent/test_vision_provider_base_url.py diff --git a/agent/auxiliary_client.py b/agent/auxiliary_client.py index 0afb0add20bf6..32d4d059b9aec 100644 --- a/agent/auxiliary_client.py +++ b/agent/auxiliary_client.py @@ -4948,6 +4948,13 @@ def _expand_direct_api_alias(prov: Optional[str], existing_base: Optional[str]) cfg_provider, cfg_base_url = _expand_direct_api_alias(cfg_provider, cfg_base_url) if base_url: + # Preserve a known (non-auto, non-empty) provider so each provider + # resolves credentials from its own env vars (e.g. DASHSCOPE_API_KEY + # for alibaba, OPENROUTER_API_KEY for openrouter). Without this, + # base_url forces "custom" which reads OPENAI_API_KEY — wrong key + # for every non-OpenAI provider. + if provider and provider not in {"auto", ""}: + return provider, resolved_model, base_url, api_key, resolved_api_mode return "custom", resolved_model, base_url, api_key, resolved_api_mode if provider: return provider, resolved_model, base_url, api_key, resolved_api_mode diff --git a/tests/agent/test_vision_provider_base_url.py b/tests/agent/test_vision_provider_base_url.py new file mode 100644 index 0000000000000..0179c4af45972 --- /dev/null +++ b/tests/agent/test_vision_provider_base_url.py @@ -0,0 +1,221 @@ +"""Regression test: _resolve_task_provider_model preserves provider when base_url is set. + +Before the fix, passing both ``provider`` and ``base_url`` as direct arguments +forced provider to ``"custom"`` regardless of whether the caller specified a +known provider (e.g. ``"alibaba"``). This caused vision/auxiliary calls to +resolve credentials from the wrong env var (OPENAI_API_KEY instead of +DASHSCOPE_API_KEY) and fail with 401. + +The fix preserves the caller's provider when it is a non-empty, non-"auto" +value, so each provider resolves credentials from its own env vars. +""" + +from __future__ import annotations + +import os +import shutil +import sys +import tempfile + +import pytest + + +@pytest.fixture +def isolated_home(monkeypatch): + """Temp HERMES_HOME with clean credential env vars.""" + test_home = tempfile.mkdtemp(prefix="hermes_test_vision_burl_") + hermes_home = os.path.join(test_home, ".hermes") + os.makedirs(hermes_home) + monkeypatch.setenv("HERMES_HOME", hermes_home) + + # Write a minimal config so load_config() doesn't fail. + with open(os.path.join(hermes_home, "config.yaml"), "w") as fp: + fp.write("model:\n default: test-model\n") + + # Strip credential env vars for hermetic tests. + for k in list(os.environ.keys()): + if k.endswith("_API_KEY") or k.endswith("_TOKEN"): + monkeypatch.delenv(k, raising=False) + + yield hermes_home + shutil.rmtree(test_home, ignore_errors=True) + + +def _fresh_modules(): + """Drop cached hermes modules so each test reloads cleanly.""" + for mod in list(sys.modules.keys()): + if mod.startswith(("agent.auxiliary_client", "hermes_cli.config")): + del sys.modules[mod] + + +class TestProviderPreservedWithBaseUrl: + """Direct provider + base_url args must not collapse to 'custom'.""" + + def test_known_provider_preserved_with_base_url(self, isolated_home): + """provider='alibaba' + base_url should return 'alibaba', not 'custom'.""" + _fresh_modules() + from agent.auxiliary_client import _resolve_task_provider_model + + provider, model, base_url, api_key, api_mode = _resolve_task_provider_model( + task=None, + provider="alibaba", + model="qwen-vl-max", + base_url="https://dashscope.aliyuncs.com/compatible-mode/v1", + api_key="sk-test", + ) + assert provider == "alibaba", ( + f"Expected 'alibaba' but got '{provider}'. " + "base_url should not force provider to 'custom' when a known " + "provider is explicitly specified." + ) + assert base_url == "https://dashscope.aliyuncs.com/compatible-mode/v1" + assert model == "qwen-vl-max" + + def test_auto_provider_still_collapses_to_custom(self, isolated_home): + """provider='auto' + base_url should still return 'custom' (unchanged behavior).""" + _fresh_modules() + from agent.auxiliary_client import _resolve_task_provider_model + + provider, _model, base_url, _key, _mode = _resolve_task_provider_model( + task=None, + provider="auto", + base_url="https://some-proxy.example.com/v1", + ) + assert provider == "custom", ( + "provider='auto' with base_url should still resolve to 'custom'" + ) + + def test_empty_provider_still_collapses_to_custom(self, isolated_home): + """provider='' + base_url should still return 'custom' (unchanged behavior).""" + _fresh_modules() + from agent.auxiliary_client import _resolve_task_provider_model + + provider, _model, base_url, _key, _mode = _resolve_task_provider_model( + task=None, + provider="", + base_url="https://some-proxy.example.com/v1", + ) + assert provider == "custom" + + def test_none_provider_still_collapses_to_custom(self, isolated_home): + """provider=None + base_url should still return 'custom' (unchanged behavior).""" + _fresh_modules() + from agent.auxiliary_client import _resolve_task_provider_model + + provider, _model, _base_url, _key, _mode = _resolve_task_provider_model( + task=None, + provider=None, + base_url="https://some-proxy.example.com/v1", + ) + assert provider == "custom" + + def test_openrouter_provider_preserved_with_base_url(self, isolated_home): + """Any known provider should be preserved — not just 'alibaba'.""" + _fresh_modules() + from agent.auxiliary_client import _resolve_task_provider_model + + provider, _model, base_url, _key, _mode = _resolve_task_provider_model( + task=None, + provider="openrouter", + base_url="https://my-proxy.example.com/v1", + api_key="sk-or-test", + ) + assert provider == "openrouter" + assert base_url == "https://my-proxy.example.com/v1" + + def test_base_url_without_provider_still_custom(self, isolated_home): + """base_url alone (no provider arg) should return 'custom'.""" + _fresh_modules() + from agent.auxiliary_client import _resolve_task_provider_model + + provider, _model, _base_url, _key, _mode = _resolve_task_provider_model( + task=None, + base_url="https://some-proxy.example.com/v1", + ) + assert provider == "custom" + + +class TestProviderFromConfigWithBaseUrl: + """When auxiliary task config specifies provider + base_url, + the config values must flow through _resolve_task_provider_model + and preserve the provider (not collapse to 'custom').""" + + def test_config_provider_preserved_via_task(self, isolated_home): + """Simulate real usage: config.yaml has auxiliary.vision.provider=alibaba + and auxiliary.vision.base_url, task='vision' triggers config lookup. + + This exercises the config-driven branch (cfg_base_url + cfg_provider, + no cfg_api_key) rather than the direct-argument branch covered above. + """ + _fresh_modules() + # Overwrite the fixture's minimal config with one that mirrors + # real-world per-task auxiliary usage. + config_path = os.path.join(os.environ["HERMES_HOME"], "config.yaml") + with open(config_path, "w") as fp: + fp.write( + "model:\n" + " default: test-model\n" + "auxiliary:\n" + " vision:\n" + " provider: alibaba\n" + " base_url: https://dashscope.aliyuncs.com/compatible-mode/v1\n" + ) + + from agent.auxiliary_client import _resolve_task_provider_model + + provider, _model, base_url, api_key, _mode = _resolve_task_provider_model( + task="vision", + ) + assert provider == "alibaba", ( + f"Expected config provider 'alibaba' but got '{provider}'. " + "A config-supplied base_url without api_key must keep the known " + "provider instead of collapsing to 'custom'." + ) + assert base_url == "https://dashscope.aliyuncs.com/compatible-mode/v1" + # No api_key in config → resolution is deferred to the provider's own + # env vars downstream (see TestCredentialResolutionEndToEnd). + assert api_key is None + + +class TestCredentialResolutionEndToEnd: + """The whole point of preserving provider is to read the right env var. + + _resolve_task_provider_model itself does not read credentials from the + environment — it only returns the *provider string*. The credential + lookup happens downstream in + ``hermes_cli.auth.resolve_api_key_provider_credentials`` which maps the + provider to its api_key_env_vars (alibaba → DASHSCOPE_API_KEY). This test + chains both steps so the "provider drives the right key" linkage is proven + end-to-end, not asserted by construction. + """ + + def test_alibaba_provider_reads_dashscope_api_key(self, isolated_home, monkeypatch): + _fresh_modules() + monkeypatch.setenv("DASHSCOPE_API_KEY", "sk-dashscope-test") + monkeypatch.setenv("OPENAI_API_KEY", "sk-openai-should-not-be-used") + + from agent.auxiliary_client import _resolve_task_provider_model + + # Step 1: resolve the provider (alibaba preserved despite base_url). + provider, _model, _base_url, api_key, _mode = _resolve_task_provider_model( + task=None, + provider="alibaba", + base_url="https://dashscope.aliyuncs.com/compatible-mode/v1", + ) + assert provider == "alibaba" + # The resolver returns the *passed* api_key (None) — it never touches + # env vars. The real credential lookup happens in step 2. + assert api_key is None + + # Step 2: feed the *resolved* provider (not a hardcoded literal) into + # the credential resolver so we prove the linkage. + from hermes_cli.auth import resolve_api_key_provider_credentials + + creds = resolve_api_key_provider_credentials(provider) + assert creds["api_key"] == "sk-dashscope-test", ( + f"Expected DASHSCOPE_API_KEY but got api_key={creds['api_key']!r}. " + "Provider 'alibaba' should resolve credentials from DASHSCOPE_API_KEY, " + "not OPENAI_API_KEY." + ) + assert creds["api_key"] != "sk-openai-should-not-be-used" + assert creds["source"] == "DASHSCOPE_API_KEY"