diff --git a/src/main/git/gh-rate-limit-breaker.test.ts b/src/main/git/gh-rate-limit-breaker.test.ts index 0975dd325ae..8cd972ac776 100644 --- a/src/main/git/gh-rate-limit-breaker.test.ts +++ b/src/main/git/gh-rate-limit-breaker.test.ts @@ -41,6 +41,7 @@ describe('isGhRateLimitProbe', () => { it('recognizes the exempt rate_limit endpoint only', () => { expect(isGhRateLimitProbe(['api', 'rate_limit'])).toBe(true) expect(isGhRateLimitProbe(['api', '/rate_limit'])).toBe(true) + expect(isGhRateLimitProbe(['api', '--hostname', 'github.com', 'rate_limit'])).toBe(true) expect(isGhRateLimitProbe(['api', 'search/issues?q=x'])).toBe(false) expect(isGhRateLimitProbe(['pr', 'list'])).toBe(false) }) @@ -76,6 +77,15 @@ describe('breaker state', () => { expect(getGhRateLimitBlockedUntilMs('core', now)).toBe(now + 60_000) }) + it('isolates blocks by host and execution runtime scope', () => { + const now = 1_000_000 + recordGhPrimaryRateLimit('core', now + 60_000, 'native:github.com') + + expect(getGhRateLimitBlockedUntilMs('core', now, 'native:github.com')).toBe(now + 60_000) + expect(getGhRateLimitBlockedUntilMs('core', now, 'native:github.acme-corp.com')).toBeNull() + expect(getGhRateLimitBlockedUntilMs('core', now, 'wsl:ubuntu:github.com')).toBeNull() + }) + it('notifyGhPrimaryRateLimit applies a fallback block and fires the reset probe', () => { const probe = vi.fn() registerGhRateLimitResetProbe(probe) @@ -84,6 +94,18 @@ describe('breaker state', () => { expect(getGhRateLimitBlockedUntilMs('search')).toBeGreaterThan(Date.now()) }) + it('does not refine a non-default scope with the native github.com probe', () => { + const probe = vi.fn() + registerGhRateLimitResetProbe(probe) + + notifyGhPrimaryRateLimit('core', 'native:github.acme-corp.com') + + expect(probe).not.toHaveBeenCalled() + expect( + getGhRateLimitBlockedUntilMs('core', Date.now(), 'native:github.acme-corp.com') + ).not.toBeNull() + }) + it('creates an error that classifies as rate_limited, not permission_denied', () => { const error = createGhRateLimitBlockedError('search', Date.now() + 30_000) expect(error.stderr.toLowerCase()).toContain('rate limit') diff --git a/src/main/git/gh-rate-limit-breaker.ts b/src/main/git/gh-rate-limit-breaker.ts index 94bef46ff7a..cf26b0f0f50 100644 --- a/src/main/git/gh-rate-limit-breaker.ts +++ b/src/main/git/gh-rate-limit-breaker.ts @@ -25,9 +25,14 @@ const FALLBACK_BLOCK_MS: Record = { graphql: 5 * 60_000 } -const blockedUntilMsByBucket = new Map() +const DEFAULT_SCOPE = 'native:github.com' +const blockedUntilMsByScopeAndBucket = new Map() let resetProbe: ((bucket: GhRateLimitBucket) => void) | null = null +function breakerKey(bucket: GhRateLimitBucket, scope = DEFAULT_SCOPE): string { + return `${scope}\0${bucket}` +} + // gh api flags that take a separate value, so the endpoint arg can be found. const GH_API_VALUE_FLAGS = new Set([ '--cache', @@ -103,25 +108,32 @@ export function isGhPrimaryRateLimitStderr(stderr: string): boolean { return s.includes('api rate limit exceeded') && !s.includes('secondary rate limit') } -export function recordGhPrimaryRateLimit(bucket: GhRateLimitBucket, blockedUntilMs: number): void { - const existing = blockedUntilMsByBucket.get(bucket) ?? 0 - blockedUntilMsByBucket.set(bucket, Math.max(existing, blockedUntilMs)) +export function recordGhPrimaryRateLimit( + bucket: GhRateLimitBucket, + blockedUntilMs: number, + scope = DEFAULT_SCOPE +): void { + const key = breakerKey(bucket, scope) + const existing = blockedUntilMsByScopeAndBucket.get(key) ?? 0 + blockedUntilMsByScopeAndBucket.set(key, Math.max(existing, blockedUntilMs)) } -export function clearGhRateLimitBlock(bucket: GhRateLimitBucket): void { - blockedUntilMsByBucket.delete(bucket) +export function clearGhRateLimitBlock(bucket: GhRateLimitBucket, scope = DEFAULT_SCOPE): void { + blockedUntilMsByScopeAndBucket.delete(breakerKey(bucket, scope)) } export function getGhRateLimitBlockedUntilMs( bucket: GhRateLimitBucket, - nowMs: number = Date.now() + nowMs: number = Date.now(), + scope = DEFAULT_SCOPE ): number | null { - const blockedUntil = blockedUntilMsByBucket.get(bucket) + const key = breakerKey(bucket, scope) + const blockedUntil = blockedUntilMsByScopeAndBucket.get(key) if (blockedUntil === undefined) { return null } if (blockedUntil <= nowMs) { - blockedUntilMsByBucket.delete(bucket) + blockedUntilMsByScopeAndBucket.delete(key) return null } return blockedUntil @@ -136,8 +148,13 @@ export function registerGhRateLimitResetProbe( } /** Called by the runner when a gh spawn came back with a primary 403. */ -export function notifyGhPrimaryRateLimit(bucket: GhRateLimitBucket): void { - recordGhPrimaryRateLimit(bucket, Date.now() + FALLBACK_BLOCK_MS[bucket]) +export function notifyGhPrimaryRateLimit(bucket: GhRateLimitBucket, scope = DEFAULT_SCOPE): void { + recordGhPrimaryRateLimit(bucket, Date.now() + FALLBACK_BLOCK_MS[bucket], scope) + // The registered probe uses native github.com credentials; it cannot refine + // a GHES or WSL bucket without querying the wrong account/runtime. + if (scope !== DEFAULT_SCOPE) { + return + } try { resetProbe?.(bucket) } catch { @@ -161,6 +178,6 @@ export function createGhRateLimitBlockedError( /** @internal — test-only */ export function _resetGhRateLimitBreaker(): void { - blockedUntilMsByBucket.clear() + blockedUntilMsByScopeAndBucket.clear() resetProbe = null } 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 dc22bb54f4e..471e9df45ce 100644 --- a/src/main/git/runner-gh-rate-limit-breaker.test.ts +++ b/src/main/git/runner-gh-rate-limit-breaker.test.ts @@ -71,11 +71,35 @@ describe('ghExecFileAsync rate-limit breaker', () => { expect(execFileMock).toHaveBeenCalledTimes(2) }) + it('keeps other GitHub hosts and WSL runtimes working when github.com is blocked', async () => { + mockGhFailure(PRIMARY_RATE_LIMIT_STDERR) + await expect(ghExecFileAsync(['api', 'repos/a/b/pulls'])).rejects.toThrow() + + mockGhSuccess('[]') + await expect( + ghExecFileAsync(['api', '--hostname', 'github.acme-corp.com', 'repos/a/b/pulls']) + ).resolves.toMatchObject({ stdout: '[]' }) + await expect( + ghExecFileAsync(['pr', 'list', '--repo', 'github.acme-corp.com/a/b']) + ).resolves.toMatchObject({ stdout: '[]' }) + await expect( + ghExecFileAsync(['api', 'repos/a/b/pulls'], { + env: { ...process.env, GH_HOST: 'github.acme-corp.com' } + }) + ).resolves.toMatchObject({ stdout: '[]' }) + await expect( + ghExecFileAsync(['api', 'repos/a/b/pulls'], { wslDistro: 'Ubuntu' }) + ).resolves.toMatchObject({ stdout: '[]' }) + expect(execFileMock).toHaveBeenCalledTimes(5) + }) + it('never blocks the exempt rate_limit probe', async () => { mockGhFailure(PRIMARY_RATE_LIMIT_STDERR) await expect(ghExecFileAsync(['api', 'repos/a/b/pulls'])).rejects.toThrow() mockGhSuccess('{"resources":{}}') - await expect(ghExecFileAsync(['api', 'rate_limit'])).resolves.toMatchObject({ + await expect( + ghExecFileAsync(['api', '--hostname', 'github.com', 'rate_limit']) + ).resolves.toMatchObject({ stdout: '{"resources":{}}' }) expect(execFileMock).toHaveBeenCalledTimes(2) diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index fac9f57ecac..55e28c9704f 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -1253,6 +1253,44 @@ 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 ghRequestHost(args: readonly string[], env?: NodeJS.ProcessEnv): string { + const explicitHost = ghFlagValue(args, ['--hostname']) + if (explicitHost) { + return explicitHost.toLowerCase() + } + 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() +} + +function ghRateLimitScope( + args: readonly string[], + 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)}` +} + /** * Async gh CLI execution. Drop-in replacement for * `execFileAsync('gh', args, { cwd, encoding, ... })`. @@ -1264,15 +1302,21 @@ export async function ghExecFileAsync( args: string[], options: GhExecOptions = {} ): Promise<{ stdout: string; stderr: string }> { + 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) if (!isGhRateLimitProbe(args)) { - const blockedUntilMs = getGhRateLimitBlockedUntilMs(rateLimitBucket) + const blockedUntilMs = getGhRateLimitBlockedUntilMs( + rateLimitBucket, + Date.now(), + initialRateLimitScope + ) if (blockedUntilMs !== null) { throw createGhRateLimitBlockedError(rateLimitBucket, blockedUntilMs) } } - let resolved = resolveCommand('gh', args, options.cwd, options.wslDistro) let lastError: unknown let attemptedHostFallback = false let attemptedDefaultWslFallback = false @@ -1291,7 +1335,7 @@ export async function ghExecFileAsync( lastError = err const { stderr } = extractExecError(err) if (isGhPrimaryRateLimitStderr(stderr)) { - notifyGhPrimaryRateLimit(rateLimitBucket) + notifyGhPrimaryRateLimit(rateLimitBucket, ghRateLimitScope(args, options, resolved)) } if ( process.platform === 'win32' && diff --git a/src/main/github/client-file-viewed.test.ts b/src/main/github/client-file-viewed.test.ts index c4345ffb6b6..45cd1146cbd 100644 --- a/src/main/github/client-file-viewed.test.ts +++ b/src/main/github/client-file-viewed.test.ts @@ -1,7 +1,15 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -const { ghExecFileAsyncMock, acquireMock, releaseMock } = vi.hoisted(() => ({ +const { + ghExecFileAsyncMock, + getOwnerRepoMock, + getEnterpriseGitHubRepoSlugMock, + acquireMock, + releaseMock +} = vi.hoisted(() => ({ ghExecFileAsyncMock: vi.fn(), + getOwnerRepoMock: vi.fn(), + getEnterpriseGitHubRepoSlugMock: vi.fn(), acquireMock: vi.fn(), releaseMock: vi.fn() })) @@ -10,7 +18,7 @@ vi.mock('./gh-utils', () => ({ execFileAsync: vi.fn(), ghExecFileAsync: ghExecFileAsyncMock, gitExecFileAsync: vi.fn(), - getOwnerRepo: vi.fn(), + getOwnerRepo: getOwnerRepoMock, getIssueOwnerRepo: vi.fn(), getOwnerRepoForRemote: vi.fn(), resolveIssueSource: vi.fn(), @@ -31,11 +39,19 @@ vi.mock('./gh-utils', () => ({ _resetOwnerRepoCache: vi.fn() })) +vi.mock('./github-enterprise-repository', () => ({ + getEnterpriseGitHubRepoSlug: getEnterpriseGitHubRepoSlugMock +})) + import { setPRFileViewed } from './client' describe('setPRFileViewed', () => { beforeEach(() => { ghExecFileAsyncMock.mockReset() + getOwnerRepoMock.mockReset() + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'orca' }) + getEnterpriseGitHubRepoSlugMock.mockReset() + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null) acquireMock.mockReset() releaseMock.mockReset() acquireMock.mockResolvedValue(undefined) @@ -98,4 +114,35 @@ describe('setPRFileViewed', () => { wslDistro: 'Ubuntu' }) }) + + it('routes SSH-backed GitHub Enterprise file-viewed mutations to the Enterprise host', async () => { + getOwnerRepoMock.mockResolvedValueOnce(null) + getEnterpriseGitHubRepoSlugMock.mockResolvedValueOnce({ + owner: 'team', + repo: 'orca', + host: 'github.acme-corp.com' + }) + ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '{}' }) + + await expect( + setPRFileViewed({ + repoPath: '/remote/repo', + connectionId: 'ssh-1', + pullRequestId: 'PR_enterprise', + path: 'src/enterprise.ts', + viewed: true + }) + ).resolves.toBe(true) + + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + expect.arrayContaining([ + 'api', + '--hostname', + 'github.acme-corp.com', + 'graphql', + 'pullRequestId=PR_enterprise' + ]), + {} + ) + }) }) diff --git a/src/main/github/client-pr-check-details.test.ts b/src/main/github/client-pr-check-details.test.ts index 429969b52f1..9cacf42ab74 100644 --- a/src/main/github/client-pr-check-details.test.ts +++ b/src/main/github/client-pr-check-details.test.ts @@ -210,4 +210,60 @@ describe('getPRCheckDetails', () => { ) expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(4) }) + + it('isolates failed-job log tails for the same job ID on different GitHub hosts', async () => { + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + const endpoint = args.find((arg) => arg.startsWith('repos/')) ?? '' + const enterprise = args.includes('github.acme-corp.com') + if (endpoint.endsWith('/check-runs/99')) { + return { + stdout: JSON.stringify({ + name: 'verify', + status: 'completed', + conclusion: 'failure', + check_suite: { workflow_run: { id: 77 } } + }) + } + } + if (endpoint.endsWith('/check-runs/99/annotations?per_page=20')) { + return { stdout: '[]' } + } + if (endpoint.endsWith('/actions/runs/77/jobs?per_page=100')) { + return { + stdout: JSON.stringify({ + jobs: [ + { + id: 9901, + name: 'verify', + status: 'completed', + conclusion: 'failure', + steps: [] + } + ] + }) + } + } + if (endpoint.endsWith('/actions/jobs/9901/logs')) { + return { stdout: enterprise ? 'enterprise log' : 'github.com log' } + } + throw new Error(`unexpected gh call: ${args.join(' ')}`) + }) + + const githubDotCom = await getPRCheckDetails('/repo-root', { + checkRunId: 99, + prRepo: { owner: 'acme', repo: 'widgets', host: 'github.com' } + }) + const enterprise = await getPRCheckDetails('/repo-root', { + checkRunId: 99, + prRepo: { owner: 'acme', repo: 'widgets', host: 'github.acme-corp.com' } + }) + + expect(githubDotCom?.jobs[0]?.logTail).toBe('github.com log') + expect(enterprise?.jobs[0]?.logTail).toBe('enterprise log') + const logCalls = ghExecFileAsyncMock.mock.calls.filter(([args]) => + 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'])) + }) }) diff --git a/src/main/github/client-pr-checks.test.ts b/src/main/github/client-pr-checks.test.ts index 151bd94fef6..673da049943 100644 --- a/src/main/github/client-pr-checks.test.ts +++ b/src/main/github/client-pr-checks.test.ts @@ -591,7 +591,11 @@ describe('getPRChecks', () => { stdout: JSON.stringify([{ name: 'lint', state: 'PASS', link: 'https://example.com/lint' }]) }) - await getPRChecks('/repo-root', 42, undefined, { owner: 'acme', repo: 'widgets' }) + await getPRChecks('/repo-root', 42, undefined, { + owner: 'acme', + repo: 'widgets', + host: 'github.com' + }) expect(getOwnerRepoMock).not.toHaveBeenCalled() expectGraphQLRollupCall() diff --git a/src/main/github/client-pr-local-runtime.test.ts b/src/main/github/client-pr-local-runtime.test.ts index 9da948f43c4..a70d8bab9b8 100644 --- a/src/main/github/client-pr-local-runtime.test.ts +++ b/src/main/github/client-pr-local-runtime.test.ts @@ -10,6 +10,7 @@ const { getOwnerRepoMock, getIssueOwnerRepoMock, getOwnerRepoForRemoteMock, + getEnterpriseGitHubRepoSlugMock, resolvePRRepositoryCandidatesMock, getRemoteUrlForRepoMock, gitExecFileAsyncMock, @@ -24,6 +25,7 @@ const { getOwnerRepoMock: vi.fn(), getIssueOwnerRepoMock: vi.fn(), getOwnerRepoForRemoteMock: vi.fn(), + getEnterpriseGitHubRepoSlugMock: vi.fn(), resolvePRRepositoryCandidatesMock: vi.fn(), getRemoteUrlForRepoMock: vi.fn(), gitExecFileAsyncMock: vi.fn(), @@ -76,6 +78,10 @@ vi.mock('../git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) +vi.mock('./github-enterprise-repository', () => ({ + getEnterpriseGitHubRepoSlug: getEnterpriseGitHubRepoSlugMock +})) + vi.mock('../providers/ssh-git-dispatch', () => ({ getSshGitProvider: vi.fn() })) @@ -93,9 +99,13 @@ vi.mock('./rate-limit', () => ({ import { addPRReviewComment, addPRReviewCommentReply, + getPRCheckDetails, + getPRChecks, + getWorkItemByOwnerRepo, getPRComments, mergePR, removePRReviewers, + rerunPRChecks, requestPRReviewers, resolveReviewThread, setPRAutoMerge, @@ -111,6 +121,8 @@ describe('GitHub PR local runtime routing', () => { getOwnerRepoMock.mockReset() getIssueOwnerRepoMock.mockReset() getOwnerRepoForRemoteMock.mockReset() + getEnterpriseGitHubRepoSlugMock.mockReset() + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null) resolvePRRepositoryCandidatesMock.mockReset() getRemoteUrlForRepoMock.mockReset() gitExecFileAsyncMock.mockReset() @@ -127,12 +139,28 @@ describe('GitHub PR local runtime routing', () => { it('routes PR details and mutations through the selected WSL distro', async () => { const localGitOptions = { wslDistro: 'Ubuntu' } const prRepo = { owner: 'acme', repo: 'orca' } + rateLimitGuardMock.mockReturnValue({ + blocked: true, + remaining: 0, + limit: 5000, + resetAt: 1_800_000_000 + }) getOwnerRepoMock.mockResolvedValue(prRepo) ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { const endpoint = args.find((arg) => arg.startsWith('repos/acme/orca/')) ?? '' const query = args.find((arg) => arg.startsWith('query=')) ?? '' if (args[0] === 'pr' && args[1] === 'view') { + const jsonFields = args[args.indexOf('--json') + 1] + if (jsonFields === 'id,headRefOid,baseRefName') { + return { + stdout: JSON.stringify({ + id: 'PR_local', + headRefOid: 'head-oid', + baseRefName: 'main' + }) + } + } return { stdout: JSON.stringify({ id: 'PR_kwDO123', @@ -238,5 +266,250 @@ describe('GitHub PR local runtime routing', () => { expect(ghExecFileAsyncMock.mock.calls.every((call) => call[1]?.wslDistro === 'Ubuntu')).toBe( true ) + expect(getRateLimitMock).not.toHaveBeenCalled() + expect(rateLimitGuardMock).not.toHaveBeenCalled() + expect(noteRateLimitSpendMock).not.toHaveBeenCalled() + }) + + it('never falls through to the default gh host for an unresolved SSH repository', async () => { + const legacyRepo = { owner: 'team', repo: 'orca' } + getOwnerRepoMock.mockResolvedValue(null) + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null) + + await expect(getPRComments('/remote/repo', 7, { prRepo: legacyRepo }, 'ssh-1')).rejects.toThrow( + 'GitHub remote' + ) + await expect( + getPRChecks('/remote/repo', 7, undefined, legacyRepo, undefined, 'ssh-1') + ).rejects.toThrow('GitHub remote') + await expect(mergePR('/remote/repo', 7, 'squash', 'ssh-1', legacyRepo)).resolves.toMatchObject({ + ok: false + }) + await expect( + setPRAutoMerge('/remote/repo', 7, true, 'squash', 'ssh-1', legacyRepo) + ).resolves.toMatchObject({ ok: false }) + await expect(updatePRTitle('/remote/repo', 7, 'New title', 'ssh-1', legacyRepo)).resolves.toBe( + false + ) + await expect(requestPRReviewers('/remote/repo', 7, ['octo'], 'ssh-1')).resolves.toMatchObject({ + ok: false + }) + await expect(removePRReviewers('/remote/repo', 7, ['octo'], 'ssh-1')).resolves.toMatchObject({ + ok: false + }) + await expect( + addPRReviewCommentReply( + '/remote/repo', + 7, + 11, + 'Reply', + undefined, + undefined, + undefined, + 'ssh-1', + legacyRepo + ) + ).resolves.toMatchObject({ ok: false }) + + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('host-qualifies SSH-backed GitHub Enterprise review reads and mutations', async () => { + const enterpriseRepo = { + owner: 'team', + repo: 'orca', + host: 'github.acme-corp.com' + } + getOwnerRepoMock.mockResolvedValue(null) + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(enterpriseRepo) + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + const endpoint = args.find((arg) => arg.startsWith('repos/team/orca/')) ?? '' + const query = args.find((arg) => arg.startsWith('query=')) ?? '' + if (args[0] === 'pr' && args[1] === 'checks') { + return { stdout: '[]' } + } + if (args[0] === 'pr' && args[1] === 'view') { + if (args.includes('id,headRefOid,baseRefName')) { + return { + stdout: JSON.stringify({ + id: 'PR_enterprise', + headRefOid: 'head-sha', + baseRefName: 'main' + }) + } + } + return { + stdout: JSON.stringify({ + number: 7, + title: 'Enterprise PR', + state: 'OPEN', + url: 'https://github.acme-corp.com/team/orca/pull/7', + labels: [], + updatedAt: '2026-07-16T00:00:00Z', + author: { login: 'pr-author' }, + headRefName: 'feature', + baseRefName: 'main' + }) + } + } + if (query.includes('reviewThreads')) { + return { + stdout: JSON.stringify({ + data: { + repository: { + pullRequest: { + reviewThreads: { nodes: [] }, + comments: { nodes: [] } + } + } + } + }) + } + } + if ( + endpoint.endsWith('/issues/7/comments?per_page=100') || + endpoint.endsWith('/pulls/7/reviews?per_page=100') + ) { + return { stdout: '[]' } + } + if (endpoint.endsWith('/commits/head-sha/check-runs?per_page=100')) { + return { + stdout: JSON.stringify({ + check_runs: [ + { + id: 88, + name: 'lint', + status: 'completed', + conclusion: 'failure', + details_url: 'https://github.acme-corp.com/team/orca/actions/runs/77/job/88' + } + ] + }) + } + } + if (endpoint.endsWith('/commits/head-sha/status?per_page=100')) { + return { stdout: JSON.stringify({ statuses: [] }) } + } + if (endpoint.endsWith('/commits/head-sha/check-suites?per_page=100')) { + return { stdout: JSON.stringify({ check_suites: [] }) } + } + if (endpoint.endsWith('/check-runs/88')) { + return { + stdout: JSON.stringify({ + id: 88, + name: 'lint', + status: 'completed', + conclusion: 'failure', + details_url: 'https://github.acme-corp.com/team/orca/actions/runs/77/job/88', + output: { title: 'Lint failed', summary: 'One error' } + }) + } + } + if (endpoint.endsWith('/check-runs/88/annotations?per_page=20')) { + return { stdout: '[]' } + } + if (query) { + return { stdout: JSON.stringify({ data: { repository: {} } }) } + } + return { + stdout: JSON.stringify({ + id: 13, + user: null, + body: 'Enterprise inline comment' + }) + } + }) + + await expect( + getWorkItemByOwnerRepo('/remote/repo', enterpriseRepo, 7, 'pr', 'ssh-1') + ).resolves.toMatchObject({ number: 7, title: 'Enterprise PR' }) + await expect( + getPRComments('/remote/repo', 7, { prRepo: enterpriseRepo }, 'ssh-1') + ).resolves.toEqual([]) + await expect( + getPRChecks('/remote/repo', 7, undefined, enterpriseRepo, undefined, 'ssh-1') + ).resolves.toEqual([]) + await expect( + getPRCheckDetails('/remote/repo', { checkRunId: 88, prRepo: enterpriseRepo }, 'ssh-1') + ).resolves.toMatchObject({ name: 'lint', conclusion: 'failure' }) + await expect( + rerunPRChecks('/remote/repo', 7, { headSha: 'head-sha', failedOnly: true }, 'ssh-1') + ).resolves.toEqual({ ok: true, count: 1 }) + await expect(resolveReviewThread('/remote/repo', 'thread-1', true, 'ssh-1')).resolves.toBe(true) + await expect( + addPRReviewCommentReply( + '/remote/repo', + 7, + 11, + 'Enterprise reply', + 'thread-1', + 'src/enterprise.ts', + 10, + 'ssh-1', + enterpriseRepo + ) + ).resolves.toMatchObject({ ok: true }) + await expect( + addPRReviewComment({ + repoPath: '/remote/repo', + connectionId: 'ssh-1', + prNumber: 7, + body: 'Enterprise inline comment', + commitId: 'head-sha', + path: 'src/enterprise.ts', + line: 10 + }) + ).resolves.toMatchObject({ ok: true }) + await expect( + updatePRTitle('/remote/repo', 7, 'New title', 'ssh-1', enterpriseRepo) + ).resolves.toBe(true) + await expect( + updatePRDetails('/remote/repo', 7, { body: 'New body' }, 'ssh-1', enterpriseRepo) + ).resolves.toEqual({ ok: true }) + await expect(updatePRState('/remote/repo', 7, { state: 'closed' }, 'ssh-1')).resolves.toEqual({ + ok: true + }) + await expect(requestPRReviewers('/remote/repo', 7, ['octo'], 'ssh-1')).resolves.toEqual({ + ok: true + }) + await expect(removePRReviewers('/remote/repo', 7, ['octo'], 'ssh-1')).resolves.toEqual({ + ok: true + }) + await expect( + setPRAutoMerge('/remote/repo', 7, true, 'squash', 'ssh-1', enterpriseRepo) + ).resolves.toEqual({ ok: true }) + await expect( + setPRAutoMerge('/remote/repo', 7, false, 'squash', 'ssh-1', enterpriseRepo) + ).resolves.toEqual({ ok: true }) + await expect(mergePR('/remote/repo', 7, 'squash', 'ssh-1', enterpriseRepo)).resolves.toEqual({ + ok: true + }) + + 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({}) + 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' + ) + ).toBe(true) + const apiCalls = ghExecFileAsyncMock.mock.calls.filter(([args]) => args[0] === 'api') + expect(apiCalls.length).toBeGreaterThan(0) + expect( + apiCalls.every( + ([args, options]) => + args.includes('--hostname') && + args[args.indexOf('--hostname') + 1] === 'github.acme-corp.com' && + options.cwd === undefined && + options.wslDistro === undefined + ) + ).toBe(true) }) }) diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index 21fc7cc60b8..d78137fcb09 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -112,6 +112,7 @@ import { getRepoSlug, getRepoUpstream, getWorkItem, + getWorkItemByOwnerRepo, getPullRequestPushTarget, mergePR, resolveReviewThread, @@ -3815,7 +3816,12 @@ describe('GitHub GraphQL rate-limit guard', () => { }) .mockResolvedValueOnce({ stdout: '[]' }) - await getPRComments('/repo-root', 7, { prRepo: { owner: 'stablyai', repo: 'orca' } }, undefined) + await getPRComments( + '/repo-root', + 7, + { prRepo: { owner: 'stablyai', repo: 'orca', host: 'github.com' } }, + undefined + ) expect(getOwnerRepoMock).not.toHaveBeenCalled() expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( @@ -3850,10 +3856,18 @@ describe('GitHub GraphQL rate-limit guard', () => { .mockResolvedValue({ stdout: '', stderr: '' }) await expect( - mergePR('/repo-root', 7, 'squash', undefined, { owner: 'stablyai', repo: 'orca' }) + mergePR('/repo-root', 7, 'squash', undefined, { + owner: 'stablyai', + repo: 'orca', + host: 'github.com' + }) ).resolves.toEqual({ ok: true }) await expect( - updatePRTitle('/repo-root', 7, 'New title', undefined, { owner: 'stablyai', repo: 'orca' }) + updatePRTitle('/repo-root', 7, 'New title', undefined, { + owner: 'stablyai', + repo: 'orca', + host: 'github.com' + }) ).resolves.toBe(true) expect(getOwnerRepoMock).not.toHaveBeenCalled() @@ -3895,13 +3909,15 @@ describe('GitHub GraphQL rate-limit guard', () => { await expect( setPRAutoMerge('/remote/repo-root', 7, true, 'squash', 'ssh-1', { owner: 'stablyai', - repo: 'orca' + repo: 'orca', + host: 'github.com' }) ).resolves.toEqual({ ok: true }) await expect( setPRAutoMerge('/remote/repo-root', 7, false, 'squash', 'ssh-1', { owner: 'stablyai', - repo: 'orca' + repo: 'orca', + host: 'github.com' }) ).resolves.toEqual({ ok: true }) @@ -4039,7 +4055,11 @@ describe('GitHub GraphQL rate-limit guard', () => { }) await expect( - mergePR('/repo-root', 7, 'squash', undefined, { owner: 'stablyai', repo: 'orca' }) + mergePR('/repo-root', 7, 'squash', undefined, { + owner: 'stablyai', + repo: 'orca', + host: 'github.com' + }) ).resolves.toEqual({ ok: false, error: 'This pull request requires review approval before it can be merged.' @@ -4074,7 +4094,11 @@ describe('GitHub GraphQL rate-limit guard', () => { .mockResolvedValueOnce({ stdout: JSON.stringify(prView) }) await expect( - mergePR('/repo-root', 7, 'squash', undefined, { owner: 'stablyai', repo: 'orca' }) + mergePR('/repo-root', 7, 'squash', undefined, { + owner: 'stablyai', + repo: 'orca', + host: 'github.com' + }) ).resolves.toEqual({ ok: false, error: @@ -4163,6 +4187,58 @@ describe('GitHub GraphQL rate-limit guard', () => { expect(_getMergeQueueCacheSizeForTests()).toBe(256) }) + it('isolates merge metadata for the same slug on different GitHub hosts', async () => { + const prView = { + number: 7, + title: 'PR', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/7', + statusCheckRollup: [], + updatedAt: '2026-07-16T00:00:00Z', + isDraft: false, + mergeable: 'MERGEABLE', + baseRefName: 'main', + headRefOid: 'head-oid' + } + ghExecFileAsyncMock.mockImplementation(async (args) => { + if (args.includes('graphql')) { + const enterprise = args.includes('github.acme-corp.com') + return { + stdout: JSON.stringify({ + data: { + repository: { + mergeQueue: null, + autoMergeAllowed: !enterprise + } + } + }) + } + } + return { stdout: JSON.stringify(prView) } + }) + + const githubDotCom = await getWorkItemByOwnerRepo( + '/repo-root', + { owner: 'acme', repo: 'widgets', host: 'github.com' }, + 7, + 'pr' + ) + const enterprise = await getWorkItemByOwnerRepo( + '/repo-root', + { owner: 'acme', repo: 'widgets', host: 'github.acme-corp.com' }, + 7, + 'pr' + ) + + expect(githubDotCom?.autoMergeAllowed).toBe(true) + 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']) + ) + }) + it('returns conflicting file details instead of running gh merge when PR is dirty', async () => { ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: JSON.stringify({ @@ -4187,7 +4263,11 @@ describe('GitHub GraphQL rate-limit guard', () => { .mockResolvedValueOnce({ stdout: 'result-tree-oid\u0000src/conflict.ts\u0000' }) await expect( - mergePR('/repo-root', 7, 'squash', undefined, { owner: 'stablyai', repo: 'orca' }) + mergePR('/repo-root', 7, 'squash', undefined, { + owner: 'stablyai', + repo: 'orca', + host: 'github.com' + }) ).resolves.toEqual({ ok: false, error: @@ -4220,7 +4300,11 @@ describe('GitHub GraphQL rate-limit guard', () => { .mockResolvedValueOnce({ stdout: '', stderr: '' }) await expect( - mergePR('/remote/repo-root', 7, 'squash', 'ssh-1', { owner: 'stablyai', repo: 'orca' }) + mergePR('/remote/repo-root', 7, 'squash', 'ssh-1', { + owner: 'stablyai', + repo: 'orca', + host: 'github.com' + }) ).resolves.toEqual({ ok: true }) expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index d67b3e4405b..cb96f8bf408 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -86,6 +86,15 @@ import { } from './gh-cwd-repo-negative-cache' import type { GitHubRepoContext } from './github-repository-identity' import { getEnterpriseGitHubRepoSlug } from './github-enterprise-repository' +import { + getOriginGitHubApiRepository, + githubApiHostArgs, + githubCliRepositoryArgument, + githubRepositoryWebHost, + isGitHubDotComRepository, + resolveGitHubApiRepository, + type GitHubApiRepository +} from './github-api-repository' export { _resetOwnerRepoCache } from './gh-utils' export { getIssue, @@ -175,7 +184,16 @@ function pruneRepositoryMergeMetadataCache(now = Date.now()): void { } } -async function assertRateLimitBudget(bucket: RateLimitBucketKind): Promise { +async function assertRateLimitBudget( + bucket: RateLimitBucketKind, + 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) { + return + } await getRateLimit() const guard = rateLimitGuard(bucket) if (guard.blocked) { @@ -185,6 +203,27 @@ async function assertRateLimitBudget(bucket: RateLimitBucketKind): Promise } } +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 { @@ -723,7 +762,15 @@ function mapPullRequestWorkItem( ? String((item.base as { ref?: unknown }).ref ?? '') : String(item.baseRefName ?? ''), ...(headSha ? { headSha } : {}), - ...(baseOwnerRepo ? { prRepo: { owner: baseOwnerRepo.owner, repo: baseOwnerRepo.repo } } : {}), + ...(baseOwnerRepo + ? { + prRepo: { + owner: baseOwnerRepo.owner, + repo: baseOwnerRepo.repo, + ...(baseOwnerRepo.host ? { host: baseOwnerRepo.host } : {}) + } + } + : {}), ...(additions !== undefined ? { additions } : {}), ...(deletions !== undefined ? { deletions } : {}), ...(changedFiles !== undefined ? { changedFiles } : {}), @@ -787,7 +834,7 @@ async function hydrateWorkItemRepositoryMergeMetadata( async function fetchIssueWorkItem( repoPath: string, - ownerRepo: OwnerRepo | null, + ownerRepo: GitHubApiRepository | null, number: number, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} @@ -795,7 +842,11 @@ async function fetchIssueWorkItem( const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) if (ownerRepo) { const { stdout } = await ghExecFileAsync( - ['api', `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues/${number}`], + [ + 'api', + ...githubApiHostArgs(ownerRepo), + `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues/${number}` + ], ghOptions ) const item = JSON.parse(stdout) as Record @@ -817,7 +868,7 @@ const WORK_ITEM_PR_REVIEW_JSON_FIELDS = 'reviewRequests,latestReviews' async function fetchPullRequestReviewFields( number: number, - ownerRepo: OwnerRepo | null, + ownerRepo: GitHubApiRepository | null, ghOptions: GhExecOptions ): Promise> { try { @@ -827,7 +878,7 @@ async function fetchPullRequestReviewFields( 'view', String(number), '--repo', - `${ownerRepo.owner}/${ownerRepo.repo}`, + githubCliRepositoryArgument(ownerRepo), '--json', WORK_ITEM_PR_REVIEW_JSON_FIELDS ] @@ -849,7 +900,7 @@ async function fetchPullRequestReviewFields( async function fetchPullRequestWorkItem( repoPath: string, - ownerRepo: OwnerRepo | null, + ownerRepo: GitHubApiRepository | null, number: number, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} @@ -863,7 +914,7 @@ async function fetchPullRequestWorkItem( 'view', String(number), '--repo', - `${ownerRepo.owner}/${ownerRepo.repo}`, + githubCliRepositoryArgument(ownerRepo), '--json', WORK_ITEM_PR_DETAIL_JSON_FIELDS ], @@ -890,7 +941,11 @@ async function fetchPullRequestWorkItem( } } catch { const { stdout } = await ghExecFileAsync( - ['api', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${number}`], + [ + 'api', + ...githubApiHostArgs(ownerRepo), + `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${number}` + ], ghOptions ) const mapped = mapPullRequestWorkItem( @@ -1681,13 +1736,6 @@ function parseCreatePRPayload(stdout: string): { number: number; url: string } | return { number: Number(urlMatch[1]), url: urlMatch[0] } } -// Why: bare OWNER/REPO resolves against gh's default host, so qualify with host for GHES; the only host signal for cwd-less SSH repos (#8312). -function ghRepoArg(slug: { owner: string; repo: string; host?: string }): string { - return slug.host && slug.host.toLowerCase() !== 'github.com' - ? `${slug.host}/${slug.owner}/${slug.repo}` - : `${slug.owner}/${slug.repo}` -} - async function findOpenPRByHeadBase(args: { repoPath: string repoArg: string @@ -1792,7 +1840,7 @@ export async function createGitHubPullRequest( } } // Host-qualified for GHES so gh targets the Enterprise server, not github.com. - const repoArg = ghRepoArg(ownerRepo) + const repoArg = githubCliRepositoryArgument(ownerRepo) const base = normalizeHostedReviewBaseRef(input.base) const head = input.head ? normalizeHostedReviewHeadRef(input.head) || undefined : undefined @@ -1963,7 +2011,7 @@ export async function getWorkItem( export async function getWorkItemByOwnerRepo( repoPath: string, - ownerRepo: OwnerRepo, + ownerRepo: GitHubApiRepository, number: number, type: 'issue' | 'pr', connectionId?: string | null, @@ -2142,19 +2190,18 @@ function cacheRepositoryMergeMetadata( } async function detectRepositoryMergeMetadata( - ownerRepo: OwnerRepo, + ownerRepo: GitHubApiRepository, branchName: string | undefined, ghOptions: GhExecOptions ): Promise { - const cacheKey = `${ownerRepo.owner.toLowerCase()}/${ownerRepo.repo.toLowerCase()}:${ - branchName ?? '__repo__' - }` + const host = ownerRepo.host?.toLowerCase() ?? 'github.com' + const cacheKey = `${host}/${ownerRepo.owner.toLowerCase()}/${ownerRepo.repo.toLowerCase()}:${branchName ?? '__repo__'}` pruneRepositoryMergeMetadataCache() const cached = repositoryMergeMetadataCache.get(cacheKey) if (cached) { return cached.value } - const guard = rateLimitGuard('graphql') + const guard = repositoryRateLimitGuard(ownerRepo, 'graphql', ghOptions) if (guard.blocked) { return { mergeQueueRequired: null, autoMergeAllowed: null } } @@ -2179,9 +2226,10 @@ async function detectRepositoryMergeMetadata( } }` try { - noteRateLimitSpend('graphql') + noteRepositoryRateLimitSpend(ownerRepo, 'graphql', 1, ghOptions) const args = [ 'api', + ...githubApiHostArgs(ownerRepo), 'graphql', '-f', `query=${query}`, @@ -2271,14 +2319,18 @@ async function hydrateBranchLookupWithExactPR( } async function getRestPRForBranch( - prRepo: OwnerRepo, + prRepo: GitHubApiRepository, headOwner: string, branchName: string, ghOptions: ReturnType ): Promise { const head = encodeURIComponent(`${headOwner}:${branchName}`) const { stdout } = await ghExecFileAsync( - ['api', `repos/${prRepo.owner}/${prRepo.repo}/pulls?head=${head}&state=all&per_page=1`], + [ + 'api', + ...githubApiHostArgs(prRepo), + `repos/${prRepo.owner}/${prRepo.repo}/pulls?head=${head}&state=all&per_page=1` + ], ghOptions ) const list = JSON.parse(stdout) as RestPullRequest[] @@ -2287,7 +2339,7 @@ async function getRestPRForBranch( } async function getFallbackPRListForBranch( - prRepo: OwnerRepo, + prRepo: GitHubApiRepository, branchName: string, ghOptions: ReturnType ): Promise { @@ -2296,7 +2348,7 @@ async function getFallbackPRListForBranch( 'pr', 'list', '--repo', - `${prRepo.owner}/${prRepo.repo}`, + githubCliRepositoryArgument(prRepo), '--head', branchName, '--state', @@ -2695,19 +2747,23 @@ async function lookupPRByBranchName(args: { } async function getRestPRByNumber( - ownerRepo: OwnerRepo, + ownerRepo: GitHubApiRepository, number: number, ghOptions: ReturnType ): Promise { const { stdout } = await ghExecFileAsync( - ['api', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${number}`], + [ + 'api', + ...githubApiHostArgs(ownerRepo), + `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${number}` + ], ghOptions ) return mapRestPullRequest(JSON.parse(stdout) as RestPullRequest) } async function getPRByNumber( - ownerRepo: OwnerRepo, + ownerRepo: GitHubApiRepository, number: number, ghOptions: ReturnType ): Promise { @@ -2718,7 +2774,7 @@ async function getPRByNumber( 'view', String(number), '--repo', - `${ownerRepo.owner}/${ownerRepo.repo}`, + githubCliRepositoryArgument(ownerRepo), '--json', PR_LOOKUP_JSON_FIELDS ], @@ -3321,7 +3377,7 @@ function mapRestCommitStatus(status: RestCommitStatus): PRCheckDetail | null { } function mapGraphQLPendingApprovalCheckSuite( - ownerRepo: OwnerRepo, + ownerRepo: GitHubApiRepository, suite: GraphQLCheckSuite, headSha: string | null | undefined, index: number @@ -3338,7 +3394,7 @@ function mapGraphQLPendingApprovalCheckSuite( } function mapGraphQLPRChecksResponse( - ownerRepo: OwnerRepo, + ownerRepo: GitHubApiRepository, response: GraphQLPRChecksResponse ): PRCheckDetail[] | null { const pullRequest = response.data?.repository?.pullRequest @@ -3380,7 +3436,7 @@ function mapGraphQLPRChecksResponse( } async function getPRChecksViaRestFallback( - ownerRepo: OwnerRepo, + ownerRepo: GitHubApiRepository, headSha: string | undefined, ghOptions: GhExecOptions, noCache?: boolean @@ -3389,7 +3445,7 @@ async function getPRChecksViaRestFallback( return null } try { - await assertRateLimitBudget('core') + await assertRateLimitBudget('core', ownerRepo, ghOptions) } catch (err) { console.warn('getPRChecks skipped REST fallback, falling back to gh pr checks:', err) return null @@ -3402,12 +3458,13 @@ async function getPRChecksViaRestFallback( const { stdout } = await ghExecFileAsync( [ 'api', + ...githubApiHostArgs(ownerRepo), ...cacheArgs, `repos/${ownerRepo.owner}/${ownerRepo.repo}/commits/${encodedHeadSha}/check-runs?per_page=100` ], ghOptions ) - noteRateLimitSpend('core') + noteRepositoryRateLimitSpend(ownerRepo, 'core', 1, ghOptions) const checkRunData = JSON.parse(stdout) as { check_runs?: RestCheckRun[] } @@ -3419,12 +3476,13 @@ async function getPRChecksViaRestFallback( const statusResult = await ghExecFileAsync( [ 'api', + ...githubApiHostArgs(ownerRepo), ...cacheArgs, `repos/${ownerRepo.owner}/${ownerRepo.repo}/commits/${encodedHeadSha}/status?per_page=100` ], ghOptions ) - noteRateLimitSpend('core') + noteRepositoryRateLimitSpend(ownerRepo, 'core', 1, ghOptions) const statusData = JSON.parse(statusResult.stdout) as { statuses?: RestCommitStatus[] } @@ -3441,12 +3499,13 @@ async function getPRChecksViaRestFallback( const suitesResult = await ghExecFileAsync( [ 'api', + ...githubApiHostArgs(ownerRepo), ...cacheArgs, `repos/${ownerRepo.owner}/${ownerRepo.repo}/commits/${encodedHeadSha}/check-suites?per_page=100` ], ghOptions ) - noteRateLimitSpend('core') + noteRepositoryRateLimitSpend(ownerRepo, 'core', 1, ghOptions) const suitesData = JSON.parse(suitesResult.stdout) as { check_suites?: RestCheckSuite[] } @@ -3481,21 +3540,29 @@ export async function getPRChecks( repoPath: string, prNumber: number, headSha?: string, - prRepo?: OwnerRepo | null, + prRepo?: GitHubApiRepository | null, options?: { noCache?: boolean }, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { void headSha const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = prRepo ?? (await getOwnerRepo(repoPath, connectionId, localGitOptions)) + const ownerRepo = await resolveGitHubApiRepository( + repoPath, + prRepo, + connectionId, + localGitOptions + ) + if (connectionId && !ownerRepo) { + throw new Error(GITHUB_WORK_ITEMS_SSH_REMOTE_REQUIRED_MESSAGE) + } const fallbackToPRChecks = async (): Promise => { - await assertRateLimitBudget('graphql') + await assertRateLimitBudget('graphql', ownerRepo, ghOptions) await acquire() try { const fallbackArgs = ['pr', 'checks', String(prNumber), '--json', 'name,state,link'] if (ownerRepo) { - fallbackArgs.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) + fallbackArgs.push('--repo', githubCliRepositoryArgument(ownerRepo)) } const { stdout } = await ghExecFileAsync(fallbackArgs, ghOptions).catch((err: unknown) => { const { stderr } = extractExecError(err) @@ -3505,7 +3572,7 @@ export async function getPRChecks( } throw err }) - noteRateLimitSpend('graphql') + noteRepositoryRateLimitSpend(ownerRepo, 'graphql', 1, ghOptions) const data = JSON.parse(stdout) as { name: string; state: string; link: string }[] return data.map((d) => ({ name: d.name, @@ -3522,7 +3589,7 @@ export async function getPRChecks( if (ownerRepo) { let canUseGraphQLRollup = true try { - await assertRateLimitBudget('graphql') + await assertRateLimitBudget('graphql', ownerRepo, ghOptions) } catch (err) { canUseGraphQLRollup = false console.warn('getPRChecks skipped GraphQL rollup, falling back to gh pr checks:', err) @@ -3535,6 +3602,7 @@ export async function getPRChecks( const { stdout } = await ghExecFileAsync( [ 'api', + ...githubApiHostArgs(ownerRepo), 'graphql', ...cacheArgs, '-f', @@ -3548,7 +3616,7 @@ export async function getPRChecks( ], ghOptions ) - noteRateLimitSpend('graphql') + noteRepositoryRateLimitSpend(ownerRepo, 'graphql', 1, ghOptions) const checks = mapGraphQLPRChecksResponse( ownerRepo, JSON.parse(stdout) as GraphQLPRChecksResponse @@ -3608,11 +3676,11 @@ function getPendingApprovalCheckSuiteName( } function getPendingApprovalCheckSuiteUrl( - ownerRepo: OwnerRepo, + ownerRepo: GitHubApiRepository, headSha: string, suiteId: number | null | undefined ): string { - const base = `https://github.com/${ownerRepo.owner}/${ownerRepo.repo}/commits/${headSha}/checks` + const base = `https://${githubRepositoryWebHost(ownerRepo)}/${ownerRepo.owner}/${ownerRepo.repo}/commits/${headSha}/checks` return typeof suiteId === 'number' && Number.isFinite(suiteId) ? `${base}#check-suite-${suiteId}` : base @@ -3695,7 +3763,7 @@ function getCheckJobLogTailCacheKey(job: PRCheckRunDetails['jobs'][number]): str async function attachFailedJobLogTails( jobs: PRCheckRunDetails['jobs'], - ownerRepo: OwnerRepo, + ownerRepo: GitHubApiRepository, ghOptions: GhExecOptions ): Promise { const failedJobs = jobs @@ -3707,7 +3775,10 @@ 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 cacheKey = getCheckJobLogTailCacheKey(job) + const jobCacheKey = getCheckJobLogTailCacheKey(job) + const cacheKey = jobCacheKey + ? `${githubRepositoryWebHost(ownerRepo).toLowerCase()}/${ownerRepo.owner.toLowerCase()}/${ownerRepo.repo.toLowerCase()}:${jobCacheKey}` + : null if (!cacheKey) { continue } @@ -3717,7 +3788,11 @@ async function attachFailedJobLogTails( } try { const { stdout } = await ghExecFileAsync( - ['api', `repos/${ownerRepo.owner}/${ownerRepo.repo}/actions/jobs/${job.id}/logs`], + [ + 'api', + ...githubApiHostArgs(ownerRepo), + `repos/${ownerRepo.owner}/${ownerRepo.repo}/actions/jobs/${job.id}/logs` + ], ghOptions ) job.logTail = sliceCheckLogTail(stdout) @@ -3751,13 +3826,18 @@ export async function getPRCheckDetails( workflowRunId?: number checkName?: string url?: string | null - prRepo?: OwnerRepo | null + prRepo?: GitHubApiRepository | null }, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = args.prRepo ?? (await getOwnerRepo(repoPath, connectionId, localGitOptions)) + const ownerRepo = await resolveGitHubApiRepository( + repoPath, + args.prRepo, + connectionId, + localGitOptions + ) if (!ownerRepo) { return null } @@ -3768,7 +3848,11 @@ export async function getPRCheckDetails( let annotations: PRCheckRunDetails['annotations'] = [] if (args.checkRunId) { const { stdout } = await ghExecFileAsync( - ['api', `repos/${ownerRepo.owner}/${ownerRepo.repo}/check-runs/${args.checkRunId}`], + [ + 'api', + ...githubApiHostArgs(ownerRepo), + `repos/${ownerRepo.owner}/${ownerRepo.repo}/check-runs/${args.checkRunId}` + ], ghOptions ) checkRun = JSON.parse(stdout) as Record @@ -3776,6 +3860,7 @@ 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 @@ -3793,6 +3878,7 @@ export async function getPRCheckDetails( const { stdout } = await ghExecFileAsync( [ 'api', + ...githubApiHostArgs(ownerRepo), `repos/${ownerRepo.owner}/${ownerRepo.repo}/actions/runs/${workflowRunId}/jobs?per_page=100` ], ghOptions @@ -3850,7 +3936,7 @@ export async function rerunPRChecks( localGitOptions: LocalGitExecOptions = {} ): Promise { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = await getOwnerRepo(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' } } @@ -3897,7 +3983,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', '-X', 'POST', endpoint], { + await ghExecFileAsync(['api', ...githubApiHostArgs(ownerRepo), '-X', 'POST', endpoint], { ...ghOptions, env: { ...process.env, GH_PROMPT_DISABLED: '1' } }) @@ -3907,6 +3993,7 @@ export async function rerunPRChecks( await ghExecFileAsync( [ 'api', + ...githubApiHostArgs(ownerRepo), '-X', 'POST', `repos/${ownerRepo.owner}/${ownerRepo.repo}/check-runs/${checkRunId}/rerequest` @@ -3983,14 +4070,22 @@ query($owner: String!, $repo: String!, $pr: Int!) { export async function getPRComments( repoPath: string, prNumber: number, - options?: { noCache?: boolean; prRepo?: OwnerRepo | null }, + options?: { noCache?: boolean; prRepo?: GitHubApiRepository | null }, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = options?.prRepo ?? (await getOwnerRepo(repoPath, connectionId, localGitOptions)) + const ownerRepo = await resolveGitHubApiRepository( + repoPath, + options?.prRepo, + connectionId, + localGitOptions + ) + if (connectionId && !ownerRepo) { + throw new Error(GITHUB_WORK_ITEMS_SSH_REMOTE_REQUIRED_MESSAGE) + } if (ownerRepo) { - await assertRateLimitBudget('core') + await assertRateLimitBudget('core', ownerRepo, ghOptions) } await acquire() try { @@ -4000,15 +4095,16 @@ export async function getPRComments( const base = `repos/${ownerRepo.owner}/${ownerRepo.repo}` // Why: allSettled so one failing endpoint doesn't blank out all comments; failed sources contribute zero. - const reviewThreadsGuard = rateLimitGuard('graphql') + const reviewThreadsGuard = repositoryRateLimitGuard(ownerRepo, 'graphql', ghOptions) let reviewThreadsFetch: Promise<{ stdout: string; stderr: string } | null> if (reviewThreadsGuard.blocked) { reviewThreadsFetch = Promise.resolve(null) } else { - noteRateLimitSpend('graphql') + noteRepositoryRateLimitSpend(ownerRepo, 'graphql', 1, ghOptions) reviewThreadsFetch = ghExecFileAsync( [ 'api', + ...githubApiHostArgs(ownerRepo), 'graphql', '-f', `query=${REVIEW_THREADS_QUERY}`, @@ -4024,17 +4120,27 @@ export async function getPRComments( } const [issueResult, threadsResult, reviewsResult] = await Promise.allSettled([ ghExecFileAsync( - ['api', ...cacheArgs, `${base}/issues/${prNumber}/comments?per_page=100`], + [ + 'api', + ...githubApiHostArgs(ownerRepo), + ...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', ...cacheArgs, `${base}/pulls/${prNumber}/reviews?per_page=100`], + [ + 'api', + ...githubApiHostArgs(ownerRepo), + ...cacheArgs, + `${base}/pulls/${prNumber}/reviews?per_page=100` + ], ghOptions ) ]) - noteRateLimitSpend('core', 2) + noteRepositoryRateLimitSpend(ownerRepo, 'core', 2, ghOptions) // Parse issue comments (REST) type RESTComment = { @@ -4223,6 +4329,14 @@ export async function setPRFileViewed(args: { const ghOptions = ghRepoExecOptions( githubRepoContext(args.repoPath, args.connectionId, args.localGitOptions) ) + const ownerRepo = await getOriginGitHubApiRepository( + args.repoPath, + args.connectionId, + args.localGitOptions + ) + if (args.connectionId && !ownerRepo) { + return false + } const mutation = args.viewed ? 'markFileAsViewed' : 'unmarkFileAsViewed' const query = `mutation($pullRequestId: ID!, $path: String!) { ${mutation}(input: { pullRequestId: $pullRequestId, path: $path }) { @@ -4234,6 +4348,7 @@ export async function setPRFileViewed(args: { await ghExecFileAsync( [ 'api', + ...(ownerRepo ? githubApiHostArgs(ownerRepo) : []), 'graphql', '-f', `query=${query}`, @@ -4266,7 +4381,11 @@ export async function resolveReviewThread( const mutation = resolve ? 'resolveReviewThread' : 'unresolveReviewThread' const query = `mutation($threadId: ID!) { ${mutation}(input: { threadId: $threadId }) { thread { isResolved } } }` const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const guard = rateLimitGuard('graphql') + const ownerRepo = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) + if (connectionId && !ownerRepo) { + return false + } + const guard = repositoryRateLimitGuard(ownerRepo, 'graphql', ghOptions) if (guard.blocked) { console.warn( `${mutation} skipped: GitHub GraphQL rate limit nearly exhausted (${guard.remaining}/${guard.limit})` @@ -4275,9 +4394,17 @@ export async function resolveReviewThread( } await acquire() try { - noteRateLimitSpend('graphql') + noteRepositoryRateLimitSpend(ownerRepo, 'graphql', 1, ghOptions) await ghExecFileAsync( - ['api', 'graphql', '-f', `query=${query}`, '-f', `threadId=${threadId}`], + [ + 'api', + ...(ownerRepo ? githubApiHostArgs(ownerRepo) : []), + 'graphql', + '-f', + `query=${query}`, + '-f', + `threadId=${threadId}` + ], ghOptions ) return true @@ -4329,11 +4456,16 @@ export async function addPRReviewCommentReply( path?: string, line?: number, connectionId?: string | null, - prRepo?: OwnerRepo | null, + prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = prRepo ?? (await getOwnerRepo(repoPath, connectionId, localGitOptions)) + const ownerRepo = await resolveGitHubApiRepository( + repoPath, + prRepo, + connectionId, + localGitOptions + ) if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } @@ -4342,6 +4474,7 @@ export async function addPRReviewCommentReply( const { stdout } = await ghExecFileAsync( [ 'api', + ...githubApiHostArgs(ownerRepo), '-X', 'POST', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${prNumber}/comments/${commentId}/replies`, @@ -4375,7 +4508,11 @@ export async function addPRReviewComment( const ghOptions = ghRepoExecOptions( githubRepoContext(args.repoPath, args.connectionId, args.localGitOptions) ) - const ownerRepo = await getOwnerRepo(args.repoPath, args.connectionId, args.localGitOptions) + const ownerRepo = await getOriginGitHubApiRepository( + args.repoPath, + args.connectionId, + args.localGitOptions + ) if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } @@ -4383,6 +4520,7 @@ export async function addPRReviewComment( try { const fields = [ 'api', + ...githubApiHostArgs(ownerRepo), '-X', 'POST', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${args.prNumber}/comments`, @@ -4433,11 +4571,19 @@ export async function mergePR( prNumber: number, method: 'merge' | 'squash' | 'rebase' = 'squash', connectionId?: string | null, - prRepo?: OwnerRepo | null, + prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ ok: true } | { ok: false; error: string }> { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = prRepo ?? (await getOwnerRepo(repoPath, connectionId, localGitOptions)) + const ownerRepo = await resolveGitHubApiRepository( + repoPath, + prRepo, + connectionId, + localGitOptions + ) + if (connectionId && !ownerRepo) { + return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } + } await acquire() try { const mergeBlocker = await getPRMergeBlocker( @@ -4455,7 +4601,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', `${ownerRepo.owner}/${ownerRepo.repo}`) + args.push('--repo', githubCliRepositoryArgument(ownerRepo)) } await ghExecFileAsync(args, { ...ghOptions, @@ -4477,11 +4623,19 @@ export async function setPRAutoMerge( enabled: boolean, method: GitHubPRMergeMethod = 'squash', connectionId?: string | null, - prRepo?: OwnerRepo | null, + prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ ok: true } | { ok: false; error: string }> { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = prRepo ?? (await getOwnerRepo(repoPath, connectionId, localGitOptions)) + const ownerRepo = await resolveGitHubApiRepository( + repoPath, + prRepo, + connectionId, + localGitOptions + ) + if (connectionId && !ownerRepo) { + return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } + } await acquire() try { if (enabled) { @@ -4489,7 +4643,7 @@ export async function setPRAutoMerge( } const args = ['pr', 'merge', String(prNumber), '--disable-auto'] if (ownerRepo) { - args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) + args.push('--repo', githubCliRepositoryArgument(ownerRepo)) } await ghExecFileAsync(args, { ...ghOptions, @@ -4521,12 +4675,12 @@ type PRAutoMergeIdentity = { async function getPRAutoMergeIdentity( prNumber: number, - ownerRepo: OwnerRepo | null, + ownerRepo: GitHubApiRepository | null, ghOptions: GhExecOptions ): Promise { const args = ['pr', 'view', String(prNumber), '--json', PR_AUTO_MERGE_IDENTITY_JSON_FIELDS] if (ownerRepo) { - args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) + args.push('--repo', githubCliRepositoryArgument(ownerRepo)) } const { stdout } = await ghExecFileAsync(args, ghOptions) const data = JSON.parse(stdout) as PRAutoMergeIdentity @@ -4540,12 +4694,12 @@ async function getPRAutoMergeIdentity( async function runPRAutoMergeCommand( prNumber: number, method: GitHubPRMergeMethod, - ownerRepo: OwnerRepo | null, + ownerRepo: GitHubApiRepository | null, ghOptions: GhExecOptions ): Promise { const args = ['pr', 'merge', String(prNumber), '--auto', `--${method}`] if (ownerRepo) { - args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) + args.push('--repo', githubCliRepositoryArgument(ownerRepo)) } await ghExecFileAsync(args, { ...ghOptions, @@ -4555,7 +4709,7 @@ async function runPRAutoMergeCommand( async function shouldUseMergeQueueAutoMerge( pr: PRAutoMergeIdentity, - ownerRepo: OwnerRepo | null, + ownerRepo: GitHubApiRepository | null, ghOptions: GhExecOptions ): Promise { if (!ownerRepo || !pr.baseRefName) { @@ -4568,7 +4722,7 @@ async function shouldUseMergeQueueAutoMerge( async function enablePRAutoMerge( prNumber: number, method: GitHubPRMergeMethod, - ownerRepo: OwnerRepo | null, + ownerRepo: GitHubApiRepository | null, ghOptions: GhExecOptions ): Promise<{ ok: true } | { ok: false; error: string }> { const pr = await getPRAutoMergeIdentity(prNumber, ownerRepo, ghOptions) @@ -4590,6 +4744,7 @@ async function enablePRAutoMerge( }` const args = [ 'api', + ...(ownerRepo ? githubApiHostArgs(ownerRepo) : []), 'graphql', '-f', `query=${query}`, @@ -4612,7 +4767,7 @@ async function enablePRAutoMerge( async function getPRMergeBlocker( repoPath: string, prNumber: number, - ownerRepo: OwnerRepo | null, + ownerRepo: GitHubApiRepository | null, ghOptions: GhExecOptions, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} @@ -4683,7 +4838,7 @@ export async function updatePRState( ): Promise<{ ok: true } | { ok: false; error: string }> { const context = githubRepoContext(repoPath, connectionId, localGitOptions) const ghOptions = ghRepoExecOptions(context) - const ownerRepo = await getOwnerRepo(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' } } @@ -4693,7 +4848,7 @@ export async function updatePRState( 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', `${ownerRepo.owner}/${ownerRepo.repo}`], + ['pr', cmd, String(prNumber), '--repo', githubCliRepositoryArgument(ownerRepo)], { ...ghOptions } @@ -4720,12 +4875,15 @@ export async function requestPRReviewers( return { ok: false, error: 'Enter at least one reviewer' } } const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = await getOwnerRepo(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' } + } await acquire() try { const args = ['pr', 'edit', String(prNumber), '--add-reviewer', logins.join(',')] if (ownerRepo) { - args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) + args.push('--repo', githubCliRepositoryArgument(ownerRepo)) } await ghExecFileAsync(args, { ...ghOptions, @@ -4753,12 +4911,15 @@ export async function removePRReviewers( return { ok: false, error: 'Enter at least one reviewer' } } const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = await getOwnerRepo(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' } + } await acquire() try { const args = ['pr', 'edit', String(prNumber), '--remove-reviewer', logins.join(',')] if (ownerRepo) { - args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) + args.push('--repo', githubCliRepositoryArgument(ownerRepo)) } await ghExecFileAsync(args, { ...ghOptions, @@ -4782,16 +4943,24 @@ export async function updatePRTitle( prNumber: number, title: string, connectionId?: string | null, - prRepo?: OwnerRepo | null, + prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = prRepo ?? (await getOwnerRepo(repoPath, connectionId, localGitOptions)) + const ownerRepo = await resolveGitHubApiRepository( + repoPath, + prRepo, + connectionId, + localGitOptions + ) + if (connectionId && !ownerRepo) { + return false + } await acquire() try { const args = ['pr', 'edit', String(prNumber), '--title', title] if (ownerRepo) { - args.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) + args.push('--repo', githubCliRepositoryArgument(ownerRepo)) } await ghExecFileAsync(args, { ...ghOptions @@ -4810,11 +4979,16 @@ export async function updatePRDetails( prNumber: number, updates: { title?: string; body?: string }, connectionId?: string | null, - prRepo?: OwnerRepo | null, + prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ ok: true } | { ok: false; error: string }> { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = prRepo ?? (await getOwnerRepo(repoPath, connectionId, localGitOptions)) + const ownerRepo = await resolveGitHubApiRepository( + repoPath, + prRepo, + connectionId, + localGitOptions + ) if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } @@ -4839,6 +5013,7 @@ 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 new file mode 100644 index 00000000000..2998805fd35 --- /dev/null +++ b/src/main/github/github-api-repository.ts @@ -0,0 +1,67 @@ +import type { GitHubOwnerRepo } from '../../shared/types' +import { getOwnerRepo, type LocalGitExecOptions } from './gh-utils' +import { getEnterpriseGitHubRepoSlug } from './github-enterprise-repository' + +export type GitHubApiRepository = GitHubOwnerRepo + +export async function getOriginGitHubApiRepository( + repoPath: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): Promise { + const ownerRepo = await getOwnerRepo(repoPath, connectionId, localGitOptions) + if (ownerRepo) { + return { ...ownerRepo, host: 'github.com' } + } + const enterpriseOptions = + Object.keys(localGitOptions).length > 0 ? { localGitExecOptions: localGitOptions } : {} + return getEnterpriseGitHubRepoSlug(repoPath, connectionId, enterpriseOptions) +} + +export async function resolveGitHubApiRepository( + repoPath: string, + repository?: GitHubApiRepository | null, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): Promise { + if (repository?.host) { + return repository + } + const originRepository = await getOriginGitHubApiRepository( + repoPath, + connectionId, + localGitOptions + ) + if (!repository) { + return originRepository + } + // Why: older clients only send owner/repo. The origin still supplies the + // execution host for fork-base slugs on the same GitHub Enterprise server. + if (originRepository?.host) { + return { ...repository, host: originRepository.host } + } + // Why: connection-backed gh has no cwd to infer a host from. An unhosted + // legacy identity is unsafe there because gh would target its default account. + 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}` +} + +export function githubRepositoryWebHost(repository: GitHubApiRepository): string { + return repository.host ?? 'github.com' +} diff --git a/src/main/github/github-enterprise-repository.test.ts b/src/main/github/github-enterprise-repository.test.ts index 73de02ddae9..84e29abc678 100644 --- a/src/main/github/github-enterprise-repository.test.ts +++ b/src/main/github/github-enterprise-repository.test.ts @@ -149,6 +149,23 @@ describe('isGitHubHostAuthenticated', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) }) + it('coalesces concurrent probes for the same runtime and host', async () => { + let finishProbe: (() => void) | undefined + ghExecFileAsyncMock.mockImplementation( + () => + new Promise((resolve) => { + finishProbe = () => resolve({ stdout: '', stderr: '' }) + }) + ) + + const first = isGitHubHostAuthenticated('github.acme-corp.com', '/repo') + const second = isGitHubHostAuthenticated('github.acme-corp.com', '/repo') + + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + finishProbe?.() + await expect(Promise.all([first, second])).resolves.toEqual([true, true]) + }) + it('does not share cache state across WSL distros', async () => { mockHostAuthenticated() diff --git a/src/main/github/github-enterprise-repository.ts b/src/main/github/github-enterprise-repository.ts index 94cffe725fa..c08075654ca 100644 --- a/src/main/github/github-enterprise-repository.ts +++ b/src/main/github/github-enterprise-repository.ts @@ -27,6 +27,7 @@ type HostAuthCacheEntry = { } const hostAuthCache = new Map() +const hostAuthInFlight = new Map>() // Why: gh's authenticated hosts live in per-runtime config — a WSL distro and an // SSH host each carry their own `hosts.yml` — so cache state must be keyed by the @@ -39,6 +40,7 @@ function runtimeCacheKey(connectionId?: string | null, wslDistro?: string): stri /** @internal - exposed for tests only */ export function _resetGitHubHostAuthCache(): void { hostAuthCache.clear() + hostAuthInFlight.clear() } // Only gh's own stdout/stderr — not the Error.message — counts as an @@ -74,26 +76,44 @@ export async function isGitHubHostAuthenticated( if (cached && cached.expiresAt > now) { return cached.authenticated } - const execOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - let authenticated: boolean + const inFlight = hostAuthInFlight.get(cacheKey) + if (inFlight) { + return inFlight + } + // 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) + ) + let authenticated: boolean + try { + await ghExecFileAsync(['auth', 'status', '--hostname', normalizedHost], execOptions) + authenticated = true + } catch (error) { + const output = ghCommandOutput(error) + if (!output) { + // Indeterminate (gh missing / spawn failure) — do not cache so a later + // probe (gh installed, tunnel ready, token added) can recover. + return false + } + // gh exits non-zero when a host has a token problem but still prints the + // per-host status; treat the host as GitHub only when it is actually listed. + authenticated = parseAuthStatus(output).some( + (account) => account.host.toLowerCase() === normalizedHost + ) + } + hostAuthCache.set(cacheKey, { authenticated, expiresAt: Date.now() + HOST_AUTH_TTL_MS }) + return authenticated + })() + hostAuthInFlight.set(cacheKey, probe) try { - await ghExecFileAsync(['auth', 'status', '--hostname', normalizedHost], execOptions) - authenticated = true - } catch (error) { - const output = ghCommandOutput(error) - if (!output) { - // Indeterminate (gh missing / spawn failure) — do not cache so a later - // probe (gh installed, tunnel ready, token added) can recover. - return false + return await probe + } finally { + if (hostAuthInFlight.get(cacheKey) === probe) { + hostAuthInFlight.delete(cacheKey) } - // gh exits non-zero when a host has a token problem but still prints the - // per-host status; treat the host as GitHub only when it is actually listed. - authenticated = parseAuthStatus(output).some( - (account) => account.host.toLowerCase() === normalizedHost - ) } - hostAuthCache.set(cacheKey, { authenticated, expiresAt: now + HOST_AUTH_TTL_MS }) - return authenticated } /** diff --git a/src/main/github/issues.test.ts b/src/main/github/issues.test.ts index 0f65fc1572c..926b5835f27 100644 --- a/src/main/github/issues.test.ts +++ b/src/main/github/issues.test.ts @@ -138,6 +138,40 @@ describe('issue source operations', () => { ) }) + it('routes PR conversation comments to the supplied Enterprise host', async () => { + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ + id: 9, + user: { login: 'octo', avatar_url: '', type: 'User' }, + body: 'Enterprise comment', + created_at: '2026-07-16T00:00:00.000Z', + html_url: 'https://github.acme-corp.com/team/orca/pull/7#issuecomment-9' + }) + }) + + await expect( + addIssueComment('/remote/repo', 7, 'Enterprise comment', 'ssh-1', { + owner: 'team', + repo: 'orca', + host: 'github.acme-corp.com' + }) + ).resolves.toMatchObject({ ok: true }) + + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + [ + 'api', + '--hostname', + 'github.acme-corp.com', + '-X', + 'POST', + 'repos/team/orca/issues/7/comments', + '--raw-field', + 'body=Enterprise comment' + ], + {} + ) + }) + it('lists issues from the issue owner/repo', async () => { getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }) diff --git a/src/main/github/issues.ts b/src/main/github/issues.ts index b0f3fa6cb89..424cf7cee6b 100644 --- a/src/main/github/issues.ts +++ b/src/main/github/issues.ts @@ -15,6 +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' // prettier-ignore import { ghExecFileAsync, acquire, release, getIssueOwnerRepo, resolveIssueSource, classifyGhError, classifyListIssuesError, ghRepoExecOptions, githubRepoContext, extractExecError } from './gh-utils' @@ -411,8 +412,9 @@ export async function addIssueComment( ): Promise { const context = githubRepoContext(repoPath, connectionId, localGitOptions) const ghOptions = ghRepoExecOptions(context) - const ownerRepo = - ownerRepoOverride ?? (await getIssueOwnerRepo(repoPath, connectionId, localGitOptions)) + 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' } } @@ -421,6 +423,7 @@ export async function addIssueComment( const { stdout } = await ghExecFileAsync( [ 'api', + ...githubApiHostArgs(ownerRepo), '-X', 'POST', `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues/${issueNumber}/comments`, diff --git a/src/main/github/rate-limit.test.ts b/src/main/github/rate-limit.test.ts index ab1373a70f0..ec952841f50 100644 --- a/src/main/github/rate-limit.test.ts +++ b/src/main/github/rate-limit.test.ts @@ -49,12 +49,15 @@ describe('getRateLimit', () => { fetchedAt: 1_000 } }) + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + ['api', '--hostname', 'github.com', 'rate_limit'], + { encoding: 'utf-8' } + ) }) it('caches a failed probe for the TTL instead of re-spawning gh', async () => { - // Why: regression for #7553 — GHES with rate limiting disabled fails every - // probe. Refreshes fail open past the failure, so without a negative cache - // each queued refresh would spawn its own doomed gh subprocess. + // Why: refreshes fail open past a probe failure, so without a negative cache + // an auth or transport failure would spawn the same doomed gh call repeatedly. ghExecFileAsyncMock.mockRejectedValue(new Error('HTTP 404: Rate limiting is not enabled.')) const first = await getRateLimit() diff --git a/src/main/github/rate-limit.ts b/src/main/github/rate-limit.ts index 1b77198db68..8ba6cf9c21a 100644 --- a/src/main/github/rate-limit.ts +++ b/src/main/github/rate-limit.ts @@ -176,7 +176,11 @@ export async function getRateLimit(options?: { force?: boolean }): Promise { await acquire() try { - const { stdout } = await ghExecFileAsync(['api', 'rate_limit'], { encoding: 'utf-8' }) + // 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 parsed = JSON.parse(stdout) as GhRateLimitPayload const snapshot: GitHubRateLimitSnapshot = { core: parseBucket(parsed.resources?.core), 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 a1ec6824aaa..1b7bd44a2b0 100644 --- a/src/main/github/work-item-details-file-viewed.test.ts +++ b/src/main/github/work-item-details-file-viewed.test.ts @@ -43,6 +43,7 @@ vi.mock('./gh-utils', () => ({ vi.mock('./client', () => ({ getWorkItem: getWorkItemMock, + getWorkItemByOwnerRepo: vi.fn(), getPRChecks: getPRChecksMock, getPRComments: getPRCommentsMock })) @@ -162,7 +163,7 @@ describe('getWorkItemDetails PR file viewed state', () => { '/repo-root', 42, 'head-sha', - null, + { owner: 'stablyai', repo: 'orca', host: 'github.com' }, undefined, 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 new file mode 100644 index 00000000000..1a2754a9d1f --- /dev/null +++ b/src/main/github/work-item-details-pr-files.test.ts @@ -0,0 +1,183 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + ghExecFileAsyncMock, + getOwnerRepoMock, + getWorkItemMock, + getPRChecksMock, + getPRCommentsMock +} = vi.hoisted(() => ({ + ghExecFileAsyncMock: vi.fn(), + getOwnerRepoMock: vi.fn(), + getWorkItemMock: vi.fn(), + getPRChecksMock: vi.fn(), + getPRCommentsMock: vi.fn() +})) + +vi.mock('./gh-utils', () => ({ + ghExecFileAsync: ghExecFileAsyncMock, + getOwnerRepo: getOwnerRepoMock, + getIssueOwnerRepo: vi.fn(), + ghRepoExecOptions: vi.fn((context) => ({ cwd: context.repoPath })), + githubRepoContext: vi.fn((repoPath, connectionId, localGitOptions) => ({ + repoPath, + connectionId: connectionId ?? null, + ...localGitOptions + })), + acquire: vi.fn(), + release: vi.fn() +})) + +vi.mock('./client', () => ({ + getWorkItem: getWorkItemMock, + getWorkItemByOwnerRepo: vi.fn(), + getPRChecks: getPRChecksMock, + getPRComments: getPRCommentsMock +})) + +vi.mock('./github-enterprise-repository', () => ({ + getEnterpriseGitHubRepoSlug: vi.fn().mockResolvedValue(null) +})) + +vi.mock('./rate-limit', () => ({ + rateLimitGuard: vi.fn(() => ({ blocked: false })), + noteRateLimitSpend: vi.fn() +})) + +import { getWorkItemDetails } from './work-item-details' + +function pullRequestItem(number: number, title: string): Record { + return { + id: `pr:${number}`, + type: 'pr', + number, + title, + state: 'open', + url: `https://github.com/acme/widgets/pull/${number}`, + labels: [], + updatedAt: '2026-07-16T00:00:00Z', + author: 'pr-author' + } +} + +function auxiliaryPRResponse(args: string[]): { stdout: string } { + const query = args.find((arg) => arg.startsWith('query=')) ?? '' + if (query.includes('viewerViewedState')) { + return { + stdout: JSON.stringify({ + data: { + repository: { + pullRequest: { + id: 'PR_file_list', + files: { pageInfo: { hasNextPage: false }, nodes: [] } + } + } + } + }) + } + } + if (query.includes('participants(first: 100)')) { + return { + stdout: JSON.stringify({ + data: { repository: { pullRequest: { participants: { nodes: [] } } } } + }) + } + } + return { stdout: JSON.stringify({ data: {} }) } +} + +describe('getWorkItemDetails PR file listing', () => { + beforeEach(() => { + ghExecFileAsyncMock.mockReset() + getOwnerRepoMock.mockReset() + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + getWorkItemMock.mockReset() + getPRChecksMock.mockReset() + getPRChecksMock.mockResolvedValue([]) + getPRCommentsMock.mockReset() + getPRCommentsMock.mockResolvedValue([]) + }) + + it('loads files beyond the first 100-result REST page', async () => { + getWorkItemMock.mockResolvedValueOnce(pullRequestItem(108, 'Large PR')) + const restFile = (index: number) => ({ + filename: `src/file-${index}.ts`, + status: 'modified', + additions: 1, + deletions: 0, + changes: 1, + patch: '@@ -1 +1 @@' + }) + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + const endpoint = args.find((arg) => arg.startsWith('repos/')) ?? '' + if (endpoint === 'repos/acme/widgets/pulls/108') { + return { stdout: JSON.stringify({ head: { sha: 'head' }, base: { sha: 'base' } }) } + } + if (endpoint === 'repos/acme/widgets/pulls/108/files?per_page=100') { + return { + stdout: JSON.stringify(Array.from({ length: 100 }, (_, index) => restFile(index))) + } + } + if (endpoint === 'repos/acme/widgets/pulls/108/files?per_page=100&page=2') { + return { stdout: JSON.stringify([restFile(100)]) } + } + return auxiliaryPRResponse(args) + }) + + const details = await getWorkItemDetails('/repo-root', 108, 'pr') + + expect(details?.files).toHaveLength(101) + expect(details?.files?.at(-1)?.path).toBe('src/file-100.ts') + const fileEndpoints = ghExecFileAsyncMock.mock.calls + .map(([args]) => (args as string[]).find((arg) => arg.includes('/files?'))) + .filter(Boolean) + expect(fileEndpoints).toEqual([ + 'repos/acme/widgets/pulls/108/files?per_page=100', + 'repos/acme/widgets/pulls/108/files?per_page=100&page=2' + ]) + }) + + // Why: a rate-limited/auth-failed file fetch must not render as an empty PR; + // the Files tab keys its retry state off details.filesUnavailable. + it('flags filesUnavailable when the file fetch fails', async () => { + getWorkItemMock.mockResolvedValueOnce(pullRequestItem(8305, 'Files fetch fails')) + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + const target = args.at(-1) + if (target === 'repos/acme/widgets/pulls/8305') { + return { + stdout: JSON.stringify({ head: { sha: 'head-sha' }, base: { sha: 'base-sha' } }) + } + } + if (target === 'repos/acme/widgets/pulls/8305/files?per_page=100') { + throw new Error('gh: API rate limit exceeded (403)') + } + return auxiliaryPRResponse(args) + }) + + const details = await getWorkItemDetails('/repo-root', 8305, 'pr') + + expect(details?.filesUnavailable).toBe(true) + expect(details?.files).toBeUndefined() + }) + + it('preserves an empty file list as an available result', async () => { + getWorkItemMock.mockResolvedValueOnce(pullRequestItem(8306, 'Empty PR')) + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + const target = args.at(-1) + if (target === 'repos/acme/widgets/pulls/8306') { + return { + stdout: JSON.stringify({ head: { sha: 'head-sha' }, base: { sha: 'base-sha' } }) + } + } + if (target === 'repos/acme/widgets/pulls/8306/files?per_page=100') { + return { stdout: '[]' } + } + return auxiliaryPRResponse(args) + }) + + const details = await getWorkItemDetails('/repo-root', 8306, 'pr') + + expect(details?.filesUnavailable).toBe(false) + expect(details?.files).toEqual([]) + }) +}) diff --git a/src/main/github/work-item-details.test.ts b/src/main/github/work-item-details.test.ts index 1a30e1e7f28..ff595a27337 100644 --- a/src/main/github/work-item-details.test.ts +++ b/src/main/github/work-item-details.test.ts @@ -10,6 +10,7 @@ const { getIssueOwnerRepoMock, getEnterpriseGitHubRepoSlugMock, getWorkItemMock, + getWorkItemByOwnerRepoMock, getPRChecksMock, getPRCommentsMock, rateLimitGuardMock, @@ -24,6 +25,7 @@ const { getIssueOwnerRepoMock: vi.fn(), getEnterpriseGitHubRepoSlugMock: vi.fn(), getWorkItemMock: vi.fn(), + getWorkItemByOwnerRepoMock: vi.fn(), getPRChecksMock: vi.fn(), getPRCommentsMock: vi.fn(), rateLimitGuardMock: vi.fn<() => RateLimitGuardResult>(() => ({ blocked: false })), @@ -54,6 +56,7 @@ vi.mock('./gh-utils', () => ({ vi.mock('./client', () => ({ getWorkItem: getWorkItemMock, + getWorkItemByOwnerRepo: getWorkItemByOwnerRepoMock, getPRChecks: getPRChecksMock, getPRComments: getPRCommentsMock })) @@ -77,6 +80,7 @@ describe('getWorkItemDetails', () => { getEnterpriseGitHubRepoSlugMock.mockReset() getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null) getWorkItemMock.mockReset() + getWorkItemByOwnerRepoMock.mockReset() getPRChecksMock.mockReset() getPRCommentsMock.mockReset() rateLimitGuardMock.mockReset() @@ -478,6 +482,12 @@ describe('getWorkItemDetails', () => { it('routes local WSL PR detail fan-out through the selected distro', async () => { const localGitOptions = { wslDistro: 'Ubuntu' } + rateLimitGuardMock.mockReturnValue({ + blocked: true, + remaining: 0, + limit: 5000, + resetAt: 1_800_000_000 + }) getWorkItemMock.mockResolvedValueOnce({ id: 'pr:42', type: 'pr', @@ -539,7 +549,7 @@ describe('getWorkItemDetails', () => { expect(getPRCommentsMock).toHaveBeenCalledWith( '/repo-root', 42, - undefined, + { prRepo: { owner: 'acme', repo: 'widgets', host: 'github.com' } }, null, localGitOptions ) @@ -547,7 +557,7 @@ describe('getWorkItemDetails', () => { '/repo-root', 42, 'head-sha', - null, + { owner: 'acme', repo: 'widgets', host: 'github.com' }, undefined, null, localGitOptions @@ -555,10 +565,12 @@ describe('getWorkItemDetails', () => { expect(ghExecFileAsyncMock.mock.calls.every((call) => call[1]?.wslDistro === 'Ubuntu')).toBe( true ) + expect(rateLimitGuardMock).not.toHaveBeenCalled() + expect(noteRateLimitSpendMock).not.toHaveBeenCalled() }) - it('uses the GitHub Enterprise host for PR files in work item details', async () => { - getWorkItemMock.mockResolvedValueOnce({ + it('uses the GitHub Enterprise host for SSH-backed PR work item details', async () => { + getWorkItemByOwnerRepoMock.mockResolvedValueOnce({ id: 'pr:7', type: 'pr', number: 7, @@ -630,7 +642,7 @@ describe('getWorkItemDetails', () => { throw new Error(`unexpected gh call: ${args.join(' ')}`) }) - const details = await getWorkItemDetails('/repo-root', 7, 'pr') + const details = await getWorkItemDetails('/remote/repo', 7, 'pr', 'ssh-1') expect(details?.body).toBe('Enterprise PR body') expect(details?.headSha).toBe('head-sha') @@ -648,6 +660,29 @@ describe('getWorkItemDetails', () => { viewerViewedState: 'VIEWED' } ]) + expect(getWorkItemMock).not.toHaveBeenCalled() + expect(getWorkItemByOwnerRepoMock).toHaveBeenCalledWith( + '/remote/repo', + { owner: 'team', repo: 'orca', host: 'github.acme-corp.com' }, + 7, + 'pr', + 'ssh-1' + ) + expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(1) + expect(getPRCommentsMock).toHaveBeenCalledWith( + '/remote/repo', + 7, + { prRepo: { owner: 'team', repo: 'orca', host: 'github.acme-corp.com' } }, + 'ssh-1' + ) + expect(getPRChecksMock).toHaveBeenCalledWith( + '/remote/repo', + 7, + 'head-sha', + { owner: 'team', repo: 'orca', host: 'github.acme-corp.com' }, + undefined, + 'ssh-1' + ) const apiCalls = ghExecFileAsyncMock.mock.calls .map(([args]) => args as string[]) .filter((args) => args[0] === 'api') @@ -656,6 +691,46 @@ describe('getWorkItemDetails', () => { 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) + ).toBe(true) + }) + + it('does not fall back to the default host after an Enterprise PR lookup fails', async () => { + getOwnerRepoMock.mockResolvedValue(null) + getEnterpriseGitHubRepoSlugMock.mockResolvedValue({ + owner: 'team', + repo: 'orca', + host: 'github.acme-corp.com' + }) + getWorkItemByOwnerRepoMock.mockResolvedValue(null) + getWorkItemMock.mockResolvedValue({ + id: 'pr:7', + type: 'pr', + number: 7, + title: 'Wrong github.com PR', + state: 'open', + url: 'https://github.com/team/orca/pull/7', + labels: [], + updatedAt: '2026-07-16T00:00:00Z', + author: 'wrong-author' + }) + + await expect(getWorkItemDetails('/remote/repo', 7, 'pr', 'ssh-1')).resolves.toBeNull() + + expect(getWorkItemByOwnerRepoMock).toHaveBeenCalledTimes(1) + expect(getWorkItemMock).not.toHaveBeenCalled() + }) + + it('does not query the default host when an SSH PR repository is unresolved', async () => { + getOwnerRepoMock.mockResolvedValue(null) + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null) + + await expect(getWorkItemDetails('/remote/repo', 7, 'pr', 'ssh-1')).resolves.toBeNull() + + expect(getWorkItemMock).not.toHaveBeenCalled() + expect(getWorkItemByOwnerRepoMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() }) it('uses the GitHub Enterprise host when fetching PR file contents', async () => { diff --git a/src/main/github/work-item-details.ts b/src/main/github/work-item-details.ts index 3fc9d411920..98d6de19e20 100644 --- a/src/main/github/work-item-details.ts +++ b/src/main/github/work-item-details.ts @@ -15,14 +15,18 @@ import { ghExecFileAsync, acquire, release, - getOwnerRepo, getIssueOwnerRepo, ghRepoExecOptions, githubRepoContext, type LocalGitExecOptions } from './gh-utils' -import { getWorkItem, getPRChecks, getPRComments } from './client' -import { getEnterpriseGitHubRepoSlug } from './github-enterprise-repository' +import { getWorkItem, getWorkItemByOwnerRepo, getPRChecks, getPRComments } from './client' +import { + getOriginGitHubApiRepository, + githubApiHostArgs, + isGitHubDotComRepository, + type GitHubApiRepository +} from './github-api-repository' import { noteRateLimitSpend, rateLimitGuard } from './rate-limit' import { getPRReviewCommentLineNumbersFromPatch } from './pr-review-comment-lines' import { isMaxBufferOverflowError } from '../git/max-buffer-overflow' @@ -39,40 +43,31 @@ function localGitOptionArgs(options: LocalGitExecOptions = {}): [] | [LocalGitEx return Object.keys(options).length > 0 ? [options] : [] } -function hostedReviewLocalGitOptions(options: LocalGitExecOptions = {}): { - localGitExecOptions?: LocalGitExecOptions -} { - return Object.keys(options).length > 0 ? { localGitExecOptions: options } : {} -} - -async function getOriginApiOwnerRepo( - repoPath: string, - connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} -): Promise { - const ownerRepo = await getOwnerRepo( - repoPath, - connectionId, - ...localGitOptionArgs(localGitOptions) - ) - return ( - ownerRepo ?? - (await getEnterpriseGitHubRepoSlug( - repoPath, - connectionId, - hostedReviewLocalGitOptions(localGitOptions) - )) - ) -} - -function ghApiHostArgs(ownerRepo: GitHubOwnerRepoSlug): string[] { - return ownerRepo.host && ownerRepo.host !== 'github.com' ? ['--hostname', ownerRepo.host] : [] -} - 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) { @@ -155,8 +150,6 @@ type GraphQLIssueDetailsResponse = { errors?: { message?: string }[] } -type GitHubOwnerRepoSlug = { owner: string; repo: string; host?: string } - type RestTimelineUser = { login?: string | null avatar_url?: string | null @@ -305,7 +298,7 @@ function parseRestTimelineEventLines(stdout: string): RestTimelineEvent[] { } async function getIssueTimelineItems( - ownerRepo: GitHubOwnerRepoSlug, + ownerRepo: GitHubApiRepository, issueNumber: number, ghOptions: ReturnType ): Promise { @@ -315,7 +308,7 @@ async function getIssueTimelineItems( const { stdout } = await ghExecFileAsync( [ 'api', - ...ghApiHostArgs(ownerRepo), + ...githubApiHostArgs(ownerRepo), '--cache', '60s', `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues/${issueNumber}/timeline?per_page=${GITHUB_REST_PAGE_SIZE}&page=${page}`, @@ -512,20 +505,20 @@ function isBinaryHint(file: RESTPRFile): boolean { return file.patch === undefined && file.changes > 0 } -async function getPRHeadBaseSha( +async function getPRMetadata( repoPath: string, prNumber: number, + ownerRepo: GitHubApiRepository | null, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} -): Promise<{ headSha: string; baseSha: string } | null> { +): Promise<{ body: string; headSha?: string; baseSha?: string }> { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = await getOriginApiOwnerRepo(repoPath, connectionId, localGitOptions) try { if (ownerRepo) { const { stdout } = await ghExecFileAsync( [ 'api', - ...ghApiHostArgs(ownerRepo), + ...githubApiHostArgs(ownerRepo), '--cache', '60s', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${prNumber}` @@ -533,28 +526,32 @@ async function getPRHeadBaseSha( ghOptions ) const data = JSON.parse(stdout) as { + body?: string | null head?: { sha?: string } base?: { sha?: string } } - if (data.head?.sha && data.base?.sha) { - return { headSha: data.head.sha, baseSha: data.base.sha } + return { + body: data.body ?? '', + ...(data.head?.sha ? { headSha: data.head.sha } : {}), + ...(data.base?.sha ? { baseSha: data.base.sha } : {}) } - return null } const { stdout } = await ghExecFileAsync( - ['pr', 'view', String(prNumber), '--json', 'headRefOid,baseRefOid'], + ['pr', 'view', String(prNumber), '--json', 'body,headRefOid,baseRefOid'], ghOptions ) const data = JSON.parse(stdout) as { + body?: string headRefOid?: string baseRefOid?: string } - if (data.headRefOid && data.baseRefOid) { - return { headSha: data.headRefOid, baseSha: data.baseRefOid } + return { + body: data.body ?? '', + ...(data.headRefOid ? { headSha: data.headRefOid } : {}), + ...(data.baseRefOid ? { baseSha: data.baseRefOid } : {}) } - return null } catch { - return null + return { body: '' } } } @@ -562,27 +559,35 @@ async function getPRHeadBaseSha( async function getPRFiles( repoPath: string, prNumber: number, + ownerRepo: GitHubApiRepository | null, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = await getOriginApiOwnerRepo(repoPath, connectionId, localGitOptions) if (!ownerRepo) { return null } try { - const { stdout } = await ghExecFileAsync( - [ - 'api', - ...ghApiHostArgs(ownerRepo), - '--cache', - '60s', - `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${prNumber}/files?per_page=100` - ], - ghOptions - ) - const data = JSON.parse(stdout) as RESTPRFile[] - return data.slice(0, MAX_PR_FILES).map((file) => ({ + const data: RESTPRFile[] = [] + for (let page = 1; data.length < MAX_PR_FILES; page += 1) { + const pageSuffix = page === 1 ? '' : `&page=${page}` + const { stdout } = await ghExecFileAsync( + [ + 'api', + ...githubApiHostArgs(ownerRepo), + '--cache', + '60s', + `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${prNumber}/files?per_page=100${pageSuffix}` + ], + ghOptions + ) + const pageData = JSON.parse(stdout) as RESTPRFile[] + data.push(...pageData.slice(0, MAX_PR_FILES - data.length)) + if (pageData.length < 100) { + break + } + } + return data.map((file) => ({ path: file.filename, oldPath: file.previous_filename, status: mapFileStatus(file.status), @@ -604,15 +609,15 @@ type PRFileViewedStatesResult = { async function getPRFileViewedStates( repoPath: string, prNumber: number, + ownerRepo: GitHubApiRepository | null, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = await getOriginApiOwnerRepo(repoPath, connectionId, localGitOptions) if (!ownerRepo) { return null } - if (rateLimitGuard('graphql').blocked) { + if (repositoryRateLimitGuard(ownerRepo, 'graphql', localGitOptions).blocked) { return null } const viewedStates = new Map() @@ -623,7 +628,7 @@ async function getPRFileViewedStates( for (let fetched = 0; fetched < MAX_PR_FILES; fetched += 100) { const args = [ 'api', - ...ghApiHostArgs(ownerRepo), + ...githubApiHostArgs(ownerRepo), 'graphql', '-f', `query=${PR_FILE_VIEWED_STATES_QUERY}`, @@ -637,7 +642,7 @@ async function getPRFileViewedStates( if (after) { args.push('-f', `after=${after}`) } - noteRateLimitSpend('graphql') + noteRepositoryRateLimitSpend(ownerRepo, 'graphql', 1, localGitOptions) const { stdout } = await ghExecFileAsync(args, ghOptions) const parsed = JSON.parse(stdout) as { data?: { @@ -791,43 +796,10 @@ async function getIssueBodyAndComments( } } -async function getPRBody( - repoPath: string, - prNumber: number, - connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} -): Promise { - const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - const ownerRepo = await getOriginApiOwnerRepo(repoPath, connectionId, localGitOptions) - try { - if (ownerRepo) { - const { stdout } = await ghExecFileAsync( - [ - 'api', - ...ghApiHostArgs(ownerRepo), - '--cache', - '60s', - `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${prNumber}` - ], - ghOptions - ) - const data = JSON.parse(stdout) as { body?: string | null } - return data.body ?? '' - } - const { stdout } = await ghExecFileAsync( - ['pr', 'view', String(prNumber), '--json', 'body'], - ghOptions - ) - const data = JSON.parse(stdout) as { body?: string } - return data.body ?? '' - } catch { - return '' - } -} - async function getWorkItemParticipants( repoPath: string, item: Pick, + resolvedRepository: GitHubApiRepository | null, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { @@ -835,19 +807,19 @@ async function getWorkItemParticipants( const ownerRepo = item.type === 'issue' ? await getIssueOwnerRepo(repoPath, connectionId, ...localGitOptionArgs(localGitOptions)) - : await getOriginApiOwnerRepo(repoPath, connectionId, localGitOptions) + : resolvedRepository if (!ownerRepo) { return [] } - if (rateLimitGuard('graphql').blocked) { + if (repositoryRateLimitGuard(ownerRepo, 'graphql', localGitOptions).blocked) { return [] } try { - noteRateLimitSpend('graphql') + noteRepositoryRateLimitSpend(ownerRepo, 'graphql', 1, localGitOptions) const { stdout } = await ghExecFileAsync( [ 'api', - ...ghApiHostArgs(ownerRepo), + ...githubApiHostArgs(ownerRepo), 'graphql', '-f', `query=${WORK_ITEM_PARTICIPANTS_QUERY}`, @@ -893,6 +865,7 @@ async function getWorkItemParticipants( async function getGitHubUsersByLogin( repoPath: string, logins: string[], + repository: GitHubApiRepository | null, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { @@ -902,14 +875,14 @@ async function getGitHubUsersByLogin( if (uniqueLogins.length === 0) { return [] } - if (rateLimitGuard('graphql').blocked) { + const guard = repositoryRateLimitGuard(repository, 'graphql', localGitOptions) + if (guard.blocked) { // Why: log the skip — callers degrade silently to blank GHE avatars, so it's otherwise untraceable. console.warn( `getGitHubUsersByLogin skipped: GraphQL rate-limit budget exhausted (${uniqueLogins.length} logins unresolved)` ) return [] } - const ownerRepo = await getOriginApiOwnerRepo(repoPath, connectionId, localGitOptions) const fields = uniqueLogins .map( (login, index) => @@ -917,11 +890,11 @@ async function getGitHubUsersByLogin( ) .join('\n') try { - noteRateLimitSpend('graphql') + noteRepositoryRateLimitSpend(repository, 'graphql', 1, localGitOptions) const { stdout } = await ghExecFileAsync( [ 'api', - ...(ownerRepo ? ghApiHostArgs(ownerRepo) : []), + ...(repository ? githubApiHostArgs(repository) : []), 'graphql', '-f', `query=query { ${fields} }` @@ -1020,6 +993,7 @@ async function getMentionParticipants( >, comments: PRComment[], participants: GitHubAssignableUser[], + repository: GitHubApiRepository | null, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { @@ -1035,6 +1009,7 @@ async function getMentionParticipants( const graphQlUsers = await getGitHubUsersByLogin( repoPath, visibleLogins, + repository, connectionId, localGitOptions ) @@ -1045,6 +1020,7 @@ async function getPRChecksForDetails( repoPath: string, prNumber: number, headSha: string | undefined, + repository: GitHubApiRepository | null, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { @@ -1053,7 +1029,7 @@ async function getPRChecksForDetails( repoPath, prNumber, headSha, - null, + repository, undefined, connectionId, ...localGitOptionArgs(localGitOptions) @@ -1072,14 +1048,33 @@ export async function getWorkItemDetails( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - // Why: getWorkItem self-manages acquire/release; call it outside our semaphore so the cheap lookup doesn't block detail fetches. - const item: Omit | null = await getWorkItem( - repoPath, - number, - type, - connectionId, - ...localGitOptionArgs(localGitOptions) - ) + const originApiRepository = + type === 'issue' + ? null + : await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) + // Why: cwd-less SSH gh must not fall through to a same-number github.com PR. + if (connectionId && type === 'pr' && !originApiRepository) { + return null + } + // Why: connection-backed GHES repos need a host-qualified identity during the initial lookup. + const useExplicitEnterpriseRepository = + type === 'pr' && originApiRepository !== null && !isGitHubDotComRepository(originApiRepository) + const item: Omit | null = useExplicitEnterpriseRepository + ? await getWorkItemByOwnerRepo( + repoPath, + originApiRepository, + number, + 'pr', + connectionId, + ...localGitOptionArgs(localGitOptions) + ) + : await getWorkItem( + repoPath, + number, + type, + connectionId, + ...localGitOptionArgs(localGitOptions) + ) if (!item) { return null } @@ -1111,13 +1106,14 @@ export async function getWorkItemDetails( // Fallback: fetch body/comments and participants in parallel. const [{ body, comments, assignees, timelineItems }, participants] = await Promise.all([ getIssueBodyAndComments(repoPath, item.number, connectionId, localGitOptions), - getWorkItemParticipants(repoPath, item, connectionId, localGitOptions) + getWorkItemParticipants(repoPath, item, null, connectionId, localGitOptions) ]) const mentionParticipants = await getMentionParticipants( repoPath, item, comments, participants, + null, connectionId, localGitOptions ) @@ -1131,34 +1127,54 @@ export async function getWorkItemDetails( } } - // PR: fetch body + comments + checks + files + head/base SHAs in parallel. - const [body, comments, shas, files, viewedStates, participants] = await Promise.all([ - getPRBody(repoPath, item.number, connectionId, localGitOptions), + // PR: fetch metadata + comments + files + viewed states + participants in parallel. + const [metadata, comments, files, viewedStates, participants] = await Promise.all([ + getPRMetadata(repoPath, item.number, originApiRepository, connectionId, localGitOptions), getPRComments( repoPath, item.number, - undefined, + { prRepo: originApiRepository }, connectionId, ...localGitOptionArgs(localGitOptions) ), - getPRHeadBaseSha(repoPath, item.number, connectionId, localGitOptions), - getPRFiles(repoPath, item.number, connectionId, localGitOptions), - getPRFileViewedStates(repoPath, item.number, connectionId, localGitOptions), - getWorkItemParticipants(repoPath, item, connectionId, localGitOptions) + getPRFiles(repoPath, item.number, originApiRepository, connectionId, localGitOptions), + getPRFileViewedStates( + repoPath, + item.number, + originApiRepository, + connectionId, + localGitOptions + ), + getWorkItemParticipants(repoPath, item, originApiRepository, connectionId, localGitOptions) ]) // Why: mention-author lookup and checks are independent, so run them in parallel. const [mentionParticipants, checks] = await Promise.all([ - getMentionParticipants(repoPath, item, comments, participants, connectionId, localGitOptions), - getPRChecksForDetails(repoPath, item.number, shas?.headSha, connectionId, localGitOptions) + getMentionParticipants( + repoPath, + item, + comments, + participants, + originApiRepository, + connectionId, + localGitOptions + ), + getPRChecksForDetails( + repoPath, + item.number, + metadata.headSha, + originApiRepository, + connectionId, + localGitOptions + ) ]) return { item: enrichItemDisplayAvatars(item, mentionParticipants), - body, + body: metadata.body, comments, - headSha: shas?.headSha, - baseSha: shas?.baseSha, + headSha: metadata.headSha, + baseSha: metadata.baseSha, pullRequestId: viewedStates?.pullRequestId, checks, // Why: null (failed fetch) vs empty PR — Files tab shows retry, not "No files changed." @@ -1176,7 +1192,7 @@ async function fetchContentAtRef(args: { repoPath: string connectionId?: string | null localGitOptions?: LocalGitExecOptions - ownerRepo: GitHubOwnerRepoSlug + ownerRepo: GitHubApiRepository path: string ref: string }): Promise<{ content: string; isBinary: boolean; tooLarge?: boolean }> { @@ -1188,7 +1204,7 @@ async function fetchContentAtRef(args: { '300s', '-H', 'Accept: application/vnd.github.raw', - ...ghApiHostArgs(args.ownerRepo), + ...githubApiHostArgs(args.ownerRepo), `repos/${args.ownerRepo.owner}/${args.ownerRepo.repo}/contents/${encodeGitHubContentPath(args.path)}?ref=${encodeURIComponent(args.ref)}` ], { @@ -1223,7 +1239,7 @@ export async function getPRFileContents(args: { headSha: string baseSha: string }): Promise { - const ownerRepo = await getOriginApiOwnerRepo( + const ownerRepo = await getOriginGitHubApiRepository( args.repoPath, args.connectionId, args.localGitOptions diff --git a/src/main/runtime/rpc/methods/github.test.ts b/src/main/runtime/rpc/methods/github.test.ts index 2135507c3e9..7ea054aefc4 100644 --- a/src/main/runtime/rpc/methods/github.test.ts +++ b/src/main/runtime/rpc/methods/github.test.ts @@ -175,7 +175,7 @@ describe('github RPC methods', () => { repo: 'repo-1', prNumber: 7, headSha: 'abc123', - prRepo: { owner: 'acme', repo: 'widgets' }, + prRepo: { owner: 'acme', repo: 'widgets', host: 'github.acme-corp.com' }, noCache: true }) ) @@ -184,7 +184,7 @@ describe('github RPC methods', () => { 'repo-1', 7, 'abc123', - { owner: 'acme', repo: 'widgets' }, + { owner: 'acme', repo: 'widgets', host: 'github.acme-corp.com' }, { noCache: true } diff --git a/src/main/runtime/rpc/methods/github.ts b/src/main/runtime/rpc/methods/github.ts index 44375181da0..b72019dccac 100644 --- a/src/main/runtime/rpc/methods/github.ts +++ b/src/main/runtime/rpc/methods/github.ts @@ -42,7 +42,10 @@ const RateLimit = z.object({ const SlugRepo = z.object({ owner: requiredString('Missing owner'), - repo: requiredString('Missing repo') + repo: 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 }) const SlugAssignableUsers = SlugRepo.extend({ diff --git a/src/preload/index.ts b/src/preload/index.ts index eed959cd2cb..be71495f782 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -25,6 +25,7 @@ import type { GitHubAssignableUser, GitHubCommentResult, GitHubCreateIssueResult, + GitHubOwnerRepo, GitHubWorkItem, JiraProjectStatusOrder, GitPushTarget, @@ -1258,7 +1259,7 @@ const api = { sourceContext?: TaskSourceContext | null prNumber: number headSha?: string - prRepo?: { owner: string; repo: string } | null + prRepo?: GitHubOwnerRepo | null noCache?: boolean }): Promise => ipcRenderer.invoke('gh:prChecks', args), @@ -1270,7 +1271,7 @@ const api = { workflowRunId?: number checkName?: string url?: string | null - prRepo?: { owner: string; repo: string } | null + prRepo?: GitHubOwnerRepo | null }): Promise => ipcRenderer.invoke('gh:prCheckDetails', args), rerunPRChecks: (args: { @@ -1288,7 +1289,7 @@ const api = { repoId?: string sourceContext?: TaskSourceContext | null prNumber: number - prRepo?: { owner: string; repo: string } | null + prRepo?: GitHubOwnerRepo | null noCache?: boolean }): Promise => ipcRenderer.invoke('gh:prComments', args), @@ -1315,7 +1316,7 @@ const api = { repoId?: string prNumber: number title: string - prRepo?: { owner: string; repo: string } | null + prRepo?: GitHubOwnerRepo | null }): Promise => ipcRenderer.invoke('gh:updatePRTitle', args), mergePR: (args: { @@ -1324,7 +1325,7 @@ const api = { sourceContext?: TaskSourceContext | null prNumber: number method?: 'merge' | 'squash' | 'rebase' - prRepo?: { owner: string; repo: string } | null + prRepo?: GitHubOwnerRepo | null }): Promise<{ ok: true } | { ok: false; error: string }> => ipcRenderer.invoke('gh:mergePR', args), @@ -1335,7 +1336,7 @@ const api = { prNumber: number enabled: boolean method?: 'merge' | 'squash' | 'rebase' - prRepo?: { owner: string; repo: string } | null + prRepo?: GitHubOwnerRepo | null }): Promise<{ ok: true } | { ok: false; error: string }> => ipcRenderer.invoke('gh:setPRAutoMerge', args), @@ -1382,7 +1383,7 @@ const api = { number: number body: string type?: 'issue' | 'pr' - prRepo?: { owner: string; repo: string } | null + prRepo?: GitHubOwnerRepo | null }): Promise => ipcRenderer.invoke('gh:addIssueComment', args), addPRReviewCommentReply: (args: { @@ -1395,7 +1396,7 @@ const api = { threadId?: string path?: string line?: number - prRepo?: { owner: string; repo: string } | null + prRepo?: GitHubOwnerRepo | null }): Promise => ipcRenderer.invoke('gh:addPRReviewCommentReply', args), addPRReviewComment: (args: { diff --git a/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.test.ts b/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.test.ts index c67a3b8cc2b..a011344e609 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.test.ts +++ b/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.test.ts @@ -27,6 +27,23 @@ describe('checksPanelAsyncResultKey', () => { ).toBe('repo-id::feature/test::acme/widgets::12::none') }) + it('includes non-default GitHub hosts in PR repo identity', () => { + const githubDotComKey = checksPanelAsyncResultKey('repo-id', 'feature/test', 12, { + owner: 'Acme', + repo: 'Widgets', + host: 'github.com' + }) + const enterpriseKey = checksPanelAsyncResultKey('repo-id', 'feature/test', 12, { + owner: 'Acme', + repo: 'Widgets', + host: 'github.acme-corp.com' + }) + + expect(githubDotComKey).toBe('repo-id::feature/test::acme/widgets::12::none') + expect(enterpriseKey).toBe('repo-id::feature/test::github.acme-corp.com/acme/widgets::12::none') + expect(enterpriseKey).not.toBe(githubDotComKey) + }) + it('includes PR head SHA so stale checks cannot commit after a new head is discovered', () => { expect(checksPanelAsyncResultKey('repo-id', 'feature/test', 12, null, 'head-a')).toBe( 'repo-id::feature/test::none::12::head-a' 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 feee86c980d..3728f8b91c9 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 @@ -4,7 +4,9 @@ function normalizedPRRepoIdentity(prRepo?: GitHubOwnerRepo | null): string { if (!prRepo) { return 'none' } - return `${prRepo.owner.toLowerCase()}/${prRepo.repo.toLowerCase()}` + const slug = `${prRepo.owner.toLowerCase()}/${prRepo.repo.toLowerCase()}` + const host = prRepo.host?.toLowerCase() + return host && host !== 'github.com' ? `${host}/${slug}` : slug } export function checksPanelAsyncResultKey( diff --git a/src/renderer/src/store/slices/github-checks.test.ts b/src/renderer/src/store/slices/github-checks.test.ts index 240521b8b15..2f295ab9328 100644 --- a/src/renderer/src/store/slices/github-checks.test.ts +++ b/src/renderer/src/store/slices/github-checks.test.ts @@ -91,4 +91,34 @@ describe('syncPRChecksStatus', () => { expect(result?.prCache?.['repo-id::main']?.data?.checksStatus).toBe('success') expect(result?.prCache?.['runtime:env-win::repo-id::main']?.data?.checksStatus).toBe('neutral') }) + + it('rejects a checks result from the same slug on a different GitHub host', () => { + const state = { + prCache: { + 'repo-id::main': { + fetchedAt: 0, + data: { + checksStatus: 'neutral' as const, + prRepo: { + owner: 'acme', + repo: 'widgets', + host: 'github.acme-corp.com' + } + } + } + } + } as unknown as AppState + + const result = syncPRChecksStatus( + state, + '/repo', + 'repo-id', + 'main', + [{ name: 'build', status: 'completed', conclusion: 'success', url: null }], + undefined, + { owner: 'acme', repo: 'widgets', host: 'github.com' } + ) + + expect(result).toBeNull() + }) }) diff --git a/src/renderer/src/store/slices/github-checks.ts b/src/renderer/src/store/slices/github-checks.ts index 13e60a2c878..c90a83d2932 100644 --- a/src/renderer/src/store/slices/github-checks.ts +++ b/src/renderer/src/store/slices/github-checks.ts @@ -100,7 +100,9 @@ function normalizedPRRepo(repo?: GitHubOwnerRepo | null): string | null { if (!repo) { return null } - return `${repo.owner.toLowerCase()}/${repo.repo.toLowerCase()}` + const slug = `${repo.owner.toLowerCase()}/${repo.repo.toLowerCase()}` + const host = repo.host?.toLowerCase() + return host && host !== 'github.com' ? `${host}/${slug}` : slug } function samePRRepo(left?: GitHubOwnerRepo | null, right?: GitHubOwnerRepo | null): boolean { diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index dd4453c09ec..fba993e6655 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -966,6 +966,22 @@ describe('createGitHubSlice.fetchPRChecks', () => { }) }) + it('isolates PR detail caches by Enterprise host', () => { + const githubRepo = { owner: 'Acme', repo: 'Widgets', host: 'github.com' } + const enterpriseRepo = { + owner: 'Acme', + repo: 'Widgets', + host: 'github.acme-corp.com' + } + + expect(prChecksCacheSuffix(12, enterpriseRepo, 'head')).not.toBe( + prChecksCacheSuffix(12, githubRepo, 'head') + ) + expect(prCommentsCacheSuffix(12, enterpriseRepo)).not.toBe( + prCommentsCacheSuffix(12, githubRepo) + ) + }) + it('bounds checks cache entries across many repo and head combinations', async () => { vi.useFakeTimers() diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index 6d1af5ad691..46799393ed6 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -859,7 +859,9 @@ function evictRepoCacheEntries( } function normalizedRepoIdentity(repo: GitHubOwnerRepo): string { - return `${repo.owner.toLowerCase()}/${repo.repo.toLowerCase()}` + const slug = `${repo.owner.toLowerCase()}/${repo.repo.toLowerCase()}` + const host = repo.host?.toLowerCase() + return host && host !== 'github.com' ? `${host}/${slug}` : slug } function normalizedHeadSha(headSha?: string): string | null { diff --git a/src/shared/types.ts b/src/shared/types.ts index fbb1288de3d..35abb82bcab 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -1154,7 +1154,9 @@ export type PRConflictSummary = { localMergeState?: 'clean' } -export type GitHubRepositoryIdentity = { owner: string; repo: string } +// Why: host must survive renderer/RPC boundaries so Enterprise review actions +// cannot silently fall back to a same-named repository on github.com. +export type GitHubRepositoryIdentity = { owner: string; repo: string; host?: string } export type GitHubPRMergeMethod = 'merge' | 'squash' | 'rebase'