Skip to content

fix(slack): normalize bang commands before parsing - #60907

Closed
paulpitchford wants to merge 1 commit into
NousResearch:mainfrom
paulpitchford:agent/fix-slack-bang-command-rich-text
Closed

fix(slack): normalize bang commands before parsing#60907
paulpitchford wants to merge 1 commit into
NousResearch:mainfrom
paulpitchford:agent/fix-slack-bang-command-rich-text

Conversation

@paulpitchford

@paulpitchford paulpitchford commented Jul 8, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes two Slack alternate command-prefix edge cases for messages like !commands and <@bot> !status.

Hermes rewrites known !cmd messages to /cmd because Slack native slash commands do not work in threads. In practice, Slack can decorate those messages before they reach the adapter:

  • ordinary Slack messages can include both plain text and mirrored rich_text blocks;
  • app/channel mentions include the literal leading bot mention in text, for example <@U_BOT> !status.

Before this change, those shapes either duplicated the original bang command or prevented command parsing from seeing the command at the start of the message.

Root cause

The command normalization path only handled text that already started with !.

For !commands with matching rich text blocks, the adapter rewrote the plain text to /commands, then compared extracted block text against the rewritten text only. Because the block still contained !commands, it was appended as extra content and downstream handlers saw:

/commands
!commands

For <@bot> !status, routing correctly detected that the bot was mentioned, but the command parser still received the leading mention text. That made the message an addressed prompt rather than a gateway command.

Changes Made

  • Preserve the raw Slack message text for routing and rich-text dedupe.
  • Strip only a leading literal bot mention before command detection and downstream message text construction.
  • Avoid appending block text when it exactly mirrors either the raw Slack text or the mention-stripped text.
  • Add regressions for both !commands rich-text duplication and <@U_BOT> !status command parsing.

Reproduction

Rich text duplication before this change:

  1. Send a Slack event with text: "!commands".
  2. Include a matching rich_text block containing !commands.
  3. The adapter rewrites the plain text to /commands.
  4. The block extraction appends !commands, so the command parser receives duplicate content.

Leading mention before this change:

  1. Send a Slack app mention/message event with text: "<@U_BOT> !status".
  2. Routing sees the bot mention and processes the event.
  3. Command detection does not see !status at the start, so it is handled as normal message text instead of /status.

After this change, the downstream command text is exactly /commands or /status respectively.

How to Test

  • python -m pytest tests/gateway/test_slack.py::TestBangPrefixCommands -q
  • scripts/run_tests.sh tests/gateway/test_slack.py -q
  • python -m ruff check plugins/platforms/slack/adapter.py tests/gateway/test_slack.py

Issue / PR search

I searched for existing issues and PRs around Slack !commands, rich text blocks, alternate command prefixes, duplicate command content, and mention-prefixed bang commands, and did not find an existing match.

Checklist

  • Bug fix
  • Regression tests added
  • Relevant Slack gateway tests pass
  • Lint check passes
  • Documentation not required; this preserves existing command syntax

@paulpitchford
paulpitchford marked this pull request as ready for review July 8, 2026 12:52
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/slack Slack app adapter P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jul 8, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #59903 — same fix (skip appending the Slack rich_text block when it mirrors the pre-rewrite !cmd plain text) on the same current path plugins/platforms/slack/adapter.py. #59903 is the earlier open PR. Related: #26309 / #43533 patch the now-deleted gateway/platforms/slack.py.

@paulpitchford
paulpitchford force-pushed the agent/fix-slack-bang-command-rich-text branch from d8a581a to 6a2fb08 Compare July 8, 2026 13:00
@paulpitchford paulpitchford changed the title fix(slack): avoid duplicating bang command rich text fix(slack): normalize bang commands before parsing Jul 8, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Comment (token read-only)

PR 60907 normalizes bang commands (!) before parsing in the Slack integration. Fixes a potential command parsing edge case. Well-scoped (2 files, 74 additions, 3 deletions). No security issues or debug artifacts detected.

LGTM - awaiting maintainer approval.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Comment (74 additions — normalize bang commands before parsing in Slack)

Scope

Normalizes bang commands before parsing in Slack to handle edge cases.

Observations

  • Good robustness fix for Slack command parsing.
  • The existing 1 review is a COMMENT-only — this is a fresh formal review.

Recommendation

Human reviewer familiar with Slack parsing should confirm the normalization handles all expected bang command formats.


Reviewed by Hermes Agent

@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 the focused Slack regression coverage. The current main implementation still has the reported behavior: normalization is gated by original_text.startswith("!") at plugins/platforms/slack/adapter.py:2631, and block deduplication compares only against the rewritten text at plugins/platforms/slack/adapter.py:2662.

Problems

  • The new mention test invokes _make_event() with its DM defaults. That bypasses the channel mention-routing branch at plugins/platforms/slack/adapter.py:2835, so it does not cover the proposed raw_original_text routing preservation.

Suggested changes

  • Make that regression a channel event and assert it reaches handle_message; this covers both command normalization and preservation of the raw mention for routing.

Automated hermes-sweeper review.

# same thread.
assert msg_event.source.thread_id == "1111111111.000001"

@pytest.mark.asyncio

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.

This uses the helper's default DM event, which bypasses channel mention gating. Please make it a channel event (channel_type="channel", channel="C123") so the regression also proves raw_original_text preserves the mention for routing after early stripping.

@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 10, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded by #69479 (merged): the mention-strip/bang ordering fix landed via #30592 (earliest submission for this sub-bug) — your diagnosis was correct.

Thanks for the work — it's credited in #69479's summary.

@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/gateway Gateway runner, session dispatch, delivery duplicate This issue or pull request already exists 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-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.

4 participants