From f054b2b9ceab04dfc48b4084394e270d4eae7aca Mon Sep 17 00:00:00 2001 From: Tharanee Date: Sun, 20 Sep 2026 17:24:19 -0400 Subject: [PATCH] fix(moa): persist named preset deletion without losing config --- hermes_cli/web_routers/models.py | 20 +-- tests/hermes_cli/test_moa_preset_deletion.py | 128 ++++++++++++++++++ ...est_moa_set_models_preserves_extra_keys.py | 4 +- 3 files changed, 142 insertions(+), 10 deletions(-) create mode 100644 tests/hermes_cli/test_moa_preset_deletion.py diff --git a/hermes_cli/web_routers/models.py b/hermes_cli/web_routers/models.py index a23296334f..53f917f670 100644 --- a/hermes_cli/web_routers/models.py +++ b/hermes_cli/web_routers/models.py @@ -29,6 +29,7 @@ _config_profile_scope = late("_config_profile_scope", "hermes_cli.web_server_pro _profile_scope = late("_profile_scope", "hermes_cli.web_server_profiles") load_config = late("load_config", "hermes_cli.config") save_config = late("save_config", "hermes_cli.config") +read_raw_config = late("read_raw_config", "hermes_cli.config") _EMPTY_MODEL_INFO: dict = { @@ -309,7 +310,6 @@ def set_moa_models(body: MoaConfigPayload, profile: Optional[str] = None): # desktop's debounced PUT /api/config autosave races it, so the whole # span holds _CONFIG_MUTATION_LOCK or one of the two saves is dropped. with config_write_scope(body.profile or profile): - cfg = load_config() if body.presets: raw = { "default_preset": body.default_preset, @@ -328,14 +328,18 @@ def set_moa_models(body: MoaConfigPayload, profile: Optional[str] = None): if problems: raise HTTPException(status_code=422, detail="Invalid MoA config: " + "; ".join(problems)) normalized = normalize_moa_config(raw) - # Merge, don't overwrite: hand-edited keys not in MoaConfigPayload (save_traces, trace_dir) survive. - # See issue #58819. Write ONLY the moa section (merge_existing deep-merges it over the - # on-disk raw file): saving the whole default-expanded ``cfg`` snapshot re-persisted - # every other section too, so a Desktop MoA autosave could wipe a chain another - # surface wrote meanwhile (#89184, ``fallback_providers: []``). - moa_section = dict(cfg.get("moa") or {}) + + # Read the raw profile file because the effective config contains defaults and + # ``merge_existing=True`` deep-merges dicts, which resurrects omitted preset names. + # Keep unknown MoA keys and sibling sections by rebuilding only the MoA section on + # the raw document, while treating the submitted named map as authoritative. + raw_cfg = read_raw_config() + moa_section = dict(raw_cfg.get("moa") or {}) moa_section.update(normalized) - save_config({"moa": moa_section}, merge_existing=True) + if body.presets: + moa_section["presets"] = normalized["presets"] + raw_cfg["moa"] = moa_section + save_config(raw_cfg) return {"ok": True, **normalized} diff --git a/tests/hermes_cli/test_moa_preset_deletion.py b/tests/hermes_cli/test_moa_preset_deletion.py new file mode 100644 index 0000000000..b3fd320db2 --- /dev/null +++ b/tests/hermes_cli/test_moa_preset_deletion.py @@ -0,0 +1,128 @@ +"""A MoA preset dropped from the payload must leave config.yaml. + +``PUT /api/model/moa`` expresses a deletion by OMITTING the preset from +``presets``. The save used ``save_config(..., merge_existing=True)``, whose +``_deep_merge`` recurses dict-over-dict and therefore restored every omitted +preset from disk — the GUI's delete button returned ok while the preset stayed +in config.yaml forever. ``presets`` is authoritative on write; undeclared +sibling keys (``save_traces``, ``trace_dir``) must still survive (#58819), and +other config sections must not be clobbered (#89184). + +Real config pipeline against a temp HERMES_HOME — a mocked save cannot show the +merge that caused the bug. +""" + +from __future__ import annotations + +import yaml + +from hermes_cli.web_models import MoaConfigPayload, MoaModelSlot, MoaPresetPayload +from hermes_cli.web_routers.models import set_moa_models + + +def _preset(model: str) -> MoaPresetPayload: + return MoaPresetPayload( + reference_models=[MoaModelSlot(provider="openai-codex", model=model)], + aggregator=MoaModelSlot(provider="anthropic", model="claude-opus-5"), + enabled=True, + ) + + +def _on_disk(home) -> dict: + return yaml.safe_load((home / "config.yaml").read_text(encoding="utf-8")) or {} + + +def _seed(home, monkeypatch, moa: dict, **sections) -> None: + monkeypatch.setenv("HERMES_HOME", str(home)) + (home / "config.yaml").write_text(yaml.safe_dump({"moa": moa, **sections}), encoding="utf-8") + + +def _three_presets() -> dict: + return { + "default_preset": "keep_a", + "presets": { + "keep_a": { + "reference_models": [{"provider": "openai-codex", "model": "gpt-5.5"}], + "aggregator": {"provider": "anthropic", "model": "claude-opus-5"}, + "enabled": True, + }, + "doomed": { + "reference_models": [{"provider": "openai-codex", "model": "gpt-5.6-luna"}], + "aggregator": {"provider": "anthropic", "model": "claude-opus-5"}, + "enabled": True, + }, + "keep_b": { + "reference_models": [{"provider": "openai-codex", "model": "gpt-5.7"}], + "aggregator": {"provider": "anthropic", "model": "claude-opus-5"}, + "enabled": True, + }, + }, + } + + +def test_omitted_preset_is_deleted_from_disk(tmp_path, monkeypatch): + """The whole point: a preset the payload omits must not survive the save.""" + home = tmp_path / ".hermes" + home.mkdir() + _seed(home, monkeypatch, _three_presets()) + + # The GUI re-sends the surviving presets; "doomed" is simply absent. + set_moa_models( + MoaConfigPayload( + default_preset="keep_a", + active_preset="", + presets={"keep_a": _preset("gpt-5.5"), "keep_b": _preset("gpt-5.7")}, + ) + ) + + presets = _on_disk(home)["moa"]["presets"] + assert "doomed" not in presets, "deleted preset was restored by the merge" + assert set(presets) == {"keep_a", "keep_b"} + + +def test_deletion_preserves_undeclared_moa_keys_and_other_sections(tmp_path, monkeypatch): + """Authoritative presets must not cost us #58819 (save_traces) or #89184 (siblings).""" + home = tmp_path / ".hermes" + home.mkdir() + moa = _three_presets() + moa.update(save_traces=True, trace_dir="/custom/traces") + chain = [{"provider": "custom", "model": "glm-5.08", "base_url": "http://gw:8080/v1"}] + _seed(home, monkeypatch, moa, fallback_providers=chain) + + set_moa_models( + MoaConfigPayload( + default_preset="keep_a", + active_preset="", + presets={"keep_a": _preset("gpt-5.5"), "keep_b": _preset("gpt-5.7")}, + ) + ) + + on_disk = _on_disk(home) + assert "doomed" not in on_disk["moa"]["presets"] + assert on_disk["moa"]["save_traces"] is True, "save_traces dropped (#58819)" + assert on_disk["moa"]["trace_dir"] == "/custom/traces", "trace_dir dropped (#58819)" + assert on_disk["fallback_providers"] == chain, "MoA save clobbered fallback_providers (#89184)" + + +def test_adding_a_preset_still_works(tmp_path, monkeypatch): + """Authoritative writes must not break the add/edit path.""" + home = tmp_path / ".hermes" + home.mkdir() + _seed(home, monkeypatch, _three_presets()) + + set_moa_models( + MoaConfigPayload( + default_preset="keep_a", + active_preset="", + presets={ + "keep_a": _preset("gpt-5.5"), + "doomed": _preset("gpt-5.6-luna"), + "keep_b": _preset("gpt-5.7"), + "fresh": _preset("gpt-5.9"), + }, + ) + ) + + presets = _on_disk(home)["moa"]["presets"] + assert set(presets) == {"keep_a", "doomed", "keep_b", "fresh"} + assert presets["fresh"]["reference_models"][0]["model"] == "gpt-5.9" diff --git a/tests/hermes_cli/test_moa_set_models_preserves_extra_keys.py b/tests/hermes_cli/test_moa_set_models_preserves_extra_keys.py index 70af2f7708..5db29bcfdc 100644 --- a/tests/hermes_cli/test_moa_set_models_preserves_extra_keys.py +++ b/tests/hermes_cli/test_moa_set_models_preserves_extra_keys.py @@ -58,7 +58,7 @@ class TestSetMoaModelsPreservesUndeclaredKeys: saved_cfg = {} - def fake_load_config(): + def fake_read_raw_config(): return dict(existing_cfg) # shallow copy def fake_save_config(cfg, **_kwargs): @@ -67,7 +67,7 @@ class TestSetMoaModelsPreservesUndeclaredKeys: payload = _base_payload() with ( - patch("hermes_cli.config.load_config", side_effect=fake_load_config), + patch("hermes_cli.config.read_raw_config", side_effect=fake_read_raw_config), patch("hermes_cli.config.save_config", side_effect=fake_save_config), patch("hermes_cli.web_server_profiles._profile_scope"), ):