Preserve shared bundled skill caches

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::<Vec<_>>();
+    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::<Vec<_>>();
+    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()?;

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<Arc<dyn ExecutorFileSystem>>,
     ) -> Vec<SkillRoot> {
+        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<Arc<dyn ExecutorFileSystem>>,
     ) -> 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)]

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!(