feat: improve skills cache key to take into account config layering

codex-rs/core/src/codex.rs

@@ -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

@@ -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

@@ -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

@@ -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);
+}