From bb2dfa37be12d0d11274a164881bdddaf7f18128 Mon Sep 17 00:00:00 2001 From: Matt Date: Sun, 26 Jul 2026 14:57:27 +0800 Subject: [PATCH] fix: use recursive SKILL.md discovery for all skill roots _skill_md_paths previously used a flat glob (*/SKILL.md) for non-Hermes directories, only enabling recursive (**/SKILL.md) for ~/.hermes/skills. This caused Hermes profile directories (~/.hermes/profiles/*/skills/) to miss 88 of 91 skills. Changes: - skill_manager.py: always use recursive glob in _skill_md_paths - skill_manager.py: extend _is_hermes_skill_root to match profile dirs - skill_hub.py: extend _is_hermes_skill_root to match profile dirs - nacos_skill_hub.py: always use recursive glob in skill list - tests: add 9 regression tests covering default root, profile root, depth-3, flat layout, unrelated dirs, symlink dedup, and real-world detection of all 91 production skills All 9 new tests pass. Existing suite: 108 pass, 17 fail (all pre-existing, missing pytest-asyncio or unrelated dashboard KeyError). Fixes #? --- skillclaw/nacos_skill_hub.py | 2 +- skillclaw/skill_hub.py | 22 +++- skillclaw/skill_manager.py | 31 +++-- tests/test_recursive_skill_discovery.py | 147 ++++++++++++++++++++++++ 4 files changed, 192 insertions(+), 10 deletions(-) create mode 100644 tests/test_recursive_skill_discovery.py diff --git a/skillclaw/nacos_skill_hub.py b/skillclaw/nacos_skill_hub.py index 01299ed..acc61ba 100644 --- a/skillclaw/nacos_skill_hub.py +++ b/skillclaw/nacos_skill_hub.py @@ -511,7 +511,7 @@ def pull_skills( } local_dirs_by_name = {} - for skill_md in Path(skills_dir).glob("**/SKILL.md" if _is_hermes_skill_root(skills_dir) else "*/SKILL.md"): + for skill_md in Path(skills_dir).glob("**/SKILL.md"): local_dirs_by_name.setdefault(skill_md.parent.name, []).append(str(skill_md.parent)) downloaded = skipped = deleted = failed = 0 diff --git a/skillclaw/skill_hub.py b/skillclaw/skill_hub.py index 3bd16d7..086baa8 100644 --- a/skillclaw/skill_hub.py +++ b/skillclaw/skill_hub.py @@ -40,7 +40,27 @@ def _is_hermes_skill_root(skills_dir: str) -> bool: - return os.path.realpath(skills_dir) == os.path.realpath(os.path.join(os.path.expanduser("~"), ".hermes", "skills")) + skills = os.path.realpath(skills_dir) + # Match any .hermes/skills or .hermes/profiles/*/skills path + parts = skills.split(os.sep) + try: + hermes_idx = parts.index(".hermes") + except ValueError: + return False + if hermes_idx + 2 > len(parts): + return False + # .hermes/skills + if parts[hermes_idx + 1] == "skills" and hermes_idx + 2 == len(parts): + return True + # .hermes/profiles//skills + if ( + hermes_idx + 4 <= len(parts) + and parts[hermes_idx + 1] == "profiles" + and parts[hermes_idx + 3] == "skills" + and hermes_idx + 4 == len(parts) + ): + return True + return False def _skill_dir_for_root(skills_dir: str, skill_name: str, category: str = "general") -> str: diff --git a/skillclaw/skill_manager.py b/skillclaw/skill_manager.py index 55547a2..cc544c0 100644 --- a/skillclaw/skill_manager.py +++ b/skillclaw/skill_manager.py @@ -309,11 +309,8 @@ def _load_skills(self) -> Dict[str, Any]: return result def _skill_md_paths(self) -> list[str]: - if self._is_hermes_skill_root(): - pattern = os.path.join(self._skills_dir, "**", "SKILL.md") - return sorted(glob.glob(pattern, recursive=True)) - pattern = os.path.join(self._skills_dir, "*", "SKILL.md") - return sorted(glob.glob(pattern)) + pattern = os.path.join(self._skills_dir, "**", "SKILL.md") + return sorted(glob.glob(pattern, recursive=True)) def _compute_skills_fingerprint(self) -> tuple[tuple[str, int, int], ...]: fingerprint: list[tuple[str, int, int]] = [] @@ -332,9 +329,27 @@ def _compute_skills_fingerprint(self) -> tuple[tuple[str, int, int], ...]: return tuple(fingerprint) def _is_hermes_skill_root(self) -> bool: - return os.path.realpath(self._skills_dir) == os.path.realpath( - os.path.join(os.path.expanduser("~"), ".hermes", "skills") - ) + skills = os.path.realpath(self._skills_dir) + # Match any .hermes/skills or .hermes/profiles/*/skills path + parts = skills.split(os.sep) + try: + hermes_idx = parts.index(".hermes") + except ValueError: + return False + if hermes_idx + 2 > len(parts): + return False + # .hermes/skills + if parts[hermes_idx + 1] == "skills" and hermes_idx + 2 == len(parts): + return True + # .hermes/profiles//skills + if ( + hermes_idx + 4 <= len(parts) + and parts[hermes_idx + 1] == "profiles" + and parts[hermes_idx + 3] == "skills" + and hermes_idx + 4 == len(parts) + ): + return True + return False def _skill_dir_path(self, skill: dict) -> str: name = str(skill.get("name", "unknown") or "unknown").strip() diff --git a/tests/test_recursive_skill_discovery.py b/tests/test_recursive_skill_discovery.py new file mode 100644 index 0000000..cf83097 --- /dev/null +++ b/tests/test_recursive_skill_discovery.py @@ -0,0 +1,147 @@ +""" +Regression tests for recursive SKILL.md discovery. + +Covers: + - ~/.hermes/skills/category/skill/SKILL.md + - ~/.hermes/profiles/lightning-node/skills/category/skill/SKILL.md + - depth-3 skills + - flat skills + - unrelated directories excluded + - symlink duplicates not double-counted + - real-world: all 91 production skills detected +""" + +import os +import shutil +import tempfile +from pathlib import Path + +import pytest + +from skillclaw.skill_manager import SkillManager + + +def _write_skill(skill_dir: str, name: str, description: str, category: str = "general") -> str: + """Write a minimal SKILL.md and return its path.""" + skill_path = os.path.join(skill_dir, name) + os.makedirs(skill_path, exist_ok=True) + md_path = os.path.join(skill_path, "SKILL.md") + with open(md_path, "w") as f: + f.write(f"---\nname: {name}\ndescription: \"{description}\"\n") + if category != "general": + f.write(f"category: {category}\n") + f.write("---\n\n# {name}\n") + return md_path + + +class TestRecursiveSkillDiscovery: + """Prove that _skill_md_paths finds all skills regardless of directory layout.""" + + def test_hermes_default_skills_root(self): + """~/.hermes/skills/category/skill/SKILL.md is found.""" + with tempfile.TemporaryDirectory() as td: + hermes_skills = os.path.join(td, ".hermes", "skills") + _write_skill(hermes_skills, "my-skill", "A test skill", category="coding") + sm = SkillManager(hermes_skills) + names = {s["name"] for s in sm.skills["all_skills"]} + assert "my-skill" in names + + def test_hermes_profile_skills_root(self): + """~/.hermes/profiles/lightning-node/skills/category/skill/SKILL.md is found.""" + with tempfile.TemporaryDirectory() as td: + profile_skills = os.path.join(td, ".hermes", "profiles", "lightning-node", "skills") + _write_skill(profile_skills, "lightning-ops", "Lightning node operations", category="infrastructure") + sm = SkillManager(profile_skills) + names = {s["name"] for s in sm.skills["all_skills"]} + assert "lightning-ops" in names + # Verify _is_hermes_skill_root recognises profile dirs + assert sm._is_hermes_skill_root() + + def test_depth_3_skills_found(self): + """depth-3 skills (category/subcategory/skill/SKILL.md) are found.""" + with tempfile.TemporaryDirectory() as td: + skills_dir = os.path.join(td, "skills") + deep_path = os.path.join(skills_dir, "mlops", "evaluation", "my-eval-skill") + os.makedirs(deep_path, exist_ok=True) + with open(os.path.join(deep_path, "SKILL.md"), "w") as f: + f.write("---\nname: my-eval-skill\ndescription: \"Eval skill at depth 3\"\n---\n\n# Eval Skill\n") + sm = SkillManager(skills_dir) + names = {s["name"] for s in sm.skills["all_skills"]} + assert "my-eval-skill" in names + + def test_flat_skills_remain_discoverable(self): + """Flat skills (skills_dir/skill/SKILL.md) are still found.""" + with tempfile.TemporaryDirectory() as td: + skills_dir = os.path.join(td, "skills") + _write_skill(skills_dir, "flat-skill", "A flat skill") + sm = SkillManager(skills_dir) + names = {s["name"] for s in sm.skills["all_skills"]} + assert "flat-skill" in names + + def test_unrelated_dirs_excluded(self): + """Unrelated directories outside the configured skill root are excluded.""" + with tempfile.TemporaryDirectory() as td: + skills_dir = os.path.join(td, "skills") + unrelated = os.path.join(td, "not-skills", "intruder") + os.makedirs(unrelated, exist_ok=True) + with open(os.path.join(unrelated, "SKILL.md"), "w") as f: + f.write("---\nname: intruder\ndescription: \"Should not be found\"\n---\n\n# Intruder\n") + _write_skill(skills_dir, "real-skill", "A real skill") + sm = SkillManager(skills_dir) + names = {s["name"] for s in sm.skills["all_skills"]} + assert "real-skill" in names + assert "intruder" not in names + + def test_symlink_duplicates_not_double_counted(self): + """Symlink duplicates are not returned twice (realpath dedup).""" + with tempfile.TemporaryDirectory() as td: + skills_dir = os.path.join(td, "skills") + os.makedirs(skills_dir, exist_ok=True) + real_skill_dir = os.path.join(td, "real-skills", "linked-skill") + os.makedirs(real_skill_dir, exist_ok=True) + with open(os.path.join(real_skill_dir, "SKILL.md"), "w") as f: + f.write("---\nname: linked-skill\ndescription: \"Linked skill\"\n---\n\n# Linked\n") + # Create symlink inside skills_dir + os.symlink(real_skill_dir, os.path.join(skills_dir, "linked-skill")) + sm = SkillManager(skills_dir) + linked = [s for s in sm.skills["all_skills"] if s["name"] == "linked-skill"] + assert len(linked) == 1, f"Expected 1, got {len(linked)}" + + def test_real_world_91_skills_detected(self): + """All 91 current skills are detected in the user's production profile.""" + skills_dir = os.path.expanduser("~/.hermes/profiles/lightning-node/skills") + if not os.path.isdir(skills_dir): + pytest.skip("Production skills directory not available") + sm = SkillManager(skills_dir) + count = len(sm.skills["all_skills"]) + assert count >= 91, f"Expected at least 91 skills, found {count}" + # Verify key Lightning skills are present + names = {s["name"] for s in sm.skills["all_skills"]} + expected = { + "lightning-node-ops", + "core-lightning-ops", + "lndg-remote-db-ops", + "loopd-cost-reconciliation", + "intervention-ledger-reconciliation", + "hermes-agent", + "obsidian", + "github-workflow", + } + missing = expected - names + assert not missing, f"Missing critical skills: {missing}" + + def test_is_hermes_skill_root_profile(self): + """_is_hermes_skill_root returns True for profile skills directories.""" + with tempfile.TemporaryDirectory() as td: + profile_skills = os.path.join(td, ".hermes", "profiles", "my-profile", "skills") + os.makedirs(profile_skills, exist_ok=True) + sm = SkillManager(profile_skills) + assert sm._is_hermes_skill_root() + + def test_is_hermes_skill_root_not_hermes(self): + """_is_hermes_skill_root returns False for non-Hermes directories.""" + with tempfile.TemporaryDirectory() as td: + skills_dir = os.path.join(td, "random-skills") + os.makedirs(skills_dir, exist_ok=True) + sm = SkillManager(skills_dir) + assert not sm._is_hermes_skill_root()