From 672469440594b85f77dc98f55e96db51c9851eb5 Mon Sep 17 00:00:00 2001 From: Roomote Date: Mon, 13 Jul 2026 08:36:06 +0000 Subject: [PATCH] refactor: collapse dual terminal PR status writes into one helper --- .../ado/__tests__/handlePullRequest.test.ts | 47 +++--- .../api/src/handlers/ado/handlePullRequest.ts | 81 ++-------- .../__tests__/handlePullRequest.test.ts | 147 ++++++++++++++++++ .../handlers/bitbucket/handlePullRequest.ts | 22 +-- .../gitea/__tests__/handlePullRequest.test.ts | 43 +++-- .../src/handlers/gitea/handlePullRequest.ts | 22 +-- .../handlers/github/__tests__/index.test.ts | 7 +- apps/api/src/handlers/github/index.ts | 52 +++---- .../__tests__/handleMergeRequest.test.ts | 43 +++-- .../src/handlers/gitlab/handleMergeRequest.ts | 22 +-- packages/sdk/src/server/index.ts | 4 + .../mark-task-pull-request-terminal.test.ts | 108 +++++++++++++ .../mark-task-pull-request-terminal.ts | 63 ++++++++ 13 files changed, 466 insertions(+), 195 deletions(-) create mode 100644 apps/api/src/handlers/bitbucket/__tests__/handlePullRequest.test.ts create mode 100644 packages/sdk/src/server/lib/pull-requests/__tests__/mark-task-pull-request-terminal.test.ts create mode 100644 packages/sdk/src/server/lib/pull-requests/mark-task-pull-request-terminal.ts diff --git a/apps/api/src/handlers/ado/__tests__/handlePullRequest.test.ts b/apps/api/src/handlers/ado/__tests__/handlePullRequest.test.ts index 6f648d4a..f57abfa4 100644 --- a/apps/api/src/handlers/ado/__tests__/handlePullRequest.test.ts +++ b/apps/api/src/handlers/ado/__tests__/handlePullRequest.test.ts @@ -1,8 +1,7 @@ const { mockEnqueueTask, mockGetAdoAutomationTargets, - mockUpdateTaskPrStatus, - mockRecordPrStatusChangeInTaskHistory, + mockMarkTaskPullRequestTerminal, mockRepositoriesFindFirst, mockDedupSelect, mockNotifySlackPrMerge, @@ -11,8 +10,7 @@ const { } = vi.hoisted(() => ({ mockEnqueueTask: vi.fn(), mockGetAdoAutomationTargets: vi.fn(), - mockUpdateTaskPrStatus: vi.fn(), - mockRecordPrStatusChangeInTaskHistory: vi.fn(), + mockMarkTaskPullRequestTerminal: vi.fn(), mockRepositoriesFindFirst: vi.fn(), // Resolves the rows for each `db.select().from(tasks).innerJoin(...)` // dedup lookup, in call order. @@ -27,8 +25,7 @@ vi.mock('@roomote/cloud-agents/server', () => ({ })); vi.mock('@roomote/sdk/server', () => ({ - updateTaskPrStatus: mockUpdateTaskPrStatus, - recordPrStatusChangeInTaskHistory: mockRecordPrStatusChangeInTaskHistory, + markTaskPullRequestTerminal: mockMarkTaskPullRequestTerminal, })); vi.mock('@roomote/db/server', () => ({ @@ -168,7 +165,7 @@ describe('handleAdoPullRequest', () => { beforeEach(() => { mockEnqueueTask.mockReset(); mockGetAdoAutomationTargets.mockReset(); - mockUpdateTaskPrStatus.mockReset(); + mockMarkTaskPullRequestTerminal.mockReset(); mockRepositoriesFindFirst.mockReset(); mockDedupSelect.mockReset(); mockNotifySlackPrMerge.mockReset(); @@ -374,11 +371,18 @@ describe('handleAdoPullRequest', () => { ), ).resolves.toEqual({ status: 'ok' }); - expect(mockUpdateTaskPrStatus).toHaveBeenCalledWith( - 'ado', - 'acme/Platform/backend', - 42, - 'merged', + expect(mockMarkTaskPullRequestTerminal).toHaveBeenCalledWith( + { + sourceControlProvider: 'ado', + repository: 'acme/Platform/backend', + prNumber: 42, + prTitle: 'Update backend', + prUrl: + 'https://dev.azure.com/acme/Platform/_git/backend/pullrequest/42', + status: 'merged', + actorLogin: 'roomote-bot@acme.example', + }, + { logLabel: 'handleAdoPullRequest' }, ); expect(mockNotifySlackPrMerge).toHaveBeenCalledWith({ sourceControlProvider: 'ado', @@ -417,7 +421,7 @@ describe('handleAdoPullRequest', () => { message: 'unsupported_ado_pull_request_event:git.pullrequest.merged', }); - expect(mockUpdateTaskPrStatus).not.toHaveBeenCalled(); + expect(mockMarkTaskPullRequestTerminal).not.toHaveBeenCalled(); expect(mockNotifySlackPrMerge).not.toHaveBeenCalled(); expect(mockNotifyTeamsPrMerge).not.toHaveBeenCalled(); expect(mockNotifyTelegramAndLinearPrMerge).not.toHaveBeenCalled(); @@ -430,11 +434,18 @@ describe('handleAdoPullRequest', () => { { updatedNotificationType: 'StatusUpdateNotification' }, ); - expect(mockUpdateTaskPrStatus).toHaveBeenCalledWith( - 'ado', - 'acme/Platform/backend', - 42, - 'closed', + expect(mockMarkTaskPullRequestTerminal).toHaveBeenCalledWith( + { + sourceControlProvider: 'ado', + repository: 'acme/Platform/backend', + prNumber: 42, + prTitle: 'Update backend', + prUrl: + 'https://dev.azure.com/acme/Platform/_git/backend/pullrequest/42', + status: 'closed', + actorLogin: 'roomote-bot@acme.example', + }, + { logLabel: 'handleAdoPullRequest' }, ); expect(mockNotifySlackPrMerge).not.toHaveBeenCalled(); expect(mockNotifyTeamsPrMerge).not.toHaveBeenCalled(); diff --git a/apps/api/src/handlers/ado/handlePullRequest.ts b/apps/api/src/handlers/ado/handlePullRequest.ts index de364d0b..c4c7aced 100644 --- a/apps/api/src/handlers/ado/handlePullRequest.ts +++ b/apps/api/src/handlers/ado/handlePullRequest.ts @@ -20,10 +20,7 @@ import { sql, } from '@roomote/db/server'; import { enqueueTask } from '@roomote/cloud-agents/server'; -import { - recordPrStatusChangeInTaskHistory, - updateTaskPrStatus, -} from '@roomote/sdk/server'; +import { markTaskPullRequestTerminal } from '@roomote/sdk/server'; import type { WebhookResponse } from '../../types'; import { notifySlackPrMerge } from '../github/notifySlackPrMerge'; @@ -221,7 +218,10 @@ export async function handleAdoPullRequest( pullRequest, }); - if (pullRequest.status === 'abandoned') { + if ( + pullRequest.status === 'abandoned' || + pullRequest.status === 'completed' + ) { if (payload.eventType !== 'git.pullrequest.updated') { return { status: 'ok', @@ -229,15 +229,13 @@ export async function handleAdoPullRequest( }; } - await updateTaskPrStatus( - 'ado', - repoFullName, - pullRequest.pullRequestId, - 'closed', - ); + const status = + pullRequest.status === 'completed' + ? ('merged' as const) + : ('closed' as const); - await Promise.resolve( - recordPrStatusChangeInTaskHistory({ + await markTaskPullRequestTerminal( + { sourceControlProvider: 'ado', repository: repoFullName, prNumber: pullRequest.pullRequestId, @@ -247,62 +245,17 @@ export async function handleAdoPullRequest( pullRequest, repositoryFullName: repoFullName, }), - status: 'closed', + status, actorLogin: getAdoIdentityName(payload.resource.closedBy) ?? 'someone in Azure DevOps', - }), - ).catch((error) => { - console.warn( - `[handleAdoPullRequest] Failed to record PR status in task history for ${repoFullName}#${pullRequest.pullRequestId}: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - }); - - return { status: 'ok' }; - } - - if (pullRequest.status === 'completed') { - if (payload.eventType !== 'git.pullrequest.updated') { - return { - status: 'ok', - message: `unsupported_ado_pull_request_event:${payload.eventType}`, - }; - } - - await updateTaskPrStatus( - 'ado', - repoFullName, - pullRequest.pullRequestId, - 'merged', + }, + { logLabel: 'handleAdoPullRequest' }, ); - await Promise.resolve( - recordPrStatusChangeInTaskHistory({ - sourceControlProvider: 'ado', - repository: repoFullName, - prNumber: pullRequest.pullRequestId, - prTitle: pullRequest.title, - prUrl: getAdoPullRequestUrl({ - resourceContainers: payload.resourceContainers, - pullRequest, - repositoryFullName: repoFullName, - }), - status: 'merged', - actorLogin: - getAdoIdentityName(payload.resource.closedBy) ?? - 'someone in Azure DevOps', - }), - ).catch((error) => { - console.warn( - `[handleAdoPullRequest] Failed to record PR status in task history for ${repoFullName}#${pullRequest.pullRequestId}: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - }); - - await notifyMergedPullRequestThreads(payload, repoFullName); + if (status === 'merged') { + await notifyMergedPullRequestThreads(payload, repoFullName); + } return { status: 'ok' }; } diff --git a/apps/api/src/handlers/bitbucket/__tests__/handlePullRequest.test.ts b/apps/api/src/handlers/bitbucket/__tests__/handlePullRequest.test.ts new file mode 100644 index 00000000..4c59cbb8 --- /dev/null +++ b/apps/api/src/handlers/bitbucket/__tests__/handlePullRequest.test.ts @@ -0,0 +1,147 @@ +const { + mockMarkTaskPullRequestTerminal, + mockRepositoriesFindFirst, + mockNotifySlackPrMerge, + mockNotifyTeamsPrMerge, + mockNotifyTelegramAndLinearPrMerge, +} = vi.hoisted(() => ({ + mockMarkTaskPullRequestTerminal: vi.fn(), + mockRepositoriesFindFirst: vi.fn(), + mockNotifySlackPrMerge: vi.fn(), + mockNotifyTeamsPrMerge: vi.fn(), + mockNotifyTelegramAndLinearPrMerge: vi.fn(), +})); + +vi.mock('@roomote/cloud-agents/server', () => ({ + enqueueTask: vi.fn(), +})); + +vi.mock('@roomote/sdk/server', () => ({ + markTaskPullRequestTerminal: mockMarkTaskPullRequestTerminal, +})); + +vi.mock('@roomote/db/server', () => ({ + db: { + query: { + repositories: { + findFirst: (...args: unknown[]) => mockRepositoriesFindFirst(...args), + }, + }, + }, + repositories: { + sourceControlProvider: 'repositories.sourceControlProvider', + fullName: 'repositories.fullName', + isActive: 'repositories.isActive', + }, + and: vi.fn((...conditions: unknown[]) => ({ type: 'and', conditions })), + eq: vi.fn((left: unknown, right: unknown) => ({ type: 'eq', left, right })), + findActiveGitHubPrReviewTask: vi.fn(), +})); + +vi.mock('../../github/notifySlackPrMerge', () => ({ + notifySlackPrMerge: mockNotifySlackPrMerge, +})); + +vi.mock('../../github/notifyTeamsPrMerge', () => ({ + notifyTeamsPrMerge: mockNotifyTeamsPrMerge, +})); + +vi.mock('../../github/notifyTelegramAndLinearPrMerge', () => ({ + notifyTelegramAndLinearPrMerge: mockNotifyTelegramAndLinearPrMerge, +})); + +import { handleBitbucketPullRequest } from '../handlePullRequest'; +import type { BitbucketPullRequestWebhook } from '../types'; + +function makePayload( + overrides: Partial = {}, +): BitbucketPullRequestWebhook { + return { + actor: { uuid: '{user-1}', nickname: 'roomote-bot' }, + repository: { + uuid: '{repo-1}', + full_name: 'acme/backend', + }, + pullrequest: { + id: 42, + title: 'Update backend', + state: 'OPEN', + links: { + html: { + href: 'https://bitbucket.org/acme/backend/pull-requests/42', + }, + }, + source: { + branch: { name: 'feature/test' }, + commit: { hash: 'abc123' }, + }, + destination: { + branch: { name: 'main' }, + commit: { hash: 'def456' }, + }, + ...overrides, + }, + }; +} + +describe('handleBitbucketPullRequest terminal status', () => { + beforeEach(() => { + mockMarkTaskPullRequestTerminal.mockReset(); + mockRepositoriesFindFirst.mockReset(); + mockNotifySlackPrMerge.mockReset(); + mockNotifyTeamsPrMerge.mockReset(); + mockNotifyTelegramAndLinearPrMerge.mockReset(); + + mockMarkTaskPullRequestTerminal.mockResolvedValue(undefined); + mockRepositoriesFindFirst.mockResolvedValue({ id: 'repo-row-1' }); + mockNotifySlackPrMerge.mockResolvedValue(undefined); + mockNotifyTeamsPrMerge.mockResolvedValue(undefined); + mockNotifyTelegramAndLinearPrMerge.mockResolvedValue(undefined); + }); + + it('marks fulfilled pull requests as merged via the terminal helper', async () => { + await expect( + handleBitbucketPullRequest( + makePayload({ state: 'MERGED' }), + 'pullrequest:fulfilled', + ), + ).resolves.toEqual({ status: 'ok' }); + + expect(mockMarkTaskPullRequestTerminal).toHaveBeenCalledWith( + { + sourceControlProvider: 'bitbucket', + repository: 'acme/backend', + prNumber: 42, + prTitle: 'Update backend', + prUrl: 'https://bitbucket.org/acme/backend/pull-requests/42', + status: 'merged', + actorLogin: 'roomote-bot', + }, + { logLabel: 'handleBitbucketPullRequest' }, + ); + expect(mockNotifySlackPrMerge).toHaveBeenCalled(); + }); + + it('marks rejected pull requests as closed without merge notifications', async () => { + await handleBitbucketPullRequest( + makePayload({ state: 'DECLINED' }), + 'pullrequest:rejected', + ); + + expect(mockMarkTaskPullRequestTerminal).toHaveBeenCalledWith( + { + sourceControlProvider: 'bitbucket', + repository: 'acme/backend', + prNumber: 42, + prTitle: 'Update backend', + prUrl: 'https://bitbucket.org/acme/backend/pull-requests/42', + status: 'closed', + actorLogin: 'roomote-bot', + }, + { logLabel: 'handleBitbucketPullRequest' }, + ); + expect(mockNotifySlackPrMerge).not.toHaveBeenCalled(); + expect(mockNotifyTeamsPrMerge).not.toHaveBeenCalled(); + expect(mockNotifyTelegramAndLinearPrMerge).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/handlers/bitbucket/handlePullRequest.ts b/apps/api/src/handlers/bitbucket/handlePullRequest.ts index b265e4ec..1e23900f 100644 --- a/apps/api/src/handlers/bitbucket/handlePullRequest.ts +++ b/apps/api/src/handlers/bitbucket/handlePullRequest.ts @@ -14,10 +14,7 @@ import { findActiveGitHubPrReviewTask, } from '@roomote/db/server'; import { enqueueTask } from '@roomote/cloud-agents/server'; -import { - recordPrStatusChangeInTaskHistory, - updateTaskPrStatus, -} from '@roomote/sdk/server'; +import { markTaskPullRequestTerminal } from '@roomote/sdk/server'; import type { WebhookResponse } from '../../types'; import { notifySlackPrMerge } from '../github/notifySlackPrMerge'; @@ -128,10 +125,8 @@ export async function handleBitbucketPullRequest( const merged = isBitbucketPullRequestMerged(pullRequest); const status = merged ? ('merged' as const) : ('closed' as const); - await updateTaskPrStatus('bitbucket', repoFullName, prNumber, status); - - await Promise.resolve( - recordPrStatusChangeInTaskHistory({ + await markTaskPullRequestTerminal( + { sourceControlProvider: 'bitbucket', repository: repoFullName, prNumber, @@ -140,14 +135,9 @@ export async function handleBitbucketPullRequest( status, actorLogin: getBitbucketUsername(payload.actor) ?? 'someone on Bitbucket', - }), - ).catch((error) => { - console.warn( - `[handleBitbucketPullRequest] Failed to record PR status in task history for ${repoFullName}#${prNumber}: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - }); + }, + { logLabel: 'handleBitbucketPullRequest' }, + ); if (merged) { await notifyMergedPullRequestThreads(payload, repoFullName); diff --git a/apps/api/src/handlers/gitea/__tests__/handlePullRequest.test.ts b/apps/api/src/handlers/gitea/__tests__/handlePullRequest.test.ts index 7f3394ea..a010b2e3 100644 --- a/apps/api/src/handlers/gitea/__tests__/handlePullRequest.test.ts +++ b/apps/api/src/handlers/gitea/__tests__/handlePullRequest.test.ts @@ -1,8 +1,7 @@ const { mockEnqueueTask, mockGetGiteaAutomationTargets, - mockUpdateTaskPrStatus, - mockRecordPrStatusChangeInTaskHistory, + mockMarkTaskPullRequestTerminal, mockRepositoriesFindFirst, mockNotifySlackPrMerge, mockNotifyTeamsPrMerge, @@ -11,8 +10,7 @@ const { } = vi.hoisted(() => ({ mockEnqueueTask: vi.fn(), mockGetGiteaAutomationTargets: vi.fn(), - mockUpdateTaskPrStatus: vi.fn(), - mockRecordPrStatusChangeInTaskHistory: vi.fn(), + mockMarkTaskPullRequestTerminal: vi.fn(), mockRepositoriesFindFirst: vi.fn(), mockNotifySlackPrMerge: vi.fn(), mockNotifyTeamsPrMerge: vi.fn(), @@ -25,8 +23,7 @@ vi.mock('@roomote/cloud-agents/server', () => ({ })); vi.mock('@roomote/sdk/server', () => ({ - updateTaskPrStatus: mockUpdateTaskPrStatus, - recordPrStatusChangeInTaskHistory: mockRecordPrStatusChangeInTaskHistory, + markTaskPullRequestTerminal: mockMarkTaskPullRequestTerminal, })); vi.mock('@roomote/db/server', () => ({ @@ -105,7 +102,7 @@ describe('handleGiteaPullRequest', () => { beforeEach(() => { mockEnqueueTask.mockReset(); mockGetGiteaAutomationTargets.mockReset(); - mockUpdateTaskPrStatus.mockReset(); + mockMarkTaskPullRequestTerminal.mockReset(); mockRepositoriesFindFirst.mockReset(); mockNotifySlackPrMerge.mockReset(); mockNotifyTeamsPrMerge.mockReset(); @@ -234,11 +231,17 @@ describe('handleGiteaPullRequest', () => { handleGiteaPullRequest(makePayload('closed', { merged: true })), ).resolves.toEqual({ status: 'ok' }); - expect(mockUpdateTaskPrStatus).toHaveBeenCalledWith( - 'gitea', - 'acme/backend', - 42, - 'merged', + expect(mockMarkTaskPullRequestTerminal).toHaveBeenCalledWith( + { + sourceControlProvider: 'gitea', + repository: 'acme/backend', + prNumber: 42, + prTitle: 'Update backend', + prUrl: 'https://git.example.com/acme/backend/pulls/42', + status: 'merged', + actorLogin: 'roomote-bot', + }, + { logLabel: 'handleGiteaPullRequest' }, ); expect(mockNotifySlackPrMerge).toHaveBeenCalledWith({ sourceControlProvider: 'gitea', @@ -270,11 +273,17 @@ describe('handleGiteaPullRequest', () => { it('updates tracked task PR status without notifications for closed pull requests', async () => { await handleGiteaPullRequest(makePayload('closed')); - expect(mockUpdateTaskPrStatus).toHaveBeenCalledWith( - 'gitea', - 'acme/backend', - 42, - 'closed', + expect(mockMarkTaskPullRequestTerminal).toHaveBeenCalledWith( + { + sourceControlProvider: 'gitea', + repository: 'acme/backend', + prNumber: 42, + prTitle: 'Update backend', + prUrl: 'https://git.example.com/acme/backend/pulls/42', + status: 'closed', + actorLogin: 'roomote-bot', + }, + { logLabel: 'handleGiteaPullRequest' }, ); expect(mockNotifySlackPrMerge).not.toHaveBeenCalled(); expect(mockNotifyTeamsPrMerge).not.toHaveBeenCalled(); diff --git a/apps/api/src/handlers/gitea/handlePullRequest.ts b/apps/api/src/handlers/gitea/handlePullRequest.ts index 79d84886..aacd6fc6 100644 --- a/apps/api/src/handlers/gitea/handlePullRequest.ts +++ b/apps/api/src/handlers/gitea/handlePullRequest.ts @@ -14,10 +14,7 @@ import { findActiveGitHubPrReviewTask, } from '@roomote/db/server'; import { enqueueTask } from '@roomote/cloud-agents/server'; -import { - recordPrStatusChangeInTaskHistory, - updateTaskPrStatus, -} from '@roomote/sdk/server'; +import { markTaskPullRequestTerminal } from '@roomote/sdk/server'; import type { WebhookResponse } from '../../types'; import { notifySlackPrMerge } from '../github/notifySlackPrMerge'; @@ -125,10 +122,8 @@ export async function handleGiteaPullRequest( ? ('merged' as const) : ('closed' as const); - await updateTaskPrStatus('gitea', repoFullName, payload.number, status); - - await Promise.resolve( - recordPrStatusChangeInTaskHistory({ + await markTaskPullRequestTerminal( + { sourceControlProvider: 'gitea', repository: repoFullName, prNumber: payload.number, @@ -136,14 +131,9 @@ export async function handleGiteaPullRequest( prUrl: getPullRequestUrl(payload), status, actorLogin: getGiteaUsername(payload.sender) ?? 'someone on Gitea', - }), - ).catch((error) => { - console.warn( - `[handleGiteaPullRequest] Failed to record PR status in task history for ${repoFullName}#${payload.number}: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - }); + }, + { logLabel: 'handleGiteaPullRequest' }, + ); if (pullRequest.merged) { await notifyMergedPullRequestThreads(payload, repoFullName); diff --git a/apps/api/src/handlers/github/__tests__/index.test.ts b/apps/api/src/handlers/github/__tests__/index.test.ts index cb417104..d8aec1cb 100644 --- a/apps/api/src/handlers/github/__tests__/index.test.ts +++ b/apps/api/src/handlers/github/__tests__/index.test.ts @@ -18,7 +18,7 @@ const { mockResolveDeploymentEnvVar, mockUpdateTaskPrStatus, mockUpsertGitHubPullRequestFactFromWebhook, - mockRecordPrStatusChangeInTaskHistory, + mockMarkTaskPullRequestTerminal, mockVerifyAndReceive, webhooksConstructorParams, } = vi.hoisted(() => ({ @@ -46,7 +46,7 @@ const { mockResolveDeploymentEnvVar: vi.fn(), mockUpdateTaskPrStatus: vi.fn(), mockUpsertGitHubPullRequestFactFromWebhook: vi.fn(), - mockRecordPrStatusChangeInTaskHistory: vi.fn(), + mockMarkTaskPullRequestTerminal: vi.fn(), mockVerifyAndReceive: vi.fn(), webhooksConstructorParams: [] as unknown[], })); @@ -98,7 +98,7 @@ vi.mock('@roomote/sdk/server', () => ({ updateTaskPrStatus: mockUpdateTaskPrStatus, upsertGitHubPullRequestFactFromWebhook: mockUpsertGitHubPullRequestFactFromWebhook, - recordPrStatusChangeInTaskHistory: mockRecordPrStatusChangeInTaskHistory, + markTaskPullRequestTerminal: mockMarkTaskPullRequestTerminal, })); vi.mock('../../logging', () => ({ @@ -177,6 +177,7 @@ describe('github webhook router', () => { mockResolveDeploymentEnvVar.mockReset(); mockUpdateTaskPrStatus.mockReset(); mockUpsertGitHubPullRequestFactFromWebhook.mockReset(); + mockMarkTaskPullRequestTerminal.mockReset(); mockVerifyAndReceive.mockReset(); mockIsRepoSkipped.mockReturnValue(false); diff --git a/apps/api/src/handlers/github/index.ts b/apps/api/src/handlers/github/index.ts index 518cea27..bd87b260 100644 --- a/apps/api/src/handlers/github/index.ts +++ b/apps/api/src/handlers/github/index.ts @@ -4,7 +4,7 @@ import { Webhooks } from '@octokit/webhooks'; import { resolveDeploymentEnvVar } from '@roomote/db/server'; import { isRepoSkipped, resolveConfiguredGitHubAppSlug } from '@roomote/github'; import { - recordPrStatusChangeInTaskHistory, + markTaskPullRequestTerminal, updateTaskPrStatus, upsertGitHubPullRequestFactFromWebhook, } from '@roomote/sdk/server'; @@ -423,12 +423,31 @@ github.post('/', async (c) => { webhooks.on('pull_request.closed', ({ id, name, payload }) => recordWebhook(id, `${name}.${payload.action}`, payload, async () => { const status = payload.pull_request.merged ? 'merged' : 'closed'; + const repository = payload.repository.full_name; + const prNumber = payload.pull_request.number; - syncPrStatus( - payload.repository.full_name, - payload.pull_request.number, - status, + void markTaskPullRequestTerminal( + { + sourceControlProvider: 'github', + repository, + prNumber, + prTitle: payload.pull_request.title, + prUrl: payload.pull_request.html_url, + status, + actorLogin: + (payload.pull_request.merged + ? payload.pull_request.merged_by?.login + : null) || payload.sender.login, + }, + { logLabel: 'pull_request.closed' }, + ).catch((error) => + console.warn( + `[pull_request.closed] Failed to update PR status for ${repository}#${prNumber}: ${ + error instanceof Error ? error.message : String(error) + }`, + ), ); + syncPullRequestFact({ githubRepoId: payload.repository.id, repositoryFullName: payload.repository.full_name, @@ -447,29 +466,6 @@ github.post('/', async (c) => { }, }); - // Persist merged/closed status into linked task history so agents get - // the same out-of-band context path as PR review-feedback notifications. - void Promise.resolve( - recordPrStatusChangeInTaskHistory({ - sourceControlProvider: 'github', - repository: payload.repository.full_name, - prNumber: payload.pull_request.number, - prTitle: payload.pull_request.title, - prUrl: payload.pull_request.html_url, - status, - actorLogin: - (payload.pull_request.merged - ? payload.pull_request.merged_by?.login - : null) || payload.sender.login, - }), - ).catch((error) => { - console.warn( - `[pull_request.closed] Failed to record PR status in task history for ${payload.repository.full_name}#${payload.pull_request.number}: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - }); - if (isRepoSkipped(payload.repository.full_name)) { return { status: 'ok' as const, diff --git a/apps/api/src/handlers/gitlab/__tests__/handleMergeRequest.test.ts b/apps/api/src/handlers/gitlab/__tests__/handleMergeRequest.test.ts index 3cd9694b..cccbd21d 100644 --- a/apps/api/src/handlers/gitlab/__tests__/handleMergeRequest.test.ts +++ b/apps/api/src/handlers/gitlab/__tests__/handleMergeRequest.test.ts @@ -1,8 +1,7 @@ const { mockEnqueueTask, mockGetGitLabAutomationTargets, - mockUpdateTaskPrStatus, - mockRecordPrStatusChangeInTaskHistory, + mockMarkTaskPullRequestTerminal, mockRepositoriesFindFirst, mockNotifySlackPrMerge, mockNotifyTeamsPrMerge, @@ -11,8 +10,7 @@ const { } = vi.hoisted(() => ({ mockEnqueueTask: vi.fn(), mockGetGitLabAutomationTargets: vi.fn(), - mockUpdateTaskPrStatus: vi.fn(), - mockRecordPrStatusChangeInTaskHistory: vi.fn(), + mockMarkTaskPullRequestTerminal: vi.fn(), mockRepositoriesFindFirst: vi.fn(), mockNotifySlackPrMerge: vi.fn(), mockNotifyTeamsPrMerge: vi.fn(), @@ -25,8 +23,7 @@ vi.mock('@roomote/cloud-agents/server', () => ({ })); vi.mock('@roomote/sdk/server', () => ({ - updateTaskPrStatus: mockUpdateTaskPrStatus, - recordPrStatusChangeInTaskHistory: mockRecordPrStatusChangeInTaskHistory, + markTaskPullRequestTerminal: mockMarkTaskPullRequestTerminal, })); vi.mock('@roomote/db/server', () => ({ @@ -99,7 +96,7 @@ describe('handleGitLabMergeRequest', () => { beforeEach(() => { mockEnqueueTask.mockReset(); mockGetGitLabAutomationTargets.mockReset(); - mockUpdateTaskPrStatus.mockReset(); + mockMarkTaskPullRequestTerminal.mockReset(); mockRepositoriesFindFirst.mockReset(); mockNotifySlackPrMerge.mockReset(); mockNotifyTeamsPrMerge.mockReset(); @@ -238,11 +235,17 @@ describe('handleGitLabMergeRequest', () => { handleGitLabMergeRequest(makePayload('merge')), ).resolves.toEqual({ status: 'ok' }); - expect(mockUpdateTaskPrStatus).toHaveBeenCalledWith( - 'gitlab', - 'acme/backend', - 42, - 'merged', + expect(mockMarkTaskPullRequestTerminal).toHaveBeenCalledWith( + { + sourceControlProvider: 'gitlab', + repository: 'acme/backend', + prNumber: 42, + prTitle: 'Update backend', + prUrl: 'https://gitlab.com/acme/backend/-/merge_requests/42', + status: 'merged', + actorLogin: 'roomote-bot', + }, + { logLabel: 'handleGitLabMergeRequest' }, ); expect(mockEnqueueTask).not.toHaveBeenCalled(); }); @@ -270,11 +273,17 @@ describe('handleGitLabMergeRequest', () => { it('does not notify merge threads for closed merge requests', async () => { await handleGitLabMergeRequest(makePayload('close')); - expect(mockUpdateTaskPrStatus).toHaveBeenCalledWith( - 'gitlab', - 'acme/backend', - 42, - 'closed', + expect(mockMarkTaskPullRequestTerminal).toHaveBeenCalledWith( + { + sourceControlProvider: 'gitlab', + repository: 'acme/backend', + prNumber: 42, + prTitle: 'Update backend', + prUrl: 'https://gitlab.com/acme/backend/-/merge_requests/42', + status: 'closed', + actorLogin: 'roomote-bot', + }, + { logLabel: 'handleGitLabMergeRequest' }, ); expect(mockNotifySlackPrMerge).not.toHaveBeenCalled(); expect(mockNotifyTeamsPrMerge).not.toHaveBeenCalled(); diff --git a/apps/api/src/handlers/gitlab/handleMergeRequest.ts b/apps/api/src/handlers/gitlab/handleMergeRequest.ts index 467ae30f..5a2d3964 100644 --- a/apps/api/src/handlers/gitlab/handleMergeRequest.ts +++ b/apps/api/src/handlers/gitlab/handleMergeRequest.ts @@ -14,10 +14,7 @@ import { findActiveGitHubPrReviewTask, } from '@roomote/db/server'; import { enqueueTask } from '@roomote/cloud-agents/server'; -import { - recordPrStatusChangeInTaskHistory, - updateTaskPrStatus, -} from '@roomote/sdk/server'; +import { markTaskPullRequestTerminal } from '@roomote/sdk/server'; import type { WebhookResponse } from '../../types'; import { notifySlackPrMerge } from '../github/notifySlackPrMerge'; @@ -126,10 +123,8 @@ export async function handleGitLabMergeRequest( ? ('merged' as const) : ('closed' as const); - await updateTaskPrStatus('gitlab', repoFullName, mergeRequest.iid, status); - - await Promise.resolve( - recordPrStatusChangeInTaskHistory({ + await markTaskPullRequestTerminal( + { sourceControlProvider: 'gitlab', repository: repoFullName, prNumber: mergeRequest.iid, @@ -138,14 +133,9 @@ export async function handleGitLabMergeRequest( status, actorLogin: payload.user?.username ?? payload.user?.name ?? 'someone on GitLab', - }), - ).catch((error) => { - console.warn( - `[handleGitLabMergeRequest] Failed to record PR status in task history for ${repoFullName}!${mergeRequest.iid}: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - }); + }, + { logLabel: 'handleGitLabMergeRequest' }, + ); if (mergeRequest.action === 'merge') { await notifyMergedMergeRequestThreads(payload, repoFullName); diff --git a/packages/sdk/src/server/index.ts b/packages/sdk/src/server/index.ts index 51ebc97d..c440c888 100644 --- a/packages/sdk/src/server/index.ts +++ b/packages/sdk/src/server/index.ts @@ -162,6 +162,10 @@ export { } from './lib/slack-conversation-log'; export { updateTaskPrStatus } from './lib/pull-requests/update-task-pr-status'; +export { + markTaskPullRequestTerminal, + type MarkTaskPullRequestTerminalInput, +} from './lib/pull-requests/mark-task-pull-request-terminal'; export { createOrUpdateSourceControlPullRequestForTaskRun, findTaskRunForSourceControlMutation, diff --git a/packages/sdk/src/server/lib/pull-requests/__tests__/mark-task-pull-request-terminal.test.ts b/packages/sdk/src/server/lib/pull-requests/__tests__/mark-task-pull-request-terminal.test.ts new file mode 100644 index 00000000..61b3bbca --- /dev/null +++ b/packages/sdk/src/server/lib/pull-requests/__tests__/mark-task-pull-request-terminal.test.ts @@ -0,0 +1,108 @@ +const mockUpdateTaskPrStatus = vi.fn(); +const mockRecordPrStatusChangeInTaskHistory = vi.fn(); + +vi.mock('../update-task-pr-status', () => ({ + updateTaskPrStatus: (...args: unknown[]) => mockUpdateTaskPrStatus(...args), +})); + +vi.mock('../../task-runs/record-pr-status-change', async () => { + const actual = await vi.importActual< + typeof import('../../task-runs/record-pr-status-change') + >('../../task-runs/record-pr-status-change'); + + return { + ...actual, + recordPrStatusChangeInTaskHistory: (...args: unknown[]) => + mockRecordPrStatusChangeInTaskHistory(...args), + }; +}); + +import { markTaskPullRequestTerminal } from '../mark-task-pull-request-terminal'; + +describe('markTaskPullRequestTerminal', () => { + beforeEach(() => { + mockUpdateTaskPrStatus.mockReset(); + mockRecordPrStatusChangeInTaskHistory.mockReset(); + mockUpdateTaskPrStatus.mockResolvedValue(undefined); + mockRecordPrStatusChangeInTaskHistory.mockResolvedValue({ + recordedTaskCount: 1, + }); + }); + + it('updates task PR status then records history with the same terminal facts', async () => { + await markTaskPullRequestTerminal({ + sourceControlProvider: 'gitlab', + repository: 'acme/backend', + prNumber: 42, + status: 'merged', + prTitle: 'Ship it', + prUrl: 'https://gitlab.com/acme/backend/-/merge_requests/42', + actorLogin: 'merger', + }); + + expect(mockUpdateTaskPrStatus).toHaveBeenCalledWith( + 'gitlab', + 'acme/backend', + 42, + 'merged', + ); + expect(mockRecordPrStatusChangeInTaskHistory).toHaveBeenCalledWith({ + sourceControlProvider: 'gitlab', + repository: 'acme/backend', + prNumber: 42, + status: 'merged', + prTitle: 'Ship it', + prUrl: 'https://gitlab.com/acme/backend/-/merge_requests/42', + actorLogin: 'merger', + }); + }); + + it('swallows history failures after a successful status update', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + mockRecordPrStatusChangeInTaskHistory.mockRejectedValue( + new Error('history write failed'), + ); + + await expect( + markTaskPullRequestTerminal( + { + sourceControlProvider: 'github', + repository: 'acme/backend', + prNumber: 7, + status: 'closed', + prTitle: 'WIP', + prUrl: 'https://github.com/acme/backend/pull/7', + actorLogin: 'closer', + }, + { logLabel: 'test.closed' }, + ), + ).resolves.toBeUndefined(); + + expect(mockUpdateTaskPrStatus).toHaveBeenCalled(); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining( + '[test.closed] Failed to record PR status in task history for acme/backend#7: history write failed', + ), + ); + + warnSpy.mockRestore(); + }); + + it('propagates status-update failures and skips history recording', async () => { + mockUpdateTaskPrStatus.mockRejectedValue(new Error('db unavailable')); + + await expect( + markTaskPullRequestTerminal({ + sourceControlProvider: 'ado', + repository: 'acme/Platform/backend', + prNumber: 9, + status: 'merged', + prTitle: 'Done', + prUrl: 'https://dev.azure.com/acme/Platform/_git/backend/pullrequest/9', + actorLogin: 'merger', + }), + ).rejects.toThrow('db unavailable'); + + expect(mockRecordPrStatusChangeInTaskHistory).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/sdk/src/server/lib/pull-requests/mark-task-pull-request-terminal.ts b/packages/sdk/src/server/lib/pull-requests/mark-task-pull-request-terminal.ts new file mode 100644 index 00000000..f05d7d68 --- /dev/null +++ b/packages/sdk/src/server/lib/pull-requests/mark-task-pull-request-terminal.ts @@ -0,0 +1,63 @@ +import type { SourceControlProvider } from '@roomote/types'; + +import { + formatPullRequestReference, + recordPrStatusChangeInTaskHistory, + type RecordPrStatusChangeInTaskHistoryInput, +} from '../task-runs/record-pr-status-change'; +import { updateTaskPrStatus } from './update-task-pr-status'; + +export type MarkTaskPullRequestTerminalInput = + RecordPrStatusChangeInTaskHistoryInput & { + sourceControlProvider: SourceControlProvider; + }; + +/** + * Owns the co-required terminal PR side effects for merged/closed webhooks: + * update matching `task_pull_requests` rows, then record status into linked + * task history best-effort (history failures are logged, never thrown). + * + * Status-update failures still propagate so callers can choose await vs + * fire-and-forget semantics intentionally. + */ +export async function markTaskPullRequestTerminal( + input: MarkTaskPullRequestTerminalInput, + options?: { logLabel?: string }, +): Promise { + const logLabel = options?.logLabel ?? 'markTaskPullRequestTerminal'; + const { + sourceControlProvider, + repository, + prNumber, + status, + prTitle, + prUrl, + actorLogin, + } = input; + + await updateTaskPrStatus(sourceControlProvider, repository, prNumber, status); + + try { + await recordPrStatusChangeInTaskHistory({ + sourceControlProvider, + repository, + prNumber, + status, + prTitle, + prUrl, + actorLogin, + }); + } catch (error) { + const reference = formatPullRequestReference({ + repository, + prNumber, + sourceControlProvider, + }); + + console.warn( + `[${logLabel}] Failed to record PR status in task history for ${reference}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } +}