diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 97e31737c594..c9ace542046f 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -25,6 +25,7 @@ use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; use codex_utils_path_uri::PathUri; +use codex_utils_plugins::DISCOVERABLE_PLUGIN_MANIFEST_PATHS; use codex_utils_plugins::PluginSkillRoot; use codex_utils_plugins::plugin_namespace_for_skill_path; use dirs::home_dir; @@ -145,6 +146,8 @@ enum SymlinkPolicy { struct SkillFileDiscovery { skill_files: Vec, + plugin_roots: HashSet, + namespace_roots: HashSet, warnings: Vec, } @@ -500,6 +503,8 @@ async fn discover_skills_under_root( let root = root.clone(); let mut discovery = SkillFileDiscovery { skill_files: Vec::new(), + plugin_roots: HashSet::new(), + namespace_roots: HashSet::from([root.clone()]), warnings: Vec::new(), }; match fs.get_metadata(&root, /*sandbox*/ None).await { @@ -553,6 +558,12 @@ async fn discover_skills_under_root( .into_iter() .filter_map(|entry| { let file_name = entry.file_name; + if DISCOVERABLE_PLUGIN_MANIFEST_PATHS + .iter() + .any(|path| path.split('/').next() == Some(file_name.as_str())) + { + discovery.plugin_roots.insert(dir.clone()); + } if file_name.starts_with('.') { return None; } @@ -592,6 +603,7 @@ async fn discover_skills_under_root( match fs.read_directory(&path, /*sandbox*/ None).await { Ok(_) => { let resolved_dir = canonicalize_uri_for_skill_identity(fs, &path).await; + discovery.namespace_roots.insert(resolved_dir.clone()); enqueue_dir( &mut queue, &mut visited_dirs, @@ -669,6 +681,7 @@ async fn load_skills_under_root( let SkillFileDiscovery { skill_files, warnings, + .. } = discover_skills_under_root(fs, &PathUri::from_abs_path(root), symlink_policy).await; for warning in warnings { error!("{warning}"); diff --git a/codex-rs/core-skills/src/loader/environment.rs b/codex-rs/core-skills/src/loader/environment.rs index 55fe6e29f002..fd7016d71845 100644 --- a/codex-rs/core-skills/src/loader/environment.rs +++ b/codex-rs/core-skills/src/loader/environment.rs @@ -1,9 +1,13 @@ +use std::collections::HashMap; +use std::collections::HashSet; use std::io; use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::Product; use codex_utils_path_uri::PathUri; +use codex_utils_plugins::plugin_namespace_for_root_uri; use codex_utils_plugins::plugin_namespace_for_skill_uri; +use futures::future::join_all; use crate::model::SkillDependencies; use crate::model::SkillPolicy; @@ -52,7 +56,11 @@ impl EnvironmentSkillMetadata { } } - async fn parse(file_system: &dyn ExecutorFileSystem, path: &PathUri) -> Result { + async fn parse( + file_system: &dyn ExecutorFileSystem, + path: &PathUri, + plugin_namespace: Option<&str>, + ) -> Result { let contents = file_system .read_file_text(path, /*sandbox*/ None) .await @@ -63,8 +71,7 @@ impl EnvironmentSkillMetadata { short_description, } = parse_skill_frontmatter_metadata_inner(&contents, || default_skill_name(path)) .map_err(|err| err.to_string())?; - let name = plugin_namespace_for_skill_uri(file_system, path) - .await + let name = plugin_namespace .map(|namespace| format!("{namespace}:{base_name}")) .unwrap_or(base_name); validate_len(&name, MAX_QUALIFIED_NAME_LEN, "qualified name") @@ -98,8 +105,65 @@ pub async fn load_environment_skills_from_root( let discovery = discover_skills_under_root(file_system, root, SymlinkPolicy::FollowDirectories).await; outcome.warnings.extend(discovery.warnings); + if discovery.skill_files.is_empty() { + return outcome; + } + + let mut skill_ancestors = HashSet::new(); + for skill_path in &discovery.skill_files { + let mut ancestor = skill_path.parent(); + while let Some(path) = ancestor { + skill_ancestors.insert(path.clone()); + ancestor = path.parent(); + } + } + + let namespace_roots = discovery.namespace_roots; + let namespace_lookups = join_all(namespace_roots.iter().map(|namespace_root| async { + ( + namespace_root.clone(), + plugin_namespace_for_skill_uri(file_system, namespace_root).await, + ) + })) + .await; + let plugin_lookups = join_all( + discovery + .plugin_roots + .iter() + .filter(|plugin_root| skill_ancestors.contains(*plugin_root)) + .filter(|plugin_root| !namespace_roots.contains(*plugin_root)) + .map(|plugin_root| async { + ( + plugin_root.clone(), + plugin_namespace_for_root_uri(file_system, plugin_root).await, + ) + }), + ) + .await; + let plugin_namespaces = namespace_lookups + .into_iter() + .chain(plugin_lookups) + .filter_map(|(plugin_root, namespace)| namespace.map(|namespace| (plugin_root, namespace))) + .collect::>(); + for path in discovery.skill_files { - match EnvironmentSkillMetadata::parse(file_system, &path).await { + let mut ancestor = path.parent(); + let plugin_namespace = loop { + let Some(current) = ancestor else { + break None; + }; + if let Some(namespace) = plugin_namespaces.get(¤t) { + break Some(namespace.as_str()); + } + ancestor = current.parent(); + }; + match EnvironmentSkillMetadata::parse( + file_system, + &path, + /*plugin_namespace*/ plugin_namespace, + ) + .await + { Ok(skill) if skill.matches_product_restriction(restriction_product) => { outcome.skills.push(skill); } diff --git a/codex-rs/core-skills/tests/environment_loader.rs b/codex-rs/core-skills/tests/environment_loader.rs new file mode 100644 index 000000000000..42d24460835e --- /dev/null +++ b/codex-rs/core-skills/tests/environment_loader.rs @@ -0,0 +1,216 @@ +use std::fs; +use std::sync::Mutex; + +use codex_core_skills::loader::EnvironmentSkillMetadata; +use codex_core_skills::loader::load_environment_skills_from_root; +use codex_exec_server::CopyOptions; +use codex_exec_server::CreateDirectoryOptions; +use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::ExecutorFileSystemFuture; +use codex_exec_server::FileMetadata; +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_utils_path_uri::PathUri; +use pretty_assertions::assert_eq; +use tempfile::tempdir; + +struct RecordingFileSystem<'a> { + inner: &'a dyn ExecutorFileSystem, + read_files: Mutex>, +} + +impl<'a> RecordingFileSystem<'a> { + fn new(inner: &'a dyn ExecutorFileSystem) -> Self { + Self { + inner, + read_files: Mutex::new(Vec::new()), + } + } + + fn read_files(&self) -> Vec { + self.read_files + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .clone() + } +} + +impl ExecutorFileSystem for RecordingFileSystem<'_> { + fn canonicalize<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, PathUri> { + self.inner.canonicalize(path, sandbox) + } + + fn read_file<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, Vec> { + self.read_files + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .push(path.clone()); + self.inner.read_file(path, sandbox) + } + + fn read_file_stream<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, FileSystemReadStream> { + self.inner.read_file_stream(path, sandbox) + } + + fn write_file<'a>( + &'a self, + path: &'a PathUri, + contents: Vec, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + self.inner.write_file(path, contents, sandbox) + } + + fn create_directory<'a>( + &'a self, + path: &'a PathUri, + options: CreateDirectoryOptions, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + self.inner.create_directory(path, options, sandbox) + } + + fn get_metadata<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, FileMetadata> { + self.inner.get_metadata(path, sandbox) + } + + fn read_directory<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, Vec> { + self.inner.read_directory(path, sandbox) + } + + fn remove<'a>( + &'a self, + path: &'a PathUri, + options: RemoveOptions, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + self.inner.remove(path, options, sandbox) + } + + fn copy<'a>( + &'a self, + source_path: &'a PathUri, + destination_path: &'a PathUri, + options: CopyOptions, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + self.inner + .copy(source_path, destination_path, options, sandbox) + } +} + +#[tokio::test] +async fn loads_nearest_plugin_namespaces_without_reading_unused_sibling_manifests() { + let root = tempdir().expect("tempdir"); + let standalone_skill = root.path().join("standalone/SKILL.md"); + let outer_root = root.path().join("plugins/outer"); + let outer_skill = outer_root.join("skills/deploy/SKILL.md"); + let inner_root = outer_root.join("nested/inner"); + let inner_skill = inner_root.join("skills/audit/SKILL.md"); + let unused_root = root.path().join("plugins/unused"); + + for path in [&standalone_skill, &outer_skill, &inner_skill] { + fs::create_dir_all(path.parent().expect("skill parent")).expect("skill dir"); + } + for (plugin_root, name) in [ + (&outer_root, "outer"), + (&inner_root, "inner"), + (&unused_root, "unused"), + ] { + fs::create_dir_all(plugin_root.join(".codex-plugin")).expect("manifest dir"); + fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + format!(r#"{{"name":"{name}"}}"#), + ) + .expect("manifest"); + } + for (path, name) in [ + (&standalone_skill, "standalone"), + (&outer_skill, "deploy"), + (&inner_skill, "audit"), + ] { + fs::write( + path, + format!("---\nname: {name}\ndescription: {name} skill.\n---\n"), + ) + .expect("skill"); + } + + let file_system = RecordingFileSystem::new(LOCAL_FS.as_ref()); + let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI"); + let outcome = load_environment_skills_from_root( + &file_system, + &root_uri, + /*restriction_product*/ None, + ) + .await; + + assert_eq!(outcome.warnings, Vec::::new()); + assert_eq!( + outcome.skills, + vec![ + EnvironmentSkillMetadata { + path_to_skills_md: PathUri::from_host_native_path(&inner_skill).unwrap(), + name: "inner:audit".to_string(), + description: "audit skill.".to_string(), + short_description: None, + dependencies: None, + policy: None, + }, + EnvironmentSkillMetadata { + path_to_skills_md: PathUri::from_host_native_path(&outer_skill).unwrap(), + name: "outer:deploy".to_string(), + description: "deploy skill.".to_string(), + short_description: None, + dependencies: None, + policy: None, + }, + EnvironmentSkillMetadata { + path_to_skills_md: PathUri::from_host_native_path(&standalone_skill).unwrap(), + name: "standalone".to_string(), + description: "standalone skill.".to_string(), + short_description: None, + dependencies: None, + policy: None, + }, + ] + ); + + let mut manifest_reads = file_system + .read_files() + .into_iter() + .filter(|path| path.basename().as_deref() == Some("plugin.json")) + .collect::>(); + manifest_reads.sort_by_key(ToString::to_string); + let mut expected_manifest_reads = [&outer_root, &inner_root] + .into_iter() + .map(|plugin_root| { + PathUri::from_host_native_path(plugin_root.join(".codex-plugin/plugin.json")).unwrap() + }) + .collect::>(); + expected_manifest_reads.sort_by_key(ToString::to_string); + assert_eq!(manifest_reads, expected_manifest_reads); +} diff --git a/codex-rs/utils/plugins/src/lib.rs b/codex-rs/utils/plugins/src/lib.rs index 70e4dcdb869c..5cb590549545 100644 --- a/codex-rs/utils/plugins/src/lib.rs +++ b/codex-rs/utils/plugins/src/lib.rs @@ -9,6 +9,7 @@ pub mod plugin_namespace; pub use plugin_namespace::DISCOVERABLE_PLUGIN_MANIFEST_PATHS; pub use plugin_namespace::find_plugin_manifest_path; +pub use plugin_namespace::plugin_namespace_for_root_uri; pub use plugin_namespace::plugin_namespace_for_skill_path; pub use plugin_namespace::plugin_namespace_for_skill_uri; diff --git a/codex-rs/utils/plugins/src/plugin_namespace.rs b/codex-rs/utils/plugins/src/plugin_namespace.rs index 085c4e1dbd64..4604942ffe97 100644 --- a/codex-rs/utils/plugins/src/plugin_namespace.rs +++ b/codex-rs/utils/plugins/src/plugin_namespace.rs @@ -24,7 +24,8 @@ struct RawPluginManifestName { name: String, } -async fn plugin_manifest_name( +/// Returns the plugin manifest `name` defined directly below `plugin_root`. +pub async fn plugin_namespace_for_root_uri( fs: &dyn ExecutorFileSystem, plugin_root: &PathUri, ) -> Option { @@ -68,7 +69,7 @@ pub async fn plugin_namespace_for_skill_uri( ) -> Option { let mut ancestor = Some(path.clone()); while let Some(path) = ancestor { - if let Some(name) = plugin_manifest_name(fs, &path).await { + if let Some(name) = plugin_namespace_for_root_uri(fs, &path).await { return Some(name); } ancestor = path.parent();