codex: address PR review feedback (#27819)

codex-rs/utils/path-uri/src/api_path_string.rs

@@ -51,7 +51,7 @@ impl fmt::Display for PathConvention {
 /// accidentally applying the current host's path rules. Opaque fallback paths
 /// are decoded using the supplied convention and converted to UTF-8 lossily at
 /// this API boundary because the value is serialized as a JSON string.
-#[derive(Clone, Debug, PartialEq, Eq, Hash, TS)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash, Deserialize, TS)]
 #[ts(type = "string")]
 pub struct ApiPathString(String);
 
@@ -86,6 +86,19 @@ impl ApiPathString {
         .map(Self)
     }
 
+    /// Parses this API string as an absolute path using the requested native
+    /// path convention and returns its canonical path URI.
+    pub fn to_path_uri(&self, convention: PathConvention) -> Result<PathUri, ApiPathStringError> {
+        let path = match convention {
+            PathConvention::Posix => parse_posix_path(&self.0),
+            PathConvention::Windows => parse_windows_path(&self.0),
+        };
+        path.ok_or_else(|| ApiPathStringError::InvalidNativePath {
+            path: self.0.clone(),
+            convention,
+        })
+    }
+
     pub fn as_str(&self) -> &str {
         &self.0
     }
@@ -95,6 +108,88 @@ impl ApiPathString {
     }
 }
 
+fn parse_posix_path(path: &str) -> Option<PathUri> {
+    let path = path.strip_prefix('/')?;
+    if path.contains('\0') {
+        return Some(PathUri::from_opaque_path_bytes(
+            format!("/{path}").as_bytes(),
+        ));
+    }
+    path_uri_from_segments(None, path.split('/'))
+}
+
+fn parse_windows_path(path: &str) -> Option<PathUri> {
+    let bytes = path.as_bytes();
+    let uses_namespace = matches!(
+        bytes,
+        [first, second, namespace @ (b'.' | b'?'), separator, ..]
+            if is_windows_separator_byte(*first)
+                && is_windows_separator_byte(*second)
+                && is_windows_separator_byte(*separator)
+                && matches!(*namespace, b'.' | b'?')
+    );
+    if uses_namespace || path.contains('\0') {
+        return Some(windows_opaque_path_uri(path));
+    }
+
+    if matches!(
+        bytes,
+        [drive, b':', separator, ..]
+            if drive.is_ascii_alphabetic() && is_windows_separator_byte(*separator)
+    ) {
+        return path_uri_from_segments(
+            None,
+            std::iter::once(&path[..2]).chain(path[3..].split(is_windows_separator_char)),
+        );
+    }
+
+    if matches!(bytes, [first, second, ..]
+        if is_windows_separator_byte(*first) && is_windows_separator_byte(*second))
+    {
+        let mut components = path[2..].split(is_windows_separator_char);
+        let host = components.next().filter(|host| !host.is_empty())?;
+        let share = components.next().filter(|share| !share.is_empty())?;
+        return path_uri_from_segments(Some(host), std::iter::once(share).chain(components))
+            .or_else(|| Some(windows_opaque_path_uri(path)));
+    }
+
+    None
+}
+
+fn path_uri_from_segments<'a>(
+    host: Option<&str>,
+    segments: impl Iterator<Item = &'a str>,
+) -> Option<PathUri> {
+    let mut url = url::Url::parse("file:///").ok()?;
+    if let Some(host) = host {
+        url.set_host(Some(host)).ok()?;
+    }
+    {
+        let mut url_segments = url.path_segments_mut().ok()?;
+        url_segments.clear();
+        for segment in segments {
+            url_segments.push(segment);
+        }
+    }
+    PathUri::try_from(url).ok()
+}
+
+fn windows_opaque_path_uri(path: &str) -> PathUri {
+    let path_bytes = path
+        .encode_utf16()
+        .flat_map(u16::to_le_bytes)
+        .collect::<Vec<_>>();
+    PathUri::from_opaque_path_bytes(&path_bytes)
+}
+
+fn is_windows_separator_char(character: char) -> bool {
+    matches!(character, '\\' | '/')
+}
+
+fn is_windows_separator_byte(character: u8) -> bool {
+    matches!(character, b'\\' | b'/')
+}
+
 fn render_opaque_fallback(
     path: &PathUri,
     path_bytes: &[u8],
@@ -263,6 +358,11 @@ pub enum ApiPathStringError {
         path: String,
         convention: PathConvention,
     },
+    #[error("path `{path}` is not absolute using {convention} path syntax")]
+    InvalidNativePath {
+        path: String,
+        convention: PathConvention,
+    },
 }
 
 #[cfg(test)]

codex-rs/utils/path-uri/src/api_path_string_tests.rs

@@ -11,7 +11,7 @@ struct RenderCase {
 }
 
 impl RenderCase {
-    const fn renders(
+    const fn round_trips(
         uri: &'static str,
         convention: PathConvention,
         rendered: &'static str,
@@ -19,7 +19,7 @@ impl RenderCase {
         Self {
             uri,
             convention,
-            expected: RenderExpectation::Rendered(rendered),
+            expected: RenderExpectation::RoundTrip(rendered),
         }
     }
 
@@ -30,11 +30,24 @@ impl RenderCase {
             expected: RenderExpectation::Error(error),
         }
     }
+
+    const fn renders_lossily(
+        uri: &'static str,
+        convention: PathConvention,
+        rendered: &'static str,
+    ) -> Self {
+        Self {
+            uri,
+            convention,
+            expected: RenderExpectation::RenderOnly(rendered),
+        }
+    }
 }
 
 #[derive(Clone, Copy, Debug)]
 enum RenderExpectation {
-    Rendered(&'static str),
+    RoundTrip(&'static str),
+    RenderOnly(&'static str),
     Error(ExpectedError),
 }
 
@@ -46,185 +59,199 @@ enum ExpectedError {
 
 const RENDER_CASES: &[RenderCase] = &[
     // POSIX paths.
-    RenderCase::renders("file:///", PathConvention::Posix, "/"),
-    RenderCase::renders(
+    RenderCase::round_trips("file:///", PathConvention::Posix, "/"),
+    RenderCase::round_trips(
         "file:///home/alice/src/main.rs",
         PathConvention::Posix,
         "/home/alice/src/main.rs",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file:///home/alice/a%20file.rs",
         PathConvention::Posix,
         "/home/alice/a file.rs",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file:///workspace/src/lib.rs",
         PathConvention::Posix,
         "/workspace/src/lib.rs",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "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(
+    RenderCase::round_trips("file:///etc", PathConvention::Posix, "/etc"),
+    RenderCase::round_trips("file:///tmp/", PathConvention::Posix, "/tmp/"),
+    RenderCase::round_trips("file:///C:/Project", PathConvention::Posix, "/C:/Project"),
+    RenderCase::round_trips("file:///C:", PathConvention::Posix, "/C:"),
+    RenderCase::round_trips("file:///tmp/%E2%98%83", PathConvention::Posix, "/tmp/☃"),
+    RenderCase::round_trips("file:///tmp/a%5Cb", PathConvention::Posix, "/tmp/a\\b"),
+    RenderCase::round_trips(
         "file:///tmp/100%25/file",
         PathConvention::Posix,
         "/tmp/100%/file",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "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(
+    RenderCase::round_trips("file:///tmp/a%252Fb", PathConvention::Posix, "/tmp/a%2Fb"),
+    RenderCase::round_trips(
         "file:///bad/path/L3RtcC9udWxsLQAt_y1ieXRl",
         PathConvention::Posix,
         "/bad/path/L3RtcC9udWxsLQAt_y1ieXRl",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "FILE:///workspace/src",
         PathConvention::Posix,
         "/workspace/src",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file:/workspace/src",
         PathConvention::Posix,
         "/workspace/src",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file://localhost/workspace/src",
         PathConvention::Posix,
         "/workspace/src",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file://LOCALHOST/workspace/src",
         PathConvention::Posix,
         "/workspace/src",
     ),
     // Windows drive paths.
-    RenderCase::renders(
+    RenderCase::round_trips(
         "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(
+    RenderCase::round_trips("file:///C:/", PathConvention::Windows, "C:\\"),
+    RenderCase::renders_lossily("file:///C:", PathConvention::Windows, "C:\\"),
+    RenderCase::round_trips("file:///C:/Users", PathConvention::Windows, r"C:\Users"),
+    RenderCase::round_trips("file:///C:/Windows", PathConvention::Windows, r"C:\Windows"),
+    RenderCase::round_trips(
         "file:///d:/snowman/%E2%98%83",
         PathConvention::Windows,
         r"d:\snowman\☃",
     ),
-    RenderCase::renders("file:///C:/tmp/", PathConvention::Windows, "C:\\tmp\\"),
-    RenderCase::renders(
+    RenderCase::round_trips("file:///C:/tmp/", PathConvention::Windows, "C:\\tmp\\"),
+    RenderCase::round_trips(
         "file:///C:/test%20with%20%25/path",
         PathConvention::Windows,
         r"C:\test with %\path",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file:///C:/test%20with%20%2525/c%23code",
         PathConvention::Windows,
         r"C:\test with %25\c#code",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "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",
+    RenderCase::round_trips(
+        "file:///C:/project/owner's_file/database.sqlite",
         PathConvention::Windows,
-        r"C:\Users\Abd-al-Haseeb's_Dell\Studio\w3mage\wp-content\database.ht.sqlite",
+        r"C:\project\owner's_file\database.sqlite",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file:///C:/project/%25A0.txt",
         PathConvention::Windows,
         r"C:\project\%A0.txt",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file:///C:/project/%252e.txt",
         PathConvention::Windows,
         r"C:\project\%2e.txt",
     ),
     // Windows UNC paths.
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file://server/share/src/main.rs",
         PathConvention::Windows,
         r"\\server\share\src\main.rs",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file://server/share",
         PathConvention::Windows,
         r"\\server\share",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file://server/share/",
         PathConvention::Windows,
         "\\\\server\\share\\",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file://shares/files/c%23/p.cs",
         PathConvention::Windows,
         r"\\shares\files\c#\p.cs",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file://monacotools1/certificates/SSL/",
         PathConvention::Windows,
         "\\\\monacotools1\\certificates\\SSL\\",
     ),
     // Opaque fallbacks rendered according to their source convention.
-    RenderCase::renders(
+    RenderCase::renders_lossily(
         "file:///%00/bad/path/L3RtcC9udWxsLQAt_y1ieXRl",
         PathConvention::Posix,
         "/tmp/null-\0-�-byte",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file:///%00/bad/path/XABcAC4AXABDAE8ATQAxAFwA",
         PathConvention::Windows,
         r"\\.\COM1\",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "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(
+    RenderCase::round_trips("file:///C:/a%3Fb", PathConvention::Windows, "C:\\a?b"),
+    RenderCase::round_trips("file:///C:/a*b", PathConvention::Windows, "C:\\a*b"),
+    RenderCase::round_trips(
         "file:///C:/trailing.",
         PathConvention::Windows,
         "C:\\trailing.",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file:///C:/trailing%20",
         PathConvention::Windows,
         "C:\\trailing ",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file:///C:/control-%01",
         PathConvention::Windows,
         "C:\\control-\u{1}",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file:///C:/file.txt:stream",
         PathConvention::Windows,
         "C:\\file.txt:stream",
     ),
-    RenderCase::renders(
+    RenderCase::round_trips(
         "file://server/sh%3Fare/file.rs",
         PathConvention::Windows,
         "\\\\server\\sh?are\\file.rs",
     ),
+    // These renderings intentionally lose URI byte or segment boundaries.
+    RenderCase::renders_lossily(
+        "file:///tmp/non-utf8-%FF",
+        PathConvention::Posix,
+        "/tmp/non-utf8-�",
+    ),
+    RenderCase::renders_lossily(
+        "file:///tmp/non-utf8-%A0",
+        PathConvention::Posix,
+        "/tmp/non-utf8-�",
+    ),
+    RenderCase::renders_lossily("file:///tmp/a%2Fb", PathConvention::Posix, "/tmp/a/b"),
+    RenderCase::renders_lossily("file:///C:/a%2Fb", PathConvention::Windows, "C:\\a/b"),
+    RenderCase::renders_lossily("file:///C:/a%5Cb", PathConvention::Windows, "C:\\a\\b"),
     // URI shapes that do not match the requested convention.
     RenderCase::rejects(
         "file://server/share/file.txt",
@@ -267,20 +294,6 @@ const RENDER_CASES: &[RenderCase] = &[
         PathConvention::Windows,
         ExpectedError::OpaqueFallback,
     ),
-    // Non-UTF-8 bytes and encoded separators are rendered lossily as path text.
-    RenderCase::renders(
-        "file:///tmp/non-utf8-%FF",
-        PathConvention::Posix,
-        "/tmp/non-utf8-�",
-    ),
-    RenderCase::renders(
-        "file:///tmp/non-utf8-%A0",
-        PathConvention::Posix,
-        "/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]
@@ -288,7 +301,8 @@ 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(ApiPathString(rendered.to_string())),
+            RenderExpectation::RoundTrip(rendered) => Ok(ApiPathString(rendered.to_string())),
+            RenderExpectation::RenderOnly(rendered) => Ok(ApiPathString(rendered.to_string())),
             RenderExpectation::Error(ExpectedError::OpaqueFallback) => {
                 Err(ApiPathStringError::OpaqueFallback {
                     path: path.to_string(),
@@ -307,6 +321,38 @@ fn renders_native_paths_from_shared_cases() {
             expected,
             "rendering {case:?}"
         );
+
+        if let RenderExpectation::RoundTrip(rendered) = case.expected {
+            let api_path = ApiPathString(rendered.to_string());
+            let reparsed = api_path
+                .to_path_uri(case.convention)
+                .expect("native path should parse using its convention");
+            assert_eq!(reparsed, path, "parsing {case:?}");
+            assert_eq!(
+                ApiPathString::from_path_uri(&reparsed, case.convention),
+                Ok(api_path),
+                "round-tripping {case:?}"
+            );
+        }
+    }
+}
+
+#[test]
+fn rejects_relative_api_paths() {
+    for (raw_path, convention) in [
+        ("workspace/file.rs", PathConvention::Posix),
+        (r"workspace\file.rs", PathConvention::Windows),
+        (r"C:file.rs", PathConvention::Windows),
+    ] {
+        let path = ApiPathString(raw_path.to_string());
+
+        assert_eq!(
+            path.to_path_uri(convention),
+            Err(ApiPathStringError::InvalidNativePath {
+                path: raw_path.to_string(),
+                convention,
+            })
+        );
     }
 }
 
@@ -359,13 +405,16 @@ fn renders_native_non_unicode_windows_fallback_lossily() {
 }
 
 #[test]
-fn serializes_as_a_string() {
+fn serializes_and_deserializes_as_a_string() {
     let path = PathUri::parse("file:///workspace/src/lib.rs").expect("valid file URI");
     let rendered = ApiPathString::from_path_uri(&path, PathConvention::Posix)
         .expect("POSIX URI should render");
 
+    let json = serde_json::to_string(&rendered).expect("rendered path should serialize");
+    assert_eq!(json, r#""/workspace/src/lib.rs""#);
     assert_eq!(
-        serde_json::to_string(&rendered).expect("rendered path should serialize"),
-        r#""/workspace/src/lib.rs""#
+        serde_json::from_str::<ApiPathString>(&json)
+            .expect("rendered path should deserialize from a string"),
+        rendered
     );
 }

codex-rs/utils/path-uri/src/lib.rs

@@ -82,22 +82,24 @@ impl PathUri {
         }
 
         #[cfg(unix)]
-        let encoded_path = {
+        let path_bytes = {
             use std::os::unix::ffi::OsStrExt;
-            base64::engine::general_purpose::URL_SAFE_NO_PAD
-                .encode(path.as_path().as_os_str().as_bytes())
+            path.as_path().as_os_str().as_bytes().to_vec()
         };
         #[cfg(windows)]
-        let encoded_path = {
+        let path_bytes = {
             use std::os::windows::ffi::OsStrExt;
-            let path_bytes = path
-                .as_path()
+            path.as_path()
                 .as_os_str()
                 .encode_wide()
                 .flat_map(u16::to_le_bytes)
-                .collect::<Vec<_>>();
-            base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(path_bytes)
+                .collect::<Vec<_>>()
         };
+        Self::from_opaque_path_bytes(&path_bytes)
+    }
+
+    fn from_opaque_path_bytes(path_bytes: &[u8]) -> Self {
+        let encoded_path = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(path_bytes);
         let Ok(uri) = Self::parse(&format!("{BAD_PATH_URI_PREFIX}{encoded_path}")) else {
             unreachable!("URL-safe base64 always produces a valid fallback path URI");
         };