diff --git a/hermes_cli/main.py b/hermes_cli/main.py index eebb4771d2c2..ff88de141e1a 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -7295,6 +7295,11 @@ def _update_via_zip(args): ) if result.get("cleaned"): print(f" − {len(result['cleaned'])} removed from manifest") + if result.get("relocated"): + print( + f" → {len(result['relocated'])} moved to new upstream paths: " + f"{', '.join(result['relocated'])}" + ) if not result["copied"] and not result.get("updated"): print(" ✓ Skills are up to date") except Exception: @@ -11893,6 +11898,11 @@ def _cmd_update_impl(args, gateway_mode: bool): ) if result.get("cleaned"): print(f" − {len(result['cleaned'])} removed from manifest") + if result.get("relocated"): + print( + f" → {len(result['relocated'])} moved to new upstream paths: " + f"{', '.join(result['relocated'])}" + ) if not result["copied"] and not result.get("updated"): print(" ✓ Skills are up to date") except Exception as e: diff --git a/tests/tools/test_skills_sync.py b/tests/tools/test_skills_sync.py index 42d59d78e1e1..9034e964a74a 100644 --- a/tests/tools/test_skills_sync.py +++ b/tests/tools/test_skills_sync.py @@ -395,6 +395,139 @@ class TestExternalDirsIndexing: assert result["shadowed_by_external"] == [] +class TestRenamedBundledSkillRecovery: + """Upstream renames/recategorizations must not strand the user's copy. + + ``sync_skills()`` keys the manifest by frontmatter *name*, but computes the + destination from the bundled *path*. When upstream moves a skill, the name + still matches while the new dest does not exist yet — the pre-fix code fell + into its "in manifest but not on disk" branch, misread the skill as + user-deleted, and left the old directory stranded at the stale path forever. + """ + + def _patches(self, bundled, skills_dir, manifest_file): + from contextlib import ExitStack + stack = ExitStack() + stack.enter_context(patch("tools.skills_sync._get_bundled_dir", return_value=bundled)) + stack.enter_context( + patch( + "tools.skills_sync._get_optional_dir", + return_value=bundled.parent / "optional-skills", + ) + ) + stack.enter_context(patch("tools.skills_sync.SKILLS_DIR", skills_dir)) + stack.enter_context(patch("tools.skills_sync.MANIFEST_FILE", manifest_file)) + return stack + + def _skill(self, root, rel, body="# Body\n"): + d = root / rel + d.mkdir(parents=True, exist_ok=True) + (d / "SKILL.md").write_text(f"---\nname: moved-skill\n---\n{body}") + return d + + def test_rename_relocates_unmodified_copy(self, tmp_path): + """The stale copy is moved to the new path and updated, not stranded.""" + bundled = tmp_path / "bundled" + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + + # User's copy sits at the OLD path, byte-identical to what sync wrote. + old = self._skill(skills_dir, "oldcat/moved-skill") + origin_hash = _dir_hash(old) + manifest_file.parent.mkdir(parents=True, exist_ok=True) + manifest_file.write_text(f"moved-skill:{origin_hash}\n") + + # Upstream moved it to a NEW category and changed the content. + self._skill(bundled, "newcat/moved-skill", body="# Updated upstream\n") + + with self._patches(bundled, skills_dir, manifest_file): + result = sync_skills(quiet=True) + # Manifest now tracks the current bundled hash (read inside the + # patch context — MANIFEST_FILE is a module global). + recorded = _read_manifest()["moved-skill"] + + new = skills_dir / "newcat" / "moved-skill" + assert new.exists(), "renamed skill was not relocated to the new path" + assert not old.exists(), "stale copy left behind — would shadow forever" + assert "moved-skill" in result["relocated"] + # Having been relocated, it then takes the normal update path. + assert "moved-skill" in result["updated"] + assert "Updated upstream" in (new / "SKILL.md").read_text() + # Future syncs can now detect further upstream changes. + assert recorded == _dir_hash(bundled / "newcat" / "moved-skill") + + def test_rename_preserves_user_modified_copy(self, tmp_path): + """A user-edited copy at the old path is never moved or overwritten.""" + bundled = tmp_path / "bundled" + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + + old = self._skill(skills_dir, "oldcat/moved-skill") + origin_hash = _dir_hash(old) + # User then edits their copy, so it no longer matches the origin hash. + (old / "SKILL.md").write_text("---\nname: moved-skill\n---\n# MY EDITS\n") + manifest_file.parent.mkdir(parents=True, exist_ok=True) + manifest_file.write_text(f"moved-skill:{origin_hash}\n") + + self._skill(bundled, "newcat/moved-skill", body="# Updated upstream\n") + + with self._patches(bundled, skills_dir, manifest_file): + result = sync_skills(quiet=True) + + assert old.exists(), "user's modified copy must not be moved" + assert "MY EDITS" in (old / "SKILL.md").read_text() + assert "moved-skill" not in result.get("relocated", []) + + def test_rename_does_not_move_hub_installed_skill(self, tmp_path): + """A hub-owned path is never relocated — the hub lock owns it.""" + bundled = tmp_path / "bundled" + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + + old = self._skill(skills_dir, "oldcat/moved-skill") + origin_hash = _dir_hash(old) + manifest_file.parent.mkdir(parents=True, exist_ok=True) + manifest_file.write_text(f"moved-skill:{origin_hash}\n") + + lock = skills_dir / ".hub" / "lock.json" + lock.parent.mkdir(parents=True, exist_ok=True) + lock.write_text( + json.dumps( + { + "version": 1, + "installed": { + "moved-skill": {"install_path": "oldcat/moved-skill"} + }, + } + ) + ) + + self._skill(bundled, "newcat/moved-skill", body="# Updated upstream\n") + + with self._patches(bundled, skills_dir, manifest_file): + result = sync_skills(quiet=True) + + assert old.exists(), "hub-installed skill must not be relocated" + assert "moved-skill" not in result.get("relocated", []) + + def test_genuine_user_deletion_still_respected(self, tmp_path): + """No copy anywhere on disk = a real deletion; must not be resurrected.""" + bundled = tmp_path / "bundled" + skills_dir = tmp_path / "user_skills" + skills_dir.mkdir(parents=True, exist_ok=True) + manifest_file = skills_dir / ".bundled_manifest" + manifest_file.write_text("moved-skill:deadbeef\n") + + self._skill(bundled, "newcat/moved-skill") + + with self._patches(bundled, skills_dir, manifest_file): + result = sync_skills(quiet=True) + + assert not (skills_dir / "newcat" / "moved-skill").exists() + assert "moved-skill" not in result["copied"] + assert "moved-skill" not in result.get("relocated", []) + + class TestSyncSkills: def _setup_bundled(self, tmp_path): """Create a fake bundled skills directory.""" diff --git a/tools/skills_sync.py b/tools/skills_sync.py index 731544a046e7..b76449593ff3 100644 --- a/tools/skills_sync.py +++ b/tools/skills_sync.py @@ -495,6 +495,114 @@ def _backfill_optional_provenance(quiet: bool = False) -> List[str]: return backfilled +def _read_hub_install_paths() -> Set[str]: + """Return install paths recorded in the skills-hub lock, as POSIX strings. + + Hub-installed skills are owned by the hub (``hermes skills uninstall``), + never by bundled sync. Rename recovery must not move them even when their + content happens to match a bundled origin hash, or the lock's + ``install_path`` would point at a directory that no longer exists. + """ + lock_path = SKILLS_DIR / ".hub" / "lock.json" + if not lock_path.exists(): + return set() + try: + data = json.loads(lock_path.read_text(encoding="utf-8")) + except (json.JSONDecodeError, OSError): + return set() + paths: Set[str] = set() + for entry in (data.get("installed") or {}).values(): + if isinstance(entry, dict): + install_path = entry.get("install_path") + if install_path: + paths.add(str(install_path).strip("/")) + return paths + + +def _index_active_skills() -> Dict[str, List[Path]]: + """Index every skill in the user's tree by frontmatter name. + + Returns ``{skill_name: [skill_dir, ...]}``. Used by rename recovery to + locate a bundled skill that upstream moved to a new category/directory. + """ + index: Dict[str, List[Path]] = {} + if not SKILLS_DIR.exists(): + return index + for skill_md in SKILLS_DIR.rglob("SKILL.md"): + if is_excluded_skill_path(skill_md): + continue + skill_dir = skill_md.parent + name = _read_skill_name(skill_md, skill_dir.name) + index.setdefault(name, []).append(skill_dir) + return index + + +def _recover_renamed_skill( + skill_name: str, + origin_hash: str, + dest: Path, + active_index: Dict[str, List[Path]], + hub_paths: Set[str], + quiet: bool, +) -> Optional[str]: + """Move a bundled skill's stale copy to its new canonical path. + + When upstream RENAMES or RECATEGORIZES a bundled skill, the manifest key + (frontmatter name) still matches but ``dest`` is a brand-new path that does + not exist yet. Without recovery, ``sync_skills()`` falls through to its + "in manifest but not on disk" branch and misreads the skill as + *user-deleted*: the old directory is stranded forever and never receives + another update. + + A stale copy is only moved when it is byte-identical to ``origin_hash`` — + the hash recorded the last time sync wrote that skill — which proves the + directory is the copy *we* placed there rather than the user's own work. + Anything else (user-edited, hub-installed) is left untouched. + + Returns the relative source path when a move happened, else ``None``. + """ + if not origin_hash: + return None + + for candidate in active_index.get(skill_name, []): + if candidate == dest or not candidate.is_dir(): + continue + try: + rel = candidate.relative_to(SKILLS_DIR).as_posix() + except ValueError: + continue + # Never relocate a hub-installed skill — the hub owns its path. + if rel in hub_paths: + continue + if _dir_hash(candidate) != origin_hash: + # User customized the copy at the old path. Moving it would edit + # their work; leaving it avoids a duplicate-name collision. Warn + # so they can migrate deliberately. + if not quiet: + print( + f" ⚠ {skill_name}: upstream moved this skill to " + f"{dest.relative_to(SKILLS_DIR).as_posix()}, but your " + f"modified copy at {rel} was kept — it will not receive " + f"updates. Run `hermes skills reset {skill_name} --restore` " + f"to move to the new location." + ) + continue + try: + dest.parent.mkdir(parents=True, exist_ok=True) + shutil.move(str(candidate), str(dest)) + except (OSError, IOError): + logger.warning( + "Could not relocate renamed skill %s -> %s", candidate, dest, + exc_info=True, + ) + return None + logger.info("Relocated renamed bundled skill: %s -> %s", candidate, dest) + if not quiet: + print(f" → {skill_name} (moved {rel} → {dest.relative_to(SKILLS_DIR).as_posix()})") + return rel + return None + + def sync_skills(quiet: bool = False) -> dict: """ Sync bundled skills into ~/.hermes/skills/ using the manifest. @@ -533,11 +641,15 @@ def sync_skills(quiet: bool = False) -> dict: # Index of skills already provided by external_dirs (skip writing them) external_index = _build_external_skill_index() shadowed_by_external: List[str] = [] + # Rename recovery indexes, built once per sync (see _recover_renamed_skill). + active_index = _index_active_skills() + hub_paths = _read_hub_install_paths() copied = [] updated = [] user_modified = [] suppressed_skipped: List[str] = [] + relocated: List[str] = [] skipped = 0 for skill_name, skill_src in bundled_skills: @@ -571,6 +683,23 @@ def sync_skills(quiet: bool = False) -> dict: exc_info=True, ) + # Recover an upstream RENAME / RECATEGORIZATION before classifying. + # The manifest key (frontmatter name) survives a directory move, but + # ``dest`` is a new path that does not exist yet — without this the + # "in manifest but not on disk" branch below misreads the skill as + # user-deleted, stranding the old copy at its stale path forever. + if not dest.exists() and skill_name in manifest: + _moved_from = _recover_renamed_skill( + skill_name, + manifest.get(skill_name, ""), + dest, + active_index, + hub_paths, + quiet, + ) + if _moved_from: + relocated.append(skill_name) + if skill_name in external_index: # An external_dirs source already provides this skill. Writing it # into the profile-local tree would create a name collision the @@ -729,6 +858,7 @@ def sync_skills(quiet: bool = False) -> dict: "user_modified": user_modified, "cleaned": cleaned, "suppressed": suppressed_skipped, + "relocated": relocated, "total_bundled": len(bundled_skills), "optional_provenance_backfilled": optional_provenance_backfilled, "shadowed_by_external": shadowed_by_external,