fix(web): fix GitLab MR inline comment 400 errors on context lines and renames

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

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,
             });
         }

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',
+                }),
+            }),
+        );
+    });
 });

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<Parameters<InstanceType<typeof Gitlab>['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 `<lineNum>:<content>` 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<string, Map<number, number>> {
+    const fileMap = new Map<string, Map<number, number>>();
+    for (const fileDiff of prPayload.file_diffs) {
+        const lineMap = new Map<number, number>();
+        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<typeof Gitlab>,
     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

packages/web/src/features/agents/review-agent/types.ts

@@ -52,6 +52,7 @@ export type sourcebot_diff_review = z.infer<typeof sourcebot_diff_review_schema>
 
 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<typeof sourcebot_file_diff_review_schema>;