mirror of
https://github.com/stablyai/orca.git
synced 2026-11-15 18:37:24 +00:00
fix(github): separate historical association from merged containment proof
This commit is contained in:
@@ -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<MergedPRCommitMembership> => {
|
||||
): Promise<MergedPRHeadRelationship> => {
|
||||
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 &&
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
|
||||
@@ -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')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<MergedPRHeadRelationship>()
|
||||
|
||||
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<MergedPRCommitMembership> {
|
||||
}): Promise<MergedPRHeadRelationship> {
|
||||
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<MergedPRCommitMembership> {
|
||||
): Promise<MergedPRHeadRelationship> {
|
||||
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)
|
||||
|
||||
@@ -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<MergedPRCommitMembership> {
|
||||
const membership = await isCommitPartOfMergedPR({
|
||||
}): Promise<MergedPRHeadRelationship> {
|
||||
const membership = await getMergedPRHeadRelationship({
|
||||
ownerRepo: args.ownerRepo,
|
||||
prNumber: args.prNumber,
|
||||
commitOid: args.headOid,
|
||||
|
||||
@@ -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({
|
||||
|
||||
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user