fix(tool): normalize Windows drive aliases for bash paths#2635
Conversation
Mapped MSYS and WSL drive aliases before bash cwd validation and brush filesystem resolution so cd, stat-style tests, and tool cwd handling agree on Windows. Fixes #2634
![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: 3f74f48776
ℹ️ 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".
| } | ||
|
|
||
| function windowsDriveAliasPath(filePath: string): string | undefined { | ||
| const normalizedSeparators = filePath.replace(/\\/g, "/"); |
There was a problem hiding this comment.
On win32 this rewrites all backslashes to / before detecting /d aliases, so a legitimate Windows rooted path such as \d\logs (which path.win32.resolve("C:\\work", "\\d\\logs") resolves on the current C: drive) is treated as the MSYS alias and becomes D:\logs. Any tool cwd/path argument for a root-level one-letter directory is then validated or read on the wrong drive; only forward-slash aliases should be translated.
Useful? React with 👍 / 👎.
|
|
||
| #[cfg(any(windows, test))] | ||
| const fn is_path_separator(byte: u8) -> bool { | ||
| byte == b'/' || byte == b'\\' |
There was a problem hiding this comment.
On Windows this makes the alias detector accept a leading backslash as if it were an MSYS/WSL slash, so \d\logs is translated to D:\logs before absolute_path. A backslash-rooted path is a normal Windows path rooted on the current drive, so brush builtins like cd, redirects, and stat tests can operate on the wrong drive whenever the first root component is a single letter; restrict alias detection to / for the leading alias separators.
Useful? React with 👍 / 👎.
Restricted MSYS and WSL drive alias normalization to forward-slash roots so native Windows root-relative paths like \d\logs stay on the current drive. Fixes #2634
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e6a5fa812
ℹ️ 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".
| /// * `path` - The path to get the absolute form of. | ||
| pub fn absolute_path(&self, path: impl AsRef<Path>) -> PathBuf { | ||
| let path = path.as_ref(); | ||
| let normalized_path = crate::sys::fs::normalize_shell_path(path.as_ref()); |
There was a problem hiding this comment.
When the command word contains a glob, brush expands it in Pattern::expand before the command receives arguments; I checked that path in crates/brush-core-vendored/src/patterns.rs, and it builds roots via sys::fs::pattern_path_root and calls read_dir() directly without going through Shell::absolute_path. On Windows this means echo /d/project/*.txt still enumerates \d\project on the current drive (or leaves the literal unmatched) while /d/project/file.txt now works, because the new drive-alias translation is only installed on this absolute_path path. Please apply the same alias normalization in pathname expansion/root parsing.
Useful? React with 👍 / 👎.
Applied MSYS and WSL drive alias root parsing before brush pathname expansion reads directories, preserving backslash-rooted Windows paths. Fixes #2634
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6671e18994
ℹ️ 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".
| native.push(':'); | ||
| native.push('\\'); | ||
| for &byte in tail { | ||
| native.push(if is_path_separator(byte) { '\\' } else { char::from(byte) }); |
There was a problem hiding this comment.
On Windows, when an MSYS/WSL alias contains non-ASCII path text such as /d/Users/José, tail is the UTF-8 byte sequence from path.to_str(), but this loop appends each byte as an independent Unicode scalar. That turns é into é, so brush builtins resolve the wrong native path for any aliased path with non-ASCII characters.
Useful? React with 👍 / 👎.
| export function resolveToCwd(filePath: string, cwd: string): string { | ||
| const normalized = normalizeLocalScheme(filePath); | ||
| const expanded = expandPath(normalized); | ||
| const expanded = normalizeWindowsDriveAliasPath(expandPath(normalized)); |
There was a problem hiding this comment.
Fresh evidence: parseSearchPathPreferringLiteral still routes search/find targets through normalizePathSeparators before calling this resolver. On Windows, a root-relative backslash path like \d\logs becomes /d/logs, then this new alias normalization resolves it to D:\logs instead of path.win32.resolve(cwd, '\\d\\logs') on the current drive; the direct \d\logs test does not cover these parsed search/find paths.
Useful? React with 👍 / 👎.
|
1 participant
Repro
On the current branch, the tool-layer path resolver leaves MSYS drive aliases unchanged:
bun --print 'import { resolveToCwd } from "@oh-my-pi/pi-coding-agent/tools/path-utils"; resolveToCwd("/d/project", "D:/project")'prints/d/project, so bashcwdvalidation reachesfs.statwith the wrong Windows path. The same missing normalization exists in brush filesystem resolution used bycdand stat-backed builtins.Cause
packages/coding-agent/src/tools/path-utils.tsonly expanded~and then trusted platform absolute-path detection, which does not translate/d/...or/mnt/d/...before cwd validation. In brush,crates/brush-core-vendored/src/shell/fs.rspassed shell paths directly intostd::fs::metadata/OpenOptions; only glob pattern expansion had Windows drive-root awareness, solsandcddisagreed.Fix
/c/...,/d/..., and/mnt/<drive>/...aliases to native Windows drive paths before coding-agent tool path resolution.absolute_path, coveringcd, redirects, and stat-style builtins that resolve through the shell./,/dev/null, and non-Windows platforms unchanged.Verification
bun test packages/coding-agent/test/tools/windows-drive-alias.test.ts packages/coding-agent/test/tools/root-path-alias.test.tspassed.cargo test --manifest-path crates/brush-core-vendored/Cargo.toml sys::fs::testspassed.cargo check -p pi-shellpassed.bun checkpassed. I also attemptedcargo test -p pi-shell; it currently fails in two embedded-session tests with exit 143 on this Linux runner after 797/799 tests pass. Fixes #2634