Skip to content

fix(slack): respect reply_in_thread and reply_to_mode for channel replies - #8598

Closed
dorukardahan wants to merge 4 commits into
NousResearch:mainfrom
dorukardahan:fix/slack-thread-routing-and-typing
Closed

fix(slack): respect reply_in_thread and reply_to_mode for channel replies#8598
dorukardahan wants to merge 4 commits into
NousResearch:mainfrom
dorukardahan:fix/slack-thread-routing-and-typing

Conversation

@dorukardahan

@dorukardahan dorukardahan commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fix _resolve_thread_ts() returning thread_ts for top-level channel messages when reply_in_thread=false or reply_to_mode="off", causing unwanted thread replies
  • Fix send_typing() calling assistant_threads_setStatus on top-level messages, which activates a Slack assistant thread and creates orphaned "Processing..." indicators
  • Add emoji reaction (hourglass) as lightweight processing indicator for non-thread contexts, cleaned up after response
  • Add 9 unit tests covering both fixes

Problem

When reply_in_thread is set to false in the platform extra config (or reply_to_mode is "off"), the bot still replies in threads because:

  1. _resolve_thread_ts bug: The gateway sets thread_id on ALL channel messages for session keying (thread_ts = event.get("thread_ts") or ts). The existing guard returned existing_thread unconditionally, which was always non-None. The fix distinguishes top-level messages (where reply_to == existing_thread) from genuine in-thread replies (where they differ).

  2. send_typing side effect: assistant_threads_setStatus creates/activates a Slack assistant thread on the target message. Even when _resolve_thread_ts correctly returns None, the assistant thread forces subsequent messages into a thread. The fix skips setStatus when non-thread config is active and uses an emoji reaction instead.

Changes

  • gateway/platforms/slack.py: 3 targeted changes (25 insertions, 3 deletions)
  • tests/gateway/test_slack.py: 9 new tests for TestResolveThreadTs and TestSendTyping

Test plan

  • Existing TestSendTyping tests pass (backward compatibility)
  • New TestResolveThreadTs tests verify top-level vs in-thread distinction
  • New TestSendTyping tests verify reaction fallback when reply_in_thread=false
  • Manual: send top-level channel message with reply_in_thread: false — reply goes to channel, not thread
  • Manual: send message in existing thread — reply stays in thread (preserved)
  • Manual: hourglass reaction appears during processing, removed after response

Related

@dorukardahan
fix(slack): respect reply_in_thread and reply_to_mode for channel rep…
34845ba 
…lies

When reply_in_thread is false or reply_to_mode is "off", top-level
channel messages should receive direct channel replies instead of
thread replies.  Two runtime bugs prevented this:

1. _resolve_thread_ts returned the message's own ts for top-level
   messages because the gateway sets thread_id on all channel messages
   for session keying.  The fix distinguishes top-level messages (where
   reply_to == thread_id, both equal the message ts) from genuine
   in-thread replies (where they differ).

2. send_typing called assistant_threads_setStatus on top-level messages,
   which activates a Slack assistant thread and forces subsequent replies
   into that thread — even when _resolve_thread_ts correctly returns
   None.  The fix uses an emoji reaction (hourglass_flowing_sand) as a
   lightweight processing indicator instead, cleaned up after the
   response is sent.

Also adds reply_to_mode=="off" as an alternative trigger (in addition
to reply_in_thread=false) for consistency with the platform config
schema.

Fixes NousResearch#7532 (runtime logic — complementary to config-bridging PRs)
Fixes NousResearch#8387 (prevention — complementary to cleanup PRs)
dorukardahan
dorukardahan commented Apr 12, 2026
View reviewed changes

@dorukardahan dorukardahan left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Self-review notes

  1. Why reply_to == existing_thread works: For top-level channel messages, the gateway sets thread_id to the message's own ts for session keying (thread_ts = event.get("thread_ts") or ts at line 1001). When the bot replies, reply_to is also the message's ts. So reply_to == existing_thread is true for top-level messages and false for genuine thread replies (where existing_thread is the parent and reply_to is the child).

  2. Why skip setStatus instead of just clearing it: The Slack assistant.threads.setStatus API activates an assistant thread on the target message. Even if cleared afterward, the thread is already created. Skipping entirely avoids this side effect.

  3. Emoji reaction vs setStatus: The hourglass reaction is a lightweight alternative that doesn't create assistant threads. It's cleaned up after the response — guarded by the same reply_in_thread/reply_to_mode check so it only runs when the reaction was actually added.

  4. Backward compatibility: When reply_in_thread=true (default), the new condition at line 393 short-circuits and the entire default path (lines 399-407) is unchanged. Existing behavior preserved.

  5. Relationship to #7534/#7561: Those PRs fix config bridging (making sure reply_in_thread reaches config.extra). This PR fixes the runtime logic that consumes it. They're complementary — both are needed for a complete fix.

@dorukardahan
fix(slack): preserve in-thread delivery for internal sends
d2e2b08 
Address Codex review finding: when reply_to is None but metadata
has a real thread_id (internal sends like typing updates, TTS, media),
the previous check `not reply_to` incorrectly returned None, causing
these messages to go to the channel instead of the thread.

Fix: use `reply_to == existing_thread` instead, which correctly
handles all four cases:
- Top-level: reply_to == thread_id (same ts) → None (channel)
- Proactive: reply_to == thread_id (both None) → None (channel)
- Internal send: reply_to=None, thread_id=parent → thread reply
- Genuine thread: reply_to=child, thread_id=parent → thread reply

Add test coverage for the internal send case.
@dorukardahan
fix(slack): respect reply_in_thread and reply_to_mode for channel rep…
86c82a9 
…lies

When reply_in_thread is false or reply_to_mode is "off", the bot
replies in threads instead of directly in the channel.  Root cause:
the gateway sets thread_id on ALL channel messages for session keying,
and the base class propagates it via _progress_metadata to every send
call.  _resolve_thread_ts cannot distinguish top-level from in-thread
because reply_to is always None and thread_id is always set.

Fix: set a per-message _force_channel_reply flag in _handle_slack_message
where is_thread_reply context is available.  _resolve_thread_ts and
send_typing check this flag to suppress threading and assistant status
for top-level messages while preserving in-thread delivery for genuine
thread replies.

Three changes in gateway/platforms/slack.py:
1. _handle_slack_message: set _force_channel_reply flag
2. _resolve_thread_ts: return None when flag is set
3. send_typing: skip setStatus when flag is set (prevents
   assistant_threads_setStatus from creating a thread)

Fixes NousResearch#7532, Fixes NousResearch#8387
@dorukardahan
fix(slack): use contextvars for async-safe per-task routing state
72df7da 
Address 3-model consensus review finding: _force_channel_reply as an
instance variable creates a race condition when Bolt dispatches
concurrent events as separate asyncio tasks.  Replace with a
contextvars.ContextVar which is isolated per-task.

Also use defensive getattr for config.reply_to_mode to avoid
AttributeError on older config objects.

Add backward compatibility test for default config path.
@dorukardahan

dorukardahan commented Apr 12, 2026

Copy link
Copy Markdown
Contributor Author

CI Note

68 test failures are all pre-existing upstream issues — none related to this PR's changes. Our 7 new tests (TestResolveThreadTs + TestSendTyping.test_skips_status_when_force_channel_reply) all pass.

Failing test modules (all unrelated to gateway/platforms/slack.py):

  • test_session_race_guard — missing _running_agents attribute
  • test_run_agent — AIAgent API changes (request_overrides, system prompt structure)
  • test_voice_mode — AudioRecorder.is_recording removed
  • test_provider_parity — context window minimum enforcement
  • test_ctx_halving_fix — AIAgent.request_overrides removed
  • test_code_execution — watch_patterns schema drift
  • test_interrupt — thread safety + SIGKILL escalation

These failures reproduce on upstream main without our changes.

@alt-glitch alt-glitch mentioned this pull request Apr 25, 2026
Closed
4 tasks
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists platform/slack Slack app adapter comp/gateway Gateway runner, session dispatch, delivery labels Apr 28, 2026
@alt-glitch

alt-glitch commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Overlaps with merged #16188 which fixed _resolve_thread_ts treating synthetic thread_id as real thread when reply_in_thread=false. The send_typing side-effect fix (emoji reaction fallback) may be net-new. Please rebase and verify which changes are still needed post-#16188.

@dorukardahan dorukardahan mentioned this pull request Apr 29, 2026
Closed
@dorukardahan

dorukardahan commented Apr 29, 2026

Copy link
Copy Markdown
Contributor Author

I refreshed this as a smaller PR on current main: #17184.

I kept only the part that is still needed after #16188. The old _resolve_thread_ts changes are not carried over.

Closing this one so review stays in one place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/slack Slack app adapter type/bug Something isn't working

Projects

None yet

Milestone

No milestone

2 participants

@dorukardahan @alt-glitch