Skip to content

Commit 2594ea2

Browse files
committed
refactor(bot_turns): extract TurnAction helper + fix MultibotMentions gating
Addresses PR openabdev#487 review comments: 1. (bug) src/slack.rs MultibotMentions arm did `if other_bot { continue; }` without checking `mentions_bot` — logic didn't match the 'requiring @mention' debug message and could diverge from Discord parity. 2. (nit) The ~50-line `match tracker.on_bot_message(..) { TurnResult::.. }` block was duplicated in src/discord.rs and src/slack.rs. Extracted into `BotTurnTracker::classify_bot_message` returning a new `TurnAction` enum so adapters collapse to a 4-arm match. Severity hint keeps info! (soft) vs warn! (hard) log levels at the call site. Added 4 unit tests for the new helper.
1 parent c245915 commit 2594ea2

3 files changed

Lines changed: 186 additions & 51 deletions

File tree

src/bot_turns.rs

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,59 @@ impl BotTurnTracker {
6060
*hard = 0;
6161
}
6262
}
63+
64+
/// High-level decision for a bot message: increments the counter and
65+
/// returns what the adapter should do. Collapses the warn-once semantics
66+
/// and user-facing message formatting so Discord/Slack (and future adapters)
67+
/// don't duplicate the match.
68+
pub fn classify_bot_message(&mut self, thread_id: &str) -> TurnAction {
69+
match self.on_bot_message(thread_id) {
70+
TurnResult::Ok => TurnAction::Continue,
71+
TurnResult::SoftLimit(n) => TurnAction::WarnAndStop {
72+
severity: TurnSeverity::Soft,
73+
user_message: format!(
74+
"⚠️ Bot turn limit reached ({n}/{soft}). \
75+
A human must reply in this thread to continue bot-to-bot conversation.",
76+
soft = self.soft_limit,
77+
),
78+
},
79+
TurnResult::HardLimit => TurnAction::WarnAndStop {
80+
severity: TurnSeverity::Hard,
81+
user_message: format!(
82+
"🛑 Hard bot turn limit reached ({HARD_BOT_TURN_LIMIT}). \
83+
A human must reply to continue."
84+
),
85+
},
86+
TurnResult::Throttled | TurnResult::Stopped => TurnAction::SilentStop,
87+
}
88+
}
89+
}
90+
91+
/// Log severity hint for `TurnAction::WarnAndStop`.
92+
#[derive(Debug, PartialEq, Eq, Clone, Copy)]
93+
pub enum TurnSeverity {
94+
/// Soft limit — typically logged at `info!`.
95+
Soft,
96+
/// Hard absolute cap — typically logged at `warn!`.
97+
Hard,
98+
}
99+
100+
/// High-level action for a bot message after calling
101+
/// [`BotTurnTracker::classify_bot_message`].
102+
#[derive(Debug, PartialEq, Eq, Clone)]
103+
pub enum TurnAction {
104+
/// Safe to continue processing this bot message.
105+
Continue,
106+
/// Stop processing; if the message did not come from our own bot, the
107+
/// caller should post `user_message` to the thread so humans see why
108+
/// the bot went quiet.
109+
WarnAndStop {
110+
severity: TurnSeverity,
111+
user_message: String,
112+
},
113+
/// Stop processing silently — the warning was already sent on a previous
114+
/// turn; further warnings would spam the thread.
115+
SilentStop,
63116
}
64117

65118
#[cfg(test)]
@@ -191,4 +244,52 @@ mod tests {
191244
assert_eq!(t.on_bot_message("t1"), TurnResult::Ok);
192245
assert_eq!(t.on_bot_message("t1"), TurnResult::SoftLimit(3));
193246
}
247+
248+
#[test]
249+
fn classify_returns_continue_under_limits() {
250+
let mut t = BotTurnTracker::new(5);
251+
assert_eq!(t.classify_bot_message("t1"), TurnAction::Continue);
252+
}
253+
254+
#[test]
255+
fn classify_returns_warn_and_stop_on_soft_limit() {
256+
let mut t = BotTurnTracker::new(3);
257+
let _ = t.classify_bot_message("t1");
258+
let _ = t.classify_bot_message("t1");
259+
match t.classify_bot_message("t1") {
260+
TurnAction::WarnAndStop { severity, user_message } => {
261+
assert_eq!(severity, TurnSeverity::Soft);
262+
assert!(user_message.contains("3/3"));
263+
assert!(user_message.contains("⚠️"));
264+
}
265+
other => panic!("expected WarnAndStop(Soft), got {other:?}"),
266+
}
267+
}
268+
269+
#[test]
270+
fn classify_returns_silent_stop_past_soft_limit() {
271+
let mut t = BotTurnTracker::new(2);
272+
let _ = t.classify_bot_message("t1");
273+
let _ = t.classify_bot_message("t1"); // soft limit
274+
assert_eq!(t.classify_bot_message("t1"), TurnAction::SilentStop);
275+
assert_eq!(t.classify_bot_message("t1"), TurnAction::SilentStop);
276+
}
277+
278+
#[test]
279+
fn classify_returns_warn_and_stop_on_hard_limit() {
280+
let mut t = BotTurnTracker::new(HARD_BOT_TURN_LIMIT + 1);
281+
for _ in 0..HARD_BOT_TURN_LIMIT - 1 {
282+
let _ = t.classify_bot_message("t1");
283+
}
284+
match t.classify_bot_message("t1") {
285+
TurnAction::WarnAndStop { severity, user_message } => {
286+
assert_eq!(severity, TurnSeverity::Hard);
287+
assert!(user_message.contains("🛑"));
288+
assert!(user_message.contains(&HARD_BOT_TURN_LIMIT.to_string()));
289+
}
290+
other => panic!("expected WarnAndStop(Hard), got {other:?}"),
291+
}
292+
// Next call is silent (already warned).
293+
assert_eq!(t.classify_bot_message("t1"), TurnAction::SilentStop);
294+
}
194295
}

src/discord.rs

Lines changed: 70 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use crate::acp::ContentBlock;
22
use crate::acp::protocol::ConfigOption;
33
use crate::adapter::{AdapterRouter, ChatAdapter, ChannelRef, MessageRef, SenderContext};
4-
use crate::bot_turns::{BotTurnTracker, TurnResult, HARD_BOT_TURN_LIMIT};
4+
use crate::bot_turns::{BotTurnTracker, TurnAction, TurnSeverity};
55
use crate::config::{AllowBots, AllowUsers, SttConfig};
66
use crate::format;
77
use crate::media;
@@ -10,7 +10,7 @@ use std::sync::LazyLock;
1010
use serenity::builder::{CreateActionRow, CreateCommand, CreateInteractionResponse, CreateInteractionResponseMessage, CreateSelectMenu, CreateSelectMenuKind, CreateSelectMenuOption, CreateThread, EditMessage};
1111
use serenity::http::Http;
1212
use serenity::model::application::{ComponentInteractionDataKind, Interaction};
13-
use serenity::model::channel::{AutoArchiveDuration, Message, MessageType, ReactionType};
13+
use serenity::model::channel::{AutoArchiveDuration, ChannelType, Message, MessageType, ReactionType};
1414
use serenity::model::gateway::Ready;
1515
use serenity::model::id::{ChannelId, MessageId, UserId};
1616
use serenity::prelude::*;
@@ -259,30 +259,26 @@ impl EventHandler for Handler {
259259
let thread_key = msg.channel_id.to_string();
260260
let mut tracker = self.bot_turns.lock().await;
261261
if msg.author.bot {
262-
match tracker.on_bot_message(&thread_key) {
263-
TurnResult::HardLimit => {
264-
tracing::warn!(channel_id = %msg.channel_id, "hard bot turn limit reached");
265-
if msg.author.id != bot_id {
266-
let _ = msg.channel_id.say(
267-
&ctx.http,
268-
format!("🛑 Hard bot turn limit reached ({HARD_BOT_TURN_LIMIT}). A human must reply to continue."),
269-
).await;
262+
match tracker.classify_bot_message(&thread_key) {
263+
TurnAction::Continue => {}
264+
TurnAction::SilentStop => return,
265+
TurnAction::WarnAndStop { severity, user_message } => {
266+
match severity {
267+
TurnSeverity::Hard => tracing::warn!(
268+
channel_id = %msg.channel_id,
269+
"hard bot turn limit reached",
270+
),
271+
TurnSeverity::Soft => tracing::info!(
272+
channel_id = %msg.channel_id,
273+
max = self.max_bot_turns,
274+
"soft bot turn limit reached",
275+
),
270276
}
271-
return;
272-
}
273-
TurnResult::Stopped => return,
274-
TurnResult::SoftLimit(n) => {
275-
tracing::info!(channel_id = %msg.channel_id, turns = n, max = self.max_bot_turns, "soft bot turn limit reached");
276277
if msg.author.id != bot_id {
277-
let _ = msg.channel_id.say(
278-
&ctx.http,
279-
format!("⚠️ Bot turn limit reached ({n}/{}). A human must reply in this thread to continue bot-to-bot conversation.", self.max_bot_turns),
280-
).await;
278+
let _ = msg.channel_id.say(&ctx.http, &user_message).await;
281279
}
282280
return;
283281
}
284-
TurnResult::Throttled => return,
285-
TurnResult::Ok => {}
286282
}
287283
} else if matches!(msg.kind, MessageType::Regular | MessageType::InlineReply)
288284
&& !msg.content.is_empty()
@@ -359,9 +355,12 @@ impl EventHandler for Handler {
359355

360356
// Thread detection: single to_channel() call for both allowed and
361357
// non-allowed channels. A message is "in a thread" when the channel
362-
// has a parent_id AND the parent is in the allowlist (or allow_all).
358+
// type is a thread variant AND the parent is in the allowlist (or allow_all).
363359
let (in_thread, bot_owns_thread) = match msg.channel_id.to_channel(&ctx.http).await {
364-
Ok(serenity::model::channel::Channel::Guild(gc)) if gc.parent_id.is_some() => {
360+
Ok(serenity::model::channel::Channel::Guild(gc))
361+
if is_thread_channel(gc.kind) =>
362+
{
363+
// parent_id here points from thread → parent channel (not channel → category)
365364
let parent_allowed = in_allowed_channel
366365
|| self.allow_all_channels
367366
|| gc.parent_id.is_some_and(|pid| self.allowed_channels.contains(&pid.get()));
@@ -864,6 +863,12 @@ fn resolve_mentions(content: &str, bot_id: UserId) -> String {
864863
out.trim().to_string()
865864
}
866865

866+
/// Returns `true` if the given `ChannelType` is a Discord thread.
867+
/// Extracted for testability and to centralise thread detection logic.
868+
fn is_thread_channel(kind: ChannelType) -> bool {
869+
matches!(kind, ChannelType::PublicThread | ChannelType::PrivateThread | ChannelType::NewsThread)
870+
}
871+
867872
/// Pure decision function: should this message be processed or ignored?
868873
/// Returns `true` if the message should be processed (bot responds).
869874
/// Extracted from the EventHandler::message gating logic for testability.
@@ -1069,4 +1074,46 @@ mod tests {
10691074
false, // other_bot_present
10701075
));
10711076
}
1077+
1078+
// --- is_thread_channel tests (regression for #518) ---
1079+
// PR #506 used parent_id.is_some() to detect threads, but category text
1080+
// channels also have parent_id (pointing to the category). This caused
1081+
// the bot to skip thread creation for normal channels inside categories.
1082+
1083+
/// Regression test for #518: a text channel inside a category has parent_id
1084+
/// set but is NOT a thread — is_thread_channel must return false.
1085+
#[test]
1086+
fn category_text_channel_is_not_thread() {
1087+
assert!(!is_thread_channel(ChannelType::Text));
1088+
}
1089+
1090+
/// Category channel itself is not a thread.
1091+
#[test]
1092+
fn category_channel_is_not_thread() {
1093+
assert!(!is_thread_channel(ChannelType::Category));
1094+
}
1095+
1096+
/// Voice channel is not a thread.
1097+
#[test]
1098+
fn voice_channel_is_not_thread() {
1099+
assert!(!is_thread_channel(ChannelType::Voice));
1100+
}
1101+
1102+
/// PublicThread is correctly detected as a thread.
1103+
#[test]
1104+
fn public_thread_is_thread() {
1105+
assert!(is_thread_channel(ChannelType::PublicThread));
1106+
}
1107+
1108+
/// PrivateThread is correctly detected as a thread.
1109+
#[test]
1110+
fn private_thread_is_thread() {
1111+
assert!(is_thread_channel(ChannelType::PrivateThread));
1112+
}
1113+
1114+
/// NewsThread is correctly detected as a thread.
1115+
#[test]
1116+
fn news_thread_is_thread() {
1117+
assert!(is_thread_channel(ChannelType::NewsThread));
1118+
}
10721119
}

src/slack.rs

Lines changed: 15 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use crate::acp::ContentBlock;
22
use crate::adapter::{AdapterRouter, ChatAdapter, ChannelRef, MessageRef, SenderContext};
3-
use crate::bot_turns::{BotTurnTracker, TurnResult, HARD_BOT_TURN_LIMIT};
3+
use crate::bot_turns::{BotTurnTracker, TurnAction, TurnSeverity};
44
use crate::config::{AllowBots, AllowUsers, SttConfig};
55
use crate::media;
66
use anyhow::{anyhow, Result};
@@ -672,42 +672,25 @@ pub async fn run_slack_adapter(
672672
{
673673
let mut tracker = bot_turns.lock().await;
674674
if is_bot {
675-
match tracker.on_bot_message(&turn_key) {
676-
TurnResult::HardLimit => {
677-
warn!(channel_id, "hard bot turn limit reached");
678-
if !is_own_bot_msg {
679-
let warn_channel = ChannelRef {
680-
platform: "slack".into(),
681-
channel_id: channel_id.to_string(),
682-
thread_id: event["thread_ts"].as_str().map(|s| s.to_string()),
683-
parent_id: None,
684-
};
685-
let _ = adapter.send_message(
686-
&warn_channel,
687-
&format!("🛑 Hard bot turn limit reached ({HARD_BOT_TURN_LIMIT}). A human must reply to continue."),
688-
).await;
675+
match tracker.classify_bot_message(&turn_key) {
676+
TurnAction::Continue => {}
677+
TurnAction::SilentStop => continue,
678+
TurnAction::WarnAndStop { severity, user_message } => {
679+
match severity {
680+
TurnSeverity::Hard => warn!(channel_id, "hard bot turn limit reached"),
681+
TurnSeverity::Soft => info!(channel_id, max = max_bot_turns, "soft bot turn limit reached"),
689682
}
690-
continue;
691-
}
692-
TurnResult::Stopped => continue,
693-
TurnResult::SoftLimit(n) => {
694-
info!(channel_id, turns = n, max = max_bot_turns, "soft bot turn limit reached");
695683
if !is_own_bot_msg {
696684
let warn_channel = ChannelRef {
697685
platform: "slack".into(),
698686
channel_id: channel_id.to_string(),
699687
thread_id: event["thread_ts"].as_str().map(|s| s.to_string()),
700688
parent_id: None,
701689
};
702-
let _ = adapter.send_message(
703-
&warn_channel,
704-
&format!("⚠️ Bot turn limit reached ({n}/{max_bot_turns}). A human must reply in this thread to continue bot-to-bot conversation."),
705-
).await;
690+
let _ = adapter.send_message(&warn_channel, &user_message).await;
706691
}
707692
continue;
708693
}
709-
TurnResult::Throttled => continue,
710-
TurnResult::Ok => {}
711694
}
712695
} else {
713696
tracker.on_human_message(&turn_key);
@@ -813,8 +796,12 @@ pub async fn run_slack_adapter(
813796
debug!(channel_id, thread_ts, "bot not involved in thread, ignoring");
814797
continue;
815798
}
816-
if other_bot {
817-
debug!(channel_id, thread_ts, "multi-bot thread, requiring @mention");
799+
// In multi-bot threads, require @mention. The Slack
800+
// `app_mention` event usually handles the @-path on a
801+
// separate code path, but gate here too for Discord
802+
// parity and robustness if `app_mention` is missed.
803+
if other_bot && !mentions_bot {
804+
debug!(channel_id, thread_ts, "multi-bot thread without @mention, ignoring");
818805
continue;
819806
}
820807
}

0 commit comments

Comments
 (0)