diff --git a/nanobot/channels/slack.py b/nanobot/channels/slack.py index 4c9c25ba..2083f7b1 100644 --- a/nanobot/channels/slack.py +++ b/nanobot/channels/slack.py @@ -133,14 +133,12 @@ class SlackChannel(BaseChannel): target_chat_id = await self._resolve_target_chat_id(msg.chat_id) slack_meta = msg.metadata.get("slack", {}) if msg.metadata else {} thread_ts = slack_meta.get("thread_ts") - channel_type = slack_meta.get("channel_type") origin_chat_id = str((slack_meta.get("event", {}) or {}).get("channel") or msg.chat_id) - # Slack DMs don't use threads; channel/group replies may keep thread_ts. - thread_ts_param = ( - thread_ts - if thread_ts and channel_type != "im" and target_chat_id == origin_chat_id - else None - ) + # Reply in the same thread the inbound message belongs to (works + # for both real channel threads and DM threads). When the agent + # is forwarding to a different channel, drop thread_ts because it + # only makes sense within the originating conversation. + thread_ts_param = thread_ts if thread_ts and target_chat_id == origin_chat_id else None if msg.content or not (msg.media or []): mrkdwn = self._to_mrkdwn(msg.content) if msg.content else " " @@ -352,7 +350,14 @@ class SlackChannel(BaseChannel): event_ts = event.get("ts") raw_thread_ts = event.get("thread_ts") thread_ts = raw_thread_ts - if self.config.reply_in_thread and not thread_ts: + # In DMs we don't auto-open a thread on top-level messages (it would + # bury replies under "1 reply"). But if the user explicitly opened a + # thread inside the DM, raw_thread_ts is set and we honor it. + if ( + self.config.reply_in_thread + and not thread_ts + and channel_type != "im" + ): thread_ts = event_ts # Add :eyes: reaction to the triggering message (best-effort) try: @@ -365,8 +370,12 @@ class SlackChannel(BaseChannel): except Exception as e: logger.debug("Slack reactions_add failed: {}", e) - # Thread-scoped session key for channel/group messages - session_key = f"slack:{chat_id}:{thread_ts}" if thread_ts and channel_type != "im" else None + # Thread-scoped session key whenever the user is in a real thread + # (raw_thread_ts is set). DM threads get their own session, separate + # from the DM root, so context doesn't bleed across thread boundaries. + session_key = ( + f"slack:{chat_id}:{thread_ts}" if thread_ts and raw_thread_ts else None + ) media_paths: list[str] = [] file_markers: list[str] = [] for file_info in event.get("files") or []: @@ -494,10 +503,10 @@ class SlackChannel(BaseChannel): current_ts: str | None, ) -> str: """Include thread history the first time the bot is pulled into a Slack thread.""" + del channel_type # DM and channel threads are both fetched via conversations.replies if ( not self.config.include_thread_context or not self._web_client - or channel_type == "im" or not raw_thread_ts or not thread_ts or current_ts == thread_ts @@ -644,7 +653,7 @@ class SlackChannel(BaseChannel): if not text: return "" text = cls._TABLE_RE.sub(cls._convert_table, text) - return cls._fixup_mrkdwn(slackify_markdown(text)) + return cls._fixup_mrkdwn(slackify_markdown(text)).rstrip("\n") @classmethod def _fixup_mrkdwn(cls, text: str) -> str: diff --git a/tests/channels/test_slack_channel.py b/tests/channels/test_slack_channel.py index df7a7b56..49ae3908 100644 --- a/tests/channels/test_slack_channel.py +++ b/tests/channels/test_slack_channel.py @@ -132,14 +132,15 @@ async def test_send_uses_thread_for_channel_messages() -> None: ) assert len(fake_web.chat_post_calls) == 1 - assert fake_web.chat_post_calls[0]["text"] == "hello\n" + assert fake_web.chat_post_calls[0]["text"] == "hello" assert fake_web.chat_post_calls[0]["thread_ts"] == "1700000000.000100" assert len(fake_web.file_upload_calls) == 1 assert fake_web.file_upload_calls[0]["thread_ts"] == "1700000000.000100" @pytest.mark.asyncio -async def test_send_omits_thread_for_dm_messages() -> None: +async def test_send_omits_thread_for_dm_root_messages() -> None: + """DM root replies should not be threaded; metadata carries thread_ts=None.""" channel = SlackChannel(SlackConfig(enabled=True), MessageBus()) fake_web = _FakeAsyncWebClient() channel._web_client = fake_web @@ -150,17 +151,46 @@ async def test_send_omits_thread_for_dm_messages() -> None: chat_id="D123", content="hello", media=["/tmp/demo.txt"], - metadata={"slack": {"thread_ts": "1700000000.000100", "channel_type": "im"}}, + metadata={"slack": {"thread_ts": None, "channel_type": "im"}}, ) ) assert len(fake_web.chat_post_calls) == 1 - assert fake_web.chat_post_calls[0]["text"] == "hello\n" + assert fake_web.chat_post_calls[0]["text"] == "hello" assert fake_web.chat_post_calls[0]["thread_ts"] is None assert len(fake_web.file_upload_calls) == 1 assert fake_web.file_upload_calls[0]["thread_ts"] is None +@pytest.mark.asyncio +async def test_send_keeps_thread_for_dm_thread_messages() -> None: + """When the user replies inside a DM thread, bot replies stay in the same thread.""" + channel = SlackChannel(SlackConfig(enabled=True), MessageBus()) + fake_web = _FakeAsyncWebClient() + channel._web_client = fake_web + + await channel.send( + OutboundMessage( + channel="slack", + chat_id="D123", + content="hello", + media=["/tmp/demo.txt"], + metadata={ + "slack": { + "thread_ts": "1700000000.000100", + "channel_type": "im", + "event": {"channel": "D123"}, + } + }, + ) + ) + + assert len(fake_web.chat_post_calls) == 1 + assert fake_web.chat_post_calls[0]["thread_ts"] == "1700000000.000100" + assert len(fake_web.file_upload_calls) == 1 + assert fake_web.file_upload_calls[0]["thread_ts"] == "1700000000.000100" + + @pytest.mark.asyncio async def test_send_splits_long_messages() -> None: channel = SlackChannel(SlackConfig(enabled=True), MessageBus()) @@ -262,7 +292,7 @@ async def test_send_resolves_channel_name_to_channel_id() -> None: ) assert fake_web.chat_post_calls == [ - {"channel": "C999", "text": "hello\n", "thread_ts": None} + {"channel": "C999", "text": "hello", "thread_ts": None} ] assert len(fake_web.conversations_list_calls) == 1 @@ -296,7 +326,7 @@ async def test_send_resolves_user_handle_to_dm_channel() -> None: assert fake_web.conversations_open_calls == [{"users": "U234"}] assert fake_web.chat_post_calls == [ - {"channel": "D234", "text": "hello\n", "thread_ts": None} + {"channel": "D234", "text": "hello", "thread_ts": None} ] @@ -327,7 +357,7 @@ async def test_send_updates_reaction_on_origin_channel_for_cross_channel_send() ) assert fake_web.chat_post_calls == [ - {"channel": "C999", "text": "done\n", "thread_ts": None} + {"channel": "C999", "text": "done", "thread_ts": None} ] assert fake_web.reactions_remove_calls == [ {"channel": "D_ORIGIN", "name": "eyes", "timestamp": "1700000000.000100"} @@ -365,7 +395,7 @@ async def test_send_does_not_reuse_origin_thread_ts_for_cross_channel_send() -> ) assert fake_web.chat_post_calls == [ - {"channel": "C999", "text": "done\n", "thread_ts": None} + {"channel": "C999", "text": "done", "thread_ts": None} ] @@ -431,6 +461,100 @@ async def test_with_thread_context_fetches_root_once() -> None: assert len(fake_web.conversations_replies_calls) == 1 +@pytest.mark.asyncio +async def test_with_thread_context_fetches_replies_in_dm_thread() -> None: + """DM threads should also pull thread history (not only channel threads).""" + channel = SlackChannel(SlackConfig(enabled=True), MessageBus()) + channel._bot_user_id = "UBOT" + fake_web = _FakeAsyncWebClient() + fake_web._conversations_replies_response = { + "messages": [ + {"ts": "211.000", "user": "UA", "text": "here is the file"}, + {"ts": "212.000", "user": "UA", "text": "please read it"}, + ] + } + channel._web_client = fake_web + + content = await channel._with_thread_context( + "what did you see?", + chat_id="D123", + channel_type="im", + thread_ts="211.000", + raw_thread_ts="211.000", + current_ts="213.000", + ) + + assert fake_web.conversations_replies_calls == [ + {"channel": "D123", "ts": "211.000", "limit": 20} + ] + assert "Slack thread context before this mention:" in content + assert "- <@UA>: here is the file" in content + + +@pytest.mark.asyncio +async def test_dm_root_message_has_no_thread_ts_and_no_thread_session() -> None: + """A top-level DM should not synthesize a thread_ts and uses the default session.""" + channel = SlackChannel(SlackConfig(enabled=True), MessageBus()) + channel._bot_user_id = "UBOT" + channel._web_client = _FakeAsyncWebClient() + channel._handle_message = AsyncMock() # type: ignore[method-assign] + client = SimpleNamespace(send_socket_mode_response=AsyncMock()) + req = SimpleNamespace( + type="events_api", + envelope_id="env-dm-root", + payload={ + "event": { + "type": "message", + "user": "U1", + "channel": "D123", + "channel_type": "im", + "text": "hello", + "ts": "1700000000.000100", + } + }, + ) + + await channel._on_socket_request(client, req) + + channel._handle_message.assert_awaited_once() + kwargs = channel._handle_message.await_args.kwargs + assert kwargs["session_key"] is None + assert kwargs["metadata"]["slack"]["thread_ts"] is None + + +@pytest.mark.asyncio +async def test_dm_thread_message_keeps_thread_ts_and_threaded_session() -> None: + """A DM message inside a real thread should preserve thread_ts and isolate the session.""" + channel = SlackChannel(SlackConfig(enabled=True), MessageBus()) + channel._bot_user_id = "UBOT" + channel._web_client = _FakeAsyncWebClient() + channel._handle_message = AsyncMock() # type: ignore[method-assign] + channel._with_thread_context = AsyncMock(return_value="hello") # type: ignore[method-assign] + client = SimpleNamespace(send_socket_mode_response=AsyncMock()) + req = SimpleNamespace( + type="events_api", + envelope_id="env-dm-thread", + payload={ + "event": { + "type": "message", + "user": "U1", + "channel": "D123", + "channel_type": "im", + "text": "hello", + "ts": "1700000000.000200", + "thread_ts": "1700000000.000100", + } + }, + ) + + await channel._on_socket_request(client, req) + + channel._handle_message.assert_awaited_once() + kwargs = channel._handle_message.await_args.kwargs + assert kwargs["session_key"] == "slack:D123:1700000000.000100" + assert kwargs["metadata"]["slack"]["thread_ts"] == "1700000000.000100" + + @pytest.mark.asyncio async def test_slack_slash_command_skips_thread_context() -> None: channel = SlackChannel(SlackConfig(enabled=True, allow_from=[]), MessageBus())