From dd5b9c8c042b183c96e705a1c4a8b1d224a2a2d1 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 29 Jul 2026 00:23:06 -0400 Subject: [PATCH] fix(slack): prevent duplicate MPIM mention replies (#115528) * fix(slack): make MPIM app mentions type-safe Resolve typeless app mentions through explicit metadata, the scoped event cache, then conversations.info so modern C-prefixed MPIMs cannot be misclassified by event ordering. Add a real Slack QA scenario that requires one reply and cleans up its temporary MPIM. Co-authored-by: moeealii <75953662+moeealii@users.noreply.github.com> * fix(slack): type unresolved mention metadata * fix(slack): satisfy MPIM type contracts --------- Co-authored-by: moeealii <75953662+moeealii@users.noreply.github.com> --- docs/concepts/qa-e2e-automation.md | 2 + .../live-transports/slack/scenario-runtime.ts | 142 ++++++++++-------- .../slack/slack-live.config.ts | 3 + .../slack/slack-live.contracts.ts | 4 + .../slack/slack-live.runtime.test.ts | 67 +++++++++ .../slack/slack-live.scenarios.ts | 88 +++++++++++ .../slack/src/monitor/events/messages.test.ts | 66 +++++++- .../slack/src/monitor/events/messages.ts | 60 +++++--- .../events/system-event-test-harness.ts | 2 +- .../slack-mpim-app-mention-dedupe.yaml | 20 +++ 10 files changed, 364 insertions(+), 90 deletions(-) create mode 100644 qa/scenarios/channels/slack-mpim-app-mention-dedupe.yaml diff --git a/docs/concepts/qa-e2e-automation.md b/docs/concepts/qa-e2e-automation.md index 045bd0f4c41b..40d005fd494b 100644 --- a/docs/concepts/qa-e2e-automation.md +++ b/docs/concepts/qa-e2e-automation.md @@ -660,6 +660,8 @@ Slack YAML module scenarios (`qa/scenarios/channels/slack-*.yaml`): - `slack-canary` - `slack-mention-gating` +- `slack-mpim-app-mention-dedupe` - opens a real C-prefixed group DM, sends one + mention, verifies exactly one SUT reply in that MPIM, then closes it. - `slack-allowlist-block` - `slack-channel-disabled-warning` - opt-in real-Slack probe that confirms a configured disabled channel emits a structured warning without replying. diff --git a/extensions/qa-lab/src/live-transports/slack/scenario-runtime.ts b/extensions/qa-lab/src/live-transports/slack/scenario-runtime.ts index 088532ad3f3e..34c8b14f278e 100644 --- a/extensions/qa-lab/src/live-transports/slack/scenario-runtime.ts +++ b/extensions/qa-lab/src/live-transports/slack/scenario-runtime.ts @@ -21,22 +21,48 @@ async function runSlackMessageScenario(params: { scenarioTitle: string; timeoutMs: number; }) { - const beforeRunResult = await params.run.beforeRun?.(params.environment.context); - const beforeRunDetails = - typeof beforeRunResult === "string" ? beforeRunResult : beforeRunResult?.details; - const observedMessageStartIndex = params.environment.observedMessages.length; - const requestStartedAt = new Date(); - const sent = await sendSlackChannelMessage({ - channelId: params.environment.channelId, - client: params.environment.context.driverClient, - text: params.run.input, - threadTs: typeof beforeRunResult === "object" ? beforeRunResult?.inputThreadTs : undefined, - }); - const requestThreadTs = - (typeof beforeRunResult === "object" ? beforeRunResult?.inputThreadTs : undefined) ?? sent.ts; - if (!params.run.expectReply) { - await waitForSlackNoReply({ - channelId: params.environment.channelId, + let scenarioContext = params.environment.context; + try { + const beforeRunResult = await params.run.beforeRun?.(params.environment.context); + const beforeRunDetails = + typeof beforeRunResult === "string" ? beforeRunResult : beforeRunResult?.details; + const channelId = + typeof beforeRunResult === "object" && beforeRunResult.inputChannelId?.trim() + ? beforeRunResult.inputChannelId.trim() + : params.environment.channelId; + scenarioContext = { ...params.environment.context, channelId }; + const observedMessageStartIndex = params.environment.observedMessages.length; + const requestStartedAt = new Date(); + const sent = await sendSlackChannelMessage({ + channelId, + client: params.environment.context.driverClient, + text: params.run.input, + threadTs: typeof beforeRunResult === "object" ? beforeRunResult?.inputThreadTs : undefined, + }); + const requestThreadTs = + (typeof beforeRunResult === "object" ? beforeRunResult?.inputThreadTs : undefined) ?? sent.ts; + if (!params.run.expectReply) { + await waitForSlackNoReply({ + channelId, + client: params.environment.context.sutReadClient, + matchText: params.run.matchText, + observedMessages: params.environment.observedMessages, + observationScenarioId: params.scenarioId, + observationScenarioTitle: params.scenarioTitle, + sentTs: sent.ts, + sutIdentity: params.environment.sutIdentity, + timeoutMs: params.timeoutMs, + }); + const afterNoReplyDetails = await params.run.afterNoReply?.({ + ...scenarioContext, + sentTs: sent.ts, + }); + return { + details: ["no reply", beforeRunDetails, afterNoReplyDetails].filter(Boolean).join("; "), + }; + } + const reply = await waitForSlackScenarioReply({ + channelId, client: params.environment.context.sutReadClient, matchText: params.run.matchText, observedMessages: params.environment.observedMessages, @@ -44,58 +70,42 @@ async function runSlackMessageScenario(params: { observationScenarioTitle: params.scenarioTitle, sentTs: sent.ts, sutIdentity: params.environment.sutIdentity, - timeoutMs: params.timeoutMs, - }); - const afterNoReplyDetails = await params.run.afterNoReply?.({ - ...params.environment.context, - sentTs: sent.ts, - }); - return { - details: ["no reply", beforeRunDetails, afterNoReplyDetails].filter(Boolean).join("; "), - }; - } - const reply = await waitForSlackScenarioReply({ - channelId: params.environment.channelId, - client: params.environment.context.sutReadClient, - matchText: params.run.matchText, - observedMessages: params.environment.observedMessages, - observationScenarioId: params.scenarioId, - observationScenarioTitle: params.scenarioTitle, - sentTs: sent.ts, - threadTs: requestThreadTs, - sutIdentity: params.environment.sutIdentity, - timeoutMs: params.timeoutMs, - }); - params.run.verify?.(reply.message, { requestThreadTs, sentTs: sent.ts }); - if (params.run.settleObservedMs) { - await observeSlackScenarioMessages({ - channelId: params.environment.channelId, - client: params.environment.context.sutReadClient, - matchText: params.run.matchText, - observedMessages: params.environment.observedMessages, - observationScenarioId: params.scenarioId, - observationScenarioTitle: params.scenarioTitle, - sentTs: sent.ts, - settleMs: params.run.settleObservedMs, - sutIdentity: params.environment.sutIdentity, threadTs: requestThreadTs, + timeoutMs: params.timeoutMs, }); + params.run.verify?.(reply.message, { requestThreadTs, sentTs: sent.ts }); + if (params.run.settleObservedMs) { + await observeSlackScenarioMessages({ + channelId, + client: params.environment.context.sutReadClient, + matchText: params.run.matchText, + observedMessages: params.environment.observedMessages, + observationScenarioId: params.scenarioId, + observationScenarioTitle: params.scenarioTitle, + sentTs: sent.ts, + settleMs: params.run.settleObservedMs, + sutIdentity: params.environment.sutIdentity, + threadTs: requestThreadTs, + }); + } + const observedDetails = params.run.verifyObserved?.({ + finalMessage: reply.message, + messages: params.environment.observedMessages.slice(observedMessageStartIndex), + }); + const afterReplyDetails = await params.run.afterReply?.(reply.message, { + ...scenarioContext, + sentTs: sent.ts, + }); + const responseObservedAt = new Date(reply.observedAt); + const rttMs = responseObservedAt.getTime() - requestStartedAt.getTime(); + return { + details: [`reply matched in ${rttMs}ms`, beforeRunDetails, observedDetails, afterReplyDetails] + .filter(Boolean) + .join("; "), + }; + } finally { + await params.run.cleanup?.(scenarioContext); } - const observedDetails = params.run.verifyObserved?.({ - finalMessage: reply.message, - messages: params.environment.observedMessages.slice(observedMessageStartIndex), - }); - const afterReplyDetails = await params.run.afterReply?.(reply.message, { - ...params.environment.context, - sentTs: sent.ts, - }); - const responseObservedAt = new Date(reply.observedAt); - const rttMs = responseObservedAt.getTime() - requestStartedAt.getTime(); - return { - details: [`reply matched in ${rttMs}ms`, beforeRunDetails, observedDetails, afterReplyDetails] - .filter(Boolean) - .join("; "), - }; } async function runSlackScenario(environment: SlackQaScenarioEnvironment, scenarioId: string) { @@ -173,6 +183,8 @@ export const runSlackCanaryScenario = (context: SlackQaScenarioEnvironment) => runSlackScenario(context, "slack-canary"); export const runSlackMentionGatingScenario = (context: SlackQaScenarioEnvironment) => runSlackScenario(context, "slack-mention-gating"); +export const runSlackMpimAppMentionDedupeScenario = (context: SlackQaScenarioEnvironment) => + runSlackScenario(context, "slack-mpim-app-mention-dedupe"); export const runSlackAllowlistBlockScenario = (context: SlackQaScenarioEnvironment) => runSlackScenario(context, "slack-allowlist-block"); export const runSlackChannelDisabledWarningScenario = (context: SlackQaScenarioEnvironment) => diff --git a/extensions/qa-lab/src/live-transports/slack/slack-live.config.ts b/extensions/qa-lab/src/live-transports/slack/slack-live.config.ts index 191eb7897b1d..0567921994d1 100644 --- a/extensions/qa-lab/src/live-transports/slack/slack-live.config.ts +++ b/extensions/qa-lab/src/live-transports/slack/slack-live.config.ts @@ -282,6 +282,9 @@ export function buildSlackQaConfig( allowFrom: params.overrides?.allowFrom ?? [params.driverBotUserId], groupPolicy: "allowlist", allowBots: true, + ...(params.overrides?.groupDmEnabled + ? { dm: { enabled: true, groupEnabled: true } } + : {}), replyToMode: params.overrides?.replyToMode ?? "off", ...(progressOverrides ? { diff --git a/extensions/qa-lab/src/live-transports/slack/slack-live.contracts.ts b/extensions/qa-lab/src/live-transports/slack/slack-live.contracts.ts index c3f4124e8536..2e7cd23b02c1 100644 --- a/extensions/qa-lab/src/live-transports/slack/slack-live.contracts.ts +++ b/extensions/qa-lab/src/live-transports/slack/slack-live.contracts.ts @@ -95,6 +95,7 @@ export type SlackQaScenarioId = | "slack-chart-presentation-native" | "slack-channel-disabled-warning" | "slack-mention-gating" + | "slack-mpim-app-mention-dedupe" | "slack-progress-commentary-false" | "slack-progress-commentary-omitted" | "slack-progress-commentary-true" @@ -130,6 +131,7 @@ export function assertSlackCodexApprovalModelSupported(modelRef: string) { export type SlackQaMessageScenarioRun = { afterNoReply?: (context: SlackQaScenarioContext) => Promise; + cleanup?: (context: Omit) => Promise; kind?: "message"; expectReply: boolean; input: string; @@ -193,12 +195,14 @@ type SlackQaBeforeRunResult = | void | { details?: string; + inputChannelId?: string; inputThreadTs?: string; }; export type SlackQaConfigOverrides = { allowFrom?: string[]; channelEnabled?: boolean; + groupDmEnabled?: boolean; approvals?: { exec?: boolean; plugin?: boolean; diff --git a/extensions/qa-lab/src/live-transports/slack/slack-live.runtime.test.ts b/extensions/qa-lab/src/live-transports/slack/slack-live.runtime.test.ts index c5c47a864652..7e1a0276bc6a 100644 --- a/extensions/qa-lab/src/live-transports/slack/slack-live.runtime.test.ts +++ b/extensions/qa-lab/src/live-transports/slack/slack-live.runtime.test.ts @@ -139,6 +139,73 @@ describe("Slack live QA runtime helpers", () => { ]); }); + it("selects the MPIM app-mention dedupe scenario", () => { + expect( + testing.findScenario(["slack-mpim-app-mention-dedupe"]).map((scenario) => scenario.id), + ).toEqual(["slack-mpim-app-mention-dedupe"]); + }); + + it("enables group DMs for the MPIM app-mention scenario", () => { + const cfg = testing.buildSlackQaConfig( + {}, + { + channelId: "C123456789", + driverBotUserId: "U999999999", + overrides: { groupDmEnabled: true }, + sutAccountId: "sut", + sutAppToken: "xapp-sut", + sutBotToken: "xoxb-sut", + }, + ); + + expect(cfg.channels?.slack?.accounts?.sut?.dm).toEqual({ + enabled: true, + groupEnabled: true, + }); + }); + + it("surfaces MPIM cleanup failures and retains ownership for a retry", async () => { + const run = testing.findScenario(["slack-mpim-app-mention-dedupe"])[0]?.buildRun("U_SUT"); + if ( + !run || + run.kind === "approval" || + run.kind === "codex-approval" || + run.kind === "direct-transport" + ) { + throw new Error("expected Slack MPIM message scenario"); + } + const close = vi + .fn() + .mockRejectedValueOnce(new Error("close failed")) + .mockRejectedValueOnce(new Error("close failed again")) + .mockResolvedValueOnce({}); + const context = { + channelId: "C_QA", + driverClient: { auth: { test: vi.fn(async () => ({ user_id: "U_DRIVER" })) } }, + sutIdentity: { userId: "U_SUT" }, + sutReadClient: { + conversations: { + close, + info: vi.fn(async () => { + throw new Error("metadata unavailable"); + }), + members: vi.fn(async () => ({ members: ["U_DRIVER", "U_SUT", "U_HUMAN"] })), + open: vi.fn(async () => ({ channel: { id: "C_MPIM" } })), + }, + users: { info: vi.fn(async () => ({ user: { id: "U_HUMAN" } })) }, + }, + } as never; + + await expect(run.beforeRun?.(context)).rejects.toThrow("metadata unavailable"); + await expect(run.cleanup?.(context)).rejects.toThrow("close failed again"); + await expect(run.cleanup?.(context)).resolves.toBeUndefined(); + + expect(close).toHaveBeenCalledTimes(3); + expect(close).toHaveBeenNthCalledWith(1, { channel: "C_MPIM" }); + expect(close).toHaveBeenNthCalledWith(2, { channel: "C_MPIM" }); + expect(close).toHaveBeenNthCalledWith(3, { channel: "C_MPIM" }); + }); + it("selects native scenarios by explicit id", () => { expect( testing diff --git a/extensions/qa-lab/src/live-transports/slack/slack-live.scenarios.ts b/extensions/qa-lab/src/live-transports/slack/slack-live.scenarios.ts index 2384489030d1..9932f84f2bb6 100644 --- a/extensions/qa-lab/src/live-transports/slack/slack-live.scenarios.ts +++ b/extensions/qa-lab/src/live-transports/slack/slack-live.scenarios.ts @@ -1,11 +1,13 @@ // QA Lab Slack live scenario catalog. import { randomUUID } from "node:crypto"; +import { setTimeout as sleep } from "node:timers/promises"; import { waitForSlackReaction } from "./slack-live.codex-approval.js"; import { SLACK_QA_REACTION_VERIFY_TIMEOUT_MS, SLACK_QA_NATIVE_DATA_VERIFY_TIMEOUT_MS, SLACK_QA_LOG_TAIL_TIMEOUT_MS, type SlackQaScenarioDefinition, + type SlackQaScenarioContext, } from "./slack-live.contracts.js"; import { isExpectedSlackNativeChartMessage, @@ -48,6 +50,92 @@ const SLACK_QA_SCENARIOS: SlackQaScenarioDefinition[] = [ }; }, }, + { + id: "slack-mpim-app-mention-dedupe", + title: "Slack MPIM app mention dispatches once", + timeoutMs: 90_000, + configOverrides: { groupDmEnabled: true }, + buildRun: (sutUserId) => { + const token = `SLACK_QA_MPIM_${randomUUID().slice(0, 8).toUpperCase()}`; + let openedChannelId: string | undefined; + const closeOpenedChannel = async (context: Omit) => { + if (!openedChannelId) { + return; + } + const channelId = openedChannelId; + for (let attempt = 1; attempt <= 2; attempt += 1) { + try { + await context.sutReadClient.conversations.close({ channel: channelId }); + openedChannelId = undefined; + return; + } catch (error) { + if (attempt === 2) { + throw error; + } + // Retain ownership until Slack confirms closure; one bounded retry + // covers a transient API failure without hiding a leaked MPIM. + await sleep(500); + } + } + }; + return { + expectReply: true, + input: `<@${sutUserId}> reply with only this exact marker: ${token}`, + matchText: token, + settleObservedMs: 60_000, + beforeRun: async (context) => { + const driverAuth = await context.driverClient.auth.test(); + const driverUserId = driverAuth.user_id?.trim(); + if (!driverUserId) { + throw new Error("Slack QA driver auth.test returned no user_id"); + } + const members = await context.sutReadClient.conversations.members({ + channel: context.channelId, + limit: 100, + }); + const candidateUserIds = (members.members ?? []).filter( + (userId) => userId !== driverUserId && userId !== context.sutIdentity.userId, + ); + for (const userId of candidateUserIds) { + const user = (await context.sutReadClient.users.info({ user: userId })).user; + if (!user || user.deleted || user.is_bot) { + continue; + } + const opened = await context.sutReadClient.conversations.open({ + return_im: true, + users: `${driverUserId},${userId}`, + }); + const channelId = opened.channel?.id?.trim(); + if (!channelId) { + continue; + } + // Track ownership before the metadata call so outer cleanup can still + // close the MPIM when Slack rejects or times out during inspection. + openedChannelId = channelId; + const info = await context.sutReadClient.conversations.info({ channel: channelId }); + if (info.channel?.is_mpim && channelId.startsWith("C")) { + return { details: "opened C-prefixed MPIM", inputChannelId: channelId }; + } + await closeOpenedChannel(context); + } + throw new Error("Slack QA channel has no human member yielding a C-prefixed MPIM"); + }, + verifyObserved: ({ messages }) => { + const uniqueReplies = new Map(messages.map((message) => [message.ts, message])); + const matchingReplies = [...uniqueReplies.values()].filter((message) => + message.text.includes(token), + ); + if (uniqueReplies.size !== 1 || matchingReplies.length !== 1) { + throw new Error( + `expected one MPIM response with the marker, got ${uniqueReplies.size} response(s) and ${matchingReplies.length} marker match(es)`, + ); + } + return "one MPIM reply observed after message/app_mention twin delivery"; + }, + cleanup: closeOpenedChannel, + }; + }, + }, { id: "slack-allowlist-block", title: "Slack non-allowlisted sender does not trigger", diff --git a/extensions/slack/src/monitor/events/messages.test.ts b/extensions/slack/src/monitor/events/messages.test.ts index 82d1ecebc076..d33d00ce03d7 100644 --- a/extensions/slack/src/monitor/events/messages.test.ts +++ b/extensions/slack/src/monitor/events/messages.test.ts @@ -87,6 +87,7 @@ function createHandlers(eventName: RegisteredEventName, overrides?: SlackSystemE handleSlackMessage, }); return { + ctx: harness.ctx, handler: harness.getHandler(eventName) as MessageHandler | null, handleSlackMessage, }; @@ -718,15 +719,70 @@ describe("registerSlackMessageEvents", () => { expect(inboundLogLines()).toEqual([]); }); - it("skips app_mention events for modern C-prefixed group DMs without channel_type", async () => { - const { handleSlackMessage } = await invokeRegisteredHandler({ - eventName: "app_mention", - overrides: { dmPolicy: "open", channelType: "mpim" }, + it.each(["C0MPDM42", "G0MPDM42"])( + "skips typeless app_mention events for metadata-resolved MPIM %s", + async (channel) => { + const { handleSlackMessage } = await invokeRegisteredHandler({ + eventName: "app_mention", + overrides: { dmPolicy: "open", channelType: "mpim" }, + event: { ...makeAppMentionEvent({ channel }), channel_type: undefined }, + }); + + expect(handleSlackMessage).not.toHaveBeenCalled(); + // Handled via message.mpim; must not log a duplicate receipt. + expect(inboundLogLines()).toEqual([]); + }, + ); + + it("uses a remembered MPIM type without loading channel metadata", async () => { + const { ctx, handler, handleSlackMessage } = createHandlers("app_mention", { + dmPolicy: "open", + }); + const resolveChannelName = vi.fn(async () => ({ type: "channel" as const })); + ctx.recallSlackChannelType = () => "mpim"; + ctx.resolveChannelName = resolveChannelName; + + await requireMessageHandler(handler)({ event: { ...makeAppMentionEvent({ channel: "C0MPDM42" }), channel_type: undefined }, + body: {}, + }); + + expect(handleSlackMessage).not.toHaveBeenCalled(); + expect(resolveChannelName).not.toHaveBeenCalled(); + expect(inboundLogLines()).toEqual([]); + }); + + it.each([ + { channel: "C123", resolvedType: "channel" as const }, + { channel: "G123", resolvedType: "group" as const }, + ])("routes typeless app_mention after resolving $resolvedType metadata", async (testCase) => { + const { handleSlackMessage } = await invokeRegisteredHandler({ + eventName: "app_mention", + overrides: { dmPolicy: "open", channelType: testCase.resolvedType }, + event: { ...makeAppMentionEvent({ channel: testCase.channel }), channel_type: undefined }, + }); + + expect(handleSlackMessage).toHaveBeenCalledOnce(); + expect(handleSlackMessage).toHaveBeenCalledWith( + expect.objectContaining({ channel: testCase.channel }), + expect.objectContaining({ source: "app_mention", wasMentioned: true }), + ); + }); + + it("drops typeless app_mention when metadata lookup fails", async () => { + const { ctx, handler, handleSlackMessage } = createHandlers("app_mention", { + dmPolicy: "open", + }); + ctx.resolveChannelName = vi.fn(async () => { + throw new Error("missing_scope"); + }); + + await requireMessageHandler(handler)({ + event: { ...makeAppMentionEvent({ channel: "C123" }), channel_type: undefined }, + body: {}, }); expect(handleSlackMessage).not.toHaveBeenCalled(); - // Handled via message.mpim; must not log a duplicate receipt. expect(inboundLogLines()).toEqual([]); }); diff --git a/extensions/slack/src/monitor/events/messages.ts b/extensions/slack/src/monitor/events/messages.ts index e45c6682ce08..1c04489c426e 100644 --- a/extensions/slack/src/monitor/events/messages.ts +++ b/extensions/slack/src/monitor/events/messages.ts @@ -59,6 +59,32 @@ function isBotAuthoredEnterpriseEvent(event: { bot_id?: unknown; subtype?: unkno return Boolean(asString(event.bot_id)) || event.subtype === "bot_message"; } +async function resolveSlackAppMentionChannelType(params: { + ctx: SlackMonitorContext; + eventScope?: SlackEventScope; + mention: SlackAppMentionEvent; +}): Promise { + const explicitType = asString(params.mention.channel_type); + if (explicitType) { + return normalizeSlackChannelType(explicitType, params.mention.channel); + } + const rememberedType = params.ctx.recallSlackChannelType( + params.mention.channel, + params.eventScope, + ); + if (rememberedType) { + return normalizeSlackChannelType(rememberedType, params.mention.channel); + } + // app_mention omits channel_type, and Slack ID prefixes are not a type contract. + // Only an authoritative event/cache/API type may choose this event's owner. + const resolved = await params.ctx + .resolveChannelName(params.mention.channel, params.eventScope) + .catch(() => ({ type: undefined })); + return resolved.type + ? normalizeSlackChannelType(resolved.type, params.mention.channel) + : undefined; +} + function addUserCandidate(candidates: Set, value: unknown, botUserId: string): void { const id = asString(value); if (!id || id === botUserId || !isSlackUserId(id)) { @@ -313,27 +339,23 @@ export function registerSlackMessageEvents(params: { return; } - // Skip app_mention for DMs - they're already handled by message.im event - // This prevents duplicate processing when both message and app_mention fire for DMs - const channelType = normalizeSlackChannelType(mention.channel_type, mention.channel); - if (channelType === "im" || channelType === "mpim") { + // DM and MPIM messages are owned by message.im/message.mpim. Resolve the + // omitted type before this guard so event ordering cannot change ownership. + const channelType = await resolveSlackAppMentionChannelType({ + ctx, + mention, + ...(eventScope ? { eventScope } : {}), + }); + if (!channelType) { + // OpenClaw manifests pair app_mention with message.channels/groups/im/mpim. + // Never guess here: the canonical message event still owns delivery. + logVerbose( + `slack: drop typeless app_mention channel=${mention.channel} (conversation type unresolved; waiting for message event)`, + ); return; } - // Modern Slack group DMs (mpims) use C-prefixed channel ids and - // app_mention events carry no channel_type, so the prefix inference - // above reports "channel" for them. Confirm against the event-carried - // type cache (fed by message.mpim) and, on a miss, conversations.info - // before treating this as a channel mention — otherwise group-DM - // mentions run down the channel path and are double-handled alongside - // the paired message.mpim event. - if (channelType === "channel" && !mention.channel_type) { - const scope = eventScope ?? undefined; - const knownType = - ctx.recallSlackChannelType(mention.channel, scope) ?? - (await ctx.resolveChannelName(mention.channel, scope)).type; - if (knownType === "mpim") { - return; - } + if (channelType === "im" || channelType === "mpim") { + return; } // Emit a per-inbound receipt before dispatch so a silently-dropped mention diff --git a/extensions/slack/src/monitor/events/system-event-test-harness.ts b/extensions/slack/src/monitor/events/system-event-test-harness.ts index ae2b0404baae..8c0271177102 100644 --- a/extensions/slack/src/monitor/events/system-event-test-harness.ts +++ b/extensions/slack/src/monitor/events/system-event-test-harness.ts @@ -9,7 +9,7 @@ export type SlackSystemEventHandler = (args: { export type SlackSystemEventTestOverrides = { dmPolicy?: "open" | "pairing" | "allowlist" | "disabled"; allowFrom?: string[]; - channelType?: "im" | "channel" | "mpim"; + channelType?: "im" | "channel" | "group" | "mpim"; channelUsers?: string[]; reactionMode?: "off" | "own" | "all" | "allowlist"; reactionAllowlist?: Array; diff --git a/qa/scenarios/channels/slack-mpim-app-mention-dedupe.yaml b/qa/scenarios/channels/slack-mpim-app-mention-dedupe.yaml new file mode 100644 index 000000000000..59ccdfe55aa2 --- /dev/null +++ b/qa/scenarios/channels/slack-mpim-app-mention-dedupe.yaml @@ -0,0 +1,20 @@ +title: Slack MPIM app mention dispatches once +scenario: + id: slack-mpim-app-mention-dedupe + surface: channels + coverage: + primary: + - slack.socket + - slack.mpim + - slack.delivery-dedupe + execution: + kind: flow + channel: slack + timeoutMs: 180000 + retryCount: 0 + suiteIsolation: isolated + config: { slackScenarioId: slack-mpim-app-mention-dedupe } +flow: + module: ./live-transports/slack/scenario-runtime.js + call: runSlackMpimAppMentionDedupeScenario + args: [{ expr: slackScenarioContext }]