From 2755bf558e6dd0d557bd8a1fa1ebbb932b689ec3 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 22 Jul 2026 22:01:35 -0700 Subject: [PATCH] fix(auxiliary): route all MoA aux resolution through one shared aggregator helper Follow-up to srojk34's explicit-provider unwrap (PR #56691): - Extract _resolve_moa_aggregator() as the single preset->aggregator resolver shared by _resolve_auto(), _resolve_task_provider_model(), and resolve_provider_client() so preset lookup/validation can't drift. - When the main provider is moa, the aggregator model is now the default for every UNSET auxiliary model: _read_main_model_for_aux() substitutes the preset's acting (aggregator) model wherever fallback chains pre-filled from _read_main_model() (router prefill, custom-endpoint fallback, named-custom default, external-process default, _try_main_agent_model_fallback). - Unwrap moa at the resolve_provider_client() chokepoint so direct callers (vision auto-detect, plugin code) can't dead-end in the unknown-provider branch, and unwrap the vision auto-detect main provider before capability probes run against the preset name. - Real-config tests: temp HERMES_HOME + actual config.yaml exercising the genuine load_config()/resolve_moa_preset() boundary. --- agent/auxiliary_client.py | 163 ++++++++++++++++++++------- tests/agent/test_auxiliary_client.py | 145 ++++++++++++++++++++++++ 2 files changed, 270 insertions(+), 38 deletions(-) diff --git a/agent/auxiliary_client.py b/agent/auxiliary_client.py index 08107583ee14..7cb0849ea220 100644 --- a/agent/auxiliary_client.py +++ b/agent/auxiliary_client.py @@ -2303,6 +2303,62 @@ def _read_main_base_url() -> str: return "" +def _resolve_moa_aggregator(preset_name: Optional[str]) -> Tuple[Optional[str], Optional[str]]: + """Resolve a MoA preset to its aggregator (provider, model) pair. + + "moa" is a virtual provider — the acting model of a preset is its + aggregator slot, and there is no real "moa" HTTP endpoint. Auxiliary + tasks (title generation, compression, vision, commit messages, …) don't + need the reference fan-out, so every aux resolution layer maps + provider="moa"/model= to the aggregator's real provider+model + through this single helper (shared by ``_resolve_auto``, + ``_resolve_task_provider_model``, and ``resolve_provider_client`` so the + preset lookup and validation cannot drift between paths). + + Args: + preset_name: The MoA preset name (usually carried in the "model" + field), or None/"" to resolve the user's default preset. + + Returns: + (aggregator_provider, aggregator_model), or (None, None) when the + preset cannot be resolved (missing config, renamed/deleted preset, + or a malformed aggregator slot). + """ + try: + from hermes_cli.config import load_config + from hermes_cli.moa_config import resolve_moa_preset + + preset = resolve_moa_preset(load_config().get("moa") or {}, preset_name or None) + agg = preset.get("aggregator") or {} + agg_provider = str(agg.get("provider") or "").strip() + agg_model = str(agg.get("model") or "").strip() + if agg_provider and agg_model and agg_provider.lower() != "moa": + return agg_provider, agg_model + except Exception: + logger.debug( + "MoA aggregator resolution failed for preset %r", preset_name, exc_info=True + ) + return None, None + + +def _read_main_model_for_aux() -> str: + """Main model with MoA presets unwrapped to the aggregator's model. + + When the main provider is ``moa``, ``_read_main_model()`` returns a MoA + *preset name* (e.g. "opus-gpt") — never a valid wire model id on any + provider. Auxiliary fallback chains that pre-fill a missing model from + the main model must use this reader instead, so unset aux models default + to the preset's acting (aggregator) model. Returns "" when the main + provider is moa but the preset cannot be resolved — sending nothing is + strictly better than sending a preset name that 400s. + """ + model = _read_main_model() + if (_read_main_provider() or "").strip().lower() == "moa": + _, agg_model = _resolve_moa_aggregator(model) + return agg_model or "" + return model + + def _read_main_api_key_if_same_host(aux_base_url: str) -> str: """Return the main api_key only when *aux_base_url* points at the same host as the main model's base_url. @@ -2575,7 +2631,7 @@ def _try_custom_endpoint() -> Tuple[Optional[Any], Optional[str]]: return None, None if custom_base.lower().startswith(_CODEX_AUX_BASE_URL.lower()): return None, None - model = _read_main_model() or "gpt-4o-mini" + model = _read_main_model_for_aux() or "gpt-4o-mini" logger.debug("Auxiliary client: custom endpoint (%s, api_mode=%s)", model, custom_mode or "chat_completions") _clean_base, _dq = _extract_url_query_params(custom_base) _extra = {"default_query": _dq} if _dq else {} @@ -4045,6 +4101,13 @@ def _try_main_agent_model_fallback( """ main_provider = (_read_main_provider() or "").strip() main_model = (_read_main_model() or "").strip() + if main_provider.lower() == "moa": + # MoA virtual provider: fall back to the preset's aggregator — the + # acting model — instead of the unreachable "moa"/ pair. + _agg_provider, _agg_model = _resolve_moa_aggregator(main_model) + if not _agg_provider or not _agg_model: + return None, None, "" + main_provider, main_model = _agg_provider, _agg_model if not main_provider or not main_model or main_provider.lower() in {"auto", ""}: return None, None, "" @@ -4445,26 +4508,17 @@ def _resolve_auto( # model. Resolve the MoA preset to its aggregator slot and continue Step 1 # with that real provider+model. Mirrors the MoA context-length resolution. if main_provider == "moa": - try: - from hermes_cli.config import load_config - from hermes_cli.moa_config import resolve_moa_preset - - _preset = resolve_moa_preset(load_config().get("moa") or {}, main_model) - _agg = _preset.get("aggregator") or {} - _agg_provider = str(_agg.get("provider") or "").strip() - _agg_model = str(_agg.get("model") or "").strip() - if _agg_provider and _agg_model and _agg_provider.lower() != "moa": - main_provider = _agg_provider - main_model = _agg_model - # The MoA virtual runtime carries a non-HTTP base_url - # ("moa://local") and a placeholder api_key; they belong to the - # facade, not the aggregator's real provider. Drop them so the - # aggregator resolves through its own provider credentials. - runtime_base_url = "" - runtime_api_key = "" - runtime_api_mode = "" - except Exception: - logger.debug("MoA aux resolution to aggregator failed", exc_info=True) + _agg_provider, _agg_model = _resolve_moa_aggregator(main_model) + if _agg_provider and _agg_model: + main_provider = _agg_provider + main_model = _agg_model + # The MoA virtual runtime carries a non-HTTP base_url + # ("moa://local") and a placeholder api_key; they belong to the + # facade, not the aggregator's real provider. Drop them so the + # aggregator resolves through its own provider credentials. + runtime_base_url = "" + runtime_api_key = "" + runtime_api_mode = "" if (main_provider and main_model and main_provider not in {"auto", ""}): @@ -4719,6 +4773,27 @@ def resolve_provider_client( # Normalise aliases provider = _normalize_aux_provider(provider) + # MoA virtual provider chokepoint: "moa" is not a real HTTP provider — + # its acting model is the preset's aggregator slot. The two resolver + # layers above (_resolve_auto, _resolve_task_provider_model) already + # unwrap their own paths, but callers that route here directly (vision + # auto-detect, _try_main_agent_model_fallback, get_available_vision_backends, + # plugin code) would otherwise dead-end in the unknown-provider branch. + # ``model`` carries the preset name for moa calls; when the preset can't + # be resolved we leave the call untouched and let the normal + # missing-provider handling produce its diagnostic. + if provider == "moa": + _agg_provider, _agg_model = _resolve_moa_aggregator(model) + if _agg_provider and _agg_model: + original_provider = _agg_provider.strip().lower() + provider = _normalize_aux_provider(_agg_provider) + model = _agg_model + # The moa:// facade endpoint and placeholder key belong to the + # virtual runtime, not the aggregator's real provider. + if explicit_base_url and str(explicit_base_url).lower().startswith("moa://"): + explicit_base_url = None + explicit_api_key = None + # Universal model-resolution fallback for concrete providers. ``auto`` is # intentionally excluded: `_resolve_auto(main_runtime=...)` returns the # model paired with the provider it actually selected. Pre-filling an auto @@ -4739,6 +4814,10 @@ def resolve_provider_client( # the load-bearing step for OAuth providers: an xai-oauth user # with grok-4.3 configured gets grok-4.3 for title generation # instead of silently dropping to whatever Step-2 fallback (#31845). + # When the main provider is MoA, ``_read_main_model_for_aux()`` + # substitutes the preset's aggregator model — the preset NAME is + # never a valid wire model id, so unset aux models default to the + # preset's acting model instead. # # Each provider branch below sees a non-empty ``model`` whenever the # user has *anything* configured — no provider-specific empty-model @@ -4755,7 +4834,7 @@ def resolve_provider_client( # return the actual current runtime model when the caller did not explicitly # request one. (# compression-current-model) if not model and provider != "auto": - model = _get_aux_model_for_provider(provider) or _read_main_model() or model + model = _get_aux_model_for_provider(provider) or _read_main_model_for_aux() or model def _needs_codex_wrap(client_obj, base_url_str: str, model_str: str) -> bool: """Decide if a plain OpenAI client should be wrapped for Responses API. @@ -5029,7 +5108,7 @@ def resolve_provider_client( model or custom_entry.get("model") or (main_runtime.get("model") if main_runtime else None) - or _read_main_model() + or _read_main_model_for_aux() or "gpt-4o-mini", provider, ) @@ -5264,7 +5343,7 @@ def resolve_provider_client( final_model = _normalize_resolved_model( model or (main_runtime.get("model") if main_runtime else None) - or _read_main_model(), + or _read_main_model_for_aux(), provider, ) if provider == "copilot-acp": @@ -5632,7 +5711,24 @@ def resolve_vision_provider_client( # 5. Stop main_provider = str(runtime.get("provider") or _read_main_provider()) main_model = str(runtime.get("model") or _read_main_model()) - if main_provider and main_provider not in {"auto", ""}: + if main_provider.strip().lower() == "moa": + # MoA virtual provider: main_model is a preset NAME, and every + # capability probe below (_PROVIDERS_WITHOUT_VISION, + # _main_model_supports_vision, _resolve_provider_vision_default) + # would run against a provider/model pair that doesn't exist on + # any wire. Unwrap to the preset's aggregator slot first so the + # checks and the eventual client target the real acting model. + _agg_provider, _agg_model = _resolve_moa_aggregator(main_model) + if _agg_provider and _agg_model: + main_provider, main_model = _agg_provider, _agg_model + # Drop the moa:// facade endpoint from the runtime view used + # below — it belongs to the virtual provider, not the + # aggregator's real provider. + runtime = dict(runtime) + runtime["base_url"] = "" + runtime["api_key"] = "" + runtime["api_mode"] = "" + if main_provider and main_provider not in {"auto", "", "moa"}: # A provider-specific vision default wins over the user's chat model: # static overrides (xiaomi/zai) and catalog-backed discovery (the # DeepInfra profile hook) both yield a *known* vision-capable model, @@ -6290,22 +6386,13 @@ def _resolve_task_provider_model( # "MOA_API_KEY environment variable" error for a provider that was never # meant to be reached over the wire. Auxiliary tasks don't need the # reference fan-out — resolve to the preset's aggregator slot instead, - # exactly like the implicit path does. + # exactly like the implicit path does (shared helper: _resolve_moa_aggregator). def _unwrap_moa_provider(prov: str, mdl: Optional[str]) -> Tuple[str, Optional[str]]: if prov.strip().lower() != "moa": return prov, mdl - try: - from hermes_cli.config import load_config - from hermes_cli.moa_config import resolve_moa_preset - - preset = resolve_moa_preset(load_config().get("moa") or {}, mdl) - agg = preset.get("aggregator") or {} - agg_provider = str(agg.get("provider") or "").strip() - agg_model = str(agg.get("model") or "").strip() - if agg_provider and agg_model and agg_provider.lower() != "moa": - return agg_provider, agg_model - except Exception: - logger.debug("MoA aux resolution (explicit provider override) to aggregator failed", exc_info=True) + agg_provider, agg_model = _resolve_moa_aggregator(mdl) + if agg_provider and agg_model: + return agg_provider, agg_model return prov, mdl if provider and str(provider).strip().lower() == "moa": diff --git a/tests/agent/test_auxiliary_client.py b/tests/agent/test_auxiliary_client.py index 3d51e862523d..cdc34955e857 100644 --- a/tests/agent/test_auxiliary_client.py +++ b/tests/agent/test_auxiliary_client.py @@ -395,6 +395,151 @@ class TestResolveTaskProviderModel: assert model == "claude-haiku-4-5-20251001" +class TestMoaAggregatorSharedResolution: + """The shared MoA→aggregator helper and the layers that consume it. + + Real-config tests: write an actual config.yaml under a temp HERMES_HOME + and exercise the genuine load_config() → resolve_moa_preset() boundary — + no mocking of the configuration-resolution chain. + """ + + @staticmethod + def _write_moa_config(tmp_path, monkeypatch, default_preset="opus-gpt"): + import yaml + + home = tmp_path / ".hermes" + home.mkdir(exist_ok=True) + (home / "config.yaml").write_text( + yaml.safe_dump( + { + "moa": { + "default_preset": default_preset, + "presets": { + "opus-gpt": { + "enabled": True, + "reference_models": [ + {"provider": "openrouter", "model": "openai/gpt-5.5"} + ], + "aggregator": { + "provider": "openrouter", + "model": "anthropic/claude-opus-4.8", + }, + }, + "nous-mix": { + "enabled": True, + "reference_models": [ + {"provider": "nous", "model": "hermes-4-70b"} + ], + "aggregator": { + "provider": "nous", + "model": "hermes-4-405b", + }, + }, + }, + } + } + ) + ) + monkeypatch.setenv("HERMES_HOME", str(home)) + return home + + def test_real_config_explicit_task_provider_moa(self, tmp_path, monkeypatch): + """auxiliary..provider: moa in a REAL config.yaml resolves to the + aggregator through the genuine load_config()/resolve_moa_preset() path.""" + import yaml + + home = self._write_moa_config(tmp_path, monkeypatch) + cfg = yaml.safe_load((home / "config.yaml").read_text()) + cfg["auxiliary"] = {"title_generation": {"provider": "moa", "model": "opus-gpt"}} + (home / "config.yaml").write_text(yaml.safe_dump(cfg)) + + resolved_provider, model, base_url, api_key, api_mode = _resolve_task_provider_model( + task="title_generation", + ) + + assert resolved_provider == "openrouter" + assert model == "anthropic/claude-opus-4.8" + assert base_url is None + assert api_key is None + + def test_real_config_explicit_task_provider_moa_default_preset(self, tmp_path, monkeypatch): + """provider: moa with no model resolves the default preset's aggregator.""" + import yaml + + home = self._write_moa_config(tmp_path, monkeypatch, default_preset="nous-mix") + cfg = yaml.safe_load((home / "config.yaml").read_text()) + cfg["auxiliary"] = {"compression": {"provider": "moa"}} + (home / "config.yaml").write_text(yaml.safe_dump(cfg)) + + resolved_provider, model, base_url, api_key, api_mode = _resolve_task_provider_model( + task="compression", + ) + + assert resolved_provider == "nous" + assert model == "hermes-4-405b" + + def test_read_main_model_for_aux_unwraps_preset_name(self, tmp_path, monkeypatch): + """Main provider moa → the aux-facing main model is the aggregator's + model, so every unset auxiliary model defaults to the acting model + instead of the preset name.""" + from agent.auxiliary_client import _read_main_model_for_aux + + self._write_moa_config(tmp_path, monkeypatch) + with patch("agent.auxiliary_client._read_main_provider", return_value="moa"), \ + patch("agent.auxiliary_client._read_main_model", return_value="opus-gpt"): + assert _read_main_model_for_aux() == "anthropic/claude-opus-4.8" + + def test_read_main_model_for_aux_unresolvable_preset_returns_empty(self, tmp_path, monkeypatch): + """A moa main with a deleted/renamed preset yields "" — never the + preset name, which would 400 on any wire.""" + from agent.auxiliary_client import _read_main_model_for_aux + + self._write_moa_config(tmp_path, monkeypatch) + with patch("agent.auxiliary_client._read_main_provider", return_value="moa"), \ + patch("agent.auxiliary_client._read_main_model", return_value="gone-preset"): + assert _read_main_model_for_aux() == "" + + def test_read_main_model_for_aux_passthrough_for_non_moa(self, monkeypatch): + from agent.auxiliary_client import _read_main_model_for_aux + + with patch("agent.auxiliary_client._read_main_provider", return_value="openrouter"), \ + patch("agent.auxiliary_client._read_main_model", return_value="anthropic/claude-opus-4.6"): + assert _read_main_model_for_aux() == "anthropic/claude-opus-4.6" + + def test_resolve_provider_client_direct_moa_unwraps(self, tmp_path, monkeypatch): + """Callers that hit resolve_provider_client("moa", ) directly + (vision auto-detect, plugin code) unwrap at the router chokepoint + instead of dead-ending in the unknown-provider branch.""" + self._write_moa_config(tmp_path, monkeypatch) + monkeypatch.setenv("OPENROUTER_API_KEY", "or-test-key") + + client, model = resolve_provider_client("moa", "opus-gpt") + + assert client is not None + assert model == "anthropic/claude-opus-4.8" + + def test_main_agent_fallback_uses_aggregator_for_moa_main(self, tmp_path, monkeypatch): + """_try_main_agent_model_fallback with a moa main resolves the + aggregator instead of asking for a literal "moa" client.""" + from agent.auxiliary_client import _try_main_agent_model_fallback + + self._write_moa_config(tmp_path, monkeypatch) + with patch("agent.auxiliary_client._read_main_provider", return_value="moa"), \ + patch("agent.auxiliary_client._read_main_model", return_value="opus-gpt"), \ + patch("agent.auxiliary_client._is_provider_unhealthy", return_value=False), \ + patch("agent.auxiliary_client.resolve_provider_client") as mock_resolve: + mock_client = MagicMock() + mock_resolve.return_value = (mock_client, "anthropic/claude-opus-4.8") + + client, model, label = _try_main_agent_model_fallback("anthropic", task="compression") + + assert client is mock_client + assert model == "anthropic/claude-opus-4.8" + assert label == "main-agent(openrouter)" + assert mock_resolve.call_args.kwargs["provider"] == "openrouter" + assert mock_resolve.call_args.kwargs["model"] == "anthropic/claude-opus-4.8" + + class TestBuildCallKwargsMaxTokens: """_build_call_kwargs should not cap output by default (#34530).