🤖 Expand tilde paths at workspace creation #451
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![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.
ℹ️ 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".
ammario
approved these changes
Oct 27, 2025
Previously, LocalRuntime would store srcBaseDir paths with tildes unexpanded, leading to incorrect path construction when using path.join(). This simplifies logic by ensuring all local paths are fully expanded at the IPC boundary. Changes: - Add expandTilde() utility for local file system path expansion - Update LocalRuntime constructor to expand tilde in srcBaseDir - Add comprehensive tests for both utilities For SSH runtime, tilde paths are preserved and expanded using expandTildeForSSH() when constructing remote commands, which is the correct behavior for remote paths.
SSH runtime now requires absolute paths for srcBaseDir to simplify logic and avoid ambiguity about which user's home directory is being referenced. Changes: - SSHRuntime constructor validates srcBaseDir doesn't start with tilde - Updated default SSH srcBaseDir from ~/cmux to /home/cmux - Updated all tests to use absolute paths or sshConfig.workdir - Replaced tilde-handling tests with tilde-rejection tests - Updated Runtime.ts documentation to clarify path requirements LocalRuntime continues to expand tilde paths at construction (previous commit), ensuring all runtime paths are fully resolved at the IPC boundary.
Updated tests to use absolute paths instead of tilde paths for SSH runtime, consistent with the requirement that SSH srcBaseDir must be absolute.
Reverted from educational note back to rejection behavior. When a command tries to cd into the current working directory (e.g., 'cd /workspace && command'), the bash tool now rejects it with a clear error message. Changes: - Replaced educational note with redundant cd detection and rejection - Uses runtime.normalizePath() for cross-runtime path comparison - Works correctly for both local and SSH runtimes - Updated tests to verify rejection behavior
When parsing SSH runtime strings, extract the username from user@host format and use it to construct the srcBaseDir path. This ensures workspace creation targets the correct user's home directory. - 'ssh user@host' -> srcBaseDir: /home/user/cmux - 'ssh hostname' -> srcBaseDir: /home/$USER/cmux (current user) Resolves Codex review comment about hard-coded /home/cmux.
When parsing SSH runtime strings with user 'root', use /root as the home directory instead of /home/root.
Add validation to reject absolute paths that contain the workspace directory prefix, encouraging agents to use relative paths instead. This helps save tokens by avoiding redundant path prefixes in tool calls. Changes: - Add validateNoRedundantPrefix() to fileCommon.ts to detect absolute paths containing cwd - Integrate validation into all file_* tools (file_read, file_edit_insert, file_edit_operation) - Update all tool tests to use relative paths instead of absolute paths - Add comprehensive test coverage for redundant prefix detection The validation provides helpful error messages suggesting the relative path equivalent: "Redundant path prefix detected. The path '/workspace/project/src/file.ts' contains the workspace directory. Please use relative paths to save tokens: 'src/file.ts'" _Generated with `cmux`_
The WORKSPACE_CREATE IPC handler wasn't catching errors thrown during
runtime creation (e.g., validation errors for tilde paths in SSH runtime).
This caused tests to fail with unhandled exceptions instead of returning
{ success: false, error: ... } as expected.
Wrap createRuntime() call in try-catch to properly handle validation errors
and return them through the IPC boundary.
Fixes integration test: 'rejects tilde paths in srcBaseDir (SSH only)'
_Generated with `cmux`_
Previously, validateNoRedundantPrefix() skipped validation for SSH runtimes under the assumption that remote paths needed different handling. However, the validation is equally valuable for SSH - absolute paths like '/home/user/cmux/project/branch/src/file.ts' are just as redundant as '/workspace/project/src/file.ts' on local runtime. Changes: - Remove SSH runtime bypass in validateNoRedundantPrefix() - Use simple string matching instead of Node's path module (works for both) - Handle trailing slashes and partial directory name matches correctly - Add comprehensive tests for SSH runtime validation - Add edge case tests (path equals cwd, partial name matches) The validation now saves tokens on both local and SSH runtimes by encouraging agents to use relative paths consistently. _Generated with `cmux`_
ESLint requires unused parameters to be prefixed with underscore. The runtime parameter is kept for API consistency even though it's not used after removing the SSH bypass. _Generated with `cmux`_
Instead of manually normalizing paths with string operations, leverage the
runtime's existing normalizePath() method for consistent path handling.
This reduces duplication and ensures we handle paths the same way the
runtime does.
Changes:
- Use runtime.normalizePath(".", cwd) to normalize the working directory
- Keeps manual trailing slash cleanup for absolute paths (can't use
normalizePath directly since it's designed for relative path resolution)
- Update documentation to reflect use of runtime method
- Remove underscore prefix from runtime param (now used)
_Generated with `cmux`_
- Add resolvePath() method to Runtime interface - LocalRuntime: Expands tildes using expandTilde() and verifies path exists - SSHRuntime: Uses remote 'realpath' command to expand and verify paths - WORKSPACE_CREATE: Resolves srcBaseDir before creating runtime and storing in config - Remove tilde validation from SSHRuntime constructor (now handled by resolvePath) - Both tilde (~) and non-tilde paths now work for workspace creation - Add comprehensive unit tests for both runtimes This enables storing resolved paths in config, ensuring consistency across local and SSH runtimes regardless of how the path was originally specified. Generated with `cmux`
- Remove existence checking from LocalRuntime.resolvePath() - Now just expands tildes and resolves to absolute path - No longer uses fsPromises.access() to verify path exists - Update SSHRuntime.resolvePath() to use readlink -m instead of realpath - readlink -m normalizes paths without requiring them to exist - Properly separates path resolution from validation - Extract SSH command spawning pattern into execSSHCommand() helper - Reduces duplication in SSHRuntime - Reusable for simple SSH commands that return stdout - Remove SSH integration tests for resolvePath() - Will rely on createWorkspace matrix tests for end-to-end verification - Keep only constructor unit tests that don't require SSH - Update LocalRuntime test to expect non-existent paths to resolve successfully - Revert unrelated gitService.test.ts debug logging This improves separation of concerns: resolvePath() now purely resolves paths without side effects, allowing callers to decide whether to validate existence. Generated with `cmux`
Method no longer uses await, so removing async keyword to satisfy linter. Generated with `cmux`
- Add timeoutMs parameter to execSSHCommand() to prevent network hangs - Implement timeout with clearTimeout cleanup on success/error - Update resolvePath() to use 5 second timeout (should be near-instant) - Add class-level comment documenting timeout requirement for all SSH operations - Timeouts should be either set literally or forwarded from upstream This prevents indefinite hangs on network issues, dropped connections, or unresponsive SSH servers. Generated with `cmux`
…ogic - Change tests from expecting tilde rejection to expecting tilde resolution - Update "rejects tilde paths" → "resolves tilde paths to absolute paths" - Update "rejects bare tilde" → "resolves bare tilde to home directory" - Verify that resolved paths are stored in config (not tilde paths) - Remove skip logic for missing SSH server - SSH server is now required - Change skip checks to throw errors if SSH server unavailable - Fix type errors: use workspace.name instead of workspace.branch Tests now verify that: 1. Tildes are accepted in srcBaseDir for both local and SSH runtimes 2. Tildes are resolved to absolute paths via runtime.resolvePath() 3. Resolved paths are stored in config (ensuring consistency) 4. SSH server must be available for SSH tests (no silent skips) Generated with `cmux`
ammar-agent
changed the title
ammario
changed the title
Add note to Testing section that tests in tests/ folder must be run with 'bun x jest' (not 'bun test') since they use Jest as the test runner. Unit tests in src/ use bun test, integration tests in tests/ use Jest. Generated with `cmux`
Use simple bash echo to expand tildes instead of readlink -m, which is not available in BusyBox (used in SSH Docker test container). The command `bash -c 'p=$path; echo $p'` lets bash expand ~ naturally without requiring GNU coreutils or python3. This fixes runtimeFileEditing integration tests that were failing with: readlink: unrecognized option: m All tests now pass (8/8 in runtimeFileEditing). Generated with `cmux`
ammar-agent
added a commit
that referenced
this pull request
Oct 27, 2025
- Change parseRuntimeString to use ~/cmux instead of process.env.USER - Backend resolves tildes via runtime.resolvePath() (from PR #451) - Add eslint rule to prevent process.env usage in renderer code - Prevents 'process is not defined' error in workspace creation This fixes the regression introduced in PR #451 where chatCommands.ts (renderer code) was trying to access process.env.USER to construct SSH paths. Now we just use ~/cmux and let the backend resolve it. ESLint rule catches future attempts to use process.env in renderer: - Applies to src/**/*.ts(x) except main, preload, services, runtime, etc. - Error: 'Renderer code cannot access process.env' Generated with `cmux`
ammario
pushed a commit
that referenced
this pull request
Oct 27, 2025
…#458) Fixes regression from #451 where was using `process.env.USER` in renderer code, causing "process is not defined" error during workspace creation. ## Problem After merging #451, workspace creation fails with: ``` ReferenceError: process is not defined at parseRuntimeString (chatCommands.ts:56:66) ``` The issue: `chatCommands.ts` is renderer code that was trying to access `process.env.USER` to construct SSH default paths. The `process` global is not available in Electron renderer processes. ## Solution **Use tilde paths instead** - Now that we have tilde resolution from #451, we can simply use `~/cmux` as the default SSH `srcBaseDir`. The backend will resolve it via `runtime.resolvePath()`. Before: ```typescript const user = atIndex > 0 ? hostPart.substring(0, atIndex) : (process.env.USER ?? "user"); const homeDir = user === "root" ? "/root" : `/home/${user}`; return { type: "ssh", host: hostPart, srcBaseDir: `${homeDir}/cmux`, }; ``` After: ```typescript return { type: "ssh", host: hostPart, srcBaseDir: "~/cmux", // Backend resolves via runtime.resolvePath() }; ``` ## ESLint Rule Added `no-restricted-globals` and `no-restricted-syntax` rules to prevent future `process.env` usage in renderer code: - Applies to: `src/**/*.ts(x)` - Excludes: main, preload, services, runtime, telemetry, utils/main, utils/providers, tests - Error message guides developers to use IPC or constants instead This catches the issue at development time rather than runtime. ## Testing - Unit tests updated to expect `~/cmux` instead of computed paths - ESLint passes with new rules - Workspace creation now works without process.env _Generated with `cmux`_
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds
Runtime.resolvePath()method to expand tildes and normalize paths at workspace creation, ensuring consistent absolute paths are stored in config for both local and SSH runtimes.Changes
Runtime Interface:
resolvePath(filePath: string): Promise<string>to Runtime interfaceLocalRuntime:
~to home directory using existingexpandTilde()utility./foo) to absolute paths viapath.resolve()~/workspace→/home/user/workspaceSSHRuntime:
readlink -mcommand to normalize paths without requiring they exist~to remote user's home directoryexecSSHCommand()helper to reduce duplication~/cmux→/home/testuser/cmuxWORKSPACE_CREATE Handler:
srcBaseDirviaruntime.resolvePath()Integration Tests:
Benefits
~/workspaceand/home/user/workspacework and store the same resolved pathTechnical Notes
Why separate resolution from validation:
~, normalizing./../)Timeout requirement:
execSSHCommand()now requires explicit timeout parameterGenerated with
cmux