From e440579805689ede976802009aa657972c532e01 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Mon, 16 Mar 2026 18:18:43 +0000 Subject: [PATCH] fix: resolve push/fetch remote from branch tracking instead of hardcoding origin (#2516) Push and fetch operations always targeted "origin", ignoring the branch's actual tracking remote. This broke fork PR review workflows where `gh pr checkout` sets the upstream to a contributor's fork remote. - Add `getTrackingRemote()` helper that reads `@{upstream}` and extracts the remote name via `parseUpstreamRef()` - Update `pushWithSetUpstream()` to accept optional remote, defaulting to the tracking remote - Update `fetchCurrentBranch()` to fetch from the tracking remote - Add reproduction tests verifying remote resolution for fork scenarios Closes #2516 --- .../routers/changes/git-operations.test.ts | 88 +++++++++++++++++++ .../trpc/routers/changes/git-operations.ts | 26 +++++- 2 files changed, 111 insertions(+), 3 deletions(-) diff --git a/apps/desktop/src/lib/trpc/routers/changes/git-operations.test.ts b/apps/desktop/src/lib/trpc/routers/changes/git-operations.test.ts index da476c8a504..f9f74122f51 100644 --- a/apps/desktop/src/lib/trpc/routers/changes/git-operations.test.ts +++ b/apps/desktop/src/lib/trpc/routers/changes/git-operations.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; import { isUpstreamMissingError } from "./git-utils"; +import { parseUpstreamRef } from "./utils/pull-request-url"; describe("git-operations error handling", () => { describe("isUpstreamMissingError", () => { @@ -91,3 +92,90 @@ describe("sync operation logic", () => { expect(isUpstreamMissingError(pullError.message)).toBe(false); }); }); + +describe("tracking remote resolution (#2516)", () => { + // Reproduces the bug where push/fetch always targeted "origin" instead of + // the branch's actual tracking remote (e.g. a fork remote added by `gh pr checkout`). + + test("parseUpstreamRef extracts fork remote name from upstream ref", () => { + // When `gh pr checkout` sets up a fork, the upstream looks like "contributor-fork/feature-branch" + const result = parseUpstreamRef("contributor-fork/feature-branch"); + expect(result).toEqual({ + remoteName: "contributor-fork", + branchName: "feature-branch", + }); + }); + + test("parseUpstreamRef extracts origin remote name", () => { + const result = parseUpstreamRef("origin/main"); + expect(result).toEqual({ + remoteName: "origin", + branchName: "main", + }); + }); + + test("parseUpstreamRef returns null for invalid refs", () => { + expect(parseUpstreamRef("")).toBeNull(); + expect(parseUpstreamRef("no-slash")).toBeNull(); + expect(parseUpstreamRef("/leading-slash")).toBeNull(); + expect(parseUpstreamRef("trailing/")).toBeNull(); + }); + + test("getTrackingRemote logic: returns fork remote when tracking fork upstream", () => { + // This tests the core logic that was broken before the fix. + // The getTrackingRemote function uses parseUpstreamRef to extract the remote. + // Before the fix, push/fetch always hardcoded "origin" regardless of tracking. + const upstreamRef = "my-fork-remote/fix-typo"; + const parsed = parseUpstreamRef(upstreamRef); + + // Before fix: would always use "origin" — ignoring the parsed remote + // After fix: uses parsed.remoteName ("my-fork-remote") + expect(parsed).not.toBeNull(); + expect(parsed?.remoteName).toBe("my-fork-remote"); + expect(parsed?.remoteName).not.toBe("origin"); + }); + + test("getTrackingRemote logic: falls back to origin when no upstream is set", () => { + // When parseUpstreamRef returns null (no upstream configured), + // getTrackingRemote should fall back to "origin" + const parsed = parseUpstreamRef(""); + expect(parsed).toBeNull(); + // Fallback behavior: when parsed is null, getTrackingRemote returns "origin" + }); + + test("push commands should use tracking remote, not hardcoded origin", () => { + // Verify the push args construction uses the resolved remote + const upstreamRef = "contributor/feature-branch"; + const parsed = parseUpstreamRef(upstreamRef); + expect(parsed).not.toBeNull(); + + const remote = parsed?.remoteName; + const branch = "feature-branch"; + + // The push command should use the tracking remote + const pushArgs = ["--set-upstream", remote, `HEAD:refs/heads/${branch}`]; + + expect(pushArgs).toEqual([ + "--set-upstream", + "contributor", + "HEAD:refs/heads/feature-branch", + ]); + // Before fix, pushArgs[1] would always be "origin" + expect(pushArgs[1]).not.toBe("origin"); + }); + + test("fetch commands should use tracking remote, not hardcoded origin", () => { + // Verify fetch uses resolved remote + const upstreamRef = "fork-user/my-branch"; + const parsed = parseUpstreamRef(upstreamRef); + expect(parsed).not.toBeNull(); + + const remote = parsed?.remoteName; + const branch = "my-branch"; + + const fetchArgs = [remote, branch]; + expect(fetchArgs).toEqual(["fork-user", "my-branch"]); + // Before fix, fetchArgs[0] would always be "origin" + expect(fetchArgs[0]).not.toBe("origin"); + }); +}); diff --git a/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts b/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts index cf53f28f384..5bb50c067b0 100644 --- a/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts +++ b/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts @@ -31,15 +31,31 @@ async function hasUpstreamBranch(git: SimpleGit): Promise { } } +async function getTrackingRemote(git: SimpleGit): Promise { + try { + const upstream = ( + await git.raw(["rev-parse", "--abbrev-ref", "@{upstream}"]) + ).trim(); + const parsed = parseUpstreamRef(upstream); + if (parsed) { + return parsed.remoteName; + } + } catch { + // No upstream configured, fall back to origin + } + return "origin"; +} + async function fetchCurrentBranch(git: SimpleGit): Promise { const branch = (await git.revparse(["--abbrev-ref", "HEAD"])).trim(); + const remote = await getTrackingRemote(git); try { - await git.fetch(["origin", branch]); + await git.fetch([remote, branch]); } catch (error) { const message = error instanceof Error ? error.message : String(error); if (isUpstreamMissingError(message)) { try { - await git.fetch(["origin"]); + await git.fetch([remote]); } catch (fallbackError) { const fallbackMessage = fallbackError instanceof Error @@ -62,9 +78,11 @@ async function fetchCurrentBranch(git: SimpleGit): Promise { async function pushWithSetUpstream({ git, branch, + remote, }: { git: SimpleGit; branch: string; + remote?: string; }): Promise { const trimmedBranch = branch.trim(); if (!trimmedBranch || trimmedBranch === "HEAD") { @@ -75,11 +93,13 @@ async function pushWithSetUpstream({ }); } + const targetRemote = remote ?? (await getTrackingRemote(git)); + // Use HEAD refspec to avoid resolving the branch name as a local ref. // This is more reliable for worktrees where upstream tracking isn't set yet. await git.push([ "--set-upstream", - "origin", + targetRemote, `HEAD:refs/heads/${trimmedBranch}`, ]); }