fix(slack): normalize conversation ID to prevent thread splits

[jamiepine]

Summary

• Slack thread replies included thread_ts in the conversation ID (slack:TEAM:CHANNEL:THREAD_TS), which differed from the top-level form (slack:TEAM:CHANNEL). This caused the main event loop to create a second, disconnected channel for the same Slack conversation — orphaning the original worker results and outbound response routing.
• Now always uses slack:{team}:{channel} as the conversation ID. Thread targeting for outbound replies is unaffected since slack_thread_ts is read from message metadata in extract_thread_ts().

Bug behavior

1. User @-mentions bot in a channel → channel created, worker spawned
2. Worker completes, response routed through latest_message which has no slack_thread_ts metadata → reply either posted as top-level message or lost
3. User sends ? in the thread → different conversation ID → new channel created → immediate response from fresh context
4. Original worker result never visible to user in the thread

Reported by Tyler and Ohmna in #debug — worker dashboard showed the response was generated but it never reached Slack.

Changes

Three sites in slack.rs where conversation IDs are constructed:

• handle_message_event
• handle_app_mention_event
• handle_block_actions

All now use the stable slack:{team}:{channel} form.

Note

This collapses all threads in a single Slack channel into one conversation context. If per-thread isolation is needed later, the conversation ID can be re-keyed with thread_ts but would need reconciliation logic in the main loop to avoid the orphaning problem.

Note

This fix removes thread-specific conversation ID splitting across three event handlers in slack.rs, ensuring all interactions in a channel route to the same conversation context. Thread targeting for replies is preserved via existing metadata extraction, resolving worker result orphaning observed in thread responses.

_{Written by Tembo for commit 8f20589.}

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

Introduces routed outbound envelopes (RoutedResponse/RoutedSender) and propagates them through agent/channel, tools, cron, main, and tests. Slack message handling now uses a channel-level conversation ID; broadcast and outbound paths parse and attach optional Slack thread_ts to outgoing messages.

Changes

Cohort / File(s)	Summary
Slack messaging src/messaging/slack.rs	Use channel-level conversation ID (no thread_ts) for inbound handling; broadcast parses optional #thread:<ts> suffix from targets and consistently attaches thread_ts to outbound messages.
Agent / Channel runtime src/agent/channel.rs	Channel.response_tx changed to carry RoutedResponse; added current_inbound: Option<InboundMessage> and routing-aware send paths (send_routed) wired into turn processing and outbound actions.
Core routing types src/lib.rs	Added RoutedResponse, RoutedSender, and InboundMessage::empty() plus tokio::sync::mpsc import to model routed envelopes and provide a sender wrapper.
Application entry / SSE dispatch src/main.rs	Replaced raw outbound channel usage with RoutedResponse, removed per-channel latest_message, added forward_sse_event / route_outbound helpers to forward routed responses to SSE and adapters.
Cron scheduler src/cron/scheduler.rs	Switched cron outbound channel to RoutedResponse and wraps outbound Text/RichMessage into RoutedResponse before processing.
Tools wiring src/tools.rs, src/tools/... src/tools/react.rs, src/tools/reply.rs, src/tools/send_file.rs, src/tools/skip.rs	Propagated RoutedSender through tools APIs and struct fields; updated signatures (e.g., add_channel_tools) to accept slack thread context and RoutedSender; tests adapted to routed envelopes.
Message forwarding helper src/tools/send_message_to_another_channel.rs	Control-flow refactor combining guards with let-bindings; behavior unchanged.
Tests / context helpers tests/context_dump.rs	Replaced direct mpsc response usage with a raw tx wrapped by RoutedSender::new(..., InboundMessage::empty()) and adjusted add_channel_tools calls to include slack thread arg.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• fix: preserve conversation history and improve worker retrigger reliability #270 — Modifies src/agent/channel.rs run_agent_turn and channel routing/state; likely overlaps with current_inbound and routing changes.
• feat(messaging): add Signal adapter via signal-cli JSON-RPC daemon #347 — Touches add_channel_tools and tooling wiring; overlaps with RoutedSender propagation through tools.
• feat: add email messaging adapter and setup docs #244 — Changes channel outbound routing and tool initialization surface; related to the channel/tools API updates.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix(slack): normalize conversation ID to prevent thread splits' accurately captures the primary fix: removing thread_ts from conversation IDs to prevent duplicate channel creation and worker result orphaning.
Description check	✅ Passed	The description thoroughly explains the bug (thread-specific conversation IDs causing channel splits), reproduction steps, the fix applied, and implications for future per-thread isolation.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/slack-thread-conversation-id

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[tembo]

Now that conversation_id is per-channel, watch out for cross-thread reply misrouting if multiple threads interleave: outbound routing that pulls slack_thread_ts from the current latest_message can end up replying into the wrong thread when a worker finishes later. If this shows up in practice, consider pinning the outbound target (thread_ts) from the triggering inbound message at enqueue time instead of reading it from latest_message at send time.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/tools.rs (1)

348-359: ⚠️ Potential issue | 🟠 Major

conversation_id is no longer enough for thread-aware cron defaults.

After Slack IDs were normalized to slack:{team}:{channel}, this API no longer has access to the originating thread when it wires up per-turn tools. default_delivery_target_for_conversation() can now only persist the channel root, and run_cron_job() later broadcasts using only that stored delivery target, so reminders created inside a thread will post top-level. Pass the pinned inbound target—or at least slack_thread_ts—through add_channel_tools() as well.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tools.rs` around lines 348 - 359, The add_channel_tools function
currently only receives conversation_id which has been normalized to
slack:{team}:{channel}, losing thread context so
default_delivery_target_for_conversation() and run_cron_job() end up posting
reminders at channel root; modify the add_channel_tools signature to accept the
pinned inbound delivery target or at minimum a slack_thread_ts (e.g., add a
slack_thread_ts: Option<String> parameter or inbound_target:
Option<DeliveryTarget>) and thread that value through to where
default_delivery_target_for_conversation() is called and persisted so that
run_cron_job() can use the full thread-aware delivery target when scheduling and
broadcasting cron messages. Ensure callers that construct ChannelState or call
add_channel_tools (the call sites creating per-turn tools) pass the thread
ts/target along, and update any types (e.g., ChannelState handling or ToolServer
wiring) to carry the new optional thread info.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/agent/channel.rs`:
- Around line 2246-2251: The RoutedSender is being constructed from the mutable
channel-wide self.current_inbound (and may fall back to
InboundMessage::empty()), which pins replies to a stale target; instead capture
and attach the originating inbound target to the queued batch/pending result at
enqueue time (when building the batch or pending result), and then use that
captured inbound target to construct RoutedSender and pass it into
run_agent_turn() (rather than reading self.current_inbound inside
handle_message_batch()/retrigger paths). Update the code paths around
handle_message_batch(), the queue/enqueue logic, and run_agent_turn() to accept
an InboundMessage parameter and create
RoutedSender::new(self.response_tx.clone(), captured_inbound.clone()) using that
captured value.
- Around line 2289-2291: The send currently uses a discarded Result via "let _ =
self.send_routed(OutboundResponse::Status(crate::StatusUpdate::Thinking)).await;"
which violates the repo rule—replace it with the sanctioned dropped-send form by
calling .ok() on the awaited Result: invoke
self.send_routed(OutboundResponse::Status(crate::StatusUpdate::Thinking)).await.ok();
so the send_routed call (and its OutboundResponse::Status/StatusUpdate::Thinking
payload) remains best-effort while conforming to the repository pattern.

---

Outside diff comments:
In `@src/tools.rs`:
- Around line 348-359: The add_channel_tools function currently only receives
conversation_id which has been normalized to slack:{team}:{channel}, losing
thread context so default_delivery_target_for_conversation() and run_cron_job()
end up posting reminders at channel root; modify the add_channel_tools signature
to accept the pinned inbound delivery target or at minimum a slack_thread_ts
(e.g., add a slack_thread_ts: Option<String> parameter or inbound_target:
Option<DeliveryTarget>) and thread that value through to where
default_delivery_target_for_conversation() is called and persisted so that
run_cron_job() can use the full thread-aware delivery target when scheduling and
broadcasting cron messages. Ensure callers that construct ChannelState or call
add_channel_tools (the call sites creating per-turn tools) pass the thread
ts/target along, and update any types (e.g., ChannelState handling or ToolServer
wiring) to carry the new optional thread info.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ad79f104-a684-47b0-b539-270e730454c1

📥 Commits

Reviewing files that changed from the base of the PR and between 8f20589 and bda4c9d.

📒 Files selected for processing (9)

• src/agent/channel.rs
• src/cron/scheduler.rs
• src/lib.rs
• src/main.rs
• src/tools.rs
• src/tools/react.rs
• src/tools/reply.rs
• src/tools/send_file.rs
• src/tools/skip.rs

✅ Files skipped from review due to trivial changes (1)

• src/main.rs