From 75398ed56ee60dd726eec3774ec0b2b0a4cdca23 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 16 Jul 2026 12:00:19 -0700 Subject: [PATCH] Refactor GitHub repository execution setup --- src/main/github/client.ts | 130 +++++++----------- src/main/github/github-api-repository.test.ts | 30 +++- src/main/github/github-api-repository.ts | 40 +++++- src/main/github/issues.ts | 91 ++++++++---- 4 files changed, 181 insertions(+), 110 deletions(-) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 23952a70d2e..cc43aec30b9 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -81,9 +81,10 @@ import { githubHostExecOptions, githubRepositorySlugArg, githubRepositoryWebHost, - resolveGitHubApiRepository, resolveGitHubApiRepositoryCandidates, + resolveGitHubRepoExecution, resolveIssueGitHubApiRepositorySource, + type GitHubRepoExecOptions, type GitHubApiRepository } from './github-api-repository' import { githubRepoIdentityKey } from '../../shared/github-repository-identity-key' @@ -116,7 +117,7 @@ import { type RateLimitBucketKind } from './rate-limit' -type GhExecOptions = ReturnType & { host?: string } +type GhExecOptions = GitHubRepoExecOptions type HostedReviewLocalGitOptions = ReturnType const ORCA_REPO = 'stablyai/orca' @@ -1742,7 +1743,12 @@ export async function getRepoUpstream( ): Promise { const localGitArgs = hostedReviewLocalGitOptionArgs(options) const localGitOptions = localGitArgs[0] ?? {} - const origin = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) + const { ownerRepo: origin, ghOptions } = await resolveGitHubRepoExecution( + repoPath, + undefined, + connectionId, + localGitOptions + ) if (!origin) { return null } @@ -1764,8 +1770,7 @@ export async function getRepoUpstream( // Why: best-effort fork lookup runs at add-time; cap latency so a stalled // gh process can't hold up repo creation. { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(origin), + ...ghOptions, timeout: 10_000 } ) @@ -3691,16 +3696,12 @@ export async function getPRChecks( localGitOptions: LocalGitExecOptions = {} ): Promise { void headSha - const ownerRepo = await resolveGitHubApiRepository( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, prRepo, connectionId, localGitOptions ) - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } if (connectionId && !ownerRepo) { throw new Error(GITHUB_WORK_ITEMS_SSH_REMOTE_REQUIRED_MESSAGE) } @@ -3976,7 +3977,7 @@ export async function getPRCheckDetails( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ownerRepo = await resolveGitHubApiRepository( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, args.prRepo, connectionId, @@ -3985,10 +3986,6 @@ export async function getPRCheckDetails( if (!ownerRepo) { return null } - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } await acquire() try { @@ -4077,14 +4074,15 @@ export async function rerunPRChecks( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ownerRepo = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( + repoPath, + undefined, + connectionId, + localGitOptions + ) if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } const checks = await getPRChecks( repoPath, @@ -4220,16 +4218,12 @@ export async function getPRComments( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ownerRepo = await resolveGitHubApiRepository( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, options?.prRepo, connectionId, localGitOptions ) - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } if (connectionId && !ownerRepo) { throw new Error(GITHUB_WORK_ITEMS_SSH_REMOTE_REQUIRED_MESSAGE) } @@ -4474,18 +4468,15 @@ export async function setPRFileViewed(args: { path: string viewed: boolean }): Promise { - const ownerRepo = await getOriginGitHubApiRepository( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( args.repoPath, + undefined, args.connectionId, args.localGitOptions ) if (args.connectionId && !ownerRepo) { return false } - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(args.repoPath, args.connectionId, args.localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } const mutation = args.viewed ? 'markFileAsViewed' : 'unmarkFileAsViewed' const query = `mutation($pullRequestId: ID!, $path: String!) { ${mutation}(input: { pullRequestId: $pullRequestId, path: $path }) { @@ -4528,14 +4519,15 @@ export async function resolveReviewThread( ): Promise { const mutation = resolve ? 'resolveReviewThread' : 'unresolveReviewThread' const query = `mutation($threadId: ID!) { ${mutation}(input: { threadId: $threadId }) { thread { isResolved } } }` - const ownerRepo = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( + repoPath, + undefined, + connectionId, + localGitOptions + ) if (connectionId && !ownerRepo) { return false } - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } const guard = repositoryRateLimitGuard(ownerRepo, 'graphql', ghOptions) if (guard.blocked) { console.warn( @@ -4602,7 +4594,7 @@ export async function addPRReviewCommentReply( prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ownerRepo = await resolveGitHubApiRepository( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, prRepo, connectionId, @@ -4611,10 +4603,6 @@ export async function addPRReviewCommentReply( if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } await acquire() try { const { stdout } = await ghExecFileAsync( @@ -4650,18 +4638,15 @@ export async function addPRReviewComment( localGitOptions?: LocalGitExecOptions } ): Promise { - const ownerRepo = await getOriginGitHubApiRepository( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( args.repoPath, + undefined, args.connectionId, args.localGitOptions ) if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(args.repoPath, args.connectionId, args.localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } await acquire() try { const fields = [ @@ -4719,7 +4704,7 @@ export async function mergePR( prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ ok: true } | { ok: false; error: string }> { - const ownerRepo = await resolveGitHubApiRepository( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, prRepo, connectionId, @@ -4728,10 +4713,6 @@ export async function mergePR( if (connectionId && !ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } await acquire() try { const mergeBlocker = await getPRMergeBlocker( @@ -4776,7 +4757,7 @@ export async function setPRAutoMerge( prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ ok: true } | { ok: false; error: string }> { - const ownerRepo = await resolveGitHubApiRepository( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, prRepo, connectionId, @@ -4785,10 +4766,6 @@ export async function setPRAutoMerge( if (connectionId && !ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } await acquire() try { if (enabled) { @@ -4993,12 +4970,15 @@ export async function updatePRState( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ ok: true } | { ok: false; error: string }> { - const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ownerRepo = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( + repoPath, + undefined, + connectionId, + localGitOptions + ) if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } - const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } await acquire() try { @@ -5032,14 +5012,15 @@ export async function requestPRReviewers( if (logins.length === 0) { return { ok: false, error: 'Enter at least one reviewer' } } - const ownerRepo = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( + repoPath, + undefined, + connectionId, + localGitOptions + ) if (connectionId && !ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } await acquire() try { const args = ['pr', 'edit', String(prNumber), '--add-reviewer', logins.join(',')] @@ -5071,14 +5052,15 @@ export async function removePRReviewers( if (logins.length === 0) { return { ok: false, error: 'Enter at least one reviewer' } } - const ownerRepo = await getOriginGitHubApiRepository(repoPath, connectionId, localGitOptions) + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( + repoPath, + undefined, + connectionId, + localGitOptions + ) if (connectionId && !ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } await acquire() try { const args = ['pr', 'edit', String(prNumber), '--remove-reviewer', logins.join(',')] @@ -5110,7 +5092,7 @@ export async function updatePRTitle( prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const ownerRepo = await resolveGitHubApiRepository( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, prRepo, connectionId, @@ -5119,10 +5101,6 @@ export async function updatePRTitle( if (connectionId && !ownerRepo) { return false } - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } await acquire() try { const args = ['pr', 'edit', String(prNumber), '--title', title] @@ -5149,7 +5127,7 @@ export async function updatePRDetails( prRepo?: GitHubApiRepository | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ ok: true } | { ok: false; error: string }> { - const ownerRepo = await resolveGitHubApiRepository( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, prRepo, connectionId, @@ -5158,10 +5136,6 @@ export async function updatePRDetails( if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } const fields: string[] = [] if (updates.title !== undefined) { diff --git a/src/main/github/github-api-repository.test.ts b/src/main/github/github-api-repository.test.ts index b63ee1530e6..57c94259b0c 100644 --- a/src/main/github/github-api-repository.test.ts +++ b/src/main/github/github-api-repository.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest' -import { githubHostExecOptions } from './github-api-repository' +import { githubHostExecOptions, resolveGitHubRepoExecution } from './github-api-repository' describe('githubHostExecOptions', () => { it('pins every known repository host', () => { @@ -17,3 +17,31 @@ describe('githubHostExecOptions', () => { expect(githubHostExecOptions(null)).toEqual({}) }) }) + +describe('resolveGitHubRepoExecution', () => { + it('combines local repository and GitHub host execution options', async () => { + const ownerRepo = { owner: 'acme', repo: 'widgets', host: 'github.acme-corp.com' } + + await expect( + resolveGitHubRepoExecution('/repo', ownerRepo, null, { wslDistro: 'Ubuntu' }) + ).resolves.toEqual({ + ownerRepo, + ghOptions: { + cwd: '/repo', + wslDistro: 'Ubuntu', + host: 'github.acme-corp.com' + } + }) + }) + + it('uses a caller-specific repository resolver without changing its identity', async () => { + const ownerRepo = { owner: 'upstream', repo: 'widgets' } + + await expect( + resolveGitHubRepoExecution('/remote/repo', async () => ownerRepo, 'ssh-1') + ).resolves.toEqual({ + ownerRepo, + ghOptions: {} + }) + }) +}) diff --git a/src/main/github/github-api-repository.ts b/src/main/github/github-api-repository.ts index 875fb778714..33c281b0d5c 100644 --- a/src/main/github/github-api-repository.ts +++ b/src/main/github/github-api-repository.ts @@ -3,13 +3,30 @@ import { githubRepoIdentityKey, isDefaultGitHubHost } from '../../shared/github-repository-identity-key' -import { getOwnerRepo, getOwnerRepoForRemote, type LocalGitExecOptions } from './gh-utils' +import { + getOwnerRepo, + getOwnerRepoForRemote, + ghRepoExecOptions, + githubRepoContext, + type LocalGitExecOptions +} from './gh-utils' import { getEnterpriseGitHubRepoSlug, getEnterpriseGitHubRepoSlugForRemote } from './github-enterprise-repository' export type GitHubApiRepository = GitHubOwnerRepo +export type GitHubRepoExecOptions = ReturnType & { host?: string } +export type GitHubRepoExecution = { + ownerRepo: GitHubApiRepository | null + ghOptions: GitHubRepoExecOptions +} + +type GitHubApiRepositoryResolution = + | GitHubApiRepository + | null + | undefined + | (() => Promise) // 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 @@ -256,6 +273,27 @@ export function githubHostExecOptions(repository: GitHubApiRepository | null | u return repository?.host ? { host: repository.host } : {} } +export async function resolveGitHubRepoExecution( + repoPath: string, + repository?: GitHubApiRepositoryResolution, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): Promise { + // Why: issue-scoped paths retain their upstream-first resolver while sharing + // the same repo-scoped and host-scoped gh execution option construction. + const ownerRepo = + typeof repository === 'function' + ? await repository() + : await resolveGitHubApiRepository(repoPath, repository, connectionId, localGitOptions) + return { + ownerRepo, + ghOptions: { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } + } +} + export function githubRepositoryWebHost(repository: GitHubApiRepository): string { return repository.host ?? 'github.com' } diff --git a/src/main/github/issues.ts b/src/main/github/issues.ts index 15219994859..44b29625105 100644 --- a/src/main/github/issues.ts +++ b/src/main/github/issues.ts @@ -17,12 +17,11 @@ import { mapIssueInfo } from './mappers' import type { LocalGitExecOptions, OwnerRepo } from './gh-utils' import { getIssueGitHubApiRepository, - githubHostExecOptions, - resolveGitHubApiRepository, + resolveGitHubRepoExecution, resolveIssueGitHubApiRepositorySource } from './github-api-repository' // prettier-ignore -import { ghExecFileAsync, acquire, release, classifyGhError, classifyListIssuesError, ghRepoExecOptions, githubRepoContext, extractExecError } from './gh-utils' +import { ghExecFileAsync, acquire, release, classifyGhError, classifyListIssuesError, extractExecError } from './gh-utils' // Why: distinguishes a successful-empty listing from a failed fetch. The // previous `catch { return [] }` conflated a 403 on a private upstream with an @@ -60,9 +59,12 @@ export async function getIssue( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ownerRepo = await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions) - const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( + repoPath, + () => getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions), + connectionId, + localGitOptions + ) await acquire() try { if (ownerRepo) { @@ -110,14 +112,20 @@ export async function listIssues( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const { source: ownerRepo } = await resolveIssueGitHubApiRepositorySource( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, - preference, + async () => + ( + await resolveIssueGitHubApiRepositorySource( + repoPath, + preference, + connectionId, + localGitOptions + ) + ).source, connectionId, localGitOptions ) - const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } await acquire() try { if (ownerRepo) { @@ -179,14 +187,20 @@ export async function createIssue( if (!trimmedTitle) { return { ok: false, error: 'Title is required' } } - const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const { source: ownerRepo } = await resolveIssueGitHubApiRepositorySource( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, - preference, + async () => + ( + await resolveIssueGitHubApiRepositorySource( + repoPath, + preference, + connectionId, + localGitOptions + ) + ).source, connectionId, localGitOptions ) - const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } @@ -292,12 +306,15 @@ export async function updateIssue( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise<{ ok: true } | { ok: false; error: string }> { - const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ownerRepo = await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions) + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( + repoPath, + () => getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions), + connectionId, + localGitOptions + ) if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } - const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } const repo = `${ownerRepo.owner}/${ownerRepo.repo}` const errors: string[] = [] @@ -415,14 +432,16 @@ export async function addIssueComment( ownerRepoOverride?: OwnerRepo | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ownerRepo = ownerRepoOverride - ? await resolveGitHubApiRepository(repoPath, ownerRepoOverride, connectionId, localGitOptions) - : await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions) + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( + repoPath, + ownerRepoOverride ?? + (() => getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions)), + connectionId, + localGitOptions + ) if (!ownerRepo) { return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' } } - const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } await acquire() try { const { stdout } = await ghExecFileAsync( @@ -470,14 +489,20 @@ export async function listLabels( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const { source: ownerRepo } = await resolveIssueGitHubApiRepositorySource( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, - preference, + async () => + ( + await resolveIssueGitHubApiRepositorySource( + repoPath, + preference, + connectionId, + localGitOptions + ) + ).source, connectionId, localGitOptions ) - const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } if (!ownerRepo) { return [] } @@ -510,14 +535,20 @@ export async function listAssignableUsers( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const { source: ownerRepo } = await resolveIssueGitHubApiRepositorySource( + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( repoPath, - preference, + async () => + ( + await resolveIssueGitHubApiRepositorySource( + repoPath, + preference, + connectionId, + localGitOptions + ) + ).source, connectionId, localGitOptions ) - const ghOptions = { ...ghRepoExecOptions(context), ...githubHostExecOptions(ownerRepo) } if (!ownerRepo) { return [] }