diff --git a/.gitignore b/.gitignore index c58f29e8..1ef91ab7 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ config.toml .DS_Store .env .kiro/ +CLAUDE.md diff --git a/Cargo.lock b/Cargo.lock index 1deb9211..de2240b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -960,7 +960,7 @@ checksum = "384b8ab6d37215f3c5301a95a4accb5d64aa607f1fcb26a11b5303878451b4fe" [[package]] name = "openab" -version = "0.7.8" +version = "0.8.1" dependencies = [ "anyhow", "async-trait", diff --git a/config.toml.example b/config.toml.example index 8a317b28..ef3d7201 100644 --- a/config.toml.example +++ b/config.toml.example @@ -24,9 +24,12 @@ allowed_channels = ["1234567890"] # ↑ omitted + non-empty list → auto- # allowed_users = ["U0123456789"] # only checked when allow_all_users = false # allow_bot_messages = "off" # "off" (default) | "mentions" | "all" # trusted_bot_ids = [] # empty = any bot (mode permitting); set to restrict -# allow_user_messages = "involved" # "involved" (default) | "mentions" +# allow_user_messages = "involved" # "involved" (default) | "mentions" | "multibot-mentions" # "involved" = reply in threads bot has participated in # "mentions" = always require @mention + # "multibot-mentions" = like "involved", but require @mention + # once another bot has posted in the thread +# max_bot_turns = 20 # soft cap on consecutive bot turns per thread (human msg resets) [agent] command = "kiro-cli" diff --git a/docs/slack-bot-howto.md b/docs/slack-bot-howto.md index 8fa774e5..37898ab6 100644 --- a/docs/slack-bot-howto.md +++ b/docs/slack-bot-howto.md @@ -92,6 +92,26 @@ In a channel where the bot is invited: The bot will reply in a thread. After that, just type in the thread — no @mention needed for follow-ups. +## Slash commands are not supported on Slack + +openab supports `/models`, `/agents`, and `/cancel` on **Discord**, but **not on Slack**. If you previously configured these commands in your Slack app's **Slash Commands** page, you can safely delete them — the Slack adapter ignores both `slash_commands` and `interactive` envelope types. + +The root cause is a combination of three Slack-specific platform constraints, none of which is fixable from openab's side: + +1. **Slack blocks third-party slash commands inside threads.** Invoking `/models` from a thread's reply composer returns the Slack error `"/models is not supported in threads. Sorry!"`. This is enforced by the Slack client itself, not by any app setting — enabling Interactivity, Socket Mode, or reinstalling the app does not bypass it. Slack's built-in commands (`/remind`, `/shrug`, etc.) get special treatment that custom apps cannot. + +2. **Channel-level slash command payloads have no thread context.** If the user types `/models` in the channel's main composer instead of a thread, Slack delivers the command but the payload carries no `thread_ts`. Since openab keys each ACP session by thread (`slack:` or `slack:`), the command cannot be routed to the right session. Sessions are never keyed by `channel_id` alone, so there's no workaround on the adapter side. + +3. **Most ACP agents don't expose a model-switch surface.** Even when routing succeeded, `/models` reads the session's `configOptions` from the ACP `initialize` response. Only `kiro-cli` emits these in the expected format (via its `models`/`modes` fallback). `claude-code`, `codex`, `gemini`, `cursor-agent`, and `opencode` do not, so the menu would be empty for those backends — the user would see `"⚠️ No model options available"` with no recourse. + +On Discord, none of these apply: slash commands work in thread-channels, the channel ID *is* the thread key, and users typically stay within a single agent per deployment anyway. + +### If you need to switch models or agents with a Slack deployment + +- **Change the agent**: edit `[agent]` in `config.toml` (or the Helm chart values) and restart the pod / process +- **Change the Claude model** (for `claude-code`): set `ANTHROPIC_DEFAULT_MODEL` (or equivalent env var depending on your claude-code-acp version) and restart — model selection happens at process start, not at runtime +- **Cancel an in-flight turn**: there is no built-in way on Slack currently. + ## Finding Channel and User IDs - **Channel ID**: Right-click the channel name → **View channel details** → ID at the bottom (starts with `C` for public, `G` for private) diff --git a/src/bot_turns.rs b/src/bot_turns.rs new file mode 100644 index 00000000..9f031d14 --- /dev/null +++ b/src/bot_turns.rs @@ -0,0 +1,339 @@ +//! Per-thread bot turn tracking for runaway-loop prevention. +//! +//! Shared between Discord and Slack adapters so both platforms apply the same +//! soft/hard limit semantics. Both counters reset on a human message in the +//! thread. Runs before self-check so a bot's own messages count too — this +//! means `soft_limit=20` caps the *total* bot messages in a thread, not per-bot. + +use std::collections::HashMap; + +/// Absolute per-thread cap on consecutive bot turns without human intervention. +/// A human message resets both soft and hard counters to 0, allowing bots to +/// resume. This is *not* a lifetime total — it guards against runaway loops +/// between human resets. +pub const HARD_BOT_TURN_LIMIT: u32 = 100; + +#[derive(Debug, PartialEq, Eq)] +pub enum TurnResult { + /// Counter below limits — continue normally. + Ok, + /// Counter == soft_limit — warn once, then stop. + SoftLimit(u32), + /// Counter > soft_limit — silently stop (already warned). + Throttled, + /// Counter == HARD_BOT_TURN_LIMIT — warn once, then stop. + HardLimit, + /// Counter > HARD_BOT_TURN_LIMIT — silently stop (already warned). + Stopped, +} + +pub struct BotTurnTracker { + soft_limit: u32, + counts: HashMap, +} + +impl BotTurnTracker { + pub fn new(soft_limit: u32) -> Self { + Self { soft_limit, counts: HashMap::new() } + } + + pub fn on_bot_message(&mut self, thread_id: &str) -> TurnResult { + let (soft, hard) = self.counts.entry(thread_id.to_string()).or_insert((0, 0)); + *soft += 1; + *hard += 1; + if *hard > HARD_BOT_TURN_LIMIT { + TurnResult::Stopped + } else if *hard == HARD_BOT_TURN_LIMIT { + TurnResult::HardLimit + } else if *soft > self.soft_limit { + TurnResult::Throttled + } else if *soft == self.soft_limit { + TurnResult::SoftLimit(*soft) + } else { + TurnResult::Ok + } + } + + pub fn on_human_message(&mut self, thread_id: &str) { + if let Some((soft, hard)) = self.counts.get_mut(thread_id) { + *soft = 0; + *hard = 0; + } + } + + /// High-level decision for a bot message: increments the counter and + /// returns what the adapter should do. Collapses the warn-once semantics + /// and user-facing message formatting so Discord/Slack (and future adapters) + /// don't duplicate the match. + pub fn classify_bot_message(&mut self, thread_id: &str) -> TurnAction { + match self.on_bot_message(thread_id) { + TurnResult::Ok => TurnAction::Continue, + TurnResult::SoftLimit(n) => TurnAction::WarnAndStop { + severity: TurnSeverity::Soft, + turns: n, + user_message: format!( + "⚠️ Bot turn limit reached ({n}/{soft}). \ + A human must reply in this thread to continue bot-to-bot conversation.", + soft = self.soft_limit, + ), + }, + TurnResult::HardLimit => TurnAction::WarnAndStop { + severity: TurnSeverity::Hard, + turns: HARD_BOT_TURN_LIMIT, + user_message: format!( + "🛑 Hard bot turn limit reached ({HARD_BOT_TURN_LIMIT}). \ + A human must reply to continue." + ), + }, + TurnResult::Throttled | TurnResult::Stopped => TurnAction::SilentStop, + } + } +} + +/// Log severity hint for `TurnAction::WarnAndStop`. +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +pub enum TurnSeverity { + /// Soft limit — typically logged at `info!`. + Soft, + /// Hard absolute cap — typically logged at `warn!`. + Hard, +} + +/// High-level action for a bot message after calling +/// [`BotTurnTracker::classify_bot_message`]. +#[derive(Debug, PartialEq, Eq, Clone)] +pub enum TurnAction { + /// Safe to continue processing this bot message. + Continue, + /// Stop processing; if the message did not come from our own bot, the + /// caller should post `user_message` to the thread so humans see why + /// the bot went quiet. `turns` is the counter value at the warning + /// point — useful as a structured log field. + WarnAndStop { + severity: TurnSeverity, + turns: u32, + user_message: String, + }, + /// Stop processing silently — the warning was already sent on a previous + /// turn; further warnings would spam the thread. + SilentStop, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn bot_turns_increment() { + let mut t = BotTurnTracker::new(5); + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + } + + #[test] + fn soft_limit_triggers() { + let mut t = BotTurnTracker::new(3); + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(3)); + } + + #[test] + fn human_resets_both_counters() { + let mut t = BotTurnTracker::new(3); + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + t.on_human_message("t1"); + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(3)); + } + + #[test] + fn hard_limit_triggers() { + let mut t = BotTurnTracker::new(HARD_BOT_TURN_LIMIT + 1); + for _ in 0..HARD_BOT_TURN_LIMIT - 1 { + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + } + assert_eq!(t.on_bot_message("t1"), TurnResult::HardLimit); + } + + #[test] + fn hard_limit_resets_on_human() { + let mut t = BotTurnTracker::new(HARD_BOT_TURN_LIMIT + 1); + for _ in 0..HARD_BOT_TURN_LIMIT - 1 { + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + } + t.on_human_message("t1"); + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + } + + #[test] + fn hard_before_soft_when_equal() { + let mut t = BotTurnTracker::new(HARD_BOT_TURN_LIMIT); + for _ in 0..HARD_BOT_TURN_LIMIT - 1 { + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + } + assert_eq!(t.on_bot_message("t1"), TurnResult::HardLimit); + } + + #[test] + fn threads_are_independent() { + let mut t = BotTurnTracker::new(3); + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(3)); + assert_eq!(t.on_bot_message("t2"), TurnResult::Ok); + } + + #[test] + fn human_on_unknown_thread_is_noop() { + let mut t = BotTurnTracker::new(5); + t.on_human_message("unknown"); + } + + #[test] + fn two_bot_pingpong_hits_soft_limit() { + let mut t = BotTurnTracker::new(20); + for i in 1..20 { + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok, "turn {i}"); + } + assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(20)); + } + + #[test] + fn two_bot_pingpong_human_resets() { + let mut t = BotTurnTracker::new(20); + for _ in 0..15 { + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + } + t.on_human_message("t1"); + for _ in 0..15 { + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + } + for _ in 0..4 { + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + } + assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(20)); + } + + #[test] + fn soft_limit_warn_once_semantics() { + let mut t = BotTurnTracker::new(20); + for _ in 0..19 { + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + } + assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(20)); + assert_eq!(t.on_bot_message("t1"), TurnResult::Throttled); + assert_eq!(t.on_bot_message("t1"), TurnResult::Throttled); + } + + #[test] + fn hard_limit_warn_once_semantics() { + let mut t = BotTurnTracker::new(HARD_BOT_TURN_LIMIT + 1); + for _ in 0..HARD_BOT_TURN_LIMIT - 1 { + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + } + assert_eq!(t.on_bot_message("t1"), TurnResult::HardLimit); + assert_eq!(t.on_bot_message("t1"), TurnResult::Stopped); + } + + // System messages (thread created, pin, etc.) must not reset the counter. + // Filtering happens at the call site; this verifies the counter stays put + // when on_human_message is never called. Regression for openabdev/openab#497. + #[test] + fn system_message_does_not_reset_counter() { + let mut t = BotTurnTracker::new(3); + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); + assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(3)); + } + + #[test] + fn classify_returns_continue_under_limits() { + let mut t = BotTurnTracker::new(5); + assert_eq!(t.classify_bot_message("t1"), TurnAction::Continue); + } + + #[test] + fn classify_returns_warn_and_stop_on_soft_limit() { + let mut t = BotTurnTracker::new(3); + let _ = t.classify_bot_message("t1"); + let _ = t.classify_bot_message("t1"); + assert_eq!( + t.classify_bot_message("t1"), + TurnAction::WarnAndStop { + severity: TurnSeverity::Soft, + turns: 3, + user_message: "⚠️ Bot turn limit reached (3/3). \ + A human must reply in this thread to continue bot-to-bot conversation." + .to_string(), + }, + ); + } + + #[test] + fn classify_returns_silent_stop_past_soft_limit() { + let mut t = BotTurnTracker::new(2); + let _ = t.classify_bot_message("t1"); + let _ = t.classify_bot_message("t1"); + assert_eq!(t.classify_bot_message("t1"), TurnAction::SilentStop); + assert_eq!(t.classify_bot_message("t1"), TurnAction::SilentStop); + } + + #[test] + fn classify_returns_warn_and_stop_on_hard_limit() { + let mut t = BotTurnTracker::new(HARD_BOT_TURN_LIMIT + 1); + for _ in 0..HARD_BOT_TURN_LIMIT - 1 { + let _ = t.classify_bot_message("t1"); + } + assert_eq!( + t.classify_bot_message("t1"), + TurnAction::WarnAndStop { + severity: TurnSeverity::Hard, + turns: HARD_BOT_TURN_LIMIT, + user_message: format!( + "🛑 Hard bot turn limit reached ({HARD_BOT_TURN_LIMIT}). \ + A human must reply to continue." + ), + }, + ); + assert_eq!(t.classify_bot_message("t1"), TurnAction::SilentStop); + } + + #[test] + fn classify_is_per_thread_independent() { + let mut t = BotTurnTracker::new(2); + assert_eq!(t.classify_bot_message("t1"), TurnAction::Continue); + assert!(matches!( + t.classify_bot_message("t1"), + TurnAction::WarnAndStop { severity: TurnSeverity::Soft, .. }, + )); + assert_eq!(t.classify_bot_message("t2"), TurnAction::Continue); + assert!(matches!( + t.classify_bot_message("t2"), + TurnAction::WarnAndStop { severity: TurnSeverity::Soft, .. }, + )); + } + + // End-to-end: human message must fully reset classify behavior on the + // same thread, including unlocking new `Continue` responses. + #[test] + fn classify_resumes_after_human_message() { + let mut t = BotTurnTracker::new(2); + let _ = t.classify_bot_message("t1"); // Continue + assert!(matches!( + t.classify_bot_message("t1"), + TurnAction::WarnAndStop { .. }, + )); + // Without a human message, the next classify is silent. + assert_eq!(t.classify_bot_message("t1"), TurnAction::SilentStop); + // Human resets — classify starts at Continue again. + t.on_human_message("t1"); + assert_eq!(t.classify_bot_message("t1"), TurnAction::Continue); + assert!(matches!( + t.classify_bot_message("t1"), + TurnAction::WarnAndStop { severity: TurnSeverity::Soft, turns: 2, .. }, + )); + } +} diff --git a/src/config.rs b/src/config.rs index 5ca2506f..ce108350 100644 --- a/src/config.rs +++ b/src/config.rs @@ -153,6 +153,10 @@ pub struct SlackConfig { pub trusted_bot_ids: Vec, #[serde(default)] pub allow_user_messages: AllowUsers, + /// Max consecutive bot turns (without human intervention) before throttling. + /// Human message resets the counter. Default: 20. + #[serde(default = "default_max_bot_turns")] + pub max_bot_turns: u32, } #[derive(Debug, Deserialize)] diff --git a/src/discord.rs b/src/discord.rs index 4040e324..cbc25ab9 100644 --- a/src/discord.rs +++ b/src/discord.rs @@ -1,6 +1,7 @@ use crate::acp::ContentBlock; use crate::acp::protocol::ConfigOption; use crate::adapter::{AdapterRouter, ChatAdapter, ChannelRef, MessageRef, SenderContext}; +use crate::bot_turns::{BotTurnTracker, TurnAction, TurnSeverity}; use crate::config::{AllowBots, AllowUsers, SttConfig}; use crate::format; use crate::media; @@ -21,9 +22,6 @@ use tracing::{debug, error, info}; /// Prevents runaway loops between multiple bots in "all" mode. const MAX_CONSECUTIVE_BOT_TURNS: u8 = 10; -/// Absolute per-thread cap on bot turns. Cannot be overridden by config or human intervention. -const HARD_BOT_TURN_LIMIT: u32 = 100; - /// Maximum entries in the participation cache before eviction. const PARTICIPATION_CACHE_MAX: usize = 1000; @@ -261,30 +259,28 @@ impl EventHandler for Handler { let thread_key = msg.channel_id.to_string(); let mut tracker = self.bot_turns.lock().await; if msg.author.bot { - match tracker.on_bot_message(&thread_key) { - TurnResult::HardLimit => { - tracing::warn!(channel_id = %msg.channel_id, "hard bot turn limit reached"); - if msg.author.id != bot_id { - let _ = msg.channel_id.say( - &ctx.http, - format!("🛑 Hard bot turn limit reached ({HARD_BOT_TURN_LIMIT}). A human must reply to continue."), - ).await; + match tracker.classify_bot_message(&thread_key) { + TurnAction::Continue => {} + TurnAction::SilentStop => return, + TurnAction::WarnAndStop { severity, turns, user_message } => { + match severity { + TurnSeverity::Hard => tracing::warn!( + channel_id = %msg.channel_id, + turns, + "hard bot turn limit reached", + ), + TurnSeverity::Soft => tracing::info!( + channel_id = %msg.channel_id, + turns, + max = self.max_bot_turns, + "soft bot turn limit reached", + ), } - return; - } - TurnResult::Stopped => return, - TurnResult::SoftLimit(n) => { - tracing::info!(channel_id = %msg.channel_id, turns = n, max = self.max_bot_turns, "soft bot turn limit reached"); if msg.author.id != bot_id { - let _ = msg.channel_id.say( - &ctx.http, - format!("⚠️ Bot turn limit reached ({n}/{}). A human must reply in this thread to continue bot-to-bot conversation.", self.max_bot_turns), - ).await; + let _ = msg.channel_id.say(&ctx.http, &user_message).await; } return; } - TurnResult::Throttled => return, - TurnResult::Ok => {} } } else if matches!(msg.kind, MessageType::Regular | MessageType::InlineReply) && !msg.content.is_empty() @@ -812,58 +808,50 @@ async fn get_or_create_thread( parent_id: None, }; let trigger_ref = discord_msg_ref(msg); - adapter.create_thread(&parent, &trigger_ref, &thread_name).await -} - -// --- Bot turn tracking --- - -#[derive(Debug, PartialEq, Eq)] -pub(crate) enum TurnResult { - /// Counter below limits — continue normally. - Ok, - /// Counter == soft_limit — warn once, then stop. - SoftLimit(u32), - /// Counter > soft_limit — silently stop (already warned). - Throttled, - /// Counter == HARD_BOT_TURN_LIMIT — warn once, then stop. - HardLimit, - /// Counter > HARD_BOT_TURN_LIMIT — silently stop (already warned). - Stopped, -} - -pub(crate) struct BotTurnTracker { - soft_limit: u32, - counts: HashMap, -} - -impl BotTurnTracker { - pub fn new(soft_limit: u32) -> Self { - Self { soft_limit, counts: HashMap::new() } - } - - pub fn on_bot_message(&mut self, thread_id: &str) -> TurnResult { - let (soft, hard) = self.counts.entry(thread_id.to_string()).or_insert((0, 0)); - *soft += 1; - *hard += 1; - if *hard > HARD_BOT_TURN_LIMIT { - TurnResult::Stopped - } else if *hard == HARD_BOT_TURN_LIMIT { - TurnResult::HardLimit - } else if *soft > self.soft_limit { - TurnResult::Throttled - } else if *soft == self.soft_limit { - TurnResult::SoftLimit(*soft) - } else { - TurnResult::Ok + match adapter.create_thread(&parent, &trigger_ref, &thread_name).await { + Ok(ch) => Ok(ch), + Err(e) if is_thread_already_exists_error(&e) => { + // Another bot won the race from the same trigger message. Discord + // only allows one thread per message, so refetch the message and + // join the thread our sibling just created. + let refreshed = msg + .channel_id + .message(&ctx.http, msg.id) + .await + .map_err(|fe| anyhow::anyhow!( + "thread_already_exists (race), but refetch failed: {fe}" + ))?; + let existing = refreshed.thread.ok_or_else(|| { + anyhow::anyhow!( + "thread_already_exists (race), but message has no thread after refetch" + ) + })?; + tracing::info!( + channel_id = %msg.channel_id, + thread_id = %existing.id, + "joining thread created by sibling bot from same trigger message" + ); + Ok(ChannelRef { + platform: "discord".into(), + channel_id: existing.id.to_string(), + thread_id: None, + parent_id: Some(msg.channel_id.get().to_string()), + }) } + Err(e) => Err(e), } +} - pub fn on_human_message(&mut self, thread_id: &str) { - if let Some((soft, hard)) = self.counts.get_mut(thread_id) { - *soft = 0; - *hard = 0; - } - } +/// Detect Discord's "A thread has already been created for this message" error +/// (JSON error code 160004). Triggered when two bots responding to the same +/// @-mention race to create a thread from the same trigger message. +/// +/// Uses string matching because serenity surfaces Discord API errors as +/// formatted strings — there is no structured error code we can match on. +/// Unit tests pin the expected patterns so serenity formatting changes are caught. +fn is_thread_already_exists_error(err: &anyhow::Error) -> bool { + let msg = err.to_string(); + msg.contains("160004") || msg.contains("already been created") } static ROLE_MENTION_RE: LazyLock = LazyLock::new(|| { @@ -917,123 +905,6 @@ fn should_process_user_message( mod tests { use super::*; - // --- Bot turn tracker tests --- - - /// Basic increment: bot messages below the soft limit return Ok. - #[test] - fn bot_turns_increment() { - let mut t = BotTurnTracker::new(5); - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - } - - /// Soft limit: after N consecutive bot turns, returns SoftLimit. - #[test] - fn soft_limit_triggers() { - let mut t = BotTurnTracker::new(3); - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(3)); - } - - /// Human message resets both soft and hard counters, allowing bots to continue. - #[test] - fn human_resets_both_counters() { - let mut t = BotTurnTracker::new(3); - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - t.on_human_message("t1"); - // Both reset — can do 2 more before soft limit - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(3)); - } - - /// Hard limit: absolute cap on bot turns, triggers after HARD_BOT_TURN_LIMIT. - #[test] - fn hard_limit_triggers() { - let mut t = BotTurnTracker::new(HARD_BOT_TURN_LIMIT + 1); - for _ in 0..HARD_BOT_TURN_LIMIT - 1 { - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - } - assert_eq!(t.on_bot_message("t1"), TurnResult::HardLimit); - } - - /// Hard limit resets on human message, allowing bots to continue. - #[test] - fn hard_limit_resets_on_human() { - let mut t = BotTurnTracker::new(HARD_BOT_TURN_LIMIT + 1); - for _ in 0..HARD_BOT_TURN_LIMIT - 1 { - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - } - t.on_human_message("t1"); - // Hard counter reset — can go again - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - } - - /// When soft and hard limits are equal, hard limit takes precedence. - #[test] - fn hard_before_soft_when_equal() { - let mut t = BotTurnTracker::new(HARD_BOT_TURN_LIMIT); - for _ in 0..HARD_BOT_TURN_LIMIT - 1 { - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - } - // soft == hard == HARD_BOT_TURN_LIMIT → hard wins - assert_eq!(t.on_bot_message("t1"), TurnResult::HardLimit); - } - - /// Turn counters are per-thread — one thread hitting the limit doesn't affect others. - #[test] - fn threads_are_independent() { - let mut t = BotTurnTracker::new(3); - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(3)); - // t2 is unaffected - assert_eq!(t.on_bot_message("t2"), TurnResult::Ok); - } - - /// Human message on an unknown thread is a no-op (should not panic). - #[test] - fn human_on_unknown_thread_is_noop() { - let mut t = BotTurnTracker::new(5); - t.on_human_message("unknown"); // should not panic - } - - /// Two-bot ping-pong: both bots' messages count toward the same per-thread - /// limit. With soft_limit=20, the limit triggers after 20 total bot messages - /// (~10 per bot). This simulates what each bot's process sees when the - /// tracker runs before self-check — own messages are counted too. (#483) - #[test] - fn two_bot_pingpong_hits_soft_limit() { - let mut t = BotTurnTracker::new(20); - // Simulate 20 bot messages (alternating bot A and bot B, - // but the tracker doesn't distinguish — it just counts) - for i in 1..20 { - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok, "turn {i}"); - } - assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(20)); - } - - /// Human message in the middle of a ping-pong resets the counter, - /// allowing bots to continue. - #[test] - fn two_bot_pingpong_human_resets() { - let mut t = BotTurnTracker::new(20); - for _ in 0..15 { - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - } - t.on_human_message("t1"); // human intervenes at 15 - for _ in 0..15 { - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); // can do 15 more - } - // now at 15 again, 5 more to hit limit - for _ in 0..4 { - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - } - assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(20)); - } - // --- resolve_mentions tests --- /// Bot's own <@UID> mention is stripped from the prompt. @@ -1076,6 +947,35 @@ mod tests { assert_eq!(result, ""); } + // --- thread-race error detection --- + + /// Detects the Discord error code for "thread already exists" (160004). + #[test] + fn is_thread_already_exists_matches_code() { + let err = anyhow::Error::msg( + r#"HTTP error: {"code": 160004, "message": "A thread has already been created for this message."}"#, + ); + assert!(is_thread_already_exists_error(&err)); + } + + /// Detects the human-readable form of the error in case serenity renders + /// it without the numeric code. + #[test] + fn is_thread_already_exists_matches_message() { + let err = anyhow::anyhow!("A thread has already been created for this message."); + assert!(is_thread_already_exists_error(&err)); + } + + /// Unrelated errors do not match — we don't want the fallback path + /// swallowing real failures like permission denied. + #[test] + fn is_thread_already_exists_ignores_other_errors() { + let err = anyhow::anyhow!("Missing Permissions"); + assert!(!is_thread_already_exists_error(&err)); + let err = anyhow::anyhow!("rate limit exceeded"); + assert!(!is_thread_already_exists_error(&err)); + } + // --- should_process_user_message tests (GIVEN/WHEN/THEN) --- // Tests the multibot-mentions gating logic extracted from EventHandler::message. // The bug in #481 was that other bots' messages were filtered by bot gating @@ -1181,49 +1081,6 @@ mod tests { )); } - /// After soft limit fires once (n==20), subsequent bot messages still return - /// SoftLimit but with n>20. The caller warns only when n==max (exact hit), - /// preventing warning messages from ping-ponging between bots. - #[test] - fn soft_limit_warn_once_semantics() { - let mut t = BotTurnTracker::new(20); - for _ in 0..19 { - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - } - // n==20: exact hit — caller should send warning - assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(20)); - // n==21: past limit — caller should silently return (no warning) - assert_eq!(t.on_bot_message("t1"), TurnResult::Throttled); - // n==22: still past — still silent - assert_eq!(t.on_bot_message("t1"), TurnResult::Throttled); - } - - /// Hard limit also carries count for warn-once semantics. - #[test] - fn hard_limit_warn_once_semantics() { - let mut t = BotTurnTracker::new(HARD_BOT_TURN_LIMIT + 1); // soft > hard so hard fires first - for _ in 0..HARD_BOT_TURN_LIMIT - 1 { - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - } - // Exact hit — warn - assert_eq!(t.on_bot_message("t1"), TurnResult::HardLimit); - // Past — silent - assert_eq!(t.on_bot_message("t1"), TurnResult::Stopped); - } - - /// Regression test for #497: system messages (thread created, pin, etc.) - /// should NOT reset the bot turn counter. The filtering happens at the - /// call site (MessageType check); this verifies the counter stays put - /// when on_human_message is never called. - #[test] - fn system_message_does_not_reset_counter() { - let mut t = BotTurnTracker::new(3); - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - assert_eq!(t.on_bot_message("t1"), TurnResult::Ok); - // No on_human_message (system message filtered out at call site) - assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(3)); - } - // --- is_thread_channel tests (regression for #518) --- // PR #506 used parent_id.is_some() to detect threads, but category text // channels also have parent_id (pointing to the category). This caused diff --git a/src/main.rs b/src/main.rs index 53927f40..6cbfc161 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,5 +1,6 @@ mod acp; mod adapter; +mod bot_turns; mod config; mod discord; mod error_display; @@ -128,6 +129,7 @@ async fn main() -> anyhow::Result<()> { let router = router.clone(); let stt = cfg.stt.clone(); let session_ttl = std::time::Duration::from_secs(ttl_secs); + let max_bot_turns = slack_cfg.max_bot_turns; Some(tokio::spawn(async move { if let Err(e) = slack::run_slack_adapter( slack_cfg.bot_token, @@ -139,6 +141,7 @@ async fn main() -> anyhow::Result<()> { slack_cfg.allow_bot_messages, slack_cfg.trusted_bot_ids.into_iter().collect(), slack_cfg.allow_user_messages, + max_bot_turns, session_ttl, stt, router, @@ -190,7 +193,7 @@ async fn main() -> anyhow::Result<()> { multibot_threads: tokio::sync::Mutex::new(std::collections::HashMap::new()), session_ttl: std::time::Duration::from_secs(ttl_secs), max_bot_turns: discord_cfg.max_bot_turns, - bot_turns: tokio::sync::Mutex::new(discord::BotTurnTracker::new(discord_cfg.max_bot_turns)), + bot_turns: tokio::sync::Mutex::new(bot_turns::BotTurnTracker::new(discord_cfg.max_bot_turns)), }; let intents = GatewayIntents::GUILD_MESSAGES diff --git a/src/slack.rs b/src/slack.rs index 553b0922..1fc6f1e7 100644 --- a/src/slack.rs +++ b/src/slack.rs @@ -1,5 +1,6 @@ use crate::acp::ContentBlock; use crate::adapter::{AdapterRouter, ChatAdapter, ChannelRef, MessageRef, SenderContext}; +use crate::bot_turns::{BotTurnTracker, TurnAction, TurnSeverity}; use crate::config::{AllowBots, AllowUsers, SttConfig}; use crate::media; use anyhow::{anyhow, Result}; @@ -61,6 +62,9 @@ pub struct SlackAdapter { bot_id_cache: tokio::sync::Mutex>, /// Positive-only cache: thread_ts → cached_at for threads where bot has participated. participated_threads: tokio::sync::Mutex>, + /// Positive-only cache: thread_ts → cached_at for threads where other bots have posted. + /// Like participation, a thread becoming multi-bot is irreversible (bot messages don't disappear). + multibot_threads: tokio::sync::Mutex>, /// TTL for participation cache entries (matches session_ttl_hours from config). session_ttl: std::time::Duration, /// Controls streaming behavior: Off → streaming edit, Mentions/All → send-once. @@ -76,11 +80,21 @@ impl SlackAdapter { user_cache: tokio::sync::Mutex::new(HashMap::new()), bot_id_cache: tokio::sync::Mutex::new(HashMap::new()), participated_threads: tokio::sync::Mutex::new(HashMap::new()), + multibot_threads: tokio::sync::Mutex::new(HashMap::new()), session_ttl, allow_bot_messages, } } + /// Eagerly record that another bot has posted in a thread. Called from the + /// event loop when a bot message arrives, so multibot detection doesn't + /// depend on fetching thread history. Idempotent. + async fn note_other_bot_in_thread(&self, thread_ts: &str) { + let mut cache = self.multibot_threads.lock().await; + cache.entry(thread_ts.to_string()).or_insert_with(tokio::time::Instant::now); + enforce_cache_bounds(&mut cache, self.session_ttl); + } + /// Get the bot's own Slack user ID (cached after first call). async fn get_bot_user_id(&self) -> Option<&str> { self.bot_user_id.get_or_try_init(|| async { @@ -201,26 +215,33 @@ impl SlackAdapter { Some(user_id) } - /// Check if the bot has participated in a Slack thread. - /// Returns true if: parent message @mentions the bot, OR any message in thread is from the bot. - /// Fail-closed: returns false on API error (consistent with Discord's approach). - /// Only caches positive results (involved=true is irreversible). - async fn bot_participated_in_thread(&self, channel: &str, thread_ts: &str) -> bool { - // Check positive cache first - { + /// Check whether the bot has participated in a Slack thread and whether + /// other bots have also posted in it. + /// Returns `(involved, other_bot_present)`. + /// Involved = parent message @mentions the bot OR any message in thread is from the bot. + /// Fail-closed: returns `(false, false)` on API error (consistent with Discord's approach). + /// Caches positive results only — both states are irreversible. + async fn bot_participated_in_thread(&self, channel: &str, thread_ts: &str) -> (bool, bool) { + let cached_involved = { let cache = self.participated_threads.lock().await; - if let Some(cached_at) = cache.get(thread_ts) { - if cached_at.elapsed() < self.session_ttl { - return true; - } - } + cache.get(thread_ts).is_some_and(|ts| ts.elapsed() < self.session_ttl) + }; + let cached_multibot = { + let cache = self.multibot_threads.lock().await; + cache.get(thread_ts).is_some_and(|ts| ts.elapsed() < self.session_ttl) + }; + + // Eager multibot detection from message events populates the cache + // before this runs. When already involved and cached, skip the fetch. + if cached_involved { + return (true, cached_multibot); } let bot_id = match self.get_bot_user_id().await { Some(id) => id, None => { warn!("cannot resolve bot user ID, rejecting (fail-closed)"); - return false; + return (false, false); } }; @@ -240,49 +261,60 @@ impl SlackAdapter { Ok(json) => json, Err(e) => { warn!(channel, thread_ts, error = %e, "failed to fetch thread replies, rejecting (fail-closed)"); - return false; + return (false, false); } }; - let Some(messages) = json["messages"].as_array() else { return false }; + let Some(messages) = json["messages"].as_array() else { return (false, false) }; - // Check if parent message @mentions the bot let parent_mentions_bot = messages .first() .and_then(|m| m["text"].as_str()) .is_some_and(|text| text.contains(&format!("<@{bot_id}>"))); - // Check if any message in thread is from the bot let bot_posted = messages.iter().any(|m| m["user"].as_str() == Some(bot_id)); let involved = parent_mentions_bot || bot_posted; + let other_bot_present = cached_multibot + || messages.iter().any(|m| { + let is_bot_msg = m["bot_id"].is_string() + || m["subtype"].as_str() == Some("bot_message"); + is_bot_msg && m["user"].as_str() != Some(bot_id) + }); if involved { self.cache_participation(thread_ts).await; } + if other_bot_present && !cached_multibot { + self.note_other_bot_in_thread(thread_ts).await; + } - involved + (involved, other_bot_present) } /// Insert a positive participation entry, enforcing cache bounds. async fn cache_participation(&self, thread_ts: &str) { let mut cache = self.participated_threads.lock().await; - let now = tokio::time::Instant::now(); - - cache.insert(thread_ts.to_string(), now); - - if cache.len() > PARTICIPATION_CACHE_MAX { - // Evict expired entries first - cache.retain(|_, ts| ts.elapsed() < self.session_ttl); + cache.insert(thread_ts.to_string(), tokio::time::Instant::now()); + enforce_cache_bounds(&mut cache, self.session_ttl); + } +} - // If still over, evict oldest half - if cache.len() > PARTICIPATION_CACHE_MAX { - let mut entries: Vec<_> = cache.iter().map(|(k, v)| (k.clone(), *v)).collect(); - entries.sort_by_key(|(_, ts)| *ts); - let evict_count = entries.len() / 2; - for (key, _) in entries.into_iter().take(evict_count) { - cache.remove(&key); - } - } +/// Shared eviction policy for positive-only caches. +/// First drops expired entries; if still over, drops the oldest half. +fn enforce_cache_bounds( + cache: &mut HashMap, + ttl: std::time::Duration, +) { + if cache.len() <= PARTICIPATION_CACHE_MAX { + return; + } + cache.retain(|_, ts| ts.elapsed() < ttl); + if cache.len() > PARTICIPATION_CACHE_MAX { + let mut entries: Vec<_> = cache.iter().map(|(k, v)| (k.clone(), *v)).collect(); + entries.sort_by_key(|(_, ts)| *ts); + let evict_count = entries.len() / 2; + for (key, _) in entries.into_iter().take(evict_count) { + cache.remove(&key); } } } @@ -454,6 +486,7 @@ pub async fn run_slack_adapter( allow_bot_messages: AllowBots, trusted_bot_ids: HashSet, allow_user_messages: AllowUsers, + max_bot_turns: u32, session_ttl: std::time::Duration, stt_config: SttConfig, router: Arc, @@ -461,6 +494,7 @@ pub async fn run_slack_adapter( ) -> Result<()> { let adapter = Arc::new(SlackAdapter::new(bot_token.clone(), session_ttl, allow_bot_messages)); let queue = Arc::new(KeyedAsyncQueue::new()); + let bot_turns = Arc::new(tokio::sync::Mutex::new(BotTurnTracker::new(max_bot_turns))); loop { // Check for shutdown before (re)connecting @@ -504,6 +538,22 @@ pub async fn run_slack_adapter( .await; } + // Slash commands and interactive block_actions aren't + // handled on Slack: slash commands are blocked by Slack + // in thread composers, and the channel-level delivery + // lacks the thread_ts needed to route to a session. + // Ack only; ignore payload. + match envelope["type"].as_str() { + Some("slash_commands") | Some("interactive") => { + debug!( + envelope_type = envelope["type"].as_str().unwrap_or(""), + "ignoring Slack envelope type (not supported on this adapter)" + ); + continue; + } + _ => {} + } + // Route events if envelope["type"].as_str() == Some("events_api") { let event = &envelope["payload"]["event"]; @@ -551,7 +601,6 @@ pub async fn run_slack_adapter( let Some(_permit) = queue.acquire(&queue_key).await else { return }; handle_message( &event, - true, &adapter, &bot_token, allow_all_channels, @@ -571,12 +620,15 @@ pub async fn run_slack_adapter( || event["subtype"].as_str() == Some("bot_message"); let subtype = event["subtype"].as_str().unwrap_or(""); let msg_text = event["text"].as_str().unwrap_or(""); - let mentions_bot = if let Some(bot_uid) = adapter.get_bot_user_id().await { - msg_text.contains(&format!("<@{bot_uid}>")) - } else { - false - }; + let bot_uid_opt = adapter.get_bot_user_id().await.map(|s| s.to_string()); + let mentions_bot = bot_uid_opt + .as_ref() + .is_some_and(|bot_uid| msg_text.contains(&format!("<@{bot_uid}>"))); let is_dm = channel_id.starts_with('D'); + let event_user_id = event["user"].as_str(); + let is_own_bot_msg = is_bot + && bot_uid_opt.as_deref().is_some() + && event_user_id == bot_uid_opt.as_deref(); debug!( channel_id, @@ -597,6 +649,60 @@ pub async fn run_slack_adapter( ); if skip_subtype { continue; } + // --- Eager multibot detection --- + // Runs before self-check and bot gating so we always detect + // other bots even when allow_bot_messages=Off filters them out. + // Matches Discord #481 ordering. + if is_bot && !is_own_bot_msg { + if let Some(thread_ts) = event["thread_ts"].as_str() { + adapter.note_other_bot_in_thread(thread_ts).await; + } + } + + // --- Bot turn tracking --- + // Runs before self-check so ALL bot messages (including own) + // count toward the per-thread limit. Matches Discord #483. + // Keyed on thread_ts when in a thread, else channel:ts (the + // same key shape used for per-thread queueing below). + // Non-thread messages get a unique key per message, so the + // counter never accumulates — intentional, because bot-to-bot + // loops only happen inside threads. + let turn_key = if let Some(thread_ts) = event["thread_ts"].as_str() { + thread_ts.to_string() + } else { + format!("{}:{}", channel_id, event["ts"].as_str().unwrap_or("")) + }; + { + let mut tracker = bot_turns.lock().await; + if is_bot { + match tracker.classify_bot_message(&turn_key) { + TurnAction::Continue => {} + TurnAction::SilentStop => continue, + TurnAction::WarnAndStop { severity, turns, user_message } => { + match severity { + TurnSeverity::Hard => warn!(channel_id, turns, "hard bot turn limit reached"), + TurnSeverity::Soft => info!(channel_id, turns, max = max_bot_turns, "soft bot turn limit reached"), + } + if !is_own_bot_msg { + let warn_channel = ChannelRef { + platform: "slack".into(), + channel_id: channel_id.to_string(), + thread_id: event["thread_ts"].as_str().map(|s| s.to_string()), + parent_id: None, + }; + let _ = adapter.send_message(&warn_channel, &user_message).await; + } + continue; + } + } + } else if is_plain_user_message(subtype, msg_text) { + tracker.on_human_message(&turn_key); + } + } + + // Ignore own bot messages (after counting toward turns) + if is_own_bot_msg { continue; } + // Skip messages that @mention the bot — app_mention handles those // (except in DMs where app_mention doesn't fire) if mentions_bot && !is_dm { continue; } @@ -668,19 +774,43 @@ pub async fn run_slack_adapter( AllowUsers::Mentions => { if !mentions_bot { continue; } } - AllowUsers::Involved | AllowUsers::MultibotMentions => { + AllowUsers::Involved => { if !has_thread { - // Non-thread channel message: require mention - // (app_mention handles this, but DMs don't get app_mention) continue; } - // Thread message: check bot participation let thread_ts = event["thread_ts"].as_str().unwrap_or(""); - if !adapter.bot_participated_in_thread(channel_id, thread_ts).await { + let (involved, _) = adapter + .bot_participated_in_thread(channel_id, thread_ts) + .await; + if !involved { debug!(channel_id, thread_ts, "bot not involved in thread, ignoring"); continue; } } + AllowUsers::MultibotMentions => { + if !has_thread { + continue; + } + let thread_ts = event["thread_ts"].as_str().unwrap_or(""); + let (involved, other_bot) = adapter + .bot_participated_in_thread(channel_id, thread_ts) + .await; + if !involved { + debug!(channel_id, thread_ts, "bot not involved in thread, ignoring"); + continue; + } + // In multi-bot threads, require @mention — mirrors + // Discord's `should_process_user_message`. In practice + // mention-bearing message events are already deduped + // earlier (app_mention handles the @-path), so this + // branch rarely sees `mentions_bot == true`, but keep + // the explicit check so the logic is self-consistent + // and survives changes to the earlier dedup. + if other_bot && !mentions_bot { + debug!(channel_id, thread_ts, "multi-bot thread without @mention, ignoring"); + continue; + } + } } } } @@ -708,7 +838,6 @@ pub async fn run_slack_adapter( let Some(_permit) = queue.acquire(&queue_key).await else { return }; handle_message( &event, - is_dm, &adapter, &bot_token, allow_all_channels, @@ -780,7 +909,6 @@ async fn get_socket_mode_url(app_token: &str) -> Result { #[allow(clippy::too_many_arguments)] async fn handle_message( event: &serde_json::Value, - strip_mentions: bool, adapter: &Arc, bot_token: &str, allow_all_channels: bool, @@ -832,12 +960,10 @@ async fn handle_message( return; } - // Strip bot mention from text for @mention events; DMs and thread follow-ups pass through as-is - let prompt = if strip_mentions { - strip_slack_mention(&text) - } else { - text.trim().to_string() - }; + // Resolve mentions: strip only this bot's own trigger mention so the LLM + // can still @-mention other users in its reply. + let bot_id = adapter.get_bot_user_id().await; + let prompt = resolve_slack_mentions(&text, bot_id); // Process file attachments (images, audio) let files = event["files"].as_array(); @@ -847,17 +973,23 @@ async fn handle_message( return; } + // Caps mirror Discord's text-file attachment flow (PR #291) so both + // adapters apply the same limits: 5 files or 1 MB of text per message. + const TEXT_TOTAL_CAP: u64 = 1024 * 1024; + const TEXT_FILE_COUNT_CAP: u32 = 5; + let mut extra_blocks = Vec::new(); + let mut text_file_bytes: u64 = 0; + let mut text_file_count: u32 = 0; + if let Some(files) = files { for file in files { - let mimetype = file["mimetype"].as_str().unwrap_or(""); + let mimetype_raw = file["mimetype"].as_str().unwrap_or(""); + let mimetype = strip_mime_params(mimetype_raw); let filename = file["name"].as_str().unwrap_or("file"); let size = file["size"].as_u64().unwrap_or(0); // Slack private files require Bearer token to download - let url = file["url_private_download"] - .as_str() - .or_else(|| file["url_private"].as_str()) - .unwrap_or(""); + let url = slack_file_download_url(file); if url.is_empty() { continue; @@ -891,6 +1023,40 @@ async fn handle_message( }; let _ = adapter.add_reaction(&msg_ref, "🎤").await; } + } else if media::is_text_file(filename, Some(mimetype)) { + if text_file_count >= TEXT_FILE_COUNT_CAP { + debug!(filename, count = text_file_count, "text file count cap reached, skipping"); + continue; + } + // Pre-check with Slack-reported size as a fast path when the + // field is populated. Slack can report `size == 0` for + // externally-backed files, so this is advisory only — the + // authoritative cap check happens after download using + // `actual_bytes`. + if size > 0 && text_file_bytes + size > TEXT_TOTAL_CAP { + debug!(filename, total = text_file_bytes, "text attachments total exceeds 1MB cap, skipping remaining"); + continue; + } + if let Some((block, actual_bytes)) = media::download_and_read_text_file( + url, + filename, + size, + Some(bot_token), + ).await { + if text_file_bytes + actual_bytes > TEXT_TOTAL_CAP { + debug!( + filename, + running = text_file_bytes, + actual = actual_bytes, + "text attachments total exceeds 1MB cap after download, dropping file", + ); + continue; + } + text_file_bytes += actual_bytes; + text_file_count += 1; + debug!(filename, "adding text file attachment"); + extra_blocks.push(block); + } } else if let Some(block) = media::download_and_encode_image( url, Some(mimetype), @@ -960,11 +1126,55 @@ async fn handle_message( } } -static SLACK_MENTION_RE: LazyLock = - LazyLock::new(|| regex::Regex::new(r"<@[A-Z0-9]+>").unwrap()); +/// Strip only the bot's own `<@BOT_UID>` trigger mention. +/// Other users' mentions stay intact so the LLM can @-mention them back. +/// If the bot UID isn't known, fall back to returning the text trimmed — +/// safer than stripping all mentions and losing user addressability. +fn resolve_slack_mentions(text: &str, bot_id: Option<&str>) -> String { + match bot_id { + Some(id) => text.replace(&format!("<@{id}>"), "").trim().to_string(), + None => text.trim().to_string(), + } +} + +/// Pick the best download URL for a Slack file object. `url_private_download` +/// streams the raw bytes; `url_private` is the fallback for older file shapes. +/// Returns `""` when neither is present (caller should skip the file). +fn slack_file_download_url(file: &serde_json::Value) -> &str { + file["url_private_download"] + .as_str() + .or_else(|| file["url_private"].as_str()) + .unwrap_or("") +} + +/// Strip MIME parameters like `; charset=utf-8` so type-detection helpers see +/// the bare media type. Slack occasionally sends mimetypes like +/// `text/plain; charset=utf-8`; `media::is_text_file` expects the bare form. +fn strip_mime_params(mimetype: &str) -> &str { + mimetype.split(';').next().unwrap_or(mimetype).trim() +} -fn strip_slack_mention(text: &str) -> String { - SLACK_MENTION_RE.replace_all(text, "").trim().to_string() +/// True only when a Slack non-bot event represents a real user message +/// that should reset the bot-turn counter. +/// +/// Many Slack subtypes (pinned_item, channel_name, channel_archive, +/// group_join / group_leave / group_topic / group_purpose, reminder_add, +/// tombstone, …) carry a `user` field so the event loop sees +/// `is_bot == false`, but they represent administrative/system actions, +/// not conversation. Resetting the counter on them would let runaway +/// bot-to-bot loops re-arm whenever any pin / rename / archive happens. +/// +/// Mirrors Discord's `MessageType::Regular | InlineReply` + non-empty +/// content gate in `src/discord.rs`. Regression parity for +/// openabdev/openab#497. +fn is_plain_user_message(subtype: &str, text: &str) -> bool { + if text.is_empty() { + return false; + } + matches!( + subtype, + "" | "me_message" | "thread_broadcast" | "file_share", + ) } /// Convert Markdown (as output by Claude Code) to Slack mrkdwn format. @@ -996,6 +1206,147 @@ mod tests { use super::*; use crate::adapter::ChatAdapter; + /// Bot's own `<@UID>` trigger mention is stripped. + #[test] + fn resolve_mentions_strips_bot_mention() { + let out = resolve_slack_mentions("<@U1BOT> hello", Some("U1BOT")); + assert_eq!(out, "hello"); + } + + /// Other users' mentions are preserved so the LLM can address them back — + /// this is the core fix: the old `strip_slack_mention` wiped all `<@...>`. + #[test] + fn resolve_mentions_preserves_other_user_mentions() { + let out = resolve_slack_mentions("<@U1BOT> say hi to <@U2ALICE>", Some("U1BOT")); + assert_eq!(out, "say hi to <@U2ALICE>"); + } + + /// Multiple occurrences of the bot mention all get stripped. + #[test] + fn resolve_mentions_strips_repeated_bot_mentions() { + let out = resolve_slack_mentions("<@U1BOT> ping <@U1BOT>", Some("U1BOT")); + assert_eq!(out, "ping"); + } + + /// When the bot UID is unknown, fall back to preserving the text + /// (safer than stripping all user mentions). + #[test] + fn resolve_mentions_unknown_bot_preserves_all() { + let out = resolve_slack_mentions("<@U1BOT> hi <@U2ALICE>", None); + assert_eq!(out, "<@U1BOT> hi <@U2ALICE>"); + } + + // --- is_plain_user_message tests (regression for openabdev/openab#497 parity) --- + + /// Empty message text never counts as a user message (regardless of subtype). + #[test] + fn empty_text_is_not_plain_user_message() { + assert!(!is_plain_user_message("", "")); + assert!(!is_plain_user_message("me_message", "")); + } + + /// No subtype + non-empty text = plain user message (the common case). + #[test] + fn no_subtype_nonempty_text_is_plain_user_message() { + assert!(is_plain_user_message("", "hello")); + } + + /// Whitelisted subtypes with non-empty text are user messages. + #[test] + fn whitelisted_subtypes_are_plain_user_messages() { + assert!(is_plain_user_message("me_message", "waves")); + assert!(is_plain_user_message("thread_broadcast", "see channel")); + assert!(is_plain_user_message("file_share", "caption")); + } + + /// System-ish subtypes (even from real users) are NOT user messages — + /// resetting the counter on them would let bot-to-bot loops re-arm. + #[test] + fn system_subtypes_are_not_plain_user_messages() { + for subtype in [ + "pinned_item", + "unpinned_item", + "channel_name", + "channel_archive", + "channel_unarchive", + "group_join", + "group_leave", + "group_topic", + "group_purpose", + "reminder_add", + "tombstone", + ] { + assert!( + !is_plain_user_message(subtype, "some text"), + "subtype {subtype} must not count as a user message", + ); + } + } + + // --- slack_file_download_url tests --- + + /// Prefers url_private_download when both fields are present — + /// that endpoint always streams raw bytes even for browser-previewed types. + #[test] + fn slack_file_url_prefers_download_variant() { + let file = serde_json::json!({ + "url_private_download": "https://files.slack.com/.../download/log.txt", + "url_private": "https://files.slack.com/.../preview/log.txt", + }); + assert_eq!( + slack_file_download_url(&file), + "https://files.slack.com/.../download/log.txt", + ); + } + + /// Falls back to url_private when url_private_download is absent. + #[test] + fn slack_file_url_falls_back_to_private() { + let file = serde_json::json!({ + "url_private": "https://files.slack.com/.../log.txt", + }); + assert_eq!( + slack_file_download_url(&file), + "https://files.slack.com/.../log.txt", + ); + } + + /// Externally-backed files with no private URL return empty — caller skips. + #[test] + fn slack_file_url_empty_for_external_only() { + let file = serde_json::json!({ + "external_type": "gdrive", + "permalink": "https://docs.google.com/...", + }); + assert_eq!(slack_file_download_url(&file), ""); + } + + // --- strip_mime_params tests --- + + /// MIME with charset parameter strips to bare media type. + #[test] + fn strip_mime_params_removes_charset() { + assert_eq!(strip_mime_params("text/plain; charset=utf-8"), "text/plain"); + } + + /// Bare MIME is unchanged. + #[test] + fn strip_mime_params_bare_unchanged() { + assert_eq!(strip_mime_params("image/png"), "image/png"); + } + + /// Empty input is unchanged. + #[test] + fn strip_mime_params_empty() { + assert_eq!(strip_mime_params(""), ""); + } + + /// Surrounding whitespace is trimmed. + #[test] + fn strip_mime_params_trims_whitespace() { + assert_eq!(strip_mime_params(" text/plain "), "text/plain"); + } + /// Regression test: Slack streaming depends on allow_bot_messages config. /// Off → stream (better human UX), Mentions/All → send-once (avoids bot-to-bot interference). /// See PR #420 for design rationale.