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.
2026-10-25 08:04:05 +00:00 · 2026-06-28 16:09:53 -07:00
parent 1b8e7c5bb5
commit bc65c5866a
2 changed files with 255 additions and 11 deletions
@@ -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
@@ -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 =