diff --git a/agent/skill_commands.py b/agent/skill_commands.py index 18264c44bd3b..910abed18f36 100644 --- a/agent/skill_commands.py +++ b/agent/skill_commands.py @@ -26,6 +26,21 @@ _SKILL_INVALID_CHARS = re.compile(r"[^a-z0-9-]") _SKILL_MULTI_HYPHEN = re.compile(r"-{2,}") + +def slugify_skill_name(name: str) -> str: + """Reduce a skill's display name to its slash-command slug (no leading slash). + + Lowercase, spaces/underscores to hyphens, drop everything outside ``[a-z0-9-]`` + (so ``+`` / ``/`` can't produce invalid downstream command names), collapse + runs of hyphens, and trim leading/trailing ones. Returns ``""`` when nothing + usable remains. This is the single source of truth for the slug: both the + command registry (``scan_skill_commands``) and the ``GET /v1/skills`` listing + derive the command from it, so a slug shown to a client always resolves. + """ + slug = name.lower().replace(" ", "-").replace("_", "-") + slug = _SKILL_INVALID_CHARS.sub("", slug) + return _SKILL_MULTI_HYPHEN.sub("-", slug).strip("-") + # --------------------------------------------------------------------------- # Skill-scaffolding markers and the canonical extractor. # @@ -394,12 +409,7 @@ def scan_skill_commands() -> Dict[str, Dict[str, Any]]: description = line[:80] break seen_names.add(name) - # Normalize to hyphen-separated slug, stripping - # non-alnum chars (e.g. +, /) to avoid invalid - # Telegram command names downstream. - cmd_name = name.lower().replace(' ', '-').replace('_', '-') - cmd_name = _SKILL_INVALID_CHARS.sub('', cmd_name) - cmd_name = _SKILL_MULTI_HYPHEN.sub('-', cmd_name).strip('-') + cmd_name = slugify_skill_name(name) if not cmd_name: continue _skill_commands[f"/{cmd_name}"] = { diff --git a/gateway/platforms/api_server.py b/gateway/platforms/api_server.py index b562a460eb88..6f47ccab291e 100644 --- a/gateway/platforms/api_server.py +++ b/gateway/platforms/api_server.py @@ -1345,17 +1345,45 @@ async def _handle_skills(self, request: "web.Request") -> "web.Response": the model. Mirrors what the gateway/CLI surfaces through ``/skills list``, but as a deterministic JSON payload. - Returns the same skill metadata (name, description, category) the - skills hub uses internally. Disabled skills are excluded so the - listing matches what the agent actually loads. + Returns the skill metadata (name, description, category) the skills hub + uses internally, plus the ``command`` slug a client sends to invoke the + skill (``/`` on this same endpoint). ``command`` is ``null`` + when the name reduces to an empty slug. Disabled skills are excluded so + the listing matches what the agent actually loads. """ auth_err = self._check_auth(request) if auth_err: return auth_err try: + from agent.skill_commands import get_skill_commands, slugify_skill_name from tools.skills_tool import _find_all_skills, _sort_skills + skills = _sort_skills(_find_all_skills(skip_disabled=False)) + # Validate each derived command against the live command registry so a + # non-null `command` is GUARANTEED to resolve on the chat path: this + # rejects any slug that doesn't round-trip (e.g. a name truncated for + # display, or a skill disabled/incompatible for this platform and thus + # absent from the registry), reporting it as command=null rather than a + # command that silently fails to invoke. + registry = get_skill_commands() + data = [] + for skill in skills: + slug = slugify_skill_name(skill.get("name", "")) + command = slug if slug and f"/{slug}" in registry else None + data.append({**skill, "command": command}) + # Surface the user-facing built-in commands the palette offers (just + # /learn today) from this same endpoint, so the client lists them from + # the sprite rather than hardcoding them — the palette is empty until + # the gateway responds, instead of showing commands while it's cold. + # category "command" buckets them separately client-side; the chat path + # expands /learn via build_learn_prompt. + data.append({ + "name": "learn", + "description": "Learn a reusable skill from a description, files, URLs, or this chat", + "category": "command", + "command": "learn", + }) except Exception: logger.exception("GET /v1/skills failed") return web.json_response( @@ -1365,7 +1393,7 @@ async def _handle_skills(self, request: "web.Request") -> "web.Response": return web.json_response({ "object": "list", - "data": skills, + "data": data, }) async def _handle_toolsets(self, request: "web.Request") -> "web.Response": @@ -1939,6 +1967,102 @@ async def _run_and_signal() -> None: logger.debug("[api_server] session SSE stream error: %s", exc) return response + def _maybe_expand_slash_command(self, user_message: Any, session_id: str) -> Optional[str]: + """Expand a leading ``/command`` into the message the agent should process. + + Mirrors how every other Hermes surface (CLI/TUI/messaging gateway, see + ``gateway/run.py``) dispatches slash commands. Two cases expand: + + - ``/ [instruction]`` — resolved against the installed + bundles (which take precedence) and skills; the turn is replaced with + the payload built by ``build_skill_invocation_message`` (activation note + + skill content + skill-dir + config + the trailing instruction). + - ``/learn [what to learn from]`` — the one built-in command that makes + sense on the chat path (no Omnia-UI equivalent): rewritten to the + standards-guided ``build_learn_prompt`` that drives the agent to author + a skill via ``skill_manage``. Side-effecting built-ins (``/new``, + ``/yolo``, …) are deliberately NOT handled here. + + The unmodified builder output is returned so the memory layer's invocation + markers stay intact. + + Returns the expanded message, or ``None`` when the message is not a + recognized command — in which case the caller forwards the original text + untouched. Unlike the messaging gateway, the chat-completions API carries + general prose, so a message that merely starts with ``/`` (a path like + ``/Users/x``, a question about ``/etc``) must pass through: only a + confirmed match is intercepted. + + Only plain-text turns expand: a multimodal turn (text + image) arrives as + a content list, not a ``str``, so a ``/skill`` typed alongside an + attachment is forwarded as-is rather than expanded. Acceptable because the + palette's skills are text-instruction driven. + """ + if not isinstance(user_message, str): + return None + text = user_message.lstrip() + if not text.startswith("/"): + return None + # "/command rest of the instruction" -> ("command", "rest of the instruction") + parts = text[1:].split(None, 1) + if not parts: + return None + command = parts[0] + user_instruction = parts[1].strip() if len(parts) > 1 else "" + + # Built-in /learn (a message-expansion command) before skills/bundles — + # it rewrites the turn to a prompt that has the agent author a skill. + if command == "learn": + try: + from agent.learn_prompt import build_learn_prompt + + return build_learn_prompt(user_instruction) + except Exception: + logger.exception("Failed to build the /learn prompt") + return None + + try: + from agent.skill_bundles import ( + build_bundle_invocation_message, + resolve_bundle_command_key, + ) + from agent.skill_commands import ( + build_skill_invocation_message, + resolve_skill_command_key, + ) + except Exception as exc: + logger.warning("Skill command modules unavailable; forwarding /%s as-is: %s", command, exc) + return None + + # Bundles take precedence over single skills (matches gateway dispatch order). + # A command that RESOLVES to a real bundle/skill but then fails to BUILD a + # payload (e.g. a SKILL.md removed or unreadable on disk) is a genuine + # failure, not a non-match — log it loudly instead of silently forwarding + # the raw "/foo" to the model, which would look like the skill "did nothing". + try: + bundle_key = resolve_bundle_command_key(command) + if bundle_key is not None: + bundle_result = build_bundle_invocation_message( + bundle_key, user_instruction, task_id=session_id + ) + if bundle_result and bundle_result[0]: + return bundle_result[0] + logger.error("Bundle /%s resolved but built no payload; forwarding raw text", command) + except Exception: + logger.exception("Bundle command expansion failed for /%s", command) + + try: + cmd_key = resolve_skill_command_key(command) + if cmd_key is not None: + msg = build_skill_invocation_message(cmd_key, user_instruction, task_id=session_id) + if msg: + return msg + logger.error("Skill /%s resolved but built no payload; forwarding raw text", command) + except Exception: + logger.exception("Skill command expansion failed for /%s", command) + + return None + async def _handle_chat_completions(self, request: "web.Request") -> "web.Response": """POST /v1/chat/completions — OpenAI Chat Completions format.""" auth_err = self._check_auth(request) @@ -2068,6 +2192,14 @@ async def _handle_chat_completions(self, request: "web.Request") -> "web.Respons session_id = _derive_chat_session_id(system_prompt, first_user) # history already set from request body above + # Honor slash commands on the OpenAI chat path the same way the + # CLI/TUI/messaging surfaces do: a recognized /skill, /bundle, or /learn + # is expanded before the agent runs; anything else (incl. an unrecognized + # "/...") is forwarded untouched. + expanded_command = self._maybe_expand_slash_command(user_message, session_id) + if expanded_command is not None: + user_message = expanded_command + completion_id = f"chatcmpl-{uuid.uuid4().hex[:29]}" model_name = body.get("model", self._model_name) created = int(time.time()) diff --git a/tests/agent/test_slugify_skill_name.py b/tests/agent/test_slugify_skill_name.py new file mode 100644 index 000000000000..fa73daaf8762 --- /dev/null +++ b/tests/agent/test_slugify_skill_name.py @@ -0,0 +1,32 @@ +"""Unit tests for ``slugify_skill_name`` — the single source of truth for the +slash-command slug shared between the command registry and the /v1/skills listing. +""" + +import pytest + +from agent.skill_commands import slugify_skill_name + + +@pytest.mark.parametrize( + "name,expected", + [ + ("site-audit", "site-audit"), + ("Site Audit", "site-audit"), + ("create_pdf", "create-pdf"), + ("My Cool Skill", "my-cool-skill"), + ("Foo + Bar / Baz", "foo-bar-baz"), # invalid chars dropped, runs collapsed + (" spaced ", "spaced"), + ("Trailing-", "trailing"), # leading/trailing hyphens trimmed + ("-Leading", "leading"), + ("a--b", "a-b"), # multi-hyphen collapse + ("ALLCAPS", "allcaps"), + ], +) +def test_slugify_produces_expected_slug(name, expected): + assert slugify_skill_name(name) == expected + + +def test_slugify_returns_empty_when_nothing_usable_remains(): + # A name made entirely of invalid characters reduces to "". + assert slugify_skill_name("+++") == "" + assert slugify_skill_name(" ") == "" diff --git a/tests/gateway/test_api_server.py b/tests/gateway/test_api_server.py index 0ea247c307c4..9a6b4100e115 100644 --- a/tests/gateway/test_api_server.py +++ b/tests/gateway/test_api_server.py @@ -843,10 +843,44 @@ async def test_skills_returns_list_envelope(self, adapter): data = await resp.json() assert data["object"] == "list" names = sorted(s["name"] for s in data["data"]) - assert names == ["ascii-art", "github"] + # Skills, plus the always-appended /learn built-in command entry. + assert names == ["ascii-art", "github", "learn"] for entry in data["data"]: assert set(entry.keys()) >= {"name", "description", "category"} + @pytest.mark.asyncio + async def test_skills_includes_the_learn_command(self, adapter): + with patch("tools.skills_tool._find_all_skills", return_value=[]): + app = _create_app(adapter) + async with TestClient(TestServer(app)) as cli: + resp = await cli.get("/v1/skills") + data = await resp.json() + learn = next(s for s in data["data"] if s["command"] == "learn") + # Displayed as the command itself, bucketed as a command (not a skill). + assert learn["name"] == "learn" + assert learn["category"] == "command" + + @pytest.mark.asyncio + async def test_skills_command_is_validated_against_the_registry(self, adapter): + # "Site Audit" slugifies to a registered command; "Orphan" does not, so + # its command must be null (a non-null command is guaranteed to resolve). + fake_skills = [ + {"name": "Site Audit", "description": "Audit", "category": "omnio"}, + {"name": "Orphan", "description": "Unregistered", "category": "omnio"}, + ] + with ( + patch("tools.skills_tool._find_all_skills", return_value=list(fake_skills)), + patch("agent.skill_commands.get_skill_commands", return_value={"/site-audit": {}}), + ): + app = _create_app(adapter) + async with TestClient(TestServer(app)) as cli: + resp = await cli.get("/v1/skills") + assert resp.status == 200 + data = await resp.json() + by_name = {s["name"]: s for s in data["data"]} + assert by_name["Site Audit"]["command"] == "site-audit" + assert by_name["Orphan"]["command"] is None + @pytest.mark.asyncio async def test_skills_handles_enumeration_failure(self, adapter): with patch( diff --git a/tests/gateway/test_api_server_slash_commands.py b/tests/gateway/test_api_server_slash_commands.py new file mode 100644 index 000000000000..bbe0eb67e0d0 --- /dev/null +++ b/tests/gateway/test_api_server_slash_commands.py @@ -0,0 +1,165 @@ +"""Unit tests for ``/skill-name`` slash-command expansion on /v1/chat/completions. + +The OpenAI chat path honors slash commands like every other Hermes surface: a +recognized ``/ [instruction]`` is expanded into its skill-invocation +payload before the agent runs, while anything else (a path, a question about +``/etc``, an unknown command) passes through untouched. These tests exercise +``APIServerAdapter._maybe_expand_slash_command`` directly — it carries no +instance state, so it is constructed via ``__new__`` and the skill-resolution +functions it imports lazily are patched at their source modules. +""" + +from unittest.mock import patch + +from gateway.platforms.api_server import APIServerAdapter + +SESSION_ID = "api_123_abcd" + + +def _adapter() -> APIServerAdapter: + # _maybe_expand_slash_command touches no instance attributes; skip __init__. + return APIServerAdapter.__new__(APIServerAdapter) + + +def _patch_skills(*, skill_key=None, skill_msg=None, bundle_key=None, bundle_result=None): + """Patch the bundle + skill resolvers/builders the method imports lazily.""" + return ( + patch("agent.skill_bundles.resolve_bundle_command_key", return_value=bundle_key), + patch("agent.skill_bundles.build_bundle_invocation_message", return_value=bundle_result), + patch("agent.skill_commands.resolve_skill_command_key", return_value=skill_key), + patch("agent.skill_commands.build_skill_invocation_message", return_value=skill_msg), + ) + + +class TestNoExpansion: + def test_non_string_content_passes_through(self): + # Multimodal content is a list — never a slash command. + content = [{"type": "text", "text": "/site-audit"}] + assert _adapter()._maybe_expand_slash_command(content, SESSION_ID) is None + + def test_message_without_leading_slash_passes_through(self): + assert _adapter()._maybe_expand_slash_command("hello there", SESSION_ID) is None + + def test_lone_slash_passes_through(self): + assert _adapter()._maybe_expand_slash_command("/", SESSION_ID) is None + assert _adapter()._maybe_expand_slash_command("/ ", SESSION_ID) is None + + def test_unrecognized_command_passes_through(self): + # A "/"-leading message that matches no bundle and no skill (e.g. a path + # like "/Users/x") must be forwarded untouched, not rejected. + patches = _patch_skills(skill_key=None, bundle_key=None) + with patches[0], patches[1], patches[2], patches[3]: + assert _adapter()._maybe_expand_slash_command("/Users/pablo/notes", SESSION_ID) is None + + def test_skill_resolves_but_build_returns_none_passes_through_and_logs(self, caplog): + # A command that resolves to a real skill but builds no payload is a + # failure, not a non-match: it still passes through, but loudly. + patches = _patch_skills(skill_key="/site-audit", skill_msg=None, bundle_key=None) + with caplog.at_level("ERROR"), patches[0], patches[1], patches[2], patches[3]: + assert _adapter()._maybe_expand_slash_command("/site-audit", SESSION_ID) is None + assert any("resolved but built no payload" in record.message for record in caplog.records) + + def test_bundle_resolves_but_build_returns_none_passes_through_and_logs(self, caplog): + patches = _patch_skills(skill_key=None, bundle_key="/research", bundle_result=None) + with caplog.at_level("ERROR"), patches[0], patches[1], patches[2], patches[3]: + assert _adapter()._maybe_expand_slash_command("/research", SESSION_ID) is None + assert any("resolved but built no payload" in record.message for record in caplog.records) + + def test_resolver_exception_passes_through(self): + with patch("agent.skill_bundles.resolve_bundle_command_key", return_value=None), \ + patch("agent.skill_commands.resolve_skill_command_key", side_effect=RuntimeError("boom")): + assert _adapter()._maybe_expand_slash_command("/site-audit", SESSION_ID) is None + + +class TestSkillExpansion: + def test_known_skill_is_expanded(self): + patches = _patch_skills(skill_key="/site-audit", skill_msg="EXPANDED PAYLOAD", bundle_key=None) + with patches[0], patches[1], patches[2], patches[3] as build: + out = _adapter()._maybe_expand_slash_command("/site-audit run on example.com", SESSION_ID) + assert out == "EXPANDED PAYLOAD" + # command stripped of slash; trailing instruction + session id threaded through. + build.assert_called_once_with("/site-audit", "run on example.com", task_id=SESSION_ID) + + def test_skill_without_instruction_uses_empty_string(self): + patches = _patch_skills(skill_key="/site-audit", skill_msg="X", bundle_key=None) + with patches[0], patches[1], patches[2], patches[3] as build: + out = _adapter()._maybe_expand_slash_command("/site-audit", SESSION_ID) + assert out == "X" + build.assert_called_once_with("/site-audit", "", task_id=SESSION_ID) + + def test_leading_whitespace_before_slash_is_tolerated(self): + patches = _patch_skills(skill_key="/site-audit", skill_msg="X", bundle_key=None) + with patches[0], patches[1], patches[2], patches[3] as build: + out = _adapter()._maybe_expand_slash_command(" /site-audit go", SESSION_ID) + assert out == "X" + build.assert_called_once_with("/site-audit", "go", task_id=SESSION_ID) + + def test_multiline_instruction_is_preserved(self): + patches = _patch_skills(skill_key="/site-audit", skill_msg="X", bundle_key=None) + with patches[0], patches[1], patches[2], patches[3] as build: + _adapter()._maybe_expand_slash_command("/site-audit\nline one\nline two", SESSION_ID) + build.assert_called_once_with("/site-audit", "line one\nline two", task_id=SESSION_ID) + + def test_resolver_receives_command_resolution(self): + patches = _patch_skills(skill_key="/site-audit", skill_msg="X", bundle_key=None) + with patches[0], patches[1], patches[2] as resolve, patches[3]: + _adapter()._maybe_expand_slash_command("/site_audit now", SESSION_ID) + # The raw command token (underscores and all) is handed to the resolver, + # which owns underscore->hyphen normalization. + resolve.assert_called_once_with("site_audit") + + +class TestBundlePrecedence: + def test_bundle_wins_over_skill(self): + patches = _patch_skills( + skill_key="/research", + skill_msg="SKILL PAYLOAD", + bundle_key="/research", + bundle_result=("BUNDLE PAYLOAD", ["a", "b"], []), + ) + with patches[0], patches[1] as build_bundle, patches[2], patches[3] as build_skill: + out = _adapter()._maybe_expand_slash_command("/research deep dive", SESSION_ID) + assert out == "BUNDLE PAYLOAD" + build_bundle.assert_called_once_with("/research", "deep dive", task_id=SESSION_ID) + build_skill.assert_not_called() + + def test_bundle_build_failure_falls_back_to_skill(self): + # Bundle resolves but produces no message -> fall through to skill. + patches = _patch_skills( + skill_key="/research", + skill_msg="SKILL PAYLOAD", + bundle_key="/research", + bundle_result=None, + ) + with patches[0], patches[1], patches[2], patches[3]: + out = _adapter()._maybe_expand_slash_command("/research", SESSION_ID) + assert out == "SKILL PAYLOAD" + + +class TestLearnCommand: + def test_learn_with_instruction_is_rewritten_to_the_learn_prompt(self): + with patch("agent.learn_prompt.build_learn_prompt", return_value="LEARN PROMPT") as build: + out = _adapter()._maybe_expand_slash_command("/learn from this repo dir", SESSION_ID) + assert out == "LEARN PROMPT" + build.assert_called_once_with("from this repo dir") + + def test_learn_without_instruction_passes_empty_string(self): + with patch("agent.learn_prompt.build_learn_prompt", return_value="LEARN PROMPT") as build: + out = _adapter()._maybe_expand_slash_command("/learn", SESSION_ID) + assert out == "LEARN PROMPT" + build.assert_called_once_with("") + + def test_learn_is_dispatched_before_skill_and_bundle_resolution(self): + with ( + patch("agent.learn_prompt.build_learn_prompt", return_value="LEARN PROMPT"), + patch("agent.skill_commands.resolve_skill_command_key") as resolve_skill, + patch("agent.skill_bundles.resolve_bundle_command_key") as resolve_bundle, + ): + out = _adapter()._maybe_expand_slash_command("/learn x", SESSION_ID) + assert out == "LEARN PROMPT" + resolve_skill.assert_not_called() + resolve_bundle.assert_not_called() + + def test_learn_prompt_build_failure_passes_through(self): + with patch("agent.learn_prompt.build_learn_prompt", side_effect=RuntimeError("boom")): + assert _adapter()._maybe_expand_slash_command("/learn x", SESSION_ID) is None