diff --git a/agent/auxiliary_client.py b/agent/auxiliary_client.py index 3a8a04a01e51..2e2cdec689d7 100644 --- a/agent/auxiliary_client.py +++ b/agent/auxiliary_client.py @@ -517,6 +517,42 @@ def _get_aux_model_for_provider(provider_id: str) -> str: # can still use this dict directly. Kept in sync with _FALLBACK above. _API_KEY_PROVIDER_AUX_MODELS: Dict[str, str] = _API_KEY_PROVIDER_AUX_MODELS_FALLBACK +# Provider names that mean "whatever lane the main runtime resolved" rather +# than a user-declared endpoint. A blank model on one of these must keep +# inheriting the main chat model, so callers of +# :func:`_named_provider_default_model` skip them. Bare ``custom`` belongs +# here: it is the anonymous OPENAI_BASE_URL / ``model.base_url`` endpoint the +# main lane already owns, not a named ``providers:`` row. +_MAIN_LANE_PROVIDER_SENTINELS = frozenset({"auto", "main", "custom"}) + + +def _named_provider_default_model(provider: Optional[str]) -> Optional[str]: + """Return the default model a named provider entry declares, or None. + + Both config shapes are covered because ``_get_named_custom_provider`` + normalises them onto one key: ``providers..default_model`` (dict + shape) and ``custom_providers[].model`` (legacy list shape) both arrive + as ``entry["model"]``. + + Callers filter the main-lane sentinels themselves rather than having this + helper do it — ``auxiliary.route`` normalises a bare ``base_url`` to + ``provider: custom`` and still wants the lookup for an entry a user + literally named ``custom``. + """ + name = (provider or "").strip() + if not name: + return None + try: + from hermes_cli.runtime_provider import _get_named_custom_provider + + entry = _get_named_custom_provider(name) + except Exception: + return None + if not entry: + return None + return str(entry.get("model") or "").strip() or None + + # Vision-specific model overrides for direct providers. # When the user's main provider has a dedicated vision/multimodal model that # differs from their main chat model, map it here. The vision auto-detect @@ -6468,14 +6504,9 @@ def _auxiliary_route_target( # (resolve_provider_client, "if not model and provider != auto") fills # it from ``_read_main_model()`` — i.e. it sends the MAIN model's id to # the auxiliary endpoint, which is the one thing this lane must not do. - try: - from hermes_cli.runtime_provider import _get_named_custom_provider - - entry = _get_named_custom_provider(named_provider) - except Exception: - entry = None - if entry: - model = str(entry.get("model") or "").strip() or None + # ``_resolve_task_provider_model`` applies the same rule to the + # per-task ``auxiliary..provider`` pin. + model = _named_provider_default_model(named_provider) api_mode = str(route.get("api_mode", "") or "").strip() or None return { "provider": provider, @@ -6832,6 +6863,33 @@ def _preserve_provider_with_base_url(prov: Optional[str]) -> bool: if cfg_provider: cfg_provider, cfg_base_url = _expand_direct_api_alias(cfg_provider, cfg_base_url) + # ── Named lane, no model pinned → that lane's own default model ────── + # ``auxiliary..provider: my-local-lane`` with no ``model:`` reads as + # "use that provider's default model". Leave the slot blank and + # resolve_provider_client's universal fallback ("if not model and provider + # != auto", :4923) fills it from ``_read_main_model()`` — it sends the MAIN + # chat model's id to an endpoint that may not serve that model at all, + # which is a 404 from a local lane rather than an answer. The entry's own + # ``default_model`` is the user's stated intent, so it wins first; the + # provider catalog default and ``_read_main_model()`` still backstop a lane + # that declares no model of its own. + # + # ``auxiliary.route`` already resolves its lane this way + # (_auxiliary_route_target); PR #340 deliberately left this per-task path + # alone to keep its blast radius to the route it was adding. + # + # Runs AFTER the alias expansion above so a direct-API alias + # (``provider: openai`` → custom + api.openai.com) is a sentinel by then + # and cannot pair a same-named entry's model with the aliased endpoint. + # + # The main-lane sentinels are excluded on purpose: ``auto``, ``main`` and + # bare ``custom`` all mean "the lane the main runtime already resolved", so + # a blank model there must keep inheriting the main chat model. + if not resolved_model: + lane_provider = str(provider or cfg_provider or "").strip() + if lane_provider and lane_provider.lower() not in _MAIN_LANE_PROVIDER_SENTINELS: + resolved_model = _named_provider_default_model(lane_provider) + # An explicit provider arg without an explicit base_url must not bypass # the task's configured endpoint: adopt auxiliary..base_url/api_key # when the config targets the same provider (or names none), so the diff --git a/cli-config.yaml.example b/cli-config.yaml.example index 7ab96d873f89..9dfed5a489f0 100644 --- a/cli-config.yaml.example +++ b/cli-config.yaml.example @@ -513,7 +513,10 @@ prompt_caching: # # Model: leave empty to use the provider's default. When empty, OpenRouter # uses "google/gemini-3-flash-preview" and Nous uses "gemini-3-flash". -# Other providers pick a sensible default automatically. +# Other providers pick a sensible default automatically. When the provider +# names one of your own providers:/custom_providers: entries, empty means that +# entry's own default_model — not your main chat model, which the endpoint may +# not serve at all. "auto" and "main" still use your main chat model. # # auxiliary: # # One lane for EVERY task that pins nothing of its own — smart approval, diff --git a/tests/agent/test_auxiliary_task_provider_model.py b/tests/agent/test_auxiliary_task_provider_model.py new file mode 100644 index 000000000000..821fd5f00ac1 --- /dev/null +++ b/tests/agent/test_auxiliary_task_provider_model.py @@ -0,0 +1,326 @@ +"""``auxiliary..provider: `` with no ``model:`` — which model ships. + +WHY THIS FILE EXISTS AS AN END-TO-END SUITE + +The bug it pins is invisible to any test that stubs the client boundary: the +provider, the base_url and the api_key were all resolved correctly, so a +resolution-level assertion on "did we reach the right endpoint" passed while +the request carried the wrong ``model`` id. ``resolve_provider_client``'s +universal fallback (``if not model and provider != "auto"``) filled the blank +from ``_read_main_model()``, so a lane pinned at a local endpoint received the +MAIN chat model's slug — a 404 from a backend that serves a different model +set, not an answer. + +So these tests drive the REAL resolution path against a temp ``HERMES_HOME`` +with a real ``config.yaml`` and assert on the ``model`` kwarg that reaches +``.chat.completions.create()``. ``auxiliary.route`` already resolved its lane +this way (see ``test_auxiliary_route.py``); this is the per-task twin, which +PR #340 deliberately left out of scope. +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest +import yaml + +import agent.auxiliary_client as aux +from agent.auxiliary_client import ( + _named_provider_default_model, + _resolve_task_provider_model, + call_llm, + get_text_auxiliary_client, + resolve_vision_provider_client, +) + +LANE_URL = "http://127.0.0.1:8080/v1" +LANE_MODEL = "qwen3-4b-local" +MAIN = {"provider": "deepseek", "default": "deepseek-chat"} + + +@pytest.fixture +def hermes_home(tmp_path, monkeypatch): + """Isolated HERMES_HOME with a real config.yaml the loaders will read.""" + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setattr(Path, "home", lambda: tmp_path) + monkeypatch.setenv("HERMES_HOME", str(home)) + return home + + +@pytest.fixture(autouse=True) +def _clean_client_cache(monkeypatch): + """The client cache is keyed without the model — a leaked entry would let + one test's resolved model answer for the next one.""" + for key in ("OPENAI_BASE_URL", "OPENAI_API_KEY", "OPENROUTER_API_KEY", + "DEEPSEEK_API_KEY", "ANTHROPIC_API_KEY"): + monkeypatch.delenv(key, raising=False) + aux._client_cache.clear() + aux._aux_unhealthy_until.clear() + yield + aux._client_cache.clear() + aux._aux_unhealthy_until.clear() + + +def write_config(home: Path, data: dict) -> None: + (home / "config.yaml").write_text(yaml.safe_dump(data), encoding="utf-8") + + +def lane_config(task_block: dict, *, entry: dict | None = None, + legacy: bool = False) -> dict: + """Config with a main model plus one user-declared provider entry. + + ``legacy`` writes the ``custom_providers:`` list shape (``model:``) + instead of the ``providers:`` dict shape (``default_model:``). + """ + cfg: dict = { + "model": dict(MAIN), + "approvals": {"mode": "smart", "timeout": 60}, + "auxiliary": task_block, + } + if legacy: + cfg["custom_providers"] = [ + {"name": "my-local-lane", "base_url": LANE_URL, + **({"model": LANE_MODEL} if entry is None else entry)}, + ] + else: + cfg["providers"] = { + "my-local-lane": { + "api": LANE_URL, + **({"default_model": LANE_MODEL} if entry is None else entry), + }, + } + return cfg + + +def reply(text: str = "APPROVE"): + return MagicMock(choices=[MagicMock(message=MagicMock(content=text))]) + + +def wire_model(task: str = "approval") -> str: + """Run one real ``call_llm`` and return the model id that hit the wire. + + Only the OpenAI client construction is stubbed, so task config → + ``_resolve_task_provider_model`` → ``resolve_provider_client`` → + ``_build_call_kwargs`` all run for real. + """ + client = MagicMock() + client.base_url = LANE_URL + client.api_key = "no-key-required" + client.chat.completions.create.return_value = reply() + # The client cache is keyed without the model, so a second call in the + # same test would be served from the first call's entry and this stub + # would never see a request. + aux._client_cache.clear() + with patch.object(aux, "_create_openai_client", return_value=client): + call_llm(messages=[{"role": "user", "content": "hi"}], task=task) + return client.chat.completions.create.call_args.kwargs["model"] + + +# ────────────────────────────────────────────────────────────────────── +# The headline: a named lane with no model pinned +# ────────────────────────────────────────────────────────────────────── + + +class TestNamedLaneWithoutAModel: + def test_entrys_default_model_is_sent_not_the_main_chat_model(self, hermes_home): + """THE regression test. + + ``auxiliary.approval.provider: my-local-lane`` with no ``model:`` used + to send ``deepseek-chat`` — the MAIN chat model — to a local endpoint + that has never heard of it. + """ + write_config(hermes_home, lane_config({"approval": {"provider": "my-local-lane"}})) + + assert wire_model() == LANE_MODEL + assert wire_model() != MAIN["default"] + + def test_resolution_reports_the_entrys_model(self, hermes_home): + write_config(hermes_home, lane_config({"approval": {"provider": "my-local-lane"}})) + + provider, model, base_url, _key, _mode = _resolve_task_provider_model("approval") + assert provider == "my-local-lane" + assert model == LANE_MODEL + assert base_url is None # the entry owns the endpoint, not the task + + def test_the_client_is_built_against_the_lane_with_that_model(self, hermes_home): + """The endpoint was always right; only the model id was wrong.""" + write_config(hermes_home, lane_config({"compression": {"provider": "my-local-lane"}})) + + client, model = get_text_auxiliary_client("compression") + assert client is not None + assert str(client.base_url).rstrip("/") == LANE_URL + assert model == LANE_MODEL + + def test_custom_colon_name_spelling_resolves_the_same_way(self, hermes_home): + """``custom:`` is the canonical menu key for the same entry.""" + write_config(hermes_home, lane_config( + {"approval": {"provider": "custom:my-local-lane"}})) + + _provider, model, _base, _key, _mode = _resolve_task_provider_model("approval") + assert model == LANE_MODEL + + def test_legacy_custom_providers_list_shape_is_covered(self, hermes_home): + """The legacy list shape spells it ``model:``, the dict shape + ``default_model:`` — both arrive as ``entry["model"]``.""" + write_config(hermes_home, lane_config( + {"approval": {"provider": "my-local-lane"}}, legacy=True)) + + _provider, model, _base, _key, _mode = _resolve_task_provider_model("approval") + assert model == LANE_MODEL + assert wire_model() == LANE_MODEL + + @pytest.mark.parametrize("model_key", ["model", "default_model"]) + def test_both_legacy_model_spellings_resolve(self, hermes_home, model_key): + """``_normalize_custom_provider_entry`` accepts either spelling on a + list entry, so neither may be dropped here.""" + write_config(hermes_home, lane_config( + {"approval": {"provider": "my-local-lane"}}, + entry={"base_url": LANE_URL, model_key: LANE_MODEL}, + legacy=True)) + + _provider, model, _base, _key, _mode = _resolve_task_provider_model("approval") + assert model == LANE_MODEL + + +# ────────────────────────────────────────────────────────────────────── +# What must NOT change +# ────────────────────────────────────────────────────────────────────── + + +class TestUnchangedBehaviour: + def test_a_task_level_model_pin_still_wins(self, hermes_home): + write_config(hermes_home, lane_config( + {"approval": {"provider": "my-local-lane", "model": "pinned-tiny"}})) + + _provider, model, _base, _key, _mode = _resolve_task_provider_model("approval") + assert model == "pinned-tiny" + assert wire_model() == "pinned-tiny" + + def test_an_explicit_call_model_still_wins(self, hermes_home): + write_config(hermes_home, lane_config({"approval": {"provider": "my-local-lane"}})) + + _provider, model, _base, _key, _mode = _resolve_task_provider_model( + "approval", model="caller-choice") + assert model == "caller-choice" + + @pytest.mark.parametrize("sentinel", ["auto", "main", "custom"]) + def test_main_lane_sentinels_keep_inheriting_the_main_model( + self, hermes_home, sentinel, + ): + """``auto`` / ``main`` / bare ``custom`` all mean "the lane the main + runtime resolved". Even with an entry literally named ``custom`` on + disk, their blank model must still fall through to + ``_read_main_model()`` in resolve_provider_client.""" + cfg = lane_config({"approval": {"provider": sentinel}}) + cfg["providers"]["custom"] = {"api": "http://127.0.0.1:9999/v1", + "default_model": "shadow-model"} + write_config(hermes_home, cfg) + + _provider, model, _base, _key, _mode = _resolve_task_provider_model("approval") + assert model is None + + def test_model_auto_sentinel_still_drops_to_none_for_a_named_lane(self, hermes_home): + """``model: auto`` is not a model id. It is nulled before the lane + lookup, so the lane's own default fills the slot instead of the + literal string reaching the wire.""" + write_config(hermes_home, lane_config( + {"approval": {"provider": "my-local-lane", "model": "auto"}})) + + _provider, model, _base, _key, _mode = _resolve_task_provider_model("approval") + assert model == LANE_MODEL + + def test_entry_without_a_default_model_falls_back_as_before(self, hermes_home): + """No ``default_model`` on the entry → nothing to prefer, so the old + chain (provider catalog default, then the main model) still runs.""" + write_config(hermes_home, lane_config( + {"approval": {"provider": "my-local-lane"}}, entry={"api": LANE_URL})) + + _provider, model, _base, _key, _mode = _resolve_task_provider_model("approval") + assert model is None + assert wire_model() == MAIN["default"] + + def test_a_first_class_provider_is_untouched(self, hermes_home): + """``_get_named_custom_provider`` defers to canonical built-ins, so a + task pinned at one resolves exactly as before.""" + write_config(hermes_home, lane_config({"approval": {"provider": "nous"}})) + + provider, model, _base, _key, _mode = _resolve_task_provider_model("approval") + assert provider == "nous" + assert model is None + + def test_a_task_that_pins_nothing_is_untouched(self, hermes_home): + write_config(hermes_home, lane_config({"approval": {}})) + + assert _resolve_task_provider_model("approval") == ("auto", None, None, None, None) + + +# ────────────────────────────────────────────────────────────────────── +# The other two doors into the same resolver +# ────────────────────────────────────────────────────────────────────── + + +class TestVisionAndAsyncPaths: + def test_vision_lane_uses_the_entrys_default_model(self, hermes_home): + """``resolve_vision_provider_client`` funnels through the same + resolver, so the vision lane carried the main model too.""" + write_config(hermes_home, lane_config({"vision": {"provider": "my-local-lane"}})) + + provider, client, model = resolve_vision_provider_client() + assert provider == "my-local-lane" + assert client is not None + assert model == LANE_MODEL + assert model != MAIN["default"] + + def test_vision_provider_main_still_inherits_the_main_model(self, hermes_home): + """The sentinel guard has to hold on the vision path as well — + ``_normalize_vision_provider`` resolves ``main`` away before + ``resolve_provider_client`` ever sees the raw name.""" + cfg = lane_config({"vision": {"provider": "main"}}) + cfg["model"] = {"provider": "custom:my-local-lane", "default": "deepseek-chat"} + write_config(hermes_home, cfg) + + _provider, model, _base, _key, _mode = _resolve_task_provider_model("vision") + assert model is None + + def test_async_client_gets_the_entrys_default_model(self, hermes_home): + from agent.auxiliary_client import get_async_text_auxiliary_client + + write_config(hermes_home, lane_config({"compression": {"provider": "my-local-lane"}})) + + client, model = get_async_text_auxiliary_client("compression") + assert client is not None + assert model == LANE_MODEL + + +# ────────────────────────────────────────────────────────────────────── +# The shared helper (also feeds _auxiliary_route_target) +# ────────────────────────────────────────────────────────────────────── + + +class TestNamedProviderDefaultModelHelper: + def test_reads_the_providers_dict_shape(self, hermes_home): + write_config(hermes_home, lane_config({})) + assert _named_provider_default_model("my-local-lane") == LANE_MODEL + + def test_reads_the_legacy_list_shape(self, hermes_home): + write_config(hermes_home, lane_config({}, legacy=True)) + assert _named_provider_default_model("my-local-lane") == LANE_MODEL + + def test_returns_none_for_an_unknown_name(self, hermes_home): + write_config(hermes_home, lane_config({})) + assert _named_provider_default_model("not-configured") is None + + def test_returns_none_for_blank_input(self, hermes_home): + write_config(hermes_home, lane_config({})) + assert _named_provider_default_model("") is None + assert _named_provider_default_model(None) is None + + def test_survives_a_broken_config_loader(self, hermes_home): + """Model resolution must never be the thing that takes a turn down.""" + write_config(hermes_home, lane_config({})) + with patch("hermes_cli.runtime_provider._get_named_custom_provider", + side_effect=RuntimeError("config exploded")): + assert _named_provider_default_model("my-local-lane") is None diff --git a/website/docs/user-guide/configuration.md b/website/docs/user-guide/configuration.md index ac3a2a95c388..829710675823 100644 --- a/website/docs/user-guide/configuration.md +++ b/website/docs/user-guide/configuration.md @@ -1011,6 +1011,28 @@ When `base_url` is set, Hermes ignores the provider and calls that endpoint dire Available providers for auxiliary tasks: `auto`, `main`, plus any provider in the [provider registry](/reference/environment-variables) — `openrouter`, `nous`, `openai-codex`, `copilot`, `copilot-acp`, `anthropic`, `gemini`, `qwen-oauth`, `zai`, `kimi-coding`, `kimi-coding-cn`, `minimax`, `minimax-cn`, `minimax-oauth`, `deepseek`, `nvidia`, `xai`, `xai-oauth`, `ollama-cloud`, `alibaba`, `bedrock`, `huggingface`, `arcee`, `xiaomi`, `kilocode`, `opencode-zen`, `opencode-go`, `azure-foundry` — or any named custom provider from your `custom_providers` list (e.g. `provider: "beans"`). +**Pointing a task at one of your own provider entries.** When `provider` names a +`providers:` or `custom_providers:` entry and you leave `model` empty, the task +uses that entry's own `default_model`: + +```yaml +providers: + my-local-lane: + api: "http://127.0.0.1:8080/v1" + default_model: "qwen3-4b" + +auxiliary: + approval: + provider: "my-local-lane" # no model: → sends qwen3-4b +``` + +That matters most for local endpoints, which serve a different model set than +your main provider: sending your main chat model's id there is a 404, not an +answer. `auto` and `main` are unchanged — they still mean "use my main chat +model" — and an explicit `model:` on the task always wins. If the entry +declares no `default_model`, the task falls back to your main chat model as +before. + :::tip MiniMax OAuth `minimax-oauth` logs in via browser OAuth (no API key needed). Run `hermes model` and select **MiniMax (OAuth)** to authenticate. Auxiliary tasks use `MiniMax-M2.7-highspeed` automatically. See the [MiniMax OAuth guide](../guides/minimax-oauth.md). :::