Skip to content

fix(slack): slash commands broken in DM threads due to session key mismatch - #39527

Closed
drafish wants to merge 1 commit into
NousResearch:mainfrom
drafish:fix/slack-dm-thread-slash-commands
Closed

fix(slack): slash commands broken in DM threads due to session key mismatch#39527
drafish wants to merge 1 commit into
NousResearch:mainfrom
drafish:fix/slack-dm-thread-slash-commands

Conversation

@drafish

@drafish drafish commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Problem

Slash commands (/usage, /stop, !cmd etc.) are broken inside Slack DM threads — they get treated as regular text messages and forwarded to the agent instead of being intercepted as commands.

Root Cause

_has_active_session_for_thread() (line 2869 in gateway/platforms/slack.py) hardcodes chat_type="group" when building the session key for lookup:

source = SessionSource(
    platform=Platform.SLACK,
    chat_id=channel_id,
    chat_type="group",  # ← always "group", even for DMs
    user_id=user_id,
    thread_id=thread_ts,
)

This generates a key like agent:main:slack:group:D0ASJ8Q7TDG:{user_id}:{thread_ts}, but the actual DM session key is agent:main:slack:dm:D0ASJ8Q7TDG:{thread_ts}. The keys never match, so the function always returns False for DM threads.

Impact chain:

  1. _has_active_session_for_thread() returns False → thread context is prepended to every message
  2. text becomes [Thread context — ...]\n/usage instead of /usage
  3. get_command() checks text.startswith("/")False → returns None
  4. Gateway treats the message as regular user text → forwards to the agent

This affects all slash commands in DM threads, not just the ! prefix workaround.

Fix

Detect DM vs group from the channel ID (Slack DM channel IDs start with "D"):

chat_type="dm" if str(channel_id).startswith("D") else "group",

Testing

Verified locally: after the fix, !usage in a DM thread is correctly intercepted as a command and returns the usage report instead of being forwarded to the agent.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/slack Slack app adapter labels Jun 5, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating a real current-main session-key mismatch: _has_active_session_for_thread() still builds a group source at plugins/platforms/slack/adapter.py:4015-4021, while inbound IM/MPIM messages create dm sources at plugins/platforms/slack/adapter.py:2763,3163-3170.

Problems

  • The proposed D-prefix rule is incomplete. Main treats both im and mpim as DM-style sessions (plugins/platforms/slack/adapter.py:2763), while the test suite uses G_MPIM for an MPIM (tests/gateway/test_slack_mention.py:369-374). A non-D MPIM would still be looked up as group.
  • The PR adds no regression test. Existing lookup coverage is group-only (tests/gateway/test_slack_approval_buttons.py:618-641).

Suggested changes

  • During salvage, port this to plugins/platforms/slack/adapter.py and pass the event-derived is_dm/chat type into _has_active_session_for_thread() at both call sites (:2858, :2888) instead of re-inferring it from the ID.
  • Add an existing-DM-thread test with fetched context and a command, plus an MPIM case.

Automated hermes-sweeper review.

Comment thread gateway/platforms/slack.py Outdated
@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
…hread

_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.
@drafish
drafish force-pushed the fix/slack-dm-thread-slash-commands branch from 8f486fd to 301acc7 Compare July 15, 2026 12:53
@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via #69479 — your commit was cherry-picked/reapplied onto current main with your authorship preserved in git history: your DM-thread session-key fix was cherry-picked directly.

Thanks for the contribution!

@teknium1 teknium1 closed this Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/slack Slack app adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants