From 747b8745d4c655ac4dc55c11cbe73979e79bdc37 Mon Sep 17 00:00:00 2001 From: levsky22 <56281588+LevSky22@users.noreply.github.com> Date: Mon, 30 Mar 2026 02:52:17 -0400 Subject: [PATCH 1/4] feat(slack): add button-based approval flow for dangerous commands --- gateway/platforms/base.py | 13 ++++ gateway/platforms/slack.py | 134 +++++++++++++++++++++++++++++++++++++ gateway/run.py | 45 +++++++++++++ 3 files changed, 192 insertions(+) diff --git a/gateway/platforms/base.py b/gateway/platforms/base.py index a023a972ec1e3..161d0d64c9d86 100644 --- a/gateway/platforms/base.py +++ b/gateway/platforms/base.py @@ -402,6 +402,7 @@ class SendResult: # Type for message handlers MessageHandler = Callable[[MessageEvent], Awaitable[Optional[str]]] +ApprovalActionHandler = Callable[[str, str], Awaitable[Optional[str]]] class BasePlatformAdapter(ABC): @@ -419,6 +420,7 @@ def __init__(self, config: PlatformConfig, platform: Platform): self.config = config self.platform = platform self._message_handler: Optional[MessageHandler] = None + self._approval_action_handler: Optional[ApprovalActionHandler] = None self._running = False self._fatal_error_code: Optional[str] = None self._fatal_error_message: Optional[str] = None @@ -518,6 +520,17 @@ def set_message_handler(self, handler: MessageHandler) -> None: an optional response string. """ self._message_handler = handler + + def set_approval_action_handler(self, handler: ApprovalActionHandler) -> None: + """ + Set the handler for platform-native approval actions. + + The handler receives ``(approval_id, action)`` where ``approval_id`` is + the opaque platform-provided approval token and ``action`` is one of + the platform's canonical approval actions such as ``approve``, + ``approve session``, or ``deny``. + """ + self._approval_action_handler = handler @abstractmethod async def connect(self) -> bool: diff --git a/gateway/platforms/slack.py b/gateway/platforms/slack.py index 2e7bbee739ba4..e817ad4b0c6ba 100644 --- a/gateway/platforms/slack.py +++ b/gateway/platforms/slack.py @@ -176,6 +176,21 @@ async def handle_hermes_command(ack, command): await ack() await self._handle_slash_command(command) + @self._app.action("hermes_approve_once") + async def handle_approve_once(ack, body, action): + await ack() + await self._handle_approval_action(body, "approve") + + @self._app.action("hermes_approve_session") + async def handle_approve_session(ack, body, action): + await ack() + await self._handle_approval_action(body, "approve session") + + @self._app.action("hermes_deny") + async def handle_deny(ack, body, action): + await ack() + await self._handle_approval_action(body, "deny") + # Start Socket Mode handler in background self._handler = AsyncSocketModeHandler(self._app, app_token) self._socket_mode_task = asyncio.create_task(self._handler.start_async()) @@ -933,6 +948,125 @@ async def _handle_slash_command(self, command: dict) -> None: await self.handle_message(event) + async def _handle_approval_action(self, body: dict, command_text: str) -> None: + """Handle Slack Block Kit approval buttons inside a thread or DM. + + Slack slash commands do not work inside threads, so approvals use + buttons that resolve the pending approval directly by approval ID. + """ + if not self._approval_action_handler: + return + + channel = body.get("channel") or {} + user = body.get("user") or {} + message = body.get("message") or {} + container = body.get("container") or {} + actions = body.get("actions") or [] + + channel_id = channel.get("id", "") + user_id = user.get("id", "") + approval_id = actions[0].get("value", "") if actions else "" + + actual_thread_ts = ( + container.get("thread_ts") + or message.get("thread_ts") + or message.get("ts") + ) + + try: + response = await self._approval_action_handler(approval_id, command_text) + except Exception as e: # pragma: no cover - defensive logging + logger.error("[Slack] Approval action failed: %s", e, exc_info=True) + response = f"❌ Approval action failed: {e}" + + try: + action_label = { + "approve": "Approved once", + "approve session": "Approved for session", + "deny": "Denied", + }.get(command_text, command_text) + await self._app.client.chat_update( + channel=channel_id, + ts=container.get("message_ts") or message.get("ts"), + text=f"✅ {action_label} by <@{user_id}>", + blocks=[], + ) + except Exception: + pass + + if response: + metadata = {"thread_id": actual_thread_ts} if actual_thread_ts else None + await self.send(channel_id, response, metadata=metadata) + + async def send_exec_approval( + self, + chat_id: str, + command: str, + approval_id: str, + reply_to: Optional[str] = None, + metadata: Optional[Dict[str, Any]] = None, + ) -> SendResult: + """Send a Slack Block Kit approval prompt for a dangerous command.""" + if not self._app: + return SendResult(success=False, error="Not connected") + + try: + max_text = 2700 + cmd_display = command if len(command) <= max_text else command[: max_text - 3] + "..." + thread_ts = self._resolve_thread_ts(reply_to, metadata) + + blocks = [ + { + "type": "section", + "text": { + "type": "mrkdwn", + "text": ( + "*Dangerous command requires approval*\n" + f"```{cmd_display}```" + ), + }, + }, + { + "type": "actions", + "elements": [ + { + "type": "button", + "text": {"type": "plain_text", "text": "Approve Once"}, + "style": "primary", + "action_id": "hermes_approve_once", + "value": approval_id, + }, + { + "type": "button", + "text": {"type": "plain_text", "text": "Approve Session"}, + "action_id": "hermes_approve_session", + "value": approval_id, + }, + { + "type": "button", + "text": {"type": "plain_text", "text": "Deny"}, + "style": "danger", + "action_id": "hermes_deny", + "value": approval_id, + }, + ], + }, + ] + + kwargs = { + "channel": chat_id, + "text": "Dangerous command requires approval", + "blocks": blocks, + } + if thread_ts: + kwargs["thread_ts"] = thread_ts + + result = await self._app.client.chat_postMessage(**kwargs) + return SendResult(success=True, message_id=result.get("ts"), raw_response=result) + except Exception as e: # pragma: no cover - defensive logging + logger.error("[Slack] Failed to send approval prompt: %s", e, exc_info=True) + return SendResult(success=False, error=str(e)) + async def _download_slack_file(self, url: str, ext: str, audio: bool = False, team_id: str = "") -> str: """Download a Slack file using the bot token for auth, with retry.""" import asyncio diff --git a/gateway/run.py b/gateway/run.py index 52bc9f7a0d802..35d7358dd3a9d 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -1115,6 +1115,7 @@ async def start(self) -> bool: # Set up message + fatal error handlers adapter.set_message_handler(self._handle_message) + adapter.set_approval_action_handler(self._handle_platform_approval_action) adapter.set_fatal_error_handler(self._handle_adapter_fatal_error) # Try to connect @@ -1362,6 +1363,7 @@ async def _platform_reconnect_watcher(self) -> None: continue adapter.set_message_handler(self._handle_message) + adapter.set_approval_action_handler(self._handle_platform_approval_action) adapter.set_fatal_error_handler(self._handle_adapter_fatal_error) success = await adapter.connect() @@ -5094,6 +5096,49 @@ async def _handle_reload_mcp_command(self, event: MessageEvent) -> str: _APPROVAL_TIMEOUT_SECONDS = 300 # 5 minutes + async def _handle_platform_approval_action( + self, + approval_id: str, + action: str, + ) -> str: + """Resolve a pending dangerous-command approval by approval ID. + + Messaging platforms with native UI controls, such as Slack buttons, + should call this directly instead of synthesizing a new slash-command + message. The approval ID is the stored session key for the pending + command. Signals the blocked agent thread via the blocking gateway + approval mechanism in tools/approval.py. + """ + session_key = (approval_id or "").strip() + if not session_key: + return "Invalid approval request." + + canonical = action.strip().lower() + if canonical not in {"approve", "approve session", "deny"}: + return f"Unsupported approval action: {action}" + + from tools.approval import resolve_gateway_approval, has_blocking_approval + + if not has_blocking_approval(session_key): + return "No pending command to approve." + + choice = "deny" if canonical == "deny" else ("session" if canonical == "approve session" else "once") + scope_msg = " (pattern approved for this session)" if choice == "session" else "" + + count = resolve_gateway_approval(session_key, choice) + if not count: + return "No pending command to approve." + + if choice == "deny": + logger.info("User denied dangerous command via platform approval UI") + return "❌ Command denied." + + logger.info( + "User approved dangerous command via platform approval UI%s", + scope_msg, + ) + return f"✅ Command approved{scope_msg}. The agent is resuming..." + async def _handle_approve_command(self, event: MessageEvent) -> Optional[str]: """Handle /approve command — unblock waiting agent thread(s). From b3ab4ceb1a9aea711d5b7b1cd05411c81f8e8c2f Mon Sep 17 00:00:00 2001 From: levsky22 <56281588+LevSky22@users.noreply.github.com> Date: Mon, 30 Mar 2026 02:59:00 -0400 Subject: [PATCH 2/4] test(gateway): cover Slack button approvals --- tests/gateway/test_approve_deny_commands.py | 69 +++++++++++++++++++++ tests/gateway/test_slack.py | 60 ++++++++++++++++++ 2 files changed, 129 insertions(+) diff --git a/tests/gateway/test_approve_deny_commands.py b/tests/gateway/test_approve_deny_commands.py index 18f3009b0de2b..94f9fe3b8d732 100644 --- a/tests/gateway/test_approve_deny_commands.py +++ b/tests/gateway/test_approve_deny_commands.py @@ -338,6 +338,75 @@ async def test_deny_no_pending(self): assert "No pending command" in result +class TestPlatformApprovalActions: + + @pytest.mark.asyncio + async def test_platform_approve_once_executes_pending_command(self): + runner = _make_runner() + source = _make_source() + session_key = runner._session_key_for_source(source) + runner._pending_approvals[session_key] = _make_pending_approval() + + with ( + patch("tools.terminal_tool.terminal_tool", return_value="done") as mock_term, + patch("tools.approval.approve_session") as mock_session, + ): + result = await runner._handle_platform_approval_action(session_key, "approve") + + assert "✅ Command approved and executed" in result + mock_session.assert_called_once_with(session_key, "sudo") + mock_term.assert_called_once_with(command="sudo rm -rf /tmp/test", force=True) + assert session_key not in runner._pending_approvals + + @pytest.mark.asyncio + async def test_platform_approve_session_marks_session_scope(self): + runner = _make_runner() + source = _make_source() + session_key = runner._session_key_for_source(source) + runner._pending_approvals[session_key] = _make_pending_approval() + + with ( + patch("tools.terminal_tool.terminal_tool", return_value="done"), + patch("tools.approval.approve_session") as mock_session, + ): + result = await runner._handle_platform_approval_action(session_key, "approve session") + + assert "pattern approved for this session" in result + mock_session.assert_called_once_with(session_key, "sudo") + + @pytest.mark.asyncio + async def test_platform_deny_clears_pending(self): + runner = _make_runner() + source = _make_source() + session_key = runner._session_key_for_source(source) + runner._pending_approvals[session_key] = _make_pending_approval() + + result = await runner._handle_platform_approval_action(session_key, "deny") + + assert "❌ Command denied" in result + assert session_key not in runner._pending_approvals + + @pytest.mark.asyncio + async def test_platform_approval_invalid_action(self): + runner = _make_runner() + result = await runner._handle_platform_approval_action("approval-1", "maybe") + assert "Unsupported approval action" in result + + @pytest.mark.asyncio + async def test_platform_approval_expired(self): + runner = _make_runner() + source = _make_source() + session_key = runner._session_key_for_source(source) + approval = _make_pending_approval() + approval["timestamp"] = time.time() - 600 + runner._pending_approvals[session_key] = approval + + result = await runner._handle_platform_approval_action(session_key, "approve") + + assert "expired" in result + assert session_key not in runner._pending_approvals + + # ------------------------------------------------------------------ # Bare "yes" must NOT trigger approval # ------------------------------------------------------------------ diff --git a/tests/gateway/test_slack.py b/tests/gateway/test_slack.py index 81f8077ad6b35..a3d81b1aeb197 100644 --- a/tests/gateway/test_slack.py +++ b/tests/gateway/test_slack.py @@ -103,6 +103,7 @@ def test_app_mention_registered_on_connect(self): # Track which events get registered registered_events = [] registered_commands = [] + registered_actions = [] mock_app = MagicMock() @@ -118,8 +119,15 @@ def decorator(fn): return fn return decorator + def mock_action(action_id): + def decorator(fn): + registered_actions.append(action_id) + return fn + return decorator + mock_app.event = mock_event mock_app.command = mock_command + mock_app.action = mock_action mock_app.client = AsyncMock() mock_app.client.auth_test = AsyncMock(return_value={ "user_id": "U_BOT", @@ -146,6 +154,58 @@ def decorator(fn): assert "message" in registered_events assert "app_mention" in registered_events assert "/hermes" in registered_commands + assert "hermes_approve_once" in registered_actions + assert "hermes_approve_session" in registered_actions + assert "hermes_deny" in registered_actions + + +class TestSlackApprovalButtons: + @pytest.mark.asyncio + async def test_send_exec_approval_posts_buttons(self, adapter): + adapter._app.client.chat_postMessage = AsyncMock(return_value={"ts": "123.456"}) + + result = await adapter.send_exec_approval( + chat_id="C123", + command="rm -rf /tmp/test", + approval_id="approval-1", + metadata={"thread_id": "123.000"}, + ) + + assert result.success + call_kwargs = adapter._app.client.chat_postMessage.call_args.kwargs + assert call_kwargs["channel"] == "C123" + assert call_kwargs["thread_ts"] == "123.000" + assert call_kwargs["blocks"][1]["elements"][0]["action_id"] == "hermes_approve_once" + assert call_kwargs["blocks"][1]["elements"][1]["action_id"] == "hermes_approve_session" + assert call_kwargs["blocks"][1]["elements"][2]["action_id"] == "hermes_deny" + + @pytest.mark.asyncio + async def test_handle_approval_action_invokes_platform_handler_and_updates_thread(self, adapter): + adapter._approval_action_handler = AsyncMock(return_value="approved output") + adapter.send = AsyncMock(return_value=SendResult(success=True)) + adapter._app.client.chat_update = AsyncMock() + + body = { + "channel": {"id": "C123"}, + "user": {"id": "U123"}, + "container": {"message_ts": "200.000", "thread_ts": "100.000"}, + "message": {"ts": "200.000", "thread_ts": "100.000"}, + "actions": [{"value": "approval-1"}], + } + + await adapter._handle_approval_action(body, "approve session") + + adapter._approval_action_handler.assert_awaited_once_with("approval-1", "approve session") + adapter._app.client.chat_update.assert_awaited_once() + update_kwargs = adapter._app.client.chat_update.call_args.kwargs + assert update_kwargs["channel"] == "C123" + assert update_kwargs["ts"] == "200.000" + assert "Approved for session" in update_kwargs["text"] + adapter.send.assert_awaited_once_with( + "C123", + "approved output", + metadata={"thread_id": "100.000"}, + ) # --------------------------------------------------------------------------- From fb086a40f9bac6cf82fb6a36e8095925f98d0df0 Mon Sep 17 00:00:00 2001 From: levsky22 <56281588+LevSky22@users.noreply.github.com> Date: Sun, 5 Apr 2026 15:01:10 -0400 Subject: [PATCH 3/4] fix(slack): adapt approval button flow to blocking gateway mechanism The original PR was built on the old post-loop pop_pending approval architecture, which upstream replaced with a blocking mechanism in tools/approval.py (agent thread blocks until user responds via /approve or /deny). Rebased onto current main and adapted: - _handle_platform_approval_action now calls has_blocking_approval / resolve_gateway_approval instead of terminal_tool(force=True) and self._pending_approvals, matching the blocking flow used by _handle_approve_command and _handle_deny_command. - Updated TestPlatformApprovalActions tests to mock the blocking approval API (has_blocking_approval, resolve_gateway_approval) instead of the removed _pending_approvals dict. Replaced the timestamp-expiry test (no longer applicable) with a no-pending test. - Preserved _download_slack_file team_id parameter added upstream. All 93 tests in test_slack.py and test_approve_deny_commands.py pass. --- tests/gateway/test_approve_deny_commands.py | 39 ++++++++++----------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/tests/gateway/test_approve_deny_commands.py b/tests/gateway/test_approve_deny_commands.py index 94f9fe3b8d732..e7e90e7fe922e 100644 --- a/tests/gateway/test_approve_deny_commands.py +++ b/tests/gateway/test_approve_deny_commands.py @@ -345,46 +345,45 @@ async def test_platform_approve_once_executes_pending_command(self): runner = _make_runner() source = _make_source() session_key = runner._session_key_for_source(source) - runner._pending_approvals[session_key] = _make_pending_approval() with ( - patch("tools.terminal_tool.terminal_tool", return_value="done") as mock_term, - patch("tools.approval.approve_session") as mock_session, + patch("tools.approval.has_blocking_approval", return_value=True), + patch("tools.approval.resolve_gateway_approval", return_value=1) as mock_resolve, ): result = await runner._handle_platform_approval_action(session_key, "approve") - assert "✅ Command approved and executed" in result - mock_session.assert_called_once_with(session_key, "sudo") - mock_term.assert_called_once_with(command="sudo rm -rf /tmp/test", force=True) - assert session_key not in runner._pending_approvals + assert "✅ Command approved" in result + mock_resolve.assert_called_once_with(session_key, "once") @pytest.mark.asyncio async def test_platform_approve_session_marks_session_scope(self): runner = _make_runner() source = _make_source() session_key = runner._session_key_for_source(source) - runner._pending_approvals[session_key] = _make_pending_approval() with ( - patch("tools.terminal_tool.terminal_tool", return_value="done"), - patch("tools.approval.approve_session") as mock_session, + patch("tools.approval.has_blocking_approval", return_value=True), + patch("tools.approval.resolve_gateway_approval", return_value=1) as mock_resolve, ): result = await runner._handle_platform_approval_action(session_key, "approve session") assert "pattern approved for this session" in result - mock_session.assert_called_once_with(session_key, "sudo") + mock_resolve.assert_called_once_with(session_key, "session") @pytest.mark.asyncio async def test_platform_deny_clears_pending(self): runner = _make_runner() source = _make_source() session_key = runner._session_key_for_source(source) - runner._pending_approvals[session_key] = _make_pending_approval() - result = await runner._handle_platform_approval_action(session_key, "deny") + with ( + patch("tools.approval.has_blocking_approval", return_value=True), + patch("tools.approval.resolve_gateway_approval", return_value=1) as mock_resolve, + ): + result = await runner._handle_platform_approval_action(session_key, "deny") assert "❌ Command denied" in result - assert session_key not in runner._pending_approvals + mock_resolve.assert_called_once_with(session_key, "deny") @pytest.mark.asyncio async def test_platform_approval_invalid_action(self): @@ -393,18 +392,16 @@ async def test_platform_approval_invalid_action(self): assert "Unsupported approval action" in result @pytest.mark.asyncio - async def test_platform_approval_expired(self): + async def test_platform_approval_no_pending(self): + """Returns a clear message when no blocked approval exists for the session.""" runner = _make_runner() source = _make_source() session_key = runner._session_key_for_source(source) - approval = _make_pending_approval() - approval["timestamp"] = time.time() - 600 - runner._pending_approvals[session_key] = approval - result = await runner._handle_platform_approval_action(session_key, "approve") + with patch("tools.approval.has_blocking_approval", return_value=False): + result = await runner._handle_platform_approval_action(session_key, "approve") - assert "expired" in result - assert session_key not in runner._pending_approvals + assert "No pending command" in result # ------------------------------------------------------------------ From 86f87256e726b0c23fe810ecb227fc1becd6d9d1 Mon Sep 17 00:00:00 2001 From: levsky22 <56281588+LevSky22@users.noreply.github.com> Date: Sun, 5 Apr 2026 15:45:24 -0400 Subject: [PATCH 4/4] fix(gateway): approve-once must not grant session-wide approval When a gateway user chose "once", approve_session() was still being called (because the condition tested `choice in ("once", "session")`), silently adding the pattern to the session allowlist and making every subsequent identical command skip the prompt. Fix: only call approve_session() when choice is "session" or "always". Also adds a regression test (TestApproveOnceScope) that runs two dangerous commands sequentially, approves the first with "once", and asserts the second still triggers a fresh prompt. Refs: NousResearch/hermes-agent#4698 Co-Authored-By: Claude Sonnet 4.6 --- tests/gateway/test_approve_deny_commands.py | 83 +++++++++++++++++++++ tools/approval.py | 2 +- 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/tests/gateway/test_approve_deny_commands.py b/tests/gateway/test_approve_deny_commands.py index e7e90e7fe922e..5331712a6fc5a 100644 --- a/tests/gateway/test_approve_deny_commands.py +++ b/tests/gateway/test_approve_deny_commands.py @@ -705,3 +705,86 @@ def test_no_callback_returns_approval_required(self): assert result["approved"] is False assert result.get("status") == "approval_required" + + +# ------------------------------------------------------------------ +# Regression: "Approve Once" must not grant session-wide approval +# Fix: NousResearch/hermes-agent#4698 +# ------------------------------------------------------------------ + + +class TestApproveOnceScope: + + def setup_method(self): + _clear_approval_state() + + def test_approve_once_does_not_grant_session_wide_approval(self): + """Approving with 'once' must not add the pattern to the session allowlist. + + Regression test for the bug where choice in ("once", "session") caused + approve_session() to be called for both choices, making a one-time + approval silently cover all subsequent identical commands in the session. + """ + from tools.approval import ( + register_gateway_notify, + unregister_gateway_notify, + resolve_gateway_approval, + check_all_command_guards, + reset_current_session_key, + set_current_session_key, + ) + + session_key = "once-scope-test" + notified = [] + register_gateway_notify(session_key, lambda d: notified.append(d)) + + results = [None, None] + + def run_command(index, command): + token = set_current_session_key(session_key) + os.environ["HERMES_EXEC_ASK"] = "1" + os.environ["HERMES_SESSION_KEY"] = session_key + try: + results[index] = check_all_command_guards(command, "local") + finally: + os.environ.pop("HERMES_EXEC_ASK", None) + os.environ.pop("HERMES_SESSION_KEY", None) + reset_current_session_key(token) + + # First command — run and approve with "once" + t1 = threading.Thread(target=run_command, args=(0, "rm -rf /first")) + t1.start() + + for _ in range(50): + if notified: + break + time.sleep(0.05) + + assert len(notified) == 1, "First command should have triggered a notification" + resolve_gateway_approval(session_key, "once") + t1.join(timeout=5) + + assert results[0] is not None + assert results[0]["approved"] is True + + # Second command with the same dangerous pattern — must prompt again + t2 = threading.Thread(target=run_command, args=(1, "rm -rf /second")) + t2.start() + + for _ in range(50): + if len(notified) >= 2: + break + time.sleep(0.05) + + assert len(notified) == 2, ( + "Second command should have prompted again — 'once' must not grant " + "session-wide approval" + ) + + resolve_gateway_approval(session_key, "once") + t2.join(timeout=5) + + assert results[1] is not None + assert results[1]["approved"] is True + + unregister_gateway_notify(session_key) diff --git a/tools/approval.py b/tools/approval.py index c47672b8bb372..8f64f02ad122f 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -813,7 +813,7 @@ def check_all_command_guards(command: str, env_type: str, # User approved — persist based on scope (same logic as CLI) for key, _, is_tirith in warnings: - if choice in ("once", "session") or (choice == "always" and is_tirith): + if choice == "session" or (choice == "always" and is_tirith): approve_session(session_key, key) elif choice == "always": approve_session(session_key, key)