diff --git a/doc/connections/SLACK-TASK-TOOLS.md b/doc/connections/SLACK-TASK-TOOLS.md index 351cc8ef79e..68c4e573ed9 100644 --- a/doc/connections/SLACK-TASK-TOOLS.md +++ b/doc/connections/SLACK-TASK-TOOLS.md @@ -207,18 +207,18 @@ ordinary task finals are not automatically broadcast to Slack. ## Require an at-mention for inbound channel messages -Slack app settings include **Require at-mention** directly below **Allow direct -messages**. It defaults to on for new Slack connections. Existing connections +Slack app settings include **Require at-mention in channels** directly below +**Allow direct messages**. It defaults to on for new Slack connections. Existing connections keep their saved setting; the setting applies only to channels. -When enabled, each inbound channel message must explicitly @mention the connected -bot, including replies in existing channel threads. Direct messages never require +When enabled, each top-level channel message must explicitly @mention the connected +bot. Replies in existing channel threads do not require an @mention. Direct messages never require an @mention, but must be enabled with **Allow direct messages** and pass the usual sender authorization checks. Top-level DMs remain a linear conversation with top-level replies; use a private channel for conversations organized into threads. -Unmentioned channel messages do not create or continue a task. Queued messages +Unmentioned top-level channel messages do not create a task. Queued messages are checked again before starting agent work. If a queued run combines several -channel messages, an unmentioned input cancels that queued run when this setting -is enabled. Send a new mention to start it again. Unmentioned DM inputs do not -trigger this cancellation. +channel messages, an unmentioned top-level input cancels that queued run when this setting +is enabled. Send a new mention to start it again. Unmentioned thread replies and +DM inputs do not trigger this cancellation. This setting does not cancel work that has already started or revoke its Slack tool access. diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index 909f9bd60a8..edee564c8b5 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -11666,7 +11666,7 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { await expect(resolveSlackTaskAuthority(db, binding)).resolves.toMatchObject({ slackUserId: "UBOARD" }); }); - it.each([false, true])("applies Slack mention settings to channel replies while exempting DMs (DM=%s)", async (isDM) => { + it.each([false, true])("allows Slack thread replies without mentions while exempting DMs (DM=%s)", async (isDM) => { const fixture = await seedCompany(); const { callbacks, endpoint, service, wakeup } = await configuredSlackEndpoint(fixture); expect((await service.get(endpoint.id)).requireAtMention).toBe(false); @@ -11680,29 +11680,23 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { expect(wakeup).toHaveBeenCalledTimes(2); expect((await service.update(endpoint.id, { requireAtMention: true })).requireAtMention).toBe(true); await send("7100.3", false, "subscribed_message"); - expect(wakeup).toHaveBeenCalledTimes(isDM ? 3 : 2); + expect(wakeup).toHaveBeenCalledTimes(3); const [filtered] = await db.select().from(chatDeliveries).where(and( eq(chatDeliveries.endpointId, endpoint.id), eq(chatDeliveries.state, "filtered"))); - if (isDM) { - expect(filtered).toBeUndefined(); - } else { - expect(filtered.redactedError).toBe("Message did not @mention the bot"); - expect(filtered.normalizedEvent).toMatchObject({ filtering: { contentRetained: false } }); - expect(filtered.normalizedEvent).not.toHaveProperty("message.text"); - } + expect(filtered).toBeUndefined(); // Both Slack callback kinds can carry an explicit mention. Retries cannot // produce a second wakeup for the same provider message. await send("7100.4", true); await send("7100.4", true, "mention"); - expect(wakeup).toHaveBeenCalledTimes(isDM ? 4 : 3); + expect(wakeup).toHaveBeenCalledTimes(4); const conversations = await readChatConversationsByEndpointId(endpoint.id); expect(conversations).toHaveLength(1); const comments = await readIssueCommentsByIssueId(conversations[0].issueId); - expect(comments).toHaveLength(isDM ? 4 : 3); + expect(comments).toHaveLength(4); expect(conversations[0].externalThreadId).toBe(thread.thread.id); await service.update(endpoint.id, { requireAtMention: false }); await send("7100.5", false); - expect(wakeup).toHaveBeenCalledTimes(isDM ? 5 : 4); + expect(wakeup).toHaveBeenCalledTimes(5); if (isDM) { await service.update(endpoint.id, { requireAtMention: true, allowDirectMessages: false }); await send("7100.6", false); @@ -11710,7 +11704,11 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { } }); - it.each([false, true])("rechecks queued Slack channel mentions but exempts DMs before drain (DM=%s)", async (isDM) => { + it.each([ + { isDM: false, isReply: false }, + { isDM: false, isReply: true }, + { isDM: true, isReply: false }, + ])("rechecks queued Slack channel roots but exempts replies and DMs (DM=$isDM, reply=$isReply)", async ({ isDM, isReply }) => { const fixture = await seedCompany(); const { callbacks, endpoint, service, wakeup } = await configuredSlackEndpoint(fixture, { deferWebhookProcessing: true, scheduleDeferredWork: () => undefined, @@ -11718,14 +11716,28 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { const channelId = isDM ? "D-QUEUED-MENTION" : "C-QUEUED-MENTION"; const thread = makeThread({ channelId, id: `slack:${channelId}:${isDM ? "" : "7300.1"}`, isDM }); await deliverMessage({ callbacks, endpointId: endpoint.id, thread: thread.thread, - message: makeMessage({ id: "7300.1", text: "Start work" }), trigger: isDM ? "direct_message" : "subscribed_message" }); + message: makeMessage({ id: "7300.1", text: "@maya start", mentioned: true }), trigger: isDM ? "direct_message" : "mention" }); + const [starter] = await readChatDeliveriesByEndpointId(endpoint.id); + await db.update(chatDeliveries).set({ nextAttemptAt: new Date(0) }).where(eq(chatDeliveries.id, starter.id)); + await service.processPendingDeliveries(25, starter.id); + expect(wakeup).toHaveBeenCalledTimes(1); + const nextThread = isReply || isDM ? thread.thread : makeThread({ channelId, id: `slack:${channelId}:7300.2` }).thread; + await deliverMessage({ callbacks, endpointId: endpoint.id, thread: nextThread, + message: makeMessage({ id: "7300.2", text: "Continue work" }), trigger: isDM ? "direct_message" : "subscribed_message" }); await service.update(endpoint.id, { requireAtMention: true }); - const [delivery] = await readChatDeliveriesByEndpointId(endpoint.id); + const delivery = (await readChatDeliveriesByEndpointId(endpoint.id)).find(row => row.id !== starter.id)!; await db.update(chatDeliveries).set({ nextAttemptAt: new Date(0) }).where(eq(chatDeliveries.id, delivery.id)); await service.processPendingDeliveries(25, delivery.id); - expect(wakeup).toHaveBeenCalledTimes(isDM ? 1 : 0); - expect(await readChatConversationsByEndpointId(endpoint.id)).toHaveLength(isDM ? 1 : 0); - expect((await readChatDeliveriesById(delivery.id))[0].state).toBe(isDM ? "processed" : "filtered"); + const allowed = isDM || isReply; + expect(wakeup).toHaveBeenCalledTimes(allowed ? 2 : 1); + expect(await readChatConversationsByEndpointId(endpoint.id)).toHaveLength(1); + const [result] = await readChatDeliveriesById(delivery.id); + expect(result.state).toBe(allowed ? "processed" : "filtered"); + if (!allowed) { + expect(result.redactedError).toBe("Message did not @mention the bot"); + expect(result.normalizedEvent).toMatchObject({ filtering: { contentRetained: false } }); + expect(result.normalizedEvent).not.toHaveProperty("message.text"); + } }); it("snapshots channel workspace defaults on new tasks and preserves them on replies", async () => { diff --git a/server/src/__tests__/durable-chat-wakeup.test.ts b/server/src/__tests__/durable-chat-wakeup.test.ts index ceb16cec24d..6168d8eee74 100644 --- a/server/src/__tests__/durable-chat-wakeup.test.ts +++ b/server/src/__tests__/durable-chat-wakeup.test.ts @@ -197,11 +197,11 @@ describe("durable inbound chat scheduler receipts", () => { } it.each([ - ...["unmentioned", "mentioned", "coalesced", "recovery", "deferred"].map((scenario) => ({ scenario, isDM: false })), + ...["unmentioned", "mentioned", "reply", "coalesced_reply", "coalesced", "recovery", "deferred"].map((scenario) => ({ scenario, isDM: false })), ...["unmentioned", "coalesced"].map((scenario) => ({ scenario, isDM: true })), ])("checks Slack channel mentions after a real heartbeat queue wait ($scenario, DM=$isDM)", async ({ scenario, isDM }) => { const mentioned = scenario === "mentioned" || scenario === "coalesced"; - const allowed = isDM || scenario === "mentioned" || scenario === "recovery"; + const allowed = isDM || scenario === "mentioned" || scenario === "recovery" || scenario === "reply" || scenario === "coalesced_reply"; const f = await fixture(); await db.update(agents).set({ adapterType: "durable_chat_retry_test" }).where(eq(agents.id, f.agentId)); const applicationId = randomUUID(), connectionId = randomUUID(), endpointId = randomUUID(); @@ -215,7 +215,7 @@ describe("durable inbound chat scheduler receipts", () => { authorUserId: "board-user", body: mentioned ? "@bot work" : "Work" }); const [delivery] = await db.insert(chatDeliveries).values({ companyId: f.companyId, endpointId, providerEventId: randomUUID(), deduplicationKey: randomUUID(), eventKind: isDM ? "direct_message" : "message", state: "processed", - normalizedEvent: { trigger: isDM ? "direct_message" : "subscribed_message", conversation: { isDirectMessage: isDM }, message: { mentionedBot: mentioned } } }).returning(); + normalizedEvent: { trigger: isDM ? "direct_message" : "subscribed_message", conversation: { isDirectMessage: isDM, externalThreadId: "slack:C-MENTION:7300.1" }, message: { providerMessageId: scenario === "reply" || scenario === "coalesced_reply" ? "7300.2" : "7300.1", mentionedBot: mentioned } } }).returning(); await db.insert(chatActions).values({ id: request.id, companyId: f.companyId, endpointId, deliveryId: delivery.id, kind: "inbound_wakeup", providerActionId: randomUUID(), status: "processed", payload: { issueId: f.issueId, commentId: request.commentId } }); @@ -231,13 +231,13 @@ describe("durable inbound chat scheduler receipts", () => { .where(eq(heartbeatRuns.id, receipt.runId!)); } let deferredRequestId: string | undefined; - if (scenario === "coalesced" || scenario === "deferred") { + if (scenario === "coalesced" || scenario === "coalesced_reply" || scenario === "deferred") { const second = f.request(); await db.insert(issueComments).values({ id: second.commentId, companyId: f.companyId, issueId: f.issueId, authorUserId: "board-user", body: scenario === "deferred" ? "@bot follow-up" : "Unmentioned follow-up" }); const [secondDelivery] = await db.insert(chatDeliveries).values({ companyId: f.companyId, endpointId, providerEventId: randomUUID(), deduplicationKey: randomUUID(), eventKind: isDM ? "direct_message" : "message", state: "processed", - normalizedEvent: { trigger: isDM ? "direct_message" : "subscribed_message", conversation: { isDirectMessage: isDM }, message: { mentionedBot: scenario === "deferred" } } }).returning(); + normalizedEvent: { trigger: isDM ? "direct_message" : "subscribed_message", conversation: { isDirectMessage: isDM, externalThreadId: "slack:C-MENTION:7300.1" }, message: { providerMessageId: scenario === "coalesced_reply" ? "7300.3" : "7300.1", mentionedBot: scenario === "deferred" } } }).returning(); await db.insert(chatActions).values({ id: second.id, companyId: f.companyId, endpointId, deliveryId: secondDelivery.id, kind: "inbound_wakeup", providerActionId: randomUUID(), status: "processed", payload: { issueId: f.issueId, commentId: second.commentId } }); diff --git a/server/src/services/chat-channels/inbound/admission.ts b/server/src/services/chat-channels/inbound/admission.ts index 34329d81557..e2638a83771 100644 --- a/server/src/services/chat-channels/inbound/admission.ts +++ b/server/src/services/chat-channels/inbound/admission.ts @@ -1,3 +1,4 @@ +import { slackMessageMeetsMentionRequirement } from "../slack/messages.js"; import { createTeamsDestinationAdmission } from "../microsoft-teams/destination-admission.js"; import { createTelegramDestinationAdmission } from "../telegram/destination-admission.js"; import { createPhotonDestinationAdmission } from "../imessage-photon/destination-admission.js"; @@ -167,10 +168,7 @@ export function createInboundAdmission(db: Db, options: InboundAdmissionOptions) } const mentionRequired = endpoint.provider === "slack" && - currentEndpoint.requireAtMention && - !thread.isDM && - trigger !== "mention" && - message.isMention !== true; + !slackMessageMeetsMentionRequirement(currentEndpoint, thread, trigger, message); // GitHub sends the same comment to every installed App. Subscribed // conversations are a fallback for unaddressed replies, not permission // to wake every bot when the person addresses a different recipient. diff --git a/server/src/services/chat-channels/slack/messages.ts b/server/src/services/chat-channels/slack/messages.ts index 36196eab18f..6981d2650cc 100644 --- a/server/src/services/chat-channels/slack/messages.ts +++ b/server/src/services/chat-channels/slack/messages.ts @@ -106,9 +106,15 @@ export function stripSlackTitleMentions(text: string): string { export function slackMessageMeetsMentionRequirement( endpoint: Pick, - thread: Pick, + thread: Pick, trigger: ChatSdkMessageCallbackEvent["trigger"], - message: Pick, + message: Pick, ): boolean { - return !endpoint.requireAtMention || thread.isDM || trigger === "mention" || message.isMention === true; + return ( + !endpoint.requireAtMention || + thread.isDM || + (Boolean(message.id) && slackOrphanFollowUp(thread.id, message.id)) || + trigger === "mention" || + message.isMention === true + ); } diff --git a/server/src/services/chat-channels/slack/wake-authority.test.ts b/server/src/services/chat-channels/slack/wake-authority.test.ts new file mode 100644 index 00000000000..1fb2f11467f --- /dev/null +++ b/server/src/services/chat-channels/slack/wake-authority.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from "vitest"; +import { slackDeliveryMeetsMentionRequirement } from "./wake-authority.js"; + +describe("Slack channel mention policy", () => { + const root = { + trigger: "subscribed_message", + conversation: { externalThreadId: "slack:C123:7300.000001", isDirectMessage: false }, + message: { providerMessageId: "7300.000001", mentionedBot: false }, + }; + + it("requires a mention on a channel root, even for a subscribed callback", () => { + expect(slackDeliveryMeetsMentionRequirement({ requireAtMention: true }, root)).toBe(false); + expect(slackDeliveryMeetsMentionRequirement({ requireAtMention: false }, root)).toBe(true); + }); + + it("allows replies with a known message ID in a channel thread", () => { + expect(slackDeliveryMeetsMentionRequirement({ requireAtMention: true }, { + ...root, message: { ...root.message, providerMessageId: "7300.000002" }, + })).toBe(true); + }); + + it.each([ + {}, + { ...root, message: {} }, + { ...root, message: { providerMessageId: "" } }, + { ...root, message: { providerMessageId: 7300 } }, + { ...root, conversation: {} }, + { ...root, conversation: { externalThreadId: "slack:C123:" } }, + { ...root, conversation: { externalThreadId: "slack:C123" } }, + { ...root, conversation: { externalThreadId: "discord:C123:7300.000001" } }, + ])("does not infer a thread reply from incomplete or invalid identity (%j)", (event) => { + expect(slackDeliveryMeetsMentionRequirement({ requireAtMention: true }, event)).toBe(false); + }); + + it("accepts either explicit mention callback", () => { + expect(slackDeliveryMeetsMentionRequirement({ requireAtMention: true }, { ...root, trigger: "mention" })).toBe(true); + expect(slackDeliveryMeetsMentionRequirement({ requireAtMention: true }, { + ...root, message: { ...root.message, mentionedBot: true }, + })).toBe(true); + }); + + it("exempts DMs and uses the current conversation type when it is supplied", () => { + const directMessage = { ...root, conversation: { ...root.conversation, isDirectMessage: true } }; + expect(slackDeliveryMeetsMentionRequirement({ requireAtMention: true }, directMessage)).toBe(true); + expect(slackDeliveryMeetsMentionRequirement({ requireAtMention: true }, root, true)).toBe(true); + expect(slackDeliveryMeetsMentionRequirement({ requireAtMention: true }, directMessage, false)).toBe(false); + }); +}); diff --git a/server/src/services/chat-channels/slack/wake-authority.ts b/server/src/services/chat-channels/slack/wake-authority.ts index e09a7ff93a5..79662476879 100644 --- a/server/src/services/chat-channels/slack/wake-authority.ts +++ b/server/src/services/chat-channels/slack/wake-authority.ts @@ -1,14 +1,44 @@ import { forbidden } from "../../../errors.js"; +import { slackMessageMeetsMentionRequirement } from "./messages.js"; import type { EndpointRow } from "../runtime-types.js"; import type { chatConversations, chatDeliveries } from "@paperclipai/db"; -export function assertSlackWakeMention(endpoint: EndpointRow, conversation: Pick, delivery: Pick): void { - if ( - endpoint.requireAtMention && - !conversation.isDirectMessage && - delivery.normalizedEvent.trigger !== "mention" && - (delivery.normalizedEvent.message as { mentionedBot?: boolean } | undefined)?.mentionedBot !== true - ) { + +type SlackDeliveryEvent = typeof chatDeliveries.$inferSelect["normalizedEvent"]; + +export function slackDeliveryMeetsMentionRequirement( + endpoint: Pick, + event: SlackDeliveryEvent, + isDirectMessage?: boolean, +): boolean { + const conversation = event.conversation as { + externalThreadId?: unknown; + isDirectMessage?: boolean; + } | undefined; + const message = event.message as { + providerMessageId?: unknown; + mentionedBot?: boolean; + } | undefined; + return slackMessageMeetsMentionRequirement( + endpoint, + { + id: typeof conversation?.externalThreadId === "string" ? conversation.externalThreadId : "", + isDM: isDirectMessage ?? conversation?.isDirectMessage === true, + }, + event.trigger === "mention" ? "mention" : "subscribed_message", + { + id: typeof message?.providerMessageId === "string" ? message.providerMessageId : "", + isMention: message?.mentionedBot === true, + }, + ); +} + +export function assertSlackWakeMention( + endpoint: EndpointRow, + conversation: Pick, + delivery: Pick, +): void { + if (!slackDeliveryMeetsMentionRequirement(endpoint, delivery.normalizedEvent, conversation.isDirectMessage)) { throw forbidden("Message did not @mention the bot", { code: "chat_action_authorization_changed", }); diff --git a/server/src/services/durable-chat-wakeup.ts b/server/src/services/durable-chat-wakeup.ts index 367386ffaca..acfb4b4ca63 100644 --- a/server/src/services/durable-chat-wakeup.ts +++ b/server/src/services/durable-chat-wakeup.ts @@ -1,3 +1,4 @@ +import { slackDeliveryMeetsMentionRequirement } from "./chat-channels/slack/wake-authority.js"; import { createHash } from "node:crypto"; import { agentWakeupRequests, chatActions, chatConversations, chatDeliveries, chatEndpoints, type Db } from "@paperclipai/db"; import { and, eq, exists, inArray, or, sql, type SQLWrapper } from "drizzle-orm"; @@ -410,7 +411,5 @@ export async function slackMentionAllowsRunStart( ), )); return inputs.every(({ event }) => - (event.conversation as { isDirectMessage?: boolean } | undefined)?.isDirectMessage === true || - event.trigger === "mention" || - (event.message as { mentionedBot?: boolean } | undefined)?.mentionedBot === true); + slackDeliveryMeetsMentionRequirement({ requireAtMention: true }, event)); } diff --git a/ui/src/pages/apps/chat/ChatEndpointDetail.tsx b/ui/src/pages/apps/chat/ChatEndpointDetail.tsx index 2560e7bee1c..476173a6cb9 100644 --- a/ui/src/pages/apps/chat/ChatEndpointDetail.tsx +++ b/ui/src/pages/apps/chat/ChatEndpointDetail.tsx @@ -621,8 +621,8 @@ function Settings({ /> {endpoint.provider === "slack" && (