From 66e70d501ac42cdbf0fcbcb9772043d2bc326648 Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Thu, 30 Apr 2026 14:34:40 -0700 Subject: [PATCH] fix(github): resolve PR for branches whose local and remote names diverge Two-stage resolver in a new branch-pr-resolution module: 1. Resolve @{upstream} and query gh by the remote branch name. 2. On miss, search by HEAD SHA and require headRefOid === headSha to reject stale/recycled branches. Fixes the silent "no PR" when a worktree's local branch differs from its pushed remote name (e.g. after git push origin HEAD:prefix/my-branch with no upstream set). getPRForBranch keeps its signature and becomes a shim; client.ts unchanged externally. Adds conflict-summary.test.ts restoring the three conflict-summary assertions that previously lived in client.test.ts. Co-authored-by: Orca --- docs/pr-for-branch-upstream-resolution.md | 348 ++++++++++++++++ src/main/github/branch-pr-resolution.test.ts | 407 +++++++++++++++++++ src/main/github/branch-pr-resolution.ts | 269 ++++++++++++ src/main/github/client.test.ts | 272 ++----------- src/main/github/client.ts | 100 +---- src/main/github/conflict-summary.test.ts | 67 +++ 6 files changed, 1128 insertions(+), 335 deletions(-) create mode 100644 docs/pr-for-branch-upstream-resolution.md create mode 100644 src/main/github/branch-pr-resolution.test.ts create mode 100644 src/main/github/branch-pr-resolution.ts create mode 100644 src/main/github/conflict-summary.test.ts diff --git a/docs/pr-for-branch-upstream-resolution.md b/docs/pr-for-branch-upstream-resolution.md new file mode 100644 index 00000000000..f006ad219ff --- /dev/null +++ b/docs/pr-for-branch-upstream-resolution.md @@ -0,0 +1,348 @@ +# PR discovery: resolve the branch-to-PR mapping before we give up + +## The bug + +Orca's "PR for this worktree" indicator silently shows nothing when +a worktree's **local** branch name differs from the **remote** branch +name it tracks. + +Concrete repro observed in workspace `node-pty-ci-failure`: + +- Local branch: `fix-pr-check-node-gyp-perms` +- Remote branch (on `origin`): `brennanb2025/fix-pr-check-node-gyp-perms` +- Open PR on GitHub: #733, `headRefName = brennanb2025/fix-pr-check-node-gyp-perms` + +Orca shows "no PR". `gh pr list --head fix-pr-check-node-gyp-perms` +returns `[]`; `gh pr list --head brennanb2025/fix-pr-check-node-gyp-perms` +returns PR #733. + +This happens whenever a user pushes with an explicit refspec, e.g. +`git push origin HEAD:brennanb2025/my-branch`, or configures +`push.autoSetupRemote = true` together with a `user.pushDefault` that +namespaces their remote branches. It is not a rare configuration — it +is common for contributors who namespace remote branches by username. + +## Root cause + +`getPRForBranch` in `src/main/github/client.ts:745` passes the branch +string it receives from the renderer straight to +`gh pr list --head `. The renderer computes that string from +the worktree's local `HEAD` (see `fetchPRForBranch` in +`src/renderer/src/store/slices/github.ts:386`), so the value is always +the **local** branch name. + +`gh pr list --head` filters by `headRefName`, which is the **remote** +branch name. GitHub has no concept of a local branch, so when the two +names disagree the filter matches nothing and Orca concludes no PR +exists. + +The existing fallback path (`gh pr view `) hits the same +problem for a different reason: `gh pr view` resolves an argument by +first trying it as a PR number, then as a URL, then as a **remote +branch name**. A local-only name fails here too. + +### What about the user's upstream config? + +One natural fix is to read `@{upstream}` and pass the remote branch +name to `gh`. That works for branches created with `git push -u` +or under `push.autoSetupRemote = true`, because both paths write +`branch..merge` in `.git/config`. + +It does **not** work for the motivating repro. The user ran +`git push origin HEAD:brennanb2025/fix-pr-check-node-gyp-perms` +without `-u` and without `push.autoSetupRemote` enabled. That form +pushes the commits but leaves `branch..merge` unset, so +`@{upstream}` errors and upstream-name resolution falls back to the +local name — the same name that doesn't match anything on GitHub. The +bug stays broken. + +Upstream resolution is still useful (it correctly handles the common +`git push -u` case), but it is not sufficient on its own. We need a +second stage that doesn't depend on local git config. + +## The fix: a two-stage resolver with validation + +Resolve the branch → PR mapping with a short cascade. Each stage +produces a candidate; we accept it only if it passes a validation +check for the current worktree. If no stage produces a validated +candidate, return `null`. + +### Stage 1 — upstream-aware name lookup + +Resolve the local branch's upstream: + +```ts +// e.g. "refs/remotes/origin/brennanb2025/fix-pr-check-node-gyp-perms" +const { stdout } = await gitExecFileAsync( + ['rev-parse', '--symbolic-full-name', `${localBranch}@{upstream}`], + { cwd: repoPath } +) +// Strip "refs/remotes//" — the first two segments. +const m = stdout.trim().match(/^refs\/remotes\/[^/]+\/(.+)$/) +const remoteName = m ? m[1] : localBranch +``` + +Git disallows `/` in remote names, so `[^/]+` safely matches exactly the +remote segment regardless of what the user named the remote. + +Pass `remoteName` to `gh pr list --repo O/R --head `. If +a PR comes back, we accept it without further validation: `--head` is +already a `headRefName` filter scoped to the repo we derived from +`origin`, so a match is by definition the PR whose remote branch is +this worktree's upstream. + +If `@{upstream}` errors (no upstream configured — e.g. the user +pushed with an explicit refspec, or never pushed), or if it points at +something that isn't a `refs/remotes/*` ref, fall through to stage 2. +A `gh` error in stage 1 (auth, network, rate-limit) also falls +through: we'd rather try the SHA path than return `null` on a +transient failure. + +### Stage 2 — HEAD-SHA fallback + +Read the worktree's HEAD SHA and search PRs by commit: + +```ts +const { stdout: headShaOut } = await gitExecFileAsync( + ['rev-parse', 'HEAD'], + { cwd: repoPath } +) +const headSha = headShaOut.trim() + +// gh pr list --search " is:pr" --state all --repo O/R --json ... +``` + +`gh pr list --search " is:pr"` returns any PR whose commit +history contains ``, which is **not** the same question as "is +`` the PR's tip". To avoid matching an unrelated PR that merely +contains our commit (easy to trigger with closed/merged PRs whose +branch name was later recycled), require +`candidate.headRefOid === headSha` before accepting. If none of the +candidates match, return `null`. + +This stage fixes the motivating repro (no upstream config, but the +local HEAD is the same commit as the PR's head) and, as a side +benefit, works during a rebase when HEAD is detached on the same +commit the PR tip points to. + +Stage 2 errors (gh exits non-zero, malformed JSON, git rev-parse +fails) are caught and returned as `null`. One stage's error never +short-circuits the other: stage 1 always falls through on any error, +stage 2's result is terminal. + +### Why stop at two stages + +Superset's cascade (see Prior art) has a third stage — `gh pr view` +with no argument, which relies on gh's own tracking-ref inference. +It is more forgiving but also more dangerous: gh can latch onto a +stale `refs/pull/N/head` from a prior `gh pr checkout` and return +an unrelated PR. Accepting its output requires a separate validation +step (compare `headRepositoryOwner.login` and re-check the ref). For +Orca's motivating cases the two stages above are enough. If telemetry +later shows unresolved-PR reports that stage 1+2 can't handle, we can +add a validated `gh pr view` stage without reshaping the resolver. + +### Prior art + +Superset's `pr-resolution.ts` implements the same general shape: try +a name-based lookup, then search by SHA, and validate each candidate +against the local branch (`prMatchesLocalBranch`, +`shouldAcceptPRMatch`). Their third `gh pr view` stage adds coverage +for fork branches at the cost of stale-ref handling. We adopt the +cascade-with-validation pattern and the SHA-head equality check, and +skip the third stage for now. emdash's naive `headRefName == localBranch` +equality and warp's single-shot `gh pr view` fall into the same silent- +failure class as Orca's current code, so neither is a useful reference. + +## Module layout + +Inline resolution belongs in its own file. `client.ts` is already at +1380 lines under `eslint-disable max-lines`, and the codebase pattern +for multi-step gh/git derivations is a dedicated side-file (see +`src/main/github/conflict-summary.ts`). + +- **New file:** `src/main/github/branch-pr-resolution.ts` + - Exports `resolvePRForBranch(repoPath, localBranch): Promise`. + - Owns stage 1 (upstream → name lookup), stage 2 (SHA fallback), + the validation predicates, and the debug logging. + - Imports the gh/git runners from `../git/runner` and the shared + gh helpers (`getOwnerRepo`, `acquire`/`release`) from + `./gh-utils`. +- **Thin shim in `client.ts`:** `getPRForBranch` becomes a one-liner + that delegates to `resolvePRForBranch`. The public signature, + IPC surface, and renderer contract do not change. +- **New test file:** `src/main/github/branch-pr-resolution.test.ts` + covers the cascade exhaustively. `client.test.ts` keeps only a + boundary test that mocks the resolver and verifies wiring. + +## Validation rules + +Candidates from each stage must clear these checks before we return +them: + +1. **Stage 1 (name lookup).** `gh pr list --repo O/R --head ` + already answers the question we're asking; any non-empty result is + valid. We keep the existing `--state all --limit 1` to match + today's behavior. +2. **Stage 2 (SHA fallback).** Require + `candidate.headRefOid === headSha`. This guards against: + - PRs that merely contain our commit in history (common for + merge-commit workflows and long-lived feature branches). + - Recycled branch names that resurface a stale closed/merged PR + whose HEAD diverged long ago. +3. Both stages should respect `getOwnerRepo(repoPath)` and scope + queries to `--repo O/R`, so a branch pushed to multiple remotes + never pulls in a PR from a different fork. + +Accepting a merged/closed PR as the match is fine — the renderer +shows that state — **as long as** `headRefOid === headSha`. The +renderer already handles all `PRInfo.state` values; no UI change. + +## Edge cases + +1. **No upstream configured.** Stage 1 errors; stage 2 runs. If the + branch has never been pushed (no matching PR by SHA either), we + return `null` — correct. +2. **Detached HEAD during rebase.** `getPRForBranch` early-returns on + empty branch name (`client.ts:751`); that check moves into the + resolver unchanged. If we do run with a branch name but HEAD is + detached on the PR's tip, stage 2 still resolves correctly via + `rev-parse HEAD`. +3. **Upstream is set but no PR exists yet.** Stage 1 returns empty; + stage 2 runs and also finds nothing; we return `null`. Correct. +4. **Upstream points at a local ref (e.g. `refs/heads/main`).** The + regex fails; stage 1 falls through to stage 2. Unusual config, + acceptable behavior. +5. **SHA matches a merged PR but heads differ.** Common when a + commit lands via rebase-merge and the branch name gets reused. + Stage 2's `headRefOid === headSha` check rejects it; we return + `null` rather than a misleading old PR. +6. **Multiple remotes.** `getOwnerRepo` already resolves to `origin`'s + owner/repo. Stage 1's upstream-stripping regex handles any remote + name. If a branch tracks a non-`origin` remote that points at the + same GitHub repo, both stages still work. Cross-repo fork cases + (tracking a fork's remote) are out of scope for this fix — they + need an explicit cross-repo lookup that Orca doesn't currently + support anywhere. +7. **Submodules.** Out of scope. Submodule worktrees don't flow + through `getPRForBranch` today, and we're not adding that path. +8. **Worktree path.** Upstream config lives in the main repo's + `.git/config`, shared across worktrees. `gitExecFileAsync` with + `cwd: worktreePath` reads the same config. No special handling. + +## Silent failure (acknowledged limitation) + +Even with the cascade, "no PR found" still collapses two distinct +outcomes: *no PR exists yet* and *we couldn't resolve this branch to +a PR*. For this round we log the resolution path at debug level and +leave the UI unchanged — Orca just shows the "no PR" state. + +A future iteration could change the return type to a tri-state — +`{ kind: 'found', pr } | { kind: 'none' } | { kind: 'unresolved', reason }` +— and surface an "unresolved" hint in the PR card. That's a larger +UX change (renderer, IPC, cache shape) and out of scope here; calling +it out so we remember the gap. + +## Observability + +Debug-level log at each resolution decision: + +```ts +log.debug(`[pr-for-branch] stage=upstream-name localBranch=${b} remoteName=${r}`) +log.debug(`[pr-for-branch] stage=sha-fallback headSha=${sha.slice(0,7)}`) +log.debug(`[pr-for-branch] result=found stage=${n} pr=#${pr.number}`) +log.debug(`[pr-for-branch] result=null reason=${reason}`) +``` + +Kept at debug because divergence is not itself an error. Enough +signal to diagnose future "PR not showing" reports without re-deriving +the user's config by hand. + +## Cache key implication + +The renderer caches PR lookups under `${repoPath}::${branch}` where +`branch` is the **local** name +(`src/renderer/src/store/slices/github.ts:387`). This fix does **not** +change the cache key — two worktrees with different local names but +the same upstream make two separate lookups and cache separately. +That's fine: they're different worktrees at different paths, and +`repoPath` in the key already disambiguates them. + +We should *not* switch the cache key to the resolved remote name or +to the HEAD SHA. Doing so would require the renderer to know the +resolved identity, which means either plumbing it back through IPC +or duplicating the resolution in the renderer. Neither is worth the +minor cache efficiency. + +## Performance + +- Stage 1 adds one `git rev-parse --symbolic-full-name @{upstream}` + per PR poll (a few ms). This is the only extra call in the common + case where upstream is configured and matches. +- Stage 2 runs only when stage 1 errors or returns empty. It adds + one `git rev-parse HEAD` plus one `gh pr list --search`. +- Both git calls are cheap and run *outside* the `acquire()` lock, + which is there to throttle concurrent `gh` processes, not git. + The `gh` calls inside each stage keep using `acquire()`/`release()` + as today. + +The renderer polls on focus and worktree open, not in a tight loop, +so the extra calls are negligible even in the fallback path. + +## Files to change + +- **New** `src/main/github/branch-pr-resolution.ts` — cascade + + validation + debug logging. Exports `resolvePRForBranch`. +- **New** `src/main/github/branch-pr-resolution.test.ts` — covers: + - Upstream matches local: stage 1 resolves, stage 2 not called. + - Upstream differs from local: stage 1 resolves to remote name, + returns PR. + - Upstream set but no PR yet: stage 1 runs, `gh pr list --head` + returns `[]`, stage 2 runs and returns `null`. (The empty-array + case is distinct from the stage-1-errors case and both must fall + through.) + - No upstream: stage 1 skipped, stage 2 matches by SHA with equal + heads. + - No upstream, SHA search returns a PR whose `headRefOid` differs: + rejected, returns `null`. + - Upstream errors, SHA search returns empty: returns `null`. + - Upstream points at `refs/heads/*`: falls through to stage 2. + - Branch-name edge: `refs/heads/` prefix stripped before use. +- **Modify** `src/main/github/client.ts` — `getPRForBranch` delegates + to `resolvePRForBranch`. Keep the empty-branch early return (move + it into the resolver for symmetry). +- **Modify** `src/main/github/client.test.ts` — drop branch-resolution + cases in favor of a mocked-resolver boundary test. + +No changes expected in `src/main/ipc/github.ts`, +`src/main/runtime/orca-runtime.ts`, `src/main/ipc/worktree-remote.ts`, +or any renderer code — resolution remains an internal detail. + +## Why the renderer stays local-branch-centric + +The renderer's input is the worktree's local branch — that's what it +knows, and it's the stable identifier for caching and UI. Both the +remote-name derivation and the SHA fallback are git/gh concerns, not +renderer concerns, and belong next to the other git shell-outs in +the main process. Keeping the renderer local-branch-centric also +means the fix has no IPC surface impact. + +## Risks and rollout + +- **Backwards compatibility.** Users whose local and remote branch + names already match see identical behavior — stage 1 resolves to + the same name on the first try. +- **False positives.** The SHA fallback's equality check on + `headRefOid` is the key guard. Without it, long-lived branches + could match unrelated PRs; with it, we only match when heads are + identical. +- **Search-index lag.** GitHub's code/PR search indexes commits + asynchronously (seconds to ~a minute after push). Stage 2 can miss + a PR whose head commit was pushed moments ago. On the next poll the + commit is found. Acceptable: the window is short and the failure + mode (`null`) matches the pre-push state. +- **Extra gh calls.** Only in the fallback path, and only one extra + `gh pr list --search` per poll. Bounded. +- **Rollout.** Pure main-process change. Ship behind no flag — the + existing behavior is a silent bug, and the new behavior's failure + mode (return `null`) is identical to the old one. diff --git a/src/main/github/branch-pr-resolution.test.ts b/src/main/github/branch-pr-resolution.test.ts new file mode 100644 index 00000000000..95341abc11e --- /dev/null +++ b/src/main/github/branch-pr-resolution.test.ts @@ -0,0 +1,407 @@ +/* eslint-disable max-lines -- Why: exhaustive coverage of the branch → PR +resolver cascade keeps the fixtures for each stage (upstream-name, SHA fallback, +validation rejections, boundary/misc) adjacent so a future maintainer can audit +the full matrix without jumping files. */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + ghExecFileAsyncMock, + gitExecFileAsyncMock, + getOwnerRepoMock, + acquireMock, + releaseMock, + getPRConflictSummaryMock +} = vi.hoisted(() => ({ + ghExecFileAsyncMock: vi.fn(), + gitExecFileAsyncMock: vi.fn(), + getOwnerRepoMock: vi.fn(), + acquireMock: vi.fn(), + releaseMock: vi.fn(), + getPRConflictSummaryMock: vi.fn() +})) + +vi.mock('./gh-utils', () => ({ + ghExecFileAsync: ghExecFileAsyncMock, + getOwnerRepo: getOwnerRepoMock, + acquire: acquireMock, + release: releaseMock +})) + +vi.mock('../git/runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock +})) + +vi.mock('./conflict-summary', () => ({ + getPRConflictSummary: getPRConflictSummaryMock +})) + +import { resolvePRForBranch } from './branch-pr-resolution' + +const PR_JSON_FIELDS = + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + +function makeRawPR(overrides: Record = {}): Record { + return { + number: 42, + title: 'Test PR', + 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', + ...overrides + } +} + +describe('resolvePRForBranch', () => { + beforeEach(() => { + ghExecFileAsyncMock.mockReset() + gitExecFileAsyncMock.mockReset() + getOwnerRepoMock.mockReset() + acquireMock.mockReset() + releaseMock.mockReset() + getPRConflictSummaryMock.mockReset() + acquireMock.mockResolvedValue(undefined) + getPRConflictSummaryMock.mockResolvedValue(undefined) + }) + + describe('stage 1 — upstream-aware name lookup', () => { + it('resolves via upstream when local and remote names match, skipping stage 2', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock.mockResolvedValueOnce({ + stdout: 'refs/remotes/origin/feature/test\n' + }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([makeRawPR()]) + }) + + const pr = await resolvePRForBranch('/repo', 'refs/heads/feature/test') + + expect(pr?.number).toBe(42) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['rev-parse', '--symbolic-full-name', 'feature/test@{upstream}'], + { cwd: '/repo' } + ) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + [ + 'pr', + 'list', + '--repo', + 'acme/widgets', + '--head', + 'feature/test', + '--state', + 'all', + '--limit', + '1', + '--json', + PR_JSON_FIELDS + ], + { cwd: '/repo' } + ) + }) + + it('resolves via upstream when upstream branch name differs from local branch', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock.mockResolvedValueOnce({ + stdout: 'refs/remotes/origin/user/fix-thing\n' + }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([makeRawPR({ number: 733, headRefName: 'user/fix-thing' })]) + }) + + const pr = await resolvePRForBranch('/repo', 'fix-thing') + + expect(pr?.number).toBe(733) + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + expect.arrayContaining(['--head', 'user/fix-thing']), + { cwd: '/repo' } + ) + }) + + it('handles non-origin remotes correctly when stripping the remote prefix', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock.mockResolvedValueOnce({ + stdout: 'refs/remotes/fork/feature-x\n' + }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([makeRawPR({ headRefName: 'feature-x' })]) + }) + + const pr = await resolvePRForBranch('/repo', 'feature-x') + + expect(pr?.number).toBe(42) + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + expect.arrayContaining(['--head', 'feature-x']), + { cwd: '/repo' } + ) + }) + + it('falls through when upstream points at refs/heads/* (local ref)', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'refs/heads/main\n' }) // upstream resolves to local + .mockResolvedValueOnce({ stdout: 'abc1234567890\n' }) // rev-parse HEAD + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([makeRawPR({ headRefOid: 'abc1234567890' })]) + }) + + const pr = await resolvePRForBranch('/repo', 'topic') + + expect(pr?.number).toBe(42) + // Stage 1 never invoked gh (upstream was rejected), stage 2 did. + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + expect.arrayContaining(['--search', 'abc1234567890 is:pr']), + { cwd: '/repo' } + ) + }) + + it('falls through to stage 2 when gh pr list returns an empty array', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'refs/remotes/origin/feature/test\n' }) + .mockResolvedValueOnce({ stdout: 'head-sha\n' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }).mockResolvedValueOnce({ + stdout: JSON.stringify([makeRawPR({ headRefOid: 'head-sha' })]) + }) + + const pr = await resolvePRForBranch('/repo', 'feature/test') + + expect(pr?.number).toBe(42) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('falls through to stage 2 when stage 1 gh errors transiently', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'refs/remotes/origin/feature/test\n' }) + .mockResolvedValueOnce({ stdout: 'head-sha\n' }) + ghExecFileAsyncMock.mockRejectedValueOnce(new Error('network down')).mockResolvedValueOnce({ + stdout: JSON.stringify([makeRawPR({ headRefOid: 'head-sha' })]) + }) + + const pr = await resolvePRForBranch('/repo', 'feature/test') + + expect(pr?.number).toBe(42) + }) + + it('releases the gh lock on stage 1 error', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'refs/remotes/origin/feature/test\n' }) + .mockRejectedValueOnce(new Error('rev-parse HEAD failed')) + ghExecFileAsyncMock.mockRejectedValueOnce(new Error('boom')) + + await resolvePRForBranch('/repo', 'feature/test') + + expect(acquireMock).toHaveBeenCalledTimes(1) + expect(releaseMock).toHaveBeenCalledTimes(1) + }) + }) + + describe('stage 2 — HEAD-SHA fallback', () => { + it('resolves by SHA when upstream is unconfigured', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('no upstream configured')) + .mockResolvedValueOnce({ stdout: 'deadbeefcafe\n' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([makeRawPR({ headRefOid: 'deadbeefcafe', number: 733 })]) + }) + + const pr = await resolvePRForBranch('/repo', 'fix-pr-check-node-gyp-perms') + + expect(pr?.number).toBe(733) + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + [ + 'pr', + 'list', + '--repo', + 'acme/widgets', + '--search', + 'deadbeefcafe is:pr', + '--state', + 'all', + '--limit', + '100', + '--json', + PR_JSON_FIELDS + ], + { cwd: '/repo' } + ) + }) + + it('rejects a search match whose headRefOid differs from HEAD (stale/recycled PR)', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('no upstream')) + .mockResolvedValueOnce({ stdout: 'new-sha\n' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([makeRawPR({ headRefOid: 'old-stale-sha', state: 'MERGED' })]) + }) + + const pr = await resolvePRForBranch('/repo', 'recycled-branch') + + expect(pr).toBeNull() + }) + + it('picks the candidate with matching headRefOid when search returns multiple', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('no upstream')) + .mockResolvedValueOnce({ stdout: 'my-sha\n' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([ + makeRawPR({ number: 100, headRefOid: 'other-sha' }), + makeRawPR({ number: 200, headRefOid: 'my-sha' }) + ]) + }) + + const pr = await resolvePRForBranch('/repo', 'branch') + + expect(pr?.number).toBe(200) + }) + + it('returns null when stage 1 errors and stage 2 search returns empty', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('no upstream')) + .mockResolvedValueOnce({ stdout: 'some-sha\n' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }) + + const pr = await resolvePRForBranch('/repo', 'unpublished') + + expect(pr).toBeNull() + }) + + // Why: design doc §"Edge cases" item 3 — upstream is configured (stage 1 + // runs a real gh pr list query) but no PR exists yet, so stage 1 returns + // []. Stage 2 must still run as a fallback (the branch could have been + // pushed under a different name) and also come back empty, at which point + // we return null rather than surfacing a stale or unrelated PR. + it('returns null when upstream is set but both stages return empty', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'refs/remotes/origin/feature/test\n' }) + .mockResolvedValueOnce({ stdout: 'head-sha\n' }) + ghExecFileAsyncMock + .mockResolvedValueOnce({ stdout: '[]' }) + .mockResolvedValueOnce({ stdout: '[]' }) + + const pr = await resolvePRForBranch('/repo', 'feature/test') + + expect(pr).toBeNull() + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('returns null when rev-parse HEAD fails', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('no upstream')) + .mockRejectedValueOnce(new Error('rev-parse HEAD failed')) + + const pr = await resolvePRForBranch('/repo', 'branch') + + expect(pr).toBeNull() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('returns null when gh search errors', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('no upstream')) + .mockResolvedValueOnce({ stdout: 'head-sha\n' }) + ghExecFileAsyncMock.mockRejectedValueOnce(new Error('rate limit')) + + const pr = await resolvePRForBranch('/repo', 'branch') + + expect(pr).toBeNull() + }) + }) + + describe('boundary / misc', () => { + it('returns null on empty branch name (detached HEAD during rebase)', async () => { + const pr = await resolvePRForBranch('/repo', '') + + expect(pr).toBeNull() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('returns null for refs/heads/ only branch (empty after strip)', async () => { + const pr = await resolvePRForBranch('/repo', 'refs/heads/') + + expect(pr).toBeNull() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('strips refs/heads/ prefix before passing to @{upstream}', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock.mockResolvedValueOnce({ + stdout: 'refs/remotes/origin/feature/test\n' + }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([makeRawPR()]) + }) + + await resolvePRForBranch('/repo', 'refs/heads/feature/test') + + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['rev-parse', '--symbolic-full-name', 'feature/test@{upstream}'], + { cwd: '/repo' } + ) + }) + + it('falls back to gh pr view when the remote is not a recognizable GitHub repo', async () => { + getOwnerRepoMock.mockResolvedValue(null) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify(makeRawPR({ number: 7, isDraft: true })) + }) + + const pr = await resolvePRForBranch('/non-github', 'feature/test') + + expect(pr?.number).toBe(7) + expect(pr?.state).toBe('draft') + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + ['pr', 'view', 'feature/test', '--json', PR_JSON_FIELDS], + { cwd: '/non-github' } + ) + // Never tried stage 1 / 2 cascade — no git invocations. + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('attaches a conflict summary when the matched PR is CONFLICTING', async () => { + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock.mockResolvedValueOnce({ + stdout: 'refs/remotes/origin/feature/test\n' + }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([makeRawPR({ mergeable: 'CONFLICTING' })]) + }) + getPRConflictSummaryMock.mockResolvedValueOnce({ + baseRef: 'main', + baseCommit: 'abc1234', + commitsBehind: 2, + files: ['a.ts'] + }) + + const pr = await resolvePRForBranch('/repo', 'feature/test') + + expect(pr?.mergeable).toBe('CONFLICTING') + expect(pr?.conflictSummary).toEqual({ + baseRef: 'main', + baseCommit: 'abc1234', + commitsBehind: 2, + files: ['a.ts'] + }) + }) + }) +}) diff --git a/src/main/github/branch-pr-resolution.ts b/src/main/github/branch-pr-resolution.ts new file mode 100644 index 00000000000..ac25ac3606f --- /dev/null +++ b/src/main/github/branch-pr-resolution.ts @@ -0,0 +1,269 @@ +/** + * Resolve the local branch of a worktree to the PR whose remote head it tracks. + * + * Why a dedicated module: `gh pr list --head` filters by `headRefName` (the + * remote branch name), but the renderer only knows the worktree's local branch + * name. When the user pushes with an explicit refspec + * (e.g. `git push origin HEAD:prefix/my-branch`) or configures a remote-branch + * namespace via `push.autoSetupRemote` + `user.pushDefault`, local and remote + * names diverge and a naive `--head ` query silently returns `[]`. + * + * This resolver runs a two-stage cascade with validation: + * 1. Resolve `@{upstream}` and look up by the remote name. Covers + * the common case where upstream is configured (git push -u, autoSetupRemote). + * 2. Read HEAD's SHA and search PRs by commit. Covers the case where the user + * pushed with an explicit refspec and left `branch..merge` unset, so + * `@{upstream}` errors. To avoid matching an unrelated PR that merely + * contains our commit in history, we require `candidate.headRefOid === headSha`. + * + * If both stages fail we return `null`, matching the pre-fix failure mode. + */ +import type { PRInfo, PRMergeableState } from '../../shared/types' +import { getPRConflictSummary } from './conflict-summary' +import { gitExecFileAsync } from '../git/runner' +import { ghExecFileAsync, acquire, release, getOwnerRepo, type OwnerRepo } from './gh-utils' +import { mapPRState, deriveCheckStatus } from './mappers' + +const PR_JSON_FIELDS = + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + +type RawPR = { + number: number + title: string + state: string + url: string + statusCheckRollup: unknown[] + updatedAt: string + isDraft?: boolean + mergeable: string + baseRefName?: string + headRefName?: string + baseRefOid?: string + headRefOid?: string +} + +/** + * Resolve a worktree's local branch to its PR on GitHub, if one exists. + * + * Returns `null` when no PR matches, when the remote isn't a resolvable GitHub + * repo, during a rebase (empty branch), or when both stages fail transiently. + */ +export async function resolvePRForBranch(repoPath: string, branch: string): Promise { + // Why: strip refs/heads/ prefix so the resolver accepts both the symbolic + // full-name ("refs/heads/foo") and plain form ("foo"). + const localBranch = branch.replace(/^refs\/heads\//, '') + + // Why: during a rebase the worktree is in detached HEAD with no branch + // name. An empty --head filter causes gh to return an arbitrary PR — bail + // early. Same guard lived at the call site before the resolver existed. + if (!localBranch) { + return null + } + + const ownerRepo = await getOwnerRepo(repoPath) + + if (!ownerRepo) { + // Why: the remote isn't a recognizable GitHub URL. Preserve pre-resolver + // behavior by trying `gh pr view `, which lets gh resolve the + // argument via its own tracking-ref inference. Not subject to the same + // local/remote name mismatch because it doesn't filter by headRefName. + const stage3 = await lookupByGhPrView(repoPath, localBranch) + if (stage3) { + console.debug(`[pr-for-branch] result=found stage=gh-pr-view pr=#${stage3.number}`) + return await enrichWithConflictSummary(repoPath, stage3) + } + console.debug('[pr-for-branch] result=null reason=no-match stage=gh-pr-view') + return null + } + + const stage1 = await lookupByUpstreamName(repoPath, localBranch, ownerRepo) + if (stage1) { + console.debug(`[pr-for-branch] result=found stage=upstream-name pr=#${stage1.number}`) + return await enrichWithConflictSummary(repoPath, stage1) + } + + const stage2 = await lookupBySha(repoPath, ownerRepo) + if (stage2) { + console.debug(`[pr-for-branch] result=found stage=sha-fallback pr=#${stage2.number}`) + return await enrichWithConflictSummary(repoPath, stage2) + } + + console.debug('[pr-for-branch] result=null reason=no-match') + return null +} + +/** + * Stage 1 — resolve `@{upstream}` and query gh by the remote + * branch name. Returns the raw PR or null if upstream is unresolvable, points + * at an unexpected ref form, or gh returns no match / errors transiently. + */ +async function lookupByUpstreamName( + repoPath: string, + localBranch: string, + ownerRepo: OwnerRepo +): Promise { + let remoteName: string + try { + const { stdout } = await gitExecFileAsync( + ['rev-parse', '--symbolic-full-name', `${localBranch}@{upstream}`], + { cwd: repoPath } + ) + // Why: upstream output is "refs/remotes//". Git disallows + // "/" in remote names, so [^/]+ matches the remote segment exactly no + // matter what the user named it. A match we can't parse (e.g. upstream + // pointing at refs/heads/main) falls through to stage 2. + const match = stdout.trim().match(/^refs\/remotes\/[^/]+\/(.+)$/) + if (!match) { + console.debug( + `[pr-for-branch] stage=upstream-name skip reason=non-remote-upstream value=${stdout.trim()}` + ) + return null + } + remoteName = match[1] + } catch { + // Why: no upstream configured (e.g. user pushed with an explicit refspec + // and without -u). Fall through — stage 2 works without upstream info. + console.debug('[pr-for-branch] stage=upstream-name skip reason=no-upstream') + return null + } + + console.debug( + `[pr-for-branch] stage=upstream-name localBranch=${localBranch} remoteName=${remoteName}` + ) + + await acquire() + try { + const { stdout } = await ghExecFileAsync( + [ + 'pr', + 'list', + '--repo', + `${ownerRepo.owner}/${ownerRepo.repo}`, + '--head', + remoteName, + '--state', + 'all', + '--limit', + '1', + '--json', + PR_JSON_FIELDS + ], + { cwd: repoPath } + ) + const list = JSON.parse(stdout) as RawPR[] + // Why: --head is already a headRefName filter scoped to the repo we + // derived from origin, so any match is by definition the PR whose remote + // branch is this worktree's upstream. No extra validation required. + return list[0] ?? null + } catch { + // Why: transient gh failure (auth, network, rate-limit) should not + // short-circuit the cascade — stage 2 may still succeed offline of the + // search index. + return null + } finally { + release() + } +} + +/** + * Stage 2 — read HEAD's SHA and search by commit. Validates + * `candidate.headRefOid === headSha` so a PR that merely *contains* our + * commit in history (rebase-merge, recycled branches) doesn't masquerade as + * the one we're looking for. + */ +async function lookupBySha(repoPath: string, ownerRepo: OwnerRepo): Promise { + let headSha: string + try { + const { stdout } = await gitExecFileAsync(['rev-parse', 'HEAD'], { cwd: repoPath }) + headSha = stdout.trim() + if (!headSha) { + return null + } + } catch { + return null + } + + console.debug(`[pr-for-branch] stage=sha-fallback headSha=${headSha.slice(0, 7)}`) + + await acquire() + try { + const { stdout } = await ghExecFileAsync( + [ + 'pr', + 'list', + '--repo', + `${ownerRepo.owner}/${ownerRepo.repo}`, + '--search', + `${headSha} is:pr`, + '--state', + 'all', + // Why: gh's default page size is 30. `--search " is:pr"` can + // match any PR whose history contains — in long-lived repos + // that's more than 30 for shared base commits. Raise the cap so + // the headRefOid === headSha candidate isn't silently paged out. + '--limit', + '100', + '--json', + PR_JSON_FIELDS + ], + { cwd: repoPath } + ) + const list = JSON.parse(stdout) as RawPR[] + // Why: `gh pr list --search " is:pr"` returns any PR whose commit + // history contains , which is *not* the same question as "is + // the PR's tip". Require equality to headRefOid to guard against + // recycled-branch and merge-commit false positives. + const match = list.find((pr) => pr.headRefOid === headSha) + return match ?? null + } catch { + return null + } finally { + release() + } +} + +/** + * Fallback for non-GitHub remotes: let gh's own branch inference resolve the + * PR. Preserves pre-resolver behavior for users whose remote URL doesn't + * parse as a github.com URL (e.g. SAML-SSO enterprise hosts, self-hosted). + * + * Why returns RawPR instead of enriched PRInfo: symmetric with stages 1 and 2 + * — acquire() throttles gh, not git, so the caller must release the lock + * before invoking enrichWithConflictSummary (which can do a `git fetch` with + * a 10s timeout). Holding the gh lock across a git-only call would + * needlessly serialize unrelated gh requests. See docs/pr-for-branch-upstream-resolution.md + * §"Performance". + */ +async function lookupByGhPrView(repoPath: string, branchName: string): Promise { + await acquire() + try { + const { stdout } = await ghExecFileAsync(['pr', 'view', branchName, '--json', PR_JSON_FIELDS], { + cwd: repoPath + }) + const raw = JSON.parse(stdout) as RawPR + return raw ?? null + } catch { + return null + } finally { + release() + } +} + +async function enrichWithConflictSummary(repoPath: string, data: RawPR): Promise { + const conflictSummary = + data.mergeable === 'CONFLICTING' && data.baseRefName && data.baseRefOid && data.headRefOid + ? await getPRConflictSummary(repoPath, data.baseRefName, data.baseRefOid, data.headRefOid) + : undefined + + return { + number: data.number, + title: data.title, + state: mapPRState(data.state, data.isDraft), + url: data.url, + checksStatus: deriveCheckStatus(data.statusCheckRollup), + updatedAt: data.updatedAt, + mergeable: (data.mergeable as PRMergeableState) ?? 'UNKNOWN', + headSha: data.headRefOid, + conflictSummary + } +} diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index 5ba69ebd357..21bbdb5c0ba 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -1,263 +1,47 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -const { - execFileAsyncMock, - ghExecFileAsyncMock, - getOwnerRepoMock, - getIssueOwnerRepoMock, - gitExecFileAsyncMock, - acquireMock, - releaseMock -} = vi.hoisted(() => ({ - execFileAsyncMock: vi.fn(), - ghExecFileAsyncMock: vi.fn(), - getOwnerRepoMock: vi.fn(), - getIssueOwnerRepoMock: vi.fn(), - gitExecFileAsyncMock: vi.fn(), - acquireMock: vi.fn(), - releaseMock: vi.fn() +const { resolvePRForBranchMock } = vi.hoisted(() => ({ + resolvePRForBranchMock: vi.fn() })) -vi.mock('./gh-utils', () => ({ - execFileAsync: execFileAsyncMock, - ghExecFileAsync: ghExecFileAsyncMock, - getOwnerRepo: getOwnerRepoMock, - getIssueOwnerRepo: getIssueOwnerRepoMock, - acquire: acquireMock, - release: releaseMock, - _resetOwnerRepoCache: vi.fn() +// Why: the resolver owns all the branch → PR mapping logic and is covered +// exhaustively in branch-pr-resolution.test.ts. This file keeps a boundary +// test that just verifies client.getPRForBranch delegates correctly. +vi.mock('./branch-pr-resolution', () => ({ + resolvePRForBranch: resolvePRForBranchMock })) -vi.mock('../git/runner', () => ({ - gitExecFileAsync: gitExecFileAsyncMock -})) - -import { getPRForBranch, _resetOwnerRepoCache } from './client' +import { getPRForBranch } from './client' describe('getPRForBranch', () => { beforeEach(() => { - execFileAsyncMock.mockReset() - ghExecFileAsyncMock.mockReset() - getOwnerRepoMock.mockReset() - getIssueOwnerRepoMock.mockReset() - gitExecFileAsyncMock.mockReset() - acquireMock.mockReset() - releaseMock.mockReset() - acquireMock.mockResolvedValue(undefined) - _resetOwnerRepoCache() + resolvePRForBranchMock.mockReset() }) - it('queries GitHub by head branch when the remote is on github.com', async () => { - getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock.mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 42, - title: 'Fix PR discovery', - 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' - } - ]) - }) + it('delegates to resolvePRForBranch and returns its result', async () => { + const pr = { + number: 42, + title: 'Test', + state: 'open' as const, + url: 'https://github.com/acme/widgets/pull/42', + checksStatus: 'success' as const, + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'MERGEABLE' as const, + headSha: 'head-oid' + } + resolvePRForBranchMock.mockResolvedValueOnce(pr) - const pr = await getPRForBranch('/repo-root', 'refs/heads/feature/test') + const result = await getPRForBranch('/repo-root', 'refs/heads/feature/test') - expect(getOwnerRepoMock).toHaveBeenCalledWith('/repo-root') - expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - [ - 'pr', - 'list', - '--repo', - 'acme/widgets', - '--head', - 'feature/test', - '--state', - 'all', - '--limit', - '1', - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' - ], - { cwd: '/repo-root' } - ) - expect(pr?.number).toBe(42) - expect(pr?.state).toBe('open') - expect(pr?.mergeable).toBe('MERGEABLE') + expect(resolvePRForBranchMock).toHaveBeenCalledWith('/repo-root', 'refs/heads/feature/test') + expect(result).toBe(pr) }) - it('falls back to gh pr view when the remote cannot be resolved to GitHub', async () => { - getOwnerRepoMock.mockResolvedValueOnce(null) - ghExecFileAsyncMock.mockResolvedValueOnce({ - stdout: JSON.stringify({ - number: 7, - title: 'Fallback lookup', - state: 'OPEN', - url: 'https://example.com/pr/7', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: true, - mergeable: 'CONFLICTING', - baseRefName: 'main', - headRefName: 'feature/test', - baseRefOid: 'base-oid', - headRefOid: 'head-oid' - }) - }) + it('propagates a null result from the resolver', async () => { + resolvePRForBranchMock.mockResolvedValueOnce(null) - const pr = await getPRForBranch('/non-github-repo', 'feature/test') + const result = await getPRForBranch('/repo-root', 'no-pr-branch') - expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - [ - 'pr', - 'view', - 'feature/test', - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' - ], - { cwd: '/non-github-repo' } - ) - expect(pr?.number).toBe(7) - expect(pr?.state).toBe('draft') - expect(pr?.mergeable).toBe('CONFLICTING') - }) - - it('derives a read-only conflict summary for conflicting PRs when the base ref exists locally', async () => { - getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock.mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 42, - title: 'Fix PR discovery', - state: 'OPEN', - url: 'https://github.com/acme/widgets/pull/42', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'CONFLICTING', - baseRefName: 'main', - headRefName: 'feature/test', - baseRefOid: 'base-oid', - headRefOid: 'head-oid' - } - ]) - }) - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: '' }) - .mockResolvedValueOnce({ stdout: 'latest-base-oid\n' }) - .mockResolvedValueOnce({ stdout: 'merge-base-oid\n' }) - .mockResolvedValueOnce({ stdout: '3\n' }) - .mockResolvedValueOnce({ stdout: 'result-tree-oid\u0000src/a.ts\u0000src/b.ts\u0000' }) - - const pr = await getPRForBranch('/repo-root', 'feature/test') - - expect(pr?.conflictSummary).toEqual({ - baseRef: 'main', - baseCommit: 'latest-', - commitsBehind: 3, - files: ['src/a.ts', 'src/b.ts'] - }) - }) - - it('keeps conflicted file paths when git merge-tree exits 1 with stdout', async () => { - getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock.mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 42, - title: 'Fix PR discovery', - state: 'OPEN', - url: 'https://github.com/acme/widgets/pull/42', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'CONFLICTING', - baseRefName: 'main', - headRefName: 'feature/test', - baseRefOid: 'base-oid', - headRefOid: 'head-oid' - } - ]) - }) - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: '' }) - .mockResolvedValueOnce({ stdout: 'latest-base-oid\n' }) - .mockResolvedValueOnce({ stdout: 'merge-base-oid\n' }) - .mockResolvedValueOnce({ stdout: '2\n' }) - .mockRejectedValueOnce({ - stdout: 'result-tree-oid\u0000src/conflict.ts\u0000' - }) - - const pr = await getPRForBranch('/repo-root', 'feature/test') - - expect(pr?.conflictSummary?.files).toEqual(['src/conflict.ts']) - }) - - it('falls back to GitHub baseRefOid when fetching or resolving the base ref fails', async () => { - getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock.mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 42, - title: 'Fix PR discovery', - state: 'OPEN', - url: 'https://github.com/acme/widgets/pull/42', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'CONFLICTING', - baseRefName: 'main', - headRefName: 'feature/test', - baseRefOid: 'base-oid', - headRefOid: 'head-oid' - } - ]) - }) - gitExecFileAsyncMock - .mockRejectedValueOnce(new Error('fetch failed')) - .mockRejectedValueOnce(new Error('missing refs/remotes/origin/main')) - .mockRejectedValueOnce(new Error('missing origin/main')) - .mockResolvedValueOnce({ stdout: 'merge-base-oid\n' }) - .mockResolvedValueOnce({ stdout: '1\n' }) - .mockResolvedValueOnce({ stdout: 'result-tree-oid\u0000src/fallback.ts\u0000' }) - - const pr = await getPRForBranch('/repo-root', 'feature/test') - - expect(pr?.conflictSummary).toEqual({ - baseRef: 'main', - baseCommit: 'base-oi', - commitsBehind: 1, - files: ['src/fallback.ts'] - }) - }) - - it('returns null for empty branch (e.g. during rebase with detached HEAD)', async () => { - const pr = await getPRForBranch('/repo-root', '') - expect(pr).toBeNull() - // Should not call gh at all - expect(execFileAsyncMock).not.toHaveBeenCalled() - }) - - it('returns null for refs/heads/ only branch (detached after strip)', async () => { - const pr = await getPRForBranch('/repo-root', 'refs/heads/') - expect(pr).toBeNull() - expect(execFileAsyncMock).not.toHaveBeenCalled() - }) - - it('returns null when pr list returns an empty array', async () => { - execFileAsyncMock - .mockResolvedValueOnce({ stdout: 'git@github.com:acme/widgets.git\n' }) - .mockResolvedValueOnce({ stdout: '[]' }) - - const pr = await getPRForBranch('/repo-root', 'no-pr-branch') - - expect(pr).toBeNull() + expect(result).toBeNull() }) }) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index bbd8a1f915b..1e8d678eea7 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -2,7 +2,6 @@ concurrency acquire/release pattern and error handling consistent across operations. */ import type { PRInfo, - PRMergeableState, PRCheckDetail, GitHubCommentResult, GitHubPRReviewCommentInput, @@ -12,7 +11,7 @@ import type { } from '../../shared/types' import { parseTaskQuery, type ParsedTaskQuery } from '../../shared/task-query' import { sortWorkItemsByUpdatedAt } from '../../shared/work-items' -import { getPRConflictSummary } from './conflict-summary' +import { resolvePRForBranch } from './branch-pr-resolution' import { execFileAsync, ghExecFileAsync, @@ -37,9 +36,7 @@ import { mapCheckRunRESTStatus, mapCheckRunRESTConclusion, mapCheckStatus, - mapCheckConclusion, - mapPRState, - deriveCheckStatus + mapCheckConclusion } from './mappers' import { mapGraphQLReactionGroups, type GitHubGraphQLReactionGroup } from './comment-reactions' @@ -741,94 +738,15 @@ export async function getWorkItem( /** * Get PR info for a given branch using gh CLI. * Returns null if gh is not installed, or no PR exists for the branch. + * + * Why the indirection: naive `gh pr list --head ` silently + * returns `[]` when a worktree's local branch name differs from the remote + * branch name it tracks (e.g. pushed with an explicit refspec). The + * resolver runs a two-stage cascade — upstream-name lookup, then HEAD-SHA + * fallback with validation — to recover the PR in those cases. */ export async function getPRForBranch(repoPath: string, branch: string): Promise { - // Strip refs/heads/ prefix if present - const branchName = branch.replace(/^refs\/heads\//, '') - - // During a rebase the worktree is in detached HEAD and branch is empty. - // An empty --head filter causes gh to return an arbitrary PR — bail early. - if (!branchName) { - return null - } - - await acquire() - try { - const ownerRepo = await getOwnerRepo(repoPath) - let data: { - number: number - title: string - state: string - url: string - statusCheckRollup: unknown[] - updatedAt: string - isDraft?: boolean - mergeable: string - baseRefName?: string - headRefName?: string - baseRefOid?: string - headRefOid?: string - } | null = null - - if (ownerRepo) { - const { stdout } = await ghExecFileAsync( - [ - 'pr', - 'list', - '--repo', - `${ownerRepo.owner}/${ownerRepo.repo}`, - '--head', - branchName, - '--state', - 'all', - '--limit', - '1', - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' - ], - { cwd: repoPath } - ) - const list = JSON.parse(stdout) as NonNullable[] - data = list[0] ?? null - } else { - const { stdout } = await ghExecFileAsync( - [ - 'pr', - 'view', - branchName, - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' - ], - { cwd: repoPath } - ) - data = JSON.parse(stdout) - } - - if (!data) { - return null - } - - const conflictSummary = - data.mergeable === 'CONFLICTING' && data.baseRefName && data.baseRefOid && data.headRefOid - ? await getPRConflictSummary(repoPath, data.baseRefName, data.baseRefOid, data.headRefOid) - : undefined - - return { - number: data.number, - title: data.title, - state: mapPRState(data.state, data.isDraft), - url: data.url, - checksStatus: deriveCheckStatus(data.statusCheckRollup), - updatedAt: data.updatedAt, - mergeable: (data.mergeable as PRMergeableState) ?? 'UNKNOWN', - headSha: data.headRefOid, - conflictSummary - } - } catch { - return null - } finally { - release() - } + return await resolvePRForBranch(repoPath, branch) } /** diff --git a/src/main/github/conflict-summary.test.ts b/src/main/github/conflict-summary.test.ts new file mode 100644 index 00000000000..831d5170735 --- /dev/null +++ b/src/main/github/conflict-summary.test.ts @@ -0,0 +1,67 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn() +})) + +vi.mock('../git/runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock +})) + +import { getPRConflictSummary } from './conflict-summary' + +describe('getPRConflictSummary', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + }) + + it('derives a read-only conflict summary when the base ref exists locally', async () => { + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: '' }) // fetch + .mockResolvedValueOnce({ stdout: 'latest-base-oid\n' }) // rev-parse refs/remotes/origin/main + .mockResolvedValueOnce({ stdout: 'merge-base-oid\n' }) // merge-base + .mockResolvedValueOnce({ stdout: '3\n' }) // rev-list --count + .mockResolvedValueOnce({ stdout: 'result-tree-oid\0src/a.ts\0src/b.ts\0' }) // merge-tree + + const result = await getPRConflictSummary('/repo-root', 'main', 'base-oid', 'head-oid') + + expect(result).toEqual({ + baseRef: 'main', + baseCommit: 'latest-', + commitsBehind: 3, + files: ['src/a.ts', 'src/b.ts'] + }) + }) + + it('keeps conflicted file paths when git merge-tree exits 1 with stdout', async () => { + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: '' }) // fetch + .mockResolvedValueOnce({ stdout: 'latest-base-oid\n' }) // rev-parse refs/remotes/origin/main + .mockResolvedValueOnce({ stdout: 'merge-base-oid\n' }) // merge-base + .mockResolvedValueOnce({ stdout: '3\n' }) // rev-list --count + .mockRejectedValueOnce({ stdout: 'result-tree-oid\0src/conflict.ts\0' }) // merge-tree exit 1 + + const result = await getPRConflictSummary('/repo-root', 'main', 'base-oid', 'head-oid') + + expect(result?.files).toEqual(['src/conflict.ts']) + }) + + it('falls back to GitHub baseRefOid when fetching or resolving the base ref fails', async () => { + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('fetch failed')) // fetch + .mockRejectedValueOnce(new Error('missing refs/remotes/origin/main')) // first rev-parse + .mockRejectedValueOnce(new Error('missing origin/main')) // second rev-parse + .mockResolvedValueOnce({ stdout: 'merge-base-oid\n' }) // merge-base + .mockResolvedValueOnce({ stdout: '1\n' }) // rev-list --count + .mockResolvedValueOnce({ stdout: 'result-tree-oid\0src/fallback.ts\0' }) // merge-tree + + const result = await getPRConflictSummary('/repo-root', 'main', 'base-oid', 'head-oid') + + expect(result).toEqual({ + baseRef: 'main', + baseCommit: 'base-oi', + commitsBehind: 1, + files: ['src/fallback.ts'] + }) + }) +})