Skip to content

Commit 1f8864c

Browse files
howiechaodu-agent
andauthored
fix(slack): match both bare and labelled mention forms in mentions_bot (#808)
- Add text_mentions_uid() helper accepting both <@uid> and <@uid|handle> - Rewrite resolve_slack_mentions() to strip both forms symmetrically - Add .inspect_err() warning on get_bot_user_id failure - Add doc comment to pool.rs with_connection Fixes #800 Co-authored-by: chaodufashi <chaodu-agent@users.noreply.github.com>
1 parent d06bf26 commit 1f8864c

2 files changed

Lines changed: 132 additions & 6 deletions

File tree

src/acp/pool.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,10 @@ impl SessionPool {
257257
}
258258

259259
/// Get mutable access to a connection. Caller must have called get_or_create first.
260+
///
261+
/// Only the per-connection `Mutex` is held during `f`; the pool-level
262+
/// `RwLock` is acquired briefly (read-only) to look up the `Arc` and then
263+
/// released, so other connections can be used concurrently.
260264
pub async fn with_connection<F, R>(&self, thread_id: &str, f: F) -> Result<R>
261265
where
262266
F: for<'a> FnOnce(

src/slack.rs

Lines changed: 128 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,7 @@ impl SlackAdapter {
117117
.ok_or_else(|| anyhow!("no user_id in auth.test response"))
118118
})
119119
.await
120+
.inspect_err(|e| warn!(error = %e, "bot user ID unavailable; mention detection may suppress bot messages under Mentions mode"))
120121
.ok()
121122
.map(|s| s.as_str())
122123
}
@@ -307,7 +308,7 @@ impl SlackAdapter {
307308
let parent_mentions_bot = messages
308309
.first()
309310
.and_then(|m| m["text"].as_str())
310-
.is_some_and(|text| text.contains(&format!("<@{bot_id}>")));
311+
.is_some_and(|text| text_mentions_uid(text, bot_id));
311312

312313
let bot_posted = messages.iter().any(|m| m["user"].as_str() == Some(bot_id));
313314

@@ -607,7 +608,7 @@ pub async fn run_slack_adapter(
607608
let bot_uid_opt = adapter.get_bot_user_id().await.map(|s| s.to_string());
608609
let mentions_bot = bot_uid_opt
609610
.as_ref()
610-
.is_some_and(|bot_uid| msg_text.contains(&format!("<@{bot_uid}>")));
611+
.is_some_and(|bot_uid| text_mentions_uid(msg_text, bot_uid));
611612
let is_dm = channel_id.starts_with('D');
612613
let event_user_id = event["user"].as_str();
613614
let is_own_bot_msg = is_bot
@@ -1168,15 +1169,41 @@ async fn handle_message(
11681169
}
11691170
}
11701171

1171-
/// Strip only the bot's own `<@BOT_UID>` trigger mention.
1172+
/// Strip all occurrences of the bot's own `<@BOT_UID>` or `<@BOT_UID|handle>` mention.
11721173
/// Other users' mentions stay intact so the LLM can @-mention them back.
11731174
/// If the bot UID isn't known, fall back to returning the text trimmed —
11741175
/// safer than stripping all mentions and losing user addressability.
11751176
fn resolve_slack_mentions(text: &str, bot_id: Option<&str>) -> String {
1176-
match bot_id {
1177-
Some(id) => text.replace(&format!("<@{id}>"), "").trim().to_string(),
1178-
None => text.trim().to_string(),
1177+
let Some(id) = bot_id else {
1178+
return text.trim().to_string();
1179+
};
1180+
let prefix = format!("<@{id}");
1181+
let mut out = String::with_capacity(text.len());
1182+
let mut s = text;
1183+
while let Some(pos) = s.find(&prefix) {
1184+
let after = &s[pos + prefix.len()..];
1185+
match after.as_bytes().first() {
1186+
Some(b'>') => {
1187+
out.push_str(&s[..pos]);
1188+
s = &after[1..];
1189+
}
1190+
Some(b'|') => {
1191+
if let Some(close) = after.find('>') {
1192+
out.push_str(&s[..pos]);
1193+
s = &after[close + 1..];
1194+
} else {
1195+
out.push_str(&s[..pos + prefix.len()]);
1196+
s = after;
1197+
}
1198+
}
1199+
_ => {
1200+
out.push_str(&s[..pos + prefix.len()]);
1201+
s = after;
1202+
}
1203+
}
11791204
}
1205+
out.push_str(s);
1206+
out.trim().to_string()
11801207
}
11811208

11821209
/// Pick the best download URL for a Slack file object. `url_private_download`
@@ -1196,6 +1223,18 @@ fn strip_mime_params(mimetype: &str) -> &str {
11961223
mimetype.split(';').next().unwrap_or(mimetype).trim()
11971224
}
11981225

1226+
/// Returns `true` if `text` contains a Slack user mention for `uid`.
1227+
///
1228+
/// Accepts both `<@U...>` (bare) and `<@U...|handle>` (labelled) wire forms.
1229+
/// Slack (and bots addressing peers) can emit the labelled form; `<@UID>` is
1230+
/// not a substring of `<@UID|handle>`, so a bare `contains("<@UID>")` silently
1231+
/// misses it.
1232+
fn text_mentions_uid(text: &str, uid: &str) -> bool {
1233+
let prefix = format!("<@{uid}");
1234+
text.match_indices(&prefix)
1235+
.any(|(i, _)| matches!(text.as_bytes().get(i + prefix.len()), Some(b'>') | Some(b'|')))
1236+
}
1237+
11991238
fn bot_id_matches_trusted(
12001239
trusted_bot_ids: &HashSet<String>,
12011240
event_bot_id: &str,
@@ -1291,6 +1330,89 @@ mod tests {
12911330
assert_eq!(out, "<@U1BOT> hi <@U2ALICE>");
12921331
}
12931332

1333+
/// Labelled form of another user's mention (`<@UID|handle>`) is preserved.
1334+
#[test]
1335+
fn resolve_mentions_preserves_labelled_other_user_mention() {
1336+
let out = resolve_slack_mentions("<@U1BOT> say hi to <@U2ALICE|alice>", Some("U1BOT"));
1337+
assert_eq!(out, "say hi to <@U2ALICE|alice>");
1338+
}
1339+
1340+
/// Labelled form `<@UID|handle>` is stripped the same as bare form.
1341+
#[test]
1342+
fn resolve_mentions_strips_labelled_bot_mention() {
1343+
let out = resolve_slack_mentions("<@U1BOT|my-bot> hello", Some("U1BOT"));
1344+
assert_eq!(out, "hello");
1345+
}
1346+
1347+
/// Labelled form mid-sentence is stripped and surrounding text preserved.
1348+
#[test]
1349+
fn resolve_mentions_strips_labelled_mid_sentence() {
1350+
let out = resolve_slack_mentions("please ask <@U1BOT|handle> to run", Some("U1BOT"));
1351+
assert_eq!(out, "please ask to run");
1352+
}
1353+
1354+
/// Mixed bare and labelled forms of the same UID in one string are both stripped.
1355+
#[test]
1356+
fn resolve_mentions_strips_mixed_bare_and_labelled() {
1357+
let out = resolve_slack_mentions("<@U1BOT> and <@U1BOT|handle> run", Some("U1BOT"));
1358+
assert_eq!(out, "and run");
1359+
}
1360+
1361+
/// Malformed unclosed `<@UID|label` (no closing `>`) is preserved verbatim.
1362+
#[test]
1363+
fn resolve_mentions_malformed_unclosed_label_preserved() {
1364+
let out = resolve_slack_mentions("ask <@U1BOT|nolabel to run", Some("U1BOT"));
1365+
assert!(out.contains("<@U1BOT"));
1366+
}
1367+
1368+
#[test]
1369+
fn resolve_mentions_preserves_longer_uid_prefix() {
1370+
let out = resolve_slack_mentions("<@U1BOTX> hello", Some("U1BOT"));
1371+
assert_eq!(out, "<@U1BOTX> hello");
1372+
}
1373+
1374+
// --- text_mentions_uid tests ---
1375+
1376+
#[test]
1377+
fn mentions_uid_bare_form() {
1378+
assert!(text_mentions_uid("<@U123BOT> hello", "U123BOT"));
1379+
}
1380+
1381+
#[test]
1382+
fn mentions_uid_labelled_form() {
1383+
assert!(text_mentions_uid("<@U123BOT|my-bot> hello", "U123BOT"));
1384+
}
1385+
1386+
#[test]
1387+
fn mentions_uid_labelled_form_mid_sentence() {
1388+
assert!(text_mentions_uid("please ask <@U123BOT|handle> to run", "U123BOT"));
1389+
}
1390+
1391+
#[test]
1392+
fn mentions_uid_no_match() {
1393+
assert!(!text_mentions_uid("hello world", "U123BOT"));
1394+
}
1395+
1396+
#[test]
1397+
fn mentions_uid_no_false_positive_on_uid_prefix() {
1398+
assert!(!text_mentions_uid("<@U123BOT> hello", "U123"));
1399+
}
1400+
1401+
#[test]
1402+
fn mentions_uid_second_mention_matches() {
1403+
assert!(text_mentions_uid("<@U999OTHER> and <@U123BOT>", "U123BOT"));
1404+
}
1405+
1406+
#[test]
1407+
fn mentions_uid_empty_label_form() {
1408+
assert!(text_mentions_uid("<@U123BOT|> hello", "U123BOT"));
1409+
}
1410+
1411+
#[test]
1412+
fn mentions_uid_truncated_no_closing_delimiter() {
1413+
assert!(!text_mentions_uid("<@U123BOT", "U123BOT"));
1414+
}
1415+
12941416
// --- is_plain_user_message tests (regression for openabdev/openab#497 parity) ---
12951417

12961418
/// Empty message text never counts as a user message (regardless of subtype).

0 commit comments

Comments
 (0)