refactor: share category detection and prune vendored SKILL.md hits
/simplify-code findings on the salvage stack: - extract _category_skill_dirs() as the single category detector; the install guard and hermes_cli._existing_categories() now share it (third copy of the heuristic eliminated) - filter rglob hits through is_excluded_skill_path so vendored / support-dir SKILL.md files (node_modules, references/pkg) no longer misclassify a plain directory as a category and block install - fix inaccurate WHY comment (lock-file check only guards hub-installed skills, not hand-authored dirs), drop underscore prefixes on locals, fold the file-collision guard under the single exists() check
This commit is contained in:
kshitijk4poor
@@ -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 ``<entry>/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]:
|
||||
|
||||
+51
-31
@@ -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/<skill>`` 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)
|
||||
|
||||
Reference in New Issue
Block a user