From 15a21e11f315c88a65b197f34ad84eef9475aa9b Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Thu, 11 Jun 2026 19:14:27 +0000 Subject: [PATCH 1/5] exec-server: migrate filesystem protocol to PathUri --- codex-rs/exec-server/README.md | 6 +- codex-rs/exec-server/src/fs_helper.rs | 58 ++++--------- codex-rs/exec-server/src/protocol.rs | 59 ++++++++++---- .../exec-server/src/remote_file_system.rs | 29 +++---- .../src/remote_file_system_path_uri_tests.rs | 81 ++----------------- .../exec-server/src/sandboxed_file_system.rs | 20 ++--- .../src/server/file_system_handler.rs | 56 ++++++------- 7 files changed, 116 insertions(+), 193 deletions(-) diff --git a/codex-rs/exec-server/README.md b/codex-rs/exec-server/README.md index 8fa1a9eb75ce..208e53bc202b 100644 --- a/codex-rs/exec-server/README.md +++ b/codex-rs/exec-server/README.md @@ -338,13 +338,15 @@ Params: ## Filesystem RPCs -Filesystem methods use absolute paths and return JSON-RPC errors for invalid -or unavailable paths: +Filesystem methods use canonical `file:` URIs and return JSON-RPC errors for +invalid or unavailable paths. For compatibility, requests also accept native +absolute path strings and normalize them to `file:` URIs: - `fs/readFile` - `fs/writeFile` - `fs/createDirectory` - `fs/getMetadata` +- `fs/canonicalize` - `fs/readDirectory` - `fs/remove` - `fs/copy` diff --git a/codex-rs/exec-server/src/fs_helper.rs b/codex-rs/exec-server/src/fs_helper.rs index 526e682976ba..6f3dc92910ae 100644 --- a/codex-rs/exec-server/src/fs_helper.rs +++ b/codex-rs/exec-server/src/fs_helper.rs @@ -189,10 +189,8 @@ pub(crate) async fn run_direct_request( let file_system = DirectFileSystem; match request { FsHelperRequest::ReadFile(params) => { - let path = - codex_utils_path_uri::PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; let data = file_system - .read_file(&path, /*sandbox*/ None) + .read_file(¶ms.path, /*sandbox*/ None) .await .map_err(map_fs_error)?; Ok(FsHelperPayload::ReadFile(FsReadFileResponse { @@ -200,25 +198,21 @@ pub(crate) async fn run_direct_request( })) } FsHelperRequest::WriteFile(params) => { - let path = - codex_utils_path_uri::PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; let bytes = STANDARD.decode(params.data_base64).map_err(|err| { invalid_request(format!( "{FS_WRITE_FILE_METHOD} requires valid base64 dataBase64: {err}" )) })?; file_system - .write_file(&path, bytes, /*sandbox*/ None) + .write_file(¶ms.path, bytes, /*sandbox*/ None) .await .map_err(map_fs_error)?; Ok(FsHelperPayload::WriteFile(FsWriteFileResponse {})) } FsHelperRequest::CreateDirectory(params) => { - let path = - codex_utils_path_uri::PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; file_system .create_directory( - &path, + ¶ms.path, CreateDirectoryOptions { recursive: params.recursive.unwrap_or(true), }, @@ -231,10 +225,8 @@ pub(crate) async fn run_direct_request( )) } FsHelperRequest::GetMetadata(params) => { - let path = - codex_utils_path_uri::PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; let metadata = file_system - .get_metadata(&path, /*sandbox*/ None) + .get_metadata(¶ms.path, /*sandbox*/ None) .await .map_err(map_fs_error)?; Ok(FsHelperPayload::GetMetadata(FsGetMetadataResponse { @@ -246,22 +238,17 @@ pub(crate) async fn run_direct_request( })) } FsHelperRequest::Canonicalize(params) => { - let path = - codex_utils_path_uri::PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; let path = file_system - .canonicalize(&path, /*sandbox*/ None) + .canonicalize(¶ms.path, /*sandbox*/ None) .await .map_err(map_fs_error)?; - let path = path.to_abs_path().map_err(map_fs_error)?; Ok(FsHelperPayload::Canonicalize(FsCanonicalizeResponse { path, })) } FsHelperRequest::ReadDirectory(params) => { - let path = - codex_utils_path_uri::PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; let entries = file_system - .read_directory(&path, /*sandbox*/ None) + .read_directory(¶ms.path, /*sandbox*/ None) .await .map_err(map_fs_error)? .into_iter() @@ -276,11 +263,9 @@ pub(crate) async fn run_direct_request( })) } FsHelperRequest::Remove(params) => { - let path = - codex_utils_path_uri::PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; file_system .remove( - &path, + ¶ms.path, RemoveOptions { recursive: params.recursive.unwrap_or(true), force: params.force.unwrap_or(true), @@ -292,15 +277,10 @@ pub(crate) async fn run_direct_request( Ok(FsHelperPayload::Remove(FsRemoveResponse {})) } FsHelperRequest::Copy(params) => { - let source_path = codex_utils_path_uri::PathUri::from_abs_path(¶ms.source_path) - .map_err(map_fs_error)?; - let destination_path = - codex_utils_path_uri::PathUri::from_abs_path(¶ms.destination_path) - .map_err(map_fs_error)?; file_system .copy( - &source_path, - &destination_path, + ¶ms.source_path, + ¶ms.destination_path, CopyOptions { recursive: params.recursive, }, @@ -325,27 +305,23 @@ fn map_fs_error(err: io::Error) -> JSONRPCErrorError { #[cfg(test)] mod tests { - use codex_utils_absolute_path::AbsolutePathBuf; + use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use serde_json::json; use super::*; #[test] - fn helper_protocol_keeps_native_absolute_paths() -> serde_json::Result<()> { - let local_path = - AbsolutePathBuf::from_absolute_path(std::env::current_dir().expect("cwd").join("file")) - .expect("absolute path"); - #[cfg(not(windows))] - let paths = [local_path]; - #[cfg(windows)] + fn helper_protocol_uses_path_uris() -> serde_json::Result<()> { + let local_path = PathUri::from_path(std::env::current_dir().expect("cwd").join("file")) + .expect("path URI"); let paths = [ local_path, - AbsolutePathBuf::from_absolute_path(r"\\server\share\file").expect("absolute UNC path"), + PathUri::parse("file://server/share/file").expect("path URI"), ]; for path in paths { - let expected_path = path.to_string_lossy().into_owned(); + let expected_path = path.to_string(); let request = serde_json::to_value(FsHelperRequest::WriteFile(FsWriteFileParams { path: path.clone(), @@ -367,7 +343,7 @@ mod tests { .as_str() .expect("request path should be a string"); assert_eq!(request_path, expected_path); - assert!(!request_path.starts_with("file:")); + assert!(request_path.starts_with("file:")); let response = serde_json::to_value(FsHelperResponse::Ok( FsHelperPayload::Canonicalize(FsCanonicalizeResponse { path }), @@ -388,7 +364,7 @@ mod tests { .as_str() .expect("canonicalize response path should be a string"); assert_eq!(response_path, expected_path); - assert!(!response_path.starts_with("file:")); + assert!(response_path.starts_with("file:")); } Ok(()) diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index b85bff3577f4..49b1f762db99 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -4,7 +4,7 @@ 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_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; use serde::Deserialize; use serde::Serialize; @@ -199,7 +199,7 @@ pub struct TerminateResponse { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsReadFileParams { - pub path: AbsolutePathBuf, + pub path: PathUri, pub sandbox: Option, } @@ -212,7 +212,7 @@ pub struct FsReadFileResponse { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsWriteFileParams { - pub path: AbsolutePathBuf, + pub path: PathUri, pub data_base64: String, pub sandbox: Option, } @@ -224,7 +224,7 @@ pub struct FsWriteFileResponse {} #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsCreateDirectoryParams { - pub path: AbsolutePathBuf, + pub path: PathUri, pub recursive: Option, pub sandbox: Option, } @@ -236,7 +236,7 @@ pub struct FsCreateDirectoryResponse {} #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsGetMetadataParams { - pub path: AbsolutePathBuf, + pub path: PathUri, pub sandbox: Option, } @@ -253,45 +253,47 @@ pub struct FsGetMetadataResponse { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsCanonicalizeParams { - pub path: AbsolutePathBuf, + pub path: PathUri, pub sandbox: Option, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsCanonicalizeResponse { - pub path: AbsolutePathBuf, + pub path: PathUri, } +// TODO(anp): remove fs/join from the protocol. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsJoinParams { - pub base_path: AbsolutePathBuf, + pub base_path: PathUri, pub path: PathBuf, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsJoinResponse { - pub path: AbsolutePathBuf, + pub path: PathUri, } +// TODO(anp): remove fs/parent from the protocol. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsParentParams { - pub path: AbsolutePathBuf, + pub path: PathUri, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsParentResponse { - pub path: Option, + pub path: Option, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsReadDirectoryParams { - pub path: AbsolutePathBuf, + pub path: PathUri, pub sandbox: Option, } @@ -312,7 +314,7 @@ pub struct FsReadDirectoryResponse { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsRemoveParams { - pub path: AbsolutePathBuf, + pub path: PathUri, pub recursive: Option, pub force: Option, pub sandbox: Option, @@ -325,8 +327,8 @@ pub struct FsRemoveResponse {} #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsCopyParams { - pub source_path: AbsolutePathBuf, - pub destination_path: AbsolutePathBuf, + pub source_path: PathUri, + pub destination_path: PathUri, pub recursive: bool, pub sandbox: Option, } @@ -475,9 +477,36 @@ mod base64_bytes { #[cfg(test)] mod tests { + use super::FsReadFileParams; use super::HttpRequestParams; + use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; + #[test] + fn filesystem_protocol_accepts_legacy_absolute_paths_and_serializes_path_uris() { + let legacy_path = std::env::current_dir() + .expect("current directory") + .join("legacy-file.txt"); + let params: FsReadFileParams = serde_json::from_value(serde_json::json!({ + "path": legacy_path.to_string_lossy(), + "sandbox": null, + })) + .expect("legacy absolute path should deserialize"); + let expected = FsReadFileParams { + path: PathUri::from_path(legacy_path).expect("path URI"), + sandbox: None, + }; + + assert_eq!(params, expected); + assert_eq!( + serde_json::to_value(params).expect("params should serialize"), + serde_json::json!({ + "path": expected.path.to_string(), + "sandbox": null, + }) + ); + } + #[test] fn http_request_timeout_treats_omitted_and_null_as_no_timeout() { let omitted: HttpRequestParams = serde_json::from_value(serde_json::json!({ diff --git a/codex-rs/exec-server/src/remote_file_system.rs b/codex-rs/exec-server/src/remote_file_system.rs index 9bcdcc5b0965..d47b71cba228 100644 --- a/codex-rs/exec-server/src/remote_file_system.rs +++ b/codex-rs/exec-server/src/remote_file_system.rs @@ -46,16 +46,15 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult { trace!("remote fs canonicalize"); - let path = path.to_abs_path()?; let client = self.client.get().await.map_err(map_remote_error)?; let response = client .fs_canonicalize(FsCanonicalizeParams { - path, + path: path.clone(), sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; - PathUri::from_abs_path(&response.path) + Ok(response.path) } async fn read_file( @@ -64,11 +63,10 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult> { trace!("remote fs read_file"); - let path = path.to_abs_path()?; let client = self.client.get().await.map_err(map_remote_error)?; let response = client .fs_read_file(FsReadFileParams { - path, + path: path.clone(), sandbox: remote_sandbox_context(sandbox), }) .await @@ -88,11 +86,10 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { trace!("remote fs write_file"); - let path = path.to_abs_path()?; let client = self.client.get().await.map_err(map_remote_error)?; client .fs_write_file(FsWriteFileParams { - path, + path: path.clone(), data_base64: STANDARD.encode(contents), sandbox: remote_sandbox_context(sandbox), }) @@ -108,11 +105,10 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { trace!("remote fs create_directory"); - let path = path.to_abs_path()?; let client = self.client.get().await.map_err(map_remote_error)?; client .fs_create_directory(FsCreateDirectoryParams { - path, + path: path.clone(), recursive: Some(options.recursive), sandbox: remote_sandbox_context(sandbox), }) @@ -127,11 +123,10 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult { trace!("remote fs get_metadata"); - let path = path.to_abs_path()?; let client = self.client.get().await.map_err(map_remote_error)?; let response = client .fs_get_metadata(FsGetMetadataParams { - path, + path: path.clone(), sandbox: remote_sandbox_context(sandbox), }) .await @@ -151,11 +146,10 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult> { trace!("remote fs read_directory"); - let path = path.to_abs_path()?; let client = self.client.get().await.map_err(map_remote_error)?; let response = client .fs_read_directory(FsReadDirectoryParams { - path, + path: path.clone(), sandbox: remote_sandbox_context(sandbox), }) .await @@ -178,11 +172,10 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { trace!("remote fs remove"); - let path = path.to_abs_path()?; let client = self.client.get().await.map_err(map_remote_error)?; client .fs_remove(FsRemoveParams { - path, + path: path.clone(), recursive: Some(options.recursive), force: Some(options.force), sandbox: remote_sandbox_context(sandbox), @@ -200,13 +193,11 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { trace!("remote fs copy"); - let source_path = source_path.to_abs_path()?; - let destination_path = destination_path.to_abs_path()?; let client = self.client.get().await.map_err(map_remote_error)?; client .fs_copy(FsCopyParams { - source_path, - destination_path, + source_path: source_path.clone(), + destination_path: destination_path.clone(), recursive: options.recursive, sandbox: remote_sandbox_context(sandbox), }) diff --git a/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs b/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs index 29d0edaffb12..50e46b48542b 100644 --- a/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs +++ b/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs @@ -1,123 +1,59 @@ #![allow(clippy::expect_used)] -#[cfg(windows)] use codex_app_server_protocol::JSONRPCMessage; -#[cfg(windows)] use codex_app_server_protocol::JSONRPCResponse; -#[cfg(windows)] -use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; -#[cfg(windows)] use futures::SinkExt; -#[cfg(windows)] use futures::StreamExt; use pretty_assertions::assert_eq; -use tokio::io; -#[cfg(windows)] use tokio::net::TcpListener; -#[cfg(windows)] use tokio::net::TcpStream; -#[cfg(windows)] use tokio::sync::oneshot; -#[cfg(windows)] use tokio::time::Duration; -#[cfg(windows)] use tokio::time::timeout; -#[cfg(windows)] use tokio_tungstenite::WebSocketStream; -#[cfg(windows)] use tokio_tungstenite::accept_async; -#[cfg(windows)] use tokio_tungstenite::tungstenite::Message; use super::*; use crate::client_api::ExecServerTransportParams; -#[cfg(windows)] use crate::protocol::FS_READ_FILE_METHOD; -#[cfg(windows)] use crate::protocol::FsReadFileParams; -#[cfg(windows)] use crate::protocol::FsReadFileResponse; -#[cfg(windows)] use crate::protocol::INITIALIZE_METHOD; -#[cfg(windows)] use crate::protocol::INITIALIZED_METHOD; -#[cfg(windows)] use crate::protocol::InitializeResponse; #[tokio::test] -async fn non_native_uri_is_rejected_before_connecting() { - let file_system = RemoteFileSystem::new(LazyRemoteExecServerClient::new( - ExecServerTransportParams::websocket_url("not a websocket URL".to_string()), - )); - - let error = file_system - .read_file(&non_native_uri(), /*sandbox*/ None) - .await - .expect_err("non-native URI should be rejected"); - - assert_eq!(error.kind(), io::ErrorKind::InvalidInput); -} - -#[cfg(windows)] -#[tokio::test] -async fn remote_file_system_sends_explicit_windows_native_paths() { +async fn remote_file_system_sends_path_uris_without_native_conversion() { let (websocket_url, captured_paths, server) = record_read_file_paths(2).await; let file_system = RemoteFileSystem::new(LazyRemoteExecServerClient::new( ExecServerTransportParams::websocket_url(websocket_url), )); let paths = vec![ - ( - PathUri::parse("file:///C:/Users/Alice/src/main.rs").expect("valid drive URI"), - absolute_windows_path(r"C:\Users\Alice\src\main.rs"), - ), - ( - PathUri::parse("file://server/share/src/main.rs").expect("valid UNC URI"), - absolute_windows_path(r"\\server\share\src\main.rs"), - ), + PathUri::parse("file:///C:/Users/Alice/src/main.rs").expect("valid drive URI"), + PathUri::parse("file://server/share/src/main.rs").expect("valid UNC URI"), ]; - let expected_paths = paths - .iter() - .map(|(_, expected_path)| expected_path.clone()) - .collect::>(); - for (path, _) in paths { + for path in &paths { assert_eq!( file_system - .read_file(&path, /*sandbox*/ None) + .read_file(path, /*sandbox*/ None) .await .expect("remote read should succeed"), Vec::::new() ); } - assert_eq!( - captured_paths.await.expect("captured paths"), - expected_paths - ); + assert_eq!(captured_paths.await.expect("captured paths"), paths); server.await.expect("recording server should succeed"); } -fn non_native_uri() -> PathUri { - #[cfg(unix)] - let uri = "file://server/share/file.txt"; - #[cfg(windows)] - let uri = "file:///usr/local/file.txt"; - - PathUri::parse(uri).expect("valid non-native URI") -} - -#[cfg(windows)] -fn absolute_windows_path(path: &str) -> AbsolutePathBuf { - AbsolutePathBuf::from_absolute_path_checked(path).expect("absolute Windows path") -} - -#[cfg(windows)] async fn record_read_file_paths( expected_requests: usize, ) -> ( String, - oneshot::Receiver>, + oneshot::Receiver>, tokio::task::JoinHandle<()>, ) { let listener = TcpListener::bind("127.0.0.1:0") @@ -164,7 +100,6 @@ async fn record_read_file_paths( (websocket_url, captured_paths_rx, server) } -#[cfg(windows)] async fn complete_websocket_initialize(websocket: &mut WebSocketStream) { let request = match read_jsonrpc_websocket(websocket).await { JSONRPCMessage::Request(request) if request.method == INITIALIZE_METHOD => request, @@ -189,7 +124,6 @@ async fn complete_websocket_initialize(websocket: &mut WebSocketStream) -> JSONRPCMessage { loop { match timeout(Duration::from_secs(1), websocket.next()) @@ -212,7 +146,6 @@ async fn read_jsonrpc_websocket(websocket: &mut WebSocketStream) -> J } } -#[cfg(windows)] async fn write_jsonrpc_websocket( websocket: &mut WebSocketStream, message: JSONRPCMessage, diff --git a/codex-rs/exec-server/src/sandboxed_file_system.rs b/codex-rs/exec-server/src/sandboxed_file_system.rs index 14f26e60b948..95d52ed7f03c 100644 --- a/codex-rs/exec-server/src/sandboxed_file_system.rs +++ b/codex-rs/exec-server/src/sandboxed_file_system.rs @@ -62,14 +62,14 @@ impl ExecutorFileSystem for SandboxedFileSystem { .run_sandboxed( sandbox, FsHelperRequest::Canonicalize(FsCanonicalizeParams { - path: path.to_abs_path()?, + path: path.clone(), sandbox: None, }), ) .await? .expect_canonicalize() .map_err(map_sandbox_error)?; - PathUri::from_abs_path(&response.path) + Ok(response.path) } async fn read_file( @@ -82,7 +82,7 @@ impl ExecutorFileSystem for SandboxedFileSystem { .run_sandboxed( sandbox, FsHelperRequest::ReadFile(FsReadFileParams { - path: path.to_abs_path()?, + path: path.clone(), sandbox: None, }), ) @@ -107,7 +107,7 @@ impl ExecutorFileSystem for SandboxedFileSystem { self.run_sandboxed( sandbox, FsHelperRequest::WriteFile(FsWriteFileParams { - path: path.to_abs_path()?, + path: path.clone(), data_base64: STANDARD.encode(contents), sandbox: None, }), @@ -128,7 +128,7 @@ impl ExecutorFileSystem for SandboxedFileSystem { self.run_sandboxed( sandbox, FsHelperRequest::CreateDirectory(FsCreateDirectoryParams { - path: path.to_abs_path()?, + path: path.clone(), recursive: Some(options.recursive), sandbox: None, }), @@ -149,7 +149,7 @@ impl ExecutorFileSystem for SandboxedFileSystem { .run_sandboxed( sandbox, FsHelperRequest::GetMetadata(FsGetMetadataParams { - path: path.to_abs_path()?, + path: path.clone(), sandbox: None, }), ) @@ -175,7 +175,7 @@ impl ExecutorFileSystem for SandboxedFileSystem { .run_sandboxed( sandbox, FsHelperRequest::ReadDirectory(FsReadDirectoryParams { - path: path.to_abs_path()?, + path: path.clone(), sandbox: None, }), ) @@ -203,7 +203,7 @@ impl ExecutorFileSystem for SandboxedFileSystem { self.run_sandboxed( sandbox, FsHelperRequest::Remove(FsRemoveParams { - path: path.to_abs_path()?, + path: path.clone(), recursive: Some(remove_options.recursive), force: Some(remove_options.force), sandbox: None, @@ -226,8 +226,8 @@ impl ExecutorFileSystem for SandboxedFileSystem { self.run_sandboxed( sandbox, FsHelperRequest::Copy(FsCopyParams { - source_path: source_path.to_abs_path()?, - destination_path: destination_path.to_abs_path()?, + source_path: source_path.clone(), + destination_path: destination_path.clone(), recursive: options.recursive, sandbox: None, }), diff --git a/codex-rs/exec-server/src/server/file_system_handler.rs b/codex-rs/exec-server/src/server/file_system_handler.rs index 97d028f89b7e..54f60ebb376b 100644 --- a/codex-rs/exec-server/src/server/file_system_handler.rs +++ b/codex-rs/exec-server/src/server/file_system_handler.rs @@ -3,7 +3,6 @@ use std::io; use base64::Engine as _; use base64::engine::general_purpose::STANDARD; use codex_app_server_protocol::JSONRPCErrorError; -use codex_utils_path_uri::PathUri; use crate::CopyOptions; use crate::CreateDirectoryOptions; @@ -53,10 +52,9 @@ impl FileSystemHandler { &self, params: FsReadFileParams, ) -> Result { - let path = PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; let bytes = self .file_system - .read_file(&path, params.sandbox.as_ref()) + .read_file(¶ms.path, params.sandbox.as_ref()) .await .map_err(map_fs_error)?; Ok(FsReadFileResponse { @@ -68,14 +66,13 @@ impl FileSystemHandler { &self, params: FsWriteFileParams, ) -> Result { - let path = PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; let bytes = STANDARD.decode(params.data_base64).map_err(|err| { invalid_request(format!( "{FS_WRITE_FILE_METHOD} requires valid base64 dataBase64: {err}" )) })?; self.file_system - .write_file(&path, bytes, params.sandbox.as_ref()) + .write_file(¶ms.path, bytes, params.sandbox.as_ref()) .await .map_err(map_fs_error)?; Ok(FsWriteFileResponse {}) @@ -86,10 +83,9 @@ impl FileSystemHandler { params: FsCreateDirectoryParams, ) -> Result { let recursive = params.recursive.unwrap_or(true); - let path = PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; self.file_system .create_directory( - &path, + ¶ms.path, CreateDirectoryOptions { recursive }, params.sandbox.as_ref(), ) @@ -102,10 +98,9 @@ impl FileSystemHandler { &self, params: FsGetMetadataParams, ) -> Result { - let path = PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; let metadata = self .file_system - .get_metadata(&path, params.sandbox.as_ref()) + .get_metadata(¶ms.path, params.sandbox.as_ref()) .await .map_err(map_fs_error)?; Ok(FsGetMetadataResponse { @@ -121,13 +116,11 @@ impl FileSystemHandler { &self, params: FsCanonicalizeParams, ) -> Result { - let requested_path = PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; let path = self .file_system - .canonicalize(&requested_path, params.sandbox.as_ref()) + .canonicalize(¶ms.path, params.sandbox.as_ref()) .await .map_err(map_fs_error)?; - let path = path.to_abs_path().map_err(map_fs_error)?; Ok(FsCanonicalizeResponse { path }) } @@ -136,7 +129,10 @@ impl FileSystemHandler { params: FsJoinParams, ) -> Result { // TODO(anp): remove and migrate callers to PathUri. - let path = params.base_path.join(params.path); + let path = params + .base_path + .join(¶ms.path.to_string_lossy()) + .map_err(|err| invalid_request(err.to_string()))?; Ok(FsJoinResponse { path }) } @@ -153,10 +149,9 @@ impl FileSystemHandler { &self, params: FsReadDirectoryParams, ) -> Result { - let path = PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; let entries = self .file_system - .read_directory(&path, params.sandbox.as_ref()) + .read_directory(¶ms.path, params.sandbox.as_ref()) .await .map_err(map_fs_error)? .into_iter() @@ -175,10 +170,9 @@ impl FileSystemHandler { ) -> Result { let recursive = params.recursive.unwrap_or(true); let force = params.force.unwrap_or(true); - let path = PathUri::from_abs_path(¶ms.path).map_err(map_fs_error)?; self.file_system .remove( - &path, + ¶ms.path, RemoveOptions { recursive, force }, params.sandbox.as_ref(), ) @@ -191,13 +185,10 @@ impl FileSystemHandler { &self, params: FsCopyParams, ) -> Result { - let source_path = PathUri::from_abs_path(¶ms.source_path).map_err(map_fs_error)?; - let destination_path = - PathUri::from_abs_path(¶ms.destination_path).map_err(map_fs_error)?; self.file_system .copy( - &source_path, - &destination_path, + ¶ms.source_path, + ¶ms.destination_path, CopyOptions { recursive: params.recursive, }, @@ -224,6 +215,7 @@ mod tests { use codex_protocol::protocol::NetworkAccess; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; + use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use super::*; @@ -252,9 +244,7 @@ mod tests { }, ), ] { - let path = - AbsolutePathBuf::from_absolute_path(temp_dir.path().join(file_name).as_path()) - .expect("absolute path"); + let path = PathUri::from_path(temp_dir.path().join(file_name)).expect("path URI"); handler .write_file(FsWriteFileParams { @@ -280,10 +270,10 @@ mod tests { .expect("canonicalize file"); assert_eq!( canonicalized.path, - AbsolutePathBuf::from_absolute_path( - std::fs::canonicalize(path.as_path()).expect("canonical path"), + PathUri::from_path( + std::fs::canonicalize(temp_dir.path().join(file_name)).expect("canonical path"), ) - .expect("absolute canonical path"), + .expect("canonical path URI"), ); let response = handler @@ -302,7 +292,7 @@ mod tests { } #[tokio::test] - async fn protocol_join_and_parent_remain_native_path_operations() { + async fn protocol_join_and_parent_use_path_uri_operations() { let temp_dir = tempfile::tempdir().expect("tempdir"); let runtime_paths = ExecServerRuntimePaths::new( std::env::current_exe().expect("current exe"), @@ -310,8 +300,7 @@ mod tests { ) .expect("runtime paths"); let handler = FileSystemHandler::new(runtime_paths); - let base_path = - AbsolutePathBuf::from_absolute_path(temp_dir.path()).expect("absolute tempdir"); + let base_path = PathUri::from_path(temp_dir.path()).expect("path URI"); let joined = handler .join(FsJoinParams { @@ -320,7 +309,10 @@ mod tests { }) .await .expect("join path"); - assert_eq!(joined.path, base_path.join("nested/file.txt")); + assert_eq!( + joined.path, + base_path.join("nested/file.txt").expect("joined path URI") + ); let parent = handler .parent(FsParentParams { From eafb482e8d0e8d723794462bc543798272031b9f Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Thu, 11 Jun 2026 19:30:32 +0000 Subject: [PATCH 2/5] codex: fix CI failure on PR #27653 --- codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs b/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs index 50e46b48542b..47703f87094c 100644 --- a/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs +++ b/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs @@ -26,7 +26,8 @@ use crate::protocol::InitializeResponse; #[tokio::test] async fn remote_file_system_sends_path_uris_without_native_conversion() { - let (websocket_url, captured_paths, server) = record_read_file_paths(2).await; + let (websocket_url, captured_paths, server) = + record_read_file_paths(/*expected_requests*/ 2).await; let file_system = RemoteFileSystem::new(LazyRemoteExecServerClient::new( ExecServerTransportParams::websocket_url(websocket_url), )); From f69b2d1e2a1e1c6db6e2aaf2a10ed2aca461a289 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Thu, 11 Jun 2026 19:42:23 +0000 Subject: [PATCH 3/5] codex: address review feedback on PR #27653 --- codex-rs/exec-server/README.md | 5 +- codex-rs/exec-server/src/client.rs | 92 +++++++++++++--- codex-rs/exec-server/src/protocol.rs | 18 ++++ .../src/remote_file_system_path_uri_tests.rs | 88 ++++++++++++--- .../src/server/file_system_handler.rs | 57 ++++++++-- codex-rs/exec-server/src/server/handler.rs | 73 ++++++++++--- .../exec-server/src/server/handler/tests.rs | 102 ++++++++++++++---- codex-rs/exec-server/src/server/registry.rs | 4 +- 8 files changed, 361 insertions(+), 78 deletions(-) diff --git a/codex-rs/exec-server/README.md b/codex-rs/exec-server/README.md index 208e53bc202b..82c48f7bd251 100644 --- a/codex-rs/exec-server/README.md +++ b/codex-rs/exec-server/README.md @@ -339,8 +339,9 @@ Params: ## Filesystem RPCs Filesystem methods use canonical `file:` URIs and return JSON-RPC errors for -invalid or unavailable paths. For compatibility, requests also accept native -absolute path strings and normalize them to `file:` URIs: +invalid or unavailable paths. Peers negotiate URI support during `initialize`; +connections to older peers retain native absolute path strings on the wire. +Requests accept either form and normalize it to a `file:` URI: - `fs/readFile` - `fs/writeFile` diff --git a/codex-rs/exec-server/src/client.rs b/codex-rs/exec-server/src/client.rs index 26b3e46e8859..87b7fa5a5ad0 100644 --- a/codex-rs/exec-server/src/client.rs +++ b/codex-rs/exec-server/src/client.rs @@ -3,11 +3,14 @@ use std::collections::HashMap; use std::sync::Arc; use std::sync::Mutex as StdMutex; use std::sync::OnceLock; +use std::sync::atomic::AtomicBool; use std::sync::atomic::AtomicU64; +use std::sync::atomic::Ordering; use std::time::Duration; use arc_swap::ArcSwap; use codex_app_server_protocol::JSONRPCNotification; +use codex_utils_path_uri::PathUri; use futures::FutureExt; use futures::future::BoxFuture; use serde_json::Value; @@ -80,6 +83,8 @@ use crate::protocol::INITIALIZE_METHOD; use crate::protocol::INITIALIZED_METHOD; use crate::protocol::InitializeParams; use crate::protocol::InitializeResponse; +use crate::protocol::InitializeWireParams; +use crate::protocol::InitializeWireResponse; use crate::protocol::ProcessOutputChunk; use crate::protocol::ProcessSignal; use crate::protocol::ReadParams; @@ -189,6 +194,7 @@ struct Inner { http_body_stream_failures: ArcSwap>, http_body_streams_write_lock: Mutex<()>, http_body_stream_next_id: AtomicU64, + filesystem_path_uris: AtomicBool, session_id: std::sync::RwLock>, reader_task: tokio::task::JoinHandle<()>, } @@ -343,17 +349,24 @@ impl ExecServerClient { } = options; timeout(initialize_timeout, async { - let response: InitializeResponse = self + let wire_response: InitializeWireResponse = self .inner .client .call( INITIALIZE_METHOD, - &InitializeParams { - client_name, - resume_session_id, + &InitializeWireParams { + params: InitializeParams { + client_name, + resume_session_id, + }, + filesystem_path_uris: true, }, ) .await?; + self.inner + .filesystem_path_uris + .store(wire_response.filesystem_path_uris, Ordering::Relaxed); + let response = wire_response.response; { let mut session_id = self .inner @@ -432,64 +445,85 @@ impl ExecServerClient { &self, params: FsReadFileParams, ) -> Result { - self.call(FS_READ_FILE_METHOD, ¶ms).await + self.call_filesystem(FS_READ_FILE_METHOD, ¶ms, &[("path", ¶ms.path)]) + .await } pub async fn fs_write_file( &self, params: FsWriteFileParams, ) -> Result { - self.call(FS_WRITE_FILE_METHOD, ¶ms).await + self.call_filesystem(FS_WRITE_FILE_METHOD, ¶ms, &[("path", ¶ms.path)]) + .await } pub async fn fs_create_directory( &self, params: FsCreateDirectoryParams, ) -> Result { - self.call(FS_CREATE_DIRECTORY_METHOD, ¶ms).await + self.call_filesystem( + FS_CREATE_DIRECTORY_METHOD, + ¶ms, + &[("path", ¶ms.path)], + ) + .await } pub async fn fs_get_metadata( &self, params: FsGetMetadataParams, ) -> Result { - self.call(FS_GET_METADATA_METHOD, ¶ms).await + self.call_filesystem(FS_GET_METADATA_METHOD, ¶ms, &[("path", ¶ms.path)]) + .await } pub async fn fs_canonicalize( &self, params: FsCanonicalizeParams, ) -> Result { - self.call(FS_CANONICALIZE_METHOD, ¶ms).await + self.call_filesystem(FS_CANONICALIZE_METHOD, ¶ms, &[("path", ¶ms.path)]) + .await } pub async fn fs_join(&self, params: FsJoinParams) -> Result { - self.call(FS_JOIN_METHOD, ¶ms).await + self.call_filesystem(FS_JOIN_METHOD, ¶ms, &[("basePath", ¶ms.base_path)]) + .await } pub async fn fs_parent( &self, params: FsParentParams, ) -> Result { - self.call(FS_PARENT_METHOD, ¶ms).await + self.call_filesystem(FS_PARENT_METHOD, ¶ms, &[("path", ¶ms.path)]) + .await } pub async fn fs_read_directory( &self, params: FsReadDirectoryParams, ) -> Result { - self.call(FS_READ_DIRECTORY_METHOD, ¶ms).await + self.call_filesystem(FS_READ_DIRECTORY_METHOD, ¶ms, &[("path", ¶ms.path)]) + .await } pub async fn fs_remove( &self, params: FsRemoveParams, ) -> Result { - self.call(FS_REMOVE_METHOD, ¶ms).await + self.call_filesystem(FS_REMOVE_METHOD, ¶ms, &[("path", ¶ms.path)]) + .await } pub async fn fs_copy(&self, params: FsCopyParams) -> Result { - self.call(FS_COPY_METHOD, ¶ms).await + self.call_filesystem( + FS_COPY_METHOD, + ¶ms, + &[ + ("sourcePath", ¶ms.source_path), + ("destinationPath", ¶ms.destination_path), + ], + ) + .await } pub(crate) async fn register_session( @@ -569,6 +603,7 @@ impl ExecServerClient { http_body_stream_failures: ArcSwap::from_pointee(HashMap::new()), http_body_streams_write_lock: Mutex::new(()), http_body_stream_next_id: AtomicU64::new(1), + filesystem_path_uris: AtomicBool::new(false), session_id: std::sync::RwLock::new(None), reader_task, } @@ -616,6 +651,35 @@ impl ExecServerClient { } } } + + async fn call_filesystem( + &self, + method: &str, + params: &P, + path_fields: &[(&str, &PathUri)], + ) -> Result + where + P: serde::Serialize, + T: serde::de::DeserializeOwned, + { + if self.inner.filesystem_path_uris.load(Ordering::Relaxed) { + return self.call(method, params).await; + } + + let mut legacy_params = serde_json::to_value(params)?; + let fields = legacy_params.as_object_mut().ok_or_else(|| { + ExecServerError::Protocol(format!("{method} params must serialize as an object")) + })?; + for (field, path) in path_fields { + let path = path.to_abs_path().map_err(|err| { + ExecServerError::Protocol(format!( + "legacy exec-server cannot represent path URI {path}: {err}" + )) + })?; + fields.insert((*field).to_string(), serde_json::to_value(path)?); + } + self.call(method, &legacy_params).await + } } impl From for ExecServerError { diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 49b1f762db99..5448cdb7ead3 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -66,6 +66,24 @@ pub struct InitializeResponse { pub session_id: String, } +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct InitializeWireParams { + #[serde(flatten)] + pub params: InitializeParams, + #[serde(default)] + pub filesystem_path_uris: bool, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct InitializeWireResponse { + #[serde(flatten)] + pub response: InitializeResponse, + #[serde(default)] + pub filesystem_path_uris: bool, +} + /// Information about an execution/filesystem environment. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] diff --git a/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs b/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs index 47703f87094c..6311a9deb40c 100644 --- a/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs +++ b/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs @@ -18,16 +18,23 @@ use tokio_tungstenite::tungstenite::Message; use super::*; use crate::client_api::ExecServerTransportParams; use crate::protocol::FS_READ_FILE_METHOD; -use crate::protocol::FsReadFileParams; use crate::protocol::FsReadFileResponse; use crate::protocol::INITIALIZE_METHOD; use crate::protocol::INITIALIZED_METHOD; use crate::protocol::InitializeResponse; +use crate::protocol::InitializeWireParams; +use crate::protocol::InitializeWireResponse; + +#[derive(Clone, Copy)] +enum ServerPathFormat { + PathUri, + LegacyNative, +} #[tokio::test] async fn remote_file_system_sends_path_uris_without_native_conversion() { let (websocket_url, captured_paths, server) = - record_read_file_paths(/*expected_requests*/ 2).await; + record_read_file_paths(ServerPathFormat::PathUri, /*expected_requests*/ 2).await; let file_system = RemoteFileSystem::new(LazyRemoteExecServerClient::new( ExecServerTransportParams::websocket_url(websocket_url), )); @@ -46,15 +53,49 @@ async fn remote_file_system_sends_path_uris_without_native_conversion() { ); } - assert_eq!(captured_paths.await.expect("captured paths"), paths); + assert_eq!( + captured_paths.await.expect("captured paths"), + paths.iter().map(ToString::to_string).collect::>() + ); + server.await.expect("recording server should succeed"); +} + +#[tokio::test] +async fn remote_file_system_uses_native_paths_with_legacy_servers() { + let (websocket_url, captured_paths, server) = + record_read_file_paths(ServerPathFormat::LegacyNative, /*expected_requests*/ 1).await; + let file_system = RemoteFileSystem::new(LazyRemoteExecServerClient::new( + ExecServerTransportParams::websocket_url(websocket_url), + )); + let path = PathUri::from_path(std::env::temp_dir().join("legacy-server.txt")) + .expect("native path URI"); + + assert_eq!( + file_system + .read_file(&path, /*sandbox*/ None) + .await + .expect("remote read should succeed"), + Vec::::new() + ); + + assert_eq!( + captured_paths.await.expect("captured paths"), + vec![ + path.to_abs_path() + .expect("native path") + .to_string_lossy() + .into_owned() + ] + ); server.await.expect("recording server should succeed"); } async fn record_read_file_paths( + server_path_format: ServerPathFormat, expected_requests: usize, ) -> ( String, - oneshot::Receiver>, + oneshot::Receiver>, tokio::task::JoinHandle<()>, ) { let listener = TcpListener::bind("127.0.0.1:0") @@ -67,7 +108,7 @@ async fn record_read_file_paths( let mut websocket = accept_async(stream) .await .expect("websocket handshake should succeed"); - complete_websocket_initialize(&mut websocket).await; + complete_websocket_initialize(&mut websocket, server_path_format).await; let mut captured_paths = Vec::with_capacity(expected_requests); for _ in 0..expected_requests { @@ -77,10 +118,14 @@ async fn record_read_file_paths( } other => panic!("expected fs/readFile request, got {other:?}"), }; - let params: FsReadFileParams = - serde_json::from_value(request.params.expect("fs/readFile params should exist")) - .expect("fs/readFile params should deserialize"); - captured_paths.push(params.path); + let params = request.params.expect("fs/readFile params should exist"); + captured_paths.push( + params + .get("path") + .and_then(serde_json::Value::as_str) + .expect("fs/readFile path should be a string") + .to_string(), + ); write_jsonrpc_websocket( &mut websocket, JSONRPCMessage::Response(JSONRPCResponse { @@ -101,19 +146,34 @@ async fn record_read_file_paths( (websocket_url, captured_paths_rx, server) } -async fn complete_websocket_initialize(websocket: &mut WebSocketStream) { +async fn complete_websocket_initialize( + websocket: &mut WebSocketStream, + server_path_format: ServerPathFormat, +) { let request = match read_jsonrpc_websocket(websocket).await { JSONRPCMessage::Request(request) if request.method == INITIALIZE_METHOD => request, other => panic!("expected initialize request, got {other:?}"), }; + let params: InitializeWireParams = + serde_json::from_value(request.params.expect("initialize params should exist")) + .expect("initialize params should deserialize"); + assert!(params.filesystem_path_uris); + let response = InitializeResponse { + session_id: "session-1".to_string(), + }; + let result = match server_path_format { + ServerPathFormat::PathUri => serde_json::to_value(InitializeWireResponse { + response, + filesystem_path_uris: true, + }), + ServerPathFormat::LegacyNative => serde_json::to_value(response), + } + .expect("initialize response should serialize"); write_jsonrpc_websocket( websocket, JSONRPCMessage::Response(JSONRPCResponse { id: request.id, - result: serde_json::to_value(InitializeResponse { - session_id: "session-1".to_string(), - }) - .expect("initialize response should serialize"), + result, }), ) .await; diff --git a/codex-rs/exec-server/src/server/file_system_handler.rs b/codex-rs/exec-server/src/server/file_system_handler.rs index 54f60ebb376b..4ffdc1d9a9bd 100644 --- a/codex-rs/exec-server/src/server/file_system_handler.rs +++ b/codex-rs/exec-server/src/server/file_system_handler.rs @@ -3,6 +3,7 @@ use std::io; use base64::Engine as _; use base64::engine::general_purpose::STANDARD; use codex_app_server_protocol::JSONRPCErrorError; +use codex_utils_path_uri::PathUri; use crate::CopyOptions; use crate::CreateDirectoryOptions; @@ -129,10 +130,9 @@ impl FileSystemHandler { params: FsJoinParams, ) -> Result { // TODO(anp): remove and migrate callers to PathUri. - let path = params - .base_path - .join(¶ms.path.to_string_lossy()) - .map_err(|err| invalid_request(err.to_string()))?; + let base_path = params.base_path.to_abs_path().map_err(map_fs_error)?; + let path = base_path.join(params.path); + let path = PathUri::from_abs_path(&path).map_err(map_fs_error)?; Ok(FsJoinResponse { path }) } @@ -141,7 +141,14 @@ impl FileSystemHandler { params: FsParentParams, ) -> Result { // TODO(anp): remove and migrate callers to PathUri. - let path = params.path.parent(); + let path = params + .path + .to_abs_path() + .map_err(map_fs_error)? + .parent() + .map(|path| PathUri::from_abs_path(&path)) + .transpose() + .map_err(map_fs_error)?; Ok(FsParentResponse { path }) } @@ -292,7 +299,7 @@ mod tests { } #[tokio::test] - async fn protocol_join_and_parent_use_path_uri_operations() { + async fn protocol_join_and_parent_preserve_native_path_operations() { let temp_dir = tempfile::tempdir().expect("tempdir"); let runtime_paths = ExecServerRuntimePaths::new( std::env::current_exe().expect("current exe"), @@ -300,7 +307,9 @@ mod tests { ) .expect("runtime paths"); let handler = FileSystemHandler::new(runtime_paths); - let base_path = PathUri::from_path(temp_dir.path()).expect("path URI"); + let native_base = + AbsolutePathBuf::from_absolute_path(temp_dir.path()).expect("absolute tempdir"); + let base_path = PathUri::from_abs_path(&native_base).expect("path URI"); let joined = handler .join(FsJoinParams { @@ -311,7 +320,7 @@ mod tests { .expect("join path"); assert_eq!( joined.path, - base_path.join("nested/file.txt").expect("joined path URI") + PathUri::from_abs_path(&native_base.join("nested/file.txt")).expect("joined path URI") ); let parent = handler @@ -320,6 +329,36 @@ mod tests { }) .await .expect("parent path"); - assert_eq!(parent.path, joined.path.parent()); + assert_eq!( + parent.path, + native_base + .join("nested/file.txt") + .parent() + .map(|path| PathUri::from_abs_path(&path).expect("parent path URI")) + ); + + let absolute_path = native_base.join("absolute.txt"); + let joined = handler + .join(FsJoinParams { + base_path, + path: absolute_path.as_path().to_path_buf(), + }) + .await + .expect("join absolute path"); + assert_eq!( + joined.path, + PathUri::from_abs_path(&absolute_path).expect("absolute path URI") + ); + + let native_root = native_base + .ancestors() + .last() + .expect("absolute path should have a root"); + let root = PathUri::from_abs_path(&native_root).expect("root path URI"); + let parent = handler + .parent(FsParentParams { path: root }) + .await + .expect("root parent"); + assert_eq!(parent, FsParentResponse { path: None }); } } diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index 395ce6773050..9d81b3a26828 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -5,6 +5,9 @@ use std::sync::atomic::Ordering; use codex_app_server_protocol::JSONRPCErrorError; use codex_app_server_protocol::RequestId; +use codex_utils_path_uri::PathUri; +use serde::Serialize; +use serde_json::Value; use serde_json::to_value; use std::collections::HashSet; use tokio::sync::Mutex; @@ -18,7 +21,6 @@ use crate::protocol::EnvironmentInfo; use crate::protocol::ExecParams; use crate::protocol::ExecResponse; use crate::protocol::FsCanonicalizeParams; -use crate::protocol::FsCanonicalizeResponse; use crate::protocol::FsCopyParams; use crate::protocol::FsCopyResponse; use crate::protocol::FsCreateDirectoryParams; @@ -26,9 +28,7 @@ use crate::protocol::FsCreateDirectoryResponse; use crate::protocol::FsGetMetadataParams; use crate::protocol::FsGetMetadataResponse; use crate::protocol::FsJoinParams; -use crate::protocol::FsJoinResponse; use crate::protocol::FsParentParams; -use crate::protocol::FsParentResponse; use crate::protocol::FsReadDirectoryParams; use crate::protocol::FsReadDirectoryResponse; use crate::protocol::FsReadFileParams; @@ -38,8 +38,9 @@ use crate::protocol::FsRemoveResponse; use crate::protocol::FsWriteFileParams; use crate::protocol::FsWriteFileResponse; use crate::protocol::HttpRequestParams; -use crate::protocol::InitializeParams; use crate::protocol::InitializeResponse; +use crate::protocol::InitializeWireParams; +use crate::protocol::InitializeWireResponse; use crate::protocol::ReadParams; use crate::protocol::ReadResponse; use crate::protocol::SignalParams; @@ -64,6 +65,7 @@ pub(crate) struct ExecServerHandler { background_task_shutdown: CancellationToken, background_tasks: TaskTracker, file_system: FileSystemHandler, + filesystem_path_uris: AtomicBool, initialize_requested: AtomicBool, initialized: AtomicBool, } @@ -82,6 +84,7 @@ impl ExecServerHandler { background_task_shutdown: CancellationToken::new(), background_tasks: TaskTracker::new(), file_system: FileSystemHandler::new(runtime_paths), + filesystem_path_uris: AtomicBool::new(false), initialize_requested: AtomicBool::new(false), initialized: AtomicBool::new(false), } @@ -103,8 +106,8 @@ impl ExecServerHandler { pub(crate) async fn initialize( &self, - params: InitializeParams, - ) -> Result { + params: InitializeWireParams, + ) -> Result { if self.initialize_requested.swap(true, Ordering::SeqCst) { return Err(invalid_request( "initialize may only be sent once per connection".to_string(), @@ -113,7 +116,10 @@ impl ExecServerHandler { let session = match self .session_registry - .attach(params.resume_session_id.clone(), self.notifications.clone()) + .attach( + params.params.resume_session_id.clone(), + self.notifications.clone(), + ) .await { Ok(session) => session, @@ -132,7 +138,12 @@ impl ExecServerHandler { .session .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) = Some(session); - Ok(InitializeResponse { session_id }) + self.filesystem_path_uris + .store(params.filesystem_path_uris, Ordering::Relaxed); + Ok(InitializeWireResponse { + response: InitializeResponse { session_id }, + filesystem_path_uris: params.filesystem_path_uris, + }) } pub(crate) fn initialized(&self) -> Result<(), String> { @@ -265,25 +276,25 @@ impl ExecServerHandler { pub(crate) async fn fs_canonicalize( &self, params: FsCanonicalizeParams, - ) -> Result { + ) -> Result { self.require_initialized_for("filesystem")?; - self.file_system.canonicalize(params).await + let response = self.file_system.canonicalize(params).await?; + self.serialize_filesystem_response(&response, &[("path", Some(&response.path))]) } - pub(crate) async fn fs_join( - &self, - params: FsJoinParams, - ) -> Result { + pub(crate) async fn fs_join(&self, params: FsJoinParams) -> Result { self.require_initialized_for("filesystem")?; - self.file_system.join(params).await + let response = self.file_system.join(params).await?; + self.serialize_filesystem_response(&response, &[("path", Some(&response.path))]) } pub(crate) async fn fs_parent( &self, params: FsParentParams, - ) -> Result { + ) -> Result { self.require_initialized_for("filesystem")?; - self.file_system.parent(params).await + let response = self.file_system.parent(params).await?; + self.serialize_filesystem_response(&response, &[("path", response.path.as_ref())]) } pub(crate) async fn fs_read_directory( @@ -310,6 +321,34 @@ impl ExecServerHandler { self.file_system.copy(params).await } + fn serialize_filesystem_response( + &self, + response: &R, + path_fields: &[(&str, Option<&PathUri>)], + ) -> Result { + let mut result = to_value(response).map_err(|err| internal_error(err.to_string()))?; + if self.filesystem_path_uris.load(Ordering::Relaxed) { + return Ok(result); + } + + let fields = result.as_object_mut().ok_or_else(|| { + internal_error("filesystem response must serialize as an object".to_string()) + })?; + for (field, path) in path_fields { + let Some(path) = path else { + continue; + }; + let path = path + .to_abs_path() + .map_err(|err| internal_error(err.to_string()))?; + fields.insert( + (*field).to_string(), + to_value(path).map_err(|err| internal_error(err.to_string()))?, + ); + } + Ok(result) + } + fn require_initialized_for( &self, method_family: &str, diff --git a/codex-rs/exec-server/src/server/handler/tests.rs b/codex-rs/exec-server/src/server/handler/tests.rs index 6b632fe8909b..043ee3bce574 100644 --- a/codex-rs/exec-server/src/server/handler/tests.rs +++ b/codex-rs/exec-server/src/server/handler/tests.rs @@ -2,7 +2,9 @@ use std::collections::HashMap; use std::sync::Arc; use std::time::Duration; +use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; +use serde_json::json; use tokio::sync::mpsc; use uuid::Uuid; @@ -10,7 +12,9 @@ use super::ExecServerHandler; use crate::ExecServerRuntimePaths; use crate::ProcessId; use crate::protocol::ExecParams; +use crate::protocol::FsCanonicalizeParams; use crate::protocol::InitializeParams; +use crate::protocol::InitializeWireParams; use crate::protocol::ReadParams; use crate::protocol::ReadResponse; use crate::protocol::TerminateParams; @@ -18,6 +22,12 @@ use crate::protocol::TerminateResponse; use crate::rpc::RpcNotificationSender; use crate::server::session_registry::SessionRegistry; +#[derive(Clone, Copy)] +enum ClientPathFormat { + PathUri, + LegacyNative, +} + fn exec_params(process_id: &str) -> ExecParams { exec_params_with_argv(process_id, sleep_argv()) } @@ -76,6 +86,12 @@ fn test_runtime_paths() -> ExecServerRuntimePaths { } async fn initialized_handler() -> Arc { + initialized_handler_with_path_format(ClientPathFormat::PathUri).await +} + +async fn initialized_handler_with_path_format( + client_path_format: ClientPathFormat, +) -> Arc { let (outgoing_tx, _outgoing_rx) = mpsc::channel(16); let registry = SessionRegistry::new(); let handler = Arc::new(ExecServerHandler::new( @@ -84,17 +100,48 @@ async fn initialized_handler() -> Arc { test_runtime_paths(), )); let initialize_response = handler - .initialize(InitializeParams { - client_name: "exec-server-test".to_string(), - resume_session_id: None, + .initialize(InitializeWireParams { + params: InitializeParams { + client_name: "exec-server-test".to_string(), + resume_session_id: None, + }, + filesystem_path_uris: matches!(client_path_format, ClientPathFormat::PathUri), }) .await .expect("initialize"); - Uuid::parse_str(&initialize_response.session_id).expect("session id should be a UUID"); + Uuid::parse_str(&initialize_response.response.session_id).expect("session id should be a UUID"); handler.initialized().expect("initialized"); handler } +#[tokio::test] +async fn canonicalize_response_uses_negotiated_path_format() { + let temp_dir = tempfile::tempdir().expect("tempdir"); + let native_path = + std::fs::canonicalize(temp_dir.path()).expect("canonical tempdir should exist"); + let path = PathUri::from_path(temp_dir.path()).expect("tempdir path URI"); + let path_uri = PathUri::from_path(&native_path).expect("canonical path URI"); + + for (client_path_format, expected_path) in [ + (ClientPathFormat::PathUri, path_uri.to_string()), + ( + ClientPathFormat::LegacyNative, + native_path.to_string_lossy().into_owned(), + ), + ] { + let handler = initialized_handler_with_path_format(client_path_format).await; + let response = handler + .fs_canonicalize(FsCanonicalizeParams { + path: path.clone(), + sandbox: None, + }) + .await + .expect("canonicalize response"); + assert_eq!(response, json!({ "path": expected_path })); + handler.shutdown().await; + } +} + #[tokio::test] async fn duplicate_process_ids_allow_only_one_successful_start() { let handler = initialized_handler().await; @@ -162,9 +209,12 @@ async fn long_poll_read_fails_after_session_resume() { test_runtime_paths(), )); let initialize_response = first_handler - .initialize(InitializeParams { - client_name: "exec-server-test".to_string(), - resume_session_id: None, + .initialize(InitializeWireParams { + params: InitializeParams { + client_name: "exec-server-test".to_string(), + resume_session_id: None, + }, + filesystem_path_uris: true, }) .await .expect("initialize"); @@ -202,9 +252,12 @@ async fn long_poll_read_fails_after_session_resume() { test_runtime_paths(), )); second_handler - .initialize(InitializeParams { - client_name: "exec-server-test".to_string(), - resume_session_id: Some(initialize_response.session_id), + .initialize(InitializeWireParams { + params: InitializeParams { + client_name: "exec-server-test".to_string(), + resume_session_id: Some(initialize_response.response.session_id), + }, + filesystem_path_uris: true, }) .await .expect("initialize second connection"); @@ -235,9 +288,12 @@ async fn active_session_resume_is_rejected() { test_runtime_paths(), )); let initialize_response = first_handler - .initialize(InitializeParams { - client_name: "exec-server-test".to_string(), - resume_session_id: None, + .initialize(InitializeWireParams { + params: InitializeParams { + client_name: "exec-server-test".to_string(), + resume_session_id: None, + }, + filesystem_path_uris: true, }) .await .expect("initialize"); @@ -249,9 +305,12 @@ async fn active_session_resume_is_rejected() { test_runtime_paths(), )); let err = second_handler - .initialize(InitializeParams { - client_name: "exec-server-test".to_string(), - resume_session_id: Some(initialize_response.session_id.clone()), + .initialize(InitializeWireParams { + params: InitializeParams { + client_name: "exec-server-test".to_string(), + resume_session_id: Some(initialize_response.response.session_id.clone()), + }, + filesystem_path_uris: true, }) .await .expect_err("active session resume should fail"); @@ -261,7 +320,7 @@ async fn active_session_resume_is_rejected() { err.message, format!( "session {} is already attached to another connection", - initialize_response.session_id + initialize_response.response.session_id ) ); @@ -277,9 +336,12 @@ async fn output_and_exit_are_retained_after_notification_receiver_closes() { test_runtime_paths(), )); handler - .initialize(InitializeParams { - client_name: "exec-server-test".to_string(), - resume_session_id: None, + .initialize(InitializeWireParams { + params: InitializeParams { + client_name: "exec-server-test".to_string(), + resume_session_id: None, + }, + filesystem_path_uris: true, }) .await .expect("initialize"); diff --git a/codex-rs/exec-server/src/server/registry.rs b/codex-rs/exec-server/src/server/registry.rs index 1f652c1c4c3d..f73c98d26d0f 100644 --- a/codex-rs/exec-server/src/server/registry.rs +++ b/codex-rs/exec-server/src/server/registry.rs @@ -31,7 +31,7 @@ use crate::protocol::HTTP_REQUEST_METHOD; use crate::protocol::HttpRequestParams; use crate::protocol::INITIALIZE_METHOD; use crate::protocol::INITIALIZED_METHOD; -use crate::protocol::InitializeParams; +use crate::protocol::InitializeWireParams; use crate::protocol::ReadParams; use crate::protocol::SignalParams; use crate::protocol::TerminateParams; @@ -49,7 +49,7 @@ pub(crate) fn build_router() -> RpcRouter { ); router.request( INITIALIZE_METHOD, - |handler: Arc, params: InitializeParams| async move { + |handler: Arc, params: InitializeWireParams| async move { handler.initialize(params).await }, ); From 9a8ae04cb9831aebe459f0e45b492cd6bbfb9dc9 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Thu, 11 Jun 2026 19:55:20 +0000 Subject: [PATCH 4/5] codex: fix CI failure on PR #27653 --- codex-rs/exec-server/src/sandboxed_file_system.rs | 13 +++++++++++++ codex-rs/exec-server/src/server/handler/tests.rs | 14 ++++++++------ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/codex-rs/exec-server/src/sandboxed_file_system.rs b/codex-rs/exec-server/src/sandboxed_file_system.rs index 95d52ed7f03c..df708ffac709 100644 --- a/codex-rs/exec-server/src/sandboxed_file_system.rs +++ b/codex-rs/exec-server/src/sandboxed_file_system.rs @@ -58,6 +58,7 @@ impl ExecutorFileSystem for SandboxedFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult { let sandbox = require_platform_sandbox(sandbox)?; + validate_native_path(path)?; let response = self .run_sandboxed( sandbox, @@ -78,6 +79,7 @@ impl ExecutorFileSystem for SandboxedFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult> { let sandbox = require_platform_sandbox(sandbox)?; + validate_native_path(path)?; let response = self .run_sandboxed( sandbox, @@ -104,6 +106,7 @@ impl ExecutorFileSystem for SandboxedFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { let sandbox = require_platform_sandbox(sandbox)?; + validate_native_path(path)?; self.run_sandboxed( sandbox, FsHelperRequest::WriteFile(FsWriteFileParams { @@ -125,6 +128,7 @@ impl ExecutorFileSystem for SandboxedFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { let sandbox = require_platform_sandbox(sandbox)?; + validate_native_path(path)?; self.run_sandboxed( sandbox, FsHelperRequest::CreateDirectory(FsCreateDirectoryParams { @@ -145,6 +149,7 @@ impl ExecutorFileSystem for SandboxedFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult { let sandbox = require_platform_sandbox(sandbox)?; + validate_native_path(path)?; let response = self .run_sandboxed( sandbox, @@ -171,6 +176,7 @@ impl ExecutorFileSystem for SandboxedFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult> { let sandbox = require_platform_sandbox(sandbox)?; + validate_native_path(path)?; let response = self .run_sandboxed( sandbox, @@ -200,6 +206,7 @@ impl ExecutorFileSystem for SandboxedFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { let sandbox = require_platform_sandbox(sandbox)?; + validate_native_path(path)?; self.run_sandboxed( sandbox, FsHelperRequest::Remove(FsRemoveParams { @@ -223,6 +230,8 @@ impl ExecutorFileSystem for SandboxedFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { let sandbox = require_platform_sandbox(sandbox)?; + validate_native_path(source_path)?; + validate_native_path(destination_path)?; self.run_sandboxed( sandbox, FsHelperRequest::Copy(FsCopyParams { @@ -239,6 +248,10 @@ impl ExecutorFileSystem for SandboxedFileSystem { } } +fn validate_native_path(path: &PathUri) -> FileSystemResult<()> { + path.to_abs_path().map(drop) +} + fn require_platform_sandbox( sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<&FileSystemSandboxContext> { diff --git a/codex-rs/exec-server/src/server/handler/tests.rs b/codex-rs/exec-server/src/server/handler/tests.rs index 043ee3bce574..e72c92d90e47 100644 --- a/codex-rs/exec-server/src/server/handler/tests.rs +++ b/codex-rs/exec-server/src/server/handler/tests.rs @@ -117,17 +117,19 @@ async fn initialized_handler_with_path_format( #[tokio::test] async fn canonicalize_response_uses_negotiated_path_format() { let temp_dir = tempfile::tempdir().expect("tempdir"); - let native_path = + let canonical_path = std::fs::canonicalize(temp_dir.path()).expect("canonical tempdir should exist"); let path = PathUri::from_path(temp_dir.path()).expect("tempdir path URI"); - let path_uri = PathUri::from_path(&native_path).expect("canonical path URI"); + let path_uri = PathUri::from_path(canonical_path).expect("canonical path URI"); + let legacy_path = path_uri + .to_abs_path() + .expect("legacy native path") + .to_string_lossy() + .into_owned(); for (client_path_format, expected_path) in [ (ClientPathFormat::PathUri, path_uri.to_string()), - ( - ClientPathFormat::LegacyNative, - native_path.to_string_lossy().into_owned(), - ), + (ClientPathFormat::LegacyNative, legacy_path), ] { let handler = initialized_handler_with_path_format(client_path_format).await; let response = handler From 8eb6f5af6fcdd069dbb0b31d6fd62f91900d9443 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Thu, 11 Jun 2026 20:25:30 +0000 Subject: [PATCH 5/5] exec-server: simplify PathUri protocol rollout --- codex-rs/exec-server/README.md | 5 +- codex-rs/exec-server/src/client.rs | 92 +++------------- codex-rs/exec-server/src/protocol.rs | 18 --- .../src/remote_file_system_path_uri_tests.rs | 88 +++------------ codex-rs/exec-server/src/server/handler.rs | 73 +++--------- .../exec-server/src/server/handler/tests.rs | 104 ++++-------------- codex-rs/exec-server/src/server/registry.rs | 4 +- 7 files changed, 69 insertions(+), 315 deletions(-) diff --git a/codex-rs/exec-server/README.md b/codex-rs/exec-server/README.md index 82c48f7bd251..208e53bc202b 100644 --- a/codex-rs/exec-server/README.md +++ b/codex-rs/exec-server/README.md @@ -339,9 +339,8 @@ Params: ## Filesystem RPCs Filesystem methods use canonical `file:` URIs and return JSON-RPC errors for -invalid or unavailable paths. Peers negotiate URI support during `initialize`; -connections to older peers retain native absolute path strings on the wire. -Requests accept either form and normalize it to a `file:` URI: +invalid or unavailable paths. For compatibility, requests also accept native +absolute path strings and normalize them to `file:` URIs: - `fs/readFile` - `fs/writeFile` diff --git a/codex-rs/exec-server/src/client.rs b/codex-rs/exec-server/src/client.rs index 87b7fa5a5ad0..26b3e46e8859 100644 --- a/codex-rs/exec-server/src/client.rs +++ b/codex-rs/exec-server/src/client.rs @@ -3,14 +3,11 @@ use std::collections::HashMap; use std::sync::Arc; use std::sync::Mutex as StdMutex; use std::sync::OnceLock; -use std::sync::atomic::AtomicBool; use std::sync::atomic::AtomicU64; -use std::sync::atomic::Ordering; use std::time::Duration; use arc_swap::ArcSwap; use codex_app_server_protocol::JSONRPCNotification; -use codex_utils_path_uri::PathUri; use futures::FutureExt; use futures::future::BoxFuture; use serde_json::Value; @@ -83,8 +80,6 @@ use crate::protocol::INITIALIZE_METHOD; use crate::protocol::INITIALIZED_METHOD; use crate::protocol::InitializeParams; use crate::protocol::InitializeResponse; -use crate::protocol::InitializeWireParams; -use crate::protocol::InitializeWireResponse; use crate::protocol::ProcessOutputChunk; use crate::protocol::ProcessSignal; use crate::protocol::ReadParams; @@ -194,7 +189,6 @@ struct Inner { http_body_stream_failures: ArcSwap>, http_body_streams_write_lock: Mutex<()>, http_body_stream_next_id: AtomicU64, - filesystem_path_uris: AtomicBool, session_id: std::sync::RwLock>, reader_task: tokio::task::JoinHandle<()>, } @@ -349,24 +343,17 @@ impl ExecServerClient { } = options; timeout(initialize_timeout, async { - let wire_response: InitializeWireResponse = self + let response: InitializeResponse = self .inner .client .call( INITIALIZE_METHOD, - &InitializeWireParams { - params: InitializeParams { - client_name, - resume_session_id, - }, - filesystem_path_uris: true, + &InitializeParams { + client_name, + resume_session_id, }, ) .await?; - self.inner - .filesystem_path_uris - .store(wire_response.filesystem_path_uris, Ordering::Relaxed); - let response = wire_response.response; { let mut session_id = self .inner @@ -445,85 +432,64 @@ impl ExecServerClient { &self, params: FsReadFileParams, ) -> Result { - self.call_filesystem(FS_READ_FILE_METHOD, ¶ms, &[("path", ¶ms.path)]) - .await + self.call(FS_READ_FILE_METHOD, ¶ms).await } pub async fn fs_write_file( &self, params: FsWriteFileParams, ) -> Result { - self.call_filesystem(FS_WRITE_FILE_METHOD, ¶ms, &[("path", ¶ms.path)]) - .await + self.call(FS_WRITE_FILE_METHOD, ¶ms).await } pub async fn fs_create_directory( &self, params: FsCreateDirectoryParams, ) -> Result { - self.call_filesystem( - FS_CREATE_DIRECTORY_METHOD, - ¶ms, - &[("path", ¶ms.path)], - ) - .await + self.call(FS_CREATE_DIRECTORY_METHOD, ¶ms).await } pub async fn fs_get_metadata( &self, params: FsGetMetadataParams, ) -> Result { - self.call_filesystem(FS_GET_METADATA_METHOD, ¶ms, &[("path", ¶ms.path)]) - .await + self.call(FS_GET_METADATA_METHOD, ¶ms).await } pub async fn fs_canonicalize( &self, params: FsCanonicalizeParams, ) -> Result { - self.call_filesystem(FS_CANONICALIZE_METHOD, ¶ms, &[("path", ¶ms.path)]) - .await + self.call(FS_CANONICALIZE_METHOD, ¶ms).await } pub async fn fs_join(&self, params: FsJoinParams) -> Result { - self.call_filesystem(FS_JOIN_METHOD, ¶ms, &[("basePath", ¶ms.base_path)]) - .await + self.call(FS_JOIN_METHOD, ¶ms).await } pub async fn fs_parent( &self, params: FsParentParams, ) -> Result { - self.call_filesystem(FS_PARENT_METHOD, ¶ms, &[("path", ¶ms.path)]) - .await + self.call(FS_PARENT_METHOD, ¶ms).await } pub async fn fs_read_directory( &self, params: FsReadDirectoryParams, ) -> Result { - self.call_filesystem(FS_READ_DIRECTORY_METHOD, ¶ms, &[("path", ¶ms.path)]) - .await + self.call(FS_READ_DIRECTORY_METHOD, ¶ms).await } pub async fn fs_remove( &self, params: FsRemoveParams, ) -> Result { - self.call_filesystem(FS_REMOVE_METHOD, ¶ms, &[("path", ¶ms.path)]) - .await + self.call(FS_REMOVE_METHOD, ¶ms).await } pub async fn fs_copy(&self, params: FsCopyParams) -> Result { - self.call_filesystem( - FS_COPY_METHOD, - ¶ms, - &[ - ("sourcePath", ¶ms.source_path), - ("destinationPath", ¶ms.destination_path), - ], - ) - .await + self.call(FS_COPY_METHOD, ¶ms).await } pub(crate) async fn register_session( @@ -603,7 +569,6 @@ impl ExecServerClient { http_body_stream_failures: ArcSwap::from_pointee(HashMap::new()), http_body_streams_write_lock: Mutex::new(()), http_body_stream_next_id: AtomicU64::new(1), - filesystem_path_uris: AtomicBool::new(false), session_id: std::sync::RwLock::new(None), reader_task, } @@ -651,35 +616,6 @@ impl ExecServerClient { } } } - - async fn call_filesystem( - &self, - method: &str, - params: &P, - path_fields: &[(&str, &PathUri)], - ) -> Result - where - P: serde::Serialize, - T: serde::de::DeserializeOwned, - { - if self.inner.filesystem_path_uris.load(Ordering::Relaxed) { - return self.call(method, params).await; - } - - let mut legacy_params = serde_json::to_value(params)?; - let fields = legacy_params.as_object_mut().ok_or_else(|| { - ExecServerError::Protocol(format!("{method} params must serialize as an object")) - })?; - for (field, path) in path_fields { - let path = path.to_abs_path().map_err(|err| { - ExecServerError::Protocol(format!( - "legacy exec-server cannot represent path URI {path}: {err}" - )) - })?; - fields.insert((*field).to_string(), serde_json::to_value(path)?); - } - self.call(method, &legacy_params).await - } } impl From for ExecServerError { diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 5448cdb7ead3..49b1f762db99 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -66,24 +66,6 @@ pub struct InitializeResponse { pub session_id: String, } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] -pub(crate) struct InitializeWireParams { - #[serde(flatten)] - pub params: InitializeParams, - #[serde(default)] - pub filesystem_path_uris: bool, -} - -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] -pub(crate) struct InitializeWireResponse { - #[serde(flatten)] - pub response: InitializeResponse, - #[serde(default)] - pub filesystem_path_uris: bool, -} - /// Information about an execution/filesystem environment. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] diff --git a/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs b/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs index 6311a9deb40c..47703f87094c 100644 --- a/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs +++ b/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs @@ -18,23 +18,16 @@ use tokio_tungstenite::tungstenite::Message; use super::*; use crate::client_api::ExecServerTransportParams; use crate::protocol::FS_READ_FILE_METHOD; +use crate::protocol::FsReadFileParams; use crate::protocol::FsReadFileResponse; use crate::protocol::INITIALIZE_METHOD; use crate::protocol::INITIALIZED_METHOD; use crate::protocol::InitializeResponse; -use crate::protocol::InitializeWireParams; -use crate::protocol::InitializeWireResponse; - -#[derive(Clone, Copy)] -enum ServerPathFormat { - PathUri, - LegacyNative, -} #[tokio::test] async fn remote_file_system_sends_path_uris_without_native_conversion() { let (websocket_url, captured_paths, server) = - record_read_file_paths(ServerPathFormat::PathUri, /*expected_requests*/ 2).await; + record_read_file_paths(/*expected_requests*/ 2).await; let file_system = RemoteFileSystem::new(LazyRemoteExecServerClient::new( ExecServerTransportParams::websocket_url(websocket_url), )); @@ -53,49 +46,15 @@ async fn remote_file_system_sends_path_uris_without_native_conversion() { ); } - assert_eq!( - captured_paths.await.expect("captured paths"), - paths.iter().map(ToString::to_string).collect::>() - ); - server.await.expect("recording server should succeed"); -} - -#[tokio::test] -async fn remote_file_system_uses_native_paths_with_legacy_servers() { - let (websocket_url, captured_paths, server) = - record_read_file_paths(ServerPathFormat::LegacyNative, /*expected_requests*/ 1).await; - let file_system = RemoteFileSystem::new(LazyRemoteExecServerClient::new( - ExecServerTransportParams::websocket_url(websocket_url), - )); - let path = PathUri::from_path(std::env::temp_dir().join("legacy-server.txt")) - .expect("native path URI"); - - assert_eq!( - file_system - .read_file(&path, /*sandbox*/ None) - .await - .expect("remote read should succeed"), - Vec::::new() - ); - - assert_eq!( - captured_paths.await.expect("captured paths"), - vec![ - path.to_abs_path() - .expect("native path") - .to_string_lossy() - .into_owned() - ] - ); + assert_eq!(captured_paths.await.expect("captured paths"), paths); server.await.expect("recording server should succeed"); } async fn record_read_file_paths( - server_path_format: ServerPathFormat, expected_requests: usize, ) -> ( String, - oneshot::Receiver>, + oneshot::Receiver>, tokio::task::JoinHandle<()>, ) { let listener = TcpListener::bind("127.0.0.1:0") @@ -108,7 +67,7 @@ async fn record_read_file_paths( let mut websocket = accept_async(stream) .await .expect("websocket handshake should succeed"); - complete_websocket_initialize(&mut websocket, server_path_format).await; + complete_websocket_initialize(&mut websocket).await; let mut captured_paths = Vec::with_capacity(expected_requests); for _ in 0..expected_requests { @@ -118,14 +77,10 @@ async fn record_read_file_paths( } other => panic!("expected fs/readFile request, got {other:?}"), }; - let params = request.params.expect("fs/readFile params should exist"); - captured_paths.push( - params - .get("path") - .and_then(serde_json::Value::as_str) - .expect("fs/readFile path should be a string") - .to_string(), - ); + let params: FsReadFileParams = + serde_json::from_value(request.params.expect("fs/readFile params should exist")) + .expect("fs/readFile params should deserialize"); + captured_paths.push(params.path); write_jsonrpc_websocket( &mut websocket, JSONRPCMessage::Response(JSONRPCResponse { @@ -146,34 +101,19 @@ async fn record_read_file_paths( (websocket_url, captured_paths_rx, server) } -async fn complete_websocket_initialize( - websocket: &mut WebSocketStream, - server_path_format: ServerPathFormat, -) { +async fn complete_websocket_initialize(websocket: &mut WebSocketStream) { let request = match read_jsonrpc_websocket(websocket).await { JSONRPCMessage::Request(request) if request.method == INITIALIZE_METHOD => request, other => panic!("expected initialize request, got {other:?}"), }; - let params: InitializeWireParams = - serde_json::from_value(request.params.expect("initialize params should exist")) - .expect("initialize params should deserialize"); - assert!(params.filesystem_path_uris); - let response = InitializeResponse { - session_id: "session-1".to_string(), - }; - let result = match server_path_format { - ServerPathFormat::PathUri => serde_json::to_value(InitializeWireResponse { - response, - filesystem_path_uris: true, - }), - ServerPathFormat::LegacyNative => serde_json::to_value(response), - } - .expect("initialize response should serialize"); write_jsonrpc_websocket( websocket, JSONRPCMessage::Response(JSONRPCResponse { id: request.id, - result, + result: serde_json::to_value(InitializeResponse { + session_id: "session-1".to_string(), + }) + .expect("initialize response should serialize"), }), ) .await; diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index 9d81b3a26828..395ce6773050 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -5,9 +5,6 @@ use std::sync::atomic::Ordering; use codex_app_server_protocol::JSONRPCErrorError; use codex_app_server_protocol::RequestId; -use codex_utils_path_uri::PathUri; -use serde::Serialize; -use serde_json::Value; use serde_json::to_value; use std::collections::HashSet; use tokio::sync::Mutex; @@ -21,6 +18,7 @@ use crate::protocol::EnvironmentInfo; use crate::protocol::ExecParams; use crate::protocol::ExecResponse; use crate::protocol::FsCanonicalizeParams; +use crate::protocol::FsCanonicalizeResponse; use crate::protocol::FsCopyParams; use crate::protocol::FsCopyResponse; use crate::protocol::FsCreateDirectoryParams; @@ -28,7 +26,9 @@ use crate::protocol::FsCreateDirectoryResponse; use crate::protocol::FsGetMetadataParams; use crate::protocol::FsGetMetadataResponse; use crate::protocol::FsJoinParams; +use crate::protocol::FsJoinResponse; use crate::protocol::FsParentParams; +use crate::protocol::FsParentResponse; use crate::protocol::FsReadDirectoryParams; use crate::protocol::FsReadDirectoryResponse; use crate::protocol::FsReadFileParams; @@ -38,9 +38,8 @@ use crate::protocol::FsRemoveResponse; use crate::protocol::FsWriteFileParams; use crate::protocol::FsWriteFileResponse; use crate::protocol::HttpRequestParams; +use crate::protocol::InitializeParams; use crate::protocol::InitializeResponse; -use crate::protocol::InitializeWireParams; -use crate::protocol::InitializeWireResponse; use crate::protocol::ReadParams; use crate::protocol::ReadResponse; use crate::protocol::SignalParams; @@ -65,7 +64,6 @@ pub(crate) struct ExecServerHandler { background_task_shutdown: CancellationToken, background_tasks: TaskTracker, file_system: FileSystemHandler, - filesystem_path_uris: AtomicBool, initialize_requested: AtomicBool, initialized: AtomicBool, } @@ -84,7 +82,6 @@ impl ExecServerHandler { background_task_shutdown: CancellationToken::new(), background_tasks: TaskTracker::new(), file_system: FileSystemHandler::new(runtime_paths), - filesystem_path_uris: AtomicBool::new(false), initialize_requested: AtomicBool::new(false), initialized: AtomicBool::new(false), } @@ -106,8 +103,8 @@ impl ExecServerHandler { pub(crate) async fn initialize( &self, - params: InitializeWireParams, - ) -> Result { + params: InitializeParams, + ) -> Result { if self.initialize_requested.swap(true, Ordering::SeqCst) { return Err(invalid_request( "initialize may only be sent once per connection".to_string(), @@ -116,10 +113,7 @@ impl ExecServerHandler { let session = match self .session_registry - .attach( - params.params.resume_session_id.clone(), - self.notifications.clone(), - ) + .attach(params.resume_session_id.clone(), self.notifications.clone()) .await { Ok(session) => session, @@ -138,12 +132,7 @@ impl ExecServerHandler { .session .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) = Some(session); - self.filesystem_path_uris - .store(params.filesystem_path_uris, Ordering::Relaxed); - Ok(InitializeWireResponse { - response: InitializeResponse { session_id }, - filesystem_path_uris: params.filesystem_path_uris, - }) + Ok(InitializeResponse { session_id }) } pub(crate) fn initialized(&self) -> Result<(), String> { @@ -276,25 +265,25 @@ impl ExecServerHandler { pub(crate) async fn fs_canonicalize( &self, params: FsCanonicalizeParams, - ) -> Result { + ) -> Result { self.require_initialized_for("filesystem")?; - let response = self.file_system.canonicalize(params).await?; - self.serialize_filesystem_response(&response, &[("path", Some(&response.path))]) + self.file_system.canonicalize(params).await } - pub(crate) async fn fs_join(&self, params: FsJoinParams) -> Result { + pub(crate) async fn fs_join( + &self, + params: FsJoinParams, + ) -> Result { self.require_initialized_for("filesystem")?; - let response = self.file_system.join(params).await?; - self.serialize_filesystem_response(&response, &[("path", Some(&response.path))]) + self.file_system.join(params).await } pub(crate) async fn fs_parent( &self, params: FsParentParams, - ) -> Result { + ) -> Result { self.require_initialized_for("filesystem")?; - let response = self.file_system.parent(params).await?; - self.serialize_filesystem_response(&response, &[("path", response.path.as_ref())]) + self.file_system.parent(params).await } pub(crate) async fn fs_read_directory( @@ -321,34 +310,6 @@ impl ExecServerHandler { self.file_system.copy(params).await } - fn serialize_filesystem_response( - &self, - response: &R, - path_fields: &[(&str, Option<&PathUri>)], - ) -> Result { - let mut result = to_value(response).map_err(|err| internal_error(err.to_string()))?; - if self.filesystem_path_uris.load(Ordering::Relaxed) { - return Ok(result); - } - - let fields = result.as_object_mut().ok_or_else(|| { - internal_error("filesystem response must serialize as an object".to_string()) - })?; - for (field, path) in path_fields { - let Some(path) = path else { - continue; - }; - let path = path - .to_abs_path() - .map_err(|err| internal_error(err.to_string()))?; - fields.insert( - (*field).to_string(), - to_value(path).map_err(|err| internal_error(err.to_string()))?, - ); - } - Ok(result) - } - fn require_initialized_for( &self, method_family: &str, diff --git a/codex-rs/exec-server/src/server/handler/tests.rs b/codex-rs/exec-server/src/server/handler/tests.rs index e72c92d90e47..6b632fe8909b 100644 --- a/codex-rs/exec-server/src/server/handler/tests.rs +++ b/codex-rs/exec-server/src/server/handler/tests.rs @@ -2,9 +2,7 @@ use std::collections::HashMap; use std::sync::Arc; use std::time::Duration; -use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; -use serde_json::json; use tokio::sync::mpsc; use uuid::Uuid; @@ -12,9 +10,7 @@ use super::ExecServerHandler; use crate::ExecServerRuntimePaths; use crate::ProcessId; use crate::protocol::ExecParams; -use crate::protocol::FsCanonicalizeParams; use crate::protocol::InitializeParams; -use crate::protocol::InitializeWireParams; use crate::protocol::ReadParams; use crate::protocol::ReadResponse; use crate::protocol::TerminateParams; @@ -22,12 +18,6 @@ use crate::protocol::TerminateResponse; use crate::rpc::RpcNotificationSender; use crate::server::session_registry::SessionRegistry; -#[derive(Clone, Copy)] -enum ClientPathFormat { - PathUri, - LegacyNative, -} - fn exec_params(process_id: &str) -> ExecParams { exec_params_with_argv(process_id, sleep_argv()) } @@ -86,12 +76,6 @@ fn test_runtime_paths() -> ExecServerRuntimePaths { } async fn initialized_handler() -> Arc { - initialized_handler_with_path_format(ClientPathFormat::PathUri).await -} - -async fn initialized_handler_with_path_format( - client_path_format: ClientPathFormat, -) -> Arc { let (outgoing_tx, _outgoing_rx) = mpsc::channel(16); let registry = SessionRegistry::new(); let handler = Arc::new(ExecServerHandler::new( @@ -100,50 +84,17 @@ async fn initialized_handler_with_path_format( test_runtime_paths(), )); let initialize_response = handler - .initialize(InitializeWireParams { - params: InitializeParams { - client_name: "exec-server-test".to_string(), - resume_session_id: None, - }, - filesystem_path_uris: matches!(client_path_format, ClientPathFormat::PathUri), + .initialize(InitializeParams { + client_name: "exec-server-test".to_string(), + resume_session_id: None, }) .await .expect("initialize"); - Uuid::parse_str(&initialize_response.response.session_id).expect("session id should be a UUID"); + Uuid::parse_str(&initialize_response.session_id).expect("session id should be a UUID"); handler.initialized().expect("initialized"); handler } -#[tokio::test] -async fn canonicalize_response_uses_negotiated_path_format() { - let temp_dir = tempfile::tempdir().expect("tempdir"); - let canonical_path = - std::fs::canonicalize(temp_dir.path()).expect("canonical tempdir should exist"); - let path = PathUri::from_path(temp_dir.path()).expect("tempdir path URI"); - let path_uri = PathUri::from_path(canonical_path).expect("canonical path URI"); - let legacy_path = path_uri - .to_abs_path() - .expect("legacy native path") - .to_string_lossy() - .into_owned(); - - for (client_path_format, expected_path) in [ - (ClientPathFormat::PathUri, path_uri.to_string()), - (ClientPathFormat::LegacyNative, legacy_path), - ] { - let handler = initialized_handler_with_path_format(client_path_format).await; - let response = handler - .fs_canonicalize(FsCanonicalizeParams { - path: path.clone(), - sandbox: None, - }) - .await - .expect("canonicalize response"); - assert_eq!(response, json!({ "path": expected_path })); - handler.shutdown().await; - } -} - #[tokio::test] async fn duplicate_process_ids_allow_only_one_successful_start() { let handler = initialized_handler().await; @@ -211,12 +162,9 @@ async fn long_poll_read_fails_after_session_resume() { test_runtime_paths(), )); let initialize_response = first_handler - .initialize(InitializeWireParams { - params: InitializeParams { - client_name: "exec-server-test".to_string(), - resume_session_id: None, - }, - filesystem_path_uris: true, + .initialize(InitializeParams { + client_name: "exec-server-test".to_string(), + resume_session_id: None, }) .await .expect("initialize"); @@ -254,12 +202,9 @@ async fn long_poll_read_fails_after_session_resume() { test_runtime_paths(), )); second_handler - .initialize(InitializeWireParams { - params: InitializeParams { - client_name: "exec-server-test".to_string(), - resume_session_id: Some(initialize_response.response.session_id), - }, - filesystem_path_uris: true, + .initialize(InitializeParams { + client_name: "exec-server-test".to_string(), + resume_session_id: Some(initialize_response.session_id), }) .await .expect("initialize second connection"); @@ -290,12 +235,9 @@ async fn active_session_resume_is_rejected() { test_runtime_paths(), )); let initialize_response = first_handler - .initialize(InitializeWireParams { - params: InitializeParams { - client_name: "exec-server-test".to_string(), - resume_session_id: None, - }, - filesystem_path_uris: true, + .initialize(InitializeParams { + client_name: "exec-server-test".to_string(), + resume_session_id: None, }) .await .expect("initialize"); @@ -307,12 +249,9 @@ async fn active_session_resume_is_rejected() { test_runtime_paths(), )); let err = second_handler - .initialize(InitializeWireParams { - params: InitializeParams { - client_name: "exec-server-test".to_string(), - resume_session_id: Some(initialize_response.response.session_id.clone()), - }, - filesystem_path_uris: true, + .initialize(InitializeParams { + client_name: "exec-server-test".to_string(), + resume_session_id: Some(initialize_response.session_id.clone()), }) .await .expect_err("active session resume should fail"); @@ -322,7 +261,7 @@ async fn active_session_resume_is_rejected() { err.message, format!( "session {} is already attached to another connection", - initialize_response.response.session_id + initialize_response.session_id ) ); @@ -338,12 +277,9 @@ async fn output_and_exit_are_retained_after_notification_receiver_closes() { test_runtime_paths(), )); handler - .initialize(InitializeWireParams { - params: InitializeParams { - client_name: "exec-server-test".to_string(), - resume_session_id: None, - }, - filesystem_path_uris: true, + .initialize(InitializeParams { + client_name: "exec-server-test".to_string(), + resume_session_id: None, }) .await .expect("initialize"); diff --git a/codex-rs/exec-server/src/server/registry.rs b/codex-rs/exec-server/src/server/registry.rs index f73c98d26d0f..1f652c1c4c3d 100644 --- a/codex-rs/exec-server/src/server/registry.rs +++ b/codex-rs/exec-server/src/server/registry.rs @@ -31,7 +31,7 @@ use crate::protocol::HTTP_REQUEST_METHOD; use crate::protocol::HttpRequestParams; use crate::protocol::INITIALIZE_METHOD; use crate::protocol::INITIALIZED_METHOD; -use crate::protocol::InitializeWireParams; +use crate::protocol::InitializeParams; use crate::protocol::ReadParams; use crate::protocol::SignalParams; use crate::protocol::TerminateParams; @@ -49,7 +49,7 @@ pub(crate) fn build_router() -> RpcRouter { ); router.request( INITIALIZE_METHOD, - |handler: Arc, params: InitializeWireParams| async move { + |handler: Arc, params: InitializeParams| async move { handler.initialize(params).await }, );