diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index c0457af4ac..8677251bb5 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -4109,6 +4109,7 @@ dependencies = [ "codex-utils-absolute-path", "codex-utils-cargo-bin", "codex-utils-path-uri", + "codex-utils-plugins", "codex-utils-string", "futures", "insta", diff --git a/codex-rs/core-skills/tests/environment_loader.rs b/codex-rs/core-skills/tests/environment_loader.rs index 0150322703..53cbac4abd 100644 --- a/codex-rs/core-skills/tests/environment_loader.rs +++ b/codex-rs/core-skills/tests/environment_loader.rs @@ -1,12 +1,10 @@ +#![cfg(unix)] + use std::fs; use std::sync::Mutex; -use std::sync::atomic::AtomicBool; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; -use std::time::Duration; -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; @@ -22,22 +20,12 @@ use codex_exec_server::WalkOutcome; use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use tempfile::tempdir; -use tokio::sync::Notify; - -#[derive(Clone, Copy)] -enum ManifestMetadataBehavior { - Immediate, - WaitForSkillRead, -} struct RecordingFileSystem<'a> { inner: &'a dyn ExecutorFileSystem, read_files: Mutex>, metadata_files: Mutex>, walks: AtomicUsize, - manifest_metadata_behavior: ManifestMetadataBehavior, - skill_read_started: AtomicBool, - skill_read_started_notify: Notify, } #[derive(Debug, PartialEq, Eq)] @@ -48,18 +36,12 @@ struct FileSystemCalls { } impl<'a> RecordingFileSystem<'a> { - fn new( - inner: &'a dyn ExecutorFileSystem, - manifest_metadata_behavior: ManifestMetadataBehavior, - ) -> Self { + fn new(inner: &'a dyn ExecutorFileSystem) -> Self { Self { inner, read_files: Mutex::new(Vec::new()), metadata_files: Mutex::new(Vec::new()), walks: AtomicUsize::new(0), - manifest_metadata_behavior, - skill_read_started: AtomicBool::new(false), - skill_read_started_notify: Notify::new(), } } @@ -102,10 +84,6 @@ impl ExecutorFileSystem for RecordingFileSystem<'_> { .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) .push(path.clone()); - if path.basename().as_deref() == Some("SKILL.md") { - self.skill_read_started.store(true, Ordering::Release); - self.skill_read_started_notify.notify_waiters(); - } self.inner.read_file(path, sandbox) } @@ -144,22 +122,6 @@ impl ExecutorFileSystem for RecordingFileSystem<'_> { .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) .push(path.clone()); - if matches!( - self.manifest_metadata_behavior, - ManifestMetadataBehavior::WaitForSkillRead - ) && path.basename().as_deref() == Some("plugin.json") - { - return Box::pin(async move { - loop { - let notified = self.skill_read_started_notify.notified(); - if self.skill_read_started.load(Ordering::Acquire) { - break; - } - notified.await; - } - self.inner.get_metadata(path, sandbox).await - }); - } self.inner.get_metadata(path, sandbox) } @@ -202,216 +164,6 @@ impl ExecutorFileSystem for RecordingFileSystem<'_> { } } -#[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(), ManifestMetadataBehavior::Immediate); - 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 - .calls() - .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); -} - -#[tokio::test] -async fn reuses_walk_inventory_for_missing_skill_metadata() { - const SKILL_COUNT: usize = 66; - - let root = tempdir().expect("tempdir"); - let manifest_path = root.path().join(".codex-plugin/plugin.json"); - fs::create_dir_all(manifest_path.parent().expect("manifest parent")).expect("manifest dir"); - fs::write(&manifest_path, r#"{"name":"inventory"}"#).expect("manifest"); - - let mut skill_paths = Vec::new(); - for index in 0..SKILL_COUNT { - let name = format!("skill-{index}"); - let skill_path = root.path().join(&name).join("SKILL.md"); - fs::create_dir_all(skill_path.parent().expect("skill parent")).expect("skill dir"); - fs::write( - &skill_path, - format!("---\nname: {name}\ndescription: {name} skill.\n---\n"), - ) - .expect("skill"); - skill_paths.push(skill_path); - } - - let file_system = - RecordingFileSystem::new(LOCAL_FS.as_ref(), ManifestMetadataBehavior::Immediate); - 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; - - let mut expected_skills = skill_paths - .iter() - .enumerate() - .map(|(index, skill_path)| EnvironmentSkillMetadata { - path_to_skills_md: PathUri::from_host_native_path(skill_path).unwrap(), - name: format!("inventory:skill-{index}"), - description: format!("skill-{index} skill."), - short_description: None, - dependencies: None, - policy: None, - }) - .collect::>(); - expected_skills.sort_by(|left, right| { - left.name.cmp(&right.name).then_with(|| { - left.path_to_skills_md - .to_string() - .cmp(&right.path_to_skills_md.to_string()) - }) - }); - assert_eq!(outcome.skills, expected_skills); - assert_eq!(outcome.warnings, Vec::::new()); - - let mut expected_read_files = skill_paths - .iter() - .map(|path| PathUri::from_host_native_path(path).unwrap()) - .collect::>(); - let manifest_uri = PathUri::from_host_native_path(manifest_path).unwrap(); - expected_read_files.push(manifest_uri.clone()); - expected_read_files.sort_by_key(ToString::to_string); - assert_eq!( - file_system.calls(), - FileSystemCalls { - walks: 1, - read_files: expected_read_files, - metadata_files: vec![manifest_uri], - } - ); -} - -#[tokio::test] -async fn reads_skill_files_while_resolving_plugin_namespaces() { - let root = tempdir().expect("tempdir"); - let manifest_path = root.path().join(".codex-plugin/plugin.json"); - fs::create_dir_all(manifest_path.parent().expect("manifest parent")).expect("manifest dir"); - fs::write(&manifest_path, r#"{"name":"parallel"}"#).expect("manifest"); - let skill_path = root.path().join("demo/SKILL.md"); - fs::create_dir_all(skill_path.parent().expect("skill parent")).expect("skill dir"); - fs::write( - &skill_path, - "---\nname: demo\ndescription: demo skill.\n---\n", - ) - .expect("skill"); - - let file_system = RecordingFileSystem::new( - LOCAL_FS.as_ref(), - ManifestMetadataBehavior::WaitForSkillRead, - ); - let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI"); - let outcome = tokio::time::timeout( - Duration::from_secs(5), - load_environment_skills_from_root( - &file_system, - &root_uri, - /*restriction_product*/ None, - ), - ) - .await - .expect("skill reads should start before namespace resolution finishes"); - - assert_eq!(outcome.warnings, Vec::::new()); - assert_eq!( - outcome.skills, - vec![EnvironmentSkillMetadata { - path_to_skills_md: PathUri::from_host_native_path(skill_path).unwrap(), - name: "parallel:demo".to_string(), - description: "demo skill.".to_string(), - short_description: None, - dependencies: None, - policy: None, - }] - ); -} - #[cfg(unix)] #[tokio::test] async fn host_loading_reuses_walk_inventory_for_symlinked_skill_pack() { @@ -455,10 +207,7 @@ async fn host_loading_reuses_walk_inventory_for_symlinked_skill_pack() { let linked_root = host_root.join("linked-plugin"); symlink(&skills_root, &linked_root).expect("skill pack symlink"); - let recording = Arc::new(RecordingFileSystem::new( - LOCAL_FS.as_ref(), - ManifestMetadataBehavior::Immediate, - )); + let recording = Arc::new(RecordingFileSystem::new(LOCAL_FS.as_ref())); let file_system: Arc = recording.clone(); let future = load_skills_from_roots( [SkillRoot { diff --git a/codex-rs/ext/skills/Cargo.toml b/codex-rs/ext/skills/Cargo.toml index 692f6ba4e2..8c64028ac2 100644 --- a/codex-rs/ext/skills/Cargo.toml +++ b/codex-rs/ext/skills/Cargo.toml @@ -22,6 +22,7 @@ codex-protocol = { workspace = true } codex-skills = { workspace = true } codex-tools = { workspace = true } codex-utils-path-uri = { workspace = true } +codex-utils-plugins = { workspace = true } codex-utils-string = { workspace = true } futures = { workspace = true } schemars = { workspace = true } diff --git a/codex-rs/ext/skills/src/loader/discovery.rs b/codex-rs/ext/skills/src/loader/discovery.rs new file mode 100644 index 0000000000..3d1609b237 --- /dev/null +++ b/codex-rs/ext/skills/src/loader/discovery.rs @@ -0,0 +1,167 @@ +use std::collections::HashSet; +use std::io; + +use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::WalkEntryKind; +use codex_exec_server::WalkOptions; +use codex_utils_path_uri::PathUri; +use codex_utils_plugins::DISCOVERABLE_PLUGIN_MANIFEST_PATHS; + +use super::MAX_SCAN_DEPTH; +use super::MAX_SKILLS_DIRS_PER_ROOT; +use super::SKILLS_FILENAME; +use super::SKILLS_METADATA_DIR; +use super::SKILLS_METADATA_FILENAME; + +const MAX_SKILLS_ENTRIES_PER_ROOT: usize = 20_000; +pub(super) const MAX_CONCURRENT_SKILL_LOADS: usize = 64; + +pub(super) struct SkillDiscovery { + pub skills: Vec, + pub plugin_roots: HashSet, + pub namespace_roots: HashSet, + pub warnings: Vec, +} + +pub(super) struct DiscoveredSkill { + pub path: PathUri, + pub metadata: SkillMetadataDiscovery, +} + +pub(super) enum SkillMetadataDiscovery { + Present(PathUri), + Absent, + Probe(PathUri), +} + +pub(super) async fn discover_skills( + file_system: &dyn ExecutorFileSystem, + root: &PathUri, +) -> SkillDiscovery { + let empty_discovery = || SkillDiscovery { + skills: Vec::new(), + plugin_roots: HashSet::new(), + namespace_roots: HashSet::new(), + warnings: Vec::new(), + }; + let walk = match file_system + .walk( + root, + WalkOptions { + max_depth: MAX_SCAN_DEPTH, + max_directories: MAX_SKILLS_DIRS_PER_ROOT, + max_entries: MAX_SKILLS_ENTRIES_PER_ROOT, + follow_directory_symlinks: true, + prune_hidden_directories: false, + }, + /*sandbox*/ None, + ) + .await + { + Ok(walk) => walk, + Err(error) if error.kind() == io::ErrorKind::NotFound => return empty_discovery(), + Err(error) => { + let mut discovery = empty_discovery(); + discovery + .warnings + .push(format!("failed to walk skills root {root}: {error:#}")); + return discovery; + } + }; + + let inventory_complete = !walk.truncated && walk.errors.is_empty(); + let mut warnings = walk + .errors + .into_iter() + .map(|error| { + format!( + "failed to scan skill path {}: {}", + error.path, error.message + ) + }) + .collect::>(); + if walk.truncated { + warnings.push(format!( + "skills scan reached its traversal limit (root: {root})" + )); + } + + let mut skill_files = Vec::new(); + let mut file_paths = HashSet::new(); + let mut metadata_directory_parents = HashSet::new(); + let mut plugin_roots = HashSet::new(); + for entry in walk.entries { + match entry.kind { + WalkEntryKind::Directory => { + if entry + .path + .basename() + .is_some_and(|name| name.eq_ignore_ascii_case(SKILLS_METADATA_DIR)) + && let Some(skill_dir) = entry.path.parent() + { + metadata_directory_parents.insert(skill_dir); + } + if DISCOVERABLE_PLUGIN_MANIFEST_PATHS + .iter() + .any(|path| path.split('/').next() == entry.path.basename().as_deref()) + && let Some(plugin_root) = entry.path.parent() + { + plugin_roots.insert(plugin_root); + } + } + WalkEntryKind::File => { + file_paths.insert(entry.path.clone()); + if entry.path.basename().as_deref() == Some(SKILLS_FILENAME) { + skill_files.push(entry.path); + } + } + } + } + let skills = skill_files + .into_iter() + .map(|path| DiscoveredSkill { + metadata: discover_skill_metadata( + &path, + &file_paths, + &metadata_directory_parents, + inventory_complete, + ), + path, + }) + .collect(); + + SkillDiscovery { + skills, + plugin_roots, + namespace_roots: HashSet::from([root.clone()]), + warnings, + } +} + +fn discover_skill_metadata( + skill_path: &PathUri, + file_paths: &HashSet, + metadata_directory_parents: &HashSet, + inventory_complete: bool, +) -> SkillMetadataDiscovery { + let Some(skill_dir) = skill_path.parent() else { + return SkillMetadataDiscovery::Absent; + }; + let Ok(metadata_dir) = skill_dir.join(SKILLS_METADATA_DIR) else { + return SkillMetadataDiscovery::Absent; + }; + let Ok(metadata_path) = metadata_dir.join(SKILLS_METADATA_FILENAME) else { + return SkillMetadataDiscovery::Absent; + }; + if file_paths.contains(&metadata_path) { + return SkillMetadataDiscovery::Present(metadata_path); + } + + if inventory_complete && !metadata_directory_parents.contains(&skill_dir) { + SkillMetadataDiscovery::Absent + } else { + // A complete walk proves ordinary absence, but keep a filesystem probe for case aliases, + // file symlinks omitted by the walk, and incomplete inventories. + SkillMetadataDiscovery::Probe(metadata_path) + } +} diff --git a/codex-rs/ext/skills/src/loader/environment.rs b/codex-rs/ext/skills/src/loader/environment.rs index ce7624bdd5..dcf3ac15ce 100644 --- a/codex-rs/ext/skills/src/loader/environment.rs +++ b/codex-rs/ext/skills/src/loader/environment.rs @@ -1,18 +1,37 @@ use std::collections::HashMap; +use std::io; use codex_exec_server::CapabilityRootDiscovery; +use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::Product; use codex_skills::EnvironmentSkillMetadata; use codex_skills::ParsedSkillFrontmatter; +use codex_skills::SkillDependencies; +use codex_skills::SkillPolicy; use codex_skills::parse_skill_frontmatter_metadata; use codex_utils_path_uri::PathUri; +use futures::StreamExt; use super::MAX_QUALIFIED_NAME_LEN; +use super::discovery::DiscoveredSkill; +use super::discovery::MAX_CONCURRENT_SKILL_LOADS; +use super::discovery::SkillMetadataDiscovery; +use super::discovery::discover_skills; use super::metadata::SkillMetadataFile; use super::metadata::resolve_dependencies; use super::metadata::resolve_policy; use super::metadata::sanitize_single_line; use super::metadata::validate_len; +use super::namespace::SkillNamespaceResolver; + +struct ParsedEnvironmentSkill { + path_to_skills_md: PathUri, + base_name: String, + description: String, + short_description: Option, + dependencies: Option, + policy: Option, +} /// Parsed executor skill plus the instructions already materialized by capability discovery. #[derive(Clone, Debug, PartialEq, Eq)] @@ -27,6 +46,144 @@ pub struct EnvironmentSkillSnapshotOutcome { pub warnings: Vec, } +impl ParsedEnvironmentSkill { + async fn load( + file_system: &dyn ExecutorFileSystem, + skill: &DiscoveredSkill, + ) -> Result { + let (contents, discovered_metadata) = match &skill.metadata { + SkillMetadataDiscovery::Present(metadata_path) => { + let (contents, metadata) = tokio::join!( + read_skill_contents(file_system, &skill.path), + read_skill_metadata(file_system, metadata_path), + ); + (contents?, metadata) + } + SkillMetadataDiscovery::Absent | SkillMetadataDiscovery::Probe(_) => ( + read_skill_contents(file_system, &skill.path).await?, + (None, None), + ), + }; + let ParsedSkillFrontmatter { + name: base_name, + description, + short_description, + } = parse_skill_frontmatter_metadata(&contents, || default_skill_name(&skill.path)) + .map_err(|err| err.to_string())?; + let (dependencies, policy) = match &skill.metadata { + SkillMetadataDiscovery::Present(_) | SkillMetadataDiscovery::Absent => { + discovered_metadata + } + SkillMetadataDiscovery::Probe(metadata_path) => { + probe_skill_metadata(file_system, metadata_path).await + } + }; + + Ok(Self { + path_to_skills_md: skill.path.clone(), + base_name, + description, + short_description, + dependencies, + policy, + }) + } +} + +#[derive(Debug, Default)] +pub struct EnvironmentSkillLoadOutcome { + pub skills: Vec, + pub warnings: Vec, +} + +/// Discovers skills without converting environment-owned paths to host paths. +#[tracing::instrument( + name = "skills.environment.load", + level = "info", + skip_all, + fields(skill_count = tracing::field::Empty) +)] +pub async fn load_environment_skills_from_root( + file_system: &dyn ExecutorFileSystem, + root: &PathUri, + restriction_product: Option, +) -> EnvironmentSkillLoadOutcome { + let mut outcome = EnvironmentSkillLoadOutcome::default(); + // Preserve environment discovery behavior by following directory aliases and including + // hidden directories exposed by the executor. + let discovery = discover_skills(file_system, root).await; + tracing::Span::current().record("skill_count", discovery.skills.len()); + outcome.warnings.extend(discovery.warnings); + if discovery.skills.is_empty() { + return outcome; + } + + let skill_paths = discovery + .skills + .iter() + .map(|skill| skill.path.clone()) + .collect::>(); + let namespace_resolver = SkillNamespaceResolver::discover( + file_system, + root, + &skill_paths, + discovery.plugin_roots, + discovery.namespace_roots, + ); + + // Remote executors can multiplex these independent per-skill reads, so polling a bounded + // number together allows the I/O for each skill and its metadata to happen concurrently. + let skill_results = futures::stream::iter(discovery.skills) + .map(|skill| { + let path = skill.path.clone(); + async move { + ( + path, + ParsedEnvironmentSkill::load(file_system, &skill).await, + ) + } + }) + .buffered(MAX_CONCURRENT_SKILL_LOADS) + .collect::>(); + let (namespace_resolver, skill_results) = tokio::join!(namespace_resolver, skill_results); + + for (path, result) in skill_results { + let result = result.and_then(|skill| { + let name = namespace_resolver + .for_skill(root, &skill.path_to_skills_md) + .qualify(&skill.base_name); + validate_len(&name, MAX_QUALIFIED_NAME_LEN, "qualified name") + .map_err(|err| err.to_string())?; + + Ok(EnvironmentSkillMetadata { + path_to_skills_md: skill.path_to_skills_md, + name, + description: skill.description, + short_description: skill.short_description, + dependencies: skill.dependencies, + policy: skill.policy, + }) + }); + match result { + Ok(skill) if skill.matches_product_restriction(restriction_product) => { + outcome.skills.push(skill); + } + Ok(_) => {} + Err(message) => outcome.warnings.push(format!( + "Failed to load environment skill at {path}: {message}" + )), + } + } + outcome.skills.sort_by(|left, right| { + left.name.cmp(&right.name).then_with(|| { + left.path_to_skills_md + .to_string() + .cmp(&right.path_to_skills_md.to_string()) + }) + }); + outcome +} + /// Parses an executor-produced manifest bundle without issuing additional filesystem requests. pub fn load_environment_skills_from_discovery( discovery: &CapabilityRootDiscovery, @@ -151,6 +308,62 @@ fn nearest_plugin_namespace<'a>( } None } +async fn read_skill_contents( + file_system: &dyn ExecutorFileSystem, + skill_path: &PathUri, +) -> Result { + file_system + .read_file_text(skill_path, /*sandbox*/ None) + .await + .map_err(|err| format!("failed to read file: {err}")) +} + +async fn probe_skill_metadata( + file_system: &dyn ExecutorFileSystem, + metadata_path: &PathUri, +) -> (Option, Option) { + match file_system + .get_metadata(metadata_path, /*sandbox*/ None) + .await + { + Ok(metadata) if metadata.is_file => {} + Ok(_) => return (None, None), + Err(error) if error.kind() == io::ErrorKind::NotFound => return (None, None), + Err(error) => { + tracing::warn!("ignoring {metadata_path}: failed to stat metadata: {error}"); + return (None, None); + } + } + read_skill_metadata(file_system, metadata_path).await +} + +async fn read_skill_metadata( + file_system: &dyn ExecutorFileSystem, + metadata_path: &PathUri, +) -> (Option, Option) { + let contents = match file_system + .read_file_text(metadata_path, /*sandbox*/ None) + .await + { + Ok(contents) => contents, + Err(error) => { + tracing::warn!("ignoring {metadata_path}: failed to read metadata: {error}"); + return (None, None); + } + }; + let parsed: SkillMetadataFile = match serde_yaml::from_str(&contents) { + Ok(parsed) => parsed, + Err(error) => { + tracing::warn!("ignoring {metadata_path}: invalid metadata: {error}"); + return (None, None); + } + }; + + ( + resolve_dependencies(parsed.dependencies), + resolve_policy(parsed.policy), + ) +} fn default_skill_name(path: &PathUri) -> String { path.parent() @@ -163,3 +376,7 @@ fn default_skill_name(path: &PathUri) -> String { #[cfg(test)] #[path = "environment_tests.rs"] mod tests; + +#[cfg(test)] +#[path = "environment_io_tests.rs"] +mod io_tests; diff --git a/codex-rs/ext/skills/src/loader/environment_io_tests.rs b/codex-rs/ext/skills/src/loader/environment_io_tests.rs new file mode 100644 index 0000000000..0a02413dc0 --- /dev/null +++ b/codex-rs/ext/skills/src/loader/environment_io_tests.rs @@ -0,0 +1,414 @@ +use std::fs; +use std::sync::Mutex; +use std::sync::atomic::AtomicBool; +use std::sync::atomic::AtomicUsize; +use std::sync::atomic::Ordering; +use std::time::Duration; + +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_exec_server::WalkOptions; +use codex_exec_server::WalkOutcome; +use codex_skills::EnvironmentSkillMetadata; +use codex_utils_path_uri::PathUri; +use pretty_assertions::assert_eq; +use tempfile::tempdir; +use tokio::sync::Notify; + +use super::load_environment_skills_from_root; + +#[derive(Clone, Copy)] +enum ManifestMetadataBehavior { + Immediate, + WaitForSkillRead, +} + +struct RecordingFileSystem<'a> { + inner: &'a dyn ExecutorFileSystem, + read_files: Mutex>, + metadata_files: Mutex>, + walks: AtomicUsize, + manifest_metadata_behavior: ManifestMetadataBehavior, + skill_read_started: AtomicBool, + skill_read_started_notify: Notify, +} + +#[derive(Debug, PartialEq, Eq)] +struct FileSystemCalls { + walks: usize, + read_files: Vec, + metadata_files: Vec, +} + +impl<'a> RecordingFileSystem<'a> { + fn new( + inner: &'a dyn ExecutorFileSystem, + manifest_metadata_behavior: ManifestMetadataBehavior, + ) -> Self { + Self { + inner, + read_files: Mutex::new(Vec::new()), + metadata_files: Mutex::new(Vec::new()), + walks: AtomicUsize::new(0), + manifest_metadata_behavior, + skill_read_started: AtomicBool::new(false), + skill_read_started_notify: Notify::new(), + } + } + + fn calls(&self) -> FileSystemCalls { + let mut read_files = self + .read_files + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .clone(); + read_files.sort_by_key(ToString::to_string); + let mut metadata_files = self + .metadata_files + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .clone(); + metadata_files.sort_by_key(ToString::to_string); + FileSystemCalls { + walks: self.walks.load(Ordering::Relaxed), + read_files, + metadata_files, + } + } +} + +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()); + if path.basename().as_deref() == Some("SKILL.md") { + self.skill_read_started.store(true, Ordering::Release); + self.skill_read_started_notify.notify_waiters(); + } + 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.metadata_files + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .push(path.clone()); + if matches!( + self.manifest_metadata_behavior, + ManifestMetadataBehavior::WaitForSkillRead + ) && path.basename().as_deref() == Some("plugin.json") + { + return Box::pin(async move { + loop { + let notified = self.skill_read_started_notify.notified(); + if self.skill_read_started.load(Ordering::Acquire) { + break; + } + notified.await; + } + self.inner.get_metadata(path, sandbox).await + }); + } + 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 walk<'a>( + &'a self, + path: &'a PathUri, + options: WalkOptions, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, WalkOutcome> { + self.walks.fetch_add(1, Ordering::Relaxed); + self.inner.walk(path, options, 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(), ManifestMetadataBehavior::Immediate); + 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 + .calls() + .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); +} + +#[tokio::test] +async fn reuses_walk_inventory_for_missing_skill_metadata() { + const SKILL_COUNT: usize = 66; + + let root = tempdir().expect("tempdir"); + let manifest_path = root.path().join(".codex-plugin/plugin.json"); + fs::create_dir_all(manifest_path.parent().expect("manifest parent")).expect("manifest dir"); + fs::write(&manifest_path, r#"{"name":"inventory"}"#).expect("manifest"); + + let mut skill_paths = Vec::new(); + for index in 0..SKILL_COUNT { + let name = format!("skill-{index}"); + let skill_path = root.path().join(&name).join("SKILL.md"); + fs::create_dir_all(skill_path.parent().expect("skill parent")).expect("skill dir"); + fs::write( + &skill_path, + format!("---\nname: {name}\ndescription: {name} skill.\n---\n"), + ) + .expect("skill"); + skill_paths.push(skill_path); + } + + let file_system = + RecordingFileSystem::new(LOCAL_FS.as_ref(), ManifestMetadataBehavior::Immediate); + 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; + + let mut expected_skills = skill_paths + .iter() + .enumerate() + .map(|(index, skill_path)| EnvironmentSkillMetadata { + path_to_skills_md: PathUri::from_host_native_path(skill_path).unwrap(), + name: format!("inventory:skill-{index}"), + description: format!("skill-{index} skill."), + short_description: None, + dependencies: None, + policy: None, + }) + .collect::>(); + expected_skills.sort_by(|left, right| { + left.name.cmp(&right.name).then_with(|| { + left.path_to_skills_md + .to_string() + .cmp(&right.path_to_skills_md.to_string()) + }) + }); + assert_eq!(outcome.skills, expected_skills); + assert_eq!(outcome.warnings, Vec::::new()); + + let mut expected_read_files = skill_paths + .iter() + .map(|path| PathUri::from_host_native_path(path).unwrap()) + .collect::>(); + let manifest_uri = PathUri::from_host_native_path(manifest_path).unwrap(); + expected_read_files.push(manifest_uri.clone()); + expected_read_files.sort_by_key(ToString::to_string); + assert_eq!( + file_system.calls(), + FileSystemCalls { + walks: 1, + read_files: expected_read_files, + metadata_files: vec![manifest_uri], + } + ); +} + +#[tokio::test] +async fn reads_skill_files_while_resolving_plugin_namespaces() { + let root = tempdir().expect("tempdir"); + let manifest_path = root.path().join(".codex-plugin/plugin.json"); + fs::create_dir_all(manifest_path.parent().expect("manifest parent")).expect("manifest dir"); + fs::write(&manifest_path, r#"{"name":"parallel"}"#).expect("manifest"); + let skill_path = root.path().join("demo/SKILL.md"); + fs::create_dir_all(skill_path.parent().expect("skill parent")).expect("skill dir"); + fs::write( + &skill_path, + "---\nname: demo\ndescription: demo skill.\n---\n", + ) + .expect("skill"); + + let file_system = RecordingFileSystem::new( + LOCAL_FS.as_ref(), + ManifestMetadataBehavior::WaitForSkillRead, + ); + let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI"); + let outcome = tokio::time::timeout( + Duration::from_secs(5), + load_environment_skills_from_root( + &file_system, + &root_uri, + /*restriction_product*/ None, + ), + ) + .await + .expect("skill reads should start before namespace resolution finishes"); + + assert_eq!(outcome.warnings, Vec::::new()); + assert_eq!( + outcome.skills, + vec![EnvironmentSkillMetadata { + path_to_skills_md: PathUri::from_host_native_path(skill_path).unwrap(), + name: "parallel:demo".to_string(), + description: "demo skill.".to_string(), + short_description: None, + dependencies: None, + policy: None, + }] + ); +} diff --git a/codex-rs/ext/skills/src/loader/mod.rs b/codex-rs/ext/skills/src/loader/mod.rs index ac5ed067ff..f2e7ae7a1c 100644 --- a/codex-rs/ext/skills/src/loader/mod.rs +++ b/codex-rs/ext/skills/src/loader/mod.rs @@ -1,8 +1,14 @@ +mod discovery; mod environment; mod metadata; +mod namespace; pub(crate) use environment::load_environment_skills_from_discovery; +pub(crate) use environment::load_environment_skills_from_root; +pub(super) const SKILLS_FILENAME: &str = "SKILL.md"; +pub(super) const SKILLS_METADATA_DIR: &str = "agents"; +pub(super) const SKILLS_METADATA_FILENAME: &str = "openai.yaml"; pub(super) const MAX_NAME_LEN: usize = 64; pub(super) const MAX_QUALIFIED_NAME_LEN: usize = 128; pub(super) const MAX_DESCRIPTION_LEN: usize = 1024; @@ -12,3 +18,5 @@ pub(super) const MAX_DEPENDENCY_VALUE_LEN: usize = MAX_DESCRIPTION_LEN; pub(super) const MAX_DEPENDENCY_DESCRIPTION_LEN: usize = MAX_DESCRIPTION_LEN; pub(super) const MAX_DEPENDENCY_COMMAND_LEN: usize = MAX_DESCRIPTION_LEN; pub(super) const MAX_DEPENDENCY_URL_LEN: usize = MAX_DESCRIPTION_LEN; +pub(super) const MAX_SCAN_DEPTH: usize = 6; +pub(super) const MAX_SKILLS_DIRS_PER_ROOT: usize = 2000; diff --git a/codex-rs/ext/skills/src/loader/namespace.rs b/codex-rs/ext/skills/src/loader/namespace.rs new file mode 100644 index 0000000000..3027a235bf --- /dev/null +++ b/codex-rs/ext/skills/src/loader/namespace.rs @@ -0,0 +1,172 @@ +use std::collections::HashMap; +use std::collections::HashSet; + +use codex_exec_server::ExecutorFileSystem; +use codex_utils_path_uri::PathUri; +use codex_utils_plugins::plugin_namespace_for_root_uri; +use futures::StreamExt; + +use super::discovery::MAX_CONCURRENT_SKILL_LOADS; + +/// Resolves the namespace prefix applied to skill names during one skills scan. +/// +/// A plugin namespace is the plugin name from the nearest valid plugin manifest +/// above a skill path. For example, a skill named `search` beneath a plugin named +/// `sample` is exposed as `sample:search`. +/// +/// Resolving the namespace separately for every `SKILL.md` repeats the same +/// ancestor manifest probes for sibling skills. This resolver resolves relevant +/// roots once per scan, then selects the nearest matching root for each skill. +/// +/// The deepest matching canonical symlink root or nested plugin root wins, +/// followed by the namespace inherited from the scanned skills root. +pub(crate) struct SkillNamespaceResolver { + inherited_namespace: ResolvedSkillNamespace, + nested_namespaces: Vec<(PathUri, ResolvedSkillNamespace)>, +} + +impl SkillNamespaceResolver { + pub(crate) async fn discover( + fs: &dyn ExecutorFileSystem, + root: &PathUri, + skill_paths: &[PathUri], + plugin_roots: HashSet, + namespace_roots: HashSet, + ) -> Self { + // Only probe plugin roots above loaded skills; unused siblings cannot affect names. + let mut skill_ancestors = HashSet::new(); + for skill_path in skill_paths { + let mut ancestor = skill_path.parent(); + while let Some(path) = ancestor { + skill_ancestors.insert(path.clone()); + ancestor = path.parent(); + } + } + let plugin_roots = plugin_roots + .into_iter() + .filter(|plugin_root| skill_ancestors.contains(plugin_root)) + .collect::>(); + + // The scan root is already the fallback above if nothing else matches, exclude from the search. + let namespace_roots = namespace_roots + .into_iter() + .filter(|namespace_root| namespace_root != root) + .collect::>(); + let namespace_root_set = namespace_roots.iter().cloned().collect::>(); + let plugin_roots = plugin_roots + .into_iter() + .filter(|plugin_root| plugin_root != root && !namespace_root_set.contains(plugin_root)) + .collect::>(); + + let lookup_roots = std::iter::once(root.clone()) + .chain(namespace_roots.iter().cloned()) + .collect::>(); + let mut pending_lookups = lookup_roots + .iter() + .cloned() + .map(|lookup_root| (lookup_root.clone(), lookup_root)) + .collect::>(); + let mut direct_plugin_roots = plugin_roots.iter().cloned().collect::>(); + let mut namespaces_by_root = HashMap::new(); + let mut namespaces_by_lookup_root = HashMap::new(); + while !pending_lookups.is_empty() { + let probe_roots = pending_lookups + .iter() + .map(|(_, ancestor)| ancestor.clone()) + .chain(direct_plugin_roots.drain()) + .filter(|ancestor| !namespaces_by_root.contains_key(ancestor)) + .collect::>(); + namespaces_by_root.extend( + futures::stream::iter(probe_roots) + .map(|manifest_root| async move { + let namespace = plugin_namespace_for_root_uri(fs, &manifest_root).await; + (manifest_root, namespace) + }) + .buffered(MAX_CONCURRENT_SKILL_LOADS) + .collect::>() + .await, + ); + + let mut next_lookups = Vec::new(); + for (lookup_root, ancestor) in pending_lookups { + match namespaces_by_root.get(&ancestor) { + Some(Some(namespace)) => { + namespaces_by_lookup_root.insert(lookup_root, Some(namespace.clone())); + } + Some(None) => match ancestor.parent() { + Some(parent) => next_lookups.push((lookup_root, parent)), + None => { + namespaces_by_lookup_root.insert(lookup_root, None); + } + }, + None => unreachable!("pending namespace ancestor was not probed"), + } + } + pending_lookups = next_lookups; + } + + // Ordinary descendants fall back to the nearest valid manifest at or above the scan root. + let inherited_namespace = namespaces_by_lookup_root + .get(root) + .and_then(Option::as_ref) + .cloned() + .map(ResolvedSkillNamespace::Plugin) + .unwrap_or(ResolvedSkillNamespace::Plain); + let namespace_lookups = namespace_roots.into_iter().map(|namespace_root| { + let namespace = namespaces_by_lookup_root + .get(&namespace_root) + .and_then(Option::as_ref) + .cloned() + .map(ResolvedSkillNamespace::Plugin) + .unwrap_or(ResolvedSkillNamespace::Plain); + (namespace_root, namespace) + }); + // Invalid nested manifests are omitted, so the deepest remaining match wins. + let plugin_lookups = plugin_roots.into_iter().filter_map(|plugin_root| { + namespaces_by_root + .get(&plugin_root) + .and_then(Option::as_ref) + .cloned() + .map(|namespace| (plugin_root, ResolvedSkillNamespace::Plugin(namespace))) + }); + let nested_namespaces = namespace_lookups.chain(plugin_lookups).collect(); + + Self { + inherited_namespace, + nested_namespaces, + } + } + + pub(crate) fn for_skill(&self, root: &PathUri, path: &PathUri) -> &ResolvedSkillNamespace { + // Ancestor symlink targets cannot override skills still owned by the scan root. + let path_is_under_root = path.starts_with(root); + // The deepest matching path prefix is the nearest applicable namespace. + self.nested_namespaces + .iter() + .filter(|(namespace_root, _)| { + path.starts_with(namespace_root) + && (!path_is_under_root || !root.starts_with(namespace_root)) + }) + .max_by_key(|(namespace_root, _)| namespace_root.ancestors().count()) + .map(|(_, namespace)| namespace) + .unwrap_or(&self.inherited_namespace) + } +} + +/// The completed namespace resolution for a skill root. +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) enum ResolvedSkillNamespace { + /// No plugin namespace applies to matching skills. + Plain, + /// Qualify matching skill names with this plugin namespace. + Plugin(String), +} + +impl ResolvedSkillNamespace { + pub(crate) fn qualify(&self, base_name: &str) -> String { + match self { + Self::Plain => base_name.to_string(), + Self::Plugin(namespace) => format!("{namespace}:{base_name}"), + } + } +} diff --git a/codex-rs/ext/skills/src/provider/executor.rs b/codex-rs/ext/skills/src/provider/executor.rs index dde6473a89..b095708d86 100644 --- a/codex-rs/ext/skills/src/provider/executor.rs +++ b/codex-rs/ext/skills/src/provider/executor.rs @@ -1,6 +1,5 @@ use std::sync::Arc; -use codex_core_skills::loader::load_environment_skills_from_root; use codex_exec_server::EnvironmentManager; use codex_exec_server::FileSystemSandboxContext; use codex_protocol::capabilities::CapabilityRootLocation; @@ -20,6 +19,7 @@ use crate::catalog::SkillResourceId; use crate::catalog::SkillSearchResult; use crate::catalog::SkillSourceKind; use crate::loader::load_environment_skills_from_discovery; +use crate::loader::load_environment_skills_from_root; use crate::provider::MAX_SKILL_RESOURCE_CONTENT_BYTES; use crate::provider::SkillListQuery; use crate::provider::SkillProvider; 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 a74e9b4875..e3160b43ed 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -641,6 +641,124 @@ async fn pre_discovered_executor_catalog_snapshot() { std::fs::remove_dir_all(test_root).expect("remove skill directory"); } +#[tokio::test] +async fn direct_executor_discovery_preserves_hidden_nested_and_probed_metadata() { + let id = NEXT_TEST_ROOT_ID.fetch_add(1, Ordering::Relaxed); + let test_root = std::env::temp_dir().join(format!( + "codex-executor-skill-discovery-{}-{id}", + std::process::id() + )); + let outer_manifest = test_root.join(".codex-plugin/plugin.json"); + let hidden_skill = test_root.join(".hidden/deploy/SKILL.md"); + let hidden_metadata = test_root.join(".hidden/deploy/agents/openai.yaml"); + let inner_manifest = test_root.join("nested/.codex-plugin/plugin.json"); + let inner_skill = test_root.join("nested/skills/audit/SKILL.md"); + for (path, contents) in [ + (&outer_manifest, r#"{"name":"outer"}"#), + ( + &hidden_skill, + "---\nname: hidden\ndescription: Hidden skill.\n---\n", + ), + (&inner_manifest, r#"{"name":"inner"}"#), + ( + &inner_skill, + "---\nname: audit\ndescription: Audit skill.\n---\n", + ), + ] { + std::fs::create_dir_all(path.parent().expect("test file parent")) + .expect("create test directory"); + std::fs::write(path, contents).expect("write test file"); + } + std::fs::create_dir_all(hidden_metadata.parent().expect("metadata parent")) + .expect("create metadata directory"); + let metadata_contents = "policy:\n allow_implicit_invocation: false\n"; + #[cfg(unix)] + { + use std::os::unix::fs::symlink; + + let metadata_target = test_root.join("linked-openai.yaml"); + std::fs::write(&metadata_target, metadata_contents).expect("write metadata target"); + symlink(metadata_target, &hidden_metadata).expect("link metadata"); + } + #[cfg(not(unix))] + std::fs::write(&hidden_metadata, metadata_contents).expect("write metadata"); + + let root_uri = PathUri::from_host_native_path(&test_root).expect("skill root URI"); + let selected_root = SelectedCapabilityRoot { + id: "discovery-root".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "local".to_string(), + path: root_uri.clone(), + }, + }; + let manager = Arc::new(EnvironmentManager::default_for_tests()); + let legacy_file_system = manager + .get_environment("local") + .expect("local environment") + .get_filesystem(); + let legacy = load_legacy_environment_skills_from_root( + legacy_file_system.as_ref(), + &root_uri, + /*restriction_product*/ None, + ) + .await; + let provider = ExecutorSkillProvider::new_with_restriction_product( + manager, /*restriction_product*/ None, + ); + let direct = provider + .list(SkillListQuery { + turn_id: "turn-1".to_string(), + executor_roots: vec![selected_root], + resolved_executor_roots: Vec::new(), + host_snapshot: None, + include_host_skills: false, + include_bundled_skills: true, + include_orchestrator_skills: false, + mcp_resources: None, + executor_capability_discovery: None, + }) + .await + .expect("list directly discovered executor skills"); + + assert_eq!(direct.warnings, legacy.warnings); + let legacy_metadata = legacy + .skills + .iter() + .map(|skill| { + ( + skill.name.clone(), + skill.description.clone(), + skill.allows_implicit_invocation(), + ) + }) + .collect::>(); + let catalog_metadata = direct + .entries + .iter() + .map(|entry| { + ( + entry.name.clone(), + entry.description.clone(), + entry.prompt_visible, + ) + }) + .collect::>(); + assert_eq!(catalog_metadata, legacy_metadata); + assert_eq!( + catalog_metadata, + vec![ + ("inner:audit".to_string(), "Audit skill.".to_string(), true), + ( + "outer:hidden".to_string(), + "Hidden skill.".to_string(), + false, + ), + ] + ); + + std::fs::remove_dir_all(test_root).expect("remove skill directory"); +} + #[tokio::test] async fn high_level_discovery_reuses_materialized_skill_contents_for_reads() { let test_root = create_local_skill_root("materialized").expect("create local skill root");