mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
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
This commit is contained in:
1 parent
eeae88d8a6
commit
bae8d8f5b6
3 files changed
+200
-36
No files matched your search
@@ -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)]
|
||||
|
||||
@@ -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!(
|
||||
|
||||
Reference in new issue
Block a user