From 0cf900ea172d84fc0aef3afbc1de2bd787cc2043 Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Thu, 24 Sep 2026 23:45:27 -0500 Subject: [PATCH] fix(skills): keep built-in provenance for skills the catalog dropped Co-authored-by: fangliquanflq --- hermes_cli/web_routers/skills.py | 4 +-- tests/tools/test_skills_sync.py | 46 ++++++++++++++++++++++++++++++++ tools/skill_usage.py | 12 +++++---- tools/skills_sync.py | 15 ++++++++--- tui_gateway/server.py | 4 +-- 5 files changed, 69 insertions(+), 12 deletions(-) diff --git a/hermes_cli/web_routers/skills.py b/hermes_cli/web_routers/skills.py index c92a1dd9cb..648dd6e61b 100644 --- a/hermes_cli/web_routers/skills.py +++ b/hermes_cli/web_routers/skills.py @@ -345,7 +345,7 @@ async def get_skills(profile: Optional[str] = None): from tools.skills_tool import _find_all_skills from hermes_cli.skills_config import get_disabled_skills from tools.skill_usage import ( - _read_bundled_manifest_names, _read_hub_installed_names, activity_count, load_usage) + _read_bundled_names, _read_hub_installed_names, activity_count, load_usage) def _run(): with _profile_scope(profile): @@ -357,7 +357,7 @@ async def get_skills(profile: Optional[str] = None): # without a per-skill manifest read): hub > bundled > agent, where # "agent" covers agent-authored AND local hand-made skills — the ones # the user may edit/delete from the UI. - bundled_names = _read_bundled_manifest_names() + bundled_names = _read_bundled_names() hub_names = _read_hub_installed_names() for s in skills: s["enabled"] = s["name"] not in disabled diff --git a/tests/tools/test_skills_sync.py b/tests/tools/test_skills_sync.py index 84dc88c598..b22575c748 100644 --- a/tests/tools/test_skills_sync.py +++ b/tests/tools/test_skills_sync.py @@ -513,6 +513,52 @@ class TestSyncSkills: assert (skills_dir / "category" / "new-skill" / "SKILL.md").exists() +class TestDroppedBuiltinProvenance: + """#95415: a built-in the catalog dropped keeps built-in provenance while any copy of it is left; + otherwise /api/skills badges it "Learned" and the Desktop offers edit/archive for it.""" + + def _bundled(self, tmp_path, *names): + bundled = tmp_path / "bundled_skills" + shutil.rmtree(bundled, ignore_errors=True) + for name in names: + (bundled / "cat" / name).mkdir(parents=True) + (bundled / "cat" / name / "SKILL.md").write_text(f"---\nname: {name}\n---\n# {name}\n") + return bundled + + def _sync(self, bundled): + with patch("tools.skills_sync._get_bundled_dir", return_value=bundled), \ + patch("tools.skills_sync._get_optional_dir", return_value=bundled.parent / "optional-skills"): + return sync_skills(quiet=True) + + def test_catalog_drop_keeps_builtin_provenance_while_a_copy_exists(self, tmp_path): + from hermes_constants import get_hermes_home + from tools.skill_usage import provenance + + skills = get_hermes_home() / "skills" + self._sync(self._bundled(tmp_path, "kept", "dropped", "dropped-archived", "dropped-deleted")) + (skills / ".archive").mkdir() + shutil.move(str(skills / "cat" / "dropped-archived"), str(skills / ".archive" / "dropped-archived")) + shutil.rmtree(skills / "cat" / "dropped-deleted") + + result = self._sync(self._bundled(tmp_path, "kept")) + + assert result["cleaned"] == ["dropped-deleted"] + assert {"dropped", "dropped-archived"} <= set(_read_manifest()) + assert provenance("dropped") == "bundled" and provenance("dropped-archived") == "bundled" + + def test_suppressed_builtin_is_not_agent_authored_after_manifest_cleanup(self, tmp_path): + """Profiles an older sync already cleaned: the curator suppression list only records built-ins.""" + from hermes_constants import get_hermes_home + from tools.skill_usage import provenance + + skills = get_hermes_home() / "skills" + (skills / "resurrected").mkdir(parents=True) + (skills / "resurrected" / "SKILL.md").write_text("---\nname: resurrected\n---\n") + (skills / ".curator_suppressed").write_text("resurrected\n") + + assert provenance("resurrected") == "bundled" + + class TestGetBundledDir: def test_env_var_override_with_default_fallback(self, tmp_path, monkeypatch): custom_dir = tmp_path / "custom_skills" diff --git a/tools/skill_usage.py b/tools/skill_usage.py index 9632c77c4b..f0147ad242 100644 --- a/tools/skill_usage.py +++ b/tools/skill_usage.py @@ -143,10 +143,12 @@ def activity_count(record: Dict[str, Any]) -> int: # --- Provenance — which skills are agent-created (and thus eligible for curation) --- -def _read_bundled_manifest_names() -> Set[str]: - """Names from ``.bundled_manifest`` ("name:hash" per line); empty if missing/unreadable.""" +def _read_bundled_names() -> Set[str]: + """Built-in names: ``.bundled_manifest`` ("name:hash" per line) plus the curator suppression list, which + only ever records built-ins; a pruned built-in whose manifest entry an older sync cleaned after the + catalog dropped it is still not agent-authored (#95415). Empty if both are missing/unreadable.""" lines = _read_lines(_skills_dir() / ".bundled_manifest", "Failed to read bundled manifest: %s") - return {n for n in (line.split(":", 1)[0].strip() for line in lines) if n} + return {n for n in (line.split(":", 1)[0].strip() for line in lines) if n} | read_suppressed_names() def _read_hub_installed_names() -> Set[str]: @@ -221,7 +223,7 @@ def _scan_local_skills(keep: Callable[[str, Path, Set[str], Dict[str, Any]], boo """Sorted local skill names passing *keep(name, skill_md, bundled, usage)*; hub/protected names never reach it.""" if not (base := _skills_dir()).exists(): return [] - hub, bundled, usage = _read_hub_installed_names(), _read_bundled_manifest_names(), load_usage() + hub, bundled, usage = _read_hub_installed_names(), _read_bundled_names(), load_usage() return sorted({name for name, skill_md in _iter_skill_mds(base, local_only=True) if name not in hub and not is_protected_builtin(name) and keep(name, skill_md, bundled, usage)}) @@ -265,7 +267,7 @@ def is_hub_installed(skill_name: str) -> bool: def is_bundled(skill_name: str) -> bool: - return skill_name in _read_bundled_manifest_names() + return skill_name in _read_bundled_names() def _external_read_only_message(skill_name: str) -> str: diff --git a/tools/skills_sync.py b/tools/skills_sync.py index 68c8bfa3a6..d3e0c74318 100644 --- a/tools/skills_sync.py +++ b/tools/skills_sync.py @@ -3,7 +3,8 @@ ~/.hermes/skills/, tracking each synced skill's origin hash in .bundled_manifest (v2 "name:hash" lines; v1 plain names auto-migrate). NEW skills are copied and recorded; EXISTING skills update only when bundled changed AND the user copy still matches the origin hash (else user-customized --> SKIP); user-DELETED skills are not re-added; upstream-REMOVED ones leave the manifest.""" +-> SKIP); user-DELETED skills are not re-added; upstream-REMOVED ones leave the manifest once no active or +archived copy remains (the entry is that copy's only built-in provenance record).""" import hashlib import logging @@ -430,9 +431,17 @@ def sync_skills(quiet: bool = False) -> dict: _update_existing_skill(st, skill_name, skill_src, dest, bundled_hash) else: st.skipped += 1 # in manifest but not on disk — user deleted it - # Clean manifest entries for skills removed upstream. Skipped when opted out: bundled_skills + # Clean manifest entries for skills removed upstream once no copy is left. A dropped built-in still + # on disk (active or archived) keeps its entry: it is the only provenance record, and without it the + # copy reads as agent-authored ("Learned", editable) (#95415). Skipped when opted out: bundled_skills # is only the essential set there, so cleaning would drop tracking for everything else. - cleaned = [] if essential_only else sorted(set(st.manifest) - {name for name, _ in bundled_skills}) + removed = [] if essential_only else sorted(set(st.manifest) - {name for name, _ in bundled_skills}) + present = set() + if removed: # curator archive is flat: directory name == skill name + archive = _skills_dir() / ".archive" + present = {_read_skill_name(md, md.parent.name) for md in _iter_active_skill_mds()} | ( + {p.name for p in archive.iterdir() if p.is_dir()} if archive.is_dir() else set()) + cleaned = [name for name in removed if name not in present] for name in cleaned: del st.manifest[name] _seed_category_descriptions( diff --git a/tui_gateway/server.py b/tui_gateway/server.py index a42092deb4..e5f5b62b8b 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -3330,8 +3330,8 @@ def _skill_usage_lookup(): "hub" / "bundled" / "local" (``/api/skills`` ``provenance``, "local" spelled "agent"). Failure → 0 / "local".""" try: from tools.skill_usage import ( - _read_bundled_manifest_names, _read_hub_installed_names, activity_count, load_usage) - records, bundled, hub = load_usage(), _read_bundled_manifest_names(), _read_hub_installed_names() + _read_bundled_names, _read_hub_installed_names, activity_count, load_usage) + records, bundled, hub = load_usage(), _read_bundled_names(), _read_hub_installed_names() except Exception as e: logger.debug("skill usage lookup unavailable: %s", e) return (lambda _name: 0), (lambda _name: "local")