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 <noreply@anthropic.com>
This commit is contained in:
Phil Thomas 2026-05-14 15:39:52 -06:00 committed by Teknium
parent 7aecab56db
commit e7d01dd021
5 changed files with 538 additions and 2 deletions

View File

@ -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 ─────────────────────────────────────────────

View File

@ -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

View File

@ -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)]

View File

@ -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

118
utils.py
View File

@ -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 ─────────────────────────────────────────────────────────────