diff --git a/docs/concepts/qa-e2e-automation.md b/docs/concepts/qa-e2e-automation.md index 0b8dac707bac..68deb9d14bf4 100644 --- a/docs/concepts/qa-e2e-automation.md +++ b/docs/concepts/qa-e2e-automation.md @@ -685,8 +685,9 @@ 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-mpim-app-mention-dedupe` - opens a real C-prefixed group DM, verifies + exactly one SUT reply after message/app-mention twin delivery, confirms a + native threaded follow-up can recall that bot reply, then closes the MPIM. - `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/slack-live.runtime.test.ts b/extensions/qa-lab/src/live-transports/slack/slack-live.runtime.test.ts index 59d8254eefbf..108a895e3042 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 @@ -166,13 +166,17 @@ describe("Slack live QA runtime helpers", () => { ).toEqual(["slack-mpim-app-mention-dedupe"]); }); - it("enables group DMs for the MPIM app-mention scenario", () => { + it("enables group DMs and threaded replies for the MPIM app-mention scenario", () => { + const scenario = testing.findScenario(["slack-mpim-app-mention-dedupe"])[0]; + if (!scenario) { + throw new Error("missing Slack MPIM app-mention scenario"); + } const cfg = testing.buildSlackQaConfig( {}, { channelId: "C123456789", driverBotUserId: "U999999999", - overrides: { groupDmEnabled: true }, + overrides: scenario.configOverrides, sutAccountId: "sut", sutAppToken: "xapp-sut", sutBotToken: "xoxb-sut", @@ -183,6 +187,7 @@ describe("Slack live QA runtime helpers", () => { enabled: true, groupEnabled: true, }); + expect(cfg.channels?.slack?.accounts?.sut?.replyToMode).toBe("all"); }); it("surfaces MPIM cleanup failures and retains ownership for a retry", async () => { @@ -227,6 +232,144 @@ describe("Slack live QA runtime helpers", () => { expect(close).toHaveBeenNthCalledWith(3, { channel: "C_MPIM" }); }); + it("keeps the MPIM recall turn in the native thread", 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" || + !run.afterReply + ) { + throw new Error("expected Slack MPIM message scenario with a recall turn"); + } + const seedMarker = /SLACK_QA_MPIM_SEED_[A-Z0-9]+/u.exec(run.input)?.[0]; + if (!seedMarker) { + throw new Error("missing Slack MPIM seed marker"); + } + expect(run.input).toContain( + `Reply with only a marker in this exact format: ${seedMarker}_BOT_.`, + ); + expect(run.input).toContain("Replace with 8 to 32 new uppercase letters or digits."); + const botReplyMarker = `${seedMarker}_BOT_TESTNONCE`; + const recallMarker = seedMarker.replace("SEED", "RECALL"); + const expectedRecallMarker = `${recallMarker}_TESTNONCE`; + const postMessage = vi.fn(async (_request: { text?: string }) => ({ + channel: "C_MPIM", + ts: "2.000000", + })); + const history = vi.fn(async () => ({ messages: [] })); + const replies = vi.fn(async () => ({ + messages: [ + { + bot_id: "B_SUT", + text: expectedRecallMarker, + thread_ts: "1.000000", + ts: "3.000000", + user: "U_SUT", + }, + ], + })); + + await expect( + run.afterReply( + { + text: botReplyMarker, + thread_ts: "1.000000", + ts: "1.500000", + user: "U_SUT", + }, + { + channelId: "C_MPIM", + driverClient: { chat: { postMessage } }, + sentTs: "1.000000", + sutIdentity: { botId: "B_SUT", userId: "U_SUT" }, + sutReadClient: { conversations: { history, replies } }, + } as never, + ), + ).resolves.toContain("recovered the prior bot reply"); + + expect(postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + channel: "C_MPIM", + thread_ts: "1.000000", + }), + ); + const recallText = postMessage.mock.calls[0]?.[0]?.text; + expect(recallText).toContain(`previous reply beginning with ${seedMarker}_BOT_`); + expect(recallText).toContain(`exact format: ${recallMarker}_`); + expect(recallText).not.toContain(botReplyMarker); + expect(recallText).not.toContain("TESTNONCE"); + }); + + it("rejects an MPIM seed reply without text before sending the recall turn", 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" || + !run.afterReply + ) { + throw new Error("expected Slack MPIM message scenario with a recall turn"); + } + const postMessage = vi.fn(); + + await expect( + run.afterReply( + { + thread_ts: "1.000000", + ts: "1.500000", + user: "U_SUT", + }, + { + channelId: "C_MPIM", + driverClient: { chat: { postMessage } }, + sentTs: "1.000000", + sutIdentity: { botId: "B_SUT", userId: "U_SUT" }, + sutReadClient: { conversations: {} }, + } as never, + ), + ).rejects.toThrow("MPIM seed reply did not contain the provider-generated bot nonce"); + expect(postMessage).not.toHaveBeenCalled(); + }); + + it("rejects an MPIM seed reply outside the native thread", 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" || + !run.afterReply + ) { + throw new Error("expected Slack MPIM message scenario with a recall turn"); + } + const seedMarker = /SLACK_QA_MPIM_SEED_[A-Z0-9]+/u.exec(run.input)?.[0]; + if (!seedMarker) { + throw new Error("missing Slack MPIM seed marker"); + } + const postMessage = vi.fn(); + + await expect( + run.afterReply( + { + text: `${seedMarker}_BOT_TESTNONCE`, + ts: "1.500000", + user: "U_SUT", + }, + { + channelId: "C_MPIM", + driverClient: { chat: { postMessage } }, + sentTs: "1.000000", + sutIdentity: { botId: "B_SUT", userId: "U_SUT" }, + sutReadClient: { conversations: {} }, + } as never, + ), + ).rejects.toThrow("MPIM seed reply escaped the native Slack thread"); + expect(postMessage).not.toHaveBeenCalled(); + }); + it("selects native scenarios by explicit id", () => { const scenarioIds = [ "slack-chart-presentation-native", diff --git a/extensions/qa-lab/src/live-transports/slack/slack-live.scenario-implementations.ts b/extensions/qa-lab/src/live-transports/slack/slack-live.scenario-implementations.ts index ed048546576f..f41588978b1b 100644 --- a/extensions/qa-lab/src/live-transports/slack/slack-live.scenario-implementations.ts +++ b/extensions/qa-lab/src/live-transports/slack/slack-live.scenario-implementations.ts @@ -9,10 +9,12 @@ import { type SlackQaScenarioImplementation, type SlackQaScenarioContext, } from "./slack-live.contracts.js"; +import { waitForSlackScenarioReply } from "./slack-live.message-observations.js"; import { isExpectedSlackNativeChartMessage, isExpectedSlackNativeTableMessage, runSlackTableInvalidBlocksFallbackScenario, + sendSlackChannelMessage, waitForSlackStoredMessage, } from "./slack-live.observations.js"; import { @@ -46,9 +48,12 @@ export const slackQaMentionGatingScenario: SlackQaScenarioImplementation = { }; export const slackQaMpimAppMentionDedupeScenario: SlackQaScenarioImplementation = { - configOverrides: { groupDmEnabled: true }, + configOverrides: { groupDmEnabled: true, replyToMode: "all" }, buildRun: (sutUserId) => { - const token = `SLACK_QA_MPIM_${randomUUID().slice(0, 8).toUpperCase()}`; + const suffix = randomUUID().slice(0, 8).toUpperCase(); + const seedMarker = `SLACK_QA_MPIM_SEED_${suffix}`; + const recallMarker = `SLACK_QA_MPIM_RECALL_${suffix}`; + const missingMarker = `SLACK_QA_MPIM_MISSING_${suffix}`; let openedChannelId: string | undefined; const closeOpenedChannel = async (context: Omit) => { if (!openedChannelId) { @@ -72,8 +77,13 @@ export const slackQaMpimAppMentionDedupeScenario: SlackQaScenarioImplementation }; return { expectReply: true, - input: `<@${sutUserId}> reply with only this exact marker: ${token}`, - matchText: token, + input: [ + `<@${sutUserId}> Slack MPIM assistant-history seed check.`, + `Reply with only a marker in this exact format: ${seedMarker}_BOT_.`, + "Replace with 8 to 32 new uppercase letters or digits.", + "Do not include angle brackets, spaces, Markdown, or punctuation.", + ].join(" "), + matchText: seedMarker, settleObservedMs: 60_000, beforeRun: async (context) => { const driverAuth = await context.driverClient.auth.test(); @@ -115,7 +125,7 @@ export const slackQaMpimAppMentionDedupeScenario: SlackQaScenarioImplementation verifyObserved: ({ messages }) => { const uniqueReplies = new Map(messages.map((message) => [message.ts, message])); const matchingReplies = [...uniqueReplies.values()].filter((message) => - message.text.includes(token), + message.text.includes(seedMarker), ); if (uniqueReplies.size !== 1 || matchingReplies.length !== 1) { throw new Error( @@ -124,6 +134,55 @@ export const slackQaMpimAppMentionDedupeScenario: SlackQaScenarioImplementation } return "one MPIM reply observed after message/app_mention twin delivery"; }, + afterReply: async (message, context) => { + if (message.thread_ts !== context.sentTs) { + throw new Error("MPIM seed reply escaped the native Slack thread"); + } + const botReplyMarker = message.text?.trim() ?? ""; + const botReplyPrefix = `${seedMarker}_BOT_`; + const botNonce = botReplyMarker.startsWith(botReplyPrefix) + ? botReplyMarker.slice(botReplyPrefix.length) + : ""; + if (!/^[A-Z0-9]{8,32}$/u.test(botNonce)) { + throw new Error("MPIM seed reply did not contain the provider-generated bot nonce"); + } + const expectedRecallMarker = `${recallMarker}_${botNonce}`; + const sent = await sendSlackChannelMessage({ + channelId: context.channelId, + client: context.driverClient, + text: [ + `<@${sutUserId}> Slack MPIM assistant-history recall check.`, + `Recall the nonce from your immediately previous reply beginning with ${botReplyPrefix}.`, + `Reply with only this exact format: ${recallMarker}_, using that same nonce.`, + `Otherwise reply with only: ${missingMarker}`, + ].join(" "), + threadTs: context.sentTs, + }); + const reply = await waitForSlackScenarioReply({ + channelId: context.channelId, + client: context.sutReadClient, + matchText: expectedRecallMarker, + observedMessages: [], + observationScenarioId: "slack-mpim-app-mention-dedupe", + observationScenarioTitle: "Slack MPIM app mention dispatches once with thread context", + sentTs: sent.ts, + sutIdentity: context.sutIdentity, + threadTs: context.sentTs, + timeoutMs: 60_000, + }); + if (reply.message.thread_ts !== context.sentTs) { + throw new Error("MPIM assistant-history recall reply escaped the native Slack thread"); + } + if (reply.message.text?.trim() !== expectedRecallMarker) { + throw new Error("MPIM assistant-history recall reply did not reproduce the hidden nonce"); + } + return [ + "threadHistoryHeader=true", + "assistantAttributedSeed=true", + "recalledNonceMatched=true", + "threaded MPIM follow-up recovered the prior bot reply as assistant history", + ].join("; "); + }, cleanup: closeOpenedChannel, }; }, diff --git a/extensions/qa-lab/src/providers/mock-openai/mock-openai-assistant-text.ts b/extensions/qa-lab/src/providers/mock-openai/mock-openai-assistant-text.ts index 763da5ed5f56..a1ed54f3ad7b 100644 --- a/extensions/qa-lab/src/providers/mock-openai/mock-openai-assistant-text.ts +++ b/extensions/qa-lab/src/providers/mock-openai/mock-openai-assistant-text.ts @@ -11,6 +11,9 @@ import { QA_SUBAGENT_DIRECT_FALLBACK_MARKER, QA_IMAGE_GENERATION_PROMPT_RE, QA_SKILL_WORKSHOP_GIF_PROMPT_RE, + QA_SLACK_MPIM_HISTORY_RECALL_PROMPT_RE, + QA_SLACK_MPIM_HISTORY_SEED_PROMPT_RE, + buildSlackMpimHistoryBotReply, QA_TOOL_SEARCH_PROMPT_RE, QA_TOOL_SEARCH_FAILURE_PROMPT_RE, type MockScenarioState, @@ -33,6 +36,7 @@ import { extractLastUserText, extractToolOutput, extractLatestToolOutput, + extractSlackMpimRetainedBotNonce, extractAllUserTexts, extractAllRequestTexts, extractLatestImageUserTurn, @@ -137,6 +141,18 @@ export function buildAssistantText( toolJson, }); + const slackMpimHistoryRecall = QA_SLACK_MPIM_HISTORY_RECALL_PROMPT_RE.exec(prompt); + if (slackMpimHistoryRecall) { + const [, botReplyPrefix, recalledMarker, missingMarker] = slackMpimHistoryRecall; + const nonce = botReplyPrefix + ? extractSlackMpimRetainedBotNonce(prompt, botReplyPrefix) + : undefined; + return nonce && recalledMarker ? `${recalledMarker}_${nonce}` : (missingMarker ?? ""); + } + const slackMpimHistorySeed = QA_SLACK_MPIM_HISTORY_SEED_PROMPT_RE.exec(prompt)?.[1]; + if (slackMpimHistorySeed) { + return buildSlackMpimHistoryBotReply(slackMpimHistorySeed); + } if (/what was the qa canary code/i.test(prompt) && rememberedFact) { return `Protocol note: the QA canary code was ${rememberedFact}.`; } diff --git a/extensions/qa-lab/src/providers/mock-openai/mock-openai-contracts.ts b/extensions/qa-lab/src/providers/mock-openai/mock-openai-contracts.ts index 5fab7342c5b5..de23202ad2b9 100644 --- a/extensions/qa-lab/src/providers/mock-openai/mock-openai-contracts.ts +++ b/extensions/qa-lab/src/providers/mock-openai/mock-openai-contracts.ts @@ -1,4 +1,5 @@ // QA Lab mock provider contracts, wire helpers, and scenario constants. +import { randomUUID } from "node:crypto"; import type { IncomingMessage, ServerResponse } from "node:http"; import { setTimeout as sleep } from "node:timers/promises"; import { readRequestBodyWithLimit } from "openclaw/plugin-sdk/webhook-ingress"; @@ -207,6 +208,14 @@ export const QA_TELEGRAM_LONG_FINAL_PROMPT_RE = /telegram long final qa check/i; export const QA_WHATSAPP_LONG_FINAL_PROMPT_RE = /whatsapp long final qa check/i; export const QA_SLACK_CHART_PRESENTATION_PROMPT_RE = /Slack native chart QA check\s+(SLACK_QA_CHART_SUMMARY_[A-Z0-9]+)[\s\S]*?reply with only this exact marker:\s*(SLACK_QA_CHART_DONE_[A-Z0-9]+)/i; +export const QA_SLACK_MPIM_HISTORY_SEED_PROMPT_RE = + /Slack MPIM assistant-history seed check[\s\S]*?exact format:\s*(SLACK_QA_MPIM_SEED_[A-Z0-9]+)_BOT_/i; +export const QA_SLACK_MPIM_HISTORY_RECALL_PROMPT_RE = + /Slack MPIM assistant-history recall check[\s\S]*?previous reply beginning with\s+(SLACK_QA_MPIM_SEED_[A-Z0-9]+_BOT_)[\s\S]*?exact format:\s*(SLACK_QA_MPIM_RECALL_[A-Z0-9]+)_[\s\S]*?otherwise reply with only:\s*(SLACK_QA_MPIM_MISSING_[A-Z0-9]+)/i; + +export function buildSlackMpimHistoryBotReply(seedMarker: string) { + return `${seedMarker}_BOT_${randomUUID().replaceAll("-", "").toUpperCase()}`; +} export const QA_WHATSAPP_AGENT_MESSAGE_ACTION_REACT_PROMPT_RE = /react to this whatsapp(?: group)? message with thumbs up for qa action check\s+(?:WHATSAPP_QA_AGENT_REACT|WHATSAPP_QA_GROUP_AGENT_REACT)_[A-Z0-9]+/i; export const QA_WHATSAPP_AGENT_MESSAGE_ACTION_UPLOAD_PROMPT_RE = diff --git a/extensions/qa-lab/src/providers/mock-openai/mock-openai-input.ts b/extensions/qa-lab/src/providers/mock-openai/mock-openai-input.ts index 05fb0b84cb45..efa78cd139b8 100644 --- a/extensions/qa-lab/src/providers/mock-openai/mock-openai-input.ts +++ b/extensions/qa-lab/src/providers/mock-openai/mock-openai-input.ts @@ -291,6 +291,41 @@ export function extractAllUserTexts(input: ResponsesInputItem[]) { return texts; } +export function extractSlackMpimRetainedBotNonce( + prompt: string, + botReplyPrefix: string, +): string | undefined { + const historyHeader = "[Thread history - for context]\n"; + const historyStart = prompt.indexOf(historyHeader); + if (historyStart < 0) { + return undefined; + } + const historyBodyStart = historyStart + historyHeader.length; + const currentTurnStart = prompt.lastIndexOf("Slack MPIM assistant-history recall check."); + if (currentTurnStart < historyBodyStart) { + return undefined; + } + for (const line of prompt.slice(historyBodyStart, currentTurnStart).split(/\r?\n/u)) { + const headerEnd = line.indexOf("] "); + if (headerEnd < 0) { + continue; + } + const header = line.slice(0, headerEnd); + if (!header.startsWith("[Slack ") || !header.includes(" (this assistant) (assistant) ")) { + continue; + } + const reply = line.slice(headerEnd + 2); + if (!reply.startsWith(botReplyPrefix)) { + continue; + } + const nonce = reply.slice(botReplyPrefix.length); + if (/^[A-Z0-9]{8,32}$/u.test(nonce)) { + return nonce; + } + } + return undefined; +} + export function extractAllInputTexts(input: ResponsesInputItem[]) { const texts: string[] = []; for (const item of input) { diff --git a/extensions/qa-lab/src/providers/mock-openai/server.test.ts b/extensions/qa-lab/src/providers/mock-openai/server.test.ts index 2e10bc97181b..ce7936bc9c6d 100644 --- a/extensions/qa-lab/src/providers/mock-openai/server.test.ts +++ b/extensions/qa-lab/src/providers/mock-openai/server.test.ts @@ -1433,6 +1433,85 @@ describe("qa mock openai server", () => { expect(body).not.toContain('"name":"read"'); }); + it("requires retained bot history in the Slack MPIM thread-history prelude", async () => { + const server = await startMockServer(); + const seedMarker = "SLACK_QA_MPIM_SEED_A1B2C3D4"; + const recallMarker = "SLACK_QA_MPIM_RECALL_A1B2C3D4"; + const missingMarker = "SLACK_QA_MPIM_MISSING_A1B2C3D4"; + const seedPrompt = + `Slack MPIM assistant-history seed check. Reply with only a marker in this exact format: ${seedMarker}_BOT_. ` + + "Replace with 8 to 32 new uppercase letters or digits. " + + "Do not include angle brackets, spaces, Markdown, or punctuation."; + const seedResponse = await expectResponsesJson(server, { + stream: false, + model: "gpt-5.6-luna", + input: [makeUserInput(seedPrompt)], + }); + const botReplyMarker = outputText(seedResponse); + expect(botReplyMarker).toMatch(new RegExp(`^${seedMarker}_BOT_[A-Z0-9]+$`, "u")); + const botNonce = botReplyMarker.slice(`${seedMarker}_BOT_`.length); + const expectedRecallMarker = `${recallMarker}_${botNonce}`; + const recallPrompt = [ + "Slack MPIM assistant-history recall check.", + `Recall the nonce from your immediately previous reply beginning with ${seedMarker}_BOT_.`, + `Reply with only this exact format: ${recallMarker}_, using that same nonce.`, + `Otherwise reply with only: ${missingMarker}`, + ].join(" "); + expect(recallPrompt).not.toContain(botReplyMarker); + expect(recallPrompt).not.toContain(botNonce); + + const withRetainedBotHistory = await expectResponsesJson(server, { + stream: false, + model: "gpt-5.6-luna", + input: [ + makeUserInput( + [ + "[Thread history - for context]", + `[Slack Driver (user) Fri 2026-07-31 10:00 UTC] ${seedPrompt}`, + "[slack message id: 1.000000 channel: C123]", + "", + `[Slack OpenClaw (this assistant) (assistant) Fri 2026-07-31 10:01 UTC] ${botReplyMarker}`, + "[slack message id: 1.500000 channel: C123]", + "", + `[Slack Driver (user) Fri 2026-07-31 10:02 UTC] ${recallPrompt}`, + ].join("\n"), + ), + ], + }); + expect(outputText(withRetainedBotHistory)).toBe(expectedRecallMarker); + + const withStructuredAssistantHistoryOnly = await expectResponsesJson(server, { + stream: false, + model: "gpt-5.6-luna", + input: [ + makeUserInput(seedPrompt), + { + role: "assistant", + content: [{ type: "output_text", text: botReplyMarker }], + }, + makeUserInput(recallPrompt), + ], + }); + expect(outputText(withStructuredAssistantHistoryOnly)).toBe(missingMarker); + + const withHumanAttributedSeed = await expectResponsesJson(server, { + stream: false, + model: "gpt-5.6-luna", + input: [ + makeUserInput( + [ + "[Thread history - for context]", + `[Slack Alice (user) Fri 2026-07-31 10:00 UTC] ${botReplyMarker}`, + "[slack message id: 1.000000 channel: C123]", + "", + `[Slack Driver (user) Fri 2026-07-31 10:02 UTC] ${recallPrompt}`, + ].join("\n"), + ), + ], + }); + expect(outputText(withHumanAttributedSeed)).toBe(missingMarker); + }); + it("drives repo-contract followthrough as read-read-read-write-then-report", async () => { const server = await startMockServer(); diff --git a/extensions/qa-lab/src/providers/mock-openai/server.ts b/extensions/qa-lab/src/providers/mock-openai/server.ts index 36a30ae4908a..b41e10c46c4b 100644 --- a/extensions/qa-lab/src/providers/mock-openai/server.ts +++ b/extensions/qa-lab/src/providers/mock-openai/server.ts @@ -43,6 +43,9 @@ import { QA_TELEGRAM_LONG_FINAL_PROMPT_RE, QA_WHATSAPP_LONG_FINAL_PROMPT_RE, QA_SLACK_CHART_PRESENTATION_PROMPT_RE, + QA_SLACK_MPIM_HISTORY_RECALL_PROMPT_RE, + QA_SLACK_MPIM_HISTORY_SEED_PROMPT_RE, + buildSlackMpimHistoryBotReply, QA_WHATSAPP_AGENT_MESSAGE_ACTION_REACT_PROMPT_RE, QA_WHATSAPP_AGENT_MESSAGE_ACTION_UPLOAD_PROMPT_RE, QA_SUBAGENT_DIRECT_FALLBACK_PROMPT_RE, @@ -126,6 +129,7 @@ import { extractLatestToolOutput, extractAllToolOutputText, extractUserTextAfterLatestToolOutput, + extractSlackMpimRetainedBotNonce, extractAllUserTexts, extractAllInputTexts, extractInstructionsText, @@ -1015,6 +1019,20 @@ async function buildResponsesPayload( if (whatsAppStickerMarker) { return buildAssistantEvents(whatsAppStickerMarker); } + const slackMpimHistoryRecall = QA_SLACK_MPIM_HISTORY_RECALL_PROMPT_RE.exec(prompt); + if (slackMpimHistoryRecall) { + const [, botReplyPrefix, recalledMarker, missingMarker] = slackMpimHistoryRecall; + const nonce = botReplyPrefix + ? extractSlackMpimRetainedBotNonce(prompt, botReplyPrefix) + : undefined; + return buildAssistantEvents( + nonce && recalledMarker ? `${recalledMarker}_${nonce}` : (missingMarker ?? ""), + ); + } + const slackMpimHistorySeed = QA_SLACK_MPIM_HISTORY_SEED_PROMPT_RE.exec(prompt)?.[1]; + if (slackMpimHistorySeed) { + return buildAssistantEvents(buildSlackMpimHistoryBotReply(slackMpimHistorySeed)); + } if (/\bmarker\b/i.test(prompt) && promptExactMarkerDirective) { return buildAssistantEvents(promptExactMarkerDirective); } diff --git a/extensions/slack/src/monitor/message-handler/prepare-thread-context-root.test.ts b/extensions/slack/src/monitor/message-handler/prepare-thread-context-root.test.ts index 49837055af5c..a47abc82f094 100644 --- a/extensions/slack/src/monitor/message-handler/prepare-thread-context-root.test.ts +++ b/extensions/slack/src/monitor/message-handler/prepare-thread-context-root.test.ts @@ -72,7 +72,7 @@ describe("resolveSlackThreadHistoryFilterPolicy", () => { includeBotStarterAsRootContext: true, starterTs: "1", }), - ).toEqual({ retainCurrentBotRootTs: "1" }); + ).toEqual({ currentBot: "root-only", rootTs: "1" }); }); it("filters current-bot messages on existing sessions", () => { @@ -81,7 +81,17 @@ describe("resolveSlackThreadHistoryFilterPolicy", () => { includeBotStarterAsRootContext: false, starterTs: "1", }), - ).toEqual({}); + ).toEqual({ currentBot: "omit" }); + }); + + it("retains current-bot history when the owner requests thread reconstruction", () => { + expect( + resolveSlackThreadHistoryFilterPolicy({ + includeBotStarterAsRootContext: false, + starterTs: "1", + retainCurrentBotHistory: true, + }), + ).toEqual({ currentBot: "all" }); }); }); @@ -96,7 +106,7 @@ describe("applySlackThreadHistoryFilterPolicy", () => { ]; const result = applySlackThreadHistoryFilterPolicy({ history, - policy: { retainCurrentBotRootTs: "1" }, + policy: { currentBot: "root-only", rootTs: "1" }, identity, }); expect(result.kept.map((entry) => entry.ts)).toEqual(["1", "2"]); @@ -112,17 +122,32 @@ describe("applySlackThreadHistoryFilterPolicy", () => { ]; const result = applySlackThreadHistoryFilterPolicy({ history, - policy: {}, + policy: { currentBot: "omit" }, identity, }); expect(result.kept.map((entry) => entry.ts)).toEqual(["3", "4"]); expect(result.omittedCurrentBot).toBe(2); }); + it("keeps all current-bot messages when reconstructing a fresh thread", () => { + const history = [ + { ts: "1", userId: "U1", text: "user root" }, + { ts: "2", botId: "B1", text: "assistant reply" }, + { ts: "3", userId: "U_BOT", text: "assistant follow-up" }, + ]; + const result = applySlackThreadHistoryFilterPolicy({ + history, + policy: { currentBot: "all" }, + identity, + }); + expect(result.kept).toEqual(history); + expect(result.omittedCurrentBot).toBe(0); + }); + it("returns an empty result for empty history", () => { const result = applySlackThreadHistoryFilterPolicy({ history: [] as Array<{ ts: string; userId?: string; botId?: string }>, - policy: {}, + policy: { currentBot: "omit" }, identity, }); expect(result.kept).toEqual([]); diff --git a/extensions/slack/src/monitor/message-handler/prepare-thread-context-root.ts b/extensions/slack/src/monitor/message-handler/prepare-thread-context-root.ts index 1941f5d1cf99..27069af7f575 100644 --- a/extensions/slack/src/monitor/message-handler/prepare-thread-context-root.ts +++ b/extensions/slack/src/monitor/message-handler/prepare-thread-context-root.ts @@ -16,9 +16,9 @@ type SlackThreadRootCandidate = SlackThreadAuthorTuple & { ts?: string; }; -type SlackThreadHistoryFilterPolicy = { - retainCurrentBotRootTs?: string; -}; +type SlackThreadHistoryFilterPolicy = + | { currentBot: "omit" | "all" } + | { currentBot: "root-only"; rootTs: string }; type SlackThreadHistoryFilterResult = { kept: T[]; @@ -42,12 +42,17 @@ export function isSlackThreadAuthorCurrentBot(params: { export function resolveSlackThreadHistoryFilterPolicy(params: { includeBotStarterAsRootContext: boolean; starterTs?: string; + retainCurrentBotHistory?: boolean; }): SlackThreadHistoryFilterPolicy { + if (params.retainCurrentBotHistory) { + return { currentBot: "all" }; + } if (!params.includeBotStarterAsRootContext || !params.starterTs) { - return {}; + return { currentBot: "omit" }; } return { - retainCurrentBotRootTs: params.starterTs, + currentBot: "root-only", + rootTs: params.starterTs, }; } @@ -67,7 +72,10 @@ export function applySlackThreadHistoryFilterPolicy { allowFromLower: string[]; allowNameMatching: boolean; sessionState?: "missing" | "fresh" | "stale"; + sessionLastInteractionAt?: number; + sessionUpdatedAt?: number; + isGroupDm?: boolean; }) { const { storePath } = storeFixture.makeTmpStorePath(); const replies = vi.fn().mockResolvedValue({ @@ -73,7 +76,20 @@ describe("resolveSlackThreadContextData", () => { ctx.channelRuntime = { ...ctx.channelRuntime!, session: { - resolveEntryResetFreshness: () => ({ state: params.sessionState }), + resolveEntryResetFreshness: () => + params.sessionState === "missing" + ? { state: "missing", entry: undefined } + : { + state: params.sessionState, + entry: { + ...(params.sessionLastInteractionAt !== undefined + ? { lastInteractionAt: params.sessionLastInteractionAt } + : {}), + ...(params.sessionUpdatedAt !== undefined + ? { updatedAt: params.sessionUpdatedAt } + : {}), + }, + }, }, }; } @@ -87,6 +103,7 @@ describe("resolveSlackThreadContextData", () => { ctx, account: createSlackTestAccount({ thread: { initialHistoryLimit: 20 } }), message: createThreadMessage(), + isGroupDm: params.isGroupDm ?? false, isThreadReply: true, threadTs: "100.000", threadStarter: params.threadStarter, @@ -128,14 +145,20 @@ describe("resolveSlackThreadContextData", () => { { title: "does not hydrate starter media for an existing thread session", sessionState: "fresh" as const, + sessionLastInteractionAt: 100, hydrates: false, }, + { + title: "hydrates starter media for an outbound-only thread session", + sessionState: "fresh" as const, + hydrates: true, + }, { title: "hydrates starter media after a thread session reset", sessionState: "stale" as const, hydrates: true, }, - ])("$title", async ({ sessionState, hydrates }) => { + ])("$title", async ({ sessionState, sessionLastInteractionAt, hydrates }) => { const resolveSlackMedia = vi .spyOn(mediaModule, "resolveSlackMedia") .mockResolvedValue(starterMedia); @@ -145,6 +168,7 @@ describe("resolveSlackThreadContextData", () => { allowFromLower: ["u1"], allowNameMatching: false, sessionState, + sessionLastInteractionAt, }); expect(result.threadStarterMedia).toEqual(hydrates ? starterMedia : null); @@ -179,28 +203,120 @@ describe("resolveSlackThreadContextData", () => { expect(replies).toHaveBeenCalledTimes(1); }); - it("filters prior current-bot replies from user-started threads on new sessions", async () => { + it.each([ + { + title: "filters them from missing channel threads", + isGroupDm: false, + sessionState: "missing" as const, + retained: false, + }, + { + title: "filters them from fresh outbound-only channel threads", + isGroupDm: false, + sessionState: "fresh" as const, + retained: false, + }, + { + title: "filters them from stale outbound-only channel threads", + isGroupDm: false, + sessionState: "stale" as const, + retained: false, + }, + { + title: "retains them for missing MPIM threads", + isGroupDm: true, + sessionState: "missing" as const, + retained: true, + }, + { + title: "retains them for fresh outbound-only MPIM threads", + isGroupDm: true, + sessionState: "fresh" as const, + retained: true, + }, + { + title: "retains them for stale outbound-only MPIM threads", + isGroupDm: true, + sessionState: "stale" as const, + retained: true, + }, + { + title: "filters them after an inbound MPIM interaction", + isGroupDm: true, + sessionState: "stale" as const, + sessionLastInteractionAt: 100, + retained: false, + }, + { + title: "filters them after an explicit MPIM reset", + isGroupDm: true, + sessionState: "stale" as const, + sessionUpdatedAt: 0, + retained: false, + }, + ])( + "$title", + async ({ isGroupDm, sessionState, sessionLastInteractionAt, sessionUpdatedAt, retained }) => { + const { result } = await resolveAllowlistedThreadContext({ + repliesMessages: [ + { text: "starter from Alice", user: "U1", ts: "100.000" }, + { text: "assistant progress update", bot_id: "B1", ts: "100.200" }, + { text: "allowed follow-up", user: "U1", ts: "100.800" }, + { text: "current message", user: "U1", ts: "101.000" }, + ], + threadStarter: { + text: "starter from Alice", + userId: "U1", + ts: "100.000", + }, + allowFromLower: ["u1"], + allowNameMatching: false, + sessionState, + sessionLastInteractionAt, + sessionUpdatedAt, + isGroupDm, + }); + + expect(result.threadStarterBody).toBe("starter from Alice"); + expect(result.threadHistoryBody).toContain("starter from Alice"); + expect(result.threadHistoryBody).toContain("allowed follow-up"); + if (retained) { + expect(result.threadHistoryBody).toContain("assistant progress update"); + expect(result.threadHistoryBody).toContain("Bot (this assistant) (assistant)"); + } else { + expect(result.threadHistoryBody).not.toContain("assistant progress update"); + } + expect(result.threadHistoryBody).not.toContain("current message"); + }, + ); + + it("keeps the 20-message cap and excludes the current MPIM message", async () => { + const priorMessages = Array.from({ length: 22 }, (_, index) => ({ + text: index === 20 ? "assistant answer to retain" : `prior user message ${index}`, + ...(index === 20 ? { bot_id: "B1" } : { user: "U1" }), + ts: `100.${String(index).padStart(3, "0")}`, + })); const { result } = await resolveAllowlistedThreadContext({ - repliesMessages: [ - { text: "starter from Alice", user: "U1", ts: "100.000" }, - { text: "assistant progress update", bot_id: "B1", ts: "100.200" }, - { text: "allowed follow-up", user: "U1", ts: "100.800" }, - { text: "current message", user: "U1", ts: "101.000" }, - ], + repliesMessages: [...priorMessages, { text: "current message", user: "U1", ts: "101.000" }], threadStarter: { - text: "starter from Alice", + text: "prior user message 0", userId: "U1", ts: "100.000", }, allowFromLower: ["u1"], allowNameMatching: false, + sessionState: "fresh", + isGroupDm: true, }); - expect(result.threadStarterBody).toBe("starter from Alice"); - expect(result.threadHistoryBody).toContain("starter from Alice"); - expect(result.threadHistoryBody).toContain("allowed follow-up"); - expect(result.threadHistoryBody).not.toContain("assistant progress update"); - expect(result.threadHistoryBody).not.toContain("current message"); + const history = result.threadHistoryBody ?? ""; + expect(history.match(/\[slack message id:/g)).toHaveLength(20); + expect(history).not.toContain("[slack message id: 100.000 channel: C123]"); + expect(history).not.toContain("[slack message id: 100.001 channel: C123]"); + expect(history).toContain("prior user message 21"); + expect(history).toContain("assistant answer to retain"); + expect(history).toContain("Bot (this assistant) (assistant)"); + expect(history).not.toContain("current message"); }); it("keeps starter text and history when allowNameMatching authorizes the sender", async () => { @@ -285,6 +401,7 @@ describe("resolveSlackThreadContextData", () => { ctx, account: createSlackTestAccount({ thread: { initialHistoryLimit: 20 } }), message: createThreadMessage(), + isGroupDm: false, isThreadReply: true, threadTs: "100.000", threadStarter: { @@ -331,6 +448,7 @@ describe("resolveSlackThreadContextData", () => { ctx, account: createSlackTestAccount({ thread: { initialHistoryLimit: 1 } }), message: createThreadMessage(), + isGroupDm: false, isThreadReply: true, threadTs: "100.000", threadStarter: { @@ -455,6 +573,7 @@ describe("resolveSlackThreadContextData", () => { text: "actually it's Sunday 12:30 pm - apologize and correct", ts: "101.000", }), + isGroupDm: false, isThreadReply: true, threadTs: "100.000", threadStarter: { diff --git a/extensions/slack/src/monitor/message-handler/prepare-thread-context.ts b/extensions/slack/src/monitor/message-handler/prepare-thread-context.ts index ca7220122d52..fc482a6aaf8a 100644 --- a/extensions/slack/src/monitor/message-handler/prepare-thread-context.ts +++ b/extensions/slack/src/monitor/message-handler/prepare-thread-context.ts @@ -43,9 +43,18 @@ type SlackThreadContextData = { const SLACK_THREAD_CONTEXT_USER_LOOKUP_CONCURRENCY = 4; -type SlackSessionResetFreshness = { - state: "missing" | "fresh" | "stale"; -}; +type SlackSessionResetFreshness = + | { + state: "missing"; + entry: undefined; + } + | { + state: "fresh" | "stale"; + entry: { + lastInteractionAt?: number; + updatedAt?: number; + }; + }; type SlackSessionFreshnessRuntime = { session?: { @@ -146,6 +155,7 @@ export async function resolveSlackThreadContextData(params: { ctx: SlackMonitorContext; account: ResolvedSlackAccount; message: SlackMessageEvent; + isGroupDm: boolean; isThreadReply: boolean; threadTs: string | undefined; threadStarter: SlackThreadStarter | null; @@ -188,11 +198,21 @@ export async function resolveSlackThreadContextData(params: { sessionKey: params.sessionKey, }) : undefined; + const isMissingThreadSession = threadSessionFreshness + ? threadSessionFreshness.state === "missing" + : threadSessionPreviousTimestamp === undefined; + // A zero updatedAt is an explicit reset tombstone, not an outbound-created row. + // Rehydrating it would resurrect history that the reset intentionally discarded. + const isOutboundOnlyThreadSession = + threadSessionFreshness !== undefined && + threadSessionFreshness.state !== "missing" && + threadSessionFreshness.entry.lastInteractionAt === undefined && + threadSessionFreshness.entry.updatedAt !== 0; const shouldSeedInitialThreadContext = Boolean( params.isThreadReply && params.threadTs && (threadSessionFreshness - ? threadSessionFreshness.state !== "fresh" + ? threadSessionFreshness.state !== "fresh" || isOutboundOnlyThreadSession : threadSessionPreviousTimestamp === undefined), ); const shouldLoadInitialThreadHistory = @@ -309,6 +329,11 @@ export async function resolveSlackThreadContextData(params: { const historyFilterPolicy = resolveSlackThreadHistoryFilterPolicy({ includeBotStarterAsRootContext, starterTs: currentBotRootTs, + // MPIM roots intentionally stay on the flat group session. Outbound + // delivery may create the reply-thread session before its first inbound + // turn, so recover those assistant replies when hydrating that session. + retainCurrentBotHistory: + params.isGroupDm && (isMissingThreadSession || isOutboundOnlyThreadSession), }); const { kept: threadHistoryWithoutCurrentBot, diff --git a/extensions/slack/src/monitor/message-handler/prepare.test.ts b/extensions/slack/src/monitor/message-handler/prepare.test.ts index 3be445fdb6a4..34667563ff9a 100644 --- a/extensions/slack/src/monitor/message-handler/prepare.test.ts +++ b/extensions/slack/src/monitor/message-handler/prepare.test.ts @@ -1182,6 +1182,7 @@ describe("slack prepareSlackMessage inbound contract", () => { currentTs: string; channelsConfig?: Parameters[0]["channelsConfig"]; allowFrom?: string[]; + outboundOnlySessionKey?: string; resolveChannelName?: (channelId: string) => Promise<{ name?: string; type?: SlackMessageEvent["channel_type"]; @@ -1192,6 +1193,16 @@ describe("slack prepareSlackMessage inbound contract", () => { async function prepareThreadContextAllowlistCase(params: ThreadContextAllowlistCaseParams) { const { storePath } = storeFixture.makeTmpStorePath(); + if (params.outboundOnlySessionKey) { + const now = Date.now(); + await seedSessionEntries(storePath, { + [params.outboundOnlySessionKey]: { + sessionId: "outbound-only-thread-session", + updatedAt: now, + sessionStartedAt: now, + }, + }); + } const historyUser = params.historyUser ?? params.user; const replies = vi .fn() @@ -1257,7 +1268,7 @@ describe("slack prepareSlackMessage inbound contract", () => { replies: ReturnType, starterText: string, followUpText: string, - options?: { expectStarterBody?: boolean }, + options?: { expectStarterBody?: boolean; expectAssistantHistory?: boolean }, ) { assertPrepared(prepared); if (options?.expectStarterBody === false) { @@ -1267,7 +1278,12 @@ describe("slack prepareSlackMessage inbound contract", () => { } expect(prepared.ctxPayload.ThreadHistoryBody).toContain(starterText); expect(prepared.ctxPayload.ThreadHistoryBody).toContain(followUpText); - expect(prepared.ctxPayload.ThreadHistoryBody).not.toContain("assistant reply"); + if (options?.expectAssistantHistory) { + expect(prepared.ctxPayload.ThreadHistoryBody).toContain("assistant reply"); + expect(prepared.ctxPayload.ThreadHistoryBody).toContain("Bot (this assistant) (assistant)"); + } else { + expect(prepared.ctxPayload.ThreadHistoryBody).not.toContain("assistant reply"); + } expect(prepared.ctxPayload.ThreadHistoryBody).not.toContain("current message"); expect(replies).toHaveBeenCalledTimes(2); } @@ -2873,9 +2889,18 @@ Second paragraph should still reach the agent after Slack's preview cutoff.`; followUpTs: "400.800", currentTs: "401.000", allowFrom: ["U4"], + outboundOnlySessionKey: "agent:main:slack:group:g400:thread:400.000", }); - expectThreadContextAllowsHumanHistory(prepared, replies, "starter from mpim", "mpim follow-up"); + expectThreadContextAllowsHumanHistory( + prepared, + replies, + "starter from mpim", + "mpim follow-up", + { + expectAssistantHistory: true, + }, + ); }); it("skips loading thread history when thread session already exists in store (bloat fix)", async () => { diff --git a/extensions/slack/src/monitor/message-handler/prepare.thread-session-key.test.ts b/extensions/slack/src/monitor/message-handler/prepare.thread-session-key.test.ts index 81275e6291b0..0d72edf6986a 100644 --- a/extensions/slack/src/monitor/message-handler/prepare.thread-session-key.test.ts +++ b/extensions/slack/src/monitor/message-handler/prepare.thread-session-key.test.ts @@ -365,9 +365,96 @@ describe("thread-level session keys", () => { expect(routing.threadContext.replyToId).toBeUndefined(); }); - it("does not seed top-level group DM mentions into thread sessions", () => { + it.each( + (["off", "first", "all", "batched"] as const).flatMap((replyToMode) => + [false, true].map((mentioned) => ({ replyToMode, mentioned })), + ), + )( + "keeps $replyToMode MPIM roots flat and routes $mentioned mention follow-ups by thread", + ({ replyToMode, mentioned }) => { + const ctx = buildCtx({ replyToMode }); + const account = buildAccount(replyToMode); + const rootTs = "1777244692.409919"; + const root = resolveSlackRoutingContext({ + ctx, + account, + message: buildChannelMessage({ + channel: "G123", + channel_type: "mpim", + text: mentioned ? "<@B1> send a subagent" : "send a subagent", + ts: rootTs, + }), + isDirectMessage: false, + isGroupDm: true, + isRoom: false, + isRoomish: true, + seedTopLevelRoomThread: mentioned, + }); + const followUp = resolveSlackRoutingContext({ + ctx, + account, + message: buildChannelMessage({ + channel: "G123", + channel_type: "mpim", + text: "what did you find?", + ts: "1777244714.000100", + thread_ts: rootTs, + parent_user_id: "U1", + }), + isDirectMessage: false, + isGroupDm: true, + isRoom: false, + isRoomish: true, + }); + + expect(root.sessionKey).toBe("agent:main:slack:group:g123"); + expect(root.historyKey).toBe("G123"); + expect(root.threadContext.replyToId).toBeUndefined(); + expect(root.threadContext.messageThreadId).toBe(replyToMode === "all" ? rootTs : undefined); + expect(followUp.sessionKey).toBe(`agent:main:slack:group:g123:thread:${rootTs}`); + expect(followUp.historyKey).toBe(followUp.sessionKey); + expect(followUp.threadContext.replyToId).toBe(rootTs); + expect(followUp.threadContext.messageThreadId).toBe(rootTs); + expect(followUp.sessionKey).not.toContain("1777244714.000100"); + }, + ); + + it("keeps configured MPIM bindings flat when Slack starts a reply thread", () => { const ctx = buildCtx({ replyToMode: "all" }); const account = buildAccount("all"); + const targetSessionKey = "agent:codex:acp:binding:slack:default:g123"; + resolveConfiguredBindingRouteMock.mockImplementation(({ route, conversation }) => ({ + bindingResolution: { + conversation, + record: { + bindingId: "config:acp:slack:default:g123", + targetSessionKey, + targetKind: "session", + conversation: { + channel: "slack", + accountId: "default", + conversationId: "g123", + }, + status: "active", + boundAt: 0, + metadata: { + source: "config", + mode: "persistent", + agentId: "codex", + }, + }, + }, + boundSessionKey: targetSessionKey, + boundAgentId: "codex", + route: { + ...route, + agentId: "codex", + sessionKey: targetSessionKey, + mainSessionKey: "agent:codex:main", + matchedBy: "binding.channel", + lastRoutePolicy: "session", + }, + })); const routing = resolveSlackRoutingContext({ ctx, @@ -375,17 +462,18 @@ describe("thread-level session keys", () => { message: buildChannelMessage({ channel: "G123", channel_type: "mpim", - text: "<@B1> send a subagent", - ts: "1777244692.409919", + text: "what did you find?", + ts: "1777244714.000100", + thread_ts: "1777244692.409919", + parent_user_id: "U1", }), isDirectMessage: false, isGroupDm: true, isRoom: false, isRoomish: true, - seedTopLevelRoomThread: true, }); - expect(routing.sessionKey).toBe("agent:main:slack:group:g123"); + expect(routing.sessionKey).toBe(targetSessionKey); expect(routing.sessionKey).not.toContain(":thread:"); }); diff --git a/extensions/slack/src/monitor/message-handler/prepare.ts b/extensions/slack/src/monitor/message-handler/prepare.ts index f147bd94790f..67e7bacfc550 100644 --- a/extensions/slack/src/monitor/message-handler/prepare.ts +++ b/extensions/slack/src/monitor/message-handler/prepare.ts @@ -1555,6 +1555,7 @@ export async function prepareSlackMessage(params: { ctx, account, message, + isGroupDm, isThreadReply, threadTs, threadStarter, diff --git a/qa/scenarios/channels/slack-mpim-app-mention-dedupe.yaml b/qa/scenarios/channels/slack-mpim-app-mention-dedupe.yaml index c09cfa438a23..c9d23c1274a7 100644 --- a/qa/scenarios/channels/slack-mpim-app-mention-dedupe.yaml +++ b/qa/scenarios/channels/slack-mpim-app-mention-dedupe.yaml @@ -1,4 +1,4 @@ -title: Slack MPIM app mention dispatches once +title: Slack MPIM app mention dispatches once with thread context scenario: id: slack-mpim-app-mention-dedupe surface: channels