mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-24 16:54:43 +00:00
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.
This commit is contained in:
drafish
Teknium
parent
a83ec95c76
commit
02f5ced766
2 changed files with 136 additions and 3 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
# ===========================================================================
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue