Harden stale PR recovery across refresh races

This commit is contained in:
Brennan Benson
2026-07-14 23:15:31 -07:00
parent 8771b39f55
commit fa2a8f5e91
7 changed files with 767 additions and 104 deletions
+5 -1
View File
@@ -926,7 +926,11 @@ describe('getPRForBranch', () => {
expect(gitExecFileAsyncMock).not.toHaveBeenCalled()
expect(outcome).toMatchObject({
kind: 'found',
pr: { number: 6012, confirmedContainedHeadOid: 'cccc3333cccc3333' }
pr: {
number: 6012,
headRefName: 'fix-hibernation-wake',
confirmedContainedHeadOid: 'cccc3333cccc3333'
}
})
})
+291 -1
View File
@@ -3127,6 +3127,287 @@ describe('createGitHubSlice.fetchPRForBranch', () => {
expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedPR).toBeNull()
})
it('clears a branch-mismatched linked open PR from an accepted coordinator event', () => {
const store = createTestStore()
const repoPath = '/repo'
const repoId = 'repo-1'
const branch = 'feature/current'
const worktreeId = 'wt-coordinator-open-mismatch'
const cacheKey = `${repoId}::${branch}`
const worktree = makePRRefreshWorktree({
id: worktreeId,
repoId,
branch,
head: 'current-head',
linkedPR: 12
})
const updateWorktreeMeta = installLinkedPRClearStub(store, {
repoId,
repoPath,
branch,
worktree
})
store.getState().applyGitHubPRRefreshEvent({
sequence: 1,
reason: 'swr',
aliases: [
{
cacheKey,
repoPath,
repoId,
branch,
worktreeId,
linkedPRNumber: 12,
currentHeadOid: 'current-head'
}
],
outcome: {
kind: 'found',
pr: makePR({ number: 12, state: 'open', headSha: 'old-head', headRefName: 'feature/old' }),
fetchedAt: 2
}
})
expect(updateWorktreeMeta).toHaveBeenCalledWith(
worktreeId,
{ linkedPR: null },
{ shouldApply: expect.any(Function) }
)
expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedPR).toBeNull()
})
it('does not scan worktrees for a found coordinator outcome without a durable PR link', () => {
const store = createTestStore()
const repoPath = '/repo'
const repoId = 'repo-1'
const branch = 'feature/unlinked'
let worktreeIdReads = 0
const worktrees = Array.from({ length: 100 }, (_, index) => {
const worktree = makePRRefreshWorktree({
id: `wt-${index}`,
repoId,
branch: `feature/${index}`,
head: `head-${index}`,
linkedPR: null
})
Object.defineProperty(worktree, 'id', {
enumerable: true,
get: () => {
worktreeIdReads += 1
return `wt-${index}`
}
})
return worktree
})
store.setState({
repos: [{ id: repoId, path: repoPath, name: 'repo', kind: 'git' }],
worktreesByRepo: { [repoId]: worktrees }
} as unknown as Partial<AppState>)
worktreeIdReads = 0
store.getState().applyGitHubPRRefreshEvent({
sequence: 1,
reason: 'swr',
aliases: [
{
cacheKey: `${repoId}::${branch}`,
repoPath,
repoId,
branch,
worktreeId: 'wt-0',
linkedPRNumber: null,
currentHeadOid: 'head-0'
}
],
outcome: {
kind: 'found',
pr: makePR({ number: 12, state: 'open', headRefName: branch }),
fetchedAt: 2
}
})
expect(worktreeIdReads).toBe(0)
})
it('does not unlink from a branch-mismatched outcome rejected by the sequence gate', () => {
const store = createTestStore()
const repoPath = '/repo'
const repoId = 'repo-1'
const branch = 'feature/current'
const worktreeId = 'wt-coordinator-stale-sequence'
const cacheKey = `${repoId}::${branch}`
const worktree = makePRRefreshWorktree({
id: worktreeId,
repoId,
branch,
head: 'current-head',
linkedPR: 12
})
const updateWorktreeMeta = installLinkedPRClearStub(store, {
repoId,
repoPath,
branch,
worktree
})
const aliases = [
{
cacheKey,
repoPath,
repoId,
branch,
worktreeId,
linkedPRNumber: 12,
currentHeadOid: 'current-head'
}
]
store.getState().applyGitHubPRRefreshEvent({
sequence: 2,
reason: 'swr',
aliases,
outcome: {
kind: 'found',
pr: makePR({ number: 12, state: 'open', headRefName: branch }),
fetchedAt: 3
}
})
store.getState().applyGitHubPRRefreshEvent({
sequence: 1,
reason: 'swr',
aliases,
outcome: {
kind: 'found',
pr: makePR({ number: 12, state: 'open', headSha: 'old-head', headRefName: 'feature/old' }),
fetchedAt: 4
}
})
expect(updateWorktreeMeta).not.toHaveBeenCalled()
expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedPR).toBe(12)
expect(store.getState().prRefreshSequences[cacheKey]).toBe(2)
})
it('does not unlink an ambiguous worktree id shared by multiple hosts', () => {
const store = createTestStore()
const repoPath = '/repo'
const repoId = 'repo-1'
const branch = 'feature/current'
const worktreeId = 'repo-1::/same/path'
const cacheKey = `${repoId}::${branch}`
const updateWorktreeMeta = vi.fn()
store.setState({
repos: [{ id: repoId, path: repoPath, name: 'repo', kind: 'git' }],
worktreesByRepo: {
[repoId]: [
makePRRefreshWorktree({
id: worktreeId,
repoId,
branch,
head: 'local-head',
linkedPR: 12,
hostId: 'local'
}),
makePRRefreshWorktree({
id: worktreeId,
repoId,
branch,
head: 'ssh-head',
linkedPR: 12,
hostId: 'ssh:ssh-1'
})
]
},
updateWorktreeMeta
} as unknown as Partial<AppState>)
store.getState().applyGitHubPRRefreshEvent({
sequence: 1,
reason: 'swr',
aliases: [
{
cacheKey,
repoPath,
repoId,
branch,
worktreeId,
linkedPRNumber: 12,
currentHeadOid: 'local-head'
}
],
outcome: {
kind: 'found',
pr: makePR({ number: 12, state: 'open', headSha: 'old-head', headRefName: 'feature/old' }),
fetchedAt: 2
}
})
expect(updateWorktreeMeta).not.toHaveBeenCalled()
expect(store.getState().worktreesByRepo[repoId]).toHaveLength(2)
expect(
store.getState().worktreesByRepo[repoId]?.every((worktree) => worktree.linkedPR === 12)
).toBe(true)
})
it('does not let an old-host coordinator event unlink a row now owned by another host', () => {
const store = createTestStore()
const repoPath = '/repo'
const repoId = 'repo-1'
const branch = 'feature/current'
const worktreeId = 'repo-1::/same/path'
const cacheKey = `ssh:ssh-1::${repoId}::${branch}`
const updateWorktreeMeta = vi.fn()
store.setState({
repos: [
{
id: repoId,
path: repoPath,
name: 'repo',
kind: 'git',
connectionId: 'ssh-2'
}
],
worktreesByRepo: {
[repoId]: [
makePRRefreshWorktree({
id: worktreeId,
repoId,
branch,
head: 'ssh-2-head',
linkedPR: 12,
hostId: 'ssh:ssh-2'
})
]
},
updateWorktreeMeta
} as unknown as Partial<AppState>)
store.getState().applyGitHubPRRefreshEvent({
sequence: 1,
reason: 'swr',
aliases: [
{
cacheKey,
repoPath,
repoId,
branch,
worktreeId,
linkedPRNumber: 12,
currentHeadOid: 'old-host-head',
executionHostId: 'ssh:ssh-1'
}
],
outcome: {
kind: 'found',
pr: makePR({ number: 12, state: 'open', headSha: 'old-head', headRefName: 'feature/old' }),
fetchedAt: 2
}
})
expect(updateWorktreeMeta).not.toHaveBeenCalled()
expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedPR).toBe(12)
})
it('does not clear a linked merged PR from a coordinator refresh event without a request head', () => {
const store = createTestStore()
const repoPath = '/repo'
@@ -6555,6 +6836,12 @@ describe('shouldClearBranchMismatchedLinkedOpenPR', () => {
expect(shouldClearBranchMismatchedLinkedOpenPR(baseArgs)).toBe(true)
})
it('clears when the mismatched linked PR is a draft', () => {
expect(
shouldClearBranchMismatchedLinkedOpenPR({ ...baseArgs, pr: basePR({ state: 'draft' }) })
).toBe(true)
})
it('strips refs/heads/ from the worktree branch before comparing', () => {
expect(
shouldClearBranchMismatchedLinkedOpenPR({ ...baseArgs, branch: 'refs/heads/feature-one' })
@@ -6582,7 +6869,7 @@ describe('shouldClearBranchMismatchedLinkedOpenPR', () => {
)
})
it('only applies to open PRs', () => {
it('only applies to active PRs', () => {
expect(
shouldClearBranchMismatchedLinkedOpenPR({ ...baseArgs, pr: basePR({ state: 'merged' }) })
).toBe(false)
@@ -6600,6 +6887,9 @@ describe('shouldClearBranchMismatchedLinkedOpenPR', () => {
).toBe(false)
// Detached HEAD reports an empty branch; there is no mismatch to act on.
expect(shouldClearBranchMismatchedLinkedOpenPR({ ...baseArgs, branch: '' })).toBe(false)
expect(shouldClearBranchMismatchedLinkedOpenPR({ ...baseArgs, requestHeadOid: null })).toBe(
false
)
})
it('ignores lookups that resolved a different PR than the link', () => {
+148 -58
View File
@@ -994,6 +994,45 @@ function findWorktreeById(state: AppState, worktreeId: string): Worktree | null
return null
}
function findUniqueWorktreeById(
state: AppState,
worktreeId: string,
executionHostId?: string
): Worktree | null {
let match: Worktree | null = null
for (const worktrees of Object.values(state.worktreesByRepo)) {
for (const worktree of worktrees) {
if (worktree.id !== worktreeId) {
continue
}
if (match) {
// Why: metadata persistence is keyed only by worktree id. If two hosts
// own that id, a destructive linked-PR clear cannot be routed safely.
return null
}
match = worktree
}
}
if (!match || executionHostId === undefined) {
return match
}
const expectedHostId = normalizeExecutionHostId(executionHostId) ?? LOCAL_EXECUTION_HOST_ID
const explicitWorktreeHostId = normalizeExecutionHostId(match.hostId)
if (explicitWorktreeHostId) {
return explicitWorktreeHostId === expectedHostId ? match : null
}
const repoHostIds = new Set(
state.repos
.filter((repo) => repo.id === match.repoId)
.map((repo) => getRepoExecutionHostId(repo))
)
// Pre-host persisted rows are safe only when their repo has one unambiguous owner.
if (repoHostIds.size !== 1 || !repoHostIds.has(expectedHostId)) {
return null
}
return match
}
function isStaleExactLinkedPRLookup(
state: AppState,
worktreeId: string | undefined,
@@ -1044,8 +1083,8 @@ function shouldApplyDivergedLinkedPRClear(args: {
}
// Why: a linked PR is a branch-scoped hint. When a lookup returns the linked
// OPEN PR while the worktree sits on a different branch — and neither the push
// target nor the worktree HEAD still points at the PR head — the link is stale
// open or draft PR while the worktree sits on a different branch — and neither
// the push target nor the worktree HEAD still points at the PR head — the link is stale
// (a branch switch whose identity-path clear was missed) and would otherwise
// pin Checks to the previous branch's PR on every refresh.
export function shouldClearBranchMismatchedLinkedOpenPR(args: {
@@ -1061,29 +1100,35 @@ export function shouldClearBranchMismatchedLinkedOpenPR(args: {
return (
linkedPRNumber != null &&
pr?.number === linkedPRNumber &&
pr.state === 'open' &&
// Draft reviews are open PRs too; their distinct renderer state must not
// leave the same stale durable link permanently wedged after a branch switch.
(pr.state === 'open' || pr.state === 'draft') &&
requestHeadOid !== null &&
headRefName !== '' &&
currentBranch !== '' &&
headRefName !== currentBranch &&
(pushTargetBranch === null || pushTargetBranch !== headRefName) &&
// A worktree parked on the PR's own head commit is the same line of work
// (e.g. a PR checkout under a renamed local branch); keep the link.
!(pr.headSha != null && requestHeadOid !== null && pr.headSha === requestHeadOid)
!(pr.headSha != null && pr.headSha === requestHeadOid)
)
}
function shouldApplyBranchMismatchedLinkedPRClear(args: {
worktree: Pick<Worktree, 'linkedPR' | 'branch' | 'isBare' | 'isArchived'> | undefined
worktree: Pick<Worktree, 'linkedPR' | 'branch' | 'head' | 'isBare' | 'isArchived'> | undefined
linkedPRNumber: number
branch: string
requestHeadOid: string | null
}): boolean {
const { worktree, linkedPRNumber, branch } = args
const { worktree, linkedPRNumber, branch, requestHeadOid } = args
return (
Boolean(worktree) &&
requestHeadOid !== null &&
worktree?.linkedPR === linkedPRNumber &&
// Branch-scoped: only clear while the worktree is still on the branch the
// mismatch was computed against; a newer switch gets its own validation.
worktree.branch.replace(/^refs\/heads\//, '') === branch.replace(/^refs\/heads\//, '') &&
worktree.head === requestHeadOid &&
worktree.isBare !== true &&
worktree.isArchived !== true
)
@@ -1361,17 +1406,23 @@ function shouldPreserveExistingPRForFallbackMiss(args: {
fallbackPRNumber?: number | null
fallbackPRSource?: GitHubPRFallbackSource | null
}): boolean {
if (
args.nextPR !== null ||
args.linkedPRNumber != null ||
args.currentPR?.state !== 'merged' ||
typeof args.currentPR.headSha !== 'string' ||
args.currentPR.headSha.length === 0
) {
return false
}
// Why: the common found/non-merged paths do not depend on worktree state.
// Gate the global lookup so batched refresh aliases do not multiply full scans.
const worktree = args.worktreeId ? findWorktreeById(args.state, args.worktreeId) : null
const worktreeHead = worktree?.head
// Why: merged branch PRs are only safe to keep when cached PR metadata still
// matches the commit this stored worktree is actually on — exactly, or via a
// head confirmed to be part of the merged PR.
const preservesMergedPRForCurrentHead =
args.nextPR === null &&
args.linkedPRNumber == null &&
args.currentPR?.state === 'merged' &&
typeof args.currentPR.headSha === 'string' &&
args.currentPR.headSha.length > 0 &&
typeof worktreeHead === 'string' &&
worktreeHead.length > 0 &&
(args.currentPR.headSha === worktreeHead ||
@@ -3179,8 +3230,17 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
if (didUpdatePRCache) {
debouncedSaveCache(get())
}
const linkedPRWorktree =
options?.worktreeId && linkedPRNumber != null
? findUniqueWorktreeById(
get(),
options.worktreeId,
repo ? getRepoExecutionHostId(repo) : LOCAL_EXECUTION_HOST_ID
)
: null
if (
options?.worktreeId &&
linkedPRWorktree &&
linkedPRNumber != null &&
shouldClearDivergedLinkedMergedPR({ pr, linkedPRNumber, requestHeadOid })
) {
@@ -3190,9 +3250,14 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
options.worktreeId,
{ linkedPR: null },
{
shouldApply: (worktree) =>
shouldApply: () =>
shouldApplyDivergedLinkedPRClear({
worktree,
worktree:
findUniqueWorktreeById(
get(),
options.worktreeId!,
repo ? getRepoExecutionHostId(repo) : LOCAL_EXECUTION_HOST_ID
) ?? undefined,
linkedPRNumber,
branch,
requestHeadOid
@@ -3202,22 +3267,32 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
}
if (
options?.worktreeId &&
linkedPRWorktree &&
linkedPRNumber != null &&
shouldClearBranchMismatchedLinkedOpenPR({
pr,
linkedPRNumber,
branch,
requestHeadOid,
pushTargetBranch:
findWorktreeById(get(), options.worktreeId)?.pushTarget?.branchName ?? null
pushTargetBranch: linkedPRWorktree.pushTarget?.branchName ?? null
})
) {
void get().updateWorktreeMeta(
options.worktreeId,
{ linkedPR: null },
{
shouldApply: (worktree) =>
shouldApplyBranchMismatchedLinkedPRClear({ worktree, linkedPRNumber, branch })
shouldApply: () =>
shouldApplyBranchMismatchedLinkedPRClear({
worktree:
findUniqueWorktreeById(
get(),
options.worktreeId!,
repo ? getRepoExecutionHostId(repo) : LOCAL_EXECUTION_HOST_ID
) ?? undefined,
linkedPRNumber,
branch,
requestHeadOid
})
}
)
// Re-resolve by branch right away so visible Checks recover on this
@@ -3955,48 +4030,15 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
linkedPRNumber: number
branch: string
requestHeadOid: string | null
executionHostId: string
}[] = []
const branchMismatchedLinkedPRClears: {
worktreeId: string
linkedPRNumber: number
branch: string
requestHeadOid: string | null
executionHostId: string
}[] = []
if (event.outcome?.kind === 'found') {
const pr = event.outcome.pr
for (const alias of event.aliases) {
const linkedPRNumber = alias.linkedPRNumber ?? null
const requestHeadOid = alias.currentHeadOid ?? null
if (!alias.worktreeId || linkedPRNumber == null) {
continue
}
if (shouldClearDivergedLinkedMergedPR({ pr, linkedPRNumber, requestHeadOid })) {
divergedLinkedPRClears.push({
worktreeId: alias.worktreeId,
linkedPRNumber,
branch: alias.branch,
requestHeadOid
})
} else if (
shouldClearBranchMismatchedLinkedOpenPR({
pr,
linkedPRNumber,
branch: alias.branch,
requestHeadOid,
pushTargetBranch:
findWorktreeById(get(), alias.worktreeId)?.pushTarget?.branchName ?? null
})
) {
// Why: the background PR coordinator also refreshes with the durable
// link; without this clear a stale-linked worktree never re-resolves
// by branch on its own.
branchMismatchedLinkedPRClears.push({
worktreeId: alias.worktreeId,
linkedPRNumber,
branch: alias.branch
})
}
}
}
let didUpdatePRCache = false
set((s) => {
const nextSequences = { ...s.prRefreshSequences }
@@ -4116,6 +4158,51 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
if (isStaleExactLinkedPRLookup(s, alias.worktreeId, alias.linkedPRNumber)) {
continue
}
if (event.outcome.kind === 'found' && alias.worktreeId) {
const linkedPRNumber = alias.linkedPRNumber ?? null
const requestHeadOid = alias.currentHeadOid ?? null
const worktree =
linkedPRNumber != null
? findUniqueWorktreeById(s, alias.worktreeId, aliasExecutionHostId)
: null
// Why: only an event that won the sequence gate above owns metadata
// side effects; rejected late outcomes must not unlink a newer PR.
if (
worktree &&
linkedPRNumber != null &&
shouldClearDivergedLinkedMergedPR({
pr: event.outcome.pr,
linkedPRNumber,
requestHeadOid
})
) {
divergedLinkedPRClears.push({
worktreeId: alias.worktreeId,
linkedPRNumber,
branch: alias.branch,
requestHeadOid,
executionHostId: aliasExecutionHostId
})
} else if (
worktree &&
linkedPRNumber != null &&
shouldClearBranchMismatchedLinkedOpenPR({
pr: event.outcome.pr,
linkedPRNumber,
branch: alias.branch,
requestHeadOid,
pushTargetBranch: worktree.pushTarget?.branchName ?? null
})
) {
branchMismatchedLinkedPRClears.push({
worktreeId: alias.worktreeId,
linkedPRNumber,
branch: alias.branch,
requestHeadOid,
executionHostId: aliasExecutionHostId
})
}
}
const nextCaches = applyGitHubPRResultToCaches({
prCache: nextPRCache,
hostedReviewCache: nextHostedReviewCache,
@@ -4198,9 +4285,10 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
clear.worktreeId,
{ linkedPR: null },
{
shouldApply: (worktree) =>
shouldApply: () =>
shouldApplyDivergedLinkedPRClear({
worktree,
worktree:
findUniqueWorktreeById(get(), clear.worktreeId, clear.executionHostId) ?? undefined,
linkedPRNumber: clear.linkedPRNumber,
branch: clear.branch,
requestHeadOid: clear.requestHeadOid
@@ -4213,11 +4301,13 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
clear.worktreeId,
{ linkedPR: null },
{
shouldApply: (worktree) =>
shouldApply: () =>
shouldApplyBranchMismatchedLinkedPRClear({
worktree,
worktree:
findUniqueWorktreeById(get(), clear.worktreeId, clear.executionHostId) ?? undefined,
linkedPRNumber: clear.linkedPRNumber,
branch: clear.branch
branch: clear.branch,
requestHeadOid: clear.requestHeadOid
})
}
)
@@ -27,14 +27,24 @@ function makeWorktree(overrides: Partial<Worktree> & { id: string }): Worktree {
}
const hasLinkedPR = (worktree: Worktree): boolean => worktree.linkedPR != null
const matchesAnyHost = (): boolean => true
describe('routeListingBranchSwitchesThroughGitIdentity', () => {
it('routes a listing-observed branch switch through updateWorktreeGitIdentity', () => {
const updateWorktreeGitIdentity = vi.fn()
const current = [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })]
routeListingBranchSwitchesThroughGitIdentity({
current: [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })],
incoming: [{ id: 'repo1::/path/wt1', branch: 'refs/heads/feature-two', head: 'def456' }],
requestStarted: current,
current,
incoming: [
makeWorktree({
id: 'repo1::/path/wt1',
branch: 'refs/heads/feature-two',
head: 'def456'
})
],
matchesRefreshHost: matchesAnyHost,
hasBranchScopedReviewContext: hasLinkedPR,
updateWorktreeGitIdentity
})
@@ -48,10 +58,19 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => {
it('does nothing when the branch is unchanged', () => {
const updateWorktreeGitIdentity = vi.fn()
const current = [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })]
routeListingBranchSwitchesThroughGitIdentity({
current: [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })],
incoming: [{ id: 'repo1::/path/wt1', branch: 'refs/heads/feature-one', head: 'def456' }],
requestStarted: current,
current,
incoming: [
makeWorktree({
id: 'repo1::/path/wt1',
branch: 'refs/heads/feature-one',
head: 'def456'
})
],
matchesRefreshHost: matchesAnyHost,
hasBranchScopedReviewContext: hasLinkedPR,
updateWorktreeGitIdentity
})
@@ -61,10 +80,19 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => {
it('skips entries without branch-scoped review context (stale-refetch protection)', () => {
const updateWorktreeGitIdentity = vi.fn()
const current = [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: null })]
routeListingBranchSwitchesThroughGitIdentity({
current: [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: null })],
incoming: [{ id: 'repo1::/path/wt1', branch: 'refs/heads/feature-two', head: 'def456' }],
requestStarted: current,
current,
incoming: [
makeWorktree({
id: 'repo1::/path/wt1',
branch: 'refs/heads/feature-two',
head: 'def456'
})
],
matchesRefreshHost: matchesAnyHost,
hasBranchScopedReviewContext: hasLinkedPR,
updateWorktreeGitIdentity
})
@@ -74,10 +102,19 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => {
it('skips incoming worktrees with no current entry (cold hydration)', () => {
const updateWorktreeGitIdentity = vi.fn()
const current = [makeWorktree({ id: 'repo1::/path/other', linkedPR: 101 })]
routeListingBranchSwitchesThroughGitIdentity({
current: [makeWorktree({ id: 'repo1::/path/other', linkedPR: 101 })],
incoming: [{ id: 'repo1::/path/wt1', branch: 'refs/heads/feature-two', head: 'def456' }],
requestStarted: current,
current,
incoming: [
makeWorktree({
id: 'repo1::/path/wt1',
branch: 'refs/heads/feature-two',
head: 'def456'
})
],
matchesRefreshHost: matchesAnyHost,
hasBranchScopedReviewContext: hasLinkedPR,
updateWorktreeGitIdentity
})
@@ -89,8 +126,16 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => {
const updateWorktreeGitIdentity = vi.fn()
routeListingBranchSwitchesThroughGitIdentity({
requestStarted: undefined,
current: undefined,
incoming: [{ id: 'repo1::/path/wt1', branch: 'refs/heads/feature-two', head: 'def456' }],
incoming: [
makeWorktree({
id: 'repo1::/path/wt1',
branch: 'refs/heads/feature-two',
head: 'def456'
})
],
matchesRefreshHost: matchesAnyHost,
hasBranchScopedReviewContext: hasLinkedPR,
updateWorktreeGitIdentity
})
@@ -100,10 +145,13 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => {
it('maps an empty listing branch to the explicit detached-HEAD signal', () => {
const updateWorktreeGitIdentity = vi.fn()
const current = [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })]
routeListingBranchSwitchesThroughGitIdentity({
current: [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })],
incoming: [{ id: 'repo1::/path/wt1', branch: '', head: 'def456' }],
requestStarted: current,
current,
incoming: [makeWorktree({ id: 'repo1::/path/wt1', branch: '', head: 'def456' })],
matchesRefreshHost: matchesAnyHost,
hasBranchScopedReviewContext: hasLinkedPR,
updateWorktreeGitIdentity
})
@@ -113,4 +161,114 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => {
branch: null
})
})
it('rejects a listing response when the branch changed again after the request started', () => {
const updateWorktreeGitIdentity = vi.fn()
const current = [
makeWorktree({
id: 'repo1::/path/wt1',
branch: 'refs/heads/feature-three',
head: 'newest-head',
linkedPR: 303
})
]
const reconciled = routeListingBranchSwitchesThroughGitIdentity({
requestStarted: [
makeWorktree({
id: 'repo1::/path/wt1',
branch: 'refs/heads/feature-one',
linkedPR: 101
})
],
current,
incoming: [
makeWorktree({
id: 'repo1::/path/wt1',
branch: 'refs/heads/feature-two',
head: 'stale-head'
})
],
matchesRefreshHost: matchesAnyHost,
hasBranchScopedReviewContext: hasLinkedPR,
updateWorktreeGitIdentity
})
expect(updateWorktreeGitIdentity).not.toHaveBeenCalled()
expect(reconciled).toEqual(current)
})
it('preserves a manual relink made while the listing request was in flight', () => {
const updateWorktreeGitIdentity = vi.fn()
const requestStarted = [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })]
const current = [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 303 })]
const reconciled = routeListingBranchSwitchesThroughGitIdentity({
requestStarted,
current,
incoming: [
makeWorktree({
id: 'repo1::/path/wt1',
branch: 'refs/heads/feature-two',
head: 'stale-head',
linkedPR: 101
})
],
matchesRefreshHost: matchesAnyHost,
hasBranchScopedReviewContext: hasLinkedPR,
updateWorktreeGitIdentity
})
expect(updateWorktreeGitIdentity).not.toHaveBeenCalled()
expect(reconciled).toEqual(current)
})
it('fails closed when the same worktree id belongs to multiple execution hosts', () => {
const updateWorktreeGitIdentity = vi.fn()
const requestStarted = [
makeWorktree({ id: 'repo1::/same/path', hostId: 'local', linkedPR: 101 }),
makeWorktree({ id: 'repo1::/same/path', hostId: 'ssh:ssh-1', linkedPR: 202 })
]
routeListingBranchSwitchesThroughGitIdentity({
requestStarted,
current: requestStarted,
incoming: [
makeWorktree({
id: 'repo1::/same/path',
branch: 'refs/heads/feature-two',
head: 'remote-head'
})
],
matchesRefreshHost: matchesAnyHost,
hasBranchScopedReviewContext: hasLinkedPR,
updateWorktreeGitIdentity
})
expect(updateWorktreeGitIdentity).not.toHaveBeenCalled()
})
it('does not route an old-host response through a row now owned by another host', () => {
const updateWorktreeGitIdentity = vi.fn()
routeListingBranchSwitchesThroughGitIdentity({
requestStarted: [
makeWorktree({ id: 'repo1::/same/path', hostId: 'ssh:ssh-1', linkedPR: 101 })
],
current: [makeWorktree({ id: 'repo1::/same/path', hostId: 'ssh:ssh-2', linkedPR: 202 })],
incoming: [
makeWorktree({
id: 'repo1::/same/path',
hostId: 'ssh:ssh-1',
branch: 'refs/heads/feature-two',
head: 'old-host-head'
})
],
matchesRefreshHost: (worktree) => worktree.hostId === 'ssh:ssh-1',
hasBranchScopedReviewContext: hasLinkedPR,
updateWorktreeGitIdentity
})
expect(updateWorktreeGitIdentity).not.toHaveBeenCalled()
})
})
@@ -1,5 +1,31 @@
import type { Worktree } from '../../../../shared/types'
function indexUnambiguousWorktrees(
worktrees: readonly Worktree[],
include?: (worktree: Worktree) => boolean
): Map<string, Worktree | null> {
const byId = new Map<string, Worktree | null>()
for (const worktree of worktrees) {
if (include && !include(worktree)) {
continue
}
byId.set(worktree.id, byId.has(worktree.id) ? null : worktree)
}
return byId
}
function branchScopedReviewContextMatches(left: Worktree, right: Worktree): boolean {
return (
left.linkedPR === right.linkedPR &&
left.linkedGitLabMR === right.linkedGitLabMR &&
left.linkedBitbucketPR === right.linkedBitbucketPR &&
left.linkedAzureDevOpsPR === right.linkedAzureDevOpsPR &&
left.linkedGiteaPR === right.linkedGiteaPR &&
left.pushTarget?.remoteName === right.pushTarget?.remoteName &&
left.pushTarget?.branchName === right.pushTarget?.branchName
)
}
/**
* Route branch switches observed by a worktree-listing refresh through
* `updateWorktreeGitIdentity` before the listing is merged into the store.
@@ -18,22 +44,52 @@ import type { Worktree } from '../../../../shared/types'
* listing row must not roll back a newer branch identity.
*/
export function routeListingBranchSwitchesThroughGitIdentity(args: {
requestStarted: readonly Worktree[] | undefined
current: readonly Worktree[] | undefined
incoming: readonly Pick<Worktree, 'id' | 'branch' | 'head'>[]
incoming: Worktree[]
matchesRefreshHost: (worktree: Worktree) => boolean
hasBranchScopedReviewContext: (worktree: Worktree) => boolean
updateWorktreeGitIdentity: (
worktreeId: string,
identity: { head?: string; branch?: string | null }
) => void
}): void {
const { current, incoming, hasBranchScopedReviewContext, updateWorktreeGitIdentity } = args
if (!current?.length) {
return
}): Worktree[] {
const {
requestStarted,
current,
incoming,
matchesRefreshHost,
hasBranchScopedReviewContext,
updateWorktreeGitIdentity
} = args
if (!requestStarted?.length || !current?.length) {
return incoming
}
const currentById = new Map(current.map((worktree) => [worktree.id, worktree]))
for (const worktree of incoming) {
const existing = currentById.get(worktree.id)
const allLatestById = indexUnambiguousWorktrees(current)
const startedById = indexUnambiguousWorktrees(requestStarted, matchesRefreshHost)
const latestById = indexUnambiguousWorktrees(current, matchesRefreshHost)
let reconciled: Worktree[] | null = null
for (const [index, worktree] of incoming.entries()) {
const requestStartedWorktree = startedById.get(worktree.id)
const existing = latestById.get(worktree.id)
const isAmbiguousAcrossHosts = allLatestById.get(worktree.id) === null
if (
requestStartedWorktree &&
existing &&
(existing.branch !== requestStartedWorktree.branch ||
existing.head !== requestStartedWorktree.head ||
!branchScopedReviewContextMatches(existing, requestStartedWorktree))
) {
// Why: rejecting the clear alone is insufficient; preserve the newer row
// so the subsequent listing merge cannot roll its branch and metadata back.
reconciled ??= [...incoming]
reconciled[index] = existing
continue
}
if (
isAmbiguousAcrossHosts ||
!requestStartedWorktree ||
!existing ||
existing.branch === worktree.branch ||
!hasBranchScopedReviewContext(existing)
@@ -47,4 +103,5 @@ export function routeListingBranchSwitchesThroughGitIdentity(args: {
branch: worktree.branch === '' ? null : worktree.branch
})
}
return reconciled ?? incoming
}
@@ -450,6 +450,47 @@ describe('fetchWorktrees', () => {
})
})
it('does not merge a stale listing row over a newer branch and review link', async () => {
const store = createTestStore()
const worktreeId = 'repo1::/path/wt1'
const requestStarted = makeWorktree({
id: worktreeId,
repoId: 'repo1',
path: '/path/wt1',
branch: 'refs/heads/feature-one',
head: 'first-head',
linkedPR: 101
})
const staleResponse = makeWorktree({
...requestStarted,
branch: 'refs/heads/feature-two',
head: 'stale-head',
linkedPR: 202
})
let resolveListing!: (worktrees: Worktree[]) => void
const listing = new Promise<Worktree[]>((resolve) => {
resolveListing = resolve
})
worktreeListMock.mockReturnValueOnce(listing)
store.setState({ worktreesByRepo: { repo1: [requestStarted] } } as Partial<AppState>)
const refresh = store.getState().fetchWorktrees('repo1')
await vi.waitFor(() => expect(worktreeListMock).toHaveBeenCalledTimes(1))
const latest = makeWorktree({
...requestStarted,
branch: 'refs/heads/feature-three',
head: 'latest-head',
linkedPR: 303
})
store.setState({ worktreesByRepo: { repo1: [latest] } } as Partial<AppState>)
resolveListing([staleResponse])
await refresh
expect(store.getState().worktreesByRepo.repo1).toEqual([latest])
expect(mockApi.worktrees.updateMeta).not.toHaveBeenCalled()
})
it('updates the repo entry when only the persisted base ref changes', async () => {
const store = createTestStore()
const existing = makeWorktree({
+48 -25
View File
@@ -2327,6 +2327,7 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
fetchWorktrees: async (repoId, options) => {
try {
const ownerState = get()
const requestStartedWorktrees = ownerState.worktreesByRepo[repoId]
const hostId = repoHostId(ownerState, repoId)
const ownerWasMissingAtStart = !ownerState.repos.some((repo) => repo.id === repoId)
const setup = getProjectHostSetupForRepoHost(ownerState, repoId, hostId)
@@ -2338,13 +2339,19 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
if (options?.requireAuthoritative && !detected.authoritative) {
return false
}
const incoming = toVisibleWorktrees(detected, hostId, setup)
routeListingBranchSwitchesThroughGitIdentity({
current: get().worktreesByRepo[repoId],
incoming,
hasBranchScopedReviewContext: hasBranchScopedHostedReviewContext,
updateWorktreeGitIdentity: get().updateWorktreeGitIdentity
})
let incoming = toVisibleWorktrees(detected, hostId, setup)
const latestState = get()
if (repoHasExecutionHost(latestState, repoId, hostId, ownerWasMissingAtStart)) {
const matchOptions = worktreeHostMatchOptions(latestState, repoId, hostId)
incoming = routeListingBranchSwitchesThroughGitIdentity({
requestStarted: requestStartedWorktrees,
current: latestState.worktreesByRepo[repoId],
incoming,
matchesRefreshHost: (worktree) => worktreeMatchesHost(worktree, hostId, matchOptions),
hasBranchScopedReviewContext: hasBranchScopedHostedReviewContext,
updateWorktreeGitIdentity: latestState.updateWorktreeGitIdentity
})
}
const current = get().worktreesByRepo[repoId]
const worktrees = sanitizeHostedReviewLinksForBranchClears(incoming, current)
const currentMatchOptions = worktreeHostMatchOptions(get(), repoId, hostId)
@@ -2482,20 +2489,28 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
if (get().hasHydratedWorktreePurge) {
await mapReposForWorktreeRefresh(repos, async (r) => {
try {
const requestStartedState = get()
const requestStartedWorktrees = requestStartedState.worktreesByRepo[r.id]
const hostId = getRepoExecutionHostId(r)
const setup = getProjectHostSetupForRepoHost(get(), r.id, hostId)
const settings = settingsForKnownRepoOwner(get().settings, r)
const setup = getProjectHostSetupForRepoHost(requestStartedState, r.id, hostId)
const settings = settingsForKnownRepoOwner(requestStartedState.settings, r)
const detected = await listDetectedWorktreesForRepoCoalesced(settings, r.id, {
executionHostId: hostId,
reuseRecentCompatibilityFailure: true
})
const incoming = toVisibleWorktrees(detected, hostId, setup)
routeListingBranchSwitchesThroughGitIdentity({
current: get().worktreesByRepo[r.id],
incoming,
hasBranchScopedReviewContext: hasBranchScopedHostedReviewContext,
updateWorktreeGitIdentity: get().updateWorktreeGitIdentity
})
let incoming = toVisibleWorktrees(detected, hostId, setup)
const latestState = get()
if (repoHasExecutionHost(latestState, r.id, hostId, false)) {
const matchOptions = worktreeHostMatchOptions(latestState, r.id, hostId)
incoming = routeListingBranchSwitchesThroughGitIdentity({
requestStarted: requestStartedWorktrees,
current: latestState.worktreesByRepo[r.id],
incoming,
matchesRefreshHost: (worktree) => worktreeMatchesHost(worktree, hostId, matchOptions),
hasBranchScopedReviewContext: hasBranchScopedHostedReviewContext,
updateWorktreeGitIdentity: latestState.updateWorktreeGitIdentity
})
}
const worktrees = sanitizeHostedReviewLinksForBranchClears(
incoming,
get().worktreesByRepo[r.id]
@@ -2568,20 +2583,28 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
| { repoId: string; ok: false }
> => {
try {
const requestStartedState = get()
const requestStartedWorktrees = requestStartedState.worktreesByRepo[r.id]
const hostId = getRepoExecutionHostId(r)
const setup = getProjectHostSetupForRepoHost(get(), r.id, hostId)
const setup = getProjectHostSetupForRepoHost(requestStartedState, r.id, hostId)
const detected = await listDetectedWorktreesForRepoCoalesced(
settingsForKnownRepoOwner(get().settings, r),
settingsForKnownRepoOwner(requestStartedState.settings, r),
r.id,
{ executionHostId: hostId, reuseRecentCompatibilityFailure: true }
)
const incoming = toVisibleWorktrees(detected, hostId, setup)
routeListingBranchSwitchesThroughGitIdentity({
current: get().worktreesByRepo[r.id],
incoming,
hasBranchScopedReviewContext: hasBranchScopedHostedReviewContext,
updateWorktreeGitIdentity: get().updateWorktreeGitIdentity
})
let incoming = toVisibleWorktrees(detected, hostId, setup)
const latestState = get()
if (repoHasExecutionHost(latestState, r.id, hostId, false)) {
const matchOptions = worktreeHostMatchOptions(latestState, r.id, hostId)
incoming = routeListingBranchSwitchesThroughGitIdentity({
requestStarted: requestStartedWorktrees,
current: latestState.worktreesByRepo[r.id],
incoming,
matchesRefreshHost: (worktree) => worktreeMatchesHost(worktree, hostId, matchOptions),
hasBranchScopedReviewContext: hasBranchScopedHostedReviewContext,
updateWorktreeGitIdentity: latestState.updateWorktreeGitIdentity
})
}
const current = get().worktreesByRepo[r.id]
const list = sanitizeHostedReviewLinksForBranchClears(incoming, current)
const currentMatchOptions = worktreeHostMatchOptions(get(), r.id, hostId)