From 76de6ec5a8c8d142a99a0cc2ff9e950d6817236c Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 7 Sep 2026 02:14:50 -0700 Subject: [PATCH] fix: retain ClawHub owner through version and bundle requests --- tests/tools/test_clawhub_owner_http.py | 90 ++++++++++++++++++++++++++ tests/tools/test_skills_hub_clawhub.py | 78 ---------------------- tools/skills_hub_clawhub.py | 36 ++++++++--- 3 files changed, 116 insertions(+), 88 deletions(-) create mode 100644 tests/tools/test_clawhub_owner_http.py diff --git a/tests/tools/test_clawhub_owner_http.py b/tests/tools/test_clawhub_owner_http.py new file mode 100644 index 0000000000..0a31f13b39 --- /dev/null +++ b/tests/tools/test_clawhub_owner_http.py @@ -0,0 +1,90 @@ +"""Owner-qualified ClawHub fetches retain identity across actual HTTP requests.""" + +import io +import json +import threading +import zipfile +from contextlib import contextmanager +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from urllib.parse import parse_qs, urlsplit + +from tools.skills_hub_clawhub import ClawHubSource + + +@contextmanager +def registry(*, fallback=False, mismatch=False): + requests = [] + + class Handler(BaseHTTPRequestHandler): + def log_message(self, *args): + pass + + def do_GET(self): + url = urlsplit(self.path) + query = parse_qs(url.query) + requests.append((url.path, query)) + status = 200 + if query.get("owner") != ["alice"]: + status, payload = 409, {"code": "AMBIGUOUS_SKILL_SLUG"} + elif url.path.endswith("/download"): + if fallback: + status, payload = 404, {} + else: + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w") as archive: + archive.writestr("SKILL.md", "# Alice fixture") + payload = buf.getvalue() + elif url.path.endswith("/versions"): + payload = [{"version": "1.0"}] + elif url.path.endswith("/versions/1.0"): + payload = {"files": {"SKILL.md": "# Alice fixture"}} + else: + # Explicit owner must survive even when metadata omits it. + payload = {"skill": {"slug": "collision"}} + if mismatch: + payload["owner"] = {"handle": "bob"} + body = payload if isinstance(payload, bytes) else json.dumps(payload).encode() + self.send_response(status) + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + server = ThreadingHTTPServer(("127.0.0.1", 0), Handler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + source = ClawHubSource() + source.BASE_URL = f"http://127.0.0.1:{server.server_port}/api/v1" + try: + yield source, requests + finally: + server.shutdown() + server.server_close() + thread.join(timeout=5) + + +def test_qualified_owner_survives_metadata_versions_and_download(): + for fallback in (False, True): + with registry(fallback=fallback) as (source, requests): + for identifier in ("@alice/collision", "clawhub/@alice/collision", "alice/skills/collision"): + meta = source.inspect(identifier) + assert meta is not None + assert meta.identifier == "@alice/collision" + bundle = source.fetch(identifier) + assert bundle is not None + assert bundle.identifier == "@alice/collision" + assert bundle.files == {"SKILL.md": "# Alice fixture"} + assert all(query.get("owner") == ["alice"] for _, query in requests) + assert any(path.endswith("/versions") for path, _ in requests) + assert any(path.endswith("/download") for path, _ in requests) + if fallback: + assert any(path.endswith("/versions/1.0") for path, _ in requests) + + +def test_ambiguous_or_mismatched_owner_never_downloads(): + with registry() as (source, requests): + assert source.fetch("collision") is None + assert source.fetch("github-owner/repository/collision") is None + assert len(requests) == 1 + with registry(mismatch=True) as (source, requests): + assert source.fetch("@alice/collision") is None + assert len(requests) == 1 diff --git a/tests/tools/test_skills_hub_clawhub.py b/tests/tools/test_skills_hub_clawhub.py index 4882df768b..20eee328de 100644 --- a/tests/tools/test_skills_hub_clawhub.py +++ b/tests/tools/test_skills_hub_clawhub.py @@ -431,84 +431,6 @@ class TestClawHubSource(unittest.TestCase): self.assertIsNone(meta) mock_get.assert_called_once() - @patch("tools.skills_hub.httpx.get") - def test_inspect_passes_owner_param_to_disambiguate_slug(self, mock_get): - """An @owner/slug identifier must reach the detail API as ?owner=. - - ClawHub answers a slug claimed by multiple owners with 409 - AMBIGUOUS_SKILL_SLUG on the bare detail GET (#104117); the disambiguated - form only resolves when the owner hint is forwarded as a query param.""" - mock_get.return_value = _MockResponse( - status_code=200, - json_data={ - "slug": "ponytail", - "displayName": "ponytail", - "summary": "Lazy senior dev mode", - "owner": {"handle": "dietrichgebert"}, - "latestVersion": {"version": "1.0.0"}, - }, - ) - - meta = self.src.inspect("clawhub/@dietrichgebert/ponytail") - - self.assertIsNotNone(meta) - self.assertEqual(meta.extra.get("owner"), "dietrichgebert") - args, kwargs = mock_get.call_args - self.assertTrue(args[0].endswith("/skills/ponytail")) - self.assertEqual(kwargs["params"], {"owner": "dietrichgebert"}) - - @patch("tools.skills_hub.httpx.get") - def test_inspect_bare_slug_omits_owner_param(self, mock_get): - """Bare slugs keep the plain detail GET — no owner to forward.""" - mock_get.return_value = _MockResponse( - status_code=200, - json_data={ - "slug": "caldav-calendar", - "displayName": "CalDAV Calendar", - "summary": "Calendar integration", - "latestVersion": {"version": "1.0.0"}, - }, - ) - - meta = self.src.inspect("caldav-calendar") - - self.assertIsNotNone(meta) - _, kwargs = mock_get.call_args - self.assertIsNone(kwargs.get("params")) - - @patch("tools.skills_hub._ssrf_safe_http_get") - @patch("tools.skills_hub.httpx.get") - def test_fetch_qualified_slug_resolves_ambiguous_slug_end_to_end(self, mock_get, mock_safe_get): - """install clawhub/@owner/slug must succeed for a multi-owner slug: - the detail GET carries ?owner=, so ClawHub never answers 409.""" - def side_effect(url, *args, **kwargs): - if url.endswith("/skills/ponytail"): - return _MockResponse( - status_code=200, - json_data={ - "slug": "ponytail", - "owner": {"handle": "dietrichgebert"}, - "latestVersion": {"version": "1.0.0"}, - }, - ) - if url.endswith("/skills/ponytail/versions/1.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 - mock_safe_get.return_value = _MockResponse(status_code=200, text="# Skill") - - bundle = self.src.fetch("@dietrichgebert/ponytail") - - self.assertIsNotNone(bundle) - self.assertEqual(bundle.name, "ponytail") - self.assertEqual(bundle.files["SKILL.md"], "# Skill") - detail_args, detail_kwargs = mock_get.call_args_list[0] - self.assertTrue(detail_args[0].endswith("/skills/ponytail")) - self.assertEqual(detail_kwargs["params"], {"owner": "dietrichgebert"}) class TestClawHubCatalogWalkBounded(unittest.TestCase): diff --git a/tools/skills_hub_clawhub.py b/tools/skills_hub_clawhub.py index 263bfc384d..c2d40b54f9 100644 --- a/tools/skills_hub_clawhub.py +++ b/tools/skills_hub_clawhub.py @@ -235,17 +235,22 @@ class ClawHubSource(GuardedFetchMixin, SkillSource): if detail is None: return None slug, skill_data = detail - latest_version = self._resolve_latest_version(slug, skill_data) + # Keep the requested identity even when the detail payload omits its owner. + owner = self._parse_identifier(identifier)[1] or self._owner_from_payload(skill_data) + owner_params = {"owner": owner} if owner else None + latest_version = self._resolve_latest_version(slug, skill_data, owner=owner) if not latest_version: logger.warning("ClawHub fetch failed for %s: could not resolve latest version", slug) return None # Primary: ZIP bundle from /download. Fallback: version metadata with # inline/raw content (files may sit under version_data["version"]). - files = self._download_zip(slug, latest_version) + files = self._download_zip(slug, latest_version, owner=owner) if "SKILL.md" not in files: - version_data = self._get_json(f"{self.BASE_URL}/skills/{slug}/versions/{latest_version}") - if isinstance(version_data, dict): + version_data = self._get_json( + f"{self.BASE_URL}/skills/{slug}/versions/{latest_version}", params=owner_params, + ) + if isinstance(version_data, dict) and self._owner_matches(owner, version_data): files = self._extract_files(version_data) or files nested = version_data.get("version", {}) if "SKILL.md" not in files and isinstance(nested, dict): @@ -255,14 +260,20 @@ class ClawHubSource(GuardedFetchMixin, SkillSource): "ClawHub fetch for %s resolved version %s but could not retrieve file content", slug, latest_version, ) return None - return SkillBundle(name=slug, files=files, source="clawhub", identifier=slug, trust_level="community") + return SkillBundle(name=slug, files=files, source="clawhub", + identifier=f"@{owner}/{slug}" if owner else slug, trust_level="community") def inspect(self, identifier: str) -> Optional[SkillMeta]: detail = self._skill_detail(identifier) if detail is None: return None slug, data = detail - return self._item_to_meta({**data, "slug": data.get("slug") or slug}) + meta = self._item_to_meta({**data, "slug": data.get("slug") or slug}) + owner = self._parse_identifier(identifier)[1] or self._owner_from_payload(data) + if meta is not None and owner: + meta.identifier = f"@{owner}/{slug}" + meta.extra["owner"] = owner + return meta def _search_catalog(self, query: str, limit: int = 10) -> List[SkillMeta]: cache_key = f"clawhub_search_catalog_v1_{hashlib.md5(f'{query}|{limit}'.encode()).hexdigest()}" @@ -325,7 +336,8 @@ class ClawHubSource(GuardedFetchMixin, SkillSource): _cache_metas(cache_key, results) return results - def _resolve_latest_version(self, slug: str, skill_data: Dict[str, Any]) -> Optional[str]: + def _resolve_latest_version(self, slug: str, skill_data: Dict[str, Any], + owner: Optional[str] = None) -> Optional[str]: latest, tags = skill_data.get("latestVersion"), skill_data.get("tags") version = _first_str( latest.get("version") if isinstance(latest, dict) else None, @@ -333,7 +345,8 @@ class ClawHubSource(GuardedFetchMixin, SkillSource): ) if version: return version - vd = self._get_json(f"{self.BASE_URL}/skills/{slug}/versions") + vd = self._get_json(f"{self.BASE_URL}/skills/{slug}/versions", + params={"owner": owner} if owner else None) return _first_str(vd[0].get("version")) if isinstance(vd, list) and vd and isinstance(vd[0], dict) else None def _fetch_owner_handle(self, slug: str) -> Optional[str]: @@ -439,16 +452,19 @@ class ClawHubSource(GuardedFetchMixin, SkillSource): files[fname] = content return files - def _download_zip(self, slug: str, version: str) -> Dict[str, str]: + def _download_zip(self, slug: str, version: str, owner: Optional[str] = None) -> Dict[str, str]: """Download the skill ZIP from /download and extract its text files.""" import io import zipfile files: Dict[str, str] = {} + params = {"slug": slug, "version": version} + if owner: + params["owner"] = owner max_retries = 3 for attempt in range(max_retries): try: - resp = httpx.get(f"{self.BASE_URL}/download", params={"slug": slug, "version": version}, + resp = httpx.get(f"{self.BASE_URL}/download", params=params, timeout=30, follow_redirects=True) if resp.status_code == 429: try: