diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index d936c0cfb90b..6f3409317dcc 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -1180,44 +1180,66 @@ class SlackAdapter(BasePlatformAdapter): 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. + Long replies are chunked: the first chunk replaces the ack, the + rest are posted as additional ephemeral messages. Slack allows at + most 5 POSTs to a response_url, so anything beyond that is closed + with an explicit truncation notice instead of being silently + dropped (#19688). + + Returns ``success=False`` on delivery failure so the caller + (``send()``) can fall back to normal channel delivery — the reply + must never be silently dropped just because the ephemeral swap + failed (#19688). """ formatted = self.format_message(content) # Slack's response_url has the same ~40k char limit as chat_postMessage. - # Truncate to MAX_MESSAGE_LENGTH and use only the first chunk — the - # response_url replaces a single ephemeral ack, so multi-chunk isn't - # possible. Long responses are rare for command replies. chunks = self.truncate_message(formatted, self.MAX_MESSAGE_LENGTH) - text = chunks[0] if chunks else formatted - payload = { - "response_type": "ephemeral", - "replace_original": True, - "text": text, - } + if not chunks: + chunks = [formatted] + # Slack allows at most 5 POSTs per response_url. Reserve the flow: + # 1 replace + up to 4 follow-ups; announce anything left over. + _MAX_RESPONSE_URL_POSTS = 5 + if len(chunks) > _MAX_RESPONSE_URL_POSTS: + dropped = len(chunks) - _MAX_RESPONSE_URL_POSTS + chunks = chunks[:_MAX_RESPONSE_URL_POSTS] + chunks[-1] = ( + chunks[-1].rstrip() + + f"\n\n_[Reply truncated: {dropped} more part(s) exceeded " + "Slack's ephemeral reply limit.]_" + ) try: async with aiohttp.ClientSession(trust_env=True) 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 _read_error_text_limited(resp) - logger.warning( - "[Slack] response_url POST returned %s: %s", - resp.status, - body[:200], - ) + for idx, chunk in enumerate(chunks): + payload = { + "response_type": "ephemeral", + # Only the first chunk replaces the "Running /cmd…" + # ack; the rest append as new ephemeral messages. + "replace_original": idx == 0, + "text": chunk, + } + async with session.post( + ctx["response_url"], + json=payload, + timeout=aiohttp.ClientTimeout(total=10), + ) as resp: + if resp.status != 200: + body = await _read_error_text_limited(resp) + logger.warning( + "[Slack] response_url POST returned %s: %s", + resp.status, + body[:200], + ) + return SendResult( + success=False, + error=f"response_url POST returned {resp.status}", + ) + return SendResult(success=True, message_id=None) 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) + return SendResult(success=False, error=str(e)) def _warn_if_missing_group_dm_scopes(self, auth_response, team_name: str) -> None: """Nudge existing installs to reinstall when group-DM scopes are absent. @@ -1825,10 +1847,21 @@ class SlackAdapter(BasePlatformAdapter): # 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( + ephemeral_result = await self._send_slash_ephemeral( slash_ctx, content, ) + if ephemeral_result.success: + return ephemeral_result + # Ephemeral delivery failed (#19688): fall through to normal + # channel delivery so the command reply is never silently + # dropped. The stale "Running /cmd…" ack remains, but the + # user still gets the actual answer. + logger.warning( + "[Slack] Ephemeral slash reply failed (%s); falling back " + "to channel delivery", + ephemeral_result.error, + ) # Convert standard markdown → Slack mrkdwn formatted = self.format_message(content) diff --git a/tests/gateway/test_slack.py b/tests/gateway/test_slack.py index f951b3089464..faa6502e6121 100644 --- a/tests/gateway/test_slack.py +++ b/tests/gateway/test_slack.py @@ -5515,7 +5515,7 @@ class TestSlashEphemeralAck: @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.""" + """Failed response_url POST falls back to normal channel delivery (#19688).""" import time from plugins.platforms.slack.adapter import _slash_user_id @@ -5536,6 +5536,10 @@ class TestSlashEphemeralAck: mock_session.__aenter__ = AsyncMock(return_value=mock_session) mock_session.__aexit__ = AsyncMock(return_value=False) + adapter._app.client.chat_postMessage = AsyncMock( + return_value={"ts": "1234.5678", "ok": True} + ) + token = _slash_user_id.set("U1") try: with patch( @@ -5545,8 +5549,112 @@ class TestSlashEphemeralAck: finally: _slash_user_id.reset(token) - # Still success — the user saw the initial ack already + # Reply must not be silently dropped: channel fallback delivered it. assert result.success is True + adapter._app.client.chat_postMessage.assert_awaited_once() + + @pytest.mark.asyncio + async def test_send_slash_ephemeral_fallback_on_exception(self, adapter): + """aiohttp exception on response_url falls back to channel delivery (#19688).""" + 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", + "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) + + adapter._app.client.chat_postMessage = AsyncMock( + return_value={"ts": "1234.5678", "ok": True} + ) + + 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 + adapter._app.client.chat_postMessage.assert_awaited_once() + + @pytest.mark.asyncio + async def test_send_slash_ephemeral_multichunk_delivers_all_parts(self, adapter): + """Long slash replies post every chunk instead of dropping the tail (#19688).""" + import time + + adapter._slash_command_contexts[("C1", "U1")] = { + "response_url": "https://hooks.slack.com/commands/long", + "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) + + long_content = "A" * (adapter.MAX_MESSAGE_LENGTH + 500) + + with patch( + "plugins.platforms.slack.adapter.aiohttp.ClientSession", return_value=mock_session + ): + result = await adapter._send_slash_ephemeral( + {"response_url": "https://hooks.slack.com/commands/long"}, + long_content, + ) + + assert result.success is True + assert mock_session.post.call_count >= 2 + # First POST replaces the ack; follow-ups append. + first_payload = mock_session.post.call_args_list[0][1]["json"] + second_payload = mock_session.post.call_args_list[1][1]["json"] + assert first_payload["replace_original"] is True + assert second_payload["replace_original"] is False + # No content byte is lost. + total_text = "".join( + c[1]["json"]["text"] for c in mock_session.post.call_args_list + ) + assert total_text.count("A") == len(long_content) + + @pytest.mark.asyncio + async def test_send_slash_ephemeral_caps_posts_with_truncation_notice(self, adapter): + """Beyond Slack's 5-POST response_url budget, truncation is announced.""" + 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) + + very_long = "B" * (adapter.MAX_MESSAGE_LENGTH * 7) + + with patch( + "plugins.platforms.slack.adapter.aiohttp.ClientSession", return_value=mock_session + ): + result = await adapter._send_slash_ephemeral( + {"response_url": "https://hooks.slack.com/commands/huge"}, + very_long, + ) + + assert result.success is True + assert mock_session.post.call_count == 5 + last_text = mock_session.post.call_args_list[-1][1]["json"]["text"] + assert "Reply truncated" in last_text @pytest.mark.asyncio async def test_send_slash_ephemeral_limits_error_body(self, adapter): @@ -5601,11 +5709,21 @@ 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") + adapter._app.client.chat_postMessage = AsyncMock( + return_value={"ts": "1234.5678", "ok": True} + ) + + from plugins.platforms.slack.adapter import _slash_user_id + + 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 assert response.text_calls == 0 @@ -5615,33 +5733,6 @@ class TestSlashEphemeralAck: ) assert response.released 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 - from plugins.platforms.slack.adapter import _slash_user_id - - 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) - - 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 - @pytest.mark.asyncio async def test_native_slash_stashes_context_and_dispatches(self, adapter): """Full flow: native /q slash → stash + handle_message dispatch."""