From 6e3ccd1824df65f36b9ed998718a7d8a6ee7edbf Mon Sep 17 00:00:00 2001 From: ciregenz Date: Fri, 19 Jun 2026 04:08:53 -0700 Subject: [PATCH] [eric] skills: harden skills.sh resolver, optional GitHub token (60->5000/hr), real default-branch fallback when main/master miss, deterministic skill-path tie-break --- backend/apps/skill_registry/skill_registry.py | 70 ++++++++++++++----- .../tests/test_skill_registry_community.py | 27 +++++++ 2 files changed, 80 insertions(+), 17 deletions(-) diff --git a/backend/apps/skill_registry/skill_registry.py b/backend/apps/skill_registry/skill_registry.py index 946b65ef..ab5ef5a5 100644 --- a/backend/apps/skill_registry/skill_registry.py +++ b/backend/apps/skill_registry/skill_registry.py @@ -304,15 +304,36 @@ def _is_script_path(rel: str) -> bool: return head in ("scripts", "bin", "hooks") +def _github_headers() -> dict: + """GitHub request headers, with auth if a token is set. Unauthenticated is + 60 req/hr/IP (fine for the odd install, the wall for a power user); a token + (OPENSWARM_GITHUB_TOKEN or GITHUB_TOKEN) raises it to 5000/hr.""" + headers = {"User-Agent": "openswarm-skill-registry", "Accept": "application/vnd.github+json"} + token = os.environ.get("OPENSWARM_GITHUB_TOKEN") or os.environ.get("GITHUB_TOKEN") + if token: + headers["Authorization"] = f"Bearer {token}" + return headers + + def _select_skill_paths(tree: list[dict], skill_id: str) -> tuple[str, list[str]]: - """From a GitHub recursive tree, pick the SKILL.md for `skill_id` (shortest - matching path) and every file living beside it. Pure so the resolution logic - is unit-tested without a network round-trip.""" + """From a GitHub recursive tree, pick the SKILL.md for `skill_id` and every + file beside it. Pure, so the resolution logic is unit-tested without a network + round-trip. When a repo has several `//SKILL.md` matches the pick + is deterministic: prefer a top-level `/`, then `skills//`, + then the shallowest, then alphabetical, never an arbitrary tie.""" blobs = [t["path"] for t in tree if t.get("type") == "blob" and isinstance(t.get("path"), str)] candidates = [p for p in blobs if p.endswith(f"/{skill_id}/SKILL.md") or p == f"{skill_id}/SKILL.md"] if not candidates: raise ValueError(f"no SKILL.md for '{skill_id}' in this repo") - skill_md = min(candidates, key=len) + + def _rank(p: str) -> tuple: + if p == f"{skill_id}/SKILL.md": + return (0, 0, p) + if p == f"skills/{skill_id}/SKILL.md": + return (1, p.count("/"), p) + return (2, p.count("/"), p) + + skill_md = min(candidates, key=_rank) skill_dir = skill_md[: -len("/SKILL.md")] if "/" in skill_md else "" prefix = (skill_dir + "/") if skill_dir else "" members = [p for p in blobs if (p.startswith(prefix) if prefix else "/" not in p)] @@ -324,19 +345,35 @@ class RegistryRateLimited(Exception): 'try again shortly' rather than a generic failure.""" +async def _tree_at(client: httpx.AsyncClient, owner: str, repo: str, branch: str): + """(tree | None) for a branch. None on 404 (branch absent); raises on 403.""" + r = await client.get(f"{_GH_API}/repos/{owner}/{repo}/git/trees/{branch}?recursive=1") + if r.status_code == 200: + return r.json().get("tree", []) + if r.status_code == 403: + raise RegistryRateLimited() + return None + + async def _fetch_repo_tree(client: httpx.AsyncClient, owner: str, repo: str) -> tuple[str, list[dict]]: - """Recursive tree of owner/repo, trying main then master (one API call each, - usually just one). Avoids a separate repo-meta call to halve GitHub API use. - Raises RegistryRateLimited on a 403, ValueError if no usable branch.""" - last_status = None + """Recursive tree of owner/repo. Tries main then master first (one call, the + 99% case, no quota wasted on a repo-meta lookup); only if BOTH are absent + does it ask the repo for its real default branch (handles develop/trunk/etc). + Raises RegistryRateLimited on a 403, ValueError if no branch resolves.""" for branch in ("main", "master"): - r = await client.get(f"{_GH_API}/repos/{owner}/{repo}/git/trees/{branch}?recursive=1") - if r.status_code == 200: - return branch, r.json().get("tree", []) - if r.status_code == 403: - raise RegistryRateLimited() - last_status = r.status_code - raise ValueError(f"repo {owner}/{repo} has no main/master branch (last status {last_status})") + tree = await _tree_at(client, owner, repo, branch) + if tree is not None: + return branch, tree + meta = await client.get(f"{_GH_API}/repos/{owner}/{repo}") + if meta.status_code == 403: + raise RegistryRateLimited() + if meta.status_code == 200: + default = meta.json().get("default_branch") + if default and default not in ("main", "master"): + tree = await _tree_at(client, owner, repo, default) + if tree is not None: + return default, tree + raise ValueError(f"repo {owner}/{repo} has no resolvable default branch") async def resolve_community_skill(source: str, skill_id: str) -> dict: @@ -348,8 +385,7 @@ async def resolve_community_skill(source: str, skill_id: str) -> dict: owner, _, repo = source.partition("/") if not owner or not repo: raise ValueError(f"unrecognized source '{source}' (expected owner/repo)") - headers = {"User-Agent": "openswarm-skill-registry", "Accept": "application/vnd.github+json"} - async with httpx.AsyncClient(timeout=30.0, headers=headers) as client: + async with httpx.AsyncClient(timeout=30.0, headers=_github_headers()) as client: branch, tree = await _fetch_repo_tree(client, owner, repo) skill_md, members = _select_skill_paths(tree, skill_id) skill_dir = skill_md[: -len("/SKILL.md")] if "/" in skill_md else "" diff --git a/backend/tests/test_skill_registry_community.py b/backend/tests/test_skill_registry_community.py index 945f17b0..c7fa3900 100644 --- a/backend/tests/test_skill_registry_community.py +++ b/backend/tests/test_skill_registry_community.py @@ -47,6 +47,33 @@ def test_missing_skill_raises(): _select_skill_paths([{"type": "blob", "path": "a/SKILL.md"}], "nonexistent") +def test_ambiguous_match_picks_deterministically(): + # Several /pdf/SKILL.md: a top-level pdf/ wins, else skills/pdf/, never arbitrary. + tree = [ + {"type": "blob", "path": "plugins/z/pdf/SKILL.md"}, + {"type": "blob", "path": "skills/pdf/SKILL.md"}, + {"type": "blob", "path": "pdf/SKILL.md"}, + ] + skill_md, _ = _select_skill_paths(tree, "pdf") + assert skill_md == "pdf/SKILL.md" + # Without a top-level one, prefer skills//. + tree2 = [ + {"type": "blob", "path": "plugins/z/pdf/SKILL.md"}, + {"type": "blob", "path": "skills/pdf/SKILL.md"}, + ] + skill_md2, _ = _select_skill_paths(tree2, "pdf") + assert skill_md2 == "skills/pdf/SKILL.md" + + +def test_github_headers_adds_token_when_set(monkeypatch): + from backend.apps.skill_registry.skill_registry import _github_headers + monkeypatch.delenv("OPENSWARM_GITHUB_TOKEN", raising=False) + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + assert "Authorization" not in _github_headers() + monkeypatch.setenv("OPENSWARM_GITHUB_TOKEN", "ghp_test") + assert _github_headers()["Authorization"] == "Bearer ghp_test" + + def test_install_disclosure_flags_secret_shaped_files(): # The scan we wire into the install disclosure (reused from the .swarm importer) # must flag a community skill shipping credentials, and leave clean files alone.