From 679487b80782b5e085dc5de02aecd289b7e4228e Mon Sep 17 00:00:00 2001 From: Dan Schnurbusch Date: Wed, 15 Jul 2026 18:36:18 -0500 Subject: [PATCH] fix(auth): enforce complete fallback routes Skip provider-only setup fallbacks, keep fallback selection explicit for resumed sessions, preserve configured primary identity for cron drift checks, and make the auth lost-update regression deterministic. --- cron/scheduler.py | 23 ++++--- ...test_credential_pool_oauth_writethrough.py | 16 ++++- tests/cron/test_scheduler.py | 10 ++- tests/test_tui_gateway_server.py | 56 ++++++++++++++--- tui_gateway/server.py | 62 ++++++++++++------- 5 files changed, 123 insertions(+), 44 deletions(-) diff --git a/cron/scheduler.py b/cron/scheduler.py index 3c82d7fa1..d5c9fcfff 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -2935,6 +2935,7 @@ def run_job( # Load config.yaml for model, reasoning, prefill, toolsets, provider routing _cfg = {} + _model_cfg = {} try: import yaml _cfg_path = str(_get_hermes_home() / "config.yaml") @@ -3037,7 +3038,16 @@ def run_job( _guard_job_credential_exfil(job) primary_model_for_drift = model - primary_provider_for_drift = (job.get("provider") or "").strip().lower() or None + configured_provider_for_drift = ( + str(_model_cfg.get("provider") or "").strip().lower() + if isinstance(_model_cfg, dict) + else "" + ) + primary_provider_for_drift = ( + str(job.get("provider") or "").strip().lower() + or configured_provider_for_drift + or None + ) try: # Do not inject HERMES_INFERENCE_PROVIDER here. resolve_runtime_provider() # already prefers persisted config over stale shell/env overrides when @@ -3070,9 +3080,9 @@ def run_job( if not isinstance(entry, dict): continue fb_provider = str(entry.get("provider") or "").strip() - if not fb_provider: + fb_model = str(entry.get("model") or "").strip() + if not fb_provider or not fb_model: continue - fb_model = str(entry.get("model") or "").strip() or None try: fb_kwargs = { "requested": fb_provider, @@ -3083,13 +3093,12 @@ def run_job( if entry.get("api_key"): fb_kwargs["explicit_api_key"] = entry["api_key"] runtime = resolve_runtime_provider(**fb_kwargs) - if fb_model: - model = fb_model + model = fb_model logger.info( - "Job '%s': fallback resolved to %s%s", + "Job '%s': fallback resolved to %s model %s", job_id, runtime.get("provider"), - f" model {fb_model}" if fb_model else "", + fb_model, ) break except Exception as fb_exc: diff --git a/tests/agent/test_credential_pool_oauth_writethrough.py b/tests/agent/test_credential_pool_oauth_writethrough.py index ece25da22..4003ccad0 100644 --- a/tests/agent/test_credential_pool_oauth_writethrough.py +++ b/tests/agent/test_credential_pool_oauth_writethrough.py @@ -213,6 +213,7 @@ def test_global_write_through_preserves_concurrent_root_update( ) helper_loaded = threading.Event() + helper_has_target_lock = threading.Event() allow_helper_save = threading.Event() writer_started = threading.Event() writer_done = threading.Event() @@ -221,11 +222,17 @@ def test_global_write_through_preserves_concurrent_root_update( def paused_helper_load(path=None): store = real_auth_load(path) if threading.current_thread().name == "profile-write-through": + target_holder = A._auth_lock_holder_for(root_path) + if getattr(target_holder, "depth", 0) > 0: + helper_has_target_lock.set() helper_loaded.set() assert allow_helper_save.wait(timeout=5) return store monkeypatch.setattr(A, "_load_auth_store", paused_helper_load) + # The pre-fix implementation imported the loader directly; patch both + # bindings so reverting the safe helper still exercises the stale ordering. + monkeypatch.setattr(CP, "_load_auth_store", paused_helper_load) def profile_write_through(): CP._write_through_provider_state_to_global_root( @@ -255,9 +262,12 @@ def test_global_write_through_preserves_concurrent_root_update( writer = threading.Thread(target=concurrent_codex_login, name="concurrent-login") writer.start() assert writer_started.wait(timeout=5) - # Before the fix the writer completes while the stale helper is paused. - # After the fix it blocks on the root lock until the helper saves and exits. - writer_done.wait(timeout=0.2) + # A fixed helper already owns the target lock, so the writer will merge + # after release. A reverted unlocked helper must first let the competing + # login finish; only then do we release its stale save. This makes the + # losing pre-fix ordering deterministic rather than scheduler-dependent. + if not helper_has_target_lock.is_set(): + assert writer_done.wait(timeout=5) allow_helper_save.set() helper.join(timeout=5) writer.join(timeout=5) diff --git a/tests/cron/test_scheduler.py b/tests/cron/test_scheduler.py index b83a05839..cd720ad21 100644 --- a/tests/cron/test_scheduler.py +++ b/tests/cron/test_scheduler.py @@ -2011,6 +2011,7 @@ class TestRunJobConfigEnvVarExpansion: " default: gpt-5.6-sol\n" " provider: openai-codex\n" "fallback_providers:\n" + " - provider: anthropic\n" " - provider: openrouter\n" " model: z-ai/glm-5.2\n", encoding="utf-8", @@ -2023,12 +2024,14 @@ class TestRunJobConfigEnvVarExpansion: "model_snapshot": "gpt-5.6-sol", } fake_db = MagicMock() + requested = [] def resolve_runtime(**kwargs): + requested.append(kwargs.get("requested")) if kwargs.get("requested") in (None, "openai-codex"): - raise AuthError( - "No Codex credentials stored", provider="openai-codex" - ) + # Cron must retain the configured primary provider for drift + # comparison even when older/custom AuthError sites omit it. + raise AuthError("No Codex credentials stored") assert kwargs["requested"] == "openrouter" assert kwargs["target_model"] == "z-ai/glm-5.2" return {**self._RUNTIME, "provider": "openrouter"} @@ -2049,6 +2052,7 @@ class TestRunJobConfigEnvVarExpansion: assert success is True assert error is None + assert requested == [None, "openrouter"] kwargs = mock_agent_cls.call_args.kwargs assert kwargs["provider"] == "openrouter" assert kwargs["model"] == "z-ai/glm-5.2" diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index b7989b30c..6c0d57118 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -9528,11 +9528,12 @@ class TestResolveRuntimeWithFallback: "hermes_cli.runtime_provider.resolve_runtime_provider", lambda **kw: expected, ) - runtime, fallback_model = server._resolve_runtime_with_fallback( + resolution = server._resolve_runtime_with_fallback( {"requested": "openai"} ) - assert runtime == expected - assert fallback_model is None + assert resolution.runtime == expected + assert resolution.selected_model is None + assert resolution.used_fallback is False def test_auth_error_tries_fallback_chain(self, monkeypatch): """On AuthError from primary, walk fallback_providers chain.""" @@ -9554,11 +9555,47 @@ class TestResolveRuntimeWithFallback: "_load_fallback_model", lambda: [{"provider": "deepseek", "model": "deepseek-v4-pro"}], ) - runtime, fallback_model = server._resolve_runtime_with_fallback( + resolution = server._resolve_runtime_with_fallback( {"requested": "openai-codex"}, ) - assert runtime == fallback_runtime - assert fallback_model == "deepseek-v4-pro" + assert resolution.runtime == fallback_runtime + assert resolution.selected_model == "deepseek-v4-pro" + assert resolution.used_fallback is True + + def test_auth_error_skips_provider_only_fallback(self, monkeypatch): + """Auth fallback requires one complete provider/model pair.""" + from hermes_cli.auth import AuthError + + requested = [] + fallback_runtime = {"provider": "openrouter", "api_key": "fb-tok"} + + def fake_resolve(**kwargs): + requested.append(kwargs.get("requested")) + if kwargs.get("requested") == "openai-codex": + raise AuthError("No Codex credentials stored") + return fallback_runtime + + monkeypatch.setattr( + "hermes_cli.runtime_provider.resolve_runtime_provider", + fake_resolve, + ) + monkeypatch.setattr( + server, + "_load_fallback_model", + lambda: [ + {"provider": "anthropic"}, + {"provider": "openrouter", "model": "z-ai/glm-5.2"}, + ], + ) + + resolution = server._resolve_runtime_with_fallback( + {"requested": "openai-codex"} + ) + + assert requested == ["openai-codex", "openrouter"] + assert resolution.runtime == fallback_runtime + assert resolution.selected_model == "z-ai/glm-5.2" + assert resolution.used_fallback is True def test_auth_error_all_fallbacks_fail_raises(self, monkeypatch): """When all fallbacks also fail, re-raise the original AuthError.""" @@ -9606,11 +9643,12 @@ class TestResolveRuntimeWithFallback: {"provider": "anthropic", "model": "claude-sonnet-4-6"}, ], ) - runtime, fallback_model = server._resolve_runtime_with_fallback( + resolution = server._resolve_runtime_with_fallback( {"requested": "openai-codex"}, ) - assert runtime == fallback_runtime - assert fallback_model == "claude-sonnet-4-6" + assert resolution.runtime == fallback_runtime + assert resolution.selected_model == "claude-sonnet-4-6" + assert resolution.used_fallback is True def test_make_agent_uses_fallback_on_auth_error(self, monkeypatch): """Integration: _make_agent falls back to configured fallback diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 5dfa39377..f0a554044 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -15,7 +15,7 @@ import time import uuid from datetime import datetime from pathlib import Path -from typing import Any, Optional +from typing import Any, NamedTuple, Optional from hermes_constants import ( get_hermes_home, @@ -4471,34 +4471,46 @@ def _schedule_mcp_late_refresh(sid: str, agent) -> None: ).start() +class _RuntimeFallbackResolution(NamedTuple): + runtime: dict + selected_model: str | None + used_fallback: bool + + def _resolve_runtime_with_fallback( resolve_kwargs: dict | None = None, -) -> tuple[dict, str | None]: - """Resolve a runtime and the fallback model selected after auth failure. +) -> _RuntimeFallbackResolution: + """Resolve the primary runtime or one complete provider/model fallback. - A fallback entry is one provider/model pair. Returning the model alongside - the runtime prevents callers from accidentally pairing the fallback - provider with the unavailable primary model. + Setup-time auth fallback only accepts entries with both fields. Provider- + only entries are skipped so the unavailable primary model can never leak + into a different runtime. ``used_fallback`` remains explicit rather than + overloading a nullable model as control flow. """ from hermes_cli.auth import AuthError from hermes_cli.runtime_provider import resolve_runtime_provider kwargs = resolve_kwargs or {} try: - return resolve_runtime_provider(**kwargs), None + return _RuntimeFallbackResolution( + resolve_runtime_provider(**kwargs), + None, + False, + ) except AuthError as primary_exc: fb_chain = _load_fallback_model() or [] for entry in fb_chain: if not isinstance(entry, dict): continue - fb_provider = (entry.get("provider") or "").strip() - if not fb_provider: + fb_provider = str(entry.get("provider") or "").strip() + fb_model = str(entry.get("model") or "").strip() + if not fb_provider or not fb_model: continue - fb_model = (entry.get("model") or "").strip() or None try: - fb_kwargs: dict = {"requested": fb_provider} - if fb_model: - fb_kwargs["target_model"] = fb_model + fb_kwargs: dict = { + "requested": fb_provider, + "target_model": fb_model, + } if entry.get("base_url"): fb_kwargs["explicit_base_url"] = entry["base_url"] if entry.get("api_key"): @@ -4507,12 +4519,12 @@ def _resolve_runtime_with_fallback( import logging logging.getLogger(__name__).warning( - "Primary auth failed (%s), falling back to %s%s", + "Primary auth failed (%s), falling back to %s model %s", primary_exc, fb_provider, - f" model {fb_model}" if fb_model else "", + fb_model, ) - return runtime, fb_model + return _RuntimeFallbackResolution(runtime, fb_model, True) except Exception: continue raise @@ -4613,9 +4625,12 @@ def _make_agent( resolve_kwargs["explicit_base_url"] = override_base_url resolve_kwargs["requested"] = requested_provider resolve_kwargs["target_model"] = model or None - runtime, auth_fallback_model = _resolve_runtime_with_fallback(resolve_kwargs) - if auth_fallback_model: - model = auth_fallback_model + resolution = _resolve_runtime_with_fallback(resolve_kwargs) + runtime = resolution.runtime + if resolution.used_fallback: + if not resolution.selected_model: + raise RuntimeError("Auth fallback resolved without a model") + model = resolution.selected_model else: # The switch already resolved concrete credentials/endpoint; honor # persisted overrides only while using that original runtime. They @@ -4632,12 +4647,15 @@ def _make_agent( model = model_override if provider_override: requested_provider = provider_override - runtime, auth_fallback_model = _resolve_runtime_with_fallback({ + resolution = _resolve_runtime_with_fallback({ "requested": requested_provider, "target_model": model or None, }) - if auth_fallback_model: - model = auth_fallback_model + runtime = resolution.runtime + if resolution.used_fallback: + if not resolution.selected_model: + raise RuntimeError("Auth fallback resolved without a model") + model = resolution.selected_model _pr = _load_provider_routing() return AIAgent( model=model,