From 82c5484f66c83f56bb908f8da294d9a47cebf3e1 Mon Sep 17 00:00:00 2001 From: Neil Date: Sat, 10 Oct 2026 16:43:23 -0700 Subject: [PATCH] fix(github): scope PR selection and clear rejected cached links Apply the default-branch policy while probing each repository, so an integration PR cannot preempt a valid result or suppress the same number in another repository. Check branch heads by repository and server. Invalidate remote defaults when their review scope or SSH provider changes. Report rejected PR URLs through an optional refresh-result field, so merged integration PRs clear from both caches even at the current worktree head. Share the manual and coordinator cache update implementation. Co-authored-by: petersindex <287557875+petersindex@users.noreply.github.com> --- ...nt-tracked-upstream-default-branch.test.ts | 180 +++++++++++++++--- .../client/lookup/branch-lookup-resolution.ts | 78 ++++---- .../lookup/implicit-default-branch-pr.ts | 39 +++- .../github/client/lookup/pr-branch-lookup.ts | 33 +++- .../github/client/lookup/pr-number-lookup.ts | 12 +- .../client/lookup/pull-request-lookup-data.ts | 20 ++ .../repo-default-branch.test.ts | 3 +- .../source-control/repo-default-branch.ts | 12 +- .../repo-remote-head-branch.test.ts | 40 +++- .../src/store/github/pr-result-cache.ts | 136 ++++--------- .../store/github/pull-request-execution.ts | 4 +- .../src/store/github/refresh-event-actions.ts | 2 + .../slices/github-pr-rejected-cache.test.ts | 130 +++++++++++++ .../pull-request-for-branch-outcome.test.ts | 7 + .../github/pull-request-refresh-types.ts | 2 +- 15 files changed, 505 insertions(+), 193 deletions(-) create mode 100644 src/renderer/src/store/slices/github-pr-rejected-cache.test.ts 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