diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 2a3a3189be9b..da0e01300a4b 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -13,6 +13,10 @@ use codex_config::default_project_root_markers; use codex_config::merge_toml_values; use codex_config::project_root_markers_from_config; use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::FS_GET_METADATA_METHOD; +use codex_exec_server::FileMetadata; +use codex_exec_server::FsGetMetadataParams; +use codex_exec_server::FsGetMetadataResponse; use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; @@ -534,15 +538,26 @@ async fn discover_skills_under_root( } }; - for entry in entries { - let file_name = entry.file_name; - if file_name.starts_with('.') { - continue; - } - - let path = dir.join(&file_name); - let path_uri = PathUri::from_abs_path(&path); - let metadata = match fs.get_metadata(&path_uri, /*sandbox*/ None).await { + let paths = entries + .into_iter() + .filter_map(|entry| { + let file_name = entry.file_name; + if file_name.starts_with('.') { + return None; + } + let path = dir.join(&file_name); + let path_uri = PathUri::from_abs_path(&path); + Some((file_name, path, path_uri)) + }) + .collect::>(); + let mut metadata_results = batched_get_metadata(fs, &paths).await.map(Vec::into_iter); + + for (file_name, path, path_uri) in paths { + let metadata_result = match metadata_results.as_mut().and_then(Iterator::next) { + Some(result) => result, + None => fs.get_metadata(&path_uri, /*sandbox*/ None).await, + }; + let metadata = match metadata_result { Ok(metadata) => metadata, Err(e) => { error!("failed to stat skills path {}: {e:#}", path.display()); @@ -628,6 +643,70 @@ async fn discover_skills_under_root( } } +async fn batched_get_metadata( + fs: &dyn ExecutorFileSystem, + paths: &[(String, AbsolutePathBuf, PathUri)], +) -> Option>> { + if paths.is_empty() { + return Some(Vec::new()); + } + + let mut requests = Vec::with_capacity(paths.len()); + for (_, _, path) in paths { + let params = match serde_json::to_value(FsGetMetadataParams { + path: path.clone(), + sandbox: None, + }) { + Ok(params) => params, + Err(err) => { + error!("failed to encode batched skills path stat: {err:#}"); + return None; + } + }; + requests.push((FS_GET_METADATA_METHOD.to_string(), params)); + } + + let results = match fs.execute_rpc_batch(requests).await { + Ok(results) if results.len() == paths.len() => results, + Ok(results) => { + error!( + "batched skills path stat returned {} results for {} requests", + results.len(), + paths.len() + ); + return None; + } + Err(err) if err.kind() == io::ErrorKind::Unsupported => return None, + Err(err) => { + error!("failed to batch-stat skills paths: {err:#}"); + return None; + } + }; + + Some( + results + .into_iter() + .map(|result| { + let response: FsGetMetadataResponse = + serde_json::from_value(result?).map_err(|err| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("fs/getMetadata returned invalid response: {err}"), + ) + })?; + Ok(FileMetadata { + is_directory: response.is_directory, + is_file: response.is_file, + is_symlink: response.is_symlink, + size: response.size, + created_at_ms: response.created_at_ms, + modified_at_ms: response.modified_at_ms, + }) + }) + .collect(), + ) +} + async fn parse_skill_file( fs: &dyn ExecutorFileSystem, path: &AbsolutePathBuf, diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index e22ed33f3df1..be7cbc2175cc 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -4,7 +4,14 @@ use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; +use codex_exec_server::CopyOptions; +use codex_exec_server::CreateDirectoryOptions; +use codex_exec_server::ExecutorFileSystemFuture; +use codex_exec_server::FileSystemReadStream; +use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::LOCAL_FS; +use codex_exec_server::ReadDirectoryEntry; +use codex_exec_server::RemoveOptions; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -16,6 +23,7 @@ use std::fs; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; +use std::sync::Mutex; use tempfile::TempDir; use toml::Value as TomlValue; @@ -2157,3 +2165,171 @@ async fn skill_roots_include_admin_with_lowest_priority() { expected.push(SkillScope::Admin); assert_eq!(scopes, expected); } + +#[tokio::test] +async fn batched_get_metadata_sends_one_request_per_path() { + let tmp = tempfile::tempdir().expect("tempdir"); + let first_path = tmp.path().join("first").abs(); + let second_path = tmp.path().join("second").abs(); + let first_uri = PathUri::from_abs_path(&first_path); + let second_uri = PathUri::from_abs_path(&second_path); + let fs = BatchMetadataFileSystem::default(); + + let results = super::batched_get_metadata( + &fs, + &[ + ("first".to_string(), first_path, first_uri.clone()), + ("second".to_string(), second_path, second_uri.clone()), + ], + ) + .await + .expect("batch should be supported") + .into_iter() + .map(|result| result.expect("metadata result")) + .collect::>(); + + assert_eq!( + results, + vec![ + FileMetadata { + is_directory: true, + is_file: false, + is_symlink: false, + size: 0, + created_at_ms: 1, + modified_at_ms: 2, + }, + FileMetadata { + is_directory: true, + is_file: false, + is_symlink: false, + size: 0, + created_at_ms: 1, + modified_at_ms: 2, + }, + ] + ); + + let calls = fs.calls.lock().expect("calls lock"); + assert_eq!(calls.len(), 1); + assert_eq!(calls[0].len(), 2); + assert_eq!(calls[0][0].0, FS_GET_METADATA_METHOD); + assert_eq!(calls[0][1].0, FS_GET_METADATA_METHOD); + let first_params: FsGetMetadataParams = + serde_json::from_value(calls[0][0].1.clone()).expect("first params"); + let second_params: FsGetMetadataParams = + serde_json::from_value(calls[0][1].1.clone()).expect("second params"); + assert_eq!(first_params.path, first_uri); + assert_eq!(second_params.path, second_uri); +} + +#[derive(Default)] +struct BatchMetadataFileSystem { + calls: Mutex>>, +} + +impl ExecutorFileSystem for BatchMetadataFileSystem { + fn canonicalize<'a>( + &'a self, + _path: &'a PathUri, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, PathUri> { + Box::pin(async { unsupported() }) + } + + fn read_file<'a>( + &'a self, + _path: &'a PathUri, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, Vec> { + Box::pin(async { unsupported() }) + } + + fn read_file_stream<'a>( + &'a self, + _path: &'a PathUri, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, FileSystemReadStream> { + Box::pin(async { unsupported() }) + } + + fn write_file<'a>( + &'a self, + _path: &'a PathUri, + _contents: Vec, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + Box::pin(async { unsupported() }) + } + + fn create_directory<'a>( + &'a self, + _path: &'a PathUri, + _create_directory_options: CreateDirectoryOptions, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + Box::pin(async { unsupported() }) + } + + fn get_metadata<'a>( + &'a self, + _path: &'a PathUri, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, FileMetadata> { + Box::pin(async { unsupported() }) + } + + fn read_directory<'a>( + &'a self, + _path: &'a PathUri, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, Vec> { + Box::pin(async { unsupported() }) + } + + fn remove<'a>( + &'a self, + _path: &'a PathUri, + _remove_options: RemoveOptions, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + Box::pin(async { unsupported() }) + } + + fn copy<'a>( + &'a self, + _source_path: &'a PathUri, + _destination_path: &'a PathUri, + _copy_options: CopyOptions, + _sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + Box::pin(async { unsupported() }) + } + + fn execute_rpc_batch<'a>( + &'a self, + requests: Vec<(String, serde_json::Value)>, + ) -> ExecutorFileSystemFuture<'a, Vec>> { + Box::pin(async move { + let len = requests.len(); + self.calls.lock().expect("calls lock").push(requests); + Ok((0..len) + .map(|_| { + serde_json::to_value(FsGetMetadataResponse { + is_directory: true, + is_file: false, + is_symlink: false, + size: 0, + created_at_ms: 1, + modified_at_ms: 2, + }) + .map_err(io::Error::other) + }) + .collect()) + }) + } +} + +fn unsupported() -> io::Result { + Err(io::Error::new(io::ErrorKind::Unsupported, "unsupported")) +}