Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bun.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 18 additions & 16 deletions packages/host-service/src/trpc/router/workspaces/workspaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import type { HostServiceContext } from "../../../types";
import { protectedProcedure, router } from "../../index";
import { type AgentRunResult, runAgentInWorkspace } from "../agents";
import { ensureMainWorkspace } from "../project/utils/ensure-main-workspace";
import { getWorktreeBranchAtPath } from "../workspace-creation/shared/branch-search";
import { listWorktreeBranches } from "../workspace-creation/shared/branch-search";
import { enablePushAutoSetupRemote } from "../workspace-creation/shared/git-config";
import { requireLocalProject } from "../workspace-creation/shared/local-project";
import { startSetupTerminalIfPresent } from "../workspace-creation/shared/setup-terminal";
Expand Down Expand Up @@ -708,21 +708,23 @@ export const workspacesRouter = router({
workspaceRow = existing;
alreadyExists = true;
} else {
worktreePath = safeResolveWorktreePath(
localProject.id,
resolvedBranch,
);

// Adopt: a worktree already exists at the standard path with the
// matching branch checked out (e.g. left behind by a prior session
// or registered outside Superset). Skip `git worktree add` and
// proceed straight to register. Only meaningful when the user
// supplied the branch — auto-gen names are deduped and can't
// collide with anything pre-existing.
const adopted =
!!typedBranch &&
(await getWorktreeBranchAtPath(git, worktreePath)) ===
resolvedBranch;
// Adopt: a worktree already exists for this branch (at the
// standard path, or anywhere else git has registered it — e.g.
// left behind by a prior session, created outside Superset, or
// renamed in place). Skip `git worktree add`; git refuses to
// check out a branch into a second worktree, so failing here
// would block the user from re-entering their own work. Only
// meaningful when the user supplied the branch — auto-gen
// names are deduped and can't collide with anything pre-existing.
const existingWorktreePath = typedBranch
? (await listWorktreeBranches(git)).worktreeMap.get(
resolvedBranch,
)
: undefined;
const adopted = !!existingWorktreePath;
worktreePath =
existingWorktreePath ??
safeResolveWorktreePath(localProject.id, resolvedBranch);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Stale/prunable worktrees can be adopted as worktreePath

listWorktreeBranches iterates all worktrees — including ones marked prunable (directory deleted without git worktree remove). When such a stale entry exists for resolvedBranch, existingWorktreePath is set to the now-missing path, adopted = true skips git worktree add, and the subsequent enablePushAutoSetupRemote(git, worktreePath, …) runs against a directory that no longer exists, likely throwing. Before this PR the same risk existed only at the canonical path; now it applies to any registered path. Filtering wt.prunable != null entries in listWorktreeBranches (or inline here) would prevent this.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/workspaces/workspaces.ts
Line: 719-727

Comment:
**Stale/prunable worktrees can be adopted as `worktreePath`**

`listWorktreeBranches` iterates all worktrees — including ones marked `prunable` (directory deleted without `git worktree remove`). When such a stale entry exists for `resolvedBranch`, `existingWorktreePath` is set to the now-missing path, `adopted = true` skips `git worktree add`, and the subsequent `enablePushAutoSetupRemote(git, worktreePath, …)` runs against a directory that no longer exists, likely throwing. Before this PR the same risk existed only at the canonical path; now it applies to any registered path. Filtering `wt.prunable != null` entries in `listWorktreeBranches` (or inline here) would prevent this.

How can I resolve this? If you propose a fix, please make it concise.


mkdirSync(dirname(worktreePath), { recursive: true });

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { afterEach, describe, expect, test } from "bun:test";
import { randomUUID } from "node:crypto";
import { existsSync } from "node:fs";
import { join } from "node:path";
import { TRPCClientError } from "@trpc/client";
import { eq } from "drizzle-orm";
import { workspaces } from "../../src/db/schema";
Expand Down Expand Up @@ -49,6 +50,47 @@ describe("workspace.create + workspace.delete integration", () => {
expect(existsSync(persisted?.worktreePath ?? "")).toBe(true);
});

test("create() adopts an existing worktree at a non-canonical path instead of failing on `git worktree add`", async () => {
// Regress: when the user typed a branch that already has a worktree
// somewhere outside `~/.superset/worktrees/<projectId>/<branch>`,
// `workspaces.create` used to call `git worktree add` and crash with
// `fatal: '<branch>' is already used by worktree at ...`. Adopt the
// existing path instead.
const scenario = await createProjectScenario({
hostOptions: { apiOverrides: cloudFlows.workspaceCreateOk() },
});
dispose = scenario.dispose;

const branch = "new-workspace-9";
const nonCanonicalPath = join(
scenario.repo.repoPath,
".worktrees",
"glorious-ground",
);
await scenario.repo.git.raw([
"worktree",
"add",
"-b",
branch,
nonCanonicalPath,
]);

const result = await scenario.host.trpc.workspaces.create.mutate({
projectId: scenario.projectId,
name: "adopted",
branch,
});

expect(result?.workspace?.branch).toBe(branch);
const persisted = scenario.host.db
.select()
.from(workspaces)
.where(eq(workspaces.id, result?.workspace?.id ?? ""))
.get();
expect(persisted?.worktreePath).toBe(nonCanonicalPath);
expect(existsSync(nonCanonicalPath)).toBe(true);
});

test("create() rolls back the worktree if cloud v2Workspace.create fails", async () => {
const scenario = await createProjectScenario({
hostOptions: {
Expand Down
Loading