fix: preserve nested official skill install paths

This commit is contained in:
wysie 2026-05-27 18:15:52 +08:00 committed by kshitij
parent 53bdef5775
commit a38e283395
4 changed files with 64 additions and 10 deletions

View file

@ -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()

View file

@ -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.

View file

@ -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

View file

@ -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 ``<skill_name>`` or
``<category>/<skill_name>``. Reject anything else.
Enforce that ``install_path`` ends with ``<skill_name>``. Nested
official optional skills may legitimately install below paths such as
``mlops/training/<skill_name>``; 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 (<skill_name> or
# <category>/<skill_name>), 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
# <skill_name>, 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