From 27d29bb61af2dbea88e1892bdd4f42c9bcbf8f6f Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 22 Oct 2026 20:31:02 -0700 Subject: [PATCH] fix(github): reject cached same-branch PRs with foreign heads (#32060) * fix(github): reject cached same-branch PRs with foreign heads * fix(github): keep same-repo cached PRs behind renamed remote URLs A stale origin URL after a repository rename or transfer made the foreign-head check reject the checkout's own same-repo PR and clear its cache. Treat a PR whose head matches its canonical base URL as local. Co-Authored-By: Claude --------- Co-authored-by: Claude --- .../client-pr-foreign-head-cache.test.ts | 196 ++++++++++++++++++ .../lookup/implicit-default-branch-pr.ts | 32 ++- 2 files changed, 226 insertions(+), 2 deletions(-) create mode 100644 src/main/github/client-pr-foreign-head-cache.test.ts diff --git a/src/main/github/client-pr-foreign-head-cache.test.ts b/src/main/github/client-pr-foreign-head-cache.test.ts new file mode 100644 index 00000000000..e60b7707040 --- /dev/null +++ b/src/main/github/client-pr-foreign-head-cache.test.ts @@ -0,0 +1,196 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GithubApiRepositoryModule from './github-api-repository' +import type * as GitHubEnterpriseRepositoryModule from './github-enterprise-repository' + +const { clientMocks, moduleMocks } = await vi.hoisted(async () => { + const moduleMocks = await import('./client-test-mocks') + return { clientMocks: moduleMocks.createGitHubClientMocks(), moduleMocks } +}) + +vi.mock('./gh-utils', () => moduleMocks.ghUtilsModuleMock(clientMocks)) +vi.mock('../git/runner', () => moduleMocks.gitRunnerModuleMock(clientMocks)) +vi.mock('./local-git-config-signature', () => + moduleMocks.localGitConfigSignatureModuleMock(clientMocks) +) +vi.mock('./github-enterprise-repository', async (importOriginal) => + moduleMocks.githubEnterpriseRepositoryModuleMock( + await importOriginal() + ) +) +vi.mock('./rate-limit', () => moduleMocks.rateLimitModuleMock(clientMocks)) +vi.mock('./github-api-repository', async (importOriginal) => + moduleMocks.githubApiRepositoryModuleMock( + clientMocks, + await importOriginal() + ) +) + +import { getPRForBranchOutcome } from './client/lookup/pr-for-branch-outcome' +import { resetPRForBranchMocks } from './client-test-harness' + +const { + ghExecFileAsyncMock, + resolvePRRepositoryCandidatesMock, + gitExecFileAsyncMock, + getOwnerRepoForRemoteMock +} = clientMocks + +const upstream = { owner: 'acme', repo: 'widgets' } +const foreign = { owner: 'contributor', repo: 'widgets' } + +function primeLookup( + options: { + head?: { owner: string; repo: string } | null + expected?: { owner: string; repo: string } | null + checkout?: { owner: string; repo: string } + trackedFork?: boolean + branchResult?: boolean + headRefName?: string + } = {} +) { + const candidate = options.checkout ?? upstream + resolvePRRepositoryCandidatesMock.mockResolvedValue({ + candidates: [candidate], + headRepo: options.expected === undefined ? candidate : options.expected + }) + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'for-each-ref') { + return { stdout: options.trackedFork ? 'refs/heads/main\0fork/main\n' : '', stderr: '' } + } + throw new Error(`Unexpected Git invocation: ${args.join(' ')}`) + }) + if (options.trackedFork) { + getOwnerRepoForRemoteMock.mockResolvedValue(foreign) + } + const head = options.head === undefined ? foreign : options.head + const pr = { + number: 7, + title: 'A pull request on main', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/7', + updatedAt: '2026-10-22T00:00:00Z', + statusCheckRollup: [], + isDraft: false, + mergeable: 'MERGEABLE', + baseRefName: 'release', + headRefName: options.headRefName ?? 'main', + headRefOid: 'foreign-head', + ...(head + ? { headRepositoryOwner: { login: head.owner }, headRepository: { name: head.repo } } + : {}) + } + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'api' && args[1].includes('/pulls?head=')) { + return { + stdout: JSON.stringify( + options.branchResult + ? [ + { + number: pr.number, + title: pr.title, + state: 'open', + html_url: pr.url, + updated_at: pr.updatedAt, + base: { ref: pr.baseRefName }, + head: { + ref: pr.headRefName, + sha: pr.headRefOid, + repo: head ? { name: head.repo, owner: { login: head.owner } } : null + } + } + ] + : [] + ) + } + } + if (args[0] === 'pr' && args[1] === 'view' && args[2] === '7') { + return { stdout: JSON.stringify(pr) } + } + throw new Error(`Unexpected GitHub invocation: ${args.join(' ')}`) + }) + return pr +} + +describe('cached pull request head repository identity', () => { + beforeEach(() => resetPRForBranchMocks(clientMocks)) + + it.each(['main', 'feature/local'])('rejects a foreign fork cached for %s', async (branch) => { + const pr = primeLookup({ headRefName: branch }) + expect(await getPRForBranchOutcome('/repo', branch, null, null, 7)).toMatchObject({ + kind: 'no-pr', + rejectedPRUrls: [pr.url] + }) + }) + + it('rejects a different repository owned by the same account', async () => { + const pr = primeLookup({ head: { owner: 'acme', repo: 'other-project' } }) + expect(await getPRForBranchOutcome('/repo', 'main', null, null, 7)).toMatchObject({ + kind: 'no-pr', + rejectedPRUrls: [pr.url] + }) + }) + + it('cannot recover a rejected branch result through its cached number', async () => { + const pr = primeLookup({ branchResult: true }) + expect(await getPRForBranchOutcome('/repo', 'main', null, null, 7)).toMatchObject({ + kind: 'no-pr', + rejectedPRUrls: [pr.url] + }) + }) + + it('keeps a cached PR from the checkout repository', async () => { + primeLookup({ head: upstream }) + expect(await getPRForBranchOutcome('/repo', 'main', null, null, 7)).toMatchObject({ + kind: 'found', + pr: { number: 7, headRepo: upstream } + }) + }) + + it('keeps a cached PR belonging to the tracked fork after branch discovery misses', async () => { + primeLookup({ trackedFork: true }) + expect(await getPRForBranchOutcome('/repo', 'main', null, null, 7)).toMatchObject({ + kind: 'found', + pr: { number: 7, headRepo: foreign } + }) + }) + + it('keeps an explicitly linked cross-fork PR', async () => { + primeLookup() + expect(await getPRForBranchOutcome('/repo', 'main', 7)).toMatchObject({ + kind: 'found', + pr: { number: 7, headRepo: foreign } + }) + }) + + it('preserves cached recovery for a renamed local review branch', async () => { + primeLookup() + expect(await getPRForBranchOutcome('/repo', 'review/local', null, null, 7)).toMatchObject({ + kind: 'found', + pr: { number: 7, headRepo: foreign } + }) + }) + + it('preserves recovery when head repository metadata is unavailable', async () => { + primeLookup({ head: null }) + expect(await getPRForBranchOutcome('/repo', 'main', null, null, 7)).toMatchObject({ + kind: 'found', + pr: { number: 7 } + }) + }) + + it('preserves recovery when the checkout head repository is unknown', async () => { + primeLookup({ expected: null }) + expect(await getPRForBranchOutcome('/repo', 'main', null, null, 7)).toMatchObject({ + kind: 'found', + pr: { number: 7, headRepo: foreign } + }) + }) + + it('keeps a same-repository PR when origin still uses the pre-rename URL', async () => { + primeLookup({ head: upstream, checkout: { owner: 'acme', repo: 'old-widgets' } }) + expect(await getPRForBranchOutcome('/repo', 'main', null, null, 7)).toMatchObject({ + kind: 'found', + pr: { number: 7, headRepo: upstream } + }) + }) +}) 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 ca852798bea..fb50608d4d7 100644 --- a/src/main/github/client/lookup/implicit-default-branch-pr.ts +++ b/src/main/github/client/lookup/implicit-default-branch-pr.ts @@ -4,10 +4,15 @@ import { getRemoteHeadBranchName, getRepoDefaultBranchName } from '../../../source-control/repo-default-branch' -import type { GhExecOptions, HostedReviewLocalGitOptions } from './../github-exec-scope' +import { + ownerRepoFromPullRequestUrl, + type GhExecOptions, + type HostedReviewLocalGitOptions +} from './../github-exec-scope' import type { TrackedUpstreamBranch } from './tracked-upstream-cache' import { pullRequestHeadRepository, + pullRequestMatchesHeadRepository, type PullRequestLookupData, type PullRequestLookupPolicy } from './pull-request-lookup-data' @@ -32,7 +37,9 @@ export function createImplicitDefaultBranchPRPolicy( return { rejectedPRUrls, async accepts(data, prRepo) { - const hidden = await shouldHideImplicitDefaultBranchPR({ ...context, data, prRepo }) + const hidden = + isForeignSameNamedHead(context, data, prRepo) || + (await shouldHideImplicitDefaultBranchPR({ ...context, data, prRepo })) if (hidden && prRepo) { rejected.add(`${githubRepoIdentityKey(prRepo)}#${data.number}`) if (data.url) { @@ -47,6 +54,27 @@ export function createImplicitDefaultBranchPRPolicy( } } +// Cached numbers must not revive a foreign head rejected by branch discovery. +function isForeignSameNamedHead( + context: ImplicitDefaultBranchPRContext, + data: PullRequestLookupData, + prRepo: OwnerRepo | null +): boolean { + const expectedHeads = [context.originHeadRepo, context.trackedUpstream?.headRepo].filter( + (head) => head != null + ) + if (data.headRefName !== context.branchName || expectedHeads.length === 0) { + return false + } + const headRepo = pullRequestHeadRepository(data, prRepo) + const baseRepo = data.url ? ownerRepoFromPullRequestUrl(data.url) : null + // Why: a same-repo PR lives in a checkout remote; its canonical URL survives stale renamed-remote URLs. + if (headRepo && baseRepo && githubRepoIdentityKey(headRepo) === githubRepoIdentityKey(baseRepo)) { + return false + } + return expectedHeads.every((head) => !pullRequestMatchesHeadRepository(data, prRepo, head)) +} + async function shouldHideImplicitDefaultBranchPR( input: ImplicitDefaultBranchPRContext & { data: PullRequestLookupData; prRepo: OwnerRepo | null } ): Promise {