From 3f651a63abe498707215cf6a5494b417c655e01d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=BDan=20Mlakar?= <148876659+zanmlakar@users.noreply.github.com> Date: Sat, 17 Oct 2026 04:54:11 +0200 Subject: [PATCH] Fix GitLab label, member and assignee edits through glab (#28776) --- src/main/gitlab/issue-update.ts | 12 +- src/main/gitlab/issues.test.ts | 198 ++++++++++++++---- .../gitlab/project-label-and-member-lookup.ts | 122 ++++++----- 3 files changed, 235 insertions(+), 97 deletions(-) diff --git a/src/main/gitlab/issue-update.ts b/src/main/gitlab/issue-update.ts index 2181b6e8208..4e98dfd8260 100644 --- a/src/main/gitlab/issue-update.ts +++ b/src/main/gitlab/issue-update.ts @@ -62,11 +62,9 @@ export async function updateIssue( ...glabHostEnvOptions(projectRef, connectionId) }) } catch (err) { + // Why: glab exits 0 for an issue already in the target state, so every failure is real. const stderr = err instanceof Error ? err.message : String(err) - // Treat "already closed/reopened" as a no-op (matches gh path). - if (!stderr.toLowerCase().includes('already')) { - errors.push(classifyGlabError(stderr).message) - } + errors.push(classifyGlabError(stderr).message) } finally { release() } @@ -111,12 +109,14 @@ export async function updateIssue( editArgs.push('--unlabel', label) hasEditArgs = true } + // Why: an unprefixed --assignee replaces every assignee and --unassign clears them all; glab + // reads a `+`/`-` prefix as add/remove (since at least v1.22). for (const assignee of updates.addAssignees ?? []) { - editArgs.push('--assignee', assignee) + editArgs.push(`--assignee=+${assignee}`) hasEditArgs = true } for (const assignee of updates.removeAssignees ?? []) { - editArgs.push('--unassignee', assignee) + editArgs.push(`--assignee=-${assignee}`) hasEditArgs = true } diff --git a/src/main/gitlab/issues.test.ts b/src/main/gitlab/issues.test.ts index 29009ee6235..1d4e77a6947 100644 --- a/src/main/gitlab/issues.test.ts +++ b/src/main/gitlab/issues.test.ts @@ -33,10 +33,52 @@ vi.mock('./gl-utils', async () => { } }) +import { parseGlabApiResponse } from './gl-utils' import { addIssueComment, createIssue, getIssue, listIssues } from './issues' import { updateIssue } from './issue-update' import { listAssignableUsers, listLabels } from './project-label-and-member-lookup' +/** Raw `glab api -i` stdout: status line, CRLF header lines, blank line, then the JSON body. */ +function glabIncludeOutput(headers: Record, body: unknown): string { + const headerLines = Object.entries(headers) + .map(([name, value]) => `${name}: ${value}\r\n`) + .join('') + return `HTTP/2.0 200 OK\r\nContent-Type: application/json\r\n${headerLines}\r\n${JSON.stringify(body)}\n` +} + +/** Answers each `glab api -i` call by endpoint through the real header/body parser. */ +function serveGlabApiPages(stdoutByEndpoint: Map): void { + glabApiWithHeadersMock.mockImplementation(async (args: string[]) => { + const stdout = stdoutByEndpoint.get(args.at(-1) ?? '') + if (stdout === undefined) { + throw new Error(`unexpected glab api call: ${args.join(' ')}`) + } + return parseGlabApiResponse(stdout) + }) +} + +/** Mirrors glab's cmdutils.ParseAssignees: `+` adds, `-` or `!` removes, anything else replaces all. */ +function glabAssigneeEdits(args: string[]): { add: string[]; remove: string[]; replace: string[] } { + const add: string[] = [] + const remove: string[] = [] + const replace: string[] = [] + for (const arg of args) { + if (!arg.startsWith('--assignee=')) { + continue + } + for (const value of arg.slice('--assignee='.length).split(',')) { + if (value.startsWith('+')) { + add.push(value.slice(1)) + } else if (value.startsWith('-') || value.startsWith('!')) { + remove.push(value.slice(1)) + } else { + replace.push(value) + } + } + } + return { add, remove, replace } +} + describe('gitlab issue operations', () => { beforeEach(() => { glabExecFileAsyncMock.mockReset() @@ -105,9 +147,13 @@ describe('gitlab issue operations', () => { created_at: '2026-06-16T00:00:00.000Z' }) }) - .mockResolvedValueOnce({ stdout: 'bug\nfrontend\n' }) - .mockResolvedValueOnce({ stdout: '{"id":1,"username":"octo","avatar_url":""}\n' }) - glabApiWithHeadersMock.mockResolvedValueOnce({ body: '[]', headers: {} }) + glabApiWithHeadersMock + .mockResolvedValueOnce({ body: '[]', headers: {} }) + .mockResolvedValueOnce({ body: '[{"id":1,"name":"bug"}]', headers: { 'x-next-page': '' } }) + .mockResolvedValueOnce({ + body: '[{"id":1,"username":"octo","avatar_url":""}]', + headers: { 'x-next-page': '' } + }) await getIssue('/repo-root', 923, null, localGitOptions) await listIssues('/repo-root', 5, undefined, 'opened', undefined, null, localGitOptions) @@ -122,8 +168,12 @@ describe('gitlab issue operations', () => { localGitOptions ) await addIssueComment('/repo-root', 923, 'Comment', undefined, null, null, localGitOptions) - await listLabels('/repo-root', undefined, null, localGitOptions) - await listAssignableUsers('/repo-root', undefined, null, localGitOptions) + await expect(listLabels('/repo-root', undefined, null, localGitOptions)).resolves.toEqual([ + 'bug' + ]) + await expect( + listAssignableUsers('/repo-root', undefined, null, localGitOptions) + ).resolves.toMatchObject([{ username: 'octo' }]) expect(getIssueProjectRefMock).toHaveBeenCalledWith( '/repo-root', @@ -141,6 +191,10 @@ describe('gitlab issue operations', () => { expect(glabExecFileAsyncMock.mock.calls.every((call) => call[1]?.wslDistro === 'Ubuntu')).toBe( true ) + expect(glabApiWithHeadersMock).toHaveBeenCalledTimes(3) + expect(glabApiWithHeadersMock.mock.calls.every((call) => call[1]?.wslDistro === 'Ubuntu')).toBe( + true + ) expect(glabApiWithHeadersMock).toHaveBeenCalledWith( [ 'projects/stablyai%2Forca/issues?page=1&per_page=5&order_by=updated_at&sort=desc&state=opened' @@ -371,11 +425,14 @@ describe('gitlab issue operations', () => { expect(options.env?.GITLAB_HOST).toBe('gitlab.example.internal') }) - it("updateIssue treats 'already closed' as a no-op", async () => { + // Why: `glab issue close` exits 0 for a closed issue, so a failure mentioning "already" is real. + it('updateIssue reports a failed close even when its text says "already"', async () => { getIssueProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'stablyai/orca' }) glabExecFileAsyncMock.mockRejectedValueOnce(new Error('Issue is already closed')) - await expect(updateIssue('/repo-root', 5, { state: 'closed' })).resolves.toEqual({ ok: true }) + await expect(updateIssue('/repo-root', 5, { state: 'closed' })).resolves.toMatchObject({ + ok: false + }) }) it('updateIssue applies field edits via `glab issue update`', async () => { @@ -405,15 +462,32 @@ describe('gitlab issue operations', () => { 'bug', '--unlabel', 'stale', - '--assignee', - 'alice', - '--unassignee', - 'bob' + '--assignee=+alice', + '--assignee=-bob' ], { cwd: '/repo-root' } ) }) + it('updateIssue sends assignee edits glab applies as add/remove, never as a replace', async () => { + getIssueProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'stablyai/orca' }) + glabExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) + + await updateIssue('/repo-root', 5, { + addAssignees: ['alice', 'carol'], + removeAssignees: ['bob'] + }) + + const [args] = glabExecFileAsyncMock.mock.calls[0] + expect(glabAssigneeEdits(args)).toEqual({ + add: ['alice', 'carol'], + remove: ['bob'], + replace: [] + }) + // Why: glab has no --unassignee, and its --unassign clears every assignee. + expect(args.filter((arg: string) => arg.startsWith('--unassign'))).toEqual([]) + }) + it('updateIssue applies body edits via the issue API', async () => { getIssueProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'stablyai/orca' }) glabExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) @@ -428,16 +502,40 @@ describe('gitlab issue operations', () => { ) }) - it('routes issue metadata reads through the selected SSH GitLab host', async () => { + it('walks label and member pages of glab api -i output on the selected SSH GitLab host', async () => { getIssueProjectRefMock .mockResolvedValueOnce({ host: 'git.internal', path: 'stablyai/orca' }) .mockResolvedValueOnce({ host: 'git.internal', path: 'stablyai/orca' }) - glabExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'bug\nfeature\n' }) - .mockResolvedValueOnce({ - stdout: - '{"id":12,"username":"alice","name":"Alice","avatar_url":"https://example.com/a.png","state":"active"}\n' - }) + serveGlabApiPages( + new Map([ + [ + 'projects/stablyai%2Forca/labels?per_page=100&page=1', + glabIncludeOutput({ 'X-Next-Page': '2', 'X-Page': '1', 'X-Total-Pages': '2' }, [ + { id: 1, name: 'bug', color: '#d9534f', is_project_label: true } + ]) + ], + [ + 'projects/stablyai%2Forca/labels?per_page=100&page=2', + glabIncludeOutput({ 'X-Next-Page': '', 'X-Page': '2', 'X-Total-Pages': '2' }, [ + { id: 4, name: 'feature', color: '#f0ad4e', is_project_label: false } + ]) + ], + [ + 'projects/stablyai%2Forca/members/all?per_page=100&page=1', + glabIncludeOutput({ 'X-Next-Page': '', 'X-Page': '1' }, [ + { + id: 12, + username: 'alice', + name: 'Alice', + state: 'active', + avatar_url: 'https://example.com/a.png', + web_url: 'https://git.internal/alice', + access_level: 30 + } + ]) + ] + ]) + ) await expect(listLabels('/repo-root', 'upstream', 'conn-1')).resolves.toEqual([ 'bug', @@ -453,24 +551,54 @@ describe('gitlab issue operations', () => { } ]) - expect(glabExecFileAsyncMock.mock.calls[0][0]).toEqual([ - 'api', - '--hostname', - 'git.internal', - '--paginate', - 'projects/stablyai%2Forca/labels', - '--jq', - '.[].name' - ]) - expect(glabExecFileAsyncMock.mock.calls[1][0]).toEqual([ - 'api', - '--hostname', - 'git.internal', - '--paginate', - 'projects/stablyai%2Forca/members/all?per_page=100', - '--jq', - '.[] | {id, username, name, avatar_url, state}' + expect(glabApiWithHeadersMock.mock.calls.map(([args]) => args)).toEqual([ + ['--hostname', 'git.internal', 'projects/stablyai%2Forca/labels?per_page=100&page=1'], + ['--hostname', 'git.internal', 'projects/stablyai%2Forca/labels?per_page=100&page=2'], + ['--hostname', 'git.internal', 'projects/stablyai%2Forca/members/all?per_page=100&page=1'] ]) + expect(glabExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('probes past a full label page when a proxy strips the pagination headers', async () => { + getIssueProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'stablyai/orca' }) + const fullPage = Array.from({ length: 100 }, (_, index) => ({ id: index, name: `l${index}` })) + serveGlabApiPages( + new Map([ + ['projects/stablyai%2Forca/labels?per_page=100&page=1', glabIncludeOutput({}, fullPage)], + [ + 'projects/stablyai%2Forca/labels?per_page=100&page=2', + glabIncludeOutput({}, [{ id: 100, name: 'last' }]) + ] + ]) + ) + + const labels = await listLabels('/repo-root') + + expect(labels).toHaveLength(101) + expect(labels.at(-1)).toBe('last') + expect(glabApiWithHeadersMock).toHaveBeenCalledTimes(2) + }) + + it('stops paging when x-next-page does not advance and returns no members on an error body', async () => { + getIssueProjectRefMock + .mockResolvedValueOnce({ host: 'gitlab.com', path: 'stablyai/orca' }) + .mockResolvedValueOnce({ host: 'gitlab.com', path: 'stablyai/orca' }) + serveGlabApiPages( + new Map([ + [ + 'projects/stablyai%2Forca/labels?per_page=100&page=1', + glabIncludeOutput({ 'X-Next-Page': '1' }, [{ id: 1, name: 'bug' }]) + ], + [ + 'projects/stablyai%2Forca/members/all?per_page=100&page=1', + glabIncludeOutput({}, { message: '403 Forbidden' }) + ] + ]) + ) + + await expect(listLabels('/repo-root')).resolves.toEqual(['bug']) + await expect(listAssignableUsers('/repo-root')).resolves.toEqual([]) + expect(glabApiWithHeadersMock).toHaveBeenCalledTimes(2) }) it('addIssueComment posts to /notes and maps the response', async () => { diff --git a/src/main/gitlab/project-label-and-member-lookup.ts b/src/main/gitlab/project-label-and-member-lookup.ts index fcad19b9fdf..f223aafccd3 100644 --- a/src/main/gitlab/project-label-and-member-lookup.ts +++ b/src/main/gitlab/project-label-and-member-lookup.ts @@ -1,17 +1,28 @@ import type { GitLabAssignableUser } from '../../shared/gitlab-types' import type { IssueSourcePreference } from '../../shared/repo-types' +import { mapGitLabUser, type GitLabRawUser } from './gitlab-assignable-user-mapping' import { acquire, getGlabKnownHosts, - glabExecFileAsync, + glabApiWithHeaders, glabHostnameArgs, glabRepoExecOptions, + parseGlabJsonList, + parseGlabPaginationHeader, release, resolveIssueSource, - type LocalGitExecOptions + type LocalGitExecOptions, + type ProjectRef } from './gl-utils' import { encodedProject } from './project-path-encoding' +// GitLab's per_page maximum. +const PAGE_SIZE = 100 +// Why: bounds the walk (5,000 entries) when a proxy strips x-next-page or it never advances. +const MAX_PAGES = 50 + +type GitLabRawLabel = { name?: string | null } | null + export async function listLabels( repoPath: string, preference?: IssueSourcePreference, @@ -31,21 +42,14 @@ export async function listLabels( } await acquire() try { - const { stdout } = await glabExecFileAsync( - [ - 'api', - ...glabHostnameArgs(projectRef, connectionId), - '--paginate', - `projects/${encodedProject(projectRef.path)}/labels`, - '--jq', - '.[].name' - ], - glabRepoExecOptions(repoPath, connectionId, localGitOptions) + const labels = await fetchAllPages( + repoPath, + projectRef, + `projects/${encodedProject(projectRef.path)}/labels`, + connectionId, + localGitOptions ) - return stdout - .trim() - .split('\n') - .filter((l) => l.length > 0) + return labels.flatMap((label) => (label?.name ? [label.name] : [])) } catch { return [] } finally { @@ -75,50 +79,56 @@ export async function listAssignableUsers( // Why: `members/all` returns project members including those inherited // from parent groups — important for projects under a top-level group // where assignable users typically come from the group, not the project. - // --paginate walks every page; --jq emits NDJSON. - const { stdout } = await glabExecFileAsync( - [ - 'api', - ...glabHostnameArgs(projectRef, connectionId), - '--paginate', - `projects/${encodedProject(projectRef.path)}/members/all?per_page=100`, - '--jq', - '.[] | {id, username, name, avatar_url, state}' - ], - glabRepoExecOptions(repoPath, connectionId, localGitOptions) + const members = await fetchAllPages( + repoPath, + projectRef, + `projects/${encodedProject(projectRef.path)}/members/all`, + connectionId, + localGitOptions ) - type RESTMember = { - id?: number - username?: string - name?: string | null - avatar_url?: string | null - state?: string | null - } - const users: GitLabAssignableUser[] = [] - for (const line of stdout.split('\n')) { - const trimmed = line.trim() - if (!trimmed) { - continue - } - try { - const user = JSON.parse(trimmed) as RESTMember - if (user.username) { - users.push({ - ...(typeof user.id === 'number' ? { id: user.id } : {}), - username: user.username, - name: user.name ?? null, - avatarUrl: user.avatar_url ?? '', - ...(user.state !== undefined ? { state: user.state } : {}) - }) - } - } catch { - // Skip malformed NDJSON lines defensively. - } - } - return users + return members.map(mapGitLabUser).filter((user): user is GitLabAssignableUser => user !== null) } catch { return [] } finally { release() } } + +// Why: `glab api` has no --jq and older glab lacks `--output ndjson`, so walk the pages here +// and parse each one as a JSON list. +async function fetchAllPages( + repoPath: string, + projectRef: ProjectRef, + endpoint: string, + connectionId: string | null | undefined, + localGitOptions: LocalGitExecOptions +): Promise { + const items: T[] = [] + let page: number | undefined = 1 + for (let fetched = 0; page !== undefined && fetched < MAX_PAGES; fetched += 1) { + const { body, headers } = await glabApiWithHeaders( + [ + ...glabHostnameArgs(projectRef, connectionId), + `${endpoint}?per_page=${PAGE_SIZE}&page=${page}` + ], + glabRepoExecOptions(repoPath, connectionId, localGitOptions) + ) + const pageItems = parseGlabJsonList(body) + items.push(...pageItems) + page = nextPageNumber(headers['x-next-page'], page, pageItems.length) + } + return items +} + +function nextPageNumber( + nextPageHeader: string | undefined, + page: number, + pageLength: number +): number | undefined { + // Why: GitLab sends an empty x-next-page on the last page, so an absent one means a proxy + // stripped it; a full page then probes one more. + if (nextPageHeader === undefined) { + return pageLength < PAGE_SIZE ? undefined : page + 1 + } + return parseGlabPaginationHeader(nextPageHeader, page + 1) +}