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