From 33e365b19e4a7023b9b3ed74b57aa11748165a53 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Fri, 7 Aug 2026 15:04:25 +0000 Subject: [PATCH] Remove the legacy core skill loader (#37457) ## What changed - Use `HostSkillRoot` throughout host root resolution and loading, including plugin roots. - Remove the duplicate loader, root snapshot, and product-filtering implementation from `core-skills` now that loading is owned by the skills extension. - Move and expand loader coverage for discovery, namespaces, filesystem routing, root merging, symlinks, and frontmatter parsing. ## Testing - Added focused unit and integration tests under `ext/skills` and `skills` for the consolidated loader behavior. GitOrigin-RevId: 214d06d59bf3033ee0f220ea5959ee4feff66782 --- codex-rs/Cargo.lock | 5 - codex-rs/app-server/src/skills_watcher.rs | 2 +- .../core-plugins/src/tool_suggest_metadata.rs | 4 +- codex-rs/core-skills/Cargo.toml | 5 - codex-rs/core-skills/src/lib.rs | 4 - codex-rs/core-skills/src/loader.rs | 645 ----- codex-rs/core-skills/src/loader/discovery.rs | 219 -- .../core-skills/src/loader/environment.rs | 252 -- .../src/loader/environment_tests.rs | 78 - codex-rs/core-skills/src/loader/namespace.rs | 182 -- codex-rs/core-skills/src/loader_tests.rs | 2089 ----------------- codex-rs/core-skills/src/model.rs | 77 - codex-rs/core-skills/src/root_loader.rs | 221 -- .../core-skills/tests/environment_loader.rs | 294 --- codex-rs/core/src/skills.rs | 2 - codex-rs/ext/skills/src/host_roots.rs | 47 +- codex-rs/ext/skills/src/host_roots_tests.rs | 40 +- codex-rs/ext/skills/src/host_service.rs | 41 +- codex-rs/ext/skills/src/lib.rs | 1 + codex-rs/ext/skills/src/loader/discovery.rs | 4 + .../ext/skills/src/loader/discovery_tests.rs | 125 + .../skills/src/loader/environment_io_tests.rs | 198 +- .../skills/src/loader/environment_tests.rs | 78 +- codex-rs/ext/skills/src/loader/host.rs | 7 +- .../ext/skills/src/loader/host_io_tests.rs | 138 ++ codex-rs/ext/skills/src/loader/host_merge.rs | 2 +- .../ext/skills/src/loader/host_merge_tests.rs | 195 ++ codex-rs/ext/skills/src/loader/host_tests.rs | 70 +- .../ext/skills/src/loader/io_test_support.rs | 214 ++ codex-rs/ext/skills/src/loader/mod.rs | 5 +- codex-rs/ext/skills/src/loader/namespace.rs | 4 + .../ext/skills/src/loader/namespace_tests.rs | 172 ++ .../ext/skills/src/provider/host_tests.rs | 42 +- codex-rs/ext/skills/src/render_tests.rs | 35 +- .../tests/executor_file_system_authority.rs | 127 +- codex-rs/ext/skills/tests/skills_extension.rs | 54 +- codex-rs/skills/src/parser_tests.rs | 76 + 37 files changed, 1223 insertions(+), 4531 deletions(-) delete mode 100644 codex-rs/core-skills/src/loader.rs delete mode 100644 codex-rs/core-skills/src/loader/discovery.rs delete mode 100644 codex-rs/core-skills/src/loader/environment.rs delete mode 100644 codex-rs/core-skills/src/loader/environment_tests.rs delete mode 100644 codex-rs/core-skills/src/loader/namespace.rs delete mode 100644 codex-rs/core-skills/src/loader_tests.rs delete mode 100644 codex-rs/core-skills/src/root_loader.rs delete mode 100644 codex-rs/core-skills/tests/environment_loader.rs create mode 100644 codex-rs/ext/skills/src/loader/discovery_tests.rs create mode 100644 codex-rs/ext/skills/src/loader/host_io_tests.rs create mode 100644 codex-rs/ext/skills/src/loader/io_test_support.rs create mode 100644 codex-rs/ext/skills/src/loader/namespace_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 3d8580a22e..8e4fda1034 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2919,18 +2919,13 @@ dependencies = [ "codex-login", "codex-model-provider", "codex-otel", - "codex-protocol", "codex-skills", "codex-utils-absolute-path", "codex-utils-path-uri", - "codex-utils-plugins", "codex-utils-string", - "dunce", - "futures", "pretty_assertions", "serde", "serde_json", - "serde_yaml", "tempfile", "tokio", "toml 0.9.11+spec-1.1.0", diff --git a/codex-rs/app-server/src/skills_watcher.rs b/codex-rs/app-server/src/skills_watcher.rs index 49a83297ab..e09358b6f2 100644 --- a/codex-rs/app-server/src/skills_watcher.rs +++ b/codex-rs/app-server/src/skills_watcher.rs @@ -126,7 +126,7 @@ impl SkillsWatcher { .into_iter() // Plugin roots have explicit lifecycle invalidation; generated system skills are // installed before this watcher starts. - .filter(|root| root.plugin_identity.is_none() && root.scope != SkillScope::System) + .filter(|root| root.plugin_identity().is_none() && root.scope != SkillScope::System) .map(|root| WatchPath { path: root.path.into_path_buf(), recursive: true, diff --git a/codex-rs/core-plugins/src/tool_suggest_metadata.rs b/codex-rs/core-plugins/src/tool_suggest_metadata.rs index 91bdcdb365..28671050a5 100644 --- a/codex-rs/core-plugins/src/tool_suggest_metadata.rs +++ b/codex-rs/core-plugins/src/tool_suggest_metadata.rs @@ -20,7 +20,7 @@ use crate::app_mcp_routing::apply_app_mcp_routing_policy; use crate::loader::PluginSkillInventory; use crate::loader::load_plugin_apps; use crate::loader::load_plugin_mcp_servers; -use crate::loader::load_plugin_skill_inventory_with_loader; +use crate::loader::load_plugin_skill_inventory; use crate::manager::ConfiguredMarketplacePlugin; use crate::manager::remote_plugin_install_required_description; use crate::manifest::PluginManifestFormat; @@ -230,7 +230,7 @@ async fn load_plugin_metadata( remote_plugin_id: None, }; let manifest = loaded_manifest.manifest; - let skill_inventory = load_plugin_skill_inventory_with_loader( + let skill_inventory = load_plugin_skill_inventory( plugin_root, &plugin_identity, &manifest, diff --git a/codex-rs/core-skills/Cargo.toml b/codex-rs/core-skills/Cargo.toml index 43d054cbf7..c0d27559aa 100644 --- a/codex-rs/core-skills/Cargo.toml +++ b/codex-rs/core-skills/Cargo.toml @@ -21,17 +21,12 @@ codex-exec-server = { workspace = true } codex-login = { workspace = true } codex-model-provider = { workspace = true } codex-otel = { workspace = true } -codex-protocol = { workspace = true } codex-skills = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-path-uri = { workspace = true } -codex-utils-plugins = { workspace = true } codex-utils-string = { workspace = true } -dunce = { workspace = true } -futures = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } -serde_yaml = { workspace = true } tokio = { workspace = true, features = ["fs", "macros", "rt"] } tracing = { workspace = true } zip = { workspace = true } diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs index 5e45368241..f7e5ad130a 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -1,10 +1,8 @@ pub mod config_rules; pub mod injection; pub(crate) mod invocation_utils; -pub mod loader; pub mod model; pub mod remote; -mod root_loader; mod skill_instructions; /// Hard byte limit for one model-visible skill instruction body. @@ -21,6 +19,4 @@ pub use model::SkillError; pub use model::SkillLoadOutcome; pub use model::SkillMetadata; pub use model::SkillPolicy; -pub use model::filter_skill_load_outcome_for_product; -pub use root_loader::PluginSkillSnapshots; pub use skill_instructions::SkillInstructions; diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs deleted file mode 100644 index 2a84238358..0000000000 --- a/codex-rs/core-skills/src/loader.rs +++ /dev/null @@ -1,645 +0,0 @@ -mod discovery; -mod environment; -mod namespace; - -pub use crate::root_loader::load_skill_root_snapshot; -pub use environment::EnvironmentSkillLoadOutcome; -pub use environment::EnvironmentSkillMetadata; -pub use environment::load_environment_skills_from_root; - -use crate::model::SkillDependencies; -use crate::model::SkillError; -use crate::model::SkillInterface; -use crate::model::SkillLoadOutcome; -use crate::model::SkillMetadata; -use crate::model::SkillPolicy; -use crate::model::SkillToolDependency; -use codex_exec_server::ExecutorFileSystem; -use codex_protocol::protocol::Product; -use codex_protocol::protocol::SkillScope; -use codex_skills::ParsedSkillFrontmatter; -use codex_skills::SkillInterfaceAssetPolicy; -use codex_skills::SkillInterfaceFile; -use codex_skills::SkillParseError; -use codex_skills::parse_skill_frontmatter_metadata; -use codex_skills::resolve_skill_interface; -use codex_utils_absolute_path::AbsolutePathBuf; -use codex_utils_absolute_path::AbsolutePathBufGuard; -use codex_utils_path_uri::PathUri; -use codex_utils_plugins::PluginIdentity; -use codex_utils_plugins::SkillDiscoveryMode; -use discovery::DirectorySymlinkPolicy; -use discovery::DiscoveredSkill; -use discovery::HiddenDirectoryPolicy; -use discovery::MAX_CONCURRENT_SKILL_LOADS; -use discovery::SkillDiscovery; -use discovery::SkillDiscoveryOptions; -use discovery::SkillMetadataDiscovery; -use discovery::discover_skills; -use futures::FutureExt; -use futures::StreamExt; -use namespace::SkillNamespaceResolver; -use serde::Deserialize; -use std::collections::HashMap; -use std::error::Error; -use std::fmt; -use std::io; -use std::sync::Arc; -use tokio::sync::Semaphore; -use tracing::error; - -// TODO(anp): Tune this eight-scan limit after revisiting byte-based backpressure. -pub const MAX_CONCURRENT_ROOT_SCANS: usize = 8; - -#[derive(Debug, Default, Deserialize)] -struct SkillMetadataFile { - #[serde(default)] - interface: Option, - #[serde(default)] - dependencies: Option, - #[serde(default)] - policy: Option, -} - -#[derive(Default)] -struct LoadedSkillMetadata { - interface: Option, - dependencies: Option, - policy: Option, -} - -#[derive(Debug, Default, Deserialize)] -struct Dependencies { - #[serde(default)] - tools: Vec, -} - -#[derive(Debug, Deserialize)] -struct Policy { - #[serde(default)] - allow_implicit_invocation: Option, - #[serde(default)] - products: Vec, -} - -#[derive(Debug, Default, Deserialize)] -struct DependencyTool { - #[serde(rename = "type")] - kind: Option, - value: Option, - description: Option, - transport: Option, - command: Option, - url: Option, -} - -const SKILLS_FILENAME: &str = "SKILL.md"; -const SKILLS_METADATA_DIR: &str = "agents"; -const SKILLS_METADATA_FILENAME: &str = "openai.yaml"; -const MAX_NAME_LEN: usize = 64; -const MAX_QUALIFIED_NAME_LEN: usize = MAX_NAME_LEN * 2 + 1; -const MAX_DESCRIPTION_LEN: usize = 1024; -const MAX_DEPENDENCY_TYPE_LEN: usize = MAX_NAME_LEN; -const MAX_DEPENDENCY_TRANSPORT_LEN: usize = MAX_NAME_LEN; -const MAX_DEPENDENCY_VALUE_LEN: usize = MAX_DESCRIPTION_LEN; -const MAX_DEPENDENCY_DESCRIPTION_LEN: usize = MAX_DESCRIPTION_LEN; -const MAX_DEPENDENCY_COMMAND_LEN: usize = MAX_DESCRIPTION_LEN; -const MAX_DEPENDENCY_URL_LEN: usize = MAX_DESCRIPTION_LEN; -// Traversal depth from the skills root. -const MAX_SCAN_DEPTH: usize = 6; -const MAX_SKILLS_DIRS_PER_ROOT: usize = 2000; - -struct ResolvedDiscoveredSkill { - skill: DiscoveredSkill, - path: AbsolutePathBuf, - path_uri: PathUri, -} - -#[derive(Debug)] -enum SkillLoadError { - Read(std::io::Error), - Parse(SkillParseError), -} - -impl fmt::Display for SkillLoadError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::Read(error) => write!(f, "failed to read file: {error}"), - Self::Parse(error) => write!(f, "{error}"), - } - } -} - -impl Error for SkillLoadError {} - -impl From for SkillLoadError { - fn from(error: SkillParseError) -> Self { - Self::Parse(error) - } -} - -pub struct SkillRoot { - pub path: AbsolutePathBuf, - pub scope: SkillScope, - pub file_system: Arc, - pub plugin_identity: Option, - pub plugin_namespace: Option, - pub plugin_root: Option, - pub discovery_mode: SkillDiscoveryMode, -} - -pub async fn load_skills_from_roots( - roots: I, - plugin_skill_snapshots: Option<&crate::PluginSkillSnapshots>, - root_scan_slots: Arc, -) -> SkillLoadOutcome -where - I: IntoIterator + Send, - I::IntoIter: Send, -{ - crate::root_loader::load_and_merge_skill_roots(roots, plugin_skill_snapshots, &root_scan_slots) - .boxed() - .await -} - -/// One loaded skill root and the filesystem that supplied it. -/// -/// This is exposed temporarily while host discovery moves out of `core-skills`; callers should -/// combine snapshots through [`SkillLoadOutcome::from_root_snapshots`]. -#[derive(Clone)] -pub struct SkillRootSnapshot { - pub(crate) root: AbsolutePathBuf, - pub(crate) is_agent_plugin: bool, - pub(crate) skills: Vec, - pub(crate) skill_discovery_path_by_path: Arc>, - pub(crate) errors: Vec, - pub(crate) file_system: Arc, -} - -impl SkillRootSnapshot { - pub fn new( - root: AbsolutePathBuf, - skills: Vec, - skill_discovery_path_by_path: Arc>, - errors: Vec, - file_system: Arc, - ) -> Self { - Self { - root, - is_agent_plugin: false, - skills, - skill_discovery_path_by_path, - errors, - file_system, - } - } -} - -pub(crate) async fn load_skill_root(root: SkillRoot) -> SkillRootSnapshot { - let canonical_root = - canonicalize_for_skill_identity(root.file_system.as_ref(), &root.path).await; - let mut outcome = SkillLoadOutcome::default(); - load_skills_under_root(&root, &canonical_root, &mut outcome).await; - SkillRootSnapshot { - root: canonical_root, - is_agent_plugin: root.discovery_mode == SkillDiscoveryMode::DirectChildren, - skills: outcome.skills, - skill_discovery_path_by_path: outcome.skill_discovery_path_by_path, - errors: outcome.errors, - file_system: root.file_system, - } -} - -async fn canonicalize_for_skill_identity( - fs: &dyn ExecutorFileSystem, - path: &AbsolutePathBuf, -) -> AbsolutePathBuf { - let path_uri = PathUri::from_abs_path(path); - fs.canonicalize(&path_uri, /*sandbox*/ None) - .await - .and_then(|path| path.to_abs_path()) - .unwrap_or_else(|_| path.clone()) -} - -async fn load_skills_under_root( - skill_root: &SkillRoot, - root: &AbsolutePathBuf, - outcome: &mut SkillLoadOutcome, -) { - let fs = skill_root.file_system.as_ref(); - let plugin_identity = skill_root.plugin_identity.as_ref(); - let plugin_root = match skill_root.plugin_root.as_ref() { - Some(plugin_root) => Some(canonicalize_for_skill_identity(fs, plugin_root).await), - None => None, - }; - let directory_symlinks = match skill_root.scope { - SkillScope::User | SkillScope::Repo | SkillScope::Admin => DirectorySymlinkPolicy::Follow, - SkillScope::System => DirectorySymlinkPolicy::Ignore, - }; - let SkillDiscovery { - skills, - plugin_roots, - mut namespace_roots, - warnings, - } = discover_skills( - fs, - &PathUri::from_abs_path(root), - // Preserve host discovery behavior: directory aliases are scope-dependent, while hidden - // directories are skipped unless reached through a visible alias. - SkillDiscoveryOptions { - directory_symlinks, - hidden_directories: HiddenDirectoryPolicy::Skip, - mode: skill_root.discovery_mode, - }, - ) - .await; - for warning in warnings { - error!("{warning}"); - } - // With no skills, there is nothing to canonicalize, parse, or namespace-qualify. - if skills.is_empty() { - return; - } - let root_uri = PathUri::from_abs_path(root); - let resolved_plugin_root = plugin_root.as_ref(); - let resolved_skills = futures::stream::iter(skills) - .map(|skill| async move { - let path_uri = match fs.canonicalize(&skill.path, /*sandbox*/ None).await { - Ok(path) => path, - Err(err) if skill_root.discovery_mode == SkillDiscoveryMode::DirectChildren => { - error!( - "failed to resolve Agent Plugin skill path {}: {err}", - skill.path - ); - return None; - } - Err(_) => skill.path.clone(), - }; - let path = match path_uri.to_abs_path() { - Ok(path) => path, - Err(err) => { - error!("failed to convert discovered skill path {path_uri}: {err}"); - return None; - } - }; - if skill_root.discovery_mode == SkillDiscoveryMode::DirectChildren { - let Some(plugin_root) = resolved_plugin_root else { - error!("Agent Plugin skill root is missing its plugin root"); - return None; - }; - if !path.as_path().starts_with(plugin_root.as_path()) { - error!( - "Agent Plugin skill path {} resolves outside plugin root {}", - path.display(), - plugin_root.display() - ); - return None; - } - match fs.get_metadata(&path_uri, /*sandbox*/ None).await { - Ok(metadata) if metadata.is_file => {} - Ok(_) => { - error!( - "Agent Plugin skill path {} is not a regular file", - path.display() - ); - return None; - } - Err(err) => { - error!( - "failed to inspect Agent Plugin skill path {}: {err}", - path.display() - ); - return None; - } - } - } - Some(ResolvedDiscoveredSkill { - skill, - path, - path_uri, - }) - }) - .buffered(MAX_CONCURRENT_SKILL_LOADS) - .filter_map(futures::future::ready) - .collect::>() - .await; - namespace_roots.extend(resolved_skills.iter().filter_map(|skill| { - (skill.path_uri != skill.skill.path) - .then(|| skill.path_uri.parent()) - .flatten() - })); - let skill_paths = resolved_skills - .iter() - .map(|skill| skill.path_uri.clone()) - .collect::>(); - let namespace_resolver = async { - match skill_root.plugin_namespace.as_deref() { - Some(namespace) => SkillNamespaceResolver::with_provided_namespace(namespace), - None => { - SkillNamespaceResolver::discover( - fs, - &root_uri, - &skill_paths, - plugin_roots, - namespace_roots, - ) - .await - } - } - }; - let skill_results = futures::stream::iter(resolved_skills) - .map(|skill| { - let plugin_root = plugin_root.as_ref(); - async move { - let discovery_path = skill - .skill - .path - .to_abs_path() - .unwrap_or_else(|_| skill.path.clone()); - let result = parse_skill_file( - fs, - &skill.skill, - &skill.path, - &skill.path_uri, - skill_root.scope, - plugin_identity, - plugin_root, - ) - .await - .map_err(|err| err.to_string()); - (skill.path, skill.path_uri, discovery_path, result) - } - }) - .buffered(MAX_CONCURRENT_SKILL_LOADS) - .collect::>() - .boxed(); - let (namespace_resolver, skill_results) = tokio::join!(namespace_resolver, skill_results); - for (path, path_uri, discovery_path, result) in skill_results { - let result = result.and_then(|mut skill| { - skill.name = namespace_resolver - .for_skill(&root_uri, &path_uri) - .qualify(&skill.name); - validate_len(&skill.name, MAX_QUALIFIED_NAME_LEN, "qualified name") - .map_err(|err| err.to_string())?; - Ok(skill) - }); - match result { - Ok(skill) => { - Arc::make_mut(&mut outcome.skill_discovery_path_by_path) - .insert(skill.path_to_skills_md.clone(), discovery_path); - outcome.skills.push(skill); - } - Err(err) if skill_root.scope != SkillScope::System => { - outcome.errors.push(SkillError { path, message: err }) - } - Err(_) => {} - } - } -} - -async fn parse_skill_file( - fs: &dyn ExecutorFileSystem, - skill: &DiscoveredSkill, - path: &AbsolutePathBuf, - path_uri: &PathUri, - scope: SkillScope, - plugin_identity: Option<&PluginIdentity>, - plugin_root: Option<&AbsolutePathBuf>, -) -> Result { - let metadata_path = path_uri - .parent() - .and_then(|parent| parent.join(SKILLS_METADATA_DIR).ok()) - .and_then(|directory| directory.join(SKILLS_METADATA_FILENAME).ok()); - let metadata = match &skill.metadata { - SkillMetadataDiscovery::Present(_) => metadata_path.map(SkillMetadataDiscovery::Present), - SkillMetadataDiscovery::Probe(_) => metadata_path.map(SkillMetadataDiscovery::Probe), - SkillMetadataDiscovery::Absent => None, - } - .unwrap_or(SkillMetadataDiscovery::Absent); - let (contents, loaded_metadata) = tokio::join!( - fs.read_file_text(path_uri, /*sandbox*/ None), - load_skill_metadata(fs, path, &metadata, plugin_root), - ); - let contents = contents.map_err(SkillLoadError::Read)?; - let ParsedSkillFrontmatter { - name: base_name, - description, - short_description, - } = parse_skill_frontmatter_metadata(&contents, || default_skill_name(path))?; - let LoadedSkillMetadata { - interface, - dependencies, - policy, - } = loaded_metadata; - - Ok(SkillMetadata { - name: base_name, - description, - short_description, - interface, - dependencies, - policy, - path_to_skills_md: path.clone(), - scope, - plugin_id: plugin_identity.map(|identity| identity.plugin_id.clone()), - remote_plugin_id: plugin_identity.and_then(|identity| identity.remote_plugin_id.clone()), - }) -} - -fn default_skill_name(path: &AbsolutePathBuf) -> String { - path.parent() - .and_then(|parent| { - parent - .file_name() - .and_then(|name| name.to_str()) - .map(sanitize_single_line) - }) - .filter(|value| !value.is_empty()) - .unwrap_or_else(|| "skill".to_string()) -} - -async fn load_skill_metadata( - fs: &dyn ExecutorFileSystem, - skill_path: &AbsolutePathBuf, - metadata: &SkillMetadataDiscovery, - plugin_root: Option<&AbsolutePathBuf>, -) -> LoadedSkillMetadata { - // Fail open: optional metadata should not block loading SKILL.md. - let Some(skill_dir) = skill_path.parent() else { - return LoadedSkillMetadata::default(); - }; - let metadata_path_uri = match metadata { - SkillMetadataDiscovery::Present(path) => path, - SkillMetadataDiscovery::Absent => return LoadedSkillMetadata::default(), - SkillMetadataDiscovery::Probe(path) => { - match fs.get_metadata(path, /*sandbox*/ None).await { - Ok(metadata) if metadata.is_file => {} - Ok(_) => return LoadedSkillMetadata::default(), - Err(error) if error.kind() == io::ErrorKind::NotFound => { - return LoadedSkillMetadata::default(); - } - Err(error) => { - tracing::warn!( - "ignoring {path}: failed to stat {label}: {error}", - path = path, - label = SKILLS_METADATA_FILENAME - ); - return LoadedSkillMetadata::default(); - } - } - path - } - }; - - let contents = match fs.read_file_text(metadata_path_uri, /*sandbox*/ None).await { - Ok(contents) => contents, - Err(error) => { - tracing::warn!( - "ignoring {path}: failed to read {label}: {error}", - path = metadata_path_uri, - label = SKILLS_METADATA_FILENAME - ); - return LoadedSkillMetadata::default(); - } - }; - - let parsed: SkillMetadataFile = { - let _guard = AbsolutePathBufGuard::new(skill_dir.as_path()); - match serde_yaml::from_str(&contents) { - Ok(parsed) => parsed, - Err(error) => { - tracing::warn!( - "ignoring {path}: invalid {label}: {error}", - path = metadata_path_uri, - label = SKILLS_METADATA_FILENAME - ); - return LoadedSkillMetadata::default(); - } - } - }; - - let SkillMetadataFile { - interface, - dependencies, - policy, - } = parsed; - let asset_policy = match plugin_root { - Some(plugin_root) => SkillInterfaceAssetPolicy::PluginShared { plugin_root }, - None => SkillInterfaceAssetPolicy::LocalOnly, - }; - LoadedSkillMetadata { - interface: resolve_skill_interface(interface, &skill_dir, asset_policy), - dependencies: resolve_dependencies(dependencies), - policy: resolve_policy(policy), - } -} - -fn resolve_dependencies(dependencies: Option) -> Option { - let dependencies = dependencies?; - let tools: Vec = dependencies - .tools - .into_iter() - .filter_map(resolve_dependency_tool) - .collect(); - if tools.is_empty() { - None - } else { - Some(SkillDependencies { tools }) - } -} - -fn resolve_policy(policy: Option) -> Option { - policy.map(|policy| SkillPolicy { - allow_implicit_invocation: policy.allow_implicit_invocation, - products: policy.products, - }) -} - -fn resolve_dependency_tool(tool: DependencyTool) -> Option { - let r#type = resolve_required_str( - tool.kind, - MAX_DEPENDENCY_TYPE_LEN, - "dependencies.tools.type", - )?; - let value = resolve_required_str( - tool.value, - MAX_DEPENDENCY_VALUE_LEN, - "dependencies.tools.value", - )?; - let description = resolve_str( - tool.description, - MAX_DEPENDENCY_DESCRIPTION_LEN, - "dependencies.tools.description", - ); - let transport = resolve_str( - tool.transport, - MAX_DEPENDENCY_TRANSPORT_LEN, - "dependencies.tools.transport", - ); - let command = resolve_str( - tool.command, - MAX_DEPENDENCY_COMMAND_LEN, - "dependencies.tools.command", - ); - let url = resolve_str(tool.url, MAX_DEPENDENCY_URL_LEN, "dependencies.tools.url"); - - Some(SkillToolDependency { - r#type, - value, - description, - transport, - command, - url, - }) -} - -fn sanitize_single_line(raw: &str) -> String { - raw.split_whitespace().collect::>().join(" ") -} - -fn validate_len( - value: &str, - max_len: usize, - field_name: &'static str, -) -> Result<(), SkillParseError> { - if value.is_empty() { - return Err(SkillParseError::MissingField(field_name)); - } - if value.chars().count() > max_len { - return Err(SkillParseError::InvalidField { - field: field_name, - reason: format!("exceeds maximum length of {max_len} characters"), - }); - } - Ok(()) -} - -fn resolve_str(value: Option, max_len: usize, field: &'static str) -> Option { - let value = value?; - let value = sanitize_single_line(&value); - if value.is_empty() { - tracing::warn!("ignoring {field}: value is empty"); - return None; - } - if value.chars().count() > max_len { - tracing::warn!("ignoring {field}: exceeds maximum length of {max_len} characters"); - return None; - } - Some(value) -} - -fn resolve_required_str( - value: Option, - max_len: usize, - field: &'static str, -) -> Option { - let Some(value) = value else { - tracing::warn!("ignoring {field}: value is missing"); - return None; - }; - resolve_str(Some(value), max_len, field) -} - -#[cfg(test)] -#[path = "loader_tests.rs"] -mod tests; diff --git a/codex-rs/core-skills/src/loader/discovery.rs b/codex-rs/core-skills/src/loader/discovery.rs deleted file mode 100644 index cadc8218c0..0000000000 --- a/codex-rs/core-skills/src/loader/discovery.rs +++ /dev/null @@ -1,219 +0,0 @@ -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 codex_utils_plugins::SkillDiscoveryMode; - -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) enum DirectorySymlinkPolicy { - Follow, - Ignore, -} - -pub(super) enum HiddenDirectoryPolicy { - Include, - Skip, -} - -pub(super) struct SkillDiscoveryOptions { - pub directory_symlinks: DirectorySymlinkPolicy, - pub hidden_directories: HiddenDirectoryPolicy, - pub mode: SkillDiscoveryMode, -} - -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, - options: SkillDiscoveryOptions, -) -> 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: match options.mode { - SkillDiscoveryMode::Recursive => MAX_SCAN_DEPTH, - SkillDiscoveryMode::DirectChildren => 2, - }, - max_directories: MAX_SKILLS_DIRS_PER_ROOT, - max_entries: MAX_SKILLS_ENTRIES_PER_ROOT, - follow_directory_symlinks: matches!( - options.directory_symlinks, - DirectorySymlinkPolicy::Follow - ), - prune_hidden_directories: matches!( - options.hidden_directories, - HiddenDirectoryPolicy::Skip - ), - }, - /*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 skip_hidden = matches!(options.hidden_directories, HiddenDirectoryPolicy::Skip); - 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 { - if skip_hidden && has_hidden_ancestor_below_root(&entry.path, root) { - continue; - } - 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) - && (options.mode == SkillDiscoveryMode::Recursive - || is_direct_child_skill_path(&entry.path, root)) - { - 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 is_direct_child_skill_path(path: &PathUri, root: &PathUri) -> bool { - path.parent().and_then(|parent| parent.parent()).as_ref() == Some(root) -} - -fn has_hidden_ancestor_below_root(path: &PathUri, root: &PathUri) -> bool { - let mut ancestor = path.parent(); - while let Some(current) = ancestor { - if ¤t == root { - return false; - } - if current.basename().is_some_and(|name| name.starts_with('.')) { - return true; - } - ancestor = current.parent(); - } - false -} - -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/core-skills/src/loader/environment.rs b/codex-rs/core-skills/src/loader/environment.rs deleted file mode 100644 index fa950ec7b1..0000000000 --- a/codex-rs/core-skills/src/loader/environment.rs +++ /dev/null @@ -1,252 +0,0 @@ -use std::io; - -use codex_exec_server::ExecutorFileSystem; -use codex_protocol::protocol::Product; -pub use codex_skills::EnvironmentSkillMetadata; -use codex_skills::ParsedSkillFrontmatter; -use codex_skills::parse_skill_frontmatter_metadata; -use codex_utils_path_uri::PathUri; -use futures::StreamExt; - -use crate::model::SkillDependencies; -use crate::model::SkillPolicy; - -use super::MAX_QUALIFIED_NAME_LEN; -use super::SkillMetadataFile; -use super::discovery::DirectorySymlinkPolicy; -use super::discovery::DiscoveredSkill; -use super::discovery::HiddenDirectoryPolicy; -use super::discovery::MAX_CONCURRENT_SKILL_LOADS; -use super::discovery::SkillDiscoveryOptions; -use super::discovery::SkillMetadataDiscovery; -use super::discovery::discover_skills; -use super::namespace::SkillNamespaceResolver; -use super::resolve_dependencies; -use super::resolve_policy; -use super::sanitize_single_line; -use super::validate_len; - -struct ParsedEnvironmentSkill { - path_to_skills_md: PathUri, - base_name: String, - description: String, - short_description: Option, - dependencies: Option, - policy: Option, -} - -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(); - let discovery = discover_skills( - file_system, - root, - // Preserve environment discovery behavior by following directory aliases and including - // hidden directories exposed by the executor. - SkillDiscoveryOptions { - directory_symlinks: DirectorySymlinkPolicy::Follow, - hidden_directories: HiddenDirectoryPolicy::Include, - mode: codex_utils_plugins::SkillDiscoveryMode::Recursive, - }, - ) - .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 -} - -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() - .and_then(|parent| parent.basename()) - .map(|name| sanitize_single_line(&name)) - .filter(|name| !name.is_empty()) - .unwrap_or_else(|| "skill".to_string()) -} - -#[cfg(test)] -#[path = "environment_tests.rs"] -mod tests; diff --git a/codex-rs/core-skills/src/loader/environment_tests.rs b/codex-rs/core-skills/src/loader/environment_tests.rs deleted file mode 100644 index a817830b37..0000000000 --- a/codex-rs/core-skills/src/loader/environment_tests.rs +++ /dev/null @@ -1,78 +0,0 @@ -use std::fs; - -use codex_exec_server::LOCAL_FS; -use codex_protocol::protocol::Product; -use codex_utils_path_uri::PathUri; -use pretty_assertions::assert_eq; -use tempfile::tempdir; - -use crate::model::SkillDependencies; -use crate::model::SkillPolicy; -use crate::model::SkillToolDependency; - -use super::EnvironmentSkillMetadata; -use super::load_environment_skills_from_root; - -#[tokio::test] -async fn loads_plugin_namespace_dependencies_and_policy() { - let root = tempdir().expect("tempdir"); - let skill_dir = root.path().join("skills/deploy"); - fs::create_dir_all(root.path().join(".codex-plugin")).expect("manifest dir"); - fs::create_dir_all(skill_dir.join("agents")).expect("metadata dir"); - fs::write( - root.path().join(".codex-plugin/plugin.json"), - r#"{"name":"demo-plugin"}"#, - ) - .expect("manifest"); - fs::write( - skill_dir.join("SKILL.md"), - "---\nname: deploy\ndescription: Deploy the service.\n---\n", - ) - .expect("skill"); - fs::write( - skill_dir.join("agents/openai.yaml"), - r#" -dependencies: - tools: - - type: mcp - value: deploy-server - description: Deploy MCP -policy: - allow_implicit_invocation: false - products: [codex] -"#, - ) - .expect("metadata"); - - let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI"); - let outcome = - load_environment_skills_from_root(LOCAL_FS.as_ref(), &root_uri, Some(Product::Codex)).await; - - assert_eq!( - outcome.skills, - vec![EnvironmentSkillMetadata { - path_to_skills_md: PathUri::from_host_native_path(skill_dir.join("SKILL.md"),).unwrap(), - name: "demo-plugin:deploy".to_string(), - description: "Deploy the service.".to_string(), - short_description: None, - dependencies: Some(SkillDependencies { - tools: vec![SkillToolDependency { - r#type: "mcp".to_string(), - value: "deploy-server".to_string(), - description: Some("Deploy MCP".to_string()), - transport: None, - command: None, - url: None, - }], - }), - policy: Some(SkillPolicy { - allow_implicit_invocation: Some(false), - products: vec![Product::Codex], - }), - }] - ); - let filtered = - load_environment_skills_from_root(LOCAL_FS.as_ref(), &root_uri, Some(Product::Chatgpt)) - .await; - assert!(filtered.skills.is_empty()); -} diff --git a/codex-rs/core-skills/src/loader/namespace.rs b/codex-rs/core-skills/src/loader/namespace.rs deleted file mode 100644 index fa00b1005d..0000000000 --- a/codex-rs/core-skills/src/loader/namespace.rs +++ /dev/null @@ -1,182 +0,0 @@ -use codex_exec_server::ExecutorFileSystem; -use codex_utils_path_uri::PathUri; -use codex_utils_plugins::plugin_namespace_for_root_uri; -use futures::StreamExt; -use std::collections::HashMap; -use std::collections::HashSet; - -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. -/// -/// Namespace precedence is: -/// -/// 1. an explicitly provided plugin namespace; -/// 2. the deepest matching canonical symlink root or nested plugin root; -/// 3. the namespace inherited from the scanned skills root. -pub(crate) struct SkillNamespaceResolver { - inherited_namespace: ResolvedSkillNamespace, - nested_namespaces: Vec<(PathUri, ResolvedSkillNamespace)>, -} - -impl SkillNamespaceResolver { - /// Builds a resolver whose explicit plugin-owned namespace overrides discovery. - pub(crate) fn with_provided_namespace(namespace: &str) -> Self { - Self { - inherited_namespace: ResolvedSkillNamespace::Plugin(namespace.to_string()), - nested_namespaces: Vec::new(), - } - } - - 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/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs deleted file mode 100644 index 9f09cbfdf6..0000000000 --- a/codex-rs/core-skills/src/loader_tests.rs +++ /dev/null @@ -1,2089 +0,0 @@ -use super::*; -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_protocol::protocol::Product; -use codex_protocol::protocol::SkillScope; -use codex_utils_absolute_path::AbsolutePathBuf; -use codex_utils_absolute_path::test_support::PathBufExt; -use codex_utils_absolute_path::test_support::PathExt; -use codex_utils_path_uri::PathUri; -use dunce::canonicalize as canonicalize_path; -use pretty_assertions::assert_eq; -use std::fs; -use std::path::Path; -use std::path::PathBuf; -use std::sync::Arc; -use std::sync::atomic::AtomicUsize; -use std::sync::atomic::Ordering; -use tempfile::TempDir; -use tokio::sync::Notify; -use tokio::sync::Semaphore; - -const REPO_ROOT_CONFIG_DIR_NAME: &str = ".codex"; -const SKILLS_DIR_NAME: &str = "skills"; - -struct BlockingRepoSkillRootFileSystem { - inner: Arc, - blocked_walk_root: Option, - blocked_walk_gate: Semaphore, - walks_started: AtomicUsize, - walk_started: Notify, -} - -impl ExecutorFileSystem for BlockingRepoSkillRootFileSystem { - 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.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 walk<'a>( - &'a self, - path: &'a PathUri, - options: WalkOptions, - sandbox: Option<&'a FileSystemSandboxContext>, - ) -> ExecutorFileSystemFuture<'a, WalkOutcome> { - self.walks_started.fetch_add(/*val*/ 1, Ordering::AcqRel); - self.walk_started.notify_waiters(); - if self.blocked_walk_root.as_ref() != Some(path) { - return self.inner.walk(path, options, sandbox); - } - Box::pin(async move { - self.blocked_walk_gate - .acquire() - .await - .expect("blocked walk gate should remain open") - .forget(); - self.inner.walk(path, options, sandbox).await - }) - } - - 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) - } -} - -async fn load_skills_for_test(roots: I) -> SkillLoadOutcome -where - I: IntoIterator + Send, - I::IntoIter: Send, -{ - super::load_skills_from_roots( - roots, - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await -} - -fn local_skill_root(path: &Path, scope: SkillScope) -> SkillRoot { - SkillRoot { - path: path.abs(), - scope, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - } -} - -async fn load_user_skills_for_test(codex_home: &TempDir) -> SkillLoadOutcome { - load_skills_for_test([local_skill_root( - &codex_home.path().join(SKILLS_DIR_NAME), - SkillScope::User, - )]) - .await -} - -fn normalized(path: &Path) -> AbsolutePathBuf { - canonicalize_path(path) - .unwrap_or_else(|_| path.to_path_buf()) - .abs() -} -fn write_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) -> PathBuf { - write_skill_at(&codex_home.path().join("skills"), dir, name, description) -} - -fn write_system_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) -> PathBuf { - write_skill_at( - &codex_home.path().join("skills/.system"), - dir, - name, - description, - ) -} - -fn write_skill_at(root: &Path, dir: &str, name: &str, description: &str) -> PathBuf { - let skill_dir = root.join(dir); - fs::create_dir_all(&skill_dir).unwrap(); - let indented_description = description.replace('\n', "\n "); - let content = - format!("---\nname: {name}\ndescription: |-\n {indented_description}\n---\n\n# Body\n"); - let path = skill_dir.join(SKILLS_FILENAME); - fs::write(&path, content).unwrap(); - path -} - -fn write_raw_skill_at(root: &Path, dir: &str, frontmatter: &str) -> PathBuf { - let skill_dir = root.join(dir); - fs::create_dir_all(&skill_dir).unwrap(); - let path = skill_dir.join(SKILLS_FILENAME); - let content = format!("---\n{frontmatter}\n---\n\n# Body\n"); - fs::write(&path, content).unwrap(); - path -} - -fn write_skill_metadata_at(skill_dir: &Path, contents: &str) -> PathBuf { - let path = skill_dir - .join(SKILLS_METADATA_DIR) - .join(SKILLS_METADATA_FILENAME); - if let Some(parent) = path.parent() { - fs::create_dir_all(parent).unwrap(); - } - fs::write(&path, contents).unwrap(); - path -} - -fn write_skill_interface_at(skill_dir: &Path, contents: &str) -> PathBuf { - write_skill_metadata_at(skill_dir, contents) -} - -fn write_plugin_manifest(plugin_root: &Path, contents: &str) { - let manifest_path = plugin_root.join(".codex-plugin/plugin.json"); - fs::create_dir_all(manifest_path.parent().expect("manifest parent")).unwrap(); - fs::write(manifest_path, contents).unwrap(); -} - -async fn load_user_skills_root(root: &Path) -> SkillLoadOutcome { - load_skills_from_roots( - [SkillRoot { - path: root.abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }], - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await -} - -fn expected_user_skill(path: &Path, name: &str, description: &str) -> SkillMetadata { - SkillMetadata { - name: name.to_string(), - description: description.to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - } -} - -#[tokio::test] -async fn loads_skill_dependencies_metadata_from_yaml() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "dep-skill", "from json"); - let skill_dir = skill_path.parent().expect("skill dir"); - - write_skill_metadata_at( - skill_dir, - r#" -{ - "dependencies": { - "tools": [ - { - "type": "mcp", - "value": "github", - "description": "GitHub MCP server", - "transport": "streamable_http", - "url": "https://example.com/mcp" - }, - { - "type": "cli", - "value": "gh", - "description": "GitHub CLI" - }, - { - "type": "mcp", - "value": "local-gh", - "description": "Local GH MCP server", - "transport": "stdio", - "command": "gh-mcp" - } - ] - } -} -"#, - ); - - let outcome = load_user_skills_for_test(&codex_home).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "dep-skill".to_string(), - description: "from json".to_string(), - short_description: None, - interface: None, - dependencies: Some(SkillDependencies { - tools: vec![ - SkillToolDependency { - r#type: "mcp".to_string(), - value: "github".to_string(), - description: Some("GitHub MCP server".to_string()), - transport: Some("streamable_http".to_string()), - command: None, - url: Some("https://example.com/mcp".to_string()), - }, - SkillToolDependency { - r#type: "cli".to_string(), - value: "gh".to_string(), - description: Some("GitHub CLI".to_string()), - transport: None, - command: None, - url: None, - }, - SkillToolDependency { - r#type: "mcp".to_string(), - value: "local-gh".to_string(), - description: Some("Local GH MCP server".to_string()), - transport: Some("stdio".to_string()), - command: Some("gh-mcp".to_string()), - url: None, - }, - ], - }), - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn loads_skill_interface_metadata_from_yaml() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); - let skill_dir = skill_path.parent().expect("skill dir"); - let normalized_skill_dir = normalized(skill_dir); - - write_skill_interface_at( - skill_dir, - r##" -interface: - display_name: "UI Skill" - short_description: " short desc " - icon_small: "./assets/small-400px.png" - icon_large: "./assets/large-logo.svg" - brand_color: "#3B82F6" - default_prompt: " default prompt " -"##, - ); - - let outcome = load_user_skills_for_test(&codex_home).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - let user_skills: Vec = outcome - .skills - .into_iter() - .filter(|skill| skill.scope == SkillScope::User) - .collect(); - assert_eq!( - user_skills, - vec![SkillMetadata { - name: "ui-skill".to_string(), - description: "from json".to_string(), - short_description: None, - interface: Some(SkillInterface { - display_name: Some("UI Skill".to_string()), - short_description: Some("short desc".to_string()), - icon_small: Some(normalized_skill_dir.join("assets/small-400px.png")), - icon_large: Some(normalized_skill_dir.join("assets/large-logo.svg")), - brand_color: Some("#3B82F6".to_string()), - default_prompt: Some("default prompt".to_string()), - }), - dependencies: None, - policy: None, - path_to_skills_md: normalized(skill_path.as_path()), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn loads_skill_policy_from_yaml() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "policy-skill", "from json"); - let skill_dir = skill_path.parent().expect("skill dir"); - - write_skill_metadata_at( - skill_dir, - r#" -policy: - allow_implicit_invocation: false -"#, - ); - - let outcome = load_user_skills_for_test(&codex_home).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!( - outcome.skills[0].policy, - Some(SkillPolicy { - allow_implicit_invocation: Some(false), - products: vec![], - }) - ); - assert!(outcome.allowed_skills_for_implicit_invocation().is_empty()); -} - -#[tokio::test] -async fn empty_skill_policy_defaults_to_allow_implicit_invocation() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "policy-empty", "from json"); - let skill_dir = skill_path.parent().expect("skill dir"); - - write_skill_metadata_at( - skill_dir, - r#" -policy: {} -"#, - ); - - let outcome = load_user_skills_for_test(&codex_home).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!( - outcome.skills[0].policy, - Some(SkillPolicy { - allow_implicit_invocation: None, - products: vec![], - }) - ); - assert_eq!( - outcome.allowed_skills_for_implicit_invocation(), - outcome.skills - ); -} - -#[tokio::test] -async fn loads_skill_policy_products_from_yaml() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "policy-products", "from yaml"); - let skill_dir = skill_path.parent().expect("skill dir"); - - write_skill_metadata_at( - skill_dir, - r#" -policy: - products: - - codex - - CHATGPT - - atlas -"#, - ); - - let outcome = load_user_skills_for_test(&codex_home).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!( - outcome.skills[0].policy, - Some(SkillPolicy { - allow_implicit_invocation: None, - products: vec![Product::Codex, Product::Chatgpt, Product::Atlas], - }) - ); -} - -#[tokio::test] -async fn loads_plugin_skill_interface_icons_from_shared_plugin_assets() { - let root = tempfile::tempdir().expect("tempdir"); - let plugin_root = root.path().join("plugins/twilio-developer-kit"); - let skill_path = write_skill_at( - &plugin_root.join("skills"), - "twilio-send-message", - "send-message", - "send messages", - ); - let skill_dir = skill_path.parent().expect("skill dir"); - fs::create_dir_all(plugin_root.join("assets")).unwrap(); - fs::write(plugin_root.join("assets/logo.svg"), "").unwrap(); - write_skill_interface_at( - skill_dir, - r##" -interface: - icon_small: "../../assets/logo.svg" - icon_large: "../../assets/logo.svg" -"##, - ); - - let plugin_root_abs = plugin_root.abs(); - let outcome = load_skills_from_roots( - [SkillRoot { - path: plugin_root.join("skills").abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: Some(PluginIdentity { - plugin_id: "twilio-developer-kit@test".to_string(), - remote_plugin_id: None, - }), - plugin_namespace: None, - plugin_root: Some(plugin_root_abs.clone()), - discovery_mode: SkillDiscoveryMode::Recursive, - }], - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - let expected_icon_path = normalized(&plugin_root.join("assets/logo.svg")); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "send-message".to_string(), - description: "send messages".to_string(), - short_description: None, - interface: Some(SkillInterface { - display_name: None, - short_description: None, - icon_small: Some(expected_icon_path.clone()), - icon_large: Some(expected_icon_path), - brand_color: None, - default_prompt: None, - }), - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: Some("twilio-developer-kit@test".to_string()), - remote_plugin_id: None, - }] - ); -} - -#[cfg(unix)] -fn symlink_dir(target: &Path, link: &Path) { - std::os::unix::fs::symlink(target, link).unwrap(); -} - -#[cfg(unix)] -fn symlink_file(target: &Path, link: &Path) { - std::os::unix::fs::symlink(target, link).unwrap(); -} - -#[tokio::test] -#[cfg(unix)] -async fn loads_skills_via_symlinked_subdir_for_user_scope() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let shared = tempfile::tempdir().expect("tempdir"); - - let shared_skill_path = write_skill_at(shared.path(), "demo", "linked-skill", "from link"); - - fs::create_dir_all(codex_home.path().join("skills")).unwrap(); - symlink_dir(shared.path(), &codex_home.path().join("skills/shared")); - - let outcome = load_user_skills_for_test(&codex_home).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "linked-skill".to_string(), - description: "from link".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&shared_skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); - let canonical_skill_path = normalized(&shared_skill_path); - let discovery_path = outcome - .skill_root_for_path(&canonical_skill_path) - .expect("symlinked skill should retain its discovery root") - .join("shared/demo/SKILL.md"); - assert_eq!( - outcome.skill_discovery_path_for_path(&canonical_skill_path), - Some(&discovery_path) - ); - let filtered_outcome = - crate::filter_skill_load_outcome_for_product(outcome, Some(Product::Codex)); - assert_eq!( - filtered_outcome.skill_discovery_path_for_path(&canonical_skill_path), - Some(&discovery_path) - ); -} - -// Directory symlinks on Windows can require Developer Mode or administrator privileges. -#[tokio::test] -#[cfg(unix)] -async fn loads_skills_through_visible_alias_to_hidden_directory() { - let root = tempfile::tempdir().expect("tempdir"); - let hidden_root = root.path().join(".hidden"); - let skill_path = write_skill_at(&hidden_root, "search", "search-skill", "search description"); - symlink_dir(&hidden_root, &root.path().join("visible")); - - let outcome = load_user_skills_root(root.path()).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![expected_user_skill( - &skill_path, - "search-skill", - "search description", - )] - ); -} - -#[tokio::test] -#[cfg(unix)] -async fn ignores_symlinked_skill_file_for_user_scope() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let shared = tempfile::tempdir().expect("tempdir"); - - let shared_skill_path = write_skill_at(shared.path(), "demo", "linked-file-skill", "from link"); - - let skill_dir = codex_home.path().join("skills/demo"); - fs::create_dir_all(&skill_dir).unwrap(); - symlink_file(&shared_skill_path, &skill_dir.join(SKILLS_FILENAME)); - - let outcome = load_user_skills_for_test(&codex_home).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills, Vec::new()); -} - -#[tokio::test] -#[cfg(unix)] -async fn does_not_loop_on_symlink_cycle_for_user_scope() { - let codex_home = tempfile::tempdir().expect("tempdir"); - - // Create a cycle: - // $CODEX_HOME/skills/cycle/loop -> $CODEX_HOME/skills/cycle - let cycle_dir = codex_home.path().join("skills/cycle"); - fs::create_dir_all(&cycle_dir).unwrap(); - symlink_dir(&cycle_dir, &cycle_dir.join("loop")); - - let skill_path = write_skill_at(&cycle_dir, "demo", "cycle-skill", "still loads"); - - let outcome = load_user_skills_for_test(&codex_home).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "cycle-skill".to_string(), - description: "still loads".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -#[cfg(unix)] -async fn loads_skills_via_symlinked_subdir_for_admin_scope() { - let admin_root = tempfile::tempdir().expect("tempdir"); - let shared = tempfile::tempdir().expect("tempdir"); - - let shared_skill_path = - write_skill_at(shared.path(), "demo", "admin-linked-skill", "from link"); - fs::create_dir_all(admin_root.path()).unwrap(); - symlink_dir(shared.path(), &admin_root.path().join("shared")); - - let outcome = load_skills_from_roots( - [SkillRoot { - path: admin_root.path().abs(), - scope: SkillScope::Admin, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }], - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "admin-linked-skill".to_string(), - description: "from link".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&shared_skill_path), - scope: SkillScope::Admin, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -#[cfg(unix)] -async fn loads_skills_via_symlinked_subdir_for_repo_scope() { - let repo_dir = tempfile::tempdir().expect("tempdir"); - let shared = tempfile::tempdir().expect("tempdir"); - - let linked_skill_path = write_skill_at(shared.path(), "demo", "repo-linked-skill", "from link"); - let repo_skills_root = repo_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME); - fs::create_dir_all(&repo_skills_root).unwrap(); - symlink_dir(shared.path(), &repo_skills_root.join("shared")); - - let outcome = - load_skills_for_test([local_skill_root(&repo_skills_root, SkillScope::Repo)]).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "repo-linked-skill".to_string(), - description: "from link".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&linked_skill_path), - scope: SkillScope::Repo, - plugin_id: None, - remote_plugin_id: None, - }] - ); - let canonical_skill_path = normalized(&linked_skill_path); - let discovery_path = outcome - .skill_root_for_path(&canonical_skill_path) - .expect("repo skill should retain its discovery root") - .join("shared/demo/SKILL.md"); - assert_eq!( - outcome.skill_discovery_path_for_path(&canonical_skill_path), - Some(&discovery_path) - ); -} - -#[tokio::test] -#[cfg(unix)] -async fn system_scope_ignores_symlinked_subdir() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let shared = tempfile::tempdir().expect("tempdir"); - - write_skill_at(shared.path(), "demo", "system-linked-skill", "from link"); - - let system_root = codex_home.path().join("skills/.system"); - fs::create_dir_all(&system_root).unwrap(); - symlink_dir(shared.path(), &system_root.join("shared")); - - let outcome = load_skills_from_roots( - [SkillRoot { - path: system_root.abs(), - scope: SkillScope::System, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }], - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills.len(), 0); -} - -#[tokio::test] -async fn respects_max_scan_depth_for_user_scope() { - let codex_home = tempfile::tempdir().expect("tempdir"); - - let within_depth_path = write_skill( - &codex_home, - "d0/d1/d2/d3/d4/d5", - "within-depth-skill", - "loads", - ); - let _too_deep_path = write_skill( - &codex_home, - "d0/d1/d2/d3/d4/d5/d6", - "too-deep-skill", - "should not load", - ); - - let skills_root = codex_home.path().join("skills"); - let outcome = load_skills_from_roots( - [SkillRoot { - path: skills_root.abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }], - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "within-depth-skill".to_string(), - description: "loads".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&within_depth_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn loads_valid_skill() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "demo-skill", "does things\ncarefully"); - let outcome = load_user_skills_for_test(&codex_home).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "demo-skill".to_string(), - description: "does things carefully".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn falls_back_to_directory_name_when_skill_name_is_missing() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_raw_skill_at( - &codex_home.path().join("skills"), - "directory-derived", - "description: fallback name", - ); - let outcome = load_user_skills_for_test(&codex_home).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "directory-derived".to_string(), - description: "fallback name".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn namespaces_plugin_skills_using_provided_namespace() { - let root = tempfile::tempdir().expect("tempdir"); - let plugin_root = root.path().join("plugins/sample"); - let skill_path = write_raw_skill_at( - &plugin_root.join("skills"), - "sample-search", - "description: search sample data", - ); - fs::create_dir_all(plugin_root.join(".codex-plugin")).unwrap(); - fs::write( - plugin_root.join(".codex-plugin/plugin.json"), - r#"{"name":"should-not-be-read"}"#, - ) - .unwrap(); - - let outcome = load_skills_from_roots( - [SkillRoot { - path: plugin_root.join("skills").abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: Some(PluginIdentity { - plugin_id: "sample@test".to_string(), - remote_plugin_id: None, - }), - plugin_namespace: Some("sample".to_string()), - plugin_root: Some(plugin_root.abs()), - discovery_mode: SkillDiscoveryMode::Recursive, - }], - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "sample:sample-search".to_string(), - description: "search sample data".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: Some("sample@test".to_string()), - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn namespaces_nested_plugin_skills_without_namespacing_plain_siblings() { - let root = tempfile::tempdir().expect("tempdir"); - let skills_root = root.path().join("skills"); - let plain_skill_path = - write_skill_at(&skills_root, "plain", "plain-skill", "plain description"); - let plugin_root = skills_root.join("nested-plugin"); - write_plugin_manifest(&plugin_root, r#"{"name":"nested"}"#); - let plugin_skill_path = write_skill_at( - &plugin_root.join("skills"), - "search", - "plugin-skill", - "plugin description", - ); - - let outcome = load_user_skills_root(&skills_root).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![ - expected_user_skill( - &plugin_skill_path, - "nested:plugin-skill", - "plugin description" - ), - expected_user_skill(&plain_skill_path, "plain-skill", "plain description"), - ] - ); -} - -#[tokio::test] -async fn inherits_plugin_namespace_from_above_scanned_skills_root() { - let root = tempfile::tempdir().expect("tempdir"); - let plugin_root = root.path().join("plugin"); - write_plugin_manifest(&plugin_root, r#"{"name":"outer"}"#); - let skills_root = plugin_root.join("skills"); - let skill_path = write_skill_at(&skills_root, "search", "search-skill", "search description"); - - let outcome = load_user_skills_root(&skills_root).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![expected_user_skill( - &skill_path, - "outer:search-skill", - "search description", - )] - ); -} - -#[tokio::test] -async fn nearest_valid_nested_plugin_namespace_overrides_outer_namespace() { - let root = tempfile::tempdir().expect("tempdir"); - let outer_plugin_root = root.path().join("outer-plugin"); - write_plugin_manifest(&outer_plugin_root, r#"{"name":"outer"}"#); - let skills_root = outer_plugin_root.join("skills"); - let nested_plugin_root = skills_root.join("nested-plugin"); - write_plugin_manifest(&nested_plugin_root, r#"{"name":"nested"}"#); - let skill_path = write_skill_at( - &nested_plugin_root.join("skills"), - "search", - "search-skill", - "search description", - ); - - let outcome = load_user_skills_root(&skills_root).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![expected_user_skill( - &skill_path, - "nested:search-skill", - "search description", - )] - ); -} - -#[tokio::test] -async fn invalid_nested_plugin_manifest_falls_back_to_outer_namespace() { - let root = tempfile::tempdir().expect("tempdir"); - let outer_plugin_root = root.path().join("outer-plugin"); - write_plugin_manifest(&outer_plugin_root, r#"{"name":"outer"}"#); - let skills_root = outer_plugin_root.join("skills"); - let nested_plugin_root = skills_root.join("nested-plugin"); - write_plugin_manifest(&nested_plugin_root, "not json"); - let skill_path = write_skill_at( - &nested_plugin_root.join("skills"), - "search", - "search-skill", - "search description", - ); - - let outcome = load_user_skills_root(&skills_root).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![expected_user_skill( - &skill_path, - "outer:search-skill", - "search description", - )] - ); -} - -// Directory symlinks on Windows can require Developer Mode or administrator privileges. -#[cfg(unix)] -#[tokio::test] -async fn does_not_inherit_namespace_for_skills_in_symlinked_plain_dir() { - // outer-plugin/ - // ├── .codex-plugin/plugin.json - // └── skills/linked-plain -> plain-root/ - // plain-root/ - // └── search/SKILL.md - let root = tempfile::tempdir().expect("tempdir"); - let plugin_root = root.path().join("outer-plugin"); - write_plugin_manifest(&plugin_root, r#"{"name":"outer"}"#); - let skills_root = plugin_root.join("skills"); - let plain_root = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill_at( - plain_root.path(), - "search", - "plain-skill", - "plain description", - ); - fs::create_dir_all(&skills_root).unwrap(); - symlink_dir(plain_root.path(), &skills_root.join("linked-plain")); - - let outcome = load_user_skills_root(&skills_root).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![expected_user_skill( - &skill_path, - "plain-skill", - "plain description", - )] - ); -} - -// Directory symlinks on Windows can require Developer Mode or administrator privileges. -#[cfg(unix)] -#[tokio::test] -async fn keeps_inherited_namespace_when_symlink_target_is_scan_root_ancestor() { - // temp-root/ - // └── a/b/c/d/e/f/outer-plugin/ - // ├── .codex-plugin/plugin.json - // └── skills/ - // ├── root/SKILL.md - // └── link -> temp-root/ - let root = tempfile::tempdir().expect("tempdir"); - let plugin_root = root.path().join("a/b/c/d/e/f/outer-plugin"); - write_plugin_manifest(&plugin_root, r#"{"name":"outer"}"#); - let skills_root = plugin_root.join("skills"); - let skill_path = write_skill_at(&skills_root, "root", "root-skill", "root description"); - symlink_dir(root.path(), &skills_root.join("link")); - - let outcome = load_user_skills_root(&skills_root).await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![expected_user_skill( - &skill_path, - "outer:root-skill", - "root description", - )] - ); -} - -#[tokio::test] -async fn plugin_skill_name_length_limit_allows_max_qualified_name() { - let root = tempfile::tempdir().expect("tempdir"); - let plugin_name = "p".repeat(MAX_NAME_LEN); - let skill_name = "s".repeat(MAX_NAME_LEN); - let plugin_root = root.path().join("plugins").join(&plugin_name); - let frontmatter = format!("name: {skill_name}\ndescription: search sample data"); - let skill_path = write_raw_skill_at(&plugin_root.join("skills"), "sample-search", &frontmatter); - fs::create_dir_all(plugin_root.join(".codex-plugin")).unwrap(); - fs::write( - plugin_root.join(".codex-plugin/plugin.json"), - format!(r#"{{"name":"{plugin_name}"}}"#), - ) - .unwrap(); - - let outcome = load_skills_from_roots( - [SkillRoot { - path: plugin_root.join("skills").abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: Some(PluginIdentity { - plugin_id: "sample@test".to_string(), - remote_plugin_id: None, - }), - plugin_namespace: Some(plugin_name.clone()), - plugin_root: Some(plugin_root.abs()), - discovery_mode: SkillDiscoveryMode::Recursive, - }], - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: format!("{plugin_name}:{skill_name}"), - description: "search sample data".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: Some("sample@test".to_string()), - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn plugin_skill_name_length_limit_rejects_overlong_qualified_name() { - let root = tempfile::tempdir().expect("tempdir"); - let plugin_name = "p".repeat(MAX_NAME_LEN + 1); - let skill_name = "s".repeat(MAX_NAME_LEN); - let plugin_root = root.path().join("plugins").join(&plugin_name); - let frontmatter = format!("name: {skill_name}\ndescription: search sample data"); - write_raw_skill_at(&plugin_root.join("skills"), "sample-search", &frontmatter); - fs::create_dir_all(plugin_root.join(".codex-plugin")).unwrap(); - fs::write( - plugin_root.join(".codex-plugin/plugin.json"), - format!(r#"{{"name":"{plugin_name}"}}"#), - ) - .unwrap(); - - let outcome = load_skills_from_roots( - [SkillRoot { - path: plugin_root.join("skills").abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: Some(PluginIdentity { - plugin_id: "sample@test".to_string(), - remote_plugin_id: None, - }), - plugin_namespace: Some(plugin_name.clone()), - plugin_root: Some(plugin_root.abs()), - discovery_mode: SkillDiscoveryMode::Recursive, - }], - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await; - - assert_eq!(outcome.skills, Vec::new()); - assert_eq!(outcome.errors.len(), 1); - assert!( - outcome.errors[0].message.contains("invalid qualified name"), - "expected qualified name length error, got: {:?}", - outcome.errors - ); -} - -#[tokio::test] -async fn direct_child_discovery_ignores_nested_skills() { - let root = tempfile::tempdir().expect("tempdir"); - let plugin_root = root.path().join("plugin"); - let skills_root = plugin_root.join("skills"); - let direct = write_skill_at(&skills_root, "direct", "direct", "direct skill"); - write_skill_at(&skills_root, "nested/too-deep", "too-deep", "nested skill"); - - let outcome = load_skills_from_roots( - [SkillRoot { - path: skills_root.abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: Some(PluginIdentity { - plugin_id: "plugin@test".to_string(), - remote_plugin_id: None, - }), - plugin_namespace: Some("plugin".to_string()), - plugin_root: Some(plugin_root.abs()), - discovery_mode: SkillDiscoveryMode::DirectChildren, - }], - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await; - - assert!(outcome.errors.is_empty()); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "plugin:direct".to_string(), - description: "direct skill".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&direct), - scope: SkillScope::User, - plugin_id: Some("plugin@test".to_string()), - remote_plugin_id: None, - }] - ); -} - -#[cfg(unix)] -#[tokio::test] -async fn direct_child_discovery_skips_skills_resolving_outside_plugin_root() { - let root = tempfile::tempdir().expect("tempdir"); - let plugin_root = root.path().join("plugin"); - let skills_root = plugin_root.join("skills"); - let outside_root = root.path().join("outside"); - write_skill_at(&outside_root, "escaped", "escaped", "escaped skill"); - fs::create_dir_all(&skills_root).expect("create skills root"); - std::os::unix::fs::symlink(outside_root.join("escaped"), skills_root.join("escaped")) - .expect("create skill symlink"); - - let outcome = load_skills_from_roots( - [SkillRoot { - path: skills_root.abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: Some(PluginIdentity { - plugin_id: "plugin@test".to_string(), - remote_plugin_id: None, - }), - plugin_namespace: Some("plugin".to_string()), - plugin_root: Some(plugin_root.abs()), - discovery_mode: SkillDiscoveryMode::DirectChildren, - }], - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await; - - assert!(outcome.skills.is_empty()); -} - -#[tokio::test] -async fn loads_short_description_from_metadata() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_dir = codex_home.path().join("skills/demo"); - fs::create_dir_all(&skill_dir).unwrap(); - let contents = "---\nname: demo-skill\ndescription: long description\nmetadata:\n short-description: short summary\n---\n\n# Body\n"; - let skill_path = skill_dir.join(SKILLS_FILENAME); - fs::write(&skill_path, contents).unwrap(); - - let outcome = load_user_skills_for_test(&codex_home).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "demo-skill".to_string(), - description: "long description".to_string(), - short_description: Some("short summary".to_string()), - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn loads_unquoted_description_containing_colon_space() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_raw_skill_at( - &codex_home.path().join("skills"), - "colon-description", - "name: colon-description\ndescription: AWS deployment patterns: ECS Fargate, Lambda, and S3", - ); - - let outcome = load_user_skills_for_test(&codex_home).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "colon-description".to_string(), - description: "AWS deployment patterns: ECS Fargate, Lambda, and S3".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn loads_unquoted_short_description_containing_colon_space_and_apostrophe() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_raw_skill_at( - &codex_home.path().join("skills"), - "colon-short-description", - "name: colon-short-description\ndescription: long description\nmetadata:\n short-description: What's included: builds and tests", - ); - - let outcome = load_user_skills_for_test(&codex_home).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "colon-short-description".to_string(), - description: "long description".to_string(), - short_description: Some("What's included: builds and tests".to_string()), - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn loads_unrecognized_frontmatter_fields_that_need_quotes() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_raw_skill_at( - &codex_home.path().join("skills"), - "repaired-unknown-fields", - "name: repaired-unknown-fields\ndescription: valid description\nargument-hint: \ntags: [next,@supabase/ssr]", - ); - - let outcome = load_user_skills_for_test(&codex_home).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "repaired-unknown-fields".to_string(), - description: "valid description".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn preserves_block_scalar_body_while_repairing_other_fields() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_raw_skill_at( - &codex_home.path().join("skills"), - "block-description-with-repair", - "name: block-description-with-repair\ndescription: |-\n Build for AWS: ECS\nargument-hint: ", - ); - - let outcome = load_user_skills_for_test(&codex_home).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "block-description-with-repair".to_string(), - description: "Build for AWS: ECS".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[tokio::test] -async fn preserves_overlong_short_descriptions() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_dir = codex_home.path().join("skills/demo"); - fs::create_dir_all(&skill_dir).unwrap(); - let too_long = "x".repeat(MAX_DESCRIPTION_LEN + 1); - let contents = format!( - "---\nname: demo-skill\ndescription: long description\nmetadata:\n short-description: {too_long}\n---\n\n# Body\n" - ); - fs::write(skill_dir.join(SKILLS_FILENAME), contents).unwrap(); - - let outcome = load_user_skills_for_test(&codex_home).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!(outcome.skills[0].short_description, Some(too_long)); -} - -#[tokio::test] -async fn skips_hidden_and_invalid() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let hidden_dir = codex_home.path().join("skills/.hidden"); - fs::create_dir_all(&hidden_dir).unwrap(); - fs::write( - hidden_dir.join(SKILLS_FILENAME), - "---\nname: hidden\ndescription: hidden\n---\n", - ) - .unwrap(); - - // Invalid because missing closing frontmatter. - let invalid_dir = codex_home.path().join("skills/invalid"); - fs::create_dir_all(&invalid_dir).unwrap(); - fs::write(invalid_dir.join(SKILLS_FILENAME), "---\nname: bad").unwrap(); - - let outcome = load_user_skills_for_test(&codex_home).await; - assert_eq!(outcome.skills.len(), 0); - assert_eq!(outcome.errors.len(), 1); - assert!( - outcome.errors[0] - .message - .contains("missing YAML frontmatter"), - "expected frontmatter error" - ); -} - -#[tokio::test] -async fn preserves_overlong_descriptions() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let max_desc = "\u{1F4A1}".repeat(MAX_DESCRIPTION_LEN); - write_skill(&codex_home, "max-len", "max-len", &max_desc); - - let outcome = load_user_skills_for_test(&codex_home).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills.len(), 1); - - let too_long_desc = "\u{1F4A1}".repeat(MAX_DESCRIPTION_LEN + 1); - write_skill(&codex_home, "too-long", "too-long", &too_long_desc); - let outcome = load_user_skills_for_test(&codex_home).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills.len(), 2); - let too_long_skill = outcome - .skills - .iter() - .find(|skill| skill.name == "too-long") - .expect("too-long skill"); - assert_eq!(too_long_skill.description, too_long_desc); -} - -#[tokio::test] -async fn loads_skills_from_repo_scoped_root() { - let repo_dir = tempfile::tempdir().expect("tempdir"); - - let skills_root = repo_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME); - let skill_path = write_skill_at(&skills_root, "repo", "repo-skill", "from repo"); - - let outcome = load_skills_for_test([local_skill_root(&skills_root, SkillScope::Repo)]).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "repo-skill".to_string(), - description: "from repo".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::Repo, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} -#[tokio::test] -async fn loads_skills_from_multiple_repo_scoped_roots() { - let repo_dir = tempfile::tempdir().expect("tempdir"); - - let root_skills_root = repo_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME); - let nested_skills_root = repo_dir - .path() - .join("nested") - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME); - let root_skill_path = write_skill_at(&root_skills_root, "root", "root-skill", "from root"); - let nested_skill_path = - write_skill_at(&nested_skills_root, "nested", "nested-skill", "from nested"); - - let outcome = load_skills_for_test([ - local_skill_root(&nested_skills_root, SkillScope::Repo), - local_skill_root(&root_skills_root, SkillScope::Repo), - ]) - .await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![ - SkillMetadata { - name: "nested-skill".to_string(), - description: "from nested".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&nested_skill_path), - scope: SkillScope::Repo, - plugin_id: None, - remote_plugin_id: None, - }, - SkillMetadata { - name: "root-skill".to_string(), - description: "from root".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&root_skill_path), - scope: SkillScope::Repo, - plugin_id: None, - remote_plugin_id: None, - }, - ] - ); -} -#[tokio::test] -async fn merges_root_results_in_input_order_when_scans_finish_out_of_order() { - const ROOT_COUNT: usize = MAX_CONCURRENT_ROOT_SCANS + 1; - - let temp = tempfile::tempdir().expect("tempdir"); - let roots = (0..ROOT_COUNT) - .map(|index| temp.path().join(format!("root-{index}"))) - .collect::>(); - for root in &roots { - fs::create_dir_all(root).expect("create root"); - } - let first_skill = roots[0].join("broken/SKILL.md"); - let second_skill = roots[1].join("broken/SKILL.md"); - for (path, contents) in [ - (&first_skill, "missing frontmatter"), - (&second_skill, "also missing frontmatter"), - ] { - fs::create_dir_all(path.parent().expect("skill parent")).expect("create skill directory"); - fs::write(path, contents).expect("write skill"); - } - - let blocked_walk_root = PathUri::from_abs_path(&roots[0].abs()); - let file_system = Arc::new(BlockingRepoSkillRootFileSystem { - inner: Arc::clone(&LOCAL_FS), - blocked_walk_root: Some(blocked_walk_root), - blocked_walk_gate: Semaphore::new(/*permits*/ 0), - walks_started: AtomicUsize::new(/*v*/ 0), - walk_started: Notify::new(), - }); - let root_file_system: Arc = file_system.clone(); - let skill_roots = roots - .iter() - .enumerate() - .map(|(index, root)| SkillRoot { - path: root.abs(), - scope: if index == 0 { - SkillScope::Repo - } else { - SkillScope::User - }, - file_system: Arc::clone(&root_file_system), - plugin_identity: None, - plugin_namespace: Some("test".to_string()), - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }) - .collect::>(); - let root_scan_slots = Semaphore::new(MAX_CONCURRENT_ROOT_SCANS); - let load = tokio::spawn(async move { - crate::root_loader::load_and_merge_skill_roots( - skill_roots, - /*plugin_skill_snapshots*/ None, - &root_scan_slots, - ) - .await - }); - - tokio::time::timeout(std::time::Duration::from_secs(/*secs*/ 5), async { - loop { - let started = file_system.walk_started.notified(); - if file_system.walks_started.load(Ordering::Acquire) == ROOT_COUNT { - break; - } - started.await; - } - }) - .await - .expect("all skill-root walks should start despite the blocked first root"); - file_system.blocked_walk_gate.add_permits(/*n*/ 1); - let outcome = load.await.expect("skill-root load should finish"); - - assert_eq!(outcome.skills, Vec::new()); - assert_eq!( - outcome.errors, - vec![ - SkillError { - path: canonicalize_path(first_skill) - .expect("canonical first skill") - .abs(), - message: "missing YAML frontmatter delimited by ---".to_string(), - }, - SkillError { - path: canonicalize_path(second_skill) - .expect("canonical second skill") - .abs(), - message: "missing YAML frontmatter delimited by ---".to_string(), - }, - ] - ); -} - -#[tokio::test] -async fn skill_root_scans_wait_for_shared_capacity() { - let temp = tempfile::tempdir().expect("tempdir"); - let root = temp.path().join("root"); - fs::create_dir_all(&root).expect("create root"); - let root_scan_slots = Semaphore::new(MAX_CONCURRENT_ROOT_SCANS); - let held_slots = root_scan_slots - .try_acquire_many( - u32::try_from(MAX_CONCURRENT_ROOT_SCANS).expect("root scan limit should fit in u32"), - ) - .expect("root scan slots should be available"); - let load = crate::root_loader::load_and_merge_skill_roots( - [SkillRoot { - path: root.abs(), - scope: SkillScope::Repo, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: Some("test".to_string()), - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }], - /*plugin_skill_snapshots*/ None, - &root_scan_slots, - ); - tokio::pin!(load); - - assert!(futures::poll!(load.as_mut()).is_pending()); - drop(held_slots); - let outcome = load.await; - - assert_eq!(outcome.skills, Vec::new()); - assert_eq!(outcome.errors, Vec::new()); -} - -#[tokio::test] -async fn deduplicates_by_path_preferring_first_root() { - let root = tempfile::tempdir().expect("tempdir"); - - let skill_path = write_skill_at(root.path(), "dupe", "dupe-skill", "from repo"); - - let outcome = load_skills_from_roots( - [ - SkillRoot { - path: root.path().abs(), - scope: SkillScope::Repo, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }, - SkillRoot { - path: root.path().abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }, - ], - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "dupe-skill".to_string(), - description: "from repo".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::Repo, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} - -#[cfg(unix)] -#[tokio::test] -async fn deduplicated_symlinked_skill_preserves_first_discovery_path() { - let source_root = tempfile::tempdir().expect("source tempdir"); - let first_root = tempfile::tempdir().expect("first tempdir"); - let second_root = tempfile::tempdir().expect("second tempdir"); - let skill_path = write_skill_at(source_root.path(), "demo", "demo", "shared skill"); - symlink_dir( - &source_root.path().join("demo"), - &first_root.path().join("first-link"), - ); - symlink_dir( - &source_root.path().join("demo"), - &second_root.path().join("second-link"), - ); - - let outcome = load_skills_for_test([ - local_skill_root(first_root.path(), SkillScope::Repo), - local_skill_root(second_root.path(), SkillScope::User), - ]) - .await; - let canonical_skill_path = normalized(&skill_path); - let expected_root = normalized(first_root.path()); - let expected_discovery_path = expected_root.join("first-link/SKILL.md"); - - assert_eq!(outcome.skills.len(), 1); - assert_eq!( - ( - outcome.skill_root_for_path(&canonical_skill_path), - outcome.skill_discovery_path_for_path(&canonical_skill_path), - ), - (Some(&expected_root), Some(&expected_discovery_path)) - ); -} - -#[tokio::test] -async fn keeps_duplicate_names_from_repo_and_user() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let repo_dir = tempfile::tempdir().expect("tempdir"); - - let user_skill_path = write_skill(&codex_home, "user", "dupe-skill", "from user"); - let repo_skills_root = repo_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME); - let repo_skill_path = write_skill_at(&repo_skills_root, "repo", "dupe-skill", "from repo"); - - let outcome = load_skills_for_test([ - local_skill_root(&repo_skills_root, SkillScope::Repo), - local_skill_root(&codex_home.path().join(SKILLS_DIR_NAME), SkillScope::User), - ]) - .await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![ - SkillMetadata { - name: "dupe-skill".to_string(), - description: "from repo".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&repo_skill_path), - scope: SkillScope::Repo, - plugin_id: None, - remote_plugin_id: None, - }, - SkillMetadata { - name: "dupe-skill".to_string(), - description: "from user".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&user_skill_path), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }, - ] - ); -} - -#[tokio::test] -async fn keeps_duplicate_names_from_nested_codex_dirs() { - let repo_dir = tempfile::tempdir().expect("tempdir"); - - let root_skills_root = repo_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME); - let nested_skills_root = repo_dir - .path() - .join("nested") - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME); - let root_skill_path = write_skill_at(&root_skills_root, "root", "dupe-skill", "from root"); - let nested_skill_path = - write_skill_at(&nested_skills_root, "nested", "dupe-skill", "from nested"); - - let outcome = load_skills_for_test([ - local_skill_root(&nested_skills_root, SkillScope::Repo), - local_skill_root(&root_skills_root, SkillScope::Repo), - ]) - .await; - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - let root_path = normalized(&root_skill_path); - let nested_path = normalized(&nested_skill_path); - let (first_path, second_path, first_description, second_description) = - if root_path <= nested_path { - (root_path, nested_path, "from root", "from nested") - } else { - (nested_path, root_path, "from nested", "from root") - }; - assert_eq!( - outcome.skills, - vec![ - SkillMetadata { - name: "dupe-skill".to_string(), - description: first_description.to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: first_path, - scope: SkillScope::Repo, - plugin_id: None, - remote_plugin_id: None, - }, - SkillMetadata { - name: "dupe-skill".to_string(), - description: second_description.to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: second_path, - scope: SkillScope::Repo, - plugin_id: None, - remote_plugin_id: None, - }, - ] - ); -} -#[tokio::test] -async fn loads_skills_from_system_scoped_root() { - let codex_home = tempfile::tempdir().expect("tempdir"); - - let skill_path = write_system_skill(&codex_home, "system", "system-skill", "from system"); - let system_root = codex_home.path().join("skills/.system"); - - let outcome = load_skills_for_test([local_skill_root(&system_root, SkillScope::System)]).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "system-skill".to_string(), - description: "from system".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: normalized(&skill_path), - scope: SkillScope::System, - plugin_id: None, - remote_plugin_id: None, - }] - ); -} diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index c9d2677c2e..a26916da27 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -6,7 +6,6 @@ use std::sync::Arc; use codex_exec_server::ExecutorFileSystem; use codex_exec_server::LOCAL_FS; -use codex_protocol::protocol::Product; pub use codex_skills::SkillDependencies; pub use codex_skills::SkillError; pub use codex_skills::SkillInterface; @@ -120,14 +119,6 @@ impl SkillLoadOutcome { .get(&skill.path_to_skills_md) } - /// Builds the legacy aggregate from independently loaded roots. - /// - /// This is a temporary migration boundary while host root loading moves to the skills - /// extension. It deliberately reuses the existing merge, precedence, and deduplication logic. - pub fn from_root_snapshots(snapshots: Vec) -> Self { - crate::root_loader::merge_skill_root_snapshots(snapshots) - } - /// Reads one loaded skill through the filesystem that discovered it. pub async fn read_skill_text(&self, skill: &SkillMetadata) -> io::Result { let fs = self @@ -181,16 +172,6 @@ impl SkillFileSystemsByPath { fn get(&self, path: &AbsolutePathBuf) -> Option> { self.values.get(path).map(Arc::clone) } - - fn retain_paths(&mut self, paths: &HashSet) { - self.values = Arc::new( - self.values - .iter() - .filter(|(path, _)| paths.contains(*path)) - .map(|(path, fs)| (path.clone(), Arc::clone(fs))) - .collect(), - ); - } } impl fmt::Debug for SkillFileSystemsByPath { @@ -200,61 +181,3 @@ impl fmt::Debug for SkillFileSystemsByPath { .finish() } } - -pub fn filter_skill_load_outcome_for_product( - mut outcome: SkillLoadOutcome, - restriction_product: Option, -) -> SkillLoadOutcome { - outcome - .skills - .retain(|skill| skill.matches_product_restriction_for_product(restriction_product)); - let retained_paths: HashSet = outcome - .skills - .iter() - .map(|skill| skill.path_to_skills_md.clone()) - .collect(); - outcome - .file_systems_by_skill_path - .retain_paths(&retained_paths); - outcome.skill_root_by_path = Arc::new( - outcome - .skill_root_by_path - .iter() - .filter(|(path, _)| retained_paths.contains(*path)) - .map(|(path, root)| (path.clone(), root.clone())) - .collect(), - ); - outcome.skill_discovery_path_by_path = Arc::new( - outcome - .skill_discovery_path_by_path - .iter() - .filter(|(path, _)| retained_paths.contains(*path)) - .map(|(path, discovery_path)| (path.clone(), discovery_path.clone())) - .collect(), - ); - outcome - .agent_plugin_skill_paths - .retain(|path| retained_paths.contains(path)); - let retained_roots: HashSet = - outcome.skill_root_by_path.values().cloned().collect(); - outcome - .skill_roots - .retain(|root| retained_roots.contains(root)); - outcome.implicit_skills_by_scripts_dir = Arc::new( - outcome - .implicit_skills_by_scripts_dir - .iter() - .filter(|(_, skill)| skill.matches_product_restriction_for_product(restriction_product)) - .map(|(path, skill)| (path.clone(), skill.clone())) - .collect(), - ); - outcome.implicit_skills_by_doc_path = Arc::new( - outcome - .implicit_skills_by_doc_path - .iter() - .filter(|(_, skill)| skill.matches_product_restriction_for_product(restriction_product)) - .map(|(path, skill)| (path.clone(), skill.clone())) - .collect(), - ); - outcome -} diff --git a/codex-rs/core-skills/src/root_loader.rs b/codex-rs/core-skills/src/root_loader.rs deleted file mode 100644 index 5ba4206565..0000000000 --- a/codex-rs/core-skills/src/root_loader.rs +++ /dev/null @@ -1,221 +0,0 @@ -use std::collections::HashMap; -use std::collections::HashSet; -use std::fmt; -use std::hash::Hash; -use std::hash::Hasher; -use std::sync::Arc; -use std::sync::Mutex; - -use codex_utils_plugins::PluginSkillRoot; -use futures::StreamExt; -use tokio::sync::Semaphore; - -use crate::SkillLoadOutcome; -use crate::loader::MAX_CONCURRENT_ROOT_SCANS; -use crate::loader::SkillRoot; -use crate::loader::SkillRootSnapshot; -use crate::loader::load_skill_root; -use crate::model::SkillFileSystemsByPath; - -/// Parsed plugin skill-root snapshots produced by one plugin load. -/// -/// Clones share the same snapshots. The plugins manager stores them with the corresponding loaded -/// plugins and passes a clone to skill loading as an optional preload. -#[derive(Clone)] -pub struct PluginSkillSnapshots { - snapshots_by_root: Arc>>, -} - -impl PluginSkillSnapshots { - /// Creates an empty snapshot collection for a plugin load to populate. - pub fn for_plugin_load() -> Self { - Self { - snapshots_by_root: Arc::new(Mutex::new(HashMap::new())), - } - } -} - -impl PartialEq for PluginSkillSnapshots { - fn eq(&self, other: &Self) -> bool { - Arc::ptr_eq(&self.snapshots_by_root, &other.snapshots_by_root) - } -} - -impl Eq for PluginSkillSnapshots {} - -impl Hash for PluginSkillSnapshots { - fn hash(&self, state: &mut H) { - Arc::as_ptr(&self.snapshots_by_root).hash(state); - } -} - -impl fmt::Debug for PluginSkillSnapshots { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("PluginSkillSnapshots") - .finish_non_exhaustive() - } -} - -pub(crate) async fn load_and_merge_skill_roots( - roots: I, - plugin_skill_snapshots: Option<&PluginSkillSnapshots>, - root_scan_slots: &Semaphore, -) -> SkillLoadOutcome -where - I: IntoIterator, -{ - let mut indexed_root_snapshots = futures::stream::iter(roots.into_iter().enumerate()) - .map(|(root_index, root)| async move { - // Bound root scans across all concurrent loads sharing this pool. - let _root_scan_slot = root_scan_slots - .acquire() - .await - .unwrap_or_else(|_| unreachable!()); - let snapshot = load_skill_root_snapshot(root, plugin_skill_snapshots).await; - (root_index, snapshot) - }) - // Keep each load's scan queue bounded while avoiding head-of-line blocking. - .buffer_unordered(MAX_CONCURRENT_ROOT_SCANS) - .collect::>() - .await; - // Keep every scan slot productive, then restore root precedence for deterministic merging. - indexed_root_snapshots.sort_unstable_by_key(|(root_index, _)| *root_index); - let root_snapshots = indexed_root_snapshots - .into_iter() - .map(|(_, snapshot)| snapshot) - .collect(); - - merge_skill_root_snapshots(root_snapshots) -} - -/// Loads one root while preserving the existing plugin-loader snapshot reuse. -/// -/// This is a temporary migration boundary for callers that load non-plugin roots elsewhere but -/// still need plugin roots to use the legacy preload cache. -pub async fn load_skill_root_snapshot( - root: SkillRoot, - plugin_skill_snapshots: Option<&PluginSkillSnapshots>, -) -> SkillRootSnapshot { - let cache_key = match ( - root.plugin_identity.clone(), - root.plugin_namespace.clone(), - root.plugin_root.clone(), - ) { - (Some(plugin_identity), Some(plugin_namespace), Some(plugin_root)) => { - Some(PluginSkillRoot { - path: root.path.clone(), - plugin_identity, - plugin_namespace, - plugin_root, - discovery_mode: root.discovery_mode, - }) - } - _ => None, - }; - let cached_snapshot = cache_key.as_ref().and_then(|cache_key| { - let plugin_skill_snapshots = plugin_skill_snapshots?; - plugin_skill_snapshots - .snapshots_by_root - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .get(cache_key) - .cloned() - }); - - match cached_snapshot { - Some(snapshot) => snapshot, - None => { - let snapshot = load_skill_root(root).await; - if let Some(plugin_skill_snapshots) = plugin_skill_snapshots - && let Some(cache_key) = cache_key - { - plugin_skill_snapshots - .snapshots_by_root - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .insert(cache_key, snapshot.clone()); - } - snapshot - } - } -} - -pub(crate) fn merge_skill_root_snapshots(snapshots: Vec) -> SkillLoadOutcome { - fn scope_rank(scope: codex_protocol::protocol::SkillScope) -> u8 { - use codex_protocol::protocol::SkillScope; - - // Higher-priority scopes first (matches root scan order for dedupe). - match scope { - SkillScope::Repo => 0, - SkillScope::User => 1, - SkillScope::System => 2, - SkillScope::Admin => 3, - } - } - - let mut outcome = SkillLoadOutcome::default(); - let mut skill_roots = Vec::new(); - let mut skill_root_by_path = HashMap::new(); - let mut skill_discovery_path_by_path = HashMap::new(); - let mut file_systems_by_skill_path = HashMap::new(); - - for snapshot in snapshots { - let SkillRootSnapshot { - root, - is_agent_plugin, - skills, - skill_discovery_path_by_path: discovery_paths, - errors, - file_system, - } = snapshot; - if !skills.is_empty() && !skill_roots.contains(&root) { - skill_roots.push(root.clone()); - } - for skill in &skills { - let path = skill.path_to_skills_md.clone(); - if !skill_root_by_path.contains_key(&path) { - skill_root_by_path.insert(path.clone(), root.clone()); - if let Some(discovery_path) = discovery_paths.get(&path) { - skill_discovery_path_by_path.insert(path.clone(), discovery_path.clone()); - } - file_systems_by_skill_path.insert(path.clone(), Arc::clone(&file_system)); - if is_agent_plugin { - outcome.agent_plugin_skill_paths.insert(path); - } - } - } - outcome.skills.extend(skills); - outcome.errors.extend(errors); - } - - let mut seen = HashSet::new(); - outcome - .skills - .retain(|skill| seen.insert(skill.path_to_skills_md.clone())); - let retained_skill_paths = outcome - .skills - .iter() - .map(|skill| skill.path_to_skills_md.clone()) - .collect::>(); - skill_root_by_path.retain(|path, _| retained_skill_paths.contains(path)); - skill_discovery_path_by_path.retain(|path, _| retained_skill_paths.contains(path)); - let used_roots = skill_root_by_path.values().cloned().collect::>(); - skill_roots.retain(|root| used_roots.contains(root)); - file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(path)); - outcome - .agent_plugin_skill_paths - .retain(|path| retained_skill_paths.contains(path)); - outcome.skill_roots = skill_roots; - outcome.skill_root_by_path = Arc::new(skill_root_by_path); - outcome.skill_discovery_path_by_path = Arc::new(skill_discovery_path_by_path); - outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path); - - outcome.skills.sort_by(|a, b| { - scope_rank(a.scope) - .cmp(&scope_rank(b.scope)) - .then_with(|| a.name.cmp(&b.name)) - .then_with(|| a.path_to_skills_md.cmp(&b.path_to_skills_md)) - }); - - outcome -} diff --git a/codex-rs/core-skills/tests/environment_loader.rs b/codex-rs/core-skills/tests/environment_loader.rs deleted file mode 100644 index 53cbac4abd..0000000000 --- a/codex-rs/core-skills/tests/environment_loader.rs +++ /dev/null @@ -1,294 +0,0 @@ -#![cfg(unix)] - -use std::fs; -use std::sync::Mutex; -use std::sync::atomic::AtomicUsize; -use std::sync::atomic::Ordering; - -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_utils_path_uri::PathUri; -use pretty_assertions::assert_eq; -use tempfile::tempdir; - -struct RecordingFileSystem<'a> { - inner: &'a dyn ExecutorFileSystem, - read_files: Mutex>, - metadata_files: Mutex>, - walks: AtomicUsize, -} - -#[derive(Debug, PartialEq, Eq)] -struct FileSystemCalls { - walks: usize, - read_files: Vec, - metadata_files: Vec, -} - -impl<'a> RecordingFileSystem<'a> { - 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), - } - } - - 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()); - 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()); - 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) - } -} - -#[cfg(unix)] -#[tokio::test] -async fn host_loading_reuses_walk_inventory_for_symlinked_skill_pack() { - use std::os::unix::fs::symlink; - use std::sync::Arc; - - use codex_core_skills::SkillMetadata; - use codex_core_skills::SkillPolicy; - use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS; - use codex_core_skills::loader::SkillRoot; - use codex_core_skills::loader::load_skills_from_roots; - use codex_protocol::protocol::SkillScope; - use codex_utils_absolute_path::test_support::PathBufExt; - - let root = tempdir().expect("tempdir"); - let shared_plugin_root = tempdir().expect("tempdir"); - let manifest_path = shared_plugin_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":"linked"}"#).expect("manifest"); - - let skills_root = shared_plugin_root.path().join("skills"); - for name in ["first", "second"] { - let skill_path = skills_root.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"); - } - let metadata_path = skills_root.join("first/agents/openai.yaml"); - fs::create_dir_all(metadata_path.parent().expect("metadata parent")).expect("metadata dir"); - fs::write( - &metadata_path, - "policy:\n allow_implicit_invocation: false\n", - ) - .expect("metadata"); - - let host_root = root.path().join("skills"); - fs::create_dir_all(&host_root).expect("host skills dir"); - 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())); - let file_system: Arc = recording.clone(); - let future = load_skills_from_roots( - [SkillRoot { - path: host_root.abs(), - scope: SkillScope::User, - file_system, - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: codex_utils_plugins::SkillDiscoveryMode::Recursive, - }], - /*plugin_skill_snapshots*/ None, - Arc::new(tokio::sync::Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ); - fn assert_send(_: &T) {} - assert_send(&future); - let outcome = future.await; - - assert_eq!(outcome.errors, Vec::new()); - let first_skill_path = dunce::canonicalize(skills_root.join("first/SKILL.md")) - .unwrap() - .abs(); - let second_skill_path = dunce::canonicalize(skills_root.join("second/SKILL.md")) - .unwrap() - .abs(); - assert_eq!( - outcome.skills, - vec![ - SkillMetadata { - name: "linked:first".to_string(), - description: "first skill.".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: Some(SkillPolicy { - allow_implicit_invocation: Some(false), - products: Vec::new(), - }), - path_to_skills_md: first_skill_path, - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }, - SkillMetadata { - name: "linked:second".to_string(), - description: "second skill.".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: second_skill_path, - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - }, - ] - ); - - let calls = recording.calls(); - assert_eq!(calls.walks, 1); - let linked_root = PathUri::from_host_native_path(linked_root).unwrap(); - assert!( - calls - .read_files - .iter() - .all(|path| !path.starts_with(&linked_root)) - ); - assert!( - calls - .metadata_files - .iter() - .all(|path| path.basename().as_deref() != Some("openai.yaml")) - ); - let manifest_uri = - PathUri::from_host_native_path(dunce::canonicalize(manifest_path).unwrap()).unwrap(); - assert_eq!( - calls - .metadata_files - .iter() - .filter(|path| **path == manifest_uri) - .count(), - 1 - ); -} diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index 4a71209e5b..8fef509fd8 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -18,11 +18,9 @@ pub use codex_core_skills::SkillLoadOutcome; pub use codex_core_skills::build_skill_name_counts; pub use codex_core_skills::config_rules; pub use codex_core_skills::detect_implicit_skill_invocation_for_command; -pub use codex_core_skills::filter_skill_load_outcome_for_product; pub use codex_core_skills::injection; pub use codex_core_skills::injection::SkillInjections; pub use codex_core_skills::injection::build_skill_injections; -pub use codex_core_skills::loader; pub use codex_core_skills::model; pub use codex_core_skills::remote; pub use codex_skills::SkillMetadata; diff --git a/codex-rs/ext/skills/src/host_roots.rs b/codex-rs/ext/skills/src/host_roots.rs index 1bb0b21658..ae9367cc4e 100644 --- a/codex-rs/ext/skills/src/host_roots.rs +++ b/codex-rs/ext/skills/src/host_roots.rs @@ -7,7 +7,6 @@ use codex_config::ConfigLayerStack; use codex_config::default_project_root_markers; use codex_config::merge_toml_values; use codex_config::project_root_markers_from_config; -use codex_core_skills::loader::SkillRoot; use codex_exec_server::ExecutorFileSystem; use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::SkillScope; @@ -15,7 +14,6 @@ use codex_skills::system_cache_root_dir; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use codex_utils_plugins::PluginSkillRoot; -use codex_utils_plugins::SkillDiscoveryMode; use dirs::home_dir; use futures::StreamExt; use toml::Value as TomlValue; @@ -32,7 +30,7 @@ pub(crate) async fn resolve_skill_roots( cwd: &AbsolutePathBuf, plugin_skill_roots: Vec, extra_skill_roots: Vec, -) -> Vec { +) -> Vec { let home_dir = home_dir().and_then(|path| AbsolutePathBuf::from_absolute_path_checked(path).ok()); resolve_skill_roots_with_home_dir( @@ -53,33 +51,20 @@ async fn resolve_skill_roots_with_home_dir( home_dir: Option<&AbsolutePathBuf>, plugin_skill_roots: Vec, extra_skill_roots: Vec, -) -> Vec { +) -> Vec { let mut roots = - roots_from_layer_stack(config_layer_stack, home_dir, repository_file_system.clone()) + roots_from_layer_stack(config_layer_stack, home_dir, repository_file_system.clone()); + roots.extend( + plugin_skill_roots .into_iter() - .map(host_root_to_skill_root) - .collect::>(); - roots.extend(plugin_skill_roots.into_iter().map(|root| SkillRoot { - path: root.path, - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: Some(root.plugin_identity), - plugin_namespace: Some(root.plugin_namespace), - plugin_root: Some(root.plugin_root), - discovery_mode: root.discovery_mode, - })); + .map(|root| HostSkillRoot::plugin(root, Arc::clone(&LOCAL_FS))), + ); roots.extend( extra_skill_roots .into_iter() - .map(|path| local_root(path, SkillScope::User)) - .map(host_root_to_skill_root), - ); - roots.extend( - repo_agents_skill_roots(repository_file_system, config_layer_stack, cwd) - .await - .into_iter() - .map(host_root_to_skill_root), + .map(|path| local_root(path, SkillScope::User)), ); + roots.extend(repo_agents_skill_roots(repository_file_system, config_layer_stack, cwd).await); dedupe_skill_roots_by_path(&mut roots); roots } @@ -147,18 +132,6 @@ fn local_root(path: AbsolutePathBuf, scope: SkillScope) -> HostSkillRoot { HostSkillRoot::host(path, scope, Arc::clone(&LOCAL_FS)) } -fn host_root_to_skill_root(root: HostSkillRoot) -> SkillRoot { - SkillRoot { - path: root.path, - scope: root.scope, - file_system: root.file_system, - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - } -} - async fn repo_agents_skill_roots( repository_file_system: Option>, config_layer_stack: &ConfigLayerStack, @@ -285,7 +258,7 @@ fn dirs_between_project_root_and_cwd( directories } -fn dedupe_skill_roots_by_path(roots: &mut Vec) { +fn dedupe_skill_roots_by_path(roots: &mut Vec) { let mut seen = HashSet::new(); roots.retain(|root| seen.insert(root.path.clone())); } diff --git a/codex-rs/ext/skills/src/host_roots_tests.rs b/codex-rs/ext/skills/src/host_roots_tests.rs index b31072ae69..76b91e0ef7 100644 --- a/codex-rs/ext/skills/src/host_roots_tests.rs +++ b/codex-rs/ext/skills/src/host_roots_tests.rs @@ -8,8 +8,6 @@ use codex_config::ConfigLayerSource; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirementsToml; use codex_core_skills::SkillMetadata; -use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS; -use codex_core_skills::loader::load_skills_from_roots; use codex_exec_server::CopyOptions; use codex_exec_server::CreateDirectoryOptions; use codex_exec_server::ExecutorFileSystem; @@ -36,6 +34,8 @@ use tokio::sync::Semaphore; use super::repo_agents_skill_roots; use super::resolve_skill_roots_with_home_dir; use super::roots_from_layer_stack; +use crate::loader::MAX_CONCURRENT_ROOT_SCANS; +use crate::loader::load_and_merge_host_skill_roots; struct BlockingMetadataFileSystem { inner: Arc, @@ -317,10 +317,10 @@ async fn plugin_roots_preserve_plugin_resolution_metadata() { ( root.path.clone(), root.scope, - root.plugin_identity.clone(), - root.plugin_namespace.clone(), - root.plugin_root.clone(), - root.discovery_mode, + root.plugin_identity().cloned(), + root.plugin_namespace().map(str::to_string), + root.plugin_root().cloned(), + root.discovery_mode(), ), ( skills_root, @@ -357,10 +357,10 @@ async fn unique_extra_root_loads_as_recursive_user_root() { ( root.path.clone(), root.scope, - root.plugin_identity.clone(), - root.plugin_namespace.clone(), - root.plugin_root.clone(), - root.discovery_mode, + root.plugin_identity().cloned(), + root.plugin_namespace().map(str::to_string), + root.plugin_root().cloned(), + root.discovery_mode(), ), ( extra_root, @@ -373,10 +373,11 @@ async fn unique_extra_root_loads_as_recursive_user_root() { ); assert!(Arc::ptr_eq(&root.file_system, &LOCAL_FS)); - let outcome = load_skills_from_roots( + let outcome = load_and_merge_host_skill_roots( roots, + &Semaphore::new(MAX_CONCURRENT_ROOT_SCANS), + /*restriction_product*/ None, /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), ) .await; @@ -455,10 +456,11 @@ async fn resolved_project_layer_loads_skill_without_git_marker() { Vec::new(), ) .await; - let outcome = load_skills_from_roots( + let outcome = load_and_merge_host_skill_roots( roots, + &Semaphore::new(MAX_CONCURRENT_ROOT_SCANS), + /*restriction_product*/ None, /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), ) .await; @@ -491,10 +493,11 @@ async fn resolved_project_layer_loads_skill_when_cwd_is_file() { Vec::new(), ) .await; - let outcome = load_skills_from_roots( + let outcome = load_and_merge_host_skill_roots( roots, + &Semaphore::new(MAX_CONCURRENT_ROOT_SCANS), + /*restriction_product*/ None, /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), ) .await; @@ -645,10 +648,11 @@ async fn resolved_config_and_repo_roots_preserve_order_and_dedupe_paths_not_name ) .await; assert_eq!(roots.len(), 8); - let outcome = load_skills_from_roots( + let outcome = load_and_merge_host_skill_roots( roots, + &Semaphore::new(MAX_CONCURRENT_ROOT_SCANS), + /*restriction_product*/ None, /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), ) .await; assert!(outcome.errors.is_empty()); diff --git a/codex-rs/ext/skills/src/host_service.rs b/codex-rs/ext/skills/src/host_service.rs index ab375a1ad3..0a33736bc1 100644 --- a/codex-rs/ext/skills/src/host_service.rs +++ b/codex-rs/ext/skills/src/host_service.rs @@ -24,8 +24,6 @@ use codex_core_skills::SkillLoadOutcome; use codex_core_skills::config_rules::SkillConfigRules; use codex_core_skills::config_rules::resolve_disabled_skill_paths; use codex_core_skills::config_rules::skill_config_rules_from_stack; -use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS; -use codex_core_skills::loader::SkillRoot; use codex_skills::LoadedSkills; use codex_skills::SkillLoadFuture; use codex_skills::SkillRootLoadRequest; @@ -36,6 +34,7 @@ use codex_skills::install_system_skills; use crate::HostSkillsSnapshot; use crate::host_roots::resolve_skill_roots; use crate::loader::HostSkillRoot; +use crate::loader::MAX_CONCURRENT_ROOT_SCANS; use crate::loader::load_and_merge_host_skill_roots; #[derive(Debug, Clone)] @@ -165,7 +164,7 @@ impl HostSkillsService { &self, input: &HostSkillsLoadInput, fs: Option>, - ) -> Vec { + ) -> Vec { if input.bundled_skills_enabled { self.ensure_system_skills_installed(); } @@ -247,7 +246,7 @@ impl HostSkillsService { async fn snapshot_for_skill_roots( &self, input: &HostSkillsLoadInput, - roots: Vec, + roots: Vec, skill_config_rules: &SkillConfigRules, cache_key: ConfigSkillsCacheKey, force_reload: bool, @@ -285,33 +284,9 @@ impl HostSkillsService { async fn build_skill_outcome( &self, input: &HostSkillsLoadInput, - roots: Vec, + roots: Vec, skill_config_rules: &SkillConfigRules, ) -> SkillLoadOutcome { - let roots = roots - .into_iter() - .map(|root| { - match ( - root.plugin_identity, - root.plugin_namespace, - root.plugin_root, - ) { - (Some(plugin_identity), Some(plugin_namespace), Some(plugin_root)) => { - HostSkillRoot::plugin( - PluginSkillRoot { - path: root.path, - plugin_identity, - plugin_namespace, - plugin_root, - discovery_mode: root.discovery_mode, - }, - root.file_system, - ) - } - _ => HostSkillRoot::host(root.path, root.scope, root.file_system), - } - }) - .collect(); let outcome = load_and_merge_host_skill_roots( roots, &self.root_scan_slots, @@ -466,7 +441,7 @@ pub fn bundled_skills_enabled_from_stack( } fn config_skills_cache_key( - roots: &[SkillRoot], + roots: &[HostSkillRoot], skill_config_rules: &SkillConfigRules, plugin_skill_snapshots: Option<&SkillRootSnapshots>, ) -> ConfigSkillsCacheKey { @@ -483,15 +458,15 @@ fn config_skills_cache_key( ConfigSkillRootCacheKey { path: root.path.clone(), scope_rank, - plugin_identity: root.plugin_identity.clone(), - plugin_namespace: root.plugin_namespace.clone(), + plugin_identity: root.plugin_identity().cloned(), + plugin_namespace: root.plugin_namespace().map(str::to_string), file_system: FileSystemIdentity(Arc::downgrade(&root.file_system)), } }) .collect(), skill_config_rules: skill_config_rules.clone(), plugin_skill_snapshots: plugin_skill_snapshots - .filter(|_| roots.iter().any(|root| root.plugin_identity.is_some())) + .filter(|_| roots.iter().any(|root| root.plugin_identity().is_some())) .cloned(), } } diff --git a/codex-rs/ext/skills/src/lib.rs b/codex-rs/ext/skills/src/lib.rs index 5416e30d02..eba9e6e8c2 100644 --- a/codex-rs/ext/skills/src/lib.rs +++ b/codex-rs/ext/skills/src/lib.rs @@ -28,6 +28,7 @@ pub use host_service::HostSkillsLoadInput; pub use host_service::HostSkillsService; pub use host_service::bundled_skills_enabled_from_stack; pub use host_snapshot::HostSkillsSnapshot; +pub use loader::HostSkillRoot; pub use provider::ExecutorSkillProvider; pub use provider::HostSkillProvider; pub use provider::OrchestratorSkillProvider; diff --git a/codex-rs/ext/skills/src/loader/discovery.rs b/codex-rs/ext/skills/src/loader/discovery.rs index 50e06533a6..8d55776444 100644 --- a/codex-rs/ext/skills/src/loader/discovery.rs +++ b/codex-rs/ext/skills/src/loader/discovery.rs @@ -218,3 +218,7 @@ fn discover_skill_metadata( SkillMetadataDiscovery::Probe(metadata_path) } } + +#[cfg(test)] +#[path = "discovery_tests.rs"] +mod tests; diff --git a/codex-rs/ext/skills/src/loader/discovery_tests.rs b/codex-rs/ext/skills/src/loader/discovery_tests.rs new file mode 100644 index 0000000000..294636e0d2 --- /dev/null +++ b/codex-rs/ext/skills/src/loader/discovery_tests.rs @@ -0,0 +1,125 @@ +use std::fs; +use std::path::Path; + +use codex_exec_server::LOCAL_FS; +use codex_utils_path_uri::PathUri; +use codex_utils_plugins::SkillDiscoveryMode; +use pretty_assertions::assert_eq; +use tempfile::TempDir; + +use super::DirectorySymlinkPolicy; +use super::HiddenDirectoryPolicy; +use super::SkillDiscovery; +use super::SkillDiscoveryOptions; +use super::discover_skills; + +fn write_skill(root: &Path, directory: &str) { + let path = root.join(directory).join("SKILL.md"); + fs::create_dir_all(path.parent().expect("skill parent")).expect("create skill directory"); + fs::write(path, "---\nname: demo\ndescription: Demo skill\n---\n") + .expect("write skill frontmatter"); +} + +fn canonical_uri(path: &Path) -> PathUri { + PathUri::from_host_native_path(fs::canonicalize(path).expect("canonical path")) + .expect("absolute path URI") +} + +async fn discover(root: &Path) -> SkillDiscovery { + discover_skills( + LOCAL_FS.as_ref(), + &canonical_uri(root), + SkillDiscoveryOptions { + directory_symlinks: DirectorySymlinkPolicy::Follow, + hidden_directories: HiddenDirectoryPolicy::Skip, + mode: SkillDiscoveryMode::Recursive, + }, + ) + .await +} + +#[cfg(unix)] +#[tokio::test] +async fn discovers_hidden_directory_through_visible_symlink() { + let root = TempDir::new().expect("root temp dir"); + write_skill(root.path(), ".hidden/search"); + std::os::unix::fs::symlink(root.path().join(".hidden"), root.path().join("visible")) + .expect("create visible skill directory symlink"); + + let discovery = discover(root.path()).await; + + assert_eq!(discovery.warnings, Vec::::new()); + assert_eq!( + discovery + .skills + .into_iter() + .map(|skill| skill.path) + .collect::>(), + vec![ + canonical_uri(root.path()) + .join("visible/search/SKILL.md") + .expect("visible skill URI"), + ] + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn ignores_symlinked_skill_files_and_directory_cycles() { + let root = TempDir::new().expect("root temp dir"); + let external = TempDir::new().expect("external temp dir"); + write_skill(external.path(), "external"); + write_skill(root.path(), "cycle/demo"); + fs::create_dir_all(root.path().join("linked-file")).expect("create linked skill directory"); + std::os::unix::fs::symlink( + external.path().join("external/SKILL.md"), + root.path().join("linked-file/SKILL.md"), + ) + .expect("create skill file symlink"); + std::os::unix::fs::symlink(root.path().join("cycle"), root.path().join("cycle/loop")) + .expect("create directory symlink cycle"); + + let discovery = tokio::time::timeout( + std::time::Duration::from_secs(/*secs*/ 5), + discover(root.path()), + ) + .await + .expect("directory symlink cycles must not prevent discovery from completing"); + + assert_eq!( + discovery + .skills + .into_iter() + .map(|skill| skill.path) + .collect::>(), + vec![ + canonical_uri(root.path()) + .join("cycle/demo/SKILL.md") + .expect("valid skill URI"), + ] + ); +} + +#[tokio::test] +async fn respects_maximum_skill_scan_depth() { + let root = TempDir::new().expect("root temp dir"); + let within_depth = "d0/d1/d2/d3/d4/d5"; + let beyond_depth = "d0/d1/d2/d3/d4/d5/d6"; + write_skill(root.path(), within_depth); + write_skill(root.path(), beyond_depth); + + let discovery = discover(root.path()).await; + + assert_eq!( + discovery + .skills + .into_iter() + .map(|skill| skill.path) + .collect::>(), + vec![ + canonical_uri(root.path()) + .join(&format!("{within_depth}/SKILL.md")) + .expect("reachable skill URI"), + ] + ); +} diff --git a/codex-rs/ext/skills/src/loader/environment_io_tests.rs b/codex-rs/ext/skills/src/loader/environment_io_tests.rs index 0a02413dc0..fc95ec6093 100644 --- a/codex-rs/ext/skills/src/loader/environment_io_tests.rs +++ b/codex-rs/ext/skills/src/loader/environment_io_tests.rs @@ -1,208 +1,18 @@ 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 crate::loader::io_test_support::FileSystemCalls; +use crate::loader::io_test_support::ManifestMetadataBehavior; +use crate::loader::io_test_support::RecordingFileSystem; 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"); diff --git a/codex-rs/ext/skills/src/loader/environment_tests.rs b/codex-rs/ext/skills/src/loader/environment_tests.rs index 884b53aee5..59305a3e93 100644 --- a/codex-rs/ext/skills/src/loader/environment_tests.rs +++ b/codex-rs/ext/skills/src/loader/environment_tests.rs @@ -1,18 +1,92 @@ use std::fs; -use codex_core_skills::loader::load_environment_skills_from_root; use codex_exec_server::CapabilityRootDiscoverRequest; use codex_exec_server::CapabilityRootsDiscoverParams; use codex_exec_server::LOCAL_FS; use codex_exec_server::discover_capability_roots; +use codex_protocol::protocol::Product; +use codex_skills::EnvironmentSkillMetadata; +use codex_skills::SkillDependencies; +use codex_skills::SkillPolicy; +use codex_skills::SkillToolDependency; use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use tempfile::tempdir; use super::load_environment_skills_from_discovery; +use super::load_environment_skills_from_root; #[tokio::test] -async fn executor_bundle_parser_matches_the_existing_environment_loader() { +async fn direct_environment_loader_preserves_plugin_dependencies_and_product_policy() { + let root = tempdir().expect("tempdir"); + let skill_dir = root.path().join("skills/deploy"); + fs::create_dir_all(root.path().join(".codex-plugin")).expect("manifest dir"); + fs::create_dir_all(skill_dir.join("agents")).expect("metadata dir"); + fs::write( + root.path().join(".codex-plugin/plugin.json"), + r#"{"name":"demo-plugin"}"#, + ) + .expect("manifest"); + fs::write( + skill_dir.join("SKILL.md"), + "---\nname: deploy\ndescription: Deploy the service.\n---\n", + ) + .expect("skill"); + fs::write( + skill_dir.join("agents/openai.yaml"), + r#" +dependencies: + tools: + - type: mcp + value: deploy-server + description: Deploy MCP + transport: stdio + command: deploy-mcp +policy: + allow_implicit_invocation: false + products: [codex, atlas] +"#, + ) + .expect("metadata"); + + let root_uri = PathUri::from_host_native_path(root.path()).expect("root URI"); + let outcome = + load_environment_skills_from_root(LOCAL_FS.as_ref(), &root_uri, Some(Product::Codex)).await; + + assert_eq!( + outcome.skills, + vec![EnvironmentSkillMetadata { + path_to_skills_md: PathUri::from_host_native_path(skill_dir.join("SKILL.md")).unwrap(), + name: "demo-plugin:deploy".to_string(), + description: "Deploy the service.".to_string(), + short_description: None, + dependencies: Some(SkillDependencies { + tools: vec![SkillToolDependency { + r#type: "mcp".to_string(), + value: "deploy-server".to_string(), + description: Some("Deploy MCP".to_string()), + transport: Some("stdio".to_string()), + command: Some("deploy-mcp".to_string()), + url: None, + }], + }), + policy: Some(SkillPolicy { + allow_implicit_invocation: Some(false), + products: vec![Product::Codex, Product::Atlas], + }), + }] + ); + let atlas = + load_environment_skills_from_root(LOCAL_FS.as_ref(), &root_uri, Some(Product::Atlas)).await; + assert_eq!(atlas.skills, outcome.skills); + let filtered = + load_environment_skills_from_root(LOCAL_FS.as_ref(), &root_uri, Some(Product::Chatgpt)) + .await; + assert!(filtered.skills.is_empty()); +} + +#[tokio::test] +async fn executor_bundle_parser_matches_direct_environment_loader() { let root = tempdir().expect("tempdir"); let plugin_manifest = root.path().join(".codex-plugin/plugin.json"); let nested_manifest = root.path().join("nested/.claude-plugin/plugin.json"); diff --git a/codex-rs/ext/skills/src/loader/host.rs b/codex-rs/ext/skills/src/loader/host.rs index f0029638a5..4c654f1645 100644 --- a/codex-rs/ext/skills/src/loader/host.rs +++ b/codex-rs/ext/skills/src/loader/host.rs @@ -75,7 +75,8 @@ impl HostSkillRoot { } } - pub(crate) fn plugin_identity(&self) -> Option<&PluginIdentity> { + /// Returns the owning plugin identity when this root belongs to a plugin. + pub fn plugin_identity(&self) -> Option<&PluginIdentity> { self.plugin.as_ref().map(|plugin| &plugin.identity) } @@ -404,3 +405,7 @@ async fn canonicalize_for_skill_identity( #[cfg(test)] #[path = "host_tests.rs"] mod tests; + +#[cfg(all(test, unix))] +#[path = "host_io_tests.rs"] +mod io_tests; diff --git a/codex-rs/ext/skills/src/loader/host_io_tests.rs b/codex-rs/ext/skills/src/loader/host_io_tests.rs new file mode 100644 index 0000000000..a3b8f92a60 --- /dev/null +++ b/codex-rs/ext/skills/src/loader/host_io_tests.rs @@ -0,0 +1,138 @@ +use std::fs; +use std::os::unix::fs::symlink; +use std::sync::Arc; + +use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::LOCAL_FS; +use codex_protocol::protocol::SkillScope; +use codex_skills::SkillMetadata; +use codex_skills::SkillPolicy; +use codex_utils_absolute_path::test_support::PathBufExt; +use codex_utils_path_uri::PathUri; +use pretty_assertions::assert_eq; +use tempfile::tempdir; +use tokio::sync::Semaphore; + +use crate::loader::HostSkillRoot; +use crate::loader::MAX_CONCURRENT_ROOT_SCANS; +use crate::loader::io_test_support::ManifestMetadataBehavior; +use crate::loader::io_test_support::RecordingFileSystem; +use crate::loader::load_and_merge_host_skill_roots; + +#[tokio::test] +async fn host_loading_reuses_walk_inventory_for_symlinked_skill_pack() { + let root = tempdir().expect("tempdir"); + let shared_plugin_root = tempdir().expect("tempdir"); + let manifest_path = shared_plugin_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":"linked"}"#).expect("manifest"); + + let skills_root = shared_plugin_root.path().join("skills"); + for name in ["first", "second"] { + let skill_path = skills_root.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"); + } + let metadata_path = skills_root.join("first/agents/openai.yaml"); + fs::create_dir_all(metadata_path.parent().expect("metadata parent")).expect("metadata dir"); + fs::write( + &metadata_path, + "policy:\n allow_implicit_invocation: false\n", + ) + .expect("metadata"); + + let host_root = root.path().join("skills"); + fs::create_dir_all(&host_root).expect("host skills dir"); + 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 file_system: Arc = recording.clone(); + let root_scan_slots = Semaphore::new(MAX_CONCURRENT_ROOT_SCANS); + let future = load_and_merge_host_skill_roots( + vec![HostSkillRoot::host( + host_root.abs(), + SkillScope::User, + file_system, + )], + &root_scan_slots, + /*restriction_product*/ None, + /*plugin_skill_snapshots*/ None, + ); + fn assert_send(_: &T) {} + assert_send(&future); + let outcome = future.await; + + assert_eq!(outcome.errors, Vec::new()); + let first_skill_path = dunce::canonicalize(skills_root.join("first/SKILL.md")) + .unwrap() + .abs(); + let second_skill_path = dunce::canonicalize(skills_root.join("second/SKILL.md")) + .unwrap() + .abs(); + assert_eq!( + outcome.skills, + vec![ + SkillMetadata { + name: "linked:first".to_string(), + description: "first skill.".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: Some(SkillPolicy { + allow_implicit_invocation: Some(false), + products: Vec::new(), + }), + path_to_skills_md: first_skill_path, + scope: SkillScope::User, + plugin_id: None, + remote_plugin_id: None, + }, + SkillMetadata { + name: "linked:second".to_string(), + description: "second skill.".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + path_to_skills_md: second_skill_path, + scope: SkillScope::User, + plugin_id: None, + remote_plugin_id: None, + }, + ] + ); + + let calls = recording.calls(); + assert_eq!(calls.walks, 1); + let linked_root = PathUri::from_host_native_path(linked_root).unwrap(); + assert!( + calls + .read_files + .iter() + .all(|path| !path.starts_with(&linked_root)) + ); + assert!( + calls + .metadata_files + .iter() + .all(|path| path.basename().as_deref() != Some("openai.yaml")) + ); + let manifest_uri = + PathUri::from_host_native_path(dunce::canonicalize(manifest_path).unwrap()).unwrap(); + assert_eq!( + calls + .metadata_files + .iter() + .filter(|path| **path == manifest_uri) + .count(), + 1 + ); +} diff --git a/codex-rs/ext/skills/src/loader/host_merge.rs b/codex-rs/ext/skills/src/loader/host_merge.rs index 3bbfafcf9a..6fcb5a4b03 100644 --- a/codex-rs/ext/skills/src/loader/host_merge.rs +++ b/codex-rs/ext/skills/src/loader/host_merge.rs @@ -3,7 +3,6 @@ use std::collections::HashSet; use std::sync::Arc; use codex_core_skills::SkillLoadOutcome; -use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS; use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; @@ -18,6 +17,7 @@ use tokio::sync::OnceCell; use tokio::sync::Semaphore; use super::HostSkillRoot; +use super::MAX_CONCURRENT_ROOT_SCANS; use super::host::HostSkillRootSnapshot; use super::load_host_skill_root; diff --git a/codex-rs/ext/skills/src/loader/host_merge_tests.rs b/codex-rs/ext/skills/src/loader/host_merge_tests.rs index ccd593d93c..0444278b41 100644 --- a/codex-rs/ext/skills/src/loader/host_merge_tests.rs +++ b/codex-rs/ext/skills/src/loader/host_merge_tests.rs @@ -2,13 +2,19 @@ use std::collections::HashMap; use std::fs; use std::sync::Arc; use std::sync::Mutex; +use std::sync::atomic::Ordering; +use codex_exec_server::ExecutorFileSystem; use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; +use codex_protocol::protocol::SkillScope; use codex_skills::LoadedSkillRoot; +use codex_skills::SkillError; use codex_skills::SkillRootSnapshotCache; use codex_skills::SkillRootSnapshots; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_absolute_path::test_support::PathBufExt; +use codex_utils_path_uri::PathUri; use codex_utils_plugins::PluginIdentity; use codex_utils_plugins::PluginSkillRoot; use codex_utils_plugins::SkillDiscoveryMode; @@ -17,6 +23,10 @@ use pretty_assertions::assert_eq; use tempfile::TempDir; use tokio::sync::Semaphore; +use crate::loader::MAX_CONCURRENT_ROOT_SCANS; +use crate::loader::io_test_support::ManifestMetadataBehavior; +use crate::loader::io_test_support::RecordingFileSystem; + use super::HostSkillRoot; use super::load_and_merge_host_skill_roots; @@ -311,3 +321,188 @@ async fn recognizes_symlinked_migrated_command_roots() { ] ); } + +#[tokio::test] +async fn host_root_scans_wait_for_shared_capacity() { + let temp = TempDir::new().expect("temp dir"); + let root = AbsolutePathBuf::from_absolute_path(temp.path()).expect("absolute skill root"); + let root_scan_slots = Semaphore::new(/*permits*/ 1); + let held_slot = root_scan_slots + .try_acquire() + .expect("root scan slot should be available"); + let load = load_and_merge_host_skill_roots( + vec![HostSkillRoot::host( + root, + SkillScope::Repo, + Arc::clone(&LOCAL_FS), + )], + &root_scan_slots, + /*restriction_product*/ None, + /*plugin_skill_snapshots*/ None, + ); + tokio::pin!(load); + + assert!(futures::poll!(load.as_mut()).is_pending()); + drop(held_slot); + let outcome = load.await; + + assert_eq!(outcome.skills, Vec::new()); + assert_eq!(outcome.errors, Vec::new()); +} + +#[tokio::test] +async fn duplicate_host_root_paths_preserve_first_root_scope() { + let temp = TempDir::new().expect("temp dir"); + let root = AbsolutePathBuf::from_absolute_path(temp.path()).expect("absolute skill root"); + write_skill(&root, "demo", "demo", "Shared skill"); + + let outcome = load_and_merge_host_skill_roots( + vec![ + HostSkillRoot::host(root.clone(), SkillScope::Repo, Arc::clone(&LOCAL_FS)), + HostSkillRoot::host(root.clone(), SkillScope::User, Arc::clone(&LOCAL_FS)), + ], + &Semaphore::new(/*permits*/ 2), + /*restriction_product*/ None, + /*plugin_skill_snapshots*/ None, + ) + .await; + + assert_eq!(outcome.errors, Vec::new()); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].scope, SkillScope::Repo); + let canonical_root = AbsolutePathBuf::from_absolute_path( + fs::canonicalize(root.as_path()).expect("canonicalize skill root"), + ) + .expect("absolute canonical skill root"); + assert_eq!( + outcome.skill_root_for_path(&outcome.skills[0].path_to_skills_md), + Some(&canonical_root) + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn deduplicated_symlinked_skill_preserves_first_discovery_path() { + let source = TempDir::new().expect("source temp dir"); + let first = TempDir::new().expect("first root temp dir"); + let second = TempDir::new().expect("second root temp dir"); + let source_root = + AbsolutePathBuf::from_absolute_path(source.path()).expect("absolute source root"); + let first_root = + AbsolutePathBuf::from_absolute_path(first.path()).expect("absolute first root"); + let second_root = + AbsolutePathBuf::from_absolute_path(second.path()).expect("absolute second root"); + write_skill(&source_root, "demo", "demo", "Shared skill"); + std::os::unix::fs::symlink(source.path().join("demo"), first.path().join("first-link")) + .expect("create first skill directory symlink"); + std::os::unix::fs::symlink( + source.path().join("demo"), + second.path().join("second-link"), + ) + .expect("create second skill directory symlink"); + + let outcome = load_and_merge_host_skill_roots( + vec![ + HostSkillRoot::host(first_root.clone(), SkillScope::Repo, Arc::clone(&LOCAL_FS)), + HostSkillRoot::host(second_root, SkillScope::User, Arc::clone(&LOCAL_FS)), + ], + &Semaphore::new(/*permits*/ 2), + /*restriction_product*/ None, + /*plugin_skill_snapshots*/ None, + ) + .await; + + assert_eq!(outcome.errors, Vec::new()); + assert_eq!(outcome.skills.len(), 1); + let skill_path = &outcome.skills[0].path_to_skills_md; + let canonical_first_root = AbsolutePathBuf::from_absolute_path( + fs::canonicalize(first_root.as_path()).expect("canonicalize first skill root"), + ) + .expect("absolute first skill root"); + assert_eq!( + ( + outcome.skill_root_for_path(skill_path), + outcome.skill_discovery_path_for_path(skill_path), + ), + ( + Some(&canonical_first_root), + Some(&canonical_first_root.join("first-link/SKILL.md")), + ) + ); +} + +#[tokio::test] +async fn merges_host_root_results_in_input_order_when_scans_finish_out_of_order() { + const ROOT_COUNT: usize = MAX_CONCURRENT_ROOT_SCANS + 1; + + let temp = TempDir::new().expect("temp dir"); + let roots = (0..ROOT_COUNT) + .map(|index| temp.path().join(format!("root-{index}"))) + .collect::>(); + for root in &roots { + fs::create_dir_all(root).expect("create skill root"); + } + let first_skill = roots[0].join("broken/SKILL.md"); + let second_skill = roots[1].join("broken/SKILL.md"); + for path in [&first_skill, &second_skill] { + fs::create_dir_all(path.parent().expect("skill parent")) + .expect("create invalid skill directory"); + fs::write(path, "missing frontmatter").expect("write invalid skill"); + } + + let mut recording = + RecordingFileSystem::new(LOCAL_FS.as_ref(), ManifestMetadataBehavior::Immediate); + recording.blocked_walk_root = Some(PathUri::from_abs_path( + &dunce::canonicalize(&roots[0]) + .expect("canonical blocked skill root") + .abs(), + )); + let recording = Arc::new(recording); + let file_system: Arc = recording.clone(); + let host_roots = roots + .iter() + .map(|root| HostSkillRoot::host(root.clone().abs(), SkillScope::User, file_system.clone())) + .collect::>(); + let load = tokio::spawn(async move { + load_and_merge_host_skill_roots( + host_roots, + &Semaphore::new(MAX_CONCURRENT_ROOT_SCANS), + /*restriction_product*/ None, + /*plugin_skill_snapshots*/ None, + ) + .await + }); + + tokio::time::timeout(std::time::Duration::from_secs(/*secs*/ 5), async { + loop { + let walk_started = recording.walk_started.notified(); + if recording.walks.load(Ordering::Acquire) == ROOT_COUNT { + break; + } + walk_started.await; + } + }) + .await + .expect("all host root scans should start despite the blocked first root"); + recording.blocked_walk_gate.add_permits(/*n*/ 1); + let outcome = load.await.expect("host skill root loading should complete"); + + assert_eq!(outcome.skills, Vec::new()); + assert_eq!( + outcome.errors, + vec![ + SkillError { + path: dunce::canonicalize(first_skill) + .expect("canonical first invalid skill path") + .abs(), + message: "missing YAML frontmatter delimited by ---".to_string(), + }, + SkillError { + path: dunce::canonicalize(second_skill) + .expect("canonical second invalid skill path") + .abs(), + message: "missing YAML frontmatter delimited by ---".to_string(), + }, + ] + ); +} diff --git a/codex-rs/ext/skills/src/loader/host_tests.rs b/codex-rs/ext/skills/src/loader/host_tests.rs index c5d85750e4..ddaa00442e 100644 --- a/codex-rs/ext/skills/src/loader/host_tests.rs +++ b/codex-rs/ext/skills/src/loader/host_tests.rs @@ -119,7 +119,7 @@ async fn loads_host_frontmatter_dependencies_and_policy() { description: Demo tool policy: allow_implicit_invocation: false - products: [codex] + products: [codex, CHATGPT, atlas] "##, ); @@ -145,7 +145,7 @@ policy: }), policy: Some(SkillPolicy { allow_implicit_invocation: Some(false), - products: vec![Product::Codex], + products: vec![Product::Codex, Product::Chatgpt, Product::Atlas], }), path_to_skills_md: skill_path, scope: SkillScope::User, @@ -433,6 +433,41 @@ async fn plugin_root_accepts_maximum_length_qualified_skill_name() { ); } +#[tokio::test] +async fn plugin_root_rejects_overlong_qualified_skill_name() { + let root = TempDir::new().expect("temp dir"); + let skill_name = "s".repeat(MAX_NAME_LEN); + write_skill( + &root, + "skills/search", + &format!("name: {skill_name}\ndescription: Search skill"), + ); + let plugin_root = AbsolutePathBuf::from_absolute_path(root.path()).expect("plugin root"); + + let snapshot = load_host_skill_root(HostSkillRoot::plugin( + PluginSkillRoot { + path: plugin_root.join("skills"), + plugin_identity: PluginIdentity { + plugin_id: "demo@test".to_string(), + remote_plugin_id: None, + }, + plugin_namespace: "p".repeat(MAX_NAME_LEN + 1), + plugin_root, + discovery_mode: SkillDiscoveryMode::Recursive, + }, + Arc::clone(&LOCAL_FS), + )) + .await; + + assert_eq!(snapshot.skills, Vec::new()); + assert_eq!(snapshot.errors.len(), 1); + assert!( + snapshot.errors[0] + .message + .contains("invalid qualified name") + ); +} + #[tokio::test] async fn recursive_plugin_root_preserves_owner_namespace_and_shared_asset_policy() { let root = TempDir::new().expect("temp dir"); @@ -638,7 +673,7 @@ async fn recursive_plugin_root_preserves_symlinked_skill_discovery_path() { #[cfg(unix)] #[tokio::test] -async fn follows_directory_symlinks_for_user_but_not_system_scope() { +async fn follows_directory_symlinks_except_for_system_scope() { use std::os::unix::fs::symlink; let root = TempDir::new().expect("temp dir"); @@ -646,18 +681,25 @@ async fn follows_directory_symlinks_for_user_but_not_system_scope() { let target_skill = write_skill(&target, "demo", "name: demo\ndescription: Symlinked skill"); symlink(target.path().join("demo"), root.path().join("alias")).expect("create symlink"); - let user_snapshot = load_host_skill_root(root_for(&root, SkillScope::User)).await; - let system_snapshot = load_host_skill_root(root_for(&root, SkillScope::System)).await; + for scope in [SkillScope::User, SkillScope::Repo, SkillScope::Admin] { + let snapshot = load_host_skill_root(root_for(&root, scope)).await; - assert_eq!(user_snapshot.errors, Vec::new()); - assert_eq!(user_snapshot.skills.len(), 1); - assert_eq!(user_snapshot.skills[0].path_to_skills_md, target_skill); - assert_eq!( - user_snapshot - .skill_discovery_path_by_path - .get(&target_skill), - Some(&user_snapshot.root.join("alias/SKILL.md")) - ); + assert_eq!(snapshot.errors, Vec::new()); + assert_eq!(snapshot.skills.len(), 1); + assert_eq!( + ( + &snapshot.skills[0].path_to_skills_md, + snapshot.skills[0].scope + ), + (&target_skill, scope) + ); + assert_eq!( + snapshot.skill_discovery_path_by_path.get(&target_skill), + Some(&snapshot.root.join("alias/SKILL.md")) + ); + } + + let system_snapshot = load_host_skill_root(root_for(&root, SkillScope::System)).await; assert_eq!(system_snapshot.errors, Vec::new()); assert_eq!(system_snapshot.skills, Vec::new()); } diff --git a/codex-rs/ext/skills/src/loader/io_test_support.rs b/codex-rs/ext/skills/src/loader/io_test_support.rs new file mode 100644 index 0000000000..4972cffdcf --- /dev/null +++ b/codex-rs/ext/skills/src/loader/io_test_support.rs @@ -0,0 +1,214 @@ +use std::sync::Mutex; +use std::sync::atomic::AtomicBool; +use std::sync::atomic::AtomicUsize; +use std::sync::atomic::Ordering; + +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::ReadDirectoryEntry; +use codex_exec_server::RemoveOptions; +use codex_exec_server::WalkOptions; +use codex_exec_server::WalkOutcome; +use codex_utils_path_uri::PathUri; +use tokio::sync::Notify; +use tokio::sync::Semaphore; + +#[derive(Clone, Copy)] +pub(super) enum ManifestMetadataBehavior { + Immediate, + WaitForSkillRead, +} + +pub(super) struct RecordingFileSystem<'a> { + inner: &'a dyn ExecutorFileSystem, + read_files: Mutex>, + metadata_files: Mutex>, + pub(super) walks: AtomicUsize, + manifest_metadata_behavior: ManifestMetadataBehavior, + skill_read_started: AtomicBool, + skill_read_started_notify: Notify, + pub(super) blocked_walk_root: Option, + pub(super) blocked_walk_gate: Semaphore, + pub(super) walk_started: Notify, +} + +#[derive(Debug, PartialEq, Eq)] +pub(super) struct FileSystemCalls { + pub(super) walks: usize, + pub(super) read_files: Vec, + pub(super) metadata_files: Vec, +} + +impl<'a> RecordingFileSystem<'a> { + pub(super) 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(/*v*/ 0), + manifest_metadata_behavior, + skill_read_started: AtomicBool::new(/*v*/ false), + skill_read_started_notify: Notify::new(), + blocked_walk_root: None, + blocked_walk_gate: Semaphore::new(/*permits*/ 0), + walk_started: Notify::new(), + } + } + + pub(super) 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(/*val*/ 1, Ordering::AcqRel); + self.walk_started.notify_waiters(); + if self.blocked_walk_root.as_ref() != Some(path) { + return self.inner.walk(path, options, sandbox); + } + Box::pin(async move { + self.blocked_walk_gate + .acquire() + .await + .expect("blocked walk gate should remain open") + .forget(); + self.inner.walk(path, options, sandbox).await + }) + } + + 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) + } +} diff --git a/codex-rs/ext/skills/src/loader/mod.rs b/codex-rs/ext/skills/src/loader/mod.rs index eced4944bf..a94cc6a0c3 100644 --- a/codex-rs/ext/skills/src/loader/mod.rs +++ b/codex-rs/ext/skills/src/loader/mod.rs @@ -2,15 +2,18 @@ mod discovery; mod environment; mod host; mod host_merge; +#[cfg(test)] +mod io_test_support; mod metadata; mod namespace; pub(crate) use environment::load_environment_skills_from_discovery; pub(crate) use environment::load_environment_skills_from_root; -pub(crate) use host::HostSkillRoot; +pub use host::HostSkillRoot; pub(crate) use host::load_host_skill_root; pub(crate) use host_merge::load_and_merge_host_skill_roots; +pub(crate) const MAX_CONCURRENT_ROOT_SCANS: usize = 8; 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"; diff --git a/codex-rs/ext/skills/src/loader/namespace.rs b/codex-rs/ext/skills/src/loader/namespace.rs index 687238138b..5dac7538ae 100644 --- a/codex-rs/ext/skills/src/loader/namespace.rs +++ b/codex-rs/ext/skills/src/loader/namespace.rs @@ -180,3 +180,7 @@ impl ResolvedSkillNamespace { } } } + +#[cfg(test)] +#[path = "namespace_tests.rs"] +mod tests; diff --git a/codex-rs/ext/skills/src/loader/namespace_tests.rs b/codex-rs/ext/skills/src/loader/namespace_tests.rs new file mode 100644 index 0000000000..a85a3afb8e --- /dev/null +++ b/codex-rs/ext/skills/src/loader/namespace_tests.rs @@ -0,0 +1,172 @@ +use std::collections::HashSet; +use std::fs; +use std::path::Path; +#[cfg(unix)] +use std::sync::Arc; + +use codex_exec_server::LOCAL_FS; +#[cfg(unix)] +use codex_protocol::protocol::SkillScope; +#[cfg(unix)] +use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; +use pretty_assertions::assert_eq; +use tempfile::TempDir; + +#[cfg(unix)] +use crate::loader::HostSkillRoot; +#[cfg(unix)] +use crate::loader::load_host_skill_root; + +use super::SkillNamespaceResolver; + +fn canonical_uri(path: &Path) -> PathUri { + PathUri::from_host_native_path(fs::canonicalize(path).expect("canonical path")) + .expect("absolute path URI") +} + +fn write_skill(root: &Path, directory: &str) -> PathUri { + let path = root.join(directory).join("SKILL.md"); + fs::create_dir_all(path.parent().expect("skill parent")).expect("create skill directory"); + fs::write(&path, "---\nname: skill\ndescription: Demo skill\n---\n") + .expect("write skill frontmatter"); + canonical_uri(&path) +} + +fn write_manifest(root: &Path, contents: &str) -> PathUri { + let path = root.join(".codex-plugin/plugin.json"); + fs::create_dir_all(path.parent().expect("manifest parent")) + .expect("create plugin manifest directory"); + fs::write(path, contents).expect("write plugin manifest"); + canonical_uri(root) +} + +async fn namespaces_for( + root: &Path, + skill_paths: &[PathUri], + plugin_roots: HashSet, + mut namespace_roots: HashSet, +) -> Vec { + let root = canonical_uri(root); + namespace_roots.insert(root.clone()); + let resolver = SkillNamespaceResolver::discover( + LOCAL_FS.as_ref(), + &root, + skill_paths, + plugin_roots, + namespace_roots, + ) + .await; + + skill_paths + .iter() + .map(|path| resolver.for_skill(&root, path).qualify("skill")) + .collect() +} + +#[tokio::test] +async fn nested_plugin_namespaces_do_not_apply_to_plain_siblings() { + let root = TempDir::new().expect("root temp dir"); + let nested_plugin = root.path().join("nested-plugin"); + let plugin_root = write_manifest(&nested_plugin, r#"{"name":"nested"}"#); + let plain_skill = write_skill(root.path(), "plain"); + let nested_skill = write_skill(&nested_plugin, "skills/search"); + + let namespaces = namespaces_for( + root.path(), + &[plain_skill, nested_skill], + HashSet::from([plugin_root]), + HashSet::new(), + ) + .await; + + assert_eq!(namespaces, vec!["skill", "nested:skill"]); +} + +#[tokio::test] +async fn nearest_valid_manifest_overrides_or_falls_back_to_outer_namespace() { + let root = TempDir::new().expect("root temp dir"); + let plugin_root = root.path().join("outer"); + let skills_root = plugin_root.join("skills"); + let nested_plugin = skills_root.join("nested"); + let invalid_plugin = skills_root.join("invalid"); + write_manifest(&plugin_root, r#"{"name":"outer"}"#); + let nested_root = write_manifest(&nested_plugin, r#"{"name":"nested"}"#); + let invalid_root = write_manifest(&invalid_plugin, "not json"); + let direct_skill = write_skill(&skills_root, "direct"); + let nested_skill = write_skill(&nested_plugin, "skills/search"); + let fallback_skill = write_skill(&invalid_plugin, "skills/fallback"); + + let namespaces = namespaces_for( + &skills_root, + &[direct_skill, nested_skill, fallback_skill], + HashSet::from([nested_root, invalid_root]), + HashSet::new(), + ) + .await; + + assert_eq!( + namespaces, + vec!["outer:skill", "nested:skill", "outer:skill"] + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn symlinked_external_directories_do_not_inherit_plugin_namespace() { + let root = TempDir::new().expect("root temp dir"); + let external = TempDir::new().expect("external temp dir"); + let plugin_root = root.path().join("outer"); + let skills_root = plugin_root.join("skills"); + write_manifest(&plugin_root, r#"{"name":"outer"}"#); + write_skill(external.path(), "plain"); + fs::create_dir_all(&skills_root).expect("create skill root"); + std::os::unix::fs::symlink(external.path(), skills_root.join("linked")) + .expect("create external directory symlink"); + + let snapshot = load_host_skill_root(HostSkillRoot::host( + AbsolutePathBuf::from_absolute_path(&skills_root).expect("absolute skills root"), + SkillScope::User, + Arc::clone(&LOCAL_FS), + )) + .await; + + assert_eq!(snapshot.errors, Vec::new()); + assert_eq!( + snapshot + .skills + .into_iter() + .map(|skill| skill.name) + .collect::>(), + vec!["skill"] + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn ancestor_symlink_roots_preserve_inherited_plugin_namespace() { + let root = TempDir::new().expect("root temp dir"); + let plugin_root = root.path().join("a/b/c/d/e/f/outer"); + let skills_root = plugin_root.join("skills"); + write_manifest(&plugin_root, r#"{"name":"outer"}"#); + write_skill(&skills_root, "direct"); + std::os::unix::fs::symlink(root.path(), skills_root.join("ancestor")) + .expect("create ancestor directory symlink"); + + let snapshot = load_host_skill_root(HostSkillRoot::host( + AbsolutePathBuf::from_absolute_path(&skills_root).expect("absolute skills root"), + SkillScope::User, + Arc::clone(&LOCAL_FS), + )) + .await; + + assert_eq!(snapshot.errors, Vec::new()); + assert_eq!( + snapshot + .skills + .into_iter() + .map(|skill| skill.name) + .collect::>(), + vec!["outer:skill"] + ); +} diff --git a/codex-rs/ext/skills/src/provider/host_tests.rs b/codex-rs/ext/skills/src/provider/host_tests.rs index 0d9b0b0cc2..6269585f60 100644 --- a/codex-rs/ext/skills/src/provider/host_tests.rs +++ b/codex-rs/ext/skills/src/provider/host_tests.rs @@ -2,8 +2,6 @@ use std::sync::Arc; use std::time::SystemTime; use std::time::UNIX_EPOCH; -use codex_core_skills::loader::SkillRoot; -use codex_core_skills::loader::load_skills_from_roots; use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -11,6 +9,8 @@ use pretty_assertions::assert_eq; use tokio::sync::Semaphore; use super::catalog_from_outcome; +use crate::loader::HostSkillRoot; +use crate::loader::load_and_merge_host_skill_roots; #[tokio::test] async fn host_catalog_entries_carry_their_render_metadata() -> Result<(), Box> @@ -31,18 +31,15 @@ async fn host_catalog_entries_carry_their_render_metadata() -> Result<(), Box) -> SkillCatalogEntry { entry_with_path( @@ -301,29 +301,18 @@ async fn host_alias_roots_follow_core_discovery_order() -> Result<(), Box>() }; assert_eq!(comparable_entries(&bundled), comparable_entries(&direct)); - assert_eq!(legacy.warnings, direct.warnings); assert_eq!(direct.warnings.len(), 1); assert!(direct.warnings[0].contains("missing field `description`")); - let legacy_metadata = legacy - .skills - .iter() - .map(|skill| { - ( - skill.name.clone(), - skill.description.clone(), - skill.short_description.clone(), - skill.dependencies.clone(), - skill.allows_implicit_invocation(), - ) - }) - .collect::>(); - let catalog_metadata = direct - .entries - .iter() - .map(|entry| { - ( - entry.name.clone(), - entry.description.clone(), - entry.short_description.clone(), - entry.dependencies.clone(), - entry.prompt_visible, - ) - }) - .collect::>(); - assert_eq!(catalog_metadata, legacy_metadata); let entry = direct .entries .iter() @@ -692,16 +669,6 @@ async fn direct_executor_discovery_preserves_hidden_nested_and_probed_metadata() }, }; 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, ); @@ -720,18 +687,7 @@ async fn direct_executor_discovery_preserves_hidden_nested_and_probed_metadata() .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::>(); + assert_eq!(direct.warnings, Vec::::new()); let catalog_metadata = direct .entries .iter() @@ -743,7 +699,6 @@ async fn direct_executor_discovery_preserves_hidden_nested_and_probed_metadata() ) }) .collect::>(); - assert_eq!(catalog_metadata, legacy_metadata); assert_eq!( catalog_metadata, vec![ diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index 6dfb938576..9e6b1ee925 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -4,11 +4,10 @@ use std::sync::Mutex; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; +use codex_config::ConfigLayerStack; +use codex_config::ConfigRequirementsToml; use codex_core_skills::SkillLoadOutcome; use codex_core_skills::injection::InjectedHostSkillPrompts; -use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS; -use codex_core_skills::loader::SkillRoot; -use codex_core_skills::loader::load_skills_from_roots; use codex_exec_server::LOCAL_FS; use codex_extension_api::ConversationHistory; use codex_extension_api::ExtensionData; @@ -44,6 +43,8 @@ use codex_protocol::protocol::TurnEnvironmentSelection; use codex_protocol::user_input::UserInput; use codex_skills::SkillMetadata; use codex_skills_extension::HostSkillProvider; +use codex_skills_extension::HostSkillsLoadInput; +use codex_skills_extension::HostSkillsService; use codex_skills_extension::HostSkillsSnapshot; use codex_skills_extension::SkillProviders; use codex_skills_extension::SkillsExtensionConfig; @@ -70,7 +71,6 @@ use opentelemetry_sdk::metrics::InMemoryMetricExporter; use opentelemetry_sdk::metrics::data::AggregatedMetrics; use opentelemetry_sdk::metrics::data::MetricData; use pretty_assertions::assert_eq; -use tokio::sync::Semaphore; type TestResult = Result<(), Box>; @@ -1876,22 +1876,34 @@ async fn host_catalog_compacts_shared_paths_under_budget_pressure() -> TestResul } let root = AbsolutePathBuf::try_from(std::fs::canonicalize(root)?)?; let rendered_root = root.to_string_lossy().replace('\\', "/"); - let outcome = load_skills_from_roots( - [SkillRoot { - path: root, - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: Default::default(), - }], - /*plugin_skill_snapshots*/ None, - Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), - ) - .await; - assert_eq!(outcome.errors, Vec::new()); - assert_eq!(outcome.skills.len(), 12 + usize::from(cfg!(unix))); + let config_layer_stack = ConfigLayerStack::new( + Vec::new(), + Default::default(), + ConfigRequirementsToml::default(), + )?; + let codex_home = AbsolutePathBuf::try_from(test_root.clone())?; + let service = HostSkillsService::new_with_restriction_product( + codex_home.clone(), + /*bundled_skills_enabled*/ false, + /*restriction_product*/ None, + ); + service.set_extra_roots(vec![root]); + let snapshot = service + .snapshot_for_config( + &HostSkillsLoadInput::new( + codex_home, + Vec::new(), + config_layer_stack, + /*bundled_skills_enabled*/ false, + ), + Some(Arc::clone(&LOCAL_FS)), + ) + .await; + assert_eq!(snapshot.outcome().errors, Vec::new()); + assert_eq!( + snapshot.outcome().skills.len(), + 12 + usize::from(cfg!(unix)) + ); let mut builder = ExtensionRegistryBuilder::new(); install(&mut builder, skills_extension_config); @@ -1916,7 +1928,7 @@ async fn host_catalog_compacts_shared_paths_under_budget_pressure() -> TestResul model_info.context_window = Some(10_000); thread_store.insert(model_info); let turn_store = ExtensionData::new("turn-1"); - turn_store.insert(HostSkillsSnapshot::new(Arc::new(outcome))); + turn_store.insert(snapshot); let fragments = registry.turn_input_contributors()[0] .contribute( diff --git a/codex-rs/skills/src/parser_tests.rs b/codex-rs/skills/src/parser_tests.rs index 2bbad1e2c0..d0fe46c679 100644 --- a/codex-rs/skills/src/parser_tests.rs +++ b/codex-rs/skills/src/parser_tests.rs @@ -41,3 +41,79 @@ fn uses_default_name_and_requires_description() { .expect_err("description should be required"); assert_eq!(error.to_string(), "missing field `description`"); } + +#[test] +fn repairs_short_descriptions_containing_colons_and_apostrophes() { + let parsed = parse_skill_frontmatter_metadata( + "---\nname: short\ndescription: Short skill\nmetadata:\n short-description: What's included: builds and tests\n---\n", + || "fallback".to_string(), + ) + .expect("frontmatter with an unquoted short description should be repaired"); + + assert_eq!( + parsed, + ParsedSkillFrontmatter { + name: "short".to_string(), + description: "Short skill".to_string(), + short_description: Some("What's included: builds and tests".to_string()), + } + ); +} + +#[test] +fn repairs_unrecognized_frontmatter_fields_that_need_quotes() { + let parsed = parse_skill_frontmatter_metadata( + "---\nname: unknown\ndescription: Unknown fields\nargument-hint: \ntags: [next,@supabase/ssr]\n---\n", + || "fallback".to_string(), + ) + .expect("frontmatter with unrecognized fields should be repaired"); + + assert_eq!( + parsed, + ParsedSkillFrontmatter { + name: "unknown".to_string(), + description: "Unknown fields".to_string(), + short_description: None, + } + ); +} + +#[test] +fn preserves_block_scalar_bodies_while_repairing_other_fields() { + let parsed = parse_skill_frontmatter_metadata( + "---\nname: block\ndescription: |-\n Build for AWS: ECS\nargument-hint: \n---\n", + || "fallback".to_string(), + ) + .expect("frontmatter repair should preserve block scalar bodies"); + + assert_eq!( + parsed, + ParsedSkillFrontmatter { + name: "block".to_string(), + description: "Build for AWS: ECS".to_string(), + short_description: None, + } + ); +} + +#[test] +fn preserves_overlong_descriptions_and_short_descriptions() { + let description = "💡".repeat(/*n*/ 1_025); + let short_description = "x".repeat(/*n*/ 1_025); + let parsed = parse_skill_frontmatter_metadata( + &format!( + "---\nname: long\ndescription: {description}\nmetadata:\n short-description: {short_description}\n---\n" + ), + || "fallback".to_string(), + ) + .expect("descriptions should not be truncated during frontmatter parsing"); + + assert_eq!( + parsed, + ParsedSkillFrontmatter { + name: "long".to_string(), + description, + short_description: Some(short_description), + } + ); +}