From 23866cf1d0c92dc58fef3473f95921459cb8295c Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Fri, 7 Aug 2026 15:22:38 +0000 Subject: [PATCH] fix: harden suggestion reaction launches --- .../reactions-chat-reply-suggestions.test.ts | 85 +++++++++++++++++-- .../src/handlers/slack/events/reactions.ts | 20 ++++- .../handlers/teams/__tests__/index.test.ts | 53 ++++++++++++ .../__tests__/suggestion-start.db.test.ts | 34 +++++++- .../__tests__/callback-actions.test.ts | 40 ++++++++- ...laim-telegram-suggestion-launch.db.test.ts | 17 ++-- .../lib/__tests__/work-item-claims.test.ts | 13 +++ 7 files changed, 239 insertions(+), 23 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 f78f3ab66..bcbf3debf 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 @@ -132,6 +132,16 @@ vi.mock('./task-suggestion-reaction-contention.js', () => ({ import { handleReactionAddedEvent } from './reactions'; describe('chat reply suggestion reactions', () => { + function buildReactionEvent(eventTs: string) { + return { + type: 'reaction_added', + user: 'U1', + reaction: 'thumbsup', + item: { type: 'message', channel: 'C1', ts: 'card-ts' }, + event_ts: eventTs, + } as const; + } + beforeEach(() => { vi.clearAllMocks(); mocks.getConfiguration.mockResolvedValue(null); @@ -173,13 +183,7 @@ describe('chat reply suggestion reactions', () => { slackInstallation: { botUserId: 'UROOMOTE' }, slack, } as never, - event: { - type: 'reaction_added', - user: 'U1', - reaction: 'thumbsup', - item: { type: 'message', channel: 'C1', ts: 'card-ts' }, - event_ts: 'event-ts', - }, + event: buildReactionEvent('event-ts'), }); expect(mocks.resolveWorkspace).toHaveBeenCalledWith({ @@ -193,6 +197,11 @@ describe('chat reply suggestion reactions', () => { repo: 'acme/app', environmentId: 'environment-1', agentPromptText: 'implementation prompt', + initiator: { + kind: 'user', + externalId: 'U1', + matchedUserId: 'user-1', + }, }), ); expect(mocks.finalizeWorkItemLaunched).toHaveBeenCalledWith( @@ -211,4 +220,66 @@ describe('chat reply suggestion reactions', () => { }), ); }); + + it('requires the reacting user to have a linked Roomote account', async () => { + mocks.lookupSlackUserMapping.mockResolvedValue({ + hasInactiveMapping: false, + activeMapping: null, + }); + const slack = { + postMessage: vi.fn(async () => 'failure-message-ts'), + deleteMessage: vi.fn(async () => undefined), + getMessageMetadata: vi.fn(), + }; + + await handleReactionAddedEvent({ + context: { + teamId: 'T1', + slackInstallation: { botUserId: 'UROOMOTE' }, + slack, + } as never, + event: buildReactionEvent('unlinked-event-ts'), + }); + + expect(slack.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + text: expect.stringContaining( + 'Link your Roomote account to start tasks from Slack.', + ), + }), + ); + expect(mocks.claimWorkItem).not.toHaveBeenCalled(); + expect(mocks.startSlackAppMentionTask).not.toHaveBeenCalled(); + }); + + it('creates one task when duplicate reactions contend for the same suggestion', async () => { + mocks.claimWorkItem + .mockResolvedValueOnce({ launchClaimedAt: claimedAt }) + .mockResolvedValue(null); + const slack = { + postMessage: vi.fn(async () => 'seeded-thread-ts'), + deleteMessage: vi.fn(async () => undefined), + getMessageMetadata: vi.fn(), + }; + const context = { + teamId: 'T1', + slackInstallation: { botUserId: 'UROOMOTE' }, + slack, + } as never; + + await Promise.all([ + handleReactionAddedEvent({ + context, + event: buildReactionEvent('contention-event-1'), + }), + handleReactionAddedEvent({ + context, + event: buildReactionEvent('contention-event-2'), + }), + ]); + + expect(mocks.claimWorkItem).toHaveBeenCalledTimes(2); + expect(mocks.startSlackAppMentionTask).toHaveBeenCalledTimes(1); + expect(mocks.finalizeWorkItemLaunched).toHaveBeenCalledTimes(1); + }); }); diff --git a/apps/api/src/handlers/slack/events/reactions.ts b/apps/api/src/handlers/slack/events/reactions.ts index 3f2103777..0d484837b 100644 --- a/apps/api/src/handlers/slack/events/reactions.ts +++ b/apps/api/src/handlers/slack/events/reactions.ts @@ -172,6 +172,8 @@ async function markWorkItemLaunched(params: { const REMOVED_SLACK_ACCOUNT_LAUNCH_FAILURE = 'I could not start this because your linked Roomote account was removed. Ask an admin to restore your access, then reconnect Slack.'; +const UNLINKED_SLACK_ACCOUNT_LAUNCH_FAILURE = + 'Link your Roomote account to start tasks from Slack.'; async function launchTaskSuggestionTaskFromReaction({ teamId, @@ -403,6 +405,20 @@ async function launchTaskSuggestionTaskFromReaction({ return true; } + if (!reactingUserMapping.activeMapping) { + await postSuggestionLaunchFailureMessage({ + slack, + channelId, + title: `${buildSuggestionBadgePrefix({ + category: workItem.category, + priority: workItem.priority, + })}${workItem.title}`, + brief: suggestionBrief, + reason: UNLINKED_SLACK_ACCOUNT_LAUNCH_FAILURE, + }); + return true; + } + const claimedWorkItem = await claimWorkItem(db, { id: workItemId }); if (!claimedWorkItem) { @@ -496,9 +512,7 @@ async function launchTaskSuggestionTaskFromReaction({ initiator: { kind: 'user', externalId: reactionEvent.user, - ...(reactingUserMapping.activeMapping?.userId - ? { matchedUserId: reactingUserMapping.activeMapping.userId } - : {}), + matchedUserId: reactingUserMapping.activeMapping.userId, }, trigger: 'manual', channel: channelId, diff --git a/apps/api/src/handlers/teams/__tests__/index.test.ts b/apps/api/src/handlers/teams/__tests__/index.test.ts index f1e2756aa..17618aa9a 100644 --- a/apps/api/src/handlers/teams/__tests__/index.test.ts +++ b/apps/api/src/handlers/teams/__tests__/index.test.ts @@ -533,6 +533,59 @@ describe('Teams webhook handler', () => { expect(callViaEmojiConfigMock).not.toHaveBeenCalled(); }); + it('creates one task when duplicate likes contend for the same suggestion', async () => { + trackedSuggestionMessageFindFirstMock.mockResolvedValue({ + workItemId: 'suggestion-1', + }); + teamsUserMappingFindFirstMock.mockResolvedValue({ + userId: 'mapped-user-1', + }); + resolveAndClaimTeamsSuggestionReactionMock + .mockResolvedValueOnce({ + outcome: 'claimed', + suggestion: { + id: 'suggestion-1', + title: 'Fix the flaky test', + brief: 'Remove the timing race.', + investigationContext: null, + targetRepositoryFullName: null, + targetEnvironmentId: null, + launchClaimedAt: new Date('2026-08-07T00:00:00.000Z'), + }, + }) + .mockResolvedValue({ + outcome: 'already_started', + title: 'Fix the flaky test', + }); + + const request = (id: string) => + createApp().request('/teams', { + method: 'POST', + headers: { + authorization: 'Bearer valid-token', + 'content-type': 'application/json', + }, + body: JSON.stringify( + createTeamsActivity({ + type: 'messageReaction', + id, + text: undefined, + entities: undefined, + replyToId: 'suggestion-card-1', + reactionsAdded: [{ type: 'like' }], + }), + ), + }); + + await Promise.all([ + request('suggestion-reaction-contention-1'), + request('suggestion-reaction-contention-2'), + ]); + + expect(resolveAndClaimTeamsSuggestionReactionMock).toHaveBeenCalledTimes(2); + expect(launchClaimedTeamsSuggestionMock).toHaveBeenCalledTimes(1); + }); + it('does not claim a reaction suggestion when account mapping fails', async () => { trackedSuggestionMessageFindFirstMock.mockResolvedValue({ workItemId: 'suggestion-1', diff --git a/apps/api/src/handlers/teams/__tests__/suggestion-start.db.test.ts b/apps/api/src/handlers/teams/__tests__/suggestion-start.db.test.ts index 8787bf9d7..fe91726ca 100644 --- a/apps/api/src/handlers/teams/__tests__/suggestion-start.db.test.ts +++ b/apps/api/src/handlers/teams/__tests__/suggestion-start.db.test.ts @@ -13,7 +13,10 @@ import { workItems, } from '@roomote/db/server'; -import { resolveAndClaimTeamsSuggestionStart } from '../suggestion-start'; +import { + resolveAndClaimTeamsSuggestionReaction, + resolveAndClaimTeamsSuggestionStart, +} from '../suggestion-start'; describe('resolveAndClaimTeamsSuggestionStart (work_items launch CAS)', () => { const workItemIds: string[] = []; @@ -144,6 +147,35 @@ describe('resolveAndClaimTeamsSuggestionStart (work_items launch CAS)', () => { expect(resolution).toEqual({ outcome: 'no_cards' }); }); + it('claims a reaction card exactly once under contention', async () => { + await seedSuggestionGroup({ + introMessageId: 'reaction-card', + titles: ['Idea one'], + createdAt: new Date(), + oneMessagePerSuggestion: true, + }); + + const resolutions = await Promise.all([ + resolveAndClaimTeamsSuggestionReaction({ + conversationId, + messageId: 'reaction-card-1', + }), + resolveAndClaimTeamsSuggestionReaction({ + conversationId, + messageId: 'reaction-card-1', + }), + ]); + + expect( + resolutions.filter((resolution) => resolution.outcome === 'claimed'), + ).toHaveLength(1); + expect( + resolutions.filter( + (resolution) => resolution.outcome === 'already_started', + ), + ).toHaveLength(1); + }); + it('returns already_started when the claim CAS loses (double reply)', async () => { await seedSuggestionGroup({ introMessageId: 'intro-1', diff --git a/apps/api/src/handlers/telegram/__tests__/callback-actions.test.ts b/apps/api/src/handlers/telegram/__tests__/callback-actions.test.ts index d196961f9..983ddfb10 100644 --- a/apps/api/src/handlers/telegram/__tests__/callback-actions.test.ts +++ b/apps/api/src/handlers/telegram/__tests__/callback-actions.test.ts @@ -7,7 +7,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -import type { TelegramCallbackQuery } from '@roomote/communication/telegram-update'; +import type { + TelegramCallbackQuery, + TelegramMessageReaction, +} from '@roomote/communication/telegram-update'; const { answerCallbackMock, @@ -210,6 +213,41 @@ describe('handleTelegramCallbackQuery suggestion launch lifecycle', () => { expect(answerCallbackMock).not.toHaveBeenCalled(); }); + it('creates one task when duplicate reactions contend for the same suggestion', async () => { + const suggestion = { + id: WORK_ITEM_ID, + title: 'Fix the flaky test', + brief: 'The retry loop never terminates.', + investigationContext: null, + targetRepositoryFullName: null, + launchClaimedAt: CLAIMED_AT, + }; + claimCurrentThreadSuggestionByMessageMock + .mockResolvedValueOnce({ outcome: 'claimed', suggestion }) + .mockResolvedValue({ outcome: 'already_started' }); + startNewTelegramTaskMock.mockResolvedValue({ + status: 'started', + launchResult: { id: 7, taskId: 'task-1' }, + }); + const reaction: TelegramMessageReaction = { + chat: { id: 555, type: 'private' }, + message_id: 100, + date: 0, + user: { id: 42, first_name: 'Matt' }, + old_reaction: [], + new_reaction: [{ type: 'emoji', emoji: '👍' }], + }; + + await Promise.all([ + handleTelegramSuggestionReaction(reaction), + handleTelegramSuggestionReaction(reaction), + ]); + + expect(claimCurrentThreadSuggestionByMessageMock).toHaveBeenCalledTimes(2); + expect(startNewTelegramTaskMock).toHaveBeenCalledTimes(1); + expect(finalizeWorkItemLaunchedMock).toHaveBeenCalledTimes(1); + }); + it('starts a suggestion in a fresh topic while preserving its source topic for fallback', async () => { startNewTelegramTaskMock.mockResolvedValue({ status: 'started', diff --git a/apps/api/src/handlers/telegram/__tests__/claim-telegram-suggestion-launch.db.test.ts b/apps/api/src/handlers/telegram/__tests__/claim-telegram-suggestion-launch.db.test.ts index 97fc664e7..bc3f3ef8b 100644 --- a/apps/api/src/handlers/telegram/__tests__/claim-telegram-suggestion-launch.db.test.ts +++ b/apps/api/src/handlers/telegram/__tests__/claim-telegram-suggestion-launch.db.test.ts @@ -99,20 +99,15 @@ describe('claimTelegramSuggestionLaunch (work_items launch CAS)', () => { ); }); - it('returns null for a second claim (double-tap is a no-op)', async () => { + it('grants one claim when concurrent taps contend', async () => { const workItemId = await seedSuggestionWorkItem(); - const first = await claimTelegramSuggestionLaunch({ - suggestionId: workItemId, - chatId, - }); - const second = await claimTelegramSuggestionLaunch({ - suggestionId: workItemId, - chatId, - }); + const [first, second] = await Promise.all([ + claimTelegramSuggestionLaunch({ suggestionId: workItemId, chatId }), + claimTelegramSuggestionLaunch({ suggestionId: workItemId, chatId }), + ]); - expect(first).not.toBeNull(); - expect(second).toBeNull(); + expect([first, second].filter((claim) => claim !== null)).toHaveLength(1); }); it('does not claim a launched work item', async () => { diff --git a/packages/db/src/lib/__tests__/work-item-claims.test.ts b/packages/db/src/lib/__tests__/work-item-claims.test.ts index 10d70d52a..d033da3ce 100644 --- a/packages/db/src/lib/__tests__/work-item-claims.test.ts +++ b/packages/db/src/lib/__tests__/work-item-claims.test.ts @@ -132,6 +132,19 @@ describe('work_items launch claim helpers', () => { expect(row?.launchClaimedAt).toBeInstanceOf(Date); }); + it('grants exactly one claim under concurrent contention', async () => { + const id = await seedWorkItem(); + + const claims = await Promise.all( + Array.from({ length: 8 }, () => claimWorkItem(db, { id })), + ); + + expect(claims.filter((claim) => claim !== null)).toHaveLength(1); + const row = await readStatus(id); + expect(row?.status).toBe('launching'); + expect(row?.launchClaimedAt).toBeInstanceOf(Date); + }); + it('reclaims a stale launching item (crash recovery)', async () => { const staleClaimedAt = new Date( Date.now() - WORK_ITEM_LAUNCH_STALE_CLAIM_MS - 60_000,