From 1a7a3f8414420cf2fafb480b23216b1f144af876 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 22 Jul 2026 08:30:44 -0700 Subject: [PATCH] fix(slack): resolve bare user targets to DMs across all adapter send paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Widen the #19237 send_message fix to the live adapter: send, _upload_file, send_multiple_images, send_image, send_video, send_document, send_exec_approval, send_slash_confirm, and send_clarify now route bare Slack user IDs (U.../W...) through a shared _ensure_dm_conversation helper before calling chat.postMessage / files_upload_v2, which reject user IDs. This closes the gap in #17261 where an attachment worked when replying in a thread but failed when directed at a user DM, and extends the DM-open fallback to clarify/approval Block Kit prompts so gated actions can reach a user directly. Resolution uses the workspace-scoped client (multi-workspace installs open the DM with the right bot token), caches per (team, user), and records the opened D... channel in the channel→team map. On failure the original target passes through so the downstream API call surfaces the real Slack error. Fixes #17261 Refs #19236 --- plugins/platforms/slack/adapter.py | 73 +++++++++++++ tests/gateway/test_slack.py | 158 +++++++++++++++++++++++++++++ 2 files changed, 231 insertions(+) diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 830e2bdedd61..ac6e5221dab7 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -681,6 +681,8 @@ class SlackAdapter(BasePlatformAdapter): self._team_clients: Dict[str, Any] = {} # team_id → WebClient self._team_bot_user_ids: Dict[str, str] = {} # team_id → bot_user_id self._channel_team: Dict[str, str] = {} # channel_id → team_id + # 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 @@ -1742,6 +1744,7 @@ class SlackAdapter(BasePlatformAdapter): self._team_clients = {} self._team_bot_user_ids = {} self._channel_team = {} + self._dm_conversation_cache = {} self._release_platform_lock() @@ -1768,6 +1771,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, @@ -1779,6 +1825,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 @@ -2227,6 +2276,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): @@ -2277,6 +2329,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 @@ -2878,6 +2933,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( @@ -2951,6 +3009,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 @@ -3013,6 +3074,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 @@ -4607,6 +4671,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) @@ -4698,6 +4765,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 @@ -4802,6 +4872,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 240fbac10067..bc306ea76041 100644 --- a/tests/gateway/test_slack.py +++ b/tests/gateway/test_slack.py @@ -5755,3 +5755,161 @@ class TestMissingCredentials: assert "SLACK_APP_TOKEN" in fatal_errors[0]["message"] assert "hermes gateway setup" in fatal_errors[0]["message"].lower() or ".env" in fatal_errors[0]["message"] + + +# --------------------------------------------------------------------------- +# 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"