From 6ceda6ae4e28a8d9305e4a9cf442ca2e9308709d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Tue, 5 May 2026 22:05:01 +0100 Subject: [PATCH] fix(local-executor): preserve commits in changes panel after refresh (#834) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(local-executor): preserve commits in changes panel after refresh For a local executor on a feature branch, the create-task dialog sent the chip's branch as base_branch with empty checkout_branch. agentctl then recomputed merge-base(HEAD, origin/) on every changes-panel refresh; for a never-pushed branch that collapsed to HEAD and the panel went empty. Repository creation also pinned default_branch to .git/HEAD, permanently anchoring the repo's integration ref to whatever feature branch the user happened to be on at first task-create time. The frontend now splits the chip into base_branch=default_branch + checkout_branch=working_branch (skipping the split when fresh-branch is on, where the chip semantically means "fork from"). The backend probes origin/HEAD then origin/main → main → master (new common/gitref package) to pick a repo's integration branch, and resolveRepoInput no longer falls back to BaseBranch when default_branch is missing. Covered by 9 unit tests on the helper, 6 Go tests on the probe order, and 7 e2e tests across plain creation, the fork toggle, and the validate-path probe. * fix: address PR review feedback - gitref: preserve slashes in HEAD-fallback branch names (TrimPrefix refs/heads/ instead of Split + last component); branches like "feature/my-feature" no longer collapse to "my-feature" and poison the stored default_branch. - gitref: drop the broken packed-refs symref scan in readOriginHEAD; the loose refs/remotes/origin/HEAD check + pickFirstExistingBranch candidates already cover every realistic clone. - service.resolveRepoInput: validate repoInput.LocalPath via resolveAllowedLocalPath() before probing — closes 8 CodeQL path-injection alerts. - buildRepositoriesPayload: fall through to the legacy non-split shape when default_branch is unknown for the row (legacy repos with unset default_branch). Synthesizing base_branch=rowBranch was reproducing the very bug this PR fixes. * fix(gitref): explicit traversal guard at I/O boundary for CodeQL Move the path validation from service.resolveAllowedLocalPath (which CodeQL's taint analysis doesn't recognize as a sanitizer) into a guardRepoPath check at gitref.DefaultBranch's entry. The new check rejects relative paths and any '..' segments before any os.Stat / os.ReadFile call sees the value, which CodeQL recognizes as a go/path-injection sanitizer. The upstream allowlist check stays in place — this is a belt-and- suspenders second barrier at the actual I/O boundary, and it also makes the gitref package safe for any future callers (migrations, tests) without requiring them to thread a Service in. * fix: address review feedback on hardening + e2e race - gitref.guardRepoPath: check the RAW input for '..' segments before filepath.Clean. Clean resolves '..' away from absolute paths ("/allowed/../etc" -> "/etc"), so iterating the cleaned path was a silent no-op for the very inputs we wanted to reject. Split on both '/' and the OS separator. Updated the test with three traversal cases. - gitref_test.go: switch from strings.EqualFold to strict equality — git branch names are case-sensitive, "Main" should not pass for "main". - e2e: replace waitForRequest with waitForResponse in local-executor-branch-split.spec.ts. waitForRequest resolves on send (before the server has persisted the task), so the followup apiClient.getTask call could race the write on slow CI. The two tests that need the request body keep access via (await waitForResponse).request(). * fix(ws): forward CheckoutBranch in wsUpdateTask The PR fix added CheckoutBranch forwarding to wsCreateTask but missed the same conversion loop in wsUpdateTask. Any task-update payload with a repositories array would silently zero out checkout_branch and re-introduce the merge-base collapse on refresh — same root cause this PR fixes, in the update path. --------- Co-authored-by: Kandev Agent --- apps/backend/internal/common/gitref/gitref.go | 207 ++++++ .../internal/common/gitref/gitref_test.go | 67 ++ apps/backend/internal/task/dto/dto.go | 34 +- .../task/handlers/task_ws_handlers.go | 26 +- .../task/service/repository_discovery.go | 27 +- .../task/service/repository_discovery_test.go | 119 +++- .../internal/task/service/service_tasks.go | 19 +- .../components/task-create-dialog-footer.tsx | 1 + ...k-create-dialog-helpers.multi-repo.test.ts | 155 ++++- .../components/task-create-dialog-helpers.ts | 80 ++- .../components/task-create-dialog-setup.tsx | 4 + .../components/task-create-dialog-submit.tsx | 5 + .../components/task-create-dialog-types.ts | 2 + apps/web/components/task-create-dialog.tsx | 4 + apps/web/e2e/helpers/api-client.ts | 8 + .../task/local-executor-branch-split.spec.ts | 594 ++++++++++++++++++ 16 files changed, 1279 insertions(+), 73 deletions(-) create mode 100644 apps/backend/internal/common/gitref/gitref.go create mode 100644 apps/backend/internal/common/gitref/gitref_test.go create mode 100644 apps/web/e2e/tests/task/local-executor-branch-split.spec.ts 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"); + }); +});