diff --git a/codex-rs/app-server/src/request_processors/catalog_processor.rs b/codex-rs/app-server/src/request_processors/catalog_processor.rs index d7931e9f24..0fe9267dca 100644 --- a/codex-rs/app-server/src/request_processors/catalog_processor.rs +++ b/codex-rs/app-server/src/request_processors/catalog_processor.rs @@ -17,12 +17,12 @@ const SKILLS_LIST_CWD_CONCURRENCY: usize = 5; fn skills_to_info( skills: &[codex_core::skills::SkillMetadata], - disabled_paths: &HashSet, + disabled_paths: &HashSet, ) -> Vec { skills .iter() .map(|skill| { - let enabled = !disabled_paths.contains(&skill.path_to_skills_md); + let enabled = !disabled_paths.contains(&skill.source_path); codex_app_server_protocol::SkillMetadata { name: skill.name.clone(), description: skill.description.clone(), diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 615a1c24ff..fe8939bf2c 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -666,7 +666,10 @@ pub async fn load_plugin_skills( .into_iter() .filter(|skill| skill.matches_product_restriction_for_product(restriction_product)) .collect::>(); - let disabled_skill_paths = resolve_disabled_skill_paths(&skills, skill_config_rules); + let disabled_skill_paths = resolve_disabled_skill_paths(&skills, skill_config_rules) + .into_iter() + .map(|path| path.path().clone()) + .collect(); ResolvedPluginSkills { skills, diff --git a/codex-rs/core-skills/src/config_rules.rs b/codex-rs/core-skills/src/config_rules.rs index 92ad2ab1a6..65d3c40107 100644 --- a/codex-rs/core-skills/src/config_rules.rs +++ b/codex-rs/core-skills/src/config_rules.rs @@ -5,6 +5,7 @@ use codex_config::ConfigLayerStack; use codex_config::ConfigLayerStackOrdering; use codex_config::SkillConfig; use codex_config::SkillsConfig; +use codex_exec_server::EnvironmentPathRef; use codex_utils_absolute_path::AbsolutePathBuf; use tracing::warn; @@ -71,28 +72,34 @@ pub fn skill_config_rules_from_stack(config_layer_stack: &ConfigLayerStack) -> S pub fn resolve_disabled_skill_paths( skills: &[SkillMetadata], rules: &SkillConfigRules, -) -> HashSet { +) -> HashSet { let mut disabled_paths = HashSet::new(); for entry in &rules.entries { match &entry.selector { SkillConfigRuleSelector::Path(path) => { - if entry.enabled { - disabled_paths.remove(path); - } else { - disabled_paths.insert(path.clone()); + for source_path in skills + .iter() + .filter(|skill| skill.path_to_skills_md == *path) + .map(|skill| skill.source_path.clone()) + { + if entry.enabled { + disabled_paths.remove(&source_path); + } else { + disabled_paths.insert(source_path); + } } } SkillConfigRuleSelector::Name(name) => { - for path in skills + for source_path in skills .iter() .filter(|skill| skill.name == *name) - .map(|skill| skill.path_to_skills_md.clone()) + .map(|skill| skill.source_path.clone()) { if entry.enabled { - disabled_paths.remove(&path); + disabled_paths.remove(&source_path); } else { - disabled_paths.insert(path); + disabled_paths.insert(source_path); } } } diff --git a/codex-rs/core-skills/src/injection.rs b/codex-rs/core-skills/src/injection.rs index 21ab1377c8..c4e77daf3b 100644 --- a/codex-rs/core-skills/src/injection.rs +++ b/codex-rs/core-skills/src/injection.rs @@ -1,13 +1,13 @@ use std::collections::HashMap; use std::collections::HashSet; -use crate::SkillLoadOutcome; use crate::SkillMetadata; use crate::build_skill_name_counts; use codex_analytics::AnalyticsEventsClient; use codex_analytics::InvocationType; use codex_analytics::SkillInvocation; use codex_analytics::TrackEventsContext; +use codex_exec_server::EnvironmentPathRef; use codex_otel::SessionTelemetry; use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; @@ -28,7 +28,6 @@ pub struct SkillInjection { pub async fn build_skill_injections( mentioned_skills: &[SkillMetadata], - _loaded_skills: Option<&SkillLoadOutcome>, otel: Option<&SessionTelemetry>, analytics_client: &AnalyticsEventsClient, tracking: TrackEventsContext, @@ -107,7 +106,7 @@ fn emit_skill_injected_metric( pub fn collect_explicit_skill_mentions( inputs: &[UserInput], skills: &[SkillMetadata], - disabled_paths: &HashSet, + disabled_paths: &HashSet, connector_slug_counts: &HashMap, ) -> Vec { let skill_name_counts = build_skill_name_counts(skills, disabled_paths).0; @@ -120,7 +119,7 @@ pub fn collect_explicit_skill_mentions( }; let mut selected: Vec = Vec::new(); let mut seen_names: HashSet = HashSet::new(); - let mut seen_paths: HashSet = HashSet::new(); + let mut seen_paths: HashSet = HashSet::new(); let mut blocked_plain_names: HashSet = HashSet::new(); for input in inputs { @@ -130,18 +129,22 @@ pub fn collect_explicit_skill_mentions( continue; }; - if selection_context.disabled_paths.contains(&path) || seen_paths.contains(&path) { - continue; - } - - if let Some(skill) = selection_context + let matching_skills = selection_context .skills .iter() - .find(|skill| skill.path_to_skills_md == path) + .filter(|skill| { + skill.path_to_skills_md == path + && !selection_context + .disabled_paths + .contains(&skill.source_path) + }) + .collect::>(); + if let [skill] = matching_skills.as_slice() + && !seen_paths.contains(&skill.source_path) { - seen_paths.insert(skill.path_to_skills_md.clone()); + seen_paths.insert(skill.source_path.clone()); seen_names.insert(skill.name.clone()); - selected.push(skill.clone()); + selected.push((*skill).clone()); } } } @@ -165,7 +168,7 @@ pub fn collect_explicit_skill_mentions( struct SkillSelectionContext<'a> { skills: &'a [SkillMetadata], - disabled_paths: &'a HashSet, + disabled_paths: &'a HashSet, skill_name_counts: &'a HashMap, connector_slug_counts: &'a HashMap, } @@ -316,7 +319,7 @@ fn select_skills_from_mentions( blocked_plain_names: &HashSet, mentions: &ToolMentions<'_>, seen_names: &mut HashSet, - seen_paths: &mut HashSet, + seen_paths: &mut HashSet, selected: &mut Vec, ) { if mentions.is_empty() { @@ -334,18 +337,39 @@ fn select_skills_from_mentions( .map(normalize_skill_path) .collect(); + let mention_skill_path_counts = selection_context + .skills + .iter() + .filter(|skill| { + !selection_context + .disabled_paths + .contains(&skill.source_path) + }) + .filter_map(|skill| { + let path = skill.path_to_skills_md.to_string_lossy(); + mention_skill_paths + .contains(path.as_ref()) + .then_some(path.into_owned()) + }) + .fold(HashMap::new(), |mut counts, path| { + *counts.entry(path).or_insert(0usize) += 1; + counts + }); + for skill in selection_context.skills { if selection_context .disabled_paths - .contains(&skill.path_to_skills_md) - || seen_paths.contains(&skill.path_to_skills_md) + .contains(&skill.source_path) + || seen_paths.contains(&skill.source_path) { continue; } let path_str = skill.path_to_skills_md.to_string_lossy(); - if mention_skill_paths.contains(path_str.as_ref()) { - seen_paths.insert(skill.path_to_skills_md.clone()); + if mention_skill_paths.contains(path_str.as_ref()) + && mention_skill_path_counts.get(path_str.as_ref()) == Some(&1) + { + seen_paths.insert(skill.source_path.clone()); seen_names.insert(skill.name.clone()); selected.push(skill.clone()); } @@ -354,8 +378,8 @@ fn select_skills_from_mentions( for skill in selection_context.skills { if selection_context .disabled_paths - .contains(&skill.path_to_skills_md) - || seen_paths.contains(&skill.path_to_skills_md) + .contains(&skill.source_path) + || seen_paths.contains(&skill.source_path) { continue; } @@ -382,7 +406,7 @@ fn select_skills_from_mentions( } if seen_names.insert(skill.name.clone()) { - seen_paths.insert(skill.path_to_skills_md.clone()); + seen_paths.insert(skill.source_path.clone()); selected.push(skill.clone()); } } diff --git a/codex-rs/core-skills/src/injection_tests.rs b/codex-rs/core-skills/src/injection_tests.rs index 9972d67dc8..a82e933c6c 100644 --- a/codex-rs/core-skills/src/injection_tests.rs +++ b/codex-rs/core-skills/src/injection_tests.rs @@ -1,10 +1,12 @@ use super::*; -use codex_utils_absolute_path::AbsolutePathBuf; +use codex_exec_server::EnvironmentPathRef; +use codex_exec_server::LocalFileSystem; use codex_utils_absolute_path::test_support::PathBufExt; use codex_utils_absolute_path::test_support::test_path_buf; use pretty_assertions::assert_eq; use std::collections::HashMap; use std::collections::HashSet; +use std::sync::Arc; fn make_skill(name: &str, path: &str) -> SkillMetadata { SkillMetadata { @@ -39,7 +41,7 @@ fn linked_skill_mention(name: &str, unix_path: &str) -> String { fn collect_mentions( inputs: &[UserInput], skills: &[SkillMetadata], - disabled_paths: &HashSet, + disabled_paths: &HashSet, connector_slug_counts: &HashMap, ) -> Vec { collect_explicit_skill_mentions(inputs, skills, disabled_paths, connector_slug_counts) @@ -206,7 +208,7 @@ fn collect_explicit_skill_mentions_skips_disabled_structured_and_blocks_plain_fa path: test_path_buf("/tmp/alpha"), }, ]; - let disabled = HashSet::from([test_path_buf("/tmp/alpha").abs()]); + let disabled = HashSet::from([EnvironmentPathRef::local(test_path_buf("/tmp/alpha").abs())]); let connector_counts = HashMap::new(); let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts); @@ -230,6 +232,27 @@ fn collect_explicit_skill_mentions_dedupes_by_path() { assert_eq!(selected, vec![alpha]); } +#[test] +fn collect_explicit_skill_mentions_skips_ambiguous_same_path_across_envs() { + let mut devbox = make_skill("demo-skill", "/tmp/demo/SKILL.md"); + devbox.environment_id = "devbox".to_string(); + devbox.source_path = EnvironmentPathRef::new( + Arc::new(LocalFileSystem::unsandboxed()), + test_path_buf("/tmp/demo/SKILL.md").abs(), + ); + let local = make_skill("demo-skill", "/tmp/demo/SKILL.md"); + let skills = vec![devbox, local]; + let inputs = vec![UserInput::Skill { + name: "demo-skill".to_string(), + path: test_path_buf("/tmp/demo/SKILL.md"), + }]; + let connector_counts = HashMap::new(); + + let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); + + assert_eq!(selected, Vec::new()); +} + #[test] fn collect_explicit_skill_mentions_skips_ambiguous_name() { let alpha = make_skill("demo-skill", "/tmp/alpha"); @@ -304,7 +327,7 @@ fn collect_explicit_skill_mentions_skips_when_linked_path_disabled() { text: format!("use {}", linked_skill_mention("demo-skill", "/tmp/alpha")), text_elements: Vec::new(), }]; - let disabled = HashSet::from([test_path_buf("/tmp/alpha").abs()]); + let disabled = HashSet::from([EnvironmentPathRef::local(test_path_buf("/tmp/alpha").abs())]); let connector_counts = HashMap::new(); let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts); diff --git a/codex-rs/core-skills/src/invocation_utils.rs b/codex-rs/core-skills/src/invocation_utils.rs index 4c9d0a4119..5ee9aaaccb 100644 --- a/codex-rs/core-skills/src/invocation_utils.rs +++ b/codex-rs/core-skills/src/invocation_utils.rs @@ -3,22 +3,24 @@ use std::path::Path; use crate::SkillLoadOutcome; use crate::SkillMetadata; +use codex_exec_server::EnvironmentPathRef; use codex_utils_absolute_path::AbsolutePathBuf; pub(crate) fn build_implicit_skill_path_indexes( skills: Vec, ) -> ( - HashMap, - HashMap, + HashMap, + HashMap, ) { let mut by_scripts_dir = HashMap::new(); let mut by_skill_doc_path = HashMap::new(); for skill in skills { - let skill_doc_path = canonicalize_if_exists(&skill.path_to_skills_md); + let skill_doc_path = canonicalize_if_exists(&skill.source_path); by_skill_doc_path.insert(skill_doc_path, skill.clone()); if let Some(skill_dir) = skill.path_to_skills_md.parent() { - let scripts_dir = canonicalize_if_exists(&skill_dir.join("scripts")); + let scripts_dir = + canonicalize_if_exists(&skill.source_path.with_path(skill_dir.join("scripts"))); by_scripts_dir.insert(scripts_dir, skill); } } @@ -29,7 +31,7 @@ pub(crate) fn build_implicit_skill_path_indexes( pub fn detect_implicit_skill_invocation_for_command( outcome: &SkillLoadOutcome, command: &str, - workdir: &AbsolutePathBuf, + workdir: &EnvironmentPathRef, ) -> Option { let workdir = canonicalize_if_exists(workdir); let tokens = tokenize_command(command); @@ -81,14 +83,20 @@ fn script_run_token(tokens: &[String]) -> Option<&str> { fn detect_skill_script_run( outcome: &SkillLoadOutcome, tokens: &[String], - workdir: &AbsolutePathBuf, + workdir: &EnvironmentPathRef, ) -> Option { let script_token = script_run_token(tokens)?; let script_path = Path::new(script_token); - let script_path = canonicalize_if_exists(&workdir.join(script_path)); + let script_path = canonicalize_if_exists(&resolve_path_ref(workdir, script_path)?); - for path in script_path.ancestors() { - if let Some(candidate) = outcome.implicit_skills_by_scripts_dir.get(&path) { + for path in script_path.path().ancestors() { + let Ok(path) = AbsolutePathBuf::from_absolute_path(path) else { + continue; + }; + if let Some(candidate) = outcome + .implicit_skills_by_scripts_dir + .get(&script_path.with_path(path)) + { return Some(candidate.clone()); } } @@ -99,7 +107,7 @@ fn detect_skill_script_run( fn detect_skill_doc_read( outcome: &SkillLoadOutcome, tokens: &[String], - workdir: &AbsolutePathBuf, + workdir: &EnvironmentPathRef, ) -> Option { if !command_reads_file(tokens) { return None; @@ -110,7 +118,7 @@ fn detect_skill_doc_read( continue; } let path = Path::new(token); - let candidate_path = canonicalize_if_exists(&workdir.join(path)); + let candidate_path = canonicalize_if_exists(&resolve_path_ref(workdir, path)?); if let Some(candidate) = outcome.implicit_skills_by_doc_path.get(&candidate_path) { return Some(candidate.clone()); } @@ -136,8 +144,22 @@ fn command_basename(command: &str) -> String { .to_string() } -fn canonicalize_if_exists(path: &AbsolutePathBuf) -> AbsolutePathBuf { - path.canonicalize().unwrap_or_else(|_| path.clone()) +fn resolve_path_ref(workdir: &EnvironmentPathRef, path: &Path) -> Option { + if path.is_absolute() { + AbsolutePathBuf::from_absolute_path(path) + .ok() + .map(|path| workdir.with_path(path)) + } else { + workdir.join_relative(path) + } +} + +fn canonicalize_if_exists(path: &EnvironmentPathRef) -> EnvironmentPathRef { + path.with_path( + path.path() + .canonicalize() + .unwrap_or_else(|_| path.path().clone()), + ) } #[cfg(test)] diff --git a/codex-rs/core-skills/src/invocation_utils_tests.rs b/codex-rs/core-skills/src/invocation_utils_tests.rs index 8a3dfc7f2d..b5a2b7a947 100644 --- a/codex-rs/core-skills/src/invocation_utils_tests.rs +++ b/codex-rs/core-skills/src/invocation_utils_tests.rs @@ -4,6 +4,7 @@ use super::canonicalize_if_exists; use super::detect_skill_doc_read; use super::detect_skill_script_run; use super::script_run_token; +use codex_exec_server::EnvironmentPathRef; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::test_support::PathBufExt; use codex_utils_absolute_path::test_support::test_path_buf; @@ -56,7 +57,8 @@ fn script_run_detection_excludes_python_c() { #[test] fn skill_doc_read_detection_matches_absolute_path() { let skill_doc_path = test_path_buf("/tmp/skill-test/SKILL.md").abs(); - let normalized_skill_doc_path = canonicalize_if_exists(&skill_doc_path); + let normalized_skill_doc_path = + canonicalize_if_exists(&EnvironmentPathRef::local(skill_doc_path.clone())); let skill = test_skill_metadata(skill_doc_path); let outcome = SkillLoadOutcome { implicit_skills_by_scripts_dir: Arc::new(HashMap::new()), @@ -70,7 +72,11 @@ fn skill_doc_read_detection_matches_absolute_path() { "|".to_string(), "head".to_string(), ]; - let found = detect_skill_doc_read(&outcome, &tokens, &test_path_buf("/tmp").abs()); + let found = detect_skill_doc_read( + &outcome, + &tokens, + &EnvironmentPathRef::local(test_path_buf("/tmp").abs()), + ); assert_eq!( found.map(|value| value.name), @@ -81,7 +87,9 @@ fn skill_doc_read_detection_matches_absolute_path() { #[test] fn skill_script_run_detection_matches_relative_path_from_skill_root() { let skill_doc_path = test_path_buf("/tmp/skill-test/SKILL.md").abs(); - let scripts_dir = canonicalize_if_exists(&test_path_buf("/tmp/skill-test/scripts").abs()); + let scripts_dir = canonicalize_if_exists(&EnvironmentPathRef::local( + test_path_buf("/tmp/skill-test/scripts").abs(), + )); let skill = test_skill_metadata(skill_doc_path); let outcome = SkillLoadOutcome { implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])), @@ -93,7 +101,11 @@ fn skill_script_run_detection_matches_relative_path_from_skill_root() { "scripts/fetch_comments.py".to_string(), ]; - let found = detect_skill_script_run(&outcome, &tokens, &test_path_buf("/tmp/skill-test").abs()); + let found = detect_skill_script_run( + &outcome, + &tokens, + &EnvironmentPathRef::local(test_path_buf("/tmp/skill-test").abs()), + ); assert_eq!( found.map(|value| value.name), @@ -104,7 +116,9 @@ fn skill_script_run_detection_matches_relative_path_from_skill_root() { #[test] fn skill_script_run_detection_matches_absolute_path_from_any_workdir() { let skill_doc_path = test_path_buf("/tmp/skill-test/SKILL.md").abs(); - let scripts_dir = canonicalize_if_exists(&test_path_buf("/tmp/skill-test/scripts").abs()); + let scripts_dir = canonicalize_if_exists(&EnvironmentPathRef::local( + test_path_buf("/tmp/skill-test/scripts").abs(), + )); let skill = test_skill_metadata(skill_doc_path); let outcome = SkillLoadOutcome { implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])), @@ -116,7 +130,11 @@ fn skill_script_run_detection_matches_absolute_path_from_any_workdir() { test_path_display("/tmp/skill-test/scripts/fetch_comments.py"), ]; - let found = detect_skill_script_run(&outcome, &tokens, &test_path_buf("/tmp/other").abs()); + let found = detect_skill_script_run( + &outcome, + &tokens, + &EnvironmentPathRef::local(test_path_buf("/tmp/other").abs()), + ); assert_eq!( found.map(|value| value.name), diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 1e8453985f..0c84800055 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -262,7 +262,8 @@ async fn skill_roots_with_home_dir( // Assemble one precedence-ordered root list from the two authorities above, then dedupe before // any reads happen so downstream loading stays oblivious to local-vs-selected-env routing. let mut roots = env_paths - .iter() + .first() + .into_iter() .flat_map(|env_path| repo_config_skill_roots(env_path, config_layer_stack)) .collect::>(); roots.extend(local_skill_roots( @@ -383,6 +384,7 @@ fn local_skill_roots( })); roots.extend(extra_skill_roots.into_iter().map(|path| SkillRoot { path: EnvironmentPathRef::new(Arc::clone(local_file_system), path), + environment_id: "local".to_string(), scope: SkillScope::User, plugin_id: None, plugin_root: None, @@ -395,9 +397,9 @@ fn repo_config_skill_roots( env_path: &SkillEnvironment, config_layer_stack: &ConfigLayerStack, ) -> Vec { - // Project config layers describe workspace-local `.codex/skills` directories. Their absolute - // paths only make sense inside the selected environment, so keep them bound to `env_path` - // rather than the optional local filesystem used for client-local system/user/plugin roots. + // Project config layers come from the one config stack for this turn/cwd. Bind them to the + // primary selected environment only; rebinding those same absolute folders into every + // secondary environment would attribute the primary cwd's `.codex/skills` to the wrong env. config_layer_stack .get_layers( ConfigLayerStackOrdering::HighestPrecedenceFirst, diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index ba70b01fa7..2dcafef6f6 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -351,6 +351,49 @@ async fn skill_roots_bind_repo_and_local_roots_to_their_own_file_systems() -> an Ok(()) } +#[tokio::test] +async fn skill_roots_bind_project_config_roots_to_primary_environment_only() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let project_root = codex_home.path().join("workspace"); + fs::create_dir_all(project_root.join(".git"))?; + fs::create_dir_all(project_root.join(REPO_ROOT_CONFIG_DIR_NAME))?; + let cfg = make_config_for_cwd(&codex_home, project_root.clone()).await; + + let primary_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); + let secondary_file_system: Arc = + Arc::new(LocalFileSystem::unsandboxed()); + let roots = super::skill_roots( + &[ + super::SkillEnvironment { + environment_id: "primary".to_string(), + path: EnvironmentPathRef::new(primary_file_system.clone(), cfg.cwd.clone()), + }, + super::SkillEnvironment { + environment_id: "secondary".to_string(), + path: EnvironmentPathRef::new(secondary_file_system, cfg.cwd.clone()), + }, + ], + /*local_file_system*/ None, + &cfg.config_layer_stack, + Vec::new(), + Vec::new(), + ) + .await; + + let repo_roots = roots + .iter() + .filter(|root| root.scope == SkillScope::Repo) + .collect::>(); + assert_eq!(repo_roots.len(), 1); + assert_eq!(repo_roots[0].environment_id, "primary"); + assert!(Arc::ptr_eq( + &repo_roots[0].path.file_system(), + &primary_file_system + )); + + Ok(()) +} + #[tokio::test] async fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<()> { let tmp = tempfile::tempdir()?; diff --git a/codex-rs/core-skills/src/manager.rs b/codex-rs/core-skills/src/manager.rs index 07ac1f36f4..edfc1f0e32 100644 --- a/codex-rs/core-skills/src/manager.rs +++ b/codex-rs/core-skills/src/manager.rs @@ -344,7 +344,7 @@ fn config_skills_cache_key( fn finalize_skill_outcome( mut outcome: SkillLoadOutcome, - disabled_paths: HashSet, + disabled_paths: HashSet, ) -> SkillLoadOutcome { outcome.disabled_paths = disabled_paths; let (by_scripts_dir, by_doc_path) = diff --git a/codex-rs/core-skills/src/manager_tests.rs b/codex-rs/core-skills/src/manager_tests.rs index 8707aaad57..0484cb391e 100644 --- a/codex-rs/core-skills/src/manager_tests.rs +++ b/codex-rs/core-skills/src/manager_tests.rs @@ -387,7 +387,7 @@ async fn skills_for_config_disables_plugin_skills_by_name() { .abs(); assert_eq!(skill.path_to_skills_md, skill_path); - assert!(outcome.disabled_paths.contains(&skill.path_to_skills_md)); + assert!(outcome.disabled_paths.contains(&skill.source_path)); assert!( !outcome .allowed_skills_for_implicit_invocation() @@ -665,10 +665,12 @@ fn disabled_paths_for_skills_allows_session_flags_to_disable_user_enabled_skill( let skill_config_rules = skill_config_rules_from_stack(&stack); assert_eq!( resolve_disabled_skill_paths(&[skill], &skill_config_rules), - HashSet::from([skill_path - .abs() - .canonicalize() - .expect("skill path should canonicalize")]) + HashSet::from([EnvironmentPathRef::local( + skill_path + .abs() + .canonicalize() + .expect("skill path should canonicalize"), + )]) ); } @@ -698,10 +700,12 @@ fn disabled_paths_for_skills_disables_matching_name_selectors() { let skill_config_rules = skill_config_rules_from_stack(&stack); assert_eq!( resolve_disabled_skill_paths(&[skill], &skill_config_rules), - HashSet::from([skill_path - .abs() - .canonicalize() - .expect("skill path should canonicalize")]) + HashSet::from([EnvironmentPathRef::local( + skill_path + .abs() + .canonicalize() + .expect("skill path should canonicalize"), + )]) ); } @@ -740,6 +744,45 @@ fn disabled_paths_for_skills_allows_name_selector_to_override_path_selector() { ); } +#[cfg_attr(windows, ignore)] +#[test] +fn disabled_paths_for_skills_disables_same_raw_path_in_each_environment() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_path = write_demo_skill(&tempdir); + let mut devbox_skill = test_skill("demo-skill", skill_path.clone()); + devbox_skill.environment_id = "devbox".to_string(); + devbox_skill.source_path = EnvironmentPathRef::new( + Arc::new(codex_exec_server::LocalFileSystem::unsandboxed()), + devbox_skill.path_to_skills_md.clone(), + ); + let local_skill = test_skill("demo-skill", skill_path.clone()); + let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml")) + .expect("user config path should be absolute"); + let user_layer = ConfigLayerEntry::new( + ConfigLayerSource::User { + file: user_file, + profile: None, + }, + toml::from_str(&path_toggle_config(&skill_path, /*enabled*/ false)) + .expect("user layer toml"), + ); + let stack = ConfigLayerStack::new( + vec![user_layer], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + + let skill_config_rules = skill_config_rules_from_stack(&stack); + assert_eq!( + resolve_disabled_skill_paths( + &[devbox_skill.clone(), local_skill.clone()], + &skill_config_rules + ), + HashSet::from([devbox_skill.source_path, local_skill.source_path]) + ); +} + #[cfg_attr(windows, ignore)] #[tokio::test] async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() { diff --git a/codex-rs/core-skills/src/mention_counts.rs b/codex-rs/core-skills/src/mention_counts.rs index b7482ca36e..2eabc07c6f 100644 --- a/codex-rs/core-skills/src/mention_counts.rs +++ b/codex-rs/core-skills/src/mention_counts.rs @@ -2,17 +2,17 @@ use std::collections::HashMap; use std::collections::HashSet; use super::SkillMetadata; -use codex_utils_absolute_path::AbsolutePathBuf; +use codex_exec_server::EnvironmentPathRef; /// Counts how often each skill name appears (exact and ASCII-lowercase), excluding disabled paths. pub fn build_skill_name_counts( skills: &[SkillMetadata], - disabled_paths: &HashSet, + disabled_paths: &HashSet, ) -> (HashMap, HashMap) { let mut exact_counts: HashMap = HashMap::new(); let mut lower_counts: HashMap = HashMap::new(); for skill in skills { - if disabled_paths.contains(&skill.path_to_skills_md) { + if disabled_paths.contains(&skill.source_path) { continue; } *exact_counts.entry(skill.name.clone()).or_insert(0) += 1; diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index 562925b72e..cf68d15309 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -91,16 +91,16 @@ pub struct SkillError { pub struct SkillLoadOutcome { pub skills: Vec, pub errors: Vec, - pub disabled_paths: HashSet, + pub disabled_paths: HashSet, pub(crate) skill_roots: Vec, pub(crate) skill_root_by_path: Arc>, - pub(crate) implicit_skills_by_scripts_dir: Arc>, - pub(crate) implicit_skills_by_doc_path: Arc>, + pub(crate) implicit_skills_by_scripts_dir: Arc>, + pub(crate) implicit_skills_by_doc_path: Arc>, } impl SkillLoadOutcome { pub fn is_skill_enabled(&self, skill: &SkillMetadata) -> bool { - !self.disabled_paths.contains(&skill.path_to_skills_md) + !self.disabled_paths.contains(&skill.source_path) } pub fn is_skill_allowed_for_implicit_invocation(&self, skill: &SkillMetadata) -> bool { @@ -120,6 +120,15 @@ impl SkillLoadOutcome { .iter() .map(|skill| (skill, self.is_skill_enabled(skill))) } + + pub fn has_multiple_environments(&self) -> bool { + self.skills + .iter() + .map(|skill| skill.environment_id.as_str()) + .collect::>() + .len() + > 1 + } } pub fn filter_skill_load_outcome_for_product( diff --git a/codex-rs/core/src/agent/role_tests.rs b/codex-rs/core/src/agent/role_tests.rs index cc19abee4f..b5a6f7fb93 100644 --- a/codex-rs/core/src/agent/role_tests.rs +++ b/codex-rs/core/src/agent/role_tests.rs @@ -1,7 +1,7 @@ use super::*; use crate::SkillsManager; use crate::config::ConfigBuilder; -use crate::skills_load_input_from_config; +use crate::skills_load_input; use codex_config::ConfigLayerStackOrdering; use codex_core_plugins::PluginsManager; use codex_protocol::config_types::ServiceTier; @@ -425,10 +425,12 @@ enabled = false Arc::clone(&codex_exec_server::LOCAL_FS), config.cwd.clone(), ); - let skills_input = skills_load_input_from_config( + let skills_input = skills_load_input( &config, - codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), - cwd, + vec![codex_core_skills::loader::SkillEnvironment { + environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), + path: cwd, + }], Some(Arc::clone(&codex_exec_server::LOCAL_FS)), effective_skill_roots, ); diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index aa5784d4fe..a46c104651 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -95,7 +95,7 @@ pub(crate) use skills::default_skill_metadata_budget; pub(crate) use skills::injection; pub(crate) use skills::manager; pub(crate) use skills::maybe_emit_implicit_skill_invocation; -pub(crate) use skills::skills_load_input_from_config; +pub(crate) use skills::skills_load_input; mod stream_events_utils; pub mod test_support; mod unified_exec; diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 8270db5ac1..622be87cad 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -36,7 +36,6 @@ use crate::path_utils::normalize_for_native_workdir; use crate::realtime_conversation::RealtimeConversationManager; use crate::session_prefix::format_subagent_notification_message; use crate::skills::SkillRenderSideEffects; -use crate::skills_load_input_from_config; use crate::turn_metadata::TurnMetadataState; use crate::turn_timing::now_unix_timestamp_ms; use async_channel::Receiver; @@ -2744,7 +2743,9 @@ impl Session { let available_skills = build_available_skills( &turn_context.turn_skills.outcome, default_skill_metadata_budget(turn_context.model_info.context_window), - if turn_context.environments.turn_environments.len() > 1 { + if turn_context.environments.turn_environments.len() > 1 + || turn_context.turn_skills.outcome.has_multiple_environments() + { crate::skills::render::SkillEnvironmentRender::Include } else { crate::skills::render::SkillEnvironmentRender::Omit diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 95862b5bf4..aca3fb42af 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -3,6 +3,7 @@ use super::*; use crate::config::ConstraintError; use crate::goals::GoalRuntimeState; use crate::skills::SkillError; +use crate::skills_load_input; use crate::state::ActiveTurn; use codex_protocol::SessionId; use codex_protocol::config_types::SERVICE_TIER_DEFAULT_REQUEST_VALUE; @@ -473,10 +474,12 @@ async fn warm_plugins_and_skills_for_session_init( let plugins_input = config.plugins_config_input(); let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); - let skills_input = skills_load_input_from_config( + let skills_input = skills_load_input( config.as_ref(), - environment_id, - path_ref, + vec![codex_core_skills::loader::SkillEnvironment { + environment_id, + path: path_ref, + }], local_file_system, effective_skill_roots, ); diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 86cce77229..bb250c2b7d 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -8,6 +8,7 @@ use crate::context::TurnAborted; use crate::function_tool::FunctionCallError; use crate::shell::default_user_shell; use crate::skills::SkillRenderSideEffects; +use crate::skills::render::SkillEnvironmentRender; use crate::skills::render::SkillMetadataBudget; use crate::test_support::models_manager_with_provider; use crate::tools::format_exec_output_str; @@ -4127,13 +4128,15 @@ async fn new_default_turn_uses_config_aware_skills_for_role_overrides() { .services .skills_manager .skills_for_cwd( - &crate::skills_load_input_from_config( + &crate::skills_load_input( &parent_config, - codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), - crate::skills::EnvironmentPathRef::new( - Arc::clone(&skill_fs), - parent_config.cwd.clone(), - ), + vec![codex_core_skills::loader::SkillEnvironment { + environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), + path: crate::skills::EnvironmentPathRef::new( + Arc::clone(&skill_fs), + parent_config.cwd.clone(), + ), + }], session .services .environment_manager @@ -4708,10 +4711,12 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { environment.get_filesystem(), per_turn_config.cwd.clone(), ); - let skills_input = crate::skills_load_input_from_config( + let skills_input = crate::skills_load_input( &per_turn_config, - codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), - cwd, + vec![codex_core_skills::loader::SkillEnvironment { + environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), + path: cwd, + }], services .environment_manager .try_local_environment() @@ -6562,10 +6567,12 @@ where environment.get_filesystem(), per_turn_config.cwd.clone(), ); - let skills_input = crate::skills_load_input_from_config( + let skills_input = crate::skills_load_input( &per_turn_config, - codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), - cwd, + vec![codex_core_skills::loader::SkillEnvironment { + environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), + path: cwd, + }], services .environment_manager .try_local_environment() @@ -7302,6 +7309,68 @@ async fn build_initial_context_trims_skill_metadata_from_context_window_budget() ); } +#[tokio::test] +async fn build_initial_context_qualifies_skills_only_when_skill_envs_are_ambiguous() { + let (session, mut turn_context) = make_session_and_context().await; + let make_skill = |name: &str, environment_id: &str| SkillMetadata { + name: name.to_string(), + description: "desc".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + path_to_skills_md: test_path_buf(&format!("/tmp/{name}/SKILL.md")) + .abs() + .clone(), + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf(&format!("/tmp/{name}/SKILL.md")).abs(), + ), + environment_id: environment_id.to_string(), + scope: SkillScope::Repo, + plugin_id: None, + }; + + let mut single_env_outcome = SkillLoadOutcome::default(); + single_env_outcome.skills = vec![make_skill("repo-skill", "local")]; + turn_context.turn_skills = TurnSkillsContext::new(Arc::new(single_env_outcome)); + let single_env_context = session.build_initial_context(&turn_context).await; + let single_env_texts = developer_input_texts(&single_env_context); + assert!( + single_env_texts + .iter() + .any(|text| text.contains("- repo-skill: desc (file: /tmp/repo-skill/SKILL.md)")) + ); + assert!(single_env_texts.iter().all(|text| !text.contains("env:"))); + + let mut secondary_environment = turn_context.environments.turn_environments[0].clone(); + secondary_environment.environment_id = "devbox".to_string(); + turn_context + .environments + .turn_environments + .push(secondary_environment); + let multi_turn_env_context = session.build_initial_context(&turn_context).await; + let multi_turn_env_texts = developer_input_texts(&multi_turn_env_context); + assert!(multi_turn_env_texts.iter().any(|text| { + text.contains("- repo-skill: desc (env: local, file: /tmp/repo-skill/SKILL.md)") + })); + turn_context.environments.turn_environments.truncate(1); + + let mut mixed_env_outcome = SkillLoadOutcome::default(); + mixed_env_outcome.skills = vec![ + make_skill("repo-skill", "devbox"), + make_skill("user-skill", "local"), + ]; + turn_context.turn_skills = TurnSkillsContext::new(Arc::new(mixed_env_outcome)); + let mixed_env_context = session.build_initial_context(&turn_context).await; + let mixed_env_texts = developer_input_texts(&mixed_env_context); + assert!(mixed_env_texts.iter().any(|text| { + text.contains("- repo-skill: desc (env: devbox, file: /tmp/repo-skill/SKILL.md)") + })); + assert!(mixed_env_texts.iter().any(|text| { + text.contains("- user-skill: desc (env: local, file: /tmp/user-skill/SKILL.md)") + })); +} + #[test] fn emit_thread_start_skill_metrics_records_enabled_kept_and_truncated_values() { let session_telemetry = test_session_telemetry_without_metadata(); diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 8219c19019..784bfa30e7 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -533,7 +533,6 @@ async fn build_skills_and_plugins( warnings: skill_warnings, } = build_skill_injections( &mentioned_skills, - Some(skills_outcome), Some(&turn_context.session_telemetry), &sess.services.analytics_events_client, tracking.clone(), diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index e6e487a20c..47e6b20d53 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -706,7 +706,7 @@ impl Session { .plugins_for_config(&plugins_input) .await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); - let skills_input = crate::skills::skills_load_input_from_environments( + let skills_input = crate::skills::skills_load_input( &per_turn_config, skill_env_paths, local_file_system, diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index 64fd33206a..d9b3f940c3 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -36,26 +36,7 @@ pub use codex_core_skills::render; pub use codex_core_skills::render::SkillRenderSideEffects; pub use codex_core_skills::system; -pub(crate) fn skills_load_input_from_config( - config: &Config, - environment_id: String, - env_path: EnvironmentPathRef, - local_file_system: Option>, - effective_skill_roots: Vec, -) -> SkillsLoadInput { - SkillsLoadInput::new( - vec![codex_core_skills::loader::SkillEnvironment { - environment_id, - path: env_path, - }], - local_file_system, - effective_skill_roots, - config.config_layer_stack.clone(), - config.bundled_skills_enabled(), - ) -} - -pub(crate) fn skills_load_input_from_environments( +pub(crate) fn skills_load_input( config: &Config, env_paths: Vec, local_file_system: Option>, @@ -76,13 +57,20 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation( command: &str, workdir: &AbsolutePathBuf, ) { + let Some(turn_environment) = turn_context.environments.primary() else { + return; + }; let Some(candidate) = detect_implicit_skill_invocation_for_command( turn_context.turn_skills.outcome.as_ref(), command, - workdir, + &EnvironmentPathRef::new( + turn_environment.environment.get_filesystem(), + workdir.clone(), + ), ) else { return; }; + let environment_id = candidate.environment_id.clone(); let invocation = SkillInvocation { skill_name: candidate.name, skill_scope: candidate.scope, @@ -98,7 +86,7 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation( }; let skill_path = invocation.skill_path.to_string_lossy(); let skill_name = invocation.skill_name.clone(); - let seen_key = format!("{skill_scope}:{skill_path}:{skill_name}"); + let seen_key = format!("{environment_id}:{skill_scope}:{skill_path}:{skill_name}"); let inserted = { let mut seen_skills = turn_context .turn_skills