diff --git a/apps/api/src/handlers/github/__tests__/notifyPrReviewActivity.test.ts b/apps/api/src/handlers/github/__tests__/notifyPrReviewActivity.test.ts index 1ce39b2cd..931bb991f 100644 --- a/apps/api/src/handlers/github/__tests__/notifyPrReviewActivity.test.ts +++ b/apps/api/src/handlers/github/__tests__/notifyPrReviewActivity.test.ts @@ -81,6 +81,7 @@ const pullRequest = { number: 42, html_url: 'https://github.com/owner/repo/pull/42', }; +const reviewHeadSha = 'f0c89ce4'; function reviewPayload(review: { body?: string | null; @@ -92,6 +93,7 @@ function reviewPayload(review: { pull_request: pullRequest, review: { body: review.body ?? null, + commit_id: reviewHeadSha, state: review.state ?? 'approved', html_url: 'https://github.com/owner/repo/pull/42#pullrequestreview-1000', user: review.login === null ? null : { login: review.login ?? 'alice' }, @@ -109,6 +111,7 @@ function reviewCommentPayload(comment: { pull_request: pullRequest, comment: { body: comment.body ?? 'Looks off to me', + commit_id: reviewHeadSha, in_reply_to_id: comment.inReplyToId, html_url: 'https://github.com/owner/repo/pull/42#discussion_r2000', user: comment.login === null ? null : { login: comment.login ?? 'alice' }, @@ -130,6 +133,7 @@ describe('buildPrReviewActivityNotificationInput', () => { event: { kind: 'review', authorLogin: 'alice', + reviewHeadSha, reviewState: 'changes_requested', url: 'https://github.com/owner/repo/pull/42#pullrequestreview-1000', }, @@ -181,6 +185,7 @@ describe('buildPrReviewActivityNotificationInput', () => { event: { kind: 'review_comment', authorLogin: 'bob', + reviewHeadSha, url: 'https://github.com/owner/repo/pull/42#discussion_r2000', }, }); @@ -246,6 +251,7 @@ describe('queuePrReviewActivityNotification', () => { event: { kind: 'review', authorLogin: 'alice', + reviewHeadSha, reviewState: 'approved', url: 'https://github.com/owner/repo/pull/42#pullrequestreview-1000', }, @@ -366,6 +372,7 @@ describe('buildPrReviewSummaryNotification', () => { event: { kind: 'review_summary', authorLogin: 'roomote[bot]', + reviewHeadSha, summary: '1 minor doc note; no blocking issues.', url: 'https://github.com/owner/repo/pull/42#issuecomment-99', roomoteAuthored: true, diff --git a/apps/api/src/handlers/github/notifyPrReviewActivity.ts b/apps/api/src/handlers/github/notifyPrReviewActivity.ts index c3bce38fe..3e8951290 100644 --- a/apps/api/src/handlers/github/notifyPrReviewActivity.ts +++ b/apps/api/src/handlers/github/notifyPrReviewActivity.ts @@ -78,6 +78,7 @@ export function buildPrReviewActivityNotificationInput( event: { kind: 'review', authorLogin, + ...(review.commit_id ? { reviewHeadSha: review.commit_id } : {}), reviewState: review.state, ...(review.html_url ? { url: review.html_url } : {}), ...(GitHubSchemas.isRoomoteGitHubLogin(authorLogin) @@ -110,6 +111,7 @@ export function buildPrReviewActivityNotificationInput( event: { kind: 'review_comment', authorLogin, + ...(comment.commit_id ? { reviewHeadSha: comment.commit_id } : {}), ...(comment.html_url ? { url: comment.html_url } : {}), ...(GitHubSchemas.isRoomoteGitHubLogin(authorLogin) ? { roomoteAuthored: true } @@ -254,6 +256,7 @@ export function buildPrReviewSummaryNotification( event: { kind: 'review_summary', authorLogin, + ...(markerSha ? { reviewHeadSha: markerSha } : {}), summary, ...(comment.html_url ? { url: comment.html_url } : {}), roomoteAuthored: true, diff --git a/packages/sdk/src/server/lib/task-runs/__tests__/pr-review-notification-delivery.test.ts b/packages/sdk/src/server/lib/task-runs/__tests__/pr-review-notification-delivery.test.ts index 1b8ecdf08..c37618e93 100644 --- a/packages/sdk/src/server/lib/task-runs/__tests__/pr-review-notification-delivery.test.ts +++ b/packages/sdk/src/server/lib/task-runs/__tests__/pr-review-notification-delivery.test.ts @@ -10,6 +10,8 @@ const { mockPullsGet, mockListCheckRunsForRef, mockGetCombinedStatusForRef, + mockIsRoomoteGitHubLogin, + mockResolveConfiguredGitHubAppSlug, } = vi.hoisted(() => ({ mockGenerateObject: vi.fn(), mockReadSourceControlPullRequest: vi.fn(), @@ -22,6 +24,8 @@ const { mockPullsGet: vi.fn(), mockListCheckRunsForRef: vi.fn(), mockGetCombinedStatusForRef: vi.fn(), + mockIsRoomoteGitHubLogin: vi.fn((login: string) => login === 'roomote[bot]'), + mockResolveConfiguredGitHubAppSlug: vi.fn(), })); vi.mock('@roomote/cloud-agents/server/non-task-provider-usage', () => ({ @@ -53,6 +57,8 @@ vi.mock('@roomote/cloud-agents/server', () => ({ return content.slice(start + startMarker.length, end); }, + isReviewInProgressStatusLine: (line: string) => + /^(Self-reviewing|Reviewing|Re-reviewing)/i.test(line.trim()), })); vi.mock('../../pull-requests/source-control-pull-request-reads', () => ({ @@ -81,6 +87,10 @@ vi.mock('@roomote/slack', () => ({ })); vi.mock('@roomote/github', () => ({ + Schemas: { + isRoomoteGitHubLogin: (login: string) => mockIsRoomoteGitHubLogin(login), + }, + resolveConfiguredGitHubAppSlug: () => mockResolveConfiguredGitHubAppSlug(), createTaskRunGitHubToken: (...args: unknown[]) => mockCreateTaskRunGitHubToken(...args), getOctokit: () => ({ @@ -134,6 +144,13 @@ const events: PrReviewActivityEvent[] = [ const eventsWithoutSelfReview: PrReviewActivityEvent[] = events.slice(0, 2); +beforeEach(() => { + mockIsRoomoteGitHubLogin.mockImplementation( + (login: string) => login === 'roomote[bot]', + ); + mockResolveConfiguredGitHubAppSlug.mockResolvedValue('roomote'); +}); + function mockGreenCiChecks() { mockCreateTaskRunGitHubToken.mockResolvedValue('github-token'); mockPullsGet.mockResolvedValue({ @@ -330,6 +347,180 @@ describe('preparePrReviewNotificationDelivery', () => { }), ).resolves.toEqual({ post: false, reason: 'not_worth_notifying' }); }); + + it('suppresses Roomote activity represented by a terminal summary for the same head', async () => { + await expect( + preparePrReviewNotificationDelivery({ + taskRun, + request, + events: [ + { + kind: 'review_comment', + authorLogin: 'roomote[bot]', + roomoteAuthored: true, + reviewHeadSha: 'abc', + }, + ], + }), + ).resolves.toEqual({ post: false, reason: 'not_worth_notifying' }); + + expect(mockGenerateObject).not.toHaveBeenCalled(); + expect(mockFormatMessage).not.toHaveBeenCalled(); + }); + + it('keeps human activity when matching Roomote activity is coalesced', async () => { + await preparePrReviewNotificationDelivery({ + taskRun, + request, + events: [ + { + kind: 'review_comment', + authorLogin: 'roomote[bot]', + roomoteAuthored: true, + reviewHeadSha: 'abc', + }, + { + kind: 'review_comment', + authorLogin: 'alice', + reviewHeadSha: 'abc', + }, + ], + }); + + const prompt = mockGenerateObject.mock.calls[0]?.[0]?.prompt as string; + expect(prompt).toContain('- alice left an inline review comment'); + expect(prompt).not.toContain('you (this is your own review)'); + }); + + it('keeps Roomote activity from a different reviewed head', async () => { + await preparePrReviewNotificationDelivery({ + taskRun, + request, + events: [ + { + kind: 'review_comment', + authorLogin: 'roomote[bot]', + roomoteAuthored: true, + reviewHeadSha: 'def', + }, + ], + }); + + const prompt = mockGenerateObject.mock.calls[0]?.[0]?.prompt as string; + expect(prompt).toContain('you (this is your own review)'); + }); + + it('keeps Roomote activity while the matching summary is still in progress', async () => { + mockReadSourceControlPullRequest.mockResolvedValue({ + success: true, + provider: 'github', + repositoryFullName: 'owner/repo', + number: 42, + threads: [], + issueComments: [ + { + id: 'c1', + author: 'roomote[bot]', + body: '\n\nReviewing the PR now.\n', + createdAt: null, + url: null, + }, + ], + warnings: [], + }); + + await preparePrReviewNotificationDelivery({ + taskRun, + request, + events: [ + { + kind: 'review_comment', + authorLogin: 'roomote[bot]', + roomoteAuthored: true, + reviewHeadSha: 'abc', + }, + ], + }); + + expect(mockGenerateObject).toHaveBeenCalled(); + }); + + it('keeps Roomote activity when a human posts a marker-shaped comment', async () => { + mockReadSourceControlPullRequest.mockResolvedValue({ + success: true, + provider: 'github', + repositoryFullName: 'owner/repo', + number: 42, + threads: [], + issueComments: [ + { + id: 'c1', + author: 'alice', + body: '\n\n1 issue outstanding.\n', + createdAt: null, + url: null, + }, + ], + warnings: [], + }); + + await preparePrReviewNotificationDelivery({ + taskRun, + request, + events: [ + { + kind: 'review_comment', + authorLogin: 'roomote[bot]', + roomoteAuthored: true, + reviewHeadSha: 'abc', + }, + ], + }); + + expect(mockGenerateObject).toHaveBeenCalled(); + }); + + it('resolves a custom GitHub App slug before classifying summary authors', async () => { + mockResolveConfiguredGitHubAppSlug.mockResolvedValue('acme'); + mockIsRoomoteGitHubLogin.mockImplementation((login: string) => { + expect(mockResolveConfiguredGitHubAppSlug).toHaveBeenCalled(); + return login === 'acme[bot]'; + }); + mockReadSourceControlPullRequest.mockResolvedValue({ + success: true, + provider: 'github', + repositoryFullName: 'owner/repo', + number: 42, + threads: [], + issueComments: [ + { + id: 'c1', + author: 'acme[bot]', + body: '\n\n1 issue outstanding.\n', + createdAt: null, + url: null, + }, + ], + warnings: [], + }); + + await expect( + preparePrReviewNotificationDelivery({ + taskRun, + request, + events: [ + { + kind: 'review_comment', + authorLogin: 'acme[bot]', + roomoteAuthored: true, + reviewHeadSha: 'abc', + }, + ], + }), + ).resolves.toEqual({ post: false, reason: 'not_worth_notifying' }); + + expect(mockGenerateObject).not.toHaveBeenCalled(); + }); }); describe('triagePrReviewActivity', () => { @@ -455,6 +646,7 @@ describe('triagePrReviewActivity', () => { latestReviewStatus: '2 issues outstanding.', latestReviewSummaryComment: '\n\n- [ ] `apps/api/src/foo.ts:10` - Handle null actor ids\n- [ ] `apps/api/src/bar.ts:20` - Rename the helper to match its return shape\n', + latestTerminalReviewSummaryHeadSha: 'abc', ciStatus: { checks: [ { name: 'CI / Lint', status: 'success' }, @@ -497,6 +689,7 @@ describe('triagePrReviewActivity', () => { unresolvedThreadCount: 0, latestReviewStatus: null, latestReviewSummaryComment: null, + latestTerminalReviewSummaryHeadSha: null, ciStatus: { checks: [{ name: 'CI / Tests', status: 'failure' }], }, @@ -603,6 +796,7 @@ describe('gatherPrReviewTriageContext', () => { latestReviewStatus: 'All 1 issue addressed. See task', latestReviewSummaryComment: '\n\n**All 1 issue addressed.** [See task](https://example.com)\n', + latestTerminalReviewSummaryHeadSha: 'abc', ciStatus: { checks: [ { name: 'CI / Lint', status: 'success' }, @@ -709,6 +903,7 @@ describe('gatherPrReviewTriageContext', () => { unresolvedThreadCount: null, latestReviewStatus: null, latestReviewSummaryComment: null, + latestTerminalReviewSummaryHeadSha: null, ciStatus: null, mergeable: null, }); diff --git a/packages/sdk/src/server/lib/task-runs/pr-review-notification-delivery.ts b/packages/sdk/src/server/lib/task-runs/pr-review-notification-delivery.ts index 2030d34e0..c9448f219 100644 --- a/packages/sdk/src/server/lib/task-runs/pr-review-notification-delivery.ts +++ b/packages/sdk/src/server/lib/task-runs/pr-review-notification-delivery.ts @@ -3,13 +3,19 @@ import { REVIEW_STATUS_START_MARKER, REVIEW_SUMMARY_MARKER, getMarkedSection, + isReviewInProgressStatusLine, } from '@roomote/cloud-agents/server'; import { generateTrackedNonTaskObject, NON_TASK_INFERENCE_SURFACES, } from '@roomote/cloud-agents/server/non-task-provider-usage'; import type { TaskRun } from '@roomote/db/server'; -import { createTaskRunGitHubToken, getOctokit } from '@roomote/github'; +import { + Schemas as GitHubSchemas, + createTaskRunGitHubToken, + getOctokit, + resolveConfiguredGitHubAppSlug, +} from '@roomote/github'; import { setLatestSlackBotReply, trackSlackBotReply } from '@roomote/slack'; import { ACP_ENVELOPE_EVENT_TYPES, @@ -65,6 +71,7 @@ export type PrReviewTriageContext = { unresolvedThreadCount: number | null; latestReviewStatus: string | null; latestReviewSummaryComment: string | null; + latestTerminalReviewSummaryHeadSha: string | null; /** * Per-check CI state for the PR head, when available. Fed into the * triage LLM so the chat message can mention CI naturally. @@ -527,6 +534,12 @@ function sanitizeReviewStatus(status: string): string { .slice(0, MAX_REVIEW_STATUS_LENGTH); } +function getReviewSummaryHeadSha(body: string): string | null { + return ( + body.match(/