From 0ff2875a9e29be9d35b3c4961607ef8bfdf5d019 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Tue, 16 Jun 2026 13:02:54 -0700 Subject: [PATCH 1/4] refactor(worktree): resolve worktree path from stored value, not derived from name Make the persisted `worktrees.path` column the source of truth for a task's worktree path. The `TaskAssociation` worktree variant now carries the stored `path` alongside the cosmetic `name`, and every read/delete/occupancy path in WorkspaceService uses it instead of recomputing `//` via the private `deriveWorktreePath` helper (now removed here). Guard `cleanupRepoWorktreeFolder` so it only reclaims the managed `/` parent folder when the deleted worktree actually lived under the managed base path. Externally located worktrees never created it, so their parent dirs are left untouched. Behavioral no-op for app-created worktrees: their stored `path` already equals the previously derived path, so no data migration is needed. The legacy-layout name-recovery hack the original scope referenced (findExistingWorktreeForBranch / finalSegment) no longer exists in the tree, so that item produced no diff. --- .../src/services/workspace/workspace.test.ts | 144 +++++++++++++++++- .../src/services/workspace/workspace.ts | 67 ++++---- 2 files changed, 175 insertions(+), 36 deletions(-) diff --git a/packages/workspace-server/src/services/workspace/workspace.test.ts b/packages/workspace-server/src/services/workspace/workspace.test.ts index 351b1f33cf..409c54611c 100644 --- a/packages/workspace-server/src/services/workspace/workspace.test.ts +++ b/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(); + 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, + 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): WorkspaceService { return new WorkspaceService( mocks.agent, @@ -273,4 +310,109 @@ 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 //; that path must not match. + expect( + service.getWorktreeTasks("/tmp/worktrees/fancy-slug/myrepo"), + ).toEqual([]); + }); + + it("deletes via the stored path and leaves an external worktree's parent dirs untouched", async () => { + const base = mkTemp("wt-base-"); + mocks.workspaceSettings.getWorktreeLocation = () => base; + + const repoPath = "/code/myrepo"; + const managedParent = path.join(base, "myrepo"); + fs.mkdirSync(managedParent); + const externalPath = mkTemp("external-wt-"); + + seedWorktreeTask(mocks, { + taskId: "ext", + repoPath, + name: "ext-name", + worktreePath: externalPath, + }); + + await service.deleteWorkspace("ext", repoPath); + + // The guard short-circuits managed-folder cleanup for external worktrees. + expect(fs.existsSync(managedParent)).toBe(true); + // The external worktree dir itself is removed by git (mocked), never by + // the managed-folder cleanup. + expect(fs.existsSync(externalPath)).toBe(true); + }); + + it("reclaims the empty managed parent folder for a worktree under the base path", async () => { + const base = mkTemp("wt-base-"); + mocks.workspaceSettings.getWorktreeLocation = () => base; + + const repoPath = "/code/myrepo"; + const managedParent = path.join(base, "myrepo"); + fs.mkdirSync(managedParent); + const managedPath = path.join(base, "some-name", "myrepo"); + + seedWorktreeTask(mocks, { + taskId: "mng", + repoPath, + name: "some-name", + worktreePath: managedPath, + }); + + await service.deleteWorkspace("mng", repoPath); + + // Same setup as the external case, but a managed path clears the guard, so + // the empty parent folder is reclaimed. This proves the guard discriminates. + expect(fs.existsSync(managedParent)).toBe(false); + }); + }); }); diff --git a/packages/workspace-server/src/services/workspace/workspace.ts b/packages/workspace-server/src/services/workspace/workspace.ts index 35f645b79b..db6df4db56 100644 --- a/packages/workspace-server/src/services/workspace/workspace.ts +++ b/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,19 @@ 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); + return rel !== "" && !rel.startsWith("..") && !path.isAbsolute(rel); +} + export const WorkspaceServiceEvent = { Error: "error", Warning: "warning", @@ -139,14 +147,6 @@ export class WorkspaceService extends TypedEventEmitter private creatingWorkspaces = new Map>(); 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 +169,7 @@ export class WorkspaceService extends TypedEventEmitter folderId: workspace.repositoryId, mode: "worktree", worktree: worktree.name, + path: worktree.path, branchName: null, }; } @@ -209,6 +210,7 @@ export class WorkspaceService extends TypedEventEmitter folderId: workspace.repositoryId, mode: "worktree", worktree: worktree.name, + path: worktree.path, branchName: null, }); } else { @@ -250,10 +252,7 @@ export class WorkspaceService extends TypedEventEmitter 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 +276,7 @@ export class WorkspaceService extends TypedEventEmitter 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 +716,7 @@ export class WorkspaceService extends TypedEventEmitter 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 +738,7 @@ export class WorkspaceService extends TypedEventEmitter ); if (otherWorkspacesForFolder.length === 0) { - await this.cleanupRepoWorktreeFolder(folderPath); + await this.cleanupRepoWorktreeFolder(folderPath, worktreePath); } } @@ -760,8 +755,19 @@ export class WorkspaceService extends TypedEventEmitter this.workspaceRepo.deleteByTaskId(taskId); } - private async cleanupRepoWorktreeFolder(folderPath: string): Promise { + private async cleanupRepoWorktreeFolder( + folderPath: string, + worktreePath: string, + ): Promise { const worktreeBasePath = this.workspaceSettings.getWorktreeLocation(); + + // Safety check 0: Only reclaim the managed `/` 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 +844,7 @@ export class WorkspaceService extends TypedEventEmitter } 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 +887,7 @@ export class WorkspaceService extends TypedEventEmitter 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") { @@ -934,10 +937,7 @@ export class WorkspaceService extends TypedEventEmitter if (association.mode === "worktree") { if (folderPath) { - const worktreePath = this.deriveWorktreePath( - folderPath, - association.worktree, - ); + const worktreePath = association.path; const gitBranch = await getBranchFromPath(worktreePath); branchName = gitBranch ?? association.branchName; worktreeInfo = { @@ -994,7 +994,7 @@ export class WorkspaceService extends TypedEventEmitter if (assoc.mode === "worktree") { worktreeName = assoc.worktree; - worktreePath = this.deriveWorktreePath(folderPath, worktreeName); + worktreePath = assoc.path; } let branchName: string | null = null; @@ -1118,10 +1118,7 @@ export class WorkspaceService extends TypedEventEmitter 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 }); } } From 7b31c2299b4d3408ca0fa4e59fd926913e5493c0 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Tue, 16 Jun 2026 13:19:43 -0700 Subject: [PATCH 2/4] refactor(worktree): tidy worktree-path resolution after review Fold the now-redundant folderPath guard in getWorktreeInfo's worktree branch into the branch condition, matching the sibling local branch. Parameterise the two delete-guard tests into a single it.each table since they share setup and differ only by whether the stored path is under the base path. --- .../src/services/workspace/workspace.test.ts | 78 +++++++++---------- .../src/services/workspace/workspace.ts | 24 +++--- 2 files changed, 46 insertions(+), 56 deletions(-) diff --git a/packages/workspace-server/src/services/workspace/workspace.test.ts b/packages/workspace-server/src/services/workspace/workspace.test.ts index 409c54611c..128483fa8a 100644 --- a/packages/workspace-server/src/services/workspace/workspace.test.ts +++ b/packages/workspace-server/src/services/workspace/workspace.test.ts @@ -367,52 +367,44 @@ describe("WorkspaceService", () => { ).toEqual([]); }); - it("deletes via the stored path and leaves an external worktree's parent dirs untouched", async () => { - const base = mkTemp("wt-base-"); - mocks.workspaceSettings.getWorktreeLocation = () => base; - - const repoPath = "/code/myrepo"; - const managedParent = path.join(base, "myrepo"); - fs.mkdirSync(managedParent); - const externalPath = mkTemp("external-wt-"); - - seedWorktreeTask(mocks, { - taskId: "ext", - repoPath, - name: "ext-name", - worktreePath: externalPath, - }); - - await service.deleteWorkspace("ext", repoPath); - - // The guard short-circuits managed-folder cleanup for external worktrees. - expect(fs.existsSync(managedParent)).toBe(true); - // The external worktree dir itself is removed by git (mocked), never by - // the managed-folder cleanup. - expect(fs.existsSync(externalPath)).toBe(true); - }); - - it("reclaims the empty managed parent folder for a worktree under the base path", async () => { - const base = mkTemp("wt-base-"); - mocks.workspaceSettings.getWorktreeLocation = () => base; + // Identical setup (empty managed `/` 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 an external worktree's parent dirs untouched", + 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); - const managedPath = path.join(base, "some-name", "myrepo"); + const repoPath = "/code/myrepo"; + const managedParent = path.join(base, "myrepo"); + fs.mkdirSync(managedParent); - seedWorktreeTask(mocks, { - taskId: "mng", - repoPath, - name: "some-name", - worktreePath: managedPath, - }); + seedWorktreeTask(mocks, { + taskId: "task", + repoPath, + name: "some-name", + worktreePath: makeWorktreePath(base), + }); - await service.deleteWorkspace("mng", repoPath); + await service.deleteWorkspace("task", repoPath); - // Same setup as the external case, but a managed path clears the guard, so - // the empty parent folder is reclaimed. This proves the guard discriminates. - expect(fs.existsSync(managedParent)).toBe(false); - }); + expect(fs.existsSync(managedParent)).toBe(managedParentSurvives); + }, + ); }); }); diff --git a/packages/workspace-server/src/services/workspace/workspace.ts b/packages/workspace-server/src/services/workspace/workspace.ts index db6df4db56..d9cf142a42 100644 --- a/packages/workspace-server/src/services/workspace/workspace.ts +++ b/packages/workspace-server/src/services/workspace/workspace.ts @@ -935,19 +935,17 @@ export class WorkspaceService extends TypedEventEmitter let worktreeInfo: WorktreeInfo | null = null; let branchName: string | null = null; - if (association.mode === "worktree") { - if (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(), - }; - } + 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); } From 17911829aa3066cca79077fabce6de9a1910433c Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Tue, 16 Jun 2026 15:51:51 -0700 Subject: [PATCH 3/4] Add test for worktree verification with external paths and tidy comments Adds test case verifying that verifyWorkspaceExists correctly uses the stored external path rather than deriving it from the worktree name. Also improves comments in cleanupRepoWorktreeFolder for clarity. --- .../src/services/workspace/workspace.test.ts | 25 ++++++++++++++++++- .../src/services/workspace/workspace.ts | 12 ++++++--- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/packages/workspace-server/src/services/workspace/workspace.test.ts b/packages/workspace-server/src/services/workspace/workspace.test.ts index 128483fa8a..8161b10186 100644 --- a/packages/workspace-server/src/services/workspace/workspace.test.ts +++ b/packages/workspace-server/src/services/workspace/workspace.test.ts @@ -367,13 +367,36 @@ describe("WorkspaceService", () => { ).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 + // // 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 `/` 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 an external worktree's parent dirs untouched", + label: + "leaves the managed parent folder alone for an external worktree", makeWorktreePath: () => mkTemp("external-wt-"), managedParentSurvives: true, }, diff --git a/packages/workspace-server/src/services/workspace/workspace.ts b/packages/workspace-server/src/services/workspace/workspace.ts index d9cf142a42..6102383513 100644 --- a/packages/workspace-server/src/services/workspace/workspace.ts +++ b/packages/workspace-server/src/services/workspace/workspace.ts @@ -755,15 +755,21 @@ export class WorkspaceService extends TypedEventEmitter this.workspaceRepo.deleteByTaskId(taskId); } + /** + * Reclaims the managed `/` 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 { const worktreeBasePath = this.workspaceSettings.getWorktreeLocation(); - // Safety check 0: Only reclaim the managed `/` parent folder for - // worktrees that actually live under the managed base path. Externally - // located worktrees never created it, so its contents are unrelated. + // Only reclaim the managed `/` 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; } From 63f532c9409abd1a131100a19ddb1481bdbc06fa Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Wed, 17 Jun 2026 13:24:24 -0700 Subject: [PATCH 4/4] Fix isPathUnder false negative for child paths starting with .. --- .../workspace-server/src/services/workspace/workspace.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/workspace-server/src/services/workspace/workspace.ts b/packages/workspace-server/src/services/workspace/workspace.ts index 6102383513..84f30b5da7 100644 --- a/packages/workspace-server/src/services/workspace/workspace.ts +++ b/packages/workspace-server/src/services/workspace/workspace.ts @@ -89,7 +89,10 @@ type TaskAssociation = /** True when `child` resolves to a path strictly inside `parent`. */ function isPathUnder(child: string, parent: string): boolean { const rel = path.relative(parent, child); - return rel !== "" && !rel.startsWith("..") && !path.isAbsolute(rel); + const up = `..${path.sep}`; + return ( + rel !== "" && rel !== ".." && !rel.startsWith(up) && !path.isAbsolute(rel) + ); } export const WorkspaceServiceEvent = {