From 7b359240df80287d499e0ffda51109672a41fa5a Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Sat, 13 Jun 2026 03:01:39 +0000 Subject: [PATCH] codex: address PR review feedback (#27819) --- .../utils/path-uri/src/native_path_string.rs | 16 +- .../path-uri/src/native_path_string_tests.rs | 537 +++++++++++------- 2 files changed, 332 insertions(+), 221 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 c732a5b582..df9cf917f0 100644 --- a/codex-rs/utils/path-uri/src/native_path_string.rs +++ b/codex-rs/utils/path-uri/src/native_path_string.rs @@ -56,6 +56,12 @@ pub struct NativePathString(String); impl NativePathString { /// Renders a path URI using the requested native path convention. + /// + /// 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. pub fn from_path_uri( path: &PathUri, convention: PathConvention, @@ -63,11 +69,11 @@ impl NativePathString { if let Some(path_bytes) = path.opaque_fallback_bytes() { return render_opaque_fallback(path, &path_bytes, convention).map(Self); } - let value = match convention { - PathConvention::Posix => render_posix_path(path)?, - PathConvention::Windows => render_windows_path(path)?, - }; - Ok(Self(value)) + match convention { + PathConvention::Posix => render_posix_path(path), + PathConvention::Windows => render_windows_path(path), + } + .map(Self) } pub fn as_str(&self) -> &str { 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 52061b073d..3f84039a45 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 @@ -2,183 +2,341 @@ use super::*; use crate::PathUri; use pretty_assertions::assert_eq; -#[test] -fn renders_posix_paths_on_every_host() { - for (uri, expected) in [ - ("file:///", "/"), - ("file:///home/alice/src/main.rs", "/home/alice/src/main.rs"), - ("file:///home/alice/a%20file.rs", "/home/alice/a file.rs"), - ("file:///workspace/src/lib.rs", "/workspace/src/lib.rs"), - ( - "file:///workspace/tests/test.rs", - "/workspace/tests/test.rs", - ), - ("file:///etc", "/etc"), - ("file:///tmp/", "/tmp/"), - ("file:///C:/Project", "/C:/Project"), - ("file:///C:", "/C:"), - ("file:///tmp/%E2%98%83", "/tmp/☃"), - ("file:///tmp/a%5Cb", "/tmp/a\\b"), - ("file:///tmp/100%25/file", "/tmp/100%/file"), - ("file:///tmp/a%3Fb%23c%25d", "/tmp/a?b#c%d"), - ("file:///tmp/a%252Fb", "/tmp/a%2Fb"), - ( - "file:///bad/path/L3RtcC9udWxsLQAt_y1ieXRl", - "/bad/path/L3RtcC9udWxsLQAt_y1ieXRl", - ), - ("FILE:///workspace/src", "/workspace/src"), - ("file:/workspace/src", "/workspace/src"), - ("file://localhost/workspace/src", "/workspace/src"), - ("file://LOCALHOST/workspace/src", "/workspace/src"), - ] { - let path = PathUri::parse(uri).expect("valid file URI"); - assert_eq!( - NativePathString::from_path_uri(&path, PathConvention::Posix) - .map(NativePathString::into_string), - Ok(expected.to_string()), - "rendering {uri}" - ); +#[derive(Clone, Copy, Debug)] +struct RenderCase { + uri: &'static str, + convention: PathConvention, + expected: RenderExpectation, +} + +impl RenderCase { + const fn renders( + uri: &'static str, + convention: PathConvention, + rendered: &'static str, + ) -> Self { + Self { + uri, + convention, + expected: RenderExpectation::Rendered(rendered), + } + } + + const fn rejects(uri: &'static str, convention: PathConvention, error: ExpectedError) -> Self { + Self { + uri, + convention, + expected: RenderExpectation::Error(error), + } } } -#[test] -fn renders_windows_drive_paths_on_every_host() { - for (uri, expected) in [ - ( - "file:///C:/Users/Alice%20Smith/src/main.rs", - r"C:\Users\Alice Smith\src\main.rs", - ), - ("file:///C:/", "C:\\"), - ("file:///C:", "C:\\"), - ("file:///C:/Users", r"C:\Users"), - ("file:///C:/Windows", r"C:\Windows"), - ("file:///d:/snowman/%E2%98%83", r"d:\snowman\☃"), - ("file:///C:/tmp/", "C:\\tmp\\"), - ("file:///C:/test%20with%20%25/path", r"C:\test with %\path"), - ( - "file:///C:/test%20with%20%2525/c%23code", - r"C:\test with %25\c#code", - ), - ( - "file:///C:/Source/Z%C3%BCrich%20or%20Zurich%20(%CB%88zj%CA%8A%C9%99r%C9%AAk,/Code/resources/app/plugins/c%23/plugin.json", - r"C:\Source\Zürich or Zurich (ˈzjʊərɪk,\Code\resources\app\plugins\c#\plugin.json", - ), - ( - "file:///C:/Users/Abd-al-Haseeb%27s_Dell/Studio/w3mage/wp-content/database.ht.sqlite", - r"C:\Users\Abd-al-Haseeb's_Dell\Studio\w3mage\wp-content\database.ht.sqlite", - ), - ("file:///C:/project/%25A0.txt", r"C:\project\%A0.txt"), - ("file:///C:/project/%252e.txt", r"C:\project\%2e.txt"), - ] { - let path = PathUri::parse(uri).expect("valid file URI"); - assert_eq!( - NativePathString::from_path_uri(&path, PathConvention::Windows) - .map(NativePathString::into_string), - Ok(expected.to_string()), - "rendering {uri}" - ); - } +#[derive(Clone, Copy, Debug)] +enum RenderExpectation { + Rendered(&'static str), + Error(ExpectedError), } -#[test] -fn renders_windows_unc_paths_on_every_host() { - for (uri, expected) in [ - ( - "file://server/share/src/main.rs", - r"\\server\share\src\main.rs", - ), - ("file://server/share", r"\\server\share"), - ("file://server/share/", "\\\\server\\share\\"), - ("file://shares/files/c%23/p.cs", r"\\shares\files\c#\p.cs"), - ( - "file://monacotools1/certificates/SSL/", - "\\\\monacotools1\\certificates\\SSL\\", - ), - ] { - let path = PathUri::parse(uri).expect("valid file URI"); - assert_eq!( - NativePathString::from_path_uri(&path, PathConvention::Windows) - .map(NativePathString::into_string), - Ok(expected.to_string()), - "rendering {uri}" - ); - } +#[derive(Clone, Copy, Debug)] +enum ExpectedError { + OpaqueFallback, + IncompatibleConvention, + NonUtf8, + EncodedSeparator, } -#[test] -fn rejects_paths_incompatible_with_the_convention() { - for (uri, convention) in [ - ("file://server/share/file.txt", PathConvention::Posix), - ("file://server/share/file.rs", PathConvention::Posix), - ("file:///usr/local/file.txt", PathConvention::Windows), - ("file:///home/alice/file.rs", PathConvention::Windows), - ("file://server/", PathConvention::Windows), - ("file:///_:/path", PathConvention::Windows), - ] { - let path = PathUri::parse(uri).expect("valid file URI"); - assert!(matches!( - NativePathString::from_path_uri(&path, convention), - Err(NativePathStringError::IncompatibleConvention { .. }) - )); - } -} +const RENDER_CASES: &[RenderCase] = &[ + // POSIX paths. + RenderCase::renders("file:///", PathConvention::Posix, "/"), + RenderCase::renders( + "file:///home/alice/src/main.rs", + PathConvention::Posix, + "/home/alice/src/main.rs", + ), + RenderCase::renders( + "file:///home/alice/a%20file.rs", + PathConvention::Posix, + "/home/alice/a file.rs", + ), + RenderCase::renders( + "file:///workspace/src/lib.rs", + PathConvention::Posix, + "/workspace/src/lib.rs", + ), + RenderCase::renders( + "file:///workspace/tests/test.rs", + PathConvention::Posix, + "/workspace/tests/test.rs", + ), + RenderCase::renders("file:///etc", PathConvention::Posix, "/etc"), + RenderCase::renders("file:///tmp/", PathConvention::Posix, "/tmp/"), + RenderCase::renders("file:///C:/Project", PathConvention::Posix, "/C:/Project"), + RenderCase::renders("file:///C:", PathConvention::Posix, "/C:"), + RenderCase::renders("file:///tmp/%E2%98%83", PathConvention::Posix, "/tmp/☃"), + RenderCase::renders("file:///tmp/a%5Cb", PathConvention::Posix, "/tmp/a\\b"), + RenderCase::renders( + "file:///tmp/100%25/file", + PathConvention::Posix, + "/tmp/100%/file", + ), + RenderCase::renders( + "file:///tmp/a%3Fb%23c%25d", + PathConvention::Posix, + "/tmp/a?b#c%d", + ), + RenderCase::renders("file:///tmp/a%252Fb", PathConvention::Posix, "/tmp/a%2Fb"), + RenderCase::renders( + "file:///bad/path/L3RtcC9udWxsLQAt_y1ieXRl", + PathConvention::Posix, + "/bad/path/L3RtcC9udWxsLQAt_y1ieXRl", + ), + RenderCase::renders( + "FILE:///workspace/src", + PathConvention::Posix, + "/workspace/src", + ), + RenderCase::renders( + "file:/workspace/src", + PathConvention::Posix, + "/workspace/src", + ), + RenderCase::renders( + "file://localhost/workspace/src", + PathConvention::Posix, + "/workspace/src", + ), + RenderCase::renders( + "file://LOCALHOST/workspace/src", + PathConvention::Posix, + "/workspace/src", + ), + // Windows drive paths. + RenderCase::renders( + "file:///C:/Users/Alice%20Smith/src/main.rs", + PathConvention::Windows, + r"C:\Users\Alice Smith\src\main.rs", + ), + RenderCase::renders("file:///C:/", PathConvention::Windows, "C:\\"), + RenderCase::renders("file:///C:", PathConvention::Windows, "C:\\"), + RenderCase::renders("file:///C:/Users", PathConvention::Windows, r"C:\Users"), + RenderCase::renders("file:///C:/Windows", PathConvention::Windows, r"C:\Windows"), + RenderCase::renders( + "file:///d:/snowman/%E2%98%83", + PathConvention::Windows, + r"d:\snowman\☃", + ), + RenderCase::renders("file:///C:/tmp/", PathConvention::Windows, "C:\\tmp\\"), + RenderCase::renders( + "file:///C:/test%20with%20%25/path", + PathConvention::Windows, + r"C:\test with %\path", + ), + RenderCase::renders( + "file:///C:/test%20with%20%2525/c%23code", + PathConvention::Windows, + r"C:\test with %25\c#code", + ), + RenderCase::renders( + "file:///C:/Source/Z%C3%BCrich%20or%20Zurich%20(%CB%88zj%CA%8A%C9%99r%C9%AAk,/Code/resources/app/plugins/c%23/plugin.json", + PathConvention::Windows, + r"C:\Source\Zürich or Zurich (ˈzjʊərɪk,\Code\resources\app\plugins\c#\plugin.json", + ), + RenderCase::renders( + "file:///C:/Users/Abd-al-Haseeb%27s_Dell/Studio/w3mage/wp-content/database.ht.sqlite", + PathConvention::Windows, + r"C:\Users\Abd-al-Haseeb's_Dell\Studio\w3mage\wp-content\database.ht.sqlite", + ), + RenderCase::renders( + "file:///C:/project/%25A0.txt", + PathConvention::Windows, + r"C:\project\%A0.txt", + ), + RenderCase::renders( + "file:///C:/project/%252e.txt", + PathConvention::Windows, + r"C:\project\%2e.txt", + ), + // Windows UNC paths. + RenderCase::renders( + "file://server/share/src/main.rs", + PathConvention::Windows, + r"\\server\share\src\main.rs", + ), + RenderCase::renders( + "file://server/share", + PathConvention::Windows, + r"\\server\share", + ), + RenderCase::renders( + "file://server/share/", + PathConvention::Windows, + "\\\\server\\share\\", + ), + RenderCase::renders( + "file://shares/files/c%23/p.cs", + PathConvention::Windows, + r"\\shares\files\c#\p.cs", + ), + RenderCase::renders( + "file://monacotools1/certificates/SSL/", + PathConvention::Windows, + "\\\\monacotools1\\certificates\\SSL\\", + ), + // Opaque fallbacks rendered according to their source convention. + RenderCase::renders( + "file:///%00/bad/path/L3RtcC9udWxsLQAt_y1ieXRl", + PathConvention::Posix, + "/tmp/null-\0-�-byte", + ), + RenderCase::renders( + "file:///%00/bad/path/XABcAC4AXABDAE8ATQAxAFwA", + PathConvention::Windows, + r"\\.\COM1\", + ), + RenderCase::renders( + "file:///%00/bad/path/XABcAD8AXABWAG8AbAB1AG0AZQB7ADAAMAAwADAAMAAwADAAMAAtADAAMAAwADAALQAwADAAMAAwAC0AMAAwADAAMAAtADAAMAAwADAAMAAwADAAMAAwADAAMAAwAH0AXABmAGkAbABlAC4AcgBzAA", + PathConvention::Windows, + r"\\?\Volume{00000000-0000-0000-0000-000000000000}\file.rs", + ), + // Windows rendering preserves path text without filesystem validation. + RenderCase::renders("file:///C:/a%3Fb", PathConvention::Windows, "C:\\a?b"), + RenderCase::renders("file:///C:/a%2Ab", PathConvention::Windows, "C:\\a*b"), + RenderCase::renders( + "file:///C:/trailing.", + PathConvention::Windows, + "C:\\trailing.", + ), + RenderCase::renders( + "file:///C:/trailing%20", + PathConvention::Windows, + "C:\\trailing ", + ), + RenderCase::renders( + "file:///C:/control-%01", + PathConvention::Windows, + "C:\\control-\u{1}", + ), + RenderCase::renders( + "file:///C:/file.txt:stream", + PathConvention::Windows, + "C:\\file.txt:stream", + ), + RenderCase::renders( + "file://server/sh%3Fare/file.rs", + PathConvention::Windows, + "\\\\server\\sh?are\\file.rs", + ), + // URI shapes that do not match the requested convention. + RenderCase::rejects( + "file://server/share/file.txt", + PathConvention::Posix, + ExpectedError::IncompatibleConvention, + ), + RenderCase::rejects( + "file://server/share/file.rs", + PathConvention::Posix, + ExpectedError::IncompatibleConvention, + ), + RenderCase::rejects( + "file:///usr/local/file.txt", + PathConvention::Windows, + ExpectedError::IncompatibleConvention, + ), + RenderCase::rejects( + "file:///home/alice/file.rs", + PathConvention::Windows, + ExpectedError::IncompatibleConvention, + ), + RenderCase::rejects( + "file://server/", + PathConvention::Windows, + ExpectedError::IncompatibleConvention, + ), + RenderCase::rejects( + "file:///_:/path", + PathConvention::Windows, + ExpectedError::IncompatibleConvention, + ), + // Invalid opaque fallback payloads. + RenderCase::rejects( + "file:///%00/bad/path/YQ", + PathConvention::Posix, + ExpectedError::OpaqueFallback, + ), + RenderCase::rejects( + "file:///%00/bad/path/L3RtcC9udWxsLQAt_y1ieXRl", + PathConvention::Windows, + ExpectedError::OpaqueFallback, + ), + // URI segment encodings that cannot be rendered without changing meaning. + RenderCase::rejects( + "file:///tmp/non-utf8-%FF", + PathConvention::Posix, + ExpectedError::NonUtf8, + ), + RenderCase::rejects( + "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, + ), +]; #[test] -fn rejects_opaque_fallback_paths_that_cannot_be_recovered() { - let path = PathUri::parse("file:///%00/bad/path/YQ").expect("canonical opaque fallback URI"); - - assert_eq!( - NativePathString::from_path_uri(&path, PathConvention::native()), - Err(NativePathStringError::OpaqueFallback { - path: path.to_string(), - }) - ); -} - -#[test] -fn renders_posix_opaque_fallback_paths_lossily_on_every_host() { - let path = PathUri::parse("file:///%00/bad/path/L3RtcC9udWxsLQAt_y1ieXRl") - .expect("valid POSIX opaque fallback URI"); - - assert_eq!( - NativePathString::from_path_uri(&path, PathConvention::Posix) - .map(NativePathString::into_string), - Ok("/tmp/null-\0-�-byte".to_string()) - ); - assert_eq!( - NativePathString::from_path_uri(&path, PathConvention::Windows), - Err(NativePathStringError::OpaqueFallback { - path: path.to_string(), - }) - ); -} - -#[test] -fn renders_windows_namespace_fallback_paths_on_every_host() { - for (uri, expected) in [ - ( - "file:///%00/bad/path/XABcAC4AXABDAE8ATQAxAFwA", - r"\\.\COM1\", - ), - ( - "file:///%00/bad/path/XABcAD8AXABWAG8AbAB1AG0AZQB7ADAAMAAwADAAMAAwADAAMAAtADAAMAAwADAALQAwADAAMAAwAC0AMAAwADAAMAAtADAAMAAwADAAMAAwADAAMAAwADAAMAAwAH0AXABmAGkAbABlAC4AcgBzAA", - r"\\?\Volume{00000000-0000-0000-0000-000000000000}\file.rs", - ), - ] { - let path = PathUri::parse(uri).expect("valid Windows opaque fallback URI"); +fn renders_native_paths_from_shared_cases() { + for case in RENDER_CASES { + let path = PathUri::parse(case.uri).expect("valid file URI"); + let expected = match case.expected { + RenderExpectation::Rendered(rendered) => Ok(NativePathString(rendered.to_string())), + RenderExpectation::Error(ExpectedError::OpaqueFallback) => { + Err(NativePathStringError::OpaqueFallback { + path: path.to_string(), + }) + } + RenderExpectation::Error(ExpectedError::IncompatibleConvention) => { + Err(NativePathStringError::IncompatibleConvention { + path: path.to_string(), + 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!( - NativePathString::from_path_uri(&path, PathConvention::Windows) - .map(NativePathString::into_string), - Ok(expected.to_string()), - "rendering {uri}" + NativePathString::from_path_uri(&path, case.convention), + expected, + "rendering {case:?}" ); } } #[cfg(windows)] #[test] -fn renders_native_opaque_fallback_paths_lossily() { +fn renders_native_non_unicode_windows_fallback_lossily() { use std::os::windows::ffi::OsStringExt; let native_path = std::path::PathBuf::from(std::ffi::OsString::from_wide( @@ -190,70 +348,17 @@ fn renders_native_opaque_fallback_paths_lossily() { let path = PathUri::from_path(native_path).expect("absolute native path"); assert_eq!( - NativePathString::from_path_uri(&path, PathConvention::Windows) - .map(NativePathString::into_string), - Ok(r"C:\bad\�".to_string()) + NativePathString::from_path_uri(&path, PathConvention::Windows), + Ok(NativePathString(r"C:\bad\�".to_string())) ); assert_eq!( NativePathString::from_path_uri(&path, PathConvention::Posix), - Err(NativePathStringError::IncompatibleConvention { + Err(NativePathStringError::OpaqueFallback { path: path.to_string(), - convention: PathConvention::Posix, }) ); } -#[test] -fn rejects_non_utf8_paths() { - for uri in ["file:///tmp/non-utf8-%FF", "file:///tmp/non-utf8-%A0"] { - let path = PathUri::parse(uri).expect("valid file URI"); - - assert!(matches!( - NativePathString::from_path_uri(&path, PathConvention::Posix), - Err(NativePathStringError::NonUtf8 { .. }) - )); - } -} - -#[test] -fn rejects_encoded_separators() { - for (uri, convention) in [ - ("file:///tmp/a%2Fb", PathConvention::Posix), - ("file:///C:/a%2Fb", PathConvention::Windows), - ("file:///C:/a%5Cb", PathConvention::Windows), - ] { - let path = PathUri::parse(uri).expect("valid file URI"); - assert!(matches!( - NativePathString::from_path_uri(&path, convention), - Err(NativePathStringError::EncodedSeparator { .. }) - )); - } -} - -#[test] -fn renders_windows_components_without_filesystem_validation() { - for (uri, expected) in [ - ("file:///C:/a%3Fb", "C:\\a?b"), - ("file:///C:/a%2Ab", "C:\\a*b"), - ("file:///C:/trailing.", "C:\\trailing."), - ("file:///C:/trailing%20", "C:\\trailing "), - ("file:///C:/control-%01", "C:\\control-\u{1}"), - ("file:///C:/file.txt:stream", "C:\\file.txt:stream"), - ( - "file://server/sh%3Fare/file.rs", - "\\\\server\\sh?are\\file.rs", - ), - ] { - let path = PathUri::parse(uri).expect("valid file URI"); - - assert_eq!( - NativePathString::from_path_uri(&path, PathConvention::Windows), - Ok(NativePathString(expected.to_string())), - "rendering {uri}" - ); - } -} - #[test] fn serializes_as_a_string() { let path = PathUri::parse("file:///workspace/src/lib.rs").expect("valid file URI");