From 02f5ced7660debf8043714e75ee982926e414f81 Mon Sep 17 00:00:00 2001 From: drafish Date: Fri, 5 Jun 2026 11:22:06 +0800 Subject: [PATCH] fix(slack): pass event-derived chat_type to _has_active_session_for_thread _has_active_session_for_thread() hardcoded chat_type='group', causing session key mismatch for DM and MPIM threads. DM sessions key as agent:main:slack:dm:{chat_id}:{thread_ts} but the lookup built agent:main:slack:group:{chat_id}:{user_id}:{thread_ts}. Impact: _has_active_session_for_thread always returned False for DM threads, causing thread context to be prepended on every message. The prepended context broke slash command detection (get_command() checks text.startswith('/')), so /cmd and !cmd never worked in DM threads. Fix: accept event-derived chat_type parameter instead of hardcoding 'group'. Both call sites pass chat_type='dm' if is_dm else 'group', where is_dm is already computed from channel_type in {'im', 'mpim'}. This correctly handles: - IM channels (D-prefix): chat_type='dm' - MPIM channels (G-prefix): chat_type='dm' (was missed by D-prefix heuristic) - Channel messages (C-prefix): chat_type='group' (unchanged) Added regression tests covering DM thread lookup, MPIM thread lookup, and negative cases verifying the old hardcoded 'group' behavior fails. --- plugins/platforms/slack/adapter.py | 30 ++++- tests/gateway/test_slack_approval_buttons.py | 109 +++++++++++++++++++ 2 files changed, 136 insertions(+), 3 deletions(-) diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index e7c9fb54e136..b518a96bc214 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -3699,6 +3699,7 @@ class SlackAdapter(BasePlatformAdapter): user_id: str, is_thread_reply: bool, team_id: str = "", + chat_type: str = "group", ) -> bool: """Return True if the bot should wake on an un-mentioned message. @@ -3725,6 +3726,7 @@ class SlackAdapter(BasePlatformAdapter): thread_ts=event_thread_ts, user_id=user_id, team_id=team_id, + chat_type=chat_type, ): return True # 4th check: bot-initiated thread via direct chat.postMessage. @@ -4030,6 +4032,7 @@ class SlackAdapter(BasePlatformAdapter): user_id=user_id, team_id=team_id, is_thread_reply=is_thread_reply, + chat_type="dm" if is_dm else "group", ): return @@ -4079,6 +4082,7 @@ class SlackAdapter(BasePlatformAdapter): thread_ts=event_thread_ts, user_id=user_id, team_id=team_id, + chat_type="dm" if is_dm else "group", ) if is_thread_reply and not has_active_thread_session: thread_context = await self._fetch_thread_context( @@ -5832,6 +5836,8 @@ class SlackAdapter(BasePlatformAdapter): thread_ts: str, user_id: str, team_id: str = "", + *, + chat_type: str = "group", ) -> Optional[str]: """Build the backing session key for a Slack thread. @@ -5839,6 +5845,14 @@ class SlackAdapter(BasePlatformAdapter): 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: @@ -5849,7 +5863,7 @@ class SlackAdapter(BasePlatformAdapter): 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, @@ -5985,11 +5999,21 @@ class SlackAdapter(BasePlatformAdapter): thread_ts: str, user_id: str, team_id: str = "", + *, + chat_type: str = "group", ) -> bool: """Check if there's an active session for a thread. Used to determine if thread replies without @mentions should be processed (they should if there's an active session). + + 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: @@ -6001,14 +6025,14 @@ class SlackAdapter(BasePlatformAdapter): 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, ) session_key = self._build_thread_session_key( - channel_id, thread_ts, user_id, team_id=team_id + channel_id, thread_ts, user_id, team_id=team_id, chat_type=chat_type ) if not session_key: return False diff --git a/tests/gateway/test_slack_approval_buttons.py b/tests/gateway/test_slack_approval_buttons.py index fa0256c4f631..e848abb6e2f1 100644 --- a/tests/gateway/test_slack_approval_buttons.py +++ b/tests/gateway/test_slack_approval_buttons.py @@ -980,6 +980,115 @@ class TestSessionKeyFix: 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 # ===========================================================================