diff --git a/codex-rs/config/src/loader/tests.rs b/codex-rs/config/src/loader/tests.rs index 4c9847cf4c..30cdd4be22 100644 --- a/codex-rs/config/src/loader/tests.rs +++ b/codex-rs/config/src/loader/tests.rs @@ -10,6 +10,8 @@ use codex_file_system::GetMetadataOptions; use codex_file_system::ReadDirectoryEntry; use codex_file_system::ReadFileOptions; use codex_file_system::RemoveOptions; +use codex_file_system::WalkOptions; +use codex_file_system::WalkOutcome; use codex_file_system::WriteFileOptions; use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; @@ -140,6 +142,15 @@ impl ExecutorFileSystem for TestFileSystem { Box::pin(async move { unimplemented!("test filesystem only supports reads") }) } + fn walk<'a>( + &'a self, + _path: &'a PathUri, + _options: WalkOptions, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, WalkOutcome> { + unimplemented!() + } + fn remove<'a>( &'a self, _path: &'a PathUri, diff --git a/codex-rs/core-plugins/src/provider_tests.rs b/codex-rs/core-plugins/src/provider_tests.rs index 51734741c3..612f22c8e9 100644 --- a/codex-rs/core-plugins/src/provider_tests.rs +++ b/codex-rs/core-plugins/src/provider_tests.rs @@ -16,6 +16,8 @@ use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_exec_server::ReadDirectoryEntry; use codex_exec_server::ReadFileOptions; use codex_exec_server::RemoveOptions; +use codex_exec_server::WalkOptions; +use codex_exec_server::WalkOutcome; use codex_exec_server::WriteFileOptions; use codex_exec_server_test_support::environment_manager_without_environments; use codex_plugin::PluginProvider; @@ -157,6 +159,15 @@ impl ExecutorFileSystem for SyntheticPluginFileSystem { Box::pin(async { Self::unsupported() }) } + fn walk<'a>( + &'a self, + _path: &'a PathUri, + _options: WalkOptions, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, WalkOutcome> { + Box::pin(async { Self::unsupported() }) + } + fn remove<'a>( &'a self, _path: &'a PathUri, diff --git a/codex-rs/core/src/agents_md_tests.rs b/codex-rs/core/src/agents_md_tests.rs index 8e889a1990..c4c77ff2a5 100644 --- a/codex-rs/core/src/agents_md_tests.rs +++ b/codex-rs/core/src/agents_md_tests.rs @@ -22,6 +22,8 @@ use codex_exec_server::LOCAL_FS; use codex_exec_server::ReadDirectoryEntry; use codex_exec_server::ReadFileOptions; use codex_exec_server::RemoveOptions; +use codex_exec_server::WalkOptions; +use codex_exec_server::WalkOutcome; use codex_exec_server::WriteFileOptions; use codex_extension_api::UserInstructions; use codex_features::Feature; @@ -272,6 +274,15 @@ impl ExecutorFileSystem for FailingFileSystem { Box::pin(FailingFileSystem::read_directory(self, path, sandbox)) } + fn walk<'a>( + &'a self, + _path: &'a PathUri, + _options: WalkOptions, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, WalkOutcome> { + Box::pin(async { unreachable!("walk should not be called") }) + } + fn remove<'a>( &'a self, path: &'a PathUri, diff --git a/codex-rs/core/src/git_info_tests.rs b/codex-rs/core/src/git_info_tests.rs index e8ba696981..05bc54a9ad 100644 --- a/codex-rs/core/src/git_info_tests.rs +++ b/codex-rs/core/src/git_info_tests.rs @@ -11,6 +11,8 @@ use codex_exec_server::LOCAL_FS; use codex_exec_server::ReadDirectoryEntry; use codex_exec_server::ReadFileOptions; use codex_exec_server::RemoveOptions; +use codex_exec_server::WalkOptions; +use codex_exec_server::WalkOutcome; use codex_exec_server::WriteFileOptions; use codex_git_utils::GitInfo; use codex_git_utils::GitSha; @@ -140,6 +142,15 @@ impl ExecutorFileSystem for MetadataOverrideFileSystem { Box::pin(async { Self::unsupported() }) } + fn walk<'a>( + &'a self, + _path: &'a PathUri, + _options: WalkOptions, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, WalkOutcome> { + Box::pin(async { Self::unsupported() }) + } + fn remove<'a>( &'a self, _path: &'a PathUri, diff --git a/codex-rs/exec-server/src/local_file_system.rs b/codex-rs/exec-server/src/local_file_system.rs index c1cd76061b..7f33b5a697 100644 --- a/codex-rs/exec-server/src/local_file_system.rs +++ b/codex-rs/exec-server/src/local_file_system.rs @@ -1,5 +1,12 @@ +use codex_file_system::MAX_WALK_DEPTH; +use codex_file_system::MAX_WALK_DIRECTORIES; +use codex_file_system::MAX_WALK_ENTRIES; +use codex_file_system::MAX_WALK_RESPONSE_BYTES; +use codex_file_system::WALK_RESPONSE_ITEM_OVERHEAD_BYTES; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; +use std::collections::HashSet; +use std::collections::VecDeque; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; @@ -9,6 +16,7 @@ use std::time::UNIX_EPOCH; use tokio::io; use tokio::io::AsyncReadExt; use tokio_util::io::ReaderStream; +use tokio_util::sync::CancellationToken; use crate::CopyOptions; use crate::CreateDirectoryOptions; @@ -24,6 +32,9 @@ use crate::GetMetadataOptions; use crate::ReadDirectoryEntry; use crate::ReadFileOptions; use crate::RemoveOptions; +use crate::WalkEntry; +use crate::WalkEntryKind; +use crate::WalkError; use crate::WalkOptions; use crate::WalkOutcome; use crate::WriteFileOptions; @@ -506,6 +517,18 @@ impl ExecutorFileSystem for UnsandboxedFileSystem { Box::pin(UnsandboxedFileSystem::read_directory(self, path, sandbox)) } + fn walk<'a>( + &'a self, + path: &'a PathUri, + options: WalkOptions, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, WalkOutcome> { + Box::pin(async move { + reject_platform_sandbox_context(sandbox)?; + self.file_system.walk(path, options, /*sandbox*/ None).await + }) + } + fn remove<'a>( &'a self, path: &'a PathUri, @@ -677,6 +700,165 @@ impl DirectFileSystem { Ok(entries) } + fn sync_walk( + root: &PathUri, + options: WalkOptions, + cancelled: &CancellationToken, + ) -> io::Result { + if options.max_directories == 0 || options.max_entries == 0 { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "filesystem walk limits must be greater than zero", + )); + } + if options.max_depth > MAX_WALK_DEPTH + || options.max_directories > MAX_WALK_DIRECTORIES + || options.max_entries > MAX_WALK_ENTRIES + { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!( + "filesystem walk limits exceed maximums: depth={MAX_WALK_DEPTH}, directories={MAX_WALK_DIRECTORIES}, entries={MAX_WALK_ENTRIES}" + ), + )); + } + + check_walk_cancelled(cancelled)?; + let (root_metadata, root_is_symlink) = walk_metadata(root)?; + if !root_metadata.is_dir() || (root_is_symlink && !options.follow_directory_symlinks) { + return Ok(WalkOutcome::default()); + } + + let root_identity = if options.follow_directory_symlinks { + check_walk_cancelled(cancelled)?; + walk_canonicalize(root)? + } else { + root.clone() + }; + let mut outcome = WalkOutcome::default(); + let mut queue = VecDeque::from([(root.clone(), 0usize)]); + let mut visited_directories = HashSet::from([root_identity]); + let mut directory_count = 1usize; + let mut entry_count = 0usize; + let mut response_bytes = 0usize; + + while let Some((directory, depth)) = queue.pop_front() { + let entries = walk_read_directory(&directory, cancelled); + check_walk_cancelled(cancelled)?; + let mut entries = match entries { + Ok(entries) => entries, + Err(error) => { + if !push_walk_error( + &mut outcome, + &mut response_bytes, + directory, + error.to_string(), + ) { + return Ok(outcome); + } + continue; + } + }; + entries.sort(); + + for file_name in entries { + check_walk_cancelled(cancelled)?; + if entry_count == options.max_entries { + outcome.truncated = true; + return Ok(outcome); + } + entry_count += 1; + + let path = match directory.join(&file_name) { + Ok(path) => path, + Err(error) => { + if !push_walk_error( + &mut outcome, + &mut response_bytes, + directory.clone(), + error.to_string(), + ) { + return Ok(outcome); + } + continue; + } + }; + let (metadata, is_symlink) = match walk_metadata(&path) { + Ok(metadata) => metadata, + Err(error) => { + if !push_walk_error( + &mut outcome, + &mut response_bytes, + path, + error.to_string(), + ) { + return Ok(outcome); + } + continue; + } + }; + if is_symlink && (!options.follow_directory_symlinks || !metadata.is_dir()) { + continue; + } + + let kind = if metadata.is_dir() { + WalkEntryKind::Directory + } else if metadata.is_file() { + WalkEntryKind::File + } else { + continue; + }; + if !reserve_walk_response_bytes( + &mut outcome, + &mut response_bytes, + path.to_string().len(), + ) { + return Ok(outcome); + } + outcome.entries.push(WalkEntry { + path: path.clone(), + kind, + }); + + if kind == WalkEntryKind::Directory && depth < options.max_depth { + if options.prune_hidden_directories && file_name.starts_with('.') { + continue; + } + let directory_identity = if options.follow_directory_symlinks { + check_walk_cancelled(cancelled)?; + match walk_canonicalize(&path) { + Ok(path) => path, + Err(error) => { + if !push_walk_error( + &mut outcome, + &mut response_bytes, + path, + error.to_string(), + ) { + return Ok(outcome); + } + continue; + } + } + } else { + path.clone() + }; + if !visited_directories.insert(directory_identity) { + continue; + } + if directory_count == options.max_directories { + outcome.truncated = true; + } else { + directory_count += 1; + queue.push_back((path, depth + 1)); + } + } + } + } + + Ok(outcome) + } + async fn remove( &self, path: &PathUri, @@ -827,6 +1009,23 @@ impl ExecutorFileSystem for DirectFileSystem { Box::pin(DirectFileSystem::read_directory(self, path, sandbox)) } + fn walk<'a>( + &'a self, + path: &'a PathUri, + options: WalkOptions, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, WalkOutcome> { + Box::pin(async move { + reject_sandbox_context(sandbox)?; + let path = path.clone(); + let cancelled = CancellationToken::new(); + let _cancel_on_drop = cancelled.clone().drop_guard(); + tokio::task::spawn_blocking(move || Self::sync_walk(&path, options, &cancelled)) + .await + .map_err(|err| io::Error::other(format!("filesystem task failed: {err}")))? + }) + } + fn remove<'a>( &'a self, path: &'a PathUri, @@ -853,6 +1052,89 @@ impl ExecutorFileSystem for DirectFileSystem { } } +fn check_walk_cancelled(cancelled: &CancellationToken) -> io::Result<()> { + if cancelled.is_cancelled() { + return Err(io::Error::new( + io::ErrorKind::Interrupted, + "filesystem walk cancelled", + )); + } + Ok(()) +} + +fn walk_metadata(path: &PathUri) -> io::Result<(std::fs::Metadata, bool)> { + let path = path.to_abs_path()?; + let metadata = std::fs::symlink_metadata(path.as_path())?; + let is_symlink = metadata.is_symlink(); + let metadata = if is_symlink { + std::fs::metadata(path.as_path())? + } else { + metadata + }; + Ok((metadata, is_symlink)) +} + +fn walk_canonicalize(path: &PathUri) -> io::Result { + let path = path.to_abs_path()?; + let canonicalized = + AbsolutePathBuf::from_absolute_path(std::fs::canonicalize(path.as_path())?)?; + Ok(PathUri::from_abs_path(&canonicalized)) +} + +fn walk_read_directory(path: &PathUri, cancelled: &CancellationToken) -> io::Result> { + check_walk_cancelled(cancelled)?; + let path = path.to_abs_path()?; + let mut entries = Vec::new(); + for entry in std::fs::read_dir(path.as_path())? { + check_walk_cancelled(cancelled)?; + let entry = entry?; + let Ok(file_type) = entry.file_type() else { + continue; + }; + // Match DirectFileSystem::read_directory: omit broken or inaccessible links. + if file_type.is_symlink() { + check_walk_cancelled(cancelled)?; + if std::fs::metadata(entry.path()).is_err() { + continue; + } + } + entries.push(entry.file_name().to_string_lossy().into_owned()); + } + Ok(entries) +} + +fn push_walk_error( + outcome: &mut WalkOutcome, + response_bytes: &mut usize, + path: PathUri, + message: String, +) -> bool { + let item_bytes = path.to_string().len().saturating_add(message.len()); + if !reserve_walk_response_bytes(outcome, response_bytes, item_bytes) { + return false; + } + outcome.errors.push(WalkError { path, message }); + true +} + +fn reserve_walk_response_bytes( + outcome: &mut WalkOutcome, + response_bytes: &mut usize, + content_bytes: usize, +) -> bool { + let item_bytes = content_bytes.saturating_add(WALK_RESPONSE_ITEM_OVERHEAD_BYTES); + let Some(total_bytes) = response_bytes.checked_add(item_bytes) else { + outcome.truncated = true; + return false; + }; + if total_bytes > MAX_WALK_RESPONSE_BYTES { + outcome.truncated = true; + return false; + } + *response_bytes = total_bytes; + true +} + fn reject_sandbox_context(sandbox: Option<&FileSystemSandboxContext>) -> io::Result<()> { if sandbox.is_some() { return Err(io::Error::new( @@ -1039,3 +1321,103 @@ mod tests { Ok(()) } } + +#[cfg(test)] +mod walk_tests { + use super::*; + use codex_protocol::models::PermissionProfile; + use codex_protocol::permissions::FileSystemSandboxPolicy; + use codex_protocol::permissions::NetworkSandboxPolicy; + use pretty_assertions::assert_eq; + + #[tokio::test] + async fn sync_walk_rejects_sandbox_context() -> io::Result<()> { + let temp = tempfile::tempdir()?; + let root = PathUri::from_host_native_path(temp.path())?; + let sandbox = FileSystemSandboxContext::from_permission_profile( + PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(Vec::new()), + NetworkSandboxPolicy::Restricted, + ), + ); + let options = WalkOptions { + max_depth: 1, + max_directories: 1, + max_entries: 1, + follow_directory_symlinks: false, + prune_hidden_directories: false, + }; + let direct_error = DirectFileSystem + .walk(&root, options, Some(&sandbox)) + .await + .expect_err("direct walk must reject sandbox contexts"); + let wrapper_error = UnsandboxedFileSystem::default() + .walk(&root, options, Some(&sandbox)) + .await + .expect_err("unsandboxed walk must reject restricted contexts"); + assert_eq!(direct_error.kind(), io::ErrorKind::InvalidInput); + assert_eq!(wrapper_error.kind(), io::ErrorKind::InvalidInput); + Ok(()) + } + + #[test] + fn sync_walk_cancellation_stops_before_io() -> io::Result<()> { + let temp = tempfile::tempdir()?; + let missing = PathUri::from_host_native_path(temp.path().join("missing"))?; + let options = WalkOptions { + max_depth: 1, + max_directories: 1, + max_entries: 1, + follow_directory_symlinks: true, + prune_hidden_directories: false, + }; + let cancelled = CancellationToken::new(); + let cancel_on_drop = cancelled.clone().drop_guard(); + drop(cancel_on_drop); + + for result in [ + DirectFileSystem::sync_walk(&missing, options, &cancelled).map(|_| ()), + walk_read_directory(&missing, &cancelled).map(|_| ()), + ] { + assert_eq!( + result + .expect_err("cancelled walks must stop before I/O") + .kind(), + io::ErrorKind::Interrupted, + ); + } + Ok(()) + } + + #[test] + fn sync_walk_response_budget_counts_entries_and_errors() -> io::Result<()> { + let temp = tempfile::tempdir()?; + let root = PathUri::from_host_native_path(temp.path())?; + let mut outcome = WalkOutcome::default(); + let mut response_bytes = + MAX_WALK_RESPONSE_BYTES - WALK_RESPONSE_ITEM_OVERHEAD_BYTES - root.to_string().len(); + assert!(push_walk_error( + &mut outcome, + &mut response_bytes, + root.clone(), + String::new() + )); + assert!(!reserve_walk_response_bytes( + &mut outcome, + &mut response_bytes, + /*content_bytes*/ 0 + )); + assert_eq!( + outcome, + WalkOutcome { + entries: Vec::new(), + errors: vec![WalkError { + path: root, + message: String::new() + }], + truncated: true, + }, + ); + Ok(()) + } +} diff --git a/codex-rs/exec-server/src/remote_file_system.rs b/codex-rs/exec-server/src/remote_file_system.rs index a2242ca668..d574cece8c 100644 --- a/codex-rs/exec-server/src/remote_file_system.rs +++ b/codex-rs/exec-server/src/remote_file_system.rs @@ -37,7 +37,6 @@ use crate::protocol::FsWalkParams; use crate::protocol::FsWriteFileParams; const INVALID_REQUEST_ERROR_CODE: i64 = -32600; -const METHOD_NOT_FOUND_ERROR_CODE: i64 = -32601; const NOT_FOUND_ERROR_CODE: i64 = -32004; #[path = "remote_file_stream.rs"] @@ -252,27 +251,14 @@ impl RemoteFileSystem { ) -> FileSystemResult { trace!("remote fs walk"); let client = self.client.get().await.map_err(map_remote_error)?; - let response = match client + client .fs_walk(FsWalkParams { path: path.clone(), options, sandbox: remote_sandbox_context(sandbox), }) .await - { - Ok(response) => response, - Err(ExecServerError::Server { - code: METHOD_NOT_FOUND_ERROR_CODE, - .. - }) => { - return ::walk_via_directory_reads( - self, path, options, sandbox, - ) - .await; - } - Err(error) => return Err(map_remote_error(error)), - }; - Ok(response) + .map_err(map_remote_error) } async fn remove( diff --git a/codex-rs/exec-server/tests/file_system/shared.rs b/codex-rs/exec-server/tests/file_system/shared.rs index 4a1c856cea..96dfa14349 100644 --- a/codex-rs/exec-server/tests/file_system/shared.rs +++ b/codex-rs/exec-server/tests/file_system/shared.rs @@ -17,6 +17,9 @@ use codex_exec_server::WalkEntryKind; use codex_exec_server::WalkOptions; use codex_exec_server::WalkOutcome; use codex_exec_server::WriteFileOptions; +use codex_file_system::MAX_WALK_DEPTH; +use codex_file_system::MAX_WALK_DIRECTORIES; +use codex_file_system::MAX_WALK_ENTRIES; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; @@ -570,6 +573,89 @@ async fn file_system_walk_returns_a_bounded_tree( Ok(()) } +#[test_case(FileSystemImplementation::Local ; "local")] +#[test_case(FileSystemImplementation::Remote ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_walk_handles_invalid_roots_and_limits( + implementation: FileSystemImplementation, +) -> Result<()> { + let context = create_file_system_context(implementation).await?; + let file_system = context.file_system; + let tmp = TempDir::new()?; + let file_path = tmp.path().join("file.txt"); + std::fs::write(&file_path, "contents")?; + let missing = PathUri::from_host_native_path(tmp.path().join("missing"))?; + let options = WalkOptions { + max_depth: 8, + max_directories: 100, + max_entries: 100, + follow_directory_symlinks: false, + prune_hidden_directories: false, + }; + + let outcome = file_system + .walk( + &PathUri::from_host_native_path(file_path)?, + options, + /*sandbox*/ None, + ) + .await + .with_context(|| format!("mode={implementation}"))?; + assert_eq!(outcome, WalkOutcome::default()); + + let error = file_system + .walk(&missing, options, /*sandbox*/ None) + .await + .expect_err("a missing root must fail"); + assert_eq!(error.kind(), std::io::ErrorKind::NotFound); + + let zero_limit_message = "filesystem walk limits must be greater than zero"; + let excessive_limit_message = format!( + "filesystem walk limits exceed maximums: depth={MAX_WALK_DEPTH}, directories={MAX_WALK_DIRECTORIES}, entries={MAX_WALK_ENTRIES}" + ); + for (max_depth, max_directories, max_entries, message) in [ + (8, 0, 100, zero_limit_message), + (8, 100, 0, zero_limit_message), + ( + MAX_WALK_DEPTH + 1, + 100, + 100, + excessive_limit_message.as_str(), + ), + ( + 8, + MAX_WALK_DIRECTORIES + 1, + 100, + excessive_limit_message.as_str(), + ), + ( + 8, + 100, + MAX_WALK_ENTRIES + 1, + excessive_limit_message.as_str(), + ), + ] { + let options = WalkOptions { + max_depth, + max_directories, + max_entries, + ..options + }; + // Invalid limits take precedence over the missing root. + let error = file_system + .walk(&missing, options, /*sandbox*/ None) + .await + .expect_err("invalid walk limits must fail"); + assert_eq!( + (error.kind(), error.to_string()), + (std::io::ErrorKind::InvalidInput, message.to_owned()), + "mode={implementation}, options={options:?}", + ); + } + + Ok(()) +} + #[test_case(FileSystemImplementation::Local ; "local")] #[test_case(FileSystemImplementation::Remote ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/codex-rs/exec-server/tests/file_system_unix.rs b/codex-rs/exec-server/tests/file_system_unix.rs index 7b6f04c31c..e27c8ed1be 100644 --- a/codex-rs/exec-server/tests/file_system_unix.rs +++ b/codex-rs/exec-server/tests/file_system_unix.rs @@ -815,14 +815,105 @@ async fn file_system_walk_handles_directory_symlinks( std::fs::write(&target_file, "target")?; symlink(&target, &target_link)?; symlink(&root, &root_link)?; + symlink(&target_file, root.join("file-link"))?; + symlink(root.join("missing"), root.join("broken-link"))?; - let outcome = file_system + for root in [&root, &root_link] { + let target_link = root.join("target-link"); + + let outcome = file_system + .walk( + &PathUri::from_host_native_path(root)?, + WalkOptions { + max_depth: 2, + max_directories: 4, + max_entries: 8, + follow_directory_symlinks: false, + prune_hidden_directories: false, + }, + /*sandbox*/ None, + ) + .await + .with_context(|| format!("mode={implementation}"))?; + assert_eq!( + outcome, + WalkOutcome { + entries: Vec::new(), + errors: Vec::new(), + truncated: false, + } + ); + + let outcome = file_system + .walk( + &PathUri::from_host_native_path(root)?, + WalkOptions { + max_depth: 2, + max_directories: 4, + max_entries: 8, + follow_directory_symlinks: true, + prune_hidden_directories: false, + }, + /*sandbox*/ None, + ) + .await + .with_context(|| format!("mode={implementation}"))?; + assert_eq!( + outcome, + WalkOutcome { + entries: vec![ + WalkEntry { + path: PathUri::from_host_native_path(&target_link)?, + kind: WalkEntryKind::Directory, + }, + WalkEntry { + path: PathUri::from_host_native_path(target_link.join("note.txt"))?, + kind: WalkEntryKind::File, + }, + WalkEntry { + path: PathUri::from_host_native_path(target_link.join("root-link"))?, + kind: WalkEntryKind::Directory, + }, + ], + errors: Vec::new(), + truncated: false, + } + ); + } + + Ok(()) +} + +#[cfg(target_os = "linux")] +#[test_case(FileSystemImplementation::Local ; "local")] +#[test_case(FileSystemImplementation::Remote ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_walk_reports_non_utf8_names( + implementation: FileSystemImplementation, +) -> Result<()> { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + use codex_exec_server::WalkError; + + let context = create_file_system_context(implementation).await?; + let tmp = TempDir::new()?; + std::fs::write( + tmp.path() + .join(OsString::from_vec(b"invalid-\xff".to_vec())), + "contents", + )?; + let lossy_path = tmp.path().join("invalid-\u{fffd}"); + let error = + std::fs::symlink_metadata(&lossy_path).expect_err("the lossy filename must not exist"); + let outcome = context + .file_system .walk( - &PathUri::from_host_native_path(&root)?, + &PathUri::from_host_native_path(tmp.path())?, WalkOptions { - max_depth: 2, - max_directories: 4, - max_entries: 8, + max_depth: 0, + max_directories: 1, + max_entries: 1, follow_directory_symlinks: false, prune_hidden_directories: false, }, @@ -834,47 +925,13 @@ async fn file_system_walk_handles_directory_symlinks( outcome, WalkOutcome { entries: Vec::new(), - errors: Vec::new(), + errors: vec![WalkError { + path: PathUri::from_host_native_path(lossy_path)?, + message: error.to_string(), + }], truncated: false, } ); - - let outcome = file_system - .walk( - &PathUri::from_host_native_path(&root)?, - WalkOptions { - max_depth: 2, - max_directories: 4, - max_entries: 8, - follow_directory_symlinks: true, - prune_hidden_directories: false, - }, - /*sandbox*/ None, - ) - .await - .with_context(|| format!("mode={implementation}"))?; - assert_eq!( - outcome, - WalkOutcome { - entries: vec![ - WalkEntry { - path: PathUri::from_host_native_path(&target_link)?, - kind: WalkEntryKind::Directory, - }, - WalkEntry { - path: PathUri::from_host_native_path(target_link.join("note.txt"))?, - kind: WalkEntryKind::File, - }, - WalkEntry { - path: PathUri::from_host_native_path(target_link.join("root-link"))?, - kind: WalkEntryKind::Directory, - }, - ], - errors: Vec::new(), - truncated: false, - } - ); - Ok(()) } diff --git a/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs b/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs index 7870d92a2a..b1f95c9948 100644 --- a/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs +++ b/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs @@ -15,6 +15,8 @@ use codex_exec_server::GetMetadataOptions; use codex_exec_server::ReadDirectoryEntry; use codex_exec_server::ReadFileOptions; use codex_exec_server::RemoveOptions; +use codex_exec_server::WalkOptions; +use codex_exec_server::WalkOutcome; use codex_exec_server::WriteFileOptions; use codex_plugin::ResolvedPlugin; use codex_plugin::manifest::PluginManifest; @@ -124,6 +126,15 @@ impl ExecutorFileSystem for SyntheticExecutorFileSystem { Box::pin(async { Self::unsupported() }) } + fn walk<'a>( + &'a self, + _path: &'a PathUri, + _options: WalkOptions, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, WalkOutcome> { + Box::pin(async { Self::unsupported() }) + } + fn remove<'a>( &'a self, _path: &'a PathUri, diff --git a/codex-rs/ext/skills/tests/executor_file_system_authority.rs b/codex-rs/ext/skills/tests/executor_file_system_authority.rs index 0f0af7cb64..215670139e 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -25,6 +25,10 @@ use codex_exec_server::GetMetadataOptions; use codex_exec_server::ReadDirectoryEntry; use codex_exec_server::ReadFileOptions; use codex_exec_server::RemoveOptions; +use codex_exec_server::WalkEntry; +use codex_exec_server::WalkEntryKind; +use codex_exec_server::WalkOptions; +use codex_exec_server::WalkOutcome; use codex_exec_server::WriteFileOptions; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::SelectedCapabilityRoot; @@ -191,6 +195,34 @@ impl ExecutorFileSystem for SyntheticFileSystem { Box::pin(SyntheticFileSystem::read_directory(self, path)) } + fn walk<'a>( + &'a self, + path: &'a PathUri, + options: WalkOptions, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, WalkOutcome> { + Box::pin(async move { + self.metadata(path)?; + assert_eq!(path, &self.canonical_root); + assert!(options.max_depth >= 1); + assert!(options.max_directories >= 2); + assert!(options.max_entries >= 2); + Ok(WalkOutcome { + entries: vec![ + WalkEntry { + path: self.path("skill")?, + kind: WalkEntryKind::Directory, + }, + WalkEntry { + path: self.path("skill/SKILL.md")?, + kind: WalkEntryKind::File, + }, + ], + ..WalkOutcome::default() + }) + }) + } + fn remove<'a>( &'a self, _path: &'a PathUri, diff --git a/codex-rs/file-system/src/lib.rs b/codex-rs/file-system/src/lib.rs index 833e68926e..2a651ca67a 100644 --- a/codex-rs/file-system/src/lib.rs +++ b/codex-rs/file-system/src/lib.rs @@ -22,8 +22,6 @@ pub use find_up::find_nearest_native_ancestor_with_markers; use futures::Stream; use serde::Deserialize; use serde::Serialize; -use std::collections::HashSet; -use std::collections::VecDeque; use std::future::Future; use std::io; use std::num::NonZeroUsize; @@ -34,11 +32,16 @@ use std::task::Poll; /// Maximum chunk size returned by [`ExecutorFileSystem::read_file_stream`]. pub const FILE_READ_CHUNK_SIZE: usize = 1024 * 1024; -const MAX_WALK_DEPTH: usize = 64; -const MAX_WALK_DIRECTORIES: usize = 10_000; -const MAX_WALK_ENTRIES: usize = 50_000; -const MAX_WALK_RESPONSE_BYTES: usize = 4 * 1024 * 1024; -const WALK_RESPONSE_ITEM_OVERHEAD_BYTES: usize = 64; +/// Maximum accepted directory depth for a filesystem walk. +pub const MAX_WALK_DEPTH: usize = 64; +/// Maximum accepted directory count, including the walk root. +pub const MAX_WALK_DIRECTORIES: usize = 10_000; +/// Maximum accepted number of directory entries to examine. +pub const MAX_WALK_ENTRIES: usize = 50_000; +/// Maximum estimated size of a walk response. +pub const MAX_WALK_RESPONSE_BYTES: usize = 4 * 1024 * 1024; +/// Per-entry or per-error overhead charged to the walk response budget. +pub const WALK_RESPONSE_ITEM_OVERHEAD_BYTES: usize = 64; #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub struct ReadFileOptions { @@ -519,21 +522,7 @@ pub trait ExecutorFileSystem: Send + Sync { path: &'a PathUri, options: WalkOptions, sandbox: Option<&'a FileSystemSandboxContext>, - ) -> ExecutorFileSystemFuture<'a, WalkOutcome> { - self.walk_via_directory_reads(path, options, sandbox) - } - - /// Performs a bounded walk using the primitive filesystem operations. - /// - /// Implementations with an optimized walk transport can use this as a compatibility fallback. - fn walk_via_directory_reads<'a>( - &'a self, - path: &'a PathUri, - options: WalkOptions, - sandbox: Option<&'a FileSystemSandboxContext>, - ) -> ExecutorFileSystemFuture<'a, WalkOutcome> { - Box::pin(walk_via_directory_reads(self, path, options, sandbox)) - } + ) -> ExecutorFileSystemFuture<'a, WalkOutcome>; fn remove<'a>( &'a self, @@ -550,193 +539,3 @@ pub trait ExecutorFileSystem: Send + Sync { sandbox: Option<&'a FileSystemSandboxContext>, ) -> ExecutorFileSystemFuture<'a, ()>; } - -async fn walk_via_directory_reads( - file_system: &F, - root: &PathUri, - options: WalkOptions, - sandbox: Option<&FileSystemSandboxContext>, -) -> FileSystemResult { - if options.max_directories == 0 || options.max_entries == 0 { - return Err(io::Error::new( - io::ErrorKind::InvalidInput, - "filesystem walk limits must be greater than zero", - )); - } - if options.max_depth > MAX_WALK_DEPTH - || options.max_directories > MAX_WALK_DIRECTORIES - || options.max_entries > MAX_WALK_ENTRIES - { - return Err(io::Error::new( - io::ErrorKind::InvalidInput, - format!( - "filesystem walk limits exceed maximums: depth={MAX_WALK_DEPTH}, directories={MAX_WALK_DIRECTORIES}, entries={MAX_WALK_ENTRIES}" - ), - )); - } - - let root_metadata = file_system - .get_metadata(root, GetMetadataOptions::default(), sandbox) - .await?; - if !root_metadata.is_directory - || (root_metadata.is_symlink && !options.follow_directory_symlinks) - { - return Ok(WalkOutcome::default()); - } - - let root_identity = if options.follow_directory_symlinks { - file_system.canonicalize(root, sandbox).await? - } else { - root.clone() - }; - let mut outcome = WalkOutcome::default(); - let mut queue = VecDeque::from([(root.clone(), 0usize)]); - let mut visited_directories = HashSet::from([root_identity]); - let mut directory_count = 1usize; - let mut entry_count = 0usize; - let mut response_bytes = 0usize; - - while let Some((directory, depth)) = queue.pop_front() { - let mut entries = match file_system.read_directory(&directory, sandbox).await { - Ok(entries) => entries, - Err(error) => { - if !push_walk_error( - &mut outcome, - &mut response_bytes, - directory, - error.to_string(), - ) { - return Ok(outcome); - } - continue; - } - }; - entries.sort_by(|left, right| left.file_name.cmp(&right.file_name)); - - for entry in entries { - if entry_count == options.max_entries { - outcome.truncated = true; - return Ok(outcome); - } - entry_count += 1; - - let path = match directory.join(&entry.file_name) { - Ok(path) => path, - Err(error) => { - if !push_walk_error( - &mut outcome, - &mut response_bytes, - directory.clone(), - error.to_string(), - ) { - return Ok(outcome); - } - continue; - } - }; - let metadata = match file_system - .get_metadata(&path, GetMetadataOptions::default(), sandbox) - .await - { - Ok(metadata) => metadata, - Err(error) => { - if !push_walk_error(&mut outcome, &mut response_bytes, path, error.to_string()) - { - return Ok(outcome); - } - continue; - } - }; - if metadata.is_symlink && (!options.follow_directory_symlinks || !metadata.is_directory) - { - continue; - } - - let kind = if metadata.is_directory { - WalkEntryKind::Directory - } else if metadata.is_file { - WalkEntryKind::File - } else { - continue; - }; - if !reserve_walk_response_bytes( - &mut outcome, - &mut response_bytes, - path.to_string().len(), - ) { - return Ok(outcome); - } - outcome.entries.push(WalkEntry { - path: path.clone(), - kind, - }); - - if kind == WalkEntryKind::Directory && depth < options.max_depth { - if options.prune_hidden_directories && entry.file_name.starts_with('.') { - continue; - } - let directory_identity = if options.follow_directory_symlinks { - match file_system.canonicalize(&path, sandbox).await { - Ok(path) => path, - Err(error) => { - if !push_walk_error( - &mut outcome, - &mut response_bytes, - path, - error.to_string(), - ) { - return Ok(outcome); - } - continue; - } - } - } else { - path.clone() - }; - if !visited_directories.insert(directory_identity) { - continue; - } - if directory_count == options.max_directories { - outcome.truncated = true; - } else { - directory_count += 1; - queue.push_back((path, depth + 1)); - } - } - } - } - - Ok(outcome) -} - -fn push_walk_error( - outcome: &mut WalkOutcome, - response_bytes: &mut usize, - path: PathUri, - message: String, -) -> bool { - let item_bytes = path.to_string().len().saturating_add(message.len()); - if !reserve_walk_response_bytes(outcome, response_bytes, item_bytes) { - return false; - } - outcome.errors.push(WalkError { path, message }); - true -} - -fn reserve_walk_response_bytes( - outcome: &mut WalkOutcome, - response_bytes: &mut usize, - content_bytes: usize, -) -> bool { - let item_bytes = content_bytes.saturating_add(WALK_RESPONSE_ITEM_OVERHEAD_BYTES); - let Some(total_bytes) = response_bytes.checked_add(item_bytes) else { - outcome.truncated = true; - return false; - }; - if total_bytes > MAX_WALK_RESPONSE_BYTES { - outcome.truncated = true; - return false; - } - *response_bytes = total_bytes; - true -}