From 9e5c5648299cb66fd68fa237b56b98c99e0dbc36 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 16:18:03 -0700 Subject: [PATCH] git-utils: serialize Git error paths safely --- codex-rs/core/src/turn_metadata_tests.rs | 58 ++++++++++++++++++++++++ codex-rs/git-utils/src/errors.rs | 33 +++++++++++++- 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/codex-rs/core/src/turn_metadata_tests.rs b/codex-rs/core/src/turn_metadata_tests.rs index be49f1b72c..39cf2bb8c6 100644 --- a/codex-rs/core/src/turn_metadata_tests.rs +++ b/codex-rs/core/src/turn_metadata_tests.rs @@ -69,6 +69,64 @@ fn workspace_metadata_serializes_structured_has_changes_refusal() { ); } +#[cfg(any(unix, windows))] +#[test] +fn turn_metadata_keeps_workspace_failure_with_non_utf8_repository_path() { + use std::ffi::OsString; + #[cfg(unix)] + use std::os::unix::ffi::OsStringExt; + #[cfg(windows)] + use std::os::windows::ffi::OsStringExt; + + let temp_dir = TempDir::new().expect("temp dir"); + #[cfg(unix)] + let path = std::path::PathBuf::from(OsString::from_vec(b"/repo/\xff".to_vec())); + #[cfg(windows)] + let path = std::path::PathBuf::from(OsString::from_wide(&[ + b'C' as u16, + b':' as u16, + b'\\' as u16, + 0xd800, + ])); + let lossy = path.to_string_lossy().into_owned(); + let (has_changes, has_changes_unavailable_reason) = + split_has_changes(Err(codex_git_utils::GitReadError::NotRepository { path })); + let workspace = WorkspaceGitMetadata { + associated_remote_urls: None, + latest_git_commit_hash: None, + has_changes, + has_changes_unavailable_reason, + } + .into(); + let state = TurnMetadataState::new( + "session-a".to_string(), + "thread-a".to_string(), + /*forked_from_thread_id*/ None, + /*parent_thread_id*/ None, + &SessionSource::Exec, + /*thread_source*/ None, + "turn-a".to_string(), + temp_dir.path().abs(), + &PermissionProfile::read_only(), + WindowsSandboxLevel::Disabled, + /*enforce_managed_network*/ false, + ); + *state.enriched_workspaces.write().expect("workspace lock") = + Some(BTreeMap::from([("/repo".to_string(), workspace)])); + + let metadata: Value = serde_json::from_str(&test_turn_metadata_header(&state)) + .expect("full turn metadata remains serializable"); + assert_eq!( + metadata["workspaces"]["/repo"], + serde_json::json!({ + "has_changes_unavailable_reason": { + "reason": "notRepository", + "path": lossy, + } + }) + ); +} + fn test_responses_metadata_json( state: &TurnMetadataState, window_id: &str, diff --git a/codex-rs/git-utils/src/errors.rs b/codex-rs/git-utils/src/errors.rs index d22c57de25..4494c7e897 100644 --- a/codex-rs/git-utils/src/errors.rs +++ b/codex-rs/git-utils/src/errors.rs @@ -49,10 +49,15 @@ pub enum GitReadError { #[error("no trusted Git executable is available")] NoTrustedGit, #[error("{path:?} is not a Git repository")] - NotRepository { path: PathBuf }, + NotRepository { + #[serde(with = "lossy_path")] + path: PathBuf, + }, #[error("Git resolved {reported_root:?} instead of expected root {expected_root:?}")] RepositoryRootMismatch { + #[serde(with = "lossy_path")] expected_root: PathBuf, + #[serde(with = "lossy_path")] reported_root: PathBuf, }, #[error("Git operation {operation:?} timed out")] @@ -67,3 +72,29 @@ pub enum GitReadError { #[error("executable filter {driver:?} is selected for {path:?}")] SelectedExecutableFilter { driver: String, path: String }, } + +/// Path fields in caller-visible diagnostic metadata use a deliberately lossy +/// UTF-8 wire representation. The Rust API remains `PathBuf`; deserialization +/// reconstructs the serialized path, not any replaced platform-native bytes. +mod lossy_path { + use std::path::Path; + use std::path::PathBuf; + + use serde::Deserialize; + use serde::Deserializer; + use serde::Serializer; + + pub(super) fn serialize(path: &Path, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(&path.to_string_lossy()) + } + + pub(super) fn deserialize<'de, D>(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + String::deserialize(deserializer).map(PathBuf::from) + } +}