diff --git a/src/main/github/client-create-pr.test.ts b/src/main/github/client-create-pr.test.ts index 9e2f41c7dc1..1e9900376e1 100644 --- a/src/main/github/client-create-pr.test.ts +++ b/src/main/github/client-create-pr.test.ts @@ -30,12 +30,16 @@ vi.mock('./gh-utils', () => ({ getOwnerRepo: getOwnerRepoMock, getIssueOwnerRepo: vi.fn(), getOwnerRepoForRemote: getOwnerRepoForRemoteMock, - githubRepoContext: vi.fn((repoPath: string, connectionId?: string | null) => ({ + githubRepoContext: vi.fn((repoPath: string, connectionId?: string | null, options = {}) => ({ + ...options, repoPath, connectionId: connectionId ?? null })), - ghRepoExecOptions: vi.fn((context: { repoPath: string; connectionId?: string | null }) => - context.connectionId ? {} : { cwd: context.repoPath } + ghRepoExecOptions: vi.fn( + (context: { repoPath: string; connectionId?: string | null; wslDistro?: string }) => + context.connectionId + ? {} + : { cwd: context.repoPath, ...(context.wslDistro ? { wslDistro: context.wslDistro } : {}) } ), gitExecFileAsync: vi.fn(), extractExecError: extractExecErrorMock, diff --git a/src/main/github/client/check/get-pr-checks.ts b/src/main/github/client/check/get-pr-checks.ts index 2c04bc0b828..5862c1ab566 100644 --- a/src/main/github/client/check/get-pr-checks.ts +++ b/src/main/github/client/check/get-pr-checks.ts @@ -1,3 +1,8 @@ +import { + withGitHubRepoExecution, + githubRepoContext, + ghRepoExecOptions +} from '../../github-repository-identity' import { runCoalescedProbe, type CoalescedProbes } from '../../../git/coalesced-probe' import { githubReadExecutionScope } from '../../github-read-execution-scope' import type { PRCheckDetail } from '../../../../shared/github/check-types' @@ -241,16 +246,20 @@ export function getPRChecks( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const key = JSON.stringify([ - repoPath, - prNumber, - headSha ?? null, - prRepo ?? null, - Boolean(options?.noCache), - connectionId ?? null, - githubReadExecutionScope(localGitOptions) - ]) - return runCoalescedProbe(checksReads, key, () => - readPRChecks(repoPath, prNumber, headSha, prRepo, options, connectionId, localGitOptions) - ) + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + const key = JSON.stringify([ + repoPath, + prNumber, + headSha ?? null, + prRepo ?? null, + Boolean(options?.noCache), + connectionId ?? null, + githubReadExecutionScope( + ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) + ) + ]) + return runCoalescedProbe(checksReads, key, () => + readPRChecks(repoPath, prNumber, headSha, prRepo, options, connectionId, localGitOptions) + ) + }) } diff --git a/src/main/github/client/check/rerun-pr-checks.ts b/src/main/github/client/check/rerun-pr-checks.ts index 39298f3f6f6..211e4ff1c1d 100644 --- a/src/main/github/client/check/rerun-pr-checks.ts +++ b/src/main/github/client/check/rerun-pr-checks.ts @@ -1,3 +1,4 @@ +import { withGitHubRepoExecution } from '../../github-repository-identity' import type { GitHubRerunPRChecksResult } from '../../../../shared/github/check-types' import { ghExecFileAsync, acquire, release, type LocalGitExecOptions } from '../../gh-utils' // Keep real error parsing available when tests mock gh-utils. @@ -18,85 +19,87 @@ export async function rerunPRChecks( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( - repoPath, - options.prRepo, - connectionId, - localGitOptions - ) - if (!ownerRepo) { - return codeHostFailure('github.repoUnresolved') - } - - const checks = await getPRChecks( - repoPath, - prNumber, - options.headSha, - ownerRepo, - { noCache: true }, - connectionId, - localGitOptions - ) - const candidates = options.failedOnly - ? checks.filter((check) => - ['failure', 'cancelled', 'timed_out'].includes(check.conclusion ?? '') - ) - : checks - const workflowRunIds = new Set( - candidates - .map((check) => check.workflowRunId ?? parseActionsRunId(check.url)) - .filter((id): id is number => typeof id === 'number') - ) - const checkRunIds = new Set( - candidates - .filter((check) => !check.workflowRunId && !parseActionsRunId(check.url)) - .map((check) => check.checkRunId) - .filter((id): id is number => typeof id === 'number') - ) - - if (workflowRunIds.size === 0 && checkRunIds.size === 0) { - return codeHostFailure( - options.failedOnly ? 'github.noFailedChecks' : 'github.noRerunnableChecks' + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( + repoPath, + options.prRepo, + connectionId, + localGitOptions ) - } + if (!ownerRepo) { + return codeHostFailure('github.repoUnresolved') + } - let count = 0 - await acquire() - try { - for (const runId of workflowRunIds) { - const endpoint = options.failedOnly - ? `repos/${ownerRepo.owner}/${ownerRepo.repo}/actions/runs/${runId}/rerun-failed-jobs` - : `repos/${ownerRepo.owner}/${ownerRepo.repo}/actions/runs/${runId}/rerun` - await ghExecFileAsync(['api', '-X', 'POST', endpoint], { - ...ghOptions, - env: { ...process.env, GH_PROMPT_DISABLED: '1' } - }) - count += 1 - } - for (const checkRunId of checkRunIds) { - await ghExecFileAsync( - [ - 'api', - '-X', - 'POST', - `repos/${ownerRepo.owner}/${ownerRepo.repo}/check-runs/${checkRunId}/rerequest` - ], - { ...ghOptions, env: { ...process.env, GH_PROMPT_DISABLED: '1' } } + const checks = await getPRChecks( + repoPath, + prNumber, + options.headSha, + ownerRepo, + { noCache: true }, + connectionId, + localGitOptions + ) + const candidates = options.failedOnly + ? checks.filter((check) => + ['failure', 'cancelled', 'timed_out'].includes(check.conclusion ?? '') + ) + : checks + const workflowRunIds = new Set( + candidates + .map((check) => check.workflowRunId ?? parseActionsRunId(check.url)) + .filter((id): id is number => typeof id === 'number') + ) + const checkRunIds = new Set( + candidates + .filter((check) => !check.workflowRunId && !parseActionsRunId(check.url)) + .map((check) => check.checkRunId) + .filter((id): id is number => typeof id === 'number') + ) + + if (workflowRunIds.size === 0 && checkRunIds.size === 0) { + return codeHostFailure( + options.failedOnly ? 'github.noFailedChecks' : 'github.noRerunnableChecks' ) - count += 1 } - return { ok: true, count } - } catch (err) { - const { stderr } = extractExecError(err) - const classified = classifyRerunChecksError(stderr) - // Rerun POSTs are not idempotent; report partial success before the user retries. - return count > 0 - ? { - ok: false, - error: `${classified.message} (${count} rerun${count === 1 ? '' : 's'} already started)` - } - : classifiedFailure(classified) - } finally { - release() - } + + let count = 0 + await acquire() + try { + for (const runId of workflowRunIds) { + const endpoint = options.failedOnly + ? `repos/${ownerRepo.owner}/${ownerRepo.repo}/actions/runs/${runId}/rerun-failed-jobs` + : `repos/${ownerRepo.owner}/${ownerRepo.repo}/actions/runs/${runId}/rerun` + await ghExecFileAsync(['api', '-X', 'POST', endpoint], { + ...ghOptions, + env: { ...process.env, GH_PROMPT_DISABLED: '1' } + }) + count += 1 + } + for (const checkRunId of checkRunIds) { + await ghExecFileAsync( + [ + 'api', + '-X', + 'POST', + `repos/${ownerRepo.owner}/${ownerRepo.repo}/check-runs/${checkRunId}/rerequest` + ], + { ...ghOptions, env: { ...process.env, GH_PROMPT_DISABLED: '1' } } + ) + count += 1 + } + return { ok: true, count } + } catch (err) { + const { stderr } = extractExecError(err) + const classified = classifyRerunChecksError(stderr) + // Rerun POSTs are not idempotent; report partial success before the user retries. + return count > 0 + ? { + ok: false, + error: `${classified.message} (${count} rerun${count === 1 ? '' : 's'} already started)` + } + : classifiedFailure(classified) + } finally { + release() + } + }) } diff --git a/src/main/github/client/create/create-github-pull-request.ts b/src/main/github/client/create/create-github-pull-request.ts index 40728c330ed..0d769ac55b8 100644 --- a/src/main/github/client/create/create-github-pull-request.ts +++ b/src/main/github/client/create/create-github-pull-request.ts @@ -25,6 +25,7 @@ import { import { getOriginGitHubApiRepository, githubHostExecOptions } from '../../github-api-repository' import { classifyCreatePRError, parseCreatePRPayload } from './create-pr-error-classification' import { findOpenPRByHeadBase, readPullRequestTemplate } from './pull-request-template' +import { withGitHubRepoExecution } from '../../github-repository-identity' export async function createGitHubPullRequest( repoPath: string, input: CreateHostedReviewInput, @@ -43,131 +44,137 @@ export async function createGitHubPullRequest( // `gh` runs on this client whatever the host; only the git reads under it are routed. const connectionId = hostedReviewSshConnectionId(executionHostId) - // The unqualified head branch belongs to origin; retain its resolved host. - const ownerRepo = await getOriginGitHubApiRepository( + return withGitHubRepoExecution( repoPath, connectionId, - getHostedReviewLocalGitOptions(options) - ) - if (!ownerRepo) { - return { - ok: false, - code: 'unsupported_provider', - error: 'Creating pull requests requires a GitHub remote.', - reason: 'unsupported_provider' - } - } - // The runner qualifies --repo using options.host. - const repoArg = `${ownerRepo.owner}/${ownerRepo.repo}` - - const base = normalizeHostedReviewBaseRef(input.base) - const head = input.head ? normalizeHostedReviewHeadRef(input.head) || undefined : undefined - const title = input.title.trim() - if (!base || !title) { - return { - ok: false, - code: 'validation', - error: 'Create PR failed: base branch and title are required.' - } - } - if (head && head.toLowerCase() === base.toLowerCase()) { - return { - ok: false, - code: 'validation', - error: 'Create PR failed: choose a different base branch before creating a pull request.', - reason: 'same_base_branch' - } - } - - const tempDir = await mkdtemp(join(tmpdir(), 'orca-pr-body-')) - await acquire() - const bodyPath = join(tempDir, 'body.md') - try { - const body = - input.useTemplate && !input.body?.trim() - ? await readPullRequestTemplate(repoPath, connectionId) - : (input.body ?? '') - await writeFile(bodyPath, body, 'utf8') - const createArgs = [ - 'pr', - 'create', - '--repo', - repoArg, - '--base', - base, - '--title', - title, - '--body-file', - bodyPath - ] - if (head) { - createArgs.push('--head', head) - } - if (input.draft) { - createArgs.push('--draft') - } - try { - const localGitOptions = getHostedReviewLocalGitOptions(options) - const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const { stdout } = await ghExecFileAsync(createArgs, { - ...ghRepoExecOptions(context), - ...(connectionId ? {} : localGitOptions), - ...githubHostExecOptions(ownerRepo), - timeout: 60_000, - idempotent: false - }) - const created = parseCreatePRPayload(stdout) - if (created) { - return { ok: true, ...created } - } - const found = head - ? await findOpenPRByHeadBase({ - repoPath, - repo: ownerRepo, - head, - base, - connectionId, - options - }).catch(() => null) - : null - if (found) { - return { ok: true, ...found } - } - return { - ok: false, - code: 'unknown_completion', - error: 'PR creation may have completed. Refreshing branch review state...', - reason: 'creation_unconfirmed' - } - } catch (error) { - const classified = classifyCreatePRError(error) - if ( - !classified.ok && - (classified.code === 'already_exists' || classified.code === 'unknown_completion') && - head - ) { - const existing = await findOpenPRByHeadBase({ - repoPath, - repo: ownerRepo, - head, - base, - connectionId, - options - }).catch(() => null) - if (existing) { - return { - ok: false, - code: 'already_exists', - error: 'A pull request already exists for this branch.', - reason: 'existing_review', - existingReview: existing - } + getHostedReviewLocalGitOptions(options), + async () => { + // The unqualified head branch belongs to origin; retain its resolved host. + const ownerRepo = await getOriginGitHubApiRepository( + repoPath, + connectionId, + getHostedReviewLocalGitOptions(options) + ) + if (!ownerRepo) { + return { + ok: false, + code: 'unsupported_provider', + error: 'Creating pull requests requires a GitHub remote.', + reason: 'unsupported_provider' } } - return classified + // The runner qualifies --repo using options.host. + const repoArg = `${ownerRepo.owner}/${ownerRepo.repo}` + + const base = normalizeHostedReviewBaseRef(input.base) + const head = input.head ? normalizeHostedReviewHeadRef(input.head) || undefined : undefined + const title = input.title.trim() + if (!base || !title) { + return { + ok: false, + code: 'validation', + error: 'Create PR failed: base branch and title are required.' + } + } + if (head && head.toLowerCase() === base.toLowerCase()) { + return { + ok: false, + code: 'validation', + error: 'Create PR failed: choose a different base branch before creating a pull request.', + reason: 'same_base_branch' + } + } + + const tempDir = await mkdtemp(join(tmpdir(), 'orca-pr-body-')) + await acquire() + const bodyPath = join(tempDir, 'body.md') + try { + const body = + input.useTemplate && !input.body?.trim() + ? await readPullRequestTemplate(repoPath, connectionId) + : (input.body ?? '') + await writeFile(bodyPath, body, 'utf8') + const createArgs = [ + 'pr', + 'create', + '--repo', + repoArg, + '--base', + base, + '--title', + title, + '--body-file', + bodyPath + ] + if (head) { + createArgs.push('--head', head) + } + if (input.draft) { + createArgs.push('--draft') + } + try { + const localGitOptions = getHostedReviewLocalGitOptions(options) + const context = githubRepoContext(repoPath, connectionId, localGitOptions) + const { stdout } = await ghExecFileAsync(createArgs, { + ...ghRepoExecOptions(context), + ...githubHostExecOptions(ownerRepo), + timeout: 60_000, + idempotent: false + }) + const created = parseCreatePRPayload(stdout) + if (created) { + return { ok: true, ...created } + } + const found = head + ? await findOpenPRByHeadBase({ + repoPath, + repo: ownerRepo, + head, + base, + connectionId, + options + }).catch(() => null) + : null + if (found) { + return { ok: true, ...found } + } + return { + ok: false, + code: 'unknown_completion', + error: 'PR creation may have completed. Refreshing branch review state...', + reason: 'creation_unconfirmed' + } + } catch (error) { + const classified = classifyCreatePRError(error) + if ( + !classified.ok && + (classified.code === 'already_exists' || classified.code === 'unknown_completion') && + head + ) { + const existing = await findOpenPRByHeadBase({ + repoPath, + repo: ownerRepo, + head, + base, + connectionId, + options + }).catch(() => null) + if (existing) { + return { + ok: false, + code: 'already_exists', + error: 'A pull request already exists for this branch.', + reason: 'existing_review', + existingReview: existing + } + } + } + return classified + } + } finally { + await rm(tempDir, { recursive: true, force: true }).catch(() => undefined) + release() + } } - } finally { - await rm(tempDir, { recursive: true, force: true }).catch(() => undefined) - release() - } + ) } diff --git a/src/main/github/client/create/pull-request-template.ts b/src/main/github/client/create/pull-request-template.ts index 5ceceb74372..e1659791c74 100644 --- a/src/main/github/client/create/pull-request-template.ts +++ b/src/main/github/client/create/pull-request-template.ts @@ -35,7 +35,6 @@ export async function findOpenPRByHeadBase(args: { ], { ...ghRepoExecOptions(context), - ...(args.connectionId ? {} : localGitOptions), ...githubHostExecOptions(args.repo) } ) diff --git a/src/main/github/client/detect/repository-merge-metadata.ts b/src/main/github/client/detect/repository-merge-metadata.ts index e984865d919..067afa7bcab 100644 --- a/src/main/github/client/detect/repository-merge-metadata.ts +++ b/src/main/github/client/detect/repository-merge-metadata.ts @@ -154,10 +154,7 @@ async function readRepositoryMergeMetadata( args.push('-f', `branch=${branchName}`) args.push('-f', `qualified=refs/heads/${branchName}`) } - const { stdout } = await ghExecFileAsync(args, { - ...ghOptions, - ...githubHostExecOptions(ownerRepo) - }) + const { stdout } = await ghExecFileAsync(args, ghOptions) const parsed = JSON.parse(stdout) as { data?: { repository?: { diff --git a/src/main/github/client/fetch/get-work-item.ts b/src/main/github/client/fetch/get-work-item.ts index 5507b6fe7e4..b9baf15607d 100644 --- a/src/main/github/client/fetch/get-work-item.ts +++ b/src/main/github/client/fetch/get-work-item.ts @@ -1,3 +1,4 @@ +import { withGitHubRepoExecution } from '../../github-repository-identity' import type { IssueSourcePreference } from '../../../../shared/repo-types' import { acquire, release, classifyGhError, type LocalGitExecOptions } from '../../gh-utils' import { @@ -52,40 +53,79 @@ export async function getWorkItemWithRepository( preference?: IssueSourcePreference, includeIssueDetails = false ): Promise { - await acquire() - try { - // Number-based issue lookup must honor the same selected source as its list. - if (type === 'issue') { - const { source } = await resolveIssueGitHubApiRepositorySource( - repoPath, - preference, - connectionId, - localGitOptions - ) - // Explicit origin cannot fall back to ambient gh repository discovery. - if (!source && preference === 'origin') { - return { item: null, repository: null } + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + await acquire() + try { + // Number-based issue lookup must honor the same selected source as its list. + if (type === 'issue') { + const { source } = await resolveIssueGitHubApiRepositorySource( + repoPath, + preference, + connectionId, + localGitOptions + ) + // Explicit origin cannot fall back to ambient gh repository discovery. + if (!source && preference === 'origin') { + return { item: null, repository: null } + } + const issueDetails = + includeIssueDetails && source + ? await getIssueDetailsViaGraphQL( + repoPath, + number, + source, + connectionId, + localGitOptions, + true + ) + : undefined + return { + item: + issueDetails?.item ?? + (await fetchIssueWorkItem(repoPath, source, number, connectionId, localGitOptions)), + repository: source, + ...(includeIssueDetails ? { issueDetails } : {}) + } } - const issueDetails = - includeIssueDetails && source - ? await getIssueDetailsViaGraphQL( - repoPath, - number, - source, - connectionId, - localGitOptions, - true - ) - : undefined - return { - item: - issueDetails?.item ?? - (await fetchIssueWorkItem(repoPath, source, number, connectionId, localGitOptions)), - repository: source, - ...(includeIssueDetails ? { issueDetails } : {}) + if (type === 'pr') { + return { + item: await fetchPullRequestWorkItemFromCandidates( + repoPath, + number, + connectionId, + localGitOptions, + preference + ), + repository: null + } + } + + try { + const { source } = await resolveIssueGitHubApiRepositorySource( + repoPath, + preference, + connectionId, + localGitOptions + ) + if (source || preference !== 'origin') { + const issue = await fetchIssueWorkItem( + repoPath, + source, + number, + connectionId, + localGitOptions + ) + if (issue) { + return { item: issue, repository: source } + } + } + } catch (err) { + // Only genuine issue 404s permit trying a same-number PR; preserve transient failures. + const stderr = err instanceof Error ? err.message : String(err) + if (classifyGhError(stderr).type !== 'not_found') { + throw err + } } - } - if (type === 'pr') { return { item: await fetchPullRequestWorkItemFromCandidates( repoPath, @@ -96,49 +136,12 @@ export async function getWorkItemWithRepository( ), repository: null } + } catch { + return { item: null, repository: null } + } finally { + release() } - - try { - const { source } = await resolveIssueGitHubApiRepositorySource( - repoPath, - preference, - connectionId, - localGitOptions - ) - if (source || preference !== 'origin') { - const issue = await fetchIssueWorkItem( - repoPath, - source, - number, - connectionId, - localGitOptions - ) - if (issue) { - return { item: issue, repository: source } - } - } - } catch (err) { - // Only genuine issue 404s permit trying a same-number PR; preserve transient failures. - const stderr = err instanceof Error ? err.message : String(err) - if (classifyGhError(stderr).type !== 'not_found') { - throw err - } - } - return { - item: await fetchPullRequestWorkItemFromCandidates( - repoPath, - number, - connectionId, - localGitOptions, - preference - ), - repository: null - } - } catch { - return { item: null, repository: null } - } finally { - release() - } + }) } export async function getWorkItemByOwnerRepo( @@ -150,57 +153,59 @@ export async function getWorkItemByOwnerRepo( localGitOptions: LocalGitExecOptions = {}, options: { preview?: boolean } = {} ): 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(requestedRepository) - const matchedRepository = candidates.find( - (candidate) => githubRepoIdentityKey(candidate) === requestedKey - ) - // Pasted hosts must match configured remotes before receiving credentials. - if (!matchedRepository) { - return null - } - await acquire() - try { - if (options.preview) { - return await fetchWorkItemPreview( - repoPath, - matchedRepository, - number, - type, - connectionId, - localGitOptions - ) + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + const requestedHost = ownerRepo.host?.trim().toLowerCase() + const requestedRepository = requestedHost + ? { ...ownerRepo, host: requestedHost } + : await resolveGitHubApiRepository(repoPath, ownerRepo, connectionId, localGitOptions) + if (!requestedRepository) { + return null } - if (type === 'issue') { - return await fetchIssueWorkItem( - repoPath, - matchedRepository, - number, - connectionId, - localGitOptions - ) - } - return await fetchPullRequestWorkItem( + const { candidates } = await resolveGitHubApiRepositoryCandidates( repoPath, - matchedRepository, - number, connectionId, localGitOptions ) - } catch { - return null - } finally { - release() - } + const requestedKey = githubRepoIdentityKey(requestedRepository) + const matchedRepository = candidates.find( + (candidate) => githubRepoIdentityKey(candidate) === requestedKey + ) + // Pasted hosts must match configured remotes before receiving credentials. + if (!matchedRepository) { + return null + } + await acquire() + try { + if (options.preview) { + return await fetchWorkItemPreview( + repoPath, + matchedRepository, + number, + type, + connectionId, + localGitOptions + ) + } + if (type === 'issue') { + return await fetchIssueWorkItem( + repoPath, + matchedRepository, + number, + connectionId, + localGitOptions + ) + } + return await fetchPullRequestWorkItem( + repoPath, + matchedRepository, + number, + connectionId, + localGitOptions + ) + } catch { + return null + } finally { + release() + } + }) } diff --git a/src/main/github/client/list/count-work-items.ts b/src/main/github/client/list/count-work-items.ts index 98f57cf2dbe..0c0f9b7532d 100644 --- a/src/main/github/client/list/count-work-items.ts +++ b/src/main/github/client/list/count-work-items.ts @@ -1,3 +1,4 @@ +import { withGitHubRepoExecution } from '../../github-repository-identity' import type { IssueSourcePreference } from '../../../../shared/repo-types' import { isGitHubWorkItemsQueryTooLarge } from '../../../../shared/github/work-items-query-bounds' import { parseTaskQuery, type ParsedTaskQuery } from '../../../../shared/task-query' @@ -114,99 +115,101 @@ export async function countWorkItems( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const trimmedQuery = query?.trim() ?? '' - if (isGitHubWorkItemsQueryTooLarge(trimmedQuery)) { - return 0 - } - const [issueResolved, prResolved] = await Promise.all([ - resolveIssueGitHubApiRepositorySource(repoPath, preference, connectionId, localGitOptions), - resolvePrWorkItemSource(repoPath, preference, connectionId, localGitOptions) - ]) - const issueOwnerRepo = issueResolved.source - const prOwnerRepo = prResolved.source - const ownerRepo = prOwnerRepo ?? issueOwnerRepo - if (!ownerRepo) { - return 0 - } - - const parsedQuery = trimmedQuery ? parseTaskQuery(trimmedQuery) : null - const effectiveQuery = parsedQuery ?? defaultOpenWorkItemQuery() - const ghOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) - } - - // Decorative counts must stop at the search budget floor. - if (spendsSharedGitHubComQuota(ownerRepo, ghOptions)) { - await getRateLimit() - } - if ( - !usesGraphqlWorkItemSearch(ownerRepo, ghOptions) && - repositoryRateLimitGuard(ownerRepo, 'search', ghOptions).blocked - ) { - return 0 - } - - await acquire() - try { - if (sameOwnerRepo(issueOwnerRepo, prOwnerRepo)) { - return await countWorkItemsForQuery( - repoPath, - ownerRepo, - effectiveQuery, - connectionId, - localGitOptions - ) + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + const trimmedQuery = query?.trim() ?? '' + if (isGitHubWorkItemsQueryTooLarge(trimmedQuery)) { + return 0 + } + const [issueResolved, prResolved] = await Promise.all([ + resolveIssueGitHubApiRepositorySource(repoPath, preference, connectionId, localGitOptions), + resolvePrWorkItemSource(repoPath, preference, connectionId, localGitOptions) + ]) + const issueOwnerRepo = issueResolved.source + const prOwnerRepo = prResolved.source + const ownerRepo = prOwnerRepo ?? issueOwnerRepo + if (!ownerRepo) { + return 0 } - const counts: Promise[] = [] - // PR-only filters cannot match issues; avoid spending a search call on that half. - const hasPrOnlyFilter = - effectiveQuery.draft || - effectiveQuery.reviewRequested !== null || - effectiveQuery.reviewedBy !== null + const parsedQuery = trimmedQuery ? parseTaskQuery(trimmedQuery) : null + const effectiveQuery = parsedQuery ?? defaultOpenWorkItemQuery() + const ghOptions = { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } + + // Decorative counts must stop at the search budget floor. + if (spendsSharedGitHubComQuota(ownerRepo, ghOptions)) { + await getRateLimit() + } if ( - effectiveQuery.scope !== 'pr' && - effectiveQuery.state !== 'merged' && - !hasPrOnlyFilter && - issueOwnerRepo + !usesGraphqlWorkItemSearch(ownerRepo, ghOptions) && + repositoryRateLimitGuard(ownerRepo, 'search', ghOptions).blocked ) { - counts.push( - countWorkItemsForQuery( + return 0 + } + + await acquire() + try { + if (sameOwnerRepo(issueOwnerRepo, prOwnerRepo)) { + return await countWorkItemsForQuery( repoPath, - issueOwnerRepo, - { ...effectiveQuery, scope: 'issue' }, + ownerRepo, + effectiveQuery, connectionId, localGitOptions ) - ) - } - if (effectiveQuery.scope !== 'issue' && prOwnerRepo) { - counts.push( - countWorkItemsForQuery( - repoPath, - prOwnerRepo, - { ...effectiveQuery, scope: 'pr' }, - connectionId, - localGitOptions - ) - ) - } - // Preserve counts from successful sources when another search fails. - const results = await Promise.allSettled(counts) - let total = 0 - for (const r of results) { - if (r.status === 'fulfilled') { - total += r.value - } else { - console.warn('countWorkItems partial failure:', r.reason) } + + const counts: Promise[] = [] + // PR-only filters cannot match issues; avoid spending a search call on that half. + const hasPrOnlyFilter = + effectiveQuery.draft || + effectiveQuery.reviewRequested !== null || + effectiveQuery.reviewedBy !== null + if ( + effectiveQuery.scope !== 'pr' && + effectiveQuery.state !== 'merged' && + !hasPrOnlyFilter && + issueOwnerRepo + ) { + counts.push( + countWorkItemsForQuery( + repoPath, + issueOwnerRepo, + { ...effectiveQuery, scope: 'issue' }, + connectionId, + localGitOptions + ) + ) + } + if (effectiveQuery.scope !== 'issue' && prOwnerRepo) { + counts.push( + countWorkItemsForQuery( + repoPath, + prOwnerRepo, + { ...effectiveQuery, scope: 'pr' }, + connectionId, + localGitOptions + ) + ) + } + // Preserve counts from successful sources when another search fails. + const results = await Promise.allSettled(counts) + let total = 0 + for (const r of results) { + if (r.status === 'fulfilled') { + total += r.value + } else { + console.warn('countWorkItems partial failure:', r.reason) + } + } + return total + } catch (err) { + console.warn('countWorkItems failed:', err) + return 0 + } finally { + release() } - return total - } catch (err) { - console.warn('countWorkItems failed:', err) - return 0 - } finally { - release() - } + }) } diff --git a/src/main/github/client/list/list-work-items.ts b/src/main/github/client/list/list-work-items.ts index 836bdb1d9df..0a7b935362c 100644 --- a/src/main/github/client/list/list-work-items.ts +++ b/src/main/github/client/list/list-work-items.ts @@ -1,3 +1,4 @@ +import { withGitHubRepoExecution } from '../../github-repository-identity' import type { ListWorkItemsResult } from '../../../../shared/github/work-item-types' import type { IssueSourcePreference } from '../../../../shared/repo-types' import { isGitHubWorkItemsQueryTooLarge } from '../../../../shared/github/work-items-query-bounds' @@ -18,75 +19,77 @@ export async function listWorkItems( noCache?: boolean, localGitOptions: LocalGitExecOptions = {} ): Promise> { - const trimmedQuery = query?.trim() ?? '' - const requestedPage = normalizeWorkItemPage(page) - if (isGitHubWorkItemsQueryTooLarge(trimmedQuery)) { - return { - items: [], - sources: { - issues: null, - prs: null, - originCandidate: null, - upstreamCandidate: null + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + const trimmedQuery = query?.trim() ?? '' + const requestedPage = normalizeWorkItemPage(page) + if (isGitHubWorkItemsQueryTooLarge(trimmedQuery)) { + return { + items: [], + sources: { + issues: null, + prs: null, + originCandidate: null, + upstreamCandidate: null + } } } - } - const [issueResolved, prResolved] = await Promise.all([ - resolveIssueGitHubApiRepositorySource(repoPath, preference, connectionId, localGitOptions), - resolvePrWorkItemSource(repoPath, preference, connectionId, localGitOptions) - ]) - const issueOwnerRepo = issueResolved.source - const prOwnerRepo = prResolved.source - const ineligible = - !issueOwnerRepo && - !prOwnerRepo && - (await isTaskRepositoryIneligible(repoPath, 'github', connectionId, localGitOptions)) - await acquire() - try { - // Preserve fetch failures for IPC; an empty fallback would hide per-repo outages. - const partial = !trimmedQuery - ? await listRecentWorkItems( - repoPath, - issueOwnerRepo, - prOwnerRepo, - limit, - requestedPage, - connectionId, - noCache, - localGitOptions - ) - : await listQueriedWorkItems( - repoPath, - issueOwnerRepo, - prOwnerRepo, - parseTaskQuery(trimmedQuery), - limit, - requestedPage, - connectionId, - localGitOptions, - noCache - ) + const [issueResolved, prResolved] = await Promise.all([ + resolveIssueGitHubApiRepositorySource(repoPath, preference, connectionId, localGitOptions), + resolvePrWorkItemSource(repoPath, preference, connectionId, localGitOptions) + ]) + const issueOwnerRepo = issueResolved.source + const prOwnerRepo = prResolved.source + const ineligible = + !issueOwnerRepo && + !prOwnerRepo && + (await isTaskRepositoryIneligible(repoPath, 'github', connectionId, localGitOptions)) + await acquire() + try { + // Preserve fetch failures for IPC; an empty fallback would hide per-repo outages. + const partial = !trimmedQuery + ? await listRecentWorkItems( + repoPath, + issueOwnerRepo, + prOwnerRepo, + limit, + requestedPage, + connectionId, + noCache, + localGitOptions + ) + : await listQueriedWorkItems( + repoPath, + issueOwnerRepo, + prOwnerRepo, + parseTaskQuery(trimmedQuery), + limit, + requestedPage, + connectionId, + localGitOptions, + noCache + ) - const errors = - partial.issuesError || partial.prsError - ? { - ...(partial.issuesError ? { issues: partial.issuesError } : {}), - ...(partial.prsError ? { prs: partial.prsError } : {}) - } - : undefined - return { - items: partial.items, - sources: { - issues: issueOwnerRepo, - prs: prOwnerRepo, - originCandidate: prResolved.originCandidate, - upstreamCandidate: prResolved.upstreamCandidate, - ...(ineligible ? { ineligible: true as const } : {}) - }, - ...(errors ? { errors } : {}), - ...(issueResolved.fellBack ? { issueSourceFellBack: true } : {}) + const errors = + partial.issuesError || partial.prsError + ? { + ...(partial.issuesError ? { issues: partial.issuesError } : {}), + ...(partial.prsError ? { prs: partial.prsError } : {}) + } + : undefined + return { + items: partial.items, + sources: { + issues: issueOwnerRepo, + prs: prOwnerRepo, + originCandidate: prResolved.originCandidate, + upstreamCandidate: prResolved.upstreamCandidate, + ...(ineligible ? { ineligible: true as const } : {}) + }, + ...(errors ? { errors } : {}), + ...(issueResolved.fellBack ? { issueSourceFellBack: true } : {}) + } + } finally { + release() } - } finally { - release() - } + }) } diff --git a/src/main/github/client/lookup/pr-for-branch-outcome.ts b/src/main/github/client/lookup/pr-for-branch-outcome.ts index e9b80e9e4c4..aa3f4b8377e 100644 --- a/src/main/github/client/lookup/pr-for-branch-outcome.ts +++ b/src/main/github/client/lookup/pr-for-branch-outcome.ts @@ -1,3 +1,4 @@ +import { withGitHubRepoExecution } from '../../github-repository-identity' import { getRepoExecutionHostId } from '../../../../shared/execution-host' import { hostedReviewRepoScope, @@ -21,51 +22,62 @@ export async function getPRForBranchOutcome( fallbackPRNumber?: number | null, options: GitHubPRBranchLookupOptions = {} ): Promise { - const branchName = branch.replace(/^refs\/heads\//, '') - // Detached HEAD cannot discover by branch, but explicit PR numbers remain safe. - if (!branchName && typeof linkedPRNumber !== 'number' && typeof fallbackPRNumber !== 'number') { - return { kind: 'no-pr', fetchedAt: Date.now() } - } - const localGitArgs = hostedReviewLocalGitOptionArgs(options) - const localGitOptions = localGitArgs[0] ?? {} - const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ghOptions = ghRepoExecOptions(context) - const executionScope = githubPRStackExecutionScope(connectionId, localGitOptions) - - const key = JSON.stringify([ - executionScope, + return withGitHubRepoExecution( repoPath, - scopeGeneration(hostedReviewRepoScope(repoPath, getRepoExecutionHostId({ connectionId }))), - branchName, - linkedPRNumber ?? null, - fallbackPRNumber ?? null, - options.acceptMergedFallbackPR ?? false, - options.currentHeadOid ?? null, - githubReadExecutionScope(ghOptions) - ]) - return runCoalescedProbe( - reads, - key, - async () => { - await acquire() - try { - return await resolvePRForBranchOutcome({ - repoPath, - branchName, - linkedPRNumber, - connectionId, - fallbackPRNumber, - options, - localGitOptions, - ghOptions, - executionScope - }) - } catch (err) { - return prRefreshUpstreamError(err) - } finally { - release() + connectionId, + hostedReviewLocalGitOptionArgs(options)[0] ?? {}, + async (): Promise => { + const branchName = branch.replace(/^refs\/heads\//, '') + // Detached HEAD cannot discover by branch, but explicit PR numbers remain safe. + if ( + !branchName && + typeof linkedPRNumber !== 'number' && + typeof fallbackPRNumber !== 'number' + ) { + return { kind: 'no-pr', fetchedAt: Date.now() } } - }, - 2 * 60_000 + const localGitArgs = hostedReviewLocalGitOptionArgs(options) + const localGitOptions = localGitArgs[0] ?? {} + const context = githubRepoContext(repoPath, connectionId, localGitOptions) + const ghOptions = ghRepoExecOptions(context) + const executionScope = githubPRStackExecutionScope(connectionId, localGitOptions) + + const key = JSON.stringify([ + executionScope, + repoPath, + scopeGeneration(hostedReviewRepoScope(repoPath, getRepoExecutionHostId({ connectionId }))), + branchName, + linkedPRNumber ?? null, + fallbackPRNumber ?? null, + options.acceptMergedFallbackPR ?? false, + options.currentHeadOid ?? null, + githubReadExecutionScope(ghOptions) + ]) + return runCoalescedProbe( + reads, + key, + async () => { + await acquire() + try { + return await resolvePRForBranchOutcome({ + repoPath, + branchName, + linkedPRNumber, + connectionId, + fallbackPRNumber, + options, + localGitOptions, + ghOptions, + executionScope + }) + } catch (err) { + return prRefreshUpstreamError(err) + } finally { + release() + } + }, + 2 * 60_000 + ) + } ) } diff --git a/src/main/github/client/lookup/pr-stack-summary-cache.ts b/src/main/github/client/lookup/pr-stack-summary-cache.ts index e39ec4b686e..c21baa63106 100644 --- a/src/main/github/client/lookup/pr-stack-summary-cache.ts +++ b/src/main/github/client/lookup/pr-stack-summary-cache.ts @@ -1,6 +1,7 @@ import type { GitHubPRStack } from '../../../../shared/github/pull-request-types' import type { ghRepoExecOptions } from '../../gh-utils' -import type { GitHubApiRepository } from '../../github-api-repository' +import { githubHostExecOptions, type GitHubApiRepository } from '../../github-api-repository' +import { githubReadExecutionScope } from '../../github-read-execution-scope' import { githubRepoIdentityKey } from '../../../../shared/github/repository-identity-key' import { getRestPRByNumber } from './pr-number-lookup' export const PR_STACK_SUMMARY_CACHE_TTL_MS = 60_000 @@ -42,7 +43,8 @@ export async function getCachedGitHubPRStackSummary( ghOptions: ReturnType, executionScope: string ): Promise { - const key = `${executionScope}\0${githubRepoIdentityKey(ownerRepo)}#${number}` + const execution = { ...ghOptions, ...githubHostExecOptions(ownerRepo) } + const key = `${executionScope}\0${githubReadExecutionScope(execution)}\0${githubRepoIdentityKey(ownerRepo)}#${number}` const now = Date.now() prunePRStackSummaryCache(now) const cached = prStackSummaryCache.get(key) @@ -56,7 +58,7 @@ export async function getCachedGitHubPRStackSummary( if (existing) { return existing } - const request = getRestPRByNumber(ownerRepo, number, ghOptions).then((pr) => pr.stack) + const request = getRestPRByNumber(ownerRepo, number, execution).then((pr) => pr.stack) prStackSummaryInFlight.set(key, request) try { const value = await request diff --git a/src/main/github/client/lookup/pull-request-push-target.ts b/src/main/github/client/lookup/pull-request-push-target.ts index 13fdc1f6ea0..be96c55dfb1 100644 --- a/src/main/github/client/lookup/pull-request-push-target.ts +++ b/src/main/github/client/lookup/pull-request-push-target.ts @@ -1,3 +1,4 @@ +import { withGitHubRepoExecution } from '../../github-repository-identity' import type { IssueSourcePreference } from '../../../../shared/repo-types' import type { GitPushTarget } from '../../../../shared/worktree/types' import { @@ -53,99 +54,101 @@ export async function getPullRequestPushTarget( localGitOptions: LocalGitExecOptions = {}, preference?: IssueSourcePreference ): Promise { - const context = githubRepoContext(repoPath, connectionId, localGitOptions) - const ghOptions = ghRepoExecOptions(context) - const candidates = await resolvePullRequestLookupCandidates( - repoPath, - preference, - connectionId, - localGitOptions - ) - if (candidates.length === 0) { - return null - } - - await acquire() - try { - let prStdout = '' - let matchedRepository: GitHubApiRepository | null = null - for (const candidate of candidates) { - try { - const { stdout } = await ghExecFileAsync( - ['api', `repos/${candidate.owner}/${candidate.repo}/pulls/${prNumber}`], - { ...ghOptions, ...githubHostExecOptions(candidate) } - ) - prStdout = stdout - matchedRepository = candidate - break - } catch (error) { - // Fork origin may differ from the upstream PR namespace; exhaust candidate repositories. - if (isNotFoundGhError(error)) { - continue - } - throw error - } - } - if (!prStdout || !matchedRepository) { - return null - } - const origin = await getGitHubApiRepositoryForRemote( + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + const context = githubRepoContext(repoPath, connectionId, localGitOptions) + const ghOptions = ghRepoExecOptions(context) + const candidates = await resolvePullRequestLookupCandidates( repoPath, - 'origin', + preference, connectionId, localGitOptions ) - const pr = JSON.parse(prStdout) as { - maintainer_can_modify?: boolean - head?: { - ref?: string - repo?: { - full_name?: string - clone_url?: string - ssh_url?: string - owner?: { login?: string } - name?: string - } | null - } - } - const headRepo = pr.head?.repo - const branchName = pr.head?.ref?.trim() - const owner = headRepo?.owner?.login?.trim() - const repo = headRepo?.name?.trim() ?? headRepo?.full_name?.split('/')[1]?.trim() - const cloneUrl = headRepo?.clone_url?.trim() - const sshUrl = headRepo?.ssh_url?.trim() - const maintainerCanModify = - typeof pr.maintainer_can_modify === 'boolean' ? pr.maintainer_can_modify : undefined - if (!owner || !repo || !branchName || !cloneUrl || !sshUrl) { + if (candidates.length === 0) { return null } - if ( - origin && - githubRepoIdentityKey(origin) === - githubRepoIdentityKey({ owner, repo, host: matchedRepository.host }) - ) { + + await acquire() + try { + let prStdout = '' + let matchedRepository: GitHubApiRepository | null = null + for (const candidate of candidates) { + try { + const { stdout } = await ghExecFileAsync( + ['api', `repos/${candidate.owner}/${candidate.repo}/pulls/${prNumber}`], + { ...ghOptions, ...githubHostExecOptions(candidate) } + ) + prStdout = stdout + matchedRepository = candidate + break + } catch (error) { + // Fork origin may differ from the upstream PR namespace; exhaust candidate repositories. + if (isNotFoundGhError(error)) { + continue + } + throw error + } + } + if (!prStdout || !matchedRepository) { + return null + } + const origin = await getGitHubApiRepositoryForRemote( + repoPath, + 'origin', + connectionId, + localGitOptions + ) + const pr = JSON.parse(prStdout) as { + maintainer_can_modify?: boolean + head?: { + ref?: string + repo?: { + full_name?: string + clone_url?: string + ssh_url?: string + owner?: { login?: string } + name?: string + } | null + } + } + const headRepo = pr.head?.repo + const branchName = pr.head?.ref?.trim() + const owner = headRepo?.owner?.login?.trim() + const repo = headRepo?.name?.trim() ?? headRepo?.full_name?.split('/')[1]?.trim() + const cloneUrl = headRepo?.clone_url?.trim() + const sshUrl = headRepo?.ssh_url?.trim() + const maintainerCanModify = + typeof pr.maintainer_can_modify === 'boolean' ? pr.maintainer_can_modify : undefined + if (!owner || !repo || !branchName || !cloneUrl || !sshUrl) { + return null + } + if ( + origin && + githubRepoIdentityKey(origin) === + githubRepoIdentityKey({ owner, repo, host: matchedRepository.host }) + ) { + return { + pushTarget: { remoteName: 'origin', branchName }, + ...(maintainerCanModify !== undefined ? { maintainerCanModify } : {}) + } + } + + let originUrl: string | null = null + try { + const rawOriginUrl = await getRemoteUrlForRepo(context, 'origin') + originUrl = rawOriginUrl?.trim() || null + } catch { + originUrl = null + } return { - pushTarget: { remoteName: 'origin', branchName }, + pushTarget: { + remoteName: sanitizeRemoteName(owner, repo), + branchName, + remoteUrl: pickPushRemoteUrl({ originUrl, cloneUrl, sshUrl }) + }, ...(maintainerCanModify !== undefined ? { maintainerCanModify } : {}) } + } finally { + release() } - - let originUrl: string | null = null - try { - const rawOriginUrl = await getRemoteUrlForRepo(context, 'origin') - originUrl = rawOriginUrl?.trim() || null - } catch { - originUrl = null - } - return { - pushTarget: { - remoteName: sanitizeRemoteName(owner, repo), - branchName, - remoteUrl: pickPushRemoteUrl({ originUrl, cloneUrl, sshUrl }) - }, - ...(maintainerCanModify !== undefined ? { maintainerCanModify } : {}) - } - } finally { - release() - } + }) } diff --git a/src/main/github/github-api-repository-probe.ts b/src/main/github/github-api-repository-probe.ts index 569f4153bb0..b5a4e4e1b80 100644 --- a/src/main/github/github-api-repository-probe.ts +++ b/src/main/github/github-api-repository-probe.ts @@ -1,4 +1,4 @@ -import type { LocalGitExecOptions } from './github-repository-identity' +import { githubRepoContext, type LocalGitExecOptions } from './github-repository-identity' export function githubApiRepositoryProbeCacheKey( repoPath: string, @@ -10,7 +10,8 @@ export function githubApiRepositoryProbeCacheKey( const runtimeKey = connectionId ? `ssh:${connectionId}` : `local:${localGitOptions.wslDistro ?? 'host'}` - return `${runtimeKey}\0${repoPath}\0${remoteName}\0${requireVerifiedSshProbe ? 'verified' : 'tolerant'}` + const { ghAccount } = githubRepoContext(repoPath, connectionId, localGitOptions) + return `${runtimeKey}\0${repoPath}\0${remoteName}\0${requireVerifiedSshProbe ? 'verified' : 'tolerant'}\0${JSON.stringify(ghAccount ?? null)}` } export function resolveGitHubApiRepositoryProbe( diff --git a/src/main/github/github-api-repository-remote-probe.ts b/src/main/github/github-api-repository-remote-probe.ts index 823287d4e92..10d98af4124 100644 --- a/src/main/github/github-api-repository-remote-probe.ts +++ b/src/main/github/github-api-repository-remote-probe.ts @@ -1,3 +1,4 @@ +import { withGitHubRepoExecution } from './github-repository-identity' import type { GitHubApiRepository } from './github-api-repository' import { getOwnerRepoForRemote, @@ -47,73 +48,75 @@ export async function getGitHubApiRepositoryForRemote( localGitOptions: LocalGitExecOptions = {}, probeOptions: GitHubRemoteIdentityProbeOptions = {} ): Promise { - // Generic PR resolution prefers upstream; this entry point must honor the selected remote. - const requireVerifiedSshProbe = probeOptions.requireVerifiedSshProbe === true - const verifiedIdentityArgs = requireVerifiedSshProbe ? ([probeOptions] as const) : [] - const ownerRepo = await getOwnerRepoForRemote( - repoPath, - remoteName, - connectionId, - localGitOptions, - ...verifiedIdentityArgs - ) - if (ownerRepo) { - return { ...ownerRepo, host: 'github.com' } - } - const cacheKey = githubApiRepositoryProbeCacheKey( - repoPath, - remoteName, - connectionId, - localGitOptions, - requireVerifiedSshProbe - ) - const now = Date.now() - pruneOriginRepoCache(now) - const cached = originRepoCache.get(cacheKey) - if (cached && cached.expiresAt > 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 verifiedEnterpriseArgs = requireVerifiedSshProbe ? ([true] as const) : [] - const slug = - remoteName === 'origin' - ? await getEnterpriseGitHubRepoSlug( - repoPath, - connectionId, - enterpriseOptions, - ...verifiedEnterpriseArgs - ) - : await getEnterpriseGitHubRepoSlugForRemote( - repoPath, - remoteName, - connectionId, - enterpriseOptions, - ...verifiedEnterpriseArgs - ) - // Undefined means auth inventory was unreadable; do not cache transient probe failures. - if (slug !== undefined) { - originRepoCache.set(cacheKey, { - value: slug, - expiresAt: Date.now() + ORIGIN_REPO_CACHE_TTL_MS - }) - pruneOriginRepoCache(Date.now()) + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + // Generic PR resolution prefers upstream; this entry point must honor the selected remote. + const requireVerifiedSshProbe = probeOptions.requireVerifiedSshProbe === true + const verifiedIdentityArgs = requireVerifiedSshProbe ? ([probeOptions] as const) : [] + const ownerRepo = await getOwnerRepoForRemote( + repoPath, + remoteName, + connectionId, + localGitOptions, + ...verifiedIdentityArgs + ) + if (ownerRepo) { + return { ...ownerRepo, host: 'github.com' } } - return resolveGitHubApiRepositoryProbe(slug, requireVerifiedSshProbe) - })() - originRepoInFlight.set(cacheKey, probe) - try { - return await probe - } finally { - if (originRepoInFlight.get(cacheKey) === probe) { - originRepoInFlight.delete(cacheKey) + const cacheKey = githubApiRepositoryProbeCacheKey( + repoPath, + remoteName, + connectionId, + localGitOptions, + requireVerifiedSshProbe + ) + const now = Date.now() + pruneOriginRepoCache(now) + const cached = originRepoCache.get(cacheKey) + if (cached && cached.expiresAt > 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 verifiedEnterpriseArgs = requireVerifiedSshProbe ? ([true] as const) : [] + const slug = + remoteName === 'origin' + ? await getEnterpriseGitHubRepoSlug( + repoPath, + connectionId, + enterpriseOptions, + ...verifiedEnterpriseArgs + ) + : await getEnterpriseGitHubRepoSlugForRemote( + repoPath, + remoteName, + connectionId, + enterpriseOptions, + ...verifiedEnterpriseArgs + ) + // Undefined means auth inventory was unreadable; do not cache transient probe failures. + if (slug !== undefined) { + originRepoCache.set(cacheKey, { + value: slug, + expiresAt: Date.now() + ORIGIN_REPO_CACHE_TTL_MS + }) + pruneOriginRepoCache(Date.now()) + } + return resolveGitHubApiRepositoryProbe(slug, requireVerifiedSshProbe) + })() + originRepoInFlight.set(cacheKey, probe) + try { + return await probe + } finally { + if (originRepoInFlight.get(cacheKey) === probe) { + originRepoInFlight.delete(cacheKey) + } + } + }) } export async function getOriginGitHubApiRepository( diff --git a/src/main/github/github-api-repository.ts b/src/main/github/github-api-repository.ts index aefbcb99359..b7afa54ba5e 100644 --- a/src/main/github/github-api-repository.ts +++ b/src/main/github/github-api-repository.ts @@ -1,3 +1,4 @@ +import { withGitHubRepoExecution } from './github-repository-identity' import type { GitHubOwnerRepo } from '../../shared/github/pull-request-types' import type { IssueSourcePreference } from '../../shared/repo-types' import { @@ -44,26 +45,33 @@ export async function getIssueGitHubApiRepository( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const originPromise = getGitHubApiRepositoryForRemote( - repoPath, - 'origin', - connectionId, - localGitOptions - ).then( - (value) => ({ status: 'fulfilled' as const, value }), - (reason: unknown) => ({ status: 'rejected' as const, reason }) - ) - const upstream = (await shouldProbeGitRemote(repoPath, 'upstream', connectionId, localGitOptions)) - ? await getGitHubApiRepositoryForRemote(repoPath, 'upstream', connectionId, localGitOptions) - : null - if (upstream) { - return upstream - } - const origin = await originPromise - if (origin.status === 'rejected') { - throw origin.reason - } - return origin.value + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + const originPromise = getGitHubApiRepositoryForRemote( + repoPath, + 'origin', + connectionId, + localGitOptions + ).then( + (value) => ({ status: 'fulfilled' as const, value }), + (reason: unknown) => ({ status: 'rejected' as const, reason }) + ) + const upstream = (await shouldProbeGitRemote( + repoPath, + 'upstream', + connectionId, + localGitOptions + )) + ? await getGitHubApiRepositoryForRemote(repoPath, 'upstream', connectionId, localGitOptions) + : null + if (upstream) { + return upstream + } + const origin = await originPromise + if (origin.status === 'rejected') { + throw origin.reason + } + return origin.value + }) } export type GitHubApiRepositoryCandidates = { @@ -77,50 +85,52 @@ export async function resolveGitHubApiRepositoryCandidates( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - const originPromise = getGitHubApiRepositoryForRemote( - repoPath, - 'origin', - connectionId, - localGitOptions, - { - requireVerifiedSshProbe: true + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + const originPromise = getGitHubApiRepositoryForRemote( + repoPath, + 'origin', + connectionId, + localGitOptions, + { + requireVerifiedSshProbe: true + } + ).then( + (value) => ({ status: 'fulfilled' as const, value }), + (reason: unknown) => ({ status: 'rejected' as const, reason }) + ) + const probeUpstream = await shouldProbeGitRemote( + repoPath, + 'upstream', + connectionId, + localGitOptions + ) + const [upstream, originResult] = await Promise.all([ + probeUpstream + ? getGitHubApiRepositoryForRemote(repoPath, 'upstream', connectionId, localGitOptions, { + requireVerifiedSshProbe: true + }) + : null, + originPromise + ]) + if (originResult.status === 'rejected') { + throw originResult.reason } - ).then( - (value) => ({ status: 'fulfilled' as const, value }), - (reason: unknown) => ({ status: 'rejected' as const, reason }) - ) - const probeUpstream = await shouldProbeGitRemote( - repoPath, - 'upstream', - connectionId, - localGitOptions - ) - const [upstream, originResult] = await Promise.all([ - probeUpstream - ? getGitHubApiRepositoryForRemote(repoPath, 'upstream', connectionId, localGitOptions, { - requireVerifiedSshProbe: true - }) - : null, - originPromise - ]) - if (originResult.status === 'rejected') { - throw originResult.reason - } - const origin = originResult.value - const seen = new Set() - const candidates: GitHubApiRepository[] = [] - for (const candidate of [upstream, origin]) { - if (!candidate) { - continue + const origin = originResult.value + const seen = new Set() + const candidates: GitHubApiRepository[] = [] + for (const candidate of [upstream, origin]) { + if (!candidate) { + continue + } + const key = githubRepoIdentityKey(candidate) + if (seen.has(key)) { + continue + } + seen.add(key) + candidates.push(candidate) } - const key = githubRepoIdentityKey(candidate) - if (seen.has(key)) { - continue - } - seen.add(key) - candidates.push(candidate) - } - return { candidates, headRepo: origin } + return { candidates, headRepo: origin } + }) } export type ResolvedGitHubApiRepositorySource = { @@ -135,39 +145,41 @@ export async function resolveIssueGitHubApiRepositorySource( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - if (preference === 'upstream') { - const upstream = await getGitHubApiRepositoryForRemote( - repoPath, - 'upstream', - connectionId, - localGitOptions - ) - if (upstream) { - return { source: upstream, fellBack: false } - } - const origin = await getGitHubApiRepositoryForRemote( - repoPath, - 'origin', - connectionId, - localGitOptions - ) - return { source: origin, fellBack: origin !== null } - } - if (preference === 'origin') { - return { - source: await getGitHubApiRepositoryForRemote( + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + if (preference === 'upstream') { + const upstream = await getGitHubApiRepositoryForRemote( + repoPath, + 'upstream', + connectionId, + localGitOptions + ) + if (upstream) { + return { source: upstream, fellBack: false } + } + const origin = await getGitHubApiRepositoryForRemote( repoPath, 'origin', connectionId, localGitOptions - ), + ) + return { source: origin, fellBack: origin !== null } + } + if (preference === 'origin') { + return { + source: await getGitHubApiRepositoryForRemote( + repoPath, + 'origin', + connectionId, + localGitOptions + ), + fellBack: false + } + } + return { + source: await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions), fellBack: false } - } - return { - source: await getIssueGitHubApiRepository(repoPath, connectionId, localGitOptions), - fellBack: false - } + }) } export async function resolveGitHubApiRepository( @@ -176,40 +188,42 @@ 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) { + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + if (repository && !isValidGitHubApiRepository(repository)) { return null } - if (isDefaultGitHubHost(host)) { - return { ...repository, host } + if (repository?.host) { + const host = repository.host.trim().toLowerCase() + if (!host) { + return null + } + if (isDefaultGitHubHost(host)) { + return { ...repository, host } + } + // Verify supplied hosts against local auth inventory before giving them ambient Enterprise credentials. + const authenticated = await isGitHubHostAuthenticated( + host, + repoPath, + connectionId, + localGitOptions + ) + return authenticated ? { ...repository, host } : null } - // Verify supplied hosts against local auth inventory before giving them ambient Enterprise credentials. - const authenticated = await isGitHubHostAuthenticated( - host, + const originRepository = await getOriginGitHubApiRepository( repoPath, connectionId, localGitOptions ) - return authenticated ? { ...repository, host } : null - } - const originRepository = await getOriginGitHubApiRepository( - repoPath, - connectionId, - localGitOptions - ) - if (!repository) { - return originRepository - } - // Older clients omit host; resolved origin supplies it for fork-base slugs on the same server. - if (originRepository?.host) { - return { ...repository, host: originRepository.host } - } - // Host-less calls could follow ambient GH_HOST; only resolved origin can supply the legacy host. - return null + if (!repository) { + return originRepository + } + // Older clients omit host; resolved origin supplies it for fork-base slugs on the same server. + if (originRepository?.host) { + return { ...repository, host: originRepository.host } + } + // Host-less calls could follow ambient GH_HOST; only resolved origin can supply the legacy host. + return null + }) } export async function resolveGitHubRepoExecution( @@ -218,23 +232,25 @@ export async function resolveGitHubRepoExecution( connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): Promise { - // Retain upstream-first issue resolution with the same host-pinned execution options. - const requestedRepository = typeof repository === 'function' ? await repository() : repository - // Preserve authoritative nulls instead of replacing them with origin. - const ownerRepo = - typeof repository === 'function' && !requestedRepository - ? null - : await resolveGitHubApiRepository( - repoPath, - requestedRepository, - connectionId, - localGitOptions - ) - return { - ownerRepo, - ghOptions: { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), - ...githubHostExecOptions(ownerRepo) + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + // Retain upstream-first issue resolution with the same host-pinned execution options. + const requestedRepository = typeof repository === 'function' ? await repository() : repository + // Preserve authoritative nulls instead of replacing them with origin. + const ownerRepo = + typeof repository === 'function' && !requestedRepository + ? null + : await resolveGitHubApiRepository( + repoPath, + requestedRepository, + connectionId, + localGitOptions + ) + return { + ownerRepo, + ghOptions: { + ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), + ...githubHostExecOptions(ownerRepo) + } } - } + }) } diff --git a/src/main/github/github-enterprise-repository.ts b/src/main/github/github-enterprise-repository.ts index d9cb599c531..f583e983c75 100644 --- a/src/main/github/github-enterprise-repository.ts +++ b/src/main/github/github-enterprise-repository.ts @@ -132,7 +132,8 @@ async function resolveAuthenticatedGitHubHost( localGitOptions: LocalGitExecOptions = {} ): Promise { const normalizedHost = normalizeGitHubHost(host)?.authority ?? host.trim().toLowerCase() - const cacheKey = `${runtimeCacheKey(repoPath, connectionId, localGitOptions.wslDistro)}\0${normalizedHost}\0${githubReadExecutionScope({ ghAccount: localGitOptions.ghAccount })}` + const execOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) + const cacheKey = `${runtimeCacheKey(repoPath, connectionId, localGitOptions.wslDistro)}\0${normalizedHost}\0${githubReadExecutionScope(execOptions)}` const now = Date.now() pruneHostAuthCache(now) const cached = hostAuthCache.get(cacheKey) @@ -144,9 +145,6 @@ async function resolveAuthenticatedGitHubHost( return inFlight } const probe = (async () => { - const execOptions = { - ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) - } let authenticatedHost: string | null try { const { stdout, stderr } = await ghExecFileAsync(['auth', 'status'], execOptions) diff --git a/src/main/github/github-pr-stack.test.ts b/src/main/github/github-pr-stack.test.ts index 0bc4d901be4..2bfac0f8827 100644 --- a/src/main/github/github-pr-stack.test.ts +++ b/src/main/github/github-pr-stack.test.ts @@ -94,7 +94,7 @@ describe('hydrateGitHubPRStack', () => { repository, 201, { ...summary, position: 1 }, - { cwd: '/other-worktree' } + { cwd: '/repo' } ) expect(first).toMatchObject({ diff --git a/src/main/github/github-pr-stack.ts b/src/main/github/github-pr-stack.ts index 3dc65f6175a..5b8114dfe99 100644 --- a/src/main/github/github-pr-stack.ts +++ b/src/main/github/github-pr-stack.ts @@ -8,6 +8,7 @@ import type { } from '../../shared/github/pull-request-types' import { githubRepoIdentityKey } from '../../shared/github/repository-identity-key' import { ghExecFileAsync } from '../git/runner' +import { githubReadExecutionScope } from './github-read-execution-scope' import { githubHostExecOptions, type GitHubApiRepository, @@ -67,9 +68,10 @@ export function _resetGitHubPRStackCacheForTests(): void { function stackCacheKey( repository: GitHubApiRepository, stackNumber: number, - executionScope: string + executionScope: string, + execution: GitHubRepoExecOptions ): string { - return `${executionScope}\0${githubRepoIdentityKey(repository)}:${stackNumber}` + return `${executionScope}\0${githubReadExecutionScope(execution)}\0${githubRepoIdentityKey(repository)}:${stackNumber}` } function pruneStackCache(now = Date.now()): void { @@ -228,7 +230,7 @@ async function fetchStackDetails( '-F', `pr=${prNumber}` ], - { ...ghOptions, ...githubHostExecOptions(repository) } + ghOptions ) return parseStackDetails( JSON.parse(stdout) as GraphQLStackResponse, @@ -246,7 +248,8 @@ export async function hydrateGitHubPRStack( prUpdatedAt?: string, executionScope = 'local:host' ): Promise { - const key = stackCacheKey(repository, summary.number, executionScope) + const execution = { ...ghOptions, ...githubHostExecOptions(repository) } + const key = stackCacheKey(repository, summary.number, executionScope, execution) const now = Date.now() pruneStackCache(now) const cached = stackDetailsCache.get(key) @@ -264,7 +267,7 @@ export async function hydrateGitHubPRStack( const value = await existing return value ? { ...value, position: summary.position, baseSha: summary.baseSha } : summary } - const request = fetchStackDetails(repository, prNumber, summary, ghOptions).catch(() => null) + const request = fetchStackDetails(repository, prNumber, summary, execution).catch(() => null) stackDetailsInFlight.set(key, request) try { const value = await request diff --git a/src/main/github/github-project-account.test.ts b/src/main/github/github-project-account.test.ts new file mode 100644 index 00000000000..11661811cba --- /dev/null +++ b/src/main/github/github-project-account.test.ts @@ -0,0 +1,145 @@ +import { afterEach, expect, it } from 'vitest' +import type { Project } from '../../shared/project-types' +import type { Repo } from '../../shared/repo-types' +import { ghRepoExecOptions, githubRepoContext } from './github-repository-identity' +import { installGitHubProjectAccounts } from './github-project-account' +import { githubReadExecutionScope } from './github-read-execution-scope' +import { resolveGitHubRepoExecution } from './github-api-repository' + +let release: (() => void) | undefined +afterEach(() => release?.()) +const alice = { host: 'github.com', user: 'Alice' } +const bob = { host: 'github.com', user: 'Bob' } +function repo(overrides: Partial = {}): Repo { + return { + id: 'repo', + path: '/repo', + displayName: 'Repo', + badgeColor: '#000000', + addedAt: 0, + ghAccount: alice, + ...overrides + } +} +function options(path: string, connectionId?: string | null, extra = {}) { + return ghRepoExecOptions(githubRepoContext(path, connectionId, extra)) +} + +it('uses the saved bound or unbound selection over stale caller options, and isolates cache keys', () => { + let current = repo() + release = installGitHubProjectAccounts({ getRepos: () => [current] }) + const first = options('/repo', null, { ghAccount: bob }) + expect(first.ghAccount).toEqual(alice) + current = repo({ ghAccount: bob }) + const second = options('/repo', null, { ghAccount: alice }) + expect(second.ghAccount).toEqual(bob) + expect(githubReadExecutionScope(first, {})).not.toBe(githubReadExecutionScope(second, {})) + current = repo({ ghAccount: undefined }) + expect(options('/repo', null, { ghAccount: alice }).ghAccount).toBeUndefined() + expect(first.ghAccount).toEqual(alice) +}) + +it.each(['cold', 'warm'] as const)( + 'resolves a %s child worktree by its host-qualified owning project', + (mode) => { + const local = repo() + const remote = repo({ path: '/repo', connectionId: 'server', ghAccount: bob }) + release = installGitHubProjectAccounts( + { + getRepos: () => [local, remote], + getAllWorktreeMetaForHost: (host) => + mode === 'cold' && host === 'ssh:server' ? { 'repo::/worktrees/child': {} } : {} + }, + () => + mode === 'warm' ? [{ repoId: 'repo', path: '/worktrees/child', hostId: 'ssh:server' }] : [] + ) + expect(options('/worktrees/child', 'server')).toEqual({ ghAccount: bob }) + expect(options('/worktrees/child')).toEqual({ cwd: '/worktrees/child' }) + expect(options('/repo').ghAccount).toEqual(alice) + expect(options('/repo', 'server').ghAccount).toEqual(bob) + } +) + +it('preserves explicit options on ownership misses without adding a refusal', () => { + release = installGitHubProjectAccounts({ getRepos: () => [repo()] }) + expect(options('/unseen-worktree', null, { ghAccount: bob })).toMatchObject({ ghAccount: bob }) + expect(options('/unseen-worktree')).toEqual({ cwd: '/unseen-worktree' }) +}) + +it('uses WSL namespace and Windows path spelling without merging POSIX case', () => { + release = installGitHubProjectAccounts({ + getRepos: () => [ + repo({ path: String.raw`\\wsl.localhost\Ubuntu\home\work` }), + repo({ id: 'win', path: String.raw`C:\Work`, ghAccount: bob }) + ] + }) + expect(options('/home/work', null, { wslDistro: 'ubuntu' }).ghAccount).toEqual(alice) + expect(options('/home/Work', null, { wslDistro: 'Ubuntu' }).ghAccount).toBeUndefined() + expect(options('/home/work', null, { wslDistro: 'Debian' }).ghAccount).toBeUndefined() + expect(options('c:/work/').ghAccount).toEqual(bob) +}) + +it('keeps folder cards on their git child account and plain folders outside project gh selection', () => { + release = installGitHubProjectAccounts({ + getRepos: () => [ + repo({ id: 'folder', path: '/folder', kind: 'folder' }), + repo({ id: 'child', path: '/folder/repo', ghAccount: bob }) + ] + }) + expect(options('/folder/repo').ghAccount).toEqual(bob) + expect(options('/folder').ghAccount).toBeUndefined() +}) + +it('does not let an earlier owner release a replacement installation', () => { + const previous = installGitHubProjectAccounts({ getRepos: () => [repo()] }) + release = installGitHubProjectAccounts({ getRepos: () => [repo({ ghAccount: bob })] }) + previous() + expect(options('/repo').ghAccount).toEqual(bob) + installGitHubProjectAccounts(null) + expect(options('/repo').ghAccount).toBeUndefined() +}) + +it('selects before an asynchronous repository lookup and retains that account for execution', async () => { + let current = repo() + release = installGitHubProjectAccounts({ getRepos: () => [current] }) + let finish: () => void = () => {} + const waiting = new Promise((resolve) => { + finish = resolve + }) + const pending = resolveGitHubRepoExecution('/repo', async () => { + await waiting + return { owner: 'acme', repo: 'widgets', host: 'github.com' } + }) + current = repo({ ghAccount: bob }) + finish() + expect((await pending).ghOptions.ghAccount).toEqual(alice) + expect(options('/repo').ghAccount).toEqual(bob) +}) + +it('derives runtime ownership again after project runtime changes', () => { + const platform = process.platform + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + try { + let project: Project = { + id: 'p', + displayName: 'Work', + badgeColor: '#000000', + createdAt: 0, + updatedAt: 0, + sourceRepoIds: ['repo'], + localWindowsRuntimePreference: { kind: 'wsl', distro: 'Ubuntu' } + } + release = installGitHubProjectAccounts({ + getRepos: () => [repo()], + getProjects: () => [project], + getSettings: () => ({}) + }) + expect(options('/repo', null, { wslDistro: 'Ubuntu' }).ghAccount).toEqual(alice) + expect(options('/repo').ghAccount).toBeUndefined() + project = { ...project, localWindowsRuntimePreference: { kind: 'windows-host' } } + expect(options('/repo').ghAccount).toEqual(alice) + expect(options('/repo', null, { wslDistro: 'Ubuntu' }).ghAccount).toBeUndefined() + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: platform }) + } +}) diff --git a/src/main/github/github-project-account.ts b/src/main/github/github-project-account.ts new file mode 100644 index 00000000000..8ed7844c43c --- /dev/null +++ b/src/main/github/github-project-account.ts @@ -0,0 +1,89 @@ +import type { Repo } from '../../shared/repo-types' +import { normalizeRuntimePathForComparison } from '../../shared/cross-platform-path' +import { getConnectionExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' +import { normalizeGhAccountBinding } from '../../shared/github/account-binding' +import { isGitRepoKind } from '../../shared/repo-kind' +import { splitWorktreeIdForFilesystem } from '../../shared/worktree/id' +import { parseWslUncPath } from '../../shared/wsl-paths' +import { + resolveLocalProjectRuntimesForRepos, + type ProjectRuntimeResolutionStore +} from '../local-project-runtime-resolution' +import { getStoredRepoExecutionHostId } from '../repo-execution-host' +import type { GitHubRepoContext } from './github-repository-identity' + +type AccountStore = ProjectRuntimeResolutionStore & { + getRepos: () => readonly Repo[] + getAllWorktreeMetaForHost?: (hostId: ExecutionHostId) => Record +} +type KnownWorktree = { repoId: string; path: string; hostId?: ExecutionHostId } +let source: { store: AccountStore; worktrees: () => readonly KnownWorktree[] } | null = null + +export function installGitHubProjectAccounts( + store: AccountStore | null, + worktrees: () => readonly KnownWorktree[] = () => [] +): () => void { + const installed = store ? { store, worktrees } : null + source = installed + return () => { + if (source === installed) { + source = null + } + } +} + +function pathKey(path: string, distro?: string): string { + const unc = parseWslUncPath(path) + const runtime = (unc?.distro ?? distro)?.toLowerCase() ?? '' + return `${runtime}\0${normalizeRuntimePathForComparison(unc?.linuxPath ?? path)}` +} + +/** A recognized unbound project differs from a path with no known owner. */ +export function resolveGitHubProjectAccount(context: GitHubRepoContext) { + if (!source) { + return null + } + const { store, worktrees } = source + const hostId = getConnectionExecutionHostId(context.connectionId) + const key = pathKey(context.repoPath, context.connectionId ? undefined : context.wslDistro) + const repos = store + .getRepos() + .filter((repo) => isGitRepoKind(repo) && getStoredRepoExecutionHostId(repo) === hostId) + const runtimes = resolveLocalProjectRuntimesForRepos(store, repos) + const reposById = new Map(repos.map((repo) => [repo.id, repo])) + const repoDistro = (repo: Repo): string | undefined => { + if (context.connectionId) { + return undefined + } + const runtime = runtimes.get(repo.id) + return runtime?.status === 'resolved' && runtime.runtime.kind === 'wsl' + ? runtime.runtime.distro + : parseWslUncPath(repo.path)?.distro + } + let matches = repos.filter((repo) => pathKey(repo.path, repoDistro(repo)) === key) + if (!matches.length) { + const owners = new Set() + for (const worktree of worktrees()) { + const repo = reposById.get(worktree.repoId) + if ( + repo && + getStoredRepoExecutionHostId({ executionHostId: worktree.hostId }) === hostId && + pathKey(worktree.path, repoDistro(repo)) === key + ) { + owners.add(repo.id) + } + } + // Persisted workspace identities cover cold starts without starting a Git scan. + for (const id of Object.keys(store.getAllWorktreeMetaForHost?.(hostId) ?? {})) { + const parsed = splitWorktreeIdForFilesystem(id) + const repo = parsed && reposById.get(parsed.repoId) + if (repo && parsed && pathKey(parsed.worktreePath, repoDistro(repo)) === key) { + owners.add(repo.id) + } + } + matches = repos.filter((repo) => owners.has(repo.id)) + } + return matches.length === 1 + ? { ghAccount: normalizeGhAccountBinding(matches[0].ghAccount) ?? undefined } + : null +} diff --git a/src/main/github/github-repository-identity.ts b/src/main/github/github-repository-identity.ts index 5d56c2ba536..8a8707b08b4 100644 --- a/src/main/github/github-repository-identity.ts +++ b/src/main/github/github-repository-identity.ts @@ -1,3 +1,4 @@ +import { AsyncLocalStorage } from 'node:async_hooks' import { runCoalescedProbe, type CoalescedProbes } from '../git/coalesced-probe' import { readRemoteUrl } from '../git/remote-url-probe' import type { GhAccountBinding } from '../../shared/github/account-binding' @@ -13,6 +14,7 @@ import { isStableMissingGitRemoteError } from '../git/stable-missing-git-remote- import type { GitAdmissionTier } from '../git/command-runner/git-exec-options' import { requireReachableGitRoute } from '../providers/execution-host-provider-dispatch' import { getConnectionExecutionHostId } from '../../shared/execution-host' +import { resolveGitHubProjectAccount } from './github-project-account' export type OwnerRepo = GitHubOwnerRepo @@ -38,18 +40,41 @@ export type GitHubRemoteIdentityProbeOptions = { requireVerifiedSshProbe?: boolean } +const operations = new AsyncLocalStorage() + +export async function withGitHubRepoExecution( + repoPath: string, + connectionId: string | null | undefined, + options: LocalGitExecOptions, + operation: () => Promise +): Promise { + // Async descendants keep the selection through settlement, including detached cache work. + return operations.run(githubRepoContext(repoPath, connectionId, options), operation) +} + export function githubRepoContext( repoPath: string, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} ): GitHubRepoContext { - return { + const context = { repoPath, connectionId: connectionId ?? null, ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}), ...(localGitOptions.admissionTier ? { admissionTier: localGitOptions.admissionTier } : {}), ...(localGitOptions.ghAccount ? { ghAccount: localGitOptions.ghAccount } : {}) } + const scope = operations.getStore() + const pinned = + scope && + scope.repoPath === repoPath && + scope.connectionId === context.connectionId && + scope.wslDistro === context.wslDistro + ? scope + : null + const account = pinned ?? resolveGitHubProjectAccount(context) ?? context + const binding = account.ghAccount ? Object.freeze({ ...account.ghAccount }) : undefined + return Object.freeze({ ...context, ghAccount: binding }) } export function ghRepoExecOptions(context: GitHubRepoContext): { diff --git a/src/main/github/merged-pr-commit-membership.ts b/src/main/github/merged-pr-commit-membership.ts index 8635dabce7e..97c1e819090 100644 --- a/src/main/github/merged-pr-commit-membership.ts +++ b/src/main/github/merged-pr-commit-membership.ts @@ -1,11 +1,10 @@ import { ghExecFileAsync } from './gh-utils' +import { githubReadExecutionScope } from './github-read-execution-scope' import { noteRepositoryRateLimitSpend, repositoryRateLimitGuard } from './rate-limit' -import { githubHostExecOptions } from './github-api-repository' +import { githubHostExecOptions, type GitHubRepoExecOptions } from './github-api-repository' import { githubRepoIdentityKey } from '../../shared/github/repository-identity-key' import type { OwnerRepo } from './github-repository-identity' -type GhExecOptions = Parameters[1] - // Definitive merged-commit membership is immutable; errors expire sooner so later pushes can resolve unknowns. const MEMBERSHIP_CACHE_MAX_ENTRIES = 200 const MEMBERSHIP_DEFINITIVE_TTL_MS = 6 * 60 * 60 * 1000 @@ -42,7 +41,7 @@ export async function isCommitPartOfMergedPR(args: { ownerRepo: OwnerRepo prNumber: number commitOid: string - ghOptions: GhExecOptions + ghOptions: GitHubRepoExecOptions }): Promise { const oid = args.commitOid.trim().toLowerCase() if (!/^[0-9a-f]{4,64}$/.test(oid) || !Number.isInteger(args.prNumber)) { @@ -50,8 +49,8 @@ export async function isCommitPartOfMergedPR(args: { } const owner = args.ownerRepo.owner const repo = args.ownerRepo.repo - const cacheKey = `${githubRepoIdentityKey(args.ownerRepo)}#${args.prNumber}@${oid}` const ghOptions = { ...args.ghOptions, ...githubHostExecOptions(args.ownerRepo) } + const cacheKey = `${githubReadExecutionScope(ghOptions)}\0${githubRepoIdentityKey(args.ownerRepo)}#${args.prNumber}@${oid}` const now = Date.now() pruneMergedPRCommitMembershipCache(now) const cached = membershipCache.get(cacheKey) diff --git a/src/main/github/pr-start-point.ts b/src/main/github/pr-start-point.ts index 17d83232824..b131725c7f4 100644 --- a/src/main/github/pr-start-point.ts +++ b/src/main/github/pr-start-point.ts @@ -1,3 +1,4 @@ +import { withGitHubRepoExecution } from './github-repository-identity' import type { IssueSourcePreference } from '../../shared/repo-types' import type { GhAccountBinding } from '../../shared/github/account-binding' import type { GitHubPrStartPoint, GitPushTarget } from '../../shared/worktree/types' @@ -36,168 +37,155 @@ type ResolveGitHubPrStartPointResult = GitHubPrStartPoint | { error: string } export async function resolveGitHubPrStartPoint( args: ResolveGitHubPrStartPointArgs ): Promise { - let headRefName = args.headRefName?.trim() ?? '' - let baseRefName = args.baseRefName?.trim() ?? '' - let isCrossRepository = args.isCrossRepository === true - let pushTarget: GitPushTarget | undefined - let maintainerCanModify: boolean | undefined + return withGitHubRepoExecution( + args.repoPath, + args.connectionId, + args.localGitOptions ?? {}, + async (): Promise => { + let headRefName = args.headRefName?.trim() ?? '' + let baseRefName = args.baseRefName?.trim() ?? '' + let isCrossRepository = args.isCrossRepository === true + let pushTarget: GitPushTarget | undefined + let maintainerCanModify: boolean | undefined - const resolvePushTarget = async (): Promise => { - if (pushTarget) { - return - } - try { - const resolved = await getPullRequestPushTarget( - args.repoPath, - args.prNumber, - args.connectionId ?? null, - args.localGitOptions ?? {}, - args.issueSourcePreference - ) - pushTarget = resolved?.pushTarget - maintainerCanModify = resolved?.maintainerCanModify - } catch { - // Missing fork metadata can block push-target discovery while the PR head ref remains fetchable. - pushTarget = undefined - } - } - - if (!headRefName) { - const item = await getWorkItem( - args.repoPath, - args.prNumber, - 'pr', - args.connectionId ?? null, - args.localGitOptions ?? {}, - args.issueSourcePreference - ) - if (!item || item.type !== 'pr') { - return reviewStartPointFailure('github.prNotFound', { number: args.prNumber }) - } - headRefName = (item.branchName ?? '').trim() - baseRefName = (item.baseRefName ?? '').trim() - if (!headRefName) { - return reviewStartPointFailure('github.prHeadBranchMissing', { number: args.prNumber }) - } - if (item.isCrossRepository === true) { - isCrossRepository = true - } - } - - if (isCrossRepository) { - await resolvePushTarget() - } - - let remote: string - try { - remote = await args.resolveRemote() - } catch (error) { - return { error: error instanceof Error ? error.message : 'Could not resolve git remote.' } - } - - const compareBaseRef = baseRefName ? `refs/remotes/${remote}/${baseRefName}` : undefined - - const fetchCompareBaseRef = (): Promise => - fetchCompareBaseRefWithLocalFallback({ - compareBaseRef, - fetchCompareBaseRef: () => args.fetchRemoteTrackingRef(remote, baseRefName), - gitExec: args.gitExec, - logLabel: '[github:resolvePrStartPoint]', - logContext: { remote, baseRefName, prNumber: args.prNumber } - }) - - const fetchPullRequestHeadSha = async (): Promise<{ baseBranch: string } | { error: string }> => { - const pullRef = `refs/pull/${args.prNumber}/head` - // Failed fetches return no ref path, so only soft-keep must independently resolve its identity. - let softKeepLocalRefPromise: Promise | undefined - const resolveSoftKeepLocalRef = (): Promise => { - softKeepLocalRefPromise ??= (async () => { + const resolvePushTarget = async (): Promise => { + if (pushTarget) { + return + } try { - const { stdout } = await args.gitExec(['remote', 'get-url', remote]) - const remoteUrl = stdout.trim() - if (!remoteUrl) { + const resolved = await getPullRequestPushTarget( + args.repoPath, + args.prNumber, + args.connectionId ?? null, + args.localGitOptions ?? {}, + args.issueSourcePreference + ) + pushTarget = resolved?.pushTarget + maintainerCanModify = resolved?.maintainerCanModify + } catch { + // Missing fork metadata can block push-target discovery while the PR head ref remains fetchable. + pushTarget = undefined + } + } + + if (!headRefName) { + const item = await getWorkItem( + args.repoPath, + args.prNumber, + 'pr', + args.connectionId ?? null, + args.localGitOptions ?? {}, + args.issueSourcePreference + ) + if (!item || item.type !== 'pr') { + return reviewStartPointFailure('github.prNotFound', { number: args.prNumber }) + } + headRefName = (item.branchName ?? '').trim() + baseRefName = (item.baseRefName ?? '').trim() + if (!headRefName) { + return reviewStartPointFailure('github.prHeadBranchMissing', { number: args.prNumber }) + } + if (item.isCrossRepository === true) { + isCrossRepository = true + } + } + + if (isCrossRepository) { + await resolvePushTarget() + } + + let remote: string + try { + remote = await args.resolveRemote() + } catch (error) { + return { error: error instanceof Error ? error.message : 'Could not resolve git remote.' } + } + + const compareBaseRef = baseRefName ? `refs/remotes/${remote}/${baseRefName}` : undefined + + const fetchCompareBaseRef = (): Promise => + fetchCompareBaseRefWithLocalFallback({ + compareBaseRef, + fetchCompareBaseRef: () => args.fetchRemoteTrackingRef(remote, baseRefName), + gitExec: args.gitExec, + logLabel: '[github:resolvePrStartPoint]', + logContext: { remote, baseRefName, prNumber: args.prNumber } + }) + + const fetchPullRequestHeadSha = async (): Promise< + { baseBranch: string } | { error: string } + > => { + const pullRef = `refs/pull/${args.prNumber}/head` + // Failed fetches return no ref path, so only soft-keep must independently resolve its identity. + let softKeepLocalRefPromise: Promise | undefined + const resolveSoftKeepLocalRef = (): Promise => { + softKeepLocalRefPromise ??= (async () => { + try { + const { stdout } = await args.gitExec(['remote', 'get-url', remote]) + const remoteUrl = stdout.trim() + if (!remoteUrl) { + return null + } + return githubPullRequestHeadLocalRef( + reviewHeadRemoteRefComponent(remote, remoteUrl), + args.prNumber + ) + } catch { + return null + } + })() + return softKeepLocalRefPromise + } + const resolveDurableHeadSha = async (localRef: string | null): Promise => { + if (!localRef) { + return null + } + try { + const { stdout } = await args.gitExec(['rev-parse', '--verify', `${localRef}^{commit}`]) + return stdout.trim() || null + } catch { return null } - return githubPullRequestHeadLocalRef( - reviewHeadRemoteRefComponent(remote, remoteUrl), - args.prNumber - ) - } catch { - return null } - })() - return softKeepLocalRefPromise - } - const resolveDurableHeadSha = async (localRef: string | null): Promise => { - if (!localRef) { - return null - } - try { - const { stdout } = await args.gitExec(['rev-parse', '--verify', `${localRef}^{commit}`]) - return stdout.trim() || null - } catch { - return null - } - } - try { - const localRef = await args.fetchPullRequestHeadRef(remote, args.prNumber) - const sha = await resolveDurableHeadSha(localRef) - if (!sha) { - return reviewStartPointFailure('github.forkHeadUnresolved', { number: args.prNumber }) - } - return { baseBranch: sha } - } catch (error) { - const message = error instanceof Error ? error.message : String(error) - // Reuse durable heads only for transient transport failures; deleted/auth/stale-relay errors stay actionable. - if (isTransientReviewHeadFetchError(error)) { - const localSha = await resolveDurableHeadSha(await resolveSoftKeepLocalRef()) - if (localSha) { - console.warn( - '[github:resolvePrStartPoint] PR head fetch failed; using durable local ref', - { - remote, - prNumber: args.prNumber, - error: message.split('\n')[0] + try { + const localRef = await args.fetchPullRequestHeadRef(remote, args.prNumber) + const sha = await resolveDurableHeadSha(localRef) + if (!sha) { + return reviewStartPointFailure('github.forkHeadUnresolved', { number: args.prNumber }) + } + return { baseBranch: sha } + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + // Reuse durable heads only for transient transport failures; deleted/auth/stale-relay errors stay actionable. + if (isTransientReviewHeadFetchError(error)) { + const localSha = await resolveDurableHeadSha(await resolveSoftKeepLocalRef()) + if (localSha) { + console.warn( + '[github:resolvePrStartPoint] PR head fetch failed; using durable local ref', + { + remote, + prNumber: args.prNumber, + error: message.split('\n')[0] + } + ) + return { baseBranch: localSha } } - ) - return { baseBranch: localSha } + } + return reviewStartPointFailure('fetchFailed', { + ref: pullRef, + detail: message.split('\n')[0] + }) } } - return reviewStartPointFailure('fetchFailed', { - ref: pullRef, - detail: message.split('\n')[0] - }) - } - } - // Upstream exposes fork heads through refs/pull//head even without a configured fork remote. - if (isCrossRepository) { - const result = await fetchPullRequestHeadSha() - if ('error' in result) { - return result - } - const compareBaseFetched = await fetchCompareBaseRef() - // Keep the contributor's branch name locally; push still targets the fork. - return { - ...result, - ...(compareBaseFetched && compareBaseRef ? { compareBaseRef } : {}), - headSha: result.baseBranch, - branchNameOverride: headRefName, - ...(pushTarget ? { pushTarget } : {}), - ...(maintainerCanModify !== undefined ? { maintainerCanModify } : {}) - } - } - - try { - await args.fetchRemoteTrackingRef(remote, headRefName) - } catch (error) { - const message = error instanceof Error ? error.message : String(error) - // Missing fork metadata can mimic a same-repo branch; only missing-ref errors allow pull-head fallback. - if (isMissingRemoteRefGitError(error)) { - const result = await fetchPullRequestHeadSha() - if (!('error' in result)) { - await resolvePushTarget() + // Upstream exposes fork heads through refs/pull//head even without a configured fork remote. + if (isCrossRepository) { + const result = await fetchPullRequestHeadSha() + if ('error' in result) { + return result + } const compareBaseFetched = await fetchCompareBaseRef() + // Keep the contributor's branch name locally; push still targets the fork. return { ...result, ...(compareBaseFetched && compareBaseRef ? { compareBaseRef } : {}), @@ -207,33 +195,55 @@ export async function resolveGitHubPrStartPoint( ...(maintainerCanModify !== undefined ? { maintainerCanModify } : {}) } } - // Return the actionable pull-head fallback failure instead of the earlier branch miss. - return result + + try { + await args.fetchRemoteTrackingRef(remote, headRefName) + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + // Missing fork metadata can mimic a same-repo branch; only missing-ref errors allow pull-head fallback. + if (isMissingRemoteRefGitError(error)) { + const result = await fetchPullRequestHeadSha() + if (!('error' in result)) { + await resolvePushTarget() + const compareBaseFetched = await fetchCompareBaseRef() + return { + ...result, + ...(compareBaseFetched && compareBaseRef ? { compareBaseRef } : {}), + headSha: result.baseBranch, + branchNameOverride: headRefName, + ...(pushTarget ? { pushTarget } : {}), + ...(maintainerCanModify !== undefined ? { maintainerCanModify } : {}) + } + } + // Return the actionable pull-head fallback failure instead of the earlier branch miss. + return result + } + return reviewStartPointFailure('fetchFailed', { + ref: `${remote}/${headRefName}`, + detail: message.split('\n')[0] + }) + } + + const remoteRef = `${remote}/${headRefName}` + let headSha: string + try { + const { stdout } = await args.gitExec(['rev-parse', '--verify', remoteRef]) + headSha = stdout.trim() + } catch { + return reviewStartPointFailure('remoteRefMissing', { ref: remoteRef }) + } + if (!headSha) { + return reviewStartPointFailure('github.headShaEmpty', { number: args.prNumber }) + } + const compareBaseFetched = await fetchCompareBaseRef() + + return { + baseBranch: headSha, + ...(compareBaseFetched && compareBaseRef ? { compareBaseRef } : {}), + headSha, + branchNameOverride: headRefName, + pushTarget: { remoteName: remote, branchName: headRefName } + } } - return reviewStartPointFailure('fetchFailed', { - ref: `${remote}/${headRefName}`, - detail: message.split('\n')[0] - }) - } - - const remoteRef = `${remote}/${headRefName}` - let headSha: string - try { - const { stdout } = await args.gitExec(['rev-parse', '--verify', remoteRef]) - headSha = stdout.trim() - } catch { - return reviewStartPointFailure('remoteRefMissing', { ref: remoteRef }) - } - if (!headSha) { - return reviewStartPointFailure('github.headShaEmpty', { number: args.prNumber }) - } - const compareBaseFetched = await fetchCompareBaseRef() - - return { - baseBranch: headSha, - ...(compareBaseFetched && compareBaseRef ? { compareBaseRef } : {}), - headSha, - branchNameOverride: headRefName, - pushTarget: { remoteName: remote, branchName: headRefName } - } + ) } diff --git a/src/main/github/project-account-operations.test.ts b/src/main/github/project-account-operations.test.ts new file mode 100644 index 00000000000..506cff00969 --- /dev/null +++ b/src/main/github/project-account-operations.test.ts @@ -0,0 +1,716 @@ +import type { createGitHubPullRequest } from './client/create/create-github-pull-request' +import { afterEach, beforeEach, expect, it, vi } from 'vitest' +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { createFakeSpawnedChild } from '../../shared/__fixtures__/fake-spawned-child' +import { + createStore, + closeTestStores, + testState, + makeRepo +} from '../persistence/persistence-test-harness' + +const { spawnMock, remote } = vi.hoisted(() => ({ spawnMock: vi.fn(), remote: vi.fn() })) +vi.mock('node:child_process', async (original) => ({ ...(await original()), spawn: spawnMock })) +vi.mock('./github-api-repository-remote-probe', () => ({ + getOriginGitHubApiRepository: remote, + getGitHubApiRepositoryForRemote: remote +})) +vi.mock('../git/remote-name-listing', () => ({ shouldProbeGitRemote: async () => false })) +vi.mock('./rate-limit', () => ({ + repositoryRateLimitGuard: () => ({ blocked: false }), + noteRepositoryRateLimitSpend: () => {}, + spendsSharedGitHubComQuota: () => false +})) +vi.mock('./client/list/work-item-search-page', () => ({ usesGraphqlWorkItemSearch: () => false })) +vi.mock('./client/lookup/pr-lookup-rate-limit', () => ({ assertRateLimitBudget: async () => {} })) + +vi.mock('../source-control/forge-provider', () => ({ + detectHostedReviewProvider: async () => 'github', + getForgeProviderForRepository: async () => ({ + id: 'github', + getReviewForBranch: async () => null, + createReview: async (...args: Parameters) => + (await import('./client/create/create-github-pull-request')).createGitHubPullRequest(...args) + }) +})) +vi.mock('../source-control/hosted-review-creation-git-state', async (original) => ({ + ...(await original()), + getCurrentBranch: async () => 'feature', + hasUncommittedChanges: async () => false, + getHostedReviewUpstreamStatus: async () => ({ hasUpstream: true, ahead: 0, behind: 0 }), + baseRefExistsOnRemote: async () => true, + getDefaultBaseRef: async () => 'parent' +})) +vi.mock('../source-control/hosted-review-creation-provider', async (original) => ({ + ...(await original()), + isProviderAuthenticated: async () => true +})) + +import { installGitHubProjectAccounts } from './github-project-account' +import { getPRChecks } from './client/check/get-pr-checks' +import { rerunPRChecks } from './client/check/rerun-pr-checks' +import { getPRFileContents } from './work-item-details' +import { resolveGitHubPrStartPoint } from './pr-start-point' +import { RuntimeGitHubRepositoryQueryCommands } from '../runtime/runtime-github-repository-query-commands' +import { clearGhAccountTokenCacheForTests } from './gh-account-token' +import { clearGhCapabilityStateForTests } from './gh-capability-state' +import { _resetGhRateLimitBreaker } from '../git/gh-rate-limit-breaker' +import { getPRForBranchOutcome } from './client/lookup/pr-for-branch-outcome' +import { createStackedHostedReview } from '../source-control/stacked-hosted-review-creation' +import { hostedReviewOptionArgs } from './pr-refresh-candidate-policy' +import { fetchPullRequestWorkItem } from './client/fetch/work-item-fetch' +import { repositoryMergeMetadataCache } from './client/detect/repository-merge-metadata-cache' +import { + getCachedGitHubPRStackSummary, + _resetPRStackSummaryCacheForTests +} from './client/lookup/pr-stack-summary-cache' +import { hydrateGitHubPRStack, _resetGitHubPRStackCacheForTests } from './github-pr-stack' +import { resetMergedPRCommitMembershipCacheForTest } from './merged-pr-commit-membership' +import { ghRepoExecOptions, githubRepoContext } from './github-repository-identity' + +const alice = { host: 'github.com', user: 'Alice' } +const bob = { host: 'github.com', user: 'Bob' } +const repository = { host: 'github.com', owner: 'acme', repo: 'widgets' } +let store: ReturnType +let release: () => void +let onRead: () => void +let requests: { args: string[]; token: string | undefined }[] +let request: ( + args: string[], + child: ReturnType, + token?: string +) => void +function complete(child: ReturnType, stdout: string) { + child.stdout?.emit('data', Buffer.from(stdout)) + child.emit('exit', 0, null) + child.emit('close', 0, null) +} +function checkResponse(name = 'CI') { + return JSON.stringify({ + data: { + repository: { + pullRequest: { + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + contexts: { + nodes: [ + { + __typename: 'CheckRun', + name, + status: 'COMPLETED', + conclusion: 'FAILURE', + databaseId: 7, + checkSuite: { workflowRun: { databaseId: 99 } } + } + ] + } + } + } + } + ] + } + } + } + } + }) +} +function issue(title: string) { + return { number: 42, title, state: 'open', html_url: 'https://github.com/acme/widgets/issues/42' } +} +function pr(title: string) { + return { + number: 42, + title, + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/42', + headRefName: 'feature', + baseRefName: 'main', + headRepositoryOwner: { login: 'acme' }, + headRepository: { name: 'widgets' }, + statusCheckRollup: [], + mergeable: 'MERGEABLE' + } +} +function rebind() { + store.updateRepo({ id: 'work' }, { ghAccount: bob }) +} +function rebindDuringFirstResolution() { + let first = true + onRead = () => { + if (first) { + first = false + queueMicrotask(rebind) + } + } +} +beforeEach(() => { + testState.dir = mkdtempSync(join(tmpdir(), 'orca-pr35023-')) + const original = createStore() + original.addRepo(makeRepo({ id: 'work', path: '/work', issueSourcePreference: 'origin' })) + original.updateRepo({ id: 'work' }, { ghAccount: { host: ' GitHub.COM ', user: ' Alice ' } }) + original.flush() + store = createStore() + expect(store.getRepo('work')?.ghAccount).toEqual(alice) + clearGhAccountTokenCacheForTests() + clearGhCapabilityStateForTests() + _resetGhRateLimitBreaker() + repositoryMergeMetadataCache.clear() + _resetPRStackSummaryCacheForTests() + _resetGitHubPRStackCacheForTests() + resetMergedPRCommitMembershipCacheForTest() + requests = [] + onRead = () => {} + release = installGitHubProjectAccounts({ + getRepos: () => { + onRead() + return store.getRepos() + } + }) + remote.mockReset().mockResolvedValue(repository) + request = (args, child) => + queueMicrotask(() => complete(child, args.includes('POST') ? '{}' : checkResponse())) + spawnMock + .mockReset() + .mockImplementation((_command, args: string[], options: { env: NodeJS.ProcessEnv }) => { + const child = createFakeSpawnedChild() + if (args[0] === 'auth') { + queueMicrotask(() => + complete( + child, + args.includes('--help') ? '--user' : `${args[args.indexOf('--user') + 1]}-token` + ) + ) + } else { + requests.push({ args, token: options.env.GH_TOKEN }) + request(args, child, options.env.GH_TOKEN) + } + return child + }) +}) +afterEach(async () => { + release() + vi.unstubAllEnvs() + _resetGhRateLimitBreaker() + await closeTestStores() + rmSync(testState.dir, { recursive: true, force: true }) +}) + +it('separates unresolved callers by saved account before coalescing and returns each account result', async () => { + let finish: () => void = () => {} + let started: () => void = () => {} + const dispatched = new Promise((resolve) => { + started = resolve + }) + request = (_args, child, token) => { + if (token === 'Alice-token') { + finish = () => complete(child, checkResponse('Alice CI')) + started() + } else { + queueMicrotask(() => complete(child, checkResponse('Bob CI'))) + } + } + const first = getPRChecks('/work', 42, undefined, repository) + await dispatched + rebind() + const second = getPRChecks('/work', 42, undefined, repository) + // Bob must settle independently while Alice's child is still pending. + await expect(second).resolves.toMatchObject([{ name: 'Bob CI', workflowRunId: 99 }]) + finish() + await expect(first).resolves.toMatchObject([{ name: 'Alice CI', workflowRunId: 99 }]) + expect(requests.map((r) => r.token)).toEqual(['Alice-token', 'Bob-token']) +}) + +it('settles the full rerun read/write on the initial account and uses the rebind on the next operation', async () => { + rebindDuringFirstResolution() + await expect( + rerunPRChecks('/work', 42, { prRepo: repository, failedOnly: true }, null, { ghAccount: bob }) + ).resolves.toEqual({ ok: true, count: 1 }) + expect(requests.map((r) => ({ post: r.args.includes('POST'), token: r.token }))).toEqual([ + { post: false, token: 'Alice-token' }, + { post: true, token: 'Alice-token' } + ]) + await expect( + rerunPRChecks('/work', 42, { prRepo: repository, failedOnly: true }) + ).resolves.toEqual({ ok: true, count: 1 }) + expect(requests.slice(2).map((r) => r.token)).toEqual(['Bob-token', 'Bob-token']) +}) + +it('loads both file versions under one saved selection across repository resolution and future rebind', async () => { + rebindDuringFirstResolution() + request = (args, child, token) => + queueMicrotask(() => + complete(child, `${token}:${args.some((arg) => arg.includes('ref=base')) ? 'base' : 'head'}`) + ) + const input = { + repoPath: '/work', + prRepo: repository, + prNumber: 42, + path: 'src/a.ts', + status: 'modified', + headSha: 'head', + baseSha: 'base' + } as const + await expect(getPRFileContents(input)).resolves.toMatchObject({ + original: 'Alice-token:base', + modified: 'Alice-token:head', + originalIsBinary: false, + modifiedIsBinary: false + }) + await expect(getPRFileContents(input)).resolves.toMatchObject({ + original: 'Bob-token:base', + modified: 'Bob-token:head' + }) +}) + +it('carries reloaded selection through runtime work-item details/list/count and returns fresh unbound results', async () => { + request = (args, child, token) => { + const title = token ?? 'ambient' + let output: unknown + if (args.includes('.total_count')) { + output = token === 'Alice-token' ? 7 : 13 + } else if (args[0] === 'pr') { + output = [pr(title)] + } else if (args.some((arg) => arg.includes('search/issues'))) { + output = [issue(title)] + } else if (args.includes('graphql')) { + output = { + data: { + repository: { + issue: { body: title, comments: { nodes: [] }, participants: { nodes: [] } } + } + } + } + } else if (args.some((arg) => arg.includes('/timeline'))) { + output = [] + } else { + output = issue(title) + } + queueMicrotask(() => complete(child, JSON.stringify(output))) + } + const runtime = new RuntimeGitHubRepositoryQueryCommands({ + resolveRepo: async () => { + const repo = store.getRepo('work') + if (!repo) { + throw new Error('missing fixture') + } + return repo + }, + getLocalGitArgs: () => [] + }) + let finish: () => void = () => {} + let started: () => void = () => {} + const resolving = new Promise((resolve) => { + started = resolve + }) + remote.mockImplementationOnce( + () => + new Promise((resolve) => { + finish = () => resolve(repository) + started() + }) + ) + const pending = runtime.getRepoWorkItemDetails('work', 42, 'issue') + await resolving + rebind() + finish() + await expect(pending).resolves.toMatchObject({ + item: { title: 'Alice-token' }, + body: 'Alice-token' + }) + await expect(runtime.getRepoWorkItem('work', 42, 'issue')).resolves.toMatchObject({ + title: 'Bob-token' + }) + store.updateRepo({ id: 'work' }, { ghAccount: alice }) + rebindDuringFirstResolution() + await expect(runtime.listRepoWorkItems('work')).resolves.toMatchObject({ + items: [{ title: 'Alice-token' }, { title: 'Alice-token' }] + }) + store.updateRepo({ id: 'work' }, { ghAccount: alice }) + rebindDuringFirstResolution() + await expect(runtime.countRepoWorkItems('work')).resolves.toBe(7) + await expect(runtime.countRepoWorkItems('work')).resolves.toBe(13) + expect(requests.at(-1)?.token).toBe('Bob-token') + store.updateRepo({ id: 'work' }, { ghAccount: null }) + await expect(runtime.getRepoWorkItem('work', 42, 'issue')).resolves.toMatchObject({ + title: expect.not.stringMatching(/Alice|Bob/) + }) +}) + +it('returns the branch review and worktree start point from account-free refresh inputs', async () => { + vi.stubEnv('GH_TOKEN', 'personal-token') + request = (args, child, token) => { + const output = + args[0] === 'pr' + ? args[1] === 'list' + ? [pr(token ?? 'ambient')] + : pr(token ?? 'ambient') + : args.some((arg) => arg.includes('/pulls?')) + ? [ + { + number: 42, + title: token, + state: 'open', + html_url: 'https://github.com/acme/widgets/pull/42', + head: { ref: 'feature', repo: { name: 'widgets', owner: { login: 'acme' } } }, + base: { ref: 'main' } + } + ] + : args.includes('graphql') + ? { data: { repository: {} } } + : args.some((arg) => arg.includes('/stacks')) + ? [] + : pr(token ?? 'ambient') + queueMicrotask(() => + complete( + child, + `${args.includes('--include') ? 'HTTP/2.0 200 OK\r\n\r\n' : ''}${JSON.stringify(output)}` + ) + ) + } + const localGitOptions = { ghAccount: alice, wslDistro: undefined } + const [refresh] = hostedReviewOptionArgs({ localGitOptions }) + expect(refresh?.localGitExecOptions?.ghAccount).toBeUndefined() + const outcome = await getPRForBranchOutcome('/work', 'feature', null, null, null, refresh) + expect(outcome).toMatchObject({ kind: 'found', pr: { title: 'Alice-token', number: 42 } }) + const fetched: string[] = [] + await expect( + resolveGitHubPrStartPoint({ + repoPath: '/work', + prNumber: 42, + gitExec: async () => ({ stdout: 'head-sha', stderr: '' }), + resolveRemote: async () => 'origin', + fetchRemoteTrackingRef: async (remoteName, branch) => { + fetched.push(`${remoteName}/${branch}`) + }, + fetchPullRequestHeadRef: async () => 'refs/pull/42/head' + }) + ).resolves.toMatchObject({ baseBranch: 'head-sha', headSha: 'head-sha' }) + expect(fetched).toContain('origin/feature') + expect(requests.every((r) => r.token === 'Alice-token')).toBe(true) +}) + +it('settles stack prepare/create/register with one saved selection after a pending rebind', async () => { + let finish: () => void = () => {} + let started: () => void = () => {} + const resolving = new Promise((resolve) => { + started = resolve + }) + remote.mockImplementationOnce( + () => + new Promise((resolve) => { + finish = () => resolve(repository) + started() + }) + ) + request = (args, child) => { + let output: unknown = [] + if (args[0] === 'pr' && args[1] === 'create') { + output = { number: 43, url: 'https://github.com/acme/widgets/pull/43' } + } else if (args.some((arg) => arg.includes('pulls?') && arg.includes('%3Aparent'))) { + output = [ + { + number: 42, + html_url: 'https://github.com/acme/widgets/pull/42', + head: { ref: 'parent' }, + base: { ref: 'main' } + } + ] + } else if (args.includes('POST')) { + output = { number: 50 } + } + queueMicrotask(() => complete(child, JSON.stringify(output))) + } + const pending = createStackedHostedReview( + '/work', + { provider: 'github', base: 'parent', head: 'feature', title: 'Change' }, + 'local' + ) + await resolving + rebind() + finish() + await expect(pending).resolves.toMatchObject({ + ok: true, + number: 43, + stackNumber: 50, + parentReview: { number: 42 } + }) + expect(requests.some((r) => r.args[0] === 'pr' && r.args[1] === 'create')).toBe(true) + expect(requests.some((r) => r.args.includes('POST'))).toBe(true) + expect(requests.every((r) => r.token === 'Alice-token')).toBe(true) +}) + +it('returns each saved viewer merge preference with omitted detector scope and reuses only that viewer', async () => { + request = (args, child, token) => + queueMicrotask(() => + complete( + child, + JSON.stringify( + args.includes('graphql') + ? { + data: { + repository: { + viewerDefaultMergeMethod: token === 'Alice-token' ? 'MERGE' : 'SQUASH', + mergeCommitAllowed: true, + rebaseMergeAllowed: true, + squashMergeAllowed: true + } + } + } + : pr(token ?? 'ambient') + ) + ) + ) + const read = () => fetchPullRequestWorkItem('/work', repository, 42) + await expect(read()).resolves.toMatchObject({ + title: 'Alice-token', + mergeMethodSettings: { defaultMethod: 'merge' } + }) + await expect(read()).resolves.toMatchObject({ mergeMethodSettings: { defaultMethod: 'merge' } }) + rebind() + await expect(read()).resolves.toMatchObject({ + title: 'Bob-token', + mergeMethodSettings: { defaultMethod: 'squash' } + }) + await expect(read()).resolves.toMatchObject({ mergeMethodSettings: { defaultMethod: 'squash' } }) + expect(requests.filter((r) => r.args.includes('graphql')).map((r) => r.token)).toEqual([ + 'Alice-token', + 'Bob-token' + ]) +}) + +it.each(['error', 'absent'] as const)( + 'separates pending and retained %s stack summaries by held account, joining and reusing identical reads', + async (mode) => { + let finish: () => void = () => {} + let started: () => void = () => {} + const dispatched = new Promise((resolve) => { + started = resolve + }) + const summary = { number: 51, position: 1, size: 2, baseRefName: 'main' } + request = (_args, child, token) => { + const respond = () => { + if (token === 'Alice-token' && mode === 'error') { + child.stderr?.emit('data', Buffer.from('request failed')) + child.emit('exit', 1, null) + child.emit('close', 1, null) + } else { + complete( + child, + `HTTP/2.0 200 OK\r\n\r\n${JSON.stringify({ + number: 42, + title: token, + state: 'open', + ...(token === 'Bob-token' + ? { stack: { number: 51, position: 1, size: 2, base: { ref: 'main' } } } + : {}) + })}` + ) + } + } + if (token === 'Alice-token') { + finish = respond + started() + } else { + queueMicrotask(respond) + } + } + const read = () => + getCachedGitHubPRStackSummary( + repository, + 42, + ghRepoExecOptions(githubRepoContext('/work')), + 'local:host' + ) + const first = read().then( + (value) => value, + () => 'failed' + ) + await dispatched + const same = read().then( + (value) => value, + () => 'failed' + ) + rebind() + let bobSettled = false + const second = read().then( + (value) => { + bobSettled = true + return value + }, + () => 'failed' + ) + const secondJoin = read().then( + (value) => value, + () => 'failed' + ) + // Let Bob's real child settle while Alice remains pending. + await new Promise((resolve) => setImmediate(resolve)) + const independent = bobSettled + finish() + expect(await first).toEqual(mode === 'error' ? 'failed' : undefined) + expect(await same).toEqual(mode === 'error' ? 'failed' : undefined) + await expect(second).resolves.toEqual(summary) + await expect(secondJoin).resolves.toEqual(summary) + expect(independent).toBe(true) + await expect(read()).resolves.toEqual(summary) + store.updateRepo({ id: 'work' }, { ghAccount: alice }) + expect( + await read().then( + (value) => value, + () => 'failed' + ) + ).toEqual(mode === 'error' ? 'failed' : undefined) + expect(requests.map((r) => r.token)).toEqual(['Alice-token', 'Bob-token']) + } +) + +it.each(['error', 'absent'] as const)( + 'separates pending and retained %s stack details by held account, joining and reusing identical reads', + async (mode) => { + let finish: () => void = () => {} + let started: () => void = () => {} + const dispatched = new Promise((resolve) => { + started = resolve + }) + const summary = { number: 51, position: 1, size: 2, baseRefName: 'main' } + request = (_args, child, token) => { + const respond = () => { + if (token === 'Alice-token' && mode === 'error') { + child.stderr?.emit('data', Buffer.from('request failed')) + child.emit('exit', 1, null) + child.emit('close', 1, null) + } else { + complete( + child, + JSON.stringify({ + data: { + repository: { + pullRequest: { + stack: + token === 'Alice-token' + ? null + : { + ...summary, + entries: { nodes: [{ position: 1, pullRequest: pr('Bob stack') }] } + } + } + } + } + }) + ) + } + } + if (token === 'Alice-token') { + finish = respond + started() + } else { + queueMicrotask(respond) + } + } + const read = () => + hydrateGitHubPRStack(repository, 42, summary, ghRepoExecOptions(githubRepoContext('/work'))) + const first = read() + await dispatched + const same = read() + rebind() + let bobSettled = false + const second = read().then((value) => { + bobSettled = true + return value + }) + const secondJoin = read() + await new Promise((resolve) => setImmediate(resolve)) + const independent = bobSettled + finish() + await expect(first).resolves.toEqual(summary) + await expect(same).resolves.toEqual(summary) + for (const result of [second, secondJoin, read()]) { + await expect(result).resolves.toMatchObject({ + entries: [{ title: 'Bob stack', number: 42, position: 1 }] + }) + } + expect(independent).toBe(true) + store.updateRepo({ id: 'work' }, { ghAccount: alice }) + await expect(read()).resolves.toEqual(summary) + expect(requests.map((r) => r.token)).toEqual(['Alice-token', 'Bob-token']) + } +) + +it('recovers visible merged review through Bob complete membership iteration after Alice error and reuses Bob confirmation', async () => { + request = (args, child, token) => + queueMicrotask(() => { + const membership = args.find((arg) => arg.includes('/commits/')) + if (membership && token === 'Alice-token') { + child.stderr?.emit('data', Buffer.from('request failed')) + child.emit('exit', 1, null) + child.emit('close', 1, null) + return + } + const merged = { ...pr(token ?? 'ambient'), state: 'MERGED', headRefOid: 'aaaa1111aaaa1111' } + const restMerged = { + number: 42, + title: token, + state: 'closed', + merged_at: '2026-10-01T00:00:00Z', + html_url: merged.url, + head: { + ref: 'feature', + sha: 'aaaa1111aaaa1111', + repo: { name: 'widgets', owner: { login: 'acme' } } + }, + base: { ref: 'main' } + } + const output = membership + ? membership.endsWith('page=1') + ? Array.from({ length: 100 }, (_, i) => ({ number: 1000 + i })) + : [{ number: 42 }] + : args.includes('graphql') + ? { data: { repository: {} } } + : args.some((arg) => arg.includes('/pulls?')) + ? [restMerged] + : args[1] === 'list' + ? [merged] + : merged + complete( + child, + (args.includes('--include') ? 'HTTP/2.0 200 OK\r\n\r\n' : '') + JSON.stringify(output) + ) + }) + const read = () => + getPRForBranchOutcome('/work', 'feature', null, null, null, { + currentHeadOid: 'bbbb2222bbbb2222' + }) + await expect(read()).resolves.toMatchObject({ kind: 'no-pr' }) + rebind() + await expect(read()).resolves.toMatchObject({ + kind: 'found', + pr: { title: 'Bob-token', state: 'merged', confirmedContainedHeadOid: 'bbbb2222bbbb2222' } + }) + await expect(read()).resolves.toMatchObject({ + kind: 'found', + pr: { confirmedContainedHeadOid: 'bbbb2222bbbb2222' } + }) + expect( + requests + .filter((r) => r.args.some((arg) => arg.includes('/commits/'))) + .map((r) => ({ endpoint: r.args.find((arg) => arg.includes('/commits/')), token: r.token })) + ).toEqual([ + { + endpoint: 'repos/acme/widgets/commits/bbbb2222bbbb2222/pulls?per_page=100&page=1', + token: 'Alice-token' + }, + { + endpoint: 'repos/acme/widgets/commits/bbbb2222bbbb2222/pulls?per_page=100&page=1', + token: 'Bob-token' + }, + { + endpoint: 'repos/acme/widgets/commits/bbbb2222bbbb2222/pulls?per_page=100&page=2', + token: 'Bob-token' + } + ]) +}) diff --git a/src/main/github/project-bound-gh-requests.test.ts b/src/main/github/project-bound-gh-requests.test.ts new file mode 100644 index 00000000000..53be5be8ce4 --- /dev/null +++ b/src/main/github/project-bound-gh-requests.test.ts @@ -0,0 +1,121 @@ +import { afterEach, beforeEach, expect, it, vi } from 'vitest' +import { createFakeSpawnedChild } from '../../shared/__fixtures__/fake-spawned-child' +import type { Repo } from '../../shared/repo-types' + +const { spawnMock, requestMock } = vi.hoisted(() => ({ spawnMock: vi.fn(), requestMock: vi.fn() })) + +vi.mock('node:child_process', async (importOriginal) => ({ + ...(await importOriginal()), + spawn: spawnMock +})) +vi.mock('./github-api-repository', () => ({ + getOriginGitHubApiRepository: vi.fn(async () => ({ + owner: 'acme', + repo: 'widgets', + host: 'github.com' + })), + githubHostExecOptions: () => ({ host: 'github.com' }) +})) + +import { _resetGhRateLimitBreaker } from '../git/gh-rate-limit-breaker' +import { installGitHubProjectAccounts } from './github-project-account' +import { createGitHubPullRequest } from './client/create/create-github-pull-request' +import { clearGhAccountTokenCacheForTests } from './gh-account-token' +import { clearGhCapabilityStateForTests } from './gh-capability-state' +import { runGraphql } from './project-view/internals' + +let releaseAccounts: (() => void) | undefined +let repo: Repo +beforeEach(() => { + _resetGhRateLimitBreaker() + clearGhAccountTokenCacheForTests() + clearGhCapabilityStateForTests() + requestMock.mockReset() + spawnMock.mockReset() + spawnMock.mockImplementation((_command, args: string[], options: { env: NodeJS.ProcessEnv }) => { + if (args[0] !== 'auth') { + return requestMock(args, options) + } + const child = createFakeSpawnedChild() + const output = args.includes('--help') ? '--user' : `${args[args.indexOf('--user') + 1]}-token` + queueMicrotask(() => complete(child, output)) + return child + }) + repo = { + id: 'work', + displayName: 'Work', + path: '/work', + badgeColor: '#000000', + addedAt: 0, + ghAccount: { host: 'github.com', user: 'Alice' } + } + releaseAccounts = installGitHubProjectAccounts({ getRepos: () => [repo] }) +}) +afterEach(() => { + releaseAccounts?.() + _resetGhRateLimitBreaker() + vi.unstubAllEnvs() +}) + +function complete(child: ReturnType, stdout: string): void { + child.stdout?.emit('data', Buffer.from(stdout)) + child.emit('exit', 0, null) + child.emit('close', 0, null) +} + +it('keeps Project View ambient even with a bound project cwd', async () => { + vi.stubEnv('GH_TOKEN', 'personal-token') + let selectedPersonal = false + requestMock.mockImplementation((_args, options: { env: NodeJS.ProcessEnv }) => { + selectedPersonal = options.env.GH_TOKEN === 'personal-token' + const child = createFakeSpawnedChild() + queueMicrotask(() => complete(child, '{"data":{"viewer":{"login":"personal"}}}')) + return child + }) + await expect( + runGraphql('query { viewer { login } }', {}, { cwd: repo.path, host: 'github.com' }) + ).resolves.toMatchObject({ ok: true, data: { viewer: { login: 'personal' } } }) + expect(selectedPersonal).toBe(true) + expect(spawnMock.mock.calls.some(([, args]) => args[0] === 'auth')).toBe(false) +}) + +it('keeps creation recovery on its original account while the next operation reads a rebind', async () => { + const tokens: (string | undefined)[] = [] + let startCreate: (finish: () => void) => void = () => {} + const started = new Promise<() => void>((resolve) => { + startCreate = resolve + }) + let firstCreate = true + requestMock.mockImplementation((args: string[], options: { env: NodeJS.ProcessEnv }) => { + tokens.push(options.env.GH_TOKEN) + const child = createFakeSpawnedChild() + if (args[0] === 'pr' && args[1] === 'create' && firstCreate) { + firstCreate = false + startCreate(() => complete(child, 'unconfirmed')) + } else { + queueMicrotask(() => + complete( + child, + JSON.stringify( + args[1] === 'list' + ? [{ number: 42, url: 'https://github.com/acme/widgets/pull/42' }] + : { number: 43, url: 'https://github.com/acme/widgets/pull/43' } + ) + ) + ) + } + return child + }) + const input = { provider: 'github', base: 'main', head: 'feature', title: 'Change' } as const + const pending = createGitHubPullRequest(repo.path, input, 'local') + const finish = await started + repo = { ...repo, ghAccount: { host: 'github.com', user: 'Bob' } } + finish() + await expect(pending).resolves.toMatchObject({ ok: true, number: 42 }) + expect(tokens).toEqual(['Alice-token', 'Alice-token']) + await expect(createGitHubPullRequest(repo.path, input, 'local')).resolves.toMatchObject({ + ok: true, + number: 43 + }) + expect(tokens.at(-1)).toBe('Bob-token') +}) diff --git a/src/main/github/pull-request-file-contents.ts b/src/main/github/pull-request-file-contents.ts index a018e8572a1..6b2817fdc2c 100644 --- a/src/main/github/pull-request-file-contents.ts +++ b/src/main/github/pull-request-file-contents.ts @@ -1,3 +1,4 @@ +import { withGitHubRepoExecution } from './github-repository-identity' import type { GitHubPRFile, GitHubPRFileContents } from '../../shared/github/pull-request-types' import { acquire, ghExecFileAsync, ghRepoExecOptions, githubRepoContext, release } from './gh-utils' import type { LocalGitExecOptions } from './gh-utils' @@ -69,63 +70,70 @@ export async function getPRFileContents(args: { headSha: string baseSha: string }): Promise { - const { ownerRepo } = await resolveGitHubRepoExecution( + return withGitHubRepoExecution( args.repoPath, - args.prRepo, args.connectionId, - args.localGitOptions - ) - if (!ownerRepo) { - return { - original: '', - modified: '', - originalIsBinary: false, - modifiedIsBinary: false - } - } + args.localGitOptions ?? {}, + async () => { + const { ownerRepo } = await resolveGitHubRepoExecution( + args.repoPath, + args.prRepo, + args.connectionId, + args.localGitOptions + ) + if (!ownerRepo) { + return { + original: '', + modified: '', + originalIsBinary: false, + modifiedIsBinary: false + } + } - await acquire() - try { - const needsOriginal = args.status !== 'added' - const needsModified = args.status !== 'removed' - const originalPath = args.oldPath ?? args.path - const [original, modified] = await Promise.all([ - needsOriginal - ? fetchContentAtRef({ - repoPath: args.repoPath, - connectionId: args.connectionId, - localGitOptions: args.localGitOptions, - ownerRepo, - path: originalPath, - ref: args.baseSha - }) - : Promise.resolve<{ content: string; isBinary: boolean; tooLarge?: boolean }>({ - content: '', - isBinary: false - }), - needsModified - ? fetchContentAtRef({ - repoPath: args.repoPath, - connectionId: args.connectionId, - localGitOptions: args.localGitOptions, - ownerRepo, - path: args.path, - ref: args.headSha - }) - : Promise.resolve<{ content: string; isBinary: boolean; tooLarge?: boolean }>({ - content: '', - isBinary: false - }) - ]) - return { - original: original.content, - modified: modified.content, - originalIsBinary: original.isBinary, - modifiedIsBinary: modified.isBinary, - originalTooLarge: original.tooLarge, - modifiedTooLarge: modified.tooLarge + await acquire() + try { + const needsOriginal = args.status !== 'added' + const needsModified = args.status !== 'removed' + const originalPath = args.oldPath ?? args.path + const [original, modified] = await Promise.all([ + needsOriginal + ? fetchContentAtRef({ + repoPath: args.repoPath, + connectionId: args.connectionId, + localGitOptions: args.localGitOptions, + ownerRepo, + path: originalPath, + ref: args.baseSha + }) + : Promise.resolve<{ content: string; isBinary: boolean; tooLarge?: boolean }>({ + content: '', + isBinary: false + }), + needsModified + ? fetchContentAtRef({ + repoPath: args.repoPath, + connectionId: args.connectionId, + localGitOptions: args.localGitOptions, + ownerRepo, + path: args.path, + ref: args.headSha + }) + : Promise.resolve<{ content: string; isBinary: boolean; tooLarge?: boolean }>({ + content: '', + isBinary: false + }) + ]) + return { + original: original.content, + modified: modified.content, + originalIsBinary: original.isBinary, + modifiedIsBinary: modified.isBinary, + originalTooLarge: original.tooLarge, + modifiedTooLarge: modified.tooLarge + } + } finally { + release() + } } - } finally { - release() - } + ) } diff --git a/src/main/github/stacked-pr-creation.ts b/src/main/github/stacked-pr-creation.ts index d4803ac3e67..126ccaee27f 100644 --- a/src/main/github/stacked-pr-creation.ts +++ b/src/main/github/stacked-pr-creation.ts @@ -58,35 +58,25 @@ function ghOptions( } async function findOpenPullRequestsForBranch( - repoPath: string, repository: GitHubApiRepository, branch: string, - connectionId?: string | null, - options: HostedReviewExecutionOptions = {}, + execOptions: ReturnType, base?: string ): Promise { const head = encodeURIComponent(`${repository.owner}:${branch}`) const baseQuery = base ? `&base=${encodeURIComponent(base)}` : '' const endpoint = `repos/${repository.owner}/${repository.repo}/pulls?head=${head}${baseQuery}&state=open&per_page=2` - const { stdout } = await ghExecFileAsync( - ['api', endpoint], - ghOptions(repoPath, repository, connectionId, options) - ) + const { stdout } = await ghExecFileAsync(['api', endpoint], execOptions) return parseGitHubStackPullRequests(stdout) } async function getStacksForPullRequest( - repoPath: string, repository: GitHubApiRepository, pullRequestNumber: number, - connectionId?: string | null, - options: HostedReviewExecutionOptions = {} + execOptions: ReturnType ): Promise { const endpoint = `repos/${repository.owner}/${repository.repo}/stacks?pull_request=${pullRequestNumber}` - const { stdout } = await ghExecFileAsync( - ['api', endpoint], - ghOptions(repoPath, repository, connectionId, options) - ) + const { stdout } = await ghExecFileAsync(['api', endpoint], execOptions) return parseGitHubStacks(stdout) } @@ -121,6 +111,7 @@ export async function prepareGitHubStackedPullRequest( } // `gh` runs on this client whatever the host; only the git reads under it are routed. const connectionId = hostedReviewSshConnectionId(executionHostId) + const context = githubRepoContext(repoPath, connectionId, getHostedReviewLocalGitOptions(options)) const repository = await getOriginGitHubApiRepository( repoPath, connectionId, @@ -129,6 +120,11 @@ export async function prepareGitHubStackedPullRequest( if (!repository || !isDefaultGitHubHost(repository.host)) { return stackedReviewFailure('stack_github_com_only') } + const execOptions = { + ...ghRepoExecOptions(context), + ...githubHostExecOptions(repository), + timeout: 60_000 + } const base = normalizeHostedReviewBaseRef(input.base).trim() const head = input.head ? normalizeHostedReviewHeadRef(input.head).trim() : '' if (!base || !head || base.toLowerCase() === head.toLowerCase()) { @@ -138,8 +134,8 @@ export async function prepareGitHubStackedPullRequest( await acquire() try { const [parentPullRequests, currentPullRequests] = await Promise.all([ - findOpenPullRequestsForBranch(repoPath, repository, base, connectionId, options), - findOpenPullRequestsForBranch(repoPath, repository, head, connectionId, options, base) + findOpenPullRequestsForBranch(repository, base, execOptions), + findOpenPullRequestsForBranch(repository, head, execOptions, base) ]) if (parentPullRequests.length === 0) { return stackedReviewFailure('stack_parent_without_review', { base }) @@ -153,9 +149,9 @@ export async function prepareGitHubStackedPullRequest( const parentReview = parentPullRequests[0] const currentReview = currentPullRequests[0] ?? null const [parentStacks, currentStacks] = await Promise.all([ - getStacksForPullRequest(repoPath, repository, parentReview.number, connectionId, options), + getStacksForPullRequest(repository, parentReview.number, execOptions), currentReview - ? getStacksForPullRequest(repoPath, repository, currentReview.number, connectionId, options) + ? getStacksForPullRequest(repository, currentReview.number, execOptions) : Promise.resolve([]) ]) if ( @@ -219,23 +215,12 @@ export async function registerGitHubStackedPullRequest(args: { }): Promise { const options = args.options ?? {} const connectionId = hostedReviewSshConnectionId(args.executionHostId) + const execOptions = ghOptions(args.repoPath, args.repository, connectionId, options) await acquire() try { const [parentStacks, currentStacks] = await Promise.all([ - getStacksForPullRequest( - args.repoPath, - args.repository, - args.parentReview.number, - connectionId, - options - ), - getStacksForPullRequest( - args.repoPath, - args.repository, - args.currentReview.number, - connectionId, - options - ) + getStacksForPullRequest(args.repository, args.parentReview.number, execOptions), + getStacksForPullRequest(args.repository, args.currentReview.number, execOptions) ]) const existingStackNumber = registeredStackNumber( args.parentReview, @@ -274,7 +259,7 @@ export async function registerGitHubStackedPullRequest(args: { command.push('-F', `pull_requests[]=${pullRequest}`) } const { stdout } = await ghExecFileAsync(command, { - ...ghOptions(args.repoPath, args.repository, connectionId, options), + ...execOptions, idempotent: false }) const stackNumber = Number((JSON.parse(stdout) as { number?: unknown }).number) diff --git a/src/main/github/work-item-details.ts b/src/main/github/work-item-details.ts index 147db244d54..8d47b1a868e 100644 --- a/src/main/github/work-item-details.ts +++ b/src/main/github/work-item-details.ts @@ -1,3 +1,4 @@ +import { withGitHubRepoExecution } from './github-repository-identity' import type { PRCheckDetail } from '../../shared/github/check-types' import type { GitHubPRFile, GitHubPRFileContents } from '../../shared/github/pull-request-types' import type { GitHubWorkItemDetails } from '../../shared/github/work-item-types' @@ -70,169 +71,176 @@ export async function getWorkItemDetails( preference?: IssueSourcePreference, repositoryOverride?: unknown ): Promise { - let issueRepositoryOverride: GitHubApiRepository | null = null - if (type === 'issue' && repositoryOverride != null) { - if (!isValidGitHubApiRepository(repositoryOverride)) { - return null - } - issueRepositoryOverride = ( - await resolveGitHubRepoExecution(repoPath, repositoryOverride, connectionId, localGitOptions) - ).ownerRepo - if (!issueRepositoryOverride) { - return null - } - } - const { - item, - repository: selectedRepository, - issueDetails - } = issueRepositoryOverride - ? { - item: await getWorkItemByOwnerRepo( + return withGitHubRepoExecution(repoPath, connectionId, localGitOptions, async () => { + let issueRepositoryOverride: GitHubApiRepository | null = null + if (type === 'issue' && repositoryOverride != null) { + if (!isValidGitHubApiRepository(repositoryOverride)) { + return null + } + issueRepositoryOverride = ( + await resolveGitHubRepoExecution( repoPath, - issueRepositoryOverride, - number, - 'issue', + repositoryOverride, connectionId, localGitOptions - ), - repository: issueRepositoryOverride, - issueDetails: undefined + ) + ).ownerRepo + if (!issueRepositoryOverride) { + return null } - : await getWorkItemWithRepository( - repoPath, - number, - type, - connectionId, - localGitOptions, - preference, - type === 'issue' - ) - if (!item) { - return null - } - - // Keep the conversation in the same repository as the selected issue. - const resolvedRepository = - item.type === 'issue' - ? selectedRepository - : (await resolveGitHubRepoExecution(repoPath, item.prRepo, connectionId, localGitOptions)) - .ownerRepo - - if (item.type === 'issue') { - return withWorkItemDetailsPermit(async () => { - const collapsed = - issueDetails === undefined - ? await getIssueDetailsViaGraphQL( - repoPath, - item.number, - resolvedRepository, - connectionId, - localGitOptions - ) - : issueDetails - if (collapsed) { - return { - item: enrichItemDisplayAvatars(item, [ - ...collapsed.participants, - ...collapsed.assigneeUsers - ]), - body: collapsed.body, - comments: collapsed.comments, - assignees: collapsed.assignees, - participants: collapsed.participants, - timelineItems: collapsed.timelineItems + } + const { + item, + repository: selectedRepository, + issueDetails + } = issueRepositoryOverride + ? { + item: await getWorkItemByOwnerRepo( + repoPath, + issueRepositoryOverride, + number, + 'issue', + connectionId, + localGitOptions + ), + repository: issueRepositoryOverride, + issueDetails: undefined } - } - const [{ body, comments, assignees, timelineItems }, participants] = await Promise.all([ - getIssueBodyAndComments( + : await getWorkItemWithRepository( repoPath, - item.number, + number, + type, + connectionId, + localGitOptions, + preference, + type === 'issue' + ) + if (!item) { + return null + } + + // Keep the conversation in the same repository as the selected issue. + const resolvedRepository = + item.type === 'issue' + ? selectedRepository + : (await resolveGitHubRepoExecution(repoPath, item.prRepo, connectionId, localGitOptions)) + .ownerRepo + + if (item.type === 'issue') { + return withWorkItemDetailsPermit(async () => { + const collapsed = + issueDetails === undefined + ? await getIssueDetailsViaGraphQL( + repoPath, + item.number, + resolvedRepository, + connectionId, + localGitOptions + ) + : issueDetails + if (collapsed) { + return { + item: enrichItemDisplayAvatars(item, [ + ...collapsed.participants, + ...collapsed.assigneeUsers + ]), + body: collapsed.body, + comments: collapsed.comments, + assignees: collapsed.assignees, + participants: collapsed.participants, + timelineItems: collapsed.timelineItems + } + } + const [{ body, comments, assignees, timelineItems }, participants] = await Promise.all([ + getIssueBodyAndComments( + repoPath, + item.number, + resolvedRepository, + connectionId, + localGitOptions + ), + getWorkItemParticipants(repoPath, item, resolvedRepository, connectionId, localGitOptions) + ]) + const mentionParticipants = await getMentionParticipants( + repoPath, + item, + comments, + participants, resolvedRepository, connectionId, localGitOptions - ), - getWorkItemParticipants(repoPath, item, resolvedRepository, connectionId, localGitOptions) - ]) - const mentionParticipants = await getMentionParticipants( - repoPath, - item, - comments, - participants, - resolvedRepository, - connectionId, - localGitOptions - ) - return { - item: enrichItemDisplayAvatars(item, mentionParticipants), - body, - comments, - assignees, - participants: mentionParticipants, - timelineItems - } - }) - } - - // Nested client reads own their semaphore permits, so they stay outside these permits. - const [{ viewedStates, participants }, metadata, files, comments] = await Promise.all([ - withWorkItemDetailsPermit(() => - getPRDetailContext(repoPath, item.number, resolvedRepository, connectionId, localGitOptions) - ), - withWorkItemDetailsPermit(() => - getPRMetadata(repoPath, item.number, resolvedRepository, connectionId, localGitOptions) - ), - withWorkItemDetailsPermit(() => - getPRFiles(repoPath, item.number, resolvedRepository, connectionId, localGitOptions) - ), - resolvedRepository - ? getPRComments( - repoPath, - item.number, - { prRepo: resolvedRepository }, - connectionId, - ...localGitOptionArgs(localGitOptions) ) - : Promise.resolve([]) - ]) + return { + item: enrichItemDisplayAvatars(item, mentionParticipants), + body, + comments, + assignees, + participants: mentionParticipants, + timelineItems + } + }) + } - const [mentionParticipants, checks] = await Promise.all([ - withWorkItemDetailsPermit(() => - getMentionParticipants( + // Nested client reads own their semaphore permits, so they stay outside these permits. + const [{ viewedStates, participants }, metadata, files, comments] = await Promise.all([ + withWorkItemDetailsPermit(() => + getPRDetailContext(repoPath, item.number, resolvedRepository, connectionId, localGitOptions) + ), + withWorkItemDetailsPermit(() => + getPRMetadata(repoPath, item.number, resolvedRepository, connectionId, localGitOptions) + ), + withWorkItemDetailsPermit(() => + getPRFiles(repoPath, item.number, resolvedRepository, connectionId, localGitOptions) + ), + resolvedRepository + ? getPRComments( + repoPath, + item.number, + { prRepo: resolvedRepository }, + connectionId, + ...localGitOptionArgs(localGitOptions) + ) + : Promise.resolve([]) + ]) + + const [mentionParticipants, checks] = await Promise.all([ + withWorkItemDetailsPermit(() => + getMentionParticipants( + repoPath, + item, + comments, + participants, + resolvedRepository, + connectionId, + localGitOptions + ) + ), + getPRChecksForDetails( repoPath, - item, - comments, - participants, + item.number, + metadata.headSha, resolvedRepository, connectionId, localGitOptions ) - ), - getPRChecksForDetails( - repoPath, - item.number, - metadata.headSha, - resolvedRepository, - connectionId, - localGitOptions - ) - ]) + ]) - return { - item: enrichItemDisplayAvatars( - resolvedRepository ? { ...item, prRepo: resolvedRepository } : item, - mentionParticipants - ), - body: metadata.body, - comments, - headSha: metadata.headSha, - baseSha: metadata.baseSha, - pullRequestId: viewedStates?.pullRequestId, - checks, - files: files === null ? undefined : mergePRFileViewedStates(files, viewedStates), - filesUnavailable: files === null, - participants: mentionParticipants - } + return { + item: enrichItemDisplayAvatars( + resolvedRepository ? { ...item, prRepo: resolvedRepository } : item, + mentionParticipants + ), + body: metadata.body, + comments, + headSha: metadata.headSha, + baseSha: metadata.baseSha, + pullRequestId: viewedStates?.pullRequestId, + checks, + files: files === null ? undefined : mergePRFileViewedStates(files, viewedStates), + filesUnavailable: files === null, + participants: mentionParticipants + } + }) } export async function getPRFileContents(args: { diff --git a/src/main/runtime/construction/runtime-construction.ts b/src/main/runtime/construction/runtime-construction.ts index 3be938d2db5..35d19a37b75 100644 --- a/src/main/runtime/construction/runtime-construction.ts +++ b/src/main/runtime/construction/runtime-construction.ts @@ -19,6 +19,7 @@ import type { ProjectExecutionRuntimeResolution } from '../../../shared/project- import { OrcaRuntimeWithLinearCommands } from '../integrations/linear/linear-commands' import type { RuntimeStore } from '../runtime-store-contract' +import { installGitHubProjectAccounts } from '../../github/github-project-account' import type { WorkspaceSleepPlatform } from '../workspaces/workspace-sleep-platform' import type { StatsCollector } from '../../stats/collector' @@ -118,6 +119,7 @@ export abstract class OrcaRuntimeWithStateFields extends OrcaRuntimeWithLinearCo ) { super() this.store = store + installGitHubProjectAccounts(store, () => this.resolvedWorktrees.peek()?.worktrees ?? []) this.legacyStructuredSessionTabIds = Object.freeze( collectSavedStructuredAgentSessionIds( store?.getWorkspaceSession?.(LOCAL_EXECUTION_HOST_ID) ?? null diff --git a/src/main/source-control/hosted-review-project-account.test.ts b/src/main/source-control/hosted-review-project-account.test.ts new file mode 100644 index 00000000000..71c1190472f --- /dev/null +++ b/src/main/source-control/hosted-review-project-account.test.ts @@ -0,0 +1,216 @@ +import { afterEach, beforeEach, expect, it, vi } from 'vitest' +import { + HOSTED_REVIEW_LOOKUP_DEADLINE_MS, + NO_REVIEW_REFRESH_INTERVAL_MS +} from './hosted-review-refresh-pacing' +import { githubRepoContext } from '../github/github-repository-identity' +import type { Repo } from '../../shared/repo-types' +import { installGitHubProjectAccounts } from '../github/github-project-account' +import { __resetHostedReviewBranchCacheForTests } from './hosted-review-branch-cache' +import { getHostedReviewForBranch } from './hosted-review' + +const { read, detect, probe } = vi.hoisted(() => ({ + read: vi.fn(), + detect: vi.fn(), + probe: vi.fn() +})) +vi.mock('../git/remote-url-probe', () => ({ assertRemoteUrlReadable: probe })) +vi.mock('./forge-provider', () => ({ getForgeProviderForRepository: detect })) +let release: () => void +let repo: Repo +beforeEach(() => { + __resetHostedReviewBranchCacheForTests() + probe.mockReset() + read.mockReset() + detect.mockReset().mockResolvedValue({ id: 'github', getReviewForBranch: read }) + repo = { + id: 'repo', + path: '/repo', + displayName: 'Repo', + badgeColor: '#000000', + addedAt: 0, + ghAccount: { host: 'github.com', user: 'Alice' } + } + release = installGitHubProjectAccounts({ getRepos: () => [repo] }) +}) +afterEach(() => { + release() + vi.useRealTimers() + __resetHostedReviewBranchCacheForTests() +}) + +it('does not reuse a negative review result after the saved account changes or clears', async () => { + read.mockResolvedValue(null) + const input = { repoPath: '/repo', executionHostId: 'local', branch: 'feature' } as const + await getHostedReviewForBranch(input) + await getHostedReviewForBranch(input) + expect(read).toHaveBeenCalledTimes(1) + expect(read.mock.calls[0][0].localGitExecOptions.ghAccount.user).toBe('Alice') + repo = { ...repo, ghAccount: { host: 'github.com', user: 'Bob' } } + await getHostedReviewForBranch(input) + expect(read).toHaveBeenCalledTimes(2) + expect(read.mock.calls[1][0].localGitExecOptions.ghAccount.user).toBe('Bob') + repo = { ...repo, ghAccount: undefined } + await getHostedReviewForBranch(input) + expect(read).toHaveBeenCalledTimes(3) + expect(read.mock.calls[2][0].localGitExecOptions.ghAccount).toBeUndefined() +}) + +it('leaves other providers outside GitHub account selection', async () => { + detect.mockResolvedValue({ id: 'gitlab', getReviewForBranch: read }) + read.mockResolvedValue(null) + await getHostedReviewForBranch({ repoPath: '/repo', executionHostId: 'local', branch: 'feature' }) + expect(read.mock.calls[0][0].localGitExecOptions).toBeUndefined() +}) + +it.each(['github', 'gitlab'] as const)( + 'keeps %s discovery inside cache, failure/backoff and total deadline', + async (id) => { + vi.useFakeTimers() + const review = { provider: id, title: 'Last good' } + detect.mockResolvedValue({ id, getReviewForBranch: read }) + read.mockResolvedValue(review) + const localGitExecOptions = { ghAccount: repo.ghAccount, admissionTier: 'background' } as const + const input = { + repoPath: '/repo', + executionHostId: 'local', + branch: 'feature', + localGitExecOptions + } as const + await expect(getHostedReviewForBranch(input)).resolves.toEqual(review) + expect(detect.mock.calls[0][0]).toEqual({ + repoPath: input.repoPath, + executionHostId: input.executionHostId, + localGitExecOptions + }) + expect(read.mock.calls[0][0]).toMatchObject({ branch: 'feature', localGitExecOptions }) + detect.mockResolvedValue(null) + probe.mockRejectedValue(new Error('Remote discovery unavailable')) + // Same fresh hit as the old path: no discovery/probe, regardless of new detector outcome. + await expect(getHostedReviewForBranch(input)).resolves.toEqual(review) + expect(detect).toHaveBeenCalledTimes(1) + await vi.advanceTimersByTimeAsync(NO_REVIEW_REFRESH_INTERVAL_MS) + await expect(getHostedReviewForBranch(input)).resolves.toEqual(review) + expect(detect).toHaveBeenCalledTimes(2) + expect(probe).toHaveBeenCalledTimes(1) + await expect(getHostedReviewForBranch(input)).resolves.toEqual(review) + expect(detect).toHaveBeenCalledTimes(2) + + // New branch, unsettled discovery: coalescing and deadline enclose discovery itself. + detect.mockImplementation(() => new Promise(() => {})) + const fresh = { ...input, branch: 'new' } + const first = getHostedReviewForBranch(fresh).catch((error: Error) => error.message) + const second = getHostedReviewForBranch(fresh).catch((error: Error) => error.message) + await vi.advanceTimersByTimeAsync(HOSTED_REVIEW_LOOKUP_DEADLINE_MS) + expect(await first).toContain('timed out') + expect(await second).toContain('timed out') + expect(detect).toHaveBeenCalledTimes(3) + await expect(getHostedReviewForBranch(fresh)).rejects.toThrow('backing off') + expect(detect).toHaveBeenCalledTimes(3) + } +) + +it('pins detached provider work through late publication, while independent operations reselect', async () => { + vi.useFakeTimers() + let resume: () => void = () => {} + const gate = new Promise((resolve) => { + resume = resolve + }) + let entered: () => void = () => {} + const started = new Promise((resolve) => { + entered = resolve + }) + const observed: string[] = [] + read.mockImplementation(async (input) => { + entered() + await gate + const selected = githubRepoContext(input.repoPath, null, input.localGitExecOptions).ghAccount + ?.user + observed.push(selected ?? 'ambient') + return { title: selected, state: 'open' } + }) + const input = { repoPath: '/repo', executionHostId: 'local', branch: 'feature' } as const + const pending = getHostedReviewForBranch(input).catch((error: Error) => error.message) + await started + await vi.advanceTimersByTimeAsync(HOSTED_REVIEW_LOOKUP_DEADLINE_MS) + expect(await pending).toContain('timed out') + repo = { ...repo, ghAccount: { host: 'github.com', user: 'Bob' } } + resume() + await vi.advanceTimersByTimeAsync(0) + await expect(getHostedReviewForBranch(input)).resolves.toMatchObject({ title: 'Bob' }) + repo = { ...repo, ghAccount: { host: 'github.com', user: 'Alice' } } + await expect(getHostedReviewForBranch(input)).resolves.toMatchObject({ title: 'Alice' }) + expect(observed).toEqual(['Alice', 'Bob']) + expect(read).toHaveBeenCalledTimes(2) +}) + +// A polling batch makes N independent requests; each may scan the live catalog once. +it.each(['root', 'warm', 'cold'] as const)( + 'bounds %s ownership work per refresh at N and 10N', + async (mode) => { + const measurements: number[] = [] + for (const count of [10, 100]) { + let visits = 0 + const repos = Array.from({ length: count }, (_, i) => ({ + ...repo, + get id() { + visits++ + return `r${i}` + }, + path: `/r${i}`, + ghAccount: { host: 'github.com', user: i === count - 1 ? 'Bob' : 'Alice' } + })) + const projects = repos.map((entry) => ({ + id: entry.id, + displayName: 'Project', + badgeColor: '#000000', + createdAt: 0, + updatedAt: 0, + get sourceRepoIds() { + visits++ + return [entry.id] + } + })) + const children = Array.from({ length: count }, (_, i) => ({ + get repoId() { + visits++ + return `r${count - 1}` + }, + get path() { + visits++ + return `/children/${i}` + } + })) + release = installGitHubProjectAccounts( + { + getRepos: () => repos, + getProjects: () => projects, + getSettings: () => ({}), + getAllWorktreeMetaForHost: () => + mode === 'cold' + ? Object.fromEntries(children.map((child) => [`${child.repoId}::${child.path}`, {}])) + : {} + }, + () => (mode === 'warm' ? children : []) + ) + __resetHostedReviewBranchCacheForTests() + read.mockImplementation(async (input) => ({ + title: githubRepoContext(input.repoPath, null, input.localGitExecOptions).ghAccount?.user + })) + visits = 0 + for (let i = 0; i < count; i++) { + const selected = await getHostedReviewForBranch({ + repoPath: mode === 'root' ? `/r${i}` : `/children/${i}`, + executionHostId: 'local', + branch: 'feature' + }) + expect(selected?.title).toBe(mode === 'root' && i !== count - 1 ? 'Alice' : 'Bob') + } + measurements.push(visits) + // O(N) live catalog/ownership work per independent request, not nested project scans. + expect(visits).toBeLessThanOrEqual(16 * count * count) + release() + } + expect(measurements[1]).toBeLessThanOrEqual(110 * measurements[0]) + } +) diff --git a/src/main/source-control/hosted-review.ts b/src/main/source-control/hosted-review.ts index 08480bb8a6e..02743851851 100644 --- a/src/main/source-control/hosted-review.ts +++ b/src/main/source-control/hosted-review.ts @@ -1,3 +1,4 @@ +import { githubRepoContext, withGitHubRepoExecution } from '../github/github-repository-identity' import type { ExecutionHostId } from '../../shared/execution-host' import type { HostedReviewInfo } from '../../shared/hosted-review' import { assertRemoteUrlReadable } from '../git/remote-url-probe' @@ -61,39 +62,65 @@ export async function getHostedReviewForBranch( return null } - const headOid = input.currentHeadOid?.trim() || null - // All clients spend one host-user API quota; cache above the provider funnel (#11532). - return withHostedReviewBranchCache( - { ...input, branch: branchName }, - { - headOid, - ...(input.active === true ? { active: true } : {}), - ...(input.force === true ? { force: true } : {}) - }, + const connectionId = hostedReviewSshConnectionId(input.executionHostId) + return withGitHubRepoExecution( + input.repoPath, + connectionId, + getHostedReviewLocalGitOptions(input), async () => { - const provider = await getForgeProviderForRepository({ - repoPath: input.repoPath, - executionHostId: input.executionHostId, - ...(input.localGitExecOptions ? { localGitExecOptions: input.localGitExecOptions } : {}) - }) - if (!provider) { - // Detection swallows errors; probe reachability first so timeouts/relay loss cannot cache definitive no-review (P1-D). - await assertRemoteUrlReadable({ - repoPath: input.repoPath, - // The probe is the leaf that still dials: it takes the SSH target, not the host id. - connectionId: hostedReviewSshConnectionId(input.executionHostId), - ...getHostedReviewLocalGitOptions(input) - }) - return null - } - return provider.getReviewForBranch({ - repoPath: input.repoPath, - executionHostId: input.executionHostId, - branch: branchName, - ...(input.localGitExecOptions ? { localGitExecOptions: input.localGitExecOptions } : {}), - githubCurrentHeadOid: headOid, - ...reviewLinkForProvider(input, provider.id) + const context = githubRepoContext( + input.repoPath, + connectionId, + getHostedReviewLocalGitOptions(input) + ) + return lookup({ + ...input, + localGitExecOptions: { + ...getHostedReviewLocalGitOptions(input), + ghAccount: context.ghAccount + } }) } ) + + function lookup(resolvedInput: Parameters[0]) { + const headOid = input.currentHeadOid?.trim() || null + // Cache review API results by the resolved account and execution identity (#11532). + return withHostedReviewBranchCache( + { ...resolvedInput, branch: branchName }, + { + headOid, + ...(input.active === true ? { active: true } : {}), + ...(input.force === true ? { force: true } : {}) + }, + async () => { + const provider = await getForgeProviderForRepository({ + repoPath: input.repoPath, + executionHostId: input.executionHostId, + ...(input.localGitExecOptions ? { localGitExecOptions: input.localGitExecOptions } : {}) + }) + if (!provider) { + // Detection swallows errors; probe reachability first so timeouts/relay loss cannot cache definitive no-review (P1-D). + await assertRemoteUrlReadable({ + repoPath: input.repoPath, + // The probe is the leaf that still dials: it takes the SSH target, not the host id. + connectionId: hostedReviewSshConnectionId(input.executionHostId), + ...getHostedReviewLocalGitOptions(input) + }) + return null + } + const providerOptions = provider.id === 'github' ? resolvedInput : input + return provider.getReviewForBranch({ + repoPath: input.repoPath, + executionHostId: input.executionHostId, + branch: branchName, + ...(providerOptions.localGitExecOptions + ? { localGitExecOptions: providerOptions.localGitExecOptions } + : {}), + githubCurrentHeadOid: headOid, + ...reviewLinkForProvider(input, provider.id) + }) + } + ) + } } diff --git a/src/main/source-control/stacked-hosted-review-creation.ts b/src/main/source-control/stacked-hosted-review-creation.ts index acf9e98e3e1..2360d75d19c 100644 --- a/src/main/source-control/stacked-hosted-review-creation.ts +++ b/src/main/source-control/stacked-hosted-review-creation.ts @@ -1,3 +1,6 @@ +import { withGitHubRepoExecution } from '../github/github-repository-identity' +import { hostedReviewSshConnectionId } from './hosted-review-execution-host' +import { getHostedReviewLocalGitOptions } from './hosted-review-git-options' import type { ExecutionHostId } from '../../shared/execution-host' import type { CreateStackedHostedReviewInput, @@ -18,38 +21,48 @@ export async function createStackedHostedReview( executionHostId: ExecutionHostId, options: HostedReviewExecutionOptions = {} ): Promise { - const plan = await prepareGitHubStackedPullRequest(repoPath, input, executionHostId, options) - if (!plan.ok) { - return plan - } + const operation = async (): Promise => { + const plan = await prepareGitHubStackedPullRequest(repoPath, input, executionHostId, options) + if (!plan.ok) { + return plan + } - let currentReview: (HostedReviewSummary & { number: number }) | null = plan.currentReview - if (!currentReview) { - const created = await createHostedReview(repoPath, input, executionHostId, options) - if (!created.ok) { - if (!created.existingReview?.number) { - return created - } - return { - ...stackedReviewFailure('stack_existing_review_other_base'), - createdReview: { - number: created.existingReview.number, - url: created.existingReview.url + let currentReview: (HostedReviewSummary & { number: number }) | null = plan.currentReview + if (!currentReview) { + const created = await createHostedReview(repoPath, input, executionHostId, options) + if (!created.ok) { + if (!created.existingReview?.number) { + return created + } + return { + ...stackedReviewFailure('stack_existing_review_other_base'), + createdReview: { + number: created.existingReview.number, + url: created.existingReview.url + } } } + currentReview = { number: created.number, url: created.url } + } + if (!currentReview) { + return stackedReviewFailure('stack_creation_unconfirmed', {}, 'unknown_completion') } - currentReview = { number: created.number, url: created.url } - } - if (!currentReview) { - return stackedReviewFailure('stack_creation_unconfirmed', {}, 'unknown_completion') - } - return registerGitHubStackedPullRequest({ - repoPath, - repository: plan.repository, - parentReview: plan.parentReview, - currentReview, - executionHostId, - options - }) + return registerGitHubStackedPullRequest({ + repoPath, + repository: plan.repository, + parentReview: plan.parentReview, + currentReview, + executionHostId, + options + }) + } + return input.provider === 'github' + ? withGitHubRepoExecution( + repoPath, + hostedReviewSshConnectionId(executionHostId), + getHostedReviewLocalGitOptions(options), + operation + ) + : operation() }