diff --git a/hermes_cli/profile_distribution.py b/hermes_cli/profile_distribution.py index eaf76cd73b..97b227aa43 100644 --- a/hermes_cli/profile_distribution.py +++ b/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 diff --git a/tests/hermes_cli/test_profile_distribution.py b/tests/hermes_cli/test_profile_distribution.py index cf7ec7fd5c..8d99da68de 100644 --- a/tests/hermes_cli/test_profile_distribution.py +++ b/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"])