diff --git a/apps/api/src/handlers/discord/__tests__/fast-agent.test.ts b/apps/api/src/handlers/discord/__tests__/fast-agent.test.ts index 1bd5a0830..30723465a 100644 --- a/apps/api/src/handlers/discord/__tests__/fast-agent.test.ts +++ b/apps/api/src/handlers/discord/__tests__/fast-agent.test.ts @@ -1,4 +1,5 @@ const mocks = vi.hoisted(() => ({ + redisState: new Map(), acquireLock: vi.fn(), answerQuestion: vi.fn(), fetchHistory: vi.fn(), @@ -23,12 +24,35 @@ vi.mock('@roomote/redis', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - // The sticky-footer lock and state live in Redis; unit tests run without - // a server, so satisfy lock acquisition and empty prior state. + // Preserve lock ownership and footer state without a Redis server. getRedis: () => ({ - set: async () => 'OK', - get: async () => null, - eval: async () => 1, + zadd: async () => 1, + set: async (key: string, value: string, ...args: unknown[]) => { + if (args.includes('NX') && mocks.redisState.has(key)) return null; + mocks.redisState.set(key, value); + return 'OK'; + }, + get: async (key: string) => mocks.redisState.get(key) ?? null, + eval: async ( + script: string, + count: number, + key: string, + ownerOrPointerKey: string, + owner?: string, + value?: string, + ttl?: string | number, + ) => { + if (count === 2) { + if (mocks.redisState.get(key) !== owner) return 0; + if (ttl !== 'keepTtl' || mocks.redisState.has(ownerOrPointerKey)) { + mocks.redisState.set(ownerOrPointerKey, value!); + } + return 1; + } + if (mocks.redisState.get(key) !== ownerOrPointerKey) return 0; + if (script.includes("'del'")) mocks.redisState.delete(key); + return 1; + }, }), }; }); @@ -127,6 +151,7 @@ describe('getDiscordFastLaunchSourceEventId', () => { describe('processDiscordFastAgentMessage', () => { beforeEach(() => { vi.clearAllMocks(); + mocks.redisState.clear(); mocks.acquireLock.mockResolvedValue(mocks.releaseLock); mocks.getSession.mockResolvedValue({ id: 'fast-session-1' }); mocks.admitHumanFollowUp.mockResolvedValue({ @@ -420,7 +445,7 @@ describe('processDiscordFastAgentMessage', () => { expect(provider.editMessage).toHaveBeenCalledWith({ channelId: 'channel-1', messageId: 'retry-1', - text: 'Connection restored.', + text: 'Connection restored.\n\n-# [Open in Roomote](https://roomote.example.com/sessions/fast-session-1?utm_source=discord&utm_medium=link&utm_campaign=discord.fast_reply)', }); expect(mocks.releaseLock).toHaveBeenCalledOnce(); }); diff --git a/apps/api/src/handlers/discord/__tests__/index.test.ts b/apps/api/src/handlers/discord/__tests__/index.test.ts index 794f76d40..35a805bbc 100644 --- a/apps/api/src/handlers/discord/__tests__/index.test.ts +++ b/apps/api/src/handlers/discord/__tests__/index.test.ts @@ -77,6 +77,7 @@ vi.mock('@roomote/redis', async (importOriginal) => { return { ...actual, getRedis: () => ({ + zadd: async () => 1, set: mocks.redisSet, eval: mocks.redisEval, get: mocks.redisGet, @@ -355,9 +356,24 @@ describe('Discord Gateway event handler', () => { messageId: 'message-1', }); mocks.postMessage.mockResolvedValue({ messageId: 'dm-msg-1' }); - mocks.redisSet.mockResolvedValue('OK'); - mocks.redisEval.mockResolvedValue(1); - mocks.redisGet.mockResolvedValue(null); + const redisState = new Map(); + mocks.redisSet.mockImplementation( + async (key: string, value: string, ...args: unknown[]) => { + if (args.includes('NX') && redisState.has(key)) return null; + redisState.set(key, value); + return 'OK'; + }, + ); + mocks.redisEval.mockImplementation( + async (script: string, _count: number, key: string, owner: string) => { + if (redisState.get(key) !== owner) return 0; + if (script.includes("'del'")) redisState.delete(key); + return 1; + }, + ); + mocks.redisGet.mockImplementation( + async (key: string) => redisState.get(key) ?? null, + ); mocks.redisGetdel.mockResolvedValue(null); mocks.redisDel.mockResolvedValue(1); mocks.component.mockResolvedValue('handled'); @@ -965,7 +981,7 @@ describe('Discord Gateway event handler', () => { expect.objectContaining({ replyToMessageId: 'message-1', text: expect.stringMatching( - /^A quick answer\n\n-# Reply or use the \[web app\]\(.*\/sessions\/fast-session-1.*\)\.$/, + /^A quick answer\n\n-# \[Open in Roomote\]\(.*\/sessions\/fast-session-1\?utm_source=discord&utm_medium=link&utm_campaign=discord.fast_reply\)$/, ), }), ); diff --git a/apps/api/src/handlers/discord/fast-agent.ts b/apps/api/src/handlers/discord/fast-agent.ts index 8b1c6b175..f5886aa5e 100644 --- a/apps/api/src/handlers/discord/fast-agent.ts +++ b/apps/api/src/handlers/discord/fast-agent.ts @@ -23,7 +23,7 @@ import { getDiscordFooterlessFinalChunk, getThreadReplyFooterRecord, resolveFastSessionReplyFooterContext, - setThreadReplyFooterRecord, + rememberThreadReplyFooterAfterEdit, withThreadReplyFooterLock, } from '@roomote/communication'; import { @@ -334,6 +334,7 @@ export async function processDiscordFastAgentMessage( textWithFooter, footerText, }), + refresh: { footerText, channelId: footerMessageChannelId }, }; }, clearPreviousFooter: async (previousFooterRecord) => { @@ -513,7 +514,7 @@ export async function processDiscordFastAgentMessage( // and this replacement would re-mark the old message as carrier. const replaced = await withThreadReplyFooterLock({ lockKey: `discord:thread_reply_footer_lock:${footerChannelId}:${footerStateThreadId}`, - fn: async () => { + fn: async (assertLock, lock) => { const footerRecord = await getThreadReplyFooterRecord( 'discord', footerChannelId, @@ -523,6 +524,7 @@ export async function processDiscordFastAgentMessage( const replacementText = isFooterCarrier ? `${text}\n\n${footerText}` : text; + await assertLock(); if (replacementText.length > DISCORD_MAX_MESSAGE_LENGTH) { const placeholder = 'Reconnected to the inference provider.'; @@ -537,12 +539,26 @@ export async function processDiscordFastAgentMessage( // The relocation that follows rewrites this message to its // stored footerless text; keep that text current so the // edit does not resurrect the pre-retry notice. - await setThreadReplyFooterRecord( - 'discord', - footerChannelId, - footerStateThreadId, - { messageId, textWithoutFooter: placeholder }, - ).catch(() => {}); + await rememberThreadReplyFooterAfterEdit({ + provider: 'discord', + channelId: footerChannelId, + threadId: footerStateThreadId, + assertLock, + lock, + record: { + ...footerRecord, + messageId, + textWithoutFooter: placeholder, + refresh: { footerText, channelId: channel.channelId }, + }, + clearOwnFooter: () => + input.provider.editMessage({ + channelId: channel.channelId, + messageId, + text: placeholder, + preserveButtons: true, + }), + }).catch(() => {}); } return false; } @@ -553,12 +569,26 @@ export async function processDiscordFastAgentMessage( text: replacementText, }); if (isFooterCarrier) { - await setThreadReplyFooterRecord( - 'discord', - footerChannelId, - footerStateThreadId, - { messageId, textWithoutFooter: text }, - ).catch(() => {}); + await rememberThreadReplyFooterAfterEdit({ + provider: 'discord', + channelId: footerChannelId, + threadId: footerStateThreadId, + assertLock, + lock, + record: { + ...footerRecord, + messageId, + textWithoutFooter: text, + refresh: { footerText, channelId: channel.channelId }, + }, + clearOwnFooter: () => + input.provider.editMessage({ + channelId: channel.channelId, + messageId, + text, + preserveButtons: true, + }), + }).catch(() => {}); } return true; }, diff --git a/apps/api/src/handlers/mcp/__tests__/communication-thread-replies.test.ts b/apps/api/src/handlers/mcp/__tests__/communication-thread-replies.test.ts index b79987608..71ee4339c 100644 --- a/apps/api/src/handlers/mcp/__tests__/communication-thread-replies.test.ts +++ b/apps/api/src/handlers/mcp/__tests__/communication-thread-replies.test.ts @@ -83,7 +83,18 @@ vi.mock('@roomote/communication', () => ({ linkedPrs: [], livePreviewUrl: null, }), - setThreadReplyFooterRecord: vi.fn(), + setThreadReplyFooterRecord: vi.fn().mockResolvedValue(true), + postTextThreadReplyWithFooter: async ({ + input, + footerText, + }: { + input: Record; + footerText: string; + }) => + postMessageMock({ + ...input, + text: [input.text, footerText].filter(Boolean).join('\n\n'), + }), })); vi.mock('@roomote/communication/chat-messages', () => ({ @@ -206,8 +217,8 @@ describe('maybeSendCommunicationThreadReply (Discord)', () => { discordEditMessageMock.mockResolvedValue(undefined); vi.mocked(buildThreadReplyFooterText).mockReturnValue(null as never); vi.mocked(getThreadReplyFooterRecord).mockResolvedValue(null); - withThreadReplyFooterLockMock.mockImplementation( - async ({ fn }: { fn: () => Promise }) => fn(), + withThreadReplyFooterLockMock.mockImplementation(async ({ fn, lockKey }) => + fn(async () => {}, { key: lockKey, ownerId: 'owner' }), ); discordAddReactionMock.mockResolvedValue({ channelId: 'thread-1', @@ -458,6 +469,16 @@ describe('maybeSendCommunicationThreadReply (Discord)', () => { { messageId: 'latest-reply', textWithoutFooter: 'Latest update', + refresh: { + channelId: 'thread-1', + footerText: '[Open task](https://app.example.com/task/task-3)', + }, + }, + { + lock: { + key: 'discord:thread_reply_footer_lock:channel-1:thread-1', + ownerId: 'owner', + }, }, ); }); @@ -481,6 +502,13 @@ describe('maybeSendCommunicationThreadReply (Discord)', () => { { messageId: 'footer-chunk', textWithoutFooter: '', + refresh: { channelId: 'thread-1', footerText: 'Task footer' }, + }, + { + lock: { + key: 'discord:thread_reply_footer_lock:channel-1:thread-1', + ownerId: 'owner', + }, }, ); }); @@ -518,8 +546,8 @@ describe('maybeSendCommunicationThreadReply (Teams)', () => { // Tests force no managed-footer path unless they override this. vi.mocked(buildThreadReplyFooterText).mockReturnValue(null as never); vi.mocked(getThreadReplyFooterRecord).mockResolvedValue(null); - withThreadReplyFooterLockMock.mockImplementation( - async ({ fn }: { fn: () => Promise }) => fn(), + withThreadReplyFooterLockMock.mockImplementation(async ({ fn, lockKey }) => + fn(async () => {}, { key: lockKey, ownerId: 'owner' }), ); vi.mocked( createTeamsCommunicationProviderFromRuntimeCredentials, @@ -569,9 +597,6 @@ describe('maybeSendCommunicationThreadReply (Teams)', () => { }, ]; - withThreadReplyFooterLockMock.mockImplementation( - async ({ fn }: { fn: () => Promise }) => fn(), - ); vi.mocked(buildThreadReplyFooterText).mockReturnValue( '[View task](https://app.example.com/task/task-2)', ); @@ -580,7 +605,7 @@ describe('maybeSendCommunicationThreadReply (Teams)', () => { textWithoutFooter: 'earlier reply with image', images: footerImages, }); - vi.mocked(setThreadReplyFooterRecord).mockResolvedValue(undefined); + vi.mocked(setThreadReplyFooterRecord).mockResolvedValue(true); postMessageMock.mockResolvedValue({ messageId: 'new-reply' }); vi.mocked( createTeamsCommunicationProviderFromRuntimeCredentials, @@ -620,6 +645,17 @@ describe('maybeSendCommunicationThreadReply (Teams)', () => { messageId: 'new-reply', textWithoutFooter: 'later update', images: footerImages, + refresh: { + channelId: '19:conversation@thread.v2', + serviceUrl: 'https://smba.trafficmanager.net/amer/', + footerText: '[View task](https://app.example.com/task/task-2)', + }, + }, + { + lock: { + key: 'teams:thread_reply_footer_lock:19:conversation@thread.v2:activity-root', + ownerId: 'owner', + }, }, ); }); @@ -633,7 +669,9 @@ describe('maybeSendCommunicationThreadReply (Telegram)', () => { getLatestInboundMessageIdMock.mockResolvedValue(null); postMessageMock.mockResolvedValue({ messageId: '999' }); sendChatActionMock.mockResolvedValue(undefined); - // Skip the footer path by returning a null footer. + vi.mocked(buildThreadReplyFooterText).mockReturnValue( + 'Current task footer', + ); }); it('prefers the latest inbound message id over the launch message id', async () => { @@ -696,7 +734,7 @@ describe('maybeSendCommunicationThreadReply (Telegram)', () => { ); }); - it('does not post the managed live-preview footer in Telegram', async () => { + it('routes Telegram replies through managed footer delivery without extra posts', async () => { await maybeSendCommunicationThreadReply({ taskRun: telegramTaskRun, parsedBody: { text: 'done', images: [] }, @@ -704,7 +742,7 @@ describe('maybeSendCommunicationThreadReply (Telegram)', () => { expect(postMessageMock).toHaveBeenCalledTimes(1); expect(postMessageMock).toHaveBeenCalledWith( - expect.objectContaining({ text: 'done' }), + expect.objectContaining({ text: 'done\n\nCurrent task footer' }), ); }); @@ -718,7 +756,10 @@ describe('maybeSendCommunicationThreadReply (Telegram)', () => { expect(response).not.toBeNull(); expect(postMessageMock).toHaveBeenCalledWith( - expect.objectContaining({ channelId: '222', text: 'done' }), + expect.objectContaining({ + channelId: '222', + text: 'done\n\nCurrent task footer', + }), ); }); }); diff --git a/apps/api/src/handlers/mcp/__tests__/communication-thread-reply-shared.test.ts b/apps/api/src/handlers/mcp/__tests__/communication-thread-reply-shared.test.ts index 99fe09224..c6498e687 100644 --- a/apps/api/src/handlers/mcp/__tests__/communication-thread-reply-shared.test.ts +++ b/apps/api/src/handlers/mcp/__tests__/communication-thread-reply-shared.test.ts @@ -44,10 +44,100 @@ import { } from '../communication-thread-reply-shared'; describe('deliverManagedThreadReplyFooter', () => { + it('rejects an initial write when the lease changes after the final check', async () => { + const lock = { key: 'lock', ownerId: 'original-owner' }; + const original = { messageId: 'original', textWithoutFooter: 'Old' }; + const competitor = { messageId: 'competitor', textWithoutFooter: 'B' }; + const posted = { messageId: 'orphan', textWithoutFooter: 'A' }; + let current = original; + let owner = lock.ownerId; + const assertLock = vi.fn(async () => { + expect(owner).toBe(lock.ownerId); + if (assertLock.mock.calls.length === 2) { + owner = 'competitor-owner'; + current = competitor; + } + }); + withThreadReplyFooterLockMock.mockImplementation(async ({ fn }) => + fn(assertLock, lock), + ); + getThreadReplyFooterRecordMock.mockImplementation(async () => current); + setThreadReplyFooterRecordMock.mockImplementation( + async (_provider, _channel, _thread, record, options) => { + if (options?.lock && options.lock.ownerId !== owner) return false; + current = record; + return true; + }, + ); + const cleanup = vi.fn().mockResolvedValue(undefined); + const warning = vi.spyOn(console, 'error').mockImplementation(() => {}); + await expect( + deliverManagedThreadReplyFooter({ + provider: 'teams', + providerLabel: 'Teams', + channelId: 'C', + footerStateThreadId: 'T', + lockKey: 'lock', + runId: 1, + logContext: 'test', + postReplyWithFooter: async () => posted, + clearPreviousFooter: cleanup, + }), + ).resolves.toEqual(posted); + expect(current).toEqual(competitor); + expect(setThreadReplyFooterRecordMock).toHaveBeenCalledExactlyOnceWith( + 'teams', + 'C', + 'T', + posted, + { lock }, + ); + expect(cleanup).toHaveBeenCalledExactlyOnceWith(posted); + expect(owner).toBe('competitor-owner'); + warning.mockRestore(); + }); + it('a lease lost after posting leaves the competitor pointer untouched and cleans only the new reply', async () => { + let owned = true; + const competitor = { messageId: 'competitor', textWithoutFooter: 'B' }; + const original = { messageId: 'original', textWithoutFooter: 'Old' }; + getThreadReplyFooterRecordMock.mockResolvedValue(original); + withThreadReplyFooterLockMock.mockImplementation(async ({ fn }) => + fn(async () => { + if (!owned) throw new Error('lease lost'); + }), + ); + const cleanup = vi.fn().mockResolvedValue(undefined); + const warning = vi.spyOn(console, 'error').mockImplementation(() => {}); + const result = await deliverManagedThreadReplyFooter({ + provider: 'teams', + providerLabel: 'Teams', + channelId: 'C', + footerStateThreadId: 'T', + lockKey: 'lock', + runId: 1, + logContext: 'test', + postReplyWithFooter: async () => { + owned = false; + getThreadReplyFooterRecordMock.mockResolvedValue(competitor); + return { messageId: 'orphan', textWithoutFooter: 'A' }; + }, + clearPreviousFooter: cleanup, + }); + expect(result.messageId).toBe('orphan'); + expect(setThreadReplyFooterRecordMock).not.toHaveBeenCalled(); + expect(cleanup).toHaveBeenCalledTimes(1); + expect(cleanup).toHaveBeenCalledWith({ + messageId: 'orphan', + textWithoutFooter: 'A', + }); + warning.mockRestore(); + }); beforeEach(() => { vi.clearAllMocks(); - withThreadReplyFooterLockMock.mockImplementation(async ({ fn }) => fn()); - setThreadReplyFooterRecordMock.mockResolvedValue(undefined); + withThreadReplyFooterLockMock.mockImplementation(async ({ fn, lockKey }) => + fn(async () => {}, { key: lockKey, ownerId: 'owner' }), + ); + setThreadReplyFooterRecordMock.mockResolvedValue(true); resolveThreadReplyFooterContextMock.mockResolvedValue({ linkedPrs: [], livePreviewUrl: null, @@ -125,6 +215,7 @@ describe('deliverManagedThreadReplyFooter', () => { }, ], }, + { lock: { key: 'lock-1', ownerId: 'owner' } }, ); expect(reply).toEqual({ messageId: 'new-message', @@ -170,6 +261,7 @@ describe('deliverManagedThreadReplyFooter', () => { messageId: 'same-message', textWithoutFooter: '', }, + { lock: { key: 'lock-1', ownerId: 'owner' } }, ); }); }); diff --git a/apps/api/src/handlers/mcp/__tests__/slack-thread-reply-quotes.test.ts b/apps/api/src/handlers/mcp/__tests__/slack-thread-reply-quotes.test.ts index a0e361d29..7b22f885a 100644 --- a/apps/api/src/handlers/mcp/__tests__/slack-thread-reply-quotes.test.ts +++ b/apps/api/src/handlers/mcp/__tests__/slack-thread-reply-quotes.test.ts @@ -21,6 +21,8 @@ const { slackInstallationFindManyMock, suppressNextSlackReplyQuoteMock, taskRunFindFirstMock, + resolveThreadReplyFooterContextMock, + buildSlackThreadFooterTextMock, } = vi.hoisted(() => ({ buildThreadReplyImageBlocksMock: vi.fn(), clearNextSlackReplyQuoteSuppressionIfIdMock: vi.fn(), @@ -39,6 +41,8 @@ const { slackInstallationFindManyMock: vi.fn(), suppressNextSlackReplyQuoteMock: vi.fn(), taskRunFindFirstMock: vi.fn(), + resolveThreadReplyFooterContextMock: vi.fn(), + buildSlackThreadFooterTextMock: vi.fn().mockReturnValue('Task footer'), })); vi.mock('@roomote/db/server', () => ({ @@ -75,7 +79,7 @@ vi.mock('@roomote/slack', async (importOriginal) => { return { ...actual, SlackPostDeliveryError: actual.SlackPostDeliveryError, - buildSlackThreadFooterText: vi.fn().mockReturnValue('Task footer'), + buildSlackThreadFooterText: buildSlackThreadFooterTextMock, buildSlackThreadReplyFooterBlock: vi.fn(({ footerText }) => ({ type: 'context', block_id: 'footer', @@ -97,7 +101,7 @@ vi.mock('@roomote/slack', async (importOriginal) => { resolveSlackThreadLinkedPrs: vi.fn().mockResolvedValue([]), ROOMOTE_THREAD_REPLY_QUOTE_BLOCK_ID: 'roomote_thread_reply_quote', setLatestSlackBotReply: vi.fn().mockResolvedValue(undefined), - setSlackThreadReplyFooterMessageTs: vi.fn(), + setSlackThreadReplyFooterMessageTs: vi.fn().mockResolvedValue(undefined), SlackNotifier: vi.fn( class { isAppInChannel = isAppInChannelMock; @@ -108,7 +112,11 @@ vi.mock('@roomote/slack', async (importOriginal) => { trackLatestUserMessageForSlackQuote: vi.fn(), trackSlackBotReply: vi.fn().mockResolvedValue(undefined), withSlackThreadReplyFooterLock: vi.fn( - async ({ fn }: { fn: () => Promise }) => fn(), + async ({ + fn, + }: { + fn: (assertLock: () => Promise) => Promise; + }) => fn(async () => {}), ), THREAD_REPLY_FOOTER_LOCK_TIMEOUT_MESSAGE: 'busy', }; @@ -120,6 +128,11 @@ vi.mock('@roomote/communication/messages', () => ({ setLatestUserMessageForReplyQuote: vi.fn(), })); +vi.mock('@roomote/communication', async (importOriginal) => ({ + ...(await importOriginal()), + resolveThreadReplyFooterContext: resolveThreadReplyFooterContextMock, +})); + vi.mock('@roomote/sdk/server', () => ({ buildAutomationIconUrl: (icon: string) => `https://app.example.com/automation-icons/${icon}.png`, @@ -166,6 +179,12 @@ vi.mock('@roomote/env', async (importOriginal) => { import { mcpAuthMiddleware } from '../middleware'; import { slackMcp } from '../slack'; import { eq } from '@roomote/db/server'; +import { + getSlackThreadReplyFooterMessageTs, + setSlackThreadReplyFooterMessageTs, + removeSlackThreadReplyFooter, + withSlackThreadReplyFooterLock, +} from '@roomote/slack'; const runToken: RunTokenContext = { runId: 42, @@ -177,7 +196,7 @@ const runToken: RunTokenContext = { function createApp() { const app = new Hono<{ Variables: Variables }>(); - app.onError(() => new Response(null, { status: 500 })); + app.onError((error) => new Response(error.message, { status: 500 })); app.use('*', async (c, next) => { c.set('authContext', runToken); await next(); @@ -188,8 +207,38 @@ function createApp() { } describe('Slack thread reply quotes', () => { + it('does not overwrite a newer reply carrier while binding an already-visible root', async () => { + taskRunFindFirstMock.mockResolvedValue({ + id: 42, + actingUserId: null, + taskId: 'task-1', + payload: { channel: 'C123', customAutomationId: 'automation-1' }, + }); + getCustomAutomationByIdMock.mockResolvedValue(null); + buildThreadReplyImageBlocksMock.mockResolvedValue([]); + vi.mocked(getSlackThreadReplyFooterMessageTs).mockResolvedValueOnce( + 'competitor', + ); + const response = await createApp().request('/mcp/thread_reply', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'Root report' }), + }); + expect(response.status, await response.text()).toBe(200); + expect(withSlackThreadReplyFooterLock).toHaveBeenCalledWith( + expect.objectContaining({ channel: 'C123', threadTs: '333.444' }), + ); + expect(setSlackThreadReplyFooterMessageTs).not.toHaveBeenCalled(); + expect(removeSlackThreadReplyFooter).toHaveBeenCalledWith( + expect.objectContaining({ messageTs: '333.444' }), + ); + }); beforeEach(() => { vi.clearAllMocks(); + resolveThreadReplyFooterContextMock.mockResolvedValue({ + linkedPrs: [], + livePreviewUrl: null, + }); getActiveSlackRunReplyTargetMock.mockResolvedValue(null); taskRunFindFirstMock.mockResolvedValue({ id: 42, @@ -419,6 +468,55 @@ describe('Slack thread reply quotes', () => { ); }); + it.each([0, 1, 2])( + 'propagates Session navigation, preview and %i running tasks into a late-bound root footer', + async (count) => { + const context = { + linkedPrs: [], + livePreviewUrl: 'https://preview.example.com', + runningTasks: { + count, + url: + count === 1 + ? 'https://app.example.com/sessions/owner?task=task-1' + : 'https://app.example.com/tasks', + }, + webAppUrl: 'https://app.example.com/sessions/owner', + }; + resolveThreadReplyFooterContextMock.mockResolvedValue(context); + taskRunFindFirstMock.mockResolvedValue({ + id: 42, + actingUserId: null, + taskId: 'task-1', + payload: { channel: 'C123', customAutomationId: 'automation-1' }, + }); + getCustomAutomationByIdMock.mockResolvedValue(null); + buildThreadReplyImageBlocksMock.mockResolvedValue([]); + const response = await createApp().request('/mcp/thread_reply', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'Root report' }), + }); + expect(response.status, await response.text()).toBe(200); + expect(resolveThreadReplyFooterContextMock).toHaveBeenCalledWith({ + taskId: 'task-1', + prRepo: null, + prNumber: null, + }); + expect(buildSlackThreadFooterTextMock).toHaveBeenCalledWith({ + ...context, + taskUrl: expect.stringContaining('/task/task-1?'), + }); + expect(postMessageDetailedMock).toHaveBeenCalledWith( + expect.objectContaining({ + blocks: expect.arrayContaining([ + expect.objectContaining({ block_id: 'footer' }), + ]), + }), + ); + }, + ); + it('preserves native charts as top-level automation report blocks', async () => { taskRunFindFirstMock.mockResolvedValue({ id: 42, diff --git a/apps/api/src/handlers/mcp/chat-reply-helpers.ts b/apps/api/src/handlers/mcp/chat-reply-helpers.ts index d6269b1bd..dd1f4227d 100644 --- a/apps/api/src/handlers/mcp/chat-reply-helpers.ts +++ b/apps/api/src/handlers/mcp/chat-reply-helpers.ts @@ -1,7 +1,9 @@ -import crypto from 'node:crypto'; import { basename } from 'node:path'; -import { getRedis } from '@roomote/redis'; +export { + withThreadReplyFooterLock, + THREAD_REPLY_FOOTER_LOCK_TIMEOUT_MESSAGE, +} from '@roomote/communication/thread-reply-footer-delivery'; import { Env, getArtifactSigningKey } from '@roomote/env'; import { db, inArray, taskArtifacts } from '@roomote/db/server'; @@ -10,15 +12,6 @@ import { currentEpochSeconds, } from '@roomote/sdk/server'; -const THREAD_REPLY_FOOTER_LOCK_TTL_SECONDS = 30; -const THREAD_REPLY_FOOTER_LOCK_MAX_ATTEMPTS = 8; -const THREAD_REPLY_FOOTER_LOCK_RETRY_MS = 100; -const RELEASE_LOCK_SCRIPT = - "if redis.call('get',KEYS[1])==ARGV[1] then return redis.call('del',KEYS[1]) else return 0 end"; - -export const THREAD_REPLY_FOOTER_LOCK_TIMEOUT_MESSAGE = - 'Timed out acquiring thread reply footer lock'; - export type ThreadReplyImage = { url: string; altText: string; @@ -151,40 +144,3 @@ export function errorResponseForThreadReplyImageError( return null; } - -export async function withThreadReplyFooterLock(params: { - lockKey: string; - maxAcquireAttempts?: number; - fn: () => Promise; -}): Promise { - const redis = getRedis(); - const maxAcquireAttempts = - params.maxAcquireAttempts ?? THREAD_REPLY_FOOTER_LOCK_MAX_ATTEMPTS; - - for (let attempt = 0; attempt < maxAcquireAttempts; attempt += 1) { - const ownerId = crypto.randomUUID(); - const acquired = await redis.set( - params.lockKey, - ownerId, - 'EX', - THREAD_REPLY_FOOTER_LOCK_TTL_SECONDS, - 'NX', - ); - - if (acquired) { - try { - return await params.fn(); - } finally { - await redis - .eval(RELEASE_LOCK_SCRIPT, 1, params.lockKey, ownerId) - .catch(() => {}); - } - } - - await new Promise((resolve) => - setTimeout(resolve, THREAD_REPLY_FOOTER_LOCK_RETRY_MS), - ); - } - - throw new Error(THREAD_REPLY_FOOTER_LOCK_TIMEOUT_MESSAGE); -} diff --git a/apps/api/src/handlers/mcp/communication-thread-replies.ts b/apps/api/src/handlers/mcp/communication-thread-replies.ts index 7981d9e97..ad091d097 100644 --- a/apps/api/src/handlers/mcp/communication-thread-replies.ts +++ b/apps/api/src/handlers/mcp/communication-thread-replies.ts @@ -4,6 +4,7 @@ import { chunkDiscordMessage, getLatestInboundMessageId, getLatestUserMessageForReplyQuote, + postTextThreadReplyWithFooter, type DiscordCommunicationProvider, type TelegramCommunicationProvider, } from '@roomote/communication'; @@ -403,6 +404,7 @@ async function sendTeamsThreadReply(params: { postReplyWithFooter: async () => ({ ...(await postTeamsReply()), textWithoutFooter: text ?? '', + refresh: { footerText, channelId, serviceUrl }, ...(images.length > 0 ? { images } : {}), }), clearPreviousFooter: async (previousFooterRecord) => { @@ -529,14 +531,23 @@ async function sendTelegramThreadReply(params: { let reply; try { - reply = await provider.postMessage({ + const footerText = await buildCommunicationThreadReplyFooterTextBestEffort({ + provider: 'telegram', + providerLabel: 'Telegram', + taskRun: params.taskRun, + logContext: LOG_CONTEXT, + }); + const input = { channelId, ...(threadId ? { threadId } : {}), replyToMessageId: replyToMessageId ?? undefined, ...(text ? { text } : {}), - textFormat: 'markdown', + textFormat: 'markdown' as const, images, - }); + }; + reply = footerText + ? await postTextThreadReplyWithFooter({ provider, input, footerText }) + : await provider.postMessage(input); } finally { stopTyping(); } @@ -667,6 +678,7 @@ async function sendDiscordThreadReply(params: { ...posted, messageId: posted.lastTextMessageId ?? posted.messageId, textWithoutFooter: footerlessFinalChunk, + refresh: { footerText, channelId: footerMessageChannelId }, }; }, clearPreviousFooter: async (previousFooterRecord) => { diff --git a/apps/api/src/handlers/mcp/communication-thread-reply-shared.ts b/apps/api/src/handlers/mcp/communication-thread-reply-shared.ts index fdd04bbe2..6fb00aa66 100644 --- a/apps/api/src/handlers/mcp/communication-thread-reply-shared.ts +++ b/apps/api/src/handlers/mcp/communication-thread-reply-shared.ts @@ -33,6 +33,7 @@ type CommunicationThreadReplyProvider = 'discord' | 'telegram' | 'teams'; type PostedFooterRecord = T & { textWithoutFooter: string; images?: ThreadReplyFooterRecord['images']; + refresh?: ThreadReplyFooterRecord['refresh']; }; function getThreadReplyWebPath(payload: unknown): string | null { @@ -159,7 +160,7 @@ export async function deliverManagedThreadReplyFooter< }): Promise { return withThreadReplyFooterLock({ lockKey: params.lockKey, - fn: async () => { + fn: async (assertLock, lock) => { let previousFooterRecord: ThreadReplyFooterRecord | null = null; try { previousFooterRecord = await getThreadReplyFooterRecord( @@ -175,42 +176,56 @@ export async function deliverManagedThreadReplyFooter< ); } + await assertLock(); const posted = await params.postReplyWithFooter(); - if ( - previousFooterRecord && - previousFooterRecord.messageId !== posted.messageId - ) { - try { - await params.clearPreviousFooter(previousFooterRecord); - } catch (error) { - console.error( - `[${params.logContext}] Failed to clear prior ${params.providerLabel} footer message ${previousFooterRecord.messageId}: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - } - } - try { - await setThreadReplyFooterRecord( + await assertLock(); + const written = await setThreadReplyFooterRecord( params.provider, params.channelId, params.footerStateThreadId, { messageId: posted.messageId, textWithoutFooter: posted.textWithoutFooter, + ...(posted.refresh ? { refresh: posted.refresh } : {}), ...(posted.images && posted.images.length > 0 ? { images: posted.images } : {}), }, + { lock }, ); + if (!written) throw new Error('Thread reply footer lock lease lost'); } catch (error) { console.error( `[${params.logContext}] Failed to persist latest ${params.providerLabel} footer record ${posted.messageId}: ${ error instanceof Error ? error.message : String(error) }`, ); + const current = await getThreadReplyFooterRecord( + params.provider, + params.channelId, + params.footerStateThreadId, + ).catch(() => undefined); + if (current !== undefined && current?.messageId !== posted.messageId) { + await params.clearPreviousFooter(posted).catch(() => {}); + } + return posted; + } + + if ( + previousFooterRecord && + previousFooterRecord.messageId !== posted.messageId + ) { + try { + await assertLock(); + await params.clearPreviousFooter(previousFooterRecord); + } catch (error) { + console.error( + `[${params.logContext}] Failed to clear prior ${params.providerLabel} footer message ${previousFooterRecord.messageId}`, + error, + ); + } } return posted; diff --git a/apps/api/src/handlers/mcp/slack.ts b/apps/api/src/handlers/mcp/slack.ts index cf0538d62..5a99ee00d 100644 --- a/apps/api/src/handlers/mcp/slack.ts +++ b/apps/api/src/handlers/mcp/slack.ts @@ -2,6 +2,7 @@ import type { ContentfulStatusCode } from 'hono/utils/http-status'; import { Hono } from 'hono'; import { Env } from '@roomote/env'; +import { resolveThreadReplyFooterContext } from '@roomote/communication'; import { and, asc, @@ -38,9 +39,9 @@ import { removeSlackThreadReplyFooter, resolveSlackThreadFooterContext, resolveSlackThreadLinkedPrs, - resolveSlackThreadLivePreviewUrl, setLatestSlackBotReply, setSlackThreadReplyFooterMessageTs, + rememberSlackThreadFooterRefresh, SlackNotifier, SlackPostDeliveryError, suppressNextSlackReplyQuote, @@ -146,22 +147,16 @@ async function buildLateBoundSlackRootFooterText(params: { taskId: string; }): Promise { // The explicit-mention marker is per-thread, so a brand-new root message - // can never carry it; only the linked PR and live preview need resolving - // here. PR metadata lives in taskPullRequests and is resolved by task id. - const [linkedPrs, livePreviewUrl] = await Promise.all([ - resolveSlackThreadLinkedPrs({ - taskId: params.taskId, - prRepo: null, - prNumber: null, - }), - resolveSlackThreadLivePreviewUrl(params.taskId), - ]); + // can never carry it. Resolve the shared Session navigation and task context. + const context = await resolveThreadReplyFooterContext({ + taskId: params.taskId, + prRepo: null, + prNumber: null, + }); return buildSlackThreadFooterText({ taskUrl: params.taskUrl, - linkedPrs, - livePreviewUrl, - explicitMentionRequired: false, + ...context, }); } @@ -1269,11 +1264,41 @@ slackMcp.post('/thread_reply', async (c) => { } if (trackRootFooterMessageTs) { - await setSlackThreadReplyFooterMessageTs( - slackReplyTarget.channel, - rootMessageTs, - rootMessageTs, - ).catch((error) => { + await withSlackThreadReplyFooterLock({ + channel: slackReplyTarget.channel, + threadTs: rootMessageTs, + fn: async (assertLock) => { + const current = await getSlackThreadReplyFooterMessageTs( + slackReplyTarget.channel, + rootMessageTs, + ); + await assertLock(); + // A reply may already have relocated the footer while the root was bound. + if (current && current !== rootMessageTs) { + await removeSlackThreadReplyFooter({ + slack: resolvedSlack, + channel: slackReplyTarget.channel, + threadTs: rootMessageTs, + messageTs: rootMessageTs, + assertLock, + }); + return; + } + await setSlackThreadReplyFooterMessageTs( + slackReplyTarget.channel, + rootMessageTs, + rootMessageTs, + ); + await rememberSlackThreadFooterRefresh( + { + slack: resolvedSlack, + channel: slackReplyTarget.channel, + threadTs: rootMessageTs, + }, + assertLock, + ); + }, + }).catch((error) => { console.error( `[slackMcp#thread_reply] Failed to persist late-bound footer message ts ${rootMessageTs}: ${ error instanceof Error ? error.message : String(error) @@ -1332,7 +1357,7 @@ slackMcp.post('/thread_reply', async (c) => { return withSlackThreadReplyFooterLock({ channel: slackReplyTarget.channel, threadTs: existingThreadTs, - fn: async () => { + fn: async (assertLock) => { const previousFooterMessageTs = await getSlackThreadReplyFooterMessageTs( slackReplyTarget.channel, @@ -1391,6 +1416,7 @@ slackMcp.post('/thread_reply', async (c) => { ); } + await assertLock(); const replyPostResult = await resolvedSlack.postMessageDetailed({ channel: slackReplyTarget.channel, thread_ts: existingThreadTs, @@ -1408,6 +1434,23 @@ slackMcp.post('/thread_reply', async (c) => { throw new SlackPostDeliveryError(replyPostResult); } + try { + await assertLock(); + } catch { + const current = await getSlackThreadReplyFooterMessageTs( + slackReplyTarget.channel, + existingThreadTs, + ).catch(() => undefined); + if (current !== undefined && current !== nextMessageTs) + await removeSlackThreadReplyFooter({ + slack: resolvedSlack, + channel: slackReplyTarget.channel, + threadTs: existingThreadTs, + messageTs: nextMessageTs, + }).catch(() => {}); + return nextMessageTs; + } + if (pendingQuote) { try { await clearLatestUserMessageForReplyQuoteIfId( @@ -1440,6 +1483,7 @@ slackMcp.post('/thread_reply', async (c) => { } try { + await assertLock(); await trackSlackBotReply( slackReplyTarget.channel, existingThreadTs, @@ -1454,6 +1498,7 @@ slackMcp.post('/thread_reply', async (c) => { } try { + await assertLock(); await setLatestSlackBotReply( slackReplyTarget.channel, existingThreadTs, @@ -1481,6 +1526,7 @@ slackMcp.post('/thread_reply', async (c) => { channel: slackReplyTarget.channel, threadTs: existingThreadTs, messageTs: previousFooterMessageTs, + assertLock, }); } catch (error) { console.error( @@ -1493,17 +1539,32 @@ slackMcp.post('/thread_reply', async (c) => { if (includeFooter) { try { + await assertLock(); await setSlackThreadReplyFooterMessageTs( slackReplyTarget.channel, existingThreadTs, nextMessageTs, ); + await rememberSlackThreadFooterRefresh( + { + slack: resolvedSlack, + channel: slackReplyTarget.channel, + threadTs: existingThreadTs, + }, + assertLock, + ); } catch (error) { console.error( `[slackMcp#thread_reply] Failed to persist latest footer message ts ${nextMessageTs}: ${ error instanceof Error ? error.message : String(error) }`, ); + const current = await getSlackThreadReplyFooterMessageTs( + slackReplyTarget.channel, + existingThreadTs, + ).catch(() => undefined); + if (current === undefined || current === nextMessageTs) + return nextMessageTs; try { await removeSlackThreadReplyFooter({ slack: resolvedSlack, diff --git a/apps/api/src/handlers/slack/dispatch/thread-reply-details-toggle.test.ts b/apps/api/src/handlers/slack/dispatch/thread-reply-details-toggle.test.ts new file mode 100644 index 000000000..22cd73790 --- /dev/null +++ b/apps/api/src/handlers/slack/dispatch/thread-reply-details-toggle.test.ts @@ -0,0 +1,170 @@ +const mocks = vi.hoisted(() => ({ + current: 'old', + owned: true, + locked: false, + busy: false, + post: vi.fn(), + getBlocks: vi.fn(), + remove: vi.fn(), + deleteMessage: vi.fn(), + setFooter: vi.fn(), + clearFooter: vi.fn(), +})); +vi.mock('@roomote/db/server', () => ({ + db: { + query: { + slackInstallations: { + findFirst: async () => ({ botAccessToken: 'test-token' }), + }, + taskSlackReplyDetails: { + findFirst: async () => ({ summary: 'Summary', findings: ['Finding'] }), + }, + }, + }, + and: vi.fn(), + eq: vi.fn(), + slackInstallations: {}, + taskSlackReplyDetails: {}, +})); +vi.mock('@roomote/slack', () => ({ + parseRoomoteSlackReplyToggleValue: () => ({ + taskId: 'task', + detailId: 'detail', + expanded: false, + }), + ROOMOTE_SLACK_REPLY_ACTIONS_BLOCK_ID: 'actions', + ROOMOTE_THREAD_REPLY_QUOTE_BLOCK_ID: 'quote', + buildRoomoteSlackReplyBlocks: () => [ + { type: 'markdown', text: 'Expanded details' }, + ], + buildRoomoteSlackReplyFallbackText: () => 'Expanded details', + SlackNotifier: class { + getMessageBlocks = mocks.getBlocks; + postMessage = mocks.post; + deleteMessage = mocks.deleteMessage; + }, + getSlackThreadReplyFooterMessageTs: async () => mocks.current, + setSlackThreadReplyFooterMessageTs: mocks.setFooter, + clearSlackThreadReplyFooterMessageTs: mocks.clearFooter, + removeSlackThreadReplyFooter: mocks.remove, + rememberSlackThreadFooterRefresh: async () => {}, + trackSlackBotReply: async () => {}, + getLatestSlackBotReply: async () => null, + setLatestSlackBotReply: async () => {}, + THREAD_REPLY_FOOTER_LOCK_TIMEOUT_MESSAGE: + 'Timed out acquiring thread reply footer lock', + withSlackThreadReplyFooterLock: async ({ + fn, + }: { + fn: (assertLock: () => Promise) => Promise; + }) => { + if (mocks.busy) + throw new Error('Timed out acquiring thread reply footer lock'); + mocks.locked = true; + try { + return await fn(async () => { + if (!mocks.owned) throw new Error('lease lost'); + }); + } finally { + mocks.locked = false; + } + }, +})); +import type { SlackInteractivePayload } from '@roomote/slack'; +import { handleThreadReplyDetailsToggle } from './thread-reply-details-toggle'; + +const payload = { + actions: [{ type: 'button', value: 'toggle' }], + team: { id: 'team' }, + channel: { id: 'C' }, + message: { ts: 'old', thread_ts: 'T' }, + response_url: 'https://hooks.slack.test/response', +} as unknown as SlackInteractivePayload; + +describe('details-toggle footer serialization', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.current = 'old'; + mocks.owned = true; + mocks.locked = false; + mocks.busy = false; + mocks.getBlocks.mockImplementation(async () => { + expect(mocks.locked).toBe(true); + return [ + { type: 'markdown', text: 'Old body' }, + { type: 'actions', block_id: 'actions' }, + { + type: 'context', + block_id: 'roomote_thread_reply_footer', + elements: [], + }, + ]; + }); + mocks.post.mockImplementation(async () => { + expect(mocks.locked).toBe(true); + return 'replacement'; + }); + mocks.deleteMessage.mockResolvedValue(true); + mocks.remove.mockResolvedValue(undefined); + mocks.setFooter.mockResolvedValue(undefined); + mocks.clearFooter.mockResolvedValue(undefined); + }); + + it('holds the delivery lock from block read through pointer handoff and deletion', async () => { + await handleThreadReplyDetailsToggle(payload); + expect(mocks.setFooter).toHaveBeenCalledWith('C', 'T', 'replacement'); + expect(mocks.deleteMessage).toHaveBeenCalledWith({ + channel: 'C', + ts: 'old', + }); + expect(mocks.setFooter.mock.invocationCallOrder[0]).toBeLessThan( + mocks.deleteMessage.mock.invocationCallOrder[0]!, + ); + }); + + it('does not move or clear a competing pointer when its provider post outlives the lease', async () => { + mocks.post.mockImplementationOnce(async () => { + mocks.owned = false; + mocks.current = 'competitor'; + return 'replacement'; + }); + await handleThreadReplyDetailsToggle(payload); + expect(mocks.setFooter).not.toHaveBeenCalled(); + expect(mocks.clearFooter).not.toHaveBeenCalled(); + expect(mocks.deleteMessage).not.toHaveBeenCalled(); + expect(mocks.remove).toHaveBeenCalledWith( + expect.objectContaining({ + channel: 'C', + threadTs: 'T', + messageTs: 'replacement', + }), + ); + expect(mocks.current).toBe('competitor'); + }); + + it('does not resurrect an old footer when toggling a historical reply', async () => { + mocks.current = 'competitor'; + await handleThreadReplyDetailsToggle(payload); + expect(mocks.post.mock.calls[0]![0].blocks).not.toContainEqual( + expect.objectContaining({ block_id: 'roomote_thread_reply_footer' }), + ); + expect(mocks.setFooter).not.toHaveBeenCalled(); + expect(mocks.clearFooter).not.toHaveBeenCalled(); + }); + + it('tells the user to retry when a reply or footer refresh holds the thread', async () => { + mocks.busy = true; + const fetch = vi + .spyOn(globalThis, 'fetch') + .mockResolvedValue(new Response('ok')); + await handleThreadReplyDetailsToggle(payload); + expect(mocks.post).not.toHaveBeenCalled(); + expect(fetch).toHaveBeenCalledWith( + 'https://hooks.slack.test/response', + expect.objectContaining({ + body: expect.stringContaining('being updated right now'), + }), + ); + fetch.mockRestore(); + }); +}); diff --git a/apps/api/src/handlers/slack/dispatch/thread-reply-details-toggle.ts b/apps/api/src/handlers/slack/dispatch/thread-reply-details-toggle.ts index 699d57678..db5047c13 100644 --- a/apps/api/src/handlers/slack/dispatch/thread-reply-details-toggle.ts +++ b/apps/api/src/handlers/slack/dispatch/thread-reply-details-toggle.ts @@ -17,7 +17,11 @@ import { setLatestSlackBotReply, setSlackThreadReplyFooterMessageTs, SlackNotifier, + THREAD_REPLY_FOOTER_LOCK_TIMEOUT_MESSAGE, trackSlackBotReply, + withSlackThreadReplyFooterLock, + removeSlackThreadReplyFooter, + rememberSlackThreadFooterRefresh, type RoomoteSlackReplyDetailRecord, type SlackInteractivePayload, } from '@roomote/slack'; @@ -29,6 +33,8 @@ const TOGGLE_RELOAD_ERROR_TEXT = "I couldn't reload that reply just now. Please try again."; const TOGGLE_UPDATE_ERROR_TEXT = "I couldn't update that reply just now. Please try again."; +const TOGGLE_BUSY_TEXT = + 'That thread is being updated right now. Please try again in a moment.'; async function findSlackReplyDetailRecord(params: { taskId: string; @@ -256,6 +262,8 @@ async function resolveThreadTs(params: { } async function syncReplacedReplyTracking(params: { + slack: SlackNotifier; + assertLock: () => Promise; channel: string; threadTs: string; previousMessageTs: string; @@ -266,6 +274,7 @@ async function syncReplacedReplyTracking(params: { const hasFooter = hasSlackFooterBlock(params.blocks); try { + await params.assertLock(); await trackSlackBotReply( params.channel, params.threadTs, @@ -286,6 +295,7 @@ async function syncReplacedReplyTracking(params: { ); if (latestReply?.ts === params.previousMessageTs) { + await params.assertLock(); await setLatestSlackBotReply( params.channel, params.threadTs, @@ -309,12 +319,14 @@ async function syncReplacedReplyTracking(params: { ); if (footerMessageTs === params.previousMessageTs) { + await params.assertLock(); if (hasFooter) { await setSlackThreadReplyFooterMessageTs( params.channel, params.threadTs, params.nextMessageTs, ); + await rememberSlackThreadFooterRefresh(params, params.assertLock); } else { await clearSlackThreadReplyFooterMessageTs( params.channel, @@ -332,6 +344,7 @@ async function syncReplacedReplyTracking(params: { } async function replaceThreadReply(params: { + assertLock: () => Promise; slack: SlackNotifier; channel: string; threadTs: string; @@ -339,6 +352,7 @@ async function replaceThreadReply(params: { text: string; blocks: unknown[]; }): Promise { + await params.assertLock(); const nextMessageTs = await params.slack.postMessage({ channel: params.channel, thread_ts: params.threadTs, @@ -350,7 +364,30 @@ async function replaceThreadReply(params: { return false; } + const lostLease = async () => { + try { + await params.assertLock(); + return false; + } catch { + const current = await getSlackThreadReplyFooterMessageTs( + params.channel, + params.threadTs, + ).catch(() => undefined); + if (current !== undefined && current !== nextMessageTs) + await removeSlackThreadReplyFooter({ + slack: params.slack, + channel: params.channel, + threadTs: params.threadTs, + messageTs: nextMessageTs, + }).catch(() => {}); + return true; + } + }; + if (await lostLease()) return true; + await syncReplacedReplyTracking({ + slack: params.slack, + assertLock: params.assertLock, channel: params.channel, threadTs: params.threadTs, previousMessageTs: params.previousMessageTs, @@ -359,6 +396,7 @@ async function replaceThreadReply(params: { blocks: params.blocks, }); + if (await lostLease()) return true; const deleted = await params.slack.deleteMessage({ channel: params.channel, ts: params.previousMessageTs, @@ -440,48 +478,80 @@ export async function handleThreadReplyDetailsToggle( return; } - const expanded = !toggleValue.expanded; - const existingBlocks = await slack.getMessageBlocks({ - channel: payload.channel.id, - messageTs: payload.message.ts, - threadTs, - }); - - if (existingBlocks === null) { - console.warn( - `[ThreadReplyDetailsToggle] Failed to load Slack blocks for message ${payload.message.ts} in thread ${threadTs} for task ${toggleValue.taskId} detail ${toggleValue.detailId}`, - ); + let updated: boolean; + try { + updated = await withSlackThreadReplyFooterLock({ + channel: payload.channel.id, + threadTs, + fn: async (assertLock) => { + const expanded = !toggleValue.expanded; + const existingBlocks = await slack.getMessageBlocks({ + channel: payload.channel.id, + messageTs: payload.message.ts, + threadTs, + }); + + if (existingBlocks === null) { + console.warn( + `[ThreadReplyDetailsToggle] Failed to load Slack blocks for message ${payload.message.ts} in thread ${threadTs} for task ${toggleValue.taskId} detail ${toggleValue.detailId}`, + ); + await postToggleErrorResponse({ + responseUrl: payload.response_url, + text: TOGGLE_RELOAD_ERROR_TEXT, + }); + return true; + } + + const blocks = buildUpdatedReplyBlocks({ + existingBlocks, + summary: detailRecord.summary, + findings: detailRecord.findings, + taskId: toggleValue.taskId, + detailId: toggleValue.detailId, + expanded, + }); + const currentFooterTs = await getSlackThreadReplyFooterMessageTs( + payload.channel.id, + threadTs, + ); + if (currentFooterTs !== payload.message.ts) { + for (let index = blocks.length - 1; index >= 0; index -= 1) { + if (isSlackFooterBlock(blocks[index])) blocks.splice(index, 1); + } + } + const text = + buildRoomoteSlackReplyFallbackText({ + summary: detailRecord.summary, + findings: detailRecord.findings, + expanded, + }) ?? 'Slack reply'; + + return replaceThreadReply({ + assertLock, + slack, + channel: payload.channel.id, + threadTs, + previousMessageTs: payload.message.ts, + text, + blocks, + }); + }, + }); + } catch (error) { + // The thread is busy (a reply or footer refresh holds it). Tell the user + // instead of letting the click fail silently. + if ( + !(error instanceof Error) || + error.message !== THREAD_REPLY_FOOTER_LOCK_TIMEOUT_MESSAGE + ) + throw error; await postToggleErrorResponse({ responseUrl: payload.response_url, - text: TOGGLE_RELOAD_ERROR_TEXT, + text: TOGGLE_BUSY_TEXT, }); return; } - const blocks = buildUpdatedReplyBlocks({ - existingBlocks, - summary: detailRecord.summary, - findings: detailRecord.findings, - taskId: toggleValue.taskId, - detailId: toggleValue.detailId, - expanded, - }); - const text = - buildRoomoteSlackReplyFallbackText({ - summary: detailRecord.summary, - findings: detailRecord.findings, - expanded, - }) ?? 'Slack reply'; - - const updated = await replaceThreadReply({ - slack, - channel: payload.channel.id, - threadTs, - previousMessageTs: payload.message.ts, - text, - blocks, - }); - if (!updated) { console.warn( `[ThreadReplyDetailsToggle] Failed to update Slack message ${payload.message.ts} in channel ${payload.channel.id} for task ${toggleValue.taskId} detail ${toggleValue.detailId}`, diff --git a/apps/api/src/handlers/slack/events/fast-agent-processing.test.ts b/apps/api/src/handlers/slack/events/fast-agent-processing.test.ts index ffb7d90f9..0198aeb38 100644 --- a/apps/api/src/handlers/slack/events/fast-agent-processing.test.ts +++ b/apps/api/src/handlers/slack/events/fast-agent-processing.test.ts @@ -1,4 +1,5 @@ const mocks = vi.hoisted(() => ({ + redisState: new Map(), acquireLock: vi.fn(), acquireRootBindingLock: vi.fn(), hasSession: vi.fn(), @@ -20,13 +21,24 @@ vi.mock('@roomote/redis', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - // The sticky-footer state lives in Redis; unit tests run without a - // server (a real client would wait on commands forever), so serve - // empty state. + // Preserve lock ownership and footer state without a Redis server. getRedis: () => ({ - set: async () => 'OK', - get: async () => null, - eval: async () => 1, + set: async (key: string, value: string, ...args: unknown[]) => { + if (args.includes('NX') && mocks.redisState.has(key)) return null; + mocks.redisState.set(key, value); + return 'OK'; + }, + get: async (key: string) => mocks.redisState.get(key) ?? null, + eval: async ( + script: string, + _count: number, + key: string, + owner: string, + ) => { + if (mocks.redisState.get(key) !== owner) return 0; + if (script.includes("'del'")) mocks.redisState.delete(key); + return 1; + }, }), }; }); @@ -107,6 +119,7 @@ function createDeferred() { describe('processFastAgentMessage', () => { beforeEach(() => { vi.clearAllMocks(); + mocks.redisState.clear(); mocks.acquireLock.mockResolvedValue(mocks.releaseLock); mocks.acquireRootBindingLock.mockResolvedValue( mocks.releaseRootBindingLock, diff --git a/apps/api/src/handlers/slack/events/fast-agent-reaction.test.ts b/apps/api/src/handlers/slack/events/fast-agent-reaction.test.ts index df7944565..5bd449135 100644 --- a/apps/api/src/handlers/slack/events/fast-agent-reaction.test.ts +++ b/apps/api/src/handlers/slack/events/fast-agent-reaction.test.ts @@ -20,6 +20,7 @@ const mocks = vi.hoisted(() => ({ })); vi.mock('@roomote/cloud-agents/server', () => ({ + FastAgentDurableRetryScheduledError: class extends Error {}, acquireFastAgentTurnLock: mocks.acquireLock, answerFastAgentQuestion: mocks.answerQuestion, buildFastAgentReactionExternalInputQuestion: vi.fn( @@ -75,7 +76,11 @@ vi.mock('@roomote/slack', () => ({ createFastAgentSlackSessionActivity: mocks.createActivity, getSlackThreadReplyFooterMessageTs: vi.fn(async () => null), withSlackThreadReplyFooterLock: vi.fn( - async ({ fn }: { fn: () => Promise }) => fn(), + async ({ + fn, + }: { + fn: (assertLock: () => Promise) => Promise; + }) => fn(async () => {}), ), })); diff --git a/apps/api/src/handlers/slack/events/message-entry-unmentioned-routing.test.ts b/apps/api/src/handlers/slack/events/message-entry-unmentioned-routing.test.ts index daefa56ea..ac1c323d2 100644 --- a/apps/api/src/handlers/slack/events/message-entry-unmentioned-routing.test.ts +++ b/apps/api/src/handlers/slack/events/message-entry-unmentioned-routing.test.ts @@ -5,7 +5,6 @@ const { hasPendingRoutingConfirmationMock, findRoomoteOwnedSlackThreadMock, markSlackThreadExplicitMentionRequiredMock, - getSlackThreadReplyFooterMessageTsMock, acquireRootBindingLockMock, releaseRootBindingLockMock, hasFastAgentSessionMock, @@ -16,7 +15,6 @@ const { hasPendingRoutingConfirmationMock: vi.fn(), findRoomoteOwnedSlackThreadMock: vi.fn(), markSlackThreadExplicitMentionRequiredMock: vi.fn(), - getSlackThreadReplyFooterMessageTsMock: vi.fn(), acquireRootBindingLockMock: vi.fn(), releaseRootBindingLockMock: vi.fn(), hasFastAgentSessionMock: vi.fn(), @@ -45,7 +43,6 @@ vi.mock('@roomote/slack', async (importOriginal) => ({ hasPendingRoutingConfirmation: hasPendingRoutingConfirmationMock, markSlackThreadExplicitMentionRequired: markSlackThreadExplicitMentionRequiredMock, - getSlackThreadReplyFooterMessageTs: getSlackThreadReplyFooterMessageTsMock, findActiveSlackTaskRun: findActiveSlackTaskRunMock, findCompletedSlackTaskRunWithSnapshot: findCompletedSlackTaskRunWithSnapshotMock, @@ -132,7 +129,6 @@ describe('shouldRouteUnmentionedSlackThreadReplyToAgent', () => { slackUserId: 'U111', }); markSlackThreadExplicitMentionRequiredMock.mockResolvedValue(undefined); - getSlackThreadReplyFooterMessageTsMock.mockResolvedValue(null); acquireRootBindingLockMock.mockResolvedValue(releaseRootBindingLockMock); releaseRootBindingLockMock.mockResolvedValue(undefined); hasFastAgentSessionMock.mockResolvedValue(false); diff --git a/apps/api/src/handlers/slack/events/message-entry.ts b/apps/api/src/handlers/slack/events/message-entry.ts index 50bd7e71b..0af9ddd5b 100644 --- a/apps/api/src/handlers/slack/events/message-entry.ts +++ b/apps/api/src/handlers/slack/events/message-entry.ts @@ -13,7 +13,6 @@ import { acquireSlackFastRootBindingLock, createFastAgentSlackLiveTaskLauncher, findActiveSlackTaskRun, - getSlackThreadReplyFooterMessageTs, isTargetSlackBotMessage, markSlackThreadExplicitMentionRequired, resolveSlackReactionNames, @@ -147,110 +146,6 @@ type UnmentionedSlackThreadReplyRoutingDecision = taskId?: string; }; -function getGroupSlackThreadReplyFooterText(text: string): string { - const genericMatch = text.match( - /^_Reply(?: with @-mention)? or use the (<[^>]+\|web app>)\._$/, - ); - - if (genericMatch?.[1]) { - return `_Reply with @-mention or use the ${genericMatch[1]}._`; - } - - return text.replace( - /^_(Working on (?:<[^>]+\|PR(?:\s+#)?\d+>(?:, <[^>]+\|live preview>)?|a <[^>]+\|live preview>)), reply(?: with @-mention)? or use the (<[^>]+\|web app>)\._$/, - '_$1, reply with @-mention or use the $2._', - ); -} - -function updateSlackThreadReplyFooterBlocksForGroupThread( - blocks: unknown[] | null, -): { blocks: unknown[]; updated: boolean } | null { - if (!blocks) { - return null; - } - - let updated = false; - const nextBlocks = blocks.map((block) => { - if (!block || typeof block !== 'object' || Array.isArray(block)) { - return block; - } - - const record = block as { - block_id?: unknown; - elements?: unknown; - text?: unknown; - }; - - if (record.block_id !== 'roomote_thread_reply_footer') { - return block; - } - - if (!Array.isArray(record.elements)) { - return block; - } - - const nextElements = record.elements.map((element) => { - if (!element || typeof element !== 'object' || Array.isArray(element)) { - return element; - } - - const elementRecord = element as { text?: unknown }; - if (typeof elementRecord.text !== 'string') { - return element; - } - - const nextText = getGroupSlackThreadReplyFooterText(elementRecord.text); - - if (nextText === elementRecord.text) { - return element; - } - - updated = true; - return { ...elementRecord, text: nextText }; - }); - - return { ...record, elements: nextElements }; - }); - - return { blocks: nextBlocks, updated }; -} - -async function updateSlackThreadReplyFooterForGroupThread(params: { - event: SlackEvent; - slack: SlackNotifier; -}): Promise { - if (!params.event.thread_ts) { - return; - } - - const footerMessageTs = await getSlackThreadReplyFooterMessageTs( - params.event.channel, - params.event.thread_ts, - ); - - if (!footerMessageTs) { - return; - } - - const footerBlocks = updateSlackThreadReplyFooterBlocksForGroupThread( - await params.slack.getMessageBlocks({ - channel: params.event.channel, - messageTs: footerMessageTs, - threadTs: params.event.thread_ts, - }), - ); - - if (!footerBlocks?.updated) { - return; - } - - await params.slack.updateMessage({ - channel: params.event.channel, - ts: footerMessageTs, - message: { blocks: footerBlocks.blocks }, - }); -} - async function markExplicitMentionRequiredSlackThread(params: { event: SlackEvent; slack: SlackNotifier; @@ -263,18 +158,6 @@ async function markExplicitMentionRequiredSlackThread(params: { params.event.channel, params.event.thread_ts, ); - - try { - await updateSlackThreadReplyFooterForGroupThread({ - event: params.event, - slack: params.slack, - }); - } catch (error) { - console.error( - `[SlackWebhook] Failed to update thread reply footer after human mention in ${params.event.channel}:${params.event.thread_ts}:`, - error instanceof Error ? error.message : String(error), - ); - } } async function markHumanMentionedSlackThread(params: { diff --git a/apps/api/src/handlers/slack/helpers/__tests__/thread-posting-charts.test.ts b/apps/api/src/handlers/slack/helpers/__tests__/thread-posting-charts.test.ts index 583623642..029b16407 100644 --- a/apps/api/src/handlers/slack/helpers/__tests__/thread-posting-charts.test.ts +++ b/apps/api/src/handlers/slack/helpers/__tests__/thread-posting-charts.test.ts @@ -22,8 +22,8 @@ vi.mock('@roomote/slack', () => ({ withSlackThreadReplyFooterLock: async ({ fn, }: { - fn: () => Promise; - }) => fn(), + fn: (assertLock: () => Promise) => Promise; + }) => fn(async () => {}), })); vi.mock('@roomote/communication', () => ({ buildFastSessionReplyFooterText: () => 'Session footer', diff --git a/apps/api/src/handlers/slack/helpers/__tests__/thread-posting-videos.test.ts b/apps/api/src/handlers/slack/helpers/__tests__/thread-posting-videos.test.ts index da3e439ed..9b8122c1d 100644 --- a/apps/api/src/handlers/slack/helpers/__tests__/thread-posting-videos.test.ts +++ b/apps/api/src/handlers/slack/helpers/__tests__/thread-posting-videos.test.ts @@ -22,8 +22,8 @@ vi.mock('@roomote/slack', () => ({ withSlackThreadReplyFooterLock: async ({ fn, }: { - fn: () => Promise; - }) => fn(), + fn: (assertLock: () => Promise) => Promise; + }) => fn(async () => {}), })); vi.mock('@roomote/communication', () => ({ buildFastSessionReplyFooterText: () => 'Session footer', diff --git a/apps/api/src/handlers/slack/helpers/thread-posting.ts b/apps/api/src/handlers/slack/helpers/thread-posting.ts index f18c29288..9b2ab4092 100644 --- a/apps/api/src/handlers/slack/helpers/thread-posting.ts +++ b/apps/api/src/handlers/slack/helpers/thread-posting.ts @@ -12,6 +12,7 @@ import { getSlackThreadReplyFooterMessageTs, postSlackThreadMessageWithFooterText, withSlackThreadReplyFooterLock, + removeSlackThreadReplyFooter, type SlackNotifier, } from '@roomote/slack'; import { @@ -125,12 +126,13 @@ export async function postSlackThreadMarkdownMessage({ const updated = await withSlackThreadReplyFooterLock({ channel, threadTs, - fn: async () => { + fn: async (assertLock) => { const footerMessageTs = await getSlackThreadReplyFooterMessageTs( channel, threadTs, ); - return slack.updateMessage({ + await assertLock(); + const updated = await slack.updateMessage({ channel, ts: messageTs, message: { @@ -150,6 +152,22 @@ export async function postSlackThreadMarkdownMessage({ ], }, }); + try { + await assertLock(); + } catch { + const current = await getSlackThreadReplyFooterMessageTs( + channel, + threadTs, + ).catch(() => undefined); + if (current !== undefined && current !== messageTs) + await removeSlackThreadReplyFooter({ + slack, + channel, + threadTs, + messageTs, + }).catch(() => {}); + } + return updated; }, }); if (!updated) { diff --git a/apps/bullmq/src/scheduled-jobs/index.ts b/apps/bullmq/src/scheduled-jobs/index.ts index a2b2061f0..b5f1dd4e9 100644 --- a/apps/bullmq/src/scheduled-jobs/index.ts +++ b/apps/bullmq/src/scheduled-jobs/index.ts @@ -10,3 +10,4 @@ export { prReviewNotificationDispatchJob } from './pr-review-notification-dispat export { brainOutboxDrainJob, brainCollectorsJob } from './brain-outbox-drain'; export { brainMaintenanceJob } from './brain-maintenance'; export { sessionsReconcileJob } from './sessions-reconcile'; +export { threadFooterRefreshJob } from './thread-footer-refresh'; diff --git a/apps/bullmq/src/scheduled-jobs/thread-footer-refresh.ts b/apps/bullmq/src/scheduled-jobs/thread-footer-refresh.ts new file mode 100644 index 000000000..fefbaeff3 --- /dev/null +++ b/apps/bullmq/src/scheduled-jobs/thread-footer-refresh.ts @@ -0,0 +1,5 @@ +import { refreshCurrentThreadFooters } from '@roomote/sdk/server'; + +export async function threadFooterRefreshJob(): Promise { + await refreshCurrentThreadFooters(); +} diff --git a/apps/bullmq/src/scheduler.test.ts b/apps/bullmq/src/scheduler.test.ts index 53c80dadf..649e70b26 100644 --- a/apps/bullmq/src/scheduler.test.ts +++ b/apps/bullmq/src/scheduler.test.ts @@ -8,6 +8,7 @@ const mocks = vi.hoisted(() => ({ }, workerConstructor: vi.fn(), queueEventsConstructor: vi.fn(), + threadFooterRefreshJob: vi.fn(), })); vi.mock('bullmq', () => ({ @@ -64,12 +65,26 @@ vi.mock('./scheduled-jobs', () => ({ brainOutboxDrainJob: vi.fn(), brainCollectorsJob: vi.fn(), brainMaintenanceJob: vi.fn(), + sessionsReconcileJob: vi.fn(), + threadFooterRefreshJob: mocks.threadFooterRefreshJob, })); import { ScheduledJobName } from './types'; import { startScheduler } from './scheduler'; describe('startScheduler', () => { + it('schedules current footer refresh every 30 seconds and dispatches it', async () => { + await startScheduler(); + expect(mocks.queue.upsertJobScheduler).toHaveBeenCalledWith( + ScheduledJobName.ThreadFooterRefresh, + { every: 30_000 }, + ); + const handler = mocks.workerConstructor.mock.calls[0]![1] as (job: { + name: string; + }) => Promise; + await handler({ name: ScheduledJobName.ThreadFooterRefresh }); + expect(mocks.threadFooterRefreshJob).toHaveBeenCalledTimes(1); + }); beforeEach(() => { vi.clearAllMocks(); mocks.queue.removeJobScheduler.mockResolvedValue(undefined); diff --git a/apps/bullmq/src/scheduler.ts b/apps/bullmq/src/scheduler.ts index 44b34dfa5..f5dd4b35f 100644 --- a/apps/bullmq/src/scheduler.ts +++ b/apps/bullmq/src/scheduler.ts @@ -36,6 +36,7 @@ import { brainCollectorsJob, brainMaintenanceJob, sessionsReconcileJob, + threadFooterRefreshJob, } from './scheduled-jobs'; const QUEUE_NAME = 'scheduled-jobs'; @@ -229,6 +230,9 @@ async function createJobs(queue: Queue): Promise { await queue.upsertJobScheduler(ScheduledJobName.SessionsReconcile, { every: 60 * 1000, }); + await queue.upsertJobScheduler(ScheduledJobName.ThreadFooterRefresh, { + every: 30 * 1000, + }); const schedulers = await queue.getJobSchedulers(); console.log('[createJobs] getJobSchedulers ->', schedulers); @@ -273,6 +277,8 @@ const runJobs = async (job: ScheduledJob): Promise => { return brainMaintenanceJob(); case ScheduledJobName.SessionsReconcile: return sessionsReconcileJob(); + case ScheduledJobName.ThreadFooterRefresh: + return threadFooterRefreshJob(); case ScheduledJobName.CustomAutomations: await customAutomationsJob(); return; diff --git a/apps/bullmq/src/types.ts b/apps/bullmq/src/types.ts index f657d0f6c..940c8c998 100644 --- a/apps/bullmq/src/types.ts +++ b/apps/bullmq/src/types.ts @@ -19,6 +19,7 @@ export enum ScheduledJobName { BrainCollectors = 'BrainCollectors', BrainMaintenance = 'BrainMaintenance', SessionsReconcile = 'SessionsReconcile', + ThreadFooterRefresh = 'ThreadFooterRefresh', } /** diff --git a/apps/docs/providers/communications/discord.mdx b/apps/docs/providers/communications/discord.mdx index d44d34f9e..26c2ec67f 100644 --- a/apps/docs/providers/communications/discord.mdx +++ b/apps/docs/providers/communications/discord.mdx @@ -146,8 +146,14 @@ request spans every repository, Fast can delegate it to the deployment's all-repositories environment. Fast automation reports can be delivered to configured Discord channels or the -automation owner's DM. Replies continue the report's Fast session in Discord, -and the footer also links to the same session in the web app. +automation owner's DM. Replies continue the report's Fast session in Discord. + +Every Fast reply ends with a compact footer: a running-task count, a **Live +preview** link when a delegated task exposes one, links to pull requests the +Session is working on, and **Open in Roomote**, which opens the Session transcript. +Roomote keeps the footer on the latest reply current as delegated tasks start +and finish (checking about every 30 seconds while work is running), and +earlier replies drop their footer when a new reply posts. Roomote does not quote every message it answers. In a task thread, the conversation itself supplies the context. When you start a task by tagging diff --git a/apps/docs/providers/communications/microsoft-teams.mdx b/apps/docs/providers/communications/microsoft-teams.mdx index e699e8ee3..0e1f25cab 100644 --- a/apps/docs/providers/communications/microsoft-teams.mdx +++ b/apps/docs/providers/communications/microsoft-teams.mdx @@ -167,7 +167,14 @@ When a task calls `request_user_input`, Teams accepts a text reply in the same conversation. Sensitive input remains in the web app; Teams does not currently render interactive answer buttons. Fast automation reports can target a Teams channel, chat, or owner direct message. Replies continue the report's Fast -session, and its footer opens the same session in the web app. +session. + +Every Fast reply ends with a compact footer: a running-task count, a **Live +preview** link when a delegated task exposes one, links to pull requests the +Session is working on, and **Open in Roomote**, which opens the Session transcript. +Roomote keeps the footer on the latest reply current as delegated tasks start +and finish (checking about every 30 seconds while work is running), and +earlier replies drop their footer when a new reply posts. The first verified Teams message also captures the conversation Roomote uses for proactive output. When Slack and Telegram are not connected, setup diff --git a/apps/docs/providers/communications/slack.mdx b/apps/docs/providers/communications/slack.mdx index b30652646..a896cc77b 100644 --- a/apps/docs/providers/communications/slack.mdx +++ b/apps/docs/providers/communications/slack.mdx @@ -245,6 +245,16 @@ channels implicitly or inspect repository contents without delegating the work to a task. When the request spans every repository, Fast can delegate it to the deployment's all-repositories environment. +Every Fast reply ends with a compact footer: a running-task count (linking to +the running task, or to the task list when several are running), a **Live +preview** link when a delegated task exposes one, links to pull requests the +Session is working on, and **Open in Roomote**, which opens the Session transcript. +Roomote keeps the footer on the latest reply current as delegated tasks start +and finish, checking about every 30 seconds while work is running, so the +thread shows whether coding is still going without another reply. Only the +newest reply carries the footer; earlier replies drop it when a new reply +posts. + Agent replies and delegated reports can include up to two native Slack charts. Ask Roomote to visualize suitable data as a pie, bar, area, or line chart. Each chart keeps a Markdown fallback for notifications and accessibility, and the diff --git a/apps/docs/providers/communications/telegram.mdx b/apps/docs/providers/communications/telegram.mdx index dd144e5bf..58bec71d8 100644 --- a/apps/docs/providers/communications/telegram.mdx +++ b/apps/docs/providers/communications/telegram.mdx @@ -123,8 +123,14 @@ Existing task chats and topics keep their active-task, `request_user_input`, and resumable-snapshot behavior. If Fast cannot start a conversation, Roomote says so in the chat instead of starting a task another way. Fast automation reports can target a Telegram chat, topic, or owner direct message; replies -continue the report's Fast session, and its footer opens the same session in -the web app. +continue the report's Fast session. + +Every Fast reply ends with a compact footer: a running-task count, a **Live +preview** link when a delegated task exposes one, links to pull requests the +Session is working on, and **Open in Roomote**, which opens the Session transcript. +Roomote keeps the footer on the latest reply current as delegated tasks start +and finish (checking about every 30 seconds while work is running), and +earlier replies drop their footer when a new reply posts. Photos are passed to Fast or the task as image input. Supported text documents are downloaded server-side and their extracted content is added to the request; diff --git a/packages/communication/package.json b/packages/communication/package.json index 457b9cd35..4f8488add 100644 --- a/packages/communication/package.json +++ b/packages/communication/package.json @@ -24,7 +24,9 @@ "./teams-provider": "./src/teams-provider.ts", "./telegram-provider": "./src/telegram-provider.ts", "./telegram-update": "./src/telegram-update.ts", - "./thread-reply-footer-state": "./src/thread-reply-footer-state.ts" + "./thread-reply-footer-state": "./src/thread-reply-footer-state.ts", + "./thread-reply-footer-delivery": "./src/thread-reply-footer-delivery.ts", + "./thread-footer-refresh": "./src/thread-footer-refresh.ts" }, "scripts": { "format": "oxfmt \"**/*.{js,jsx,ts,tsx,json,css}\"", diff --git a/packages/communication/src/__tests__/chat-messages.test.ts b/packages/communication/src/__tests__/chat-messages.test.ts index 94c5066af..66f7f151f 100644 --- a/packages/communication/src/__tests__/chat-messages.test.ts +++ b/packages/communication/src/__tests__/chat-messages.test.ts @@ -235,16 +235,7 @@ describe('chat message copy builders', () => { buildThreadReplyFooterText({ taskUrl: 'https://roomote.dev/task/123', }), - ).toBe('_Reply or use the [web app](https://roomote.dev/task/123)._'); - - expect( - buildThreadReplyFooterText({ - taskUrl: 'https://roomote.dev/task/123', - explicitMentionRequired: true, - }), - ).toBe( - '_Reply with @-mention or use the [web app](https://roomote.dev/task/123)._', - ); + ).toBe('[Open in Roomote](https://roomote.dev/task/123)'); expect( buildThreadReplyFooterText({ @@ -258,7 +249,7 @@ describe('chat message copy builders', () => { livePreviewUrl: 'https://preview.roomote.dev', }), ).toBe( - '_Working on [PR #7](https://github.com/org/repo/pull/7), [live preview](https://preview.roomote.dev), reply or use the [web app](https://roomote.dev/task/123)._', + '[Live preview](https://preview.roomote.dev) · [PR #7](https://github.com/org/repo/pull/7) · [Open in Roomote](https://roomote.dev/task/123)', ); expect( @@ -276,7 +267,7 @@ describe('chat message copy builders', () => { ], }), ).toBe( - '_Working on [PR #7](https://github.com/org/repo/pull/7) and [PR #8](https://github.com/org/other-repo/pull/8), reply or use the [web app](https://roomote.dev/task/123)._', + '[PR #7](https://github.com/org/repo/pull/7) · [PR #8](https://github.com/org/other-repo/pull/8) · [Open in Roomote](https://roomote.dev/task/123)', ); expect( @@ -286,7 +277,7 @@ describe('chat message copy builders', () => { formatLink: (label, url) => `<${url}|${label}>`, }), ).toBe( - '_Working on a , reply or use the ._', + ' · ', ); expect( @@ -294,9 +285,28 @@ describe('chat message copy builders', () => { taskUrl: 'https://roomote.dev/task/123', formatFooterText: (text) => `-# ${text}`, }), - ).toBe('-# Reply or use the [web app](https://roomote.dev/task/123).'); + ).toBe('-# [Open in Roomote](https://roomote.dev/task/123)'); }); + it.each([0, 1, 2])( + 'puts %i running tasks first, independently of preview', + (count) => { + const label = + count === 0 + ? 'No running tasks' + : `${count} running task${count === 1 ? '' : 's'}`; + expect( + buildThreadReplyFooterText({ + taskUrl: 'https://roomote.dev/sessions/1', + runningTasks: { count, url: 'https://roomote.dev/tasks' }, + livePreviewUrl: 'https://preview.roomote.dev', + }), + ).toBe( + `[${label}](https://roomote.dev/tasks) · [Live preview](https://preview.roomote.dev) · [Open in Roomote](https://roomote.dev/sessions/1)`, + ); + }, + ); + it('escapes Markdown link labels', () => { expect( formatMarkdownLink( @@ -308,6 +318,40 @@ describe('chat message copy builders', () => { ); }); + it.each([ + 'https://roomote.dev/task/task-1?artifact=notes.md&v=2&utm_source=slack', + 'https://roomote.dev/sessions/1?task=task-1&artifact=notes.md&v=2&utm_source=slack', + ])('preserves caller-provided navigation in %s', (taskUrl) => { + expect(buildThreadReplyFooterText({ taskUrl })).toBe( + `[Open in Roomote](${taskUrl})`, + ); + }); + + it('keeps non-task navigation such as setup even when the task has a Session', () => { + expect( + buildThreadReplyFooterText({ + taskUrl: 'https://roomote.dev/setup?utm_source=slack', + webAppUrl: 'https://roomote.dev/sessions/owner', + }), + ).toBe('[Open in Roomote](https://roomote.dev/setup?utm_source=slack)'); + }); + + it('opens the owning Session with the thread task selected, retaining attribution', () => { + expect( + buildThreadReplyFooterText({ + taskUrl: + 'https://roomote.dev/task/task-1?artifact=notes.md&v=2&utm_source=slack&utm_medium=link&utm_campaign=reply', + webAppUrl: 'https://roomote.dev/sessions/owner', + runningTasks: { + count: 1, + url: 'https://roomote.dev/sessions/owner?task=task-1', + }, + }), + ).toBe( + '[1 running task](https://roomote.dev/sessions/owner?task=task-1) · [Open in Roomote](https://roomote.dev/sessions/owner?utm_source=slack&utm_medium=link&utm_campaign=reply&task=task-1)', + ); + }); + it('keeps shared failure copy and PR status copy in one place', () => { expect(TASK_STARTUP_FAILURE_TEXT).toContain("couldn't get started"); expect(TASK_RUNTIME_FAILURE_TEXT).toContain('while working on this task'); diff --git a/packages/communication/src/__tests__/fast-session-footer-context.test.ts b/packages/communication/src/__tests__/fast-session-footer-context.test.ts index 71d271cad..113c08e30 100644 --- a/packages/communication/src/__tests__/fast-session-footer-context.test.ts +++ b/packages/communication/src/__tests__/fast-session-footer-context.test.ts @@ -4,10 +4,18 @@ const { getSessionForFastConversationMock, selectWhereMock, resolveThreadReplyFooterContextMock, + latestRunsMock, + latestRunsQuery, } = vi.hoisted(() => ({ getSessionForFastConversationMock: vi.fn(), selectWhereMock: vi.fn(), resolveThreadReplyFooterContextMock: vi.fn(), + latestRunsMock: vi.fn(), + latestRunsQuery: { + selectDistinctOn: vi.fn(), + where: vi.fn(), + orderBy: vi.fn(), + }, })); vi.mock('@roomote/db/server', () => ({ @@ -19,10 +27,29 @@ vi.mock('@roomote/db/server', () => ({ })), })), })), + // One DISTINCT ON query returns every linked task's latest run. + selectDistinctOn: vi.fn((...args: unknown[]) => { + latestRunsQuery.selectDistinctOn(...args); + return { + from: vi.fn(() => ({ + where: vi.fn((...whereArgs: unknown[]) => { + latestRunsQuery.where(...whereArgs); + return { + orderBy: vi.fn((...orderArgs: unknown[]) => { + latestRunsQuery.orderBy(...orderArgs); + return latestRunsMock(); + }), + }; + }), + })), + }; + }), }, and: vi.fn((...args: unknown[]) => ({ and: args })), asc: vi.fn((value: unknown) => ({ asc: value })), + desc: vi.fn((value: unknown) => ({ desc: value })), eq: vi.fn((...args: unknown[]) => ({ eq: args })), + inArray: vi.fn((...args: unknown[]) => ({ inArray: args })), getSessionForFastConversation: getSessionForFastConversationMock, isNull: vi.fn((value: unknown) => ({ isNull: value })), sessionTasks: { @@ -34,9 +61,17 @@ vi.mock('@roomote/db/server', () => ({ id: 'id', deletedAt: 'deletedAt', }, + taskRuns: { + id: 'id', + taskId: 'taskId', + status: 'status', + taskPhase: 'taskPhase', + createdAt: 'createdAt', + }, })); -vi.mock('../thread-reply-footer-context', () => ({ +vi.mock('../thread-reply-footer-context', async (importOriginal) => ({ + ...(await importOriginal()), resolveThreadReplyFooterContext: resolveThreadReplyFooterContextMock, })); @@ -45,6 +80,7 @@ vi.mock('@roomote/env', () => ({ })); import { resolveFastSessionReplyFooterContext } from '../fast-session-footer'; +import { RunStatus } from '@roomote/types'; describe('resolveFastSessionReplyFooterContext', () => { beforeEach(() => { @@ -54,6 +90,18 @@ describe('resolveFastSessionReplyFooterContext', () => { { taskId: 'task-1' }, { taskId: 'task-2' }, ]); + latestRunsMock.mockResolvedValue([ + { + taskId: 'task-1', + status: RunStatus.Idle, + taskPhase: 'waiting_for_prompt', + }, + { + taskId: 'task-2', + status: RunStatus.Idle, + taskPhase: 'waiting_for_prompt', + }, + ]); resolveThreadReplyFooterContextMock.mockImplementation( async ({ taskId }: { taskId: string }) => ({ linkedPrs: @@ -94,6 +142,8 @@ describe('resolveFastSessionReplyFooterContext', () => { }, ], livePreviewUrl: 'https://preview.example', + runningTasks: { count: 0, url: 'https://roomote.example/tasks' }, + sessionActivityAt: null, }); expect(getSessionForFastConversationMock).toHaveBeenCalledWith( @@ -102,4 +152,88 @@ describe('resolveFastSessionReplyFooterContext', () => { ); expect(resolveThreadReplyFooterContextMock).toHaveBeenCalledTimes(2); }); + + it.each([null, { id: 'session-1' }])( + 'omits status without coding-task history (%j)', + async (session) => { + getSessionForFastConversationMock.mockResolvedValue(session); + selectWhereMock.mockResolvedValue([]); + const context = await resolveFastSessionReplyFooterContext({ + sessionId: 'conversation', + }); + expect(context.runningTasks).toBeUndefined(); + expect(latestRunsMock).not.toHaveBeenCalled(); + }, + ); + + it('links one executing follow-up to its owning Session, not the Fast conversation', async () => { + latestRunsMock.mockResolvedValueOnce([ + { taskId: 'task-1', status: RunStatus.Idle, taskPhase: 'running' }, + { + taskId: 'task-2', + status: RunStatus.Idle, + taskPhase: 'waiting_for_prompt', + }, + ]); + const context = await resolveFastSessionReplyFooterContext({ + sessionId: 'conversation', + }); + expect(context.runningTasks).toEqual({ + count: 1, + url: 'https://roomote.example/sessions/session-1?task=task-1', + }); + }); + + it('links multiple executing tasks to the supported task-list route', async () => { + latestRunsMock.mockResolvedValue([ + { taskId: 'task-1', status: RunStatus.Running, taskPhase: 'running' }, + { taskId: 'task-2', status: RunStatus.Running, taskPhase: 'running' }, + ]); + const context = await resolveFastSessionReplyFooterContext({ + sessionId: 'conversation', + }); + expect(context.runningTasks).toEqual({ + count: 2, + url: 'https://roomote.example/tasks', + }); + }); + + it.each([ + { status: RunStatus.Running, taskPhase: 'waiting_for_prompt' }, + { status: RunStatus.Idle, taskPhase: 'waiting_for_prompt' }, + { status: RunStatus.Completed, taskPhase: 'running' }, + null, + ])('does not count non-executing latest runs: %j', async (run) => { + latestRunsMock.mockResolvedValue( + run + ? [ + { taskId: 'task-1', ...run }, + { taskId: 'task-2', ...run }, + ] + : [], + ); + const context = await resolveFastSessionReplyFooterContext({ + sessionId: 'conversation', + }); + expect(context.runningTasks?.count).toBe(0); + expect(context.livePreviewUrl).toBe('https://preview.example'); + }); + + it('requests every task latest run in one deterministic query, never an arbitrary older running run', async () => { + await resolveFastSessionReplyFooterContext({ sessionId: 'conversation' }); + expect(latestRunsMock).toHaveBeenCalledTimes(1); + expect(latestRunsQuery.selectDistinctOn).toHaveBeenCalledWith(['taskId'], { + taskId: 'taskId', + status: 'status', + taskPhase: 'taskPhase', + }); + expect(latestRunsQuery.where).toHaveBeenCalledWith({ + inArray: ['taskId', ['task-1', 'task-2']], + }); + expect(latestRunsQuery.orderBy).toHaveBeenCalledWith( + 'taskId', + { desc: 'createdAt' }, + { desc: 'id' }, + ); + }); }); diff --git a/packages/communication/src/__tests__/fast-session-footer.test.ts b/packages/communication/src/__tests__/fast-session-footer.test.ts index 7b5adbc53..7d0106957 100644 --- a/packages/communication/src/__tests__/fast-session-footer.test.ts +++ b/packages/communication/src/__tests__/fast-session-footer.test.ts @@ -34,7 +34,7 @@ describe('buildFastSessionReplyFooterText', () => { sessionId: '11111111-1111-4111-8111-111111111111', }); - expect(footer).toContain('Reply or use the'); + expect(footer).toContain('Open in Roomote'); expect(footer).toContain( '/sessions/11111111-1111-4111-8111-111111111111', ); @@ -46,22 +46,22 @@ describe('buildFastSessionReplyFooterText', () => { { provider: 'slack' as const, expectedPrLink: '', - expectedWebLink: '|web app>', + expectedWebLink: '|Open in Roomote>', }, { provider: 'discord' as const, expectedPrLink: '[PR #123](https://github.com/roomote/roomote/pull/123)', - expectedWebLink: '[web app](', + expectedWebLink: '[Open in Roomote](', }, { provider: 'teams' as const, expectedPrLink: '[PR #123](https://github.com/roomote/roomote/pull/123)', - expectedWebLink: '[web app](', + expectedWebLink: '[Open in Roomote](', }, { provider: 'telegram' as const, expectedPrLink: '[PR #123](https://github.com/roomote/roomote/pull/123)', - expectedWebLink: '[web app](', + expectedWebLink: '[Open in Roomote](', }, ])( 'includes a linked pull request in the $provider footer', @@ -75,7 +75,7 @@ describe('buildFastSessionReplyFooterText', () => { }, }); - expect(footer).toContain(`Working on ${expectedPrLink}`); + expect(footer).toContain(expectedPrLink); expect(footer).toContain(expectedWebLink); }, ); @@ -98,7 +98,7 @@ describe('buildFastSessionReplyFooterText', () => { }); expect(footer).toContain( - 'Working on [PR #123](https://github.com/roomote/roomote/pull/123) and [PR #456](https://github.com/roomote/roomote/pull/456), [live preview](https://preview.roomote.dev)', + '[Live preview](https://preview.roomote.dev) · [PR #123](https://github.com/roomote/roomote/pull/123) · [PR #456](https://github.com/roomote/roomote/pull/456)', ); }); @@ -116,7 +116,7 @@ describe('buildFastSessionReplyFooterText', () => { livePreviewUrl: 'https://preview.roomote.dev', }), ).toBe( - `Working on [PR #123](https://github.com/roomote/roomote/pull/123), [live preview](https://preview.roomote.dev), reply with @-mention or use the [web app](${buildFastSessionUrl('github', sessionId)}).`, + `[Live preview](https://preview.roomote.dev) · [PR #123](https://github.com/roomote/roomote/pull/123) · [Open in Roomote](${buildFastSessionUrl('github', sessionId)})`, ); }); @@ -132,6 +132,50 @@ describe('buildFastSessionReplyFooterText', () => { }); expect(footer).not.toContain('Working on'); - expect(footer).toContain('Reply or use the'); + expect(footer).toContain('Open in Roomote'); }); + + it.each([ + 'slack', + 'discord', + 'teams', + 'telegram', + 'github', + 'gitlab', + 'bitbucket', + 'ado', + 'gitea', + ] as const)( + 'keeps task selection out of the %s transcript link', + (provider) => { + const taskUrl = 'https://roomote.example/sessions/owner?task=task-1'; + const footer = buildFastSessionReplyFooterText({ + provider, + sessionId: 'conversation', + runningTasks: { count: 1, url: taskUrl }, + }); + expect(footer).toContain( + provider === 'slack' + ? `<${taskUrl}|1 running task>` + : `[1 running task](${taskUrl})`, + ); + expect( + new URL(buildFastSessionUrl(provider, 'conversation')).searchParams.has( + 'task', + ), + ).toBe(false); + expect(footer).toContain( + provider === 'slack' ? '|Open in Roomote>' : '[Open in Roomote](', + ); + expect(footer).toMatch( + provider === 'github' + ? /^/ + : provider === 'discord' + ? /^-# / + : provider === 'slack' + ? /^ { + const taskIds: string[] = []; + const sessionIds: string[] = []; + + afterEach(async () => { + if (taskIds.length) { + await db.delete(taskRuns).where(inArray(taskRuns.taskId, taskIds)); + await db + .delete(sessionTasks) + .where(inArray(sessionTasks.taskId, taskIds)); + await db.delete(tasks).where(inArray(tasks.id, taskIds)); + taskIds.length = 0; + } + if (sessionIds.length) { + await db.delete(sessions).where(inArray(sessions.id, sessionIds)); + sessionIds.length = 0; + } + }); + + it('uses only this Session and each task latest run, retaining zero after history', async () => { + const session = await sessionFactory.create(); + sessionIds.push(session.id); + expect(await resolveSessionRunningTasks(session.id)).toBeNull(); + + const first = await taskFactory.create({ state: 'active' }); + const second = await taskFactory.create({ state: 'active' }); + const unrelated = await taskFactory.create({ state: 'active' }); + taskIds.push(first.id, second.id, unrelated.id); + await db.insert(sessionTasks).values([ + { sessionId: session.id, taskId: first.id, origin: 'direct_launch' }, + { sessionId: session.id, taskId: second.id, origin: 'fast_delegation' }, + ]); + await runFactory.create({ + taskId: unrelated.id, + status: RunStatus.Running, + }); + await runFactory.create({ + taskId: first.id, + status: RunStatus.Running, + }); + // A newer waiting run supersedes the old running one even though task.state is active. + const latest = await runFactory.create({ + taskId: first.id, + status: RunStatus.Idle, + taskPhase: 'waiting_for_prompt', + }); + const listUrl = new URL('/tasks', Env.R_APP_URL).toString(); + expect(await resolveSessionRunningTasks(session.id)).toEqual({ + count: 0, + url: listUrl, + }); + + await db + .update(taskRuns) + .set({ taskPhase: 'running' }) + .where(eq(taskRuns.id, latest.id)); + const selected = new URL(`/sessions/${session.id}`, Env.R_APP_URL); + selected.searchParams.set('task', first.id); + const context = await resolveThreadReplyFooterContext({ + taskId: second.id, + prRepo: null, + prNumber: null, + }); + expect(context.runningTasks).toEqual({ + count: 1, + url: selected.toString(), + }); + expect(context.webAppUrl).toBe( + new URL(`/sessions/${session.id}`, Env.R_APP_URL).toString(), + ); + const taskUrl = new URL(`/task/${second.id}`, Env.R_APP_URL); + taskUrl.search = + 'utm_source=slack&utm_medium=link&utm_campaign=slack.thread_reply'; + expect( + buildThreadReplyFooterText({ taskUrl: taskUrl.toString(), ...context }), + ).toBe( + `[1 running task](${selected}) · [Open in Roomote](${context.webAppUrl}${taskUrl.search}&task=${second.id})`, + ); + expect( + ( + await resolveThreadReplyFooterContext({ + taskId: unrelated.id, + prRepo: null, + prNumber: null, + }) + ).webAppUrl, + ).toBeUndefined(); + + await runFactory.create({ taskId: second.id, status: RunStatus.Running }); + expect(await resolveSessionRunningTasks(session.id)).toEqual({ + count: 2, + url: listUrl, + }); + await db + .update(tasks) + .set({ deletedAt: new Date() }) + .where(eq(tasks.id, second.id)); + expect(await resolveSessionRunningTasks(session.id)).toEqual({ + count: 1, + url: selected.toString(), + }); + await db + .update(taskRuns) + .set({ status: RunStatus.Completed }) + .where(eq(taskRuns.id, latest.id)); + expect(await resolveSessionRunningTasks(session.id)).toEqual({ + count: 0, + url: listUrl, + }); + }); +}); diff --git a/packages/communication/src/__tests__/text-thread-reply-footer.test.ts b/packages/communication/src/__tests__/text-thread-reply-footer.test.ts new file mode 100644 index 000000000..ef800f622 --- /dev/null +++ b/packages/communication/src/__tests__/text-thread-reply-footer.test.ts @@ -0,0 +1,190 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { TeamsCommunicationProvider } from '../teams-provider'; +import type { TelegramCommunicationProvider } from '../telegram-provider'; + +const mocks = vi.hoisted(() => ({ + store: new Map(), + resolve: vi.fn(), +})); +vi.mock('../thread-footer-refresh', () => ({ + resolveCurrentThreadFooterText: mocks.resolve, + resolveCurrentThreadFooter: async (provider: string, footerText: string) => { + const text = await mocks.resolve(provider, footerText); + return text === null ? null : { text, active: true, settled: false }; + }, + scheduleThreadFooterRefresh: vi.fn().mockResolvedValue(undefined), + forgetThreadFooterRefresh: vi.fn(), +})); +vi.mock('@roomote/redis', () => ({ + getRedis: () => ({ + get: async (key: string) => mocks.store.get(key) ?? null, + set: async (key: string, value: string, ...args: unknown[]) => { + if (args.includes('NX') && mocks.store.has(key)) return null; + mocks.store.set(key, value); + return 'OK'; + }, + eval: async ( + script: string, + count: number, + key: string, + ...args: string[] + ) => { + const owner = args[count - 1]; + if (mocks.store.get(key) !== owner) return 0; + if (count === 2) { + const [pointerKey, , value, ttl] = args; + if (ttl !== 'keepTtl' || mocks.store.has(pointerKey!)) + mocks.store.set(pointerKey!, value!); + } else if (script.includes("'del'")) mocks.store.delete(key); + return 1; + }, + }), +})); + +import { + postTextThreadReplyWithFooter, + replaceTextThreadReplyWithFooter, +} from '../text-thread-reply-footer'; +import { getThreadReplyFooterRecord } from '../thread-reply-footer-state'; +import { refreshManagedThreadReplyFooter } from '../thread-reply-footer-delivery'; +import { editTextThreadFooterMessage } from '../text-thread-reply-footer'; + +describe('text provider current carriers', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.store.clear(); + mocks.resolve.mockResolvedValue('current footer'); + }); + + it('Teams retains attachments and service routing on refresh, relocation and current replacement', async () => { + const postMessage = vi + .fn() + .mockResolvedValueOnce({ + provider: 'teams', + channelId: 'C', + messageId: '1', + }) + .mockResolvedValueOnce({ + provider: 'teams', + channelId: 'C', + messageId: '2', + }); + const updateMessage = vi.fn().mockResolvedValue(undefined); + const provider = { + provider: 'teams', + postMessage, + updateMessage, + } as unknown as TeamsCommunicationProvider; + const images = [{ url: 'https://image', altText: 'proof' }]; + await postTextThreadReplyWithFooter({ + provider, + input: { + channelId: 'C', + threadId: 'T', + serviceUrl: 'https://service', + text: 'Body', + images, + }, + footerText: 'old footer', + }); + // Like Slack and Discord, the caller's footer is posted as written. + expect(postMessage).toHaveBeenCalledWith( + expect.objectContaining({ text: 'Body\n\nold footer', images }), + ); + expect(mocks.resolve).not.toHaveBeenCalled(); + mocks.resolve.mockResolvedValue('idle with live preview'); + await refreshManagedThreadReplyFooter({ + provider: 'teams', + channelId: 'C', + threadId: 'T', + edit: (record, text) => + editTextThreadFooterMessage(provider, record, text), + }); + expect(updateMessage).toHaveBeenLastCalledWith({ + channelId: 'C', + messageId: '1', + serviceUrl: 'https://service', + text: 'Body\n\nidle with live preview', + textFormat: 'markdown', + images, + }); + await replaceTextThreadReplyWithFooter({ + provider, + channelId: 'C', + threadId: 'T', + serviceUrl: 'https://service', + messageId: '1', + text: 'Updated body', + }); + await postTextThreadReplyWithFooter({ + provider, + input: { + channelId: 'C', + threadId: 'T', + serviceUrl: 'https://service', + text: 'New reply', + }, + footerText: 'old footer', + }); + expect(updateMessage).toHaveBeenLastCalledWith( + expect.objectContaining({ messageId: '1', text: 'Updated body', images }), + ); + await replaceTextThreadReplyWithFooter({ + provider, + channelId: 'C', + threadId: 'T', + serviceUrl: 'https://service', + messageId: '1', + text: 'Historical update', + }); + expect(updateMessage).toHaveBeenLastCalledWith( + expect.objectContaining({ messageId: '1', text: 'Historical update' }), + ); + expect( + (await getThreadReplyFooterRecord('teams', 'C', 'T'))?.messageId, + ).toBe('2'); + }); + + it('Telegram records the final text chunk and preserves its buttons on refresh', async () => { + const postMessage = vi.fn().mockResolvedValue({ + provider: 'telegram', + channelId: 'C', + messageId: 'first', + lastTextMessageId: 'last', + }); + const editMessageText = vi.fn().mockResolvedValue(undefined); + const provider = { + provider: 'telegram', + postMessage, + editMessageText, + } as unknown as TelegramCommunicationProvider; + const buttons = [[{ text: 'Open', url: 'https://app' }]]; + const body = `${'Long narrative. '.repeat(400)}\n\nFinal paragraph`; + const posted = await postTextThreadReplyWithFooter({ + provider, + input: { channelId: 'C', text: body, buttons }, + footerText: 'old footer', + }); + expect(posted.messageId).toBe('last'); + const record = await getThreadReplyFooterRecord('telegram', 'C', 'root'); + expect(record?.textWithoutFooter.endsWith('Final paragraph')).toBe(true); + expect(record?.textWithoutFooter.length).toBeLessThan(body.length); + expect(record?.buttons).toEqual(buttons); + mocks.resolve.mockResolvedValue('No running tasks'); + await refreshManagedThreadReplyFooter({ + provider: 'telegram', + channelId: 'C', + threadId: 'root', + edit: (current, text) => + editTextThreadFooterMessage(provider, current, text), + }); + expect(editMessageText).toHaveBeenCalledWith({ + channelId: 'C', + messageId: 'last', + text: `${record?.textWithoutFooter}\n\nNo running tasks`, + textFormat: 'markdown', + buttons, + }); + expect(postMessage).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/communication/src/__tests__/thread-footer-refresh-delivery.test.ts b/packages/communication/src/__tests__/thread-footer-refresh-delivery.test.ts new file mode 100644 index 000000000..d88a3407f --- /dev/null +++ b/packages/communication/src/__tests__/thread-footer-refresh-delivery.test.ts @@ -0,0 +1,596 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const { + store, + get, + resolveFooter, + activity, + schedule, + forget, + renew, + failures, +} = vi.hoisted(() => ({ + store: new Map(), + get: vi.fn(), + resolveFooter: vi.fn(), + activity: { active: true, settled: false }, + schedule: vi.fn().mockResolvedValue(undefined), + forget: vi.fn(), + renew: vi.fn(), + failures: { recordWrite: false }, +})); +vi.mock('../thread-footer-refresh', () => ({ + resolveCurrentThreadFooterText: resolveFooter, + resolveCurrentThreadFooter: async (provider: string, footerText: string) => { + const text = await resolveFooter(provider, footerText); + return text === null ? null : { text, ...activity }; + }, + scheduleThreadFooterRefresh: schedule, + forgetThreadFooterRefresh: forget, +})); +vi.mock('@roomote/redis', () => ({ + getRedis: () => ({ + get, + set: async (key: string, value: string, ...args: unknown[]) => { + if (failures.recordWrite && key.includes(':thread_reply_footer:')) + throw new Error('Redis write unavailable'); + if ( + (args.includes('NX') && store.has(key)) || + (args.includes('XX') && !store.has(key)) + ) + return null; + store.set(key, value); + return 'OK'; + }, + eval: async ( + script: string, + count: number, + key: string, + ...args: string[] + ) => { + const owner = args[count - 1]; + if (store.get(key) !== owner) return 0; + if (count === 2) { + if (failures.recordWrite) throw new Error('Redis write unavailable'); + const [pointerKey, , value, ttl] = args; + if (ttl === 'keepTtl' && !store.has(pointerKey!)) return 0; + store.set(pointerKey!, value!); + return 1; + } + if (script.includes("'expire'")) renew(); + if (script.includes("'del'")) store.delete(key); + return 1; + }, + }), +})); + +import { + deliverManagedThreadReplyFooter, + refreshManagedThreadReplyFooter, + rememberThreadReplyFooterAfterEdit, + withThreadReplyFooterLock, +} from '../thread-reply-footer-delivery'; +import { + getThreadReplyFooterRecord, + setThreadReplyFooterRecord, +} from '../thread-reply-footer-state'; +import { DiscordCommunicationProvider } from '../discord-provider'; + +const target = { + provider: 'discord' as const, + channelId: 'parent', + threadId: 'thread', +}; +const record = { + messageId: 'current', + textWithoutFooter: 'Body and quote', + images: [{ url: 'https://image', altText: 'proof' }], + refresh: { footerText: 'idle', channelId: 'thread' }, +}; +const read = () => + getThreadReplyFooterRecord( + target.provider, + target.channelId, + target.threadId, + ); +const write = (value = record) => + setThreadReplyFooterRecord( + target.provider, + target.channelId, + target.threadId, + value, + ); +const tick = (edit = vi.fn().mockResolvedValue(undefined)) => + refreshManagedThreadReplyFooter({ ...target, edit }); + +describe('current footer refresh serialization', () => { + it('rejects an initial write when the lease changes after the final check', async () => { + await write(); + const lockKey = 'discord:thread_reply_footer_lock:parent:thread'; + const competitor = { ...record, messageId: 'competitor' }; + const posted = { ...record, messageId: 'orphan' }; + const cleanup = vi.fn().mockResolvedValue(undefined); + const warning = vi.spyOn(console, 'error').mockImplementation(() => {}); + await expect( + deliverManagedThreadReplyFooter({ + provider: 'discord', + providerLabel: 'Discord', + channelId: 'parent', + footerStateThreadId: 'thread', + lockKey, + logRef: 'test', + logContext: 'test', + clearPreviousFooter: cleanup, + postReplyWithFooter: async () => { + get.mockImplementationOnce(async (key: string) => { + expect(key).toBe(lockKey); + const owner = store.get(key); + store.set(key, 'competitor-owner'); + await write(competitor); + return owner; + }); + return posted; + }, + }), + ).resolves.toEqual(posted); + expect(await read()).toEqual(competitor); + expect(cleanup).toHaveBeenCalledExactlyOnceWith(posted); + expect(store.get(lockKey)).toBe('competitor-owner'); + expect(schedule).toHaveBeenCalledTimes(2); + warning.mockRestore(); + }); + it.each(['competitor', 'current'])( + 'cleans a late refresh only when another carrier is current (%s)', + async (currentId) => { + await write(); + const edit = vi + .fn() + .mockImplementationOnce(async () => { + store.set( + 'discord:thread_reply_footer_lock:parent:thread', + 'new-owner', + ); + await write({ ...record, messageId: currentId }); + }) + .mockResolvedValue(undefined); + await tick(edit); + expect((await read())?.messageId).toBe(currentId); + expect((await read())?.refresh?.footerText).toBe('idle'); + expect(edit).toHaveBeenCalledTimes(currentId === 'current' ? 1 : 2); + if (currentId !== 'current') { + expect(edit).toHaveBeenLastCalledWith(record, record.textWithoutFooter); + } + expect(store.get('discord:thread_reply_footer_lock:parent:thread')).toBe( + 'new-owner', + ); + }, + ); + + it('a post finishing after lease loss cannot overwrite the competing carrier and strips only its own footer', async () => { + await write(); + const clearOwn = vi.fn().mockResolvedValue(undefined); + const warning = vi.spyOn(console, 'error').mockImplementation(() => {}); + const params = { + provider: 'discord' as const, + providerLabel: 'Discord', + channelId: 'parent', + footerStateThreadId: 'thread', + lockKey: 'discord:thread_reply_footer_lock:parent:thread', + logRef: 'test', + logContext: 'test', + }; + const result = await deliverManagedThreadReplyFooter({ + ...params, + clearPreviousFooter: clearOwn, + postReplyWithFooter: async () => { + store.delete(params.lockKey); // A's lease expires while its provider call is pending. + await deliverManagedThreadReplyFooter({ + ...params, + clearPreviousFooter: vi.fn().mockResolvedValue(undefined), + postReplyWithFooter: async () => ({ + ...record, + messageId: 'competitor', + textWithoutFooter: 'B', + }), + }); + return { ...record, messageId: 'orphan', textWithoutFooter: 'A' }; + }, + }); + expect(result.messageId).toBe('orphan'); + expect((await read())?.messageId).toBe('competitor'); + expect(clearOwn).toHaveBeenCalledTimes(1); + expect(clearOwn).toHaveBeenCalledWith( + expect.objectContaining({ messageId: 'orphan', textWithoutFooter: 'A' }), + ); + expect(schedule).toHaveBeenCalledTimes(2); // Original and competitor, never orphan. + warning.mockRestore(); + }); + + it('does not strip a deduplicated post adopted as the competitor current carrier', async () => { + await write(); + const clearOwn = vi.fn(); + const warning = vi.spyOn(console, 'error').mockImplementation(() => {}); + await deliverManagedThreadReplyFooter({ + provider: 'discord', + providerLabel: 'Discord', + channelId: 'parent', + footerStateThreadId: 'thread', + lockKey: 'discord:thread_reply_footer_lock:parent:thread', + logRef: 'test', + logContext: 'test', + clearPreviousFooter: clearOwn, + postReplyWithFooter: async () => { + store.set( + 'discord:thread_reply_footer_lock:parent:thread', + 'competitor', + ); + await write({ ...record, messageId: 'deduplicated' }); + return { ...record, messageId: 'deduplicated' }; + }, + }); + expect((await read())?.messageId).toBe('deduplicated'); + expect(clearOwn).not.toHaveBeenCalled(); + warning.mockRestore(); + }); + + it('an edit finishing after lease loss cannot repoint the carrier', async () => { + await write({ ...record, messageId: 'competitor' }); + const clearOwnFooter = vi.fn().mockResolvedValue(undefined); + await rememberThreadReplyFooterAfterEdit({ + ...target, + record, + assertLock: async () => { + throw new Error('lease lost'); + }, + lock: { + key: 'discord:thread_reply_footer_lock:parent:thread', + ownerId: 'stale', + }, + clearOwnFooter, + }); + expect((await read())?.messageId).toBe('competitor'); + expect(clearOwnFooter).toHaveBeenCalledTimes(1); + }); + + it.each([false, true])( + 'rejects a pointer write when ownership changes after a successful assertion (keepTtl=%s)', + async (keepTtl) => { + await write(); + const lockKey = 'discord:thread_reply_footer_lock:parent:thread'; + const competitor = { ...record, messageId: 'competitor' }; + const clearOwnFooter = vi.fn().mockResolvedValue(undefined); + const successfulAssertion = vi.fn(); + await withThreadReplyFooterLock({ + lockKey, + fn: async (assertLock, lock) => { + await rememberThreadReplyFooterAfterEdit({ + ...target, + record, + lock, + keepTtl, + assertLock: async () => { + await assertLock(); + successfulAssertion(); + // Expire A's lease after GET succeeds, before A's pointer mutation. + store.set(lockKey, 'competitor-owner'); + await write(competitor); + }, + clearOwnFooter, + }); + }, + }); + expect(successfulAssertion).toHaveBeenCalledTimes(1); + expect(await read()).toEqual(competitor); + expect(store.get(lockKey)).toBe('competitor-owner'); + expect(clearOwnFooter).toHaveBeenCalledTimes(1); + expect(schedule).toHaveBeenCalledTimes(2); + }, + ); + + it.each([404, 410])( + 'forgets a provider-deleted carrier (%s) while holding its lock', + async (status) => { + await write(); + await tick(vi.fn().mockRejectedValue({ status })); + expect(forget).toHaveBeenCalledWith(target); + expect((await read())?.refresh?.footerText).toBe('idle'); + }, + ); + + it('does not unsubscribe a competitor when a stale edit reports deletion after losing its lease', async () => { + await write(); + await expect( + tick( + vi.fn(async () => { + store.set( + 'discord:thread_reply_footer_lock:parent:thread', + 'competitor', + ); + await write({ ...record, messageId: 'competitor' }); + throw Object.assign(new Error('message deleted'), { status: 404 }); + }), + ), + ).rejects.toThrow('lease lost'); + expect(forget).not.toHaveBeenCalled(); + expect((await read())?.messageId).toBe('competitor'); + }); + it('a Discord footer-only PATCH leaves the carrier interactive components untouched', async () => { + const fetch = vi + .fn() + .mockResolvedValue( + new Response(JSON.stringify({ id: 'current' }), { status: 200 }), + ); + const provider = new DiscordCommunicationProvider({ + botToken: 'test-token', + fetch, + }); + await provider.editMessage({ + channelId: 'thread', + messageId: 'current', + text: 'Body\n\nNew footer', + preserveButtons: true, + }); + const payload = JSON.parse(fetch.mock.calls[0]![1].body as string); + expect(payload).toEqual({ + content: 'Body\n\nNew footer', + allowed_mentions: { parse: [] }, + }); + expect(payload).not.toHaveProperty('components'); + }); + beforeEach(() => { + get + .mockReset() + .mockImplementation(async (key: string) => store.get(key) ?? null); + store.clear(); + vi.clearAllMocks(); + failures.recordWrite = false; + activity.active = true; + activity.settled = false; + resolveFooter.mockResolvedValue('running'); + }); + + it('reports activity so the scheduler can slow down idle threads and drop settled ones', async () => { + await write(); + expect(await tick()).toBe('active'); + activity.active = false; + resolveFooter.mockResolvedValue('idle'); + expect(await tick()).toBe('idle'); + expect(forget).not.toHaveBeenCalled(); + activity.settled = true; + expect(await tick()).toBe('gone'); + expect(forget).toHaveBeenCalledWith(target); + expect(await read()).toEqual(record); // Settling never touches the carrier. + }); + + it('unregisters after editing a settled footer, but not while a reply owns the thread', async () => { + await write(); + activity.active = false; + activity.settled = true; + resolveFooter.mockResolvedValue('No running tasks'); + const edit = vi.fn().mockResolvedValue(undefined); + expect(await tick(edit)).toBe('gone'); + expect(edit).toHaveBeenCalledTimes(1); + expect(forget).toHaveBeenCalledWith(target); + forget.mockClear(); + store.set('discord:thread_reply_footer_lock:parent:thread', 'a-reply'); + expect(await tick(edit)).toBe('active'); + expect(forget).not.toHaveBeenCalled(); + expect(edit).toHaveBeenCalledTimes(1); + }); + + it('retires a footer whose navigation no longer resolves instead of polling it forever', async () => { + await write(); + resolveFooter.mockResolvedValue(null); + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect(await tick()).toBe('gone'); + expect(forget).toHaveBeenCalledWith(target); + expect(warning).toHaveBeenCalledWith( + expect.stringContaining('no longer resolves'), + expect.objectContaining(target), + ); + warning.mockRestore(); + }); + + it('strips a footer it applied after the record expired mid-edit, so a later reply cannot leave two footers', async () => { + await write(); + const edit = vi + .fn() + .mockImplementationOnce(async () => { + store.delete('discord:thread_reply_footer:parent:thread'); + }) + .mockResolvedValue(undefined); + await tick(edit); + expect(edit).toHaveBeenCalledTimes(2); + expect(edit).toHaveBeenLastCalledWith(record, record.textWithoutFooter); + expect(await read()).toBeNull(); + }); + + it('updates start/finish/fail/cancel/wait/resume from each newly resolved state, retaining body and images', async () => { + await write(); + const edit = vi.fn().mockResolvedValue(undefined); + for (const footer of [ + '1 running task', + 'No running tasks (finished)', + '1 running task (new run)', + 'No running tasks (failed)', + '1 running task (retry)', + 'No running tasks (cancelled)', + '1 running task (resumed)', + 'No running tasks (waiting)', + ]) { + resolveFooter.mockResolvedValueOnce(footer); + await tick(edit); + expect(edit).toHaveBeenLastCalledWith( + expect.objectContaining({ + messageId: 'current', + images: record.images, + }), + `${record.textWithoutFooter}\n\n${footer}`, + ); + expect((await read())?.refresh?.footerText).toBe(footer); + } + expect(schedule).toHaveBeenCalledTimes(1); // refresh does not extend the carrier TTL + }); + + it('does not edit unchanged, missing or unresolvable carriers or scan history', async () => { + const edit = vi.fn(); + await tick(edit); + expect(forget).toHaveBeenCalledWith(target); + await write(); + resolveFooter.mockResolvedValueOnce('idle').mockResolvedValueOnce(null); + await tick(edit); + await tick(edit); + expect(edit).not.toHaveBeenCalled(); + }); + + it('does not acknowledge a failed edit, so the next tick retries it', async () => { + await write(); + await expect( + tick(vi.fn().mockRejectedValue(new Error('provider unavailable'))), + ).rejects.toThrow('provider unavailable'); + expect((await read())?.refresh?.footerText).toBe('idle'); + const edit = vi.fn().mockResolvedValue(undefined); + await tick(edit); + expect(edit).toHaveBeenCalledTimes(1); + }); + + it('holds the delivery lock across the edit only; later ticks target only the relocated carrier', async () => { + await write(); + let release!: () => void; + let started!: () => void; + const editing = new Promise((resolve) => { + started = resolve; + }); + const gate = new Promise((resolve) => { + release = resolve; + }); + const refresh = tick( + vi.fn(async () => { + started(); + await gate; + }), + ); + await editing; + const clear = vi.fn().mockResolvedValue(undefined); + const post = vi.fn(async () => ({ + ...record, + messageId: 'new', + textWithoutFooter: 'New body', + })); + const delivery = deliverManagedThreadReplyFooter({ + provider: 'discord', + providerLabel: 'Discord', + channelId: 'parent', + footerStateThreadId: 'thread', + lockKey: 'discord:thread_reply_footer_lock:parent:thread', + logRef: 'test', + logContext: 'test', + postReplyWithFooter: post, + clearPreviousFooter: clear, + }); + expect(post).not.toHaveBeenCalled(); + release(); + await refresh; + await delivery; + expect(clear).toHaveBeenCalledWith( + expect.objectContaining({ + messageId: 'current', + textWithoutFooter: 'Body and quote', + }), + ); + const edit = vi.fn().mockResolvedValue(undefined); + await tick(edit); + expect(edit).toHaveBeenCalledWith( + expect.objectContaining({ messageId: 'new' }), + 'New body\n\nrunning', + ); + }); + + it('yields to a reply that took the thread while it was resolving, without waiting or editing', async () => { + await write(); + resolveFooter.mockImplementationOnce(async () => { + store.set('discord:thread_reply_footer_lock:parent:thread', 'new-owner'); + return 'running'; + }); + const edit = vi.fn(); + const startedAt = Date.now(); + expect(await tick(edit)).toBe('active'); + expect(Date.now() - startedAt).toBeLessThan(500); + expect(edit).not.toHaveBeenCalled(); + expect(store.get('discord:thread_reply_footer_lock:parent:thread')).toBe( + 'new-owner', + ); + }); + + it('skips a carrier that a reply relocated between resolution and the edit', async () => { + await write(); + resolveFooter.mockImplementationOnce(async () => { + await write({ ...record, messageId: 'relocated' }); + return 'running'; + }); + const edit = vi.fn(); + expect(await tick(edit)).toBe('active'); + expect(edit).not.toHaveBeenCalled(); + expect((await read())?.messageId).toBe('relocated'); + }); + + it('renews its lease across slow provider work and releases it on completion', async () => { + vi.useFakeTimers(); + try { + await write(); + let release!: () => void; + let started!: () => void; + const ready = new Promise((resolve) => { + started = resolve; + }); + const gate = new Promise((resolve) => { + release = resolve; + }); + const refresh = tick( + vi.fn(async () => { + started(); + await gate; + }), + ); + await ready; + await vi.advanceTimersByTimeAsync(35_000); + expect(renew).toHaveBeenCalledTimes(3); + expect(store.has('discord:thread_reply_footer_lock:parent:thread')).toBe( + true, + ); + release(); + await refresh; + expect(store.has('discord:thread_reply_footer_lock:parent:thread')).toBe( + false, + ); + } finally { + vi.useRealTimers(); + } + }); + + it('does not clear the prior carrier if persisting its replacement fails', async () => { + await write(); + failures.recordWrite = true; + const clear = vi.fn().mockResolvedValue(undefined); + const warning = vi.spyOn(console, 'error').mockImplementation(() => {}); + await deliverManagedThreadReplyFooter({ + provider: 'discord', + providerLabel: 'Discord', + channelId: 'parent', + footerStateThreadId: 'thread', + lockKey: 'discord:thread_reply_footer_lock:parent:thread', + logRef: 'test', + logContext: 'test', + postReplyWithFooter: async () => ({ ...record, messageId: 'new' }), + clearPreviousFooter: clear, + }); + expect(clear).toHaveBeenCalledWith( + expect.objectContaining({ messageId: 'new' }), + ); + expect(clear).not.toHaveBeenCalledWith( + expect.objectContaining({ messageId: 'current' }), + ); + expect((await read())?.messageId).toBe('current'); + warning.mockRestore(); + }); +}); diff --git a/packages/communication/src/__tests__/thread-footer-refresh-lifecycle.test.ts b/packages/communication/src/__tests__/thread-footer-refresh-lifecycle.test.ts new file mode 100644 index 000000000..1a516be60 --- /dev/null +++ b/packages/communication/src/__tests__/thread-footer-refresh-lifecycle.test.ts @@ -0,0 +1,157 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { + db, + eq, + inArray, + runFactory, + sessionFactory, + sessionTasks, + sessions, + taskFactory, + taskRuns, + tasks, +} from '@roomote/db/server'; +import { Env } from '@roomote/env'; +import { RunStatus } from '@roomote/types'; + +const store = vi.hoisted(() => new Map()); +vi.mock('@roomote/redis', () => ({ + getRedis: () => ({ + get: async (key: string) => store.get(key) ?? null, + set: async (key: string, value: string, ...args: unknown[]) => { + if (args.includes('NX') && store.has(key)) return null; + store.set(key, value); + return 'OK'; + }, + zadd: async () => 1, + zrem: async () => 1, + eval: async ( + script: string, + count: number, + key: string, + ...args: string[] + ) => { + const owner = args[count - 1]; + if (store.get(key) !== owner) return 0; + if (count === 2) { + const [pointerKey, , value, ttl] = args; + if (ttl !== 'keepTtl' || store.has(pointerKey!)) + store.set(pointerKey!, value!); + return 1; + } + if (script.includes("'del'")) store.delete(key); + return 1; + }, + }), +})); +import { buildThreadReplyFooterText } from '../chat-messages'; +import { setThreadReplyFooterRecord } from '../thread-reply-footer-state'; +import { refreshManagedThreadReplyFooter } from '../thread-reply-footer-delivery'; + +describe('persisted latest-run lifecycle reaches the current carrier without another reply', () => { + const taskIds: string[] = []; + const sessionIds: string[] = []; + afterEach(async () => { + if (taskIds.length) { + await db.delete(taskRuns).where(inArray(taskRuns.taskId, taskIds)); + await db + .delete(sessionTasks) + .where(inArray(sessionTasks.taskId, taskIds)); + await db.delete(tasks).where(inArray(tasks.id, taskIds)); + taskIds.length = 0; + } + if (sessionIds.length) { + await db.delete(sessions).where(inArray(sessions.id, sessionIds)); + sessionIds.length = 0; + } + store.clear(); + }); + + it.each(['task', 'sessions'])( + 'refreshes %s navigation through start, finish, fail, cancel, wait and a superseding run', + async (navigation) => { + const session = await sessionFactory.create(); + sessionIds.push(session.id); + const task = await taskFactory.create({ state: 'active' }); + taskIds.push(task.id); + await db.insert(sessionTasks).values({ + sessionId: session.id, + taskId: task.id, + origin: 'direct_launch', + }); + await runFactory.create({ taskId: task.id, status: RunStatus.Running }); + const latest = await runFactory.create({ + taskId: task.id, + status: RunStatus.Idle, + taskPhase: 'waiting_for_prompt', + }); + await setThreadReplyFooterRecord('discord', 'C', 'T', { + messageId: 'current', + textWithoutFooter: '> Quote\n\nOriginal body', + refresh: { + channelId: 'T', + footerText: buildThreadReplyFooterText({ + taskUrl: new URL( + `/${navigation}/${navigation === 'task' ? task.id : session.id}`, + Env.R_APP_URL, + ).toString(), + }), + }, + }); + const edit = vi.fn().mockResolvedValue(undefined); + const tick = () => + refreshManagedThreadReplyFooter({ + provider: 'discord', + channelId: 'C', + threadId: 'T', + edit, + }); + await tick(); + expect(edit.mock.calls.at(-1)?.[1]).toContain('[No running tasks]'); + for (const status of [ + RunStatus.Running, + RunStatus.Completed, + RunStatus.Running, + RunStatus.Failed, + RunStatus.Running, + RunStatus.Canceled, + RunStatus.Running, + RunStatus.Idle, + ]) { + await db + .update(taskRuns) + .set({ + status, + taskPhase: + status === RunStatus.Idle ? 'waiting_for_prompt' : 'running', + }) + .where(eq(taskRuns.id, latest.id)); + await tick(); + expect(edit.mock.calls.at(-1)?.[1]).toContain( + status === RunStatus.Running + ? '[1 running task]' + : '[No running tasks]', + ); + expect(edit.mock.calls.at(-1)?.[0].messageId).toBe('current'); + expect(edit.mock.calls.at(-1)?.[1]).toContain( + '> Quote\n\nOriginal body', + ); + } + await db + .update(taskRuns) + .set({ status: RunStatus.Running, taskPhase: 'running' }) + .where(eq(taskRuns.id, latest.id)); + await tick(); + await runFactory.create({ + taskId: task.id, + status: RunStatus.Idle, + taskPhase: 'waiting_for_prompt', + }); + await tick(); + expect(edit.mock.calls.at(-1)?.[1]).toContain('[No running tasks]'); + const count = edit.mock.calls.length; + await tick(); + expect(edit).toHaveBeenCalledTimes(count); + }, + ); +}); diff --git a/packages/communication/src/__tests__/thread-footer-refresh-registry.test.ts b/packages/communication/src/__tests__/thread-footer-refresh-registry.test.ts new file mode 100644 index 000000000..a41e87f73 --- /dev/null +++ b/packages/communication/src/__tests__/thread-footer-refresh-registry.test.ts @@ -0,0 +1,141 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +vi.mock('@roomote/env', () => ({ + Env: { R_APP_URL: 'https://app.example.com' }, +})); +const mocks = vi.hoisted(() => ({ + zadd: vi.fn(), + zrem: vi.fn(), + eval: vi.fn(), +})); +vi.mock('@roomote/redis', () => ({ getRedis: () => mocks })); +import { + claimThreadFooterRefreshTargets, + classifyThreadFooterActivity, + scheduleThreadFooterRefresh, + forgetThreadFooterRefresh, + getThreadFooterNavigationUrl, + getThreadFooterPullRequestLinks, + rescheduleThreadFooterRefresh, + THREAD_FOOTER_SETTLED_AFTER_MS, +} from '../thread-footer-refresh'; + +describe('bounded footer refresh registry', () => { + it('parses supported navigation links without consuming nested open parentheses', () => { + const url = 'https://app.example.com/sessions/session'; + expect( + getThreadFooterNavigationUrl(`_[Open in Roomote](${url})_`)?.href, + ).toBe(url); + expect( + getThreadFooterNavigationUrl(`_<${url}|Open in Roomote>_`)?.href, + ).toBe(url); + expect( + getThreadFooterNavigationUrl('[Open in Roomote](('.repeat(20_000)), + ).toBeNull(); + expect( + getThreadFooterNavigationUrl( + '_[Open in Roomote](https://other.example/sessions/session)_', + ), + ).toBeNull(); + }); + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(Date, 'now').mockReturnValue(1000); + }); + const target = { + provider: 'discord' as const, + channelId: 'C', + threadId: 'T', + }; + it('indexes only current destinations, with a 30-second due time', async () => { + await scheduleThreadFooterRefresh(target); + expect(mocks.zadd).toHaveBeenCalledWith( + 'thread_footer_refresh:due', + 31_000, + JSON.stringify(target), + ); + await forgetThreadFooterRefresh(target); + expect(mocks.zrem).toHaveBeenCalledWith( + 'thread_footer_refresh:due', + JSON.stringify(target), + ); + }); + it('caps claims at 100 and leases claimed targets past the scheduler cadence', async () => { + mocks.eval.mockResolvedValue([JSON.stringify(target)]); + expect(await claimThreadFooterRefreshTargets(1000)).toEqual([target]); + expect(mocks.eval).toHaveBeenCalledWith( + expect.stringContaining("'zrangebyscore'"), + 1, + 'thread_footer_refresh:due', + 1000, + 301_000, + 100, + ); + expect(mocks.eval.mock.calls[0]![0]).toContain("'zadd'"); + }); + it('checks active destinations every 30 seconds and idle ones every 5 minutes', async () => { + await rescheduleThreadFooterRefresh(target, 'active'); + expect(mocks.zadd).toHaveBeenLastCalledWith( + 'thread_footer_refresh:due', + 31_000, + JSON.stringify(target), + ); + await rescheduleThreadFooterRefresh(target, 'idle'); + expect(mocks.zadd).toHaveBeenLastCalledWith( + 'thread_footer_refresh:due', + 301_000, + JSON.stringify(target), + ); + }); + it('settles only quiet Sessions with nothing running or previewing', () => { + const now = THREAD_FOOTER_SETTLED_AFTER_MS * 2; + vi.spyOn(Date, 'now').mockReturnValue(now); + const recent = { sessionActivityAt: now - 60_000 }; + const quiet = { + sessionActivityAt: now - THREAD_FOOTER_SETTLED_AFTER_MS - 1, + }; + expect( + classifyThreadFooterActivity({ + ...quiet, + runningTasks: { count: 1, url: 'u' }, + }), + ).toEqual({ active: true, settled: false }); + expect( + classifyThreadFooterActivity({ ...quiet, livePreviewUrl: 'https://p' }), + ).toEqual({ active: true, settled: false }); + expect( + classifyThreadFooterActivity({ + ...recent, + runningTasks: { count: 0, url: 'u' }, + }), + ).toEqual({ active: false, settled: false }); + expect( + classifyThreadFooterActivity({ + ...quiet, + runningTasks: { count: 0, url: 'u' }, + }), + ).toEqual({ active: false, settled: true }); + // No Session at all: nothing can start, so an idle footer settles at once. + expect(classifyThreadFooterActivity({})).toEqual({ + active: false, + settled: true, + }); + }); + it('reads the pull request links a footer already shows, in order', () => { + expect( + getThreadFooterPullRequestLinks( + '_ · · · _', + ), + ).toEqual([ + { prNumber: 7, prUrl: 'https://github.com/o/r/pull/7?a=1&b=2' }, + { prNumber: 9, prUrl: 'https://github.com/o/r/pull/9' }, + ]); + expect( + getThreadFooterPullRequestLinks( + '-# _[PR #12](https://github.com/o/r/pull/12) · [Open in Roomote](https://app/sessions/s)_', + ), + ).toEqual([{ prNumber: 12, prUrl: 'https://github.com/o/r/pull/12' }]); + expect( + getThreadFooterPullRequestLinks('_[Open in Roomote](https://app)_'), + ).toEqual([]); + }); +}); diff --git a/packages/communication/src/__tests__/thread-reply-footer-context.test.ts b/packages/communication/src/__tests__/thread-reply-footer-context.test.ts index b69f18e9c..206a41a4a 100644 --- a/packages/communication/src/__tests__/thread-reply-footer-context.test.ts +++ b/packages/communication/src/__tests__/thread-reply-footer-context.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, beforeEach, vi } from 'vitest'; +import { CODE_SERVER_NAMED_PORT, RunStatus } from '@roomote/types'; const { findFirstMock, @@ -6,12 +7,14 @@ const { taskRunFindFirstMock, environmentFindFirstMock, resolveEffectivePreviewRuntimeConfigMock, + getSessionForTaskMock, } = vi.hoisted(() => ({ findFirstMock: vi.fn(), findManyMock: vi.fn(), taskRunFindFirstMock: vi.fn(), environmentFindFirstMock: vi.fn(), resolveEffectivePreviewRuntimeConfigMock: vi.fn(), + getSessionForTaskMock: vi.fn(), })); vi.mock('@roomote/db/server', () => ({ @@ -30,6 +33,7 @@ vi.mock('@roomote/db/server', () => ({ }, }, eq: vi.fn((...args: unknown[]) => ({ eq: args })), + getSessionForTask: getSessionForTaskMock, taskPullRequests: { taskId: 'taskId', }, @@ -54,6 +58,7 @@ import { buildThreadReplyPrUrl, resolveThreadReplyFooterContext, resolveThreadReplyLinkedPrs, + resolveThreadReplyLivePreviewUrl, } from '../thread-reply-footer-context'; function mockEnvironmentBackedTaskRun(params?: { @@ -62,12 +67,14 @@ function mockEnvironmentBackedTaskRun(params?: { taskRunFindFirstMock.mockResolvedValue({ payload: { environmentId: 'env-1' }, primaryPortName: params?.primaryPortName ?? null, + status: RunStatus.Idle, }); } describe('thread reply footer context', () => { beforeEach(() => { vi.clearAllMocks(); + getSessionForTaskMock.mockResolvedValue(null); findFirstMock.mockResolvedValue(null); findManyMock.mockResolvedValue([]); taskRunFindFirstMock.mockResolvedValue(null); @@ -171,4 +178,44 @@ describe('thread reply footer context', () => { livePreviewUrl: null, }); }); + + it.each([ + { status: RunStatus.Completed }, + { snapshotId: 'snapshot' }, + { sleepRequestedAt: new Date() }, + { snapshotRequestedAt: new Date() }, + ])('omits unavailable previews (%j)', async (unavailable) => { + taskRunFindFirstMock.mockResolvedValue({ + status: RunStatus.Idle, + payload: { environmentId: 'env-1' }, + ...unavailable, + }); + expect(await resolveThreadReplyLivePreviewUrl('task-1')).toBeNull(); + expect(environmentFindFirstMock).not.toHaveBeenCalled(); + }); + + it('keeps an awake idle preview after a failed sleep attempt and skips system ports', async () => { + taskRunFindFirstMock.mockResolvedValue({ + status: RunStatus.Idle, + payload: { environmentId: 'env-1' }, + primaryPortName: CODE_SERVER_NAMED_PORT.name, + sleepRequestedAt: new Date(), + snapshotFailedAt: new Date(), + }); + environmentFindFirstMock.mockResolvedValue({ + config: { + ports: [ + { name: CODE_SERVER_NAMED_PORT.name, port: 8080, primary: true }, + { name: 'WEB', port: 3000 }, + ], + }, + }); + expect(await resolveThreadReplyLivePreviewUrl('task-1')).toBe( + 'https://task-1-web.preview.example.com', + ); + environmentFindFirstMock.mockResolvedValue({ + config: { ports: [{ name: CODE_SERVER_NAMED_PORT.name, port: 8080 }] }, + }); + expect(await resolveThreadReplyLivePreviewUrl('task-1')).toBeNull(); + }); }); diff --git a/packages/communication/src/__tests__/thread-reply-footer-state.test.ts b/packages/communication/src/__tests__/thread-reply-footer-state.test.ts index bca7e39bf..0c3a232fd 100644 --- a/packages/communication/src/__tests__/thread-reply-footer-state.test.ts +++ b/packages/communication/src/__tests__/thread-reply-footer-state.test.ts @@ -1,14 +1,21 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -const { getMock, setMock } = vi.hoisted(() => ({ +const { getMock, setMock, evalMock, scheduleMock } = vi.hoisted(() => ({ getMock: vi.fn(), setMock: vi.fn(), + evalMock: vi.fn(), + scheduleMock: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock('../thread-footer-refresh', () => ({ + scheduleThreadFooterRefresh: scheduleMock, })); vi.mock('@roomote/redis', () => ({ getRedis: vi.fn(() => ({ get: getMock, set: setMock, + eval: evalMock, })), })); @@ -22,6 +29,71 @@ describe('thread reply footer state', () => { vi.clearAllMocks(); getMock.mockResolvedValue(null); setMock.mockResolvedValue('OK'); + evalMock.mockResolvedValue(1); + }); + + it.each([false, true])( + 'atomically checks ownership with keepTtl=%s', + async (keepTtl) => { + const record = { + messageId: 'activity-2', + textWithoutFooter: 'reply', + refresh: { footerText: 'idle', channelId: 'channel' }, + }; + const lock = { key: 'lock', ownerId: 'owner' }; + await expect( + setThreadReplyFooterRecord('teams', 'channel', 'thread', record, { + keepTtl, + lock, + }), + ).resolves.toBe(true); + expect(evalMock).toHaveBeenCalledWith( + expect.stringContaining("redis.call('get', KEYS[1]) ~= ARGV[1]"), + 2, + lock.key, + 'teams:thread_reply_footer:channel:thread', + lock.ownerId, + JSON.stringify(record), + keepTtl ? 'keepTtl' : 30 * 24 * 60 * 60, + ); + expect(setMock).not.toHaveBeenCalled(); + expect(scheduleMock).toHaveBeenCalledTimes(keepTtl ? 0 : 1); + scheduleMock.mockClear(); + evalMock.mockResolvedValueOnce(0); + await expect( + setThreadReplyFooterRecord('teams', 'channel', 'thread', record, { + keepTtl, + lock, + }), + ).resolves.toBe(false); + expect(scheduleMock).not.toHaveBeenCalled(); + }, + ); + + it('reports a keepTtl write that found no record to update', async () => { + const record = { messageId: 'activity-2', textWithoutFooter: 'reply' }; + expect( + await setThreadReplyFooterRecord('teams', 'channel', 'thread', record, { + keepTtl: true, + lock: { key: 'lock', ownerId: 'owner' }, + }), + ).toBe(true); + expect(evalMock.mock.calls[0]![0]).toContain( + "if not redis.call('set', KEYS[2], ARGV[2], 'KEEPTTL', 'XX') then return 0 end", + ); + setMock.mockResolvedValueOnce(null); + expect( + await setThreadReplyFooterRecord('teams', 'channel', 'thread', record, { + keepTtl: true, + }), + ).toBe(false); + expect(setMock).toHaveBeenCalledWith( + 'teams:thread_reply_footer:channel:thread', + JSON.stringify(record), + 'KEEPTTL', + 'XX', + ); + expect(scheduleMock).not.toHaveBeenCalled(); }); it('stores footer records under a provider-scoped key with a TTL', async () => { diff --git a/packages/communication/src/chat-messages.ts b/packages/communication/src/chat-messages.ts index fc2ae90f2..d1ee66703 100644 --- a/packages/communication/src/chat-messages.ts +++ b/packages/communication/src/chat-messages.ts @@ -289,59 +289,59 @@ export type ThreadReplyLinkedPr = { prUrl: string; }; +export type ThreadReplyRunningTasks = { + count: number; + url: string; +}; + export function buildThreadReplyFooterText({ taskUrl, linkedPrs, livePreviewUrl, - explicitMentionRequired = false, + runningTasks, + webAppUrl, formatLink = formatMarkdownLink, - formatFooterText = (text) => `_${text}_`, + formatFooterText = (text) => text, }: { taskUrl: string; linkedPrs?: ThreadReplyLinkedPr[]; livePreviewUrl?: string | null; - explicitMentionRequired?: boolean; + runningTasks?: ThreadReplyRunningTasks | null; + webAppUrl?: string | null; formatLink?: LinkFormatter; formatFooterText?: (text: string) => string; }): string { - const replyInstruction = explicitMentionRequired - ? 'reply with @-mention or use' - : 'reply or use'; - const livePreviewLink = livePreviewUrl - ? formatLink('live preview', livePreviewUrl) - : null; - const webAppLink = formatLink('web app', taskUrl); - - const activePullRequests = linkedPrs ?? []; - - if (activePullRequests.length > 0) { - const prLinks = activePullRequests.map((pr) => - formatLink(`PR #${pr.prNumber}`, pr.prUrl), - ); - const prLink = - prLinks.length === 1 - ? prLinks[0] - : `${prLinks.slice(0, -1).join(', ')} and ${prLinks.at(-1)}`; - const workingOn = livePreviewLink - ? `${prLink}, ${livePreviewLink}` - : prLink; - - return formatFooterText( - `Working on ${workingOn}, ${replyInstruction} the ${webAppLink}.`, - ); - } - - if (livePreviewLink) { - return formatFooterText( - `Working on a ${livePreviewLink}, ${replyInstruction} the ${webAppLink}.`, + const items: string[] = []; + if (runningTasks) { + items.push( + formatLink( + runningTasks.count === 0 + ? 'No running tasks' + : `${runningTasks.count} running task${runningTasks.count === 1 ? '' : 's'}`, + runningTasks.url, + ), ); } - - return formatFooterText( - explicitMentionRequired - ? `Reply with @-mention or use the ${webAppLink}.` - : `Reply or use the ${webAppLink}.`, + if (livePreviewUrl) items.push(formatLink('Live preview', livePreviewUrl)); + items.push( + ...(linkedPrs ?? []).map((pr) => + formatLink(`PR #${pr.prNumber}`, pr.prUrl), + ), ); + // Default task navigation opens the owning Session with that task selected; + // any other caller-owned destination (setup, a specific artifact) is kept. + let webUrl = new URL(taskUrl); + const ownTaskId = /^\/task\/([^/]+)$/.exec(webUrl.pathname)?.[1]; + if (webAppUrl && ownTaskId) { + const sessionUrl = new URL(webAppUrl); + for (const [key, value] of webUrl.searchParams) { + if (key.startsWith('utm_')) sessionUrl.searchParams.set(key, value); + } + sessionUrl.searchParams.set('task', ownTaskId); + webUrl = sessionUrl; + } + items.push(formatLink('Open in Roomote', webUrl.toString())); + return formatFooterText(items.join(' · ')); } /** diff --git a/packages/communication/src/discord-provider.ts b/packages/communication/src/discord-provider.ts index d767cc521..fd381a618 100644 --- a/packages/communication/src/discord-provider.ts +++ b/packages/communication/src/discord-provider.ts @@ -616,6 +616,8 @@ export class DiscordCommunicationProvider implements CommunicationProviderAdapte messageId: string; text: string; buttons?: CommunicationMessageButton[][]; + /** Footer-only edits must not clear interactive controls on the carrier. */ + preserveButtons?: boolean; }): Promise { if (input.text.length > DISCORD_MAX_MESSAGE_LENGTH) { throw new Error( @@ -628,7 +630,9 @@ export class DiscordCommunicationProvider implements CommunicationProviderAdapte { content: input.text, allowed_mentions: { parse: [] }, - components: buildDiscordComponents(input.buttons) ?? [], + ...(input.preserveButtons + ? {} + : { components: buildDiscordComponents(input.buttons) ?? [] }), }, { retryNetworkErrors: true, retryServerErrors: true }, ); diff --git a/packages/communication/src/fast-session-footer.ts b/packages/communication/src/fast-session-footer.ts index 7efb273d6..39375602b 100644 --- a/packages/communication/src/fast-session-footer.ts +++ b/packages/communication/src/fast-session-footer.ts @@ -14,9 +14,13 @@ import { buildThreadReplyFooterText, formatMarkdownLink, type ThreadReplyLinkedPr, + type ThreadReplyRunningTasks, } from './chat-messages'; import { chunkDiscordMessage } from './discord-provider'; -import { resolveThreadReplyFooterContext } from './thread-reply-footer-context'; +import { + resolveSessionRunningTasks, + resolveThreadReplyFooterContext, +} from './thread-reply-footer-context'; export type FastSessionFooterProvider = | 'slack' @@ -38,6 +42,9 @@ export type FastSessionPullRequestReference = { export type FastSessionReplyFooterContext = { linkedPrs: ThreadReplyLinkedPr[]; livePreviewUrl: string | null; + runningTasks?: ThreadReplyRunningTasks | null; + /** Session `activityAt` (epoch ms), so a refresh can tell when it has settled. */ + sessionActivityAt?: number | null; }; const TERMINAL_PULL_REQUEST_STATUSES = new Set(['closed', 'merged']); @@ -65,9 +72,7 @@ function collectFastSessionLinkedPrs(params: { return [...uniquePrs.values()]; } -async function getFastSessionLinkedTaskIds( - sessionId: string, -): Promise { +async function getFastSessionLinkedTasks(sessionId: string) { const session = await getSessionForFastConversation(db, sessionId); const linkedTasks = session ? await db @@ -81,7 +86,7 @@ async function getFastSessionLinkedTaskIds( // across footer rebuilds. .orderBy(asc(sessionTasks.attachedAt), asc(sessionTasks.taskId)) : []; - return linkedTasks.map(({ taskId }) => taskId); + return { session, linkedTaskIds: linkedTasks.map(({ taskId }) => taskId) }; } export async function resolveFastSessionReplyFooterContext(params: { @@ -89,18 +94,26 @@ export async function resolveFastSessionReplyFooterContext(params: { pullRequest?: FastSessionPullRequestReference | null; pullRequests?: readonly FastSessionPullRequestReference[]; }): Promise { - const linkedTaskIds = await getFastSessionLinkedTaskIds(params.sessionId); - const contexts = await Promise.all( - linkedTaskIds.map((taskId) => - resolveThreadReplyFooterContext({ - taskId, - prRepo: null, - prNumber: null, - }), - ), + const { session, linkedTaskIds } = await getFastSessionLinkedTasks( + params.sessionId, ); + const [runningTasks, contexts] = await Promise.all([ + session ? resolveSessionRunningTasks(session.id, linkedTaskIds) : null, + Promise.all( + linkedTaskIds.map((taskId) => + resolveThreadReplyFooterContext({ + taskId, + prRepo: null, + prNumber: null, + includeRunningTasks: false, + }), + ), + ), + ]); return { + ...(runningTasks ? { runningTasks } : {}), + sessionActivityAt: session?.activityAt ?? null, linkedPrs: collectFastSessionLinkedPrs({ pullRequest: params.pullRequest, pullRequests: params.pullRequests, @@ -135,8 +148,7 @@ export function buildSelectedTaskSessionUrl(params: { } /** - * The Fast-session variant of the task thread-reply footer: always the plain - * "Reply or use the web app." shape, linking to the session view. + * Compact Session links, with task navigation separate from the transcript. */ export function buildFastSessionReplyFooterText(params: { provider: FastSessionFooterProvider; @@ -145,23 +157,15 @@ export function buildFastSessionReplyFooterText(params: { pullRequests?: readonly FastSessionPullRequestReference[]; linkedPrs?: readonly ThreadReplyLinkedPr[]; livePreviewUrl?: string | null; + runningTasks?: ThreadReplyRunningTasks | null; }): string { const sessionUrl = buildFastSessionUrl(params.provider, params.sessionId); - // Chat surfaces route any thread reply to the Session; source-control - // discussions only hear @-mentions, so the footer must say so. - const explicitMentionRequired = - params.provider === 'github' || - params.provider === 'gitlab' || - params.provider === 'bitbucket' || - params.provider === 'ado' || - params.provider === 'gitea'; - return buildThreadReplyFooterText({ taskUrl: sessionUrl, linkedPrs: collectFastSessionLinkedPrs(params), livePreviewUrl: params.livePreviewUrl, - explicitMentionRequired, + runningTasks: params.runningTasks, ...(params.provider === 'slack' ? { formatLink: (label: string, url: string) => `<${url}|${label}>` } : params.provider === 'discord' diff --git a/packages/communication/src/index.ts b/packages/communication/src/index.ts index 0b0748ea9..a2bd05cb1 100644 --- a/packages/communication/src/index.ts +++ b/packages/communication/src/index.ts @@ -18,3 +18,5 @@ export * from './fast-session-footer'; export * from './thread-reply-footer-context'; export * from './thread-reply-footer-delivery'; export * from './thread-reply-footer-state'; +export * from './thread-footer-refresh'; +export * from './text-thread-reply-footer'; diff --git a/packages/communication/src/text-thread-reply-footer.ts b/packages/communication/src/text-thread-reply-footer.ts new file mode 100644 index 000000000..db3c18611 --- /dev/null +++ b/packages/communication/src/text-thread-reply-footer.ts @@ -0,0 +1,192 @@ +import type { + CommunicationPostMessageInput, + CommunicationPostMessageResult, +} from './provider'; +import type { TeamsCommunicationProvider } from './teams-provider'; +import type { TelegramCommunicationProvider } from './telegram-provider'; +import { chunkTelegramMarkdownAsHtml } from './telegram-format'; +import { + deliverManagedThreadReplyFooter, + withThreadReplyFooterLock, + rememberThreadReplyFooterAfterEdit, +} from './thread-reply-footer-delivery'; +import { + getThreadReplyFooterRecord, + type ThreadReplyFooterRecord, +} from './thread-reply-footer-state'; +import { resolveCurrentThreadFooterText } from './thread-footer-refresh'; + +type TextProvider = TeamsCommunicationProvider | TelegramCommunicationProvider; + +export async function editTextThreadFooterMessage( + provider: TextProvider, + record: ThreadReplyFooterRecord, + text: string, +): Promise { + if (!record.refresh) return; + const input = { + channelId: record.refresh.channelId, + messageId: record.messageId, + text: text || '\u200b', + textFormat: 'markdown' as const, + }; + if (provider.provider === 'teams') { + await provider.updateMessage({ + ...input, + serviceUrl: record.refresh.serviceUrl, + images: record.images, + }); + } else { + await provider.editMessageText({ + ...input, + ...(record.buttons ? { buttons: record.buttons } : {}), + }); + } +} + +/** Managed text-provider delivery, including replacing a known automation root. */ +export async function postTextThreadReplyWithFooter(params: { + provider: TextProvider; + input: CommunicationPostMessageInput; + footerText: string; + messageId?: string; +}): Promise { + const { provider, input } = params; + const threadId = input.threadId ?? 'root'; + return deliverManagedThreadReplyFooter({ + provider: provider.provider, + providerLabel: provider.provider, + channelId: input.channelId, + footerStateThreadId: threadId, + lockKey: `${provider.provider}:thread_reply_footer_lock:${input.channelId}:${threadId}`, + logRef: 'text reply', + logContext: 'threadFooter', + postReplyWithFooter: async () => { + // The caller's footer already reflects the event that produced this + // reply; the scheduled refresh keeps it current from here. + const footerText = params.footerText; + const text = [input.text, footerText].filter(Boolean).join('\n\n'); + const refresh = { + footerText, + channelId: input.channelId, + ...(input.serviceUrl ? { serviceUrl: input.serviceUrl } : {}), + }; + let posted: CommunicationPostMessageResult; + if (params.messageId) { + await editTextThreadFooterMessage( + provider, + { + messageId: params.messageId, + textWithoutFooter: input.text ?? '', + images: input.images, + refresh, + }, + text, + ); + posted = { + provider: provider.provider, + channelId: input.channelId, + messageId: params.messageId, + }; + } else { + posted = await provider.postMessage({ + ...input, + text, + textFormat: 'markdown', + }); + } + const finalChunk = + provider.provider === 'telegram' + ? (chunkTelegramMarkdownAsHtml(text).at(-1)?.markdown ?? '') + : text; + const textWithoutFooter = + finalChunk === footerText + ? '' + : finalChunk.endsWith(`\n\n${footerText}`) + ? finalChunk.slice(0, -footerText.length - 2) + : finalChunk; + return { + ...posted, + messageId: posted.lastTextMessageId ?? posted.messageId, + textWithoutFooter, + ...(provider.provider === 'telegram' && + !input.images?.length && + input.buttons + ? { buttons: input.buttons } + : {}), + ...(provider.provider === 'teams' && input.images?.length + ? { images: input.images } + : {}), + ...(finalChunk.endsWith(footerText) ? { refresh } : {}), + }; + }, + clearPreviousFooter: (record) => + editTextThreadFooterMessage( + provider, + { + ...record, + refresh: record.refresh ?? { + footerText: '', + channelId: input.channelId, + serviceUrl: input.serviceUrl, + }, + }, + record.textWithoutFooter, + ), + }); +} + +export async function replaceTextThreadReplyWithFooter(params: { + provider: TextProvider; + channelId: string; + threadId?: string; + serviceUrl?: string; + messageId: string; + text: string; +}): Promise { + const threadId = params.threadId ?? 'root'; + await withThreadReplyFooterLock({ + lockKey: `${params.provider.provider}:thread_reply_footer_lock:${params.channelId}:${threadId}`, + fn: async (assertLock, lock) => { + const record = await getThreadReplyFooterRecord( + params.provider.provider, + params.channelId, + threadId, + ); + const current = record?.messageId === params.messageId ? record : null; + const footerText = current?.refresh + ? ((await resolveCurrentThreadFooterText( + params.provider.provider, + current.refresh.footerText, + )) ?? current.refresh.footerText) + : ''; + const next = { + ...(current ?? {}), + messageId: params.messageId, + textWithoutFooter: params.text, + refresh: { + footerText, + channelId: params.channelId, + serviceUrl: params.serviceUrl, + }, + }; + await assertLock(); + await editTextThreadFooterMessage( + params.provider, + next, + [params.text, footerText].filter(Boolean).join('\n\n'), + ); + if (current) + await rememberThreadReplyFooterAfterEdit({ + provider: params.provider.provider, + channelId: params.channelId, + threadId, + record: next, + assertLock, + lock, + clearOwnFooter: () => + editTextThreadFooterMessage(params.provider, next, params.text), + }); + }, + }); +} diff --git a/packages/communication/src/thread-footer-refresh.ts b/packages/communication/src/thread-footer-refresh.ts new file mode 100644 index 000000000..8666cec59 --- /dev/null +++ b/packages/communication/src/thread-footer-refresh.ts @@ -0,0 +1,340 @@ +import { getRedis } from '@roomote/redis'; +import { Env } from '@roomote/env'; +import { + and, + asc, + db, + eq, + getSessionForTask, + inArray, + isNull, + or, + sessions, + sessionTasks, + taskPullRequests, + tasks, +} from '@roomote/db/server'; +import type { CommunicationProvider } from '@roomote/types'; + +import { + buildThreadReplyFooterText, + formatMarkdownLink, + type ThreadReplyLinkedPr, + type ThreadReplyRunningTasks, +} from './chat-messages'; +import { resolveFastSessionReplyFooterContext } from './fast-session-footer'; +import { + resolveSessionRunningTasks, + resolveThreadReplyFooterContext, +} from './thread-reply-footer-context'; + +export type ThreadFooterRefreshTarget = { + provider: CommunicationProvider | 'source-control'; + channelId: string; + threadId: string; +}; + +/** + * What a refresh pass learned about a destination. + * + * - `active`: coding is running or a preview is live; check again soon. + * - `idle`: nothing is running, but the Session was recently active, so a + * task may still start; check again on the slower cadence. + * - `gone`: the destination unregistered itself (carrier missing, Session + * settled, or the footer can no longer be resolved). Nothing to reschedule. + */ +export type ThreadFooterRefreshOutcome = 'active' | 'idle' | 'gone'; + +const DUE_KEY = 'thread_footer_refresh:due'; + +export const THREAD_FOOTER_REFRESH_ACTIVE_MS = 30_000; +export const THREAD_FOOTER_REFRESH_IDLE_MS = 5 * 60_000; +/** + * A claimed target is leased to the running batch. A batch that crashes or + * outlives the scheduler cadence must not have its targets re-claimed by the + * next tick, so the lease is longer than any healthy batch. + */ +export const THREAD_FOOTER_REFRESH_CLAIM_LEASE_MS = 5 * 60_000; +/** + * An idle footer stops refreshing once its Session has been quiet this long. + * The next reply into the thread re-registers it, so a Session that resumes + * through chat picks refresh back up; one that resumes only from the web app + * shows its new activity on the next chat reply. + */ +export const THREAD_FOOTER_SETTLED_AFTER_MS = 6 * 60 * 60_000; + +/** The index contains destinations, never historical message ids or bodies. */ +export async function scheduleThreadFooterRefresh( + target: ThreadFooterRefreshTarget, +): Promise { + await getRedis().zadd( + DUE_KEY, + Date.now() + THREAD_FOOTER_REFRESH_ACTIVE_MS, + JSON.stringify(target), + ); +} + +/** Set the next check for a destination that stays registered. */ +export async function rescheduleThreadFooterRefresh( + target: ThreadFooterRefreshTarget, + outcome: Exclude, +): Promise { + await getRedis().zadd( + DUE_KEY, + Date.now() + + (outcome === 'active' + ? THREAD_FOOTER_REFRESH_ACTIVE_MS + : THREAD_FOOTER_REFRESH_IDLE_MS), + JSON.stringify(target), + ); +} + +/** + * Atomically claim a bounded, fair batch. Claimed targets are leased to this + * batch; the batch reschedules or forgets each one when it reports, and a + * target whose refresh threw becomes due again when the lease lapses. + */ +export async function claimThreadFooterRefreshTargets( + limit = 100, +): Promise { + const values = (await getRedis().eval( + `local targets = redis.call('zrangebyscore', KEYS[1], '-inf', ARGV[1], 'LIMIT', 0, ARGV[3]) + for _, target in ipairs(targets) do redis.call('zadd', KEYS[1], ARGV[2], target) end + return targets`, + 1, + DUE_KEY, + Date.now(), + Date.now() + THREAD_FOOTER_REFRESH_CLAIM_LEASE_MS, + Math.max(1, Math.min(limit, 100)), + )) as string[]; + const targets: ThreadFooterRefreshTarget[] = []; + for (const value of values) { + try { + const target = JSON.parse(value) as ThreadFooterRefreshTarget; + if ( + !['slack', 'discord', 'teams', 'telegram', 'source-control'].includes( + target.provider, + ) || + typeof target.channelId !== 'string' || + typeof target.threadId !== 'string' + ) + throw new Error('Invalid footer target'); + targets.push(target); + } catch { + await getRedis().zrem(DUE_KEY, value); + } + } + return targets; +} + +/** Call under the destination lock so a concurrent delivery cannot lose registration. */ +export async function forgetThreadFooterRefresh( + target: ThreadFooterRefreshTarget, +): Promise { + await getRedis().zrem(DUE_KEY, JSON.stringify(target)); +} + +/** Only accept the navigation link in a product-generated footer, never body links. */ +export function getThreadFooterNavigationUrl(footerText: string): URL | null { + const link = + /<([^<>|]+)\|Open in Roomote>/.exec(footerText)?.[1] ?? + /\[Open in Roomote\]\(([^()]+)\)/.exec(footerText)?.[1]; + if (!link) return null; + try { + const url = new URL(link.replaceAll('&', '&')); + return url.origin === new URL(Env.R_APP_URL).origin ? url : null; + } catch { + return null; + } +} + +/** PR links already shown by a footer, in display order. */ +export function getThreadFooterPullRequestLinks( + footerText: string, +): ThreadReplyLinkedPr[] { + const links: ThreadReplyLinkedPr[] = []; + for (const match of footerText.matchAll(/<([^<>|]+)\|PR #(\d+)>/g)) { + links.push({ + prNumber: Number(match[2]), + prUrl: match[1]!.replaceAll('&', '&'), + }); + } + for (const match of footerText.matchAll(/\[PR #(\d+)\]\(([^()]+)\)/g)) { + links.push({ prNumber: Number(match[1]), prUrl: match[2]! }); + } + return links; +} + +const TERMINAL_PULL_REQUEST_STATUSES = new Set(['closed', 'merged']); + +/** + * A delivery can know about a pull request the database does not link to the + * Session (an event on a pull request no linked task opened). A refresh only + * sees database state, so it keeps such links until the database says the + * pull request is closed or merged, and keeps the posted order stable. + */ +async function mergeCarriedPullRequests( + footerText: string, + resolved: ThreadReplyLinkedPr[], +): Promise { + const carried = getThreadFooterPullRequestLinks(footerText); + if (carried.length === 0) return resolved; + const resolvedByUrl = new Map(resolved.map((pr) => [pr.prUrl, pr])); + const unresolved = carried.filter((pr) => !resolvedByUrl.has(pr.prUrl)); + const closed = new Set(); + if (unresolved.length > 0) { + const known = await db.query.taskPullRequests.findMany({ + columns: { prUrl: true, status: true }, + where: inArray( + taskPullRequests.prUrl, + unresolved.map((pr) => pr.prUrl), + ), + }); + for (const row of known) { + if (row.status && TERMINAL_PULL_REQUEST_STATUSES.has(row.status)) + closed.add(row.prUrl); + } + } + const merged = new Map(); + for (const pr of carried) { + const current = resolvedByUrl.get(pr.prUrl); + if (current) merged.set(pr.prUrl, current); + else if (!closed.has(pr.prUrl)) merged.set(pr.prUrl, pr); + } + for (const pr of resolved) { + if (!merged.has(pr.prUrl)) merged.set(pr.prUrl, pr); + } + return [...merged.values()]; +} + +export type ThreadFooterActivity = { + /** Coding is running or a preview is live: the footer can change any moment. */ + active: boolean; + /** Nothing is running and the Session has been quiet long enough to stop polling. */ + settled: boolean; +}; + +export function classifyThreadFooterActivity(context: { + runningTasks?: ThreadReplyRunningTasks | null; + livePreviewUrl?: string | null; + /** Session `activityAt` in epoch milliseconds; null when there is no Session. */ + sessionActivityAt?: number | null; +}): ThreadFooterActivity { + const active = + (context.runningTasks?.count ?? 0) > 0 || Boolean(context.livePreviewUrl); + const quietForMs = Date.now() - (context.sessionActivityAt ?? 0); + return { + active, + settled: !active && quietForMs > THREAD_FOOTER_SETTLED_AFTER_MS, + }; +} + +export type CurrentThreadFooter = ThreadFooterActivity & { text: string }; + +/** Re-resolve current state, keeping the carrier's navigation and presentation. */ +export async function resolveCurrentThreadFooter( + provider: string, + footerText: string, +): Promise { + const url = getThreadFooterNavigationUrl(footerText); + if (!url) return null; + const match = /^\/(sessions|task)\/([^/]+)$/.exec(url.pathname); + if (!match) return null; + const id = match[2]!; + let context: { + runningTasks?: ThreadReplyRunningTasks | null; + linkedPrs: ThreadReplyLinkedPr[]; + livePreviewUrl: string | null; + webAppUrl?: string | null; + }; + let sessionActivityAt: number | null = null; + if (match[1] === 'sessions') { + // Session links carry either the Session id or its Fast conversation id. + const session = await db.query.sessions.findFirst({ + columns: { id: true, fastConversationId: true, activityAt: true }, + where: or(eq(sessions.id, id), eq(sessions.fastConversationId, id)), + }); + sessionActivityAt = session?.activityAt ?? null; + if (session && !session.fastConversationId) { + const linkedTasks = await db + .select({ taskId: sessionTasks.taskId }) + .from(sessionTasks) + .innerJoin(tasks, eq(tasks.id, sessionTasks.taskId)) + .where( + and(eq(sessionTasks.sessionId, session.id), isNull(tasks.deletedAt)), + ) + .orderBy(asc(sessionTasks.attachedAt), asc(sessionTasks.taskId)); + const taskIds = linkedTasks.map(({ taskId }) => taskId); + const [runningTasks, contexts] = await Promise.all([ + resolveSessionRunningTasks(session.id, taskIds), + Promise.all( + taskIds.map((taskId) => + resolveThreadReplyFooterContext({ + taskId, + prRepo: null, + prNumber: null, + includeRunningTasks: false, + }), + ), + ), + ]); + context = { + runningTasks, + linkedPrs: [ + ...new Map( + contexts + .flatMap((entry) => entry.linkedPrs) + .map((pr) => [pr.prUrl, pr]), + ).values(), + ], + livePreviewUrl: + contexts.find((entry) => entry.livePreviewUrl)?.livePreviewUrl ?? + null, + }; + } else { + context = await resolveFastSessionReplyFooterContext({ + sessionId: session?.fastConversationId ?? id, + }); + } + } else { + const [taskContext, session] = await Promise.all([ + resolveThreadReplyFooterContext({ + taskId: id, + prRepo: null, + prNumber: null, + }), + getSessionForTask(db, id), + ]); + context = taskContext; + sessionActivityAt = session?.activityAt ?? null; + } + const linkedPrs = await mergeCarriedPullRequests( + footerText, + context.linkedPrs, + ); + const text = buildThreadReplyFooterText({ + taskUrl: url.toString(), + ...context, + linkedPrs, + formatLink: + provider === 'slack' + ? (label, href) => `<${href}|${label}>` + : formatMarkdownLink, + ...(provider === 'discord' + ? { formatFooterText: (text: string) => `-# ${text}` } + : provider === 'github' + ? { formatFooterText: (text: string) => `${text}` } + : {}), + }); + return { + text, + ...classifyThreadFooterActivity({ ...context, sessionActivityAt }), + }; +} + +export async function resolveCurrentThreadFooterText( + provider: string, + footerText: string, +): Promise { + return (await resolveCurrentThreadFooter(provider, footerText))?.text ?? null; +} diff --git a/packages/communication/src/thread-reply-footer-context.ts b/packages/communication/src/thread-reply-footer-context.ts index 429d63af8..e5184c6b4 100644 --- a/packages/communication/src/thread-reply-footer-context.ts +++ b/packages/communication/src/thread-reply-footer-context.ts @@ -1,10 +1,17 @@ import { db, + and, + desc, environments, eq, + getSessionForTask, + inArray, + isNull, resolveEffectivePreviewRuntimeConfig, taskPullRequests, taskRuns, + sessionTasks, + tasks, } from '@roomote/db/server'; import { Env } from '@roomote/env'; import type { PullRequestStatus } from '@roomote/types'; @@ -13,10 +20,16 @@ import { buildPreviewProxyUrl, getPrimaryPortFromConfig, hasConfiguredPreviewPorts, + isExitedRunStatus, + isTaskExecutingTurn, portNameToSlug, + SYSTEM_PORT_NAMES, } from '@roomote/types'; -import type { ThreadReplyLinkedPr } from './chat-messages'; +import type { + ThreadReplyLinkedPr, + ThreadReplyRunningTasks, +} from './chat-messages'; const TERMINAL_LINKED_TASK_PR_STATUSES = new Set([ 'closed', @@ -26,6 +39,48 @@ const TERMINAL_LINKED_TASK_PR_STATUSES = new Set([ export interface ThreadReplyFooterContext { linkedPrs: ThreadReplyLinkedPr[]; livePreviewUrl: string | null; + runningTasks?: ThreadReplyRunningTasks | null; + webAppUrl?: string | null; +} + +export async function resolveSessionRunningTasks( + sessionId: string, + linkedTaskIds?: string[], +): Promise { + const taskIds = + linkedTaskIds ?? + ( + await db + .select({ taskId: sessionTasks.taskId }) + .from(sessionTasks) + .innerJoin(tasks, eq(tasks.id, sessionTasks.taskId)) + .where( + and(eq(sessionTasks.sessionId, sessionId), isNull(tasks.deletedAt)), + ) + ).map(({ taskId }) => taskId); + if (taskIds.length === 0) return null; + // One query for every task's latest run; this runs on each reply and refresh. + const latestRuns = await db + .selectDistinctOn([taskRuns.taskId], { + taskId: taskRuns.taskId, + status: taskRuns.status, + taskPhase: taskRuns.taskPhase, + }) + .from(taskRuns) + .where(inArray(taskRuns.taskId, taskIds)) + .orderBy(taskRuns.taskId, desc(taskRuns.createdAt), desc(taskRuns.id)); + const latestRunByTaskId = new Map(latestRuns.map((run) => [run.taskId, run])); + const runningTaskIds = taskIds.filter((taskId) => { + const run = latestRunByTaskId.get(taskId); + return isTaskExecutingTurn(run?.status, run?.taskPhase); + }); + // Session's task-list panel has no URL state; /tasks is the supported list route. + const url = new URL(`${Env.R_APP_URL}/tasks`); + if (runningTaskIds.length === 1) { + url.pathname = `/sessions/${sessionId}`; + url.searchParams.set('task', runningTaskIds[0]!); + } + return { count: runningTaskIds.length, url: url.toString() }; } export function buildThreadReplyPrUrl(params: { @@ -142,11 +197,28 @@ export async function resolveThreadReplyLivePreviewUrl( columns: { payload: true, primaryPortName: true, + status: true, + sleepRequestedAt: true, + snapshotRequestedAt: true, + snapshotCreatedAt: true, + snapshotFailedAt: true, + snapshotId: true, }, where: eq(taskRuns.taskId, taskId), - orderBy: (table, { desc }) => [desc(table.createdAt)], + orderBy: (table, { desc }) => [desc(table.createdAt), desc(table.id)], }); + if ( + !taskRun || + isExitedRunStatus(taskRun.status) || + taskRun.snapshotId || + ((taskRun.sleepRequestedAt || taskRun.snapshotRequestedAt) && + !taskRun.snapshotCreatedAt && + !taskRun.snapshotFailedAt) + ) { + return null; + } + const environmentId = ( taskRun?.payload as { environmentId?: string } | undefined )?.environmentId; @@ -166,9 +238,12 @@ export async function resolveThreadReplyLivePreviewUrl( return null; } + const ports = environment?.config?.ports?.filter( + (port) => !SYSTEM_PORT_NAMES.has(port.name.toUpperCase()), + ); const primaryPortName = - taskRun?.primaryPortName ?? - getPrimaryPortFromConfig(environment?.config?.ports)?.name; + ports?.find((port) => port.name === taskRun.primaryPortName)?.name ?? + getPrimaryPortFromConfig(ports)?.name; if (!primaryPortName) { return null; @@ -208,14 +283,28 @@ export async function resolveThreadReplyFooterContext(params: { taskId: string | null | undefined; prRepo: string | null | undefined; prNumber: number | null | undefined; + /** Fast resolves status once for the entire Session, not once per task. */ + includeRunningTasks?: boolean; }): Promise { const [linkedPrs, livePreviewUrl] = await Promise.all([ resolveThreadReplyLinkedPrs(params), resolveThreadReplyLivePreviewUrl(params.taskId), ]); + const session = + params.taskId && params.includeRunningTasks !== false + ? await getSessionForTask(db, params.taskId) + : null; + const runningTasks = session + ? await resolveSessionRunningTasks(session.id) + : null; + return { linkedPrs, livePreviewUrl, + ...(session + ? { webAppUrl: `${Env.R_APP_URL}/sessions/${session.id}` } + : {}), + ...(runningTasks ? { runningTasks } : {}), }; } diff --git a/packages/communication/src/thread-reply-footer-delivery.ts b/packages/communication/src/thread-reply-footer-delivery.ts index 840e2cf2e..24e483428 100644 --- a/packages/communication/src/thread-reply-footer-delivery.ts +++ b/packages/communication/src/thread-reply-footer-delivery.ts @@ -6,11 +6,24 @@ import { getThreadReplyFooterRecord, setThreadReplyFooterRecord, type ThreadReplyFooterRecord, + type ThreadReplyFooterLock, } from './thread-reply-footer-state'; import type { CommunicationProvider } from '@roomote/types'; +import { + forgetThreadFooterRefresh, + resolveCurrentThreadFooter, + type ThreadFooterRefreshOutcome, + type ThreadFooterRefreshTarget, +} from './thread-footer-refresh'; +import { chunkTelegramMarkdownAsHtml } from './telegram-format'; const THREAD_REPLY_FOOTER_LOCK_TTL_SECONDS = 30; -const THREAD_REPLY_FOOTER_LOCK_MAX_ATTEMPTS = 8; +/** + * A delivery waits out a concurrent delivery or a refresh's provider edit + * (which can sit in a rate-limit backoff for a few seconds) rather than + * failing the reply. Refreshes themselves never wait: they try once. + */ +const THREAD_REPLY_FOOTER_LOCK_MAX_ATTEMPTS = 40; const THREAD_REPLY_FOOTER_LOCK_RETRY_MS = 100; const RELEASE_LOCK_SCRIPT = "if redis.call('get',KEYS[1])==ARGV[1] then return redis.call('del',KEYS[1]) else return 0 end"; @@ -21,7 +34,10 @@ export const THREAD_REPLY_FOOTER_LOCK_TIMEOUT_MESSAGE = export async function withThreadReplyFooterLock(params: { lockKey: string; maxAcquireAttempts?: number; - fn: () => Promise; + fn: ( + assertLock: () => Promise, + lock: ThreadReplyFooterLock, + ) => Promise; }): Promise { const redis = getRedis(); const maxAcquireAttempts = @@ -38,9 +54,40 @@ export async function withThreadReplyFooterLock(params: { ); if (acquired) { + let lost = false; + const assertLock = async () => { + if (lost || (await redis.get(params.lockKey)) !== ownerId) { + lost = true; + throw new Error('Thread reply footer lock lease lost'); + } + }; + // Provider retries can outlive the initial lease. Renew only our own lock. + let renewal = Promise.resolve(); + const timer = setInterval( + () => { + renewal = renewal + .then(async () => { + const renewed = await redis.eval( + "if redis.call('get',KEYS[1])==ARGV[1] then return redis.call('expire',KEYS[1],ARGV[2]) else return 0 end", + 1, + params.lockKey, + ownerId, + THREAD_REPLY_FOOTER_LOCK_TTL_SECONDS, + ); + if (!renewed) lost = true; + }) + .catch(() => { + lost = true; + }); + }, + (THREAD_REPLY_FOOTER_LOCK_TTL_SECONDS * 1000) / 3, + ); + timer.unref(); try { - return await params.fn(); + return await params.fn(assertLock, { key: params.lockKey, ownerId }); } finally { + clearInterval(timer); + await renewal; await redis .eval(RELEASE_LOCK_SCRIPT, 1, params.lockKey, ownerId) .catch(() => {}); @@ -55,16 +102,47 @@ export async function withThreadReplyFooterLock(params: { throw new Error(THREAD_REPLY_FOOTER_LOCK_TIMEOUT_MESSAGE); } +/** + * Run `fn` under the destination lock only if it is free right now. Background + * work (a scheduled refresh) must never delay a reply, so it yields instead of + * waiting and reports `acquired: false`. + */ +export async function tryThreadReplyFooterLock(params: { + lockKey: string; + fn: ( + assertLock: () => Promise, + lock: ThreadReplyFooterLock, + ) => Promise; +}): Promise<{ acquired: true; value: T } | { acquired: false }> { + try { + const value = await withThreadReplyFooterLock({ + lockKey: params.lockKey, + maxAcquireAttempts: 1, + fn: params.fn, + }); + return { acquired: true, value }; + } catch (error) { + if ( + error instanceof Error && + error.message === THREAD_REPLY_FOOTER_LOCK_TIMEOUT_MESSAGE + ) + return { acquired: false }; + throw error; + } +} + type PostedFooterRecord = T & { textWithoutFooter: string; images?: ThreadReplyFooterRecord['images']; + refresh?: ThreadReplyFooterRecord['refresh']; + buttons?: ThreadReplyFooterRecord['buttons']; }; /** * Post a reply that becomes the thread's footer-bearing message: read the * previous footer record, post the new reply (with footer attached by the - * caller), rewrite the previous message without its footer, and persist the - * new record. Managed-provider counterpart of the Slack sticky footer ops. + * caller), persist the new pointer, then clear the previous message's footer. + * Managed-provider counterpart of the Slack sticky footer ops. * * apps/api's MCP thread replies keep their own copy of this flow * (handlers/mcp/communication-thread-reply-shared.ts) because its tests mock @@ -88,7 +166,7 @@ export async function deliverManagedThreadReplyFooter< }): Promise { return withThreadReplyFooterLock({ lockKey: params.lockKey, - fn: async () => { + fn: async (assertLock, lock) => { let previousFooterRecord: ThreadReplyFooterRecord | null = null; try { previousFooterRecord = await getThreadReplyFooterRecord( @@ -104,45 +182,254 @@ export async function deliverManagedThreadReplyFooter< ); } + await assertLock(); const posted = await params.postReplyWithFooter(); - if ( - previousFooterRecord && - previousFooterRecord.messageId !== posted.messageId - ) { - try { - await params.clearPreviousFooter(previousFooterRecord); - } catch (error) { - console.error( - `[${params.logContext}] Failed to clear prior ${params.providerLabel} footer message ${previousFooterRecord.messageId}: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - } - } - try { - await setThreadReplyFooterRecord( + await assertLock(); + const written = await setThreadReplyFooterRecord( params.provider, params.channelId, params.footerStateThreadId, { messageId: posted.messageId, textWithoutFooter: posted.textWithoutFooter, + ...(posted.refresh ? { refresh: posted.refresh } : {}), + ...(posted.buttons ? { buttons: posted.buttons } : {}), ...(posted.images && posted.images.length > 0 ? { images: posted.images } : {}), }, + { lock }, ); + if (!written) throw new Error('Thread reply footer lock lease lost'); } catch (error) { console.error( `[${params.logContext}] Failed to persist latest ${params.providerLabel} footer record ${posted.messageId}: ${ error instanceof Error ? error.message : String(error) }`, ); + // Do not clear the old carrier while its pointer may still be current. + // Otherwise a later refresh could put a footer back onto history. + const current = await getThreadReplyFooterRecord( + params.provider, + params.channelId, + params.footerStateThreadId, + ).catch(() => undefined); + if (current !== undefined && current?.messageId !== posted.messageId) { + await params.clearPreviousFooter(posted).catch(() => {}); + } + return posted; + } + + if ( + previousFooterRecord && + previousFooterRecord.messageId !== posted.messageId + ) { + try { + await assertLock(); + await params.clearPreviousFooter(previousFooterRecord); + } catch (error) { + console.error( + `[${params.logContext}] Failed to clear prior ${params.providerLabel} footer message ${previousFooterRecord.messageId}`, + error, + ); + } } return posted; }, }); } + +function managedFooterLockKey(target: ThreadFooterRefreshTarget): string { + return `${target.provider}:thread_reply_footer_lock:${target.channelId}:${target.threadId}`; +} + +/** + * Unregister a destination, but only if its record still matches what this + * refresh pass read: a delivery that raced in owns the registration now. + */ +async function forgetManagedFooterIfUnchanged( + target: ThreadFooterRefreshTarget & { provider: CommunicationProvider }, + seen: ThreadReplyFooterRecord | null, +): Promise { + const result = await tryThreadReplyFooterLock({ + lockKey: managedFooterLockKey(target), + fn: async (assertLock): Promise => { + const current = await getThreadReplyFooterRecord( + target.provider, + target.channelId, + target.threadId, + ); + const unchanged = + current?.messageId === seen?.messageId && + current?.refresh?.footerText === seen?.refresh?.footerText; + if (!unchanged) return 'active'; + await assertLock(); + await forgetThreadFooterRefresh(target); + return 'gone'; + }, + }); + return result.acquired ? result.value : 'active'; +} + +/** + * Bring the current carrier's footer up to date. All resolution happens + * outside the destination lock; the lock is held only for the provider edit + * and the pointer write, so a concurrent reply is never starved by DB work. + */ +export async function refreshManagedThreadReplyFooter(params: { + provider: CommunicationProvider; + channelId: string; + threadId: string; + edit: (record: ThreadReplyFooterRecord, text: string) => Promise; +}): Promise { + const target = { + provider: params.provider, + channelId: params.channelId, + threadId: params.threadId, + }; + const record = await getThreadReplyFooterRecord( + params.provider, + params.channelId, + params.threadId, + ); + if (!record?.refresh) return forgetManagedFooterIfUnchanged(target, record); + const refresh = record.refresh; + const current = await resolveCurrentThreadFooter( + params.provider, + refresh.footerText, + ); + if (!current) { + // The footer no longer names a Session or task on this deployment (for + // example the app URL changed). Polling cannot fix that; a new reply + // registers a fresh footer. + console.warn('[threadFooter] Retiring a footer that no longer resolves', { + provider: params.provider, + channelId: params.channelId, + threadId: params.threadId, + }); + return forgetManagedFooterIfUnchanged(target, record); + } + const outcome: ThreadFooterRefreshOutcome = current.active + ? 'active' + : 'idle'; + if (current.text === refresh.footerText) { + return current.settled + ? forgetManagedFooterIfUnchanged(target, record) + : outcome; + } + const text = [record.textWithoutFooter, current.text] + .filter(Boolean) + .join('\n\n'); + // A refresh cannot split a message or post a replacement carrier, and the + // body never shrinks, so this carrier will not fit on later passes either. + if ( + (params.provider === 'discord' && text.length > 2000) || + (params.provider === 'telegram' && + chunkTelegramMarkdownAsHtml(text).length > 1) + ) { + console.warn('[threadFooter] Retiring a footer whose carrier is full', { + provider: params.provider, + channelId: params.channelId, + threadId: params.threadId, + }); + return forgetManagedFooterIfUnchanged(target, record); + } + const result = await tryThreadReplyFooterLock({ + lockKey: managedFooterLockKey(target), + fn: async (assertLock, lock): Promise => { + const latest = await getThreadReplyFooterRecord( + params.provider, + params.channelId, + params.threadId, + ); + // A reply relocated the footer while this pass was resolving; the next + // pass reads the new carrier. + if ( + !latest?.refresh || + latest.messageId !== record.messageId || + latest.refresh.footerText !== refresh.footerText + ) + return 'active'; + await assertLock(); + try { + await params.edit(latest, text); + } catch (error) { + const status = + error && typeof error === 'object' && 'status' in error + ? error.status + : null; + const message = error instanceof Error ? error.message : ''; + const missing = + status === 404 || + status === 410 || + (params.provider === 'telegram' && + /message to edit not found/i.test(message)) || + (params.provider === 'teams' && + /^Teams updateActivity failed with (404|410):/.test(message)); + if (!missing) throw error; + await assertLock(); + await forgetThreadFooterRefresh(target); + return 'gone'; + } + await rememberThreadReplyFooterAfterEdit({ + ...target, + record: { + ...latest, + refresh: { ...latest.refresh, footerText: current.text }, + }, + assertLock, + lock, + clearOwnFooter: () => params.edit(latest, latest.textWithoutFooter), + keepTtl: true, + }); + if (current.settled) { + await assertLock(); + await forgetThreadFooterRefresh(target); + return 'gone'; + } + return outcome; + }, + }); + // A delivery holds the lock: it re-registers the destination itself. + return result.acquired ? result.value : 'active'; +} + +/** An edit may finish after a competing delivery acquired the lease. */ +export async function rememberThreadReplyFooterAfterEdit(params: { + provider: CommunicationProvider; + channelId: string; + threadId: string; + record: ThreadReplyFooterRecord; + assertLock: () => Promise; + lock: ThreadReplyFooterLock; + clearOwnFooter: () => Promise; + keepTtl?: boolean; +}): Promise { + let ownsLock = true; + try { + await params.assertLock(); + } catch { + ownsLock = false; + } + if ( + ownsLock && + (await setThreadReplyFooterRecord( + params.provider, + params.channelId, + params.threadId, + params.record, + { keepTtl: params.keepTtl, lock: params.lock }, + )) + ) + return; + const current = await getThreadReplyFooterRecord( + params.provider, + params.channelId, + params.threadId, + ).catch(() => undefined); + if (current !== undefined && current?.messageId !== params.record.messageId) + await params.clearOwnFooter().catch(() => {}); +} diff --git a/packages/communication/src/thread-reply-footer-state.ts b/packages/communication/src/thread-reply-footer-state.ts index 403af4ab2..99baad1ac 100644 --- a/packages/communication/src/thread-reply-footer-state.ts +++ b/packages/communication/src/thread-reply-footer-state.ts @@ -1,8 +1,12 @@ import { getRedis } from '@roomote/redis'; import type { CommunicationProvider } from '@roomote/types'; +import { scheduleThreadFooterRefresh } from './thread-footer-refresh'; +import type { CommunicationMessageButton } from './provider'; const THREAD_REPLY_FOOTER_TTL_SECONDS = 30 * 24 * 60 * 60; +export type ThreadReplyFooterLock = { key: string; ownerId: string }; + export type ThreadReplyFooterImage = { url: string; altText: string; @@ -23,6 +27,13 @@ export type ThreadReplyFooterRecord = { * re-edits do not drop attachment content. */ images?: ThreadReplyFooterImage[]; + buttons?: CommunicationMessageButton[][]; + refresh?: { + footerText: string; + /** Discord thread channels differ from the pointer's parent channel. */ + channelId: string; + serviceUrl?: string; + }; }; function parseThreadReplyFooterImages( @@ -106,6 +117,12 @@ export async function getThreadReplyFooterRecord( messageId: parsed.messageId, textWithoutFooter: parsed.textWithoutFooter, ...(images ? { images } : {}), + ...(Array.isArray(parsed.buttons) ? { buttons: parsed.buttons } : {}), + ...(parsed.refresh && + typeof parsed.refresh.footerText === 'string' && + typeof parsed.refresh.channelId === 'string' + ? { refresh: parsed.refresh } + : {}), }; } @@ -115,17 +132,58 @@ export async function getThreadReplyFooterRecord( } } +/** + * Returns false when the supplied lease no longer owns the lock, or when a + * `keepTtl` write found no record to update (it expired since it was read). + */ export async function setThreadReplyFooterRecord( provider: CommunicationProvider, channelId: string, threadId: string, record: ThreadReplyFooterRecord, -): Promise { + options?: { keepTtl?: boolean; lock?: ThreadReplyFooterLock }, +): Promise { const redis = getRedis(); - await redis.set( - getThreadReplyFooterKey(provider, channelId, threadId), - JSON.stringify(record), - 'EX', - THREAD_REPLY_FOOTER_TTL_SECONDS, - ); + if (options?.lock) { + const written = await redis.eval( + `if redis.call('get', KEYS[1]) ~= ARGV[1] then return 0 end + if ARGV[3] == 'keepTtl' then + -- A record that expired since it was read cannot be revived: report it. + if not redis.call('set', KEYS[2], ARGV[2], 'KEEPTTL', 'XX') then return 0 end + else + redis.call('set', KEYS[2], ARGV[2], 'EX', ARGV[3]) + end + return 1`, + 2, + options.lock.key, + getThreadReplyFooterKey(provider, channelId, threadId), + options.lock.ownerId, + JSON.stringify(record), + options.keepTtl ? 'keepTtl' : THREAD_REPLY_FOOTER_TTL_SECONDS, + ); + if (!written) return false; + if (options.keepTtl) return true; + } else if (options?.keepTtl) { + const written = await redis.set( + getThreadReplyFooterKey(provider, channelId, threadId), + JSON.stringify(record), + 'KEEPTTL', + 'XX', + ); + return written === 'OK'; + } else { + await redis.set( + getThreadReplyFooterKey(provider, channelId, threadId), + JSON.stringify(record), + 'EX', + THREAD_REPLY_FOOTER_TTL_SECONDS, + ); + } + if (record.refresh) + await scheduleThreadFooterRefresh({ provider, channelId, threadId }).catch( + (error) => { + console.warn('[threadFooter] Failed to schedule footer refresh', error); + }, + ); + return true; } diff --git a/packages/sdk/src/server/index.ts b/packages/sdk/src/server/index.ts index 07b0d0ef0..c9e72d24c 100644 --- a/packages/sdk/src/server/index.ts +++ b/packages/sdk/src/server/index.ts @@ -207,6 +207,7 @@ export { } from './lib/discord-persistence'; export { createDiscordCommunicationProviderFromRuntimeCredentials } from './lib/discord-communication'; +export { refreshCurrentThreadFooters } from './lib/thread-footer-refresh'; export { createTeamsCommunicationProviderFromRuntimeCredentials } from './lib/teams-communication'; diff --git a/packages/sdk/src/server/lib/fast-agent-parent-event.test.ts b/packages/sdk/src/server/lib/fast-agent-parent-event.test.ts index 565793ed0..c062f209e 100644 --- a/packages/sdk/src/server/lib/fast-agent-parent-event.test.ts +++ b/packages/sdk/src/server/lib/fast-agent-parent-event.test.ts @@ -1,4 +1,5 @@ const mocks = vi.hoisted(() => ({ + redisStore: new Map(), acquireTurnLock: vi.fn(), releaseTurnLock: Object.assign(vi.fn(), { signal: new AbortController().signal, @@ -69,11 +70,37 @@ vi.mock('@roomote/redis', async (importOriginal) => { return { ...actual, // The sticky-footer lock and state live in Redis; these tests run without - // a server, so satisfy lock acquisition and empty prior state. + // a server, so model lock ownership and atomic carrier writes in memory. getRedis: () => ({ - set: async () => 'OK', - get: async () => null, - eval: async () => 1, + set: async (key: string, value: string, ...args: unknown[]) => { + if (args.includes('NX') && mocks.redisStore.has(key)) return null; + mocks.redisStore.set(key, value); + return 'OK'; + }, + get: async (key: string) => mocks.redisStore.get(key) ?? null, + eval: async ( + script: string, + count: number, + key: string, + ...args: (string | number)[] + ) => { + const owner = args[count - 1]; + if (mocks.redisStore.get(key) !== owner) return 0; + if (count === 2) { + const [pointerKey, , record, ttl] = args as [ + string, + string, + string, + string | number, + ]; + if (ttl !== 'keepTtl' || mocks.redisStore.has(pointerKey)) { + mocks.redisStore.set(pointerKey, record); + } + } + if (script.includes("'del'")) mocks.redisStore.delete(key); + return 1; + }, + zadd: async () => 1, }), }; }); @@ -90,6 +117,19 @@ vi.mock('@roomote/communication', async (importOriginal) => ({ ), })); +vi.mock( + '@roomote/communication/thread-footer-refresh', + async (importOriginal) => ({ + ...(await importOriginal< + typeof import('@roomote/communication/thread-footer-refresh') + >()), + resolveCurrentThreadFooterText: async ( + _provider: string, + footerText: string, + ) => footerText, + }), +); + vi.mock('@roomote/cloud-agents/server', () => ({ acquireFastAgentTurnLock: mocks.acquireTurnLock, answerFastAgentQuestion: mocks.answerQuestion, @@ -285,6 +325,7 @@ const event = { describe('deliverFastAgentParentEvent', () => { beforeEach(() => { + mocks.redisStore.clear(); vi.clearAllMocks(); mocks.findWakeupSession.mockResolvedValue({ id: originSessionId }); mocks.releaseTurnLock.signal = new AbortController().signal; @@ -383,6 +424,7 @@ describe('deliverFastAgentParentEvent', () => { messageId: 'teams-message-1', }); mocks.createTeamsProvider.mockResolvedValue({ + provider: 'teams', postMessage: mocks.teamsPostMessage, updateMessage: mocks.teamsUpdateMessage, }); @@ -393,6 +435,7 @@ describe('deliverFastAgentParentEvent', () => { lastTextMessageId: 'telegram-message-2', }); mocks.createTelegramProvider.mockResolvedValue({ + provider: 'telegram', postMessage: mocks.telegramPostMessage, sendChatAction: mocks.telegramTyping, editMessageText: mocks.telegramEditMessage, @@ -803,7 +846,7 @@ describe('deliverFastAgentParentEvent', () => { elements: [ { type: 'mrkdwn', - text: expect.stringContaining('Reply or use the'), + text: expect.stringContaining('|Open in Roomote>'), }, ], }, @@ -2169,7 +2212,7 @@ describe('deliverFastAgentParentEvent', () => { channelId: 'channel-1', idempotencyKey: 'fast-parent-artifact:artifact-1:v1', text: expect.stringMatching( - /^The proof is ready\.\n\n-# Reply or use the \[web app\]\(.*\/sessions\/.*\)\.$/, + /^The proof is ready\.\n\n-# \[Open in Roomote\]\(.*\/sessions\/.*\)$/, ), textFormat: 'markdown', images: [ @@ -2217,6 +2260,8 @@ describe('deliverFastAgentParentEvent', () => { channelId: 'teams-channel-1', threadId: 'teams-root-1', post: mocks.teamsPostMessage, + edit: mocks.teamsUpdateMessage, + messageId: 'teams-message-1', }, { surface: 'telegram' as const, @@ -2224,10 +2269,20 @@ describe('deliverFastAgentParentEvent', () => { channelId: 'telegram-chat-1', threadId: undefined, post: mocks.telegramPostMessage, + edit: mocks.telegramEditMessage, + messageId: 'telegram-message-2', }, ])( 'delivers a $surface parent event through its provider adapter', - async ({ surface, workspaceId, channelId, threadId, post }) => { + async ({ + surface, + workspaceId, + channelId, + threadId, + post, + edit, + messageId, + }) => { await deliverFastAgentParentEvent({ parent: { ...parent, @@ -2250,7 +2305,7 @@ describe('deliverFastAgentParentEvent', () => { ...(threadId ? { threadId } : {}), text: expect.stringMatching( new RegExp( - `^The proof is ready\\.\\n\\n.*Reply or use the \\[web app\\]\\(.*utm_source=${surface}.*\\)\\..*$`, + `^The proof is ready\\.\\n\\n\\[Open in Roomote\\]\\(.*utm_source=${surface}.*\\)$`, ), ), textFormat: 'markdown', @@ -2263,9 +2318,67 @@ describe('deliverFastAgentParentEvent', () => { ], }), ); + const footerText = post.mock.calls[0]![0].text.split('\n\n').at(-1); + expect( + JSON.parse( + mocks.redisStore.get( + `${surface}:thread_reply_footer:${channelId}:${threadId ?? 'root'}`, + )!, + ), + ).toEqual( + expect.objectContaining({ + messageId, + textWithoutFooter: 'The proof is ready.', + refresh: expect.objectContaining({ channelId, footerText }), + }), + ); + expect(edit).not.toHaveBeenCalled(); }, ); + it('clears its own Teams footer without recording a carrier after losing the lease', async () => { + const lockKey = + 'teams:thread_reply_footer_lock:teams-channel-1:teams-root-1'; + mocks.teamsPostMessage.mockImplementationOnce(async () => { + mocks.redisStore.set(lockKey, 'new-owner'); + return { + provider: 'teams', + channelId: 'teams-channel-1', + messageId: 'teams-message-1', + }; + }); + + await deliverFastAgentParentEvent({ + parent: { + ...parent, + conversation: { + surface: 'teams', + workspaceId: 'tenant-1', + conversationId: 'teams-conversation-1', + replyTarget: { + channelId: 'teams-channel-1', + threadId: 'teams-root-1', + }, + }, + }, + event, + }); + + expect(mocks.teamsUpdateMessage).toHaveBeenCalledExactlyOnceWith( + expect.objectContaining({ + channelId: 'teams-channel-1', + messageId: 'teams-message-1', + text: 'The proof is ready.', + }), + ); + expect( + mocks.redisStore.has( + 'teams:thread_reply_footer:teams-channel-1:teams-root-1', + ), + ).toBe(false); + expect(mocks.redisStore.get(lockKey)).toBe('new-owner'); + }); + it('updates the Teams automation root instead of posting a duplicate report', async () => { await deliverFastAgentParentEvent({ parent: { @@ -3100,7 +3213,7 @@ describe('deliverFastAgentParentEvent', () => { threadId: 'thread-1', idempotencyKey: 'fast-parent-pr-feedback:feedback-123', text: expect.stringMatching( - /^There is new PR feedback\.\n\n-# Working on \[PR #42\]\(https:\/\/github\.com\/acme\/web\/pull\/42\), reply or use the \[web app\]\(.*\/sessions\/.*\)\.$/, + /^There is new PR feedback\.\n\n-# \[PR #42\]\(https:\/\/github\.com\/acme\/web\/pull\/42\) · \[Open in Roomote\]\(.*\/sessions\/.*\)$/, ), textFormat: 'markdown', images: [], diff --git a/packages/sdk/src/server/lib/fast-agent-parent-event.ts b/packages/sdk/src/server/lib/fast-agent-parent-event.ts index 71e700eef..c16a4dc95 100644 --- a/packages/sdk/src/server/lib/fast-agent-parent-event.ts +++ b/packages/sdk/src/server/lib/fast-agent-parent-event.ts @@ -43,6 +43,7 @@ import { import { buildFastSessionReplyFooterText, deliverManagedThreadReplyFooter, + postTextThreadReplyWithFooter, getDiscordFooterlessFinalChunk, resolveFastSessionReplyFooterContext, type FastSessionReplyFooterContext, @@ -1287,6 +1288,10 @@ async function postDiscordFastParentMessageWithFooter(params: { textWithFooter: params.textWithFooter, footerText: params.footerText, }), + refresh: { + footerText: params.footerText, + channelId: params.conversation.replyTarget.threadId ?? channelId, + }, }; }, clearPreviousFooter: async (previousFooterRecord) => { @@ -1631,19 +1636,28 @@ async function createTeamsFastAgentParentTurn( suggestions.length > 0, ) : message; - const text = `${reportMessage}\n\n${buildFastSessionReplyFooterText({ provider: 'teams', sessionId: params.parent.sessionId, ...params.footerContext })}`; + const footerText = buildFastSessionReplyFooterText({ + provider: 'teams', + sessionId: params.parent.sessionId, + ...params.footerContext, + }); if ( params.event.type === 'automation_triggered' && params.event.rootMessageId && !kickoff ) { - await provider.updateMessage({ - channelId: conversation.replyTarget.channelId, + await postTextThreadReplyWithFooter({ + provider, messageId: params.event.rootMessageId, - serviceUrl, - text, - textFormat: 'markdown', - images, + footerText, + input: { + channelId: conversation.replyTarget.channelId, + threadId: conversation.replyTarget.threadId, + serviceUrl, + text: reportMessage, + textFormat: 'markdown', + images, + }, }); await recordFastAgentConversationMessageBestEffort({ sessionId: session.id, @@ -1669,18 +1683,22 @@ async function createTeamsFastAgentParentTurn( params.onReplyPosted(); return { messageId: params.event.rootMessageId }; } - const posted = await provider.postMessage({ - channelId: conversation.replyTarget.channelId, - serviceUrl, - ...(conversation.replyTarget.threadId - ? { - threadId: conversation.replyTarget.threadId, - replyToMessageId: conversation.replyTarget.threadId, - } - : {}), - text, - textFormat: 'markdown', - images, + const posted = await postTextThreadReplyWithFooter({ + provider, + footerText, + input: { + channelId: conversation.replyTarget.channelId, + serviceUrl, + ...(conversation.replyTarget.threadId + ? { + threadId: conversation.replyTarget.threadId, + replyToMessageId: conversation.replyTarget.threadId, + } + : {}), + text: reportMessage, + textFormat: 'markdown', + images, + }, }); if ( isFastAutomationReportEvent(params.event) && @@ -1784,14 +1802,22 @@ async function createTelegramFastAgentParentTurn( suggestions.length > 0, ) : message; - const posted = await provider.postMessage({ - channelId: conversation.replyTarget.channelId, - ...(conversation.replyTarget.threadId - ? { threadId: conversation.replyTarget.threadId } - : {}), - text: `${reportMessage}\n\n${buildFastSessionReplyFooterText({ provider: 'telegram', sessionId: params.parent.sessionId, ...params.footerContext })}`, - textFormat: 'markdown', - images, + const posted = await postTextThreadReplyWithFooter({ + provider, + input: { + channelId: conversation.replyTarget.channelId, + ...(conversation.replyTarget.threadId + ? { threadId: conversation.replyTarget.threadId } + : {}), + text: reportMessage, + textFormat: 'markdown', + images, + }, + footerText: buildFastSessionReplyFooterText({ + provider: 'telegram', + sessionId: params.parent.sessionId, + ...params.footerContext, + }), }); activity.reassert(); await recordFastAgentConversationMessageBestEffort({ diff --git a/packages/sdk/src/server/lib/fast-agent-reply-replacement.test.ts b/packages/sdk/src/server/lib/fast-agent-reply-replacement.test.ts new file mode 100644 index 000000000..1f231e97a --- /dev/null +++ b/packages/sdk/src/server/lib/fast-agent-reply-replacement.test.ts @@ -0,0 +1,149 @@ +const store = vi.hoisted(() => new Map()); +vi.mock('@roomote/redis', () => ({ + getRedis: () => ({ + get: async (key: string) => store.get(key) ?? null, + set: async (key: string, value: string, ...args: unknown[]) => { + if (args.includes('NX') && store.has(key)) return null; + store.set(key, value); + return 'OK'; + }, + zadd: async () => 1, + eval: async ( + script: string, + count: number, + key: string, + ownerOrPointerKey: string, + owner?: string, + value?: string, + ttl?: string | number, + ) => { + if (count === 2) { + if (store.get(key) !== owner) return 0; + if (ttl !== 'keepTtl' || store.has(ownerOrPointerKey)) { + store.set(ownerOrPointerKey, value!); + } + return 1; + } + if (store.get(key) !== ownerOrPointerKey) return 0; + if (script.includes("'del'")) store.delete(key); + return 1; + }, + }), +})); +vi.mock('./fast-agent-provider-message', () => ({ + recordFastAgentConversationMessageBestEffort: async () => {}, +})); +import { + getThreadReplyFooterRecord, + setThreadReplyFooterRecord, + type DiscordCommunicationProvider, +} from '@roomote/communication'; +import { + buildSlackThreadReplyFooterBlock, + type SlackNotifier, +} from '@roomote/slack'; +import { + createDiscordFastReplyReplacer, + createSlackFastReplyReplacer, +} from './fast-agent-reply-replacement'; + +describe('replacement writes obey footer lease ownership', () => { + beforeEach(() => store.clear()); + const footerContext = { linkedPrs: [], livePreviewUrl: null }; + it.each([false, true])( + 'Discord replacement respects lease loss=%s', + async (loseLease) => { + const original = { + messageId: 'old', + textWithoutFooter: 'Original', + refresh: { footerText: 'old footer', channelId: 'T' }, + }; + await setThreadReplyFooterRecord('discord', 'C', 'T', original); + const editMessage = vi.fn().mockResolvedValue(undefined); + editMessage.mockImplementationOnce(async () => { + if (!loseLease) return; + store.set('discord:thread_reply_footer_lock:C:T', 'competitor'); + await setThreadReplyFooterRecord('discord', 'C', 'T', { + ...original, + messageId: 'competitor', + textWithoutFooter: 'New body', + }); + }); + const replace = createDiscordFastReplyReplacer({ + provider: { editMessage } as unknown as DiscordCommunicationProvider, + conversation: { + surface: 'discord', + workspaceId: 'guild', + conversationId: 'C', + replyTarget: { channelId: 'C', threadId: 'T' }, + }, + channelId: 'C', + threadId: 'T', + sessionId: 'session', + footerContext, + postReplacement: vi.fn(), + }); + await replace( + { messageId: 'old' }, + { purpose: 'closeout', message: 'Updated old reply' }, + ); + expect( + (await getThreadReplyFooterRecord('discord', 'C', 'T'))?.messageId, + ).toBe(loseLease ? 'competitor' : 'old'); + if (loseLease) { + expect(editMessage).toHaveBeenLastCalledWith({ + channelId: 'T', + messageId: 'old', + text: 'Updated old reply', + preserveButtons: true, + }); + } else { + expect(editMessage).toHaveBeenCalledTimes(1); + expect( + (await getThreadReplyFooterRecord('discord', 'C', 'T')) + ?.textWithoutFooter, + ).toBe('Updated old reply'); + } + }, + ); + it('Slack removes only its stale replacement footer after a competing relocation', async () => { + store.set('slack:thread_reply_footer:C:T', 'old'); + const body = { type: 'markdown', text: 'Updated old reply' }; + const updateMessage = vi.fn().mockResolvedValue(true); + updateMessage.mockImplementationOnce(async () => { + store.set('slack:thread_reply_footer_lock:C:T', 'competitor'); + store.set('slack:thread_reply_footer:C:T', 'competitor'); + return true; + }); + const slack = { + updateMessage, + getMessageBlocks: vi.fn(async () => [ + body, + buildSlackThreadReplyFooterBlock({ footerText: 'footer' }), + ]), + } as unknown as SlackNotifier; + const replace = createSlackFastReplyReplacer({ + slack, + conversation: { + surface: 'slack', + workspaceId: 'team', + conversationId: 'T', + replyTarget: { channelId: 'C', threadId: 'T' }, + }, + channelId: 'C', + threadTs: 'T', + sessionId: 'session', + footerContext, + }); + await replace( + { messageId: 'old' }, + { purpose: 'closeout', message: 'Updated old reply' }, + ); + expect(store.get('slack:thread_reply_footer:C:T')).toBe('competitor'); + expect(updateMessage).toHaveBeenLastCalledWith({ + channel: 'C', + ts: 'old', + message: { blocks: [body] }, + }); + }); +}); diff --git a/packages/sdk/src/server/lib/fast-agent-reply-replacement.ts b/packages/sdk/src/server/lib/fast-agent-reply-replacement.ts index ae069189f..9e628108f 100644 --- a/packages/sdk/src/server/lib/fast-agent-reply-replacement.ts +++ b/packages/sdk/src/server/lib/fast-agent-reply-replacement.ts @@ -6,8 +6,9 @@ import type { import { buildFastSessionReplyFooterText, getThreadReplyFooterRecord, - setThreadReplyFooterRecord, + rememberThreadReplyFooterAfterEdit, withThreadReplyFooterLock, + replaceTextThreadReplyWithFooter, type FastSessionReplyFooterContext, } from '@roomote/communication'; import { @@ -20,6 +21,7 @@ import { buildSlackThreadReplyFooterBlock, getSlackThreadReplyFooterMessageTs, withSlackThreadReplyFooterLock, + removeSlackThreadReplyFooter, type SlackNotifier, } from '@roomote/slack'; @@ -50,12 +52,13 @@ export function createSlackFastReplyReplacer(params: { const updated = await withSlackThreadReplyFooterLock({ channel: params.channelId, threadTs: params.threadTs, - fn: async () => { + fn: async (assertLock) => { const footerMessageTs = await getSlackThreadReplyFooterMessageTs( params.channelId, params.threadTs, ).catch(() => null); - return params.slack.updateMessage({ + await assertLock(); + const updated = await params.slack.updateMessage({ channel: params.channelId, ts: handle.messageId, message: { @@ -76,6 +79,22 @@ export function createSlackFastReplyReplacer(params: { ], }, }); + try { + await assertLock(); + } catch { + const current = await getSlackThreadReplyFooterMessageTs( + params.channelId, + params.threadTs, + ).catch(() => undefined); + if (current !== undefined && current !== handle.messageId) + await removeSlackThreadReplyFooter({ + slack: params.slack, + channel: params.channelId, + threadTs: params.threadTs, + messageTs: handle.messageId, + }).catch(() => {}); + } + return updated; }, }); if (!updated) { @@ -114,7 +133,7 @@ export function createDiscordFastReplyReplacer(params: { // replacement would re-mark the old message as carrier. const replaced = await withThreadReplyFooterLock({ lockKey: `discord:thread_reply_footer_lock:${params.channelId}:${footerStateThreadId}`, - fn: async () => { + fn: async (assertLock, lock) => { const footerRecord = await getThreadReplyFooterRecord( 'discord', params.channelId, @@ -124,6 +143,7 @@ export function createDiscordFastReplyReplacer(params: { const replacementText = isFooterCarrier ? `${text}\n\n${footerText}` : text; + await assertLock(); if (replacementText.length > DISCORD_MAX_MESSAGE_LENGTH) { const placeholder = 'Reconnected to the inference provider.'; @@ -138,12 +158,26 @@ export function createDiscordFastReplyReplacer(params: { // The relocation that follows rewrites this message to its // stored footerless text; keep that text current so the edit // does not resurrect the pre-retry notice. - await setThreadReplyFooterRecord( - 'discord', - params.channelId, - footerStateThreadId, - { messageId, textWithoutFooter: placeholder }, - ).catch(() => {}); + await rememberThreadReplyFooterAfterEdit({ + provider: 'discord', + channelId: params.channelId, + threadId: footerStateThreadId, + assertLock, + lock, + record: { + ...footerRecord, + messageId, + textWithoutFooter: placeholder, + refresh: { footerText, channelId: editChannelId }, + }, + clearOwnFooter: () => + params.provider.editMessage({ + channelId: editChannelId, + messageId, + text: placeholder, + preserveButtons: true, + }), + }).catch(() => {}); } return false; } @@ -154,12 +188,26 @@ export function createDiscordFastReplyReplacer(params: { text: replacementText, }); if (isFooterCarrier) { - await setThreadReplyFooterRecord( - 'discord', - params.channelId, - footerStateThreadId, - { messageId, textWithoutFooter: text }, - ).catch(() => {}); + await rememberThreadReplyFooterAfterEdit({ + provider: 'discord', + channelId: params.channelId, + threadId: footerStateThreadId, + assertLock, + lock, + record: { + ...footerRecord, + messageId, + textWithoutFooter: text, + refresh: { footerText, channelId: editChannelId }, + }, + clearOwnFooter: () => + params.provider.editMessage({ + channelId: editChannelId, + messageId, + text, + preserveButtons: true, + }), + }).catch(() => {}); } return true; }, @@ -189,12 +237,16 @@ export function createTeamsFastReplyReplacer(params: { footerContext: FastSessionReplyFooterContext; }): FastAgentReplyReplacer { return async (handle, { message }) => { - await params.provider.updateMessage({ + await replaceTextThreadReplyWithFooter({ + provider: params.provider, channelId: params.channelId, + threadId: + 'replyTarget' in params.conversation + ? params.conversation.replyTarget.threadId + : undefined, messageId: handle.messageId, serviceUrl: params.serviceUrl, - text: `${message}\n\n${buildFastSessionReplyFooterText({ provider: 'teams', sessionId: params.sessionId, ...params.footerContext })}`, - textFormat: 'markdown', + text: message, }); await recordFastAgentConversationMessageBestEffort({ sessionId: params.sessionId, @@ -213,11 +265,15 @@ export function createTelegramFastReplyReplacer(params: { footerContext: FastSessionReplyFooterContext; }): FastAgentReplyReplacer { return async (handle, { message }) => { - await params.provider.editMessageText({ + await replaceTextThreadReplyWithFooter({ + provider: params.provider, channelId: params.channelId, + threadId: + 'replyTarget' in params.conversation + ? params.conversation.replyTarget.threadId + : undefined, messageId: handle.messageId, - text: `${message}\n\n${buildFastSessionReplyFooterText({ provider: 'telegram', sessionId: params.sessionId, ...params.footerContext })}`, - textFormat: 'markdown', + text: message, }); await recordFastAgentConversationMessageBestEffort({ sessionId: params.sessionId, diff --git a/packages/sdk/src/server/lib/fast-agent-surface-reply.test.ts b/packages/sdk/src/server/lib/fast-agent-surface-reply.test.ts index 5646c841f..ffaf0a400 100644 --- a/packages/sdk/src/server/lib/fast-agent-surface-reply.test.ts +++ b/packages/sdk/src/server/lib/fast-agent-surface-reply.test.ts @@ -41,7 +41,11 @@ vi.mock('@roomote/slack', () => ({ getSlackThreadReplyFooterMessageTs: vi.fn(async () => null), postSlackThreadMessageWithFooterText: mocks.slackPostThreadMessage, withSlackThreadReplyFooterLock: vi.fn( - async ({ fn }: { fn: () => Promise }) => fn(), + async ({ + fn, + }: { + fn: (assertLock: () => Promise) => Promise; + }) => fn(async () => {}), ), ROOMOTE_THREAD_REPLY_QUOTE_BLOCK_ID: 'quote', SlackNotifier: vi.fn(function () { @@ -152,6 +156,7 @@ describe('buildFastAgentSurfaceReplyDelivery', () => { messageId: 'teams-message-1', }); mocks.createTeamsProvider.mockResolvedValue({ + provider: 'teams', postMessage: mocks.teamsPostMessage, updateMessage: mocks.teamsUpdateMessage, }); @@ -162,6 +167,7 @@ describe('buildFastAgentSurfaceReplyDelivery', () => { lastTextMessageId: 'telegram-message-2', }); mocks.createTelegramProvider.mockResolvedValue({ + provider: 'telegram', postMessage: mocks.telegramPostMessage, editMessageText: mocks.telegramEditMessage, sendChatAction: mocks.telegramTyping, @@ -708,7 +714,7 @@ describe('buildFastAgentSurfaceReplyDelivery', () => { channelId, ...(threadId ? { threadId } : {}), ...(currentMessageId ? { replyToMessageId: currentMessageId } : {}), - text: expect.stringContaining('Reply or use the [web app]'), + text: expect.stringContaining('[Open in Roomote]'), }), ); expect(binding?.messageId).toBe( @@ -718,7 +724,7 @@ describe('buildFastAgentSurfaceReplyDelivery', () => { expect.objectContaining({ channelId, messageId: - surface === 'teams' ? 'teams-message-1' : 'telegram-message-1', + surface === 'teams' ? 'teams-message-1' : 'telegram-message-2', text: expect.stringContaining('Updated'), }), ); diff --git a/packages/sdk/src/server/lib/fast-agent-surface-reply.ts b/packages/sdk/src/server/lib/fast-agent-surface-reply.ts index b0ebeb891..222db55ee 100644 --- a/packages/sdk/src/server/lib/fast-agent-surface-reply.ts +++ b/packages/sdk/src/server/lib/fast-agent-surface-reply.ts @@ -21,6 +21,7 @@ import { deliverManagedThreadReplyFooter, getDiscordFooterlessFinalChunk, resolveFastSessionReplyFooterContext, + postTextThreadReplyWithFooter, } from '@roomote/communication'; import { createFastAgentSlackLiveTaskLauncher, @@ -437,6 +438,7 @@ export async function buildFastAgentSurfaceReplyDelivery(params: { textWithFooter, footerText, }), + refresh: { footerText, channelId: footerMessageChannelId }, }; }, clearPreviousFooter: async (previousFooterRecord) => { @@ -495,17 +497,25 @@ export async function buildFastAgentSurfaceReplyDelivery(params: { serviceUrl, }), postReply: async ({ message }) => { - const posted = await provider.postMessage({ - channelId: conversation.replyTarget.channelId, - serviceUrl, - ...(conversation.replyTarget.threadId - ? { - threadId: conversation.replyTarget.threadId, - replyToMessageId: conversation.replyTarget.threadId, - } - : {}), - text: `${message}\n\n${buildFastSessionReplyFooterText({ provider: 'teams', sessionId: session.id, ...footerContext })}`, - textFormat: 'markdown', + const posted = await postTextThreadReplyWithFooter({ + provider, + input: { + channelId: conversation.replyTarget.channelId, + serviceUrl, + ...(conversation.replyTarget.threadId + ? { + threadId: conversation.replyTarget.threadId, + replyToMessageId: conversation.replyTarget.threadId, + } + : {}), + text: message, + textFormat: 'markdown', + }, + footerText: buildFastSessionReplyFooterText({ + provider: 'teams', + sessionId: session.id, + ...footerContext, + }), }); await recordFastAgentConversationMessageBestEffort({ sessionId: session.id, @@ -606,14 +616,22 @@ export async function buildFastAgentSurfaceReplyDelivery(params: { conversation, }), postReply: async ({ message }) => { - const posted = await provider.postMessage({ - channelId: conversation.replyTarget.channelId, - ...(conversation.replyTarget.threadId - ? { threadId: conversation.replyTarget.threadId } - : {}), - ...(replyToMessageId ? { replyToMessageId } : {}), - text: `${message}\n\n${buildFastSessionReplyFooterText({ provider: 'telegram', sessionId: session.id, ...footerContext })}`, - textFormat: 'markdown', + const posted = await postTextThreadReplyWithFooter({ + provider, + input: { + channelId: conversation.replyTarget.channelId, + ...(conversation.replyTarget.threadId + ? { threadId: conversation.replyTarget.threadId } + : {}), + ...(replyToMessageId ? { replyToMessageId } : {}), + text: message, + textFormat: 'markdown', + }, + footerText: buildFastSessionReplyFooterText({ + provider: 'telegram', + sessionId: session.id, + ...footerContext, + }), }); activity.reassert(); await recordFastAgentConversationMessageBestEffort({ diff --git a/packages/sdk/src/server/lib/source-control-fast-delivery.test.ts b/packages/sdk/src/server/lib/source-control-fast-delivery.test.ts index e14d21cce..3590af27c 100644 --- a/packages/sdk/src/server/lib/source-control-fast-delivery.test.ts +++ b/packages/sdk/src/server/lib/source-control-fast-delivery.test.ts @@ -46,6 +46,14 @@ vi.mock('@roomote/redis', () => ({ vi.mock('@roomote/communication', () => ({ buildFastSessionReplyFooterText: ({ provider }: { provider: string }) => `[footer:${provider}]`, + resolveFastSessionReplyFooterContext: async () => ({}), + withThreadReplyFooterLock: async ({ + fn, + }: { + fn: (assertLock: () => Promise) => Promise; + }) => fn(async () => {}), + scheduleThreadFooterRefresh: async () => {}, + forgetThreadFooterRefresh: async () => {}, })); vi.mock('@roomote/github', () => ({ @@ -564,13 +572,18 @@ describe('GitHub Fast delivery', () => { ).resolves.toEqual({ messageId: '5003' }); await taskTurn.postReply({ message: 'Checks are green.' }); - // The stale comment was tried once, then this turn posted its own reply - // and kept editing that one. - expect(updateReviewComment).toHaveBeenCalledTimes(2); + // The stale comment was tried once, then this turn posted its own reply, + // tried to strip the footer the relocation displaced from the stale + // comment, and kept editing its own. + expect(updateReviewComment).toHaveBeenCalledTimes(3); expect(updateReviewComment).toHaveBeenNthCalledWith( 1, expect.objectContaining({ comment_id: 5002 }), ); + expect(updateReviewComment).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ comment_id: 5002, body: 'Rebasing now.' }), + ); expect(request).toHaveBeenCalledTimes(2); expect(request).toHaveBeenLastCalledWith( 'POST /repos/{owner}/{repo}/pulls/{pull_number}/comments/{comment_id}/replies', @@ -579,7 +592,7 @@ describe('GitHub Fast delivery', () => { }), ); expect(updateReviewComment).toHaveBeenNthCalledWith( - 2, + 3, expect.objectContaining({ comment_id: 5003, body: 'Rebased and pushed.\n\nChecks are green.\n\n[footer:github]', diff --git a/packages/sdk/src/server/lib/source-control-fast-delivery.ts b/packages/sdk/src/server/lib/source-control-fast-delivery.ts index 09a58f281..dbc28b6f4 100644 --- a/packages/sdk/src/server/lib/source-control-fast-delivery.ts +++ b/packages/sdk/src/server/lib/source-control-fast-delivery.ts @@ -5,7 +5,17 @@ import { type LaunchFastAgentTask, } from '@roomote/cloud-agents/server'; import { and, db, eq, repositories } from '@roomote/db/server'; -import { buildFastSessionReplyFooterText } from '@roomote/communication'; +import { + buildFastSessionReplyFooterText, + classifyThreadFooterActivity, + resolveFastSessionReplyFooterContext, + withThreadReplyFooterLock, + forgetThreadFooterRefresh, + scheduleThreadFooterRefresh, + type ThreadFooterRefreshOutcome, + type ThreadReplyFooterLock, +} from '@roomote/communication'; +import { tryThreadReplyFooterLock } from '@roomote/communication/thread-reply-footer-delivery'; import { ALL_REPOSITORIES, buildFastAgentChildTaskMetadata, @@ -20,6 +30,11 @@ import { import { getSourceControlThreadCommentRecord, setSourceControlThreadCommentRecord, + sourceControlFooterTarget, + getSourceControlFooterRecord, + setSourceControlFooterRecord, + clearSourceControlFooterRecord, + type SourceControlFooterRecord, } from './source-control-thread-comment-state'; export type SourceControlDiscussionKind = 'pull' | 'issues'; @@ -873,6 +888,97 @@ function isCommentGoneError(error: unknown): boolean { return status === 404 || status === 410; } +/** + * After a fenced pointer write loses its lease, put the carrier back the way + * its current owner recorded it. The competitor may have relocated the footer + * to another comment (then this comment keeps only its body) or rewritten + * this same comment with newer content (then that content comes back). + * + * The restoration runs under the destination lock (the caller's lease when it + * still holds it, otherwise a fresh one) and re-reads the record under it, so + * a further owner cannot persist newer content between the read and the edit. + * A provider edit can still outlive the lease, and only a lease still held + * after the edit proves nothing newer landed meanwhile. When that proof is + * missing, the recovery does not guess again: it marks the record's footer + * as unknown under a fenced write and schedules a refresh, so the next pass + * rewrites the comment from the record (the newest owner's body) with its own + * fenced post-edit write. The record, not a stale response, always wins. + */ +async function restoreCompetingCarrier(params: { + channelId: string; + threadId: string; + mine: { messageId: string; body: string; footerText: string }; + /** The caller's lease; used directly while it still holds the lock. */ + assertLock: () => Promise; + update: (body: string) => Promise; +}): Promise { + const lockKey = `source_control:thread_reply_footer_lock:${params.channelId}:${params.threadId}`; + const restore = async ( + assertLock: () => Promise, + ): Promise<'done' | 'unproven'> => { + const current = await getSourceControlFooterRecord( + params.channelId, + params.threadId, + ); + const desired = + !current || current.messageId !== params.mine.messageId + ? { body: params.mine.body, footerText: '' } + : { body: current.body, footerText: current.footerText }; + if ( + desired.body === params.mine.body && + desired.footerText === params.mine.footerText + ) + return 'done'; + await assertLock(); + await params.update( + desired.footerText + ? `${desired.body}\n\n${desired.footerText}` + : desired.body, + ); + try { + await assertLock(); + return 'done'; + } catch { + return 'unproven'; + } + }; + const reconcileLater = async () => { + await withThreadReplyFooterLock({ + lockKey, + fn: async (_assertLock, lock) => { + const current = await getSourceControlFooterRecord( + params.channelId, + params.threadId, + ); + if (!current) return; + await setSourceControlFooterRecord( + { ...current, footerText: '' }, + { keepTtl: true, lock }, + ); + }, + }); + await scheduleThreadFooterRefresh({ + provider: 'source-control', + channelId: params.channelId, + threadId: params.threadId, + }); + }; + try { + let heldLease = true; + await params.assertLock().catch(() => { + heldLease = false; + }); + const outcome = heldLease + ? await restore(params.assertLock) + : await withThreadReplyFooterLock({ lockKey, fn: restore }); + if (outcome === 'unproven') await reconcileLater(); + } catch (error) { + console.warn( + `[Fast Agent] Could not restore the comment a lost footer lease edited: ${formatErrorForLog(error)}`, + ); + } +} + /** * The reply surface a Session uses in a discussion: replies post as comments * with the Session footer, and tasks launch against the discussion's target. @@ -917,19 +1023,31 @@ export function buildSourceControlFastAdapter(params: { const discussion = parseSourceControlFastConversation(params.conversation); const threadId = params.conversation.replyTarget.threadId; const threaded = Boolean(discussion?.reviewCommentId && threadId); - const footer = discussion - ? buildFastSessionReplyFooterText({ - provider: discussion.provider, - sessionId: params.sessionId, - }) - : ''; + const target = sourceControlFooterTarget(params.conversation); + const lockKey = `source_control:thread_reply_footer_lock:${target.channelId}:${target.threadId}`; + let footer = ''; const quote = threaded ? null : params.quote; let turnComment: SourceControlPostedComment | null = null; let turnBody = ''; // True while the turn is editing a comment it adopted from the thread's // record rather than one it posted itself. let adoptedThreadComment = false; - const renderBody = () => `${turnBody}\n\n${footer}`; + const renderBody = async () => { + const current = await getSourceControlFooterRecord( + target.channelId, + target.threadId, + ); + if (turnComment && current && current.messageId !== turnComment.messageId) + return turnBody; + footer = buildFastSessionReplyFooterText({ + provider: params.conversation.surface, + sessionId: params.sessionId, + ...(await resolveFastSessionReplyFooterContext({ + sessionId: params.sessionId, + })), + }); + return `${turnBody}\n\n${footer}`; + }; const editorFor = (messageId: string): SourceControlPostedComment => ({ messageId, ...(params.delivery.updateCommentById && discussion @@ -943,20 +1061,94 @@ export function buildSourceControlFastAdapter(params: { } : {}), }); + // The footer moved to a newer comment: the old carrier must not keep a + // status that will never update again. Best effort, like the chat surfaces. + const stripPreviousFooter = async ( + previous: SourceControlFooterRecord, + assertLock: () => Promise, + ) => { + if (!params.delivery.updateCommentById || !discussion) return; + try { + await assertLock(); + await params.delivery.updateCommentById({ + discussion, + messageId: previous.messageId, + body: previous.body, + }); + } catch (error) { + if (isCommentGoneError(error)) return; + console.warn( + `[Fast Agent] Failed to remove the footer from the previous comment ${previous.messageId}: ${formatErrorForLog(error)}`, + ); + } + }; // The thread's comment record is a best-effort nicety: losing it costs one // extra comment, never the reply. - const rememberThreadComment = async () => { - if (!threaded || !threadId || !turnComment) { - return; - } - await setSourceControlThreadCommentRecord(params.sessionId, threadId, { - messageId: turnComment.messageId, - body: turnBody, - }).catch((error) => { + const rememberThreadComment = async ( + assertLock: () => Promise, + lock: ThreadReplyFooterLock | undefined, + relocate = false, + ) => { + const comment = turnComment; + if (!comment) return; + const body = turnBody; + const footerText = footer; + let current: SourceControlFooterRecord | null = null; + try { + await assertLock(); + current = await getSourceControlFooterRecord( + target.channelId, + target.threadId, + ); + await assertLock(); + if (!relocate && current && current.messageId !== comment.messageId) + return; + const written = await setSourceControlFooterRecord( + { + conversation: params.conversation, + sessionId: params.sessionId, + messageId: comment.messageId, + body, + footerText, + }, + { lock }, + ); + if (!written) throw new Error('Thread reply footer lock lease lost'); + if (relocate && current && current.messageId !== comment.messageId) + await stripPreviousFooter(current, assertLock); + if (!threaded || !threadId) return; + await assertLock(); + await setSourceControlThreadCommentRecord(params.sessionId, threadId, { + messageId: comment.messageId, + body, + }); + } catch (error) { + // A successful provider write is not permission to mutate a pointer + // after losing its lease. Even persistence-failure cleanup is fenced. + try { + await assertLock(); + if (current) + await clearSourceControlFooterRecord( + target.channelId, + target.threadId, + current, + ); + } catch { + /* The current pointer may belong to another delivery. */ + } + const update = comment.update ?? editorFor(comment.messageId).update; + if (update) + await restoreCompetingCarrier({ + channelId: target.channelId, + threadId: target.threadId, + mine: { messageId: comment.messageId, body, footerText }, + assertLock, + update, + }); console.warn( - `[Fast Agent] Failed to remember the review thread comment: ${formatErrorForLog(error)}`, + `[Fast Agent] Failed to remember the current comment footer: ${formatErrorForLog(error)}`, ); - }); + } }; const adoptThreadComment = async (): Promise => { if ( @@ -987,14 +1179,19 @@ export function buildSourceControlFastAdapter(params: { const postTurnComment = async ( discussion: SourceControlFastDiscussion, message: string, + assertLock: () => Promise, + lock: ThreadReplyFooterLock | undefined, ) => { turnBody = quote ? `${quote}\n\n${message}` : message; + turnComment = null; + const body = await renderBody(); + await assertLock(); turnComment = await params.delivery.postComment({ discussion, - body: renderBody(), + body, }); adoptedThreadComment = false; - await rememberThreadComment(); + await rememberThreadComment(assertLock, lock, true); params.onReplyPosted?.(); return { messageId: turnComment.messageId }; }; @@ -1007,57 +1204,210 @@ export function buildSourceControlFastAdapter(params: { : {}), resolveTarget: params.delivery.resolveTarget, }), - postReply: async ({ message }) => { - if (!discussion) { - throw new Error( - 'The discussion for this Session could not be resolved.', - ); - } - if (!turnComment) { - await adoptThreadComment(); - } - if (turnComment?.update) { - const previousBody = turnBody; - turnBody = `${turnBody}\n\n${message}`; - try { - await turnComment.update(renderBody()); - } catch (error) { - // The remembered comment can be gone (deleted by its author or a - // maintainer). Treat that as a miss on the thread record and post - // this turn's own comment, which replaces the stale record so - // later turns stop adopting it. Any other failure rethrows so the - // normal retry path keeps one comment per human turn. - if (!adoptedThreadComment || !isCommentGoneError(error)) { - throw error; + postReply: async ({ message }) => + withThreadReplyFooterLock({ + lockKey, + fn: async (assertLock, lock) => { + if (!discussion) { + throw new Error( + 'The discussion for this Session could not be resolved.', + ); } - console.warn( - `[Fast Agent] The remembered review thread comment could not be updated; posting a new comment: ${formatErrorForLog(error)}`, - ); - turnBody = previousBody; - turnComment = null; - return postTurnComment(discussion, message); - } - await rememberThreadComment(); - params.onReplyPosted?.(); - return { messageId: turnComment.messageId }; - } - return postTurnComment(discussion, message); - }, + if (!turnComment) { + await adoptThreadComment(); + } + if (turnComment?.update) { + // Another resumed turn may have appended to this same comment + // since this adapter last used it. The locked pointer owns the body. + const current = await getSourceControlFooterRecord( + target.channelId, + target.threadId, + ); + if (current?.messageId === turnComment.messageId) + turnBody = current.body; + const previousBody = turnBody; + turnBody = `${turnBody}\n\n${message}`; + try { + const body = await renderBody(); + await assertLock(); + await turnComment.update(body); + } catch (error) { + // The remembered comment can be gone (deleted by its author or a + // maintainer). Treat that as a miss on the thread record and post + // this turn's own comment, which replaces the stale record so + // later turns stop adopting it. Any other failure rethrows so the + // normal retry path keeps one comment per human turn. + if (!adoptedThreadComment || !isCommentGoneError(error)) { + turnBody = previousBody; + throw error; + } + console.warn( + `[Fast Agent] The remembered review thread comment could not be updated; posting a new comment: ${formatErrorForLog(error)}`, + ); + turnBody = previousBody; + turnComment = null; + return postTurnComment(discussion, message, assertLock, lock); + } + await rememberThreadComment(assertLock, lock); + params.onReplyPosted?.(); + return { messageId: turnComment.messageId }; + } + return postTurnComment(discussion, message, assertLock, lock); + }, + }), ...(params.delivery.updateCommentById && discussion ? { // A resumed turn only carries the prior comment's id: rebuild the // editor from it, replace the comment's body, and adopt it as the // turn's comment so later replies keep appending in place. - replaceReply: async ({ messageId }, { message }) => { - turnComment = editorFor(messageId); - adoptedThreadComment = false; - turnBody = message; - await turnComment.update!(renderBody()); - await rememberThreadComment(); - params.onReplyPosted?.(); - return { messageId }; - }, + replaceReply: async ({ messageId }, { message }) => + withThreadReplyFooterLock({ + lockKey, + fn: async (assertLock, lock) => { + turnComment = editorFor(messageId); + adoptedThreadComment = false; + turnBody = message; + const body = await renderBody(); + await assertLock(); + await turnComment.update!(body); + await rememberThreadComment(assertLock, lock); + params.onReplyPosted?.(); + return { messageId }; + }, + }), } : {}), }; } + +/** + * Unregister a destination, but only if its record still matches what this + * refresh pass read: a delivery that raced in owns the registration now. + */ +async function forgetSourceControlFooterIfUnchanged( + target: { channelId: string; threadId: string }, + seen: SourceControlFooterRecord | null, +): Promise { + const result = await tryThreadReplyFooterLock({ + lockKey: `source_control:thread_reply_footer_lock:${target.channelId}:${target.threadId}`, + fn: async (assertLock): Promise => { + const current = await getSourceControlFooterRecord( + target.channelId, + target.threadId, + ); + if ( + current?.messageId !== seen?.messageId || + current?.footerText !== seen?.footerText + ) + return 'active'; + await assertLock(); + await forgetThreadFooterRefresh({ + provider: 'source-control', + ...target, + }); + return 'gone'; + }, + }); + return result.acquired ? result.value : 'active'; +} + +/** + * Refresh only the recorded carrier; missing comments never trigger a post. + * Resolution happens outside the destination lock; the lock is held only for + * the comment edit and the pointer write, so a reply is never starved. + */ +export async function refreshSourceControlThreadFooter(target: { + channelId: string; + threadId: string; +}): Promise { + const record = await getSourceControlFooterRecord( + target.channelId, + target.threadId, + ); + if (!record) return forgetSourceControlFooterIfUnchanged(target, null); + const context = await resolveFastSessionReplyFooterContext({ + sessionId: record.sessionId, + }); + const footerText = buildFastSessionReplyFooterText({ + provider: record.conversation.surface, + sessionId: record.sessionId, + ...context, + }); + const { active, settled } = classifyThreadFooterActivity(context); + const outcome: ThreadFooterRefreshOutcome = active ? 'active' : 'idle'; + if (footerText === record.footerText) { + return settled + ? forgetSourceControlFooterIfUnchanged(target, record) + : outcome; + } + const delivery = await buildSourceControlFastDelivery(record.conversation); + const discussion = parseSourceControlFastConversation(record.conversation); + if (!delivery?.updateCommentById || !discussion) + return forgetSourceControlFooterIfUnchanged(target, record); + const result = await tryThreadReplyFooterLock({ + lockKey: `source_control:thread_reply_footer_lock:${target.channelId}:${target.threadId}`, + fn: async (assertLock, lock): Promise => { + const latest = await getSourceControlFooterRecord( + target.channelId, + target.threadId, + ); + // A reply relocated or rewrote the carrier while this pass was + // resolving; the next pass reads the new state. + if ( + !latest || + latest.messageId !== record.messageId || + latest.footerText !== record.footerText + ) + return 'active'; + await assertLock(); + try { + await delivery.updateCommentById!({ + discussion, + messageId: latest.messageId, + body: `${latest.body}\n\n${footerText}`, + }); + } catch (error) { + if (!isCommentGoneError(error)) throw error; + await assertLock(); + await forgetThreadFooterRefresh({ + provider: 'source-control', + ...target, + }); + return 'gone'; + } + // The pointer write is fenced on the lease in one Redis operation: a + // lease that lapsed during the edit cannot repoint refresh at this + // comment once a newer carrier exists. Then this edit's footer must go. + const written = await setSourceControlFooterRecord( + { ...latest, footerText }, + { keepTtl: true, lock }, + ); + if (!written) { + await restoreCompetingCarrier({ + channelId: target.channelId, + threadId: target.threadId, + mine: { messageId: latest.messageId, body: latest.body, footerText }, + assertLock, + update: (body) => + delivery.updateCommentById!({ + discussion, + messageId: latest.messageId, + body, + }), + }); + return 'active'; + } + if (settled) { + await assertLock(); + await forgetThreadFooterRefresh({ + provider: 'source-control', + ...target, + }); + return 'gone'; + } + return outcome; + }, + }); + // A delivery holds the lock: it re-registers the destination itself. + return result.acquired ? result.value : 'active'; +} diff --git a/packages/sdk/src/server/lib/source-control-footer-refresh.test.ts b/packages/sdk/src/server/lib/source-control-footer-refresh.test.ts new file mode 100644 index 000000000..ab7042c6a --- /dev/null +++ b/packages/sdk/src/server/lib/source-control-footer-refresh.test.ts @@ -0,0 +1,614 @@ +const mocks = vi.hoisted(() => ({ + store: new Map(), + context: vi.fn(), + update: vi.fn(), + create: vi.fn(), + schedule: vi.fn().mockResolvedValue(undefined), + forget: vi.fn(), + failRemember: false, + repositoryAvailable: true, +})); +vi.mock('@roomote/cloud-agents/server', () => ({ + createFastAgentTaskLauncher: () => vi.fn(), +})); +vi.mock('@roomote/redis', () => ({ + getRedis: () => ({ + get: async (key: string) => mocks.store.get(key) ?? null, + set: async (key: string, value: string, ...args: unknown[]) => { + if (mocks.failRemember && key.startsWith('source_control:footer:')) + throw new Error('pointer write failed'); + if ( + (args.includes('NX') && mocks.store.has(key)) || + (args.includes('XX') && !mocks.store.has(key)) + ) + return null; + mocks.store.set(key, value); + return 'OK'; + }, + del: async (key: string) => Number(mocks.store.delete(key)), + eval: async ( + script: string, + count: number, + key: string, + ...args: string[] + ) => { + const owner = args[count - 1]; + if (mocks.store.get(key) !== owner) return 0; + if (count === 2) { + const [pointerKey, , value, ttl] = args; + if ( + mocks.failRemember && + pointerKey!.startsWith('source_control:footer:') + ) + throw new Error('pointer write failed'); + if (ttl === 'keepTtl' && !mocks.store.has(pointerKey!)) return 0; + mocks.store.set(pointerKey!, value!); + return 1; + } + if (script.includes("'del'")) mocks.store.delete(key); + return 1; + }, + }), +})); +vi.mock('@roomote/db/server', () => ({ + and: vi.fn(), + eq: vi.fn(), + repositories: {}, + db: { + query: { + repositories: { + findMany: async () => + mocks.repositoryAvailable + ? [ + { + host: 'github.com', + githubInstallation: { id: 'installation' }, + }, + ] + : [], + }, + }, + }, +})); +vi.mock('@roomote/github', () => ({ + getInstallationOctokit: async () => ({ + rest: { + issues: { updateComment: mocks.update, createComment: mocks.create }, + pulls: { updateReviewComment: mocks.update }, + }, + }), +})); +vi.mock('@roomote/communication', async () => { + const { withThreadReplyFooterLock } = + await import('@roomote/communication/thread-reply-footer-delivery'); + return { + withThreadReplyFooterLock, + buildFastSessionReplyFooterText: ({ + runningTasks, + livePreviewUrl, + }: { + runningTasks?: { count: number }; + livePreviewUrl?: string; + }) => + `${runningTasks?.count ?? 0} running; preview=${livePreviewUrl ?? 'none'}`, + resolveFastSessionReplyFooterContext: mocks.context, + classifyThreadFooterActivity: (context: { + runningTasks?: { count: number } | null; + livePreviewUrl?: string | null; + sessionActivityAt?: number | null; + }) => { + const active = + (context.runningTasks?.count ?? 0) > 0 || + Boolean(context.livePreviewUrl); + return { + active, + settled: + !active && + Date.now() - (context.sessionActivityAt ?? 0) > 6 * 60 * 60_000, + }; + }, + scheduleThreadFooterRefresh: mocks.schedule, + forgetThreadFooterRefresh: mocks.forget, + }; +}); + +import { + buildSourceControlFastAdapter, + refreshSourceControlThreadFooter, +} from './source-control-fast-delivery'; +import { + getSourceControlFooterRecord, + sourceControlFooterTarget, + clearSourceControlFooterRecord, +} from './source-control-thread-comment-state'; + +const conversation = { + surface: 'github' as const, + workspaceId: 'github.com/o/r', + conversationId: 'pull/42', + replyTarget: { channelId: 'pull/42' }, +}; +const target = sourceControlFooterTarget(conversation); +const record = () => + getSourceControlFooterRecord(target.channelId, target.threadId); +const adapter = (id: string, post = vi.fn(async () => ({ messageId: id }))) => + buildSourceControlFastAdapter({ + conversation, + sessionId: 'session', + userId: 'user', + quote: '> Quoted message', + delivery: { + postComment: post, + updateCommentById: async ({ messageId, body }) => { + await mocks.update({ messageId, body }); + }, + resolveTarget: async () => ({}), + }, + }); + +describe('source-control current footer refresh', () => { + beforeEach(() => { + mocks.store.clear(); + mocks.failRemember = false; + mocks.repositoryAvailable = true; + vi.clearAllMocks(); + mocks.context.mockResolvedValue({ + runningTasks: { count: 0 }, + livePreviewUrl: 'https://preview', + }); + mocks.update.mockResolvedValue(undefined); + }); + + it('refreshes latest lifecycle counts without posting or losing quote/body/preview, and skips unchanged state', async () => { + const post = vi.fn(async () => ({ messageId: '123' })); + await adapter('123', post).postReply({ message: 'Body' }); + await refreshSourceControlThreadFooter(target); + expect(mocks.update).not.toHaveBeenCalled(); + for (const count of [1, 0, 2, 0, 1, 0]) { + mocks.context.mockResolvedValue({ + runningTasks: { count }, + livePreviewUrl: 'https://preview', + }); + await refreshSourceControlThreadFooter(target); + expect(mocks.update).toHaveBeenLastCalledWith({ + owner: 'o', + repo: 'r', + comment_id: 123, + body: `> Quoted message\n\nBody\n\n${count} running; preview=https://preview`, + }); + } + expect(post).toHaveBeenCalledTimes(1); + expect(mocks.create).not.toHaveBeenCalled(); + expect(mocks.context).toHaveBeenLastCalledWith({ sessionId: 'session' }); + expect((await record())?.body).toBe('> Quoted message\n\nBody'); + }); + + it('a post finishing after lease loss cannot set or clear a competitor pointer and removes only its own footer', async () => { + await adapter('111').postReply({ message: 'Original' }); + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const post = vi.fn(async () => { + mocks.store.delete( + `source_control:thread_reply_footer_lock:${target.channelId}:${target.threadId}`, + ); + await adapter('456').postReply({ message: 'Competitor' }); + return { messageId: '123' }; + }); + await expect( + adapter('123', post).postReply({ message: 'Orphan' }), + ).resolves.toEqual({ messageId: '123' }); + expect((await record())?.messageId).toBe('456'); + expect((await record())?.body).toContain('Competitor'); + // The competitor stripped the footer it displaced; the orphan stripped its own. + expect(mocks.update).toHaveBeenCalledTimes(2); + expect(mocks.update).toHaveBeenCalledWith({ + messageId: '111', + body: '> Quoted message\n\nOriginal', + }); + expect(mocks.update).toHaveBeenCalledWith({ + messageId: '123', + body: '> Quoted message\n\nOrphan', + }); + expect(mocks.schedule).toHaveBeenCalledTimes(2); + warning.mockRestore(); + }); + + it('a replacement finishing after lease loss cannot clear or overwrite a competitor pointer', async () => { + const first = adapter('123'); + await first.postReply({ message: 'Original' }); + mocks.update.mockImplementationOnce(async () => { + mocks.store.delete( + `source_control:thread_reply_footer_lock:${target.channelId}:${target.threadId}`, + ); + await adapter('456').postReply({ message: 'Competitor' }); + }); + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}); + await first.replaceReply!({ messageId: '123' }, { message: 'Updated A' }); + expect((await record())?.messageId).toBe('456'); + expect(mocks.update).toHaveBeenLastCalledWith({ + messageId: '123', + body: 'Updated A', + }); + expect(mocks.update).not.toHaveBeenCalledWith( + expect.objectContaining({ messageId: '456' }), + ); + warning.mockRestore(); + }); + + it('compare-and-delete cleanup cannot delete a newer record even on the same comment', async () => { + await adapter('123').postReply({ message: 'Original' }); + const expected = (await record())!; + const newer = { ...expected, body: 'Updated by another owner' }; + mocks.store.set( + `source_control:footer:${target.channelId}:${target.threadId}`, + JSON.stringify(newer), + ); + await clearSourceControlFooterRecord( + target.channelId, + target.threadId, + expected, + ); + expect(await record()).toEqual(newer); + }); + + it('a relocated footer is removed from the previous comment so it cannot go stale', async () => { + await adapter('123').postReply({ message: 'First turn' }); + await adapter('456').postReply({ message: 'Second turn' }); + expect(mocks.update).toHaveBeenCalledWith({ + messageId: '123', + body: '> Quoted message\n\nFirst turn', + }); + expect((await record())?.messageId).toBe('456'); + mocks.update.mockClear(); + mocks.context.mockResolvedValue({ runningTasks: { count: 3 } }); + await refreshSourceControlThreadFooter(target); + expect(mocks.update).toHaveBeenCalledTimes(1); + expect(mocks.update).toHaveBeenCalledWith( + expect.objectContaining({ comment_id: 456 }), + ); + }); + + it('reports idle Sessions and unregisters settled ones without editing', async () => { + await adapter('123').postReply({ message: 'Body' }); + mocks.update.mockClear(); + const idle = { runningTasks: { count: 0 }, sessionActivityAt: Date.now() }; + mocks.context.mockResolvedValue(idle); + await refreshSourceControlThreadFooter(target); + mocks.context.mockResolvedValue(idle); + expect(await refreshSourceControlThreadFooter(target)).toBe('idle'); + expect(mocks.forget).not.toHaveBeenCalled(); + mocks.context.mockResolvedValue({ ...idle, sessionActivityAt: 0 }); + expect(await refreshSourceControlThreadFooter(target)).toBe('gone'); + expect(mocks.forget).toHaveBeenCalledWith(target); + expect(mocks.update).toHaveBeenCalledTimes(1); // Only the idle edit itself. + }); + + it('forgets a carrier when the repository no longer has an available updater', async () => { + await adapter('123').postReply({ message: 'Original' }); + mocks.repositoryAvailable = false; + mocks.context.mockResolvedValue({ runningTasks: { count: 1 } }); + await refreshSourceControlThreadFooter(target); + expect(mocks.forget).toHaveBeenCalledWith(target); + expect(mocks.update).not.toHaveBeenCalled(); + expect(mocks.create).not.toHaveBeenCalled(); + }); + + it('missing carriers or deleted comments never post replacements', async () => { + await refreshSourceControlThreadFooter(target); + expect(mocks.context).not.toHaveBeenCalled(); + expect(mocks.forget).toHaveBeenCalled(); + await adapter('123').postReply({ message: 'Body' }); + mocks.context.mockResolvedValue({ runningTasks: { count: 1 } }); + mocks.update.mockRejectedValueOnce({ status: 404 }); + await refreshSourceControlThreadFooter(target); + expect(mocks.create).not.toHaveBeenCalled(); + expect((await record())?.footerText).toBe( + '0 running; preview=https://preview', + ); + }); + + it('refresh never edits a carrier a concurrent reply relocated; it follows only the latest carrier', async () => { + await adapter('123').postReply({ message: 'Old body' }); + let release!: () => void; + let started!: () => void; + const ready = new Promise((resolve) => { + started = resolve; + }); + const gate = new Promise((resolve) => { + release = resolve; + }); + mocks.context.mockImplementationOnce(async () => { + started(); + await gate; + return { runningTasks: { count: 1 } }; + }); + const refresh = refreshSourceControlThreadFooter(target); + await ready; + // Resolution holds no lock, so the reply is never delayed by it. + const post = vi.fn(async () => ({ messageId: '456' })); + await adapter('456', post).postReply({ message: 'New body' }); + expect(post).toHaveBeenCalledTimes(1); + release(); + expect(await refresh).toBe('active'); + expect((await record())?.messageId).toBe('456'); + expect(mocks.update).not.toHaveBeenCalledWith( + expect.objectContaining({ comment_id: 123 }), + ); + mocks.update.mockClear(); + mocks.context.mockResolvedValue({ runningTasks: { count: 2 } }); + await refreshSourceControlThreadFooter(target); + expect(mocks.update).toHaveBeenCalledWith( + expect.objectContaining({ + comment_id: 456, + body: expect.stringContaining('New body'), + }), + ); + expect(mocks.update).not.toHaveBeenCalledWith( + expect.objectContaining({ comment_id: 123 }), + ); + }); + + it('replacing an older reply cannot repoint refresh or restore its footer', async () => { + const old = adapter('123'); + await old.postReply({ message: 'Old' }); + await adapter('456').postReply({ message: 'Current' }); + await old.replaceReply!( + { messageId: '123' }, + { message: 'Historical edit' }, + ); + expect(mocks.update).toHaveBeenCalledWith({ + messageId: '123', + body: 'Historical edit', + }); + expect((await record())?.messageId).toBe('456'); + }); + + it('a long-lived adapter appends to the current body after a resumed turn edited the same carrier', async () => { + const reviewConversation = { + ...conversation, + replyTarget: { channelId: 'pull/42', threadId: 'review-1' }, + }; + const delivery = { + postComment: async () => ({ + messageId: '123', + update: async (body: string) => { + await mocks.update({ messageId: '123', body }); + }, + }), + updateCommentById: async ({ + messageId, + body, + }: { + messageId: string; + body: string; + }) => { + await mocks.update({ messageId, body }); + }, + resolveTarget: async () => ({}), + }; + const first = buildSourceControlFastAdapter({ + conversation: reviewConversation, + sessionId: 'session', + userId: 'user', + delivery, + }); + await first.postReply({ message: 'First' }); + const resumed = buildSourceControlFastAdapter({ + conversation: reviewConversation, + sessionId: 'session', + userId: 'user', + delivery, + continuesThreadComment: true, + }); + await resumed.postReply({ message: 'Resumed' }); + await first.postReply({ message: 'Later' }); + expect(mocks.update).toHaveBeenLastCalledWith({ + messageId: '123', + body: 'First\n\nResumed\n\nLater\n\n0 running; preview=https://preview', + }); + }); + + it('a refresh whose lease lapses during the edit cannot repoint refresh at its comment, and strips the footer it applied', async () => { + await adapter('123').postReply({ message: 'Old body' }); + mocks.context.mockResolvedValue({ runningTasks: { count: 1 } }); + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}); + mocks.update.mockImplementationOnce(async () => { + // The refresh's lease expires mid-edit and a newer reply takes over. + mocks.store.delete( + `source_control:thread_reply_footer_lock:${target.channelId}:${target.threadId}`, + ); + await adapter('456').postReply({ message: 'Newer body' }); + }); + expect(await refreshSourceControlThreadFooter(target)).toBe('active'); + expect((await record())?.messageId).toBe('456'); + // The refresh removes the footer it just applied through its own delivery. + expect(mocks.update).toHaveBeenLastCalledWith( + expect.objectContaining({ + comment_id: 123, + body: '> Quoted message\n\nOld body', + }), + ); + mocks.update.mockClear(); + mocks.context.mockResolvedValue({ runningTasks: { count: 2 } }); + await refreshSourceControlThreadFooter(target); + expect(mocks.update).toHaveBeenCalledTimes(1); + expect(mocks.update).toHaveBeenCalledWith( + expect.objectContaining({ comment_id: 456 }), + ); + warning.mockRestore(); + }); + + it('a refresh that loses its lease to a rewrite of the same comment restores the newer content', async () => { + const old = adapter('123'); + await old.postReply({ message: 'Old body' }); + mocks.context.mockResolvedValue({ runningTasks: { count: 1 } }); + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}); + mocks.update.mockImplementationOnce(async () => { + // The refresh's lease expires mid-edit; a resumed turn rewrites this + // same comment and records the newer body. + mocks.store.delete( + `source_control:thread_reply_footer_lock:${target.channelId}:${target.threadId}`, + ); + await old.replaceReply!({ messageId: '123' }, { message: 'Replaced' }); + }); + expect(await refreshSourceControlThreadFooter(target)).toBe('active'); + expect(await record()).toMatchObject({ + messageId: '123', + body: 'Replaced', + }); + expect(mocks.update).toHaveBeenLastCalledWith( + expect.objectContaining({ + comment_id: 123, + body: 'Replaced\n\n1 running; preview=none', + }), + ); + warning.mockRestore(); + }); + + it('a reply that loses its lease to a rewrite of the same comment restores the newer content', async () => { + const first = adapter('123'); + await first.postReply({ message: 'Original' }); + mocks.context.mockResolvedValue({ runningTasks: { count: 1 } }); + mocks.update.mockImplementationOnce(async () => { + mocks.store.delete( + `source_control:thread_reply_footer_lock:${target.channelId}:${target.threadId}`, + ); + await adapter('123').replaceReply!( + { messageId: '123' }, + { message: 'Updated B' }, + ); + }); + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}); + await first.replaceReply!({ messageId: '123' }, { message: 'Updated A' }); + expect(await record()).toMatchObject({ + messageId: '123', + body: 'Updated B', + }); + expect(mocks.update).toHaveBeenLastCalledWith({ + messageId: '123', + body: 'Updated B\n\n1 running; preview=none', + }); + warning.mockRestore(); + }); + + it('restores under the destination lock, so a later owner cannot be painted over', async () => { + const first = adapter('123'); + await first.postReply({ message: 'Original' }); + mocks.context.mockResolvedValue({ runningTasks: { count: 1 } }); + let releaseRestore!: () => void; + let restoreStarted!: () => void; + const restoring = new Promise((resolve) => { + restoreStarted = resolve; + }); + const gate = new Promise((resolve) => { + releaseRestore = resolve; + }); + mocks.update + .mockImplementationOnce(async () => { + mocks.store.delete( + `source_control:thread_reply_footer_lock:${target.channelId}:${target.threadId}`, + ); + await adapter('123').replaceReply!( + { messageId: '123' }, + { message: 'Updated B' }, + ); + }) + .mockImplementationOnce(async () => {}) // The competitor's own edit. + .mockImplementationOnce(async () => { + restoreStarted(); + await gate; + }); + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const recovery = first.replaceReply!( + { messageId: '123' }, + { message: 'Updated A' }, + ); + await restoring; + // A third owner arrives while the restoration edit is in flight: it must + // wait for the lock rather than have its content overwritten. + const later = adapter('123').replaceReply!( + { messageId: '123' }, + { message: 'Updated C' }, + ); + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(mocks.update).toHaveBeenCalledTimes(3); + releaseRestore(); + await recovery; + await later; + expect(mocks.update).toHaveBeenLastCalledWith({ + messageId: '123', + body: 'Updated C\n\n1 running; preview=none', + }); + expect(await record()).toMatchObject({ + messageId: '123', + body: 'Updated C', + }); + warning.mockRestore(); + }); + + it('hands an unproven restoration to the scheduled refresh instead of guessing again', async () => { + const first = adapter('123'); + await first.postReply({ message: 'Original' }); + mocks.context.mockResolvedValue({ runningTasks: { count: 1 } }); + const lockKey = `source_control:thread_reply_footer_lock:${target.channelId}:${target.threadId}`; + mocks.update + .mockImplementationOnce(async () => { + mocks.store.delete(lockKey); + await adapter('123').replaceReply!( + { messageId: '123' }, + { message: 'Updated B' }, + ); + }) + .mockImplementationOnce(async () => {}) // The competitor's own edit. + .mockImplementationOnce(async () => { + // The restoring edit outlives its lease; a third owner rewrites the + // comment and records it before the stale edit completes. + mocks.store.delete(lockKey); + await adapter('123').replaceReply!( + { messageId: '123' }, + { message: 'Updated C' }, + ); + }); + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}); + mocks.schedule.mockClear(); + await first.replaceReply!({ messageId: '123' }, { message: 'Updated A' }); + // No further guess: the record keeps C's body with its footer marked + // unknown, and a refresh is scheduled to rewrite the comment from it. + expect(mocks.update).toHaveBeenCalledTimes(4); + expect(await record()).toMatchObject({ + messageId: '123', + body: 'Updated C', + footerText: '', + }); + expect(mocks.schedule).toHaveBeenLastCalledWith(target); + expect(mocks.store.has(lockKey)).toBe(false); + mocks.update.mockClear(); + await refreshSourceControlThreadFooter(target); + expect(mocks.update).toHaveBeenCalledExactlyOnceWith( + expect.objectContaining({ + comment_id: 123, + body: 'Updated C\n\n1 running; preview=none', + }), + ); + expect((await record())?.footerText).toBe('1 running; preview=none'); + warning.mockRestore(); + }); + + it('does not fail an accepted reply or refresh an old body when pointer persistence fails', async () => { + await adapter('123').postReply({ message: 'Old body' }); + mocks.failRemember = true; + const post = vi.fn(async () => ({ messageId: '456' })); + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}); + await expect( + adapter('456', post).postReply({ message: 'New body' }), + ).resolves.toEqual({ messageId: '456' }); + expect(post).toHaveBeenCalledTimes(1); + expect(await record()).toBeNull(); + await refreshSourceControlThreadFooter(target); + expect(mocks.update).toHaveBeenCalledTimes(1); + expect(mocks.update).toHaveBeenCalledWith({ + messageId: '456', + body: '> Quoted message\n\nNew body', + }); + warning.mockRestore(); + }); +}); diff --git a/packages/sdk/src/server/lib/source-control-thread-comment-state.ts b/packages/sdk/src/server/lib/source-control-thread-comment-state.ts index 71e89ef00..19582117f 100644 --- a/packages/sdk/src/server/lib/source-control-thread-comment-state.ts +++ b/packages/sdk/src/server/lib/source-control-thread-comment-state.ts @@ -1,7 +1,118 @@ import { getRedis } from '@roomote/redis'; +import { + scheduleThreadFooterRefresh, + type ThreadReplyFooterLock, +} from '@roomote/communication'; +import type { FastAgentSourceControlConversation } from '@roomote/types'; const THREAD_COMMENT_TTL_SECONDS = 30 * 24 * 60 * 60; +export type SourceControlFooterRecord = { + conversation: FastAgentSourceControlConversation; + sessionId: string; + messageId: string; + body: string; + footerText: string; +}; + +export function sourceControlFooterTarget( + conversation: FastAgentSourceControlConversation, +) { + return { + provider: 'source-control' as const, + channelId: JSON.stringify([ + conversation.surface, + conversation.workspaceId, + conversation.conversationId, + ]), + threadId: conversation.replyTarget.threadId?.split(':')[0] ?? 'root', + }; +} + +export async function getSourceControlFooterRecord( + channelId: string, + threadId: string, +): Promise { + const raw = await getRedis().get( + `source_control:footer:${channelId}:${threadId}`, + ); + if (!raw) return null; + try { + const record = JSON.parse(raw) as SourceControlFooterRecord; + return typeof record.messageId === 'string' && + typeof record.body === 'string' && + typeof record.footerText === 'string' && + typeof record.sessionId === 'string' && + record.conversation + ? record + : null; + } catch { + return null; + } +} + +/** + * Returns false when the supplied lease no longer owns the destination lock, + * or when a `keepTtl` write found no record to update (it expired since it + * was read). Ownership and the write happen in one Redis operation, so a + * lease that lapses after `assertLock` cannot repoint a newer carrier. + */ +export async function setSourceControlFooterRecord( + record: SourceControlFooterRecord, + options: { keepTtl?: boolean; lock?: ThreadReplyFooterLock } = {}, +): Promise { + const target = sourceControlFooterTarget(record.conversation); + const key = `source_control:footer:${target.channelId}:${target.threadId}`; + const value = JSON.stringify(record); + const redis = getRedis(); + let written: boolean; + if (options.lock) { + written = + (await redis.eval( + `if redis.call('get', KEYS[1]) ~= ARGV[1] then return 0 end + if ARGV[3] == 'keepTtl' then + if not redis.call('set', KEYS[2], ARGV[2], 'KEEPTTL', 'XX') then return 0 end + else + redis.call('set', KEYS[2], ARGV[2], 'EX', ARGV[3]) + end + return 1`, + 2, + options.lock.key, + key, + options.lock.ownerId, + value, + options.keepTtl ? 'keepTtl' : THREAD_COMMENT_TTL_SECONDS, + )) === 1; + } else if (options.keepTtl) { + written = (await redis.set(key, value, 'KEEPTTL', 'XX')) === 'OK'; + } else { + await redis.set(key, value, 'EX', THREAD_COMMENT_TTL_SECONDS); + written = true; + } + if (written && !options.keepTtl) { + await scheduleThreadFooterRefresh(target).catch((error) => { + console.warn( + '[sourceControlFooter] Failed to schedule footer refresh', + error, + ); + }); + } + return written; +} + +export async function clearSourceControlFooterRecord( + channelId: string, + threadId: string, + expected: SourceControlFooterRecord, +): Promise { + await getRedis().eval( + "if redis.call('get',KEYS[1])==ARGV[1] then return redis.call('del',KEYS[1]) else return 0 end", + 1, + `source_control:footer:${channelId}:${threadId}`, + JSON.stringify(expected), + ); +} + /** * The comment a Session last opened inside a review thread, so later turns * that report on the same thread (a delegated task finishing, a pull request diff --git a/packages/sdk/src/server/lib/thread-footer-refresh.test.ts b/packages/sdk/src/server/lib/thread-footer-refresh.test.ts new file mode 100644 index 000000000..8352ac8d0 --- /dev/null +++ b/packages/sdk/src/server/lib/thread-footer-refresh.test.ts @@ -0,0 +1,202 @@ +const mocks = vi.hoisted(() => ({ + claim: vi.fn(), + managed: vi.fn(), + slackRefresh: vi.fn(), + sourceRefresh: vi.fn(), + discordEdit: vi.fn(), + textEdit: vi.fn(), + installation: vi.fn(), + redisGet: vi.fn(), + forget: vi.fn(), + reschedule: vi.fn(), + jobLock: vi.fn(), + jobRelease: vi.fn(), +})); +vi.mock('@roomote/communication', () => ({ + claimThreadFooterRefreshTargets: mocks.claim, + refreshManagedThreadReplyFooter: mocks.managed, + editTextThreadFooterMessage: mocks.textEdit, + forgetThreadFooterRefresh: mocks.forget, + rescheduleThreadFooterRefresh: mocks.reschedule, +})); +vi.mock('@roomote/db/server', () => ({ + db: { query: { slackInstallations: { findFirst: mocks.installation } } }, + and: vi.fn(), + eq: vi.fn(), + slackInstallations: {}, +})); +vi.mock('@roomote/redis', () => ({ + getRedis: () => ({ get: mocks.redisGet }), + acquireRedisLock: mocks.jobLock, +})); +vi.mock('@roomote/slack', () => ({ + SlackNotifier: class { + constructor(readonly token: string) {} + }, + refreshSlackThreadReplyFooter: mocks.slackRefresh, + withSlackThreadReplyFooterLock: async ({ + fn, + }: { + fn: (assertLock: () => Promise) => Promise; + }) => fn(async () => {}), +})); +vi.mock('./discord-communication', () => ({ + createDiscordCommunicationProviderFromRuntimeCredentials: async () => ({ + editMessage: mocks.discordEdit, + }), +})); +vi.mock('./teams-communication', () => ({ + createTeamsCommunicationProviderFromRuntimeCredentials: async () => ({ + provider: 'teams', + }), +})); +vi.mock('./telegram-communication', () => ({ + createTelegramCommunicationProviderFromRuntimeCredentials: async () => ({ + provider: 'telegram', + }), +})); +vi.mock('./source-control-fast-delivery', () => ({ + refreshSourceControlThreadFooter: mocks.sourceRefresh, +})); + +import { + refreshCurrentThreadFooters, + refreshThreadFooterTarget, +} from './thread-footer-refresh'; + +describe('footer refresh control-plane dispatch', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.claim.mockResolvedValue([]); + mocks.redisGet.mockResolvedValue('team'); + mocks.installation.mockResolvedValue({ botAccessToken: 'test-token' }); + mocks.managed.mockResolvedValue('active'); + mocks.slackRefresh.mockResolvedValue('active'); + mocks.sourceRefresh.mockResolvedValue('active'); + mocks.jobLock.mockResolvedValue(mocks.jobRelease); + }); + + it('runs one pass at a time and releases the pass lock when done', async () => { + mocks.jobLock.mockResolvedValueOnce(null); + await refreshCurrentThreadFooters(); + expect(mocks.claim).not.toHaveBeenCalled(); + mocks.claim.mockRejectedValueOnce(new Error('redis down')); + await expect(refreshCurrentThreadFooters()).rejects.toThrow('redis down'); + expect(mocks.jobRelease).toHaveBeenCalledTimes(1); + }); + + it('reschedules each destination by what the refresh learned and drops unregistered ones', async () => { + const targets = ['active', 'idle', 'gone'].map((outcome) => ({ + provider: 'discord' as const, + channelId: outcome, + threadId: 'T', + })); + mocks.claim.mockResolvedValue(targets); + mocks.managed.mockImplementation(async ({ channelId }) => channelId); + await refreshCurrentThreadFooters(); + expect(mocks.reschedule).toHaveBeenCalledTimes(2); + expect(mocks.reschedule).toHaveBeenCalledWith(targets[0], 'active'); + expect(mocks.reschedule).toHaveBeenCalledWith(targets[1], 'idle'); + }); + + it('retires a Slack footer whose workspace has no active installation', async () => { + mocks.installation.mockResolvedValue(null); + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect( + await refreshThreadFooterTarget({ + provider: 'slack', + channelId: 'C', + threadId: 'T', + }), + ).toBe('gone'); + expect(mocks.forget).toHaveBeenCalledWith({ + provider: 'slack', + channelId: 'C', + threadId: 'T', + }); + expect(mocks.slackRefresh).not.toHaveBeenCalled(); + warning.mockRestore(); + }); + + it('does no provider work when there are no recorded due carriers', async () => { + await refreshCurrentThreadFooters(); + expect(mocks.managed).not.toHaveBeenCalled(); + expect(mocks.slackRefresh).not.toHaveBeenCalled(); + expect(mocks.sourceRefresh).not.toHaveBeenCalled(); + }); + + it('routes source control and Slack only to recorded destinations and the recorded workspace credential', async () => { + await refreshThreadFooterTarget({ + provider: 'source-control', + channelId: 'discussion', + threadId: 'review', + }); + expect(mocks.sourceRefresh).toHaveBeenCalledWith({ + provider: 'source-control', + channelId: 'discussion', + threadId: 'review', + }); + await refreshThreadFooterTarget({ + provider: 'slack', + channelId: 'C', + threadId: 'T', + }); + expect(mocks.redisGet).toHaveBeenCalledWith( + 'slack:thread_footer_workspace:C:T', + ); + expect(mocks.slackRefresh).toHaveBeenCalledWith({ + slack: expect.objectContaining({ token: 'test-token' }), + channel: 'C', + threadTs: 'T', + }); + }); + + it('preserves Discord controls and uses the carrier destination rather than the parent channel', async () => { + mocks.managed.mockImplementation(async ({ edit }) => + edit( + { + messageId: 'current', + textWithoutFooter: 'Body', + refresh: { channelId: 'thread-channel', footerText: 'old' }, + }, + 'Body\n\nnew footer', + ), + ); + await refreshThreadFooterTarget({ + provider: 'discord', + channelId: 'parent-channel', + threadId: 'thread-channel', + }); + expect(mocks.discordEdit).toHaveBeenCalledWith({ + channelId: 'thread-channel', + messageId: 'current', + text: 'Body\n\nnew footer', + preserveButtons: true, + }); + }); + + it('bounds concurrent work to five destinations and isolates a failed target', async () => { + mocks.claim.mockResolvedValue( + Array.from({ length: 13 }, (_, index) => ({ + provider: 'discord', + channelId: `C${index}`, + threadId: 'T', + })), + ); + let active = 0; + let maximum = 0; + mocks.managed.mockImplementation(async ({ channelId }) => { + active += 1; + maximum = Math.max(maximum, active); + await new Promise((resolve) => setTimeout(resolve, 1)); + active -= 1; + if (channelId === 'C2') throw new Error('temporary failure'); + }); + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}); + await refreshCurrentThreadFooters(); + expect(mocks.managed).toHaveBeenCalledTimes(13); + expect(maximum).toBe(5); + expect(warning).toHaveBeenCalledTimes(1); + warning.mockRestore(); + }); +}); diff --git a/packages/sdk/src/server/lib/thread-footer-refresh.ts b/packages/sdk/src/server/lib/thread-footer-refresh.ts new file mode 100644 index 000000000..89b0cce8d --- /dev/null +++ b/packages/sdk/src/server/lib/thread-footer-refresh.ts @@ -0,0 +1,161 @@ +import { and, db, eq, slackInstallations } from '@roomote/db/server'; +import { acquireRedisLock, getRedis } from '@roomote/redis'; +import { + claimThreadFooterRefreshTargets, + refreshManagedThreadReplyFooter, + editTextThreadFooterMessage, + forgetThreadFooterRefresh, + rescheduleThreadFooterRefresh, + type ThreadFooterRefreshOutcome, + type ThreadFooterRefreshTarget, +} from '@roomote/communication'; +import { + SlackNotifier, + refreshSlackThreadReplyFooter, + withSlackThreadReplyFooterLock, +} from '@roomote/slack'; + +import { createDiscordCommunicationProviderFromRuntimeCredentials } from './discord-communication'; +import { createTeamsCommunicationProviderFromRuntimeCredentials } from './teams-communication'; +import { createTelegramCommunicationProviderFromRuntimeCredentials } from './telegram-communication'; +import { refreshSourceControlThreadFooter } from './source-control-fast-delivery'; + +const JOB_LOCK_KEY = 'thread_footer_refresh:job'; +/** One batch never overlaps the next scheduler tick; the lock outlives the deadline. */ +const JOB_LOCK_TTL_SECONDS = 5 * 60; +const JOB_DEADLINE_MS = 4 * 60_000; +const JOB_CONCURRENCY = 5; + +/** Unregister a Slack destination whose workspace can no longer be reached. */ +async function forgetSlackTarget( + target: ThreadFooterRefreshTarget, +): Promise { + await withSlackThreadReplyFooterLock({ + channel: target.channelId, + threadTs: target.threadId, + maxAcquireAttempts: 1, + fn: async (assertLock) => { + await assertLock(); + await forgetThreadFooterRefresh(target); + }, + }); + return 'gone'; +} + +export async function refreshThreadFooterTarget( + target: ThreadFooterRefreshTarget, +): Promise { + if (target.provider === 'source-control') { + return refreshSourceControlThreadFooter(target); + } + if (target.provider === 'slack') { + const workspaceKey = `slack:thread_footer_workspace:${target.channelId}:${target.threadId}`; + const teamId = await getRedis().get(workspaceKey); + if (!teamId) { + let outcome: ThreadFooterRefreshOutcome = 'active'; + await withSlackThreadReplyFooterLock({ + channel: target.channelId, + threadTs: target.threadId, + maxAcquireAttempts: 1, + fn: async (assertLock) => { + if (!(await getRedis().get(workspaceKey))) { + await assertLock(); + await forgetThreadFooterRefresh(target); + outcome = 'gone'; + } + }, + }); + return outcome; + } + const installation = await db.query.slackInstallations.findFirst({ + where: and( + eq(slackInstallations.isActive, true), + eq(slackInstallations.teamId, teamId), + ), + columns: { botAccessToken: true }, + }); + if (!installation?.botAccessToken) { + // The workspace uninstalled or reinstalled the app: this carrier can + // never be edited again, and a new reply registers under the new team. + console.warn( + '[threadFooterRefresh] Retiring a footer with no active Slack installation', + { channelId: target.channelId, threadId: target.threadId }, + ); + return forgetSlackTarget(target); + } + return refreshSlackThreadReplyFooter({ + slack: new SlackNotifier(installation.botAccessToken), + channel: target.channelId, + threadTs: target.threadId, + }); + } + return refreshManagedThreadReplyFooter({ + ...target, + provider: target.provider, + edit: async (record, text) => { + if (target.provider === 'discord') { + const provider = + await createDiscordCommunicationProviderFromRuntimeCredentials(); + if (!provider) + throw new Error('Discord footer refresh credentials unavailable'); + await provider.editMessage({ + channelId: record.refresh!.channelId, + messageId: record.messageId, + text, + preserveButtons: true, + }); + } else { + const provider = + target.provider === 'teams' + ? await createTeamsCommunicationProviderFromRuntimeCredentials() + : await createTelegramCommunicationProviderFromRuntimeCredentials(); + if (!provider) + throw new Error('Text footer refresh credentials unavailable'); + await editTextThreadFooterMessage(provider, record, text); + } + }, + }); +} + +/** + * One bounded pass over due destinations. No history scans, task-status + * event coupling, or provider calls from workers. Only one pass runs at a + * time: a slow pass makes the next tick skip rather than double the work. + */ +export async function refreshCurrentThreadFooters(): Promise { + const release = await acquireRedisLock(JOB_LOCK_KEY, { + ttlSeconds: JOB_LOCK_TTL_SECONDS, + }); + if (!release) return; + const deadline = Date.now() + JOB_DEADLINE_MS; + try { + const targets = await claimThreadFooterRefreshTargets(); + // Limit parallel provider/DB work and isolate one destination's failures. + for (let index = 0; index < targets.length; index += JOB_CONCURRENCY) { + if (Date.now() > deadline) { + // Unfinished targets keep their claim lease and come back with it. + console.warn('[threadFooterRefresh] Pass hit its deadline', { + remaining: targets.length - index, + }); + break; + } + await Promise.all( + targets.slice(index, index + JOB_CONCURRENCY).map(async (target) => { + try { + const outcome = await refreshThreadFooterTarget(target); + if (outcome !== 'gone') + await rescheduleThreadFooterRefresh(target, outcome); + } catch (error) { + // The claim lease retries this destination on the slow cadence. + console.warn('[threadFooterRefresh] Refresh deferred', { + provider: target.provider, + error, + }); + } + }), + ); + } + } finally { + await release(); + } +} diff --git a/packages/sdk/src/server/routers/task-runs.test.ts b/packages/sdk/src/server/routers/task-runs.test.ts index f086ab528..dfd364a68 100644 --- a/packages/sdk/src/server/routers/task-runs.test.ts +++ b/packages/sdk/src/server/routers/task-runs.test.ts @@ -443,35 +443,41 @@ describe('taskRunsRouter queue message guards', () => { expect(mockFindTaskRun).not.toHaveBeenCalled(); }); - it('resolves the footer text for the run thread', async () => { - mockFindTaskRun.mockResolvedValue({ - id: 42, - taskId: 'task-1', - prRepo: null, - prNumber: null, - }); - - await expect( - createRunCaller().getSlackThreadFooterText({ - runId: 42, - slackChannelId: 'C123', - threadTs: '1710000000.123', + it.each([0, 1, 2])( + 'uses the complete shared footer for the run thread with %i running tasks', + async (count) => { + mockFindTaskRun.mockResolvedValue({ + id: 42, + taskId: 'task-1', + prRepo: null, + prNumber: null, + }); + const navigationUrl = + count === 1 + ? 'https://app.example.com/sessions/owner?task=task-1' + : 'https://app.example.com/tasks'; + const footer = `<${navigationUrl}|${count === 0 ? 'No running tasks' : `${count} running task${count === 1 ? '' : 's'}`}> · · `; + mockGetSlackThreadFooterText.mockResolvedValue(footer); + + await expect( + createRunCaller().getSlackThreadFooterText({ + runId: 42, + slackChannelId: 'C123', + threadTs: '1710000000.123', + taskUrl: 'http://localhost:3000/task/task-1', + }), + ).resolves.toBe(footer); + + expect(mockGetSlackThreadFooterText).toHaveBeenCalledWith({ taskUrl: 'http://localhost:3000/task/task-1', - }), - ).resolves.toBe( - '_Reply or use the ._', - ); - - expect(mockGetSlackThreadFooterText).toHaveBeenCalledWith({ - taskUrl: 'http://localhost:3000/task/task-1', - taskId: 'task-1', - prRepo: null, - prNumber: null, - linkedPrs: [], - channelId: 'C123', - threadTs: '1710000000.123', - }); - }); + taskId: 'task-1', + prRepo: null, + prNumber: null, + channelId: 'C123', + threadTs: '1710000000.123', + }); + }, + ); it('rejects recordMessageEnvelope for auth-token callers', async () => { await expect( diff --git a/packages/sdk/src/server/routers/task-runs.ts b/packages/sdk/src/server/routers/task-runs.ts index 4c6da2392..ebc28099b 100644 --- a/packages/sdk/src/server/routers/task-runs.ts +++ b/packages/sdk/src/server/routers/task-runs.ts @@ -1,15 +1,12 @@ import { TRPCError } from '@trpc/server'; import { z } from 'zod'; import { - and, claimTaskGoalContinuationForRun, db, eq, getTaskGoalForRun, - isNotNull, releaseTaskGoalContinuationForRun, slackInstallations, - taskPullRequests, } from '@roomote/db/server'; import { @@ -47,7 +44,7 @@ import { clearActiveSlackRunReplyTarget, clearPendingSlackRequestUserInput, getActiveSlackRunReplyTarget, - getSlackThreadFooterText as buildSlackThreadFooterText, + getSlackThreadFooterText, getSlackStartedMessageData, getSlackMessages, getSlackRequestUserInputAnswers, @@ -592,38 +589,11 @@ export const taskRunsRouter = router({ }); } - // PR linkage lives on task_pull_requests; include every active GitHub PR - // so the existing footer can point users to the full task split. - const linkedPrs = await db.query.taskPullRequests.findMany({ - where: and( - eq(taskPullRequests.taskId, taskRun.taskId), - eq(taskPullRequests.sourceControlProvider, 'github'), - isNotNull(taskPullRequests.repository), - isNotNull(taskPullRequests.prNumber), - ), - orderBy: (row, { asc }) => [asc(row.detectedAt), asc(row.createdAt)], - columns: { - repository: true, - prNumber: true, - prUrl: true, - status: true, - }, - }); - - const activeLinkedPrs = linkedPrs.filter( - (pr) => pr.status !== 'closed' && pr.status !== 'merged', - ); - - return buildSlackThreadFooterText({ + return getSlackThreadFooterText({ taskUrl: input.taskUrl, taskId: taskRun.taskId, - prRepo: activeLinkedPrs[0]?.repository ?? null, - prNumber: activeLinkedPrs[0]?.prNumber ?? null, - linkedPrs: activeLinkedPrs.flatMap((pr) => - pr.prNumber !== null && pr.prUrl - ? [{ prNumber: pr.prNumber, prUrl: pr.prUrl }] - : [], - ), + prRepo: null, + prNumber: null, channelId: input.slackChannelId, threadTs: input.threadTs, }); diff --git a/packages/slack/src/__tests__/request-user-input-blocks.test.ts b/packages/slack/src/__tests__/request-user-input-blocks.test.ts index 1c53808eb..e4669a16e 100644 --- a/packages/slack/src/__tests__/request-user-input-blocks.test.ts +++ b/packages/slack/src/__tests__/request-user-input-blocks.test.ts @@ -9,8 +9,7 @@ describe('buildSlackRequestUserInputBlocks', () => { const blocks = buildSlackRequestUserInputBlocks({ requestId: 'rui:session:turn:call', currentQuestionIndex: 1, - footerText: - '_Reply with @-mention or use the ._', + footerText: '', answers: { stack: { answers: ['Blessed'], @@ -78,7 +77,7 @@ describe('buildSlackRequestUserInputBlocks', () => { type: 'context', elements: [ expect.objectContaining({ - text: '_Reply with @-mention or use the ._', + text: '', }), ], }), diff --git a/packages/slack/src/__tests__/thread-footer-refresh-read.test.ts b/packages/slack/src/__tests__/thread-footer-refresh-read.test.ts new file mode 100644 index 000000000..7a9ae3ee3 --- /dev/null +++ b/packages/slack/src/__tests__/thread-footer-refresh-read.test.ts @@ -0,0 +1,62 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +const fetch = vi.hoisted(() => vi.fn()); +vi.mock('../slack-api-fetch', async (importOriginal) => ({ + ...(await importOriginal()), + slackFetch: fetch, +})); +import { SlackNotifier } from '../slack-notifier'; + +describe('refresh carrier lookup distinguishes missing from unavailable', () => { + afterEach(() => { + vi.restoreAllMocks(); + fetch.mockReset(); + }); + const input = { + channel: 'C', + threadTs: 'T', + messageTs: 'M', + throwOnUnavailable: true, + }; + it.each(['thread_not_found', 'message_not_found', 'channel_not_found'])( + 'returns a missing result for %s', + async (error) => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + fetch.mockResolvedValue( + new Response(JSON.stringify({ ok: false, error })), + ); + expect( + await new SlackNotifier('test-token').getMessageBlocks(input), + ).toBeNull(); + }, + ); + it('returns missing only when a successful response lacks the requested message', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + fetch.mockResolvedValue( + new Response( + JSON.stringify({ ok: true, messages: [{ ts: 'T', blocks: [] }] }), + ), + ); + expect( + await new SlackNotifier('test-token').getMessageBlocks(input), + ).toBeNull(); + }); + it.each([429, 500])( + 'throws HTTP %s so refresh retries rather than forgetting a live carrier', + async (status) => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + fetch.mockResolvedValue(new Response('', { status })); + await expect( + new SlackNotifier('test-token').getMessageBlocks(input), + ).rejects.toThrow('unavailable'); + }, + ); + it('throws transient Slack API errors rather than returning a missing result', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + fetch.mockResolvedValue( + new Response(JSON.stringify({ ok: false, error: 'ratelimited' })), + ); + await expect( + new SlackNotifier('test-token').getMessageBlocks(input), + ).rejects.toThrow('ratelimited'); + }); +}); diff --git a/packages/slack/src/__tests__/thread-footer-refresh.test.ts b/packages/slack/src/__tests__/thread-footer-refresh.test.ts new file mode 100644 index 000000000..61f5e55cf --- /dev/null +++ b/packages/slack/src/__tests__/thread-footer-refresh.test.ts @@ -0,0 +1,370 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + store: new Map(), + current: 'old', + resolve: vi.fn(), + activity: { active: true, settled: false }, + forget: vi.fn(), + schedule: vi.fn(), +})); +vi.mock('@roomote/redis', () => ({ + getRedis: () => ({ + get: async (key: string) => mocks.store.get(key) ?? null, + set: async (key: string, value: string, ...args: unknown[]) => { + if (args.includes('NX') && mocks.store.has(key)) return null; + mocks.store.set(key, value); + return 'OK'; + }, + eval: async ( + _script: string, + _count: number, + key: string, + owner: string, + ) => { + if (mocks.store.get(key) !== owner) return 0; + mocks.store.delete(key); + return 1; + }, + }), +})); +vi.mock('@roomote/communication', () => ({ + resolveCurrentThreadFooterText: mocks.resolve, + resolveCurrentThreadFooter: async (provider: string, footerText: string) => { + const text = await mocks.resolve(provider, footerText); + return text === null ? null : { text, ...mocks.activity }; + }, + forgetThreadFooterRefresh: mocks.forget, + scheduleThreadFooterRefresh: mocks.schedule, +})); +vi.mock('../thread-footer', () => ({ + buildSlackThreadFooterText: vi.fn(), + resolveSlackThreadFooterContext: vi.fn(), +})); +vi.mock('../slack-messages', () => ({ + getSlackThreadReplyFooterMessageTs: async () => mocks.current || null, + setSlackThreadReplyFooterMessageTs: async ( + _channel: string, + _thread: string, + ts: string, + ) => { + mocks.current = ts; + }, +})); + +import { + buildSlackThreadReplyFooterBlock, + postSlackThreadMessageWithFooterText, + refreshSlackThreadReplyFooter, + updateSlackThreadMessageWithFooterText, +} from '../thread-reply-footer-ops'; + +describe('Slack lifecycle footer refresh', () => { + beforeEach(() => { + mocks.store.clear(); + mocks.current = 'old'; + vi.clearAllMocks(); + mocks.activity.active = true; + mocks.activity.settled = false; + mocks.resolve.mockResolvedValue('running'); + }); + const body = { type: 'markdown', text: 'Narrative' }; + const image = { + type: 'image', + image_url: 'https://image', + alt_text: 'proof', + }; + const slack = () => ({ + getMessageBlocks: vi.fn( + async (): Promise => [ + body, + image, + buildSlackThreadReplyFooterBlock({ footerText: 'idle' }), + ], + ), + updateMessage: vi.fn(async () => true), + postMessage: vi.fn(async () => 'new'), + }); + + it('edits just the current footer block, preserving body, images and fallback text', async () => { + const provider = slack(); + await refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }); + expect(provider.getMessageBlocks).toHaveBeenCalledWith({ + channel: 'C', + threadTs: 'T', + messageTs: 'old', + throwOnUnavailable: true, + }); + expect(provider.updateMessage).toHaveBeenCalledWith({ + channel: 'C', + ts: 'old', + message: { + blocks: [ + body, + image, + buildSlackThreadReplyFooterBlock({ footerText: 'running' }), + ], + }, + }); + expect(provider.postMessage).not.toHaveBeenCalled(); + }); + + it.each(['post', 'update'])( + 'a late %s preserves a competing pointer and strips only its own footer', + async (operation) => { + const provider = slack(); + const competitor = slack(); + const takeOver = async () => { + mocks.store.delete('slack:thread_reply_footer_lock:C:T'); + await postSlackThreadMessageWithFooterText({ + slack: competitor, + channel: 'C', + threadTs: 'T', + text: 'B', + bodyBlocks: [body], + footerText: 'running', + }); + }; + const warning = vi.spyOn(console, 'error').mockImplementation(() => {}); + const params = { + slack: provider, + channel: 'C', + threadTs: 'T', + text: 'A', + bodyBlocks: [body, image], + footerText: 'running', + }; + if (operation === 'post') { + provider.postMessage.mockImplementationOnce(async () => { + await takeOver(); + return 'orphan'; + }); + await postSlackThreadMessageWithFooterText(params); + } else { + provider.updateMessage.mockImplementationOnce(async () => { + await takeOver(); + return true; + }); + await updateSlackThreadMessageWithFooterText({ + ...params, + messageTs: 'orphan', + }); + } + expect(mocks.current).toBe('new'); + expect(provider.updateMessage).toHaveBeenLastCalledWith({ + channel: 'C', + ts: 'orphan', + message: { blocks: [body, image] }, + }); + expect(provider.updateMessage).not.toHaveBeenCalledWith( + expect.objectContaining({ ts: 'old' }), + ); + expect(provider.updateMessage).not.toHaveBeenCalledWith( + expect.objectContaining({ ts: 'new' }), + ); + warning.mockRestore(); + }, + ); + + it('forgets a missing provider message without posting or looking through history', async () => { + const provider = slack(); + provider.getMessageBlocks.mockResolvedValueOnce(null); + await refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }); + expect(mocks.forget).toHaveBeenCalledWith({ + provider: 'slack', + channelId: 'C', + threadId: 'T', + }); + expect(provider.getMessageBlocks).toHaveBeenCalledTimes(1); + expect(provider.updateMessage).not.toHaveBeenCalled(); + expect(provider.postMessage).not.toHaveBeenCalled(); + }); + + it('does not forget a competitor that relocated the footer during the missing-message lookup', async () => { + const provider = slack(); + provider.getMessageBlocks.mockImplementationOnce(async () => { + mocks.store.set('slack:thread_reply_footer_lock:C:T', 'competitor'); + mocks.current = 'new'; + return null; + }); + expect( + await refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }), + ).toBe('active'); + expect(mocks.forget).not.toHaveBeenCalled(); + mocks.store.delete('slack:thread_reply_footer_lock:C:T'); + provider.getMessageBlocks.mockImplementationOnce(async () => { + mocks.current = 'newer'; + return null; + }); + expect( + await refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }), + ).toBe('active'); + expect(mocks.forget).not.toHaveBeenCalled(); + }); + + it('compares against the footer as written, not as Slack escapes it on read-back', async () => { + const provider = slack(); + const written = + ''; + provider.getMessageBlocks.mockResolvedValue([ + body, + buildSlackThreadReplyFooterBlock({ + footerText: written.replaceAll('&', '&'), + }), + ]); + mocks.resolve.mockResolvedValue(written); + expect( + await refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }), + ).toBe('active'); + expect(mocks.resolve).toHaveBeenCalledWith('slack', written); + expect(provider.updateMessage).not.toHaveBeenCalled(); + }); + + it('reports idle threads, and unregisters settled or unresolvable ones without editing', async () => { + const provider = slack(); + mocks.activity.active = false; + mocks.resolve.mockResolvedValue('idle'); + expect( + await refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }), + ).toBe('idle'); + expect(mocks.forget).not.toHaveBeenCalled(); + mocks.activity.settled = true; + expect( + await refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }), + ).toBe('gone'); + expect(mocks.forget).toHaveBeenCalledTimes(1); + mocks.forget.mockClear(); + mocks.activity.settled = false; + mocks.resolve.mockResolvedValue(null); + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect( + await refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }), + ).toBe('gone'); + expect(mocks.forget).toHaveBeenCalledTimes(1); + expect(provider.updateMessage).not.toHaveBeenCalled(); + warning.mockRestore(); + }); + + it('yields to a reply holding the thread instead of waiting for it', async () => { + const provider = slack(); + mocks.store.set('slack:thread_reply_footer_lock:C:T', 'a-reply'); + const startedAt = Date.now(); + expect( + await refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }), + ).toBe('active'); + expect(Date.now() - startedAt).toBeLessThan(500); + expect(provider.updateMessage).not.toHaveBeenCalled(); + }); + + it('skips unchanged, missing and already footerless messages without history lookup or posting', async () => { + const provider = slack(); + mocks.current = ''; + await refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }); + expect(provider.getMessageBlocks).not.toHaveBeenCalled(); + expect(mocks.forget).toHaveBeenCalled(); + mocks.current = 'old'; + mocks.resolve.mockResolvedValue('idle'); + await refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }); + provider.getMessageBlocks.mockResolvedValueOnce([body, image]); + await refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }); + expect(provider.updateMessage).not.toHaveBeenCalled(); + expect(provider.postMessage).not.toHaveBeenCalled(); + }); + + it('never edits a carrier that a reply relocated while it was resolving; the next tick reads the new pointer', async () => { + const provider = slack(); + let release!: () => void; + let started!: () => void; + const ready = new Promise((resolve) => { + started = resolve; + }); + const gate = new Promise((resolve) => { + release = resolve; + }); + mocks.resolve.mockImplementationOnce(async () => { + started(); + await gate; + return 'running'; + }); + const refresh = refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }); + await ready; + // Resolution holds no lock, so the reply is never delayed by it. + await postSlackThreadMessageWithFooterText({ + slack: provider, + channel: 'C', + threadTs: 'T', + text: 'New body', + bodyBlocks: [body], + footerText: 'running', + }); + expect(provider.postMessage).toHaveBeenCalledTimes(1); + provider.updateMessage.mockClear(); // The reply's own footer strip of 'old'. + release(); + expect(await refresh).toBe('active'); + expect(mocks.current).toBe('new'); + expect(provider.updateMessage).not.toHaveBeenCalled(); + await refreshSlackThreadReplyFooter({ + slack: provider, + channel: 'C', + threadTs: 'T', + }); + expect(provider.updateMessage).toHaveBeenCalledWith( + expect.objectContaining({ ts: 'new' }), + ); + expect(provider.updateMessage).not.toHaveBeenCalledWith( + expect.objectContaining({ ts: 'old' }), + ); + }); +}); diff --git a/packages/slack/src/__tests__/thread-footer.test.ts b/packages/slack/src/__tests__/thread-footer.test.ts index 87c5ff306..58de58339 100644 --- a/packages/slack/src/__tests__/thread-footer.test.ts +++ b/packages/slack/src/__tests__/thread-footer.test.ts @@ -1,17 +1,17 @@ +import { RunStatus } from '@roomote/types'; + const { findFirstMock, findManyMock, taskRunFindFirstMock, environmentFindFirstMock, resolveEffectivePreviewRuntimeConfigMock, - redisGetMock, } = vi.hoisted(() => ({ findFirstMock: vi.fn(), findManyMock: vi.fn(), taskRunFindFirstMock: vi.fn(), environmentFindFirstMock: vi.fn(), resolveEffectivePreviewRuntimeConfigMock: vi.fn(), - redisGetMock: vi.fn(), })); vi.mock('@roomote/db/server', () => ({ @@ -30,6 +30,7 @@ vi.mock('@roomote/db/server', () => ({ }, }, eq: vi.fn((...args: unknown[]) => ({ eq: args })), + getSessionForTask: vi.fn().mockResolvedValue(null), taskPullRequests: { taskId: 'taskId', }, @@ -50,13 +51,10 @@ vi.mock('@roomote/env', () => ({ }, })); -vi.mock('@roomote/redis', () => ({ - getRedis: vi.fn(() => ({ - get: redisGetMock, - })), -})); - -import { getSlackThreadFooterText } from '../thread-footer'; +import { + buildSlackThreadFooterText, + getSlackThreadFooterText, +} from '../thread-footer'; function mockEnvironmentBackedTaskRun(params?: { primaryPortName?: string | null; @@ -64,10 +62,26 @@ function mockEnvironmentBackedTaskRun(params?: { taskRunFindFirstMock.mockResolvedValue({ payload: { environmentId: 'env-1' }, primaryPortName: params?.primaryPortName ?? null, + status: RunStatus.Idle, }); } describe('getSlackThreadFooterText', () => { + it('renders the owning Session transcript separately from task navigation', () => { + expect( + buildSlackThreadFooterText({ + taskUrl: 'https://app.example.com/task/task-1?utm_source=slack', + webAppUrl: 'https://app.example.com/sessions/owner', + runningTasks: { + count: 1, + url: 'https://app.example.com/sessions/owner?task=task-1', + }, + }), + ).toBe( + ' · ', + ); + }); + beforeEach(() => { vi.clearAllMocks(); findFirstMock.mockResolvedValue(null); @@ -79,16 +93,14 @@ describe('getSlackThreadFooterText', () => { previewProxyBaseUrl: 'https://preview.example.com', }, }); - redisGetMock.mockResolvedValue(null); }); - it('prefers the linked task PR and uses the explicit-mention marker', async () => { + it('prefers the linked task PR', async () => { findFirstMock.mockResolvedValue({ prUrl: 'https://github.com/roomote/app/pull/4321', prNumber: 4321, status: 'open', }); - redisGetMock.mockResolvedValue('1'); await expect( getSlackThreadFooterText({ @@ -100,7 +112,7 @@ describe('getSlackThreadFooterText', () => { threadTs: '111.000', }), ).resolves.toBe( - '_Working on , reply with @-mention or use the ._', + ' · ', ); }); @@ -120,9 +132,7 @@ describe('getSlackThreadFooterText', () => { channelId: 'C123', threadTs: '111.000', }), - ).resolves.toBe( - '_Reply or use the ._', - ); + ).resolves.toBe(''); }); it('falls back to the task run PR when no linked task PR row exists', async () => { @@ -136,7 +146,7 @@ describe('getSlackThreadFooterText', () => { threadTs: '111.000', }), ).resolves.toBe( - '_Working on , reply or use the ._', + ' · ', ); }); @@ -158,7 +168,7 @@ describe('getSlackThreadFooterText', () => { threadTs: '111.000', }), ).resolves.toBe( - '_Working on , , reply or use the ._', + ' · · ', ); }); @@ -180,7 +190,7 @@ describe('getSlackThreadFooterText', () => { threadTs: '111.000', }), ).resolves.toBe( - '_Working on a , reply or use the ._', + ' · ', ); }); @@ -210,7 +220,7 @@ describe('getSlackThreadFooterText', () => { threadTs: '111.000', }), ).resolves.toBe( - '_Working on a , reply or use the ._', + ' · ', ); }); @@ -232,7 +242,7 @@ describe('getSlackThreadFooterText', () => { threadTs: '111.000', }), ).resolves.toBe( - '_Working on a , reply or use the ._', + ' · ', ); }); @@ -255,7 +265,7 @@ describe('getSlackThreadFooterText', () => { threadTs: '111.000', }), ).resolves.toBe( - '_Working on a , reply or use the ._', + ' · ', ); }); @@ -274,9 +284,7 @@ describe('getSlackThreadFooterText', () => { channelId: 'C123', threadTs: '111.000', }), - ).resolves.toBe( - '_Reply or use the ._', - ); + ).resolves.toBe(''); }); it('omits the live preview link for repo-only tasks without an environment', async () => { @@ -295,7 +303,7 @@ describe('getSlackThreadFooterText', () => { threadTs: '111.000', }), ).resolves.toBe( - '_Working on , reply or use the ._', + ' · ', ); expect(environmentFindFirstMock).not.toHaveBeenCalled(); @@ -323,31 +331,6 @@ describe('getSlackThreadFooterText', () => { channelId: 'C123', threadTs: '111.000', }), - ).resolves.toBe( - '_Reply or use the ._', - ); - }); - - it('keeps the explicit-mention instruction with the live preview link', async () => { - mockEnvironmentBackedTaskRun({ primaryPortName: 'WEB' }); - environmentFindFirstMock.mockResolvedValue({ - config: { - ports: [{ name: 'WEB', port: 3000 }], - }, - }); - redisGetMock.mockResolvedValue('1'); - - await expect( - getSlackThreadFooterText({ - taskUrl: 'https://app.example.com/task/task-1', - taskId: 'task-1', - prRepo: null, - prNumber: null, - channelId: 'C123', - threadTs: '111.000', - }), - ).resolves.toBe( - '_Working on a , reply with @-mention or use the ._', - ); + ).resolves.toBe(''); }); }); diff --git a/packages/slack/src/__tests__/thread-reply-footer-ops.test.ts b/packages/slack/src/__tests__/thread-reply-footer-ops.test.ts index ca2d2b779..36518a279 100644 --- a/packages/slack/src/__tests__/thread-reply-footer-ops.test.ts +++ b/packages/slack/src/__tests__/thread-reply-footer-ops.test.ts @@ -22,6 +22,10 @@ vi.mock('@roomote/env', () => ({ vi.mock('@roomote/redis', () => ({ getRedis: () => ({ + get: async (key: string) => + [...mockRedisSet.mock.calls] + .reverse() + .find((call) => call[0] === key)?.[1] ?? null, set: mockRedisSet, eval: mockRedisEval, }), @@ -54,7 +58,6 @@ describe('thread-reply-footer-ops', () => { mockResolveFooterContext.mockResolvedValue({ linkedPrs: [{ prNumber: 7, prUrl: 'https://github.com/o/r/pull/7' }], livePreviewUrl: null, - explicitMentionRequired: false, }); mockBuildFooterText.mockReturnValue( '_Working on , reply or use the ._', @@ -211,6 +214,34 @@ describe('thread-reply-footer-ops', () => { expect(mockSetFooterTs).toHaveBeenCalledWith('C1', '100.000', '333.000'); }); + it('preserves awake preview, zero status and Session navigation in reply-only posts', async () => { + const context = { + linkedPrs: [{ prNumber: 7, prUrl: 'https://github.com/o/r/pull/7' }], + livePreviewUrl: 'https://preview.example.com', + runningTasks: { count: 0, url: 'https://app.example.com/tasks' }, + webAppUrl: 'https://app.example.com/sessions/owner', + }; + mockResolveFooterContext.mockResolvedValue(context); + mockGetFooterTs.mockResolvedValue(null); + await postSlackThreadMessageWithStickyFooter({ + slack: { + postMessage: vi.fn().mockResolvedValue('222.000'), + getMessageBlocks: vi.fn(), + updateMessage: vi.fn(), + }, + channel: 'C1', + threadTs: '100.000', + taskId: 'task-1', + text: 'PR merged', + footerStyle: 'reply-only', + }); + expect(mockBuildFooterText).toHaveBeenCalledWith({ + ...context, + taskUrl: expect.stringContaining('/task/task-1?'), + linkedPrs: [], + }); + }); + it('takes the footer back off a rewritten message when the pointer cannot be saved', async () => { mockGetFooterTs.mockResolvedValue(null); mockSetFooterTs.mockRejectedValue(new Error('redis down')); diff --git a/packages/slack/src/markdown-converter.ts b/packages/slack/src/markdown-converter.ts index 92960862d..986bc7f81 100644 --- a/packages/slack/src/markdown-converter.ts +++ b/packages/slack/src/markdown-converter.ts @@ -147,7 +147,8 @@ export function convertMarkdownLinksToSlack(text: string): string { ); } -function decodeSlackEntity(text: string): string { +/** Reverse Slack's `&`/`<`/`>` escaping on text read back from the API. */ +export function decodeSlackEntity(text: string): string { // Decode lt/gt before amp so nested entities are not double-unescaped. return text .replaceAll('<', '<') diff --git a/packages/slack/src/slack-notifier.ts b/packages/slack/src/slack-notifier.ts index 29c424a82..c01124af3 100644 --- a/packages/slack/src/slack-notifier.ts +++ b/packages/slack/src/slack-notifier.ts @@ -81,6 +81,7 @@ type SlackAuthTestResponse = { error?: string; user_id?: string; bot_id?: string; + team_id?: string; }; type SlackUsersListResponse = { @@ -233,6 +234,7 @@ export class SlackNotifier { private ownBotIdentityPromise?: Promise<{ userId?: string; botId?: string; + teamId?: string; } | null>; constructor( @@ -350,6 +352,7 @@ export class SlackNotifier { private async getOwnBotIdentity(): Promise<{ userId?: string; botId?: string; + teamId?: string; } | null> { if (!this.ownBotIdentityPromise) { this.ownBotIdentityPromise = (async () => { @@ -385,6 +388,7 @@ export class SlackNotifier { return { userId: result.user_id, botId: result.bot_id, + teamId: result.team_id, }; } catch (error) { console.error( @@ -404,6 +408,11 @@ export class SlackNotifier { return ownBotIdentity; } + /** Non-secret routing identity for durable, control-plane message refresh. */ + async getWorkspaceId(): Promise { + return (await this.getOwnBotIdentity())?.teamId ?? null; + } + private async normalizeFetchedMessages( messages: SlackApiThreadMessage[], options: { @@ -1379,10 +1388,13 @@ export class SlackNotifier { channel, messageTs, threadTs, + throwOnUnavailable = false, }: { channel: string; messageTs: string; threadTs: string; + /** Distinguish an unavailable API from a confirmed missing carrier. */ + throwOnUnavailable?: boolean; }): Promise { try { const response = await slackFetch( @@ -1397,6 +1409,10 @@ export class SlackNotifier { ); if (!response.ok) { + if (throwOnUnavailable) + throw new Error( + `Slack message lookup unavailable (${response.status})`, + ); console.error( `[fetchMessageBlocks] Slack API failed: ${response.status} ${response.statusText}`, ); @@ -1421,6 +1437,18 @@ export class SlackNotifier { }; if (!result.ok || !result.messages) { + if ( + throwOnUnavailable && + ![ + 'message_not_found', + 'thread_not_found', + 'channel_not_found', + ].includes(result.error ?? '') + ) { + throw new Error( + `Slack message lookup unavailable: ${result.error ?? 'missing response data'}`, + ); + } console.error( `[fetchMessageBlocks] Slack error: ${result.error || 'No messages returned'}`, ); @@ -1443,6 +1471,7 @@ export class SlackNotifier { console.error( `[fetchMessageBlocks] Failed: ${error instanceof Error ? error.message : String(error)}`, ); + if (throwOnUnavailable) throw error; return null; } } diff --git a/packages/slack/src/thread-footer.ts b/packages/slack/src/thread-footer.ts index 78538d262..cd296ca7f 100644 --- a/packages/slack/src/thread-footer.ts +++ b/packages/slack/src/thread-footer.ts @@ -6,15 +6,12 @@ import { resolveThreadReplyLivePreviewUrl, type ThreadReplyFooterContext, type ThreadReplyLinkedPr, + type ThreadReplyRunningTasks, } from '@roomote/communication'; -import { isSlackThreadExplicitMentionRequired } from './slack-messages'; - export type SlackThreadLinkedPr = ThreadReplyLinkedPr; -export interface SlackThreadFooterContext extends ThreadReplyFooterContext { - explicitMentionRequired: boolean; -} +export type SlackThreadFooterContext = ThreadReplyFooterContext; export { buildThreadReplyPrUrl as buildSlackThreadReplyPrUrl, @@ -29,28 +26,22 @@ export async function resolveSlackThreadFooterContext(params: { channelId: string; threadTs: string; }): Promise { - const [context, explicitMentionRequired] = await Promise.all([ - resolveThreadReplyFooterContext(params), - isSlackThreadExplicitMentionRequired(params.channelId, params.threadTs), - ]); - - return { - ...context, - explicitMentionRequired, - }; + return resolveThreadReplyFooterContext(params); } export function buildSlackThreadFooterText(params: { taskUrl: string; linkedPrs?: SlackThreadLinkedPr[]; livePreviewUrl?: string | null; - explicitMentionRequired: boolean; + runningTasks?: ThreadReplyRunningTasks | null; + webAppUrl?: string | null; }): string { return buildThreadReplyFooterText({ taskUrl: params.taskUrl, linkedPrs: params.linkedPrs, livePreviewUrl: params.livePreviewUrl, - explicitMentionRequired: params.explicitMentionRequired, + runningTasks: params.runningTasks, + webAppUrl: params.webAppUrl, formatLink: (label, url) => `<${url}|${label}>`, }); } @@ -70,6 +61,7 @@ export async function getSlackThreadFooterText(params: { taskUrl: params.taskUrl, linkedPrs: params.linkedPrs ?? context.linkedPrs, livePreviewUrl: context.livePreviewUrl, - explicitMentionRequired: context.explicitMentionRequired, + runningTasks: context.runningTasks, + webAppUrl: context.webAppUrl, }); } diff --git a/packages/slack/src/thread-reply-footer-ops.ts b/packages/slack/src/thread-reply-footer-ops.ts index 785a2743d..235f9798d 100644 --- a/packages/slack/src/thread-reply-footer-ops.ts +++ b/packages/slack/src/thread-reply-footer-ops.ts @@ -1,8 +1,17 @@ -import crypto from 'node:crypto'; - import { Env } from '@roomote/env'; import { getRedis } from '@roomote/redis'; +import { + tryThreadReplyFooterLock, + withThreadReplyFooterLock, +} from '@roomote/communication/thread-reply-footer-delivery'; +import { + scheduleThreadFooterRefresh, + forgetThreadFooterRefresh, + resolveCurrentThreadFooter, + type ThreadFooterRefreshOutcome, +} from '@roomote/communication'; +import { decodeSlackEntity } from './markdown-converter'; import type { SlackNotifier } from './slack-notifier'; import { getSlackThreadReplyFooterMessageTs, @@ -16,11 +25,6 @@ import { export const SLACK_THREAD_REPLY_FOOTER_BLOCK_ID = 'roomote_thread_reply_footer'; const SLACK_THREAD_REPLY_FOOTER_LOCK_PREFIX = 'slack:thread_reply_footer_lock:'; -const THREAD_REPLY_FOOTER_LOCK_TTL_SECONDS = 30; -const THREAD_REPLY_FOOTER_LOCK_MAX_ATTEMPTS = 8; -const THREAD_REPLY_FOOTER_LOCK_RETRY_MS = 100; -const RELEASE_LOCK_SCRIPT = - "if redis.call('get',KEYS[1])==ARGV[1] then return redis.call('del',KEYS[1]) else return 0 end"; export const THREAD_REPLY_FOOTER_LOCK_TIMEOUT_MESSAGE = 'Timed out acquiring thread reply footer lock'; @@ -96,39 +100,13 @@ export async function withSlackThreadReplyFooterLock(params: { channel: string; threadTs: string; maxAcquireAttempts?: number; - fn: () => Promise; + fn: (assertLock: () => Promise) => Promise; }): Promise { - const redis = getRedis(); - const lockKey = `${SLACK_THREAD_REPLY_FOOTER_LOCK_PREFIX}${params.channel}:${params.threadTs}`; - const maxAcquireAttempts = - params.maxAcquireAttempts ?? THREAD_REPLY_FOOTER_LOCK_MAX_ATTEMPTS; - - for (let attempt = 0; attempt < maxAcquireAttempts; attempt += 1) { - const ownerId = crypto.randomUUID(); - const acquired = await redis.set( - lockKey, - ownerId, - 'EX', - THREAD_REPLY_FOOTER_LOCK_TTL_SECONDS, - 'NX', - ); - - if (acquired) { - try { - return await params.fn(); - } finally { - await redis - .eval(RELEASE_LOCK_SCRIPT, 1, lockKey, ownerId) - .catch(() => {}); - } - } - - await new Promise((resolve) => - setTimeout(resolve, THREAD_REPLY_FOOTER_LOCK_RETRY_MS), - ); - } - - throw new Error(THREAD_REPLY_FOOTER_LOCK_TIMEOUT_MESSAGE); + return withThreadReplyFooterLock({ + lockKey: `${SLACK_THREAD_REPLY_FOOTER_LOCK_PREFIX}${params.channel}:${params.threadTs}`, + maxAcquireAttempts: params.maxAcquireAttempts, + fn: params.fn, + }); } export async function removeSlackThreadReplyFooter(params: { @@ -136,6 +114,7 @@ export async function removeSlackThreadReplyFooter(params: { channel: string; threadTs: string; messageTs: string; + assertLock?: () => Promise; }): Promise { const blocks = await params.slack.getMessageBlocks({ channel: params.channel, @@ -155,6 +134,7 @@ export async function removeSlackThreadReplyFooter(params: { return; } + await params.assertLock?.(); const updated = await params.slack.updateMessage({ channel: params.channel, ts: params.messageTs, @@ -185,7 +165,8 @@ export async function postSlackThreadMessageWithFooterText(params: { slack: Pick< SlackNotifier, 'postMessage' | 'getMessageBlocks' | 'updateMessage' - >; + > & + Partial>; channel: string; threadTs: string; text: string; @@ -202,12 +183,13 @@ export async function postSlackThreadMessageWithFooterText(params: { return withSlackThreadReplyFooterLock({ channel: params.channel, threadTs: params.threadTs, - fn: async () => { + fn: async (assertLock) => { const previousFooterMessageTs = await getSlackThreadReplyFooterMessageTs( params.channel, params.threadTs, ); + await assertLock(); const nextMessageTs = await params.slack.postMessage({ channel: params.channel, thread_ts: params.threadTs, @@ -222,38 +204,26 @@ export async function postSlackThreadMessageWithFooterText(params: { return null; } - if ( - previousFooterMessageTs && - previousFooterMessageTs !== nextMessageTs - ) { - try { - await removeSlackThreadReplyFooter({ - slack: params.slack, - channel: params.channel, - threadTs: params.threadTs, - messageTs: previousFooterMessageTs, - }); - } catch (error) { - console.error( - `[slackThreadFooter] Failed to remove footer from prior Slack message ${previousFooterMessageTs}: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - } - } - try { + await assertLock(); await setSlackThreadReplyFooterMessageTs( params.channel, params.threadTs, nextMessageTs, ); + await rememberSlackThreadFooterRefresh(params, assertLock); } catch (error) { console.error( `[slackThreadFooter] Failed to persist latest footer message ts ${nextMessageTs}: ${ error instanceof Error ? error.message : String(error) }`, ); + const current = await getSlackThreadReplyFooterMessageTs( + params.channel, + params.threadTs, + ).catch(() => undefined); + if (current === undefined || current === nextMessageTs) + return nextMessageTs; try { await removeSlackThreadReplyFooter({ slack: params.slack, @@ -270,6 +240,27 @@ export async function postSlackThreadMessageWithFooterText(params: { }`, ); } + return nextMessageTs; + } + + if ( + previousFooterMessageTs && + previousFooterMessageTs !== nextMessageTs + ) { + try { + await removeSlackThreadReplyFooter({ + slack: params.slack, + channel: params.channel, + threadTs: params.threadTs, + messageTs: previousFooterMessageTs, + assertLock, + }); + } catch (error) { + console.error( + '[slackThreadFooter] Failed to remove prior footer', + error, + ); + } } return nextMessageTs; @@ -283,7 +274,8 @@ export async function postSlackThreadMessageWithFooterText(params: { * freshly posted reply would be. */ export async function updateSlackThreadMessageWithFooterText(params: { - slack: Pick; + slack: Pick & + Partial>; channel: string; threadTs: string; messageTs: string; @@ -299,11 +291,12 @@ export async function updateSlackThreadMessageWithFooterText(params: { return withSlackThreadReplyFooterLock({ channel: params.channel, threadTs: params.threadTs, - fn: async () => { + fn: async (assertLock) => { const previousFooterMessageTs = await getSlackThreadReplyFooterMessageTs( params.channel, params.threadTs, ); + await assertLock(); const updated = await params.slack.updateMessage({ channel: params.channel, ts: params.messageTs, @@ -316,38 +309,25 @@ export async function updateSlackThreadMessageWithFooterText(params: { return false; } - if ( - previousFooterMessageTs && - previousFooterMessageTs !== params.messageTs - ) { - try { - await removeSlackThreadReplyFooter({ - slack: params.slack, - channel: params.channel, - threadTs: params.threadTs, - messageTs: previousFooterMessageTs, - }); - } catch (error) { - console.error( - `[slackThreadFooter] Failed to remove footer from prior Slack message ${previousFooterMessageTs}: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - } - } - try { + await assertLock(); await setSlackThreadReplyFooterMessageTs( params.channel, params.threadTs, params.messageTs, ); + await rememberSlackThreadFooterRefresh(params, assertLock); } catch (error) { console.error( `[slackThreadFooter] Failed to persist latest footer message ts ${params.messageTs}: ${ error instanceof Error ? error.message : String(error) }`, ); + const current = await getSlackThreadReplyFooterMessageTs( + params.channel, + params.threadTs, + ).catch(() => undefined); + if (current === undefined || current === params.messageTs) return true; // Without the pointer no later reply could strip this footer, so // take it back off rather than let the thread collect duplicates. try { @@ -366,6 +346,27 @@ export async function updateSlackThreadMessageWithFooterText(params: { }`, ); } + return true; + } + + if ( + previousFooterMessageTs && + previousFooterMessageTs !== params.messageTs + ) { + try { + await removeSlackThreadReplyFooter({ + slack: params.slack, + channel: params.channel, + threadTs: params.threadTs, + messageTs: previousFooterMessageTs, + assertLock, + }); + } catch (error) { + console.error( + '[slackThreadFooter] Failed to remove prior footer', + error, + ); + } } return true; @@ -374,7 +375,7 @@ export async function updateSlackThreadMessageWithFooterText(params: { } /** - * Posts a Slack thread reply that becomes the sticky "Working on..." footer + * Posts a Slack thread reply that becomes the sticky navigation footer * message for the thread: attaches the current footer, then removes it from * whatever prior reply still carries the tracked footer. * @@ -394,10 +395,8 @@ export async function postSlackThreadMessageWithStickyFooter(params: { blocks?: unknown[]; utmCampaign?: string; /** - * Footer content style. `active` keeps linked PR / live preview when the - * shared resolver still considers the task active. `reply-only` is for - * terminal events (merged/closed PRs) so the relocated sticky line never - * reads as "still working on this" after the task becomes terminal. + * `reply-only` omits PR links for terminal PR events. Task status and any + * available preview remain independent of the PR lifecycle. */ footerStyle?: 'active' | 'reply-only'; }): Promise { @@ -414,10 +413,9 @@ export async function postSlackThreadMessageWithStickyFooter(params: { }); const replyOnly = params.footerStyle === 'reply-only'; const footerText = buildSlackThreadFooterText({ + ...footerContext, taskUrl, linkedPrs: replyOnly ? [] : footerContext.linkedPrs, - livePreviewUrl: replyOnly ? null : footerContext.livePreviewUrl, - explicitMentionRequired: footerContext.explicitMentionRequired, }); const bodyBlocks = params.blocks && params.blocks.length > 0 @@ -441,3 +439,166 @@ export async function postSlackThreadMessageWithStickyFooter(params: { footerText, }); } + +export async function rememberSlackThreadFooterRefresh( + params: { + slack: Partial>; + channel: string; + threadTs: string; + }, + assertLock: () => Promise, +): Promise { + // Registration is best effort, separate from successful carrier persistence. + try { + const teamId = await params.slack.getWorkspaceId?.(); + if (!teamId) return; + await assertLock(); + await getRedis().set( + `slack:thread_footer_workspace:${params.channel}:${params.threadTs}`, + teamId, + 'EX', + 30 * 24 * 60 * 60, + ); + await assertLock(); + await scheduleThreadFooterRefresh({ + provider: 'slack', + channelId: params.channel, + threadId: params.threadTs, + }); + } catch (error) { + console.warn( + '[slackThreadFooter] Failed to schedule footer refresh', + error, + ); + } +} + +/** + * Unregister a Slack destination, but only if its pointer still matches what + * this refresh pass read: a delivery that raced in owns the registration now. + */ +async function forgetSlackFooterIfUnchanged(params: { + channel: string; + threadTs: string; + seenMessageTs: string | null; +}): Promise { + const result = await tryThreadReplyFooterLock({ + lockKey: `${SLACK_THREAD_REPLY_FOOTER_LOCK_PREFIX}${params.channel}:${params.threadTs}`, + fn: async (assertLock): Promise => { + const current = await getSlackThreadReplyFooterMessageTs( + params.channel, + params.threadTs, + ); + if ((current ?? null) !== params.seenMessageTs) return 'active'; + await assertLock(); + await forgetThreadFooterRefresh({ + provider: 'slack', + channelId: params.channel, + threadId: params.threadTs, + }); + return 'gone'; + }, + }); + return result.acquired ? result.value : 'active'; +} + +/** + * Bring the current carrier's footer block up to date. Reading the message + * and resolving state happen outside the destination lock; the lock is held + * only for the edit, so a concurrent reply is never starved by Slack reads. + */ +export async function refreshSlackThreadReplyFooter(params: { + slack: Pick; + channel: string; + threadTs: string; +}): Promise { + const messageTs = await getSlackThreadReplyFooterMessageTs( + params.channel, + params.threadTs, + ); + if (!messageTs) + return forgetSlackFooterIfUnchanged({ ...params, seenMessageTs: null }); + const blocks = await params.slack.getMessageBlocks({ + channel: params.channel, + threadTs: params.threadTs, + messageTs, + throwOnUnavailable: true, + }); + const index = blocks?.findIndex(isSlackThreadReplyFooterBlock) ?? -1; + if (!blocks || index < 0) + return forgetSlackFooterIfUnchanged({ + ...params, + seenMessageTs: messageTs, + }); + const block = blocks[index] as { + text?: string; + elements?: { text?: string }[]; + }; + const posted = + block.elements?.find((element) => typeof element.text === 'string')?.text ?? + block.text; + if (!posted) + return forgetSlackFooterIfUnchanged({ + ...params, + seenMessageTs: messageTs, + }); + // Slack returns block text with `&`, `<` and `>` escaped; compare and + // resolve against the text as it was written. + const previous = decodeSlackEntity(posted); + const current = await resolveCurrentThreadFooter('slack', previous); + if (!current) { + console.warn( + '[slackThreadFooter] Retiring a footer that no longer resolves', + { channel: params.channel, threadTs: params.threadTs }, + ); + return forgetSlackFooterIfUnchanged({ + ...params, + seenMessageTs: messageTs, + }); + } + const outcome: ThreadFooterRefreshOutcome = current.active + ? 'active' + : 'idle'; + if (current.text === previous) { + return current.settled + ? forgetSlackFooterIfUnchanged({ ...params, seenMessageTs: messageTs }) + : outcome; + } + const result = await tryThreadReplyFooterLock({ + lockKey: `${SLACK_THREAD_REPLY_FOOTER_LOCK_PREFIX}${params.channel}:${params.threadTs}`, + fn: async (assertLock): Promise => { + // A reply relocated the footer while this pass was resolving; the next + // pass reads the new carrier. + if ( + (await getSlackThreadReplyFooterMessageTs( + params.channel, + params.threadTs, + )) !== messageTs + ) + return 'active'; + await assertLock(); + const updatedBlocks = [...blocks]; + updatedBlocks[index] = buildSlackThreadReplyFooterBlock({ + footerText: current.text, + }); + // updateMessage verifies ownership and preserves fallback text/attachments. + await params.slack.updateMessage({ + channel: params.channel, + ts: messageTs, + message: { blocks: updatedBlocks }, + }); + if (current.settled) { + await assertLock(); + await forgetThreadFooterRefresh({ + provider: 'slack', + channelId: params.channel, + threadId: params.threadTs, + }); + return 'gone'; + } + return outcome; + }, + }); + // A delivery holds the lock: it re-registers the destination itself. + return result.acquired ? result.value : 'active'; +}