-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix(tool): normalize Windows drive aliases for bash paths #2635
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,107 @@ | ||
| //! Filesystem utilities | ||
|
|
||
| use std::{borrow::Cow, path::Path}; | ||
| #[cfg(any(windows, test))] | ||
| use std::path::PathBuf; | ||
|
|
||
| /// Normalizes shell-facing path aliases before std::fs sees them. | ||
| pub fn normalize_shell_path(path: &Path) -> Cow<'_, Path> { | ||
| #[cfg(windows)] | ||
| { | ||
| translate_unix_drive_path(path).map_or(Cow::Borrowed(path), Cow::Owned) | ||
| } | ||
| #[cfg(not(windows))] | ||
| { | ||
| Cow::Borrowed(path) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(any(windows, test))] | ||
| fn translate_unix_drive_path(path: &Path) -> Option<PathBuf> { | ||
| let raw = path.to_str()?; | ||
| let bytes = raw.as_bytes(); | ||
| let (drive, tail) = drive_alias_parts(bytes)?; | ||
|
|
||
| let mut native = String::with_capacity(3 + tail.len()); | ||
| native.push(char::from(drive).to_ascii_uppercase()); | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On Windows, when an MSYS/WSL alias contains non-ASCII path text such as Useful? React with 👍 / 👎. |
||
| } | ||
| Some(PathBuf::from(native)) | ||
| } | ||
|
|
||
| #[cfg(any(windows, test))] | ||
| fn drive_alias_parts(bytes: &[u8]) -> Option<(u8, &[u8])> { | ||
| if bytes.len() >= 2 | ||
| && bytes[0] == b'/' | ||
| && bytes[1].is_ascii_alphabetic() | ||
| && bytes.get(2).is_none_or(|byte| *byte == b'/') | ||
| { | ||
| let tail = if bytes.len() > 2 { &bytes[3..] } else { &[] }; | ||
| return Some((bytes[1], tail)); | ||
| } | ||
|
|
||
| if bytes.len() >= 6 | ||
| && bytes[0] == b'/' | ||
| && bytes[1..4].eq_ignore_ascii_case(b"mnt") | ||
| && bytes[4] == b'/' | ||
| && bytes[5].is_ascii_alphabetic() | ||
| && bytes.get(6).is_none_or(|byte| *byte == b'/') | ||
| { | ||
| let tail = if bytes.len() > 6 { &bytes[7..] } else { &[] }; | ||
| return Some((bytes[5], tail)); | ||
| } | ||
|
|
||
| None | ||
| } | ||
|
|
||
| #[cfg(any(windows, test))] | ||
| const fn is_path_separator(byte: u8) -> bool { | ||
| byte == b'/' || byte == b'\\' | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On Windows this makes the alias detector accept a leading backslash as if it were an MSYS/WSL slash, so Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn unix_drive_aliases_translate_to_windows_roots() { | ||
| assert_eq!(translate_unix_drive_path(Path::new("/c")).as_deref(), Some(Path::new("C:\\"))); | ||
| assert_eq!( | ||
| translate_unix_drive_path(Path::new("/d/project/app")).as_deref(), | ||
| Some(Path::new("D:\\project\\app")), | ||
| ); | ||
| assert_eq!( | ||
| translate_unix_drive_path(Path::new("/D/project")).as_deref(), | ||
| Some(Path::new("D:\\project")), | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn wsl_mount_drive_aliases_translate_to_windows_roots() { | ||
| assert_eq!( | ||
| translate_unix_drive_path(Path::new("/mnt/d/project")).as_deref(), | ||
| Some(Path::new("D:\\project")), | ||
| ); | ||
| assert_eq!( | ||
| translate_unix_drive_path(Path::new("/MNT/c")).as_deref(), | ||
| Some(Path::new("C:\\")), | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn non_drive_absolute_paths_are_left_native() { | ||
| assert_eq!(translate_unix_drive_path(Path::new("/")).as_deref(), None); | ||
| assert_eq!(translate_unix_drive_path(Path::new("/dev/null")).as_deref(), None); | ||
| assert_eq!(translate_unix_drive_path(Path::new("/mnt/data")).as_deref(), None); | ||
| assert_eq!(translate_unix_drive_path(Path::new("relative/path")).as_deref(), None); | ||
| assert_eq!(translate_unix_drive_path(Path::new("\\d\\logs")).as_deref(), None); | ||
| assert_eq!(translate_unix_drive_path(Path::new("\\mnt\\d\\logs")).as_deref(), None); | ||
| } | ||
| } | ||
|
|
||
| pub use super::platform::fs::*; | ||
|
|
||
| /// Extension trait for path-related filesystem operations. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -143,6 +143,37 @@ export function expandPath(filePath: string): string { | |
| const normalized = stripFileUrl(normalizeUnicodeSpaces(normalizeAtPrefix(filePath))); | ||
| return expandTilde(normalized); | ||
| } | ||
|
|
||
| function isAsciiDriveLetter(value: string): boolean { | ||
| if (value.length !== 1) return false; | ||
| const code = value.charCodeAt(0); | ||
| return (code >= 65 && code <= 90) || (code >= 97 && code <= 122); | ||
| } | ||
|
|
||
| function windowsDriveAliasPath(filePath: string): string | undefined { | ||
| if (!filePath.startsWith("/")) return undefined; | ||
| const parts = filePath.split("/"); | ||
| if (parts[0] !== "") return undefined; | ||
|
|
||
| let drive: string | undefined; | ||
| let tailStart = 2; | ||
| if (parts.length >= 2 && isAsciiDriveLetter(parts[1] ?? "")) { | ||
| drive = parts[1]!.toUpperCase(); | ||
| } else if (parts.length >= 3 && (parts[1] ?? "").toLowerCase() === "mnt" && isAsciiDriveLetter(parts[2] ?? "")) { | ||
| drive = parts[2]!.toUpperCase(); | ||
| tailStart = 3; | ||
| } | ||
| if (!drive) return undefined; | ||
|
|
||
| const tail = parts.slice(tailStart).filter(Boolean).join("\\"); | ||
| return tail ? `${drive}:\\${tail}` : `${drive}:\\`; | ||
| } | ||
|
|
||
| export function normalizeWindowsDriveAliasPath(filePath: string, platform: NodeJS.Platform = process.platform): string { | ||
| if (platform !== "win32") return filePath; | ||
| return windowsDriveAliasPath(filePath) ?? filePath; | ||
| } | ||
|
|
||
| /** | ||
| * Inclusive line range describing one selector segment (e.g. `50-100`, | ||
| * `301-`, or `50+10`). `endLine` is `undefined` for open-ended ranges. | ||
|
|
@@ -353,7 +384,7 @@ export function isInternalUrlPath(filePath: string): boolean { | |
| */ | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fresh evidence: Useful? React with 👍 / 👎. |
||
| const expandedAndNormalized = normalizeLocalScheme(expanded); | ||
|
|
||
| assertNotInternalUrl(expandedAndNormalized, normalized); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { describe, expect, it } from "bun:test"; | ||
| import { normalizeWindowsDriveAliasPath } from "@oh-my-pi/pi-coding-agent/tools/path-utils"; | ||
|
|
||
| describe("Windows drive alias paths", () => { | ||
| it("maps MSYS drive roots to native Windows paths", () => { | ||
| expect(normalizeWindowsDriveAliasPath("/c", "win32")).toBe("C:\\"); | ||
| expect(normalizeWindowsDriveAliasPath("/d/project/app", "win32")).toBe("D:\\project\\app"); | ||
| expect(normalizeWindowsDriveAliasPath("/D/project", "win32")).toBe("D:\\project"); | ||
| }); | ||
|
|
||
| it("maps WSL mount roots to native Windows paths", () => { | ||
| expect(normalizeWindowsDriveAliasPath("/mnt/d/project", "win32")).toBe("D:\\project"); | ||
| expect(normalizeWindowsDriveAliasPath("/MNT/c", "win32")).toBe("C:\\"); | ||
| }); | ||
|
|
||
| it("leaves non-drive absolute paths and non-Windows platforms unchanged", () => { | ||
| expect(normalizeWindowsDriveAliasPath("/", "win32")).toBe("/"); | ||
| expect(normalizeWindowsDriveAliasPath("/dev/null", "win32")).toBe("/dev/null"); | ||
| expect(normalizeWindowsDriveAliasPath("/mnt/data", "win32")).toBe("/mnt/data"); | ||
| expect(normalizeWindowsDriveAliasPath("/d/project", "linux")).toBe("/d/project"); | ||
| expect(normalizeWindowsDriveAliasPath("\\d\\logs", "win32")).toBe("\\d\\logs"); | ||
| expect(normalizeWindowsDriveAliasPath("\\mnt\\d\\logs", "win32")).toBe("\\mnt\\d\\logs"); | ||
| }); | ||
| }); |
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.
When the command word contains a glob, brush expands it in
Pattern::expandbefore the command receives arguments; I checked that path incrates/brush-core-vendored/src/patterns.rs, and it builds roots viasys::fs::pattern_path_rootand callsread_dir()directly without going throughShell::absolute_path. On Windows this meansecho /d/project/*.txtstill enumerates\d\projecton the current drive (or leaves the literal unmatched) while/d/project/file.txtnow works, because the new drive-alias translation is only installed on thisabsolute_pathpath. Please apply the same alias normalization in pathname expansion/root parsing.Useful? React with 👍 / 👎.