fix(cli): make profile.yaml and skin writes atomic to stop silent field loss
`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 `<skin>.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.
This commit is contained in:
parent
652ebc5899
commit
649ce1f811
|
|
@ -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)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -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 <skin>.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)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
# ===================================================================
|
||||
|
|
|
|||
|
|
@ -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
|
||||
``<skin>.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"
|
||||
|
|
|
|||
Loading…
Reference in New Issue