diff --git a/src/main/github/client-tracked-upstream-default-branch.test.ts b/src/main/github/client-tracked-upstream-default-branch.test.ts index 361a6edb098..ca01f0a06f9 100644 --- a/src/main/github/client-tracked-upstream-default-branch.test.ts +++ b/src/main/github/client-tracked-upstream-default-branch.test.ts @@ -80,7 +80,7 @@ function restPR( title: 'Release develop', state: 'closed', merged_at: null, - html_url: `https://github.com/${prRepo.owner}/${prRepo.repo}/pull/7`, + html_url: `https://${prRepo.host ?? 'github.com'}/${prRepo.owner}/${prRepo.repo}/pull/7`, updated_at: '2025-01-01T00:00:00Z', draft: false, mergeable: null, @@ -95,39 +95,53 @@ function restPR( function primeGh( listsByHead: Record, - exactPR: RestPullRequest = Object.values(listsByHead).flat()[0] ?? restPR() + exactPR: RestPullRequest | RestPullRequest[] = Object.values(listsByHead).flat()[0] ?? restPR() ): void { - const prsByNumber = new Map( - [...Object.values(listsByHead).flat(), exactPR].map((pr) => [pr.number, pr]) + const repositorySlug = (pr: RestPullRequest): string => + new URL(pr.html_url ?? '').pathname.split('/').slice(1, 3).join('/') + const repositoryHost = (pr: RestPullRequest): string => new URL(pr.html_url ?? '').host + const exactPRs = Array.isArray(exactPR) ? exactPR : [exactPR] + const prsByRepositoryAndNumber = new Map( + [...Object.values(listsByHead).flat(), ...exactPRs].map((pr) => [ + `${repositoryHost(pr)}/${repositorySlug(pr)}#${pr.number}`, + pr + ]) ) - ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { - const head = args[1]?.match(/pulls\?head=([^&]+)/)?.[1] - if (args[0] === 'api' && head) { - return { stdout: JSON.stringify(listsByHead[decodeURIComponent(head)] ?? []) } - } - const exact = prsByNumber.get(Number(args[2])) - if (args[0] === 'pr' && args[1] === 'view' && exact) { - return { - stdout: JSON.stringify({ - number: exact.number, - title: exact.title, - state: exact.merged_at ? 'MERGED' : exact.state.toUpperCase(), - url: exact.html_url, - statusCheckRollup: [], - updatedAt: exact.updated_at, - isDraft: exact.draft, - mergeable: 'UNKNOWN', - baseRefName: exact.base?.ref, - headRefName: exact.head?.ref, - baseRefOid: exact.base?.sha, - headRefOid: exact.head?.sha, - headRepositoryOwner: exact.head?.repo?.owner, - headRepository: exact.head?.repo ? { name: exact.head.repo.name } : null - }) + ghExecFileAsyncMock.mockImplementation( + async (args: string[], options: { host?: string } = {}) => { + const host = options.host ?? 'github.com' + const match = args[1]?.match(/^repos\/([^/]+\/[^/]+)\/pulls\?head=([^&]+)/) + if (args[0] === 'api' && match) { + const list = (listsByHead[decodeURIComponent(match[2])] ?? []).filter( + (pr) => repositoryHost(pr) === host && repositorySlug(pr) === match[1] + ) + return { stdout: JSON.stringify(list) } } + const repoIndex = args.indexOf('--repo') + const exact = prsByRepositoryAndNumber.get(`${host}/${args[repoIndex + 1]}#${args[2]}`) + if (args[0] === 'pr' && args[1] === 'view' && exact) { + return { + stdout: JSON.stringify({ + number: exact.number, + title: exact.title, + state: exact.merged_at ? 'MERGED' : exact.state.toUpperCase(), + url: exact.html_url, + statusCheckRollup: [], + updatedAt: exact.updated_at, + isDraft: exact.draft, + mergeable: 'UNKNOWN', + baseRefName: exact.base?.ref, + headRefName: exact.head?.ref, + baseRefOid: exact.base?.sha, + headRefOid: exact.head?.sha, + headRepositoryOwner: exact.head?.repo?.owner, + headRepository: exact.head?.repo ? { name: exact.head.repo.name } : null + }) + } + } + throw new Error(`gh unavailable: ${args.join(' ')}`) } - throw new Error(`gh unavailable: ${args.join(' ')}`) - }) + ) } /** A fork clone: `origin` is the fork, `upstream` is the repo PRs target. */ @@ -165,6 +179,26 @@ describe('issue #26948: a branch tracking the default branch', () => { expect(outcome.kind).toBe('no-pr') }) + it('identifies the rejected merged integration PR so head-current caches can clear it', async () => { + primeGit('refs/remotes/origin/develop') + const integrationPR = { + ...restPR(), + merged_at: '2025-01-02T00:00:00Z', + head: { ...restPR().head, sha: 'feature-head-oid' } + } + primeGh({ 'acme:develop': [integrationPR] }) + + expect( + await getPRForBranchOutcome('/repo-root', 'feature/my-change', null, null, 7, { + currentHeadOid: 'feature-head-oid', + acceptMergedFallbackPR: true + }) + ).toMatchObject({ + kind: 'no-pr', + rejectedPRUrls: ['https://github.com/acme/widgets/pull/7'] + }) + }) + it('skips the default branch on a second remote that PRs target (fork checkout off upstream)', async () => { primeForkWithUpstream() primeGit('refs/remotes/upstream/develop', { origin: 'develop', upstream: 'develop' }) @@ -296,4 +330,90 @@ describe('issue #26948: a branch tracking the default branch', () => { ) expect(exactCalls).toHaveLength(1) }) + + it('does not reject a cached PR because another repository rejected the same number', async () => { + primeForkWithUpstream() + primeGit('refs/remotes/upstream/develop', { origin: 'main', upstream: 'develop' }) + const unrelated = restPR('develop', UPSTREAM, UPSTREAM) + const cached = { ...restPR('feature/my-change'), state: 'open' } + primeGh({ 'stablyai:develop': [unrelated] }, [unrelated, cached]) + const pr = await getPRForBranch('/repo-root', 'feature/my-change', null, null, 7) + expect(pr).toMatchObject({ number: 7, prRepo: ACME, headRefName: 'feature/my-change' }) + }) + + it('continues cached number recovery after rejecting another repository default PR', async () => { + primeForkWithUpstream() + primeGit('refs/remotes/upstream/develop', { origin: 'main', upstream: 'develop' }) + primeGh({}, [ + restPR('develop', UPSTREAM, UPSTREAM), + { ...restPR('feature/my-change'), state: 'open' } + ]) + const pr = await getPRForBranch('/repo-root', 'feature/my-change', null, null, 7) + expect(pr).toMatchObject({ number: 7, prRepo: ACME, headRefName: 'feature/my-change' }) + }) + + it('does not accept another repository head with the same owner and branch name', async () => { + const other = { owner: 'acme', repo: 'other-widgets' } + resolvePRRepositoryCandidatesMock.mockResolvedValue({ + candidates: [other, ACME], + headRepo: ACME + }) + primeGit('') + primeGh({ + 'acme:feature/my-change': [{ ...restPR('feature/my-change', other, other), state: 'open' }] + }) + expect(await getPRForBranch('/repo-root', 'feature/my-change')).toBeNull() + }) + + it('does not query another GitHub server for a known public GitHub head', async () => { + const enterprise = { ...ACME, host: 'ghe.example' } + resolvePRRepositoryCandidatesMock.mockResolvedValue({ + candidates: [enterprise, ACME], + headRepo: ACME + }) + primeGit('') + primeGh({ + 'acme:feature/my-change': [ + { ...restPR('feature/my-change', enterprise, enterprise), state: 'open' } + ] + }) + expect(await getPRForBranch('/repo-root', 'feature/my-change')).toBeNull() + expect( + ghExecFileAsyncMock.mock.calls.some(([, options]) => options?.host === 'ghe.example') + ).toBe(false) + }) + + it('preserves a tracked contributor fork with a different repository name', async () => { + const fork = { owner: 'contributor', repo: 'renamed-widgets' } + getOwnerRepoForRemoteMock.mockImplementation(async (_path: string, remote: string) => + remote === 'contributor' ? fork : ACME + ) + primeGit('refs/remotes/contributor/develop') + primeGh({ 'contributor:develop': [{ ...restPR('develop', fork), state: 'open' }] }) + expect(await getPRForBranch('/repo-root', 'feature/my-change')).toMatchObject({ + number: 7, + headRepo: fork, + prRepo: ACME + }) + }) + + it('continues to a valid branch result after another repository returns the wrong head', async () => { + const other = { owner: 'acme', repo: 'other-widgets' } + resolvePRRepositoryCandidatesMock.mockResolvedValue({ + candidates: [other, ACME], + headRepo: ACME + }) + primeGit('') + primeGh({ + 'acme:feature/my-change': [ + { ...restPR('feature/my-change', other, other), state: 'open' }, + { ...restPR('feature/my-change'), state: 'open' } + ] + }) + expect(await getPRForBranch('/repo-root', 'feature/my-change')).toMatchObject({ + number: 7, + headRepo: ACME, + prRepo: ACME + }) + }) }) diff --git a/src/main/github/client/lookup/branch-lookup-resolution.ts b/src/main/github/client/lookup/branch-lookup-resolution.ts index f5afb563b27..242645abfcb 100644 --- a/src/main/github/client/lookup/branch-lookup-resolution.ts +++ b/src/main/github/client/lookup/branch-lookup-resolution.ts @@ -26,12 +26,12 @@ import { lookupPRByBranchName } from './pr-branch-lookup' import { lookupPRByNumber } from './pr-number-lookup' import { derivePRRefreshData } from './branch-lookup-derived-data' import { assemblePRRefreshFoundOutcome } from './pr-refresh-outcome-assembly' -import { - shouldRetryTrackedUpstreamBranch, - type TrackedUpstreamBranch -} from './tracked-upstream-cache' +import { shouldRetryTrackedUpstreamBranch } from './tracked-upstream-cache' import { getTrackedUpstreamBranch } from './tracked-upstream-branch' -import { shouldHideImplicitDefaultBranchPR } from './implicit-default-branch-pr' +import { + createImplicitDefaultBranchPRPolicy, + type ImplicitDefaultBranchPRContext +} from './implicit-default-branch-pr' import { PR_BRANCH_LOOKUP_BUCKETS } from './pr-lookup-rate-limit' import type { HostedReviewLocalGitOptions } from './../github-exec-scope' export async function resolvePRForBranchOutcome(input: { @@ -80,14 +80,22 @@ export async function resolvePRForBranchOutcome(input: { let hasPendingBranchLookupError = false let currentHeadOidForMergedImplicit: string | null | undefined let usedExactNumberLookup = false - let retriedUpstream: { branch: TrackedUpstreamBranch; headRepo: OwnerRepo } | null = null + const defaultBranchPRContext: ImplicitDefaultBranchPRContext = { + originHeadRepo: headRepo, + trackedUpstream: null, + branchName, + linkedPRNumber, + repoPath, + connectionId, + localGitOptions + } + const implicitPRPolicy = createImplicitDefaultBranchPRPolicy(defaultBranchPRContext) const explicitCurrentHeadOid = typeof options.currentHeadOid === 'string' && options.currentHeadOid.trim().length > 0 ? options.currentHeadOid.trim() : null let confirmedContainedHeadOid: string | null = null - let headDivergedFromMergedPRAtOid: string | null = null const mergedPRContainsHead = async ( candidate: PullRequestLookupData, candidateRepo: OwnerRepo | null, @@ -146,7 +154,8 @@ export async function resolvePRForBranchOutcome(input: { headRepo, branchName, ghOptions, - executionScope + executionScope, + policy: implicitPRPolicy }) data = branchLookup.data dataRepo = branchLookup.dataRepo @@ -174,13 +183,17 @@ export async function resolvePRForBranchOutcome(input: { upstreamHeadRepo && shouldRetryTrackedUpstreamBranch(upstreamBranch, branchName, upstreamHeadRepo, headRepo) ) { - retriedUpstream = { branch: upstreamBranch, headRepo: upstreamHeadRepo } + defaultBranchPRContext.trackedUpstream = { + branch: upstreamBranch, + headRepo: upstreamHeadRepo + } const upstreamLookup = await lookupPRByBranchName({ candidates, headRepo: upstreamHeadRepo, branchName: upstreamBranch.branchName, ghOptions, - executionScope + executionScope, + policy: implicitPRPolicy }) data = upstreamLookup.data dataRepo = upstreamLookup.dataRepo @@ -195,29 +208,6 @@ export async function resolvePRForBranchOutcome(input: { } } } - let rejectedDefaultPRNumber: number | null = null - const hideUnrelatedDefaultPR = async (): Promise => { - if ( - data && - (await shouldHideImplicitDefaultBranchPR({ - data, - prRepo: dataRepo, - originHeadRepo: headRepo, - trackedUpstream: retriedUpstream, - branchName, - linkedPRNumber, - repoPath, - connectionId, - localGitOptions - })) - ) { - rejectedDefaultPRNumber = data.number - data = null - dataRepo = null - dataHeadRepo = headRepo - } - } - await hideUnrelatedDefaultPR() let mergedBranchLookupNumber: number | null = null if (await hideMergedImplicitPR(data, dataRepo)) { mergedBranchLookupNumber = data?.number ?? null @@ -225,32 +215,32 @@ export async function resolvePRForBranchOutcome(input: { dataRepo = null dataHeadRepo = headRepo } - if ( - !data && - typeof linkedPRNumber !== 'number' && - typeof fallbackPRNumber === 'number' && - fallbackPRNumber !== rejectedDefaultPRNumber - ) { + if (!data && typeof linkedPRNumber !== 'number' && typeof fallbackPRNumber === 'number') { usedExactNumberLookup = true const fallbackLookup = await lookupPRByNumber({ candidates, number: fallbackPRNumber, ghOptions, - executionScope + executionScope, + policy: implicitPRPolicy }) data = fallbackLookup.data dataRepo = fallbackLookup.dataRepo } - // Recheck exact fallbacks so links cached before #26948 heal without upstream tracking. - await hideUnrelatedDefaultPR() if (!data) { if (hasPendingBranchLookupError) { return prRefreshUpstreamError(pendingBranchLookupError) } - return { kind: 'no-pr', fetchedAt: Date.now() } + return { + kind: 'no-pr', + fetchedAt: Date.now(), + ...(implicitPRPolicy.rejectedPRUrls.size > 0 + ? { rejectedPRUrls: [...implicitPRPolicy.rejectedPRUrls] } + : {}) + } } dataHeadRepo = pullRequestHeadRepository(data, dataRepo) ?? dataHeadRepo - headDivergedFromMergedPRAtOid = await linkedMergedPRDivergedHeadOid( + const headDivergedFromMergedPRAtOid = await linkedMergedPRDivergedHeadOid( data, linkedPRNumber, explicitCurrentHeadOid, diff --git a/src/main/github/client/lookup/implicit-default-branch-pr.ts b/src/main/github/client/lookup/implicit-default-branch-pr.ts index 58f882b34d3..1211330265a 100644 --- a/src/main/github/client/lookup/implicit-default-branch-pr.ts +++ b/src/main/github/client/lookup/implicit-default-branch-pr.ts @@ -6,11 +6,13 @@ import { } from '../../../source-control/repo-default-branch' import type { HostedReviewLocalGitOptions } from './../github-exec-scope' import type { TrackedUpstreamBranch } from './tracked-upstream-cache' -import { pullRequestHeadRepository, type PullRequestLookupData } from './pull-request-lookup-data' +import { + pullRequestHeadRepository, + type PullRequestLookupData, + type PullRequestLookupPolicy +} from './pull-request-lookup-data' -export async function shouldHideImplicitDefaultBranchPR(input: { - data: PullRequestLookupData - prRepo: OwnerRepo | null +export type ImplicitDefaultBranchPRContext = { originHeadRepo: OwnerRepo | null trackedUpstream: { branch: TrackedUpstreamBranch; headRepo: OwnerRepo } | null branchName: string @@ -18,7 +20,34 @@ export async function shouldHideImplicitDefaultBranchPR(input: { repoPath: string connectionId?: string | null localGitOptions: HostedReviewLocalGitOptions -}): Promise { +} + +export function createImplicitDefaultBranchPRPolicy( + context: ImplicitDefaultBranchPRContext +): PullRequestLookupPolicy { + const rejected = new Set() + const rejectedPRUrls = new Set() + return { + rejectedPRUrls, + async accepts(data, prRepo) { + const hidden = await shouldHideImplicitDefaultBranchPR({ ...context, data, prRepo }) + if (hidden && prRepo) { + rejected.add(`${githubRepoIdentityKey(prRepo)}#${data.number}`) + if (data.url) { + rejectedPRUrls.add(data.url) + } + } + return !hidden + }, + isRejected(prRepo, number) { + return rejected.has(`${githubRepoIdentityKey(prRepo)}#${number}`) + } + } +} + +async function shouldHideImplicitDefaultBranchPR( + input: ImplicitDefaultBranchPRContext & { data: PullRequestLookupData; prRepo: OwnerRepo | null } +): Promise { const { data, branchName, prRepo } = input if ( !branchName || diff --git a/src/main/github/client/lookup/pr-branch-lookup.ts b/src/main/github/client/lookup/pr-branch-lookup.ts index 2ee667ceacc..82fc4494a7c 100644 --- a/src/main/github/client/lookup/pr-branch-lookup.ts +++ b/src/main/github/client/lookup/pr-branch-lookup.ts @@ -1,6 +1,10 @@ import { ghExecFileAsync } from '../../gh-utils' import type { OwnerRepo, ghRepoExecOptions } from '../../gh-utils' -import { githubHostExecOptions, type GitHubApiRepository } from '../../github-api-repository' +import { + githubHostExecOptions, + githubRepositoryWebHost, + type GitHubApiRepository +} from '../../github-api-repository' import type { GhExecOptions } from './../github-exec-scope' import { isNoPullRequestError } from './../gh-error-predicates' import { @@ -8,7 +12,9 @@ import { PR_BRANCH_LIST_JSON_FIELDS, mapRestPullRequest, normalizePullRequestLookupData, + pullRequestMatchesHeadRepository, type PullRequestLookupData, + type PullRequestLookupPolicy, type RestPullRequest } from './pull-request-lookup-data' import { getPRByNumber } from './pr-number-lookup' @@ -79,6 +85,7 @@ export async function lookupPRByBranchName(args: { branchName: string ghOptions: GhExecOptions executionScope: string + policy?: PullRequestLookupPolicy }): Promise<{ data: PullRequestLookupData | null dataRepo: OwnerRepo | null @@ -88,6 +95,13 @@ export async function lookupPRByBranchName(args: { let pendingError: unknown let hasPendingError = false for (const candidate of args.candidates) { + if ( + args.headRepo && + githubRepositoryWebHost(candidate).trim().toLowerCase() !== + githubRepositoryWebHost(args.headRepo).trim().toLowerCase() + ) { + continue + } try { const branchData = args.headRepo ? await getRestPRForBranch( @@ -104,7 +118,11 @@ export async function lookupPRByBranchName(args: { args.ghOptions, args.executionScope ) - if (data) { + if ( + data && + pullRequestMatchesHeadRepository(data, candidate, args.headRepo) && + (!args.policy || (await args.policy.accepts(data, candidate))) + ) { return { data, dataRepo: candidate } } } catch (err) { @@ -128,7 +146,11 @@ export async function lookupPRByBranchName(args: { args.ghOptions, args.executionScope ) - if (data) { + if ( + data && + pullRequestMatchesHeadRepository(data, candidate, args.headRepo) && + (!args.policy || (await args.policy.accepts(data, candidate))) + ) { return { data, dataRepo: candidate } } } catch (retryErr) { @@ -150,10 +172,13 @@ export async function lookupPRByBranchName(args: { ['pr', 'view', args.branchName, '--json', PR_LOOKUP_JSON_FIELDS], args.ghOptions ) - return { + const lookup = { data: normalizePullRequestLookupData(JSON.parse(stdout) as PullRequestLookupData), dataRepo: null } + return !args.policy || (await args.policy.accepts(lookup.data, null)) + ? lookup + : { data: null, dataRepo: null } } catch (err) { if (isNoPullRequestError(err)) { return { data: null, dataRepo: null } diff --git a/src/main/github/client/lookup/pr-number-lookup.ts b/src/main/github/client/lookup/pr-number-lookup.ts index 42a605eb295..3df2bdeaf5b 100644 --- a/src/main/github/client/lookup/pr-number-lookup.ts +++ b/src/main/github/client/lookup/pr-number-lookup.ts @@ -11,6 +11,7 @@ import { mapRestPullRequest, normalizePullRequestLookupData, type PullRequestLookupData, + type PullRequestLookupPolicy, type RestPullRequest } from './pull-request-lookup-data' import { hydratePullRequestLookupData } from './pull-request-lookup-hydration' @@ -108,8 +109,12 @@ export async function lookupPRByNumber(args: { number: number ghOptions: ReturnType executionScope: string + policy?: PullRequestLookupPolicy }): Promise<{ data: PullRequestLookupData | null; dataRepo: OwnerRepo | null }> { for (const candidate of args.candidates) { + if (args.policy?.isRejected(candidate, args.number)) { + continue + } try { const linkedData = await getPRByNumber( candidate, @@ -117,7 +122,7 @@ export async function lookupPRByNumber(args: { args.ghOptions, args.executionScope ) - if (!linkedData) { + if (!linkedData || (args.policy && !(await args.policy.accepts(linkedData, candidate)))) { continue } return { data: linkedData, dataRepo: candidate } @@ -138,10 +143,13 @@ export async function lookupPRByNumber(args: { ['pr', 'view', String(args.number), '--json', PR_LOOKUP_JSON_FIELDS], args.ghOptions ) - return { + const lookup = { data: normalizePullRequestLookupData(JSON.parse(stdout) as PullRequestLookupData), dataRepo: null } + return !args.policy || (await args.policy.accepts(lookup.data, null)) + ? lookup + : { data: null, dataRepo: null } } catch (err) { if (isNoPullRequestError(err)) { // Why: stale cached fallback numbers shouldn't error every poll when the PR was deleted or belongs to another repo. diff --git a/src/main/github/client/lookup/pull-request-lookup-data.ts b/src/main/github/client/lookup/pull-request-lookup-data.ts index 60263b5915b..8d4a00cea43 100644 --- a/src/main/github/client/lookup/pull-request-lookup-data.ts +++ b/src/main/github/client/lookup/pull-request-lookup-data.ts @@ -9,6 +9,7 @@ import type { GitAdmissionTier } from '../../../git/command-runner/git-exec-opti import type { HostedReviewExecutionOptions } from '../../../source-control/hosted-review-git-options' import { mapPRState } from '../../mappers' import type { MergedPRCommitMembership } from '../../merged-pr-commit-membership' +import { githubRepoIdentityKey } from '../../../../shared/github/repository-identity-key' import { normalizePRMergeable, normalizeReviewDecision, @@ -43,6 +44,12 @@ export type PullRequestLookupData = { stackMetadataChecked?: boolean } +export type PullRequestLookupPolicy = { + accepts: (data: PullRequestLookupData, prRepo: OwnerRepo | null) => Promise + isRejected: (prRepo: OwnerRepo, number: number) => boolean + rejectedPRUrls: ReadonlySet +} + export type RestPullRequest = { number: number title: string @@ -144,6 +151,19 @@ export function pullRequestHeadRepository( return owner && repo ? { owner, repo, ...(prRepo?.host ? { host: prRepo.host } : {}) } : null } +export function pullRequestMatchesHeadRepository( + data: PullRequestLookupData, + prRepo: OwnerRepo | null, + expectedHeadRepo: OwnerRepo | null +): boolean { + const actualHeadRepo = pullRequestHeadRepository(data, prRepo) + return ( + !actualHeadRepo || + !expectedHeadRepo || + githubRepoIdentityKey(actualHeadRepo) === githubRepoIdentityKey(expectedHeadRepo) + ) +} + export function isMergedImplicitPR( data: PullRequestLookupData, linkedPRNumber?: number | null diff --git a/src/main/source-control/repo-default-branch.test.ts b/src/main/source-control/repo-default-branch.test.ts index a2dd56a8cd0..02ea27b1e94 100644 --- a/src/main/source-control/repo-default-branch.test.ts +++ b/src/main/source-control/repo-default-branch.test.ts @@ -10,7 +10,8 @@ vi.mock('../git/runner', () => ({ })) vi.mock('../providers/ssh-git-dispatch', () => ({ - getSshGitProvider: getSshGitProviderMock + getSshGitProvider: getSshGitProviderMock, + getSshGitProviderGeneration: vi.fn(() => 0) })) import { diff --git a/src/main/source-control/repo-default-branch.ts b/src/main/source-control/repo-default-branch.ts index 61de5b0537d..8a45ba7f13a 100644 --- a/src/main/source-control/repo-default-branch.ts +++ b/src/main/source-control/repo-default-branch.ts @@ -1,7 +1,9 @@ import { resolveDefaultBaseRefViaExec } from '../git/repo' import { resolveRemoteHeadBranchViaExec, type GitExec } from '../../shared/git-default-base-ref' import { gitExecFileAsync } from '../git/runner' -import { getSshGitProvider } from '../providers/ssh-git-dispatch' +import { getSshGitProvider, getSshGitProviderGeneration } from '../providers/ssh-git-dispatch' +import { getRepoExecutionHostId } from '../../shared/execution-host' +import { hostedReviewRepoScope, scopeGeneration } from './hosted-review-scope-generations' import type { HostedReviewLocalGitOptions } from './hosted-review-git-options' // Why: bounded like TRACKED_UPSTREAM_SNAPSHOT_CACHE in github/client.ts — PR @@ -28,7 +30,13 @@ function getRepoDefaultBranchCacheKey( const runtimeKey = connectionId ? `ssh:${connectionId}` : `local:${localGitOptions.wslDistro ?? 'host'}` - return [runtimeKey, repoPath].join('\0') + const scope = hostedReviewRepoScope(repoPath, getRepoExecutionHostId({ connectionId })) + return [ + runtimeKey, + repoPath, + scopeGeneration(scope), + connectionId ? getSshGitProviderGeneration(connectionId) : 0 + ].join('\0') } function pruneRepoDefaultBranchCache(now: number): void { diff --git a/src/main/source-control/repo-remote-head-branch.test.ts b/src/main/source-control/repo-remote-head-branch.test.ts index b2dfccaad08..6c223844235 100644 --- a/src/main/source-control/repo-remote-head-branch.test.ts +++ b/src/main/source-control/repo-remote-head-branch.test.ts @@ -1,12 +1,21 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -const { gitExecFileAsyncMock, getSshGitProviderMock } = vi.hoisted(() => ({ - gitExecFileAsyncMock: vi.fn(), - getSshGitProviderMock: vi.fn() -})) +const { gitExecFileAsyncMock, getSshGitProviderMock, getSshGitProviderGenerationMock } = vi.hoisted( + () => ({ + gitExecFileAsyncMock: vi.fn(), + getSshGitProviderMock: vi.fn(), + getSshGitProviderGenerationMock: vi.fn(() => 0) + }) +) vi.mock('../git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) -vi.mock('../providers/ssh-git-dispatch', () => ({ getSshGitProvider: getSshGitProviderMock })) +vi.mock('../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: getSshGitProviderMock, + getSshGitProviderGeneration: getSshGitProviderGenerationMock +})) + +import { bumpScopeGeneration, hostedReviewRepoScope } from './hosted-review-scope-generations' +import { LOCAL_EXECUTION_HOST_ID } from '../../shared/execution-host' import { getRemoteHeadBranchName, @@ -22,6 +31,7 @@ describe('getRemoteHeadBranchName', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() getSshGitProviderMock.mockReset() + getSshGitProviderGenerationMock.mockReset().mockReturnValue(0) __resetRepoDefaultBranchCacheForTests() }) @@ -120,4 +130,24 @@ describe('getRemoteHeadBranchName', () => { now.mockRestore() } }) + + it('does not reuse defaults from a replaced SSH provider', async () => { + getSshGitProviderGenerationMock.mockReturnValue(1) + const exec = vi.fn().mockResolvedValue(remoteHead('upstream', 'old-default')) + getSshGitProviderMock.mockReturnValue({ exec }) + await expect(getRemoteHeadBranchName('/repo', 'upstream', 'ssh-1')).resolves.toBe('old-default') + getSshGitProviderGenerationMock.mockReturnValue(2) + exec.mockResolvedValue(remoteHead('upstream', 'new-default')) + await expect(getRemoteHeadBranchName('/repo', 'upstream', 'ssh-1')).resolves.toBe('new-default') + expect(exec).toHaveBeenCalledTimes(2) + }) + + it('refreshes defaults after the repository review scope is invalidated', async () => { + gitExecFileAsyncMock.mockResolvedValue(remoteHead('upstream', 'old-default')) + await expect(getRemoteHeadBranchName('/repo', 'upstream')).resolves.toBe('old-default') + bumpScopeGeneration(hostedReviewRepoScope('/repo', LOCAL_EXECUTION_HOST_ID)) + gitExecFileAsyncMock.mockResolvedValue(remoteHead('upstream', 'new-default')) + await expect(getRemoteHeadBranchName('/repo', 'upstream')).resolves.toBe('new-default') + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) }) diff --git a/src/renderer/src/store/github/pr-result-cache.ts b/src/renderer/src/store/github/pr-result-cache.ts index d041753fe32..fcf3b346522 100644 --- a/src/renderer/src/store/github/pr-result-cache.ts +++ b/src/renderer/src/store/github/pr-result-cache.ts @@ -12,6 +12,27 @@ import { findWorktreeById } from './worktree-refresh' const PR_REFRESH_STARTED_HOSTED_REVIEW_ENTRY_MAX = 128 +type GitHubPRResultCacheArgs = { + prCacheKey: string + repoPath: string + branch: string + settings: AppState['settings'] + repoId?: string + connectionId?: string | null + executionHostId?: string | null + hasRepoOwner?: boolean + pr: PRInfo | null + fetchedAt: number + rejectedPRUrls?: readonly string[] + worktreeId?: string + linkedPRNumber?: number | null + fallbackPRNumber?: number | null + fallbackPRSource?: GitHubPRFallbackSource | null + requestStartedAt?: number + fetchedHeadOid?: string | null + requestStartedEntry?: AppState['hostedReviewCache'][string] +} + export function applyPRCacheResult( cache: AppState['prCache'], cacheKey: string, @@ -70,107 +91,29 @@ export function setPRRefreshStartedHostedReviewEntry( export function setGitHubPRResultCaches( state: AppState, - args: { - prCacheKey: string - repoPath: string - branch: string - settings: AppState['settings'] - repoId?: string - connectionId?: string | null - executionHostId?: string | null - hasRepoOwner?: boolean - pr: PRInfo | null - fetchedAt: number - worktreeId?: string - linkedPRNumber?: number | null - fallbackPRNumber?: number | null - fallbackPRSource?: GitHubPRFallbackSource | null - requestStartedAt?: number - fetchedHeadOid?: string | null - requestStartedEntry?: AppState['hostedReviewCache'][string] - } + args: GitHubPRResultCacheArgs ): Partial { - const preserveExistingPRForFallbackMiss = shouldPreserveExistingPRForFallbackMiss({ - currentPR: state.prCache[args.prCacheKey]?.data, - nextPR: args.pr, + const next = applyGitHubPRResultToCaches({ + ...args, state, - worktreeId: args.worktreeId, - linkedPRNumber: args.linkedPRNumber, - fallbackPRNumber: args.fallbackPRNumber, - fallbackPRSource: args.fallbackPRSource + prCache: state.prCache, + hostedReviewCache: state.hostedReviewCache }) - const hostedReviewSync = syncHostedReviewCacheFromGitHubPRResult({ - cache: state.hostedReviewCache, - repoPath: args.repoPath, - branch: args.branch, - settings: args.settings, - repoId: args.repoId, - connectionId: args.connectionId, - executionHostId: args.executionHostId, - hasRepoOwner: args.hasRepoOwner, - pr: args.pr, - fetchedAt: args.fetchedAt, - linkedPRNumber: args.linkedPRNumber, - fallbackPRNumber: args.fallbackPRNumber, - fallbackPRSource: args.fallbackPRSource, - preserveExistingPRForFallbackMiss, - requestStartedAt: args.requestStartedAt, - requestStartedEntry: args.requestStartedEntry - }) - const hostedReviewCacheKey = getHostedReviewCacheKey( - args.repoPath, - args.branch, - args.settings, - args.repoId, - args.connectionId, - args.executionHostId, - args.hasRepoOwner === true - ) - const nextPRCache = applyPRCacheResult( - state.prCache, - args.prCacheKey, - args.pr, - args.fetchedAt, - shouldWritePRCacheForHostedReviewSync({ - hostedReviewSyncAccepted: hostedReviewSync.accepted, - hostedReviewEntry: state.hostedReviewCache[hostedReviewCacheKey], - pr: args.pr, - linkedPRNumber: args.linkedPRNumber, - fallbackPRNumber: args.fallbackPRNumber - }), - preserveExistingPRForFallbackMiss, - args.fetchedHeadOid - ) return { - ...(nextPRCache === state.prCache ? {} : { prCache: nextPRCache }), - ...(hostedReviewSync.cache === state.hostedReviewCache + ...(next.prCache === state.prCache ? {} : { prCache: next.prCache }), + ...(next.hostedReviewCache === state.hostedReviewCache ? {} - : { hostedReviewCache: hostedReviewSync.cache }) + : { hostedReviewCache: next.hostedReviewCache }) } } -export function applyGitHubPRResultToCaches(args: { - prCache: AppState['prCache'] - hostedReviewCache: AppState['hostedReviewCache'] - prCacheKey: string - repoPath: string - branch: string - settings: AppState['settings'] - repoId?: string - connectionId?: string | null - executionHostId?: string | null - hasRepoOwner?: boolean - pr: PRInfo | null - fetchedAt: number - state: AppState - worktreeId?: string - linkedPRNumber?: number | null - fallbackPRNumber?: number | null - fallbackPRSource?: GitHubPRFallbackSource | null - requestStartedAt?: number - fetchedHeadOid?: string | null - requestStartedEntry?: AppState['hostedReviewCache'][string] -}): { +export function applyGitHubPRResultToCaches( + args: GitHubPRResultCacheArgs & { + prCache: AppState['prCache'] + hostedReviewCache: AppState['hostedReviewCache'] + state: AppState + } +): { prCache: AppState['prCache'] hostedReviewCache: AppState['hostedReviewCache'] } { @@ -180,8 +123,7 @@ export function applyGitHubPRResultToCaches(args: { state: args.state, worktreeId: args.worktreeId, linkedPRNumber: args.linkedPRNumber, - fallbackPRNumber: args.fallbackPRNumber, - fallbackPRSource: args.fallbackPRSource + rejectedPRUrls: args.rejectedPRUrls }) const hostedReviewSync = syncHostedReviewCacheFromGitHubPRResult({ cache: args.hostedReviewCache, @@ -235,13 +177,13 @@ export function shouldPreserveExistingPRForFallbackMiss(args: { state: AppState worktreeId?: string linkedPRNumber?: number | null - fallbackPRNumber?: number | null - fallbackPRSource?: GitHubPRFallbackSource | null + rejectedPRUrls?: readonly string[] }): boolean { if ( args.nextPR !== null || args.linkedPRNumber != null || args.currentPR?.state !== 'merged' || + args.rejectedPRUrls?.includes(args.currentPR.url) || typeof args.currentPR.headSha !== 'string' || args.currentPR.headSha.length === 0 ) { diff --git a/src/renderer/src/store/github/pull-request-execution.ts b/src/renderer/src/store/github/pull-request-execution.ts index 21f48d530d8..40f4d94eff9 100644 --- a/src/renderer/src/store/github/pull-request-execution.ts +++ b/src/renderer/src/store/github/pull-request-execution.ts @@ -171,6 +171,7 @@ export function startPullRequestLookup(args: { hasRepoOwner: repo !== undefined, pr, fetchedAt: outcome.fetchedAt, + rejectedPRUrls: outcome.kind === 'no-pr' ? outcome.rejectedPRUrls : undefined, fetchedHeadOid: requestHeadOid, worktreeId: options?.worktreeId, linkedPRNumber, @@ -270,8 +271,7 @@ export function startPullRequestLookup(args: { state: get(), worktreeId: options?.worktreeId, linkedPRNumber, - fallbackPRNumber, - fallbackPRSource + rejectedPRUrls: outcome.kind === 'no-pr' ? outcome.rejectedPRUrls : undefined }) ) { return get().prCache[cacheKey]?.data ?? null diff --git a/src/renderer/src/store/github/refresh-event-actions.ts b/src/renderer/src/store/github/refresh-event-actions.ts index 8396ff3feca..c345bb3c41d 100644 --- a/src/renderer/src/store/github/refresh-event-actions.ts +++ b/src/renderer/src/store/github/refresh-event-actions.ts @@ -177,6 +177,8 @@ export const createRefreshEventActions = ( hasRepoOwner: true, pr: data, fetchedAt: event.outcome.fetchedAt, + rejectedPRUrls: + event.outcome.kind === 'no-pr' ? event.outcome.rejectedPRUrls : undefined, fetchedHeadOid: alias.currentHeadOid, state: s, worktreeId: alias.worktreeId, diff --git a/src/renderer/src/store/slices/github-pr-rejected-cache.test.ts b/src/renderer/src/store/slices/github-pr-rejected-cache.test.ts new file mode 100644 index 00000000000..921828e75c1 --- /dev/null +++ b/src/renderer/src/store/slices/github-pr-rejected-cache.test.ts @@ -0,0 +1,130 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { hostedReviewInfoFromGitHubPRInfo } from '../../../../shared/hosted-review-github' +import { _clearGitHubPRRefreshStartedEntriesForTest } from '../github/request-coordination' +import { getHostedReviewCacheKey } from './hosted-review-cache-identity' +import { + createTestStore, + makePR, + makePRRefreshWorktree, + mockApi, + resetRemoteRuntimeMocks +} from './github-slice-test-harness' + +describe.each(['manual', 'coordinator'] as const)('%s rejected PR cache results', (source) => { + beforeEach(() => { + vi.clearAllMocks() + vi.useFakeTimers() + resetRemoteRuntimeMocks() + mockApi.gh.refreshPRNow.mockReset() + _clearGitHubPRRefreshStartedEntriesForTest() + }) + + afterEach(() => { + _clearGitHubPRRefreshStartedEntriesForTest() + vi.useRealTimers() + }) + + it.each([ + { + name: 'clears a rejected integration PR even at its merged head', + rejectedUrl: 'https://github.com/acme/orca/pull/12', + head: 'merged-head', + preserves: false + }, + { + name: 'clears a rejected integration PR at a confirmed contained commit', + rejectedUrl: 'https://github.com/acme/orca/pull/12', + head: 'contained-head', + preserves: false + }, + { + name: 'preserves a merged PR when the rejected number belongs to another repository', + rejectedUrl: 'https://github.com/other/orca/pull/12', + head: 'merged-head', + preserves: true + }, + { + name: 'preserves a merged PR when the rejected number belongs to another server', + rejectedUrl: 'https://github.example.com/acme/orca/pull/12', + head: 'merged-head', + preserves: true + } + ])('$name', async ({ rejectedUrl, head, preserves }) => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/from-develop' + const worktreeId = 'wt-new-feature' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + const cachedPR = makePR({ + number: 12, + state: 'merged', + url: 'https://github.com/acme/orca/pull/12', + headSha: 'merged-head', + confirmedContainedHeadOid: 'contained-head' + }) + const cachedReview = hostedReviewInfoFromGitHubPRInfo(cachedPR) + store.setState({ + repos: [ + { id: repoId, path: repoPath, displayName: 'repo', badgeColor: '', addedAt: 1, kind: 'git' } + ], + worktreesByRepo: { + [repoId]: [makePRRefreshWorktree({ id: worktreeId, repoId, branch, head })] + }, + prCache: { [cacheKey]: { data: cachedPR, fetchedAt: 1 } }, + hostedReviewCache: { + [hostedReviewCacheKey]: { + data: cachedReview, + fetchedAt: 1, + linkedReviewHintKey: 'github:12' + } + } + }) + const outcome = { kind: 'no-pr' as const, fetchedAt: 2, rejectedPRUrls: [rejectedUrl] } + if (source === 'manual') { + mockApi.gh.refreshPRNow.mockResolvedValueOnce(outcome) + await expect( + store.getState().fetchPRForBranch(repoPath, branch, { + force: true, + repoId, + worktreeId, + fallbackPRNumber: 12, + fallbackPRSource: 'pr-cache' + }) + ).resolves.toEqual(preserves ? cachedPR : null) + } else { + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + reason: 'visible', + aliases: [ + { + cacheKey, + repoId, + repoPath, + branch, + worktreeId, + fallbackPRNumber: 12, + fallbackPRSource: 'pr-cache' + } + ], + outcome + }) + } + + expect(store.getState().prCache[cacheKey]?.data).toEqual(preserves ? cachedPR : null) + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]?.data).toEqual( + preserves ? cachedReview : null + ) + await vi.advanceTimersByTimeAsync(1000) + if (preserves) { + expect(mockApi.cache.setGitHub).not.toHaveBeenCalled() + } else { + expect(mockApi.cache.setGitHub).toHaveBeenCalledWith({ + cache: expect.objectContaining({ + pr: { [cacheKey]: expect.objectContaining({ data: null, fetchedAt: 2 }) } + }) + }) + } + }) +}) diff --git a/src/shared/github/pull-request-for-branch-outcome.test.ts b/src/shared/github/pull-request-for-branch-outcome.test.ts index 7ad79b1f61c..f3b124a3318 100644 --- a/src/shared/github/pull-request-for-branch-outcome.test.ts +++ b/src/shared/github/pull-request-for-branch-outcome.test.ts @@ -19,6 +19,13 @@ describe('normalizeGitHubPRForBranchOutcome', () => { expect(normalizeGitHubPRForBranchOutcome(outcome, 20)).toBe(outcome) }) + it.each([ + { kind: 'no-pr' as const, fetchedAt: 10 }, + { kind: 'no-pr' as const, fetchedAt: 10, rejectedPRUrls: [PR.url] } + ])('preserves no-PR responses from older and newer hosts', (outcome) => { + expect(normalizeGitHubPRForBranchOutcome(outcome, 20)).toEqual(outcome) + }) + it('normalizes legacy PRInfo and null responses', () => { expect(normalizeGitHubPRForBranchOutcome(PR, 20)).toEqual({ kind: 'found', diff --git a/src/shared/github/pull-request-refresh-types.ts b/src/shared/github/pull-request-refresh-types.ts index f735a8e0fe2..b4f61d5fd44 100644 --- a/src/shared/github/pull-request-refresh-types.ts +++ b/src/shared/github/pull-request-refresh-types.ts @@ -22,7 +22,7 @@ export type PRRefreshUpstreamErrorType = PRRefreshErrorType export type PRRefreshOutcome = | { kind: 'found'; pr: PRInfo; fetchedAt: number } - | { kind: 'no-pr'; fetchedAt: number } + | { kind: 'no-pr'; fetchedAt: number; rejectedPRUrls?: string[] } | { kind: 'upstream-error' errorType: PRRefreshErrorType