fix(slack): harden slash/bang command handling — parse order, session keys, response_url delivery - #69479
Merged
Conversation
Contributor
૮ >ﻌ< ა ci reviewrunning on dc5d16f CI timingsCI timings · View jobWall time 8m3s vs 10m11s (-20.9%). 10 job(s) slower, 10 faster,
|
Contributor
|
One correction before merge: when Suggested implementation: retain the invoking |
alt-glitch
added
type/bug
…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.
…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.
_pop_slash_context fell back to a channel-only scan when the _slash_user_id ContextVar was unset (i.e. send() invoked from a non-slash code path such as a cron delivery or a normal channel reply). That scan could steal another user's pending slash reply context: the normal message got swallowed into an ephemeral response_url POST that replaces the invoker's ack, and the slash invoker's actual reply then posted publicly. Remove the fallback — when the ContextVar is unset, match nothing. Surgical reapply of PR #26788 (originally against gateway/platforms/slack.py).
…rs (#46762) Slack's send() caught all exceptions and returned a bare SendResult(success=False) — never setting retryable=True or extracting the server's Retry-After header. When Slack returned a 429 rate-limit error, the base _send_with_retry() layer saw retryable=False and did not retry, silently dropping remaining message chunks. Reuse the existing _is_retryable_upload_error() helper (which already detects 429, 500+, and connection-type errors) to set retryable=True, and extract the Retry-After header from the SlackApiError response when present so the base retry layer honors Slack's backoff schedule instead of its own default. Sibling of the Telegram FloodWait fix (PR #46762 / commit 404b06a) which added the SendResult.retry_after plumbing to the base layer. Adds five regression tests covering 429 with/without Retry-After, 500 server errors, 403 non-retryable errors, and connection errors.
Commands typed in Slack could be mangled by every enrichment layer the adapter applies to normal messages: - Block Kit / unfurl / attachment-notice / text-file injection could prepend or append content around a command, moving the command token away from character zero or polluting its arguments. Commands are now restored from canonical authored input after all enrichment (final is_command_text guard before MessageEvent construction). - @bot /cmd (typed slash behind a mention) was never classified as a command; the mention-strip branch now re-probes for both slash and bang forms. - The Slack Agent-view context label ([Slack app context: ...]) was prepended to command events too; now command-exempt. - Native slash payload arguments were strip()ed, destroying meaningful spacing inside/after arguments; only the command delimiter is nonsemantic now. - Slash payload thread identity (thread_ts/message_ts, top-level or nested in message/container) is preserved onto SessionSource so session-scoped commands hit the same thread session. - /queue and /steer queued fallbacks now propagate channel_context so a command that triggered first-entry thread backfill doesn't lose the history when re-queued. Adapted from PR #66310 to the post-#69320 channel_context design (thread history already rides MessageEvent.channel_context, never text).
Two silent-loss modes in the ephemeral slash reply path: 1. Delivery failure was swallowed: _send_slash_ephemeral returned success=True on any POST failure, so the user's actual command reply vanished behind the stale 'Running /cmd…' ack. It now returns success=False and send() falls back to normal channel delivery. 2. Long replies were truncated to the first ~39k chunk with no notice. Replies are now chunked across response_url POSTs (first replaces the ack, follow-ups append, all ephemeral), capped at Slack's 5-POST response_url budget with an explicit truncation notice when exceeded. Also updates the #55357 bounded-error-read test for the #26788 precise context matching (ContextVar must be set) and channel fallback. Fixes #19688
- test_thread_command_skips_context_prefix: post-#69320 thread context IS fetched on first thread entry but rides channel_context; assert the command token stays at char zero and the backfill is preserved, instead of asserting the fetch never happens. - test_slack_send_retry.py: main's _get_client() now takes team_id; update the lambda stubs.
…r slash replies Per review (Victor): response_url failure does not mean ephemeral delivery is impossible — chat.postEphemeral is an independent API path that keeps the reply private. Public-channel fallback removed entirely; when both ephemeral paths fail the reply is dropped with a logged error rather than leaked to the channel. No config knob needed.
teknium1
force-pushed
the
slack/c3-commands
branch
from
July 22, 2026 21:55
342b61b to
dc5d16f
Compare
Contributor
Author
|
Review feedback applied (thanks Victor): the public-channel fallback is gone. New chain when
New regression test pins the invariant: |
alt-glitch
removed
the
needs-decision
This was referenced Jul 23, 2026
This was referenced Jul 23, 2026
19 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Slack slash/bang command parsing is now robust across the full inbound pipeline (mention strip → bang rewrite → command detect → session key → reply delivery) — commands are no longer mangled by leading whitespace, mention stripping, rich-text enrichment, or DM-thread session-key mismatches, and slash replies are never silently dropped.
Fixes #19688, #55356.
Changes
@bot !cmdre-normalized after mention strip so bang commands dispatch in threads (fix(slack): dispatch @bot !cmd as command in threads #30592); command messages skip block extraction to avoid rich-text duplication (fix: avoid Slack rich-text duplication in commands #43533); typed command integrity preserved against ALL enrichment paths — blocks, unfurls, attachment notices, text-file injection (fix(slack): preserve typed command integrity #66310, adapted to the post-fix(slack): thread-context lifecycle — cold-start, restart rehydration, mention refresh, reply-wake #69320 channel_context design).retryable+Retry-After(fix(slack): surface retryable + Retry-After on send() rate-limit errors (#46762) #52436); slash replies chunk across ≤5 response_url POSTs and fall back to public channel delivery on failure — never silently dropped (widening, fixes Slack slash-command replies can be silently truncated or dropped when response_url delivery fails #19688).Credits
Salvaged with authorship preserved: #56718 (@nu476), #30592 (@cypres0099), #43533 (@th3wingman), #39527 (@drafish), #26788 (@soynchux), #55357 (@ooiuuii), #52436 (@srojk34), #66310 (@PavelTajdus).
Supersedes #60907, #47115 (both covered by #30592's earlier mention/bang ordering fix), #59903 (covered by #43533's block-skip). #10052's Slack half was already fixed by #69320; its session_search half is deferred.
Validation
scripts/run_tests.sh tests/gateway/ -q -k slackInfographic