From e71709bbc9227742022edf3b67ef48e0f9809855 Mon Sep 17 00:00:00 2001 From: Kelvin Amoaba <97001695+AmoabaKelvin@users.noreply.github.com> Date: Thu, 15 Oct 2026 22:14:48 +0000 Subject: [PATCH] fix(pr): show the open PR opened from where a branch pushes Branches matched reviews by name only, so a newer closed PR, another fork's same-named branch, or the base branch's PR could stand in for the branch's own. Fixes #28324 --- src/main/azure-devops/client.test.ts | 148 ++-- src/main/azure-devops/client.ts | 137 ++-- src/main/azure-devops/pull-request-mappers.ts | 1 + src/main/bitbucket/client.test.ts | 74 +- src/main/bitbucket/client.ts | 92 ++- .../bitbucket/pull-request-creation.test.ts | 4 +- src/main/bitbucket/pull-request-mappers.ts | 1 + .../gitea/client-scan-invalidation.test.ts | 2 +- src/main/gitea/client.test.ts | 48 +- src/main/gitea/client.ts | 126 +-- src/main/gitea/pull-request-mappers.ts | 17 + .../github/client-pr-branch-discovery.test.ts | 6 +- .../github/client-pr-conflict-summary.test.ts | 30 +- .../github/client-pr-fallback-number.test.ts | 339 +------- src/main/github/client-pr-review-head.test.ts | 362 +++++++++ ...nt-ssh-provider-execution-boundary.test.ts | 87 +-- src/main/github/client-test-harness.ts | 14 +- src/main/github/client-test-mocks.ts | 9 +- ...nt-tracked-upstream-default-branch.test.ts | 515 ------------- ...client-tracked-upstream-fork-owner.test.ts | 200 ----- .../client-tracked-upstream-snapshot.test.ts | 728 ------------------ src/main/github/client.ts | 4 - .../client/lookup/branch-lookup-resolution.ts | 141 ++-- .../lookup/implicit-default-branch-pr.ts | 103 --- .../github/client/lookup/pr-branch-lookup.ts | 185 ++--- .../client/lookup/review-head-pr-policy.ts | 53 ++ .../client/lookup/tracked-upstream-branch.ts | 181 ----- .../client/lookup/tracked-upstream-cache.ts | 146 ---- .../github/default-branch-stale-pr.test.ts | 14 +- .../gitlab/client-mr-branch-lookup.test.ts | 94 ++- src/main/gitlab/client-mr-test-harness.ts | 13 + src/main/gitlab/gitlab-project-id.ts | 47 ++ src/main/gitlab/mappers.ts | 2 + src/main/gitlab/merge-request-lookup.ts | 88 ++- .../source-control/branch-review-selection.ts | 28 + ...osted-review-bitbucket.integration.test.ts | 13 +- ...hosted-review-creation-eligibility.test.ts | 11 +- .../source-control/hosted-review-creation.ts | 3 +- .../hosted-review-gitea.integration.test.ts | 7 +- .../repo-git-config-snapshot.test.ts | 72 ++ .../repo-git-config-snapshot.ts | 108 +++ src/main/source-control/review-head.ts | 28 + src/shared/git-config-snapshot-runner.ts | 13 +- src/shared/git-review-head.test.ts | 93 +++ src/shared/git-review-head.ts | 62 ++ 45 files changed, 1704 insertions(+), 2745 deletions(-) create mode 100644 src/main/github/client-pr-review-head.test.ts delete mode 100644 src/main/github/client-tracked-upstream-default-branch.test.ts delete mode 100644 src/main/github/client-tracked-upstream-fork-owner.test.ts delete mode 100644 src/main/github/client-tracked-upstream-snapshot.test.ts delete mode 100644 src/main/github/client/lookup/implicit-default-branch-pr.ts create mode 100644 src/main/github/client/lookup/review-head-pr-policy.ts delete mode 100644 src/main/github/client/lookup/tracked-upstream-branch.ts delete mode 100644 src/main/github/client/lookup/tracked-upstream-cache.ts create mode 100644 src/main/gitlab/gitlab-project-id.ts create mode 100644 src/main/source-control/branch-review-selection.ts create mode 100644 src/main/source-control/repo-git-config-snapshot.test.ts create mode 100644 src/main/source-control/repo-git-config-snapshot.ts create mode 100644 src/main/source-control/review-head.ts create mode 100644 src/shared/git-review-head.test.ts create mode 100644 src/shared/git-review-head.ts diff --git a/src/main/azure-devops/client.test.ts b/src/main/azure-devops/client.test.ts index 679074df53f9..791fe4f9777a 100644 --- a/src/main/azure-devops/client.test.ts +++ b/src/main/azure-devops/client.test.ts @@ -42,6 +42,7 @@ describe('Azure DevOps client', () => { }) afterEach(() => { + vi.unstubAllGlobals() process.env = OLD_ENV globalThis.fetch = OLD_FETCH _resetAzureDevOpsRepoRefCache() @@ -200,60 +201,119 @@ describe('Azure DevOps client', () => { ).rejects.toThrow(/Azure DevOps request failed/) }) - it('uses the most recent branch PR instead of preferring stale active PRs', async () => { + it('shows the active branch PR over a newer completed one (#28324)', async () => { gitExecFileAsyncMock.mockResolvedValue({ stdout: 'https://dev.azure.com/acme/Project/_git/repo\n' }) - globalThis.fetch = vi.fn(async (input: string | URL | Request) => { - const url = new URL(String(input)) - const response = (body: unknown): Response => - new Response(JSON.stringify(body), { - status: 200, - headers: { 'Content-Type': 'application/json' } - }) + vi.stubGlobal( + 'fetch', + vi.fn(async (input: string | URL | Request) => { + const url = new URL(String(input)) + const response = (body: unknown): Response => + new Response(JSON.stringify(body), { + status: 200, + headers: { 'Content-Type': 'application/json' } + }) + + if (url.pathname === '/acme/Project/_apis/git/repositories/repo') { + return response({ id: 'repo-guid' }) + } + if (url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests') { + expect(url.searchParams.get('searchCriteria.sourceRefName')).toBe( + 'refs/heads/feature/azure' + ) + expect(url.searchParams.get('searchCriteria.sourceRepositoryId')).toBe('repo-guid') + expect(url.searchParams.get('searchCriteria.status')).toBe('all') + return response({ + value: [ + { + pullRequestId: 20, + title: 'Open PR', + status: 'active', + creationDate: '2026-05-10T00:00:00Z', + mergeStatus: 'succeeded', + lastMergeSourceCommit: { commitId: 'oldsha' } + }, + { + pullRequestId: 21, + title: 'Latest completed PR', + status: 'completed', + creationDate: '2026-05-15T00:00:00Z', + closedDate: '2026-05-16T00:00:00Z', + mergeStatus: 'succeeded', + lastMergeSourceCommit: { commitId: 'newsha' } + } + ] + }) + } + if ( + url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests/20/statuses' + ) { + return response({ value: [{ state: 'succeeded' }] }) + } + return new Response(JSON.stringify({ message: 'not found' }), { status: 404 }) + }) + ) + + await expect( + getAzureDevOpsPullRequestForBranch('/repo', 'refs/heads/feature/azure') + ).resolves.toMatchObject({ + number: 20, + title: 'Open PR', + state: 'open', + headSha: 'oldsha' + }) + }) + it('shows the linked PR without consulting the branch', async () => { + gitExecFileAsyncMock.mockResolvedValue({ + stdout: 'https://dev.azure.com/acme/Project/_git/repo\n' + }) + const fetchMock = vi.fn(async (input: string | URL | Request) => { + const url = new URL(String(input)) if (url.pathname === '/acme/Project/_apis/git/repositories/repo') { - return response({ id: 'repo-guid' }) - } - if (url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests') { - return response({ - value: [ - { - pullRequestId: 20, - title: 'Stale active PR', - status: 'active', - creationDate: '2026-05-10T00:00:00Z', - mergeStatus: 'succeeded', - lastMergeSourceCommit: { commitId: 'oldsha' } - }, - { - pullRequestId: 21, - title: 'Latest completed PR', - status: 'completed', - creationDate: '2026-05-15T00:00:00Z', - closedDate: '2026-05-16T00:00:00Z', - mergeStatus: 'succeeded', - lastMergeSourceCommit: { commitId: 'newsha' } - } - ] - }) + return Response.json({ id: 'repo-guid' }) } - if ( - url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests/21/statuses' - ) { - return response({ value: [{ state: 'succeeded' }] }) + if (url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests/42') { + return Response.json({ pullRequestId: 42, title: 'Linked PR', status: 'active' }) } - return new Response(JSON.stringify({ message: 'not found' }), { status: 404 }) - }) as never + return Response.json({ value: [] }) + }) + vi.stubGlobal('fetch', fetchMock) await expect( - getAzureDevOpsPullRequestForBranch('/repo', 'refs/heads/feature/azure') - ).resolves.toMatchObject({ - number: 21, - title: 'Latest completed PR', - state: 'merged', - headSha: 'newsha' + getAzureDevOpsPullRequestForBranch('/repo', 'feature/azure', 42) + ).resolves.toMatchObject({ number: 42, title: 'Linked PR' }) + expect( + fetchMock.mock.calls.some(([input]) => + new URL(String(input)).pathname.endsWith('/pullRequests') + ) + ).toBe(false) + }) + + it('ignores a same-named branch from a fork', async () => { + gitExecFileAsyncMock.mockResolvedValue({ + stdout: 'https://dev.azure.com/acme/Project/_git/repo\n' }) + vi.stubGlobal( + 'fetch', + vi.fn(async (input: string | URL | Request) => + new URL(String(input)).pathname === '/acme/Project/_apis/git/repositories/repo' + ? Response.json({ id: 'repo-guid' }) + : Response.json({ + value: [ + { + pullRequestId: 5, + title: 'Fork PR', + status: 'active', + forkSource: { repository: { id: 'fork-guid' } } + } + ] + }) + ) + ) + + await expect(getAzureDevOpsPullRequestForBranch('/repo', 'feature/azure')).resolves.toBeNull() }) it('hides a stale completed/abandoned PR whose source branch is the repo default branch (#9171)', async () => { diff --git a/src/main/azure-devops/client.ts b/src/main/azure-devops/client.ts index 9ddae9ffe457..15861eafbe95 100644 --- a/src/main/azure-devops/client.ts +++ b/src/main/azure-devops/client.ts @@ -7,7 +7,16 @@ import { type RawAzureDevOpsStatus } from './pull-request-mappers' import { shouldHideNonOpenReviewOnDefaultBranch } from '../source-control/repo-default-branch' -import { getAzureDevOpsRepoRef, type AzureDevOpsRepoRef } from './repository-ref' +import { + BRANCH_REVIEW_PAGE_SIZE, + selectReviewForHead +} from '../source-control/branch-review-selection' +import { resolveBranchReviewTarget } from '../source-control/review-head' +import { + getAzureDevOpsRepoRef, + getAzureDevOpsRepoRefForRemote, + type AzureDevOpsRepoRef +} from './repository-ref' import { getHostedReviewLocalGitOptions, type HostedReviewExecutionOptions @@ -111,25 +120,6 @@ async function normalizePullRequest( return mapAzureDevOpsPullRequest(raw, deriveAzureDevOpsStatus(statuses), webBaseUrl) } -function sortPullRequestsForBranch( - left: RawAzureDevOpsPullRequest, - right: RawAzureDevOpsPullRequest -): number { - const leftStatus = left.status?.trim().toLowerCase() - const rightStatus = right.status?.trim().toLowerCase() - const abandonedOrder = Number(leftStatus === 'abandoned') - Number(rightStatus === 'abandoned') - // Why: abandoned PRs can have newer close dates, but should not hide a usable branch PR. - if (abandonedOrder !== 0) { - return abandonedOrder - } - const leftTime = Date.parse(left.closedDate ?? left.creationDate ?? '') || 0 - const rightTime = Date.parse(right.closedDate ?? right.creationDate ?? '') || 0 - if (leftTime !== rightTime) { - return rightTime - leftTime - } - return Number(rightStatus === 'active') - Number(leftStatus === 'active') -} - export async function getAzureDevOpsAuthStatus(): Promise { const config = getAzureDevOpsAuthConfig() const baseUrl = config.apiBaseUrl ? normalizeAzureDevOpsApiBaseUrl(config.apiBaseUrl) : null @@ -207,60 +197,89 @@ export async function getAzureDevOpsPullRequestForBranch( return null } - const repo = await getAzureDevOpsRepoRef( - repoPath, - connectionId, - getHostedReviewLocalGitOptions(options) - ) + const localGitOptions = getHostedReviewLocalGitOptions(options) + const repo = await getAzureDevOpsRepoRef(repoPath, connectionId, localGitOptions) const repository = repo ? await getRepository(repo) : null if (!repo || !repository) { return null } + const { linkedNumber, head } = await resolveBranchReviewTarget( + repoPath, + branchName, + linkedPRNumber, + connectionId, + localGitOptions + ) - if (branchName) { - const list = await requestAzureDevOpsJson<{ value?: RawAzureDevOpsPullRequest[] }>( + if (linkedNumber !== null) { + const raw = await requestAzureDevOpsJson( repo, - `/_apis/git/repositories/${encodePathSegment(repository.idOrName)}/pullRequests`, - { - searchParams: { - 'searchCriteria.sourceRefName': `refs/heads/${branchName}`, - 'searchCriteria.status': 'all', - $top: 10 - } - }, + `/_apis/git/repositories/${encodePathSegment(repository.idOrName)}/pullRequests/${encodePathSegment( + String(linkedNumber) + )}`, + {}, throwOnFailure ) - const raw = (list?.value ?? []).sort(sortPullRequestsForBranch)[0] if (raw) { - // Why (#9171): discard a non-open implicit branch match on the repo - // default branch and fall through to the linked-number fallback below. - const hideOnDefaultBranch = await shouldHideNonOpenReviewOnDefaultBranch({ - state: mapAzureDevOpsPullRequestState(raw), - reviewNumber: raw.pullRequestId ?? null, - linkedReviewNumber: linkedPRNumber, - branchName, - repoPath, - connectionId, - localGitOptions: getHostedReviewLocalGitOptions(options) - }) - if (!hideOnDefaultBranch) { - return normalizePullRequest(repo, repository.idOrName, repository.webBaseUrl, raw) - } + return normalizePullRequest(repo, repository.idOrName, repository.webBaseUrl, raw) } } - if (typeof linkedPRNumber !== 'number') { + if (!head) { return null } - const raw = await requestAzureDevOpsJson( - repo, - `/_apis/git/repositories/${encodePathSegment(repository.idOrName)}/pullRequests/${encodePathSegment( - String(linkedPRNumber) - )}`, - {}, - throwOnFailure + const headRepositoryId = + head.remote === 'origin' + ? null + : await getAzureDevOpsRepoRefForRemote(repoPath, head.remote, connectionId, localGitOptions) + .then((headRepo) => (headRepo ? getRepository(headRepo) : null)) + .then((headRepository) => headRepository?.idOrName ?? null) + if (head.remote !== 'origin' && !headRepositoryId) { + return null + } + const isFromHead = (pr: RawAzureDevOpsPullRequest): boolean => + headRepositoryId === null ? !pr.forkSource : pr.forkSource?.repository?.id === headRepositoryId + const raw = await selectReviewForHead( + async (states) => { + const list = await requestAzureDevOpsJson<{ value?: RawAzureDevOpsPullRequest[] }>( + repo, + `/_apis/git/repositories/${encodePathSegment(repository.idOrName)}/pullRequests`, + { + searchParams: { + 'searchCriteria.sourceRefName': `refs/heads/${head.branch}`, + 'searchCriteria.sourceRepositoryId': headRepositoryId ?? repository.idOrName, + 'searchCriteria.status': states === 'open' ? 'active' : 'all', + $top: BRANCH_REVIEW_PAGE_SIZE + } + }, + throwOnFailure + ) + // Why: Azure doesn't document its order, so sort newest first here. + return (list?.value ?? []) + .filter(isFromHead) + .sort( + (left, right) => + (Date.parse(right.creationDate ?? '') || 0) - (Date.parse(left.creationDate ?? '') || 0) + ) + }, + (pr) => pr.status?.trim().toLowerCase() === 'active' ) - return raw ? normalizePullRequest(repo, repository.idOrName, repository.webBaseUrl, raw) : null + if (!raw) { + return null + } + // Why (#9171): discard a non-open implicit branch match on the repo default branch. + const hideOnDefaultBranch = await shouldHideNonOpenReviewOnDefaultBranch({ + state: mapAzureDevOpsPullRequestState(raw), + reviewNumber: raw.pullRequestId ?? null, + linkedReviewNumber: linkedPRNumber, + branchName, + repoPath, + connectionId, + localGitOptions + }) + return hideOnDefaultBranch + ? null + : normalizePullRequest(repo, repository.idOrName, repository.webBaseUrl, raw) } /** diff --git a/src/main/azure-devops/pull-request-mappers.ts b/src/main/azure-devops/pull-request-mappers.ts index 132f98260cf2..09c897e5c8e9 100644 --- a/src/main/azure-devops/pull-request-mappers.ts +++ b/src/main/azure-devops/pull-request-mappers.ts @@ -14,6 +14,7 @@ export type RawAzureDevOpsPullRequest = { closedDate?: string | null mergeStatus?: string | null sourceRefName?: string | null + forkSource?: { repository?: { id?: string | null } | null } | null lastMergeSourceCommit?: { commitId?: string | null } | null diff --git a/src/main/bitbucket/client.test.ts b/src/main/bitbucket/client.test.ts index 943e74b70b26..ff91c2cf5471 100644 --- a/src/main/bitbucket/client.test.ts +++ b/src/main/bitbucket/client.test.ts @@ -75,7 +75,13 @@ describe('Bitbucket client', () => { return Response.json({ values: [] }) } return Response.json({ - values: [{ ...bitbucketPr(7), state: 'DECLINED', source: { branch: { name: 'main' } } }] + values: [ + { + ...bitbucketPr(7), + state: 'DECLINED', + source: { branch: { name: 'main' }, repository: { full_name: 'team/repo' } } + } + ] }) }) vi.stubGlobal('fetch', fetchMock) @@ -90,7 +96,14 @@ describe('Bitbucket client', () => { } return Response.json({ values: [ - { ...bitbucketPr(8), source: { branch: { name: 'main' }, commit: { hash: 'abc123' } } } + { + ...bitbucketPr(8), + source: { + branch: { name: 'main' }, + commit: { hash: 'abc123' }, + repository: { full_name: 'team/repo' } + } + } ] }) }) @@ -111,7 +124,13 @@ describe('Bitbucket client', () => { return Response.json(bitbucketPr(42)) } return Response.json({ - values: [{ ...bitbucketPr(7), state: 'MERGED', source: { branch: { name: 'main' } } }] + values: [ + { + ...bitbucketPr(7), + state: 'MERGED', + source: { branch: { name: 'main' }, repository: { full_name: 'team/repo' } } + } + ] }) }) vi.stubGlobal('fetch', fetchMock) @@ -121,24 +140,27 @@ describe('Bitbucket client', () => { ).resolves.toMatchObject({ number: 42 }) }) - it('hides a MERGED PR matched only by feature-branch name so a new PR can be created', async () => { + it('shows a MERGED PR for its branch, like every other provider', async () => { const fetchMock = vi.fn(async (url: string) => { if (url.includes('/statuses/build')) { return Response.json({ values: [] }) } return Response.json({ values: [ - { ...bitbucketPr(7), state: 'MERGED', source: { branch: { name: 'feature/login' } } } + { + ...bitbucketPr(7), + state: 'MERGED', + source: { branch: { name: 'feature/login' }, repository: { full_name: 'team/repo' } } + } ] }) }) vi.stubGlobal('fetch', fetchMock) - // Why: a merged branch match is history. Returning it made eligibility - // report "a pull request already exists" and blocked the branch's next PR. + // Create eligibility only blocks on open reviews, so a merged one no longer blocks the next PR. await expect( getBitbucketPullRequestForBranch('/repo', 'refs/heads/feature/login') - ).resolves.toBeNull() + ).resolves.toMatchObject({ number: 7, state: 'merged' }) }) it('keeps a DECLINED PR on a feature branch visible', async () => { @@ -148,19 +170,47 @@ describe('Bitbucket client', () => { } return Response.json({ values: [ - { ...bitbucketPr(9), state: 'DECLINED', source: { branch: { name: 'feature/login' } } } + { + ...bitbucketPr(9), + state: 'DECLINED', + source: { branch: { name: 'feature/login' }, repository: { full_name: 'team/repo' } } + } ] }) }) vi.stubGlobal('fetch', fetchMock) - // Why: only merged matches are hidden. Hiding declined too made a declined - // PR permanently invisible off the default branch, unlike every other provider. await expect( getBitbucketPullRequestForBranch('/repo', 'refs/heads/feature/login') ).resolves.toMatchObject({ number: 9, state: 'closed' }) }) + it('shows the OPEN PR over a newer DECLINED one from the same branch (#28324)', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async (url: string) => + url.includes('/statuses/build') + ? Response.json({ values: [] }) + : Response.json({ values: [{ ...bitbucketPr(9), state: 'DECLINED' }, bitbucketPr(3)] }) + ) + ) + + await expect( + getBitbucketPullRequestForBranch('/repo', 'feature/bitbucket') + ).resolves.toMatchObject({ number: 3, state: 'open' }) + }) + + it('ignores a same-named branch from another fork', async () => { + const forkPr = bitbucketPr(5) + forkPr.source.repository.full_name = 'alice/repo' + vi.stubGlobal( + 'fetch', + vi.fn(async () => Response.json({ values: [forkPr] })) + ) + + await expect(getBitbucketPullRequestForBranch('/repo', 'feature/bitbucket')).resolves.toBeNull() + }) + it('returns null instead of querying anonymously when no credential resolves', async () => { delete process.env.ORCA_BITBUCKET_EMAIL delete process.env.ORCA_BITBUCKET_API_TOKEN @@ -273,7 +323,7 @@ describe('Bitbucket client', () => { const parsed = new URL(listUrl) expect(parsed.pathname).toBe('/2.0/repositories/team/repo/pullrequests') expect(parsed.searchParams.get('q')).toBe( - 'source.branch.name = "feature/bitbucket" AND (state = "OPEN" OR state = "MERGED" OR state = "DECLINED" OR state = "SUPERSEDED")' + 'source.repository.full_name = "team/repo" AND source.branch.name = "feature/bitbucket" AND (state = "OPEN" OR state = "MERGED" OR state = "DECLINED" OR state = "SUPERSEDED")' ) expect(parsed.searchParams.getAll('state')).toEqual([ 'OPEN', diff --git a/src/main/bitbucket/client.ts b/src/main/bitbucket/client.ts index 86c22aeb1933..261d1b3e30a9 100644 --- a/src/main/bitbucket/client.ts +++ b/src/main/bitbucket/client.ts @@ -7,8 +7,17 @@ import { type RawBitbucketBuildStatus, type RawBitbucketPullRequest } from './pull-request-mappers' -import { getBitbucketRepoRef, type BitbucketRepoRef } from './repository-ref' +import { + getBitbucketRepoRef, + getBitbucketRepoRefForRemote, + type BitbucketRepoRef +} from './repository-ref' import { shouldHideNonOpenReviewOnDefaultBranch } from '../source-control/repo-default-branch' +import { + BRANCH_REVIEW_PAGE_SIZE, + selectReviewForHead +} from '../source-control/branch-review-selection' +import { resolveBranchReviewTarget } from '../source-control/review-head' import { getHostedReviewLocalGitOptions, type HostedReviewExecutionOptions @@ -120,8 +129,8 @@ function escapeBitbucketQueryString(value: string): string { return value.replace(/\\/g, '\\\\').replace(/"/g, '\\"') } -function allStateFilter(): string { - return `(${ALL_PULL_REQUEST_STATES.map((state) => `state = "${state}"`).join(' OR ')})` +function stateFilter(states: readonly string[]): string { + return `(${states.map((state) => `state = "${state}"`).join(' OR ')})` } async function getBuildStatus( @@ -216,11 +225,8 @@ export async function getBitbucketPullRequestForBranch( return null } - const repo = await getBitbucketRepoRef( - repoPath, - connectionId, - getHostedReviewLocalGitOptions(options) - ) + const localGitOptions = getHostedReviewLocalGitOptions(options) + const repo = await getBitbucketRepoRef(repoPath, connectionId, localGitOptions) if (!repo) { return null } @@ -237,43 +243,59 @@ export async function getBitbucketPullRequestForBranch( } } - if (branchName) { - const query = [ - `source.branch.name = "${escapeBitbucketQueryString(branchName)}"`, - allStateFilter() - ].join(' AND ') - const list = await requestJson<{ values?: RawBitbucketPullRequest[] }>( - `/repositories/${encodedRepoPath(repo)}/pullrequests`, - { - searchParams: { - pagelen: '1', - sort: '-updated_on', - q: query, - state: ALL_PULL_REQUEST_STATES - } + // Why: a stale link falls back to the branch, so resolve its head as if unlinked. + const { head } = await resolveBranchReviewTarget( + repoPath, + branchName, + null, + connectionId, + localGitOptions + ) + const headRepo = !head + ? null + : head.remote === 'origin' + ? repo + : await getBitbucketRepoRefForRemote(repoPath, head.remote, connectionId, localGitOptions) + if (head && headRepo) { + const headFullName = `${headRepo.workspace}/${headRepo.repoSlug}` + const raw = await selectReviewForHead( + async (states) => { + const pullRequestStates = states === 'open' ? ['OPEN'] : ALL_PULL_REQUEST_STATES + const list = await requestJson<{ values?: RawBitbucketPullRequest[] }>( + `/repositories/${encodedRepoPath(repo)}/pullrequests`, + { + searchParams: { + pagelen: String(BRANCH_REVIEW_PAGE_SIZE), + sort: '-updated_on', + // Why: `q=` drops Bitbucket's default open-only filter, so name every state. + q: [ + `source.repository.full_name = "${escapeBitbucketQueryString(headFullName)}"`, + `source.branch.name = "${escapeBitbucketQueryString(head.branch)}"`, + stateFilter(pullRequestStates) + ].join(' AND '), + state: pullRequestStates + } + }, + throwOnFailure + ) + return (list?.values ?? []).filter( + (pr) => pr.source?.repository?.full_name?.toLowerCase() === headFullName.toLowerCase() + ) }, - throwOnFailure + (pr) => mapBitbucketPullRequestState(pr.state) === 'open' ) - const raw = list?.values?.[0] if (raw) { - const state = mapBitbucketPullRequestState(raw.state) - // Why: a merged PR we only matched by branch name is history, not review - // context (GitHub's isMergedImplicitPR rule). Keeping it reported "a pull - // request already exists" and blocked the branch's next PR. Scoped to - // merged so a declined PR stays visible off the default branch, matching - // every other provider; the linked lookup above already returned early - // for an explicitly linked review. - const isMergedImplicitMatch = state === 'merged' + // Why (#9171): discard a non-open implicit branch match on the repo default branch. const hideOnDefaultBranch = await shouldHideNonOpenReviewOnDefaultBranch({ - state, + state: mapBitbucketPullRequestState(raw.state), reviewNumber: raw.id ?? null, linkedReviewNumber: linkedPRNumber, branchName, repoPath, connectionId, - localGitOptions: getHostedReviewLocalGitOptions(options) + localGitOptions }) - if (!isMergedImplicitMatch && !hideOnDefaultBranch) { + if (!hideOnDefaultBranch) { return normalizePullRequest(repo, raw) } } diff --git a/src/main/bitbucket/pull-request-creation.test.ts b/src/main/bitbucket/pull-request-creation.test.ts index cb0e0b26236f..57d4de16c350 100644 --- a/src/main/bitbucket/pull-request-creation.test.ts +++ b/src/main/bitbucket/pull-request-creation.test.ts @@ -34,7 +34,7 @@ function createdPullRequestResponse(): Response { state: 'OPEN', updated_on: '2026-08-11T00:00:00Z', links: { html: { href: 'https://bitbucket.org/team/repo/pull-requests/42' } }, - source: { branch: { name: 'feature/login' } }, + source: { branch: { name: 'feature/login' }, repository: { full_name: 'team/repo' } }, destination: { branch: { name: 'main' } } }) } @@ -132,7 +132,7 @@ describe('Bitbucket pull request creation', () => { state: 'OPEN', updated_on: '2026-08-11T00:00:00Z', links: { html: { href: 'https://bitbucket.org/team/repo/pull-requests/7' } }, - source: { branch: { name: 'feature/login' } }, + source: { branch: { name: 'feature/login' }, repository: { full_name: 'team/repo' } }, destination: { branch: { name: 'main' } } } ] diff --git a/src/main/bitbucket/pull-request-mappers.ts b/src/main/bitbucket/pull-request-mappers.ts index 7b9c5a07e84b..b28e9868323c 100644 --- a/src/main/bitbucket/pull-request-mappers.ts +++ b/src/main/bitbucket/pull-request-mappers.ts @@ -17,6 +17,7 @@ export type RawBitbucketPullRequest = { commit?: { hash?: string } | null + repository?: { full_name?: string } | null } destination?: { branch?: { diff --git a/src/main/gitea/client-scan-invalidation.test.ts b/src/main/gitea/client-scan-invalidation.test.ts index d25fcd07f246..ba9c9b5f7c67 100644 --- a/src/main/gitea/client-scan-invalidation.test.ts +++ b/src/main/gitea/client-scan-invalidation.test.ts @@ -26,7 +26,7 @@ const pullRequest = { title: 'Add Gitea', state: 'open', html_url: 'https://git.example.com/team/repo/pulls/7', - head: { ref: branch } + head: { ref: branch, repo: { full_name: 'team/repo' } } } function createReview() { diff --git a/src/main/gitea/client.test.ts b/src/main/gitea/client.test.ts index ab581ab39429..d547d18face2 100644 --- a/src/main/gitea/client.test.ts +++ b/src/main/gitea/client.test.ts @@ -48,8 +48,9 @@ function giteaPr(index = 7, branch = 'feature/gitea') { mergeable: true, head: { ref: branch, - label: `team:${branch}`, - sha: 'abc123' + label: branch, + sha: 'abc123', + repo: { full_name: 'team/repo' } } } } @@ -368,6 +369,49 @@ describe('Gitea client', () => { }) }) + it('shows the linked PR even when the branch matches another one', async () => { + const fetchMock = vi.fn(async (url: string | URL) => { + if (String(url).includes('/status')) { + return Response.json({ state: 'success' }) + } + return String(url).endsWith('/pulls/42') + ? Response.json(giteaPr(42, 'renamed-local-branch')) + : Response.json([giteaPr(7)]) + }) + vi.stubGlobal('fetch', fetchMock) + + await expect(getGiteaPullRequestForBranch('/repo', 'feature/gitea', 42)).resolves.toMatchObject( + { number: 42 } + ) + }) + + it('shows the open PR over a newer closed one from the same branch (#28324)', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async (url: string | URL) => + String(url).includes('/status') + ? Response.json({ state: 'success' }) + : Response.json([{ ...giteaPr(9), state: 'closed' }, giteaPr(3)]) + ) + ) + + await expect(getGiteaPullRequestForBranch('/repo', 'feature/gitea')).resolves.toMatchObject({ + number: 3, + state: 'open' + }) + }) + + it('ignores a same-named branch from another fork', async () => { + const forkPr = giteaPr(5) + forkPr.head.repo.full_name = 'alice/repo' + vi.stubGlobal( + 'fetch', + vi.fn(async () => Response.json([forkPr])) + ) + + await expect(getGiteaPullRequestForBranch('/repo', 'feature/gitea')).resolves.toBeNull() + }) + it('reports configured token auth without a global API base URL', async () => { await expect(getGiteaAuthStatus()).resolves.toEqual({ configured: true, diff --git a/src/main/gitea/client.ts b/src/main/gitea/client.ts index ba768bb81319..50ec8798923d 100644 --- a/src/main/gitea/client.ts +++ b/src/main/gitea/client.ts @@ -1,5 +1,6 @@ import { deriveGiteaCommitStatus, + isGiteaPullRequestFromHead, mapGiteaPullRequest, mapGiteaPullRequestState, type GiteaPullRequestInfo, @@ -7,7 +8,9 @@ import { type RawGiteaPullRequest } from './pull-request-mappers' import { shouldHideNonOpenReviewOnDefaultBranch } from '../source-control/repo-default-branch' -import { getGiteaRepoRef, type GiteaRepoRef } from './repository-ref' +import { pickReviewForHead } from '../source-control/branch-review-selection' +import { resolveBranchReviewTarget } from '../source-control/review-head' +import { getGiteaRepoRef, getGiteaRepoRefForRemote, type GiteaRepoRef } from './repository-ref' import { invalidateGiteaPullRequestScan, scanGiteaPullRequests } from './pull-request-scan-cache' import { getHostedReviewLocalGitOptions, @@ -158,15 +161,6 @@ async function normalizePullRequest( return mapGiteaPullRequest(raw, status) } -function matchesBranch(raw: RawGiteaPullRequest, branchName: string): boolean { - const ref = raw.head?.ref?.trim() - if (ref === branchName) { - return true - } - const label = raw.head?.label?.trim() - return label === branchName || label?.endsWith(`:${branchName}`) === true -} - export async function getGiteaAuthStatus(): Promise { const config = getAuthConfig() const tokenConfigured = config.token !== null @@ -250,68 +244,80 @@ export async function getGiteaPullRequestForBranch( return null } - const repo = await getGiteaRepoRef( - repoPath, - connectionId, - getHostedReviewLocalGitOptions(options) - ) + const localGitOptions = getHostedReviewLocalGitOptions(options) + const repo = await getGiteaRepoRef(repoPath, connectionId, localGitOptions) if (!repo) { return null } - if (branchName) { - const pullRequests = await scanGiteaPullRequests( - // Why: throwing scans keep a separate cache namespace so they never reuse a - // prior swallowing scan's cached-empty from a failed page fetch, which would - // otherwise let a real lookup failure masquerade as "no PR". - throwOnFailure ? `${giteaPullRequestScanKey(repo)}::strict` : giteaPullRequestScanKey(repo), - (page) => - requestJson( - repo, - `/repos/${encodedRepoPath(repo)}/pulls`, - { - searchParams: { - state: 'all', - sort: 'recentupdate', - page, - limit: PULL_REQUEST_PAGE_LIMIT - }, - timeoutMs: PULL_REQUEST_LIST_TIMEOUT_MS - }, - throwOnFailure - ), - PULL_REQUEST_PAGE_LIMIT, - MAX_PULL_REQUEST_PAGES + const { linkedNumber, head } = await resolveBranchReviewTarget( + repoPath, + branchName, + linkedPRNumber, + connectionId, + localGitOptions + ) + if (linkedNumber !== null) { + const raw = await requestJson( + repo, + `/repos/${encodedRepoPath(repo)}/pulls/${encodeURIComponent(String(linkedNumber))}`, + {}, + throwOnFailure ) - const raw = pullRequests.find((item) => matchesBranch(item, branchName)) if (raw) { - // Why (#9171): discard a non-open implicit branch match on the repo - // default branch and fall through to the linked-number fallback below. - const hideOnDefaultBranch = await shouldHideNonOpenReviewOnDefaultBranch({ - state: mapGiteaPullRequestState(raw), - reviewNumber: raw.number ?? null, - linkedReviewNumber: linkedPRNumber, - branchName, - repoPath, - connectionId, - localGitOptions: getHostedReviewLocalGitOptions(options) - }) - if (!hideOnDefaultBranch) { - return normalizePullRequest(repo, raw) - } + return normalizePullRequest(repo, raw) } } - if (typeof linkedPRNumber !== 'number') { + const headRepo = !head + ? null + : head.remote === 'origin' + ? repo + : await getGiteaRepoRefForRemote(repoPath, head.remote, connectionId, localGitOptions) + if (!head || !headRepo) { return null } - const raw = await requestJson( - repo, - `/repos/${encodedRepoPath(repo)}/pulls/${encodeURIComponent(String(linkedPRNumber))}`, - {}, - throwOnFailure + const pullRequests = await scanGiteaPullRequests( + // Why: throwing scans keep a separate cache namespace so they never reuse a + // prior swallowing scan's cached-empty from a failed page fetch, which would + // otherwise let a real lookup failure masquerade as "no PR". + throwOnFailure ? `${giteaPullRequestScanKey(repo)}::strict` : giteaPullRequestScanKey(repo), + (page) => + requestJson( + repo, + `/repos/${encodedRepoPath(repo)}/pulls`, + { + searchParams: { + state: 'all', + sort: 'recentupdate', + page, + limit: PULL_REQUEST_PAGE_LIMIT + }, + timeoutMs: PULL_REQUEST_LIST_TIMEOUT_MS + }, + throwOnFailure + ), + PULL_REQUEST_PAGE_LIMIT, + MAX_PULL_REQUEST_PAGES ) - return raw ? normalizePullRequest(repo, raw) : null + const raw = pickReviewForHead( + pullRequests.filter((item) => isGiteaPullRequestFromHead(item, headRepo, head.branch)), + (item) => ['open', 'draft'].includes(mapGiteaPullRequestState(item)) + ) + if (!raw) { + return null + } + // Why (#9171): discard a non-open implicit branch match on the repo default branch. + const hideOnDefaultBranch = await shouldHideNonOpenReviewOnDefaultBranch({ + state: mapGiteaPullRequestState(raw), + reviewNumber: raw.number ?? null, + linkedReviewNumber: linkedPRNumber, + branchName, + repoPath, + connectionId, + localGitOptions + }) + return hideOnDefaultBranch ? null : normalizePullRequest(repo, raw) } /** diff --git a/src/main/gitea/pull-request-mappers.ts b/src/main/gitea/pull-request-mappers.ts index 9e877f14bb84..dfae7badc8a3 100644 --- a/src/main/gitea/pull-request-mappers.ts +++ b/src/main/gitea/pull-request-mappers.ts @@ -13,6 +13,7 @@ export type RawGiteaPullRequest = { ref?: string | null label?: string | null sha?: string | null + repo?: { full_name?: string | null } | null } | null } @@ -104,6 +105,22 @@ export function mapGiteaPullRequestState( return 'open' } +/** + * Gitea has no server-side head filter, so a branch lookup matches the scan on head repository and + * branch (a Forgejo AGit PR carries its topic as the label). + */ +export function isGiteaPullRequestFromHead( + raw: RawGiteaPullRequest, + headRepo: { owner: string; repo: string }, + branch: string +): boolean { + return ( + raw.head?.repo?.full_name?.toLowerCase() === + `${headRepo.owner}/${headRepo.repo}`.toLowerCase() && + (raw.head.ref?.trim() === branch || raw.head.label?.trim() === branch) + ) +} + export function mapGiteaMergeable(value: boolean | null | undefined): PRMergeableState { if (value === true) { return 'MERGEABLE' diff --git a/src/main/github/client-pr-branch-discovery.test.ts b/src/main/github/client-pr-branch-discovery.test.ts index ca41a3361f38..fa00f41d0f61 100644 --- a/src/main/github/client-pr-branch-discovery.test.ts +++ b/src/main/github/client-pr-branch-discovery.test.ts @@ -62,7 +62,7 @@ describe('getPRForBranch', () => { expect(getOwnerRepoMock).toHaveBeenCalledWith('/repo-root', undefined) expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], + ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=10'], { cwd: '/repo-root' } ) expect(pr?.number).toBe(42) @@ -99,7 +99,7 @@ describe('getPRForBranch', () => { const pr = await getPRForBranch('/repo-root', 'feature/test') expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - ['api', 'repos/stablyai/orca/pulls?head=fork%3Afeature%2Ftest&state=all&per_page=1'], + ['api', 'repos/stablyai/orca/pulls?head=fork%3Afeature%2Ftest&state=all&per_page=10'], { cwd: '/repo-root' } ) expect(pr).toMatchObject({ @@ -131,7 +131,7 @@ describe('getPRForBranch', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 1, - ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], + ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=10'], { cwd: '/repo-root' } ) expect(pr).toMatchObject({ diff --git a/src/main/github/client-pr-conflict-summary.test.ts b/src/main/github/client-pr-conflict-summary.test.ts index a73bc1e58ffd..90ef5c56f68b 100644 --- a/src/main/github/client-pr-conflict-summary.test.ts +++ b/src/main/github/client-pr-conflict-summary.test.ts @@ -182,23 +182,25 @@ describe('getPRForBranch', () => { it('omits conflict summaries for SSH-backed repos', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + // Linked, so no review-head git read is needed for this SSH repo. ghExecFileAsyncMock.mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 42, - title: 'Fix PR discovery', - state: 'open', - html_url: 'https://github.com/acme/widgets/pull/42', - updated_at: '2026-03-28T00:00:00Z', - draft: false, - mergeable_state: 'dirty', - base: { ref: 'main', sha: 'base-oid' }, - head: { ref: 'feature/test', sha: 'head-oid' } - } - ]) + stdout: JSON.stringify({ + number: 42, + title: 'Fix PR discovery', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/42', + statusCheckRollup: [], + updatedAt: '2026-03-28T00:00:00Z', + isDraft: false, + mergeable: 'CONFLICTING', + baseRefName: 'main', + headRefName: 'feature/test', + baseRefOid: 'base-oid', + headRefOid: 'head-oid' + }) }) - const pr = await getPRForBranch('/remote/repo-root', 'feature/test', undefined, 'ssh-1') + const pr = await getPRForBranch('/remote/repo-root', 'feature/test', 42, 'ssh-1') expect(pr?.mergeable).toBe('CONFLICTING') expect(pr?.conflictSummary).toBeUndefined() diff --git a/src/main/github/client-pr-fallback-number.test.ts b/src/main/github/client-pr-fallback-number.test.ts index c3e6282347da..cebdcb2f399c 100644 --- a/src/main/github/client-pr-fallback-number.test.ts +++ b/src/main/github/client-pr-fallback-number.test.ts @@ -29,13 +29,7 @@ import { getPRForBranchOutcome } from './client/lookup/pr-for-branch-outcome' import { getPRForBranch } from './client/lookup/get-pr-for-branch' import { resetPRForBranchMocks } from './client-test-harness' -const { - ghExecFileAsyncMock, - getOwnerRepoMock, - getOwnerRepoForRemoteMock, - resolvePRRepositoryCandidatesMock, - gitExecFileAsyncMock -} = clientMocks +const { ghExecFileAsyncMock, getOwnerRepoMock, resolvePRRepositoryCandidatesMock } = clientMocks describe('getPRForBranch', () => { beforeEach(() => { @@ -82,7 +76,7 @@ describe('getPRForBranch', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 1, - ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], + ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=10'], { cwd: '/repo-root' } ) expect(pr).toMatchObject({ number: 43, title: 'Hydrated branch PR wins' }) @@ -103,7 +97,7 @@ describe('getPRForBranch', () => { isDraft: false, mergeable: 'MERGEABLE', baseRefName: 'main', - headRefName: 'contributor/original', + headRefName: 'feature/test', baseRefOid: 'base-oid', headRefOid: 'fallback-head-oid' }) @@ -113,7 +107,7 @@ describe('getPRForBranch', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 1, - ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], + ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=10'], { cwd: '/repo-root' } ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( @@ -132,59 +126,46 @@ describe('getPRForBranch', () => { expect(pr).toMatchObject({ number: 42, title: 'Fallback PR lookup' }) }) - it('reports upstream error when fallback branch discovery fails transiently then retry misses', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ - candidates: [{ owner: 'stablyai', repo: 'orca' }], - headRepo: null - }) + it('drops a remembered PR number whose head is another branch and reports it', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock - .mockRejectedValueOnce(new Error('HTTP 429: API rate limit exceeded')) .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 42, + title: 'Develop into release', + state: 'MERGED', + url: 'https://github.com/acme/widgets/pull/42', + statusCheckRollup: [], + updatedAt: '2026-03-28T00:00:00Z', + isDraft: false, + mergeable: 'MERGEABLE', + baseRefName: 'release', + headRefName: 'develop', + baseRefOid: 'base-oid', + headRefOid: 'develop-head-oid' + }) + }) - const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test') + const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test', null, null, 42) expect(outcome).toMatchObject({ - kind: 'upstream-error', - errorType: 'rate_limited' + kind: 'no-pr', + rejectedPRUrls: ['https://github.com/acme/widgets/pull/42'] }) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 1, - [ - 'pr', - 'list', - '--repo', - 'stablyai/orca', - '--head', - 'feature/test', - '--state', - 'all', - '--limit', - '1', - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' - ], - { cwd: '/repo-root' } - ) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 2, - ['api', 'repos/stablyai/orca/pulls?head=stablyai%3Afeature%2Ftest&state=all&per_page=1'], - { cwd: '/repo-root' } - ) }) it('propagates a Retry-After cooldown into the rate-limited retry schedule', async () => { resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ candidates: [{ owner: 'stablyai', repo: 'orca' }], - headRepo: null + headRepo: { owner: 'stablyai', repo: 'orca' } }) // gh puts the diagnostic on `.stderr`; a secondary limit carries Retry-After. - ghExecFileAsyncMock - .mockRejectedValueOnce( - Object.assign(new Error('gh exited with 1.'), { - stderr: 'HTTP 403: You have exceeded a secondary rate limit\nRetry-After: 120' - }) - ) - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) + ghExecFileAsyncMock.mockRejectedValueOnce( + Object.assign(new Error('gh exited with 1.'), { + stderr: 'HTTP 403: You have exceeded a secondary rate limit\nRetry-After: 120' + }) + ) const before = Date.now() const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test') @@ -200,192 +181,6 @@ describe('getPRForBranch', () => { expect(outcome.retryDisabledUntil ?? 0).toBeLessThanOrEqual(Date.now() + 121_000) }) - it('reports no PR when fallback branch discovery cleanly misses', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ - candidates: [{ owner: 'stablyai', repo: 'orca' }], - headRepo: null - }) - ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - - const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test') - - expect(outcome.kind).toBe('no-pr') - expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) - }) - - it('returns found when fallback branch discovery retry finds the PR', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ - candidates: [{ owner: 'stablyai', repo: 'orca' }], - headRepo: null - }) - ghExecFileAsyncMock - .mockRejectedValueOnce(new Error('HTTP 429: API rate limit exceeded')) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 42, - title: 'Retry branch PR', - state: 'open', - html_url: 'https://github.com/stablyai/orca/pull/42', - updated_at: '2026-03-28T00:00:00Z', - draft: false, - mergeable: true, - base: { ref: 'main', sha: 'base-oid' }, - head: { ref: 'feature/test', sha: 'retry-head-oid' } - } - ]) - }) - .mockResolvedValueOnce({ - stdout: JSON.stringify({ - number: 42, - title: 'Hydrated retry branch PR', - state: 'OPEN', - url: 'https://github.com/stablyai/orca/pull/42', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'MERGEABLE', - baseRefName: 'main', - headRefName: 'feature/test', - baseRefOid: 'base-oid', - headRefOid: 'retry-head-oid' - }) - }) - - const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test') - - expect(outcome).toMatchObject({ - kind: 'found', - pr: { - number: 42, - title: 'Hydrated retry branch PR', - prRepo: { owner: 'stablyai', repo: 'orca' } - } - }) - }) - - it('lets fallback PR number recovery win after fallback branch queries throw', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ - candidates: [{ owner: 'stablyai', repo: 'orca' }], - headRepo: null - }) - ghExecFileAsyncMock - .mockRejectedValueOnce(new Error('HTTP 429: API rate limit exceeded')) - .mockRejectedValueOnce(new Error('HTTP 502: Bad Gateway')) - .mockResolvedValueOnce({ - stdout: JSON.stringify({ - number: 42, - title: 'Fallback number recovered PR', - state: 'OPEN', - url: 'https://github.com/stablyai/orca/pull/42', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'MERGEABLE', - baseRefName: 'main', - headRefName: 'feature/test', - baseRefOid: 'base-oid', - headRefOid: 'fallback-head-oid' - }) - }) - - const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test', null, null, 42) - - expect(outcome).toMatchObject({ - kind: 'found', - pr: { - number: 42, - title: 'Fallback number recovered PR' - } - }) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 3, - [ - 'pr', - 'view', - '42', - '--repo', - 'stablyai/orca', - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' - ], - { cwd: '/repo-root' } - ) - }) - - it('reports upstream error when fallback branch discovery has a network failure', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ - candidates: [{ owner: 'stablyai', repo: 'orca' }], - headRepo: null - }) - ghExecFileAsyncMock - .mockRejectedValueOnce(new Error('could not resolve host: api.github.com')) - .mockRejectedValueOnce(new Error('could not resolve host: api.github.com')) - - const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test') - - expect(outcome).toMatchObject({ - kind: 'upstream-error', - errorType: 'network' - }) - }) - - it('reports a GitHub server error when fallback branch discovery receives 5xx responses', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ - candidates: [{ owner: 'stablyai', repo: 'orca' }], - headRepo: null - }) - ghExecFileAsyncMock - .mockRejectedValueOnce(new Error('HTTP 503: Service Unavailable')) - .mockRejectedValueOnce(new Error('HTTP 502: Bad Gateway')) - - const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test') - - expect(outcome).toMatchObject({ - kind: 'upstream-error', - errorType: 'server_error' - }) - }) - - it('keeps a pending fallback branch error when a later candidate cleanly misses', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ - candidates: [ - { owner: 'stablyai', repo: 'orca' }, - { owner: 'fork', repo: 'orca' } - ], - headRepo: null - }) - ghExecFileAsyncMock - .mockRejectedValueOnce(new Error('HTTP 429: API rate limit exceeded')) - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - - const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test') - - expect(outcome).toMatchObject({ - kind: 'upstream-error', - errorType: 'rate_limited' - }) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 3, - [ - 'pr', - 'list', - '--repo', - 'fork/orca', - '--head', - 'feature/test', - '--state', - 'all', - '--limit', - '1', - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' - ], - { cwd: '/repo-root' } - ) - }) - it('treats a merged branch lookup as a miss before using a fallback PR number', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock @@ -432,7 +227,7 @@ describe('getPRForBranch', () => { isDraft: false, mergeable: 'MERGEABLE', baseRefName: 'main', - headRefName: 'contributor/original', + headRefName: 'feature/test', baseRefOid: 'base-oid', headRefOid: 'fallback-head-oid' }) @@ -513,76 +308,6 @@ describe('getPRForBranch', () => { expect(pr).toMatchObject({ number: 5511, state: 'merged', title: 'Merged current PR' }) }) - it('does not carry a merged upstream branch head repo into a fallback PR number', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ - candidates: [{ owner: 'stablyai', repo: 'orca' }], - headRepo: { owner: 'origin-owner', repo: 'orca' } - }) - getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'fork-owner', repo: 'orca' }) - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'local-created-from-pr\0fork/contributor/original\n', - stderr: '' - }) - ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 5511, - title: 'Merged upstream branch PR', - state: 'closed', - merged_at: '2026-06-16T17:15:33Z', - html_url: 'https://github.com/stablyai/orca/pull/5511', - updated_at: '2026-06-16T17:15:33Z', - draft: false, - mergeable_state: 'clean', - head: { ref: 'contributor/original', sha: 'merged-head-oid' }, - base: { ref: 'main', sha: 'base-oid' } - } - ]) - }) - .mockResolvedValueOnce({ - stdout: JSON.stringify({ - number: 5511, - title: 'Merged upstream branch PR', - state: 'MERGED', - url: 'https://github.com/stablyai/orca/pull/5511', - statusCheckRollup: [], - updatedAt: '2026-06-16T17:15:33Z', - isDraft: false, - mergeable: 'MERGEABLE', - baseRefName: 'main', - headRefName: 'contributor/original', - baseRefOid: 'base-oid', - headRefOid: 'merged-head-oid' - }) - }) - .mockResolvedValueOnce({ - stdout: JSON.stringify({ - number: 42, - title: 'Open fallback PR', - state: 'OPEN', - url: 'https://github.com/stablyai/orca/pull/42', - statusCheckRollup: [], - updatedAt: '2026-06-17T00:00:00Z', - isDraft: false, - mergeable: 'MERGEABLE', - baseRefName: 'main', - headRefName: 'fresh/fallback', - baseRefOid: 'base-oid', - headRefOid: 'fallback-head-oid' - }) - }) - - const pr = await getPRForBranch('/repo-root', 'local-created-from-pr', null, null, 42) - - expect(pr).toMatchObject({ - number: 42, - title: 'Open fallback PR', - headRepo: { owner: 'origin-owner', repo: 'orca' } - }) - }) - it('ignores merged PRs discovered only from a fallback PR number', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock diff --git a/src/main/github/client-pr-review-head.test.ts b/src/main/github/client-pr-review-head.test.ts new file mode 100644 index 000000000000..7021352a1c56 --- /dev/null +++ b/src/main/github/client-pr-review-head.test.ts @@ -0,0 +1,362 @@ +// A branch's PR is the one whose head is where the branch is pushed (#28324, #27811, #26948). +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 { getPRForBranch } from './client/lookup/get-pr-for-branch' +import { getPRForBranchOutcome } from './client/lookup/pr-for-branch-outcome' +import { gitConfigListOutput, resetPRForBranchMocks } from './client-test-harness' +import type { RestPullRequest } from './client/lookup/pull-request-lookup-data' +import type { OwnerRepo } from './gh-utils' + +const { + ghExecFileAsyncMock, + getOwnerRepoMock, + getOwnerRepoForRemoteMock, + resolvePRRepositoryCandidatesMock, + gitConfigListMock, + gitExecFileAsyncMock +} = clientMocks + +const ACME = { owner: 'acme', repo: 'widgets' } +const UPSTREAM = { owner: 'stablyai', repo: 'widgets' } +const CONTRIBUTOR = { owner: 'contributor', repo: 'widgets' } + +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://${prRepo.host ?? '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: headRefName, + sha: 'old-develop-oid', + repo: { name: headRepo.repo, owner: { login: headRepo.owner } } + } + } +} + +function primeGh( + listsByHead: Record, + exactPR: RestPullRequest | RestPullRequest[] = Object.values(listsByHead).flat()[0] ?? restPR() +): void { + 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[], 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 exactRestMatch = args[1]?.match(/^repos\/([^/]+\/[^/]+)\/pulls\/(\d+)$/) + if (args[0] === 'api' && exactRestMatch) { + const pr = prsByRepositoryAndNumber.get(`${host}/${exactRestMatch[1]}#${exactRestMatch[2]}`) + if (pr) { + return { stdout: JSON.stringify(pr) } + } + throw new Error('HTTP 404 Not Found') + } + const repoIndex = args.indexOf('--repo') + const exact = prsByRepositoryAndNumber.get(`${host}/${args[repoIndex + 1]}#${args[2]}`) + if (args[0] === 'pr' && args[1] === 'view') { + if (!exact) { + throw new Error('HTTP 404 Not Found') + } + 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(' ')}`) + } + ) +} + +/** A fork clone: `origin` is the fork, `upstream` is the repo PRs target. */ +function primeForkWithUpstream(): void { + resolvePRRepositoryCandidatesMock.mockResolvedValue({ + candidates: [UPSTREAM, ACME], + headRepo: ACME + }) + getOwnerRepoForRemoteMock.mockImplementation(async (_repoPath: string, remoteName: string) => + remoteName === 'upstream' ? UPSTREAM : remoteName === 'origin' ? ACME : null + ) +} + +const BRANCH = 'feature/my-change' + +/** The branch's push config, as `git config --list -z` reports it, plus HEAD and origin's default branch. */ +function primeBranchConfig(records: [string, string][] = []): void { + gitConfigListMock.mockResolvedValue(gitConfigListOutput(records)) + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'for-each-ref' && args.includes('--format=%(refname)%00%(symref)')) { + return { stdout: 'refs/remotes/origin/HEAD\0refs/remotes/origin/main\n', stderr: '' } + } + if (args[0] === 'rev-parse' && args[1] === 'HEAD') { + return { stdout: 'feature-head-oid\n', stderr: '' } + } + throw new Error(`unexpected git call: ${args.join(' ')}`) + }) +} + +function tracking(remote: string, branch: string): [string, string][] { + return [ + [`branch.${BRANCH}.remote`, remote], + [`branch.${BRANCH}.merge`, `refs/heads/${branch}`] + ] +} + +describe('a branch PR is matched by where the branch is pushed', () => { + beforeEach(() => { + resetPRForBranchMocks(clientMocks) + getOwnerRepoMock.mockResolvedValue(ACME) + }) + + it('shows the open PR over a newer closed one on the same head (#28324)', async () => { + primeBranchConfig(tracking('origin', BRANCH)) + primeGh( + { + [`acme:${BRANCH}`]: [ + { ...restPR(BRANCH), number: 8, html_url: 'https://github.com/acme/widgets/pull/8' }, + { ...restPR(BRANCH), state: 'open' } + ] + }, + [{ ...restPR(BRANCH), state: 'open' }] + ) + + expect(await getPRForBranch('/repo-root', BRANCH)).toMatchObject({ number: 7, state: 'open' }) + }) + + it('asks for open PRs when ten newer closed PRs fill the first page', async () => { + primeBranchConfig(tracking('origin', BRANCH)) + const closed = Array.from({ length: 10 }, (_, i) => ({ + ...restPR(BRANCH), + number: 20 - i, + html_url: `https://github.com/acme/widgets/pull/${20 - i}` + })) + const open = { ...restPR(BRANCH), state: 'open' } + primeGh({ [`acme:${BRANCH}`]: [...closed, open] }, [open]) + const listCall = ghExecFileAsyncMock.getMockImplementation() + ghExecFileAsyncMock.mockImplementation(async (args: string[], options) => { + if (args[0] === 'api' && args[1]?.includes('state=all')) { + return { stdout: JSON.stringify(closed) } + } + return listCall?.(args, options) + }) + + expect(await getPRForBranch('/repo-root', BRANCH)).toMatchObject({ number: 7, state: 'open' }) + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + [ + 'api', + `repos/acme/widgets/pulls?head=acme%3A${encodeURIComponent(BRANCH)}&state=open&per_page=10` + ], + expect.anything() + ) + }) + + it("does not attach another owner's PR from a same-named branch (#27811)", async () => { + primeBranchConfig(tracking('origin', BRANCH)) + primeGh({ [`contributor:${BRANCH}`]: [{ ...restPR(BRANCH, CONTRIBUTOR), state: 'open' }] }) + + expect(await getPRForBranch('/repo-root', BRANCH)).toBeNull() + }) + + it('does not attach the PR of the base branch the branch tracks (#26948)', async () => { + primeBranchConfig(tracking('origin', 'develop')) + primeGh({ 'acme:develop': [restPR('develop')] }) + + expect(await getPRForBranch('/repo-root', BRANCH)).toBeNull() + }) + + it('does not attach the PR of an upstream base branch in a fork checkout', async () => { + primeForkWithUpstream() + primeBranchConfig(tracking('upstream', 'develop')) + primeGh({ 'stablyai:develop': [{ ...restPR('develop', UPSTREAM, UPSTREAM), state: 'open' }] }) + + expect(await getPRForBranch('/repo-root', BRANCH)).toBeNull() + }) + + it('drops a remembered integration PR whose head commit matches but whose head is not this branch', async () => { + primeBranchConfig(tracking('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', BRANCH, null, null, 7, { + currentHeadOid: 'feature-head-oid', + acceptMergedFallbackPR: true + }) + ).toMatchObject({ kind: 'no-pr', rejectedPRUrls: ['https://github.com/acme/widgets/pull/7'] }) + }) + + it('keeps a remembered PR whose head is this branch', async () => { + primeBranchConfig() + primeGh({}, { ...restPR(BRANCH), state: 'open' }) + + expect(await getPRForBranch('/repo-root', BRANCH, null, null, 7)).toMatchObject({ + number: 7, + headRefName: BRANCH + }) + }) + + it('keeps a remembered PR from a deleted fork when the branch name matches', async () => { + primeBranchConfig() + primeGh({}, { ...restPR(BRANCH), state: 'open', head: { ref: BRANCH, sha: 'oid', repo: null } }) + + expect(await getPRForBranch('/repo-root', BRANCH, null, null, 7)).toMatchObject({ number: 7 }) + }) + + it("follows a contributor fork's branch checked out under a local name", async () => { + const fork = { owner: 'contributor', repo: 'renamed-widgets' } + getOwnerRepoForRemoteMock.mockImplementation(async (_path: string, remote: string) => + remote === 'contributor' ? fork : ACME + ) + primeBranchConfig(tracking('contributor', 'develop')) + primeGh({ 'contributor:develop': [{ ...restPR('develop', fork), state: 'open' }] }) + + expect(await getPRForBranch('/repo-root', BRANCH)).toMatchObject({ + number: 7, + headRepo: fork, + prRepo: ACME + }) + }) + + it('finds a fork PR into upstream when the branch is pushed to the fork', async () => { + primeForkWithUpstream() + primeBranchConfig(tracking('origin', BRANCH)) + primeGh({ [`acme:${BRANCH}`]: [{ ...restPR(BRANCH, ACME, UPSTREAM), state: 'open' }] }) + + expect(await getPRForBranch('/repo-root', BRANCH)).toMatchObject({ + number: 7, + headRepo: ACME, + prRepo: UPSTREAM + }) + }) + + it('follows the push remote when it differs from the tracked one (triangular)', async () => { + getOwnerRepoForRemoteMock.mockImplementation(async (_path: string, remote: string) => + remote === 'fork' ? CONTRIBUTOR : ACME + ) + primeBranchConfig([...tracking('origin', 'main'), [`branch.${BRANCH}.pushremote`, 'fork']]) + primeGh({ [`contributor:${BRANCH}`]: [{ ...restPR(BRANCH, CONTRIBUTOR), state: 'open' }] }) + + expect(await getPRForBranch('/repo-root', BRANCH)).toMatchObject({ + number: 7, + headRepo: CONTRIBUTOR + }) + }) + + it('treats a branch checked out from refs/pull/N/head as linked to PR N', async () => { + primeBranchConfig([ + [`branch.${BRANCH}.remote`, 'origin'], + [`branch.${BRANCH}.merge`, 'refs/pull/7/head'] + ]) + primeGh({}, { ...restPR('contributor-branch', CONTRIBUTOR), state: 'open' }) + + expect(await getPRForBranch('/repo-root', BRANCH)).toMatchObject({ number: 7 }) + }) + + it('keeps an explicit link without reading git config', async () => { + primeBranchConfig(tracking('origin', 'develop')) + primeGh({}) + + expect(await getPRForBranch('/repo-root', BRANCH, 7)).toMatchObject({ number: 7 }) + expect(gitConfigListMock).not.toHaveBeenCalled() + }) + + it('keeps a remembered PR while detached', async () => { + primeGh({}) + + expect(await getPRForBranch('/repo-root', '', null, null, 7)).toMatchObject({ number: 7 }) + expect(gitConfigListMock).not.toHaveBeenCalled() + }) + + 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 + }) + primeBranchConfig() + primeGh({ [`acme:${BRANCH}`]: [{ ...restPR(BRANCH, other, other), state: 'open' }] }) + + expect(await getPRForBranch('/repo-root', BRANCH)).toBeNull() + }) + + it('does not query another GitHub server for a public GitHub head', async () => { + const enterprise = { ...ACME, host: 'ghe.example' } + resolvePRRepositoryCandidatesMock.mockResolvedValue({ + candidates: [enterprise, ACME], + headRepo: ACME + }) + primeBranchConfig() + primeGh({ [`acme:${BRANCH}`]: [{ ...restPR(BRANCH, enterprise, enterprise), state: 'open' }] }) + + expect(await getPRForBranch('/repo-root', BRANCH)).toBeNull() + expect( + ghExecFileAsyncMock.mock.calls.some(([, options]) => options?.host === 'ghe.example') + ).toBe(false) + }) +}) diff --git a/src/main/github/client-ssh-provider-execution-boundary.test.ts b/src/main/github/client-ssh-provider-execution-boundary.test.ts index 875fd10406da..eef723258227 100644 --- a/src/main/github/client-ssh-provider-execution-boundary.test.ts +++ b/src/main/github/client-ssh-provider-execution-boundary.test.ts @@ -27,11 +27,13 @@ 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 { gitConfigListOutput, resetPRForBranchMocks } from './client-test-harness' const { ghExecFileAsyncMock, getOwnerRepoMock, + getOwnerRepoForRemoteMock, + gitConfigListMock, gitExecFileAsyncMock, resolvePRRepositoryCandidatesMock } = clientMocks @@ -75,42 +77,6 @@ function mockMergedBranchPRLookup(): void { }) } -function mockUpstreamOnlyPRLookup(): void { - ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 78, - title: 'Upstream branch PR', - state: 'open', - html_url: 'https://github.com/acme/widgets/pull/78', - updated_at: '2026-03-28T00:00:00Z', - draft: false, - mergeable: true, - base: { ref: 'main', sha: 'base-oid' }, - head: { ref: 'contributor/original', sha: 'upstream-head-oid' } - } - ]) - }) - .mockResolvedValueOnce({ - stdout: JSON.stringify({ - number: 78, - title: 'Hydrated upstream branch PR', - state: 'OPEN', - url: 'https://github.com/acme/widgets/pull/78', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'MERGEABLE', - baseRefName: 'main', - headRefName: 'contributor/original', - baseRefOid: 'base-oid', - headRefOid: 'upstream-head-oid' - }) - }) -} - // The strict execution boundary: for an SSH-hosted repoPath, an unregistered // provider means "we could not ask" — it must never degrade into a client-side // git run, which on a same-named local path answers for the wrong repository. @@ -167,18 +133,13 @@ describe('getPRForBranch SSH execution boundary', () => { expect(pr).toMatchObject({ number: 5875, state: 'merged', headSha: MERGED_HEAD_OID }) }) - it('does not read tracked upstreams locally when the SSH provider is unregistered', async () => { - mockUpstreamOnlyPRLookup() - gitExecFileAsyncMock.mockResolvedValue({ - stdout: `${MERGED_BRANCH}\0origin/contributor/original\n`, - stderr: '' - }) - + it('does not read git config locally when the SSH provider is unregistered', async () => { const outcome = await getPRForBranchOutcome('/remote/repo', MERGED_BRANCH, null, 'ssh-1') + expect(gitConfigListMock).not.toHaveBeenCalled() expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() expect(outcome).toMatchObject({ kind: 'upstream-error', errorType: 'unknown' }) - expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) }) it('reports an upstream error when the SSH HEAD probe loses its provider mid-flight', async () => { @@ -190,31 +151,29 @@ describe('getPRForBranch SSH execution boundary', () => { expect(outcome).toMatchObject({ kind: 'upstream-error', errorType: 'unknown' }) }) - it('reports an upstream error when the SSH upstream probe loses its provider mid-flight', async () => { - mockUpstreamOnlyPRLookup() - - const outcome = await getPRForBranchOutcome('/remote/repo', MERGED_BRANCH, null, 'ssh-1') - - expect(gitExecFileAsyncMock).not.toHaveBeenCalled() - expect(outcome).toMatchObject({ kind: 'upstream-error', errorType: 'unknown' }) - }) - - it('reads tracked upstreams through the local runtime for a WSL repository', async () => { - const forEachRefArgs = ['for-each-ref', '--format=%(refname)%00%(upstream)', 'refs/heads'] - mockUpstreamOnlyPRLookup() - gitExecFileAsyncMock.mockResolvedValue({ - stdout: `${MERGED_BRANCH}\0origin/contributor/original\n`, - stderr: '' - }) + it('reads the review head from git config through the local runtime for a WSL repository', async () => { + gitConfigListMock.mockResolvedValue( + gitConfigListOutput([ + [`branch.${MERGED_BRANCH}.remote`, 'fork'], + [`branch.${MERGED_BRANCH}.merge`, `refs/heads/${MERGED_BRANCH}`] + ]) + ) + getOwnerRepoForRemoteMock.mockImplementation(async (_repoPath: string, remote: string) => + remote === 'fork' ? { owner: 'contributor', repo: 'widgets' } : null + ) + ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - const pr = await getPRForBranch('/repo-root', MERGED_BRANCH, null, null, null, { + await getPRForBranch('/repo-root', MERGED_BRANCH, null, null, null, { localGitExecOptions: { wslDistro: 'Ubuntu' } }) - expect(gitExecFileAsyncMock).toHaveBeenCalledWith(forEachRefArgs, { + expect(gitConfigListMock).toHaveBeenCalledWith(['config', '--list', '-z'], { cwd: '/repo-root', wslDistro: 'Ubuntu' }) - expect(pr).toMatchObject({ number: 78, title: 'Hydrated upstream branch PR' }) + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + ['api', `repos/acme/widgets/pulls?head=contributor%3A${MERGED_BRANCH}&state=all&per_page=10`], + expect.anything() + ) }) }) diff --git a/src/main/github/client-test-harness.ts b/src/main/github/client-test-harness.ts index ae8e3c8a6929..3c8a721de384 100644 --- a/src/main/github/client-test-harness.ts +++ b/src/main/github/client-test-harness.ts @@ -2,7 +2,7 @@ import type { GitHubClientMocks } from './client-test-mocks' import { _resetOwnerRepoCache } from './gh-utils' import { _resetMergeQueueCacheForTests } from './client/detect/repository-merge-metadata-cache' import { _resetPRStackSummaryCacheForTests } from './client/lookup/pr-stack-summary-cache' -import { __resetTrackedUpstreamBranchCacheForTests } from './client/lookup/tracked-upstream-cache' +import { __resetRepoGitConfigSnapshotCacheForTests } from '../source-control/repo-git-config-snapshot' import { __resetPRConflictSummaryCachesForTests } from './conflict-summary' import { resetMergedPRCommitMembershipCacheForTest } from './merged-pr-commit-membership' import { __resetRepoDefaultBranchCacheForTests } from '../source-control/repo-default-branch' @@ -34,6 +34,8 @@ export function resetPRForBranchMocks(mocks: GitHubClientMocks): void { }) mocks.getRemoteUrlForRepoMock.mockReset() mocks.gitExecFileAsyncMock.mockReset() + mocks.gitConfigListMock.mockReset() + mocks.gitConfigListMock.mockRejectedValue(new Error('git config unavailable')) mocks.getRateLimitMock.mockReset() mocks.getRateLimitMock.mockResolvedValue({ resources: {} }) mocks.rateLimitGuardMock.mockReset() @@ -50,7 +52,7 @@ export function resetPRForBranchMocks(mocks: GitHubClientMocks): void { _resetMergeQueueCacheForTests() _resetPRStackSummaryCacheForTests() _resetGitHubPRStackCacheForTests() - __resetTrackedUpstreamBranchCacheForTests() + __resetRepoGitConfigSnapshotCacheForTests() __resetPRConflictSummaryCachesForTests() resetMergedPRCommitMembershipCacheForTest() // Why: the #9171 guard caches default-branch resolutions per repoPath; @@ -88,3 +90,11 @@ export function resetGraphQLRateLimitGuardMocks(mocks: GitHubClientMocks): void _resetPRStackSummaryCacheForTests() __resetPRConflictSummaryCachesForTests() } + +/** `git config --list -z` output for the review-head config read (`gitConfigListMock`). */ +export function gitConfigListOutput(records: [string, string][]): { + stdout: string + stderr: string +} { + return { stdout: records.map(([key, value]) => `${key}\n${value}`).join('\0'), stderr: '' } +} diff --git a/src/main/github/client-test-mocks.ts b/src/main/github/client-test-mocks.ts index 11762e87b7ce..67f40455a18e 100644 --- a/src/main/github/client-test-mocks.ts +++ b/src/main/github/client-test-mocks.ts @@ -18,6 +18,8 @@ export type GitHubClientMocks = { resolvePRRepositoryCandidatesMock: Mock getRemoteUrlForRepoMock: Mock gitExecFileAsyncMock: Mock + /** Answers `git config --list -z` (the review-head config read) apart from other git calls. */ + gitConfigListMock: Mock getRateLimitMock: Mock rateLimitGuardMock: Mock<(bucket?: string) => RateLimitGuardResult> noteRateLimitSpendMock: Mock @@ -41,6 +43,7 @@ export function createGitHubClientMocks(): GitHubClientMocks { resolvePRRepositoryCandidatesMock: vi.fn(), getRemoteUrlForRepoMock: vi.fn(), gitExecFileAsyncMock: vi.fn(), + gitConfigListMock: vi.fn(), getRateLimitMock: vi.fn(), rateLimitGuardMock: vi.fn<(bucket?: string) => RateLimitGuardResult>(() => ({ blocked: false @@ -124,7 +127,11 @@ export type GitRunnerModuleMock = { gitExecFileAsync: Mock; ghExecFileAsync: Moc export function gitRunnerModuleMock(mocks: GitHubClientMocks): GitRunnerModuleMock { return { - gitExecFileAsync: mocks.gitExecFileAsyncMock, + gitExecFileAsync: vi.fn((args: string[], options?: unknown) => + args[0] === 'config' && args[1] === '--list' + ? mocks.gitConfigListMock(args, options) + : mocks.gitExecFileAsyncMock(args, options) + ), ghExecFileAsync: mocks.ghExecFileAsyncMock } } diff --git a/src/main/github/client-tracked-upstream-default-branch.test.ts b/src/main/github/client-tracked-upstream-default-branch.test.ts deleted file mode 100644 index 3cf563fb61d5..000000000000 --- a/src/main/github/client-tracked-upstream-default-branch.test.ts +++ /dev/null @@ -1,515 +0,0 @@ -// #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' - -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 { 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, - getOwnerRepoMock, - getOwnerRepoForRemoteMock, - resolvePRRepositoryCandidatesMock, - gitExecFileAsyncMock -} = 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( - trackedUpstream: string, - remoteHeads: Record = { origin: 'develop' } -): void { - gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { - if (args[0] === 'for-each-ref' && args.includes('--format=%(refname)%00%(upstream)')) { - return { stdout: `refs/heads/feature/my-change\0${trackedUpstream}\n`, stderr: '' } - } - if (args[0] === 'for-each-ref' && args.includes('--format=%(refname)%00%(symref)')) { - const stdout = Object.entries(remoteHeads) - .filter(([remote]) => args.includes(`refs/remotes/${remote}/HEA[D]`)) - .map( - ([remote, branch]) => `refs/remotes/${remote}/HEAD\0refs/remotes/${remote}/${branch}\n` - ) - .join('') - return { stdout, stderr: '' } - } - if (args[0] === 'rev-parse' && args[1] === 'HEAD') { - return { stdout: 'feature-head-oid\n', stderr: '' } - } - throw new Error(`unexpected git call: ${args.join(' ')}`) - }) -} - -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://${prRepo.host ?? '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: headRefName, - sha: 'old-develop-oid', - repo: { name: headRepo.repo, owner: { login: headRepo.owner } } - } - } -} - -function primeGh( - listsByHead: Record, - exactPR: RestPullRequest | RestPullRequest[] = Object.values(listsByHead).flat()[0] ?? restPR() -): void { - 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[], 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 exactRestMatch = args[1]?.match(/^repos\/([^/]+\/[^/]+)\/pulls\/(\d+)$/) - if (args[0] === 'api' && exactRestMatch) { - const pr = prsByRepositoryAndNumber.get(`${host}/${exactRestMatch[1]}#${exactRestMatch[2]}`) - if (pr) { - return { stdout: JSON.stringify(pr) } - } - throw new Error('HTTP 404 Not Found') - } - const repoIndex = args.indexOf('--repo') - const exact = prsByRepositoryAndNumber.get(`${host}/${args[repoIndex + 1]}#${args[2]}`) - if (args[0] === 'pr' && args[1] === 'view') { - if (!exact) { - throw new Error('HTTP 404 Not Found') - } - 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(' ')}`) - } - ) -} - -/** A fork clone: `origin` is the fork, `upstream` is the repo PRs target. */ -function primeForkWithUpstream(): void { - resolvePRRepositoryCandidatesMock.mockResolvedValue({ - candidates: [UPSTREAM, ACME], - headRepo: ACME - }) - getOwnerRepoForRemoteMock.mockImplementation(async (_repoPath: string, remoteName: string) => - remoteName === 'upstream' ? UPSTREAM : remoteName === 'origin' ? ACME : null - ) -} - -describe('issue #26948: a branch tracking the default branch', () => { - beforeEach(() => { - resetPRForBranchMocks(clientMocks) - getOwnerRepoMock.mockResolvedValue(ACME) - }) - - it('does not attach a PR whose head is the default branch', async () => { - primeGit('refs/remotes/origin/develop') - primeGh({ 'acme:develop': [restPR('develop')] }) - - const pr = await getPRForBranch('/repo-root', 'feature/my-change') - - expect(pr).toBeNull() - }) - - it('drops a cached PR number whose head is the tracked default branch', async () => { - primeGit('refs/remotes/origin/develop') - primeGh({}) - - const outcome = await getPRForBranchOutcome('/repo-root', 'feature/my-change', null, null, 7) - - 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' }) - primeGh({ 'stablyai:develop': [restPR('develop', UPSTREAM, UPSTREAM)] }) - - const pr = await getPRForBranch('/repo-root', 'feature/my-change') - - expect(pr).toBeNull() - }) - - 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', UPSTREAM, UPSTREAM)] }) - - const pr = await getPRForBranch('/repo-root', 'feature/my-change') - - expect(pr).toBeNull() - }) - - 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', UPSTREAM, UPSTREAM)] }) - - const pr = await getPRForBranch('/repo-root', 'feature/my-change') - - expect(pr).toMatchObject({ number: 7 }) - }) - - it("still follows a contributor fork's branch that shares the default branch's name", async () => { - getOwnerRepoForRemoteMock.mockImplementation(async (_repoPath: string, remoteName: string) => - remoteName === 'contributor' ? { owner: 'contributor', repo: 'widgets' } : ACME - ) - primeGit('refs/remotes/contributor/develop') - 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.each(['closed', 'merged'] as const)( - 'clears a cached %s integration PR in a fork after upstream tracking is removed', - async (state) => { - primeForkWithUpstream() - primeGit('', { origin: 'main' }) - const integrationPR = restPR('develop', UPSTREAM, UPSTREAM) - primeGh( - {}, - { - ...integrationPR, - merged_at: state === 'merged' ? '2025-01-02T00:00:00Z' : null, - head: { - ...integrationPR.head, - sha: 'feature-head-oid', - repo: { name: 'widgets', owner: { login: 'stablyai' }, default_branch: 'develop' } - } - } - ) - - 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/stablyai/widgets/pull/7'] - }) - } - ) - - it('preserves a cached renamed feature PR using its own repository default metadata', async () => { - primeForkWithUpstream() - primeGit('', { origin: 'develop' }) - const featurePR = restPR('develop', UPSTREAM, UPSTREAM) - primeGh( - {}, - { - ...featurePR, - state: 'open', - head: { - ...featurePR.head, - repo: { name: 'widgets', owner: { login: 'stablyai' }, default_branch: 'main' } - } - } - ) - - expect(await getPRForBranch('/repo-root', 'feature/my-change', null, null, 7)).toMatchObject({ - number: 7, - headRefName: 'develop' - }) - }) - - it.each(['unavailable', 'changed-head'] as const)( - 'preserves an untracked cached PR when REST metadata is %s', - async (metadataState) => { - primeForkWithUpstream() - primeGit('', { origin: 'develop' }) - primeGh({}, { ...restPR('develop', UPSTREAM, UPSTREAM), state: 'open' }) - const ghImplementation = ghExecFileAsyncMock.getMockImplementation() - ghExecFileAsyncMock.mockImplementation(async (args: string[], options) => { - if (args[0] === 'api' && args[1] === 'repos/stablyai/widgets/pulls/7') { - if (metadataState === 'unavailable') { - throw new Error('network unavailable') - } - const pr = restPR('develop', ACME, UPSTREAM) - return { - stdout: JSON.stringify({ - ...pr, - head: { - ...pr.head, - repo: { name: 'widgets', owner: { login: 'acme' }, default_branch: 'develop' } - } - }) - } - } - return ghImplementation?.(args, options) - }) - - expect(await getPRForBranch('/repo-root', 'feature/my-change', null, null, 7)).toMatchObject({ - number: 7, - headRepo: UPSTREAM - }) - } - ) - - 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) - }) - - 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-tracked-upstream-fork-owner.test.ts b/src/main/github/client-tracked-upstream-fork-owner.test.ts deleted file mode 100644 index 617bbaff365c..000000000000 --- a/src/main/github/client-tracked-upstream-fork-owner.test.ts +++ /dev/null @@ -1,200 +0,0 @@ -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 { getPRForBranch } from './client/lookup/get-pr-for-branch' -import { resetPRForBranchMocks } from './client-test-harness' - -const { - ghExecFileAsyncMock, - getOwnerRepoForRemoteMock, - resolvePRRepositoryCandidatesMock, - gitExecFileAsyncMock -} = clientMocks - -describe('getPRForBranch', () => { - beforeEach(() => { - resetPRForBranchMocks(clientMocks) - }) - - it('uses the tracked upstream remote owner for fork branch lookup', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ - candidates: [ - { owner: 'stablyai', repo: 'orca' }, - { owner: 'origin-owner', repo: 'orca' } - ], - headRepo: { owner: 'origin-owner', repo: 'orca' } - }) - getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'fork-owner', repo: 'orca' }) - ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 78, - title: 'Fork upstream branch PR', - state: 'open', - html_url: 'https://github.com/stablyai/orca/pull/78', - updated_at: '2026-03-28T00:00:00Z', - draft: false, - mergeable: true, - base: { ref: 'main', sha: 'base-oid' }, - head: { ref: 'contributor/original', sha: 'upstream-head-oid' } - } - ]) - }) - .mockResolvedValueOnce({ - stdout: JSON.stringify({ - number: 78, - title: 'Hydrated fork upstream branch PR', - state: 'OPEN', - url: 'https://github.com/stablyai/orca/pull/78', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'MERGEABLE', - baseRefName: 'main', - headRefName: 'contributor/original', - baseRefOid: 'base-oid', - headRefOid: 'upstream-head-oid' - }) - }) - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'local-created-from-pr\0fork/contributor/original\n', - stderr: '' - }) - - const pr = await getPRForBranch('/repo-root', 'local-created-from-pr') - - expect(getOwnerRepoForRemoteMock).toHaveBeenCalledWith('/repo-root', 'fork', undefined) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 3, - [ - 'api', - 'repos/stablyai/orca/pulls?head=fork-owner%3Acontributor%2Foriginal&state=all&per_page=1' - ], - { cwd: '/repo-root' } - ) - expect(pr).toMatchObject({ - number: 78, - title: 'Hydrated fork upstream branch PR', - prRepo: { owner: 'stablyai', repo: 'orca' }, - headRepo: { owner: 'fork-owner', repo: 'orca' } - }) - }) - - it('uses the tracked upstream remote owner when the fork branch name matches locally', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ - candidates: [ - { owner: 'stablyai', repo: 'orca' }, - { owner: 'origin-owner', repo: 'orca' } - ], - headRepo: { owner: 'origin-owner', repo: 'orca' } - }) - getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'brennanb2025', repo: 'orca' }) - ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 6433, - title: 'Recover Windows worktree deletes from long paths', - state: 'open', - html_url: 'https://github.com/stablyai/orca/pull/6433', - updated_at: '2026-06-26T00:00:00Z', - draft: false, - mergeable: true, - base: { ref: 'main', sha: 'base-oid' }, - head: { - ref: 'brennanb2025/worktree-remove-fix', - sha: 'same-name-fork-head-oid' - } - } - ]) - }) - .mockResolvedValueOnce({ - stdout: JSON.stringify({ - number: 6433, - title: 'Recover Windows worktree deletes from long paths', - state: 'OPEN', - url: 'https://github.com/stablyai/orca/pull/6433', - statusCheckRollup: [], - updatedAt: '2026-06-26T00:00:00Z', - isDraft: false, - mergeable: 'MERGEABLE', - baseRefName: 'main', - headRefName: 'brennanb2025/worktree-remove-fix', - baseRefOid: 'base-oid', - headRefOid: 'same-name-fork-head-oid' - }) - }) - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'brennanb2025/worktree-remove-fix\0brennan/brennanb2025/worktree-remove-fix\n', - stderr: '' - }) - - const pr = await getPRForBranch('/repo-root', 'brennanb2025/worktree-remove-fix') - - expect(getOwnerRepoForRemoteMock).toHaveBeenCalledWith('/repo-root', 'brennan', undefined) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 3, - [ - 'api', - 'repos/stablyai/orca/pulls?head=brennanb2025%3Abrennanb2025%2Fworktree-remove-fix&state=all&per_page=1' - ], - { cwd: '/repo-root' } - ) - expect(pr).toMatchObject({ - number: 6433, - prRepo: { owner: 'stablyai', repo: 'orca' }, - headRepo: { owner: 'brennanb2025', repo: 'orca' } - }) - }) - - it('does not retry same-name tracked upstream lookup for the same head repo', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ - candidates: [{ owner: 'acme', repo: 'widgets' }], - headRepo: { owner: 'acme', repo: 'widgets' } - }) - getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'feature/no-pr\0origin/feature/no-pr\n', - stderr: '' - }) - - await expect(getPRForBranch('/repo-root', 'feature/no-pr')).resolves.toBeNull() - - expect(getOwnerRepoForRemoteMock).toHaveBeenCalledWith('/repo-root', 'origin', undefined) - expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) - expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Fno-pr&state=all&per_page=1'], - { cwd: '/repo-root' } - ) - }) -}) diff --git a/src/main/github/client-tracked-upstream-snapshot.test.ts b/src/main/github/client-tracked-upstream-snapshot.test.ts deleted file mode 100644 index bb1d78a62120..000000000000 --- a/src/main/github/client-tracked-upstream-snapshot.test.ts +++ /dev/null @@ -1,728 +0,0 @@ -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 { getPRForBranch } from './client/lookup/get-pr-for-branch' -import { - _getTrackedUpstreamBranchCacheSizesForTests, - __resetTrackedUpstreamBranchCacheForTests -} from './client/lookup/tracked-upstream-cache' -import { resetPRForBranchMocks } from './client-test-harness' - -const { - ghExecFileAsyncMock, - getOwnerRepoMock, - getOwnerRepoForRemoteMock, - resolvePRRepositoryCandidatesMock, - gitExecFileAsyncMock, - readLocalGitConfigSignatureMock -} = clientMocks - -describe('getPRForBranch', () => { - beforeEach(() => { - resetPRForBranchMocks(clientMocks) - }) - - it('falls back to the tracked upstream branch when the local branch name differs', async () => { - getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 78, - title: 'Upstream branch PR', - state: 'open', - html_url: 'https://github.com/acme/widgets/pull/78', - updated_at: '2026-03-28T00:00:00Z', - draft: false, - mergeable: true, - base: { ref: 'main', sha: 'base-oid' }, - head: { ref: 'contributor/original', sha: 'upstream-head-oid' } - } - ]) - }) - .mockResolvedValueOnce({ - stdout: JSON.stringify({ - number: 78, - title: 'Hydrated upstream branch PR', - state: 'OPEN', - url: 'https://github.com/acme/widgets/pull/78', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'MERGEABLE', - baseRefName: 'main', - headRefName: 'contributor/original', - baseRefOid: 'base-oid', - headRefOid: 'upstream-head-oid' - }) - }) - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'local-created-from-pr\0origin/contributor/original\n', - stderr: '' - }) - - const pr = await getPRForBranch('/repo-root', 'local-created-from-pr') - - expect(gitExecFileAsyncMock).toHaveBeenCalledWith( - ['for-each-ref', '--format=%(refname)%00%(upstream)', 'refs/heads'], - { cwd: '/repo-root' } - ) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 1, - ['api', 'repos/acme/widgets/pulls?head=acme%3Alocal-created-from-pr&state=all&per_page=1'], - { cwd: '/repo-root' } - ) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 2, - ['api', 'repos/acme/widgets/pulls?head=acme%3Acontributor%2Foriginal&state=all&per_page=1'], - { cwd: '/repo-root' } - ) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 3, - [ - 'pr', - 'view', - '78', - '--repo', - 'acme/widgets', - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid,headRepositoryOwner,headRepository' - ], - { cwd: '/repo-root' } - ) - expect(pr).toMatchObject({ - number: 78, - title: 'Hydrated upstream branch PR', - headSha: 'upstream-head-oid' - }) - }) - - it('does not repeat missing tracked-upstream probes during PR refresh polling', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValue({ - candidates: [{ owner: 'acme', repo: 'widgets' }], - headRepo: { owner: 'acme', repo: 'widgets' } - }) - ghExecFileAsyncMock.mockResolvedValue({ stdout: JSON.stringify([]) }) - gitExecFileAsyncMock.mockResolvedValue({ stdout: 'no-pr-branch\0\n', stderr: '' }) - - await getPRForBranch('/repo-root', 'no-pr-branch') - await getPRForBranch('/repo-root', 'no-pr-branch') - await getPRForBranch('/repo-root', 'no-pr-branch') - - const trackedUpstreamCalls = gitExecFileAsyncMock.mock.calls.filter(([args]) => - (args as string[]).includes('refs/heads') - ) - expect(trackedUpstreamCalls).toHaveLength(1) - }) - - it('releases tracked-upstream probe generations across runtime identities', async () => { - getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock.mockResolvedValue({ stdout: JSON.stringify([]) }) - gitExecFileAsyncMock.mockResolvedValue({ stdout: 'feature\0\n', stderr: '' }) - - await getPRForBranch('/repo-root', 'feature') - await getPRForBranch('/repo-root', 'feature', null, null, null, { - localGitExecOptions: { wslDistro: 'Ubuntu' } - }) - - expect(_getTrackedUpstreamBranchCacheSizesForTests()).toEqual({ - snapshots: 2, - inFlight: 0, - generations: 0 - }) - expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) - }) - - it('bounds unique tracked-upstream snapshots and sweeps expired identities', async () => { - vi.useFakeTimers() - try { - getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock.mockResolvedValue({ stdout: JSON.stringify([]) }) - gitExecFileAsyncMock.mockResolvedValue({ stdout: 'feature\0\n', stderr: '' }) - - for (let index = 0; index < 513; index += 1) { - await getPRForBranch(`/repo-root-${index}`, 'feature') - } - await getPRForBranch('/repo-root-512', 'feature') - - expect(_getTrackedUpstreamBranchCacheSizesForTests()).toEqual({ - snapshots: 512, - inFlight: 0, - generations: 0 - }) - expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(513) - - await vi.advanceTimersByTimeAsync(30_001) - await getPRForBranch('/repo-root-fresh', 'feature') - - expect(_getTrackedUpstreamBranchCacheSizesForTests()).toEqual({ - snapshots: 1, - inFlight: 0, - generations: 0 - }) - } finally { - vi.useRealTimers() - } - }) - - it('keeps stale probe cleanup from releasing a replacement generation', async () => { - let resolveOldProbe: (value: { stdout: string; stderr: string }) => void - let resolveCurrentProbe: (value: { stdout: string; stderr: string }) => void - getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock.mockResolvedValue({ stdout: JSON.stringify([]) }) - gitExecFileAsyncMock - .mockImplementationOnce( - () => - new Promise((resolve) => { - resolveOldProbe = resolve - }) - ) - .mockResolvedValueOnce({ stdout: 'replacement\0\n', stderr: '' }) - .mockImplementationOnce( - () => - new Promise((resolve) => { - resolveCurrentProbe = resolve - }) - ) - - const oldLookup = getPRForBranch('/repo-root', 'old') - await vi.waitFor(() => expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1)) - __resetTrackedUpstreamBranchCacheForTests() - await getPRForBranch('/repo-root', 'replacement') - const currentLookup = getPRForBranch('/repo-root', 'current') - await vi.waitFor(() => expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(3)) - - resolveOldProbe!({ stdout: 'old\0\n', stderr: '' }) - await oldLookup - expect(_getTrackedUpstreamBranchCacheSizesForTests()).toEqual({ - snapshots: 0, - inFlight: 1, - generations: 1 - }) - - resolveCurrentProbe!({ stdout: 'current\0\n', stderr: '' }) - await currentLookup - expect(_getTrackedUpstreamBranchCacheSizesForTests()).toEqual({ - snapshots: 1, - inFlight: 0, - generations: 0 - }) - }) - - it('releases tracked-upstream probe state when setup rejects', async () => { - getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock.mockResolvedValue({ stdout: JSON.stringify([]) }) - readLocalGitConfigSignatureMock.mockRejectedValue(new Error('git config unavailable')) - - await expect(getPRForBranch('/repo-root', 'feature')).resolves.toBeNull() - expect(_getTrackedUpstreamBranchCacheSizesForTests()).toEqual({ - snapshots: 0, - inFlight: 0, - generations: 0 - }) - }) - - it('does not fan out tracked-upstream probes after a transient for-each-ref failure', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValue({ - candidates: [{ owner: 'acme', repo: 'widgets' }], - headRepo: { owner: 'acme', repo: 'widgets' } - }) - ghExecFileAsyncMock.mockResolvedValue({ stdout: JSON.stringify([]) }) - gitExecFileAsyncMock - .mockRejectedValueOnce(new Error('fatal: cannot lock ref')) - .mockResolvedValue({ stdout: 'alpha\0\nbeta\0\ngamma\0\n', stderr: '' }) - - await getPRForBranch('/repo-root', 'alpha') - await getPRForBranch('/repo-root', 'beta') - await getPRForBranch('/repo-root', 'gamma') - - const trackedUpstreamCalls = gitExecFileAsyncMock.mock.calls.filter(([args]) => - (args as string[]).includes('refs/heads') - ) - expect(trackedUpstreamCalls).toHaveLength(2) - }) - - it('refreshes the tracked-upstream snapshot when a branch appears inside the TTL', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValue({ - candidates: [{ owner: 'acme', repo: 'widgets' }], - headRepo: { owner: 'acme', repo: 'widgets' } - }) - getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'existing\0\n', stderr: '' }) - .mockResolvedValueOnce({ - stdout: 'existing\0\nnew-feature\0origin/contributor/original\n', - stderr: '' - }) - ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 78, - title: 'New branch upstream PR', - state: 'open', - html_url: 'https://github.com/acme/widgets/pull/78', - updated_at: '2026-03-28T00:00:00Z', - draft: false, - mergeable: true, - base: { ref: 'main', sha: 'base-oid' }, - head: { ref: 'contributor/original', sha: 'upstream-head-oid' } - } - ]) - }) - .mockResolvedValueOnce({ - stdout: JSON.stringify({ - number: 78, - title: 'Hydrated new branch upstream PR', - state: 'OPEN', - url: 'https://github.com/acme/widgets/pull/78', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'MERGEABLE', - baseRefName: 'main', - headRefName: 'contributor/original', - baseRefOid: 'base-oid', - headRefOid: 'upstream-head-oid' - }) - }) - - await getPRForBranch('/repo-root', 'existing') - const pr = await getPRForBranch('/repo-root', 'new-feature') - - const trackedUpstreamCalls = gitExecFileAsyncMock.mock.calls.filter(([args]) => - (args as string[]).includes('refs/heads') - ) - expect(trackedUpstreamCalls).toHaveLength(2) - expect(pr).toMatchObject({ - number: 78, - title: 'Hydrated new branch upstream PR' - }) - }) - - it('parses full local branch refs from the tracked-upstream snapshot', async () => { - getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'refs/heads/feature\0origin/contributor/original\n', - stderr: '' - }) - ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 80, - title: 'Ambiguous ref upstream PR', - state: 'open', - html_url: 'https://github.com/acme/widgets/pull/80', - updated_at: '2026-03-28T00:00:00Z', - draft: false, - mergeable: true, - base: { ref: 'main', sha: 'base-oid' }, - head: { ref: 'contributor/original', sha: 'upstream-head-oid' } - } - ]) - }) - .mockResolvedValueOnce({ - stdout: JSON.stringify({ - number: 80, - title: 'Hydrated ambiguous ref upstream PR', - state: 'OPEN', - url: 'https://github.com/acme/widgets/pull/80', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'MERGEABLE', - baseRefName: 'main', - headRefName: 'contributor/original', - baseRefOid: 'base-oid', - headRefOid: 'upstream-head-oid' - }) - }) - - const pr = await getPRForBranch('/repo-root', 'feature') - - expect(pr).toMatchObject({ - number: 80, - title: 'Hydrated ambiguous ref upstream PR' - }) - }) - - it('parses full upstream refs from the tracked-upstream snapshot', async () => { - getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'fork-owner', repo: 'widgets' }) - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'refs/heads/feature\0refs/remotes/fork/contributor/original\n', - stderr: '' - }) - ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 83, - title: 'Full upstream ref PR', - state: 'open', - html_url: 'https://github.com/acme/widgets/pull/83', - updated_at: '2026-03-28T00:00:00Z', - draft: false, - mergeable: true, - base: { ref: 'main', sha: 'base-oid' }, - head: { ref: 'contributor/original', sha: 'upstream-head-oid' } - } - ]) - }) - .mockResolvedValueOnce({ - stdout: JSON.stringify({ - number: 83, - title: 'Hydrated full upstream ref PR', - state: 'OPEN', - url: 'https://github.com/acme/widgets/pull/83', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'MERGEABLE', - baseRefName: 'main', - headRefName: 'contributor/original', - baseRefOid: 'base-oid', - headRefOid: 'upstream-head-oid' - }) - }) - - const pr = await getPRForBranch('/repo-root', 'feature') - - expect(getOwnerRepoForRemoteMock).toHaveBeenCalledWith('/repo-root', 'fork', undefined) - expect(pr).toMatchObject({ - number: 83, - title: 'Hydrated full upstream ref PR', - headRepo: { owner: 'fork-owner', repo: 'widgets' } - }) - }) - - it('ignores full local-branch upstream refs from the tracked-upstream snapshot', async () => { - getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - gitExecFileAsyncMock.mockResolvedValueOnce({ - stdout: 'refs/heads/feature\0refs/heads/main\n', - stderr: '' - }) - ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - - const pr = await getPRForBranch('/repo-root', 'feature') - - expect(pr).toBeNull() - expect(getOwnerRepoForRemoteMock).not.toHaveBeenCalled() - expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) - }) - - it('invalidates the local tracked-upstream snapshot when git config changes', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValue({ - candidates: [{ owner: 'acme', repo: 'widgets' }], - headRepo: { owner: 'acme', repo: 'widgets' } - }) - getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - readLocalGitConfigSignatureMock - .mockResolvedValueOnce('/repo-root/.git/config\u0000mtime-a\u0000100') - .mockResolvedValueOnce('/repo-root/.git/config\u0000mtime-a\u0000100') - .mockResolvedValueOnce('/repo-root/.git/config\u0000mtime-b\u0000120') - .mockResolvedValueOnce('/repo-root/.git/config\u0000mtime-b\u0000120') - .mockResolvedValueOnce('/repo-root/.git/config\u0000mtime-b\u0000120') - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'feature\0\n', stderr: '' }) - .mockResolvedValueOnce({ stdout: 'feature\0origin/contributor/original\n', stderr: '' }) - ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 79, - title: 'Reconfigured upstream PR', - state: 'open', - html_url: 'https://github.com/acme/widgets/pull/79', - updated_at: '2026-03-28T00:00:00Z', - draft: false, - mergeable: true, - base: { ref: 'main', sha: 'base-oid' }, - head: { ref: 'contributor/original', sha: 'upstream-head-oid' } - } - ]) - }) - .mockResolvedValueOnce({ - stdout: JSON.stringify({ - number: 79, - title: 'Hydrated reconfigured upstream PR', - state: 'OPEN', - url: 'https://github.com/acme/widgets/pull/79', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'MERGEABLE', - baseRefName: 'main', - headRefName: 'contributor/original', - baseRefOid: 'base-oid', - headRefOid: 'upstream-head-oid' - }) - }) - - await getPRForBranch('/repo-root', 'feature') - const pr = await getPRForBranch('/repo-root', 'feature') - - const trackedUpstreamCalls = gitExecFileAsyncMock.mock.calls.filter(([args]) => - (args as string[]).includes('refs/heads') - ) - expect(trackedUpstreamCalls).toHaveLength(2) - expect(pr).toMatchObject({ - number: 79, - title: 'Hydrated reconfigured upstream PR' - }) - }) - - it('does not cache positive tracked-upstream entries when config changes during the snapshot', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValue({ - candidates: [{ owner: 'acme', repo: 'widgets' }], - headRepo: { owner: 'acme', repo: 'widgets' } - }) - getOwnerRepoForRemoteMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) - readLocalGitConfigSignatureMock - .mockResolvedValueOnce('/repo-root/.git/config\u0000mtime-a\u0000100') - .mockResolvedValueOnce('/repo-root/.git/config\u0000mtime-b\u0000120') - .mockResolvedValueOnce('/repo-root/.git/config\u0000mtime-b\u0000120') - .mockResolvedValueOnce('/repo-root/.git/config\u0000mtime-b\u0000120') - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'feature\0origin/old-upstream\n', stderr: '' }) - .mockResolvedValueOnce({ stdout: 'feature\0origin/contributor/original\n', stderr: '' }) - ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 82, - title: 'Stable config upstream PR', - state: 'open', - html_url: 'https://github.com/acme/widgets/pull/82', - updated_at: '2026-03-28T00:00:00Z', - draft: false, - mergeable: true, - base: { ref: 'main', sha: 'base-oid' }, - head: { ref: 'contributor/original', sha: 'upstream-head-oid' } - } - ]) - }) - - await getPRForBranch('/repo-root', 'feature') - const pr = await getPRForBranch('/repo-root', 'feature') - - const trackedUpstreamCalls = gitExecFileAsyncMock.mock.calls.filter(([args]) => - (args as string[]).includes('refs/heads') - ) - expect(trackedUpstreamCalls).toHaveLength(2) - expect(pr).toMatchObject({ - number: 82, - title: 'Stable config upstream PR' - }) - }) - - it('does not cache null tracked-upstream entries when config changes during the snapshot', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValue({ - candidates: [{ owner: 'acme', repo: 'widgets' }], - headRepo: { owner: 'acme', repo: 'widgets' } - }) - getOwnerRepoForRemoteMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) - readLocalGitConfigSignatureMock - .mockResolvedValueOnce('/repo-root/.git/config\u0000mtime-a\u0000100') - .mockResolvedValueOnce('/repo-root/.git/config\u0000mtime-b\u0000120') - .mockResolvedValueOnce('/repo-root/.git/config\u0000mtime-b\u0000120') - .mockResolvedValueOnce('/repo-root/.git/config\u0000mtime-b\u0000120') - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'feature\0\n', stderr: '' }) - .mockResolvedValueOnce({ stdout: 'feature\0origin/contributor/original\n', stderr: '' }) - ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 85, - title: 'Unstable config upstream PR', - state: 'open', - html_url: 'https://github.com/acme/widgets/pull/85', - updated_at: '2026-03-28T00:00:00Z', - draft: false, - mergeable: true, - base: { ref: 'main', sha: 'base-oid' }, - head: { ref: 'contributor/original', sha: 'upstream-head-oid' } - } - ]) - }) - - await getPRForBranch('/repo-root', 'feature') - const pr = await getPRForBranch('/repo-root', 'feature') - - const trackedUpstreamCalls = gitExecFileAsyncMock.mock.calls.filter(([args]) => - (args as string[]).includes('refs/heads') - ) - expect(trackedUpstreamCalls).toHaveLength(2) - expect(pr).toMatchObject({ - number: 85, - title: 'Unstable config upstream PR' - }) - }) - - it('coalesces concurrent missing tracked-upstream probes', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValue({ - candidates: [{ owner: 'acme', repo: 'widgets' }], - headRepo: { owner: 'acme', repo: 'widgets' } - }) - ghExecFileAsyncMock.mockResolvedValue({ stdout: JSON.stringify([]) }) - gitExecFileAsyncMock.mockImplementation(async () => { - await Promise.resolve() - return { stdout: 'no-pr-branch\0\n', stderr: '' } - }) - - await Promise.all([ - getPRForBranch('/repo-root', 'no-pr-branch'), - getPRForBranch('/repo-root', 'no-pr-branch'), - getPRForBranch('/repo-root', 'no-pr-branch') - ]) - - const trackedUpstreamCalls = gitExecFileAsyncMock.mock.calls.filter(([args]) => - (args as string[]).includes('refs/heads') - ) - expect(trackedUpstreamCalls).toHaveLength(1) - }) - - it('does not cache synthetic nulls from concurrent tracked-upstream waiters', async () => { - let resolveSnapshot: (value: { stdout: string; stderr: string }) => void - resolvePRRepositoryCandidatesMock.mockResolvedValue({ - candidates: [{ owner: 'acme', repo: 'widgets' }], - headRepo: { owner: 'acme', repo: 'widgets' } - }) - getOwnerRepoForRemoteMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) - readLocalGitConfigSignatureMock.mockResolvedValue( - '/repo-root/.git/config\u0000mtime-a\u0000100' - ) - gitExecFileAsyncMock - .mockImplementationOnce( - () => - new Promise((resolve) => { - resolveSnapshot = resolve - }) - ) - .mockResolvedValueOnce({ stdout: 'new-feature\0origin/contributor/original\n', stderr: '' }) - ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 84, - title: 'Concurrent waiter upstream PR', - state: 'open', - html_url: 'https://github.com/acme/widgets/pull/84', - updated_at: '2026-03-28T00:00:00Z', - draft: false, - mergeable: true, - base: { ref: 'main', sha: 'base-oid' }, - head: { ref: 'contributor/original', sha: 'upstream-head-oid' } - } - ]) - }) - - const existingLookup = getPRForBranch('/repo-root', 'existing') - const waiterLookup = getPRForBranch('/repo-root', 'new-feature') - await vi.waitFor(() => expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1)) - resolveSnapshot!({ stdout: 'existing\0\n', stderr: '' }) - const [, waiterPr] = await Promise.all([existingLookup, waiterLookup]) - - const trackedUpstreamCalls = gitExecFileAsyncMock.mock.calls.filter(([args]) => - (args as string[]).includes('refs/heads') - ) - expect(trackedUpstreamCalls).toHaveLength(2) - expect(waiterPr).toMatchObject({ - number: 84, - title: 'Concurrent waiter upstream PR' - }) - }) - - it('keeps missing tracked-upstream probes separate for host and WSL runtimes', async () => { - resolvePRRepositoryCandidatesMock.mockResolvedValue({ - candidates: [{ owner: 'acme', repo: 'widgets' }], - headRepo: { owner: 'acme', repo: 'widgets' } - }) - ghExecFileAsyncMock.mockResolvedValue({ stdout: JSON.stringify([]) }) - gitExecFileAsyncMock.mockResolvedValue({ stdout: 'no-pr-branch\0\n', stderr: '' }) - - await getPRForBranch('/repo-root', 'no-pr-branch') - await getPRForBranch('/repo-root', 'no-pr-branch', null, null, null, { - localGitExecOptions: { wslDistro: 'Ubuntu' } - }) - await getPRForBranch('/repo-root', 'no-pr-branch') - await getPRForBranch('/repo-root', 'no-pr-branch', null, null, null, { - localGitExecOptions: { wslDistro: 'Ubuntu' } - }) - - const trackedUpstreamCalls = gitExecFileAsyncMock.mock.calls.filter(([args]) => - (args as string[]).includes('refs/heads') - ) - expect(trackedUpstreamCalls).toHaveLength(2) - expect(trackedUpstreamCalls[0][1]).toEqual({ cwd: '/repo-root' }) - expect(trackedUpstreamCalls[1][1]).toEqual({ - cwd: '/repo-root', - wslDistro: 'Ubuntu' - }) - }) - - it('rechecks missing tracked-upstream probes after the null-cache TTL expires', async () => { - vi.useFakeTimers() - try { - resolvePRRepositoryCandidatesMock.mockResolvedValue({ - candidates: [{ owner: 'acme', repo: 'widgets' }], - headRepo: { owner: 'acme', repo: 'widgets' } - }) - ghExecFileAsyncMock.mockResolvedValue({ stdout: JSON.stringify([]) }) - gitExecFileAsyncMock - .mockRejectedValueOnce(new Error("fatal: no upstream configured for branch 'feature'")) - .mockResolvedValueOnce({ stdout: 'feature\0origin/contributor/original\n', stderr: '' }) - - await getPRForBranch('/repo-root', 'feature') - await vi.advanceTimersByTimeAsync(30_001) - await getPRForBranch('/repo-root', 'feature') - - const trackedUpstreamCalls = gitExecFileAsyncMock.mock.calls.filter(([args]) => - (args as string[]).includes('refs/heads') - ) - expect(trackedUpstreamCalls).toHaveLength(2) - } finally { - vi.useRealTimers() - } - }) -}) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index ed72bc122323..e93b0e11de6d 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -3,10 +3,6 @@ export { _resetMergeQueueCacheForTests } from './client/detect/repository-merge-metadata-cache' export { _resetPRStackSummaryCacheForTests } from './client/lookup/pr-stack-summary-cache' -export { - _getTrackedUpstreamBranchCacheSizesForTests, - __resetTrackedUpstreamBranchCacheForTests -} from './client/lookup/tracked-upstream-cache' export { addPRReviewComment, addPRReviewCommentReply } from './client/create/add-pr-review-comment' export { __resetOrcaStarCheckForTests, checkOrcaStarred, starOrca } from './client/fetch/orca-star' export { countWorkItems } from './client/list/count-work-items' diff --git a/src/main/github/client/lookup/branch-lookup-resolution.ts b/src/main/github/client/lookup/branch-lookup-resolution.ts index 4c30a4dad717..7e4c8590650b 100644 --- a/src/main/github/client/lookup/branch-lookup-resolution.ts +++ b/src/main/github/client/lookup/branch-lookup-resolution.ts @@ -12,7 +12,6 @@ import { import { mapPRState } from '../../mappers' import { noteRepositoryRateLimitSpend } from '../../rate-limit' import { ownerRepoFromPullRequestUrl } from './../github-exec-scope' -import { prRefreshUpstreamError } from './../gh-error-predicates' import { isMergedImplicitPR, shouldHideMergedImplicitPR, @@ -22,16 +21,12 @@ import { type PullRequestLookupData, type GitHubPRBranchLookupOptions } from './pull-request-lookup-data' -import { lookupPRByBranchName } from './pr-branch-lookup' +import { lookupPRByGhBranch, lookupPRByHead } 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 } from './tracked-upstream-cache' -import { getTrackedUpstreamBranch } from './tracked-upstream-branch' -import { - createImplicitDefaultBranchPRPolicy, - type ImplicitDefaultBranchPRContext -} from './implicit-default-branch-pr' +import { createReviewHeadPRPolicy } from './review-head-pr-policy' +import { resolveBranchReviewTarget } from '../../../source-control/review-head' import { PR_BRANCH_LOOKUP_BUCKETS } from './pr-lookup-rate-limit' import type { HostedReviewLocalGitOptions } from './../github-exec-scope' export async function resolvePRForBranchOutcome(input: { @@ -73,24 +68,33 @@ export async function resolvePRForBranchOutcome(input: { for (const bucket of PR_BRANCH_LOOKUP_BUCKETS) { noteRepositoryRateLimitSpend(headRepo ?? candidates[0], bucket, 1, ghOptions) } + // Why: a branch's review is the one whose head is where this branch is pushed — not whatever + // shares its local name (#27811) or the branch it tracks (#26948). + const { linkedNumber, head: branchHead } = await resolveBranchReviewTarget( + repoPath, + branchName, + linkedPRNumber, + connectionId, + localGitOptions + ) + const branchHeadRepo = !branchHead + ? null + : branchHead.remote === 'origin' + ? headRepo + : await getGitHubApiRepositoryForRemote( + repoPath, + branchHead.remote, + connectionId, + localGitOptions + ).catch(() => null) + const headPolicy = createReviewHeadPRPolicy( + branchHead ? { repo: branchHeadRepo, branch: branchHead.branch } : null + ) let data: PullRequestLookupData | null = null let dataRepo: OwnerRepo | null = null - let dataHeadRepo: OwnerRepo | null = headRepo - let pendingBranchLookupError: unknown - let hasPendingBranchLookupError = false + let dataHeadRepo: OwnerRepo | null = branchHeadRepo ?? headRepo let currentHeadOidForMergedImplicit: string | null | undefined let usedExactNumberLookup = false - const defaultBranchPRContext: ImplicitDefaultBranchPRContext = { - originHeadRepo: headRepo, - trackedUpstream: null, - branchName, - linkedPRNumber, - repoPath, - connectionId, - localGitOptions, - ghOptions - } - const implicitPRPolicy = createImplicitDefaultBranchPRPolicy(defaultBranchPRContext) const explicitCurrentHeadOid = typeof options.currentHeadOid === 'string' && options.currentHeadOid.trim().length > 0 @@ -120,7 +124,7 @@ export async function resolvePRForBranchOutcome(input: { candidate: PullRequestLookupData | null, candidateRepo: OwnerRepo | null ) => { - if (!candidate || !isMergedImplicitPR(candidate, linkedPRNumber)) { + if (!candidate || !isMergedImplicitPR(candidate, linkedNumber)) { return false } // Why: prefer the caller's worktree HEAD; only shell out (main repo path) when no explicit oid, keeping merged-at-head PRs visible for secondary worktrees. @@ -128,7 +132,7 @@ export async function resolvePRForBranchOutcome(input: { explicitCurrentHeadOid !== null ? explicitCurrentHeadOid : await getCurrentHeadOid(repoPath, connectionId, localGitOptions) - if (!shouldHideMergedImplicitPR(candidate, linkedPRNumber, currentHeadOidForMergedImplicit)) { + if (!shouldHideMergedImplicitPR(candidate, linkedNumber, currentHeadOidForMergedImplicit)) { return false } // Why: a head that is one of the PR's own commits (update-branch/web commits) is the same work, not a reused branch name — keep the merged PR visible. @@ -138,112 +142,61 @@ export async function resolvePRForBranchOutcome(input: { ) } - if (typeof linkedPRNumber === 'number') { + if (typeof linkedNumber === 'number') { usedExactNumberLookup = true const exactLookup = await lookupPRByNumber({ candidates, - number: linkedPRNumber, + number: linkedNumber, ghOptions, executionScope }) data = exactLookup.data dataRepo = exactLookup.dataRepo - } else if (branchName) { - // During a rebase (detached HEAD) branch is empty; an empty --head filter makes gh return an arbitrary PR. - const branchLookup = await lookupPRByBranchName({ + } else if (branchHead && branchHeadRepo && candidates.length > 0) { + const headLookup = await lookupPRByHead({ candidates, - headRepo, - branchName, + headRepo: branchHeadRepo, + branchName: branchHead.branch, ghOptions, - executionScope, - policy: implicitPRPolicy + executionScope }) - data = branchLookup.data - dataRepo = branchLookup.dataRepo - if ('pendingError' in branchLookup) { - pendingBranchLookupError = branchLookup.pendingError - hasPendingBranchLookupError = true - } - if (!data) { - // Why: the tracked upstream identifies the real PR head by branch name or fork owner even when local branch names match. - const upstreamBranch = await getTrackedUpstreamBranch( - repoPath, - branchName, - connectionId, - localGitOptions - ) - if (upstreamBranch) { - const upstreamHeadRepo = - (await getGitHubApiRepositoryForRemote( - repoPath, - upstreamBranch.remoteName, - connectionId, - localGitOptions - )) ?? headRepo - if ( - upstreamHeadRepo && - shouldRetryTrackedUpstreamBranch(upstreamBranch, branchName, upstreamHeadRepo, headRepo) - ) { - defaultBranchPRContext.trackedUpstream = { - branch: upstreamBranch, - headRepo: upstreamHeadRepo - } - const upstreamLookup = await lookupPRByBranchName({ - candidates, - headRepo: upstreamHeadRepo, - branchName: upstreamBranch.branchName, - ghOptions, - executionScope, - policy: implicitPRPolicy - }) - data = upstreamLookup.data - dataRepo = upstreamLookup.dataRepo - if (!hasPendingBranchLookupError && 'pendingError' in upstreamLookup) { - pendingBranchLookupError = upstreamLookup.pendingError - hasPendingBranchLookupError = true - } - if (data) { - dataHeadRepo = upstreamHeadRepo - } - } - } - } + data = headLookup.data + dataRepo = headLookup.dataRepo + } else if (branchHead && candidates.length === 0) { + data = await lookupPRByGhBranch(branchHead.branch, ghOptions) } let mergedBranchLookupNumber: number | null = null if (await hideMergedImplicitPR(data, dataRepo)) { mergedBranchLookupNumber = data?.number ?? null data = null dataRepo = null - dataHeadRepo = headRepo + dataHeadRepo = branchHeadRepo ?? headRepo } - if (!data && typeof linkedPRNumber !== 'number' && typeof fallbackPRNumber === 'number') { + if (!data && linkedNumber === null && typeof fallbackPRNumber === 'number') { usedExactNumberLookup = true const fallbackLookup = await lookupPRByNumber({ candidates, number: fallbackPRNumber, ghOptions, executionScope, - policy: implicitPRPolicy + policy: headPolicy }) data = fallbackLookup.data dataRepo = fallbackLookup.dataRepo } if (!data) { - if (hasPendingBranchLookupError) { - return prRefreshUpstreamError(pendingBranchLookupError) - } return { kind: 'no-pr', fetchedAt: Date.now(), - ...(implicitPRPolicy.rejectedPRUrls.size > 0 - ? { rejectedPRUrls: [...implicitPRPolicy.rejectedPRUrls] } + ...(headPolicy.rejectedPRUrls.size > 0 + ? { rejectedPRUrls: [...headPolicy.rejectedPRUrls] } : {}) } } dataHeadRepo = pullRequestHeadRepository(data, dataRepo) ?? dataHeadRepo const headDivergedFromMergedPRAtOid = await linkedMergedPRDivergedHeadOid( data, - linkedPRNumber, + linkedNumber, explicitCurrentHeadOid, (candidate, headOid) => mergedPRContainsHead( @@ -258,7 +211,7 @@ export async function resolvePRForBranchOutcome(input: { data.number === fallbackPRNumber const explicitHeadHidesMergedImplicitPR = explicitCurrentHeadOid !== null && - shouldHideMergedImplicitPR(data, linkedPRNumber, explicitCurrentHeadOid) && + shouldHideMergedImplicitPR(data, linkedNumber, explicitCurrentHeadOid) && (await mergedPRContainsHead(data, dataRepo, explicitCurrentHeadOid)) !== 'contained' // Why no lazy-HEAD re-check: fallback numbers were already gated on head equality/containment; re-hiding would blank kept deleted-head merged PRs. const shouldPreserveMergedFallback = @@ -276,7 +229,7 @@ export async function resolvePRForBranchOutcome(input: { await shouldHideNonOpenReviewOnDefaultBranch({ state: mapPRState(data.state, data.isDraft), reviewNumber: data.number, - linkedReviewNumber: linkedPRNumber, + linkedReviewNumber: linkedNumber, branchName, repoPath, connectionId, diff --git a/src/main/github/client/lookup/implicit-default-branch-pr.ts b/src/main/github/client/lookup/implicit-default-branch-pr.ts deleted file mode 100644 index ca852798beab..000000000000 --- a/src/main/github/client/lookup/implicit-default-branch-pr.ts +++ /dev/null @@ -1,103 +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 { GhExecOptions, HostedReviewLocalGitOptions } from './../github-exec-scope' -import type { TrackedUpstreamBranch } from './tracked-upstream-cache' -import { - pullRequestHeadRepository, - type PullRequestLookupData, - type PullRequestLookupPolicy -} from './pull-request-lookup-data' -import { getRestPRByNumber } from './pr-number-lookup' - -export type ImplicitDefaultBranchPRContext = { - originHeadRepo: OwnerRepo | null - trackedUpstream: { branch: TrackedUpstreamBranch; headRepo: OwnerRepo } | null - branchName: string - linkedPRNumber?: number | null - repoPath: string - connectionId?: string | null - localGitOptions: HostedReviewLocalGitOptions - ghOptions: GhExecOptions -} - -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 || - !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) { - try { - // Removed tracking can leave a cached integration PR without a matching remote. - const metadata = await getRestPRByNumber(prRepo, data.number, input.ghOptions) - const metadataHeadRepo = pullRequestHeadRepository(metadata, prRepo) - return ( - metadataHeadRepo !== null && - githubRepoIdentityKey(metadataHeadRepo) === headRepoKey && - metadata.headRefName === data.headRefName && - metadata.headDefaultBranchName === data.headRefName - ) - } catch { - 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/pr-branch-lookup.ts b/src/main/github/client/lookup/pr-branch-lookup.ts index 82fc4494a7c3..1b56fdb431d6 100644 --- a/src/main/github/client/lookup/pr-branch-lookup.ts +++ b/src/main/github/client/lookup/pr-branch-lookup.ts @@ -5,61 +5,51 @@ import { githubRepositoryWebHost, type GitHubApiRepository } from '../../github-api-repository' +import { noteRepositoryRateLimitSpend } from '../../rate-limit' import type { GhExecOptions } from './../github-exec-scope' import { isNoPullRequestError } from './../gh-error-predicates' import { PR_LOOKUP_JSON_FIELDS, - 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' -export async function getRestPRForBranch( +import { + BRANCH_REVIEW_PAGE_SIZE, + selectReviewForHead +} from '../../../source-control/branch-review-selection' + +/** The PR whose head is exactly `headOwner:branchName`, filtered by GitHub (`head=` matches the owner and branch). */ +export async function getRestPRForHead( prRepo: GitHubApiRepository, headOwner: string, branchName: string, ghOptions: ReturnType ): Promise { const head = encodeURIComponent(`${headOwner}:${branchName}`) - const { stdout } = await ghExecFileAsync( - ['api', `repos/${prRepo.owner}/${prRepo.repo}/pulls?head=${head}&state=all&per_page=1`], - { ...ghOptions, ...githubHostExecOptions(prRepo) } + const pr = await selectReviewForHead( + async (states) => { + if (states === 'open') { + noteRepositoryRateLimitSpend(prRepo, 'core', 1, ghOptions) + } + const { stdout } = await ghExecFileAsync( + [ + 'api', + `repos/${prRepo.owner}/${prRepo.repo}/pulls?head=${head}&state=${states}&per_page=${BRANCH_REVIEW_PAGE_SIZE}` + ], + { ...ghOptions, ...githubHostExecOptions(prRepo) } + ) + const list: RestPullRequest[] = JSON.parse(stdout) + return list + }, + (pr) => pr.state.toLowerCase() === 'open' ) - const list = JSON.parse(stdout) as RestPullRequest[] - const pr = list[0] return pr ? mapRestPullRequest(pr) : null } -export async function getFallbackPRListForBranch( - prRepo: GitHubApiRepository, - branchName: string, - ghOptions: ReturnType -): Promise { - const { stdout } = await ghExecFileAsync( - [ - 'pr', - 'list', - '--repo', - `${prRepo.owner}/${prRepo.repo}`, - '--head', - branchName, - '--state', - 'all', - '--limit', - '1', - '--json', - PR_BRANCH_LIST_JSON_FIELDS - ], - { ...ghOptions, ...githubHostExecOptions(prRepo) } - ) - const list = JSON.parse(stdout) as PullRequestLookupData[] - return list[0] ?? null -} - export async function hydrateBranchLookupWithExactPR( ownerRepo: OwnerRepo, branchData: PullRequestLookupData | null, @@ -79,109 +69,54 @@ export async function hydrateBranchLookupWithExactPR( } } -export async function lookupPRByBranchName(args: { +/** The PR for a branch pushed to `headRepo`, asked of each base candidate on that host. */ +export async function lookupPRByHead(args: { candidates: OwnerRepo[] - headRepo: OwnerRepo | null + headRepo: OwnerRepo branchName: string ghOptions: GhExecOptions executionScope: string - policy?: PullRequestLookupPolicy -}): Promise<{ - data: PullRequestLookupData | null - dataRepo: OwnerRepo | null - pendingError?: unknown -}> { - if (args.candidates.length > 0) { - 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( - candidate, - args.headRepo.owner, - args.branchName, - args.ghOptions - ) - : await getFallbackPRListForBranch(candidate, args.branchName, args.ghOptions) - // Why: REST/list branch lookup identifies the PR cheaply; exact `gh pr view` carries review, merge-queue, and auto-merge state. - const data = await hydrateBranchLookupWithExactPR( - candidate, - branchData, - args.ghOptions, - args.executionScope - ) - if ( - data && - pullRequestMatchesHeadRepository(data, candidate, args.headRepo) && - (!args.policy || (await args.policy.accepts(data, candidate))) - ) { - return { data, dataRepo: candidate } - } - } catch (err) { - if (args.headRepo) { - throw err - } - if (!hasPendingError) { - pendingError = err - hasPendingError = true - } - try { - const branchData = await getRestPRForBranch( - candidate, - candidate.owner, - args.branchName, - args.ghOptions - ) - const data = await hydrateBranchLookupWithExactPR( - candidate, - branchData, - args.ghOptions, - args.executionScope - ) - if ( - data && - pullRequestMatchesHeadRepository(data, candidate, args.headRepo) && - (!args.policy || (await args.policy.accepts(data, candidate))) - ) { - return { data, dataRepo: candidate } - } - } catch (retryErr) { - if (!hasPendingError) { - pendingError = retryErr - hasPendingError = true - } - } - } +}): Promise<{ data: PullRequestLookupData | null; dataRepo: OwnerRepo | null }> { + const headHost = githubRepositoryWebHost(args.headRepo).trim().toLowerCase() + for (const candidate of args.candidates) { + if (githubRepositoryWebHost(candidate).trim().toLowerCase() !== headHost) { + continue + } + const branchData = await getRestPRForHead( + candidate, + args.headRepo.owner, + args.branchName, + args.ghOptions + ) + // Why: REST/list branch lookup identifies the PR cheaply; exact `gh pr view` carries review, merge-queue, and auto-merge state. + const data = await hydrateBranchLookupWithExactPR( + candidate, + branchData, + args.ghOptions, + args.executionScope + ) + if (data && pullRequestMatchesHeadRepository(data, candidate, args.headRepo)) { + return { data, dataRepo: candidate } } - // Why: branch-list failures are ambiguous for fork discovery; give exact fallback-number recovery a chance before surfacing the error. - return hasPendingError - ? { data: null, dataRepo: null, pendingError } - : { data: null, dataRepo: null } } + return { data: null, dataRepo: null } +} +/** No GitHub candidate resolved here: let `gh` find the branch's PR in its own repo context. */ +export async function lookupPRByGhBranch( + branchName: string, + ghOptions: GhExecOptions +): Promise { try { const { stdout } = await ghExecFileAsync( - ['pr', 'view', args.branchName, '--json', PR_LOOKUP_JSON_FIELDS], - args.ghOptions + ['pr', 'view', branchName, '--json', PR_LOOKUP_JSON_FIELDS], + ghOptions ) - 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 } + const data: PullRequestLookupData = JSON.parse(stdout) + return normalizePullRequestLookupData(data) } catch (err) { if (isNoPullRequestError(err)) { - return { data: null, dataRepo: null } + return null } throw err } diff --git a/src/main/github/client/lookup/review-head-pr-policy.ts b/src/main/github/client/lookup/review-head-pr-policy.ts new file mode 100644 index 000000000000..b0a7da940eee --- /dev/null +++ b/src/main/github/client/lookup/review-head-pr-policy.ts @@ -0,0 +1,53 @@ +import type { OwnerRepo } from '../../gh-utils' +import { githubRepoIdentityKey } from '../../../../shared/github/repository-identity-key' +import { + pullRequestHeadRepository, + type PullRequestLookupData, + type PullRequestLookupPolicy +} from './pull-request-lookup-data' + +export type GitHubReviewHead = { repo: OwnerRepo | null; branch: string } + +function isFromReviewHead( + data: PullRequestLookupData, + prRepo: OwnerRepo | null, + head: GitHubReviewHead +): boolean { + if (data.headRefName !== head.branch) { + return false + } + const prHeadRepo = pullRequestHeadRepository(data, prRepo) + return ( + !prHeadRepo || + !head.repo || + githubRepoIdentityKey(prHeadRepo) === githubRepoIdentityKey(head.repo) + ) +} + +/** + * Accepts a remembered PR number only when that PR's head is this branch's review head, so a + * number cached from a wrong match (#26948, #27811) is dropped instead of re-shown. Without a head + * (detached HEAD mid-rebase) there is nothing to compare, so the remembered PR stays. + */ +export function createReviewHeadPRPolicy(head: GitHubReviewHead | null): PullRequestLookupPolicy { + const rejected = new Set() + const rejectedPRUrls = new Set() + return { + rejectedPRUrls, + async accepts(data, prRepo) { + if (!head || isFromReviewHead(data, prRepo, head)) { + return true + } + if (prRepo) { + rejected.add(`${githubRepoIdentityKey(prRepo)}#${data.number}`) + } + if (data.url) { + rejectedPRUrls.add(data.url) + } + return false + }, + isRejected(prRepo, number) { + return rejected.has(`${githubRepoIdentityKey(prRepo)}#${number}`) + } + } +} diff --git a/src/main/github/client/lookup/tracked-upstream-branch.ts b/src/main/github/client/lookup/tracked-upstream-branch.ts deleted file mode 100644 index edfc57934e46..000000000000 --- a/src/main/github/client/lookup/tracked-upstream-branch.ts +++ /dev/null @@ -1,181 +0,0 @@ -import { gitExecFileAsync } from '../../gh-utils' -import { readLocalGitConfigSignature } from '../../local-git-config-signature' -import type { GitAdmissionTier } from '../../../git/command-runner/git-exec-options' -import { - TRACKED_UPSTREAM_SNAPSHOT_CACHE_TTL_MS, - trackedUpstreamSnapshotCache, - trackedUpstreamSnapshotInFlight, - trackedUpstreamSnapshotGenerations, - beginTrackedUpstreamSnapshotProbe, - finishTrackedUpstreamSnapshotProbe, - pruneTrackedUpstreamSnapshotCache, - parseTrackedUpstreamBranch, - getCacheableTrackedUpstreamSnapshot, - canUseCachedTrackedUpstreamBranch, - doesTrackedUpstreamCacheConfigSignatureMatch, - getTrackedUpstreamBranchCacheKey, - type TrackedUpstreamBranch, - type TrackedUpstreamSnapshotProbeResult -} from './tracked-upstream-cache' -import { requireReachableGitRoute } from '../../../providers/execution-host-provider-dispatch' -import { getConnectionExecutionHostId } from '../../../../shared/execution-host' -export async function getTrackedUpstreamBranch( - repoPath: string, - branchName: string, - connectionId?: string | null, - localGitOptions: { wslDistro?: string; admissionTier?: GitAdmissionTier } = {} -): Promise { - const cacheKey = getTrackedUpstreamBranchCacheKey(repoPath, connectionId, localGitOptions) - const now = Date.now() - const cached = trackedUpstreamSnapshotCache.get(cacheKey) - if (cached && cached.expiresAt > now) { - const configSignatureMatches = await doesTrackedUpstreamCacheConfigSignatureMatch( - cached, - repoPath, - connectionId, - localGitOptions - ) - if ( - configSignatureMatches && - cached.upstreamsByBranchName.has(branchName) && - canUseCachedTrackedUpstreamBranch(cached, branchName) - ) { - return cached.upstreamsByBranchName.get(branchName) ?? null - } - trackedUpstreamSnapshotCache.delete(cacheKey) - } - if (cached) { - trackedUpstreamSnapshotCache.delete(cacheKey) - } - - const inFlight = trackedUpstreamSnapshotInFlight.get(cacheKey) - if (inFlight) { - const result = await inFlight - if (result.upstreamsByBranchName.has(branchName)) { - return result.upstreamsByBranchName.get(branchName) ?? null - } - // Why: a concurrent snapshot may finish before this branch exists in git; re-probe instead of returning a synthetic null. - const retryInFlight = trackedUpstreamSnapshotInFlight.get(cacheKey) - if (retryInFlight) { - const retryResult = await retryInFlight - return retryResult.upstreamsByBranchName.get(branchName) ?? null - } - } - - // Why: PR polling asks about hundreds of branches at once; read all upstreams in one git process per repo/runtime, not one probe per branch. - const probeGeneration = beginTrackedUpstreamSnapshotProbe(cacheKey) - const probe = probeTrackedUpstreamSnapshot(repoPath, connectionId, localGitOptions) - trackedUpstreamSnapshotInFlight.set(cacheKey, probe) - try { - const result = await probe - if (result.cacheable && trackedUpstreamSnapshotGenerations.get(cacheKey) === probeGeneration) { - trackedUpstreamSnapshotCache.set(cacheKey, { - ...(result.gitConfigSignature ? { gitConfigSignature: result.gitConfigSignature } : {}), - upstreamsByBranchName: getCacheableTrackedUpstreamSnapshot(result.upstreamsByBranchName), - expiresAt: Date.now() + TRACKED_UPSTREAM_SNAPSHOT_CACHE_TTL_MS - }) - pruneTrackedUpstreamSnapshotCache(Date.now()) - } - if (trackedUpstreamSnapshotGenerations.get(cacheKey) !== probeGeneration) { - const fresherCached = trackedUpstreamSnapshotCache.get(cacheKey) - if (fresherCached?.upstreamsByBranchName.has(branchName)) { - return fresherCached.upstreamsByBranchName.get(branchName) ?? null - } - } - return result.upstreamsByBranchName.get(branchName) ?? null - } finally { - if (trackedUpstreamSnapshotInFlight.get(cacheKey) === probe) { - trackedUpstreamSnapshotInFlight.delete(cacheKey) - } - finishTrackedUpstreamSnapshotProbe(cacheKey, probeGeneration) - } -} - -export async function probeTrackedUpstreamSnapshot( - repoPath: string, - connectionId?: string | null, - localGitOptions: { wslDistro?: string; admissionTier?: GitAdmissionTier } = {} -): Promise { - const startingGitConfigSignature = await readLocalGitConfigSignature({ - repoPath, - connectionId: connectionId ?? null, - ...localGitOptions - }) - const { probeFailed, upstreamsByBranchName } = await probeTrackedUpstreamBranches( - repoPath, - connectionId, - localGitOptions - ) - const endingGitConfigSignature = await readLocalGitConfigSignature({ - repoPath, - connectionId: connectionId ?? null, - ...localGitOptions - }) - const isLocalHostRuntime = !connectionId && !localGitOptions.wslDistro - const configSignatureChanged = - isLocalHostRuntime && startingGitConfigSignature !== endingGitConfigSignature - const gitConfigSignature = - startingGitConfigSignature === endingGitConfigSignature ? endingGitConfigSignature : undefined - return { - // Why: don't cache an empty snapshot after a transient git failure, or every branch lookup re-probes on the next refresh tick. - cacheable: !configSignatureChanged && !probeFailed, - probeFailed, - ...(gitConfigSignature ? { gitConfigSignature } : {}), - upstreamsByBranchName - } -} - -export async function probeTrackedUpstreamBranches( - repoPath: string, - connectionId?: string | null, - localGitOptions: { wslDistro?: string; admissionTier?: GitAdmissionTier } = {} -): Promise<{ - probeFailed: boolean - upstreamsByBranchName: Map -}> { - const args = ['for-each-ref', '--format=%(refname)%00%(upstream)', 'refs/heads'] - requireReachableGitRoute(getConnectionExecutionHostId(connectionId)) - try { - const result = await gitExecFileAsync(args, { - cwd: repoPath, - ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}), - ...(localGitOptions.admissionTier ? { admissionTier: localGitOptions.admissionTier } : {}) - }) - return { - probeFailed: false, - upstreamsByBranchName: parseTrackedUpstreamBranches(result.stdout) - } - } catch { - return { probeFailed: true, upstreamsByBranchName: new Map() } - } -} - -export function parseTrackedUpstreamBranches( - stdout: string -): Map { - const upstreamsByBranchName = new Map() - for (const line of stdout.split(/\r?\n/)) { - if (!line) { - continue - } - const [branchName, upstreamRef] = line.split('\0') - const localBranchName = branchName?.replace(/^refs\/heads\//, '') - if (!localBranchName) { - continue - } - upstreamsByBranchName.set(localBranchName, parseTrackedUpstreamRef(upstreamRef ?? '')) - } - return upstreamsByBranchName -} - -export function parseTrackedUpstreamRef(upstreamRef: string): TrackedUpstreamBranch | null { - const remoteRefPrefix = 'refs/remotes/' - const normalizedRef = upstreamRef.trim() - if (normalizedRef.startsWith(remoteRefPrefix)) { - return parseTrackedUpstreamBranch(normalizedRef.slice(remoteRefPrefix.length)) - } - if (normalizedRef.startsWith('refs/heads/')) { - return null - } - return parseTrackedUpstreamBranch(normalizedRef) -} diff --git a/src/main/github/client/lookup/tracked-upstream-cache.ts b/src/main/github/client/lookup/tracked-upstream-cache.ts deleted file mode 100644 index 167bdad435d3..000000000000 --- a/src/main/github/client/lookup/tracked-upstream-cache.ts +++ /dev/null @@ -1,146 +0,0 @@ -import { splitRemoteBranchName } from '../../../../shared/git-effective-upstream' -import type { OwnerRepo } from '../../gh-utils' -import { readLocalGitConfigSignature } from '../../local-git-config-signature' -import { githubRepoIdentityKey } from '../../../../shared/github/repository-identity-key' -export type TrackedUpstreamBranch = { - remoteName: string - branchName: string -} - -export const TRACKED_UPSTREAM_SNAPSHOT_CACHE_TTL_MS = 30_000 - -export const TRACKED_UPSTREAM_SNAPSHOT_CACHE_MAX_ENTRIES = 512 - -export type TrackedUpstreamSnapshotCacheEntry = { - expiresAt: number - gitConfigSignature?: string - upstreamsByBranchName: Map -} - -export type TrackedUpstreamSnapshotProbeResult = { - cacheable: boolean - gitConfigSignature?: string - probeFailed: boolean - upstreamsByBranchName: Map -} - -export const trackedUpstreamSnapshotCache = new Map() - -export const trackedUpstreamSnapshotInFlight = new Map< - string, - Promise ->() - -export const trackedUpstreamSnapshotGenerations = new Map() - -export function beginTrackedUpstreamSnapshotProbe(cacheKey: string): symbol { - const generation = Symbol() - trackedUpstreamSnapshotGenerations.set(cacheKey, generation) - return generation -} - -export function finishTrackedUpstreamSnapshotProbe(cacheKey: string, generation: symbol): void { - // Why: generations only guard an active probe; retaining completed keys leaks worktree/runtime identities past the snapshot TTL. - if (trackedUpstreamSnapshotGenerations.get(cacheKey) === generation) { - trackedUpstreamSnapshotGenerations.delete(cacheKey) - } -} - -export function pruneTrackedUpstreamSnapshotCache(now: number): void { - for (const [cacheKey, cached] of trackedUpstreamSnapshotCache) { - if (cached.expiresAt <= now) { - trackedUpstreamSnapshotCache.delete(cacheKey) - } - } - // Why: workspace/runtime churn can create unbounded unique keys within one TTL window, so expiry sweeping alone isn't a memory bound. - while (trackedUpstreamSnapshotCache.size > TRACKED_UPSTREAM_SNAPSHOT_CACHE_MAX_ENTRIES) { - const oldestKey = trackedUpstreamSnapshotCache.keys().next().value - if (oldestKey === undefined) { - break - } - trackedUpstreamSnapshotCache.delete(oldestKey) - } -} - -export function _getTrackedUpstreamBranchCacheSizesForTests(): { - snapshots: number - inFlight: number - generations: number -} { - return { - snapshots: trackedUpstreamSnapshotCache.size, - inFlight: trackedUpstreamSnapshotInFlight.size, - generations: trackedUpstreamSnapshotGenerations.size - } -} - -export function __resetTrackedUpstreamBranchCacheForTests(): void { - trackedUpstreamSnapshotCache.clear() - trackedUpstreamSnapshotInFlight.clear() - trackedUpstreamSnapshotGenerations.clear() -} - -export function parseTrackedUpstreamBranch(upstreamRef: string): TrackedUpstreamBranch | null { - const parsed = splitRemoteBranchName(upstreamRef.trim()) - if (!parsed) { - return null - } - return parsed -} - -export function shouldRetryTrackedUpstreamBranch( - upstreamBranch: TrackedUpstreamBranch, - branchName: string, - upstreamHeadRepo: OwnerRepo, - headRepo: OwnerRepo | null -): boolean { - if (upstreamBranch.branchName !== branchName) { - return true - } - if (!headRepo) { - return true - } - return githubRepoIdentityKey(upstreamHeadRepo) !== githubRepoIdentityKey(headRepo) -} - -export function getCacheableTrackedUpstreamSnapshot( - upstreamsByBranchName: Map -): Map { - // Why: SSH/WSL can't cheaply inspect remote .git/config here; the short TTL bounds stale positives while refreshes share one scan. - return upstreamsByBranchName -} - -export function canUseCachedTrackedUpstreamBranch( - cached: TrackedUpstreamSnapshotCacheEntry, - branchName: string -): boolean { - return cached.upstreamsByBranchName.has(branchName) -} - -export async function doesTrackedUpstreamCacheConfigSignatureMatch( - cached: TrackedUpstreamSnapshotCacheEntry, - repoPath: string, - connectionId?: string | null, - localGitOptions: { wslDistro?: string } = {} -): Promise { - if (!cached.gitConfigSignature) { - return true - } - const currentSignature = await readLocalGitConfigSignature({ - repoPath, - connectionId: connectionId ?? null, - ...localGitOptions - }) - return currentSignature === cached.gitConfigSignature -} - -export function getTrackedUpstreamBranchCacheKey( - repoPath: string, - connectionId?: string | null, - localGitOptions: { wslDistro?: string } = {} -): string { - const runtimeKey = connectionId - ? `ssh:${connectionId}` - : `local:${localGitOptions.wslDistro ?? 'host'}` - return [runtimeKey, repoPath].join('\0') -} diff --git a/src/main/github/default-branch-stale-pr.test.ts b/src/main/github/default-branch-stale-pr.test.ts index 803d02d3f72f..2468fdf03ef6 100644 --- a/src/main/github/default-branch-stale-pr.test.ts +++ b/src/main/github/default-branch-stale-pr.test.ts @@ -120,7 +120,7 @@ import { getPRForBranch } from './client/lookup/get-pr-for-branch' import { getPRForBranchOutcome } from './client/lookup/pr-for-branch-outcome' import { _resetOwnerRepoCache } from './gh-utils' import { _resetMergeQueueCacheForTests } from './client/detect/repository-merge-metadata-cache' -import { __resetTrackedUpstreamBranchCacheForTests } from './client/lookup/tracked-upstream-cache' +import { __resetRepoGitConfigSnapshotCacheForTests } from '../source-control/repo-git-config-snapshot' import { __resetPRConflictSummaryCachesForTests } from './conflict-summary' import { resetMergedPRCommitMembershipCacheForTest } from './merged-pr-commit-membership' import { __resetRepoDefaultBranchCacheForTests } from '../source-control/repo-default-branch' @@ -128,7 +128,7 @@ import { __resetRepoDefaultBranchCacheForTests } from '../source-control/repo-de const DEFAULT_BRANCH_REF = 'refs/remotes/origin/master' /** - * Answer the default-base and tracked-upstream snapshots and merged-at-head probe. + * Answer the default-base snapshot and merged-at-head probe. */ function primeGitExecForDefaultBranch({ defaultRef = DEFAULT_BRANCH_REF, @@ -222,7 +222,7 @@ describe('issue #9171: default-branch checkout must not attach a stale non-open acquireMock.mockResolvedValue(undefined) _resetOwnerRepoCache() _resetMergeQueueCacheForTests() - __resetTrackedUpstreamBranchCacheForTests() + __resetRepoGitConfigSnapshotCacheForTests() __resetPRConflictSummaryCachesForTests() resetMergedPRCommitMembershipCacheForTest() __resetRepoDefaultBranchCacheForTests() @@ -236,7 +236,7 @@ describe('issue #9171: default-branch checkout must not attach a stale non-open // The head-branch lookup still runs (state=all) and sees the closed PR… expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - ['api', 'repos/acme/widgets/pulls?head=acme%3Amaster&state=all&per_page=1'], + ['api', 'repos/acme/widgets/pulls?head=acme%3Amaster&state=all&per_page=10'], { cwd: '/repo-root', host: 'github.com' } ) // …but the default-branch guard discards it: no PR on the trunk (#9171). @@ -261,12 +261,12 @@ describe('issue #9171: default-branch checkout must not attach a stale non-open expect(pr?.number).toBe(8) expect(pr?.state).toBe('open') // Open results never consult git for the default branch (lazy resolution). - // Remote-name listing is a separate concern from default-branch resolution, - // so allow it and keep every other git command forbidden here. + // Remote-name listing and the review-head config read are separate concerns + // from default-branch resolution, so allow them and forbid every other git command. expect( gitExecFileAsyncMock.mock.calls .map(([args]) => args[0]) - .filter((command) => command !== 'remote') + .filter((command) => command !== 'remote' && command !== 'config') ).toEqual([]) }) diff --git a/src/main/gitlab/client-mr-branch-lookup.test.ts b/src/main/gitlab/client-mr-branch-lookup.test.ts index b311882da3b8..b8909322efc3 100644 --- a/src/main/gitlab/client-mr-branch-lookup.test.ts +++ b/src/main/gitlab/client-mr-branch-lookup.test.ts @@ -1,6 +1,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type * as GlUtils from './gl-utils' +const { PROJECT_ID } = vi.hoisted(() => ({ PROJECT_ID: 42 })) + const { glabExecFileAsyncMock, glabApiWithHeadersMock, @@ -27,6 +29,8 @@ vi.mock('../git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) +vi.mock('./gitlab-project-id', () => ({ getGitLabProjectId: vi.fn(async () => PROJECT_ID) })) + vi.mock('./gl-utils', async () => { const actual = await vi.importActual('./gl-utils') return { @@ -46,7 +50,7 @@ import { getMergeRequestForBranch, getMergeRequestForBranchOrThrow } from './client' -import { resetGitLabMrMocks } from './client-mr-test-harness' +import { primeGitConfig, resetGitLabMrMocks } from './client-mr-test-harness' describe('gitlab client — MR operations', () => { beforeEach(() => { @@ -145,6 +149,7 @@ describe('gitlab client — MR operations', () => { stdout: JSON.stringify([ { iid: 7, + source_project_id: PROJECT_ID, title: 'WIP', state: 'merged', sha: 'abc', @@ -160,7 +165,7 @@ describe('gitlab client — MR operations', () => { expect(glabExecFileAsyncMock).toHaveBeenCalledWith( [ 'api', - 'projects/g%2Fp/merge_requests?source_branch=feature%2Ffoo&order_by=updated_at&sort=desc&per_page=1&with_merge_status_recheck=true' + 'projects/g%2Fp/merge_requests?source_project_id=42&source_branch=feature%2Ffoo&order_by=updated_at&sort=desc&per_page=10&with_merge_status_recheck=true' ], { cwd: '/repo' } ) @@ -184,6 +189,7 @@ describe('gitlab client — MR operations', () => { stdout: JSON.stringify([ { iid: 8, + source_project_id: PROJECT_ID, title: 'Legacy pipeline branch', state: 'opened', sha: 'def', @@ -217,6 +223,7 @@ describe('gitlab client — MR operations', () => { glabExecFileAsyncMock.mockResolvedValueOnce({ stdout: JSON.stringify({ iid: 9, + source_project_id: PROJECT_ID, title: 'Linked MR', state: 'opened', pipeline: { status: 'success' } @@ -238,6 +245,7 @@ describe('gitlab client — MR operations', () => { glabExecFileAsyncMock.mockResolvedValueOnce({ stdout: JSON.stringify({ iid: 2, + source_project_id: PROJECT_ID, title: 'Replacement linked MR', state: 'opened', sha: 'head-2', @@ -264,6 +272,7 @@ describe('gitlab client — MR operations', () => { glabExecFileAsyncMock.mockResolvedValueOnce({ stdout: JSON.stringify({ iid: 2, + source_project_id: PROJECT_ID, title: 'Already selected MR', state: 'opened', sha: 'head-2', @@ -286,6 +295,7 @@ describe('gitlab client — MR operations', () => { glabExecFileAsyncMock.mockResolvedValueOnce({ stdout: JSON.stringify({ iid: 10, + source_project_id: PROJECT_ID, title: 'Merged linked MR', state: 'merged', pipeline: { status: 'success' } @@ -307,6 +317,7 @@ describe('gitlab client — MR operations', () => { stdout: JSON.stringify([ { iid: 12, + source_project_id: PROJECT_ID, title: 'WSL branch', state: 'opened', sha: 'abc', @@ -335,6 +346,7 @@ describe('gitlab client — MR operations', () => { stdout: JSON.stringify([ { iid: 7, + source_project_id: PROJECT_ID, title: 'Accidental MR from main', state: 'closed', sha: 'stale-main-oid', @@ -352,6 +364,7 @@ describe('gitlab client — MR operations', () => { stdout: JSON.stringify([ { iid: 9, + source_project_id: PROJECT_ID, title: 'Wedged mid-merge MR from main', state: 'locked', sha: 'locked-main-oid', @@ -369,6 +382,7 @@ describe('gitlab client — MR operations', () => { stdout: JSON.stringify([ { iid: 8, + source_project_id: PROJECT_ID, title: 'main → release', state: 'opened', sha: 'abc', @@ -379,8 +393,9 @@ describe('gitlab client — MR operations', () => { const mr = await getMergeRequestForBranch('/repo', 'main') expect(mr?.number).toBe(8) - // Open results never consult git for the default branch (lazy resolution). - expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + // Open results never consult git for the default branch (lazy resolution); the + // review-head config read is the only git call. + expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args[0])).toEqual(['config']) }) it('keeps a closed MR on a feature branch (behavior preserved)', async () => { @@ -389,6 +404,7 @@ describe('gitlab client — MR operations', () => { stdout: JSON.stringify([ { iid: 9, + source_project_id: PROJECT_ID, title: 'Rejected work', state: 'closed', sha: 'def', @@ -407,6 +423,7 @@ describe('gitlab client — MR operations', () => { glabExecFileAsyncMock.mockResolvedValueOnce({ stdout: JSON.stringify({ iid: 42, + source_project_id: PROJECT_ID, title: 'Linked MR', state: 'merged', pipeline: { status: 'success' } @@ -429,6 +446,7 @@ describe('gitlab client — MR operations', () => { glabExecFileAsyncMock.mockResolvedValueOnce({ stdout: JSON.stringify({ iid: 7, + source_project_id: PROJECT_ID, title: 'Linked trunk MR', state: 'merged', sha: 'abc', @@ -442,6 +460,74 @@ describe('gitlab client — MR operations', () => { expect(glabExecFileAsyncMock).toHaveBeenCalledOnce() }) + it('shows the open MR over a newer closed one from the same branch (#28324)', async () => { + getProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'g/p' }) + glabExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([ + { iid: 9, source_project_id: PROJECT_ID, title: 'Retry', state: 'closed' }, + { iid: 3, source_project_id: PROJECT_ID, title: 'Feature', state: 'opened' } + ]) + }) + + await expect(getMergeRequestForBranch('/repo', 'feature/foo')).resolves.toMatchObject({ + number: 3, + state: 'opened' + }) + }) + + it('asks for open MRs only when a full page of closed ones could hide one', async () => { + getProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'g/p' }) + const closed = Array.from({ length: 10 }, (_, index) => ({ + iid: 20 + index, + source_project_id: PROJECT_ID, + title: 'Retry', + state: 'closed' + })) + glabExecFileAsyncMock + .mockResolvedValueOnce({ stdout: JSON.stringify(closed) }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { iid: 3, source_project_id: PROJECT_ID, title: 'Feature', state: 'opened' } + ]) + }) + + await expect(getMergeRequestForBranch('/repo', 'feature/foo')).resolves.toMatchObject({ + number: 3 + }) + expect(glabExecFileAsyncMock.mock.calls[1][0][1]).toContain( + 'source_branch=feature%2Ffoo&state=opened&' + ) + }) + + it('ignores a same-named branch from another project', async () => { + getProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'g/p' }) + glabExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([ + { iid: 5, source_project_id: PROJECT_ID + 1, title: 'Fork MR', state: 'opened' } + ]) + }) + + await expect(getMergeRequestForBranch('/repo', 'feature/foo')).resolves.toBeNull() + }) + + it('follows a `glab mr checkout` branch to its MR', async () => { + primeGitConfig(gitExecFileAsyncMock, [ + ['branch.mr-9.remote', 'origin'], + ['branch.mr-9.merge', 'refs/merge-requests/9/head'] + ]) + getProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'g/p' }) + glabExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ iid: 9, source_project_id: 77, title: 'Fork MR', state: 'opened' }) + }) + + await expect(getMergeRequestForBranch('/repo', 'mr-9')).resolves.toMatchObject({ number: 9 }) + expect(glabExecFileAsyncMock).toHaveBeenCalledOnce() + expect(glabExecFileAsyncMock).toHaveBeenCalledWith( + ['api', 'projects/g%2Fp/merge_requests/9?with_merge_status_recheck=true'], + { cwd: '/repo' } + ) + }) + it('returns null for an empty / detached-HEAD branch arg', async () => { // Why: during a rebase the branch is empty — mirror github/getPRForBranch's // early return without calling glab. diff --git a/src/main/gitlab/client-mr-test-harness.ts b/src/main/gitlab/client-mr-test-harness.ts index bcf40affb096..c264d163fa15 100644 --- a/src/main/gitlab/client-mr-test-harness.ts +++ b/src/main/gitlab/client-mr-test-harness.ts @@ -1,6 +1,7 @@ import type { Mock } from 'vitest' import { _resetGitLabRateLimitCache } from './client' import { __resetRepoDefaultBranchCacheForTests } from '../source-control/repo-default-branch' +import { __resetRepoGitConfigSnapshotCacheForTests } from '../source-control/repo-git-config-snapshot' import { _resetKnownHostsCache } from './gitlab-known-host-probe' /** The hoisted gl-utils / git-runner mocks every GitLab MR test file installs. */ @@ -28,6 +29,17 @@ export function primeGitDefaultBranch( }) } +/** Answer `git config --list -z` with these records, on top of the default-branch probes. */ +export function primeGitConfig(gitExecFileAsyncMock: Mock, records: [string, string][]): void { + primeGitDefaultBranch(gitExecFileAsyncMock) + const defaultBranchProbe = gitExecFileAsyncMock.getMockImplementation() + gitExecFileAsyncMock.mockImplementation(async (args: string[], options: unknown) => + args[0] === 'config' && args[1] === '--list' + ? { stdout: records.map(([key, value]) => `${key}\n${value}`).join('\0'), stderr: '' } + : defaultBranchProbe?.(args, options) + ) +} + /** Shared per-test reset: mocks, module caches, and the default host/source stubs. */ export function resetGitLabMrMocks(mocks: GitLabMrMocks): void { mocks.glabExecFileAsyncMock.mockReset() @@ -41,6 +53,7 @@ export function resetGitLabMrMocks(mocks: GitLabMrMocks): void { mocks.gitExecFileAsyncMock.mockReset() primeGitDefaultBranch(mocks.gitExecFileAsyncMock) __resetRepoDefaultBranchCacheForTests() + __resetRepoGitConfigSnapshotCacheForTests() _resetKnownHostsCache() _resetGitLabRateLimitCache() mocks.getGlabKnownHostsMock.mockResolvedValue(['gitlab.com']) diff --git a/src/main/gitlab/gitlab-project-id.ts b/src/main/gitlab/gitlab-project-id.ts new file mode 100644 index 000000000000..e253250219d4 --- /dev/null +++ b/src/main/gitlab/gitlab-project-id.ts @@ -0,0 +1,47 @@ +import { + glabExecFileAsync, + glabHostnameArgs, + glabRepoExecOptions, + type LocalGitExecOptions, + type ProjectRef +} from './gl-utils' +import { encodedProject } from './project-path-encoding' + +const PROJECT_ID_CACHE_MAX_ENTRIES = 256 + +// Why: a project's numeric id never changes for its path, and MR lists filter heads by it. +const projectIds = new Map() + +/** The numeric id GitLab filters merge requests by (`source_project_id`), or null if unknown. */ +export async function getGitLabProjectId( + project: ProjectRef, + repoPath: string, + connectionId: string | null | undefined, + localGitOptions: LocalGitExecOptions +): Promise { + const key = `${project.host}\0${project.path}` + const cached = projectIds.get(key) + if (cached !== undefined) { + return cached + } + const { stdout } = await glabExecFileAsync( + ['api', ...glabHostnameArgs(project, connectionId), `projects/${encodedProject(project.path)}`], + glabRepoExecOptions(repoPath, connectionId, localGitOptions) + ) + const parsed: { id?: unknown } = JSON.parse(stdout) + if (typeof parsed.id !== 'number') { + return null + } + projectIds.set(key, parsed.id) + if (projectIds.size > PROJECT_ID_CACHE_MAX_ENTRIES) { + const oldest = projectIds.keys().next().value + if (oldest !== undefined) { + projectIds.delete(oldest) + } + } + return parsed.id +} + +export function _resetGitLabProjectIdCacheForTests(): void { + projectIds.clear() +} diff --git a/src/main/gitlab/mappers.ts b/src/main/gitlab/mappers.ts index 645392ef2cbf..3012e813ee5d 100644 --- a/src/main/gitlab/mappers.ts +++ b/src/main/gitlab/mappers.ts @@ -96,6 +96,8 @@ type GitLabMRRaw = { merge_status?: string description?: string | null target_branch?: string + source_project_id?: number + target_project_id?: number author?: { username?: string | null; avatar_url?: string | null } | null } diff --git a/src/main/gitlab/merge-request-lookup.ts b/src/main/gitlab/merge-request-lookup.ts index 96b6f7bad28d..4908adbaaca1 100644 --- a/src/main/gitlab/merge-request-lookup.ts +++ b/src/main/gitlab/merge-request-lookup.ts @@ -4,6 +4,7 @@ import { acquire, getGlabKnownHosts, getProjectRef, + getProjectRefForRemote, glabHostnameArgs, glabRepoExecOptions, glabExecFileAsync, @@ -17,9 +18,20 @@ import { type HostedReviewExecutionOptions } from '../source-control/hosted-review-git-options' import { shouldHideNonOpenReviewOnDefaultBranch } from '../source-control/repo-default-branch' +import { + BRANCH_REVIEW_PAGE_SIZE, + selectReviewForHead +} from '../source-control/branch-review-selection' +import { resolveBranchReviewTarget } from '../source-control/review-head' +import { getGitLabProjectId } from './gitlab-project-id' type HostedReviewLocalGitOptions = ReturnType +type ListedMergeRequest = Parameters[0] & { + head_pipeline?: { status?: string } | null + pipeline?: { status?: string } | null +} + function hostedReviewLocalGitOptionArgs( options: HostedReviewExecutionOptions = {} ): [] | [HostedReviewLocalGitOptions] { @@ -78,8 +90,8 @@ export async function getMergeRequest( } /** - * Find the explicitly linked merge request, or the newest MR whose source branch matches. - * Returns null when neither exists. + * Find the explicitly linked merge request, or the MR whose source is where this branch is pushed + * (the open one first, else the newest). Returns null when neither exists. */ export async function getMergeRequestForBranch( repoPath: string, @@ -101,14 +113,32 @@ export async function getMergeRequestForBranch( if (!projectRef) { return null } + const { linkedNumber: linkedIid, head: branchHead } = await resolveBranchReviewTarget( + repoPath, + branchName, + linkedMRIid, + connectionId, + localGitOptions + ) + const headProject = !branchHead + ? null + : branchHead.remote === 'origin' + ? projectRef + : await getProjectRefForRemote( + repoPath, + branchHead.remote, + knownHosts, + connectionId, + localGitOptions + ) await acquire() try { - if (typeof linkedMRIid === 'number') { + if (typeof linkedIid === 'number') { const { stdout } = await glabExecFileAsync( [ 'api', ...glabHostnameArgs(projectRef, connectionId), - `projects/${encodedProject(projectRef.path)}/merge_requests/${linkedMRIid}?with_merge_status_recheck=true` + `projects/${encodedProject(projectRef.path)}/merge_requests/${linkedIid}?with_merge_status_recheck=true` ], glabRepoExecOptions(repoPath, connectionId, localGitOptions) ) @@ -119,25 +149,39 @@ export async function getMergeRequestForBranch( const pipelineStatus = derivePipelineStatus(raw.head_pipeline ?? raw.pipeline ?? null) return mapMRInfo(raw, pipelineStatus) } - if (branchName) { - const { stdout } = await glabExecFileAsync( - [ - 'api', - ...glabHostnameArgs(projectRef, connectionId), - // Why: GitLab does not proactively recompute merge status on list endpoints, so this row - // can sit at `unchecked` forever — and the sidebar merge button gates on MERGEABLE. Ask - // for the async recalculation (best-effort; ignored for non-Developers when - // `restrict_merge_status_recheck` is on) so polling converges instead of stalling. - `projects/${encodedProject(projectRef.path)}/merge_requests?source_branch=${encodeURIComponent(branchName)}&order_by=updated_at&sort=desc&per_page=1&with_merge_status_recheck=true` - ], - glabRepoExecOptions(repoPath, connectionId, localGitOptions) + if (branchHead && headProject && headProject.host === projectRef.host) { + const headProjectId = await getGitLabProjectId( + headProject, + repoPath, + connectionId, + localGitOptions ) - const data = JSON.parse(stdout) as (Parameters[0] & { - head_pipeline?: { status?: string } | null - pipeline?: { status?: string } | null - })[] - if (Array.isArray(data) && data.length > 0) { - const raw = data[0] + if (headProjectId === null) { + return null + } + const raw = await selectReviewForHead( + async (states) => { + const { stdout } = await glabExecFileAsync( + [ + 'api', + ...glabHostnameArgs(projectRef, connectionId), + // Why: GitLab does not proactively recompute merge status on list endpoints, so this row + // can sit at `unchecked` forever — and the sidebar merge button gates on MERGEABLE. Ask + // for the async recalculation (best-effort; ignored for non-Developers when + // `restrict_merge_status_recheck` is on) so polling converges instead of stalling. + `projects/${encodedProject(projectRef.path)}/merge_requests?source_project_id=${headProjectId}&source_branch=${encodeURIComponent(branchHead.branch)}${states === 'open' ? '&state=opened' : ''}&order_by=updated_at&sort=desc&per_page=${BRANCH_REVIEW_PAGE_SIZE}&with_merge_status_recheck=true` + ], + glabRepoExecOptions(repoPath, connectionId, localGitOptions) + ) + const data: ListedMergeRequest[] = JSON.parse(stdout) + // Why: servers older than 11.10 ignore `source_project_id`. + return Array.isArray(data) + ? data.filter((mr) => mr.source_project_id === headProjectId) + : [] + }, + (mr) => mr.state?.toLowerCase() === 'opened' + ) + if (raw) { // Why: older GitLab list payloads expose `pipeline` instead of `head_pipeline`. const pipelineStatus = derivePipelineStatus(raw.head_pipeline ?? raw.pipeline ?? null) const info = mapMRInfo(raw, pipelineStatus) diff --git a/src/main/source-control/branch-review-selection.ts b/src/main/source-control/branch-review-selection.ts new file mode 100644 index 000000000000..21157b698cae --- /dev/null +++ b/src/main/source-control/branch-review-selection.ts @@ -0,0 +1,28 @@ +// Reviews one head-scoped request asks for. A head rarely has more; when it does, see below. +export const BRANCH_REVIEW_PAGE_SIZE = 10 + +/** + * The review for one head, from reviews filtered to exactly that head and newest first: the open + * one if any (a head has at most one open review per base), else the most recent (#28324: a newer + * closed review must not hide the open one). + */ +export function pickReviewForHead( + reviews: readonly T[], + isOpen: (review: T) => boolean +): T | undefined { + return reviews.find(isOpen) ?? reviews[0] +} + +/** `pickReviewForHead` over a provider list that returns one page at a time. */ +export async function selectReviewForHead( + fetchReviews: (states: 'all' | 'open') => Promise, + isOpen: (review: T) => boolean +): Promise { + const recent = await fetchReviews('all') + if (recent.length < BRANCH_REVIEW_PAGE_SIZE || recent.some(isOpen)) { + return pickReviewForHead(recent, isOpen) + } + // Why: a full page of closed reviews can hide an older open one; ask only then, so ordinary + // heads stay at one request. + return (await fetchReviews('open')).find(isOpen) ?? recent[0] +} diff --git a/src/main/source-control/hosted-review-bitbucket.integration.test.ts b/src/main/source-control/hosted-review-bitbucket.integration.test.ts index 4dc26fe3892e..76bed221651a 100644 --- a/src/main/source-control/hosted-review-bitbucket.integration.test.ts +++ b/src/main/source-control/hosted-review-bitbucket.integration.test.ts @@ -62,7 +62,8 @@ describe('Bitbucket hosted review integration', () => { }, source: { branch: { name: 'feature/bitbucket' }, - commit: { hash: 'abc123' } + commit: { hash: 'abc123' }, + repository: { full_name: 'team/repo' } } } ] @@ -117,7 +118,9 @@ describe('Bitbucket hosted review integration', () => { ]) expect(seen.every((request) => request.authorization === 'Bearer local-token')).toBe(true) const query = new URLSearchParams(seen[0].search) - expect(query.get('q')).toContain('source.branch.name = "feature/bitbucket"') + expect(query.get('q')).toContain( + 'source.repository.full_name = "team/repo" AND source.branch.name = "feature/bitbucket"' + ) expect(query.getAll('state')).toEqual(['OPEN', 'MERGED', 'DECLINED', 'SUPERSEDED']) } finally { await rm(repoPath, { recursive: true, force: true }) @@ -148,7 +151,11 @@ describe('Bitbucket hosted review integration', () => { state: 'OPEN', updated_on: '2026-05-15T00:00:00Z', links: { html: { href: 'https://bitbucket.org/team/repo/pull-requests/13' } }, - source: { branch: { name: 'feature/recovery' }, commit: { hash: 'def456' } } + source: { + branch: { name: 'feature/recovery' }, + commit: { hash: 'def456' }, + repository: { full_name: 'team/repo' } + } } ] }) diff --git a/src/main/source-control/hosted-review-creation-eligibility.test.ts b/src/main/source-control/hosted-review-creation-eligibility.test.ts index 294390b9d1fe..fa680ed4d00b 100644 --- a/src/main/source-control/hosted-review-creation-eligibility.test.ts +++ b/src/main/source-control/hosted-review-creation-eligibility.test.ts @@ -469,8 +469,13 @@ describe('getHostedReviewCreationEligibility', () => { expect(gitExecFileAsyncMock).not.toHaveBeenCalled() }) - it('reports reviewLookupOutcome: found when an existing review is returned', async () => { - getHostedReviewForBranchMock.mockResolvedValue({ number: 7, url: 'https://x/pull/7' }) + it.each([ + ['open', 'existing_review'], + ['draft', 'existing_review'], + ['merged', null], + ['closed', null] + ])('blocks creation only on an open review (found %s)', async (state, blockedReason) => { + getHostedReviewForBranchMock.mockResolvedValue({ number: 7, url: 'https://x/pull/7', state }) await expect( getHostedReviewCreationEligibility({ ...LOCAL_REPO_ARGS, @@ -480,7 +485,7 @@ describe('getHostedReviewCreationEligibility', () => { ahead: 0, behind: 0 }) - ).resolves.toMatchObject({ blockedReason: 'existing_review', reviewLookupOutcome: 'found' }) + ).resolves.toMatchObject({ blockedReason, reviewLookupOutcome: 'found' }) }) it('reports reviewLookupOutcome: not_found when the lookup accepts no review', async () => { diff --git a/src/main/source-control/hosted-review-creation.ts b/src/main/source-control/hosted-review-creation.ts index 43309a54a2a9..c500b169f1d0 100644 --- a/src/main/source-control/hosted-review-creation.ts +++ b/src/main/source-control/hosted-review-creation.ts @@ -172,7 +172,8 @@ export async function getHostedReviewCreationEligibility( if (!branch || branch === 'HEAD') { return { ...baseResult, canCreate: false, blockedReason: 'detached_head', nextAction: null } } - if (review) { + // Why: providers accept a new review from a head whose earlier reviews were merged or closed. + if (review?.state === 'open' || review?.state === 'draft') { return { ...baseResult, canCreate: false, diff --git a/src/main/source-control/hosted-review-gitea.integration.test.ts b/src/main/source-control/hosted-review-gitea.integration.test.ts index b83e2fd5f340..bb43a1fbf60d 100644 --- a/src/main/source-control/hosted-review-gitea.integration.test.ts +++ b/src/main/source-control/hosted-review-gitea.integration.test.ts @@ -53,7 +53,12 @@ describe('Gitea hosted review integration', () => { html_url: 'http://127.0.0.1/team/repo/pulls/9', updated_at: '2026-05-15T00:00:00Z', mergeable: true, - head: { ref: 'feature/gitea', label: 'team:feature/gitea', sha: 'abc123' } + head: { + ref: 'feature/gitea', + label: 'feature/gitea', + sha: 'abc123', + repo: { full_name: 'team/repo' } + } } ]) return diff --git a/src/main/source-control/repo-git-config-snapshot.test.ts b/src/main/source-control/repo-git-config-snapshot.test.ts new file mode 100644 index 000000000000..df0bb1cc4547 --- /dev/null +++ b/src/main/source-control/repo-git-config-snapshot.test.ts @@ -0,0 +1,72 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { readGitConfigSnapshotValue } from '../../shared/git-config-snapshot-runner' +import { + __resetRepoGitConfigSnapshotCacheForTests, + getRepoGitConfigSnapshot +} from './repo-git-config-snapshot' + +const { gitExecFileAsyncMock, readLocalGitConfigSignatureMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + readLocalGitConfigSignatureMock: vi.fn() +})) + +vi.mock('../git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) +vi.mock('../github/local-git-config-signature', () => ({ + readLocalGitConfigSignature: readLocalGitConfigSignatureMock +})) + +function configOutput(pushRemote: string): { stdout: string; stderr: string } { + return { stdout: `branch.feature.pushremote\n${pushRemote}\0`, stderr: '' } +} + +async function readPushRemote(): Promise { + const snapshot = await getRepoGitConfigSnapshot('/repo') + return snapshot ? readGitConfigSnapshotValue(snapshot, 'branch.feature.pushRemote') : null +} + +describe('getRepoGitConfigSnapshot', () => { + beforeEach(() => { + __resetRepoGitConfigSnapshotCacheForTests() + gitExecFileAsyncMock.mockReset() + readLocalGitConfigSignatureMock.mockReset() + readLocalGitConfigSignatureMock.mockResolvedValue('sig-1') + }) + + it('shares one read across lookups while the config is unchanged', async () => { + gitExecFileAsyncMock.mockResolvedValue(configOutput('fork')) + + await expect(Promise.all([readPushRemote(), readPushRemote()])).resolves.toEqual([ + 'fork', + 'fork' + ]) + await expect(readPushRemote()).resolves.toBe('fork') + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['config', '--list', '-z'], { + cwd: '/repo' + }) + }) + + it('re-reads as soon as the config files change', async () => { + gitExecFileAsyncMock + .mockResolvedValueOnce(configOutput('fork')) + .mockResolvedValueOnce(configOutput('alice')) + + await expect(readPushRemote()).resolves.toBe('fork') + readLocalGitConfigSignatureMock.mockResolvedValue('sig-2') + await expect(readPushRemote()).resolves.toBe('alice') + }) + + it('returns null for a failed read and tries again next time', async () => { + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('fatal: not a git repository')) + .mockResolvedValueOnce(configOutput('fork')) + + await expect(getRepoGitConfigSnapshot('/repo')).resolves.toBeNull() + await expect(readPushRemote()).resolves.toBe('fork') + }) + + it('throws for a host whose git does not run in this process', async () => { + await expect(getRepoGitConfigSnapshot('/repo', 'ssh-1')).rejects.toThrow() + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/source-control/repo-git-config-snapshot.ts b/src/main/source-control/repo-git-config-snapshot.ts new file mode 100644 index 000000000000..97650b9c5a23 --- /dev/null +++ b/src/main/source-control/repo-git-config-snapshot.ts @@ -0,0 +1,108 @@ +import { getConnectionExecutionHostId } from '../../shared/execution-host' +import { + parseGitConfigListSnapshot, + type GitConfigSnapshot +} from '../../shared/git-config-snapshot-runner' +import { runCoalescedProbe, type CoalescedProbes } from '../git/coalesced-probe' +import type { GitAdmissionTier } from '../git/command-runner/git-exec-options' +import { gitExecFileAsync } from '../git/runner' +import { readLocalGitConfigSignature } from '../github/local-git-config-signature' +import { requireReachableGitRoute } from '../providers/execution-host-provider-dispatch' + +const REPO_GIT_CONFIG_SNAPSHOT_TTL_MS = 30_000 +const REPO_GIT_CONFIG_SNAPSHOT_MAX_ENTRIES = 512 + +export type RepoGitConfigOptions = { wslDistro?: string; admissionTier?: GitAdmissionTier } + +type RepoGitConfigSnapshotEntry = { + expiresAt: number + gitConfigSignature?: string + snapshot: GitConfigSnapshot +} + +const snapshots = new Map() +const inFlight: CoalescedProbes = new Map() + +function snapshotKey( + repoPath: string, + connectionId: string | null | undefined, + options: RepoGitConfigOptions +): string { + const runtimeKey = connectionId ? `ssh:${connectionId}` : `local:${options.wslDistro ?? 'host'}` + return `${runtimeKey}\0${repoPath}` +} + +function pruneSnapshots(now: number): void { + for (const [key, entry] of snapshots) { + if (entry.expiresAt <= now) { + snapshots.delete(key) + } + } + // Why: workspace/runtime churn can mint unbounded keys inside one TTL window. + while (snapshots.size > REPO_GIT_CONFIG_SNAPSHOT_MAX_ENTRIES) { + const oldest = snapshots.keys().next().value + if (oldest === undefined) { + break + } + snapshots.delete(oldest) + } +} + +/** + * The repo's git config, read with one `git config --list -z` per repo and runtime and shared by + * every branch lookup for 30s. Null when the read fails, so callers fall back to defaults instead + * of caching an empty config; throws when the host's git is unreachable from here. + */ +export async function getRepoGitConfigSnapshot( + repoPath: string, + connectionId?: string | null, + options: RepoGitConfigOptions = {} +): Promise { + const key = snapshotKey(repoPath, connectionId, options) + const signatureContext = { + repoPath, + connectionId: connectionId ?? null, + ...(options.wslDistro ? { wslDistro: options.wslDistro } : {}) + } + const cached = snapshots.get(key) + if (cached && cached.expiresAt > Date.now()) { + if ( + !cached.gitConfigSignature || + cached.gitConfigSignature === (await readLocalGitConfigSignature(signatureContext)) + ) { + return cached.snapshot + } + } + snapshots.delete(key) + // Why: an unreachable host is not "no config" — throwing keeps the lookup unavailable rather + // than guessing a head that could attach the wrong review. + requireReachableGitRoute(getConnectionExecutionHostId(connectionId)) + return runCoalescedProbe(inFlight, key, async (ownsKey) => { + try { + const signatureBefore = await readLocalGitConfigSignature(signatureContext) + const { stdout } = await gitExecFileAsync(['config', '--list', '-z'], { + cwd: repoPath, + ...(options.wslDistro ? { wslDistro: options.wslDistro } : {}), + ...(options.admissionTier ? { admissionTier: options.admissionTier } : {}) + }) + const snapshot = parseGitConfigListSnapshot(stdout) + // Why: config written mid-read may be half-seen; serve it once but don't cache it. + if (ownsKey() && signatureBefore === (await readLocalGitConfigSignature(signatureContext))) { + snapshots.set(key, { + snapshot, + expiresAt: Date.now() + REPO_GIT_CONFIG_SNAPSHOT_TTL_MS, + ...(signatureBefore ? { gitConfigSignature: signatureBefore } : {}) + }) + pruneSnapshots(Date.now()) + } + return snapshot + } catch { + return null + } + }) +} + +export function __resetRepoGitConfigSnapshotCacheForTests(): void { + snapshots.clear() + inFlight.clear() +} diff --git a/src/main/source-control/review-head.ts b/src/main/source-control/review-head.ts new file mode 100644 index 000000000000..ed181fdb305e --- /dev/null +++ b/src/main/source-control/review-head.ts @@ -0,0 +1,28 @@ +import { readReviewHead, type ReviewHead } from '../../shared/git-review-head' +import { getRepoGitConfigSnapshot, type RepoGitConfigOptions } from './repo-git-config-snapshot' + +export type BranchReviewHead = Extract + +/** + * What a branch lookup asks the provider for. An explicit link wins without reading git; a branch + * checked out from a review links itself; any other branch is matched by where it is pushed + * (#27811, #26948). `head` is null for a detached HEAD. + */ +export async function resolveBranchReviewTarget( + repoPath: string, + branch: string, + linkedNumber: number | null | undefined, + connectionId?: string | null, + options: RepoGitConfigOptions = {} +): Promise<{ linkedNumber: number | null; head: BranchReviewHead | null }> { + const branchName = branch.replace(/^refs\/heads\//, '') + if (typeof linkedNumber === 'number' || !branchName) { + return { linkedNumber: linkedNumber ?? null, head: null } + } + const config = await getRepoGitConfigSnapshot(repoPath, connectionId, options) + // Why: an unreadable config defaults to `origin/` rather than hiding the review. + const head = readReviewHead(config ?? new Map(), branchName) + return head.kind === 'review' + ? { linkedNumber: head.number, head: null } + : { linkedNumber: null, head } +} diff --git a/src/shared/git-config-snapshot-runner.ts b/src/shared/git-config-snapshot-runner.ts index 2f3e56a7bf60..16c4735ced2b 100644 --- a/src/shared/git-config-snapshot-runner.ts +++ b/src/shared/git-config-snapshot-runner.ts @@ -1,6 +1,6 @@ type GitCommandRunner = (args: string[]) => Promise<{ stdout: string }> -type GitConfigSnapshot = Map +export type GitConfigSnapshot = Map // Why: mirror `git config --get`'s exit-1-on-absent-key so an intercepted miss // rejects (matching real git) instead of resolving an empty success value. @@ -25,7 +25,7 @@ function canonicalizeGitConfigLookupKey(key: string): string { return [firstPart, ...parts.slice(1, -1), lastPart].join('.') } -function parseGitConfigListSnapshot(stdout: string): GitConfigSnapshot { +export function parseGitConfigListSnapshot(stdout: string): GitConfigSnapshot { const snapshot: GitConfigSnapshot = new Map() for (const record of iterateNulDelimitedFields(stdout)) { if (!record.trim()) { @@ -44,6 +44,15 @@ function parseGitConfigListSnapshot(stdout: string): GitConfigSnapshot { return snapshot } +/** The value `git config --get ` would print from this snapshot (last occurrence), or null. */ +export function readGitConfigSnapshotValue( + snapshot: GitConfigSnapshot, + key: string +): string | null { + const value = snapshot.get(canonicalizeGitConfigLookupKey(key))?.at(-1)?.trim() + return value ? value : null +} + export function createGitConfigSnapshotRunner(runGit: GitCommandRunner): GitCommandRunner { let snapshotPromise: Promise | null = null let snapshot: GitConfigSnapshot | null = null diff --git a/src/shared/git-review-head.test.ts b/src/shared/git-review-head.test.ts new file mode 100644 index 000000000000..9dbac2145ff8 --- /dev/null +++ b/src/shared/git-review-head.test.ts @@ -0,0 +1,93 @@ +import { describe, expect, it } from 'vitest' +import { parseGitConfigListSnapshot } from './git-config-snapshot-runner' +import { readReviewHead } from './git-review-head' + +// Captured from `git config --list -z` (git 2.54) in a scratch repo with these branches, keeping only +// the remote/branch/push records. `git push --dry-run` there sent `own` to origin, `tri` to fork and +// `url-remote` to fork, and refused `tracks-develop`, `tracks-upstream-develop`, `feature-x-2`, `pr-7`. +const CAPTURED_CONFIG: [string, string][] = [ + ['remote.origin.url', '../upstream.git'], + ['remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'], + ['remote.upstream.url', '../upstream.git'], + ['remote.upstream.fetch', '+refs/heads/*:refs/remotes/upstream/*'], + ['remote.fork.url', '../fork.git'], + ['remote.fork.fetch', '+refs/heads/*:refs/remotes/fork/*'], + ['remote.alice.url', '../alice.git'], + ['remote.alice.fetch', '+refs/heads/*:refs/remotes/alice/*'], + ['branch.own.remote', 'origin'], + ['branch.own.merge', 'refs/heads/own'], + ['branch.tracks-develop.remote', 'origin'], + ['branch.tracks-develop.merge', 'refs/heads/develop'], + ['branch.tracks-upstream-develop.remote', 'upstream'], + ['branch.tracks-upstream-develop.merge', 'refs/heads/develop'], + ['branch.tri.remote', 'origin'], + ['branch.tri.merge', 'refs/heads/main'], + ['branch.tri.pushremote', 'fork'], + ['branch.feature-x-2.remote', 'alice'], + ['branch.feature-x-2.merge', 'refs/heads/feature-x'], + ['branch.pr-7.remote', 'origin'], + ['branch.pr-7.merge', 'refs/pull/7/head'], + ['branch.url-remote.remote', '../fork.git'], + ['branch.url-remote.merge', 'refs/heads/url-remote'] +] + +function configWith(extra: [string, string][] = []) { + return parseGitConfigListSnapshot( + [...CAPTURED_CONFIG, ...extra].map(([key, value]) => `${key}\n${value}`).join('\0') + ) +} + +describe('readReviewHead', () => { + it.each([ + ['own', { kind: 'branch', remote: 'origin', branch: 'own' }], + // #26948: tracking the base branch must not make develop's PR this branch's. + ['tracks-develop', { kind: 'branch', remote: 'origin', branch: 'tracks-develop' }], + [ + 'tracks-upstream-develop', + { kind: 'branch', remote: 'upstream', branch: 'tracks-upstream-develop' } + ], + ['tri', { kind: 'branch', remote: 'fork', branch: 'tri' }], + // A contributor fork's branch checked out under another local name, as create-from-PR does. + ['feature-x-2', { kind: 'branch', remote: 'alice', branch: 'feature-x' }], + ['pr-7', { kind: 'review', number: 7 }], + ['url-remote', { kind: 'branch', remote: 'fork', branch: 'url-remote' }], + ['never-pushed', { kind: 'branch', remote: 'origin', branch: 'never-pushed' }] + ])('follows where %s is pushed', (branch, head) => { + expect(readReviewHead(configWith(), branch)).toEqual(head) + }) + + it('follows the tracked name when push.default pushes to the upstream branch', () => { + const config = configWith([['push.default', 'upstream']]) + expect(readReviewHead(config, 'tracks-develop')).toEqual({ + kind: 'branch', + remote: 'origin', + branch: 'develop' + }) + }) + + it('sends every branch without its own push remote to remote.pushDefault', () => { + const config = configWith([['remote.pushdefault', 'fork']]) + expect(readReviewHead(config, 'own')).toEqual({ kind: 'branch', remote: 'fork', branch: 'own' }) + expect(readReviewHead(config, 'tracks-develop')).toEqual({ + kind: 'branch', + remote: 'fork', + branch: 'tracks-develop' + }) + }) + + it('reads a glab merge-request checkout as a link to that review', () => { + const config = configWith([ + ['branch.mr-9.remote', 'origin'], + ['branch.mr-9.merge', 'refs/merge-requests/9/head'] + ]) + expect(readReviewHead(config, 'mr-9')).toEqual({ kind: 'review', number: 9 }) + }) + + it('defaults to origin when git config could not be read', () => { + expect(readReviewHead(new Map(), 'feature')).toEqual({ + kind: 'branch', + remote: 'origin', + branch: 'feature' + }) + }) +}) diff --git a/src/shared/git-review-head.ts b/src/shared/git-review-head.ts new file mode 100644 index 000000000000..a70832fe1d6c --- /dev/null +++ b/src/shared/git-review-head.ts @@ -0,0 +1,62 @@ +import { readGitConfigSnapshotValue, type GitConfigSnapshot } from './git-config-snapshot-runner' + +/** + * Where a branch's review comes from: the remote branch it is pushed to, or a review the branch + * was checked out from (`branch..merge = refs/pull/N/head` from `gh pr checkout`, or + * `refs/merge-requests/N/head` from `glab mr checkout`). + */ +export type ReviewHead = + | { kind: 'branch'; remote: string; branch: string } + | { kind: 'review'; number: number } + +// Remotes reviews are filed against; a differently named branch tracked there is a base, not a head. +const REVIEW_BASE_REMOTES = new Set(['origin', 'upstream']) + +function remoteNameForUrl(config: GitConfigSnapshot, url: string): string | null { + for (const [key, values] of config) { + const match = /^remote\.(.+)\.url$/.exec(key) + if (match?.[1] && values.includes(url)) { + return match[1] + } + } + return null +} + +function readRemote(config: GitConfigSnapshot, key: string): string | null { + const remote = readGitConfigSnapshotValue(config, key) + if (!remote || remote === '.') { + return null + } + return remoteNameForUrl(config, remote) ?? remote +} + +/** + * The branch's review head, following where a plain push of this branch lands (as `gh pr view` + * does), never the branch it pulls from: a branch tracking `origin/develop` is not develop. + */ +export function readReviewHead(config: GitConfigSnapshot, branch: string): ReviewHead { + const merge = readGitConfigSnapshotValue(config, `branch.${branch}.merge`) + const reviewRef = merge ? /^refs\/(?:pull|merge-requests)\/(\d+)\/head$/.exec(merge) : null + if (reviewRef) { + return { kind: 'review', number: Number(reviewRef[1]) } + } + const trackedRemote = readRemote(config, `branch.${branch}.remote`) + const remote = + readRemote(config, `branch.${branch}.pushRemote`) ?? + readRemote(config, 'remote.pushDefault') ?? + trackedRemote ?? + 'origin' + const trackedBranch = merge?.startsWith('refs/heads/') ? merge.slice('refs/heads/'.length) : null + if (trackedBranch && trackedBranch !== branch && remote === trackedRemote) { + const pushDefault = readGitConfigSnapshotValue(config, 'push.default') + if ( + pushDefault === 'upstream' || + pushDefault === 'tracking' || + !REVIEW_BASE_REMOTES.has(remote) + ) { + // A contributor fork's branch checked out under a local name is that branch's review head. + return { kind: 'branch', remote, branch: trackedBranch } + } + } + return { kind: 'branch', remote, branch } +}