Skip to content

fix: avoid Slack rich-text duplication in commands - #43533

Closed
th3wingman wants to merge 1 commit into
NousResearch:mainfrom
th3wingman:fix/slack-command-blocks-model-parser
Closed

fix: avoid Slack rich-text duplication in commands#43533
th3wingman wants to merge 1 commit into
NousResearch:mainfrom
th3wingman:fix/slack-command-blocks-model-parser

Conversation

@th3wingman

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a Slack gateway command parsing bug where Slack rich_text blocks can duplicate the visible command text.

For message-based bang commands such as:

!model qwen3.7-plus --provider opencode-go

Hermes rewrites the command to /model ... so it can be handled like a normal gateway slash command. However, Slack rich-text blocks may also contain a copy of the same visible text. The Slack adapter was appending that block text to the rewritten command, so the /model parser received duplicated/formatted content as part of the command arguments.

That caused valid model-switch commands to fail with:

Model names cannot contain spaces.

This PR skips Slack block extraction for slash/bang command messages while preserving block extraction for normal Slack messages, including quoted text. It also preserves thread_ts from native Slack slash-command payloads when Slack provides it, so session-scoped commands apply to the intended Slack thread.

Related Issue

Related: #10688

Similar symptom, different platform/root cause: #22716 / #23300

Related Slack block/bang-command context: #11426, #25355, #30592

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • gateway/platforms/slack.py

    • Preserve the original Slack message text before bang-command rewrite.
    • Detect slash commands and known bang commands before Slack block extraction.
    • Skip rich-text/block extraction for command messages so command args are not polluted by duplicated Slack block content.
    • Keep rich-text/block extraction for normal messages, including quoted slash-like text.
    • Preserve thread_ts from native Slack slash-command payloads when present.
  • tests/gateway/test_slack.py

    • Add regression coverage for !model ... --provider ... with Slack rich_text blocks.
    • Add regression coverage for preserving thread_ts from native Slack slash-command payloads.

How to Test

  1. Run the Slack gateway test file:
scripts/run_tests.sh tests/gateway/test_slack.py
  1. Confirm all Slack tests pass:
196 tests passed, 0 failed
  1. Manual behavior verified in Slack:
    • !model qwen3.7-plus --provider opencode-go no longer fails with Model names cannot contain spaces.
    • Normal quoted Slack rich-text content is still preserved as message text.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Linux

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — N/A; Slack gateway parsing logic only
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

scripts/run_tests.sh tests/gateway/test_slack.py

▶ running per-file parallel test suite via run_tests_parallel.py
  (TZ=UTC LANG=C.UTF-8 PYTHONHASHSEED=0; clean env)
Discovered 1 test files (196 tests) under ['tests/gateway/test_slack.py']; running with -j 16
[100.0% |   196/196 | ✓196 | ✗  0] ✓ tests/gateway/test_slack.py (196✓, 3.0s)

=== Summary: 1 files, 196 tests passed, 0 failed (100% complete) in 3.0s (16 workers) ===

Slack rich_text blocks can mirror the original message text. When bang
commands are rewritten from !model to /model, appending block text causes
the command arguments to include a duplicate payload, so the model switcher
sees spaces in the model name and rejects valid commands like:

  !model qwen3.7-plus --provider opencode-go

Skip block extraction for slash/bang commands while preserving it for
normal messages. Also preserve Slack thread_ts on native slash-command
payloads when present so session-scoped commands apply to the intended
thread.

Adds regression coverage for both cases.
@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 duplicate This issue or pull request already exists labels Jun 10, 2026
@alt-glitch

alt-glitch commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #59903: this PR addresses the same command rich-text duplication, but it edits the retired gateway/platforms/slack.py; the live adapter is plugins/platforms/slack/adapter.py. It also includes thread-ID preservation, so it is not a duplicate and needs a rebase or salvage against the current adapter.

@liuhao1024

Copy link
Copy Markdown
Contributor

✅ Verified — Slack rich-text block deduplication for commands

Reviewed the diff for gateway/platforms/slack.py and tests/gateway/test_slack.py.

  • Dedup logic: The fix saves original_slack_text before bang-command rewriting and checks blocks text against both the rewritten text and the original. This prevents the common case where !model qwen is rewritten to /model qwen but blocks still contains !model qwen, causing duplicate args.
  • Command detection: Block extraction is now skipped entirely for slash commands (/-prefixed) and bang commands (!-prefixed with a known gateway command). This is the right approach — command messages should never have their args polluted by rich-text formatting.
  • Bidirectional containment check: The not in checks test both directions (blocks not in text and text not in blocks), catching the case where the blocks text is a superset of the command text.
  • Thread ID preservation: Slash commands now extract thread_ts from multiple Slack payload shapes (thread_ts, message_ts, message.thread_ts, container.thread_ts), ensuring session-scoped commands like /model in threads key correctly.
  • Test coverage: test_bang_command_with_rich_text_block_is_not_duplicated directly reproduces the reported bug. test_slash_command_preserves_thread_id_when_payload_includes_it validates the thread context fix.

The fix is correct and well-scoped. No issues found.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused reproduction and regression test. The underlying defect remains on current main: plugins/platforms/slack/adapter.py:2644 rewrites a known bang command, and :2655-2668 can append the original rich-text form; hermes_cli/models.py:3840-3845 then rejects the whitespace-polluted model argument.

Problems

  • The PR edits the removed gateway/platforms/slack.py; Slack moved to plugins/platforms/slack/adapter.py in 5600105478ffde29d7566b45421b100eaa29c4ef, and GitHub reports this PR as conflicting.
  • A Block Kit-only port would leave other current command-enrichment paths active: attachments (adapter.py:2678-2733), first-entry thread context (:2886-2900), text-file injection (:3110-3127), and attachment notices (:3144-3148).
  • The added nested slash-payload lookups assume message and container are mappings; they need type guards before .get().

Suggested changes

  • Salvage the command classification before all enrichment in the plugin adapter and add coverage for malformed nested slash payloads plus the remaining enrichment sources.
  • The earlier duplicate fix(slack): dedupe rich text bang commands #26309 already includes a plugin-path-oriented approach and incorporates the useful thread-context work from this PR.

Automated hermes-sweeper review.

@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 14, 2026
@alt-glitch alt-glitch added needs-decision Awaiting maintainer decision before any implementation and removed duplicate This issue or pull request already exists 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 labels Jul 17, 2026
teknium1 pushed a commit that referenced this pull request Jul 22, 2026
…ad identity

Slack rich_text blocks mirror the original message text. When bang
commands are rewritten from !model to /model, appending block text makes
the command arguments include a duplicate payload, so the model switcher
sees spaces in the model name and rejects valid commands like:

  !model qwen3.7-plus --provider opencode-go

Skip block extraction for command messages while preserving it for
normal messages. Also preserve Slack thread_ts (top-level or nested in
message/container payload shapes) on native slash-command payloads so
session-scoped commands like /model apply to the intended thread instead
of a channel+user key the next threaded message never matches.

Surgical reapply of PR #43533 (originally against gateway/platforms/slack.py,
now plugins/platforms/slack/adapter.py). Thread-shape widening credit also
to #66310.
teknium1 pushed a commit that referenced this pull request Jul 23, 2026
…ad identity

Slack rich_text blocks mirror the original message text. When bang
commands are rewritten from !model to /model, appending block text makes
the command arguments include a duplicate payload, so the model switcher
sees spaces in the model name and rejects valid commands like:

  !model qwen3.7-plus --provider opencode-go

Skip block extraction for command messages while preserving it for
normal messages. Also preserve Slack thread_ts (top-level or nested in
message/container payload shapes) on native slash-command payloads so
session-scoped commands like /model apply to the intended thread instead
of a channel+user key the next threaded message never matches.

Surgical reapply of PR #43533 (originally against gateway/platforms/slack.py,
now plugins/platforms/slack/adapter.py). Thread-shape widening credit also
to #66310.
@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 block-skip for commands + slash thread identity was reapplied onto the plugin adapter with your authorship.

Thanks for the contribution!

@teknium1 teknium1 closed this Jul 23, 2026
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
…ad identity

Slack rich_text blocks mirror the original message text. When bang
commands are rewritten from !model to /model, appending block text makes
the command arguments include a duplicate payload, so the model switcher
sees spaces in the model name and rejects valid commands like:

  !model qwen3.7-plus --provider opencode-go

Skip block extraction for command messages while preserving it for
normal messages. Also preserve Slack thread_ts (top-level or nested in
message/container payload shapes) on native slash-command payloads so
session-scoped commands like /model apply to the intended thread instead
of a channel+user key the next threaded message never matches.

Surgical reapply of PR NousResearch#43533 (originally against gateway/platforms/slack.py,
now plugins/platforms/slack/adapter.py). Thread-shape widening credit also
to NousResearch#66310.
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 needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists platform/slack Slack app adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants