mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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.
This commit is contained in:
committed by
GitHub
Unverified
parent
5283522939
commit
c26f961b85
@@ -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
|
||||
|
||||
@@ -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::<FsReadFileParams>(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::<FsReadFileParams>(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]
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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::<PathUri>(&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::<PathUri>(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]
|
||||
|
||||
Reference in New Issue
Block a user