From 7a2019a55383eb628dd21cd3debe371d7b97a944 Mon Sep 17 00:00:00 2001 From: ModeIO Local Date: Wed, 15 Apr 2026 23:12:55 +0800 Subject: [PATCH] Persist exact managed skill source links --- .../application/marketplace/catalog.py | 8 +- .../application/marketplace/resolver.py | 25 ++--- skill_manager/application/skills/inventory.py | 4 + skill_manager/application/skills/mutations.py | 17 +++- skill_manager/application/skills/queries.py | 25 +++-- .../application/source_fetch_service.py | 24 ++++- skill_manager/domain/observations.py | 2 + skill_manager/sources/__init__.py | 6 +- skill_manager/sources/github.py | 93 ++++++++++++++----- skill_manager/store/manifest.py | 11 ++- skill_manager/store/shared_store.py | 25 ++++- tests/integration/test_marketplace_api.py | 14 ++- tests/support/marketplace_fixture.py | 2 +- tests/unit/test_application_service.py | 72 +++++++++++++- tests/unit/test_manifest.py | 2 + tests/unit/test_marketplace_service.py | 4 +- tests/unit/test_sources.py | 39 +++++++- tests/unit/test_store.py | 8 +- 18 files changed, 299 insertions(+), 82 deletions(-) diff --git a/skill_manager/application/marketplace/catalog.py b/skill_manager/application/marketplace/catalog.py index 5dc7233..8c9b402 100644 --- a/skill_manager/application/marketplace/catalog.py +++ b/skill_manager/application/marketplace/catalog.py @@ -43,7 +43,7 @@ class MarketplaceCatalog: DETAIL_MISSING_FALLBACK = "No summary available on skills.sh." _LEADERBOARD_TTL_SECONDS = 3600 _DETAIL_TTL_SECONDS = 86400 - _DETAIL_NAMESPACE = "details-v2" + _DETAIL_NAMESPACE = "details-v3" _SEARCH_TTL_SECONDS = 900 _SEARCH_FETCH_FLOOR = 40 _SEARCH_CACHE_LIMIT = 24 @@ -153,7 +153,7 @@ def repo_metadata(self, repo: str) -> RepoDisplayMetadata: def detail_enrichment(self, record: SkillsShSkill) -> DetailEnrichment: cached = self._cached_detail(record) - if cached is not None and cached.folder_resolution_complete and not self._needs_folder_refresh(cached): + if cached is not None and cached.folder_resolution_complete: return cached summary = cached @@ -289,10 +289,6 @@ def _cached_detail(self, record: SkillsShSkill) -> DetailEnrichment | None: return None return DetailEnrichment.from_dict(detail_cache.payload) - @staticmethod - def _needs_folder_refresh(detail: DetailEnrichment) -> bool: - return bool(detail.folder_url and "/tree/HEAD/" in detail.folder_url) - def _resolve_summary_enrichment(self, record: SkillsShSkill) -> DetailEnrichment: description = record.description_hint.strip() if self._is_usable_description(record.description_hint) else "" document = self._detail_fetcher(record.detail_url) diff --git a/skill_manager/application/marketplace/resolver.py b/skill_manager/application/marketplace/resolver.py index d1834a4..fd5dea6 100644 --- a/skill_manager/application/marketplace/resolver.py +++ b/skill_manager/application/marketplace/resolver.py @@ -2,11 +2,9 @@ from dataclasses import dataclass from pathlib import Path -import subprocess from tempfile import TemporaryDirectory -from urllib.parse import quote -from skill_manager.sources import GitHubSource +from skill_manager.sources import GitHubSource, github_folder_url as build_github_folder_url from .repo_snapshots import GitHubRepoSnapshotService from .models import RepoDisplayMetadata @@ -55,20 +53,9 @@ def github_folder_url(self, repo: str, skill_id: str, *, default_branch: str | N locator = f"{owner}/{repo_name}/{skill_id}" with TemporaryDirectory(prefix="skill-manager-marketplace-") as temp_dir: work_dir = Path(temp_dir) - skill_path = GitHubSource().fetch(locator, work_dir) - clone_dir = work_dir / f"{owner}--{repo_name}" - branch = default_branch or self._checked_out_branch(clone_dir) or "HEAD" - relative_path = skill_path.relative_to(clone_dir).as_posix() - return f"https://github.com/{repo}/tree/{quote(branch, safe='')}/{quote(relative_path, safe='/')}" - - @staticmethod - def _checked_out_branch(clone_dir: Path) -> str | None: - result = subprocess.run( - ["git", "-C", str(clone_dir), "branch", "--show-current"], - check=True, - capture_output=True, - text=True, - timeout=10, + resolved = GitHubSource().resolve(locator, work_dir) + return build_github_folder_url( + repo, + ref=default_branch or resolved.ref, + relative_path=resolved.relative_path, ) - branch = result.stdout.strip() - return branch or None diff --git a/skill_manager/application/skills/inventory.py b/skill_manager/application/skills/inventory.py index 6965900..9aa78ea 100644 --- a/skill_manager/application/skills/inventory.py +++ b/skill_manager/application/skills/inventory.py @@ -38,6 +38,8 @@ class InventoryEntry: source: SourceDescriptor current_revision: str | None = None recorded_revision: str | None = None + source_ref: str | None = None + source_path: str | None = None package_dir: str | None = None package_path: Path | None = None sightings: list[InventorySighting] = field(default_factory=list) @@ -104,6 +106,8 @@ def from_snapshot(cls, *, store_scan: StoreScan, harness_scans: tuple[HarnessSca source=package.source, current_revision=package.revision, recorded_revision=store_package.recorded_revision, + source_ref=store_package.recorded_source_ref, + source_path=store_package.recorded_source_path, package_dir=package.root_path.name, package_path=package.root_path, ) diff --git a/skill_manager/application/skills/mutations.py b/skill_manager/application/skills/mutations.py index 197ebd5..83518b2 100644 --- a/skill_manager/application/skills/mutations.py +++ b/skill_manager/application/skills/mutations.py @@ -92,13 +92,18 @@ def update_skill(self, skill_ref: str) -> dict[str, bool]: if entry.package_dir is None: raise MutationError("managed skill is missing its package directory name", status=500) with TemporaryDirectory(prefix="skill-update-") as work_dir: - skill_path = self.source_fetcher.fetch( + fetched = self.source_fetcher.fetch_package( source_kind=entry.source.kind, source_locator=entry.source.locator, work_dir=Path(work_dir), ) try: - self.read_models.store.update(entry.package_dir, source_path=skill_path) + self.read_models.store.update( + entry.package_dir, + source_path=fetched.package_path, + source_ref=fetched.source_ref, + source_path_hint=fetched.source_path, + ) except ValueError as error: raise MutationError(str(error), status=409) from error self.read_models.invalidate() @@ -176,21 +181,23 @@ def delete_skill(self, skill_ref: str) -> dict[str, bool]: def install_skill(self, *, source_kind: str, source_locator: str) -> dict[str, bool]: with TemporaryDirectory(prefix="skill-install-") as work_dir: - skill_path = self.source_fetcher.fetch( + fetched = self.source_fetcher.fetch_package( source_kind=source_kind, source_locator=source_locator, work_dir=Path(work_dir), ) package = parse_skill_package( - skill_path, + fetched.package_path, default_source=SourceDescriptor(kind=source_kind, locator=source_locator), ) try: self.read_models.store.ingest( - source_path=skill_path, + source_path=fetched.package_path, declared_name=package.declared_name, source_kind=source_kind, source_locator=source_locator, + source_ref=fetched.source_ref, + source_path_hint=fetched.source_path, ) except ValueError as error: raise MutationError(str(error), status=409) from error diff --git a/skill_manager/application/skills/queries.py b/skill_manager/application/skills/queries.py index f38165e..5537e69 100644 --- a/skill_manager/application/skills/queries.py +++ b/skill_manager/application/skills/queries.py @@ -6,7 +6,7 @@ from skill_manager.domain import fingerprint_package from skill_manager.errors import MutationError -from skill_manager.sources import github_repo_from_locator, github_repo_url, github_skill_dir_from_locator +from skill_manager.sources import github_folder_url, github_repo_from_locator, github_repo_url from ..document_utils import read_skill_document_markdown from ..read_model_service import ReadModelService @@ -101,17 +101,28 @@ def build_source_links(self, entry: InventoryEntry) -> dict[str, str | None] | N if repo is None: return None - skill_dir = github_skill_dir_from_locator(entry.source.locator) - folder_url = None - if skill_dir: - folder_url = f"{github_repo_url(repo)}/tree/HEAD/{skill_dir}" - return { "repoLabel": repo, "repoUrl": github_repo_url(repo), - "folderUrl": folder_url, + "folderUrl": self._github_folder_url(entry, repo), } + def _github_folder_url(self, entry: InventoryEntry, repo: str) -> str | None: + if entry.source_ref is not None and entry.source_path is not None: + return github_folder_url(repo, ref=entry.source_ref, relative_path=entry.source_path) + if entry.source.locator.removeprefix("github:").count("/") < 2: + return None + with TemporaryDirectory(prefix="skill-source-links-") as work_dir: + try: + fetched = self.source_fetcher.fetch_package( + source_kind=entry.source.kind, + source_locator=entry.source.locator, + work_dir=Path(work_dir), + ) + except MutationError: + return None + return github_folder_url(repo, ref=fetched.source_ref, relative_path=fetched.source_path) + def resolve_update_status( self, entry: InventoryEntry, diff --git a/skill_manager/application/source_fetch_service.py b/skill_manager/application/source_fetch_service.py index 780acb6..f675a1c 100644 --- a/skill_manager/application/source_fetch_service.py +++ b/skill_manager/application/source_fetch_service.py @@ -1,22 +1,42 @@ from __future__ import annotations +from dataclasses import dataclass from pathlib import Path from skill_manager.errors import MutationError from skill_manager.sources import GitHubSource +@dataclass(frozen=True) +class FetchedSourcePackage: + package_path: Path + source_ref: str | None = None + source_path: str | None = None + + class SourceFetchService: def __init__(self, *, github: GitHubSource | None = None) -> None: self._github = github or GitHubSource() - def fetch(self, *, source_kind: str, source_locator: str, work_dir: Path) -> Path: + def fetch_package(self, *, source_kind: str, source_locator: str, work_dir: Path) -> FetchedSourcePackage: try: if source_kind == "github": locator = source_locator.removeprefix("github:") - return self._github.fetch(locator, work_dir) + resolved = self._github.resolve(locator, work_dir) + return FetchedSourcePackage( + package_path=resolved.package_path, + source_ref=resolved.ref, + source_path=resolved.relative_path, + ) except MutationError: raise except Exception as error: # noqa: BLE001 raise MutationError(str(error), status=400) from error raise MutationError(f"unsupported source kind: {source_kind}", status=400) + + def fetch(self, *, source_kind: str, source_locator: str, work_dir: Path) -> Path: + return self.fetch_package( + source_kind=source_kind, + source_locator=source_locator, + work_dir=work_dir, + ).package_path diff --git a/skill_manager/domain/observations.py b/skill_manager/domain/observations.py index 75e12c1..603cb73 100644 --- a/skill_manager/domain/observations.py +++ b/skill_manager/domain/observations.py @@ -27,6 +27,8 @@ class BuiltinObservation: class StorePackageObservation: package: SkillPackage recorded_revision: str | None = None + recorded_source_ref: str | None = None + recorded_source_path: str | None = None @dataclass(frozen=True) diff --git a/skill_manager/sources/__init__.py b/skill_manager/sources/__init__.py index f539640..d5ab225 100644 --- a/skill_manager/sources/__init__.py +++ b/skill_manager/sources/__init__.py @@ -3,10 +3,11 @@ GitHubRepoMetadataError, GitHubRepoMetadataClient, GitHubSource, + ResolvedGitHubSkill, + github_folder_url, github_owner_avatar_url, github_repo_owner, github_repo_from_locator, - github_skill_dir_from_locator, github_repo_url, is_valid_github_repo, ) @@ -16,10 +17,11 @@ "GitHubRepoMetadataError", "GitHubRepoMetadataClient", "GitHubSource", + "ResolvedGitHubSkill", + "github_folder_url", "github_owner_avatar_url", "github_repo_owner", "github_repo_from_locator", - "github_skill_dir_from_locator", "github_repo_url", "is_valid_github_repo", ] diff --git a/skill_manager/sources/github.py b/skill_manager/sources/github.py index b375b52..b8df0e3 100644 --- a/skill_manager/sources/github.py +++ b/skill_manager/sources/github.py @@ -8,6 +8,7 @@ import time from typing import Callable from urllib.error import HTTPError, URLError +from urllib.parse import quote from urllib.request import Request, urlopen _CACHE_TTL_SECONDS = 900 @@ -21,6 +22,15 @@ class GitHubRepoMetadata: default_branch: str | None = None +@dataclass(frozen=True) +class ResolvedGitHubSkill: + repo: str + ref: str | None + relative_path: str + package_path: Path + clone_dir: Path + + class GitHubRepoMetadataError(Exception): def __init__(self, repo: str, detail: str, *, status_code: int | None = None) -> None: self.repo = repo @@ -33,20 +43,18 @@ def __init__(self, repo: str, detail: str, *, status_code: int | None = None) -> def _parse_locator(locator: str) -> tuple[str, str, str]: - parts = locator.split("/", 2) - if len(parts) != 3: - raise ValueError(f"invalid github locator (expected owner/repo/skill-dir): {locator}") - return parts[0], parts[1], parts[2] + owner, repo, skill_dir = _parse_repo_identity(locator) + if skill_dir is None or not skill_dir.strip(): + raise ValueError(f"invalid github locator (expected owner/repo/): {locator}") + return owner, repo, skill_dir def _parse_repo_identity(locator: str) -> tuple[str, str, str | None]: stripped = locator.removeprefix("github:") - parts = stripped.split("/") - if len(parts) == 2: - return parts[0], parts[1], None - if len(parts) == 3: - return parts[0], parts[1], parts[2] - raise ValueError(f"invalid github locator (expected owner/repo or owner/repo/skill-dir): {locator}") + parts = stripped.split("/", 2) + if len(parts) < 2 or not parts[0] or not parts[1]: + raise ValueError(f"invalid github locator (expected owner/repo or owner/repo/): {locator}") + return parts[0], parts[1], parts[2] if len(parts) == 3 else None def github_repo_from_locator(locator: str) -> str | None: @@ -57,14 +65,6 @@ def github_repo_from_locator(locator: str) -> str | None: return f"{owner}/{repo}" -def github_skill_dir_from_locator(locator: str) -> str | None: - try: - _, _, skill_dir = _parse_repo_identity(locator) - except ValueError: - return None - return skill_dir - - def is_valid_github_repo(repo: str) -> bool: if repo.count("/") != 1: return False @@ -82,6 +82,15 @@ def github_repo_url(repo: str) -> str: return f"https://github.com/{repo}" +def github_folder_url(repo: str, *, ref: str | None, relative_path: str | None) -> str | None: + if not ref: + return None + normalized_path = _normalize_relative_path(relative_path) + if normalized_path == ".": + return None + return f"{github_repo_url(repo)}/tree/{quote(ref, safe='')}/{quote(normalized_path, safe='/')}" + + def github_owner_avatar_url(repo: str, *, size: int = 96) -> str | None: owner = github_repo_owner(repo) if owner is None: @@ -181,17 +190,24 @@ def _find_skill(clone_dir: Path, skill_dir: str) -> Path | None: return None +def _normalize_relative_path(relative_path: str | None) -> str: + if relative_path is None: + return "." + normalized = relative_path.strip().strip("/") + return normalized or "." + + class GitHubSource: - def fetch(self, locator: str, work_dir: Path) -> Path: - owner, repo, skill_dir = _parse_locator(locator) - clone_dir = work_dir / f"{owner}--{repo}" + def resolve(self, locator: str, work_dir: Path) -> ResolvedGitHubSkill: + owner, repo_name, skill_dir = _parse_locator(locator) + clone_dir = work_dir / f"{owner}--{repo_name}" subprocess.run( [ "git", "clone", "--depth", "1", - f"https://github.com/{owner}/{repo}.git", + f"https://github.com/{owner}/{repo_name}.git", str(clone_dir), ], check=True, @@ -200,5 +216,34 @@ def fetch(self, locator: str, work_dir: Path) -> Path: ) skill_path = _find_skill(clone_dir, skill_dir) if skill_path is None: - raise ValueError(f"skill directory '{skill_dir}' not found in {owner}/{repo}") - return skill_path + raise ValueError(f"skill directory '{skill_dir}' not found in {owner}/{repo_name}") + return ResolvedGitHubSkill( + repo=f"{owner}/{repo_name}", + ref=self._checked_out_ref(clone_dir), + relative_path=_normalize_relative_path(skill_path.relative_to(clone_dir).as_posix()), + package_path=skill_path, + clone_dir=clone_dir, + ) + + def fetch(self, locator: str, work_dir: Path) -> Path: + return self.resolve(locator, work_dir).package_path + + @staticmethod + def _checked_out_ref(clone_dir: Path) -> str | None: + branch = subprocess.run( + ["git", "-C", str(clone_dir), "rev-parse", "--abbrev-ref", "HEAD"], + check=True, + capture_output=True, + text=True, + timeout=10, + ).stdout.strip() + if branch and branch != "HEAD": + return branch + commit = subprocess.run( + ["git", "-C", str(clone_dir), "rev-parse", "HEAD"], + check=True, + capture_output=True, + text=True, + timeout=10, + ).stdout.strip() + return commit or None diff --git a/skill_manager/store/manifest.py b/skill_manager/store/manifest.py index a62ceef..2857def 100644 --- a/skill_manager/store/manifest.py +++ b/skill_manager/store/manifest.py @@ -12,15 +12,22 @@ class ManifestEntry: source_kind: str source_locator: str revision: str + source_ref: str | None = None + source_path: str | None = None def to_dict(self) -> dict[str, str]: - return { + payload = { "packageDir": self.package_dir, "declaredName": self.declared_name, "sourceKind": self.source_kind, "sourceLocator": self.source_locator, "revision": self.revision, } + if self.source_ref is not None: + payload["sourceRef"] = self.source_ref + if self.source_path is not None: + payload["sourcePath"] = self.source_path + return payload @dataclass(frozen=True) @@ -42,6 +49,8 @@ def load_manifest(path: Path) -> StoreManifest: source_kind=item["sourceKind"], source_locator=item["sourceLocator"], revision=item["revision"], + source_ref=item.get("sourceRef") if isinstance(item.get("sourceRef"), str) else None, + source_path=item.get("sourcePath") if isinstance(item.get("sourcePath"), str) else None, ) for item in payload.get("entries", []) ) diff --git a/skill_manager/store/shared_store.py b/skill_manager/store/shared_store.py index 493a8a2..6a20237 100644 --- a/skill_manager/store/shared_store.py +++ b/skill_manager/store/shared_store.py @@ -34,6 +34,8 @@ def scan(self) -> StoreScan: StorePackageObservation( package=parse_skill_package(path, default_source=source), recorded_revision=entry.revision if entry else None, + recorded_source_ref=entry.source_ref if entry else None, + recorded_source_path=entry.source_path if entry else None, ) ) return StoreScan(packages=tuple(packages), issues=tuple(issue.message for issue in self.check_integrity())) @@ -45,6 +47,8 @@ def ingest( declared_name: str, source_kind: str, source_locator: str, + source_ref: str | None = None, + source_path_hint: str | None = None, ) -> Path: """Copy a skill package into the shared store and update the manifest.""" self.root.mkdir(parents=True, exist_ok=True) @@ -60,11 +64,20 @@ def ingest( source_kind=source_kind, source_locator=source_locator, revision=fingerprint, + source_ref=source_ref, + source_path=source_path_hint, ) write_manifest(self.manifest_path, StoreManifest(entries=manifest.entries + (entry,))) return dest - def update(self, package_dir: str, *, source_path: Path) -> tuple[Path, bool]: + def update( + self, + package_dir: str, + *, + source_path: Path, + source_ref: str | None = None, + source_path_hint: str | None = None, + ) -> tuple[Path, bool]: """Replace a shared package with a new version. Returns (path, changed).""" dest = self.root / package_dir if not dest.is_dir(): @@ -77,7 +90,15 @@ def update(self, package_dir: str, *, source_path: Path) -> tuple[Path, bool]: shutil.copytree(source_path, dest) manifest = load_manifest(self.manifest_path) updated = tuple( - ManifestEntry(e.package_dir, e.declared_name, e.source_kind, e.source_locator, new_fp) + ManifestEntry( + e.package_dir, + e.declared_name, + e.source_kind, + e.source_locator, + new_fp, + e.source_ref if source_ref is None else source_ref, + e.source_path if source_path_hint is None else source_path_hint, + ) if e.package_dir == package_dir else e for e in manifest.entries diff --git a/tests/integration/test_marketplace_api.py b/tests/integration/test_marketplace_api.py index 75dcbba..da3eacf 100644 --- a/tests/integration/test_marketplace_api.py +++ b/tests/integration/test_marketplace_api.py @@ -10,7 +10,7 @@ from skill_manager.application.marketplace.skillssh import fetch_all_time_leaderboard, fetch_detail_page, search_skills from skill_manager.application.source_fetch_service import SourceFetchService from skill_manager.errors import MARKETPLACE_UNAVAILABLE_MESSAGE, MutationError -from skill_manager.sources import github_owner_avatar_url +from skill_manager.sources import ResolvedGitHubSkill, github_owner_avatar_url from tests.support.app_harness import AppTestHarness from tests.support.fake_home import seed_skill_package from tests.support.marketplace_fixture import create_fixture_marketplace_service @@ -47,12 +47,22 @@ def __init__(self, roots: dict[str, Path], failures: set[str] | None = None) -> self._failures = failures or set() def fetch(self, locator: str, work_dir: Path) -> Path: + return self.resolve(locator, work_dir).package_path + + def resolve(self, locator: str, work_dir: Path) -> ResolvedGitHubSkill: if locator in self._failures: raise MutationError("unable to fetch source", status=400) root = self._roots.get(locator) if root is None: raise MutationError("unknown source", status=400) - return root + owner, repo_name, _skill = locator.split("/", 2) + return ResolvedGitHubSkill( + repo=f"{owner}/{repo_name}", + ref="main", + relative_path=f"skills/{root.name}", + package_path=root, + clone_dir=work_dir / f"{owner}--{repo_name}", + ) def _fixture_catalog(env: dict[str, str], *, broken_repos: set[str] | None = None) -> MarketplaceCatalog: diff --git a/tests/support/marketplace_fixture.py b/tests/support/marketplace_fixture.py index 3fc137b..f4e33ab 100644 --- a/tests/support/marketplace_fixture.py +++ b/tests/support/marketplace_fixture.py @@ -29,7 +29,7 @@ def create_fixture_marketplace_service() -> MarketplaceCatalog: cache.write("leaderboard", "all-time", [skill_to_dict(item) for item in skills]) for skill in skills: cache.write( - "details-v2", + "details-v3", skill.detail_url, DetailEnrichment( description=next(item["description"] for item in FIXTURE_SKILLS if item["skillId"] == skill.skill_id), diff --git a/tests/unit/test_application_service.py b/tests/unit/test_application_service.py index 8720687..f409ad6 100644 --- a/tests/unit/test_application_service.py +++ b/tests/unit/test_application_service.py @@ -6,6 +6,7 @@ from skill_manager.application import build_backend_container from skill_manager.domain import fingerprint_package +from skill_manager.sources import ResolvedGitHubSkill from skill_manager.store import ManifestEntry from tests.support.fake_home import ( @@ -20,12 +21,31 @@ class _StaticGitHubSource: - def __init__(self, package_root: Path) -> None: + def __init__( + self, + package_root: Path, + *, + repo: str = "mode-io/skills", + ref: str = "main", + relative_path: str = ".", + ) -> None: self.package_root = package_root + self.repo = repo + self.ref = ref + self.relative_path = relative_path def fetch(self, locator: str, work_dir: Path) -> Path: return self.package_root + def resolve(self, locator: str, work_dir: Path) -> ResolvedGitHubSkill: + return ResolvedGitHubSkill( + repo=self.repo, + ref=self.ref, + relative_path=self.relative_path, + package_path=self.package_root, + clone_dir=work_dir / self.repo.replace("/", "--"), + ) + class BackendContainerTests(unittest.TestCase): def test_list_skills_groups_identical_local_copies_and_preserves_builtins(self) -> None: @@ -165,7 +185,7 @@ def test_skill_detail_orders_managed_locations_with_shared_store_first(self) -> self.assertEqual(detail["actions"]["stopManagingStatus"], "available") self.assertEqual(detail["actions"]["stopManagingHarnessLabels"], ["Codex"]) - def test_source_links_use_local_head_folder_url(self) -> None: + def test_source_links_use_persisted_exact_folder_url(self) -> None: with TemporaryDirectory() as temp_dir: spec = create_fake_home_spec(Path(temp_dir)) package_root = seed_skill_package( @@ -185,12 +205,13 @@ def test_source_links_use_local_head_folder_url(self) -> None: source_kind="github", source_locator="github:mode-io/skills/shared-audit", revision=fingerprint_package(package_root)[0], + source_ref="main", + source_path="skills/shared-audit", ) ], ) container = build_backend_container(spec.env()) - container.source_fetcher._github = _StaticGitHubSource(package_root) # type: ignore[assignment] payload = container.skills_queries.list_skills() shared = next(row for row in payload["rows"] if row["name"] == "Shared Audit") detail = container.skills_queries.get_skill_detail(shared["skillRef"]) @@ -202,10 +223,53 @@ def test_source_links_use_local_head_folder_url(self) -> None: self.assertEqual(detail["sourceLinks"], { "repoLabel": "mode-io/skills", "repoUrl": "https://github.com/mode-io/skills", - "folderUrl": "https://github.com/mode-io/skills/tree/HEAD/shared-audit", + "folderUrl": "https://github.com/mode-io/skills/tree/main/skills/shared-audit", }) self.assertEqual(source_status["updateStatus"], "no_update_available") + def test_source_links_fall_back_to_exact_github_resolution_for_legacy_entries(self) -> None: + with TemporaryDirectory() as temp_dir: + spec = create_fake_home_spec(Path(temp_dir)) + package_root = seed_skill_package( + spec.shared_store_root, + "agent-browser", + "agent-browser", + body="Shared package fixture.", + source_kind="github", + source_locator="github:vercel-labs/agent-browser/agent-browser", + ) + seed_store_manifest( + spec, + [ + ManifestEntry( + package_dir="agent-browser", + declared_name="agent-browser", + source_kind="github", + source_locator="github:vercel-labs/agent-browser/agent-browser", + revision=fingerprint_package(package_root)[0], + ) + ], + ) + + container = build_backend_container(spec.env()) + container.source_fetcher._github = _StaticGitHubSource( # type: ignore[assignment] + package_root, + repo="vercel-labs/agent-browser", + ref="main", + relative_path="skills/agent-browser", + ) + payload = container.skills_queries.list_skills() + shared = next(row for row in payload["rows"] if row["name"] == "agent-browser") + detail = container.skills_queries.get_skill_detail(shared["skillRef"]) + + assert detail is not None + + self.assertEqual(detail["sourceLinks"], { + "repoLabel": "vercel-labs/agent-browser", + "repoUrl": "https://github.com/vercel-labs/agent-browser", + "folderUrl": "https://github.com/vercel-labs/agent-browser/tree/main/skills/agent-browser", + }) + def test_marketplace_queries_mark_matching_managed_source_as_installed(self) -> None: with TemporaryDirectory() as temp_dir: spec = create_fake_home_spec(Path(temp_dir)) diff --git a/tests/unit/test_manifest.py b/tests/unit/test_manifest.py index dccbc6b..1fdcf26 100644 --- a/tests/unit/test_manifest.py +++ b/tests/unit/test_manifest.py @@ -19,6 +19,8 @@ def test_manifest_round_trip(self) -> None: source_kind="github", source_locator="github:mode-io/shared-audit", revision="abc123", + source_ref="main", + source_path="skills/shared-audit", ), ) ) diff --git a/tests/unit/test_marketplace_service.py b/tests/unit/test_marketplace_service.py index 36d1497..6945014 100644 --- a/tests/unit/test_marketplace_service.py +++ b/tests/unit/test_marketplace_service.py @@ -143,7 +143,7 @@ def test_popular_page_fetches_real_descriptions_on_cold_cache(self) -> None: payload = service.popular_page()["items"] self.assertEqual(payload[0]["description"], "Switch between supported skill execution modes.") - cached = cache.read("details-v2", record.detail_url, ttl_seconds=3600) + cached = cache.read("details-v3", record.detail_url, ttl_seconds=3600) self.assertIsNotNone(cached) def test_search_page_falls_back_to_detail_when_hint_is_missing(self) -> None: @@ -302,7 +302,7 @@ def test_detail_enrichment_fetches_and_caches_on_first_access(self) -> None: self.assertEqual(detail.description, "Switch between supported skill execution modes.") self.assertEqual(detail.folder_url, "https://github.com/mode-io/skills/tree/main/skills/mode-switch") - cached = cache.read("details-v2", record.detail_url, ttl_seconds=3600) + cached = cache.read("details-v3", record.detail_url, ttl_seconds=3600) self.assertIsNotNone(cached) diff --git a/tests/unit/test_sources.py b/tests/unit/test_sources.py index 28cf1c6..2d43ca9 100644 --- a/tests/unit/test_sources.py +++ b/tests/unit/test_sources.py @@ -13,11 +13,12 @@ ) from skill_manager.sources.github import ( GitHubSource, + ResolvedGitHubSkill, _find_skill, _parse_locator, + github_folder_url, github_owner_avatar_url, github_repo_from_locator, - github_skill_dir_from_locator, is_valid_github_repo, ) @@ -31,6 +32,12 @@ def test_parse_three_part_locator(self) -> None: self.assertEqual(repo, "skills") self.assertEqual(skill_dir, "commit-message") + def test_parse_locator_preserves_nested_skill_hint(self) -> None: + owner, repo, skill_dir = _parse_locator("vercel-labs/agent-browser/agent-browser") + self.assertEqual(owner, "vercel-labs") + self.assertEqual(repo, "agent-browser") + self.assertEqual(skill_dir, "agent-browser") + def test_parse_rejects_two_parts(self) -> None: with self.assertRaises(ValueError): _parse_locator("anthropics/skills") @@ -41,9 +48,21 @@ def test_github_repo_from_locator(self) -> None: def test_github_repo_from_two_part_locator(self) -> None: self.assertEqual(github_repo_from_locator("github:mode-io/shared-audit"), "mode-io/shared-audit") - def test_github_skill_dir_from_locator(self) -> None: - self.assertEqual(github_skill_dir_from_locator("github:anthropics/skills/commit-message"), "commit-message") - self.assertIsNone(github_skill_dir_from_locator("github:mode-io/shared-audit")) + def test_github_repo_from_nested_locator(self) -> None: + self.assertEqual(github_repo_from_locator("github:vercel-labs/agent-browser/skills/agent-browser"), "vercel-labs/agent-browser") + + def test_github_folder_url_omits_repo_root(self) -> None: + self.assertIsNone(github_folder_url("mode-io/shared-audit", ref="main", relative_path=".")) + + def test_github_folder_url_uses_exact_relative_path(self) -> None: + self.assertEqual( + github_folder_url( + "vercel-labs/agent-browser", + ref="main", + relative_path="skills/agent-browser", + ), + "https://github.com/vercel-labs/agent-browser/tree/main/skills/agent-browser", + ) class FindSkillTests(unittest.TestCase): @@ -100,6 +119,18 @@ def local_fetch(locator: str, work_dir: Path) -> Path: self.assertIsNotNone(result) self.assertTrue((result / "SKILL.md").is_file()) + def test_resolved_skill_keeps_nested_repo_path(self) -> None: + resolved = ResolvedGitHubSkill( + repo="vercel-labs/agent-browser", + ref="main", + relative_path="skills/agent-browser", + package_path=Path("/tmp/agent-browser/skills/agent-browser"), + clone_dir=Path("/tmp/agent-browser"), + ) + + self.assertEqual(resolved.repo, "vercel-labs/agent-browser") + self.assertEqual(resolved.relative_path, "skills/agent-browser") + class SkillsShParsingTests(unittest.TestCase): def test_parse_homepage_leaderboard_reads_embedded_initial_skills_payload(self) -> None: html = """ diff --git a/tests/unit/test_store.py b/tests/unit/test_store.py index 4c98c0c..cf40429 100644 --- a/tests/unit/test_store.py +++ b/tests/unit/test_store.py @@ -20,6 +20,8 @@ def test_ingest_copies_package_and_updates_manifest(self) -> None: declared_name="Audit Skill", source_kind="centralized", source_locator="centralized:Audit Skill", + source_ref="main", + source_path_hint="skills/audit", ) self.assertTrue(dest.is_dir()) self.assertTrue((dest / "SKILL.md").is_file()) @@ -27,6 +29,8 @@ def test_ingest_copies_package_and_updates_manifest(self) -> None: self.assertEqual(len(manifest.entries), 1) self.assertEqual(manifest.entries[0].package_dir, "audit") self.assertEqual(manifest.entries[0].declared_name, "Audit Skill") + self.assertEqual(manifest.entries[0].source_ref, "main") + self.assertEqual(manifest.entries[0].source_path, "skills/audit") def test_ingest_refuses_existing_directory(self) -> None: with TemporaryDirectory() as temp_dir: @@ -66,12 +70,14 @@ def test_update_replaces_changed_package(self) -> None: source_v1 = seed_skill_package(Path(temp_dir) / "v1", "audit", "Audit", body="version 1") store.ingest(source_path=source_v1, declared_name="Audit", source_kind="github", source_locator="github:test/test/audit") source_v2 = seed_skill_package(Path(temp_dir) / "v2", "audit", "Audit", body="version 2") - _, changed = store.update("audit", source_path=source_v2) + _, changed = store.update("audit", source_path=source_v2, source_ref="main", source_path_hint="skills/audit") self.assertTrue(changed) content = (spec.shared_store_root / "audit" / "SKILL.md").read_text() self.assertIn("version 2", content) manifest = load_manifest(store.manifest_path) self.assertEqual(len(manifest.entries), 1) + self.assertEqual(manifest.entries[0].source_ref, "main") + self.assertEqual(manifest.entries[0].source_path, "skills/audit") def test_update_noop_when_identical(self) -> None: with TemporaryDirectory() as temp_dir: