From f3b157a9c8217abd04302484780ecb7d5e1a712b Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Mon, 22 Jun 2026 22:26:30 +0000 Subject: [PATCH] codex: address PR review feedback (#29472) --- .../src/protocol/item_builders.rs | 10 ++-------- .../src/protocol/item_builders_tests.rs | 5 +---- codex-rs/rollout-trace/src/protocol_event.rs | 10 ++-------- codex-rs/utils/path-uri/src/api_path_string.rs | 12 ++++++++++++ codex-rs/utils/path-uri/src/api_path_string_tests.rs | 12 ++++++++---- 5 files changed, 25 insertions(+), 24 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/item_builders.rs b/codex-rs/app-server-protocol/src/protocol/item_builders.rs index dcf18b5626..842aefb50f 100644 --- a/codex-rs/app-server-protocol/src/protocol/item_builders.rs +++ b/codex-rs/app-server-protocol/src/protocol/item_builders.rs @@ -94,7 +94,7 @@ pub fn build_command_execution_begin_item(payload: &ExecCommandBeginEvent) -> Th ThreadItem::CommandExecution { id: payload.call_id.clone(), command: shlex_join(&payload.command), - cwd: normalized_app_server_cwd(&payload.cwd), + cwd: payload.cwd.normalize_file_uri(), process_id: payload.process_id.clone(), source: payload.source.into(), status: CommandExecutionStatus::InProgress, @@ -117,7 +117,7 @@ pub fn build_command_execution_end_item(payload: &ExecCommandEndEvent) -> Thread ThreadItem::CommandExecution { id: payload.call_id.clone(), command: shlex_join(&payload.command), - cwd: normalized_app_server_cwd(&payload.cwd), + cwd: payload.cwd.normalize_file_uri(), process_id: payload.process_id.clone(), source: payload.source.into(), status: (&payload.status).into(), @@ -128,12 +128,6 @@ pub fn build_command_execution_end_item(payload: &ExecCommandEndEvent) -> Thread } } -fn normalized_app_server_cwd(cwd: &LegacyAppPathString) -> LegacyAppPathString { - cwd.to_file_uri() - .map(LegacyAppPathString::from) - .unwrap_or_else(|| cwd.clone()) -} - fn command_actions_for_legacy_cwd( parsed_cmd: &[ParsedCommand], cwd: &LegacyAppPathString, diff --git a/codex-rs/app-server-protocol/src/protocol/item_builders_tests.rs b/codex-rs/app-server-protocol/src/protocol/item_builders_tests.rs index 3faaef066f..a2556ba15e 100644 --- a/codex-rs/app-server-protocol/src/protocol/item_builders_tests.rs +++ b/codex-rs/app-server-protocol/src/protocol/item_builders_tests.rs @@ -60,10 +60,7 @@ fn raw_file_uri_cwd_is_converted_for_command_actions() { path: PathBuf::from("file.txt"), }]; - assert_eq!( - normalized_app_server_cwd(&cwd), - LegacyAppPathString::from(cwd_uri), - ); + assert_eq!(cwd.normalize_file_uri(), LegacyAppPathString::from(cwd_uri)); assert_eq!( command_actions_for_legacy_cwd(&parsed_cmd, &cwd), vec![CommandAction::Read { diff --git a/codex-rs/rollout-trace/src/protocol_event.rs b/codex-rs/rollout-trace/src/protocol_event.rs index cd1474e788..84563cfbf7 100644 --- a/codex-rs/rollout-trace/src/protocol_event.rs +++ b/codex-rs/rollout-trace/src/protocol_event.rs @@ -182,10 +182,7 @@ impl<'a> From<&'a ExecCommandBeginEvent> for ExecCommandBeginTracePayload<'a> { turn_id, started_at_ms: *started_at_ms, command, - cwd: cwd - .to_file_uri() - .map(|cwd| cwd.inferred_native_path_string()) - .unwrap_or_else(|| cwd.as_str().to_string()), + cwd: cwd.normalize_file_uri().into_string(), parsed_cmd, source: *source, interaction_input: interaction_input.as_deref(), @@ -244,10 +241,7 @@ impl<'a> From<&'a ExecCommandEndEvent> for ExecCommandEndTracePayload<'a> { turn_id, completed_at_ms: *completed_at_ms, command, - cwd: cwd - .to_file_uri() - .map(|cwd| cwd.inferred_native_path_string()) - .unwrap_or_else(|| cwd.as_str().to_string()), + cwd: cwd.normalize_file_uri().into_string(), parsed_cmd, source: *source, interaction_input: interaction_input.as_deref(), diff --git a/codex-rs/utils/path-uri/src/api_path_string.rs b/codex-rs/utils/path-uri/src/api_path_string.rs index 209c505a79..3826554c38 100644 --- a/codex-rs/utils/path-uri/src/api_path_string.rs +++ b/codex-rs/utils/path-uri/src/api_path_string.rs @@ -83,6 +83,9 @@ impl LegacyAppPathString { /// Parses this API string as a serialized file URI. pub fn to_file_uri(&self) -> Option { + // Check the original spelling before parsing because URL parsing + // canonicalizes relative aliases such as `file:repo`, losing the + // distinction from serialized absolute file URI spellings. let scheme = self.0.get(..7)?; if !scheme.eq_ignore_ascii_case("file://") { return None; @@ -90,6 +93,15 @@ impl LegacyAppPathString { PathUri::parse(&self.0).ok() } + /// Converts a serialized file URI spelling to its inferred native path spelling. + /// + /// Non-URI path strings are returned unchanged. + pub fn normalize_file_uri(&self) -> Self { + self.to_file_uri() + .map(Self::from) + .unwrap_or_else(|| self.clone()) + } + /// Parses this API string as a file URI or as an absolute path using its inferred convention. pub fn to_inferred_path_uri(&self) -> Option { self.to_file_uri() diff --git a/codex-rs/utils/path-uri/src/api_path_string_tests.rs b/codex-rs/utils/path-uri/src/api_path_string_tests.rs index 54009e8abd..52766544ee 100644 --- a/codex-rs/utils/path-uri/src/api_path_string_tests.rs +++ b/codex-rs/utils/path-uri/src/api_path_string_tests.rs @@ -467,13 +467,16 @@ fn converts_absolute_api_paths_using_the_inferred_convention() { #[test] fn converts_raw_file_uris_to_inferred_path_uris() { - for raw_uri in [ - "file:///workspace/file.rs", - "file:///C:/workspace/file.rs", - "file://server/share/file.rs", + for (raw_uri, expected_path) in [ + ("file:///workspace/file.rs", "/workspace/file.rs"), + ("file:///C:/workspace/file.rs", r"C:\workspace\file.rs"), + ("file://server/share/file.rs", r"\\server\share\file.rs"), ] { let path = serde_json::from_value::(serde_json::json!(raw_uri)) .expect("raw file URI should deserialize as API text"); + let expected_path = + serde_json::from_value::(serde_json::json!(expected_path)) + .expect("expected native path should deserialize as API text"); assert_eq!( path.to_inferred_path_uri(), @@ -481,6 +484,7 @@ fn converts_raw_file_uris_to_inferred_path_uris() { "round-tripping {raw_uri:?}", ); assert_eq!(path.to_file_uri(), path.to_inferred_path_uri()); + assert_eq!(path.normalize_file_uri(), expected_path); } }