diff --git a/src/main/git/runner-gh-host-args.test.ts b/src/main/git/runner-gh-host-args.test.ts index ef32a6f2b52..e4168dbaf9a 100644 --- a/src/main/git/runner-gh-host-args.test.ts +++ b/src/main/git/runner-gh-host-args.test.ts @@ -53,12 +53,22 @@ describe('applyGhHostToArgs', () => { ]) }) - it('qualifies the inline --repo= form for a GHES host', () => { + it('qualifies inline long and short repo forms for a GHES host', () => { expect(applyGhHostToArgs(['pr', 'list', '--repo=a/b'], 'github.acme-corp.com')).toEqual([ 'pr', 'list', '--repo=github.acme-corp.com/a/b' ]) + expect(applyGhHostToArgs(['pr', 'list', '-R=a/b'], 'github.acme-corp.com')).toEqual([ + 'pr', + 'list', + '-R=github.acme-corp.com/a/b' + ]) + expect(applyGhHostToArgs(['pr', 'list', '-Ra/b'], 'github.acme-corp.com')).toEqual([ + 'pr', + 'list', + '-Rgithub.acme-corp.com/a/b' + ]) }) it('leaves --repo untouched for the github.com host (gh default resolution)', () => { @@ -89,6 +99,15 @@ describe('applyGhHostToArgs', () => { 'github.acme-corp.com' ) ).toEqual(['pr', 'list', '--repo', 'github.acme-corp.com/a/b']) + expect( + applyGhHostToArgs( + ['pr', 'list', '-R=https://github.acme-corp.com/a/b'], + 'github.acme-corp.com' + ) + ).toEqual(['pr', 'list', '-R=https://github.acme-corp.com/a/b']) + expect( + applyGhHostToArgs(['pr', 'list', '-Rgithub.acme-corp.com/a/b'], 'github.acme-corp.com') + ).toEqual(['pr', 'list', '-Rgithub.acme-corp.com/a/b']) }) it('injects --hostname and qualifies --repo together for GHES api calls', () => { diff --git a/src/main/git/runner-gh-rate-limit-breaker.test.ts b/src/main/git/runner-gh-rate-limit-breaker.test.ts index 6b2633bf15f..6d089971b3c 100644 --- a/src/main/git/runner-gh-rate-limit-breaker.test.ts +++ b/src/main/git/runner-gh-rate-limit-breaker.test.ts @@ -89,9 +89,18 @@ describe('ghExecFileAsync rate-limit breaker', () => { env: { ...process.env, GH_HOST: 'github.acme-corp.com' } }) ).resolves.toMatchObject({ stdout: '[]' }) - await expect( - ghExecFileAsync(['api', 'repos/a/b/pulls'], { wslDistro: 'Ubuntu' }) - ).resolves.toMatchObject({ stdout: '[]' }) + const originalPlatform = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + try { + await expect( + ghExecFileAsync(['api', 'repos/a/b/pulls'], { wslDistro: 'Ubuntu' }) + ).resolves.toMatchObject({ stdout: '[]' }) + } finally { + Object.defineProperty(process, 'platform', { + configurable: true, + value: originalPlatform + }) + } expect(execFileMock).toHaveBeenCalledTimes(5) }) diff --git a/src/main/git/runner-wsl-gh-fallback.test.ts b/src/main/git/runner-wsl-gh-fallback.test.ts index c7710858838..26b58196cef 100644 --- a/src/main/git/runner-wsl-gh-fallback.test.ts +++ b/src/main/git/runner-wsl-gh-fallback.test.ts @@ -20,6 +20,10 @@ vi.mock('../wsl', async (importOriginal) => ({ })) import { ghExecFileAsync, glabExecFileAsync } from './runner' +import { _resetGhRateLimitBreaker } from './gh-rate-limit-breaker' + +const PRIMARY_RATE_LIMIT_STDERR = + 'gh: API rate limit exceeded for user ID 1775218. Please wait. (HTTP 403)' describe('ghExecFileAsync WSL fallback', () => { const originalPlatform = process.platform @@ -28,6 +32,7 @@ describe('ghExecFileAsync WSL fallback', () => { execFileMock.mockReset() getDefaultWslDistroMock.mockReset() getDefaultWslDistroMock.mockReturnValue(null) + _resetGhRateLimitBreaker() Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' @@ -35,6 +40,7 @@ describe('ghExecFileAsync WSL fallback', () => { }) afterEach(() => { + _resetGhRateLimitBreaker() Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform @@ -210,6 +216,43 @@ describe('ghExecFileAsync WSL fallback', () => { expect(execFileMock).toHaveBeenCalledTimes(2) }) + it('retries a host-pinned idempotent gh GraphQL query after host injection', async () => { + execFileMock + .mockImplementationOnce((_binary, _args, _options, callback) => { + callback( + Object.assign(new Error('HTTP 502 Bad Gateway'), { + stdout: '', + stderr: 'HTTP 502 Bad Gateway' + }) + ) + }) + .mockImplementationOnce((_binary, _args, _options, callback) => { + callback(null, { stdout: '{"data":{}}', stderr: '' }) + }) + + await expect( + ghExecFileAsync(['api', 'graphql', '-f', 'query=query { viewer { login } }'], { + host: 'github.acme-corp.com' + }) + ).resolves.toEqual({ stdout: '{"data":{}}', stderr: '' }) + + expect(execFileMock).toHaveBeenCalledTimes(2) + expect(execFileMock).toHaveBeenNthCalledWith( + 1, + 'gh', + [ + 'api', + '--hostname', + 'github.acme-corp.com', + 'graphql', + '-f', + 'query=query { viewer { login } }' + ], + expect.any(Object), + expect.any(Function) + ) + }) + it('does not retry non-idempotent gh API transient failures', async () => { execFileMock.mockImplementation((_binary, _args, _options, callback) => { callback( @@ -290,6 +333,63 @@ describe('ghExecFileAsync WSL fallback', () => { ) }) + it('checks a blocked WSL scope before repeating a native-to-WSL fallback', async () => { + getDefaultWslDistroMock.mockReturnValue('Ubuntu') + execFileMock.mockImplementation((binary, _args, _options, callback) => { + if (binary === 'gh') { + callback(Object.assign(new Error('spawn gh ENOENT'), { code: 'ENOENT', stderr: '' })) + return + } + callback( + Object.assign(new Error(PRIMARY_RATE_LIMIT_STDERR), { + stdout: '', + stderr: PRIMARY_RATE_LIMIT_STDERR + }) + ) + }) + + await expect(ghExecFileAsync(['api', 'repos/acme/widgets/pulls'])).rejects.toThrow('rate limit') + await expect(ghExecFileAsync(['api', 'repos/acme/widgets/pulls'])).rejects.toMatchObject({ + ghRateLimitBlocked: true + }) + + expect(execFileMock).toHaveBeenCalledTimes(3) + expect(execFileMock.mock.calls.map(([binary]) => binary)).toEqual(['gh', 'wsl.exe', 'gh']) + }) + + it('checks a blocked native scope before repeating a WSL-to-native fallback', async () => { + execFileMock.mockImplementation((binary, _args, _options, callback) => { + if (binary === 'wsl.exe') { + callback( + Object.assign(new Error('Command failed: wsl.exe'), { + stdout: '', + stderr: 'bash: line 1: gh: command not found\n' + }) + ) + return + } + callback( + Object.assign(new Error(PRIMARY_RATE_LIMIT_STDERR), { + stdout: '', + stderr: PRIMARY_RATE_LIMIT_STDERR + }) + ) + }) + + const options = { + cwd: String.raw`\\wsl.localhost\Ubuntu\home\jinwoo\stably\noqa` + } + await expect(ghExecFileAsync(['api', 'repos/acme/widgets/pulls'], options)).rejects.toThrow( + 'rate limit' + ) + await expect( + ghExecFileAsync(['api', 'repos/acme/widgets/pulls'], options) + ).rejects.toMatchObject({ ghRateLimitBlocked: true }) + + expect(execFileMock).toHaveBeenCalledTimes(3) + expect(execFileMock.mock.calls.map(([binary]) => binary)).toEqual(['wsl.exe', 'gh', 'wsl.exe']) + }) + it('does not retry non-idempotent glab transient failures', async () => { execFileMock.mockImplementation((_binary, _args, _options, callback) => { callback( diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index 98b9c0afdc6..65ff7501b3a 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -26,7 +26,8 @@ import { getGhRateLimitBlockedUntilMs, isGhPrimaryRateLimitStderr, isGhRateLimitProbe, - notifyGhPrimaryRateLimit + notifyGhPrimaryRateLimit, + type GhRateLimitBucket } from './gh-rate-limit-breaker' import { getDefaultWslDistro, parseWslPath, toWindowsWslPath, type WslPathInfo } from '../wsl' import { addWslEnvKeys } from '../wsl-env' @@ -1446,17 +1447,41 @@ export function applyGhHostToArgs(args: string[], host?: string): string[] { if (arg.startsWith('--repo=')) { return `--repo=${hostQualifiedGhRepoValue(arg.slice('--repo='.length), host)}` } + if (arg.startsWith('-R=')) { + return `-R=${hostQualifiedGhRepoValue(arg.slice('-R='.length), host)}` + } + if (arg.startsWith('-R') && arg.length > '-R'.length) { + return `-R${hostQualifiedGhRepoValue(arg.slice('-R'.length), host)}` + } return arg }) } function ghRateLimitScope(options: GhExecOptions, resolved: ResolvedCommand): string { - const distro = resolved.wsl?.distro ?? options.wslDistro - const runtime = distro ? `wsl:${distro.toLowerCase()}` : 'native' + const runtime = resolved.wsl ? `wsl:${resolved.wsl.distro.toLowerCase()}` : 'native' const host = options.host ?? options.env?.GH_HOST ?? process.env.GH_HOST ?? 'github.com' return `${runtime}:${host.toLowerCase()}` } +function assertGhRateLimitScopeAvailable( + options: GhExecOptions, + resolved: ResolvedCommand, + bucket: GhRateLimitBucket, + exemptProbe: boolean +): void { + if (exemptProbe) { + return + } + const blockedUntilMs = getGhRateLimitBlockedUntilMs( + bucket, + Date.now(), + ghRateLimitScope(options, resolved) + ) + if (blockedUntilMs !== null) { + throw createGhRateLimitBlockedError(bucket, blockedUntilMs) + } +} + /** * Async gh CLI execution. Drop-in replacement for * `execFileAsync('gh', args, { cwd, encoding, ... })`. @@ -1469,6 +1494,7 @@ export async function ghExecFileAsync( args: string[], options: GhExecOptions = {} ): Promise<{ stdout: string; stderr: string }> { + const idempotent = options.idempotent ?? argsLookIdempotent(args) args = applyGhHostToArgs(args, options.host) let resolved = resolveCommand('gh', args, options.cwd, options.wslDistro) // Why: while a bucket's primary rate limit is exhausted, every spawn would @@ -1477,17 +1503,8 @@ export async function ghExecFileAsync( // Scope by execution runtime and host so github.com, GHES, and WSL accounts // cannot block one another after an unrelated quota exhaustion. const rateLimitBucket = classifyGhRateLimitBucket(args) - const initialRateLimitScope = ghRateLimitScope(options, resolved) - if (!isGhRateLimitProbe(args)) { - const blockedUntilMs = getGhRateLimitBlockedUntilMs( - rateLimitBucket, - Date.now(), - initialRateLimitScope - ) - if (blockedUntilMs !== null) { - throw createGhRateLimitBlockedError(rateLimitBucket, blockedUntilMs) - } - } + const rateLimitProbe = isGhRateLimitProbe(args) + assertGhRateLimitScopeAvailable(options, resolved, rateLimitBucket, rateLimitProbe) let lastError: unknown let attemptedHostFallback = false let attemptedDefaultWslFallback = false @@ -1523,6 +1540,7 @@ export async function ghExecFileAsync( // global calls like rate_limit/auth do not carry a repo cwd to route by. resolved = wslResolved attemptedDefaultWslFallback = true + assertGhRateLimitScopeAvailable(options, resolved, rateLimitBucket, rateLimitProbe) attempt = -1 continue } @@ -1530,6 +1548,7 @@ export async function ghExecFileAsync( if (!attemptedHostFallback && canFallBackToHostGitHubCli('gh', args, resolved, stderr)) { resolved = resolveHostGitHubCli('gh', args) attemptedHostFallback = true + assertGhRateLimitScopeAvailable(options, resolved, rateLimitBucket, rateLimitProbe) attempt = -1 continue } @@ -1540,7 +1559,6 @@ export async function ghExecFileAsync( // a label). When the caller doesn't say, we auto-detect from argv — // explicit `-X ` and GraphQL `query=mutation …` are treated // as non-idempotent. See bug-scan finding 1. - const idempotent = options.idempotent ?? argsLookIdempotent(args) if (idempotent && !isLastAttempt && isTransientGhError(stderr)) { // Why: when the upstream surfaced a Retry-After (e.g. on a transient // 5xx that GitHub explicitly recommends backing off for), honor it diff --git a/src/main/github/client-file-viewed.test.ts b/src/main/github/client-file-viewed.test.ts index d30779d3a78..79ad4e6a752 100644 --- a/src/main/github/client-file-viewed.test.ts +++ b/src/main/github/client-file-viewed.test.ts @@ -75,7 +75,10 @@ describe('setPRFileViewed', () => { expect(args.find((arg: string) => arg.startsWith('query='))).toContain('markFileAsViewed') expect(args).toContain('pullRequestId=PR_kwDO123') expect(args).toContain('path=src/app.ts') - expect(ghExecFileAsyncMock.mock.calls[0][1]).toEqual({ cwd: '/repo-root' }) + expect(ghExecFileAsyncMock.mock.calls[0][1]).toEqual({ + cwd: '/repo-root', + host: 'github.com' + }) expect(releaseMock).toHaveBeenCalledTimes(1) }) @@ -111,6 +114,7 @@ describe('setPRFileViewed', () => { expect(ghExecFileAsyncMock.mock.calls[0][1]).toEqual({ cwd: '/repo-root', + host: 'github.com', wslDistro: 'Ubuntu' }) }) diff --git a/src/main/github/client-pr-check-details.test.ts b/src/main/github/client-pr-check-details.test.ts index 952006667f3..ff210f7bc7e 100644 --- a/src/main/github/client-pr-check-details.test.ts +++ b/src/main/github/client-pr-check-details.test.ts @@ -132,17 +132,17 @@ describe('getPRCheckDetails', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 1, ['api', 'repos/acme/widgets/check-runs/88'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 2, ['api', 'repos/acme/widgets/check-runs/88/annotations?per_page=20'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 3, ['api', 'repos/acme/widgets/actions/runs/77/jobs?per_page=100'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) }) @@ -209,7 +209,7 @@ describe('getPRCheckDetails', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 4, ['api', 'repos/acme/widgets/actions/jobs/8801/logs'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(4) }) diff --git a/src/main/github/client-pr-checks.test.ts b/src/main/github/client-pr-checks.test.ts index 2c7c509f4eb..82310af697b 100644 --- a/src/main/github/client-pr-checks.test.ts +++ b/src/main/github/client-pr-checks.test.ts @@ -422,12 +422,12 @@ describe('getPRChecks', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 2, ['api', '--cache', '60s', 'repos/acme/widgets/commits/head-oid/check-runs?per_page=100'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 3, ['api', '--cache', '60s', 'repos/acme/widgets/commits/head-oid/status?per_page=100'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) expect(checks).toEqual([ { @@ -505,7 +505,7 @@ describe('getPRChecks', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 3, ['pr', 'checks', '42', '--json', 'name,state,link', '--repo', 'acme/widgets'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) expect(checks).toEqual([ { @@ -542,7 +542,11 @@ describe('getPRChecks', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 2, ['api', '-X', 'POST', 'repos/acme/widgets/actions/runs/77/rerun-failed-jobs'], - { cwd: '/repo-root', env: { ...process.env, GH_PROMPT_DISABLED: '1' } } + { + cwd: '/repo-root', + env: { ...process.env, GH_PROMPT_DISABLED: '1' }, + host: 'github.com' + } ) }) @@ -611,7 +615,7 @@ describe('getPRChecks', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 2, ['pr', 'checks', '42', '--json', 'name,state,link', '--repo', 'acme/widgets'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) }) diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index adb34ca6bcc..2a3c102f0c0 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -3637,7 +3637,7 @@ describe('updatePRState', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledWith( ['pr', 'reopen', '3977', '--repo', 'stablyai/orca'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) expect(acquireMock).toHaveBeenCalledTimes(1) expect(releaseMock).toHaveBeenCalledTimes(1) @@ -3653,7 +3653,7 @@ describe('updatePRState', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledWith( ['pr', 'close', '3977', '--repo', 'stablyai/orca'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) }) @@ -3669,7 +3669,7 @@ describe('updatePRState', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledWith( ['pr', 'reopen', '3977', '--repo', 'stablyai/orca'], - {} + { host: 'github.com' } ) }) }) @@ -3758,12 +3758,12 @@ describe('GitHub GraphQL rate-limit guard', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 1, ['api', '--cache', '60s', 'repos/stablyai/orca/issues/7/comments?per_page=100'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 2, ['api', '--cache', '60s', 'repos/stablyai/orca/pulls/7/reviews?per_page=100'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) }) @@ -3813,20 +3813,21 @@ describe('GitHub GraphQL rate-limit guard', () => { '--json', 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' ], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 2, ['pr', 'merge', '7', '--squash', '--repo', 'stablyai/orca'], expect.objectContaining({ cwd: '/repo-root', - env: expect.objectContaining({ GH_PROMPT_DISABLED: '1' }) + env: expect.objectContaining({ GH_PROMPT_DISABLED: '1' }), + host: 'github.com' }) ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 3, ['pr', 'edit', '7', '--title', 'New title', '--repo', 'stablyai/orca'], - { cwd: '/repo-root' } + { cwd: '/repo-root', host: 'github.com' } ) }) @@ -3855,7 +3856,7 @@ describe('GitHub GraphQL rate-limit guard', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 1, ['pr', 'view', '7', '--json', 'id,headRefOid,baseRefName', '--repo', 'stablyai/orca'], - {} + { host: 'github.com' } ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 2, @@ -3870,14 +3871,16 @@ describe('GitHub GraphQL rate-limit guard', () => { 'expectedHeadOid=head-oid' ]), expect.objectContaining({ - env: expect.objectContaining({ GH_PROMPT_DISABLED: '1' }) + env: expect.objectContaining({ GH_PROMPT_DISABLED: '1' }), + host: 'github.com' }) ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 3, ['pr', 'merge', '7', '--disable-auto', '--repo', 'stablyai/orca'], expect.objectContaining({ - env: expect.objectContaining({ GH_PROMPT_DISABLED: '1' }) + env: expect.objectContaining({ GH_PROMPT_DISABLED: '1' }), + host: 'github.com' }) ) expect(ghExecFileAsyncMock.mock.calls[0]?.[1]).not.toHaveProperty('cwd') diff --git a/src/main/github/github-api-repository.test.ts b/src/main/github/github-api-repository.test.ts new file mode 100644 index 00000000000..b63ee1530e6 --- /dev/null +++ b/src/main/github/github-api-repository.test.ts @@ -0,0 +1,19 @@ +import { describe, expect, it } from 'vitest' + +import { githubHostExecOptions } from './github-api-repository' + +describe('githubHostExecOptions', () => { + it('pins every known repository host', () => { + expect(githubHostExecOptions({ owner: 'acme', repo: 'widgets', host: 'github.com' })).toEqual({ + host: 'github.com' + }) + expect( + githubHostExecOptions({ owner: 'acme', repo: 'widgets', host: 'github.acme-corp.com' }) + ).toEqual({ host: 'github.acme-corp.com' }) + }) + + it('does not invent a host when repository identity is unavailable or legacy', () => { + expect(githubHostExecOptions({ owner: 'acme', repo: 'widgets' })).toEqual({}) + expect(githubHostExecOptions(null)).toEqual({}) + }) +}) diff --git a/src/main/github/github-api-repository.ts b/src/main/github/github-api-repository.ts index 0484f1d03a5..cd7284dd90e 100644 --- a/src/main/github/github-api-repository.ts +++ b/src/main/github/github-api-repository.ts @@ -49,14 +49,13 @@ export function isGitHubDotComRepository(repository: GitHubApiRepository): boole return !repository.host || repository.host.toLowerCase() === 'github.com' } -// Why: the gh runner host-qualifies argv (`--hostname`, `HOST/OWNER/REPO`) -// from `options.host`, so merging this into a call's exec options is the one -// step a call site needs to target GHES. github.com stays unset to keep gh's -// default-host resolution (and existing argv) unchanged. +// Why: the gh runner host-qualifies argv from `options.host`, so every known +// host must be carried through. Pinning github.com prevents a process-level +// GH_HOST from silently redirecting an otherwise unambiguous API request. export function githubHostExecOptions(repository: GitHubApiRepository | null | undefined): { host?: string } { - return repository?.host && !isGitHubDotComRepository(repository) ? { host: repository.host } : {} + return repository?.host ? { host: repository.host } : {} } export function githubRepositoryWebHost(repository: GitHubApiRepository): string {