Skip to content
Merged
Changes from all commits
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
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { existsSync } from "node:fs";
import { join, resolve, sep } from "node:path";
import { existsSync, mkdirSync } from "node:fs";
import { homedir } from "node:os";
import { dirname, join, resolve, sep } from "node:path";
import { getDeviceName, getHashedDeviceId } from "@superset/shared/device-info";
import { TRPCError } from "@trpc/server";
import { and, eq } from "drizzle-orm";
Expand Down Expand Up @@ -84,12 +85,25 @@ function projectNotSetupError(projectId: string): TRPCError {
});
}

function safeResolveWorktreePath(repoPath: string, branchName: string): string {
const worktreesRoot = resolve(repoPath, ".worktrees");
const worktreePath = resolve(worktreesRoot, branchName);
// Kept outside the primary checkout so editors, file watchers, and
// ignore rules treat worktrees as separate trees, not nested ones.
function supersetWorktreesRoot(): string {
return join(homedir(), ".superset", "worktrees");
}

function projectWorktreesRoot(projectId: string): string {
return resolve(supersetWorktreesRoot(), projectId);
}

function safeResolveWorktreePath(
projectId: string,
branchName: string,
): string {
const projectRoot = projectWorktreesRoot(projectId);
const worktreePath = resolve(projectRoot, branchName);
if (
worktreePath !== worktreesRoot &&
!worktreePath.startsWith(worktreesRoot + sep)
worktreePath !== projectRoot &&
!worktreePath.startsWith(projectRoot + sep)
) {
throw new TRPCError({
code: "BAD_REQUEST",
Expand Down Expand Up @@ -168,18 +182,29 @@ function markRefetchRemote(projectId: string): void {
type GitClient = Awaited<ReturnType<HostServiceContext["git"]>>;

async function listWorktreeBranches(
ctx: HostServiceContext,
git: GitClient,
repoPath: string,
projectId: string,
): Promise<{
// Superset-managed worktrees only (under <repoPath>/.worktrees/).
// These count as "has a workspace" for the picker.
// A worktree counts as "ours" if its path either matches a row in
// the local `workspaces` table or lives under our managed root. The
// second case catches orphans (worktree on disk, no workspaces row,
// e.g. partial create rollback) so they surface for adoption.
worktreeMap: Map<string, string>;
// Every branch checked out in any git worktree, including the primary
// working tree. Used to disable the Checkout action when a branch is
// already in use elsewhere — `git worktree add <path> <branch>` would fail.
checkedOutBranches: Set<string>;
}> {
const worktreesRoot = resolve(repoPath, ".worktrees");
const managedRoot = projectWorktreesRoot(projectId);
const knownPaths = new Set<string>(
ctx.db
.select({ path: workspaces.worktreePath })
.from(workspaces)
.where(eq(workspaces.projectId, projectId))
.all()
.map((w) => w.path),
);
const worktreeMap = new Map<string, string>();
const checkedOutBranches = new Set<string>();
try {
Expand All @@ -192,9 +217,10 @@ async function listWorktreeBranches(
const branch = line.slice("branch refs/heads/".length).trim();
if (!branch) continue;
checkedOutBranches.add(branch);
// Superset-managed worktrees live under <repoPath>/.worktrees/<name>;
// the primary working tree is at repoPath itself and skipped here.
if (currentPath.startsWith(worktreesRoot + sep)) {
if (
knownPaths.has(currentPath) ||
currentPath.startsWith(managedRoot + sep)
) {
worktreeMap.set(branch, currentPath);
}
} else if (line === "") {
Expand Down Expand Up @@ -558,8 +584,9 @@ export const workspaceCreationRouter = router({
const defaultBranch: string | null = await resolveDefaultBranchName(git);

const { worktreeMap, checkedOutBranches } = await listWorktreeBranches(
ctx,
git,
localProject.repoPath,
input.projectId,
);
const recencyMap = await getRecentBranchOrder(git, 30);

Expand Down Expand Up @@ -793,10 +820,8 @@ export const workspaceCreationRouter = router({
);

// 3. Create worktree
const worktreePath = safeResolveWorktreePath(
localProject.repoPath,
branchName,
);
const worktreePath = safeResolveWorktreePath(localProject.id, branchName);
mkdirSync(dirname(worktreePath), { recursive: true });
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 Unhandled mkdirSync failure

mkdirSync can throw (e.g. permission denied, disk full, a file already exists at that path) and is called outside any try/catch here. If it throws, the mutation will propagate a raw EACCES/ENOSPC/EEXIST Node error to the client rather than a structured TRPCError. The same pattern is repeated at lines 1133 and 1259.

Consider wrapping each call so failures surface with a clear, user-friendly message:

try {
  mkdirSync(dirname(worktreePath), { recursive: true });
} catch (err) {
  clearProgress(input.pendingId);
  throw new TRPCError({
    code: "INTERNAL_SERVER_ERROR",
    message: `Failed to create worktree directory: ${err instanceof Error ? err.message : String(err)}`,
  });
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
Line: 789

Comment:
**Unhandled `mkdirSync` failure**

`mkdirSync` can throw (e.g. permission denied, disk full, a file already exists at that path) and is called outside any try/catch here. If it throws, the mutation will propagate a raw `EACCES`/`ENOSPC`/`EEXIST` Node error to the client rather than a structured `TRPCError`. The same pattern is repeated at lines 1133 and 1259.

Consider wrapping each call so failures surface with a clear, user-friendly message:

```ts
try {
  mkdirSync(dirname(worktreePath), { recursive: true });
} catch (err) {
  clearProgress(input.pendingId);
  throw new TRPCError({
    code: "INTERNAL_SERVER_ERROR",
    message: `Failed to create worktree directory: ${err instanceof Error ? err.message : String(err)}`,
  });
}
```

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


const git = await ctx.git(localProject.repoPath);

Expand Down Expand Up @@ -1166,11 +1191,12 @@ export const workspaceCreationRouter = router({

let worktreePath: string;
try {
worktreePath = safeResolveWorktreePath(localProject.repoPath, branch);
worktreePath = safeResolveWorktreePath(localProject.id, branch);
} catch (err) {
clearProgress(input.pendingId);
throw err;
}
mkdirSync(dirname(worktreePath), { recursive: true });
const git = await ctx.git(localProject.repoPath);

// Detect a pre-existing local branch with the same derived name
Expand Down Expand Up @@ -1291,11 +1317,12 @@ export const workspaceCreationRouter = router({

let worktreePath: string;
try {
worktreePath = safeResolveWorktreePath(localProject.repoPath, branch);
worktreePath = safeResolveWorktreePath(localProject.id, branch);
} catch (err) {
clearProgress(input.pendingId);
throw err;
}
mkdirSync(dirname(worktreePath), { recursive: true });
const git = await ctx.git(localProject.repoPath);

// Resolve via the discriminated-ref helper so we don't infer kind
Expand Down Expand Up @@ -1394,10 +1421,10 @@ export const workspaceCreationRouter = router({

/**
* Adopt an existing git worktree as a workspace. Used when the Worktree
* tab surfaces a branch whose `.worktrees/<branch>` directory exists on
* disk but has no corresponding workspaces row (e.g. created by an older
* flow, or partial create rollback). No git ops — just registers the
* cloud + local workspace row over the existing worktree path.
* tab surfaces a branch whose worktree directory exists on disk but has
* no corresponding workspaces row (e.g. partial create rollback). No git
* ops — just registers the cloud + local workspace row over the
* existing worktree path.
*/
adopt: protectedProcedure
.input(
Expand Down Expand Up @@ -1446,8 +1473,9 @@ export const workspaceCreationRouter = router({
worktreePath = input.worktreePath;
} else {
const { worktreeMap } = await listWorktreeBranches(
ctx,
git,
localProject.repoPath,
input.projectId,
);
const found = worktreeMap.get(branch);
if (!found) {
Expand Down
Loading