diff --git a/scripts/build_skills_index.py b/scripts/build_skills_index.py index 69f9fbf0dd..5eb60f8bf6 100644 --- a/scripts/build_skills_index.py +++ b/scripts/build_skills_index.py @@ -295,6 +295,43 @@ def main(): # Batch resolve GitHub paths for skills.sh entries all_skills = batch_resolve_paths(all_skills, auth) + # Enrich ClawHub skills with owner handles. The listing API does not + # include owner info, so we fetch each skill's detail page concurrently. + # This is needed to build valid "View source" URLs on the Skills Hub page: + # https://clawhub.ai/{owner}/skills/{slug}. Without the owner segment the + # URL leads to a 404. + clawhub_skills = [s for s in all_skills if s["source"] == "clawhub"] + if clawhub_skills: + # Convert dicts back to SkillMeta for enrichment, then update in place. + clawhub_metas = [] + for s in clawhub_skills: + meta = SkillMeta( + name=s["name"], + description=s["description"], + source=s["source"], + identifier=s["identifier"], + trust_level=s["trust_level"], + repo=s.get("repo") or None, + path=s.get("path") or None, + tags=s.get("tags") or [], + extra=dict(s.get("extra") or {}), + ) + clawhub_metas.append(meta) + + print(f" Enriching {len(clawhub_metas)} ClawHub skills with owner handles...", + flush=True) + enrich_start = time.time() + enriched = sources["clawhub"].enrich_owners(clawhub_metas, max_workers=30) + # Write enriched owner back into the index dicts. + meta_by_id = {m.identifier: m for m in clawhub_metas} + for s in clawhub_skills: + meta = meta_by_id.get(s["identifier"]) + if meta and meta.extra.get("owner"): + s.setdefault("extra", {})["owner"] = meta.extra["owner"] + enrich_elapsed = time.time() - enrich_start + print(f" Enriched {enriched}/{len(clawhub_metas)} ClawHub owners " + f"({enrich_elapsed:.1f}s)", flush=True) + # Collect which sources hit a GitHub API rate limit during the crawl. # github / well-known both read api.github.com, so a rate-limited token # zeroes both at once — surfaced below so the failure message names the diff --git a/tests/scripts/test_build_skills_index_health.py b/tests/scripts/test_build_skills_index_health.py index 75e6589340..dfb58b8f7a 100644 --- a/tests/scripts/test_build_skills_index_health.py +++ b/tests/scripts/test_build_skills_index_health.py @@ -39,6 +39,10 @@ class _FakeSource: def search(self, query, limit=10): return [_meta(f"{self._src}-{i}", self._src) for i in range(self._n)] + def enrich_owners(self, skills, max_workers=30): + # No-op: fake source doesn't need owner enrichment. + return 0 + def _install_fake_sources(monkeypatch, *, github_count, well_known_count=10, github_rate_limited=False): diff --git a/tests/tools/test_skills_hub_clawhub.py b/tests/tools/test_skills_hub_clawhub.py index 6918188827..9e6d306dc5 100644 --- a/tests/tools/test_skills_hub_clawhub.py +++ b/tests/tools/test_skills_hub_clawhub.py @@ -89,6 +89,125 @@ class TestClawHubSource(unittest.TestCase): self.assertEqual(meta.description, "Calendar integration") self.assertEqual(meta.identifier, "caldav-calendar") + @patch("tools.skills_hub.httpx.get") + def test_inspect_handles_nested_skill_payload(self, mock_get): + mock_get.return_value = _MockResponse( + status_code=200, + json_data={ + "skill": { + "slug": "self-improving-agent", + "displayName": "self-improving-agent", + "summary": "Captures learnings and errors for continuous improvement.", + "tags": {"latest": "3.0.2", "automation": "3.0.2"}, + }, + "latestVersion": {"version": "3.0.2"}, + }, + ) + + meta = self.src.inspect("self-improving-agent") + + self.assertIsNotNone(meta) + self.assertEqual(meta.name, "self-improving-agent") + self.assertIn("continuous improvement", meta.description) + self.assertEqual(meta.identifier, "self-improving-agent") + self.assertEqual(meta.tags, ["automation"]) + + @patch("tools.skills_hub._ssrf_safe_http_get") + @patch("tools.skills_hub.httpx.get") + def test_inspect_captures_owner_from_detail_api(self, mock_get, mock_safe_get): + """inspect() fetches the detail API which includes owner — capture it.""" + mock_get.return_value = _MockResponse( + status_code=200, + json_data={ + "skill": { + "slug": "apple-docs", + "displayName": "Apple Docs", + "summary": "Documentation reader", + "tags": {"latest": "1.0.0"}, + }, + "latestVersion": {"version": "1.0.0"}, + "owner": {"handle": "thesethrose", "displayName": "Seth Rose"}, + }, + ) + + meta = self.src.inspect("apple-docs") + + self.assertIsNotNone(meta) + self.assertEqual(meta.extra.get("owner"), "thesethrose") + + @patch("tools.skills_hub.httpx.get") + def test_inspect_tolerates_missing_owner(self, mock_get): + """inspect() should still work when the API omits owner.""" + mock_get.return_value = _MockResponse( + status_code=200, + json_data={ + "skill": { + "slug": "some-skill", + "displayName": "Some Skill", + "summary": "A skill", + "tags": {"latest": "1.0.0"}, + }, + "latestVersion": {"version": "1.0.0"}, + }, + ) + + meta = self.src.inspect("some-skill") + + self.assertIsNotNone(meta) + self.assertNotIn("owner", meta.extra or {}) + + @patch("tools.skills_hub._ssrf_safe_http_get") + @patch("tools.skills_hub.httpx.get") + def test_fetch_resolves_latest_version_and_downloads_raw_files(self, mock_get, mock_safe_get): + def side_effect(url, *args, **kwargs): + if url.endswith("/skills/caldav-calendar"): + return _MockResponse( + status_code=200, + json_data={ + "slug": "caldav-calendar", + "latestVersion": {"version": "1.0.1"}, + }, + ) + if url.endswith("/skills/caldav-calendar/versions/1.0.1"): + return _MockResponse( + status_code=200, + json_data={ + "files": [ + {"path": "SKILL.md", "rawUrl": "https://files.example/skill-md"}, + {"path": "README.md", "content": "hello"}, + ] + }, + ) + return _MockResponse(status_code=404, json_data={}) + + mock_get.side_effect = side_effect + mock_safe_get.return_value = _MockResponse(status_code=200, text="# Skill") + + bundle = self.src.fetch("caldav-calendar") + + self.assertIsNotNone(bundle) + self.assertEqual(bundle.name, "caldav-calendar") + self.assertIn("SKILL.md", bundle.files) + self.assertEqual(bundle.files["SKILL.md"], "# Skill") + self.assertEqual(bundle.files["README.md"], "hello") + mock_safe_get.assert_called_once_with("https://files.example/skill-md", timeout=20) + + @patch("tools.skills_hub.httpx.get") + def test_fetch_falls_back_to_versions_list(self, mock_get): + def side_effect(url, *args, **kwargs): + if url.endswith("/skills/caldav-calendar"): + return _MockResponse(status_code=200, json_data={"slug": "caldav-calendar"}) + if url.endswith("/skills/caldav-calendar/versions"): + return _MockResponse(status_code=200, json_data=[{"version": "2.0.0"}]) + if url.endswith("/skills/caldav-calendar/versions/2.0.0"): + return _MockResponse(status_code=200, json_data={"files": {"SKILL.md": "# Skill"}}) + return _MockResponse(status_code=404, json_data={}) + + mock_get.side_effect = side_effect + + bundle = self.src.fetch("caldav-calendar") + self.assertIsNotNone(bundle) + self.assertEqual(bundle.files["SKILL.md"], "# Skill") @patch("tools.skills_hub.check_website_access", return_value=None) @patch("tools.skills_hub.is_safe_url") @@ -345,5 +464,161 @@ class TestClawHubCatalogWalkBounded(unittest.TestCase): self.assertLess(page_calls["n"], 30) +class TestFetchOwnerHandleRetry(unittest.TestCase): + """Verify _fetch_owner_handle() retry/backoff on rate-limit and transient errors.""" + + def setUp(self): + self.src = ClawHubSource() + + @patch("tools.skills_hub.time.sleep") + @patch("tools.skills_hub.httpx.get") + def test_retries_on_429_with_retry_after_header(self, mock_get, mock_sleep): + """On 429, _fetch_owner_handle retries and honours Retry-After.""" + mock_get.side_effect = [ + _MockResponse(status_code=429, headers={"Retry-After": "3"}), + _MockResponse( + status_code=200, + json_data={"skill": {"slug": "test"}, "owner": {"handle": "alice"}}, + ), + ] + + handle = self.src._fetch_owner_handle("test") + + self.assertEqual(handle, "alice") + self.assertEqual(mock_get.call_count, 2) + mock_sleep.assert_called_once_with(3.0) + + @patch("tools.skills_hub.time.sleep") + @patch("tools.skills_hub.httpx.get") + def test_retries_on_429_without_retry_after_uses_exponential_backoff(self, mock_get, mock_sleep): + """On 429 without Retry-After, use exponential backoff (2s, 4s).""" + mock_get.side_effect = [ + _MockResponse(status_code=429, headers={}), + _MockResponse(status_code=429, headers={}), + _MockResponse( + status_code=200, + json_data={"skill": {"slug": "test"}, "owner": {"handle": "bob"}}, + ), + ] + + handle = self.src._fetch_owner_handle("test") + + self.assertEqual(handle, "bob") + self.assertEqual(mock_get.call_count, 3) + self.assertEqual(mock_sleep.call_count, 2) + mock_sleep.assert_any_call(2.0) # first backoff: 2s + mock_sleep.assert_any_call(4.0) # second backoff: 4s + + @patch("tools.skills_hub.time.sleep") + @patch("tools.skills_hub.httpx.get") + def test_gives_up_after_max_attempts_on_429(self, mock_get, mock_sleep): + """After 3 attempts all 429, return None (no more retries).""" + mock_get.return_value = _MockResponse(status_code=429, headers={}) + + handle = self.src._fetch_owner_handle("test") + + self.assertIsNone(handle) + self.assertEqual(mock_get.call_count, 3) + self.assertEqual(mock_sleep.call_count, 2) # sleeps between attempts, not after last + + @patch("tools.skills_hub.time.sleep") + @patch("tools.skills_hub.httpx.get") + def test_retries_on_5xx_transient_error(self, mock_get, mock_sleep): + """On 500/502/503, retries with exponential backoff.""" + mock_get.side_effect = [ + _MockResponse(status_code=503, headers={}), + _MockResponse( + status_code=200, + json_data={"skill": {"slug": "test"}, "owner": {"handle": "carol"}}, + ), + ] + + handle = self.src._fetch_owner_handle("test") + + self.assertEqual(handle, "carol") + self.assertEqual(mock_get.call_count, 2) + mock_sleep.assert_called_once_with(2.0) + + @patch("tools.skills_hub.time.sleep") + @patch("tools.skills_hub.httpx.get") + def test_does_not_retry_on_4xx_non_429(self, mock_get, mock_sleep): + """4xx (not 429) means the resource doesn't exist — no retry.""" + mock_get.return_value = _MockResponse(status_code=404, headers={}) + + handle = self.src._fetch_owner_handle("test") + + self.assertIsNone(handle) + self.assertEqual(mock_get.call_count, 1) + mock_sleep.assert_not_called() + + @patch("tools.skills_hub.time.sleep") + @patch("tools.skills_hub.httpx.get") + def test_retries_on_transport_error(self, mock_get, mock_sleep): + """Network/transport errors (httpx.HTTPError) trigger retry with backoff.""" + import httpx + mock_get.side_effect = [ + httpx.ConnectError("connection refused"), + _MockResponse( + status_code=200, + json_data={"skill": {"slug": "test"}, "owner": {"handle": "dave"}}, + ), + ] + + handle = self.src._fetch_owner_handle("test") + + self.assertEqual(handle, "dave") + self.assertEqual(mock_get.call_count, 2) + mock_sleep.assert_called_once_with(2.0) + + @patch("tools.skills_hub.time.sleep") + @patch("tools.skills_hub.httpx.get") + def test_enrich_owners_survives_burst_429_then_succeeds(self, mock_get, mock_sleep): + """enrich_owners() should not abort when a burst of 429s is followed by success. + + Regression test: the '50 consecutive failures' safety rail previously + fired immediately under rate-limiting because _fetch_owner_handle() did + no retry. With retry, transient 429s are absorbed per-request. + """ + # Each skill: 1st attempt 429 → retry → 200 with owner. + # call_count tracks attempts across all skills. + call_count = {"n": 0} + + def side_effect(url, *args, **kwargs): + call_count["n"] += 1 + # Odd calls → 429, even calls → 200 with data + if call_count["n"] % 2 == 1: + return _MockResponse(status_code=429, headers={"Retry-After": "0"}) + return _MockResponse( + status_code=200, + json_data={"skill": {"slug": "s"}, "owner": {"handle": "eve"}}, + ) + + mock_get.side_effect = side_effect + + skills = [ + SkillMeta( + name="s1", description="", source="clawhub", + identifier="s1", trust_level="community", + ), + SkillMeta( + name="s2", description="", source="clawhub", + identifier="s2", trust_level="community", + ), + SkillMeta( + name="s3", description="", source="clawhub", + identifier="s3", trust_level="community", + ), + ] + + enriched = self.src.enrich_owners(skills, max_workers=1) + + self.assertEqual(enriched, 3) + for s in skills: + self.assertEqual(s.extra.get("owner"), "eve") + # 3 skills × 2 attempts each = 6 total HTTP calls (no abort) + self.assertEqual(call_count["n"], 6) + mock_sleep.assert_called() + + if __name__ == "__main__": unittest.main() diff --git a/tests/website/test_extract_skills.py b/tests/website/test_extract_skills.py index 307d1f4445..0cb2c20281 100644 --- a/tests/website/test_extract_skills.py +++ b/tests/website/test_extract_skills.py @@ -65,14 +65,32 @@ def test_source_url_synthesizes_github_root_when_no_subpath(mod): def test_source_url_synthesizes_clawhub(mod): - assert mod._source_url("clawhub", "go-music-skill", {}) == "https://clawhub.ai/skills/go-music-skill" + # ClawHub URLs require the owner handle; without it we cannot build a + # valid URL, so the result is "" (better than a broken 404 link). + assert mod._source_url("clawhub", "go-music-skill", {}) == "" def test_source_url_synthesizes_clawhub_strips_prefix(mod): # identifier may arrive already prefixed; we must not double-prefix. assert ( mod._source_url("clawhub", "clawhub/go-music-skill", {}) - == "https://clawhub.ai/skills/go-music-skill" + == "" + ) + + +def test_source_url_synthesizes_clawhub_with_owner(mod): + # When the owner handle is available in extra, the URL includes it. + assert ( + mod._source_url("clawhub", "go-music-skill", {"owner": "somepublisher"}) + == "https://clawhub.ai/somepublisher/skills/go-music-skill" + ) + + +def test_source_url_synthesizes_clawhub_with_owner_strips_prefix(mod): + # Owner + prefixed identifier: prefix is stripped, owner is used. + assert ( + mod._source_url("clawhub", "clawhub/go-music-skill", {"owner": "somepublisher"}) + == "https://clawhub.ai/somepublisher/skills/go-music-skill" ) diff --git a/tools/skills_hub.py b/tools/skills_hub.py index 34c12fb3cf..5f979eac8f 100644 --- a/tools/skills_hub.py +++ b/tools/skills_hub.py @@ -2221,6 +2221,10 @@ class ClawHubSource(SkillSource): latest_version = data.get("latestVersion") if latest_version is not None and "latestVersion" not in merged: merged["latestVersion"] = latest_version + # Carry over top-level fields that the listing API nests alongside + # the skill object — owner is needed for building valid detail URLs. + if "owner" in data and "owner" not in merged: + merged["owner"] = data["owner"] return merged return data @@ -2409,6 +2413,14 @@ class ClawHubSource(SkillSource): display_name = item.get("displayName") or item.get("name") or slug summary = item.get("summary") or item.get("description") or "" tags = self._normalize_tags(item.get("tags", [])) + extra: Dict[str, Any] = {} + owner = item.get("owner") + if isinstance(owner, dict): + handle = owner.get("handle") + if isinstance(handle, str) and handle: + extra["owner"] = handle + elif isinstance(owner, str) and owner: + extra["owner"] = owner results.append(SkillMeta( name=display_name, description=summary, @@ -2416,6 +2428,7 @@ class ClawHubSource(SkillSource): identifier=slug, trust_level="community", tags=tags, + extra=extra, )) final_results = self._finalize_search_results(query, results, limit) @@ -2471,6 +2484,16 @@ class ClawHubSource(SkillSource): return None tags = self._normalize_tags(data.get("tags", [])) + extra: Dict[str, Any] = {} + # The detail API returns owner info — capture it so callers can build + # valid ClawHub URLs (https://clawhub.ai/{owner}/skills/{slug}). + owner = data.get("owner") + if isinstance(owner, dict): + handle = owner.get("handle") + if isinstance(handle, str) and handle: + extra["owner"] = handle + elif isinstance(owner, str) and owner: + extra["owner"] = owner return SkillMeta( name=data.get("displayName") or data.get("name") or data.get("slug") or slug, @@ -2479,6 +2502,7 @@ class ClawHubSource(SkillSource): identifier=data.get("slug") or slug, trust_level="community", tags=tags, + extra=extra, ) def _search_catalog(self, query: str, limit: int = 10) -> List[SkillMeta]: @@ -2564,6 +2588,16 @@ class ClawHubSource(SkillSource): display_name = item.get("displayName") or item.get("name") or slug summary = item.get("summary") or item.get("description") or "" tags = self._normalize_tags(item.get("tags", [])) + extra: Dict[str, Any] = {} + # The listing API may include owner info (handle) in future; + # capture it if present so we can build valid detail URLs. + owner = item.get("owner") + if isinstance(owner, dict): + handle = owner.get("handle") + if isinstance(handle, str) and handle: + extra["owner"] = handle + elif isinstance(owner, str) and owner: + extra["owner"] = owner results.append(SkillMeta( name=display_name, description=summary, @@ -2571,6 +2605,7 @@ class ClawHubSource(SkillSource): identifier=slug, trust_level="community", tags=tags, + extra=extra, )) cursor = data.get("nextCursor") if isinstance(data, dict) else None @@ -2623,6 +2658,170 @@ class ClawHubSource(SkillSource): return version return None + def _fetch_owner_handle(self, slug: str) -> Optional[str]: + """Fetch the owner handle for a single ClawHub skill via the detail API. + + Returns the owner handle string, or None if unavailable. + The detail endpoint at ``/api/v1/skills/{slug}`` returns an ``owner`` + object with a ``handle`` field — the listing API does not include this. + + Retry semantics (bounded): + - Up to 3 attempts total (initial + 2 retries). + - On HTTP 429: respects ``Retry-After`` header (seconds) when present, + otherwise exponential backoff (2s → 4s). + - On HTTP 5xx: exponential backoff (transient server errors). + - On HTTP 4xx (non-429): no retry — the resource doesn't exist. + """ + url = f"{self.BASE_URL}/skills/{slug}" + max_attempts = 3 + backoff_base = 2.0 # seconds + + for attempt in range(max_attempts): + try: + resp = httpx.get(url, timeout=20) + except (httpx.HTTPError, OSError): + # Network/transport error — treat as transient, retry with backoff. + if attempt < max_attempts - 1: + delay = backoff_base * (2 ** attempt) + logger.debug( + "_fetch_owner_handle(%s): transport error on attempt %d/%d, " + "retrying in %.1fs", + slug, attempt + 1, max_attempts, delay, + ) + time.sleep(delay) + continue + return None + + if resp.status_code == 200: + try: + raw = resp.json() + except (json.JSONDecodeError, ValueError): + return None + data = self._coerce_skill_payload(raw) + if not isinstance(data, dict): + return None + owner = data.get("owner") + if isinstance(owner, dict): + handle = owner.get("handle") + if isinstance(handle, str) and handle: + return handle + if isinstance(owner, str) and owner: + return owner + return None + + if resp.status_code == 429: + # Rate-limited — honour Retry-After if present, else backoff. + if attempt < max_attempts - 1: + retry_after_raw = resp.headers.get("Retry-After") + try: + delay = float(retry_after_raw) if retry_after_raw else backoff_base * (2 ** attempt) + except (TypeError, ValueError): + delay = backoff_base * (2 ** attempt) + logger.debug( + "_fetch_owner_handle(%s): HTTP 429 on attempt %d/%d, " + "retrying in %.1fs", + slug, attempt + 1, max_attempts, delay, + ) + time.sleep(delay) + continue + return None + + if 500 <= resp.status_code < 600: + # Transient server error — retry with backoff. + if attempt < max_attempts - 1: + delay = backoff_base * (2 ** attempt) + logger.debug( + "_fetch_owner_handle(%s): HTTP %d on attempt %d/%d, " + "retrying in %.1fs", + slug, resp.status_code, attempt + 1, max_attempts, delay, + ) + time.sleep(delay) + continue + return None + + # 4xx (non-429) — resource doesn't exist / bad request. No retry. + return None + + return None + + def enrich_owners(self, skills: List[SkillMeta], max_workers: int = 30) -> int: + """Batch-fetch owner handles for ClawHub skills missing ``extra["owner"]``. + + Mutates each SkillMeta in-place, setting ``extra["owner"]`` when the + detail API returns a handle. Returns the number of skills enriched. + + This is intended for the offline index builder, which walks the full + 50k+ catalog. The listing API does not include owner info, so we + fetch each skill's detail page concurrently. With ``max_workers=30`` + the full catalog takes ~5–10 minutes — acceptable for a twice-daily + batch job. + + Safety rails: + - Aborts early if 50 consecutive requests all fail (systemic outage). + - Respects HTTP 429 rate-limit responses with exponential backoff. + - Logs progress every 1000 skills so the batch job is observable. + """ + needs_enrichment = [ + s for s in skills + if s.source == "clawhub" and not (s.extra or {}).get("owner") + ] + if not needs_enrichment: + return 0 + + enriched = 0 + consecutive_failures = 0 + max_consecutive_failures = 50 + processed = 0 + import threading + lock = threading.Lock() + + def _fetch(meta: SkillMeta) -> Optional[str]: + return self._fetch_owner_handle(meta.identifier) + + from concurrent.futures import ThreadPoolExecutor, as_completed + with ThreadPoolExecutor(max_workers=max_workers) as pool: + futures = {pool.submit(_fetch, s): s for s in needs_enrichment} + for future in as_completed(futures): + meta = futures[future] + processed += 1 + try: + handle = future.result() + if handle: + with lock: + if not meta.extra: + meta.extra = {} + meta.extra["owner"] = handle + enriched += 1 + consecutive_failures = 0 + else: + with lock: + consecutive_failures += 1 + except Exception: + with lock: + consecutive_failures += 1 + + if processed % 1000 == 0: + logger.info( + "ClawHub owner enrichment: %d/%d processed, %d enriched", + processed, len(needs_enrichment), enriched, + ) + + with lock: + if consecutive_failures >= max_consecutive_failures: + logger.warning( + "ClawHub owner enrichment: %d consecutive failures — " + "aborting early (%d/%d processed, %d enriched). " + "The ClawHub API may be down or rate-limited.", + max_consecutive_failures, processed, + len(needs_enrichment), enriched, + ) + # Cancel pending futures + for f in futures: + f.cancel() + break + + return enriched + def _extract_files(self, version_data: Dict[str, Any]) -> Dict[str, str]: files: Dict[str, str] = {} file_list = version_data.get("files") diff --git a/website/scripts/extract-skills.py b/website/scripts/extract-skills.py index b77732f83c..a463d1e258 100644 --- a/website/scripts/extract-skills.py +++ b/website/scripts/extract-skills.py @@ -221,9 +221,15 @@ def _source_url(source: str, identifier: str, extra: dict) -> str: return "" if src == "clawhub": - # identifier is a bare slug (the "clawhub/" prefix is added at install time) + # identifier is a bare slug (the "clawhub/" prefix is added at install time). + # ClawHub URLs require the owner handle: https://clawhub.ai/{owner}/skills/{slug}. + # Without the owner we cannot build a valid URL — return "" rather than + # a broken link (the card will simply omit the "View source" button). slug = identifier[len("clawhub/"):] if identifier.startswith("clawhub/") else identifier - return f"https://clawhub.ai/skills/{slug}" + owner = extra.get("owner", "") if isinstance(extra, dict) else "" + if owner: + return f"https://clawhub.ai/{owner}/skills/{slug}" + return "" if src in {"skills.sh", "skills-sh"}: # "skills-sh/owner/repo/skill" -> the skills.sh detail page