diff --git a/extensions/slack/src/monitor/context.test.ts b/extensions/slack/src/monitor/context.test.ts index 76f31aef171..f7dd4e23db5 100644 --- a/extensions/slack/src/monitor/context.test.ts +++ b/extensions/slack/src/monitor/context.test.ts @@ -15,6 +15,7 @@ function createTestContext() { app: { client: {} } as App, runtime: {} as RuntimeEnv, botUserId: "U_BOT", + botId: "B_BOT", teamId: "T_EXPECTED", apiAppId: "A_EXPECTED", historyLimit: 0, diff --git a/extensions/slack/src/monitor/context.ts b/extensions/slack/src/monitor/context.ts index d04d4b5953d..e1f1c901e30 100644 --- a/extensions/slack/src/monitor/context.ts +++ b/extensions/slack/src/monitor/context.ts @@ -35,6 +35,7 @@ export type SlackMonitorContext = { runtime: RuntimeEnv; botUserId: string; + botId?: string; teamId: string; apiAppId: string; @@ -103,6 +104,7 @@ export function createSlackMonitorContext(params: { runtime: RuntimeEnv; botUserId: string; + botId?: string; teamId: string; apiAppId: string; @@ -398,6 +400,7 @@ export function createSlackMonitorContext(params: { app: params.app, runtime: params.runtime, botUserId: params.botUserId, + botId: params.botId, teamId: params.teamId, apiAppId: params.apiAppId, historyLimit: params.historyLimit, diff --git a/extensions/slack/src/monitor/message-handler/prepare-thread-context.test.ts b/extensions/slack/src/monitor/message-handler/prepare-thread-context.test.ts index 8aeeb2be822..2c0dbccfdca 100644 --- a/extensions/slack/src/monitor/message-handler/prepare-thread-context.test.ts +++ b/extensions/slack/src/monitor/message-handler/prepare-thread-context.test.ts @@ -46,7 +46,7 @@ describe("resolveSlackThreadContextData", () => { async function resolveAllowlistedThreadContext(params: { repliesMessages: Array>; - threadStarter: { text: string; userId: string; ts: string }; + threadStarter: { text: string; userId?: string; ts: string; botId?: string }; allowFromLower: string[]; allowNameMatching: boolean; }) { @@ -56,6 +56,8 @@ describe("resolveSlackThreadContextData", () => { response_metadata: { next_cursor: "" }, }); const ctx = createThreadContext({ replies }); + ctx.botUserId = "U_BOT"; + ctx.botId = "B1"; ctx.resolveUserName = async (id: string) => ({ name: id === "U1" ? "Alice" : "Mallory", }); @@ -100,8 +102,8 @@ describe("resolveSlackThreadContextData", () => { expect(result.threadStarterBody).toBeUndefined(); expect(result.threadLabel).toBe("Slack thread #general"); - expect(result.threadHistoryBody).toContain("assistant reply"); expect(result.threadHistoryBody).toContain("allowed follow-up"); + expect(result.threadHistoryBody).not.toContain("assistant reply"); expect(result.threadHistoryBody).not.toContain("starter secret"); expect(result.threadHistoryBody).not.toContain("blocked follow-up"); expect(result.threadHistoryBody).not.toContain("current message"); @@ -129,4 +131,72 @@ describe("resolveSlackThreadContextData", () => { expect(result.threadHistoryBody).toContain("starter from Alice"); expect(result.threadHistoryBody).not.toContain("blocked follow-up"); }); + + it("omits bot-authored starter text and history from a new thread session", async () => { + const { result } = await resolveAllowlistedThreadContext({ + repliesMessages: [ + { text: "bot starter", bot_id: "B1", ts: "100.000" }, + { text: "allowed follow-up", user: "U1", ts: "100.800" }, + { text: "current message", user: "U1", ts: "101.000" }, + ], + threadStarter: { + text: "bot starter", + botId: "B1", + ts: "100.000", + }, + allowFromLower: ["u1"], + allowNameMatching: false, + }); + + expect(result.threadStarterBody).toBeUndefined(); + expect(result.threadLabel).toBe("Slack thread #general"); + expect(result.threadHistoryBody).toContain("allowed follow-up"); + expect(result.threadHistoryBody).not.toContain("bot starter"); + expect(result.threadHistoryBody).not.toContain("current message"); + }); + + it("keeps third-party bot starter text in a new thread session", async () => { + const { result } = await resolveAllowlistedThreadContext({ + repliesMessages: [ + { text: "other bot starter", bot_id: "B2", ts: "100.000" }, + { text: "allowed follow-up", user: "U1", ts: "100.800" }, + { text: "current message", user: "U1", ts: "101.000" }, + ], + threadStarter: { + text: "other bot starter", + botId: "B2", + ts: "100.000", + }, + allowFromLower: ["u1"], + allowNameMatching: false, + }); + + expect(result.threadStarterBody).toBe("other bot starter"); + expect(result.threadLabel).toContain("other bot starter"); + expect(result.threadHistoryBody).toContain("other bot starter"); + expect(result.threadHistoryBody).toContain("Bot (B2) (assistant)"); + expect(result.threadHistoryBody).toContain("allowed follow-up"); + expect(result.threadHistoryBody).not.toContain("Unknown (user)"); + }); + + it("omits self-authored starter text when identified by bot user id", async () => { + const { result } = await resolveAllowlistedThreadContext({ + repliesMessages: [ + { text: "self starter", user: "U_BOT", ts: "100.000" }, + { text: "allowed follow-up", user: "U1", ts: "100.800" }, + { text: "current message", user: "U1", ts: "101.000" }, + ], + threadStarter: { + text: "self starter", + userId: "U_BOT", + ts: "100.000", + }, + allowFromLower: ["u1"], + allowNameMatching: false, + }); + + expect(result.threadStarterBody).toBeUndefined(); + expect(result.threadHistoryBody).toContain("allowed follow-up"); + expect(result.threadHistoryBody).not.toContain("self starter"); + }); }); 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 260a26b9e8c..a929df13ff2 100644 --- a/extensions/slack/src/monitor/message-handler/prepare-thread-context.ts +++ b/extensions/slack/src/monitor/message-handler/prepare-thread-context.ts @@ -64,6 +64,12 @@ export async function resolveSlackThreadContextData(params: { >; effectiveDirectMedia: SlackMediaResult[] | null; }): Promise { + const isCurrentBotAuthor = (author: { userId?: string; botId?: string }): boolean => + Boolean( + (params.ctx.botUserId && author.userId && author.userId === params.ctx.botUserId) || + (params.ctx.botId && author.botId && author.botId === params.ctx.botId), + ); + let threadStarterBody: string | undefined; let threadHistoryBody: string | undefined; let threadSessionPreviousTimestamp: number | undefined; @@ -85,22 +91,31 @@ export async function resolveSlackThreadContextData(params: { params.allowNameMatching && starter?.userId ? (await params.ctx.resolveUserName(starter.userId))?.name : undefined; + const starterIsCurrentBot = Boolean( + starter && + isCurrentBotAuthor({ + userId: starter.userId, + botId: starter.botId, + }), + ); const starterAllowed = !starter || - isSlackThreadContextSenderAllowed({ - allowFromLower: params.allowFromLower, - allowNameMatching: params.allowNameMatching, - userId: starter.userId, - userName: starterSenderName, - botId: starter.botId, - }); + (!starterIsCurrentBot && + isSlackThreadContextSenderAllowed({ + allowFromLower: params.allowFromLower, + allowNameMatching: params.allowNameMatching, + userId: starter.userId, + userName: starterSenderName, + botId: starter.botId, + })); const includeStarterContext = !starter || - shouldIncludeSupplementalContext({ - mode: params.contextVisibilityMode, - kind: "thread", - senderAllowed: starterAllowed, - }); + (!starterIsCurrentBot && + shouldIncludeSupplementalContext({ + mode: params.contextVisibilityMode, + kind: "thread", + senderAllowed: starterAllowed, + })); if (starter?.text && includeStarterContext) { threadStarterBody = starter.text; @@ -120,7 +135,9 @@ export async function resolveSlackThreadContextData(params: { } else { threadLabel = `Slack thread ${params.roomLabel}`; } - if (starter?.text && !includeStarterContext) { + if (starter?.text && starterIsCurrentBot) { + logVerbose("slack: omitted current-bot thread starter from context"); + } else if (starter?.text && !includeStarterContext) { logVerbose( `slack: omitted thread starter from context (mode=${params.contextVisibilityMode}, sender_allowed=${starterAllowed ? "yes" : "no"})`, ); @@ -142,9 +159,21 @@ export async function resolveSlackThreadContextData(params: { }); if (threadHistory.length > 0) { + const threadHistoryWithoutCurrentBot = threadHistory.filter( + (historyMsg) => + !isCurrentBotAuthor({ + userId: historyMsg.userId, + botId: historyMsg.botId, + }), + ); + const omittedCurrentBotHistoryCount = + threadHistory.length - threadHistoryWithoutCurrentBot.length; + const uniqueUserIds = [ ...new Set( - threadHistory.map((item) => item.userId).filter((id): id is string => Boolean(id)), + threadHistoryWithoutCurrentBot + .map((item) => item.userId) + .filter((id): id is string => Boolean(id)), ), ]; const userMap = new Map(); @@ -159,7 +188,7 @@ export async function resolveSlackThreadContextData(params: { const { items: filteredThreadHistory, omitted: omittedHistoryCount } = filterSupplementalContextItems({ - items: threadHistory, + items: threadHistoryWithoutCurrentBot, mode: params.contextVisibilityMode, kind: "thread", isSenderAllowed: (historyMsg) => { @@ -173,18 +202,17 @@ export async function resolveSlackThreadContextData(params: { }); }, }); - if (omittedHistoryCount > 0) { + if (omittedHistoryCount > 0 || omittedCurrentBotHistoryCount > 0) { logVerbose( - `slack: omitted ${omittedHistoryCount} thread message(s) from context (mode=${params.contextVisibilityMode})`, + `slack: omitted ${omittedHistoryCount + omittedCurrentBotHistoryCount} thread message(s) from context (mode=${params.contextVisibilityMode})`, ); } const historyParts: string[] = []; for (const historyMsg of filteredThreadHistory) { const msgUser = historyMsg.userId ? userMap.get(historyMsg.userId) : null; - const msgSenderName = - msgUser?.name ?? (historyMsg.botId ? `Bot (${historyMsg.botId})` : "Unknown"); const isBot = Boolean(historyMsg.botId); + const msgSenderName = msgUser?.name ?? (isBot ? `Bot (${historyMsg.botId})` : "Unknown"); const role = isBot ? "assistant" : "user"; const msgWithId = `${historyMsg.text}\n[slack message id: ${historyMsg.ts ?? "unknown"} channel: ${params.message.channel}]`; historyParts.push( diff --git a/extensions/slack/src/monitor/message-handler/prepare.test-helpers.ts b/extensions/slack/src/monitor/message-handler/prepare.test-helpers.ts index 1394957a2b5..c2fd3dcf3f2 100644 --- a/extensions/slack/src/monitor/message-handler/prepare.test-helpers.ts +++ b/extensions/slack/src/monitor/message-handler/prepare.test-helpers.ts @@ -23,6 +23,7 @@ export function createInboundSlackTestContext(params: { app: { client: params.appClient ?? {} } as App, runtime: {} as RuntimeEnv, botUserId: "B1", + botId: "B1", teamId: "T1", apiAppId: "A1", historyLimit: 0, diff --git a/extensions/slack/src/monitor/message-handler/prepare.test.ts b/extensions/slack/src/monitor/message-handler/prepare.test.ts index e096022cbff..fc141f6bd34 100644 --- a/extensions/slack/src/monitor/message-handler/prepare.test.ts +++ b/extensions/slack/src/monitor/message-handler/prepare.test.ts @@ -473,8 +473,8 @@ describe("slack prepareSlackMessage inbound contract", () => { expect(prepared).toBeTruthy(); expect(prepared!.ctxPayload.IsFirstThreadTurn).toBe(true); - expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("assistant reply"); expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("follow-up question"); + expect(prepared!.ctxPayload.ThreadHistoryBody).not.toContain("assistant reply"); expect(prepared!.ctxPayload.ThreadHistoryBody).not.toContain("current message"); expect(replies).toHaveBeenCalledTimes(2); }); diff --git a/extensions/slack/src/monitor/message-handler/prepare.thread-context-allowlist.test.ts b/extensions/slack/src/monitor/message-handler/prepare.thread-context-allowlist.test.ts index 58738094686..359447c811d 100644 --- a/extensions/slack/src/monitor/message-handler/prepare.thread-context-allowlist.test.ts +++ b/extensions/slack/src/monitor/message-handler/prepare.thread-context-allowlist.test.ts @@ -140,8 +140,8 @@ describe("prepareSlackMessage thread context allowlists", () => { expect(prepared).toBeTruthy(); expect(prepared!.ctxPayload.ThreadStarterBody).toBe("starter from room user"); expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("starter from room user"); - expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("assistant reply"); expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("allowed follow-up"); + expect(prepared!.ctxPayload.ThreadHistoryBody).not.toContain("assistant reply"); expect(prepared!.ctxPayload.ThreadHistoryBody).not.toContain("current message"); expect(replies).toHaveBeenCalledTimes(2); }); @@ -169,8 +169,8 @@ describe("prepareSlackMessage thread context allowlists", () => { expect(prepared).toBeTruthy(); expect(prepared!.ctxPayload.ThreadStarterBody).toBe("starter from open room"); expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("starter from open room"); - expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("assistant reply"); expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("open-room follow-up"); + expect(prepared!.ctxPayload.ThreadHistoryBody).not.toContain("assistant reply"); expect(prepared!.ctxPayload.ThreadHistoryBody).not.toContain("current message"); expect(replies).toHaveBeenCalledTimes(2); }); @@ -192,8 +192,8 @@ describe("prepareSlackMessage thread context allowlists", () => { expect(prepared).toBeTruthy(); expect(prepared!.ctxPayload.ThreadStarterBody).toBe("starter from open dm"); expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("starter from open dm"); - expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("assistant reply"); expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("dm follow-up"); + expect(prepared!.ctxPayload.ThreadHistoryBody).not.toContain("assistant reply"); expect(prepared!.ctxPayload.ThreadHistoryBody).not.toContain("current message"); expect(replies).toHaveBeenCalledTimes(2); }); @@ -215,8 +215,8 @@ describe("prepareSlackMessage thread context allowlists", () => { expect(prepared).toBeTruthy(); expect(prepared!.ctxPayload.ThreadStarterBody).toBe("starter from mpim"); expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("starter from mpim"); - expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("assistant reply"); expect(prepared!.ctxPayload.ThreadHistoryBody).toContain("mpim follow-up"); + expect(prepared!.ctxPayload.ThreadHistoryBody).not.toContain("assistant reply"); expect(prepared!.ctxPayload.ThreadHistoryBody).not.toContain("current message"); expect(replies).toHaveBeenCalledTimes(2); }); diff --git a/extensions/slack/src/monitor/monitor.test.ts b/extensions/slack/src/monitor/monitor.test.ts index 3a3a7ddd2a4..84defd823fe 100644 --- a/extensions/slack/src/monitor/monitor.test.ts +++ b/extensions/slack/src/monitor/monitor.test.ts @@ -113,6 +113,7 @@ const baseParams = () => ({ app: { client: {} } as App, runtime: {} as RuntimeEnv, botUserId: "B1", + botId: "B1", teamId: "T1", apiAppId: "A1", historyLimit: 0, diff --git a/extensions/slack/src/monitor/provider.ts b/extensions/slack/src/monitor/provider.ts index c2e3aeb4b70..52324ebd428 100644 --- a/extensions/slack/src/monitor/provider.ts +++ b/extensions/slack/src/monitor/provider.ts @@ -383,12 +383,14 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { let unregisterHttpHandler: (() => void) | null = null; let botUserId = ""; + let botId = ""; let teamId = ""; let apiAppId = ""; const expectedApiAppIdFromAppToken = parseApiAppIdFromAppToken(appToken); try { const auth = await app.client.auth.test({ token: botToken }); botUserId = auth.user_id ?? ""; + botId = (auth as { bot_id?: string }).bot_id ?? ""; teamId = auth.team_id ?? ""; apiAppId = (auth as { api_app_id?: string }).api_app_id ?? ""; } catch { @@ -408,6 +410,7 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { app, runtime, botUserId, + botId, teamId, apiAppId, historyLimit, diff --git a/src/auto-reply/reply/get-reply-run.media-only.test.ts b/src/auto-reply/reply/get-reply-run.media-only.test.ts index 422d3b1770d..46930bbf682 100644 --- a/src/auto-reply/reply/get-reply-run.media-only.test.ts +++ b/src/auto-reply/reply/get-reply-run.media-only.test.ts @@ -290,6 +290,117 @@ describe("runPreparedReply media-only handling", () => { expect(call?.followupRun.prompt).toContain("Earlier message in this thread"); }); + it("falls back to thread starter context on follow-up turns when history is absent", async () => { + const result = await runPreparedReply( + baseParams({ + isNewSession: false, + ctx: { + Body: "", + RawBody: "", + CommandBody: "", + ThreadStarterBody: "starter message", + ThreadHistoryBody: undefined, + OriginatingChannel: "slack", + OriginatingTo: "C123", + ChatType: "group", + }, + sessionCtx: { + Body: "", + BodyStripped: "", + ThreadStarterBody: "starter message", + ThreadHistoryBody: undefined, + MediaPath: "/tmp/input.png", + Provider: "slack", + ChatType: "group", + OriginatingChannel: "slack", + OriginatingTo: "C123", + }, + }), + ); + expect(result).toEqual({ text: "ok" }); + + const call = vi.mocked(runReplyAgent).mock.calls[0]?.[0]; + expect(call).toBeTruthy(); + expect(call?.followupRun.prompt).toContain("[Thread starter - for context]"); + expect(call?.followupRun.prompt).toContain("starter message"); + }); + + it("prefers thread history over thread starter on follow-up turns", async () => { + const result = await runPreparedReply( + baseParams({ + isNewSession: false, + ctx: { + Body: "", + RawBody: "", + CommandBody: "", + ThreadStarterBody: "starter message", + ThreadHistoryBody: "Earlier message in this thread", + OriginatingChannel: "slack", + OriginatingTo: "C123", + ChatType: "group", + }, + sessionCtx: { + Body: "", + BodyStripped: "", + ThreadStarterBody: "starter message", + ThreadHistoryBody: "Earlier message in this thread", + MediaPath: "/tmp/input.png", + Provider: "slack", + ChatType: "group", + OriginatingChannel: "slack", + OriginatingTo: "C123", + }, + }), + ); + expect(result).toEqual({ text: "ok" }); + + const call = vi.mocked(runReplyAgent).mock.calls[0]?.[0]; + expect(call).toBeTruthy(); + expect(call?.followupRun.prompt).toContain("[Thread history - for context]"); + expect(call?.followupRun.prompt).not.toContain("[Thread starter - for context]"); + }); + + it("does not duplicate thread starter text with a plain-text prelude", async () => { + vi.mocked(buildInboundUserContextPrefix).mockReturnValueOnce( + [ + "Thread starter (untrusted, for context):", + "```json", + '{"body":"starter message"}', + "```", + ].join("\n"), + ); + + const result = await runPreparedReply( + baseParams({ + ctx: { + Body: "", + RawBody: "", + CommandBody: "", + ThreadStarterBody: "starter message", + OriginatingChannel: "slack", + OriginatingTo: "C123", + ChatType: "group", + }, + sessionCtx: { + Body: "", + BodyStripped: "", + ThreadStarterBody: "starter message", + MediaPath: "/tmp/input.png", + Provider: "slack", + ChatType: "group", + OriginatingChannel: "slack", + OriginatingTo: "C123", + }, + }), + ); + expect(result).toEqual({ text: "ok" }); + + const call = vi.mocked(runReplyAgent).mock.calls[0]?.[0]; + expect(call).toBeTruthy(); + expect(call?.followupRun.prompt).toContain("Thread starter (untrusted, for context):"); + expect(call?.followupRun.prompt).not.toContain("[Thread starter - for context]"); + }); + it("returns the empty-body reply when there is no text and no media", async () => { const result = await runPreparedReply( baseParams({ diff --git a/src/auto-reply/reply/get-reply-run.ts b/src/auto-reply/reply/get-reply-run.ts index 9459e886a3e..f2cc3554f5b 100644 --- a/src/auto-reply/reply/get-reply-run.ts +++ b/src/auto-reply/reply/get-reply-run.ts @@ -437,7 +437,7 @@ export async function runPreparedReply( const threadHistoryBody = normalizeOptionalString(ctx.ThreadHistoryBody); const threadContextNote = threadHistoryBody ? `[Thread history - for context]\n${threadHistoryBody}` - : threadStarterBody + : !isNewSession && threadStarterBody ? `[Thread starter - for context]\n${threadStarterBody}` : undefined; const drainedSystemEventBlocks: string[] = [];