fix(workspace-worktree): restore re-attaches existing branch instead of recreating with -b (#1742)
* fix(workspace-worktree): restore re-attaches existing branch instead of recreating with -b Restoring a session whose worktree directory was cleaned up but whose branch still existed locally would 422 with `fatal: a branch named <X> already exists`. The recovery path in `restore()` unconditionally fell through to `git worktree add -b`, even though `destroy()` deliberately preserves session branches so the user's commits aren't lost. When the local branch already exists, restore now clears any stale worktree registration at the target path and retries `git worktree add <path> <branch>` (no -b/-B). The existing -b fallback is preserved verbatim for the case where the local branch is genuinely missing (only the remote ref exists). -B is intentionally not used — it would force-reset the branch back to the base ref and silently discard the session's commits, which is the opposite of restore's intent. Test coverage: - 6 new unit tests covering the recovery path, cleanup tolerance, error propagation, and "no -b/-B" invariants - 2 updated existing unit tests (now mock the new refExists check) - 2 new integration tests exercising real git: branch preservation on clean restore, and recovery from a dirty teardown that left a stale registry entry Closes #1741 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(workspace-worktree): explicitly create main branch in restore integration tests CI runs git with a different `init.defaultBranch` than the local dev environment, so the bare clone has no `main` branch when the test attempts to push. Mirror the existing tests in this file (which also call `git switch -c <branch>` before the first commit). Fixes integration-test failures on PR #1742. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(workspace-worktree): rmSync stale workspace dir before retry Follow-up to the previous commit on this branch. The recovery path cleared the git worktree registry but didn't touch the filesystem, so when restore was triggered by a stale junk directory at the workspace path (workspace.exists() returns false because rev-parse fails on a non-working-tree dir), the retry would fail with the same error: fatal: '<workspacePath>' already exists Replace the inline `worktree prune` cleanup with a call to the existing `clearStaleWorktreePath()` helper, which handles all three states: - dir gone → no-op - dir present and not registered → rmSync - dir present and still registered after prune → throws (safety: never delete a registered worktree) This mirrors how create() already handles the same stale-state cases upfront via clearStaleWorktreePath at the top of its flow. Test coverage: - 1 new unit test: rmSyncs a stale workspace directory before retry - 1 new unit test: refuses to rmSync a still-registered worktree dir (data safety — error must propagate, not be swallowed) - 1 new integration test on real git: dir physically present as non-working-tree leftover, restore must rmSync it before retry, and the session commit must survive Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(workspace-worktree): extract restore helpers, drop redundant prune Addresses review feedback on PR #1742: 1. Extract two named helpers, reattachExistingBranch and createBranchFromBase, so restore()'s catch block reads as the bifurcation it actually is — 2 lines per branch, no nested try/catch hierarchy. Behavior is unchanged. 2. Drop the redundant `worktree prune` that ran inside the recovery path (via clearStaleWorktreePath). The entry-point prune in restore() is sufficient. reattachExistingBranch now inlines the existsSync + isRegisteredWorktree + rmSync sequence directly, keeping the data-safety guard ("refuse to rmSync a registered worktree") intact but skipping the second prune call. The catch block shrinks from ~46 lines to 12 (the rest moves into the helpers, where the docstrings can explain WHY each branch exists without cluttering the call site). Tests: same 64 unit + 13 integration tests still pass — the mock sequences in the recovery-path tests no longer expect a second prune call, since the new code doesn't make one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(workspace-worktree): address Copilot review on PR #1742 Two real concerns flagged in inline review: 1. isRegisteredWorktree did strict string equality on paths. If `workspacePath` was passed in a non-canonical form (trailing slash, ".." segments) and git reported the canonical path, the check would false-negative — and the subsequent rmSync in cleanupStaleWorkspacePath would silently delete a still-registered worktree (data loss). Fix by resolve()-normalizing both sides before comparison. 2. createBranchFromBase (the "branch missing locally" recovery path) skipped the stale-path cleanup that reattachExistingBranch did. So if `workspacePath` had a stale dir AND the branch was missing, `git worktree add -b ...` would fail with the same "<path> already exists" error this PR was fixing for the re-attach case. Fix by factoring the cleanup into cleanupStaleWorkspacePath, called from both helpers. Test coverage: - Unit: path normalization safety — workspacePath with trailing slash vs canonical registered path must still throw "still registered" (proves rmSync is NOT called) - Unit: createBranchFromBase clears stale dir before -b add - Integration: branch missing locally + stale dir at workspacePath → restore must clean dir AND recreate branch from origin (commit preserved end-to-end on real git) Existing 2 "branch missing" tests updated to mock the new cleanup calls in createBranchFromBase. 66 unit tests + 14 integration tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Harshit Singh Bhandari
GitHub
parent
13c5a50d02
commit
a33b2ba0ef
|
|
@ -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 <X> 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)
|
||||
|
|
@ -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 <path> <branch>` 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 <path> <branch>` fails with `'<path>' 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
|
||||
// `'<path>' 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/<branch>
|
||||
// exists) AND `workspacePath` has stale state, the -b fallback must also
|
||||
// run the cleanup. Without it, `git worktree add -b ...` fails with
|
||||
// `'<path>' 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 `'<path>' 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-"));
|
||||
|
|
|
|||
|
|
@ -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 <path> (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 <path> (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 <path>` (best-effort: clears registry)
|
||||
// 2. existsSync(<path>) — bail if dir already gone
|
||||
// 3. `git worktree list --porcelain` (isRegisteredWorktree)
|
||||
// 4. rmSync(<path>) if not still registered (else throw — data safety)
|
||||
// 5. `git worktree add <path> <branch>` 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 <path>
|
||||
mockExistsSync.mockReturnValueOnce(false); // no leftover dir, skip cleanup
|
||||
mockGitSuccess(""); // RETRY: worktree add <path> <branch> 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
|
||||
// "<path> 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/<branch>", 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
|
||||
// "<path> 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 <path> <branch> 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()", () => {
|
||||
|
|
|
|||
|
|
@ -135,13 +135,17 @@ async function resolveBaseRef(
|
|||
async function isRegisteredWorktree(repoPath: string, worktreePath: string): Promise<boolean> {
|
||||
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 `<path> 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<void> {
|
||||
// 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 <path> <branch>` 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<void> {
|
||||
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/<branch>` 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<void> {
|
||||
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<string, unknown>): 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,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue