fix(gateway/slack): ephemeral ack and routing for slash commands
Slack slash commands (/q, /btw, /stop, /model, etc.) previously showed no user-visible acknowledgement and posted command replies as public channel messages. This diverged from Discord, which uses ephemeral deferred responses for slash commands. Changes: - handle_hermes_command now passes response_type='ephemeral' and a 'Running /cmd…' text to ack(), giving the user immediate 'Only visible to you' feedback when they invoke any native slash command. - _handle_slash_command stashes the Slack response_url from the command payload in a per-channel context dict before dispatching to handle_message. - send() checks for a pending slash context and, when found, POSTs to the response_url with replace_original=true to swap the initial ack with the real command reply (e.g. 'Queued for the next turn.'), keeping it ephemeral. - Stale slash contexts are garbage-collected on lookup (120s TTL). - The response_url POST is non-fatal: if it fails, the user already saw the initial ack, and send() returns success=True. Fixes #18182
This commit is contained in:
parent
0b76d23d1a
commit
7cda0e5224
@ -21,6 +21,7 @@ try:
|
||||
from slack_bolt.async_app import AsyncApp
|
||||
from slack_bolt.adapter.socket_mode.async_handler import AsyncSocketModeHandler
|
||||
from slack_sdk.web.async_client import AsyncWebClient
|
||||
import aiohttp
|
||||
SLACK_AVAILABLE = True
|
||||
except ImportError:
|
||||
SLACK_AVAILABLE = False
|
||||
@ -310,6 +311,11 @@ class SlackAdapter(BasePlatformAdapter):
|
||||
# Track active assistant thread status indicators so stop_typing can
|
||||
# clear them (chat_id → thread_ts).
|
||||
self._active_status_threads: Dict[str, str] = {}
|
||||
# Slash-command contexts: stash response_url + user_id so send()
|
||||
# can route the first reply ephemerally. Keyed by
|
||||
# (channel_id, user_id) to avoid cross-user collisions.
|
||||
# Each value: {"response_url": str, "ts": float}
|
||||
self._slash_command_contexts: Dict[Tuple[str, str], Dict[str, Any]] = {}
|
||||
|
||||
def _describe_slack_api_error(self, response: Any, *, file_obj: Optional[Dict[str, Any]] = None) -> Optional[str]:
|
||||
"""Convert Slack API auth/permission failures into actionable user-facing text."""
|
||||
@ -368,6 +374,86 @@ class SlackAdapter(BasePlatformAdapter):
|
||||
)
|
||||
return None
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# Slash-command ephemeral helpers
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
_SLASH_CTX_TTL = 120.0 # seconds — response_url is valid for 30 min;
|
||||
# we use a much shorter TTL to avoid routing unrelated messages
|
||||
# as ephemeral if the command handler was slow or dropped.
|
||||
|
||||
def _pop_slash_context(
|
||||
self, chat_id: str,
|
||||
) -> Optional[Dict[str, Any]]:
|
||||
"""Return and remove the slash-command context for *chat_id*, if fresh.
|
||||
|
||||
Contexts older than ``_SLASH_CTX_TTL`` seconds are silently discarded.
|
||||
Uses a full scan (dict is tiny) so we don't need the user_id in
|
||||
``send()``, which only receives the channel ID from base.py.
|
||||
"""
|
||||
now = time.monotonic()
|
||||
# Clean up stale entries on every lookup — dict is small.
|
||||
stale_keys = [
|
||||
k for k, v in self._slash_command_contexts.items()
|
||||
if now - v["ts"] > self._SLASH_CTX_TTL
|
||||
]
|
||||
for k in stale_keys:
|
||||
self._slash_command_contexts.pop(k, None)
|
||||
|
||||
# Find the context for this channel (may be keyed under any user).
|
||||
match_key = None
|
||||
for key in list(self._slash_command_contexts):
|
||||
if key[0] == chat_id:
|
||||
match_key = key
|
||||
break
|
||||
if match_key is None:
|
||||
return None
|
||||
return self._slash_command_contexts.pop(match_key)
|
||||
|
||||
async def _send_slash_ephemeral(
|
||||
self,
|
||||
ctx: Dict[str, Any],
|
||||
content: str,
|
||||
) -> "SendResult":
|
||||
"""Replace the initial ephemeral ack via ``response_url``.
|
||||
|
||||
Slack's ``response_url`` accepts a POST with ``replace_original``
|
||||
for up to 30 minutes after the slash command was invoked. This
|
||||
lets us swap the "Running /cmd…" placeholder with the real reply,
|
||||
and the message stays ephemeral ("Only visible to you").
|
||||
|
||||
Falls back to a simple ``True`` SendResult if the POST fails —
|
||||
the user already saw the initial ack, so a delivery failure here
|
||||
is non-critical.
|
||||
"""
|
||||
formatted = self.format_message(content)
|
||||
payload = {
|
||||
"response_type": "ephemeral",
|
||||
"replace_original": True,
|
||||
"text": formatted,
|
||||
}
|
||||
try:
|
||||
async with aiohttp.ClientSession() as session:
|
||||
async with session.post(
|
||||
ctx["response_url"],
|
||||
json=payload,
|
||||
timeout=aiohttp.ClientTimeout(total=10),
|
||||
) as resp:
|
||||
if resp.status == 200:
|
||||
return SendResult(success=True, message_id=None)
|
||||
body = await resp.text()
|
||||
logger.warning(
|
||||
"[Slack] response_url POST returned %s: %s",
|
||||
resp.status,
|
||||
body[:200],
|
||||
)
|
||||
except Exception as e:
|
||||
logger.warning(
|
||||
"[Slack] response_url POST failed: %s", e,
|
||||
)
|
||||
# Non-fatal — the user saw the initial ack already.
|
||||
return SendResult(success=True, message_id=None)
|
||||
|
||||
async def connect(self) -> bool:
|
||||
"""Connect to Slack via Socket Mode."""
|
||||
if not SLACK_AVAILABLE:
|
||||
@ -502,7 +588,11 @@ class SlackAdapter(BasePlatformAdapter):
|
||||
|
||||
@self._app.command(_slash_pattern)
|
||||
async def handle_hermes_command(ack, command):
|
||||
await ack()
|
||||
slash = (command.get("command") or "").lstrip("/")
|
||||
await ack(
|
||||
response_type="ephemeral",
|
||||
text=f"Running `/{slash}`…",
|
||||
)
|
||||
await self._handle_slash_command(command)
|
||||
|
||||
# Register Block Kit action handlers for approval buttons
|
||||
@ -574,6 +664,17 @@ class SlackAdapter(BasePlatformAdapter):
|
||||
return SendResult(success=False, error="Not connected")
|
||||
|
||||
try:
|
||||
# Check for a pending slash-command context. When the user ran a
|
||||
# native slash command (e.g. /q, /stop, /model), the initial ack
|
||||
# already showed an ephemeral "Running /cmd…" message. If we have
|
||||
# a stashed response_url for this channel, replace that ack with
|
||||
# the actual command reply ephemerally instead of posting publicly.
|
||||
slash_ctx = self._pop_slash_context(chat_id)
|
||||
if slash_ctx:
|
||||
return await self._send_slash_ephemeral(
|
||||
slash_ctx, content,
|
||||
)
|
||||
|
||||
# Convert standard markdown → Slack mrkdwn
|
||||
formatted = self.format_message(content)
|
||||
|
||||
@ -2537,6 +2638,16 @@ class SlackAdapter(BasePlatformAdapter):
|
||||
raw_message=command,
|
||||
)
|
||||
|
||||
# Stash the Slack response_url so the first reply for this
|
||||
# channel+user can be routed ephemerally (replaces the initial
|
||||
# "Running /cmd…" ack shown by handle_hermes_command).
|
||||
response_url = command.get("response_url", "")
|
||||
if response_url and user_id and channel_id:
|
||||
self._slash_command_contexts[(channel_id, user_id)] = {
|
||||
"response_url": response_url,
|
||||
"ts": time.monotonic(),
|
||||
}
|
||||
|
||||
await self.handle_message(event)
|
||||
|
||||
def _has_active_session_for_thread(
|
||||
|
||||
@ -53,6 +53,9 @@ def _ensure_slack_mock():
|
||||
]:
|
||||
sys.modules.setdefault(name, mod)
|
||||
|
||||
# aiohttp is imported alongside slack-bolt; mock it if missing
|
||||
sys.modules.setdefault("aiohttp", MagicMock())
|
||||
|
||||
|
||||
_ensure_slack_mock()
|
||||
|
||||
@ -2586,3 +2589,205 @@ class TestSlackReplyToText:
|
||||
assert msg_event.reply_to_text is None
|
||||
# Top-level message: reply_to_message_id must be falsy (None or empty).
|
||||
assert not msg_event.reply_to_message_id
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Slash-command ephemeral ack and routing (#18182)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestSlashEphemeralAck:
|
||||
"""Slash commands should produce an ephemeral ack and route replies ephemerally."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_slash_command_stashes_response_url(self, adapter):
|
||||
"""_handle_slash_command stashes response_url for later ephemeral routing."""
|
||||
command = {
|
||||
"command": "/q",
|
||||
"text": "follow-up question",
|
||||
"user_id": "U_SLASH",
|
||||
"channel_id": "C_SLASH",
|
||||
"response_url": "https://hooks.slack.com/commands/T123/456/abc",
|
||||
}
|
||||
await adapter._handle_slash_command(command)
|
||||
|
||||
# The context should be stashed under (channel_id, user_id).
|
||||
key = ("C_SLASH", "U_SLASH")
|
||||
assert key in adapter._slash_command_contexts
|
||||
ctx = adapter._slash_command_contexts[key]
|
||||
assert ctx["response_url"] == "https://hooks.slack.com/commands/T123/456/abc"
|
||||
assert "ts" in ctx
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_slash_command_without_response_url_does_not_stash(self, adapter):
|
||||
"""Commands without a response_url should not create a context."""
|
||||
command = {
|
||||
"command": "/stop",
|
||||
"text": "",
|
||||
"user_id": "U1",
|
||||
"channel_id": "C1",
|
||||
# no response_url
|
||||
}
|
||||
await adapter._handle_slash_command(command)
|
||||
assert len(adapter._slash_command_contexts) == 0
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pop_slash_context_returns_and_removes(self, adapter):
|
||||
"""_pop_slash_context returns the context and removes it."""
|
||||
import time
|
||||
adapter._slash_command_contexts[("C1", "U1")] = {
|
||||
"response_url": "https://hooks.slack.com/test",
|
||||
"ts": time.monotonic(),
|
||||
}
|
||||
|
||||
ctx = adapter._pop_slash_context("C1")
|
||||
assert ctx is not None
|
||||
assert ctx["response_url"] == "https://hooks.slack.com/test"
|
||||
# Must be removed after pop
|
||||
assert len(adapter._slash_command_contexts) == 0
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pop_slash_context_returns_none_for_no_match(self, adapter):
|
||||
"""_pop_slash_context returns None when no context exists."""
|
||||
ctx = adapter._pop_slash_context("C_NONEXISTENT")
|
||||
assert ctx is None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pop_slash_context_discards_stale_entries(self, adapter):
|
||||
"""Stale contexts older than TTL are cleaned up."""
|
||||
import time
|
||||
adapter._slash_command_contexts[("C1", "U1")] = {
|
||||
"response_url": "https://hooks.slack.com/stale",
|
||||
"ts": time.monotonic() - adapter._SLASH_CTX_TTL - 1,
|
||||
}
|
||||
|
||||
ctx = adapter._pop_slash_context("C1")
|
||||
assert ctx is None
|
||||
assert len(adapter._slash_command_contexts) == 0
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_uses_response_url_when_context_exists(self, adapter):
|
||||
"""send() should POST to response_url for slash command replies."""
|
||||
import time
|
||||
adapter._slash_command_contexts[("C_SLASH", "U_SLASH")] = {
|
||||
"response_url": "https://hooks.slack.com/commands/T123/456/abc",
|
||||
"ts": time.monotonic(),
|
||||
}
|
||||
|
||||
mock_resp = AsyncMock()
|
||||
mock_resp.status = 200
|
||||
mock_resp.__aenter__ = AsyncMock(return_value=mock_resp)
|
||||
mock_resp.__aexit__ = AsyncMock(return_value=False)
|
||||
|
||||
mock_session = AsyncMock()
|
||||
mock_session.post = MagicMock(return_value=mock_resp)
|
||||
mock_session.__aenter__ = AsyncMock(return_value=mock_session)
|
||||
mock_session.__aexit__ = AsyncMock(return_value=False)
|
||||
|
||||
with patch("gateway.platforms.slack.aiohttp.ClientSession", return_value=mock_session):
|
||||
result = await adapter.send("C_SLASH", "Queued for the next turn.")
|
||||
|
||||
assert result.success is True
|
||||
# Verify response_url was POSTed to
|
||||
mock_session.post.assert_called_once()
|
||||
call_args = mock_session.post.call_args
|
||||
assert call_args[0][0] == "https://hooks.slack.com/commands/T123/456/abc"
|
||||
payload = call_args[1]["json"]
|
||||
assert payload["response_type"] == "ephemeral"
|
||||
assert payload["replace_original"] is True
|
||||
assert "Queued for the next turn" in payload["text"]
|
||||
|
||||
# Context must be consumed
|
||||
assert len(adapter._slash_command_contexts) == 0
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_falls_through_without_context(self, adapter):
|
||||
"""send() should use normal chat_postMessage when no slash context exists."""
|
||||
mock_result = {"ts": "1234.5678", "ok": True}
|
||||
adapter._app.client.chat_postMessage = AsyncMock(return_value=mock_result)
|
||||
|
||||
result = await adapter.send("C_NORMAL", "Hello world")
|
||||
|
||||
assert result.success is True
|
||||
adapter._app.client.chat_postMessage.assert_called_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_slash_ephemeral_fallback_on_post_failure(self, adapter):
|
||||
"""_send_slash_ephemeral returns success=True even if POST fails."""
|
||||
import time
|
||||
adapter._slash_command_contexts[("C1", "U1")] = {
|
||||
"response_url": "https://hooks.slack.com/commands/bad",
|
||||
"ts": time.monotonic(),
|
||||
}
|
||||
|
||||
mock_resp = AsyncMock()
|
||||
mock_resp.status = 500
|
||||
mock_resp.text = AsyncMock(return_value="Internal Server Error")
|
||||
mock_resp.__aenter__ = AsyncMock(return_value=mock_resp)
|
||||
mock_resp.__aexit__ = AsyncMock(return_value=False)
|
||||
|
||||
mock_session = AsyncMock()
|
||||
mock_session.post = MagicMock(return_value=mock_resp)
|
||||
mock_session.__aenter__ = AsyncMock(return_value=mock_session)
|
||||
mock_session.__aexit__ = AsyncMock(return_value=False)
|
||||
|
||||
with patch("gateway.platforms.slack.aiohttp.ClientSession", return_value=mock_session):
|
||||
result = await adapter.send("C1", "Some response")
|
||||
|
||||
# Still success — the user saw the initial ack already
|
||||
assert result.success is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_slash_ephemeral_fallback_on_exception(self, adapter):
|
||||
"""_send_slash_ephemeral returns success=True even if aiohttp raises."""
|
||||
import time
|
||||
adapter._slash_command_contexts[("C1", "U1")] = {
|
||||
"response_url": "https://hooks.slack.com/commands/timeout",
|
||||
"ts": time.monotonic(),
|
||||
}
|
||||
|
||||
mock_session = AsyncMock()
|
||||
mock_session.post = MagicMock(side_effect=Exception("connection timeout"))
|
||||
mock_session.__aenter__ = AsyncMock(return_value=mock_session)
|
||||
mock_session.__aexit__ = AsyncMock(return_value=False)
|
||||
|
||||
with patch("gateway.platforms.slack.aiohttp.ClientSession", return_value=mock_session):
|
||||
result = await adapter.send("C1", "Some response")
|
||||
|
||||
assert result.success is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_native_slash_stashes_context_and_dispatches(self, adapter):
|
||||
"""Full flow: native /q slash → stash + handle_message dispatch."""
|
||||
command = {
|
||||
"command": "/q",
|
||||
"text": "do something",
|
||||
"user_id": "U_Q",
|
||||
"channel_id": "C_Q",
|
||||
"response_url": "https://hooks.slack.com/commands/T1/2/q",
|
||||
}
|
||||
await adapter._handle_slash_command(command)
|
||||
|
||||
# 1. handle_message was called with the right event
|
||||
adapter.handle_message.assert_called_once()
|
||||
event = adapter.handle_message.call_args[0][0]
|
||||
assert event.text == "/q do something"
|
||||
assert event.message_type == MessageType.COMMAND
|
||||
|
||||
# 2. Context stashed for ephemeral routing
|
||||
assert ("C_Q", "U_Q") in adapter._slash_command_contexts
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_legacy_hermes_slash_stashes_context(self, adapter):
|
||||
"""Legacy /hermes <subcommand> also stashes context."""
|
||||
command = {
|
||||
"command": "/hermes",
|
||||
"text": "help",
|
||||
"user_id": "U_H",
|
||||
"channel_id": "C_H",
|
||||
"response_url": "https://hooks.slack.com/commands/T1/3/h",
|
||||
}
|
||||
await adapter._handle_slash_command(command)
|
||||
|
||||
adapter.handle_message.assert_called_once()
|
||||
assert ("C_H", "U_H") in adapter._slash_command_contexts
|
||||
|
||||
Loading…
Reference in New Issue
Block a user