From da396580584991cd2a8effb8acdd64dde882e705 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 12 Nov 2026 14:31:04 -0800 Subject: [PATCH] fix(github): separate historical association from merged containment proof --- .../client/lookup/branch-lookup-resolution.ts | 23 +++--- .../client-configured-push-head.test.ts | 71 ++++++++++++++++++- .../merged-pr-commit-membership.test.ts | 10 +-- .../refresh/merged-pr-commit-membership.ts | 15 ++-- .../refresh/merged-pr-head-membership.ts | 12 ++-- .../remotes/git-review-branch-source.test.ts | 7 +- .../git/remotes/git-review-branch-source.ts | 3 + 7 files changed, 109 insertions(+), 32 deletions(-) diff --git a/src/main/github/client/lookup/branch-lookup-resolution.ts b/src/main/github/client/lookup/branch-lookup-resolution.ts index a80c6f9d89d..987253f405c 100644 --- a/src/main/github/client/lookup/branch-lookup-resolution.ts +++ b/src/main/github/client/lookup/branch-lookup-resolution.ts @@ -2,7 +2,7 @@ import type { PRRefreshOutcome } from '../../../../shared/github/pull-requests/r import type { ghRepoExecOptions } from '../../repositories/github-repository-identity' import type { GitHubRepositoryIdentity as OwnerRepo } from '../../../../shared/github/repositories/repository-identity' import { mergedPRHeadMembership } from '../../pull-requests/refresh/merged-pr-head-membership' -import type { MergedReviewCommitMembership as MergedPRCommitMembership } from '../../../source-control/merged-review-membership-memo' +import type { MergedPRHeadRelationship } from '../../pull-requests/refresh/merged-pr-commit-membership' import { shouldHideNonOpenReviewOnDefaultBranch } from '../../../source-control/repo-default-branch' import { resolveGitHubApiRepositoryCandidates } from '../../repositories/github-api-repository' import { exactRepositoryCandidates } from '../pull-request-lookup-candidates' @@ -89,11 +89,11 @@ export async function resolvePRForBranchOutcome(input: { ? options.currentHeadOid.trim() : null let confirmedContainedHeadOid: string | null = null - const mergedPRContainsHead = async ( + const mergedPRHeadRelationship = async ( candidate: PullRequestLookupData, candidateRepo: OwnerRepo | null, headOid: string | null - ): Promise => { + ): Promise => { if (!candidateRepo || !headOid) { return 'unknown' } @@ -128,10 +128,9 @@ export async function resolvePRForBranchOutcome(input: { if (!shouldHideMergedImplicitPR(candidate, linkedPRNumber, currentHeadOidForMergedImplicit)) { return false } - // PR-history membership preserves merged work after web commits or update-branch merges. - return ( - (await mergedPRContainsHead(candidate, candidateRepo, currentHeadOidForMergedImplicit)) !== - 'contained' + // Historical association keeps the review visible without granting cleanup containment proof. + return !['contained', 'associated'].includes( + await mergedPRHeadRelationship(candidate, candidateRepo, currentHeadOidForMergedImplicit) ) } @@ -234,12 +233,14 @@ export async function resolvePRForBranchOutcome(input: { data, linkedPRNumber, explicitCurrentHeadOid, - (candidate, headOid) => - mergedPRContainsHead( + async (candidate, headOid) => { + const relationship = await mergedPRHeadRelationship( candidate, dataRepo ?? ownerRepoFromPullRequestUrl(candidate.url), headOid ) + return relationship === 'associated' ? 'unknown' : relationship + } ) const fallbackConfirmedMergedBranch = typeof fallbackPRNumber === 'number' && @@ -248,7 +249,9 @@ export async function resolvePRForBranchOutcome(input: { const explicitHeadHidesMergedImplicitPR = explicitCurrentHeadOid !== null && shouldHideMergedImplicitPR(data, linkedPRNumber, explicitCurrentHeadOid) && - (await mergedPRContainsHead(data, dataRepo, explicitCurrentHeadOid)) !== 'contained' + !['contained', 'associated'].includes( + await mergedPRHeadRelationship(data, dataRepo, explicitCurrentHeadOid) + ) // Fallback preservation already checked head identity; repeating lazy-HEAD checks could hide valid deleted heads. const shouldPreserveMergedFallback = !explicitHeadHidesMergedImplicitPR && diff --git a/src/main/github/client/lookup/client-configured-push-head.test.ts b/src/main/github/client/lookup/client-configured-push-head.test.ts index 846046b0631..171817acfe8 100644 --- a/src/main/github/client/lookup/client-configured-push-head.test.ts +++ b/src/main/github/client/lookup/client-configured-push-head.test.ts @@ -193,11 +193,27 @@ describe('getPRForBranch configured push head', () => { expect(pr).toMatchObject({ number: 91, headRepo: { owner: 'contributor', repo: 'orca' } }) }) + it.each([ + ['remote.origin.push', 'refs/heads/other:refs/heads/other'], + ['push.default', 'nothing'] + ])( + 'does not invent a fork head when this branch has no configured push destination: %j', + async (key, value) => { + scriptGit({ 'remote.origin.url': 'git@github.com:stablyai/orca.git', [key]: value }, [ + 'origin\tgit@github.com:stablyai/orca.git (fetch)', + 'origin\tgit@github.com:contributor/orca.git (push)' + ]) + scriptGitHub('contributor', BRANCH) + await expect(getPRForBranch('/repo-root', BRANCH)).resolves.toBeNull() + } + ) + it('finds a PR whose head is the remote branch a push refspec publishes to', async () => { scriptGit( { 'remote.origin.url': 'git@github.com:stablyai/orca.git', - 'remote.origin.push': `refs/heads/${BRANCH}:refs/heads/published-name` + 'remote.origin.push': `refs/heads/${BRANCH}:refs/heads/published-name`, + 'push.default': 'nothing' }, [ 'origin\tgit@github.com:stablyai/orca.git (fetch)', @@ -231,3 +247,56 @@ describe('getPRForBranch configured push head', () => { }) }) }) + +it('keeps historical association visible without emitting merged containment proof', async () => { + resetPRForBranchMocks(clientMocks) + getOwnerRepoMock.mockResolvedValue({ owner: 'stablyai', repo: 'orca' }) + scriptGit({}, []) + scriptGitHub('stablyai', BRANCH) + const oldHead = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' + const replacement = 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb' + const original = ghExecFileAsyncMock.getMockImplementation()! + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[1]?.includes('/commits/')) { + return { stdout: '[]' } + } + if (args.some((arg) => arg.includes('HEAD_REF_FORCE_PUSHED_EVENT'))) { + return { + stdout: JSON.stringify({ + data: { + repository: { + pullRequest: { + timelineItems: { totalCount: 1, nodes: [{ beforeCommit: { oid: oldHead } }] } + } + } + } + }) + } + } + const result = await original(args) + const parsed = JSON.parse(result.stdout) + return { + stdout: JSON.stringify( + Array.isArray(parsed) + ? parsed.map((pr) => ({ + ...pr, + state: 'closed', + merged_at: '2026-11-10T00:00:00Z', + head: { ...pr.head, sha: replacement } + })) + : { + ...parsed, + state: 'MERGED', + headRefOid: replacement, + mergedAt: '2026-11-10T00:00:00Z' + } + ) + } + }) + const pr = await getPRForBranch('/repo-root', BRANCH, null, null, null, { + currentHeadOid: oldHead, + includeDerivedData: false + }) + expect(pr).toMatchObject({ number: 91, state: 'merged' }) + expect(pr?.confirmedContainedHeadOid).toBeUndefined() +}) diff --git a/src/main/github/pull-requests/refresh/merged-pr-commit-membership.test.ts b/src/main/github/pull-requests/refresh/merged-pr-commit-membership.test.ts index cb768893db9..de2df8d0e11 100644 --- a/src/main/github/pull-requests/refresh/merged-pr-commit-membership.test.ts +++ b/src/main/github/pull-requests/refresh/merged-pr-commit-membership.test.ts @@ -11,14 +11,14 @@ vi.mock('../../api/rate-limit', () => ({ })) import { - isCommitPartOfMergedPR, + getMergedPRHeadRelationship, resetMergedPRCommitMembershipCacheForTest } from './merged-pr-commit-membership' const REBASED_AWAY = '6ada46e391c5e4025d13d0304c4c0729af13a923' function membership() { - return isCommitPartOfMergedPR({ + return getMergedPRHeadRelationship({ ownerRepo: { owner: 'stablyai', repo: 'orca' }, prNumber: 41082, commitOid: REBASED_AWAY, @@ -49,7 +49,7 @@ function scriptForcePushes(beforeOids: (string | null)[]): void { }) } -describe('isCommitPartOfMergedPR', () => { +describe('getMergedPRHeadRelationship', () => { it('preserves uncertainty when a force-push event has no prior commit', async () => { scriptForcePushes([null]) await expect(membership()).resolves.toBe('unknown') @@ -60,8 +60,8 @@ describe('isCommitPartOfMergedPR', () => { resetMergedPRCommitMembershipCacheForTest() }) - it('counts a head the PR was force-pushed away from, as after a rebase pushed elsewhere', async () => { + it('treats a force-pushed-away head as association without claiming its changes landed', async () => { scriptForcePushes([null, REBASED_AWAY]) - await expect(membership()).resolves.toBe('contained') + await expect(membership()).resolves.toBe('associated') }) }) diff --git a/src/main/github/pull-requests/refresh/merged-pr-commit-membership.ts b/src/main/github/pull-requests/refresh/merged-pr-commit-membership.ts index 9937a971c66..6ddcae7f939 100644 --- a/src/main/github/pull-requests/refresh/merged-pr-commit-membership.ts +++ b/src/main/github/pull-requests/refresh/merged-pr-commit-membership.ts @@ -13,19 +13,20 @@ import type { GitHubRepositoryIdentity as OwnerRepo } from '../../../../shared/g const COMMIT_PULLS_PAGE_SIZE = 100 const COMMIT_PULLS_MAX_PAGES = 5 -const membershipMemo = new MergedReviewMembershipMemo() +export type MergedPRHeadRelationship = MergedPRCommitMembership | 'associated' +const membershipMemo = new MergedReviewMembershipMemo() export function resetMergedPRCommitMembershipCacheForTest(): void { membershipMemo.clear() } -/** Check PR-history membership rather than branch-name reuse; failures return unknown. */ -export async function isCommitPartOfMergedPR(args: { +/** Force-push history proves association only; it cannot prove those changes landed. */ +export async function getMergedPRHeadRelationship(args: { ownerRepo: OwnerRepo prNumber: number commitOid: string ghOptions: GitHubRepoExecOptions -}): Promise { +}): Promise { const oid = args.commitOid.trim().toLowerCase() if (!/^[0-9a-f]{4,64}$/.test(oid) || !Number.isInteger(args.prNumber)) { return 'unknown' @@ -112,7 +113,7 @@ async function wasForcePushedAwayFromPR( prNumber: number, oid: string, ghOptions: GitHubRepoExecOptions -): Promise { +): Promise { if (repositoryRateLimitGuard(ownerRepo, 'graphql', ghOptions).blocked) { return 'unknown' } @@ -137,8 +138,8 @@ async function wasForcePushedAwayFromPR( if (!timeline) { return 'unknown' } - if (timeline.nodes.some((node) => node?.beforeCommit?.oid.toLowerCase().startsWith(oid))) { - return 'contained' + if (timeline.nodes.some((node) => node?.beforeCommit?.oid.toLowerCase() === oid)) { + return 'associated' } return timeline.totalCount > timeline.nodes.length || timeline.nodes.some((node) => !node?.beforeCommit?.oid) diff --git a/src/main/github/pull-requests/refresh/merged-pr-head-membership.ts b/src/main/github/pull-requests/refresh/merged-pr-head-membership.ts index 26a9d1da6bd..9132f885818 100644 --- a/src/main/github/pull-requests/refresh/merged-pr-head-membership.ts +++ b/src/main/github/pull-requests/refresh/merged-pr-head-membership.ts @@ -1,11 +1,13 @@ import type { GitHubRepositoryIdentity as OwnerRepo } from '../../../../shared/github/repositories/repository-identity' import type { GitHubRepoExecOptions } from '../../repositories/github-api-repository' import type { HostedReviewLocalGitOptions } from '../../../source-control/hosted-review-git-options' -import { isCommitPartOfMergedPR } from './merged-pr-commit-membership' -import type { MergedReviewCommitMembership as MergedPRCommitMembership } from '../../../source-control/merged-review-membership-memo' +import { + getMergedPRHeadRelationship, + type MergedPRHeadRelationship +} from './merged-pr-commit-membership' import { mergedReviewHeadIsEquivalent } from '../../../source-control/merged-review-head-equivalence' -/** PR-history membership, or a local proof that HEAD is an equivalent rebase GitHub never saw. */ +/** Membership or local equivalence proves containment; force-push history alone stays associated. */ export async function mergedPRHeadMembership(args: { ownerRepo: OwnerRepo prNumber: number @@ -16,8 +18,8 @@ export async function mergedPRHeadMembership(args: { repoPath: string connectionId?: string | null localGitOptions: HostedReviewLocalGitOptions -}): Promise { - const membership = await isCommitPartOfMergedPR({ +}): Promise { + const membership = await getMergedPRHeadRelationship({ ownerRepo: args.ownerRepo, prNumber: args.prNumber, commitOid: args.headOid, diff --git a/src/shared/git/remotes/git-review-branch-source.test.ts b/src/shared/git/remotes/git-review-branch-source.test.ts index ec9d85dc23c..146dc566e69 100644 --- a/src/shared/git/remotes/git-review-branch-source.test.ts +++ b/src/shared/git/remotes/git-review-branch-source.test.ts @@ -45,11 +45,10 @@ it('resolves real Git tracking, triangular publication, local tracking and expli await git(['config', 'remote.origin.push', 'HEAD']) expect((await snapshot()).get('feature')).toMatchObject({ branchName: 'feature' }) await git(['config', 'remote.origin.push', 'refs/heads/main:refs/heads/main']) - expect((await snapshot()).get('feature')).toMatchObject({ - branchName: 'feature', - trackedBranchName: 'main' - }) + expect((await snapshot()).get('feature')).toBeInstanceOf(Error) await git(['config', '--unset', 'remote.origin.push']) + await git(['config', 'push.default', 'nothing']) + expect((await snapshot()).get('feature')).toBeInstanceOf(Error) await git(['config', 'branch.feature.merge', 'refs/tags/v1']) await git(['config', 'push.default', 'upstream']) expect((await snapshot()).get('feature')).toEqual({ diff --git a/src/shared/git/remotes/git-review-branch-source.ts b/src/shared/git/remotes/git-review-branch-source.ts index 33f825f01b5..f3794c13f32 100644 --- a/src/shared/git/remotes/git-review-branch-source.ts +++ b/src/shared/git/remotes/git-review-branch-source.ts @@ -125,6 +125,9 @@ export async function readGitReviewBranchSources( if (destinations.size > 1) { throw new Error('Review push destination is ambiguous') } + if (destinations.size === 0 && (specs.length > 0 || get('push.default') === 'nothing')) { + throw new Error('Review push destination is not configured for this branch') + } let branchName = destinations.values().next().value ?? branch if (destinations.size === 0) { if (!localTracking && remote === trackedRemote) {