fix(skills): rmtree scope guard + default pre_update_backup to true (#48200)

Defense-in-depth fix for the silent wipe of ~/.hermes/ documented in
#48200. A `hermes update --yes` run silently destroyed a user's
.env, MEMORY.md, kanban.db, custom skills, and scripts. Two changes:

1. `_rmtree_writable` in tools/skills_sync.py now refuses to rmtree
   anything outside SKILLS_DIR (the HERMES_HOME/skills/ root).
   All five call sites pass paths under SKILLS_DIR, so the guard is
   a no-op for current code and a loud, recoverable failure for
   any future regression (bad path join, malicious bundled
   manifest, stale path in scope after an exception).

2. The default `updates.pre_update_backup` flips from false to
   true in hermes_cli/config.py. A few minutes of zip per update
   is negligible compared to silent total data loss. Still
   overridable; --no-backup still works for one-off opt-out.

Five new tests in TestRmtreeWritableScopeGuard (root path,
hermes home, sibling dir, skills root itself, subdir) plus a
flipped `test_default_enabled_creates_backup` in test_backup.py.
178/178 tests pass in the two affected files. Public method
signatures unchanged, no test-stub blast radius.

Closes #48200

hermes_cli/config.py

@@ -2496,11 +2496,14 @@ DEFAULT_CONFIG = {
     "updates": {
         # Run a full ``hermes backup``-style zip of HERMES_HOME before every
         # ``hermes update``.  Backups land in ``<HERMES_HOME>/backups/`` and
-        # can be restored with ``hermes import <path>``.  Off by default —
+        # can be restored with ``hermes import <path>``.  Defaults to true
-        # on large HERMES_HOME directories the zip can add minutes to every
+        # after the #48200 incident: a ``hermes update --yes`` run that
-        # update.  Set to true to re-enable, or pass ``--backup`` to opt in
+        # computed a wrong path silently wiped the user's ``.env``,
-        # for a single update run.
+        # ``MEMORY.md``, ``kanban.db``, custom skills, and scripts in one
-        "pre_update_backup": False,
+        # go.  The cost of a few minutes of zip time per update is
+        # negligible compared to the alternative.  Set to false to opt
+        # out, or pass ``--no-backup`` for a single update run.
+        "pre_update_backup": True,
         # How many pre-update backup zips to retain.  Older ones are pruned
         # automatically after each successful backup.  Values below 1 are
         # floored to 1 — the backup just created is always preserved.  To

hermes_cli/main.py

@@ -8118,7 +8118,13 @@ def _run_pre_update_backup(args) -> None:
         cfg = {}
 
     updates_cfg = cfg.get("updates", {}) if isinstance(cfg, dict) else {}
-    enabled = updates_cfg.get("pre_update_backup", False)
+    # The default config ships with ``pre_update_backup: true`` (see
+    # ``hermes_cli/config.py``). Fall back to true if the key is missing
+    # (e.g. a user has an older custom config without the field). The
+    # ``False`` default from before #48200 caused silent data loss when
+    # an update step computed a wrong path — the cost of a few minutes
+    # of zip time per update is negligible compared to the alternative.
+    enabled = updates_cfg.get("pre_update_backup", True)
     keep = updates_cfg.get("backup_keep", 5)
 
     if not enabled and not force_backup:

tests/hermes_cli/test_backup.py

@@ -1726,16 +1726,21 @@ class TestRunPreUpdateBackup:
         backups = list((hermes_home / "backups").glob("pre-update-*.zip"))
         assert len(backups) == 1
 
-    def test_default_disabled_is_silent(self, hermes_home, capsys):
+    def test_default_enabled_creates_backup(self, hermes_home, capsys):
-        """With the default-off config and no --backup flag, the hook is silent
+        """With the new safe default (``pre_update_backup: true``), every
-        and creates no backup.  This is the common case for every update."""
+        ``hermes update`` creates a backup before any destructive step
+        runs — the cost is a few minutes of zip time vs. the alternative
+        of silent total data loss of ``~/.hermes/`` observed in #48200
+        when an update step computes a wrong path and the user had no
+        safety net.
+        """
         from hermes_cli.main import _run_pre_update_backup
         _run_pre_update_backup(Namespace(no_backup=False, backup=False))
         out = capsys.readouterr().out
-        assert out == ""
+        assert "Creating pre-update backup" in out
-        assert not (hermes_home / "backups").exists() or not list(
+        assert "Saved:" in out
-            (hermes_home / "backups").glob("pre-update-*.zip")
+        backups = list((hermes_home / "backups").glob("pre-update-*.zip"))
-        )
+        assert len(backups) == 1
 
     def test_no_backup_flag_skips(self, hermes_home, capsys):
         from hermes_cli.main import _run_pre_update_backup

tests/tools/test_skills_sync.py

@@ -2,6 +2,7 @@
 
 import shutil
 import json
+import pytest
 from pathlib import Path
 from unittest.mock import patch
 
@@ -180,6 +181,90 @@ class TestComputeRelativeDest:
         assert dest.name == "simple"
 
 
+class TestRmtreeWritableScopeGuard:
+    """``_rmtree_writable`` must refuse to remove anything outside
+    ``HERMES_HOME/skills/``.
+
+    The previous implementation called ``shutil.rmtree(path)`` on whatever
+    argument the caller passed. If any of the five call sites in
+    ``tools/skills_sync.py`` ever computes a path outside the skills
+    root — through a bad join, a missing default, a malicious
+    bundled-manifest entry, or a stale path in scope after an
+    exception — the result is a silent ``shutil.rmtree(~/.hermes/)``
+    that destroys the user's ``.env``, ``MEMORY.md``, ``kanban.db``,
+    custom skills, scripts, and the rest of the install in one go
+    (#48200).
+
+    The scope guard turns that into a loud ``ValueError`` so the
+    failure is observable, reproducible, and recoverable rather than
+    a data-loss incident.
+    """
+
+    def test_refuses_root_path(self, tmp_path):
+        """``Path('/')`` is the entire filesystem — must always be rejected."""
+        from tools.skills_sync import _rmtree_writable, SKILLS_DIR
+
+        skills = tmp_path / "skills"
+        skills.mkdir()
+        with patch("tools.skills_sync.SKILLS_DIR", skills):
+            with pytest.raises(ValueError, match="refusing to rmtree"):
+                _rmtree_writable(Path("/"))
+
+    def test_refuses_hermes_home_itself(self, tmp_path):
+        """``~/.hermes/`` itself is what the #48200 wipe destroyed."""
+        from tools.skills_sync import _rmtree_writable
+
+        hermes = tmp_path / "home"
+        hermes.mkdir()
+        (hermes / "skills").mkdir()
+        with patch("tools.skills_sync.SKILLS_DIR", hermes / "skills"):
+            with pytest.raises(ValueError, match="refusing to rmtree"):
+                _rmtree_writable(hermes)
+
+    def test_refuses_sibling_directory(self, tmp_path):
+        """A directory that is a sibling of SKILLS_DIR (e.g. a wrong
+        ``bundled_dir`` computation) must be rejected, not silently rmtree'd.
+        """
+        from tools.skills_sync import _rmtree_writable
+
+        hermes = tmp_path / "home"
+        hermes.mkdir()
+        skills = hermes / "skills"
+        skills.mkdir()
+        not_skills = hermes / "kanban.db"  # any non-skills path
+        not_skills.mkdir()
+        with patch("tools.skills_sync.SKILLS_DIR", skills):
+            with pytest.raises(ValueError, match="refusing to rmtree"):
+                _rmtree_writable(not_skills)
+
+    def test_allows_skills_root_itself(self, tmp_path):
+        """The skills root directory IS allowed (used for full reset)."""
+        from tools.skills_sync import _rmtree_writable
+
+        skills = tmp_path / "skills"
+        skills.mkdir()
+        with patch("tools.skills_sync.SKILLS_DIR", skills):
+            # No exception — rmtree is allowed on the root itself.
+            _rmtree_writable(skills)
+        assert not skills.exists()
+
+    def test_allows_subdirectory_of_skills(self, tmp_path):
+        """Any directory strictly under SKILLS_DIR is allowed."""
+        from tools.skills_sync import _rmtree_writable
+
+        skills = tmp_path / "skills"
+        skills.mkdir()
+        sub = skills / "category" / "old-skill"
+        sub.mkdir(parents=True)
+        (sub / "SKILL.md").write_text("# old")
+
+        with patch("tools.skills_sync.SKILLS_DIR", skills):
+            _rmtree_writable(sub)
+
+        assert skills.exists()
+        assert not sub.exists()
+
+
 class TestSyncSkills:
     def _setup_bundled(self, tmp_path):
         """Create a fake bundled skills directory."""

tools/skills_sync.py

@@ -671,6 +671,24 @@ def _rmtree_writable(path: Path) -> None:
     parent directory, so the retry handler makes the failing path **and its
     parent** writable before re-attempting.  See #34860, #34972.
     """
+    # Defense in depth (#48200): refuse to rmtree anything outside
+    # ``HERMES_HOME/skills/`` to prevent the catastrophic wipe of
+    # ``~/.hermes/`` (``.env``, ``MEMORY.md``, ``kanban.db``, custom
+    # skills, scripts, …) that an earlier incident observed. Five call
+    # sites in this file invoke this helper; if any one of them ever
+    # computes a destination outside the skills root — through a bad
+    # path join, a missing ``HERMES_HOME`` default, a malicious
+    # bundled-manifest entry, or a mid-flight exception that leaves a
+    # stale path in scope — this guard turns the resulting
+    # ``shutil.rmtree(~/.hermes)`` into a loud, recoverable ``ValueError``
+    # instead of silently destroying the user's install.
+    target = Path(path).resolve()
+    skills_root = SKILLS_DIR.resolve()
+    if not (target == skills_root or skills_root in target.parents):
+        raise ValueError(
+            f"refusing to rmtree {target!r}: not under {skills_root!r} "
+            f"(scope guard — see #48200)"
+        )
     import stat
 
     def _on_error(func, fpath, exc_info):