-
Notifications
You must be signed in to change notification settings - Fork 14
🤖 feat: allow slashes in branch names with directory sanitization #564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- 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`_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
https://github.com/coder/cmux/blob/907e291120db7ba1e8708a567e12eebb04e42929/src/runtime/LocalRuntime.ts#L481-L496
Sanitize branch names when deleting local workspaces
Workspaces created through IPC_CHANNELS.WORKSPACE_CREATE now store directories under sanitizeBranchNameForDirectory(branchName), but LocalRuntime.deleteWorkspace still passes the raw workspaceName directly into getWorkspacePath. When the branch name contains /, the computed path points to a non‑existent nested directory (e.g. feature/foo instead of feature-foo) and the deletion command fails, leaving the worktree on disk. Any attempt to delete or clean up a workspace whose name includes slashes will error out or leak the directory.
https://github.com/coder/cmux/blob/907e291120db7ba1e8708a567e12eebb04e42929/src/runtime/SSHRuntime.ts#L925-L928
SSH workspace rename uses unsanitized paths
The SSH runtime now creates workspaces under sanitized directory names, but SSHRuntime.renameWorkspace still derives oldPath and newPath from the raw branch names. For names containing /, the computed paths will not match the actual directories (which replaced slashes with dashes), so the mv command fails and the workspace cannot be renamed. Remote runtimes therefore cannot rename slash‑named workspaces even though creation is allowed.
https://github.com/coder/cmux/blob/907e291120db7ba1e8708a567e12eebb04e42929/src/runtime/SSHRuntime.ts#L986-L988
SSH workspace deletion ignores directory sanitization
Similarly, SSHRuntime.deleteWorkspace still calls getWorkspacePath(projectPath, workspaceName) with the raw branch name. After permitting / in names, the runtime created directories with slashes converted to dashes, so deleting a slash‑named workspace now targets a different path and the pre‑deletion checks and removal commands fail. Remote workspace deletion for these names will consistently error or leave directories undeleted.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
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`_
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
https://github.com/coder/cmux/blob/7f5eb7eda2a4b6f45922c933de121cb8b16bf4c2/src/services/ipcMain.ts#L942-L950
Sanitize workspace name before computing bash cwd
The new create path uses sanitizeBranchNameForDirectory, but WORKSPACE_EXECUTE_BASH still calls runtime.getWorkspacePath(metadata.projectPath, metadata.name). When metadata.name contains a slash (now allowed), this reconstructs a path with embedded directories (project/feature/foo) while the worktree was created at the sanitized path (project/feature-foo). Every bash execution for such workspaces will run in a non‑existent directory and fail. Use the sanitized directory name (or sanitize inside getWorkspacePath) before building workspacePath.
https://github.com/coder/cmux/blob/7f5eb7eda2a4b6f45922c933de121cb8b16bf4c2/src/services/aiService.ts#L637-L646
AI session resolves incorrect path for slash branch names
The system message setup still derives the workspace directory with runtime.getWorkspacePath(metadata.projectPath, metadata.name). After this commit worktrees are created using the sanitized directory name (slashes → dashes). For branch names like feature/foo, the reconstructed path will be …/feature/foo, which does not exist, causing tool initialization and system message generation to fail. The branch name should be sanitized before calling getWorkspacePath or the runtime helper should handle sanitization internally.
https://github.com/coder/cmux/blob/7f5eb7eda2a4b6f45922c933de121cb8b16bf4c2/src/services/agentSession.ts#L184-L194
Agent session path assertion fails for slash names
During workspace registration the code recomputes the expected path via runtime.getWorkspacePath(metadata.projectPath, metadata.name) and asserts it matches the supplied path. With the new sanitization, workspaces created for feature/foo live at …/feature-foo; the recomputed path still uses the unsanitized branch, so the assertion will fire on subsequent registrations (e.g., app restart) and the session will not initialize. Sanitize metadata.name before calling getWorkspacePath or make getWorkspacePath perform the conversion.
https://github.com/coder/cmux/blob/7f5eb7eda2a4b6f45922c933de121cb8b16bf4c2/src/runtime/LocalRuntime.ts#L584-L588
Forking uses unsanitized source path
Local workspace forking still calls this.getWorkspacePath(projectPath, sourceWorkspaceName) where sourceWorkspaceName is the branch name from config. When that name contains slashes, this constructs project/feature/foo even though the workspace lives at project/feature-foo; the subsequent git branch --show-current executes in a non‑existent directory and the fork fails. Sanitize sourceWorkspaceName (and probably make getWorkspacePath handle sanitization) before resolving the path.
https://github.com/coder/cmux/blob/7f5eb7eda2a4b6f45922c933de121cb8b16bf4c2/src/config.ts#L370-L375
Forked workspace paths stored unsanitized in config
addWorkspace still derives workspacePath as path.join(this.srcDir, projectName, metadata.name). When IPC_WORKSPACE_FORK passes a branch like docs/api, the saved path becomes …/docs/api while the runtime created …/docs-api. The config entry will not match the actual directory, breaking later lookups that rely on the stored path. Compute the path with the sanitized directory name or use the path returned by the runtime instead of reconstructing it here.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
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`_
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/services/aiService.ts
Outdated
| 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use relative path for runtime sanitization require
The new workspace-path logic calls require("@/utils/workspace/directoryName") inside AIService. Unlike static import statements, this dynamic require executes at runtime and is not processed by the TypeScript path alias resolver, so Node will look for a package literally named @ and throw MODULE_NOT_FOUND. That means every call to build the system message for a workspace (almost all non in-place workspaces) will crash before it can sanitize the branch name. Use a relative path (e.g. require("../utils/workspace/directoryName")) or convert the call to a top-level import to ensure it resolves correctly.
Useful? React with 👍 / 👎.
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`_
💡 Better Solution: Sanitization in
|
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Users want to create workspaces with branch names like
feature/foo,bugfix/bar,docs/api/endpoints- common in Git workflows. Previously blocked because slashes aren't filesystem-safe.Solution
Allow slashes in branch names but sanitize them for directory names (
/→-). Git commands use the actual branch name; filesystem operations use the sanitized version.Key insight: Config stores only the branch name. Directory names are ALWAYS computed on-the-fly via
sanitizeBranchNameForDirectory(). Never stored separately.Changes (~120 LoC)
src/utils/workspace/directoryName.ts): Sanitization + conflict detectiondirectoryNameparam (sanitized) for paths,branchNamefor gitdocs-foo→docs/foo)Conflict Detection
Prevents collisions like:
feature/foo+ existingfeature-foo→ ERROR (both use directoryfeature-foo)docs/api/guide+ existingdocs-api-guide→ ERRORTests
Generated with
cmux