From d4b20f5295e077f05236aca7556ebc794baade14 Mon Sep 17 00:00:00 2001 From: scoootscooob Date: Mon, 2 Mar 2026 09:11:17 -0800 Subject: [PATCH] fix(slack): scope debounce key by message timestamp to prevent cross-thread collisions Top-level channel messages from the same sender shared a bare channel debounce key, causing concurrent messages in different threads to merge into a single reply on the wrong thread. Now the debounce key includes the message timestamp for top-level messages, matching how the downstream session layer already scopes by canonicalThreadId. Extracted buildSlackDebounceKey() for testability. Closes #31935 Co-Authored-By: Claude Opus 4.6 --- .../message-handler.debounce-key.test.ts | 62 +++++++++++++++++++ src/slack/monitor/message-handler.ts | 40 +++++++----- 2 files changed, 88 insertions(+), 14 deletions(-) create mode 100644 src/slack/monitor/message-handler.debounce-key.test.ts diff --git a/src/slack/monitor/message-handler.debounce-key.test.ts b/src/slack/monitor/message-handler.debounce-key.test.ts new file mode 100644 index 000000000..5b415fd73 --- /dev/null +++ b/src/slack/monitor/message-handler.debounce-key.test.ts @@ -0,0 +1,62 @@ +import { describe, expect, it } from "vitest"; +import type { SlackMessageEvent } from "../types.js"; +import { buildSlackDebounceKey } from "./message-handler.js"; + +function makeMessage(overrides: Partial = {}): SlackMessageEvent { + return { + type: "message", + channel: "C123", + user: "U456", + ts: "1709000000.000100", + text: "hello", + ...overrides, + } as SlackMessageEvent; +} + +describe("buildSlackDebounceKey", () => { + const accountId = "default"; + + it("returns null when message has no sender", () => { + const msg = makeMessage({ user: undefined, bot_id: undefined }); + expect(buildSlackDebounceKey(msg, accountId)).toBeNull(); + }); + + it("scopes thread replies by thread_ts", () => { + const msg = makeMessage({ thread_ts: "1709000000.000001" }); + expect(buildSlackDebounceKey(msg, accountId)).toBe("slack:default:C123:1709000000.000001:U456"); + }); + + it("isolates unresolved thread replies with maybe-thread prefix", () => { + const msg = makeMessage({ + parent_user_id: "U789", + thread_ts: undefined, + ts: "1709000000.000200", + }); + expect(buildSlackDebounceKey(msg, accountId)).toBe( + "slack:default:C123:maybe-thread:1709000000.000200:U456", + ); + }); + + it("scopes top-level messages by their own timestamp to prevent cross-thread collisions", () => { + const msgA = makeMessage({ ts: "1709000000.000100" }); + const msgB = makeMessage({ ts: "1709000000.000200" }); + + const keyA = buildSlackDebounceKey(msgA, accountId); + const keyB = buildSlackDebounceKey(msgB, accountId); + + // Different timestamps => different debounce keys + expect(keyA).not.toBe(keyB); + expect(keyA).toBe("slack:default:C123:1709000000.000100:U456"); + expect(keyB).toBe("slack:default:C123:1709000000.000200:U456"); + }); + + it("falls back to bare channel when no timestamp is available", () => { + const msg = makeMessage({ ts: undefined, event_ts: undefined }); + expect(buildSlackDebounceKey(msg, accountId)).toBe("slack:default:C123:U456"); + }); + + it("uses bot_id as sender fallback", () => { + const msg = makeMessage({ user: undefined, bot_id: "B999" }); + expect(buildSlackDebounceKey(msg, accountId)).toBe("slack:default:C123:1709000000.000100:B999"); + }); +}); diff --git a/src/slack/monitor/message-handler.ts b/src/slack/monitor/message-handler.ts index e763bfb0c..2c884803f 100644 --- a/src/slack/monitor/message-handler.ts +++ b/src/slack/monitor/message-handler.ts @@ -16,6 +16,31 @@ export type SlackMessageHandler = ( opts: { source: "message" | "app_mention"; wasMentioned?: boolean }, ) => Promise; +/** + * Build a debounce key that isolates messages by thread (or by message timestamp + * for top-level channel messages). Without per-message scoping, concurrent + * top-level messages from the same sender would share a key and get merged + * into a single reply on the wrong thread. + */ +export function buildSlackDebounceKey( + message: SlackMessageEvent, + accountId: string, +): string | null { + const senderId = message.user ?? message.bot_id; + if (!senderId) { + return null; + } + const messageTs = message.ts ?? message.event_ts; + const threadKey = message.thread_ts + ? `${message.channel}:${message.thread_ts}` + : message.parent_user_id && messageTs + ? `${message.channel}:maybe-thread:${messageTs}` + : messageTs + ? `${message.channel}:${messageTs}` + : message.channel; + return `slack:${accountId}:${threadKey}:${senderId}`; +} + export function createSlackMessageHandler(params: { ctx: SlackMonitorContext; account: ResolvedSlackAccount; @@ -31,20 +56,7 @@ export function createSlackMessageHandler(params: { opts: { source: "message" | "app_mention"; wasMentioned?: boolean }; }>({ debounceMs, - buildKey: (entry) => { - const senderId = entry.message.user ?? entry.message.bot_id; - if (!senderId) { - return null; - } - const messageTs = entry.message.ts ?? entry.message.event_ts; - // If Slack flags a thread reply but omits thread_ts, isolate it from root debouncing. - const threadKey = entry.message.thread_ts - ? `${entry.message.channel}:${entry.message.thread_ts}` - : entry.message.parent_user_id && messageTs - ? `${entry.message.channel}:maybe-thread:${messageTs}` - : entry.message.channel; - return `slack:${ctx.accountId}:${threadKey}:${senderId}`; - }, + buildKey: (entry) => buildSlackDebounceKey(entry.message, ctx.accountId), shouldDebounce: (entry) => { const text = entry.message.text ?? ""; if (!text.trim()) {