From efb09e1db9dcd83a2b1d2ee0259224d257f1c8bf Mon Sep 17 00:00:00 2001 From: thedancingdeveloper <306930456+thedancingdeveloper@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:25:27 +0000 Subject: [PATCH 1/4] Add configurable extra skill roots under [skills] extra_user_roots and extra_project_roots add skill directories beside the built-in ones, which are unchanged when the keys are absent. Each value is relative to its base directory and dropped when it resolves outside it, so the config cannot point skill loading at an arbitrary path. WI-1133 --- codex-rs/config/src/skills_config.rs | 10 +++ codex-rs/ext/skills/src/host_roots.rs | 59 ++++++++++++++++++ codex-rs/ext/skills/src/host_roots_tests.rs | 69 +++++++++++++++++++++ 3 files changed, 138 insertions(+) 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/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..021484106f5c 100644 --- a/codex-rs/ext/skills/src/host_roots_tests.rs +++ b/codex-rs/ext/skills/src/host_roots_tests.rs @@ -676,3 +676,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:?}" + ); +} From b8324fa7152d6c8ffbd34a6efb06686c88414319 Mon Sep 17 00:00:00 2001 From: thedancingdeveloper <306930456+thedancingdeveloper@users.noreply.github.com> Date: Fri, 9 Oct 2026 17:12:43 +0000 Subject: [PATCH 2/4] Fill the new skill root fields in the config test SkillsConfig gained extra_user_roots and extra_project_roots, and the bundled-skills assertion builds one by hand. Both default to empty. WI-1133 --- codex-rs/core/src/config/config_tests.rs | 2 ++ 1 file changed, 2 insertions(+) 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(), }) ); From a2d734c8834495dc6e9bb4ec1345bfd1d52bd1db Mon Sep 17 00:00:00 2001 From: thedancingdeveloper <306930456+thedancingdeveloper@users.noreply.github.com> Date: Fri, 9 Oct 2026 19:54:52 +0000 Subject: [PATCH 3/4] Regenerate the config schema for the extra skill roots extra_user_roots and extra_project_roots are part of SkillsConfig, so the committed config.schema.json fixture has to include them. WI-1133 --- codex-rs/core/config.schema.json | 14 ++++++++++++++ 1 file changed, 14 insertions(+) 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" From f8ce9331439e293176614692c49c46b211b0a77b Mon Sep 17 00:00:00 2001 From: thedancingdeveloper <306930456+thedancingdeveloper@users.noreply.github.com> Date: Fri, 9 Oct 2026 23:06:16 +0000 Subject: [PATCH 4/4] Fix hermeticity of repo ancestry test without project markers Set project_root_markers = [] via a User config layer so the test does not depend on ancestor directories of the temp dir (e.g. a real .git). --- codex-rs/ext/skills/src/host_roots_tests.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/codex-rs/ext/skills/src/host_roots_tests.rs b/codex-rs/ext/skills/src/host_roots_tests.rs index 021484106f5c..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)