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 <noreply@anthropic.com>
This commit is contained in:
committed by
Peter Steinberger
parent
07eaeb7350
commit
d4b20f5295
62
src/slack/monitor/message-handler.debounce-key.test.ts
Normal file
62
src/slack/monitor/message-handler.debounce-key.test.ts
Normal file
@@ -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> = {}): 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");
|
||||
});
|
||||
});
|
||||
@@ -16,6 +16,31 @@ export type SlackMessageHandler = (
|
||||
opts: { source: "message" | "app_mention"; wasMentioned?: boolean },
|
||||
) => Promise<void>;
|
||||
|
||||
/**
|
||||
* 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()) {
|
||||
|
||||
Reference in New Issue
Block a user