From 7d486ffa9493df78137202c024830b5c826421f5 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Tue, 11 Aug 2026 10:15:41 +0000 Subject: [PATCH] Honor per-directory bundled skill settings in `skills/list` (#37979) ## Why `skills/list` can load multiple working directories in one request, but bundled skill discovery did not account for different effective configurations across those directories. ## What changed - Resolve `skills.bundled.enabled` from each load input's effective config layer stack inside the host skills service. - Include or exclude system-scoped skills for each working directory based on that resolved setting. - Keep bundled skills enabled by default when the setting is absent or invalid. ## Testing - Add a multi-directory `skills/list` test with bundled skills disabled in one project and enabled in another. - Add config-layer tests for defaults, overrides, and malformed bundled-skill configuration. GitOrigin-RevId: e4e97dc7d0a374ac71d0aa11e55a9e337f661717 --- .../request_processors/catalog_processor.rs | 2 - codex-rs/app-server/src/skills_watcher.rs | 1 - .../app-server/tests/suite/v2/skills_list.rs | 52 +++++++++++++++++++ codex-rs/config/src/lib.rs | 1 + codex-rs/config/src/skills_config.rs | 21 ++++++++ codex-rs/config/src/skills_config_tests.rs | 33 ++++++++++++ codex-rs/core/src/config/mod.rs | 2 +- .../core/src/plugins/skill_snapshot_tests.rs | 4 +- codex-rs/core/src/skills.rs | 2 - codex-rs/ext/skills/src/host_service.rs | 33 ++---------- codex-rs/ext/skills/src/host_service_tests.rs | 50 ++++-------------- codex-rs/ext/skills/src/lib.rs | 1 - .../tests/executor_file_system_authority.rs | 10 ++-- codex-rs/ext/skills/tests/skills_extension.rs | 14 ++--- 14 files changed, 135 insertions(+), 91 deletions(-) diff --git a/codex-rs/app-server/src/request_processors/catalog_processor.rs b/codex-rs/app-server/src/request_processors/catalog_processor.rs index 14a60482a7..f6cdd775fe 100644 --- a/codex-rs/app-server/src/request_processors/catalog_processor.rs +++ b/codex-rs/app-server/src/request_processors/catalog_processor.rs @@ -533,7 +533,6 @@ impl CatalogRequestProcessor { let skills_request = skills_service.for_request(); let mut data = futures::stream::iter(cwds.into_iter().enumerate()) .map(|(index, cwd)| { - let config = &config; let fs = fs.clone(); let skills_request = &skills_request; let effective_skill_roots = effective_skill_roots.clone(); @@ -560,7 +559,6 @@ impl CatalogRequestProcessor { cwd_abs.clone(), effective_skill_roots, config_layer_stack, - config.bundled_skills_enabled(), ) .with_plugin_skill_snapshots(plugin_skill_snapshots); let snapshot = skills_request diff --git a/codex-rs/app-server/src/skills_watcher.rs b/codex-rs/app-server/src/skills_watcher.rs index 80993e13b5..026d0b41ce 100644 --- a/codex-rs/app-server/src/skills_watcher.rs +++ b/codex-rs/app-server/src/skills_watcher.rs @@ -116,7 +116,6 @@ impl SkillsWatcher { config.cwd.clone(), plugin_outcome.effective_plugin_skill_roots(), config.config_layer_stack.clone(), - config.bundled_skills_enabled(), ); let roots = thread_manager .skills_service() diff --git a/codex-rs/app-server/tests/suite/v2/skills_list.rs b/codex-rs/app-server/tests/suite/v2/skills_list.rs index 2685182748..6a8253a4f2 100644 --- a/codex-rs/app-server/tests/suite/v2/skills_list.rs +++ b/codex-rs/app-server/tests/suite/v2/skills_list.rs @@ -248,6 +248,58 @@ async fn skills_list_disabled_bundled_skills_preserves_shared_system_skill_cache Ok(()) } +#[tokio::test] +async fn skills_list_uses_each_cwds_bundled_skills_configuration() -> Result<()> { + let codex_home = TempDir::new()?; + let disabled_cwd = TempDir::new()?; + let enabled_cwd = TempDir::new()?; + + for (cwd, enabled) in [(disabled_cwd.path(), false), (enabled_cwd.path(), true)] { + std::fs::create_dir_all(cwd.join(".git"))?; + std::fs::create_dir_all(cwd.join(".codex"))?; + std::fs::write( + cwd.join(".codex/config.toml"), + format!("[skills.bundled]\nenabled = {enabled}\n"), + )?; + set_project_trust_level(codex_home.path(), cwd, TrustLevel::Trusted)?; + } + + let mut app_server = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized_with_timeout(DEFAULT_TIMEOUT) + .await?; + + let request_id = app_server + .send_skills_list_request(SkillsListParams { + cwds: vec![ + disabled_cwd.path().to_path_buf(), + enabled_cwd.path().to_path_buf(), + ], + force_reload: true, + }) + .await?; + let SkillsListResponse { data } = + timeout(DEFAULT_TIMEOUT, app_server.read_response(request_id)).await??; + + assert_eq!(data.len(), 2); + for (entry, (cwd, enabled)) in data + .iter() + .zip([(disabled_cwd.path(), false), (enabled_cwd.path(), true)]) + { + assert_eq!(entry.cwd, cwd); + assert_eq!(entry.errors, Vec::new()); + assert_eq!( + entry + .skills + .iter() + .any(|skill| skill.scope == SkillScope::System), + enabled + ); + } + + Ok(()) +} + #[tokio::test] async fn skills_list_runtime_enable_refreshes_shared_system_skill_cache() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index dfe54af894..b94cc7ab3b 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -158,6 +158,7 @@ pub use skills_config::SkillConfigRule; pub use skills_config::SkillConfigRuleSelector; pub use skills_config::SkillConfigRules; pub use skills_config::SkillsConfig; +pub use skills_config::bundled_skills_enabled_from_stack; pub use skills_config::skill_config_rules_from_stack; pub use state::ConfigLayerEntry; pub use state::ConfigLayerStack; diff --git a/codex-rs/config/src/skills_config.rs b/codex-rs/config/src/skills_config.rs index 09f482df02..8b0593c887 100644 --- a/codex-rs/config/src/skills_config.rs +++ b/codex-rs/config/src/skills_config.rs @@ -119,6 +119,27 @@ impl SkillConfigRules { } } +/// Returns whether bundled skills are enabled by the effective configuration. +pub fn bundled_skills_enabled_from_stack(config_layer_stack: &ConfigLayerStack) -> bool { + let effective_config = config_layer_stack.effective_config(); + let Some(skills_value) = effective_config + .as_table() + .and_then(|table| table.get("skills")) + else { + return true; + }; + + let skills: SkillsConfig = match skills_value.clone().try_into() { + Ok(skills) => skills, + Err(err) => { + warn!("invalid skills config: {err}"); + return true; + } + }; + + skills.bundled.unwrap_or_default().enabled +} + /// Resolves skill enablement rules from user and session configuration layers. pub fn skill_config_rules_from_stack(config_layer_stack: &ConfigLayerStack) -> SkillConfigRules { let mut entries = Vec::new(); diff --git a/codex-rs/config/src/skills_config_tests.rs b/codex-rs/config/src/skills_config_tests.rs index 57ff1622bb..23cb3c5c5f 100644 --- a/codex-rs/config/src/skills_config_tests.rs +++ b/codex-rs/config/src/skills_config_tests.rs @@ -11,6 +11,7 @@ use tempfile::TempDir; use super::SkillConfigRule; use super::SkillConfigRuleSelector; use super::SkillConfigRules; +use super::bundled_skills_enabled_from_stack; use super::skill_config_rules_from_stack; fn user_layer(codex_home: &TempDir, config: &str) -> ConfigLayerEntry { @@ -50,6 +51,38 @@ enabled = {enabled} ) } +#[test] +fn bundled_skills_follow_effective_configuration() { + let codex_home = TempDir::new().expect("temp dir"); + + assert!(bundled_skills_enabled_from_stack(&stack( + &codex_home, + "", + "" + ))); + assert!(!bundled_skills_enabled_from_stack(&stack( + &codex_home, + "[skills.bundled]\nenabled = false\n", + "" + ))); + assert!(bundled_skills_enabled_from_stack(&stack( + &codex_home, + "[skills.bundled]\nenabled = false\n", + "[skills.bundled]\nenabled = true\n" + ))); +} + +#[test] +fn malformed_bundled_skills_config_defaults_to_enabled() { + let codex_home = TempDir::new().expect("temp dir"); + + assert!(bundled_skills_enabled_from_stack(&stack( + &codex_home, + "[skills]\nbundled = 'invalid'\n", + "" + ))); +} + #[test] fn session_flags_can_reenable_user_disabled_path() { let codex_home = TempDir::new().expect("temp dir"); diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 6463032282..70476fcd1a 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -4509,7 +4509,7 @@ impl Config { } pub fn bundled_skills_enabled(&self) -> bool { - crate::skills::bundled_skills_enabled_from_stack(&self.config_layer_stack) + codex_config::bundled_skills_enabled_from_stack(&self.config_layer_stack) } /// Returns whether effective requirements allow selecting a concrete profile. diff --git a/codex-rs/core/src/plugins/skill_snapshot_tests.rs b/codex-rs/core/src/plugins/skill_snapshot_tests.rs index 39e23930fe..ff8abc1bcc 100644 --- a/codex-rs/core/src/plugins/skill_snapshot_tests.rs +++ b/codex-rs/core/src/plugins/skill_snapshot_tests.rs @@ -36,6 +36,9 @@ remote_plugin = true [plugins."{PLUGIN_CONFIG_NAME}"] enabled = true + +[skills.bundled] +enabled = false "#, ), ); @@ -68,7 +71,6 @@ enabled = true cwd, plugin_outcome.effective_plugin_skill_roots(), config.config_layer_stack.clone(), - /*bundled_skills_enabled*/ false, ) .with_plugin_skill_snapshots( plugins_manager.plugin_skill_snapshots_for_config(&plugins_input), diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index 4b7656404e..a0d2aefde3 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -23,7 +23,6 @@ pub use codex_skills::detect_implicit_skill_invocation_for_command; pub use codex_skills_extension::HostSkillsLoadInput; pub use codex_skills_extension::HostSkillsService; pub use codex_skills_extension::SkillLoadOutcome; -pub use codex_skills_extension::bundled_skills_enabled_from_stack; #[derive(Debug, Default)] struct ImplicitSkillInvocations(Mutex>); @@ -36,7 +35,6 @@ pub(crate) fn skills_load_input_from_config( config.cwd.clone(), effective_skill_roots, config.config_layer_stack.clone(), - config.bundled_skills_enabled(), ) } diff --git a/codex-rs/ext/skills/src/host_service.rs b/codex-rs/ext/skills/src/host_service.rs index d07a097192..c13f9d07da 100644 --- a/codex-rs/ext/skills/src/host_service.rs +++ b/codex-rs/ext/skills/src/host_service.rs @@ -7,6 +7,7 @@ use std::sync::Weak; use codex_config::ConfigLayerStack; use codex_config::SkillConfigRules; +use codex_config::bundled_skills_enabled_from_stack; use codex_config::skill_config_rules_from_stack; use codex_exec_server::ExecutorFileSystem; use codex_exec_server::LOCAL_FS; @@ -19,9 +20,7 @@ use tokio::sync::OnceCell; use tokio::sync::Semaphore; use tracing::info; use tracing::instrument; -use tracing::warn; -use codex_config::SkillsConfig; use codex_skills::LoadedSkills; use codex_skills::SkillLoadFuture; use codex_skills::SkillRootLoadRequest; @@ -43,7 +42,6 @@ pub struct HostSkillsLoadInput { cwd: AbsolutePathBuf, effective_skill_roots: Vec, config_layer_stack: ConfigLayerStack, - bundled_skills_enabled: bool, plugin_skill_snapshots: Option>, } @@ -52,13 +50,11 @@ impl HostSkillsLoadInput { cwd: AbsolutePathBuf, effective_skill_roots: Vec, config_layer_stack: ConfigLayerStack, - bundled_skills_enabled: bool, ) -> Self { Self { cwd, effective_skill_roots, config_layer_stack, - bundled_skills_enabled, plugin_skill_snapshots: None, } } @@ -219,7 +215,8 @@ impl HostSkillsService { input: &HostSkillsLoadInput, fs: Option>, ) -> Vec { - if input.bundled_skills_enabled { + let bundled_skills_enabled = bundled_skills_enabled_from_stack(&input.config_layer_stack); + if bundled_skills_enabled { self.ensure_system_skills_installed(); } let mut roots = resolve_skill_roots( @@ -230,7 +227,7 @@ impl HostSkillsService { self.extra_roots(), ) .await; - if !input.bundled_skills_enabled { + if !bundled_skills_enabled { roots.retain(|root| root.scope != SkillScope::System); } roots @@ -487,28 +484,6 @@ impl Hash for FileSystemIdentity { } } -pub fn bundled_skills_enabled_from_stack( - config_layer_stack: &codex_config::ConfigLayerStack, -) -> bool { - let effective_config = config_layer_stack.effective_config(); - let Some(skills_value) = effective_config - .as_table() - .and_then(|table| table.get("skills")) - else { - return true; - }; - - let skills: SkillsConfig = match skills_value.clone().try_into() { - Ok(skills) => skills, - Err(err) => { - warn!("invalid skills config: {err}"); - return true; - } - }; - - skills.bundled.unwrap_or_default().enabled -} - fn config_skills_cache_key( roots: &[HostSkillRoot], skill_config_rules: &SkillConfigRules, diff --git a/codex-rs/ext/skills/src/host_service_tests.rs b/codex-rs/ext/skills/src/host_service_tests.rs index 86ed5538b1..046e5cf549 100644 --- a/codex-rs/ext/skills/src/host_service_tests.rs +++ b/codex-rs/ext/skills/src/host_service_tests.rs @@ -172,7 +172,6 @@ async fn skills_for_config_with_stack( cwd.path().abs(), effective_skill_roots.to_vec(), config_layer_stack.clone(), - bundled_skills_enabled_from_stack(config_layer_stack), ); skills_service .snapshot_for_config(&skills_input, Some(Arc::clone(&LOCAL_FS))) @@ -226,7 +225,6 @@ async fn watchable_skill_root_paths_exclude_plugin_and_system_roots() { cwd.path().abs(), vec![plugin_skill_root.clone()], config_layer_stack, - /*bundled_skills_enabled*/ true, ); let skills_service = HostSkillsService::new( codex_home.path().abs(), @@ -262,7 +260,6 @@ async fn snapshot_for_config_merges_extension_host_and_legacy_plugin_roots() { cwd.path().abs(), vec![plugin_skill_root], config_layer_stack, - /*bundled_skills_enabled*/ false, ); let skills_service = HostSkillsService::new( codex_home.path().abs(), @@ -356,12 +353,7 @@ async fn skills_list_snapshots_share_host_roots_only_within_one_request() { ); let request = skills_service.for_request(); let input = |cwd: &TempDir, config_layer_stack| { - HostSkillsLoadInput::new( - cwd.path().abs(), - Vec::new(), - config_layer_stack, - /*bundled_skills_enabled*/ true, - ) + HostSkillsLoadInput::new(cwd.path().abs(), Vec::new(), config_layer_stack) }; write_user_skill(&codex_home, "first", "first-skill", "first skill"); @@ -434,7 +426,6 @@ async fn skills_for_config_refreshes_cache_when_remote_plugin_id_changes() { cwd.path().abs(), vec![plugin_skill_root.clone()], config_layer_stack.clone(), - bundled_skills_enabled_from_stack(&config_layer_stack), ) .with_plugin_skill_snapshots(Some(test_plugin_skill_snapshots())); let plugin_snapshot = skills_service @@ -504,12 +495,8 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() { /*bundled_skills_enabled*/ true, ); - let skills_input = HostSkillsLoadInput::new( - cwd.path().abs(), - Vec::new(), - config_layer_stack.clone(), - bundled_skills_enabled_from_stack(&config_layer_stack), - ); + let skills_input = + HostSkillsLoadInput::new(cwd.path().abs(), Vec::new(), config_layer_stack.clone()); let empty_snapshot = skills_service .for_request() .snapshot_for_cwd( @@ -699,12 +686,8 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() { ConfigRequirementsToml::default(), ) .expect("valid config layer stack"); - let skills_input = HostSkillsLoadInput::new( - cwd.path().abs(), - Vec::new(), - config_layer_stack.clone(), - bundled_skills_enabled_from_stack(&config_layer_stack), - ); + let skills_input = + HostSkillsLoadInput::new(cwd.path().abs(), Vec::new(), config_layer_stack.clone()); let skills_service = HostSkillsService::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, @@ -770,12 +753,8 @@ async fn skills_for_cwd_without_fs_skips_repo_roots() { ConfigRequirementsToml::default(), ) .expect("valid config layer stack"); - let skills_input = HostSkillsLoadInput::new( - cwd.path().abs(), - Vec::new(), - config_layer_stack.clone(), - bundled_skills_enabled_from_stack(&config_layer_stack), - ); + let skills_input = + HostSkillsLoadInput::new(cwd.path().abs(), Vec::new(), config_layer_stack.clone()); let skills_service = HostSkillsService::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, @@ -843,12 +822,8 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() { codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); - let base_input = HostSkillsLoadInput::new( - cwd.path().abs(), - Vec::new(), - config_layer_stack.clone(), - bundled_skills_enabled_from_stack(&config_layer_stack), - ); + let base_input = + HostSkillsLoadInput::new(cwd.path().abs(), Vec::new(), config_layer_stack.clone()); let config_input = base_input .clone() .with_plugin_skill_snapshots(Some(test_plugin_skill_snapshots())); @@ -929,12 +904,7 @@ async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); - let parent_input = HostSkillsLoadInput::new( - cwd.path().abs(), - Vec::new(), - parent_stack.clone(), - bundled_skills_enabled_from_stack(&parent_stack), - ); + let parent_input = HostSkillsLoadInput::new(cwd.path().abs(), Vec::new(), parent_stack.clone()); let parent_snapshot = skills_service .for_request() diff --git a/codex-rs/ext/skills/src/lib.rs b/codex-rs/ext/skills/src/lib.rs index a7cd744477..1495ac69c3 100644 --- a/codex-rs/ext/skills/src/lib.rs +++ b/codex-rs/ext/skills/src/lib.rs @@ -34,7 +34,6 @@ pub use host_prompt::InjectedHostSkillPrompts; pub use host_service::HostSkillsLoadInput; pub use host_service::HostSkillsRequest; pub use host_service::HostSkillsService; -pub use host_service::bundled_skills_enabled_from_stack; pub use host_snapshot::HostSkillsSnapshot; pub use provider::ExecutorSkillProvider; pub use provider::HostSkillProvider; diff --git a/codex-rs/ext/skills/tests/executor_file_system_authority.rs b/codex-rs/ext/skills/tests/executor_file_system_authority.rs index ed4a92ebcc..3b9cbf7448 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -222,7 +222,8 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() { ConfigLayerSource::Project { dot_codex_folder: project_folder, }, - toml::Value::Table(Default::default()), + toml::from_str("[skills.bundled]\nenabled = false\n") + .expect("valid bundled skills config"), )], Default::default(), ConfigRequirementsToml::default(), @@ -236,12 +237,7 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() { ); let snapshot = service .snapshot_for_config( - &HostSkillsLoadInput::new( - cwd, - Vec::new(), - config_layer_stack, - /*bundled_skills_enabled*/ false, - ), + &HostSkillsLoadInput::new(cwd, Vec::new(), config_layer_stack), Some(Arc::new(SyntheticFileSystem { alias_root: PathUri::from_abs_path(&alias_root), canonical_root: PathUri::from_abs_path(&canonical_root), diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index 0eed785886..6995249bb9 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -4,6 +4,8 @@ use std::sync::Mutex; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; +use codex_config::ConfigLayerEntry; +use codex_config::ConfigLayerSource; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirementsToml; use codex_exec_server::LOCAL_FS; @@ -1879,7 +1881,10 @@ async fn host_catalog_compacts_shared_paths_under_budget_pressure() -> TestResul let root = AbsolutePathBuf::try_from(std::fs::canonicalize(root)?)?; let rendered_root = root.to_string_lossy().replace('\\', "/"); let config_layer_stack = ConfigLayerStack::new( - Vec::new(), + vec![ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::from_str("[skills.bundled]\nenabled = false\n")?, + )], Default::default(), ConfigRequirementsToml::default(), )?; @@ -1892,12 +1897,7 @@ async fn host_catalog_compacts_shared_paths_under_budget_pressure() -> TestResul service.set_extra_roots(vec![root]); let snapshot = service .snapshot_for_config( - &HostSkillsLoadInput::new( - codex_home, - Vec::new(), - config_layer_stack, - /*bundled_skills_enabled*/ false, - ), + &HostSkillsLoadInput::new(codex_home, Vec::new(), config_layer_stack), Some(Arc::clone(&LOCAL_FS)), ) .await;