feat(skills): install skills from a direct HTTP(S) URL (#16323)
* feat(skills): install skills from a direct HTTP(S) URL Adds UrlSource adapter so `hermes skills install <url-to-SKILL.md>` and `/skills install <url>` work as first-class operations — no more improvising with curl + patch + cp. - Claims identifiers that start with http(s):// and end in .md - Skips /.well-known/skills/ URLs (WellKnownSkillSource handles those) - Skill name from YAML frontmatter, URL-slug fallback - Single-file SKILL.md only (v1 scope — multi-file skills need a manifest) - Trust level 'community'; full security scan still runs - Lock file stores the URL as identifier so `hermes skills update` re-fetches from the same URL cleanly Scope matches real user need from @versun's docx feedback where `https://sharethis.chat/SKILL.md` had no first-class install path. * feat(skills): interactive name/category for URL installs + --name override Follow-up to the UrlSource adapter. The previous commit fell back to weak heuristics when frontmatter had no ``name:`` and could produce garbage names like ``SKILL`` or ``unnamed-skill``. Now: tools/skills_hub.py - ``UrlSource._is_valid_skill_name()`` — strict identifier check (``^[a-z][a-z0-9_-]*$``), rejects sentinel values (``SKILL``, ``README``, ``INDEX``, ``unnamed-skill``, empty, non-strings). - ``_resolve_skill_name()`` returns ``Optional[str]`` — ``None`` when nothing valid is resolvable. Also ignores unsafe frontmatter names (``../evil``) and falls through to URL slug instead of returning None immediately, so a URL with a bad frontmatter but a good path still works. - ``fetch()``/``inspect()`` carry an ``awaiting_name=True`` marker in metadata/extra when resolution fails, letting ``do_install`` decide whether to prompt, apply an override, or error out. hermes_cli/skills_hub.py - ``do_install`` gains a ``name_override`` parameter. - On URL-sourced bundles with ``awaiting_name=True``: 1. If ``name_override`` is valid → use it. 2. If ``name_override`` is invalid → refuse with a clear error. 3. Else if ``skip_confirm=True`` (non-interactive: slash / TUI / gateway / scripts) → refuse with an actionable retry hint pointing at ``--name <your-name>`` on both CLI and slash forms. 4. Else (interactive TTY) → prompt for the name. - Interactive TTY also prompts for a category when none is given for a URL-sourced install, hinting existing category buckets so users can reuse ``productivity``, ``devops``, etc. Empty input → flat install. - ``_existing_categories()`` scans ``~/.hermes/skills/`` for subdirs that look like category buckets (contain nested SKILL.md files); skips top-level skills and hidden dirs. - ``_prompt_for_skill_name()`` / ``_prompt_for_category()`` helpers (EOF/Ctrl-C-safe, match the existing ``Confirm [y/N]`` prompt style). hermes_cli/main.py - ``hermes skills install`` argparse gains ``--name <name>``. hermes_cli/skills_hub.py (slash) - ``/skills install <url> --name <x>`` parsing added. Tests - tests/tools/test_skills_hub.py: updated ``UrlSource`` tests to assert the new ``awaiting_name`` metadata; added 4 new tests for ``_is_valid_skill_name`` rejection sets and the awaiting-name marker. - tests/hermes_cli/test_skills_hub.py: 8 new tests covering --name override accept/reject, non-interactive error, interactive name prompt, interactive category prompt, cancel-aborts-install, and ``_existing_categories`` scan behavior (buckets vs flat skills). - E2E verified all four paths (no-name/no-override → error; --name override → install; frontmatter name → install; invalid --name → rejection). --------- Co-authored-by: teknium1 <teknium@noreply.github.com>
This commit is contained in:
Teknium
@@ -12,6 +12,7 @@ from tools.skills_hub import (
|
||||
GitHubSource,
|
||||
LobeHubSource,
|
||||
SkillsShSource,
|
||||
UrlSource,
|
||||
WellKnownSkillSource,
|
||||
OptionalSkillSource,
|
||||
SkillMeta,
|
||||
@@ -673,6 +674,211 @@ class TestWellKnownSkillSource:
|
||||
assert bundle is None
|
||||
|
||||
|
||||
class TestUrlSource:
|
||||
def _source(self):
|
||||
return UrlSource()
|
||||
|
||||
# ── _matches ────────────────────────────────────────────────────────
|
||||
def test_matches_bare_md_url(self):
|
||||
assert self._source()._matches("https://example.com/path/SKILL.md") is True
|
||||
|
||||
def test_matches_http_scheme(self):
|
||||
assert self._source()._matches("http://example.com/SKILL.md") is True
|
||||
|
||||
def test_rejects_non_md_url(self):
|
||||
assert self._source()._matches("https://example.com/path/") is False
|
||||
assert self._source()._matches("https://example.com/skills.json") is False
|
||||
|
||||
def test_rejects_well_known_url(self):
|
||||
# Leave these for WellKnownSkillSource.
|
||||
assert self._source()._matches(
|
||||
"https://example.com/.well-known/skills/git-workflow/SKILL.md"
|
||||
) is False
|
||||
assert self._source()._matches(
|
||||
"https://example.com/.well-known/skills/index.json"
|
||||
) is False
|
||||
|
||||
def test_rejects_wrapped_identifiers(self):
|
||||
assert self._source()._matches("github:owner/repo/skill") is False
|
||||
assert self._source()._matches("well-known:https://example.com/x") is False
|
||||
assert self._source()._matches("official/security/1password") is False
|
||||
|
||||
def test_rejects_non_string(self):
|
||||
assert self._source()._matches(None) is False # type: ignore[arg-type]
|
||||
assert self._source()._matches(123) is False # type: ignore[arg-type]
|
||||
|
||||
def test_search_returns_empty(self):
|
||||
# Direct-URL source is not searchable.
|
||||
assert self._source().search("anything") == []
|
||||
|
||||
# ── inspect ─────────────────────────────────────────────────────────
|
||||
@patch("tools.skills_hub.httpx.get")
|
||||
def test_inspect_reads_frontmatter_from_url(self, mock_get):
|
||||
mock_get.return_value = MagicMock(
|
||||
status_code=200,
|
||||
text=(
|
||||
"---\n"
|
||||
"name: sharethis-chat\n"
|
||||
"description: Share agent conversations.\n"
|
||||
"metadata:\n"
|
||||
" hermes:\n"
|
||||
" tags: [sharing, chat]\n"
|
||||
"---\n\n# Body\n"
|
||||
),
|
||||
)
|
||||
meta = self._source().inspect("https://sharethis.chat/SKILL.md")
|
||||
assert meta is not None
|
||||
assert meta.name == "sharethis-chat"
|
||||
assert meta.description == "Share agent conversations."
|
||||
assert meta.source == "url"
|
||||
assert meta.identifier == "https://sharethis.chat/SKILL.md"
|
||||
assert meta.trust_level == "community"
|
||||
assert meta.tags == ["sharing", "chat"]
|
||||
assert meta.extra["awaiting_name"] is False
|
||||
|
||||
@patch("tools.skills_hub.httpx.get")
|
||||
def test_inspect_returns_none_when_url_not_md(self, mock_get):
|
||||
# _matches filters first — no HTTP call.
|
||||
meta = self._source().inspect("https://example.com/not-a-skill")
|
||||
assert meta is None
|
||||
mock_get.assert_not_called()
|
||||
|
||||
@patch("tools.skills_hub.httpx.get")
|
||||
def test_inspect_returns_none_on_404(self, mock_get):
|
||||
mock_get.return_value = MagicMock(status_code=404)
|
||||
assert self._source().inspect("https://example.com/SKILL.md") is None
|
||||
|
||||
@patch("tools.skills_hub.httpx.get")
|
||||
def test_inspect_returns_none_on_http_error(self, mock_get):
|
||||
mock_get.side_effect = httpx.HTTPError("boom")
|
||||
assert self._source().inspect("https://example.com/SKILL.md") is None
|
||||
|
||||
@patch("tools.skills_hub.httpx.get")
|
||||
def test_inspect_flags_awaiting_name_when_unresolvable(self, mock_get):
|
||||
# No frontmatter name + a URL path that can't produce a valid slug
|
||||
# (``SKILL`` isn't a valid skill name).
|
||||
mock_get.return_value = MagicMock(
|
||||
status_code=200,
|
||||
text="---\ndescription: unnamed.\n---\n",
|
||||
)
|
||||
meta = self._source().inspect("https://example.com/SKILL.md")
|
||||
assert meta is not None
|
||||
assert meta.name == ""
|
||||
assert meta.extra["awaiting_name"] is True
|
||||
|
||||
# ── fetch ───────────────────────────────────────────────────────────
|
||||
@patch("tools.skills_hub.httpx.get")
|
||||
def test_fetch_builds_single_file_bundle(self, mock_get):
|
||||
skill_md = (
|
||||
"---\n"
|
||||
"name: sharethis-chat\n"
|
||||
"description: Share.\n"
|
||||
"---\n\n# Body\n"
|
||||
)
|
||||
mock_get.return_value = MagicMock(status_code=200, text=skill_md)
|
||||
|
||||
bundle = self._source().fetch("https://sharethis.chat/SKILL.md")
|
||||
|
||||
assert bundle is not None
|
||||
assert bundle.name == "sharethis-chat"
|
||||
assert bundle.source == "url"
|
||||
assert bundle.identifier == "https://sharethis.chat/SKILL.md"
|
||||
assert bundle.trust_level == "community"
|
||||
assert bundle.files == {"SKILL.md": skill_md}
|
||||
assert bundle.metadata["url"] == "https://sharethis.chat/SKILL.md"
|
||||
assert bundle.metadata["awaiting_name"] is False
|
||||
|
||||
@patch("tools.skills_hub.httpx.get")
|
||||
def test_fetch_falls_back_to_url_directory_name(self, mock_get):
|
||||
# Frontmatter has no ``name:`` — we slug from the URL directory.
|
||||
mock_get.return_value = MagicMock(
|
||||
status_code=200,
|
||||
text="---\ndescription: No name.\n---\n\n# Body\n",
|
||||
)
|
||||
bundle = self._source().fetch("https://example.com/my-skill/SKILL.md")
|
||||
assert bundle is not None
|
||||
assert bundle.name == "my-skill"
|
||||
assert bundle.metadata["awaiting_name"] is False
|
||||
|
||||
@patch("tools.skills_hub.httpx.get")
|
||||
def test_fetch_falls_back_to_filename_when_no_parent_dir(self, mock_get):
|
||||
mock_get.return_value = MagicMock(
|
||||
status_code=200,
|
||||
text="---\ndescription: Bare file.\n---\n",
|
||||
)
|
||||
bundle = self._source().fetch("https://example.com/my-skill.md")
|
||||
assert bundle is not None
|
||||
assert bundle.name == "my-skill"
|
||||
assert bundle.metadata["awaiting_name"] is False
|
||||
|
||||
@patch("tools.skills_hub.httpx.get")
|
||||
def test_fetch_awaiting_name_when_unresolvable(self, mock_get):
|
||||
# Bare ``SKILL.md`` at the domain root with no frontmatter name.
|
||||
mock_get.return_value = MagicMock(
|
||||
status_code=200,
|
||||
text="---\ndescription: Bare.\n---\n\n# Body\n",
|
||||
)
|
||||
bundle = self._source().fetch("https://example.com/SKILL.md")
|
||||
assert bundle is not None
|
||||
assert bundle.name == ""
|
||||
assert bundle.metadata["awaiting_name"] is True
|
||||
# File content still present — CLI will reuse it after picking a name.
|
||||
assert bundle.files["SKILL.md"].startswith("---\n")
|
||||
|
||||
@patch("tools.skills_hub.httpx.get")
|
||||
def test_fetch_awaiting_name_rejects_sentinel_slug(self, mock_get):
|
||||
# Frontmatter has no name AND the URL filename slug is ``README`` —
|
||||
# our valid-name check rejects it, so we flag awaiting_name.
|
||||
mock_get.return_value = MagicMock(
|
||||
status_code=200,
|
||||
text="---\ndescription: no name.\n---\n",
|
||||
)
|
||||
bundle = self._source().fetch("https://example.com/README.md")
|
||||
assert bundle is not None
|
||||
assert bundle.name == ""
|
||||
assert bundle.metadata["awaiting_name"] is True
|
||||
|
||||
@patch("tools.skills_hub.httpx.get")
|
||||
def test_fetch_ignores_unsafe_frontmatter_name_and_falls_through_to_slug(self, mock_get):
|
||||
# Traversal / unsafe names are rejected by ``_is_valid_skill_name``;
|
||||
# resolver falls through to URL slug (``my-skill`` here) and succeeds.
|
||||
mock_get.return_value = MagicMock(
|
||||
status_code=200,
|
||||
text="---\nname: ../evil\ndescription: Bad.\n---\n",
|
||||
)
|
||||
bundle = self._source().fetch("https://example.com/my-skill/SKILL.md")
|
||||
assert bundle is not None
|
||||
assert bundle.name == "my-skill"
|
||||
|
||||
@patch("tools.skills_hub.httpx.get")
|
||||
def test_fetch_returns_none_on_404(self, mock_get):
|
||||
mock_get.return_value = MagicMock(status_code=404)
|
||||
assert self._source().fetch("https://example.com/SKILL.md") is None
|
||||
|
||||
@patch("tools.skills_hub.httpx.get")
|
||||
def test_fetch_skips_non_matching_identifier(self, mock_get):
|
||||
assert self._source().fetch("owner/repo/skill") is None
|
||||
mock_get.assert_not_called()
|
||||
|
||||
# ── _is_valid_skill_name ────────────────────────────────────────────
|
||||
def test_is_valid_skill_name_accepts_identifiers(self):
|
||||
valid = ["my-skill", "my_skill", "sharethis-chat", "a", "skill-1", "s1"]
|
||||
for name in valid:
|
||||
assert UrlSource._is_valid_skill_name(name), f"should accept {name!r}"
|
||||
|
||||
def test_is_valid_skill_name_rejects_sentinel_and_garbage(self):
|
||||
invalid = [
|
||||
"",
|
||||
"SKILL", "skill", "README", "readme", "INDEX", "index",
|
||||
"unnamed-skill",
|
||||
"../evil", "a/b", "has space", "has.dot",
|
||||
"-leading-dash", "1-leading-digit",
|
||||
None, 123, ["list"],
|
||||
]
|
||||
for name in invalid:
|
||||
assert not UrlSource._is_valid_skill_name(name), f"should reject {name!r}"
|
||||
|
||||
|
||||
class TestCheckForSkillUpdates:
|
||||
def test_bundle_content_hash_matches_installed_content_hash(self, tmp_path):
|
||||
from tools.skills_guard import content_hash
|
||||
@@ -755,6 +961,17 @@ class TestCreateSourceRouter:
|
||||
sources = create_source_router(auth=MagicMock(spec=GitHubAuth))
|
||||
assert any(isinstance(src, WellKnownSkillSource) for src in sources)
|
||||
|
||||
def test_includes_url_source(self):
|
||||
sources = create_source_router(auth=MagicMock(spec=GitHubAuth))
|
||||
assert any(isinstance(src, UrlSource) for src in sources)
|
||||
|
||||
def test_url_source_runs_before_github_source(self):
|
||||
# UrlSource must win over GitHubSource when both could claim a URL.
|
||||
sources = create_source_router(auth=MagicMock(spec=GitHubAuth))
|
||||
url_idx = next(i for i, src in enumerate(sources) if isinstance(src, UrlSource))
|
||||
gh_idx = next(i for i, src in enumerate(sources) if isinstance(src, GitHubSource))
|
||||
assert url_idx < gh_idx
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# HubLockFile
|
||||
|
||||
Reference in New Issue
Block a user