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>
This commit is contained in:
Peter Steinberger
2026-07-29 00:23:06 -04:00
committed by GitHub
parent a6d58f0e6d
commit dd5b9c8c04
10 changed files with 364 additions and 90 deletions
+2
View File
@@ -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.
@@ -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) =>
@@ -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
? {
@@ -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<string | void>;
cleanup?: (context: Omit<SlackQaScenarioContext, "sentTs">) => Promise<void>;
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;
@@ -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
@@ -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<SlackQaScenarioContext, "sentTs">) => {
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",
@@ -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([]);
});
+41 -19
View File
@@ -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<SlackMessageEvent["channel_type"] | undefined> {
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<string>, 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
@@ -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<string | number>;
@@ -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 }]