From 5c7c888fcdde27c0ecf38bb921d3d92360e7176b Mon Sep 17 00:00:00 2001 From: Roomote Date: Fri, 7 Aug 2026 02:40:26 +0000 Subject: [PATCH] refactor(sdk): separate PR review comment writes --- ...ntrol-pull-request-review-comments.test.ts | 1113 +++++++++++++++++ ...source-control-pull-request-writes.test.ts | 989 --------------- .../source-control-pull-request-ado-writes.ts | 58 + ...ce-control-pull-request-review-comments.ts | 565 +++++++++ ...ource-control-pull-request-write-errors.ts | 14 + .../source-control-pull-request-writes.ts | 519 +------- 6 files changed, 1788 insertions(+), 1470 deletions(-) create mode 100644 packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-review-comments.test.ts create mode 100644 packages/sdk/src/server/lib/pull-requests/source-control-pull-request-ado-writes.ts create mode 100644 packages/sdk/src/server/lib/pull-requests/source-control-pull-request-review-comments.ts create mode 100644 packages/sdk/src/server/lib/pull-requests/source-control-pull-request-write-errors.ts diff --git a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-review-comments.test.ts b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-review-comments.test.ts new file mode 100644 index 000000000..b7449805b --- /dev/null +++ b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-review-comments.test.ts @@ -0,0 +1,1113 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { RunStatus, TaskPayloadKind } from '@roomote/types'; +import type { TaskRun } from '@roomote/db/server'; + +const { + mockCreateGitHubToken, + mockGetOctokit, + mockRepositoriesFindFirst, + mockEnvironmentsFindFirst, + mockResolveGitLabToken, + mockResolveGiteaToken, + mockResolveGiteaBaseUrl, + mockBuildGiteaApiBaseUrl, + mockResolveAdoToken, + mockResolveAdoBaseUrl, + mockBuildAdoOrganizationApiBaseUrl, +} = vi.hoisted(() => ({ + mockCreateGitHubToken: vi.fn(), + mockGetOctokit: vi.fn(), + mockRepositoriesFindFirst: vi.fn(), + mockEnvironmentsFindFirst: vi.fn(), + mockResolveGitLabToken: vi.fn(), + mockResolveGiteaToken: vi.fn(), + mockResolveGiteaBaseUrl: vi.fn(), + mockBuildGiteaApiBaseUrl: vi.fn(), + mockResolveAdoToken: vi.fn(), + mockResolveAdoBaseUrl: vi.fn(), + mockBuildAdoOrganizationApiBaseUrl: vi.fn(), +})); + +vi.mock('@roomote/auth', () => ({ + createGitHubToken: (...args: unknown[]) => mockCreateGitHubToken(...args), +})); + +vi.mock('@roomote/github', () => ({ + getOctokit: (...args: unknown[]) => mockGetOctokit(...args), +})); + +vi.mock('@roomote/gitlab', () => ({ + resolveGitLabToken: (...args: unknown[]) => mockResolveGitLabToken(...args), + isGitLabOAuthAccessToken: () => false, + resolveGitLabBaseUrl: async () => 'https://gitlab.com', + buildGitLabApiBaseUrl: (baseUrl: string) => + `${baseUrl.replace(/\/+$/, '')}/api/v4`, +})); + +vi.mock('@roomote/gitea', () => ({ + resolveGiteaToken: (...args: unknown[]) => mockResolveGiteaToken(...args), + resolveGiteaBaseUrl: (...args: unknown[]) => mockResolveGiteaBaseUrl(...args), + buildGiteaApiBaseUrl: (...args: unknown[]) => + mockBuildGiteaApiBaseUrl(...args), +})); + +vi.mock('@roomote/bitbucket', () => ({ + resolveBitbucketAuth: async () => ({ + token: 'bitbucket-token', + username: 'bb-bot', + baseUrl: 'https://bitbucket.org', + apiBaseUrl: 'https://api.bitbucket.org/2.0', + authScheme: 'bearer', + }), + buildBitbucketApiBaseUrl: () => 'https://api.bitbucket.org/2.0', +})); + +vi.mock('@roomote/ado', () => ({ + resolveAdoToken: (...args: unknown[]) => mockResolveAdoToken(...args), + resolveAdoBaseUrl: (...args: unknown[]) => mockResolveAdoBaseUrl(...args), + buildAdoOrganizationApiBaseUrl: (...args: unknown[]) => + mockBuildAdoOrganizationApiBaseUrl(...args), +})); + +vi.mock('@roomote/db/server', () => ({ + db: { + query: { + repositories: { + findMany: async (...args: unknown[]) => { + const row = await mockRepositoriesFindFirst(...args); + return row == null ? [] : [row]; + }, + }, + environments: { + findFirst: (...args: unknown[]) => mockEnvironmentsFindFirst(...args), + }, + }, + }, + repositories: { + sourceControlProvider: 'repositories.sourceControlProvider', + fullName: 'repositories.fullName', + isActive: 'repositories.isActive', + }, + environments: { id: 'environments.id' }, + and: vi.fn((...conditions: unknown[]) => ({ type: 'and', conditions })), + eq: vi.fn((left: unknown, right: unknown) => ({ type: 'eq', left, right })), +})); + +import { writeSourceControlPullRequestForTaskRun } from '../source-control-pull-request-writes'; + +function makeTaskRun(payload: TaskRun['payload']): TaskRun { + return { + id: 123, + status: RunStatus.Dequeued, + kind: 'fresh', + payloadKind: TaskPayloadKind.StandardTask, + taskId: 'task-123', + actingUserId: 'user-123', + payload, + result: null, + artifacts: null, + } as TaskRun; +} + +function jsonResponse(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json' }, + }); +} + +beforeEach(() => { + vi.clearAllMocks(); + mockEnvironmentsFindFirst.mockResolvedValue(null); + mockResolveGitLabToken.mockResolvedValue('gitlab-token'); + mockResolveGiteaToken.mockResolvedValue('gitea-token'); + mockResolveGiteaBaseUrl.mockResolvedValue('https://git.example.com'); + mockBuildGiteaApiBaseUrl.mockReturnValue('https://git.example.com/api/v1'); + mockResolveAdoToken.mockResolvedValue('ado-token'); + mockResolveAdoBaseUrl.mockResolvedValue('https://dev.azure.com'); + mockBuildAdoOrganizationApiBaseUrl.mockReturnValue( + 'https://dev.azure.com/acme', + ); +}); + +describe('create_pull_request_review_comment', () => { + const githubRepoRow = { + installationId: 'installation-1', + externalRepoId: null, + fullName: 'acme/backend', + htmlUrl: 'https://github.com/acme/backend', + }; + + it('posts a GitHub inline comment anchored on the head SHA resolved at call time', async () => { + mockRepositoriesFindFirst.mockResolvedValue(githubRepoRow); + mockCreateGitHubToken.mockResolvedValue('github-token'); + const get = vi + .fn() + .mockResolvedValue({ data: { head: { sha: 'headsha123' } } }); + const createReviewComment = vi.fn().mockResolvedValue({ + data: { + id: 3001, + html_url: 'https://github.com/acme/backend/pull/55#discussion_r3001', + }, + }); + mockGetOctokit.mockReturnValue({ + rest: { pulls: { get, createReviewComment } }, + }); + + const result = await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'github', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 55, + path: 'src/index.ts', + line: 42, + body: 'Missing error handling here.', + sourceControlProvider: 'github', + }, + }); + + expect(get).toHaveBeenCalledWith({ + owner: 'acme', + repo: 'backend', + pull_number: 55, + }); + expect(createReviewComment).toHaveBeenCalledWith({ + owner: 'acme', + repo: 'backend', + pull_number: 55, + commit_id: 'headsha123', + path: 'src/index.ts', + line: 42, + side: 'RIGHT', + body: 'Missing error handling here.', + }); + expect(result).toMatchObject({ + success: true, + action: 'create_pull_request_review_comment', + provider: 'github', + number: 55, + threadId: null, + commentId: '3001', + url: 'https://github.com/acme/backend/pull/55#discussion_r3001', + applied: true, + warnings: [], + }); + }); + + it('passes a GitHub multi-line range through as start_line and start_side', async () => { + mockRepositoriesFindFirst.mockResolvedValue(githubRepoRow); + mockCreateGitHubToken.mockResolvedValue('github-token'); + const get = vi + .fn() + .mockResolvedValue({ data: { head: { sha: 'headsha123' } } }); + const createReviewComment = vi + .fn() + .mockResolvedValue({ data: { id: 3002, html_url: null } }); + mockGetOctokit.mockReturnValue({ + rest: { pulls: { get, createReviewComment } }, + }); + + const result = await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'github', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 55, + path: 'src/index.ts', + startLine: 40, + line: 42, + body: 'This whole block can be simplified.', + sourceControlProvider: 'github', + }, + }); + + expect(createReviewComment).toHaveBeenCalledWith( + expect.objectContaining({ + start_line: 40, + start_side: 'RIGHT', + line: 42, + side: 'RIGHT', + }), + ); + expect(result).toMatchObject({ applied: true, warnings: [] }); + }); + + it('maps a GitHub 422 to a retryable anchor rejection carrying the provider message', async () => { + mockRepositoriesFindFirst.mockResolvedValue(githubRepoRow); + mockCreateGitHubToken.mockResolvedValue('github-token'); + const get = vi + .fn() + .mockResolvedValue({ data: { head: { sha: 'headsha123' } } }); + const createReviewComment = vi + .fn() + .mockRejectedValue( + Object.assign( + new Error( + 'Validation Failed: Pull request review thread line must be part of the diff', + ), + { status: 422 }, + ), + ); + mockGetOctokit.mockReturnValue({ + rest: { pulls: { get, createReviewComment } }, + }); + + await expect( + writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'github', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 55, + path: 'src/index.ts', + line: 9999, + body: 'Missing error handling here.', + sourceControlProvider: 'github', + }, + }), + ).rejects.toMatchObject({ + name: 'SourceControlWriteError', + httpStatus: 422, + message: expect.stringContaining( + 'rejected the inline comment anchor (path=src/index.ts, line=9999, side=RIGHT)', + ), + }); + }); + + it('posts a GitLab positioned discussion using the merge request diff_refs', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: '101', + fullName: 'acme/backend', + htmlUrl: 'https://gitlab.com/acme/backend', + }); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ + diff_refs: { + base_sha: 'base1', + start_sha: 'start1', + head_sha: 'head1', + }, + }), + ) + .mockResolvedValueOnce( + jsonResponse([{ old_path: 'src/index.ts', new_path: 'src/index.ts' }]), + ) + .mockResolvedValueOnce( + jsonResponse({ id: 'disc-9', notes: [{ id: 601 }] }, 201), + ); + + const result = await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'gitlab', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 42, + path: 'src/index.ts', + line: 42, + body: 'Missing error handling here.', + sourceControlProvider: 'gitlab', + }, + fetchImpl, + }); + + expect(fetchImpl).toHaveBeenNthCalledWith( + 1, + 'https://gitlab.com/api/v4/projects/101/merge_requests/42', + expect.objectContaining({ method: 'GET' }), + ); + expect(fetchImpl).toHaveBeenNthCalledWith( + 2, + 'https://gitlab.com/api/v4/projects/101/merge_requests/42/diffs?page=1&per_page=100', + expect.objectContaining({ method: 'GET' }), + ); + expect(fetchImpl).toHaveBeenNthCalledWith( + 3, + 'https://gitlab.com/api/v4/projects/101/merge_requests/42/discussions', + expect.objectContaining({ + method: 'POST', + body: JSON.stringify({ + body: 'Missing error handling here.', + position: { + position_type: 'text', + base_sha: 'base1', + start_sha: 'start1', + head_sha: 'head1', + new_path: 'src/index.ts', + old_path: 'src/index.ts', + new_line: 42, + }, + }), + }), + ); + expect(result).toMatchObject({ + success: true, + provider: 'gitlab', + threadId: 'disc-9', + commentId: '601', + applied: true, + warnings: [], + }); + }); + + it('anchors LEFT-side GitLab comments with old_line instead of new_line', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: '101', + fullName: 'acme/backend', + htmlUrl: 'https://gitlab.com/acme/backend', + }); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ + diff_refs: { + base_sha: 'base1', + start_sha: 'start1', + head_sha: 'head1', + }, + }), + ) + .mockResolvedValueOnce( + jsonResponse([{ old_path: 'src/index.ts', new_path: 'src/index.ts' }]), + ) + .mockResolvedValueOnce(jsonResponse({ id: 'disc-9' }, 201)); + + await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'gitlab', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 42, + path: 'src/index.ts', + line: 17, + side: 'LEFT', + body: 'This deletion drops the retry path.', + sourceControlProvider: 'gitlab', + }, + fetchImpl, + }); + + const discussionBody = JSON.parse( + (fetchImpl.mock.calls[2]?.[1] as { body: string }).body, + ) as { position: Record }; + expect(discussionBody.position.old_line).toBe(17); + expect(discussionBody.position.new_line).toBeUndefined(); + }); + + it('resolves the real old_path for renamed files from the merge request diffs', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: '101', + fullName: 'acme/backend', + htmlUrl: 'https://gitlab.com/acme/backend', + }); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ + diff_refs: { + base_sha: 'base1', + start_sha: 'start1', + head_sha: 'head1', + }, + }), + ) + .mockResolvedValueOnce( + jsonResponse([ + { old_path: 'src/legacy/index.ts', new_path: 'src/index.ts' }, + ]), + ) + .mockResolvedValueOnce(jsonResponse({ id: 'disc-9' }, 201)); + + await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'gitlab', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 42, + path: 'src/index.ts', + line: 42, + body: 'Missing error handling here.', + sourceControlProvider: 'gitlab', + }, + fetchImpl, + }); + + const discussionBody = JSON.parse( + (fetchImpl.mock.calls[2]?.[1] as { body: string }).body, + ) as { position: Record }; + expect(discussionBody.position.new_path).toBe('src/index.ts'); + expect(discussionBody.position.old_path).toBe('src/legacy/index.ts'); + }); + + it('keeps scanning diff pages until the renamed file is found', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: '101', + fullName: 'acme/backend', + htmlUrl: 'https://gitlab.com/acme/backend', + }); + const fillerPage = Array.from({ length: 100 }, (_, i) => ({ + old_path: `src/other-${i}.ts`, + new_path: `src/other-${i}.ts`, + })); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ + diff_refs: { + base_sha: 'base1', + start_sha: 'start1', + head_sha: 'head1', + }, + }), + ) + .mockResolvedValueOnce(jsonResponse(fillerPage)) + .mockResolvedValueOnce( + jsonResponse([ + { old_path: 'src/legacy/index.ts', new_path: 'src/index.ts' }, + ]), + ) + .mockResolvedValueOnce(jsonResponse({ id: 'disc-9' }, 201)); + + await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'gitlab', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 42, + path: 'src/index.ts', + line: 42, + body: 'Missing error handling here.', + sourceControlProvider: 'gitlab', + }, + fetchImpl, + }); + + expect(fetchImpl).toHaveBeenNthCalledWith( + 3, + 'https://gitlab.com/api/v4/projects/101/merge_requests/42/diffs?page=2&per_page=100', + expect.objectContaining({ method: 'GET' }), + ); + const discussionBody = JSON.parse( + (fetchImpl.mock.calls[3]?.[1] as { body: string }).body, + ) as { position: Record }; + expect(discussionBody.position.old_path).toBe('src/legacy/index.ts'); + }); + + it('surfaces an explicit warning when the diff scan backstop ends before the listing', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: '101', + fullName: 'acme/backend', + htmlUrl: 'https://gitlab.com/acme/backend', + }); + const fillerPage = Array.from({ length: 100 }, (_, i) => ({ + old_path: `src/other-${i}.ts`, + new_path: `src/other-${i}.ts`, + })); + const fetchImpl = vi.fn().mockImplementation(async (url: string) => { + if (url.includes('/diffs')) { + return jsonResponse(fillerPage); + } + if (url.endsWith('/discussions')) { + return jsonResponse({ id: 'disc-9' }, 201); + } + return jsonResponse({ + diff_refs: { + base_sha: 'base1', + start_sha: 'start1', + head_sha: 'head1', + }, + }); + }); + + const result = await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'gitlab', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 42, + path: 'src/index.ts', + line: 42, + body: 'Missing error handling here.', + sourceControlProvider: 'gitlab', + }, + fetchImpl, + }); + + const diffCalls = fetchImpl.mock.calls.filter(([url]) => + String(url).includes('/diffs'), + ); + expect(diffCalls).toHaveLength(50); + expect(result.warnings).toEqual([ + expect.stringContaining( + 'rename resolution fell back to the request path', + ), + ]); + }); + + it('falls back to the same-path pair when the diff listing is unavailable', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: '101', + fullName: 'acme/backend', + htmlUrl: 'https://gitlab.com/acme/backend', + }); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ + diff_refs: { + base_sha: 'base1', + start_sha: 'start1', + head_sha: 'head1', + }, + }), + ) + .mockResolvedValueOnce(jsonResponse({ message: 'nope' }, 500)) + .mockResolvedValueOnce(jsonResponse({ id: 'disc-9' }, 201)); + + await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'gitlab', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 42, + path: 'src/index.ts', + line: 42, + body: 'Missing error handling here.', + sourceControlProvider: 'gitlab', + }, + fetchImpl, + }); + + const discussionBody = JSON.parse( + (fetchImpl.mock.calls[2]?.[1] as { body: string }).body, + ) as { position: Record }; + expect(discussionBody.position.new_path).toBe('src/index.ts'); + expect(discussionBody.position.old_path).toBe('src/index.ts'); + }); + + it('maps a GitLab 400 on discussions to a retryable anchor rejection', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: '101', + fullName: 'acme/backend', + htmlUrl: 'https://gitlab.com/acme/backend', + }); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ + diff_refs: { + base_sha: 'base1', + start_sha: 'start1', + head_sha: 'head1', + }, + }), + ) + .mockResolvedValueOnce( + jsonResponse([{ old_path: 'src/index.ts', new_path: 'src/index.ts' }]), + ) + .mockResolvedValueOnce( + jsonResponse({ message: 'line_code must be a valid line code' }, 400), + ); + + await expect( + writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'gitlab', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 42, + path: 'src/index.ts', + line: 9999, + body: 'Missing error handling here.', + sourceControlProvider: 'gitlab', + }, + fetchImpl, + }), + ).rejects.toMatchObject({ + name: 'SourceControlWriteError', + httpStatus: 422, + message: expect.stringContaining( + 'target a line changed in this merge request', + ), + }); + }); + + it('reports missing GitLab diff_refs as a retryable 409', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: '101', + fullName: 'acme/backend', + htmlUrl: 'https://gitlab.com/acme/backend', + }); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(jsonResponse({ diff_refs: null })); + + await expect( + writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'gitlab', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 42, + path: 'src/index.ts', + line: 42, + body: 'Missing error handling here.', + sourceControlProvider: 'gitlab', + }, + fetchImpl, + }), + ).rejects.toMatchObject({ + name: 'SourceControlWriteError', + httpStatus: 409, + }); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + + it('degrades a GitLab multi-line range to the end line with a warning', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: '101', + fullName: 'acme/backend', + htmlUrl: 'https://gitlab.com/acme/backend', + }); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ + diff_refs: { + base_sha: 'base1', + start_sha: 'start1', + head_sha: 'head1', + }, + }), + ) + .mockResolvedValueOnce( + jsonResponse([{ old_path: 'src/index.ts', new_path: 'src/index.ts' }]), + ) + .mockResolvedValueOnce(jsonResponse({ id: 'disc-9' }, 201)); + + const result = await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'gitlab', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 42, + path: 'src/index.ts', + startLine: 40, + line: 42, + body: 'This whole block can be simplified.', + sourceControlProvider: 'gitlab', + }, + fetchImpl, + }); + + expect(result).toMatchObject({ + applied: true, + warnings: [ + 'GitLab does not support multi-line comment positions through this surface; the comment is anchored to line 42.', + ], + }); + }); + + it('posts a Gitea single-comment review with a positioned comment', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: null, + fullName: 'acme/backend', + htmlUrl: 'https://git.example.com/acme/backend', + }); + const fetchImpl = vi.fn().mockResolvedValueOnce( + jsonResponse( + { + id: 71, + html_url: 'https://git.example.com/acme/backend/pulls/9#review-71', + }, + 201, + ), + ); + + const result = await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'gitea', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 9, + path: 'src/index.ts', + line: 42, + body: 'Missing error handling here.', + sourceControlProvider: 'gitea', + }, + fetchImpl, + }); + + expect(fetchImpl).toHaveBeenCalledWith( + 'https://git.example.com/api/v1/repos/acme/backend/pulls/9/reviews', + expect.objectContaining({ + method: 'POST', + body: JSON.stringify({ + event: 'COMMENT', + body: '', + comments: [ + { + path: 'src/index.ts', + body: 'Missing error handling here.', + new_position: 42, + }, + ], + }), + }), + ); + expect(result).toMatchObject({ + success: true, + provider: 'gitea', + threadId: '71', + url: 'https://git.example.com/acme/backend/pulls/9#review-71', + applied: true, + warnings: [], + }); + }); + + it('maps a Gitea 422 to a retryable anchor rejection', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: null, + fullName: 'acme/backend', + htmlUrl: 'https://git.example.com/acme/backend', + }); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ message: 'position is invalid' }, 422), + ); + + await expect( + writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'gitea', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 9, + path: 'src/index.ts', + line: 9999, + body: 'Missing error handling here.', + sourceControlProvider: 'gitea', + }, + fetchImpl, + }), + ).rejects.toMatchObject({ + name: 'SourceControlWriteError', + httpStatus: 422, + message: expect.stringContaining('rejected the inline comment anchor'), + }); + }); + + it('posts a Bitbucket inline comment anchored with to on the destination side', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: null, + fullName: 'acme/backend', + htmlUrl: 'https://bitbucket.org/acme/backend', + }); + const fetchImpl = vi.fn().mockResolvedValueOnce( + jsonResponse( + { + id: 88, + links: { + html: { + href: 'https://bitbucket.org/acme/backend/pull-requests/5#comment-88', + }, + }, + }, + 201, + ), + ); + + const result = await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'bitbucket', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 5, + path: 'src/index.ts', + line: 42, + body: 'Missing error handling here.', + sourceControlProvider: 'bitbucket', + }, + fetchImpl, + }); + + const [url, request] = fetchImpl.mock.calls[0] as [ + string, + { method: string; body: string }, + ]; + expect(url).toContain('/pullrequests/5/comments'); + expect(JSON.parse(request.body)).toEqual({ + content: { raw: 'Missing error handling here.' }, + inline: { path: 'src/index.ts', to: 42 }, + }); + expect(result).toMatchObject({ + success: true, + provider: 'bitbucket', + threadId: '88', + commentId: '88', + applied: true, + warnings: [], + }); + }); + + it('anchors LEFT-side Bitbucket comments with from instead of to', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: null, + fullName: 'acme/backend', + htmlUrl: 'https://bitbucket.org/acme/backend', + }); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(jsonResponse({ id: 89 }, 201)); + + await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'bitbucket', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 5, + path: 'src/index.ts', + line: 17, + side: 'LEFT', + body: 'This deletion drops the retry path.', + sourceControlProvider: 'bitbucket', + }, + fetchImpl, + }); + + const request = fetchImpl.mock.calls[0]?.[1] as { body: string }; + expect(JSON.parse(request.body)).toMatchObject({ + inline: { path: 'src/index.ts', from: 17 }, + }); + }); + + it('creates an Azure DevOps thread with a right-side file range including startLine', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: 'repo-uuid', + fullName: 'acme/Platform/backend', + htmlUrl: 'https://dev.azure.com/acme/Platform/_git/backend', + }); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + jsonResponse({ id: 31, comments: [{ id: 1 }] }, 200), + ); + + const result = await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/Platform/backend', + sourceControlProvider: 'ado', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/Platform/backend', + prNumber: 7, + path: 'src/index.ts', + startLine: 40, + line: 42, + body: 'This whole block can be simplified.', + sourceControlProvider: 'ado', + }, + fetchImpl, + }); + + expect(fetchImpl).toHaveBeenCalledWith( + 'https://dev.azure.com/acme/Platform/_apis/git/repositories/repo-uuid/pullrequests/7/threads?api-version=7.1', + expect.objectContaining({ + method: 'POST', + body: JSON.stringify({ + comments: [ + { + content: 'This whole block can be simplified.', + commentType: 'text', + }, + ], + status: 'active', + threadContext: { + filePath: '/src/index.ts', + rightFileStart: { line: 40, offset: 1 }, + rightFileEnd: { line: 42, offset: 1 }, + }, + }), + }), + ); + expect(result).toMatchObject({ + success: true, + provider: 'ado', + threadId: '31', + commentId: '1', + applied: true, + warnings: [], + }); + }); + + it('anchors LEFT-side Azure DevOps comments with leftFileStart and leftFileEnd', async () => { + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: null, + externalRepoId: 'repo-uuid', + fullName: 'acme/Platform/backend', + htmlUrl: 'https://dev.azure.com/acme/Platform/_git/backend', + }); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(jsonResponse({ id: 32 }, 200)); + + await writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/Platform/backend', + sourceControlProvider: 'ado', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/Platform/backend', + prNumber: 7, + path: 'src/index.ts', + line: 17, + side: 'LEFT', + body: 'This deletion drops the retry path.', + sourceControlProvider: 'ado', + }, + fetchImpl, + }); + + const request = fetchImpl.mock.calls[0]?.[1] as { body: string }; + expect(JSON.parse(request.body)).toMatchObject({ + threadContext: { + filePath: '/src/index.ts', + leftFileStart: { line: 17, offset: 1 }, + leftFileEnd: { line: 17, offset: 1 }, + }, + }); + }); + + it('rejects requests without a path, line, or body before touching the database', async () => { + const fetchImpl = vi.fn(); + const base = { + action: 'create_pull_request_review_comment' as const, + repositoryFullName: 'acme/backend', + prNumber: 1, + sourceControlProvider: 'github' as const, + }; + const taskRun = makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'github', + }); + + await expect( + writeSourceControlPullRequestForTaskRun({ + taskRun, + input: { ...base, line: 42, body: 'x' }, + fetchImpl, + }), + ).rejects.toThrow( + 'path is required for create_pull_request_review_comment.', + ); + await expect( + writeSourceControlPullRequestForTaskRun({ + taskRun, + input: { ...base, path: 'src/index.ts', body: 'x' }, + fetchImpl, + }), + ).rejects.toThrow( + 'line is required for create_pull_request_review_comment.', + ); + await expect( + writeSourceControlPullRequestForTaskRun({ + taskRun, + input: { ...base, path: 'src/index.ts', line: 42 }, + fetchImpl, + }), + ).rejects.toThrow( + 'body is required for create_pull_request_review_comment.', + ); + expect(fetchImpl).not.toHaveBeenCalled(); + expect(mockRepositoriesFindFirst).not.toHaveBeenCalled(); + }); + + it('rejects a startLine greater than line', async () => { + await expect( + writeSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ + repo: 'acme/backend', + sourceControlProvider: 'github', + }), + input: { + action: 'create_pull_request_review_comment', + repositoryFullName: 'acme/backend', + prNumber: 1, + path: 'src/index.ts', + startLine: 50, + line: 42, + body: 'x', + sourceControlProvider: 'github', + }, + }), + ).rejects.toThrow('startLine must not be greater than line'); + }); +}); diff --git a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-writes.test.ts b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-writes.test.ts index 152668cff..e95a1c809 100644 --- a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-writes.test.ts +++ b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-writes.test.ts @@ -707,995 +707,6 @@ describe('writeSourceControlPullRequestForTaskRun', () => { expect(mockRepositoriesFindFirst).not.toHaveBeenCalled(); }); - describe('create_pull_request_review_comment', () => { - const githubRepoRow = { - installationId: 'installation-1', - externalRepoId: null, - fullName: 'acme/backend', - htmlUrl: 'https://github.com/acme/backend', - }; - - it('posts a GitHub inline comment anchored on the head SHA resolved at call time', async () => { - mockRepositoriesFindFirst.mockResolvedValue(githubRepoRow); - mockCreateGitHubToken.mockResolvedValue('github-token'); - const get = vi - .fn() - .mockResolvedValue({ data: { head: { sha: 'headsha123' } } }); - const createReviewComment = vi.fn().mockResolvedValue({ - data: { - id: 3001, - html_url: 'https://github.com/acme/backend/pull/55#discussion_r3001', - }, - }); - mockGetOctokit.mockReturnValue({ - rest: { pulls: { get, createReviewComment } }, - }); - - const result = await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'github', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 55, - path: 'src/index.ts', - line: 42, - body: 'Missing error handling here.', - sourceControlProvider: 'github', - }, - }); - - expect(get).toHaveBeenCalledWith({ - owner: 'acme', - repo: 'backend', - pull_number: 55, - }); - expect(createReviewComment).toHaveBeenCalledWith({ - owner: 'acme', - repo: 'backend', - pull_number: 55, - commit_id: 'headsha123', - path: 'src/index.ts', - line: 42, - side: 'RIGHT', - body: 'Missing error handling here.', - }); - expect(result).toMatchObject({ - success: true, - action: 'create_pull_request_review_comment', - provider: 'github', - number: 55, - threadId: null, - commentId: '3001', - url: 'https://github.com/acme/backend/pull/55#discussion_r3001', - applied: true, - warnings: [], - }); - }); - - it('passes a GitHub multi-line range through as start_line and start_side', async () => { - mockRepositoriesFindFirst.mockResolvedValue(githubRepoRow); - mockCreateGitHubToken.mockResolvedValue('github-token'); - const get = vi - .fn() - .mockResolvedValue({ data: { head: { sha: 'headsha123' } } }); - const createReviewComment = vi - .fn() - .mockResolvedValue({ data: { id: 3002, html_url: null } }); - mockGetOctokit.mockReturnValue({ - rest: { pulls: { get, createReviewComment } }, - }); - - const result = await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'github', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 55, - path: 'src/index.ts', - startLine: 40, - line: 42, - body: 'This whole block can be simplified.', - sourceControlProvider: 'github', - }, - }); - - expect(createReviewComment).toHaveBeenCalledWith( - expect.objectContaining({ - start_line: 40, - start_side: 'RIGHT', - line: 42, - side: 'RIGHT', - }), - ); - expect(result).toMatchObject({ applied: true, warnings: [] }); - }); - - it('maps a GitHub 422 to a retryable anchor rejection carrying the provider message', async () => { - mockRepositoriesFindFirst.mockResolvedValue(githubRepoRow); - mockCreateGitHubToken.mockResolvedValue('github-token'); - const get = vi - .fn() - .mockResolvedValue({ data: { head: { sha: 'headsha123' } } }); - const createReviewComment = vi - .fn() - .mockRejectedValue( - Object.assign( - new Error( - 'Validation Failed: Pull request review thread line must be part of the diff', - ), - { status: 422 }, - ), - ); - mockGetOctokit.mockReturnValue({ - rest: { pulls: { get, createReviewComment } }, - }); - - await expect( - writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'github', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 55, - path: 'src/index.ts', - line: 9999, - body: 'Missing error handling here.', - sourceControlProvider: 'github', - }, - }), - ).rejects.toMatchObject({ - name: 'SourceControlWriteError', - httpStatus: 422, - message: expect.stringContaining( - 'rejected the inline comment anchor (path=src/index.ts, line=9999, side=RIGHT)', - ), - }); - }); - - it('posts a GitLab positioned discussion using the merge request diff_refs', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: '101', - fullName: 'acme/backend', - htmlUrl: 'https://gitlab.com/acme/backend', - }); - const fetchImpl = vi - .fn() - .mockResolvedValueOnce( - jsonResponse({ - diff_refs: { - base_sha: 'base1', - start_sha: 'start1', - head_sha: 'head1', - }, - }), - ) - .mockResolvedValueOnce( - jsonResponse([ - { old_path: 'src/index.ts', new_path: 'src/index.ts' }, - ]), - ) - .mockResolvedValueOnce( - jsonResponse({ id: 'disc-9', notes: [{ id: 601 }] }, 201), - ); - - const result = await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'gitlab', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 42, - path: 'src/index.ts', - line: 42, - body: 'Missing error handling here.', - sourceControlProvider: 'gitlab', - }, - fetchImpl, - }); - - expect(fetchImpl).toHaveBeenNthCalledWith( - 1, - 'https://gitlab.com/api/v4/projects/101/merge_requests/42', - expect.objectContaining({ method: 'GET' }), - ); - expect(fetchImpl).toHaveBeenNthCalledWith( - 2, - 'https://gitlab.com/api/v4/projects/101/merge_requests/42/diffs?page=1&per_page=100', - expect.objectContaining({ method: 'GET' }), - ); - expect(fetchImpl).toHaveBeenNthCalledWith( - 3, - 'https://gitlab.com/api/v4/projects/101/merge_requests/42/discussions', - expect.objectContaining({ - method: 'POST', - body: JSON.stringify({ - body: 'Missing error handling here.', - position: { - position_type: 'text', - base_sha: 'base1', - start_sha: 'start1', - head_sha: 'head1', - new_path: 'src/index.ts', - old_path: 'src/index.ts', - new_line: 42, - }, - }), - }), - ); - expect(result).toMatchObject({ - success: true, - provider: 'gitlab', - threadId: 'disc-9', - commentId: '601', - applied: true, - warnings: [], - }); - }); - - it('anchors LEFT-side GitLab comments with old_line instead of new_line', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: '101', - fullName: 'acme/backend', - htmlUrl: 'https://gitlab.com/acme/backend', - }); - const fetchImpl = vi - .fn() - .mockResolvedValueOnce( - jsonResponse({ - diff_refs: { - base_sha: 'base1', - start_sha: 'start1', - head_sha: 'head1', - }, - }), - ) - .mockResolvedValueOnce( - jsonResponse([ - { old_path: 'src/index.ts', new_path: 'src/index.ts' }, - ]), - ) - .mockResolvedValueOnce(jsonResponse({ id: 'disc-9' }, 201)); - - await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'gitlab', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 42, - path: 'src/index.ts', - line: 17, - side: 'LEFT', - body: 'This deletion drops the retry path.', - sourceControlProvider: 'gitlab', - }, - fetchImpl, - }); - - const discussionBody = JSON.parse( - (fetchImpl.mock.calls[2]?.[1] as { body: string }).body, - ) as { position: Record }; - expect(discussionBody.position.old_line).toBe(17); - expect(discussionBody.position.new_line).toBeUndefined(); - }); - - it('resolves the real old_path for renamed files from the merge request diffs', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: '101', - fullName: 'acme/backend', - htmlUrl: 'https://gitlab.com/acme/backend', - }); - const fetchImpl = vi - .fn() - .mockResolvedValueOnce( - jsonResponse({ - diff_refs: { - base_sha: 'base1', - start_sha: 'start1', - head_sha: 'head1', - }, - }), - ) - .mockResolvedValueOnce( - jsonResponse([ - { old_path: 'src/legacy/index.ts', new_path: 'src/index.ts' }, - ]), - ) - .mockResolvedValueOnce(jsonResponse({ id: 'disc-9' }, 201)); - - await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'gitlab', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 42, - path: 'src/index.ts', - line: 42, - body: 'Missing error handling here.', - sourceControlProvider: 'gitlab', - }, - fetchImpl, - }); - - const discussionBody = JSON.parse( - (fetchImpl.mock.calls[2]?.[1] as { body: string }).body, - ) as { position: Record }; - expect(discussionBody.position.new_path).toBe('src/index.ts'); - expect(discussionBody.position.old_path).toBe('src/legacy/index.ts'); - }); - - it('keeps scanning diff pages until the renamed file is found', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: '101', - fullName: 'acme/backend', - htmlUrl: 'https://gitlab.com/acme/backend', - }); - const fillerPage = Array.from({ length: 100 }, (_, i) => ({ - old_path: `src/other-${i}.ts`, - new_path: `src/other-${i}.ts`, - })); - const fetchImpl = vi - .fn() - .mockResolvedValueOnce( - jsonResponse({ - diff_refs: { - base_sha: 'base1', - start_sha: 'start1', - head_sha: 'head1', - }, - }), - ) - .mockResolvedValueOnce(jsonResponse(fillerPage)) - .mockResolvedValueOnce( - jsonResponse([ - { old_path: 'src/legacy/index.ts', new_path: 'src/index.ts' }, - ]), - ) - .mockResolvedValueOnce(jsonResponse({ id: 'disc-9' }, 201)); - - await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'gitlab', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 42, - path: 'src/index.ts', - line: 42, - body: 'Missing error handling here.', - sourceControlProvider: 'gitlab', - }, - fetchImpl, - }); - - expect(fetchImpl).toHaveBeenNthCalledWith( - 3, - 'https://gitlab.com/api/v4/projects/101/merge_requests/42/diffs?page=2&per_page=100', - expect.objectContaining({ method: 'GET' }), - ); - const discussionBody = JSON.parse( - (fetchImpl.mock.calls[3]?.[1] as { body: string }).body, - ) as { position: Record }; - expect(discussionBody.position.old_path).toBe('src/legacy/index.ts'); - }); - - it('surfaces an explicit warning when the diff scan backstop ends before the listing', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: '101', - fullName: 'acme/backend', - htmlUrl: 'https://gitlab.com/acme/backend', - }); - const fillerPage = Array.from({ length: 100 }, (_, i) => ({ - old_path: `src/other-${i}.ts`, - new_path: `src/other-${i}.ts`, - })); - const fetchImpl = vi.fn().mockImplementation(async (url: string) => { - if (url.includes('/diffs')) { - return jsonResponse(fillerPage); - } - if (url.endsWith('/discussions')) { - return jsonResponse({ id: 'disc-9' }, 201); - } - return jsonResponse({ - diff_refs: { - base_sha: 'base1', - start_sha: 'start1', - head_sha: 'head1', - }, - }); - }); - - const result = await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'gitlab', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 42, - path: 'src/index.ts', - line: 42, - body: 'Missing error handling here.', - sourceControlProvider: 'gitlab', - }, - fetchImpl, - }); - - const diffCalls = fetchImpl.mock.calls.filter(([url]) => - String(url).includes('/diffs'), - ); - expect(diffCalls).toHaveLength(50); - expect(result.warnings).toEqual([ - expect.stringContaining( - 'rename resolution fell back to the request path', - ), - ]); - }); - - it('falls back to the same-path pair when the diff listing is unavailable', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: '101', - fullName: 'acme/backend', - htmlUrl: 'https://gitlab.com/acme/backend', - }); - const fetchImpl = vi - .fn() - .mockResolvedValueOnce( - jsonResponse({ - diff_refs: { - base_sha: 'base1', - start_sha: 'start1', - head_sha: 'head1', - }, - }), - ) - .mockResolvedValueOnce(jsonResponse({ message: 'nope' }, 500)) - .mockResolvedValueOnce(jsonResponse({ id: 'disc-9' }, 201)); - - await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'gitlab', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 42, - path: 'src/index.ts', - line: 42, - body: 'Missing error handling here.', - sourceControlProvider: 'gitlab', - }, - fetchImpl, - }); - - const discussionBody = JSON.parse( - (fetchImpl.mock.calls[2]?.[1] as { body: string }).body, - ) as { position: Record }; - expect(discussionBody.position.new_path).toBe('src/index.ts'); - expect(discussionBody.position.old_path).toBe('src/index.ts'); - }); - - it('maps a GitLab 400 on discussions to a retryable anchor rejection', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: '101', - fullName: 'acme/backend', - htmlUrl: 'https://gitlab.com/acme/backend', - }); - const fetchImpl = vi - .fn() - .mockResolvedValueOnce( - jsonResponse({ - diff_refs: { - base_sha: 'base1', - start_sha: 'start1', - head_sha: 'head1', - }, - }), - ) - .mockResolvedValueOnce( - jsonResponse([ - { old_path: 'src/index.ts', new_path: 'src/index.ts' }, - ]), - ) - .mockResolvedValueOnce( - jsonResponse({ message: 'line_code must be a valid line code' }, 400), - ); - - await expect( - writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'gitlab', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 42, - path: 'src/index.ts', - line: 9999, - body: 'Missing error handling here.', - sourceControlProvider: 'gitlab', - }, - fetchImpl, - }), - ).rejects.toMatchObject({ - name: 'SourceControlWriteError', - httpStatus: 422, - message: expect.stringContaining( - 'target a line changed in this merge request', - ), - }); - }); - - it('reports missing GitLab diff_refs as a retryable 409', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: '101', - fullName: 'acme/backend', - htmlUrl: 'https://gitlab.com/acme/backend', - }); - const fetchImpl = vi - .fn() - .mockResolvedValueOnce(jsonResponse({ diff_refs: null })); - - await expect( - writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'gitlab', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 42, - path: 'src/index.ts', - line: 42, - body: 'Missing error handling here.', - sourceControlProvider: 'gitlab', - }, - fetchImpl, - }), - ).rejects.toMatchObject({ - name: 'SourceControlWriteError', - httpStatus: 409, - }); - expect(fetchImpl).toHaveBeenCalledTimes(1); - }); - - it('degrades a GitLab multi-line range to the end line with a warning', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: '101', - fullName: 'acme/backend', - htmlUrl: 'https://gitlab.com/acme/backend', - }); - const fetchImpl = vi - .fn() - .mockResolvedValueOnce( - jsonResponse({ - diff_refs: { - base_sha: 'base1', - start_sha: 'start1', - head_sha: 'head1', - }, - }), - ) - .mockResolvedValueOnce( - jsonResponse([ - { old_path: 'src/index.ts', new_path: 'src/index.ts' }, - ]), - ) - .mockResolvedValueOnce(jsonResponse({ id: 'disc-9' }, 201)); - - const result = await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'gitlab', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 42, - path: 'src/index.ts', - startLine: 40, - line: 42, - body: 'This whole block can be simplified.', - sourceControlProvider: 'gitlab', - }, - fetchImpl, - }); - - expect(result).toMatchObject({ - applied: true, - warnings: [ - 'GitLab does not support multi-line comment positions through this surface; the comment is anchored to line 42.', - ], - }); - }); - - it('posts a Gitea single-comment review with a positioned comment', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: null, - fullName: 'acme/backend', - htmlUrl: 'https://git.example.com/acme/backend', - }); - const fetchImpl = vi.fn().mockResolvedValueOnce( - jsonResponse( - { - id: 71, - html_url: 'https://git.example.com/acme/backend/pulls/9#review-71', - }, - 201, - ), - ); - - const result = await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'gitea', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 9, - path: 'src/index.ts', - line: 42, - body: 'Missing error handling here.', - sourceControlProvider: 'gitea', - }, - fetchImpl, - }); - - expect(fetchImpl).toHaveBeenCalledWith( - 'https://git.example.com/api/v1/repos/acme/backend/pulls/9/reviews', - expect.objectContaining({ - method: 'POST', - body: JSON.stringify({ - event: 'COMMENT', - body: '', - comments: [ - { - path: 'src/index.ts', - body: 'Missing error handling here.', - new_position: 42, - }, - ], - }), - }), - ); - expect(result).toMatchObject({ - success: true, - provider: 'gitea', - threadId: '71', - url: 'https://git.example.com/acme/backend/pulls/9#review-71', - applied: true, - warnings: [], - }); - }); - - it('maps a Gitea 422 to a retryable anchor rejection', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: null, - fullName: 'acme/backend', - htmlUrl: 'https://git.example.com/acme/backend', - }); - const fetchImpl = vi - .fn() - .mockResolvedValueOnce( - jsonResponse({ message: 'position is invalid' }, 422), - ); - - await expect( - writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'gitea', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 9, - path: 'src/index.ts', - line: 9999, - body: 'Missing error handling here.', - sourceControlProvider: 'gitea', - }, - fetchImpl, - }), - ).rejects.toMatchObject({ - name: 'SourceControlWriteError', - httpStatus: 422, - message: expect.stringContaining('rejected the inline comment anchor'), - }); - }); - - it('posts a Bitbucket inline comment anchored with to on the destination side', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: null, - fullName: 'acme/backend', - htmlUrl: 'https://bitbucket.org/acme/backend', - }); - const fetchImpl = vi.fn().mockResolvedValueOnce( - jsonResponse( - { - id: 88, - links: { - html: { - href: 'https://bitbucket.org/acme/backend/pull-requests/5#comment-88', - }, - }, - }, - 201, - ), - ); - - const result = await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'bitbucket', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 5, - path: 'src/index.ts', - line: 42, - body: 'Missing error handling here.', - sourceControlProvider: 'bitbucket', - }, - fetchImpl, - }); - - const [url, request] = fetchImpl.mock.calls[0] as [ - string, - { method: string; body: string }, - ]; - expect(url).toContain('/pullrequests/5/comments'); - expect(JSON.parse(request.body)).toEqual({ - content: { raw: 'Missing error handling here.' }, - inline: { path: 'src/index.ts', to: 42 }, - }); - expect(result).toMatchObject({ - success: true, - provider: 'bitbucket', - threadId: '88', - commentId: '88', - applied: true, - warnings: [], - }); - }); - - it('anchors LEFT-side Bitbucket comments with from instead of to', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: null, - fullName: 'acme/backend', - htmlUrl: 'https://bitbucket.org/acme/backend', - }); - const fetchImpl = vi - .fn() - .mockResolvedValueOnce(jsonResponse({ id: 89 }, 201)); - - await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'bitbucket', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 5, - path: 'src/index.ts', - line: 17, - side: 'LEFT', - body: 'This deletion drops the retry path.', - sourceControlProvider: 'bitbucket', - }, - fetchImpl, - }); - - const request = fetchImpl.mock.calls[0]?.[1] as { body: string }; - expect(JSON.parse(request.body)).toMatchObject({ - inline: { path: 'src/index.ts', from: 17 }, - }); - }); - - it('creates an Azure DevOps thread with a right-side file range including startLine', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: 'repo-uuid', - fullName: 'acme/Platform/backend', - htmlUrl: 'https://dev.azure.com/acme/Platform/_git/backend', - }); - const fetchImpl = vi - .fn() - .mockResolvedValueOnce( - jsonResponse({ id: 31, comments: [{ id: 1 }] }, 200), - ); - - const result = await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/Platform/backend', - sourceControlProvider: 'ado', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/Platform/backend', - prNumber: 7, - path: 'src/index.ts', - startLine: 40, - line: 42, - body: 'This whole block can be simplified.', - sourceControlProvider: 'ado', - }, - fetchImpl, - }); - - expect(fetchImpl).toHaveBeenCalledWith( - 'https://dev.azure.com/acme/Platform/_apis/git/repositories/repo-uuid/pullrequests/7/threads?api-version=7.1', - expect.objectContaining({ - method: 'POST', - body: JSON.stringify({ - comments: [ - { - content: 'This whole block can be simplified.', - commentType: 'text', - }, - ], - status: 'active', - threadContext: { - filePath: '/src/index.ts', - rightFileStart: { line: 40, offset: 1 }, - rightFileEnd: { line: 42, offset: 1 }, - }, - }), - }), - ); - expect(result).toMatchObject({ - success: true, - provider: 'ado', - threadId: '31', - commentId: '1', - applied: true, - warnings: [], - }); - }); - - it('anchors LEFT-side Azure DevOps comments with leftFileStart and leftFileEnd', async () => { - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: null, - externalRepoId: 'repo-uuid', - fullName: 'acme/Platform/backend', - htmlUrl: 'https://dev.azure.com/acme/Platform/_git/backend', - }); - const fetchImpl = vi - .fn() - .mockResolvedValueOnce(jsonResponse({ id: 32 }, 200)); - - await writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/Platform/backend', - sourceControlProvider: 'ado', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/Platform/backend', - prNumber: 7, - path: 'src/index.ts', - line: 17, - side: 'LEFT', - body: 'This deletion drops the retry path.', - sourceControlProvider: 'ado', - }, - fetchImpl, - }); - - const request = fetchImpl.mock.calls[0]?.[1] as { body: string }; - expect(JSON.parse(request.body)).toMatchObject({ - threadContext: { - filePath: '/src/index.ts', - leftFileStart: { line: 17, offset: 1 }, - leftFileEnd: { line: 17, offset: 1 }, - }, - }); - }); - - it('rejects requests without a path, line, or body before touching the database', async () => { - const fetchImpl = vi.fn(); - const base = { - action: 'create_pull_request_review_comment' as const, - repositoryFullName: 'acme/backend', - prNumber: 1, - sourceControlProvider: 'github' as const, - }; - const taskRun = makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'github', - }); - - await expect( - writeSourceControlPullRequestForTaskRun({ - taskRun, - input: { ...base, line: 42, body: 'x' }, - fetchImpl, - }), - ).rejects.toThrow( - 'path is required for create_pull_request_review_comment.', - ); - await expect( - writeSourceControlPullRequestForTaskRun({ - taskRun, - input: { ...base, path: 'src/index.ts', body: 'x' }, - fetchImpl, - }), - ).rejects.toThrow( - 'line is required for create_pull_request_review_comment.', - ); - await expect( - writeSourceControlPullRequestForTaskRun({ - taskRun, - input: { ...base, path: 'src/index.ts', line: 42 }, - fetchImpl, - }), - ).rejects.toThrow( - 'body is required for create_pull_request_review_comment.', - ); - expect(fetchImpl).not.toHaveBeenCalled(); - expect(mockRepositoriesFindFirst).not.toHaveBeenCalled(); - }); - - it('rejects a startLine greater than line', async () => { - await expect( - writeSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ - repo: 'acme/backend', - sourceControlProvider: 'github', - }), - input: { - action: 'create_pull_request_review_comment', - repositoryFullName: 'acme/backend', - prNumber: 1, - path: 'src/index.ts', - startLine: 50, - line: 42, - body: 'x', - sourceControlProvider: 'github', - }, - }), - ).rejects.toThrow('startLine must not be greater than line'); - }); - }); - it('rejects resolve requests without a threadId before touching the database', async () => { const fetchImpl = vi.fn(); diff --git a/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-ado-writes.ts b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-ado-writes.ts new file mode 100644 index 000000000..c3a31b35c --- /dev/null +++ b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-ado-writes.ts @@ -0,0 +1,58 @@ +import { z } from 'zod'; +import { requestSourceControlJson as requestJson } from './source-control-pull-request-http'; +import { + buildApiUrl, + type FetchImpl, +} from './source-control-pull-request-shared'; + +export const ADO_API_VERSION = '7.1'; + +export const adoCreatedCommentSchema = z + .object({ id: z.number().int().optional() }) + .passthrough(); + +export const adoThreadSchema = z + .object({ + id: z.number().int(), + status: z.string().nullable().optional(), + comments: z.array(adoCreatedCommentSchema).optional(), + }) + .passthrough(); + +export async function createAdoCommentThread({ + fetchImpl, + tokenHeader, + organizationApiBaseUrl, + threadsPath, + content, + threadContext, +}: { + fetchImpl: FetchImpl; + tokenHeader: { name: string; value: string }; + organizationApiBaseUrl: string; + threadsPath: string; + content: string; + threadContext?: Record; +}): Promise> { + return requestJson({ + fetchImpl, + method: 'POST', + url: buildApiUrl(organizationApiBaseUrl, threadsPath, { + 'api-version': ADO_API_VERSION, + }), + tokenHeader, + body: { + comments: [{ content, commentType: 'text' }], + status: 'active', + ...(threadContext ? { threadContext } : {}), + }, + schema: adoThreadSchema, + }); +} + +export function getFirstAdoCommentId( + thread: z.infer, +): string | null { + const id = thread.comments?.[0]?.id; + return id != null ? String(id) : null; +} diff --git a/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-review-comments.ts b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-review-comments.ts new file mode 100644 index 000000000..04a0bb359 --- /dev/null +++ b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-review-comments.ts @@ -0,0 +1,565 @@ +import { type getOctokit } from '@roomote/github'; +import { + getSourceControlProviderLabel, + type SourceControlProvider, +} from '@roomote/types'; +import { z } from 'zod'; +import { + createAdoCommentThread, + getFirstAdoCommentId, +} from './source-control-pull-request-ado-writes'; +import { SourceControlWriteError } from './source-control-pull-request-write-errors'; +import { + buildSourceControlRequestFailureMessage, + performSourceControlRequest as performRequest, + requestSourceControlJson as requestJson, +} from './source-control-pull-request-http'; +import { + buildApiUrl, + formatResponseBody, + type FetchImpl, + type RepositoryRow, +} from './source-control-pull-request-shared'; +import type { + SourceControlPullRequestWriteInput, + SourceControlPullRequestWriteResult, +} from './source-control-pull-request-writes'; + +type ReviewCommentInput = SourceControlPullRequestWriteInput; + +const gitLabMergeRequestDiffRefsSchema = z + .object({ + diff_refs: z + .object({ + base_sha: z.string().nullable().optional(), + start_sha: z.string().nullable().optional(), + head_sha: z.string().nullable().optional(), + }) + .nullable() + .optional(), + }) + .passthrough(); + +const gitLabNoteSchema = z + .object({ id: z.union([z.number(), z.string()]) }) + .passthrough(); + +const gitLabDiscussionSchema = z + .object({ + id: z.union([z.string(), z.number()]), + notes: z.array(gitLabNoteSchema).optional(), + }) + .passthrough(); + +const gitLabMergeRequestDiffEntrySchema = z + .object({ + old_path: z.string().optional(), + new_path: z.string().optional(), + }) + .passthrough(); + +const giteaCreatedReviewSchema = z + .object({ + id: z.number().int(), + html_url: z.string().optional(), + }) + .passthrough(); + +const bitbucketCreatedCommentSchema = z + .object({ + id: z.union([z.number(), z.string()]).optional(), + links: z + .object({ + html: z.object({ href: z.string().optional() }).optional(), + }) + .optional(), + }) + .passthrough(); + +export function assertReviewCommentInputFields( + input: SourceControlPullRequestWriteInput, +): asserts input is ReviewCommentInput { + requirePath(input); + requireLine(input); + requireBody(input); +} + +export async function createGitHubPullRequestReviewComment({ + input, + repository, + octokit, + owner, + repo, +}: { + input: ReviewCommentInput; + repository: RepositoryRow; + octokit: ReturnType; + owner: string; + repo: string; +}): Promise { + const path = requirePath(input); + const line = requireLine(input); + const side = resolveSide(input); + const body = requireBody(input); + // Resolve the head at write time so moved pull requests cannot create an + // outdated comment from a caller-supplied commit. + const { data: pullRequest } = await octokit.rest.pulls.get({ + owner, + repo, + pull_number: input.prNumber, + }); + + try { + const { data } = await octokit.rest.pulls.createReviewComment({ + owner, + repo, + pull_number: input.prNumber, + commit_id: pullRequest.head.sha, + path, + line, + side, + ...(input.startLine !== undefined + ? { + start_line: input.startLine, + start_side: input.startSide ?? side, + } + : {}), + body, + }); + + return buildReviewCommentResult({ + input, + provider: 'github', + repository, + commentId: String(data.id), + url: data.html_url ?? null, + }); + } catch (error) { + if (getHttpErrorStatus(error) === 422) { + throw anchorRejectionError( + 'github', + input, + error instanceof Error ? error.message : String(error), + ); + } + + throw error; + } +} + +export async function createGitLabPullRequestReviewComment({ + input, + repository, + fetchImpl, + apiBaseUrl, + tokenHeader, + mergeRequestPath, +}: { + input: ReviewCommentInput; + repository: RepositoryRow; + fetchImpl: FetchImpl; + apiBaseUrl: string; + tokenHeader: { name: string; value: string }; + mergeRequestPath: string; +}): Promise { + const path = requirePath(input); + const line = requireLine(input); + const side = resolveSide(input); + // GitLab positions require the merge request's current diff SHA triple. + const mergeRequest = await requestJson({ + fetchImpl, + url: buildApiUrl(apiBaseUrl, mergeRequestPath, {}), + tokenHeader, + schema: gitLabMergeRequestDiffRefsSchema, + acceptedStatuses: [200], + }); + const diffRefs = mergeRequest.diff_refs; + + if (!diffRefs?.base_sha || !diffRefs.start_sha || !diffRefs.head_sha) { + throw new SourceControlWriteError( + 409, + 'GitLab has not computed diff refs for this merge request yet; retry shortly or carry the finding in the review summary comment instead.', + ); + } + + const positionPaths = await resolveGitLabPositionPaths({ + fetchImpl, + apiBaseUrl, + tokenHeader, + mergeRequestPath, + path, + }); + const response = await performRequest({ + fetchImpl, + method: 'POST', + url: buildApiUrl(apiBaseUrl, `${mergeRequestPath}/discussions`, {}), + tokenHeader, + body: { + body: requireBody(input), + position: { + position_type: 'text', + base_sha: diffRefs.base_sha, + start_sha: diffRefs.start_sha, + head_sha: diffRefs.head_sha, + new_path: positionPaths.newPath, + old_path: positionPaths.oldPath, + ...(side === 'RIGHT' ? { new_line: line } : { old_line: line }), + }, + }, + }); + + if (response.status === 400) { + throw anchorRejectionError( + 'gitlab', + input, + `GitLab could not map the position onto the merge request diff${await formatResponseBody(response)}; target a line changed in this merge request${ + positionPaths.warnings.length + ? `. ${positionPaths.warnings.join(' ')}` + : '' + }`, + ); + } + + if (![200, 201].includes(response.status)) { + throw new Error(await buildSourceControlRequestFailureMessage(response)); + } + + const discussion = gitLabDiscussionSchema.parse(await response.json()); + const firstNote = discussion.notes?.[0]; + + return buildReviewCommentResult({ + input, + provider: 'gitlab', + repository, + threadId: String(discussion.id), + commentId: firstNote ? String(firstNote.id) : null, + warnings: [ + ...positionPaths.warnings, + ...multiLineRangeWarnings('gitlab', input), + ], + }); +} + +export async function createGiteaPullRequestReviewComment({ + input, + repository, + fetchImpl, + apiBaseUrl, + owner, + repo, + tokenHeader, +}: { + input: ReviewCommentInput; + repository: RepositoryRow; + fetchImpl: FetchImpl; + apiBaseUrl: string; + owner: string; + repo: string; + tokenHeader: { name: string; value: string }; +}): Promise { + const line = requireLine(input); + const side = resolveSide(input); + // Gitea creates a review containing one positioned comment and anchors it + // against the latest diff when commit_id is omitted. + const response = await performRequest({ + fetchImpl, + method: 'POST', + url: buildApiUrl( + apiBaseUrl, + `/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/pulls/${input.prNumber}/reviews`, + {}, + ), + tokenHeader, + body: { + event: 'COMMENT', + body: '', + comments: [ + { + path: requirePath(input), + body: requireBody(input), + ...(side === 'RIGHT' + ? { new_position: line } + : { old_position: line }), + }, + ], + }, + }); + + if (response.status === 422) { + throw anchorRejectionError( + 'gitea', + input, + `Gitea rejected the review position${await formatResponseBody(response)}`, + ); + } + + if (![200, 201].includes(response.status)) { + throw new Error(await buildSourceControlRequestFailureMessage(response)); + } + + const review = giteaCreatedReviewSchema.parse(await response.json()); + return buildReviewCommentResult({ + input, + provider: 'gitea', + repository, + threadId: String(review.id), + url: review.html_url ?? null, + warnings: multiLineRangeWarnings('gitea', input), + }); +} + +export async function createBitbucketPullRequestReviewComment({ + input, + repository, + fetchImpl, + commentsUrl, + tokenHeader, +}: { + input: ReviewCommentInput; + repository: RepositoryRow; + fetchImpl: FetchImpl; + commentsUrl: string; + tokenHeader: { name: string; value: string }; +}): Promise { + const line = requireLine(input); + const side = resolveSide(input); + // Bitbucket uses destination (`to`) and source (`from`) line anchors. + const comment = await requestJson({ + fetchImpl, + method: 'POST', + url: commentsUrl, + tokenHeader, + body: { + content: { raw: requireBody(input) }, + inline: { + path: requirePath(input), + ...(side === 'RIGHT' ? { to: line } : { from: line }), + }, + }, + schema: bitbucketCreatedCommentSchema, + }); + const commentId = + comment.id === undefined || comment.id === null ? null : String(comment.id); + + return buildReviewCommentResult({ + input, + provider: 'bitbucket', + repository, + threadId: commentId, + commentId, + url: comment.links?.html?.href ?? null, + warnings: multiLineRangeWarnings('bitbucket', input), + }); +} + +export async function createAdoPullRequestReviewComment({ + input, + repository, + fetchImpl, + tokenHeader, + organizationApiBaseUrl, + threadsPath, +}: { + input: ReviewCommentInput; + repository: RepositoryRow; + fetchImpl: FetchImpl; + tokenHeader: { name: string; value: string }; + organizationApiBaseUrl: string; + threadsPath: string; +}): Promise { + const line = requireLine(input); + const side = resolveSide(input); + const path = requirePath(input); + const start = { line: input.startLine ?? line, offset: 1 }; + const end = { line, offset: 1 }; + const thread = await createAdoCommentThread({ + fetchImpl, + tokenHeader, + organizationApiBaseUrl, + threadsPath, + content: requireBody(input), + threadContext: { + filePath: path.startsWith('/') ? path : `/${path}`, + ...(side === 'RIGHT' + ? { rightFileStart: start, rightFileEnd: end } + : { leftFileStart: start, leftFileEnd: end }), + }, + }); + + return buildReviewCommentResult({ + input, + provider: 'ado', + repository, + threadId: String(thread.id), + commentId: getFirstAdoCommentId(thread), + }); +} + +function requirePath(input: SourceControlPullRequestWriteInput): string { + if (!input.path) { + throw new SourceControlWriteError( + 400, + `path is required for ${input.action}.`, + ); + } + return input.path; +} + +function requireLine(input: SourceControlPullRequestWriteInput): number { + if (input.line === undefined) { + throw new SourceControlWriteError( + 400, + `line is required for ${input.action}.`, + ); + } + if (input.startLine !== undefined && input.startLine > input.line) { + throw new SourceControlWriteError( + 400, + `startLine must not be greater than line (got startLine=${input.startLine}, line=${input.line}); line is the end of the range.`, + ); + } + return input.line; +} + +function requireBody(input: SourceControlPullRequestWriteInput): string { + if (!input.body) { + throw new SourceControlWriteError( + 400, + `body is required for ${input.action}.`, + ); + } + return input.body; +} + +function resolveSide( + input: SourceControlPullRequestWriteInput, +): 'LEFT' | 'RIGHT' { + return input.side ?? 'RIGHT'; +} + +function anchorRejectionError( + provider: SourceControlProvider, + input: SourceControlPullRequestWriteInput, + detail: string, +): SourceControlWriteError { + return new SourceControlWriteError( + 422, + `${getSourceControlProviderLabel(provider)} rejected the inline comment anchor (path=${input.path}, line=${input.line}, side=${resolveSide(input)}): ${detail}. The anchor must target a line in the current pull request diff; re-check the hunk and retry once with a corrected anchor, or carry the finding in the review summary comment instead.`, + ); +} + +function getHttpErrorStatus(error: unknown): number | undefined { + if ( + typeof error === 'object' && + error !== null && + 'status' in error && + typeof (error as { status: unknown }).status === 'number' + ) { + return (error as { status: number }).status; + } + return undefined; +} + +function multiLineRangeWarnings( + provider: SourceControlProvider, + input: SourceControlPullRequestWriteInput, +): string[] { + if (input.startLine === undefined) { + return []; + } + return [ + `${getSourceControlProviderLabel(provider)} does not support multi-line comment positions through this surface; the comment is anchored to line ${input.line}.`, + ]; +} + +async function resolveGitLabPositionPaths({ + fetchImpl, + apiBaseUrl, + tokenHeader, + mergeRequestPath, + path, +}: { + fetchImpl: FetchImpl; + apiBaseUrl: string; + tokenHeader: { name: string; value: string }; + mergeRequestPath: string; + path: string; +}): Promise<{ oldPath: string; newPath: string; warnings: string[] }> { + // Renamed files require their distinct old and new paths. Scan all diff + // pages with a defensive cap before falling back to the requested path. + const maxPages = 50; + const perPage = 100; + + for (let page = 1; page <= maxPages; page++) { + const response = await performRequest({ + fetchImpl, + url: buildApiUrl(apiBaseUrl, `${mergeRequestPath}/diffs`, { + page, + per_page: perPage, + }), + tokenHeader, + }); + if (response.status !== 200) { + break; + } + + const entries = z + .array(gitLabMergeRequestDiffEntrySchema) + .parse(await response.json()); + const entry = entries.find( + (candidate) => candidate.new_path === path || candidate.old_path === path, + ); + if (entry) { + return { + oldPath: entry.old_path ?? path, + newPath: entry.new_path ?? path, + warnings: [], + }; + } + if (page === maxPages && entries.length === perPage) { + return { + oldPath: path, + newPath: path, + warnings: [ + `The merge request diff listing exceeded ${maxPages * perPage} files before ${path} was found; rename resolution fell back to the request path, so an anchor on a renamed file may be rejected.`, + ], + }; + } + if (entries.length < perPage) { + break; + } + } + + return { oldPath: path, newPath: path, warnings: [] }; +} + +function buildReviewCommentResult({ + input, + provider, + repository, + threadId = null, + commentId = null, + url = null, + warnings = [], +}: { + input: ReviewCommentInput; + provider: SourceControlProvider; + repository: RepositoryRow; + threadId?: string | null; + commentId?: string | null; + url?: string | null; + warnings?: string[]; +}): SourceControlPullRequestWriteResult { + return { + success: true, + action: input.action, + provider, + repositoryFullName: repository.fullName, + number: input.prNumber, + threadId, + commentId, + url, + applied: true, + warnings, + }; +} diff --git a/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-write-errors.ts b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-write-errors.ts new file mode 100644 index 000000000..a0b2f4430 --- /dev/null +++ b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-write-errors.ts @@ -0,0 +1,14 @@ +/** + * Mirrors SourceControlReadError in source-control-pull-request-reads.ts so + * write callers can map client-addressable failures to HTTP statuses the same + * way the read and mutation surfaces do. + */ +export class SourceControlWriteError extends Error { + constructor( + public readonly httpStatus: number, + message: string, + ) { + super(message); + this.name = 'SourceControlWriteError'; + } +} diff --git a/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-writes.ts b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-writes.ts index 69c87253f..7e4cd4fea 100644 --- a/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-writes.ts +++ b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-writes.ts @@ -7,6 +7,14 @@ import { type SourceControlProvider, } from '@roomote/types'; import { z } from 'zod'; +import { + ADO_API_VERSION, + adoCreatedCommentSchema, + adoThreadSchema, + createAdoCommentThread, + getFirstAdoCommentId, +} from './source-control-pull-request-ado-writes'; +import { SourceControlWriteError } from './source-control-pull-request-write-errors'; import { buildSourceControlRequestFailureMessage, performSourceControlRequest as performRequest, @@ -18,6 +26,14 @@ import { resolveGiteaProviderContext, resolveGitLabProviderContext, } from './source-control-pull-request-provider-context'; +import { + assertReviewCommentInputFields, + createAdoPullRequestReviewComment, + createBitbucketPullRequestReviewComment, + createGiteaPullRequestReviewComment, + createGitHubPullRequestReviewComment, + createGitLabPullRequestReviewComment, +} from './source-control-pull-request-review-comments'; import { assertRepositoryInTaskRunScope, buildAdoBasicAuthHeader, @@ -33,7 +49,6 @@ import { type RepositoryRow, } from './source-control-pull-request-shared'; -const ADO_API_VERSION = '7.1'; // `/_apis/connectionData` is a preview-only resource: Azure DevOps answers // plain `7.1` (and `7.0`) with a 400 demanding the `-preview` suffix. const ADO_CONNECTION_DATA_API_VERSION = '7.1-preview'; @@ -136,20 +151,7 @@ export type SourceControlPullRequestWriteResult = { warnings: string[]; }; -/** - * Mirrors SourceControlReadError in source-control-pull-request-reads.ts so - * write callers can map client-addressable failures to HTTP statuses the same - * way the read and mutation surfaces do. - */ -export class SourceControlWriteError extends Error { - constructor( - public readonly httpStatus: number, - message: string, - ) { - super(message); - this.name = 'SourceControlWriteError'; - } -} +export { SourceControlWriteError } from './source-control-pull-request-write-errors'; const GITHUB_REVIEW_EVENTS = { approve: 'APPROVE', @@ -195,33 +197,6 @@ const gitLabNoteSchema = z .object({ id: z.union([z.number(), z.string()]) }) .passthrough(); -const gitLabMergeRequestDiffRefsSchema = z - .object({ - diff_refs: z - .object({ - base_sha: z.string().nullable().optional(), - start_sha: z.string().nullable().optional(), - head_sha: z.string().nullable().optional(), - }) - .nullable() - .optional(), - }) - .passthrough(); - -const gitLabDiscussionSchema = z - .object({ - id: z.union([z.string(), z.number()]), - notes: z.array(gitLabNoteSchema).optional(), - }) - .passthrough(); - -const gitLabMergeRequestDiffEntrySchema = z - .object({ - old_path: z.string().optional(), - new_path: z.string().optional(), - }) - .passthrough(); - const giteaCreatedCommentSchema = z .object({ id: z.number().int(), @@ -247,18 +222,6 @@ const bitbucketCreatedCommentSchema = z }) .passthrough(); -const adoCreatedCommentSchema = z - .object({ id: z.number().int().optional() }) - .passthrough(); - -const adoThreadSchema = z - .object({ - id: z.number().int(), - status: z.string().nullable().optional(), - comments: z.array(adoCreatedCommentSchema).optional(), - }) - .passthrough(); - const adoConnectionDataSchema = z .object({ authenticatedUser: z.object({ id: z.string() }).passthrough(), @@ -366,9 +329,7 @@ function assertWriteInputFields( requireBody(input); break; case 'create_pull_request_review_comment': - requirePath(input); - requireLine(input); - requireBody(input); + assertReviewCommentInputFields(input); break; case 'update_pull_request_comment': requireCommentId(input); @@ -417,84 +378,6 @@ function requireBody(input: SourceControlPullRequestWriteInput): string { return input.body; } -function requirePath(input: SourceControlPullRequestWriteInput): string { - if (!input.path) { - throw new SourceControlWriteError( - 400, - `path is required for ${input.action}.`, - ); - } - - return input.path; -} - -function requireLine(input: SourceControlPullRequestWriteInput): number { - if (input.line === undefined) { - throw new SourceControlWriteError( - 400, - `line is required for ${input.action}.`, - ); - } - - if (input.startLine !== undefined && input.startLine > input.line) { - throw new SourceControlWriteError( - 400, - `startLine must not be greater than line (got startLine=${input.startLine}, line=${input.line}); line is the end of the range.`, - ); - } - - return input.line; -} - -function resolveSide( - input: SourceControlPullRequestWriteInput, -): 'LEFT' | 'RIGHT' { - return input.side ?? 'RIGHT'; -} - -/** - * The provider could not map the requested anchor onto the current PR diff. - * Surfaced as a 422 error (not an applied:false capability gap) so the agent - * can correct the anchor and retry, or fall back to the summary comment. - */ -function anchorRejectionError( - provider: SourceControlProvider, - input: SourceControlPullRequestWriteInput, - detail: string, -): SourceControlWriteError { - return new SourceControlWriteError( - 422, - `${getSourceControlProviderLabel(provider)} rejected the inline comment anchor (path=${input.path}, line=${input.line}, side=${resolveSide(input)}): ${detail}. The anchor must target a line in the current pull request diff; re-check the hunk and retry once with a corrected anchor, or carry the finding in the review summary comment instead.`, - ); -} - -/** Reads the HTTP status carried by errors such as octokit's RequestError. */ -function getHttpErrorStatus(error: unknown): number | undefined { - if ( - typeof error === 'object' && - error !== null && - 'status' in error && - typeof (error as { status: unknown }).status === 'number' - ) { - return (error as { status: number }).status; - } - - return undefined; -} - -function multiLineRangeWarnings( - provider: SourceControlProvider, - input: SourceControlPullRequestWriteInput, -): string[] { - if (input.startLine === undefined) { - return []; - } - - return [ - `${getSourceControlProviderLabel(provider)} does not support multi-line comment positions through this surface; the comment is anchored to line ${input.line}.`, - ]; -} - function requireResolved(input: SourceControlPullRequestWriteInput): boolean { if (input.resolved === undefined) { throw new SourceControlWriteError( @@ -610,55 +493,13 @@ async function writeGitHubPullRequest({ }); } case 'create_pull_request_review_comment': { - const path = requirePath(input); - const line = requireLine(input); - const side = resolveSide(input); - const body = requireBody(input); - // Anchor against the head SHA resolved at call time: a caller-supplied - // SHA that has since moved would only produce an "outdated" comment or - // a hard 422 with no useful recovery. - const { data: pullRequest } = await octokit.rest.pulls.get({ + return createGitHubPullRequestReviewComment({ + input, + repository, + octokit, owner, repo, - pull_number: input.prNumber, }); - - try { - const { data } = await octokit.rest.pulls.createReviewComment({ - owner, - repo, - pull_number: input.prNumber, - commit_id: pullRequest.head.sha, - path, - line, - side, - ...(input.startLine !== undefined - ? { - start_line: input.startLine, - start_side: input.startSide ?? side, - } - : {}), - body, - }); - - return buildWriteResult({ - input, - provider, - repository, - commentId: String(data.id), - url: data.html_url ?? null, - }); - } catch (error) { - if (getHttpErrorStatus(error) === 422) { - throw anchorRejectionError( - provider, - input, - error instanceof Error ? error.message : String(error), - ); - } - - throw error; - } } case 'update_pull_request_comment': { const commentId = requireCommentId(input); @@ -824,88 +665,13 @@ async function writeGitLabMergeRequest({ }); } case 'create_pull_request_review_comment': { - const path = requirePath(input); - const line = requireLine(input); - const side = resolveSide(input); - const body = requireBody(input); - // Diff positions must carry the merge request's diff_refs SHA triple. - const mergeRequest = await requestJson({ - fetchImpl, - url: buildApiUrl(apiBaseUrl, mergeRequestPath, {}), - tokenHeader, - schema: gitLabMergeRequestDiffRefsSchema, - acceptedStatuses: [200], - }); - const diffRefs = mergeRequest.diff_refs; - - if (!diffRefs?.base_sha || !diffRefs.start_sha || !diffRefs.head_sha) { - throw new SourceControlWriteError( - 409, - 'GitLab has not computed diff refs for this merge request yet; retry shortly or carry the finding in the review summary comment instead.', - ); - } - - const positionPaths = await resolveGitLabPositionPaths({ + return createGitLabPullRequestReviewComment({ + input, + repository, fetchImpl, apiBaseUrl, tokenHeader, mergeRequestPath, - path, - }); - - const response = await performRequest({ - fetchImpl, - method: 'POST', - url: buildApiUrl(apiBaseUrl, `${mergeRequestPath}/discussions`, {}), - tokenHeader, - body: { - body, - position: { - position_type: 'text', - base_sha: diffRefs.base_sha, - start_sha: diffRefs.start_sha, - head_sha: diffRefs.head_sha, - new_path: positionPaths.newPath, - old_path: positionPaths.oldPath, - ...(side === 'RIGHT' ? { new_line: line } : { old_line: line }), - }, - }, - }); - - // GitLab answers 400 for positions it cannot map onto the current diff - // (including unchanged context lines, which need both old and new line - // numbers to anchor). - if (response.status === 400) { - throw anchorRejectionError( - provider, - input, - `GitLab could not map the position onto the merge request diff${await formatResponseBody(response)}; target a line changed in this merge request${ - positionPaths.warnings.length - ? `. ${positionPaths.warnings.join(' ')}` - : '' - }`, - ); - } - - if (![200, 201].includes(response.status)) { - throw new Error( - await buildSourceControlRequestFailureMessage(response), - ); - } - - const discussion = gitLabDiscussionSchema.parse(await response.json()); - const firstNote = discussion.notes?.[0]; - - return buildWriteResult({ - input, - provider, - repository, - threadId: String(discussion.id), - commentId: firstNote ? String(firstNote.id) : null, - warnings: [ - ...positionPaths.warnings, - ...multiLineRangeWarnings(provider, input), - ], }); } case 'update_pull_request_comment': { @@ -1103,81 +869,6 @@ async function submitGitLabReview({ }); } -/** - * GitLab positions must carry the file's real old_path and new_path; for - * renamed files they differ and a same-path position is rejected. Scan the - * merge request diff list for the entry matching the requested path by either - * name, falling back to the same-path pair when the file cannot be found. - * When the runaway backstop ends the scan before the listing does, the - * fallback is surfaced explicitly through `warnings`. - */ -async function resolveGitLabPositionPaths({ - fetchImpl, - apiBaseUrl, - tokenHeader, - mergeRequestPath, - path, -}: { - fetchImpl: FetchImpl; - apiBaseUrl: string; - tokenHeader: { name: string; value: string }; - mergeRequestPath: string; - path: string; -}): Promise<{ oldPath: string; newPath: string; warnings: string[] }> { - // Scan the complete diff listing (a page shorter than per_page ends it). - // GitLab's own diff rendering hard-caps merge requests around 3,000 - // changed files, so this backstop is unreachable in practice and exists - // only as a runaway guard against a misbehaving server. - const maxPages = 50; - const perPage = 100; - - for (let page = 1; page <= maxPages; page++) { - const response = await performRequest({ - fetchImpl, - url: buildApiUrl(apiBaseUrl, `${mergeRequestPath}/diffs`, { - page, - per_page: perPage, - }), - tokenHeader, - }); - - if (response.status !== 200) { - break; - } - - const entries = z - .array(gitLabMergeRequestDiffEntrySchema) - .parse(await response.json()); - const entry = entries.find( - (candidate) => candidate.new_path === path || candidate.old_path === path, - ); - - if (entry) { - return { - oldPath: entry.old_path ?? path, - newPath: entry.new_path ?? path, - warnings: [], - }; - } - - if (page === maxPages && entries.length === perPage) { - return { - oldPath: path, - newPath: path, - warnings: [ - `The merge request diff listing exceeded ${maxPages * perPage} files before ${path} was found; rename resolution fell back to the request path, so an anchor on a renamed file may be rejected.`, - ], - }; - } - - if (entries.length < perPage) { - break; - } - } - - return { oldPath: path, newPath: path, warnings: [] }; -} - async function createGitLabNote({ fetchImpl, apiBaseUrl, @@ -1270,60 +961,14 @@ async function writeGiteaPullRequest({ }); } case 'create_pull_request_review_comment': { - const path = requirePath(input); - const line = requireLine(input); - const side = resolveSide(input); - const body = requireBody(input); - // Gitea's review API is GitHub-shaped: one review with a single - // positioned comment. commit_id is omitted so Gitea anchors against - // the latest diff. - const response = await performRequest({ - fetchImpl, - method: 'POST', - url: buildApiUrl( - apiBaseUrl, - `/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/pulls/${input.prNumber}/reviews`, - {}, - ), - tokenHeader, - body: { - event: 'COMMENT', - body: '', - comments: [ - { - path, - body, - ...(side === 'RIGHT' - ? { new_position: line } - : { old_position: line }), - }, - ], - }, - }); - - if (response.status === 422) { - throw anchorRejectionError( - provider, - input, - `Gitea rejected the review position${await formatResponseBody(response)}`, - ); - } - - if (![200, 201].includes(response.status)) { - throw new Error( - await buildSourceControlRequestFailureMessage(response), - ); - } - - const review = giteaCreatedReviewSchema.parse(await response.json()); - - return buildWriteResult({ + return createGiteaPullRequestReviewComment({ input, - provider, repository, - threadId: String(review.id), - url: review.html_url ?? null, - warnings: multiLineRangeWarnings(provider, input), + fetchImpl, + apiBaseUrl, + owner, + repo, + tokenHeader, }); } case 'update_pull_request_comment': { @@ -1464,40 +1109,12 @@ async function writeBitbucketPullRequest({ }); } case 'create_pull_request_review_comment': { - const path = requirePath(input); - const line = requireLine(input); - const side = resolveSide(input); - // Bitbucket anchors inline comments by destination (`to`) or source - // (`from`) line and does not validate the anchor against the diff; - // out-of-diff anchors render under "Other comments" instead of erroring. - const comment = await requestJson({ - fetchImpl, - method: 'POST', - url: commentsUrl, - tokenHeader, - body: { - content: { raw: requireBody(input) }, - inline: { - path, - ...(side === 'RIGHT' ? { to: line } : { from: line }), - }, - }, - schema: bitbucketCreatedCommentSchema, - }); - const commentId = - comment.id === undefined || comment.id === null - ? null - : String(comment.id); - - return buildWriteResult({ + return createBitbucketPullRequestReviewComment({ input, - provider, repository, - // The read surface keys Bitbucket threads by the top comment id. - threadId: commentId, - commentId, - url: comment.links?.html?.href ?? null, - warnings: multiLineRangeWarnings(provider, input), + fetchImpl, + commentsUrl, + tokenHeader, }); } case 'update_pull_request_comment': { @@ -1711,34 +1328,13 @@ async function writeAdoPullRequest({ }); } case 'create_pull_request_review_comment': { - const path = requirePath(input); - const line = requireLine(input); - const side = resolveSide(input); - const startLine = input.startLine ?? line; - const start = { line: startLine, offset: 1 }; - const end = { line, offset: 1 }; - // ADO does not validate threadContext against the diff; anchors outside - // the diff render as file-level comments instead of erroring. - const thread = await createAdoCommentThread({ + return createAdoPullRequestReviewComment({ + input, + repository, fetchImpl, tokenHeader, organizationApiBaseUrl, threadsPath, - content: requireBody(input), - threadContext: { - filePath: path.startsWith('/') ? path : `/${path}`, - ...(side === 'RIGHT' - ? { rightFileStart: start, rightFileEnd: end } - : { leftFileStart: start, leftFileEnd: end }), - }, - }); - - return buildWriteResult({ - input, - provider, - repository, - threadId: String(thread.id), - commentId: getFirstAdoCommentId(thread), }); } case 'update_pull_request_comment': { @@ -1876,42 +1472,3 @@ async function writeAdoPullRequest({ } } } - -async function createAdoCommentThread({ - fetchImpl, - tokenHeader, - organizationApiBaseUrl, - threadsPath, - content, - threadContext, -}: { - fetchImpl: FetchImpl; - tokenHeader: { name: string; value: string }; - organizationApiBaseUrl: string; - threadsPath: string; - content: string; - /** File/line anchor for inline review comments; omit for PR-level threads. */ - threadContext?: Record; -}): Promise> { - return requestJson({ - fetchImpl, - method: 'POST', - url: buildApiUrl(organizationApiBaseUrl, threadsPath, { - 'api-version': ADO_API_VERSION, - }), - tokenHeader, - body: { - comments: [{ content, commentType: 'text' }], - status: 'active', - ...(threadContext ? { threadContext } : {}), - }, - schema: adoThreadSchema, - }); -} - -function getFirstAdoCommentId( - thread: z.infer, -): string | null { - const id = thread.comments?.[0]?.id; - return id != null ? String(id) : null; -}