authorgravatar for snorteman@gmail.comScott Norteman <snorteman@gmail.com> 2026-10-11 02:45:48-04:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-10-11 01:45:48-05:00
logbf725cb80d79334601c2833cb599524a3669e03b
treed507e70fa1e72b3f6066efee887285c87b4a81ec
parent119e61b6c1ee642fa48b4447812fb77f17b0bfe5
signature Signed by PGP key B5690EEEBB952194

fix(server): GitLab merge requests can expand unchanged lines (#17528)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

2 files changed, 69 insertions(+), 0 deletions(-)

packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts+66
...@@ -212,3 +212,69 @@ describe("rewriting what has already been said", () => {...@@ -212,3 +212,69 @@ describe("rewriting what has already been said", () => {
212 }),212 }),
213 );213 );
214});214});
215
216describe("expanding unchanged lines", () => {
217 const change = {
218 cwd: "/w",
219 repository: "acme/web",
220 host: "gitlab.com",
221 number: 7,
222 changeType: "change" as const,
223 oldPath: "src/page.ts",
224 newPath: "src/page.ts",
225 };
226
227 it.effect("reads both sides of a file through the merge request or one of its commits", () =>
228 Effect.gen(function* () {
229 const getMergeRequestDiffFileContents = vi.fn(() =>
230 Effect.succeed({ oldContents: "before\n", newContents: "after\n" }),
231 );
232 const provider = yield* make.pipe(
233 Effect.provide(
234 Layer.mock(GitLabPullRequestCli.GitLabPullRequestCli)({
235 getMergeRequestDiffFileContents,
236 }),
237 ),
238 );
239 assert.isDefined(provider.getDiffFileContents);
240 const oneCommit = { ...change, commit: "a1b2c3d" };
241
242 expect(yield* provider.getDiffFileContents(change)).toEqual({
243 oldContents: "before\n",
244 newContents: "after\n",
245 });
246 yield* provider.getDiffFileContents(oneCommit);
247 expect(getMergeRequestDiffFileContents).toHaveBeenNthCalledWith(1, change);
248 expect(getMergeRequestDiffFileContents).toHaveBeenNthCalledWith(2, oneCommit);
249 }),
250 );
251
252 it.effect("reports a file GitLab cannot expand as a failed read", () =>
253 Effect.gen(function* () {
254 const provider = yield* make.pipe(
255 Effect.provide(
256 Layer.mock(GitLabPullRequestCli.GitLabPullRequestCli)({
257 getMergeRequestDiffFileContents: () =>
258 Effect.fail(
259 new GitLabPullRequestCli.GitLabDiffFileContentsUnavailableError({
260 command: "glab",
261 cwd: "/w",
262 path: "assets/logo.png",
263 reason: "binary",
264 }),
265 ),
266 }),
267 ),
268 );
269 assert.isDefined(provider.getDiffFileContents);
270
271 expect(yield* Effect.flip(provider.getDiffFileContents(change))).toMatchObject({
272 _tag: "PullRequestProviderError",
273 provider: "gitlab",
274 operation: "getDiffFileContents",
275 reason: "failed",
276 detail: "The diff file 'assets/logo.png' is binary.",
277 });
278 }),
279 );
280});
packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts+3
...@@ -223,6 +223,9 @@ export const make = Effect.gen(function* () {...@@ -223,6 +223,9 @@ export const make = Effect.gen(function* () {
223223
224 getDiff: (input) => cli.getMergeRequestDiff(input).pipe(Effect.mapError(fail("getDiff"))),224 getDiff: (input) => cli.getMergeRequestDiff(input).pipe(Effect.mapError(fail("getDiff"))),
225225
226 getDiffFileContents: (input) =>
227 cli.getMergeRequestDiffFileContents(input).pipe(Effect.mapError(fail("getDiffFileContents"))),
228
226 // What each marked file is at the head, which is what tells a mark that still stands from one229 // What each marked file is at the head, which is what tells a mark that still stands from one
227 // the branch has moved past. GitLab's own local-storage marks are keyed on the blob id too,230 // the branch has moved past. GitLab's own local-storage marks are keyed on the blob id too,
228 // so this stales at the same moment its web UI would.231 // so this stales at the same moment its web UI would.