From a38e283395411517919b398a0d1076b72ca4a01e Mon Sep 17 00:00:00 2001 From: wysie Date: Wed, 27 May 2026 18:15:52 +0800 Subject: [PATCH] fix: preserve nested official skill install paths --- hermes_cli/skills_hub.py | 8 ++++--- tests/hermes_cli/test_skills_hub.py | 33 +++++++++++++++++++++++++++++ tests/tools/test_skills_hub.py | 13 ++++++++++++ tools/skills_hub.py | 20 +++++++++++------ 4 files changed, 64 insertions(+), 10 deletions(-) diff --git a/hermes_cli/skills_hub.py b/hermes_cli/skills_hub.py index 5598e6d2b6b..8e7276bdc30 100644 --- a/hermes_cli/skills_hub.py +++ b/hermes_cli/skills_hub.py @@ -519,11 +519,13 @@ def do_install(identifier: str, category: str = "", force: bool = False, if bundle.source == "url" and not category and not skip_confirm: category = _prompt_for_category(c, _existing_categories()) - # Auto-detect category for official skills (e.g. "official/autonomous-ai-agents/blackbox") + # Auto-detect the full parent path for official skills. Optional skills + # can be nested (e.g. "official/mlops/training/trl-fine-tuning"), so keep + # every identifier segment between "official" and the final skill slug. if bundle.source == "official" and not category: - id_parts = bundle.identifier.split("/") # ["official", "category", "skill"] + id_parts = bundle.identifier.split("/") if len(id_parts) >= 3: - category = id_parts[1] + category = "/".join(id_parts[1:-1]) # Check if already installed lock = HubLockFile() diff --git a/tests/hermes_cli/test_skills_hub.py b/tests/hermes_cli/test_skills_hub.py index 7b262a75a76..25798dcd3eb 100644 --- a/tests/hermes_cli/test_skills_hub.py +++ b/tests/hermes_cli/test_skills_hub.py @@ -371,6 +371,39 @@ def test_do_install_scans_official_bundles_with_source_provenance( assert scanned["source"] == "official" +def test_do_install_preserves_nested_official_optional_path( + monkeypatch, tmp_path, hub_env +): + class _OfficialNestedSource: + def inspect(self, identifier): + return type("Meta", (), { + "extra": {}, + "identifier": "official/mlops/training/trl-fine-tuning", + })() + + def fetch(self, identifier): + return type("Bundle", (), { + "name": "trl-fine-tuning", + "files": {"SKILL.md": "# TRL"}, + "source": "official", + "identifier": "official/mlops/training/trl-fine-tuning", + "trust_level": "builtin", + "metadata": {}, + })() + + installs = _install_mocks(monkeypatch, tmp_path, _OfficialNestedSource) + + sink = StringIO() + console = Console(file=sink, force_terminal=False, color_system=None) + do_install( + "official/mlops/training/trl-fine-tuning", + console=console, + skip_confirm=True, + ) + + assert installs == [{"name": "trl-fine-tuning", "category": "mlops/training"}] + + # --------------------------------------------------------------------------- # UrlSource-specific install paths: --name override, interactive prompts, # non-interactive error, existing-category scan. diff --git a/tests/tools/test_skills_hub.py b/tests/tools/test_skills_hub.py index 9c1c1b72a64..22406a8bacd 100644 --- a/tests/tools/test_skills_hub.py +++ b/tests/tools/test_skills_hub.py @@ -1788,6 +1788,19 @@ class TestInstallPathSafety: ) assert lock.get_installed("good")["install_path"] == "devops/good" + def test_record_install_accepts_nested_official_skill_path(self, tmp_path): + lock = HubLockFile(path=tmp_path / "lock.json") + lock.record_install( + name="trl-fine-tuning", source="official", + identifier="official/mlops/training/trl-fine-tuning", + trust_level="builtin", scan_verdict="pass", + skill_hash="h", install_path="mlops/training/trl-fine-tuning", + files=["SKILL.md"], + ) + entry = lock.get_installed("trl-fine-tuning") + assert entry is not None + assert entry["install_path"] == "mlops/training/trl-fine-tuning" + def test_uninstall_rejects_poisoned_absolute_path(self, tmp_path, isolated_skills_dir, patch_lock_file): """Hand-edited lock.json with absolute install_path must not delete anything.""" from tools.skills_hub import uninstall_skill diff --git a/tools/skills_hub.py b/tools/skills_hub.py index 9021af5222f..657a455cf4a 100644 --- a/tools/skills_hub.py +++ b/tools/skills_hub.py @@ -124,6 +124,10 @@ def _validate_category_name(category: str) -> str: return _normalize_bundle_path(category, field_name="category", allow_nested=False) +def _validate_install_parent_path(category: str) -> str: + return _normalize_bundle_path(category, field_name="install parent path", allow_nested=True) + + def _normalize_lock_install_path(install_path: str, skill_name: str) -> str: """Validate a skill install path before it touches the lock file or disk. @@ -134,8 +138,10 @@ def _normalize_lock_install_path(install_path: str, skill_name: str) -> str: let ``rmtree`` wipe either the entire ``skills/`` tree or content outside it. - Enforce that ``install_path`` is exactly ```` or - ``/``. Reject anything else. + Enforce that ``install_path`` ends with ````. Nested + official optional skills may legitimately install below paths such as + ``mlops/training/``; traversal, absolute paths, empty paths, + and mismatched final components are still rejected. """ safe_skill_name = _validate_skill_name(skill_name) normalized = _normalize_bundle_path( @@ -144,7 +150,7 @@ def _normalize_lock_install_path(install_path: str, skill_name: str) -> str: allow_nested=True, ) parts = normalized.split("/") - if len(parts) not in {1, 2} or parts[-1] != safe_skill_name: + if not parts or parts[-1] != safe_skill_name: raise ValueError(f"Unsafe install path: {install_path}") return normalized @@ -3008,7 +3014,7 @@ def install_from_quarantine( ) -> Path: """Move a scanned skill from quarantine into the skills directory.""" safe_skill_name = _validate_skill_name(skill_name) - safe_category = _validate_category_name(category) if category else "" + safe_category = _validate_install_parent_path(category) if category else "" quarantine_resolved = quarantine_path.resolve() quarantine_root = QUARANTINE_DIR.resolve() if not quarantine_resolved.is_relative_to(quarantine_root): @@ -3095,9 +3101,9 @@ def uninstall_skill(skill_name: str) -> Tuple[bool, str]: # the destructive boundary — anything that falls through to the rmtree # below MUST be inside SKILLS_DIR and MUST NOT be SKILLS_DIR itself # (an empty/"."/"/" install_path would otherwise wipe the entire tree). - # _resolve_lock_install_path enforces shape ( or - # /), rejects absolute/traversal paths, and walks - # the path component-by-component refusing symlink/junction redirects. + # _resolve_lock_install_path enforces a relative path ending in + # , rejects absolute/traversal paths, and walks the path + # component-by-component refusing symlink/junction redirects. try: install_path = _resolve_lock_install_path( entry.get("install_path", ""), skill_name