diff --git a/src/main/github/client-merge-queue-auto-merge.test.ts b/src/main/github/client-merge-queue-auto-merge.test.ts index b6fe0d535d5..f0d62ff75c6 100644 --- a/src/main/github/client-merge-queue-auto-merge.test.ts +++ b/src/main/github/client-merge-queue-auto-merge.test.ts @@ -110,7 +110,7 @@ describe('GitHub GraphQL rate-limit guard', () => { '--repo', 'stablyai/orca', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root', host: 'github.com' } ) diff --git a/src/main/github/client-pr-branch-discovery.test.ts b/src/main/github/client-pr-branch-discovery.test.ts index 82167d8bdda..3db1e981ed1 100644 --- a/src/main/github/client-pr-branch-discovery.test.ts +++ b/src/main/github/client-pr-branch-discovery.test.ts @@ -188,7 +188,7 @@ describe('getPRForBranch', () => { '--repo', 'acme/widgets', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) diff --git a/src/main/github/client-pr-fallback-number.test.ts b/src/main/github/client-pr-fallback-number.test.ts index 2c63f4ddc01..397a22fa1b5 100644 --- a/src/main/github/client-pr-fallback-number.test.ts +++ b/src/main/github/client-pr-fallback-number.test.ts @@ -126,7 +126,7 @@ describe('getPRForBranch', () => { '--repo', 'acme/widgets', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) @@ -162,7 +162,7 @@ describe('getPRForBranch', () => { '--limit', '1', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) @@ -308,7 +308,7 @@ describe('getPRForBranch', () => { '--repo', 'stablyai/orca', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) @@ -381,7 +381,7 @@ describe('getPRForBranch', () => { '--limit', '1', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) @@ -507,7 +507,7 @@ describe('getPRForBranch', () => { '--repo', 'acme/widgets', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) diff --git a/src/main/github/client-pr-linked-lookup.test.ts b/src/main/github/client-pr-linked-lookup.test.ts index 36b2d0df0ca..a7ce6cce203 100644 --- a/src/main/github/client-pr-linked-lookup.test.ts +++ b/src/main/github/client-pr-linked-lookup.test.ts @@ -80,7 +80,7 @@ describe('getPRForBranch', () => { '--repo', 'stablyai/orca', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) @@ -93,7 +93,7 @@ describe('getPRForBranch', () => { '--repo', 'fork/orca', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) @@ -132,7 +132,7 @@ describe('getPRForBranch', () => { '--repo', 'acme/widgets', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) @@ -411,7 +411,7 @@ describe('getPRForBranch', () => { '--repo', 'acme/widgets', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) @@ -502,7 +502,7 @@ describe('getPRForBranch', () => { '--repo', 'acme/widgets', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) @@ -543,7 +543,7 @@ describe('getPRForBranch', () => { '--repo', 'acme/widgets', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) @@ -579,7 +579,7 @@ describe('getPRForBranch', () => { 'view', '77', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/non-github-repo' } ) @@ -618,7 +618,7 @@ describe('getPRForBranch', () => { 'view', 'feature/test', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/non-github-repo' } ) @@ -658,7 +658,7 @@ describe('getPRForBranch', () => { '--repo', 'acme/widgets', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) 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 89082ecf2e3..361a6edb098 100644 --- a/src/main/github/client-tracked-upstream-default-branch.test.ts +++ b/src/main/github/client-tracked-upstream-default-branch.test.ts @@ -1,9 +1,4 @@ -/* - * Issue #26948: a branch created with `git switch -c feature/x origin/develop` - * tracks the default branch. Its PR lookup misses, falls back to the tracked - * upstream, and attaches the newest PR whose head is `develop` (an unrelated - * develop -> release PR), then caches that number so the link survives restarts. - */ +// #26948: tracking a base branch must not attach its integration PR to a feature branch. import { beforeEach, describe, expect, it, vi } from 'vitest' import type * as GithubApiRepositoryModule from './github-api-repository' import type * as GitHubEnterpriseRepositoryModule from './github-enterprise-repository' @@ -35,6 +30,8 @@ vi.mock('./github-api-repository', async (importOriginal) => import { getPRForBranch } from './client/lookup/get-pr-for-branch' import { getPRForBranchOutcome } from './client/lookup/pr-for-branch-outcome' import { resetPRForBranchMocks } from './client-test-harness' +import type { RestPullRequest } from './client/lookup/pull-request-lookup-data' +import type { OwnerRepo } from './gh-utils' const { ghExecFileAsyncMock, @@ -45,6 +42,8 @@ const { } = clientMocks const ACME = { owner: 'acme', repo: 'widgets' } +const UPSTREAM = { owner: 'stablyai', repo: 'widgets' } +const CONTRIBUTOR = { owner: 'contributor', repo: 'widgets' } /** remoteHeads maps a remote to the branch its `refs/remotes//HEAD` points at. */ function primeGit( @@ -71,43 +70,59 @@ function primeGit( }) } -function restPR(headOwnerRef: string): Record { +function restPR( + headRefName = 'develop', + headRepo: OwnerRepo = ACME, + prRepo: OwnerRepo = ACME +): RestPullRequest { return { number: 7, title: 'Release develop', state: 'closed', merged_at: null, - html_url: 'https://github.com/acme/widgets/pull/7', + html_url: `https://github.com/${prRepo.owner}/${prRepo.repo}/pull/7`, updated_at: '2025-01-01T00:00:00Z', draft: false, mergeable: null, base: { ref: 'release', sha: 'release-oid' }, - head: { ref: headOwnerRef, sha: 'old-develop-oid' } + head: { + ref: headRefName, + sha: 'old-develop-oid', + repo: { name: headRepo.repo, owner: { login: headRepo.owner } } + } } } -/** Branch-list responses keyed by the `head=` filter; every other gh call fails. */ -function primeGh(listsByHead: Record[]>): void { +function primeGh( + listsByHead: Record, + exactPR: RestPullRequest = Object.values(listsByHead).flat()[0] ?? restPR() +): void { + const prsByNumber = new Map( + [...Object.values(listsByHead).flat(), exactPR].map((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)] ?? []) } } - if (args[0] === 'pr' && args[1] === 'view' && args[2] === '7') { + const exact = prsByNumber.get(Number(args[2])) + if (args[0] === 'pr' && args[1] === 'view' && exact) { return { stdout: JSON.stringify({ - number: 7, - title: 'Release develop', - state: 'CLOSED', - url: 'https://github.com/acme/widgets/pull/7', + number: exact.number, + title: exact.title, + state: exact.merged_at ? 'MERGED' : exact.state.toUpperCase(), + url: exact.html_url, statusCheckRollup: [], - updatedAt: '2025-01-01T00:00:00Z', - isDraft: false, + updatedAt: exact.updated_at, + isDraft: exact.draft, mergeable: 'UNKNOWN', - baseRefName: 'release', - headRefName: 'develop', - baseRefOid: 'release-oid', - headRefOid: 'old-develop-oid' + 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 }) } } @@ -117,7 +132,6 @@ function primeGh(listsByHead: Record[]>): void { /** A fork clone: `origin` is the fork, `upstream` is the repo PRs target. */ function primeForkWithUpstream(): void { - const UPSTREAM = { owner: 'stablyai', repo: 'widgets' } resolvePRRepositoryCandidatesMock.mockResolvedValue({ candidates: [UPSTREAM, ACME], headRepo: ACME @@ -153,8 +167,8 @@ describe('issue #26948: a branch tracking the default branch', () => { it('skips the default branch on a second remote that PRs target (fork checkout off upstream)', async () => { primeForkWithUpstream() - primeGit('refs/remotes/upstream/develop') - primeGh({ 'stablyai:develop': [restPR('develop')] }) + primeGit('refs/remotes/upstream/develop', { origin: 'develop', upstream: 'develop' }) + primeGh({ 'stablyai:develop': [restPR('develop', UPSTREAM, UPSTREAM)] }) const pr = await getPRForBranch('/repo-root', 'feature/my-change') @@ -164,7 +178,7 @@ describe('issue #26948: a branch tracking the default branch', () => { it("uses the tracked remote's own default branch when it differs from origin's", async () => { primeForkWithUpstream() primeGit('refs/remotes/upstream/develop', { origin: 'main', upstream: 'develop' }) - primeGh({ 'stablyai:develop': [restPR('develop')] }) + primeGh({ 'stablyai:develop': [restPR('develop', UPSTREAM, UPSTREAM)] }) const pr = await getPRForBranch('/repo-root', 'feature/my-change') @@ -174,7 +188,7 @@ describe('issue #26948: a branch tracking the default branch', () => { it("keeps a PR headed by a tracked branch that is only origin's default", async () => { primeForkWithUpstream() primeGit('refs/remotes/upstream/develop', { origin: 'develop', upstream: 'main' }) - primeGh({ 'stablyai:develop': [restPR('develop')] }) + primeGh({ 'stablyai:develop': [restPR('develop', UPSTREAM, UPSTREAM)] }) const pr = await getPRForBranch('/repo-root', 'feature/my-change') @@ -186,10 +200,100 @@ describe('issue #26948: a branch tracking the default branch', () => { remoteName === 'contributor' ? { owner: 'contributor', repo: 'widgets' } : ACME ) primeGit('refs/remotes/contributor/develop') - primeGh({ 'contributor:develop': [{ ...restPR('develop'), state: 'open' }] }) + primeGh({ 'contributor:develop': [{ ...restPR('develop', CONTRIBUTOR), state: 'open' }] }) const pr = await getPRForBranch('/repo-root', 'feature/my-change') expect(pr).toMatchObject({ number: 7, headRepo: { owner: 'contributor', repo: 'widgets' } }) }) + + it('preserves a cached contributor PR sharing the tracked default branch name', async () => { + primeGit('refs/remotes/origin/develop') + primeGh({}, { ...restPR('develop', CONTRIBUTOR), state: 'open' }) + const pr = await getPRForBranch('/repo-root', 'feature/my-change', null, null, 7) + expect(pr).toMatchObject({ number: 7, headRepo: CONTRIBUTOR }) + }) + + it('preserves a PR from the origin fork default branch into upstream', async () => { + primeForkWithUpstream() + primeGit('refs/remotes/origin/develop') + primeGh({ 'acme:develop': [{ ...restPR('develop', ACME, UPSTREAM), state: 'open' }] }) + const pr = await getPRForBranch('/repo-root', 'feature/my-change') + expect(pr).toMatchObject({ number: 7, headRepo: ACME, prRepo: UPSTREAM }) + }) + + it('clears an old cached integration PR after upstream tracking is removed', async () => { + primeGit('') + primeGh({}) + expect( + (await getPRForBranchOutcome('/repo-root', 'feature/my-change', null, null, 7)).kind + ).toBe('no-pr') + }) + + it('tries the cached feature PR after rejecting an unrelated upstream result', async () => { + primeGit('refs/remotes/origin/develop') + primeGh( + { 'acme:develop': [restPR()] }, + { ...restPR('feature/my-change'), number: 8, state: 'open' } + ) + const pr = await getPRForBranch('/repo-root', 'feature/my-change', null, null, 8) + expect(pr).toMatchObject({ number: 8, headRefName: 'feature/my-change' }) + }) + + it('does not borrow origin default when upstream HEAD is unknown', async () => { + primeForkWithUpstream() + primeGit('refs/remotes/upstream/develop', { origin: 'develop' }) + primeGh({ 'stablyai:develop': [restPR('develop', UPSTREAM, UPSTREAM)] }) + expect(await getPRForBranch('/repo-root', 'feature/my-change')).toMatchObject({ number: 7 }) + }) + + it('uses PR head default metadata when upstream has no recorded HEAD', async () => { + primeForkWithUpstream() + primeGit('refs/remotes/upstream/develop', { origin: 'main' }) + const pr = restPR('develop', UPSTREAM, UPSTREAM) + primeGh({ + 'stablyai:develop': [ + { + ...pr, + head: { + ...pr.head, + repo: { name: 'widgets', owner: { login: 'stablyai' }, default_branch: 'develop' } + } + } + ] + }) + expect(await getPRForBranch('/repo-root', 'feature/my-change')).toBeNull() + }) + + it('preserves explicit links to integration PRs', async () => { + primeGit('refs/remotes/origin/develop') + primeGh({}) + expect(await getPRForBranch('/repo-root', 'feature/my-change', 7)).toMatchObject({ number: 7 }) + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('preserves cached review context while detached', async () => { + primeGh({}) + expect(await getPRForBranch('/repo-root', '', null, null, 7)).toMatchObject({ number: 7 }) + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('does not infer a head repository for a cached PR with unavailable fork metadata', async () => { + primeGit('refs/remotes/origin/develop') + const pr = restPR() + primeGh({}, { ...pr, head: { ref: 'develop', sha: 'deleted-fork-oid', repo: null } }) + expect(await getPRForBranch('/repo-root', 'feature/my-change', null, null, 7)).toMatchObject({ + number: 7 + }) + }) + + it('does not fetch the rejected integration PR twice when its number is cached', async () => { + primeGit('refs/remotes/origin/develop') + primeGh({ 'acme:develop': [restPR()] }) + expect(await getPRForBranch('/repo-root', 'feature/my-change', null, null, 7)).toBeNull() + const exactCalls = ghExecFileAsyncMock.mock.calls.filter( + ([args]) => args[0] === 'pr' && args[1] === 'view' + ) + expect(exactCalls).toHaveLength(1) + }) }) diff --git a/src/main/github/client-tracked-upstream-snapshot.test.ts b/src/main/github/client-tracked-upstream-snapshot.test.ts index 1eb2200415d..894c4481a09 100644 --- a/src/main/github/client-tracked-upstream-snapshot.test.ts +++ b/src/main/github/client-tracked-upstream-snapshot.test.ts @@ -113,7 +113,7 @@ describe('getPRForBranch', () => { '--repo', 'acme/widgets', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' ], { cwd: '/repo-root' } ) diff --git a/src/main/github/client/lookup/branch-lookup-resolution.ts b/src/main/github/client/lookup/branch-lookup-resolution.ts index 71751a27355..f5afb563b27 100644 --- a/src/main/github/client/lookup/branch-lookup-resolution.ts +++ b/src/main/github/client/lookup/branch-lookup-resolution.ts @@ -17,6 +17,8 @@ import { isMergedImplicitPR, shouldHideMergedImplicitPR, getCurrentHeadOid, + pullRequestHeadRepository, + linkedMergedPRDivergedHeadOid, type PullRequestLookupData, type GitHubPRBranchLookupOptions } from './pull-request-lookup-data' @@ -29,7 +31,7 @@ import { type TrackedUpstreamBranch } from './tracked-upstream-cache' import { getTrackedUpstreamBranch } from './tracked-upstream-branch' -import { isTrackedUpstreamDefaultBranch } from './tracked-upstream-default-branch' +import { shouldHideImplicitDefaultBranchPR } 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: { @@ -105,29 +107,6 @@ export async function resolvePRForBranchOutcome(input: { } return membership } - const recordLinkedMergedPRDivergence = async ( - candidate: PullRequestLookupData | null, - candidateRepo: OwnerRepo | null - ): Promise => { - if ( - typeof linkedPRNumber !== 'number' || - !candidate || - mapPRState(candidate.state, candidate.isDraft) !== 'merged' || - explicitCurrentHeadOid === null || - candidate.headRefOid === explicitCurrentHeadOid - ) { - return - } - const membership = await mergedPRContainsHead( - candidate, - candidateRepo ?? ownerRepoFromPullRequestUrl(candidate.url), - explicitCurrentHeadOid - ) - if (membership === 'not-contained') { - // explicitCurrentHeadOid is non-null here (guarded above); record the exact diverged head so consumers clear only that worktree. - headDivergedFromMergedPRAtOid = explicitCurrentHeadOid - } - } const hideMergedImplicitPR = async ( candidate: PullRequestLookupData | null, candidateRepo: OwnerRepo | null @@ -216,6 +195,29 @@ 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 @@ -223,7 +225,12 @@ export async function resolvePRForBranchOutcome(input: { dataRepo = null dataHeadRepo = headRepo } - if (!data && typeof linkedPRNumber !== 'number' && typeof fallbackPRNumber === 'number') { + if ( + !data && + typeof linkedPRNumber !== 'number' && + typeof fallbackPRNumber === 'number' && + fallbackPRNumber !== rejectedDefaultPRNumber + ) { usedExactNumberLookup = true const fallbackLookup = await lookupPRByNumber({ candidates, @@ -234,30 +241,26 @@ export async function resolvePRForBranchOutcome(input: { data = fallbackLookup.data dataRepo = fallbackLookup.dataRepo } - // Why: covers the cached fallback number too, so a link stored before #26948 heals. - if ( - data && - retriedUpstream && - data.headRefName === retriedUpstream.branch.branchName && - (await isTrackedUpstreamDefaultBranch({ - upstreamBranch: retriedUpstream.branch, - upstreamHeadRepo: retriedUpstream.headRepo, - candidates, - repoPath, - connectionId, - localGitOptions - })) - ) { - data = null - dataRepo = null - } + // 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() } } - await recordLinkedMergedPRDivergence(data, dataRepo) + dataHeadRepo = pullRequestHeadRepository(data, dataRepo) ?? dataHeadRepo + headDivergedFromMergedPRAtOid = await linkedMergedPRDivergedHeadOid( + data, + linkedPRNumber, + explicitCurrentHeadOid, + (candidate, headOid) => + mergedPRContainsHead( + candidate, + dataRepo ?? ownerRepoFromPullRequestUrl(candidate.url), + headOid + ) + ) const fallbackConfirmedMergedBranch = typeof fallbackPRNumber === 'number' && mergedBranchLookupNumber === fallbackPRNumber && diff --git a/src/main/github/client/lookup/implicit-default-branch-pr.ts b/src/main/github/client/lookup/implicit-default-branch-pr.ts new file mode 100644 index 00000000000..58f882b34d3 --- /dev/null +++ b/src/main/github/client/lookup/implicit-default-branch-pr.ts @@ -0,0 +1,60 @@ +import type { OwnerRepo } from '../../gh-utils' +import { githubRepoIdentityKey } from '../../../../shared/github/repository-identity-key' +import { + getRemoteHeadBranchName, + getRepoDefaultBranchName +} 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' + +export async function shouldHideImplicitDefaultBranchPR(input: { + data: PullRequestLookupData + prRepo: OwnerRepo | null + originHeadRepo: OwnerRepo | null + trackedUpstream: { branch: TrackedUpstreamBranch; headRepo: OwnerRepo } | null + branchName: string + linkedPRNumber?: number | null + repoPath: string + connectionId?: string | null + localGitOptions: HostedReviewLocalGitOptions +}): Promise { + const { data, branchName, prRepo } = input + if ( + !branchName || + !data.headRefName || + data.headRefName === branchName || + typeof input.linkedPRNumber === 'number' + ) { + return false + } + const headRepo = pullRequestHeadRepository(data, prRepo) + // Fork heads are real review branches even when their name is a default branch elsewhere. + if (!headRepo || !prRepo || githubRepoIdentityKey(headRepo) !== githubRepoIdentityKey(prRepo)) { + return false + } + if (data.headDefaultBranchName) { + return data.headRefName === data.headDefaultBranchName + } + const headRepoKey = githubRepoIdentityKey(headRepo) + const remoteName = + input.trackedUpstream && githubRepoIdentityKey(input.trackedUpstream.headRepo) === headRepoKey + ? input.trackedUpstream.branch.remoteName + : input.originHeadRepo && githubRepoIdentityKey(input.originHeadRepo) === headRepoKey + ? 'origin' + : null + if (!remoteName) { + return false + } + // An unknown upstream HEAD cannot borrow a different repository's origin default. + const defaultBranchName = + remoteName === 'origin' + ? await getRepoDefaultBranchName(input.repoPath, input.connectionId, input.localGitOptions) + : await getRemoteHeadBranchName( + input.repoPath, + remoteName, + input.connectionId, + input.localGitOptions + ) + return defaultBranchName !== null && data.headRefName === defaultBranchName +} 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 03c431b2e8d..60263b5915b 100644 --- a/src/main/github/client/lookup/pull-request-lookup-data.ts +++ b/src/main/github/client/lookup/pull-request-lookup-data.ts @@ -4,10 +4,11 @@ import type { PRMergeableState, PRReviewDecision } from '../../../../shared/github/pull-request-types' -import { gitExecFileAsync } from '../../gh-utils' +import { gitExecFileAsync, type OwnerRepo } from '../../gh-utils' import type { GitAdmissionTier } from '../../../git/command-runner/git-exec-options' import type { HostedReviewExecutionOptions } from '../../../source-control/hosted-review-git-options' import { mapPRState } from '../../mappers' +import type { MergedPRCommitMembership } from '../../merged-pr-commit-membership' import { normalizePRMergeable, normalizeReviewDecision, @@ -35,6 +36,9 @@ export type PullRequestLookupData = { headRefName?: string baseRefOid?: string headRefOid?: string + headRepositoryOwner?: { login?: string } | null + headRepository?: { name?: string } | null + headDefaultBranchName?: string stack?: GitHubPRStack stackMetadataChecked?: boolean } @@ -51,7 +55,12 @@ export type RestPullRequest = { mergeable?: boolean | null mergeable_state?: string | null base?: { ref?: string; sha?: string } - head?: { ref?: string; sha?: string } + head?: { + ref?: string + sha?: string + repo?: { name?: string; owner?: { login?: string }; default_branch?: string } | null + user?: { login?: string } + } stack?: { number?: number position?: number @@ -61,10 +70,10 @@ export type RestPullRequest = { } export const PR_LOOKUP_JSON_FIELDS = - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' export const PR_BRANCH_LIST_JSON_FIELDS = - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' export type GitHubPRBranchLookupOptions = HostedReviewExecutionOptions & { acceptMergedFallbackPR?: boolean @@ -118,11 +127,23 @@ export function mapRestPullRequest(pr: RestPullRequest): PullRequestLookupData { headRefName: pr.head?.ref, baseRefOid: pr.base?.sha, headRefOid: pr.head?.sha, + headRepositoryOwner: pr.head?.repo?.owner ?? pr.head?.user, + headRepository: pr.head?.repo, + headDefaultBranchName: pr.head?.repo?.default_branch, stackMetadataChecked: true, ...(stack ? { stack } : {}) } } +export function pullRequestHeadRepository( + data: PullRequestLookupData, + prRepo: OwnerRepo | null +): OwnerRepo | null { + const owner = data.headRepositoryOwner?.login?.trim() + const repo = data.headRepository?.name?.trim() + return owner && repo ? { owner, repo, ...(prRepo?.host ? { host: prRepo.host } : {}) } : null +} + export function isMergedImplicitPR( data: PullRequestLookupData, linkedPRNumber?: number | null @@ -143,6 +164,24 @@ export function shouldHideMergedImplicitPR( return !currentHeadOid || data.headRefOid !== currentHeadOid } +export async function linkedMergedPRDivergedHeadOid( + data: PullRequestLookupData | null, + linkedPRNumber: number | null | undefined, + currentHeadOid: string | null, + containsHead: (data: PullRequestLookupData, headOid: string) => Promise +): Promise { + if ( + typeof linkedPRNumber !== 'number' || + !data || + mapPRState(data.state, data.isDraft) !== 'merged' || + currentHeadOid === null || + data.headRefOid === currentHeadOid + ) { + return null + } + return (await containsHead(data, currentHeadOid)) === 'not-contained' ? currentHeadOid : null +} + export function normalizePullRequestLookupData(data: PullRequestLookupData): PullRequestLookupData { return { ...data, diff --git a/src/main/github/client/lookup/tracked-upstream-default-branch.ts b/src/main/github/client/lookup/tracked-upstream-default-branch.ts deleted file mode 100644 index baea2f0aaae..00000000000 --- a/src/main/github/client/lookup/tracked-upstream-default-branch.ts +++ /dev/null @@ -1,42 +0,0 @@ -import type { OwnerRepo } from '../../gh-utils' -import { githubRepoIdentityKey } from '../../../../shared/github/repository-identity-key' -import { - getRemoteHeadBranchName, - getRepoDefaultBranchName -} from '../../../source-control/repo-default-branch' -import type { HostedReviewLocalGitOptions } from './../github-exec-scope' -import type { TrackedUpstreamBranch } from './tracked-upstream-cache' - -/** - * #26948: `git switch -c x origin/develop` makes a branch track the default - * branch, whose PRs (e.g. develop -> release) belong to that branch, not this one. - * A same-named branch on a fork outside the PR candidates is a real head, so it stays. - */ -export async function isTrackedUpstreamDefaultBranch(input: { - upstreamBranch: TrackedUpstreamBranch - upstreamHeadRepo: OwnerRepo - candidates: OwnerRepo[] - repoPath: string - connectionId?: string | null - localGitOptions: HostedReviewLocalGitOptions -}): Promise { - const upstreamRepoKey = githubRepoIdentityKey(input.upstreamHeadRepo) - if (!input.candidates.some((candidate) => githubRepoIdentityKey(candidate) === upstreamRepoKey)) { - return false - } - const { remoteName } = input.upstreamBranch - // Why: a fork's `upstream` can default to a different branch than `origin`; use origin's only when the remote never recorded its HEAD. - const remoteDefaultBranchName = - remoteName === 'origin' - ? null - : await getRemoteHeadBranchName( - input.repoPath, - remoteName, - input.connectionId, - input.localGitOptions - ) - const defaultBranchName = - remoteDefaultBranchName ?? - (await getRepoDefaultBranchName(input.repoPath, input.connectionId, input.localGitOptions)) - return defaultBranchName === input.upstreamBranch.branchName -} diff --git a/src/main/source-control/repo-default-branch.ts b/src/main/source-control/repo-default-branch.ts index 6baaf4ce7e3..61de5b0537d 100644 --- a/src/main/source-control/repo-default-branch.ts +++ b/src/main/source-control/repo-default-branch.ts @@ -100,7 +100,7 @@ export function getRepoDefaultBranchName( ) } -/** Branch the remote's own HEAD names; null on older Git or a remote added without it. */ +/** Branch the remote's own HEAD names; null when Git has not recorded it. */ export function getRemoteHeadBranchName( repoPath: string, remoteName: string, diff --git a/src/main/source-control/repo-remote-head-branch.test.ts b/src/main/source-control/repo-remote-head-branch.test.ts new file mode 100644 index 00000000000..b2dfccaad08 --- /dev/null +++ b/src/main/source-control/repo-remote-head-branch.test.ts @@ -0,0 +1,123 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock, getSshGitProviderMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + getSshGitProviderMock: vi.fn() +})) + +vi.mock('../git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) +vi.mock('../providers/ssh-git-dispatch', () => ({ getSshGitProvider: getSshGitProviderMock })) + +import { + getRemoteHeadBranchName, + getRepoDefaultBranchName, + __resetRepoDefaultBranchCacheForTests +} from './repo-default-branch' + +function remoteHead(remote: string, branch: string): { stdout: string; stderr: string } { + return { stdout: `refs/remotes/${remote}/HEAD\0refs/remotes/${remote}/${branch}\n`, stderr: '' } +} + +describe('getRemoteHeadBranchName', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + getSshGitProviderMock.mockReset() + __resetRepoDefaultBranchCacheForTests() + }) + + it('keeps remote HEAD caches separate from each other and the repository default', async () => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => + args.includes('refs/remotes/upstream/HEA[D]') + ? remoteHead('upstream', 'develop') + : remoteHead('origin', 'main') + ) + await expect(getRepoDefaultBranchName('/repo')).resolves.toBe('main') + await expect(getRemoteHeadBranchName('/repo', 'upstream')).resolves.toBe('develop') + await expect(getRemoteHeadBranchName('/repo', 'origin')).resolves.toBe('main') + await expect(getRemoteHeadBranchName('/repo', 'upstream')).resolves.toBe('develop') + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(3) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['for-each-ref', '--format=%(refname)%00%(symref)', 'refs/remotes/upstream/HEA[D]'], + { cwd: '/repo', timeout: expect.any(Number) } + ) + }) + + it('returns null for a missing remote HEAD without resolving origin', async () => { + gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + await expect(getRemoteHeadBranchName('/repo', 'upstream')).resolves.toBeNull() + await expect(getRemoteHeadBranchName('/repo', 'upstream')).resolves.toBeNull() + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + + it('ignores a symbolic HEAD pointing into another remote', async () => { + gitExecFileAsyncMock.mockResolvedValue({ + stdout: 'refs/remotes/upstream/HEAD\0refs/remotes/origin/main\n', + stderr: '' + }) + await expect(getRemoteHeadBranchName('/repo', 'upstream')).resolves.toBeNull() + }) + + it('shares concurrent probes only for the same remote', async () => { + let releaseProbe: (() => void) | undefined + const gate = new Promise((resolve) => { + releaseProbe = resolve + }) + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + await gate + return args.includes('refs/remotes/upstream/HEA[D]') + ? remoteHead('upstream', 'develop') + : remoteHead('origin', 'main') + }) + const first = getRemoteHeadBranchName('/repo', 'upstream') + const second = getRemoteHeadBranchName('/repo', 'upstream') + const origin = getRemoteHeadBranchName('/repo', 'origin') + await vi.waitFor(() => expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2)) + releaseProbe?.() + await expect(Promise.all([first, second, origin])).resolves.toEqual([ + 'develop', + 'develop', + 'main' + ]) + }) + + it('isolates native, WSL and SSH results for the same path and remote', async () => { + gitExecFileAsyncMock.mockImplementation(async (_args, options: { wslDistro?: string }) => + remoteHead('upstream', options.wslDistro ? 'wsl-default' : 'native-default') + ) + const exec = vi.fn().mockResolvedValue(remoteHead('upstream', 'ssh-default')) + getSshGitProviderMock.mockReturnValue({ exec }) + await expect(getRemoteHeadBranchName('/repo', 'upstream')).resolves.toBe('native-default') + await expect( + getRemoteHeadBranchName('/repo', 'upstream', null, { wslDistro: 'Ubuntu' }) + ).resolves.toBe('wsl-default') + await expect(getRemoteHeadBranchName('/repo', 'upstream', 'ssh-1')).resolves.toBe('ssh-default') + expect(exec).toHaveBeenCalledWith( + ['for-each-ref', '--format=%(refname)%00%(symref)', 'refs/remotes/upstream/HEA[D]'], + '/repo', + { timeoutMs: expect.any(Number) } + ) + }) + + it('does not run local Git for a disconnected SSH host', async () => { + getSshGitProviderMock.mockReturnValue(undefined) + await expect(getRemoteHeadBranchName('/repo', 'upstream', 'ssh-gone')).resolves.toBeNull() + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('caches failed probes briefly, then retries', async () => { + const now = vi.spyOn(Date, 'now').mockReturnValue(1_000) + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('unavailable')) + .mockResolvedValue(remoteHead('upstream', 'develop')) + try { + await expect(getRemoteHeadBranchName('/repo', 'upstream')).resolves.toBeNull() + await expect(getRemoteHeadBranchName('/repo', 'upstream')).resolves.toBeNull() + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + now.mockReturnValue(32_000) + await expect(getRemoteHeadBranchName('/repo', 'upstream')).resolves.toBe('develop') + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + } finally { + now.mockRestore() + } + }) +}) diff --git a/src/shared/git-resolution-binary-compatibility.test-cases.ts b/src/shared/git-resolution-binary-compatibility.test-cases.ts index dbfc14b285e..b6c56e6b813 100644 --- a/src/shared/git-resolution-binary-compatibility.test-cases.ts +++ b/src/shared/git-resolution-binary-compatibility.test-cases.ts @@ -1,6 +1,9 @@ import { expect, it } from 'vitest' import { resolveConfiguredGitPushTarget } from './git-push-target-resolution' -import { resolveDefaultBaseRefViaExec } from './git-default-base-ref' +import { + resolveDefaultBaseRefViaExec, + resolveRemoteHeadBranchViaExec +} from './git-default-base-ref' import { buildGitSshPolicyEnv, GIT_SSH_CONFIG_ARGS, parseGitSshConfig } from './git-ssh-policy-env' export function registerGitResolutionBinaryCompatibilityCases( @@ -86,4 +89,22 @@ export function registerGitResolutionBinaryCompatibilityCases( await runGit(['update-ref', '--no-deref', 'refs/remotes/origin/HEAD', head]) await expect(resolveDefaultBaseRefViaExec(runGit)).resolves.toBe('origin/master') }) + + it('resolves each remote HEAD independently with Git 2.25-compatible ref output', async () => { + const head = (await runGit(['rev-parse', 'HEAD'])).stdout.trim() + await runGit(['update-ref', 'refs/remotes/origin/review-main', head]) + await runGit(['symbolic-ref', 'refs/remotes/origin/HEAD', 'refs/remotes/origin/review-main']) + await runGit(['update-ref', 'refs/remotes/upstream/release/stable', head]) + await runGit([ + 'symbolic-ref', + 'refs/remotes/upstream/alias', + 'refs/remotes/upstream/release/stable' + ]) + await runGit(['symbolic-ref', 'refs/remotes/upstream/HEAD', 'refs/remotes/upstream/alias']) + await expect(resolveRemoteHeadBranchViaExec(runGit, 'origin')).resolves.toBe('review-main') + await expect(resolveRemoteHeadBranchViaExec(runGit, 'upstream')).resolves.toBe('release/stable') + await expect(resolveRemoteHeadBranchViaExec(runGit, 'missing')).resolves.toBeNull() + await runGit(['update-ref', '-d', 'refs/remotes/upstream/release/stable']) + await expect(resolveRemoteHeadBranchViaExec(runGit, 'upstream')).resolves.toBeNull() + }) }