From 6c615736e30ace89066326e21e1f0ce0731959a8 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Tue, 2 Jun 2026 14:27:35 +0530 Subject: [PATCH 1/6] feat(gitworktree): apply default 30s timeout to git subprocesses Defense-in-depth so a hung `git fetch` or `worktree add` against an unreachable remote can't block indefinitely when the caller passes `context.Background()`. Caller-supplied deadlines are preserved. Co-Authored-By: Claude Opus 4.7 --- .../workspace/gitworktree/workspace.go | 20 +++++++++++++-- .../workspace/gitworktree/workspace_test.go | 25 +++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/backend/internal/adapters/workspace/gitworktree/workspace.go b/backend/internal/adapters/workspace/gitworktree/workspace.go index a2a9cd97..75ff88a5 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 @@ -393,7 +395,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_test.go b/backend/internal/adapters/workspace/gitworktree/workspace_test.go index fa14f527..f5706a45 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace_test.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace_test.go @@ -8,10 +8,35 @@ import ( "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" From 577ad9cd54cf2bb6d8952a15041502bbbb601d45 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Tue, 2 Jun 2026 14:29:02 +0530 Subject: [PATCH 2/6] feat(gitworktree): origin-aware base-ref resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Probe `git remote get-url origin` once per resolve and thread the bool into baseRefCandidates. When origin is absent, the candidate list collapses to local refs only — no wasted `rev-parse origin/...` calls. Preserves the existing qualified-defaultBranch behavior (e.g. "upstream/main" is used as-is rather than re-prefixed with origin/). Co-Authored-By: Claude Opus 4.7 --- .../workspace/gitworktree/commands.go | 24 +++++++--- .../workspace/gitworktree/workspace.go | 14 +++++- .../workspace/gitworktree/workspace_test.go | 45 +++++++++++++++++-- 3 files changed, 74 insertions(+), 9 deletions(-) diff --git a/backend/internal/adapters/workspace/gitworktree/commands.go b/backend/internal/adapters/workspace/gitworktree/commands.go index 356a50e1..196104b2 100644 --- a/backend/internal/adapters/workspace/gitworktree/commands.go +++ b/backend/internal/adapters/workspace/gitworktree/commands.go @@ -35,12 +35,26 @@ 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"} +} + +// 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 75ff88a5..2019b772 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace.go @@ -225,7 +225,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 { @@ -238,6 +238,18 @@ 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 +} + func (w *Workspace) refExists(ctx context.Context, repo, ref string) (bool, error) { _, err := w.run(ctx, w.binary, revParseVerifyArgs(repo, ref)...) if err == nil { diff --git a/backend/internal/adapters/workspace/gitworktree/workspace_test.go b/backend/internal/adapters/workspace/gitworktree/workspace_test.go index f5706a45..c40157bf 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace_test.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace_test.go @@ -57,6 +57,7 @@ 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"}}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -67,20 +68,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", From b7ac7b2fe9fd7f54f53751c41b455a0f3b3a7ee4 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Tue, 2 Jun 2026 14:31:07 +0530 Subject: [PATCH 3/6] feat(gitworktree): fetch origin before worktree creation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Create and Restore now run `git fetch origin --quiet` (swallowed on failure) before resolving the base ref, so new sessions start from current remote tips instead of whatever the working clone last fetched. Gated on `hasOriginRemote` to preserve no-origin and offline flows. Integration test pushes a commit directly to origin via a separate clone and asserts the file appears in the new worktree — proves the fetch actually ran end-to-end. The pusher clone explicitly checks out main to handle CI runners where init.defaultBranch is master. Co-Authored-By: Claude Opus 4.7 --- .../workspace/gitworktree/commands.go | 4 + .../workspace/gitworktree/workspace.go | 20 +++ .../gitworktree/workspace_integration_test.go | 39 ++++++ .../workspace/gitworktree/workspace_test.go | 132 ++++++++++++++++++ 4 files changed, 195 insertions(+) diff --git a/backend/internal/adapters/workspace/gitworktree/commands.go b/backend/internal/adapters/workspace/gitworktree/commands.go index 196104b2..864f0720 100644 --- a/backend/internal/adapters/workspace/gitworktree/commands.go +++ b/backend/internal/adapters/workspace/gitworktree/commands.go @@ -39,6 +39,10 @@ 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 diff --git a/backend/internal/adapters/workspace/gitworktree/workspace.go b/backend/internal/adapters/workspace/gitworktree/workspace.go index 2019b772..108f25b3 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace.go @@ -110,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 @@ -190,6 +195,11 @@ func (w *Workspace) Restore(ctx context.Context, cfg ports.WorkspaceConfig) (por 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 } @@ -250,6 +260,16 @@ func (w *Workspace) hasOriginRemote(ctx context.Context, repo string) bool { 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 { 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 c40157bf..18c1044e 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace_test.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace_test.go @@ -3,7 +3,9 @@ package gitworktree import ( "context" "errors" + "fmt" "os" + "os/exec" "path/filepath" "reflect" "strings" @@ -58,6 +60,7 @@ func TestCommandArgs(t *testing.T) { {"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"}}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -298,3 +301,132 @@ 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) + } +} From 37fde506fe2e09407b758e03007d99e5f8165883 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Tue, 2 Jun 2026 14:32:20 +0530 Subject: [PATCH 4/6] feat(gitworktree): clean up orphan worktree on Create failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `git worktree add` fails mid-creation, best-effort `git worktree remove --force ` clears any orphan registration or directory the failed add may have left. Cleanup errors are swallowed — the original add failure is the actionable signal. `--force` is safe here because we just attempted creation; no agent has had a chance to commit. Distinct from Destroy's no-force semantics which protect in-progress agent work — enforced via a separate `worktreeRemoveForceArgs` helper, with the existing Destroy no-force regression test still gating against accidental use. Co-Authored-By: Claude Opus 4.7 --- .../workspace/gitworktree/commands.go | 10 ++ .../workspace/gitworktree/workspace.go | 10 ++ .../workspace/gitworktree/workspace_test.go | 128 ++++++++++++++++++ 3 files changed, 148 insertions(+) diff --git a/backend/internal/adapters/workspace/gitworktree/commands.go b/backend/internal/adapters/workspace/gitworktree/commands.go index 864f0720..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"} } diff --git a/backend/internal/adapters/workspace/gitworktree/workspace.go b/backend/internal/adapters/workspace/gitworktree/workspace.go index 108f25b3..5086cfda 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace.go @@ -213,6 +213,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 @@ -222,11 +223,20 @@ 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. +func (w *Workspace) cleanupOrphan(ctx context.Context, repo, path string) { + _, _ = 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) diff --git a/backend/internal/adapters/workspace/gitworktree/workspace_test.go b/backend/internal/adapters/workspace/gitworktree/workspace_test.go index 18c1044e..2cbd9527 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace_test.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace_test.go @@ -61,6 +61,7 @@ func TestCommandArgs(t *testing.T) { {"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) { @@ -430,3 +431,130 @@ func TestCreateContinuesWhenFetchFails(t *testing.T) { 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 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]) + } + } +} From 442ffb3b4043181a162cf1637f49c91b54627aab Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Tue, 2 Jun 2026 14:33:06 +0530 Subject: [PATCH 5/6] feat(gitworktree): auto-clean unregistered residue on Restore Mirrors upstream's cleanupStaleWorkspacePath: when the workspace path exists but isn't a registered worktree, rmSync the residue and proceed with `git worktree add`. The data-loss safety invariant ("never delete a registered worktree") is preserved by the findWorktree shortcut at the top of Restore, which returns the adopted worktree before this branch is reached. Regression test TestRestoreStillRefusesRegisteredResidue locks the invariant in place. Closes the Restore self-healing gap from the upstream parity audit. Co-Authored-By: Claude Opus 4.7 --- .../workspace/gitworktree/workspace.go | 10 ++- .../workspace/gitworktree/workspace_test.go | 68 +++++++++++++++++-- 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/backend/internal/adapters/workspace/gitworktree/workspace.go b/backend/internal/adapters/workspace/gitworktree/workspace.go index 5086cfda..6349994b 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace.go @@ -190,7 +190,15 @@ 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 diff --git a/backend/internal/adapters/workspace/gitworktree/workspace_test.go b/backend/internal/adapters/workspace/gitworktree/workspace_test.go index 2cbd9527..36ae4461 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace_test.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace_test.go @@ -233,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) } } From d354c202fd5906657abf00a09774acdc1a4e1072 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Tue, 2 Jun 2026 14:41:37 +0530 Subject: [PATCH 6/6] fix(gitworktree): cleanupOrphan uses fresh context (greptile P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the caller's ctx is cancelled mid-`worktree add` (e.g. HTTP request aborted), reusing the same ctx for cleanup means `context.WithTimeout(cancelledCtx, _)` returns an already-cancelled child and the `worktree remove --force` subprocess is killed before it runs — leaving the orphan that cleanupOrphan exists to prevent. Use a fresh background context with its own timeout for the cleanup. Regression test TestCreateCleanupRunsWithFreshContextWhenCallerCancelled pre-cancels the caller ctx, runs Create, and asserts the cleanup subprocess actually executed with an uncancelled context. Co-Authored-By: Claude Opus 4.7 --- .../workspace/gitworktree/workspace.go | 7 ++- .../workspace/gitworktree/workspace_test.go | 49 +++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/backend/internal/adapters/workspace/gitworktree/workspace.go b/backend/internal/adapters/workspace/gitworktree/workspace.go index 6349994b..8db87f24 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace.go @@ -241,7 +241,12 @@ func (w *Workspace) addWorktree(ctx context.Context, repo, path, branch string) // `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. -func (w *Workspace) cleanupOrphan(ctx context.Context, repo, path string) { +// 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)...) } diff --git a/backend/internal/adapters/workspace/gitworktree/workspace_test.go b/backend/internal/adapters/workspace/gitworktree/workspace_test.go index 36ae4461..4992c16a 100644 --- a/backend/internal/adapters/workspace/gitworktree/workspace_test.go +++ b/backend/internal/adapters/workspace/gitworktree/workspace_test.go @@ -561,6 +561,55 @@ func TestCreateCleanupSwallowsSecondaryError(t *testing.T) { } } +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).