From 19ed2d0e89bcb86261e7a3fa0b2ef7108854168a Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Sat, 13 Jun 2026 00:51:35 +0000 Subject: [PATCH] codex: address PR review feedback (#27819) --- codex-rs/exec-server/src/protocol.rs | 4 +- .../utils/path-uri/src/native_path_string.rs | 27 +++++--- .../path-uri/src/native_path_string_tests.rs | 67 ++++++++++++++++--- 3 files changed, 79 insertions(+), 19 deletions(-) diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 17eea200b7..bef01344b6 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -79,8 +79,8 @@ pub struct EnvironmentInfo { impl EnvironmentInfo { /// Renders a path using this environment's native path syntax. /// - /// TODO(anp): Once `PathUri` carries an environment identifier, resolve the - /// environment from that identifier and verify it matches this metadata. + /// 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) } 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 39144e331f..97195ba62d 100644 --- a/codex-rs/utils/path-uri/src/native_path_string.rs +++ b/codex-rs/utils/path-uri/src/native_path_string.rs @@ -50,24 +50,21 @@ impl fmt::Display for PathConvention { /// "Native" refers to the supplied [`PathConvention`], which may be foreign to /// the operating system running this process. The inner string is private so /// path-producing code must render through [`Self::from_path_uri`] rather than -/// accidentally applying the current host's path rules. +/// accidentally applying the current host's path rules. Opaque fallback paths +/// recoverable on the current host are 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)] #[ts(type = "string")] pub struct NativePathString(String); impl NativePathString { /// Renders a path URI using the requested native path convention. - /// - /// TODO(anp): Once `PathUri` carries an environment identifier, resolve the path - /// convention from that identifier instead of requiring it explicitly. pub fn from_path_uri( path: &PathUri, convention: PathConvention, ) -> Result { if path.is_opaque_fallback() { - return Err(NativePathStringError::OpaqueFallback { - path: path.to_string(), - }); + return render_opaque_fallback(path, convention).map(Self); } let value = match convention { PathConvention::Posix => render_posix_path(path)?, @@ -85,6 +82,20 @@ impl NativePathString { } } +fn render_opaque_fallback( + path: &PathUri, + convention: PathConvention, +) -> Result { + if convention != PathConvention::native() { + return Err(incompatible_convention(path, convention)); + } + path.to_abs_path() + .map(|path| path.as_path().to_string_lossy().into_owned()) + .map_err(|_| NativePathStringError::OpaqueFallback { + path: path.to_string(), + }) +} + impl fmt::Display for NativePathString { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.write_str(&self.0) @@ -222,7 +233,7 @@ fn incompatible_convention(path: &PathUri, convention: PathConvention) -> Native #[derive(Debug, Error, PartialEq, Eq)] pub enum NativePathStringError { - #[error("opaque fallback path URI `{path}` cannot be rendered as a UTF-8 native path")] + #[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")] IncompatibleConvention { 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 e515749245..2e3ca90688 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 @@ -100,17 +100,66 @@ fn rejects_paths_incompatible_with_the_convention() { } #[test] -fn rejects_opaque_fallback_paths() { +fn rejects_opaque_fallback_paths_that_cannot_be_recovered() { let path = PathUri::parse("file:///%00/bad/path/YQ").expect("canonical opaque fallback URI"); - for convention in [PathConvention::Posix, PathConvention::Windows] { - assert_eq!( - NativePathString::from_path_uri(&path, convention), - Err(NativePathStringError::OpaqueFallback { - path: path.to_string(), - }) - ); - } + assert_eq!( + NativePathString::from_path_uri(&path, PathConvention::native()), + Err(NativePathStringError::OpaqueFallback { + path: path.to_string(), + }) + ); +} + +#[cfg(unix)] +#[test] +fn renders_native_opaque_fallback_paths_lossily() { + use std::os::unix::ffi::OsStringExt; + + let native_path = std::path::PathBuf::from(std::ffi::OsString::from_vec( + b"/tmp/null-\0-non-utf8-\xff".to_vec(), + )); + let path = PathUri::from_path(native_path).expect("absolute native path"); + + assert_eq!( + NativePathString::from_path_uri(&path, PathConvention::Posix) + .map(NativePathString::into_string), + Ok("/tmp/null-\0-non-utf8-�".to_string()) + ); + assert_eq!( + NativePathString::from_path_uri(&path, PathConvention::Windows), + Err(NativePathStringError::IncompatibleConvention { + path: path.to_string(), + convention: PathConvention::Windows, + }) + ); +} + +#[cfg(windows)] +#[test] +fn renders_native_opaque_fallback_paths_lossily() { + use std::os::windows::ffi::OsStringExt; + + let native_path = std::path::PathBuf::from(std::ffi::OsString::from_wide( + &r"C:\bad\" + .encode_utf16() + .chain([0xd800]) + .collect::>(), + )); + 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()) + ); + assert_eq!( + NativePathString::from_path_uri(&path, PathConvention::Posix), + Err(NativePathStringError::IncompatibleConvention { + path: path.to_string(), + convention: PathConvention::Posix, + }) + ); } #[test]