From e7d01dd021096d26a697531fd2aa400f2533bf25 Mon Sep 17 00:00:00 2001 From: Phil Thomas Date: Thu, 14 May 2026 15:39:52 -0600 Subject: [PATCH] fix(personality): preserve config comments in TUI/gateway config writes tui_gateway/server.py:_save_cfg called yaml.safe_dump on a deep-loaded config dict, which reordered top-level keys alphabetically, stripped every user-edited comment, and re-escaped non-ASCII (kaomoji/Chinese) personality prompts to \uXXXX. Every TUI setting change - /personality, /reasoning, /details_mode, /skin, /prompt - rewrote the file top to bottom. Changes: * Add atomic_roundtrip_yaml_save(path, new_state) in utils.py - a comment-, ordering-, and unicode-preserving full-state replacement for yaml.safe_dump(cfg, f). Uses ruamel round-trip mode like the existing atomic_roundtrip_yaml_update, but accepts the whole cfg dict so callers that mutate multiple keys before saving (the _save_cfg pattern) don't have to be rewritten. Recurses into nested dicts, deletes keys missing from new_state (preserves the cfg.pop()-then-save semantic), and overwrites lists/scalars wholesale. * Fail closed on an unreadable existing config.yaml the same way hermes_cli.config.atomic_config_write does, via a lazy import of require_readable_config_before_write (avoids a module-level circular import, since hermes_cli.config itself imports from utils). Also preserves both file mode and owner across the write, matching the existing atomic_roundtrip_yaml_update contract. * Force-quote any new string value that YAML 1.1 would misparse as a bool/null (yes/no/on/off/true/false/null/~). ruamel's round-trip dumper resolves against the YAML 1.2 core schema and emits these unquoted, but PyYAML-based readers elsewhere in the codebase parse under YAML 1.1 rules - so an unquoted `approvals.mode: off` would silently round-trip back as the boolean False. * tui_gateway/server.py:_save_cfg now delegates to atomic_roundtrip_yaml_save. Drop-in - all call sites (/personality, /reasoning, /details_mode, /prompt, etc.) inherit comment preservation and the fail-closed contract. Tests: * tests/test_utils_atomic_roundtrip_yaml_save.py - unit tests covering create-from-empty, top-level key-order preservation, comment preservation, readable Unicode, append-new-keys, delete-missing-keys, scalar/list overwrite, nested-dict recursion, refusal on an unreadable existing config, and owner preservation. * tests/test_atomic_replace_symlinks.py - owner-preservation regression test mirroring the existing atomic_roundtrip_yaml_update coverage. * tests/test_tui_gateway_server.py - 4 new tests pinning _save_cfg comment preservation, top-level key-order preservation, and unicode-readability under unrelated writes. Co-Authored-By: Claude Sonnet 5 --- tests/test_atomic_replace_symlinks.py | 23 ++ tests/test_tui_gateway_server.py | 111 +++++++ .../test_utils_atomic_roundtrip_yaml_save.py | 276 ++++++++++++++++++ tui_gateway/server.py | 12 +- utils.py | 118 ++++++++ 5 files changed, 538 insertions(+), 2 deletions(-) create mode 100644 tests/test_utils_atomic_roundtrip_yaml_save.py diff --git a/tests/test_atomic_replace_symlinks.py b/tests/test_atomic_replace_symlinks.py index 888d630891188..f0e77e0167f7b 100644 --- a/tests/test_atomic_replace_symlinks.py +++ b/tests/test_atomic_replace_symlinks.py @@ -29,6 +29,7 @@ if str(_REPO_ROOT) not in sys.path: from utils import ( atomic_json_write, atomic_replace, + atomic_roundtrip_yaml_save, atomic_roundtrip_yaml_update, atomic_yaml_write, ) @@ -170,6 +171,28 @@ def test_atomic_yaml_write_restores_owner_on_real_symlink_target( +def test_atomic_roundtrip_yaml_save_restores_owner( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Mirrors the update-variant owner test for the whole-state save that + backs tui_gateway/server.py:_save_cfg().""" + if os.name != "posix": + pytest.skip("POSIX-only") + + target = tmp_path / "config.yaml" + target.write_text("model:\n provider: openrouter\n", encoding="utf-8") + + chown_calls: list[tuple[Path, int, int]] = [] + monkeypatch.setattr("utils._preserve_file_owner", lambda _path: (345, 678)) + monkeypatch.setattr( + "utils.os.chown", + lambda path, uid, gid: chown_calls.append((Path(path), uid, gid)), + ) + + atomic_roundtrip_yaml_save(target, {"model": {"provider": "nvidia"}}) + + assert chown_calls == [(target, 345, 678)] + assert yaml.safe_load(target.read_text(encoding="utf-8"))["model"]["provider"] == "nvidia" # ─── Broken-symlink edge case ───────────────────────────────────────────── diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 7465966c7fedc..eb45736453d62 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -16544,3 +16544,114 @@ def test_session_branch_keeps_reasoning_fields(monkeypatch, tmp_path): finally: server._sessions.pop("sid", None) db.close() + + +# ── _save_cfg comment-preservation regression tests ──────────────────────── +# +# Until ~mid-2026 _save_cfg used yaml.safe_dump on a deep-loaded config dict. +# Every /personality (or /reasoning, /details_mode, /prompt, ...) write +# silently rewrote ~/.hermes/config.yaml top-to-bottom — top-level keys +# reordered alphabetically, comments stripped, kaomoji/Chinese in stored +# personality prompts mangled to \uXXXX escapes. These tests pin the +# user-visible behavior of the comment-preserving replacement so we don't +# regress. + + +def test_save_cfg_preserves_user_comments(tmp_path, monkeypatch): + """The TUI gateway must not strip user-edited comments on setting writes.""" + cfg_path = tmp_path / "config.yaml" + cfg_path.write_text( + "# top of file note\n" + "model:\n" + " # provider rationale\n" + " default: claude-opus-4-7\n" + "display:\n" + " skin: default # trailing skin note\n", + encoding="utf-8", + ) + monkeypatch.setattr(server, "_hermes_home", tmp_path) + + server._save_cfg( + { + "model": {"default": "claude-opus-4-7"}, + "display": {"skin": "mono"}, + } + ) + + text = cfg_path.read_text(encoding="utf-8") + assert "# top of file note" in text + assert "# provider rationale" in text + assert "# trailing skin note" in text + + import yaml as _yaml + + parsed = _yaml.safe_load(text) + assert parsed["display"]["skin"] == "mono" + + +def test_save_cfg_preserves_top_level_key_order(tmp_path, monkeypatch): + """Top-level keys must keep the file's hand-edited ordering instead of + being rewritten alphabetically by the underlying YAML dumper.""" + cfg_path = tmp_path / "config.yaml" + cfg_path.write_text( + "model:\n" + " default: claude-opus-4-7\n" + "toolsets:\n" + " - hermes-cli\n" + "agent:\n" + " max_turns: 90\n" + "display:\n" + " skin: default\n", + encoding="utf-8", + ) + monkeypatch.setattr(server, "_hermes_home", tmp_path) + + # Caller's dict iteration order is intentionally alphabetical to confirm + # the helper consults disk order, not caller order. + server._save_cfg( + { + "agent": {"max_turns": 90}, + "display": {"skin": "mono"}, + "model": {"default": "claude-opus-4-7"}, + "toolsets": ["hermes-cli"], + } + ) + + text = cfg_path.read_text(encoding="utf-8") + top_keys = [ + line.split(":", 1)[0] + for line in text.splitlines() + if line and not line.startswith(" ") and not line.startswith("-") + and not line.startswith("#") + ] + assert top_keys == ["model", "toolsets", "agent", "display"] + + +def test_save_cfg_keeps_unicode_personalities_readable(tmp_path, monkeypatch): + """The catgirl/kawaii personality prompts must stay readable on disk + instead of being \\uXXXX-escaped on every unrelated setting write.""" + cfg_path = tmp_path / "config.yaml" + cfg_path.write_text( + "agent:\n" + " personalities:\n" + " catgirl: \"nya (=^･ω･^=) 你好\"\n" + "display:\n" + " skin: default\n", + encoding="utf-8", + ) + monkeypatch.setattr(server, "_hermes_home", tmp_path) + + # Simulate an unrelated /skin write — must not corrupt the catgirl + # personality string sitting in agent.personalities. + server._save_cfg( + { + "agent": {"personalities": {"catgirl": "nya (=^･ω･^=) 你好"}}, + "display": {"skin": "mono"}, + } + ) + + text = cfg_path.read_text(encoding="utf-8") + assert "你好" in text + assert "(=^･ω･^=)" in text + assert "\\u4f60" not in text + diff --git a/tests/test_utils_atomic_roundtrip_yaml_save.py b/tests/test_utils_atomic_roundtrip_yaml_save.py new file mode 100644 index 0000000000000..ff89258b814f7 --- /dev/null +++ b/tests/test_utils_atomic_roundtrip_yaml_save.py @@ -0,0 +1,276 @@ +"""Tests for atomic_roundtrip_yaml_save() — comment-preserving full-state writes. + +This helper backs tui_gateway/server.py:_save_cfg(), which used to call +yaml.safe_dump and silently clobber user-edited config files on every +TUI/gateway setting change (e.g. /personality, /reasoning, /details_mode). +""" + +import os +from pathlib import Path +from unittest.mock import patch + +import pytest +import yaml + + +class TestAtomicRoundtripYamlSave: + @pytest.fixture + def config_path(self, tmp_path): + return tmp_path / "config.yaml" + + def test_creates_file_when_missing(self, config_path): + from utils import atomic_roundtrip_yaml_save + + atomic_roundtrip_yaml_save(config_path, {"model": {"default": "test-model"}}) + + assert config_path.exists() + assert yaml.safe_load(config_path.read_text())["model"]["default"] == "test-model" + + def test_preserves_top_level_key_order(self, config_path): + """Existing top-level keys keep their author-intended ordering.""" + config_path.write_text( + "model:\n" + " default: claude-opus-4-7\n" + "providers: {}\n" + "agent:\n" + " max_turns: 90\n" + "display:\n" + " skin: default\n", + encoding="utf-8", + ) + + from utils import atomic_roundtrip_yaml_save + + # Pass keys in alphabetical order to make sure dict iteration order + # in the caller doesn't accidentally rewrite the file alphabetically + # (the old yaml.safe_dump bug). + atomic_roundtrip_yaml_save( + config_path, + { + "agent": {"max_turns": 100}, + "display": {"skin": "mono"}, + "model": {"default": "claude-opus-4-7"}, + "providers": {}, + }, + ) + + text = config_path.read_text(encoding="utf-8") + top_keys = [ + line.split(":", 1)[0] + for line in text.splitlines() + if line and not line.startswith(" ") and not line.startswith("#") + ] + # Comments are stripped from `top_keys` above, so the surviving + # order should match the original file, NOT alphabetical. + assert top_keys == ["model", "providers", "agent", "display"] + + def test_preserves_comments(self, config_path): + config_path.write_text( + "# header comment\n" + "model:\n" + " # inline note\n" + " default: claude-opus-4-7\n" + "display:\n" + " skin: default # trailing note\n", + encoding="utf-8", + ) + + from utils import atomic_roundtrip_yaml_save + + atomic_roundtrip_yaml_save( + config_path, + { + "model": {"default": "claude-opus-4-7"}, + "display": {"skin": "mono"}, + }, + ) + + text = config_path.read_text(encoding="utf-8") + assert "# header comment" in text + assert "# inline note" in text + assert "# trailing note" in text + assert yaml.safe_load(text)["display"]["skin"] == "mono" + + def test_preserves_readable_unicode(self, config_path): + """Personalities with kaomoji/Chinese characters stay readable on disk + instead of getting mangled to \\uXXXX escapes (the headline bug: + kawaii/catgirl personality emoji turning into \\u30CE\\uFF65).""" + config_path.write_text( + "agent:\n" + " personalities:\n" + " catgirl: \"nya (=^･ω･^=) 你好\"\n" + "display:\n" + " skin: default\n", + encoding="utf-8", + ) + + from utils import atomic_roundtrip_yaml_save + + atomic_roundtrip_yaml_save( + config_path, + { + "agent": {"personalities": {"catgirl": "nya (=^･ω･^=) 你好"}}, + "display": {"skin": "mono"}, + }, + ) + + text = config_path.read_text(encoding="utf-8") + assert "你好" in text + assert "(=^･ω･^=)" in text + assert "\\u4f60" not in text + assert "\\u30CE" not in text + + def test_appends_new_keys(self, config_path): + config_path.write_text( + "model:\n" + " default: test-model\n", + encoding="utf-8", + ) + + from utils import atomic_roundtrip_yaml_save + + atomic_roundtrip_yaml_save( + config_path, + { + "model": {"default": "test-model"}, + "display": {"personality": "noir"}, + "agent": {"system_prompt": "you are noir"}, + }, + ) + + result = yaml.safe_load(config_path.read_text(encoding="utf-8")) + assert result["model"]["default"] == "test-model" + assert result["display"]["personality"] == "noir" + assert result["agent"]["system_prompt"] == "you are noir" + + def test_deletes_keys_missing_from_new_state(self, config_path): + """Mirrors the cfg.pop()-then-_save_cfg(cfg) pattern in tui_gateway: + e.g. /prompt clear removes custom_prompt entirely.""" + config_path.write_text( + "model:\n" + " default: test-model\n" + "custom_prompt: 'old prompt'\n", + encoding="utf-8", + ) + + from utils import atomic_roundtrip_yaml_save + + atomic_roundtrip_yaml_save( + config_path, + {"model": {"default": "test-model"}}, + ) + + result = yaml.safe_load(config_path.read_text(encoding="utf-8")) + assert "custom_prompt" not in result + assert result["model"]["default"] == "test-model" + + def test_overwrites_scalar_value(self, config_path): + config_path.write_text( + "display:\n" + " personality: noir\n", + encoding="utf-8", + ) + + from utils import atomic_roundtrip_yaml_save + + atomic_roundtrip_yaml_save( + config_path, + {"display": {"personality": "kawaii"}}, + ) + + result = yaml.safe_load(config_path.read_text(encoding="utf-8")) + assert result["display"]["personality"] == "kawaii" + + def test_overwrites_list_wholesale(self, config_path): + config_path.write_text( + "toolsets:\n" + " - one\n" + " - two\n", + encoding="utf-8", + ) + + from utils import atomic_roundtrip_yaml_save + + atomic_roundtrip_yaml_save(config_path, {"toolsets": ["three"]}) + + result = yaml.safe_load(config_path.read_text(encoding="utf-8")) + assert result["toolsets"] == ["three"] + + def test_recurses_into_nested_dicts(self, config_path): + """Deep mutations target the matching subtree, not the whole parent. + + Without this, writing display.personality would drop sibling display.skin. + """ + config_path.write_text( + "display:\n" + " skin: default\n" + " personality: noir\n" + " compact: false\n", + encoding="utf-8", + ) + + from utils import atomic_roundtrip_yaml_save + + atomic_roundtrip_yaml_save( + config_path, + { + "display": { + "skin": "default", + "personality": "kawaii", + "compact": False, + } + }, + ) + + result = yaml.safe_load(config_path.read_text(encoding="utf-8")) + assert result["display"]["skin"] == "default" + assert result["display"]["personality"] == "kawaii" + assert result["display"]["compact"] is False + + @staticmethod + def _deny_config_reads(config_path): + real_open = open + + def fake_open(file, mode="r", *args, **kwargs): + if Path(file) == config_path and "r" in mode: + raise PermissionError("denied") + return real_open(file, mode, *args, **kwargs) + + return fake_open + + def test_refuses_to_overwrite_unreadable_existing_config(self, config_path): + """Shares the fail-closed contract with hermes_cli.config.atomic_config_write: + an existing-but-unreadable config.yaml (permission error, broken mount) + must raise rather than being silently replaced with only new_state.""" + original = "model:\n default: test-model\n" + config_path.write_text(original, encoding="utf-8") + + from utils import atomic_roundtrip_yaml_save + + with patch("builtins.open", side_effect=self._deny_config_reads(config_path)): + with pytest.raises(RuntimeError, match="Refusing to overwrite"): + atomic_roundtrip_yaml_save(config_path, {"model": {"default": "replacement"}}) + + assert config_path.read_text(encoding="utf-8") == original + + def test_restores_owner(self, config_path, monkeypatch): + """Mirrors atomic_roundtrip_yaml_update_restores_owner — the write path + must preserve the config file's original owner across the temp-file + + atomic-replace swap, not just its mode.""" + if os.name != "posix": + pytest.skip("POSIX-only") + + config_path.write_text("model:\n default: test-model\n", encoding="utf-8") + + chown_calls: list[tuple[Path, int, int]] = [] + monkeypatch.setattr("utils._preserve_file_owner", lambda _path: (345, 678)) + monkeypatch.setattr( + "utils.os.chown", + lambda path, uid, gid: chown_calls.append((Path(path), uid, gid)), + ) + + from utils import atomic_roundtrip_yaml_save + + atomic_roundtrip_yaml_save(config_path, {"model": {"default": "updated-model"}}) + + assert chown_calls == [(config_path, 345, 678)] diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 8078ecc6cab47..21966445e1768 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -3114,10 +3114,18 @@ def _apply_managed(cfg: dict) -> dict: def _save_cfg(cfg: dict): global _cfg_cache, _cfg_mtime, _cfg_path - from hermes_cli.config import atomic_config_write + from utils import atomic_roundtrip_yaml_save path = _hermes_home / "config.yaml" - atomic_config_write(path, cfg) + # Comment-, ordering-, and Unicode-preserving full-state write. + # Replaces the previous `yaml.safe_dump(cfg, f)` (and later + # `atomic_config_write`, which is not comment-preserving) which clobbered + # the user's hand-written config every time we touched a single setting + # (top-level keys reordered alphabetically, comments dropped, kaomoji + # mangled to \\uXXXX escapes). Fails closed on an unreadable existing + # config.yaml the same way atomic_config_write does (see + # atomic_roundtrip_yaml_save's require_readable_config_before_write call). + atomic_roundtrip_yaml_save(path, cfg) with _cfg_lock: _cfg_cache = copy.deepcopy(cfg) _cfg_path = path diff --git a/utils.py b/utils.py index 66d2f82be56eb..6d56fc4ca6403 100644 --- a/utils.py +++ b/utils.py @@ -480,6 +480,124 @@ def atomic_roundtrip_yaml_update( raise +def atomic_roundtrip_yaml_save( + path: Union[str, Path], + new_state: dict, +) -> None: + """Persist a full config-state dict while preserving comments and ordering. + + Behaves like ``atomic_yaml_write`` (writes the whole file in one shot from + ``new_state``), but routes through ruamel.yaml round-trip mode so existing + comments, key order, quotes, and readable Unicode survive. + + Reconciliation rules against the on-disk YAML: + + * Keys present in both are updated in-place via assignment, which keeps + ruamel's CommentedMap anchors (and their attached comments) attached to + their original positions. + * Keys missing from ``new_state`` are deleted. + * Keys added in ``new_state`` are appended at the end of their parent map. + * Nested ``dict`` values recurse with the same rules. + * Non-dict values (lists, scalars) are overwritten wholesale — list + element comments are not individually preserved, matching ruamel's + semantics. + + This is the comment-safe replacement for ``yaml.safe_dump(cfg, f)`` in + callers that mutate a deep-loaded config dict and want to persist the + whole thing. + + Shares the fail-closed contract ``hermes_cli.config.atomic_config_write`` + enforces for plain (non-comment-preserving) full-document writes: an + existing-but-unreadable ``config.yaml`` (permission error, broken mount, + transient I/O) raises rather than being silently replaced with only + ``new_state``. Imported lazily to avoid a module-level circular import — + ``hermes_cli.config`` itself imports from this module. + """ + from ruamel.yaml import YAML + from ruamel.yaml.comments import CommentedMap + from ruamel.yaml.scalarstring import DoubleQuotedScalarString + + from hermes_cli.config import require_readable_config_before_write + + path = Path(path) + path.parent.mkdir(parents=True, exist_ok=True) + require_readable_config_before_write(path) + + yaml_rt = YAML(typ="rt") + yaml_rt.preserve_quotes = True + yaml_rt.allow_unicode = True + yaml_rt.default_flow_style = False + yaml_rt.indent(mapping=2, sequence=4, offset=2) + + if path.exists(): + with path.open("r", encoding="utf-8") as f: + existing = yaml_rt.load(f) + if not isinstance(existing, CommentedMap): + existing = CommentedMap(existing or {}) + else: + existing = CommentedMap() + + # ruamel's round-trip dumper resolves plain scalars against the YAML 1.2 + # core schema, where only true/false/null are reserved words — so a plain + # python str like "off" or "yes" is emitted unquoted. Every other config + # reader in this codebase (atomic_config_write's PyYAML path, yaml.safe_load + # call sites, etc.) parses under YAML 1.1 rules, where on/off/yes/no are + # boolean synonyms. Without forcing quotes here, a freshly written + # `approvals.mode: off` silently round-trips back as `False` under + # yaml.safe_load. Force-quote any new string value that YAML 1.1 would + # otherwise misparse as bool/null. + _YAML11_AMBIGUOUS_WORDS = { + "y", "n", "yes", "no", "true", "false", "on", "off", "null", "~", + } + + def _quote_if_yaml11_ambiguous(value): + if isinstance(value, str) and value.lower() in _YAML11_AMBIGUOUS_WORDS: + return DoubleQuotedScalarString(value) + return value + + def _merge(dst: CommentedMap, src: dict) -> None: + # Update / recurse into keys present in src. + 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) + else: + dst[key] = _quote_if_yaml11_ambiguous(value) + # Delete keys missing from src — preserves "explicit absence" semantics + # of the old _save_cfg(cfg) pattern (e.g. cfg.pop("custom_prompt", None) + # then _save_cfg must actually remove the key from disk). + for key in [k for k in dst.keys() if k not in src]: + del dst[key] + + _merge(existing, new_state) + + original_mode = _preserve_file_mode(path) + original_owner = _preserve_file_owner(path) + fd, tmp_path = tempfile.mkstemp( + dir=str(path.parent), + prefix=f".{path.stem}_", + suffix=".tmp", + ) + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + yaml_rt.dump(existing, f) + f.flush() + os.fsync(f.fileno()) + real_path = atomic_replace(tmp_path, path) + real_path_obj = Path(real_path) + _restore_file_owner(real_path_obj, original_owner) + _restore_file_mode(real_path_obj, original_mode) + except BaseException: + try: + os.unlink(tmp_path) + except OSError: + pass + raise + + # ─── JSON Helpers ─────────────────────────────────────────────────────────────