From 2c6bbaf3529fbd7dca4330d53b5c819f3d223ba5 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 25 May 2026 15:22:23 -0700 Subject: [PATCH] fix(gateway): coerce scalar `model:` to dict before /model --global persist (#32272) Reported via AskClaw. When config.yaml has `model: ` (flat string) instead of the nested `model: {default: ..., provider: ...}` form, every gateway `/model X --global` crashed silently with TypeError: 'str' object does not support item assignment The persist block did: model_cfg = cfg.setdefault("model", {}) model_cfg["default"] = result.new_model `setdefault` returns the existing scalar, and the next assignment blows up. The 'switch failed' warning was logged at WARNING level and the user never saw why their persist didn't stick. Coerce scalar/None `model:` into a dict before mutation, in both the gateway path (`gateway/run.py`) and the sister site in `hermes_cli/doctor.py --fix` (same setdefault-on-string flaw). The CLI `/model` path is unaffected because it goes through `_set_nested` which already replaces scalar leaves with dicts. Regression test `tests/gateway/test_model_command_flat_string_config.py` covers the flat-string, missing, and proper-dict cases. Without the fix, the flat-string case fails with the exact original TypeError. --- gateway/run.py | 16 +- hermes_cli/doctor.py | 13 +- .../test_model_command_flat_string_config.py | 158 ++++++++++++++++++ 3 files changed, 185 insertions(+), 2 deletions(-) create mode 100644 tests/gateway/test_model_command_flat_string_config.py diff --git a/gateway/run.py b/gateway/run.py index b1d44e4e98d..9568eab2ceb 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -10436,7 +10436,21 @@ class GatewayRunner: cfg = yaml.safe_load(f) or {} else: cfg = {} - model_cfg = cfg.setdefault("model", {}) + # Coerce scalar/None ``model:`` into a dict before mutation — + # otherwise ``cfg.setdefault("model", {})`` returns the existing + # scalar and the next assignment raises + # ``TypeError: 'str' object does not support item assignment``. + # Reproduces when ``config.yaml`` has ``model: `` (flat + # string) instead of the proper nested ``model: {default: ...}``. + raw_model = cfg.get("model") + if isinstance(raw_model, dict): + model_cfg = raw_model + elif isinstance(raw_model, str) and raw_model.strip(): + model_cfg = {"default": raw_model.strip()} + cfg["model"] = model_cfg + else: + model_cfg = {} + cfg["model"] = model_cfg model_cfg["default"] = result.new_model model_cfg["provider"] = result.target_provider if result.base_url: diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index 9bdbc77e7ef..dbc486e87b1 100644 --- a/hermes_cli/doctor.py +++ b/hermes_cli/doctor.py @@ -812,7 +812,18 @@ def run_doctor(args): "(should be under 'model:' section)" ) if should_fix: - model_section = raw_config.setdefault("model", {}) + # Coerce scalar/None ``model:`` into a dict before mutation — + # ``setdefault("model", {})`` would return an existing scalar + # and then ``model_section[k] = ...`` would raise TypeError. + raw_model = raw_config.get("model") + if isinstance(raw_model, dict): + model_section = raw_model + elif isinstance(raw_model, str) and raw_model.strip(): + model_section = {"default": raw_model.strip()} + raw_config["model"] = model_section + else: + model_section = {} + raw_config["model"] = model_section for k in stale_root_keys: if not model_section.get(k): model_section[k] = raw_config.pop(k) diff --git a/tests/gateway/test_model_command_flat_string_config.py b/tests/gateway/test_model_command_flat_string_config.py new file mode 100644 index 00000000000..38d6ea11dae --- /dev/null +++ b/tests/gateway/test_model_command_flat_string_config.py @@ -0,0 +1,158 @@ +"""Regression tests for gateway /model --global persistence when config.yaml +has a flat-string ``model:`` value instead of a nested dict. + +Before fix: ``cfg.setdefault("model", {})`` returned the existing string and +the next assignment raised ``TypeError: 'str' object does not support item +assignment``, so every ``/model X --global`` from Telegram/Discord crashed +silently and the user-visible result was "switch failed" with no persist. + +After fix: the persist block coerces a scalar ``model:`` into a nested dict +before mutation, so ``--global`` succeeds and the config is rewritten in +the proper ``model: {default: ..., provider: ...}`` form. +""" + +import yaml +import pytest + +from gateway.config import Platform +from gateway.platforms.base import MessageEvent, MessageType +from gateway.run import GatewayRunner +from gateway.session import SessionSource + + +def _make_runner(): + runner = object.__new__(GatewayRunner) + runner.adapters = {} + runner._voice_mode = {} + runner._session_model_overrides = {} + runner._running_agents = {} + return runner + + +def _make_event(text): + return MessageEvent( + text=text, + message_type=MessageType.TEXT, + source=SessionSource(platform=Platform.TELEGRAM, chat_id="12345", chat_type="dm"), + ) + + +def _fake_switch_result(): + """Build a successful ModelSwitchResult that bypasses real provider resolution.""" + from hermes_cli.model_switch import ModelSwitchResult + + return ModelSwitchResult( + success=True, + new_model="gpt-5.5", + target_provider="openrouter", + provider_changed=True, + api_key="sk-test", + base_url="https://openrouter.ai/api/v1", + api_mode="chat_completions", + provider_label="OpenRouter", + is_global=True, + ) + + +def _setup_isolated_home(tmp_path, monkeypatch, model_yaml_value): + """Write a config.yaml with the given ``model:`` value and stub the heavy bits.""" + import gateway.run as gateway_run + + hermes_home = tmp_path / ".hermes" + hermes_home.mkdir() + cfg_path = hermes_home / "config.yaml" + cfg_path.write_text( + yaml.safe_dump({"model": model_yaml_value, "providers": {}}), + encoding="utf-8", + ) + + monkeypatch.setattr(gateway_run, "_hermes_home", hermes_home) + monkeypatch.setattr("agent.models_dev.fetch_models_dev", lambda: {}) + monkeypatch.setattr( + "hermes_cli.model_switch.switch_model", + lambda **kw: _fake_switch_result(), + ) + # save_config writes to ``get_hermes_home() / config.yaml`` — point it here. + monkeypatch.setattr("hermes_constants.get_hermes_home", lambda: hermes_home) + monkeypatch.setattr("hermes_cli.config.get_hermes_home", lambda: hermes_home) + return cfg_path + + +@pytest.mark.asyncio +async def test_model_global_persists_when_config_has_flat_string_model(tmp_path, monkeypatch): + """Regression: ``model: deepseek-v4-flash`` (flat string) used to crash + the gateway ``/model X --global`` persist branch with TypeError. After + the fix, the flat string is coerced to ``{"default": ...}`` and the new + model+provider are persisted on top. + """ + cfg_path = _setup_isolated_home(tmp_path, monkeypatch, "deepseek-v4-flash") + + result = await _make_runner()._handle_model_command( + _make_event("/model gpt-5.5 --global") + ) + + # Sanity: the handler returned a success-looking message (not a crash log). + assert result is not None + assert "gpt-5.5" in result + + # The persist block must have rewritten config.yaml as a nested dict. + written = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert isinstance(written["model"], dict), ( + "model: should be coerced to a dict, got %r" % (written["model"],) + ) + assert written["model"]["default"] == "gpt-5.5" + assert written["model"]["provider"] == "openrouter" + assert written["model"]["base_url"] == "https://openrouter.ai/api/v1" + + +@pytest.mark.asyncio +async def test_model_global_persists_when_config_has_missing_model(tmp_path, monkeypatch): + """Companion case: ``model:`` key absent entirely. setdefault would have + worked here, but the coercion branch also has to handle this cleanly. + """ + import gateway.run as gateway_run + + hermes_home = tmp_path / ".hermes" + hermes_home.mkdir() + cfg_path = hermes_home / "config.yaml" + cfg_path.write_text(yaml.safe_dump({"providers": {}}), encoding="utf-8") + + monkeypatch.setattr(gateway_run, "_hermes_home", hermes_home) + monkeypatch.setattr("agent.models_dev.fetch_models_dev", lambda: {}) + monkeypatch.setattr( + "hermes_cli.model_switch.switch_model", + lambda **kw: _fake_switch_result(), + ) + monkeypatch.setattr("hermes_constants.get_hermes_home", lambda: hermes_home) + monkeypatch.setattr("hermes_cli.config.get_hermes_home", lambda: hermes_home) + + result = await _make_runner()._handle_model_command( + _make_event("/model gpt-5.5 --global") + ) + + assert result is not None + written = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert isinstance(written["model"], dict) + assert written["model"]["default"] == "gpt-5.5" + assert written["model"]["provider"] == "openrouter" + + +@pytest.mark.asyncio +async def test_model_global_persists_when_config_has_proper_dict_model(tmp_path, monkeypatch): + """Already-correct nested dict must still work — no regression on the + common case. + """ + cfg_path = _setup_isolated_home( + tmp_path, + monkeypatch, + {"default": "old-model", "provider": "openai-codex"}, + ) + + result = await _make_runner()._handle_model_command( + _make_event("/model gpt-5.5 --global") + ) + + assert result is not None + written = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert written["model"]["default"] == "gpt-5.5" + assert written["model"]["provider"] == "openrouter"