From bd707103ef481aa13d71e18b669cce1c00f4d914 Mon Sep 17 00:00:00 2001 From: Soynchux Date: Wed, 22 Jul 2026 08:18:28 -0700 Subject: [PATCH] fix(slack): stop consuming slash reply context outside slash sends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _pop_slash_context fell back to a channel-only scan when the _slash_user_id ContextVar was unset (i.e. send() invoked from a non-slash code path such as a cron delivery or a normal channel reply). That scan could steal another user's pending slash reply context: the normal message got swallowed into an ephemeral response_url POST that replaces the invoker's ack, and the slash invoker's actual reply then posted publicly. Remove the fallback — when the ContextVar is unset, match nothing. Surgical reapply of PR #26788 (originally against gateway/platforms/slack.py). --- plugins/platforms/slack/adapter.py | 17 ++----- tests/gateway/test_slack.py | 71 +++++++++++++++++++++++------- 2 files changed, 58 insertions(+), 30 deletions(-) diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 6c86ecd6a62e..6a75a4aacaf5 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -1117,9 +1117,9 @@ class SlackAdapter(BasePlatformAdapter): Uses the ``_slash_user_id`` ContextVar (set in ``_handle_slash_command``) to match the exact ``(channel_id, user_id)`` key. This prevents a concurrent slash command from a different user on the same channel from - stealing another user's ephemeral context. Falls back to a - channel-only scan when the ContextVar is unset (e.g. send() called - from a non-slash code path — should not match anything). + stealing another user's ephemeral context. When the ContextVar is + unset (e.g. send() called from a non-slash code path), do not match + anything — otherwise normal sends can steal a pending slash reply. """ now = time.monotonic() # Clean up stale entries on every lookup — dict is small. @@ -1136,16 +1136,7 @@ class SlackAdapter(BasePlatformAdapter): if uid: return self._slash_command_contexts.pop((chat_id, uid), None) - # Fallback: channel-only scan (only reachable when ContextVar is - # unset, i.e. send() called outside a slash-command async context). - 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) + return None async def _send_slash_ephemeral( self, diff --git a/tests/gateway/test_slack.py b/tests/gateway/test_slack.py index fbcaa514fe87..fcdcc31657c8 100644 --- a/tests/gateway/test_slack.py +++ b/tests/gateway/test_slack.py @@ -5218,13 +5218,18 @@ class TestSlashEphemeralAck: async def test_pop_slash_context_returns_and_removes(self, adapter): """_pop_slash_context returns the context and removes it.""" import time + from plugins.platforms.slack.adapter import _slash_user_id adapter._slash_command_contexts[("C1", "U1")] = { "response_url": "https://hooks.slack.com/test", "ts": time.monotonic(), } - ctx = adapter._pop_slash_context("C1") + token = _slash_user_id.set("U1") + try: + ctx = adapter._pop_slash_context("C1") + finally: + _slash_user_id.reset(token) assert ctx is not None assert ctx["response_url"] == "https://hooks.slack.com/test" # Must be removed after pop @@ -5254,6 +5259,7 @@ class TestSlashEphemeralAck: async def test_send_uses_response_url_when_context_exists(self, adapter): """send() should POST to response_url for slash command replies.""" import time + from plugins.platforms.slack.adapter import _slash_user_id adapter._slash_command_contexts[("C_SLASH", "U_SLASH")] = { "response_url": "https://hooks.slack.com/commands/T123/456/abc", @@ -5270,10 +5276,14 @@ class TestSlashEphemeralAck: mock_session.__aenter__ = AsyncMock(return_value=mock_session) mock_session.__aexit__ = AsyncMock(return_value=False) - with patch( - "plugins.platforms.slack.adapter.aiohttp.ClientSession", return_value=mock_session - ): - result = await adapter.send("C_SLASH", "Queued for the next turn.") + token = _slash_user_id.set("U_SLASH") + try: + with patch( + "plugins.platforms.slack.adapter.aiohttp.ClientSession", return_value=mock_session + ): + result = await adapter.send("C_SLASH", "Queued for the next turn.") + finally: + _slash_user_id.reset(token) assert result.success is True # Verify response_url was POSTed to @@ -5303,6 +5313,7 @@ class TestSlashEphemeralAck: async def test_send_slash_ephemeral_fallback_on_post_failure(self, adapter): """_send_slash_ephemeral returns success=True even if POST fails.""" import time + from plugins.platforms.slack.adapter import _slash_user_id adapter._slash_command_contexts[("C1", "U1")] = { "response_url": "https://hooks.slack.com/commands/bad", @@ -5320,10 +5331,14 @@ class TestSlashEphemeralAck: mock_session.__aenter__ = AsyncMock(return_value=mock_session) mock_session.__aexit__ = AsyncMock(return_value=False) - with patch( - "plugins.platforms.slack.adapter.aiohttp.ClientSession", return_value=mock_session - ): - result = await adapter.send("C1", "Some response") + token = _slash_user_id.set("U1") + try: + with patch( + "plugins.platforms.slack.adapter.aiohttp.ClientSession", return_value=mock_session + ): + result = await adapter.send("C1", "Some response") + finally: + _slash_user_id.reset(token) # Still success — the user saw the initial ack already assert result.success is True @@ -5332,6 +5347,7 @@ class TestSlashEphemeralAck: async def test_send_slash_ephemeral_fallback_on_exception(self, adapter): """_send_slash_ephemeral returns success=True even if aiohttp raises.""" import time + from plugins.platforms.slack.adapter import _slash_user_id adapter._slash_command_contexts[("C1", "U1")] = { "response_url": "https://hooks.slack.com/commands/timeout", @@ -5343,10 +5359,14 @@ class TestSlashEphemeralAck: mock_session.__aenter__ = AsyncMock(return_value=mock_session) mock_session.__aexit__ = AsyncMock(return_value=False) - with patch( - "plugins.platforms.slack.adapter.aiohttp.ClientSession", return_value=mock_session - ): - result = await adapter.send("C1", "Some response") + token = _slash_user_id.set("U1") + try: + with patch( + "plugins.platforms.slack.adapter.aiohttp.ClientSession", return_value=mock_session + ): + result = await adapter.send("C1", "Some response") + finally: + _slash_user_id.reset(token) assert result.success is True @@ -5460,10 +5480,27 @@ class TestSlashEphemeralAck: # ContextVar is unset (default=None) — simulates a normal message send. assert _slash_user_id.get() is None ctx = adapter._pop_slash_context("C1") - # Fallback scan still finds it (channel-only) — this is fine for - # the normal single-user case; the ContextVar path is the precise one. - # The key invariant is: when the ContextVar IS set, it matches exactly. - assert ctx is not None # fallback path finds the entry + assert ctx is None + assert ("C1", "U1") in adapter._slash_command_contexts + + @pytest.mark.asyncio + async def test_send_without_contextvar_preserves_pending_slash_context(self, adapter): + """Normal channel sends must not consume a pending slash reply context.""" + import time + from plugins.platforms.slack.adapter import _slash_user_id + + adapter._slash_command_contexts[("C1", "U1")] = { + "response_url": "https://hooks.slack.com/test", + "ts": time.monotonic(), + } + adapter._app.client.chat_postMessage = AsyncMock(return_value={"ts": "1234.5678", "ok": True}) + + assert _slash_user_id.get() is None + result = await adapter.send("C1", "public follow-up") + + assert result.success is True + adapter._app.client.chat_postMessage.assert_awaited_once() + assert ("C1", "U1") in adapter._slash_command_contexts # ---------------------------------------------------------------------------