diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 4b2fc05b1ffa..06983f910f05 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -744,6 +744,8 @@ class SlackAdapter(BasePlatformAdapter): # the primary client meanwhile. self._channel_team: Dict[str, str] = {} self._CHANNEL_TEAM_MAX = 10000 + # user target (team_id:user_id) → opened DM conversation ID (D...) + self._dm_conversation_cache: Dict[str, str] = {} # Dedup cache: prevents duplicate bot responses when Socket Mode # reconnects redeliver events (#4777). The TTL must outlast Slack's # worst-case reconnect-redelivery gap, not just a few seconds — the @@ -1966,6 +1968,7 @@ class SlackAdapter(BasePlatformAdapter): self._team_clients = {} self._team_bot_user_ids = {} self._channel_team = {} + self._dm_conversation_cache = {} self._release_platform_lock() @@ -1992,6 +1995,49 @@ class SlackAdapter(BasePlatformAdapter): return self._team_clients[team_id] return self._app.client # fallback to primary + async def _ensure_dm_conversation( + self, chat_id: str, team_id: Optional[str] = None + ) -> str: + """Resolve a bare Slack user ID target to a DM conversation ID. + + ``chat.postMessage`` and ``files_upload_v2`` reject user IDs (U.../W...) + — a DM must be opened first via ``conversations.open`` to obtain a D... + conversation ID (#19236 / #17261). Conversation IDs (C/G/D...) pass + through unchanged. Resolution goes through the workspace-scoped client + so multi-workspace installs open the DM with the right bot token, and + results are cached per (team, user) so repeated sends don't re-open. + + Returns the resolved conversation ID, or the original ``chat_id`` when + resolution is not applicable or fails (the downstream API call then + surfaces the real Slack error). + """ + cid = str(chat_id or "") + if not cid or cid[0] not in ("U", "W"): + return chat_id + cache_key = f"{team_id or ''}:{cid}" + cached = self._dm_conversation_cache.get(cache_key) + if cached: + return cached + try: + response = await self._get_client(cid, team_id=team_id).conversations_open( + users=cid + ) + dm_id = ((response or {}).get("channel") or {}).get("id") + if dm_id: + self._dm_conversation_cache[cache_key] = dm_id + # DM belongs to the same workspace as the user target. + if team_id: + self._channel_team[dm_id] = team_id + return dm_id + except Exception as e: + logger.warning( + "[Slack] conversations.open failed for user target %s: %s " + "(check the bot's im:write scope)", + cid, + e, + ) + return chat_id + async def send( self, chat_id: str, @@ -2003,6 +2049,9 @@ class SlackAdapter(BasePlatformAdapter): if not self._app: return SendResult(success=False, error="Not connected") + chat_id = await self._ensure_dm_conversation( + chat_id, team_id=self._metadata_team_id(metadata) + ) thread_ts = None try: # Check for a pending slash-command context. When the user ran a @@ -2536,6 +2585,9 @@ class SlackAdapter(BasePlatformAdapter): if not os.path.exists(file_path): raise FileNotFoundError(f"File not found: {file_path}") + chat_id = await self._ensure_dm_conversation( + chat_id, team_id=self._metadata_team_id(metadata) + ) thread_ts = self._resolve_thread_ts(reply_to, metadata) last_exc = None for attempt in range(3): @@ -2586,6 +2638,9 @@ class SlackAdapter(BasePlatformAdapter): if not images: return + chat_id = await self._ensure_dm_conversation( + chat_id, team_id=self._metadata_team_id(metadata) + ) try: import httpx as _httpx from urllib.parse import unquote as _unquote @@ -3261,6 +3316,9 @@ class SlackAdapter(BasePlatformAdapter): response.raise_for_status() thread_ts = self._resolve_thread_ts(reply_to, metadata) + chat_id = await self._ensure_dm_conversation( + chat_id, team_id=self._metadata_team_id(metadata) + ) result = await self._get_client( chat_id, team_id=self._metadata_team_id(metadata) ).files_upload_v2( @@ -3334,6 +3392,9 @@ class SlackAdapter(BasePlatformAdapter): success=False, error=f"Video file not found: {video_path}" ) + chat_id = await self._ensure_dm_conversation( + chat_id, team_id=self._metadata_team_id(metadata) + ) try: thread_ts = self._resolve_thread_ts(reply_to, metadata) last_exc = None @@ -3396,6 +3457,9 @@ class SlackAdapter(BasePlatformAdapter): display_name = file_name or os.path.basename(file_path) thread_ts = self._resolve_thread_ts(reply_to, metadata) + chat_id = await self._ensure_dm_conversation( + chat_id, team_id=self._metadata_team_id(metadata) + ) try: last_exc = None @@ -5176,6 +5240,9 @@ class SlackAdapter(BasePlatformAdapter): if not self._app: return SendResult(success=False, error="Not connected") + chat_id = await self._ensure_dm_conversation( + chat_id, team_id=self._metadata_team_id(metadata) + ) try: thread_ts = self._resolve_thread_ts(None, metadata) @@ -5270,6 +5337,9 @@ class SlackAdapter(BasePlatformAdapter): if not self._app: return SendResult(success=False, error="Not connected") + chat_id = await self._ensure_dm_conversation( + chat_id, team_id=self._metadata_team_id(metadata) + ) try: thread_ts = self._resolve_thread_ts(None, metadata) # Same 3000-char section-block cap as send_exec_approval: budget @@ -5374,6 +5444,9 @@ class SlackAdapter(BasePlatformAdapter): if not self._app: return SendResult(success=False, error="Not connected") + chat_id = await self._ensure_dm_conversation( + chat_id, team_id=self._metadata_team_id(metadata) + ) try: thread_ts = self._resolve_thread_ts(None, metadata) diff --git a/tests/gateway/test_slack.py b/tests/gateway/test_slack.py index 329ddd9f8bda..f9f6b73c2f98 100644 --- a/tests/gateway/test_slack.py +++ b/tests/gateway/test_slack.py @@ -7021,3 +7021,161 @@ class TestTrackingStructureBounds: # can never remove a newer entry while an older one remains). for i in range(450, 500): assert f"{2000 + i}.000000" in adapter._bot_message_ts + + +# --------------------------------------------------------------------------- +# TestEnsureDmConversation — bare user-ID targets resolve to DM channels +# (#19236 / #17261: attachments and Block Kit prompts to U... targets) +# --------------------------------------------------------------------------- + + +class TestEnsureDmConversation: + @pytest.mark.asyncio + async def test_user_id_target_is_opened_as_dm(self, adapter): + adapter._app.client.conversations_open = AsyncMock( + return_value={"ok": True, "channel": {"id": "D999NEW"}} + ) + + resolved = await adapter._ensure_dm_conversation("U123ABCDEF") + + assert resolved == "D999NEW" + adapter._app.client.conversations_open.assert_awaited_once_with( + users="U123ABCDEF" + ) + + @pytest.mark.asyncio + async def test_conversation_ids_pass_through(self, adapter): + adapter._app.client.conversations_open = AsyncMock() + for cid in ("C123CHAN", "G123GROUP", "D123DM"): + assert await adapter._ensure_dm_conversation(cid) == cid + adapter._app.client.conversations_open.assert_not_awaited() + + @pytest.mark.asyncio + async def test_resolution_is_cached_per_user(self, adapter): + adapter._app.client.conversations_open = AsyncMock( + return_value={"ok": True, "channel": {"id": "D999NEW"}} + ) + + first = await adapter._ensure_dm_conversation("U123ABCDEF") + second = await adapter._ensure_dm_conversation("U123ABCDEF") + + assert first == second == "D999NEW" + adapter._app.client.conversations_open.assert_awaited_once() + + @pytest.mark.asyncio + async def test_failure_returns_original_target(self, adapter): + adapter._app.client.conversations_open = AsyncMock( + side_effect=Exception("missing_scope") + ) + + resolved = await adapter._ensure_dm_conversation("U123ABCDEF") + + assert resolved == "U123ABCDEF" + + @pytest.mark.asyncio + async def test_workspace_scoped_client_used_for_team_id(self, adapter): + team_client = AsyncMock() + team_client.conversations_open = AsyncMock( + return_value={"ok": True, "channel": {"id": "D_TEAM2"}} + ) + adapter._team_clients["T_SECOND"] = team_client + adapter._app.client.conversations_open = AsyncMock() + + resolved = await adapter._ensure_dm_conversation( + "U123ABCDEF", team_id="T_SECOND" + ) + + assert resolved == "D_TEAM2" + team_client.conversations_open.assert_awaited_once_with(users="U123ABCDEF") + adapter._app.client.conversations_open.assert_not_awaited() + # The opened DM is recorded as belonging to the same workspace. + assert adapter._channel_team["D_TEAM2"] == "T_SECOND" + + @pytest.mark.asyncio + async def test_send_resolves_user_target_before_posting(self, adapter): + adapter._app.client.conversations_open = AsyncMock( + return_value={"ok": True, "channel": {"id": "D999NEW"}} + ) + adapter._app.client.chat_postMessage = AsyncMock( + return_value={"ok": True, "ts": "111.222"} + ) + + result = await adapter.send("U123ABCDEF", "hello there") + + assert result.success is True + post_kwargs = adapter._app.client.chat_postMessage.await_args.kwargs + assert post_kwargs["channel"] == "D999NEW" + + @pytest.mark.asyncio + async def test_upload_file_resolves_user_target(self, adapter, tmp_path): + media = tmp_path / "report.pdf" + media.write_bytes(b"%PDF-1.4 fake") + adapter._app.client.conversations_open = AsyncMock( + return_value={"ok": True, "channel": {"id": "D999NEW"}} + ) + adapter._app.client.files_upload_v2 = AsyncMock( + return_value={"ok": True, "file": {"id": "F1"}} + ) + + result = await adapter._upload_file("U123ABCDEF", str(media)) + + assert result.success is True + upload_kwargs = adapter._app.client.files_upload_v2.await_args.kwargs + assert upload_kwargs["channel"] == "D999NEW" + + @pytest.mark.asyncio + async def test_send_document_resolves_user_target(self, adapter, tmp_path): + media = tmp_path / "notes.md" + media.write_bytes(b"# notes") + adapter._app.client.conversations_open = AsyncMock( + return_value={"ok": True, "channel": {"id": "D999NEW"}} + ) + adapter._app.client.files_upload_v2 = AsyncMock( + return_value={"ok": True, "file": {"id": "F1"}} + ) + + result = await adapter.send_document("U123ABCDEF", str(media)) + + assert result.success is True + upload_kwargs = adapter._app.client.files_upload_v2.await_args.kwargs + assert upload_kwargs["channel"] == "D999NEW" + + @pytest.mark.asyncio + async def test_send_clarify_resolves_user_target(self, adapter): + adapter._app.client.conversations_open = AsyncMock( + return_value={"ok": True, "channel": {"id": "D999NEW"}} + ) + adapter._app.client.chat_postMessage = AsyncMock( + return_value={"ok": True, "ts": "111.222"} + ) + + result = await adapter.send_clarify( + chat_id="U123ABCDEF", + question="Which one?", + choices=["a", "b"], + clarify_id="cl-1", + session_key="sk-1", + ) + + assert result.success is True + post_kwargs = adapter._app.client.chat_postMessage.await_args.kwargs + assert post_kwargs["channel"] == "D999NEW" + + @pytest.mark.asyncio + async def test_send_exec_approval_resolves_user_target(self, adapter): + adapter._app.client.conversations_open = AsyncMock( + return_value={"ok": True, "channel": {"id": "D999NEW"}} + ) + adapter._app.client.chat_postMessage = AsyncMock( + return_value={"ok": True, "ts": "111.222"} + ) + + result = await adapter.send_exec_approval( + chat_id="U123ABCDEF", + command="rm -rf /tmp/x", + session_key="sk-1", + ) + + assert result.success is True + post_kwargs = adapter._app.client.chat_postMessage.await_args.kwargs + assert post_kwargs["channel"] == "D999NEW"