From b8a0aa02420b694e57ca8cc705a2f7b3444bfbe6 Mon Sep 17 00:00:00 2001 From: bot-hermes Date: Thu, 13 Aug 2026 17:20:30 +0000 Subject: [PATCH] feat(web): resolve walkthrough diffs for GitLab merge requests --- .../server/lib/walkthrough/DOCUMENTATION.md | 12 +- .../server/lib/walkthrough/pull-request.js | 82 +++++++++++++- .../lib/walkthrough/pull-request.test.js | 107 ++++++++++++++++++ 3 files changed, 197 insertions(+), 4 deletions(-) diff --git a/packages/web/server/lib/walkthrough/DOCUMENTATION.md b/packages/web/server/lib/walkthrough/DOCUMENTATION.md index 1d0cc404..0ace904b 100644 --- a/packages/web/server/lib/walkthrough/DOCUMENTATION.md +++ b/packages/web/server/lib/walkthrough/DOCUMENTATION.md @@ -20,7 +20,7 @@ has to ask for it. `PROMPT_VERSION`. - `schema.js` — response schema, response normalization, tolerant JSON parsing. - `store.js` — content-addressed cache entries plus mutable pointers. -- `pull-request.js` — PR diffs via the shared GitHub octokit helper. +- `pull-request.js` — PR/MR diffs; the provider (GitHub octokit or GitLab REST client) is chosen by the git remote. - `model-settings.js` — the feature's own model override. - `languages.js` — the languages the prose may be written in. - `index.js` — orchestration. @@ -53,7 +53,15 @@ written against staged code never silently re-anchors onto an unstaged edit. |---|---|---| | `working-tree` (`all` \| `staged` \| `working`) | `staged`, `working` | Untracked files are fetched individually because `git diff` omits them | | `branch` | `branch` | `getRangeDiff` uses three-dot `base...head`, so work merged in from the base branch is excluded | -| `pr` | `pr:` | GitHub returns the merge-base diff, matching the branch semantics | +| `pr` | `pr:` | GitHub returns the merge-base diff; a GitLab remote concatenates the merge request's diffs instead — both match the branch semantics | + +Provider selection lives in `pull-request.js`: the directory's git remote +decides. A GitLab remote resolves through `resolveGitLabRepoFromDirectory` and +fetches `merge_requests/:iid/diffs` pages (capped at 10), concatenating the +per-file diffs into one patch; anything else falls back to the GitHub pull +request API. A GitLab directory without a connected GitLab account fails with +`401 gitlab-not-connected`, and an MR with no diffs fails with +`404 empty-diff`, the same code an empty GitHub PR uses. For the current-branch source, the UI takes the base from the default branch of the current branch's tracking remote (`defaultBranches` in the branches diff --git a/packages/web/server/lib/walkthrough/pull-request.js b/packages/web/server/lib/walkthrough/pull-request.js index e05a5f51..39d0ce2c 100644 --- a/packages/web/server/lib/walkthrough/pull-request.js +++ b/packages/web/server/lib/walkthrough/pull-request.js @@ -1,14 +1,20 @@ import { getOctokitOrNull } from '../github/octokit.js'; import { resolveGitHubRepoFromDirectory } from '../github/repo/index.js'; +import { getGitLabClientOrNull } from '../gitlab/client.js'; +import { resolveGitLabRepoFromDirectory } from '../gitlab/repo.js'; + +// GitLab diff pagination cap: never loop more than 10 pages of 100 files, +// mirroring gitlab/routes.js. +const GITLAB_DIFFS_MAX_PAGES = 10; /** - * Raw unified diff for a pull request. + * Raw unified diff for a GitHub pull request. * * GitHub already returns the merge-base diff for a PR, so this matches the * three-dot semantics used for local branch reviews: work merged in from the * base branch is not part of it. */ -export async function getPullRequestDiff(directory, number) { +async function getGitHubPullRequestDiff(directory, number) { const octokit = getOctokitOrNull(); if (!octokit) { throw Object.assign(new Error('Connect a GitHub account to review pull requests'), { @@ -44,3 +50,75 @@ export async function getPullRequestDiff(directory, number) { return { patch, meta: { owner: repo.owner, repo: repo.repo, number } }; } + +/** + * Raw unified diff for a GitLab merge request. + * + * GitLab's merge request diffs endpoint returns one entry per file, so the + * pages are concatenated into a single patch. `repo` comes from the + * dispatcher's `resolveGitLabRepoFromDirectory` call and is never re-resolved + * here. + */ +async function getGitLabMergeRequestDiff(repo, number) { + const client = getGitLabClientOrNull(); + if (!client) { + throw Object.assign(new Error('Connect a GitLab account to review merge requests'), { + statusCode: 401, + code: 'gitlab-not-connected', + }); + } + + // The parser always populates both fields; this guards a malformed repo so + // the failure is explicit rather than a downstream TypeError. + if (!repo?.namespace || !repo?.project) { + throw Object.assign(new Error('This directory has no GitLab remote'), { + statusCode: 400, + code: 'no-gitlab-remote', + }); + } + + // The client URL-encodes the path internally; never pre-encode it. + const projectPath = `${repo.namespace}/${repo.project}`; + + const diffs = []; + for (let page = 1; page <= GITLAB_DIFFS_MAX_PAGES; page += 1) { + const response = await client.mergeRequestDiffs(projectPath, number, { per_page: 100, page }); + if (response.status !== 200 || !Array.isArray(response.data)) { + break; + } + diffs.push(...response.data); + // The page object is the authoritative signal; the 10-page cap above is + // what stops a server that lies about hasMore from looping forever. + if (!response.page?.hasMore) { + break; + } + } + + const patch = diffs + .map((item) => (typeof item?.diff === 'string' ? item.diff : '')) + .filter(Boolean) + .join('\n'); + if (!patch.trim()) { + throw Object.assign(new Error(`Merge request #${number} has no diff`), { + statusCode: 404, + code: 'empty-diff', + }); + } + + return { patch, meta: { namespace: repo.namespace, project: repo.project, number } }; +} + +/** + * Raw unified diff for a pull request or merge request. + * + * The provider is chosen by the repository's git remote: a GitLab remote uses + * the GitLab merge request API, anything else falls back to the GitHub pull + * request API. + */ +export async function getPullRequestDiff(directory, number) { + const { repo } = await resolveGitLabRepoFromDirectory(directory); + if (repo) { + return getGitLabMergeRequestDiff(repo, number); + } + return getGitHubPullRequestDiff(directory, number); +} diff --git a/packages/web/server/lib/walkthrough/pull-request.test.js b/packages/web/server/lib/walkthrough/pull-request.test.js index 6a76a337..b2cdabcd 100644 --- a/packages/web/server/lib/walkthrough/pull-request.test.js +++ b/packages/web/server/lib/walkthrough/pull-request.test.js @@ -2,10 +2,14 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; vi.mock('../github/octokit.js', () => ({ getOctokitOrNull: vi.fn() })); vi.mock('../github/repo/index.js', () => ({ resolveGitHubRepoFromDirectory: vi.fn() })); +vi.mock('../gitlab/client.js', () => ({ getGitLabClientOrNull: vi.fn() })); +vi.mock('../gitlab/repo.js', () => ({ resolveGitLabRepoFromDirectory: vi.fn() })); const { getPullRequestDiff } = await import('./pull-request.js'); const { getOctokitOrNull } = await import('../github/octokit.js'); const { resolveGitHubRepoFromDirectory } = await import('../github/repo/index.js'); +const { getGitLabClientOrNull } = await import('../gitlab/client.js'); +const { resolveGitLabRepoFromDirectory } = await import('../gitlab/repo.js'); const PATCH = `diff --git a/src/a.ts b/src/a.ts --- a/src/a.ts @@ -14,6 +18,28 @@ const PATCH = `diff --git a/src/a.ts b/src/a.ts +const added = true; `; +const GITLAB_DIFF_ONE = `diff --git a/a.txt b/a.txt +--- a/a.txt ++++ b/a.txt +@@ -1,1 +1,2 @@ ++hello +`; + +const GITLAB_DIFF_TWO = `diff --git a/b.txt b/b.txt +--- a/b.txt ++++ b/b.txt +@@ -1 +1,2 @@ ++world +`; + +const GITLAB_REPO = { + namespace: 'acme', + project: 'widgets', + host: 'gitlab.com', + baseUrl: 'https://gitlab.com', + url: 'https://gitlab.com/acme/widgets', +}; + describe('getPullRequestDiff', () => { let request; @@ -27,6 +53,10 @@ describe('getPullRequestDiff', () => { repo: { owner: 'openchamber', repo: 'openchamber' }, remoteUrl: 'git@github.com:openchamber/openchamber.git', }); + // Default to a non-GitLab directory so the GitHub cases keep routing to + // the GitHub path. + resolveGitLabRepoFromDirectory.mockResolvedValue({ repo: null, remoteUrl: null }); + getGitLabClientOrNull.mockReturnValue(null); }); afterEach(() => { @@ -74,4 +104,81 @@ describe('getPullRequestDiff', () => { statusCode: 404, }); }); + + describe('with a GitLab repository', () => { + let mergeRequestDiffs; + + beforeEach(() => { + mergeRequestDiffs = vi.fn().mockResolvedValue({ + status: 200, + data: [{ diff: GITLAB_DIFF_ONE }], + page: { page: 1, next: null, total: 1, hasMore: false }, + }); + getGitLabClientOrNull.mockReturnValue({ mergeRequestDiffs }); + resolveGitLabRepoFromDirectory.mockResolvedValue({ + repo: GITLAB_REPO, + remoteUrl: 'git@gitlab.com:acme/widgets.git', + }); + }); + + it('concatenates merge request diffs across pages into a single patch', async () => { + mergeRequestDiffs + .mockResolvedValueOnce({ + status: 200, + data: [{ diff: GITLAB_DIFF_ONE }], + page: { page: 1, next: 2, total: 2, hasMore: true }, + }) + .mockResolvedValueOnce({ + status: 200, + data: [{ diff: GITLAB_DIFF_TWO }], + page: { page: 2, next: null, total: 2, hasMore: false }, + }); + + const result = await getPullRequestDiff('/repo', 7); + + expect(result.patch).toBe(`${GITLAB_DIFF_ONE}\n${GITLAB_DIFF_TWO}`); + expect(result.meta).toEqual({ namespace: 'acme', project: 'widgets', number: 7 }); + // The unencoded namespace/project path is passed to the client, which + // URL-encodes it internally. + expect(mergeRequestDiffs).toHaveBeenCalledTimes(2); + expect(mergeRequestDiffs).toHaveBeenNthCalledWith(1, 'acme/widgets', 7, { per_page: 100, page: 1 }); + expect(mergeRequestDiffs).toHaveBeenNthCalledWith(2, 'acme/widgets', 7, { per_page: 100, page: 2 }); + }); + + it('asks the user to connect GitLab before fetching diffs', async () => { + getGitLabClientOrNull.mockReturnValue(null); + + await expect(getPullRequestDiff('/repo', 7)).rejects.toMatchObject({ + code: 'gitlab-not-connected', + statusCode: 401, + }); + expect(mergeRequestDiffs).not.toHaveBeenCalled(); + }); + + it('treats an MR with no diff as missing rather than an empty review', async () => { + mergeRequestDiffs.mockResolvedValue({ + status: 200, + data: [{ diff: ' ' }], + page: { page: 1, next: null, total: 1, hasMore: false }, + }); + + await expect(getPullRequestDiff('/repo', 7)).rejects.toMatchObject({ + code: 'empty-diff', + statusCode: 404, + }); + }); + + it('reports a GitLab repo without namespace or project as having no remote', async () => { + resolveGitLabRepoFromDirectory.mockResolvedValue({ + repo: { namespace: '', project: '' }, + remoteUrl: 'git@gitlab.com:acme/widgets.git', + }); + + await expect(getPullRequestDiff('/repo', 7)).rejects.toMatchObject({ + code: 'no-gitlab-remote', + statusCode: 400, + }); + expect(mergeRequestDiffs).not.toHaveBeenCalled(); + }); + }); });