diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 047f26d1ed..84500b5e55 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -682,7 +682,7 @@ Turns attach user input (text or images) to a thread and trigger Codex generatio - `{"type":"image","url":"https://…png"}` - `{"type":"localImage","path":"/tmp/screenshot.png"}` -You can optionally specify config overrides on the new turn. If specified, these settings become the default for subsequent turns on the same thread. `outputSchema` applies only to the current turn. Experimental `environments` is turn-scoped: omit it to inherit the thread's sticky environments, pass `[]` to run the turn with no environments, or pass explicit environment ids to override the sticky selection for this turn only. Each environment `cwd` must be absolute and use that environment's native path convention, so a Windows environment uses a value such as `C:\\workspace` even when app-server runs on Linux. +You can optionally specify config overrides on the new turn. If specified, these settings become the default for subsequent turns on the same thread. `outputSchema` applies only to the current turn. Experimental `environments` is turn-scoped: omit it to inherit the thread's sticky environments, pass `[]` to run the turn with no environments, or pass explicit environment ids to override the sticky selection for this turn only. Each environment `cwd` must be absolute and use that environment's native path convention, so a Windows environment uses a value such as `C:\\workspace` even when app-server runs on Linux. The path must also be portable to the selected executor; app-server rejects malformed paths and Windows device names before starting the thread or turn. `approvalsReviewer` accepts: diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index ce8623d8b2..516249579c 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -532,11 +532,13 @@ impl ThreadManager { "failed to resolve shell for environment `{environment_id}`: {err}" )) })?; - let cwd = cwd.to_path_uri(info.path_convention).map_err(|err| { - CodexErr::InvalidRequest(format!( - "invalid cwd for environment `{environment_id}`: {err}" - )) - })?; + let cwd = cwd + .to_normalized_path_uri(info.path_convention) + .map_err(|err| { + CodexErr::InvalidRequest(format!( + "invalid cwd for environment `{environment_id}`: {err}" + )) + })?; selections.push(TurnEnvironmentSelection { environment_id, cwd, diff --git a/codex-rs/core/src/thread_manager_tests.rs b/codex-rs/core/src/thread_manager_tests.rs index 71f9256959..fea3b35918 100644 --- a/codex-rs/core/src/thread_manager_tests.rs +++ b/codex-rs/core/src/thread_manager_tests.rs @@ -415,7 +415,26 @@ async fn native_environment_selections_reject_duplicate_ids() { assert_eq!( error.to_string(), format!( - "invalid cwd for environment `local`: path `relative` is not absolute using {} path syntax", + "invalid cwd for environment `local`: path `relative` is not a valid absolute {} path", + PathConvention::native() + ) + ); + + let invalid_native_cwd = match PathConvention::native() { + PathConvention::Posix => "/workspace/bad\0name", + PathConvention::Windows => r"C:\workspace\NUL.txt", + }; + let error = manager + .resolve_native_environment_selections([( + "local".to_string(), + ApiPathString::new(invalid_native_cwd), + )]) + .await + .expect_err("nonportable cwd must fail at the environment boundary"); + assert_eq!( + error.to_string(), + format!( + "invalid cwd for environment `local`: path `{invalid_native_cwd}` is not a valid absolute {} path", PathConvention::native() ) ); diff --git a/codex-rs/core/tests/remote_env_windows/wine_app_server_windows_exec_server_test.rs b/codex-rs/core/tests/remote_env_windows/wine_app_server_windows_exec_server_test.rs index 0d48ca2a8a..e2be06fb9d 100644 --- a/codex-rs/core/tests/remote_env_windows/wine_app_server_windows_exec_server_test.rs +++ b/codex-rs/core/tests/remote_env_windows/wine_app_server_windows_exec_server_test.rs @@ -15,6 +15,7 @@ use codex_app_server_protocol::AskForApproval; use codex_app_server_protocol::CommandExecutionStatus; use codex_app_server_protocol::ItemCompletedNotification; use codex_app_server_protocol::ItemStartedNotification; +use codex_app_server_protocol::JSONRPCError; use codex_app_server_protocol::JSONRPCResponse; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::SandboxPolicy; @@ -134,6 +135,34 @@ async fn exercise_app_server(websocket_url: String) -> Result<()> { .await?; timeout(APP_SERVER_TIMEOUT, app_server.initialize()).await??; + let invalid_cwd = r"C:\workspace\NUL.txt"; + let invalid_request_id = app_server + .send_thread_start_request(ThreadStartParams { + model: Some("mock-model".to_string()), + environments: Some(vec![TurnEnvironmentParams { + environment_id: REMOTE_ENVIRONMENT_ID.to_string(), + cwd: ApiPathString::new(invalid_cwd), + }]), + ..Default::default() + }) + .await?; + let invalid_response: JSONRPCError = timeout( + APP_SERVER_TIMEOUT, + app_server.read_stream_until_error_message(RequestId::Integer(invalid_request_id)), + ) + .await??; + assert_eq!(invalid_response.id, RequestId::Integer(invalid_request_id)); + assert_eq!( + invalid_response.error.message, + format!( + "invalid cwd for environment `{REMOTE_ENVIRONMENT_ID}`: path `{invalid_cwd}` is not a valid absolute Windows path" + ) + ); + assert!( + response_mock.requests().is_empty(), + "invalid environment cwd must fail before model inference" + ); + let environment = remote_windows_environment(); let thread_request_id = app_server .send_thread_start_request(ThreadStartParams { diff --git a/codex-rs/utils/path-uri/src/api_path_string.rs b/codex-rs/utils/path-uri/src/api_path_string.rs index ddb142b904..0597ca8987 100644 --- a/codex-rs/utils/path-uri/src/api_path_string.rs +++ b/codex-rs/utils/path-uri/src/api_path_string.rs @@ -105,6 +105,22 @@ impl ApiPathString { }) } + /// Parses this API string as an absolute, lexically normalized path URI. + /// + /// Unlike [`Self::to_path_uri`], this rejects opaque fallback forms and + /// Windows components that cannot name ordinary filesystem entries. Use + /// this at process-execution boundaries where the resulting URI must be + /// portable across hosts and renderable by the target executor. + pub fn to_normalized_path_uri( + &self, + convention: PathConvention, + ) -> Result { + match convention { + PathConvention::Posix => parse_resolved_posix_path(&self.0), + PathConvention::Windows => parse_resolved_windows_path(&self.0), + } + } + pub fn as_str(&self) -> &str { &self.0 } @@ -126,15 +142,7 @@ fn parse_posix_path(path: &str) -> Option { fn parse_windows_path(path: &str) -> Option { let bytes = path.as_bytes(); - let uses_namespace = matches!( - bytes, - [first, second, namespace @ (b'.' | b'?'), separator, ..] - if is_windows_separator_byte(*first) - && is_windows_separator_byte(*second) - && is_windows_separator_byte(*separator) - && matches!(*namespace, b'.' | b'?') - ); - if uses_namespace || path.contains('\0') { + if uses_windows_namespace(path) || path.contains('\0') { return Some(windows_opaque_path_uri(path)); } @@ -205,23 +213,24 @@ fn resolve_windows_path(base: &PathUri, path: &str) -> Result Result { + let native_path = path; let Some(path) = path.strip_prefix('/') else { return Err(invalid_native_path(path, PathConvention::Posix)); }; if path.contains('\0') { - return Err(invalid_native_path(path, PathConvention::Posix)); + return Err(invalid_native_path(native_path, PathConvention::Posix)); } build_resolved_path_uri( /*host*/ None, path.split('/'), /*protected_segments*/ 0, - path, + native_path, PathConvention::Posix, ) } fn parse_resolved_windows_path(path: &str) -> Result { - if path.contains('\0') { + if uses_windows_namespace(path) || path.contains('\0') { return Err(invalid_native_path(path, PathConvention::Windows)); } @@ -233,6 +242,9 @@ fn parse_resolved_windows_path(path: &str) -> Result Result bool { + matches!( + path.as_bytes(), + [first, second, namespace @ (b'.' | b'?'), separator, ..] + if is_windows_separator_byte(*first) + && is_windows_separator_byte(*second) + && is_windows_separator_byte(*separator) + && matches!(*namespace, b'.' | b'?') + ) +} + fn build_resolved_path_uri<'a>( host: Option<&str>, segments: impl IntoIterator, @@ -569,7 +592,7 @@ pub enum ApiPathStringError { path: String, convention: PathConvention, }, - #[error("path `{path}` is not absolute using {convention} path syntax")] + #[error("path `{path}` is not a valid absolute {convention} path")] InvalidNativePath { path: String, convention: PathConvention, diff --git a/codex-rs/utils/path-uri/src/api_path_string_tests.rs b/codex-rs/utils/path-uri/src/api_path_string_tests.rs index 1154ca84b3..52a065a6b7 100644 --- a/codex-rs/utils/path-uri/src/api_path_string_tests.rs +++ b/codex-rs/utils/path-uri/src/api_path_string_tests.rs @@ -356,6 +356,61 @@ fn rejects_relative_api_paths() { } } +#[test] +fn normalized_native_paths_are_host_independent() { + for (raw_path, convention, expected_uri) in [ + ( + "/workspace/../space #/%/é", + PathConvention::Posix, + "file:///space%20%23/%25/%C3%A9", + ), + ( + r"C:\workspace\..\space #\100%\é", + PathConvention::Windows, + "file:///C:/space%20%23/100%25/%C3%A9", + ), + ( + r"\\server\share\..\folder", + PathConvention::Windows, + "file://server/share/folder", + ), + ] { + assert_eq!( + ApiPathString::new(raw_path).to_normalized_path_uri(convention), + Ok(PathUri::parse(expected_uri).expect("expected path URI")), + "normalizing {raw_path:?}" + ); + } +} + +#[test] +fn normalized_native_paths_reject_nonportable_execution_paths() { + for (raw_path, convention) in [ + ("relative", PathConvention::Posix), + ("/workspace/bad\0name", PathConvention::Posix), + (r"C:relative", PathConvention::Windows), + (r"\root-relative", PathConvention::Windows), + (r"\\server", PathConvention::Windows), + (r"\\server\..\folder", PathConvention::Windows), + (r"\\server\.\folder", PathConvention::Windows), + (r"\\server\NUL\folder", PathConvention::Windows), + (r"C:\workspace\NUL.txt", PathConvention::Windows), + (r"C:\workspace\trailing.", PathConvention::Windows), + ("C:\\workspace\\bad\0name", PathConvention::Windows), + (r"\\?\C:\workspace", PathConvention::Windows), + ("/workspace", PathConvention::Windows), + ] { + assert_eq!( + ApiPathString::new(raw_path).to_normalized_path_uri(convention), + Err(ApiPathStringError::InvalidNativePath { + path: raw_path.to_string(), + convention, + }), + "normalizing {raw_path:?}" + ); + } +} + #[test] fn renders_an_absolute_path_using_the_explicit_convention() { #[cfg(unix)] @@ -504,6 +559,8 @@ fn native_resolution_rejects_reserved_windows_device_names() { r"C:\COM9.log", r"C:\lpt1", r"child\NUL.txt", + r"\\server\..\folder", + r"\\server\.\folder", ] { assert!( matches!(