From f986a2b103df11301e1a5a6b3672dcbce1572f67 Mon Sep 17 00:00:00 2001 From: liuhao1024 Date: Sun, 6 Sep 2026 15:36:50 +0800 Subject: [PATCH] fix(skills): pass the ClawHub owner hint as ?owner= so ambiguous slugs resolve ClawHub's detail endpoint now answers a slug claimed by multiple owners with 409 AMBIGUOUS_SKILL_SLUG; the bare GET in _skill_detail returned None for every such slug, so 'skills install clawhub/@owner/slug' (and the owner/skills/slug URL form) failed at fetch time even though the requester already knew the owner (#104117). - _skill_detail forwards expected_owner as the ?owner= query param on the detail GET (params already flows through _get_json's **kwargs). - _parse_identifier also accepts the clawhub/@owner/slug combination: the @ surfaces only after the clawhub/ prefix is stripped, so the had_at check now re-runs on the stripped form. GitHub-style owner/repo/skill paths stay rejected. --- tests/tools/test_skills_hub_clawhub.py | 83 ++++++++++++++++++++++++++ tools/skills_hub_clawhub.py | 15 ++++- 2 files changed, 96 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_skills_hub_clawhub.py b/tests/tools/test_skills_hub_clawhub.py index 90a682d8dc..4882df768b 100644 --- a/tests/tools/test_skills_hub_clawhub.py +++ b/tests/tools/test_skills_hub_clawhub.py @@ -378,6 +378,10 @@ class TestClawHubSource(unittest.TestCase): ClawHubSource._parse_identifier("@harrylabsj/skillopt"), ("skillopt", "harrylabsj"), ) + self.assertEqual( + ClawHubSource._parse_identifier("clawhub/@harrylabsj/skillopt"), + ("skillopt", "harrylabsj"), + ) self.assertEqual( ClawHubSource._parse_identifier("harrylabsj/skills/skillopt"), ("skillopt", "harrylabsj"), @@ -427,6 +431,85 @@ 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): """max_items bounds the walk so browse's cold-start fallback renders one diff --git a/tools/skills_hub_clawhub.py b/tools/skills_hub_clawhub.py index 1c46cabbf6..263bfc384d 100644 --- a/tools/skills_hub_clawhub.py +++ b/tools/skills_hub_clawhub.py @@ -131,7 +131,14 @@ class ClawHubSource(GuardedFetchMixin, SkillSource): if parsed is None: return None slug, expected_owner = parsed - data = self._coerce_skill_payload(self._get_json(f"{self.BASE_URL}/skills/{slug}")) + # ClawHub answers a slug claimed by several owners with 409 + # AMBIGUOUS_SKILL_SLUG; the ?owner= query picks ours (#104117). + data = self._coerce_skill_payload( + self._get_json( + f"{self.BASE_URL}/skills/{slug}", + params={"owner": expected_owner} if expected_owner else None, + ) + ) if not isinstance(data, dict) or not self._owner_matches(expected_owner, data): return None return slug, data @@ -208,7 +215,11 @@ class ClawHubSource(GuardedFetchMixin, SkillSource): if not raw: return None had_at = raw.startswith("@") - parts = [part for part in raw.removeprefix("@").removeprefix("clawhub/").split("/") if part] + stripped = raw.removeprefix("@").removeprefix("clawhub/") + if stripped.startswith("@"): # clawhub/@owner/slug — @ surfaces after the prefix + had_at = True + stripped = stripped.removeprefix("@") + parts = [part for part in stripped.split("/") if part] if len(parts) == 1: owner, slug = None, parts[0] elif (len(parts) == 2 and had_at) or (len(parts) == 3 and parts[1].lower() == "skills"):