fix(distribution): preserve skill roots safely

(cherry picked from commit c60656df80956e2a09670d0f7ad63b96d5d82ade)

hermes_cli/profile_distribution.py

@@ -373,6 +373,50 @@ def _copy_dist_payload(
     target.mkdir(parents=True, exist_ok=True)
     staged_resolved = staged.resolve()
 
+    def _remove_existing(path: Path) -> None:
+        """Remove one destination entry without following a destination symlink."""
+        if path.is_symlink() or path.is_file():
+            path.unlink()
+        elif path.is_dir():
+            shutil.rmtree(path)
+
+    def _replace_entry(src: Path, dest: Path, *, ignore=None) -> None:
+        """Replace one distribution entry, handling file/directory changes safely."""
+        dest.parent.mkdir(parents=True, exist_ok=True)
+        _remove_existing(dest)
+        if src.is_dir():
+            shutil.copytree(src, dest, ignore=ignore)
+        else:
+            shutil.copy2(src, dest)
+
+    def _ensure_skill_parent(parts: Tuple[str, ...]) -> Path:
+        """Create a real skills path and never write through a user symlink."""
+        skills_target = target / "skills"
+        if skills_target.is_symlink() or (skills_target.exists() and not skills_target.is_dir()):
+            _remove_existing(skills_target)
+        skills_target.mkdir(parents=True, exist_ok=True)
+        parent = skills_target
+        for part in parts:
+            parent /= part
+            if parent.is_symlink() or (parent.exists() and not parent.is_dir()):
+                _remove_existing(parent)
+            parent.mkdir(parents=True, exist_ok=True)
+        return parent
+
+    def _copy_skill_entry(src: Path, rel_parts: Tuple[str, ...]) -> None:
+        """Replace an explicitly owned skill root or nested skill path."""
+        skill_parts = rel_parts[1:]
+        if not skill_parts:
+            skills_target = _ensure_skill_parent(())
+            if src.is_dir():
+                for child in src.iterdir():
+                    _replace_entry(child, skills_target / child.name)
+            else:
+                _replace_entry(src, skills_target)
+            return
+        dest_parent = _ensure_skill_parent(skill_parts[:-1])
+        _replace_entry(src, dest_parent / skill_parts[-1])
+
     def _ignore_user_owned(d, names):
         # Only the staged root's direct children are filtered.
         return [n for n in names if n in USER_OWNED_EXCLUDE] if Path(d).resolve() == staged_resolved else []
@@ -386,14 +430,15 @@ def _copy_dist_payload(
             if name == "config.yaml" and preserve_config and (target / "config.yaml").exists():
                 continue
         dest = target.joinpath(*rel_parts)
-        dest.parent.mkdir(parents=True, exist_ok=True)
-        if src.is_dir():
-            merge_skills = preserve_skills and rel_parts[0] == "skills"
-            if dest.exists() and not merge_skills:
-                shutil.rmtree(dest)
-            shutil.copytree(src, dest, ignore=_ignore_user_owned, dirs_exist_ok=merge_skills)
+        if preserve_skills and rel_parts[0] == "skills":
+            # A skill directory is the ownership boundary. Replace roots shipped by the
+            # distribution so removed files disappear, while roots absent from the new
+            # payload stay available for user-created skills.
+            _copy_skill_entry(src, rel_parts)
+        elif src.is_dir():
+            _replace_entry(src, dest, ignore=_ignore_user_owned)
         else:
-            shutil.copy2(src, dest)
+            _replace_entry(src, dest)
 
     # Emit .env.EXAMPLE from manifest if the staged tree didn't ship one
     if manifest.env_requires and not (target / ENV_EXAMPLE_FILENAME).exists():
@@ -424,9 +469,16 @@ def install_distribution(
                 "Use `hermes profile update` to upgrade in place, or pass --force to overwrite."
             )
 
-        # Fresh install: config.yaml comes from the distribution.
+        # A forced reinstall still keeps skill roots that are not in the new payload.
+        # config.yaml is the one user-editable distribution file intentionally reset here.
         _bootstrap_user_dirs(plan.target_dir)
-        _copy_dist_payload(plan.staged_dir, plan.target_dir, plan.manifest, preserve_config=False)
+        _copy_dist_payload(
+            plan.staged_dir,
+            plan.target_dir,
+            plan.manifest,
+            preserve_config=False,
+            preserve_skills=plan.existing,
+        )
         if create_alias and check_alias_collision(plan.manifest.name) is None:
             create_wrapper_script(plan.manifest.name)
         return plan
@@ -460,13 +512,8 @@ def update_distribution(profile_name: str, force_config: bool = False) -> Instal
     with tempfile.TemporaryDirectory(prefix="hermes_dist_update_") as tmp:
         plan = plan_install(existing_manifest.source, Path(tmp), override_name=canon)
         plan.preserves_config = not force_config
-        _copy_dist_payload(
-            plan.staged_dir,
-            plan.target_dir,
-            plan.manifest,
-            preserve_config=plan.preserves_config,
-            preserve_skills=True,
-        )
+        _copy_dist_payload(plan.staged_dir, plan.target_dir, plan.manifest,
+                           preserve_config=plan.preserves_config, preserve_skills=True)
         return plan
 
 

tests/hermes_cli/test_profile_distribution.py

@@ -388,6 +388,7 @@ class TestUpdate:
         (staged / "skills" / "demo" / "SKILL.md").write_text("updated demo\n")
         (staged / "skills" / "new").mkdir()
         (staged / "skills" / "new" / "SKILL.md").write_text("new skill\n")
+        (plan.target_dir / "skills" / "demo" / "stale.txt").write_text("old file\n")
 
         update_distribution("skills_safe")
 
@@ -395,6 +396,47 @@ class TestUpdate:
         assert (plan.target_dir / "skills" / "stale" / "SKILL.md").read_text() == "stale skill\n"
         assert (plan.target_dir / "skills" / "demo" / "SKILL.md").read_text() == "updated demo\n"
         assert (plan.target_dir / "skills" / "new" / "SKILL.md").read_text() == "new skill\n"
+        assert not (plan.target_dir / "skills" / "demo" / "stale.txt").exists()
+
+    def test_update_replaces_skill_roots_without_following_target_symlinks(self, profile_env, tmp_path):
+        staged = _make_staging_dir(profile_env, "safe_roots")
+        plan = install_distribution(str(staged), name="safe_roots")
+
+        transition = plan.target_dir / "skills" / "transition"
+        transition.write_text("user file\n")
+        outside = tmp_path / "outside"
+        outside.mkdir()
+        sentinel = outside / "sentinel.txt"
+        sentinel.write_text("keep me\n")
+        linked = plan.target_dir / "skills" / "linked"
+        _symlink_file_or_skip(linked, outside)
+
+        (staged / "skills" / "transition").mkdir()
+        (staged / "skills" / "transition" / "SKILL.md").write_text("new directory\n")
+        (staged / "skills" / "linked").mkdir()
+        (staged / "skills" / "linked" / "SKILL.md").write_text("safe replacement\n")
+
+        update_distribution("safe_roots")
+
+        assert transition.is_dir()
+        assert (transition / "SKILL.md").read_text() == "new directory\n"
+        assert linked.is_dir()
+        assert (linked / "SKILL.md").read_text() == "safe replacement\n"
+        assert sentinel.read_text() == "keep me\n"
+
+    def test_force_install_preserves_unshipped_skill_roots(self, profile_env):
+        staged = _make_staging_dir(profile_env, "force_safe")
+        plan = install_distribution(str(staged), name="force_safe")
+
+        custom = plan.target_dir / "skills" / "user-created"
+        custom.mkdir()
+        (custom / "SKILL.md").write_text("keep this skill\n")
+        (staged / "skills" / "demo" / "SKILL.md").write_text("updated demo\n")
+
+        install_distribution(str(staged), name="force_safe", force=True)
+
+        assert (plan.target_dir / "skills" / "user-created" / "SKILL.md").read_text() == "keep this skill\n"
+        assert (plan.target_dir / "skills" / "demo" / "SKILL.md").read_text() == "updated demo\n"
 
     def test_update_preserves_skills_when_distribution_uses_explicit_allowlist(self, profile_env):
         mf = DistributionManifest(name="skills_allowlist", version="0.1.0", distribution_owned=["skills"])