From 6647c365a56226bcd6a3b903f2d75e555a49c58d Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Sat, 13 Jun 2026 03:44:22 +0000 Subject: [PATCH] codex: address PR review feedback (#27819) --- codex-rs/exec-server/src/protocol.rs | 8 ++-- ...tive_path_string.rs => api_path_string.rs} | 40 +++++++++++------- ...ring_tests.rs => api_path_string_tests.rs} | 42 +++++++++++++++---- codex-rs/utils/path-uri/src/lib.rs | 8 ++-- 4 files changed, 66 insertions(+), 32 deletions(-) rename codex-rs/utils/path-uri/src/{native_path_string.rs => api_path_string.rs} (89%) rename codex-rs/utils/path-uri/src/{native_path_string_tests.rs => api_path_string_tests.rs} (88%) diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index bef01344b6..2c58f1cdcf 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/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::from_path_uri(path, self.path_convention) + pub fn render_path(&self, path: &PathUri) -> Result { + ApiPathString::from_path_uri(path, self.path_convention) } } diff --git a/codex-rs/utils/path-uri/src/native_path_string.rs b/codex-rs/utils/path-uri/src/api_path_string.rs similarity index 89% rename from codex-rs/utils/path-uri/src/native_path_string.rs rename to codex-rs/utils/path-uri/src/api_path_string.rs index 66ccf4f8ad..5de3dbed3a 100644 --- a/codex-rs/utils/path-uri/src/native_path_string.rs +++ b/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::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 { + ) -> Result { 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 { +) -> Result { 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(&self, serializer: S) -> Result 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 { +fn render_posix_path(path: &PathUri) -> Result { 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 { Ok(rendered) } -fn render_windows_path(path: &PathUri) -> Result { +fn render_windows_path(path: &PathUri) -> Result { 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; diff --git a/codex-rs/utils/path-uri/src/native_path_string_tests.rs b/codex-rs/utils/path-uri/src/api_path_string_tests.rs similarity index 88% rename from codex-rs/utils/path-uri/src/native_path_string_tests.rs rename to codex-rs/utils/path-uri/src/api_path_string_tests.rs index 559dae6551..111a958515 100644 --- a/codex-rs/utils/path-uri/src/native_path_string_tests.rs +++ b/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!( diff --git a/codex-rs/utils/path-uri/src/lib.rs b/codex-rs/utils/path-uri/src/lib.rs index d726b2100a..3fc85782db 100644 --- a/codex-rs/utils/path-uri/src/lib.rs +++ b/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/";