diff --git a/src/slack/events.ts b/src/slack/events.ts index 48e64172b..49878c4a1 100644 --- a/src/slack/events.ts +++ b/src/slack/events.ts @@ -163,7 +163,7 @@ export function registerSlackEvents( if (m.channel_type === "channel" || m.channel_type === "group" || m.channel_type === "mpim") { if (m.channel_type === "mpim" && m.channel) syncForUnseenGroup(client, String(m.channel)); const threadReply = isThreadReply(m); - const isMention = mentionsBot(m.text ?? "", ids.botUserId); + const isMention = mentionsBot(m.text ?? "", ids.botUserId, ids.ownBotId); const willDispatch = threadReply && !isMention && (await botHasStakeInThread(client, m.channel, m.thread_ts)); await mirrorMessageEvent(m, client, willDispatch ? { handled: true } : {}); if (!threadReply) return; diff --git a/src/slack/message-gating.ts b/src/slack/message-gating.ts index fd175b6a5..a7a7b5e52 100644 --- a/src/slack/message-gating.ts +++ b/src/slack/message-gating.ts @@ -1,7 +1,15 @@ import { LRUCache } from "lru-cache"; -export function mentionsBot(text: string, botUserId: string): boolean { - return botUserId ? text.includes(`<@${botUserId}>`) : false; +export function mentionsBot(text: string, botUserId: string, ownBotId = ""): boolean { + // Both id forms mention the bot: the bot USER id (`<@U…>`, the form + // Slack's autocomplete inserts) and the BOT id (`<@B…>`, which + // hand-typed mentions and some clients encode). A B-form mention is a + // legitimate mention of this bot — `bots.info` maps it to the same + // single-bot app — but Slack fires no `app_mention` for it, so without + // recognizing it here the message is dropped at top level and + // misdispatched as ambient in threads (#630). + if (botUserId && text.includes(`<@${botUserId}>`)) return true; + return Boolean(ownBotId) && text.includes(`<@${ownBotId}>`); } export function threadHasBotStake( @@ -17,7 +25,7 @@ export function threadHasBotStake( return Boolean( (botUserId && user === botUserId) || (ownBotId && bot === ownBotId) || - (botUserId && mentionsBot(text, botUserId)), + mentionsBot(text, botUserId, ownBotId), ); }); } diff --git a/src/slack/mirror.ts b/src/slack/mirror.ts index 7936d0ae4..52f0e9ebe 100644 --- a/src/slack/mirror.ts +++ b/src/slack/mirror.ts @@ -117,7 +117,7 @@ export function createMirror(deps: { text, ...(Object.keys(mentions).length ? { mentions } : {}), ...(m.bot_id || m.bot_profile ? { bot: true } : {}), - ...(mentionsBot(raw, ids.botUserId) ? { mentionsSelf: true } : {}), + ...(mentionsBot(raw, ids.botUserId, ids.ownBotId) ? { mentionsSelf: true } : {}), ...(opts.editedAt ? { editedAt: opts.editedAt } : {}), ...(opts.handled ? { handled: true } : {}), ...(opts.containerName ? { containerName: opts.containerName } : {}), diff --git a/src/slack/mrkdwn.ts b/src/slack/mrkdwn.ts index 155ba5977..5a0786c6f 100644 --- a/src/slack/mrkdwn.ts +++ b/src/slack/mrkdwn.ts @@ -10,8 +10,15 @@ export function resolveMentionsInText(text: string, lookup: (id: string) => stri }); } -export function stripMention(text: string, botUserId: string): string { - const withoutMention = botUserId ? text.replace(new RegExp(`<@${botUserId}>`, "g"), "") : text; +export function stripMention(text: string, botUserId: string, ownBotId = ""): string { + // Strip BOTH mention forms (bot user id and bot id — see + // mentionsBot) including the legacy `<@ID|label>` shape, so the core + // never receives a raw `<@B…>` token for an addressed turn (#630). + let withoutMention = text; + for (const id of [botUserId, ownBotId]) { + if (!id) continue; + withoutMention = withoutMention.replace(new RegExp(`<@${id}(?:\|[^>]*)?>`, "g"), ""); + } return decodeSlackEntities(withoutMention).trim(); } diff --git a/src/slack/turn-handler.ts b/src/slack/turn-handler.ts index 8e8e64659..5b711cc72 100644 --- a/src/slack/turn-handler.ts +++ b/src/slack/turn-handler.ts @@ -202,7 +202,7 @@ export function createTurnHandler(deps: { else classified = await classifyUserCached(client, inc.userId); const actor = classified.actor; const timezone = classified.timezone; - const text = stripMention(inc.rawText, ids.botUserId); + const text = stripMention(inc.rawText, ids.botUserId, ids.ownBotId); if (!hasContent(text, inc.files)) return; let audience: ActorAssertion[] = [actor]; diff --git a/test/slack-message-gating.test.ts b/test/slack-message-gating.test.ts index 8f6e1512b..8c1cfcbeb 100644 --- a/test/slack-message-gating.test.ts +++ b/test/slack-message-gating.test.ts @@ -90,6 +90,19 @@ test("dmThreadRef gives the DM main pane one continuous lane and each thread its assert.notEqual(dmThreadRef("D1", "1699999999.000100"), dmThreadRef("D1")); }); +test("mentionsBot recognizes the bot-id form (<@B…>) alongside the user-id form (#630)", () => { + // A hand-typed mention can arrive encoded against the bot id; Slack fires + // no app_mention for it, so message-gating recognition is the only thing + // that can dispatch it. + assert.equal(mentionsBot("hey <@BOTID> do this", "UBOT", "BOTID"), true); + assert.equal(mentionsBot("<@UBOT> classic form", "UBOT", "BOTID"), true); + assert.equal(mentionsBot("both <@UBOT> <@BOTID>", "UBOT", "BOTID"), true); + assert.equal(mentionsBot("<@OTHERBOT> not me", "UBOT", "BOTID"), false); + assert.equal(mentionsBot("no ids configured", "", ""), false); + // Back-compat: the two-arg form still works. + assert.equal(mentionsBot("hey <@BOT> do this", "BOT"), true); +}); + test("mentionsBot detects the bot @mention so thread-follow leaves those to app_mention", () => { assert.equal(mentionsBot("hey <@BOT> do this", "BOT"), true); assert.equal(mentionsBot("just a follow-up", "BOT"), false); diff --git a/test/slack-mrkdwn.test.ts b/test/slack-mrkdwn.test.ts index 1b7226a39..06ec7c7d1 100644 --- a/test/slack-mrkdwn.test.ts +++ b/test/slack-mrkdwn.test.ts @@ -24,6 +24,15 @@ test("decodeSlackEntities / stripMention", () => { assert.equal(stripMention("<@BOT>", "BOT"), ""); }); +test("stripMention strips the bot-id form and the legacy label shape (#630)", () => { + // The core must not receive a raw <@B…> token for an addressed turn. + assert.equal(stripMention("<@BOTID> run the tests", "UBOT", "BOTID"), "run the tests"); + assert.equal(stripMention("<@BOTID|qm> run the tests", "UBOT", "BOTID"), "run the tests"); + assert.equal(stripMention("<@UBOT> classic", "UBOT", "BOTID"), "classic"); + assert.equal(stripMention("<@UBOT|qm> legacy label", "UBOT", "BOTID"), "legacy label"); + assert.equal(stripMention("unrelated <@OTHER> stays", "UBOT", "BOTID"), "unrelated <@OTHER> stays"); +}); + test("toSlackMrkdwn: bold uses single * (the screenshot bug: **x** rendered literally)", () => { assert.equal(toSlackMrkdwn("**Git commands**"), "*Git commands*"); assert.equal(toSlackMrkdwn("__also bold__"), "*also bold*");