diff --git a/codex-rs/core-skills/src/injection.rs b/codex-rs/core-skills/src/injection.rs index 2134890eb8..f50427f79a 100644 --- a/codex-rs/core-skills/src/injection.rs +++ b/codex-rs/core-skills/src/injection.rs @@ -1,10 +1,8 @@ -use std::collections::HashMap; use std::collections::HashSet; use std::sync::Arc; use crate::SkillLoadOutcome; use crate::SkillMetadata; -use crate::build_skill_name_counts; use codex_analytics::AnalyticsEventsClient; use codex_analytics::InvocationType; use codex_analytics::SkillInvocation; @@ -12,7 +10,6 @@ use codex_analytics::TrackEventsContext; use codex_exec_server::LOCAL_FS; use codex_otel::SessionTelemetry; use codex_otel::sanitize_metric_tag_value; -use codex_protocol::user_input::UserInput; pub use codex_skills::ToolMentionKind; pub use codex_skills::ToolMentions; pub use codex_skills::app_id_from_path; @@ -21,7 +18,6 @@ pub use codex_skills::extract_tool_mentions_with_sigil; pub use codex_skills::normalize_skill_path; pub use codex_skills::plugin_config_name_from_path; pub use codex_skills::tool_kind_for_path; -use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use codex_utils_string::take_bytes_at_char_boundary; @@ -177,169 +173,6 @@ fn emit_skill_injected_metric( ); } -/// Collect explicitly mentioned skills from structured and text mentions. -/// -/// 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 loaded skills in their existing order to preserve prior ordering semantics. -/// Explicit paths match either a skill's canonical identity or its logical discovery -/// path, and plain names are only used when the match is unambiguous. -/// -/// Complexity: `O(T + (N_s + N_t) * S)` time, `O(S + M)` space, where: -/// `S` = number of skills, `T` = total text length, `N_s` = number of structured skill inputs, -/// `N_t` = number of text inputs, `M` = max mentions parsed from a single text input. -pub fn collect_explicit_skill_mentions( - inputs: &[UserInput], - loaded_skills: &SkillLoadOutcome, - connector_slug_counts: &HashMap, -) -> Vec { - let skill_name_counts = - build_skill_name_counts(&loaded_skills.skills, &loaded_skills.disabled_paths).0; - - let selection_context = SkillSelectionContext { - loaded_skills, - skill_name_counts: &skill_name_counts, - connector_slug_counts, - }; - let mut selected: Vec = Vec::new(); - let mut seen_names: HashSet = HashSet::new(); - let mut seen_paths: HashSet = HashSet::new(); - let mut blocked_plain_names: HashSet = HashSet::new(); - - for input in inputs { - if let UserInput::Skill { name, path, .. } = input { - blocked_plain_names.insert(name.clone()); - let Ok(path) = AbsolutePathBuf::relative_to_current_dir(path) else { - continue; - }; - - let Some(skill) = selection_context.loaded_skills.skills.iter().find(|skill| { - skill.path_to_skills_md == path - || selection_context - .loaded_skills - .skill_discovery_path_for_path(&skill.path_to_skills_md) - .is_some_and(|discovery_path| discovery_path == &path) - }) else { - continue; - }; - - if !selection_context.loaded_skills.is_skill_enabled(skill) - || seen_paths.contains(&skill.path_to_skills_md) - { - continue; - } - - seen_paths.insert(skill.path_to_skills_md.clone()); - seen_names.insert(skill.name.clone()); - selected.push(skill.clone()); - } - } - - for input in inputs { - if let UserInput::Text { text, .. } = input { - let mentioned_names = extract_tool_mentions(text); - select_skills_from_mentions( - &selection_context, - &blocked_plain_names, - &mentioned_names, - &mut seen_names, - &mut seen_paths, - &mut selected, - ); - } - } - - selected -} - -struct SkillSelectionContext<'a> { - loaded_skills: &'a SkillLoadOutcome, - skill_name_counts: &'a HashMap, - connector_slug_counts: &'a HashMap, -} - -/// Select mentioned skills while preserving the order of `skills`. -fn select_skills_from_mentions( - selection_context: &SkillSelectionContext<'_>, - blocked_plain_names: &HashSet, - mentions: &ToolMentions<'_>, - seen_names: &mut HashSet, - seen_paths: &mut HashSet, - selected: &mut Vec, -) { - if mentions.is_empty() { - return; - } - - let mention_skill_paths: HashSet = mentions - .paths() - .filter(|path| { - !matches!( - tool_kind_for_path(path), - ToolMentionKind::App | ToolMentionKind::Mcp | ToolMentionKind::Plugin - ) - }) - .map(normalize_host_skill_path) - .collect(); - - for skill in &selection_context.loaded_skills.skills { - if !selection_context.loaded_skills.is_skill_enabled(skill) - || seen_paths.contains(&skill.path_to_skills_md) - { - continue; - } - - let canonical_path = normalize_host_skill_path(&skill.path_to_skills_md.to_string_lossy()); - let matches_discovery_path = selection_context - .loaded_skills - .skill_discovery_path_for_path(&skill.path_to_skills_md) - .is_some_and(|discovery_path| { - mention_skill_paths.contains(&normalize_host_skill_path( - &discovery_path.to_string_lossy(), - )) - }); - if mention_skill_paths.contains(&canonical_path) || matches_discovery_path { - seen_paths.insert(skill.path_to_skills_md.clone()); - seen_names.insert(skill.name.clone()); - selected.push(skill.clone()); - } - } - - for skill in &selection_context.loaded_skills.skills { - if !selection_context.loaded_skills.is_skill_enabled(skill) - || seen_paths.contains(&skill.path_to_skills_md) - { - continue; - } - - if blocked_plain_names.contains(skill.name.as_str()) { - continue; - } - if !mentions.contains_plain_name(skill.name.as_str()) { - continue; - } - - let skill_count = selection_context - .skill_name_counts - .get(skill.name.as_str()) - .copied() - .unwrap_or(0); - let connector_count = selection_context - .connector_slug_counts - .get(&skill.name.to_ascii_lowercase()) - .copied() - .unwrap_or(0); - if skill_count != 1 || connector_count != 0 { - continue; - } - - if seen_names.insert(skill.name.clone()) { - seen_paths.insert(skill.path_to_skills_md.clone()); - selected.push(skill.clone()); - } - } -} - #[cfg(test)] -#[path = "injection_tests.rs"] +#[path = "prompt_injection_tests.rs"] mod tests; diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index e792998d08..58cda12e3d 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -131,6 +131,24 @@ impl codex_skills::ImplicitSkillLookup for SkillLoadOutcome { } } +impl codex_skills::ExplicitSkillLookup for SkillLoadOutcome { + fn skills(&self) -> &[SkillMetadata] { + &self.skills + } + + fn disabled_paths(&self) -> &HashSet { + &self.disabled_paths + } + + fn skill_discovery_path_for_path(&self, path: &AbsolutePathBuf) -> Option<&AbsolutePathBuf> { + SkillLoadOutcome::skill_discovery_path_for_path(self, path) + } + + fn is_skill_enabled(&self, skill: &SkillMetadata) -> bool { + SkillLoadOutcome::is_skill_enabled(self, skill) + } +} + #[derive(Clone, Default)] pub(crate) struct SkillFileSystemsByPath { values: Arc>>, diff --git a/codex-rs/core-skills/src/prompt_injection_tests.rs b/codex-rs/core-skills/src/prompt_injection_tests.rs new file mode 100644 index 0000000000..bd91481c5b --- /dev/null +++ b/codex-rs/core-skills/src/prompt_injection_tests.rs @@ -0,0 +1,14 @@ +use pretty_assertions::assert_eq; + +use super::MAX_SKILL_PROMPT_BYTES; +use super::bounded_skill_prompt_contents; + +#[test] +fn skill_prompt_contents_are_bounded_at_utf8_boundaries() { + let contents = format!("{}é", "a".repeat(MAX_SKILL_PROMPT_BYTES - 1)); + + let (bounded, truncated) = bounded_skill_prompt_contents(&contents); + + assert_eq!(bounded.len(), MAX_SKILL_PROMPT_BYTES - 1); + assert_eq!(truncated, true); +} diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index c965b97f4c..4a71209e5b 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -22,12 +22,12 @@ pub use codex_core_skills::filter_skill_load_outcome_for_product; pub use codex_core_skills::injection; pub use codex_core_skills::injection::SkillInjections; pub use codex_core_skills::injection::build_skill_injections; -pub use codex_core_skills::injection::collect_explicit_skill_mentions; pub use codex_core_skills::loader; pub use codex_core_skills::model; pub use codex_core_skills::remote; pub use codex_skills::SkillMetadata; pub use codex_skills::SkillPolicy; +pub use codex_skills::collect_explicit_skill_mentions; pub use codex_skills_extension::HostSkillsLoadInput; pub use codex_skills_extension::HostSkillsService; pub use codex_skills_extension::bundled_skills_enabled_from_stack; diff --git a/codex-rs/skills/src/lib.rs b/codex-rs/skills/src/lib.rs index 73b608f9f7..d09dd3dbe4 100644 --- a/codex-rs/skills/src/lib.rs +++ b/codex-rs/skills/src/lib.rs @@ -5,6 +5,7 @@ mod model; mod name_counts; mod parser; mod policy; +mod selection; pub use interface::SkillInterfaceAssetPolicy; pub use interface::SkillInterfaceFile; @@ -33,6 +34,8 @@ pub use parser::ParsedSkillFrontmatter; pub use parser::SkillParseError; pub use parser::parse_skill_frontmatter_metadata; pub use policy::resolve_disabled_skill_paths; +pub use selection::ExplicitSkillLookup; +pub use selection::collect_explicit_skill_mentions; use codex_utils_absolute_path::AbsolutePathBuf; use include_dir::Dir; diff --git a/codex-rs/skills/src/selection.rs b/codex-rs/skills/src/selection.rs new file mode 100644 index 0000000000..c21bd47adb --- /dev/null +++ b/codex-rs/skills/src/selection.rs @@ -0,0 +1,205 @@ +use std::collections::HashMap; +use std::collections::HashSet; + +use codex_protocol::user_input::UserInput; +use codex_utils_absolute_path::AbsolutePathBuf; + +use crate::SkillMetadata; +use crate::ToolMentionKind; +use crate::ToolMentions; +use crate::build_skill_name_counts; +use crate::extract_tool_mentions; +use crate::normalize_skill_path; +use crate::tool_kind_for_path; + +/// Supplies ordered skills, disabled identities, and discovery paths for explicit selection. +/// +/// Implementations should preserve skill discovery order and expose the logical discovery path +/// associated with each canonical skill identity when one is available. +pub trait ExplicitSkillLookup { + fn skills(&self) -> &[SkillMetadata]; + + fn disabled_paths(&self) -> &HashSet; + + fn skill_discovery_path_for_path(&self, path: &AbsolutePathBuf) -> Option<&AbsolutePathBuf>; + + fn is_skill_enabled(&self, skill: &SkillMetadata) -> bool { + !self.disabled_paths().contains(&skill.path_to_skills_md) + } +} + +/// Collect explicitly mentioned skills from structured and text mentions. +/// +/// 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 loaded skills in their existing order to preserve prior ordering semantics. +/// Explicit paths match either a skill's canonical identity or its logical discovery +/// path, and plain names are only used when the match is unambiguous. +/// +/// Complexity: `O(T + (N_s + N_t) * S)` time, `O(S + M)` space, where: +/// `S` = number of skills, `T` = total text length, `N_s` = number of structured skill inputs, +/// `N_t` = number of text inputs, `M` = max mentions parsed from a single text input. +pub fn collect_explicit_skill_mentions( + inputs: &[UserInput], + loaded_skills: &impl ExplicitSkillLookup, + connector_slug_counts: &HashMap, +) -> Vec { + let skill_name_counts = + build_skill_name_counts(loaded_skills.skills(), loaded_skills.disabled_paths()).0; + + let selection_context = SkillSelectionContext { + loaded_skills, + skill_name_counts: &skill_name_counts, + connector_slug_counts, + }; + let mut selected: Vec = Vec::new(); + let mut seen_names: HashSet = HashSet::new(); + let mut seen_paths: HashSet = HashSet::new(); + let mut blocked_plain_names: HashSet = HashSet::new(); + + for input in inputs { + if let UserInput::Skill { name, path, .. } = input { + blocked_plain_names.insert(name.clone()); + let Ok(path) = AbsolutePathBuf::relative_to_current_dir(path) else { + continue; + }; + + let Some(skill) = selection_context + .loaded_skills + .skills() + .iter() + .find(|skill| { + skill.path_to_skills_md == path + || selection_context + .loaded_skills + .skill_discovery_path_for_path(&skill.path_to_skills_md) + .is_some_and(|discovery_path| discovery_path == &path) + }) + else { + continue; + }; + + if !selection_context.loaded_skills.is_skill_enabled(skill) + || seen_paths.contains(&skill.path_to_skills_md) + { + continue; + } + + seen_paths.insert(skill.path_to_skills_md.clone()); + seen_names.insert(skill.name.clone()); + selected.push(skill.clone()); + } + } + + for input in inputs { + if let UserInput::Text { text, .. } = input { + let mentioned_names = extract_tool_mentions(text); + select_skills_from_mentions( + &selection_context, + &blocked_plain_names, + &mentioned_names, + &mut seen_names, + &mut seen_paths, + &mut selected, + ); + } + } + + selected +} + +struct SkillSelectionContext<'a> { + loaded_skills: &'a dyn ExplicitSkillLookup, + skill_name_counts: &'a HashMap, + connector_slug_counts: &'a HashMap, +} + +/// Select mentioned skills while preserving the order of `skills`. +fn select_skills_from_mentions( + selection_context: &SkillSelectionContext<'_>, + blocked_plain_names: &HashSet, + mentions: &ToolMentions<'_>, + seen_names: &mut HashSet, + seen_paths: &mut HashSet, + selected: &mut Vec, +) { + if mentions.is_empty() { + return; + } + + let mention_skill_paths: HashSet = mentions + .paths() + .filter(|path| { + !matches!( + tool_kind_for_path(path), + ToolMentionKind::App | ToolMentionKind::Mcp | ToolMentionKind::Plugin + ) + }) + .map(normalize_host_skill_path) + .collect(); + + for skill in selection_context.loaded_skills.skills() { + if !selection_context.loaded_skills.is_skill_enabled(skill) + || seen_paths.contains(&skill.path_to_skills_md) + { + continue; + } + + let canonical_path = normalize_host_skill_path(&skill.path_to_skills_md.to_string_lossy()); + let matches_discovery_path = selection_context + .loaded_skills + .skill_discovery_path_for_path(&skill.path_to_skills_md) + .is_some_and(|discovery_path| { + mention_skill_paths.contains(&normalize_host_skill_path( + &discovery_path.to_string_lossy(), + )) + }); + if mention_skill_paths.contains(&canonical_path) || matches_discovery_path { + seen_paths.insert(skill.path_to_skills_md.clone()); + seen_names.insert(skill.name.clone()); + selected.push(skill.clone()); + } + } + + for skill in selection_context.loaded_skills.skills() { + if !selection_context.loaded_skills.is_skill_enabled(skill) + || seen_paths.contains(&skill.path_to_skills_md) + { + continue; + } + + if blocked_plain_names.contains(skill.name.as_str()) { + continue; + } + if !mentions.contains_plain_name(skill.name.as_str()) { + continue; + } + + let skill_count = selection_context + .skill_name_counts + .get(skill.name.as_str()) + .copied() + .unwrap_or(0); + let connector_count = selection_context + .connector_slug_counts + .get(&skill.name.to_ascii_lowercase()) + .copied() + .unwrap_or(0); + if skill_count != 1 || connector_count != 0 { + continue; + } + + if seen_names.insert(skill.name.clone()) { + seen_paths.insert(skill.path_to_skills_md.clone()); + selected.push(skill.clone()); + } + } +} + +fn normalize_host_skill_path(path: &str) -> String { + normalize_skill_path(path).replace('\\', "/") +} + +#[cfg(test)] +#[path = "selection_tests.rs"] +mod tests; diff --git a/codex-rs/core-skills/src/injection_tests.rs b/codex-rs/skills/src/selection_tests.rs similarity index 93% rename from codex-rs/core-skills/src/injection_tests.rs rename to codex-rs/skills/src/selection_tests.rs index 134f9dda76..5fbf83e855 100644 --- a/codex-rs/core-skills/src/injection_tests.rs +++ b/codex-rs/skills/src/selection_tests.rs @@ -5,7 +5,27 @@ 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; + +#[derive(Default)] +struct TestLookup { + skills: Vec, + disabled_paths: HashSet, + skill_discovery_path_by_path: HashMap, +} + +impl ExplicitSkillLookup for TestLookup { + fn skills(&self) -> &[SkillMetadata] { + &self.skills + } + + fn disabled_paths(&self) -> &HashSet { + &self.disabled_paths + } + + fn skill_discovery_path_for_path(&self, path: &AbsolutePathBuf) -> Option<&AbsolutePathBuf> { + self.skill_discovery_path_by_path.get(path) + } +} fn make_skill(name: &str, path: &str) -> SkillMetadata { SkillMetadata { @@ -22,16 +42,6 @@ fn make_skill(name: &str, path: &str) -> SkillMetadata { } } -#[test] -fn skill_prompt_contents_are_bounded_at_utf8_boundaries() { - let contents = format!("{}é", "a".repeat(MAX_SKILL_PROMPT_BYTES - 1)); - - let (bounded, truncated) = bounded_skill_prompt_contents(&contents); - - assert_eq!(bounded.len(), MAX_SKILL_PROMPT_BYTES - 1); - assert_eq!(truncated, true); -} - fn linked_skill_mention(name: &str, unix_path: &str) -> String { format!("[${name}]({})", test_path_buf(unix_path).display()) } @@ -42,7 +52,7 @@ fn collect_mentions( disabled_paths: &HashSet, connector_slug_counts: &HashMap, ) -> Vec { - let loaded_skills = SkillLoadOutcome { + let loaded_skills = TestLookup { skills: skills.to_vec(), disabled_paths: disabled_paths.clone(), ..Default::default() @@ -50,15 +60,12 @@ fn collect_mentions( collect_explicit_skill_mentions(inputs, &loaded_skills, connector_slug_counts) } -fn skill_outcome_with_discovery_path( - skill: SkillMetadata, - discovery_path: &str, -) -> SkillLoadOutcome { - SkillLoadOutcome { - skill_discovery_path_by_path: Arc::new(HashMap::from([( +fn skill_outcome_with_discovery_path(skill: SkillMetadata, discovery_path: &str) -> TestLookup { + TestLookup { + skill_discovery_path_by_path: HashMap::from([( skill.path_to_skills_md.clone(), test_path_buf(discovery_path).abs(), - )])), + )]), skills: vec![skill], ..Default::default() }