Skip to content

Commit 56cbbe4

Browse files
big-guyclaude
andcommitted
fix(pr-poller): don't claim a PR for worktrees on the default branch
A worktree sitting on main/master was being credited with the most-recently-squashed PR's status. Root cause: the SHA-based search fallback indexes the squash commit's SHA against the merged PR, so search(query: \"... <main-HEAD-sha>\") returns the just-merged PR; that PR's headRefOid is the original feature branch, not main's HEAD, so the exact-SHA check missed but the old same-origin fallback then claimed it. Two complementary guards: * Skip the per-request resolution entirely when the worktree's branch equals the repository's default branch (from defaultBranchRef.name). * Tighten resolvePRForWorktree so search-alias nodes require an exact SHA match — search results are too loose to trust via same-origin fallback the way branch-name nodes are. Tests cover the main and master cases, plus a regression test that the SHA-matched search hit still wins when branch-name returns a different PR (the existing fork-disambiguation behavior). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent d917325 commit 56cbbe4

2 files changed

Lines changed: 97 additions & 25 deletions

File tree

src/main/github.test.ts

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -444,6 +444,67 @@ describe('fetchPRStatusesForRepo matcher', () => {
444444
expect(result.get('/wt')?.number).toBe(200)
445445
})
446446

447+
it('does not claim a PR for a worktree on the repo default branch (main)', async () => {
448+
// A worktree sitting on main shouldn't be reported as "this PR" just
449+
// because the search index for main's HEAD SHA happens to surface the
450+
// most-recently-squashed PR. Same for master.
451+
const mainSha = 'a'.repeat(40)
452+
const mergedPRHeadSha = 'b'.repeat(40)
453+
fetchSpy.mockResolvedValueOnce(
454+
mockResponse(200, {
455+
data: {
456+
repository: {
457+
defaultBranchRef: { name: 'main' },
458+
milestones: { totalCount: 0 },
459+
prBr0: { nodes: [] }
460+
},
461+
// GitHub's search index returns the just-merged PR when you
462+
// search the squash commit's SHA — its headRefOid is the
463+
// original PR's head, not main's HEAD.
464+
prSearch0: {
465+
nodes: [
466+
gqlPR({
467+
number: 999,
468+
title: 'Some merged feature',
469+
state: 'MERGED',
470+
headRefOid: mergedPRHeadSha
471+
})
472+
]
473+
}
474+
}
475+
})
476+
)
477+
const result = await fetchPRStatusesForRepo(
478+
{ origin: { owner: 'o', repo: 'r' }, upstream: { owner: 'o', repo: 'r' } },
479+
[{ worktreePath: '/wt', branch: 'main', headSha: mainSha }]
480+
)
481+
expect(result.get('/wt')).toBeNull()
482+
})
483+
484+
it('does not claim a PR for a worktree on master when that is the default branch', async () => {
485+
const masterSha = 'c'.repeat(40)
486+
const mergedPRHeadSha = 'd'.repeat(40)
487+
fetchSpy.mockResolvedValueOnce(
488+
mockResponse(200, {
489+
data: {
490+
repository: {
491+
defaultBranchRef: { name: 'master' },
492+
milestones: { totalCount: 0 },
493+
prBr0: { nodes: [] }
494+
},
495+
prSearch0: {
496+
nodes: [gqlPR({ number: 999, state: 'MERGED', headRefOid: mergedPRHeadSha })]
497+
}
498+
}
499+
})
500+
)
501+
const result = await fetchPRStatusesForRepo(
502+
{ origin: { owner: 'o', repo: 'r' }, upstream: { owner: 'o', repo: 'r' } },
503+
[{ worktreePath: '/wt', branch: 'master', headSha: masterSha }]
504+
)
505+
expect(result.get('/wt')).toBeNull()
506+
})
507+
447508
it('dedupes when both lookups return the same PR', async () => {
448509
const sha = 'f'.repeat(40)
449510
fetchSpy.mockResolvedValueOnce(

src/main/github.ts

Lines changed: 36 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -444,9 +444,17 @@ ${PR_FRAGMENT}`
444444

445445
const hasMilestones = (repoData.milestones?.totalCount ?? 0) > 0
446446

447+
const defaultBranchName = repoData.defaultBranchRef?.name ?? ''
448+
447449
// Resolve per-request, then fetch behind_by + first-release-tag in parallel.
448450
const built = await Promise.all(
449451
queryable.map(async (req, i) => {
452+
// A worktree sitting on the repo's default branch (main/master) is
453+
// not the head of any PR — skip the resolution entirely to avoid
454+
// misattributing the latest squash-merged PR's status to it.
455+
if (defaultBranchName && req.branch === defaultBranchName) {
456+
return { worktreePath: req.worktreePath, status: null as PRStatus | null }
457+
}
450458
const brAlias = repoData[`prBr${i}`] as { nodes: GraphQLPR[] | null } | null | undefined
451459
const searchAlias = topData[`prSearch${i}`] as
452460
| { nodes: Array<GraphQLPR | { __typename?: string }> | null }
@@ -457,8 +465,7 @@ ${PR_FRAGMENT}`
457465
const searchNodes = (searchAlias?.nodes ?? []).filter(
458466
(n): n is GraphQLPR => !!n && typeof (n as GraphQLPR).number === 'number'
459467
)
460-
const nodes = dedupePRsByNumber([...branchNodes, ...searchNodes])
461-
const pr = pickPRBySha(nodes, req.headSha, originFull)
468+
const pr = resolvePRForWorktree(branchNodes, searchNodes, req.headSha, originFull)
462469
if (!pr) return { worktreePath: req.worktreePath, status: null as PRStatus | null }
463470
const [behindBy, firstReleaseTag] = await Promise.all([
464471
pr.state === 'MERGED' || pr.state === 'CLOSED'
@@ -476,33 +483,37 @@ ${PR_FRAGMENT}`
476483
return result
477484
}
478485

479-
/** Combine PR nodes from the branch-name and SHA-based lookups, keeping
480-
* the first occurrence per PR number. */
481-
function dedupePRsByNumber(nodes: GraphQLPR[]): GraphQLPR[] {
482-
const seen = new Set<number>()
483-
const out: GraphQLPR[] = []
484-
for (const n of nodes) {
485-
if (seen.has(n.number)) continue
486-
seen.add(n.number)
487-
out.push(n)
488-
}
489-
return out
490-
}
491-
492-
/** Among the up-to-5 PR nodes returned for a single headRefName lookup,
493-
* pick the one most likely to belong to this worktree:
494-
* 1. exact SHA match against the worktree's HEAD,
495-
* 2. same-origin head (PR opened from this repo, not a fork),
496-
* 3. fall back to the most-recently-updated (first node). */
497-
function pickPRBySha(nodes: GraphQLPR[], headSha: string, originFull: string): GraphQLPR | null {
498-
if (nodes.length === 0) return null
486+
/** Resolve which PR (if any) belongs to a given worktree.
487+
*
488+
* Branch-name nodes come from `pullRequests(headRefName: $branch)` — the
489+
* filter is by ref, so any same-origin hit is a legitimate match for
490+
* this branch. We prefer an exact SHA match if available, then
491+
* same-origin, then the most-recently-updated.
492+
*
493+
* Search nodes come from `search("type:pr repo:o/n <sha>")` — the index
494+
* matches the SHA appearing anywhere in the PR's commit history or
495+
* body, including squash-merge commits that land on the default branch.
496+
* That's too loose to trust on its own, so we require the candidate's
497+
* `headRefOid` to equal the worktree's HEAD SHA before accepting it. */
498+
function resolvePRForWorktree(
499+
branchNodes: GraphQLPR[],
500+
searchNodes: GraphQLPR[],
501+
headSha: string,
502+
originFull: string
503+
): GraphQLPR | null {
499504
if (headSha) {
500-
const bySha = nodes.find((n) => n.headRefOid === headSha)
505+
const bySha =
506+
branchNodes.find((n) => n.headRefOid === headSha) ??
507+
searchNodes.find((n) => n.headRefOid === headSha)
501508
if (bySha) return bySha
502509
}
503-
const sameRepo = nodes.find((n) => n.headRepository?.nameWithOwner === originFull)
510+
// No SHA match. Same-origin branch-name hit is still a legitimate
511+
// match (worktree slightly behind the PR head, or PR force-pushed).
512+
// Cross-fork branch-name hits are ignored — `feature/foo` on someone
513+
// else's fork is not our PR.
514+
const sameRepo = branchNodes.find((n) => n.headRepository?.nameWithOwner === originFull)
504515
if (sameRepo) return sameRepo
505-
return nodes[0]
516+
return null
506517
}
507518

508519
function buildPRStatus(

0 commit comments

Comments
 (0)