agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

feat: improve skills cache key to take into account config layering (#14806)

Browse Source 
Fix https://github.com/openai/codex/issues/14161

This fixes sub-agent [[skills.config]] overrides being ignored when
parent and child share the same cwd. The root cause was that turn skill
loading rebuilt from cwd-only state and reused a cwd-scoped cache, so
role-local skill enable/disable overrides did not reliably affect the
spawned agent's effective skill set.

This change switches turn construction to use the effective per-turn
config and adds a config-aware skills cache keyed by skill roots plus
final disabled paths.
This commit is contained in:
jif-oai
2026-03-16 16:12:44 +00:00
committed by GitHub
Unverified
parent 33acc1e65f
commit 18ad67549c
4 changed files with 242 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/codex.rs
+2 -2
View File
@@ -1201,6 +1201,7 @@ impl Session {
// todo(aibrahim): store this state somewhere else so we don't need to mut config
let config = session_configuration.original_config_do_not_use.clone();
let mut per_turn_config = (*config).clone();
per_turn_config.cwd = session_configuration.cwd.clone();
per_turn_config.model_reasoning_effort =
session_configuration.collaboration_mode.reasoning_effort();
per_turn_config.model_reasoning_summary = session_configuration.model_reasoning_summary;
@@ -2355,8 +2356,7 @@ impl Session {
let skills_outcome = Arc::new(
self.services
.skills_manager
.skills_for_cwd(&session_configuration.cwd, false)
.await,
.skills_for_config(&per_turn_config),
);
let mut turn_context: TurnContext = Self::make_turn_context(
Some(Arc::clone(&self.services.auth_manager)),
codex-rs/core/src/codex_tests.rs
+76
View File
@@ -2064,6 +2064,82 @@ async fn session_configuration_apply_preserves_split_file_system_policy_on_cwd_o
);
}
#[cfg_attr(windows, ignore)]
#[tokio::test]
async fn new_default_turn_uses_config_aware_skills_for_role_overrides() {
let (session, _turn_context) = make_session_and_context().await;
let parent_config = session.get_config().await;
let codex_home = parent_config.codex_home.clone();
let skill_dir = codex_home.join("skills").join("demo");
std::fs::create_dir_all(&skill_dir).expect("create skill dir");
let skill_path = skill_dir.join("SKILL.md");
std::fs::write(
&skill_path,
"---\nname: demo-skill\ndescription: demo description\n---\n\n# Body\n",
)
.expect("write skill");
let parent_outcome = session
.services
.skills_manager
.skills_for_cwd(&parent_config.cwd, true)
.await;
let parent_skill = parent_outcome
.skills
.iter()
.find(|skill| skill.name == "demo-skill")
.expect("demo skill should be discovered");
assert_eq!(parent_outcome.is_skill_enabled(parent_skill), true);
let role_path = codex_home.join("skills-role.toml");
std::fs::write(
&role_path,
format!(
r#"developer_instructions = "Stay focused"
[[skills.config]]
path = "{}"
enabled = false
"#,
skill_path.display()
),
)
.expect("write role config");
let mut child_config = (*parent_config).clone();
child_config.agent_roles.insert(
"custom".to_string(),
crate::config::AgentRoleConfig {
description: None,
config_file: Some(role_path),
nickname_candidates: None,
},
);
crate::agent::role::apply_role_to_config(&mut child_config, Some("custom"))
.await
.expect("custom role should apply");
{
let mut state = session.state.lock().await;
state.session_configuration.original_config_do_not_use = Arc::new(child_config);
}
let child_turn = session
.new_default_turn_with_sub_id("role-skill-turn".to_string())
.await;
let child_skill = child_turn
.turn_skills
.outcome
.skills
.iter()
.find(|skill| skill.name == "demo-skill")
.expect("demo skill should be discovered");
assert_eq!(
child_turn.turn_skills.outcome.is_skill_enabled(child_skill),
false
);
}
#[tokio::test]
async fn session_configuration_apply_rederives_legacy_file_system_policy_on_cwd_update() {
let mut session_configuration = make_session_configuration_for_tests().await;
codex-rs/core/src/skills/manager.rs
+82 -25
View File
@@ -31,6 +31,7 @@ pub struct SkillsManager {
codex_home: PathBuf,
plugins_manager: Arc<PluginsManager>,
cache_by_cwd: RwLock<HashMap<PathBuf, SkillLoadOutcome>>,
cache_by_config: RwLock<HashMap<ConfigSkillsCacheKey, SkillLoadOutcome>>,
}
impl SkillsManager {
@@ -43,6 +44,7 @@ impl SkillsManager {
codex_home,
plugins_manager,
cache_by_cwd: RwLock::new(HashMap::new()),
cache_by_config: RwLock::new(HashMap::new()),
};
if !bundled_skills_enabled {
// The loader caches bundled skills under `skills/.system`. Clearing that directory is
@@ -55,21 +57,25 @@ impl SkillsManager {
}
/// Load skills for an already-constructed [`Config`], avoiding any additional config-layer
/// loading. This also seeds the per-cwd cache for subsequent lookups.
/// loading.
///
/// This path uses a cache keyed by the effective skill-relevant config state rather than just
/// cwd so role-local and session-local skill overrides cannot bleed across sessions that happen
/// to share a directory.
pub fn skills_for_config(&self, config: &Config) -> SkillLoadOutcome {
let cwd = &config.cwd;
if let Some(outcome) = self.cached_outcome_for_cwd(cwd) {
let roots = self.skill_roots_for_config(config);
let cache_key = config_skills_cache_key(&roots, &config.config_layer_stack);
if let Some(outcome) = self.cached_outcome_for_config(&cache_key) {
return outcome;
}
let roots = self.skill_roots_for_config(config);
let outcome =
finalize_skill_outcome(load_skills_from_roots(roots), &config.config_layer_stack);
let mut cache = match self.cache_by_cwd.write() {
Ok(cache) => cache,
Err(err) => err.into_inner(),
};
cache.insert(cwd.to_path_buf(), outcome.clone());
let mut cache = self
.cache_by_config
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner);
cache.insert(cache_key, outcome.clone());
outcome
}
@@ -163,21 +169,34 @@ impl SkillsManager {
);
let outcome = load_skills_from_roots(roots);
let outcome = finalize_skill_outcome(outcome, &config_layer_stack);
let mut cache = match self.cache_by_cwd.write() {
Ok(cache) => cache,
Err(err) => err.into_inner(),
};
let mut cache = self
.cache_by_cwd
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner);
cache.insert(cwd.to_path_buf(), outcome.clone());
outcome
}
pub fn clear_cache(&self) {
let mut cache = match self.cache_by_cwd.write() {
Ok(cache) => cache,
Err(err) => err.into_inner(),
let cleared_cwd = {
let mut cache = self
.cache_by_cwd
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let cleared = cache.len();
cache.clear();
cleared
};
let cleared = cache.len();
cache.clear();
let cleared_config = {
let mut cache = self
.cache_by_config
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let cleared = cache.len();
cache.clear();
cleared
};
let cleared = cleared_cwd + cleared_config;
info!("skills cache cleared ({cleared} entries)");
}
@@ -187,6 +206,22 @@ impl SkillsManager {
Err(err) => err.into_inner().get(cwd).cloned(),
}
}
fn cached_outcome_for_config(
&self,
cache_key: &ConfigSkillsCacheKey,
) -> Option<SkillLoadOutcome> {
match self.cache_by_config.read() {
Ok(cache) => cache.get(cache_key).cloned(),
Err(err) => err.into_inner().get(cache_key).cloned(),
}
}
}
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
struct ConfigSkillsCacheKey {
roots: Vec<(PathBuf, u8)>,
disabled_paths: Vec<PathBuf>,
}
pub(crate) fn bundled_skills_enabled_from_stack(
@@ -214,7 +249,6 @@ pub(crate) fn bundled_skills_enabled_from_stack(
fn disabled_paths_from_stack(
config_layer_stack: &crate::config_loader::ConfigLayerStack,
) -> HashSet<PathBuf> {
let mut disabled = HashSet::new();
let mut configs = HashMap::new();
for layer in
config_layer_stack.get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, true)
@@ -243,13 +277,36 @@ fn disabled_paths_from_stack(
}
}
for (path, enabled) in configs {
if !enabled {
disabled.insert(path);
}
}
configs
.into_iter()
.filter_map(|(path, enabled)| (!enabled).then_some(path))
.collect()
}
disabled
fn config_skills_cache_key(
roots: &[SkillRoot],
config_layer_stack: &crate::config_loader::ConfigLayerStack,
) -> ConfigSkillsCacheKey {
let mut disabled_paths: Vec<PathBuf> = disabled_paths_from_stack(config_layer_stack)
.into_iter()
.collect();
disabled_paths.sort_unstable();
ConfigSkillsCacheKey {
roots: roots
.iter()
.map(|root| {
let scope_rank = match root.scope {
SkillScope::Repo => 0,
SkillScope::User => 1,
SkillScope::System => 2,
SkillScope::Admin => 3,
};
(root.path.clone(), scope_rank)
})
.collect(),
disabled_paths,
}
}
fn finalize_skill_outcome(
codex-rs/core/src/skills/manager_tests.rs
+82 -3
View File
@@ -36,7 +36,7 @@ fn new_with_disabled_bundled_skills_removes_stale_cached_system_skills() {
}
#[tokio::test]
async fn skills_for_config_seeds_cache_by_cwd() {
async fn skills_for_config_reuses_cache_for_same_effective_config() {
let codex_home = tempfile::tempdir().expect("tempdir");
let cwd = tempfile::tempdir().expect("tempdir");
@@ -60,8 +60,8 @@ async fn skills_for_config_seeds_cache_by_cwd() {
"expected skill-a to be discovered"
);
// Write a new skill after the first call; the second call should hit the cache and not
// reflect the new file.
// Write a new skill after the first call; the second call should reuse the config-aware cache
// entry because the effective skill config is unchanged.
write_user_skill(&codex_home, "b", "skill-b", "from b");
let outcome2 = skills_manager.skills_for_config(&cfg);
assert_eq!(outcome2.errors, outcome1.errors);
@@ -354,3 +354,82 @@ enabled = false
HashSet::from([skill_path])
);
}
#[cfg_attr(windows, ignore)]
#[tokio::test]
async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() {
let codex_home = tempfile::tempdir().expect("tempdir");
let cwd = tempfile::tempdir().expect("tempdir");
let skill_dir = codex_home.path().join("skills").join("demo");
fs::create_dir_all(&skill_dir).expect("create skill dir");
let skill_path = skill_dir.join("SKILL.md");
fs::write(
&skill_path,
"---\nname: demo-skill\ndescription: demo description\n---\n\n# Body\n",
)
.expect("write skill");
fs::write(
codex_home.path().join(crate::config::CONFIG_TOML_FILE),
format!(
r#"[[skills.config]]
path = "{}"
enabled = false
"#,
skill_path.display()
),
)
.expect("write config");
let parent_config = ConfigBuilder::default()
.codex_home(codex_home.path().to_path_buf())
.harness_overrides(ConfigOverrides {
cwd: Some(cwd.path().to_path_buf()),
..Default::default()
})
.build()
.await
.expect("load parent config");
let role_path = codex_home.path().join("enable-role.toml");
fs::write(
&role_path,
format!(
r#"[[skills.config]]
path = "{}"
enabled = true
"#,
skill_path.display()
),
)
.expect("write role config");
let mut child_config = parent_config.clone();
child_config.agent_roles.insert(
"custom".to_string(),
crate::config::AgentRoleConfig {
description: None,
config_file: Some(role_path),
nickname_candidates: None,
},
);
crate::agent::role::apply_role_to_config(&mut child_config, Some("custom"))
.await
.expect("custom role should apply");
let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf()));
let skills_manager = SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager, true);
let parent_outcome = skills_manager.skills_for_cwd(cwd.path(), true).await;
let parent_skill = parent_outcome
.skills
.iter()
.find(|skill| skill.name == "demo-skill")
.expect("demo skill should be discovered");
assert_eq!(parent_outcome.is_skill_enabled(parent_skill), false);
let child_outcome = skills_manager.skills_for_config(&child_config);
let child_skill = child_outcome
.skills
.iter()
.find(|skill| skill.name == "demo-skill")
.expect("demo skill should be discovered");
assert_eq!(child_outcome.is_skill_enabled(child_skill), true);
}