From bc65c5866a5a1da430b9d18ce146da8b8efb3898 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 28 Jun 2026 16:09:53 -0700 Subject: [PATCH] Propagate upstream errors from branch-based PR discovery Previously, transient errors during candidate branch discovery (such as rate limits or network issues) were silently ignored, leading to a false "no-pr" result and causing the sidebar PR state to flicker. Now, track and return any pending error encountered during branch lookups, propagating it as an upstream error if no PR is successfully recovered. --- src/main/github/client.test.ts | 210 +++++++++++++++++++++++++++++++++ src/main/github/client.ts | 56 +++++++-- 2 files changed, 255 insertions(+), 11 deletions(-) diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index 03de78343b4..d0d001764ed 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -106,6 +106,7 @@ import { checkOrcaStarred, getPRComments, getPRForBranch, + getPRForBranchOutcome, getRepoUpstream, getWorkItem, getPullRequestPushTarget, @@ -835,6 +836,215 @@ describe('getPRForBranch', () => { expect(pr).toMatchObject({ number: 42, title: 'Fallback PR lookup' }) }) + it('reports upstream error when fallback branch discovery fails transiently then retry misses', async () => { + resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ + candidates: [{ owner: 'stablyai', repo: 'orca' }], + headRepo: null + }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('HTTP 429: API rate limit exceeded')) + .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) + + const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test') + + expect(outcome).toMatchObject({ + kind: 'upstream-error', + errorType: 'rate_limited' + }) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 1, + [ + 'pr', + 'list', + '--repo', + 'stablyai/orca', + '--head', + 'feature/test', + '--state', + 'all', + '--limit', + '1', + '--json', + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + ], + { cwd: '/repo-root' } + ) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 2, + ['api', 'repos/stablyai/orca/pulls?head=stablyai%3Afeature%2Ftest&state=all&per_page=1'], + { cwd: '/repo-root' } + ) + }) + + it('reports no PR when fallback branch discovery cleanly misses', async () => { + resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ + candidates: [{ owner: 'stablyai', repo: 'orca' }], + headRepo: null + }) + ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: JSON.stringify([]) }) + + const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test') + + expect(outcome.kind).toBe('no-pr') + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + + it('returns found when fallback branch discovery retry finds the PR', async () => { + resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ + candidates: [{ owner: 'stablyai', repo: 'orca' }], + headRepo: null + }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('HTTP 429: API rate limit exceeded')) + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 42, + title: 'Retry branch PR', + state: 'open', + html_url: 'https://github.com/stablyai/orca/pull/42', + updated_at: '2026-03-28T00:00:00Z', + draft: false, + mergeable: true, + base: { ref: 'main', sha: 'base-oid' }, + head: { ref: 'feature/test', sha: 'retry-head-oid' } + } + ]) + }) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 42, + title: 'Hydrated retry branch PR', + state: 'OPEN', + url: 'https://github.com/stablyai/orca/pull/42', + statusCheckRollup: [], + updatedAt: '2026-03-28T00:00:00Z', + isDraft: false, + mergeable: 'MERGEABLE', + baseRefName: 'main', + headRefName: 'feature/test', + baseRefOid: 'base-oid', + headRefOid: 'retry-head-oid' + }) + }) + + const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test') + + expect(outcome).toMatchObject({ + kind: 'found', + pr: { + number: 42, + title: 'Hydrated retry branch PR', + prRepo: { owner: 'stablyai', repo: 'orca' } + } + }) + }) + + it('lets fallback PR number recovery win after fallback branch queries throw', async () => { + resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ + candidates: [{ owner: 'stablyai', repo: 'orca' }], + headRepo: null + }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('HTTP 429: API rate limit exceeded')) + .mockRejectedValueOnce(new Error('HTTP 502: Bad Gateway')) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 42, + title: 'Fallback number recovered PR', + state: 'OPEN', + url: 'https://github.com/stablyai/orca/pull/42', + statusCheckRollup: [], + updatedAt: '2026-03-28T00:00:00Z', + isDraft: false, + mergeable: 'MERGEABLE', + baseRefName: 'main', + headRefName: 'feature/test', + baseRefOid: 'base-oid', + headRefOid: 'fallback-head-oid' + }) + }) + + const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test', null, null, 42) + + expect(outcome).toMatchObject({ + kind: 'found', + pr: { + number: 42, + title: 'Fallback number recovered PR' + } + }) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 3, + [ + 'pr', + 'view', + '42', + '--repo', + 'stablyai/orca', + '--json', + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,reviewDecision,mergeStateStatus,autoMergeRequest,baseRefName,headRefName,baseRefOid,headRefOid' + ], + { cwd: '/repo-root' } + ) + }) + + it('reports upstream error when fallback branch discovery has a network failure', async () => { + resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ + candidates: [{ owner: 'stablyai', repo: 'orca' }], + headRepo: null + }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('could not resolve host: api.github.com')) + .mockRejectedValueOnce(new Error('could not resolve host: api.github.com')) + + const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test') + + expect(outcome).toMatchObject({ + kind: 'upstream-error', + errorType: 'network' + }) + }) + + it('keeps a pending fallback branch error when a later candidate cleanly misses', async () => { + resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ + candidates: [ + { owner: 'stablyai', repo: 'orca' }, + { owner: 'fork', repo: 'orca' } + ], + headRepo: null + }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('HTTP 429: API rate limit exceeded')) + .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) + .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) + + const outcome = await getPRForBranchOutcome('/repo-root', 'feature/test') + + expect(outcome).toMatchObject({ + kind: 'upstream-error', + errorType: 'rate_limited' + }) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 3, + [ + 'pr', + 'list', + '--repo', + 'fork/orca', + '--head', + 'feature/test', + '--state', + 'all', + '--limit', + '1', + '--json', + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + ], + { cwd: '/repo-root' } + ) + }) + it('treats a merged branch lookup as a miss before using a fallback PR number', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 377c5f1312e..7285f9b1202 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -2554,8 +2554,14 @@ async function lookupPRByBranchName(args: { headRepo: OwnerRepo | null branchName: string ghOptions: GhExecOptions -}): Promise<{ data: PullRequestLookupData | null; dataRepo: OwnerRepo | null }> { +}): Promise<{ + data: PullRequestLookupData | null + dataRepo: OwnerRepo | null + pendingError?: unknown +}> { if (args.candidates.length > 0) { + let pendingError: unknown + let hasPendingError = false for (const candidate of args.candidates) { try { const branchData = args.headRepo @@ -2576,19 +2582,34 @@ async function lookupPRByBranchName(args: { if (args.headRepo) { throw err } - const branchData = await getRestPRForBranch( - candidate, - candidate.owner, - args.branchName, - args.ghOptions - ) - const data = await hydrateBranchLookupWithExactPR(candidate, branchData, args.ghOptions) - if (data) { - return { data, dataRepo: candidate } + if (!hasPendingError) { + pendingError = err + hasPendingError = true + } + try { + const branchData = await getRestPRForBranch( + candidate, + candidate.owner, + args.branchName, + args.ghOptions + ) + const data = await hydrateBranchLookupWithExactPR(candidate, branchData, args.ghOptions) + if (data) { + return { data, dataRepo: candidate } + } + } catch (retryErr) { + if (!hasPendingError) { + pendingError = retryErr + hasPendingError = true + } } } } - return { data: null, dataRepo: null } + // Why: branch-list failures are ambiguous for fork discovery, but exact + // fallback-number recovery should still get a chance before surfacing error. + return hasPendingError + ? { data: null, dataRepo: null, pendingError } + : { data: null, dataRepo: null } } try { @@ -2778,6 +2799,8 @@ export async function getPRForBranchOutcome( let data: PullRequestLookupData | null = null let dataRepo: OwnerRepo | null = null let dataHeadRepo: OwnerRepo | null = headRepo + let pendingBranchLookupError: unknown + let hasPendingBranchLookupError = false let currentHeadOidForMergedImplicit: string | null | undefined const explicitCurrentHeadOid = @@ -2817,6 +2840,10 @@ export async function getPRForBranchOutcome( }) data = branchLookup.data dataRepo = branchLookup.dataRepo + if ('pendingError' in branchLookup) { + pendingBranchLookupError = branchLookup.pendingError + hasPendingBranchLookupError = true + } if (!data) { // Why: the tracked upstream can identify the real PR head by branch // name or by fork owner even when branch names match locally. @@ -2846,6 +2873,10 @@ export async function getPRForBranchOutcome( }) data = upstreamLookup.data dataRepo = upstreamLookup.dataRepo + if (!hasPendingBranchLookupError && 'pendingError' in upstreamLookup) { + pendingBranchLookupError = upstreamLookup.pendingError + hasPendingBranchLookupError = true + } if (data) { dataHeadRepo = upstreamHeadRepo } @@ -2870,6 +2901,9 @@ export async function getPRForBranchOutcome( dataRepo = fallbackLookup.dataRepo } if (!data) { + if (hasPendingBranchLookupError) { + return prRefreshUpstreamError(pendingBranchLookupError) + } return { kind: 'no-pr', fetchedAt: Date.now() } } const fallbackConfirmedMergedBranch =