From 907e291120db7ba1e8708a567e12eebb04e42929 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 12 Nov 2025 22:19:45 +0000 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=A4=96=20feat:=20allow=20slashes=20in?= =?UTF-8?q?=20branch=20names=20with=20directory=20sanitization?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add sanitization utility that converts slashes to dashes for directory names - Update validation to allow forward slashes (but still reject backslashes) - Add conflict detection for cases like feature/foo vs feature-foo - Update runtime implementations to use sanitized directory names - Handle no-op renames when sanitized names are identical - Add comprehensive unit and integration tests Users can now create workspaces with names like feature/new-ui, bugfix/issue-123, docs/api/endpoints, etc. Git operations use the actual branch name (with slashes) while filesystem operations use the sanitized version (slashes → dashes). _Generated with `cmux`_ --- src/runtime/LocalRuntime.ts | 25 +- src/runtime/Runtime.ts | 6 +- src/runtime/SSHRuntime.ts | 6 +- src/services/ipcMain.ts | 87 ++++++- .../validation/workspaceValidation.test.ts | 14 +- src/utils/validation/workspaceValidation.ts | 9 +- src/utils/workspace/directoryName.test.ts | 89 +++++++ src/utils/workspace/directoryName.ts | 36 +++ tests/ipcMain/workspaceSlashes.test.ts | 246 ++++++++++++++++++ 9 files changed, 493 insertions(+), 25 deletions(-) create mode 100644 src/utils/workspace/directoryName.test.ts create mode 100644 src/utils/workspace/directoryName.ts create mode 100644 tests/ipcMain/workspaceSlashes.test.ts diff --git a/src/runtime/LocalRuntime.ts b/src/runtime/LocalRuntime.ts index e64c162ab..e56441538 100644 --- a/src/runtime/LocalRuntime.ts +++ b/src/runtime/LocalRuntime.ts @@ -25,6 +25,7 @@ import { execAsync, DisposableProcess } from "../utils/disposableExec"; import { getProjectName } from "../utils/runtime/helpers"; import { getErrorMessage } from "../utils/errors"; import { expandTilde } from "./tildeExpansion"; +import { sanitizeBranchNameForDirectory } from "../utils/workspace/directoryName"; /** * Local runtime implementation that executes commands and file operations @@ -310,11 +311,11 @@ export class LocalRuntime implements Runtime { } async createWorkspace(params: WorkspaceCreationParams): Promise { - const { projectPath, branchName, trunkBranch, initLogger } = params; + const { projectPath, branchName, trunkBranch, directoryName, initLogger } = params; try { - // Compute workspace path using the canonical method - const workspacePath = this.getWorkspacePath(projectPath, branchName); + // Compute workspace path using the sanitized directory name + const workspacePath = this.getWorkspacePath(projectPath, directoryName); initLogger.logStep("Creating git worktree..."); // Create parent directory if needed @@ -451,11 +452,21 @@ export class LocalRuntime implements Runtime { { success: true; oldPath: string; newPath: string } | { success: false; error: string } > { // Note: _abortSignal ignored for local operations (fast, no need for cancellation) - // Compute workspace paths using canonical method - const oldPath = this.getWorkspacePath(projectPath, oldName); - const newPath = this.getWorkspacePath(projectPath, newName); + // Compute workspace paths using sanitized directory names + const oldDirName = sanitizeBranchNameForDirectory(oldName); + const newDirName = sanitizeBranchNameForDirectory(newName); + const oldPath = this.getWorkspacePath(projectPath, oldDirName); + const newPath = this.getWorkspacePath(projectPath, newDirName); try { + // Create parent directory for new path if needed (for nested directory names) + const newParentDir = path.dirname(newPath); + try { + await fsPromises.access(newParentDir); + } catch { + await fsPromises.mkdir(newParentDir, { recursive: true }); + } + // Use git worktree move to rename the worktree directory // This updates git's internal worktree metadata correctly using proc = execAsync(`git -C "${projectPath}" worktree move "${oldPath}" "${newPath}"`); @@ -593,7 +604,7 @@ export class LocalRuntime implements Runtime { projectPath, branchName: newWorkspaceName, trunkBranch: sourceBranch, // Fork from source branch instead of main/master - directoryName: newWorkspaceName, + directoryName: sanitizeBranchNameForDirectory(newWorkspaceName), initLogger, }); diff --git a/src/runtime/Runtime.ts b/src/runtime/Runtime.ts index 610a3057a..33a60fe2d 100644 --- a/src/runtime/Runtime.ts +++ b/src/runtime/Runtime.ts @@ -115,7 +115,11 @@ export interface WorkspaceCreationParams { branchName: string; /** Trunk branch to base new branches on */ trunkBranch: string; - /** Directory name to use for workspace (typically branch name) */ + /** + * Directory name to use for workspace (sanitized branch name). + * This is the filesystem-safe version with slashes converted to dashes. + * ALWAYS computed via sanitizeBranchNameForDirectory(), never stored. + */ directoryName: string; /** Logger for streaming creation progress and init hook output */ initLogger: InitLogger; diff --git a/src/runtime/SSHRuntime.ts b/src/runtime/SSHRuntime.ts index 138a064cb..d7f087daa 100644 --- a/src/runtime/SSHRuntime.ts +++ b/src/runtime/SSHRuntime.ts @@ -785,9 +785,9 @@ export class SSHRuntime implements Runtime { async createWorkspace(params: WorkspaceCreationParams): Promise { try { - const { projectPath, branchName, initLogger, abortSignal } = params; - // Compute workspace path using canonical method - const workspacePath = this.getWorkspacePath(projectPath, branchName); + const { projectPath, branchName, directoryName, initLogger, abortSignal } = params; + // Compute workspace path using sanitized directory name + const workspacePath = this.getWorkspacePath(projectPath, directoryName); // Prepare parent directory for git clone (fast - returns immediately) // Note: git clone will create the workspace directory itself during initWorkspace, diff --git a/src/services/ipcMain.ts b/src/services/ipcMain.ts index eaf06ed6e..32d6c71db 100644 --- a/src/services/ipcMain.ts +++ b/src/services/ipcMain.ts @@ -19,6 +19,10 @@ import type { SendMessageError } from "@/types/errors"; import type { SendMessageOptions, DeleteMessage } from "@/types/ipc"; import { Ok, Err } from "@/types/result"; import { validateWorkspaceName } from "@/utils/validation/workspaceValidation"; +import { + sanitizeBranchNameForDirectory, + detectDirectoryNameConflict, +} from "@/utils/workspace/directoryName"; import type { WorkspaceMetadata } from "@/types/workspace"; import { createBashTool } from "@/services/tools/bash"; import type { BashToolResult } from "@/types/tools"; @@ -220,6 +224,23 @@ export class IpcMain { return { success: false, error: validation.error }; } + // Check for directory name conflicts with existing workspaces + const config = this.config.loadConfigOrDefault(); + const projectConfig = config.projects.get(projectPath); + if (projectConfig) { + const existingBranchNames = projectConfig.workspaces + .map((ws) => ws.name) + .filter((name): name is string => name !== undefined); + const conflict = detectDirectoryNameConflict(branchName, existingBranchNames); + if (conflict) { + const sanitizedName = sanitizeBranchNameForDirectory(branchName); + return { + success: false, + error: `Branch name "${branchName}" conflicts with existing workspace "${conflict}" (both use directory "${sanitizedName}")`, + }; + } + } + if (typeof trunkBranch !== "string" || trunkBranch.trim().length === 0) { return { success: false, error: "Trunk branch is required" }; } @@ -284,11 +305,12 @@ export class IpcMain { }; // Phase 1: Create workspace structure (FAST - returns immediately) + const directoryName = sanitizeBranchNameForDirectory(branchName); const createResult = await runtime.createWorkspace({ projectPath, branchName, trunkBranch: normalizedTrunkBranch, - directoryName: branchName, // Use branch name as directory name + directoryName, // Sanitized directory name (slashes → dashes) initLogger, }); @@ -405,6 +427,48 @@ export class IpcMain { return Ok({ newWorkspaceId: workspaceId }); } + // Find project path from config (needed for subsequent checks) + const workspace = this.config.findWorkspace(workspaceId); + if (!workspace) { + return Err("Failed to find workspace in config"); + } + const { projectPath } = workspace; + + // If the sanitized directory names are the same, it's also a no-op + // (e.g., renaming "feature-foo" to "feature/foo") + const oldDirName = sanitizeBranchNameForDirectory(oldName); + const newDirName = sanitizeBranchNameForDirectory(newName); + if (oldDirName === newDirName) { + // Still need to update the name in config even though directory stays the same + this.config.editConfig((config) => { + const projectConfig = config.projects.get(projectPath); + if (projectConfig) { + const workspaceEntry = projectConfig.workspaces.find((w) => w.id === workspaceId); + if (workspaceEntry) { + workspaceEntry.name = newName; + } + } + return config; + }); + + // Get updated metadata and emit event + const allMetadata = this.config.getAllWorkspaceMetadata(); + const updatedMetadata = allMetadata.find((m) => m.id === workspaceId); + if (updatedMetadata) { + const session = this.sessions.get(workspaceId); + if (session) { + session.emitMetadata(updatedMetadata); + } else if (this.mainWindow) { + this.mainWindow.webContents.send(IPC_CHANNELS.WORKSPACE_METADATA, { + workspaceId, + metadata: updatedMetadata, + }); + } + } + + return Ok({ newWorkspaceId: workspaceId }); + } + // Check if new name collides with existing workspace name or ID const allWorkspaces = this.config.getAllWorkspaceMetadata(); const collision = allWorkspaces.find( @@ -414,12 +478,23 @@ export class IpcMain { return Err(`Workspace with name "${newName}" already exists`); } - // Find project path from config - const workspace = this.config.findWorkspace(workspaceId); - if (!workspace) { - return Err("Failed to find workspace in config"); + // Check for directory name conflicts with existing workspaces + const config = this.config.loadConfigOrDefault(); + const projectConfig = config.projects.get(projectPath); + if (projectConfig) { + // Exclude the workspace being renamed from conflict detection + const existingBranchNames = projectConfig.workspaces + .filter((ws) => ws.id !== workspaceId) + .map((ws) => ws.name) + .filter((name): name is string => name !== undefined); + const conflict = detectDirectoryNameConflict(newName, existingBranchNames); + if (conflict) { + const sanitizedName = sanitizeBranchNameForDirectory(newName); + return Err( + `Branch name "${newName}" conflicts with existing workspace "${conflict}" (both use directory "${sanitizedName}")` + ); + } } - const { projectPath } = workspace; // Create runtime instance for this workspace // For local runtimes, workdir should be srcDir, not the individual workspace path diff --git a/src/utils/validation/workspaceValidation.test.ts b/src/utils/validation/workspaceValidation.test.ts index 6ea6a4a7b..a602ae75e 100644 --- a/src/utils/validation/workspaceValidation.test.ts +++ b/src/utils/validation/workspaceValidation.test.ts @@ -27,11 +27,19 @@ describe("validateWorkspaceName", () => { expect(validateWorkspaceName("a1-b2_c3").valid).toBe(true); }); + test("accepts forward slashes", () => { + expect(validateWorkspaceName("feature/foo").valid).toBe(true); + expect(validateWorkspaceName("feature/branch").valid).toBe(true); + expect(validateWorkspaceName("docs/bash-timeout-ux").valid).toBe(true); + expect(validateWorkspaceName("bugfix/issue-123").valid).toBe(true); + }); + test("accepts single character", () => { expect(validateWorkspaceName("a").valid).toBe(true); expect(validateWorkspaceName("1").valid).toBe(true); expect(validateWorkspaceName("_").valid).toBe(true); expect(validateWorkspaceName("-").valid).toBe(true); + expect(validateWorkspaceName("/").valid).toBe(true); }); test("accepts 64 characters", () => { @@ -72,12 +80,10 @@ describe("validateWorkspaceName", () => { expect(validateWorkspaceName("branch%123").valid).toBe(false); expect(validateWorkspaceName("branch!123").valid).toBe(false); expect(validateWorkspaceName("branch.123").valid).toBe(false); - expect(validateWorkspaceName("branch/123").valid).toBe(false); - expect(validateWorkspaceName("branch\\123").valid).toBe(false); }); - test("rejects names with slashes", () => { - expect(validateWorkspaceName("feature/branch").valid).toBe(false); + test("rejects backslashes", () => { + expect(validateWorkspaceName("branch\\123").valid).toBe(false); expect(validateWorkspaceName("path\\to\\branch").valid).toBe(false); }); }); diff --git a/src/utils/validation/workspaceValidation.ts b/src/utils/validation/workspaceValidation.ts index 345d41cd9..83b1d719b 100644 --- a/src/utils/validation/workspaceValidation.ts +++ b/src/utils/validation/workspaceValidation.ts @@ -1,8 +1,8 @@ /** * Validates workspace name format * - Must be 1-64 characters long - * - Can only contain: lowercase letters, digits, underscore, hyphen - * - Pattern: [a-z0-9_-]{1,64} + * - Can only contain: lowercase letters, digits, underscore, hyphen, forward slash + * - Pattern: [a-z0-9_/-]{1,64} */ export function validateWorkspaceName(name: string): { valid: boolean; error?: string } { if (!name || name.length === 0) { @@ -13,11 +13,12 @@ export function validateWorkspaceName(name: string): { valid: boolean; error?: s return { valid: false, error: "Workspace name cannot exceed 64 characters" }; } - const validPattern = /^[a-z0-9_-]+$/; + const validPattern = /^[a-z0-9_/-]+$/; if (!validPattern.test(name)) { return { valid: false, - error: "Workspace name can only contain lowercase letters, digits, underscore, and hyphen", + error: + "Workspace name can only contain lowercase letters, digits, underscore, hyphen, and forward slash", }; } diff --git a/src/utils/workspace/directoryName.test.ts b/src/utils/workspace/directoryName.test.ts new file mode 100644 index 000000000..fc46e3f82 --- /dev/null +++ b/src/utils/workspace/directoryName.test.ts @@ -0,0 +1,89 @@ +import { describe, expect, test } from "bun:test"; +import { sanitizeBranchNameForDirectory, detectDirectoryNameConflict } from "./directoryName"; + +describe("sanitizeBranchNameForDirectory", () => { + test("converts single slash to dash", () => { + expect(sanitizeBranchNameForDirectory("feature/foo")).toBe("feature-foo"); + }); + + test("converts multiple slashes to dashes", () => { + expect(sanitizeBranchNameForDirectory("feature/sub/foo")).toBe("feature-sub-foo"); + }); + + test("handles deep hierarchies", () => { + expect(sanitizeBranchNameForDirectory("feature/sub/sub2/foo")).toBe("feature-sub-sub2-foo"); + }); + + test("handles multiple consecutive slashes", () => { + expect(sanitizeBranchNameForDirectory("feature//foo")).toBe("feature--foo"); + }); + + test("handles leading slash", () => { + expect(sanitizeBranchNameForDirectory("/feature")).toBe("-feature"); + }); + + test("handles trailing slash", () => { + expect(sanitizeBranchNameForDirectory("feature/")).toBe("feature-"); + }); + + test("handles leading and trailing slashes", () => { + expect(sanitizeBranchNameForDirectory("/feature/foo/")).toBe("-feature-foo-"); + }); + + test("passes through names without slashes unchanged", () => { + expect(sanitizeBranchNameForDirectory("feature-foo")).toBe("feature-foo"); + expect(sanitizeBranchNameForDirectory("main")).toBe("main"); + expect(sanitizeBranchNameForDirectory("my_branch")).toBe("my_branch"); + }); + + test("handles real-world examples", () => { + expect(sanitizeBranchNameForDirectory("docs/bash-timeout-ux")).toBe("docs-bash-timeout-ux"); + expect(sanitizeBranchNameForDirectory("bugfix/issue-123")).toBe("bugfix-issue-123"); + }); +}); + +describe("detectDirectoryNameConflict", () => { + test("detects conflict between slash and dash versions", () => { + const conflict = detectDirectoryNameConflict("feature/foo", ["feature-foo"]); + expect(conflict).toBe("feature-foo"); + }); + + test("detects conflict in opposite direction", () => { + const conflict = detectDirectoryNameConflict("feature-foo", ["feature/foo"]); + expect(conflict).toBe("feature/foo"); + }); + + test("returns null when no conflict exists", () => { + const conflict = detectDirectoryNameConflict("feature/foo", ["feature/bar", "bugfix/baz"]); + expect(conflict).toBeNull(); + }); + + test("allows same name (not a conflict with itself)", () => { + const conflict = detectDirectoryNameConflict("feature/foo", ["feature/foo"]); + expect(conflict).toBeNull(); + }); + + test("detects complex hierarchy conflict", () => { + const conflict = detectDirectoryNameConflict("docs/bash-timeout-ux", ["docs-bash-timeout-ux"]); + expect(conflict).toBe("docs-bash-timeout-ux"); + }); + + test("handles multiple existing workspaces", () => { + const conflict = detectDirectoryNameConflict("feature/new", [ + "main", + "feature-new", + "bugfix/123", + ]); + expect(conflict).toBe("feature-new"); + }); + + test("returns first conflict found", () => { + const conflict = detectDirectoryNameConflict("a/b", ["a-b", "x-y"]); + expect(conflict).toBe("a-b"); + }); + + test("handles empty existing workspaces list", () => { + const conflict = detectDirectoryNameConflict("feature/foo", []); + expect(conflict).toBeNull(); + }); +}); diff --git a/src/utils/workspace/directoryName.ts b/src/utils/workspace/directoryName.ts new file mode 100644 index 000000000..dc1860e30 --- /dev/null +++ b/src/utils/workspace/directoryName.ts @@ -0,0 +1,36 @@ +/** + * Sanitize a branch name for use as a directory name. + * Converts forward slashes to dashes to make branch names filesystem-safe. + * + * This is the single source of truth for branch → directory name conversion. + * + * @param branchName - The git branch name (may contain slashes) + * @returns Sanitized directory name (slashes replaced with dashes) + */ +export function sanitizeBranchNameForDirectory(branchName: string): string { + return branchName.replace(/\//g, "-"); +} + +/** + * Detect if a new branch name would conflict with existing workspaces. + * Returns the name of the conflicting workspace if found, null otherwise. + * + * @param newBranchName - The branch name being created/renamed to + * @param existingBranchNames - List of existing workspace branch names + * @returns Name of conflicting branch, or null if no conflict + */ +export function detectDirectoryNameConflict( + newBranchName: string, + existingBranchNames: string[] +): string | null { + const newDirName = sanitizeBranchNameForDirectory(newBranchName); + + for (const existingName of existingBranchNames) { + const existingDirName = sanitizeBranchNameForDirectory(existingName); + if (newDirName === existingDirName && newBranchName !== existingName) { + return existingName; + } + } + + return null; +} diff --git a/tests/ipcMain/workspaceSlashes.test.ts b/tests/ipcMain/workspaceSlashes.test.ts new file mode 100644 index 000000000..330bc33c0 --- /dev/null +++ b/tests/ipcMain/workspaceSlashes.test.ts @@ -0,0 +1,246 @@ +/** + * Integration tests for workspace creation with slashes in branch names + * + * Verifies: + * - Branch names with slashes are properly sanitized to directory names + * - Conflict detection works between slash and dash versions + * - Git operations use the original branch name (with slashes) + * - Filesystem operations use the sanitized directory name (slashes → dashes) + */ + +import * as fs from "fs/promises"; +import * as path from "path"; +import { shouldRunIntegrationTests, createTestEnvironment, cleanupTestEnvironment } from "./setup"; +import type { TestEnvironment } from "./setup"; +import { IPC_CHANNELS } from "../../src/constants/ipc-constants"; +import { createTempGitRepo, cleanupTempGitRepo, generateBranchName } from "./helpers"; +import { detectDefaultTrunkBranch } from "../../src/git"; + +const TEST_TIMEOUT_MS = 60000; +const INIT_HOOK_WAIT_MS = 1500; // Wait for async init hook completion + +// Skip all tests if TEST_INTEGRATION is not set +const describeIntegration = shouldRunIntegrationTests() ? describe : describe.skip; + +describeIntegration("Workspace Creation with Slashes in Branch Names", () => { + let env: TestEnvironment; + let projectPath: string; + let trunkBranch: string; + + beforeAll(async () => { + env = await createTestEnvironment(); + projectPath = await createTempGitRepo(); + + // Detect trunk branch + const detectedTrunk = await detectDefaultTrunkBranch(projectPath); + trunkBranch = detectedTrunk || "main"; + }, TEST_TIMEOUT_MS); + + afterAll(async () => { + await cleanupTempGitRepo(projectPath); + await cleanupTestEnvironment(env); + }, TEST_TIMEOUT_MS); + + test( + "creates workspace with single slash in branch name", + async () => { + const branchName = `feature/${generateBranchName()}`; + const result = await env.mockIpcRenderer.invoke( + IPC_CHANNELS.WORKSPACE_CREATE, + projectPath, + branchName, + trunkBranch + ); + + expect(result.success).toBe(true); + expect(result.metadata).toBeDefined(); + expect(result.metadata.name).toBe(branchName); + + // Verify directory uses sanitized name (slash → dash) + const expectedDirName = branchName.replace(/\//g, "-"); + const workspacePath = result.metadata.namedWorkspacePath; + expect(workspacePath).toContain(expectedDirName); + + // Verify directory exists + await expect(fs.access(workspacePath)).resolves.not.toThrow(); + + // Wait for init hook to complete + await new Promise((resolve) => setTimeout(resolve, INIT_HOOK_WAIT_MS)); + }, + TEST_TIMEOUT_MS + ); + + test( + "creates workspace with multiple slashes in branch name", + async () => { + const branchName = `docs/api/${generateBranchName()}`; + const result = await env.mockIpcRenderer.invoke( + IPC_CHANNELS.WORKSPACE_CREATE, + projectPath, + branchName, + trunkBranch + ); + + expect(result.success).toBe(true); + expect(result.metadata).toBeDefined(); + expect(result.metadata.name).toBe(branchName); + + // Verify directory uses sanitized name (all slashes → dashes) + const expectedDirName = branchName.replace(/\//g, "-"); + const workspacePath = result.metadata.namedWorkspacePath; + expect(workspacePath).toContain(expectedDirName); + + await expect(fs.access(workspacePath)).resolves.not.toThrow(); + await new Promise((resolve) => setTimeout(resolve, INIT_HOOK_WAIT_MS)); + }, + TEST_TIMEOUT_MS + ); + + test( + "detects conflict between slash and dash versions", + async () => { + // Create workspace with dash in name + const baseName = generateBranchName(); + const dashName = `feature-${baseName}`; + const slashName = `feature/${baseName}`; + + // Create first workspace with dash + const result1 = await env.mockIpcRenderer.invoke( + IPC_CHANNELS.WORKSPACE_CREATE, + projectPath, + dashName, + trunkBranch + ); + expect(result1.success).toBe(true); + + // Try to create second workspace with slash (should conflict) + const result2 = await env.mockIpcRenderer.invoke( + IPC_CHANNELS.WORKSPACE_CREATE, + projectPath, + slashName, + trunkBranch + ); + + expect(result2.success).toBe(false); + expect(result2.error).toBeDefined(); + expect(result2.error).toContain("conflicts"); + expect(result2.error).toContain(dashName); + + await new Promise((resolve) => setTimeout(resolve, INIT_HOOK_WAIT_MS)); + }, + TEST_TIMEOUT_MS + ); + + test( + "detects conflict in opposite direction (dash conflicts with slash)", + async () => { + // Create workspace with slash in name + const baseName = generateBranchName(); + const slashName = `bugfix/${baseName}`; + const dashName = `bugfix-${baseName}`; + + // Create first workspace with slash + const result1 = await env.mockIpcRenderer.invoke( + IPC_CHANNELS.WORKSPACE_CREATE, + projectPath, + slashName, + trunkBranch + ); + expect(result1.success).toBe(true); + + // Try to create second workspace with dash (should conflict) + const result2 = await env.mockIpcRenderer.invoke( + IPC_CHANNELS.WORKSPACE_CREATE, + projectPath, + dashName, + trunkBranch + ); + + expect(result2.success).toBe(false); + expect(result2.error).toBeDefined(); + expect(result2.error).toContain("conflicts"); + expect(result2.error).toContain(slashName); + + await new Promise((resolve) => setTimeout(resolve, INIT_HOOK_WAIT_MS)); + }, + TEST_TIMEOUT_MS + ); + + test( + "allows non-conflicting slash and dash combinations", + async () => { + const baseName1 = generateBranchName(); + const baseName2 = generateBranchName(); + const name1 = `feature/${baseName1}`; + const name2 = `feature-${baseName2}`; // Different base, won't conflict + + const result1 = await env.mockIpcRenderer.invoke( + IPC_CHANNELS.WORKSPACE_CREATE, + projectPath, + name1, + trunkBranch + ); + expect(result1.success).toBe(true); + + const result2 = await env.mockIpcRenderer.invoke( + IPC_CHANNELS.WORKSPACE_CREATE, + projectPath, + name2, + trunkBranch + ); + expect(result2.success).toBe(true); + + await new Promise((resolve) => setTimeout(resolve, INIT_HOOK_WAIT_MS)); + }, + TEST_TIMEOUT_MS + ); + + test( + "renames workspace from dash to slash with conflict detection", + async () => { + // Create two workspaces + const baseName1 = generateBranchName(); + const baseName2 = generateBranchName(); + const originalName = `docs-${baseName1}`; + const existingName = `docs/${baseName2}`; + const conflictingName = `docs/${baseName1}`; // Would conflict after rename + + // Create first workspace + const result1 = await env.mockIpcRenderer.invoke( + IPC_CHANNELS.WORKSPACE_CREATE, + projectPath, + originalName, + trunkBranch + ); + expect(result1.success).toBe(true); + const workspaceId = result1.metadata.id; + + // Create second workspace with slash + const result2 = await env.mockIpcRenderer.invoke( + IPC_CHANNELS.WORKSPACE_CREATE, + projectPath, + existingName, + trunkBranch + ); + expect(result2.success).toBe(true); + + // Try to rename first workspace to a name that conflicts with second + // This shouldn't conflict since they sanitize to different directories + const renameResult = await env.mockIpcRenderer.invoke( + IPC_CHANNELS.WORKSPACE_RENAME, + workspaceId, + conflictingName + ); + + // This should succeed because baseName1 != baseName2 + // (docs-baseName1 → docs/baseName1, which sanitizes to docs-baseName1, no conflict) + if (!renameResult.success) { + console.log("Rename failed with error:", renameResult.error); + } + expect(renameResult.success).toBe(true); + + await new Promise((resolve) => setTimeout(resolve, INIT_HOOK_WAIT_MS)); + }, + TEST_TIMEOUT_MS + ); +}); From 7f5eb7eda2a4b6f45922c933de121cb8b16bf4c2 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 12 Nov 2025 22:50:18 +0000 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=A4=96=20fix:=20sanitize=20branch=20n?= =?UTF-8?q?ames=20in=20delete/rename=20operations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Codex review feedback: - LocalRuntime.deleteWorkspace now sanitizes workspaceName before computing path - SSHRuntime.renameWorkspace now sanitizes both old and new names - SSHRuntime.deleteWorkspace now sanitizes workspaceName before computing path Without sanitization, operations on workspaces with slashes would fail because the computed paths wouldn't match the actual directory names (which have slashes converted to dashes). _Generated with `cmux`_ --- src/runtime/LocalRuntime.ts | 5 +++-- src/runtime/SSHRuntime.ts | 17 ++++++++++++----- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/runtime/LocalRuntime.ts b/src/runtime/LocalRuntime.ts index e56441538..21b9b742c 100644 --- a/src/runtime/LocalRuntime.ts +++ b/src/runtime/LocalRuntime.ts @@ -490,8 +490,9 @@ export class LocalRuntime implements Runtime { // These are direct workspace directories (e.g., CLI/benchmark sessions), not git worktrees const isInPlace = projectPath === workspaceName; - // Compute workspace path using the canonical method - const deletedPath = this.getWorkspacePath(projectPath, workspaceName); + // Compute workspace path using sanitized directory name + const directoryName = sanitizeBranchNameForDirectory(workspaceName); + const deletedPath = this.getWorkspacePath(projectPath, directoryName); // Check if directory exists - if not, operation is idempotent try { diff --git a/src/runtime/SSHRuntime.ts b/src/runtime/SSHRuntime.ts index d7f087daa..5e8af721c 100644 --- a/src/runtime/SSHRuntime.ts +++ b/src/runtime/SSHRuntime.ts @@ -922,9 +922,13 @@ export class SSHRuntime implements Runtime { if (abortSignal?.aborted) { return { success: false, error: "Rename operation aborted" }; } - // Compute workspace paths using canonical method - const oldPath = this.getWorkspacePath(projectPath, oldName); - const newPath = this.getWorkspacePath(projectPath, newName); + // Import sanitization utility for directory name conversion + const { sanitizeBranchNameForDirectory } = await import("../utils/workspace/directoryName"); + const oldDirName = sanitizeBranchNameForDirectory(oldName); + const newDirName = sanitizeBranchNameForDirectory(newName); + // Compute workspace paths using sanitized directory names + const oldPath = this.getWorkspacePath(projectPath, oldDirName); + const newPath = this.getWorkspacePath(projectPath, newDirName); try { // SSH runtimes use plain directories, not git worktrees @@ -983,8 +987,11 @@ export class SSHRuntime implements Runtime { return { success: false, error: "Delete operation aborted" }; } - // Compute workspace path using canonical method - const deletedPath = this.getWorkspacePath(projectPath, workspaceName); + // Import sanitization utility for directory name conversion + const { sanitizeBranchNameForDirectory } = await import("../utils/workspace/directoryName"); + const directoryName = sanitizeBranchNameForDirectory(workspaceName); + // Compute workspace path using sanitized directory name + const deletedPath = this.getWorkspacePath(projectPath, directoryName); try { // Combine all pre-deletion checks into a single bash script to minimize round trips From 94bdfef965cea71d305bb5be0441fd05068564d3 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 12 Nov 2025 23:03:35 +0000 Subject: [PATCH 3/4] =?UTF-8?q?=F0=9F=A4=96=20fix:=20sanitize=20branch=20n?= =?UTF-8?q?ames=20in=20all=20getWorkspacePath=20calls?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Codex P0/P1 issues: - IPC WORKSPACE_EXECUTE_BASH: sanitize before computing bash cwd - AI session setup: sanitize before resolving workspace path - Agent session registration: sanitize before path assertion - Local fork: sanitize source workspace name - Config addWorkspace: sanitize when computing workspace path Without these fixes, any operation involving workspaces with slashes in their names would use the wrong paths (e.g., 'feature/foo' instead of 'feature-foo') causing bash execution, AI tool initialization, session registration, and forking to fail. _Generated with `cmux`_ --- src/config.ts | 5 ++++- src/runtime/LocalRuntime.ts | 5 +++-- src/services/agentSession.ts | 5 ++++- src/services/aiService.ts | 7 ++++++- src/services/ipcMain.ts | 4 +++- 5 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/config.ts b/src/config.ts index 3e572f32f..21f123923 100644 --- a/src/config.ts +++ b/src/config.ts @@ -369,8 +369,11 @@ export class Config { // Compute workspace path - this is only for legacy config migration // New code should use Runtime.getWorkspacePath() directly + // Sanitize branch name for directory path + const { sanitizeBranchNameForDirectory } = require("./utils/workspace/directoryName"); + const directoryName = sanitizeBranchNameForDirectory(metadata.name); const projectName = this.getProjectName(projectPath); - const workspacePath = path.join(this.srcDir, projectName, metadata.name); + const workspacePath = path.join(this.srcDir, projectName, directoryName); const workspaceEntry: Workspace = { path: workspacePath, id: metadata.id, diff --git a/src/runtime/LocalRuntime.ts b/src/runtime/LocalRuntime.ts index 21b9b742c..bbf8342a8 100644 --- a/src/runtime/LocalRuntime.ts +++ b/src/runtime/LocalRuntime.ts @@ -584,8 +584,9 @@ export class LocalRuntime implements Runtime { async forkWorkspace(params: WorkspaceForkParams): Promise { const { projectPath, sourceWorkspaceName, newWorkspaceName, initLogger } = params; - // Get source workspace path - const sourceWorkspacePath = this.getWorkspacePath(projectPath, sourceWorkspaceName); + // Get source workspace path (sanitize source name for directory) + const sourceDirName = sanitizeBranchNameForDirectory(sourceWorkspaceName); + const sourceWorkspacePath = this.getWorkspacePath(projectPath, sourceDirName); // Get current branch from source workspace try { diff --git a/src/services/agentSession.ts b/src/services/agentSession.ts index f4c0feb95..c2df41d1b 100644 --- a/src/services/agentSession.ts +++ b/src/services/agentSession.ts @@ -188,7 +188,10 @@ export class AgentSession { const runtime = createRuntime( metadata.runtimeConfig ?? { type: "local", srcBaseDir: this.config.srcDir } ); - return runtime.getWorkspacePath(metadata.projectPath, metadata.name); + // Sanitize branch name for directory path + const { sanitizeBranchNameForDirectory } = require("../utils/workspace/directoryName"); + const directoryName = sanitizeBranchNameForDirectory(metadata.name); + return runtime.getWorkspacePath(metadata.projectPath, directoryName); })(); assert( expectedPath === normalizedWorkspacePath, diff --git a/src/services/aiService.ts b/src/services/aiService.ts index 53102fbba..cef7f64c9 100644 --- a/src/services/aiService.ts +++ b/src/services/aiService.ts @@ -643,7 +643,12 @@ export class AIService extends EventEmitter { const isInPlace = metadata.projectPath === metadata.name; const workspacePath = isInPlace ? metadata.projectPath - : runtime.getWorkspacePath(metadata.projectPath, metadata.name); + : (() => { + // Sanitize branch name for directory path + const { sanitizeBranchNameForDirectory } = require("@/utils/workspace/directoryName"); + const directoryName = sanitizeBranchNameForDirectory(metadata.name); + return runtime.getWorkspacePath(metadata.projectPath, directoryName); + })(); // Build system message from workspace metadata const systemMessage = await buildSystemMessage( diff --git a/src/services/ipcMain.ts b/src/services/ipcMain.ts index 32d6c71db..62b439079 100644 --- a/src/services/ipcMain.ts +++ b/src/services/ipcMain.ts @@ -946,7 +946,9 @@ export class IpcMain { srcBaseDir: this.config.srcDir, }; const runtime = createRuntime(runtimeConfig); - const workspacePath = runtime.getWorkspacePath(metadata.projectPath, metadata.name); + // Sanitize branch name for directory path + const directoryName = sanitizeBranchNameForDirectory(metadata.name); + const workspacePath = runtime.getWorkspacePath(metadata.projectPath, directoryName); // Create bash tool with workspace's cwd and secrets // All IPC bash calls are from UI (background operations) - use truncate to avoid temp file spam From 1f5b43d0120eaf2c579358402b386e04f9d9f72f Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 12 Nov 2025 23:10:50 +0000 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=A4=96=20refactor:=20move=20sanitizat?= =?UTF-8?q?ion=20into=20getWorkspacePath?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address recurring bug pattern where callers must remember to sanitize branch names before calling getWorkspacePath. This was error-prone and already missed in ~8 places. Solution: Make getWorkspacePath() always sanitize its input. This is: - Idempotent: sanitizing an already-sanitized name returns the same value - Safe: ensures all 23 call sites automatically get correct paths - Future-proof: single point of truth for sanitization logic Now developers can call runtime.getWorkspacePath(projectPath, branchName) without worrying about sanitization - it happens automatically. _Generated with `cmux`_ --- src/config.ts | 2 +- src/runtime/LocalRuntime.ts | 25 ++++++++++++------------- src/runtime/SSHRuntime.ts | 25 +++++++++++-------------- src/services/agentSession.ts | 6 ++---- src/services/aiService.ts | 8 ++------ src/services/ipcMain.ts | 5 ++--- 6 files changed, 30 insertions(+), 41 deletions(-) diff --git a/src/config.ts b/src/config.ts index 21f123923..cb1550939 100644 --- a/src/config.ts +++ b/src/config.ts @@ -369,7 +369,7 @@ export class Config { // Compute workspace path - this is only for legacy config migration // New code should use Runtime.getWorkspacePath() directly - // Sanitize branch name for directory path + // Note: We manually sanitize here since this is config-level code that doesn't use Runtime const { sanitizeBranchNameForDirectory } = require("./utils/workspace/directoryName"); const directoryName = sanitizeBranchNameForDirectory(metadata.name); const projectName = this.getProjectName(projectPath); diff --git a/src/runtime/LocalRuntime.ts b/src/runtime/LocalRuntime.ts index bbf8342a8..bd48ec376 100644 --- a/src/runtime/LocalRuntime.ts +++ b/src/runtime/LocalRuntime.ts @@ -307,14 +307,17 @@ export class LocalRuntime implements Runtime { getWorkspacePath(projectPath: string, workspaceName: string): string { const projectName = getProjectName(projectPath); - return path.join(this.srcBaseDir, projectName, workspaceName); + // Always sanitize workspace name to ensure filesystem-safe directory names + // This is idempotent - sanitizing an already-sanitized name returns the same value + const directoryName = sanitizeBranchNameForDirectory(workspaceName); + return path.join(this.srcBaseDir, projectName, directoryName); } async createWorkspace(params: WorkspaceCreationParams): Promise { const { projectPath, branchName, trunkBranch, directoryName, initLogger } = params; try { - // Compute workspace path using the sanitized directory name + // Compute workspace path (getWorkspacePath sanitizes the name automatically) const workspacePath = this.getWorkspacePath(projectPath, directoryName); initLogger.logStep("Creating git worktree..."); @@ -452,11 +455,9 @@ export class LocalRuntime implements Runtime { { success: true; oldPath: string; newPath: string } | { success: false; error: string } > { // Note: _abortSignal ignored for local operations (fast, no need for cancellation) - // Compute workspace paths using sanitized directory names - const oldDirName = sanitizeBranchNameForDirectory(oldName); - const newDirName = sanitizeBranchNameForDirectory(newName); - const oldPath = this.getWorkspacePath(projectPath, oldDirName); - const newPath = this.getWorkspacePath(projectPath, newDirName); + // Compute workspace paths (getWorkspacePath sanitizes the names automatically) + const oldPath = this.getWorkspacePath(projectPath, oldName); + const newPath = this.getWorkspacePath(projectPath, newName); try { // Create parent directory for new path if needed (for nested directory names) @@ -490,9 +491,8 @@ export class LocalRuntime implements Runtime { // These are direct workspace directories (e.g., CLI/benchmark sessions), not git worktrees const isInPlace = projectPath === workspaceName; - // Compute workspace path using sanitized directory name - const directoryName = sanitizeBranchNameForDirectory(workspaceName); - const deletedPath = this.getWorkspacePath(projectPath, directoryName); + // Compute workspace path (getWorkspacePath sanitizes the name automatically) + const deletedPath = this.getWorkspacePath(projectPath, workspaceName); // Check if directory exists - if not, operation is idempotent try { @@ -584,9 +584,8 @@ export class LocalRuntime implements Runtime { async forkWorkspace(params: WorkspaceForkParams): Promise { const { projectPath, sourceWorkspaceName, newWorkspaceName, initLogger } = params; - // Get source workspace path (sanitize source name for directory) - const sourceDirName = sanitizeBranchNameForDirectory(sourceWorkspaceName); - const sourceWorkspacePath = this.getWorkspacePath(projectPath, sourceDirName); + // Get source workspace path (getWorkspacePath sanitizes the name automatically) + const sourceWorkspacePath = this.getWorkspacePath(projectPath, sourceWorkspaceName); // Get current branch from source workspace try { diff --git a/src/runtime/SSHRuntime.ts b/src/runtime/SSHRuntime.ts index 5e8af721c..4c50d78c8 100644 --- a/src/runtime/SSHRuntime.ts +++ b/src/runtime/SSHRuntime.ts @@ -780,13 +780,17 @@ export class SSHRuntime implements Runtime { getWorkspacePath(projectPath: string, workspaceName: string): string { const projectName = getProjectName(projectPath); - return path.posix.join(this.config.srcBaseDir, projectName, workspaceName); + // Always sanitize workspace name to ensure filesystem-safe directory names + // This is idempotent - sanitizing an already-sanitized name returns the same value + const { sanitizeBranchNameForDirectory } = require("../utils/workspace/directoryName"); + const directoryName = sanitizeBranchNameForDirectory(workspaceName); + return path.posix.join(this.config.srcBaseDir, projectName, directoryName); } async createWorkspace(params: WorkspaceCreationParams): Promise { try { const { projectPath, branchName, directoryName, initLogger, abortSignal } = params; - // Compute workspace path using sanitized directory name + // Compute workspace path (getWorkspacePath sanitizes the name automatically) const workspacePath = this.getWorkspacePath(projectPath, directoryName); // Prepare parent directory for git clone (fast - returns immediately) @@ -922,13 +926,9 @@ export class SSHRuntime implements Runtime { if (abortSignal?.aborted) { return { success: false, error: "Rename operation aborted" }; } - // Import sanitization utility for directory name conversion - const { sanitizeBranchNameForDirectory } = await import("../utils/workspace/directoryName"); - const oldDirName = sanitizeBranchNameForDirectory(oldName); - const newDirName = sanitizeBranchNameForDirectory(newName); - // Compute workspace paths using sanitized directory names - const oldPath = this.getWorkspacePath(projectPath, oldDirName); - const newPath = this.getWorkspacePath(projectPath, newDirName); + // Compute workspace paths (getWorkspacePath sanitizes the names automatically) + const oldPath = this.getWorkspacePath(projectPath, oldName); + const newPath = this.getWorkspacePath(projectPath, newName); try { // SSH runtimes use plain directories, not git worktrees @@ -987,11 +987,8 @@ export class SSHRuntime implements Runtime { return { success: false, error: "Delete operation aborted" }; } - // Import sanitization utility for directory name conversion - const { sanitizeBranchNameForDirectory } = await import("../utils/workspace/directoryName"); - const directoryName = sanitizeBranchNameForDirectory(workspaceName); - // Compute workspace path using sanitized directory name - const deletedPath = this.getWorkspacePath(projectPath, directoryName); + // Compute workspace path (getWorkspacePath sanitizes the name automatically) + const deletedPath = this.getWorkspacePath(projectPath, workspaceName); try { // Combine all pre-deletion checks into a single bash script to minimize round trips diff --git a/src/services/agentSession.ts b/src/services/agentSession.ts index c2df41d1b..3fbb7d2bf 100644 --- a/src/services/agentSession.ts +++ b/src/services/agentSession.ts @@ -188,10 +188,8 @@ export class AgentSession { const runtime = createRuntime( metadata.runtimeConfig ?? { type: "local", srcBaseDir: this.config.srcDir } ); - // Sanitize branch name for directory path - const { sanitizeBranchNameForDirectory } = require("../utils/workspace/directoryName"); - const directoryName = sanitizeBranchNameForDirectory(metadata.name); - return runtime.getWorkspacePath(metadata.projectPath, directoryName); + // getWorkspacePath sanitizes the branch name automatically + return runtime.getWorkspacePath(metadata.projectPath, metadata.name); })(); assert( expectedPath === normalizedWorkspacePath, diff --git a/src/services/aiService.ts b/src/services/aiService.ts index cef7f64c9..534a5bc05 100644 --- a/src/services/aiService.ts +++ b/src/services/aiService.ts @@ -643,12 +643,8 @@ export class AIService extends EventEmitter { const isInPlace = metadata.projectPath === metadata.name; const workspacePath = isInPlace ? metadata.projectPath - : (() => { - // Sanitize branch name for directory path - const { sanitizeBranchNameForDirectory } = require("@/utils/workspace/directoryName"); - const directoryName = sanitizeBranchNameForDirectory(metadata.name); - return runtime.getWorkspacePath(metadata.projectPath, directoryName); - })(); + : // getWorkspacePath sanitizes the branch name automatically + runtime.getWorkspacePath(metadata.projectPath, metadata.name); // Build system message from workspace metadata const systemMessage = await buildSystemMessage( diff --git a/src/services/ipcMain.ts b/src/services/ipcMain.ts index 62b439079..f84f0b907 100644 --- a/src/services/ipcMain.ts +++ b/src/services/ipcMain.ts @@ -946,9 +946,8 @@ export class IpcMain { srcBaseDir: this.config.srcDir, }; const runtime = createRuntime(runtimeConfig); - // Sanitize branch name for directory path - const directoryName = sanitizeBranchNameForDirectory(metadata.name); - const workspacePath = runtime.getWorkspacePath(metadata.projectPath, directoryName); + // getWorkspacePath sanitizes the branch name automatically + const workspacePath = runtime.getWorkspacePath(metadata.projectPath, metadata.name); // Create bash tool with workspace's cwd and secrets // All IPC bash calls are from UI (background operations) - use truncate to avoid temp file spam