From c99a7f235aa7232d0223e4bfbbbee4648b5769ee Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Sat, 13 Jun 2026 03:22:19 +0000 Subject: [PATCH] codex: address PR review feedback (#27819) --- .../utils/path-uri/src/native_path_string.rs | 48 ++++--------------- .../path-uri/src/native_path_string_tests.rs | 41 ++++------------ 2 files changed, 17 insertions(+), 72 deletions(-) diff --git a/codex-rs/utils/path-uri/src/native_path_string.rs b/codex-rs/utils/path-uri/src/native_path_string.rs index df9cf917f0..66ccf4f8ad 100644 --- a/codex-rs/utils/path-uri/src/native_path_string.rs +++ b/codex-rs/utils/path-uri/src/native_path_string.rs @@ -60,8 +60,8 @@ impl NativePathString { /// Rendering fails when the URI shape does not match the convention, such /// as a POSIX path rendered as Windows or a UNC path rendered as POSIX. It /// also fails when an opaque fallback does not encode an absolute path for - /// the convention, when a URI segment is not valid UTF-8, or when a segment - /// contains a percent-encoded separator that would change path structure. + /// the convention. Non-UTF-8 segments are rendered lossily, and encoded + /// separators are emitted as native path text. pub fn from_path_uri( path: &PathUri, convention: PathConvention, @@ -167,16 +167,11 @@ fn render_posix_path(path: &PathUri) -> Result { } // URI segments are already separated with `/` on every host. Decode each - // one independently so `file:///a%20dir/file` becomes `/a dir/file` while - // an encoded separator cannot silently introduce another path component. + // one independently so `file:///a%20dir/file` becomes `/a dir/file`. let mut rendered = String::new(); for segment in path_segments(&url) { rendered.push('/'); - rendered.push_str(&decode_native_segment( - path, - segment, - PathConvention::Posix, - )?); + rendered.push_str(&decode_native_segment(segment)); } Ok(rendered) } @@ -192,7 +187,7 @@ fn render_windows_path(path: &PathUri) -> Result let Some(share) = segments.next() else { return Err(incompatible_convention(path, PathConvention::Windows)); }; - let share = decode_native_segment(path, share, PathConvention::Windows)?; + let share = decode_native_segment(share); if share.is_empty() { return Err(incompatible_convention(path, PathConvention::Windows)); } @@ -207,7 +202,7 @@ fn render_windows_path(path: &PathUri) -> Result let Some(drive) = segments.next() else { return Err(incompatible_convention(path, PathConvention::Windows)); }; - let drive = decode_native_segment(path, drive, PathConvention::Windows)?; + let drive = decode_native_segment(drive); let bytes = drive.as_bytes(); if bytes.len() != 2 || !bytes[0].is_ascii_alphabetic() || bytes[1] != b':' { return Err(incompatible_convention(path, PathConvention::Windows)); @@ -218,7 +213,7 @@ fn render_windows_path(path: &PathUri) -> Result for segment in segments { // URL path separators become Windows separators after each component // has been decoded. - let segment = decode_native_segment(path, segment, PathConvention::Windows)?; + let segment = decode_native_segment(segment); rendered.push('\\'); rendered.push_str(&segment); } @@ -235,29 +230,11 @@ fn path_segments(url: &url::Url) -> std::str::Split<'_, char> { .unwrap_or_else(|| unreachable!("validated file URLs have path segments")) } -fn decode_native_segment( - path: &PathUri, - segment: &str, - convention: PathConvention, -) -> Result { +fn decode_native_segment(segment: &str) -> String { // Decode exactly once. Thus `%20` becomes a space and `%252F` becomes the // literal text `%2F`, rather than being decoded a second time into `/`. let bytes = urlencoding::decode_binary(segment.as_bytes()); - // A separator encoded inside one URI segment would change path structure: - // reject `%2F` for both conventions and `%5C` for Windows. - let contains_separator = - bytes.contains(&b'/') || (convention == PathConvention::Windows && bytes.contains(&b'\\')); - if contains_separator { - return Err(NativePathStringError::EncodedSeparator { - path: path.to_string(), - convention, - }); - } - std::str::from_utf8(&bytes) - .map(str::to_string) - .map_err(|_| NativePathStringError::NonUtf8 { - path: path.to_string(), - }) + String::from_utf8_lossy(&bytes).into_owned() } fn incompatible_convention(path: &PathUri, convention: PathConvention) -> NativePathStringError { @@ -276,13 +253,6 @@ pub enum NativePathStringError { path: String, convention: PathConvention, }, - #[error("path URI `{path}` contains path bytes that are not valid UTF-8")] - NonUtf8 { path: String }, - #[error("path URI `{path}` contains a percent-encoded separator for {convention} path syntax")] - EncodedSeparator { - path: String, - convention: PathConvention, - }, } #[cfg(test)] diff --git a/codex-rs/utils/path-uri/src/native_path_string_tests.rs b/codex-rs/utils/path-uri/src/native_path_string_tests.rs index 3f84039a45..559dae6551 100644 --- a/codex-rs/utils/path-uri/src/native_path_string_tests.rs +++ b/codex-rs/utils/path-uri/src/native_path_string_tests.rs @@ -41,8 +41,6 @@ enum RenderExpectation { enum ExpectedError { OpaqueFallback, IncompatibleConvention, - NonUtf8, - EncodedSeparator, } const RENDER_CASES: &[RenderCase] = &[ @@ -268,32 +266,20 @@ const RENDER_CASES: &[RenderCase] = &[ PathConvention::Windows, ExpectedError::OpaqueFallback, ), - // URI segment encodings that cannot be rendered without changing meaning. - RenderCase::rejects( + // Non-UTF-8 bytes and encoded separators are rendered lossily as path text. + RenderCase::renders( "file:///tmp/non-utf8-%FF", PathConvention::Posix, - ExpectedError::NonUtf8, + "/tmp/non-utf8-�", ), - RenderCase::rejects( + RenderCase::renders( "file:///tmp/non-utf8-%A0", PathConvention::Posix, - ExpectedError::NonUtf8, - ), - RenderCase::rejects( - "file:///tmp/a%2Fb", - PathConvention::Posix, - ExpectedError::EncodedSeparator, - ), - RenderCase::rejects( - "file:///C:/a%2Fb", - PathConvention::Windows, - ExpectedError::EncodedSeparator, - ), - RenderCase::rejects( - "file:///C:/a%5Cb", - PathConvention::Windows, - ExpectedError::EncodedSeparator, + "/tmp/non-utf8-�", ), + RenderCase::renders("file:///tmp/a%2Fb", PathConvention::Posix, "/tmp/a/b"), + RenderCase::renders("file:///C:/a%2Fb", PathConvention::Windows, "C:\\a/b"), + RenderCase::renders("file:///C:/a%5Cb", PathConvention::Windows, "C:\\a\\b"), ]; #[test] @@ -313,17 +299,6 @@ fn renders_native_paths_from_shared_cases() { convention: case.convention, }) } - RenderExpectation::Error(ExpectedError::NonUtf8) => { - Err(NativePathStringError::NonUtf8 { - path: path.to_string(), - }) - } - RenderExpectation::Error(ExpectedError::EncodedSeparator) => { - Err(NativePathStringError::EncodedSeparator { - path: path.to_string(), - convention: case.convention, - }) - } }; assert_eq!(