refactor(worktree): resolve worktree path from stored value, not derived from name

packages/workspace-server/src/services/workspace/workspace.test.ts

@@ -1,3 +1,6 @@
+import * as fs from "node:fs";
+import * as os from "node:os";
+import * as path from "node:path";
 import type { RootLogger } from "@posthog/di/logger";
 import {
   branchExists,
@@ -8,7 +11,7 @@ import {
 import type { IAnalytics } from "@posthog/platform/analytics";
 import type { IWorkspaceSettings } from "@posthog/platform/workspace-settings";
 import { ANALYTICS_EVENTS } from "@posthog/shared";
-import { beforeEach, describe, expect, it, vi } from "vitest";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 import { createMockRepositoryRepository } from "../../db/repositories/repository-repository.mock";
 import { createMockWorkspaceRepository } from "../../db/repositories/workspace-repository.mock";
 import { createMockWorktreeRepository } from "../../db/repositories/worktree-repository.mock";
@@ -33,6 +36,17 @@ vi.mock("@posthog/git/queries", async (importOriginal) => {
   };
 });
 
+// Neutralize the real git worktree removal so delete tests exercise only the
+// service's path resolution and managed-folder cleanup, not actual git/fs ops.
+vi.mock("../worktree-query/worktree-query", async (importOriginal) => {
+  const actual =
+    await importOriginal<typeof import("../worktree-query/worktree-query")>();
+  return {
+    ...actual,
+    deleteWorktree: vi.fn(async () => {}),
+  };
+});
+
 function createMocks() {
   const agent = {
     cancelSessionsByTaskId: vi.fn(async () => {}),
@@ -90,6 +104,29 @@ function createMocks() {
   };
 }
 
+/** Seed a worktree-mode workspace whose stored row carries `name` and `path`. */
+function seedWorktreeTask(
+  mocks: ReturnType<typeof createMocks>,
+  opts: {
+    taskId: string;
+    repoPath: string;
+    name: string;
+    worktreePath: string;
+  },
+): void {
+  const repo = mocks.repositoryRepo.create({ path: opts.repoPath });
+  const workspace = mocks.workspaceRepo.create({
+    taskId: opts.taskId,
+    repositoryId: repo.id,
+    mode: "worktree",
+  });
+  mocks.worktreeRepo.create({
+    workspaceId: workspace.id,
+    name: opts.name,
+    path: opts.worktreePath,
+  });
+}
+
 function makeService(mocks: ReturnType<typeof createMocks>): WorkspaceService {
   return new WorkspaceService(
     mocks.agent,
@@ -273,4 +310,124 @@ describe("WorkspaceService", () => {
       ).toEqual({ status: "trunk" });
     });
   });
+
+  describe("worktree path resolved from the stored row", () => {
+    const tempDirs: string[] = [];
+
+    afterEach(() => {
+      for (const dir of tempDirs.splice(0)) {
+        fs.rmSync(dir, { recursive: true, force: true });
+      }
+    });
+
+    function mkTemp(prefix: string): string {
+      const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix));
+      tempDirs.push(dir);
+      return dir;
+    }
+
+    it("projects an externally-located worktree from its stored path", async () => {
+      const externalPath = "/external/checkout/my-worktree";
+      seedWorktreeTask(mocks, {
+        taskId: "ext",
+        repoPath: "/code/myrepo",
+        name: "fancy-slug",
+        worktreePath: externalPath,
+      });
+
+      expect(await service.getWorkspace("ext")).toMatchObject({
+        mode: "worktree",
+        worktreePath: externalPath,
+        worktreeName: "fancy-slug",
+      });
+      expect(await service.getWorkspaceInfo("ext")).toMatchObject({
+        mode: "worktree",
+        worktree: expect.objectContaining({
+          worktreePath: externalPath,
+          worktreeName: "fancy-slug",
+        }),
+      });
+    });
+
+    it("matches occupancy by the stored path, not a derived one", () => {
+      const externalPath = "/external/checkout/my-worktree";
+      seedWorktreeTask(mocks, {
+        taskId: "ext",
+        repoPath: "/code/myrepo",
+        name: "fancy-slug",
+        worktreePath: externalPath,
+      });
+
+      expect(service.getWorktreeTasks(externalPath)).toEqual([
+        { taskId: "ext" },
+      ]);
+      // The name would derive to <base>/<name>/<repo>; that path must not match.
+      expect(
+        service.getWorktreeTasks("/tmp/worktrees/fancy-slug/myrepo"),
+      ).toEqual([]);
+    });
+
+    it("verifies existence by the stored external path", async () => {
+      const externalPath = mkTemp("external-wt-");
+      seedWorktreeTask(mocks, {
+        taskId: "ext",
+        repoPath: "/code/myrepo",
+        name: "fancy-slug",
+        worktreePath: externalPath,
+      });
+
+      // The on-disk worktree lives at its stored external path; a derived
+      // <base>/<name>/<repo> would not exist, so this would report missing.
+      expect(await service.verifyWorkspaceExists("ext")).toEqual({
+        exists: true,
+      });
+
+      fs.rmSync(externalPath, { recursive: true, force: true });
+      expect(await service.verifyWorkspaceExists("ext")).toEqual({
+        exists: false,
+        missingPath: externalPath,
+      });
+    });
+
+    // Identical setup (empty managed `<base>/<repo>` parent, then delete the only
+    // worktree for that repo); only the stored worktree path differs. This proves
+    // the cleanup guard discriminates on whether the path is under the base path,
+    // rather than always (or never) reclaiming the parent folder.
+    it.each([
+      {
+        label:
+          "leaves the managed parent folder alone for an external worktree",
+        makeWorktreePath: () => mkTemp("external-wt-"),
+        managedParentSurvives: true,
+      },
+      {
+        label:
+          "reclaims the empty managed parent folder for a worktree under the base path",
+        makeWorktreePath: (base: string) =>
+          path.join(base, "some-name", "myrepo"),
+        managedParentSurvives: false,
+      },
+    ])(
+      "deleteWorkspace via the stored path $label",
+      async ({ makeWorktreePath, managedParentSurvives }) => {
+        const base = mkTemp("wt-base-");
+        mocks.workspaceSettings.getWorktreeLocation = () => base;
+
+        const repoPath = "/code/myrepo";
+        const managedParent = path.join(base, "myrepo");
+        fs.mkdirSync(managedParent);
+
+        seedWorktreeTask(mocks, {
+          taskId: "task",
+          repoPath,
+          name: "some-name",
+          worktreePath: makeWorktreePath(base),
+        });
+
+        await service.deleteWorkspace("task", repoPath);
+
+        expect(fs.existsSync(managedParent)).toBe(managedParentSurvives);
+      },
+    );
+  });
 });

packages/workspace-server/src/services/workspace/workspace.ts

@@ -39,7 +39,6 @@ import type { ProcessTrackingService } from "../process-tracking/process-trackin
 import { getBranchFromPath, hasAnyFiles } from "../repo-fs-query/repo-fs-query";
 import { SUSPENSION_SERVICE } from "../suspension/identifiers";
 import type { SuspensionService } from "../suspension/suspension";
-import { deriveWorktreePath as deriveWorktreePathFromBase } from "../worktree-path/worktree-path";
 import {
   deleteWorktree as deleteGitWorktree,
   listTwigWorktrees,
@@ -80,10 +79,22 @@ type TaskAssociation =
       taskId: string;
       folderId: string;
       mode: "worktree";
+      /** Cosmetic worktree name, surfaced as `worktreeName` in projections. */
       worktree: string;
+      /** Authoritative on-disk worktree path, read from the stored row. */
+      path: string;
       branchName: string | null;
     };
 
+/** True when `child` resolves to a path strictly inside `parent`. */
+function isPathUnder(child: string, parent: string): boolean {
+  const rel = path.relative(parent, child);
+  const up = `..${path.sep}`;
+  return (
+    rel !== "" && rel !== ".." && !rel.startsWith(up) && !path.isAbsolute(rel)
+  );
+}
+
 export const WorkspaceServiceEvent = {
   Error: "error",
   Warning: "warning",
@@ -139,14 +150,6 @@ export class WorkspaceService extends TypedEventEmitter<WorkspaceServiceEvents>
   private creatingWorkspaces = new Map<string, Promise<WorkspaceInfo>>();
   private branchWatcherInitialized = false;
 
-  private deriveWorktreePath(folderPath: string, worktreeName: string): string {
-    return deriveWorktreePathFromBase(
-      this.workspaceSettings.getWorktreeLocation(),
-      folderPath,
-      worktreeName,
-    );
-  }
-
   private findTaskAssociation(taskId: string): TaskAssociation | null {
     const workspace = this.workspaceRepo.findByTaskId(taskId);
     if (!workspace) return null;
@@ -169,6 +172,7 @@ export class WorkspaceService extends TypedEventEmitter<WorkspaceServiceEvents>
         folderId: workspace.repositoryId,
         mode: "worktree",
         worktree: worktree.name,
+        path: worktree.path,
         branchName: null,
       };
     }
@@ -209,6 +213,7 @@ export class WorkspaceService extends TypedEventEmitter<WorkspaceServiceEvents>
           folderId: workspace.repositoryId,
           mode: "worktree",
           worktree: worktree.name,
+          path: worktree.path,
           branchName: null,
         });
       } else {
@@ -250,10 +255,7 @@ export class WorkspaceService extends TypedEventEmitter<WorkspaceServiceEvents>
     const associations = this.getAllTaskAssociations();
     for (const assoc of associations) {
       if (assoc.mode !== "worktree") continue;
-      const folderPath = this.getFolderPath(assoc.folderId);
-      if (!folderPath) continue;
-      const derivedPath = this.deriveWorktreePath(folderPath, assoc.worktree);
-      if (derivedPath === worktreePath && assoc.branchName !== newBranch) {
+      if (assoc.path === worktreePath && assoc.branchName !== newBranch) {
         this.updateAssociationBranchName(assoc.taskId, newBranch);
         this.emit(WorkspaceServiceEvent.BranchChanged, {
           taskId: assoc.taskId,
@@ -277,11 +279,7 @@ export class WorkspaceService extends TypedEventEmitter<WorkspaceServiceEvents>
       if (!folderPath) continue;
 
       if (assoc.mode === "worktree") {
-        const worktreePath = this.deriveWorktreePath(
-          folderPath,
-          assoc.worktree,
-        );
-        if (worktreePath !== repoPath) continue;
+        if (assoc.path !== repoPath) continue;
 
         const currentBranch = await getBranchFromPath(repoPath);
         if (currentBranch !== null && currentBranch !== assoc.branchName) {
@@ -721,7 +719,7 @@ export class WorkspaceService extends TypedEventEmitter<WorkspaceServiceEvents>
     let worktreePath: string | null = null;
 
     if (association.mode === "worktree") {
-      worktreePath = this.deriveWorktreePath(folderPath, association.worktree);
+      worktreePath = association.path;
     }
 
     await this.agent.cancelSessionsByTaskId(taskId);
@@ -743,7 +741,7 @@ export class WorkspaceService extends TypedEventEmitter<WorkspaceServiceEvents>
       );
 
       if (otherWorkspacesForFolder.length === 0) {
-        await this.cleanupRepoWorktreeFolder(folderPath);
+        await this.cleanupRepoWorktreeFolder(folderPath, worktreePath);
       }
     }
 
@@ -760,8 +758,25 @@ export class WorkspaceService extends TypedEventEmitter<WorkspaceServiceEvents>
     this.workspaceRepo.deleteByTaskId(taskId);
   }
 
-  private async cleanupRepoWorktreeFolder(folderPath: string): Promise<void> {
+  /**
+   * Reclaims the managed `<base>/<repo>` parent folder once its last worktree is
+   * gone. The folder that gets removed is derived from `folderPath`, not from
+   * `worktreePath`; `worktreePath` is read only to confirm the deleted worktree
+   * was managed (lived under the base path) before reclaiming anything.
+   */
+  private async cleanupRepoWorktreeFolder(
+    folderPath: string,
+    worktreePath: string,
+  ): Promise<void> {
     const worktreeBasePath = this.workspaceSettings.getWorktreeLocation();
+
+    // Only reclaim the managed `<base>/<repo>` parent folder for worktrees that
+    // actually live under the managed base path. Externally located worktrees
+    // never created it, so its contents are unrelated.
+    if (!isPathUnder(worktreePath, worktreeBasePath)) {
+      return;
+    }
+
     const repoName = path.basename(folderPath);
     const repoWorktreeFolderPath = path.join(worktreeBasePath, repoName);
 
@@ -838,10 +853,7 @@ export class WorkspaceService extends TypedEventEmitter<WorkspaceServiceEvents>
     }
 
     if (association.mode === "worktree") {
-      const worktreePath = this.deriveWorktreePath(
-        folderPath,
-        association.worktree,
-      );
+      const worktreePath = association.path;
       const exists = fs.existsSync(worktreePath);
       if (!exists) {
         this.log.info(`Worktree for task ${taskId} no longer exists`);
@@ -884,7 +896,7 @@ export class WorkspaceService extends TypedEventEmitter<WorkspaceServiceEvents>
 
     if (assoc.mode === "worktree") {
       worktreeName = assoc.worktree;
-      worktreePath = this.deriveWorktreePath(folderPath, worktreeName);
+      worktreePath = assoc.path;
       const gitBranch = await getBranchFromPath(worktreePath);
       branchName = gitBranch ?? assoc.branchName;
     } else if (assoc.mode === "local") {
@@ -932,22 +944,17 @@ export class WorkspaceService extends TypedEventEmitter<WorkspaceServiceEvents>
     let worktreeInfo: WorktreeInfo | null = null;
     let branchName: string | null = null;
 
-    if (association.mode === "worktree") {
-      if (folderPath) {
-        const worktreePath = this.deriveWorktreePath(
-          folderPath,
-          association.worktree,
-        );
-        const gitBranch = await getBranchFromPath(worktreePath);
-        branchName = gitBranch ?? association.branchName;
-        worktreeInfo = {
-          worktreePath,
-          worktreeName: association.worktree,
-          branchName,
-          baseBranch: "main",
-          createdAt: new Date().toISOString(),
-        };
-      }
+    if (association.mode === "worktree" && folderPath) {
+      const worktreePath = association.path;
+      const gitBranch = await getBranchFromPath(worktreePath);
+      branchName = gitBranch ?? association.branchName;
+      worktreeInfo = {
+        worktreePath,
+        worktreeName: association.worktree,
+        branchName,
+        baseBranch: "main",
+        createdAt: new Date().toISOString(),
+      };
     } else if (association.mode === "local" && folderPath) {
       branchName = await getBranchFromPath(folderPath);
     }
@@ -994,7 +1001,7 @@ export class WorkspaceService extends TypedEventEmitter<WorkspaceServiceEvents>
 
       if (assoc.mode === "worktree") {
         worktreeName = assoc.worktree;
-        worktreePath = this.deriveWorktreePath(folderPath, worktreeName);
+        worktreePath = assoc.path;
       }
 
       let branchName: string | null = null;
@@ -1118,10 +1125,7 @@ export class WorkspaceService extends TypedEventEmitter<WorkspaceServiceEvents>
 
     for (const assoc of associations) {
       if (assoc.mode !== "worktree") continue;
-      const folderPath = this.getFolderPath(assoc.folderId);
-      if (!folderPath) continue;
-      const derivedPath = this.deriveWorktreePath(folderPath, assoc.worktree);
-      if (derivedPath === worktreePath) {
+      if (assoc.path === worktreePath) {
         result.push({ taskId: assoc.taskId });
       }
     }