diff --git a/CHANGELOG.md b/CHANGELOG.md index 295ca08c8..f483d7a61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- Fixed GitLab MR inline review comments returning 400 Bad Request on context (unchanged) lines and renamed files. The position object now always includes `oldPath`, and `oldLine` is included alongside `newLine` for context lines by parsing the diff snippets to map new→old line numbers. [#1149](https://github.com/sourcebot-dev/sourcebot/pull/1149) + ## [4.16.15] - 2026-04-23 ### Changed diff --git a/packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts b/packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts index 4732049a3..afea4fb5e 100644 --- a/packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts +++ b/packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts @@ -42,6 +42,7 @@ export const generatePrReviews = async (reviewAgentLogFileName: string | undefin if (reviews.length > 0) { file_diff_reviews.push({ filename: file_diff.to, + oldFilename: file_diff.from, reviews: reviews, }); } diff --git a/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts b/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts index b26ecc53b..ce7435318 100644 --- a/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts +++ b/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts @@ -1,6 +1,6 @@ import { expect, test, vi, describe } from 'vitest'; import { gitlabPushMrReviews } from './gitlabPushMrReviews'; -import { sourcebot_pr_payload, sourcebot_file_diff_review } from '../types'; +import { sourcebot_pr_payload, sourcebot_file_diff_review, sourcebot_diff_refs } from '../types'; vi.mock('@sourcebot/shared', () => ({ createLogger: () => ({ @@ -64,6 +64,7 @@ describe('gitlabPushMrReviews', () => { baseSha: 'base_sha_value', headSha: 'head_sha_value', startSha: 'start_sha_value', + oldPath: 'src/foo.ts', newPath: 'src/foo.ts', newLine: '5', }), @@ -184,4 +185,142 @@ describe('gitlabPushMrReviews', () => { gitlabPushMrReviews(client, 101, MOCK_PAYLOAD, SINGLE_REVIEW), ).resolves.not.toThrow(); }); + + test('uses oldFilename as oldPath when file was renamed', async () => { + const renamedReview: sourcebot_file_diff_review[] = [ + { + filename: 'src/new-name.ts', + oldFilename: 'src/old-name.ts', + reviews: [{ line_start: 1, line_end: 1, review: 'Comment on renamed file' }], + }, + ]; + const client = makeMockClient(); + + await gitlabPushMrReviews(client, 101, MOCK_PAYLOAD, renamedReview); + + expect(client.MergeRequestDiscussions.create).toHaveBeenCalledWith( + 101, + 42, + 'Comment on renamed file', + expect.objectContaining({ + position: expect.objectContaining({ + oldPath: 'src/old-name.ts', + newPath: 'src/new-name.ts', + }), + }), + ); + }); + + test('passes oldLine for context lines', async () => { + // old line 47 = new line 48 (a line was added at new line 47 before it) + const payloadWithDiffs: sourcebot_pr_payload = { + ...MOCK_PAYLOAD, + file_diffs: [{ + from: 'src/foo.ts', + to: 'src/foo.ts', + diffs: [{ + oldSnippet: '@@ -47,1 +48,1 @@\n47: context line\n', + newSnippet: '@@ -47,1 +48,1 @@\n48: context line\n', + }], + }], + }; + const contextReview: sourcebot_file_diff_review[] = [ + { + filename: 'src/foo.ts', + reviews: [{ line_start: 48, line_end: 48, review: 'Context line comment' }], + }, + ]; + const client = makeMockClient(); + + await gitlabPushMrReviews(client, 101, payloadWithDiffs, contextReview); + + expect(client.MergeRequestDiscussions.create).toHaveBeenCalledWith( + 101, + 42, + 'Context line comment', + expect.objectContaining({ + position: expect.objectContaining({ + newLine: '48', + oldLine: '47', + }), + }), + ); + }); + + test('does not pass oldLine for added lines', async () => { + const payloadWithDiffs: sourcebot_pr_payload = { + ...MOCK_PAYLOAD, + file_diffs: [{ + from: 'src/foo.ts', + to: 'src/foo.ts', + diffs: [{ + oldSnippet: '@@ -1,1 +1,2 @@\n1: existing line\n', + newSnippet: '@@ -1,1 +1,2 @@\n1: existing line\n2:+added line\n', + }], + }], + }; + const addedLineReview: sourcebot_file_diff_review[] = [ + { + filename: 'src/foo.ts', + reviews: [{ line_start: 2, line_end: 2, review: 'Comment on added line' }], + }, + ]; + const client = makeMockClient(); + + await gitlabPushMrReviews(client, 101, payloadWithDiffs, addedLineReview); + + const call = client.MergeRequestDiscussions.create.mock.calls[0][3]; + expect(call.position).not.toHaveProperty('oldLine'); + expect(call.position.newLine).toBe('2'); + }); + + test('uses new path for both oldPath and newPath when old path is /dev/null (added file)', async () => { + const addedFileReview: sourcebot_file_diff_review[] = [ + { + filename: 'src/new-file.ts', + oldFilename: '/dev/null', + reviews: [{ line_start: 1, line_end: 1, review: 'Comment on new file' }], + }, + ]; + const client = makeMockClient(); + + await gitlabPushMrReviews(client, 101, MOCK_PAYLOAD, addedFileReview); + + expect(client.MergeRequestDiscussions.create).toHaveBeenCalledWith( + 101, + 42, + 'Comment on new file', + expect.objectContaining({ + position: expect.objectContaining({ + oldPath: 'src/new-file.ts', + newPath: 'src/new-file.ts', + }), + }), + ); + }); + + test('uses old path for both oldPath and newPath when new path is /dev/null (deleted file)', async () => { + const deletedFileReview: sourcebot_file_diff_review[] = [ + { + filename: '/dev/null', + oldFilename: 'src/deleted-file.ts', + reviews: [{ line_start: 1, line_end: 1, review: 'Comment on deleted file' }], + }, + ]; + const client = makeMockClient(); + + await gitlabPushMrReviews(client, 101, MOCK_PAYLOAD, deletedFileReview); + + expect(client.MergeRequestDiscussions.create).toHaveBeenCalledWith( + 101, + 42, + 'Comment on deleted file', + expect.objectContaining({ + position: expect.objectContaining({ + oldPath: 'src/deleted-file.ts', + newPath: 'src/deleted-file.ts', + }), + }), + ); + }); }); diff --git a/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts b/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts index ff98ecb3c..0751a0138 100644 --- a/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts +++ b/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts @@ -2,8 +2,61 @@ import { sourcebot_pr_payload, sourcebot_file_diff_review } from "@/features/age import { Gitlab } from "@gitbeaker/rest"; import { createLogger } from "@sourcebot/shared"; +// Derive the position type from the Gitlab client to avoid importing from @gitbeaker/core. +type DiscussionNotePosition = NonNullable< + NonNullable['MergeRequestDiscussions']['create']>[3]>['position'] +>; + const logger = createLogger('gitlab-push-mr-reviews'); +/** + * Extracts new-file line numbers for context (unchanged) lines from a snippet. + * Snippet lines have the format `:` where content starts with + * a space for context lines, `+` for additions, and `-` for deletions. + */ +function extractContextLineNumbers(snippet: string): number[] { + const result: number[] = []; + for (const line of snippet.split('\n')) { + if (line.startsWith('@@')) { + continue; + } + const colonIdx = line.indexOf(':'); + if (colonIdx === -1) { + continue; + } + const lineNum = parseInt(line.substring(0, colonIdx), 10); + if (isNaN(lineNum)) { + continue; + } + const content = line.substring(colonIdx + 1); + if (content.startsWith(' ')) { + result.push(lineNum); + } + } + return result; +} + +/** + * Builds a per-file map from new-file line number → old-file line number for + * context lines. Context lines appear at the same index in oldSnippet and + * newSnippet, so zipping them gives the mapping. + */ +function buildContextLineMap(prPayload: sourcebot_pr_payload): Map> { + const fileMap = new Map>(); + for (const fileDiff of prPayload.file_diffs) { + const lineMap = new Map(); + for (const diff of fileDiff.diffs) { + const oldNums = extractContextLineNumbers(diff.oldSnippet); + const newNums = extractContextLineNumbers(diff.newSnippet); + for (let i = 0; i < Math.min(oldNums.length, newNums.length); i++) { + lineMap.set(newNums[i], oldNums[i]); + } + } + fileMap.set(fileDiff.to, lineMap); + } + return fileMap; +} + export const gitlabPushMrReviews = async ( gitlabClient: InstanceType, projectId: number, @@ -18,24 +71,34 @@ export const gitlabPushMrReviews = async ( } const { base_sha, head_sha, start_sha } = prPayload.diff_refs; + const contextLineMap = buildContextLineMap(prPayload); for (const fileDiffReview of fileDiffReviews) { + const fileContextMap = contextLineMap.get(fileDiffReview.filename); + const resolvedOldPath = fileDiffReview.oldFilename ?? fileDiffReview.filename; + // GitLab requires both oldPath and newPath in the position object. + // For added files (old is /dev/null) use the new path for both; + // for deleted files (new is /dev/null) use the old path for both. + const oldPath = resolvedOldPath !== '/dev/null' ? resolvedOldPath : fileDiffReview.filename; + const newPath = fileDiffReview.filename !== '/dev/null' ? fileDiffReview.filename : resolvedOldPath; for (const review of fileDiffReview.reviews) { + const oldLine = fileContextMap?.get(review.line_end); + const position: DiscussionNotePosition = { + positionType: 'text', + baseSha: base_sha, + headSha: head_sha, + startSha: start_sha, + oldPath, + newPath, + newLine: String(review.line_end), + ...(oldLine !== undefined ? { oldLine: String(oldLine) } : {}), + }; try { await gitlabClient.MergeRequestDiscussions.create( projectId, prPayload.number, review.review, - { - position: { - positionType: "text", - baseSha: base_sha, - headSha: head_sha, - startSha: start_sha, - newPath: fileDiffReview.filename, - newLine: String(review.line_end), - }, - }, + { position }, ); } catch (error) { // Inline comment failed (e.g. line not in diff) — fall back to a general MR note diff --git a/packages/web/src/features/agents/review-agent/types.ts b/packages/web/src/features/agents/review-agent/types.ts index 1a9aba1eb..412a99b93 100644 --- a/packages/web/src/features/agents/review-agent/types.ts +++ b/packages/web/src/features/agents/review-agent/types.ts @@ -52,6 +52,7 @@ export type sourcebot_diff_review = z.infer export const sourcebot_file_diff_review_schema = z.object({ filename: z.string(), + oldFilename: z.string().optional(), reviews: z.array(sourcebot_diff_review_schema), }); export type sourcebot_file_diff_review = z.infer;