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..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 @@ -1,123 +1,60 @@ #![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() { - let (websocket_url, captured_paths, server) = record_read_file_paths(2).await; +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; 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 +101,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 +125,6 @@ async fn complete_websocket_initialize(websocket: &mut WebSocketStream) -> JSONRPCMessage { loop { match timeout(Duration::from_secs(1), websocket.next()) @@ -212,7 +147,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..df708ffac709 100644 --- a/codex-rs/exec-server/src/sandboxed_file_system.rs +++ b/codex-rs/exec-server/src/sandboxed_file_system.rs @@ -58,18 +58,19 @@ impl ExecutorFileSystem for SandboxedFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult { let sandbox = require_platform_sandbox(sandbox)?; + validate_native_path(path)?; let response = self .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( @@ -78,11 +79,12 @@ impl ExecutorFileSystem for SandboxedFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult> { let sandbox = require_platform_sandbox(sandbox)?; + validate_native_path(path)?; let response = self .run_sandboxed( sandbox, FsHelperRequest::ReadFile(FsReadFileParams { - path: path.to_abs_path()?, + path: path.clone(), sandbox: None, }), ) @@ -104,10 +106,11 @@ 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 { - path: path.to_abs_path()?, + path: path.clone(), data_base64: STANDARD.encode(contents), sandbox: None, }), @@ -125,10 +128,11 @@ 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 { - path: path.to_abs_path()?, + path: path.clone(), recursive: Some(options.recursive), sandbox: None, }), @@ -145,11 +149,12 @@ impl ExecutorFileSystem for SandboxedFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult { let sandbox = require_platform_sandbox(sandbox)?; + validate_native_path(path)?; let response = self .run_sandboxed( sandbox, FsHelperRequest::GetMetadata(FsGetMetadataParams { - path: path.to_abs_path()?, + path: path.clone(), sandbox: None, }), ) @@ -171,11 +176,12 @@ impl ExecutorFileSystem for SandboxedFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult> { let sandbox = require_platform_sandbox(sandbox)?; + validate_native_path(path)?; let response = self .run_sandboxed( sandbox, FsHelperRequest::ReadDirectory(FsReadDirectoryParams { - path: path.to_abs_path()?, + path: path.clone(), sandbox: None, }), ) @@ -200,10 +206,11 @@ 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 { - path: path.to_abs_path()?, + path: path.clone(), recursive: Some(remove_options.recursive), force: Some(remove_options.force), sandbox: None, @@ -223,11 +230,13 @@ 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 { - 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, }), @@ -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/file_system_handler.rs b/codex-rs/exec-server/src/server/file_system_handler.rs index 97d028f89b7e..4ffdc1d9a9bd 100644 --- a/codex-rs/exec-server/src/server/file_system_handler.rs +++ b/codex-rs/exec-server/src/server/file_system_handler.rs @@ -53,10 +53,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 +67,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 +84,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 +99,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 +117,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 +130,9 @@ impl FileSystemHandler { params: FsJoinParams, ) -> Result { // TODO(anp): remove and migrate callers to PathUri. - let path = params.base_path.join(params.path); + 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 }) } @@ -145,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 }) } @@ -153,10 +156,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 +177,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 +192,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 +222,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 +251,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 +277,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 +299,7 @@ mod tests { } #[tokio::test] - async fn protocol_join_and_parent_remain_native_path_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"), @@ -310,8 +307,9 @@ mod tests { ) .expect("runtime paths"); let handler = FileSystemHandler::new(runtime_paths); - let base_path = + 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 { @@ -320,7 +318,10 @@ mod tests { }) .await .expect("join path"); - assert_eq!(joined.path, base_path.join("nested/file.txt")); + assert_eq!( + joined.path, + PathUri::from_abs_path(&native_base.join("nested/file.txt")).expect("joined path URI") + ); let parent = handler .parent(FsParentParams { @@ -328,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 }); } }