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 743111208f..604186e9f5 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 @@ -61,9 +61,24 @@ function createWorkItemSelectBuilder() { return builder; } +let updateBuilderCatchError: Error | null = null; const updateBuilder = { set: vi.fn(() => updateBuilder), - where: vi.fn(async () => undefined), + where: vi.fn(() => updateBuilder), + returning: vi.fn(async () => [{ id: 'tracked-message-1' }]), + then: ( + onfulfilled?: + | ((value: undefined) => TResult1 | PromiseLike) + | null, + onrejected?: ((reason: unknown) => TResult2 | PromiseLike) | null, + ) => Promise.resolve(undefined).then(onfulfilled, onrejected), + catch: vi.fn( + async (onRejected?: (reason: unknown) => unknown): Promise => { + const error = updateBuilderCatchError; + updateBuilderCatchError = null; + return error ? onRejected?.(error) : undefined; + }, + ), }; vi.mock('@roomote/redis', () => ({ @@ -73,6 +88,7 @@ vi.mock('@roomote/redis', () => ({ vi.mock('@roomote/db/server', () => ({ and: vi.fn((...args) => args), eq: vi.fn((...args) => args), + sql: vi.fn((strings, ...values) => ['sql', strings, values]), trackedMessages: { id: 'id', surface: 'surface', @@ -95,6 +111,7 @@ vi.mock('@roomote/db/server', () => ({ sortOrder: 'sortOrder', status: 'status', sourceTaskId: 'sourceTaskId', + launchClaimedAt: 'launchClaimedAt', }, claimWorkItem: mocks.claimWorkItem, getSessionForTask: mocks.getSessionForTask, @@ -172,6 +189,7 @@ import { handleReactionAddedEvent } from './reactions'; describe('chat reply suggestion reactions', () => { beforeEach(() => { vi.clearAllMocks(); + updateBuilderCatchError = null; mocks.getSessionForTask.mockResolvedValue(null); mocks.sessionsFindFirst.mockResolvedValue(null); mocks.conversationFindById.mockResolvedValue(null); @@ -271,33 +289,9 @@ describe('chat reply suggestion reactions', () => { expect(mocks.routeFastReaction).not.toHaveBeenCalled(); }); - it.each([ - { cardChannel: 'C1', bound: true }, - { cardChannel: 'C_OTHER', bound: true }, - { cardChannel: 'C1', bound: false }, - { cardChannel: 'C_OTHER', bound: false }, - ])( - 'does not route from metadata fallback card $cardChannel (Session bound: $bound)', - async ({ cardChannel, bound }) => { - const expectedThread = bound ? 'session-report-ts' : 'announce-ts'; - const expectedChannel = bound ? 'C_REPORTS' : 'C1'; - if (bound) { - mocks.getSessionForTask.mockResolvedValue({ id: 'session-origin' }); - mocks.sessionsFindFirst.mockResolvedValue({ - fastConversationId: 'fast-origin', - }); - mocks.conversationFindById.mockResolvedValue({ - conversation: { - surface: 'slack', - workspaceId: 'T1', - conversationId: expectedThread, - replyTarget: { - channelId: expectedChannel, - threadId: expectedThread, - }, - }, - }); - } + it.each([{ cardChannel: 'C1' }, { cardChannel: 'C_OTHER' }])( + 'does not route from metadata fallback card $cardChannel', + async ({ cardChannel }) => { mocks.parseSuggestionMetadata.mockReturnValue('work-item-1'); mocks.trackedMessageFindFirst .mockResolvedValueOnce(null) @@ -339,27 +333,20 @@ describe('chat reply suggestion reactions', () => { mocks.trackedMessageFindFirst.mock.calls[1]![0].where, ).toContainEqual(['surface', 'slack']); expect(slack.postMessage.mock.calls[0]![0]).toMatchObject({ - channel: expectedChannel, + channel: 'C1', }); - if (bound) { - expect(slack.postMessage.mock.calls[0]![0]).toHaveProperty( - 'thread_ts', - expectedThread, - ); - } else { - expect(slack.postMessage.mock.calls[0]![0]).not.toHaveProperty( - 'thread_ts', - ); - } + expect(slack.postMessage.mock.calls[0]![0]).not.toHaveProperty( + 'thread_ts', + ); expect(mocks.launchPinned).toHaveBeenCalledWith( expect.objectContaining({ conversation: { surface: 'slack', workspaceId: 'T1', - conversationId: expectedThread, + conversationId: 'announce-ts', replyTarget: { - channelId: expectedChannel, - threadId: expectedThread, + channelId: 'C1', + threadId: 'announce-ts', }, }, }), @@ -367,7 +354,7 @@ describe('chat reply suggestion reactions', () => { expect(updateBuilder.where).toHaveBeenCalledWith([ ['id', 'tracked-message-1'], ['surface', 'slack'], - ['channelId', expectedChannel], + ['channelId', cardChannel], ]); }, ); @@ -407,7 +394,7 @@ describe('chat reply suggestion reactions', () => { hasInactiveMapping: false, activeMapping: { userId: 'user-1' }, }); - updateBuilder.where.mockRejectedValueOnce(new Error('tracking failed')); + updateBuilderCatchError = new Error('tracking failed'); const slack = { postMessage: vi.fn(async () => 'seeded-thread-ts'), deleteMessage: vi.fn(async () => undefined), @@ -437,46 +424,27 @@ describe('chat reply suggestion reactions', () => { expect(slack.deleteMessage).not.toHaveBeenCalled(); }); - it.each(['task', 'fast-report'])( - 'binds a router-backed %s suggestion to the automation Session before starting Fast in its report thread', - async (source) => { - if (source === 'fast-report') workItem.sourceTaskId = null; - mocks.getSessionForTask.mockResolvedValue({ id: 'session-origin' }); - mocks.sessionsFindFirst.mockResolvedValue({ - id: 'session-origin', - fastConversationId: 'fast-origin', - }); - mocks.conversationFindById.mockResolvedValue({ - conversation: { - surface: 'slack', - workspaceId: 'T1', - conversationId: 'report-thread-ts', - replyTarget: { channelId: 'C1', threadId: 'report-thread-ts' }, - }, - }); + it.each(['router', 'pinned'])( + 'starts a %s suggestion in its own thread and Session while retaining origin metadata', + async (launchKind) => { mocks.trackedMessageFindFirst.mockResolvedValue({ id: 'tracked-message-1', workItemId: 'work-item-1', - threadTs: 'report-thread-ts', - surface: 'slack', channelId: 'C1', metadata: { suggestionType: 'suggested_tasks', - launchRouting: 'router', - ...(source === 'fast-report' - ? { originSessionId: 'session-origin' } - : {}), + originSessionId: 'session-origin', + ...(launchKind === 'router' ? { launchRouting: 'router' } : {}), }, }); mocks.lookupSlackUserMapping.mockResolvedValue({ hasInactiveMapping: false, - activeMapping: { - userId: 'user-1', - }, + activeMapping: { userId: 'user-1' }, }); const slack = { - postMessage: vi.fn(async () => 'seeded-thread-ts'), + postMessage: vi.fn(async () => 'execution-thread-ts'), deleteMessage: vi.fn(async () => undefined), + addReaction: vi.fn(async () => true), getMessageMetadata: vi.fn(), }; @@ -495,155 +463,190 @@ describe('chat reply suggestion reactions', () => { }, }); - expect(slack.postMessage).toHaveBeenCalledExactlyOnceWith( - expect.objectContaining({ - channel: 'C1', - thread_ts: 'report-thread-ts', - }), + expect(slack.postMessage).toHaveBeenCalledWith( + expect.not.objectContaining({ thread_ts: expect.anything() }), ); - expect(mocks.conversationGetOrCreate).toHaveBeenCalledWith({ - userId: 'user-1', - sessionId: 'session-origin', - conversation: { - surface: 'slack', - workspaceId: 'T1', - conversationId: 'report-thread-ts', - replyTarget: { channelId: 'C1', threadId: 'report-thread-ts' }, - }, + expect(slack.addReaction).toHaveBeenCalledWith({ + channel: 'C1', + timestamp: 'card-ts', + name: 'eyes', }); - expect( - mocks.conversationGetOrCreate.mock.invocationCallOrder[0], - ).toBeLessThan(mocks.startFastAgentResponse.mock.invocationCallOrder[0]!); - expect(mocks.startFastAgentResponse).toHaveBeenCalledWith( + expect(mocks.getSessionForTask).not.toHaveBeenCalled(); + expect(mocks.conversationGetOrCreate).not.toHaveBeenCalled(); + expect(updateBuilder.set).toHaveBeenNthCalledWith( + 1, expect.objectContaining({ - userId: 'user-1', - event: expect.objectContaining({ - channel: 'C1', - thread_ts: 'report-thread-ts', - agentContext: 'implementation prompt', + metadata: expect.objectContaining({ + originSessionId: 'session-origin', + executionChannelId: 'C1', + executionThreadTs: 'execution-thread-ts', + executionClaimedAt: claimedAt.toISOString(), }), }), ); - expect(mocks.finalizeWorkItemLaunched).toHaveBeenCalledWith( - expect.anything(), - { id: 'work-item-1', taskId: null, claimedAt }, + expect(updateBuilder.set).toHaveBeenLastCalledWith( + expect.objectContaining({ threadTs: 'execution-thread-ts' }), ); + + if (launchKind === 'router') { + expect(mocks.startFastAgentResponse).toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ + channel: 'C1', + ts: 'execution-thread-ts', + thread_ts: 'execution-thread-ts', + }), + }), + ); + } else { + expect(mocks.launchPinned).toHaveBeenCalledWith( + expect.objectContaining({ + launchId: 'slack-suggestion:work-item-1', + conversation: { + surface: 'slack', + workspaceId: 'T1', + conversationId: 'execution-thread-ts', + replyTarget: { + channelId: 'C1', + threadId: 'execution-thread-ts', + }, + }, + }), + ); + expect(mocks.launchPinned.mock.calls[0]![0]).not.toHaveProperty( + 'originSessionId', + ); + expect(mocks.liveTaskLauncher).toHaveBeenCalledWith( + expect.objectContaining({ + channelId: 'C1', + threadTs: 'execution-thread-ts', + messageId: 'execution-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) => { - if (source === 'fast-report') workItem.sourceTaskId = null; - mocks.getSessionForTask.mockResolvedValue({ id: 'session-origin' }); - mocks.sessionsFindFirst.mockResolvedValue({ - id: 'session-origin', - fastConversationId: 'fast-origin', - }); - mocks.conversationFindById.mockResolvedValue({ - id: 'fast-origin', - conversation: { - surface: 'slack', - workspaceId: 'T1', - conversationId: 'report-thread-ts', - replyTarget: { channelId: 'C_REPORTS', threadId: 'report-thread-ts' }, - }, - }); - mocks.trackedMessageFindFirst.mockResolvedValue({ - id: 'tracked-message-1', - workItemId: 'work-item-1', - metadata: { - suggestionType: 'suggested_tasks', - ...(source === 'fast-report' - ? { originSessionId: 'session-origin' } - : {}), - }, - }); - mocks.lookupSlackUserMapping.mockResolvedValue({ - hasInactiveMapping: false, - activeMapping: { userId: 'user-1' }, - }); - const postMessage = vi.fn(async () => 'announce-ts'); - const slack = { - postMessage, - deleteMessage: vi.fn(async () => undefined), - getMessageMetadata: vi.fn(), - }; + it('reuses a persisted execution thread on an acceptance retry', async () => { + mocks.trackedMessageFindFirst.mockResolvedValue({ + id: 'tracked-message-1', + workItemId: 'work-item-1', + channelId: 'C1', + metadata: { + suggestionType: 'suggested_tasks', + executionChannelId: 'C1', + executionThreadTs: 'existing-execution-thread-ts', + }, + }); + mocks.lookupSlackUserMapping.mockResolvedValue({ + hasInactiveMapping: false, + activeMapping: { userId: 'user-1' }, + }); + const slack = { + postMessage: vi.fn(), + deleteMessage: vi.fn(async () => undefined), + addReaction: vi.fn(async () => true), + 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: 'C1', ts: 'card-ts' }, - event_ts: 'event-ts', - }, - }); + 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: 'C1', ts: 'card-ts' }, + event_ts: 'event-ts', + }, + }); - // The announcement is a reply in the automation's report thread. - expect(postMessage).toHaveBeenCalledExactlyOnceWith( - expect.objectContaining({ - channel: 'C_REPORTS', - thread_ts: 'report-thread-ts', - }), - ); - expect(mocks.launchPinned).toHaveBeenCalledWith( - expect.objectContaining({ - originSessionId: 'session-origin', - conversation: { - surface: 'slack', - workspaceId: 'T1', - conversationId: 'report-thread-ts', - replyTarget: { - channelId: 'C_REPORTS', - threadId: 'report-thread-ts', - }, + expect(slack.postMessage).not.toHaveBeenCalled(); + expect(slack.addReaction).not.toHaveBeenCalled(); + expect(mocks.launchPinned).toHaveBeenCalledWith( + expect.objectContaining({ + launchId: 'slack-suggestion:work-item-1', + conversation: expect.objectContaining({ + conversationId: 'existing-execution-thread-ts', + replyTarget: { + channelId: 'C1', + threadId: 'existing-execution-thread-ts', }, }), - ); - expect(mocks.liveTaskLauncher).toHaveBeenCalledWith( - expect.objectContaining({ - channelId: 'C_REPORTS', - threadTs: 'report-thread-ts', - messageId: 'announce-ts', - }), - ); - }, - ); + }), + ); + }); - it('launches through the automation Session bound when its report was published', async () => { - mocks.getSessionForTask.mockResolvedValue({ id: 'session-origin' }); - mocks.sessionsFindFirst.mockResolvedValue({ - fastConversationId: 'fast-origin', + it('does not create an execution thread when acknowledgement fails', async () => { + mocks.trackedMessageFindFirst.mockResolvedValue({ + id: 'tracked-message-1', + workItemId: 'work-item-1', + channelId: 'C1', + metadata: { suggestionType: 'suggested_tasks' }, }); - mocks.conversationFindById.mockResolvedValue({ - conversation: { - surface: 'slack', - workspaceId: 'T1', - conversationId: 'report-thread-ts', - replyTarget: { channelId: 'C1', threadId: 'report-thread-ts' }, + mocks.lookupSlackUserMapping.mockResolvedValue({ + hasInactiveMapping: false, + activeMapping: { userId: 'user-1' }, + }); + const slack = { + postMessage: vi.fn(), + deleteMessage: vi.fn(), + addReaction: vi.fn(async () => false), + 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: 'C1', ts: 'card-ts' }, + event_ts: 'event-ts', }, }); + + expect(slack.postMessage).not.toHaveBeenCalled(); + expect(mocks.startFastAgentResponse).not.toHaveBeenCalled(); + expect(mocks.launchPinned).not.toHaveBeenCalled(); + expect(mocks.releaseWorkItemClaim).toHaveBeenCalledWith(expect.anything(), { + id: 'work-item-1', + claimedAt, + }); + }); + + it('does not delete an execution root after a newer claim takes ownership', async () => { mocks.trackedMessageFindFirst.mockResolvedValue({ id: 'tracked-message-1', workItemId: 'work-item-1', - threadTs: 'report-thread-ts', - surface: 'slack', channelId: 'C1', - metadata: { suggestionType: 'suggested_tasks' }, + metadata: { + suggestionType: 'suggested_tasks', + launchRouting: 'router', + }, }); mocks.lookupSlackUserMapping.mockResolvedValue({ hasInactiveMapping: false, activeMapping: { userId: 'user-1' }, }); + mocks.startFastAgentResponse.mockRejectedValue(new Error('startup failed')); + updateBuilder.returning + .mockResolvedValueOnce([{ id: 'tracked-message-1' }]) + .mockResolvedValueOnce([]); const slack = { - postMessage: vi.fn(async () => 'seeded-thread-ts'), + postMessage: vi + .fn() + .mockResolvedValueOnce('execution-thread-ts') + .mockResolvedValueOnce('failure-message-ts'), deleteMessage: vi.fn(async () => undefined), + addReaction: vi.fn(async () => true), getMessageMetadata: vi.fn(), }; @@ -662,49 +665,73 @@ describe('chat reply suggestion reactions', () => { }, }); - expect(mocks.resolveWorkspace).toHaveBeenCalled(); - expect(slack.postMessage).toHaveBeenCalledWith( - expect.objectContaining({ channel: 'C1', thread_ts: 'report-thread-ts' }), - ); - expect(mocks.getSessionForTask).toHaveBeenCalledWith( - expect.anything(), - 'scan-task-1', - ); - expect(mocks.launchPinned).toHaveBeenCalledWith( - expect.objectContaining({ - userId: 'user-1', - surface: 'slack', - originSessionId: 'session-origin', - conversation: { - surface: 'slack', - workspaceId: 'T1', - conversationId: 'report-thread-ts', - replyTarget: { channelId: 'C1', threadId: 'report-thread-ts' }, + expect(updateBuilder.returning).toHaveBeenCalledTimes(2); + expect(slack.deleteMessage).not.toHaveBeenCalled(); + }); + + it('gives two accepted suggestions from one origin separate execution threads', async () => { + mocks.trackedMessageFindFirst + .mockResolvedValueOnce({ + id: 'tracked-message-1', + workItemId: 'work-item-1', + channelId: 'C1', + metadata: { + suggestionType: 'suggested_tasks', + launchRouting: 'router', + originSessionId: 'session-origin', }, - kickoffMessage: 'Started a task in Acme.', - }), - ); - expect(mocks.liveTaskLauncher).toHaveBeenCalledWith( - expect.objectContaining({ - userId: 'user-1', - teamId: 'T1', + }) + .mockResolvedValueOnce({ + id: 'tracked-message-2', + workItemId: 'work-item-2', channelId: 'C1', - threadTs: 'report-thread-ts', - repoForPayload: 'acme/app', - }), - ); - expect(mocks.launchTask).toHaveBeenCalledWith( - expect.objectContaining({ - prompt: 'implementation prompt', - environmentId: 'environment-1', - parentSessionId: 'fast-1', - }), - ); - expect(mocks.startFastAgentResponse).not.toHaveBeenCalled(); - expect(mocks.finalizeWorkItemLaunched).toHaveBeenCalledWith( - expect.anything(), - { id: 'work-item-1', taskId: 'task-new', claimedAt }, - ); + metadata: { + suggestionType: 'suggested_tasks', + launchRouting: 'router', + originSessionId: 'session-origin', + }, + }); + mocks.lookupSlackUserMapping.mockResolvedValue({ + hasInactiveMapping: false, + activeMapping: { userId: 'user-1' }, + }); + const slack = { + postMessage: vi + .fn() + .mockResolvedValueOnce('execution-thread-1') + .mockResolvedValueOnce('execution-thread-2'), + deleteMessage: vi.fn(async () => undefined), + addReaction: vi.fn(async () => true), + getMessageMetadata: vi.fn(), + }; + const context = { + teamId: 'T1', + slackInstallation: { botUserId: 'UROOMOTE', teamId: 'T1' }, + slack, + } as never; + + for (const cardTs of ['card-1', 'card-2']) { + await handleReactionAddedEvent({ + context, + event: { + type: 'reaction_added', + user: 'U1', + reaction: 'thumbsup', + item: { type: 'message', channel: 'C1', ts: cardTs }, + event_ts: `event-${cardTs}`, + }, + }); + } + + expect(slack.postMessage).toHaveBeenCalledTimes(2); + expect(slack.addReaction).toHaveBeenCalledTimes(2); + expect( + mocks.startFastAgentResponse.mock.calls.map( + ([input]) => input.event.thread_ts, + ), + ).toEqual(['execution-thread-1', 'execution-thread-2']); + expect(mocks.getSessionForTask).not.toHaveBeenCalled(); + expect(mocks.conversationGetOrCreate).not.toHaveBeenCalled(); }); it('forces a concrete suggestion target to coding even when Fast is the user default', async () => { diff --git a/apps/api/src/handlers/slack/events/reactions.ts b/apps/api/src/handlers/slack/events/reactions.ts index f3b8c31ba0..c1ef55c72d 100644 --- a/apps/api/src/handlers/slack/events/reactions.ts +++ b/apps/api/src/handlers/slack/events/reactions.ts @@ -33,6 +33,7 @@ import { finalizeWorkItemLaunched, releaseWorkItemClaim, sessions, + sql, trackedMessages, workItems, } from '@roomote/db/server'; @@ -171,11 +172,6 @@ const REMOVED_SLACK_ACCOUNT_LAUNCH_FAILURE = const UNLINKED_SLACK_ACCOUNT_FAST_LAUNCH_FAILURE = 'This suggestion starts in Fast mode, which needs a linked Roomote account. Link your account, then react again.'; -/** - * The Slack thread that hosts a suggestion's origin Session, when it has one - * in this workspace. A launch announces there, inside the automation's own - * report thread, instead of seeding a separate top-level thread. - */ async function resolveOriginSessionSlackThread(input: { originSessionId: string; teamId: string; @@ -243,6 +239,7 @@ async function launchTaskSuggestionTaskFromReaction({ const cardColumns = { id: true as const, workItemId: true as const, + channelId: true as const, metadata: true as const, }; @@ -536,6 +533,7 @@ async function launchTaskSuggestionTaskFromReaction({ title: workItem.title, brief: suggestionBrief, }); + const isSuggestedTask = suggestionType === 'suggested_tasks'; const seededSuggestionSlackText = buildSeededSuggestionSlackText( suggestionSlackText, reactionEvent.user, @@ -557,29 +555,114 @@ async function launchTaskSuggestionTaskFromReaction({ : null, }); - let announceMessageTs: string | undefined; - let announceChannelId = channelId; + const storedExecutionChannelId = + isSuggestedTask && + typeof suggestionCard.metadata?.executionChannelId === 'string' + ? suggestionCard.metadata.executionChannelId + : null; + const storedExecutionThreadTs = + isSuggestedTask && + typeof suggestionCard.metadata?.executionThreadTs === 'string' + ? suggestionCard.metadata.executionThreadTs + : null; + let announceMessageTs = + storedExecutionChannelId && storedExecutionThreadTs + ? storedExecutionThreadTs + : undefined; + let announceChannelId = storedExecutionChannelId ?? channelId; + const executionClaimToken = claimedAt.toISOString(); + const clearExecutionThread = async (): Promise => { + if (!announceMessageTs) { + return; + } + if (!isSuggestedTask) { + await slack + .deleteMessage({ channel: announceChannelId, ts: announceMessageTs }) + .catch(() => {}); + return; + } + const { + executionChannelId: _executionChannelId, + executionThreadTs: _executionThreadTs, + executionClaimedAt: _executionClaimedAt, + ...metadata + } = suggestionCard.metadata ?? {}; + const cleared = await db + .update(trackedMessages) + .set({ metadata, updatedAt: new Date() }) + .where( + and( + eq(trackedMessages.id, suggestionCard.id), + eq(trackedMessages.surface, 'slack'), + eq(trackedMessages.channelId, suggestionCard.channelId ?? channelId), + sql`${trackedMessages.metadata}->>'executionChannelId' = ${announceChannelId}`, + sql`${trackedMessages.metadata}->>'executionThreadTs' = ${announceMessageTs}`, + sql`${trackedMessages.metadata}->>'executionClaimedAt' = ${executionClaimToken}`, + sql`exists ( + select 1 from ${workItems} + where ${workItems.id} = ${workItemId} + and ${workItems.status} = 'launching' + and ${workItems.launchClaimedAt} = ${claimedAt} + )`, + ), + ) + .returning({ id: trackedMessages.id }) + .then(([row]) => Boolean(row)) + .catch((error) => { + apiLogger.warn( + `${logPrefix} failed to clear rejected suggestion execution thread: ${formatErrorForLog(error)}`, + ); + return false; + }); + if (cleared) { + await slack + .deleteMessage({ channel: announceChannelId, ts: announceMessageTs }) + .catch(() => {}); + } + }; let taskRun: { id: number | null; taskId: string | null } | null = null; try { - const originSessionId = await resolveSuggestionOriginSessionId( - workItem.sourceTaskId, - suggestionCard.metadata?.originSessionId, - ); + const originSessionId = isSuggestedTask + ? null + : await resolveSuggestionOriginSessionId( + workItem.sourceTaskId, + suggestionCard.metadata?.originSessionId, + ); const originThread = originSessionId ? await resolveOriginSessionSlackThread({ originSessionId, teamId }) : null; - announceChannelId = originThread?.channelId ?? channelId; - announceMessageTs = await slack.postMessage({ - channel: announceChannelId, - ...(originThread ? { thread_ts: originThread.threadTs } : {}), - text: seededSuggestionSlackText, - blocks: [ - { - type: 'markdown', - text: seededSuggestionSlackText, - }, - ], - }); + announceChannelId = + storedExecutionChannelId ?? originThread?.channelId ?? channelId; + + let postedExecutionRoot = false; + if (!announceMessageTs) { + if (isSuggestedTask) { + const acknowledged = await slack.addReaction?.({ + channel: channelId, + timestamp: messageTs, + name: ackEmoji, + }); + if (acknowledged === false) { + await releaseWorkItemClaim(db, { id: workItemId, claimedAt }); + apiLogger.warn( + `${logPrefix} failed to add the Slack launch acknowledgement`, + ); + return false; + } + } + announceMessageTs = await slack.postMessage({ + channel: announceChannelId, + ...(originThread ? { thread_ts: originThread.threadTs } : {}), + text: seededSuggestionSlackText, + blocks: [ + { + type: 'markdown', + text: seededSuggestionSlackText, + }, + ], + }); + postedExecutionRoot = Boolean(announceMessageTs && isSuggestedTask); + } if (!announceMessageTs) { await releaseWorkItemClaim(db, { id: workItemId, claimedAt }); @@ -588,6 +671,50 @@ async function launchTaskSuggestionTaskFromReaction({ ); return false; } + if (isSuggestedTask) { + const [ownedExecutionRoot] = await db + .update(trackedMessages) + .set({ + metadata: { + ...suggestionCard.metadata, + executionChannelId: announceChannelId, + executionThreadTs: announceMessageTs, + executionClaimedAt: executionClaimToken, + }, + updatedAt: new Date(), + }) + .where( + and( + eq(trackedMessages.id, suggestionCard.id), + eq(trackedMessages.surface, 'slack'), + eq( + trackedMessages.channelId, + suggestionCard.channelId ?? channelId, + ), + sql`exists ( + select 1 from ${workItems} + where ${workItems.id} = ${workItemId} + and ${workItems.status} = 'launching' + and ${workItems.launchClaimedAt} = ${claimedAt} + )`, + ), + ) + .returning({ id: trackedMessages.id }); + if (!ownedExecutionRoot) { + if (postedExecutionRoot) { + await slack + .deleteMessage({ + channel: announceChannelId, + ts: announceMessageTs, + }) + .catch(() => {}); + } + await releaseWorkItemClaim(db, { id: workItemId, claimedAt }).catch( + () => undefined, + ); + return false; + } + } const launchThreadTs = originThread?.threadTs ?? announceMessageTs; const initiator = { @@ -661,9 +788,8 @@ async function launchTaskSuggestionTaskFromReaction({ if (!suggestionWorkspace) { throw new Error('Setup suggestion workspace was not resolved.'); } - // The card already names the workspace, so the owning Session - // delegates the task straight away, without a Fast turn. The seeded - // thread is the Session's home in Slack. + // The card already names the workspace, so a new execution Session + // delegates the task straight away without a Fast turn. const workspace = suggestionWorkspace; const launchOwnerUserId = activeUserMapping?.userId ?? null; if (!launchOwnerUserId) { @@ -684,7 +810,9 @@ async function launchTaskSuggestionTaskFromReaction({ threadId: launchThreadTs, }, }, - launchId: `slack-suggestion:${workItemId}:${launchThreadTs}`, + launchId: isSuggestedTask + ? `slack-suggestion:${workItemId}` + : `slack-suggestion:${workItemId}:${launchThreadTs}`, prompt: suggestionSlackText, surface: 'slack', initiator, @@ -730,15 +858,16 @@ async function launchTaskSuggestionTaskFromReaction({ taskId, claimedAt, }), + release: async () => { + await clearExecutionThread(); + return releaseWorkItemClaim(db, { id: workItemId, claimedAt }); + }, }); if ( launchResult.status === 'rejected' || launchResult.status === 'failed' ) { - await slack - .deleteMessage({ channel: announceChannelId, ts: announceMessageTs }) - .catch(() => {}); await postSuggestionLaunchFailureMessage({ slack, channelId, @@ -759,9 +888,6 @@ async function launchTaskSuggestionTaskFromReaction({ apiLogger.warn( `${logPrefix} failed to finalize work item ${workItemId}; task ${launchResult.taskId ?? 'null'} (run ${launchResult.runId ?? 'null'}) — ${launchResult.cancelNote}`, ); - await slack - .deleteMessage({ channel: announceChannelId, ts: announceMessageTs }) - .catch(() => {}); return true; } @@ -773,7 +899,7 @@ async function launchTaskSuggestionTaskFromReaction({ and( eq(trackedMessages.id, suggestionCard.id), eq(trackedMessages.surface, 'slack'), - eq(trackedMessages.channelId, announceChannelId), + eq(trackedMessages.channelId, suggestionCard.channelId ?? channelId), ), ) .catch((error) => { @@ -787,11 +913,7 @@ async function launchTaskSuggestionTaskFromReaction({ ); return true; } catch (error) { - if (announceMessageTs) { - await slack - .deleteMessage({ channel: announceChannelId, ts: announceMessageTs }) - .catch(() => {}); - } + await clearExecutionThread(); await releaseWorkItemClaim(db, { id: workItemId, claimedAt }).catch( () => undefined, ); diff --git a/packages/slack/src/__tests__/slack-notifier.test.ts b/packages/slack/src/__tests__/slack-notifier.test.ts index 4eee629f34..e46738a3ef 100644 --- a/packages/slack/src/__tests__/slack-notifier.test.ts +++ b/packages/slack/src/__tests__/slack-notifier.test.ts @@ -870,6 +870,21 @@ describe('SlackNotifier', () => { expect(result).toBe(false); }); + + it('treats an existing reaction as a successful acknowledgement', async () => { + getGlobalWithFetch().fetch = vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ ok: false, error: 'already_reacted' }), + }); + + const result = await notifier.addReaction({ + channel: 'C123', + timestamp: '123.000', + name: 'eyes', + }); + + expect(result).toBe(true); + }); }); describe('removeReaction', () => { diff --git a/packages/slack/src/slack-notifier.ts b/packages/slack/src/slack-notifier.ts index 29c424a82a..ed8f5c0e30 100644 --- a/packages/slack/src/slack-notifier.ts +++ b/packages/slack/src/slack-notifier.ts @@ -1997,6 +1997,9 @@ export class SlackNotifier { const result: SlackResponse = await response.json(); if (!result.ok) { + if (result.error === 'already_reacted') { + return true; + } console.error( `[addReaction] Slack reactions.add error: ${result.error} - ${JSON.stringify(result)}`, );