mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-26 17:38:36 +00:00
fix(skills): stop treating an upstream skill rename as a user deletion
`sync_skills()` keys the bundled manifest by frontmatter name but computes the destination from the bundled path. When upstream renames or recategorizes a skill, the manifest key still matches while the new dest does not exist yet, so the loop fell into its "in manifest but not on disk" branch and misread the skill as user-deleted: the user's copy was stranded at the old path forever and never received another update. Three skills hit this in the July 2026 reorg (computer-use, evaluating-llms-harness, serving-llms-vllm) — silently frozen at their pre-rename content on every machine that ran `hermes update`. Recovery only moves a stale copy when it is byte-identical to the origin hash recorded the last time sync wrote it, which proves the directory is ours rather than the user's work. User-modified copies are kept in place with a warning, hub-installed paths are never touched, and a genuine deletion (no copy anywhere on disk) is still respected. - tools/skills_sync.py: add _recover_renamed_skill() plus the _index_active_skills() / _read_hub_install_paths() indexes; call it before classification and report moves via a new `relocated` key. - hermes_cli/main.py: surface relocations in both `hermes update` skill sync reporting sites. - tests: 4 cases covering relocate, user-modified preservation, hub-installed exemption, and genuine-deletion respect.
This commit is contained in:
teknium1
Teknium
parent
9de7dfe1cc
commit
c537ae5f40
3 changed files with 273 additions and 0 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue