Skip to content

fix(slack): honor disable_dms setting - #53030

Closed
vexclawx31 wants to merge 1 commit into
NousResearch:mainfrom
vexclawx31:fix/slack-disable-dms
Closed

fix(slack): honor disable_dms setting#53030
vexclawx31 wants to merge 1 commit into
NousResearch:mainfrom
vexclawx31:fix/slack-disable-dms

Conversation

@vexclawx31

Copy link
Copy Markdown
Contributor

Summary

  • ignore incoming Slack DM message events when slack.disable_dms or SLACK_DISABLE_DMS is enabled
  • ignore Slack slash commands invoked from DM channels under the same setting
  • keep channel/group mention routing unchanged

Why

Slack channel allowlists intentionally do not filter DMs, but operators may configure external/project-scoped agents to avoid DMs. The existing disable_dms setting could be present in config/env without being enforced by the Slack adapter.

Test plan

  • /Users/vex/.hermes/hermes-agent/venv/bin/python -m pytest tests/gateway/test_slack.py -k 'disable_dms or dm_slash_command_keeps_dm_session_semantics or plain_slash_still_works' -q -o 'addopts='
  • python3 -m py_compile plugins/platforms/slack/adapter.py tests/gateway/test_slack.py
  • git diff --check

@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins platform/slack Slack app adapter P3 Low — cosmetic, nice to have labels Jun 26, 2026
@vexclawx31
vexclawx31 force-pushed the fix/slack-disable-dms branch from a0db0c6 to 5565a3d Compare June 26, 2026 10:55

@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 tracing the missing configuration bridge; the underlying 1:1-DM gap is present on current main.

Problems

  • plugins/platforms/slack/adapter.py:2527 gates on is_dm, which includes both im and mpim. That suppresses MPIM group-DM messages before normal routing. Current documentation defines MPIMs as shared surfaces that retain channel-style mention and allowlist controls (website/docs/user-guide/messaging/slack.md:419), and current main distinguishes one-to-one DMs with is_one_to_one_dm (plugins/platforms/slack/adapter.py:2835).
  • The new slash-command check only recognizes D-prefixed channels, so MPIM message events and slash commands would be handled inconsistently.

Suggested changes

  • Limit this setting to channel_type == "im" / is_one_to_one_dm, then add an MPIM regression test proving group routing is unchanged.
  • Document slack.disable_dms in the Slack messaging guide.

Automated hermes-sweeper review.

if not channel_type and channel_id.startswith("D"):
channel_type = "im"
is_dm = channel_type in {"im", "mpim"} # Both 1:1 and group DMs
if is_dm and self._slack_disable_dms():

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.

is_dm includes mpim, so this also drops group-DM messages before their established shared-surface mention/allowlist routing. Please gate only one-to-one im DMs (or consistently extend the policy and document/test MPIM behavior); the slash-command path currently recognizes only D IDs.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via #70196 — your commit was cherry-picked/reapplied onto current main with your authorship preserved in git history: your disable_dms enforcement was cherry-picked.

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

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/slack Slack app adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants