From 5e4c73cd78ba9040863a88b2a2ff5ba240c3bb1e Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Wed, 6 May 2026 18:38:24 -0700 Subject: [PATCH] feat: enforce managed skill requirements Co-authored-by: Codex noreply@openai.com --- codex-rs/core-skills/src/manager.rs | 61 ++++++++- codex-rs/core-skills/src/manager_tests.rs | 146 +++++++++++++++++++++- 2 files changed, 200 insertions(+), 7 deletions(-) diff --git a/codex-rs/core-skills/src/manager.rs b/codex-rs/core-skills/src/manager.rs index 73b1f14807d8..c6e76c7a6e4d 100644 --- a/codex-rs/core-skills/src/manager.rs +++ b/codex-rs/core-skills/src/manager.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use std::sync::RwLock; use codex_config::ConfigLayerStack; +use codex_config::SkillSourceRequirement; use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; @@ -51,7 +52,7 @@ impl SkillsLoadInput { pub struct SkillsManager { codex_home: AbsolutePathBuf, restriction_product: Option, - cache_by_cwd: RwLock>, + cache_by_cwd: RwLock>, cache_by_config: RwLock>, } @@ -123,6 +124,7 @@ impl SkillsManager { if !input.bundled_skills_enabled { roots.retain(|root| root.scope != SkillScope::System); } + retain_allowed_skill_roots(&mut roots, &input.config_layer_stack); roots } @@ -144,9 +146,10 @@ impl SkillsManager { fs: Option>, ) -> SkillLoadOutcome { let use_cwd_cache = fs.is_some(); + let cache_key = cwd_skills_cache_key(&input.cwd, &input.config_layer_stack); if use_cwd_cache && !force_reload - && let Some(outcome) = self.cached_outcome_for_cwd(&input.cwd) + && let Some(outcome) = self.cached_outcome_for_cwd(&cache_key) { return outcome; } @@ -173,6 +176,7 @@ impl SkillsManager { }), ); } + retain_allowed_skill_roots(&mut roots, &input.config_layer_stack); let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); let outcome = self.build_skill_outcome(roots, &skill_config_rules).await; if use_cwd_cache { @@ -180,7 +184,7 @@ impl SkillsManager { .cache_by_cwd .write() .unwrap_or_else(std::sync::PoisonError::into_inner); - cache.insert(input.cwd.clone(), outcome.clone()); + cache.insert(cache_key, outcome.clone()); } outcome } @@ -221,10 +225,10 @@ impl SkillsManager { info!("skills cache cleared ({cleared} entries)"); } - fn cached_outcome_for_cwd(&self, cwd: &AbsolutePathBuf) -> Option { + fn cached_outcome_for_cwd(&self, cache_key: &CwdSkillsCacheKey) -> Option { match self.cache_by_cwd.read() { - Ok(cache) => cache.get(cwd).cloned(), - Err(err) => err.into_inner().get(cwd).cloned(), + Ok(cache) => cache.get(cache_key).cloned(), + Err(err) => err.into_inner().get(cache_key).cloned(), } } @@ -239,12 +243,43 @@ impl SkillsManager { } } +fn retain_allowed_skill_roots(roots: &mut Vec, config_layer_stack: &ConfigLayerStack) { + let Some(requirements) = config_layer_stack.requirements().skills.as_ref() else { + return; + }; + + roots.retain(|root| { + requirements + .value + .allows_source(skill_source_for_root(root)) + }); +} + +fn skill_source_for_root(root: &SkillRoot) -> SkillSourceRequirement { + if root.plugin_id.is_some() { + return SkillSourceRequirement::Plugin; + } + + match root.scope { + SkillScope::Repo => SkillSourceRequirement::Repo, + SkillScope::User => SkillSourceRequirement::User, + SkillScope::System => SkillSourceRequirement::System, + SkillScope::Admin => SkillSourceRequirement::Admin, + } +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] struct ConfigSkillsCacheKey { roots: Vec<(AbsolutePathBuf, u8, Option)>, skill_config_rules: SkillConfigRules, } +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +struct CwdSkillsCacheKey { + cwd: AbsolutePathBuf, + allowed_sources: Option>, +} + pub fn bundled_skills_enabled_from_stack( config_layer_stack: &codex_config::ConfigLayerStack, ) -> bool { @@ -288,6 +323,20 @@ fn config_skills_cache_key( } } +fn cwd_skills_cache_key( + cwd: &AbsolutePathBuf, + config_layer_stack: &ConfigLayerStack, +) -> CwdSkillsCacheKey { + CwdSkillsCacheKey { + cwd: cwd.clone(), + allowed_sources: config_layer_stack + .requirements() + .skills + .as_ref() + .and_then(|requirements| requirements.value.allowed_sources.clone()), + } +} + fn finalize_skill_outcome( mut outcome: SkillLoadOutcome, disabled_paths: HashSet, diff --git a/codex-rs/core-skills/src/manager_tests.rs b/codex-rs/core-skills/src/manager_tests.rs index 532d6951ecae..e4ef19011b0b 100644 --- a/codex-rs/core-skills/src/manager_tests.rs +++ b/codex-rs/core-skills/src/manager_tests.rs @@ -6,7 +6,12 @@ use codex_app_server_protocol::ConfigLayerSource; use codex_config::CONFIG_TOML_FILE; use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerStack; +use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; +use codex_config::RequirementSource; +use codex_config::SkillSourceRequirement; +use codex_config::SkillsRequirementsToml; +use codex_config::Sourced; use codex_exec_server::LOCAL_FS; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::test_support::PathBufExt; @@ -27,6 +32,13 @@ fn write_user_skill(codex_home: &TempDir, dir: &str, name: &str, description: &s fs::write(skill_dir.join("SKILL.md"), content).unwrap(); } +fn write_repo_skill(cwd: &TempDir, dir: &str, name: &str, description: &str) { + let skill_dir = cwd.path().join(".agents/skills").join(dir); + fs::create_dir_all(&skill_dir).unwrap(); + let content = format!("---\nname: {name}\ndescription: {description}\n---\n\n# Body\n"); + fs::write(skill_dir.join("SKILL.md"), content).unwrap(); +} + fn write_plugin_skill( codex_home: &TempDir, marketplace: &str, @@ -94,9 +106,17 @@ fn user_config_layer(codex_home: &TempDir, config_toml: &str) -> ConfigLayerEntr } fn config_stack(codex_home: &TempDir, user_config_toml: &str) -> ConfigLayerStack { + config_stack_with_requirements(codex_home, user_config_toml, ConfigRequirements::default()) +} + +fn config_stack_with_requirements( + codex_home: &TempDir, + user_config_toml: &str, + requirements: ConfigRequirements, +) -> ConfigLayerStack { ConfigLayerStack::new( vec![user_config_layer(codex_home, user_config_toml)], - Default::default(), + requirements, ConfigRequirementsToml::default(), ) .expect("valid config layer stack") @@ -262,6 +282,65 @@ async fn skills_for_config_disables_plugin_skills_by_name() { ); } +#[tokio::test] +async fn skills_for_config_filters_disallowed_managed_sources() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + write_user_skill(&codex_home, "user", "user-skill", "from user"); + write_repo_skill(&cwd, "repo", "repo-skill", "from repo"); + let plugin_skill_path = write_plugin_skill( + &codex_home, + "test", + "sample", + "plugin", + "plugin-skill", + "from plugin", + ); + let config_layer_stack = config_stack_with_requirements( + &codex_home, + "", + ConfigRequirements { + skills: Some(Sourced::new( + SkillsRequirementsToml { + allowed_sources: Some(vec![ + SkillSourceRequirement::System, + SkillSourceRequirement::Admin, + SkillSourceRequirement::Plugin, + ]), + }, + RequirementSource::Unknown, + )), + ..Default::default() + }, + ); + let skills_manager = SkillsManager::new( + codex_home.path().abs(), + /*bundled_skills_enabled*/ true, + ); + + let outcome = skills_for_config_with_stack( + &skills_manager, + &cwd, + &config_layer_stack, + &[plugin_skill_path + .parent() + .and_then(std::path::Path::parent) + .expect("plugin skills root") + .to_path_buf() + .abs()], + ) + .await; + let skill_names = outcome + .skills + .iter() + .map(|skill| skill.name.as_str()) + .collect::>(); + + assert!(skill_names.contains("sample:plugin-skill")); + assert!(!skill_names.contains("user-skill")); + assert!(!skill_names.contains("repo-skill")); +} + #[tokio::test] async fn skills_for_cwd_reuses_cached_entry_even_when_entry_has_extra_roots() { let codex_home = tempfile::tempdir().expect("tempdir"); @@ -322,6 +401,71 @@ async fn skills_for_cwd_reuses_cached_entry_even_when_entry_has_extra_roots() { assert_eq!(outcome_without_extra.errors, outcome_with_extra.errors); } +#[tokio::test] +async fn skills_for_cwd_separates_cache_entries_by_managed_allowed_sources() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + write_user_skill(&codex_home, "user", "user-skill", "from user"); + let unrestricted_stack = config_stack(&codex_home, ""); + let restricted_stack = config_stack_with_requirements( + &codex_home, + "", + ConfigRequirements { + skills: Some(Sourced::new( + SkillsRequirementsToml { + allowed_sources: Some(vec![SkillSourceRequirement::System]), + }, + RequirementSource::Unknown, + )), + ..Default::default() + }, + ); + let skills_manager = SkillsManager::new( + codex_home.path().abs(), + /*bundled_skills_enabled*/ true, + ); + + let unrestricted_input = SkillsLoadInput::new( + cwd.path().abs(), + Vec::new(), + unrestricted_stack.clone(), + bundled_skills_enabled_from_stack(&unrestricted_stack), + ); + let unrestricted = skills_manager + .skills_for_cwd( + &unrestricted_input, + /*force_reload*/ false, + Some(Arc::clone(&LOCAL_FS)), + ) + .await; + assert!( + unrestricted + .skills + .iter() + .any(|skill| skill.name == "user-skill") + ); + + let restricted_input = SkillsLoadInput::new( + cwd.path().abs(), + Vec::new(), + restricted_stack.clone(), + bundled_skills_enabled_from_stack(&restricted_stack), + ); + let restricted = skills_manager + .skills_for_cwd( + &restricted_input, + /*force_reload*/ false, + Some(Arc::clone(&LOCAL_FS)), + ) + .await; + assert!( + !restricted + .skills + .iter() + .any(|skill| skill.name == "user-skill") + ); +} + #[tokio::test] async fn skills_for_cwd_loads_repo_user_and_extra_roots_with_local_fs() { let codex_home = tempfile::tempdir().expect("tempdir");