diff --git a/src/main/git/runner.test.ts b/src/main/git/runner.test.ts index f27d421222a..24a031bbceb 100644 --- a/src/main/git/runner.test.ts +++ b/src/main/git/runner.test.ts @@ -2,7 +2,47 @@ // (transient detection must propagate, not silently retry on 250ms cadence) // and stderr extraction from execFile rejections (err.message is unreliable). import { afterEach, describe, expect, it, vi } from 'vitest' -import { extractExecError, isTransientGhError, parseRetryAfterMs } from './runner' +import { + extractExecError, + isTransientGhError, + parseRetryAfterMs, + redirectPortedHostnameToEnv +} from './runner' + +describe('redirectPortedHostnameToEnv', () => { + it('moves a ported --hostname into GITLAB_HOST and strips the flag', () => { + const { args, options } = redirectPortedHostnameToEnv( + ['api', '--hostname', 'gitlab.example.com:8443', 'projects/foo%2Fbar/issues'], + { cwd: '/repo' } + ) + expect(args).toEqual(['api', 'projects/foo%2Fbar/issues']) + expect(options.env?.GITLAB_HOST).toBe('gitlab.example.com:8443') + expect(options.cwd).toBe('/repo') + }) + + it('leaves a port-less --hostname untouched', () => { + const input = ['api', '--hostname', 'gitlab.com', 'user'] + const { args, options } = redirectPortedHostnameToEnv(input, {}) + expect(args).toEqual(input) + expect(options.env).toBeUndefined() + }) + + it('is a no-op when no --hostname is present', () => { + const input = ['auth', 'status'] + const { args, options } = redirectPortedHostnameToEnv(input, { env: { A: '1' } }) + expect(args).toEqual(input) + expect(options.env).toEqual({ A: '1' }) + }) + + it('preserves existing env entries alongside GITLAB_HOST', () => { + const { options } = redirectPortedHostnameToEnv( + ['auth', 'status', '--hostname', 'gl.example.org:3001'], + { env: { PATH: '/usr/bin' } } + ) + expect(options.env?.PATH).toBe('/usr/bin') + expect(options.env?.GITLAB_HOST).toBe('gl.example.org:3001') + }) +}) afterEach(() => { vi.restoreAllMocks() diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index f8284a86468..b25629b2a92 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -1409,10 +1409,40 @@ type GlabExecOptions = Omit & { * * Retry policy mirrors ghExecFileAsync. */ +/** + * glab's `--hostname` flag rejects a host that carries a port + * ("error parsing --hostname: invalid hostname"). A self-hosted GitLab on a + * non-default port (e.g. `gitlab.example.com:8443`) must instead be selected + * via the `GITLAB_HOST` env var, which accepts `host:port`. Translate any + * `--hostname host:port` pair into `GITLAB_HOST` so every call site (`api`, + * `auth status`, …) works against ported self-hosted instances. Port-less + * `--hostname` values are left untouched. + * + * @internal exported for tests. + */ +export function redirectPortedHostnameToEnv( + args: string[], + options: GlabExecOptions +): { args: string[]; options: GlabExecOptions } { + const i = args.indexOf('--hostname') + if (i === -1 || i + 1 >= args.length) { + return { args, options } + } + const host = args[i + 1] + if (!/^[^/\s]+:\d+$/.test(host)) { + return { args, options } + } + return { + args: [...args.slice(0, i), ...args.slice(i + 2)], + options: { ...options, env: { ...(options.env ?? process.env), GITLAB_HOST: host } } + } +} + export async function glabExecFileAsync( args: string[], options: GlabExecOptions = {} ): Promise<{ stdout: string; stderr: string }> { + ;({ args, options } = redirectPortedHostnameToEnv(args, options)) let resolved = resolveCommand('glab', args, options.cwd, options.wslDistro) let lastError: unknown let attemptedDefaultWslFallback = false diff --git a/src/main/gitlab/client.ts b/src/main/gitlab/client.ts index b5a1569fa0d..beb446d9ed4 100644 --- a/src/main/gitlab/client.ts +++ b/src/main/gitlab/client.ts @@ -245,7 +245,7 @@ export async function getProjectSlug( connectionId?: string | null, options: HostedReviewExecutionOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts() + const knownHosts = await getGlabKnownHosts(connectionId) return getProjectRef( repoPath, knownHosts, @@ -265,7 +265,7 @@ export async function getMergeRequest( connectionId?: string | null, options: HostedReviewExecutionOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts() + const knownHosts = await getGlabKnownHosts(connectionId) const localGitArgs = hostedReviewLocalGitOptionArgs(options) const localGitOptions = localGitArgs[0] ?? {} const projectRef = await getProjectRef(repoPath, knownHosts, connectionId, ...localGitArgs) @@ -316,7 +316,7 @@ export async function getMergeRequestForBranch( if (!branchName && linkedMRIid == null) { return null } - const knownHosts = await getGlabKnownHosts() + const knownHosts = await getGlabKnownHosts(connectionId) const localGitArgs = hostedReviewLocalGitOptionArgs(options) const localGitOptions = localGitArgs[0] ?? {} const projectRef = await getProjectRef(repoPath, knownHosts, connectionId, ...localGitArgs) @@ -400,7 +400,7 @@ export async function listMergeRequests( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts() + const knownHosts = await getGlabKnownHosts(connectionId) // Why: MRs sit on `origin` in the fork model (the user's fork is where // they push branches and submit MRs). Mirror github's `getOwnerRepo` // call site by going through the upstream/origin preference resolver @@ -429,7 +429,7 @@ export async function listMergeRequests( } } // Why: fallback — let glab infer project from cwd, same as listIssues. - // Used when the repo's remote host is not in getGlabKnownHosts() + // Used when the repo's remote host is not in getGlabKnownHosts(connectionId) // (e.g. a fresh self-hosted instance), but glab itself can still // resolve it from the local git config. const stateFlag = mrListStateFlags(state) @@ -598,7 +598,7 @@ export async function listWorkItems( localGitOptions: LocalGitExecOptions = {} ): Promise> { const issueState = mrStateToIssueState(state) - const knownHosts = await getGlabKnownHosts() + const knownHosts = await getGlabKnownHosts(connectionId) const { source: projectRef } = await resolveIssueSource( repoPath, preference, @@ -734,7 +734,7 @@ export async function listTodos( ): Promise { const projectRef = await getProjectRef( repoPath, - await getGlabKnownHosts(), + await getGlabKnownHosts(connectionId), connectionId, localGitOptions ) @@ -813,7 +813,7 @@ async function withProjectRef( await resolveIssueSource( repoPath, preference, - await getGlabKnownHosts(), + await getGlabKnownHosts(connectionId), connectionId, localGitOptions ) diff --git a/src/main/gitlab/gitlab-project-ref-resolution.ts b/src/main/gitlab/gitlab-project-ref-resolution.ts index d414f640ccd..1dba771d05e 100644 --- a/src/main/gitlab/gitlab-project-ref-resolution.ts +++ b/src/main/gitlab/gitlab-project-ref-resolution.ts @@ -20,7 +20,17 @@ export type LocalGitExecOptions = { const PROJECT_REF_CACHE_MAX_ENTRIES = 512 const projectRefCache = new Map() -let knownHostsCache: readonly string[] | null = null +// Why: known hosts are cached PER connection. A repo on an SSH connection +// authenticates against a different glab context than the local one, so a +// process-global cache would leak one connection's hosts into another (and +// poison a connection that probes before its tunnel is ready). The local +// context uses the `'local'` key. +const LOCAL_CONNECTION_KEY = 'local' +const knownHostsCacheByConnection = new Map() + +function connectionCacheKey(connectionId?: string | null): string { + return connectionId ?? LOCAL_CONNECTION_KEY +} /** @internal - exposed for tests only */ export function _resetProjectRefCache(): void { @@ -35,7 +45,7 @@ export function _getProjectRefCacheSize(): number { /** @internal - exposed for tests only */ export function _resetKnownHostsCache(): void { - knownHostsCache = null + knownHostsCacheByConnection.clear() } function rememberProjectRefCacheEntry(cacheKey: string, value: ProjectRef | null): void { @@ -108,7 +118,7 @@ async function resolveProjectRefForRemote( localGitOptions )) ) { - rememberGlabKnownHost(remoteCandidate.host) + rememberGlabKnownHost(remoteCandidate.host, connectionId) rememberProjectRefCacheEntry(cacheKey, remoteCandidate) return remoteCandidate } @@ -220,12 +230,14 @@ export function glabHostnameArgs( return connectionId && projectRef?.host ? ['--hostname', projectRef.host] : [] } -function rememberGlabKnownHost(host: string): void { +function rememberGlabKnownHost(host: string, connectionId?: string | null): void { const normalizedHost = normalizeGitLabHost(host) - if (!knownHostsCache || knownHostsCache.map(normalizeGitLabHost).includes(normalizedHost)) { + const key = connectionCacheKey(connectionId) + const cached = knownHostsCacheByConnection.get(key) + if (!cached || cached.map(normalizeGitLabHost).includes(normalizedHost)) { return } - knownHostsCache = [...knownHostsCache, normalizedHost] + knownHostsCacheByConnection.set(key, [...cached, normalizedHost]) } async function isGlabConfiguredForRemoteHost( @@ -251,30 +263,47 @@ async function isGlabConfiguredForRemoteHost( } } -export async function getGlabKnownHosts(): Promise { - if (knownHostsCache) { - return knownHostsCache +export async function getGlabKnownHosts(connectionId?: string | null): Promise { + const key = connectionCacheKey(connectionId) + const cached = knownHostsCacheByConnection.get(key) + if (cached) { + return cached } try { + // Why: `glab auth status` is host-scoped, not cwd-scoped — glab reads its + // own config to list authenticated hosts. The connectionId is threaded so + // the RESULT is cached per connection (a connected repo can have a + // different set of authenticated self-hosted hosts than the local one), + // mirroring how project-ref resolution caches per connection. const { stdout, stderr } = await glabExecFileAsync(['auth', 'status']) const hosts = parseGlabAuthStatusHosts(`${stdout}\n${stderr}`) - knownHostsCache = Array.from(new Set([...DEFAULT_GITLAB_HOSTS, ...hosts])) - return knownHostsCache + const merged = Array.from(new Set([...DEFAULT_GITLAB_HOSTS, ...hosts])) + knownHostsCacheByConnection.set(key, merged) + return merged } catch { - knownHostsCache = [...DEFAULT_GITLAB_HOSTS] - return knownHostsCache + // Auth check failed (glab not installed, no auth, tunnel not ready, + // etc.) — fall back to the canonical default for THIS call, but do NOT + // cache the fallback. A later probe (e.g. after the SSH tunnel comes + // up) must be able to discover the real self-hosted host. + return [...DEFAULT_GITLAB_HOSTS] } } export function parseGlabAuthStatusHosts(output: string): string[] { const hosts = new Set() - for (const m of output.matchAll(/logged in to ([a-zA-Z0-9.-]+)/gi)) { + // Why: self-hosted GitLab can run on a non-default port (e.g. + // `gitlab.example.com:8443`); capture the optional `:port` so two services + // on the same hostname but different ports stay distinct downstream. + for (const m of output.matchAll(/logged in to ([a-zA-Z0-9.-]+(?::\d+)?)/gi)) { hosts.add(m[1].toLowerCase()) } for (const line of output.split('\n')) { const bareLine = line.trim() const hostLine = bareLine.endsWith(':') ? bareLine.slice(0, -1) : bareLine - if (line === bareLine && /^[a-zA-Z0-9](?:[a-zA-Z0-9.-]*[a-zA-Z0-9])?$/.test(hostLine)) { + if ( + line === bareLine && + /^[a-zA-Z0-9](?:[a-zA-Z0-9.-]*[a-zA-Z0-9])?(?::\d+)?$/.test(hostLine) + ) { hosts.add(hostLine.toLowerCase()) } } diff --git a/src/main/gitlab/gl-utils.test.ts b/src/main/gitlab/gl-utils.test.ts index 67566f6eec8..7306c5b7fd4 100644 --- a/src/main/gitlab/gl-utils.test.ts +++ b/src/main/gitlab/gl-utils.test.ts @@ -344,6 +344,30 @@ Self-hosted-git it('returns empty list for output with no hosts', () => { expect(parseGlabAuthStatusHosts('Not logged in.')).toEqual([]) }) + + it('captures a non-default port on "Logged in to" lines', () => { + const out = '✓ Logged in to gitlab.example.com:8080 as user (token)' + expect(parseGlabAuthStatusHosts(out)).toEqual(['gitlab.example.com:8080']) + }) + + it('captures a non-default port on header-style lines', () => { + const out = ` +gitlab.example.com:8080: + ✓ Logged in as user + ` + expect(parseGlabAuthStatusHosts(out)).toContain('gitlab.example.com:8080') + }) + + it('keeps two services on the same host distinct by port', () => { + const out = ` +✓ Logged in to gitlab.example.com:8443 as user (token) +✓ Logged in to gitlab.example.com:3030 as user (token) + ` + expect(parseGlabAuthStatusHosts(out).sort()).toEqual([ + 'gitlab.example.com:3030', + 'gitlab.example.com:8443' + ]) + }) }) describe('parseGlabApiResponse', () => { @@ -428,4 +452,52 @@ describe('getGlabKnownHosts', () => { await getGlabKnownHosts() expect(glabExecFileAsyncMock).toHaveBeenCalledTimes(1) }) + + it('recognizes a self-hosted host on a non-default port', async () => { + glabExecFileAsyncMock.mockResolvedValueOnce({ + stdout: '✓ Logged in to gitlab.example.com:8080 as user\n', + stderr: '' + }) + + await expect(getGlabKnownHosts()).resolves.toEqual(['gitlab.com', 'gitlab.example.com:8080']) + }) + + it('caches per connection — the local probe does not satisfy a connection probe', async () => { + glabExecFileAsyncMock + .mockResolvedValueOnce({ stdout: '✓ Logged in to gitlab.com as user\n', stderr: '' }) + .mockResolvedValueOnce({ + stdout: '✓ Logged in to gitlab.example.com:8080 as user\n', + stderr: '' + }) + + await expect(getGlabKnownHosts()).resolves.toEqual(['gitlab.com']) + await expect(getGlabKnownHosts('conn-1')).resolves.toEqual([ + 'gitlab.com', + 'gitlab.example.com:8080' + ]) + // A second probe for the same connection is served from cache. + await expect(getGlabKnownHosts('conn-1')).resolves.toEqual([ + 'gitlab.com', + 'gitlab.example.com:8080' + ]) + expect(glabExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('does not permanently cache the failure fallback — a later probe can re-discover hosts', async () => { + glabExecFileAsyncMock + .mockRejectedValueOnce(new Error('ssh tunnel not ready')) + .mockResolvedValueOnce({ + stdout: '✓ Logged in to gitlab.example.com:8080 as user\n', + stderr: '' + }) + + // First probe fails → canonical default, NOT cached. + await expect(getGlabKnownHosts('conn-1')).resolves.toEqual(['gitlab.com']) + // Re-probe (e.g. after tunnel comes up) discovers the real host. + await expect(getGlabKnownHosts('conn-1')).resolves.toEqual([ + 'gitlab.com', + 'gitlab.example.com:8080' + ]) + expect(glabExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) }) diff --git a/src/main/gitlab/issues.test.ts b/src/main/gitlab/issues.test.ts index 8095288cbf6..f567936d930 100644 --- a/src/main/gitlab/issues.test.ts +++ b/src/main/gitlab/issues.test.ts @@ -187,31 +187,35 @@ describe('gitlab issue operations', () => { expect(result.error?.type).toBe('permission_denied') }) - it('falls back to glab issue list with updated ordering for unresolved self-hosted repos', async () => { + it('returns an isolated not_found error (never a cwd-inferred glab call) when the project is unresolved', async () => { + // Why: a cwd-inferred `glab issue list` would hit `git: exit status 128` + // on an SSH connection and, in an "All projects" aggregate, sink the + // whole panel. The unresolvable project must isolate to a structured + // error instead, and must not spawn any glab subprocess. getIssueProjectRefMock.mockResolvedValueOnce(null) + + const result = await listIssues('/repo-root', 5, undefined, 'opened', '@me') + + expect(result.items).toEqual([]) + expect(result.error?.type).toBe('not_found') + expect(glabExecFileAsyncMock).not.toHaveBeenCalled() + expect(acquireMock).not.toHaveBeenCalled() + }) + + it('returns null for getIssue (and spawns no glab call) when the project is unresolved', async () => { + getIssueProjectRefMock.mockResolvedValueOnce(null) + + await expect(getIssue('/repo-root', 7)).resolves.toBeNull() + expect(glabExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('threads connectionId into getGlabKnownHosts for listIssues', async () => { + getIssueProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'stablyai/orca' }) glabExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }) - await expect(listIssues('/repo-root', 5, undefined, 'opened', '@me')).resolves.toEqual({ - items: [] - }) + await listIssues('/repo-root', 5, undefined, 'opened', undefined, 'conn-7') - expect(glabExecFileAsyncMock).toHaveBeenCalledWith( - [ - 'issue', - 'list', - '--output', - 'json', - '--per-page', - '5', - '--order', - 'updated_at', - '--sort', - 'desc', - '--assignee', - '@me' - ], - { cwd: '/repo-root' } - ) + expect(getGlabKnownHostsMock).toHaveBeenCalledWith('conn-7') }) it('creates an issue and returns its iid + web_url', async () => { diff --git a/src/main/gitlab/issues.ts b/src/main/gitlab/issues.ts index bc1bf2cc297..ec36f68a0f8 100644 --- a/src/main/gitlab/issues.ts +++ b/src/main/gitlab/issues.ts @@ -44,25 +44,24 @@ export async function getIssue( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts() + const knownHosts = await getGlabKnownHosts(connectionId) const projectRef = await getIssueProjectRef(repoPath, knownHosts, connectionId, localGitOptions) + // Why: don't fall back to a cwd-inferred `glab issue view` when the project + // can't be resolved — on an SSH connection cwd is not the repo dir, so glab + // hits a non-repo dir and fails with `git: exit status 128`. Return null + // (the caller already treats a missing project as "no issue") instead of + // spawning a doomed cwd-dependent call. + if (!projectRef) { + return null + } await acquire() try { - if (projectRef) { - const { stdout } = await glabExecFileAsync( - [ - 'api', - ...glabHostnameArgs(projectRef, connectionId), - `projects/${encodedProject(projectRef.path)}/issues/${issueNumber}` - ], - glabRepoExecOptions(repoPath, connectionId, localGitOptions) - ) - const data = JSON.parse(stdout) - return mapGitLabIssueInfo(data) - } - // Fallback for non-GitLab remotes — let glab infer the project from cwd. const { stdout } = await glabExecFileAsync( - ['issue', 'view', String(issueNumber), '--output', 'json'], + [ + 'api', + ...glabHostnameArgs(projectRef, connectionId), + `projects/${encodedProject(projectRef.path)}/issues/${issueNumber}` + ], glabRepoExecOptions(repoPath, connectionId, localGitOptions) ) const data = JSON.parse(stdout) @@ -93,7 +92,7 @@ export async function listIssues( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts() + const knownHosts = await getGlabKnownHosts(connectionId) const { source: projectRef } = await resolveIssueSource( repoPath, preference, @@ -101,49 +100,38 @@ export async function listIssues( connectionId, localGitOptions ) - await acquire() - try { - if (projectRef) { - const stateParam = state === 'all' ? '' : `&state=${state}` - const scopeParam = assignee === '@me' ? '&scope=assigned_to_me' : '' - const { stdout } = await glabExecFileAsync( - [ - 'api', - ...glabHostnameArgs(projectRef, connectionId), - `projects/${encodedProject(projectRef.path)}/issues?per_page=${limit}&order_by=updated_at&sort=desc${stateParam}${scopeParam}` - ], - glabRepoExecOptions(repoPath, connectionId, localGitOptions) - ) - const data = JSON.parse(stdout) as Record[] - // Why: GitLab's project issues endpoint returns true issues only - // (MRs are a separate endpoint), so no equivalent of GitHub's - // pull_request filter is needed here. - return { - items: data.map((d) => mapGitLabIssueInfo(d as Parameters[0])) + // Why: when the project can't be resolved we must NOT fall back to an + // unscoped `glab issue list` that infers the project from cwd. For a repo + // on an SSH connection there is no local cwd matching the repo, so glab + // runs git resolution in a non-repo dir and fails with `git: exit status + // 128`. In an "All projects" aggregate one such failure must not sink the + // whole panel — return a structured, isolated result so the resolvable + // projects still load. + if (!projectRef) { + return { + items: [], + error: { + type: 'not_found', + message: 'Could not resolve a GitLab project for this repository.' } } - // Fallback — let glab infer project from cwd. glab issue list defaults - // to opened; only pass --closed / --all when explicitly requested. - const stateFlag = state === 'closed' ? ['--closed'] : state === 'all' ? ['--all'] : [] - const assigneeFlag = assignee ? ['--assignee', assignee] : [] + } + await acquire() + try { + const stateParam = state === 'all' ? '' : `&state=${state}` + const scopeParam = assignee === '@me' ? '&scope=assigned_to_me' : '' const { stdout } = await glabExecFileAsync( [ - 'issue', - 'list', - '--output', - 'json', - '--per-page', - String(limit), - '--order', - 'updated_at', - '--sort', - 'desc', - ...stateFlag, - ...assigneeFlag + 'api', + ...glabHostnameArgs(projectRef, connectionId), + `projects/${encodedProject(projectRef.path)}/issues?per_page=${limit}&order_by=updated_at&sort=desc${stateParam}${scopeParam}` ], glabRepoExecOptions(repoPath, connectionId, localGitOptions) ) - const data = JSON.parse(stdout) as unknown[] + const data = JSON.parse(stdout) as Record[] + // Why: GitLab's project issues endpoint returns true issues only + // (MRs are a separate endpoint), so no equivalent of GitHub's + // pull_request filter is needed here. return { items: data.map((d) => mapGitLabIssueInfo(d as Parameters[0])) } @@ -174,7 +162,7 @@ export async function createIssue( if (!trimmedTitle) { return { ok: false, error: 'Title is required' } } - const knownHosts = await getGlabKnownHosts() + const knownHosts = await getGlabKnownHosts(connectionId) const { source: projectRef } = await resolveIssueSource( repoPath, preference, @@ -244,7 +232,7 @@ export async function updateIssue( await resolveIssueSource( repoPath, preference, - await getGlabKnownHosts(), + await getGlabKnownHosts(connectionId), connectionId, localGitOptions ) @@ -381,7 +369,7 @@ export async function addIssueComment( await resolveIssueSource( repoPath, preference, - await getGlabKnownHosts(), + await getGlabKnownHosts(connectionId), connectionId, localGitOptions ) @@ -439,7 +427,7 @@ export async function listLabels( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts() + const knownHosts = await getGlabKnownHosts(connectionId) const { source: projectRef } = await resolveIssueSource( repoPath, preference, @@ -480,7 +468,7 @@ export async function listAssignableUsers( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const knownHosts = await getGlabKnownHosts() + const knownHosts = await getGlabKnownHosts(connectionId) const { source: projectRef } = await resolveIssueSource( repoPath, preference, diff --git a/src/main/gitlab/project-ref-parser.test.ts b/src/main/gitlab/project-ref-parser.test.ts index 53adc0c5aa5..4d846548f82 100644 --- a/src/main/gitlab/project-ref-parser.test.ts +++ b/src/main/gitlab/project-ref-parser.test.ts @@ -38,21 +38,68 @@ describe('gitlab project ref parsing', () => { ).toEqual({ host: 'gitlab.example.com', path: 'team/api' }) }) - it('parses GitLab remotes with non-standard ports without treating the port as a path segment', () => { + it('drops the SSH transport port from the GitLab host identity', () => { + // Why: for ssh remotes the port is a transport port (e.g. :2222), not the + // web/API endpoint, so it must not become part of the recognized host. expect( parseGitLabProjectRef('ssh://git@gitlab.example.com:2222/team/api.git', [ 'gitlab.com', 'gitlab.example.com' ]) ).toEqual({ host: 'gitlab.example.com', path: 'team/api' }) + }) + + it('keeps the HTTP(S) port as part of the self-hosted GitLab host identity', () => { + // Why: a self-hosted GitLab served on a non-default web port (e.g. :8443) + // is identified by host:port end-to-end so `glab --hostname` targets it. + expect( + parseGitLabProjectRef('https://gitlab.example.com:8443/team/api.git', [ + 'gitlab.com', + 'gitlab.example.com:8443' + ]) + ).toEqual({ host: 'gitlab.example.com:8443', path: 'team/api' }) + }) + + it('matches a port-bearing http remote against a port-less legacy known host', () => { + // Why: a known host recorded without a port (legacy/bare entry) still + // recognizes a remote on any port of the same hostname. expect( parseGitLabProjectRef('https://gitlab.example.com:8443/team/api.git', [ 'gitlab.com', 'gitlab.example.com' ]) + ).toEqual({ host: 'gitlab.example.com:8443', path: 'team/api' }) + }) + + it('distinguishes two services on the same host by port — only the GitLab one matches', () => { + // Why: a GitLab on :8443 and a Gitea on :3030 share a hostname but are + // different services. With only the GitLab port in known hosts, the Gitea + // remote must NOT be classified as GitLab. + const knownHosts = ['gitlab.com', 'gitea.example.com:8443'] + expect(parseGitLabProjectRef('http://gitea.example.com:8443/team/api.git', knownHosts)).toEqual( + { host: 'gitea.example.com:8443', path: 'team/api' } + ) + expect( + parseGitLabProjectRef('http://gitea.example.com:3030/team/api.git', knownHosts) + ).toBeNull() + }) + + it('matches an SCP-like self-hosted remote against a port-less known host', () => { + expect( + parseGitLabProjectRef('git@gitlab.example.com:team/api.git', [ + 'gitlab.com', + 'gitlab.example.com' + ]) ).toEqual({ host: 'gitlab.example.com', path: 'team/api' }) }) + it('keeps gitlab.com (no port) recognized as a default host', () => { + expect(parseGitLabProjectRef('https://gitlab.com/acme/widgets.git')).toEqual({ + host: 'gitlab.com', + path: 'acme/widgets' + }) + }) + it('rejects single-segment paths (host root or user-only)', () => { expect(parseGitLabProjectRef('git@gitlab.com:foo.git')).toBeNull() expect(parseGitLabProjectRef('https://gitlab.com/foo.git')).toBeNull() diff --git a/src/main/gitlab/project-ref-parser.ts b/src/main/gitlab/project-ref-parser.ts index e42c0e2081b..9477cb8baa0 100644 --- a/src/main/gitlab/project-ref-parser.ts +++ b/src/main/gitlab/project-ref-parser.ts @@ -12,10 +12,33 @@ export function normalizeGitLabHost(value: string): string { return value.trim().toLowerCase() } +// Why: host recognition is port-aware so two services on the same hostname +// but different ports (e.g. a GitLab on :8080 and a Gitea on :3030) are not +// conflated. The hostname (port-less) part is kept for legacy known-host +// entries that were recorded without a port. +function hostnameOf(host: string): string { + // `host` may be `name` or `name:port`. Strip a trailing `:digits` port. + return host.replace(/:\d+$/, '') +} + function stripGitSuffix(path: string): string { return path.replace(/\/+$/, '').replace(/\.git$/i, '') } +// Why: the GitLab host identity is the web/API endpoint, which is what `glab +// --hostname` and the known-hosts list speak in terms of. For http(s) +// remotes the URL port IS that endpoint port (e.g. self-hosted on :8080), +// so it must be kept. For ssh/git remotes the port is a transport port +// (e.g. ssh on :2222) that does not identify the GitLab instance, so it is +// dropped and only the hostname is used. +function hostIdentityFromUrl(url: URL): string { + const protocol = url.protocol.toLowerCase() + if (protocol === 'http:' || protocol === 'https:') { + return url.host + } + return url.hostname +} + function makeProjectRefForTrustedHost(host: string, path: string): ProjectRef | null { const normalizedHost = normalizeGitLabHost(host) const normalizedPath = stripGitSuffix(path.replace(/^\/+/, '')).trim() @@ -27,6 +50,27 @@ function makeProjectRefForTrustedHost(host: string, path: string): ProjectRef | return { host: normalizedHost, path: normalizedPath } } +/** + * Does `urlHost` (which may include a `:port`) match a known-host entry? + * - An exact match (including any port) always counts. + * - A known entry WITHOUT a port also matches a URL host on the same + * hostname regardless of the URL's port — this preserves recognition for + * legacy `gitlab.com` / bare-hostname known entries. + * - A known entry WITH a port only matches a URL host with the exact same + * port, so `gitlab.example.com:8443` does not accept a + * `gitea.example.com:3000` (or same-host different-port) remote. + */ +function knownHostMatches(urlHost: string, knownHost: string): boolean { + if (urlHost === knownHost) { + return true + } + if (hostnameOf(knownHost) === knownHost) { + // Known entry has no port — match on hostname alone. + return hostnameOf(urlHost) === knownHost + } + return false +} + function makeProjectRef( host: string, path: string, @@ -34,7 +78,7 @@ function makeProjectRef( ): ProjectRef | null { const normalizedHost = normalizeGitLabHost(host) const normalizedKnownHosts = knownHosts.map(normalizeGitLabHost) - if (!normalizedKnownHosts.includes(normalizedHost)) { + if (!normalizedKnownHosts.some((knownHost) => knownHostMatches(normalizedHost, knownHost))) { return null } return makeProjectRefForTrustedHost(normalizedHost, path) @@ -54,7 +98,7 @@ export function parseRemoteProjectRefCandidate(remoteUrl: string): ProjectRef | if (!['http:', 'https:', 'ssh:', 'git:', 'git+ssh:'].includes(url.protocol.toLowerCase())) { return null } - return makeProjectRefForTrustedHost(url.hostname, url.pathname) + return makeProjectRefForTrustedHost(hostIdentityFromUrl(url), url.pathname) } catch { return null } @@ -77,7 +121,7 @@ export function parseGitLabProjectRef( if (!['http:', 'https:', 'ssh:', 'git:', 'git+ssh:'].includes(url.protocol.toLowerCase())) { return null } - return makeProjectRef(url.hostname, url.pathname, knownHosts) + return makeProjectRef(hostIdentityFromUrl(url), url.pathname, knownHosts) } catch { return null } diff --git a/src/main/gitlab/work-item-details.ts b/src/main/gitlab/work-item-details.ts index 33132748b1c..d9f744c138f 100644 --- a/src/main/gitlab/work-item-details.ts +++ b/src/main/gitlab/work-item-details.ts @@ -361,7 +361,7 @@ export async function getWorkItemDetails( await resolveIssueSource( repoPath, preference, - await getGlabKnownHosts(), + await getGlabKnownHosts(connectionId), connectionId, localGitOptions ) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index bbfa9731182..8a5906b5612 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -14584,7 +14584,7 @@ export class OrcaRuntimeService { } catch (error) { return { error: error instanceof Error ? error.message : 'Could not resolve git remote.' } } - const knownHosts = await getGlabKnownHosts() + const knownHosts = await getGlabKnownHosts(repo.connectionId ?? null) const projectRef = await getGitLabProjectRefForRemote( repo.path, remote, @@ -14709,7 +14709,7 @@ export class OrcaRuntimeService { connectionId?: string | null, localGitOptions: { wslDistro?: string } = {} ): Promise { - const knownHosts = await getGlabKnownHosts() + const knownHosts = await getGlabKnownHosts(connectionId) const localGitOptionArgs = Object.keys(localGitOptions).length > 0 ? ([localGitOptions] as const) : [] if (preference === 'origin') { diff --git a/src/renderer/src/lib/gitlab-links.test.ts b/src/renderer/src/lib/gitlab-links.test.ts index 6ce333ad516..8fd85c5d76f 100644 --- a/src/renderer/src/lib/gitlab-links.test.ts +++ b/src/renderer/src/lib/gitlab-links.test.ts @@ -24,6 +24,16 @@ describe('parseGitLabIssueOrMRNumber', () => { expect(parseGitLabIssueOrMRNumber('https://gitlab.example.com/team/api/-/issues/7')).toBe(7) }) + it('parses modern /-/work_items/ issue URLs', () => { + expect(parseGitLabIssueOrMRNumber('https://gitlab.com/stablyai/orca/-/work_items/923')).toBe( + 923 + ) + expect( + parseGitLabIssueOrMRNumber('https://gitlab.example.com:8443/team/api/-/work_items/7') + ).toBe(7) + expect(parseGitLabIssueOrMRNumber('https://gitlab.com/g/p/-/work_items/923/designs')).toBe(923) + }) + it('parses URLs with nested group paths', () => { expect( parseGitLabIssueOrMRNumber('https://gitlab.com/group/subgroup/project/-/merge_requests/55') @@ -90,6 +100,21 @@ describe('parseGitLabIssueOrMRLink', () => { }) }) + it('treats /-/work_items/ as an issue work item', () => { + expect(parseGitLabIssueOrMRLink('https://gitlab.com/stablyai/orca/-/work_items/923')).toEqual({ + slug: { host: 'gitlab.com', path: 'stablyai/orca' }, + number: 923, + type: 'issue' + }) + expect( + parseGitLabIssueOrMRLink('https://gitlab.example.com:8443/team/api/-/work_items/7') + ).toEqual({ + slug: { host: 'gitlab.example.com:8443', path: 'team/api' }, + number: 7, + type: 'issue' + }) + }) + it('extracts slug, number, and type from URLs with trailing page segments', () => { expect(parseGitLabIssueOrMRLink('https://gitlab.com/g/p/-/merge_requests/77/diffs')).toEqual({ slug: { host: 'gitlab.com', path: 'g/p' }, diff --git a/src/renderer/src/lib/gitlab-links.ts b/src/renderer/src/lib/gitlab-links.ts index 217a56ef28a..1bd36c13712 100644 --- a/src/renderer/src/lib/gitlab-links.ts +++ b/src/renderer/src/lib/gitlab-links.ts @@ -4,10 +4,12 @@ import { isWorkItemLinkQueryTooLarge } from './work-item-link-query-bounds' // be self-hosted (gitlab.example.com), so the URL pattern uses the // project-internal `/-/` separator as the GitLab-specific signal rather // than locking to gitlab.com. Anything matching `//-/(issues| -// merge_requests)/` is treated as a GitLab item URL regardless -// of host. -const GL_ITEM_PATH_RE = /\/(?:issues|merge_requests)\/(\d+)(?:\/.*)?$/i -const GL_ITEM_PATH_FULL_RE = /^\/(.+)\/-\/(issues|merge_requests)\/(\d+)(?:\/.*)?$/i +// work_items|merge_requests)/` is treated as a GitLab item URL +// regardless of host. Modern GitLab emits issue URLs as +// `/-/work_items/`; treat that as an issue work item, same as the +// legacy `/-/issues/` form. +const GL_ITEM_PATH_RE = /\/(?:issues|work_items|merge_requests)\/(\d+)(?:\/.*)?$/i +const GL_ITEM_PATH_FULL_RE = /^\/(.+)\/-\/(issues|work_items|merge_requests)\/(\d+)(?:\/.*)?$/i export type ProjectSlug = { /** GitLab hostname, preserving self-hosted instances from pasted URLs. */ diff --git a/src/renderer/src/lib/linked-work-item-provider.ts b/src/renderer/src/lib/linked-work-item-provider.ts index 381a148931d..7ce731622ff 100644 --- a/src/renderer/src/lib/linked-work-item-provider.ts +++ b/src/renderer/src/lib/linked-work-item-provider.ts @@ -1,11 +1,15 @@ import type { LinkedWorkItemSummary } from './new-workspace' +// Why: self-hosted GitLab issue URLs may not contain "gitlab", and modern +// GitLab emits issue URLs as `/-/work_items/` as well as the legacy +// `/-/issues/`. Recognize both forms. +const GL_ISSUE_PATH_RE = /\/-\/(?:issues|work_items)\//i + export function isGitLabIssueUrl(url: string): boolean { - // Why: self-hosted GitLab issue URLs may not contain "gitlab". try { - return new URL(url).pathname.includes('/-/issues/') + return GL_ISSUE_PATH_RE.test(new URL(url).pathname) } catch { - return /\/-\/issues\//i.test(url) + return GL_ISSUE_PATH_RE.test(url) } } diff --git a/src/renderer/src/lib/new-workspace.test.ts b/src/renderer/src/lib/new-workspace.test.ts index d753e0977d0..a9c6c10cc5a 100644 --- a/src/renderer/src/lib/new-workspace.test.ts +++ b/src/renderer/src/lib/new-workspace.test.ts @@ -215,6 +215,13 @@ describe('isGitLabIssueUrl', () => { expect(isGitLabIssueUrl('https://gitlab.example.com/group/project/-/issues/123')).toBe(true) }) + it('detects modern /-/work_items/ issue URLs (incl. non-default port)', () => { + expect(isGitLabIssueUrl('https://gitlab.com/group/project/-/work_items/123')).toBe(true) + expect( + isGitLabIssueUrl('https://gitlab.example.com:8443/group/project/-/work_items/7') + ).toBe(true) + }) + it('does not classify GitHub issue URLs as GitLab issues', () => { expect(isGitLabIssueUrl('https://github.com/group/project/issues/123')).toBe(false) }) diff --git a/src/renderer/src/lib/work-item-lookup-text.test.ts b/src/renderer/src/lib/work-item-lookup-text.test.ts index 46693afce43..3ced0ea2372 100644 --- a/src/renderer/src/lib/work-item-lookup-text.test.ts +++ b/src/renderer/src/lib/work-item-lookup-text.test.ts @@ -19,6 +19,13 @@ describe('isWorkItemLookupText', () => { ) }) + it('detects modern /-/work_items/ GitLab URLs', () => { + expect(isWorkItemLookupText('https://gitlab.com/group/project/-/work_items/42')).toBe(true) + expect( + isWorkItemLookupText('https://gitlab.example.com:8443/group/sub/project/-/work_items/9') + ).toBe(true) + }) + it('detects Linear issue URLs', () => { expect(isWorkItemLookupText('https://linear.app/acme/issue/STA-123/fix-the-bug')).toBe(true) })