diff --git a/hermes_cli/skills_hub.py b/hermes_cli/skills_hub.py index 78de2ee15f..dd0aae4686 100644 --- a/hermes_cli/skills_hub.py +++ b/hermes_cli/skills_hub.py @@ -23,7 +23,6 @@ from rich.table import Table # Lazy imports to avoid circular dependencies and slow startup. # tools.skills_hub and tools.skills_guard are imported inside functions. from hermes_constants import display_hermes_home -from agent.skill_utils import is_excluded_skill_path _console = Console() @@ -184,29 +183,18 @@ def _existing_categories() -> List[str]: Used to suggest reusable categories when interactively installing from a URL. Hidden dirs (``.hub``, ``.trash``) are skipped. """ - from tools.skills_hub import SKILLS_DIR - out: List[str] = [] + from tools.skills_hub import SKILLS_DIR, _category_skill_dirs try: - for entry in SKILLS_DIR.iterdir(): - if not entry.is_dir() or entry.name.startswith("."): - continue - # Only count as a category if it contains skills, not if it IS a skill. - # Heuristic: if ``/SKILL.md`` exists, it's a skill at the - # top level (no category); otherwise treat as a category bucket. - if (entry / "SKILL.md").exists(): - continue - # Has at least one nested SKILL.md (excluding dependency/cache dirs)? - try: - if any( - not is_excluded_skill_path(p) - for p in entry.rglob("SKILL.md") - ): - out.append(entry.name) - except OSError: - continue + # _category_skill_dirs returns children containing any active + # SKILL.md — including top-level skills themselves. Only children + # WITHOUT their own SKILL.md are category buckets. + return sorted( + name + for name in set(_category_skill_dirs(SKILLS_DIR)) + if not (SKILLS_DIR / name / "SKILL.md").exists() + ) except (FileNotFoundError, OSError): return [] - return sorted(set(out)) def _prompt_for_skill_name(c: Console, url: str, default: str = "") -> Optional[str]: diff --git a/tools/skills_hub.py b/tools/skills_hub.py index 53fe456ed9..a193895ca5 100644 --- a/tools/skills_hub.py +++ b/tools/skills_hub.py @@ -3680,6 +3680,32 @@ def quarantine_bundle(bundle: SkillBundle) -> Path: return dest +def _category_skill_dirs(directory: Path) -> List[str]: + """Names of direct children of *directory* that contain skills. + + A child counts when it is a non-hidden directory holding at least one + active ``SKILL.md`` anywhere below it (recursive, so nested category + layouts like ``mlops/training/`` are detected). Vendored, + cache, and progressive-disclosure support paths are pruned via + :func:`is_excluded_skill_path` so a lone ``node_modules`` or + ``references/pkg/SKILL.md`` hit does not misclassify the directory as + a category. Shared by the install-time category guard here and + ``hermes_cli.skills_hub._existing_categories``. + """ + skill_dirs: List[str] = [] + for entry in directory.iterdir(): + if not entry.is_dir() or entry.name.startswith("."): + continue + for skill_md in entry.rglob("SKILL.md"): + if is_excluded_skill_path( + skill_md.relative_to(directory), root=directory + ): + continue + skill_dirs.append(entry.name) + break + return skill_dirs + + def install_from_quarantine( quarantine_path: Path, skill_name: str, @@ -3711,49 +3737,43 @@ def install_from_quarantine( # skill-plus-category directory; a later update or uninstall of the outer # skill would then rmtree the inner one — the sibling case of the # category-bucket wipe reported in issue #75983. - _skills_root = _skills_dir().resolve() - _ancestor = install_dir.parent - while _ancestor != _skills_root and _ancestor.is_relative_to(_skills_root): - if (_ancestor / "SKILL.md").is_file(): + skills_root = _skills_dir().resolve() + ancestor = install_dir.parent + while ancestor != skills_root and ancestor.is_relative_to(skills_root): + if (ancestor / "SKILL.md").is_file(): raise ValueError( - f"Refusing to install into '{_ancestor.name}': it is an " + f"Refusing to install into '{ancestor.name}': it is an " f"existing skill directory, not a category. Choose a " f"different category." ) - _ancestor = _ancestor.parent - - if install_dir.exists() and not install_dir.is_dir(): - # A stray regular file at the install path. rmtree() on a file raises - # NotADirectoryError (an uncaught traceback at the CLI); refuse with - # the same actionable ValueError contract the guards below use. - raise ValueError( - f"Refusing to install: '{install_dir.name}' already exists and " - f"is not a directory. Remove it or choose a different skill name." - ) + ancestor = ancestor.parent if install_dir.exists(): + if not install_dir.is_dir(): + # A stray regular file at the install path. rmtree() on a file + # raises NotADirectoryError (an uncaught traceback at the CLI); + # refuse with the same actionable ValueError contract instead. + raise ValueError( + f"Refusing to install: '{install_dir.name}' already exists " + f"and is not a directory. Remove it or choose a different " + f"skill name." + ) # Guard against silent data loss when the install target collides with # an existing category bucket (a directory that holds other skills). # This was reported as GitHub issue #75983: installing a skill with # --name matching an existing category directory caused rmtree to wipe - # all sibling skills. An existing skill installation (a directory that - # directly contains SKILL.md) is safe to overwrite — that path is - # already guarded by the lock-file check in do_install(). But a - # directory that contains *other* skill directories is a category bucket - # and must NOT be silently deleted. - if install_dir.is_dir() and not (install_dir / "SKILL.md").exists(): - _skill_dirs_in = [ - entry.name - for entry in install_dir.iterdir() - if entry.is_dir() - and not entry.name.startswith(".") - and any(entry.rglob("SKILL.md")) - ] - if _skill_dirs_in: + # all sibling skills. A directory that directly contains SKILL.md is + # an existing skill installation and stays overwritable (hub-installed + # skills are additionally guarded by the lock-file check in + # do_install()). But a directory that contains *other* skill + # directories is a category bucket and must NOT be silently deleted. + if not (install_dir / "SKILL.md").exists(): + skill_dirs_in = _category_skill_dirs(install_dir) + if skill_dirs_in: raise ValueError( f"Refusing to overwrite category directory '{install_dir}' " - f"which contains {len(_skill_dirs_in)} skill(s): " - f"{', '.join(sorted(_skill_dirs_in))}. " + f"which contains {len(skill_dirs_in)} skill(s): " + f"{', '.join(sorted(skill_dirs_in))}. " f"Use a different --name or install into a subcategory." ) shutil.rmtree(install_dir)