diff --git a/hermes_cli/profile_distribution.py b/hermes_cli/profile_distribution.py index 37d0921834b08..ce269fad51318 100644 --- a/hermes_cli/profile_distribution.py +++ b/hermes_cli/profile_distribution.py @@ -251,12 +251,6 @@ def _load_yaml(text: str) -> Any: return yaml.safe_load(text) -def _dump_yaml(data: Any) -> str: - import yaml - - return yaml.safe_dump(data, sort_keys=False, default_flow_style=False) - - def read_manifest(profile_dir: Path) -> Optional[DistributionManifest]: """Return the manifest for *profile_dir*, or None if it isn't a distribution.""" mf_path = profile_dir / MANIFEST_FILENAME @@ -271,7 +265,20 @@ def read_manifest(profile_dir: Path) -> Optional[DistributionManifest]: def write_manifest(profile_dir: Path, manifest: DistributionManifest) -> Path: mf_path = profile_dir / MANIFEST_FILENAME - mf_path.write_text(_dump_yaml(manifest.to_dict()), encoding="utf-8") + # Route through the shared atomic YAML writer (temp file + fsync + atomic + # replace, preserving mode/owner and symlinks). A bare write_text() + # truncates distribution.yaml before the dump lands, and read_manifest() + # treats a missing-or-unparseable manifest as "not a distribution" -- so an + # interrupted install/update silently demotes the profile, losing update + # tracking and env_requires with no error surfaced anywhere. + from utils import atomic_yaml_write + + atomic_yaml_write( + mf_path, + manifest.to_dict(), + sort_keys=False, + default_flow_style=False, + ) return mf_path diff --git a/hermes_cli/uninstall.py b/hermes_cli/uninstall.py index 58c7598040f19..2d60eed52020b 100644 --- a/hermes_cli/uninstall.py +++ b/hermes_cli/uninstall.py @@ -8,6 +8,7 @@ Provides options for: import os import shutil +import stat import subprocess import sys from pathlib import Path @@ -87,7 +88,26 @@ def remove_path_from_shell_configs(): new_content = new_content.replace('\n\n\n', '\n\n') if new_content != original_content: - config_path.write_text(new_content, encoding="utf-8") + from utils import atomic_write_text + + # This is the user's own shell rc, not a Hermes-owned file, and + # nothing in this function backs it up. A bare write_text() + # truncates it before the new content lands, so a crash or + # SIGINT mid-write leaves the user with an empty or truncated + # ~/.zshrc -- and the enclosing `except Exception` downgrades + # that to a warning, so the next login just starts a bare + # shell. atomic_replace also resolves a symlinked rc file, so a + # dotfiles-repo setup keeps the symlink instead of having it + # replaced by a regular file. + prior_mode = stat.S_IMODE(config_path.stat().st_mode) + atomic_write_text(config_path, new_content) + # atomic_write_text swaps in a fresh 0600 temp file; shell rc + # files are normally 0644 and removing Hermes' PATH block must + # not quietly change their permissions. + try: + os.chmod(config_path, prior_mode) + except OSError: + pass removed_from.append(config_path) except Exception as e: diff --git a/hermes_cli/web_routers/profiles.py b/hermes_cli/web_routers/profiles.py index b3587ccd9eb1f..4b51973170521 100644 --- a/hermes_cli/web_routers/profiles.py +++ b/hermes_cli/web_routers/profiles.py @@ -14,6 +14,8 @@ late-binding seam in :mod:`hermes_cli.web_deps` so tests that import asyncio # noqa: F401 — used by handlers import logging +import os +import stat import subprocess # noqa: F401 import sys # noqa: F401 import time # noqa: F401 @@ -613,7 +615,28 @@ async def get_profile_soul(name: str): async def update_profile_soul(name: str, body: ProfileSoulUpdate): soul_path = _resolve_profile_dir(name) / "SOUL.md" try: - soul_path.write_text(body.content, encoding="utf-8") + from utils import atomic_write_text + + # PUT replaces the whole persona document from the dashboard editor. + # A bare write_text() truncates SOUL.md before the new body lands, and + # the paired GET above reports an unreadable file as + # ``{"content": "", "exists": False}`` -- so an interrupted save shows + # up as "your persona was never set" and the editor's next Save + # persists that empty document over it. + try: + prior_mode = stat.S_IMODE(soul_path.stat().st_mode) + except OSError: + # First save for this profile -- there is no prior file to match. + prior_mode = None + + atomic_write_text(soul_path, body.content) + # atomic_write_text swaps in a fresh 0600 temp file; profile SOUL.md is + # created 0644 and is not run through _secure_file, so re-apply. + if prior_mode is not None: + try: + os.chmod(soul_path, prior_mode) + except OSError: + pass except OSError as e: _log.exception("PUT /api/profiles/%s/soul failed", name) raise HTTPException(status_code=500, detail=f"Could not write SOUL.md: {e}") diff --git a/hermes_cli/xai_retirement.py b/hermes_cli/xai_retirement.py index 0dc8a45c6f7a9..f3d9b21ad6af3 100644 --- a/hermes_cli/xai_retirement.py +++ b/hermes_cli/xai_retirement.py @@ -137,6 +137,9 @@ def format_issue(issue: RetirementIssue) -> str: # --------------------------------------------------------------------------- import datetime as _dt +import io +import os +import stat from pathlib import Path import shutil @@ -243,10 +246,38 @@ def apply_migration( shutil.copy2(config_path, backup_path) from hermes_cli.config import require_readable_config_before_write + from utils import atomic_write_text require_readable_config_before_write(config_path) - with config_path.open("w", encoding="utf-8") as fh: - yaml.dump(doc, fh) + + # Serialize with the round-trip dumper first, then hand the finished text + # to the shared atomic writer (temp file + fsync + atomic replace). + # ``open(config_path, "w")`` truncates before the dump runs, so a crash or + # SIGINT mid-write leaves config.yaml empty or half-written -- and + # ``--no-backup`` is a documented flag, so on that path the truncated file + # is the only copy left. The load half above returns early when ``doc is + # None``, so the next `hermes migrate xai` reports nothing to migrate + # rather than surfacing the damage. atomic_replace also keeps a symlinked + # config.yaml (dotfiles repo / managed deployment) intact (GitHub #16743). + buf = io.StringIO() + yaml.dump(doc, buf) + + # atomic_write_text swaps in a fresh 0600 temp file, so carry the existing + # permission bits across: _secure_file deliberately leaves config.yaml + # alone under managed (NixOS 0640) and container installs, and a migration + # must not silently tighten what those setups widened. + try: + prior_mode = stat.S_IMODE(config_path.stat().st_mode) + except OSError: + prior_mode = None + + atomic_write_text(config_path, buf.getvalue()) + + if prior_mode is not None: + try: + os.chmod(config_path, prior_mode) + except OSError: + pass return ApplyResult( file_path=config_path, diff --git a/tests/hermes_cli/test_migrate_xai.py b/tests/hermes_cli/test_migrate_xai.py index 3a755381dd9bf..cf30099161e03 100644 --- a/tests/hermes_cli/test_migrate_xai.py +++ b/tests/hermes_cli/test_migrate_xai.py @@ -206,3 +206,89 @@ class TestUnreadableExistingConfig: os.chmod(trap_config, 0o644) assert trap_config.read_bytes() == original + + +# --------------------------------------------------------------------------- +# Crash durability — the rewrite must be atomic +# --------------------------------------------------------------------------- + +class TestCrashDurability: + """apply_migration() rewrites the whole config.yaml in place. + + A bare ``open(path, "w")`` truncates the file *before* the dump runs, so an + interruption (crash, SIGINT, ENOSPC) leaves config.yaml empty or + half-written. Routing through ``utils.atomic_write_text`` means the target + is only ever swapped in via an atomic rename after the temp file is fully + written and fsynced. + """ + + def test_config_survives_an_interrupted_write(self, trap_config: Path): + """A failure mid-write must leave the original config.yaml untouched. + + ``--no-backup`` is a documented flag, so on that path the file being + rewritten is the only copy in existence. + """ + import os + + issues = find_retired_xai_refs(_parse(trap_config)) + assert issues # sanity: trap_config has retired refs + original = trap_config.read_bytes() + + def boom(fd): + raise OSError("simulated crash mid-write") + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(os, "fsync", boom) + with pytest.raises(OSError): + apply_migration(trap_config, issues, backup=False) + + # The original bytes must survive verbatim... + assert trap_config.read_bytes() == original + # ...and the aborted write must not leave a temp file behind. + assert list(trap_config.parent.glob("*.tmp")) == [] + + def test_symlinked_config_is_replaced_in_place(self, tmp_path: Path): + """A config.yaml symlinked into a dotfiles repo must stay a symlink.""" + real_dir = tmp_path / "dotfiles" + real_dir.mkdir() + real = real_dir / "hermes-config.yaml" + real.write_text( + "principal:\n" + " provider: xai\n" + " model: grok-3\n", + encoding="utf-8", + ) + link = tmp_path / "config.yaml" + link.symlink_to(real) + + issues = find_retired_xai_refs(_parse(link)) + assert issues + apply_migration(link, issues, backup=False) + + assert link.is_symlink(), "atomic replace detached the symlink" + assert real.read_text(encoding="utf-8") == link.read_text(encoding="utf-8") + assert "grok-4.3" in real.read_text(encoding="utf-8") + + def test_existing_file_mode_is_preserved(self, trap_config: Path): + """Managed (NixOS 0640) and container installs widen config.yaml + deliberately; the migration must not silently tighten it to 0600.""" + import os + import stat + + os.chmod(trap_config, 0o640) + issues = find_retired_xai_refs(_parse(trap_config)) + apply_migration(trap_config, issues, backup=False) + + mode = stat.S_IMODE(trap_config.stat().st_mode) + assert mode == 0o640, f"mode changed to {oct(mode)}" + + def test_comments_survive_the_atomic_write(self, trap_config: Path): + """The ruamel round-trip must still run — serializing via a string + buffer instead of the file handle must not drop comments.""" + issues = find_retired_xai_refs(_parse(trap_config)) + apply_migration(trap_config, issues, backup=False) + + text = trap_config.read_text(encoding="utf-8") + assert "# Hermes config (sample)" in text + assert "# the main model" in text + assert "# not affected" in text diff --git a/tests/hermes_cli/test_profile_distribution.py b/tests/hermes_cli/test_profile_distribution.py index 9ef8a8bcd718c..dc6a45dfe446f 100644 --- a/tests/hermes_cli/test_profile_distribution.py +++ b/tests/hermes_cli/test_profile_distribution.py @@ -672,3 +672,64 @@ class TestErrorSurfaces: with pytest.raises((ValueError, DistributionError)): plan_install(str(staged), tmp_path / "work") + +# =========================================================================== +# Crash durability: write_manifest rewrites distribution.yaml in place +# =========================================================================== + + +class TestManifestCrashDurability: + """``write_manifest`` runs on every install and update of a shared profile. + + ``read_manifest`` reports a missing-or-unparseable manifest as "this isn't + a distribution", so a truncated distribution.yaml silently demotes the + profile — update tracking and ``env_requires`` just stop existing, with no + error surfaced anywhere. + """ + + def test_previous_manifest_survives_an_interrupted_write(self, tmp_path): + import os + + original = DistributionManifest( + name="keepme", + version="1.0.0", + description="the manifest already on disk", + env_requires=[EnvRequirement(name="FOO", description="foo")], + ) + write_manifest(tmp_path, original) + on_disk = (tmp_path / "distribution.yaml").read_bytes() + + def boom(fd): + raise OSError("simulated crash mid-write") + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(os, "fsync", boom) + with pytest.raises(OSError): + write_manifest( + tmp_path, + DistributionManifest(name="replacement", version="2.0.0"), + ) + + # The old manifest must still be byte-identical and still parse. + assert (tmp_path / "distribution.yaml").read_bytes() == on_disk + parsed = read_manifest(tmp_path) + assert parsed is not None, "profile silently stopped being a distribution" + assert parsed.name == "keepme" + assert parsed.env_requires[0].name == "FOO" + + # No temp file left behind next to the manifest. + assert list(tmp_path.glob("*.tmp")) == [] + + def test_existing_file_mode_is_preserved(self, tmp_path): + import os + import stat + + write_manifest(tmp_path, DistributionManifest(name="modes", version="1.0.0")) + mf = tmp_path / "distribution.yaml" + os.chmod(mf, 0o644) + + write_manifest(tmp_path, DistributionManifest(name="modes", version="2.0.0")) + + mode = stat.S_IMODE(mf.stat().st_mode) + assert mode == 0o644, f"mode changed to {oct(mode)}" + diff --git a/tests/hermes_cli/test_uninstall_shell_configs.py b/tests/hermes_cli/test_uninstall_shell_configs.py new file mode 100644 index 0000000000000..ccb5fc1cce974 --- /dev/null +++ b/tests/hermes_cli/test_uninstall_shell_configs.py @@ -0,0 +1,116 @@ +"""Tests for ``remove_path_from_shell_configs`` — the uninstaller's shell-rc rewrite. + +This rewrites files Hermes does not own (``~/.bashrc``, ``~/.zshrc``, ...) and +takes no backup of them, so the rewrite has to be atomic: a bare +``write_text()`` truncates the rc file before the new content lands, and the +caller wraps everything in ``except Exception: log_warn(...)``, so a partial +write is downgraded to a warning and the user's next login starts a bare shell. +""" +from __future__ import annotations + +import os +import stat +from pathlib import Path + +import pytest + +from hermes_cli import uninstall + + +ZSHRC = ( + "export EDITOR=vim\n" + "alias ll='ls -la'\n" + "\n" + "# Hermes Agent\n" + 'export PATH="$HOME/.local/bin:$PATH"\n' + "\n" + "source ~/.work-profile\n" +) + + +@pytest.fixture +def fake_home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Point both ``Path.home()`` and ``HERMES_HOME`` at a throwaway dir.""" + home = tmp_path / "home" + home.mkdir() + monkeypatch.setattr(Path, "home", classmethod(lambda cls: home)) + monkeypatch.setenv("HERMES_HOME", str(home / ".hermes")) + return home + + +class TestHappyPath: + def test_hermes_path_block_is_removed(self, fake_home: Path): + rc = fake_home / ".zshrc" + rc.write_text(ZSHRC, encoding="utf-8") + + removed = uninstall.remove_path_from_shell_configs() + + assert removed == [rc] + text = rc.read_text(encoding="utf-8") + assert "# Hermes Agent" not in text + # The user's own lines are untouched. + assert "export EDITOR=vim" in text + assert "source ~/.work-profile" in text + + def test_untouched_rc_is_not_reported(self, fake_home: Path): + rc = fake_home / ".zshrc" + rc.write_text("export EDITOR=vim\n", encoding="utf-8") + + assert uninstall.remove_path_from_shell_configs() == [] + assert rc.read_text(encoding="utf-8") == "export EDITOR=vim\n" + + +class TestCrashDurability: + def test_shell_config_survives_an_interrupted_rewrite(self, fake_home: Path): + """An interrupted rewrite must leave the rc file byte-identical. + + There is no backup of the user's shell rc anywhere in this code path, + so a truncated write is unrecoverable. + """ + rc = fake_home / ".zshrc" + rc.write_text(ZSHRC, encoding="utf-8") + original = rc.read_bytes() + + def boom(fd): + raise OSError("simulated crash mid-write") + + # Scoped context so restoring os.fsync doesn't also undo the + # Path.home()/HERMES_HOME patches the fake_home fixture installed. + with pytest.MonkeyPatch.context() as mp: + mp.setattr(os, "fsync", boom) + removed = uninstall.remove_path_from_shell_configs() + + # The write failed, so the rc must not be reported as modified... + assert removed == [] + # ...and it must still be exactly what the user had. + assert rc.read_bytes() == original + # The aborted write must not leave a temp file behind in $HOME. + assert list(fake_home.glob("*.tmp")) == [] + + def test_symlinked_shell_config_stays_a_symlink(self, fake_home: Path): + """A dotfiles-repo ``~/.zshrc`` is a symlink; replacing it with a + regular file silently detaches the user's dotfiles.""" + dotfiles = fake_home / "dotfiles" + dotfiles.mkdir() + real = dotfiles / "zshrc" + real.write_text(ZSHRC, encoding="utf-8") + rc = fake_home / ".zshrc" + rc.symlink_to(real) + + removed = uninstall.remove_path_from_shell_configs() + + assert removed == [rc] + assert rc.is_symlink(), "the symlink was replaced by a regular file" + assert "# Hermes Agent" not in real.read_text(encoding="utf-8") + assert "export EDITOR=vim" in real.read_text(encoding="utf-8") + + def test_existing_file_mode_is_preserved(self, fake_home: Path): + """Shell rc files are normally 0644; uninstalling must not change that.""" + rc = fake_home / ".zshrc" + rc.write_text(ZSHRC, encoding="utf-8") + os.chmod(rc, 0o644) + + uninstall.remove_path_from_shell_configs() + + mode = stat.S_IMODE(rc.stat().st_mode) + assert mode == 0o644, f"mode changed to {oct(mode)}" diff --git a/tests/hermes_cli/test_web_profile_soul_writes.py b/tests/hermes_cli/test_web_profile_soul_writes.py new file mode 100644 index 0000000000000..e912110fc5508 --- /dev/null +++ b/tests/hermes_cli/test_web_profile_soul_writes.py @@ -0,0 +1,110 @@ +"""``PUT /api/profiles/{name}/soul`` must not destroy an existing SOUL.md. + +The dashboard persona editor replaces the whole document on every Save. A bare +``write_text()`` truncates SOUL.md before the new body lands, and the paired +``GET`` reports an unreadable file as ``{"content": "", "exists": False}`` — so +an interrupted save presents as "your persona was never set" and the editor's +next Save persists that empty document over the original. + +Lives in its own module rather than ``test_web_server.py`` to keep the harness +small and focused on this one endpoint pair. +""" + +from __future__ import annotations + +import os +import stat +from pathlib import Path + +import pytest + +fastapi = pytest.importorskip("fastapi") +from fastapi.testclient import TestClient # noqa: E402 + + +SOUL = "# Persona\n\nYou are a careful, terse assistant.\n" + + +@pytest.fixture() +def client(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setenv("HERMES_DASHBOARD_SESSION_TOKEN", "soul-test-token") + from hermes_cli import web_server + + with TestClient(web_server.app, raise_server_exceptions=False) as c: + c.headers["Authorization"] = "Bearer soul-test-token" + yield c + + +@pytest.fixture() +def profile_dir(tmp_path, monkeypatch) -> Path: + """Create a real profile directory under the test HERMES_HOME.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + from hermes_cli import profiles as profiles_mod + + d = profiles_mod.get_profile_dir("demo") + d.mkdir(parents=True, exist_ok=True) + return d + + +class TestSoulWriteDurability: + def test_put_replaces_soul(self, client, profile_dir: Path): + """Happy path: the editor's Save still works.""" + (profile_dir / "SOUL.md").write_text(SOUL, encoding="utf-8") + + r = client.put("/api/profiles/demo/soul", json={"content": "# New\n"}) + + assert r.status_code == 200, r.text + assert (profile_dir / "SOUL.md").read_text(encoding="utf-8") == "# New\n" + + def test_put_creates_soul_when_absent(self, client, profile_dir: Path): + """A first save has no prior file to preserve permissions from.""" + assert not (profile_dir / "SOUL.md").exists() + + r = client.put("/api/profiles/demo/soul", json={"content": SOUL}) + + assert r.status_code == 200, r.text + assert (profile_dir / "SOUL.md").read_text(encoding="utf-8") == SOUL + + def test_existing_soul_survives_an_interrupted_save( + self, client, profile_dir: Path + ): + soul = profile_dir / "SOUL.md" + soul.write_text(SOUL, encoding="utf-8") + original = soul.read_bytes() + + def boom(fd): + raise OSError("simulated crash mid-write") + + # Scoped context so restoring os.fsync doesn't also undo the + # HERMES_HOME patch the client/profile_dir fixtures installed. + with pytest.MonkeyPatch.context() as mp: + mp.setattr(os, "fsync", boom) + r = client.put( + "/api/profiles/demo/soul", json={"content": "# clobbered\n"} + ) + + assert r.status_code == 500 + # The persona the user already had must survive verbatim... + assert soul.read_bytes() == original + # ...and the paired GET must not report it as never-set, which is what + # would make the next Save persist an empty document. + g = client.get("/api/profiles/demo/soul") + assert g.status_code == 200, g.text + assert g.json()["exists"] is True + assert g.json()["content"] == SOUL + # No temp file left behind in the profile directory. + assert list(profile_dir.glob("*.tmp")) == [] + + def test_existing_file_mode_is_preserved(self, client, profile_dir: Path): + """Profile SOUL.md is created 0644 and never run through + ``_secure_file``; saving from the dashboard must not change that.""" + soul = profile_dir / "SOUL.md" + soul.write_text(SOUL, encoding="utf-8") + os.chmod(soul, 0o644) + + r = client.put("/api/profiles/demo/soul", json={"content": "# New\n"}) + + assert r.status_code == 200, r.text + mode = stat.S_IMODE(soul.stat().st_mode) + assert mode == 0o644, f"mode changed to {oct(mode)}"