From ba7da1332cfcc067f778f424a1fac03b91b03081 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 29 Jul 2026 09:42:24 -0700 Subject: [PATCH] refactor: single-owner model switch parsing + effective-model resolution (kills the api_server/run.py divergence class) --- cli.py | 25 +- gateway/platforms/api_server.py | 22 +- gateway/run.py | 22 +- gateway/slash_commands.py | 24 +- hermes_cli/model_switch.py | 161 ++++++++++++ tests/hermes_cli/test_model_switch_parsing.py | 232 ++++++++++++++++++ tui_gateway/server.py | 15 +- 7 files changed, 461 insertions(+), 40 deletions(-) create mode 100644 tests/hermes_cli/test_model_switch_parsing.py diff --git a/cli.py b/cli.py index 33ccf2fa64e..2ed4fbb150a 100644 --- a/cli.py +++ b/cli.py @@ -9057,7 +9057,7 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin): """ from hermes_cli.model_switch import ( switch_model, - parse_model_flags_detailed, + parse_model_switch_args, resolve_persist_behavior, ) from hermes_cli.providers import get_label @@ -9067,18 +9067,17 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin): raw_args = parts[1].strip() if len(parts) > 1 else "" # Parse --provider, --global, --session, --once, and --refresh flags - parsed_flags = parse_model_flags_detailed(raw_args) - model_input = parsed_flags.model_input - explicit_provider = parsed_flags.explicit_provider - is_global_flag = parsed_flags.is_global - force_refresh = parsed_flags.force_refresh - is_session = parsed_flags.is_session - one_turn = parsed_flags.is_once - if is_global_flag and one_turn: - _cprint(" ✗ /model --once cannot be combined with --global") - return - if one_turn and not model_input and not explicit_provider: - _cprint(" ✗ /model --once requires a model or provider.") + # via the shared single-owner parser (hermes_cli.model_switch). + request = parse_model_switch_args(raw_args) + model_input = request.target + explicit_provider = request.explicit_provider + is_global_flag = request.is_global + force_refresh = request.force_refresh + is_session = request.is_session + one_turn = request.is_once + if request.errors: + # CLI decoration: " ✗ " prefix over the canonical error copy. + _cprint(f" ✗ {request.error_messages()[0]}") return # Resolve the effective persistence once: --global forces persist, # --session/--once force session-scope, otherwise defer to diff --git a/gateway/platforms/api_server.py b/gateway/platforms/api_server.py index e21c2ba4282..62dff2878fa 100644 --- a/gateway/platforms/api_server.py +++ b/gateway/platforms/api_server.py @@ -1410,17 +1410,22 @@ class APIServerAdapter(BasePlatformAdapter): 1. Explicit override (config extra or API_SERVER_MODEL_NAME env var) 2. Active profile name (so each profile advertises a distinct model) 3. Fallback: "hermes-agent" + + Delegates the tiered fallthrough to + :func:`hermes_cli.model_switch.resolve_effective_model` (the shared + override > mid-tier > default precedence owner). """ - if explicit and explicit.strip(): - return explicit.strip() + from hermes_cli.model_switch import resolve_effective_model + + profile_name = "" try: from hermes_cli.profiles import get_active_profile_name profile = get_active_profile_name() if profile and profile not in {"default", "custom"}: - return profile + profile_name = profile except Exception: pass - return "hermes-agent" + return resolve_effective_model(explicit, profile_name, "hermes-agent") def _cors_headers_for_origin(self, origin: str) -> Optional[Dict[str, str]]: """Return CORS headers for an allowed browser origin.""" @@ -2487,8 +2492,13 @@ class APIServerAdapter(BasePlatformAdapter): session_override = None if not confirmed_runtime_lock: session_override = self._session_model_override_for(session_key) + # Model-string precedence delegates to the shared owner + # hermes_cli.model_switch.resolve_effective_model (session /model + # override > session-persisted model > global) — the rule 7dd00bb47d + # had to re-fix here after it diverged from gateway/run.py. + from hermes_cli.model_switch import resolve_effective_model if session_override: - override_model = _clean_request_string(session_override.get("model")) or model + override_model = resolve_effective_model(session_override, None, model) session_provider = _clean_request_string(session_override.get("provider")) current_provider = _clean_request_string(runtime_kwargs.get("provider")) provider_runtime = _resolve_provider_runtime( @@ -2518,7 +2528,7 @@ class APIServerAdapter(BasePlatformAdapter): ) if provider_runtime: _apply_runtime_agent_overrides(runtime_kwargs, provider_runtime) - model = session_row_model + model = resolve_effective_model(None, session_row_model, model) if request_model or request_provider: logger.debug( "api_server request selection skipped: session-persisted model wins for %s", diff --git a/gateway/run.py b/gateway/run.py index 64d884c63ae..cbefcd1ba8f 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -5556,7 +5556,19 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew thread_id: Optional[str] = None, parent_id: Optional[str] = None, ) -> str: - """Resolve model for this channel: channel_overrides else global default.""" + """Resolve model for this channel: channel_overrides else global default. + + Delegates the precedence rule to + :func:`hermes_cli.model_switch.resolve_effective_model` (session + override > channel override > global default) — the single owner + shared with the API server, so the two surfaces cannot diverge + again (see 7dd00bb47d). This call site has no session tier: session + /model overrides are applied later by + ``_apply_session_model_override`` on the resolved runtime. + """ + from hermes_cli.model_switch import resolve_effective_model + + override = None config = getattr(self, "config", None) if config: override = _get_channel_override( @@ -5566,9 +5578,11 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew thread_id=thread_id, parent_id=parent_id, ) - if override and override.model: - return override.model - return _resolve_gateway_model(user_config) + return resolve_effective_model( + None, # session tier applied downstream (_apply_session_model_override) + override, + _resolve_gateway_model(user_config), + ) def _get_system_prompt_for_channel( self, diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index 4a2162a4058..4a06c164088 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -1724,7 +1724,7 @@ class GatewaySlashCommandsMixin: """ from gateway.run import _hermes_home, _load_gateway_config from hermes_cli.model_switch import ( - switch_model as _switch_model, parse_model_flags_detailed, + switch_model as _switch_model, parse_model_switch_args, resolve_persist_behavior, list_authenticated_providers, list_picker_providers, @@ -1740,17 +1740,17 @@ class GatewaySlashCommandsMixin: )(source) # Parse --provider, --global, --session, --once, and --refresh flags - parsed_flags = parse_model_flags_detailed(raw_args) - model_input = parsed_flags.model_input - explicit_provider = parsed_flags.explicit_provider - is_global_flag = parsed_flags.is_global - force_refresh = parsed_flags.force_refresh - is_session = parsed_flags.is_session - one_turn = parsed_flags.is_once - if is_global_flag and one_turn: - return "❌ /model --once cannot be combined with --global" - if one_turn and not model_input and not explicit_provider: - return "❌ /model --once requires a model or provider." + # via the shared single-owner parser (hermes_cli.model_switch). + request = parse_model_switch_args(raw_args) + model_input = request.target + explicit_provider = request.explicit_provider + is_global_flag = request.is_global + force_refresh = request.force_refresh + is_session = request.is_session + one_turn = request.is_once + if request.errors: + # Gateway decoration: "❌ " prefix over the canonical error copy. + return f"❌ {request.error_messages()[0]}" persist_global = resolve_persist_behavior( is_global_flag, is_session, diff --git a/hermes_cli/model_switch.py b/hermes_cli/model_switch.py index e9641fa7b6d..75a1e9d914d 100644 --- a/hermes_cli/model_switch.py +++ b/hermes_cli/model_switch.py @@ -598,6 +598,167 @@ def resolve_persist_behavior( return False +# --------------------------------------------------------------------------- +# Single-owner /model request parsing + effective-model resolution +# --------------------------------------------------------------------------- +# +# Historically each surface (cli.py, gateway/slash_commands.py, +# tui_gateway/server.py) re-implemented flag parsing + conflict checks, and +# each resolution surface (gateway/run.py, gateway/platforms/api_server.py) +# re-implemented the session-override > channel/session > global precedence. +# Commit 7dd00bb47d had to re-fix the api_server discarding session-persisted +# models precisely because the precedence rule lived in two places. The +# helpers below are the ONE owner; surfaces map error codes to their own +# user-facing copy but never re-derive the semantics. + +# Error codes emitted by parse_model_switch_args(). +MODEL_SWITCH_ERR_ONCE_WITH_GLOBAL = "once_with_global" +MODEL_SWITCH_ERR_ONCE_REQUIRES_TARGET = "once_requires_target" + +# Canonical (surface-neutral) error copy. Surfaces prepend their own +# decoration (" ✗ " in the CLI, "❌ " in the gateway) but MUST NOT change +# the core sentence — it is shared user-visible copy. +MODEL_SWITCH_ERROR_TEXT = { + MODEL_SWITCH_ERR_ONCE_WITH_GLOBAL: "/model --once cannot be combined with --global", + MODEL_SWITCH_ERR_ONCE_REQUIRES_TARGET: "/model --once requires a model or provider.", +} + + +@dataclass(frozen=True) +class ModelSwitchRequest: + """A fully parsed /model command request. + + ``scope`` is the *requested* persistence scope derived purely from the + flags: ``"once"`` | ``"session"`` | ``"global"`` | ``"default"`` (no + explicit scope flag; the effective decision then belongs to + :func:`resolve_persist_behavior`, which also reads config). + + ``errors`` carries error *codes* (see ``MODEL_SWITCH_ERR_*``); surfaces + render them via :data:`MODEL_SWITCH_ERROR_TEXT` plus their own prefix. + """ + + raw: str + target: str + explicit_provider: str = "" + is_global: bool = False + is_session: bool = False + is_once: bool = False + force_refresh: bool = False + scope: str = "default" + errors: tuple = () + + # Compat properties so a ModelSwitchRequest can be passed anywhere a + # ModelFlagParseResult was accepted (e.g. tui_gateway._apply_model_switch). + @property + def model_input(self) -> str: + return self.target + + @property + def flags(self) -> "ModelFlagParseResult": + return ModelFlagParseResult( + model_input=self.target, + explicit_provider=self.explicit_provider, + is_global=self.is_global, + force_refresh=self.force_refresh, + is_session=self.is_session, + is_once=self.is_once, + ) + + def error_messages(self) -> list: + """Canonical (undercorated) error strings for this request.""" + return [MODEL_SWITCH_ERROR_TEXT[code] for code in self.errors] + + +def parse_model_switch_args(raw: str) -> ModelSwitchRequest: + """Parse a raw /model argument string into a :class:`ModelSwitchRequest`. + + The ONE parser for every /model surface. Wraps + :func:`parse_model_flags_detailed` (tokenization + Unicode-dash + normalization) and layers on the flag-conflict validation that cli.py, + gateway/slash_commands.py, and tui_gateway/server.py each used to + re-implement: + + * ``--once`` + ``--global`` → ``MODEL_SWITCH_ERR_ONCE_WITH_GLOBAL`` + * ``--once`` with no model and no ``--provider`` + → ``MODEL_SWITCH_ERR_ONCE_REQUIRES_TARGET`` + + Model targets pass through untouched: bare names (``sonnet``), + aggregator slugs (``vendor/model``), and colon forms (``vendor:model``) + are all resolved later by :func:`switch_model` (aggregator-aware — bare + names resolve WITHIN the current aggregator first). + """ + raw = str(raw or "") + parsed = parse_model_flags_detailed(raw) + + errors: list = [] + if parsed.is_once and parsed.is_global: + errors.append(MODEL_SWITCH_ERR_ONCE_WITH_GLOBAL) + if parsed.is_once and not parsed.model_input and not parsed.explicit_provider: + errors.append(MODEL_SWITCH_ERR_ONCE_REQUIRES_TARGET) + + if parsed.is_once: + scope = "once" + elif parsed.is_session: + scope = "session" + elif parsed.is_global: + scope = "global" + else: + scope = "default" + + return ModelSwitchRequest( + raw=raw, + target=parsed.model_input, + explicit_provider=parsed.explicit_provider, + is_global=parsed.is_global, + is_session=parsed.is_session, + is_once=parsed.is_once, + force_refresh=parsed.force_refresh, + scope=scope, + errors=tuple(errors), + ) + + +def _effective_model_candidate(value: Any) -> str: + """Extract a model-name candidate from a str / dict / attr-object.""" + if value is None: + return "" + if isinstance(value, str): + return value.strip() + if isinstance(value, dict): + return str(value.get("model") or "").strip() + model_attr = getattr(value, "model", None) + if model_attr is not None: + return str(model_attr or "").strip() + return "" + + +def resolve_effective_model( + session_overrides: Any = None, + channel_config: Any = None, + global_config: Any = "", +) -> str: + """Resolve the effective model: session override > channel > global. + + The single owner of the precedence rule that gateway/run.py + (``_resolve_model_for_channel`` / ``_apply_session_model_override``) and + gateway/platforms/api_server.py (``_create_agent``'s session-override / + session-persisted-model branches) each encoded independently — the + divergence commit 7dd00bb47d had to close. A user-issued ``/model`` + (session override) always wins over per-channel/session-persisted + configuration, which wins over the global default. + + Each argument may be a plain model string, a dict with a ``"model"`` + key (a gateway ``_session_model_overrides`` entry), or an object with a + ``.model`` attribute (a ``ChannelOverride``). Empty/None entries fall + through to the next tier. + """ + for tier in (session_overrides, channel_config, global_config): + candidate = _effective_model_candidate(tier) + if candidate: + return candidate + return "" + + # --------------------------------------------------------------------------- # Alias resolution # --------------------------------------------------------------------------- diff --git a/tests/hermes_cli/test_model_switch_parsing.py b/tests/hermes_cli/test_model_switch_parsing.py new file mode 100644 index 00000000000..4421d91ba36 --- /dev/null +++ b/tests/hermes_cli/test_model_switch_parsing.py @@ -0,0 +1,232 @@ +"""Single-owner /model parsing + effective-model resolution tests. + +Covers the consolidation of the 7 historical parsing/resolution variants +into hermes_cli.model_switch (parse_model_switch_args + +resolve_effective_model), including the 7dd00bb47d regression class +(api_server discarding session-persisted models) as a permanent parity +test against the pre-consolidation logic captured from origin/main. + +Real imports throughout (AGENTS.md: no mocks for resolution chains). +""" + +import pytest + +from hermes_cli.model_switch import ( + MODEL_SWITCH_ERR_ONCE_REQUIRES_TARGET, + MODEL_SWITCH_ERR_ONCE_WITH_GLOBAL, + MODEL_SWITCH_ERROR_TEXT, + ModelSwitchRequest, + parse_model_flags_detailed, + parse_model_switch_args, + resolve_effective_model, +) + + +# --------------------------------------------------------------------------- +# parse_model_switch_args — the ONE parser +# --------------------------------------------------------------------------- + +def test_bare_name_on_aggregator_passes_through(): + # Bare names are NOT provider-qualified by the parser: aggregator-aware + # resolution (bare names resolve WITHIN the aggregator first, via + # switch_model's catalog search) happens downstream. The parser must not + # hardcode a provider. + req = parse_model_switch_args("sonnet") + assert req.target == "sonnet" + assert req.explicit_provider == "" + assert req.scope == "default" + assert req.errors == () + + +def test_provider_colon_model_target_preserved(): + # vendor:model colon forms are preserved verbatim — switch_model converts + # them to aggregator slugs only when the current provider is an aggregator. + req = parse_model_switch_args("anthropic:claude-sonnet-4-5") + assert req.target == "anthropic:claude-sonnet-4-5" + assert req.explicit_provider == "" + + +def test_provider_flag_and_scopes(): + req = parse_model_switch_args("sonnet --provider anthropic --global") + assert req.target == "sonnet" + assert req.explicit_provider == "anthropic" + assert req.is_global is True + assert req.scope == "global" + assert req.errors == () + + assert parse_model_switch_args("sonnet --session").scope == "session" + assert parse_model_switch_args("sonnet --once").scope == "once" + assert parse_model_switch_args("--refresh").force_refresh is True + + +def test_once_with_global_conflict(): + req = parse_model_switch_args("sonnet --once --global") + assert MODEL_SWITCH_ERR_ONCE_WITH_GLOBAL in req.errors + assert ( + MODEL_SWITCH_ERROR_TEXT[MODEL_SWITCH_ERR_ONCE_WITH_GLOBAL] + == "/model --once cannot be combined with --global" + ) + assert "/model --once cannot be combined with --global" in req.error_messages() + + +def test_once_without_target_error(): + req = parse_model_switch_args("--once") + assert MODEL_SWITCH_ERR_ONCE_REQUIRES_TARGET in req.errors + assert ( + MODEL_SWITCH_ERROR_TEXT[MODEL_SWITCH_ERR_ONCE_REQUIRES_TARGET] + == "/model --once requires a model or provider." + ) + # --once with just a provider is valid. + assert parse_model_switch_args("--once --provider anthropic").errors == () + + +def test_unicode_dash_normalization_matches_legacy_parser(): + # Telegram/iOS auto-converts "--" to em dashes; the shared parser must + # keep the legacy normalization. + req = parse_model_switch_args("sonnet \u2014global") + assert req.is_global is True + assert req.target == "sonnet" + + +def test_request_is_compatible_with_flag_result_consumers(): + # tui_gateway._apply_model_switch duck-types on .model_input; the request + # object must satisfy the same consumer surface. + req = parse_model_switch_args("sonnet --provider anthropic --once") + assert req.model_input == "sonnet" + legacy = parse_model_flags_detailed("sonnet --provider anthropic --once") + assert req.flags == legacy + + +# --------------------------------------------------------------------------- +# resolve_effective_model — session > channel/session-persisted > global +# --------------------------------------------------------------------------- + +class _ChannelOverride: + def __init__(self, model): + self.model = model + + +def test_session_override_beats_channel_and_global(): + assert ( + resolve_effective_model({"model": "session-model"}, _ChannelOverride("chan-model"), "global-model") + == "session-model" + ) + + +def test_channel_beats_global_when_no_session(): + assert ( + resolve_effective_model(None, _ChannelOverride("chan-model"), "global-model") + == "chan-model" + ) + assert resolve_effective_model({}, _ChannelOverride(""), "global-model") == "global-model" + + +def test_global_fallback_and_empty(): + assert resolve_effective_model(None, None, "global-model") == "global-model" + assert resolve_effective_model(None, None, "") == "" + + +# --------------------------------------------------------------------------- +# Parity: run.py-style channel resolution (old logic from origin/main) +# --------------------------------------------------------------------------- + +def _old_run_py_resolve(override, global_model): + # Captured from origin/main gateway/run.py:_resolve_model_for_channel: + # if override and override.model: + # return override.model + # return _resolve_gateway_model(user_config) + if override and override.model: + return override.model + return global_model + + +@pytest.mark.parametrize( + "override,global_model", + [ + (None, "global-model"), + (_ChannelOverride("chan-model"), "global-model"), + (_ChannelOverride(""), "global-model"), + (_ChannelOverride("chan-model"), ""), + (None, ""), + ], +) +def test_run_py_channel_resolution_parity(override, global_model): + assert resolve_effective_model(None, override, global_model) == _old_run_py_resolve( + override, global_model + ) + + +# --------------------------------------------------------------------------- +# Parity: api_server-style resolution (old logic from origin/main) +# --------------------------------------------------------------------------- + +def _clean(value): + # api_server._clean_request_string equivalent for the parity harness. + if value is None: + return None + text = str(value).strip() + return text or None + + +def _old_api_server_resolve(session_override, session_row_model, global_model): + # Captured from origin/main gateway/platforms/api_server.py:_create_agent + # (post-7dd00bb47d — session /model override > session-persisted model > + # global default): + model = global_model + if session_override: + model = (_clean(session_override.get("model")) or model) + elif _clean(session_row_model): + model = _clean(session_row_model) + return model + + +@pytest.mark.parametrize( + "session_override,session_row_model,global_model", + [ + (None, None, "global-model"), + (None, "session-persisted", "global-model"), # the 7dd00bb47d regression + ({"model": "override-model"}, "session-persisted", "global-model"), + ({"model": ""}, "session-persisted", "global-model"), + ({"model": "override-model"}, None, "global-model"), + (None, " ", "global-model"), + ], +) +def test_api_server_resolution_parity(session_override, session_row_model, global_model): + # New logic mirrors the migrated api_server code path exactly: + if session_override: + new = resolve_effective_model(session_override, None, global_model) + elif _clean(session_row_model): + new = resolve_effective_model(None, session_row_model, global_model) + else: + new = global_model + assert new == _old_api_server_resolve(session_override, session_row_model, global_model) + + +def test_session_persisted_model_honored_by_both_surfaces(): + """Permanent 7dd00bb47d regression test. + + A session-persisted model (POST /api/sessions {"model": ...} on the API + server; per-channel/session config on the native gateway) must be honored + over the global default by BOTH resolution styles — the divergence class + this consolidation kills. + """ + session_persisted = "vendor/session-pinned-model" + global_model = "vendor/global-default" + + # run.py-style: channel/session tier vs global. + assert ( + resolve_effective_model(None, session_persisted, global_model) + == session_persisted + ) + # api_server-style: same shared owner, same answer. + assert ( + resolve_effective_model(None, {"model": session_persisted}, global_model) + == session_persisted + ) + # And an explicit session /model override still beats both. + assert ( + resolve_effective_model( + {"model": "vendor/live-override"}, session_persisted, global_model + ) + == "vendor/live-override" + ) diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 89ac2cd3a85..7d39f0ac123 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -3991,14 +3991,16 @@ def _apply_model_switch( persist_override: bool | None = None, ) -> dict: from hermes_cli.model_switch import ( - parse_model_flags_detailed, + parse_model_switch_args, resolve_persist_behavior, switch_model, + MODEL_SWITCH_ERR_ONCE_WITH_GLOBAL, + MODEL_SWITCH_ERROR_TEXT, ) from hermes_cli.runtime_provider import resolve_runtime_provider if parsed_flags is None: - parsed_flags = parse_model_flags_detailed(raw_input) + parsed_flags = parse_model_switch_args(raw_input) if hasattr(parsed_flags, "model_input"): model_input = parsed_flags.model_input explicit_provider = parsed_flags.explicit_provider @@ -4008,8 +4010,11 @@ def _apply_model_switch( else: model_input, explicit_provider, is_global_flag, _force_refresh, is_session = parsed_flags one_turn = False + # Conflict validation delegates to the shared single-owner parser; the + # TUI surfaces it as a raised ValueError (its historical behavior) + # using the canonical error copy. if is_global_flag and one_turn: - raise ValueError("/model --once cannot be combined with --global") + raise ValueError(MODEL_SWITCH_ERROR_TEXT[MODEL_SWITCH_ERR_ONCE_WITH_GLOBAL]) persist_global = ( persist_override if persist_override is not None @@ -13656,9 +13661,9 @@ def _(rid, params: dict) -> dict: 4009, "session busy — /interrupt the current turn before switching models", ) - from hermes_cli.model_switch import parse_model_flags_detailed + from hermes_cli.model_switch import parse_model_switch_args - parsed_flags = parse_model_flags_detailed(value) + parsed_flags = parse_model_switch_args(value) explicit_provider = parsed_flags.explicit_provider if session.get("agent") is None and not explicit_provider.strip(): session_id = params.get("session_id", "")