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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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"):
|
||||
|
||||
Reference in New Issue
Block a user