From daa4e468481f142564d5c8bfb69412f477ffee92 Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Thu, 30 Apr 2026 14:41:47 -0700 Subject: [PATCH] chore: remove pr-for-branch-upstream-resolution design doc Remove internal design note that shouldn't ship with the branch. Co-authored-by: Orca --- docs/pr-for-branch-upstream-resolution.md | 348 ---------------------- 1 file changed, 348 deletions(-) delete mode 100644 docs/pr-for-branch-upstream-resolution.md diff --git a/docs/pr-for-branch-upstream-resolution.md b/docs/pr-for-branch-upstream-resolution.md deleted file mode 100644 index f006ad219ff..00000000000 --- a/docs/pr-for-branch-upstream-resolution.md +++ /dev/null @@ -1,348 +0,0 @@ -# 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.