diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index f5ec6bc45b..03b585f717 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2922,7 +2922,6 @@ dependencies = [ "codex-utils-absolute-path", "codex-utils-path-uri", "codex-utils-plugins", - "dirs", "dunce", "futures", "pretty_assertions", @@ -4115,6 +4114,7 @@ dependencies = [ "codex-utils-path-uri", "codex-utils-plugins", "codex-utils-string", + "dirs", "dunce", "futures", "insta", diff --git a/codex-rs/core-skills/Cargo.toml b/codex-rs/core-skills/Cargo.toml index d175fceec9..61b16ab236 100644 --- a/codex-rs/core-skills/Cargo.toml +++ b/codex-rs/core-skills/Cargo.toml @@ -27,7 +27,6 @@ codex-skills = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-path-uri = { workspace = true } codex-utils-plugins = { workspace = true } -dirs = { workspace = true } dunce = { workspace = true } futures = { workspace = true } serde = { workspace = true, features = ["derive"] } @@ -35,10 +34,10 @@ serde_json = { workspace = true } serde_yaml = { workspace = true } shlex = { workspace = true } tokio = { workspace = true, features = ["fs", "macros", "rt"] } -toml = { workspace = true } tracing = { workspace = true } zip = { workspace = true } [dev-dependencies] pretty_assertions = { workspace = true } tempfile = { workspace = true } +toml = { workspace = true } diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs index 8b70ad57ae..71d2ef4584 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -7,7 +7,6 @@ pub mod model; pub mod remote; mod root_loader; mod skill_instructions; -pub mod system; pub(crate) use invocation_utils::build_implicit_skill_path_indexes; pub use invocation_utils::detect_implicit_skill_invocation_for_command; diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index f8e866a23a..347d415419 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -13,14 +13,7 @@ use crate::model::SkillLoadOutcome; use crate::model::SkillMetadata; use crate::model::SkillPolicy; use crate::model::SkillToolDependency; -use crate::system::system_cache_root_dir; -use codex_config::ConfigLayerSource; -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_exec_server::ExecutorFileSystem; -use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_skills::ParsedSkillFrontmatter; @@ -33,9 +26,7 @@ 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::PluginSkillRoot; use codex_utils_plugins::SkillDiscoveryMode; -use dirs::home_dir; use discovery::DirectorySymlinkPolicy; use discovery::DiscoveredSkill; use discovery::HiddenDirectoryPolicy; @@ -48,13 +39,11 @@ use futures::FutureExt; use futures::StreamExt; use namespace::SkillNamespaceResolver; use serde::Deserialize; -use std::collections::HashSet; use std::error::Error; use std::fmt; use std::io; use std::sync::Arc; use tokio::sync::Semaphore; -use toml::Value as TomlValue; use tracing::error; // TODO(anp): Tune this eight-scan limit after revisiting byte-based backpressure. @@ -103,10 +92,8 @@ struct DependencyTool { } const SKILLS_FILENAME: &str = "SKILL.md"; -const AGENTS_DIR_NAME: &str = ".agents"; const SKILLS_METADATA_DIR: &str = "agents"; const SKILLS_METADATA_FILENAME: &str = "openai.yaml"; -const SKILLS_DIR_NAME: &str = "skills"; const MAX_NAME_LEN: usize = 64; const MAX_QUALIFIED_NAME_LEN: usize = 128; const MAX_DESCRIPTION_LEN: usize = 1024; @@ -119,9 +106,6 @@ 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; -// Keep ancestor metadata probes within one remote round trip for typical project hierarchies while -// leaving room for other startup discovery on the shared exec-server transport. -const MAX_CONCURRENT_ANCESTOR_PROBES: usize = 256; struct ResolvedDiscoveredSkill { skill: DiscoveredSkill, @@ -197,277 +181,6 @@ pub(crate) async fn load_skill_root(root: SkillRoot) -> SkillRootSnapshot { } } -pub async fn skill_roots( - fs: Option>, - config_layer_stack: &ConfigLayerStack, - cwd: &AbsolutePathBuf, - plugin_skill_roots: Vec, - extra_skill_roots: Vec, -) -> Vec { - let home_dir = - home_dir().and_then(|path| AbsolutePathBuf::from_absolute_path_checked(path).ok()); - skill_roots_with_home_dir( - fs, - config_layer_stack, - cwd, - home_dir.as_ref(), - plugin_skill_roots, - extra_skill_roots, - ) - .await -} - -async fn skill_roots_with_home_dir( - fs: Option>, - config_layer_stack: &ConfigLayerStack, - cwd: &AbsolutePathBuf, - home_dir: Option<&AbsolutePathBuf>, - plugin_skill_roots: Vec, - extra_skill_roots: Vec, -) -> Vec { - let mut roots = skill_roots_from_layer_stack_inner(config_layer_stack, home_dir, fs.clone()); - 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, - })); - roots.extend(extra_skill_roots.into_iter().map(|path| SkillRoot { - path, - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - })); - roots.extend(repo_agents_skill_roots(fs, config_layer_stack, cwd).await); - dedupe_skill_roots_by_path(&mut roots); - roots -} - -fn skill_roots_from_layer_stack_inner( - config_layer_stack: &ConfigLayerStack, - home_dir: Option<&AbsolutePathBuf>, - repo_fs: Option>, -) -> Vec { - let mut roots = Vec::new(); - - for layer in config_layer_stack.all_layers_high_to_low() { - let Some(config_folder) = layer.config_folder() else { - continue; - }; - - match &layer.name { - ConfigLayerSource::Project { .. } => { - if let Some(repo_fs) = &repo_fs { - roots.push(SkillRoot { - path: config_folder.join(SKILLS_DIR_NAME), - scope: SkillScope::Repo, - file_system: Arc::clone(repo_fs), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }); - } - } - ConfigLayerSource::User { .. } => { - // Deprecated user skills location (`$CODEX_HOME/skills`), kept for backward - // compatibility. - roots.push(SkillRoot { - path: config_folder.join(SKILLS_DIR_NAME), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }); - - // `$HOME/.agents/skills` (user-installed skills). - if let Some(home_dir) = home_dir { - roots.push(SkillRoot { - path: home_dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }); - } - - // Embedded system skills are cached under `$CODEX_HOME/skills/.system` and are a - // special case (not a config layer). - roots.push(SkillRoot { - path: system_cache_root_dir(&config_folder), - scope: SkillScope::System, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }); - } - ConfigLayerSource::System { .. } => { - // The system config layer lives under `/etc/codex/` on Unix, so treat - // `/etc/codex/skills` as admin-scoped skills. - roots.push(SkillRoot { - path: config_folder.join(SKILLS_DIR_NAME), - scope: SkillScope::Admin, - file_system: Arc::clone(&LOCAL_FS), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }); - } - ConfigLayerSource::Mdm { .. } - | ConfigLayerSource::EnterpriseManaged { .. } - | ConfigLayerSource::SessionFlags - | ConfigLayerSource::LegacyManagedConfigTomlFromFile { .. } - | ConfigLayerSource::LegacyManagedConfigTomlFromMdm => {} - } - } - - roots -} - -async fn repo_agents_skill_roots( - fs: Option>, - config_layer_stack: &ConfigLayerStack, - cwd: &AbsolutePathBuf, -) -> Vec { - let Some(fs) = fs else { - return Vec::new(); - }; - let project_root_markers = project_root_markers_from_stack(config_layer_stack); - let project_root = find_project_root(fs.as_ref(), cwd, &project_root_markers).await; - let dirs = dirs_between_project_root_and_cwd(cwd, &project_root); - let mut roots = Vec::new(); - let mut results = futures::stream::iter(dirs) - .map(|dir| { - let fs = Arc::clone(&fs); - async move { - let agents_skills = dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME); - let agents_skills_uri = PathUri::from_abs_path(&agents_skills); - let result = fs.get_metadata(&agents_skills_uri, /*sandbox*/ None).await; - (agents_skills, result) - } - }) - .buffered(MAX_CONCURRENT_ANCESTOR_PROBES); - while let Some((agents_skills, result)) = results.next().await { - match result { - Ok(metadata) if metadata.is_directory => roots.push(SkillRoot { - path: agents_skills, - scope: SkillScope::Repo, - file_system: Arc::clone(&fs), - plugin_identity: None, - plugin_namespace: None, - plugin_root: None, - discovery_mode: SkillDiscoveryMode::Recursive, - }), - Ok(_) => {} - Err(err) if err.kind() == io::ErrorKind::NotFound => {} - Err(err) => { - tracing::warn!( - "failed to stat repo skills root {}: {err:#}", - agents_skills.display() - ); - } - } - } - roots -} - -fn project_root_markers_from_stack(config_layer_stack: &ConfigLayerStack) -> Vec { - let mut merged = TomlValue::Table(toml::map::Map::new()); - for layer in config_layer_stack.layers_low_to_high() { - if matches!(layer.name, ConfigLayerSource::Project { .. }) { - continue; - } - merge_toml_values(&mut merged, &layer.config); - } - - match project_root_markers_from_config(&merged) { - Ok(Some(markers)) => markers, - Ok(None) => default_project_root_markers(), - Err(err) => { - tracing::warn!("invalid project_root_markers: {err}"); - default_project_root_markers() - } - } -} - -async fn find_project_root( - fs: &dyn ExecutorFileSystem, - cwd: &AbsolutePathBuf, - project_root_markers: &[String], -) -> AbsolutePathBuf { - if project_root_markers.is_empty() { - return cwd.clone(); - } - - let mut probes = Vec::new(); - for ancestor in cwd.ancestors() { - for marker in project_root_markers { - let marker_path = ancestor.join(marker); - probes.push((ancestor.clone(), marker_path)); - } - } - let mut results = futures::stream::iter(probes) - .map(|(ancestor, marker_path)| async move { - let marker_path_uri = PathUri::from_abs_path(&marker_path); - let result = fs.get_metadata(&marker_path_uri, /*sandbox*/ None).await; - (ancestor, marker_path, result) - }) - .buffered(MAX_CONCURRENT_ANCESTOR_PROBES); - while let Some((ancestor, marker_path, result)) = results.next().await { - match result { - Ok(_) => return ancestor, - Err(err) if err.kind() == io::ErrorKind::NotFound => {} - Err(err) => { - tracing::warn!( - "failed to stat project root marker {}: {err:#}", - marker_path.display() - ); - } - } - } - - cwd.clone() -} - -fn dirs_between_project_root_and_cwd( - cwd: &AbsolutePathBuf, - project_root: &AbsolutePathBuf, -) -> Vec { - let mut dirs = cwd - .ancestors() - .scan(false, |done, dir| { - if *done { - None - } else { - if &dir == project_root { - *done = true; - } - Some(dir) - } - }) - .collect::>(); - dirs.reverse(); - dirs -} - -fn dedupe_skill_roots_by_path(roots: &mut Vec) { - let mut seen: HashSet = HashSet::new(); - roots.retain(|root| seen.insert(root.path.clone())); -} - async fn canonicalize_for_skill_identity( fs: &dyn ExecutorFileSystem, path: &AbsolutePathBuf, @@ -889,24 +602,6 @@ fn resolve_required_str( resolve_str(Some(value), max_len, field) } -#[cfg(test)] -pub(crate) async fn skill_roots_from_layer_stack( - fs: Arc, - config_layer_stack: &ConfigLayerStack, - cwd: &AbsolutePathBuf, - home_dir: Option<&AbsolutePathBuf>, -) -> Vec { - skill_roots_with_home_dir( - Some(fs), - config_layer_stack, - cwd, - home_dir, - Vec::new(), - Vec::new(), - ) - .await -} - #[cfg(test)] #[path = "loader_tests.rs"] mod tests; diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index 51185a7c42..055fa8a830 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -1,9 +1,4 @@ use super::*; -use codex_config::CONFIG_TOML_FILE; -use codex_config::ConfigLayerEntry; -use codex_config::ConfigLayerStack; -use codex_config::ConfigRequirements; -use codex_config::ConfigRequirementsToml; use codex_exec_server::CopyOptions; use codex_exec_server::CreateDirectoryOptions; use codex_exec_server::ExecutorFileSystem; @@ -28,46 +23,23 @@ use std::fs; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; -use std::sync::Mutex; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; use tempfile::TempDir; use tokio::sync::Notify; use tokio::sync::Semaphore; -use toml::Value as TomlValue; const REPO_ROOT_CONFIG_DIR_NAME: &str = ".codex"; - -struct TestConfig { - cwd: AbsolutePathBuf, - config_layer_stack: ConfigLayerStack, -} +const SKILLS_DIR_NAME: &str = "skills"; struct BlockingRepoSkillRootFileSystem { inner: Arc, - metadata_calls: Arc, blocked_walk_root: Option, blocked_walk_gate: Semaphore, walks_started: AtomicUsize, walk_started: Notify, } -struct BlockingMetadataCalls { - paths: Mutex>, - started: Notify, - release: Semaphore, -} - -impl Default for BlockingMetadataCalls { - fn default() -> Self { - Self { - paths: Mutex::new(Vec::new()), - started: Notify::new(), - release: Semaphore::new(0), - } - } -} - impl ExecutorFileSystem for BlockingRepoSkillRootFileSystem { fn canonicalize<'a>( &'a self, @@ -116,29 +88,7 @@ impl ExecutorFileSystem for BlockingRepoSkillRootFileSystem { path: &'a PathUri, sandbox: Option<&'a FileSystemSandboxContext>, ) -> ExecutorFileSystemFuture<'a, FileMetadata> { - let repo_skill_root_suffix = Path::new(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME); - let Ok(path_abs) = path.to_abs_path() else { - return self.inner.get_metadata(path, sandbox); - }; - if !path_abs.ends_with(repo_skill_root_suffix) { - return self.inner.get_metadata(path, sandbox); - } - - self.metadata_calls - .paths - .lock() - .expect("metadata paths lock") - .push(path.clone()); - self.metadata_calls.started.notify_one(); - Box::pin(async move { - self.metadata_calls - .release - .acquire() - .await - .expect("metadata release semaphore") - .forget(); - self.inner.get_metadata(path, sandbox).await - }) + self.inner.get_metadata(path, sandbox) } fn read_directory<'a>( @@ -191,118 +141,37 @@ impl ExecutorFileSystem for BlockingRepoSkillRootFileSystem { } } -async fn make_config(codex_home: &TempDir) -> TestConfig { - make_config_for_cwd(codex_home, codex_home.path().to_path_buf()).await -} - -fn config_file(path: PathBuf) -> AbsolutePathBuf { - path.abs() -} - -fn project_layers_for_cwd(cwd: &Path) -> Vec { - let cwd_dir = if cwd.is_dir() { - cwd.to_path_buf() - } else { - cwd.parent() - .expect("file cwd should have a parent directory") - .to_path_buf() - }; - let project_root = cwd_dir - .ancestors() - .find(|ancestor| ancestor.join(".git").exists()) - .unwrap_or(cwd_dir.as_path()) - .to_path_buf(); - - let mut layers = cwd_dir - .ancestors() - .scan(false, |done, dir| { - if *done { - None - } else { - if dir == project_root { - *done = true; - } - Some(dir.to_path_buf()) - } - }) - .collect::>(); - layers.reverse(); - - layers - .into_iter() - .filter_map(|dir| { - let dot_codex = dir.join(REPO_ROOT_CONFIG_DIR_NAME); - dot_codex.is_dir().then(|| { - ConfigLayerEntry::new( - ConfigLayerSource::Project { - dot_codex_folder: dot_codex.abs(), - }, - TomlValue::Table(toml::map::Map::new()), - ) - }) - }) - .collect() -} - -async fn make_config_for_cwd(codex_home: &TempDir, cwd: PathBuf) -> TestConfig { - let user_config_path = codex_home.path().join(CONFIG_TOML_FILE); - let system_config_path = codex_home.path().join("etc/codex/config.toml"); - fs::create_dir_all( - system_config_path - .parent() - .expect("system config path should have a parent"), - ) - .expect("create fake system config dir"); - - let mut layers = vec![ - ConfigLayerEntry::new( - ConfigLayerSource::System { - file: config_file(system_config_path), - }, - TomlValue::Table(toml::map::Map::new()), - ), - ConfigLayerEntry::new( - ConfigLayerSource::User { - file: config_file(user_config_path), - profile: None, - }, - TomlValue::Table(toml::map::Map::new()), - ), - ]; - layers.extend(project_layers_for_cwd(&cwd)); - - let cwd_abs = cwd.abs(); - TestConfig { - cwd: cwd_abs, - config_layer_stack: ConfigLayerStack::new( - layers, - ConfigRequirements::default(), - ConfigRequirementsToml::default(), - ) - .expect("valid config layer stack"), - } -} - -async fn load_skills_for_test(config: &TestConfig) -> SkillLoadOutcome { - // Keep unit tests hermetic by never scanning the real `$HOME/.agents/skills`. +async fn load_skills_for_test(roots: I) -> SkillLoadOutcome +where + I: IntoIterator + Send, + I::IntoIter: Send, +{ super::load_skills_from_roots( - super::skill_roots_from_layer_stack( - Arc::clone(&LOCAL_FS), - &config.config_layer_stack, - &config.cwd, - /*home_dir*/ None, - ) - .await, + roots, /*plugin_skill_snapshots*/ None, Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), ) .await } -fn mark_as_git_repo(dir: &Path) { - // Config/project-root discovery only checks for the presence of `.git` (file or dir), - // so we can avoid shelling out to `git init` in tests. - fs::write(dir.join(".git"), "gitdir: fake\n").unwrap(); +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 { @@ -310,208 +179,6 @@ fn normalized(path: &Path) -> AbsolutePathBuf { .unwrap_or_else(|_| path.to_path_buf()) .abs() } - -#[tokio::test] -async fn skill_roots_from_layer_stack_maps_user_to_user_and_system_cache_and_system_to_admin() --> anyhow::Result<()> { - let tmp = tempfile::tempdir()?; - - let system_folder = tmp.path().join("etc/codex"); - let home_folder = tmp.path().join("home"); - let user_folder = home_folder.join("codex"); - fs::create_dir_all(&system_folder)?; - fs::create_dir_all(&user_folder)?; - - // The file path doesn't need to exist; it's only used to derive the config folder. - let system_file = system_folder.join("config.toml").abs(); - let user_file = user_folder.join("config.toml").abs(); - - let layers = vec![ - ConfigLayerEntry::new( - ConfigLayerSource::System { file: system_file }, - TomlValue::Table(toml::map::Map::new()), - ), - ConfigLayerEntry::new( - ConfigLayerSource::User { - file: user_file, - profile: None, - }, - TomlValue::Table(toml::map::Map::new()), - ), - ]; - let stack = ConfigLayerStack::new( - layers, - ConfigRequirements::default(), - ConfigRequirementsToml::default(), - )?; - - let home_folder_abs = home_folder.abs(); - let got = skill_roots_from_layer_stack( - Arc::clone(&LOCAL_FS), - &stack, - &home_folder_abs, - Some(&home_folder_abs), - ) - .await - .into_iter() - .map(|root| (root.scope, root.path.to_path_buf())) - .collect::>(); - - assert_eq!( - got, - vec![ - (SkillScope::User, user_folder.join("skills")), - ( - SkillScope::User, - home_folder.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME) - ), - ( - SkillScope::System, - user_folder.join("skills").join(".system") - ), - (SkillScope::Admin, system_folder.join("skills")), - ] - ); - - Ok(()) -} - -#[tokio::test] -async fn skill_roots_from_layer_stack_includes_disabled_project_layers() -> anyhow::Result<()> { - let tmp = tempfile::tempdir()?; - - let home_folder = tmp.path().join("home"); - let user_folder = home_folder.join("codex"); - fs::create_dir_all(&user_folder)?; - - let project_root = tmp.path().join("repo"); - let dot_codex = project_root.join(".codex"); - fs::create_dir_all(&dot_codex)?; - - let user_file = user_folder.join("config.toml").abs(); - let project_dot_codex = dot_codex.abs(); - - let layers = vec![ - ConfigLayerEntry::new( - ConfigLayerSource::User { - file: user_file, - profile: None, - }, - TomlValue::Table(toml::map::Map::new()), - ), - ConfigLayerEntry::new_disabled( - ConfigLayerSource::Project { - dot_codex_folder: project_dot_codex, - }, - TomlValue::Table(toml::map::Map::new()), - "marked untrusted", - ), - ]; - let stack = ConfigLayerStack::new( - layers, - ConfigRequirements::default(), - ConfigRequirementsToml::default(), - )?; - - let home_folder_abs = home_folder.abs(); - let project_root_abs = project_root.abs(); - let got = skill_roots_from_layer_stack( - Arc::clone(&LOCAL_FS), - &stack, - &project_root_abs, - Some(&home_folder_abs), - ) - .await - .into_iter() - .map(|root| (root.scope, root.path.to_path_buf())) - .collect::>(); - - assert_eq!( - got, - vec![ - (SkillScope::Repo, dot_codex.join("skills")), - (SkillScope::User, user_folder.join("skills")), - ( - SkillScope::User, - home_folder.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME) - ), - ( - SkillScope::System, - user_folder.join("skills").join(".system") - ), - ] - ); - - Ok(()) -} - -#[tokio::test] -async fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<()> { - let tmp = tempfile::tempdir()?; - - let home_folder = tmp.path().join("home"); - let user_folder = home_folder.join("codex"); - fs::create_dir_all(&user_folder)?; - - let user_file = user_folder.join("config.toml").abs(); - let layers = vec![ConfigLayerEntry::new( - ConfigLayerSource::User { - file: user_file, - profile: None, - }, - TomlValue::Table(toml::map::Map::new()), - )]; - let stack = ConfigLayerStack::new( - layers, - ConfigRequirements::default(), - ConfigRequirementsToml::default(), - )?; - - let skill_path = write_skill_at( - &home_folder.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME), - "agents-home", - "agents-home-skill", - "from home agents", - ); - - let home_folder_abs = home_folder.abs(); - let roots = skill_roots_from_layer_stack( - Arc::clone(&LOCAL_FS), - &stack, - &home_folder_abs, - Some(&home_folder_abs), - ) - .await; - let outcome = load_skills_from_roots( - roots, - /*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: "agents-home-skill".to_string(), - description: "from home agents".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, - }] - ); - - Ok(()) -} - fn write_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) -> PathBuf { write_skill_at(&codex_home.path().join("skills"), dir, name, description) } @@ -635,8 +302,7 @@ async fn loads_skill_dependencies_metadata_from_yaml() { "#, ); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), @@ -707,8 +373,7 @@ interface: "##, ); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), @@ -758,8 +423,7 @@ policy: "#, ); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), @@ -790,8 +454,7 @@ policy: {} "#, ); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), @@ -829,8 +492,7 @@ policy: "#, ); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), @@ -939,8 +601,7 @@ async fn loads_skills_via_symlinked_subdir_for_user_scope() { fs::create_dir_all(codex_home.path().join("skills")).unwrap(); symlink_dir(shared.path(), &codex_home.path().join("skills/shared")); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), @@ -1002,8 +663,7 @@ async fn ignores_symlinked_skill_file_for_user_scope() { fs::create_dir_all(&skill_dir).unwrap(); symlink_file(&shared_skill_path, &skill_dir.join(SKILLS_FILENAME)); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), @@ -1026,8 +686,7 @@ async fn does_not_loop_on_symlink_cycle_for_user_scope() { let skill_path = write_skill_at(&cycle_dir, "demo", "cycle-skill", "still loads"); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), @@ -1102,9 +761,7 @@ async fn loads_skills_via_symlinked_subdir_for_admin_scope() { #[tokio::test] #[cfg(unix)] async fn loads_skills_via_symlinked_subdir_for_repo_scope() { - let codex_home = tempfile::tempdir().expect("tempdir"); let repo_dir = tempfile::tempdir().expect("tempdir"); - mark_as_git_repo(repo_dir.path()); let shared = tempfile::tempdir().expect("tempdir"); let linked_skill_path = write_skill_at(shared.path(), "demo", "repo-linked-skill", "from link"); @@ -1115,8 +772,8 @@ async fn loads_skills_via_symlinked_subdir_for_repo_scope() { fs::create_dir_all(&repo_skills_root).unwrap(); symlink_dir(shared.path(), &repo_skills_root.join("shared")); - let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = + load_skills_for_test([local_skill_root(&repo_skills_root, SkillScope::Repo)]).await; assert!( outcome.errors.is_empty(), @@ -1233,9 +890,7 @@ async fn respects_max_scan_depth_for_user_scope() { 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 cfg = make_config(&codex_home).await; - - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1266,9 +921,7 @@ async fn falls_back_to_directory_name_when_skill_name_is_missing() { "directory-derived", "description: fallback name", ); - let cfg = make_config(&codex_home).await; - - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), @@ -1729,8 +1382,7 @@ async fn loads_short_description_from_metadata() { let skill_path = skill_dir.join(SKILLS_FILENAME); fs::write(&skill_path, contents).unwrap(); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1762,8 +1414,7 @@ async fn loads_unquoted_description_containing_colon_space() { "name: colon-description\ndescription: AWS deployment patterns: ECS Fargate, Lambda, and S3", ); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1795,8 +1446,7 @@ async fn loads_unquoted_short_description_containing_colon_space_and_apostrophe( "name: colon-short-description\ndescription: long description\nmetadata:\n short-description: What's included: builds and tests", ); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1828,8 +1478,7 @@ async fn loads_unrecognized_frontmatter_fields_that_need_quotes() { "name: repaired-unknown-fields\ndescription: valid description\nargument-hint: \ntags: [next,@supabase/ssr]", ); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1861,8 +1510,7 @@ async fn preserves_block_scalar_body_while_repairing_other_fields() { "name: block-description-with-repair\ndescription: |-\n Build for AWS: ECS\nargument-hint: ", ); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1896,8 +1544,7 @@ async fn preserves_overlong_short_descriptions() { ); fs::write(skill_dir.join(SKILLS_FILENAME), contents).unwrap(); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1923,8 +1570,7 @@ async fn skips_hidden_and_invalid() { fs::create_dir_all(&invalid_dir).unwrap(); fs::write(invalid_dir.join(SKILLS_FILENAME), "---\nname: bad").unwrap(); - let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert_eq!(outcome.skills.len(), 0); assert_eq!(outcome.errors.len(), 1); assert!( @@ -1940,9 +1586,8 @@ 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 cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1952,7 +1597,7 @@ async fn preserves_overlong_descriptions() { 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_skills_for_test(&cfg).await; + let outcome = load_user_skills_for_test(&codex_home).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1968,19 +1613,16 @@ async fn preserves_overlong_descriptions() { } #[tokio::test] -async fn loads_skills_from_repo_root() { - let codex_home = tempfile::tempdir().expect("tempdir"); +async fn loads_skills_from_repo_scoped_root() { let repo_dir = tempfile::tempdir().expect("tempdir"); - mark_as_git_repo(repo_dir.path()); 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 cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_skills_for_test([local_skill_root(&skills_root, SkillScope::Repo)]).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -2002,76 +1644,28 @@ async fn loads_skills_from_repo_root() { }] ); } - #[tokio::test] -async fn loads_skills_from_agents_dir_without_codex_dir() { - let codex_home = tempfile::tempdir().expect("tempdir"); +async fn loads_skills_from_multiple_repo_scoped_roots() { let repo_dir = tempfile::tempdir().expect("tempdir"); - mark_as_git_repo(repo_dir.path()); - let skill_path = write_skill_at( - &repo_dir.path().join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME), - "agents", - "agents-skill", - "from agents", - ); - let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; + 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(&cfg).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "agents-skill".to_string(), - description: "from agents".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_all_codex_dirs_under_project_root() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let repo_dir = tempfile::tempdir().expect("tempdir"); - mark_as_git_repo(repo_dir.path()); - - let nested_dir = repo_dir.path().join("nested/inner"); - fs::create_dir_all(&nested_dir).unwrap(); - - let root_skill_path = write_skill_at( - &repo_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - "root", - "root-skill", - "from root", - ); - let nested_skill_path = write_skill_at( - &repo_dir - .path() - .join("nested") - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - "nested", - "nested-skill", - "from nested", - ); - - let cfg = make_config_for_cwd(&codex_home, nested_dir).await; - - let outcome = load_skills_for_test(&cfg).await; + 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: {:?}", @@ -2107,117 +1701,6 @@ async fn loads_skills_from_all_codex_dirs_under_project_root() { ] ); } - -#[tokio::test] -async fn repo_skill_root_search_limits_concurrent_probes_and_preserves_order() { - const CONCURRENCY_LIMIT: usize = 256; - - let codex_home = tempfile::tempdir().expect("tempdir"); - let repo_dir = tempfile::tempdir().expect("tempdir"); - mark_as_git_repo(repo_dir.path()); - - let mut directories = vec![repo_dir.path().to_path_buf()]; - let mut cwd = repo_dir.path().to_path_buf(); - for _ in 0..CONCURRENCY_LIMIT { - cwd.push("d"); - directories.push(cwd.clone()); - } - fs::create_dir_all(&cwd).expect("nested cwd"); - - let expected_roots = [0, CONCURRENCY_LIMIT / 2, CONCURRENCY_LIMIT] - .map(|index| { - directories[index] - .join(AGENTS_DIR_NAME) - .join(SKILLS_DIR_NAME) - }) - .map(|path| { - fs::create_dir_all(&path).expect("repo skill root"); - path.abs() - }); - let expected_probes = directories - .iter() - .map(|directory| { - PathUri::from_abs_path(&directory.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME).abs()) - }) - .collect::>(); - let cfg = make_config_for_cwd(&codex_home, cwd).await; - let metadata_calls = Arc::new(BlockingMetadataCalls::default()); - let fs: Arc = Arc::new(BlockingRepoSkillRootFileSystem { - inner: Arc::clone(&LOCAL_FS), - metadata_calls: Arc::clone(&metadata_calls), - blocked_walk_root: None, - blocked_walk_gate: Semaphore::new(/*permits*/ 0), - walks_started: AtomicUsize::new(/*v*/ 0), - walk_started: Notify::new(), - }); - - let assertions = async { - tokio::time::timeout(std::time::Duration::from_secs(5), async { - loop { - let started = metadata_calls.started.notified(); - if metadata_calls - .paths - .lock() - .expect("metadata paths lock") - .len() - >= CONCURRENCY_LIMIT - { - break; - } - started.await; - } - }) - .await - .expect("initial repo skill root window should start"); - assert_eq!( - metadata_calls - .paths - .lock() - .expect("metadata paths lock") - .as_slice(), - &expected_probes[..CONCURRENCY_LIMIT] - ); - - metadata_calls.release.add_permits(1); - tokio::time::timeout(std::time::Duration::from_secs(5), async { - loop { - let started = metadata_calls.started.notified(); - if metadata_calls - .paths - .lock() - .expect("metadata paths lock") - .len() - > CONCURRENCY_LIMIT - { - break; - } - started.await; - } - }) - .await - .expect("next repo skill root probe should start"); - assert_eq!( - metadata_calls - .paths - .lock() - .expect("metadata paths lock") - .as_slice(), - expected_probes.as_slice() - ); - - metadata_calls.release.add_permits(expected_probes.len()); - }; - let (roots, ()) = tokio::join!( - super::repo_agents_skill_roots(Some(fs), &cfg.config_layer_stack, &cfg.cwd), - assertions - ); - - assert_eq!( - roots.into_iter().map(|root| root.path).collect::>(), - expected_roots - ); -} - #[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; @@ -2242,7 +1725,6 @@ async fn merges_root_results_in_input_order_when_scans_finish_out_of_order() { let blocked_walk_root = PathUri::from_abs_path(&roots[0].abs()); let file_system = Arc::new(BlockingRepoSkillRootFileSystem { inner: Arc::clone(&LOCAL_FS), - metadata_calls: Arc::new(BlockingMetadataCalls::default()), blocked_walk_root: Some(blocked_walk_root), blocked_walk_gate: Semaphore::new(/*permits*/ 0), walks_started: AtomicUsize::new(/*v*/ 0), @@ -2344,46 +1826,6 @@ async fn skill_root_scans_wait_for_shared_capacity() { assert_eq!(outcome.errors, Vec::new()); } -#[tokio::test] -async fn loads_skills_from_codex_dir_when_not_git_repo() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let work_dir = tempfile::tempdir().expect("tempdir"); - - let skill_path = write_skill_at( - &work_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - "local", - "local-skill", - "from cwd", - ); - - let cfg = make_config_for_cwd(&codex_home, work_dir.path().to_path_buf()).await; - - let outcome = load_skills_for_test(&cfg).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "local-skill".to_string(), - description: "from cwd".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 deduplicates_by_path_preferring_first_root() { let root = tempfile::tempdir().expect("tempdir"); @@ -2442,22 +1884,19 @@ async fn deduplicates_by_path_preferring_first_root() { async fn keeps_duplicate_names_from_repo_and_user() { let codex_home = tempfile::tempdir().expect("tempdir"); let repo_dir = tempfile::tempdir().expect("tempdir"); - mark_as_git_repo(repo_dir.path()); let user_skill_path = write_skill(&codex_home, "user", "dupe-skill", "from user"); - let repo_skill_path = write_skill_at( - &repo_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - "repo", - "dupe-skill", - "from repo", - ); + 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 cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; - - let outcome = load_skills_for_test(&cfg).await; + 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: {:?}", @@ -2496,35 +1935,26 @@ async fn keeps_duplicate_names_from_repo_and_user() { #[tokio::test] async fn keeps_duplicate_names_from_nested_codex_dirs() { - let codex_home = tempfile::tempdir().expect("tempdir"); let repo_dir = tempfile::tempdir().expect("tempdir"); - mark_as_git_repo(repo_dir.path()); - let nested_dir = repo_dir.path().join("nested/inner"); - fs::create_dir_all(&nested_dir).unwrap(); + 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 root_skill_path = write_skill_at( - &repo_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - "root", - "dupe-skill", - "from root", - ); - let nested_skill_path = write_skill_at( - &repo_dir - .path() - .join("nested") - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - "nested", - "dupe-skill", - "from nested", - ); - - let cfg = make_config_for_cwd(&codex_home, nested_dir).await; - let outcome = load_skills_for_test(&cfg).await; + 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(), @@ -2569,117 +1999,14 @@ async fn keeps_duplicate_names_from_nested_codex_dirs() { ] ); } - #[tokio::test] -async fn repo_skills_search_does_not_escape_repo_root() { +async fn loads_skills_from_system_scoped_root() { let codex_home = tempfile::tempdir().expect("tempdir"); - let outer_dir = tempfile::tempdir().expect("tempdir"); - let repo_dir = outer_dir.path().join("repo"); - fs::create_dir_all(&repo_dir).unwrap(); - - let _skill_path = write_skill_at( - &outer_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - "outer", - "outer-skill", - "from outer", - ); - mark_as_git_repo(&repo_dir); - - let cfg = make_config_for_cwd(&codex_home, repo_dir).await; - - let outcome = load_skills_for_test(&cfg).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills.len(), 0); -} - -#[tokio::test] -async fn loads_skills_when_cwd_is_file_in_repo() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let repo_dir = tempfile::tempdir().expect("tempdir"); - mark_as_git_repo(repo_dir.path()); - - let skill_path = write_skill_at( - &repo_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - "repo", - "repo-skill", - "from repo", - ); - let file_path = repo_dir.path().join("some-file.txt"); - fs::write(&file_path, "contents").unwrap(); - - let cfg = make_config_for_cwd(&codex_home, file_path).await; - - let outcome = load_skills_for_test(&cfg).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 non_git_repo_skills_search_does_not_walk_parents() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let outer_dir = tempfile::tempdir().expect("tempdir"); - let nested_dir = outer_dir.path().join("nested/inner"); - fs::create_dir_all(&nested_dir).unwrap(); - - write_skill_at( - &outer_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - "outer", - "outer-skill", - "from outer", - ); - - let cfg = make_config_for_cwd(&codex_home, nested_dir).await; - - let outcome = load_skills_for_test(&cfg).await; - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!(outcome.skills.len(), 0); -} - -#[tokio::test] -async fn loads_skills_from_system_cache_when_present() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let work_dir = 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 cfg = make_config_for_cwd(&codex_home, work_dir.path().to_path_buf()).await; - - let outcome = load_skills_for_test(&cfg).await; + let outcome = load_skills_for_test([local_skill_root(&system_root, SkillScope::System)]).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -2701,27 +2028,3 @@ async fn loads_skills_from_system_cache_when_present() { }] ); } - -#[tokio::test] -async fn skill_roots_include_admin_with_lowest_priority() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let cfg = make_config(&codex_home).await; - - let scopes: Vec = super::skill_roots( - Some(Arc::clone(&LOCAL_FS)), - &cfg.config_layer_stack, - &cfg.cwd, - Vec::new(), - Vec::new(), - ) - .await - .into_iter() - .map(|root| root.scope) - .collect(); - let mut expected = vec![SkillScope::User, SkillScope::System]; - if home_dir().is_some() { - expected.insert(1, SkillScope::User); - } - expected.push(SkillScope::Admin); - assert_eq!(scopes, expected); -} diff --git a/codex-rs/core-skills/src/system.rs b/codex-rs/core-skills/src/system.rs deleted file mode 100644 index 3031246190..0000000000 --- a/codex-rs/core-skills/src/system.rs +++ /dev/null @@ -1 +0,0 @@ -pub(crate) use codex_skills::system_cache_root_dir; diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index fb53a1ea8b..c965b97f4c 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -26,7 +26,6 @@ pub use codex_core_skills::injection::collect_explicit_skill_mentions; pub use codex_core_skills::loader; pub use codex_core_skills::model; pub use codex_core_skills::remote; -pub use codex_core_skills::system; pub use codex_skills::SkillMetadata; pub use codex_skills::SkillPolicy; pub use codex_skills_extension::HostSkillsLoadInput; diff --git a/codex-rs/ext/skills/Cargo.toml b/codex-rs/ext/skills/Cargo.toml index a79e1188fc..c1f09ccbbf 100644 --- a/codex-rs/ext/skills/Cargo.toml +++ b/codex-rs/ext/skills/Cargo.toml @@ -26,12 +26,14 @@ codex-utils-absolute-path = { workspace = true } codex-utils-path-uri = { workspace = true } codex-utils-plugins = { workspace = true } codex-utils-string = { workspace = true } +dirs = { workspace = true } futures = { workspace = true } schemars = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } serde_yaml = { workspace = true } tokio = { workspace = true, features = ["sync", "time"] } +toml = { workspace = true } tracing = { workspace = true } url = { workspace = true } @@ -44,4 +46,3 @@ opentelemetry_sdk = { workspace = true } pretty_assertions = { workspace = true } tempfile = { workspace = true } tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } -toml = { workspace = true } diff --git a/codex-rs/ext/skills/src/host_roots.rs b/codex-rs/ext/skills/src/host_roots.rs new file mode 100644 index 0000000000..dfa4652c82 --- /dev/null +++ b/codex-rs/ext/skills/src/host_roots.rs @@ -0,0 +1,302 @@ +use std::collections::HashSet; +use std::io; +use std::sync::Arc; + +use codex_config::ConfigLayerSource; +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; +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; + +use crate::loader::HostSkillRoot; + +const AGENTS_DIR_NAME: &str = ".agents"; +const SKILLS_DIR_NAME: &str = "skills"; +const MAX_CONCURRENT_ANCESTOR_PROBES: usize = 256; + +pub(crate) async fn resolve_skill_roots( + repository_file_system: Option>, + config_layer_stack: &ConfigLayerStack, + cwd: &AbsolutePathBuf, + plugin_skill_roots: Vec, + extra_skill_roots: Vec, +) -> Vec { + let home_dir = + home_dir().and_then(|path| AbsolutePathBuf::from_absolute_path_checked(path).ok()); + resolve_skill_roots_with_home_dir( + repository_file_system, + config_layer_stack, + cwd, + home_dir.as_ref(), + plugin_skill_roots, + extra_skill_roots, + ) + .await +} + +async fn resolve_skill_roots_with_home_dir( + repository_file_system: Option>, + config_layer_stack: &ConfigLayerStack, + cwd: &AbsolutePathBuf, + home_dir: Option<&AbsolutePathBuf>, + plugin_skill_roots: Vec, + extra_skill_roots: Vec, +) -> Vec { + let mut roots = + roots_from_layer_stack(config_layer_stack, home_dir, repository_file_system.clone()) + .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, + })); + 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), + ); + dedupe_skill_roots_by_path(&mut roots); + roots +} + +fn roots_from_layer_stack( + config_layer_stack: &ConfigLayerStack, + home_dir: Option<&AbsolutePathBuf>, + repository_file_system: Option>, +) -> Vec { + let mut roots = Vec::new(); + + for layer in config_layer_stack.all_layers_high_to_low() { + let Some(config_folder) = layer.config_folder() else { + continue; + }; + + match &layer.name { + ConfigLayerSource::Project { .. } => { + if let Some(repository_file_system) = &repository_file_system { + roots.push(HostSkillRoot { + path: config_folder.join(SKILLS_DIR_NAME), + scope: SkillScope::Repo, + file_system: Arc::clone(repository_file_system), + plugin_root: None, + }); + } + } + ConfigLayerSource::User { .. } => { + // Deprecated user skills location (`$CODEX_HOME/skills`), kept for backward + // compatibility. + roots.push(local_root( + config_folder.join(SKILLS_DIR_NAME), + SkillScope::User, + )); + + if let Some(home_dir) = home_dir { + roots.push(local_root( + home_dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME), + SkillScope::User, + )); + } + + roots.push(local_root( + system_cache_root_dir(&config_folder), + SkillScope::System, + )); + } + ConfigLayerSource::System { .. } => { + roots.push(local_root( + config_folder.join(SKILLS_DIR_NAME), + SkillScope::Admin, + )); + } + ConfigLayerSource::Mdm { .. } + | ConfigLayerSource::EnterpriseManaged { .. } + | ConfigLayerSource::SessionFlags + | ConfigLayerSource::LegacyManagedConfigTomlFromFile { .. } + | ConfigLayerSource::LegacyManagedConfigTomlFromMdm => {} + } + } + + roots +} + +fn local_root(path: AbsolutePathBuf, scope: SkillScope) -> HostSkillRoot { + HostSkillRoot { + path, + scope, + file_system: Arc::clone(&LOCAL_FS), + plugin_root: None, + } +} + +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, + cwd: &AbsolutePathBuf, +) -> Vec { + let Some(repository_file_system) = repository_file_system else { + return Vec::new(); + }; + let project_root_markers = project_root_markers_from_stack(config_layer_stack); + let project_root = + find_project_root(repository_file_system.as_ref(), cwd, &project_root_markers).await; + let directories = dirs_between_project_root_and_cwd(cwd, &project_root); + let mut roots = Vec::new(); + let mut results = futures::stream::iter(directories) + .map(|directory| { + let repository_file_system = Arc::clone(&repository_file_system); + async move { + let agents_skills = directory.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME); + let agents_skills_uri = PathUri::from_abs_path(&agents_skills); + let result = repository_file_system + .get_metadata(&agents_skills_uri, /*sandbox*/ None) + .await; + (agents_skills, result) + } + }) + .buffered(MAX_CONCURRENT_ANCESTOR_PROBES); + while let Some((agents_skills, result)) = results.next().await { + match result { + Ok(metadata) if metadata.is_directory => roots.push(HostSkillRoot { + path: agents_skills, + scope: SkillScope::Repo, + file_system: Arc::clone(&repository_file_system), + plugin_root: None, + }), + Ok(_) => {} + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => { + tracing::warn!( + "failed to stat repo skills root {}: {error:#}", + agents_skills.display() + ); + } + } + } + roots +} + +fn project_root_markers_from_stack(config_layer_stack: &ConfigLayerStack) -> Vec { + let mut merged = TomlValue::Table(toml::map::Map::new()); + for layer in config_layer_stack.layers_low_to_high() { + if matches!(layer.name, ConfigLayerSource::Project { .. }) { + continue; + } + merge_toml_values(&mut merged, &layer.config); + } + + match project_root_markers_from_config(&merged) { + Ok(Some(markers)) => markers, + Ok(None) => default_project_root_markers(), + Err(error) => { + tracing::warn!("invalid project_root_markers: {error}"); + default_project_root_markers() + } + } +} + +async fn find_project_root( + repository_file_system: &dyn ExecutorFileSystem, + cwd: &AbsolutePathBuf, + project_root_markers: &[String], +) -> AbsolutePathBuf { + if project_root_markers.is_empty() { + return cwd.clone(); + } + + let mut probes = Vec::new(); + for ancestor in cwd.ancestors() { + for marker in project_root_markers { + probes.push((ancestor.clone(), ancestor.join(marker))); + } + } + let mut results = futures::stream::iter(probes) + .map(|(ancestor, marker_path)| async move { + let marker_path_uri = PathUri::from_abs_path(&marker_path); + let result = repository_file_system + .get_metadata(&marker_path_uri, /*sandbox*/ None) + .await; + (ancestor, marker_path, result) + }) + .buffered(MAX_CONCURRENT_ANCESTOR_PROBES); + while let Some((ancestor, marker_path, result)) = results.next().await { + match result { + Ok(_) => return ancestor, + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => { + tracing::warn!( + "failed to stat project root marker {}: {error:#}", + marker_path.display() + ); + } + } + } + + cwd.clone() +} + +fn dirs_between_project_root_and_cwd( + cwd: &AbsolutePathBuf, + project_root: &AbsolutePathBuf, +) -> Vec { + let mut directories = cwd + .ancestors() + .scan(false, |done, directory| { + if *done { + None + } else { + if &directory == project_root { + *done = true; + } + Some(directory) + } + }) + .collect::>(); + directories.reverse(); + directories +} + +fn dedupe_skill_roots_by_path(roots: &mut Vec) { + let mut seen = HashSet::new(); + roots.retain(|root| seen.insert(root.path.clone())); +} + +#[cfg(test)] +#[path = "host_roots_tests.rs"] +mod tests; diff --git a/codex-rs/ext/skills/src/host_roots_tests.rs b/codex-rs/ext/skills/src/host_roots_tests.rs new file mode 100644 index 0000000000..b31072ae69 --- /dev/null +++ b/codex-rs/ext/skills/src/host_roots_tests.rs @@ -0,0 +1,668 @@ +use std::fs; +use std::path::Path; +use std::sync::Arc; +use std::sync::Mutex; + +use codex_config::ConfigLayerEntry; +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; +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::SkillScope; +use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; +use codex_utils_plugins::PluginIdentity; +use codex_utils_plugins::PluginSkillRoot; +use codex_utils_plugins::SkillDiscoveryMode; +use pretty_assertions::assert_eq; +use tempfile::TempDir; +use tokio::sync::Notify; +use tokio::sync::Semaphore; + +use super::repo_agents_skill_roots; +use super::resolve_skill_roots_with_home_dir; +use super::roots_from_layer_stack; + +struct BlockingMetadataFileSystem { + inner: Arc, + calls: Arc, +} + +struct BlockingMetadataCalls { + paths: Mutex>, + started: Notify, + release: Semaphore, +} + +impl Default for BlockingMetadataCalls { + fn default() -> Self { + Self { + paths: Mutex::new(Vec::new()), + started: Notify::new(), + release: Semaphore::new(0), + } + } +} + +impl ExecutorFileSystem for BlockingMetadataFileSystem { + 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> { + let Ok(path_abs) = path.to_abs_path() else { + return self.inner.get_metadata(path, sandbox); + }; + let repo_skill_root_suffix = Path::new(".agents").join("skills"); + if !path_abs.ends_with(repo_skill_root_suffix) { + return self.inner.get_metadata(path, sandbox); + } + + self.calls + .paths + .lock() + .expect("metadata paths lock") + .push(path.clone()); + self.calls.started.notify_one(); + Box::pin(async move { + self.calls + .release + .acquire() + .await + .expect("metadata release semaphore") + .forget(); + self.inner.get_metadata(path, sandbox).await + }) + } + + 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.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) + } +} + +fn absolute(path: impl Into) -> AbsolutePathBuf { + AbsolutePathBuf::try_from(path.into()).expect("absolute path") +} + +fn empty_config() -> toml::Value { + toml::Value::Table(toml::map::Map::new()) +} + +fn stack(layers: Vec) -> ConfigLayerStack { + ConfigLayerStack::new( + layers, + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config stack") +} + +fn user_layer(codex_home: &AbsolutePathBuf) -> ConfigLayerEntry { + ConfigLayerEntry::new( + ConfigLayerSource::User { + file: codex_home.join("config.toml"), + profile: None, + }, + empty_config(), + ) +} + +fn project_layer(dot_codex_folder: &AbsolutePathBuf) -> ConfigLayerEntry { + ConfigLayerEntry::new( + ConfigLayerSource::Project { + dot_codex_folder: dot_codex_folder.clone(), + }, + empty_config(), + ) +} + +fn write_skill(root: &AbsolutePathBuf, directory: &str, name: &str) -> AbsolutePathBuf { + let skill_dir = root.join(directory); + fs::create_dir_all(&skill_dir).expect("create skill directory"); + let skill_path = skill_dir.join("SKILL.md"); + fs::write( + &skill_path, + format!("---\nname: {name}\ndescription: {name} description\n---\n"), + ) + .expect("write skill"); + AbsolutePathBuf::from_absolute_path( + dunce::canonicalize(skill_path).expect("canonical skill path"), + ) + .expect("absolute skill path") +} + +fn expected_skill(path: AbsolutePathBuf, name: &str, scope: SkillScope) -> SkillMetadata { + SkillMetadata { + name: name.to_string(), + description: format!("{name} description"), + short_description: None, + interface: None, + dependencies: None, + policy: None, + path_to_skills_md: path, + scope, + plugin_id: None, + remote_plugin_id: None, + } +} + +#[test] +fn layer_roots_preserve_scope_precedence_and_disabled_projects() { + let temp_dir = TempDir::new().expect("temp dir"); + let system_folder = absolute(temp_dir.path().join("etc/codex")); + let home_folder = absolute(temp_dir.path().join("home")); + let user_folder = home_folder.join("codex"); + let project_folder = absolute(temp_dir.path().join("repo/.codex")); + let nested_project_folder = absolute(temp_dir.path().join("repo/nested/.codex")); + let config_stack = stack(vec![ + ConfigLayerEntry::new( + ConfigLayerSource::System { + file: system_folder.join("config.toml"), + }, + empty_config(), + ), + user_layer(&user_folder), + ConfigLayerEntry::new_disabled( + ConfigLayerSource::Project { + dot_codex_folder: project_folder.clone(), + }, + empty_config(), + "untrusted project", + ), + project_layer(&nested_project_folder), + ]); + + let roots = roots_from_layer_stack( + &config_stack, + Some(&home_folder), + Some(Arc::clone(&LOCAL_FS)), + ) + .into_iter() + .map(|root| (root.scope, root.path)) + .collect::>(); + + assert_eq!( + roots, + vec![ + (SkillScope::Repo, nested_project_folder.join("skills")), + (SkillScope::Repo, project_folder.join("skills")), + (SkillScope::User, user_folder.join("skills")), + (SkillScope::User, home_folder.join(".agents/skills")), + (SkillScope::System, user_folder.join("skills/.system")), + (SkillScope::Admin, system_folder.join("skills")), + ] + ); +} + +#[tokio::test] +async fn plugin_roots_preserve_plugin_resolution_metadata() { + let temp_dir = TempDir::new().expect("temp dir"); + let cwd = absolute(temp_dir.path().join("workspace")); + let plugin_root = absolute(temp_dir.path().join("plugins/example")); + let skills_root = plugin_root.join("skills"); + let plugin_identity = PluginIdentity { + plugin_id: "example@test".to_string(), + remote_plugin_id: Some("plugins~Plugin_example".to_string()), + }; + let plugin_namespace = "example".to_string(); + + let roots = resolve_skill_roots_with_home_dir( + /*repository_file_system*/ None, + &stack(Vec::new()), + &cwd, + /*home_dir*/ None, + vec![PluginSkillRoot { + path: skills_root.clone(), + plugin_identity: plugin_identity.clone(), + plugin_namespace: plugin_namespace.clone(), + plugin_root: plugin_root.clone(), + discovery_mode: SkillDiscoveryMode::DirectChildren, + }], + Vec::new(), + ) + .await; + + assert_eq!(roots.len(), 1); + let root = &roots[0]; + assert_eq!( + ( + root.path.clone(), + root.scope, + root.plugin_identity.clone(), + root.plugin_namespace.clone(), + root.plugin_root.clone(), + root.discovery_mode, + ), + ( + skills_root, + SkillScope::User, + Some(plugin_identity), + Some(plugin_namespace), + Some(plugin_root), + SkillDiscoveryMode::DirectChildren, + ) + ); + assert!(Arc::ptr_eq(&root.file_system, &LOCAL_FS)); +} + +#[tokio::test] +async fn unique_extra_root_loads_as_recursive_user_root() { + let temp_dir = TempDir::new().expect("temp dir"); + let cwd = absolute(temp_dir.path().join("workspace")); + let extra_root = absolute(temp_dir.path().join("runtime-skills")); + let skill_path = write_skill(&extra_root, "runtime", "runtime-skill"); + + let roots = resolve_skill_roots_with_home_dir( + /*repository_file_system*/ None, + &stack(Vec::new()), + &cwd, + /*home_dir*/ None, + Vec::new(), + vec![extra_root.clone()], + ) + .await; + + assert_eq!(roots.len(), 1); + let root = &roots[0]; + assert_eq!( + ( + root.path.clone(), + root.scope, + root.plugin_identity.clone(), + root.plugin_namespace.clone(), + root.plugin_root.clone(), + root.discovery_mode, + ), + ( + extra_root, + SkillScope::User, + None, + None, + None, + SkillDiscoveryMode::Recursive, + ) + ); + assert!(Arc::ptr_eq(&root.file_system, &LOCAL_FS)); + + let outcome = load_skills_from_roots( + roots, + /*plugin_skill_snapshots*/ None, + Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), + ) + .await; + + assert!(outcome.errors.is_empty()); + assert_eq!( + outcome.skills, + vec![expected_skill( + skill_path, + "runtime-skill", + SkillScope::User, + )] + ); +} + +#[tokio::test] +async fn repo_ancestry_without_project_marker_does_not_walk_parents() { + let temp_dir = TempDir::new().expect("temp dir"); + let outer = absolute(temp_dir.path().join("outer")); + let cwd = outer.join("nested/inner"); + fs::create_dir_all(outer.join(".agents/skills")).expect("create outer skills"); + fs::create_dir_all(cwd.join(".agents/skills")).expect("create cwd skills"); + + let roots = repo_agents_skill_roots(Some(Arc::clone(&LOCAL_FS)), &stack(Vec::new()), &cwd) + .await + .into_iter() + .map(|root| root.path) + .collect::>(); + + assert_eq!(roots, vec![cwd.join(".agents/skills")]); +} + +#[tokio::test] +async fn repo_ancestry_stops_at_project_root_and_preserves_root_to_cwd_order() { + let temp_dir = TempDir::new().expect("temp dir"); + let outer = absolute(temp_dir.path().join("outer")); + let repository = outer.join("repo"); + let nested = repository.join("nested/inner"); + fs::create_dir_all(&nested).expect("create nested cwd"); + fs::write(repository.join(".git"), "gitdir: fake\n").expect("write git marker"); + fs::create_dir_all(outer.join(".agents/skills")).expect("create outer skills"); + fs::create_dir_all(repository.join(".agents/skills")).expect("create repo skills"); + fs::create_dir_all(repository.join("nested/.agents/skills")).expect("create nested skills"); + let config_stack = stack(Vec::new()); + + let roots = repo_agents_skill_roots(Some(Arc::clone(&LOCAL_FS)), &config_stack, &nested) + .await + .into_iter() + .map(|root| root.path) + .collect::>(); + + assert_eq!( + roots, + vec![ + repository.join(".agents/skills"), + repository.join("nested/.agents/skills"), + ] + ); +} + +#[tokio::test] +async fn resolved_project_layer_loads_skill_without_git_marker() { + let temp_dir = TempDir::new().expect("temp dir"); + let workspace = absolute(temp_dir.path().join("workspace")); + let dot_codex = workspace.join(".codex"); + let skill_root = dot_codex.join("skills"); + fs::create_dir_all(&workspace).expect("create workspace"); + let skill_path = write_skill(&skill_root, "local", "local-skill"); + let config_stack = stack(vec![project_layer(&dot_codex)]); + + let roots = resolve_skill_roots_with_home_dir( + Some(Arc::clone(&LOCAL_FS)), + &config_stack, + &workspace, + /*home_dir*/ None, + Vec::new(), + Vec::new(), + ) + .await; + let outcome = load_skills_from_roots( + roots, + /*plugin_skill_snapshots*/ None, + Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), + ) + .await; + + assert!(outcome.errors.is_empty()); + assert_eq!( + outcome.skills, + vec![expected_skill(skill_path, "local-skill", SkillScope::Repo)] + ); +} + +#[tokio::test] +async fn resolved_project_layer_loads_skill_when_cwd_is_file() { + let temp_dir = TempDir::new().expect("temp dir"); + let repository = absolute(temp_dir.path().join("repo")); + let dot_codex = repository.join(".codex"); + let skill_root = dot_codex.join("skills"); + fs::create_dir_all(&repository).expect("create repository"); + fs::write(repository.join(".git"), "gitdir: fake\n").expect("write git marker"); + let cwd = repository.join("some-file.txt"); + fs::write(&cwd, "contents").expect("write cwd file"); + let skill_path = write_skill(&skill_root, "repo", "repo-skill"); + let config_stack = stack(vec![project_layer(&dot_codex)]); + + let roots = resolve_skill_roots_with_home_dir( + Some(Arc::clone(&LOCAL_FS)), + &config_stack, + &cwd, + /*home_dir*/ None, + Vec::new(), + Vec::new(), + ) + .await; + let outcome = load_skills_from_roots( + roots, + /*plugin_skill_snapshots*/ None, + Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), + ) + .await; + + assert!(outcome.errors.is_empty()); + assert_eq!( + outcome.skills, + vec![expected_skill(skill_path, "repo-skill", SkillScope::Repo)] + ); +} + +#[tokio::test] +async fn repo_ancestry_limits_concurrent_probes_and_preserves_order() { + const CONCURRENCY_LIMIT: usize = 256; + + let temp_dir = TempDir::new().expect("temp dir"); + let repository = absolute(temp_dir.path().join("repo")); + fs::create_dir_all(&repository).expect("create repository"); + fs::write(repository.join(".git"), "gitdir: fake\n").expect("write git marker"); + + let mut directories = vec![repository.clone()]; + let mut cwd = repository; + for _ in 0..CONCURRENCY_LIMIT { + cwd = cwd.join("d"); + directories.push(cwd.clone()); + } + fs::create_dir_all(&cwd).expect("create nested cwd"); + + let expected_roots = [0, CONCURRENCY_LIMIT / 2, CONCURRENCY_LIMIT].map(|index| { + let path = directories[index].join(".agents/skills"); + fs::create_dir_all(&path).expect("create repo skill root"); + path + }); + let expected_probes = directories + .iter() + .map(|directory| PathUri::from_abs_path(&directory.join(".agents/skills"))) + .collect::>(); + let calls = Arc::new(BlockingMetadataCalls::default()); + let file_system: Arc = Arc::new(BlockingMetadataFileSystem { + inner: Arc::clone(&LOCAL_FS), + calls: Arc::clone(&calls), + }); + + let assertions = async { + tokio::time::timeout(std::time::Duration::from_secs(/*secs*/ 5), async { + loop { + let started = calls.started.notified(); + if calls.paths.lock().expect("metadata paths lock").len() >= CONCURRENCY_LIMIT { + break; + } + started.await; + } + }) + .await + .expect("initial repo skill root window should start"); + assert_eq!( + calls.paths.lock().expect("metadata paths lock").as_slice(), + &expected_probes[..CONCURRENCY_LIMIT] + ); + + calls.release.add_permits(/*n*/ 1); + tokio::time::timeout(std::time::Duration::from_secs(/*secs*/ 5), async { + loop { + let started = calls.started.notified(); + if calls.paths.lock().expect("metadata paths lock").len() > CONCURRENCY_LIMIT { + break; + } + started.await; + } + }) + .await + .expect("next repo skill root probe should start"); + assert_eq!( + calls.paths.lock().expect("metadata paths lock").as_slice(), + expected_probes.as_slice() + ); + + calls.release.add_permits(expected_probes.len()); + }; + let config_stack = stack(Vec::new()); + let (roots, ()) = tokio::join!( + repo_agents_skill_roots(Some(file_system), &config_stack, &cwd), + assertions, + ); + + assert_eq!( + roots.into_iter().map(|root| root.path).collect::>(), + expected_roots + ); +} + +#[tokio::test] +async fn resolved_config_and_repo_roots_preserve_order_and_dedupe_paths_not_names() { + let temp_dir = TempDir::new().expect("temp dir"); + let home_folder = absolute(temp_dir.path().join("home")); + let codex_home = home_folder.join("codex"); + let system_folder = absolute(temp_dir.path().join("etc/codex")); + let repository = absolute(temp_dir.path().join("repo")); + let cwd = repository.join("nested/inner"); + fs::create_dir_all(&cwd).expect("create cwd"); + fs::write(repository.join(".git"), "gitdir: fake\n").expect("write git marker"); + + let project_dot_codex = repository.join(".codex"); + let nested_project_dot_codex = repository.join("nested/.codex"); + let user_skills = codex_home.join("skills"); + let root_project_skill = write_skill( + &project_dot_codex.join("skills"), + "root-duplicate", + "duplicate-skill", + ); + let nested_project_skill = write_skill( + &nested_project_dot_codex.join("skills"), + "nested-duplicate", + "duplicate-skill", + ); + let user_skill = write_skill(&user_skills, "user-duplicate", "duplicate-skill"); + let home_skill = write_skill(&home_folder.join(".agents/skills"), "home", "home-skill"); + let system_skill = write_skill(&codex_home.join("skills/.system"), "system", "system-skill"); + let admin_skill = write_skill(&system_folder.join("skills"), "admin", "admin-skill"); + let repo_agent_skill = write_skill( + &repository.join(".agents/skills"), + "repo-agent", + "repo-agent-skill", + ); + let nested_agent_skill = write_skill( + &repository.join("nested/.agents/skills"), + "nested-agent", + "nested-agent-skill", + ); + let config_stack = stack(vec![ + ConfigLayerEntry::new( + ConfigLayerSource::System { + file: system_folder.join("config.toml"), + }, + empty_config(), + ), + user_layer(&codex_home), + project_layer(&project_dot_codex), + project_layer(&nested_project_dot_codex), + ]); + + let roots = resolve_skill_roots_with_home_dir( + Some(Arc::clone(&LOCAL_FS)), + &config_stack, + &cwd, + Some(&home_folder), + Vec::new(), + vec![user_skills], + ) + .await; + assert_eq!(roots.len(), 8); + let outcome = load_skills_from_roots( + roots, + /*plugin_skill_snapshots*/ None, + Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), + ) + .await; + assert!(outcome.errors.is_empty()); + assert_eq!( + outcome.skills, + vec![ + expected_skill(root_project_skill, "duplicate-skill", SkillScope::Repo), + expected_skill(nested_project_skill, "duplicate-skill", SkillScope::Repo), + expected_skill(nested_agent_skill, "nested-agent-skill", SkillScope::Repo), + expected_skill(repo_agent_skill, "repo-agent-skill", SkillScope::Repo), + expected_skill(user_skill, "duplicate-skill", SkillScope::User), + expected_skill(home_skill, "home-skill", SkillScope::User), + expected_skill(system_skill, "system-skill", SkillScope::System), + expected_skill(admin_skill, "admin-skill", SkillScope::Admin), + ] + ); +} diff --git a/codex-rs/ext/skills/src/host_service.rs b/codex-rs/ext/skills/src/host_service.rs index 6edb174d2a..8bc487eaaa 100644 --- a/codex-rs/ext/skills/src/host_service.rs +++ b/codex-rs/ext/skills/src/host_service.rs @@ -24,10 +24,11 @@ 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_core_skills::loader::load_skills_from_roots; -use codex_core_skills::loader::skill_roots; use codex_skills::install_system_skills; use codex_skills::system_cache_root_dir; +use crate::host_roots::resolve_skill_roots; + #[derive(Debug, Clone)] pub struct HostSkillsLoadInput { pub cwd: AbsolutePathBuf, @@ -156,7 +157,7 @@ impl HostSkillsService { input: &HostSkillsLoadInput, fs: Option>, ) -> Vec { - let mut roots = skill_roots( + let mut roots = resolve_skill_roots( fs, &input.config_layer_stack, &input.cwd, @@ -184,7 +185,7 @@ impl HostSkillsService { return snapshot; } - let mut roots = skill_roots( + let mut roots = resolve_skill_roots( fs.clone(), &input.config_layer_stack, &input.cwd, diff --git a/codex-rs/ext/skills/src/lib.rs b/codex-rs/ext/skills/src/lib.rs index 2901d2b1a7..df740b4f9b 100644 --- a/codex-rs/ext/skills/src/lib.rs +++ b/codex-rs/ext/skills/src/lib.rs @@ -4,6 +4,7 @@ mod config; mod dynamic_skill_selector; mod extension; mod fragments; +mod host_roots; mod host_service; // Host loading is staged before its crate-internal runtime caller in this PR stack. #[allow(dead_code)] diff --git a/codex-rs/ext/skills/src/loader/mod.rs b/codex-rs/ext/skills/src/loader/mod.rs index 148fbffd96..aa8fb07adb 100644 --- a/codex-rs/ext/skills/src/loader/mod.rs +++ b/codex-rs/ext/skills/src/loader/mod.rs @@ -6,6 +6,7 @@ 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(super) const SKILLS_FILENAME: &str = "SKILL.md"; pub(super) const SKILLS_METADATA_DIR: &str = "agents";