fix(cli): make profile.yaml and skin writes atomic to stop silent field loss (supersedes #51808)

[briandevans]

What does this PR do?

Two Channel-A CLI helpers rewrite a user-visible YAML file with a bare truncating write, bypassing utils.atomic_yaml_write — the shared helper whose own docstring states the invariant:

"Used by the memory store, skill manager, and agent importer so that every destructive file rewrite in the codebase shares one implementation."

Both are read-modify-write helpers whose read half degrades an unreadable file to {}. That is what turns a torn write from transient corruption into silent, permanent data loss: the next call reads {} and drops every field the caller did not explicitly pass. The two get there differently, and the distinction matters — write_profile_meta wraps its read in try/except Exception: existing = {}, so it absorbs any corruption; _skin_set has no try/except, so invalid YAML raises and aborts loudly, and only a zero-length file degrades silently (safe_load("") is None, then or {}). Zero-length is precisely what an unsynced, non-atomic write leaves behind, so the loss chain holds for both.

hermes_cli/profiles.py — write_profile_meta (sharpest). Its docstring promises "Only the explicitly passed fields are overwritten; unspecified fields preserve existing values." open(path, "w") truncates before yaml.safe_dump runs, so a crash or Ctrl-C mid-write leaves profile.yaml empty. The user retries; that call passes only description_auto, the read falls back to {} — and the profile's description is gone from hermes profile list for good, with no error surfaced. This is a direct violation of the function's documented merge contract.

hermes_cli/skin_cmd.py — _skin_set. The module docstring says set "changes ONE color of the ACTIVE skin in place, so tweaking (say) the tool marker never disturbs the rest of the look — background included." path.write_text(...) neither fsyncs nor swaps atomically, so a crash or power loss can leave <skin>.yaml zero-length; the next hermes skin set then loads that as None, falls back to {}, and the rest of the palette is permanently gone. The gateway's skin watcher repaints every live surface from this file within ~1s, so a half-written file is directly observable, not just a crash-window concern.

Routing both through atomic_yaml_write gives temp file + fsync + atomic_replace, which additionally preserves a symlinked target (GitHub #16743 — dotfile managers symlink these paths into a tracked repo), restores owner and mode, and emits emoji descriptions and kaomoji cursors as real UTF-8 instead of \UXXXXXXXX escapes (GitHub #51356).

Supersedes #51808

@srojk34's #51808 (open since 2026-06-24, no reviews) targets the exact line this PR replaces, adding allow_unicode=True to that yaml.safe_dump call. atomic_yaml_write passes allow_unicode=True internally, so this change strictly subsumes it — it fixes the unicode escaping and the torn write. test_emoji_description_is_written_as_real_utf8 covers #51808's case and is verified to fail on current main. The assertion lives in the existing tests/hermes_cli/test_profiles.py rather than in a new file, so nothing collides with #51808's own test module if the maintainers prefer to land that one instead.

Sibling-site sweep — every yaml write site in Channel A

Enumerated with git ls-tree -r --name-only origin/main | grep -E '^(hermes_cli/|gateway/|tui_gateway/|cli\.py|utils\.py|hermes_state.*\.py)' piped through grep -n 'yaml.safe_dump\|yaml.dump\|write_text(yaml'. Six hits, all accounted for:

Site	Verdict
hermes_cli/profiles.py:871	fixed here — truncating open(w) + safe_dump on profile.yaml
hermes_cli/skin_cmd.py:76	fixed here — write_text(safe_dump(...)) on ~/.hermes/skins/<name>.yaml
hermes_cli/xai_retirement.py:249	deliberately excluded — see below
hermes_cli/config.py:1185	excluded — return yaml.safe_dump(value), a formatter returning a string; no file touched
hermes_cli/profile_distribution.py:257	excluded — same, return yaml.safe_dump(data, ...); no file touched
utils.py:345	excluded — this is atomic_yaml_write's implementation

Why xai_retirement.py:249 is not converted here. That site dumps a ruamel.yaml round-trip document (YAML(typ="rt")), which exists precisely to preserve the user's comments and quoting in config.yaml. atomic_yaml_write is PyYAML-based and raises RepresenterError: cannot represent an object on a CommentedMap — I verified this directly, so the "obvious" one-line conversion there would break the migration, not harden it. Making that path atomic needs a round-trip-aware helper (the pattern already inlined in utils.atomic_roundtrip_yaml_update), which is a separate change with its own blast radius. It is also the least exposed of the three: it is a one-shot migration, already guarded by require_readable_config_before_write, and takes a shutil.copy2 backup to .bak-pre-migrate-xai-<ts> on the default path, so loss there is recoverable. Happy to follow up on it separately if you'd like it in the same series.

Precedent on these exact files

main already carries a merged symlink-preservation and atomic-write series across hermes_cli/profiles.py — b6b9bcd2a (profile export), 8d9684c9d (default-export paths, #58394), b7192b1cb (clone-all + skills clone) — plus #3800, #4320/#4298, #18217, #10618 and #16980 elsewhere. Those covered the clone/export paths; the profile.yaml metadata write was never converted. This ships the missed site of a series you already merged.

Related Issue

No filed issue — found by sweeping the remaining destructive YAML write sites against utils.atomic_yaml_write's stated single-implementation invariant.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 🔒 Security fix
• 📝 Documentation update
• ✅ Tests (adding or improving test coverage)
• ♻️ Refactor (no behavior change)
• 🎯 New skill (bundled or hub)

Changes Made

• hermes_cli/profiles.py — write_profile_meta now calls atomic_yaml_write(path, existing, sort_keys=False, default_flow_style=False) instead of open(path, "w") + yaml.safe_dump. Same kwargs, same defaults.
• hermes_cli/skin_cmd.py — _skin_set now calls atomic_yaml_write(path, data, sort_keys=False) instead of path.write_text(yaml.safe_dump(..., allow_unicode=True)). The explicit path.parent.mkdir(...) is dropped because the helper does it; allow_unicode=True is set inside the helper, so the kaomoji cursors and box-drawing tool_prefix values still round-trip as UTF-8.
• tests/hermes_cli/test_profiles.py — new TestWriteProfileMetaDurability (4 tests), inserted mid-file between TestInternalHelpers and TestEdgeCases rather than appended at EOF, to avoid colliding with the several open PRs that append there.
• tests/hermes_cli/test_skin_cmd.py — 2 new tests.

One behavioral note: the helper serializes with IndentDumper, so nested sequences gain 2-space indentation. That is the repo's canonical layout (#31999, aligning PyYAML with the ruamel writers); neither file's schema currently contains a list (profile.yaml is description/description_auto; skins are flat str -> str maps plus tool_prefix), so no existing file's bytes change.

How to Test

uv run --with pytest --with pytest-asyncio python3 -m pytest \
  tests/hermes_cli/test_profiles.py tests/hermes_cli/test_skin_cmd.py -v

Fail-before / pass-after was verified in both directions by stashing only the two production hunks and re-running the new tests against unmodified main:

Test	On main	With this PR
test_failed_write_leaves_existing_file_intact	FAILED — profile.yaml is already truncated when the dump raises	PASSED
test_failed_write_does_not_silently_drop_unspecified_fields	FAILED — assert '' == 'Curated by hand'	PASSED
test_emoji_description_is_written_as_real_utf8	FAILED — file contains description: "Code wizard \U0001F9D9 ✨"	PASSED
test_set_persists_the_skin_durably	FAILED — no fsync before skin set returns	PASSED
test_symlinked_profile_yaml_survives_the_write	passed (guard)	PASSED
test_set_preserves_a_symlinked_skin_file	passed (guard)	PASSED

The last two are stated as conversion guards, not fixes: open(w) / write_text already write through a symlink, so symlink survival is an invariant this change must not regress — a naive os.replace conversion would detach the link, which is exactly what atomic_replace prevents (#16743). They are included because that regression is the main hazard of this kind of change, and they pass before and after by design.

The two durability tests break yaml.safe_dump and yaml.dump so the failure is serializer-agnostic — the old code used the former, the helper uses the latter — which keeps them measuring durability rather than the choice of entry point. They use a scoped pytest.MonkeyPatch.context() rather than the monkeypatch fixture so the patch is reverted mid-test without disturbing the session-wide env isolation that shares the function-scoped instance.

Manual check:

1. hermes profile create demo && hermes profile describe demo --description "Curated by hand"
2. printf '' > ~/.hermes/profiles/demo/profile.yaml (simulates the post-crash state)
3. Re-run a metadata write that does not pass a description — on main the description is gone from hermes profile list; the point of this PR is that step 2 can no longer be reached by an interrupted write.

Adjacent suites run green: tests/hermes_cli/test_profile_describer.py, tests/hermes_cli/test_skin_engine.py, tests/hermes_cli/test_skin_palettes.py, tests/cli/test_cli_skin_integration.py, tests/test_cli_skin_integration.py — 102 passed together with the two touched files.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass — ran the focused + adjacent suites listed above (102 passed), not the full suite
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: macOS 15 (Darwin 25.4), Python 3.13

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — or N/A (no user-facing behavior or docstring contract changed; the contract is now actually honored)
• I've updated cli-config.yaml.example if I added/changed config keys — or N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — reasoned but not executed on Windows: atomic_replace is the repo's existing cross-platform primitive and already has EXDEV/EBUSY fallbacks; the two symlink guard tests will skip or fail on a Windows runner without developer mode, so please flag if CI shows that and I'll gate them on os.symlink availability
• I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Related / Positioning

Checked against the open queue two ways, because the two nets are blind in opposite directions:

• By changed path (gh pr list --json files, 800 most recent open PRs, spanning feat(cli): prompt to stop running hermes.exe before Windows update #76714–fix(acp): support per-session reasoning effort for thinking control (#78229) #78295): zero rivals on hermes_cli/skin_cmd.py or tests/hermes_cli/test_skin_cmd.py. Five PRs touch hermes_cli/profiles.py or its test file (test: gate OS-specific tests by real host, add macOS + Windows CI lanes #77992, fix(buzz): latch DM classification on p-tag when channel metadata says DM #77901, fix(doctor): support local Mem0 and quoted profile aliases #77058, Dev/hermes upgrade t 16bbffad #76853, chore: upgrade carried Hermes runtime to v2026.7.30 (#4) #76791) — all hunk-disjoint from write_profile_meta's write.
• By symbol, corpus-wide (gh search prs, all open): write_profile_meta returns exactly one PR — fix(profiles): add allow_unicode=True to write_profile_meta yaml.safe_dump #51808, superseded as described above. skin_cmd / _skin_set return zero. fix(cli): normalize profile description_auto values from profile.yaml #27708 edits the description_auto coercion two lines above this hunk; different bug, no overlap with the write itself.

[Copilot]

Pull request overview

This PR hardens two CLI “read-modify-write” YAML update paths (profile.yaml metadata and user skin files) by routing them through the shared utils.atomic_yaml_write() helper. This prevents interrupted/truncated writes from causing persistent user-visible YAML corruption or silent loss of fields on subsequent writes, while preserving symlinked targets.

Changes:

• Switch hermes_cli/profiles.py:write_profile_meta() from truncating open(..., "w") + yaml.safe_dump to atomic_yaml_write(...).
• Switch hermes_cli/skin_cmd.py:_skin_set() from Path.write_text(yaml.safe_dump(...)) to atomic_yaml_write(...).
• Add regression tests covering durability, UTF-8 emission, and symlink preservation for both paths.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
hermes_cli/profiles.py	Use atomic_yaml_write for profile metadata writes to prevent torn-write data loss and preserve merge semantics.
hermes_cli/skin_cmd.py	Use atomic_yaml_write for skin edits to make writes durable and atomic (including symlink preservation).
tests/hermes_cli/test_profiles.py	Add regression tests ensuring interrupted writes don’t corrupt/erase profile.yaml fields and UTF-8 remains unescaped.
tests/hermes_cli/test_skin_cmd.py	Add tests asserting skin set reaches disk durably and preserves symlinked skin files.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[briandevans]

@copilot Correct, and fixed in commit 0a86a39dc (branch head 0a86a39dc) — comment only, no behavior change.

You are right that _skin_set has no try/except around yaml.safe_load, so invalid YAML raises and aborts the command rather than falling back. I had carried the wording over from write_profile_meta, which does wrap its read in try/except Exception: existing = {} — the two sites genuinely differ here and the comment flattened them.

The {} fallback in _skin_set comes only from safe_load("") returning None, which the or {} on line 52 converts to an empty dict. That is not a narrower case for this PR, though: a zero-length file is exactly what an unsynced, non-atomic write_text leaves behind after a crash or power loss, so the loss chain the change closes is unchanged. A file left containing invalid YAML raises — loud, and not silent data loss. The comment now says that explicitly, including the contrast.

I deliberately did not add fallback-on-parse-error handling, for the reason you name: swallowing a parse error there would be a behavioral change, and a bad one — it would convert a loud abort into the silent palette wipe this PR is trying to eliminate.

Verified by test_set_persists_the_skin_durably in tests/hermes_cli/test_skin_cmd.py:63 (asserts fsync is reached before _skin_set returns, and that background / banner_title survive the write). Fails on unmodified main, passes here. The same correction is now in the PR description.

[kshitijk4poor]

Merged via #79137 — your commits cherry-picked with authorship preserved (rebase merge). Also supersedes #51808 (the allow_unicode=True fix is subsumed since atomic_yaml_write sets it internally).

Thanks for the thorough write-up and sibling-site sweep!