From 7ac9f4d2f1e9efbb480db24432b20bc71b539069 Mon Sep 17 00:00:00 2001 From: petersindex <287557875+petersindex@users.noreply.github.com> Date: Sat, 10 Oct 2026 19:07:54 +1100 Subject: [PATCH] fix(github): compare a tracked upstream with its own remote's default branch A fork's upstream remote can default to a different branch than origin. Read refs/remotes//HEAD for the tracked remote and fall back to origin's default only when git never recorded it. Reuses the default-branch resolver's host routing and cache. --- ...nt-tracked-upstream-default-branch.test.ts | 55 +++++++++-- .../lookup/tracked-upstream-default-branch.ts | 24 +++-- .../source-control/repo-default-branch.ts | 96 +++++++++++++------ src/shared/git-default-base-ref.ts | 25 +++++ 4 files changed, 155 insertions(+), 45 deletions(-) diff --git a/src/main/github/client-tracked-upstream-default-branch.test.ts b/src/main/github/client-tracked-upstream-default-branch.test.ts index 132aa7b6b8c..89082ecf2e3 100644 --- a/src/main/github/client-tracked-upstream-default-branch.test.ts +++ b/src/main/github/client-tracked-upstream-default-branch.test.ts @@ -46,13 +46,23 @@ const { const ACME = { owner: 'acme', repo: 'widgets' } -function primeGit(trackedUpstream: string): void { +/** 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)')) { - return { stdout: 'refs/remotes/origin/HEAD\0refs/remotes/origin/develop\n', stderr: '' } + 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: '' } @@ -105,6 +115,18 @@ function primeGh(listsByHead: Record[]>): void { }) } +/** A fork clone: `origin` is the fork, `upstream` is the repo PRs target. */ +function primeForkWithUpstream(): void { + const UPSTREAM = { owner: 'stablyai', repo: 'widgets' } + resolvePRRepositoryCandidatesMock.mockResolvedValue({ + candidates: [UPSTREAM, ACME], + headRepo: ACME + }) + 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) @@ -130,14 +152,7 @@ describe('issue #26948: a branch tracking the default branch', () => { }) it('skips the default branch on a second remote that PRs target (fork checkout off upstream)', async () => { - const UPSTREAM = { owner: 'stablyai', repo: 'widgets' } - resolvePRRepositoryCandidatesMock.mockResolvedValue({ - candidates: [UPSTREAM, ACME], - headRepo: ACME - }) - getOwnerRepoForRemoteMock.mockImplementation(async (_repoPath: string, remoteName: string) => - remoteName === 'upstream' ? UPSTREAM : remoteName === 'origin' ? ACME : null - ) + primeForkWithUpstream() primeGit('refs/remotes/upstream/develop') primeGh({ 'stablyai:develop': [restPR('develop')] }) @@ -146,6 +161,26 @@ describe('issue #26948: a branch tracking the default branch', () => { 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')] }) + + 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')] }) + + 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 diff --git a/src/main/github/client/lookup/tracked-upstream-default-branch.ts b/src/main/github/client/lookup/tracked-upstream-default-branch.ts index 41b89157150..baea2f0aaae 100644 --- a/src/main/github/client/lookup/tracked-upstream-default-branch.ts +++ b/src/main/github/client/lookup/tracked-upstream-default-branch.ts @@ -1,6 +1,9 @@ import type { OwnerRepo } from '../../gh-utils' import { githubRepoIdentityKey } from '../../../../shared/github/repository-identity-key' -import { getRepoDefaultBranchName } from '../../../source-control/repo-default-branch' +import { + getRemoteHeadBranchName, + getRepoDefaultBranchName +} from '../../../source-control/repo-default-branch' import type { HostedReviewLocalGitOptions } from './../github-exec-scope' import type { TrackedUpstreamBranch } from './tracked-upstream-cache' @@ -21,10 +24,19 @@ export async function isTrackedUpstreamDefaultBranch(input: { if (!input.candidates.some((candidate) => githubRepoIdentityKey(candidate) === upstreamRepoKey)) { return false } - const defaultBranchName = await getRepoDefaultBranchName( - input.repoPath, - input.connectionId, - input.localGitOptions - ) + const { remoteName } = input.upstreamBranch + // Why: a fork's `upstream` can default to a different branch than `origin`; use origin's only when the remote never recorded its HEAD. + const remoteDefaultBranchName = + remoteName === 'origin' + ? null + : await getRemoteHeadBranchName( + input.repoPath, + remoteName, + input.connectionId, + input.localGitOptions + ) + const defaultBranchName = + remoteDefaultBranchName ?? + (await getRepoDefaultBranchName(input.repoPath, input.connectionId, input.localGitOptions)) return defaultBranchName === input.upstreamBranch.branchName } diff --git a/src/main/source-control/repo-default-branch.ts b/src/main/source-control/repo-default-branch.ts index d24cde4ea13..6baaf4ce7e3 100644 --- a/src/main/source-control/repo-default-branch.ts +++ b/src/main/source-control/repo-default-branch.ts @@ -1,4 +1,5 @@ import { resolveDefaultBaseRefViaExec } from '../git/repo' +import { resolveRemoteHeadBranchViaExec, type GitExec } from '../../shared/git-default-base-ref' import { gitExecFileAsync } from '../git/runner' import { getSshGitProvider } from '../providers/ssh-git-dispatch' import type { HostedReviewLocalGitOptions } from './hosted-review-git-options' @@ -45,17 +46,80 @@ function pruneRepoDefaultBranchCache(now: number): void { } } +/** Git over the transport the hosted-review call uses; null when the SSH provider is gone. */ +function createRepoGitExec( + repoPath: string, + connectionId: string | null | undefined, + localGitOptions: HostedReviewLocalGitOptions +): GitExec | null { + const provider = connectionId ? getSshGitProvider(connectionId) : null + if (connectionId && !provider) { + // Why: a dropped SSH provider must not fall back to local git — the + // repoPath is remote, so a local run could answer for the wrong repo. + return null + } + const resolutionDeadline = Date.now() + REPO_DEFAULT_BRANCH_RESOLUTION_BUDGET_MS + // Why: the resolver can try five refs; share one deadline so an unhealthy + // local/WSL/SSH host cannot multiply the refresh delay per fallback probe. + return (argv) => { + const timeoutMs = resolutionDeadline - Date.now() + if (timeoutMs <= 0) { + return Promise.reject(new Error('Default branch resolution timed out.')) + } + return provider + ? provider.exec(argv, repoPath, { timeoutMs }) + : gitExecFileAsync(argv, { + cwd: repoPath, + ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}), + ...(localGitOptions.admissionTier + ? { admissionTier: localGitOptions.admissionTier } + : {}), + timeout: timeoutMs + }) + } +} + /** * Resolve the repository's default branch NAME (`main`, `master`, …) over the * transport the surrounding hosted-review call already uses. Returns null when * unresolvable so callers fail open (#9171 guard skipped, current behavior). */ -export async function getRepoDefaultBranchName( +export function getRepoDefaultBranchName( repoPath: string, connectionId?: string | null, localGitOptions: HostedReviewLocalGitOptions = {} ): Promise { - const cacheKey = getRepoDefaultBranchCacheKey(repoPath, connectionId, localGitOptions) + return resolveCachedBranchName( + getRepoDefaultBranchCacheKey(repoPath, connectionId, localGitOptions), + async () => { + const exec = createRepoGitExec(repoPath, connectionId, localGitOptions) + const baseRef = exec ? await resolveDefaultBaseRefViaExec(exec) : null + // Same base-ref → branch-name normalization as git/repo.ts getRemoteFileUrl. + return baseRef ? baseRef.replace(/^origin\//, '') : null + } + ) +} + +/** Branch the remote's own HEAD names; null on older Git or a remote added without it. */ +export function getRemoteHeadBranchName( + repoPath: string, + remoteName: string, + connectionId?: string | null, + localGitOptions: HostedReviewLocalGitOptions = {} +): Promise { + return resolveCachedBranchName( + [getRepoDefaultBranchCacheKey(repoPath, connectionId, localGitOptions), remoteName].join('\0'), + async () => { + const exec = createRepoGitExec(repoPath, connectionId, localGitOptions) + return exec ? resolveRemoteHeadBranchViaExec(exec, remoteName) : null + } + ) +} + +async function resolveCachedBranchName( + cacheKey: string, + resolve: () => Promise +): Promise { const now = Date.now() const cached = repoDefaultBranchCache.get(cacheKey) if (cached && cached.expiresAt > now) { @@ -71,33 +135,7 @@ export async function getRepoDefaultBranchName( const resolution = (async (): Promise => { let branchName: string | null = null try { - const provider = connectionId ? getSshGitProvider(connectionId) : null - if (connectionId && !provider) { - // Why: a dropped SSH provider must not fall back to local git — the - // repoPath is remote, so a local run could answer for the wrong repo. - return null - } - const resolutionDeadline = Date.now() + REPO_DEFAULT_BRANCH_RESOLUTION_BUDGET_MS - // Why: the resolver can try five refs; share one deadline so an unhealthy - // local/WSL/SSH host cannot multiply the refresh delay per fallback probe. - const baseRef = await resolveDefaultBaseRefViaExec((argv) => { - const timeoutMs = resolutionDeadline - Date.now() - if (timeoutMs <= 0) { - return Promise.reject(new Error('Default branch resolution timed out.')) - } - return provider - ? provider.exec(argv, repoPath, { timeoutMs }) - : gitExecFileAsync(argv, { - cwd: repoPath, - ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}), - ...(localGitOptions.admissionTier - ? { admissionTier: localGitOptions.admissionTier } - : {}), - timeout: timeoutMs - }) - }) - // Same base-ref → branch-name normalization as git/repo.ts getRemoteFileUrl. - branchName = baseRef ? baseRef.replace(/^origin\//, '') : null + branchName = await resolve() } catch { branchName = null } diff --git a/src/shared/git-default-base-ref.ts b/src/shared/git-default-base-ref.ts index 539b0f57300..e7bf9990e42 100644 --- a/src/shared/git-default-base-ref.ts +++ b/src/shared/git-default-base-ref.ts @@ -35,3 +35,28 @@ export async function resolveDefaultBaseRefViaExec(exec: GitExec): Promise/HEAD` points at, or null when git never recorded it. */ +export async function resolveRemoteHeadBranchViaExec( + exec: GitExec, + remoteName: string +): Promise { + const headRef = `refs/remotes/${remoteName}/HEAD` + const branchPrefix = `refs/remotes/${remoteName}/` + try { + const { stdout } = await exec([ + 'for-each-ref', + '--format=%(refname)%00%(symref)', + `${headRef.slice(0, -1)}[${headRef.slice(-1)}]` + ]) + for (const line of iterateProcessOutputLines(stdout)) { + const [ref, target] = line.split('\0') + if (ref === headRef && target?.startsWith(branchPrefix)) { + return target.slice(branchPrefix.length) + } + } + return null + } catch { + return null + } +}