Skip to content

Commit 96d5d5a

Browse files
committed
fix(slack): only reset bot turn counter on real user messages (parity with openabdev#497)
PR openabdev#498 fixed the bot-turn-reset bug in Discord by gating on_human_message behind MessageType::Regular | InlineReply + non-empty content. Slack had the same class of bug but only a partial defense: the skip_subtype blacklist dropped channel_join/leave/topic/purpose, message_changed/deleted, but many system-like subtypes still reached on_human_message and reset the counter: - pinned_item / unpinned_item - channel_name / channel_archive / channel_unarchive - group_join / group_leave / group_topic / group_purpose (private channels) - reminder_add - tombstone Extract is_plain_user_message(subtype, text) — a whitelist of "" | me_message | thread_broadcast | file_share with non-empty content — and gate the reset on it. Unit tests cover empty-text, whitelist, and the full system-subtype blacklist.
1 parent 3b9c07a commit 96d5d5a

1 file changed

Lines changed: 71 additions & 1 deletion

File tree

src/slack.rs

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -692,7 +692,7 @@ pub async fn run_slack_adapter(
692692
continue;
693693
}
694694
}
695-
} else {
695+
} else if is_plain_user_message(subtype, msg_text) {
696696
tracker.on_human_message(&turn_key);
697697
}
698698
}
@@ -1094,6 +1094,29 @@ fn resolve_slack_mentions(text: &str, bot_id: Option<&str>) -> String {
10941094
}
10951095
}
10961096

1097+
/// True only when a Slack non-bot event represents a real user message
1098+
/// that should reset the bot-turn counter.
1099+
///
1100+
/// Many Slack subtypes (pinned_item, channel_name, channel_archive,
1101+
/// group_join / group_leave / group_topic / group_purpose, reminder_add,
1102+
/// tombstone, …) carry a `user` field so the event loop sees
1103+
/// `is_bot == false`, but they represent administrative/system actions,
1104+
/// not conversation. Resetting the counter on them would let runaway
1105+
/// bot-to-bot loops re-arm whenever any pin / rename / archive happens.
1106+
///
1107+
/// Mirrors Discord's `MessageType::Regular | InlineReply` + non-empty
1108+
/// content gate in `src/discord.rs`. Regression parity for
1109+
/// openabdev/openab#497.
1110+
fn is_plain_user_message(subtype: &str, text: &str) -> bool {
1111+
if text.is_empty() {
1112+
return false;
1113+
}
1114+
matches!(
1115+
subtype,
1116+
"" | "me_message" | "thread_broadcast" | "file_share",
1117+
)
1118+
}
1119+
10971120
/// Convert Markdown (as output by Claude Code) to Slack mrkdwn format.
10981121
fn markdown_to_mrkdwn(text: &str) -> String {
10991122
static BOLD_RE: LazyLock<regex::Regex> =
@@ -1153,6 +1176,53 @@ mod tests {
11531176
assert_eq!(out, "<@U1BOT> hi <@U2ALICE>");
11541177
}
11551178

1179+
// --- is_plain_user_message tests (regression for openabdev/openab#497 parity) ---
1180+
1181+
/// Empty message text never counts as a user message (regardless of subtype).
1182+
#[test]
1183+
fn empty_text_is_not_plain_user_message() {
1184+
assert!(!is_plain_user_message("", ""));
1185+
assert!(!is_plain_user_message("me_message", ""));
1186+
}
1187+
1188+
/// No subtype + non-empty text = plain user message (the common case).
1189+
#[test]
1190+
fn no_subtype_nonempty_text_is_plain_user_message() {
1191+
assert!(is_plain_user_message("", "hello"));
1192+
}
1193+
1194+
/// Whitelisted subtypes with non-empty text are user messages.
1195+
#[test]
1196+
fn whitelisted_subtypes_are_plain_user_messages() {
1197+
assert!(is_plain_user_message("me_message", "waves"));
1198+
assert!(is_plain_user_message("thread_broadcast", "see channel"));
1199+
assert!(is_plain_user_message("file_share", "caption"));
1200+
}
1201+
1202+
/// System-ish subtypes (even from real users) are NOT user messages —
1203+
/// resetting the counter on them would let bot-to-bot loops re-arm.
1204+
#[test]
1205+
fn system_subtypes_are_not_plain_user_messages() {
1206+
for subtype in [
1207+
"pinned_item",
1208+
"unpinned_item",
1209+
"channel_name",
1210+
"channel_archive",
1211+
"channel_unarchive",
1212+
"group_join",
1213+
"group_leave",
1214+
"group_topic",
1215+
"group_purpose",
1216+
"reminder_add",
1217+
"tombstone",
1218+
] {
1219+
assert!(
1220+
!is_plain_user_message(subtype, "some text"),
1221+
"subtype {subtype} must not count as a user message",
1222+
);
1223+
}
1224+
}
1225+
11561226
/// Regression test: Slack streaming depends on allow_bot_messages config.
11571227
/// Off → stream (better human UX), Mentions/All → send-once (avoids bot-to-bot interference).
11581228
/// See PR #420 for design rationale.

0 commit comments

Comments
 (0)