diff --git a/codex-rs/config/src/skills_config.rs b/codex-rs/config/src/skills_config.rs index 9a8a338fe1e0..1ffe63a59c01 100644 --- a/codex-rs/config/src/skills_config.rs +++ b/codex-rs/config/src/skills_config.rs @@ -53,6 +53,16 @@ pub struct SkillsConfig { #[serde(default, skip_serializing_if = "Vec::is_empty")] pub config: Vec, + + /// Fork (WI-1133): extra user skill roots, relative to the user config + /// directory. The built-in roots are always loaded; these are added. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub extra_user_roots: Vec, + + /// Fork (WI-1133): extra project skill roots, relative to each directory + /// between the project root and the working directory. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub extra_project_roots: Vec, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)] diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 5864a5df77c6..df2c53462c42 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -4269,6 +4269,20 @@ }, "type": "array" }, + "extra_project_roots": { + "description": "Fork (WI-1133): extra project skill roots, relative to each directory between the project root and the working directory.", + "items": { + "type": "string" + }, + "type": "array" + }, + "extra_user_roots": { + "description": "Fork (WI-1133): extra user skill roots, relative to the user config directory. The built-in roots are always loaded; these are added.", + "items": { + "type": "string" + }, + "type": "array" + }, "include_instructions": { "description": "Whether turns receive the automatic skills instructions block.", "type": "boolean" diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 806bd8082ff8..1dc4a353d6ae 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -439,6 +439,8 @@ enabled = false include_instructions: Some(false), max_context_tokens: std::num::NonZeroUsize::new(1_200), config: Vec::new(), + extra_user_roots: Vec::new(), + extra_project_roots: Vec::new(), }) ); diff --git a/codex-rs/ext/skills/src/host_roots.rs b/codex-rs/ext/skills/src/host_roots.rs index ac2bf3e9cdf8..fb960d0f164b 100644 --- a/codex-rs/ext/skills/src/host_roots.rs +++ b/codex-rs/ext/skills/src/host_roots.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use codex_config::ConfigLayerSource; use codex_config::ConfigLayerStack; +use codex_config::SkillsConfig; use codex_config::default_project_root_markers; use codex_config::merge_toml_values; use codex_config::project_root_markers_from_config; @@ -23,6 +24,40 @@ use crate::loader::HostSkillRoot; const AGENTS_DIR_NAME: &str = ".agents"; const SKILLS_DIR_NAME: &str = "skills"; + +/// The extra roots configured under `[skills]`, or none when the config is +/// absent or invalid. An invalid table warns and changes nothing. +fn extra_skill_roots(config_layer_stack: &ConfigLayerStack, project: bool) -> Vec { + let effective = config_layer_stack.effective_config(); + let Some(value) = effective.as_table().and_then(|table| table.get("skills")) else { + return Vec::new(); + }; + let skills: SkillsConfig = match value.clone().try_into() { + Ok(skills) => skills, + Err(err) => { + tracing::warn!("invalid skills config: {err}"); + return Vec::new(); + } + }; + if project { + skills.extra_project_roots + } else { + skills.extra_user_roots + } +} + +/// Joins `relative` onto `base` and keeps it only when it stays inside `base`. +/// +/// An absolute value, a `..` climb or anything else that resolves outside the +/// base is dropped, so a config cannot point skill loading at arbitrary paths. +fn confined_join(base: &AbsolutePathBuf, relative: &str) -> Option { + let relative = relative.trim().trim_start_matches(['/', '\\']); + if relative.is_empty() { + return None; + } + let joined = base.join(relative); + joined.starts_with(base).then_some(joined) +} const MAX_CONCURRENT_ANCESTOR_PROBES: usize = 256; pub(crate) async fn resolve_skill_roots( @@ -99,6 +134,11 @@ fn roots_from_layer_stack( config_folder.join(SKILLS_DIR_NAME), SkillScope::User, )); + for relative in extra_skill_roots(config_layer_stack, /*project*/ false) { + if let Some(path) = confined_join(&config_folder, &relative) { + roots.push(local_root(path, SkillScope::User)); + } + } if let Some(home_dir) = home_dir { roots.push(local_root( @@ -181,6 +221,25 @@ async fn repo_agents_skill_roots( } } } + for directory in dirs_between_project_root_and_cwd(cwd, &project_root) { + for relative in extra_skill_roots(config_layer_stack, /*project*/ true) { + let Some(path) = confined_join(&directory, &relative) else { + continue; + }; + let uri = PathUri::from_abs_path(&path); + if let Ok(metadata) = repository_file_system + .get_metadata(&uri, GetMetadataOptions::default(), /*sandbox*/ None) + .await + && metadata.is_directory + { + roots.push(HostSkillRoot::host( + path, + SkillScope::Repo, + Arc::clone(&repository_file_system), + )); + } + } + } roots } diff --git a/codex-rs/ext/skills/src/host_roots_tests.rs b/codex-rs/ext/skills/src/host_roots_tests.rs index fa8cb7d8dc2a..4584f15687d1 100644 --- a/codex-rs/ext/skills/src/host_roots_tests.rs +++ b/codex-rs/ext/skills/src/host_roots_tests.rs @@ -406,7 +406,22 @@ async fn repo_ancestry_without_project_marker_does_not_walk_parents() { 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) + // Hermetic: disable project-root marker discovery so the test does not + // depend on ancestor directories of the temp dir (e.g. a real .git). + let mut user_config = empty_config(); + user_config.as_table_mut().expect("config table").insert( + "project_root_markers".into(), + toml::Value::Array(Vec::new()), + ); + let config_stack = stack(vec![ConfigLayerEntry::new( + ConfigLayerSource::User { + file: absolute(temp_dir.path().join("home/codex/config.toml")), + profile: None, + }, + user_config, + )]); + + let roots = repo_agents_skill_roots(Some(Arc::clone(&LOCAL_FS)), &config_stack, &cwd) .await .into_iter() .map(|root| root.path) @@ -676,3 +691,72 @@ async fn resolved_config_and_repo_roots_preserve_order_and_dedupe_paths_not_name ] ); } + +/// Fork (WI-1133): `[skills].extra_project_roots` adds a root without dropping +/// `.agents/skills`, and a value that climbs out of the directory is ignored. +#[tokio::test] +async fn extra_project_roots_are_added_and_confined() { + let temp_dir = TempDir::new().expect("temp dir"); + let repository = absolute(temp_dir.path().join("repo")); + fs::create_dir_all(&repository).expect("create repo"); + fs::write(repository.join(".git"), "gitdir: fake\n").expect("write git marker"); + fs::create_dir_all(repository.join(".agents/skills")).expect("built-in root"); + fs::create_dir_all(repository.join(".claude/skills")).expect("extra root"); + + let config = toml::toml! { + [skills] + extra_project_roots = [".claude/skills", "../outside"] + }; + let config_stack = stack(vec![ConfigLayerEntry::new( + ConfigLayerSource::User { + file: repository.join("config.toml"), + profile: None, + }, + toml::Value::Table(config), + )]); + + let roots = repo_agents_skill_roots(Some(Arc::clone(&LOCAL_FS)), &config_stack, &repository) + .await + .into_iter() + .map(|root| root.path) + .collect::>(); + + assert!(roots.contains(&repository.join(".agents/skills"))); + assert!(roots.contains(&repository.join(".claude/skills"))); + assert!( + roots.iter().all(|root| root.starts_with(&repository)), + "a root escaped the project: {roots:?}" + ); +} + +/// Fork (WI-1133): `[skills].extra_user_roots` is added beside the built-in +/// user roots, and a value that leaves the config directory is dropped. +#[test] +fn extra_user_roots_are_added_and_confined() { + let temp_dir = TempDir::new().expect("temp dir"); + let home_folder = absolute(temp_dir.path().join("home")); + let user_folder = home_folder.join("codex"); + let config = toml::toml! { + [skills] + extra_user_roots = [".claude/skills", "../escaped"] + }; + let config_stack = stack(vec![ConfigLayerEntry::new( + ConfigLayerSource::User { + file: user_folder.join("config.toml"), + profile: None, + }, + toml::Value::Table(config), + )]); + + let roots = roots_from_layer_stack(&config_stack, Some(&home_folder), None) + .into_iter() + .map(|root| root.path) + .collect::>(); + + assert!(roots.contains(&user_folder.join("skills"))); + assert!(roots.contains(&user_folder.join(".claude/skills"))); + assert!( + !roots.iter().any(|root| root.ends_with("escaped")), + "a root escaped the config directory: {roots:?}" + ); +}