diff --git a/apps/api/src/handlers/github/__tests__/handleInstallationCreated.test.ts b/apps/api/src/handlers/github/__tests__/handleInstallationCreated.test.ts index 2947eaf23..e90b5d917 100644 --- a/apps/api/src/handlers/github/__tests__/handleInstallationCreated.test.ts +++ b/apps/api/src/handlers/github/__tests__/handleInstallationCreated.test.ts @@ -9,7 +9,7 @@ vi.mock('@roomote/github', () => ({ })); vi.mock('@roomote/sdk/server', () => ({ - sendUserDirectMessageBestEffort: mockSendUserDirectMessage, + attemptUserDirectMessage: mockSendUserDirectMessage, })); vi.mock('@roomote/env', () => ({ @@ -26,7 +26,10 @@ const payload = { describe('handleInstallationCreated', () => { beforeEach(() => { vi.clearAllMocks(); - mockSendUserDirectMessage.mockResolvedValue(['slack']); + mockSendUserDirectMessage.mockImplementation(async ({ provider }) => ({ + provider, + status: 'sent', + })); }); it('notifies the requesting user after completing a pending installation', async () => { @@ -41,11 +44,15 @@ describe('handleInstallationCreated', () => { expect(response).toEqual({ status: 'ok' }); expect(mockCompletePendingGitHubInstallation).toHaveBeenCalledWith(42); - expect(mockSendUserDirectMessage).toHaveBeenCalledWith({ - userId: 'user-1', - text: 'Your GitHub installation request for acme-inc was approved, and Roomote is now connected. Continue setup here: https://roomote.example.com/setup', - logContext: 'handleInstallationCreated', - }); + expect(mockSendUserDirectMessage).toHaveBeenCalledTimes(4); + for (const provider of ['slack', 'teams', 'telegram', 'discord']) { + expect(mockSendUserDirectMessage).toHaveBeenCalledWith({ + provider, + userId: 'user-1', + text: 'Your GitHub installation request for acme-inc was approved, and Roomote is now connected. Continue setup here: https://roomote.example.com/setup', + logContext: 'handleInstallationCreated', + }); + } }); it('does not notify when completion fails', async () => { diff --git a/apps/api/src/handlers/github/handleInstallationCreated.ts b/apps/api/src/handlers/github/handleInstallationCreated.ts index 46433f31c..0fd9662ae 100644 --- a/apps/api/src/handlers/github/handleInstallationCreated.ts +++ b/apps/api/src/handlers/github/handleInstallationCreated.ts @@ -1,6 +1,7 @@ import { completePendingGitHubInstallation } from '@roomote/github'; -import { sendUserDirectMessageBestEffort } from '@roomote/sdk/server'; +import { attemptUserDirectMessage } from '@roomote/sdk/server'; import { Env } from '@roomote/env'; +import { communicationProviders } from '@roomote/types'; import type { WebhookResponse } from '../../types'; @@ -23,13 +24,19 @@ export async function handleInstallationCreated( if (result.success) { // The requester was waiting on a GitHub org owner's approval; let them // know on whichever chat integrations they have linked. - await sendUserDirectMessageBestEffort({ - userId: result.requestedByUserId, - text: buildInstallationApprovedMessage( - result.githubInstallation.accountLogin, + const text = buildInstallationApprovedMessage( + result.githubInstallation.accountLogin, + ); + await Promise.all( + communicationProviders.map((provider) => + attemptUserDirectMessage({ + provider, + userId: result.requestedByUserId, + text, + logContext: 'handleInstallationCreated', + }), ), - logContext: 'handleInstallationCreated', - }); + ); } } catch (error) { console.error( diff --git a/packages/sdk/src/server/index.ts b/packages/sdk/src/server/index.ts index dc92905cd..647be84a9 100644 --- a/packages/sdk/src/server/index.ts +++ b/packages/sdk/src/server/index.ts @@ -196,13 +196,11 @@ export { } from './lib/teams-primary-conversation'; export { + attemptUserDirectMessage, findSlackUserDirectMessageDestination, findUserDirectMessageDestination, - hasUserDirectMessageIdentity, - sendUserDirectMessage, - sendUserDirectMessageBestEffort, + type UserDirectMessageAttempt, type UserDirectMessageDestination, - type UserDirectMessageProvider, } from './lib/user-direct-message'; export { diff --git a/packages/sdk/src/server/lib/task-runs/record-task-message-envelope.ts b/packages/sdk/src/server/lib/task-runs/record-task-message-envelope.ts index ca91c85b1..4adffa0e8 100644 --- a/packages/sdk/src/server/lib/task-runs/record-task-message-envelope.ts +++ b/packages/sdk/src/server/lib/task-runs/record-task-message-envelope.ts @@ -30,10 +30,7 @@ import { } from '@roomote/slack'; import { createDiscordCommunicationProviderFromRuntimeCredentials } from '../discord-communication'; import { listConnectedCommunicationProviders } from '../../automations/destination'; -import { - hasUserDirectMessageIdentity, - sendUserDirectMessage, -} from '../user-direct-message'; +import { attemptUserDirectMessage } from '../user-direct-message'; import { appendManagerSlackFooter, buildAutomationSettingsMessage, @@ -242,24 +239,14 @@ async function notifyDeploymentAdminsOfPlatformIssue(params: { let deliveredAdmins = 0; for (const admin of admins) { - const linkedProviders: CommunicationProvider[] = []; + let eligible = false; for (const provider of providers) { - if (await hasUserDirectMessageIdentity(provider, admin.id)) { - linkedProviders.push(provider); - } - } - if (linkedProviders.length === 0) { - continue; - } - - eligibleAdmins += 1; - for (const provider of linkedProviders) { const slackText = buildPlatformIssueAlertText({ taskId: params.taskId, report: params.report, utmSource: provider, }); - const sent = await sendUserDirectMessage({ + const attempt = await attemptUserDirectMessage({ provider, userId: admin.id, text: @@ -269,12 +256,18 @@ async function notifyDeploymentAdminsOfPlatformIssue(params: { logContext: 'recordTaskMessageEnvelope', }); - if (sent) { + if (attempt.status !== 'unlinked') { + eligible = true; + } + if (attempt.status === 'sent') { delivered = true; deliveredAdmins += 1; break; } } + if (eligible) { + eligibleAdmins += 1; + } } return { diff --git a/packages/sdk/src/server/lib/user-direct-message.test.ts b/packages/sdk/src/server/lib/user-direct-message.test.ts index 5cbf15b2f..ec9a1b98c 100644 --- a/packages/sdk/src/server/lib/user-direct-message.test.ts +++ b/packages/sdk/src/server/lib/user-direct-message.test.ts @@ -86,10 +86,9 @@ vi.mock('./teams-primary-conversation', () => ({ import { createTelegramCommunicationProviderFromRuntimeCredentials } from './telegram-communication'; import { + attemptUserDirectMessage, findSlackUserDirectMessageDestination, findUserDirectMessageDestination, - sendUserDirectMessage, - sendUserDirectMessageBestEffort, } from './user-direct-message'; describe('findSlackUserDirectMessageDestination', () => { @@ -181,9 +180,15 @@ describe('findUserDirectMessageDestination', () => { }); }); -describe('sendUserDirectMessage', () => { +describe('attemptUserDirectMessage', () => { beforeEach(() => { vi.clearAllMocks(); + mockSlackInstallationsFindMany.mockResolvedValue([ + { botAccessToken: 'xoxb-token', teamId: 'T123' }, + ]); + mockSlackUserMappingsFindFirst.mockResolvedValue({ slackUserId: 'U123' }); + mockOpenConversation.mockResolvedValue('D123'); + mockSlackPostMessage.mockResolvedValue('1720000000.000100'); mockDiscordUserMappingsFindFirst.mockResolvedValue({ discordDmChannelId: 'discord-dm-1', discordUserId: 'discord-user-1', @@ -195,115 +200,77 @@ describe('sendUserDirectMessage', () => { it('sends to a linked Discord DM', async () => { await expect( - sendUserDirectMessage({ + attemptUserDirectMessage({ provider: 'discord', userId: 'user-1', text: 'hello', logContext: 'test', }), - ).resolves.toBe(true); + ).resolves.toEqual({ provider: 'discord', status: 'sent' }); expect(mockDiscordPostMessage).toHaveBeenCalledWith({ channelId: 'discord-dm-1', text: 'hello', textFormat: 'markdown', }); }); -}); -describe('sendUserDirectMessageBestEffort', () => { - beforeEach(() => { - vi.clearAllMocks(); + it('returns unlinked without attempting delivery when no identity exists', async () => { + mockTelegramUserMappingsFindFirst.mockResolvedValue(undefined); - mockSlackInstallationsFindMany.mockResolvedValue([ - { botAccessToken: 'xoxb-token', teamId: 'T123' }, - ]); - mockSlackUserMappingsFindFirst.mockResolvedValue({ slackUserId: 'U123' }); - mockOpenConversation.mockResolvedValue('D123'); - mockSlackPostMessage.mockResolvedValue('1720000000.000100'); - - mockTeamsUserMappingsFindFirst.mockResolvedValue({ - teamsUserId: 'teams-user-1', - teamsTenantId: 'tenant-1', - }); - mockPostDirectMessage.mockResolvedValue({ messageId: 'teams-message-1' }); + await expect( + attemptUserDirectMessage({ + provider: 'telegram', + userId: 'user-1', + text: 'hello', + logContext: 'test', + }), + ).resolves.toEqual({ provider: 'telegram', status: 'unlinked' }); + expect(mockTelegramPostMessage).not.toHaveBeenCalled(); + }); + it('returns failed when a linked provider has no credentials', async () => { mockTelegramUserMappingsFindFirst.mockResolvedValue({ telegramChatId: '424242', }); - mockTelegramPostMessage.mockResolvedValue({ messageId: '77' }); - }); - - it('sends the message on every provider with a linked identity', async () => { - const delivered = await sendUserDirectMessageBestEffort({ - userId: 'user-1', - text: 'Your GitHub installation request was approved.', - logContext: 'test', - }); - - expect(delivered).toEqual(['slack', 'teams', 'telegram']); - - expect(mockOpenConversation).toHaveBeenCalledWith('U123'); - expect(mockSlackPostMessage).toHaveBeenCalledWith({ - channel: 'D123', - text: 'Your GitHub installation request was approved.', - }); - - expect(mockPostDirectMessage).toHaveBeenCalledWith({ - serviceUrl: 'https://smba.example.com/amer/', - tenantId: 'tenant-1', - userId: 'teams-user-1', - text: 'Your GitHub installation request was approved.', - textFormat: 'markdown', - }); - - expect(mockTelegramPostMessage).toHaveBeenCalledWith({ - channelId: '424242', - text: 'Your GitHub installation request was approved.', - textFormat: 'markdown', - }); - }); - - it('skips providers the user has not linked without failing the rest', async () => { - mockSlackUserMappingsFindFirst.mockResolvedValue(undefined); - mockTeamsUserMappingsFindFirst.mockResolvedValue(undefined); - - const delivered = await sendUserDirectMessageBestEffort({ - userId: 'user-1', - text: 'hello', - logContext: 'test', - }); - - expect(delivered).toEqual(['telegram']); - expect(mockSlackPostMessage).not.toHaveBeenCalled(); - expect(mockPostDirectMessage).not.toHaveBeenCalled(); - }); - - it('skips a provider whose credentials are not configured', async () => { vi.mocked( createTelegramCommunicationProviderFromRuntimeCredentials, ).mockResolvedValueOnce(null); - const delivered = await sendUserDirectMessageBestEffort({ - userId: 'user-1', - text: 'hello', - logContext: 'test', - }); - - expect(delivered).toEqual(['slack', 'teams']); + await expect( + attemptUserDirectMessage({ + provider: 'telegram', + userId: 'user-1', + text: 'hello', + logContext: 'test', + }), + ).resolves.toEqual({ provider: 'telegram', status: 'failed' }); expect(mockTelegramPostMessage).not.toHaveBeenCalled(); }); - it('swallows a provider error and still delivers the others', async () => { + it('resolves a linked Slack identity only once before delivery', async () => { + await expect( + attemptUserDirectMessage({ + provider: 'slack', + userId: 'user-1', + text: 'hello', + logContext: 'test', + }), + ).resolves.toEqual({ provider: 'slack', status: 'sent' }); + expect(mockSlackUserMappingsFindFirst).toHaveBeenCalledTimes(1); + }); + + it('returns failed and logs when provider delivery throws', async () => { const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); mockSlackPostMessage.mockRejectedValue(new Error('slack is down')); - const delivered = await sendUserDirectMessageBestEffort({ - userId: 'user-1', - text: 'hello', - logContext: 'test', - }); - - expect(delivered).toEqual(['teams', 'telegram']); + await expect( + attemptUserDirectMessage({ + provider: 'slack', + userId: 'user-1', + text: 'hello', + logContext: 'test', + }), + ).resolves.toEqual({ provider: 'slack', status: 'failed' }); expect(warnSpy).toHaveBeenCalledWith( '[test] Failed to send Slack DM: slack is down', ); diff --git a/packages/sdk/src/server/lib/user-direct-message.ts b/packages/sdk/src/server/lib/user-direct-message.ts index b3c6ef3e8..678006723 100644 --- a/packages/sdk/src/server/lib/user-direct-message.ts +++ b/packages/sdk/src/server/lib/user-direct-message.ts @@ -16,27 +16,34 @@ import { createTeamsCommunicationProviderFromRuntimeCredentials } from './teams- import { createTelegramCommunicationProviderFromRuntimeCredentials } from './telegram-communication'; import { findTeamsPrimaryConversation } from './teams-primary-conversation'; -export type UserDirectMessageProvider = 'slack' | 'teams' | 'telegram'; - export type UserDirectMessageDestination = { channelId: string; teamId?: string; serviceUrl?: string; }; +export type UserDirectMessageAttempt = + | { provider: CommunicationProvider; status: 'unlinked' } + | { provider: CommunicationProvider; status: 'sent' } + | { provider: CommunicationProvider; status: 'failed' }; + function formatError(error: unknown) { return error instanceof Error ? error.message : String(error); } async function resolveSlackUserDirectMessage(userId: string): Promise<{ - channelId: string; - slack: SlackNotifier; - teamId: string; -} | null> { + destination: { + channelId: string; + slack: SlackNotifier; + teamId: string; + } | null; + linked: boolean; +}> { const installations = await db.query.slackInstallations.findMany({ where: eq(slackInstallations.isActive, true), columns: { botAccessToken: true, teamId: true }, }); + let linked = false; for (const installation of installations) { const mapping = await db.query.slackUserMappings.findFirst({ @@ -50,21 +57,25 @@ async function resolveSlackUserDirectMessage(userId: string): Promise<{ if (!mapping) { continue; } + linked = true; const slack = new SlackNotifier(installation.botAccessToken); const channelId = await slack.openConversation(mapping.slackUserId); if (channelId) { - return { channelId, slack, teamId: installation.teamId }; + return { + destination: { channelId, slack, teamId: installation.teamId }, + linked, + }; } } - return null; + return { destination: null, linked }; } export async function findSlackUserDirectMessageDestination( userId: string, ): Promise<{ channelId: string; teamId: string } | null> { - const destination = await resolveSlackUserDirectMessage(userId); + const { destination } = await resolveSlackUserDirectMessage(userId); return destination ? { channelId: destination.channelId, teamId: destination.teamId } : null; @@ -145,59 +156,13 @@ export async function findUserDirectMessageDestination( return null; } -export async function hasUserDirectMessageIdentity( - provider: CommunicationProvider, - userId: string, -): Promise { - switch (provider) { - case 'slack': { - const installations = await db.query.slackInstallations.findMany({ - where: eq(slackInstallations.isActive, true), - columns: { teamId: true }, - }); - for (const installation of installations) { - const mapping = await db.query.slackUserMappings.findFirst({ - where: and( - eq(slackUserMappings.userId, userId), - eq(slackUserMappings.slackTeamId, installation.teamId), - ), - columns: { slackUserId: true }, - }); - if (mapping) return true; - } - return false; - } - case 'teams': - return Boolean( - await db.query.teamsUserMappings.findFirst({ - where: eq(teamsUserMappings.userId, userId), - columns: { teamsUserId: true }, - }), - ); - case 'telegram': - return Boolean( - await db.query.telegramUserMappings.findFirst({ - where: eq(telegramUserMappings.userId, userId), - columns: { telegramChatId: true }, - }), - ); - case 'discord': - return Boolean( - await db.query.discordUserMappings.findFirst({ - where: eq(discordUserMappings.userId, userId), - columns: { discordUserId: true }, - }), - ); - } -} - async function sendSlackUserDirectMessage( userId: string, text: string, logContext: string, -): Promise { +): Promise { try { - const destination = await resolveSlackUserDirectMessage(userId); + const { destination, linked } = await resolveSlackUserDirectMessage(userId); if (destination) { const messageTs = await destination.slack.postMessage({ channel: destination.channelId, @@ -205,23 +170,25 @@ async function sendSlackUserDirectMessage( }); if (messageTs) { - return true; + return 'sent'; } } + + return linked ? 'failed' : 'unlinked'; } catch (error) { console.warn( `[${logContext}] Failed to send Slack DM: ${formatError(error)}`, ); } - return false; + return 'failed'; } async function sendTeamsUserDirectMessage( userId: string, text: string, logContext: string, -): Promise { +): Promise { try { const mapping = await db.query.teamsUserMappings.findFirst({ where: eq(teamsUserMappings.userId, userId), @@ -229,7 +196,7 @@ async function sendTeamsUserDirectMessage( }); if (!mapping) { - return false; + return 'unlinked'; } // Proactive DMs need a service URL, which lives on installations rather @@ -237,14 +204,14 @@ async function sendTeamsUserDirectMessage( const conversation = await findTeamsPrimaryConversation(); if (!conversation) { - return false; + return 'failed'; } const provider = await createTeamsCommunicationProviderFromRuntimeCredentials(); if (!provider) { - return false; + return 'failed'; } await provider.postDirectMessage({ @@ -255,13 +222,13 @@ async function sendTeamsUserDirectMessage( textFormat: 'markdown', }); - return true; + return 'sent'; } catch (error) { console.warn( `[${logContext}] Failed to send Teams DM: ${formatError(error)}`, ); - return false; + return 'failed'; } } @@ -269,7 +236,7 @@ async function sendTelegramUserDirectMessage( userId: string, text: string, logContext: string, -): Promise { +): Promise { try { const mapping = await db.query.telegramUserMappings.findFirst({ where: eq(telegramUserMappings.userId, userId), @@ -277,14 +244,14 @@ async function sendTelegramUserDirectMessage( }); if (!mapping) { - return false; + return 'unlinked'; } const provider = await createTelegramCommunicationProviderFromRuntimeCredentials(); if (!provider) { - return false; + return 'failed'; } await provider.postMessage({ @@ -293,13 +260,13 @@ async function sendTelegramUserDirectMessage( textFormat: 'markdown', }); - return true; + return 'sent'; } catch (error) { console.warn( `[${logContext}] Failed to send Telegram DM: ${formatError(error)}`, ); - return false; + return 'failed'; } } @@ -307,34 +274,41 @@ async function sendDiscordUserDirectMessage( userId: string, text: string, logContext: string, -): Promise { +): Promise { try { - const destination = await findDiscordUserDirectMessageDestination(userId); - if (!destination) { - return false; + const mapping = await db.query.discordUserMappings.findFirst({ + where: eq(discordUserMappings.userId, userId), + columns: { discordDmChannelId: true, discordUserId: true }, + }); + if (!mapping) { + return 'unlinked'; } const provider = await createDiscordCommunicationProviderFromRuntimeCredentials(); if (!provider) { - return false; + return 'failed'; } + const channelId = + mapping.discordDmChannelId ?? + (await provider.createDirectMessage(mapping.discordUserId)).id; + await provider.postMessage({ - channelId: destination.channelId, + channelId, text, textFormat: 'markdown', }); - return true; + return 'sent'; } catch (error) { console.warn( `[${logContext}] Failed to send Discord DM: ${formatError(error)}`, ); - return false; + return 'failed'; } } -export async function sendUserDirectMessage({ +export async function attemptUserDirectMessage({ provider, userId, text, @@ -344,42 +318,22 @@ export async function sendUserDirectMessage({ userId: string; text: string; logContext: string; -}): Promise { +}): Promise { + let status: UserDirectMessageAttempt['status']; switch (provider) { case 'slack': - return sendSlackUserDirectMessage(userId, text, logContext); + status = await sendSlackUserDirectMessage(userId, text, logContext); + break; case 'teams': - return sendTeamsUserDirectMessage(userId, text, logContext); + status = await sendTeamsUserDirectMessage(userId, text, logContext); + break; case 'telegram': - return sendTelegramUserDirectMessage(userId, text, logContext); + status = await sendTelegramUserDirectMessage(userId, text, logContext); + break; case 'discord': - return sendDiscordUserDirectMessage(userId, text, logContext); + status = await sendDiscordUserDirectMessage(userId, text, logContext); + break; } -} -/** - * Best-effort DM to a Roomote user on every chat integration that is both - * connected on this deployment and linked to the user. Failures are logged - * and swallowed; returns the providers that accepted the message. - */ -export async function sendUserDirectMessageBestEffort({ - userId, - text, - logContext, -}: { - userId: string; - text: string; - logContext: string; -}): Promise { - const [slack, teams, telegram] = await Promise.all([ - sendSlackUserDirectMessage(userId, text, logContext), - sendTeamsUserDirectMessage(userId, text, logContext), - sendTelegramUserDirectMessage(userId, text, logContext), - ]); - - return [ - ...(slack ? (['slack'] as const) : []), - ...(teams ? (['teams'] as const) : []), - ...(telegram ? (['telegram'] as const) : []), - ]; + return { provider, status }; }