feat(slack): app_mention, ephemeral messages, Block Kit, scheduled messages, typing indicator#58
Conversation
04986dc to
c0b2708
Compare
jamiepine
left a comment
There was a problem hiding this comment.
Review
Solid PR overall. The refactoring of handle_push_event into extracted helpers is clean, and the new OutboundResponse variants are well-designed with appropriate fallbacks across adapters.
Two issues to address:
1. Channel filter bypass in app_mention
Location: src/messaging/slack.rs:685-733 (handle_app_mention_event)
The handle_app_mention_event function applies the workspace filter but skips the channel filter entirely. This means if someone @-mentions the bot in a filtered channel, it'll still respond.
// Workspace filter applies to mentions too
if let Some(ref filter) = perms.workspace_filter {
if !filter.contains(&team_id_str) {
return Ok(());
}
}
// MISSING: Channel filter (compare to handle_message_event:158-164)The regular message handler applies both filters. This should too.
Fix: Add the channel filter block after the workspace filter check (after line 733):
// Channel filter
if let Some(allowed) = perms.channel_filter.get(&team_id_str) {
if !allowed.is_empty() && !allowed.contains(&channel_id) {
return Ok(());
}
}2. send_status() creates fresh client on every call
Location: src/messaging/slack.rs:868-869
let (client, token) = self.create_session()?;
let session = client.open_session(&token);This allocates a new SlackClient + SlackHyperClient every time a status update fires. Status updates happen frequently (Thinking, ToolStarted, ToolCompleted, StopTyping) — a single conversation turn with 3 tool calls means ~7 fresh client allocations.
Fix: Cache the client on SlackAdapter (add client: Arc<SlackHyperClient> field, initialize once in new(), reuse across all methods). This is the same pattern Discord and other adapters use.
c0b2708 to
d8d7d5d
Compare
|
Both fixed in the force-push — thanks for the sharp eyes. 1. Channel filter bypass in app_mention — fixed Added the missing channel filter block immediately after the workspace filter in // Channel filter — same logic as handle_message_event
if let Some(allowed) = perms.channel_filter.get(&team_id_str) {
if !allowed.is_empty() && !allowed.contains(&channel_id) {
return Ok(());
}
}2. Cached client — fixed
The socket mode listener keeps its own separate client instance, which is intentional — it owns a persistent WebSocket connection internally and needs to hold that client for the lifetime of the connection.
|
…yping indicator
Add five coordinated capabilities to the Slack adapter (Phase 1 + 2a + 4
from docs/PRD-slack-enhancements.md).
- RemoveReaction(String) — remove an emoji reaction
- Ephemeral { text, user_id } — message visible only to the triggering user
- RichMessage { text, blocks } — Block Kit blocks with plain-text fallback
- ScheduledMessage { text, post_at } — chat.scheduleMessage at a unix timestamp
- AppMention handler: fires when the bot is @-mentioned in any channel;
strips the leading <@BOT_ID> token before passing text to the agent
- Typing indicator: send_status() now calls assistant.threads.setStatus
(Thinking…/Working…/empty-string-to-clear) instead of being a no-op
- RemoveReaction: reactions.remove via channel + timestamp
- Ephemeral: chat.postEphemeral threaded to the triggering message
- RichMessage: chat.postMessage with SlackBlock list; falls back to text
if blocks fail to deserialise (per-block, with warning log)
- ScheduledMessage: chat.scheduleMessage with SlackDateTime(DateTime<Utc>)
- broadcast() extended to support RichMessage in addition to Text
- bot_user_id resolved at start() via auth.test; self-messages filtered
New variants handled exhaustively with platform-appropriate fallbacks:
- Discord: Ephemeral/RichMessage/ScheduledMessage fall back to plain text;
RemoveReaction is a no-op (no per-message remove API on this path)
- Telegram: same plain-text fallbacks via existing bot.send_message()
- Webhook: Ephemeral/RichMessage/ScheduledMessage serialised as
response_type=text; RemoveReaction returns Ok(()) early
All 43 lib unit tests pass. cargo check clean.
d8d7d5d to
f591658
Compare
Summary
Adds five coordinated Slack capabilities in one cohesive PR. See
docs/PRD-slack-enhancements.md(included) for full design rationale.New
OutboundResponsevariants (src/lib.rs)RemoveReaction(String)Ephemeral { text, user_id }RichMessage { text, blocks }ScheduledMessage { text, post_at }All four variants are handled exhaustively in every adapter (Slack, Discord, Telegram, Webhook) so the compiler enforces completeness.
Slack adapter (
src/messaging/slack.rs)app_mentionhandlinghandle_app_mention_eventfunction registered alongside the existinghandle_push_eventin the Socket Mode listener<@BOT_ID>token before forwarding text to the agentstart()viaauth.test; self-mentions filteredTyping indicator
send_status()was previously a no-op with an incorrect commentassistant.threads.setStatuswithThinking…/Working…/ empty string (to clear)thread_ts(regular channel messages outside an Assistant thread context)RemoveReactionreactions.removewith channel + timestamp via builder patternEphemeralchat.postEphemeralthreaded to the triggering message'sthread_tsRichMessagechat.postMessagewith aVec<SlackBlock>deserialised fromserde_json::ValueScheduledMessagechat.scheduleMessagewithpost_at: SlackDateTime(DateTime<Utc>)constructed from a caller-supplied Unix timestampbroadcast()RichMessagein addition toTextOther adapters
Platform-appropriate fallbacks — no behaviour change for existing traffic:
bot.send_message()response_type: text; RemoveReaction → earlyOk(())Testing
cargo check— clean (zero new errors; pre-existing warnings untouched)cargo test --lib— 43/43 passNotes
markdown_content()helper in the new outbound arms (~5 lines)