fix(github): scope PR selection and clear rejected cached links

Apply the default-branch policy while probing each repository, so an
integration PR cannot preempt a valid result or suppress the same number
in another repository. Check branch heads by repository and server.

Invalidate remote defaults when their review scope or SSH provider changes.
Report rejected PR URLs through an optional refresh-result field, so merged
integration PRs clear from both caches even at the current worktree head.
Share the manual and coordinator cache update implementation.

Co-authored-by: petersindex <287557875+petersindex@users.noreply.github.com>
This commit is contained in:
Neil
2026-10-10 17:08:10 -07:00
committed by Neil
co-authored by petersindex
parent 0bc07c9954
commit 82c5484f66
15 changed files with 505 additions and 193 deletions
@@ -80,7 +80,7 @@ function restPR(
title: 'Release develop',
state: 'closed',
merged_at: null,
html_url: `https://github.com/${prRepo.owner}/${prRepo.repo}/pull/7`,
html_url: `https://${prRepo.host ?? 'github.com'}/${prRepo.owner}/${prRepo.repo}/pull/7`,
updated_at: '2025-01-01T00:00:00Z',
draft: false,
mergeable: null,
@@ -95,39 +95,53 @@ function restPR(
function primeGh(
listsByHead: Record<string, RestPullRequest[]>,
exactPR: RestPullRequest = Object.values(listsByHead).flat()[0] ?? restPR()
exactPR: RestPullRequest | RestPullRequest[] = Object.values(listsByHead).flat()[0] ?? restPR()
): void {
const prsByNumber = new Map(
[...Object.values(listsByHead).flat(), exactPR].map((pr) => [pr.number, pr])
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[]) => {
const head = args[1]?.match(/pulls\?head=([^&]+)/)?.[1]
if (args[0] === 'api' && head) {
return { stdout: JSON.stringify(listsByHead[decodeURIComponent(head)] ?? []) }
}
const exact = prsByNumber.get(Number(args[2]))
if (args[0] === 'pr' && args[1] === 'view' && exact) {
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
})
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 repoIndex = args.indexOf('--repo')
const exact = prsByRepositoryAndNumber.get(`${host}/${args[repoIndex + 1]}#${args[2]}`)
if (args[0] === 'pr' && args[1] === 'view' && exact) {
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(' ')}`)
}
throw new Error(`gh unavailable: ${args.join(' ')}`)
})
)
}
/** A fork clone: `origin` is the fork, `upstream` is the repo PRs target. */
@@ -165,6 +179,26 @@ describe('issue #26948: a branch tracking the default branch', () => {
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' })
@@ -296,4 +330,90 @@ describe('issue #26948: a branch tracking the default branch', () => {
)
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
})
})
})
@@ -26,12 +26,12 @@ import { lookupPRByBranchName } 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,
type TrackedUpstreamBranch
} from './tracked-upstream-cache'
import { shouldRetryTrackedUpstreamBranch } from './tracked-upstream-cache'
import { getTrackedUpstreamBranch } from './tracked-upstream-branch'
import { shouldHideImplicitDefaultBranchPR } from './implicit-default-branch-pr'
import {
createImplicitDefaultBranchPRPolicy,
type ImplicitDefaultBranchPRContext
} from './implicit-default-branch-pr'
import { PR_BRANCH_LOOKUP_BUCKETS } from './pr-lookup-rate-limit'
import type { HostedReviewLocalGitOptions } from './../github-exec-scope'
export async function resolvePRForBranchOutcome(input: {
@@ -80,14 +80,22 @@ export async function resolvePRForBranchOutcome(input: {
let hasPendingBranchLookupError = false
let currentHeadOidForMergedImplicit: string | null | undefined
let usedExactNumberLookup = false
let retriedUpstream: { branch: TrackedUpstreamBranch; headRepo: OwnerRepo } | null = null
const defaultBranchPRContext: ImplicitDefaultBranchPRContext = {
originHeadRepo: headRepo,
trackedUpstream: null,
branchName,
linkedPRNumber,
repoPath,
connectionId,
localGitOptions
}
const implicitPRPolicy = createImplicitDefaultBranchPRPolicy(defaultBranchPRContext)
const explicitCurrentHeadOid =
typeof options.currentHeadOid === 'string' && options.currentHeadOid.trim().length > 0
? options.currentHeadOid.trim()
: null
let confirmedContainedHeadOid: string | null = null
let headDivergedFromMergedPRAtOid: string | null = null
const mergedPRContainsHead = async (
candidate: PullRequestLookupData,
candidateRepo: OwnerRepo | null,
@@ -146,7 +154,8 @@ export async function resolvePRForBranchOutcome(input: {
headRepo,
branchName,
ghOptions,
executionScope
executionScope,
policy: implicitPRPolicy
})
data = branchLookup.data
dataRepo = branchLookup.dataRepo
@@ -174,13 +183,17 @@ export async function resolvePRForBranchOutcome(input: {
upstreamHeadRepo &&
shouldRetryTrackedUpstreamBranch(upstreamBranch, branchName, upstreamHeadRepo, headRepo)
) {
retriedUpstream = { branch: upstreamBranch, headRepo: upstreamHeadRepo }
defaultBranchPRContext.trackedUpstream = {
branch: upstreamBranch,
headRepo: upstreamHeadRepo
}
const upstreamLookup = await lookupPRByBranchName({
candidates,
headRepo: upstreamHeadRepo,
branchName: upstreamBranch.branchName,
ghOptions,
executionScope
executionScope,
policy: implicitPRPolicy
})
data = upstreamLookup.data
dataRepo = upstreamLookup.dataRepo
@@ -195,29 +208,6 @@ export async function resolvePRForBranchOutcome(input: {
}
}
}
let rejectedDefaultPRNumber: number | null = null
const hideUnrelatedDefaultPR = async (): Promise<void> => {
if (
data &&
(await shouldHideImplicitDefaultBranchPR({
data,
prRepo: dataRepo,
originHeadRepo: headRepo,
trackedUpstream: retriedUpstream,
branchName,
linkedPRNumber,
repoPath,
connectionId,
localGitOptions
}))
) {
rejectedDefaultPRNumber = data.number
data = null
dataRepo = null
dataHeadRepo = headRepo
}
}
await hideUnrelatedDefaultPR()
let mergedBranchLookupNumber: number | null = null
if (await hideMergedImplicitPR(data, dataRepo)) {
mergedBranchLookupNumber = data?.number ?? null
@@ -225,32 +215,32 @@ export async function resolvePRForBranchOutcome(input: {
dataRepo = null
dataHeadRepo = headRepo
}
if (
!data &&
typeof linkedPRNumber !== 'number' &&
typeof fallbackPRNumber === 'number' &&
fallbackPRNumber !== rejectedDefaultPRNumber
) {
if (!data && typeof linkedPRNumber !== 'number' && typeof fallbackPRNumber === 'number') {
usedExactNumberLookup = true
const fallbackLookup = await lookupPRByNumber({
candidates,
number: fallbackPRNumber,
ghOptions,
executionScope
executionScope,
policy: implicitPRPolicy
})
data = fallbackLookup.data
dataRepo = fallbackLookup.dataRepo
}
// Recheck exact fallbacks so links cached before #26948 heal without upstream tracking.
await hideUnrelatedDefaultPR()
if (!data) {
if (hasPendingBranchLookupError) {
return prRefreshUpstreamError(pendingBranchLookupError)
}
return { kind: 'no-pr', fetchedAt: Date.now() }
return {
kind: 'no-pr',
fetchedAt: Date.now(),
...(implicitPRPolicy.rejectedPRUrls.size > 0
? { rejectedPRUrls: [...implicitPRPolicy.rejectedPRUrls] }
: {})
}
}
dataHeadRepo = pullRequestHeadRepository(data, dataRepo) ?? dataHeadRepo
headDivergedFromMergedPRAtOid = await linkedMergedPRDivergedHeadOid(
const headDivergedFromMergedPRAtOid = await linkedMergedPRDivergedHeadOid(
data,
linkedPRNumber,
explicitCurrentHeadOid,
@@ -6,11 +6,13 @@ import {
} from '../../../source-control/repo-default-branch'
import type { HostedReviewLocalGitOptions } from './../github-exec-scope'
import type { TrackedUpstreamBranch } from './tracked-upstream-cache'
import { pullRequestHeadRepository, type PullRequestLookupData } from './pull-request-lookup-data'
import {
pullRequestHeadRepository,
type PullRequestLookupData,
type PullRequestLookupPolicy
} from './pull-request-lookup-data'
export async function shouldHideImplicitDefaultBranchPR(input: {
data: PullRequestLookupData
prRepo: OwnerRepo | null
export type ImplicitDefaultBranchPRContext = {
originHeadRepo: OwnerRepo | null
trackedUpstream: { branch: TrackedUpstreamBranch; headRepo: OwnerRepo } | null
branchName: string
@@ -18,7 +20,34 @@ export async function shouldHideImplicitDefaultBranchPR(input: {
repoPath: string
connectionId?: string | null
localGitOptions: HostedReviewLocalGitOptions
}): Promise<boolean> {
}
export function createImplicitDefaultBranchPRPolicy(
context: ImplicitDefaultBranchPRContext
): PullRequestLookupPolicy {
const rejected = new Set<string>()
const rejectedPRUrls = new Set<string>()
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<boolean> {
const { data, branchName, prRepo } = input
if (
!branchName ||
@@ -1,6 +1,10 @@
import { ghExecFileAsync } from '../../gh-utils'
import type { OwnerRepo, ghRepoExecOptions } from '../../gh-utils'
import { githubHostExecOptions, type GitHubApiRepository } from '../../github-api-repository'
import {
githubHostExecOptions,
githubRepositoryWebHost,
type GitHubApiRepository
} from '../../github-api-repository'
import type { GhExecOptions } from './../github-exec-scope'
import { isNoPullRequestError } from './../gh-error-predicates'
import {
@@ -8,7 +12,9 @@ import {
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'
@@ -79,6 +85,7 @@ export async function lookupPRByBranchName(args: {
branchName: string
ghOptions: GhExecOptions
executionScope: string
policy?: PullRequestLookupPolicy
}): Promise<{
data: PullRequestLookupData | null
dataRepo: OwnerRepo | null
@@ -88,6 +95,13 @@ export async function lookupPRByBranchName(args: {
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(
@@ -104,7 +118,11 @@ export async function lookupPRByBranchName(args: {
args.ghOptions,
args.executionScope
)
if (data) {
if (
data &&
pullRequestMatchesHeadRepository(data, candidate, args.headRepo) &&
(!args.policy || (await args.policy.accepts(data, candidate)))
) {
return { data, dataRepo: candidate }
}
} catch (err) {
@@ -128,7 +146,11 @@ export async function lookupPRByBranchName(args: {
args.ghOptions,
args.executionScope
)
if (data) {
if (
data &&
pullRequestMatchesHeadRepository(data, candidate, args.headRepo) &&
(!args.policy || (await args.policy.accepts(data, candidate)))
) {
return { data, dataRepo: candidate }
}
} catch (retryErr) {
@@ -150,10 +172,13 @@ export async function lookupPRByBranchName(args: {
['pr', 'view', args.branchName, '--json', PR_LOOKUP_JSON_FIELDS],
args.ghOptions
)
return {
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 }
} catch (err) {
if (isNoPullRequestError(err)) {
return { data: null, dataRepo: null }
@@ -11,6 +11,7 @@ import {
mapRestPullRequest,
normalizePullRequestLookupData,
type PullRequestLookupData,
type PullRequestLookupPolicy,
type RestPullRequest
} from './pull-request-lookup-data'
import { hydratePullRequestLookupData } from './pull-request-lookup-hydration'
@@ -108,8 +109,12 @@ export async function lookupPRByNumber(args: {
number: number
ghOptions: ReturnType<typeof ghRepoExecOptions>
executionScope: string
policy?: PullRequestLookupPolicy
}): Promise<{ data: PullRequestLookupData | null; dataRepo: OwnerRepo | null }> {
for (const candidate of args.candidates) {
if (args.policy?.isRejected(candidate, args.number)) {
continue
}
try {
const linkedData = await getPRByNumber(
candidate,
@@ -117,7 +122,7 @@ export async function lookupPRByNumber(args: {
args.ghOptions,
args.executionScope
)
if (!linkedData) {
if (!linkedData || (args.policy && !(await args.policy.accepts(linkedData, candidate)))) {
continue
}
return { data: linkedData, dataRepo: candidate }
@@ -138,10 +143,13 @@ export async function lookupPRByNumber(args: {
['pr', 'view', String(args.number), '--json', PR_LOOKUP_JSON_FIELDS],
args.ghOptions
)
return {
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 }
} catch (err) {
if (isNoPullRequestError(err)) {
// Why: stale cached fallback numbers shouldn't error every poll when the PR was deleted or belongs to another repo.
@@ -9,6 +9,7 @@ import type { GitAdmissionTier } from '../../../git/command-runner/git-exec-opti
import type { HostedReviewExecutionOptions } from '../../../source-control/hosted-review-git-options'
import { mapPRState } from '../../mappers'
import type { MergedPRCommitMembership } from '../../merged-pr-commit-membership'
import { githubRepoIdentityKey } from '../../../../shared/github/repository-identity-key'
import {
normalizePRMergeable,
normalizeReviewDecision,
@@ -43,6 +44,12 @@ export type PullRequestLookupData = {
stackMetadataChecked?: boolean
}
export type PullRequestLookupPolicy = {
accepts: (data: PullRequestLookupData, prRepo: OwnerRepo | null) => Promise<boolean>
isRejected: (prRepo: OwnerRepo, number: number) => boolean
rejectedPRUrls: ReadonlySet<string>
}
export type RestPullRequest = {
number: number
title: string
@@ -144,6 +151,19 @@ export function pullRequestHeadRepository(
return owner && repo ? { owner, repo, ...(prRepo?.host ? { host: prRepo.host } : {}) } : null
}
export function pullRequestMatchesHeadRepository(
data: PullRequestLookupData,
prRepo: OwnerRepo | null,
expectedHeadRepo: OwnerRepo | null
): boolean {
const actualHeadRepo = pullRequestHeadRepository(data, prRepo)
return (
!actualHeadRepo ||
!expectedHeadRepo ||
githubRepoIdentityKey(actualHeadRepo) === githubRepoIdentityKey(expectedHeadRepo)
)
}
export function isMergedImplicitPR(
data: PullRequestLookupData,
linkedPRNumber?: number | null