Support GitHub Enterprise work-item diffs with host-scoped execution

Enterprise PRs must use their selected host consistently across diff, comments,
and file-content loads. Validate repository slugs before authenticated execution
to prevent path-injection via renderer overrides. Scope project browsing cache
and rate-limit tracking by host to prevent cross-host pollution. Use parsed
URLs as authoritative over ambient hosts for project resolution.
This commit is contained in:
Jinjing
2026-07-16 22:32:05 -07:00
parent 7a2a503617
commit a7a2be03b4
16 changed files with 365 additions and 103 deletions
@@ -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<typeof GithubEnterpriseRepositoryModule>()),
isGitHubHostAuthenticated: vi.fn().mockResolvedValue(true)
}))
vi.mock('./rate-limit', () => ({
rateLimitGuard: rateLimitGuardMock,
noteRateLimitSpend: vi.fn(),
@@ -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([])
+9 -1
View File
@@ -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<MainWorkItem | null> {
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
)
+70 -8
View File
@@ -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<typeof GhUtils>()),
getOwnerRepo: getOwnerRepoMock
}))
vi.mock('./github-enterprise-repository', async (importOriginal) => ({
...(await importOriginal<typeof GitHubEnterpriseRepository>()),
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()
})
})
+28 -3
View File
@@ -29,6 +29,20 @@ type GitHubApiRepositoryResolution =
| undefined
| (() => Promise<GitHubApiRepository | null>)
// 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<GitHubApiRepository | null> {
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<GitHubRepoExecution> {
// 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: {
+6
View File
@@ -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<typeof GithubEnterpriseRepositoryModule>()),
isGitHubHostAuthenticated: vi.fn().mockResolvedValue(true)
}))
vi.mock('./github-api-repository', async (importOriginal) => {
const actual = await importOriginal<typeof GithubApiRepositoryModule>()
return {
@@ -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()
})
})
+11 -3
View File
@@ -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', () => {
+9 -5
View File
@@ -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<string, { projectV2?: { id?: string; title?: string } | null } | null>
>(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).
@@ -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()
})
})
+38 -53
View File
@@ -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 () => {
+20 -7
View File
@@ -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
@@ -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 }')
})
@@ -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<typeof getActiveRuntimeTarget>[0]
settings: Parameters<typeof getActiveRuntimeTarget>[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<typeof getActiveRuntimeTarget>[0]
settings: Parameters<typeof getActiveRuntimeTarget>[0],
host: string
): Promise<ListAccessibleProjectsResult> {
const target = getActiveRuntimeTarget(settings)
const args = { host: 'github.com' }
const args = { host }
return target.kind === 'environment'
? callRuntimeRpc<ListAccessibleProjectsResult>(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<typeof getActiveRuntimeTarget>[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<GitHubProjectViewError | null>(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<GitHubProjectSummary[]>(
() => 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
/>
</div>
</div>
{browseError ? <AuthErrorBanner error={browseError} /> : null}
{browseError ? <AuthErrorBanner error={browseError} host={browseHost} /> : null}
{!browseError && partialFailures.length > 0 ? (
<PartialFailuresBanner failures={partialFailures} />
) : 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 (
<GhAuthErrorHelp
error={error as GitHubProjectViewError & { type: 'auth_required' | 'scope_missing' }}
variant="banner"
host="github.com"
host={host}
/>
)
}
@@ -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',
@@ -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', () => {