fix(moa): persist named preset deletion without losing config
This commit is contained in:
@@ -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}
|
||||
|
||||
|
||||
|
||||
128
tests/hermes_cli/test_moa_preset_deletion.py
Normal file
128
tests/hermes_cli/test_moa_preset_deletion.py
Normal file
@@ -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"
|
||||
@@ -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"),
|
||||
):
|
||||
|
||||
Reference in New Issue
Block a user