diff --git a/apps/backend/internal/common/gitref/gitref.go b/apps/backend/internal/common/gitref/gitref.go new file mode 100644 index 0000000000..dfa0fae0f9 --- /dev/null +++ b/apps/backend/internal/common/gitref/gitref.go @@ -0,0 +1,207 @@ +// Package gitref reads git ref state directly from the on-disk .git +// directory, without invoking the git binary. The functions here are kept +// minimal and stdlib-only so they can be imported from both the task service +// and the sqlite migration layer (where adding a service dependency would +// invert the existing layering). +package gitref + +import ( + "fmt" + "os" + "path/filepath" + "strings" +) + +const headFile = "HEAD" + +// DefaultBranch returns the repository's *integration* branch — the branch +// that work is meant to merge back into. It is intentionally NOT the current +// HEAD: a developer who runs the dialog while checked out on a feature +// branch must still get "main" (or "master") back, otherwise downstream +// consumers (changes-panel merge-base, executor BaseBranch fallback) anchor +// 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 +// feature branch still produce a value — callers that care about +// correctness can override. +func DefaultBranch(repoPath string) (string, error) { + safe, err := guardRepoPath(repoPath) + if err != nil { + return "", err + } + gitDir, err := ResolveGitDir(safe) + if err != nil { + return "", err + } + commonDir := ResolveCommonGitDir(gitDir) + if branch := readOriginHEAD(commonDir); branch != "" { + return branch, nil + } + if branch := pickFirstExistingBranch(commonDir); branch != "" { + return branch, nil + } + return readHEADBranchFallback(gitDir) +} + +// 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), +// but CodeQL's go/path-injection taint analysis doesn't trace that as a +// sanitizer, so we re-check here at the I/O boundary. The check is also +// genuinely useful: any caller — migration code, tests, future callers — +// must hand us an absolute, traversal-free path or we refuse to read. +func guardRepoPath(repoPath string) (string, error) { + if repoPath == "" { + return "", fmt.Errorf("repository path is required") + } + if !filepath.IsAbs(repoPath) { + return "", fmt.Errorf("repository path must be absolute") + } + // Check the RAW input for '..' segments before cleaning. filepath.Clean + // resolves traversal away from absolute paths (`/allowed/../etc` → + // `/etc`), so iterating the cleaned path would silently pass attempted + // escapes. Splitting on both separators handles cross-platform inputs + // without depending on the OS-native separator. + for _, part := range strings.FieldsFunc(repoPath, func(r rune) bool { + return r == '/' || r == filepath.Separator + }) { + if part == ".." { + return "", fmt.Errorf("repository path must not contain '..' segments") + } + } + return filepath.Clean(repoPath), nil +} + +// ResolveGitDir returns the actual git directory for repoPath, following +// `.git` files (worktree pointers) when present. +func ResolveGitDir(repoPath string) (string, error) { + gitPath := filepath.Join(repoPath, ".git") + info, err := os.Stat(gitPath) + if err != nil { + return "", err + } + if info.IsDir() { + return gitPath, nil + } + content, err := os.ReadFile(gitPath) + if err != nil { + return "", err + } + line := strings.TrimSpace(string(content)) + if !strings.HasPrefix(line, "gitdir:") { + return "", fmt.Errorf("invalid gitdir reference") + } + gitDir := strings.TrimSpace(strings.TrimPrefix(line, "gitdir:")) + if filepath.IsAbs(gitDir) { + return gitDir, nil + } + return filepath.Clean(filepath.Join(repoPath, gitDir)), nil +} + +// ResolveCommonGitDir returns the shared git dir for a worktree, or gitDir +// itself for a regular repo. Refs (refs/heads/*, refs/remotes/*, packed-refs) +// live under the common dir, not the worktree's gitDir. +func ResolveCommonGitDir(gitDir string) string { + commonFile := filepath.Join(gitDir, "commondir") + content, err := os.ReadFile(commonFile) + if err != nil { + return gitDir + } + commonDir := strings.TrimSpace(string(content)) + if commonDir == "" { + return gitDir + } + if filepath.IsAbs(commonDir) { + return filepath.Clean(commonDir) + } + return filepath.Clean(filepath.Join(gitDir, commonDir)) +} + +func readOriginHEAD(commonDir string) string { + headPath := filepath.Join(commonDir, "refs", "remotes", "origin", headFile) + content, err := os.ReadFile(headPath) + if err != nil { + // 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. + return "" + } + return parseSymbolicRefToBranch(strings.TrimSpace(string(content))) +} + +func parseSymbolicRefToBranch(line string) string { + ref, ok := strings.CutPrefix(line, "ref: ") + if !ok { + ref = line + } + switch { + case strings.HasPrefix(ref, "refs/remotes/origin/"): + return strings.TrimPrefix(ref, "refs/remotes/origin/") + case strings.HasPrefix(ref, "refs/heads/"): + return strings.TrimPrefix(ref, "refs/heads/") + } + 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 + } + content, err := os.ReadFile(filepath.Join(commonDir, "packed-refs")) + if err != nil { + return false + } + for _, line := range strings.Split(string(content), "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") || strings.HasPrefix(line, "^") { + continue + } + _, after, ok := strings.Cut(line, " ") + if !ok { + continue + } + if after == ref { + return true + } + } + return false +} + +func readHEADBranchFallback(gitDir string) (string, error) { + headPath := filepath.Join(gitDir, headFile) + content, err := os.ReadFile(headPath) + if err != nil { + return "", err + } + trimmed := strings.TrimSpace(string(content)) + if ref, ok := strings.CutPrefix(trimmed, "ref: "); ok { + // Strip refs/heads/ as a prefix rather than splitting on "/" — branch + // names legally contain slashes (e.g. "feature/my-feature"), so taking + // the last path component would silently corrupt every nested branch. + return strings.TrimPrefix(ref, "refs/heads/"), nil + } + if trimmed != "" { + return headFile, nil + } + return "", fmt.Errorf("unable to determine branch") +} diff --git a/apps/backend/internal/common/gitref/gitref_test.go b/apps/backend/internal/common/gitref/gitref_test.go new file mode 100644 index 0000000000..00e43e17c9 --- /dev/null +++ b/apps/backend/internal/common/gitref/gitref_test.go @@ -0,0 +1,67 @@ +package gitref + +import ( + "os" + "path/filepath" + "testing" +) + +// TestDefaultBranch_RejectsRelativePath — guardRepoPath refuses relative +// paths so callers can't sneak `repos/..` past the I/O boundary even if +// the upstream allowlist check is bypassed. +func TestDefaultBranch_RejectsRelativePath(t *testing.T) { + if _, err := DefaultBranch("repos/foo"); err == nil { + t.Fatal("expected error for relative path") + } +} + +// TestDefaultBranch_RejectsTraversal — explicit guard against ".." +// segments in the RAW input. We must check before filepath.Clean: Clean +// resolves '..' away from absolute paths (`/allowed/../etc` → `/etc`) and +// would silently let a traversal attempt through. +func TestDefaultBranch_RejectsTraversal(t *testing.T) { + cases := []string{ + "/tmp/../etc", + "/foo/bar/../../etc", + "/a/b/..", + } + for _, p := range cases { + if _, err := DefaultBranch(p); err == nil { + t.Errorf("expected error for path %q (contains '..')", p) + } + } +} + +// TestDefaultBranch_RejectsEmpty — guardRepoPath errors on empty input +// rather than letting filepath.Join silently turn it into ".git". +func TestDefaultBranch_RejectsEmpty(t *testing.T) { + if _, err := DefaultBranch(""); err == nil { + t.Fatal("expected error for empty path") + } +} + +// TestDefaultBranch_AcceptsAbsolutePathToValidRepo — sanity check that +// the guard doesn't reject legitimate absolute paths. We seed a minimal +// .git directory with a HEAD on main and assert the probe finds it. +func TestDefaultBranch_AcceptsAbsolutePathToValidRepo(t *testing.T) { + repoPath := t.TempDir() + if !filepath.IsAbs(repoPath) { + t.Fatalf("t.TempDir() returned non-absolute path: %q", repoPath) + } + gitDir := filepath.Join(repoPath, ".git") + if err := os.MkdirAll(gitDir, 0o755); err != nil { + t.Fatalf("mkdir git: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/main\n"), 0o644); err != nil { + t.Fatalf("write HEAD: %v", err) + } + branch, err := DefaultBranch(repoPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // Git branch names are case-sensitive — assert exact equality, not + // EqualFold. A function that returns "Main" or "MAIN" should fail. + if branch != "main" { + t.Fatalf("expected main, got %q", branch) + } +} diff --git a/apps/backend/internal/task/dto/dto.go b/apps/backend/internal/task/dto/dto.go index e201c45af5..d29ca9d02d 100644 --- a/apps/backend/internal/task/dto/dto.go +++ b/apps/backend/internal/task/dto/dto.go @@ -139,14 +139,15 @@ type TaskDTO struct { } type TaskRepositoryDTO struct { - ID string `json:"id"` - TaskID string `json:"task_id"` - RepositoryID string `json:"repository_id"` - BaseBranch string `json:"base_branch"` - Position int `json:"position"` - Metadata map[string]interface{} `json:"metadata,omitempty"` - CreatedAt time.Time `json:"created_at"` - UpdatedAt time.Time `json:"updated_at"` + ID string `json:"id"` + TaskID string `json:"task_id"` + RepositoryID string `json:"repository_id"` + BaseBranch string `json:"base_branch"` + CheckoutBranch string `json:"checkout_branch,omitempty"` + Position int `json:"position"` + Metadata map[string]interface{} `json:"metadata,omitempty"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` } type TaskSessionDTO struct { @@ -519,14 +520,15 @@ func FromTaskWithSessionInfo( var repositories []TaskRepositoryDTO for _, repo := range task.Repositories { repositories = append(repositories, TaskRepositoryDTO{ - ID: repo.ID, - TaskID: repo.TaskID, - RepositoryID: repo.RepositoryID, - BaseBranch: repo.BaseBranch, - Position: repo.Position, - Metadata: repo.Metadata, - CreatedAt: repo.CreatedAt, - UpdatedAt: repo.UpdatedAt, + ID: repo.ID, + TaskID: repo.TaskID, + RepositoryID: repo.RepositoryID, + BaseBranch: repo.BaseBranch, + CheckoutBranch: repo.CheckoutBranch, + Position: repo.Position, + Metadata: repo.Metadata, + CreatedAt: repo.CreatedAt, + UpdatedAt: repo.UpdatedAt, }) } diff --git a/apps/backend/internal/task/handlers/task_ws_handlers.go b/apps/backend/internal/task/handlers/task_ws_handlers.go index b87b02ee42..97ee65ca53 100644 --- a/apps/backend/internal/task/handlers/task_ws_handlers.go +++ b/apps/backend/internal/task/handlers/task_ws_handlers.go @@ -119,12 +119,13 @@ func (h *TaskHandlers) wsCreateTask(ctx context.Context, msg *ws.Message) (*ws.M return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "repository_id, local_path, or github_url is required", nil) } repos = append(repos, dto.TaskRepositoryInput{ - RepositoryID: r.RepositoryID, - BaseBranch: r.BaseBranch, - LocalPath: r.LocalPath, - Name: r.Name, - DefaultBranch: r.DefaultBranch, - GitHubURL: r.GitHubURL, + RepositoryID: r.RepositoryID, + BaseBranch: r.BaseBranch, + CheckoutBranch: r.CheckoutBranch, + LocalPath: r.LocalPath, + Name: r.Name, + DefaultBranch: r.DefaultBranch, + GitHubURL: r.GitHubURL, }) } @@ -248,12 +249,13 @@ func (h *TaskHandlers) wsUpdateTask(ctx context.Context, msg *ws.Message) (*ws.M if req.Repositories != nil { for _, r := range req.Repositories { repos = append(repos, dto.TaskRepositoryInput{ - RepositoryID: r.RepositoryID, - BaseBranch: r.BaseBranch, - LocalPath: r.LocalPath, - Name: r.Name, - DefaultBranch: r.DefaultBranch, - GitHubURL: r.GitHubURL, + RepositoryID: r.RepositoryID, + BaseBranch: r.BaseBranch, + CheckoutBranch: r.CheckoutBranch, + LocalPath: r.LocalPath, + Name: r.Name, + DefaultBranch: r.DefaultBranch, + GitHubURL: r.GitHubURL, }) } } diff --git a/apps/backend/internal/task/service/repository_discovery.go b/apps/backend/internal/task/service/repository_discovery.go index cfbafab43c..4a5e8b526e 100644 --- a/apps/backend/internal/task/service/repository_discovery.go +++ b/apps/backend/internal/task/service/repository_discovery.go @@ -10,6 +10,8 @@ import ( "path/filepath" "sort" "strings" + + "github.com/kandev/kandev/internal/common/gitref" ) type RepositoryDiscoveryConfig struct { @@ -555,28 +557,11 @@ func readGitDirtyFiles(ctx context.Context, repoPath string) ([]string, error) { return paths, nil } +// readGitDefaultBranch is a thin wrapper around gitref.DefaultBranch so this +// file's existing callers (ValidateLocalRepositoryPath, repoWalker.makeRepo, +// the test suite) keep their existing API. func readGitDefaultBranch(repoPath string) (string, error) { - gitDir, err := resolveGitDir(repoPath) - if err != nil { - return "", err - } - headPath := filepath.Join(gitDir, gitHEAD) - content, err := os.ReadFile(headPath) - if err != nil { - return "", err - } - trimmed := strings.TrimSpace(string(content)) - if strings.HasPrefix(trimmed, "ref: ") { - ref := strings.TrimPrefix(trimmed, "ref: ") - parts := strings.Split(ref, "/") - if len(parts) > 0 { - return parts[len(parts)-1], nil - } - } - if trimmed != "" { - return gitHEAD, nil - } - return "", fmt.Errorf("unable to determine branch") + return gitref.DefaultBranch(repoPath) } func resolveGitDir(repoPath string) (string, error) { diff --git a/apps/backend/internal/task/service/repository_discovery_test.go b/apps/backend/internal/task/service/repository_discovery_test.go index 8750c7c854..5bd13c8869 100644 --- a/apps/backend/internal/task/service/repository_discovery_test.go +++ b/apps/backend/internal/task/service/repository_discovery_test.go @@ -132,17 +132,84 @@ func TestNormalizeRootsDedupesAndCleans(t *testing.T) { } } -func TestReadGitDefaultBranch(t *testing.T) { - repoPath := t.TempDir() +// seedBareGitDir creates a minimal .git directory at repoPath with HEAD set +// to the given branch (or commit SHA for detached). Returns the gitDir path. +func seedBareGitDir(t *testing.T, repoPath, headContent string) string { + t.Helper() gitDir := filepath.Join(repoPath, ".git") if err := os.MkdirAll(gitDir, 0o755); err != nil { t.Fatalf("mkdir git: %v", err) } - - headPath := filepath.Join(gitDir, "HEAD") - if err := os.WriteFile(headPath, []byte("ref: refs/heads/develop\n"), 0o644); err != nil { + if err := os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte(headContent), 0o644); err != nil { t.Fatalf("write HEAD: %v", err) } + 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) { + 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 { + t.Fatalf("mkdir origin: %v", err) + } + if err := os.WriteFile( + filepath.Join(gitDir, "refs", "remotes", "origin", "HEAD"), + []byte("ref: refs/remotes/origin/main\n"), + 0o644, + ); err != nil { + t.Fatalf("write origin/HEAD: %v", err) + } + branch, err := readGitDefaultBranch(repoPath) + if err != nil { + t.Fatalf("readGitDefaultBranch error: %v", err) + } + if branch != "main" { + t.Fatalf("expected main, got %q", branch) + } +} + +// TestReadGitDefaultBranch_FallsBackToRemoteCandidates — when origin/HEAD +// isn't set (older clones, never run `git remote set-head`), the conventional +// origin/main / origin/master refs win over local branches. +func TestReadGitDefaultBranch_FallsBackToRemoteCandidates(t *testing.T) { + repoPath := t.TempDir() + gitDir := seedBareGitDir(t, repoPath, "ref: refs/heads/feature/y\n") + writeRef(t, filepath.Join(gitDir, "refs", "remotes", "origin", "main")) + branch, err := readGitDefaultBranch(repoPath) + if err != nil { + t.Fatalf("readGitDefaultBranch error: %v", err) + } + if branch != "main" { + t.Fatalf("expected main, got %q", branch) + } +} + +// TestReadGitDefaultBranch_FallsBackToLocalCandidates — local-only repos +// (no remotes) still produce the conventional integration branch when one +// exists locally, even if the user is checked out on a feature branch. +func TestReadGitDefaultBranch_FallsBackToLocalCandidates(t *testing.T) { + repoPath := t.TempDir() + gitDir := seedBareGitDir(t, repoPath, "ref: refs/heads/feature/z\n") + writeRef(t, filepath.Join(gitDir, "refs", "heads", "main")) + branch, err := readGitDefaultBranch(repoPath) + if err != nil { + t.Fatalf("readGitDefaultBranch error: %v", err) + } + if branch != "main" { + t.Fatalf("expected main, got %q", branch) + } +} + +// TestReadGitDefaultBranch_FallsBackToHEAD — a brand-new repo with only +// a feature branch checked out and no main/master falls back to HEAD so +// callers still get *some* answer instead of an error. +func TestReadGitDefaultBranch_FallsBackToHEAD(t *testing.T) { + repoPath := t.TempDir() + seedBareGitDir(t, repoPath, "ref: refs/heads/develop\n") branch, err := readGitDefaultBranch(repoPath) if err != nil { t.Fatalf("readGitDefaultBranch error: %v", err) @@ -150,21 +217,47 @@ func TestReadGitDefaultBranch(t *testing.T) { if branch != "develop" { t.Fatalf("expected develop, got %q", branch) } +} - if err := os.WriteFile(headPath, []byte("3a3f2d3b\n"), 0o644); err != nil { - t.Fatalf("write HEAD detached: %v", err) - } - branch, err = readGitDefaultBranch(repoPath) +// TestReadGitDefaultBranch_FallsBackToHEAD_PreservesSlashes — branch names +// can contain slashes (e.g. "feature/my-feature"). The HEAD fallback must +// strip the refs/heads/ prefix verbatim rather than split on "/" and take +// the tail, otherwise nested branches get silently corrupted into their +// last path component, which then becomes the stored default_branch and +// poisons every downstream merge-base lookup. +func TestReadGitDefaultBranch_FallsBackToHEAD_PreservesSlashes(t *testing.T) { + repoPath := t.TempDir() + seedBareGitDir(t, repoPath, "ref: refs/heads/feature/my-feature\n") + branch, err := readGitDefaultBranch(repoPath) if err != nil { - t.Fatalf("readGitDefaultBranch detached error: %v", err) + t.Fatalf("readGitDefaultBranch error: %v", err) + } + if branch != "feature/my-feature" { + t.Fatalf("expected feature/my-feature, got %q", branch) + } +} + +// TestReadGitDefaultBranch_DetachedHEADReturnsHEADLiteral — detached HEAD on +// a repo with no integration branches yields the literal "HEAD" sentinel +// (preserved from the previous behavior so callers depending on it don't +// silently start failing). +func TestReadGitDefaultBranch_DetachedHEADReturnsHEADLiteral(t *testing.T) { + repoPath := t.TempDir() + seedBareGitDir(t, repoPath, "3a3f2d3b\n") + branch, err := readGitDefaultBranch(repoPath) + if err != nil { + t.Fatalf("readGitDefaultBranch error: %v", err) } if branch != "HEAD" { t.Fatalf("expected HEAD, got %q", branch) } +} - if err := os.WriteFile(headPath, []byte("\n"), 0o644); err != nil { - t.Fatalf("write HEAD empty: %v", err) - } +// TestReadGitDefaultBranch_ErrorsOnEmptyHEAD — preserves the existing +// contract: HEAD with no content is an unparseable repo. +func TestReadGitDefaultBranch_ErrorsOnEmptyHEAD(t *testing.T) { + repoPath := t.TempDir() + seedBareGitDir(t, repoPath, "\n") if _, err := readGitDefaultBranch(repoPath); err == nil { t.Fatalf("expected error for empty HEAD") } diff --git a/apps/backend/internal/task/service/service_tasks.go b/apps/backend/internal/task/service/service_tasks.go index 66228cea40..9618db3de0 100644 --- a/apps/backend/internal/task/service/service_tasks.go +++ b/apps/backend/internal/task/service/service_tasks.go @@ -13,6 +13,7 @@ import ( v1 "github.com/kandev/kandev/pkg/api/v1" "go.uber.org/zap" + "github.com/kandev/kandev/internal/common/gitref" "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/worktree" @@ -198,9 +199,25 @@ func (s *Service) resolveRepoInput(ctx context.Context, workspaceID string, repo if name == "" { name = filepath.Base(repoInput.LocalPath) } + // Resolve default_branch by probing the repo on disk so it's anchored + // to the integration branch (origin/HEAD or main/master) rather than + // whatever feature branch the user happens to have checked out. The + // frontend's `default_branch` hint wins when set; otherwise we probe + // directly. Falling back to repoInput.BaseBranch is wrong because in + // the local-executor flow that field carries the user's working + // branch, which would permanently pin repositories.default_branch to + // a feature branch and break every downstream merge-base lookup. defaultBranch := repoInput.DefaultBranch if defaultBranch == "" { - defaultBranch = repoInput.BaseBranch + // Probe must operate on a path validated against the discovery + // allowlist — repoInput.LocalPath comes straight from the HTTP body + // 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" { + defaultBranch = probed + } + } } created, createErr := s.CreateRepository(ctx, &CreateRepositoryRequest{ WorkspaceID: workspaceID, diff --git a/apps/web/components/task-create-dialog-footer.tsx b/apps/web/components/task-create-dialog-footer.tsx index 2b78f929f8..2412dd6981 100644 --- a/apps/web/components/task-create-dialog-footer.tsx +++ b/apps/web/components/task-create-dialog-footer.tsx @@ -108,6 +108,7 @@ function StartTaskSplitButton({ {isEditMode ? "Update task" : "Create without starting agent"} diff --git a/apps/web/components/task-create-dialog-helpers.multi-repo.test.ts b/apps/web/components/task-create-dialog-helpers.multi-repo.test.ts index 95321d64fe..aee02224a6 100644 --- a/apps/web/components/task-create-dialog-helpers.multi-repo.test.ts +++ b/apps/web/components/task-create-dialog-helpers.multi-repo.test.ts @@ -17,9 +17,9 @@ describe("buildRepositoriesPayload — unified rows", () => { discoveredRepositories: [], }); expect(payload).toEqual([ - { repository_id: "repo-front", base_branch: "main" }, - { repository_id: "repo-back", base_branch: "develop" }, - { repository_id: "repo-shared", base_branch: undefined }, + { repository_id: "repo-front", base_branch: "main", checkout_branch: undefined }, + { repository_id: "repo-back", base_branch: "develop", checkout_branch: undefined }, + { repository_id: "repo-shared", base_branch: undefined, checkout_branch: undefined }, ]); }); @@ -42,13 +42,156 @@ describe("buildRepositoriesPayload — unified rows", () => { { repository_id: "", base_branch: "trunk", + checkout_branch: undefined, local_path: "/home/me/projects/local-project", default_branch: "trunk", }, - { repository_id: "repo-back", base_branch: "main" }, + { repository_id: "repo-back", base_branch: "main", checkout_branch: undefined }, + ]); + }); +}); + +// Regression for the "new branch on local executor" bug: the chip's branch +// is the working branch on disk (e.g. "feature/x"), not the integration +// branch. We must send it as `checkout_branch`, with `base_branch` anchored +// to the repo's `default_branch`. Without this, agentctl recomputes +// merge-base(HEAD, origin/feature/x) which collapses to HEAD and the +// changes panel is empty after refresh. +describe("buildRepositoriesPayload — local executor branch split (core)", () => { + it("rowBranch != default_branch → swap into checkout_branch", () => { + const payload = buildRepositoriesPayload({ + useGitHubUrl: false, + githubUrl: "", + githubBranch: "", + githubPrHeadBranch: null, + repositories: [{ key: "r0", repositoryId: "repo-1", branch: "feature/x" }], + discoveredRepositories: [], + // eslint-disable-next-line @typescript-eslint/no-explicit-any + workspaceRepositories: [{ id: "repo-1", default_branch: "main" }] as any, + isLocalExecutor: true, + }); + expect(payload).toEqual([ + { repository_id: "repo-1", base_branch: "main", checkout_branch: "feature/x" }, ]); }); + it("rowBranch matches default_branch → no checkout_branch", () => { + const payload = buildRepositoriesPayload({ + useGitHubUrl: false, + githubUrl: "", + githubBranch: "", + githubPrHeadBranch: null, + repositories: [{ key: "r0", repositoryId: "repo-1", branch: "main" }], + discoveredRepositories: [], + // eslint-disable-next-line @typescript-eslint/no-explicit-any + workspaceRepositories: [{ id: "repo-1", default_branch: "main" }] as any, + isLocalExecutor: true, + }); + expect(payload).toEqual([ + { repository_id: "repo-1", base_branch: "main", checkout_branch: undefined }, + ]); + }); + + it("localPath row uses discoveredRepositories.default_branch", () => { + const payload = buildRepositoriesPayload({ + useGitHubUrl: false, + githubUrl: "", + githubBranch: "", + githubPrHeadBranch: null, + repositories: [{ key: "r0", localPath: "/p/r", branch: "feature/y" }], + discoveredRepositories: [ + // eslint-disable-next-line @typescript-eslint/no-explicit-any + { path: "/p/r", default_branch: "main" } as any, + ], + isLocalExecutor: true, + }); + expect(payload).toEqual([ + { + repository_id: "", + base_branch: "main", + checkout_branch: "feature/y", + local_path: "/p/r", + default_branch: "main", + }, + ]); + }); +}); + +describe("buildRepositoriesPayload — local executor branch split (edge cases)", () => { + it("fresh-branch flow: skips the split so the picked base is preserved as base_branch", () => { + // When the user enables "Fork a new branch", the chip's branch is the + // BASE TO FORK FROM (e.g. "develop"), not a working branch. The backend + // creates a new branch from that base and rewrites base_branch to the + // new branch name. If we split here, base_branch would land on the + // repo's default ("main") and the fork would happen from main instead + // of develop — silently wrong. + const payload = buildRepositoriesPayload({ + useGitHubUrl: false, + githubUrl: "", + githubBranch: "", + githubPrHeadBranch: null, + repositories: [{ key: "r0", repositoryId: "repo-1", branch: "develop" }], + discoveredRepositories: [], + // eslint-disable-next-line @typescript-eslint/no-explicit-any + workspaceRepositories: [{ id: "repo-1", default_branch: "main" }] as any, + isLocalExecutor: true, + freshBranch: { confirmDiscard: false, consentedDirtyFiles: [] }, + }); + expect(payload).toEqual([ + { + repository_id: "repo-1", + base_branch: "develop", + checkout_branch: undefined, + fresh_branch: true, + confirm_discard: false, + consented_dirty_files: [], + }, + ]); + }); + + it("falls through when default_branch is unknown (legacy repos)", () => { + // Repos created before the backend probe fix may have an unset + // default_branch in the workspace store. If we synthesize base_branch= + // rowBranch here (as the original draft did), we reproduce the very bug + // this PR fixes: agentctl recomputes merge-base(HEAD, origin/) + // → collapses to HEAD → empty changes panel. Better to leave the legacy + // shape alone — the next backend createRepository call will populate + // default_branch via the gitref probe. + const payload = buildRepositoriesPayload({ + useGitHubUrl: false, + githubUrl: "", + githubBranch: "", + githubPrHeadBranch: null, + repositories: [{ key: "r0", repositoryId: "repo-1", branch: "feature/x" }], + discoveredRepositories: [], + // eslint-disable-next-line @typescript-eslint/no-explicit-any + workspaceRepositories: [{ id: "repo-1", default_branch: "" }] as any, + isLocalExecutor: true, + }); + expect(payload).toEqual([ + { repository_id: "repo-1", base_branch: "feature/x", checkout_branch: undefined }, + ]); + }); + + it("non-local executor leaves rowBranch as base_branch (worktree flow unchanged)", () => { + const payload = buildRepositoriesPayload({ + useGitHubUrl: false, + githubUrl: "", + githubBranch: "", + githubPrHeadBranch: null, + repositories: [{ key: "r0", repositoryId: "repo-1", branch: "main" }], + discoveredRepositories: [], + // eslint-disable-next-line @typescript-eslint/no-explicit-any + workspaceRepositories: [{ id: "repo-1", default_branch: "main" }] as any, + isLocalExecutor: false, + }); + expect(payload).toEqual([ + { repository_id: "repo-1", base_branch: "main", checkout_branch: undefined }, + ]); + }); +}); + +describe("buildRepositoriesPayload — single-row and URL mode", () => { it("URL mode produces a single github_url entry and ignores the rows", () => { const payload = buildRepositoriesPayload({ useGitHubUrl: true, @@ -77,6 +220,8 @@ describe("buildRepositoriesPayload — unified rows", () => { repositories: [{ key: "r0", repositoryId: "repo-only", branch: "main" }], discoveredRepositories: [], }); - expect(payload).toEqual([{ repository_id: "repo-only", base_branch: "main" }]); + expect(payload).toEqual([ + { repository_id: "repo-only", base_branch: "main", checkout_branch: undefined }, + ]); }); }); diff --git a/apps/web/components/task-create-dialog-helpers.ts b/apps/web/components/task-create-dialog-helpers.ts index 51cf63fd97..6cde8bd3e4 100644 --- a/apps/web/components/task-create-dialog-helpers.ts +++ b/apps/web/components/task-create-dialog-helpers.ts @@ -1,5 +1,5 @@ import type { useRouter } from "next/navigation"; -import type { Task, Branch, LocalRepository } from "@/lib/types/http"; +import type { Task, Branch, LocalRepository, Repository } from "@/lib/types/http"; import type { AgentProfileOption } from "@/lib/state/slices"; import type { AppState } from "@/lib/state/store"; import type { StepType, TaskRepoRow } from "@/components/task-create-dialog-types"; @@ -172,6 +172,18 @@ export function buildRepositoriesPayload(opts: { repositories: TaskRepoRow[]; /** Used to look up `default_branch` for `localPath` rows. */ discoveredRepositories: LocalRepository[]; + /** Workspace repositories — used to look up `default_branch` for `repositoryId` rows. */ + workspaceRepositories?: Repository[]; + /** + * For the local executor (no worktree), the chip's branch field represents + * the working branch on disk, not the parent integration branch. Send it as + * `checkout_branch` so the session's `base_branch` stays anchored to the + * repo's `default_branch` (which is what git log / cumulative diff use as + * the merge-base reference). Without this, "new branch on local executor" + * collapses the merge-base recomputation to HEAD and the changes panel + * goes empty after a refresh. + */ + isLocalExecutor?: boolean; /** * Optional fresh-branch metadata. The UI gates this to single-row + local * executor; when present we apply it to every row (which is at most one). @@ -195,23 +207,81 @@ export function buildRepositoriesPayload(opts: { consented_dirty_files: opts.freshBranch.consentedDirtyFiles, } : {}; + // Fresh-branch flow inverts the chip's semantics: instead of "the working + // branch I'm on", row.branch becomes "the base I want to fork from". The + // backend then creates a new branch and rewrites repos[i].BaseBranch to it. + // Splitting here would force `base_branch=default_branch, checkout_branch= + // ` and the backend would fork from the wrong base. Skip the + // split entirely when fresh-branch is active. + const isLocalExecutor = !!opts.isLocalExecutor && !opts.freshBranch; return opts.repositories .filter((row) => row.repositoryId || row.localPath) .map((row) => { + const defaultBranch = resolveRowDefaultBranch(row, opts); + const branches = splitLocalExecutorBranches({ + rowBranch: row.branch, + defaultBranch, + isLocalExecutor, + }); if (row.repositoryId) { return { repository_id: row.repositoryId, - base_branch: row.branch || undefined, + base_branch: branches.base_branch, + checkout_branch: branches.checkout_branch, ...fresh, }; } - const local = opts.discoveredRepositories.find((d) => d.path === row.localPath); return { repository_id: "", - base_branch: row.branch || undefined, + base_branch: branches.base_branch, + checkout_branch: branches.checkout_branch, local_path: row.localPath, - default_branch: local?.default_branch || undefined, + default_branch: defaultBranch || undefined, ...fresh, }; }); } + +function resolveRowDefaultBranch( + row: TaskRepoRow, + opts: { + discoveredRepositories: LocalRepository[]; + workspaceRepositories?: Repository[]; + }, +): string | undefined { + if (row.repositoryId) { + return opts.workspaceRepositories?.find((r) => r.id === row.repositoryId)?.default_branch; + } + return opts.discoveredRepositories.find((d) => d.path === row.localPath)?.default_branch; +} + +/** + * For the local executor: return `base_branch=defaultBranch`, + * `checkout_branch=rowBranch` so the session anchors merge-base to the repo's + * integration branch while the preparer still checks out the user's working + * branch. When `rowBranch === defaultBranch` we omit checkout_branch — the + * preparer treats matching values as "use current state" and skips git ops. + * + * For non-local executors (worktree-based): keep the historical shape where + * `base_branch=rowBranch` (the worktree creates a new branch off of it). + */ +function splitLocalExecutorBranches(args: { + rowBranch?: string; + defaultBranch?: string; + isLocalExecutor: boolean; +}): { base_branch: string | undefined; checkout_branch: string | undefined } { + // Without a known default_branch we can't anchor base_branch to the + // integration ref — and using rowBranch as a stand-in reproduces the + // exact bug this PR fixes (changes panel collapses to HEAD on refresh). + // Fall through to the legacy non-split shape: a workspace repo with an + // unset default_branch is no worse off than before, and the backend's + // resolveRepoInput probe will populate it on the next CreateRepository + // call. Wait for that probe rather than synthesizing a guess here. + if (!args.isLocalExecutor || !args.defaultBranch) { + return { base_branch: args.rowBranch || undefined, checkout_branch: undefined }; + } + const base = args.defaultBranch; + const checkout = + args.rowBranch && args.rowBranch !== args.defaultBranch ? args.rowBranch : undefined; + return { base_branch: base, checkout_branch: checkout }; +} diff --git a/apps/web/components/task-create-dialog-setup.tsx b/apps/web/components/task-create-dialog-setup.tsx index 61fe021bc3..589f18e778 100644 --- a/apps/web/components/task-create-dialog-setup.tsx +++ b/apps/web/components/task-create-dialog-setup.tsx @@ -144,6 +144,7 @@ type SubmitWiringArgs = { props: TaskCreateDialogProps; fs: ReturnType; computed: ReturnType["computed"]; + workspaceRepositories: ReturnType["repositories"]; repositoryLocalPath: string; isSessionMode: boolean; isEditMode: boolean; @@ -153,6 +154,7 @@ function useSubmitHandlersWiring({ props, fs, computed, + workspaceRepositories, repositoryLocalPath, isSessionMode, isEditMode, @@ -171,6 +173,7 @@ function useSubmitHandlersWiring({ effectiveDefaultStepId: computed.effectiveDefaultStepId, repositories: fs.repositories, discoveredRepositories: fs.discoveredRepositories, + workspaceRepositories, useGitHubUrl: fs.useGitHubUrl, githubUrl: fs.githubUrl, githubPrHeadBranch: fs.githubPrHeadBranch, @@ -256,6 +259,7 @@ export function useTaskCreateDialogSetup(props: TaskCreateDialogProps) { props, fs, computed, + workspaceRepositories: repositories, repositoryLocalPath, isSessionMode, isEditMode, diff --git a/apps/web/components/task-create-dialog-submit.tsx b/apps/web/components/task-create-dialog-submit.tsx index 2717feba53..eec89f6345 100644 --- a/apps/web/components/task-create-dialog-submit.tsx +++ b/apps/web/components/task-create-dialog-submit.tsx @@ -33,6 +33,7 @@ export function useTaskSubmitHandlers({ effectiveDefaultStepId, repositories, discoveredRepositories, + workspaceRepositories, useGitHubUrl, githubUrl, githubPrHeadBranch, @@ -128,6 +129,8 @@ export function useTaskSubmitHandlers({ githubPrHeadBranch, repositories, discoveredRepositories, + workspaceRepositories, + isLocalExecutor, freshBranch: buildFreshBranchPayload(consentedDirtyFiles), }), // buildFreshBranchPayload is a closure over current scope; lint exception kept narrow. @@ -139,6 +142,8 @@ export function useTaskSubmitHandlers({ githubPrHeadBranch, repositories, discoveredRepositories, + workspaceRepositories, + isLocalExecutor, isFreshBranchActive, ], ); diff --git a/apps/web/components/task-create-dialog-types.ts b/apps/web/components/task-create-dialog-types.ts index 2217aab9af..c5086823e0 100644 --- a/apps/web/components/task-create-dialog-types.ts +++ b/apps/web/components/task-create-dialog-types.ts @@ -223,6 +223,8 @@ export type SubmitHandlersDeps = { repositories: TaskRepoRow[]; /** All on-machine discovered repos — used to look up `default_branch` for `localPath` rows. */ discoveredRepositories: LocalRepository[]; + /** Workspace repositories — used to look up `default_branch` for `repositoryId` rows. */ + workspaceRepositories: Repository[]; useGitHubUrl: boolean; githubUrl: string; githubPrHeadBranch: string | null; diff --git a/apps/web/components/task-create-dialog.tsx b/apps/web/components/task-create-dialog.tsx index 1bf32245ea..c36a535896 100644 --- a/apps/web/components/task-create-dialog.tsx +++ b/apps/web/components/task-create-dialog.tsx @@ -339,6 +339,7 @@ type SubmitWiringArgs = { props: TaskCreateDialogProps; fs: ReturnType; computed: ReturnType["computed"]; + workspaceRepositories: ReturnType["repositories"]; repositoryLocalPath: string; isSessionMode: boolean; isEditMode: boolean; @@ -348,6 +349,7 @@ function useSubmitHandlersWiring({ props, fs, computed, + workspaceRepositories, repositoryLocalPath, isSessionMode, isEditMode, @@ -366,6 +368,7 @@ function useSubmitHandlersWiring({ effectiveDefaultStepId: computed.effectiveDefaultStepId, repositories: fs.repositories, discoveredRepositories: fs.discoveredRepositories, + workspaceRepositories, useGitHubUrl: fs.useGitHubUrl, githubUrl: fs.githubUrl, githubPrHeadBranch: fs.githubPrHeadBranch, @@ -451,6 +454,7 @@ function useTaskCreateDialogSetup(props: TaskCreateDialogProps) { props, fs, computed, + workspaceRepositories: repositories, repositoryLocalPath, isSessionMode, isEditMode, diff --git a/apps/web/e2e/helpers/api-client.ts b/apps/web/e2e/helpers/api-client.ts index 78db75edff..6b7ebbf2ca 100644 --- a/apps/web/e2e/helpers/api-client.ts +++ b/apps/web/e2e/helpers/api-client.ts @@ -749,6 +749,14 @@ export class ApiClient { title: string; primary_session_id?: string | null; state?: string; + repositories?: Array<{ + id: string; + task_id: string; + repository_id: string; + base_branch: string; + checkout_branch?: string; + position: number; + }>; }> { return this.request("GET", `/api/v1/tasks/${taskId}`); } diff --git a/apps/web/e2e/tests/task/local-executor-branch-split.spec.ts b/apps/web/e2e/tests/task/local-executor-branch-split.spec.ts new file mode 100644 index 0000000000..7d75aa2801 --- /dev/null +++ b/apps/web/e2e/tests/task/local-executor-branch-split.spec.ts @@ -0,0 +1,594 @@ +import path from "node:path"; +import fs from "node:fs"; +import { execSync } from "node:child_process"; +import { test, expect } from "../../fixtures/test-base"; +import { KanbanPage } from "../../pages/kanban-page"; +import { makeGitEnv } from "../../helpers/git-helper"; + +/** + * Regression suite for the "new branch on local executor empties the changes + * panel after refresh" bug. + * + * Failure mode that originally shipped: + * 1. The dialog seeded the chip with the user's currently-checked-out branch + * and submitted it as `base_branch`. + * 2. agentctl recomputed merge-base(HEAD, origin/) on every + * refresh; for a never-pushed feature branch that collapsed to HEAD and + * `git log HEAD..HEAD` returned empty. + * 3. Live polling kept showing commits via WS while the user was on the page, + * but the moment they refreshed, the panel went blank. + * + * The forward fix splits the chip's branch into two pieces for the local + * executor (the only mode where there is no worktree and the chip semantics + * differ): `base_branch` anchors to the repo's `default_branch` (the + * integration target — what the merge-base needs to reference), and + * `checkout_branch` carries the actual working branch the preparer should + * check out. The shape is what these tests assert end-to-end. + * + * They also pin the second half of the fix: `repositories.default_branch` + * must be probed from the repo's actual integration ref (origin/HEAD, + * origin/main, origin/master, …), not from `.git/HEAD` — otherwise a user + * who first ran the dialog while checked out on a feature branch would + * permanently pin their repo's default to that feature branch, and every + * downstream merge-base lookup would be anchored wrong. + */ +test.describe("Local executor branch split", () => { + test.describe.configure({ retries: 1 }); + + type Setup = { + repoDir: string; + repoName: string; + repositoryId: string; + profileId: string; + profileName: string; + defaultBranchOnDisk: string; + }; + + /** + * Build an isolated repo per test inside backend.tmpDir so: + * - The discoveryRoots() allowlist accepts it (createRepository hits the + * same path validation as the dialog). + * - `git init -b main` ensures the integration branch exists locally and + * readGitDefaultBranch will probe it correctly. + * - Multiple feature branches let the chip exercise both "match default" + * and "differs from default" paths. + */ + async function setupLocalRepo(opts: { + apiClient: import("../../helpers/api-client").ApiClient; + backendTmpDir: string; + workspaceId: string; + suffix: string; + /** Branch the test wants the working tree checked out on at task-create + * time. Must already be created below. */ + headBranch: string; + /** Extra branches to create alongside `main`. The chip dropdown lists + * them; the test picks one to drive the split. */ + extraBranches?: string[]; + }): Promise { + const { executors } = await opts.apiClient.listExecutors(); + const localExec = executors.find((e) => e.type === "local"); + if (!localExec) return null; + const profileName = `E2E Branch Split ${opts.suffix}`; + const profile = await opts.apiClient.createExecutorProfile(localExec.id, profileName); + + const repoName = `E2E Branch Split Repo ${opts.suffix}`; + const repoDir = path.join(opts.backendTmpDir, "repos", `e2e-branch-split-${opts.suffix}`); + fs.mkdirSync(repoDir, { recursive: true }); + const env = makeGitEnv(opts.backendTmpDir); + execSync("git init -b main", { cwd: repoDir, env }); + execSync('git commit --allow-empty -m "init on main"', { cwd: repoDir, env }); + for (const branch of opts.extraBranches ?? []) { + execSync(`git checkout -B ${branch}`, { cwd: repoDir, env }); + execSync(`git commit --allow-empty -m "${branch} commit"`, { cwd: repoDir, env }); + } + execSync(`git checkout ${opts.headBranch}`, { cwd: repoDir, env }); + + // Register the repo with default_branch=main so the workspace store has + // the correct integration ref. The frontend's split logic looks this + // value up to populate base_branch. + const repo = await opts.apiClient.createRepository(opts.workspaceId, repoDir, "main", { + name: repoName, + }); + return { + repoDir, + repoName, + repositoryId: repo.id, + profileId: profile.id, + profileName, + defaultBranchOnDisk: "main", + }; + } + + function escapeRe(s: string) { + return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + } + + /** + * Drives the create-task dialog up to the point of submission with the + * local executor profile and the seeded repo selected. Returns when + * submission has fired so callers can read back the persisted task. + */ + async function createTaskViaDialog(opts: { + testPage: import("@playwright/test").Page; + title: string; + description: string; + profileName: string; + repoName: string; + /** When set, click the chip and pick this branch; otherwise leave the + * auto-seeded current branch. */ + pickBranch?: string; + }) { + const { testPage } = opts; + const kanban = new KanbanPage(testPage); + await kanban.goto(); + await kanban.createTaskButton.first().click(); + await expect(testPage.getByTestId("create-task-dialog")).toBeVisible(); + + await testPage.getByTestId("task-title-input").fill(opts.title); + await testPage.getByTestId("task-description-input").fill(opts.description); + + await testPage.getByTestId("repo-chip-trigger").first().click(); + await testPage + .getByRole("option", { name: new RegExp(`^${escapeRe(opts.repoName)}\\b`, "i") }) + .first() + .click(); + + await testPage.getByTestId("executor-profile-selector").click(); + await testPage + .getByRole("option", { name: new RegExp(`^${escapeRe(opts.profileName)}\\b`, "i") }) + .first() + .click(); + + // Wait for the chip to settle on the workspace's current branch — the + // autoselect effect runs after currentLocalBranch resolves, and submitting + // before then would race with the very logic we're testing. + const branchSelector = testPage.getByTestId("branch-chip-trigger").first(); + await expect(branchSelector).toBeEnabled({ timeout: 5_000 }); + + if (opts.pickBranch) { + await branchSelector.click(); + await testPage + .getByRole("option", { name: new RegExp(`^${escapeRe(opts.pickBranch)}\\b`) }) + .first() + .click(); + await expect(branchSelector).toContainText(opts.pickBranch); + } + + // Use the "create without starting agent" path so the test doesn't wait + // for agent boot. We care about the persisted task shape, not session + // lifecycle. The chevron opens the split-button menu. + // + // Wait on the *response* rather than the request — waitForRequest + // resolves the moment the browser sends the POST, before the server has + // finished writing to the database. Callers that read the task back via + // the API immediately after would race the persistence. + const createTaskResponse = opts.testPage.waitForResponse( + (res) => res.url().endsWith("/api/v1/tasks") && res.request().method() === "POST", + ); + await testPage.getByTestId("submit-start-agent-chevron").click(); + await testPage.getByTestId("submit-create-without-agent").click(); + await createTaskResponse; + } + + test("submits with base_branch=default_branch and checkout_branch=working_branch", async ({ + testPage, + apiClient, + backend, + seedData, + }) => { + // The pre-fix shape was {base_branch: "feature/x", checkout_branch: ""} + // which caused the post-refresh empty-panel bug. The new shape isolates + // the integration ref from the working branch so the merge-base + // recomputation downstream lands on the actual fork point instead of + // collapsing to HEAD. + const setup = await setupLocalRepo({ + apiClient, + backendTmpDir: backend.tmpDir, + workspaceId: seedData.workspaceId, + suffix: "split", + headBranch: "feature/x", + extraBranches: ["feature/x"], + }); + if (!setup) { + test.skip(true, "No local executor available"); + return; + } + try { + await createTaskViaDialog({ + testPage, + title: "Local executor branch split", + description: "regression: base/checkout split for local executor", + profileName: setup.profileName, + repoName: setup.repoName, + }); + + // Find the just-created task by its title and assert its repository row. + const tasksRes = await apiClient.rawRequest( + "GET", + `/api/v1/workspaces/${seedData.workspaceId}/tasks`, + ); + const tasksJson = (await tasksRes.json()) as { + tasks: Array<{ id: string; title: string }>; + }; + const task = tasksJson.tasks.find((t) => t.title === "Local executor branch split"); + expect(task, "task should exist").toBeTruthy(); + + const fullTask = await apiClient.getTask(task!.id); + const taskRepo = fullTask.repositories?.[0]; + expect(taskRepo, "task should have a repository row").toBeTruthy(); + expect(taskRepo!.repository_id).toBe(setup.repositoryId); + expect( + taskRepo!.base_branch, + "base_branch must be the repo's default_branch — that's what the changes panel uses as merge-base reference", + ).toBe("main"); + expect( + taskRepo!.checkout_branch, + "checkout_branch carries the working branch the preparer must switch to", + ).toBe("feature/x"); + } finally { + await apiClient.deleteExecutorProfile(setup.profileId).catch(() => {}); + } + }); + + test("checkout_branch is omitted when picked branch matches default_branch", async ({ + testPage, + apiClient, + backend, + seedData, + }) => { + // When the user is on `main` (matching default_branch), there's nothing + // for the preparer to check out. The frontend should NOT send a redundant + // checkout_branch — the LocalPreparer's "skip when current matches" + // optimization relies on it being absent so we don't fire spurious git + // ops on every task creation. + const setup = await setupLocalRepo({ + apiClient, + backendTmpDir: backend.tmpDir, + workspaceId: seedData.workspaceId, + suffix: "match", + headBranch: "main", + }); + if (!setup) { + test.skip(true, "No local executor available"); + return; + } + try { + await createTaskViaDialog({ + testPage, + title: "Branch matches default", + description: "no checkout_branch needed", + profileName: setup.profileName, + repoName: setup.repoName, + }); + + const tasksRes = await apiClient.rawRequest( + "GET", + `/api/v1/workspaces/${seedData.workspaceId}/tasks`, + ); + const tasksJson = (await tasksRes.json()) as { + tasks: Array<{ id: string; title: string }>; + }; + const task = tasksJson.tasks.find((t) => t.title === "Branch matches default"); + expect(task, "task should exist").toBeTruthy(); + + const fullTask = await apiClient.getTask(task!.id); + const taskRepo = fullTask.repositories?.[0]; + expect(taskRepo!.base_branch).toBe("main"); + // omitempty on the DTO — empty string round-trips as undefined/missing. + expect(taskRepo!.checkout_branch ?? "").toBe(""); + } finally { + await apiClient.deleteExecutorProfile(setup.profileId).catch(() => {}); + } + }); + + test("explicit branch pick: chip switches to develop, payload still splits correctly", async ({ + testPage, + apiClient, + backend, + seedData, + }) => { + // The chip auto-seeds with the workspace's current branch (main here), + // but the user can switch to any existing branch. The split must still + // anchor base_branch to default_branch regardless of how the chip got + // its value (autoselect vs explicit pick). + const setup = await setupLocalRepo({ + apiClient, + backendTmpDir: backend.tmpDir, + workspaceId: seedData.workspaceId, + suffix: "pick", + headBranch: "main", + extraBranches: ["develop"], + }); + if (!setup) { + test.skip(true, "No local executor available"); + return; + } + try { + await createTaskViaDialog({ + testPage, + title: "Branch pick split", + description: "user explicitly picked develop", + profileName: setup.profileName, + repoName: setup.repoName, + pickBranch: "develop", + }); + + const tasksRes = await apiClient.rawRequest( + "GET", + `/api/v1/workspaces/${seedData.workspaceId}/tasks`, + ); + const tasksJson = (await tasksRes.json()) as { + tasks: Array<{ id: string; title: string }>; + }; + const task = tasksJson.tasks.find((t) => t.title === "Branch pick split"); + expect(task, "task should exist").toBeTruthy(); + + const fullTask = await apiClient.getTask(task!.id); + const taskRepo = fullTask.repositories?.[0]; + expect(taskRepo!.base_branch).toBe("main"); + expect(taskRepo!.checkout_branch).toBe("develop"); + } finally { + await apiClient.deleteExecutorProfile(setup.profileId).catch(() => {}); + } + }); +}); + +/** + * Fresh-branch flow has different chip semantics: row.branch is the base to + * fork FROM, not a working branch. The split must NOT apply here — otherwise + * picking a non-default base would silently fork from main instead of the + * picked base. + */ +test.describe("Local executor + fresh-branch toggle", () => { + test.describe.configure({ retries: 1 }); + + type Setup = { + repoDir: string; + repoName: string; + profileId: string; + profileName: string; + }; + + async function setupFreshBranchRepo(opts: { + apiClient: import("../../helpers/api-client").ApiClient; + backendTmpDir: string; + workspaceId: string; + suffix: string; + }): Promise { + const { executors } = await opts.apiClient.listExecutors(); + const localExec = executors.find((e) => e.type === "local"); + if (!localExec) return null; + const profileName = `E2E Fresh Split ${opts.suffix}`; + const profile = await opts.apiClient.createExecutorProfile(localExec.id, profileName); + const repoName = `E2E Fresh Split Repo ${opts.suffix}`; + const repoDir = path.join(opts.backendTmpDir, "repos", `e2e-fresh-split-${opts.suffix}`); + fs.mkdirSync(repoDir, { recursive: true }); + const env = makeGitEnv(opts.backendTmpDir); + execSync("git init -b main", { cwd: repoDir, env }); + execSync('git commit --allow-empty -m "init"', { cwd: repoDir, env }); + execSync("git checkout -b develop", { cwd: repoDir, env }); + execSync('git commit --allow-empty -m "develop work"', { cwd: repoDir, env }); + execSync("git checkout main", { cwd: repoDir, env }); + await opts.apiClient.createRepository(opts.workspaceId, repoDir, "main", { name: repoName }); + return { repoDir, repoName, profileId: profile.id, profileName }; + } + + function escapeRe(s: string) { + return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + } + + test("payload sends base_branch=picked-base verbatim, no checkout_branch split", async ({ + testPage, + apiClient, + backend, + seedData, + }) => { + // Regression for the gap noticed during review: the local-executor branch + // split would otherwise force base_branch=default_branch and the backend's + // applyFreshBranch would fork from main instead of the user's pick. + // + // The frontend's contract is to send the chip's value verbatim as + // base_branch when fresh-branch is active. Backend persistence of the + // resulting fork-branch name is a separate concern covered by + // service-layer tests on PerformFreshBranch. We only assert the wire + // payload here so this test stays focused on the regression and doesn't + // entangle with the post-create session-launch path. + const setup = await setupFreshBranchRepo({ + apiClient, + backendTmpDir: backend.tmpDir, + workspaceId: seedData.workspaceId, + suffix: "develop-base", + }); + if (!setup) { + test.skip(true, "No local executor available"); + return; + } + try { + const kanban = new KanbanPage(testPage); + await kanban.goto(); + await kanban.createTaskButton.first().click(); + await expect(testPage.getByTestId("create-task-dialog")).toBeVisible(); + await testPage.getByTestId("task-title-input").fill("Fork from develop"); + await testPage.getByTestId("task-description-input").fill("non-default fork base"); + await testPage.getByTestId("repo-chip-trigger").first().click(); + await testPage + .getByRole("option", { name: new RegExp(`^${escapeRe(setup.repoName)}\\b`, "i") }) + .first() + .click(); + await testPage.getByTestId("executor-profile-selector").click(); + await testPage + .getByRole("option", { name: new RegExp(`^${escapeRe(setup.profileName)}\\b`, "i") }) + .first() + .click(); + + // Flip on fresh-branch and pick "develop" as the base to fork from. + await testPage.getByTestId("fresh-branch-toggle").click(); + const branchSelector = testPage.getByTestId("branch-chip-trigger").first(); + await expect(branchSelector).toBeEnabled({ timeout: 5_000 }); + await branchSelector.click(); + await testPage + .getByRole("option", { name: /develop/ }) + .first() + .click(); + await expect(branchSelector).toContainText("develop"); + + // Wait on the response so the server has finished persisting before we + // inspect the request body. waitForRequest resolves on send, which + // would race the rest of the test on slow CI. + const createTaskResponse = testPage.waitForResponse( + (res) => res.url().endsWith("/api/v1/tasks") && res.request().method() === "POST", + ); + await testPage.getByTestId("submit-start-agent-chevron").click(); + await testPage.getByTestId("submit-create-without-agent").click(); + const req = (await createTaskResponse).request(); + + // Wire-level contract: base_branch must carry "develop" verbatim and + // checkout_branch must be absent. fresh_branch must be true so the + // backend's applyFreshBranch flow knows to fork. Without the split + // bypass, base_branch would be "main" and applyFreshBranch would fork + // from the wrong base. + const payload = JSON.parse(req.postData() ?? "{}") as { + repositories?: Array<{ + base_branch?: string; + checkout_branch?: string; + fresh_branch?: boolean; + }>; + }; + const repo = payload.repositories?.[0]; + expect(repo?.base_branch).toBe("develop"); + expect(repo?.checkout_branch ?? undefined).toBeUndefined(); + expect(repo?.fresh_branch).toBe(true); + } finally { + await apiClient.deleteExecutorProfile(setup.profileId).catch(() => {}); + } + }); + + test("forks from default base when chip is left on the auto-seeded current branch", async ({ + testPage, + apiClient, + backend, + seedData, + }) => { + // Common path: user is on main (the workspace default), enables fresh- + // branch, leaves the chip alone. Backend forks from main. We just verify + // the resulting branch points at main's tip and the wire payload sent + // base_branch="main" without a redundant checkout_branch. + const setup = await setupFreshBranchRepo({ + apiClient, + backendTmpDir: backend.tmpDir, + workspaceId: seedData.workspaceId, + suffix: "main-base", + }); + if (!setup) { + test.skip(true, "No local executor available"); + return; + } + try { + const kanban = new KanbanPage(testPage); + await kanban.goto(); + await kanban.createTaskButton.first().click(); + await expect(testPage.getByTestId("create-task-dialog")).toBeVisible(); + await testPage.getByTestId("task-title-input").fill("Fork from main"); + await testPage.getByTestId("task-description-input").fill("default fork base"); + await testPage.getByTestId("repo-chip-trigger").first().click(); + await testPage + .getByRole("option", { name: new RegExp(`^${escapeRe(setup.repoName)}\\b`, "i") }) + .first() + .click(); + await testPage.getByTestId("executor-profile-selector").click(); + await testPage + .getByRole("option", { name: new RegExp(`^${escapeRe(setup.profileName)}\\b`, "i") }) + .first() + .click(); + await testPage.getByTestId("fresh-branch-toggle").click(); + const branchSelector = testPage.getByTestId("branch-chip-trigger").first(); + await expect(branchSelector).toBeEnabled({ timeout: 5_000 }); + await expect(branchSelector).toContainText("main"); + + const createTaskResponse = testPage.waitForResponse( + (res) => res.url().endsWith("/api/v1/tasks") && res.request().method() === "POST", + ); + await testPage.getByTestId("submit-start-agent-chevron").click(); + await testPage.getByTestId("submit-create-without-agent").click(); + const req = (await createTaskResponse).request(); + + const payload = JSON.parse(req.postData() ?? "{}") as { + repositories?: Array<{ base_branch?: string; checkout_branch?: string }>; + }; + expect(payload.repositories?.[0]?.base_branch).toBe("main"); + expect(payload.repositories?.[0]?.checkout_branch ?? undefined).toBeUndefined(); + } finally { + await apiClient.deleteExecutorProfile(setup.profileId).catch(() => {}); + } + }); +}); + +/** + * Pins the readGitDefaultBranch fix: the validate-path endpoint (which the + * dialog uses to populate `default_branch` for discovered local paths) must + * return the integration branch, not whatever ref `.git/HEAD` happens to + * point at. The pre-fix code read HEAD directly, so a user checked out on + * a feature branch would permanently pin the resulting `repositories` + * row's `default_branch` to that feature branch. + */ +test.describe("Repository default_branch detection", () => { + test.describe.configure({ retries: 1 }); + + test("validate-path returns 'main' even when HEAD is on a feature branch", async ({ + apiClient, + backend, + seedData, + }) => { + const repoDir = path.join(backend.tmpDir, "repos", "e2e-default-branch-probe"); + fs.mkdirSync(repoDir, { recursive: true }); + const env = makeGitEnv(backend.tmpDir); + execSync("git init -b main", { cwd: repoDir, env }); + execSync('git commit --allow-empty -m "init"', { cwd: repoDir, env }); + execSync("git checkout -b feature/probe", { cwd: repoDir, env }); + execSync('git commit --allow-empty -m "feature work"', { cwd: repoDir, env }); + // Leave HEAD on feature/probe — the pre-fix code returned this string, + // which is exactly what we're guarding against. + expect( + execSync("git rev-parse --abbrev-ref HEAD", { cwd: repoDir, env }).toString().trim(), + ).toBe("feature/probe"); + + const res = await apiClient.rawRequest( + "GET", + `/api/v1/workspaces/${seedData.workspaceId}/repositories/validate?path=${encodeURIComponent(repoDir)}`, + ); + expect(res.status).toBe(200); + const body = (await res.json()) as { + is_git: boolean; + default_branch: string; + allowed: boolean; + }; + expect(body.is_git).toBe(true); + expect(body.allowed).toBe(true); + expect( + body.default_branch, + "must probe origin/HEAD or main/master, not echo back .git/HEAD", + ).toBe("main"); + }); + + test("validate-path falls back to feature branch only when no main/master exists", async ({ + apiClient, + backend, + seedData, + }) => { + // A truly local-only repo with only a feature branch has no integration + // ref to anchor to. The probe should return the current HEAD as a last + // resort so the dialog doesn't error out — callers can still override. + const repoDir = path.join(backend.tmpDir, "repos", "e2e-default-branch-fallback"); + fs.mkdirSync(repoDir, { recursive: true }); + const env = makeGitEnv(backend.tmpDir); + execSync("git init -b solo", { cwd: repoDir, env }); + execSync('git commit --allow-empty -m "init"', { cwd: repoDir, env }); + + const res = await apiClient.rawRequest( + "GET", + `/api/v1/workspaces/${seedData.workspaceId}/repositories/validate?path=${encodeURIComponent(repoDir)}`, + ); + const body = (await res.json()) as { default_branch: string }; + expect(body.default_branch).toBe("solo"); + }); +});