Keep PR refreshes anchored to cached review numbers (#2541)

- Use fallback PR numbers after branch lookup misses, including detached HEAD
- Preserve review cards for forked or deleted-head PRs across manual refreshes
- Clear stale GitHub PR cache entries when unlinking worktree review metadata
This commit is contained in:
Jinjing
2026-05-21 12:12:10 -07:00
committed by GitHub
parent 2e3627fd8e
commit fca5f498db
27 changed files with 537 additions and 96 deletions
+108
View File
@@ -511,6 +511,78 @@ describe('getPRForBranch', () => {
})
})
it('prefers branch lookup over a fallback PR number', async () => {
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
ghExecFileAsyncMock.mockResolvedValueOnce({
stdout: JSON.stringify([
{
number: 43,
title: 'Branch PR wins',
state: 'open',
html_url: 'https://github.com/acme/widgets/pull/43',
updated_at: '2026-03-28T00:00:00Z',
draft: false,
mergeable: true,
head: { ref: 'feature/test', sha: 'branch-head-oid' },
base: { ref: 'main', sha: 'branch-base-oid' }
}
])
})
const pr = await getPRForBranch('/repo-root', 'feature/test', null, null, 42)
expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1)
expect(ghExecFileAsyncMock).toHaveBeenCalledWith(
['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'],
{ cwd: '/repo-root' }
)
expect(pr).toMatchObject({ number: 43, title: 'Branch PR wins' })
})
it('uses a fallback PR number only after branch lookup misses', async () => {
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
ghExecFileAsyncMock
.mockResolvedValueOnce({ stdout: JSON.stringify([]) })
.mockResolvedValueOnce({
stdout: JSON.stringify({
number: 42,
title: 'Fallback PR lookup',
state: 'OPEN',
url: 'https://github.com/acme/widgets/pull/42',
statusCheckRollup: [],
updatedAt: '2026-03-28T00:00:00Z',
isDraft: false,
mergeable: 'MERGEABLE',
baseRefName: 'main',
headRefName: 'contributor/original',
baseRefOid: 'base-oid',
headRefOid: 'fallback-head-oid'
})
})
const pr = await getPRForBranch('/repo-root', 'feature/test', null, null, 42)
expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(
1,
['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'],
{ cwd: '/repo-root' }
)
expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(
2,
[
'pr',
'view',
'42',
'--repo',
'acme/widgets',
'--json',
'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid'
],
{ cwd: '/repo-root' }
)
expect(pr).toMatchObject({ number: 42, title: 'Fallback PR lookup' })
})
it('uses linked PR number as the source of truth when provided', async () => {
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
ghExecFileAsyncMock.mockResolvedValueOnce({
@@ -722,6 +794,42 @@ describe('getPRForBranch', () => {
expect(execFileAsyncMock).not.toHaveBeenCalled()
})
it('uses fallback PR number for empty branch when detached', async () => {
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
ghExecFileAsyncMock.mockResolvedValueOnce({
stdout: JSON.stringify({
number: 42,
title: 'Detached fallback lookup',
state: 'OPEN',
url: 'https://github.com/acme/widgets/pull/42',
statusCheckRollup: [],
updatedAt: '2026-03-28T00:00:00Z',
isDraft: false,
mergeable: 'MERGEABLE',
baseRefName: 'main',
headRefName: 'feature/test',
baseRefOid: 'base-oid',
headRefOid: 'head-oid'
})
})
const pr = await getPRForBranch('/repo-root', '', null, null, 42)
expect(ghExecFileAsyncMock).toHaveBeenCalledWith(
[
'pr',
'view',
'42',
'--repo',
'acme/widgets',
'--json',
'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid'
],
{ cwd: '/repo-root' }
)
expect(pr).toMatchObject({ number: 42, title: 'Detached fallback lookup' })
})
it('returns null when pr list returns an empty array', async () => {
execFileAsyncMock
.mockResolvedValueOnce({ stdout: 'git@github.com:acme/widgets.git\n' })
+72 -37
View File
@@ -1727,6 +1727,46 @@ async function getPRByNumber(
}
}
async function lookupPRByNumber(args: {
candidates: OwnerRepo[]
number: number
ghOptions: ReturnType<typeof ghRepoExecOptions>
}): Promise<{ data: PullRequestLookupData | null; dataRepo: OwnerRepo | null }> {
for (const candidate of args.candidates) {
try {
const linkedData = await getPRByNumber(candidate, args.number, args.ghOptions)
if (!linkedData) {
continue
}
return { data: linkedData, dataRepo: candidate }
} catch (err) {
if (shouldStopAfterExactLookupError(err)) {
throw err
}
// Candidate probing is best-effort; another repo may own the PR.
}
}
if (args.candidates.length > 0) {
return { data: null, dataRepo: null }
}
try {
const { stdout } = await ghExecFileAsync(
['pr', 'view', String(args.number), '--json', PR_LOOKUP_JSON_FIELDS],
args.ghOptions
)
return { data: JSON.parse(stdout), dataRepo: null }
} catch (err) {
if (isNoPullRequestError(err)) {
// Why: stale cached fallback numbers should not turn every poll into an
// error when the PR was deleted or belonged to a different repo.
return { data: null, dataRepo: null }
}
throw err
}
}
function isNotFoundGhError(err: unknown): boolean {
const stderr = err instanceof Error ? err.message : String(err)
return classifyGhError(stderr).type === 'not_found'
@@ -1746,14 +1786,23 @@ function shouldStopAfterExactLookupError(err: unknown): boolean {
* "create from PR" worktrees whose local branch differs from the PR head ref,
* and prevents a coalesced linked-PR refresh from fanning out an unrelated
* branch lookup result to sibling aliases.
* `fallbackPRNumber` is weaker: branch lookup still wins, and exact lookup is
* used only after branch lookup misses.
*/
export async function getPRForBranch(
repoPath: string,
branch: string,
linkedPRNumber?: number | null,
connectionId?: string | null
connectionId?: string | null,
fallbackPRNumber?: number | null
): Promise<PRInfo | null> {
const outcome = await getPRForBranchOutcome(repoPath, branch, linkedPRNumber, connectionId)
const outcome = await getPRForBranchOutcome(
repoPath,
branch,
linkedPRNumber,
connectionId,
fallbackPRNumber
)
return outcome.kind === 'found' ? outcome.pr : null
}
@@ -1761,11 +1810,14 @@ export async function getPRForBranchOutcome(
repoPath: string,
branch: string,
linkedPRNumber?: number | null,
connectionId?: string | null
connectionId?: string | null,
fallbackPRNumber?: number | null
): Promise<PRRefreshOutcome> {
// Strip refs/heads/ prefix if present
const branchName = branch.replace(/^refs\/heads\//, '')
if (!branchName && typeof linkedPRNumber !== 'number') {
// Why: detached HEAD cannot use branch lookup, but an exact linked/fallback
// PR number remains safe to query and keeps review state visible.
if (!branchName && typeof linkedPRNumber !== 'number' && typeof fallbackPRNumber !== 'number') {
return { kind: 'no-pr', fetchedAt: Date.now() }
}
const context = githubRepoContext(repoPath, connectionId)
@@ -1778,39 +1830,13 @@ export async function getPRForBranchOutcome(
let dataRepo: OwnerRepo | null = null
if (typeof linkedPRNumber === 'number') {
for (const candidate of candidates) {
try {
const linkedData = await getPRByNumber(candidate, linkedPRNumber, ghOptions)
if (!linkedData) {
continue
}
data = linkedData
dataRepo = candidate
break
} catch (err) {
if (shouldStopAfterExactLookupError(err)) {
throw err
}
// Candidate probing is best-effort; another repo may own the PR.
}
}
if (!data && candidates.length === 0) {
const args = ['pr', 'view', String(linkedPRNumber), '--json', PR_LOOKUP_JSON_FIELDS]
try {
const { stdout } = await ghExecFileAsync(args, ghOptions)
data = JSON.parse(stdout)
} catch (err) {
if (!isNoPullRequestError(err)) {
return prRefreshUpstreamError(err)
}
// Why: a stale linkedPRNumber (PR deleted, wrong repo, ...) makes
// `gh pr view <number>` reject. Treat that as the no-PR case so
// callers see the historical `null` semantics instead of a thrown
// error every poll cycle.
data = null
}
}
const exactLookup = await lookupPRByNumber({
candidates,
number: linkedPRNumber,
ghOptions
})
data = exactLookup.data
dataRepo = exactLookup.dataRepo
} else if (branchName) {
// During a rebase the worktree is in detached HEAD and branch is empty.
// An empty --head filter causes gh to return an arbitrary PR.
@@ -1852,6 +1878,15 @@ export async function getPRForBranchOutcome(
}
}
}
if (!data && typeof linkedPRNumber !== 'number' && typeof fallbackPRNumber === 'number') {
const fallbackLookup = await lookupPRByNumber({
candidates,
number: fallbackPRNumber,
ghOptions
})
data = fallbackLookup.data
dataRepo = fallbackLookup.dataRepo
}
if (!data) {
return { kind: 'no-pr', fetchedAt: Date.now() }
}
+4 -2
View File
@@ -432,7 +432,8 @@ async function drainQueue(): Promise<void> {
next.candidate.repoPath,
next.candidate.branch,
next.candidate.linkedPRNumber ?? null,
next.candidate.connectionId ?? null
next.candidate.connectionId ?? null,
next.candidate.linkedPRNumber == null ? (next.candidate.fallbackPRNumber ?? null) : null
)
outcomeObserver?.(next.candidate, outcome)
broadcast({ aliases, reason: next.reason, outcome, requestStartedAt }, requestSequence)
@@ -575,7 +576,8 @@ export async function refreshPRNow(candidate: GitHubPRRefreshCandidate): Promise
candidate.repoPath,
candidate.branch,
candidate.linkedPRNumber ?? null,
candidate.connectionId ?? null
candidate.connectionId ?? null,
candidate.linkedPRNumber == null ? (candidate.fallbackPRNumber ?? null) : null
)
outcomeObserver?.(candidate, outcome)
broadcast({ aliases, reason: 'manual', outcome, requestStartedAt }, requestSequence)