mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] migrate ExecutorFileSystem paths to PathUri (#27424)
## Why We're moving exec-server to use PathUri for its internal path representations. ## What Move `ExecutorFileSystem` APIs to use `PathUri` instead of `AbsolutePathBuf`. Future changes will convert higher-level parts of exec-server.
This commit is contained in:
committed by
GitHub
Unverified
parent
4a05d3b282
commit
b2a4e3be27
@@ -9,6 +9,7 @@ use codex_protocol::models::FileSystemPermissions;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy;
|
||||
use codex_sandboxing::policy_transforms::effective_network_sandbox_policy;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::path::Path;
|
||||
use tempfile::TempDir;
|
||||
@@ -61,7 +62,7 @@ async fn file_system_get_metadata_reports_files_and_directories(
|
||||
std::fs::create_dir(&directory_path)?;
|
||||
|
||||
let file_metadata = file_system
|
||||
.get_metadata(&absolute_path(&file_path), /*sandbox*/ None)
|
||||
.get_metadata(&PathUri::from_path(&file_path)?, /*sandbox*/ None)
|
||||
.await
|
||||
.with_context(|| format!("mode={implementation}"))?;
|
||||
assert_eq!(file_metadata.is_directory, false);
|
||||
@@ -70,7 +71,7 @@ async fn file_system_get_metadata_reports_files_and_directories(
|
||||
assert!(file_metadata.modified_at_ms > 0);
|
||||
|
||||
let directory_metadata = file_system
|
||||
.get_metadata(&absolute_path(&directory_path), /*sandbox*/ None)
|
||||
.get_metadata(&PathUri::from_path(&directory_path)?, /*sandbox*/ None)
|
||||
.await
|
||||
.with_context(|| format!("mode={implementation}"))?;
|
||||
assert_eq!(directory_metadata.is_directory, true);
|
||||
@@ -95,7 +96,7 @@ async fn file_system_create_directory_creates_nested_directories(
|
||||
|
||||
file_system
|
||||
.create_directory(
|
||||
&absolute_path(&nested_dir),
|
||||
&PathUri::from_path(&nested_dir)?,
|
||||
CreateDirectoryOptions { recursive: true },
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
@@ -119,7 +120,7 @@ async fn file_system_write_file_writes_bytes(
|
||||
let file_path = tmp.path().join("note.txt");
|
||||
file_system
|
||||
.write_file(
|
||||
&absolute_path(&file_path),
|
||||
&PathUri::from_path(&file_path)?,
|
||||
b"hello from trait".to_vec(),
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
@@ -130,42 +131,26 @@ async fn file_system_write_file_writes_bytes(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(FileSystemImplementation::Local ; "local")]
|
||||
#[test_case(FileSystemImplementation::Remote ; "remote")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn file_system_join_and_parent_preserve_lexical_paths(
|
||||
implementation: FileSystemImplementation,
|
||||
) -> Result<()> {
|
||||
let context = create_file_system_context(implementation).await?;
|
||||
let file_system = context.file_system;
|
||||
|
||||
#[test]
|
||||
fn path_uri_join_and_parent_preserve_lexical_paths() -> Result<()> {
|
||||
let tmp = TempDir::new()?;
|
||||
let source_dir = tmp.path().join("source");
|
||||
let joined_nested = file_system
|
||||
.join(&absolute_path(&source_dir), Path::new("nested/note.txt"))
|
||||
.await
|
||||
.with_context(|| format!("mode={implementation}"))?;
|
||||
let source_dir_uri = PathUri::from_path(&source_dir)?;
|
||||
let joined_nested = source_dir_uri.join("nested/note.txt")?;
|
||||
assert_eq!(
|
||||
joined_nested,
|
||||
absolute_path(source_dir.join("nested").join("note.txt"))
|
||||
PathUri::from_path(source_dir.join("nested").join("note.txt"))?
|
||||
);
|
||||
let joined_parent = file_system
|
||||
.parent(&joined_nested)
|
||||
.await
|
||||
.with_context(|| format!("mode={implementation}"))?;
|
||||
let joined_parent = joined_nested.parent();
|
||||
assert_eq!(
|
||||
joined_parent,
|
||||
Some(absolute_path(source_dir.join("nested")))
|
||||
Some(PathUri::from_path(source_dir.join("nested"))?)
|
||||
);
|
||||
let joined_parent_traversal = file_system
|
||||
.join(&absolute_path(&source_dir), Path::new("../outside"))
|
||||
.await
|
||||
.with_context(|| format!("mode={implementation}"))?;
|
||||
let joined_parent_traversal = source_dir_uri.join("../outside")?;
|
||||
assert_eq!(
|
||||
joined_parent_traversal,
|
||||
absolute_path(source_dir.join("../outside"))
|
||||
PathUri::from_path(source_dir.join("../outside"))?
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -183,7 +168,7 @@ async fn file_system_read_file_returns_bytes(
|
||||
std::fs::write(&file_path, "hello from trait")?;
|
||||
|
||||
let contents = file_system
|
||||
.read_file(&absolute_path(&file_path), /*sandbox*/ None)
|
||||
.read_file(&PathUri::from_path(&file_path)?, /*sandbox*/ None)
|
||||
.await
|
||||
.with_context(|| format!("mode={implementation}"))?;
|
||||
assert_eq!(contents, b"hello from trait");
|
||||
@@ -205,7 +190,7 @@ async fn file_system_read_file_text_returns_string(
|
||||
std::fs::write(&file_path, "hello from trait")?;
|
||||
|
||||
let contents = file_system
|
||||
.read_file_text(&absolute_path(&file_path), /*sandbox*/ None)
|
||||
.read_file_text(&PathUri::from_path(&file_path)?, /*sandbox*/ None)
|
||||
.await
|
||||
.with_context(|| format!("mode={implementation}"))?;
|
||||
assert_eq!(contents, "hello from trait");
|
||||
@@ -227,8 +212,8 @@ async fn file_system_copy_copies_file(implementation: FileSystemImplementation)
|
||||
|
||||
file_system
|
||||
.copy(
|
||||
&absolute_path(&source_file),
|
||||
&absolute_path(&copied_file),
|
||||
&PathUri::from_path(&source_file)?,
|
||||
&PathUri::from_path(&copied_file)?,
|
||||
CopyOptions { recursive: false },
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
@@ -258,8 +243,8 @@ async fn file_system_copy_copies_directory_recursively(
|
||||
|
||||
file_system
|
||||
.copy(
|
||||
&absolute_path(&source_dir),
|
||||
&absolute_path(&copied_dir),
|
||||
&PathUri::from_path(&source_dir)?,
|
||||
&PathUri::from_path(&copied_dir)?,
|
||||
CopyOptions { recursive: true },
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
@@ -288,7 +273,7 @@ async fn file_system_read_directory_lists_entries(
|
||||
std::fs::write(source_dir.join("root.txt"), "hello")?;
|
||||
|
||||
let mut entries = file_system
|
||||
.read_directory(&absolute_path(&source_dir), /*sandbox*/ None)
|
||||
.read_directory(&PathUri::from_path(&source_dir)?, /*sandbox*/ None)
|
||||
.await
|
||||
.with_context(|| format!("mode={implementation}"))?;
|
||||
entries.sort_by(|left, right| left.file_name.cmp(&right.file_name));
|
||||
@@ -326,7 +311,7 @@ async fn file_system_remove_removes_directory(
|
||||
|
||||
file_system
|
||||
.remove(
|
||||
&absolute_path(&directory_path),
|
||||
&PathUri::from_path(&directory_path)?,
|
||||
RemoveOptions {
|
||||
recursive: true,
|
||||
force: true,
|
||||
@@ -354,7 +339,7 @@ async fn file_system_write_file_reports_missing_parent(
|
||||
|
||||
let error = match file_system
|
||||
.write_file(
|
||||
&absolute_path(&missing_parent_path),
|
||||
&PathUri::from_path(&missing_parent_path)?,
|
||||
b"hello from trait".to_vec(),
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
@@ -388,8 +373,8 @@ async fn file_system_copy_rejects_directory_without_recursive(
|
||||
|
||||
let error = file_system
|
||||
.copy(
|
||||
&absolute_path(&source_dir),
|
||||
&absolute_path(tmp.path().join("dest")),
|
||||
&PathUri::from_path(&source_dir)?,
|
||||
&PathUri::from_path(tmp.path().join("dest"))?,
|
||||
CopyOptions { recursive: false },
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
@@ -424,7 +409,7 @@ async fn file_system_sandboxed_read_allows_readable_root(
|
||||
let sandbox = read_only_sandbox(allowed_dir);
|
||||
|
||||
let contents = file_system
|
||||
.read_file(&absolute_path(&file_path), Some(&sandbox))
|
||||
.read_file(&PathUri::from_path(&file_path)?, Some(&sandbox))
|
||||
.await
|
||||
.with_context(|| format!("mode={implementation}"))?;
|
||||
assert_eq!(contents, b"sandboxed hello");
|
||||
@@ -448,8 +433,8 @@ pub(crate) async fn assert_canonicalize_resolves_directory_alias(
|
||||
std::fs::write(&file_path, "canonical hello")?;
|
||||
create_directory_alias(&source_dir, &alias_dir)?;
|
||||
|
||||
let requested_path = absolute_path(alias_dir.join("nested").join("note.txt"));
|
||||
let expected_path = absolute_path(std::fs::canonicalize(&file_path)?);
|
||||
let requested_path = PathUri::from_path(alias_dir.join("nested").join("note.txt"))?;
|
||||
let expected_path = PathUri::from_path(std::fs::canonicalize(&file_path)?)?;
|
||||
assert_ne!(requested_path, expected_path);
|
||||
|
||||
let canonical_path = file_system
|
||||
@@ -478,8 +463,8 @@ pub(crate) async fn assert_sandboxed_canonicalize_resolves_directory_alias(
|
||||
create_directory_alias(&source_dir, &alias_dir)?;
|
||||
let sandbox = read_only_sandbox(tmp.path().to_path_buf());
|
||||
|
||||
let requested_path = absolute_path(alias_dir.join("nested").join("note.txt"));
|
||||
let expected_path = absolute_path(std::fs::canonicalize(&file_path)?);
|
||||
let requested_path = PathUri::from_path(alias_dir.join("nested").join("note.txt"))?;
|
||||
let expected_path = PathUri::from_path(std::fs::canonicalize(&file_path)?)?;
|
||||
assert_ne!(requested_path, expected_path);
|
||||
|
||||
let canonical_path = file_system
|
||||
@@ -532,7 +517,7 @@ async fn file_system_sandboxed_write_allows_additional_write_root(
|
||||
|
||||
file_system
|
||||
.write_file(
|
||||
&absolute_path(&file_path),
|
||||
&PathUri::from_path(&file_path)?,
|
||||
b"created".to_vec(),
|
||||
Some(&sandbox),
|
||||
)
|
||||
@@ -558,8 +543,8 @@ async fn file_system_copy_rejects_copying_directory_into_descendant(
|
||||
|
||||
let error = file_system
|
||||
.copy(
|
||||
&absolute_path(&source_dir),
|
||||
&absolute_path(source_dir.join("nested").join("copy")),
|
||||
&PathUri::from_path(&source_dir)?,
|
||||
&PathUri::from_path(source_dir.join("nested").join("copy"))?,
|
||||
CopyOptions { recursive: true },
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
|
||||
@@ -1,5 +1,4 @@
|
||||
use std::fmt;
|
||||
use std::path::Path;
|
||||
use std::sync::Arc;
|
||||
|
||||
use anyhow::Result;
|
||||
@@ -71,8 +70,7 @@ pub(crate) async fn create_file_system_context(
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn absolute_path(path: impl AsRef<Path>) -> AbsolutePathBuf {
|
||||
let path = path.as_ref().to_path_buf();
|
||||
pub(crate) fn absolute_path(path: std::path::PathBuf) -> AbsolutePathBuf {
|
||||
assert!(
|
||||
path.is_absolute(),
|
||||
"path must be absolute: {}",
|
||||
|
||||
@@ -22,6 +22,7 @@ use codex_exec_server::CreateDirectoryOptions;
|
||||
#[cfg(target_os = "linux")]
|
||||
use codex_exec_server::Environment;
|
||||
use codex_exec_server::RemoveOptions;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::TempDir;
|
||||
use test_case::test_case;
|
||||
@@ -30,7 +31,6 @@ use test_case::test_case;
|
||||
use crate::common::exec_server::exec_server_with_env;
|
||||
|
||||
use crate::support::FileSystemImplementation;
|
||||
use crate::support::absolute_path;
|
||||
use crate::support::create_file_system_context;
|
||||
use crate::support::read_only_sandbox;
|
||||
use crate::support::workspace_write_sandbox;
|
||||
@@ -185,7 +185,7 @@ async fn sandboxed_file_system_helper_finds_bwrap_on_preserved_path() -> Result<
|
||||
|
||||
file_system
|
||||
.write_file(
|
||||
&absolute_path(&file_path),
|
||||
&PathUri::from_path(&file_path)?,
|
||||
b"written through fs helper".to_vec(),
|
||||
Some(&sandbox),
|
||||
)
|
||||
@@ -219,7 +219,7 @@ async fn file_system_get_metadata_reports_symlink_targets(
|
||||
let symlink_path = tmp.path().join("note-link.txt");
|
||||
symlink(&file_path, &symlink_path)?;
|
||||
let symlink_metadata = file_system
|
||||
.get_metadata(&absolute_path(&symlink_path), /*sandbox*/ None)
|
||||
.get_metadata(&PathUri::from_path(&symlink_path)?, /*sandbox*/ None)
|
||||
.await
|
||||
.with_context(|| format!("mode={implementation}"))?;
|
||||
assert_eq!(symlink_metadata.is_directory, false);
|
||||
@@ -232,7 +232,10 @@ async fn file_system_get_metadata_reports_symlink_targets(
|
||||
let dir_symlink_path = tmp.path().join("notes-link");
|
||||
symlink(&dir_path, &dir_symlink_path)?;
|
||||
let dir_symlink_metadata = file_system
|
||||
.get_metadata(&absolute_path(&dir_symlink_path), /*sandbox*/ None)
|
||||
.get_metadata(
|
||||
&PathUri::from_path(&dir_symlink_path)?,
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.await
|
||||
.with_context(|| format!("mode={implementation}"))?;
|
||||
assert_eq!(dir_symlink_metadata.is_directory, true);
|
||||
@@ -257,7 +260,7 @@ async fn file_system_sandboxed_write_rejects_unwritable_path(
|
||||
let sandbox = read_only_sandbox(tmp.path().to_path_buf());
|
||||
let error = match file_system
|
||||
.write_file(
|
||||
&absolute_path(&blocked_path),
|
||||
&PathUri::from_path(&blocked_path)?,
|
||||
b"nope".to_vec(),
|
||||
Some(&sandbox),
|
||||
)
|
||||
@@ -293,7 +296,7 @@ async fn file_system_sandboxed_write_allows_explicit_alias_roots(
|
||||
|
||||
file_system
|
||||
.write_file(
|
||||
&absolute_path(&file_path),
|
||||
&PathUri::from_path(&file_path)?,
|
||||
b"created".to_vec(),
|
||||
Some(&sandbox),
|
||||
)
|
||||
@@ -324,7 +327,7 @@ async fn file_system_sandboxed_read_rejects_symlink_escape(
|
||||
let requested_path = allowed_dir.join("link").join("secret.txt");
|
||||
let sandbox = read_only_sandbox(allowed_dir);
|
||||
let error = match file_system
|
||||
.read_file(&absolute_path(&requested_path), Some(&sandbox))
|
||||
.read_file(&PathUri::from_path(&requested_path)?, Some(&sandbox))
|
||||
.await
|
||||
{
|
||||
Ok(_) => anyhow::bail!("read should be blocked"),
|
||||
@@ -353,13 +356,14 @@ async fn file_system_sandboxed_read_rejects_symlink_parent_dotdot_escape(
|
||||
std::fs::write(&secret_path, "nope")?;
|
||||
symlink(&outside_dir, allowed_dir.join("link"))?;
|
||||
|
||||
let requested_path = absolute_path(allowed_dir.join("link").join("..").join("secret.txt"));
|
||||
let requested_path =
|
||||
PathUri::from_path(allowed_dir.join("link").join("..").join("secret.txt"))?;
|
||||
let sandbox = read_only_sandbox(allowed_dir);
|
||||
let error = match file_system.read_file(&requested_path, Some(&sandbox)).await {
|
||||
Ok(_) => anyhow::bail!("read should fail after path normalization"),
|
||||
Err(error) => error,
|
||||
};
|
||||
// AbsolutePathBuf normalizes `link/../secret.txt` to
|
||||
// PathUri's native path constructor normalizes `link/../secret.txt` to
|
||||
// `allowed/secret.txt` before the request reaches the filesystem layer.
|
||||
// Depending on whether the platform/runtime resolves that normalized path
|
||||
// through a top-level symlink alias, the request can surface as either
|
||||
@@ -389,7 +393,7 @@ async fn file_system_sandboxed_write_rejects_symlink_escape(
|
||||
let sandbox = workspace_write_sandbox(allowed_dir);
|
||||
let error = match file_system
|
||||
.write_file(
|
||||
&absolute_path(&requested_path),
|
||||
&PathUri::from_path(&requested_path)?,
|
||||
b"nope".to_vec(),
|
||||
Some(&sandbox),
|
||||
)
|
||||
@@ -427,7 +431,7 @@ async fn file_system_sandboxed_write_preserves_existing_hard_link(
|
||||
let sandbox = workspace_write_sandbox(allowed_dir);
|
||||
file_system
|
||||
.write_file(
|
||||
&absolute_path(&hard_link),
|
||||
&PathUri::from_path(&hard_link)?,
|
||||
b"updated through existing hard link\n".to_vec(),
|
||||
Some(&sandbox),
|
||||
)
|
||||
@@ -473,7 +477,7 @@ async fn file_system_create_directory_rejects_symlink_escape(
|
||||
let sandbox = workspace_write_sandbox(allowed_dir);
|
||||
let error = match file_system
|
||||
.create_directory(
|
||||
&absolute_path(&requested_path),
|
||||
&PathUri::from_path(&requested_path)?,
|
||||
CreateDirectoryOptions { recursive: false },
|
||||
Some(&sandbox),
|
||||
)
|
||||
@@ -508,7 +512,7 @@ async fn file_system_read_directory_rejects_symlink_escape(
|
||||
let requested_path = allowed_dir.join("link");
|
||||
let sandbox = read_only_sandbox(allowed_dir);
|
||||
let error = match file_system
|
||||
.read_directory(&absolute_path(&requested_path), Some(&sandbox))
|
||||
.read_directory(&PathUri::from_path(&requested_path)?, Some(&sandbox))
|
||||
.await
|
||||
{
|
||||
Ok(_) => anyhow::bail!("read_directory should be blocked"),
|
||||
@@ -540,8 +544,8 @@ async fn file_system_copy_rejects_symlink_escape_destination(
|
||||
let sandbox = workspace_write_sandbox(allowed_dir.clone());
|
||||
let error = match file_system
|
||||
.copy(
|
||||
&absolute_path(allowed_dir.join("source.txt")),
|
||||
&absolute_path(&requested_destination),
|
||||
&PathUri::from_path(allowed_dir.join("source.txt"))?,
|
||||
&PathUri::from_path(&requested_destination)?,
|
||||
CopyOptions { recursive: false },
|
||||
Some(&sandbox),
|
||||
)
|
||||
@@ -578,7 +582,7 @@ async fn file_system_remove_removes_symlink_not_target(
|
||||
let sandbox = workspace_write_sandbox(allowed_dir);
|
||||
file_system
|
||||
.remove(
|
||||
&absolute_path(&symlink_path),
|
||||
&PathUri::from_path(&symlink_path)?,
|
||||
RemoveOptions {
|
||||
recursive: false,
|
||||
force: false,
|
||||
@@ -618,8 +622,8 @@ async fn file_system_copy_preserves_symlink_source(
|
||||
let sandbox = workspace_write_sandbox(allowed_dir.clone());
|
||||
file_system
|
||||
.copy(
|
||||
&absolute_path(&source_symlink),
|
||||
&absolute_path(&copied_symlink),
|
||||
&PathUri::from_path(&source_symlink)?,
|
||||
&PathUri::from_path(&copied_symlink)?,
|
||||
CopyOptions { recursive: false },
|
||||
Some(&sandbox),
|
||||
)
|
||||
@@ -655,7 +659,7 @@ async fn file_system_remove_rejects_symlink_escape(
|
||||
let sandbox = workspace_write_sandbox(allowed_dir);
|
||||
let error = match file_system
|
||||
.remove(
|
||||
&absolute_path(&requested_path),
|
||||
&PathUri::from_path(&requested_path)?,
|
||||
RemoveOptions {
|
||||
recursive: false,
|
||||
force: false,
|
||||
@@ -696,8 +700,8 @@ async fn file_system_copy_rejects_symlink_escape_source(
|
||||
let sandbox = workspace_write_sandbox(allowed_dir);
|
||||
let error = match file_system
|
||||
.copy(
|
||||
&absolute_path(&requested_source),
|
||||
&absolute_path(&requested_destination),
|
||||
&PathUri::from_path(&requested_source)?,
|
||||
&PathUri::from_path(&requested_destination)?,
|
||||
CopyOptions { recursive: false },
|
||||
Some(&sandbox),
|
||||
)
|
||||
@@ -730,8 +734,8 @@ async fn file_system_copy_preserves_symlinks_in_recursive_copy(
|
||||
|
||||
file_system
|
||||
.copy(
|
||||
&absolute_path(&source_dir),
|
||||
&absolute_path(&copied_dir),
|
||||
&PathUri::from_path(&source_dir)?,
|
||||
&PathUri::from_path(&copied_dir)?,
|
||||
CopyOptions { recursive: true },
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
@@ -776,8 +780,8 @@ async fn file_system_copy_ignores_unknown_special_files_in_recursive_copy(
|
||||
|
||||
file_system
|
||||
.copy(
|
||||
&absolute_path(&source_dir),
|
||||
&absolute_path(&copied_dir),
|
||||
&PathUri::from_path(&source_dir)?,
|
||||
&PathUri::from_path(&copied_dir)?,
|
||||
CopyOptions { recursive: true },
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
@@ -815,8 +819,8 @@ async fn file_system_copy_rejects_standalone_fifo_source(
|
||||
|
||||
let error = file_system
|
||||
.copy(
|
||||
&absolute_path(&fifo_path),
|
||||
&absolute_path(tmp.path().join("copied")),
|
||||
&PathUri::from_path(&fifo_path)?,
|
||||
&PathUri::from_path(tmp.path().join("copied"))?,
|
||||
CopyOptions { recursive: false },
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user