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

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

Two bugs caused `MergeRequestDiscussions.create` to return 400 Bad Request for a subset of inline comments.

---

### Missing `oldPath` on modified files

GitLab's Discussions API requires both `oldPath` and `newPath` in the position object for any file that exists in both the base and head commits. Previously only `newPath` was set, causing GitLab to reject comments on modified files. `oldPath` is now always included, falling back to `filename` when no rename is present.

### Missing `oldLine` for context (unchanged) lines

GitLab requires `oldLine` in addition to `newLine` when the target line is a context line (unchanged in the diff). Without it the API returns 400 for those lines while accepting added-line comments — explaining why only *some* comments in a given MR failed.

The fix parses `oldSnippet`/`newSnippet` from the diff payload to build a new→old line number map. Context lines appear at the same index in both snippets, so zipping them gives the mapping. `oldLine` is only included when the line is confirmed to be a context line; added lines continue to use `newLine` only.

### Rename support

`oldFilename` is now propagated through `generatePrReview.ts` → `sourcebot_file_diff_review` → `gitlabPushMrReviews`, so renamed files correctly pass the old path as `oldPath`.

---

### Files changed

| File | Change |
|------|--------|
| `types.ts` | Added `oldFilename?: string` to `sourcebot_file_diff_review_schema` |
| `generatePrReview.ts` | Sets `oldFilename: file_diff.from` when building review objects |
| `gitlabPushMrReviews.ts` | Adds `oldPath`, context-line `oldLine` mapping via `buildContextLineMap` |
| `gitlabPushMrReviews.test.ts` | Adds `oldPath` to position assertion; new tests for renames and context vs added line behaviour |

* Add CHANGELOG entry

* Address Coderabbit review comments

* fix(web): fix type error in gitlabPushMrReviews position object

Spread oldPath/newPath as optional object entries instead of passing
string | undefined, which is not assignable to the gitbeaker
DiscussionNotePositionOptions type.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(web): fix DiscussionNotePosition type error in gitlabPushMrReviews

Spreading union types (`{} | { oldPath: string }`) into an object literal
causes TypeScript to widen the result to a union that doesn't satisfy the
gitbeaker discriminated union type. Also, passing `string | undefined` for
optional position properties fails due to the `Record<string, unknown>` base
type camelizing to `Record<string, {}>`, which excludes `undefined`.

Fix by building the position as a `Record<string, string>` imperatively
(only ever string values, properties conditionally added) and casting to the
position type derived from the `Gitlab` client via `Parameters<>` — no
`as any` and no direct import from `@gitbeaker/core`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(web): always set oldPath/newPath in GitLab MR discussion position

GitLab requires both oldPath and newPath in the position object. Previously
they were omitted when the path was '/dev/null', breaking inline positioning
for added and deleted files.

Now for added files (old path is /dev/null) both are set to the new path,
and for deleted files (new path is /dev/null) both are set to the old path.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* refactor(web): remove Record<string,string> cast in gitlabPushMrReviews

Now that oldPath/newPath are always plain strings, declare position
directly as DiscussionNotePosition and use a single conditional spread
for the optional oldLine field. Removes the redundant
`as unknown as DiscussionNotePosition` cast.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Update CHANGELOG.md

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Brendan Kellam <brendan@sourcebot.dev>

Author: 3 people

CHANGELOG.md

@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 - Upgraded `protobufjs` to `^7.6.2`. [#1281](https://github.com/sourcebot-dev/sourcebot/pull/1281)
+- Fixed GitLab MR inline review comments returning 400 Bad Request on context (unchanged) lines and renamed files. [#1149](https://github.com/sourcebot-dev/sourcebot/pull/1149)
 
 ## [5.0.1] - 2026-06-04
 

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,7 +1,7 @@
 import { expect, test, vi, describe } from 'vitest';
 import { Gitlab } from '@gitbeaker/rest';
 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';
 
 type GitlabClient = InstanceType<typeof Gitlab>;
 
@@ -67,6 +67,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',
                 }),
@@ -187,4 +188,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>;