codex: address PR review feedback (#27819)

codex-rs/exec-server/src/protocol.rs

@@ -4,8 +4,8 @@ use std::path::PathBuf;
 use crate::FileSystemSandboxContext;
 use base64::engine::general_purpose::STANDARD as BASE64_STANDARD;
 use codex_protocol::config_types::ShellEnvironmentPolicyInherit;
-use codex_utils_path_uri::NativePathString;
-use codex_utils_path_uri::NativePathStringError;
+use codex_utils_path_uri::ApiPathString;
+use codex_utils_path_uri::ApiPathStringError;
 use codex_utils_path_uri::PathConvention;
 use codex_utils_path_uri::PathUri;
 use serde::Deserialize;
@@ -81,8 +81,8 @@ impl EnvironmentInfo {
     ///
     /// TODO(anp): Once `PathUri` carries an environment identifier, invert this
     /// relationship and use the URI to resolve the environment information.
-    pub fn render_path(&self, path: &PathUri) -> Result<NativePathString, NativePathStringError> {
-        NativePathString::from_path_uri(path, self.path_convention)
+    pub fn render_path(&self, path: &PathUri) -> Result<ApiPathString, ApiPathStringError> {
+        ApiPathString::from_path_uri(path, self.path_convention)
     }
 }
 

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

@@ -1,4 +1,5 @@
 use crate::PathUri;
+use codex_utils_absolute_path::AbsolutePathBuf;
 use schemars::JsonSchema;
 use serde::Deserialize;
 use serde::Serialize;
@@ -52,9 +53,18 @@ impl fmt::Display for PathConvention {
 /// this API boundary because the value is serialized as a JSON string.
 #[derive(Clone, Debug, PartialEq, Eq, Hash, TS)]
 #[ts(type = "string")]
-pub struct NativePathString(String);
+pub struct ApiPathString(String);
+
+impl ApiPathString {
+    /// Converts an absolute path to a [`PathUri`] before rendering it using the
+    /// requested native path convention.
+    pub fn from_abs_path(
+        path: &AbsolutePathBuf,
+        convention: PathConvention,
+    ) -> Result<Self, ApiPathStringError> {
+        Self::from_path_uri(&PathUri::from_abs_path(path), convention)
+    }
 
-impl NativePathString {
     /// Renders a path URI using the requested native path convention.
     ///
     /// Rendering fails when the URI shape does not match the convention, such
@@ -65,7 +75,7 @@ impl NativePathString {
     pub fn from_path_uri(
         path: &PathUri,
         convention: PathConvention,
-    ) -> Result<Self, NativePathStringError> {
+    ) -> Result<Self, ApiPathStringError> {
         if let Some(path_bytes) = path.opaque_fallback_bytes() {
             return render_opaque_fallback(path, &path_bytes, convention).map(Self);
         }
@@ -89,7 +99,7 @@ fn render_opaque_fallback(
     path: &PathUri,
     path_bytes: &[u8],
     convention: PathConvention,
-) -> Result<String, NativePathStringError> {
+) -> Result<String, ApiPathStringError> {
     let rendered = match convention {
         PathConvention::Posix if path_bytes.starts_with(b"/") => {
             Some(String::from_utf8_lossy(path_bytes).into_owned())
@@ -97,7 +107,7 @@ fn render_opaque_fallback(
         PathConvention::Windows => render_windows_opaque_fallback(path_bytes),
         PathConvention::Posix => None,
     };
-    rendered.ok_or_else(|| NativePathStringError::OpaqueFallback {
+    rendered.ok_or_else(|| ApiPathStringError::OpaqueFallback {
         path: path.to_string(),
     })
 }
@@ -133,13 +143,13 @@ fn is_windows_separator(character: u16) -> bool {
     character == u16::from(b'\\') || character == u16::from(b'/')
 }
 
-impl fmt::Display for NativePathString {
+impl fmt::Display for ApiPathString {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         f.write_str(&self.0)
     }
 }
 
-impl Serialize for NativePathString {
+impl Serialize for ApiPathString {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
     where
         S: Serializer,
@@ -148,9 +158,9 @@ impl Serialize for NativePathString {
     }
 }
 
-impl JsonSchema for NativePathString {
+impl JsonSchema for ApiPathString {
     fn schema_name() -> String {
-        "NativePathString".to_string()
+        "ApiPathString".to_string()
     }
 
     fn json_schema(generator: &mut schemars::r#gen::SchemaGenerator) -> schemars::schema::Schema {
@@ -158,7 +168,7 @@ impl JsonSchema for NativePathString {
     }
 }
 
-fn render_posix_path(path: &PathUri) -> Result<String, NativePathStringError> {
+fn render_posix_path(path: &PathUri) -> Result<String, ApiPathStringError> {
     let url = path.to_url();
     // POSIX file paths do not have a UNC authority, so `file://server/share`
     // cannot be represented as `/share` without losing the server identity.
@@ -176,7 +186,7 @@ fn render_posix_path(path: &PathUri) -> Result<String, NativePathStringError> {
     Ok(rendered)
 }
 
-fn render_windows_path(path: &PathUri) -> Result<String, NativePathStringError> {
+fn render_windows_path(path: &PathUri) -> Result<String, ApiPathStringError> {
     let url = path.to_url();
     let mut segments = path_segments(&url);
     let mut rendered = String::new();
@@ -237,15 +247,15 @@ fn decode_native_segment(segment: &str) -> String {
     String::from_utf8_lossy(&bytes).into_owned()
 }
 
-fn incompatible_convention(path: &PathUri, convention: PathConvention) -> NativePathStringError {
-    NativePathStringError::IncompatibleConvention {
+fn incompatible_convention(path: &PathUri, convention: PathConvention) -> ApiPathStringError {
+    ApiPathStringError::IncompatibleConvention {
         path: path.to_string(),
         convention,
     }
 }
 
 #[derive(Debug, Error, PartialEq, Eq)]
-pub enum NativePathStringError {
+pub enum ApiPathStringError {
     #[error("opaque fallback path URI `{path}` cannot be recovered as a native path")]
     OpaqueFallback { path: String },
     #[error("path URI `{path}` cannot be rendered using {convention} path syntax")]
@@ -256,5 +266,5 @@ pub enum NativePathStringError {
 }
 
 #[cfg(test)]
-#[path = "native_path_string_tests.rs"]
+#[path = "api_path_string_tests.rs"]
 mod tests;

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

@@ -1,5 +1,6 @@
 use super::*;
 use crate::PathUri;
+use codex_utils_absolute_path::AbsolutePathBuf;
 use pretty_assertions::assert_eq;
 
 #[derive(Clone, Copy, Debug)]
@@ -287,14 +288,14 @@ 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::Rendered(rendered) => Ok(ApiPathString(rendered.to_string())),
             RenderExpectation::Error(ExpectedError::OpaqueFallback) => {
-                Err(NativePathStringError::OpaqueFallback {
+                Err(ApiPathStringError::OpaqueFallback {
                     path: path.to_string(),
                 })
             }
             RenderExpectation::Error(ExpectedError::IncompatibleConvention) => {
-                Err(NativePathStringError::IncompatibleConvention {
+                Err(ApiPathStringError::IncompatibleConvention {
                     path: path.to_string(),
                     convention: case.convention,
                 })
@@ -302,13 +303,36 @@ fn renders_native_paths_from_shared_cases() {
         };
 
         assert_eq!(
-            NativePathString::from_path_uri(&path, case.convention),
+            ApiPathString::from_path_uri(&path, case.convention),
             expected,
             "rendering {case:?}"
         );
     }
 }
 
+#[test]
+fn renders_an_absolute_path_using_the_explicit_convention() {
+    #[cfg(unix)]
+    let (native_path, convention, expected) = (
+        "/workspace/a file.rs",
+        PathConvention::Posix,
+        "/workspace/a file.rs",
+    );
+    #[cfg(windows)]
+    let (native_path, convention, expected) = (
+        r"C:\workspace\a file.rs",
+        PathConvention::Windows,
+        r"C:\workspace\a file.rs",
+    );
+    let path = AbsolutePathBuf::from_absolute_path_checked(native_path)
+        .expect("native path should be absolute");
+
+    assert_eq!(
+        ApiPathString::from_abs_path(&path, convention),
+        Ok(ApiPathString(expected.to_string()))
+    );
+}
+
 #[cfg(windows)]
 #[test]
 fn renders_native_non_unicode_windows_fallback_lossily() {
@@ -323,12 +347,12 @@ fn renders_native_non_unicode_windows_fallback_lossily() {
     let path = PathUri::from_path(native_path).expect("absolute native path");
 
     assert_eq!(
-        NativePathString::from_path_uri(&path, PathConvention::Windows),
-        Ok(NativePathString(r"C:\bad\�".to_string()))
+        ApiPathString::from_path_uri(&path, PathConvention::Windows),
+        Ok(ApiPathString(r"C:\bad\�".to_string()))
     );
     assert_eq!(
-        NativePathString::from_path_uri(&path, PathConvention::Posix),
-        Err(NativePathStringError::OpaqueFallback {
+        ApiPathString::from_path_uri(&path, PathConvention::Posix),
+        Err(ApiPathStringError::OpaqueFallback {
             path: path.to_string(),
         })
     );
@@ -337,7 +361,7 @@ fn renders_native_non_unicode_windows_fallback_lossily() {
 #[test]
 fn serializes_as_a_string() {
     let path = PathUri::parse("file:///workspace/src/lib.rs").expect("valid file URI");
-    let rendered = NativePathString::from_path_uri(&path, PathConvention::Posix)
+    let rendered = ApiPathString::from_path_uri(&path, PathConvention::Posix)
         .expect("POSIX URI should render");
 
     assert_eq!(

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

@@ -17,11 +17,11 @@ use thiserror::Error;
 use ts_rs::TS;
 use url::Url;
 
-mod native_path_string;
+mod api_path_string;
 
-pub use native_path_string::NativePathString;
-pub use native_path_string::NativePathStringError;
-pub use native_path_string::PathConvention;
+pub use api_path_string::ApiPathString;
+pub use api_path_string::ApiPathStringError;
+pub use api_path_string::PathConvention;
 
 pub const FILE_SCHEME: &str = "file";
 const BAD_PATH_URI_PREFIX: &str = "file:///%00/bad/path/";