From bae8d8f5b66954f51cde0b0d55a6e806cd9779cd Mon Sep 17 00:00:00 2001 From: andrewgu-oai Date: Tue, 4 Aug 2026 22:43:48 +0000 Subject: [PATCH] Preserve shared bundled skill caches (#36989) ## Why The bundled system-skill cache is shared by every process using the same `CODEX_HOME`. A service with bundled skills disabled must not remove cache files that another service still uses. ## What changed - Filter system skill roots from disabled services without deleting the shared cache. - Ensure bundled system skills are installed during loads when they are enabled, including after a runtime configuration change. ## Testing Add app-server coverage for preserving the cache across enabled and disabled services and refreshing stale cached skills after runtime enablement. GitOrigin-RevId: 1eca999b33b7be69873a40a91767fd26f0c2b457 --- .../app-server/tests/suite/v2/skills_list.rs | 182 ++++++++++++++++++ codex-rs/ext/skills/src/host_service.rs | 26 ++- codex-rs/ext/skills/src/host_service_tests.rs | 28 --- 3 files changed, 200 insertions(+), 36 deletions(-) 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 6e71ce9dc50a..b83be6f7f872 100644 --- a/codex-rs/app-server/tests/suite/v2/skills_list.rs +++ b/codex-rs/app-server/tests/suite/v2/skills_list.rs @@ -15,6 +15,7 @@ use codex_app_server_protocol::ExperimentalFeatureEnablementSetResponse; use codex_app_server_protocol::MergeStrategy; use codex_app_server_protocol::PluginListParams; use codex_app_server_protocol::PluginListResponse; +use codex_app_server_protocol::SkillScope; use codex_app_server_protocol::SkillsChangedNotification; use codex_app_server_protocol::SkillsExtraRootsSetParams; use codex_app_server_protocol::SkillsExtraRootsSetResponse; @@ -152,6 +153,187 @@ fn write_cached_local_curated_plugin_with_skill(codex_home: &std::path::Path) -> Ok(()) } +#[tokio::test] +async fn skills_list_disabled_bundled_skills_preserves_shared_system_skill_cache() -> Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + let mut enabled_mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .without_auto_env() + .build_initialized_with_timeout(DEFAULT_TIMEOUT) + .await?; + + let enabled_skills_request_id = enabled_mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: true, + }) + .await?; + let SkillsListResponse { data } = timeout( + DEFAULT_TIMEOUT, + enabled_mcp.read_response(enabled_skills_request_id), + ) + .await??; + assert_eq!(data.len(), 1); + assert_eq!(data[0].errors, Vec::new()); + let system_skill_paths = data[0] + .skills + .iter() + .filter(|skill| skill.scope == SkillScope::System) + .map(|skill| skill.path.clone()) + .collect::>(); + assert!( + !system_skill_paths.is_empty(), + "expected enabled app-server to materialize bundled system skills" + ); + + let mut disabled_mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .without_auto_env() + .with_args(&["-c", "skills.bundled.enabled=false"]) + .build_initialized_with_timeout(DEFAULT_TIMEOUT) + .await?; + let disabled_skills_request_id = disabled_mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: true, + }) + .await?; + let SkillsListResponse { data } = timeout( + DEFAULT_TIMEOUT, + disabled_mcp.read_response(disabled_skills_request_id), + ) + .await??; + assert_eq!(data.len(), 1); + assert_eq!(data[0].errors, Vec::new()); + assert!( + data[0] + .skills + .iter() + .all(|skill| skill.scope != SkillScope::System) + ); + assert!( + system_skill_paths + .iter() + .all(|path| path.as_path().is_file()), + "disabled app-server must not remove the cache shared by other processes" + ); + + let reloaded_skills_request_id = enabled_mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: true, + }) + .await?; + let SkillsListResponse { data } = timeout( + DEFAULT_TIMEOUT, + enabled_mcp.read_response(reloaded_skills_request_id), + ) + .await??; + assert_eq!(data.len(), 1); + assert_eq!(data[0].errors, Vec::new()); + let reloaded_system_skill_paths = data[0] + .skills + .iter() + .filter(|skill| skill.scope == SkillScope::System) + .map(|skill| skill.path.clone()) + .collect::>(); + assert_eq!(reloaded_system_skill_paths, system_skill_paths); + Ok(()) +} + +#[tokio::test] +async fn skills_list_runtime_enable_refreshes_shared_system_skill_cache() -> Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + let stale_skill_path = codex_home + .path() + .join("skills/.system/stale-system-skill/SKILL.md"); + std::fs::create_dir_all( + stale_skill_path + .parent() + .expect("stale system skill should have a parent"), + )?; + std::fs::write( + &stale_skill_path, + "---\nname: stale-system-skill\ndescription: stale system skill\n---\n\n# Body\n", + )?; + std::fs::write( + codex_home.path().join("config.toml"), + "[skills.bundled]\nenabled = false\n", + )?; + + let mut mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .without_auto_env() + .build_initialized_with_timeout(DEFAULT_TIMEOUT) + .await?; + + let disabled_skills_request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: true, + }) + .await?; + let SkillsListResponse { data } = timeout( + DEFAULT_TIMEOUT, + mcp.read_response(disabled_skills_request_id), + ) + .await??; + assert_eq!(data.len(), 1); + assert_eq!(data[0].errors, Vec::new()); + assert!( + data[0] + .skills + .iter() + .all(|skill| skill.scope != SkillScope::System) + ); + assert!(stale_skill_path.is_file()); + + let enable_request_id = mcp + .send_config_batch_write_request(ConfigBatchWriteParams { + edits: vec![ConfigEdit { + key_path: "skills.bundled.enabled".to_string(), + value: serde_json::json!(true), + merge_strategy: MergeStrategy::Replace, + }], + file_path: None, + expected_version: None, + reload_user_config: true, + }) + .await?; + let _: ConfigWriteResponse = + timeout(DEFAULT_TIMEOUT, mcp.read_response(enable_request_id)).await??; + + let enabled_skills_request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: true, + }) + .await?; + let SkillsListResponse { data } = timeout( + DEFAULT_TIMEOUT, + mcp.read_response(enabled_skills_request_id), + ) + .await??; + assert_eq!(data.len(), 1); + assert_eq!(data[0].errors, Vec::new()); + assert!( + data[0] + .skills + .iter() + .any(|skill| skill.scope == SkillScope::System) + ); + assert!( + data[0] + .skills + .iter() + .all(|skill| skill.name != "stale-system-skill") + ); + assert!(!stale_skill_path.exists()); + Ok(()) +} + #[tokio::test] async fn runtime_remote_plugin_toggle_updates_local_curated_plugin_skills() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/ext/skills/src/host_service.rs b/codex-rs/ext/skills/src/host_service.rs index 8bc487eaaa4c..1663d36d3420 100644 --- a/codex-rs/ext/skills/src/host_service.rs +++ b/codex-rs/ext/skills/src/host_service.rs @@ -25,7 +25,6 @@ use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS; use codex_core_skills::loader::SkillRoot; use codex_core_skills::loader::load_skills_from_roots; use codex_skills::install_system_skills; -use codex_skills::system_cache_root_dir; use crate::host_roots::resolve_skill_roots; @@ -95,12 +94,10 @@ impl HostSkillsService { cache_by_config: RwLock::new(HashMap::new()), root_scan_slots: Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), }; - if !bundled_skills_enabled { - // The loader caches bundled skills under `skills/.system`. Clearing that directory is - // best-effort cleanup; root selection still enforces the config even if removal fails. - let _ = std::fs::remove_dir_all(system_cache_root_dir(&service.codex_home)); - } else if let Err(err) = install_system_skills(&service.codex_home) { - tracing::error!("failed to install system skills: {err}"); + // The cache is shared by every process using this CODEX_HOME. Disabled services filter + // system roots when loading rather than mutating shared state. + if bundled_skills_enabled { + service.ensure_system_skills_installed(); } service } @@ -157,6 +154,9 @@ impl HostSkillsService { input: &HostSkillsLoadInput, fs: Option>, ) -> Vec { + if input.bundled_skills_enabled { + self.ensure_system_skills_installed(); + } let mut roots = resolve_skill_roots( fs, &input.config_layer_stack, @@ -177,6 +177,10 @@ impl HostSkillsService { force_reload: bool, fs: Option>, ) -> HostSkillsSnapshot { + let bundled_skills_enabled = bundled_skills_enabled_from_stack(&input.config_layer_stack); + if bundled_skills_enabled { + self.ensure_system_skills_installed(); + } let use_cwd_cache = fs.is_some(); if use_cwd_cache && !force_reload @@ -193,7 +197,7 @@ impl HostSkillsService { self.extra_roots(), ) .await; - if !bundled_skills_enabled_from_stack(&input.config_layer_stack) { + if !bundled_skills_enabled { roots.retain(|root| root.scope != SkillScope::System); } let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); @@ -278,6 +282,12 @@ impl HostSkillsService { Err(err) => err.into_inner().clone(), } } + + fn ensure_system_skills_installed(&self) { + if let Err(err) = install_system_skills(&self.codex_home) { + tracing::error!("failed to install system skills: {err}"); + } + } } #[derive(Debug, Clone, PartialEq, Eq, Hash)] diff --git a/codex-rs/ext/skills/src/host_service_tests.rs b/codex-rs/ext/skills/src/host_service_tests.rs index 0b355d2c1f88..5c9b6afdb0b9 100644 --- a/codex-rs/ext/skills/src/host_service_tests.rs +++ b/codex-rs/ext/skills/src/host_service_tests.rs @@ -156,25 +156,6 @@ async fn skills_for_config_with_stack( .clone() } -#[test] -fn new_with_disabled_bundled_skills_removes_stale_cached_system_skills() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let stale_system_skill_dir = codex_home.path().join("skills/.system/stale-skill"); - fs::create_dir_all(&stale_system_skill_dir).expect("create stale system skill dir"); - fs::write(stale_system_skill_dir.join("SKILL.md"), "# stale\n") - .expect("write stale system skill"); - - let _skills_service = HostSkillsService::new( - codex_home.path().abs(), - /*bundled_skills_enabled*/ false, - ); - - assert!( - !codex_home.path().join("skills/.system").exists(), - "expected disabling system skills to remove stale cached bundled skills" - ); -} - #[tokio::test] async fn skills_for_config_reuses_cache_for_same_effective_config() { let codex_home = tempfile::tempdir().expect("tempdir"); @@ -564,15 +545,6 @@ async fn skills_for_config_excludes_bundled_skills_when_disabled_in_config() { /*bundled_skills_enabled*/ false, ); - // Recreate the cached bundled skill after startup cleanup so this assertion exercises - // root selection rather than relying on directory removal succeeding. - fs::create_dir_all(&bundled_skill_dir).expect("recreate bundled skill dir"); - fs::write( - bundled_skill_dir.join("SKILL.md"), - "---\nname: bundled-skill\ndescription: from bundled root\n---\n\n# Body\n", - ) - .expect("rewrite bundled skill"); - let outcome = skills_for_config_with_stack(&skills_service, &cwd, &config_layer_stack, &[]).await; assert!(