From 8e20d8ea5f84be17312936fc072080778e202fcf Mon Sep 17 00:00:00 2001 From: starr-openai Date: Thu, 28 May 2026 16:27:09 -0700 Subject: [PATCH] Use environment path refs for skill roots --- codex-rs/core-plugins/src/loader.rs | 3 +- codex-rs/core-skills/src/lib.rs | 1 - codex-rs/core-skills/src/loader.rs | 40 ++++++++---------------- codex-rs/core-skills/src/loader_tests.rs | 28 ++++++++--------- codex-rs/core-skills/src/manager.rs | 2 +- codex-rs/core-skills/src/model.rs | 24 -------------- 6 files changed, 29 insertions(+), 69 deletions(-) diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 8145c97274..d24cbf0dcf 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -14,7 +14,6 @@ use codex_config::types::McpServerConfig; use codex_config::types::PluginConfig; use codex_config::types::PluginMcpServerConfig; use codex_core_skills::SkillMetadata; -use codex_core_skills::SkillRootPathRef; use codex_core_skills::config_rules::SkillConfigRules; use codex_core_skills::config_rules::resolve_disabled_skill_paths; use codex_core_skills::config_rules::skill_config_rules_from_stack; @@ -661,7 +660,7 @@ pub async fn load_plugin_skills( let roots = plugin_skill_roots(plugin_root.path(), manifest_paths) .into_iter() .map(|path| SkillRoot { - path: SkillRootPathRef::new(plugin_root.with_path(path)), + path: plugin_root.with_path(path), scope: SkillScope::User, plugin_id: Some(plugin_id.as_key()), plugin_root: Some(plugin_root.clone()), diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs index 928d8ebc0d..1577662bc5 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -18,7 +18,6 @@ pub use model::SkillError; pub use model::SkillLoadOutcome; pub use model::SkillMetadata; pub use model::SkillPolicy; -pub use model::SkillRootPathRef; pub use model::filter_skill_load_outcome_for_product; pub use render::AvailableSkills; pub use render::SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS; diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 0b10162afb..74301692b9 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -5,7 +5,6 @@ use crate::model::SkillInterface; use crate::model::SkillLoadOutcome; use crate::model::SkillMetadata; use crate::model::SkillPolicy; -use crate::model::SkillRootPathRef; use crate::model::SkillToolDependency; use crate::system::system_cache_root_dir; use codex_app_server_protocol::ConfigLayerSource; @@ -152,7 +151,7 @@ impl fmt::Display for SkillParseError { impl Error for SkillParseError {} pub struct SkillRoot { - pub path: SkillRootPathRef, + pub path: EnvironmentPathRef, pub scope: SkillScope, pub plugin_id: Option, pub plugin_root: Option, @@ -169,8 +168,7 @@ where HashMap::new(); for root in roots { let root_path = canonicalize_for_skill_identity(root.path.path()); - let root_path_ref = - SkillRootPathRef::new(root.path.path_ref().with_path(root_path.clone())); + let root_path_ref = root.path.with_path(root_path.clone()); let fs = root_path_ref.file_system(); let skills_before_root = outcome.skills.len(); discover_skills_under_root( @@ -264,7 +262,7 @@ async fn skill_roots_with_home_dir( ); if let Some(skill_root_path_ref) = skill_root_path_ref { roots.extend(plugin_skill_roots.into_iter().map(|root| SkillRoot { - path: SkillRootPathRef::new(skill_root_path_ref.with_path(root.path)), + path: skill_root_path_ref.with_path(root.path), scope: SkillScope::User, plugin_id: Some(root.plugin_id), plugin_root: Some(skill_root_path_ref.with_path(root.plugin_root)), @@ -295,9 +293,7 @@ fn skill_roots_from_layer_stack_inner( ConfigLayerSource::Project { .. } => { if let Some(env_path_ref) = env_path_ref { roots.push(SkillRoot { - path: SkillRootPathRef::new( - env_path_ref.with_path(config_folder.join(SKILLS_DIR_NAME)), - ), + path: env_path_ref.with_path(config_folder.join(SKILLS_DIR_NAME)), scope: SkillScope::Repo, plugin_id: None, plugin_root: None, @@ -311,9 +307,7 @@ fn skill_roots_from_layer_stack_inner( // Deprecated user skills location (`$CODEX_HOME/skills`), kept for backward // compatibility. roots.push(SkillRoot { - path: SkillRootPathRef::new( - skill_root_path_ref.with_path(config_folder.join(SKILLS_DIR_NAME)), - ), + path: skill_root_path_ref.with_path(config_folder.join(SKILLS_DIR_NAME)), scope: SkillScope::User, plugin_id: None, plugin_root: None, @@ -322,10 +316,8 @@ fn skill_roots_from_layer_stack_inner( // `$HOME/.agents/skills` (user-installed skills). if let Some(home_dir) = home_dir { roots.push(SkillRoot { - path: SkillRootPathRef::new( - skill_root_path_ref - .with_path(home_dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME)), - ), + path: skill_root_path_ref + .with_path(home_dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME)), scope: SkillScope::User, plugin_id: None, plugin_root: None, @@ -335,9 +327,7 @@ fn skill_roots_from_layer_stack_inner( // Embedded system skills are cached under `$CODEX_HOME/skills/.system` and are a // special case (not a config layer). roots.push(SkillRoot { - path: SkillRootPathRef::new( - skill_root_path_ref.with_path(system_cache_root_dir(&config_folder)), - ), + path: skill_root_path_ref.with_path(system_cache_root_dir(&config_folder)), scope: SkillScope::System, plugin_id: None, plugin_root: None, @@ -350,9 +340,7 @@ fn skill_roots_from_layer_stack_inner( // The system config layer lives under `/etc/codex/` on Unix, so treat // `/etc/codex/skills` as admin-scoped skills. roots.push(SkillRoot { - path: SkillRootPathRef::new( - skill_root_path_ref.with_path(config_folder.join(SKILLS_DIR_NAME)), - ), + path: skill_root_path_ref.with_path(config_folder.join(SKILLS_DIR_NAME)), scope: SkillScope::Admin, plugin_id: None, plugin_root: None, @@ -383,7 +371,7 @@ async fn repo_agents_skill_roots( let agents_skills = env_path_ref.with_path(dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME)); match agents_skills.metadata().await { Ok(metadata) if metadata.is_directory => roots.push(SkillRoot { - path: SkillRootPathRef::new(agents_skills), + path: agents_skills, scope: SkillScope::Repo, plugin_id: None, plugin_root: None, @@ -473,7 +461,7 @@ fn dirs_between_project_root_and_cwd( fn dedupe_skill_roots_by_path(roots: &mut Vec) { let mut seen: HashSet = HashSet::new(); - roots.retain(|root| seen.insert(root.path.path_ref().clone())); + roots.retain(|root| seen.insert(root.path.clone())); } fn canonicalize_for_skill_identity(path: &AbsolutePathBuf) -> AbsolutePathBuf { @@ -481,15 +469,13 @@ fn canonicalize_for_skill_identity(path: &AbsolutePathBuf) -> AbsolutePathBuf { } async fn discover_skills_under_root( - root: &SkillRootPathRef, + root: &EnvironmentPathRef, scope: SkillScope, plugin_id: Option<&str>, plugin_root: Option<&EnvironmentPathRef>, outcome: &mut SkillLoadOutcome, ) { - let root = root - .path_ref() - .with_path(canonicalize_for_skill_identity(root.path())); + let root = root.with_path(canonicalize_for_skill_identity(root.path())); let plugin_root = plugin_root.map(|plugin_root| { plugin_root.with_path(canonicalize_for_skill_identity(plugin_root.path())) }); diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index 72253c84c1..4f341a4b0c 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -145,7 +145,7 @@ fn normalized(path: &Path) -> AbsolutePathBuf { .abs() } -fn skill_root_path_ref(path: AbsolutePathBuf) -> EnvironmentPathRef { +fn env_path_ref(path: AbsolutePathBuf) -> EnvironmentPathRef { EnvironmentPathRef::new( LOCAL_ENVIRONMENT_ID.to_string(), Arc::clone(&LOCAL_FS), @@ -155,7 +155,7 @@ fn skill_root_path_ref(path: AbsolutePathBuf) -> EnvironmentPathRef { fn local_skill_root(path: AbsolutePathBuf, scope: SkillScope) -> SkillRoot { SkillRoot { - path: SkillRootPathRef::new(skill_root_path_ref(path)), + path: env_path_ref(path), scope, plugin_id: None, plugin_root: None, @@ -864,10 +864,10 @@ interface: let plugin_root_abs = plugin_root.abs(); let outcome = load_skills_from_roots([SkillRoot { - path: SkillRootPathRef::new(skill_root_path_ref(plugin_root.join("skills").abs())), + path: env_path_ref(plugin_root.join("skills").abs()), scope: SkillScope::User, plugin_id: Some("twilio-developer-kit@test".to_string()), - plugin_root: Some(skill_root_path_ref(plugin_root_abs.clone())), + plugin_root: Some(env_path_ref(plugin_root_abs.clone())), }]) .await; @@ -920,10 +920,10 @@ interface: ); let outcome = load_skills_from_roots([SkillRoot { - path: SkillRootPathRef::new(skill_root_path_ref(plugin_root.join("skills").abs())), + path: env_path_ref(plugin_root.join("skills").abs()), scope: SkillScope::User, plugin_id: Some("twilio-developer-kit@test".to_string()), - plugin_root: Some(skill_root_path_ref(plugin_root.abs())), + plugin_root: Some(env_path_ref(plugin_root.abs())), }]) .await; @@ -1271,10 +1271,10 @@ async fn namespaces_plugin_skills_using_plugin_name() { .unwrap(); let outcome = load_skills_from_roots([SkillRoot { - path: SkillRootPathRef::new(skill_root_path_ref(plugin_root.join("skills").abs())), + path: env_path_ref(plugin_root.join("skills").abs()), scope: SkillScope::User, plugin_id: Some("sample@test".to_string()), - plugin_root: Some(skill_root_path_ref(plugin_root.abs())), + plugin_root: Some(env_path_ref(plugin_root.abs())), }]) .await; @@ -1593,13 +1593,13 @@ async fn deduplicates_by_path_preferring_first_root() { let outcome = load_skills_from_roots([ SkillRoot { - path: SkillRootPathRef::new(skill_root_path_ref(root.path().abs())), + path: env_path_ref(root.path().abs()), scope: SkillScope::Repo, plugin_id: None, plugin_root: None, }, SkillRoot { - path: SkillRootPathRef::new(skill_root_path_ref(root.path().abs())), + path: env_path_ref(root.path().abs()), scope: SkillScope::User, plugin_id: None, plugin_root: None, @@ -1891,7 +1891,7 @@ async fn skill_roots_include_admin_with_lowest_priority() { let codex_home = tempfile::tempdir().expect("tempdir"); let cfg = make_config(&codex_home).await; - let path_ref = skill_root_path_ref(cfg.cwd.clone()); + let path_ref = env_path_ref(cfg.cwd.clone()); let scopes: Vec = super::skill_roots( Some(&path_ref), Some(&path_ref), @@ -1911,13 +1911,13 @@ async fn skill_roots_include_admin_with_lowest_priority() { } #[tokio::test] -async fn skill_roots_skip_local_roots_without_skill_root_path_ref() { +async fn skill_roots_skip_local_roots_without_local_env_path_ref() { let codex_home = tempfile::tempdir().expect("tempdir"); let cfg = make_config(&codex_home).await; - let env_path_ref = skill_root_path_ref(cfg.cwd.clone()); + let local_env_path_ref = env_path_ref(cfg.cwd.clone()); let scopes: Vec = super::skill_roots( - Some(&env_path_ref), + Some(&local_env_path_ref), /*skill_root_path_ref*/ None, &cfg.config_layer_stack, Vec::new(), diff --git a/codex-rs/core-skills/src/manager.rs b/codex-rs/core-skills/src/manager.rs index bbd0d92a42..c11d911a35 100644 --- a/codex-rs/core-skills/src/manager.rs +++ b/codex-rs/core-skills/src/manager.rs @@ -253,7 +253,7 @@ fn config_skills_cache_key( SkillScope::Admin => 3, }; ( - root.path.path_ref().clone(), + root.path.clone(), scope_rank, root.plugin_id.clone(), ) diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index 724e5f0696..69d765969b 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -1,10 +1,7 @@ use std::collections::HashMap; use std::collections::HashSet; use std::fmt; -use std::sync::Arc; - use codex_exec_server::EnvironmentPathRef; -use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -47,27 +44,6 @@ impl SkillMetadata { } } -#[derive(Clone, Debug)] -pub struct SkillRootPathRef(EnvironmentPathRef); - -impl SkillRootPathRef { - pub fn new(path_ref: EnvironmentPathRef) -> Self { - Self(path_ref) - } - - pub fn path_ref(&self) -> &EnvironmentPathRef { - &self.0 - } - - pub fn path(&self) -> &AbsolutePathBuf { - self.0.path() - } - - pub(crate) fn file_system(&self) -> Arc { - self.0.file_system() - } -} - #[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct SkillPolicy { pub allow_implicit_invocation: Option,