feat: support "/" character in workspace/branch names#2795
feat: support "/" character in workspace/branch names#2795ibetitsmike wants to merge 1 commit intomainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb7caba44b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| branchName: finalBranchName, | ||
| trunkBranch: normalizedTrunkBranch, | ||
| directoryName: finalBranchName, | ||
| directoryName: sanitizeWorkspaceNameForPath(finalBranchName), |
There was a problem hiding this comment.
Passing sanitizeWorkspaceNameForPath(finalBranchName) here makes the created workspace path diverge from the persisted workspace name for some valid inputs (e.g. feature/my-branch and even foo--bar, because sanitization also collapses hyphens). CoderSSHRuntime.createWorkspace() uses directoryName to compute the initial path, but later runtime calls resolve paths from metadata.name via getWorkspacePath(...), so follow-up operations can target a different directory than the one created and fail to find the repo.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea797fb02e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * workspace names while remaining safe for directory names and session paths. | ||
| */ | ||
| export function sanitizeWorkspaceNameForPath(name: string): string { | ||
| return name.replace(/\/+/g, "-").replace(/-+/g, "-"); |
There was a problem hiding this comment.
Avoid collapsing hyphen runs here: sanitizeWorkspaceNameForPath currently rewrites any repeated -, so a previously valid name like feature--abc now maps to feature-abc. Since WorktreeManager.getWorkspacePath and SSHRuntime.getWorkspacePath both switched to this sanitizer in this commit, existing workspaces with consecutive hyphens will be looked up at a different directory after upgrade, which breaks operations like readiness checks/rename/delete and can also create name collisions between distinct workspaces.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c87b97bd95
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * workspace names while remaining safe for directory names and session paths. | ||
| */ | ||
| export function sanitizeWorkspaceNameForPath(name: string): string { | ||
| return name.replace(/\//g, "-"); |
There was a problem hiding this comment.
sanitizeWorkspaceNameForPath() currently rewrites every / to -, so distinct workspace names like feature/a and feature-a map to the same filesystem path. Because WorktreeManager.getWorkspacePath() and SSHRuntime.getWorkspacePath() now depend on this mapping, creating/renaming/deleting one of these workspaces can target the other’s directory, and create retries may fall back to a suffixed branch instead of the requested branch. Please use a reversible encoding (or add a deterministic disambiguator) so each valid workspace name has a unique path.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 670c7a660a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const validPattern = /^[a-z0-9_-]+$/; | ||
| const validPattern = /^[a-z0-9_-]+(?:\/[a-z0-9_-]+)*$/; |
There was a problem hiding this comment.
Allowing / in workspace names here makes plan paths become nested (getPlanFilePath builds .../{workspaceName}.md in src/common/utils/planStorage.ts:25-31), but movePlanFile still runs a bare mv and neither creates the destination parent directory nor checks execBuffered exit status (src/node/utils/runtime/helpers.ts:246-253). Renaming a workspace with an existing plan from something like foo to feature/foo will therefore silently fail to move the file, and later reads target only the new path, so the plan appears to disappear after rename.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b1add58c4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Reject names that would collide on disk after slash sanitization. | ||
| // e.g. "feature/a" and "feature-a" both map to directory "feature-a". | ||
| if (branchName.includes("/")) { |
There was a problem hiding this comment.
This collision guard only runs when the requested name contains /, so creating feature-foo is allowed even if feature/foo already exists. After this commit both names normalize through sanitizeWorkspaceNameForPath, which means they can alias the same normalized storage key (including plan paths from getPlanFilePath), letting one workspace overwrite another workspace’s persisted data instead of returning a conflict.
Useful? React with 👍 / 👎.
src/node/worktree/WorktreeManager.ts
Outdated
| getWorkspacePath(projectPath: string, workspaceName: string): string { | ||
| const projectName = getProjectName(projectPath); | ||
| return path.join(this.srcBaseDir, projectName, workspaceName); | ||
| return path.join(this.srcBaseDir, projectName, sanitizeWorkspaceNameForPath(workspaceName)); |
There was a problem hiding this comment.
Sanitizing workspace names in getWorkspacePath means names like feature-a and feature/a now resolve to the same directory path. Rename flows still attempt a directory move and treat failures as fatal, so renaming between slash and hyphen variants can fail even though the physical path should be unchanged. Add a same-path short-circuit (branch/name-only rename) when normalized old/new paths match.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00a5e78960
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (oldPath === newPath) { | ||
| return { success: true, oldPath, newPath }; |
There was a problem hiding this comment.
When a workspace is renamed between names that sanitize to the same directory (for example feature-a → feature/a), this early return skips the git branch -m step later in renameWorkspace, so the persisted workspace name changes but the checked-out branch name does not. That breaks the stated branch/workspace-name sync invariant and can lead to follow-up behavior drift (e.g., branch cleanup in deleteWorkspace using the new workspace name and leaving the old branch behind).
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a0ee1687b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const validPattern = /^[a-z0-9_-]+$/; | ||
| const validPattern = /^[a-z0-9_-]+(?:\/[a-z0-9_-]+)*$/; |
There was a problem hiding this comment.
By permitting / in validateWorkspaceName, this change introduces aliasing (feature/a and feature-a) but only create()/rename() received sanitized collision guards; fork() still only validates format (see workspaceService.fork around the validateWorkspaceName(resolvedName) check) and can persist a conflicting name in runtimes that don't enforce distinct directories (notably LocalRuntime.forkWorkspace). Since plan paths now sanitize / to - (getPlanFilePath), those two workspaces resolve to the same plan file and can overwrite each other’s plan data.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c80af8e0cb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const pathCollision = allWorkspaces.find( | ||
| (ws) => | ||
| ws.id !== workspaceId && | ||
| sanitizeWorkspaceNameForPath(ws.name) === sanitizedPath | ||
| ); |
There was a problem hiding this comment.
The new sanitized-name collision check in rename() scans allWorkspaces globally, so a rename in one project can be rejected just because another project has a workspace whose sanitized name matches (for example, renaming to feature/a fails if a different project has feature-a). Since workspace directories are namespaced by project, this is a false positive and blocks valid renames; the collision search should be restricted to workspaces from the current project before comparing sanitizeWorkspaceNameForPath(ws.name).
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e41118f2e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/common/utils/planStorage.ts
Outdated
| muxHome = DEFAULT_MUX_HOME | ||
| ): string { | ||
| return `${muxHome}/plans/${projectName}/${workspaceName}.md`; | ||
| return `${muxHome}/plans/${projectName}/${sanitizeWorkspaceNameForPath(workspaceName)}.md`; |
There was a problem hiding this comment.
Sanitizing workspaceName in getPlanFilePath() changes the canonical filename for any preexisting workspace names that contain / (notably in-place workspaces where AgentSession stores workspaceName as the full path). readPlanFile() only checks the new canonical path and the legacy ID path, so plans previously written under the old name-based path are no longer found or migrated after upgrade, which makes existing plan content appear missing for those workspaces.
Useful? React with 👍 / 👎.
| if (collision) { | ||
| return Err( | ||
| `Workspace name "${resolvedName}" conflicts with existing workspace "${collision.name}" (both resolve to the same directory)` | ||
| ); |
There was a problem hiding this comment.
In the auto-name fork path, we generate resolvedName using literal name matching, then immediately return an error when sanitized paths collide. This causes avoidable failures (for example, forking feature/a when feature-a-fork-1 already exists) even though a valid next suffix exists, because the code never retries with another generated name once the sanitized collision is detected.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3c289c8f4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const validPattern = /^[a-z0-9_-]+$/; | ||
| const validPattern = /^[a-z0-9_-]+(?:\/[a-z0-9_-]+)*$/; |
There was a problem hiding this comment.
Allowing / in workspace names here introduces a silent data-loss path for plan files: movePlanFile (in src/node/utils/runtime/helpers.ts) still does mv oldPath newPath without creating the new parent directory and swallows failures, so renaming a workspace like foo to feature/foo leaves the old plan at .../foo.md and the renamed workspace appears to have no plan. This did not occur before slash names were accepted because both old/new plan paths stayed in the same directory depth.
Useful? React with 👍 / 👎.
| if (!isAutoName) { | ||
| const sanitizedPath = sanitizeWorkspaceNameForPath(resolvedName); |
There was a problem hiding this comment.
This block skips path-collision checks for auto-generated fork names, but auto-generation can still collide after slash sanitization: with an existing workspace feature-a-fork-1, forking feature/a can generate feature/a-fork-1 (because generateForkBranchName matches unsanitized prefixes), and both map to the same directory, causing fork creation to fail at runtime. The post-generation collision guard needs to run for auto names too, or generation must use sanitized equivalence.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 648ced0709
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isAutoName) { | ||
| // Auto-named fork: add collision name's sanitized form and retry | ||
| const existingNamesWithCollision = [ | ||
| ...existingNamesWithSanitized, |
There was a problem hiding this comment.
This retry path references existingNamesWithSanitized, but that identifier is declared in a different block and is out of scope here. When an auto-named fork hits a sanitized-path collision (for example, forking feature/a when feature-a-fork-1 exists), this branch throws a ReferenceError instead of returning a normal collision error, aborting the fork flow.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9d78e06d9
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| // Best-effort: branch rename can fail if the branch doesn't exist | ||
| } | ||
| return { success: true, oldPath, newPath }; |
There was a problem hiding this comment.
When oldPath === newPath, this path returns success even if git branch -m fails. That means a rename like feature-a → feature/a can be reported as successful while the branch stays on the old name (for example if Git rejects the target ref due to existing refs), and WorkspaceService.rename() will still persist newName, leaving workspace metadata and the actual branch out of sync for later branch-based operations.
Useful? React with 👍 / 👎.
| resolvedName = generateForkBranchName(sourceMetadata.name, existingNamesWithCollision); | ||
| // Re-validate after retry | ||
| const retryValidation = validateWorkspaceName(resolvedName); | ||
| if (!retryValidation.valid) { | ||
| return Err(retryValidation.error ?? "Invalid workspace name"); |
There was a problem hiding this comment.
The auto-name collision path retries only once and then continues without checking for another sanitized-path collision. If multiple existing names already occupy the next sanitized suffixes (e.g. feature-a-fork-1, feature-a-fork-2 while forking feature/a), the second generated name can still map to an existing directory and fail later in runtime creation instead of selecting the next free suffix.
Useful? React with 👍 / 👎.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ed657737b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { cwd: this.config.srcBaseDir, timeout: 10, abortSignal } | ||
| ); | ||
| await branchStream.stdin.abort(); | ||
| await branchStream.exitCode; |
There was a problem hiding this comment.
In SSHRuntime.renameWorkspace, the short-circuit path for oldPath === newPath awaits branchStream.exitCode but never validates it, and RemoteRuntime.exec does not throw on non-zero exit codes. If git branch -m fails (for example because the destination branch already exists), this method still returns success, so callers can persist the new workspace name while the underlying git branch remains unchanged.
Useful? React with 👍 / 👎.
| const maxRetries = 20; // safety bound to prevent infinite loops | ||
| for (let attempt = 0; attempt < maxRetries; attempt++) { |
There was a problem hiding this comment.
The auto-fork collision loop caps retries at 20, but after the final iteration it exits without re-checking or erroring on the newly generated resolvedName. In projects with many sanitized collisions (e.g., many existing feature-a-fork-N names while forking feature/a), this can leave resolvedName still colliding and only fail later in runtime creation instead of resolving a unique name or returning a deterministic collision error.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9ce47f49f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const maxRetries = 20; // safety bound to prevent infinite loops | ||
| let lastCollision: typeof existingWorkspaces[number] | undefined; | ||
| for (let attempt = 0; attempt < maxRetries; attempt++) { |
There was a problem hiding this comment.
The collision loop stops at attempt < maxRetries but generates the next candidate inside the last iteration, so that final candidate is never checked before returning the "after 20 retries" error. With 20 occupied sanitized names (e.g. feature-a-fork-1..20) and auto-forking feature/a, the code generates feature/a-fork-21 on the last pass and then fails immediately even if -21 is free, causing a false-negative fork failure in real projects with many prior forks.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a421315d4e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/node/worktree/WorktreeManager.ts
Outdated
| getWorkspacePath(projectPath: string, workspaceName: string): string { | ||
| const projectName = getProjectName(projectPath); | ||
| return path.join(this.srcBaseDir, projectName, workspaceName); | ||
| return path.join(this.srcBaseDir, projectName, sanitizeWorkspaceNameForPath(workspaceName)); |
There was a problem hiding this comment.
Avoid sanitizing workspaceName unconditionally here, because in-place workspaces use workspaceName === projectPath (an absolute path) and rely on path.join(..., workspaceName) preserving that absolute target. Replacing / with - first turns an absolute path like /tmp/proj into -tmp-proj, so getWorkspacePath() now points under srcBaseDir instead of the real workspace directory, which can break in-place flows that call this helper (for example readiness/deletion paths in WorktreeRuntime/WorktreeManager).
Useful? React with 👍 / 👎.
ibetitsmike
force-pushed
the
fix/support-slash-in-branch-names
branch
from
5addf3b to
0dbf34f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dbf34fee2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/node/runtime/SSHRuntime.ts
Outdated
| try { | ||
| const expandedOldPath = expandTildeForSSH(oldPath); | ||
| const branchStream = await this.exec( | ||
| `git -C ${expandedOldPath} branch -m ${shescape.quote(oldName)} ${shescape.quote(newName)}`, |
There was a problem hiding this comment.
When renameWorkspace takes the new short-circuit path (oldPath === newPath), the branch-rename command interpolates expandedOldPath without shell quoting. If the project path contains spaces or shell metacharacters, git -C parses the path incorrectly, the rename fails, and the catch block still returns success, leaving config renamed while the actual git branch stays on the old name. This affects SSH renames where names sanitize to the same directory (for example feature-a ↔ feature/a).
Useful? React with 👍 / 👎.
ibetitsmike
force-pushed
the
fix/support-slash-in-branch-names
branch
from
0dbf34f to
acd8679
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acd86793a5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| w.name !== branchName && | ||
| w.name != null && | ||
| !path.isAbsolute(w.name) && | ||
| sharesPathNamespace(runtimeConfig, w.runtimeConfig, this.config.srcDir) && |
There was a problem hiding this comment.
The new collision guard in create() uses the raw runtimeConfig to call sharesPathNamespace, but for Coder flows this config can still carry the placeholder host (coder://) until finalizeConfig runs later in the same create path. Existing Coder workspaces are persisted with resolved hosts, so this comparison can return false and miss a real namespace collision; e.g. creating feature/a next to feature-a for the same existing Coder workspace can still map both to the same sanitized directory. This can silently alias two workspaces onto one filesystem path.
Useful? React with 👍 / 👎.
ibetitsmike
force-pushed
the
fix/support-slash-in-branch-names
branch
from
acd8679 to
0769b5e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0769b5ef9d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| aHost !== bHost && | ||
| aHost !== CODER_RUNTIME_PLACEHOLDER && | ||
| bHost !== CODER_RUNTIME_PLACEHOLDER | ||
| ) { |
There was a problem hiding this comment.
The host check currently treats CODER_RUNTIME_PLACEHOLDER as matching any SSH host, so during create() (before finalizeConfig resolves the Coder host) a Coder runtime can be considered to share a filesystem namespace with unrelated non-Coder SSH runtimes that happen to use the same srcBaseDir. That makes slash-sanitization collisions (feature/a vs feature-a) fail with a false "same directory" error even when the workspaces are on different hosts and cannot collide on disk.
Useful? React with 👍 / 👎.
ibetitsmike
force-pushed
the
fix/support-slash-in-branch-names
branch
from
0769b5e to
14d2c1f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14d2c1f80c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const aSrc = getSrcBaseDir(a) ?? defaultSrcDir; | ||
| const bSrc = getSrcBaseDir(b) ?? defaultSrcDir; | ||
| if (aSrc !== bSrc) return false; |
There was a problem hiding this comment.
sharesPathNamespace uses raw string equality for srcBaseDir, but create/rename/fork call this before runtime path resolution, so equivalent paths like ~/mux/src and /home/user/mux/src are treated as different namespaces. In that case the new slash-sanitization collision guard is bypassed, allowing names such as feature/a and feature-a to both be accepted even though they resolve to the same on-disk workspace directory after runtime normalization.
Useful? React with 👍 / 👎.
ibetitsmike
force-pushed
the
fix/support-slash-in-branch-names
branch
from
14d2c1f to
6d31729
Compare
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d31729bfc
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // e.g. "~/mux/src" and "/home/user/mux/src" compare equal for local runtimes. | ||
| const normalize = (p: string | undefined): string | undefined => { | ||
| if (p == null) return p; | ||
| return stripTrailingSlashes(path.normalize(expandTilde(p))); |
There was a problem hiding this comment.
sharesPathNamespace normalizes srcBaseDir with expandTilde, which uses the local machine’s home directory, not the SSH host’s. In the create flow, existing SSH workspaces are typically persisted with resolved remote paths (for example /home/coder/mux), while a newly selected runtime often starts as ~/mux; this line turns that into a local path (for example /Users/alice/mux), so the function incorrectly treats them as different namespaces and skips the slash→hyphen collision guard. That allows names like feature/a and feature-a to pass preflight and resolve to the same remote workspace directory.
Useful? React with 👍 / 👎.
ibetitsmike
force-pushed
the
fix/support-slash-in-branch-names
branch
from
6d31729 to
1711d30
Compare
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1711d30b61
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| w.name !== branchName && | ||
| w.name != null && | ||
| !path.isAbsolute(w.name) && | ||
| sharesPathNamespace(runtimeConfig, w.runtimeConfig, this.config.srcDir) && |
There was a problem hiding this comment.
The new path-collision guard in create() calls sharesPathNamespace(runtimeConfig, ...) with the raw incoming runtime config, but this method only resolves/normalizes srcBaseDir later in the same function. For SSH, that means a new workspace using ~/... can fail to match existing workspaces already persisted with an absolute /home/... path, so slash aliases like feature/a can slip past this check even when feature-a already owns the same directory. Run namespace comparison against the resolved runtime config (or normalize SSH ~/absolute equivalents) before applying this collision check.
Useful? React with 👍 / 👎.
Allow forward slashes in workspace names (e.g. 'feature/my-branch') to match common Git branch naming conventions. Changes: - Update validation regex to accept '/' (reject leading, trailing, and consecutive slashes) - Add sanitizeWorkspaceNameForPath() that replaces '/' with '-' for filesystem paths - Apply sanitization in getWorkspacePath() for WorktreeManager and SSHRuntime, with an absolute-path guard for in-place workspaces - Add path collision detection in create(), rename(), and fork() scoped to path-based runtimes (worktree, SSH, devcontainer) using usesPathBasedDirs() helper - Auto-fork collision retry loop scales with existing workspace count and uses normalized parent for legacy name compatibility - Handle slashes in CoderSSHRuntime.toCoderCompatibleName() - Ensure movePlanFile() creates destination directories via mkdir -p - Same-path rename short-circuit runs best-effort git branch -m - Update mobile validation, UI hints, and add comprehensive tests Closes #407
ibetitsmike
force-pushed
the
fix/support-slash-in-branch-names
branch
from
1711d30 to
6cabe08
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
1 participant
Resolves #407.
Problem
Git branch names commonly contain
/(e.g.feature/my-branch), but workspace name validation rejected them with:This is because workspace names are also used as filesystem directory names, where
/would be interpreted as a path separator.Solution
Per the discussion in #407, this PR:
Accepts
/in workspace names — Updated the validation regex from[a-z0-9_-]+to[a-z0-9_-]+(?:\/[a-z0-9_-]+)*, which allows forward slashes in git branch-style positions while rejecting leading, trailing, and consecutive slashes.Sanitizes for filesystem use — Added
sanitizeWorkspaceNameForPath()that replaces/with-and collapses consecutive hyphens. Used inworkspaceService.create()for thedirectoryNameparameter, sofeature/my-branchbecomes the directoryfeature-my-branchwhile the git branch staysfeature/my-branch.Updated mobile — Same validation changes in mobile, plus updated the rename modal hint text.
Files changed
src/common/utils/validation/workspaceValidation.tssanitizeWorkspaceNameForPathsrc/common/utils/validation/workspaceValidation.test.tsmobile/src/utils/workspaceValidation.tsmobile/src/utils/workspaceValidation.test.tsmobile/src/components/RenameWorkspaceModal.tsxsrc/node/services/workspaceService.tssanitizeWorkspaceNameForPathfordirectoryNameTesting
All unit tests pass (21 common + 24 mobile).