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 6f17e4614f4..99aa8b6e726 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 9e0ef7c471e..cf51d4958ed 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -665,7 +665,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 92ad2ab1a68..65d3c401077 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 45979fa5c49..ed70fa29866 100644 --- a/codex-rs/core-skills/src/injection.rs +++ b/codex-rs/core-skills/src/injection.rs @@ -1,7 +1,3 @@ -use std::collections::HashMap; -use std::collections::HashSet; -use std::sync::Arc; - use crate::SkillLoadOutcome; use crate::SkillMetadata; use crate::build_skill_name_counts; @@ -9,11 +5,13 @@ use codex_analytics::AnalyticsEventsClient; use codex_analytics::InvocationType; use codex_analytics::SkillInvocation; use codex_analytics::TrackEventsContext; -use codex_exec_server::LOCAL_FS; +use codex_exec_server::EnvironmentPathRef; use codex_otel::SessionTelemetry; use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_plugins::mention_syntax::TOOL_MENTION_SIGIL; +use std::collections::HashMap; +use std::collections::HashSet; #[derive(Debug, Default)] pub struct SkillInjections { @@ -30,7 +28,7 @@ pub struct SkillInjection { pub async fn build_skill_injections( mentioned_skills: &[SkillMetadata], - loaded_skills: Option<&SkillLoadOutcome>, + _loaded_skills: Option<&SkillLoadOutcome>, otel: Option<&SessionTelemetry>, analytics_client: &AnalyticsEventsClient, tracking: TrackEventsContext, @@ -46,13 +44,7 @@ pub async fn build_skill_injections( let mut invocations = Vec::new(); for skill in mentioned_skills { - let fs = loaded_skills - .and_then(|outcome| outcome.file_system_for_skill(skill)) - .unwrap_or_else(|| Arc::clone(&LOCAL_FS)); - match fs - .read_file_text(&skill.path_to_skills_md, /*sandbox*/ None) - .await - { + match skill.source_path.read_to_string(/*sandbox*/ None).await { Ok(contents) => { emit_skill_injected_metric(otel, skill, "ok"); invocations.push(SkillInvocation { @@ -106,7 +98,8 @@ fn emit_skill_injected_metric( /// Structured `UserInput::Skill` selections are resolved first by path against /// enabled skills. Text inputs are then scanned to extract `$skill-name` tokens, and we /// iterate `skills` in their existing order to preserve prior ordering semantics. -/// Explicit links are resolved by path and plain names are only used when the match +/// Explicit links are resolved by path, using the linked name only when duplicate +/// filesystem-bound copies share the same raw path. Plain names are only used when the match /// is unambiguous. /// /// Complexity: `O(T + (N_s + N_t) * S)` time, `O(S + M)` space, where: @@ -115,7 +108,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; @@ -128,7 +121,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 { @@ -138,18 +131,23 @@ 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.name == *name + && 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()); } } } @@ -173,7 +171,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, } @@ -182,6 +180,7 @@ pub struct ToolMentions<'a> { names: HashSet<&'a str>, paths: HashSet<&'a str>, plain_names: HashSet<&'a str>, + linked_mentions: HashSet>, } impl<'a> ToolMentions<'a> { @@ -196,6 +195,16 @@ impl<'a> ToolMentions<'a> { pub fn paths(&self) -> impl Iterator + '_ { self.paths.iter().copied() } + + fn linked_mentions(&self) -> impl Iterator> + '_ { + self.linked_mentions.iter().copied() + } +} + +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +struct LinkedToolMention<'a> { + name: &'a str, + path: &'a str, } #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -260,6 +269,7 @@ pub fn extract_tool_mentions_with_sigil(text: &str, sigil: char) -> ToolMentions let mut mentioned_names: HashSet<&str> = HashSet::new(); let mut mentioned_paths: HashSet<&str> = HashSet::new(); let mut plain_names: HashSet<&str> = HashSet::new(); + let mut linked_mentions: HashSet> = HashSet::new(); let mut index = 0; while index < text_bytes.len() { @@ -276,6 +286,7 @@ pub fn extract_tool_mentions_with_sigil(text: &str, sigil: char) -> ToolMentions mentioned_names.insert(name); } mentioned_paths.insert(path); + linked_mentions.insert(LinkedToolMention { name, path }); } index = end_index; continue; @@ -315,6 +326,7 @@ pub fn extract_tool_mentions_with_sigil(text: &str, sigil: char) -> ToolMentions names: mentioned_names, paths: mentioned_paths, plain_names, + linked_mentions, } } @@ -324,7 +336,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() { @@ -341,19 +353,82 @@ fn select_skills_from_mentions( }) .map(normalize_skill_path) .collect(); + let mention_skill_links: HashSet> = mentions + .linked_mentions() + .filter(|mention| { + !matches!( + tool_kind_for_path(mention.path), + ToolMentionKind::App | ToolMentionKind::Mcp | ToolMentionKind::Plugin + ) + }) + .map(|mention| LinkedToolMention { + name: mention.name, + path: normalize_skill_path(mention.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 + }); + let mention_skill_link_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_links + .contains(&LinkedToolMention { + name: skill.name.as_str(), + path: path.as_ref(), + }) + .then_some((skill.name.clone(), path.into_owned())) + }) + .fold(HashMap::new(), |mut counts, link| { + *counts.entry(link).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()); + let path = path_str.as_ref(); + let linked_mention = LinkedToolMention { + name: skill.name.as_str(), + path, + }; + let linked_mention_key = (skill.name.clone(), path.to_string()); + if mention_skill_paths.contains(path) + && (mention_skill_path_counts.get(path) == Some(&1) + || (mention_skill_links.contains(&linked_mention) + && mention_skill_link_counts.get(&linked_mention_key) == Some(&1))) + { + seen_paths.insert(skill.source_path.clone()); seen_names.insert(skill.name.clone()); selected.push(skill.clone()); } @@ -362,8 +437,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; } @@ -390,7 +465,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 78aa1958952..4f1f7951c02 100644 --- a/codex-rs/core-skills/src/injection_tests.rs +++ b/codex-rs/core-skills/src/injection_tests.rs @@ -1,5 +1,6 @@ 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; @@ -14,6 +15,7 @@ fn make_skill(name: &str, path: &str) -> SkillMetadata { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(test_path_buf(path).abs()), path_to_skills_md: test_path_buf(path).abs(), scope: codex_protocol::protocol::SkillScope::User, plugin_id: None, @@ -37,7 +39,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) @@ -193,6 +195,7 @@ fn collect_explicit_skill_mentions_skips_invalid_structured_and_blocks_plain_fal #[test] fn collect_explicit_skill_mentions_skips_disabled_structured_and_blocks_plain_fallback() { let alpha = make_skill("alpha-skill", "/tmp/alpha"); + let disabled = HashSet::from([alpha.source_path.clone()]); let skills = vec![alpha]; let inputs = vec![ UserInput::Text { @@ -204,7 +207,6 @@ 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 connector_counts = HashMap::new(); let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts); @@ -212,6 +214,76 @@ fn collect_explicit_skill_mentions_skips_disabled_structured_and_blocks_plain_fa assert_eq!(selected, Vec::new()); } +#[test] +fn collect_explicit_skill_mentions_counts_only_enabled_duplicate_names() { + let alpha = make_skill("demo-skill", "/tmp/alpha"); + let beta = SkillMetadata { + source_path: EnvironmentPathRef::new( + std::sync::Arc::new(LocalFileSystem::unsandboxed()), + test_path_buf("/tmp/beta").abs(), + ), + path_to_skills_md: test_path_buf("/tmp/beta").abs(), + ..alpha.clone() + }; + let disabled = HashSet::from([beta.source_path.clone()]); + let skills = vec![alpha.clone(), beta]; + let inputs = vec![UserInput::Text { + text: "please run $demo-skill".to_string(), + text_elements: Vec::new(), + }]; + let connector_counts = HashMap::new(); + + let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts); + + assert_eq!(selected, vec![alpha]); +} + +#[test] +fn collect_explicit_skill_mentions_uses_structured_name_to_disambiguate_same_path() { + let first = make_skill("alpha-skill", "/tmp/shared"); + let second = SkillMetadata { + name: "beta-skill".to_string(), + source_path: EnvironmentPathRef::new( + std::sync::Arc::new(LocalFileSystem::unsandboxed()), + first.path_to_skills_md.clone(), + ), + ..first.clone() + }; + let skills = vec![first, second.clone()]; + let inputs = vec![UserInput::Skill { + name: "beta-skill".to_string(), + path: test_path_buf("/tmp/shared"), + }]; + let connector_counts = HashMap::new(); + + let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); + + assert_eq!(selected, vec![second]); +} + +#[test] +fn collect_explicit_skill_mentions_uses_linked_name_to_disambiguate_same_path() { + let first = make_skill("alpha-skill", "/tmp/shared"); + let second = SkillMetadata { + name: "beta-skill".to_string(), + source_path: EnvironmentPathRef::new( + std::sync::Arc::new(LocalFileSystem::unsandboxed()), + first.path_to_skills_md.clone(), + ), + ..first.clone() + }; + let skills = vec![first, second.clone()]; + let inputs = vec![UserInput::Text { + text: linked_skill_mention("beta-skill", "/tmp/shared"), + text_elements: Vec::new(), + }]; + let connector_counts = HashMap::new(); + + let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); + + assert_eq!(selected, vec![second]); +} + #[test] fn collect_explicit_skill_mentions_dedupes_by_path() { let alpha = make_skill("alpha-skill", "/tmp/alpha"); @@ -228,6 +300,29 @@ fn collect_explicit_skill_mentions_dedupes_by_path() { assert_eq!(selected, vec![alpha]); } +#[test] +fn collect_explicit_skill_mentions_skips_ambiguous_linked_path() { + let alpha = make_skill("demo-skill", "/tmp/shared"); + let beta = SkillMetadata { + name: "demo-skill".to_string(), + source_path: EnvironmentPathRef::new( + std::sync::Arc::new(LocalFileSystem::unsandboxed()), + alpha.path_to_skills_md.clone(), + ), + ..alpha.clone() + }; + let skills = vec![alpha, beta]; + let inputs = vec![UserInput::Text { + text: linked_skill_mention("demo-skill", "/tmp/shared"), + text_elements: Vec::new(), + }]; + 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"); @@ -297,12 +392,12 @@ fn collect_explicit_skill_mentions_allows_explicit_path_with_connector_conflict( fn collect_explicit_skill_mentions_skips_when_linked_path_disabled() { let alpha = make_skill("demo-skill", "/tmp/alpha"); let beta = make_skill("demo-skill", "/tmp/beta"); + let disabled = HashSet::from([alpha.source_path.clone()]); let skills = vec![alpha, beta]; let inputs = vec![UserInput::Text { 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 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 4c9d0a4119e..79363818dd9 100644 --- a/codex-rs/core-skills/src/invocation_utils.rs +++ b/codex-rs/core-skills/src/invocation_utils.rs @@ -3,22 +3,25 @@ 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( +pub(crate) async 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).await; 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")); + if let Ok(Some(skill_dir)) = skill.source_path.parent().await + && let Ok(scripts_dir) = skill_dir.join(Path::new("scripts")).await + { + let scripts_dir = canonicalize_if_exists(&scripts_dir).await; by_scripts_dir.insert(scripts_dir, skill); } } @@ -26,19 +29,19 @@ pub(crate) fn build_implicit_skill_path_indexes( (by_scripts_dir, by_skill_doc_path) } -pub fn detect_implicit_skill_invocation_for_command( +pub async fn detect_implicit_skill_invocation_for_command( outcome: &SkillLoadOutcome, command: &str, - workdir: &AbsolutePathBuf, + workdir: &EnvironmentPathRef, ) -> Option { - let workdir = canonicalize_if_exists(workdir); + let workdir = canonicalize_if_exists(workdir).await; let tokens = tokenize_command(command); - if let Some(candidate) = detect_skill_script_run(outcome, tokens.as_slice(), &workdir) { + if let Some(candidate) = detect_skill_script_run(outcome, tokens.as_slice(), &workdir).await { return Some(candidate); } - detect_skill_doc_read(outcome, tokens.as_slice(), &workdir) + detect_skill_doc_read(outcome, tokens.as_slice(), &workdir).await } fn tokenize_command(command: &str) -> Vec { @@ -78,17 +81,23 @@ fn script_run_token(tokens: &[String]) -> Option<&str> { None } -fn detect_skill_script_run( +async 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).await?).await; - 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()); } } @@ -96,10 +105,10 @@ fn detect_skill_script_run( None } -fn detect_skill_doc_read( +async fn detect_skill_doc_read( outcome: &SkillLoadOutcome, tokens: &[String], - workdir: &AbsolutePathBuf, + workdir: &EnvironmentPathRef, ) -> Option { if !command_reads_file(tokens) { return None; @@ -110,7 +119,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).await?).await; if let Some(candidate) = outcome.implicit_skills_by_doc_path.get(&candidate_path) { return Some(candidate.clone()); } @@ -136,8 +145,20 @@ fn command_basename(command: &str) -> String { .to_string() } -fn canonicalize_if_exists(path: &AbsolutePathBuf) -> AbsolutePathBuf { - path.canonicalize().unwrap_or_else(|_| path.clone()) +async 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(path).await.ok() + } +} + +async fn canonicalize_if_exists(path: &EnvironmentPathRef) -> EnvironmentPathRef { + path.canonicalize(/*sandbox*/ None) + .await + .unwrap_or_else(|_| 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 f6e3883c16d..b434e10cca2 100644 --- a/codex-rs/core-skills/src/invocation_utils_tests.rs +++ b/codex-rs/core-skills/src/invocation_utils_tests.rs @@ -19,6 +19,7 @@ fn test_skill_metadata(skill_doc_path: AbsolutePathBuf) -> SkillMetadata { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local((skill_doc_path).clone()), path_to_skills_md: skill_doc_path, scope: codex_protocol::protocol::SkillScope::User, plugin_id: None, @@ -29,6 +30,10 @@ fn test_path_display(unix_path: &str) -> String { test_path_buf(unix_path).display().to_string() } +fn local_path_ref(path: AbsolutePathBuf) -> codex_exec_server::EnvironmentPathRef { + codex_exec_server::EnvironmentPathRef::local(path) +} + #[test] fn script_run_detection_matches_runner_plus_extension() { let tokens = vec![ @@ -51,10 +56,11 @@ fn script_run_detection_excludes_python_c() { assert_eq!(script_run_token(&tokens).is_some(), false); } -#[test] -fn skill_doc_read_detection_matches_absolute_path() { +#[tokio::test] +async 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(&local_path_ref(skill_doc_path.clone())).await; let skill = test_skill_metadata(skill_doc_path); let outcome = SkillLoadOutcome { implicit_skills_by_scripts_dir: Arc::new(HashMap::new()), @@ -68,7 +74,12 @@ 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, + &local_path_ref(test_path_buf("/tmp").abs()), + ) + .await; assert_eq!( found.map(|value| value.name), @@ -76,10 +87,42 @@ fn skill_doc_read_detection_matches_absolute_path() { ); } -#[test] -fn skill_script_run_detection_matches_relative_path_from_skill_root() { +#[tokio::test] +async fn skill_doc_read_detection_matches_parent_relative_path() { + let skill_doc_path = test_path_buf("/tmp/project/.agents/skills/test-skill/SKILL.md").abs(); + let normalized_skill_doc_path = + canonicalize_if_exists(&local_path_ref(skill_doc_path.clone())).await; + let skill = test_skill_metadata(skill_doc_path); + let outcome = SkillLoadOutcome { + implicit_skills_by_scripts_dir: Arc::new(HashMap::new()), + implicit_skills_by_doc_path: Arc::new(HashMap::from([(normalized_skill_doc_path, skill)])), + ..Default::default() + }; + let tokens = vec![ + "cat".to_string(), + "../.agents/skills/test-skill/SKILL.md".to_string(), + ]; + + let found = detect_skill_doc_read( + &outcome, + &tokens, + &local_path_ref(test_path_buf("/tmp/project/nested").abs()), + ) + .await; + + assert_eq!( + found.map(|value| value.name), + Some("test-skill".to_string()) + ); +} + +#[tokio::test] +async 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(&local_path_ref( + test_path_buf("/tmp/skill-test/scripts").abs(), + )) + .await; let skill = test_skill_metadata(skill_doc_path); let outcome = SkillLoadOutcome { implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])), @@ -91,7 +134,12 @@ 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, + &local_path_ref(test_path_buf("/tmp/skill-test").abs()), + ) + .await; assert_eq!( found.map(|value| value.name), @@ -99,10 +147,13 @@ fn skill_script_run_detection_matches_relative_path_from_skill_root() { ); } -#[test] -fn skill_script_run_detection_matches_absolute_path_from_any_workdir() { +#[tokio::test] +async 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(&local_path_ref( + test_path_buf("/tmp/skill-test/scripts").abs(), + )) + .await; let skill = test_skill_metadata(skill_doc_path); let outcome = SkillLoadOutcome { implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])), @@ -114,7 +165,12 @@ 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, + &local_path_ref(test_path_buf("/tmp/other").abs()), + ) + .await; 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 8663737ee56..8c8a98c44fe 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -1,6 +1,5 @@ use crate::model::SkillDependencies; use crate::model::SkillError; -use crate::model::SkillFileSystemsByPath; use crate::model::SkillInterface; use crate::model::SkillLoadOutcome; use crate::model::SkillMetadata; @@ -13,6 +12,7 @@ use codex_config::ConfigLayerStackOrdering; use codex_config::default_project_root_markers; use codex_config::merge_toml_values; use codex_config::project_root_markers_from_config; +use codex_exec_server::EnvironmentPathRef; use codex_exec_server::ExecutorFileSystem; use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; @@ -163,52 +163,56 @@ where I: IntoIterator, { let mut outcome = SkillLoadOutcome::default(); - let mut skill_roots: Vec = Vec::new(); - let mut skill_root_by_path: HashMap = HashMap::new(); - let mut file_systems_by_skill_path: HashMap> = - HashMap::new(); + let mut skill_roots: Vec = Vec::new(); + let mut skill_root_by_path: HashMap = HashMap::new(); for root in roots { - let root_path = canonicalize_for_skill_identity(&root.path); let fs = root.file_system; + let root_path_ref = + canonicalize_for_skill_identity(&EnvironmentPathRef::new(Arc::clone(&fs), root.path)) + .await; + let plugin_root = match root.plugin_root { + Some(plugin_root) => Some( + canonicalize_for_skill_identity(&EnvironmentPathRef::new( + Arc::clone(&fs), + plugin_root, + )) + .await, + ), + None => None, + }; let skills_before_root = outcome.skills.len(); discover_skills_under_root( - fs.as_ref(), - &root_path, + &root_path_ref, root.scope, root.plugin_id.as_deref(), - root.plugin_root.as_ref(), + plugin_root.as_ref(), &mut outcome, ) .await; for skill in &outcome.skills[skills_before_root..] { - if !skill_roots.contains(&root_path) { - skill_roots.push(root_path.clone()); + if !skill_roots.contains(&root_path_ref) { + skill_roots.push(root_path_ref.clone()); } skill_root_by_path - .entry(skill.path_to_skills_md.clone()) - .or_insert_with(|| root_path.clone()); - file_systems_by_skill_path - .entry(skill.path_to_skills_md.clone()) - .or_insert_with(|| Arc::clone(&fs)); + .entry(skill.source_path.clone()) + .or_insert_with(|| root_path_ref.clone()); } } - let mut seen: HashSet = HashSet::new(); + let mut seen: HashSet = HashSet::new(); outcome .skills - .retain(|skill| seen.insert(skill.path_to_skills_md.clone())); - let retained_skill_paths: HashSet = outcome + .retain(|skill| seen.insert(skill.source_path.clone())); + let retained_skill_paths: HashSet = outcome .skills .iter() - .map(|skill| skill.path_to_skills_md.clone()) + .map(|skill| skill.source_path.clone()) .collect(); skill_root_by_path.retain(|path, _| retained_skill_paths.contains(path)); - let used_roots: HashSet = skill_root_by_path.values().cloned().collect(); + let used_roots: HashSet = skill_root_by_path.values().cloned().collect(); skill_roots.retain(|root| used_roots.contains(root)); - file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(path)); outcome.skill_roots = skill_roots; outcome.skill_root_by_path = Arc::new(skill_root_by_path); - outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path); fn scope_rank(scope: SkillScope) -> u8 { // Higher-priority scopes first (matches root scan order for dedupe). @@ -465,31 +469,44 @@ fn dirs_between_project_root_and_cwd( } fn dedupe_skill_roots_by_path(roots: &mut Vec) { - let mut seen: HashSet = HashSet::new(); - roots.retain(|root| seen.insert(root.path.clone())); + let mut seen: HashSet = HashSet::new(); + roots.retain(|root| { + seen.insert(EnvironmentPathRef::new( + Arc::clone(&root.file_system), + root.path.clone(), + )) + }); } -fn canonicalize_for_skill_identity(path: &AbsolutePathBuf) -> AbsolutePathBuf { - path.canonicalize().unwrap_or_else(|_| path.clone()) +async fn canonicalize_for_skill_identity(path: &EnvironmentPathRef) -> EnvironmentPathRef { + path.canonicalize(/*sandbox*/ None) + .await + .unwrap_or_else(|_| path.clone()) } async fn discover_skills_under_root( - fs: &dyn ExecutorFileSystem, - root: &AbsolutePathBuf, + root: &EnvironmentPathRef, scope: SkillScope, plugin_id: Option<&str>, - plugin_root: Option<&AbsolutePathBuf>, + plugin_root: Option<&EnvironmentPathRef>, outcome: &mut SkillLoadOutcome, ) { - let root = canonicalize_for_skill_identity(root); - let plugin_root = plugin_root.map(canonicalize_for_skill_identity); + let fs = root.file_system(); + let root = canonicalize_for_skill_identity(root).await; + let plugin_root = match plugin_root { + Some(plugin_root) => Some(canonicalize_for_skill_identity(plugin_root).await), + None => None, + }; - match fs.get_metadata(&root, /*sandbox*/ None).await { + match fs.get_metadata(root.path(), /*sandbox*/ None).await { Ok(metadata) if metadata.is_directory => {} Ok(_) => return, Err(err) if err.kind() == io::ErrorKind::NotFound => return, Err(err) => { - error!("failed to stat skills root {}: {err:#}", root.display()); + error!( + "failed to stat skills root {}: {err:#}", + root.path().display() + ); return; } } @@ -520,9 +537,9 @@ async fn discover_skills_under_root( ); let mut visited_dirs: HashSet = HashSet::new(); - visited_dirs.insert(root.clone()); + visited_dirs.insert(root.path().clone()); - let mut queue: VecDeque<(AbsolutePathBuf, usize)> = VecDeque::from([(root.clone(), 0)]); + let mut queue: VecDeque<(AbsolutePathBuf, usize)> = VecDeque::from([(root.path().clone(), 0)]); let mut truncated_by_dir_limit = false; while let Some((dir, depth)) = queue.pop_front() { @@ -555,7 +572,11 @@ async fn discover_skills_under_root( } match fs.read_directory(&path, /*sandbox*/ None).await { Ok(_) => { - let resolved_dir = canonicalize_for_skill_identity(&path); + let resolved_dir = + canonicalize_for_skill_identity(&root.with_path(path.clone())) + .await + .path() + .clone(); enqueue_dir( &mut queue, &mut visited_dirs, @@ -580,7 +601,10 @@ async fn discover_skills_under_root( } if metadata.is_directory { - let resolved_dir = canonicalize_for_skill_identity(&path); + let resolved_dir = canonicalize_for_skill_identity(&root.with_path(path.clone())) + .await + .path() + .clone(); enqueue_dir( &mut queue, &mut visited_dirs, @@ -592,7 +616,14 @@ async fn discover_skills_under_root( } if metadata.is_file && file_name == SKILLS_FILENAME { - match parse_skill_file(fs, &path, scope, plugin_id, plugin_root.as_ref()).await { + match parse_skill_file( + &root.with_path(path.clone()), + scope, + plugin_id, + plugin_root.as_ref(), + ) + .await + { Ok(skill) => { outcome.skills.push(skill); } @@ -613,20 +644,20 @@ async fn discover_skills_under_root( tracing::warn!( "skills scan truncated after {} directories (root: {})", MAX_SKILLS_DIRS_PER_ROOT, - root.display() + root.path().display() ); } } async fn parse_skill_file( - fs: &dyn ExecutorFileSystem, - path: &AbsolutePathBuf, + path: &EnvironmentPathRef, scope: SkillScope, plugin_id: Option<&str>, - plugin_root: Option<&AbsolutePathBuf>, + plugin_root: Option<&EnvironmentPathRef>, ) -> Result { + let fs = path.file_system(); let contents = fs - .read_file_text(path, /*sandbox*/ None) + .read_file_text(path.path(), /*sandbox*/ None) .await .map_err(SkillParseError::Read)?; @@ -640,8 +671,8 @@ async fn parse_skill_file( .as_deref() .map(sanitize_single_line) .filter(|value| !value.is_empty()) - .unwrap_or_else(|| default_skill_name(path)); - let name = namespaced_skill_name(fs, path, &base_name).await; + .unwrap_or_else(|| default_skill_name(path.path())); + let name = namespaced_skill_name(fs.as_ref(), path.path(), &base_name).await; let description = parsed .description .as_deref() @@ -657,7 +688,12 @@ async fn parse_skill_file( interface, dependencies, policy, - } = load_skill_metadata(fs, path, plugin_root).await; + } = load_skill_metadata( + fs.as_ref(), + path.path(), + plugin_root.map(EnvironmentPathRef::path), + ) + .await; validate_len(&name, MAX_NAME_LEN, "name")?; validate_len(&description, MAX_DESCRIPTION_LEN, "description")?; @@ -669,7 +705,7 @@ async fn parse_skill_file( )?; } - let resolved_path = canonicalize_for_skill_identity(path); + let resolved_path = canonicalize_for_skill_identity(path).await; Ok(SkillMetadata { name, @@ -678,7 +714,8 @@ async fn parse_skill_file( interface, dependencies, policy, - path_to_skills_md: resolved_path, + path_to_skills_md: resolved_path.path().clone(), + source_path: resolved_path, scope, plugin_id: plugin_id.map(str::to_string), }) diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index 69e357d93db..aa3cf2e8b8a 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -4,7 +4,9 @@ use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; +use codex_exec_server::ExecutorFileSystem; use codex_exec_server::LOCAL_FS; +use codex_exec_server::LocalFileSystem; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -144,6 +146,16 @@ fn normalized(path: &Path) -> AbsolutePathBuf { .abs() } +fn local_skill_root(path: AbsolutePathBuf, scope: SkillScope) -> SkillRoot { + SkillRoot { + path, + scope, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: None, + plugin_root: None, + } +} + #[tokio::test] async fn skill_roots_from_layer_stack_maps_user_to_user_and_system_cache_and_system_to_admin() -> anyhow::Result<()> { @@ -330,6 +342,7 @@ async fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<() interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -468,6 +481,7 @@ async fn loads_skill_dependencies_metadata_from_yaml() { ], }), policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -524,6 +538,9 @@ interface: }), dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + skill_path.as_path() + )), path_to_skills_md: normalized(skill_path.as_path()), scope: SkillScope::User, plugin_id: None, @@ -678,6 +695,7 @@ async fn accepts_icon_paths_under_assets_dir() { }), dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -719,6 +737,7 @@ async fn ignores_invalid_brand_color() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -773,6 +792,7 @@ async fn ignores_default_prompt_over_max_length() { }), dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -815,6 +835,7 @@ async fn drops_interface_when_icons_are_invalid() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -876,6 +897,7 @@ interface: }), dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: Some("twilio-developer-kit@test".to_string()), @@ -925,6 +947,7 @@ interface: interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: Some("twilio-developer-kit@test".to_string()), @@ -970,6 +993,9 @@ async fn loads_skills_via_symlinked_subdir_for_user_scope() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &shared_skill_path + )), path_to_skills_md: normalized(&shared_skill_path), scope: SkillScope::User, plugin_id: None, @@ -1030,6 +1056,7 @@ async fn does_not_loop_on_symlink_cycle_for_user_scope() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -1048,14 +1075,9 @@ async fn loads_skills_via_symlinked_subdir_for_admin_scope() { fs::create_dir_all(admin_root.path()).unwrap(); symlink_dir(shared.path(), &admin_root.path().join("shared")); - let outcome = load_skills_from_roots([SkillRoot { - path: admin_root.path().abs(), - scope: SkillScope::Admin, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_root: None, - }]) - .await; + let outcome = + load_skills_from_roots([local_skill_root(admin_root.path().abs(), SkillScope::Admin)]) + .await; assert!( outcome.errors.is_empty(), @@ -1071,6 +1093,9 @@ async fn loads_skills_via_symlinked_subdir_for_admin_scope() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &shared_skill_path + )), path_to_skills_md: normalized(&shared_skill_path), scope: SkillScope::Admin, plugin_id: None, @@ -1111,6 +1136,9 @@ async fn loads_skills_via_symlinked_subdir_for_repo_scope() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &linked_skill_path + )), path_to_skills_md: normalized(&linked_skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1130,14 +1158,8 @@ async fn system_scope_ignores_symlinked_subdir() { fs::create_dir_all(&system_root).unwrap(); symlink_dir(shared.path(), &system_root.join("shared")); - let outcome = load_skills_from_roots([SkillRoot { - path: system_root.abs(), - scope: SkillScope::System, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_root: None, - }]) - .await; + let outcome = + load_skills_from_roots([local_skill_root(system_root.abs(), SkillScope::System)]).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1164,14 +1186,8 @@ async fn respects_max_scan_depth_for_user_scope() { ); let skills_root = codex_home.path().join("skills"); - let outcome = load_skills_from_roots([SkillRoot { - path: skills_root.abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_root: None, - }]) - .await; + let outcome = + load_skills_from_roots([local_skill_root(skills_root.abs(), SkillScope::User)]).await; assert!( outcome.errors.is_empty(), @@ -1187,6 +1203,9 @@ async fn respects_max_scan_depth_for_user_scope() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &within_depth_path + )), path_to_skills_md: normalized(&within_depth_path), scope: SkillScope::User, plugin_id: None, @@ -1215,6 +1234,7 @@ async fn loads_valid_skill() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -1248,6 +1268,7 @@ async fn falls_back_to_directory_name_when_skill_name_is_missing() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -1294,6 +1315,7 @@ async fn namespaces_plugin_skills_using_plugin_name() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: Some("sample@test".to_string()), @@ -1326,6 +1348,7 @@ async fn loads_short_description_from_metadata() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -1439,6 +1462,7 @@ async fn loads_skills_from_repo_root() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1475,6 +1499,7 @@ async fn loads_skills_from_agents_dir_without_codex_dir() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1529,6 +1554,9 @@ async fn loads_skills_from_all_codex_dirs_under_project_root() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &nested_skill_path + )), path_to_skills_md: normalized(&nested_skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1540,6 +1568,9 @@ async fn loads_skills_from_all_codex_dirs_under_project_root() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &root_skill_path + )), path_to_skills_md: normalized(&root_skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1580,6 +1611,7 @@ async fn loads_skills_from_codex_dir_when_not_git_repo() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1594,20 +1626,8 @@ async fn deduplicates_by_path_preferring_first_root() { let skill_path = write_skill_at(root.path(), "dupe", "dupe-skill", "from repo"); let outcome = load_skills_from_roots([ - SkillRoot { - path: root.path().abs(), - scope: SkillScope::Repo, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_root: None, - }, - SkillRoot { - path: root.path().abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_root: None, - }, + local_skill_root(root.path().abs(), SkillScope::Repo), + local_skill_root(root.path().abs(), SkillScope::User), ]) .await; @@ -1625,6 +1645,7 @@ async fn deduplicates_by_path_preferring_first_root() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1632,6 +1653,71 @@ async fn deduplicates_by_path_preferring_first_root() { ); } +#[tokio::test] +async fn preserves_same_absolute_skill_path_from_distinct_file_systems() { + let root = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill_at(root.path(), "dupe", "dupe-skill", "from repo"); + let shared_path = root.path().abs(); + let first_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); + let second_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); + + let outcome = load_skills_from_roots([ + SkillRoot { + path: shared_path.clone(), + scope: SkillScope::Repo, + file_system: first_file_system, + plugin_id: None, + plugin_root: None, + }, + SkillRoot { + path: shared_path, + scope: SkillScope::Repo, + file_system: second_file_system, + plugin_id: None, + plugin_root: None, + }, + ]) + .await; + + assert_eq!( + outcome + .skills + .iter() + .map(|skill| skill.path_to_skills_md.clone()) + .collect::>(), + vec![normalized(&skill_path), normalized(&skill_path)] + ); + assert_ne!(outcome.skills[0].source_path, outcome.skills[1].source_path); +} + +#[test] +fn dedupe_skill_roots_preserves_same_path_from_distinct_file_systems() { + let root = tempfile::tempdir().expect("tempdir"); + let shared_path = root.path().abs(); + let first_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); + let second_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); + let mut roots = vec![ + SkillRoot { + path: shared_path.clone(), + scope: SkillScope::Repo, + file_system: first_file_system, + plugin_id: None, + plugin_root: None, + }, + SkillRoot { + path: shared_path, + scope: SkillScope::Repo, + file_system: second_file_system, + plugin_id: None, + plugin_root: None, + }, + ]; + + dedupe_skill_roots_by_path(&mut roots); + + assert_eq!(roots.len(), 2); +} + #[tokio::test] async fn keeps_duplicate_names_from_repo_and_user() { let codex_home = tempfile::tempdir().expect("tempdir"); @@ -1667,6 +1753,9 @@ async fn keeps_duplicate_names_from_repo_and_user() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &repo_skill_path + )), path_to_skills_md: normalized(&repo_skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1678,6 +1767,9 @@ async fn keeps_duplicate_names_from_repo_and_user() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &user_skill_path + )), path_to_skills_md: normalized(&user_skill_path), scope: SkillScope::User, plugin_id: None, @@ -1741,6 +1833,7 @@ async fn keeps_duplicate_names_from_nested_codex_dirs() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local((first_path).clone()), path_to_skills_md: first_path, scope: SkillScope::Repo, plugin_id: None, @@ -1752,6 +1845,7 @@ async fn keeps_duplicate_names_from_nested_codex_dirs() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local((second_path).clone()), path_to_skills_md: second_path, scope: SkillScope::Repo, plugin_id: None, @@ -1824,6 +1918,7 @@ async fn loads_skills_when_cwd_is_file_in_repo() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1883,6 +1978,7 @@ async fn loads_skills_from_system_cache_when_present() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::System, plugin_id: None, diff --git a/codex-rs/core-skills/src/manager.rs b/codex-rs/core-skills/src/manager.rs index 185eaa93206..369f21087d0 100644 --- a/codex-rs/core-skills/src/manager.rs +++ b/codex-rs/core-skills/src/manager.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use std::sync::RwLock; use codex_config::ConfigLayerStack; +use codex_exec_server::EnvironmentPathRef; use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; @@ -52,7 +53,7 @@ pub struct SkillsManager { codex_home: AbsolutePathBuf, restriction_product: Option, extra_roots: RwLock>, - cache_by_cwd: RwLock>, + cache_by_cwd: RwLock>, cache_by_config: RwLock>, } @@ -146,10 +147,12 @@ impl SkillsManager { force_reload: bool, fs: Option>, ) -> SkillLoadOutcome { - let use_cwd_cache = fs.is_some(); - if use_cwd_cache - && !force_reload - && let Some(outcome) = self.cached_outcome_for_cwd(&input.cwd) + let cwd_path_ref = fs + .as_ref() + .map(|fs| EnvironmentPathRef::new(Arc::clone(fs), input.cwd.clone())); + if !force_reload + && let Some(cwd_path_ref) = cwd_path_ref.as_ref() + && let Some(outcome) = self.cached_outcome_for_cwd(cwd_path_ref) { return outcome; } @@ -167,12 +170,12 @@ impl SkillsManager { } let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); let outcome = self.build_skill_outcome(roots, &skill_config_rules).await; - if use_cwd_cache { + if let Some(cwd_path_ref) = cwd_path_ref { let mut cache = self .cache_by_cwd .write() .unwrap_or_else(std::sync::PoisonError::into_inner); - cache.insert(input.cwd.clone(), outcome.clone()); + cache.insert(cwd_path_ref, outcome.clone()); } outcome } @@ -187,7 +190,7 @@ impl SkillsManager { self.restriction_product, ); let disabled_paths = resolve_disabled_skill_paths(&outcome.skills, skill_config_rules); - finalize_skill_outcome(outcome, disabled_paths) + finalize_skill_outcome(outcome, disabled_paths).await } pub fn clear_cache(&self) { @@ -213,7 +216,7 @@ impl SkillsManager { info!("skills cache cleared ({cleared} entries)"); } - fn cached_outcome_for_cwd(&self, cwd: &AbsolutePathBuf) -> Option { + fn cached_outcome_for_cwd(&self, cwd: &EnvironmentPathRef) -> Option { match self.cache_by_cwd.read() { Ok(cache) => cache.get(cwd).cloned(), Err(err) => err.into_inner().get(cwd).cloned(), @@ -240,7 +243,7 @@ impl SkillsManager { #[derive(Debug, Clone, PartialEq, Eq, Hash)] struct ConfigSkillsCacheKey { - roots: Vec<(AbsolutePathBuf, u8, Option)>, + roots: Vec<(EnvironmentPathRef, u8, Option)>, skill_config_rules: SkillConfigRules, } @@ -280,20 +283,24 @@ fn config_skills_cache_key( SkillScope::System => 2, SkillScope::Admin => 3, }; - (root.path.clone(), scope_rank, root.plugin_id.clone()) + ( + EnvironmentPathRef::new(Arc::clone(&root.file_system), root.path.clone()), + scope_rank, + root.plugin_id.clone(), + ) }) .collect(), skill_config_rules: skill_config_rules.clone(), } } -fn finalize_skill_outcome( +async 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) = - build_implicit_skill_path_indexes(outcome.allowed_skills_for_implicit_invocation()); + build_implicit_skill_path_indexes(outcome.allowed_skills_for_implicit_invocation()).await; outcome.implicit_skills_by_scripts_dir = Arc::new(by_scripts_dir); outcome.implicit_skills_by_doc_path = Arc::new(by_doc_path); outcome diff --git a/codex-rs/core-skills/src/manager_tests.rs b/codex-rs/core-skills/src/manager_tests.rs index 713b94d7b1c..fcd5e9c828e 100644 --- a/codex-rs/core-skills/src/manager_tests.rs +++ b/codex-rs/core-skills/src/manager_tests.rs @@ -7,7 +7,10 @@ use codex_config::CONFIG_TOML_FILE; use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirementsToml; +use codex_exec_server::EnvironmentPathRef; +use codex_exec_server::ExecutorFileSystem; use codex_exec_server::LOCAL_FS; +use codex_exec_server::LocalFileSystem; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::test_support::PathBufExt; use codex_utils_absolute_path::test_support::PathExt; @@ -71,6 +74,10 @@ fn plugin_skill_root_for_skill_path(skill_path: &Path, plugin_id: &str) -> Plugi } fn test_skill(name: &str, path: PathBuf) -> SkillMetadata { + let path = path + .abs() + .canonicalize() + .expect("skill path should canonicalize"); SkillMetadata { name: name.to_string(), description: "test".to_string(), @@ -78,10 +85,8 @@ fn test_skill(name: &str, path: PathBuf) -> SkillMetadata { interface: None, dependencies: None, policy: None, - path_to_skills_md: path - .abs() - .canonicalize() - .expect("skill path should canonicalize"), + source_path: EnvironmentPathRef::local(path.clone()), + path_to_skills_md: path, scope: SkillScope::User, plugin_id: None, } @@ -221,6 +226,51 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() { assert_eq!(outcome2.skills, outcome1.skills); } +#[tokio::test] +async fn skills_for_config_keeps_cache_entries_bound_to_file_system() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let config_layer_stack = config_stack(&codex_home, ""); + let repo_skill_dir = cwd.path().join(".agents/skills/repo"); + fs::create_dir_all(&repo_skill_dir).expect("create repo skill dir"); + fs::write( + repo_skill_dir.join("SKILL.md"), + "---\nname: repo-skill\ndescription: from repo root\n---\n\n# Body\n", + ) + .expect("write repo skill"); + let skills_input = SkillsLoadInput::new( + cwd.path().abs(), + Vec::new(), + config_layer_stack.clone(), + bundled_skills_enabled_from_stack(&config_layer_stack), + ); + let skills_manager = SkillsManager::new( + codex_home.path().abs(), + /*bundled_skills_enabled*/ true, + ); + let first_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); + let second_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); + + let first = skills_manager + .skills_for_config(&skills_input, Some(first_file_system)) + .await; + let second = skills_manager + .skills_for_config(&skills_input, Some(second_file_system)) + .await; + let first_skill = first + .skills + .iter() + .find(|skill| skill.name == "repo-skill") + .expect("repo skill should load"); + let second_skill = second + .skills + .iter() + .find(|skill| skill.name == "repo-skill") + .expect("repo skill should load"); + + assert_ne!(first_skill.source_path, second_skill.source_path); +} + #[tokio::test] async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() { let codex_home = tempfile::tempdir().expect("tempdir"); @@ -382,7 +432,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() @@ -616,6 +666,59 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() { ); } +#[tokio::test] +async fn skills_for_cwd_keeps_cache_entries_bound_to_file_system() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let config_layer_stack = config_stack(&codex_home, ""); + let repo_skill_dir = cwd.path().join(".agents/skills/repo"); + fs::create_dir_all(&repo_skill_dir).expect("create repo skill dir"); + fs::write( + repo_skill_dir.join("SKILL.md"), + "---\nname: repo-skill\ndescription: from repo root\n---\n\n# Body\n", + ) + .expect("write repo skill"); + let skills_input = SkillsLoadInput::new( + cwd.path().abs(), + Vec::new(), + config_layer_stack.clone(), + bundled_skills_enabled_from_stack(&config_layer_stack), + ); + let skills_manager = SkillsManager::new( + codex_home.path().abs(), + /*bundled_skills_enabled*/ true, + ); + let first_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); + let second_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); + + let first = skills_manager + .skills_for_cwd( + &skills_input, + /*force_reload*/ false, + Some(first_file_system), + ) + .await; + let second = skills_manager + .skills_for_cwd( + &skills_input, + /*force_reload*/ false, + Some(second_file_system), + ) + .await; + let first_skill = first + .skills + .iter() + .find(|skill| skill.name == "repo-skill") + .expect("repo skill should load"); + let second_skill = second + .skills + .iter() + .find(|skill| skill.name == "repo-skill") + .expect("repo skill should load"); + + assert_ne!(first_skill.source_path, second_skill.source_path); +} + #[cfg_attr(windows, ignore)] #[test] fn disabled_paths_for_skills_allows_session_flags_to_override_user_layer() { @@ -682,10 +785,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"), + )]) ); } @@ -715,10 +820,82 @@ 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"), + )]) + ); +} + +#[cfg_attr(windows, ignore)] +#[test] +fn path_rules_apply_to_every_matching_source_path() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_path = write_demo_skill(&tempdir); + let first = test_skill("demo-skill", skill_path.clone()); + let second = SkillMetadata { + source_path: EnvironmentPathRef::new( + Arc::new(LocalFileSystem::unsandboxed()), + first.path_to_skills_md.clone(), + ), + ..first.clone() + }; + let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml")) + .expect("user config path should be absolute"); + let disabled_layer = ConfigLayerEntry::new( + ConfigLayerSource::User { + file: user_file.clone(), + profile: None, + }, + toml::from_str(&path_toggle_config(&skill_path, /*enabled*/ false)) + .expect("user layer toml"), + ); + let disabled_stack = ConfigLayerStack::new( + vec![disabled_layer], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + let disabled_paths = resolve_disabled_skill_paths( + &[first.clone(), second.clone()], + &skill_config_rules_from_stack(&disabled_stack), + ); + + assert_eq!( + disabled_paths, + HashSet::from([first.source_path.clone(), second.source_path.clone()]) + ); + + let reenabled_layer = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::from_str(&path_toggle_config(&skill_path, /*enabled*/ true)) + .expect("session layer toml"), + ); + let reenabled_stack = ConfigLayerStack::new( + vec![ + ConfigLayerEntry::new( + ConfigLayerSource::User { + file: user_file, + profile: None, + }, + toml::from_str(&path_toggle_config(&skill_path, /*enabled*/ false)) + .expect("user layer toml"), + ), + reenabled_layer, + ], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + + assert_eq!( + resolve_disabled_skill_paths( + &[first, second], + &skill_config_rules_from_stack(&reenabled_stack), + ), + HashSet::new() ); } diff --git a/codex-rs/core-skills/src/mention_counts.rs b/codex-rs/core-skills/src/mention_counts.rs index b7482ca36ec..2eabc07c6fe 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 fc8e9f5917d..2d68283c63f 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -1,12 +1,10 @@ -use std::collections::HashMap; -use std::collections::HashSet; -use std::fmt; -use std::sync::Arc; - -use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::EnvironmentPathRef; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; +use std::collections::HashMap; +use std::collections::HashSet; +use std::sync::Arc; #[derive(Debug, Clone, PartialEq)] pub struct SkillMetadata { @@ -16,8 +14,10 @@ pub struct SkillMetadata { pub interface: Option, pub dependencies: Option, pub policy: Option, - /// Path to the SKILLS.md file that declares this skill. + /// Best-effort resolved raw path used for display, wire payloads, and config selectors. pub path_to_skills_md: AbsolutePathBuf, + /// Same resolved path bound to the filesystem that loaded this skill. + pub source_path: EnvironmentPathRef, pub scope: SkillScope, pub plugin_id: Option, } @@ -89,17 +89,16 @@ pub struct SkillError { pub struct SkillLoadOutcome { pub skills: Vec, pub errors: Vec, - pub disabled_paths: HashSet, - pub(crate) skill_roots: Vec, - pub(crate) skill_root_by_path: Arc>, - pub(crate) file_systems_by_skill_path: SkillFileSystemsByPath, - pub(crate) implicit_skills_by_scripts_dir: Arc>, - pub(crate) implicit_skills_by_doc_path: Arc>, + 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>, } 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 { @@ -119,49 +118,6 @@ impl SkillLoadOutcome { .iter() .map(|skill| (skill, self.is_skill_enabled(skill))) } - - pub(crate) fn file_system_for_skill( - &self, - skill: &SkillMetadata, - ) -> Option> { - self.file_systems_by_skill_path - .get(&skill.path_to_skills_md) - } -} - -#[derive(Clone, Default)] -pub(crate) struct SkillFileSystemsByPath { - values: Arc>>, -} - -impl SkillFileSystemsByPath { - pub(crate) fn new(values: HashMap>) -> Self { - Self { - values: Arc::new(values), - } - } - - fn get(&self, path: &AbsolutePathBuf) -> Option> { - self.values.get(path).map(Arc::clone) - } - - fn retain_paths(&mut self, paths: &HashSet) { - self.values = Arc::new( - self.values - .iter() - .filter(|(path, _)| paths.contains(*path)) - .map(|(path, fs)| (path.clone(), Arc::clone(fs))) - .collect(), - ); - } -} - -impl fmt::Debug for SkillFileSystemsByPath { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("SkillFileSystemsByPath") - .field("len", &self.values.len()) - .finish() - } } pub fn filter_skill_load_outcome_for_product( @@ -171,14 +127,11 @@ pub fn filter_skill_load_outcome_for_product( outcome .skills .retain(|skill| skill.matches_product_restriction_for_product(restriction_product)); - let retained_paths: HashSet = outcome + let retained_paths: HashSet = outcome .skills .iter() - .map(|skill| skill.path_to_skills_md.clone()) + .map(|skill| skill.source_path.clone()) .collect(); - outcome - .file_systems_by_skill_path - .retain_paths(&retained_paths); outcome.skill_root_by_path = Arc::new( outcome .skill_root_by_path @@ -187,7 +140,7 @@ pub fn filter_skill_load_outcome_for_product( .map(|(path, root)| (path.clone(), root.clone())) .collect(), ); - let retained_roots: HashSet = + let retained_roots: HashSet = outcome.skill_root_by_path.values().cloned().collect(); outcome .skill_roots @@ -210,3 +163,39 @@ pub fn filter_skill_load_outcome_for_product( ); outcome } + +#[cfg(test)] +mod tests { + use std::path::Path; + + use codex_exec_server::EnvironmentPathRef; + use codex_utils_absolute_path::test_support::PathBufExt; + use pretty_assertions::assert_eq; + + #[tokio::test] + async fn environment_path_ref_join_matches_absolute_path_buf() { + let root_path = std::env::temp_dir().join("skills"); + let path_ref = EnvironmentPathRef::local(root_path.abs()); + + assert_eq!( + path_ref + .join(Path::new("demo/SKILL.md")) + .await + .expect("join") + .path() + .clone(), + root_path.join("demo/SKILL.md").abs() + ); + assert_eq!( + path_ref + .join(std::env::temp_dir().join("SKILL.md").as_path()) + .await + .expect("join") + .path() + .clone(), + path_ref + .path() + .join(std::env::temp_dir().join("SKILL.md").as_path()) + ); + } +} diff --git a/codex-rs/core-skills/src/render.rs b/codex-rs/core-skills/src/render.rs index 28617fb6c42..2da51d94e6b 100644 --- a/codex-rs/core-skills/src/render.rs +++ b/codex-rs/core-skills/src/render.rs @@ -5,6 +5,7 @@ use std::path::Path; use crate::model::SkillLoadOutcome; use crate::model::SkillMetadata; +use codex_exec_server::EnvironmentPathRef; use codex_otel::SessionTelemetry; use codex_otel::THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC; use codex_otel::THREAD_SKILLS_ENABLED_TOTAL_METRIC; @@ -665,8 +666,8 @@ struct SkillPathAliases { struct AliasPlan { aliases: SkillPathAliases, - root_aliases: HashMap, - alias_root_by_path: HashMap, + root_aliases: HashMap, + alias_root_by_path: HashMap, table_cost: usize, } @@ -677,7 +678,7 @@ fn build_alias_plan( ) -> Option { let skill_paths = skills .iter() - .map(|skill| skill.path_to_skills_md.clone()) + .map(|skill| skill.source_path.clone()) .collect::>(); let skill_root_by_path = outcome .skill_root_by_path @@ -736,9 +737,9 @@ fn build_alias_plan( } fn ordered_alias_roots( - used_roots: &[AbsolutePathBuf], - alias_root_by_skill_root: &HashMap, -) -> Option> { + used_roots: &[EnvironmentPathRef], + alias_root_by_skill_root: &HashMap, +) -> Option> { let mut seen = HashSet::new(); let mut alias_roots = Vec::new(); for root in used_roots { @@ -751,10 +752,10 @@ fn ordered_alias_roots( } fn alias_root_for_skill_root( - root: &AbsolutePathBuf, + root: &EnvironmentPathRef, plugin_version_skill_counts: &HashMap, -) -> AbsolutePathBuf { - let Some(plugin_version_base) = plugin_version_base(root.as_path()) else { +) -> EnvironmentPathRef { + let Some(plugin_version_base) = plugin_version_base(root.path().as_path()) else { return root.clone(); }; let skill_count = plugin_version_skill_counts @@ -764,16 +765,18 @@ fn alias_root_for_skill_root( if skill_count > 1 { root.clone() } else { - plugin_marketplace_base(root.as_path()).unwrap_or_else(|| root.clone()) + plugin_marketplace_base(root.path().as_path()) + .map(|path| root.with_path(path)) + .unwrap_or_else(|| root.clone()) } } fn plugin_version_skill_counts_for_skill_roots<'a>( - skill_roots: impl Iterator, + skill_roots: impl Iterator, ) -> HashMap { let mut counts = HashMap::new(); for root in skill_roots { - if let Some(plugin_version_base) = plugin_version_base(root.as_path()) { + if let Some(plugin_version_base) = plugin_version_base(root.path().as_path()) { let count = counts.entry(plugin_version_base).or_insert(0usize); *count = count.saturating_add(1); } @@ -793,12 +796,12 @@ fn aliased_metadata_overhead_cost( .saturating_sub(budget.cost(&absolute_body)) } -fn build_skill_root_lines(roots: &[AbsolutePathBuf]) -> Vec { +fn build_skill_root_lines(roots: &[EnvironmentPathRef]) -> Vec { roots .iter() .enumerate() .map(|(index, root)| { - let root_str = root.to_string_lossy().replace('\\', "/"); + let root_str = root.path().to_string_lossy().replace('\\', "/"); format!("- `r{index}` = `{root_str}`") }) .collect() @@ -840,12 +843,12 @@ fn render_skill_path_with_aliases(skill: &SkillMetadata, plan: &AliasPlan) -> St } fn outcome_relative_skill_path(skill: &SkillMetadata, plan: &AliasPlan) -> Option { - let alias_root = plan.alias_root_by_path.get(&skill.path_to_skills_md)?; + let alias_root = plan.alias_root_by_path.get(&skill.source_path)?; let alias = plan.root_aliases.get(alias_root)?; let relative_path = skill .path_to_skills_md .as_path() - .strip_prefix(alias_root.as_path()) + .strip_prefix(alias_root.path().as_path()) .ok()?; let relative_path = relative_path.to_string_lossy().replace('\\', "/"); Some(format!("{alias}/{relative_path}")) @@ -920,6 +923,9 @@ mod tests { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf(&format!("/tmp/{name}/SKILL.md")).abs(), + ), path_to_skills_md: test_path_buf(&format!("/tmp/{name}/SKILL.md")).abs(), scope, plugin_id: None, @@ -948,6 +954,10 @@ mod tests { skills: Vec, roots: Vec, ) -> SkillLoadOutcome { + let roots = roots + .into_iter() + .map(codex_exec_server::EnvironmentPathRef::local) + .collect::>(); let skill_root_by_path = skills .iter() .filter_map(|skill| { @@ -957,9 +967,9 @@ mod tests { skill .path_to_skills_md .as_path() - .starts_with(root.as_path()) + .starts_with(root.path().as_path()) }) - .map(|root| (skill.path_to_skills_md.clone(), root.clone())) + .map(|root| (skill.source_path.clone(), root.clone())) }) .collect::>(); SkillLoadOutcome { @@ -1507,6 +1517,7 @@ mod tests { fn skill_with_path(name: &str, path: &AbsolutePathBuf) -> SkillMetadata { let mut skill = make_skill(name, SkillScope::User); skill.path_to_skills_md = path.clone(); + skill.source_path = codex_exec_server::EnvironmentPathRef::local(path.clone()); skill } } diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index aa2586cae4f..45355779ef7 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -7224,6 +7224,9 @@ async fn build_initial_context_trims_skill_metadata_from_context_window_budget() interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/admin-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/admin-skill/SKILL.md").abs(), scope: SkillScope::Admin, plugin_id: None, @@ -7235,6 +7238,9 @@ async fn build_initial_context_trims_skill_metadata_from_context_window_budget() interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/repo-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(), scope: SkillScope::Repo, plugin_id: None, @@ -7271,6 +7277,9 @@ fn emit_thread_start_skill_metrics_records_enabled_kept_and_truncated_values() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/repo-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(), scope: SkillScope::Repo, plugin_id: None, @@ -7316,6 +7325,9 @@ fn emit_thread_start_skill_metrics_records_description_truncated_chars_without_o interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/alpha-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/alpha-skill/SKILL.md").abs(), scope: SkillScope::Repo, plugin_id: None, @@ -7327,6 +7339,9 @@ fn emit_thread_start_skill_metrics_records_description_truncated_chars_without_o interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/beta-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/beta-skill/SKILL.md").abs(), scope: SkillScope::Repo, plugin_id: None, @@ -7375,6 +7390,9 @@ async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_buil interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/admin-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/admin-skill/SKILL.md").abs(), scope: SkillScope::Admin, plugin_id: None, @@ -7386,6 +7404,9 @@ async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_buil interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/repo-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(), scope: SkillScope::Repo, plugin_id: None, diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 4d1c27e49c8..895ab1d8933 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -2,6 +2,7 @@ use super::*; use crate::SkillLoadOutcome; use crate::config::GhostSnapshotConfig; use crate::environment_selection::ResolvedTurnEnvironments; +use crate::skills::EnvironmentPathRef; use codex_model_provider::SharedModelProvider; use codex_model_provider::create_model_provider; use codex_protocol::SessionId; @@ -18,7 +19,7 @@ use std::sync::atomic::Ordering; #[derive(Clone, Debug)] pub(crate) struct TurnSkillsContext { pub(crate) outcome: Arc, - pub(crate) implicit_invocation_seen_skills: Arc>>, + pub(crate) implicit_invocation_seen_skills: Arc>>, } impl TurnSkillsContext { diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index d540cd6d439..526c425d62f 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -4,8 +4,7 @@ use crate::session::turn_context::TurnContext; use codex_analytics::InvocationType; use codex_analytics::SkillInvocation; use codex_analytics::build_track_events_context; -use codex_protocol::protocol::SkillScope; -use codex_utils_absolute_path::AbsolutePathBuf; +pub use codex_exec_server::EnvironmentPathRef; use codex_utils_plugins::PluginSkillRoot; pub use codex_core_skills::SkillError; @@ -49,15 +48,18 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation( sess: &Session, turn_context: &TurnContext, command: &str, - workdir: &AbsolutePathBuf, + workdir: &EnvironmentPathRef, ) { let Some(candidate) = detect_implicit_skill_invocation_for_command( turn_context.turn_skills.outcome.as_ref(), command, workdir, - ) else { + ) + .await + else { return; }; + let source_path = candidate.source_path.clone(); let invocation = SkillInvocation { skill_name: candidate.name, skill_scope: candidate.scope, @@ -65,22 +67,14 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation( plugin_id: candidate.plugin_id, invocation_type: InvocationType::Implicit, }; - let skill_scope = match invocation.skill_scope { - SkillScope::User => "user", - SkillScope::Repo => "repo", - SkillScope::System => "system", - SkillScope::Admin => "admin", - }; - 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 inserted = { let mut seen_skills = turn_context .turn_skills .implicit_invocation_seen_skills .lock() .await; - seen_skills.insert(seen_key) + seen_skills.insert(source_path) }; if !inserted { return; diff --git a/codex-rs/core/src/tools/handlers/shell/shell_command.rs b/codex-rs/core/src/tools/handlers/shell/shell_command.rs index 8a501c711c3..dc741876ee4 100644 --- a/codex-rs/core/src/tools/handlers/shell/shell_command.rs +++ b/codex-rs/core/src/tools/handlers/shell/shell_command.rs @@ -167,6 +167,16 @@ impl ToolExecutor for ShellCommandHandler { let params: ShellCommandToolCallParams = parse_arguments_with_base_path(&arguments, &cwd)?; #[allow(deprecated)] let workdir = turn.resolve_path(params.workdir.clone()); + let workdir = turn + .environments + .primary() + .map(|environment| { + crate::skills::EnvironmentPathRef::new( + environment.environment.get_filesystem(), + workdir.clone(), + ) + }) + .unwrap_or_else(|| crate::skills::EnvironmentPathRef::local(workdir.clone())); maybe_emit_implicit_skill_invocation( session.as_ref(), turn.as_ref(), diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index 83236881f38..bbb89e03bb7 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -132,11 +132,12 @@ impl ToolExecutor for ExecCommandHandler { let fs = environment.get_filesystem(); let args: ExecCommandArgs = parse_arguments_with_base_path(&arguments, &cwd)?; let hook_command = args.cmd.clone(); + let workdir = crate::skills::EnvironmentPathRef::new(Arc::clone(&fs), cwd.clone()); maybe_emit_implicit_skill_invocation( session.as_ref(), context.turn.as_ref(), &hook_command, - &cwd, + &workdir, ) .await; let process_id = manager.allocate_process_id().await; diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index ec3284866d9..d1842601c9a 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -6255,6 +6255,7 @@ mod tests { dependencies: None, policy: None, path_to_skills_md: skill_path.clone(), + source_path: codex_exec_server::EnvironmentPathRef::local(skill_path.clone()), scope: crate::test_support::skill_scope_user(), plugin_id: None, }])); @@ -6298,6 +6299,7 @@ mod tests { dependencies: None, policy: None, path_to_skills_md: skill_path.clone(), + source_path: codex_exec_server::EnvironmentPathRef::local(skill_path.clone()), scope: crate::test_support::skill_scope_repo(), plugin_id: None, }])); @@ -6390,6 +6392,9 @@ mod tests { dependencies: None, policy: None, path_to_skills_md: test_path_buf("/tmp/repo/google-calendar/SKILL.md").abs(), + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/repo/google-calendar/SKILL.md").abs(), + ), scope: crate::test_support::skill_scope_repo(), plugin_id: None, }])); diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index fa92914670a..8e686a799dc 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -2577,6 +2577,9 @@ mod tests { dependencies: None, policy: None, path_to_skills_md: test_path_buf("/tmp/test-skill/SKILL.md").abs(), + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/test-skill/SKILL.md").abs(), + ), scope: crate::test_support::skill_scope_user(), plugin_id: None, }]), diff --git a/codex-rs/tui/src/chatwidget/skills.rs b/codex-rs/tui/src/chatwidget/skills.rs index 687ce564166..b21dfaf2e36 100644 --- a/codex-rs/tui/src/chatwidget/skills.rs +++ b/codex-rs/tui/src/chatwidget/skills.rs @@ -242,6 +242,9 @@ fn protocol_skill_to_core(skill: &ProtocolSkillMetadata) -> Option