diff --git a/src/Assistant.ts b/src/Assistant.ts index 687d481de..c5220a2a3 100644 --- a/src/Assistant.ts +++ b/src/Assistant.ts @@ -1,4 +1,5 @@ import type { + AllMessageEvents, AssistantThreadsSetStatusResponse, AssistantThreadsSetSuggestedPromptsResponse, AssistantThreadsSetTitleResponse, @@ -11,6 +12,7 @@ import { DefaultThreadContextStore, } from './AssistantThreadContextStore'; import { AssistantInitializationError, AssistantMissingPropertyError } from './errors'; +import { autoAcknowledge, isEventArgs, isMessageEventArgs, safelyAcknowledge } from './middleware/builtin'; import processMiddleware from './middleware/process'; import type { AllMiddlewareArgs, AnyMiddlewareArgs, Middleware, SayFn, SlackEventMiddlewareArgs } from './types'; @@ -92,9 +94,6 @@ export interface AssistantUserMessageMiddlewareArgs export type AllAssistantMiddlewareArgs = T & AllMiddlewareArgs; -/** Constants */ -const ASSISTANT_PAYLOAD_TYPES = new Set(['assistant_thread_started', 'assistant_thread_context_changed', 'message']); - export class Assistant { private threadContextStore: AssistantThreadContextStore; @@ -129,34 +128,26 @@ export class Assistant { public getMiddleware(): Middleware { return async (args): Promise => { - if (isAssistantEvent(args) && matchesConstraints(args)) { - return this.processEvent(args); + if (isAssistantThreadStartedEvent(args)) { + return this.processEvent(args, [autoAcknowledge, ...this.threadStarted]); + } + if (isAssistantThreadContextChangedEvent(args)) { + return this.processEvent(args, [autoAcknowledge, ...this.threadContextChanged]); + } + if (isUserMessageEventInAssistantThread(args)) { + return this.processEvent(args, [autoAcknowledge, ...this.userMessage]); + } + if (isOtherMessageSubEventInAssistantThread(args)) { + // ignore self message_changed, message_deleted, etc. + return await safelyAcknowledge(args); } return args.next(); }; } - private async processEvent(args: AllAssistantMiddlewareArgs): Promise { - const { payload } = args; + private async processEvent(args: AllAssistantMiddlewareArgs, middlewares: AssistantMiddleware): Promise { const assistantArgs = enrichAssistantArgs(this.threadContextStore, args); - const assistantMiddleware = this.getAssistantMiddleware(payload); - return processAssistantMiddleware(assistantArgs, assistantMiddleware); - } - - /** - * `getAssistantMiddleware()` returns the Assistant instance's middleware - */ - private getAssistantMiddleware(payload: AllAssistantMiddlewareArgs['payload']): AssistantMiddleware { - switch (payload.type) { - case 'assistant_thread_started': - return this.threadStarted; - case 'assistant_thread_context_changed': - return this.threadContextChanged; - case 'message': - return this.userMessage; - default: - return []; - } + return processAssistantMiddleware(assistantArgs, middlewares); } } @@ -184,33 +175,60 @@ export function enrichAssistantArgs( return preparedArgs; } -/** - * `isAssistantEvent()` determines if incoming event is a supported - * Assistant event type. - */ -export function isAssistantEvent(args: AnyMiddlewareArgs): args is AllAssistantMiddlewareArgs { - return ASSISTANT_PAYLOAD_TYPES.has(args.payload.type); +export function isAssistantThreadStartedEvent(args: AnyMiddlewareArgs): args is AllAssistantMiddlewareArgs { + if (isEventArgs(args)) { + return args.payload.type === 'assistant_thread_started'; + } + return false; } -/** - * `matchesConstraints()` determines if the incoming event payload - * is related to the Assistant. - */ -export function matchesConstraints(args: AssistantMiddlewareArgs): args is AssistantMiddlewareArgs { - return args.payload.type === 'message' ? isAssistantMessage(args.payload) : true; +export function isAssistantThreadContextChangedEvent(args: AnyMiddlewareArgs): args is AllAssistantMiddlewareArgs { + if (isEventArgs(args)) { + return args.payload.type === 'assistant_thread_context_changed'; + } + return false; } -/** - * `isAssistantMessage()` evaluates if the message payload is associated - * with the Assistant container. - */ -export function isAssistantMessage(payload: AnyMiddlewareArgs['payload']): boolean { - const isThreadMessage = 'channel' in payload && 'thread_ts' in payload; - const inAssistantContainer = - 'channel_type' in payload && - payload.channel_type === 'im' && - (!('subtype' in payload) || payload.subtype === 'file_share' || payload.subtype === undefined); // TODO: undefined subtype is a limitation of message event, needs fixing (see https://github.com/slackapi/node-slack-sdk/issues/1904) - return isThreadMessage && inAssistantContainer; +export function isMessageEventInAssistantThread(args: AnyMiddlewareArgs): boolean { + if (isMessageEventArgs(args)) { + return args.event.channel_type === 'im'; + } + return false; +} + +export function isUserMessageEventInAssistantThread(args: AnyMiddlewareArgs): args is AllAssistantMiddlewareArgs { + if (isMessageEventInAssistantThread(args)) { + return ( + (!('subtype' in args.payload) || [undefined, 'file_share'].includes(args.payload.subtype)) && + 'thread_ts' in args.payload && + args.payload.thread_ts !== undefined && + !('bot_id' in args.payload) + ); + } + return false; +} + +export function isOtherMessageSubEventInAssistantThread(args: AnyMiddlewareArgs): boolean { + // message_changed, message_deleted etc. + if (isMessageEventInAssistantThread(args)) { + return ( + !isUserMessageEventInAssistantThread(args) && + (('message' in args.payload && isOtherMessageSubEvent(args.payload.message)) || + ('previous_message' in args.payload && isOtherMessageSubEvent(args.payload.previous_message))) + ); + } + return false; +} + +function isOtherMessageSubEvent(message: AllMessageEvents | (AllMessageEvents & { thread_ts: string })): boolean { + if ('thread_ts' in message) { + return true; + } + if ('subtype' in message) { + // TODO: add assistant_app_thread as a valid message event subtype + return (message.subtype as string) === 'assistant_app_thread'; + } + return false; } /** diff --git a/src/middleware/builtin.ts b/src/middleware/builtin.ts index 633730e09..0424d2ee8 100644 --- a/src/middleware/builtin.ts +++ b/src/middleware/builtin.ts @@ -56,11 +56,11 @@ function isViewBody( return 'view' in body && body.view !== undefined; } -function isEventArgs(args: AnyMiddlewareArgs): args is SlackEventMiddlewareArgs { +export function isEventArgs(args: AnyMiddlewareArgs): args is SlackEventMiddlewareArgs { return 'event' in args && args.event !== undefined; } -function isMessageEventArgs(args: AnyMiddlewareArgs): args is SlackEventMiddlewareArgs<'message'> { +export function isMessageEventArgs(args: AnyMiddlewareArgs): args is SlackEventMiddlewareArgs<'message'> { return isEventArgs(args) && 'message' in args; } @@ -131,10 +131,14 @@ export const onlyViewActions: Middleware = async (args) => { * Middleware that auto acknowledges the request received */ export const autoAcknowledge: Middleware = async (args) => { + await safelyAcknowledge(args); + await args.next(); +}; + +export const safelyAcknowledge: Middleware = async (args) => { if ('ack' in args && args.ack !== undefined) { await args.ack(); } - await args.next(); }; /** @@ -318,6 +322,7 @@ export const ignoreSelf: Middleware = async (args) => { const { message } = args; // Look for an event that is identified as a bot message from the same bot ID as this app, and return to skip if (message.subtype === 'bot_message' && message.bot_id === botId) { + await safelyAcknowledge(args); return; } } @@ -332,6 +337,7 @@ export const ignoreSelf: Middleware = async (args) => { args.event.user === botUserId && !eventsWhichShouldBeKept.includes(args.event.type) ) { + await safelyAcknowledge(args); return; } } diff --git a/test/unit/Assistant.spec.ts b/test/unit/Assistant.spec.ts index 40dc4b24f..25da0ca56 100644 --- a/test/unit/Assistant.spec.ts +++ b/test/unit/Assistant.spec.ts @@ -8,17 +8,19 @@ import { Assistant, type AssistantConfig, type AssistantMiddleware, - type AssistantMiddlewareArgs, } from '../../src/Assistant'; import type { AssistantThreadContext, AssistantThreadContextStore } from '../../src/AssistantThreadContextStore'; import { AssistantInitializationError, AssistantMissingPropertyError } from '../../src/errors'; +import { autoAcknowledge } from '../../src/middleware/builtin'; import type { Middleware } from '../../src/types'; import { type Override, createDummyAppMentionEventMiddlewareArgs, + createDummyAssistantMessageChangedEventMiddlewareArgs, createDummyAssistantThreadContextChangedEventMiddlewareArgs, createDummyAssistantThreadStartedEventMiddlewareArgs, createDummyAssistantUserMessageEventMiddlewareArgs, + createDummyMessageChangedEventMiddlewareArgs, createDummyMessageEventMiddlewareArgs, wrapMiddleware, } from './helpers'; @@ -101,83 +103,138 @@ describe('Assistant class', () => { const middleware = assistant.getMiddleware(); const fakeMessageArgs = wrapMiddleware(createDummyMessageEventMiddlewareArgs()); await middleware(fakeMessageArgs); + sinon.assert.notCalled(fakeMessageArgs.ack); sinon.assert.called(fakeMessageArgs.next); }); - it('should not call next if a assistant event', async () => { + it('should not call next if assistant_thread_started_event', async () => { const assistant = new Assistant(MOCK_CONFIG_SINGLE); const middleware = assistant.getMiddleware(); const mockThreadStartedArgs = wrapMiddleware(createDummyAssistantThreadStartedEventMiddlewareArgs()); await middleware(mockThreadStartedArgs); + sinon.assert.called(mockThreadStartedArgs.ack); sinon.assert.notCalled(mockThreadStartedArgs.next); }); - describe('isAssistantEvent', () => { - it('should return true if recognized assistant event', async () => { - const mockThreadStartedArgs = wrapMiddleware(createDummyAssistantThreadStartedEventMiddlewareArgs()); + it('should not call next if assistant_thread_context_changed_event', async () => { + const assistant = new Assistant(MOCK_CONFIG_SINGLE); + const middleware = assistant.getMiddleware(); + const mockThreadStartedArgs = wrapMiddleware(createDummyAssistantThreadContextChangedEventMiddlewareArgs()); + await middleware(mockThreadStartedArgs); + sinon.assert.called(mockThreadStartedArgs.ack); + sinon.assert.notCalled(mockThreadStartedArgs.next); + }); + + it('should not call next if assistant_message_event', async () => { + const assistant = new Assistant(MOCK_CONFIG_SINGLE); + const middleware = assistant.getMiddleware(); + const mockThreadStartedArgs = wrapMiddleware(createDummyAssistantUserMessageEventMiddlewareArgs()); + await middleware(mockThreadStartedArgs); + sinon.assert.called(mockThreadStartedArgs.ack); + sinon.assert.notCalled(mockThreadStartedArgs.next); + }); + + it('should not call next if message changed by assistant event', async () => { + const assistant = new Assistant(MOCK_CONFIG_SINGLE); + const middleware = assistant.getMiddleware(); + const mockThreadStartedArgs = wrapMiddleware(createDummyAssistantMessageChangedEventMiddlewareArgs()); + await middleware(mockThreadStartedArgs); + sinon.assert.called(mockThreadStartedArgs.ack); + sinon.assert.notCalled(mockThreadStartedArgs.next); + }); + + describe('isAssistantThreadContextChangedEvent', () => { + it('should return true if recognized assistant_thread_context_changed_event', async () => { const mockThreadContextChangedArgs = wrapMiddleware( createDummyAssistantThreadContextChangedEventMiddlewareArgs(), ); - const mockUserMessageArgs = wrapMiddleware(createDummyAssistantUserMessageEventMiddlewareArgs()); - - const { isAssistantEvent } = await importAssistant(); - - assert(isAssistantEvent(mockThreadStartedArgs)); - assert(isAssistantEvent(mockThreadContextChangedArgs)); - assert(isAssistantEvent(mockUserMessageArgs)); + const { isAssistantThreadContextChangedEvent } = await importAssistant(); + assert(isAssistantThreadContextChangedEvent(mockThreadContextChangedArgs)); }); - it('should return false if not a recognized assistant event', async () => { + it('should return false if not a recognized assistant_thread_context_changed_event', async () => { const fakeMessageArgs = wrapMiddleware(createDummyAppMentionEventMiddlewareArgs()); - const { isAssistantEvent } = await importAssistant(); - assert.isFalse(isAssistantEvent(fakeMessageArgs)); + const { isAssistantThreadContextChangedEvent } = await importAssistant(); + assert.isFalse(isAssistantThreadContextChangedEvent(fakeMessageArgs)); }); }); - describe('matchesConstraints', () => { - it('should return true if recognized assistant message', async () => { - const mockUserMessageArgs = wrapMiddleware(createDummyAssistantUserMessageEventMiddlewareArgs()); - const { matchesConstraints } = await importAssistant(); - assert.ok(matchesConstraints(mockUserMessageArgs)); - }); + describe('isAssistantThreadStartedEvent', () => { + it('should return true if recognized assistant_thread_started_event', async () => { + const mockThreadStartedArgs = wrapMiddleware(createDummyAssistantThreadStartedEventMiddlewareArgs()); + const { isAssistantThreadStartedEvent } = await importAssistant(); - it('should return false if not supported message subtype', async () => { - const fakeMessageArgs = wrapMiddleware(createDummyMessageEventMiddlewareArgs()); - const { matchesConstraints } = await importAssistant(); - // casting here as we intentionally are providing type-mismatched argument as a runtime test - assert.isFalse(matchesConstraints(fakeMessageArgs as unknown as AssistantMiddlewareArgs)); + assert(isAssistantThreadStartedEvent(mockThreadStartedArgs)); }); - it('should return true if not message event', async () => { - const mockThreadStartedArgs = wrapMiddleware(createDummyAssistantThreadStartedEventMiddlewareArgs()); - const { matchesConstraints } = await importAssistant(); - assert(matchesConstraints(mockThreadStartedArgs)); + it('should return false if not a recognized assistant_thread_started_event', async () => { + const fakeMessageArgs = wrapMiddleware(createDummyAppMentionEventMiddlewareArgs()); + const { isAssistantThreadStartedEvent } = await importAssistant(); + assert.isFalse(isAssistantThreadStartedEvent(fakeMessageArgs)); }); }); - describe('isAssistantMessage', () => { - it('should return true if assistant message event', async () => { + describe('isUserMessageEventInAssistantThread', () => { + it('should return true if assistant_message_event', async () => { const mockUserMessageArgs = wrapMiddleware(createDummyAssistantUserMessageEventMiddlewareArgs()); - const { isAssistantMessage } = await importAssistant(); - assert(isAssistantMessage(mockUserMessageArgs.payload)); + const { isUserMessageEventInAssistantThread } = await importAssistant(); + assert(isUserMessageEventInAssistantThread(mockUserMessageArgs)); }); it('should return false if not correct subtype', async () => { const fakeMessageArgs = wrapMiddleware(createDummyMessageEventMiddlewareArgs({ thread_ts: '1234.56' })); - const { isAssistantMessage } = await importAssistant(); - assert.isFalse(isAssistantMessage(fakeMessageArgs.payload)); + const { isUserMessageEventInAssistantThread } = await importAssistant(); + assert.isFalse(isUserMessageEventInAssistantThread(fakeMessageArgs)); }); it('should return false if thread_ts is missing', async () => { const fakeMessageArgs = wrapMiddleware(createDummyMessageEventMiddlewareArgs()); - const { isAssistantMessage } = await importAssistant(); - assert.isFalse(isAssistantMessage(fakeMessageArgs.payload)); + const { isUserMessageEventInAssistantThread } = await importAssistant(); + assert.isFalse(isUserMessageEventInAssistantThread(fakeMessageArgs)); }); it('should return false if channel_type is incorrect', async () => { const fakeMessageArgs = wrapMiddleware(createDummyMessageEventMiddlewareArgs({ channel_type: 'mpim' })); - const { isAssistantMessage } = await importAssistant(); - assert.isFalse(isAssistantMessage(fakeMessageArgs.payload)); + const { isUserMessageEventInAssistantThread } = await importAssistant(); + assert.isFalse(isUserMessageEventInAssistantThread(fakeMessageArgs)); + }); + + it('should return false if not message event', async () => { + const mockThreadStartedArgs = wrapMiddleware(createDummyAssistantThreadStartedEventMiddlewareArgs()); + const { isUserMessageEventInAssistantThread } = await importAssistant(); + assert.isFalse(isUserMessageEventInAssistantThread(mockThreadStartedArgs)); + }); + }); + + describe('isOtherMessageSubEventInAssistantThread', () => { + it('should return true if message changed by assistant event', async () => { + const mockUserMessageArgs = wrapMiddleware(createDummyAssistantMessageChangedEventMiddlewareArgs()); + const { isOtherMessageSubEventInAssistantThread } = await importAssistant(); + assert(isOtherMessageSubEventInAssistantThread(mockUserMessageArgs)); + }); + + it('should return false if message changed not by assistant event', async () => { + const mockUserMessageArgs = wrapMiddleware(createDummyMessageChangedEventMiddlewareArgs()); + const { isOtherMessageSubEventInAssistantThread } = await importAssistant(); + assert.isFalse(isOtherMessageSubEventInAssistantThread(mockUserMessageArgs)); + }); + + it('should return false if assistant_message_event', async () => { + const mockUserMessageArgs = wrapMiddleware(createDummyAssistantUserMessageEventMiddlewareArgs()); + const { isOtherMessageSubEventInAssistantThread } = await importAssistant(); + assert.isFalse(isOtherMessageSubEventInAssistantThread(mockUserMessageArgs)); + }); + + it('should return false if assistant_thread_started_event', async () => { + const mockThreadStartedArgs = wrapMiddleware(createDummyAssistantThreadStartedEventMiddlewareArgs()); + const { isOtherMessageSubEventInAssistantThread } = await importAssistant(); + assert.isFalse(isOtherMessageSubEventInAssistantThread(mockThreadStartedArgs)); + }); + + it('should return false if assistant_thread_context_changed_event', async () => { + const mockThreadStartedArgs = wrapMiddleware(createDummyAssistantThreadContextChangedEventMiddlewareArgs()); + const { isOtherMessageSubEventInAssistantThread } = await importAssistant(); + assert.isFalse(isOtherMessageSubEventInAssistantThread(mockThreadStartedArgs)); }); }); }); @@ -420,10 +477,11 @@ describe('Assistant class', () => { await continuation(); }) as Middleware); const fn2 = sinon.spy(async () => {}); - const fakeMiddleware = [fn1, fn2] as AssistantMiddleware; + const fakeMiddleware = [autoAcknowledge, fn1, fn2] as AssistantMiddleware; await processAssistantMiddleware(mockThreadContextChangedArgs, fakeMiddleware); + sinon.assert.called(mockThreadContextChangedArgs.ack); assert(fn1.called); assert(fn2.called); }); diff --git a/test/unit/helpers/events.ts b/test/unit/helpers/events.ts index d632b6539..f32a3d945 100644 --- a/test/unit/helpers/events.ts +++ b/test/unit/helpers/events.ts @@ -6,6 +6,7 @@ import type { Block, GenericMessageEvent, KnownBlock, + MessageChangedEvent, MessageEvent, ReactionAddedEvent, } from '@slack/types'; @@ -59,13 +60,14 @@ const ack: AckFn = (_r?) => Promise.resolve(); export function wrapMiddleware( args: Args, ctx?: Context, -): Args & AllMiddlewareArgs & { next: SinonSpy } { +): Args & AllMiddlewareArgs & { next: SinonSpy; ack: SinonSpy } { const wrapped = { ...args, context: ctx || { isEnterpriseInstall: false }, logger: createFakeLogger(), client: new WebClient(), next: sinon.fake(), + ack: sinon.fake(), }; return wrapped; } @@ -187,6 +189,61 @@ export function createDummyMessageEventMiddlewareArgs( }; } +export interface DummyMessageChangedOverrides { + channel_type?: GenericMessageEvent['channel_type']; + message?: DummyMessageOverrides; + previous_message?: DummyMessageOverrides; +} + +export function createDummyMessageChangedEventMiddlewareArgs( + msgOverrides?: DummyMessageChangedOverrides, + // biome-ignore lint/suspicious/noExplicitAny: allow mocking tools to provide any override + bodyOverrides?: Record, +): SlackEventMiddlewareArgs<'message'> { + const dummyMessage: MessageEvent = msgOverrides?.message?.subtype || { + type: 'message', + subtype: msgOverrides?.message?.subtype || undefined, + event_ts: ts, + channel, + channel_type: msgOverrides?.message?.channel_type || 'channel', + user: msgOverrides?.message?.user || user, + ts, + text: msgOverrides?.message?.text || 'hi', + blocks: msgOverrides?.message?.blocks || [], + ...(msgOverrides?.message?.thread_ts ? { thread_ts: msgOverrides?.message?.thread_ts } : {}), + }; + const dummyPreviousMessage: MessageEvent = msgOverrides?.previous_message?.subtype || { + type: 'message', + subtype: msgOverrides?.previous_message?.subtype || undefined, + event_ts: ts, + channel, + channel_type: msgOverrides?.previous_message?.channel_type || 'channel', + user: msgOverrides?.previous_message?.user || user, + ts, + text: msgOverrides?.previous_message?.text || 'hi', + blocks: msgOverrides?.previous_message?.blocks || [], + ...(msgOverrides?.previous_message?.thread_ts ? { thread_ts: msgOverrides?.previous_message?.thread_ts } : {}), + }; + const payload: MessageChangedEvent = { + type: 'message', + subtype: 'message_changed', + event_ts: ts, + hidden: true, + channel, + channel_type: msgOverrides?.channel_type || 'channel', + ts, + message: dummyMessage, + previous_message: dummyPreviousMessage, + }; + return { + payload, + event: payload, + message: payload, + body: envelopeEvent(payload, bodyOverrides), + say, + }; +} + interface DummyAppMentionOverrides { event?: AppMentionEvent; text?: string; @@ -276,6 +333,21 @@ export function createDummyAssistantUserMessageEventMiddlewareArgs( }; } +export function createDummyAssistantMessageChangedEventMiddlewareArgs( + msgOverrides?: DummyMessageChangedOverrides, + // biome-ignore lint/suspicious/noExplicitAny: allow mocking tools to provide any override + bodyOverrides?: Record, +): SlackEventMiddlewareArgs<'message'> { + return createDummyMessageChangedEventMiddlewareArgs( + msgOverrides || { + channel_type: 'im', + message: { thread_ts: ts, channel_type: 'im' }, + previous_message: { thread_ts: ts, channel_type: 'im' }, + }, + bodyOverrides, + ); +} + interface DummyCommandOverride { command?: string; slashCommand?: SlashCommand; diff --git a/test/unit/middleware/builtin.spec.ts b/test/unit/middleware/builtin.spec.ts index fd8b35643..278bd01dc 100644 --- a/test/unit/middleware/builtin.spec.ts +++ b/test/unit/middleware/builtin.spec.ts @@ -232,6 +232,7 @@ describe('Built-in global middleware', () => { const args = wrapMiddleware(createDummyCommandMiddlewareArgs(), ctx); await builtins.ignoreSelf(args); sinon.assert.called(args.next); + sinon.assert.notCalled(args.ack); }); it('should ignore message events identified as a bot message from the same bot ID as this app', async () => { @@ -252,6 +253,7 @@ describe('Built-in global middleware', () => { ctx, ); await builtins.ignoreSelf(args); + sinon.assert.called(args.ack); sinon.assert.notCalled(args.next); }); @@ -259,6 +261,7 @@ describe('Built-in global middleware', () => { const ctx = { ...dummyContext, botUserId: fakeBotUserId }; const args = wrapMiddleware(createDummyReactionAddedEventMiddlewareArgs({ user: fakeBotUserId }), ctx); await builtins.ignoreSelf(args); + sinon.assert.called(args.ack); sinon.assert.notCalled(args.next); }); @@ -266,6 +269,7 @@ describe('Built-in global middleware', () => { const ctx = { ...dummyContext, botUserId: fakeBotUserId, botId: fakeBotUserId }; const args = wrapMiddleware(createDummyReactionAddedEventMiddlewareArgs({ user: fakeBotUserId }), ctx); await builtins.ignoreSelf(args); + sinon.assert.called(args.ack); sinon.assert.notCalled(args.next); }); @@ -279,6 +283,7 @@ describe('Built-in global middleware', () => { await Promise.all(listOfFakeArgs.map(builtins.ignoreSelf)); for (const args of listOfFakeArgs) { + sinon.assert.notCalled(args.ack); sinon.assert.called(args.next); } });