diff --git a/src/main/git/runner-gh-host-args.test.ts b/src/main/git/runner-gh-host-args.test.ts new file mode 100644 index 00000000000..ef32a6f2b52 --- /dev/null +++ b/src/main/git/runner-gh-host-args.test.ts @@ -0,0 +1,106 @@ +import { describe, expect, it } from 'vitest' + +import { applyGhHostToArgs } from './runner' + +describe('applyGhHostToArgs', () => { + it('returns args unchanged when no host is given', () => { + const args = ['api', 'repos/a/b/pulls'] + expect(applyGhHostToArgs(args)).toBe(args) + expect(applyGhHostToArgs(['pr', 'list', '--repo', 'a/b'])).toEqual([ + 'pr', + 'list', + '--repo', + 'a/b' + ]) + }) + + it('injects --hostname right after `api` for any host', () => { + expect(applyGhHostToArgs(['api', 'rate_limit'], 'github.com')).toEqual([ + 'api', + '--hostname', + 'github.com', + 'rate_limit' + ]) + expect(applyGhHostToArgs(['api', 'repos/a/b/pulls'], 'github.acme-corp.com')).toEqual([ + 'api', + '--hostname', + 'github.acme-corp.com', + 'repos/a/b/pulls' + ]) + }) + + it('does not double-inject when --hostname is already present', () => { + expect( + applyGhHostToArgs(['api', '--hostname', 'github.com', 'rate_limit'], 'github.acme-corp.com') + ).toEqual(['api', '--hostname', 'github.com', 'rate_limit']) + expect( + applyGhHostToArgs(['api', '--hostname=github.com', 'rate_limit'], 'github.acme-corp.com') + ).toEqual(['api', '--hostname=github.com', 'rate_limit']) + }) + + it('qualifies bare owner/repo in --repo / -R 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' + ]) + }) + + it('qualifies the inline --repo= form 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' + ]) + }) + + it('leaves --repo untouched for the github.com host (gh default resolution)', () => { + expect(applyGhHostToArgs(['pr', 'list', '--repo', 'a/b'], 'github.com')).toEqual([ + 'pr', + 'list', + '--repo', + 'a/b' + ]) + expect(applyGhHostToArgs(['pr', 'list', '--repo', 'a/b'], 'GitHub.com')).toEqual([ + 'pr', + 'list', + '--repo', + 'a/b' + ]) + }) + + it('passes through URL and already-qualified 3-part --repo values', () => { + expect( + applyGhHostToArgs( + ['pr', 'list', '--repo', 'https://github.acme-corp.com/a/b'], + 'github.acme-corp.com' + ) + ).toEqual(['pr', 'list', '--repo', 'https://github.acme-corp.com/a/b']) + expect( + applyGhHostToArgs( + ['pr', 'list', '--repo', 'github.acme-corp.com/a/b'], + 'github.acme-corp.com' + ) + ).toEqual(['pr', 'list', '--repo', 'github.acme-corp.com/a/b']) + }) + + it('injects --hostname and qualifies --repo together for GHES api calls', () => { + expect( + applyGhHostToArgs(['api', 'repos/a/b/pulls', '--repo', 'a/b'], 'github.acme-corp.com') + ).toEqual([ + 'api', + '--hostname', + 'github.acme-corp.com', + 'repos/a/b/pulls', + '--repo', + 'github.acme-corp.com/a/b' + ]) + }) +}) 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 471e9df45ce..6b2633bf15f 100644 --- a/src/main/git/runner-gh-rate-limit-breaker.test.ts +++ b/src/main/git/runner-gh-rate-limit-breaker.test.ts @@ -75,12 +75,14 @@ describe('ghExecFileAsync rate-limit breaker', () => { mockGhFailure(PRIMARY_RATE_LIMIT_STDERR) await expect(ghExecFileAsync(['api', 'repos/a/b/pulls'])).rejects.toThrow() + // The breaker scope now comes from options.host / GH_HOST / wslDistro, not + // from sniffing argv — so an Enterprise host must be routed via options.host. mockGhSuccess('[]') await expect( - ghExecFileAsync(['api', '--hostname', 'github.acme-corp.com', 'repos/a/b/pulls']) + ghExecFileAsync(['api', 'repos/a/b/pulls'], { host: 'github.acme-corp.com' }) ).resolves.toMatchObject({ stdout: '[]' }) await expect( - ghExecFileAsync(['pr', 'list', '--repo', 'github.acme-corp.com/a/b']) + ghExecFileAsync(['pr', 'list', '--repo', 'a/b'], { host: 'github.acme-corp.com' }) ).resolves.toMatchObject({ stdout: '[]' }) await expect( ghExecFileAsync(['api', 'repos/a/b/pulls'], { diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index 55e28c9704f..6631754f751 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -1116,6 +1116,11 @@ type GhExecOptions = Omit & { cwd?: string wslDistro?: string idempotent?: boolean + // Why: `gh api` and `--repo OWNER/REPO` shorthand resolve against gh's + // default host, not the repo's remote. Carrying the host here lets the + // runner qualify every spawn once, so call sites can't silently fall back + // to github.com for GHES repos; it also scopes the rate-limit breaker. + host?: string } const NON_IDEMPOTENT_METHODS = new Set(['POST', 'PATCH', 'PUT', 'DELETE']) @@ -1253,42 +1258,54 @@ function nonInteractiveGhEnv(env: NodeJS.ProcessEnv = process.env): NodeJS.Proce } } -function ghFlagValue(args: readonly string[], names: readonly string[]): string | undefined { - for (const name of names) { - const index = args.indexOf(name) - if (index >= 0 && args[index + 1]) { - return args[index + 1] - } - const prefix = `${name}=` - const inline = args.find((arg) => arg.startsWith(prefix)) - if (inline) { - return inline.slice(prefix.length) - } - } - return undefined +function hasGhHostnameFlag(args: readonly string[]): boolean { + return args.some((arg) => arg === '--hostname' || arg.startsWith('--hostname=')) } -function ghRequestHost(args: readonly string[], env?: NodeJS.ProcessEnv): string { - const explicitHost = ghFlagValue(args, ['--hostname']) - if (explicitHost) { - return explicitHost.toLowerCase() +function hostQualifiedGhRepoValue(value: string, host: string): string { + // URLs and already-qualified HOST/OWNER/REPO values pass through untouched. + if (value.includes('://') || value.split('/').length !== 2) { + return value } - const repository = ghFlagValue(args, ['--repo', '-R']) - const repositoryParts = repository?.split('/') ?? [] - if (repositoryParts.length >= 3) { - return repositoryParts[0].toLowerCase() - } - return (env?.GH_HOST ?? process.env.GH_HOST ?? 'github.com').toLowerCase() + return `${host}/${value}` } -function ghRateLimitScope( - args: readonly string[], - options: GhExecOptions, - resolved: ResolvedCommand -): string { +/** + * Host-qualify a gh invocation from `options.host`: `--hostname` for `api` + * calls, `HOST/OWNER/REPO` for `--repo`/`-R` shorthand. SSH-backed repos run + * gh with no cwd, so this is their only host signal (#8312). + * + * @internal exported for tests. + */ +export function applyGhHostToArgs(args: string[], host?: string): string[] { + if (!host) { + return args + } + let result = args + if (result[0] === 'api' && !hasGhHostnameFlag(result)) { + result = ['api', '--hostname', host, ...result.slice(1)] + } + // github.com is gh's default resolution for bare OWNER/REPO shorthand. + if (host.toLowerCase() === 'github.com') { + return result + } + return result.map((arg, i) => { + const prev = result[i - 1] + if (prev === '--repo' || prev === '-R') { + return hostQualifiedGhRepoValue(arg, host) + } + if (arg.startsWith('--repo=')) { + return `--repo=${hostQualifiedGhRepoValue(arg.slice('--repo='.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' - return `${runtime}:${ghRequestHost(args, options.env)}` + const host = options.host ?? options.env?.GH_HOST ?? process.env.GH_HOST ?? 'github.com' + return `${runtime}:${host.toLowerCase()}` } /** @@ -1302,11 +1319,12 @@ export async function ghExecFileAsync( args: string[], options: GhExecOptions = {} ): Promise<{ stdout: string; stderr: string }> { + args = applyGhHostToArgs(args, options.host) let resolved = resolveCommand('gh', args, options.cwd, options.wslDistro) // Why: while a bucket is rate-limited every spawn returns 403 — fail fast; the probe is exempt so the breaker can learn the reset. // Why: scope by runtime and host so unrelated github.com, GHES, and WSL quotas cannot block each other. const rateLimitBucket = classifyGhRateLimitBucket(args) - const initialRateLimitScope = ghRateLimitScope(args, options, resolved) + const initialRateLimitScope = ghRateLimitScope(options, resolved) if (!isGhRateLimitProbe(args)) { const blockedUntilMs = getGhRateLimitBlockedUntilMs( rateLimitBucket, @@ -1335,7 +1353,7 @@ export async function ghExecFileAsync( lastError = err const { stderr } = extractExecError(err) if (isGhPrimaryRateLimitStderr(stderr)) { - notifyGhPrimaryRateLimit(rateLimitBucket, ghRateLimitScope(args, options, resolved)) + notifyGhPrimaryRateLimit(rateLimitBucket, ghRateLimitScope(options, resolved)) } if ( process.platform === 'win32' && diff --git a/src/main/github/client-create-pr.test.ts b/src/main/github/client-create-pr.test.ts index 34b0d801aeb..27b7247dc18 100644 --- a/src/main/github/client-create-pr.test.ts +++ b/src/main/github/client-create-pr.test.ts @@ -167,7 +167,7 @@ describe('createGitHubPullRequest', () => { expect(args[args.indexOf('--head') + 1]).toBe('my-branch') }) - it('host-qualifies --repo for a GHES remote so gh targets the Enterprise server (#8312)', async () => { + it('routes --repo to the Enterprise server via options.host for a GHES remote (#8312)', async () => { // github.com-only slug parsing misses GHES, so creation comes from the // enterprise resolver, which carries the host. getOwnerRepoMock.mockResolvedValueOnce(null) @@ -194,13 +194,14 @@ describe('createGitHubPullRequest', () => { url: 'https://github.acme-corp.com/team/orca/pull/7' }) - const [args] = ghExecFileAsyncMock.mock.calls[0] - // Bare "team/orca" would resolve against gh's default host (github.com); - // the host prefix pins the command to the Enterprise server. - expect(args[args.indexOf('--repo') + 1]).toBe('github.acme-corp.com/team/orca') + const [args, options] = ghExecFileAsyncMock.mock.calls[0] + // The runner host-qualifies argv at spawn time from options.host, so the + // mocked call sees a bare owner/repo plus the host in exec options. + expect(args[args.indexOf('--repo') + 1]).toBe('team/orca') + expect(options).toMatchObject({ host: 'github.acme-corp.com' }) }) - it('host-qualifies --repo for the GHES existing-PR fallback lookup (#8312)', async () => { + it('routes the GHES existing-PR fallback lookup through options.host (#8312)', async () => { getOwnerRepoMock.mockResolvedValue(null) getEnterpriseGitHubRepoSlugMock.mockResolvedValue({ owner: 'team', @@ -234,9 +235,10 @@ describe('createGitHubPullRequest', () => { existingReview: { number: 9, url: 'https://github.acme-corp.com/team/orca/pull/9' } }) - const [listArgs] = ghExecFileAsyncMock.mock.calls[1] + const [listArgs, listOptions] = ghExecFileAsyncMock.mock.calls[1] expect(listArgs).toEqual(expect.arrayContaining(['pr', 'list'])) - expect(listArgs[listArgs.indexOf('--repo') + 1]).toBe('github.acme-corp.com/team/orca') + expect(listArgs[listArgs.indexOf('--repo') + 1]).toBe('team/orca') + expect(listOptions).toMatchObject({ host: 'github.acme-corp.com' }) }) it('runs local WSL project pull request creation through the selected distro', async () => { diff --git a/src/main/github/client-file-viewed.test.ts b/src/main/github/client-file-viewed.test.ts index 45cd1146cbd..d30779d3a78 100644 --- a/src/main/github/client-file-viewed.test.ts +++ b/src/main/github/client-file-viewed.test.ts @@ -134,15 +134,13 @@ describe('setPRFileViewed', () => { }) ).resolves.toBe(true) + // The runner injects --hostname at spawn time from options.host, so the + // mocked call sees unqualified argv plus the Enterprise host in options. expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - expect.arrayContaining([ - 'api', - '--hostname', - 'github.acme-corp.com', - 'graphql', - 'pullRequestId=PR_enterprise' - ]), - {} + expect.arrayContaining(['api', 'graphql', 'pullRequestId=PR_enterprise']), + { host: 'github.acme-corp.com' } ) + const [args] = ghExecFileAsyncMock.mock.calls[0] + expect(args).not.toContain('--hostname') }) }) diff --git a/src/main/github/client-issue-source.test.ts b/src/main/github/client-issue-source.test.ts index a828a7759f7..b4ca512be72 100644 --- a/src/main/github/client-issue-source.test.ts +++ b/src/main/github/client-issue-source.test.ts @@ -44,7 +44,10 @@ vi.mock('./gh-utils', async () => { vi.mock('./rate-limit', () => ({ rateLimitGuard: rateLimitGuardMock, noteRateLimitSpend: noteRateLimitSpendMock, - getRateLimit: vi.fn(async () => ({ ok: false, error: 'not probed in tests' })) + getRateLimit: vi.fn(async () => ({ ok: false, error: 'not probed in tests' })), + repositoryRateLimitGuard: vi.fn(() => ({ blocked: false })), + noteRepositoryRateLimitSpend: vi.fn(), + spendsSharedGitHubComQuota: () => true })) import { countWorkItems, getWorkItem, listWorkItems, _resetOwnerRepoCache } from './client' diff --git a/src/main/github/client-pr-check-details.test.ts b/src/main/github/client-pr-check-details.test.ts index 9cacf42ab74..952006667f3 100644 --- a/src/main/github/client-pr-check-details.test.ts +++ b/src/main/github/client-pr-check-details.test.ts @@ -28,7 +28,10 @@ vi.mock('../git/runner', () => ({ vi.mock('./rate-limit', () => ({ rateLimitGuard: rateLimitGuardMock, - noteRateLimitSpend: vi.fn() + noteRateLimitSpend: vi.fn(), + repositoryRateLimitGuard: vi.fn(() => ({ blocked: false })), + noteRepositoryRateLimitSpend: vi.fn(), + spendsSharedGitHubComQuota: vi.fn(() => true) })) import { getPRCheckDetails, _resetOwnerRepoCache } from './client' @@ -212,9 +215,9 @@ describe('getPRCheckDetails', () => { }) it('isolates failed-job log tails for the same job ID on different GitHub hosts', async () => { - ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + ghExecFileAsyncMock.mockImplementation(async (args: string[], options?: { host?: string }) => { const endpoint = args.find((arg) => arg.startsWith('repos/')) ?? '' - const enterprise = args.includes('github.acme-corp.com') + const enterprise = options?.host === 'github.acme-corp.com' if (endpoint.endsWith('/check-runs/99')) { return { stdout: JSON.stringify({ @@ -264,6 +267,6 @@ describe('getPRCheckDetails', () => { args.some((arg) => arg.endsWith('/actions/jobs/9901/logs')) ) expect(logCalls).toHaveLength(2) - expect(logCalls[1]?.[0]).toEqual(expect.arrayContaining(['--hostname', 'github.acme-corp.com'])) + expect(logCalls[1]?.[1]).toEqual(expect.objectContaining({ host: 'github.acme-corp.com' })) }) }) diff --git a/src/main/github/client-pr-checks.test.ts b/src/main/github/client-pr-checks.test.ts index 673da049943..2c7c509f4eb 100644 --- a/src/main/github/client-pr-checks.test.ts +++ b/src/main/github/client-pr-checks.test.ts @@ -10,6 +10,9 @@ const { getRateLimitMock, rateLimitGuardMock, noteRateLimitSpendMock, + repositoryRateLimitGuardMock, + noteRepositoryRateLimitSpendMock, + spendsSharedGitHubComQuotaMock, acquireMock, releaseMock } = vi.hoisted(() => ({ @@ -31,6 +34,9 @@ const { getRateLimitMock: vi.fn(), rateLimitGuardMock: vi.fn(() => ({ blocked: false })), noteRateLimitSpendMock: vi.fn(), + repositoryRateLimitGuardMock: vi.fn(() => ({ blocked: false })), + noteRepositoryRateLimitSpendMock: vi.fn(), + spendsSharedGitHubComQuotaMock: vi.fn(() => true), acquireMock: vi.fn(), releaseMock: vi.fn() })) @@ -66,7 +72,10 @@ vi.mock('../git/runner', () => ({ vi.mock('./rate-limit', () => ({ getRateLimit: getRateLimitMock, rateLimitGuard: rateLimitGuardMock, - noteRateLimitSpend: noteRateLimitSpendMock + noteRateLimitSpend: noteRateLimitSpendMock, + repositoryRateLimitGuard: repositoryRateLimitGuardMock, + noteRepositoryRateLimitSpend: noteRepositoryRateLimitSpendMock, + spendsSharedGitHubComQuota: spendsSharedGitHubComQuotaMock })) import { getPRChecks, rerunPRChecks, _resetOwnerRepoCache } from './client' diff --git a/src/main/github/client-pr-local-runtime.test.ts b/src/main/github/client-pr-local-runtime.test.ts index a70d8bab9b8..8e19df99f56 100644 --- a/src/main/github/client-pr-local-runtime.test.ts +++ b/src/main/github/client-pr-local-runtime.test.ts @@ -93,7 +93,15 @@ vi.mock('./conflict-summary', () => ({ vi.mock('./rate-limit', () => ({ getRateLimit: getRateLimitMock, rateLimitGuard: rateLimitGuardMock, - noteRateLimitSpend: noteRateLimitSpendMock + noteRateLimitSpend: noteRateLimitSpendMock, + repositoryRateLimitGuard: vi.fn(() => ({ blocked: false })), + noteRepositoryRateLimitSpend: vi.fn(), + spendsSharedGitHubComQuota: ( + repository?: { host?: string } | null, + executionOptions?: { wslDistro?: string } + ) => + (!repository?.host || repository.host.toLowerCase() === 'github.com') && + !executionOptions?.wslDistro })) import { @@ -488,16 +496,14 @@ describe('GitHub PR local runtime routing', () => { const prViewCall = ghExecFileAsyncMock.mock.calls.find( ([args]) => args[0] === 'pr' && args[1] === 'view' ) - expect(prViewCall?.[0]).toEqual( - expect.arrayContaining(['--repo', 'github.acme-corp.com/team/orca']) - ) - expect(prViewCall?.[1]).toEqual({}) + // The runner host-qualifies argv at spawn time from options.host, so the + // mocked call sees the unqualified --repo plus the host in exec options. + expect(prViewCall?.[0]).toEqual(expect.arrayContaining(['--repo', 'team/orca'])) + expect(prViewCall?.[1]).toEqual({ host: 'github.acme-corp.com' }) const prCalls = ghExecFileAsyncMock.mock.calls.filter(([args]) => args[0] === 'pr') expect( prCalls.every( - ([args]) => - args.includes('--repo') && - args[args.indexOf('--repo') + 1] === 'github.acme-corp.com/team/orca' + ([args]) => args.includes('--repo') && args[args.indexOf('--repo') + 1] === 'team/orca' ) ).toBe(true) const apiCalls = ghExecFileAsyncMock.mock.calls.filter(([args]) => args[0] === 'api') @@ -505,8 +511,8 @@ describe('GitHub PR local runtime routing', () => { expect( apiCalls.every( ([args, options]) => - args.includes('--hostname') && - args[args.indexOf('--hostname') + 1] === 'github.acme-corp.com' && + !args.includes('--hostname') && + options.host === 'github.acme-corp.com' && options.cwd === undefined && options.wslDistro === undefined ) diff --git a/src/main/github/client-work-items.test.ts b/src/main/github/client-work-items.test.ts index fa4f90dcd29..44afaf089f5 100644 --- a/src/main/github/client-work-items.test.ts +++ b/src/main/github/client-work-items.test.ts @@ -64,7 +64,10 @@ vi.mock('../git/runner', () => ({ vi.mock('./rate-limit', () => ({ rateLimitGuard: rateLimitGuardMock, noteRateLimitSpend: noteRateLimitSpendMock, - getRateLimit: vi.fn(async () => ({ ok: false, error: 'not probed in tests' })) + getRateLimit: vi.fn(async () => ({ ok: false, error: 'not probed in tests' })), + repositoryRateLimitGuard: vi.fn(() => ({ blocked: false })), + noteRepositoryRateLimitSpend: vi.fn(), + spendsSharedGitHubComQuota: () => true })) import { diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index d78137fcb09..a35de634e44 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -101,7 +101,14 @@ vi.mock('./local-git-config-signature', () => ({ vi.mock('./rate-limit', () => ({ getRateLimit: getRateLimitMock, rateLimitGuard: rateLimitGuardMock, - noteRateLimitSpend: noteRateLimitSpendMock + noteRateLimitSpend: noteRateLimitSpendMock, + // Why: the repository-scoped guards share the same bucket-keyed budget as the + // legacy ones, so delegate to the existing mocks to keep per-bucket blocking + // and spend assertions working unchanged. + repositoryRateLimitGuard: (_repository: unknown, bucket: string) => rateLimitGuardMock(bucket), + noteRepositoryRateLimitSpend: (_repository: unknown, bucket: string) => + noteRateLimitSpendMock(bucket), + spendsSharedGitHubComQuota: () => true })) import { @@ -4200,9 +4207,9 @@ describe('GitHub GraphQL rate-limit guard', () => { baseRefName: 'main', headRefOid: 'head-oid' } - ghExecFileAsyncMock.mockImplementation(async (args) => { + ghExecFileAsyncMock.mockImplementation(async (args, options) => { if (args.includes('graphql')) { - const enterprise = args.includes('github.acme-corp.com') + const enterprise = options?.host === 'github.acme-corp.com' return { stdout: JSON.stringify({ data: { @@ -4234,9 +4241,7 @@ describe('GitHub GraphQL rate-limit guard', () => { expect(enterprise?.autoMergeAllowed).toBe(false) const graphqlCalls = ghExecFileAsyncMock.mock.calls.filter(([args]) => args.includes('graphql')) expect(graphqlCalls).toHaveLength(2) - expect(graphqlCalls[1]?.[0]).toEqual( - expect.arrayContaining(['--hostname', 'github.acme-corp.com']) - ) + expect(graphqlCalls[1]?.[1]).toEqual(expect.objectContaining({ host: 'github.acme-corp.com' })) }) it('returns conflicting file details instead of running gh merge when PR is dirty', async () => { diff --git a/src/main/github/client.ts b/src/main/github/client.ts index cb96f8bf408..36351133abf 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -88,13 +88,12 @@ import type { GitHubRepoContext } from './github-repository-identity' import { getEnterpriseGitHubRepoSlug } from './github-enterprise-repository' import { getOriginGitHubApiRepository, - githubApiHostArgs, - githubCliRepositoryArgument, + githubHostExecOptions, githubRepositoryWebHost, - isGitHubDotComRepository, resolveGitHubApiRepository, type GitHubApiRepository } from './github-api-repository' +import { githubRepoIdentityKey } from '../../shared/github-repository-identity-key' export { _resetOwnerRepoCache } from './gh-utils' export { getIssue, @@ -119,11 +118,14 @@ import { mapGraphQLReactionGroups, type GitHubGraphQLReactionGroup } from './com import { getRateLimit, noteRateLimitSpend, + noteRepositoryRateLimitSpend, rateLimitGuard, + repositoryRateLimitGuard, + spendsSharedGitHubComQuota, type RateLimitBucketKind } from './rate-limit' -type GhExecOptions = ReturnType +type GhExecOptions = ReturnType & { host?: string } type HostedReviewLocalGitOptions = ReturnType const ORCA_REPO = 'stablyai/orca' @@ -189,9 +191,7 @@ async function assertRateLimitBudget( repository?: GitHubApiRepository | null, executionOptions?: Pick ): Promise { - // The global snapshot is for the default github.com account. Applying it to - // GHES or WSL can block a healthy runtime based on an unrelated quota. - if ((repository && !isGitHubDotComRepository(repository)) || executionOptions?.wslDistro) { + if (!spendsSharedGitHubComQuota(repository, executionOptions)) { return } await getRateLimit() @@ -203,27 +203,6 @@ async function assertRateLimitBudget( } } -function noteRepositoryRateLimitSpend( - repository: GitHubApiRepository | null, - bucket: RateLimitBucketKind, - cost = 1, - executionOptions?: Pick -): void { - if ((!repository || isGitHubDotComRepository(repository)) && !executionOptions?.wslDistro) { - noteRateLimitSpend(bucket, cost) - } -} - -function repositoryRateLimitGuard( - repository: GitHubApiRepository | null, - bucket: RateLimitBucketKind, - executionOptions?: Pick -): ReturnType { - return (!repository || isGitHubDotComRepository(repository)) && !executionOptions?.wslDistro - ? rateLimitGuard(bucket) - : { blocked: false } -} - function prRefreshUpstreamError( err: unknown ): Extract { @@ -839,14 +818,13 @@ async function fetchIssueWorkItem( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } if (ownerRepo) { const { stdout } = await ghExecFileAsync( - [ - 'api', - ...githubApiHostArgs(ownerRepo), - `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues/${number}` - ], + ['api', `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues/${number}`], ghOptions ) const item = JSON.parse(stdout) as Record @@ -878,7 +856,7 @@ async function fetchPullRequestReviewFields( 'view', String(number), '--repo', - githubCliRepositoryArgument(ownerRepo), + `${ownerRepo.owner}/${ownerRepo.repo}`, '--json', WORK_ITEM_PR_REVIEW_JSON_FIELDS ] @@ -905,7 +883,10 @@ async function fetchPullRequestWorkItem( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } if (ownerRepo) { try { const { stdout } = await ghExecFileAsync( @@ -914,7 +895,7 @@ async function fetchPullRequestWorkItem( 'view', String(number), '--repo', - githubCliRepositoryArgument(ownerRepo), + `${ownerRepo.owner}/${ownerRepo.repo}`, '--json', WORK_ITEM_PR_DETAIL_JSON_FIELDS ], @@ -941,11 +922,7 @@ async function fetchPullRequestWorkItem( } } catch { const { stdout } = await ghExecFileAsync( - [ - 'api', - ...githubApiHostArgs(ownerRepo), - `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${number}` - ], + ['api', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${number}`], ghOptions ) const mapped = mapPullRequestWorkItem( @@ -1738,7 +1715,7 @@ function parseCreatePRPayload(stdout: string): { number: number; url: string } | async function findOpenPRByHeadBase(args: { repoPath: string - repoArg: string + repo: GitHubApiRepository head: string base: string connectionId?: string | null @@ -1750,7 +1727,7 @@ async function findOpenPRByHeadBase(args: { 'pr', 'list', '--repo', - args.repoArg, + `${args.repo.owner}/${args.repo.repo}`, '--head', args.head, '--base', @@ -1764,7 +1741,8 @@ async function findOpenPRByHeadBase(args: { ], { ...ghRepoExecOptions(context), - ...(args.connectionId ? {} : getHostedReviewLocalGitOptions(args.options)) + ...(args.connectionId ? {} : getHostedReviewLocalGitOptions(args.options)), + ...githubHostExecOptions(args.repo) } ) const list = JSON.parse(stdout) as { number?: number; url?: string }[] @@ -1839,8 +1817,8 @@ export async function createGitHubPullRequest( error: 'Creating pull requests requires a GitHub remote.' } } - // Host-qualified for GHES so gh targets the Enterprise server, not github.com. - const repoArg = githubCliRepositoryArgument(ownerRepo) + // The runner host-qualifies --repo from options.host for GHES (#8312). + const repoArg = `${ownerRepo.owner}/${ownerRepo.repo}` const base = normalizeHostedReviewBaseRef(input.base) const head = input.head ? normalizeHostedReviewHeadRef(input.head) || undefined : undefined @@ -1892,6 +1870,7 @@ export async function createGitHubPullRequest( const { stdout } = await ghExecFileAsync(createArgs, { ...ghRepoExecOptions(context), ...(connectionId ? {} : getHostedReviewLocalGitOptions(options)), + ...githubHostExecOptions(ownerRepo), timeout: 60_000, idempotent: false }) @@ -1902,7 +1881,7 @@ export async function createGitHubPullRequest( const found = head ? await findOpenPRByHeadBase({ repoPath, - repoArg, + repo: ownerRepo, head, base, connectionId, @@ -1926,7 +1905,7 @@ export async function createGitHubPullRequest( ) { const existing = await findOpenPRByHeadBase({ repoPath, - repoArg, + repo: ownerRepo, head, base, connectionId, @@ -2194,8 +2173,7 @@ async function detectRepositoryMergeMetadata( branchName: string | undefined, ghOptions: GhExecOptions ): Promise { - const host = ownerRepo.host?.toLowerCase() ?? 'github.com' - const cacheKey = `${host}/${ownerRepo.owner.toLowerCase()}/${ownerRepo.repo.toLowerCase()}:${branchName ?? '__repo__'}` + const cacheKey = `${githubRepoIdentityKey(ownerRepo)}:${branchName ?? '__repo__'}` pruneRepositoryMergeMetadataCache() const cached = repositoryMergeMetadataCache.get(cacheKey) if (cached) { @@ -2229,7 +2207,6 @@ async function detectRepositoryMergeMetadata( noteRepositoryRateLimitSpend(ownerRepo, 'graphql', 1, ghOptions) const args = [ 'api', - ...githubApiHostArgs(ownerRepo), 'graphql', '-f', `query=${query}`, @@ -2241,7 +2218,10 @@ async function detectRepositoryMergeMetadata( if (branchName) { args.push('-f', `branch=${branchName}`) } - const { stdout } = await ghExecFileAsync(args, ghOptions) + const { stdout } = await ghExecFileAsync(args, { + ...ghOptions, + ...githubHostExecOptions(ownerRepo) + }) const parsed = JSON.parse(stdout) as { data?: { repository?: { @@ -2326,12 +2306,8 @@ async function getRestPRForBranch( ): Promise { const head = encodeURIComponent(`${headOwner}:${branchName}`) const { stdout } = await ghExecFileAsync( - [ - 'api', - ...githubApiHostArgs(prRepo), - `repos/${prRepo.owner}/${prRepo.repo}/pulls?head=${head}&state=all&per_page=1` - ], - ghOptions + ['api', `repos/${prRepo.owner}/${prRepo.repo}/pulls?head=${head}&state=all&per_page=1`], + { ...ghOptions, ...githubHostExecOptions(prRepo) } ) const list = JSON.parse(stdout) as RestPullRequest[] const pr = list[0] @@ -2348,7 +2324,7 @@ async function getFallbackPRListForBranch( 'pr', 'list', '--repo', - githubCliRepositoryArgument(prRepo), + `${prRepo.owner}/${prRepo.repo}`, '--head', branchName, '--state', @@ -2358,7 +2334,7 @@ async function getFallbackPRListForBranch( '--json', PR_BRANCH_LIST_JSON_FIELDS ], - ghOptions + { ...ghOptions, ...githubHostExecOptions(prRepo) } ) const list = JSON.parse(stdout) as PullRequestLookupData[] return list[0] ?? null @@ -2752,12 +2728,8 @@ async function getRestPRByNumber( ghOptions: ReturnType ): Promise { const { stdout } = await ghExecFileAsync( - [ - 'api', - ...githubApiHostArgs(ownerRepo), - `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${number}` - ], - ghOptions + ['api', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${number}`], + { ...ghOptions, ...githubHostExecOptions(ownerRepo) } ) return mapRestPullRequest(JSON.parse(stdout) as RestPullRequest) } @@ -2774,11 +2746,11 @@ async function getPRByNumber( 'view', String(number), '--repo', - githubCliRepositoryArgument(ownerRepo), + `${ownerRepo.owner}/${ownerRepo.repo}`, '--json', PR_LOOKUP_JSON_FIELDS ], - ghOptions + { ...ghOptions, ...githubHostExecOptions(ownerRepo) } ) return hydratePullRequestLookupData( ownerRepo, @@ -3458,7 +3430,6 @@ async function getPRChecksViaRestFallback( const { stdout } = await ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), ...cacheArgs, `repos/${ownerRepo.owner}/${ownerRepo.repo}/commits/${encodedHeadSha}/check-runs?per_page=100` ], @@ -3476,7 +3447,6 @@ async function getPRChecksViaRestFallback( const statusResult = await ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), ...cacheArgs, `repos/${ownerRepo.owner}/${ownerRepo.repo}/commits/${encodedHeadSha}/status?per_page=100` ], @@ -3499,7 +3469,6 @@ async function getPRChecksViaRestFallback( const suitesResult = await ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), ...cacheArgs, `repos/${ownerRepo.owner}/${ownerRepo.repo}/commits/${encodedHeadSha}/check-suites?per_page=100` ], @@ -3546,13 +3515,16 @@ export async function getPRChecks( localGitOptions: LocalGitExecOptions = {} ): Promise { void headSha - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) const ownerRepo = await resolveGitHubApiRepository( repoPath, prRepo, connectionId, localGitOptions ) + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } if (connectionId && !ownerRepo) { throw new Error(GITHUB_WORK_ITEMS_SSH_REMOTE_REQUIRED_MESSAGE) } @@ -3562,7 +3534,7 @@ export async function getPRChecks( try { const fallbackArgs = ['pr', 'checks', String(prNumber), '--json', 'name,state,link'] if (ownerRepo) { - fallbackArgs.push('--repo', githubCliRepositoryArgument(ownerRepo)) + fallbackArgs.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) } const { stdout } = await ghExecFileAsync(fallbackArgs, ghOptions).catch((err: unknown) => { const { stderr } = extractExecError(err) @@ -3602,7 +3574,6 @@ export async function getPRChecks( const { stdout } = await ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), 'graphql', ...cacheArgs, '-f', @@ -3776,9 +3747,7 @@ async function attachFailedJobLogTails( // Why: cap log fetches so failed-job details stay a bounded follow-up, not a burst of hosted log downloads. for (const job of failedJobs) { const jobCacheKey = getCheckJobLogTailCacheKey(job) - const cacheKey = jobCacheKey - ? `${githubRepositoryWebHost(ownerRepo).toLowerCase()}/${ownerRepo.owner.toLowerCase()}/${ownerRepo.repo.toLowerCase()}:${jobCacheKey}` - : null + const cacheKey = jobCacheKey ? `${githubRepoIdentityKey(ownerRepo)}:${jobCacheKey}` : null if (!cacheKey) { continue } @@ -3788,11 +3757,7 @@ async function attachFailedJobLogTails( } try { const { stdout } = await ghExecFileAsync( - [ - 'api', - ...githubApiHostArgs(ownerRepo), - `repos/${ownerRepo.owner}/${ownerRepo.repo}/actions/jobs/${job.id}/logs` - ], + ['api', `repos/${ownerRepo.owner}/${ownerRepo.repo}/actions/jobs/${job.id}/logs`], ghOptions ) job.logTail = sliceCheckLogTail(stdout) @@ -3831,7 +3796,6 @@ export async function getPRCheckDetails( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) const ownerRepo = await resolveGitHubApiRepository( repoPath, args.prRepo, @@ -3841,6 +3805,10 @@ export async function getPRCheckDetails( if (!ownerRepo) { return null } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } await acquire() try { @@ -3848,11 +3816,7 @@ export async function getPRCheckDetails( let annotations: PRCheckRunDetails['annotations'] = [] if (args.checkRunId) { const { stdout } = await ghExecFileAsync( - [ - 'api', - ...githubApiHostArgs(ownerRepo), - `repos/${ownerRepo.owner}/${ownerRepo.repo}/check-runs/${args.checkRunId}` - ], + ['api', `repos/${ownerRepo.owner}/${ownerRepo.repo}/check-runs/${args.checkRunId}`], ghOptions ) checkRun = JSON.parse(stdout) as Record @@ -3860,7 +3824,6 @@ export async function getPRCheckDetails( const annotationsResult = await ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), `repos/${ownerRepo.owner}/${ownerRepo.repo}/check-runs/${args.checkRunId}/annotations?per_page=20` ], ghOptions @@ -3878,7 +3841,6 @@ export async function getPRCheckDetails( const { stdout } = await ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), `repos/${ownerRepo.owner}/${ownerRepo.repo}/actions/runs/${workflowRunId}/jobs?per_page=100` ], ghOptions @@ -3935,11 +3897,14 @@ export async function rerunPRChecks( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) const ownerRepo = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } const checks = await getPRChecks( repoPath, @@ -3983,7 +3948,7 @@ export async function rerunPRChecks( const endpoint = options.failedOnly ? `repos/${ownerRepo.owner}/${ownerRepo.repo}/actions/runs/${runId}/rerun-failed-jobs` : `repos/${ownerRepo.owner}/${ownerRepo.repo}/actions/runs/${runId}/rerun` - await ghExecFileAsync(['api', ...githubApiHostArgs(ownerRepo), '-X', 'POST', endpoint], { + await ghExecFileAsync(['api', '-X', 'POST', endpoint], { ...ghOptions, env: { ...process.env, GH_PROMPT_DISABLED: '1' } }) @@ -3993,7 +3958,6 @@ export async function rerunPRChecks( await ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), '-X', 'POST', `repos/${ownerRepo.owner}/${ownerRepo.repo}/check-runs/${checkRunId}/rerequest` @@ -4074,13 +4038,16 @@ export async function getPRComments( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) const ownerRepo = await resolveGitHubApiRepository( repoPath, options?.prRepo, connectionId, localGitOptions ) + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } if (connectionId && !ownerRepo) { throw new Error(GITHUB_WORK_ITEMS_SSH_REMOTE_REQUIRED_MESSAGE) } @@ -4104,7 +4071,6 @@ export async function getPRComments( reviewThreadsFetch = ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), 'graphql', '-f', `query=${REVIEW_THREADS_QUERY}`, @@ -4120,23 +4086,13 @@ export async function getPRComments( } const [issueResult, threadsResult, reviewsResult] = await Promise.allSettled([ ghExecFileAsync( - [ - 'api', - ...githubApiHostArgs(ownerRepo), - ...cacheArgs, - `${base}/issues/${prNumber}/comments?per_page=100` - ], + ['api', ...cacheArgs, `${base}/issues/${prNumber}/comments?per_page=100`], ghOptions ), reviewThreadsFetch, // Why: review summaries (approve/request-changes/general) live under pulls/{n}/reviews, not issue comments or threads. ghExecFileAsync( - [ - 'api', - ...githubApiHostArgs(ownerRepo), - ...cacheArgs, - `${base}/pulls/${prNumber}/reviews?per_page=100` - ], + ['api', ...cacheArgs, `${base}/pulls/${prNumber}/reviews?per_page=100`], ghOptions ) ]) @@ -4326,9 +4282,6 @@ export async function setPRFileViewed(args: { path: string viewed: boolean }): Promise { - const ghOptions = ghRepoExecOptions( - githubRepoContext(args.repoPath, args.connectionId, args.localGitOptions) - ) const ownerRepo = await getOriginGitHubApiRepository( args.repoPath, args.connectionId, @@ -4337,6 +4290,10 @@ export async function setPRFileViewed(args: { if (args.connectionId && !ownerRepo) { return false } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(args.repoPath, args.connectionId, args.localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } const mutation = args.viewed ? 'markFileAsViewed' : 'unmarkFileAsViewed' const query = `mutation($pullRequestId: ID!, $path: String!) { ${mutation}(input: { pullRequestId: $pullRequestId, path: $path }) { @@ -4348,7 +4305,6 @@ export async function setPRFileViewed(args: { await ghExecFileAsync( [ 'api', - ...(ownerRepo ? githubApiHostArgs(ownerRepo) : []), 'graphql', '-f', `query=${query}`, @@ -4380,11 +4336,14 @@ export async function resolveReviewThread( ): Promise { const mutation = resolve ? 'resolveReviewThread' : 'unresolveReviewThread' const query = `mutation($threadId: ID!) { ${mutation}(input: { threadId: $threadId }) { thread { isResolved } } }` - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) const ownerRepo = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) if (connectionId && !ownerRepo) { return false } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } const guard = repositoryRateLimitGuard(ownerRepo, 'graphql', ghOptions) if (guard.blocked) { console.warn( @@ -4396,15 +4355,7 @@ export async function resolveReviewThread( try { noteRepositoryRateLimitSpend(ownerRepo, 'graphql', 1, ghOptions) await ghExecFileAsync( - [ - 'api', - ...(ownerRepo ? githubApiHostArgs(ownerRepo) : []), - 'graphql', - '-f', - `query=${query}`, - '-f', - `threadId=${threadId}` - ], + ['api', 'graphql', '-f', `query=${query}`, '-f', `threadId=${threadId}`], ghOptions ) return true @@ -4459,7 +4410,6 @@ export async function addPRReviewCommentReply( prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) const ownerRepo = await resolveGitHubApiRepository( repoPath, prRepo, @@ -4469,12 +4419,15 @@ export async function addPRReviewCommentReply( if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } await acquire() try { const { stdout } = await ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), '-X', 'POST', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${prNumber}/comments/${commentId}/replies`, @@ -4505,9 +4458,6 @@ export async function addPRReviewComment( localGitOptions?: LocalGitExecOptions } ): Promise { - const ghOptions = ghRepoExecOptions( - githubRepoContext(args.repoPath, args.connectionId, args.localGitOptions) - ) const ownerRepo = await getOriginGitHubApiRepository( args.repoPath, args.connectionId, @@ -4516,11 +4466,14 @@ export async function addPRReviewComment( if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(args.repoPath, args.connectionId, args.localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } await acquire() try { const fields = [ 'api', - ...githubApiHostArgs(ownerRepo), '-X', 'POST', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${args.prNumber}/comments`, @@ -4574,7 +4527,6 @@ export async function mergePR( prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ ok: true } | { ok: false; error: string }> { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) const ownerRepo = await resolveGitHubApiRepository( repoPath, prRepo, @@ -4584,6 +4536,10 @@ export async function mergePR( if (connectionId && !ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } await acquire() try { const mergeBlocker = await getPRMergeBlocker( @@ -4601,7 +4557,7 @@ export async function mergePR( // Don't use --delete-branch: it deletes the local branch, which fails while the worktree is checked out on it. const args = ['pr', 'merge', String(prNumber), `--${method}`] if (ownerRepo) { - args.push('--repo', githubCliRepositoryArgument(ownerRepo)) + args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) } await ghExecFileAsync(args, { ...ghOptions, @@ -4626,7 +4582,6 @@ export async function setPRAutoMerge( prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ ok: true } | { ok: false; error: string }> { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) const ownerRepo = await resolveGitHubApiRepository( repoPath, prRepo, @@ -4636,6 +4591,10 @@ export async function setPRAutoMerge( if (connectionId && !ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } await acquire() try { if (enabled) { @@ -4643,7 +4602,7 @@ export async function setPRAutoMerge( } const args = ['pr', 'merge', String(prNumber), '--disable-auto'] if (ownerRepo) { - args.push('--repo', githubCliRepositoryArgument(ownerRepo)) + args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) } await ghExecFileAsync(args, { ...ghOptions, @@ -4680,7 +4639,7 @@ async function getPRAutoMergeIdentity( ): Promise { const args = ['pr', 'view', String(prNumber), '--json', PR_AUTO_MERGE_IDENTITY_JSON_FIELDS] if (ownerRepo) { - args.push('--repo', githubCliRepositoryArgument(ownerRepo)) + args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) } const { stdout } = await ghExecFileAsync(args, ghOptions) const data = JSON.parse(stdout) as PRAutoMergeIdentity @@ -4699,7 +4658,7 @@ async function runPRAutoMergeCommand( ): Promise { const args = ['pr', 'merge', String(prNumber), '--auto', `--${method}`] if (ownerRepo) { - args.push('--repo', githubCliRepositoryArgument(ownerRepo)) + args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) } await ghExecFileAsync(args, { ...ghOptions, @@ -4744,7 +4703,6 @@ async function enablePRAutoMerge( }` const args = [ 'api', - ...(ownerRepo ? githubApiHostArgs(ownerRepo) : []), 'graphql', '-f', `query=${query}`, @@ -4837,18 +4795,18 @@ export async function updatePRState( localGitOptions: LocalGitExecOptions = {} ): Promise<{ ok: true } | { ok: false; error: string }> { const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ghOptions = ghRepoExecOptions(context) const ownerRepo = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } + const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } await acquire() try { const cmd = updates.state === 'closed' ? 'close' : 'reopen' // Why: gh's PR commands use GitHub's supported reopen flow; REST state PATCH can 422 on reopen. await ghExecFileAsync( - ['pr', cmd, String(prNumber), '--repo', githubCliRepositoryArgument(ownerRepo)], + ['pr', cmd, String(prNumber), '--repo', `${ownerRepo.owner}/${ownerRepo.repo}`], { ...ghOptions } @@ -4874,16 +4832,19 @@ export async function requestPRReviewers( if (logins.length === 0) { return { ok: false, error: 'Enter at least one reviewer' } } - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) const ownerRepo = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) if (connectionId && !ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } await acquire() try { const args = ['pr', 'edit', String(prNumber), '--add-reviewer', logins.join(',')] if (ownerRepo) { - args.push('--repo', githubCliRepositoryArgument(ownerRepo)) + args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) } await ghExecFileAsync(args, { ...ghOptions, @@ -4910,16 +4871,19 @@ export async function removePRReviewers( if (logins.length === 0) { return { ok: false, error: 'Enter at least one reviewer' } } - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) const ownerRepo = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) if (connectionId && !ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } await acquire() try { const args = ['pr', 'edit', String(prNumber), '--remove-reviewer', logins.join(',')] if (ownerRepo) { - args.push('--repo', githubCliRepositoryArgument(ownerRepo)) + args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) } await ghExecFileAsync(args, { ...ghOptions, @@ -4946,7 +4910,6 @@ export async function updatePRTitle( prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) const ownerRepo = await resolveGitHubApiRepository( repoPath, prRepo, @@ -4956,11 +4919,15 @@ export async function updatePRTitle( if (connectionId && !ownerRepo) { return false } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } await acquire() try { const args = ['pr', 'edit', String(prNumber), '--title', title] if (ownerRepo) { - args.push('--repo', githubCliRepositoryArgument(ownerRepo)) + args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) } await ghExecFileAsync(args, { ...ghOptions @@ -4982,7 +4949,6 @@ export async function updatePRDetails( prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ ok: true } | { ok: false; error: string }> { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) const ownerRepo = await resolveGitHubApiRepository( repoPath, prRepo, @@ -4992,6 +4958,10 @@ export async function updatePRDetails( if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } const fields: string[] = [] if (updates.title !== undefined) { @@ -5013,7 +4983,6 @@ export async function updatePRDetails( await ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), '-X', 'PATCH', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${prNumber}`, diff --git a/src/main/github/github-api-repository.ts b/src/main/github/github-api-repository.ts index 2998805fd35..0484f1d03a5 100644 --- a/src/main/github/github-api-repository.ts +++ b/src/main/github/github-api-repository.ts @@ -45,21 +45,18 @@ export async function resolveGitHubApiRepository( return connectionId ? null : repository } -export function githubApiHostArgs(repository: GitHubApiRepository): string[] { - const host = repository.host - return host && host.toLowerCase() !== 'github.com' ? ['--hostname', host] : [] -} - export function isGitHubDotComRepository(repository: GitHubApiRepository): boolean { return !repository.host || repository.host.toLowerCase() === 'github.com' } -// Why: owner/repo shorthand targets gh's default host, and SSH-backed repos -// have no local cwd from which gh can infer an Enterprise hostname. -export function githubCliRepositoryArgument(repository: GitHubApiRepository): string { - return !isGitHubDotComRepository(repository) - ? `${repository.host}/${repository.owner}/${repository.repo}` - : `${repository.owner}/${repository.repo}` +// 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. +export function githubHostExecOptions(repository: GitHubApiRepository | null | undefined): { + host?: string +} { + return repository?.host && !isGitHubDotComRepository(repository) ? { host: repository.host } : {} } export function githubRepositoryWebHost(repository: GitHubApiRepository): string { diff --git a/src/main/github/github-enterprise-repository.test.ts b/src/main/github/github-enterprise-repository.test.ts index 84e29abc678..a0ab303d961 100644 --- a/src/main/github/github-enterprise-repository.test.ts +++ b/src/main/github/github-enterprise-repository.test.ts @@ -61,10 +61,11 @@ describe('getEnterpriseGitHubRepoSlug', () => { repo: 'orca', host: 'github.acme-corp.com' }) - // The auth probe targets the remote's host, not a hardcoded github.com. + // The auth probe targets the remote's host, not a hardcoded github.com, and + // scopes any breaker trip to that host via the exec options. expect(ghExecFileAsyncMock).toHaveBeenCalledWith( ['auth', 'status', '--hostname', 'github.acme-corp.com'], - { cwd: '/repo' } + { cwd: '/repo', host: 'github.acme-corp.com' } ) }) @@ -89,7 +90,7 @@ describe('getEnterpriseGitHubRepoSlug', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledWith( ['auth', 'status', '--hostname', 'github.acme-corp.com'], - { cwd: '/repo', wslDistro: 'Ubuntu' } + { cwd: '/repo', wslDistro: 'Ubuntu', host: 'github.acme-corp.com' } ) }) @@ -137,7 +138,7 @@ describe('isGitHubHostAuthenticated', () => { ).resolves.toBe(true) expect(ghExecFileAsyncMock).toHaveBeenCalledWith( ['auth', 'status', '--hostname', 'github.acme-corp.com'], - {} + { host: 'github.acme-corp.com' } ) }) diff --git a/src/main/github/github-enterprise-repository.ts b/src/main/github/github-enterprise-repository.ts index c08075654ca..e16ab673415 100644 --- a/src/main/github/github-enterprise-repository.ts +++ b/src/main/github/github-enterprise-repository.ts @@ -83,9 +83,12 @@ export async function isGitHubHostAuthenticated( // Why: provider detection and review loading can probe the same runtime at // once; coalesce them so one host never spawns duplicate auth subprocesses. const probe = (async () => { - const execOptions = ghRepoExecOptions( - githubRepoContext(repoPath, connectionId, localGitOptions) - ) + const execOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + // Why: scope any rate-limit breaker trip from this probe to the probed + // host, not to gh's default account. + host: normalizedHost + } let authenticated: boolean try { await ghExecFileAsync(['auth', 'status', '--hostname', normalizedHost], execOptions) diff --git a/src/main/github/issues.test.ts b/src/main/github/issues.test.ts index 926b5835f27..e804a6cbc5c 100644 --- a/src/main/github/issues.test.ts +++ b/src/main/github/issues.test.ts @@ -160,15 +160,13 @@ describe('issue source operations', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledWith( [ 'api', - '--hostname', - 'github.acme-corp.com', '-X', 'POST', 'repos/team/orca/issues/7/comments', '--raw-field', 'body=Enterprise comment' ], - {} + expect.objectContaining({ host: 'github.acme-corp.com' }) ) }) diff --git a/src/main/github/issues.ts b/src/main/github/issues.ts index 424cf7cee6b..dcfb91d8013 100644 --- a/src/main/github/issues.ts +++ b/src/main/github/issues.ts @@ -15,7 +15,7 @@ import type { } from '../../shared/types' import { mapIssueInfo } from './mappers' import type { LocalGitExecOptions, OwnerRepo } from './gh-utils' -import { githubApiHostArgs, resolveGitHubApiRepository } from './github-api-repository' +import { githubHostExecOptions, resolveGitHubApiRepository } from './github-api-repository' // prettier-ignore import { ghExecFileAsync, acquire, release, getIssueOwnerRepo, resolveIssueSource, classifyGhError, classifyListIssuesError, ghRepoExecOptions, githubRepoContext, extractExecError } from './gh-utils' @@ -411,19 +411,18 @@ export async function addIssueComment( localGitOptions: LocalGitExecOptions = {} ): Promise { const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ghOptions = ghRepoExecOptions(context) const ownerRepo = ownerRepoOverride ? await resolveGitHubApiRepository(repoPath, ownerRepoOverride, connectionId, localGitOptions) : await getIssueOwnerRepo(repoPath, connectionId, localGitOptions) if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } + const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } await acquire() try { const { stdout } = await ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), '-X', 'POST', `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues/${issueNumber}/comments`, diff --git a/src/main/github/merged-pr-commit-membership.ts b/src/main/github/merged-pr-commit-membership.ts index 0bb390f6495..7350416ca6f 100644 --- a/src/main/github/merged-pr-commit-membership.ts +++ b/src/main/github/merged-pr-commit-membership.ts @@ -1,5 +1,7 @@ import { ghExecFileAsync } from './gh-utils' -import { noteRateLimitSpend, rateLimitGuard } from './rate-limit' +import { noteRepositoryRateLimitSpend, repositoryRateLimitGuard } from './rate-limit' +import { githubHostExecOptions } from './github-api-repository' +import { githubRepoIdentityKey } from '../../shared/github-repository-identity-key' import type { OwnerRepo } from './github-repository-identity' type GhExecOptions = Parameters[1] @@ -59,7 +61,8 @@ export async function isCommitPartOfMergedPR(args: { } const owner = args.ownerRepo.owner const repo = args.ownerRepo.repo - const cacheKey = `${owner.toLowerCase()}/${repo.toLowerCase()}#${args.prNumber}@${oid}` + const cacheKey = `${githubRepoIdentityKey(args.ownerRepo)}#${args.prNumber}@${oid}` + const ghOptions = { ...args.ghOptions, ...githubHostExecOptions(args.ownerRepo) } const now = Date.now() pruneMergedPRCommitMembershipCache(now) const cached = membershipCache.get(cacheKey) @@ -68,7 +71,7 @@ export async function isCommitPartOfMergedPR(args: { } // Why blocked stays unknown: hiding a transient branch match is safe, but // callers must not clear a durable linked PR when the probe never ran. - if (rateLimitGuard('core').blocked) { + if (repositoryRateLimitGuard(args.ownerRepo, 'core', ghOptions).blocked) { return 'unknown' } try { @@ -79,20 +82,20 @@ export async function isCommitPartOfMergedPR(args: { // found (contained) or a short page proves absence (not-contained); only the // pathological all-full case up to the cap stays 'unknown'. for (let page = 1; page <= COMMIT_PULLS_MAX_PAGES; page += 1) { - if (page > 1 && rateLimitGuard('core').blocked) { + if (page > 1 && repositoryRateLimitGuard(args.ownerRepo, 'core', ghOptions).blocked) { membershipCache.set(cacheKey, { value: 'unknown', expiresAt: now + MEMBERSHIP_ERROR_TTL_MS }) return 'unknown' } - noteRateLimitSpend('core') + noteRepositoryRateLimitSpend(args.ownerRepo, 'core', 1, ghOptions) const { stdout } = await ghExecFileAsync( [ 'api', `repos/${owner}/${repo}/commits/${oid}/pulls?per_page=${COMMIT_PULLS_PAGE_SIZE}&page=${page}` ], - args.ghOptions + ghOptions ) const parsed = JSON.parse(stdout) as unknown // Why: a non-array success payload is a shape mismatch, not an empty page; diff --git a/src/main/github/rate-limit.test.ts b/src/main/github/rate-limit.test.ts index ec952841f50..cddcadab361 100644 --- a/src/main/github/rate-limit.test.ts +++ b/src/main/github/rate-limit.test.ts @@ -13,7 +13,15 @@ vi.mock('./gh-utils', () => ({ release: vi.fn() })) -import { getRateLimit, rateLimitGuard, _resetRateLimitCache } from './rate-limit' +import { + getRateLimit, + rateLimitGuard, + noteRateLimitSpend, + spendsSharedGitHubComQuota, + repositoryRateLimitGuard, + noteRepositoryRateLimitSpend, + _resetRateLimitCache +} from './rate-limit' const PAYLOAD = JSON.stringify({ resources: { @@ -49,10 +57,12 @@ describe('getRateLimit', () => { fetchedAt: 1_000 } }) - expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - ['api', '--hostname', 'github.com', 'rate_limit'], - { encoding: 'utf-8' } - ) + // The runner injects --hostname at spawn from the `host` option, so the + // probe passes bare argv and pins the host to github.com through options. + expect(ghExecFileAsyncMock).toHaveBeenCalledWith(['api', 'rate_limit'], { + encoding: 'utf-8', + host: 'github.com' + }) }) it('caches a failed probe for the TTL instead of re-spawning gh', async () => { @@ -139,3 +149,81 @@ describe('rateLimitGuard', () => { expect(rateLimitGuard('core')).toEqual({ blocked: false }) }) }) + +// Why: only default github.com traffic on the native runtime shares this +// snapshot's budget. GHES hosts and WSL distros run against separate quotas, +// so the shared guard must bypass them entirely. +describe('shared-quota host scoping', () => { + beforeEach(() => { + vi.useFakeTimers() + vi.setSystemTime(1_000) + ghExecFileAsyncMock.mockReset() + _resetRateLimitCache() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + async function cacheExhaustedSnapshot(resetEpochSeconds: number): Promise { + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ + resources: { + core: { limit: 5000, remaining: 3, reset: resetEpochSeconds }, + search: { limit: 30, remaining: 28, reset: resetEpochSeconds }, + graphql: { limit: 5000, remaining: 4900, reset: resetEpochSeconds } + } + }) + }) + expect((await getRateLimit()).ok).toBe(true) + } + + it('treats github.com / no host on the native runtime as sharing the budget', () => { + expect(spendsSharedGitHubComQuota(undefined)).toBe(true) + expect(spendsSharedGitHubComQuota(null)).toBe(true) + expect(spendsSharedGitHubComQuota({ host: 'github.com' })).toBe(true) + expect(spendsSharedGitHubComQuota({ host: 'GitHub.com' })).toBe(true) + }) + + it('excludes GHES hosts and WSL runtimes from the shared budget', () => { + expect(spendsSharedGitHubComQuota({ host: 'github.acme-corp.com' })).toBe(false) + expect(spendsSharedGitHubComQuota({ host: 'github.com' }, { wslDistro: 'Ubuntu' })).toBe(false) + expect(spendsSharedGitHubComQuota(undefined, { wslDistro: 'Ubuntu' })).toBe(false) + }) + + it('applies the shared guard for github.com but bypasses non-shared scopes', async () => { + await cacheExhaustedSnapshot(61) + + expect(repositoryRateLimitGuard({ host: 'github.com' }, 'core')).toEqual({ + blocked: true, + remaining: 3, + limit: 5000, + resetAt: 61 + }) + expect(repositoryRateLimitGuard({ host: 'github.acme-corp.com' }, 'core')).toEqual({ + blocked: false + }) + expect( + repositoryRateLimitGuard({ host: 'github.com' }, 'core', { wslDistro: 'Ubuntu' }) + ).toEqual({ blocked: false }) + }) + + it('debits shared-budget spend but no-ops for non-shared scopes', async () => { + await cacheExhaustedSnapshot(61) + + noteRepositoryRateLimitSpend({ host: 'github.acme-corp.com' }, 'core') + const afterEnterprise = rateLimitGuard('core') + expect(afterEnterprise.blocked && afterEnterprise.remaining).toBe(3) + + noteRepositoryRateLimitSpend({ host: 'github.com' }, 'core') + const afterShared = rateLimitGuard('core') + expect(afterShared.blocked && afterShared.remaining).toBe(2) + }) + + it('matches noteRateLimitSpend when the scope is shared github.com', async () => { + await cacheExhaustedSnapshot(61) + noteRateLimitSpend('core') + const guard = rateLimitGuard('core') + expect(guard.blocked && guard.remaining).toBe(2) + }) +}) diff --git a/src/main/github/rate-limit.ts b/src/main/github/rate-limit.ts index 8ba6cf9c21a..97451836e56 100644 --- a/src/main/github/rate-limit.ts +++ b/src/main/github/rate-limit.ts @@ -120,6 +120,36 @@ export function noteRateLimitSpend(bucket: RateLimitBucketKind, cost = 1): void } } +// Why: this snapshot covers native github.com only; GHES and WSL use quotas protected by the runner's scoped breaker. +export function spendsSharedGitHubComQuota( + repository: { host?: string } | null | undefined, + executionOptions?: { wslDistro?: string } +): boolean { + const host = repository?.host + return (!host || host.toLowerCase() === 'github.com') && !executionOptions?.wslDistro +} + +export function repositoryRateLimitGuard( + repository: { host?: string } | null | undefined, + bucket: RateLimitBucketKind, + executionOptions?: { wslDistro?: string } +): ReturnType { + return spendsSharedGitHubComQuota(repository, executionOptions) + ? rateLimitGuard(bucket) + : { blocked: false } +} + +export function noteRepositoryRateLimitSpend( + repository: { host?: string } | null | undefined, + bucket: RateLimitBucketKind, + cost = 1, + executionOptions?: { wslDistro?: string } +): void { + if (spendsSharedGitHubComQuota(repository, executionOptions)) { + noteRateLimitSpend(bucket, cost) + } +} + // Why: the breaker only knows "blocked", not the reset time; one exempt probe refines it to the real reset or clears a stale block (single-flight so a 403 burst probes once). let resetRefinementInFlight: Promise | null = null @@ -178,8 +208,9 @@ async function fetchRateLimitSnapshot(): Promise { try { // Why: this singleton snapshot guards native github.com traffic. Pin the // host so a process-level GH_HOST cannot make it describe a GHES account. - const { stdout } = await ghExecFileAsync(['api', '--hostname', 'github.com', 'rate_limit'], { - encoding: 'utf-8' + const { stdout } = await ghExecFileAsync(['api', 'rate_limit'], { + encoding: 'utf-8', + host: 'github.com' }) const parsed = JSON.parse(stdout) as GhRateLimitPayload const snapshot: GitHubRateLimitSnapshot = { diff --git a/src/main/github/work-item-details-file-viewed.test.ts b/src/main/github/work-item-details-file-viewed.test.ts index 1b7bd44a2b0..b8816bf4e0b 100644 --- a/src/main/github/work-item-details-file-viewed.test.ts +++ b/src/main/github/work-item-details-file-viewed.test.ts @@ -13,6 +13,8 @@ const { getPRCommentsMock, rateLimitGuardMock, noteRateLimitSpendMock, + repositoryRateLimitGuardMock, + noteRepositoryRateLimitSpendMock, acquireMock, releaseMock } = vi.hoisted(() => ({ @@ -24,6 +26,8 @@ const { getPRCommentsMock: vi.fn(), rateLimitGuardMock: vi.fn<() => RateLimitGuardResult>(() => ({ blocked: false })), noteRateLimitSpendMock: vi.fn(), + repositoryRateLimitGuardMock: vi.fn<() => RateLimitGuardResult>(() => ({ blocked: false })), + noteRepositoryRateLimitSpendMock: vi.fn(), acquireMock: vi.fn(), releaseMock: vi.fn() })) @@ -50,7 +54,9 @@ vi.mock('./client', () => ({ vi.mock('./rate-limit', () => ({ rateLimitGuard: rateLimitGuardMock, - noteRateLimitSpend: noteRateLimitSpendMock + noteRateLimitSpend: noteRateLimitSpendMock, + repositoryRateLimitGuard: repositoryRateLimitGuardMock, + noteRepositoryRateLimitSpend: noteRepositoryRateLimitSpendMock })) import { getWorkItemDetails } from './work-item-details' @@ -66,6 +72,9 @@ describe('getWorkItemDetails PR file viewed state', () => { rateLimitGuardMock.mockReset() rateLimitGuardMock.mockReturnValue({ blocked: false }) noteRateLimitSpendMock.mockReset() + repositoryRateLimitGuardMock.mockReset() + repositoryRateLimitGuardMock.mockReturnValue({ blocked: false }) + noteRepositoryRateLimitSpendMock.mockReset() acquireMock.mockReset() releaseMock.mockReset() acquireMock.mockResolvedValue(undefined) diff --git a/src/main/github/work-item-details-pr-files.test.ts b/src/main/github/work-item-details-pr-files.test.ts index 1a2754a9d1f..cbd043fe996 100644 --- a/src/main/github/work-item-details-pr-files.test.ts +++ b/src/main/github/work-item-details-pr-files.test.ts @@ -41,7 +41,9 @@ vi.mock('./github-enterprise-repository', () => ({ vi.mock('./rate-limit', () => ({ rateLimitGuard: vi.fn(() => ({ blocked: false })), - noteRateLimitSpend: vi.fn() + noteRateLimitSpend: vi.fn(), + repositoryRateLimitGuard: vi.fn(() => ({ blocked: false })), + noteRepositoryRateLimitSpend: vi.fn() })) import { getWorkItemDetails } from './work-item-details' diff --git a/src/main/github/work-item-details.test.ts b/src/main/github/work-item-details.test.ts index ff595a27337..cfb6641fda9 100644 --- a/src/main/github/work-item-details.test.ts +++ b/src/main/github/work-item-details.test.ts @@ -15,6 +15,8 @@ const { getPRCommentsMock, rateLimitGuardMock, noteRateLimitSpendMock, + repositoryRateLimitGuardMock, + noteRepositoryRateLimitSpendMock, ghRepoExecOptionsMock, githubRepoContextMock, acquireMock, @@ -30,6 +32,8 @@ const { getPRCommentsMock: vi.fn(), rateLimitGuardMock: vi.fn<() => RateLimitGuardResult>(() => ({ blocked: false })), noteRateLimitSpendMock: vi.fn(), + repositoryRateLimitGuardMock: vi.fn<() => RateLimitGuardResult>(() => ({ blocked: false })), + noteRepositoryRateLimitSpendMock: vi.fn(), ghRepoExecOptionsMock: vi.fn((context) => context.connectionId ? {} @@ -67,7 +71,9 @@ vi.mock('./github-enterprise-repository', () => ({ vi.mock('./rate-limit', () => ({ rateLimitGuard: rateLimitGuardMock, - noteRateLimitSpend: noteRateLimitSpendMock + noteRateLimitSpend: noteRateLimitSpendMock, + repositoryRateLimitGuard: repositoryRateLimitGuardMock, + noteRepositoryRateLimitSpend: noteRepositoryRateLimitSpendMock })) import { getPRFileContents, getWorkItemDetails } from './work-item-details' @@ -86,6 +92,9 @@ describe('getWorkItemDetails', () => { rateLimitGuardMock.mockReset() rateLimitGuardMock.mockReturnValue({ blocked: false }) noteRateLimitSpendMock.mockReset() + repositoryRateLimitGuardMock.mockReset() + repositoryRateLimitGuardMock.mockReturnValue({ blocked: false }) + noteRepositoryRateLimitSpendMock.mockReset() ghRepoExecOptionsMock.mockClear() githubRepoContextMock.mockClear() acquireMock.mockReset() @@ -410,12 +419,20 @@ describe('getWorkItemDetails', () => { }) it('skips optional GraphQL issue detail calls when the cached GraphQL budget is low', async () => { + // github.com repos share the singleton quota, so both the raw and + // repository-scoped guards report the same low-budget block. rateLimitGuardMock.mockReturnValue({ blocked: true, remaining: 3, limit: 5000, resetAt: 1_800_000_000 }) + repositoryRateLimitGuardMock.mockReturnValue({ + blocked: true, + remaining: 3, + limit: 5000, + resetAt: 1_800_000_000 + }) getWorkItemMock.mockResolvedValueOnce({ id: 'issue:923', type: 'issue', @@ -438,6 +455,7 @@ describe('getWorkItemDetails', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(3) expect(ghExecFileAsyncMock.mock.calls.some((call) => call[0][1] === 'graphql')).toBe(false) expect(noteRateLimitSpendMock).not.toHaveBeenCalled() + expect(noteRepositoryRateLimitSpendMock).not.toHaveBeenCalled() expect(details?.body).toBe('Issue body') expect(details?.participants).toEqual([]) }) @@ -687,12 +705,11 @@ describe('getWorkItemDetails', () => { .map(([args]) => args as string[]) .filter((args) => args[0] === 'api') expect(apiCalls.length).toBeGreaterThan(0) - expect(apiCalls.every((args) => args.includes('--hostname'))).toBe(true) + expect(apiCalls.every((args) => !args.includes('--hostname'))).toBe(true) expect( - apiCalls.every((args) => args[args.indexOf('--hostname') + 1] === 'github.acme-corp.com') - ).toBe(true) - expect( - ghExecFileAsyncMock.mock.calls.every(([, options]) => Object.keys(options).length === 0) + ghExecFileAsyncMock.mock.calls.every( + ([, options]) => options?.host === 'github.acme-corp.com' + ) ).toBe(true) }) @@ -768,9 +785,11 @@ describe('getWorkItemDetails', () => { }) const apiCalls = ghExecFileAsyncMock.mock.calls.map(([args]) => args as string[]) expect(apiCalls).toHaveLength(2) - expect(apiCalls.every((args) => args.includes('--hostname'))).toBe(true) + expect(apiCalls.every((args) => !args.includes('--hostname'))).toBe(true) expect( - apiCalls.every((args) => args[args.indexOf('--hostname') + 1] === 'github.acme-corp.com') + ghExecFileAsyncMock.mock.calls.every( + ([, options]) => options?.host === 'github.acme-corp.com' + ) ).toBe(true) }) diff --git a/src/main/github/work-item-details.ts b/src/main/github/work-item-details.ts index 98d6de19e20..9ea475613d4 100644 --- a/src/main/github/work-item-details.ts +++ b/src/main/github/work-item-details.ts @@ -23,11 +23,16 @@ import { import { getWorkItem, getWorkItemByOwnerRepo, getPRChecks, getPRComments } from './client' import { getOriginGitHubApiRepository, - githubApiHostArgs, + githubHostExecOptions, isGitHubDotComRepository, type GitHubApiRepository } from './github-api-repository' -import { noteRateLimitSpend, rateLimitGuard } from './rate-limit' +import { + noteRateLimitSpend, + noteRepositoryRateLimitSpend, + rateLimitGuard, + repositoryRateLimitGuard +} from './rate-limit' import { getPRReviewCommentLineNumbersFromPatch } from './pr-review-comment-lines' import { isMaxBufferOverflowError } from '../git/max-buffer-overflow' @@ -47,27 +52,6 @@ function encodeGitHubContentPath(path: string): string { return path.split('/').map(encodeURIComponent).join('/') } -function repositoryRateLimitGuard( - repository: GitHubApiRepository | null, - bucket: 'core' | 'graphql' | 'search', - executionOptions: Pick = {} -): ReturnType { - return (!repository || isGitHubDotComRepository(repository)) && !executionOptions.wslDistro - ? rateLimitGuard(bucket) - : { blocked: false } -} - -function noteRepositoryRateLimitSpend( - repository: GitHubApiRepository | null, - bucket: 'core' | 'graphql' | 'search', - cost = 1, - executionOptions: Pick = {} -): void { - if ((!repository || isGitHubDotComRepository(repository)) && !executionOptions.wslDistro) { - noteRateLimitSpend(bucket, cost) - } -} - const PR_FILE_VIEWED_STATES_QUERY = `query($owner: String!, $repo: String!, $number: Int!, $after: String) { repository(owner: $owner, name: $repo) { pullRequest(number: $number) { @@ -308,14 +292,13 @@ async function getIssueTimelineItems( const { stdout } = await ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), '--cache', '60s', `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues/${issueNumber}/timeline?per_page=${GITHUB_REST_PAGE_SIZE}&page=${page}`, '--jq', '.[] | @json' ], - ghOptions + { ...ghOptions, ...githubHostExecOptions(ownerRepo) } ) // Why: --jq emits NDJSON and explicit paging lets us stop once we hit the drawer cap. const pageEvents = parseRestTimelineEventLines(stdout) @@ -512,17 +495,14 @@ async function getPRMetadata( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ body: string; headSha?: string; baseSha?: string }> { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } try { if (ownerRepo) { const { stdout } = await ghExecFileAsync( - [ - 'api', - ...githubApiHostArgs(ownerRepo), - '--cache', - '60s', - `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${prNumber}` - ], + ['api', '--cache', '60s', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${prNumber}`], ghOptions ) const data = JSON.parse(stdout) as { @@ -563,10 +543,13 @@ async function getPRFiles( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) if (!ownerRepo) { return null } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } try { const data: RESTPRFile[] = [] for (let page = 1; data.length < MAX_PR_FILES; page += 1) { @@ -574,7 +557,6 @@ async function getPRFiles( const { stdout } = await ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), '--cache', '60s', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${prNumber}/files?per_page=100${pageSuffix}` @@ -613,10 +595,13 @@ async function getPRFileViewedStates( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) if (!ownerRepo) { return null } + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } if (repositoryRateLimitGuard(ownerRepo, 'graphql', localGitOptions).blocked) { return null } @@ -628,7 +613,6 @@ async function getPRFileViewedStates( for (let fetched = 0; fetched < MAX_PR_FILES; fetched += 100) { const args = [ 'api', - ...githubApiHostArgs(ownerRepo), 'graphql', '-f', `query=${PR_FILE_VIEWED_STATES_QUERY}`, @@ -819,7 +803,6 @@ async function getWorkItemParticipants( const { stdout } = await ghExecFileAsync( [ 'api', - ...githubApiHostArgs(ownerRepo), 'graphql', '-f', `query=${WORK_ITEM_PARTICIPANTS_QUERY}`, @@ -832,7 +815,10 @@ async function getWorkItemParticipants( '-F', `isPr=${item.type === 'pr'}` ], - ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) + { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } ) const data = JSON.parse(stdout) as { data?: { @@ -892,14 +878,11 @@ async function getGitHubUsersByLogin( try { noteRepositoryRateLimitSpend(repository, 'graphql', 1, localGitOptions) const { stdout } = await ghExecFileAsync( - [ - 'api', - ...(repository ? githubApiHostArgs(repository) : []), - 'graphql', - '-f', - `query=query { ${fields} }` - ], - ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) + ['api', 'graphql', '-f', `query=query { ${fields} }`], + { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(repository) + } ) const data = JSON.parse(stdout) as { data?: Record< @@ -1204,13 +1187,13 @@ async function fetchContentAtRef(args: { '300s', '-H', 'Accept: application/vnd.github.raw', - ...githubApiHostArgs(args.ownerRepo), `repos/${args.ownerRepo.owner}/${args.ownerRepo.repo}/contents/${encodeGitHubContentPath(args.path)}?ref=${encodeURIComponent(args.ref)}` ], { ...ghRepoExecOptions( githubRepoContext(args.repoPath, args.connectionId, args.localGitOptions) ), + ...githubHostExecOptions(args.ownerRepo), maxBuffer: GITHUB_RAW_CONTENT_MAX_BUFFER_BYTES } ) diff --git a/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.ts b/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.ts index 3728f8b91c9..e3cee92b293 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.ts +++ b/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.ts @@ -1,12 +1,8 @@ import type { GitHubOwnerRepo } from '../../../../shared/types' +import { githubRepoIdentityKey } from '../../../../shared/github-repository-identity-key' function normalizedPRRepoIdentity(prRepo?: GitHubOwnerRepo | null): string { - if (!prRepo) { - return 'none' - } - const slug = `${prRepo.owner.toLowerCase()}/${prRepo.repo.toLowerCase()}` - const host = prRepo.host?.toLowerCase() - return host && host !== 'github.com' ? `${host}/${slug}` : slug + return prRepo ? githubRepoIdentityKey(prRepo) : 'none' } export function checksPanelAsyncResultKey( diff --git a/src/renderer/src/store/slices/github-checks.ts b/src/renderer/src/store/slices/github-checks.ts index c90a83d2932..93a0e28d392 100644 --- a/src/renderer/src/store/slices/github-checks.ts +++ b/src/renderer/src/store/slices/github-checks.ts @@ -1,6 +1,7 @@ import type { AppState } from '../types' import type { PRCheckDetail, CheckStatus, GitHubOwnerRepo } from '../../../../shared/types' import { getGitHubPRCacheKey } from './github-cache-key' +import { githubRepoIdentityKey } from '../../../../shared/github-repository-identity-key' export function normalizeBranchName(branch: string): string { return branch.replace(/^refs\/heads\//, '') @@ -97,12 +98,7 @@ export function syncPRChecksStatus( } function normalizedPRRepo(repo?: GitHubOwnerRepo | null): string | null { - if (!repo) { - return null - } - const slug = `${repo.owner.toLowerCase()}/${repo.repo.toLowerCase()}` - const host = repo.host?.toLowerCase() - return host && host !== 'github.com' ? `${host}/${slug}` : slug + return repo ? githubRepoIdentityKey(repo) : null } function samePRRepo(left?: GitHubOwnerRepo | null, right?: GitHubOwnerRepo | null): boolean { diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index 46799393ed6..f9880d75eea 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -2,6 +2,7 @@ import type { StateCreator } from 'zustand' import { toast } from 'sonner' import type { AppState } from '../types' +import { githubRepoIdentityKey } from '../../../../shared/github-repository-identity-key' import type { ClassifiedError, GitHubOwnerRepo, @@ -859,9 +860,7 @@ function evictRepoCacheEntries( } function normalizedRepoIdentity(repo: GitHubOwnerRepo): string { - const slug = `${repo.owner.toLowerCase()}/${repo.repo.toLowerCase()}` - const host = repo.host?.toLowerCase() - return host && host !== 'github.com' ? `${host}/${slug}` : slug + return githubRepoIdentityKey(repo) } function normalizedHeadSha(headSha?: string): string | null { diff --git a/src/shared/github-repository-identity-key.ts b/src/shared/github-repository-identity-key.ts new file mode 100644 index 00000000000..cace13fb5ab --- /dev/null +++ b/src/shared/github-repository-identity-key.ts @@ -0,0 +1,12 @@ +// Why: cache keys and equality checks for GitHub repos must include the host, +// or a GHES repo and a same-named github.com repo would collide. github.com is +// omitted so pre-Enterprise host-less keys stay stable. +export function githubRepoIdentityKey(repo: { + owner: string + repo: string + host?: string +}): string { + const slug = `${repo.owner.toLowerCase()}/${repo.repo.toLowerCase()}` + const host = repo.host?.toLowerCase() + return host && host !== 'github.com' ? `${host}/${slug}` : slug +}