diff --git a/src/main/git/remote-name-listing.test.ts b/src/main/git/remote-name-listing.test.ts new file mode 100644 index 00000000000..ebf78e31367 --- /dev/null +++ b/src/main/git/remote-name-listing.test.ts @@ -0,0 +1,200 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + gitExecFileAsyncMock, + getSshGitProviderMock, + getSshGitProviderGenerationMock, + readLocalGitConfigSignatureMock +} = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + getSshGitProviderMock: vi.fn(), + getSshGitProviderGenerationMock: vi.fn(() => 0), + readLocalGitConfigSignatureMock: vi.fn<() => Promise>(async () => 'sig-1') +})) + +vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) +vi.mock('../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: getSshGitProviderMock, + getSshGitProviderGeneration: getSshGitProviderGenerationMock +})) +vi.mock('../github/local-git-config-signature', () => ({ + readLocalGitConfigSignature: readLocalGitConfigSignatureMock +})) + +import { REMOTE_URL_PROBE_TIMEOUT_MS } from './remote-url-probe' +import { + _resetRemoteNameListingCache, + listCachedRemoteNames, + shouldProbeGitRemote +} from './remote-name-listing' + +function remoteListCalls(): unknown[][] { + return gitExecFileAsyncMock.mock.calls.filter( + ([args]) => Array.isArray(args) && args[0] === 'remote' && args[1] !== 'get-url' + ) +} + +describe('cached git remote name listing', () => { + beforeEach(() => { + _resetRemoteNameListingCache() + gitExecFileAsyncMock.mockReset() + getSshGitProviderMock.mockReset() + getSshGitProviderGenerationMock.mockReset() + getSshGitProviderGenerationMock.mockReturnValue(0) + readLocalGitConfigSignatureMock.mockReset() + readLocalGitConfigSignatureMock.mockImplementation(async () => 'sig-1') + }) + + it('skips probing upstream when listing only has origin', async () => { + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'origin\n' }) + + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(false) + await expect(listCachedRemoteNames('/repo')).resolves.toEqual(['origin']) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['remote'], { + cwd: '/repo', + timeout: REMOTE_URL_PROBE_TIMEOUT_MS + }) + }) + + it('still probes upstream when listing includes that remote', async () => { + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'origin\nupstream\n' }) + + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(true) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + + it('reuses a signed listing instead of spawning git remote again', async () => { + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'origin\n' }) + + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(false) + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(false) + await expect(shouldProbeGitRemote('/repo', 'origin')).resolves.toBe(true) + + expect(remoteListCalls()).toHaveLength(1) + }) + + it('re-lists as soon as the git config signature changes', async () => { + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'origin\n' }) + .mockResolvedValueOnce({ stdout: 'origin\nupstream\n' }) + + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(false) + readLocalGitConfigSignatureMock.mockImplementation(async () => 'sig-2') + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(true) + expect(remoteListCalls()).toHaveLength(2) + }) + + it('uses the short TTL when config changes during remote listing', async () => { + vi.useFakeTimers() + try { + readLocalGitConfigSignatureMock + .mockResolvedValueOnce('sig-1') + .mockResolvedValueOnce('sig-2') + .mockResolvedValue('sig-2') + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'origin\n' }) + .mockResolvedValueOnce({ stdout: 'origin\nupstream\n' }) + + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(false) + await vi.advanceTimersByTimeAsync(30_001) + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(true) + expect(remoteListCalls()).toHaveLength(2) + } finally { + vi.useRealTimers() + } + }) + + it('expires an unsigned listing after the short TTL', async () => { + vi.useFakeTimers() + try { + readLocalGitConfigSignatureMock.mockImplementation(async () => undefined) + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'origin\n' }) + .mockResolvedValueOnce({ stdout: 'origin\nupstream\n' }) + + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(false) + await vi.advanceTimersByTimeAsync(30_001) + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(true) + expect(remoteListCalls()).toHaveLength(2) + } finally { + vi.useRealTimers() + } + }) + + it('holds a signed listing past the unsigned TTL', async () => { + vi.useFakeTimers() + try { + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'origin\n' }) + + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(false) + await vi.advanceTimersByTimeAsync(4 * 60_000) + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(false) + expect(remoteListCalls()).toHaveLength(1) + } finally { + vi.useRealTimers() + } + }) + + it('fails open and does not cache when listing throws', async () => { + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('git timed out.')) + .mockResolvedValueOnce({ stdout: 'origin\n' }) + + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(true) + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(false) + expect(remoteListCalls()).toHaveLength(2) + }) + + it('coalesces concurrent listings onto one spawn', async () => { + gitExecFileAsyncMock.mockImplementation(async () => { + await Promise.resolve() + return { stdout: 'origin\n' } + }) + + await expect( + Promise.all([ + shouldProbeGitRemote('/repo', 'upstream'), + shouldProbeGitRemote('/repo', 'upstream'), + listCachedRemoteNames('/repo') + ]) + ).resolves.toEqual([false, false, ['origin']]) + expect(remoteListCalls()).toHaveLength(1) + }) + + it('keeps host and WSL listings separate', async () => { + gitExecFileAsyncMock.mockImplementation( + async (_args: string[], options: { wslDistro?: string } = {}) => ({ + stdout: options.wslDistro ? 'origin\nupstream\n' : 'origin\n' + }) + ) + + await expect(shouldProbeGitRemote('/repo', 'upstream')).resolves.toBe(false) + await expect( + shouldProbeGitRemote('/repo', 'upstream', null, { wslDistro: 'Ubuntu' }) + ).resolves.toBe(true) + expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith(2, ['remote'], { + cwd: '/repo', + timeout: REMOTE_URL_PROBE_TIMEOUT_MS, + wslDistro: 'Ubuntu' + }) + }) + + it('lists remotes through the SSH git provider', async () => { + const exec = vi.fn(async () => ({ stdout: 'origin\n', stderr: '' })) + getSshGitProviderMock.mockReturnValue({ exec }) + + await expect(shouldProbeGitRemote('/remote/repo', 'upstream', 'ssh-1')).resolves.toBe(false) + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + expect(exec).toHaveBeenCalledWith(['remote'], '/remote/repo', { + signal: expect.any(AbortSignal) + }) + }) + + it('fails open when the SSH git provider is missing instead of listing locally', async () => { + getSshGitProviderMock.mockReturnValue(undefined) + + await expect(shouldProbeGitRemote('/remote/repo', 'upstream', 'ssh-1')).resolves.toBe(true) + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/git/remote-name-listing.ts b/src/main/git/remote-name-listing.ts new file mode 100644 index 00000000000..17aa9c9444a --- /dev/null +++ b/src/main/git/remote-name-listing.ts @@ -0,0 +1,174 @@ +import { readLocalGitConfigSignature } from '../github/local-git-config-signature' +import { getSshGitProvider, getSshGitProviderGeneration } from '../providers/ssh-git-dispatch' +import { runCoalescedProbe, type CoalescedProbes } from './coalesced-probe' +import type { GitAdmissionTier } from './command-runner/git-exec-options' +import { REMOTE_URL_PROBE_TIMEOUT_MS } from './remote-url-probe' +import { gitExecFileAsync } from './runner' + +export type RemoteNameListingGitOptions = { + wslDistro?: string + admissionTier?: GitAdmissionTier +} + +const SIGNED_REMOTE_NAME_LISTING_TTL_MS = 5 * 60_000 +const UNSIGNED_REMOTE_NAME_LISTING_TTL_MS = 30_000 +const REMOTE_NAME_LISTING_CACHE_MAX_ENTRIES = 512 + +type CachedRemoteNames = { + remotes: string[] + expiresAt: number + configSignature?: string +} + +const remoteNameListingCache = new Map() +const remoteNameListingInFlight: CoalescedProbes = new Map() + +/** @internal - exposed for tests only */ +export function _resetRemoteNameListingCache(): void { + remoteNameListingCache.clear() + remoteNameListingInFlight.clear() +} + +function parseRemoteNames(stdout: string): string[] { + return stdout + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean) +} + +function remoteNameListingCacheKey( + repoPath: string, + connectionId?: string | null, + localGitOptions: RemoteNameListingGitOptions = {} +): string { + const runtimeKey = connectionId + ? `ssh:${connectionId}:${getSshGitProviderGeneration(connectionId)}` + : `local:${localGitOptions.wslDistro ?? 'host'}` + return `${runtimeKey}\0${repoPath}` +} + +function pruneRemoteNameListingCache(now: number): void { + for (const [key, entry] of remoteNameListingCache) { + if (entry.expiresAt <= now) { + remoteNameListingCache.delete(key) + } + } + while (remoteNameListingCache.size > REMOTE_NAME_LISTING_CACHE_MAX_ENTRIES) { + const oldestKey = remoteNameListingCache.keys().next().value + if (oldestKey === undefined) { + return + } + remoteNameListingCache.delete(oldestKey) + } +} + +function listingGitConfigContext( + repoPath: string, + connectionId?: string | null, + localGitOptions: RemoteNameListingGitOptions = {} +): { repoPath: string; connectionId: string | null; wslDistro?: string } { + return { + repoPath, + connectionId: connectionId ?? null, + ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}) + } +} + +/** + * `git remote` names for one repo/runtime. Failed listings are not cached: a + * missed `upstream` would otherwise send issue/PR resolvers to origin on a + * contributor clone (#7331). + */ +export async function listCachedRemoteNames( + repoPath: string, + connectionId?: string | null, + localGitOptions: RemoteNameListingGitOptions = {} +): Promise { + const cacheKey = remoteNameListingCacheKey(repoPath, connectionId, localGitOptions) + const now = Date.now() + pruneRemoteNameListingCache(now) + const cached = remoteNameListingCache.get(cacheKey) + if (cached && cached.expiresAt > now) { + if (cached.configSignature !== undefined) { + const currentSignature = await readLocalGitConfigSignature( + listingGitConfigContext(repoPath, connectionId, localGitOptions) + ) + if (currentSignature === cached.configSignature) { + return cached.remotes + } + remoteNameListingCache.delete(cacheKey) + } else { + return cached.remotes + } + } + + return runCoalescedProbe(remoteNameListingInFlight, cacheKey, async (ownsKey) => { + const configContext = listingGitConfigContext(repoPath, connectionId, localGitOptions) + const configSignatureBefore = await readLocalGitConfigSignature(configContext) + const remotes = await listUncachedRemoteNames(repoPath, connectionId, localGitOptions) + if (remotes === null) { + return null + } + if (ownsKey()) { + const configSignatureAfter = await readLocalGitConfigSignature(configContext) + const configSignature = + configSignatureBefore !== undefined && configSignatureBefore === configSignatureAfter + ? configSignatureAfter + : undefined + remoteNameListingCache.set(cacheKey, { + remotes, + expiresAt: + Date.now() + + (configSignature + ? SIGNED_REMOTE_NAME_LISTING_TTL_MS + : UNSIGNED_REMOTE_NAME_LISTING_TTL_MS), + ...(configSignature ? { configSignature } : {}) + }) + pruneRemoteNameListingCache(Date.now()) + } + return remotes + }) +} + +async function listUncachedRemoteNames( + repoPath: string, + connectionId?: string | null, + localGitOptions: RemoteNameListingGitOptions = {} +): Promise { + if (connectionId) { + const provider = getSshGitProvider(connectionId) + if (!provider) { + return null + } + try { + const { stdout } = await provider.exec(['remote'], repoPath, { + signal: AbortSignal.timeout(REMOTE_URL_PROBE_TIMEOUT_MS) + }) + return parseRemoteNames(stdout) + } catch { + return null + } + } + try { + const { stdout } = await gitExecFileAsync(['remote'], { + cwd: repoPath, + timeout: REMOTE_URL_PROBE_TIMEOUT_MS, + ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}), + ...(localGitOptions.admissionTier ? { admissionTier: localGitOptions.admissionTier } : {}) + }) + return parseRemoteNames(stdout) + } catch { + return null + } +} + +/** Probe a named remote only when listing says it exists, or listing failed. */ +export async function shouldProbeGitRemote( + repoPath: string, + remoteName: string, + connectionId?: string | null, + localGitOptions: RemoteNameListingGitOptions = {} +): Promise { + const remotes = await listCachedRemoteNames(repoPath, connectionId, localGitOptions) + return remotes === null || remotes.includes(remoteName) +} diff --git a/src/main/github/client-work-items.test.ts b/src/main/github/client-work-items.test.ts index 642b5c2aa4e..31878409768 100644 --- a/src/main/github/client-work-items.test.ts +++ b/src/main/github/client-work-items.test.ts @@ -116,6 +116,7 @@ import { _resetOwnerRepoCache } from './client' import { GITHUB_WORK_ITEMS_QUERY_MAX_BYTES } from '../../shared/github/work-items-query-bounds' +import { _resetRemoteNameListingCache } from '../git/remote-name-listing' import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' @@ -156,6 +157,7 @@ describe('listWorkItems', () => { remoteName === 'origin' ? getOwnerRepoMock(repoPath, connectionId, opts) : null ) _resetOwnerRepoCache() + _resetRemoteNameListingCache() _resetMergeQueueCacheForTests() }) @@ -383,6 +385,31 @@ describe('listWorkItems', () => { ) }) + it('skips upstream PR source probing when the clone only has origin', async () => { + getIssueOwnerRepoMock.mockResolvedValue(null) + getOwnerRepoMock.mockResolvedValue({ owner: 'fork', repo: 'orca' }) + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'origin\n' }) + ghExecFileAsyncMock.mockResolvedValue({ stdout: '[]' }) + + await expect(listWorkItems('/origin-only-repo', 10, 'is:pr')).resolves.toMatchObject({ + items: [], + sources: { + issues: null, + prs: { owner: 'fork', repo: 'orca' }, + originCandidate: { owner: 'fork', repo: 'orca' }, + upstreamCandidate: null + } + }) + + expect(getOwnerRepoForRemoteMock.mock.calls.map(([, remote]) => remote)).not.toContain( + 'upstream' + ) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['remote'], + expect.objectContaining({ cwd: '/origin-only-repo' }) + ) + }) + it('rejects oversized queries before resolving repo sources or executing gh', async () => { const secret = 'main-github-work-items-secret' const oversizedQuery = secret + 'x'.repeat(GITHUB_WORK_ITEMS_QUERY_MAX_BYTES) diff --git a/src/main/github/client/list/work-item-list-request.ts b/src/main/github/client/list/work-item-list-request.ts index 2bad4599237..20c1d482f1a 100644 --- a/src/main/github/client/list/work-item-list-request.ts +++ b/src/main/github/client/list/work-item-list-request.ts @@ -2,6 +2,7 @@ import type { ClassifiedError } from '../../../../shared/classified-error' import type { IssueSourcePreference } from '../../../../shared/repo-types' import type { ParsedTaskQuery } from '../../../../shared/task-query' import { GITHUB_WORK_ITEMS_SSH_REMOTE_REQUIRED_MESSAGE } from '../../../../shared/work-items' +import { shouldProbeGitRemote } from '../../../git/remote-name-listing' import type { LocalGitExecOptions, OwnerRepo } from '../../gh-utils' import { getGitHubApiRepositoryForRemote, @@ -134,9 +135,27 @@ export async function resolvePrWorkItemSource( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { + const originCandidatePromise = getOriginGitHubApiRepository( + repoPath, + connectionId, + localGitOptions + ) + // Why: PR list/count polling must not spawn a failing upstream lookup on + // origin-only clones, while still preserving upstream-first resolution when + // the remote is configured or remote discovery fails open. + const upstreamCandidatePromise = shouldProbeGitRemote( + repoPath, + 'upstream', + connectionId, + localGitOptions + ).then((shouldProbe) => + shouldProbe + ? getGitHubApiRepositoryForRemote(repoPath, 'upstream', connectionId, localGitOptions) + : null + ) const [originCandidate, upstreamCandidate] = await Promise.all([ - getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions), - getGitHubApiRepositoryForRemote(repoPath, 'upstream', connectionId, localGitOptions) + originCandidatePromise, + upstreamCandidatePromise ]) // Why: fork-contribution PRs live on the upstream repo (the fork's own PR // list is almost always empty), so 'auto' resolves upstream-first exactly diff --git a/src/main/github/default-branch-stale-pr.test.ts b/src/main/github/default-branch-stale-pr.test.ts index fde4b6b8b84..4361c9759e7 100644 --- a/src/main/github/default-branch-stale-pr.test.ts +++ b/src/main/github/default-branch-stale-pr.test.ts @@ -278,7 +278,13 @@ describe('issue #9171: default-branch checkout must not attach a stale non-open expect(pr?.number).toBe(8) expect(pr?.state).toBe('open') // Open results never consult git for the default branch (lazy resolution). - expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + // Remote-name listing is a separate concern from default-branch resolution, + // so allow it and keep every other git command forbidden here. + expect( + gitExecFileAsyncMock.mock.calls + .map(([args]) => args[0]) + .filter((command) => command !== 'remote') + ).toEqual([]) }) it('keeps a CLOSED PR on a feature branch visible (behavior preserved)', async () => { diff --git a/src/main/github/gh-utils.test.ts b/src/main/github/gh-utils.test.ts index ba19fc13fda..5400eaeb565 100644 --- a/src/main/github/gh-utils.test.ts +++ b/src/main/github/gh-utils.test.ts @@ -21,6 +21,7 @@ vi.mock('../providers/ssh-git-dispatch', () => ({ getSshGitProvider: getSshGitProviderMock })) +import { _resetRemoteNameListingCache } from '../git/remote-name-listing' import { _getOwnerRepoCacheSize, _resetOwnerRepoCache, @@ -40,6 +41,35 @@ import { } from './local-git-config-signature' import { GITHUB_SEARCH_RESULT_WINDOW_ERROR_PATTERN } from '../../shared/github/work-items-query-bounds' +function mockGitRemoteCommands(remotes: Record): void { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote' && args[1] !== 'get-url') { + return { stdout: `${Object.keys(remotes).join('\n')}\n` } + } + if (args[0] === 'remote' && args[1] === 'get-url') { + const url = remotes[args[2] ?? ''] + if (!url) { + throw new Error(`fatal: No such remote '${args[2]}'`) + } + return { stdout: url } + } + throw new Error(`unexpected git ${args.join(' ')}`) + }) +} + +function gitRemoteGetUrlCalls(remoteName: string): unknown[][] { + return gitExecFileAsyncMock.mock.calls.filter( + ([args]) => + Array.isArray(args) && args[0] === 'remote' && args[1] === 'get-url' && args[2] === remoteName + ) +} + +function gitRemoteListCalls(): unknown[][] { + return gitExecFileAsyncMock.mock.calls.filter( + ([args]) => Array.isArray(args) && args[0] === 'remote' && args[1] !== 'get-url' + ) +} + describe('github owner/repo resolution', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() @@ -47,6 +77,7 @@ describe('github owner/repo resolution', () => { getSshGitProviderGenerationMock.mockReturnValue(0) getSshGitProviderMock.mockReset() _resetOwnerRepoCache() + _resetRemoteNameListingCache() __resetLocalGitConfigSignatureCacheForTests() }) @@ -97,57 +128,46 @@ describe('github owner/repo resolution', () => { }) it('prefers upstream for PR owner/repo resolution (#7331)', async () => { - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'git@github.com:stablyai/orca.git\n' + mockGitRemoteCommands({ + origin: 'git@github.com:fork/orca.git\n', + upstream: 'git@github.com:stablyai/orca.git\n' }) await expect(getOwnerRepo('/repo')).resolves.toEqual({ owner: 'stablyai', repo: 'orca' }) - expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['remote', 'get-url', 'upstream'], { - cwd: '/repo', - timeout: 30_000 - }) + expect(gitRemoteGetUrlCalls('upstream')).toHaveLength(1) }) - it('resolves GitHub HTTPS origin remotes with user info and a default port', async () => { - gitExecFileAsyncMock - .mockRejectedValueOnce(new Error("fatal: No such remote 'upstream'")) - .mockResolvedValueOnce({ - stdout: 'https://alice@github.com:443/acme/widgets.git\n' - }) + it('does not spawn git remote get-url upstream on an origin-only clone', async () => { + mockGitRemoteCommands({ + origin: 'https://alice@github.com:443/acme/widgets.git\n' + }) await expect(getOwnerRepo('/repo')).resolves.toEqual({ owner: 'acme', repo: 'widgets' }) - expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['remote', 'get-url', 'origin'], { - cwd: '/repo', - timeout: 30_000 - }) + await expect(getOwnerRepo('/repo')).resolves.toEqual({ owner: 'acme', repo: 'widgets' }) + expect(gitRemoteListCalls()).toHaveLength(1) + expect(gitRemoteGetUrlCalls('upstream')).toHaveLength(0) + expect(gitRemoteGetUrlCalls('origin')).toHaveLength(1) }) it('prefers upstream for issue owner/repo resolution', async () => { - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'git@github.com:stablyai/orca.git\n' + mockGitRemoteCommands({ + origin: 'git@github.com:fork/orca.git\n', + upstream: 'git@github.com:stablyai/orca.git\n' }) await expect(getIssueOwnerRepo('/repo')).resolves.toEqual({ owner: 'stablyai', repo: 'orca' }) - expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['remote', 'get-url', 'upstream'], { - cwd: '/repo', - timeout: 30_000 - }) + expect(gitRemoteGetUrlCalls('upstream')).toHaveLength(1) }) - it('falls back to origin when upstream is missing or non-GitHub', async () => { - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'git@example.com:stablyai/orca.git\n' }) - .mockResolvedValueOnce({ stdout: 'git@github.com:fork/orca.git\n' }) + it('falls back to origin when upstream is present but non-GitHub', async () => { + mockGitRemoteCommands({ + origin: 'git@github.com:fork/orca.git\n', + upstream: 'git@example.com:stablyai/orca.git\n' + }) await expect(getIssueOwnerRepo('/repo')).resolves.toEqual({ owner: 'fork', repo: 'orca' }) - expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith(1, ['remote', 'get-url', 'upstream'], { - cwd: '/repo', - timeout: 30_000 - }) - expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith(2, ['remote', 'get-url', 'origin'], { - cwd: '/repo', - timeout: 30_000 - }) + expect(gitRemoteGetUrlCalls('upstream')).toHaveLength(1) + expect(gitRemoteGetUrlCalls('origin')).toHaveLength(1) }) it('does not mix origin and upstream cache entries for the same repo path', async () => { @@ -193,6 +213,9 @@ describe('github owner/repo resolution', () => { it('resolves SSH repo remotes through the registered SSH git provider', async () => { const sshProvider = { exec: vi.fn(async (args: string[]) => { + if (args[0] === 'remote' && args[1] !== 'get-url') { + return { stdout: 'origin\n', stderr: '' } + } if (args[2] === 'upstream') { throw new Error("fatal: No such remote 'upstream'") } @@ -219,9 +242,14 @@ describe('github owner/repo resolution', () => { it('keeps local and SSH owner/repo cache entries separate for the same path', async () => { const sshProvider = { - exec: vi.fn().mockResolvedValue({ stdout: 'git@github.com:remote/orca.git\n', stderr: '' }) + exec: vi.fn(async (args: string[]) => { + if (args[0] === 'remote' && args[1] !== 'get-url') { + return { stdout: 'origin\n', stderr: '' } + } + return { stdout: 'git@github.com:remote/orca.git\n', stderr: '' } + }) } - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'git@github.com:local/orca.git\n' }) + mockGitRemoteCommands({ origin: 'git@github.com:local/orca.git\n' }) getSshGitProviderMock.mockReturnValue(sshProvider) await expect(getOwnerRepo('/repo')).resolves.toEqual({ owner: 'local', repo: 'orca' }) @@ -231,6 +259,9 @@ describe('github owner/repo resolution', () => { it('keeps local host and local WSL owner/repo cache entries separate for the same path', async () => { gitExecFileAsyncMock.mockImplementation( async (args: string[], options: { wslDistro?: string } = {}) => { + if (args[0] === 'remote' && args[1] !== 'get-url') { + return { stdout: 'origin\n' } + } if (args[2] === 'upstream') { throw new Error("fatal: No such remote 'upstream'") } @@ -252,8 +283,9 @@ describe('github owner/repo resolution', () => { repo: 'orca' }) - // 2 runtimes x (1 upstream miss + 1 origin hit); repeat WSL call is cached. + // 2 runtimes x (1 remote list + 1 origin hit); repeat WSL call is cached. expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(4) + expect(gitRemoteGetUrlCalls('upstream')).toHaveLength(0) expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['remote', 'get-url', 'origin'], { cwd: '/repo', timeout: 30_000 @@ -272,14 +304,20 @@ describe('github owner/repo resolution', () => { gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'git@github.com:stablyai/orca.git\n' }) - await expect(getOwnerRepo('/repo-a')).resolves.toEqual({ owner: 'stablyai', repo: 'orca' }) + await expect(getOwnerRepoForRemote('/repo-a', 'origin')).resolves.toEqual({ + owner: 'stablyai', + repo: 'orca' + }) expect(_getOwnerRepoCacheSize()).toBe(1) nowSpy.mockReturnValue(32_000) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'git@github.com:acme/widgets.git\n' }) - await expect(getOwnerRepo('/repo-b')).resolves.toEqual({ owner: 'acme', repo: 'widgets' }) + await expect(getOwnerRepoForRemote('/repo-b', 'origin')).resolves.toEqual({ + owner: 'acme', + repo: 'widgets' + }) expect(_getOwnerRepoCacheSize()).toBe(1) expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) @@ -289,20 +327,38 @@ describe('github owner/repo resolution', () => { }) it('resolves PR candidates as upstream then origin and de-dupes matching slugs', async () => { - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'git@github.com:Acme/Orca.git\n' }) - .mockResolvedValueOnce({ stdout: 'git@github.com:acme/orca.git\n' }) + mockGitRemoteCommands({ + origin: 'git@github.com:acme/orca.git\n', + upstream: 'git@github.com:Acme/Orca.git\n' + }) await expect(resolvePRRepositoryCandidates('/repo')).resolves.toEqual({ candidates: [{ owner: 'Acme', repo: 'Orca' }], headRepo: { owner: 'acme', repo: 'orca' } }) + expect(gitRemoteGetUrlCalls('upstream')).toHaveLength(1) + }) + + it('does not spawn git remote get-url upstream for origin-only PR candidates', async () => { + mockGitRemoteCommands({ origin: 'git@github.com:fork/orca.git\n' }) + + await expect(resolvePRRepositoryCandidates('/repo')).resolves.toEqual({ + candidates: [{ owner: 'fork', repo: 'orca' }], + headRepo: { owner: 'fork', repo: 'orca' } + }) + await expect(resolvePRRepositoryCandidates('/repo')).resolves.toEqual({ + candidates: [{ owner: 'fork', repo: 'orca' }], + headRepo: { owner: 'fork', repo: 'orca' } + }) + expect(gitRemoteListCalls()).toHaveLength(1) + expect(gitRemoteGetUrlCalls('upstream')).toHaveLength(0) }) it('ignores non-GitHub upstream while keeping origin as the head repo', async () => { - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'git@example.com:Acme/Orca.git\n' }) - .mockResolvedValueOnce({ stdout: 'git@github.com:fork/orca.git\n' }) + mockGitRemoteCommands({ + origin: 'git@github.com:fork/orca.git\n', + upstream: 'git@example.com:Acme/Orca.git\n' + }) await expect(resolvePRRepositoryCandidates('/repo')).resolves.toEqual({ candidates: [{ owner: 'fork', repo: 'orca' }], @@ -703,23 +759,27 @@ describe('resolveIssueSource', () => { gitExecFileAsyncMock.mockReset() getSshGitProviderMock.mockReset() _resetOwnerRepoCache() + _resetRemoteNameListingCache() }) it("'auto' + upstream exists → upstream, fellBack=false", async () => { - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'git@github.com:stablyai/orca.git\n' + mockGitRemoteCommands({ + origin: 'git@github.com:fork/orca.git\n', + upstream: 'git@github.com:stablyai/orca.git\n' }) await expect(resolveIssueSource('/repo', 'auto')).resolves.toEqual({ source: { owner: 'stablyai', repo: 'orca' }, fellBack: false }) + expect(gitRemoteGetUrlCalls('upstream')).toHaveLength(1) }) - it("'auto' + no upstream → origin, fellBack=false", async () => { - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'git@example.com:stablyai/orca.git\n' }) - .mockResolvedValueOnce({ stdout: 'git@github.com:solo/orca.git\n' }) + it("'auto' + no github upstream → origin, fellBack=false", async () => { + mockGitRemoteCommands({ + origin: 'git@github.com:solo/orca.git\n', + upstream: 'git@example.com:stablyai/orca.git\n' + }) await expect(resolveIssueSource('/repo', 'auto')).resolves.toEqual({ source: { owner: 'solo', repo: 'orca' }, @@ -779,8 +839,9 @@ describe('resolveIssueSource', () => { }) it('undefined preference is treated identically to auto', async () => { - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'git@github.com:stablyai/orca.git\n' + mockGitRemoteCommands({ + origin: 'git@github.com:fork/orca.git\n', + upstream: 'git@github.com:stablyai/orca.git\n' }) await expect(resolveIssueSource('/repo', undefined)).resolves.toEqual({ diff --git a/src/main/github/github-api-repository-remote-probe.ts b/src/main/github/github-api-repository-remote-probe.ts new file mode 100644 index 00000000000..31159a4a8c4 --- /dev/null +++ b/src/main/github/github-api-repository-remote-probe.ts @@ -0,0 +1,132 @@ +import type { GitHubApiRepository } from './github-api-repository' +import { + getOwnerRepoForRemote, + type GitHubRemoteIdentityProbeOptions, + type LocalGitExecOptions +} from './gh-utils' +import { + getEnterpriseGitHubRepoSlug, + getEnterpriseGitHubRepoSlugForRemote +} from './github-enterprise-repository' +import { + githubApiRepositoryProbeCacheKey, + resolveGitHubApiRepositoryProbe +} from './github-api-repository-probe' + +// Why: cache the uncached Enterprise remote probe used by hot paths. +const ORIGIN_REPO_CACHE_TTL_MS = 30_000 +const ORIGIN_REPO_CACHE_MAX_ENTRIES = 512 +const originRepoCache = new Map() +const originRepoInFlight = new Map>() + +/** @internal - exposed for tests only */ +export function _resetOriginGitHubApiRepositoryCache(): void { + originRepoCache.clear() + originRepoInFlight.clear() +} + +function pruneOriginRepoCache(now: number): void { + for (const [key, entry] of originRepoCache) { + if (entry.expiresAt <= now) { + originRepoCache.delete(key) + } + } + while (originRepoCache.size > ORIGIN_REPO_CACHE_MAX_ENTRIES) { + const oldestKey = originRepoCache.keys().next().value + if (oldestKey === undefined) { + return + } + originRepoCache.delete(oldestKey) + } +} + +/** + * Host-qualified repository identity for one remote: github.com remotes come + * from the cached slug parser; any other GitHub-shaped host is auth-gated so a + * non-GitHub forge never routes to the GitHub provider. + */ +export async function getGitHubApiRepositoryForRemote( + repoPath: string, + remoteName: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {}, + probeOptions: GitHubRemoteIdentityProbeOptions = {} +): Promise { + // Why: generic PR resolution prefers upstream, but this API represents the + // caller-selected remote exactly (#7331). + const requireVerifiedSshProbe = probeOptions.requireVerifiedSshProbe === true + const verifiedIdentityArgs = requireVerifiedSshProbe ? ([probeOptions] as const) : [] + const ownerRepo = await getOwnerRepoForRemote( + repoPath, + remoteName, + connectionId, + localGitOptions, + ...verifiedIdentityArgs + ) + if (ownerRepo) { + return { ...ownerRepo, host: 'github.com' } + } + const cacheKey = githubApiRepositoryProbeCacheKey( + repoPath, + remoteName, + connectionId, + localGitOptions, + requireVerifiedSshProbe + ) + const now = Date.now() + pruneOriginRepoCache(now) + const cached = originRepoCache.get(cacheKey) + if (cached && cached.expiresAt > now) { + return cached.value + } + const inFlight = originRepoInFlight.get(cacheKey) + if (inFlight) { + return inFlight + } + const probe = (async () => { + const enterpriseOptions = + Object.keys(localGitOptions).length > 0 ? { localGitExecOptions: localGitOptions } : {} + const verifiedEnterpriseArgs = requireVerifiedSshProbe ? ([true] as const) : [] + const slug = + remoteName === 'origin' + ? await getEnterpriseGitHubRepoSlug( + repoPath, + connectionId, + enterpriseOptions, + ...verifiedEnterpriseArgs + ) + : await getEnterpriseGitHubRepoSlugForRemote( + repoPath, + remoteName, + connectionId, + enterpriseOptions, + ...verifiedEnterpriseArgs + ) + // Why: undefined means the gh auth inventory could not be read. Caching it + // as a negative would turn a transient spawn failure into a 30-second miss. + if (slug !== undefined) { + originRepoCache.set(cacheKey, { + value: slug, + expiresAt: Date.now() + ORIGIN_REPO_CACHE_TTL_MS + }) + pruneOriginRepoCache(Date.now()) + } + return resolveGitHubApiRepositoryProbe(slug, requireVerifiedSshProbe) + })() + originRepoInFlight.set(cacheKey, probe) + try { + return await probe + } finally { + if (originRepoInFlight.get(cacheKey) === probe) { + originRepoInFlight.delete(cacheKey) + } + } +} + +export async function getOriginGitHubApiRepository( + repoPath: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): Promise { + return getGitHubApiRepositoryForRemote(repoPath, 'origin', connectionId, localGitOptions) +} diff --git a/src/main/github/github-api-repository.test.ts b/src/main/github/github-api-repository.test.ts index fb6512dab30..ad09e2d38ec 100644 --- a/src/main/github/github-api-repository.test.ts +++ b/src/main/github/github-api-repository.test.ts @@ -8,13 +8,15 @@ const { getOwnerRepoMock, getOwnerRepoForRemoteMock, getSshGitProviderGenerationMock, - isGitHubHostAuthenticatedMock + isGitHubHostAuthenticatedMock, + shouldProbeGitRemoteMock } = vi.hoisted(() => ({ getEnterpriseGitHubRepoSlugMock: vi.fn(), getOwnerRepoMock: vi.fn(), getOwnerRepoForRemoteMock: vi.fn(), getSshGitProviderGenerationMock: vi.fn(() => 0), - isGitHubHostAuthenticatedMock: vi.fn() + isGitHubHostAuthenticatedMock: vi.fn(), + shouldProbeGitRemoteMock: vi.fn(async () => true) })) vi.mock('../providers/ssh-git-dispatch', async (importOriginal) => ({ @@ -35,12 +37,18 @@ vi.mock('./github-enterprise-repository', async (importOriginal) => ({ isGitHubHostAuthenticated: isGitHubHostAuthenticatedMock })) +vi.mock('../git/remote-name-listing', () => ({ + shouldProbeGitRemote: shouldProbeGitRemoteMock +})) + import { _resetOriginGitHubApiRepositoryCache, getGitHubApiRepositoryForRemote, + getIssueGitHubApiRepository, getOriginGitHubApiRepository, githubHostExecOptions, resolveGitHubApiRepository, + resolveGitHubApiRepositoryCandidates, resolveGitHubRepoExecution } from './github-api-repository' @@ -51,6 +59,7 @@ beforeEach(() => { getOwnerRepoForRemoteMock.mockReset().mockResolvedValue(null) getSshGitProviderGenerationMock.mockReset().mockReturnValue(0) isGitHubHostAuthenticatedMock.mockReset().mockResolvedValue(false) + shouldProbeGitRemoteMock.mockReset().mockResolvedValue(true) }) describe('githubHostExecOptions', () => { @@ -346,3 +355,146 @@ describe('origin repository cache', () => { expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(2) }) }) + +describe('skip missing upstream remote probes', () => { + it('starts the issue origin probe before checking whether upstream exists', async () => { + let releaseRemoteProbe: (value: boolean) => void = () => undefined + shouldProbeGitRemoteMock.mockReturnValue( + new Promise((resolve) => { + releaseRemoteProbe = resolve + }) + ) + let originStarted = false + getOwnerRepoForRemoteMock.mockImplementation(async (_path, remote) => { + if (remote === 'origin') { + originStarted = true + return { owner: 'fork', repo: 'orca' } + } + return { owner: 'stablyai', repo: 'orca' } + }) + + const resultPromise = getIssueGitHubApiRepository('/repo') + expect(originStarted).toBe(true) + + releaseRemoteProbe(true) + await expect(resultPromise).resolves.toEqual({ + owner: 'stablyai', + repo: 'orca', + host: 'github.com' + }) + }) + + it('does not probe upstream for issue identity when that remote is absent', async () => { + shouldProbeGitRemoteMock.mockResolvedValue(false) + getOwnerRepoForRemoteMock.mockImplementation(async (_path, remote) => + remote === 'origin' ? { owner: 'acme', repo: 'widgets' } : null + ) + + await expect(getIssueGitHubApiRepository('/repo')).resolves.toEqual({ + owner: 'acme', + repo: 'widgets', + host: 'github.com' + }) + expect(getOwnerRepoForRemoteMock.mock.calls.map(([, remote]) => remote)).toEqual(['origin']) + }) + + it('still probes upstream for issue identity when that remote is present', async () => { + getOwnerRepoForRemoteMock.mockImplementation(async (_path, remote) => + remote === 'upstream' ? { owner: 'stablyai', repo: 'orca' } : { owner: 'fork', repo: 'orca' } + ) + + await expect(getIssueGitHubApiRepository('/repo')).resolves.toEqual({ + owner: 'stablyai', + repo: 'orca', + host: 'github.com' + }) + expect(getOwnerRepoForRemoteMock).toHaveBeenCalledWith('/repo', 'upstream', undefined, {}) + }) + + it('observes a rejected origin probe when upstream resolves the issue repository', async () => { + const originError = new Error('origin probe failed') + getOwnerRepoForRemoteMock.mockImplementation(async (_path, remote) => { + if (remote === 'origin') { + throw originError + } + return { owner: 'stablyai', repo: 'orca' } + }) + + await expect(getIssueGitHubApiRepository('/repo')).resolves.toEqual({ + owner: 'stablyai', + repo: 'orca', + host: 'github.com' + }) + }) + + it('preserves a rejected origin probe when upstream cannot resolve the issue repository', async () => { + const originError = new Error('origin probe failed') + getOwnerRepoForRemoteMock.mockImplementation(async (_path, remote) => { + if (remote === 'origin') { + throw originError + } + return null + }) + + await expect(getIssueGitHubApiRepository('/repo')).rejects.toBe(originError) + }) + + it('does not probe upstream for PR candidates when that remote is absent', async () => { + shouldProbeGitRemoteMock.mockResolvedValue(false) + getOwnerRepoForRemoteMock.mockResolvedValue({ owner: 'fork', repo: 'orca' }) + + await expect(resolveGitHubApiRepositoryCandidates('/repo')).resolves.toEqual({ + candidates: [{ owner: 'fork', repo: 'orca', host: 'github.com' }], + headRepo: { owner: 'fork', repo: 'orca', host: 'github.com' } + }) + expect(getOwnerRepoForRemoteMock.mock.calls.map(([, remote]) => remote)).toEqual(['origin']) + }) + + it('observes and propagates a verified origin probe failure while listing remotes', async () => { + let releaseRemoteProbe: (value: boolean) => void = () => undefined + shouldProbeGitRemoteMock.mockReturnValue( + new Promise((resolve) => { + releaseRemoteProbe = resolve + }) + ) + const originError = new Error('origin probe failed') + getOwnerRepoForRemoteMock.mockImplementation(async (_path, remote) => { + if (remote === 'origin') { + throw originError + } + return { owner: 'stablyai', repo: 'orca' } + }) + + const resultPromise = resolveGitHubApiRepositoryCandidates('/repo') + await vi.waitFor(() => + expect(getOwnerRepoForRemoteMock).toHaveBeenCalledWith( + '/repo', + 'origin', + undefined, + {}, + { requireVerifiedSshProbe: true } + ) + ) + releaseRemoteProbe(true) + + await expect(resultPromise).rejects.toBe(originError) + }) + + it('still probes upstream for PR candidates when that remote is present', async () => { + getOwnerRepoForRemoteMock.mockImplementation(async (_path, remote) => + remote === 'upstream' ? { owner: 'Acme', repo: 'Orca' } : { owner: 'acme', repo: 'orca' } + ) + + await expect(resolveGitHubApiRepositoryCandidates('/repo')).resolves.toEqual({ + candidates: [{ owner: 'Acme', repo: 'Orca', host: 'github.com' }], + headRepo: { owner: 'acme', repo: 'orca', host: 'github.com' } + }) + expect(getOwnerRepoForRemoteMock).toHaveBeenCalledWith( + '/repo', + 'upstream', + undefined, + {}, + { requireVerifiedSshProbe: true } + ) + }) +}) diff --git a/src/main/github/github-api-repository.ts b/src/main/github/github-api-repository.ts index 7e72ec9da9e..db3eccbc106 100644 --- a/src/main/github/github-api-repository.ts +++ b/src/main/github/github-api-repository.ts @@ -4,27 +4,19 @@ import { githubRepoIdentityKey, isDefaultGitHubHost } from '../../shared/github/repository-identity-key' -import { - getOwnerRepoForRemote, - ghRepoExecOptions, - githubRepoContext, - type GitHubRemoteIdentityProbeOptions, - type LocalGitExecOptions -} from './gh-utils' -import { - getEnterpriseGitHubRepoSlug, - getEnterpriseGitHubRepoSlugForRemote, - isGitHubHostAuthenticated -} from './github-enterprise-repository' +import { shouldProbeGitRemote } from '../git/remote-name-listing' +import { ghRepoExecOptions, githubRepoContext, type LocalGitExecOptions } from './gh-utils' +import { isGitHubHostAuthenticated } from './github-enterprise-repository' import { githubHostExecOptions } from './github-repository-host' import { isValidGitHubApiRepository, type GitHubApiRepositoryResolution } from './github-api-repository-validation' import { - githubApiRepositoryProbeCacheKey, - resolveGitHubApiRepositoryProbe -} from './github-api-repository-probe' + _resetOriginGitHubApiRepositoryCache, + getGitHubApiRepositoryForRemote, + getOriginGitHubApiRepository +} from './github-api-repository-remote-probe' export { githubHostExecOptions, @@ -40,123 +32,10 @@ export type GitHubRepoExecution = { ownerRepo: GitHubApiRepository | null ghOptions: GitHubRepoExecOptions } - -// Why: cache the uncached Enterprise remote probe used by hot paths. -const ORIGIN_REPO_CACHE_TTL_MS = 30_000 -const ORIGIN_REPO_CACHE_MAX_ENTRIES = 512 -const originRepoCache = new Map() -const originRepoInFlight = new Map>() - -/** @internal - exposed for tests only */ -export function _resetOriginGitHubApiRepositoryCache(): void { - originRepoCache.clear() - originRepoInFlight.clear() -} - -function pruneOriginRepoCache(now: number): void { - for (const [key, entry] of originRepoCache) { - if (entry.expiresAt <= now) { - originRepoCache.delete(key) - } - } - while (originRepoCache.size > ORIGIN_REPO_CACHE_MAX_ENTRIES) { - const oldestKey = originRepoCache.keys().next().value - if (oldestKey === undefined) { - return - } - originRepoCache.delete(oldestKey) - } -} - -/** - * Host-qualified repository identity for one remote: github.com remotes come - * from the cached slug parser; any other GitHub-shaped host is auth-gated so a - * non-GitHub forge never routes to the GitHub provider. - */ -export async function getGitHubApiRepositoryForRemote( - repoPath: string, - remoteName: string, - connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {}, - probeOptions: GitHubRemoteIdentityProbeOptions = {} -): Promise { - // Why: generic PR resolution prefers upstream, but this API represents the - // caller-selected remote exactly (#7331). - const requireVerifiedSshProbe = probeOptions.requireVerifiedSshProbe === true - const verifiedIdentityArgs = requireVerifiedSshProbe ? ([probeOptions] as const) : [] - const ownerRepo = await getOwnerRepoForRemote( - repoPath, - remoteName, - connectionId, - localGitOptions, - ...verifiedIdentityArgs - ) - if (ownerRepo) { - return { ...ownerRepo, host: 'github.com' } - } - const cacheKey = githubApiRepositoryProbeCacheKey( - repoPath, - remoteName, - connectionId, - localGitOptions, - requireVerifiedSshProbe - ) - const now = Date.now() - pruneOriginRepoCache(now) - const cached = originRepoCache.get(cacheKey) - if (cached && cached.expiresAt > now) { - return cached.value - } - const inFlight = originRepoInFlight.get(cacheKey) - if (inFlight) { - return inFlight - } - const probe = (async () => { - const enterpriseOptions = - Object.keys(localGitOptions).length > 0 ? { localGitExecOptions: localGitOptions } : {} - const verifiedEnterpriseArgs = requireVerifiedSshProbe ? ([true] as const) : [] - const slug = - remoteName === 'origin' - ? await getEnterpriseGitHubRepoSlug( - repoPath, - connectionId, - enterpriseOptions, - ...verifiedEnterpriseArgs - ) - : await getEnterpriseGitHubRepoSlugForRemote( - repoPath, - remoteName, - connectionId, - enterpriseOptions, - ...verifiedEnterpriseArgs - ) - // Why: undefined means the gh auth inventory could not be read. Caching it - // as a negative would turn a transient spawn failure into a 30-second miss. - if (slug !== undefined) { - originRepoCache.set(cacheKey, { - value: slug, - expiresAt: Date.now() + ORIGIN_REPO_CACHE_TTL_MS - }) - pruneOriginRepoCache(Date.now()) - } - return resolveGitHubApiRepositoryProbe(slug, requireVerifiedSshProbe) - })() - originRepoInFlight.set(cacheKey, probe) - try { - return await probe - } finally { - if (originRepoInFlight.get(cacheKey) === probe) { - originRepoInFlight.delete(cacheKey) - } - } -} - -export async function getOriginGitHubApiRepository( - repoPath: string, - connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} -): Promise { - return getGitHubApiRepositoryForRemote(repoPath, 'origin', connectionId, localGitOptions) +export { + _resetOriginGitHubApiRepositoryCache, + getGitHubApiRepositoryForRemote, + getOriginGitHubApiRepository } /** Hosted mirror of getIssueOwnerRepo: issues prefer `upstream` over `origin`. */ @@ -165,16 +44,26 @@ export async function getIssueGitHubApiRepository( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const upstream = await getGitHubApiRepositoryForRemote( + const originPromise = getGitHubApiRepositoryForRemote( repoPath, - 'upstream', + 'origin', connectionId, localGitOptions + ).then( + (value) => ({ status: 'fulfilled' as const, value }), + (reason: unknown) => ({ status: 'rejected' as const, reason }) ) + const upstream = (await shouldProbeGitRemote(repoPath, 'upstream', connectionId, localGitOptions)) + ? await getGitHubApiRepositoryForRemote(repoPath, 'upstream', connectionId, localGitOptions) + : null if (upstream) { return upstream } - return getGitHubApiRepositoryForRemote(repoPath, 'origin', connectionId, localGitOptions) + const origin = await originPromise + if (origin.status === 'rejected') { + throw origin.reason + } + return origin.value } export type GitHubApiRepositoryCandidates = { @@ -188,14 +77,36 @@ export async function resolveGitHubApiRepositoryCandidates( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const [upstream, origin] = await Promise.all([ - getGitHubApiRepositoryForRemote(repoPath, 'upstream', connectionId, localGitOptions, { + const originPromise = getGitHubApiRepositoryForRemote( + repoPath, + 'origin', + connectionId, + localGitOptions, + { requireVerifiedSshProbe: true - }), - getGitHubApiRepositoryForRemote(repoPath, 'origin', connectionId, localGitOptions, { - requireVerifiedSshProbe: true - }) + } + ).then( + (value) => ({ status: 'fulfilled' as const, value }), + (reason: unknown) => ({ status: 'rejected' as const, reason }) + ) + const probeUpstream = await shouldProbeGitRemote( + repoPath, + 'upstream', + connectionId, + localGitOptions + ) + const [upstream, originResult] = await Promise.all([ + probeUpstream + ? getGitHubApiRepositoryForRemote(repoPath, 'upstream', connectionId, localGitOptions, { + requireVerifiedSshProbe: true + }) + : null, + originPromise ]) + if (originResult.status === 'rejected') { + throw originResult.reason + } + const origin = originResult.value const seen = new Set() const candidates: GitHubApiRepository[] = [] for (const candidate of [upstream, origin]) { diff --git a/src/main/github/github-owner-repo-selection.ts b/src/main/github/github-owner-repo-selection.ts index 39cd0d2da0e..2e05613d0b8 100644 --- a/src/main/github/github-owner-repo-selection.ts +++ b/src/main/github/github-owner-repo-selection.ts @@ -1,5 +1,6 @@ import type { IssueSourcePreference } from '../../shared/repo-types' import { githubRepoIdentityKey } from '../../shared/github/repository-identity-key' +import { shouldProbeGitRemote } from '../git/remote-name-listing' import { getOwnerRepoForRemote, type LocalGitExecOptions, @@ -12,11 +13,19 @@ export async function getOwnerRepo( localGitOptions: LocalGitExecOptions = {} ): Promise { // Why: on a fork checkout PRs live on the upstream parent, not origin (#7331). - const upstream = await getOwnerRepoForRemote(repoPath, 'upstream', connectionId, localGitOptions) - if (upstream) { - return upstream + const originPromise = getOwnerRepoForRemote(repoPath, 'origin', connectionId, localGitOptions) + if (await shouldProbeGitRemote(repoPath, 'upstream', connectionId, localGitOptions)) { + const upstream = await getOwnerRepoForRemote( + repoPath, + 'upstream', + connectionId, + localGitOptions + ) + if (upstream) { + return upstream + } } - return getOwnerRepoForRemote(repoPath, 'origin', connectionId, localGitOptions) + return originPromise } export const getIssueOwnerRepo = getOwnerRepo @@ -31,9 +40,18 @@ export async function resolvePRRepositoryCandidates( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { + const originPromise = getOwnerRepoForRemote(repoPath, 'origin', connectionId, localGitOptions) + const probeUpstream = await shouldProbeGitRemote( + repoPath, + 'upstream', + connectionId, + localGitOptions + ) const [upstream, origin] = await Promise.all([ - getOwnerRepoForRemote(repoPath, 'upstream', connectionId, localGitOptions), - getOwnerRepoForRemote(repoPath, 'origin', connectionId, localGitOptions) + probeUpstream + ? getOwnerRepoForRemote(repoPath, 'upstream', connectionId, localGitOptions) + : null, + originPromise ]) const seen = new Set() const candidates: OwnerRepo[] = [] diff --git a/src/main/github/github-repository-identity.fork-owner-repo.test.ts b/src/main/github/github-repository-identity.fork-owner-repo.test.ts index 5461b838877..e0cd7de6ca4 100644 --- a/src/main/github/github-repository-identity.fork-owner-repo.test.ts +++ b/src/main/github/github-repository-identity.fork-owner-repo.test.ts @@ -28,6 +28,7 @@ vi.mock('./local-git-config-signature', () => ({ readLocalGitConfigSignature: readLocalGitConfigSignatureMock })) +import { _resetRemoteNameListingCache } from '../git/remote-name-listing' import { getOwnerRepoForRemote, _resetOwnerRepoCache } from './github-repository-identity' import { getOwnerRepo, getIssueOwnerRepo } from './github-owner-repo-selection' import { getRepoUpstream } from './client' @@ -53,13 +54,17 @@ const REMOTE_URLS_BY_REPO: Record> = { beforeEach(() => { _resetOwnerRepoCache() + _resetRemoteNameListingCache() gitExecFileAsyncMock.mockReset() ghExecFileAsyncMock.mockReset() gitExecFileAsyncMock.mockImplementation( async (args: string[], options: { cwd?: string } = {}) => { - // getRemoteUrlForRepo calls: ['remote', 'get-url', ] + const configured = REMOTE_URLS_BY_REPO[options.cwd ?? ''] ?? {} + if (args[0] === 'remote' && args[1] !== 'get-url') { + return { stdout: `${Object.keys(configured).join('\n')}\n` } + } const remoteName = args[2] - const url = REMOTE_URLS_BY_REPO[options.cwd ?? '']?.[remoteName] + const url = configured[remoteName] if (!url) { const err = new Error(`fatal: No such remote '${remoteName}'`) as Error & { code?: number } err.code = 128 @@ -92,16 +97,22 @@ describe('issue #7331: fork PR owner/repo resolution', () => { expect(prRepo).toEqual({ owner: 'stablyai', repo: 'orca' }) }) - it('caches the missing-upstream probe so repeat lookups skip the git spawn', async () => { + it('skips git remote get-url upstream on origin-only clones and caches the listing', async () => { await getOwnerRepo(NON_FORK_PATH) - const upstreamProbes = (): number => - gitExecFileAsyncMock.mock.calls.filter(([args]) => args[2] === 'upstream').length - expect(upstreamProbes()).toBe(1) + const upstreamGetUrl = (): number => + gitExecFileAsyncMock.mock.calls.filter( + ([args]) => args[1] === 'get-url' && args[2] === 'upstream' + ).length + const listCalls = (): number => + gitExecFileAsyncMock.mock.calls.filter( + ([args]) => args[0] === 'remote' && args[1] !== 'get-url' + ).length + expect(upstreamGetUrl()).toBe(0) + expect(listCalls()).toBe(1) await getOwnerRepo(NON_FORK_PATH) - // Second lookup within the negative-cache TTL must not respawn git for - // the missing upstream remote. - expect(upstreamProbes()).toBe(1) + expect(upstreamGetUrl()).toBe(0) + expect(listCalls()).toBe(1) }) it('resolves the upstream parent for SSH-style remote URLs', async () => { diff --git a/src/main/gitlab/gitlab-project-ref-resolution.ts b/src/main/gitlab/gitlab-project-ref-resolution.ts index b64e8c3f44c..c337c01ae89 100644 --- a/src/main/gitlab/gitlab-project-ref-resolution.ts +++ b/src/main/gitlab/gitlab-project-ref-resolution.ts @@ -1,5 +1,6 @@ import { glabExecFileAsync } from '../git/runner' import type { GitAdmissionTier } from '../git/command-runner/git-exec-options' +import { shouldProbeGitRemote } from '../git/remote-name-listing' import { isTransientGitProbeError, readRemoteUrl } from '../git/remote-url-probe' import { NEGATIVE_ENTRY_TTL_MS } from '../git/remote-ref-probe-cache' import { getSshGitProviderGeneration } from '../providers/ssh-git-dispatch' @@ -180,17 +181,26 @@ export async function getIssueProjectRef( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const upstream = await getProjectRefForRemote( + const originPromise = getProjectRefForRemote( repoPath, - 'upstream', + 'origin', knownHosts, connectionId, localGitOptions ) - return ( - upstream ?? - getProjectRefForRemote(repoPath, 'origin', knownHosts, connectionId, localGitOptions) - ) + if (await shouldProbeGitRemote(repoPath, 'upstream', connectionId, localGitOptions)) { + const upstream = await getProjectRefForRemote( + repoPath, + 'upstream', + knownHosts, + connectionId, + localGitOptions + ) + if (upstream) { + return upstream + } + } + return originPromise } export type ResolvedIssueSource = { diff --git a/src/main/gitlab/gl-utils.test.ts b/src/main/gitlab/gl-utils.test.ts index 9e69b9e75cf..8222a1f919e 100644 --- a/src/main/gitlab/gl-utils.test.ts +++ b/src/main/gitlab/gl-utils.test.ts @@ -33,9 +33,39 @@ import { } from './gl-utils' import { GlabNonListResponseError } from './glab-api-response' import { registerSshGitProvider, unregisterSshGitProvider } from '../providers/ssh-git-dispatch' +import { _resetRemoteNameListingCache } from '../git/remote-name-listing' import { REMOTE_URL_PROBE_TIMEOUT_MS } from '../git/remote-url-probe' import { NEGATIVE_ENTRY_TTL_MS } from '../git/remote-ref-probe-cache' +function mockGitRemoteCommands(remotes: Record): void { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote' && args[1] !== 'get-url') { + return { stdout: `${Object.keys(remotes).join('\n')}\n` } + } + if (args[0] === 'remote' && args[1] === 'get-url') { + const url = remotes[args[2] ?? ''] + if (!url) { + throw new Error(`fatal: No such remote '${args[2]}'`) + } + return { stdout: url } + } + throw new Error(`unexpected git ${args.join(' ')}`) + }) +} + +function gitRemoteGetUrlCalls(remoteName: string): unknown[][] { + return gitExecFileAsyncMock.mock.calls.filter( + ([args]) => + Array.isArray(args) && args[0] === 'remote' && args[1] === 'get-url' && args[2] === remoteName + ) +} + +function gitRemoteListCalls(): unknown[][] { + return gitExecFileAsyncMock.mock.calls.filter( + ([args]) => Array.isArray(args) && args[0] === 'remote' && args[1] !== 'get-url' + ) +} + describe('gitlab project ref resolution', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() @@ -43,6 +73,7 @@ describe('gitlab project ref resolution', () => { sshExecMock.mockReset() unregisterSshGitProvider('conn-1') _resetProjectRefCache() + _resetRemoteNameListingCache() }) afterEach(() => { @@ -66,35 +97,53 @@ describe('gitlab project ref resolution', () => { }) it('prefers upstream for issue project ref resolution', async () => { - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'git@gitlab.com:stablyai/orca.git\n' + mockGitRemoteCommands({ + origin: 'git@gitlab.com:fork/orca.git\n', + upstream: 'git@gitlab.com:stablyai/orca.git\n' }) await expect(getIssueProjectRef('/repo')).resolves.toEqual({ host: 'gitlab.com', path: 'stablyai/orca' }) - expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['remote', 'get-url', 'upstream'], { - cwd: '/repo', - timeout: REMOTE_URL_PROBE_TIMEOUT_MS - }) + expect(gitRemoteGetUrlCalls('upstream')).toHaveLength(1) }) - it('falls back to origin when upstream is missing or non-GitLab', async () => { - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'git@example.com:stablyai/orca.git\n' }) - .mockResolvedValueOnce({ stdout: 'git@gitlab.com:fork/orca.git\n' }) + it('does not spawn git remote get-url upstream on an origin-only clone', async () => { + mockGitRemoteCommands({ origin: 'git@gitlab.com:fork/orca.git\n' }) await expect(getIssueProjectRef('/repo')).resolves.toEqual({ host: 'gitlab.com', path: 'fork/orca' }) + await expect(getIssueProjectRef('/repo')).resolves.toEqual({ + host: 'gitlab.com', + path: 'fork/orca' + }) + expect(gitRemoteListCalls()).toHaveLength(1) + expect(gitRemoteGetUrlCalls('upstream')).toHaveLength(0) + expect(gitRemoteGetUrlCalls('origin')).toHaveLength(1) + }) + + it('falls back to origin when upstream is present but non-GitLab', async () => { + mockGitRemoteCommands({ + origin: 'git@gitlab.com:fork/orca.git\n', + upstream: 'git@example.com:stablyai/orca.git\n' + }) + + await expect(getIssueProjectRef('/repo')).resolves.toEqual({ + host: 'gitlab.com', + path: 'fork/orca' + }) + expect(gitRemoteGetUrlCalls('upstream')).toHaveLength(1) + expect(gitRemoteGetUrlCalls('origin')).toHaveLength(1) }) it('does not mix origin and upstream cache entries for the same repo path', async () => { - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'git@gitlab.com:fork/orca.git\n' }) - .mockResolvedValueOnce({ stdout: 'git@gitlab.com:stablyai/orca.git\n' }) + mockGitRemoteCommands({ + origin: 'git@gitlab.com:fork/orca.git\n', + upstream: 'git@gitlab.com:stablyai/orca.git\n' + }) await expect(getProjectRef('/repo')).resolves.toEqual({ host: 'gitlab.com', @@ -355,23 +404,27 @@ describe('resolveIssueSource', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() _resetProjectRefCache() + _resetRemoteNameListingCache() }) it("'auto' + upstream exists → upstream, fellBack=false", async () => { - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'git@gitlab.com:stablyai/orca.git\n' + mockGitRemoteCommands({ + origin: 'git@gitlab.com:fork/orca.git\n', + upstream: 'git@gitlab.com:stablyai/orca.git\n' }) await expect(resolveIssueSource('/repo', 'auto')).resolves.toEqual({ source: { host: 'gitlab.com', path: 'stablyai/orca' }, fellBack: false }) + expect(gitRemoteGetUrlCalls('upstream')).toHaveLength(1) }) - it("'auto' + no upstream → origin, fellBack=false", async () => { - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'git@example.com:stablyai/orca.git\n' }) - .mockResolvedValueOnce({ stdout: 'git@gitlab.com:solo/orca.git\n' }) + it("'auto' + no gitlab upstream → origin, fellBack=false", async () => { + mockGitRemoteCommands({ + origin: 'git@gitlab.com:solo/orca.git\n', + upstream: 'git@example.com:stablyai/orca.git\n' + }) await expect(resolveIssueSource('/repo', 'auto')).resolves.toEqual({ source: { host: 'gitlab.com', path: 'solo/orca' }, @@ -407,8 +460,9 @@ describe('resolveIssueSource', () => { }) it('undefined preference is treated identically to auto', async () => { - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'git@gitlab.com:stablyai/orca.git\n' + mockGitRemoteCommands({ + origin: 'git@gitlab.com:fork/orca.git\n', + upstream: 'git@gitlab.com:stablyai/orca.git\n' }) await expect(resolveIssueSource('/repo', undefined)).resolves.toEqual({