From f1122011a287620864a72880eedfeb1acb13bc43 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 21 Jul 2026 15:27:43 -0500 Subject: [PATCH 1/6] fix(git): resolve GitLab merge requests, not just GitHub PRs Cloning a GitLab repo at a merge-request ref failed with "couldn't find remote ref pull/N/head": devsy hardcoded GitHub's pull/N/head refspec everywhere, but GitLab exposes merge requests at merge-requests/N/head. Introduce a git-host abstraction (pkg/git/host.go) owning each provider's refspec and local-branch convention. CheckoutPR now resolves the refspec from the repository URL's host and falls back to the other known convention when the detected one has no such ref, so self-hosted GitLab on custom domains still works. The desktop wizard is relabeled "Pull / Merge Request" and emits the host-appropriate ref. --- .../workspace/WorkspaceWizard.svelte | 8 +- .../src/lib/utils/workspace-source.test.ts | 16 +++- .../src/lib/utils/workspace-source.ts | 19 ++++- pkg/git/git.go | 28 +++++-- pkg/git/git_test.go | 12 +++ pkg/git/host.go | 75 +++++++++++++++++++ pkg/git/host_test.go | 44 +++++++++++ pkg/git/repo.go | 33 +++++--- pkg/git/repo_test.go | 46 +++++++++++- 9 files changed, 255 insertions(+), 26 deletions(-) create mode 100644 pkg/git/host.go create mode 100644 pkg/git/host_test.go diff --git a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte index 0f056e28b..e9c6aa482 100644 --- a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte +++ b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte @@ -651,18 +651,18 @@ function selectTemplate(t: { name: string; source: string }) { - {refType === "branch" ? "Branch" : refType === "commit" ? "Commit" : "Pull Request"} + {refType === "branch" ? "Branch" : refType === "commit" ? "Commit" : "Pull / Merge Request"} Branch Commit - Pull Request + Pull / Merge Request
- {refType === "branch" ? "Branch" : refType === "commit" ? "Commit" : "Pull request"} + {refType === "branch" ? "Branch" : refType === "commit" ? "Commit" : "Pull / Merge request"} {refValue}
diff --git a/desktop/src/renderer/src/lib/utils/workspace-source.test.ts b/desktop/src/renderer/src/lib/utils/workspace-source.test.ts index 8111a8560..2e5cfe3c8 100644 --- a/desktop/src/renderer/src/lib/utils/workspace-source.test.ts +++ b/desktop/src/renderer/src/lib/utils/workspace-source.test.ts @@ -59,13 +59,27 @@ describe("buildWorkspaceSource", () => { expect(out.source).toBe("github.com/org/repo@sha256:abc123") }) - it("git: PR ref appends @pull/N/head", () => { + it("git: PR ref appends @pull/N/head for GitHub", () => { const out = buildWorkspaceSource( gitForm({ repoUrl: "github.com/org/repo", refType: "pr", refValue: "42" }), ) expect(out.source).toBe("github.com/org/repo@pull/42/head") }) + it("git: MR ref appends @merge-requests/N/head for GitLab", () => { + const out = buildWorkspaceSource( + gitForm({ repoUrl: "gitlab.com/org/repo", refType: "pr", refValue: "7125" }), + ) + expect(out.source).toBe("gitlab.com/org/repo@merge-requests/7125/head") + }) + + it("git: MR ref detects self-hosted GitLab by hostname", () => { + const out = buildWorkspaceSource( + gitForm({ repoUrl: "git@gitlab.example.com:org/repo.git", refType: "pr", refValue: "7" }), + ) + expect(out.source).toBe("git@gitlab.example.com:org/repo.git@merge-requests/7/head") + }) + it("git: subpath appends @subpath: after ref", () => { const out = buildWorkspaceSource( gitForm({ diff --git a/desktop/src/renderer/src/lib/utils/workspace-source.ts b/desktop/src/renderer/src/lib/utils/workspace-source.ts index 11845fcf3..26fdf9f6f 100644 --- a/desktop/src/renderer/src/lib/utils/workspace-source.ts +++ b/desktop/src/renderer/src/lib/utils/workspace-source.ts @@ -34,7 +34,19 @@ export interface WorkspaceSourceResult { prebuildRepository?: string } -function refSuffix(refType: GitRefType, refValue: string): string { +// GitLab exposes merge requests at merge-requests/N/head; every other host +// uses pull/N/head. Detection is best-effort — the CLI fetch falls back to the +// other convention when the detected one has no such ref. +function prRefspec(repoUrl: string, number: string): string { + const segment = /gitlab/i.test(repoUrl) ? "merge-requests" : "pull" + return `@${segment}/${number}/head` +} + +function refSuffix( + refType: GitRefType, + refValue: string, + repoUrl: string, +): string { const value = refValue.trim() if (!value) return "" switch (refType) { @@ -43,7 +55,7 @@ function refSuffix(refType: GitRefType, refValue: string): string { case "commit": return `@sha256:${value}` case "pr": - return `@pull/${value}/head` + return prRefspec(repoUrl, value) } } @@ -72,7 +84,8 @@ export function buildWorkspaceSource( } } - const source = `${form.repoUrl.trim()}${refSuffix(form.refType, form.refValue)}${subPathSuffix(form.subPath)}` + const repoUrl = form.repoUrl.trim() + const source = `${repoUrl}${refSuffix(form.refType, form.refValue, repoUrl)}${subPathSuffix(form.subPath)}` return { source, diff --git a/pkg/git/git.go b/pkg/git/git.go index 1193dbd98..932ae3efe 100644 --- a/pkg/git/git.go +++ b/pkg/git/git.go @@ -10,14 +10,16 @@ import ( ) const ( - CommitDelimiter string = "@sha256:" - PullRequestReference string = "pull/([0-9]+)/head" + CommitDelimiter string = "@sha256:" + // PullRequestReference matches a pull-request (GitHub) or merge-request + // (GitLab) refspec, capturing the request number. + PullRequestReference string = "(?:pull|merge-requests)/([0-9]+)/head" SubPathDelimiter string = "@subpath:" repoBaseRegEx string = `((?:(?:https?|git|ssh|file):\/\/)?\/?(?:[^@\/\n]+@)?` + `(?:[^:\/\n]+)(?:[:\/][^\/\n]+)+(?:\.git)?)` ) -// WARN: Make sure this matches the regex in /desktop/src/views/Workspaces/CreateWorkspace/CreateWorkspaceInput.tsx! +// WARN: Keep these in sync with the ref parsing in desktop's workspace-source.ts. var ( branchRegEx = regexp.MustCompile(`^` + repoBaseRegEx + `@([a-zA-Z0-9\./\-\_]+)$`) commitRegEx = regexp.MustCompile( @@ -36,7 +38,8 @@ var recognizedSchemes = []string{"ssh://", "git@", "http://", "https://", "file: // NormalizeRepository parses a repository reference into its structured parts. // Accepts plain URLs, the "git:" workspace-source scheme, and references -// suffixed with @branch, @subpath:, @sha256:, or @pull/N/head. +// suffixed with @branch, @subpath:, @sha256:, or a pull/merge +// request ref (@pull/N/head or @merge-requests/N/head). // Bare host[/path] inputs are upgraded to https://. func NormalizeRepository(str string) *GitInfo { str = canonicalizeURL(str) @@ -87,14 +90,23 @@ func PingRepository(str string, extraEnv []string) bool { return At("", WithEnv(extraEnv)).LsRemote(timeoutCtx, str) == nil } +// GetBranchNameForPR returns the local branch name for a request ref, using the +// provider convention encoded in the ref (PR for GitHub, MR for GitLab). func GetBranchNameForPR(ref string) string { - regex := regexp.MustCompile(PullRequestReference) - return regex.ReplaceAllString(ref, "PR${1}") + number := prNumber(ref) + if number == "" { + return ref + } + return hostForRef(ref).BranchName(number) } +// GetIDForPR returns the lowercased request identifier used in workspace IDs. func GetIDForPR(ref string) string { - regex := regexp.MustCompile(PullRequestReference) - return regex.ReplaceAllString(ref, "pr${1}") + number := prNumber(ref) + if number == "" { + return ref + } + return strings.ToLower(hostForRef(ref).BranchName(number)) } // GitInfo is the parsed form of a repository reference. Branch, Commit, PR, diff --git a/pkg/git/git_test.go b/pkg/git/git_test.go index db73297ff..bd8dac642 100644 --- a/pkg/git/git_test.go +++ b/pkg/git/git_test.go @@ -151,6 +151,14 @@ var normalizeRepositoryCases = []testCaseNormalizeRepository{ expectedCommit: "", expectedSubpath: "", }, + { + in: "git@gitlab.com:h3upperbounds/data/data-team.git@merge-requests/7125/head", + expectedRepo: "git@gitlab.com:h3upperbounds/data/data-team.git", + expectedPRReference: "merge-requests/7125/head", + expectedBranch: "", + expectedCommit: "", + expectedSubpath: "", + }, { in: "github.com/devsy-org/devsy-without-protocol-with-slash.git@subpath:/test/path", expectedRepo: repoDevsyNoProtoSlash, @@ -260,6 +268,10 @@ func TestGetBranchNameForPRReference(t *testing.T) { in: testPRRef, expectedBranch: "PR996", }, + { + in: "merge-requests/7125/head", + expectedBranch: "MR7125", + }, { in: "pull/abc/head", expectedBranch: "pull/abc/head", diff --git a/pkg/git/host.go b/pkg/git/host.go new file mode 100644 index 000000000..8589f33a0 --- /dev/null +++ b/pkg/git/host.go @@ -0,0 +1,75 @@ +package git + +import ( + "regexp" + "strings" +) + +var prNumberRegEx = regexp.MustCompile(`(?:pull|merge-requests)/([0-9]+)/head`) + +// Host is a git provider's convention for referencing pull/merge requests: +// GitHub exposes them at pull/N/head, GitLab at merge-requests/N/head. +type Host struct { + Name string + refPrefix string + branchAbbr string + hostHint string // substring identifying the provider in a repository URL +} + +var ( + HostGitHub = Host{Name: "github", refPrefix: "pull", branchAbbr: "PR", hostHint: "github"} + HostGitLab = Host{Name: "gitlab", refPrefix: "merge-requests", branchAbbr: "MR", hostHint: "gitlab"} + + // GitHub first: it is both the detection default and the first fallback. + knownHosts = []Host{HostGitHub, HostGitLab} +) + +func (h Host) Refspec(number string) string { + return h.refPrefix + "/" + number + "/head" +} + +func (h Host) BranchName(number string) string { + return h.branchAbbr + number +} + +// DetectHost picks the provider from a repository URL, defaulting to GitHub. +// It is best-effort: self-hosted instances on custom domains won't be +// recognized, which is why the fetch path falls back to the other convention. +func DetectHost(repoURL string) Host { + lower := strings.ToLower(repoURL) + for _, h := range knownHosts { + if strings.Contains(lower, h.hostHint) { + return h + } + } + return HostGitHub +} + +// hostForRef infers the provider from the ref itself, which already encodes the +// convention exactly (unlike DetectHost's URL heuristic). +func hostForRef(ref string) Host { + if strings.Contains(ref, HostGitLab.refPrefix+"/") { + return HostGitLab + } + return HostGitHub +} + +func prNumber(ref string) string { + if m := prNumberRegEx.FindStringSubmatch(ref); m != nil { + return m[1] + } + return "" +} + +// prCandidates orders the hosts to try for a checkout: detected host first, +// remaining conventions as fallbacks. +func prCandidates(repoURL string) []Host { + primary := DetectHost(repoURL) + candidates := []Host{primary} + for _, h := range knownHosts { + if h.Name != primary.Name { + candidates = append(candidates, h) + } + } + return candidates +} diff --git a/pkg/git/host_test.go b/pkg/git/host_test.go new file mode 100644 index 000000000..8de764d69 --- /dev/null +++ b/pkg/git/host_test.go @@ -0,0 +1,44 @@ +package git + +import ( + "testing" + + "gotest.tools/assert" + "gotest.tools/assert/cmp" +) + +func TestDetectHost(t *testing.T) { + cases := []struct { + url string + want string + }{ + {"git@github.com:org/repo.git", "github"}, + {"https://github.com/org/repo.git", "github"}, + {"git@gitlab.com:org/repo.git", "gitlab"}, + {"https://gitlab.example.com/org/repo.git", "gitlab"}, + {"https://git.internal.example/org/repo.git", "github"}, // unknown host defaults to GitHub + } + for _, c := range cases { + assert.Check(t, cmp.Equal(c.want, DetectHost(c.url).Name), "url=%s", c.url) + } +} + +func TestHostRefspecAndBranch(t *testing.T) { + assert.Equal(t, "pull/42/head", HostGitHub.Refspec("42")) + assert.Equal(t, "PR42", HostGitHub.BranchName("42")) + assert.Equal(t, "merge-requests/42/head", HostGitLab.Refspec("42")) + assert.Equal(t, "MR42", HostGitLab.BranchName("42")) +} + +func TestPRNumber(t *testing.T) { + assert.Equal(t, "996", prNumber("pull/996/head")) + assert.Equal(t, "7125", prNumber("merge-requests/7125/head")) + assert.Equal(t, "", prNumber("refs/heads/main")) +} + +func TestPRCandidatesOrder(t *testing.T) { + got := prCandidates("git@gitlab.com:org/repo.git") + assert.Equal(t, 2, len(got)) + assert.Equal(t, "gitlab", got[0].Name) // detected host first + assert.Equal(t, "github", got[1].Name) // fallback +} diff --git a/pkg/git/repo.go b/pkg/git/repo.go index 133c25dff..789500f63 100644 --- a/pkg/git/repo.go +++ b/pkg/git/repo.go @@ -79,15 +79,30 @@ func (r *Repo) Switch(ctx context.Context, branch string) error { return nil } -// CheckoutPR fetches the given pull request refspec and switches to a local branch for it. -func (r *Repo) CheckoutPR(ctx context.Context, prRef string) error { - log.Debugf("fetching pull request: %s", prRef) - - prBranch := GetBranchNameForPR(prRef) - if err := r.Fetch(ctx, prRef+":"+prBranch); err != nil { - return err +// CheckoutPR fetches a pull/merge request into a local branch and switches to +// it. The request number is taken from prRef; the remote refspec is resolved +// against repoURL's hosting provider, falling back to the other known +// conventions when the detected one has no such ref (e.g. self-hosted GitLab on +// a custom domain that URL detection can't recognize). +func (r *Repo) CheckoutPR(ctx context.Context, repoURL, prRef string) error { + number := prNumber(prRef) + if number == "" { + return fmt.Errorf("not a pull/merge request reference: %q", prRef) + } + + var lastErr error + for _, host := range prCandidates(repoURL) { + refspec := host.Refspec(number) + prBranch := host.BranchName(number) + log.Debugf("fetching %s request: %s", host.Name, refspec) + + if err := r.Fetch(ctx, refspec+":"+prBranch); err != nil { + lastErr = err + continue + } + return r.Switch(ctx, prBranch) } - return r.Switch(ctx, prBranch) + return lastErr } // Reset moves HEAD to commit using the given mode. @@ -140,7 +155,7 @@ func (r *Repo) CloneFromInfo( switch { case gitInfo.PR != "": - if err := r.CheckoutPR(ctx, gitInfo.PR); err != nil { + if err := r.CheckoutPR(ctx, gitInfo.Repository, gitInfo.PR); err != nil { return err } case gitInfo.Commit != "": diff --git a/pkg/git/repo_test.go b/pkg/git/repo_test.go index 8abad6c15..4145d9b8d 100644 --- a/pkg/git/repo_test.go +++ b/pkg/git/repo_test.go @@ -2,6 +2,7 @@ package git import ( "context" + "errors" "testing" "gotest.tools/assert" @@ -26,10 +27,16 @@ type fakeRunner struct { calls []RunOptions stdout []byte err error + // errUntil makes the first errUntil calls fail, simulating a missing + // remote ref that triggers the CheckoutPR fallback. + errUntil int } func (f *fakeRunner) Run(_ context.Context, opts RunOptions) (RunResult, error) { f.calls = append(f.calls, opts) + if len(f.calls) <= f.errUntil { + return RunResult{}, errors.New("couldn't find remote ref") + } return RunResult{Stdout: f.stdout}, f.err } @@ -61,12 +68,49 @@ func TestRepoCheckoutPR(t *testing.T) { fake := &fakeRunner{} repo := At("/tmp/repo", WithRunner(fake)) - assert.NilError(t, repo.CheckoutPR(context.Background(), testPRRef)) + assert.NilError(t, repo.CheckoutPR(context.Background(), "https://github.com/org/repo.git", testPRRef)) // Expect a fetch of the PR ref into a local branch, then a switch to it. assert.DeepEqual(t, []string{subFetch, originRemote, testPRRefSpec}, fake.calls[0].Args) assert.DeepEqual(t, []string{"switch", testPRLocal}, fake.calls[1].Args) } +func TestRepoCheckoutPRGitLab(t *testing.T) { + fake := &fakeRunner{} + repo := At("/tmp/repo", WithRunner(fake)) + + err := repo.CheckoutPR(context.Background(), "git@gitlab.com:org/repo.git", "merge-requests/7/head") + assert.NilError(t, err) + assert.DeepEqual(t, []string{subFetch, originRemote, "merge-requests/7/head:MR7"}, fake.calls[0].Args) + assert.DeepEqual(t, []string{"switch", "MR7"}, fake.calls[1].Args) +} + +// A GitLab MR requested with a GitHub-style ref still resolves: host detection +// from the URL picks GitLab and rewrites the refspec to merge-requests/N/head. +func TestRepoCheckoutPRGitLabFromGitHubStyleRef(t *testing.T) { + fake := &fakeRunner{} + repo := At("/tmp/repo", WithRunner(fake)) + + err := repo.CheckoutPR(context.Background(), "git@gitlab.com:org/repo.git", "pull/7/head") + assert.NilError(t, err) + assert.DeepEqual(t, []string{subFetch, originRemote, "merge-requests/7/head:MR7"}, fake.calls[0].Args) + assert.DeepEqual(t, []string{"switch", "MR7"}, fake.calls[1].Args) +} + +// When the detected host's ref is missing (undetectable self-hosted instance), +// the fetch falls back to the other known convention. +func TestRepoCheckoutPRFallback(t *testing.T) { + fake := &fakeRunner{errUntil: 1} + repo := At("/tmp/repo", WithRunner(fake)) + + // URL detection yields GitHub (default); its fetch fails, so the checkout + // falls back to the GitLab convention. + err := repo.CheckoutPR(context.Background(), "https://git.internal.example/org/repo.git", "pull/7/head") + assert.NilError(t, err) + assert.DeepEqual(t, []string{subFetch, originRemote, "pull/7/head:PR7"}, fake.calls[0].Args) + assert.DeepEqual(t, []string{subFetch, originRemote, "merge-requests/7/head:MR7"}, fake.calls[1].Args) + assert.DeepEqual(t, []string{"switch", "MR7"}, fake.calls[2].Args) +} + func TestRepoLsRemote(t *testing.T) { fake := &fakeRunner{} repo := At("", WithRunner(fake)) From 89ef50d7847d464a58f582379cb8dd4ea1487e3d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 21 Jul 2026 15:35:16 -0500 Subject: [PATCH 2/6] fix(git): satisfy goconst/golines linters in host tests --- pkg/git/host.go | 19 +++++++++++++++++-- pkg/git/host_test.go | 17 ++++++++++------- pkg/git/repo_test.go | 34 ++++++++++++++++++++++------------ 3 files changed, 49 insertions(+), 21 deletions(-) diff --git a/pkg/git/host.go b/pkg/git/host.go index 8589f33a0..6ab4bef2b 100644 --- a/pkg/git/host.go +++ b/pkg/git/host.go @@ -7,6 +7,11 @@ import ( var prNumberRegEx = regexp.MustCompile(`(?:pull|merge-requests)/([0-9]+)/head`) +const ( + hostNameGitHub = "github" + hostNameGitLab = "gitlab" +) + // Host is a git provider's convention for referencing pull/merge requests: // GitHub exposes them at pull/N/head, GitLab at merge-requests/N/head. type Host struct { @@ -17,8 +22,18 @@ type Host struct { } var ( - HostGitHub = Host{Name: "github", refPrefix: "pull", branchAbbr: "PR", hostHint: "github"} - HostGitLab = Host{Name: "gitlab", refPrefix: "merge-requests", branchAbbr: "MR", hostHint: "gitlab"} + HostGitHub = Host{ + Name: hostNameGitHub, + refPrefix: "pull", + branchAbbr: "PR", + hostHint: hostNameGitHub, + } + HostGitLab = Host{ + Name: hostNameGitLab, + refPrefix: "merge-requests", + branchAbbr: "MR", + hostHint: hostNameGitLab, + } // GitHub first: it is both the detection default and the first fallback. knownHosts = []Host{HostGitHub, HostGitLab} diff --git a/pkg/git/host_test.go b/pkg/git/host_test.go index 8de764d69..53c5f8786 100644 --- a/pkg/git/host_test.go +++ b/pkg/git/host_test.go @@ -12,11 +12,14 @@ func TestDetectHost(t *testing.T) { url string want string }{ - {"git@github.com:org/repo.git", "github"}, - {"https://github.com/org/repo.git", "github"}, - {"git@gitlab.com:org/repo.git", "gitlab"}, - {"https://gitlab.example.com/org/repo.git", "gitlab"}, - {"https://git.internal.example/org/repo.git", "github"}, // unknown host defaults to GitHub + {"git@github.com:org/repo.git", hostNameGitHub}, + {"https://github.com/org/repo.git", hostNameGitHub}, + {"git@gitlab.com:org/repo.git", hostNameGitLab}, + {"https://gitlab.example.com/org/repo.git", hostNameGitLab}, + { + "https://git.internal.example/org/repo.git", + hostNameGitHub, + }, // unknown host defaults to GitHub } for _, c := range cases { assert.Check(t, cmp.Equal(c.want, DetectHost(c.url).Name), "url=%s", c.url) @@ -39,6 +42,6 @@ func TestPRNumber(t *testing.T) { func TestPRCandidatesOrder(t *testing.T) { got := prCandidates("git@gitlab.com:org/repo.git") assert.Equal(t, 2, len(got)) - assert.Equal(t, "gitlab", got[0].Name) // detected host first - assert.Equal(t, "github", got[1].Name) // fallback + assert.Equal(t, hostNameGitLab, got[0].Name) // detected host first + assert.Equal(t, hostNameGitHub, got[1].Name) // fallback } diff --git a/pkg/git/repo_test.go b/pkg/git/repo_test.go index 4145d9b8d..bba65d45e 100644 --- a/pkg/git/repo_test.go +++ b/pkg/git/repo_test.go @@ -12,11 +12,16 @@ import ( // Shared literals used across Repo tests. const ( testRepoURL = "git@host:org/repo.git" + testGitHubURL = "https://github.com/org/repo.git" + testGitLabURL = "git@gitlab.com:org/repo.git" testPRRef = "pull/996/head" testPRLocal = "PR996" testPRRefSpec = testPRRef + ":" + testPRLocal + testMRLocal = "MR7" + testMRRefSpec = "merge-requests/7/head:" + testMRLocal testCommit = "abc123" subFetch = "fetch" + subSwitch = "switch" originRemote = "origin" testTarget = "/tmp/target" ) @@ -68,20 +73,21 @@ func TestRepoCheckoutPR(t *testing.T) { fake := &fakeRunner{} repo := At("/tmp/repo", WithRunner(fake)) - assert.NilError(t, repo.CheckoutPR(context.Background(), "https://github.com/org/repo.git", testPRRef)) + err := repo.CheckoutPR(context.Background(), testGitHubURL, testPRRef) + assert.NilError(t, err) // Expect a fetch of the PR ref into a local branch, then a switch to it. assert.DeepEqual(t, []string{subFetch, originRemote, testPRRefSpec}, fake.calls[0].Args) - assert.DeepEqual(t, []string{"switch", testPRLocal}, fake.calls[1].Args) + assert.DeepEqual(t, []string{subSwitch, testPRLocal}, fake.calls[1].Args) } func TestRepoCheckoutPRGitLab(t *testing.T) { fake := &fakeRunner{} repo := At("/tmp/repo", WithRunner(fake)) - err := repo.CheckoutPR(context.Background(), "git@gitlab.com:org/repo.git", "merge-requests/7/head") + err := repo.CheckoutPR(context.Background(), testGitLabURL, "merge-requests/7/head") assert.NilError(t, err) - assert.DeepEqual(t, []string{subFetch, originRemote, "merge-requests/7/head:MR7"}, fake.calls[0].Args) - assert.DeepEqual(t, []string{"switch", "MR7"}, fake.calls[1].Args) + assert.DeepEqual(t, []string{subFetch, originRemote, testMRRefSpec}, fake.calls[0].Args) + assert.DeepEqual(t, []string{subSwitch, testMRLocal}, fake.calls[1].Args) } // A GitLab MR requested with a GitHub-style ref still resolves: host detection @@ -90,10 +96,10 @@ func TestRepoCheckoutPRGitLabFromGitHubStyleRef(t *testing.T) { fake := &fakeRunner{} repo := At("/tmp/repo", WithRunner(fake)) - err := repo.CheckoutPR(context.Background(), "git@gitlab.com:org/repo.git", "pull/7/head") + err := repo.CheckoutPR(context.Background(), testGitLabURL, "pull/7/head") assert.NilError(t, err) - assert.DeepEqual(t, []string{subFetch, originRemote, "merge-requests/7/head:MR7"}, fake.calls[0].Args) - assert.DeepEqual(t, []string{"switch", "MR7"}, fake.calls[1].Args) + assert.DeepEqual(t, []string{subFetch, originRemote, testMRRefSpec}, fake.calls[0].Args) + assert.DeepEqual(t, []string{subSwitch, testMRLocal}, fake.calls[1].Args) } // When the detected host's ref is missing (undetectable self-hosted instance), @@ -104,11 +110,15 @@ func TestRepoCheckoutPRFallback(t *testing.T) { // URL detection yields GitHub (default); its fetch fails, so the checkout // falls back to the GitLab convention. - err := repo.CheckoutPR(context.Background(), "https://git.internal.example/org/repo.git", "pull/7/head") + err := repo.CheckoutPR( + context.Background(), + "https://git.internal.example/org/repo.git", + "pull/7/head", + ) assert.NilError(t, err) assert.DeepEqual(t, []string{subFetch, originRemote, "pull/7/head:PR7"}, fake.calls[0].Args) - assert.DeepEqual(t, []string{subFetch, originRemote, "merge-requests/7/head:MR7"}, fake.calls[1].Args) - assert.DeepEqual(t, []string{"switch", "MR7"}, fake.calls[2].Args) + assert.DeepEqual(t, []string{subFetch, originRemote, testMRRefSpec}, fake.calls[1].Args) + assert.DeepEqual(t, []string{subSwitch, testMRLocal}, fake.calls[2].Args) } func TestRepoLsRemote(t *testing.T) { @@ -204,7 +214,7 @@ func TestRepoCloneFromInfoPR(t *testing.T) { assert.NilError(t, repo.CloneFromInfo(context.Background(), info, "")) assert.Equal(t, subClone, fake.calls[0].Args[0]) assert.DeepEqual(t, []string{subFetch, originRemote, testPRRefSpec}, fake.calls[1].Args) - assert.DeepEqual(t, []string{"switch", testPRLocal}, fake.calls[2].Args) + assert.DeepEqual(t, []string{subSwitch, testPRLocal}, fake.calls[2].Args) } func TestRepoCloneFromInfoHelper(t *testing.T) { From 8071c1a7abf6c642841a3b9b8af3cf48b366eb86 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 21 Jul 2026 16:21:34 -0500 Subject: [PATCH 3/6] fix(git): scope host detection to hostname and fallback to missing refs only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review: DetectHost matched the whole URL, so an owner/path containing 'gitlab'/'github' could misdetect the provider — now matches only the hostname. CheckoutPR's fallback fired on any fetch error, masking auth/network failures behind a second lookup's error — now only missing-ref errors trigger the alternate-provider retry. --- .../src/lib/utils/workspace-source.test.ts | 7 +++++ .../src/lib/utils/workspace-source.ts | 16 ++++++++++- pkg/git/host.go | 22 +++++++++++++-- pkg/git/host_test.go | 3 ++ pkg/git/repo.go | 28 ++++++++++++++++--- pkg/git/repo_test.go | 25 ++++++++++++++++- 6 files changed, 93 insertions(+), 8 deletions(-) diff --git a/desktop/src/renderer/src/lib/utils/workspace-source.test.ts b/desktop/src/renderer/src/lib/utils/workspace-source.test.ts index 2e5cfe3c8..8c635e41f 100644 --- a/desktop/src/renderer/src/lib/utils/workspace-source.test.ts +++ b/desktop/src/renderer/src/lib/utils/workspace-source.test.ts @@ -80,6 +80,13 @@ describe("buildWorkspaceSource", () => { expect(out.source).toBe("git@gitlab.example.com:org/repo.git@merge-requests/7/head") }) + it("git: PR ref uses pull/N/head when only the owner/path says gitlab", () => { + const out = buildWorkspaceSource( + gitForm({ repoUrl: "git@github.com:gitlab-org/repo.git", refType: "pr", refValue: "7" }), + ) + expect(out.source).toBe("git@github.com:gitlab-org/repo.git@pull/7/head") + }) + it("git: subpath appends @subpath: after ref", () => { const out = buildWorkspaceSource( gitForm({ diff --git a/desktop/src/renderer/src/lib/utils/workspace-source.ts b/desktop/src/renderer/src/lib/utils/workspace-source.ts index 26fdf9f6f..d56f97c95 100644 --- a/desktop/src/renderer/src/lib/utils/workspace-source.ts +++ b/desktop/src/renderer/src/lib/utils/workspace-source.ts @@ -34,11 +34,25 @@ export interface WorkspaceSourceResult { prebuildRepository?: string } +// hostFromUrl extracts the hostname from SSH (git@host:path) and URL +// (scheme://[user@]host/path) forms, so an owner or path segment can't sway +// provider detection. +function hostFromUrl(repoUrl: string): string { + let s = repoUrl + const scheme = s.indexOf("://") + if (scheme !== -1) s = s.slice(scheme + 3) + const at = s.indexOf("@") + if (at !== -1) s = s.slice(at + 1) + const sep = s.search(/[/:]/) + if (sep !== -1) s = s.slice(0, sep) + return s +} + // GitLab exposes merge requests at merge-requests/N/head; every other host // uses pull/N/head. Detection is best-effort — the CLI fetch falls back to the // other convention when the detected one has no such ref. function prRefspec(repoUrl: string, number: string): string { - const segment = /gitlab/i.test(repoUrl) ? "merge-requests" : "pull" + const segment = /gitlab/i.test(hostFromUrl(repoUrl)) ? "merge-requests" : "pull" return `@${segment}/${number}/head` } diff --git a/pkg/git/host.go b/pkg/git/host.go index 6ab4bef2b..cb57720ff 100644 --- a/pkg/git/host.go +++ b/pkg/git/host.go @@ -51,15 +51,33 @@ func (h Host) BranchName(number string) string { // It is best-effort: self-hosted instances on custom domains won't be // recognized, which is why the fetch path falls back to the other convention. func DetectHost(repoURL string) Host { - lower := strings.ToLower(repoURL) + // Match only the hostname so an owner or path segment (e.g. a GitHub repo + // named "gitlab-ci") cannot masquerade as another provider. + host := strings.ToLower(hostFromURL(repoURL)) for _, h := range knownHosts { - if strings.Contains(lower, h.hostHint) { + if strings.Contains(host, h.hostHint) { return h } } return HostGitHub } +// hostFromURL extracts the hostname from SSH (git@host:path) and URL +// (scheme://[user@]host/path) forms, returning "" when none is present. +func hostFromURL(repoURL string) string { + s := repoURL + if i := strings.Index(s, "://"); i != -1 { + s = s[i+3:] + } + if i := strings.Index(s, "@"); i != -1 { + s = s[i+1:] + } + if i := strings.IndexAny(s, "/:"); i != -1 { + s = s[:i] + } + return s +} + // hostForRef infers the provider from the ref itself, which already encodes the // convention exactly (unlike DetectHost's URL heuristic). func hostForRef(ref string) Host { diff --git a/pkg/git/host_test.go b/pkg/git/host_test.go index 53c5f8786..3e9774adb 100644 --- a/pkg/git/host_test.go +++ b/pkg/git/host_test.go @@ -16,6 +16,9 @@ func TestDetectHost(t *testing.T) { {"https://github.com/org/repo.git", hostNameGitHub}, {"git@gitlab.com:org/repo.git", hostNameGitLab}, {"https://gitlab.example.com/org/repo.git", hostNameGitLab}, + // Only the hostname decides: a "gitlab" owner/path on github.com is GitHub. + {"git@github.com:gitlab-org/repo.git", hostNameGitHub}, + {"https://github.com/org/gitlab-mirror.git", hostNameGitHub}, { "https://git.internal.example/org/repo.git", hostNameGitHub, diff --git a/pkg/git/repo.go b/pkg/git/repo.go index 789500f63..d1b627287 100644 --- a/pkg/git/repo.go +++ b/pkg/git/repo.go @@ -2,6 +2,7 @@ package git import ( "context" + "errors" "fmt" "strings" @@ -96,15 +97,34 @@ func (r *Repo) CheckoutPR(ctx context.Context, repoURL, prRef string) error { prBranch := host.BranchName(number) log.Debugf("fetching %s request: %s", host.Name, refspec) - if err := r.Fetch(ctx, refspec+":"+prBranch); err != nil { - lastErr = err - continue + err := r.Fetch(ctx, refspec+":"+prBranch) + if err == nil { + return r.Switch(ctx, prBranch) } - return r.Switch(ctx, prBranch) + // Only try the next provider's convention when this refspec is genuinely + // absent; auth, network, and cancellation errors must surface as-is + // rather than be masked by a subsequent lookup's failure. + if !isMissingRefError(err) { + return err + } + lastErr = err } return lastErr } +// isMissingRefError reports whether err is git failing to find the requested +// remote ref, as opposed to an auth, network, or cancellation failure. +func isMissingRefError(err error) bool { + var cmdErr *CommandError + if !errors.As(err, &cmdErr) { + return false + } + msg := strings.ToLower(cmdErr.Stderr) + return strings.Contains(msg, "couldn't find remote ref") || + strings.Contains(msg, "no such ref") || + strings.Contains(msg, "not found in upstream") +} + // Reset moves HEAD to commit using the given mode. func (r *Repo) Reset(ctx context.Context, commit string, mode ResetMode) error { if err := r.runLogged(ctx, "reset", string(mode), commit); err != nil { diff --git a/pkg/git/repo_test.go b/pkg/git/repo_test.go index bba65d45e..b2d0f997b 100644 --- a/pkg/git/repo_test.go +++ b/pkg/git/repo_test.go @@ -40,7 +40,12 @@ type fakeRunner struct { func (f *fakeRunner) Run(_ context.Context, opts RunOptions) (RunResult, error) { f.calls = append(f.calls, opts) if len(f.calls) <= f.errUntil { - return RunResult{}, errors.New("couldn't find remote ref") + return RunResult{}, &CommandError{ + Args: opts.Args, + ExitCode: 128, + Stderr: "fatal: couldn't find remote ref " + opts.Args[len(opts.Args)-1], + Err: errors.New("exit status 128"), + } } return RunResult{Stdout: f.stdout}, f.err } @@ -121,6 +126,24 @@ func TestRepoCheckoutPRFallback(t *testing.T) { assert.DeepEqual(t, []string{subSwitch, testMRLocal}, fake.calls[2].Args) } +// A non-ref fetch failure (auth, network, …) must surface immediately without +// being masked by the alternate-provider fallback. +func TestRepoCheckoutPRNonRefErrorNoFallback(t *testing.T) { + authErr := &CommandError{ + Args: []string{subFetch}, + ExitCode: 128, + Stderr: "fatal: Authentication failed for 'https://host/org/repo.git'", + Err: errors.New("exit status 128"), + } + fake := &fakeRunner{err: authErr} + repo := At("/tmp/repo", WithRunner(fake)) + + err := repo.CheckoutPR(context.Background(), testGitHubURL, testPRRef) + assert.ErrorContains(t, err, "Authentication failed") + // Only the first candidate is attempted; the auth error is not retried. + assert.Equal(t, 1, len(fake.calls)) +} + func TestRepoLsRemote(t *testing.T) { fake := &fakeRunner{} repo := At("", WithRunner(fake)) From 301ffa8bf7c8521f11d0aa9d1ceaba63c423ad6a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 21 Jul 2026 16:48:25 -0500 Subject: [PATCH 4/6] chore: update comments --- .../src/renderer/src/lib/utils/workspace-source.ts | 6 ++---- pkg/git/git.go | 4 +--- pkg/git/host.go | 11 ++--------- pkg/git/repo.go | 3 --- pkg/git/repo_test.go | 12 ------------ 5 files changed, 5 insertions(+), 31 deletions(-) diff --git a/desktop/src/renderer/src/lib/utils/workspace-source.ts b/desktop/src/renderer/src/lib/utils/workspace-source.ts index d56f97c95..859ce3da0 100644 --- a/desktop/src/renderer/src/lib/utils/workspace-source.ts +++ b/desktop/src/renderer/src/lib/utils/workspace-source.ts @@ -35,8 +35,7 @@ export interface WorkspaceSourceResult { } // hostFromUrl extracts the hostname from SSH (git@host:path) and URL -// (scheme://[user@]host/path) forms, so an owner or path segment can't sway -// provider detection. +// (scheme://[user@]host/path) forms. function hostFromUrl(repoUrl: string): string { let s = repoUrl const scheme = s.indexOf("://") @@ -49,8 +48,7 @@ function hostFromUrl(repoUrl: string): string { } // GitLab exposes merge requests at merge-requests/N/head; every other host -// uses pull/N/head. Detection is best-effort — the CLI fetch falls back to the -// other convention when the detected one has no such ref. +// uses pull/N/head. function prRefspec(repoUrl: string, number: string): string { const segment = /gitlab/i.test(hostFromUrl(repoUrl)) ? "merge-requests" : "pull" return `@${segment}/${number}/head` diff --git a/pkg/git/git.go b/pkg/git/git.go index 932ae3efe..bcaeb473b 100644 --- a/pkg/git/git.go +++ b/pkg/git/git.go @@ -11,15 +11,13 @@ import ( const ( CommitDelimiter string = "@sha256:" - // PullRequestReference matches a pull-request (GitHub) or merge-request - // (GitLab) refspec, capturing the request number. PullRequestReference string = "(?:pull|merge-requests)/([0-9]+)/head" SubPathDelimiter string = "@subpath:" repoBaseRegEx string = `((?:(?:https?|git|ssh|file):\/\/)?\/?(?:[^@\/\n]+@)?` + `(?:[^:\/\n]+)(?:[:\/][^\/\n]+)+(?:\.git)?)` ) -// WARN: Keep these in sync with the ref parsing in desktop's workspace-source.ts. +// WARN: Keep these in sync with the ref parsing in UI var ( branchRegEx = regexp.MustCompile(`^` + repoBaseRegEx + `@([a-zA-Z0-9\./\-\_]+)$`) commitRegEx = regexp.MustCompile( diff --git a/pkg/git/host.go b/pkg/git/host.go index cb57720ff..4f1704924 100644 --- a/pkg/git/host.go +++ b/pkg/git/host.go @@ -35,7 +35,6 @@ var ( hostHint: hostNameGitLab, } - // GitHub first: it is both the detection default and the first fallback. knownHosts = []Host{HostGitHub, HostGitLab} ) @@ -48,11 +47,7 @@ func (h Host) BranchName(number string) string { } // DetectHost picks the provider from a repository URL, defaulting to GitHub. -// It is best-effort: self-hosted instances on custom domains won't be -// recognized, which is why the fetch path falls back to the other convention. func DetectHost(repoURL string) Host { - // Match only the hostname so an owner or path segment (e.g. a GitHub repo - // named "gitlab-ci") cannot masquerade as another provider. host := strings.ToLower(hostFromURL(repoURL)) for _, h := range knownHosts { if strings.Contains(host, h.hostHint) { @@ -78,8 +73,7 @@ func hostFromURL(repoURL string) string { return s } -// hostForRef infers the provider from the ref itself, which already encodes the -// convention exactly (unlike DetectHost's URL heuristic). +// hostForRef infers the provider from the ref itself. func hostForRef(ref string) Host { if strings.Contains(ref, HostGitLab.refPrefix+"/") { return HostGitLab @@ -94,8 +88,7 @@ func prNumber(ref string) string { return "" } -// prCandidates orders the hosts to try for a checkout: detected host first, -// remaining conventions as fallbacks. +// prCandidates orders the hosts to try for a checkout. func prCandidates(repoURL string) []Host { primary := DetectHost(repoURL) candidates := []Host{primary} diff --git a/pkg/git/repo.go b/pkg/git/repo.go index d1b627287..20d18838a 100644 --- a/pkg/git/repo.go +++ b/pkg/git/repo.go @@ -101,9 +101,6 @@ func (r *Repo) CheckoutPR(ctx context.Context, repoURL, prRef string) error { if err == nil { return r.Switch(ctx, prBranch) } - // Only try the next provider's convention when this refspec is genuinely - // absent; auth, network, and cancellation errors must surface as-is - // rather than be masked by a subsequent lookup's failure. if !isMissingRefError(err) { return err } diff --git a/pkg/git/repo_test.go b/pkg/git/repo_test.go index b2d0f997b..3431e5f4c 100644 --- a/pkg/git/repo_test.go +++ b/pkg/git/repo_test.go @@ -32,8 +32,6 @@ type fakeRunner struct { calls []RunOptions stdout []byte err error - // errUntil makes the first errUntil calls fail, simulating a missing - // remote ref that triggers the CheckoutPR fallback. errUntil int } @@ -80,7 +78,6 @@ func TestRepoCheckoutPR(t *testing.T) { err := repo.CheckoutPR(context.Background(), testGitHubURL, testPRRef) assert.NilError(t, err) - // Expect a fetch of the PR ref into a local branch, then a switch to it. assert.DeepEqual(t, []string{subFetch, originRemote, testPRRefSpec}, fake.calls[0].Args) assert.DeepEqual(t, []string{subSwitch, testPRLocal}, fake.calls[1].Args) } @@ -95,8 +92,6 @@ func TestRepoCheckoutPRGitLab(t *testing.T) { assert.DeepEqual(t, []string{subSwitch, testMRLocal}, fake.calls[1].Args) } -// A GitLab MR requested with a GitHub-style ref still resolves: host detection -// from the URL picks GitLab and rewrites the refspec to merge-requests/N/head. func TestRepoCheckoutPRGitLabFromGitHubStyleRef(t *testing.T) { fake := &fakeRunner{} repo := At("/tmp/repo", WithRunner(fake)) @@ -107,14 +102,10 @@ func TestRepoCheckoutPRGitLabFromGitHubStyleRef(t *testing.T) { assert.DeepEqual(t, []string{subSwitch, testMRLocal}, fake.calls[1].Args) } -// When the detected host's ref is missing (undetectable self-hosted instance), -// the fetch falls back to the other known convention. func TestRepoCheckoutPRFallback(t *testing.T) { fake := &fakeRunner{errUntil: 1} repo := At("/tmp/repo", WithRunner(fake)) - // URL detection yields GitHub (default); its fetch fails, so the checkout - // falls back to the GitLab convention. err := repo.CheckoutPR( context.Background(), "https://git.internal.example/org/repo.git", @@ -126,8 +117,6 @@ func TestRepoCheckoutPRFallback(t *testing.T) { assert.DeepEqual(t, []string{subSwitch, testMRLocal}, fake.calls[2].Args) } -// A non-ref fetch failure (auth, network, …) must surface immediately without -// being masked by the alternate-provider fallback. func TestRepoCheckoutPRNonRefErrorNoFallback(t *testing.T) { authErr := &CommandError{ Args: []string{subFetch}, @@ -140,7 +129,6 @@ func TestRepoCheckoutPRNonRefErrorNoFallback(t *testing.T) { err := repo.CheckoutPR(context.Background(), testGitHubURL, testPRRef) assert.ErrorContains(t, err, "Authentication failed") - // Only the first candidate is attempted; the auth error is not retried. assert.Equal(t, 1, len(fake.calls)) } From a0184fe9c8647ff32b4c867512fba8ef37e7b41f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 21 Jul 2026 16:56:18 -0500 Subject: [PATCH 5/6] fix: linting errors --- pkg/git/git.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/git/git.go b/pkg/git/git.go index bcaeb473b..2693a684f 100644 --- a/pkg/git/git.go +++ b/pkg/git/git.go @@ -10,14 +10,14 @@ import ( ) const ( - CommitDelimiter string = "@sha256:" + CommitDelimiter string = "@sha256:" PullRequestReference string = "(?:pull|merge-requests)/([0-9]+)/head" SubPathDelimiter string = "@subpath:" repoBaseRegEx string = `((?:(?:https?|git|ssh|file):\/\/)?\/?(?:[^@\/\n]+@)?` + `(?:[^:\/\n]+)(?:[:\/][^\/\n]+)+(?:\.git)?)` ) -// WARN: Keep these in sync with the ref parsing in UI +// WARN: Keep these in sync with the ref parsing in UI. var ( branchRegEx = regexp.MustCompile(`^` + repoBaseRegEx + `@([a-zA-Z0-9\./\-\_]+)$`) commitRegEx = regexp.MustCompile( From 95f5c7abc8cef2842d3252d805916274cef37d92 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 21 Jul 2026 17:05:35 -0500 Subject: [PATCH 6/6] fix: spacing --- pkg/git/repo_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/git/repo_test.go b/pkg/git/repo_test.go index 3431e5f4c..39a6d5adb 100644 --- a/pkg/git/repo_test.go +++ b/pkg/git/repo_test.go @@ -29,9 +29,9 @@ const ( // fakeRunner records the invocations it receives and returns canned output, // letting git operations be tested without a real repository. type fakeRunner struct { - calls []RunOptions - stdout []byte - err error + calls []RunOptions + stdout []byte + err error errUntil int }