From 25c590ccd0c82440c27189eabb8a2e4bc2e56d48 Mon Sep 17 00:00:00 2001
From: Teknium <127238744+teknium1@users.noreply.github.com>
Date: Thu, 18 Jun 2026 05:47:20 -0700
Subject: [PATCH] fix(skills): refuse SKILLS_DIR root in rmtree guard, not just
 outside-tree
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The salvaged guard allowed _rmtree_writable(SKILLS_DIR) itself. No call
site ever passes the root — every site passes a skill subdir or its .bak
sibling — so allowing the root only preserves the #48200 footgun (a dest
that collapses to the root wipes every installed skill). Require a strict
strict-child relationship and update the test that documented the
nonexistent 'full reset' capability.
---
 tests/tools/test_skills_sync.py | 19 +++++++++++++------
 tools/skills_sync.py            | 11 +++++++++--
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/tests/tools/test_skills_sync.py b/tests/tools/test_skills_sync.py
index 88e8b20940a..fa35f01f2c3 100644
--- a/tests/tools/test_skills_sync.py
+++ b/tests/tools/test_skills_sync.py
@@ -237,16 +237,23 @@ class TestRmtreeWritableScopeGuard:
             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)."""
+    def test_refuses_skills_root_itself(self, tmp_path):
+        """The skills root directory itself must be refused.
+
+        No caller in skills_sync.py ever passes SKILLS_DIR directly — every
+        site passes a skill subdirectory or its ``.bak`` sibling. Removing
+        the root would wipe every installed skill, and a ``dest`` that
+        collapses to the root is exactly the degenerate path #48200 guards
+        against. Require a strict-child relationship.
+        """
         from tools.skills_sync import _rmtree_writable
 
         skills = tmp_path / "skills"
-        skills.mkdir()
+        (skills / "keep").mkdir(parents=True)
         with patch("tools.skills_sync.SKILLS_DIR", skills):
-            # No exception — rmtree is allowed on the root itself.
-            _rmtree_writable(skills)
-        assert not skills.exists()
+            with pytest.raises(ValueError, match="refusing to rmtree"):
+                _rmtree_writable(skills)
+        assert (skills / "keep").exists()  # nothing was wiped
 
     def test_allows_subdirectory_of_skills(self, tmp_path):
         """Any directory strictly under SKILLS_DIR is allowed."""
diff --git a/tools/skills_sync.py b/tools/skills_sync.py
index da3c684c509..a8a7265f95b 100644
--- a/tools/skills_sync.py
+++ b/tools/skills_sync.py
@@ -684,9 +684,16 @@ def _rmtree_writable(path: Path) -> None:
     # 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):
+    # Every legitimate caller passes a skill directory or its ``.bak``
+    # sibling — always a strict child of the skills root. The skills root
+    # itself must never be removed: a ``dest`` that collapses to
+    # ``SKILLS_DIR`` (e.g. a relative path resolving to ``.``) would wipe
+    # every installed skill, and its ``.bak`` sibling lands one level up in
+    # ``HERMES_HOME``. Require a strict-child relationship so both escape
+    # into the skills root and out of it are refused.
+    if skills_root not in target.parents:
         raise ValueError(
-            f"refusing to rmtree {target!r}: not under {skills_root!r} "
+            f"refusing to rmtree {target!r}: not strictly under {skills_root!r} "
             f"(scope guard — see #48200)"
         )
     import stat