From 2afa854323c38df3b57e8177350f3587a2fcfcdf Mon Sep 17 00:00:00 2001 From: Gavin Williams Date: Fri, 24 Apr 2026 12:02:19 +0100 Subject: [PATCH 1/7] fix(web): fix GitLab MR inline comment 400 errors on context lines and renames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | --- .../review-agent/nodes/generatePrReview.ts | 1 + .../nodes/gitlabPushMrReviews.test.ts | 91 ++++++++++++++++++- .../review-agent/nodes/gitlabPushMrReviews.ts | 45 +++++++++ .../src/features/agents/review-agent/types.ts | 1 + 4 files changed, 137 insertions(+), 1 deletion(-) 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..3cfdb8e67 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,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'); + }); }); 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..b948ec9ca 100644 --- a/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts +++ b/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 `:` 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,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) } : {}), }, }, ); 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; From 6c5bc334a65cb714f19547596d6d268198d29932 Mon Sep 17 00:00:00 2001 From: Gavin Williams Date: Fri, 24 Apr 2026 12:08:03 +0100 Subject: [PATCH 2/7] Add CHANGELOG entry --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) 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 From 27039741e1330d51cf6e2d8b5446b8a72944f2a0 Mon Sep 17 00:00:00 2001 From: Gavin Williams Date: Fri, 24 Apr 2026 12:14:15 +0100 Subject: [PATCH 3/7] Address Coderabbit review comments --- .../nodes/gitlabPushMrReviews.test.ts | 2 +- .../review-agent/nodes/gitlabPushMrReviews.ts | 24 ++++++++++++++----- 2 files changed, 19 insertions(+), 7 deletions(-) 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 3cfdb8e67..347e83658 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 @@ -211,7 +211,7 @@ describe('gitlabPushMrReviews', () => { ); }); - test('passes oldLine for context lines and omits it for added lines', async () => { + 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, 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 b948ec9ca..b8b0daaa9 100644 --- a/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts +++ b/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts @@ -12,13 +12,21 @@ const logger = createLogger('gitlab-push-mr-reviews'); function extractContextLineNumbers(snippet: string): number[] { const result: number[] = []; for (const line of snippet.split('\n')) { - if (line.startsWith('@@')) continue; + if (line.startsWith('@@')) { + continue; + } const colonIdx = line.indexOf(':'); - if (colonIdx === -1) continue; + if (colonIdx === -1) { + continue; + } const lineNum = parseInt(line.substring(0, colonIdx), 10); - if (isNaN(lineNum)) continue; + if (isNaN(lineNum)) { + continue; + } const content = line.substring(colonIdx + 1); - if (content.startsWith(' ')) result.push(lineNum); + if (content.startsWith(' ')) { + result.push(lineNum); + } } return result; } @@ -75,8 +83,12 @@ export const gitlabPushMrReviews = async ( baseSha: base_sha, headSha: head_sha, startSha: start_sha, - oldPath: fileDiffReview.oldFilename ?? fileDiffReview.filename, - newPath: fileDiffReview.filename, + oldPath: (fileDiffReview.oldFilename ?? fileDiffReview.filename) !== '/dev/null' + ? (fileDiffReview.oldFilename ?? fileDiffReview.filename) + : undefined, + newPath: fileDiffReview.filename !== '/dev/null' + ? fileDiffReview.filename + : undefined, newLine: String(review.line_end), ...(oldLine !== undefined ? { oldLine: String(oldLine) } : {}), }, From 3da3f91489585b81fd50473e3ebbe3bccad0288f Mon Sep 17 00:00:00 2001 From: Gavin Williams Date: Fri, 24 Apr 2026 12:41:25 +0100 Subject: [PATCH 4/7] 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 --- .../agents/review-agent/nodes/gitlabPushMrReviews.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) 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 b8b0daaa9..c392afc30 100644 --- a/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts +++ b/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts @@ -70,6 +70,9 @@ export const gitlabPushMrReviews = async ( for (const fileDiffReview of fileDiffReviews) { const fileContextMap = contextLineMap.get(fileDiffReview.filename); + const resolvedOldPath = fileDiffReview.oldFilename ?? fileDiffReview.filename; + const oldPathEntry = resolvedOldPath !== '/dev/null' ? { oldPath: resolvedOldPath } : {}; + const newPathEntry = fileDiffReview.filename !== '/dev/null' ? { newPath: fileDiffReview.filename } : {}; for (const review of fileDiffReview.reviews) { const oldLine = fileContextMap?.get(review.line_end); try { @@ -83,12 +86,8 @@ export const gitlabPushMrReviews = async ( baseSha: base_sha, headSha: head_sha, startSha: start_sha, - oldPath: (fileDiffReview.oldFilename ?? fileDiffReview.filename) !== '/dev/null' - ? (fileDiffReview.oldFilename ?? fileDiffReview.filename) - : undefined, - newPath: fileDiffReview.filename !== '/dev/null' - ? fileDiffReview.filename - : undefined, + ...oldPathEntry, + ...newPathEntry, newLine: String(review.line_end), ...(oldLine !== undefined ? { oldLine: String(oldLine) } : {}), }, From 09575702f5b833944354d0c5c5ed6420eef33f40 Mon Sep 17 00:00:00 2001 From: Gavin Williams Date: Fri, 24 Apr 2026 12:49:56 +0100 Subject: [PATCH 5/7] fix(web): fix DiscussionNotePosition type error in gitlabPushMrReviews MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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` base type camelizing to `Record`, which excludes `undefined`. Fix by building the position as a `Record` 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 --- .../review-agent/nodes/gitlabPushMrReviews.ts | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 deletions(-) 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 c392afc30..8ecfc0ee0 100644 --- a/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts +++ b/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts @@ -2,6 +2,11 @@ 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'); /** @@ -71,27 +76,30 @@ export const gitlabPushMrReviews = async ( for (const fileDiffReview of fileDiffReviews) { const fileContextMap = contextLineMap.get(fileDiffReview.filename); const resolvedOldPath = fileDiffReview.oldFilename ?? fileDiffReview.filename; - const oldPathEntry = resolvedOldPath !== '/dev/null' ? { oldPath: resolvedOldPath } : {}; - const newPathEntry = fileDiffReview.filename !== '/dev/null' ? { newPath: fileDiffReview.filename } : {}; for (const review of fileDiffReview.reviews) { const oldLine = fileContextMap?.get(review.line_end); + const position: Record = { + positionType: 'text', + baseSha: base_sha, + headSha: head_sha, + startSha: start_sha, + newLine: String(review.line_end), + }; + if (resolvedOldPath !== '/dev/null') { + position['oldPath'] = resolvedOldPath; + } + if (fileDiffReview.filename !== '/dev/null') { + position['newPath'] = fileDiffReview.filename; + } + if (oldLine !== undefined) { + position['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, - ...oldPathEntry, - ...newPathEntry, - newLine: String(review.line_end), - ...(oldLine !== undefined ? { oldLine: String(oldLine) } : {}), - }, - }, + { position: position as unknown as DiscussionNotePosition }, ); } catch (error) { // Inline comment failed (e.g. line not in diff) — fall back to a general MR note From a57f68f24f2186c1c2c33a2f1ef8c463d88f23c3 Mon Sep 17 00:00:00 2001 From: Gavin Williams Date: Fri, 24 Apr 2026 13:12:14 +0100 Subject: [PATCH 6/7] 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 --- .../nodes/gitlabPushMrReviews.test.ts | 50 +++++++++++++++++++ .../review-agent/nodes/gitlabPushMrReviews.ts | 13 ++--- 2 files changed, 57 insertions(+), 6 deletions(-) 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 347e83658..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 @@ -273,4 +273,54 @@ describe('gitlabPushMrReviews', () => { 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 8ecfc0ee0..d64b8a41c 100644 --- a/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts +++ b/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts @@ -76,6 +76,11 @@ export const gitlabPushMrReviews = async ( 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: Record = { @@ -83,14 +88,10 @@ export const gitlabPushMrReviews = async ( baseSha: base_sha, headSha: head_sha, startSha: start_sha, + oldPath, + newPath, newLine: String(review.line_end), }; - if (resolvedOldPath !== '/dev/null') { - position['oldPath'] = resolvedOldPath; - } - if (fileDiffReview.filename !== '/dev/null') { - position['newPath'] = fileDiffReview.filename; - } if (oldLine !== undefined) { position['oldLine'] = String(oldLine); } From 7653d72130dfeb4e5f06fd444cd2c4d437d29bff Mon Sep 17 00:00:00 2001 From: Gavin Williams Date: Fri, 24 Apr 2026 15:49:45 +0100 Subject: [PATCH 7/7] refactor(web): remove Record 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 --- .../agents/review-agent/nodes/gitlabPushMrReviews.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) 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 d64b8a41c..0751a0138 100644 --- a/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts +++ b/packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts @@ -83,7 +83,7 @@ export const gitlabPushMrReviews = async ( const newPath = fileDiffReview.filename !== '/dev/null' ? fileDiffReview.filename : resolvedOldPath; for (const review of fileDiffReview.reviews) { const oldLine = fileContextMap?.get(review.line_end); - const position: Record = { + const position: DiscussionNotePosition = { positionType: 'text', baseSha: base_sha, headSha: head_sha, @@ -91,16 +91,14 @@ export const gitlabPushMrReviews = async ( oldPath, newPath, newLine: String(review.line_end), + ...(oldLine !== undefined ? { oldLine: String(oldLine) } : {}), }; - if (oldLine !== undefined) { - position['oldLine'] = String(oldLine); - } try { await gitlabClient.MergeRequestDiscussions.create( projectId, prPayload.number, review.review, - { position: position as unknown as DiscussionNotePosition }, + { position }, ); } catch (error) { // Inline comment failed (e.g. line not in diff) — fall back to a general MR note