diff --git a/src/main/github/client-pr-check-details.test.ts b/src/main/github/client-pr-check-details.test.ts index ff210f7bc7e..df924954886 100644 --- a/src/main/github/client-pr-check-details.test.ts +++ b/src/main/github/client-pr-check-details.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GithubEnterpriseRepositoryModule from './github-enterprise-repository' const { ghExecFileAsyncMock, getOwnerRepoMock, rateLimitGuardMock } = vi.hoisted(() => ({ ghExecFileAsyncMock: vi.fn(), @@ -26,6 +27,11 @@ vi.mock('../git/runner', () => ({ gitExecFileAsync: vi.fn() })) +vi.mock('./github-enterprise-repository', async (importOriginal) => ({ + ...(await importOriginal()), + isGitHubHostAuthenticated: vi.fn().mockResolvedValue(true) +})) + vi.mock('./rate-limit', () => ({ rateLimitGuard: rateLimitGuardMock, noteRateLimitSpend: vi.fn(), diff --git a/src/main/github/client-pr-local-runtime.test.ts b/src/main/github/client-pr-local-runtime.test.ts index c5e28ce0fed..d54db52aaa1 100644 --- a/src/main/github/client-pr-local-runtime.test.ts +++ b/src/main/github/client-pr-local-runtime.test.ts @@ -442,6 +442,9 @@ describe('GitHub PR local runtime routing', () => { await expect( getWorkItemByOwnerRepo('/remote/repo', enterpriseRepo, 7, 'pr', 'ssh-1') ).resolves.toMatchObject({ number: 7, title: 'Enterprise PR' }) + await expect( + getWorkItemByOwnerRepo('/remote/repo', { owner: 'team', repo: 'orca' }, 7, 'pr', 'ssh-1') + ).resolves.toMatchObject({ number: 7, title: 'Enterprise PR' }) await expect( getPRComments('/remote/repo', 7, { prRepo: enterpriseRepo }, 'ssh-1') ).resolves.toEqual([]) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 01b460d3ee0..0bd5e56ae19 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -81,6 +81,7 @@ import { githubHostExecOptions, githubRepositorySlugArg, githubRepositoryWebHost, + resolveGitHubApiRepository, resolveGitHubApiRepositoryCandidates, resolveGitHubRepoExecution, resolveIssueGitHubApiRepositorySource, @@ -2156,12 +2157,19 @@ export async function getWorkItemByOwnerRepo( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { + const requestedHost = ownerRepo.host?.trim().toLowerCase() + const requestedRepository = requestedHost + ? { ...ownerRepo, host: requestedHost } + : await resolveGitHubApiRepository(repoPath, ownerRepo, connectionId, localGitOptions) + if (!requestedRepository) { + return null + } const { candidates } = await resolveGitHubApiRepositoryCandidates( repoPath, connectionId, localGitOptions ) - const requestedKey = githubRepoIdentityKey(ownerRepo) + const requestedKey = githubRepoIdentityKey(requestedRepository) const matchedRepository = candidates.find( (candidate) => githubRepoIdentityKey(candidate) === requestedKey ) diff --git a/src/main/github/github-api-repository.test.ts b/src/main/github/github-api-repository.test.ts index 64ff0f8ead1..e3fd4666c7d 100644 --- a/src/main/github/github-api-repository.test.ts +++ b/src/main/github/github-api-repository.test.ts @@ -1,22 +1,36 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type * as GitHubEnterpriseRepository from './github-enterprise-repository' +import type * as GhUtils from './gh-utils' -const { isGitHubHostAuthenticatedMock } = vi.hoisted(() => ({ - isGitHubHostAuthenticatedMock: vi.fn() +const { getEnterpriseGitHubRepoSlugMock, getOwnerRepoMock, isGitHubHostAuthenticatedMock } = + vi.hoisted(() => ({ + getEnterpriseGitHubRepoSlugMock: vi.fn(), + getOwnerRepoMock: vi.fn(), + isGitHubHostAuthenticatedMock: vi.fn() + })) + +vi.mock('./gh-utils', async (importOriginal) => ({ + ...(await importOriginal()), + getOwnerRepo: getOwnerRepoMock })) vi.mock('./github-enterprise-repository', async (importOriginal) => ({ ...(await importOriginal()), + getEnterpriseGitHubRepoSlug: getEnterpriseGitHubRepoSlugMock, isGitHubHostAuthenticated: isGitHubHostAuthenticatedMock })) import { + _resetOriginGitHubApiRepositoryCache, githubHostExecOptions, resolveGitHubApiRepository, resolveGitHubRepoExecution } from './github-api-repository' beforeEach(() => { + _resetOriginGitHubApiRepositoryCache() + getEnterpriseGitHubRepoSlugMock.mockReset().mockResolvedValue(null) + getOwnerRepoMock.mockReset().mockResolvedValue(null) isGitHubHostAuthenticatedMock.mockReset().mockResolvedValue(false) }) @@ -80,6 +94,20 @@ describe('resolveGitHubRepoExecution', () => { ) }) + it.each([ + { owner: 'acme%2Fadmin', repo: 'widgets' }, + { owner: 'acme', repo: '..' } + ])('rejects repository overrides that could alter a gh REST path: %o', async (repository) => { + await expect( + resolveGitHubApiRepository('/repo', { + ...repository, + host: 'github.com' + }) + ).resolves.toBeNull() + + expect(isGitHubHostAuthenticatedMock).not.toHaveBeenCalled() + }) + it('normalizes github.com without spending an auth inventory probe', async () => { await expect( resolveGitHubApiRepository('/repo', { @@ -92,14 +120,48 @@ describe('resolveGitHubRepoExecution', () => { expect(isGitHubHostAuthenticatedMock).not.toHaveBeenCalled() }) - it('uses a caller-specific repository resolver without changing its identity', async () => { + it('backfills the origin host for a host-less caller-specific resolver', async () => { const ownerRepo = { owner: 'upstream', repo: 'widgets' } + getOwnerRepoMock.mockResolvedValue({ owner: 'fork', repo: 'widgets' }) - await expect( - resolveGitHubRepoExecution('/remote/repo', async () => ownerRepo, 'ssh-1') - ).resolves.toEqual({ - ownerRepo, - ghOptions: {} + await expect(resolveGitHubRepoExecution('/repo', async () => ownerRepo)).resolves.toEqual({ + ownerRepo: { ...ownerRepo, host: 'github.com' }, + ghOptions: { cwd: '/repo', host: 'github.com' } }) }) + + it('backfills an Enterprise origin host for a host-less caller-specific resolver', async () => { + const ownerRepo = { owner: 'upstream', repo: 'widgets' } + getEnterpriseGitHubRepoSlugMock.mockResolvedValue({ + owner: 'fork', + repo: 'widgets', + host: 'github.acme-corp.com' + }) + + await expect(resolveGitHubRepoExecution('/repo', async () => ownerRepo)).resolves.toEqual({ + ownerRepo: { ...ownerRepo, host: 'github.acme-corp.com' }, + ghOptions: { cwd: '/repo', host: 'github.acme-corp.com' } + }) + }) + + it('rejects a host-less caller-specific resolver for a connection-backed repository', async () => { + await expect( + resolveGitHubRepoExecution( + '/remote/repo', + async () => ({ owner: 'upstream', repo: 'widgets' }), + 'ssh-1' + ) + ).resolves.toEqual({ ownerRepo: null, ghOptions: {} }) + }) + + it('preserves an authoritative null from a caller-specific resolver', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'origin', repo: 'widgets' }) + + await expect(resolveGitHubRepoExecution('/repo', async () => null)).resolves.toEqual({ + ownerRepo: null, + ghOptions: { cwd: '/repo' } + }) + + expect(getOwnerRepoMock).not.toHaveBeenCalled() + }) }) diff --git a/src/main/github/github-api-repository.ts b/src/main/github/github-api-repository.ts index ce342b74f25..1ca1ec9c155 100644 --- a/src/main/github/github-api-repository.ts +++ b/src/main/github/github-api-repository.ts @@ -29,6 +29,20 @@ type GitHubApiRepositoryResolution = | undefined | (() => Promise) +// Why: renderer/RPC repository overrides are interpolated into REST paths. +// Reject path syntax before an authenticated gh process can target it. +const GITHUB_OWNER_SLUG_RE = /^[A-Za-z0-9][A-Za-z0-9-]*$/ +const GITHUB_REPO_SLUG_RE = /^[A-Za-z0-9._-]+$/ + +function isValidGitHubApiRepository(repository: GitHubApiRepository): boolean { + return ( + GITHUB_OWNER_SLUG_RE.test(repository.owner) && + GITHUB_REPO_SLUG_RE.test(repository.repo) && + repository.repo !== '.' && + repository.repo !== '..' + ) +} + // Why: the enterprise branch spawns an uncached `git remote get-url` (an SSH // round trip on connection-backed repos) — hot paths like per-file contents // and viewed-state toggles resolve per call, so cache like ownerRepoCache does. @@ -240,6 +254,9 @@ export async function resolveGitHubApiRepository( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { + if (repository && !isValidGitHubApiRepository(repository)) { + return null + } if (repository?.host) { const host = repository.host.trim().toLowerCase() if (!host) { @@ -297,10 +314,18 @@ export async function resolveGitHubRepoExecution( ): Promise { // Why: issue-scoped paths retain their upstream-first resolver while sharing // the same repo-scoped and host-scoped gh execution option construction. + const requestedRepository = typeof repository === 'function' ? await repository() : repository + // Why: normalize host-less resolver results without replacing an + // authoritative null with the generic origin fallback. const ownerRepo = - typeof repository === 'function' - ? await repository() - : await resolveGitHubApiRepository(repoPath, repository, connectionId, localGitOptions) + typeof repository === 'function' && !requestedRepository + ? null + : await resolveGitHubApiRepository( + repoPath, + requestedRepository, + connectionId, + localGitOptions + ) return { ownerRepo, ghOptions: { diff --git a/src/main/github/issues.test.ts b/src/main/github/issues.test.ts index e510a363cb1..90b090b347a 100644 --- a/src/main/github/issues.test.ts +++ b/src/main/github/issues.test.ts @@ -1,5 +1,6 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type * as GithubApiRepositoryModule from './github-api-repository' +import type * as GithubEnterpriseRepositoryModule from './github-enterprise-repository' import type * as GhUtils from './gh-utils' const { @@ -28,6 +29,11 @@ vi.mock('./gh-utils', async () => { } }) +vi.mock('./github-enterprise-repository', async (importOriginal) => ({ + ...(await importOriginal()), + isGitHubHostAuthenticated: vi.fn().mockResolvedValue(true) +})) + vi.mock('./github-api-repository', async (importOriginal) => { const actual = await importOriginal() return { diff --git a/src/main/github/project-view-host-auth.test.ts b/src/main/github/project-view-host-auth.test.ts index ec426d95319..f243c06ed45 100644 --- a/src/main/github/project-view-host-auth.test.ts +++ b/src/main/github/project-view-host-auth.test.ts @@ -36,6 +36,7 @@ vi.mock('./github-enterprise-repository', () => ({ })) import { runGraphql, runRest } from './project-view/internals' +import { _resetProjectViewCachesForTests, resolveProjectRef } from './project-view' describe('project view host authentication boundary', () => { beforeEach(() => { @@ -44,6 +45,7 @@ describe('project view host authentication boundary', () => { ghExecFileAsyncMock.mockReset() hostAuthenticatedMock.mockReset() noteRepositoryRateLimitSpendMock.mockReset() + _resetProjectViewCachesForTests() }) it('does not send GraphQL or REST requests to an unconfigured host', async () => { @@ -90,4 +92,34 @@ describe('project view host authentication boundary', () => { expect(hostAuthenticatedMock).not.toHaveBeenCalled() }) + + it('uses a pasted github.com URL instead of the ambient Enterprise host', async () => { + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + const query = args.find((arg) => arg.startsWith('query=')) ?? '' + return query.includes('projectV2') + ? { + stdout: JSON.stringify({ + data: { organization: { projectV2: { id: 'PVT_7', title: 'Roadmap' } } } + }), + stderr: '' + } + : { + stdout: JSON.stringify({ data: { organization: { login: 'acme' } } }), + stderr: '' + } + }) + + await expect( + resolveProjectRef({ + input: 'https://github.com/orgs/acme/projects/7', + host: 'github.corp.example' + }) + ).resolves.toMatchObject({ ok: true, host: 'github.com' }) + + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + expect( + ghExecFileAsyncMock.mock.calls.every(([, options]) => options.host === 'github.com') + ).toBe(true) + expect(hostAuthenticatedMock).not.toHaveBeenCalled() + }) }) diff --git a/src/main/github/project-view.test.ts b/src/main/github/project-view.test.ts index 0c1a7472887..f5f468b7d9b 100644 --- a/src/main/github/project-view.test.ts +++ b/src/main/github/project-view.test.ts @@ -136,6 +136,7 @@ describe('parseProjectPaste', () => { kind: 'org', owner: 'acme', number: 42, + host: 'github.com', viewNumber: 3 }) }) @@ -144,7 +145,8 @@ describe('parseProjectPaste', () => { expect(parseProjectPaste('https://github.com/users/octocat/projects/1')).toEqual({ kind: 'user', owner: 'octocat', - number: 1 + number: 1, + host: 'github.com' }) }) @@ -158,13 +160,14 @@ describe('parseProjectPaste', () => { kind: 'org', owner: 'acme', number: 7, + host: 'github.corp.example', viewNumber: 2 }) expect(parseProjectPaste(url)).toBeNull() // github.com URLs still parse when a GHES host is supplied. expect( parseProjectPaste('https://github.com/orgs/acme/projects/7', 'github.corp.example') - ).toEqual({ kind: 'org', owner: 'acme', number: 7 }) + ).toEqual({ kind: 'org', owner: 'acme', number: 7, host: 'github.com' }) }) it('preserves a GHES custom port while parsing project URLs', () => { @@ -173,7 +176,12 @@ describe('parseProjectPaste', () => { 'https://github.corp.example:8443/orgs/acme/projects/7', 'github.corp.example:8443' ) - ).toEqual({ kind: 'org', owner: 'acme', number: 7 }) + ).toEqual({ + kind: 'org', + owner: 'acme', + number: 7, + host: 'github.corp.example:8443' + }) }) it('rejects credentials and paths that only begin like a Project URL', () => { diff --git a/src/main/github/project-view.ts b/src/main/github/project-view.ts index 2cd220c2b5d..d0a47b9a8aa 100644 --- a/src/main/github/project-view.ts +++ b/src/main/github/project-view.ts @@ -1612,8 +1612,8 @@ export async function listAccessibleProjects( // ─── resolveProjectRef ───────────────────────────────────────────────── type ParsedPaste = - | { kind: 'org'; owner: string; number: number; viewNumber?: number } - | { kind: 'user'; owner: string; number: number; viewNumber?: number } + | { kind: 'org'; owner: string; number: number; host: string; viewNumber?: number } + | { kind: 'user'; owner: string; number: number; host: string; viewNumber?: number } | { kind: 'bare'; owner: string; number: number } export function parseProjectPaste(input: string, host?: string): ParsedPaste | null { @@ -1656,6 +1656,7 @@ export function parseProjectPaste(input: string, host?: string): ParsedPaste | n kind: parts[0] === 'orgs' ? 'org' : 'user', owner: parts[1], number, + host: url.host.toLowerCase(), ...(viewNumber !== undefined ? { viewNumber } : {}) } } catch { @@ -1785,8 +1786,11 @@ export async function resolveProjectRef( } const preferred: GitHubProjectOwnerType | null = parsed.kind === 'org' ? 'organization' : parsed.kind === 'user' ? 'user' : null + // Why: a pasted URL is authoritative. The ambient host only applies to + // owner/number shorthand; otherwise same-number Projects can cross hosts. + const executionHost = parsed.kind === 'bare' ? githubProjectHost(args.host) : parsed.host // Verify by fetching project title. - const ownerRes = await resolveOwnerType(parsed.owner, preferred, args.host) + const ownerRes = await resolveOwnerType(parsed.owner, preferred, executionHost) if (!ownerRes.ok) { return { ok: false, error: ownerRes.error } } @@ -1799,7 +1803,7 @@ export async function resolveProjectRef( ` const res = await runGraphql< Record - >(query, { owner: parsed.owner, num: parsed.number }, projectGhExecOptions(args.host)) + >(query, { owner: parsed.owner, num: parsed.number }, projectGhExecOptions(executionHost)) if (!res.ok) { return { ok: false, error: res.error } } @@ -1813,7 +1817,7 @@ export async function resolveProjectRef( ownerType, number: parsed.number, title: p.title ?? '', - host: githubProjectHost(args.host), + host: executionHost, // Why: forward the parsed view number from /views/{n} URLs so the // renderer can skip the view-pick step. parsed.kind === 'bare' has no // viewNumber (owner/number shorthand carries no view). diff --git a/src/main/github/work-item-details-pr-files.test.ts b/src/main/github/work-item-details-pr-files.test.ts index fc86f204c8e..93c524ae888 100644 --- a/src/main/github/work-item-details-pr-files.test.ts +++ b/src/main/github/work-item-details-pr-files.test.ts @@ -1,5 +1,9 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +type RateLimitGuardResult = + | { blocked: false } + | { blocked: true; remaining: number; limit: number; resetAt: number } + const { ghExecFileAsyncMock, getOwnerRepoMock, @@ -28,7 +32,7 @@ const { repository: { host?: string } | null | undefined, bucket: string, options?: { cwd?: string; host?: string } - ) => { blocked: false } + ) => RateLimitGuardResult >(() => ({ blocked: false })) })) @@ -65,7 +69,7 @@ vi.mock('./rate-limit', () => ({ noteRepositoryRateLimitSpend: noteRepositoryRateLimitSpendMock })) -import { getWorkItemDetails } from './work-item-details' +import { getPRFileContents, getWorkItemDetails } from './work-item-details' import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' @@ -126,7 +130,8 @@ describe('getWorkItemDetails PR file listing', () => { getPRCommentsMock.mockReset() getPRCommentsMock.mockResolvedValue([]) noteRepositoryRateLimitSpendMock.mockReset() - repositoryRateLimitGuardMock.mockClear() + repositoryRateLimitGuardMock.mockReset() + repositoryRateLimitGuardMock.mockReturnValue({ blocked: false }) }) it('loads files beyond the first 100-result REST page', async () => { @@ -217,4 +222,77 @@ describe('getWorkItemDetails PR file listing', () => { expect(details?.filesUnavailable).toBe(false) expect(details?.files).toEqual([]) }) + + it('uses the selected upstream GitHub Enterprise repo for both PR file sides', async () => { + const prRepo = { + owner: 'team', + repo: 'orca', + host: 'github.acme-corp.com' + } + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + const endpoint = args.find((arg) => arg.startsWith('repos/')) ?? '' + if (endpoint === 'repos/team/orca/contents/src/path%23with%3Fchars.ts?ref=base-sha') { + return { stdout: 'base content' } + } + if (endpoint === 'repos/team/orca/contents/src/path%23with%3Fchars.ts?ref=head-sha') { + return { stdout: 'head content' } + } + throw new Error(`unexpected gh call: ${args.join(' ')}`) + }) + + const contents = await getPRFileContents({ + repoPath: '/repo-root', + prRepo, + prNumber: 7, + path: 'src/path#with?chars.ts', + status: 'modified', + headSha: 'head-sha', + baseSha: 'base-sha' + }) + + expect(contents).toMatchObject({ + original: 'base content', + modified: 'head content', + originalIsBinary: false, + modifiedIsBinary: false + }) + const apiCalls = ghExecFileAsyncMock.mock.calls.map(([args]) => args as string[]) + expect(apiCalls).toHaveLength(2) + expect(apiCalls.every((args) => !args.includes('--hostname'))).toBe(true) + expect( + ghExecFileAsyncMock.mock.calls.every( + ([, options]) => options?.host === 'github.acme-corp.com' + ) + ).toBe(true) + expect( + repositoryRateLimitGuardMock.mock.calls.filter(([, bucket]) => bucket === 'core') + ).toHaveLength(2) + expect( + noteRepositoryRateLimitSpendMock.mock.calls.filter(([, bucket]) => bucket === 'core') + ).toHaveLength(2) + }) + + it('does not fetch raw PR file contents while the repository core budget is blocked', async () => { + repositoryRateLimitGuardMock.mockReturnValue({ + blocked: true, + remaining: 0, + limit: 5000, + resetAt: 1_800_000_000 + }) + + await expect( + getPRFileContents({ + repoPath: '/repo-root', + prRepo: { owner: 'team', repo: 'orca', host: 'github.acme-corp.com' }, + prNumber: 7, + path: 'src/file.ts', + status: 'modified', + headSha: 'head-sha', + baseSha: 'base-sha' + }) + ).resolves.toMatchObject({ original: '', modified: '' }) + + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + expect(noteRepositoryRateLimitSpendMock).not.toHaveBeenCalled() + }) }) diff --git a/src/main/github/work-item-details.test.ts b/src/main/github/work-item-details.test.ts index cf9f2b86fef..5d939eda1f2 100644 --- a/src/main/github/work-item-details.test.ts +++ b/src/main/github/work-item-details.test.ts @@ -30,7 +30,13 @@ const { getWorkItemByOwnerRepoMock: vi.fn(), getPRChecksMock: vi.fn(), getPRCommentsMock: vi.fn(), - repositoryRateLimitGuardMock: vi.fn<() => RateLimitGuardResult>(() => ({ blocked: false })), + repositoryRateLimitGuardMock: vi.fn< + ( + repository: { host?: string } | null | undefined, + bucket: 'core' | 'graphql' | 'search', + options?: { cwd?: string; wslDistro?: string } + ) => RateLimitGuardResult + >(() => ({ blocked: false })), noteRepositoryRateLimitSpendMock: vi.fn(), ghRepoExecOptionsMock: vi.fn((context) => context.connectionId @@ -74,7 +80,7 @@ vi.mock('./rate-limit', () => ({ noteRepositoryRateLimitSpend: noteRepositoryRateLimitSpendMock })) -import { getPRFileContents, getWorkItemDetails } from './work-item-details' +import { getWorkItemDetails } from './work-item-details' import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' @@ -252,6 +258,17 @@ describe('getWorkItemDetails', () => { 1, { cwd: '/repo-root', host: 'github.com' } ) + expect(repositoryRateLimitGuardMock).toHaveBeenCalledWith( + { owner: 'acme', repo: 'widgets', host: 'github.com' }, + 'core', + { cwd: '/repo-root', host: 'github.com' } + ) + expect(noteRepositoryRateLimitSpendMock).toHaveBeenCalledWith( + { owner: 'acme', repo: 'widgets', host: 'github.com' }, + 'core', + 1, + { cwd: '/repo-root', host: 'github.com' } + ) }) it('scopes collapsed issue GraphQL accounting to an implicit WSL UNC runtime', async () => { @@ -486,12 +503,16 @@ describe('getWorkItemDetails', () => { }) it('skips optional GraphQL issue detail calls when the cached GraphQL budget is low', async () => { - repositoryRateLimitGuardMock.mockReturnValue({ - blocked: true, - remaining: 3, - limit: 5000, - resetAt: 1_800_000_000 - }) + repositoryRateLimitGuardMock.mockImplementation((_repository, bucket) => + bucket === 'graphql' + ? { + blocked: true, + remaining: 3, + limit: 5000, + resetAt: 1_800_000_000 + } + : { blocked: false } + ) getWorkItemMock.mockResolvedValueOnce({ id: 'issue:923', type: 'issue', @@ -513,7 +534,15 @@ describe('getWorkItemDetails', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(3) expect(ghExecFileAsyncMock.mock.calls.some((call) => call[0][1] === 'graphql')).toBe(false) - expect(noteRepositoryRateLimitSpendMock).not.toHaveBeenCalled() + expect( + noteRepositoryRateLimitSpendMock.mock.calls.some(([, bucket]) => bucket === 'graphql') + ).toBe(false) + expect(noteRepositoryRateLimitSpendMock).toHaveBeenCalledWith( + { owner: 'acme', repo: 'widgets', host: 'github.com' }, + 'core', + 2, + { cwd: '/repo-root', host: 'github.com' } + ) expect(details?.body).toBe('Issue body') expect(details?.participants).toEqual([]) }) @@ -847,50 +876,6 @@ describe('getWorkItemDetails', () => { expect(ghExecFileAsyncMock).not.toHaveBeenCalled() }) - it('uses the selected upstream GitHub Enterprise repo for both PR file sides', async () => { - getOwnerRepoMock.mockResolvedValue({ owner: 'fork', repo: 'orca' }) - const prRepo = { - owner: 'team', - repo: 'orca', - host: 'github.acme-corp.com' - } - ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { - const endpoint = args.find((arg) => arg.startsWith('repos/')) ?? '' - if (endpoint === 'repos/team/orca/contents/src/path%23with%3Fchars.ts?ref=base-sha') { - return { stdout: 'base content' } - } - if (endpoint === 'repos/team/orca/contents/src/path%23with%3Fchars.ts?ref=head-sha') { - return { stdout: 'head content' } - } - throw new Error(`unexpected gh call: ${args.join(' ')}`) - }) - - const contents = await getPRFileContents({ - repoPath: '/repo-root', - prRepo, - prNumber: 7, - path: 'src/path#with?chars.ts', - status: 'modified', - headSha: 'head-sha', - baseSha: 'base-sha' - }) - - expect(contents).toMatchObject({ - original: 'base content', - modified: 'head content', - originalIsBinary: false, - modifiedIsBinary: false - }) - const apiCalls = ghExecFileAsyncMock.mock.calls.map(([args]) => args as string[]) - expect(apiCalls).toHaveLength(2) - expect(apiCalls.every((args) => !args.includes('--hostname'))).toBe(true) - expect( - ghExecFileAsyncMock.mock.calls.every( - ([, options]) => options?.host === 'github.acme-corp.com' - ) - ).toBe(true) - }) - // 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 PR file fetch fails but leaves the PR empty otherwise intact', async () => { diff --git a/src/main/github/work-item-details.ts b/src/main/github/work-item-details.ts index 94df5d33123..c5cc9f5a479 100644 --- a/src/main/github/work-item-details.ts +++ b/src/main/github/work-item-details.ts @@ -296,6 +296,10 @@ async function getIssueTimelineItems( try { const items: GitHubIssueTimelineItem[] = [] for (let page = 1; items.length < MAX_ISSUE_TIMELINE_ITEMS; page += 1) { + if (repositoryRateLimitGuard(ownerRepo, 'core', ghOptions).blocked) { + return items + } + noteRepositoryRateLimitSpend(ownerRepo, 'core', 1, ghOptions) const { stdout } = await ghExecFileAsync( [ 'api', @@ -728,6 +732,12 @@ async function getIssueBodyAndComments( } try { if (ownerRepo) { + if (repositoryRateLimitGuard(ownerRepo, 'core', ghOptions).blocked) { + return { body: '', comments: [], assignees: [], timelineItems: [] } + } + // Why: the fallback starts the issue and comments reads together; debit + // both before spawning so a failed response cannot leave quota overstated. + noteRepositoryRateLimitSpend(ownerRepo, 'core', 2, ghOptions) const [issueResult, commentsResult, timelineItems] = await Promise.all([ ghExecFileAsync( [ @@ -1258,7 +1268,16 @@ async function fetchContentAtRef(args: { path: string ref: string }): Promise<{ content: string; isBinary: boolean; tooLarge?: boolean }> { + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(args.repoPath, args.connectionId, args.localGitOptions)), + ...githubHostExecOptions(args.ownerRepo), + maxBuffer: GITHUB_RAW_CONTENT_MAX_BUFFER_BYTES + } + if (repositoryRateLimitGuard(args.ownerRepo, 'core', ghOptions).blocked) { + return { content: '', isBinary: false } + } try { + noteRepositoryRateLimitSpend(args.ownerRepo, 'core', 1, ghOptions) const { stdout } = await ghExecFileAsync( [ 'api', @@ -1268,13 +1287,7 @@ async function fetchContentAtRef(args: { 'Accept: application/vnd.github.raw', `repos/${args.ownerRepo.owner}/${args.ownerRepo.repo}/contents/${encodeGitHubContentPath(args.path)}?ref=${encodeURIComponent(args.ref)}` ], - { - ...ghRepoExecOptions( - githubRepoContext(args.repoPath, args.connectionId, args.localGitOptions) - ), - ...githubHostExecOptions(args.ownerRepo), - maxBuffer: GITHUB_RAW_CONTENT_MAX_BUFFER_BYTES - } + ghOptions ) // Raw content response: Electron's execFile returns string in utf-8. If the // file is binary, the string will contain replacement characters — we treat diff --git a/src/renderer/src/components/github-item-dialog-source-boundary.test.ts b/src/renderer/src/components/github-item-dialog-source-boundary.test.ts index 6851fdb3a2a..f8b83e2d453 100644 --- a/src/renderer/src/components/github-item-dialog-source-boundary.test.ts +++ b/src/renderer/src/components/github-item-dialog-source-boundary.test.ts @@ -41,7 +41,7 @@ describe('GitHubItemDialog source host boundaries', () => { ) expect(section).toContain("'github.requestPRReviewers'") expect(section).toContain("'github.removePRReviewers'") - expect(section).toContain('{ repo: runtimeRepo, prNumber: item.number, reviewers: logins }') + expect(section.match(/prRepo: item\.prRepo \?\? reviewSlug/g)).toHaveLength(4) expect(section).toContain('notifyWorkItemDetailsMutation(') expect(section).toContain('{ local: false }') }) diff --git a/src/renderer/src/components/github-project/ProjectPicker.tsx b/src/renderer/src/components/github-project/ProjectPicker.tsx index b1dd25f0f28..8322a5d8134 100644 --- a/src/renderer/src/components/github-project/ProjectPicker.tsx +++ b/src/renderer/src/components/github-project/ProjectPicker.tsx @@ -3,7 +3,7 @@ // header tab strip). Pinned + Recent come from settings; Browse all lazy-loads // from `listAccessibleProjects` and is cached for 5 minutes. Paste-to-add // accepts org/user project URLs and `owner/number` shorthand. -import React, { useCallback, useEffect, useMemo, useState } from 'react' +import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { AlertTriangle, ChevronDown, Loader, Pin, Search } from 'lucide-react' import { toast } from 'sonner' import { GhAuthErrorHelp } from '@/components/github-project/GhAuthErrorHelp' @@ -61,17 +61,20 @@ type Props = { } function getProjectPickerRuntimeScope( - settings: Parameters[0] + settings: Parameters[0], + host: string ): string { const target = getActiveRuntimeTarget(settings) - return target.kind === 'environment' ? `runtime:${target.environmentId}` : 'local' + const runtimeScope = target.kind === 'environment' ? `runtime:${target.environmentId}` : 'local' + return `${runtimeScope}\0${host.toLowerCase()}` } async function listAccessibleProjectsForRuntime( - settings: Parameters[0] + settings: Parameters[0], + host: string ): Promise { const target = getActiveRuntimeTarget(settings) - const args = { host: 'github.com' } + const args = { host } return target.kind === 'environment' ? callRuntimeRpc(target, 'github.project.listAccessible', args, { timeoutMs: 60_000 @@ -79,6 +82,10 @@ async function listAccessibleProjectsForRuntime( : window.api.gh.listAccessibleProjects(args) } +export function getProjectPickerBrowseHost(activeProject: { host?: string } | null): string { + return githubProjectHost(activeProject?.host).toLowerCase() +} + async function listProjectViewsForRuntime( settings: Parameters[0], args: { @@ -131,7 +138,11 @@ export default function ProjectPicker({ activeProject, onSelect }: Props): React const [query, setQuery] = useState('') const [browseLoading, setBrowseLoading] = useState(false) const [browseError, setBrowseError] = useState(null) - const browseCache = peekProjectPickerBrowseCacheEntry(getProjectPickerRuntimeScope(settings)) + const browseHost = getProjectPickerBrowseHost(activeProject ?? projectSettings.activeProject) + const browseCacheKey = getProjectPickerRuntimeScope(settings, browseHost) + const activeBrowseCacheKeyRef = useRef(browseCacheKey) + activeBrowseCacheKeyRef.current = browseCacheKey + const browseCache = peekProjectPickerBrowseCacheEntry(browseCacheKey) const [browseProjects, setBrowseProjects] = useState( () => browseCache?.projects ?? [] ) @@ -152,46 +163,52 @@ export default function ProjectPicker({ activeProject, onSelect }: Props): React const [viewLoading, setViewLoading] = useState(false) const loadBrowse = useCallback(async () => { - const cacheKey = getProjectPickerRuntimeScope(settings) + const cacheKey = browseCacheKey const cached = getProjectPickerBrowseCacheEntry(cacheKey) if (cached) { + setBrowseLoading(false) + setBrowseError(null) setBrowseProjects(cached.projects) setPartialFailures(cached.partialFailures ?? []) return } setBrowseLoading(true) setBrowseError(null) + setBrowseProjects([]) + setPartialFailures([]) try { - const res = await listAccessibleProjectsForRuntime(settings) + const res = await listAccessibleProjectsForRuntime(settings, browseHost) if (res.ok) { rememberProjectPickerBrowseCacheEntry(cacheKey, { projects: res.projects, partialFailures: res.partialFailures }) - if (!mountedRef.current) { + // Why: runtime or active-host changes can leave the prior request in + // flight; its scoped cache is useful, but its rows must not cross hosts. + if (!mountedRef.current || activeBrowseCacheKeyRef.current !== cacheKey) { return } setBrowseProjects(res.projects) setPartialFailures(res.partialFailures ?? []) } else { - if (!mountedRef.current) { + if (!mountedRef.current || activeBrowseCacheKeyRef.current !== cacheKey) { return } setBrowseError(res.error) } } catch (err) { - if (mountedRef.current) { + if (mountedRef.current && activeBrowseCacheKeyRef.current === cacheKey) { setBrowseError({ type: 'unknown', message: err instanceof Error ? err.message : 'Failed to list projects' }) } } finally { - if (mountedRef.current) { + if (mountedRef.current && activeBrowseCacheKeyRef.current === cacheKey) { setBrowseLoading(false) } } - }, [mountedRef, settings]) + }, [browseCacheKey, browseHost, mountedRef, settings]) useEffect(() => { if (open && !viewPickFor) { @@ -449,7 +466,7 @@ export default function ProjectPicker({ activeProject, onSelect }: Props): React /> - {browseError ? : null} + {browseError ? : null} {!browseError && partialFailures.length > 0 ? ( ) : null} @@ -808,13 +825,19 @@ function PartialFailuresBanner({ ) } -function AuthErrorBanner({ error }: { error: GitHubProjectViewError }): React.JSX.Element { +function AuthErrorBanner({ + error, + host +}: { + error: GitHubProjectViewError + host: string +}): React.JSX.Element { if (error.type === 'auth_required' || error.type === 'scope_missing') { return ( ) } diff --git a/src/renderer/src/components/github-project/project-picker-input.test.ts b/src/renderer/src/components/github-project/project-picker-input.test.ts index 358f1947e90..68ae3eb89eb 100644 --- a/src/renderer/src/components/github-project/project-picker-input.test.ts +++ b/src/renderer/src/components/github-project/project-picker-input.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { parseProjectInput } from './ProjectPicker' +import { getProjectPickerBrowseHost, parseProjectInput } from './ProjectPicker' describe('ProjectPicker project input', () => { it('preserves the host and custom port from a GHES project URL', () => { @@ -17,6 +17,13 @@ describe('ProjectPicker project input', () => { expect(parseProjectInput('acme/7')).toEqual({ owner: 'acme', number: 7 }) }) + it('browses and reports auth errors against the active project host', () => { + expect(getProjectPickerBrowseHost({ host: ' GitHub.Corp.Example:8443 ' })).toBe( + 'github.corp.example:8443' + ) + expect(getProjectPickerBrowseHost(null)).toBe('github.com') + }) + it('rejects credentials and malformed Project routes', () => { for (const input of [ 'https://user:token@github.corp.example/orgs/acme/projects/7', diff --git a/src/renderer/src/components/pull-request-page-host-boundary.test.ts b/src/renderer/src/components/pull-request-page-host-boundary.test.ts index cc83a16e6f8..566ee235f25 100644 --- a/src/renderer/src/components/pull-request-page-host-boundary.test.ts +++ b/src/renderer/src/components/pull-request-page-host-boundary.test.ts @@ -36,7 +36,7 @@ describe('PullRequestPage host boundaries', () => { ) expect(section).toContain("'github.requestPRReviewers'") expect(section).toContain("'github.removePRReviewers'") - expect(section).toContain('{ repo: runtimeRepo, prNumber: item.number, reviewers: logins }') + expect(section.match(/prRepo: item\.prRepo \?\? reviewSlug/g)).toHaveLength(4) expect(section).toContain('notifyWorkItemDetailsMutation(') expect(section).toContain('{ local: false }') }) @@ -231,7 +231,9 @@ describe('PullRequestPage host boundaries', () => { expect(source).toContain('options.local !== false') expect(source).toContain('notifyWorkItemMutated({') expect(commentContextSection).toContain('sourceContext?: TaskSourceContext | null') - expect(commentContextSection).toContain('sourceContext, prNumber') + expect(commentContextSection).toMatch( + /loadPRFileContents\(\{\s*repoPath,\s*repoId,\s*sourceContext,\s*prNumber,\s*prRepo,/ + ) }) it('routes check actions through the PR source context', () => {