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