diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 69854c4a52ac7..cffa6f67df6b2 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -3364,6 +3364,7 @@ async def _handle_slack_message( thread_ts=event_thread_ts, user_id=user_id, team_id=team_id, + chat_type="dm" if is_dm else "group", ) if ( not reply_to_bot_thread @@ -3395,6 +3396,7 @@ async def _handle_slack_message( thread_ts=event_thread_ts, user_id=user_id, team_id=team_id, + chat_type="dm" if is_dm else "group", ): thread_context = await self._fetch_thread_context( channel_id=channel_id, @@ -4567,6 +4569,8 @@ def _has_active_session_for_thread( thread_ts: str, user_id: str, team_id: str = "", + *, + chat_type: str = "group", ) -> bool: """Check if there's an active session for a thread. @@ -4577,6 +4581,14 @@ def _has_active_session_for_thread( construction — avoids the bug where manual key building didn't respect ``thread_sessions_per_user`` and ``group_sessions_per_user`` settings correctly. + + Args: + chat_type: The session chat type — ``"dm"`` for IM/MPIM + conversations, ``"group"`` for channels. Must come from + the event-derived ``channel_type`` (``"im"``/``"mpim"`` + → ``"dm"``) rather than being inferred from the channel + ID prefix, because MPIM IDs start with ``"G"``, not + ``"D"``. """ session_store = getattr(self, "_session_store", None) if not session_store: @@ -4588,7 +4600,7 @@ def _has_active_session_for_thread( source = SessionSource( platform=Platform.SLACK, chat_id=channel_id, - chat_type="group", + chat_type=chat_type, user_id=user_id, thread_id=thread_ts, scope_id=team_id or None, diff --git a/tests/gateway/test_slack_approval_buttons.py b/tests/gateway/test_slack_approval_buttons.py index 13c62ee1aef07..15d3f22de2b85 100644 --- a/tests/gateway/test_slack_approval_buttons.py +++ b/tests/gateway/test_slack_approval_buttons.py @@ -731,6 +731,115 @@ def test_no_session_store(self): assert result is False +class TestSessionKeyChatType: + """Test that _has_active_session_for_thread passes event-derived chat_type. + + Regression for #39527: the old code hardcoded ``chat_type="group"``, + which produced wrong session keys for DM and MPIM threads. The fix + passes the event-derived ``chat_type`` so ``build_session_key()`` + constructs the correct key for every channel type. + """ + + def test_dm_thread_session_found(self): + """IM channel (D-prefix) with an active DM session is found.""" + adapter = _make_adapter() + mock_store = MagicMock() + # DM sessions key: agent:main:slack:dm:D_CHANNEL:thread_ts + mock_store._entries = { + "agent:main:slack:dm:D0DMCHANNEL:2000.0": MagicMock() + } + mock_store._ensure_loaded = MagicMock() + mock_store.config = MagicMock() + mock_store.config.group_sessions_per_user = True + mock_store.config.thread_sessions_per_user = False + adapter._session_store = mock_store + + result = adapter._has_active_session_for_thread( + channel_id="D0DMCHANNEL", + thread_ts="2000.0", + user_id="U_USER", + chat_type="dm", + ) + assert result is True + + def test_dm_thread_not_found_with_group_type(self): + """Without chat_type='dm', a DM session key would not match. + + This is the exact bug that the old ``hardcoded "group"`` code caused: + the lookup builds ``group:…`` while the real session is ``dm:…``. + """ + adapter = _make_adapter() + mock_store = MagicMock() + mock_store._entries = { + "agent:main:slack:dm:D0DMCHANNEL:2000.0": MagicMock() + } + mock_store._ensure_loaded = MagicMock() + mock_store.config = MagicMock() + mock_store.config.group_sessions_per_user = True + mock_store.config.thread_sessions_per_user = False + adapter._session_store = mock_store + + # Default chat_type="group" should NOT find the DM session + result = adapter._has_active_session_for_thread( + channel_id="D0DMCHANNEL", + thread_ts="2000.0", + user_id="U_USER", + ) + assert result is False + + def test_mpim_thread_session_found(self): + """MPIM channel (G-prefix, treated as DM) with an active session is found. + + MPIM channel IDs start with "G", not "D", so inferring chat_type + from the prefix would incorrectly classify this as "group". + """ + adapter = _make_adapter() + mock_store = MagicMock() + # MPIM sessions key: agent:main:slack:dm:G_MPIM_CHANNEL:thread_ts + mock_store._entries = { + "agent:main:slack:dm:G0MPIMCHANNEL:3000.0": MagicMock() + } + mock_store._ensure_loaded = MagicMock() + mock_store.config = MagicMock() + mock_store.config.group_sessions_per_user = True + mock_store.config.thread_sessions_per_user = False + adapter._session_store = mock_store + + result = adapter._has_active_session_for_thread( + channel_id="G0MPIMCHANNEL", + thread_ts="3000.0", + user_id="U_USER", + chat_type="dm", # event-derived: mpim → dm + ) + assert result is True + + def test_mpim_thread_not_found_with_group_type(self): + """Without passing chat_type='dm', MPIM sessions are invisible. + + This is the specific case the reviewer flagged: the old D-prefix + heuristic would classify G-prefixed MPIM channels as "group", + missing the DM session. + """ + adapter = _make_adapter() + mock_store = MagicMock() + mock_store._entries = { + "agent:main:slack:dm:G0MPIMCHANNEL:3000.0": MagicMock() + } + mock_store._ensure_loaded = MagicMock() + mock_store.config = MagicMock() + mock_store.config.group_sessions_per_user = True + mock_store.config.thread_sessions_per_user = False + adapter._session_store = mock_store + + # Default chat_type="group" → builds group key → no match + result = adapter._has_active_session_for_thread( + channel_id="G0MPIMCHANNEL", + thread_ts="3000.0", + user_id="U_USER", + ) + assert result is False + + # =========================================================================== # Thread engagement — bot-started threads & mentioned threads # ===========================================================================