From d0a032fa605b3862dfcbc36d1a2bfe7839c05e68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Mon, 29 Jun 2026 14:17:14 +0100 Subject: [PATCH] fix: prefer main for worktree base branches (#1532) --- .../internal/backendapp/adapters_test.go | 265 ++++++++++++++++++ .../internal/backendapp/orchestrator.go | 77 ++--- apps/backend/internal/common/gitref/gitref.go | 90 ++++-- .../internal/common/gitref/gitref_test.go | 159 +++++++++++ .../orchestrator/executor/executor_resume.go | 2 +- .../task/service/repository_discovery_test.go | 10 +- .../internal/task/service/service_tasks.go | 2 +- apps/web/lib/utils.test.ts | 11 +- 8 files changed, 550 insertions(+), 66 deletions(-) diff --git a/apps/backend/internal/backendapp/adapters_test.go b/apps/backend/internal/backendapp/adapters_test.go index 45aa924fd3..b4d72b111c 100644 --- a/apps/backend/internal/backendapp/adapters_test.go +++ b/apps/backend/internal/backendapp/adapters_test.go @@ -4,11 +4,14 @@ import ( "context" "errors" "fmt" + "os" + "path/filepath" "sync" "testing" "github.com/kandev/kandev/internal/common/logger" githubsvc "github.com/kandev/kandev/internal/github" + "github.com/kandev/kandev/internal/repoclone" taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" taskservice "github.com/kandev/kandev/internal/task/service" ) @@ -21,6 +24,268 @@ func newTestLogger() *logger.Logger { return log } +func TestDetectGitDefaultBranchDetachedHEADReturnsEmpty(t *testing.T) { + repoPath := t.TempDir() + gitDir := filepath.Join(repoPath, ".git") + if err := os.MkdirAll(gitDir, 0o755); err != nil { + t.Fatalf("mkdir git dir: %v", err) + } + if err := os.WriteFile( + filepath.Join(gitDir, "HEAD"), + []byte("3a3f2d3b00000000000000000000000000000000\n"), + 0o644, + ); err != nil { + t.Fatalf("write detached HEAD: %v", err) + } + + if got := detectGitDefaultBranch(repoPath); got != "" { + t.Fatalf("detectGitDefaultBranch = %q, want empty", got) + } +} + +func TestResolveReviewBaseBranchRedetectsStoredMasterWhenMainExists(t *testing.T) { + harness := newBootStateTestHarness(t) + ctx := context.Background() + workspace, err := harness.taskSvc.CreateWorkspace(ctx, &taskservice.CreateWorkspaceRequest{ + Name: "Workspace", + }) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + repoPath := t.TempDir() + gitDir := filepath.Join(repoPath, ".git") + if err := os.MkdirAll(filepath.Join(gitDir, "refs", "remotes", "origin"), 0o755); err != nil { + t.Fatalf("mkdir origin refs: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/feature/x\n"), 0o644); err != nil { + t.Fatalf("write HEAD: %v", err) + } + if err := os.WriteFile( + filepath.Join(gitDir, "refs", "remotes", "origin", "HEAD"), + []byte("ref: refs/remotes/origin/master\n"), + 0o644, + ); err != nil { + t.Fatalf("write origin HEAD: %v", err) + } + for _, branch := range []string{"main", "master"} { + refPath := filepath.Join(gitDir, "refs", "remotes", "origin", branch) + if err := os.WriteFile(refPath, []byte("0000000\n"), 0o644); err != nil { + t.Fatalf("write %s ref: %v", branch, err) + } + } + repo, err := harness.taskSvc.CreateRepository(ctx, &taskservice.CreateRepositoryRequest{ + WorkspaceID: workspace.ID, + Name: "owner/repo", + SourceType: "provider", + LocalPath: repoPath, + Provider: "github", + ProviderOwner: "owner", + ProviderName: "repo", + DefaultBranch: "master", + }) + if err != nil { + t.Fatalf("CreateRepository: %v", err) + } + + adapter := &repositoryResolverAdapter{ + taskSvc: harness.taskSvc, + logger: newTestLogger(), + } + if got := adapter.resolveReviewBaseBranch(ctx, repo, repoPath, ""); got != "main" { + t.Fatalf("resolveReviewBaseBranch = %q, want main", got) + } + + stored, err := harness.taskSvc.GetRepository(ctx, repo.ID) + if err != nil { + t.Fatalf("GetRepository: %v", err) + } + if stored.DefaultBranch != "main" { + t.Fatalf("stored default_branch = %q, want main", stored.DefaultBranch) + } +} + +func TestResolveReviewBaseBranchSkipsNoopPersistForStoredMaster(t *testing.T) { + harness := newBootStateTestHarness(t) + ctx := context.Background() + workspace, err := harness.taskSvc.CreateWorkspace(ctx, &taskservice.CreateWorkspaceRequest{ + Name: "Workspace", + }) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + repoPath := t.TempDir() + gitDir := filepath.Join(repoPath, ".git") + if err := os.MkdirAll(filepath.Join(gitDir, "refs", "remotes", "origin"), 0o755); err != nil { + t.Fatalf("mkdir origin refs: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/feature/x\n"), 0o644); err != nil { + t.Fatalf("write HEAD: %v", err) + } + if err := os.WriteFile( + filepath.Join(gitDir, "refs", "remotes", "origin", "HEAD"), + []byte("ref: refs/remotes/origin/master\n"), + 0o644, + ); err != nil { + t.Fatalf("write origin HEAD: %v", err) + } + refPath := filepath.Join(gitDir, "refs", "remotes", "origin", "master") + if err := os.WriteFile(refPath, []byte("0000000\n"), 0o644); err != nil { + t.Fatalf("write master ref: %v", err) + } + repo, err := harness.taskSvc.CreateRepository(ctx, &taskservice.CreateRepositoryRequest{ + WorkspaceID: workspace.ID, + Name: "owner/repo", + SourceType: "provider", + LocalPath: repoPath, + Provider: "github", + ProviderOwner: "owner", + ProviderName: "repo", + DefaultBranch: "master", + }) + if err != nil { + t.Fatalf("CreateRepository: %v", err) + } + + adapter := &repositoryResolverAdapter{ + taskSvc: harness.taskSvc, + logger: newTestLogger(), + } + if got := adapter.resolveReviewBaseBranch(ctx, repo, repoPath, ""); got != "master" { + t.Fatalf("resolveReviewBaseBranch = %q, want master", got) + } + + stored, err := harness.taskSvc.GetRepository(ctx, repo.ID) + if err != nil { + t.Fatalf("GetRepository: %v", err) + } + if !stored.UpdatedAt.Equal(repo.UpdatedAt) { + t.Fatalf("stored updated_at = %v, want unchanged %v", stored.UpdatedAt, repo.UpdatedAt) + } +} + +func TestResolveReviewBaseBranchKeepsStoredMasterOnHeadFallback(t *testing.T) { + harness := newBootStateTestHarness(t) + ctx := context.Background() + workspace, err := harness.taskSvc.CreateWorkspace(ctx, &taskservice.CreateWorkspaceRequest{ + Name: "Workspace", + }) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + repoPath := t.TempDir() + gitDir := filepath.Join(repoPath, ".git") + if err := os.MkdirAll(gitDir, 0o755); err != nil { + t.Fatalf("mkdir git dir: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/feature/x\n"), 0o644); err != nil { + t.Fatalf("write HEAD: %v", err) + } + repo, err := harness.taskSvc.CreateRepository(ctx, &taskservice.CreateRepositoryRequest{ + WorkspaceID: workspace.ID, + Name: "owner/repo", + SourceType: "provider", + LocalPath: repoPath, + Provider: "github", + ProviderOwner: "owner", + ProviderName: "repo", + DefaultBranch: "master", + }) + if err != nil { + t.Fatalf("CreateRepository: %v", err) + } + + adapter := &repositoryResolverAdapter{ + taskSvc: harness.taskSvc, + logger: newTestLogger(), + } + if got := adapter.resolveReviewBaseBranch(ctx, repo, repoPath, ""); got != "master" { + t.Fatalf("resolveReviewBaseBranch = %q, want master", got) + } + + stored, err := harness.taskSvc.GetRepository(ctx, repo.ID) + if err != nil { + t.Fatalf("GetRepository: %v", err) + } + if stored.DefaultBranch != "master" { + t.Fatalf("stored default_branch = %q, want master", stored.DefaultBranch) + } + if !stored.UpdatedAt.Equal(repo.UpdatedAt) { + t.Fatalf("stored updated_at = %v, want unchanged %v", stored.UpdatedAt, repo.UpdatedAt) + } +} + +func TestResolveForReviewRedetectsStoredMasterAfterClonePath(t *testing.T) { + harness := newBootStateTestHarness(t) + ctx := context.Background() + workspace, err := harness.taskSvc.CreateWorkspace(ctx, &taskservice.CreateWorkspaceRequest{ + Name: "Workspace", + }) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + basePath := t.TempDir() + repoPath := filepath.Join(basePath, "owner", "repo") + gitDir := filepath.Join(repoPath, ".git") + if err := os.MkdirAll(filepath.Join(gitDir, "refs", "remotes", "origin"), 0o755); err != nil { + t.Fatalf("mkdir origin refs: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/feature/x\n"), 0o644); err != nil { + t.Fatalf("write HEAD: %v", err) + } + if err := os.WriteFile( + filepath.Join(gitDir, "refs", "remotes", "origin", "HEAD"), + []byte("ref: refs/remotes/origin/master\n"), + 0o644, + ); err != nil { + t.Fatalf("write origin HEAD: %v", err) + } + for _, branch := range []string{"main", "master"} { + refPath := filepath.Join(gitDir, "refs", "remotes", "origin", branch) + if err := os.WriteFile(refPath, []byte("0000000\n"), 0o644); err != nil { + t.Fatalf("write %s ref: %v", branch, err) + } + } + repo, err := harness.taskSvc.CreateRepository(ctx, &taskservice.CreateRepositoryRequest{ + WorkspaceID: workspace.ID, + Name: "owner/repo", + SourceType: "provider", + Provider: "github", + ProviderOwner: "owner", + ProviderName: "repo", + DefaultBranch: "master", + }) + if err != nil { + t.Fatalf("CreateRepository: %v", err) + } + + adapter := &repositoryResolverAdapter{ + cloner: repoclone.NewCloner(repoclone.Config{BasePath: basePath}, repoclone.ProtocolHTTPS, "", newTestLogger()), + taskSvc: harness.taskSvc, + logger: newTestLogger(), + } + repoID, baseBranch, err := adapter.ResolveForReview(ctx, workspace.ID, "github", "owner", "repo", "") + if err != nil { + t.Fatalf("ResolveForReview: %v", err) + } + if repoID != repo.ID { + t.Fatalf("repository ID = %q, want %q", repoID, repo.ID) + } + if baseBranch != "main" { + t.Fatalf("base branch = %q, want main", baseBranch) + } + + stored, err := harness.taskSvc.GetRepository(ctx, repo.ID) + if err != nil { + t.Fatalf("GetRepository: %v", err) + } + if stored.LocalPath != repoPath { + t.Fatalf("stored local_path = %q, want %q", stored.LocalPath, repoPath) + } + if stored.DefaultBranch != "main" { + t.Fatalf("stored default_branch = %q, want main", stored.DefaultBranch) + } +} + func TestGetSessionModel_Caching(t *testing.T) { // We can't easily instantiate a full taskservice.Service here without a DB, // so we test the caching mechanism directly on the adapter struct. diff --git a/apps/backend/internal/backendapp/orchestrator.go b/apps/backend/internal/backendapp/orchestrator.go index cb489b64eb..d12e3fe6d7 100644 --- a/apps/backend/internal/backendapp/orchestrator.go +++ b/apps/backend/internal/backendapp/orchestrator.go @@ -18,6 +18,7 @@ import ( agentsettingscontroller "github.com/kandev/kandev/internal/agent/settings/controller" settingsstore "github.com/kandev/kandev/internal/agent/settings/store" "github.com/kandev/kandev/internal/common/config" + "github.com/kandev/kandev/internal/common/gitref" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/db" "github.com/kandev/kandev/internal/events/bus" @@ -37,6 +38,11 @@ import ( workflowservice "github.com/kandev/kandev/internal/workflow/service" ) +const ( + defaultMainBranch = "main" + defaultMasterBranch = "master" +) + const defaultEventNamespace = "default" func provideOrchestrator( @@ -661,13 +667,7 @@ func (a *repositoryResolverAdapter) ResolveForReview( return "", "", fmt.Errorf("lookup repository by provider info: %w", err) } if existing != nil && existing.LocalPath != "" { - baseBranch := defaultBranch - if baseBranch == "" { - baseBranch = existing.DefaultBranch - } - if baseBranch == "" { - baseBranch = a.detectAndPersistDefaultBranch(ctx, existing, existing.LocalPath) - } + baseBranch := a.resolveReviewBaseBranch(ctx, existing, existing.LocalPath, defaultBranch) return existing.ID, baseBranch, nil } @@ -693,18 +693,31 @@ func (a *repositoryResolverAdapter) ResolveForReview( return "", "", fmt.Errorf("find/create repository: %w", err) } - baseBranch := defaultBranch - if baseBranch == "" { - baseBranch = repo.DefaultBranch - } - // When no default branch is known (e.g. issue watch with no PR context), - // detect it from the cloned repo's HEAD. - if baseBranch == "" && localPath != "" { - baseBranch = a.detectAndPersistDefaultBranch(ctx, repo, localPath) - } + baseBranch := a.resolveReviewBaseBranch(ctx, repo, localPath, defaultBranch) return repo.ID, baseBranch, nil } +func (a *repositoryResolverAdapter) resolveReviewBaseBranch( + ctx context.Context, + repo *taskmodels.Repository, + localPath string, + requestedBranch string, +) string { + if requestedBranch != "" { + return requestedBranch + } + stored := strings.TrimSpace(repo.DefaultBranch) + if stored == defaultMasterBranch && localPath != "" { + if detected := detectGitDefaultBranch(localPath); detected == defaultMainBranch { + return a.persistDetectedDefaultBranch(ctx, repo, detected) + } + } + if stored != "" { + return stored + } + return a.detectAndPersistDefaultBranch(ctx, repo, localPath) +} + // detectAndPersistDefaultBranch reads the default branch from the local clone // and persists it to the repository record for future lookups. func (a *repositoryResolverAdapter) detectAndPersistDefaultBranch( @@ -714,6 +727,17 @@ func (a *repositoryResolverAdapter) detectAndPersistDefaultBranch( if detected == "" { return "" } + return a.persistDetectedDefaultBranch(ctx, repo, detected) +} + +func (a *repositoryResolverAdapter) persistDetectedDefaultBranch( + ctx context.Context, + repo *taskmodels.Repository, + detected string, +) string { + if strings.TrimSpace(repo.DefaultBranch) == detected { + return detected + } if _, err := a.taskSvc.UpdateRepository(ctx, repo.ID, &taskservice.UpdateRepositoryRequest{ DefaultBranch: &detected, }); err != nil { @@ -726,26 +750,11 @@ func (a *repositoryResolverAdapter) detectAndPersistDefaultBranch( } // detectGitDefaultBranch reads the default branch of a git repository. -// It first checks refs/remotes/origin/HEAD (set by `git clone`), then -// falls back to .git/HEAD. Returns empty string on any failure. +// Returns empty string on any failure. func detectGitDefaultBranch(repoPath string) string { - // Prefer the remote default branch pointer set by `git clone`. - originHead := filepath.Join(repoPath, ".git", "refs", "remotes", "origin", "HEAD") - if content, err := os.ReadFile(originHead); err == nil { - trimmed := strings.TrimSpace(string(content)) - if after, ok := strings.CutPrefix(trimmed, "ref: refs/remotes/origin/"); ok { - return after - } - } - // Fall back to the local HEAD (works for fresh clones that lack origin/HEAD). - headPath := filepath.Join(repoPath, ".git", "HEAD") - content, err := os.ReadFile(headPath) + branch, err := gitref.DefaultBranchOrEmpty(repoPath) if err != nil { return "" } - trimmed := strings.TrimSpace(string(content)) - if after, ok := strings.CutPrefix(trimmed, "ref: refs/heads/"); ok { - return after - } - return "" + return branch } diff --git a/apps/backend/internal/common/gitref/gitref.go b/apps/backend/internal/common/gitref/gitref.go index dfa0fae0f9..d2cc17d491 100644 --- a/apps/backend/internal/common/gitref/gitref.go +++ b/apps/backend/internal/common/gitref/gitref.go @@ -22,10 +22,13 @@ const headFile = "HEAD" // to the wrong ref. // // Resolution order: -// 1. refs/remotes/origin/HEAD when set (the upstream's declared default) -// 2. The first ref that exists from -// {origin/main, origin/master, main, master} -// 3. The current HEAD as a last resort, so brand-new repos with only a +// 1. refs/remotes/origin/HEAD when set, except origin/HEAD=master yields to +// origin/main when it exists +// 2. origin/main +// 3. origin/master +// 4. local main +// 5. local master +// 6. The current HEAD as a last resort, so brand-new repos with only a // feature branch still produce a value — callers that care about // correctness can override. func DefaultBranch(repoPath string) (string, error) { @@ -38,15 +41,44 @@ func DefaultBranch(repoPath string) (string, error) { return "", err } commonDir := ResolveCommonGitDir(gitDir) + commonDir, err = guardRepoPath(commonDir) + if err != nil { + return "", err + } if branch := readOriginHEAD(commonDir); branch != "" { + if branch == "master" { + if refExists(commonDir, "refs/remotes/origin/main") { + return "main", nil + } + } return branch, nil } - if branch := pickFirstExistingBranch(commonDir); branch != "" { - return branch, nil + for _, candidate := range []struct { + ref string + branch string + }{ + {"refs/remotes/origin/main", "main"}, + {"refs/remotes/origin/master", "master"}, + {"refs/heads/main", "main"}, + {"refs/heads/master", "master"}, + } { + if refExists(commonDir, candidate.ref) { + return candidate.branch, nil + } } return readHEADBranchFallback(gitDir) } +// DefaultBranchOrEmpty returns DefaultBranch, but collapses detached HEAD's +// literal "HEAD" sentinel to empty for callers that persist branch names. +func DefaultBranchOrEmpty(repoPath string) (string, error) { + branch, err := DefaultBranch(repoPath) + if err != nil || branch == headFile { + return "", err + } + return branch, nil +} + // guardRepoPath rejects relative paths and any path containing `..` segments // before passing the value into the file-reading helpers below. Callers // already validate against an allowlist (see service.resolveAllowedLocalPath), @@ -128,8 +160,8 @@ func readOriginHEAD(commonDir string) string { // origin/HEAD only lives in packed-refs after a `git pack-refs --all`, // and the on-disk symref format there is gnarly to parse correctly. // We deliberately skip that case rather than maintain a broken parser: - // pickFirstExistingBranch's origin/main → origin/master → main → - // master fallbacks cover every realistic clone in practice. + // the named main/master fallbacks cover every realistic clone in + // practice. return "" } return parseSymbolicRefToBranch(strings.TrimSpace(string(content))) @@ -149,23 +181,13 @@ func parseSymbolicRefToBranch(line string) string { return "" } -func pickFirstExistingBranch(commonDir string) string { - for _, candidate := range []struct{ ref, branch string }{ - {"refs/remotes/origin/main", "main"}, - {"refs/remotes/origin/master", "master"}, - {"refs/heads/main", "main"}, - {"refs/heads/master", "master"}, - } { - if refExists(commonDir, candidate.ref) { - return candidate.branch - } - } - return "" -} - func refExists(commonDir, ref string) bool { - if _, err := os.Stat(filepath.Join(commonDir, ref)); err == nil { - return true + refPath, ok := safeRefPath(commonDir, ref) + if !ok { + return false + } + if info, err := os.Stat(refPath); err == nil { + return !info.IsDir() } content, err := os.ReadFile(filepath.Join(commonDir, "packed-refs")) if err != nil { @@ -187,6 +209,26 @@ func refExists(commonDir, ref string) bool { return false } +func safeRefPath(commonDir, ref string) (string, bool) { + if ref == "" || filepath.IsAbs(ref) { + return "", false + } + for _, part := range strings.FieldsFunc(ref, func(r rune) bool { + return r == '/' || r == filepath.Separator + }) { + if part == "" || part == "." || part == ".." { + return "", false + } + } + cleanCommon := filepath.Clean(commonDir) + candidate := filepath.Join(cleanCommon, filepath.Clean(ref)) + rel, err := filepath.Rel(cleanCommon, candidate) + if err != nil || rel == "." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || rel == ".." { + return "", false + } + return candidate, true +} + func readHEADBranchFallback(gitDir string) (string, error) { headPath := filepath.Join(gitDir, headFile) content, err := os.ReadFile(headPath) diff --git a/apps/backend/internal/common/gitref/gitref_test.go b/apps/backend/internal/common/gitref/gitref_test.go index 00e43e17c9..53869cd93f 100644 --- a/apps/backend/internal/common/gitref/gitref_test.go +++ b/apps/backend/internal/common/gitref/gitref_test.go @@ -65,3 +65,162 @@ func TestDefaultBranch_AcceptsAbsolutePathToValidRepo(t *testing.T) { t.Fatalf("expected main, got %q", branch) } } + +func TestDefaultBranch_PrefersMainWhenOriginHeadPointsAtMaster(t *testing.T) { + repoPath := t.TempDir() + gitDir := filepath.Join(repoPath, ".git") + if err := os.MkdirAll(filepath.Join(gitDir, "refs", "remotes", "origin"), 0o755); err != nil { + t.Fatalf("mkdir origin refs: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/feature/x\n"), 0o644); err != nil { + t.Fatalf("write HEAD: %v", err) + } + if err := os.WriteFile( + filepath.Join(gitDir, "refs", "remotes", "origin", "HEAD"), + []byte("ref: refs/remotes/origin/master\n"), + 0o644, + ); err != nil { + t.Fatalf("write origin HEAD: %v", err) + } + for _, branch := range []string{"main", "master"} { + refPath := filepath.Join(gitDir, "refs", "remotes", "origin", branch) + if err := os.WriteFile(refPath, []byte("0000000\n"), 0o644); err != nil { + t.Fatalf("write %s ref: %v", branch, err) + } + } + + branch, err := DefaultBranch(repoPath) + if err != nil { + t.Fatalf("DefaultBranch error: %v", err) + } + if branch != "main" { + t.Fatalf("DefaultBranch = %q, want main", branch) + } +} + +func TestDefaultBranch_PreservesNonMainOriginHeadWhenMainExists(t *testing.T) { + repoPath := t.TempDir() + gitDir := filepath.Join(repoPath, ".git") + if err := os.MkdirAll(filepath.Join(gitDir, "refs", "remotes", "origin"), 0o755); err != nil { + t.Fatalf("mkdir origin refs: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/feature/x\n"), 0o644); err != nil { + t.Fatalf("write HEAD: %v", err) + } + if err := os.WriteFile( + filepath.Join(gitDir, "refs", "remotes", "origin", "HEAD"), + []byte("ref: refs/remotes/origin/trunk\n"), + 0o644, + ); err != nil { + t.Fatalf("write origin HEAD: %v", err) + } + for _, branch := range []string{"main", "trunk"} { + refPath := filepath.Join(gitDir, "refs", "remotes", "origin", branch) + if err := os.WriteFile(refPath, []byte("0000000\n"), 0o644); err != nil { + t.Fatalf("write %s ref: %v", branch, err) + } + } + + branch, err := DefaultBranch(repoPath) + if err != nil { + t.Fatalf("DefaultBranch error: %v", err) + } + if branch != "trunk" { + t.Fatalf("DefaultBranch = %q, want trunk", branch) + } +} + +func TestDefaultBranch_LocalMainDoesNotOverrideOriginHeadMaster(t *testing.T) { + repoPath := t.TempDir() + gitDir := filepath.Join(repoPath, ".git") + if err := os.MkdirAll(filepath.Join(gitDir, "refs", "remotes", "origin"), 0o755); err != nil { + t.Fatalf("mkdir origin refs: %v", err) + } + if err := os.MkdirAll(filepath.Join(gitDir, "refs", "heads"), 0o755); err != nil { + t.Fatalf("mkdir local refs: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/feature/x\n"), 0o644); err != nil { + t.Fatalf("write HEAD: %v", err) + } + if err := os.WriteFile( + filepath.Join(gitDir, "refs", "remotes", "origin", "HEAD"), + []byte("ref: refs/remotes/origin/master\n"), + 0o644, + ); err != nil { + t.Fatalf("write origin HEAD: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "refs", "remotes", "origin", "master"), []byte("0000000\n"), 0o644); err != nil { + t.Fatalf("write origin master ref: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "refs", "heads", "main"), []byte("0000000\n"), 0o644); err != nil { + t.Fatalf("write local main ref: %v", err) + } + + branch, err := DefaultBranch(repoPath) + if err != nil { + t.Fatalf("DefaultBranch error: %v", err) + } + if branch != "master" { + t.Fatalf("DefaultBranch = %q, want master", branch) + } +} + +func TestDefaultBranch_RemoteMasterBeatsLocalMainWithoutOriginHead(t *testing.T) { + repoPath := t.TempDir() + gitDir := filepath.Join(repoPath, ".git") + if err := os.MkdirAll(filepath.Join(gitDir, "refs", "remotes", "origin"), 0o755); err != nil { + t.Fatalf("mkdir origin refs: %v", err) + } + if err := os.MkdirAll(filepath.Join(gitDir, "refs", "heads"), 0o755); err != nil { + t.Fatalf("mkdir local refs: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/feature/x\n"), 0o644); err != nil { + t.Fatalf("write HEAD: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "refs", "remotes", "origin", "master"), []byte("0000000\n"), 0o644); err != nil { + t.Fatalf("write origin master ref: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "refs", "heads", "main"), []byte("0000000\n"), 0o644); err != nil { + t.Fatalf("write local main ref: %v", err) + } + + branch, err := DefaultBranch(repoPath) + if err != nil { + t.Fatalf("DefaultBranch error: %v", err) + } + if branch != "master" { + t.Fatalf("DefaultBranch = %q, want master", branch) + } +} + +func TestDefaultBranch_MainNamespaceDoesNotOverrideOriginHeadMaster(t *testing.T) { + repoPath := t.TempDir() + gitDir := filepath.Join(repoPath, ".git") + if err := os.MkdirAll(filepath.Join(gitDir, "refs", "remotes", "origin", "main"), 0o755); err != nil { + t.Fatalf("mkdir origin main namespace: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/feature/x\n"), 0o644); err != nil { + t.Fatalf("write HEAD: %v", err) + } + if err := os.WriteFile( + filepath.Join(gitDir, "refs", "remotes", "origin", "HEAD"), + []byte("ref: refs/remotes/origin/master\n"), + 0o644, + ); err != nil { + t.Fatalf("write origin HEAD: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "refs", "remotes", "origin", "main", "foo"), []byte("0000000\n"), 0o644); err != nil { + t.Fatalf("write origin main/foo ref: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "refs", "remotes", "origin", "master"), []byte("0000000\n"), 0o644); err != nil { + t.Fatalf("write origin master ref: %v", err) + } + + branch, err := DefaultBranch(repoPath) + if err != nil { + t.Fatalf("DefaultBranch error: %v", err) + } + if branch != "master" { + t.Fatalf("DefaultBranch = %q, want master", branch) + } +} diff --git a/apps/backend/internal/orchestrator/executor/executor_resume.go b/apps/backend/internal/orchestrator/executor/executor_resume.go index f32e43beb9..1e37040b6f 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume.go @@ -199,7 +199,7 @@ func (e *Executor) backfillRepoDefaultBranch(ctx context.Context, repo *models.R if repo.DefaultBranch != "" || localPath == "" { return } - detected, err := gitref.DefaultBranch(localPath) + detected, err := gitref.DefaultBranchOrEmpty(localPath) if err != nil || detected == "" { e.logger.Debug("could not detect default branch from clone; leaving empty", zap.String("repository_id", repo.ID), diff --git a/apps/backend/internal/task/service/repository_discovery_test.go b/apps/backend/internal/task/service/repository_discovery_test.go index bdc047f382..0f3b20ab3a 100644 --- a/apps/backend/internal/task/service/repository_discovery_test.go +++ b/apps/backend/internal/task/service/repository_discovery_test.go @@ -147,11 +147,11 @@ func seedBareGitDir(t *testing.T, repoPath, headContent string) string { return gitDir } -// TestReadGitDefaultBranch_PrefersOriginHEAD — the upstream's declared -// default beats every other heuristic. Regression for the bug where the -// repo's stored default_branch latched onto whatever feature branch the user -// happened to be on at task-creation time. -func TestReadGitDefaultBranch_PrefersOriginHEAD(t *testing.T) { +// TestReadGitDefaultBranch_UsesOriginHEADWhenMainRefAbsent — origin/HEAD still +// beats the checked-out branch when no main ref exists. Regression for the bug +// where the repo's stored default_branch latched onto whatever feature branch +// the user happened to be on at task-creation time. +func TestReadGitDefaultBranch_UsesOriginHEADWhenMainRefAbsent(t *testing.T) { repoPath := t.TempDir() gitDir := seedBareGitDir(t, repoPath, "ref: refs/heads/feature/x\n") if err := os.MkdirAll(filepath.Join(gitDir, "refs", "remotes", "origin"), 0o755); err != nil { diff --git a/apps/backend/internal/task/service/service_tasks.go b/apps/backend/internal/task/service/service_tasks.go index a47dbff157..4978d717cf 100644 --- a/apps/backend/internal/task/service/service_tasks.go +++ b/apps/backend/internal/task/service/service_tasks.go @@ -473,7 +473,7 @@ func (s *Service) resolveRepoInputLocal( // and feeds into os.Stat/ReadFile inside gitref.DefaultBranch, so // without this guard a caller could traverse the filesystem. if safePath, pathErr := s.resolveAllowedLocalPath(repoInput.LocalPath); pathErr == nil { - if probed, err := gitref.DefaultBranch(safePath); err == nil && probed != "" && probed != "HEAD" { + if probed, err := gitref.DefaultBranchOrEmpty(safePath); err == nil && probed != "" { defaultBranch = probed } } diff --git a/apps/web/lib/utils.test.ts b/apps/web/lib/utils.test.ts index 009a9a7713..660a5a520f 100644 --- a/apps/web/lib/utils.test.ts +++ b/apps/web/lib/utils.test.ts @@ -55,7 +55,7 @@ describe("selectPreferredBranch", () => { expect(selectPreferredBranch(branches)).toBe("main"); }); - it("prefers local master over origin/main", () => { + it("keeps local master ahead of origin/main", () => { const branches = [ { name: "master", type: "local" }, { name: "main", type: "remote", remote: "origin" }, @@ -63,6 +63,15 @@ describe("selectPreferredBranch", () => { expect(selectPreferredBranch(branches)).toBe("master"); }); + it("keeps local master ahead of remote conventional branches", () => { + const branches = [ + { name: "master", type: "local" }, + { name: "master", type: "remote", remote: "origin" }, + { name: "main", type: "remote", remote: "origin" }, + ]; + expect(selectPreferredBranch(branches)).toBe("master"); + }); + it("falls back to origin/main when no local main/master", () => { const branches = [ { name: "main", type: "remote", remote: "origin" },