Fix gh CLI GHES host pinning and rate-limit scope leaks

- Pin `--host` on every gh call site so a process-level GH_HOST can't
  silently redirect requests, and qualify `-R`/`-R=` repo shorthand
  alongside the existing `--repo=` handling.
- Check the target scope for an active rate-limit block before each
  WSL/native or host fallback retry, not just on the initial attempt,
  so a blocked scope can't be hit again through a fallback path.
- Compute idempotency once per call instead of re-deriving it after
  fallback reassigns args.
This commit is contained in:
Jinjing
2026-07-16 22:32:04 -07:00
parent 39738e9492
commit 5487dff295
10 changed files with 220 additions and 45 deletions
+20 -1
View File
@@ -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', () => {
@@ -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)
})
+100
View File
@@ -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(
+33 -15
View File
@@ -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 <method>` 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
+5 -1
View File
@@ -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'
})
})
@@ -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)
})
+9 -5
View File
@@ -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' }
)
})
+14 -11
View File
@@ -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')
@@ -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({})
})
})
+4 -5
View File
@@ -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 {