From 0706dffca19b108403aeef9a9480f4361bae8f44 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Tue, 22 Sep 2026 00:53:08 -0700 Subject: [PATCH] fix(config): route every config.yaml writer through one comment-preserving writer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `hermes_cli.config.atomic_config_write` is now THE config.yaml writer: it delegates to `utils.atomic_roundtrip_yaml_save` (ruamel round-trip), which merges the new state onto the on-disk document so user comments, key order, quoting and blank lines survive every write. Why: config.yaml is hand-edited and commented, and every writer that re-serialised the parsed dict through PyYAML (`save_config`, `config set/unset`, migrations, plugin bookkeeping, auth provider reset, credential scrub, channel strip, backup restore, profile seed, telegram topic persistence) destroyed those comments — and `save_config` re-appended the stock boilerplate on top (#92554, #63039, #50698, #109611, #107511, #66752). The round-trip writer existed (tui_gateway only) but nothing else used it, so each new writer regressed the class. - save_config / _write_user_config / atomic_config_write -> round-trip merge; the commented example blocks are appended only when the file is created. - round-trip merge only reassigns nodes whose value changed (element-wise for lists), so an untouched scalar/list keeps its inline comments; YAML 1.1-ambiguous strings (off/yes/no...) are force-quoted at every depth; duplicate keys are tolerated like PyYAML. - direct PyYAML writers in auth.py, credential_lifecycle.py, profile_channels.py, backup.py, profiles.py, telegram adapter and tui_gateway/server.py now call atomic_config_write. --- hermes_cli/AGENTS.md | 8 + hermes_cli/auth.py | 8 +- hermes_cli/backup.py | 4 +- hermes_cli/config.py | 25 +-- hermes_cli/credential_lifecycle.py | 7 +- hermes_cli/profile_channels.py | 5 +- hermes_cli/profiles.py | 5 +- plugins/platforms/telegram/adapter.py | 2 +- scripts/check_config_yaml_writers.py | 118 ++++++++++++++ tests/gateway/test_dm_topics.py | 2 +- tests/hermes_cli/test_config.py | 8 +- .../test_config_yaml_comment_preservation.py | 152 ++++++++++++++++++ .../test_model_provider_persistence.py | 7 +- tui_gateway/server.py | 6 +- utils.py | 88 +++++++--- 15 files changed, 384 insertions(+), 61 deletions(-) create mode 100755 scripts/check_config_yaml_writers.py create mode 100644 tests/hermes_cli/test_config_yaml_comment_preservation.py diff --git a/hermes_cli/AGENTS.md b/hermes_cli/AGENTS.md index db15647ecb..421af5da92 100644 --- a/hermes_cli/AGENTS.md +++ b/hermes_cli/AGENTS.md @@ -87,6 +87,14 @@ Do not add a surface-specific goal parser. ACP has no goal command or goal loop set/get/unset ` route any bare name registered in `OPTIONAL_ENV_VARS` / `_EXTRA_ENV_KEYS` (or carrying a `setup_hidden_env` platform suffix) to `.env` via `config_env_routing.py` — the file the platform setup flows write — never to the top level of config.yaml. +- **One writer.** Every write of a `config.yaml` (main or profile) goes through + `hermes_cli.config.atomic_config_write` (→ `utils.atomic_roundtrip_yaml_save`, ruamel + round-trip merge): comments, key order, quoting and blank lines survive, absent keys are + deleted, and the fail-closed unreadable-file guard runs first. `save_config`, `config set/unset`, + migrations, plugin bookkeeping, gateway/TUI RPCs and auth resets all reach it; never call + `atomic_yaml_write` / `yaml.dump` / `yaml.safe_dump` on a config path — `scripts/check_config_yaml_writers.py` + (CI lint) rejects it, and `tests/hermes_cli/test_config_yaml_comment_preservation.py` guards each + path (#92554). The commented example blocks are appended only when the file is created. - **Three loaders — know which you're in:** `load_cli_config()` (CLI, `cli.py`); `load_config()` (`hermes tools/setup`, most subcommands, `hermes_cli/config.py`, merges `DEFAULT_CONFIG`); `hermes_cli/config_effective.py::load_user_config_effective()` (gateway runtime via diff --git a/hermes_cli/auth.py b/hermes_cli/auth.py index f5ecdb95e1..3473e43649 100644 --- a/hermes_cli/auth.py +++ b/hermes_cli/auth.py @@ -30,7 +30,7 @@ from urllib.parse import urlparse from hermes_constants import OPENROUTER_BASE_URL, hermes_home_key, secure_parent_dir from agent.credential_persistence import sanitize_borrowed_credential_payload -from utils import atomic_json_write, atomic_yaml_write, env_float, file_signature, is_truthy_value # noqa: F401 (env_float: agent.credential_pool reads auth_mod.env_float) +from utils import atomic_json_write, env_float, file_signature, is_truthy_value # noqa: F401 (env_float: agent.credential_pool reads auth_mod.env_float) from hermes_cli.auth_zai_kimi import ( # noqa: F401 re-exported KIMI_CODE_BASE_URL, ZAI_ENDPOINTS, _normalize_lmstudio_runtime_base_url, _resolve_kimi_base_url, _resolve_zai_base_url, detect_zai_endpoint) @@ -256,7 +256,7 @@ BUILTIN_PROVIDER_IDS = frozenset(PROVIDER_REGISTRY) # a plugin never observes a partially initialized auth module (CONTRACT: during discovery a plugin may # rely only on ``ProviderConfig`` and ``PROVIDER_REGISTRY`` from here — nothing defined below). from hermes_cli.config import ( # noqa: E402 - get_hermes_home, get_config_path, read_raw_config, require_readable_config_before_write) + atomic_config_write, get_hermes_home, get_config_path, read_raw_config, require_readable_config_before_write) # Plugin profiles (plugins/model-providers//) are mirrored into PROVIDER_REGISTRY with the # auth_type they declare; the mirror lives in the sibling so it can be re-run after discovery. @@ -2237,7 +2237,7 @@ def _update_config_for_provider( elif clear_default: model_cfg.pop("default", None) config["model"] = model_cfg - atomic_yaml_write(config_path, config, sort_keys=False) + atomic_config_write(config_path, config) return config_path @@ -2281,7 +2281,7 @@ def _reset_config_provider() -> Path: model["provider"] = "auto" if "base_url" in model: model["base_url"] = OPENROUTER_BASE_URL - atomic_yaml_write(config_path, config, sort_keys=False) + atomic_config_write(config_path, config) return config_path diff --git a/hermes_cli/backup.py b/hermes_cli/backup.py index d344eb3c2c..16e7923db6 100644 --- a/hermes_cli/backup.py +++ b/hermes_cli/backup.py @@ -1535,8 +1535,8 @@ def restore_config_model_settings_if_rewritten( if not restored_keys: return None try: - from utils import atomic_yaml_write - atomic_yaml_write(live_path, live) + from hermes_cli.config import atomic_config_write + atomic_config_write(live_path, live) except (OSError, PermissionError) as exc: logger.error("config.yaml model settings were rewritten during update but auto-restore failed: %s", exc) return None diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 02858f0ca2..75954e84f1 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -46,7 +46,7 @@ from hermes_constants import ( # noqa: F401 apply_secure_dir_policy, get_managed_system) # Re-export from hermes_constants — canonical definition lives there. from hermes_constants import get_hermes_home, get_process_hermes_home # noqa: F401 -from utils import atomic_replace, atomic_yaml_write, fast_safe_load, file_signature +from utils import atomic_replace, fast_safe_load, file_signature logger = logging.getLogger(__name__) @@ -234,14 +234,14 @@ _CONFIG_LOCK = threading.RLock() _LAST_EXPANDED_CONFIG_BY_PATH: Dict[str, Any] = {} # path -> (user_mtime_ns, user_size, managed_mtime_ns, managed_size, merged, env_ref_snapshot). # load_config() returns a deepcopy of the cached value while the signature matches (skips -# safe_load + merge + normalize + expand, ~13 ms). Writers use atomic_yaml_write (fresh inode +# safe_load + merge + normalize + expand, ~13 ms). Writers use atomic_config_write (fresh inode # -> new mtime_ns) so no explicit invalidation is needed. The managed-file signature is folded # in so editing the managed-scope config.yaml invalidates, and the env snapshot invalidates # when a referenced ${VAR} changes value (late .env load, in-process rotation). # (path, mtime_ns, size) -> cached expanded config dict. load_config() returns a deepcopy of the cached # value when the file hasn't changed since the last load, skipping yaml.safe_load + _deep_merge + # _normalize_* + _expand_env_vars (~13 ms/call). save_config() + migrate_config() write via -# atomic_yaml_write which produces a fresh inode, so stat() sees a new signature and the next load +# atomic_config_write which produces a fresh inode, so stat() sees a new signature and the next load # repopulates automatically — no explicit invalidation hook. See #58514. _LOAD_CONFIG_CACHE: Dict[str, Tuple[int, ...]] = {} # path -> (mtime_ns, size, ino, ctime_ns, raw yaml dict) for read_raw_config() (no defaults merged in). @@ -935,7 +935,7 @@ def _format_config_get_value(value, *, as_json: bool) -> str: if value is None: return "null" if isinstance(value, (dict, list)): - return yaml.safe_dump(value, sort_keys=False).rstrip() + return yaml.safe_dump(value, sort_keys=False).rstrip() # config-writer: ok — renders a value for display, never written to disk return str(value) @@ -2073,10 +2073,15 @@ def require_readable_config_before_write(config_path: Optional[Path] = None) -> return loaded -def atomic_config_write(config_path: Path, data: Any, **kwargs: Any) -> None: - """Fail-closed atomic write for ``config.yaml`` (``require_readable_config_before_write`` first).""" - require_readable_config_before_write(config_path) - atomic_yaml_write(config_path, data, **kwargs) +def atomic_config_write(config_path: Path, data: Dict[str, Any], *, extra_content_on_create: Optional[str] = None) -> None: + """THE ``config.yaml`` writer: fail-closed (``require_readable_config_before_write``) and + comment-preserving (ruamel round-trip merge of *data* onto the on-disk document). Every code + path that persists a config.yaml — ``save_config``, ``config set``, migrations, plugin + bookkeeping, gateway/TUI RPCs, auth resets — goes through here; a PyYAML dump of a config + path anywhere else is rejected by ``scripts/check_config_yaml_writers.py`` (#92554).""" + from utils import atomic_roundtrip_yaml_save + + atomic_roundtrip_yaml_save(config_path, data, extra_content_on_create=extra_content_on_create) def load_config() -> Dict[str, Any]: @@ -2451,7 +2456,7 @@ def save_config( effective_preserve_keys = _explicit_config_paths(_raw_for_paths) | set(preserve_keys or ()) normalized = _strip_default_values(normalized, DEFAULT_CONFIG, preserve_keys=effective_preserve_keys) - atomic_yaml_write(config_path, normalized, extra_content=_commented_sections_for_save(normalized)) + atomic_config_write(config_path, normalized, extra_content_on_create=_commented_sections_for_save(normalized)) _secure_file(config_path) _RAW_CONFIG_CACHE.pop(str(config_path), None) _LAST_EXPANDED_CONFIG_BY_PATH[str(config_path)] = copy.deepcopy(current_normalized) @@ -3460,7 +3465,7 @@ def _exit_invalid(msg: str) -> None: def _write_user_config(config_path: Path, user_config: Dict[str, Any]) -> None: """Write only the user's raw config back (never the merged defaults).""" ensure_hermes_home() - atomic_yaml_write(config_path, user_config, sort_keys=False) + atomic_config_write(config_path, user_config) def _print_unknown_key_notice(key: str, suggestion: Optional[str]) -> None: diff --git a/hermes_cli/credential_lifecycle.py b/hermes_cli/credential_lifecycle.py index 79079c97ed..17233914b7 100644 --- a/hermes_cli/credential_lifecycle.py +++ b/hermes_cli/credential_lifecycle.py @@ -87,9 +87,7 @@ def _scrub_config_yaml_mirrors(old_value: str, new_value: str | None) -> List[st """ if not old_value: return [] - from utils import atomic_yaml_write - - from hermes_cli.config import get_config_path, read_user_config_raw, require_readable_config_before_write + from hermes_cli.config import atomic_config_write, get_config_path, read_user_config_raw config_path = get_config_path() if not config_path.exists(): @@ -137,8 +135,7 @@ def _scrub_config_yaml_mirrors(old_value: str, new_value: str | None) -> List[st _fix(entry, f"providers.{provider_id}", fields=("api_key",)) if touched: - require_readable_config_before_write(config_path) - atomic_yaml_write(config_path, user_config, sort_keys=False) + atomic_config_write(config_path, user_config) return touched diff --git a/hermes_cli/profile_channels.py b/hermes_cli/profile_channels.py index 70c8f12c47..e4fa15886e 100644 --- a/hermes_cli/profile_channels.py +++ b/hermes_cli/profile_channels.py @@ -330,8 +330,7 @@ def strip_channel_config(config_path: Path, index: Optional[ChannelKeyIndex] = N """Remove platform sections from a raw ``config.yaml`` in place. Returns the dotted paths removed.""" if not config_path.is_file(): return [] - from hermes_cli.config import read_user_config_raw - from utils import atomic_yaml_write + from hermes_cli.config import atomic_config_write, read_user_config_raw index = index or ChannelKeyIndex() raw = read_user_config_raw(config_path) paths = _channel_config_paths(raw, index.platforms) @@ -344,7 +343,7 @@ def strip_channel_config(config_path: Path, index: Optional[ChannelKeyIndex] = N node.pop(path[-1], None) if isinstance(raw.get("gateway"), dict) and not raw["gateway"]: raw.pop("gateway") - atomic_yaml_write(config_path, raw, sort_keys=False) + atomic_config_write(config_path, raw) return [".".join(path) for path in paths] diff --git a/hermes_cli/profiles.py b/hermes_cli/profiles.py index 5b7e1f5bbe..af3f55e4b4 100644 --- a/hermes_cli/profiles.py +++ b/hermes_cli/profiles.py @@ -659,13 +659,12 @@ def _seed_model_config(profile_dir: Path) -> None: if config_path.exists(): return with contextlib.suppress(Exception): # creation must not fail over this; `hermes model` sets it later - import yaml from hermes_constants import get_hermes_home - from hermes_cli.config import read_user_config_raw + from hermes_cli.config import atomic_config_write, read_user_config_raw source = get_hermes_home() / "config.yaml" seed = launch_model_seed(read_user_config_raw(source)) if source.is_file() else {} if seed: - config_path.write_text(yaml.safe_dump(seed, sort_keys=False), encoding="utf-8") + atomic_config_write(config_path, seed) def _check_gateway_running(profile_dir: Path) -> bool: diff --git a/plugins/platforms/telegram/adapter.py b/plugins/platforms/telegram/adapter.py index 9c2ba012db..f7db03e035 100644 --- a/plugins/platforms/telegram/adapter.py +++ b/plugins/platforms/telegram/adapter.py @@ -2662,7 +2662,7 @@ class TelegramAdapter(BasePlatformAdapter): dm_topics.append({"chat_id": chat_id, "topics": [{"name": topic_name, "thread_id": thread_id}]}) changed = True if changed: - atomic_config_write(config_path, config, default_flow_style=False, sort_keys=False) + atomic_config_write(config_path, config) logger.info("[%s] Persisted thread_id=%s for topic '%s' in config.yaml", self.name, thread_id, topic_name) except Exception as e: logger.warning("[%s] Failed to persist thread_id to config: %s", self.name, e, exc_info=True) diff --git a/scripts/check_config_yaml_writers.py b/scripts/check_config_yaml_writers.py new file mode 100755 index 0000000000..32cce5eeab --- /dev/null +++ b/scripts/check_config_yaml_writers.py @@ -0,0 +1,118 @@ +#!/usr/bin/env python3 +"""Fail when a ``config.yaml`` is written by anything but the comment-preserving writer. + +Every writer of ``~/.hermes/config.yaml`` (and ``profiles/*/config.yaml``) must go through +``hermes_cli.config.atomic_config_write`` → ``utils.atomic_roundtrip_yaml_save`` (ruamel +round-trip). A PyYAML dump (``yaml.dump`` / ``yaml.safe_dump`` / ``utils.atomic_yaml_write``) +of a config path re-serialises the parsed dict and destroys every user comment — the #92554 +class, which regressed several times because each new writer picked the plain dumper again. + +Flags, in the scanned trees: + +* a call to ``atomic_yaml_write`` / ``yaml.dump`` / ``yaml.safe_dump`` / ``safe_dump`` whose + first argument names a config path (``config_path``, ``cfg_path``, ``config.yaml``, + ``get_config_path()``, ``_active_config_path()``, ``live_path``); +* ``.write_text(... dump(...) ...)``; +* any ``yaml.dump`` / ``yaml.safe_dump`` / ``atomic_yaml_write`` call inside ``hermes_cli/config.py`` + or ``hermes_cli/config_*.py`` (the config system has exactly one writer). + +Suppress a true false positive with ``# config-writer: ok — `` on the call's line. + +Usage: python3 scripts/check_config_yaml_writers.py [paths...] +""" +from __future__ import annotations + +import ast +import re +import sys +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent +DEFAULT_TREES = ("hermes_cli", "agent", "gateway", "tui_gateway", "cron", "plugins", "tools", "cli.py", "utils.py") +# The writer module itself and the on-disk primitive it wraps. +ALLOWED_FILES = {ROOT / "utils.py"} +DUMPERS = {"atomic_yaml_write", "safe_dump", "dump"} +CONFIG_PATH_RE = re.compile( + r"config_path|cfg_path|config\.yaml|get_config_path\(\)|_active_config_path\(\)|\blive_path\b") +CONFIG_MODULE_RE = re.compile(r"^hermes_cli/config(_[a-z_]+)?\.py$") +SUPPRESS = "# config-writer: ok" + + +def _func_name(node: ast.AST) -> str | None: + if isinstance(node, ast.Attribute): + return node.attr + if isinstance(node, ast.Name): + return node.id + return None + + +def _is_yaml_dump(call: ast.Call, src: str) -> bool: + name = _func_name(call.func) + if name == "atomic_yaml_write": + return True + if name in ("safe_dump", "dump"): + # ``yaml.dump`` / ``yaml.safe_dump`` / ``yaml_rt.dump``; plain ``json.dump`` is not YAML. + receiver = ast.get_source_segment(src, call.func.value) if isinstance(call.func, ast.Attribute) else "" + return "yaml" in (receiver or "").lower() or name == "safe_dump" + return False + + +def scan_file(path: Path) -> list[str]: + src = path.read_text(encoding="utf-8") + try: + tree = ast.parse(src) + except SyntaxError: + return [] + lines = src.splitlines() + rel = path.relative_to(ROOT).as_posix() + in_config_module = bool(CONFIG_MODULE_RE.match(rel)) + problems: list[str] = [] + + def flag(node: ast.Call, why: str) -> None: + line = lines[node.lineno - 1] + if SUPPRESS in line: + return + problems.append(f"{rel}:{node.lineno}: {why} — route it through hermes_cli.config.atomic_config_write") + + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + if _is_yaml_dump(node, src): + if in_config_module: + flag(node, "PyYAML dump inside the config system") + continue + first = ast.get_source_segment(src, node.args[0]) if node.args else "" + if first and CONFIG_PATH_RE.search(first): + flag(node, f"PyYAML dump of a config path ({first})") + elif _func_name(node.func) == "write_text" and isinstance(node.func, ast.Attribute): + receiver = ast.get_source_segment(src, node.func.value) or "" + body = ast.get_source_segment(src, node) or "" + if CONFIG_PATH_RE.search(receiver) and re.search(r"\bdump\(", body): + flag(node, f"write_text of a YAML dump onto a config path ({receiver})") + return problems + + +def main(argv: list[str]) -> int: + targets = [ROOT / a for a in argv] or [ROOT / t for t in DEFAULT_TREES] + files: list[Path] = [] + for t in targets: + if t.is_dir(): + files.extend(p for p in t.rglob("*.py") if "node_modules" not in p.parts) + elif t.suffix == ".py" and t.exists(): + files.append(t) + problems: list[str] = [] + for f in sorted(set(files)): + if f.resolve() in ALLOWED_FILES: + continue + problems.extend(scan_file(f)) + if problems: + print("config.yaml must only be written through the comment-preserving writer:", file=sys.stderr) + for p in problems: + print(f" {p}", file=sys.stderr) + return 1 + print(f"check_config_yaml_writers: OK ({len(files)} files)") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/tests/gateway/test_dm_topics.py b/tests/gateway/test_dm_topics.py index 9bf90c3986..8330e8cb81 100644 --- a/tests/gateway/test_dm_topics.py +++ b/tests/gateway/test_dm_topics.py @@ -269,7 +269,7 @@ def test_persist_dm_topic_thread_id_preserves_config_on_write_failure(tmp_path): with patch.object(Path, "home", return_value=tmp_path), \ patch.dict(os.environ, {"HERMES_HOME": str(tmp_path / ".hermes")}), \ - patch("yaml.dump", side_effect=fail_dump): + patch("ruamel.yaml.YAML.dump", side_effect=fail_dump): adapter._persist_dm_topic_thread_id(111, "General", 999) assert config_file.read_text(encoding="utf-8") == original_text diff --git a/tests/hermes_cli/test_config.py b/tests/hermes_cli/test_config.py index fcfa9d1797..78cc50f985 100644 --- a/tests/hermes_cli/test_config.py +++ b/tests/hermes_cli/test_config.py @@ -555,9 +555,9 @@ class TestSaveConfigAtomicity: config_path = tmp_path / "config.yaml" assert config_path.exists() - # Simulate a crash during yaml.dump by making atomic_yaml_write's - # yaml.dump raise after the temp file is created but before replace. - with patch("utils.yaml.dump", side_effect=OSError("disk full")): + # Simulate a crash mid-dump: the round-trip writer raises after the temp file is + # created but before replace. + with patch("utils._roundtrip_dump", side_effect=OSError("disk full")): try: config["model"] = "should-not-persist" save_config(config) @@ -574,7 +574,7 @@ class TestSaveConfigAtomicity: config = load_config() save_config(config) - with patch("utils.yaml.dump", side_effect=OSError("disk full")): + with patch("ruamel.yaml.YAML.dump", side_effect=OSError("disk full")): try: save_config(config) except OSError: diff --git a/tests/hermes_cli/test_config_yaml_comment_preservation.py b/tests/hermes_cli/test_config_yaml_comment_preservation.py new file mode 100644 index 0000000000..fa27441423 --- /dev/null +++ b/tests/hermes_cli/test_config_yaml_comment_preservation.py @@ -0,0 +1,152 @@ +"""Regression guard for #92554: every config.yaml writer preserves user comments and key order. + +Each test seeds a hand-commented config.yaml, runs one production write path against it and +asserts that every comment survives, top-level key order is unchanged, and the written value +landed. A new writer that reaches for PyYAML instead of ``atomic_config_write`` fails here or in +``scripts/check_config_yaml_writers.py`` (also exercised below). +""" + +import os +import subprocess +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest +import yaml + +REPO = Path(__file__).resolve().parents[2] + +COMMENTED = """\ +# TOP COMMENT — rationale for this whole file must survive +_config_version: 12 +model: + provider: test # pinned for eval reproducibility + default: some-model + +plugins: + # rationale for the enabled list + enabled: + - alpha # kept for the demo account +approvals: + mode: "off" # quoted on purpose: PyYAML reads bare off as False +hooks: + pre_tool_call: + # why this hook exists + - command: /bin/echo + timeout: 10 +""" + +COMMENTS = [ + "# TOP COMMENT — rationale for this whole file must survive", + "# pinned for eval reproducibility", + "# rationale for the enabled list", + "# kept for the demo account", + "# quoted on purpose: PyYAML reads bare off as False", + "# why this hook exists", +] +KEY_ORDER = ["_config_version", "model", "plugins", "approvals", "hooks"] + + +@pytest.fixture +def home(tmp_path): + (tmp_path / ".env").touch() + (tmp_path / "config.yaml").write_text(COMMENTED, encoding="utf-8") + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + from hermes_cli import config as config_mod + config_mod._RAW_CONFIG_CACHE.clear() + yield tmp_path + + +def _assert_preserved(path: Path) -> dict: + text = path.read_text(encoding="utf-8") + missing = [c for c in COMMENTS if c not in text] + assert not missing, f"comments destroyed by the write: {missing}\n---\n{text}" + data = yaml.safe_load(text) + top = [k for k in data if k in KEY_ORDER] + assert top == KEY_ORDER, f"key order changed: {top}\n---\n{text}" + assert data["approvals"]["mode"] == "off" # not coerced to False + assert "── Security ──" not in text, "stock boilerplate was appended to an existing file" + return data + + +class TestEveryWriterPreservesComments: + def test_config_set(self, home): + from hermes_cli.config import set_config_value + + set_config_value("streaming.enabled", "true") + data = _assert_preserved(home / "config.yaml") + assert data["streaming"]["enabled"] is True + + def test_config_unset(self, home): + from hermes_cli.config import unset_config_value + + unset_config_value("model.default") + data = _assert_preserved(home / "config.yaml") + assert "default" not in data["model"] + + def test_save_config_plugin_enable_and_memory_provider(self, home): + """The bulk writer behind ``plugins enable``, ``memory setup``, the wizard and the dashboard.""" + from hermes_cli.config import load_config, save_config + + cfg = load_config() + cfg["plugins"]["enabled"].append("beta") + cfg.setdefault("memory", {})["provider"] = "honcho" + save_config(cfg) + data = _assert_preserved(home / "config.yaml") + assert data["plugins"]["enabled"] == ["alpha", "beta"] + assert data["memory"]["provider"] == "honcho" + + def test_migration_version_bump(self, home): + from hermes_cli.config import check_config_version, migrate_config + + _, latest = check_config_version() + migrate_config(interactive=False, quiet=True) + data = _assert_preserved(home / "config.yaml") + assert data["_config_version"] == latest + + def test_atomic_config_write_direct(self, home): + """Direct callers (auth provider reset, gateway slash commands, telegram, doctor).""" + from hermes_cli.config import atomic_config_write, read_user_config_raw + + raw = read_user_config_raw(home / "config.yaml") + raw["model"]["provider"] = "auto" + atomic_config_write(home / "config.yaml", raw) + data = _assert_preserved(home / "config.yaml") + assert data["model"]["provider"] == "auto" + + def test_boilerplate_only_on_create(self, home): + from hermes_cli.config import save_config + + (home / "config.yaml").unlink() + save_config({"model": {"provider": "test"}}) + text = (home / "config.yaml").read_text(encoding="utf-8") + assert "── Security ──" in text # fresh file gets the commented examples once + save_config({"model": {"provider": "test", "default": "m"}}) + assert (home / "config.yaml").read_text(encoding="utf-8").count("── Security ──") == 1 + + +class TestStaticGuard: + def test_repo_has_no_stray_config_writers(self): + proc = subprocess.run( + [sys.executable, str(REPO / "scripts" / "check_config_yaml_writers.py")], + capture_output=True, text=True, cwd=REPO, timeout=120) + assert proc.returncode == 0, proc.stderr + + def test_guard_flags_pyyaml_dump_of_config_path(self, tmp_path): + sys.path.insert(0, str(REPO / "scripts")) + try: + import check_config_yaml_writers as guard + finally: + sys.path.pop(0) + bad = tmp_path / "hermes_cli" / "bad_writer.py" + bad.parent.mkdir() + bad.write_text( + "import yaml\nfrom utils import atomic_yaml_write\n" + "def a(config_path, cfg):\n atomic_yaml_write(config_path, cfg)\n" + "def b(cfg_path, cfg):\n cfg_path.write_text(yaml.safe_dump(cfg))\n" + "def c(other_path, cfg):\n atomic_yaml_write(other_path, cfg)\n", + encoding="utf-8") + with patch.object(guard, "ROOT", tmp_path): + problems = guard.scan_file(bad) + assert [p.split(":")[1] for p in problems] == ["4", "6"], problems diff --git a/tests/hermes_cli/test_model_provider_persistence.py b/tests/hermes_cli/test_model_provider_persistence.py index 5607591c0d..e3a1d0639d 100644 --- a/tests/hermes_cli/test_model_provider_persistence.py +++ b/tests/hermes_cli/test_model_provider_persistence.py @@ -54,8 +54,8 @@ class TestSaveModelChoiceAlwaysDict: class TestProviderPersistsAfterModelSave: - def test_update_config_for_provider_uses_atomic_yaml_write(self, config_home): - """Provider switches should delegate config writes to atomic_yaml_write.""" + def test_update_config_for_provider_uses_atomic_config_write(self, config_home): + """Provider switches delegate config writes to the comment-preserving config writer.""" from hermes_cli.auth import _update_config_for_provider config_path = config_home / "config.yaml" @@ -66,10 +66,9 @@ class TestProviderPersistsAfterModelSave: assert data["model"]["provider"] == "nous" assert data["model"]["base_url"] == "https://inference.example.com/v1" assert data["model"]["default"] == "some-old-model" - assert kwargs["sort_keys"] is False raise OSError("simulated atomic write failure") - with patch("hermes_cli.auth.atomic_yaml_write", side_effect=_boom) as mock_write: + with patch("hermes_cli.auth.atomic_config_write", side_effect=_boom) as mock_write: with pytest.raises(OSError, match="simulated atomic write failure"): _update_config_for_provider( "nous", diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 5d92ef124f..25b5fa4010 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -1240,11 +1240,9 @@ def _load_cfg() -> dict: def _save_cfg(cfg: dict): global _cfg_cache, _cfg_sig, _cfg_path - from utils import atomic_roundtrip_yaml_save + from hermes_cli.config import atomic_config_write path = _active_config_path() - # Comment-, ordering- and Unicode-preserving write (a plain safe_dump clobbered hand-written configs); - # fails closed on an unreadable existing config.yaml like atomic_config_write. - atomic_roundtrip_yaml_save(path, cfg) + atomic_config_write(path, cfg) with _cfg_lock: _cfg_cache, _cfg_path = copy.deepcopy(cfg), path try: diff --git a/utils.py b/utils.py index 64b8c5436f..8add707c0c 100644 --- a/utils.py +++ b/utils.py @@ -441,12 +441,20 @@ def _roundtrip_load(path: Path): yaml_rt.allow_unicode = True yaml_rt.default_flow_style = False yaml_rt.indent(mapping=2, sequence=4, offset=2) + # PyYAML (every reader in the tree) tolerates duplicate keys (last wins); refusing them here + # would turn a file the CLI can read into one it cannot write. + yaml_rt.allow_duplicate_keys = True data = yaml_rt.load(path.read_text(encoding="utf-8")) if path.exists() else None return yaml_rt, data if isinstance(data, CommentedMap) else CommentedMap(data or {}) -def _roundtrip_dump(path: Path, yaml_rt, config) -> None: - _atomic_write(path, lambda f: yaml_rt.dump(config, f), prefix=f".{path.stem}_", mode=_preserve_file_mode(path)) +def _roundtrip_dump(path: Path, yaml_rt, config, *, extra_content: "str | None" = None) -> None: + def _write(f) -> None: + yaml_rt.dump(config, f) + if extra_content: + f.write(extra_content) + + _atomic_write(path, _write, prefix=f".{path.stem}_", mode=_preserve_file_mode(path)) def atomic_roundtrip_yaml_update(path: Union[str, Path], key_path: str, value: Any) -> None: @@ -499,15 +507,39 @@ def atomic_roundtrip_yaml_update(path: Union[str, Path], key_path: str, value: A _YAML11_AMBIGUOUS_WORDS = frozenset({"y", "n", "yes", "no", "true", "false", "on", "off", "null", "~"}) -def atomic_roundtrip_yaml_save(path: Union[str, Path], new_state: dict) -> None: +def _rt_value(value: Any) -> Any: + """Plain Python value → ruamel node: YAML 1.1-ambiguous strings force-quoted at every depth + (a bare ``off`` inside a list reads back as ``False`` just like one at the top level).""" + from ruamel.yaml.comments import CommentedMap, CommentedSeq + from ruamel.yaml.scalarstring import DoubleQuotedScalarString + + if isinstance(value, dict): + node = CommentedMap() + for k, v in value.items(): + node[k] = _rt_value(v) + return node + if isinstance(value, (list, tuple)): + return CommentedSeq(_rt_value(v) for v in value) + if isinstance(value, str) and value.lower() in _YAML11_AMBIGUOUS_WORDS: + return DoubleQuotedScalarString(value) + return value + + +def atomic_roundtrip_yaml_save(path: Union[str, Path], new_state: dict, *, + extra_content_on_create: "str | None" = None) -> None: """Persist a full config-state dict while preserving comments and ordering. - Comment-safe replacement for ``yaml.safe_dump(cfg, f)``: writes the whole file from - ``new_state`` through ruamel round-trip mode so existing comments, key order, quotes and - readable Unicode survive. + THE writer for ``config.yaml`` (every production caller reaches it through + ``hermes_cli.config.atomic_config_write``): the on-disk document is loaded through ruamel + round-trip mode and *new_state* is merged onto it, so comments, key order, quotes, blank + lines and readable Unicode survive. Only nodes whose value actually changed are reassigned; + an untouched scalar or list keeps its inline comments and formatting. Keys absent from + *new_state* are deleted ("explicit absence": ``cfg.pop(k)`` + save removes ``k`` from disk). + ``extra_content_on_create`` (commented example blocks) is appended only when the file is + being created — re-appending it on every rewrite is how the stock boilerplate replaced + users' own comments (#92554). """ - from ruamel.yaml.comments import CommentedMap - from ruamel.yaml.scalarstring import DoubleQuotedScalarString + from ruamel.yaml.comments import CommentedMap, CommentedSeq from hermes_cli.config import require_readable_config_before_write path = Path(path) @@ -515,27 +547,43 @@ def atomic_roundtrip_yaml_save(path: Union[str, Path], new_state: dict) -> None: mkdir_under_hermes_home(path.parent) require_readable_config_before_write(path) + creating = not path.exists() or not path.read_text(encoding="utf-8").strip() yaml_rt, existing = _roundtrip_load(path) + def _unchanged(current: Any, value: Any) -> bool: + # ``True == 1`` in Python; a bool↔int flip is a real change for YAML readers. + return current == value and isinstance(current, bool) is isinstance(value, bool) + + def _merge_seq(dst: CommentedSeq, src: list) -> None: + # Element-wise so appending/editing one entry keeps the comments on its siblings. + for i, value in enumerate(src): + if i < len(dst): + _merge_item(dst, i, value) + else: + dst.append(_rt_value(value)) + del dst[len(src):] + + def _merge_item(dst, key, value) -> None: + current = dst[key] + if isinstance(value, dict) and isinstance(current, CommentedMap): + _merge(current, value) + elif isinstance(value, list) and isinstance(current, CommentedSeq): + _merge_seq(current, value) + elif not _unchanged(current, value): + dst[key] = _rt_value(value) + # else: unchanged — keep the existing node and the comments/quoting attached to it + def _merge(dst: CommentedMap, src: dict) -> None: for key, value in src.items(): - if isinstance(value, dict): - current = dst.get(key) - if not isinstance(current, CommentedMap): - current = CommentedMap() - dst[key] = current - _merge(current, value) - elif isinstance(value, str) and value.lower() in _YAML11_AMBIGUOUS_WORDS: - dst[key] = DoubleQuotedScalarString(value) + if key in dst: + _merge_item(dst, key, value) else: - dst[key] = value - # Keys missing from src are deleted: ``cfg.pop("custom_prompt")`` then save must remove - # the key from disk ("explicit absence" semantics of the old _save_cfg pattern). + dst[key] = _rt_value(value) for key in [k for k in dst if k not in src]: del dst[key] _merge(existing, new_state) - _roundtrip_dump(path, yaml_rt, existing) + _roundtrip_dump(path, yaml_rt, existing, extra_content=extra_content_on_create if creating else None) def safe_json_loads(text: str, default: Any = None) -> Any: