diff --git a/mobile/src/session/github-pr-mutations.ts b/mobile/src/session/github-pr-mutations.ts index 7d79518f089..dff1775f34d 100644 --- a/mobile/src/session/github-pr-mutations.ts +++ b/mobile/src/session/github-pr-mutations.ts @@ -1,6 +1,6 @@ import type { GitHubPRMergeMethod } from '../../../src/shared/types' import type { RpcClient } from '../transport/rpc-client' -import { buildGithubPrParams, type GitHubPrRepoSlug } from './github-pr-rpc' +import { buildGithubPrParams, githubPrRepoSlugParam, type GitHubPrRepoSlug } from './github-pr-rpc' // Mutation wrappers for the github.* PR surface, split out so github-pr-rpc.ts // stays under the max-lines budget. They mirror the read wrappers' shape but @@ -105,7 +105,7 @@ export async function fetchUpdatePRTitle( // METHODS_ACCEPTING_PR_REPO read allow-list — pass it explicitly so it reaches the // host schema (which declares it optional/nullable). if (args.prRepo) { - params.prRepo = { owner: args.prRepo.owner, repo: args.prRepo.repo } + params.prRepo = githubPrRepoSlugParam(args.prRepo) } const response = await sendRaw( client, @@ -226,7 +226,7 @@ export async function fetchAddPRReviewCommentReply( // centralized METHODS_ACCEPTING_PR_REPO allow-list (read-focused) — pass it // explicitly so it reaches the host schema, which declares it optional. if (args.prRepo) { - params.prRepo = { owner: args.prRepo.owner, repo: args.prRepo.repo } + params.prRepo = githubPrRepoSlugParam(args.prRepo) } return sendGithubPrMutation( client, @@ -247,7 +247,7 @@ export async function fetchAddIssueComment( type: 'pr' } if (args.prRepo) { - params.prRepo = { owner: args.prRepo.owner, repo: args.prRepo.repo } + params.prRepo = githubPrRepoSlugParam(args.prRepo) } return sendGithubPrMutation( client, diff --git a/mobile/src/session/github-pr-rpc.ts b/mobile/src/session/github-pr-rpc.ts index 8fd11f710a8..e24c50e8dfc 100644 --- a/mobile/src/session/github-pr-rpc.ts +++ b/mobile/src/session/github-pr-rpc.ts @@ -31,7 +31,13 @@ export { // Why: a fork PR's head lives in a different owner/repo; the host's SlugRepo // (`{ owner, repo }`) identifies it. Only a subset of github.* methods accept it. -export type GitHubPrRepoSlug = { owner: string; repo: string } +// Why: `host` must survive the RPC boundary or GHES actions on the host fall +// back to a same-named github.com repo (src/shared/types.ts identity contract). +export type GitHubPrRepoSlug = { owner: string; repo: string; host?: string } + +export function githubPrRepoSlugParam(slug: GitHubPrRepoSlug): Record { + return { owner: slug.owner, repo: slug.repo, ...(slug.host ? { host: slug.host } : {}) } +} export type GitHubPrReadOutcome = { ok: true; result: T } | { ok: false; error: string } @@ -62,7 +68,7 @@ export function buildGithubPrParams( ...params } if (options?.prRepo && METHODS_ACCEPTING_PR_REPO.has(method) && !('prRepo' in built)) { - built.prRepo = { owner: options.prRepo.owner, repo: options.prRepo.repo } + built.prRepo = githubPrRepoSlugParam(options.prRepo) } if (options?.headSha && METHODS_ACCEPTING_HEAD_SHA.has(method) && !('headSha' in built)) { built.headSha = options.headSha diff --git a/mobile/src/session/github-pr-value-readers.ts b/mobile/src/session/github-pr-value-readers.ts index 6ba8ccd6954..1756d51c59a 100644 --- a/mobile/src/session/github-pr-value-readers.ts +++ b/mobile/src/session/github-pr-value-readers.ts @@ -153,7 +153,10 @@ export function readRepoIdentity(value: unknown): GitHubRepositoryIdentity | und if (!owner || !repo) { return undefined } - return { owner, repo } + // Why: dropping `host` here would strip the GHES identity before every + // subsequent PR RPC, forcing the host to re-derive it per call. + const host = readString(value.host) + return { owner, repo, ...(host ? { host } : {}) } } function readMergeMethod(value: unknown): GitHubPRMergeMethod | undefined { diff --git a/src/main/git/gh-rate-limit-breaker.ts b/src/main/git/gh-rate-limit-breaker.ts index cf26b0f0f50..85394f0d540 100644 --- a/src/main/git/gh-rate-limit-breaker.ts +++ b/src/main/git/gh-rate-limit-breaker.ts @@ -25,7 +25,13 @@ const FALLBACK_BLOCK_MS: Record = { graphql: 5 * 60_000 } -const DEFAULT_SCOPE = 'native:github.com' +// Why: the runner derives scopes with this builder too — a format drift would +// silently split blocks from the default-scope reads below. +export function ghRateLimitScopeKey(runtime: string, host: string): string { + return `${runtime}:${host.toLowerCase()}` +} + +const DEFAULT_SCOPE = ghRateLimitScopeKey('native', 'github.com') const blockedUntilMsByScopeAndBucket = new Map() let resetProbe: ((bucket: GhRateLimitBucket) => void) | null = null diff --git a/src/main/git/runner-gh-host-args.test.ts b/src/main/git/runner-gh-host-args.test.ts index e4168dbaf9a..0bee44eac74 100644 --- a/src/main/git/runner-gh-host-args.test.ts +++ b/src/main/git/runner-gh-host-args.test.ts @@ -53,36 +53,56 @@ describe('applyGhHostToArgs', () => { ]) }) - it('qualifies inline long and short repo forms for a GHES host', () => { + 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 combined -R short forms untouched (indistinguishable from free-text values)', () => { expect(applyGhHostToArgs(['pr', 'list', '-R=a/b'], 'github.acme-corp.com')).toEqual([ 'pr', 'list', - '-R=github.acme-corp.com/a/b' + '-R=a/b' ]) expect(applyGhHostToArgs(['pr', 'list', '-Ra/b'], 'github.acme-corp.com')).toEqual([ 'pr', 'list', - '-Rgithub.acme-corp.com/a/b' + '-Ra/b' ]) }) - it('leaves --repo untouched for the github.com host (gh default resolution)', () => { + it('does not rewrite free-text flag values that merely start with -R', () => { + expect( + applyGhHostToArgs( + ['pr', 'edit', '7', '--title', '-Refactor foo/bar', '--repo', 'a/b'], + 'github.acme-corp.com' + ) + ).toEqual([ + 'pr', + 'edit', + '7', + '--title', + '-Refactor foo/bar', + '--repo', + 'github.acme-corp.com/a/b' + ]) + }) + + it('qualifies --repo for the github.com host so GH_HOST cannot redirect it', () => { expect(applyGhHostToArgs(['pr', 'list', '--repo', 'a/b'], 'github.com')).toEqual([ 'pr', 'list', '--repo', - 'a/b' + 'github.com/a/b' ]) expect(applyGhHostToArgs(['pr', 'list', '--repo', 'a/b'], 'GitHub.com')).toEqual([ 'pr', 'list', '--repo', - 'a/b' + 'GitHub.com/a/b' ]) }) @@ -101,13 +121,10 @@ describe('applyGhHostToArgs', () => { ).toEqual(['pr', 'list', '--repo', 'github.acme-corp.com/a/b']) expect( applyGhHostToArgs( - ['pr', 'list', '-R=https://github.acme-corp.com/a/b'], + ['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']) + ).toEqual(['pr', 'list', '-R', 'https://github.acme-corp.com/a/b']) }) it('injects --hostname and qualifies --repo together for GHES api calls', () => { diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index 65ff7501b3a..bf8741ee053 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -24,6 +24,7 @@ import { classifyGhRateLimitBucket, createGhRateLimitBlockedError, getGhRateLimitBlockedUntilMs, + ghRateLimitScopeKey, isGhPrimaryRateLimitStderr, isGhRateLimitProbe, notifyGhPrimaryRateLimit, @@ -1435,32 +1436,37 @@ export function applyGhHostToArgs(args: string[], host?: string): string[] { 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) + // Why: bare OWNER/REPO shorthand resolves against gh's default host — GH_HOST + // when set — so github.com must be qualified too, not just GHES, or a + // process-level GH_HOST redirects pinned github.com commands. + // Combined short forms (`-Ra/b`, `-R=a/b`) are deliberately not rewritten: + // no call site uses them, and prefix-matching `-R` corrupts free-text values + // of other flags (e.g. a --title that happens to start with `-R`). + const qualified: string[] = [] + for (let i = 0; i < result.length; i += 1) { + const arg = result[i] + if (arg === '--repo' || arg === '-R') { + qualified.push(arg) + const value = result[i + 1] + if (value !== undefined) { + qualified.push(hostQualifiedGhRepoValue(value, host)) + i += 1 + } + continue } if (arg.startsWith('--repo=')) { - return `--repo=${hostQualifiedGhRepoValue(arg.slice('--repo='.length), host)}` + qualified.push(`--repo=${hostQualifiedGhRepoValue(arg.slice('--repo='.length), host)}`) + continue } - 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 - }) + qualified.push(arg) + } + return qualified } function ghRateLimitScope(options: GhExecOptions, resolved: ResolvedCommand): string { 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()}` + return ghRateLimitScopeKey(runtime, host) } function assertGhRateLimitScopeAvailable( diff --git a/src/main/github/client-create-pr.test.ts b/src/main/github/client-create-pr.test.ts index 1c90e068d83..dd7a3fd0c70 100644 --- a/src/main/github/client-create-pr.test.ts +++ b/src/main/github/client-create-pr.test.ts @@ -60,6 +60,14 @@ vi.mock('./github-enterprise-repository', () => ({ import { createGitHubPullRequest } from './client' +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) + describe('createGitHubPullRequest', () => { beforeEach(() => { ghExecFileAsyncMock.mockReset() @@ -259,7 +267,7 @@ describe('createGitHubPullRequest', () => { url: 'https://github.com/acme/widgets/pull/45' }) - expect(getOwnerRepoMock).toHaveBeenCalledWith('/remote/repo-root', 'ssh-1') + expect(getOwnerRepoMock).toHaveBeenCalledWith('/remote/repo-root', 'ssh-1', {}) const [args, options] = ghExecFileAsyncMock.mock.calls[0] expect(args).toEqual( expect.arrayContaining([ @@ -417,7 +425,9 @@ describe('createGitHubPullRequest', () => { '--json', 'number,url' ], - { cwd: '/repo-root' } + // Why: dotcom slugs resolve with host:'github.com' so creation stays + // pinned against a process-level GH_HOST. + { cwd: '/repo-root', host: 'github.com' } ]) }) diff --git a/src/main/github/client-file-viewed.test.ts b/src/main/github/client-file-viewed.test.ts index 79ad4e6a752..d00f34a3433 100644 --- a/src/main/github/client-file-viewed.test.ts +++ b/src/main/github/client-file-viewed.test.ts @@ -45,6 +45,14 @@ vi.mock('./github-enterprise-repository', () => ({ import { setPRFileViewed } from './client' +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) + describe('setPRFileViewed', () => { beforeEach(() => { ghExecFileAsyncMock.mockReset() diff --git a/src/main/github/client-pr-checks.test.ts b/src/main/github/client-pr-checks.test.ts index 82310af697b..782fa277d39 100644 --- a/src/main/github/client-pr-checks.test.ts +++ b/src/main/github/client-pr-checks.test.ts @@ -80,6 +80,14 @@ vi.mock('./rate-limit', () => ({ import { getPRChecks, rerunPRChecks, _resetOwnerRepoCache } from './client' +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) + function graphQLChecksResponse({ contexts = [], checkSuites = [], diff --git a/src/main/github/client-pr-local-runtime.test.ts b/src/main/github/client-pr-local-runtime.test.ts index 8e19df99f56..03c00cd0802 100644 --- a/src/main/github/client-pr-local-runtime.test.ts +++ b/src/main/github/client-pr-local-runtime.test.ts @@ -122,6 +122,14 @@ import { updatePRTitle } from './client' +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) + describe('GitHub PR local runtime routing', () => { beforeEach(() => { execFileAsyncMock.mockReset() diff --git a/src/main/github/client-work-items.test.ts b/src/main/github/client-work-items.test.ts index 70fe8fc2804..35191c3be20 100644 --- a/src/main/github/client-work-items.test.ts +++ b/src/main/github/client-work-items.test.ts @@ -20,7 +20,7 @@ const { getOwnerRepoForRemoteMock: vi.fn(), resolveIssueSourceMock: vi.fn(), gitExecFileAsyncMock: vi.fn(), - rateLimitGuardMock: vi.fn(() => ({ blocked: false })), + rateLimitGuardMock: vi.fn((_bucket?: unknown) => ({ blocked: false })), noteRateLimitSpendMock: vi.fn(), acquireMock: vi.fn(), releaseMock: vi.fn() @@ -65,8 +65,11 @@ vi.mock('./rate-limit', () => ({ rateLimitGuard: rateLimitGuardMock, noteRateLimitSpend: noteRateLimitSpendMock, getRateLimit: vi.fn(async () => ({ ok: false, error: 'not probed in tests' })), - repositoryRateLimitGuard: vi.fn(() => ({ blocked: false })), - noteRepositoryRateLimitSpend: vi.fn(), + // Mirror production: shared-scope calls delegate to the global guard/spend. + repositoryRateLimitGuard: vi.fn((_repo: unknown, bucket: string) => rateLimitGuardMock(bucket)), + noteRepositoryRateLimitSpend: vi.fn((_repo: unknown, bucket: string, cost?: number) => + noteRateLimitSpendMock(bucket, cost) + ), spendsSharedGitHubComQuota: () => true })) @@ -79,6 +82,14 @@ import { import { _resetGhCwdRepoNegativeCache } from './gh-cwd-repo-negative-cache' import { GITHUB_WORK_ITEMS_QUERY_MAX_BYTES } from '../../shared/github-work-items-query-bounds' +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) + describe('listWorkItems', () => { beforeEach(() => { execFileAsyncMock.mockReset() diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index 2a3c102f0c0..86d12c075ee 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -134,6 +134,14 @@ import { import { __resetPRConflictSummaryCachesForTests } from './conflict-summary' import { resetMergedPRCommitMembershipCacheForTest } from './merged-pr-commit-membership' +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) + describe('checkOrcaStarred', () => { beforeEach(() => { execFileAsyncMock.mockReset() diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 1b24d97648b..5e31b44dfb9 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -79,7 +79,6 @@ import { rememberGhCwdResolutionFailure } from './gh-cwd-repo-negative-cache' import type { GitHubRepoContext } from './github-repository-identity' -import { getEnterpriseGitHubRepoSlug } from './github-enterprise-repository' import { getOriginGitHubApiRepository, githubHostExecOptions, @@ -111,7 +110,6 @@ import { import { mapGraphQLReactionGroups, type GitHubGraphQLReactionGroup } from './comment-reactions' import { getRateLimit, - noteRateLimitSpend, noteRepositoryRateLimitSpend, rateLimitGuard, repositoryRateLimitGuard, @@ -1520,7 +1518,7 @@ async function countWorkItemsForQuery( ) // Why: over-counts gh cache hits, which is the safe direction — the search // bucket is only 30/min and the next probe corrects the estimate. - noteRateLimitSpend('search') + noteRepositoryRateLimitSpend(ownerRepo, 'search', 1, localGitOptions) return Number.parseInt(stdout.trim(), 10) || 0 } @@ -1580,8 +1578,11 @@ export async function countWorkItems( // 30/min, so a multi-repo Tasks page must stop counting when the budget is // gone instead of converting the remaining repos into 403 spawns. getRateLimit // is 30s-cached and single-flight, so priming here is one spawn per window. - await getRateLimit() - if (rateLimitGuard('search').blocked) { + // Only shared github.com traffic consults the snapshot; other scopes bypass. + if (spendsSharedGitHubComQuota(ownerRepo, localGitOptions)) { + await getRateLimit() + } + if (repositoryRateLimitGuard(ownerRepo, 'search', localGitOptions).blocked) { return 0 } @@ -1876,11 +1877,14 @@ export async function createGitHubPullRequest( } } - // Why: github.com-only slug parsing returns null for GHES, so fall back to the - // enterprise resolver (gh-authenticated custom host) before giving up (#8312). - const ownerRepo = - (await getOwnerRepo(repoPath, connectionId, ...hostedReviewLocalGitOptionArgs(options))) ?? - (await getEnterpriseGitHubRepoSlug(repoPath, connectionId, options)) + // Why: the shared origin resolver covers GHES fallback and stamps + // host:'github.com' on dotcom slugs, keeping creation pinned against GH_HOST + // like every other PR path (#8312). + const ownerRepo = await getOriginGitHubApiRepository( + repoPath, + connectionId, + getHostedReviewLocalGitOptions(options) + ) if (!ownerRepo) { return { ok: false, @@ -4359,7 +4363,7 @@ export async function getPRComments( ['pr', 'view', String(prNumber), '--json', 'comments'], ghOptions ) - noteRateLimitSpend('graphql') + noteRepositoryRateLimitSpend(ownerRepo, 'graphql', 1, ghOptions) const data = JSON.parse(stdout) as { comments: { author: { login: string } diff --git a/src/main/github/github-api-repository.ts b/src/main/github/github-api-repository.ts index cd7284dd90e..571ec5a5b2d 100644 --- a/src/main/github/github-api-repository.ts +++ b/src/main/github/github-api-repository.ts @@ -1,9 +1,31 @@ import type { GitHubOwnerRepo } from '../../shared/types' +import { isDefaultGitHubHost } from '../../shared/github-repository-identity-key' import { getOwnerRepo, type LocalGitExecOptions } from './gh-utils' import { getEnterpriseGitHubRepoSlug } from './github-enterprise-repository' export type GitHubApiRepository = GitHubOwnerRepo +// Why: the enterprise branch spawns an uncached `git remote get-url` (an SSH +// round trip on connection-backed repos) — hot paths like per-file contents +// and viewed-state toggles resolve per call, so cache like ownerRepoCache does. +const ORIGIN_REPO_CACHE_TTL_MS = 30_000 +const originRepoCache = new Map() +const originRepoInFlight = new Map>() + +function originRepoCacheKey( + repoPath: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): string { + return `${connectionId ?? 'local'}\0${localGitOptions.wslDistro ?? ''}\0${repoPath}` +} + +/** @internal - exposed for tests only */ +export function _resetOriginGitHubApiRepositoryCache(): void { + originRepoCache.clear() + originRepoInFlight.clear() +} + export async function getOriginGitHubApiRepository( repoPath: string, connectionId?: string | null, @@ -13,9 +35,30 @@ export async function getOriginGitHubApiRepository( if (ownerRepo) { return { ...ownerRepo, host: 'github.com' } } - const enterpriseOptions = - Object.keys(localGitOptions).length > 0 ? { localGitExecOptions: localGitOptions } : {} - return getEnterpriseGitHubRepoSlug(repoPath, connectionId, enterpriseOptions) + const cacheKey = originRepoCacheKey(repoPath, connectionId, localGitOptions) + const cached = originRepoCache.get(cacheKey) + if (cached && cached.expiresAt > Date.now()) { + return cached.value + } + const inFlight = originRepoInFlight.get(cacheKey) + if (inFlight) { + return inFlight + } + const probe = (async () => { + const enterpriseOptions = + Object.keys(localGitOptions).length > 0 ? { localGitExecOptions: localGitOptions } : {} + const slug = await getEnterpriseGitHubRepoSlug(repoPath, connectionId, enterpriseOptions) + originRepoCache.set(cacheKey, { value: slug, expiresAt: Date.now() + ORIGIN_REPO_CACHE_TTL_MS }) + return slug + })() + originRepoInFlight.set(cacheKey, probe) + try { + return await probe + } finally { + if (originRepoInFlight.get(cacheKey) === probe) { + originRepoInFlight.delete(cacheKey) + } + } } export async function resolveGitHubApiRepository( @@ -46,7 +89,7 @@ export async function resolveGitHubApiRepository( } export function isGitHubDotComRepository(repository: GitHubApiRepository): boolean { - return !repository.host || repository.host.toLowerCase() === 'github.com' + return isDefaultGitHubHost(repository.host) } // Why: the gh runner host-qualifies argv from `options.host`, so every known diff --git a/src/main/github/issues.test.ts b/src/main/github/issues.test.ts index e804a6cbc5c..7ff695ba12d 100644 --- a/src/main/github/issues.test.ts +++ b/src/main/github/issues.test.ts @@ -37,6 +37,14 @@ import { updateIssue } from './issues' +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) + describe('issue source operations', () => { beforeEach(() => { ghExecFileAsyncMock.mockReset() diff --git a/src/main/github/rate-limit.ts b/src/main/github/rate-limit.ts index 721c0b52f76..d3a4a4a056f 100644 --- a/src/main/github/rate-limit.ts +++ b/src/main/github/rate-limit.ts @@ -18,6 +18,7 @@ import type { GitHubRateLimitBucket, GitHubRateLimitSnapshot } from '../../shared/types' +import { isDefaultGitHubHost } from '../../shared/github-repository-identity-key' import { acquire, release } from './gh-utils' import { ghExecFileAsync } from '../git/runner' import { @@ -168,8 +169,7 @@ export function spendsSharedGitHubComQuota( repository: { host?: string } | null | undefined, executionOptions?: { wslDistro?: string } ): boolean { - const host = repository?.host - return (!host || host.toLowerCase() === 'github.com') && !executionOptions?.wslDistro + return isDefaultGitHubHost(repository?.host) && !executionOptions?.wslDistro } export function repositoryRateLimitGuard( 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 b8816bf4e0b..3ebe70aa1d6 100644 --- a/src/main/github/work-item-details-file-viewed.test.ts +++ b/src/main/github/work-item-details-file-viewed.test.ts @@ -61,6 +61,14 @@ vi.mock('./rate-limit', () => ({ import { getWorkItemDetails } from './work-item-details' +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) + describe('getWorkItemDetails PR file viewed state', () => { beforeEach(() => { ghExecFileAsyncMock.mockReset() 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 cbd043fe996..092b800a807 100644 --- a/src/main/github/work-item-details-pr-files.test.ts +++ b/src/main/github/work-item-details-pr-files.test.ts @@ -48,6 +48,14 @@ vi.mock('./rate-limit', () => ({ import { getWorkItemDetails } from './work-item-details' +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) + function pullRequestItem(number: number, title: string): Record { return { id: `pr:${number}`, diff --git a/src/main/github/work-item-details.test.ts b/src/main/github/work-item-details.test.ts index cfb6641fda9..3c39a252df5 100644 --- a/src/main/github/work-item-details.test.ts +++ b/src/main/github/work-item-details.test.ts @@ -78,6 +78,14 @@ vi.mock('./rate-limit', () => ({ import { getPRFileContents, getWorkItemDetails } from './work-item-details' +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' + +// The origin-repository cache is module-level state; reset it so slugs +// resolved by one test cannot leak into the next. +beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() +}) + describe('getWorkItemDetails', () => { beforeEach(() => { ghExecFileAsyncMock.mockReset() diff --git a/src/main/github/work-item-details.ts b/src/main/github/work-item-details.ts index b76508f8653..1af2c90ad4b 100644 --- a/src/main/github/work-item-details.ts +++ b/src/main/github/work-item-details.ts @@ -1086,14 +1086,15 @@ export async function getWorkItemDetails( : await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) // Why: SSH-backed gh runs without a cwd, so an unresolved PR repository must // not fall through to gh's default host and show a same-number github.com PR. - if (connectionId && type === 'pr' && !originApiRepository) { + // Covers type-less lookups too — they can also reach the PR path below. + if (connectionId && type !== 'issue' && !originApiRepository) { return null } // Why: connection-backed repos have no local cwd for gh to inspect, so GHES // must use its host-qualified repository identity during the initial lookup. const useExplicitEnterpriseRepository = type === 'pr' && originApiRepository !== null && !isGitHubDotComRepository(originApiRepository) - const item: Omit | null = useExplicitEnterpriseRepository + let item: Omit | null = useExplicitEnterpriseRepository ? await getWorkItemByOwnerRepo( repoPath, originApiRepository, @@ -1109,6 +1110,17 @@ export async function getWorkItemDetails( connectionId, ...localGitOptionArgs(localGitOptions) ) + if (!item && useExplicitEnterpriseRepository && !connectionId) { + // Why: on a local fork checkout the PR lives on the base repo, which the + // origin slug can't see — the cwd lets gh resolve the actual remote. + item = await getWorkItem( + repoPath, + number, + type, + connectionId, + ...localGitOptionArgs(localGitOptions) + ) + } if (!item) { return null } diff --git a/src/main/ipc/github.ts b/src/main/ipc/github.ts index 84afec20c3b..f693a750ed2 100644 --- a/src/main/ipc/github.ts +++ b/src/main/ipc/github.ts @@ -474,6 +474,7 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi repoPath: string owner: string repo: string + host?: string number: number type: 'issue' | 'pr' } @@ -481,7 +482,9 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi const repo = assertRegisteredRepo(args, store) return getWorkItemByOwnerRepo( repo.path, - { owner: args.owner, repo: args.repo }, + // Why: Enterprise host identity must survive the IPC boundary or the + // lookup falls back to gh's default host for a same-named repo. + { owner: args.owner, repo: args.repo, ...(args.host ? { host: args.host } : {}) }, args.number, args.type, repoConnectionId(repo), diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 6e1bce48a5a..8a8e8916f93 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -13483,7 +13483,7 @@ export class OrcaRuntimeService { async getRepoWorkItemByOwnerRepo( repoSelector: string, - ownerRepo: { owner: string; repo: string }, + ownerRepo: { owner: string; repo: string; host?: string }, number: number, type: 'issue' | 'pr' ): Promise>> { diff --git a/src/main/runtime/rpc/methods/github.ts b/src/main/runtime/rpc/methods/github.ts index b72019dccac..e10afbd82af 100644 --- a/src/main/runtime/rpc/methods/github.ts +++ b/src/main/runtime/rpc/methods/github.ts @@ -26,6 +26,9 @@ const WorkItem = RepoSelector.extend({ const WorkItemByOwnerRepo = RepoSelector.extend({ owner: requiredString('Missing owner'), ownerRepo: requiredString('Missing repo'), + // Why: Enterprise host identity must survive RPC parsing; Zod strips + // undeclared fields before the runtime can host-qualify gh requests. + host: OptionalString, number: z.number().int().positive(), type: z.enum(['issue', 'pr']) }) @@ -345,7 +348,11 @@ export const GITHUB_METHODS: RpcMethod[] = [ handler: async (params, { runtime }) => runtime.getRepoWorkItemByOwnerRepo( params.repo, - { owner: params.owner, repo: params.ownerRepo }, + { + owner: params.owner, + repo: params.ownerRepo, + ...(params.host ? { host: params.host } : {}) + }, params.number, params.type ) diff --git a/src/main/source-control/hosted-review-creation.test.ts b/src/main/source-control/hosted-review-creation.test.ts index 04c751d7c3b..abfcd11935e 100644 --- a/src/main/source-control/hosted-review-creation.test.ts +++ b/src/main/source-control/hosted-review-creation.test.ts @@ -393,7 +393,7 @@ describe('createHostedReview', () => { ) expect(ghExecFileAsyncMock).toHaveBeenCalledWith( ['auth', 'status', '--hostname', 'github.com'], - { cwd: '/repo', wslDistro: 'Ubuntu' } + { cwd: '/repo', wslDistro: 'Ubuntu', host: 'github.com' } ) expect(createGitHubPullRequestMock).toHaveBeenCalledWith( '/repo', @@ -578,7 +578,7 @@ describe('createHostedReview', () => { expect(getUpstreamStatusMock).not.toHaveBeenCalled() expect(ghExecFileAsyncMock).toHaveBeenCalledWith( ['auth', 'status', '--hostname', 'github.com'], - {} + { host: 'github.com' } ) expect(createGitHubPullRequestMock).toHaveBeenCalledWith( '/remote/repo', diff --git a/src/main/source-control/hosted-review-creation.ts b/src/main/source-control/hosted-review-creation.ts index 1e19f1d4fa8..68633a29eeb 100644 --- a/src/main/source-control/hosted-review-creation.ts +++ b/src/main/source-control/hosted-review-creation.ts @@ -74,9 +74,13 @@ async function isGitHubAuthenticated( } await acquire() try { + // Why: `host` scopes any rate-limit breaker trip to github.com — the host + // this probe actually targets — instead of a GH_HOST-derived scope. await ghExecFileAsync( ['auth', 'status', '--hostname', 'github.com'], - connectionId ? {} : { cwd: repoPath, ...getHostedReviewLocalGitOptions(options) } + connectionId + ? { host: 'github.com' } + : { cwd: repoPath, ...getHostedReviewLocalGitOptions(options), host: 'github.com' } ) return true } catch { diff --git a/src/preload/index.ts b/src/preload/index.ts index d1f0d37ea30..1395d808906 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -1275,6 +1275,7 @@ const api = { repoId?: string owner: string repo: string + host?: string number: number type: 'issue' | 'pr' }): Promise => ipcRenderer.invoke('gh:workItemByOwnerRepo', args), diff --git a/src/renderer/src/components/PullRequestPage.tsx b/src/renderer/src/components/PullRequestPage.tsx index b53ba354618..93e655c22e9 100644 --- a/src/renderer/src/components/PullRequestPage.tsx +++ b/src/renderer/src/components/PullRequestPage.tsx @@ -239,7 +239,9 @@ function parseOwnerRepoFromItemUrl(url: string): GitHubOwnerRepo | null { if (segments.length < 2) { return null } - return { owner: segments[0], repo: segments[1] } + // Why: the URL proves the host. Leaving it off lets the main process stamp + // a GHES origin's host onto what is provably a github.com repo. + return { owner: segments[0], repo: segments[1], host: 'github.com' } } catch { return null } diff --git a/src/shared/github-repository-identity-key.ts b/src/shared/github-repository-identity-key.ts index cace13fb5ab..b8631ac404b 100644 --- a/src/shared/github-repository-identity-key.ts +++ b/src/shared/github-repository-identity-key.ts @@ -1,3 +1,10 @@ +// Why: the github.com-vs-GHES boundary is the core invariant of Enterprise +// support — cache identity, quota scoping, and exec-host routing must all +// agree on it, so the predicate lives here once. +export function isDefaultGitHubHost(host?: string): boolean { + return !host || host.toLowerCase() === 'github.com' +} + // 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. @@ -7,6 +14,5 @@ export function githubRepoIdentityKey(repo: { host?: string }): string { const slug = `${repo.owner.toLowerCase()}/${repo.repo.toLowerCase()}` - const host = repo.host?.toLowerCase() - return host && host !== 'github.com' ? `${host}/${slug}` : slug + return repo.host && !isDefaultGitHubHost(repo.host) ? `${repo.host.toLowerCase()}/${slug}` : slug }