From 4f29d238b83bd6e0ae6edefa4aa2e2ef992f2857 Mon Sep 17 00:00:00 2001 From: "@mrubens" <2600+mrubens@users.noreply.github.com> Date: Thu, 10 Sep 2026 03:17:58 +0000 Subject: [PATCH] fix: keep automation suggestions in their Slack thread --- .../reactions-chat-reply-suggestions.test.ts | 68 +++++++ .../__tests__/manager-slack-target.test.ts | 9 +- .../__tests__/submitTaskSuggestions.test.ts | 171 +++++++++--------- .../handlers/tasks/submitTaskSuggestions.ts | 86 +++++---- 4 files changed, 212 insertions(+), 122 deletions(-) diff --git a/apps/api/src/handlers/slack/events/reactions-chat-reply-suggestions.test.ts b/apps/api/src/handlers/slack/events/reactions-chat-reply-suggestions.test.ts index 743111208..6b1eddab9 100644 --- a/apps/api/src/handlers/slack/events/reactions-chat-reply-suggestions.test.ts +++ b/apps/api/src/handlers/slack/events/reactions-chat-reply-suggestions.test.ts @@ -531,6 +531,74 @@ describe('chat reply suggestion reactions', () => { }, ); + it('uses a persisted task-backed card origin without consulting its source task', async () => { + mocks.getSessionForTask.mockResolvedValue(null); + mocks.sessionsFindFirst.mockResolvedValue({ + id: 'session-origin', + fastConversationId: 'fast-origin', + }); + mocks.conversationFindById.mockResolvedValue({ + conversation: { + surface: 'slack', + workspaceId: 'T1', + conversationId: 'automation-thread-ts', + replyTarget: { + channelId: 'C_REPORTS', + threadId: 'automation-thread-ts', + }, + }, + }); + mocks.trackedMessageFindFirst.mockResolvedValue({ + id: 'tracked-message-1', + workItemId: 'work-item-1', + metadata: { + suggestionType: 'suggested_tasks', + launchRouting: 'router', + originSessionId: 'session-origin', + }, + }); + mocks.lookupSlackUserMapping.mockResolvedValue({ + hasInactiveMapping: false, + activeMapping: { userId: 'user-1' }, + }); + const slack = { + postMessage: vi.fn(async () => 'announce-ts'), + deleteMessage: vi.fn(async () => undefined), + getMessageMetadata: vi.fn(), + }; + + await handleReactionAddedEvent({ + context: { + teamId: 'T1', + slackInstallation: { botUserId: 'UROOMOTE', teamId: 'T1' }, + slack, + } as never, + event: { + type: 'reaction_added', + user: 'U1', + reaction: 'thumbsup', + item: { type: 'message', channel: 'C_REPORTS', ts: 'card-ts' }, + event_ts: 'event-ts', + }, + }); + + expect(mocks.getSessionForTask).not.toHaveBeenCalled(); + expect(slack.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + channel: 'C_REPORTS', + thread_ts: 'automation-thread-ts', + }), + ); + expect(mocks.conversationGetOrCreate).toHaveBeenCalledWith( + expect.objectContaining({ sessionId: 'session-origin' }), + ); + expect(mocks.startFastAgentResponse).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ thread_ts: 'automation-thread-ts' }), + }), + ); + }); + it.each(['task', 'fast-report'])( "announces a pinned %s launch in the origin Session's own thread instead of seeding one", async (source) => { diff --git a/apps/api/src/handlers/tasks/__tests__/manager-slack-target.test.ts b/apps/api/src/handlers/tasks/__tests__/manager-slack-target.test.ts index 27d146bb5..a6af8c034 100644 --- a/apps/api/src/handlers/tasks/__tests__/manager-slack-target.test.ts +++ b/apps/api/src/handlers/tasks/__tests__/manager-slack-target.test.ts @@ -2,7 +2,9 @@ import { automations, db, deploymentSettings, + ensureSessionForTask, eq, + sessions, slackInstallationChannels, slackInstallationFactory, slackInstallations, @@ -20,7 +22,9 @@ import { resolveScheduledSuggestionSlackConfig } from '../background-automation- import { postSuggestedTasksSummaryToSlack } from '../submitTaskSuggestions'; vi.mock('@roomote/cloud-agents/server', () => ({ - fastAgentConversationRepository: { getOrCreate: vi.fn() }, + fastAgentConversationRepository: { + getOrCreate: vi.fn(async ({ conversation }) => ({ conversation })), + }, findEnvironmentForRepo: vi.fn(), })); vi.mock('@roomote/sdk/server', () => ({ @@ -60,6 +64,7 @@ describe.each(['work items', 'scheduled suggestions'] as const)( (consumer) => { let userId: string; let taskId: string; + let sessionId: string; let suggestion: Parameters< typeof postSuggestedTasksSummaryToSlack >[0]['suggestions'][number]; @@ -78,6 +83,7 @@ describe.each(['work items', 'scheduled suggestions'] as const)( }); userId = (await userFactory.create()).id; taskId = (await taskFactory.create({ initiatorUserId: userId })).id; + sessionId = (await ensureSessionForTask(db, { taskId })).id; const [row] = await db .insert(workItems) .values({ @@ -96,6 +102,7 @@ describe.each(['work items', 'scheduled suggestions'] as const)( .delete(trackedMessages) .where(eq(trackedMessages.threadTs, '123.456')); await db.delete(tasks).where(eq(tasks.id, taskId)); + await db.delete(sessions).where(eq(sessions.id, sessionId)); await db .delete(slackInstallations) .where(eq(slackInstallations.installedByUserId, userId)); diff --git a/apps/api/src/handlers/tasks/__tests__/submitTaskSuggestions.test.ts b/apps/api/src/handlers/tasks/__tests__/submitTaskSuggestions.test.ts index ad7e52af0..cd31b2019 100644 --- a/apps/api/src/handlers/tasks/__tests__/submitTaskSuggestions.test.ts +++ b/apps/api/src/handlers/tasks/__tests__/submitTaskSuggestions.test.ts @@ -7,7 +7,6 @@ import { } from '@roomote/types'; import type { Variables } from '../../../types'; -import { apiLogger } from '../../../logging'; import { mcpAuthMiddleware } from '../../mcp/middleware'; import { submitTaskSuggestions } from '../submitTaskSuggestions'; import { db, getAutomationRuntime } from '@roomote/db/server'; @@ -286,6 +285,7 @@ vi.mock('@roomote/db/server', () => ({ suggestionType: string; suggestionKey: string; suggestionGroupKey?: string; + originSessionId?: string; launchRouting?: 'router'; }>, ) => { @@ -305,6 +305,9 @@ vi.mock('@roomote/db/server', () => ({ ...(registration.suggestionGroupKey ? { suggestionGroupKey: registration.suggestionGroupKey } : {}), + ...(registration.originSessionId + ? { originSessionId: registration.originSessionId } + : {}), ...(registration.launchRouting ? { launchRouting: registration.launchRouting } : {}), @@ -452,8 +455,18 @@ describe('submitTaskSuggestions', () => { mockEnvironmentFindFirst.mockReset(); mockFindEnvironmentForRepo.mockReset(); mockPostMessage.mockReset(); - mockGetSessionForTask.mockReset().mockResolvedValue(null); - mockGetOrCreate.mockReset(); + mockGetSessionForTask.mockReset().mockResolvedValue({ + id: 'session-1', + ownerKind: 'automation', + ownerAutomation: 'suggest_ideas', + ownerUserId: null, + fastConversationId: null, + }); + mockGetOrCreate + .mockReset() + .mockImplementation(async ({ conversation }) => ({ + conversation, + })); insertedWorkItemValues.length = 0; insertedTrackedMessageValues.length = 0; automationThreadReceipts.length = 0; @@ -988,6 +1001,7 @@ describe('submitTaskSuggestions', () => { expect(insertedTrackedMessageValues).toHaveLength(1); expect(insertedTrackedMessageValues[0]).toMatchObject({ createdByUserId: null, + metadata: { originSessionId: 'session-1' }, }); expect(mockGetSessionForTask).toHaveBeenCalledWith(db, 'task-1'); expect(mockGetOrCreate).toHaveBeenCalledExactlyOnceWith({ @@ -1002,44 +1016,35 @@ describe('submitTaskSuggestions', () => { }); const postCount = mockPostMessage.mock.calls.length; - mockGetSessionForTask.mockResolvedValue({ - id: 'session-1', - fastConversationId: 'fast-1', - }); expect((await requestSuggestions(app)).status).toBe(200); expect(mockPostMessage).toHaveBeenCalledTimes(postCount); expect(mockGetSessionForTask).toHaveBeenCalledTimes(2); - expect(mockGetOrCreate).toHaveBeenCalledTimes(1); + expect(mockGetOrCreate).toHaveBeenCalledTimes(2); }); - it.each([ - { - ownerKind: 'user', - ownerUserId: 'session-owner', - fastConversationId: 'fast-1', - }, - { ownerKind: 'system', ownerUserId: null, fastConversationId: null }, - ])( - 'does not bind an already-bound or unsupported Session: $ownerKind', - async (session) => { - mockTaskFindFirst.mockResolvedValue({ - initiatorUserId: null, - initiatorAutomation: 'suggest_ideas', - }); - mockGetSessionForTask.mockResolvedValue({ id: 'session-1', ...session }); - const app = createApp({ - runId: 1, - userId: null, - principal: 'user', - tokenType: 'run', - version: 1, - }); + it('rejects publication when the origin Session cannot own the Slack thread', async () => { + mockTaskFindFirst.mockResolvedValue({ + initiatorUserId: null, + initiatorAutomation: 'suggest_ideas', + }); + mockGetSessionForTask.mockResolvedValue({ + id: 'session-1', + ownerKind: 'system', + ownerUserId: null, + fastConversationId: null, + }); + const app = createApp({ + runId: 1, + userId: null, + principal: 'user', + tokenType: 'run', + version: 1, + }); - expect((await requestSuggestions(app)).status).toBe(200); - expect(mockPostMessage.mock.calls.length).toBeGreaterThanOrEqual(2); - expect(mockGetOrCreate).not.toHaveBeenCalled(); - }, - ); + expect((await requestSuggestions(app)).status).toBe(500); + expect(mockPostMessage).not.toHaveBeenCalled(); + expect(mockGetOrCreate).not.toHaveBeenCalled(); + }); it.each(['C-AUTO', 'C-CHANGED', undefined])( 'recovers a failed Session bind without reposting when the configured channel is %s', @@ -1055,14 +1060,10 @@ describe('submitTaskSuggestions', () => { fastConversationId: null, }); mockGetOrCreate.mockRejectedValueOnce(new Error('binding unavailable')); - mockGetOrCreate.mockImplementation(async () => { + mockGetOrCreate.mockImplementation(async ({ conversation }) => { expect(transactionActive).toBe(false); - mockGetSessionForTask.mockResolvedValue({ - id: 'session-1', - fastConversationId: 'fast-1', - }); + return { conversation }; }); - const warn = vi.spyOn(apiLogger, 'warn').mockImplementation(() => {}); const app = createApp({ runId: 1, userId: null, @@ -1070,55 +1071,47 @@ describe('submitTaskSuggestions', () => { tokenType: 'run', version: 1, }); - try { - expect((await requestSuggestions(app)).status).toBe(200); - const postCount = mockPostMessage.mock.calls.length; - expect(postCount).toBeGreaterThanOrEqual(2); - expect(automationThreadReceipts).toEqual([ - { - surface: 'slack', - kind: 'automation_thread', - dedupeKey: 'T1:C-AUTO:ts-1', - channelId: 'C-AUTO', - threadTs: 'ts-1', - metadata: { - sourceTaskId: 'task-1', - slackTeamId: 'T1', - suggestionCount: 1, - }, - }, - ]); - vi.mocked(getAutomationRuntime).mockResolvedValue({ - slackChannelId: configuredChannel, - } as unknown as Awaited>); - slackInstallationChannelRows = []; - expect((await requestSuggestions(app)).status).toBe(200); - expect(mockPostMessage).toHaveBeenCalledTimes(postCount); - expect(mockGetOrCreate).toHaveBeenCalledTimes(2); - expect(mockGetOrCreate).toHaveBeenLastCalledWith({ - sessionId: 'session-1', - owner: { kind: 'automation', automationKey: 'suggest_ideas' }, - conversation: { - surface: 'slack', - workspaceId: 'T1', - conversationId: 'ts-1', - replyTarget: { channelId: 'C-AUTO', threadId: 'ts-1' }, + expect((await requestSuggestions(app)).status).toBe(500); + const postCount = mockPostMessage.mock.calls.length; + expect(postCount).toBeGreaterThanOrEqual(2); + expect(automationThreadReceipts).toEqual([ + { + surface: 'slack', + kind: 'automation_thread', + dedupeKey: 'T1:C-AUTO:ts-1', + channelId: 'C-AUTO', + threadTs: 'ts-1', + metadata: { + sourceTaskId: 'task-1', + slackTeamId: 'T1', + suggestionCount: 1, }, - }); - expect((await requestSuggestions(app)).status).toBe(200); - expect(mockPostMessage).toHaveBeenCalledTimes(postCount); - expect(mockGetOrCreate).toHaveBeenCalledTimes(2); - expect(warn).toHaveBeenCalledWith( - expect.stringContaining( - 'Slack report published but Session binding failed for task task-1: binding unavailable', - ), - ); - expect(postScheduledSuggestionsToDiscord).not.toHaveBeenCalled(); - expect(postScheduledSuggestionsToTelegram).not.toHaveBeenCalled(); - expect(postScheduledSuggestionsToTeams).not.toHaveBeenCalled(); - } finally { - warn.mockRestore(); - } + }, + ]); + vi.mocked(getAutomationRuntime).mockResolvedValue({ + slackChannelId: configuredChannel, + } as unknown as Awaited>); + slackInstallationChannelRows = []; + expect((await requestSuggestions(app)).status).toBe(200); + expect(mockPostMessage).toHaveBeenCalledTimes(postCount); + expect(mockGetOrCreate).toHaveBeenCalledTimes(2); + expect(mockGetOrCreate).toHaveBeenLastCalledWith({ + sessionId: 'session-1', + owner: { kind: 'automation', automationKey: 'suggest_ideas' }, + conversation: { + surface: 'slack', + workspaceId: 'T1', + conversationId: 'ts-1', + replyTarget: { channelId: 'C-AUTO', threadId: 'ts-1' }, + }, + }); + expect(insertedTrackedMessageValues).toHaveLength(1); + expect(insertedTrackedMessageValues[0]?.metadata).toMatchObject({ + originSessionId: 'session-1', + }); + expect(postScheduledSuggestionsToDiscord).not.toHaveBeenCalled(); + expect(postScheduledSuggestionsToTelegram).not.toHaveBeenCalled(); + expect(postScheduledSuggestionsToTeams).not.toHaveBeenCalled(); }, ); diff --git a/apps/api/src/handlers/tasks/submitTaskSuggestions.ts b/apps/api/src/handlers/tasks/submitTaskSuggestions.ts index 661d6743f..ced6dbf13 100644 --- a/apps/api/src/handlers/tasks/submitTaskSuggestions.ts +++ b/apps/api/src/handlers/tasks/submitTaskSuggestions.ts @@ -175,6 +175,7 @@ type TaskSuggestionType = type SuggestionCardMessageRow = { suggestionType: TaskSuggestionType; + originSessionId?: string; launchRouting?: 'router'; messageTs: string; channelId: string; @@ -196,6 +197,7 @@ function registerSlackSuggestionMessageRows( createdByUserId: row.createdByUserId, suggestionType: row.suggestionType, suggestionKey: row.suggestionKey, + originSessionId: row.originSessionId, launchRouting: row.launchRouting, })), executor, @@ -614,6 +616,7 @@ function getSuggestionFooterKind(category: SuggestionCategory): string { async function postTaskSuggestionsThreadToSlack(params: { sourceTaskId: string; + originSessionId?: string; slackBotAccessToken: string; slackChannelId: string; createdByUserId: string | null; @@ -785,6 +788,9 @@ async function postTaskSuggestionsThreadToSlack(params: { suggestionMessageRows.push({ suggestionType: params.suggestionType, + ...(params.originSessionId + ? { originSessionId: params.originSessionId } + : {}), ...(params.launchRouting ? { launchRouting: params.launchRouting } : {}), messageTs, channelId: params.slackChannelId, @@ -1008,6 +1014,27 @@ export async function postSuggestedTasksSummaryToSlack(params: { } const { slackInstallation } = target; + const session = await getSessionForTask(db, params.sourceTaskId); + if (!session) { + throw new Error( + `Task ${params.sourceTaskId} does not have an origin Session.`, + ); + } + const owner = + session.ownerKind === 'automation' && session.ownerAutomation + ? { + kind: 'automation' as const, + automationKey: session.ownerAutomation, + } + : session.ownerKind === 'user' && session.ownerUserId + ? { kind: 'user' as const, userId: session.ownerUserId } + : null; + if (!owner) { + throw new Error( + `Task ${params.sourceTaskId} has an unsupported origin Session owner.`, + ); + } + const automationLabel = getScheduledSuggestionBackgroundAutomationDescriptor( params.suggestionSource, @@ -1088,6 +1115,7 @@ export async function postSuggestedTasksSummaryToSlack(params: { const postResult = await postTaskSuggestionsThreadToSlack({ sourceTaskId: params.sourceTaskId, + originSessionId: session.id, slackBotAccessToken: slackInstallation.botAccessToken, slackChannelId: channelId, existingRootMessageTs: receipt?.threadTs ?? undefined, @@ -1168,38 +1196,32 @@ export async function postSuggestedTasksSummaryToSlack(params: { }); if (publication.rootMessageTs && publication.channelId) { - try { - const session = await getSessionForTask(db, params.sourceTaskId); - if (session && !session.fastConversationId) { - const owner = - session.ownerKind === 'automation' && session.ownerAutomation - ? { - kind: 'automation' as const, - automationKey: session.ownerAutomation, - } - : session.ownerKind === 'user' && session.ownerUserId - ? { kind: 'user' as const, userId: session.ownerUserId } - : null; - if (owner) { - await fastAgentConversationRepository.getOrCreate({ - sessionId: session.id, - owner, - conversation: { - surface: 'slack', - workspaceId: slackInstallation.teamId, - conversationId: publication.rootMessageTs, - replyTarget: { - channelId: publication.channelId, - threadId: publication.rootMessageTs, - }, - }, - }); - } - } - } catch (error) { - // Delivery already committed; binding failure must not trigger a repost. - apiLogger.warn( - `[submitTaskSuggestions] Slack report published but Session binding failed for task ${params.sourceTaskId}: ${error instanceof Error ? error.message : String(error)}`, + // Binding is part of publication success. If it fails, the caller retries; + // the committed receipt and cards above keep that retry from reposting. + const expectedConversation = { + surface: 'slack' as const, + workspaceId: slackInstallation.teamId, + conversationId: publication.rootMessageTs, + replyTarget: { + channelId: publication.channelId, + threadId: publication.rootMessageTs, + }, + }; + const bound = await fastAgentConversationRepository.getOrCreate({ + sessionId: session.id, + owner, + conversation: expectedConversation, + }); + if ( + bound.conversation.surface !== 'slack' || + bound.conversation.workspaceId !== expectedConversation.workspaceId || + bound.conversation.replyTarget.channelId !== + expectedConversation.replyTarget.channelId || + bound.conversation.replyTarget.threadId !== + expectedConversation.replyTarget.threadId + ) { + throw new Error( + `Task ${params.sourceTaskId} origin Session is bound to a different conversation.`, ); } }