diff --git a/backend/internal/adapters/workspace/gitworktree/commands.go b/backend/internal/adapters/workspace/gitworktree/commands.go index 356a50e1..56adfa32 100644 --- a/backend/internal/adapters/workspace/gitworktree/commands.go +++ b/backend/internal/adapters/workspace/gitworktree/commands.go @@ -27,6 +27,16 @@ func worktreeRemoveArgs(repo, path string) []string { return []string{"-C", repo, "worktree", "remove", path} } +// worktreeRemoveForceArgs is used ONLY by Create's cleanup-on-failure path. +// It is deliberately distinct from worktreeRemoveArgs (which omits --force +// for Destroy) because the contexts are different: Destroy operates on +// worktrees that may contain in-progress agent commits; Create cleanup +// operates on an orphan from a just-attempted creation where no agent has +// committed. NEVER use this from Destroy. +func worktreeRemoveForceArgs(repo, path string) []string { + return []string{"-C", repo, "worktree", "remove", "--force", path} +} + func worktreePruneArgs(repo string) []string { return []string{"-C", repo, "worktree", "prune"} } @@ -35,12 +45,30 @@ func worktreeListPorcelainArgs(repo string) []string { return []string{"-C", repo, "worktree", "list", "--porcelain"} } -func baseRefCandidates(branch, defaultBranch string) []string { - candidates := []string{"origin/" + branch} - if strings.Contains(defaultBranch, "/") { +func remoteGetURLOriginArgs(repo string) []string { + return []string{"-C", repo, "remote", "get-url", "origin"} +} + +func fetchOriginQuietArgs(repo string) []string { + return []string{"-C", repo, "fetch", "origin", "--quiet"} +} + +// baseRefCandidates returns the ordered list of refs to probe for a new +// worktree's base. When hasOrigin is true we prefer remote-tracking refs; +// when false we skip them entirely so we don't burn subprocesses on lookups +// that can't succeed. defaultBranch may already be qualified (e.g. +// "upstream/main"), in which case it is used as-is. +func baseRefCandidates(branch, defaultBranch string, hasOrigin bool) []string { + var candidates []string + if hasOrigin { + candidates = append(candidates, "origin/"+branch) + if strings.Contains(defaultBranch, "/") { + candidates = append(candidates, defaultBranch) + } else { + candidates = append(candidates, "origin/"+defaultBranch) + } + } else if strings.Contains(defaultBranch, "/") { candidates = append(candidates, defaultBranch) - } else { - candidates = append(candidates, "origin/"+defaultBranch) } return append(candidates, branch) } diff --git a/backend/internal/adapters/workspace/gitworktree/workspace.go b/backend/internal/adapters/workspace/gitworktree/workspace.go index a2a9cd97..8db87f24 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace.go @@ -8,14 +8,16 @@ import ( "os/exec" "path/filepath" "strings" + "time" "github.com/aoagents/agent-orchestrator/backend/internal/domain" "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) const ( - defaultGitBinary = "git" - defaultBranch = "main" + defaultGitBinary = "git" + defaultBranch = "main" + defaultCommandTimeout = 30 * time.Second ) // ErrUnsafePath is returned when a resolved worktree path escapes the managed @@ -108,6 +110,11 @@ func (w *Workspace) Create(ctx context.Context, cfg ports.WorkspaceConfig) (port if err := w.validateBranch(ctx, repo, cfg.Branch); err != nil { return ports.WorkspaceInfo{}, err } + // NOTE: hasOriginRemote is also called inside resolveBaseRef; consolidation + // deferred (see the followup issue for upstream-parity work). + if w.hasOriginRemote(ctx, repo) { + w.fetchOrigin(ctx, repo) + } path, err := w.managedPath(cfg.ProjectID, cfg.SessionID) if err != nil { return ports.WorkspaceInfo{}, err @@ -183,11 +190,24 @@ func (w *Workspace) Restore(ctx context.Context, cfg ports.WorkspaceConfig) (por if nonEmpty, err := pathExistsNonEmpty(path); err != nil { return ports.WorkspaceInfo{}, err } else if nonEmpty { - return ports.WorkspaceInfo{}, fmt.Errorf("gitworktree: refusing to restore %q: path exists and is not a registered worktree", path) + // Path is non-empty AND not in the `git worktree list` records + // (findWorktree above already returned for the registered case). + // Safe to remove: we are NOT deleting a registered worktree — that + // invariant is preserved by the findWorktree shortcut above. + // Mirrors upstream's cleanupStaleWorkspacePath without compromising + // data safety. + if err := os.RemoveAll(path); err != nil { + return ports.WorkspaceInfo{}, fmt.Errorf("gitworktree: remove unregistered residue %q: %w", path, err) + } } if err := w.validateBranch(ctx, repo, cfg.Branch); err != nil { return ports.WorkspaceInfo{}, err } + // NOTE: hasOriginRemote is also called inside resolveBaseRef; consolidation + // deferred (see the followup issue for upstream-parity work). + if w.hasOriginRemote(ctx, repo) { + w.fetchOrigin(ctx, repo) + } if err := w.addWorktree(ctx, repo, path, cfg.Branch); err != nil { return ports.WorkspaceInfo{}, err } @@ -201,6 +221,7 @@ func (w *Workspace) addWorktree(ctx context.Context, repo, path, branch string) } if localBranch { if _, err := w.run(ctx, w.binary, worktreeAddBranchArgs(repo, path, branch)...); err != nil { + w.cleanupOrphan(ctx, repo, path) return fmt.Errorf("gitworktree: worktree add existing branch %q: %w", branch, err) } return nil @@ -210,11 +231,25 @@ func (w *Workspace) addWorktree(ctx context.Context, repo, path, branch string) return err } if _, err := w.run(ctx, w.binary, worktreeAddNewBranchArgs(repo, branch, path, baseRef)...); err != nil { + w.cleanupOrphan(ctx, repo, path) return fmt.Errorf("gitworktree: worktree add branch %q from %q: %w", branch, baseRef, err) } return nil } +// cleanupOrphan best-effort removes an orphan worktree left by a failed +// `git worktree add`. Errors are swallowed: the caller is already returning +// the underlying add failure, which is the actionable signal. Safe to use +// --force here because no agent has had a chance to commit into this path. +// A fresh background context (with its own timeout) is used so that a +// cancelled caller context — e.g. an HTTP request aborted mid-add — does +// not also kill the cleanup subprocess and leave the orphan behind. +func (w *Workspace) cleanupOrphan(_ context.Context, repo, path string) { + ctx, cancel := context.WithTimeout(context.Background(), defaultCommandTimeout) + defer cancel() + _, _ = w.run(ctx, w.binary, worktreeRemoveForceArgs(repo, path)...) +} + func (w *Workspace) validateBranch(ctx context.Context, repo, branch string) error { if _, err := w.run(ctx, w.binary, checkRefFormatBranchArgs(repo, branch)...); err != nil { return fmt.Errorf("gitworktree: invalid branch %q: %w", branch, err) @@ -223,7 +258,7 @@ func (w *Workspace) validateBranch(ctx context.Context, repo, branch string) err } func (w *Workspace) resolveBaseRef(ctx context.Context, repo, branch string) (string, error) { - candidates := baseRefCandidates(branch, w.defaultBranch) + candidates := baseRefCandidates(branch, w.defaultBranch, w.hasOriginRemote(ctx, repo)) for _, ref := range candidates { exists, err := w.refExists(ctx, repo, ref) if err != nil { @@ -236,6 +271,28 @@ func (w *Workspace) resolveBaseRef(ctx context.Context, repo, branch string) (st return "", fmt.Errorf("gitworktree: no base ref found for branch %q (tried %s)", branch, strings.Join(candidates, ", ")) } +// hasOriginRemote reports whether the repo has a remote named "origin". The +// error from git is intentionally swallowed: any failure — including "no such +// remote", corrupt config, or transient I/O — is treated as "origin absent". +// This is safe because the callers (resolveBaseRef, Create, Restore) fall +// back to local refs; the worst outcome of a false negative is that +// remote-tracking candidates are skipped, which is exactly correct for a +// no-origin repo. +func (w *Workspace) hasOriginRemote(ctx context.Context, repo string) bool { + _, err := w.run(ctx, w.binary, remoteGetURLOriginArgs(repo)...) + return err == nil +} + +// fetchOrigin runs `git fetch origin --quiet` and swallows the error. Offline +// flows must still succeed; the caller can only act on locally-cached refs in +// that case. We don't surface the fetch error because there's nothing the +// caller can do about it that we aren't already doing (proceed with cached +// refs and let the subsequent base-ref resolve fail loudly if there's no +// usable ref). +func (w *Workspace) fetchOrigin(ctx context.Context, repo string) { + _, _ = w.run(ctx, w.binary, fetchOriginQuietArgs(repo)...) +} + func (w *Workspace) refExists(ctx context.Context, repo, ref string) (bool, error) { _, err := w.run(ctx, w.binary, revParseVerifyArgs(repo, ref)...) if err == nil { @@ -393,7 +450,21 @@ func pathExistsNonEmpty(path string) (bool, error) { return false, fmt.Errorf("gitworktree: inspect path %q: %w", path, err) } +// withDefaultTimeout returns ctx unchanged when it already has a deadline, or +// wraps it with a default per-command timeout otherwise. The caller is the +// authority on cancellation — we only fill in a backstop so a forgotten +// `context.Background()` can't hang on `git fetch` against an unreachable +// remote. +func withDefaultTimeout(ctx context.Context, d time.Duration) (context.Context, context.CancelFunc) { + if _, ok := ctx.Deadline(); ok { + return ctx, func() {} + } + return context.WithTimeout(ctx, d) +} + func runCommand(ctx context.Context, binary string, args ...string) ([]byte, error) { + ctx, cancel := withDefaultTimeout(ctx, defaultCommandTimeout) + defer cancel() cmd := exec.CommandContext(ctx, binary, args...) out, err := cmd.CombinedOutput() if err != nil { diff --git a/backend/internal/adapters/workspace/gitworktree/workspace_integration_test.go b/backend/internal/adapters/workspace/gitworktree/workspace_integration_test.go index 6dc50f76..f9a5d364 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace_integration_test.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace_integration_test.go @@ -92,6 +92,45 @@ func TestWorkspaceIntegrationDestroyRefusesLockedWorktree(t *testing.T) { } } +func TestWorkspaceIntegrationCreateFetchesLatestRemote(t *testing.T) { + git := requireGit(t) + tmp := t.TempDir() + repo := setupOriginClone(t, git, tmp) + root := filepath.Join(tmp, "managed") + ws, err := New(Options{Binary: git, ManagedRoot: root, RepoResolver: StaticRepoResolver{"proj": repo}}) + if err != nil { + t.Fatalf("new: %v", err) + } + + // Push a new commit to origin/main FROM a separate pusher clone, bypassing + // the working clone. Without fetch, the working clone wouldn't see it. + origin := filepath.Join(tmp, "origin.git") + pusher := filepath.Join(tmp, "pusher") + run(t, git, "clone", origin, pusher) + // Mirror setupOriginClone's explicit `checkout main`: on CI runners where + // `init.defaultBranch` is `master`, the bare `origin.git` HEAD still points + // at refs/heads/master after the seed pushed main, so a fresh `git clone` + // here checks out HEAD (master, which doesn't exist) and lands the pusher + // on no branch. + runGit(t, git, pusher, "checkout", "main") + runGit(t, git, pusher, "config", "user.email", "ao@example.com") + runGit(t, git, pusher, "config", "user.name", "Ao Agents") + if err := os.WriteFile(filepath.Join(pusher, "post-seed.txt"), []byte("after seed\n"), 0o644); err != nil { + t.Fatalf("write post-seed: %v", err) + } + runGit(t, git, pusher, "add", "post-seed.txt") + runGit(t, git, pusher, "commit", "-m", "post-seed") + runGit(t, git, pusher, "push", "origin", "main") + + info, err := ws.Create(context.Background(), ports.WorkspaceConfig{ProjectID: "proj", SessionID: "sess", Branch: "feature/fetch"}) + if err != nil { + t.Fatalf("create: %v", err) + } + if _, err := os.Stat(filepath.Join(info.Path, "post-seed.txt")); err != nil { + t.Fatalf("expected post-seed.txt in worktree (proves fetch ran): %v", err) + } +} + func requireGit(t *testing.T) string { t.Helper() git, err := exec.LookPath("git") diff --git a/backend/internal/adapters/workspace/gitworktree/workspace_test.go b/backend/internal/adapters/workspace/gitworktree/workspace_test.go index fa14f527..4992c16a 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace_test.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace_test.go @@ -3,15 +3,42 @@ package gitworktree import ( "context" "errors" + "fmt" "os" + "os/exec" "path/filepath" "reflect" "strings" "testing" + "time" "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) +func TestWithDefaultTimeoutNoExistingDeadline(t *testing.T) { + ctx := context.Background() + wrapped, cancel := withDefaultTimeout(ctx, defaultCommandTimeout) + defer cancel() + deadline, ok := wrapped.Deadline() + if !ok { + t.Fatal("expected deadline, got none") + } + until := time.Until(deadline) + if until <= 0 || until > defaultCommandTimeout { + t.Fatalf("deadline %v not in (0, %v]", until, defaultCommandTimeout) + } +} + +func TestWithDefaultTimeoutPreservesExistingDeadline(t *testing.T) { + parent, parentCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer parentCancel() + wrapped, cancel := withDefaultTimeout(parent, defaultCommandTimeout) + defer cancel() + if wrapped != parent { + t.Fatal("expected same context when deadline already set") + } +} + func TestCommandArgs(t *testing.T) { repo := "/repo" path := "/managed/proj/sess" @@ -32,6 +59,9 @@ func TestCommandArgs(t *testing.T) { {"remove", worktreeRemoveArgs(repo, path), []string{"-C", repo, "worktree", "remove", path}}, {"prune", worktreePruneArgs(repo), []string{"-C", repo, "worktree", "prune"}}, {"list", worktreeListPorcelainArgs(repo), []string{"-C", repo, "worktree", "list", "--porcelain"}}, + {"remote get-url", remoteGetURLOriginArgs(repo), []string{"-C", repo, "remote", "get-url", "origin"}}, + {"fetch origin quiet", fetchOriginQuietArgs(repo), []string{"-C", repo, "fetch", "origin", "--quiet"}}, + {"remove force", worktreeRemoveForceArgs(repo, path), []string{"-C", repo, "worktree", "remove", "--force", path}}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -42,20 +72,58 @@ func TestCommandArgs(t *testing.T) { } } -func TestBaseRefCandidates(t *testing.T) { - got := baseRefCandidates("feature/test", "main") +func TestBaseRefCandidatesWithOrigin(t *testing.T) { + got := baseRefCandidates("feature/test", "main", true) want := []string{"origin/feature/test", "origin/main", "feature/test"} if !reflect.DeepEqual(got, want) { t.Fatalf("candidates = %#v, want %#v", got, want) } - got = baseRefCandidates("feature/test", "upstream/main") + got = baseRefCandidates("feature/test", "upstream/main", true) want = []string{"origin/feature/test", "upstream/main", "feature/test"} if !reflect.DeepEqual(got, want) { t.Fatalf("qualified candidates = %#v, want %#v", got, want) } } +func TestBaseRefCandidatesWithoutOrigin(t *testing.T) { + got := baseRefCandidates("feature/test", "main", false) + want := []string{"feature/test"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("no-origin plain default = %#v, want %#v", got, want) + } + + got = baseRefCandidates("feature/test", "upstream/main", false) + want = []string{"upstream/main", "feature/test"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("no-origin qualified default = %#v, want %#v", got, want) + } +} + +func TestHasOriginRemote(t *testing.T) { + ws, err := New(Options{ManagedRoot: t.TempDir(), RepoResolver: StaticRepoResolver{"p": "/repo"}}) + if err != nil { + t.Fatalf("new: %v", err) + } + var lastArgs []string + ws.run = func(_ context.Context, _ string, args ...string) ([]byte, error) { + lastArgs = append([]string{}, args...) + return []byte("git@github.com:foo/bar.git\n"), nil + } + if !ws.hasOriginRemote(context.Background(), "/repo") { + t.Fatal("expected true when remote get-url succeeds") + } + if want := []string{"-C", "/repo", "remote", "get-url", "origin"}; !reflect.DeepEqual(lastArgs, want) { + t.Fatalf("args = %#v, want %#v", lastArgs, want) + } + ws.run = func(context.Context, string, ...string) ([]byte, error) { + return nil, errors.New("fatal: no such remote 'origin'") + } + if ws.hasOriginRemote(context.Background(), "/repo") { + t.Fatal("expected false when remote get-url fails") + } +} + func TestParseWorktreePorcelain(t *testing.T) { input := strings.Join([]string{ "worktree /repo", @@ -165,23 +233,77 @@ func TestValidateConfigAcceptsBenignIDs(t *testing.T) { } } -func TestRestoreRefusesNonEmptyUnregisteredPath(t *testing.T) { +func TestRestoreAutoCleansUnregisteredResidueAndProceeds(t *testing.T) { root := t.TempDir() repo := t.TempDir() ws, err := New(Options{ManagedRoot: root, RepoResolver: StaticRepoResolver{"proj": repo}}) if err != nil { t.Fatalf("new: %v", err) } - ws.run = func(context.Context, string, ...string) ([]byte, error) { - return []byte("worktree " + repo + "\nbranch refs/heads/main\n"), nil + path := filepath.Join(ws.managedRoot, "proj", "sess") + if err := mkdirFile(path, "leftover.txt"); err != nil { + t.Fatalf("seed path: %v", err) + } + ws.run = func(_ context.Context, _ string, args ...string) ([]byte, error) { + joined := strings.Join(args, " ") + switch { + case strings.Contains(joined, "worktree list --porcelain"): + // Residue is NOT registered. + return []byte("worktree " + repo + "\nbranch refs/heads/main\n"), nil + case strings.Contains(joined, "check-ref-format"): + return nil, nil + case strings.Contains(joined, "remote get-url origin"): + return nil, errors.New("no origin") + case strings.Contains(joined, "rev-parse --verify --quiet refs/heads/feature/one"): + return nil, &exec.ExitError{ProcessState: stubExitState(1)} + case strings.Contains(joined, "rev-parse --verify --quiet"): + return []byte("abc\n"), nil + case strings.Contains(joined, "worktree add"): + return nil, nil + default: + return nil, nil + } + } + info, err := ws.Restore(context.Background(), ports.WorkspaceConfig{ProjectID: "proj", SessionID: "sess", Branch: "feature/one"}) + if err != nil { + t.Fatalf("restore: %v", err) + } + if info.Path != path { + t.Fatalf("info.Path = %q, want %q", info.Path, path) + } + if _, statErr := os.Stat(filepath.Join(path, "leftover.txt")); !os.IsNotExist(statErr) { + t.Fatalf("expected leftover.txt to be removed, stat err = %v", statErr) + } +} + +func TestRestoreStillRefusesRegisteredResidue(t *testing.T) { + // Safety invariant: even if path is non-empty, if it IS a registered + // worktree we MUST NOT rmSync it (would destroy registered work). + // The findWorktree shortcut should catch this and return the registered + // info, never reaching the residue branch. + root := t.TempDir() + repo := t.TempDir() + ws, err := New(Options{ManagedRoot: root, RepoResolver: StaticRepoResolver{"proj": repo}}) + if err != nil { + t.Fatalf("new: %v", err) } path := filepath.Join(ws.managedRoot, "proj", "sess") - if err := mkdirFile(path, "keep.txt"); err != nil { + if err := mkdirFile(path, "agent-work.txt"); err != nil { t.Fatalf("seed path: %v", err) } - _, err = ws.Restore(context.Background(), ports.WorkspaceConfig{ProjectID: "proj", SessionID: "sess", Branch: "feature/one"}) - if err == nil || !strings.Contains(err.Error(), "path exists and is not a registered worktree") { - t.Fatalf("restore error = %v", err) + ws.run = func(context.Context, string, ...string) ([]byte, error) { + // Path IS registered — findWorktree shortcut should fire. + return []byte("worktree " + path + "\nbranch refs/heads/feature/one\n"), nil + } + info, err := ws.Restore(context.Background(), ports.WorkspaceConfig{ProjectID: "proj", SessionID: "sess", Branch: "feature/one"}) + if err != nil { + t.Fatalf("restore registered: %v", err) + } + if info.Branch != "feature/one" { + t.Fatalf("info.Branch = %q, want feature/one", info.Branch) + } + if _, statErr := os.Stat(filepath.Join(path, "agent-work.txt")); statErr != nil { + t.Fatalf("agent-work.txt MUST be preserved (data-loss guard): %v", statErr) } } @@ -234,3 +356,308 @@ func mkdirFile(dir, name string) error { } return os.WriteFile(filepath.Join(dir, name), []byte("data"), 0o644) } + +func containsArgs(calls [][]string, sub []string) bool { + return indexOfArgs(calls, sub) >= 0 +} + +func indexOfArgs(calls [][]string, sub []string) int { + for i, c := range calls { + joined := strings.Join(c, " ") + if strings.Contains(joined, strings.Join(sub, " ")) { + return i + } + } + return -1 +} + +// stubExitState spawns a tiny shell command that exits with the requested +// code so the returned *os.ProcessState carries a real exit code — avoids +// constructing os/exec internals. Depends on a POSIX `sh` in PATH; if/when +// Windows CI is added this needs a platform branch. +func stubExitState(code int) *os.ProcessState { + cmd := exec.Command("sh", "-c", fmt.Sprintf("exit %d", code)) + _ = cmd.Run() + return cmd.ProcessState +} + +func TestCreateFetchesOriginWhenPresent(t *testing.T) { + ws, err := New(Options{ManagedRoot: t.TempDir(), RepoResolver: StaticRepoResolver{"proj": "/repo"}}) + if err != nil { + t.Fatalf("new: %v", err) + } + var calls [][]string + ws.run = func(_ context.Context, _ string, args ...string) ([]byte, error) { + calls = append(calls, append([]string{}, args...)) + joined := strings.Join(args, " ") + switch { + case strings.Contains(joined, "check-ref-format"): + return nil, nil + case strings.Contains(joined, "remote get-url origin"): + return []byte("origin\n"), nil + case strings.Contains(joined, "fetch origin --quiet"): + return nil, nil + case strings.Contains(joined, "rev-parse --verify --quiet refs/heads/feature/one"): + return nil, &exec.ExitError{ProcessState: stubExitState(1)} + case strings.Contains(joined, "rev-parse --verify --quiet"): + return []byte("abc\n"), nil + case strings.Contains(joined, "worktree add"): + return nil, nil + default: + return nil, nil + } + } + _, err = ws.Create(context.Background(), ports.WorkspaceConfig{ProjectID: "proj", SessionID: "sess", Branch: "feature/one"}) + if err != nil { + t.Fatalf("create: %v", err) + } + fetchIdx := indexOfArgs(calls, []string{"fetch", "origin", "--quiet"}) + addIdx := indexOfArgs(calls, []string{"worktree", "add"}) + if fetchIdx < 0 { + t.Fatalf("expected git fetch origin --quiet call; calls=%v", calls) + } + if addIdx < 0 { + t.Fatalf("expected git worktree add call; calls=%v", calls) + } + if fetchIdx > addIdx { + t.Fatalf("expected fetch before worktree add: fetchIdx=%d addIdx=%d", fetchIdx, addIdx) + } +} + +func TestCreateSkipsFetchWhenNoOrigin(t *testing.T) { + ws, err := New(Options{ManagedRoot: t.TempDir(), RepoResolver: StaticRepoResolver{"proj": "/repo"}}) + if err != nil { + t.Fatalf("new: %v", err) + } + var calls [][]string + ws.run = func(_ context.Context, _ string, args ...string) ([]byte, error) { + calls = append(calls, append([]string{}, args...)) + joined := strings.Join(args, " ") + switch { + case strings.Contains(joined, "check-ref-format"): + return nil, nil + case strings.Contains(joined, "remote get-url origin"): + return nil, errors.New("fatal: no such remote 'origin'") + case strings.Contains(joined, "rev-parse --verify --quiet refs/heads/feature/one"): + return nil, &exec.ExitError{ProcessState: stubExitState(1)} + case strings.Contains(joined, "rev-parse --verify --quiet"): + return []byte("abc\n"), nil + case strings.Contains(joined, "worktree add"): + return nil, nil + default: + return nil, nil + } + } + _, err = ws.Create(context.Background(), ports.WorkspaceConfig{ProjectID: "proj", SessionID: "sess", Branch: "feature/one"}) + if err != nil { + t.Fatalf("create: %v", err) + } + if containsArgs(calls, []string{"fetch", "origin", "--quiet"}) { + t.Fatalf("expected no fetch when origin absent; calls=%v", calls) + } +} + +func TestCreateContinuesWhenFetchFails(t *testing.T) { + ws, err := New(Options{ManagedRoot: t.TempDir(), RepoResolver: StaticRepoResolver{"proj": "/repo"}}) + if err != nil { + t.Fatalf("new: %v", err) + } + ws.run = func(_ context.Context, _ string, args ...string) ([]byte, error) { + joined := strings.Join(args, " ") + switch { + case strings.Contains(joined, "check-ref-format"): + return nil, nil + case strings.Contains(joined, "remote get-url origin"): + return []byte("origin\n"), nil + case strings.Contains(joined, "fetch origin --quiet"): + return nil, errors.New("could not resolve host") + case strings.Contains(joined, "rev-parse --verify --quiet refs/heads/feature/one"): + return nil, &exec.ExitError{ProcessState: stubExitState(1)} + case strings.Contains(joined, "rev-parse --verify --quiet"): + return []byte("abc\n"), nil + case strings.Contains(joined, "worktree add"): + return nil, nil + default: + return nil, nil + } + } + if _, err := ws.Create(context.Background(), ports.WorkspaceConfig{ProjectID: "proj", SessionID: "sess", Branch: "feature/one"}); err != nil { + t.Fatalf("create with failing fetch: %v", err) + } +} + +func TestCreateCleansUpOrphanWhenAddFails(t *testing.T) { + root := t.TempDir() + repo := t.TempDir() + ws, err := New(Options{ManagedRoot: root, RepoResolver: StaticRepoResolver{"proj": repo}}) + if err != nil { + t.Fatalf("new: %v", err) + } + var calls [][]string + ws.run = func(_ context.Context, _ string, args ...string) ([]byte, error) { + calls = append(calls, append([]string{}, args...)) + joined := strings.Join(args, " ") + switch { + case strings.Contains(joined, "check-ref-format"): + return nil, nil + case strings.Contains(joined, "remote get-url origin"): + return nil, errors.New("no origin") + case strings.Contains(joined, "rev-parse --verify --quiet refs/heads/feature/one"): + return nil, &exec.ExitError{ProcessState: stubExitState(1)} + case strings.Contains(joined, "rev-parse --verify --quiet"): + return []byte("abc\n"), nil + case strings.Contains(joined, "worktree add"): + return nil, errors.New("fatal: already exists") + case strings.Contains(joined, "worktree remove --force"): + return nil, nil + default: + return nil, nil + } + } + _, err = ws.Create(context.Background(), ports.WorkspaceConfig{ProjectID: "proj", SessionID: "sess", Branch: "feature/one"}) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "worktree add") { + t.Fatalf("error should mention the add operation; got: %v", err) + } + if indexOfArgs(calls, []string{"worktree", "remove", "--force"}) < 0 { + t.Fatalf("expected worktree remove --force cleanup; calls=%v", calls) + } +} + +func TestCreateCleanupSwallowsSecondaryError(t *testing.T) { + root := t.TempDir() + repo := t.TempDir() + ws, err := New(Options{ManagedRoot: root, RepoResolver: StaticRepoResolver{"proj": repo}}) + if err != nil { + t.Fatalf("new: %v", err) + } + ws.run = func(_ context.Context, _ string, args ...string) ([]byte, error) { + joined := strings.Join(args, " ") + switch { + case strings.Contains(joined, "check-ref-format"): + return nil, nil + case strings.Contains(joined, "remote get-url origin"): + return nil, errors.New("no origin") + case strings.Contains(joined, "rev-parse --verify --quiet refs/heads/feature/one"): + return nil, &exec.ExitError{ProcessState: stubExitState(1)} + case strings.Contains(joined, "rev-parse --verify --quiet"): + return []byte("abc\n"), nil + case strings.Contains(joined, "worktree add"): + return nil, errors.New("fatal: add failed") + case strings.Contains(joined, "worktree remove --force"): + return nil, errors.New("fatal: cleanup also failed") + default: + return nil, nil + } + } + _, err = ws.Create(context.Background(), ports.WorkspaceConfig{ProjectID: "proj", SessionID: "sess", Branch: "feature/one"}) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "add failed") { + t.Fatalf("original add error should surface; got: %v", err) + } +} + +func TestCreateCleanupRunsWithFreshContextWhenCallerCancelled(t *testing.T) { + // When the caller's ctx is cancelled mid-add (e.g. HTTP request + // aborted), cleanupOrphan MUST run with a fresh context. Reusing the + // cancelled caller ctx — directly or via withDefaultTimeout — yields + // an already-cancelled child, the `worktree remove --force` subprocess + // is killed before it runs, and the orphan that cleanupOrphan exists + // to prevent is left on disk. + root := t.TempDir() + repo := t.TempDir() + ws, err := New(Options{ManagedRoot: root, RepoResolver: StaticRepoResolver{"proj": repo}}) + if err != nil { + t.Fatalf("new: %v", err) + } + var ( + cleanupCalled bool + cleanupCtxErr error + ) + ws.run = func(callerCtx context.Context, _ string, args ...string) ([]byte, error) { + joined := strings.Join(args, " ") + switch { + case strings.Contains(joined, "check-ref-format"): + return nil, nil + case strings.Contains(joined, "remote get-url origin"): + return nil, errors.New("no origin") + case strings.Contains(joined, "rev-parse --verify --quiet refs/heads/feature/one"): + return nil, &exec.ExitError{ProcessState: stubExitState(1)} + case strings.Contains(joined, "rev-parse --verify --quiet"): + return []byte("abc\n"), nil + case strings.Contains(joined, "worktree add"): + return nil, errors.New("fatal: add failed") + case strings.Contains(joined, "worktree remove --force"): + cleanupCalled = true + cleanupCtxErr = callerCtx.Err() + return nil, nil + default: + return nil, nil + } + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() + _, _ = ws.Create(ctx, ports.WorkspaceConfig{ProjectID: "proj", SessionID: "sess", Branch: "feature/one"}) + if !cleanupCalled { + t.Fatal("cleanup must run even when caller ctx is cancelled") + } + if cleanupCtxErr != nil { + t.Fatalf("cleanup ctx must be fresh, got Err()=%v", cleanupCtxErr) + } +} + +func TestCreateCleansUpOrphanWhenAddFailsExistingBranch(t *testing.T) { + // Covers the addWorktree localBranch=true path: refs/heads/ exists, + // so we skip resolveBaseRef and call `worktree add ` (no -b). + // If that fails, cleanupOrphan must still fire. + root := t.TempDir() + repo := t.TempDir() + ws, err := New(Options{ManagedRoot: root, RepoResolver: StaticRepoResolver{"proj": repo}}) + if err != nil { + t.Fatalf("new: %v", err) + } + var calls [][]string + ws.run = func(_ context.Context, _ string, args ...string) ([]byte, error) { + calls = append(calls, append([]string{}, args...)) + joined := strings.Join(args, " ") + switch { + case strings.Contains(joined, "check-ref-format"): + return nil, nil + case strings.Contains(joined, "remote get-url origin"): + return nil, errors.New("no origin") + case strings.Contains(joined, "rev-parse --verify --quiet refs/heads/feature/one"): + // Local branch DOES exist — drives addWorktree into the + // existing-branch path (worktree add , no -b). + return []byte("abc\n"), nil + case strings.Contains(joined, "worktree add"): + return nil, errors.New("fatal: already exists") + case strings.Contains(joined, "worktree remove --force"): + return nil, nil + default: + return nil, nil + } + } + _, err = ws.Create(context.Background(), ports.WorkspaceConfig{ProjectID: "proj", SessionID: "sess", Branch: "feature/one"}) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "worktree add existing branch") { + t.Fatalf("error should mention existing-branch add; got: %v", err) + } + if indexOfArgs(calls, []string{"worktree", "remove", "--force"}) < 0 { + t.Fatalf("expected worktree remove --force cleanup; calls=%v", calls) + } + addIdx := indexOfArgs(calls, []string{"worktree", "add"}) + if addIdx < 0 { + t.Fatal("no worktree add call recorded") + } + for _, a := range calls[addIdx] { + if a == "-b" { + t.Fatalf("localBranch=true path must not pass -b; calls[%d]=%v", addIdx, calls[addIdx]) + } + } +}