From 649ce1f811332862b9eeec4aac9e82fb130c744e Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:39:55 -0700 Subject: [PATCH] fix(cli): make profile.yaml and skin writes atomic to stop silent field loss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `write_profile_meta` and `hermes skin set` are both read-modify-write helpers that rewrite a user-visible YAML file with a bare truncating write, bypassing `utils.atomic_yaml_write` — the shared helper whose docstring states that "every destructive file rewrite in the codebase shares one implementation". Both read halves swallow a parse error and fall back to `{}`, so a truncated file is not transient corruption. The next call reads `{}` and silently, permanently drops every field the caller did not explicitly pass: * `write_profile_meta` promises "unspecified fields preserve existing values". After an interrupted write, a follow-up call that only sets `description_auto` erases the profile's `description` — it vanishes from `hermes profile list` and never comes back. * `_skin_set` exists so that "changing one token never disturbs the rest of the look". `path.write_text(...)` neither fsyncs nor swaps atomically, so a crash or power loss can leave `.yaml` zero-length; the next tweak then rewrites from empty and the whole palette is gone. The gateway's skin watcher repaints live surfaces from this file within ~1s, so a half-written file is observable. Routing both through `atomic_yaml_write` gives temp file + fsync + `atomic_replace`, which also preserves a symlinked target (GitHub #16743) and restores owner/mode, and emits emoji descriptions as real UTF-8 instead of `\UXXXXXXXX` escapes (GitHub #51356). Supersedes #51808, which fixed the unicode-escaping symptom alone by adding `allow_unicode=True` to the same `yaml.safe_dump` call. --- hermes_cli/profiles.py | 13 +++- hermes_cli/skin_cmd.py | 14 +++- tests/hermes_cli/test_profiles.py | 104 ++++++++++++++++++++++++++++++ tests/hermes_cli/test_skin_cmd.py | 66 +++++++++++++++++++ 4 files changed, 193 insertions(+), 4 deletions(-) diff --git a/hermes_cli/profiles.py b/hermes_cli/profiles.py index 71acde626c212..a95484d338694 100644 --- a/hermes_cli/profiles.py +++ b/hermes_cli/profiles.py @@ -870,8 +870,17 @@ def write_profile_meta( existing["description"] = description.strip() if description_auto is not None: existing["description_auto"] = bool(description_auto) - with open(path, "w", encoding="utf-8") as f: - yaml.safe_dump(existing, f, sort_keys=False, default_flow_style=False) + # Route through the shared atomic helper (temp file + fsync + replace). + # A bare ``open(path, "w")`` truncates before the dump, so a crash or + # SIGINT mid-write leaves profile.yaml empty or half-written. The read + # half above swallows the resulting parse error and falls back to + # ``{}``, so the NEXT call would silently and permanently drop every + # field the caller did not pass — breaking this function's documented + # merge contract. atomic_yaml_write also emits emoji descriptions as + # real UTF-8 rather than ``\UXXXXXXXX`` escapes (GitHub #51356). + from utils import atomic_yaml_write + + atomic_yaml_write(path, existing, sort_keys=False, default_flow_style=False) # --------------------------------------------------------------------------- diff --git a/hermes_cli/skin_cmd.py b/hermes_cli/skin_cmd.py index 9e181a2a38816..103a4a80c4ee8 100644 --- a/hermes_cli/skin_cmd.py +++ b/hermes_cli/skin_cmd.py @@ -72,8 +72,18 @@ def _skin_set(key: str, value: str, skin: str | None) -> int: data["colors"][key] = value data.setdefault("name", target) - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(yaml.safe_dump(data, sort_keys=False, allow_unicode=True), encoding="utf-8") + # Route through the shared atomic helper (temp file + fsync + replace; + # it creates the parent dir and keeps allow_unicode=True for the kaomoji + # cursors and box-drawing tool prefixes). ``write_text`` truncates and + # writes with no fsync and no atomic swap, so a crash or power loss can + # leave .yaml zero-length — and the read above falls back to ``{}`` + # on a parse error, so the next ``hermes skin set`` rewrites from empty + # and permanently drops the rest of the palette. The gateway's skin + # watcher repaints live surfaces from this file within ~1s, so a + # half-written file is observable, not only a crash-window concern. + from utils import atomic_yaml_write + + atomic_yaml_write(path, data, sort_keys=False) if target != name: _use(target) diff --git a/tests/hermes_cli/test_profiles.py b/tests/hermes_cli/test_profiles.py index 3aa9643f0e414..cb5e81d548552 100644 --- a/tests/hermes_cli/test_profiles.py +++ b/tests/hermes_cli/test_profiles.py @@ -710,6 +710,110 @@ class TestInternalHelpers: +# =================================================================== +# TestWriteProfileMetaDurability +# =================================================================== + +class TestWriteProfileMetaDurability: + """``profile.yaml`` must survive an interrupted ``write_profile_meta``. + + ``write_profile_meta`` is a read-modify-write whose docstring promises + "unspecified fields preserve existing values". Its read half swallows + any parse error and falls back to ``{}``, so a truncated profile.yaml is + not transient corruption — the *next* call reads ``{}`` and silently and + permanently drops every field the caller did not explicitly pass. + """ + + @staticmethod + def _seed(tmp_path): + profile_dir = tmp_path / "coder" + profile_dir.mkdir() + profiles.write_profile_meta( + profile_dir, description="Curated by hand", description_auto=False + ) + return profile_dir + + @staticmethod + def _interrupted_write(profile_dir): + """Run a ``write_profile_meta`` whose serialization fails mid-call. + + The pre-fix code called ``yaml.safe_dump``; ``utils.atomic_yaml_write`` + calls ``yaml.dump``. Breaking both keeps this serializer-agnostic, so + it measures durability rather than the choice of entry point. A + scoped ``MonkeyPatch.context`` is used instead of the fixture so the + patch is reverted immediately, without touching the session-wide env + isolation that shares the function-scoped ``monkeypatch`` instance. + """ + def _boom(*args, **kwargs): + raise RuntimeError("simulated interruption mid-write") + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(yaml, "safe_dump", _boom) + mp.setattr(yaml, "dump", _boom) + with pytest.raises(RuntimeError): + profiles.write_profile_meta(profile_dir, description_auto=True) + + def test_failed_write_leaves_existing_file_intact(self, tmp_path): + profile_dir = self._seed(tmp_path) + path = profile_dir / "profile.yaml" + before = path.read_text(encoding="utf-8") + assert "Curated by hand" in before + + self._interrupted_write(profile_dir) + + # A truncating open() destroys the file before the dump ever runs. + assert path.read_text(encoding="utf-8") == before + + def test_failed_write_does_not_silently_drop_unspecified_fields(self, tmp_path): + profile_dir = self._seed(tmp_path) + + self._interrupted_write(profile_dir) + + # The user retries; this call does not pass a description, so the + # documented merge contract requires the existing one to survive. + profiles.write_profile_meta(profile_dir, description_auto=True) + meta = profiles.read_profile_meta(profile_dir) + assert meta["description"] == "Curated by hand" + assert meta["description_auto"] is True + + def test_emoji_description_is_written_as_real_utf8(self, tmp_path): + """Astral-plane chars must not become ``\\UXXXXXXXX`` escapes (#51356). + + Supersedes #51808, which fixed this symptom alone by adding + ``allow_unicode=True``; ``atomic_yaml_write`` sets it internally. + """ + profile_dir = tmp_path / "wizard" + profile_dir.mkdir() + profiles.write_profile_meta(profile_dir, description="Code wizard 🧙 ✨") + + raw = (profile_dir / "profile.yaml").read_text(encoding="utf-8") + assert "\\U" not in raw + assert "🧙" in raw + assert profiles.read_profile_meta(profile_dir)["description"] == "Code wizard 🧙 ✨" + + def test_symlinked_profile_yaml_survives_the_write(self, tmp_path): + """Guard on the conversion, not a behavior change. + + Dotfile managers (chezmoi/stow) symlink profile.yaml into a tracked + repo. ``open(path, "w")`` wrote through the link; a naive + ``os.replace`` would detach it. ``atomic_replace`` resolves the link + first (GitHub #16743), so the link must still be intact afterwards. + """ + profile_dir = tmp_path / "linked" + profile_dir.mkdir() + real_dir = tmp_path / "dotfiles" + real_dir.mkdir() + real = real_dir / "profile.yaml" + real.write_text("description: from dotfiles\n", encoding="utf-8") + (profile_dir / "profile.yaml").symlink_to(real) + + profiles.write_profile_meta(profile_dir, description="updated") + + assert (profile_dir / "profile.yaml").is_symlink() + assert "updated" in real.read_text(encoding="utf-8") + assert [p.name for p in profile_dir.iterdir() if p.name.endswith(".tmp")] == [] + + # =================================================================== # Edge cases and additional coverage # =================================================================== diff --git a/tests/hermes_cli/test_skin_cmd.py b/tests/hermes_cli/test_skin_cmd.py index e8f9f179962a0..d5567b6e634eb 100644 --- a/tests/hermes_cli/test_skin_cmd.py +++ b/tests/hermes_cli/test_skin_cmd.py @@ -4,6 +4,9 @@ The whole point is that changing one token never disturbs the rest of the look (background especially), which hand-authoring kept getting wrong. """ +import os + +import pytest import yaml from hermes_cli import skin_cmd @@ -55,3 +58,66 @@ def test_set_forks_a_builtin_without_inventing_a_background(): def test_set_rejects_non_hex(): _activate("default") assert skin_cmd._skin_set("ui_tool", "teal", None) == 1 + + +def test_set_persists_the_skin_durably(): + """The palette must reach disk before ``skin set`` returns. + + ``write_text`` truncates and writes without ever calling ``fsync``, so a + power loss or container kill right after the command "succeeds" can leave + ``.yaml`` zero-length. That is not transient: ``_skin_set`` is a + read-modify-write whose read half falls back to ``{}``, so the next tweak + rewrites the file from that empty state and the rest of the palette is + gone for good — and the gateway's skin watcher repaints every live + surface from this file within ~1s either way. + """ + path = _skins() / "oasis.yaml" + path.write_text( + 'name: oasis\ncolors:\n background: "#08201f"\n banner_title: "#f2dfb3"\n', + encoding="utf-8", + ) + _activate("oasis") + + synced = [] + real_fsync = os.fsync + + def _tracking_fsync(fd): + synced.append(fd) + return real_fsync(fd) + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(os, "fsync", _tracking_fsync) + assert skin_cmd._skin_set("ui_tool", "#00FFFF", None) == 0 + + assert synced, "skin file written without fsync — a crash can leave it empty" + + data = yaml.safe_load(path.read_text(encoding="utf-8")) + assert data["colors"]["ui_tool"] == "#00FFFF" + assert data["colors"]["background"] == "#08201f" # untouched — the whole point + assert data["colors"]["banner_title"] == "#f2dfb3" + assert [p.name for p in _skins().iterdir() if p.name.endswith(".tmp")] == [] + + +def test_set_preserves_a_symlinked_skin_file(): + """Guard on the conversion, not a behavior change. + + ``write_text`` wrote through a symlink; a naive ``os.replace`` would + detach it. ``atomic_replace`` resolves the link first (GitHub #16743), + so a skin file symlinked out to a dotfiles repo must stay a symlink. + """ + real_dir = _skins().parent / "dotfiles" + real_dir.mkdir(parents=True, exist_ok=True) + real = real_dir / "oasis.yaml" + real.write_text( + 'name: oasis\ncolors:\n background: "#08201f"\n', encoding="utf-8" + ) + link = _skins() / "oasis.yaml" + link.symlink_to(real) + _activate("oasis") + + assert skin_cmd._skin_set("ui_tool", "#00FFFF", None) == 0 + + assert link.is_symlink() + data = yaml.safe_load(real.read_text(encoding="utf-8")) + assert data["colors"]["ui_tool"] == "#00FFFF" + assert data["colors"]["background"] == "#08201f"