diff --git a/.changeset/restore-preserves-existing-branch.md b/.changeset/restore-preserves-existing-branch.md new file mode 100644 index 000000000..784d776fe --- /dev/null +++ b/.changeset/restore-preserves-existing-branch.md @@ -0,0 +1,5 @@ +--- +"@aoagents/ao-plugin-workspace-worktree": patch +--- + +Restoring a session whose worktree directory was cleaned up but whose branch still existed locally would 422 with `fatal: a branch named already exists`. The recovery path in `workspace.restore()` unconditionally fell through to `git worktree add -b`, even when the local branch was present (which `destroy()` deliberately preserves). The catch now checks for the local branch and re-attaches it without `-b`/`-B`, preserving the session's commits. (#1741) diff --git a/packages/integration-tests/src/workspace-worktree.integration.test.ts b/packages/integration-tests/src/workspace-worktree.integration.test.ts index 4e508892c..6f208cbb7 100644 --- a/packages/integration-tests/src/workspace-worktree.integration.test.ts +++ b/packages/integration-tests/src/workspace-worktree.integration.test.ts @@ -206,6 +206,276 @@ describe("workspace-worktree (integration)", () => { } }, 30_000); + // Regression for https://github.com/ComposioHQ/agent-orchestrator/issues/1741. + // After a clean destroy(), the local session branch is intentionally kept so + // the user's commits aren't lost. restore() must re-attach that branch + // without recreating it (-b) or force-resetting it (-B), so the session's + // HEAD survives. + it("restore re-attaches existing session branch and preserves its commits", async () => { + const { bareDir, cloneParent, repoDir: isolatedRepoDir } = await createRepoClone(); + const rawBase = await mkdtemp(join(tmpdir(), "ao-inttest-wt-restore-preserve-")); + const isolatedWorktreeBaseDir = await realpath(rawBase); + + try { + await git(isolatedRepoDir, "switch", "-c", "main"); + const mainSha = await createCommit(isolatedRepoDir, "base.txt", "main\n"); + await git(isolatedRepoDir, "push", "-u", "origin", "main"); + + const isolatedWorkspace = worktreePlugin.create({ worktreeDir: isolatedWorktreeBaseDir }); + const proj: ProjectConfig = { + name: "restore-preserve", + repo: "test/restore-preserve", + path: isolatedRepoDir, + defaultBranch: "main", + sessionPrefix: "ao", + }; + + const created = await isolatedWorkspace.create({ + projectId: "restore-preserve", + sessionId: "ao-1", + project: proj, + branch: "session/ao-1", + }); + + // Simulate session work with a commit on the session branch. + await git(created.path, "config", "user.email", "test@test.com"); + await git(created.path, "config", "user.name", "Test"); + const sessionSha = await createCommit(created.path, "session.txt", "session work\n"); + expect(sessionSha).not.toBe(mainSha); + + // Tear down the worktree the way AO does — branch is preserved. + await isolatedWorkspace.destroy(created.path); + expect(existsSync(created.path)).toBe(false); + expect(await git(isolatedRepoDir, "rev-parse", "refs/heads/session/ao-1")).toBe(sessionSha); + + // Restore — must re-attach session/ao-1 with its existing HEAD intact. + const restored = await isolatedWorkspace.restore!( + { + projectId: "restore-preserve", + sessionId: "ao-1", + project: proj, + branch: "session/ao-1", + }, + created.path, + ); + + expect(restored.branch).toBe("session/ao-1"); + expect(await git(restored.path, "rev-parse", "HEAD")).toBe(sessionSha); + expect(await git(isolatedRepoDir, "rev-parse", "refs/heads/session/ao-1")).toBe(sessionSha); + } finally { + await rm(isolatedWorktreeBaseDir, { recursive: true, force: true }).catch(() => {}); + await rm(cloneParent, { recursive: true, force: true }).catch(() => {}); + await rm(bareDir, { recursive: true, force: true }).catch(() => {}); + } + }, 30_000); + + // Same regression — direct repro of the failure surface in #1741. We force + // the first `git worktree add ` to fail by leaving a stale + // registered worktree at the same path, then verify restore recovers + // without using -b (which would fail with "branch already exists") or -B + // (which would discard the session's commits). + it("restore recovers when a stale worktree registration conflicts with the path", async () => { + const { bareDir, cloneParent, repoDir: isolatedRepoDir } = await createRepoClone(); + const rawBase = await mkdtemp(join(tmpdir(), "ao-inttest-wt-restore-stale-")); + const isolatedWorktreeBaseDir = await realpath(rawBase); + + try { + await git(isolatedRepoDir, "switch", "-c", "main"); + const mainSha = await createCommit(isolatedRepoDir, "base.txt", "main\n"); + await git(isolatedRepoDir, "push", "-u", "origin", "main"); + + const isolatedWorkspace = worktreePlugin.create({ worktreeDir: isolatedWorktreeBaseDir }); + const proj: ProjectConfig = { + name: "restore-stale", + repo: "test/restore-stale", + path: isolatedRepoDir, + defaultBranch: "main", + sessionPrefix: "ao", + }; + + const created = await isolatedWorkspace.create({ + projectId: "restore-stale", + sessionId: "ao-1", + project: proj, + branch: "session/ao-1", + }); + + await git(created.path, "config", "user.email", "test@test.com"); + await git(created.path, "config", "user.name", "Test"); + const sessionSha = await createCommit(created.path, "session.txt", "session work\n"); + expect(sessionSha).not.toBe(mainSha); + + // Simulate a dirty teardown: rmSync the dir but leave the worktree + // entry registered (this is the failure mode from #1562 that triggers + // the buggy fallback path in #1741). + await rm(created.path, { recursive: true, force: true }); + // Worktree registration is still present — branch is still considered + // checked out at that (now-missing) path. Restore must handle this. + + const restored = await isolatedWorkspace.restore!( + { + projectId: "restore-stale", + sessionId: "ao-1", + project: proj, + branch: "session/ao-1", + }, + created.path, + ); + + expect(restored.branch).toBe("session/ao-1"); + // Most importantly, the session commit must survive — anything that + // touched -B would have reset the branch back to mainSha. + expect(await git(restored.path, "rev-parse", "HEAD")).toBe(sessionSha); + expect(await git(isolatedRepoDir, "rev-parse", "refs/heads/session/ao-1")).toBe(sessionSha); + } finally { + await rm(isolatedWorktreeBaseDir, { recursive: true, force: true }).catch(() => {}); + await rm(cloneParent, { recursive: true, force: true }).catch(() => {}); + await rm(bareDir, { recursive: true, force: true }).catch(() => {}); + } + }, 30_000); + + // Direct repro of the user-reported failure on PR #1742: the workspace dir + // physically exists on disk but is no longer a valid git working tree + // (workspace.exists() returned false because rev-parse failed). The first + // `git worktree add ` fails with `'' already exists`, + // so restore must rmSync the stale dir before retrying. Without this, my + // first fix attempt cleaned the registry but left the dir, so the retry + // failed identically. + it("restore recovers when a stale (non-worktree) directory exists at the path", async () => { + const { bareDir, cloneParent, repoDir: isolatedRepoDir } = await createRepoClone(); + const rawBase = await mkdtemp(join(tmpdir(), "ao-inttest-wt-restore-junkdir-")); + const isolatedWorktreeBaseDir = await realpath(rawBase); + + try { + await git(isolatedRepoDir, "switch", "-c", "main"); + const mainSha = await createCommit(isolatedRepoDir, "base.txt", "main\n"); + await git(isolatedRepoDir, "push", "-u", "origin", "main"); + + const isolatedWorkspace = worktreePlugin.create({ worktreeDir: isolatedWorktreeBaseDir }); + const proj: ProjectConfig = { + name: "restore-junkdir", + repo: "test/restore-junkdir", + path: isolatedRepoDir, + defaultBranch: "main", + sessionPrefix: "ao", + }; + + const created = await isolatedWorkspace.create({ + projectId: "restore-junkdir", + sessionId: "ao-1", + project: proj, + branch: "session/ao-1", + }); + + await git(created.path, "config", "user.email", "test@test.com"); + await git(created.path, "config", "user.name", "Test"); + const sessionSha = await createCommit(created.path, "session.txt", "session work\n"); + expect(sessionSha).not.toBe(mainSha); + + // Clean teardown — registry and dir both gone, branch preserved. + await isolatedWorkspace.destroy(created.path); + expect(existsSync(created.path)).toBe(false); + + // Now simulate a partially-restored or hand-mucked state: the dir + // exists at workspacePath but is just leftover files, not a working + // tree. workspace.exists() will return false (rev-parse fails), so + // restore is invoked, and its first `worktree add` will fail with + // `'' already exists`. + await execFileAsync("mkdir", ["-p", created.path]); + await writeFile(join(created.path, "stale.txt"), "junk\n"); + expect(existsSync(created.path)).toBe(true); + + const restored = await isolatedWorkspace.restore!( + { + projectId: "restore-junkdir", + sessionId: "ao-1", + project: proj, + branch: "session/ao-1", + }, + created.path, + ); + + expect(restored.branch).toBe("session/ao-1"); + // Junk file must be gone (restore rmSync'd the stale dir before retry). + expect(existsSync(join(created.path, "stale.txt"))).toBe(false); + // Session commit must survive — anything using -B would have lost it. + expect(await git(restored.path, "rev-parse", "HEAD")).toBe(sessionSha); + expect(await git(isolatedRepoDir, "rev-parse", "refs/heads/session/ao-1")).toBe(sessionSha); + } finally { + await rm(isolatedWorktreeBaseDir, { recursive: true, force: true }).catch(() => {}); + await rm(cloneParent, { recursive: true, force: true }).catch(() => {}); + await rm(bareDir, { recursive: true, force: true }).catch(() => {}); + } + }, 30_000); + + // Coverage for the createBranchFromBase recovery path (Copilot review on + // PR #1742): when the LOCAL branch is missing (only origin/ + // exists) AND `workspacePath` has stale state, the -b fallback must also + // run the cleanup. Without it, `git worktree add -b ...` fails with + // `'' already exists` exactly like the re-attach path used to. + it("restore recovers when local branch is missing and stale dir exists at the path", async () => { + const { bareDir, cloneParent, repoDir: isolatedRepoDir } = await createRepoClone(); + const rawBase = await mkdtemp(join(tmpdir(), "ao-inttest-wt-restore-missing-branch-")); + const isolatedWorktreeBaseDir = await realpath(rawBase); + + try { + await git(isolatedRepoDir, "switch", "-c", "main"); + await createCommit(isolatedRepoDir, "base.txt", "main\n"); + await git(isolatedRepoDir, "push", "-u", "origin", "main"); + + // Manually push a session branch to origin without keeping it locally, + // simulating a session whose local branch was pruned but origin still + // has it (e.g. fetched after a remote-only force-update). + await git(isolatedRepoDir, "switch", "-c", "session/ao-1"); + const sessionSha = await createCommit(isolatedRepoDir, "session.txt", "session work\n"); + await git(isolatedRepoDir, "push", "-u", "origin", "session/ao-1"); + // Switch off session/ao-1 then delete the local branch — only origin has it now. + await git(isolatedRepoDir, "switch", "main"); + await git(isolatedRepoDir, "branch", "-D", "session/ao-1"); + + const isolatedWorkspace = worktreePlugin.create({ worktreeDir: isolatedWorktreeBaseDir }); + const proj: ProjectConfig = { + name: "restore-missing-branch", + repo: "test/restore-missing-branch", + path: isolatedRepoDir, + defaultBranch: "main", + sessionPrefix: "ao", + }; + const workspacePath = join(isolatedWorktreeBaseDir, "restore-missing-branch", "ao-1"); + + // Plant a stale junk directory at workspacePath. workspace.exists() + // will return false (not a working tree) and restore is invoked. + // The first `worktree add` will fail with `'' already exists`, + // refExists for refs/heads/session/ao-1 returns false (we deleted it), + // so createBranchFromBase runs and must clean the stale dir first. + await execFileAsync("mkdir", ["-p", workspacePath]); + await writeFile(join(workspacePath, "stale.txt"), "junk\n"); + expect(existsSync(workspacePath)).toBe(true); + + const restored = await isolatedWorkspace.restore!( + { + projectId: "restore-missing-branch", + sessionId: "ao-1", + project: proj, + branch: "session/ao-1", + }, + workspacePath, + ); + + expect(restored.branch).toBe("session/ao-1"); + // Junk file gone — cleanup ran before -b add. + expect(existsSync(join(workspacePath, "stale.txt"))).toBe(false); + // Local branch was recreated from origin/session/ao-1, preserving the + // session's commit (which only existed remotely before restore). + expect(await git(restored.path, "rev-parse", "HEAD")).toBe(sessionSha); + expect(await git(isolatedRepoDir, "rev-parse", "refs/heads/session/ao-1")).toBe(sessionSha); + } finally { + await rm(isolatedWorktreeBaseDir, { recursive: true, force: true }).catch(() => {}); + await rm(cloneParent, { recursive: true, force: true }).catch(() => {}); + await rm(bareDir, { recursive: true, force: true }).catch(() => {}); + } + }, 30_000); + it("resets a stale session branch when origin default branch advances", async () => { const { bareDir, cloneParent, repoDir: isolatedRepoDir } = await createRepoClone(); const rawBase = await mkdtemp(join(tmpdir(), "ao-inttest-wt-stale-origin-")); diff --git a/packages/plugins/workspace-worktree/src/__tests__/index.test.ts b/packages/plugins/workspace-worktree/src/__tests__/index.test.ts index fa055572b..f4837fd62 100644 --- a/packages/plugins/workspace-worktree/src/__tests__/index.test.ts +++ b/packages/plugins/workspace-worktree/src/__tests__/index.test.ts @@ -713,6 +713,10 @@ describe("workspace.restore()", () => { mockGitSuccess(""); // git worktree prune mockOriginRemote(); mockGitError("fatal: invalid reference"); // git worktree add workspacePath cfg.branch fails + mockGitError("fatal: bad ref"); // refExists(refs/heads/feat/TEST-1) → false (branch missing) + // createBranchFromBase → cleanupStaleWorkspacePath + mockGitSuccess(""); // worktree remove --force (best-effort) + mockExistsSync.mockReturnValueOnce(false); // no leftover dir, skip cleanup mockGitSuccess(""); // git rev-parse --verify --quiet origin/feat/TEST-1 mockGitSuccess(""); // git worktree add -b cfg.branch workspacePath origin/feat/TEST-1 @@ -740,6 +744,10 @@ describe("workspace.restore()", () => { mockGitSuccess(""); // git worktree prune mockGitError("fatal: not a git repository"); // git remote get-url origin fails mockGitError("fatal: invalid reference"); // git worktree add workspacePath cfg.branch fails + mockGitError("fatal: bad ref"); // refExists(refs/heads/feat/TEST-1) → false (branch missing) + // createBranchFromBase → cleanupStaleWorkspacePath + mockGitSuccess(""); // worktree remove --force (best-effort) + mockExistsSync.mockReturnValueOnce(false); // no leftover dir, skip cleanup mockGitSuccess(""); // git rev-parse --verify --quiet refs/heads/main mockGitSuccess(""); // git worktree add -b cfg.branch workspacePath refs/heads/main @@ -765,6 +773,282 @@ describe("workspace.restore()", () => { projectId: "myproject", }); }); + + // --- Regression coverage for #1741 --------------------------------------- + // When the local session branch already exists (destroy() preserves it on + // purpose), restore() must re-attach it instead of falling through to the + // -b path that would either fail ("branch already exists") or discard + // commits. See https://github.com/ComposioHQ/agent-orchestrator/issues/1741. + // + // The recovery sequence (in reattachExistingBranch): + // 1. `git worktree remove --force ` (best-effort: clears registry) + // 2. existsSync() — bail if dir already gone + // 3. `git worktree list --porcelain` (isRegisteredWorktree) + // 4. rmSync() if not still registered (else throw — data safety) + // 5. `git worktree add ` retry (no -b/-B) + // + // The entry-point prune in restore() is sufficient — no second prune in + // the recovery path. + + it("re-attaches existing local branch when stale registry conflicts", async () => { + // Path was registered as a worktree but the dir was already cleaned up. + // worktree remove --force succeeds; the stale-dir cleanup short-circuits + // because existsSync returns false; retry succeeds. + const ws = create(); + + mockGitSuccess(""); // git worktree prune (entry-point) + mockOriginRemote(); + mockGitError("fatal: 'feat/TEST-1' is already checked out"); // first worktree add fails + mockGitSuccess(""); // refExists(refs/heads/feat/TEST-1) → true + mockGitSuccess(""); // worktree remove --force + mockExistsSync.mockReturnValueOnce(false); // no leftover dir, skip cleanup + mockGitSuccess(""); // RETRY: worktree add succeeds + + const info = await ws.restore!(makeCreateConfig(), "/mock-home/.worktrees/myproject/session-1"); + + // The recovery call must re-attach the existing branch — no -b, no -B. + expect(mockExecFileAsync).toHaveBeenLastCalledWith( + "git", + ["worktree", "add", "/mock-home/.worktrees/myproject/session-1", "feat/TEST-1"], + { cwd: "/repo/path", windowsHide: true, timeout: 30_000 }, + ); + + // No -b or -B should ever appear when the branch already exists locally. + const calls = mockExecFileAsync.mock.calls; + for (const [, args] of calls) { + if (Array.isArray(args)) { + expect(args).not.toContain("-b"); + expect(args).not.toContain("-B"); + } + } + + expect(info).toEqual({ + path: "/mock-home/.worktrees/myproject/session-1", + branch: "feat/TEST-1", + sessionId: "session-1", + projectId: "myproject", + }); + }); + + it("rmSyncs a stale workspace directory before retrying worktree add", async () => { + // Direct repro of the user's #1741 follow-on failure: the dir physically + // exists on disk (workspace.exists() returned false because it's not a + // git working tree, just leftover files). worktree add fails with + // " already exists" — recovery must rmSync the stale dir, not loop. + const ws = create(); + + mockGitSuccess(""); // git worktree prune (entry-point) + mockOriginRemote(); + mockGitError( + "fatal: '/mock-home/.worktrees/myproject/session-1' already exists", + ); // first worktree add fails because dir exists + mockGitSuccess(""); // refExists → true + mockGitError("fatal: not a working tree"); // worktree remove --force fails (path not registered) + mockExistsSync.mockReturnValueOnce(true); // dir exists + mockGitSuccess("worktree /some/other\nbranch refs/heads/main"); // worktree list — no entry for our path + // rmSync called (mocked) — no second prune + mockGitSuccess(""); // RETRY: worktree add succeeds + + const info = await ws.restore!(makeCreateConfig(), "/mock-home/.worktrees/myproject/session-1"); + + // The stale dir must have been removed. + expect(mockRmSync).toHaveBeenCalledWith("/mock-home/.worktrees/myproject/session-1", { + recursive: true, + force: true, + }); + + // Retry must be the no-flag form. + expect(mockExecFileAsync).toHaveBeenLastCalledWith( + "git", + ["worktree", "add", "/mock-home/.worktrees/myproject/session-1", "feat/TEST-1"], + { cwd: "/repo/path", windowsHide: true, timeout: 30_000 }, + ); + + expect(info.branch).toBe("feat/TEST-1"); + }); + + it("refuses to rmSync a still-registered worktree dir (data safety)", async () => { + // If after `worktree remove --force` the path is STILL registered, + // something is very wrong. reattachExistingBranch throws rather than + // rmSync a registered worktree (which could destroy the user's work). + // The error must propagate, not be swallowed. + const ws = create(); + + mockGitSuccess(""); // git worktree prune (entry-point) + mockOriginRemote(); + mockGitError("fatal: 'feat/TEST-1' is already checked out"); // first worktree add fails + mockGitSuccess(""); // refExists → true + mockGitError("fatal: cannot remove"); // worktree remove --force fails + mockExistsSync.mockReturnValueOnce(true); // dir exists + // Path is still registered — isRegisteredWorktree returns our path + mockGitSuccess( + "worktree /mock-home/.worktrees/myproject/session-1\nbranch refs/heads/feat/TEST-1", + ); + + await expect( + ws.restore!(makeCreateConfig(), "/mock-home/.worktrees/myproject/session-1"), + ).rejects.toThrow(/already exists and is still registered/); + + // rmSync MUST NOT have been called — we never delete a registered worktree. + expect(mockRmSync).not.toHaveBeenCalled(); + }); + + it("propagates retry error when worktree add fails after cleanup", async () => { + const ws = create(); + + mockGitSuccess(""); // prune (entry-point) + mockOriginRemote(); + mockGitError("fatal: first failure"); // first worktree add fails + mockGitSuccess(""); // refExists → true + mockGitSuccess(""); // worktree remove --force + mockExistsSync.mockReturnValueOnce(false); // no leftover dir, skip cleanup + mockGitError("fatal: persistent failure"); // RETRY also fails + + await expect( + ws.restore!(makeCreateConfig(), "/mock-home/.worktrees/myproject/session-1"), + ).rejects.toThrow(/persistent failure/); + + // Crucially, the failure surface is the underlying git error — NOT a + // misleading "branch already exists" from a -b fallback. + const calls = mockExecFileAsync.mock.calls; + for (const [, args] of calls) { + if (Array.isArray(args)) { + expect(args).not.toContain("-b"); + expect(args).not.toContain("-B"); + } + } + }); + + it("never force-resets an existing branch (preserves session commits)", async () => { + // Defense-in-depth: confirm restore() never uses -B even in the + // recovery path. -B would silently discard the user's commits, + // which is the opposite of what restore must do. + const ws = create(); + + mockGitSuccess(""); // prune (entry-point) + mockOriginRemote(); + mockGitError("fatal: registry conflict"); // first worktree add fails + mockGitSuccess(""); // refExists → true + mockGitSuccess(""); // worktree remove --force + mockExistsSync.mockReturnValueOnce(false); // no leftover dir, skip cleanup + mockGitSuccess(""); // RETRY succeeds + + await ws.restore!(makeCreateConfig(), "/mock-home/.worktrees/myproject/session-1"); + + const calls = mockExecFileAsync.mock.calls; + const dashBigB = calls.filter(([, args]) => Array.isArray(args) && args.includes("-B")); + expect(dashBigB).toHaveLength(0); + }); + + it("checks branch existence with rev-parse --verify --quiet refs/heads/", async () => { + // Lock in the exact ref form used. If someone later refactors refExists or + // forgets the refs/heads/ prefix, this regression test catches it. + const ws = create(); + + mockGitSuccess(""); // prune (entry-point) + mockOriginRemote(); + mockGitError("fatal: first failure"); // first worktree add fails + mockGitSuccess(""); // refExists → true + mockGitSuccess(""); // worktree remove --force + mockExistsSync.mockReturnValueOnce(false); // no leftover dir, skip cleanup + mockGitSuccess(""); // RETRY succeeds + + await ws.restore!(makeCreateConfig(), "/mock-home/.worktrees/myproject/session-1"); + + expect(mockExecFileAsync).toHaveBeenCalledWith( + "git", + ["rev-parse", "--verify", "--quiet", "refs/heads/feat/TEST-1"], + { cwd: "/repo/path", windowsHide: true, timeout: 30_000 }, + ); + }); + + it("matches registered worktree even when workspacePath has trailing slash", async () => { + // Path normalization safety: if `workspacePath` is passed in a non-canonical + // form (trailing slash, ".." segments) and git reports a canonical path, + // strict string equality false-negatives. That would mistakenly rmSync a + // still-registered worktree → DATA LOSS. Both sides must be resolve()d. + const ws = create(); + + mockGitSuccess(""); // entry-point prune + mockOriginRemote(); + mockGitError("fatal: 'feat/TEST-1' is already checked out"); // first worktree add fails + mockGitSuccess(""); // refExists → true + // reattachExistingBranch → cleanupStaleWorkspacePath + mockGitError("fatal: cannot remove"); // worktree remove --force fails + mockExistsSync.mockReturnValueOnce(true); // dir exists + // git reports canonical path (no trailing slash); we call restore with trailing slash + mockGitSuccess( + "worktree /mock-home/.worktrees/myproject/session-1\nbranch refs/heads/feat/TEST-1", + ); + + await expect( + ws.restore!(makeCreateConfig(), "/mock-home/.worktrees/myproject/session-1/"), + ).rejects.toThrow(/already exists and is still registered/); + + // CRITICAL: rmSync MUST NOT have been called — the resolve() normalization + // correctly identified the path as still-registered despite the trailing slash. + expect(mockRmSync).not.toHaveBeenCalled(); + }); + + it("createBranchFromBase also clears stale workspace dir before worktree add -b", async () => { + // Mirror of the re-attach path: when the local branch is MISSING and the + // workspacePath has stale state, createBranchFromBase must also do the + // cleanup. Otherwise `git worktree add -b ...` fails with the same + // " already exists" error the re-attach path was fixed for. + const ws = create(); + + mockGitSuccess(""); // entry-point prune + mockOriginRemote(); + mockGitError( + "fatal: '/mock-home/.worktrees/myproject/session-1' already exists", + ); // first worktree add fails + mockGitError("fatal: bad ref"); // refExists → false (branch missing) + // createBranchFromBase → cleanupStaleWorkspacePath + mockGitError("fatal: not registered"); // worktree remove --force fails + mockExistsSync.mockReturnValueOnce(true); // dir exists as junk + mockGitSuccess("worktree /some/other\nbranch refs/heads/main"); // not registered + // rmSync called (mocked) + mockGitSuccess(""); // resolveBaseRef: rev-parse origin/feat/TEST-1 + mockGitSuccess(""); // worktree add -b ... origin/feat/TEST-1 succeeds + + const info = await ws.restore!(makeCreateConfig(), "/mock-home/.worktrees/myproject/session-1"); + + // Stale dir must have been removed before -b add. + expect(mockRmSync).toHaveBeenCalledWith("/mock-home/.worktrees/myproject/session-1", { + recursive: true, + force: true, + }); + + expect(info.branch).toBe("feat/TEST-1"); + expect(mockExecFileAsync).toHaveBeenLastCalledWith( + "git", + [ + "worktree", + "add", + "-b", + "feat/TEST-1", + "/mock-home/.worktrees/myproject/session-1", + "origin/feat/TEST-1", + ], + { cwd: "/repo/path", windowsHide: true, timeout: 30_000 }, + ); + }); + + it("happy path: restore re-attaches branch when first worktree add already succeeds", async () => { + // No catch path — the first attempt works. Confirms we don't accidentally + // run the cleanup/retry sequence in the common case. + const ws = create(); + + mockGitSuccess(""); // prune (entry-point) + mockOriginRemote(); + mockGitSuccess(""); // worktree add succeeds first try + + const info = await ws.restore!(makeCreateConfig(), "/mock-home/.worktrees/myproject/session-1"); + + expect(info.branch).toBe("feat/TEST-1"); + // Total calls: prune + remote get-url + fetch + worktree add = 4 + expect(mockExecFileAsync).toHaveBeenCalledTimes(4); + }); }); describe("workspace.destroy()", () => { diff --git a/packages/plugins/workspace-worktree/src/index.ts b/packages/plugins/workspace-worktree/src/index.ts index 31b3fa43d..7e0b6f61e 100644 --- a/packages/plugins/workspace-worktree/src/index.ts +++ b/packages/plugins/workspace-worktree/src/index.ts @@ -135,13 +135,17 @@ async function resolveBaseRef( async function isRegisteredWorktree(repoPath: string, worktreePath: string): Promise { try { const output = await git(repoPath, "worktree", "list", "--porcelain"); - const target = toComparablePath(worktreePath); + // Normalize both sides so non-canonical inputs don't false-negative + // and let a subsequent rmSync delete a still-registered worktree + // (data loss). resolve() collapses trailing-slash / ".." segments; + // toComparablePath handles Windows backslashes and drive case. + const target = toComparablePath(resolve(worktreePath)); return output .split("\n") .some( (line) => line.startsWith("worktree ") && - toComparablePath(line.slice("worktree ".length)) === target, + toComparablePath(resolve(line.slice("worktree ".length))) === target, ); } catch { return false; @@ -166,6 +170,105 @@ async function clearStaleWorktreePath(repoPath: string, worktreePath: string): P rmSync(worktreePath, { recursive: true, force: true }); } +/** + * Restore recovery: clear any stale worktree registration and/or stale + * directory at `workspacePath` so a subsequent `git worktree add` can + * succeed. Both restore branches (re-attach existing branch, create from + * base) need this — without it, an ` already exists` failure repeats. + * + * Refuses to rmSync the path if it's still a registered worktree, which + * would silently destroy the user's work. The entry-point `worktree prune` + * in restore() already ran, so we don't prune again here. + */ +async function cleanupStaleWorkspacePath( + repoPath: string, + workspacePath: string, +): Promise { + // Force-remove any registered worktree at this path. Best-effort — the + // path may not be registered, in which case git errors and we fall + // through to the dir cleanup. + try { + await git(repoPath, "worktree", "remove", "--force", workspacePath); + } catch { + // Best-effort + } + + if (existsSync(workspacePath)) { + if (await isRegisteredWorktree(repoPath, workspacePath)) { + throw new Error( + `Worktree path "${workspacePath}" already exists and is still registered with git`, + ); + } + // Use removeDirWithRetry for Windows file-handle drain races (matches + // destroy()'s fallback). On Unix this is just rmSync. + await removeDirWithRetry(workspacePath); + } +} + +/** + * Restore recovery: re-attach an existing local branch to a worktree at + * `workspacePath`. Used when the branch is already present (destroy() + * preserves it) but the first `git worktree add ` failed + * — typically because `workspacePath` has a stale registry entry, a + * stale directory, or both. + * + * Never uses -b/-B: -b would fail with "branch already exists", and -B + * would force-reset the branch to a base ref and silently discard the + * session's commits, which is the opposite of restore's intent. + */ +async function reattachExistingBranch( + repoPath: string, + workspacePath: string, + branch: string, +): Promise { + await cleanupStaleWorkspacePath(repoPath, workspacePath); + await git(repoPath, "worktree", "add", workspacePath, branch); +} + +/** + * Restore recovery: create a fresh branch at `workspacePath` from the + * appropriate base ref. Used when the local branch is missing — typically + * because only `origin/` exists and we need to materialize the + * local ref. Tries the remote ref first, then falls back to the local + * default branch. + * + * Runs the same stale-path cleanup as reattachExistingBranch so this path + * also recovers when `workspacePath` has a stale registry entry / dir. + */ +async function createBranchFromBase( + repoPath: string, + workspacePath: string, + branch: string, + defaultBranch: string, + hasOrigin: boolean, +): Promise { + await cleanupStaleWorkspacePath(repoPath, workspacePath); + + const baseRef = await resolveBaseRef(repoPath, defaultBranch, { branch, hasOrigin }); + + if (!baseRef.startsWith("origin/")) { + // No remote available — create from the local default branch + await git(repoPath, "worktree", "add", "-b", branch, workspacePath, baseRef); + return; + } + + // Branch might not exist locally — try the remote ref first, then fall + // back to the local default branch if the remote ref is unavailable. + try { + await git(repoPath, "worktree", "add", "-b", branch, workspacePath, baseRef); + } catch { + await git( + repoPath, + "worktree", + "add", + "-b", + branch, + workspacePath, + `refs/heads/${defaultBranch}`, + ); + } +} + interface WorktreeEntry { path: string; branch: string | null; @@ -453,34 +556,20 @@ export function create(config?: Record): Workspace { } } - // Try to create worktree on the existing branch + // Try to create worktree on the existing branch. try { await git(repoPath, "worktree", "add", workspacePath, cfg.branch); } catch { - const baseRef = await resolveBaseRef(repoPath, cfg.project.defaultBranch, { - branch: cfg.branch, - hasOrigin, - }); - - if (!baseRef.startsWith("origin/")) { - // No remote available — create from the local default branch - await git(repoPath, "worktree", "add", "-b", cfg.branch, workspacePath, baseRef); + if (await refExists(repoPath, `refs/heads/${cfg.branch}`)) { + await reattachExistingBranch(repoPath, workspacePath, cfg.branch); } else { - // Branch might not exist locally — try the remote ref first, then fall back - // to the local default branch if the remote ref is unavailable. - try { - await git(repoPath, "worktree", "add", "-b", cfg.branch, workspacePath, baseRef); - } catch { - await git( - repoPath, - "worktree", - "add", - "-b", - cfg.branch, - workspacePath, - `refs/heads/${cfg.project.defaultBranch}`, - ); - } + await createBranchFromBase( + repoPath, + workspacePath, + cfg.branch, + cfg.project.defaultBranch, + hasOrigin, + ); } }