feat: add Slack adapter with multi-platform ChatAdapter architecture#259
feat: add Slack adapter with multi-platform ChatAdapter architecture#259dogzzdogzz wants to merge 18 commits intoopenabdev:mainfrom
Conversation
dogzzdogzz
force-pushed
the
feat/slack-adapter
branch
2 times, most recently
from
6f10b36 to
c233136
Compare
PR Review: feat: add Slack adapter with multi-platform ChatAdapter architectureAuthor: Tony Lee ( 1. What problem does it solve?OpenAB previously only supported Discord. Many engineering teams use Slack as their primary communication tool, so not supporting Slack blocks a large segment of enterprise users. This PR does two things:
Closes #93 (Slack adapter proposal) and implements Phase 1–3 of RFC #94 (Multi-Platform Adapter Architecture). 2. How does it solve it?Clean architectural split:
3. Were alternatives considered?From the PR description and RFC #94:
4. Is this the best approach?Overall architecture is solid, the refactor is clean, and tests were correctly relocated to 🟢 INFO
🟡 NIT
🔴 SUGGESTED CHANGES
Overall this is a high-quality PR. The architecture aligns with the RFC, the refactor introduces no regressions, and Helm backward compatibility is maintained. The main areas to watch are Slack-side event dedup and graceful shutdown. Review by 超渡法師 🙏 |
dogzzdogzz
force-pushed
the
feat/slack-adapter
branch
from
ee1cc18 to
7327064
Compare
|
@chaodu-agent Thanks for the thorough review! Just pushed a commit addressing all feedback: 🔴 Suggested Changes:
🟡 NITs:
Commit: b04d9e2 |
|
Thanks for the detailed explanation @dogzzdogzz! The overall architecture is really clean. the ChatAdapter trait, centralising streaming/session/reaction logic in AdapterRouter, and the Socket Mode choice all make a lot of sense. We learned a lot reading through it. One thing we wanted to double-check with you. regarding "prompt streaming doesn't hold the pool lock". We wrote a small timing test to verify: The conns WriteGuard inside with_connection() can't be dropped while conn borrows from it, so the write lock is held for the full duration of f(conn).await — which includes the entire streaming loop. We tried a small fix: You can reproduce both results with This might be out of scope for this PR and if there's something we've missed or you see it differently, we'd love to hear it. Just wanted to make sure this was on the radar! 🙏 |
|
@antigenius0910 Great catch — you're absolutely right. This is a pre-existing upstream design in The Your test case would be a great addition to that discussion — concrete proof of the lock contention and the fix. Would you be up for posting it there? |
|
CI is failing. |
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
NIT Fixes — Inline Suggested Changes
Here are the concrete fixes for the NITs identified in the review. Each suggestion can be applied individually via GitHub's "Apply suggestion" button.
Summary:
Cargo.toml— Addlrucratesrc/config.rs— AddReactionEmojis::all_emojis()for validation (posted as regular comment — outside diff hunk)src/slack.rs—unicode_to_slack_emoji()returnsOption, addvalidate_emoji_mapping()startup checksrc/slack.rs— Replace unboundedHashMapuser cache withlru::LruCache(cap 1000)src/slack.rs— Reusemedia::HTTP_CLIENTinget_socket_mode_url()src/adapter.rs—#[must_use]onChatAdaptertrait methods, improved doc comments,reactions_emojis()accessor
config.rs — outside diff hunk
Add after impl Default for ReactionTiming block (after line 182):
impl ReactionEmojis {
/// Return all configured emoji values for validation purposes.
pub fn all_emojis(&self) -> Vec<&str> {
vec![
&self.queued, &self.thinking, &self.tool, &self.coding,
&self.web, &self.done, &self.error,
]
}
}Suggestions by 超渡法師 🙏
| base64 = "0.22" | ||
| image = { version = "0.25", default-features = false, features = ["jpeg", "png", "gif", "webp"] } | ||
| libc = "0.2" | ||
| tokio-tungstenite = { version = "0.21", features = ["rustls-tls-webpki-roots"] } |
There was a problem hiding this comment.
Add lru crate for bounded user cache:
| tokio-tungstenite = { version = "0.21", features = ["rustls-tls-webpki-roots"] } | |
| libc = "0.2" | |
| lru = "0.12" | |
| tokio-tungstenite = { version = "0.21", features = ["rustls-tls-webpki-roots"] } |
| use std::collections::{HashMap, HashSet}; | ||
| use std::sync::{Arc, LazyLock}; |
There was a problem hiding this comment.
Import NonZeroUsize for LRU capacity, drop HashMap:
| use std::collections::{HashMap, HashSet}; | |
| use std::sync::{Arc, LazyLock}; | |
| use std::collections::HashSet; | |
| use std::num::NonZeroUsize; |
| /// Map Unicode emoji to Slack short names for reactions API. | ||
| /// Only covers the default `[reactions.emojis]` set. Custom emoji configured | ||
| /// outside this map will fall back to `grey_question`. | ||
| fn unicode_to_slack_emoji(unicode: &str) -> &str { | ||
| match unicode { | ||
| "👀" => "eyes", | ||
| "🤔" => "thinking_face", | ||
| "🔥" => "fire", | ||
| "👨\u{200d}💻" => "technologist", | ||
| "⚡" => "zap", | ||
| "🆗" => "ok", | ||
| "😱" => "scream", | ||
| "🚫" => "no_entry_sign", | ||
| "😊" => "blush", | ||
| "😎" => "sunglasses", | ||
| "🫡" => "saluting_face", | ||
| "🤓" => "nerd_face", | ||
| "😏" => "smirk", | ||
| "✌\u{fe0f}" => "v", | ||
| "💪" => "muscle", | ||
| "🦾" => "mechanical_arm", | ||
| "🥱" => "yawning_face", | ||
| "😨" => "fearful", | ||
| "✅" => "white_check_mark", | ||
| "❌" => "x", | ||
| "🔧" => "wrench", | ||
| _ => "grey_question", | ||
| } | ||
| } |
There was a problem hiding this comment.
Return Option so unmapped emoji can be detected at startup. Add validate_emoji_mapping() to warn operators about missing mappings:
| /// Map Unicode emoji to Slack short names for reactions API. | |
| /// Only covers the default `[reactions.emojis]` set. Custom emoji configured | |
| /// outside this map will fall back to `grey_question`. | |
| fn unicode_to_slack_emoji(unicode: &str) -> &str { | |
| match unicode { | |
| "👀" => "eyes", | |
| "🤔" => "thinking_face", | |
| "🔥" => "fire", | |
| "👨\u{200d}💻" => "technologist", | |
| "⚡" => "zap", | |
| "🆗" => "ok", | |
| "😱" => "scream", | |
| "🚫" => "no_entry_sign", | |
| "😊" => "blush", | |
| "😎" => "sunglasses", | |
| "🫡" => "saluting_face", | |
| "🤓" => "nerd_face", | |
| "😏" => "smirk", | |
| "✌\u{fe0f}" => "v", | |
| "💪" => "muscle", | |
| "🦾" => "mechanical_arm", | |
| "🥱" => "yawning_face", | |
| "😨" => "fearful", | |
| "✅" => "white_check_mark", | |
| "❌" => "x", | |
| "🔧" => "wrench", | |
| _ => "grey_question", | |
| } | |
| } | |
| /// Map Unicode emoji to Slack short names for reactions API. | |
| /// Returns `None` for unmapped emoji. | |
| fn unicode_to_slack_emoji(unicode: &str) -> Option<&'static str> { | |
| match unicode { | |
| "👀" => Some("eyes"), | |
| "🤔" => Some("thinking_face"), | |
| "🔥" => Some("fire"), | |
| "👨\u200d💻" => Some("technologist"), | |
| "⚡" => Some("zap"), | |
| "🆗" => Some("ok"), | |
| "😱" => Some("scream"), | |
| "🚫" => Some("no_entry_sign"), | |
| "😊" => Some("blush"), | |
| "😎" => Some("sunglasses"), | |
| "🫡" => Some("saluting_face"), | |
| "🤓" => Some("nerd_face"), | |
| "😏" => Some("smirk"), | |
| "✌\ufe0f" => Some("v"), | |
| "💪" => Some("muscle"), | |
| "🦾" => Some("mechanical_arm"), | |
| "🥱" => Some("yawning_face"), | |
| "😨" => Some("fearful"), | |
| "✅" => Some("white_check_mark"), | |
| "❌" => Some("x"), | |
| "🔧" => Some("wrench"), | |
| _ => None, | |
| } | |
| } | |
| /// Validate that all configured reaction emojis have Slack mappings. | |
| /// Logs a warning at startup for any unmapped emoji. | |
| pub fn validate_emoji_mapping(emojis: &crate::config::ReactionEmojis) { | |
| for emoji in emojis.all_emojis() { | |
| if unicode_to_slack_emoji(emoji).is_none() { | |
| warn!( | |
| emoji, | |
| "reaction emoji has no Slack mapping — will show as :grey_question:" | |
| ); | |
| } | |
| } | |
| } |
| /// TTL for cached user display names (5 minutes). | ||
| const USER_CACHE_TTL: std::time::Duration = std::time::Duration::from_secs(300); | ||
|
|
||
| pub struct SlackAdapter { | ||
| client: reqwest::Client, | ||
| bot_token: String, | ||
| bot_user_id: tokio::sync::OnceCell<String>, | ||
| user_cache: tokio::sync::Mutex<HashMap<String, (String, tokio::time::Instant)>>, | ||
| } |
There was a problem hiding this comment.
Replace unbounded HashMap with lru::LruCache capped at 1000 entries to prevent slow memory growth in large workspaces:
| /// TTL for cached user display names (5 minutes). | |
| const USER_CACHE_TTL: std::time::Duration = std::time::Duration::from_secs(300); | |
| pub struct SlackAdapter { | |
| client: reqwest::Client, | |
| bot_token: String, | |
| bot_user_id: tokio::sync::OnceCell<String>, | |
| user_cache: tokio::sync::Mutex<HashMap<String, (String, tokio::time::Instant)>>, | |
| } | |
| /// TTL for cached user display names (5 minutes). | |
| const USER_CACHE_TTL: std::time::Duration = std::time::Duration::from_secs(300); | |
| /// Maximum number of cached user display names. | |
| const USER_CACHE_CAPACITY: NonZeroUsize = unsafe { NonZeroUsize::new_unchecked(1000) }; | |
| pub struct SlackAdapter { | |
| client: reqwest::Client, | |
| bot_token: String, | |
| bot_user_id: tokio::sync::OnceCell<String>, | |
| user_cache: tokio::sync::Mutex<lru::LruCache<String, (String, tokio::time::Instant)>>, | |
| } |
| client: reqwest::Client::new(), | ||
| bot_token, | ||
| bot_user_id: tokio::sync::OnceCell::new(), | ||
| user_cache: tokio::sync::Mutex::new(HashMap::new()), |
There was a problem hiding this comment.
Init with bounded LRU:
| user_cache: tokio::sync::Mutex::new(HashMap::new()), | |
| user_cache: tokio::sync::Mutex::new(lru::LruCache::new(USER_CACHE_CAPACITY)), |
| /// Edit an existing message in-place. | ||
| async fn edit_message(&self, msg: &MessageRef, content: &str) -> Result<()>; | ||
|
|
||
| /// Create a thread from a trigger message, returns the thread channel ref. |
There was a problem hiding this comment.
| /// Create a thread from a trigger message, returns the thread channel ref. | |
| /// Create a thread from a trigger message, returns the thread channel ref. | |
| #[must_use = "create_thread returns a Result that should be checked"] |
| /// Add a reaction/emoji to a message. | ||
| async fn add_reaction(&self, msg: &MessageRef, emoji: &str) -> Result<()>; |
There was a problem hiding this comment.
| /// Add a reaction/emoji to a message. | |
| async fn add_reaction(&self, msg: &MessageRef, emoji: &str) -> Result<()>; | |
| /// Add a reaction/emoji to a message. | |
| #[must_use = "add_reaction returns a Result that should be checked"] | |
| async fn add_reaction(&self, msg: &MessageRef, emoji: &str) -> Result<()>; |
| /// Remove a reaction/emoji from a message. | ||
| async fn remove_reaction(&self, msg: &MessageRef, emoji: &str) -> Result<()>; |
There was a problem hiding this comment.
| /// Remove a reaction/emoji from a message. | |
| async fn remove_reaction(&self, msg: &MessageRef, emoji: &str) -> Result<()>; | |
| /// Remove a reaction/emoji from a message. | |
| #[must_use = "remove_reaction returns a Result that should be checked"] | |
| async fn remove_reaction(&self, msg: &MessageRef, emoji: &str) -> Result<()>; |
| /// Shared logic for routing messages to ACP agents, managing sessions, | ||
| /// streaming edits, and controlling reactions. Platform-independent. | ||
| pub struct AdapterRouter { |
There was a problem hiding this comment.
Expanded doc comment for AdapterRouter:
| /// Shared logic for routing messages to ACP agents, managing sessions, | |
| /// streaming edits, and controlling reactions. Platform-independent. | |
| pub struct AdapterRouter { | |
| /// Shared logic for routing messages to ACP agents, managing sessions, | |
| /// streaming edits, and controlling reactions. | |
| /// | |
| /// Platform adapters call [`AdapterRouter::handle_message`] after resolving | |
| /// platform-specific details (threads, sender names, file attachments). | |
| /// The router owns the [`SessionPool`] and [`ReactionsConfig`], keeping | |
| /// all session and streaming logic in one place rather than duplicated | |
| /// per adapter. | |
| pub struct AdapterRouter { |
| } | ||
| } |
There was a problem hiding this comment.
Add accessor for emoji validation at startup:
| } | |
| } | |
| } | |
| } | |
| /// Return the configured reaction emojis (for startup validation). | |
| pub fn reactions_emojis(&self) -> &ReactionEmojis { | |
| &self.reactions_config.emojis | |
| } |
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
NIT Fixes — Inline Suggested Changes
Here are the concrete fixes for the NITs identified in the review. Each suggestion can be applied individually via GitHub's "Apply suggestion" button.
Summary:
Cargo.toml— Addlrucratesrc/config.rs— AddReactionEmojis::all_emojis()for validation (posted as regular comment — outside diff hunk)src/slack.rs—unicode_to_slack_emoji()returnsOption, addvalidate_emoji_mapping()startup checksrc/slack.rs— Replace unboundedHashMapuser cache withlru::LruCache(cap 1000)src/slack.rs— Reusemedia::HTTP_CLIENTinget_socket_mode_url()src/adapter.rs—#[must_use]onChatAdaptertrait methods, improved doc comments,reactions_emojis()accessor
config.rs — outside diff hunk
Add after impl Default for ReactionTiming block (after line 182):
impl ReactionEmojis {
/// Return all configured emoji values for validation purposes.
pub fn all_emojis(&self) -> Vec<&str> {
vec![
&self.queued, &self.thinking, &self.tool, &self.coding,
&self.web, &self.done, &self.error,
]
}
}Suggestions by 超渡法師 🙏
| base64 = "0.22" | ||
| image = { version = "0.25", default-features = false, features = ["jpeg", "png", "gif", "webp"] } | ||
| libc = "0.2" | ||
| tokio-tungstenite = { version = "0.21", features = ["rustls-tls-webpki-roots"] } |
There was a problem hiding this comment.
Add lru crate for bounded user cache:
| tokio-tungstenite = { version = "0.21", features = ["rustls-tls-webpki-roots"] } | |
| libc = "0.2" | |
| lru = "0.12" | |
| tokio-tungstenite = { version = "0.21", features = ["rustls-tls-webpki-roots"] } |
| use std::collections::{HashMap, HashSet}; | ||
| use std::sync::{Arc, LazyLock}; |
There was a problem hiding this comment.
Import NonZeroUsize for LRU capacity, drop HashMap:
| use std::collections::{HashMap, HashSet}; | |
| use std::sync::{Arc, LazyLock}; | |
| use std::collections::HashSet; | |
| use std::num::NonZeroUsize; |
| /// Map Unicode emoji to Slack short names for reactions API. | ||
| /// Only covers the default `[reactions.emojis]` set. Custom emoji configured | ||
| /// outside this map will fall back to `grey_question`. | ||
| fn unicode_to_slack_emoji(unicode: &str) -> &str { | ||
| match unicode { | ||
| "👀" => "eyes", | ||
| "🤔" => "thinking_face", | ||
| "🔥" => "fire", | ||
| "👨\u{200d}💻" => "technologist", | ||
| "⚡" => "zap", | ||
| "🆗" => "ok", | ||
| "😱" => "scream", | ||
| "🚫" => "no_entry_sign", | ||
| "😊" => "blush", | ||
| "😎" => "sunglasses", | ||
| "🫡" => "saluting_face", | ||
| "🤓" => "nerd_face", | ||
| "😏" => "smirk", | ||
| "✌\u{fe0f}" => "v", | ||
| "💪" => "muscle", | ||
| "🦾" => "mechanical_arm", | ||
| "🥱" => "yawning_face", | ||
| "😨" => "fearful", | ||
| "✅" => "white_check_mark", | ||
| "❌" => "x", | ||
| "🔧" => "wrench", | ||
| _ => "grey_question", | ||
| } | ||
| } |
There was a problem hiding this comment.
Return Option so unmapped emoji can be detected at startup. Add validate_emoji_mapping() to warn operators about missing mappings:
| /// Map Unicode emoji to Slack short names for reactions API. | |
| /// Only covers the default `[reactions.emojis]` set. Custom emoji configured | |
| /// outside this map will fall back to `grey_question`. | |
| fn unicode_to_slack_emoji(unicode: &str) -> &str { | |
| match unicode { | |
| "👀" => "eyes", | |
| "🤔" => "thinking_face", | |
| "🔥" => "fire", | |
| "👨\u{200d}💻" => "technologist", | |
| "⚡" => "zap", | |
| "🆗" => "ok", | |
| "😱" => "scream", | |
| "🚫" => "no_entry_sign", | |
| "😊" => "blush", | |
| "😎" => "sunglasses", | |
| "🫡" => "saluting_face", | |
| "🤓" => "nerd_face", | |
| "😏" => "smirk", | |
| "✌\u{fe0f}" => "v", | |
| "💪" => "muscle", | |
| "🦾" => "mechanical_arm", | |
| "🥱" => "yawning_face", | |
| "😨" => "fearful", | |
| "✅" => "white_check_mark", | |
| "❌" => "x", | |
| "🔧" => "wrench", | |
| _ => "grey_question", | |
| } | |
| } | |
| /// Map Unicode emoji to Slack short names for reactions API. | |
| /// Returns `None` for unmapped emoji. | |
| fn unicode_to_slack_emoji(unicode: &str) -> Option<&'static str> { | |
| match unicode { | |
| "👀" => Some("eyes"), | |
| "🤔" => Some("thinking_face"), | |
| "🔥" => Some("fire"), | |
| "👨\u200d💻" => Some("technologist"), | |
| "⚡" => Some("zap"), | |
| "🆗" => Some("ok"), | |
| "😱" => Some("scream"), | |
| "🚫" => Some("no_entry_sign"), | |
| "😊" => Some("blush"), | |
| "😎" => Some("sunglasses"), | |
| "🫡" => Some("saluting_face"), | |
| "🤓" => Some("nerd_face"), | |
| "😏" => Some("smirk"), | |
| "✌\ufe0f" => Some("v"), | |
| "💪" => Some("muscle"), | |
| "🦾" => Some("mechanical_arm"), | |
| "🥱" => Some("yawning_face"), | |
| "😨" => Some("fearful"), | |
| "✅" => Some("white_check_mark"), | |
| "❌" => Some("x"), | |
| "🔧" => Some("wrench"), | |
| _ => None, | |
| } | |
| } | |
| /// Validate that all configured reaction emojis have Slack mappings. | |
| /// Logs a warning at startup for any unmapped emoji. | |
| pub fn validate_emoji_mapping(emojis: &crate::config::ReactionEmojis) { | |
| for emoji in emojis.all_emojis() { | |
| if unicode_to_slack_emoji(emoji).is_none() { | |
| warn!( | |
| emoji, | |
| "reaction emoji has no Slack mapping — will show as :grey_question:" | |
| ); | |
| } | |
| } | |
| } |
| /// TTL for cached user display names (5 minutes). | ||
| const USER_CACHE_TTL: std::time::Duration = std::time::Duration::from_secs(300); | ||
|
|
||
| pub struct SlackAdapter { | ||
| client: reqwest::Client, | ||
| bot_token: String, | ||
| bot_user_id: tokio::sync::OnceCell<String>, | ||
| user_cache: tokio::sync::Mutex<HashMap<String, (String, tokio::time::Instant)>>, | ||
| } |
There was a problem hiding this comment.
Replace unbounded HashMap with lru::LruCache capped at 1000 entries to prevent slow memory growth in large workspaces:
| /// TTL for cached user display names (5 minutes). | |
| const USER_CACHE_TTL: std::time::Duration = std::time::Duration::from_secs(300); | |
| pub struct SlackAdapter { | |
| client: reqwest::Client, | |
| bot_token: String, | |
| bot_user_id: tokio::sync::OnceCell<String>, | |
| user_cache: tokio::sync::Mutex<HashMap<String, (String, tokio::time::Instant)>>, | |
| } | |
| /// TTL for cached user display names (5 minutes). | |
| const USER_CACHE_TTL: std::time::Duration = std::time::Duration::from_secs(300); | |
| /// Maximum number of cached user display names. | |
| const USER_CACHE_CAPACITY: NonZeroUsize = unsafe { NonZeroUsize::new_unchecked(1000) }; | |
| pub struct SlackAdapter { | |
| client: reqwest::Client, | |
| bot_token: String, | |
| bot_user_id: tokio::sync::OnceCell<String>, | |
| user_cache: tokio::sync::Mutex<lru::LruCache<String, (String, tokio::time::Instant)>>, | |
| } |
| client: reqwest::Client::new(), | ||
| bot_token, | ||
| bot_user_id: tokio::sync::OnceCell::new(), | ||
| user_cache: tokio::sync::Mutex::new(HashMap::new()), |
There was a problem hiding this comment.
Init with bounded LRU:
| user_cache: tokio::sync::Mutex::new(HashMap::new()), | |
| user_cache: tokio::sync::Mutex::new(lru::LruCache::new(USER_CACHE_CAPACITY)), |
| /// Edit an existing message in-place. | ||
| async fn edit_message(&self, msg: &MessageRef, content: &str) -> Result<()>; | ||
|
|
||
| /// Create a thread from a trigger message, returns the thread channel ref. |
There was a problem hiding this comment.
| /// Create a thread from a trigger message, returns the thread channel ref. | |
| /// Create a thread from a trigger message, returns the thread channel ref. | |
| #[must_use = "create_thread returns a Result that should be checked"] |
| /// Add a reaction/emoji to a message. | ||
| async fn add_reaction(&self, msg: &MessageRef, emoji: &str) -> Result<()>; |
There was a problem hiding this comment.
| /// Add a reaction/emoji to a message. | |
| async fn add_reaction(&self, msg: &MessageRef, emoji: &str) -> Result<()>; | |
| /// Add a reaction/emoji to a message. | |
| #[must_use = "add_reaction returns a Result that should be checked"] | |
| async fn add_reaction(&self, msg: &MessageRef, emoji: &str) -> Result<()>; |
| /// Remove a reaction/emoji from a message. | ||
| async fn remove_reaction(&self, msg: &MessageRef, emoji: &str) -> Result<()>; |
There was a problem hiding this comment.
| /// Remove a reaction/emoji from a message. | |
| async fn remove_reaction(&self, msg: &MessageRef, emoji: &str) -> Result<()>; | |
| /// Remove a reaction/emoji from a message. | |
| #[must_use = "remove_reaction returns a Result that should be checked"] | |
| async fn remove_reaction(&self, msg: &MessageRef, emoji: &str) -> Result<()>; |
| /// Shared logic for routing messages to ACP agents, managing sessions, | ||
| /// streaming edits, and controlling reactions. Platform-independent. | ||
| pub struct AdapterRouter { |
There was a problem hiding this comment.
Expanded doc comment for AdapterRouter:
| /// Shared logic for routing messages to ACP agents, managing sessions, | |
| /// streaming edits, and controlling reactions. Platform-independent. | |
| pub struct AdapterRouter { | |
| /// Shared logic for routing messages to ACP agents, managing sessions, | |
| /// streaming edits, and controlling reactions. | |
| /// | |
| /// Platform adapters call [`AdapterRouter::handle_message`] after resolving | |
| /// platform-specific details (threads, sender names, file attachments). | |
| /// The router owns the [`SessionPool`] and [`ReactionsConfig`], keeping | |
| /// all session and streaming logic in one place rather than duplicated | |
| /// per adapter. | |
| pub struct AdapterRouter { |
| } | ||
| } |
There was a problem hiding this comment.
Add accessor for emoji validation at startup:
| } | |
| } | |
| } | |
| } | |
| /// Return the configured reaction emojis (for startup validation). | |
| pub fn reactions_emojis(&self) -> &ReactionEmojis { | |
| &self.reactions_config.emojis | |
| } |
Checked #78 — the issue is already tracked there, and @ruan330 has validated the Arc fix in production. The RFC even lists it explicitly in Current State: "Write lock held during entire prompt streaming (#58)" Our timing test (2.01s → 1.00s) would be a concrete addition to that discussion since it gives a reproducible, quantified proof of the contention. Will post it there. Thanks for pointing us in the right direction! 🙏 |
@thepagent The latest run (https://github.com/openabdev/openab/actions/runs/24379004497) shows Summary of all review feedback and our response:
Happy to pick up deferred items in follow-up PRs if maintainers prefer. Wanted to keep this PR focused on the adapter architecture + Slack support. |
Introduces a platform-agnostic adapter layer (ChatAdapter trait + AdapterRouter) that decouples session management, streaming, and reactions from any specific chat platform. Refactors Discord support to implement this trait and adds a new Slack adapter using Socket Mode (WebSocket) with auto-reconnect. - Extract ChatAdapter trait with send/edit/thread/react methods + message_limit() - Extract AdapterRouter with shared session routing, edit-streaming, and reactions - Decouple StatusReactionController from serenity types (uses Arc<dyn ChatAdapter>) - Implement DiscordAdapter (ChatAdapter for Discord via serenity) - Implement SlackAdapter (ChatAdapter for Slack via reqwest + tokio-tungstenite) - Add [slack] config section (bot_token, app_token, allowed_channels, allowed_users) - Support running Discord + Slack simultaneously in one process - Session keys namespaced by platform (discord:xxx, slack:xxx) - Unicode emoji → Slack short name mapping for reactions Relates to openabdev#93, openabdev#94, openabdev#86 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract image resize/compress, download+encode, and STT download from discord.rs into shared media.rs module - Both Discord and Slack adapters now use media::download_and_encode_image() and media::download_and_transcribe() with optional auth_token parameter - Move sender context XML wrapping into AdapterRouter.handle_message() (was duplicated in each adapter) - Add image/audio attachment support to Slack adapter via files[] in app_mention events (requires files:read bot scope) - Slack file downloads use Bearer token auth (private URLs) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ollow-up - Fix strip_slack_mention to use LazyLock<Regex> instead of compiling per-call - Fix shorten_thread_name regex to use LazyLock, move to shared format.rs - Resolve Slack display name via users.info API (fallback to user_id) - Handle message events with thread_ts for thread follow-ups without @mention - Fix misleading allowed_channels comment (empty = allow all, not deny all) - Add allowed_channels docs to config.toml.example Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add slack config to values.yaml (enabled, botToken, appToken, allowedChannels, allowedUsers) - Generate [slack] section in configmap.yaml when slack.enabled=true - Store slack-bot-token and slack-app-token in secret.yaml - Inject SLACK_BOT_TOKEN and SLACK_APP_TOKEN env vars in deployment.yaml - Make [discord] section conditional (omitted when no botToken) - Add Slack setup instructions to NOTES.txt - Backward compatible: existing Discord-only configs work unchanged Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Update README.md: add Slack to description, architecture diagram, features, Quick Start (Discord/Slack toggle), config example, Helm example, Configuration Reference, and Project Structure - Add docs/slack-bot-howto.md: complete Slack bot setup guide - Add cross-reference from discord-bot-howto.md to slack-bot-howto.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add discord.enabled field to values.yaml (default: true) - Align all templates to use `and ($cfg.<platform>).enabled ($cfg.<platform>).botToken` pattern consistently across configmap, secret, deployment, and NOTES - Sync Cargo.lock version with upstream Cargo.toml (0.7.2) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Annotations - Add global labels/annotations in values.yaml, applied to all resources (Deployment, ConfigMap, Secret, PVC) via _helpers.tpl - Add per-agent podLabels/podAnnotations, applied to pod template in Deployment - Useful for team/cost-center labels, Prometheus scrape annotations, etc. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Slack sends message events with subtype "file_share" when users attach files in threads. The previous code blocked all subtypes, which prevented image/audio attachments in thread follow-ups from being processed. Now only skip subtypes that are truly non-user messages (edits, deletes, joins, leaves). Added debug logging for message event routing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- DiscordAdapter: initialize once via OnceLock instead of per-message - Slack dedup: skip message events that @mention the bot (app_mention handles those) using bot user ID from auth.test API - Slack graceful shutdown: watch channel + tokio::select! + WebSocket Close frame, with 5s timeout in main.rs - Cache users.info results for 5 minutes to avoid Slack rate limits - Document unicode_to_slack_emoji fallback behavior - Document discord.enabled + empty botToken behavior in values.yaml Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Allow clippy::too_many_arguments on handle_message (8 params, all distinct) - Replace map_or with is_some_and for has_files check Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Code outputs standard Markdown which Discord renders natively but Slack does not. Add markdown_to_mrkdwn() conversion in SlackAdapter: - **bold** → *bold* (Slack bold) - *italic* → _italic_ (Slack italic) - [text](url) → <url|text> (Slack links) - # heading → *heading* (bold as substitute, Slack has no headings) - ```lang → ``` (strip language hints from code blocks) Applied in send_message and edit_message. Discord adapter unchanged (Markdown works natively). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dogzzdogzz
force-pushed
the
feat/slack-adapter
branch
from
1adca5d to
37b6275
Compare
Optional field — only rendered when explicitly set in values. No default value, fully backward compatible. Allows disabling service account token mount for security hardening. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve conflicts with upstream openabdev#321 (allow_bot_messages): - Port AllowBots enum + bot message gating + trusted_bot_ids into our refactored DiscordAdapter Handler - Add allow_bot_messages/trusted_bot_ids to Handler struct and main.rs Discord startup section Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve conflicts with upstream: - openabdev#321: allow_bot_messages + trustedBotIds (ported into DiscordAdapter Handler) - openabdev#191: setup wizard + clap CLI (wrapped our multi-adapter logic inside Commands::Run) - Helm: allowBotMessages/trustedBotIds config + validation in configmap/values - Cargo: added clap dependency alongside our async-trait/futures-util/tokio-tungstenite Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
This is a significant architectural change (multi-platform ChatAdapter + Slack support). To ensure proper design discussion and tracking, could you please create a Discord discussion thread and add the link to this PR description? This helps the community follow along and provide feedback before we merge a large refactor. Thanks! 🙏 |
|
@thepagent Added the Discord discussion link to the PR description: https://discord.com/channels/1491295327620169908/1491969620754567270/1491969624135438386 Also updated test count from 26 to 40 (includes upstream's new tests after merge). |
Resolve conflicts: - format.rs: keep shorten_thread_name (ours), remove truncate_chars (upstream removed it in favor of tail-priority truncation) - adapter.rs: replace truncate_chars with tail-priority truncation (keep last N chars during streaming, matching upstream's approach) - discord.rs: keep our refactored DiscordAdapter version (upstream changes to compose_display/tool collapse are in their monolithic discord.rs which we replaced) - Dockerfiles: upstream merged our openabdev#335 fix + added procps Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve discord.rs conflict — take our refactored version and port upstream's 🎤 reaction for voice messages when STT is disabled (openabdev#252). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve README.md conflict — merge our Slack additions with upstream's OpenCode CLI support. values.yaml and config.toml.example auto-merged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5 participants
Discord discussion: https://discord.com/channels/1491295327620169908/1491969620754567270/1491969624135438386
Summary
ChatAdaptertrait +AdapterRouter, decoupling session management, streaming, and reactions from any specific chat platformmedia.rsfor cross-adapter reuseSessionPoolwith namespaced keysCloses #93. Implements Phase 1–3 of RFC #94.
Changes
src/adapter.rsChatAdaptertrait,ChannelRef/MessageRef/SenderContext,AdapterRoutersrc/slack.rsSlackAdapter+ Socket Mode event loopsrc/media.rssrc/discord.rsDiscordAdapterimplementsChatAdapter, delegates to routersrc/reactions.rsArc<dyn ChatAdapter>src/format.rsshorten_thread_name()src/config.rsdiscordnow optional, addedSlackConfigsrc/main.rsconfig.toml.example[slack]section with docscharts/openab/values.yamlslackconfig per agent (enabled, botToken, appToken, allowedChannels, allowedUsers)charts/openab/templates/configmap.yaml[discord]and/or[slack]sectionscharts/openab/templates/secret.yamlslack-bot-token+slack-app-tokenalongside discord tokencharts/openab/templates/deployment.yamlSLACK_BOT_TOKEN+SLACK_APP_TOKENenv varscharts/openab/templates/NOTES.txtDesign Decisions
discord:xxx,slack:xxx) to prevent collisionAdapterRouter(not duplicated per adapter)media.rswith optionalauth_tokenparam (Discord CDN is public, Slack files need Bearer token)eyesnot👀)users.infoAPI (Discord provides names in message payload, Slack only gives user ID)messageevents withthread_tshandled without requiring re-mention (parity with Discord)[discord]section now conditional (omitted when no botToken)Helm Usage
Test plan
cargo build— zero warningscargo test— 40 tests passcargo clippy— cleanhelm template— Discord-only, Slack-only, and both modes validated🤖 Generated with Claude Code