From c26f961b85b9f4eb0218af00a402ed1ab30f3618 Mon Sep 17 00:00:00 2001 From: "Adam Perry @ OpenAI" Date: Tue, 23 Jun 2026 14:47:00 -0700 Subject: [PATCH] path-uri: remove legacy path deserialization (#29158) ## Why I'd originally added `PathUri` legacy path deserialization thinking we'd want it for having `PathUri` in public app-server APIs. Since then we've added `LegacyAppPathString` to handle the messy conversions that we need for backcompat. It's confusing for `PathUri` to support deserializing legacy paths when we don't yet want to actually expose app-server callers or rollout storage to the new URI format. Stacked on top of #29472 to avoid breaking compatibility in case those types ended up stored somewhere for someone. ## What changed - Parse deserialized `PathUri` values exclusively as valid `file:` URIs. - Replace legacy acceptance coverage with rejection coverage for top-level filesystem paths and sandbox working directories. - Serialize CWDs in hand-built exec-server process requests as `PathUri` values. --- codex-rs/exec-server/README.md | 6 +-- codex-rs/exec-server/src/protocol.rs | 53 ++++++++++++--------------- codex-rs/exec-server/tests/process.rs | 9 +++-- codex-rs/utils/path-uri/src/lib.rs | 33 ++--------------- codex-rs/utils/path-uri/src/tests.rs | 15 +++----- 5 files changed, 41 insertions(+), 75 deletions(-) diff --git a/codex-rs/exec-server/README.md b/codex-rs/exec-server/README.md index 175097c96..6d2e663e2 100644 --- a/codex-rs/exec-server/README.md +++ b/codex-rs/exec-server/README.md @@ -340,9 +340,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: +Filesystem methods require valid `file:` URI strings and return JSON-RPC errors +for invalid or unavailable paths. Native absolute path strings are rejected; +callers must convert them to `file:` URIs before sending requests: - `fs/readFile` - `fs/open`, `fs/readBlock`, and `fs/close` (internal transport for diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 9c1a6398a..d53dae471 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -591,38 +591,33 @@ mod tests { } #[test] - fn filesystem_protocol_accepts_legacy_absolute_paths_and_serializes_path_uris() { - let legacy_path = std::env::current_dir() + fn filesystem_protocol_rejects_native_absolute_paths() { + let native_path = std::env::current_dir() .expect("current directory") - .join("legacy-file.txt"); - let legacy_cwd = std::env::current_dir().expect("current directory"); - let native_sandbox = FileSystemSandboxContext::from_permission_profile_with_cwd( - PermissionProfile::default(), - PathUri::from_host_native_path(&legacy_cwd).expect("cwd URI"), - ); - let mut legacy_sandbox = - serde_json::to_value(&native_sandbox).expect("sandbox should serialize"); - legacy_sandbox["cwd"] = serde_json::json!(legacy_cwd.to_string_lossy()); - let params: FsReadFileParams = serde_json::from_value(serde_json::json!({ - "path": legacy_path.to_string_lossy(), - "sandbox": legacy_sandbox, - })) - .expect("legacy absolute path should deserialize"); - let expected_sandbox = native_sandbox; - let expected = FsReadFileParams { - path: PathUri::from_host_native_path(legacy_path).expect("path URI"), - sandbox: Some(expected_sandbox.clone()), - }; + .join("native-file.txt"); + let native_cwd = std::env::current_dir().expect("current directory"); - assert_eq!(params, expected); - assert_eq!( - serde_json::to_value(params).expect("params should serialize"), - serde_json::json!({ - "path": expected.path.to_string(), - "sandbox": serde_json::to_value(expected_sandbox) - .expect("sandbox should serialize"), - }) + serde_json::from_value::(serde_json::json!({ + "path": native_path.to_string_lossy(), + "sandbox": null, + })) + .expect_err("native absolute path should not deserialize as a URI"); + + let sandbox = FileSystemSandboxContext::from_permission_profile_with_cwd( + PermissionProfile::default(), + PathUri::from_host_native_path(&native_cwd).expect("cwd URI"), ); + let mut native_path_sandbox = + serde_json::to_value(sandbox).expect("sandbox should serialize"); + native_path_sandbox["cwd"] = serde_json::json!(native_cwd.to_string_lossy()); + + serde_json::from_value::(serde_json::json!({ + "path": PathUri::from_host_native_path(native_path) + .expect("path URI") + .to_string(), + "sandbox": native_path_sandbox, + })) + .expect_err("native absolute sandbox cwd should not deserialize as a URI"); } #[test] diff --git a/codex-rs/exec-server/tests/process.rs b/codex-rs/exec-server/tests/process.rs index ba7f64a61..7a1da19ac 100644 --- a/codex-rs/exec-server/tests/process.rs +++ b/codex-rs/exec-server/tests/process.rs @@ -12,6 +12,7 @@ use codex_exec_server::ReadResponse; use codex_exec_server::TerminateResponse; use codex_exec_server::WriteResponse; use codex_exec_server::WriteStatus; +use codex_utils_path_uri::PathUri; use common::exec_server::exec_server; use pretty_assertions::assert_eq; @@ -46,7 +47,7 @@ async fn exec_server_starts_process_over_websocket() -> anyhow::Result<()> { serde_json::json!({ "processId": "proc-1", "argv": ["true"], - "cwd": std::env::current_dir()?, + "cwd": PathUri::from_host_native_path(std::env::current_dir()?)?, "env": {}, "tty": false, "pipeStdin": false, @@ -113,7 +114,7 @@ async fn exec_server_defaults_omitted_pipe_stdin_to_closed_stdin() -> anyhow::Re "-c", "sleep 0.3; if IFS= read -r line; then printf 'read:%s\\n' \"$line\"; else printf 'eof\\n'; fi" ], - "cwd": std::env::current_dir()?, + "cwd": PathUri::from_host_native_path(std::env::current_dir()?)?, "env": {}, "tty": false, "arg0": null @@ -207,7 +208,7 @@ async fn exec_server_dedupes_retried_process_write_ids() -> anyhow::Result<()> { "-c", "IFS= read -r first; printf 'line:%s\\n' \"$first\"; IFS= read -r second; printf 'line:%s\\n' \"$second\"" ], - "cwd": std::env::current_dir()?, + "cwd": PathUri::from_host_native_path(std::env::current_dir()?)?, "env": {}, "tty": false, "pipeStdin": true, @@ -341,7 +342,7 @@ async fn exec_server_resumes_detached_session_without_killing_processes() -> any serde_json::json!({ "processId": "proc-resume", "argv": ["/bin/sh", "-c", "sleep 5"], - "cwd": std::env::current_dir()?, + "cwd": PathUri::from_host_native_path(std::env::current_dir()?)?, "env": {}, "tty": false, "pipeStdin": false, diff --git a/codex-rs/utils/path-uri/src/lib.rs b/codex-rs/utils/path-uri/src/lib.rs index 650343209..bbbdaa16c 100644 --- a/codex-rs/utils/path-uri/src/lib.rs +++ b/codex-rs/utils/path-uri/src/lib.rs @@ -45,13 +45,9 @@ const BAD_PATH_URI_PREFIX: &str = "file:///%00/bad/path/"; /// are not resolved. /// /// Serde represents a `PathUri` as its canonical URI string. Deserialization -/// also accepts an absolute native path for compatibility with fields that -/// previously used [`AbsolutePathBuf`]; relative paths are rejected. Valid -/// `file:` strings round-trip through their canonical URL form, including -/// encoded non-UTF-8 path bytes, but conversion to a native path remains -/// host-dependent as described by [RFC 8089]. +/// accepts only valid `file:` URI strings. These strings round-trip through +/// their canonical URL form, including encoded non-UTF-8 path bytes. /// -/// [RFC 8089]: https://www.rfc-editor.org/rfc/rfc8089.html /// [VS Code resources]: https://github.com/microsoft/vscode/blob/main/src/vs/base/common/resources.ts #[derive(Clone, Debug, PartialEq, Eq, Hash, TS)] #[ts(type = "string")] @@ -478,30 +474,7 @@ impl<'de> Deserialize<'de> for PathUri { D: Deserializer<'de>, { let value = String::deserialize(deserializer)?; - let unsupported_scheme = match Url::parse(&value) { - Ok(url) => match Self::try_from(url) { - Ok(uri) => return Ok(uri), - // `Url` parses a Windows drive prefix such as `C:\` as the - // scheme `c`. Give any unsupported URI one chance to satisfy - // the native absolute-path invariant before reporting it. - Err(error @ PathUriParseError::UnsupportedScheme(_)) => Some(error), - Err(error) => return Err(serde::de::Error::custom(error)), - }, - Err(url::ParseError::RelativeUrlWithoutBase) => None, - Err(error) => { - return Err(serde::de::Error::custom(PathUriParseError::InvalidUri( - error, - ))); - } - }; - - let path = AbsolutePathBuf::from_absolute_path_checked(value).map_err(|path_error| { - serde::de::Error::custom( - unsupported_scheme - .map_or_else(|| path_error.to_string(), |error| error.to_string()), - ) - })?; - Ok(Self::from_abs_path(&path)) + Self::parse(&value).map_err(serde::de::Error::custom) } } diff --git a/codex-rs/utils/path-uri/src/tests.rs b/codex-rs/utils/path-uri/src/tests.rs index 1eead137f..8954b7ef4 100644 --- a/codex-rs/utils/path-uri/src/tests.rs +++ b/codex-rs/utils/path-uri/src/tests.rs @@ -1,5 +1,4 @@ use super::*; -use codex_utils_absolute_path::AbsolutePathBufGuard; use pretty_assertions::assert_eq; #[cfg(windows)] use std::ffi::OsString; @@ -418,14 +417,14 @@ fn path_uri_serializes_as_a_string() { } #[test] -fn path_uri_deserializes_legacy_absolute_paths() { +fn path_uri_rejects_native_absolute_paths_during_deserialization() { let path = AbsolutePathBuf::current_dir() .expect("current directory") .join("workspace/src"); let json = serde_json::to_string(&path).expect("absolute path should serialize"); - let uri: PathUri = serde_json::from_str(&json).expect("legacy absolute path should parse"); - assert_eq!(uri, PathUri::from_abs_path(&path)); + serde_json::from_str::(&json) + .expect_err("native absolute path should not deserialize as a URI"); } #[test] @@ -437,13 +436,11 @@ fn path_uri_rejects_relative_native_paths() { } #[test] -fn path_uri_rejects_legacy_relative_paths_with_absolute_path_guard() { - let base = AbsolutePathBuf::current_dir().expect("current directory"); - let _guard = AbsolutePathBufGuard::new(base.as_path()); +fn path_uri_rejects_relative_strings_during_deserialization() { let error = serde_json::from_str::(r#""src/lib.rs""#) - .expect_err("legacy relative path should be rejected"); + .expect_err("relative path should be rejected"); - assert!(error.to_string().contains("path is not absolute")); + assert!(error.to_string().contains("relative URL without a base")); } #[test]