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 |

Author: Gavin Williams

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,92 @@ 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 and omits it for added 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');
+    });
 });

packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts

@@ -4,6 +4,46 @@ import { createLogger } from "@sourcebot/shared";
 
 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,9 +58,12 @@ 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);
         for (const review of fileDiffReview.reviews) {
+            const oldLine = fileContextMap?.get(review.line_end);
             try {
                 await gitlabClient.MergeRequestDiscussions.create(
                     projectId,
@@ -32,8 +75,10 @@ export const gitlabPushMrReviews = async (
                             baseSha: base_sha,
                             headSha: head_sha,
                             startSha: start_sha,
+                            oldPath: fileDiffReview.oldFilename ?? fileDiffReview.filename,
                             newPath: fileDiffReview.filename,
                             newLine: String(review.line_end),
+                            ...(oldLine !== undefined ? { oldLine: String(oldLine) } : {}),
                         },
                     },
                 );

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>;