From e3465b48ad498ef26f3926041a08568a50b394b7 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Wed, 5 Aug 2026 22:21:20 +0000 Subject: [PATCH] Centralize skill invocation helpers in `codex-skills` (#37174) ## What changed - Move tool mention parsing, skill-name counting, and implicit invocation detection into `codex-skills` and expose them through its public API. - Decouple implicit invocation detection from `SkillLoadOutcome` with an `ImplicitSkillLookup` trait, while preserving the existing `core-skills` interface through re-exports. - Cover remote plugin attribution for both implicit `SKILL.md` reads and skill script runs. GitOrigin-RevId: f3da85c9821868638ccfd2fc2e01e0869d2fd437 --- codex-rs/Cargo.lock | 4 +- codex-rs/core-skills/Cargo.toml | 2 - codex-rs/core-skills/src/injection.rs | 262 +----------------- codex-rs/core-skills/src/injection_tests.rs | 97 ------- codex-rs/core-skills/src/invocation_utils.rs | 103 ------- codex-rs/core-skills/src/lib.rs | 6 +- codex-rs/core-skills/src/model.rs | 10 + codex-rs/core/tests/suite/plugins.rs | 32 ++- codex-rs/skills/Cargo.toml | 2 + codex-rs/skills/src/invocation.rs | 117 ++++++++ .../src/invocation_tests.rs} | 56 ++-- codex-rs/skills/src/lib.rs | 14 + codex-rs/skills/src/mentions.rs | 229 +++++++++++++++ codex-rs/skills/src/mentions_tests.rs | 80 ++++++ .../src/name_counts.rs} | 3 +- 15 files changed, 530 insertions(+), 487 deletions(-) create mode 100644 codex-rs/skills/src/invocation.rs rename codex-rs/{core-skills/src/invocation_utils_tests.rs => skills/src/invocation_tests.rs} (77%) create mode 100644 codex-rs/skills/src/mentions.rs create mode 100644 codex-rs/skills/src/mentions_tests.rs rename codex-rs/{core-skills/src/mention_counts.rs => skills/src/name_counts.rs} (96%) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 3b60b78ca1ff..fe65fb41b6a8 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2917,7 +2917,6 @@ dependencies = [ "codex-model-provider", "codex-otel", "codex-protocol", - "codex-shell-command", "codex-skills", "codex-utils-absolute-path", "codex-utils-path-uri", @@ -2929,7 +2928,6 @@ dependencies = [ "serde", "serde_json", "serde_yaml", - "shlex", "tempfile", "tokio", "toml 0.9.11+spec-1.1.0", @@ -4086,12 +4084,14 @@ name = "codex-skills" version = "0.0.0" dependencies = [ "codex-protocol", + "codex-shell-command", "codex-utils-absolute-path", "codex-utils-path-uri", "include_dir", "pretty_assertions", "serde", "serde_yaml", + "shlex", "thiserror 2.0.18", "tracing", ] diff --git a/codex-rs/core-skills/Cargo.toml b/codex-rs/core-skills/Cargo.toml index e0acd97111e6..43d054cbf7b1 100644 --- a/codex-rs/core-skills/Cargo.toml +++ b/codex-rs/core-skills/Cargo.toml @@ -22,7 +22,6 @@ codex-login = { workspace = true } codex-model-provider = { workspace = true } codex-otel = { workspace = true } codex-protocol = { workspace = true } -codex-shell-command = { workspace = true } codex-skills = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-path-uri = { workspace = true } @@ -33,7 +32,6 @@ futures = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } serde_yaml = { workspace = true } -shlex = { workspace = true } tokio = { workspace = true, features = ["fs", "macros", "rt"] } tracing = { workspace = true } zip = { workspace = true } diff --git a/codex-rs/core-skills/src/injection.rs b/codex-rs/core-skills/src/injection.rs index 8f41fb223d17..2134890eb81e 100644 --- a/codex-rs/core-skills/src/injection.rs +++ b/codex-rs/core-skills/src/injection.rs @@ -13,9 +13,16 @@ 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; +pub use codex_skills::extract_tool_mentions; +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_plugins::mention_syntax::TOOL_MENTION_SIGIL; use codex_utils_string::take_bytes_at_char_boundary; use crate::MAX_SKILL_PROMPT_BYTES; @@ -251,146 +258,6 @@ struct SkillSelectionContext<'a> { connector_slug_counts: &'a HashMap, } -pub struct ToolMentions<'a> { - names: HashSet<&'a str>, - paths: HashSet<&'a str>, - plain_names: HashSet<&'a str>, -} - -impl<'a> ToolMentions<'a> { - fn is_empty(&self) -> bool { - self.names.is_empty() && self.paths.is_empty() - } - - pub fn plain_names(&self) -> impl Iterator + '_ { - self.plain_names.iter().copied() - } - - pub fn paths(&self) -> impl Iterator + '_ { - self.paths.iter().copied() - } -} - -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub enum ToolMentionKind { - App, - Mcp, - Plugin, - Skill, - Other, -} - -const APP_PATH_PREFIX: &str = "app://"; -const MCP_PATH_PREFIX: &str = "mcp://"; -const PLUGIN_PATH_PREFIX: &str = "plugin://"; -const SKILL_PATH_PREFIX: &str = "skill://"; -const SKILL_FILENAME: &str = "SKILL.md"; - -pub fn tool_kind_for_path(path: &str) -> ToolMentionKind { - if path.starts_with(APP_PATH_PREFIX) { - ToolMentionKind::App - } else if path.starts_with(MCP_PATH_PREFIX) { - ToolMentionKind::Mcp - } else if path.starts_with(PLUGIN_PATH_PREFIX) { - ToolMentionKind::Plugin - } else if path.starts_with(SKILL_PATH_PREFIX) || is_skill_filename(path) { - ToolMentionKind::Skill - } else { - ToolMentionKind::Other - } -} - -fn is_skill_filename(path: &str) -> bool { - let file_name = path.rsplit(['/', '\\']).next().unwrap_or(path); - file_name.eq_ignore_ascii_case(SKILL_FILENAME) -} - -pub fn app_id_from_path(path: &str) -> Option<&str> { - path.strip_prefix(APP_PATH_PREFIX) - .filter(|value| !value.is_empty()) -} - -pub fn plugin_config_name_from_path(path: &str) -> Option<&str> { - path.strip_prefix(PLUGIN_PATH_PREFIX) - .filter(|value| !value.is_empty()) -} - -pub(crate) fn normalize_skill_path(path: &str) -> &str { - path.strip_prefix(SKILL_PATH_PREFIX).unwrap_or(path) -} - -/// Extract `$tool-name` mentions from a single text input. -/// -/// Supports explicit resource links in the form `[$tool-name](resource path)`. When a -/// resource path is present, it is captured for exact path matching while also tracking -/// the name for fallback matching. -pub fn extract_tool_mentions(text: &str) -> ToolMentions<'_> { - extract_tool_mentions_with_sigil(text, TOOL_MENTION_SIGIL) -} - -pub fn extract_tool_mentions_with_sigil(text: &str, sigil: char) -> ToolMentions<'_> { - let text_bytes = text.as_bytes(); - 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 index = 0; - while index < text_bytes.len() { - let byte = text_bytes[index]; - if byte == b'[' - && let Some((name, path, end_index)) = - parse_linked_tool_mention(text, text_bytes, index, sigil) - { - if !is_common_env_var(name) { - if !matches!( - tool_kind_for_path(path), - ToolMentionKind::App | ToolMentionKind::Mcp | ToolMentionKind::Plugin - ) { - mentioned_names.insert(name); - } - mentioned_paths.insert(path); - } - index = end_index; - continue; - } - - if byte != sigil as u8 { - index += 1; - continue; - } - - let name_start = index + 1; - let Some(first_name_byte) = text_bytes.get(name_start) else { - index += 1; - continue; - }; - if !is_mention_name_char(*first_name_byte) { - index += 1; - continue; - } - - let mut name_end = name_start + 1; - while let Some(next_byte) = text_bytes.get(name_end) - && is_mention_name_char(*next_byte) - { - name_end += 1; - } - - let name = &text[name_start..name_end]; - if !is_common_env_var(name) { - mentioned_names.insert(name); - plain_names.insert(name); - } - index = name_end; - } - - ToolMentions { - names: mentioned_names, - paths: mentioned_paths, - plain_names, - } -} - /// Select mentioned skills while preserving the order of `skills`. fn select_skills_from_mentions( selection_context: &SkillSelectionContext<'_>, @@ -448,7 +315,7 @@ fn select_skills_from_mentions( if blocked_plain_names.contains(skill.name.as_str()) { continue; } - if !mentions.plain_names.contains(skill.name.as_str()) { + if !mentions.contains_plain_name(skill.name.as_str()) { continue; } @@ -473,117 +340,6 @@ fn select_skills_from_mentions( } } -fn parse_linked_tool_mention<'a>( - text: &'a str, - text_bytes: &[u8], - start: usize, - sigil: char, -) -> Option<(&'a str, &'a str, usize)> { - let sigil_index = start + 1; - if text_bytes.get(sigil_index) != Some(&(sigil as u8)) { - return None; - } - - let name_start = sigil_index + 1; - let first_name_byte = text_bytes.get(name_start)?; - if !is_mention_name_char(*first_name_byte) { - return None; - } - - let mut name_end = name_start + 1; - while let Some(next_byte) = text_bytes.get(name_end) - && is_mention_name_char(*next_byte) - { - name_end += 1; - } - - if text_bytes.get(name_end) != Some(&b']') { - return None; - } - - let mut path_start = name_end + 1; - while let Some(next_byte) = text_bytes.get(path_start) - && next_byte.is_ascii_whitespace() - { - path_start += 1; - } - if text_bytes.get(path_start) != Some(&b'(') { - return None; - } - - let mut path_end = path_start + 1; - while let Some(next_byte) = text_bytes.get(path_end) - && *next_byte != b')' - { - path_end += 1; - } - if text_bytes.get(path_end) != Some(&b')') { - return None; - } - - let path = text[path_start + 1..path_end].trim(); - if path.is_empty() { - return None; - } - - let name = &text[name_start..name_end]; - Some((name, path, path_end + 1)) -} - -fn is_common_env_var(name: &str) -> bool { - let upper = name.to_ascii_uppercase(); - matches!( - upper.as_str(), - "PATH" - | "HOME" - | "USER" - | "SHELL" - | "PWD" - | "TMPDIR" - | "TEMP" - | "TMP" - | "LANG" - | "TERM" - | "XDG_CONFIG_HOME" - ) -} - -#[cfg(test)] -fn text_mentions_skill(text: &str, skill_name: &str) -> bool { - if skill_name.is_empty() { - return false; - } - - let text_bytes = text.as_bytes(); - let skill_bytes = skill_name.as_bytes(); - - for (index, byte) in text_bytes.iter().copied().enumerate() { - if byte != b'$' { - continue; - } - - let name_start = index + 1; - let Some(rest) = text_bytes.get(name_start..) else { - continue; - }; - if !rest.starts_with(skill_bytes) { - continue; - } - - let after_index = name_start + skill_bytes.len(); - let after = text_bytes.get(after_index).copied(); - if after.is_none_or(|b| !is_mention_name_char(b)) { - return true; - } - } - - false -} - -fn is_mention_name_char(byte: u8) -> bool { - matches!(byte, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'_' | b'-' | b':') -} - #[cfg(test)] #[path = "injection_tests.rs"] mod tests; diff --git a/codex-rs/core-skills/src/injection_tests.rs b/codex-rs/core-skills/src/injection_tests.rs index 0716869ca03f..134f9dda76a9 100644 --- a/codex-rs/core-skills/src/injection_tests.rs +++ b/codex-rs/core-skills/src/injection_tests.rs @@ -32,16 +32,6 @@ fn skill_prompt_contents_are_bounded_at_utf8_boundaries() { assert_eq!(truncated, true); } -fn set<'a>(items: &'a [&'a str]) -> HashSet<&'a str> { - items.iter().copied().collect() -} - -fn assert_mentions(text: &str, expected_names: &[&str], expected_paths: &[&str]) { - let mentions = extract_tool_mentions(text); - assert_eq!(mentions.names, set(expected_names)); - assert_eq!(mentions.paths, set(expected_paths)); -} - fn linked_skill_mention(name: &str, unix_path: &str) -> String { format!("[${name}]({})", test_path_buf(unix_path).display()) } @@ -74,93 +64,6 @@ fn skill_outcome_with_discovery_path( } } -#[test] -fn text_mentions_skill_requires_exact_boundary() { - assert_eq!( - true, - text_mentions_skill("use $notion-research-doc please", "notion-research-doc") - ); - assert_eq!( - true, - text_mentions_skill("($notion-research-doc)", "notion-research-doc") - ); - assert_eq!( - true, - text_mentions_skill("$notion-research-doc.", "notion-research-doc") - ); - assert_eq!( - false, - text_mentions_skill("$notion-research-docs", "notion-research-doc") - ); - assert_eq!( - false, - text_mentions_skill("$notion-research-doc_extra", "notion-research-doc") - ); -} - -#[test] -fn text_mentions_skill_handles_end_boundary_and_near_misses() { - assert_eq!(true, text_mentions_skill("$alpha-skill", "alpha-skill")); - assert_eq!(false, text_mentions_skill("$alpha-skillx", "alpha-skill")); - assert_eq!( - true, - text_mentions_skill("$alpha-skillx and later $alpha-skill ", "alpha-skill") - ); -} - -#[test] -fn text_mentions_skill_handles_many_dollars_without_looping() { - let prefix = "$".repeat(256); - let text = format!("{prefix} not-a-mention"); - assert_eq!(false, text_mentions_skill(&text, "alpha-skill")); -} - -#[test] -fn extract_tool_mentions_handles_plain_and_linked_mentions() { - assert_mentions( - "use $alpha and [$beta](/tmp/beta)", - &["alpha", "beta"], - &["/tmp/beta"], - ); -} - -#[test] -fn extract_tool_mentions_skips_common_env_vars() { - assert_mentions("use $PATH and $alpha", &["alpha"], &[]); - assert_mentions("use [$HOME](/tmp/skill)", &[], &[]); - assert_mentions("use $XDG_CONFIG_HOME and $beta", &["beta"], &[]); -} - -#[test] -fn extract_tool_mentions_requires_link_syntax() { - assert_mentions("[beta](/tmp/beta)", &[], &[]); - assert_mentions("[$beta] /tmp/beta", &["beta"], &[]); - assert_mentions("[$beta]()", &["beta"], &[]); -} - -#[test] -fn extract_tool_mentions_trims_linked_paths_and_allows_spacing() { - assert_mentions("use [$beta] ( /tmp/beta )", &["beta"], &["/tmp/beta"]); -} - -#[test] -fn extract_tool_mentions_stops_at_non_name_chars() { - assert_mentions( - "use $alpha.skill and $beta_extra", - &["alpha", "beta_extra"], - &[], - ); -} - -#[test] -fn extract_tool_mentions_keeps_plugin_skill_namespaces() { - assert_mentions( - "use $slack:search and $alpha", - &["alpha", "slack:search"], - &[], - ); -} - #[test] fn collect_explicit_skill_mentions_text_respects_skill_order() { let alpha = make_skill("alpha-skill", "/tmp/alpha"); diff --git a/codex-rs/core-skills/src/invocation_utils.rs b/codex-rs/core-skills/src/invocation_utils.rs index 50864a834919..760b2e5f82b9 100644 --- a/codex-rs/core-skills/src/invocation_utils.rs +++ b/codex-rs/core-skills/src/invocation_utils.rs @@ -1,10 +1,6 @@ use std::collections::HashMap; -use std::path::Path; -use crate::SkillLoadOutcome; use crate::SkillMetadata; -use codex_protocol::parse_command::ParsedCommand; -use codex_shell_command::parse_command::parse_command_impl; use codex_utils_absolute_path::AbsolutePathBuf; pub(crate) fn build_implicit_skill_path_indexes( @@ -28,105 +24,6 @@ pub(crate) fn build_implicit_skill_path_indexes( (by_scripts_dir, by_skill_doc_path) } -pub fn detect_implicit_skill_invocation_for_command( - outcome: &SkillLoadOutcome, - command: &str, - workdir: &AbsolutePathBuf, -) -> Option { - let workdir = canonicalize_if_exists(workdir); - let tokens = tokenize_command(command); - - if let Some(candidate) = detect_skill_script_run(outcome, tokens.as_slice(), &workdir) { - return Some(candidate); - } - - detect_skill_doc_read(outcome, tokens.as_slice(), &workdir) -} - -fn tokenize_command(command: &str) -> Vec { - shlex::split(command) - .unwrap_or_else(|| command.split_whitespace().map(str::to_string).collect()) -} - -fn script_run_token(tokens: &[String]) -> Option<&str> { - const RUNNERS: [&str; 10] = [ - "python", "python3", "bash", "zsh", "sh", "node", "deno", "ruby", "perl", "pwsh", - ]; - const SCRIPT_EXTENSIONS: [&str; 7] = [".py", ".sh", ".js", ".ts", ".rb", ".pl", ".ps1"]; - - let runner_token = tokens.first()?; - let runner = command_basename(runner_token).to_ascii_lowercase(); - let runner = runner.strip_suffix(".exe").unwrap_or(&runner); - if !RUNNERS.contains(&runner) { - return None; - } - - let mut script_token = None; - for token in tokens.iter().skip(1) { - if token == "--" || token.starts_with('-') { - continue; - } - script_token = Some(token.as_str()); - break; - } - let script_token = script_token?; - if SCRIPT_EXTENSIONS - .iter() - .any(|extension| script_token.to_ascii_lowercase().ends_with(extension)) - { - return Some(script_token); - } - - None -} - -fn detect_skill_script_run( - outcome: &SkillLoadOutcome, - tokens: &[String], - workdir: &AbsolutePathBuf, -) -> 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)); - - for path in script_path.ancestors() { - if let Some(candidate) = outcome.implicit_skills_by_scripts_dir.get(&path) { - return Some(candidate.clone()); - } - } - - None -} - -fn detect_skill_doc_read( - outcome: &SkillLoadOutcome, - tokens: &[String], - workdir: &AbsolutePathBuf, -) -> Option { - for command in parse_command_impl(tokens) { - if let ParsedCommand::Read { path, .. } = command { - let candidate_path = canonicalize_if_exists(&workdir.join(path.as_path())); - if let Some(candidate) = outcome.implicit_skills_by_doc_path.get(&candidate_path) { - return Some(candidate.clone()); - } - } - } - - None -} - -fn command_basename(command: &str) -> String { - Path::new(command) - .file_name() - .and_then(|name| name.to_str()) - .unwrap_or(command) - .to_string() -} - fn canonicalize_if_exists(path: &AbsolutePathBuf) -> AbsolutePathBuf { path.canonicalize().unwrap_or_else(|_| path.clone()) } - -#[cfg(test)] -#[path = "invocation_utils_tests.rs"] -mod tests; diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs index 2d977583fe40..5e453682413d 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -2,7 +2,6 @@ pub mod config_rules; pub mod injection; pub(crate) mod invocation_utils; pub mod loader; -mod mention_counts; pub mod model; pub mod remote; mod root_loader; @@ -14,9 +13,10 @@ mod skill_instructions; /// limit so a skill cannot bypass context bounds by changing how it is loaded. pub const MAX_SKILL_PROMPT_BYTES: usize = 8_000; +pub use codex_skills::ImplicitSkillLookup; +pub use codex_skills::build_skill_name_counts; +pub use codex_skills::detect_implicit_skill_invocation_for_command; pub(crate) use invocation_utils::build_implicit_skill_path_indexes; -pub use invocation_utils::detect_implicit_skill_invocation_for_command; -pub use mention_counts::build_skill_name_counts; pub use model::SkillError; pub use model::SkillLoadOutcome; pub use model::SkillMetadata; diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index e4d982fb8c45..e792998d08bf 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -121,6 +121,16 @@ impl SkillLoadOutcome { } } +impl codex_skills::ImplicitSkillLookup for SkillLoadOutcome { + fn implicit_skill_for_scripts_dir(&self, path: &AbsolutePathBuf) -> Option<&SkillMetadata> { + self.implicit_skills_by_scripts_dir.get(path) + } + + fn implicit_skill_for_doc_path(&self, path: &AbsolutePathBuf) -> Option<&SkillMetadata> { + self.implicit_skills_by_doc_path.get(path) + } +} + #[derive(Clone, Default)] pub(crate) struct SkillFileSystemsByPath { values: Arc>>, diff --git a/codex-rs/core/tests/suite/plugins.rs b/codex-rs/core/tests/suite/plugins.rs index d90b51140ba0..563441abe832 100644 --- a/codex-rs/core/tests/suite/plugins.rs +++ b/codex-rs/core/tests/suite/plugins.rs @@ -1312,15 +1312,43 @@ async fn explicit_plugin_skill_invocation_tracks_remote_plugin_id() -> Result<() Ok(()) } +#[derive(Clone, Copy)] +enum ImplicitPluginSkillInvocation { + SkillDocumentRead, + SkillScriptRun, +} + +#[test_case(ImplicitPluginSkillInvocation::SkillDocumentRead; "skill document read")] +#[test_case(ImplicitPluginSkillInvocation::SkillScriptRun; "skill script run")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn implicit_plugin_skill_invocation_tracks_remote_plugin_id() -> Result<()> { +async fn implicit_plugin_skill_invocation_tracks_remote_plugin_id( + invocation: ImplicitPluginSkillInvocation, +) -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; let codex_home = Arc::new(TempDir::new()?); let skill_path = write_remote_plugin_skill_plugin(codex_home.as_ref()); persist_sample_remote_plugin_id(codex_home.as_ref()); + let command = match invocation { + ImplicitPluginSkillInvocation::SkillDocumentRead => { + format!("cat {}", skill_path.display()) + } + ImplicitPluginSkillInvocation::SkillScriptRun => { + let script_path = skill_path + .parent() + .expect("skill path should have a parent") + .join("scripts/test.sh"); + std::fs::create_dir_all( + script_path + .parent() + .expect("script path should have a parent"), + )?; + std::fs::write(&script_path, "echo skill script invoked\n")?; + format!("bash {}", script_path.display()) + } + }; let command_args = serde_json::json!({ - "command": format!("cat {}", skill_path.display()), + "command": command, "login": false, }) .to_string(); diff --git a/codex-rs/skills/Cargo.toml b/codex-rs/skills/Cargo.toml index 15080dad7d76..7342975aaea0 100644 --- a/codex-rs/skills/Cargo.toml +++ b/codex-rs/skills/Cargo.toml @@ -15,11 +15,13 @@ workspace = true [dependencies] codex-protocol = { workspace = true } +codex-shell-command = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-path-uri = { workspace = true } include_dir = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_yaml = { workspace = true } +shlex = { workspace = true } thiserror = { workspace = true } tracing = { workspace = true } diff --git a/codex-rs/skills/src/invocation.rs b/codex-rs/skills/src/invocation.rs new file mode 100644 index 000000000000..ca686cf86ec4 --- /dev/null +++ b/codex-rs/skills/src/invocation.rs @@ -0,0 +1,117 @@ +use std::path::Path; + +use codex_protocol::parse_command::ParsedCommand; +use codex_shell_command::parse_command::parse_command_impl; +use codex_utils_absolute_path::AbsolutePathBuf; + +use crate::SkillMetadata; + +/// Provides the indexed skill lookups used to recognize implicit invocations. +pub trait ImplicitSkillLookup { + fn implicit_skill_for_scripts_dir(&self, path: &AbsolutePathBuf) -> Option<&SkillMetadata>; + + fn implicit_skill_for_doc_path(&self, path: &AbsolutePathBuf) -> Option<&SkillMetadata>; +} + +pub fn detect_implicit_skill_invocation_for_command( + outcome: &impl ImplicitSkillLookup, + command: &str, + workdir: &AbsolutePathBuf, +) -> Option { + let workdir = canonicalize_if_exists(workdir); + let tokens = tokenize_command(command); + + if let Some(candidate) = detect_skill_script_run(outcome, tokens.as_slice(), &workdir) { + return Some(candidate); + } + + detect_skill_doc_read(outcome, tokens.as_slice(), &workdir) +} + +fn tokenize_command(command: &str) -> Vec { + shlex::split(command) + .unwrap_or_else(|| command.split_whitespace().map(str::to_string).collect()) +} + +fn script_run_token(tokens: &[String]) -> Option<&str> { + const RUNNERS: [&str; 10] = [ + "python", "python3", "bash", "zsh", "sh", "node", "deno", "ruby", "perl", "pwsh", + ]; + const SCRIPT_EXTENSIONS: [&str; 7] = [".py", ".sh", ".js", ".ts", ".rb", ".pl", ".ps1"]; + + let runner_token = tokens.first()?; + let runner = command_basename(runner_token).to_ascii_lowercase(); + let runner = runner.strip_suffix(".exe").unwrap_or(&runner); + if !RUNNERS.contains(&runner) { + return None; + } + + let mut script_token = None; + for token in tokens.iter().skip(1) { + if token == "--" || token.starts_with('-') { + continue; + } + script_token = Some(token.as_str()); + break; + } + let script_token = script_token?; + if SCRIPT_EXTENSIONS + .iter() + .any(|extension| script_token.to_ascii_lowercase().ends_with(extension)) + { + return Some(script_token); + } + + None +} + +fn detect_skill_script_run( + outcome: &impl ImplicitSkillLookup, + tokens: &[String], + workdir: &AbsolutePathBuf, +) -> 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)); + + for path in script_path.ancestors() { + if let Some(candidate) = outcome.implicit_skill_for_scripts_dir(&path) { + return Some(candidate.clone()); + } + } + + None +} + +fn detect_skill_doc_read( + outcome: &impl ImplicitSkillLookup, + tokens: &[String], + workdir: &AbsolutePathBuf, +) -> Option { + for command in parse_command_impl(tokens) { + if let ParsedCommand::Read { path, .. } = command { + let candidate_path = canonicalize_if_exists(&workdir.join(path.as_path())); + if let Some(candidate) = outcome.implicit_skill_for_doc_path(&candidate_path) { + return Some(candidate.clone()); + } + } + } + + None +} + +fn command_basename(command: &str) -> String { + Path::new(command) + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or(command) + .to_string() +} + +fn canonicalize_if_exists(path: &AbsolutePathBuf) -> AbsolutePathBuf { + path.canonicalize().unwrap_or_else(|_| path.clone()) +} + +#[cfg(test)] +#[path = "invocation_tests.rs"] +mod tests; diff --git a/codex-rs/core-skills/src/invocation_utils_tests.rs b/codex-rs/skills/src/invocation_tests.rs similarity index 77% rename from codex-rs/core-skills/src/invocation_utils_tests.rs rename to codex-rs/skills/src/invocation_tests.rs index cbbf4c52aff9..0daf84b162ff 100644 --- a/codex-rs/core-skills/src/invocation_utils_tests.rs +++ b/codex-rs/skills/src/invocation_tests.rs @@ -1,15 +1,27 @@ -use super::SkillLoadOutcome; -use super::SkillMetadata; -use super::canonicalize_if_exists; -use super::detect_skill_doc_read; -use super::detect_skill_script_run; -use super::script_run_token; +use std::collections::HashMap; + use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::test_support::PathBufExt; use codex_utils_absolute_path::test_support::test_path_buf; use pretty_assertions::assert_eq; -use std::collections::HashMap; -use std::sync::Arc; + +use super::*; + +#[derive(Default)] +struct TestLookup { + by_scripts_dir: HashMap, + by_doc_path: HashMap, +} + +impl ImplicitSkillLookup for TestLookup { + fn implicit_skill_for_scripts_dir(&self, path: &AbsolutePathBuf) -> Option<&SkillMetadata> { + self.by_scripts_dir.get(path) + } + + fn implicit_skill_for_doc_path(&self, path: &AbsolutePathBuf) -> Option<&SkillMetadata> { + self.by_doc_path.get(path) + } +} fn test_skill_metadata(skill_doc_path: AbsolutePathBuf) -> SkillMetadata { SkillMetadata { @@ -38,7 +50,7 @@ fn script_run_detection_matches_runner_plus_extension() { "scripts/fetch_comments.py".to_string(), ]; - assert_eq!(script_run_token(&tokens).is_some(), true); + assert!(script_run_token(&tokens).is_some()); } #[test] @@ -49,7 +61,7 @@ fn script_run_detection_excludes_python_c() { "print(1)".to_string(), ]; - assert_eq!(script_run_token(&tokens).is_some(), false); + assert!(script_run_token(&tokens).is_none()); } #[test] @@ -57,18 +69,17 @@ 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 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)])), + let outcome = TestLookup { + by_doc_path: HashMap::from([(normalized_skill_doc_path, skill)]), ..Default::default() }; - let tokens = vec![ "cat".to_string(), test_path_display("/tmp/skill-test/SKILL.md"), "|".to_string(), "head".to_string(), ]; + let found = detect_skill_doc_read(&outcome, &tokens, &test_path_buf("/tmp").abs()); assert_eq!( @@ -82,17 +93,16 @@ fn skill_doc_read_detection_matches_shared_read_parser() { 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 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)])), + let outcome = TestLookup { + by_doc_path: HashMap::from([(normalized_skill_doc_path, skill)]), ..Default::default() }; - let tokens = vec![ "nl".to_string(), "-ba".to_string(), test_path_display("/tmp/skill-test/SKILL.md"), ]; + let found = detect_skill_doc_read(&outcome, &tokens, &test_path_buf("/tmp").abs()); assert_eq!( @@ -106,9 +116,8 @@ 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 skill = test_skill_metadata(skill_doc_path); - let outcome = SkillLoadOutcome { - implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])), - implicit_skills_by_doc_path: Arc::new(HashMap::new()), + let outcome = TestLookup { + by_scripts_dir: HashMap::from([(scripts_dir, skill)]), ..Default::default() }; let tokens = vec![ @@ -129,9 +138,8 @@ 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 skill = test_skill_metadata(skill_doc_path); - let outcome = SkillLoadOutcome { - implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])), - implicit_skills_by_doc_path: Arc::new(HashMap::new()), + let outcome = TestLookup { + by_scripts_dir: HashMap::from([(scripts_dir, skill)]), ..Default::default() }; let tokens = vec![ diff --git a/codex-rs/skills/src/lib.rs b/codex-rs/skills/src/lib.rs index 925073f00b73..73b608f9f7ad 100644 --- a/codex-rs/skills/src/lib.rs +++ b/codex-rs/skills/src/lib.rs @@ -1,11 +1,24 @@ mod interface; +mod invocation; +mod mentions; mod model; +mod name_counts; mod parser; mod policy; pub use interface::SkillInterfaceAssetPolicy; pub use interface::SkillInterfaceFile; pub use interface::resolve_skill_interface; +pub use invocation::ImplicitSkillLookup; +pub use invocation::detect_implicit_skill_invocation_for_command; +pub use mentions::ToolMentionKind; +pub use mentions::ToolMentions; +pub use mentions::app_id_from_path; +pub use mentions::extract_tool_mentions; +pub use mentions::extract_tool_mentions_with_sigil; +pub use mentions::normalize_skill_path; +pub use mentions::plugin_config_name_from_path; +pub use mentions::tool_kind_for_path; pub use model::EnvironmentSkillMetadata; pub use model::SkillConfigRule; pub use model::SkillConfigRuleSelector; @@ -15,6 +28,7 @@ pub use model::SkillInterface; pub use model::SkillMetadata; pub use model::SkillPolicy; pub use model::SkillToolDependency; +pub use name_counts::build_skill_name_counts; pub use parser::ParsedSkillFrontmatter; pub use parser::SkillParseError; pub use parser::parse_skill_frontmatter_metadata; diff --git a/codex-rs/skills/src/mentions.rs b/codex-rs/skills/src/mentions.rs new file mode 100644 index 000000000000..507a8480dfaa --- /dev/null +++ b/codex-rs/skills/src/mentions.rs @@ -0,0 +1,229 @@ +use std::collections::HashSet; + +pub struct ToolMentions<'a> { + names: HashSet<&'a str>, + paths: HashSet<&'a str>, + plain_names: HashSet<&'a str>, +} + +impl<'a> ToolMentions<'a> { + pub fn is_empty(&self) -> bool { + self.names.is_empty() && self.paths.is_empty() + } + + pub fn plain_names(&self) -> impl Iterator + '_ { + self.plain_names.iter().copied() + } + + pub fn contains_plain_name(&self, name: &str) -> bool { + self.plain_names.contains(name) + } + + pub fn paths(&self) -> impl Iterator + '_ { + self.paths.iter().copied() + } +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum ToolMentionKind { + App, + Mcp, + Plugin, + Skill, + Other, +} + +const APP_PATH_PREFIX: &str = "app://"; +const MCP_PATH_PREFIX: &str = "mcp://"; +const PLUGIN_PATH_PREFIX: &str = "plugin://"; +const SKILL_PATH_PREFIX: &str = "skill://"; +const SKILL_FILENAME: &str = "SKILL.md"; +const TOOL_MENTION_SIGIL: char = '$'; + +pub fn tool_kind_for_path(path: &str) -> ToolMentionKind { + if path.starts_with(APP_PATH_PREFIX) { + ToolMentionKind::App + } else if path.starts_with(MCP_PATH_PREFIX) { + ToolMentionKind::Mcp + } else if path.starts_with(PLUGIN_PATH_PREFIX) { + ToolMentionKind::Plugin + } else if path.starts_with(SKILL_PATH_PREFIX) || is_skill_filename(path) { + ToolMentionKind::Skill + } else { + ToolMentionKind::Other + } +} + +fn is_skill_filename(path: &str) -> bool { + let file_name = path.rsplit(['/', '\\']).next().unwrap_or(path); + file_name.eq_ignore_ascii_case(SKILL_FILENAME) +} + +pub fn app_id_from_path(path: &str) -> Option<&str> { + path.strip_prefix(APP_PATH_PREFIX) + .filter(|value| !value.is_empty()) +} + +pub fn plugin_config_name_from_path(path: &str) -> Option<&str> { + path.strip_prefix(PLUGIN_PATH_PREFIX) + .filter(|value| !value.is_empty()) +} + +pub fn normalize_skill_path(path: &str) -> &str { + path.strip_prefix(SKILL_PATH_PREFIX).unwrap_or(path) +} + +/// Extract `$tool-name` mentions from a single text input. +/// +/// Supports explicit resource links in the form `[$tool-name](resource path)`. When a +/// resource path is present, it is captured for exact path matching while also tracking +/// the name for fallback matching. +pub fn extract_tool_mentions(text: &str) -> ToolMentions<'_> { + extract_tool_mentions_with_sigil(text, TOOL_MENTION_SIGIL) +} + +pub fn extract_tool_mentions_with_sigil(text: &str, sigil: char) -> ToolMentions<'_> { + let text_bytes = text.as_bytes(); + 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 index = 0; + while index < text_bytes.len() { + let byte = text_bytes[index]; + if byte == b'[' + && let Some((name, path, end_index)) = + parse_linked_tool_mention(text, text_bytes, index, sigil) + { + if !is_common_env_var(name) { + if !matches!( + tool_kind_for_path(path), + ToolMentionKind::App | ToolMentionKind::Mcp | ToolMentionKind::Plugin + ) { + mentioned_names.insert(name); + } + mentioned_paths.insert(path); + } + index = end_index; + continue; + } + + if byte != sigil as u8 { + index += 1; + continue; + } + + let name_start = index + 1; + let Some(first_name_byte) = text_bytes.get(name_start) else { + index += 1; + continue; + }; + if !is_mention_name_char(*first_name_byte) { + index += 1; + continue; + } + + let mut name_end = name_start + 1; + while let Some(next_byte) = text_bytes.get(name_end) + && is_mention_name_char(*next_byte) + { + name_end += 1; + } + + let name = &text[name_start..name_end]; + if !is_common_env_var(name) { + mentioned_names.insert(name); + plain_names.insert(name); + } + index = name_end; + } + + ToolMentions { + names: mentioned_names, + paths: mentioned_paths, + plain_names, + } +} + +fn parse_linked_tool_mention<'a>( + text: &'a str, + text_bytes: &[u8], + start: usize, + sigil: char, +) -> Option<(&'a str, &'a str, usize)> { + let sigil_index = start + 1; + if text_bytes.get(sigil_index) != Some(&(sigil as u8)) { + return None; + } + + let name_start = sigil_index + 1; + let first_name_byte = text_bytes.get(name_start)?; + if !is_mention_name_char(*first_name_byte) { + return None; + } + + let mut name_end = name_start + 1; + while let Some(next_byte) = text_bytes.get(name_end) + && is_mention_name_char(*next_byte) + { + name_end += 1; + } + + if text_bytes.get(name_end) != Some(&b']') { + return None; + } + + let mut path_start = name_end + 1; + while let Some(next_byte) = text_bytes.get(path_start) + && next_byte.is_ascii_whitespace() + { + path_start += 1; + } + if text_bytes.get(path_start) != Some(&b'(') { + return None; + } + + let mut path_end = path_start + 1; + while let Some(next_byte) = text_bytes.get(path_end) + && *next_byte != b')' + { + path_end += 1; + } + if text_bytes.get(path_end) != Some(&b')') { + return None; + } + + let path = text[path_start + 1..path_end].trim(); + if path.is_empty() { + return None; + } + + let name = &text[name_start..name_end]; + Some((name, path, path_end + 1)) +} + +fn is_common_env_var(name: &str) -> bool { + let upper = name.to_ascii_uppercase(); + matches!( + upper.as_str(), + "PATH" + | "HOME" + | "USER" + | "SHELL" + | "PWD" + | "TMPDIR" + | "TEMP" + | "TMP" + | "LANG" + | "TERM" + | "XDG_CONFIG_HOME" + ) +} + +fn is_mention_name_char(byte: u8) -> bool { + matches!(byte, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'_' | b'-' | b':') +} + +#[cfg(test)] +#[path = "mentions_tests.rs"] +mod tests; diff --git a/codex-rs/skills/src/mentions_tests.rs b/codex-rs/skills/src/mentions_tests.rs new file mode 100644 index 000000000000..b4f90e1d234f --- /dev/null +++ b/codex-rs/skills/src/mentions_tests.rs @@ -0,0 +1,80 @@ +use std::collections::HashSet; + +use pretty_assertions::assert_eq; + +use super::*; + +fn set<'a>(items: &'a [&'a str]) -> HashSet<&'a str> { + items.iter().copied().collect() +} + +fn assert_mentions(text: &str, expected_names: &[&str], expected_paths: &[&str]) { + let mentions = extract_tool_mentions(text); + assert_eq!(mentions.names, set(expected_names)); + assert_eq!(mentions.paths, set(expected_paths)); +} + +#[test] +fn handles_plain_and_linked_mentions() { + assert_mentions( + "use $alpha and [$beta](/tmp/beta)", + &["alpha", "beta"], + &["/tmp/beta"], + ); +} + +#[test] +fn skips_common_env_vars() { + assert_mentions("use $PATH and $alpha", &["alpha"], &[]); + assert_mentions("use [$HOME](/tmp/skill)", &[], &[]); + assert_mentions("use $XDG_CONFIG_HOME and $beta", &["beta"], &[]); +} + +#[test] +fn requires_link_syntax() { + assert_mentions("[beta](/tmp/beta)", &[], &[]); + assert_mentions("[$beta] /tmp/beta", &["beta"], &[]); + assert_mentions("[$beta]()", &["beta"], &[]); +} + +#[test] +fn trims_linked_paths_and_allows_spacing() { + assert_mentions("use [$beta] ( /tmp/beta )", &["beta"], &["/tmp/beta"]); +} + +#[test] +fn stops_at_non_name_chars() { + assert_mentions( + "use $alpha.skill and $beta_extra", + &["alpha", "beta_extra"], + &[], + ); +} + +#[test] +fn keeps_plugin_skill_namespaces() { + assert_mentions( + "use $slack:search and $alpha", + &["alpha", "slack:search"], + &[], + ); +} + +#[test] +fn requires_exact_name_boundaries() { + assert_mentions( + "use $notion-research-doc but not $notion-research-docs or $notion-research-doc_extra", + &[ + "notion-research-doc", + "notion-research-docs", + "notion-research-doc_extra", + ], + &[], + ); +} + +#[test] +fn handles_many_sigils_without_looping() { + let prefix = "$".repeat(256); + assert_mentions(&format!("{prefix} not-a-mention"), &[], &[]); +} diff --git a/codex-rs/core-skills/src/mention_counts.rs b/codex-rs/skills/src/name_counts.rs similarity index 96% rename from codex-rs/core-skills/src/mention_counts.rs rename to codex-rs/skills/src/name_counts.rs index b7482ca36ecc..78c9e13b3050 100644 --- a/codex-rs/core-skills/src/mention_counts.rs +++ b/codex-rs/skills/src/name_counts.rs @@ -1,9 +1,10 @@ use std::collections::HashMap; use std::collections::HashSet; -use super::SkillMetadata; use codex_utils_absolute_path::AbsolutePathBuf; +use crate::SkillMetadata; + /// Counts how often each skill name appears (exact and ASCII-lowercase), excluding disabled paths. pub fn build_skill_name_counts( skills: &[SkillMetadata],