diff --git a/agent/skill_commands.py b/agent/skill_commands.py index 280105daca73..a9f08a0849d0 100644 --- a/agent/skill_commands.py +++ b/agent/skill_commands.py @@ -8,9 +8,10 @@ import json import logging import re +from dataclasses import dataclass from datetime import datetime from pathlib import Path -from typing import Any, Dict, Optional +from typing import Any, Dict, Literal, Optional from hermes_constants import display_hermes_home @@ -23,6 +24,25 @@ _SKILL_MULTI_HYPHEN = re.compile(r"-{2,}") +@dataclass(frozen=True) +class SkillInvocationResult: + """Explicit outcome for slash-command skill loading.""" + + status: Literal["ok", "unknown_command", "load_failed"] + command: str + message: str | None = None + skill_name: str | None = None + + @property + def ok(self) -> bool: + return self.status == "ok" + + def __bool__(self) -> bool: + raise TypeError( + "SkillInvocationResult does not support truthiness; use .ok or .status." + ) + + def build_plan_path( user_instruction: str = "", *, @@ -302,7 +322,7 @@ def build_skill_invocation_message( user_instruction: str = "", task_id: str | None = None, runtime_note: str = "", -) -> Optional[str]: +) -> SkillInvocationResult: """Build the user message content for a skill slash command invocation. Args: @@ -310,28 +330,40 @@ def build_skill_invocation_message( user_instruction: Optional text the user typed after the command. Returns: - The formatted message string, or None if the skill wasn't found. + A structured result describing whether the command was found and loaded. """ commands = get_skill_commands() skill_info = commands.get(cmd_key) if not skill_info: - return None + return SkillInvocationResult( + status="unknown_command", + command=cmd_key, + ) loaded = _load_skill_payload(skill_info["skill_dir"], task_id=task_id) if not loaded: - return f"[Failed to load skill: {skill_info['name']}]" + return SkillInvocationResult( + status="load_failed", + command=cmd_key, + skill_name=str(skill_info.get("name") or cmd_key.lstrip("/")), + ) loaded_skill, skill_dir, skill_name = loaded activation_note = ( f'[SYSTEM: The user has invoked the "{skill_name}" skill, indicating they want ' "you to follow its instructions. The full skill content is loaded below.]" ) - return _build_skill_message( - loaded_skill, - skill_dir, - activation_note, - user_instruction=user_instruction, - runtime_note=runtime_note, + return SkillInvocationResult( + status="ok", + command=cmd_key, + message=_build_skill_message( + loaded_skill, + skill_dir, + activation_note, + user_instruction=user_instruction, + runtime_note=runtime_note, + ), + skill_name=skill_name, ) diff --git a/cli.py b/cli.py index 85a7b50828d6..f45a55cb9e1e 100644 --- a/cli.py +++ b/cli.py @@ -5640,14 +5640,14 @@ def process_command(self, command: str) -> bool: # Check for skill slash commands (/gif-search, /axolotl, etc.) elif base_cmd in _skill_commands: user_instruction = cmd_original[len(base_cmd):].strip() - msg = build_skill_invocation_message( + invocation = build_skill_invocation_message( base_cmd, user_instruction, task_id=self.session_id ) - if msg: - skill_name = _skill_commands[base_cmd]["name"] + if invocation.ok: + skill_name = invocation.skill_name or _skill_commands[base_cmd]["name"] print(f"\nāš” Loading skill: {skill_name}") if hasattr(self, '_pending_input'): - self._pending_input.put(msg) + self._pending_input.put(invocation.message) else: ChatConsole().print(f"[bold red]Failed to load skill for {base_cmd}[/]") else: @@ -5699,7 +5699,7 @@ def _handle_plan_command(self, cmd: str): user_instruction = parts[1].strip() if len(parts) > 1 else "" plan_path = build_plan_path(user_instruction) - msg = build_skill_invocation_message( + invocation = build_skill_invocation_message( "/plan", user_instruction, task_id=self.session_id, @@ -5709,13 +5709,13 @@ def _handle_plan_command(self, cmd: str): ), ) - if not msg: + if not invocation.ok: ChatConsole().print("[bold red]Failed to load the bundled /plan skill[/]") return _cprint(f" šŸ“ Plan mode queued via skill. Markdown plan target: {plan_path}") if hasattr(self, '_pending_input'): - self._pending_input.put(msg) + self._pending_input.put(invocation.message) else: ChatConsole().print("[bold red]Plan mode unavailable: input queue not initialized[/]") diff --git a/gateway/platforms/webhook.py b/gateway/platforms/webhook.py index c37445b17e8e..fd86a418156f 100644 --- a/gateway/platforms/webhook.py +++ b/gateway/platforms/webhook.py @@ -381,12 +381,17 @@ async def _handle_webhook(self, request: "web.Request") -> "web.Response": for skill_name in skills: cmd_key = f"/{skill_name}" if cmd_key in skill_cmds: - skill_content = build_skill_invocation_message( + invocation = build_skill_invocation_message( cmd_key, user_instruction=prompt ) - if skill_content: - prompt = skill_content + if invocation.ok: + prompt = invocation.message break # Load the first matching skill + if invocation.status == "load_failed": + logger.warning( + "[webhook] Failed to load skill '%s' after discovery", + invocation.skill_name or skill_name, + ) else: logger.warning( "[webhook] Skill '%s' not found", skill_name diff --git a/gateway/run.py b/gateway/run.py index ba7ea43ad46a..6c0183c60aa5 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -3066,7 +3066,7 @@ async def _handle_message(self, event: MessageEvent) -> Optional[str]: user_instruction = event.get_command_args().strip() plan_path = build_plan_path(user_instruction) - event.text = build_skill_invocation_message( + invocation = build_skill_invocation_message( "/plan", user_instruction, task_id=_quick_key, @@ -3075,8 +3075,9 @@ async def _handle_message(self, event: MessageEvent) -> Optional[str]: f"inside the active workspace/backend cwd: {plan_path}" ), ) - if not event.text: + if not invocation.ok: return "Failed to load the bundled /plan skill." + event.text = invocation.message canonical = None except Exception as e: logger.exception("Failed to prepare /plan command") @@ -3227,12 +3228,24 @@ async def _handle_message(self, event: MessageEvent) -> Optional[str]: f"Enable it with: `hermes skills config`" ) user_instruction = event.get_command_args().strip() - msg = build_skill_invocation_message( + invocation = build_skill_invocation_message( cmd_key, user_instruction, task_id=_quick_key ) - if msg: - event.text = msg + if invocation.ok: + event.text = invocation.message # Fall through to normal message processing with skill content + elif invocation.status == "load_failed": + return ( + f"Failed to load the **{invocation.skill_name or cmd_key.lstrip('/')}** " + "skill." + ) + else: + return ( + f"Unknown command `/{command}`. " + f"Type /commands to see what's available, " + f"or resend without the leading slash to send " + f"as a regular message." + ) else: # Not an active skill ā€” check if it's a known-but-disabled or # uninstalled skill and give actionable guidance. diff --git a/tests/agent/test_skill_commands.py b/tests/agent/test_skill_commands.py index 57ac7d6b58ff..57eeb5d48011 100644 --- a/tests/agent/test_skill_commands.py +++ b/tests/agent/test_skill_commands.py @@ -5,8 +5,10 @@ from pathlib import Path from unittest.mock import patch +import pytest import tools.skills_tool as skills_tool_module from agent.skill_commands import ( + SkillInvocationResult, build_plan_path, build_preloaded_skills_prompt, build_skill_invocation_message, @@ -238,26 +240,46 @@ def test_loads_skill_by_stored_path_when_frontmatter_name_differs(self, tmp_path with patch("tools.skills_tool.SKILLS_DIR", tmp_path): scan_skill_commands() - msg = build_skill_invocation_message("/audiocraft-audio-generation", "compose") + result = build_skill_invocation_message("/audiocraft-audio-generation", "compose") - assert msg is not None - assert "AudioCraft" in msg - assert "compose" in msg + assert result.ok + assert result.message is not None + assert "AudioCraft" in result.message + assert "compose" in result.message def test_builds_message(self, tmp_path): with patch("tools.skills_tool.SKILLS_DIR", tmp_path): _make_skill(tmp_path, "test-skill") scan_skill_commands() - msg = build_skill_invocation_message("/test-skill", "do stuff") - assert msg is not None - assert "test-skill" in msg - assert "do stuff" in msg + result = build_skill_invocation_message("/test-skill", "do stuff") + assert result.ok + assert result.message is not None + assert "test-skill" in result.message + assert "do stuff" in result.message def test_returns_none_for_unknown(self, tmp_path): with patch("tools.skills_tool.SKILLS_DIR", tmp_path): scan_skill_commands() - msg = build_skill_invocation_message("/nonexistent") - assert msg is None + result = build_skill_invocation_message("/nonexistent") + assert result.status == "unknown_command" + assert result.message is None + + def test_bool_is_not_allowed_for_explicit_result_contract(self): + result = SkillInvocationResult(status="ok", command="/test-skill", message="prompt") + + with pytest.raises(TypeError, match="use \\.ok or \\.status"): + bool(result) + + def test_returns_load_failed_for_post_scan_failure(self, tmp_path): + with patch("tools.skills_tool.SKILLS_DIR", tmp_path): + skill_dir = _make_skill(tmp_path, "test-skill") + scan_skill_commands() + (skill_dir / "SKILL.md").unlink() + result = build_skill_invocation_message("/test-skill", "do stuff") + + assert result.status == "load_failed" + assert result.skill_name == "test-skill" + assert result.message is None def test_uses_shared_skill_loader_for_secure_setup(self, tmp_path, monkeypatch): monkeypatch.delenv("TENOR_API_KEY", raising=False) @@ -291,10 +313,11 @@ def fake_secret_callback(var_name, prompt, metadata=None): ), ) scan_skill_commands() - msg = build_skill_invocation_message("/test-skill", "do stuff") + result = build_skill_invocation_message("/test-skill", "do stuff") - assert msg is not None - assert "test-skill" in msg + assert result.ok + assert result.message is not None + assert "test-skill" in result.message assert len(calls) == 1 assert calls[0][0] == "TENOR_API_KEY" @@ -329,10 +352,11 @@ def fail_if_called(var_name, prompt, metadata=None): ), ) scan_skill_commands() - msg = build_skill_invocation_message("/test-skill", "do stuff") + result = build_skill_invocation_message("/test-skill", "do stuff") - assert msg is not None - assert "local cli" in msg.lower() + assert result.ok + assert result.message is not None + assert "local cli" in result.message.lower() def test_preserves_remaining_remote_setup_warning(self, tmp_path, monkeypatch): monkeypatch.setenv("TERMINAL_ENV", "ssh") @@ -355,10 +379,11 @@ def test_preserves_remaining_remote_setup_warning(self, tmp_path, monkeypatch): ), ) scan_skill_commands() - msg = build_skill_invocation_message("/test-skill", "do stuff") + result = build_skill_invocation_message("/test-skill", "do stuff") - assert msg is not None - assert "remote environment" in msg.lower() + assert result.ok + assert result.message is not None + assert "remote environment" in result.message.lower() def test_supporting_file_hint_uses_file_path_argument(self, tmp_path): with patch("tools.skills_tool.SKILLS_DIR", tmp_path): @@ -367,10 +392,11 @@ def test_supporting_file_hint_uses_file_path_argument(self, tmp_path): references.mkdir() (references / "api.md").write_text("reference") scan_skill_commands() - msg = build_skill_invocation_message("/test-skill", "do stuff") + result = build_skill_invocation_message("/test-skill", "do stuff") - assert msg is not None - assert 'file_path=""' in msg + assert result.ok + assert result.message is not None + assert 'file_path=""' in result.message class TestPlanSkillHelpers: @@ -390,7 +416,7 @@ def test_plan_skill_message_can_include_runtime_save_path_note(self, tmp_path): body="Save plans under .hermes/plans in the active workspace and do not execute the work.", ) scan_skill_commands() - msg = build_skill_invocation_message( + result = build_skill_invocation_message( "/plan", "Add a /plan command", runtime_note=( @@ -399,9 +425,10 @@ def test_plan_skill_message_can_include_runtime_save_path_note(self, tmp_path): ), ) - assert msg is not None - assert "Save plans under $HERMES_HOME/plans" not in msg - assert ".hermes/plans" in msg - assert "Add a /plan command" in msg - assert ".hermes/plans/plan.md" in msg - assert "Runtime note:" in msg + assert result.ok + assert result.message is not None + assert "Save plans under $HERMES_HOME/plans" not in result.message + assert ".hermes/plans" in result.message + assert "Add a /plan command" in result.message + assert ".hermes/plans/plan.md" in result.message + assert "Runtime note:" in result.message diff --git a/tests/cli/test_quick_commands.py b/tests/cli/test_quick_commands.py index 7a89d4ca28a7..7e0789ad9d32 100644 --- a/tests/cli/test_quick_commands.py +++ b/tests/cli/test_quick_commands.py @@ -4,6 +4,25 @@ from rich.text import Text import pytest +from agent.skill_commands import scan_skill_commands + + +def _make_skill(skills_dir, name, body="Do the thing."): + skill_dir = skills_dir / name + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text( + f"""--- +name: {name} +description: Description for {name}. +--- + +# {name} + +{body} +""" + ) + return skill_dir + # ā”€ā”€ CLI tests ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ @@ -23,6 +42,8 @@ def _make_cli(self, quick_commands): cli.console = MagicMock() cli.agent = None cli.conversation_history = [] + cli.session_id = "sess-123" + cli._pending_input = MagicMock() return cli def test_exec_command_runs_and_prints_output(self): @@ -92,6 +113,22 @@ def test_quick_command_takes_priority_over_skill_commands(self): printed = self._printed_plain(cli.console.print.call_args[0][0]) assert printed == "overridden" + def test_skill_load_failure_after_scan_does_not_queue_placeholder(self, tmp_path): + cli = self._make_cli({}) + + with patch("tools.skills_tool.SKILLS_DIR", tmp_path): + skill_dir = _make_skill(tmp_path, "test-skill") + skill_commands = scan_skill_commands() + (skill_dir / "SKILL.md").unlink() + with patch("cli._skill_commands", skill_commands), patch("cli.ChatConsole") as mock_console: + result = cli.process_command("/test-skill do stuff") + + assert result is True + cli._pending_input.put.assert_not_called() + mock_console.return_value.print.assert_called_once() + printed = str(mock_console.return_value.print.call_args[0][0]) + assert "Failed to load skill for /test-skill" in printed + def test_unknown_command_still_shows_error(self): cli = self._make_cli({}) with patch("cli._cprint") as mock_cprint: diff --git a/tests/gateway/test_unknown_command.py b/tests/gateway/test_unknown_command.py index 4c644cb7362d..b41d82131b7a 100644 --- a/tests/gateway/test_unknown_command.py +++ b/tests/gateway/test_unknown_command.py @@ -7,10 +7,11 @@ from datetime import datetime from types import SimpleNamespace -from unittest.mock import AsyncMock, MagicMock +from unittest.mock import AsyncMock, MagicMock, patch import pytest +from agent.skill_commands import scan_skill_commands from gateway.config import GatewayConfig, Platform, PlatformConfig from gateway.platforms.base import MessageEvent from gateway.session import SessionEntry, SessionSource, build_session_key @@ -30,6 +31,23 @@ def _make_event(text: str) -> MessageEvent: return MessageEvent(text=text, source=_make_source(), message_id="m1") +def _make_skill(skills_dir, name, body="Do the thing."): + skill_dir = skills_dir / name + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text( + f"""--- +name: {name} +description: Description for {name}. +--- + +# {name} + +{body} +""" + ) + return skill_dir + + def _make_runner(): from gateway.run import GatewayRunner @@ -142,6 +160,35 @@ async def test_known_slash_command_not_flagged_as_unknown(monkeypatch): assert "Unknown command" not in result +@pytest.mark.asyncio +async def test_known_skill_load_failure_not_flagged_as_unknown(monkeypatch, tmp_path): + """A discovered skill that fails to load later should return a load error, + not the unknown-command guard or a forwarded agent turn.""" + import gateway.run as gateway_run + + runner = _make_runner() + runner._run_agent = AsyncMock( + side_effect=AssertionError( + "failed skill command should not be forwarded to the agent" + ) + ) + + monkeypatch.setattr( + gateway_run, "_resolve_runtime_agent_kwargs", lambda: {"api_key": "***"} + ) + + with patch("tools.skills_tool.SKILLS_DIR", tmp_path): + skill_dir = _make_skill(tmp_path, "test-skill") + scan_skill_commands() + (skill_dir / "SKILL.md").unlink() + result = await runner._handle_message(_make_event("/test-skill do stuff")) + + assert result is not None + assert "Failed to load" in result + assert "Unknown command" not in result + runner._run_agent.assert_not_called() + + @pytest.mark.asyncio async def test_underscored_alias_for_hyphenated_builtin_not_flagged(monkeypatch): """Telegram autocomplete sends /reload_mcp for the /reload-mcp built-in. diff --git a/tests/gateway/test_webhook_integration.py b/tests/gateway/test_webhook_integration.py index 5c6fe0111103..f4f76c1a90ed 100644 --- a/tests/gateway/test_webhook_integration.py +++ b/tests/gateway/test_webhook_integration.py @@ -17,6 +17,7 @@ from aiohttp import web from aiohttp.test_utils import TestClient, TestServer +from agent.skill_commands import SkillInvocationResult from gateway.config import ( GatewayConfig, HomeChannel, @@ -180,7 +181,12 @@ async def _capture(event: MessageEvent): # The imports are lazy (inside the handler), so patch the source module with patch( "agent.skill_commands.build_skill_invocation_message", - return_value=skill_content, + return_value=SkillInvocationResult( + status="ok", + command="/code-review", + message=skill_content, + skill_name="code-review", + ), ) as mock_build, patch( "agent.skill_commands.get_skill_commands", return_value={"/code-review": {"name": "code-review"}}, @@ -205,6 +211,56 @@ async def _capture(event: MessageEvent): assert "You are a code reviewer" in event.text mock_build.assert_called_once() + @pytest.mark.asyncio + async def test_skill_load_failure_keeps_rendered_prompt(self): + """A configured webhook skill that fails to load should not inject a + bogus placeholder prompt into the downstream agent event.""" + routes = { + "pr-review": { + "secret": _INSECURE_NO_AUTH, + "events": ["pull_request"], + "prompt": "Review this PR: {pull_request.title}", + "skills": ["code-review"], + } + } + adapter = _make_adapter(routes) + + captured_events: list[MessageEvent] = [] + + async def _capture(event: MessageEvent): + captured_events.append(event) + + adapter.handle_message = _capture + + with patch( + "agent.skill_commands.build_skill_invocation_message", + return_value=SkillInvocationResult( + status="load_failed", + command="/code-review", + skill_name="code-review", + ), + ), patch( + "agent.skill_commands.get_skill_commands", + return_value={"/code-review": {"name": "code-review"}}, + ): + app = _create_app(adapter) + async with TestClient(TestServer(app)) as cli: + resp = await cli.post( + "/webhooks/pr-review", + json=GITHUB_PR_PAYLOAD, + headers={ + "X-GitHub-Event": "pull_request", + "X-GitHub-Delivery": "skill-failure-001", + }, + ) + assert resp.status == 202 + + await asyncio.sleep(0.05) + + assert len(captured_events) == 1 + event = captured_events[0] + assert event.text == "Review this PR: Add webhook adapter" + # =================================================================== # Test 3: Cross-platform delivery (webhook ā†’ Telegram)