diff --git a/packages/workspace-server/src/services/workspace/workspace.test.ts b/packages/workspace-server/src/services/workspace/workspace.test.ts index 351b1f33cf..8161b10186 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,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 //; 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 + // // 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 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); + }, + ); + }); }); diff --git a/packages/workspace-server/src/services/workspace/workspace.ts b/packages/workspace-server/src/services/workspace/workspace.ts index 35f645b79b..84f30b5da7 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,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 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 +172,7 @@ export class WorkspaceService extends TypedEventEmitter folderId: workspace.repositoryId, mode: "worktree", worktree: worktree.name, + path: worktree.path, branchName: null, }; } @@ -209,6 +213,7 @@ export class WorkspaceService extends TypedEventEmitter folderId: workspace.repositoryId, mode: "worktree", worktree: worktree.name, + path: worktree.path, branchName: null, }); } else { @@ -250,10 +255,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 +279,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 +719,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 +741,7 @@ export class WorkspaceService extends TypedEventEmitter ); if (otherWorkspacesForFolder.length === 0) { - await this.cleanupRepoWorktreeFolder(folderPath); + await this.cleanupRepoWorktreeFolder(folderPath, worktreePath); } } @@ -760,8 +758,25 @@ export class WorkspaceService extends TypedEventEmitter this.workspaceRepo.deleteByTaskId(taskId); } - private async cleanupRepoWorktreeFolder(folderPath: string): Promise { + /** + * 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(); + + // 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 +853,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 +896,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") { @@ -932,22 +944,17 @@ export class WorkspaceService extends TypedEventEmitter 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 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 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 }); } }