[codex] Fix worktree delete cleanup ordering

packages/cli/src/commands/workspaces/delete/command.ts

@@ -22,6 +22,7 @@ export default command({
 		});
 
 		const deleted: string[] = [];
+		const warnings: string[] = [];
 		for (const id of ids) {
 			let hostId = explicitHostId;
 			if (!hostId) {
@@ -41,16 +42,23 @@ export default command({
 				userJwt: ctx.bearer,
 			});
 
-			await target.client.workspace.delete.mutate({ id });
+			const result = await target.client.workspace.delete.mutate({ id });
 			deleted.push(id);
+			for (const warning of result.warnings ?? []) {
+				warnings.push(`${id}: ${warning}`);
+			}
 		}
 
+		const deleteMessage =
+			deleted.length === 1
+				? `Deleted workspace ${deleted[0]}`
+				: `Deleted ${deleted.length} workspaces`;
 		return {
-			data: { deleted },
+			data: { deleted, warnings },
 			message:
-				deleted.length === 1
-					? `Deleted workspace ${deleted[0]}`
-					: `Deleted ${deleted.length} workspaces`,
+				warnings.length > 0
+					? `${deleteMessage}\nWarnings:\n${warnings.map((warning) => `- ${warning}`).join("\n")}`
+					: deleteMessage,
 		};
 	},
 });

packages/host-service/src/trpc/router/workspace-cleanup/index.ts

@@ -1 +1 @@
-export { workspaceCleanupRouter } from "./workspace-cleanup";
+export { destroyWorkspace, workspaceCleanupRouter } from "./workspace-cleanup";

packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts

@@ -29,7 +29,7 @@ const destroysInFlight = new Set<string>();
 /** @internal — exposed for tests to introspect / clear the guard. */
 export const __testDestroysInFlight = destroysInFlight;
 
-interface DestroyInput {
+export interface DestroyWorkspaceInput {
 	workspaceId: string;
 	deleteBranch: boolean;
 	force: boolean;
@@ -125,22 +125,24 @@ export const workspaceCleanupRouter = router({
 		}),
 
 	/**
-	 * Destroy a workspace in three phases:
+	 * Destroy a workspace in five phases:
 	 *
 	 *   0. Preflight     — dirty-worktree check (skip if force)
 	 *   1. Teardown      — run .superset/teardown.sh (skip if force)
-	 *   2. Cloud delete  ← COMMIT POINT — throws if it fails
-	 *   3. Local cleanup — PTYs, worktree, branch, host sqlite (best-effort)
+	 *   2. Local cleanup — PTYs, worktree
+	 *   3. Cloud delete  ← authoritative UI state
+	 *   4. Branch delete — optional local branch cleanup
+	 *   5. Host sqlite   — local index cleanup
 	 *
-	 * Any failure in phases 0–2 leaves the workspace fully intact. Failures
-	 * in phase 3 become warnings — local orphans are cheap, and the user
-	 * has a toast telling them what was left behind.
+	 * Worktree removal is intentionally before cloud delete. If it fails
+	 * while the path still exists, the cloud row remains so the workspace is
+	 * still visible and delete can be retried instead of orphaning disk state.
 	 *
 	 * Force semantics:
 	 *   - skips preflight (step 0)
 	 *   - skips teardown  (step 1)
-	 *   - step 3b always uses `--force` (we're past the commit point)
-	 *   - step 3c always uses `-D` regardless: the `deleteBranch`
+	 *   - step 2b always uses `--force --force`
+	 *   - step 4 always uses `-D` regardless: the `deleteBranch`
 	 *     checkbox is the user's consent, so refusing unmerged branches
 	 *     would just silently drop the opt-in.
 	 *
@@ -164,24 +166,32 @@ export const workspaceCleanupRouter = router({
 				force: z.boolean().default(false),
 			}),
 		)
-		.mutation(async ({ ctx, input }) => {
-			if (destroysInFlight.has(input.workspaceId)) {
-				throw new TRPCError({
-					code: "CONFLICT",
-					message: "Deletion already in progress for this workspace",
-					cause: { kind: "DELETE_IN_PROGRESS" } satisfies DeleteInProgressCause,
-				});
-			}
-			destroysInFlight.add(input.workspaceId);
-			try {
-				return await runDestroy(ctx, input);
-			} finally {
-				destroysInFlight.delete(input.workspaceId);
-			}
-		}),
+		.mutation(async ({ ctx, input }) => destroyWorkspace(ctx, input)),
 });
 
-async function runDestroy(ctx: HostServiceContext, input: DestroyInput) {
+export async function destroyWorkspace(
+	ctx: HostServiceContext,
+	input: DestroyWorkspaceInput,
+) {
+	if (destroysInFlight.has(input.workspaceId)) {
+		throw new TRPCError({
+			code: "CONFLICT",
+			message: "Deletion already in progress for this workspace",
+			cause: { kind: "DELETE_IN_PROGRESS" } satisfies DeleteInProgressCause,
+		});
+	}
+	destroysInFlight.add(input.workspaceId);
+	try {
+		return await runDestroy(ctx, input);
+	} finally {
+		destroysInFlight.delete(input.workspaceId);
+	}
+}
+
+async function runDestroy(
+	ctx: HostServiceContext,
+	input: DestroyWorkspaceInput,
+) {
 	const warnings: string[] = [];
 
 	// `isMainWorkspace` already loads workspace + project rows from sqlite;
@@ -194,7 +204,7 @@ async function runDestroy(ctx: HostServiceContext, input: DestroyInput) {
 
 	// ─── Step 0: Preflight ─────────────────────────────────────────
 	// Block only on dirty worktree (the common "I forgot to commit"
-	// case). Anything else the local-cleanup phase handles as warning.
+	// case). Missing/broken local state is handled by the cleanup phase.
 	if (!input.force && local && project) {
 		try {
 			const git = await ctx.git(local.worktreePath);
@@ -212,6 +222,15 @@ async function runDestroy(ctx: HostServiceContext, input: DestroyInput) {
 		}
 	}
 
+	// Make sure we can commit to cloud before touching local disk. The actual
+	// cloud mutation happens after the worktree is removed.
+	if (!ctx.api) {
+		throw new TRPCError({
+			code: "PRECONDITION_FAILED",
+			message: "Cloud API not configured",
+		});
+	}
+
 	// ─── Step 1: Teardown ──────────────────────────────────────────
 	// Script is the user's last chance to stop services / flush state
 	// before the workspace goes away. Failure here is recoverable
@@ -238,23 +257,8 @@ async function runDestroy(ctx: HostServiceContext, input: DestroyInput) {
 		}
 	}
 
-	// ─── Step 2: Cloud delete (commit point) ───────────────────────
-	// Past this line, the workspace is gone from the user's perspective
-	// (sidebar will reflect the cloud state). Local artifacts become
-	// cleanup debris — never a source of truth.
-	if (!ctx.api) {
-		throw new TRPCError({
-			code: "PRECONDITION_FAILED",
-			message: "Cloud API not configured",
-		});
-	}
-	await ctx.api.v2Workspace.delete.mutate({ id: input.workspaceId });
-
-	// ─── Step 3: Local cleanup (best-effort) ───────────────────────
-	// Every failure in this phase is captured as a warning; the
-	// caller always sees success.
-
-	// 3a. PTYs
+	// ─── Step 2: Local cleanup ─────────────────────────────────────
+	// 2a. PTYs
 	try {
 		const killed = await disposeSessionsByWorkspaceId(
 			input.workspaceId,
@@ -268,21 +272,31 @@ async function runDestroy(ctx: HostServiceContext, input: DestroyInput) {
 		warnings.push(`Failed to dispose terminal sessions: ${message}`);
 	}
 
-	// 3b. Worktree (always --force --force: we're past the commit point,
-	//     and double-force unlocks the rare locked-worktree case the user
-	//     can hit by manually `rm -rf`-ing a worktree that ended up locked.)
-	// 3c. Optional branch delete
+	// 2b. Worktree. Double-force unlocks the rare locked-worktree case and
+	//     clears stale metadata when the directory was manually removed.
 	let worktreeRemoved = false;
 	let branchDeleted = false;
+	let git: Awaited<ReturnType<typeof ctx.git>> | null = null;
+	if (local && !project) {
+		worktreeRemoved = !existsSync(local.worktreePath);
+		if (!worktreeRemoved) {
+			warnings.push(
+				`Skipped worktree removal at ${local.worktreePath}: project metadata is missing`,
+			);
+		}
+	}
 	if (local && project) {
-		// Past the commit point — every failure here is a warning, including
-		// failure to even open the repo. Letting `ctx.git` escape would surface
-		// as a hard error for a workspace that's already been deleted in cloud.
-		let git: Awaited<ReturnType<typeof ctx.git>> | null = null;
+		worktreeRemoved = !existsSync(local.worktreePath);
 		try {
 			git = await ctx.git(project.repoPath);
 		} catch (err) {
 			const message = err instanceof Error ? err.message : String(err);
+			if (!worktreeRemoved) {
+				throw new TRPCError({
+					code: "INTERNAL_SERVER_ERROR",
+					message: `Failed to open project repo at ${project.repoPath}: ${message}`,
+				});
+			}
 			warnings.push(
 				`Failed to open project repo at ${project.repoPath}: ${message}`,
 			);
@@ -300,42 +314,35 @@ async function runDestroy(ctx: HostServiceContext, input: DestroyInput) {
 				worktreeRemoved = true;
 			} catch (err) {
 				const message = err instanceof Error ? err.message : String(err);
-				// If the worktree dir is already gone, the user's goal is met
-				// regardless of what git complains about — locale-translated
-				// messages, future git phrasing, or "locked working tree" with
-				// the dir already rm'd. The substring matcher below stays as
-				// belt-and-braces for the rare race where the dir disappears
-				// between this check and the git invocation, but `existsSync`
-				// is the authoritative signal.
 				if (!existsSync(local.worktreePath)) {
 					worktreeRemoved = true;
-				} else if (
-					message.includes("is not a working tree") ||
-					message.includes("No such file or directory") ||
-					message.includes("does not exist") ||
-					message.includes("ENOENT")
-				) {
-					worktreeRemoved = true;
 				} else {
-					warnings.push(
-						`Failed to remove worktree at ${local.worktreePath}: ${message}`,
-					);
+					throw new TRPCError({
+						code: "INTERNAL_SERVER_ERROR",
+						message: `Failed to remove worktree at ${local.worktreePath}: ${message}`,
+					});
 				}
 			}
+		}
+	}
 
-			if (input.deleteBranch && local.branch) {
-				try {
-					await git.raw(["branch", "-D", local.branch]);
-					branchDeleted = true;
-				} catch (err) {
-					const message = err instanceof Error ? err.message : String(err);
-					warnings.push(`Failed to delete branch ${local.branch}: ${message}`);
-				}
-			}
+	// ─── Step 3: Cloud delete ──────────────────────────────────────
+	await ctx.api.v2Workspace.delete.mutate({ id: input.workspaceId });
+
+	// ─── Step 4: Optional branch delete ────────────────────────────
+	// Keep this after the cloud commit point. If cloud delete fails, the
+	// workspace stays visible/retryable and the branch pointer remains intact.
+	if (git && local?.branch && input.deleteBranch) {
+		try {
+			await git.raw(["branch", "-D", local.branch]);
+			branchDeleted = true;
+		} catch (err) {
+			const message = err instanceof Error ? err.message : String(err);
+			warnings.push(`Failed to delete branch ${local.branch}: ${message}`);
 		}
 	}
 
-	// 3d. Host sqlite row
+	// ─── Step 5: Host sqlite row ───────────────────────────────────
 	if (local) {
 		try {
 			ctx.db

packages/host-service/src/trpc/router/workspace/workspace.ts

@@ -2,9 +2,9 @@ import { existsSync } from "node:fs";
 import { TRPCError } from "@trpc/server";
 import { eq } from "drizzle-orm";
 import { z } from "zod";
-import { projects, workspaces } from "../../../db/schema";
-import { invalidateLabelCache } from "../../../ports/static-ports";
+import { workspaces } from "../../../db/schema";
 import { protectedProcedure, router } from "../../index";
+import { destroyWorkspace } from "../workspace-cleanup";
 
 export const workspaceRouter = router({
 	get: protectedProcedure
@@ -71,66 +71,12 @@ export const workspaceRouter = router({
 	delete: protectedProcedure
 		.input(z.object({ id: z.string() }))
 		.mutation(async ({ ctx, input }) => {
-			if (!ctx.api) {
-				throw new TRPCError({
-					code: "PRECONDITION_FAILED",
-					message: "Cloud API not configured",
-				});
-			}
-
-			const localWorkspace = ctx.db.query.workspaces
-				.findFirst({ where: eq(workspaces.id, input.id) })
-				.sync();
-			const localProject = localWorkspace
-				? ctx.db.query.projects
-						.findFirst({ where: eq(projects.id, localWorkspace.projectId) })
-						.sync()
-				: undefined;
-
-			if (
-				localWorkspace &&
-				localProject &&
-				localWorkspace.worktreePath === localProject.repoPath
-			) {
-				throw new TRPCError({
-					code: "BAD_REQUEST",
-					message:
-						"Main workspaces cannot be deleted. Remove them from the sidebar or remove the project from this host instead.",
-				});
-			}
-
-			const cloudWorkspace = await ctx.api.v2Workspace.getFromHost.query({
-				organizationId: ctx.organizationId,
-				id: input.id,
+			// Legacy external surface used by CLI/SDK/MCP. Preserve its
+			// non-interactive contract while reusing the v2 cleanup path.
+			return destroyWorkspace(ctx, {
+				workspaceId: input.id,
+				deleteBranch: false,
+				force: true,
 			});
-			if (cloudWorkspace?.type === "main") {
-				throw new TRPCError({
-					code: "BAD_REQUEST",
-					message:
-						"Main workspaces cannot be deleted. Remove them from the sidebar or remove the project from this host instead.",
-				});
-			}
-
-			await ctx.api.v2Workspace.delete.mutate({ id: input.id });
-
-			if (localWorkspace) {
-				if (localProject) {
-					try {
-						const git = await ctx.git(localProject.repoPath);
-						await git.raw(["worktree", "remove", localWorkspace.worktreePath]);
-					} catch (err) {
-						console.warn("[workspace.delete] failed to remove worktree", {
-							workspaceId: input.id,
-							worktreePath: localWorkspace.worktreePath,
-							err,
-						});
-					}
-				}
-			}
-
-			ctx.db.delete(workspaces).where(eq(workspaces.id, input.id)).run();
-			invalidateLabelCache(input.id);
-
-			return { success: true };
 		}),
 });

packages/host-service/test/integration/bug-hunt-2.integration.test.ts

@@ -235,7 +235,8 @@ describe("bug-hunt-2: partial-failure consistency", () => {
 			.run();
 
 		const result = await host.trpc.workspace.delete.mutate({ id: workspaceId });
-		expect(result).toEqual({ success: true });
+		expect(result.success).toBe(true);
+		expect(result.worktreeRemoved).toBe(true);
 
 		const remaining = host.db
 			.select()