From c6f639becb83aef6af1236357ee84a4db76f3969 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Thu, 25 Jun 2026 10:46:23 +0100 Subject: [PATCH] Share runtime skill rendering and selection --- codex-rs/connectors/src/snapshot.rs | 6 - codex-rs/connectors/src/snapshot_tests.rs | 15 +- codex-rs/core-skills/src/injection.rs | 343 +------- codex-rs/core-skills/src/injection_tests.rs | 303 +------ codex-rs/core-skills/src/lib.rs | 6 +- codex-rs/core-skills/src/render.rs | 299 ++++++- codex-rs/core-skills/src/runtime.rs | 79 +- codex-rs/core-skills/src/runtime_selection.rs | 180 +++++ .../src/runtime_selection_tests.rs | 66 ++ .../core-skills/src/skill_instructions.rs | 30 +- codex-rs/ext/skills/src/catalog.rs | 23 - codex-rs/ext/skills/src/config.rs | 10 - codex-rs/ext/skills/src/extension.rs | 319 +------- codex-rs/ext/skills/src/fragments.rs | 41 +- codex-rs/ext/skills/src/lib.rs | 13 - codex-rs/ext/skills/src/provider.rs | 66 -- codex-rs/ext/skills/src/provider/executor.rs | 166 ---- codex-rs/ext/skills/src/provider/host.rs | 129 --- .../ext/skills/src/provider/orchestrator.rs | 331 -------- codex-rs/ext/skills/src/render.rs | 74 +- codex-rs/ext/skills/src/selection.rs | 137 ---- codex-rs/ext/skills/src/sources.rs | 577 +++++++++----- codex-rs/ext/skills/src/state.rs | 106 +-- codex-rs/ext/skills/src/tools/list.rs | 5 +- codex-rs/ext/skills/src/tools/mod.rs | 44 +- codex-rs/ext/skills/src/tools/read.rs | 29 +- .../tests/executor_file_system_authority.rs | 312 -------- codex-rs/ext/skills/tests/skills_extension.rs | 738 ------------------ 28 files changed, 1089 insertions(+), 3358 deletions(-) create mode 100644 codex-rs/core-skills/src/runtime_selection.rs create mode 100644 codex-rs/core-skills/src/runtime_selection_tests.rs delete mode 100644 codex-rs/ext/skills/src/catalog.rs delete mode 100644 codex-rs/ext/skills/src/config.rs delete mode 100644 codex-rs/ext/skills/src/provider.rs delete mode 100644 codex-rs/ext/skills/src/provider/executor.rs delete mode 100644 codex-rs/ext/skills/src/provider/host.rs delete mode 100644 codex-rs/ext/skills/src/provider/orchestrator.rs delete mode 100644 codex-rs/ext/skills/src/selection.rs delete mode 100644 codex-rs/ext/skills/tests/executor_file_system_authority.rs delete mode 100644 codex-rs/ext/skills/tests/skills_extension.rs diff --git a/codex-rs/connectors/src/snapshot.rs b/codex-rs/connectors/src/snapshot.rs index a07b1ef589..92ad480242 100644 --- a/codex-rs/connectors/src/snapshot.rs +++ b/codex-rs/connectors/src/snapshot.rs @@ -8,7 +8,6 @@ use codex_plugin::PluginCapabilitySummary; /// Connector declarations contributed by one plugin package. #[derive(Clone, Debug, PartialEq, Eq)] pub struct PluginConnectorSource { - plugin_id: String, plugin_display_name: String, connector_ids: Vec, } @@ -16,12 +15,10 @@ pub struct PluginConnectorSource { impl PluginConnectorSource { /// Creates one plugin source from parsed app declarations. pub fn new( - plugin_id: impl Into, plugin_display_name: impl Into, declarations: impl IntoIterator, ) -> Self { Self::from_connector_ids( - plugin_id, plugin_display_name, declarations .into_iter() @@ -31,7 +28,6 @@ impl PluginConnectorSource { /// Creates one plugin source from connector IDs that were already parsed. pub fn from_connector_ids( - plugin_id: impl Into, plugin_display_name: impl Into, connector_ids: impl IntoIterator, ) -> Self { @@ -42,7 +38,6 @@ impl PluginConnectorSource { .filter(|connector_id| seen_connector_ids.insert(connector_id.clone())) .collect(); Self { - plugin_id: plugin_id.into(), plugin_display_name: plugin_display_name.into(), connector_ids, } @@ -105,7 +100,6 @@ impl ConnectorSnapshot { pub fn from_plugin_capability_summaries(summaries: &[PluginCapabilitySummary]) -> Self { Self::from_plugin_sources(summaries.iter().map(|summary| { PluginConnectorSource::from_connector_ids( - summary.config_name.clone(), summary.display_name.clone(), summary.app_connector_ids.clone(), ) diff --git a/codex-rs/connectors/src/snapshot_tests.rs b/codex-rs/connectors/src/snapshot_tests.rs index 3087bc73cd..9322f5b07b 100644 --- a/codex-rs/connectors/src/snapshot_tests.rs +++ b/codex-rs/connectors/src/snapshot_tests.rs @@ -6,14 +6,12 @@ use super::PluginConnectorSource; #[test] fn snapshot_merges_sources_in_order_and_dedupes_provenance() { - let host_source = source("host", "Zulu", &["calendar", "calendar"]); - let host = ConnectorSnapshot::from_plugin_sources([ - source("skills", "Skills only", &[]), - host_source.clone(), - ]); + let host_source = source("Zulu", &["calendar", "calendar"]); + let host = + ConnectorSnapshot::from_plugin_sources([source("Skills only", &[]), host_source.clone()]); let selected = ConnectorSnapshot::from_plugin_sources([ - source("selected-a", "Alpha", &["drive", "calendar"]), - source("selected-b", "Alpha", &["calendar"]), + source("Alpha", &["drive", "calendar"]), + source("Alpha", &["calendar"]), ]); let merged = host.merged_with(&selected); @@ -36,9 +34,8 @@ fn snapshot_merges_sources_in_order_and_dedupes_provenance() { ); } -fn source(id: &str, display_name: &str, connector_ids: &[&str]) -> PluginConnectorSource { +fn source(display_name: &str, connector_ids: &[&str]) -> PluginConnectorSource { PluginConnectorSource::from_connector_ids( - id, display_name, connector_ids .iter() diff --git a/codex-rs/core-skills/src/injection.rs b/codex-rs/core-skills/src/injection.rs index 201e7959fb..0fdd8b40a6 100644 --- a/codex-rs/core-skills/src/injection.rs +++ b/codex-rs/core-skills/src/injection.rs @@ -1,228 +1,13 @@ -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; -use codex_analytics::TrackEventsContext; -use codex_exec_server::LOCAL_FS; -use codex_otel::SessionTelemetry; -use codex_protocol::user_input::UserInput; -use codex_utils_absolute_path::AbsolutePathBuf; -use codex_utils_path_uri::PathUri; use codex_utils_plugins::mention_syntax::TOOL_MENTION_SIGIL; -#[derive(Debug, Default)] -pub struct SkillInjections { - pub items: Vec, - pub warnings: Vec, -} - -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct SkillInjection { - pub name: String, - pub path: String, - pub contents: String, -} - -/// Host skill prompts that have already been injected by an extension for this -/// turn. -/// -/// Core uses this to keep the legacy skill-injection path from sending the same -/// host `SKILL.md` body again while the skills extension is being wired in. -#[derive(Debug, Clone, Default, PartialEq, Eq)] -pub struct InjectedHostSkillPrompts { - paths: HashSet, -} - -impl InjectedHostSkillPrompts { - pub fn insert_path(&mut self, path: impl Into) { - let path = path.into(); - self.paths.insert(normalize_host_skill_path(&path)); - self.paths.insert(path); - } - - pub fn is_empty(&self) -> bool { - self.paths.is_empty() - } - - pub fn contains_path(&self, path: &str) -> bool { - self.paths.contains(path) || self.paths.contains(&normalize_host_skill_path(path)) - } -} - -#[tracing::instrument( - level = "trace", - skip_all, - fields(mentioned_skill_count = mentioned_skills.len()) -)] -pub async fn build_skill_injections( - mentioned_skills: &[SkillMetadata], - loaded_skills: Option<&SkillLoadOutcome>, - otel: Option<&SessionTelemetry>, - analytics_client: &AnalyticsEventsClient, - tracking: TrackEventsContext, -) -> SkillInjections { - if mentioned_skills.is_empty() { - return SkillInjections::default(); - } - - let mut result = SkillInjections { - items: Vec::with_capacity(mentioned_skills.len()), - warnings: Vec::new(), - }; - 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)); - let path = PathUri::from_abs_path(&skill.path_to_skills_md); - match fs.read_file_text(&path, /*sandbox*/ None).await { - Ok(contents) => { - emit_skill_injected_metric(otel, skill, "ok"); - invocations.push(SkillInvocation { - skill_name: skill.name.clone(), - skill_scope: skill.scope, - skill_path: skill.path_to_skills_md.to_path_buf(), - plugin_id: skill.plugin_id.clone(), - invocation_type: InvocationType::Explicit, - }); - result.items.push(SkillInjection { - name: skill.name.clone(), - path: skill.path_to_skills_md.to_string_lossy().into_owned(), - contents, - }); - } - Err(err) => { - emit_skill_injected_metric(otel, skill, "error"); - let message = format!( - "Failed to load skill {name} at {path}: {err:#}", - name = skill.name, - path = skill.path_to_skills_md.display() - ); - result.warnings.push(message); - } - } - } - - analytics_client.track_skill_invocations(tracking, invocations); - - result -} - -fn normalize_host_skill_path(path: &str) -> String { - normalize_skill_path(path).replace('\\', "/") -} - -fn emit_skill_injected_metric( - otel: Option<&SessionTelemetry>, - skill: &SkillMetadata, - status: &str, -) { - let Some(otel) = otel else { - return; - }; - - otel.counter( - "codex.skill.injected", - /*inc*/ 1, - &[("status", status), ("skill", skill.name.as_str())], - ); -} - -/// 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 `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 -/// 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], - skills: &[SkillMetadata], - disabled_paths: &HashSet, - connector_slug_counts: &HashMap, -) -> Vec { - let skill_name_counts = build_skill_name_counts(skills, disabled_paths).0; - - let selection_context = SkillSelectionContext { - skills, - disabled_paths, - 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; - }; - - if selection_context.disabled_paths.contains(&path) || seen_paths.contains(&path) { - continue; - } - - if let Some(skill) = selection_context - .skills - .iter() - .find(|skill| skill.path_to_skills_md == path) - { - 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> { - skills: &'a [SkillMetadata], - disabled_paths: &'a HashSet, - skill_name_counts: &'a HashMap, - 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() } @@ -276,22 +61,16 @@ pub fn plugin_config_name_from_path(path: &str) -> Option<&str> { .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. +/// Supports explicit resource links in the form `[$tool-name](resource path)`. +/// Linked mentions are selected by their exact path rather than falling back to the name. 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(); @@ -303,12 +82,6 @@ pub fn extract_tool_mentions_with_sigil(text: &str, sigil: char) -> ToolMentions 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; @@ -339,97 +112,17 @@ pub fn extract_tool_mentions_with_sigil(text: &str, sigil: char) -> ToolMentions 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<'_>, - 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<&str> = mentions - .paths() - .filter(|path| { - !matches!( - tool_kind_for_path(path), - ToolMentionKind::App | ToolMentionKind::Mcp | ToolMentionKind::Plugin - ) - }) - .map(normalize_skill_path) - .collect(); - - 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) - { - 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()); - seen_names.insert(skill.name.clone()); - selected.push(skill.clone()); - } - } - - 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) - { - continue; - } - - if blocked_plain_names.contains(skill.name.as_str()) { - continue; - } - if !mentions.plain_names.contains(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 parse_linked_tool_mention<'a>( text: &'a str, text_bytes: &[u8], @@ -505,38 +198,6 @@ fn is_common_env_var(name: &str) -> bool { ) } -#[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':') } diff --git a/codex-rs/core-skills/src/injection_tests.rs b/codex-rs/core-skills/src/injection_tests.rs index 78aa195895..d81104b3fa 100644 --- a/codex-rs/core-skills/src/injection_tests.rs +++ b/codex-rs/core-skills/src/injection_tests.rs @@ -1,94 +1,22 @@ use super::*; -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::collections::HashSet; -fn make_skill(name: &str, path: &str) -> SkillMetadata { - SkillMetadata { - name: name.to_string(), - description: format!("{name} skill"), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: test_path_buf(path).abs(), - scope: codex_protocol::protocol::SkillScope::User, - plugin_id: None, - } -} - 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.plain_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()) -} - -fn collect_mentions( - inputs: &[UserInput], - skills: &[SkillMetadata], - disabled_paths: &HashSet, - connector_slug_counts: &HashMap, -) -> Vec { - collect_explicit_skill_mentions(inputs, skills, disabled_paths, connector_slug_counts) -} - -#[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"], + &["alpha"], &["/tmp/beta"], ); } @@ -129,230 +57,3 @@ fn extract_tool_mentions_keeps_plugin_skill_namespaces() { &[], ); } - -#[test] -fn collect_explicit_skill_mentions_text_respects_skill_order() { - let alpha = make_skill("alpha-skill", "/tmp/alpha"); - let beta = make_skill("beta-skill", "/tmp/beta"); - let skills = vec![beta.clone(), alpha.clone()]; - let inputs = vec![UserInput::Text { - text: "first $alpha-skill then $beta-skill".to_string(), - text_elements: Vec::new(), - }]; - let connector_counts = HashMap::new(); - - let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); - - // Text scanning should not change the previous selection ordering semantics. - assert_eq!(selected, vec![beta, alpha]); -} - -#[test] -fn collect_explicit_skill_mentions_prioritizes_structured_inputs() { - let alpha = make_skill("alpha-skill", "/tmp/alpha"); - let beta = make_skill("beta-skill", "/tmp/beta"); - let skills = vec![alpha.clone(), beta.clone()]; - let inputs = vec![ - UserInput::Text { - text: "please run $alpha-skill".to_string(), - text_elements: Vec::new(), - }, - UserInput::Skill { - name: "beta-skill".to_string(), - path: test_path_buf("/tmp/beta"), - }, - ]; - let connector_counts = HashMap::new(); - - let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); - - assert_eq!(selected, vec![beta, alpha]); -} - -#[test] -fn collect_explicit_skill_mentions_skips_invalid_structured_and_blocks_plain_fallback() { - let alpha = make_skill("alpha-skill", "/tmp/alpha"); - let skills = vec![alpha]; - let inputs = vec![ - UserInput::Text { - text: "please run $alpha-skill".to_string(), - text_elements: Vec::new(), - }, - UserInput::Skill { - name: "alpha-skill".to_string(), - path: test_path_buf("/tmp/missing"), - }, - ]; - 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_disabled_structured_and_blocks_plain_fallback() { - let alpha = make_skill("alpha-skill", "/tmp/alpha"); - let skills = vec![alpha]; - let inputs = vec![ - UserInput::Text { - text: "please run $alpha-skill".to_string(), - text_elements: Vec::new(), - }, - UserInput::Skill { - name: "alpha-skill".to_string(), - 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); - - assert_eq!(selected, Vec::new()); -} - -#[test] -fn collect_explicit_skill_mentions_dedupes_by_path() { - let alpha = make_skill("alpha-skill", "/tmp/alpha"); - let skills = vec![alpha.clone()]; - let mention = linked_skill_mention("alpha-skill", "/tmp/alpha"); - let inputs = vec![UserInput::Text { - text: format!("use {mention} and {mention}"), - text_elements: Vec::new(), - }]; - let connector_counts = HashMap::new(); - - let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); - - assert_eq!(selected, vec![alpha]); -} - -#[test] -fn collect_explicit_skill_mentions_skips_ambiguous_name() { - let alpha = make_skill("demo-skill", "/tmp/alpha"); - let beta = make_skill("demo-skill", "/tmp/beta"); - let skills = vec![alpha, beta]; - let inputs = vec![UserInput::Text { - text: "use $demo-skill and again $demo-skill".to_string(), - 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_prefers_linked_path_over_name() { - let alpha = make_skill("demo-skill", "/tmp/alpha"); - let beta = make_skill("demo-skill", "/tmp/beta"); - let skills = vec![alpha, beta.clone()]; - let inputs = vec![UserInput::Text { - text: format!( - "use $demo-skill and {}", - linked_skill_mention("demo-skill", "/tmp/beta") - ), - text_elements: Vec::new(), - }]; - let connector_counts = HashMap::new(); - - let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); - - assert_eq!(selected, vec![beta]); -} - -#[test] -fn collect_explicit_skill_mentions_skips_plain_name_when_connector_matches() { - let alpha = make_skill("alpha-skill", "/tmp/alpha"); - let skills = vec![alpha]; - let inputs = vec![UserInput::Text { - text: "use $alpha-skill".to_string(), - text_elements: Vec::new(), - }]; - let connector_counts = HashMap::from([("alpha-skill".to_string(), 1)]); - - let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); - - assert_eq!(selected, Vec::new()); -} - -#[test] -fn collect_explicit_skill_mentions_allows_explicit_path_with_connector_conflict() { - let alpha = make_skill("alpha-skill", "/tmp/alpha"); - let skills = vec![alpha.clone()]; - let inputs = vec![UserInput::Text { - text: format!("use {}", linked_skill_mention("alpha-skill", "/tmp/alpha")), - text_elements: Vec::new(), - }]; - let connector_counts = HashMap::from([("alpha-skill".to_string(), 1)]); - - let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); - - assert_eq!(selected, vec![alpha]); -} - -#[test] -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 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); - - assert_eq!(selected, Vec::new()); -} - -#[test] -fn collect_explicit_skill_mentions_prefers_resource_path() { - let alpha = make_skill("demo-skill", "/tmp/alpha"); - let beta = make_skill("demo-skill", "/tmp/beta"); - let skills = vec![alpha, beta.clone()]; - let inputs = vec![UserInput::Text { - text: format!("use {}", linked_skill_mention("demo-skill", "/tmp/beta")), - text_elements: Vec::new(), - }]; - let connector_counts = HashMap::new(); - - let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); - - assert_eq!(selected, vec![beta]); -} - -#[test] -fn collect_explicit_skill_mentions_skips_missing_path_with_no_fallback() { - let alpha = make_skill("demo-skill", "/tmp/alpha"); - let beta = make_skill("demo-skill", "/tmp/beta"); - let skills = vec![alpha, beta]; - let inputs = vec![UserInput::Text { - text: format!("use {}", linked_skill_mention("demo-skill", "/tmp/missing")), - 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_missing_path_without_fallback() { - let alpha = make_skill("demo-skill", "/tmp/alpha"); - let skills = vec![alpha]; - let inputs = vec![UserInput::Text { - text: format!("use {}", linked_skill_mention("demo-skill", "/tmp/missing")), - text_elements: Vec::new(), - }]; - let connector_counts = HashMap::new(); - - let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); - - assert_eq!(selected, Vec::new()); -} diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs index 3539bf28c0..89e2a63b50 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -8,8 +8,8 @@ pub mod remote; pub mod render; mod root_loader; pub mod runtime; +mod runtime_selection; pub mod service; -mod skill_instructions; pub mod system; pub(crate) use invocation_utils::build_implicit_skill_path_indexes; @@ -28,10 +28,12 @@ pub use render::SKILLS_INTRO_WITH_ABSOLUTE_PATHS; pub use render::SKILLS_INTRO_WITH_ALIASES; pub use render::SkillMetadataBudget; pub use render::SkillRenderReport; +pub use render::SkillRenderSideEffects; pub use render::build_available_skills; +pub use render::build_available_skills_from_catalog; pub use render::default_skill_metadata_budget; pub use render::render_available_skills_body; pub use root_loader::PluginSkillSnapshots; +pub use runtime_selection::collect_runtime_skill_mentions; pub use service::SkillsLoadInput; pub use service::SkillsService; -pub use skill_instructions::SkillInstructions; diff --git a/codex-rs/core-skills/src/render.rs b/codex-rs/core-skills/src/render.rs index 89ea1675d0..5cbcae053e 100644 --- a/codex-rs/core-skills/src/render.rs +++ b/codex-rs/core-skills/src/render.rs @@ -6,6 +6,9 @@ use std::path::Path; use crate::model::SkillLoadOutcome; use crate::model::SkillMetadata; +use crate::runtime::SkillCatalog; +use crate::runtime::SkillCatalogEntry; +use crate::runtime::SkillSourceKind; use codex_otel::SessionTelemetry; use codex_otel::THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC; use codex_otel::THREAD_SKILLS_ENABLED_TOTAL_METRIC; @@ -16,6 +19,7 @@ use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_output_truncation::approx_token_count; const DEFAULT_SKILL_METADATA_CHAR_BUDGET: usize = 8_000; +const MAX_RUNTIME_SKILL_METADATA_BYTES: usize = 8_000; const SKILL_METADATA_CONTEXT_WINDOW_PERCENT: usize = 2; const MAX_DEFAULT_CONTEXT_SKILL_DESCRIPTION_CHARS: usize = 1_024; const TRUNCATED_SKILL_DESCRIPTION_SUFFIX: &str = "..."; @@ -25,9 +29,9 @@ pub const SKILL_DESCRIPTION_TRUNCATED_WARNING: &str = "Skill descriptions were s pub const SKILL_DESCRIPTION_TRUNCATED_WARNING_WITH_PERCENT: &str = "Skill descriptions were shortened to fit the 2% skills context budget. Codex can still see every skill, but some descriptions are shorter. Disable unused skills or plugins to leave more room for the rest."; pub const SKILL_DESCRIPTIONS_REMOVED_WARNING_PREFIX: &str = "Exceeded skills context budget. All skill descriptions were removed and"; -pub const SKILLS_INTRO_WITH_ABSOLUTE_PATHS: &str = "A skill is a set of instructions provided through a `SKILL.md` source. Below is the list of skills that can be used. Each entry includes a name, description, and source locator. `file` locators are on the host filesystem, `environment resource` locators are owned by an execution environment, `orchestrator resource` locators are opaque non-filesystem resources, and `custom resource` locators use their provider's access mechanism."; +pub const SKILLS_INTRO_WITH_ABSOLUTE_PATHS: &str = "A skill is a set of instructions provided through a `SKILL.md` source. Below is the list of skills that can be used. Each entry includes a name, description, and source locator. `file` locators are on the host filesystem, `environment resource` locators are owned by an execution environment, and `orchestrator resource` locators are opaque non-filesystem resources."; pub const SKILLS_INTRO_WITH_ALIASES: &str = "A skill is a set of local instructions to follow that is stored in a `SKILL.md` file. Below is the list of skills that can be used. Each entry includes a name, description, and a short path that can be expanded into an absolute path using the skill roots table."; -pub const SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS: &str = r###"- Discovery: The list above is the skills available in this session (name + description + source locator). `file` entries live on the host filesystem, `environment resource` entries are owned by their execution environment, `orchestrator resource` entries must be accessed through `skills.list` and `skills.read`, and `custom resource` entries use their provider's access mechanism. +pub const SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS: &str = r###"- Discovery: The list above is the skills available in this session (name + description + source locator). `file` entries live on the host filesystem, `environment resource` entries are owned by their execution environment, and `orchestrator resource` entries must be accessed through `skills.list` and `skills.read`. - Trigger rules: If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a skill's description shown above, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned. - Missing/blocked: If a named skill isn't in the list or its source can't be read, say so briefly and continue with the best fallback. - How to use a skill (progressive disclosure): @@ -202,6 +206,103 @@ pub fn build_available_skills( Some(selected) } +/// Renders one authority-aware runtime catalog with the same budget, report, +/// warnings, aliases, and metrics as host-only skills. +pub fn build_available_skills_from_catalog( + catalog: &SkillCatalog, + host_outcome: Option<&SkillLoadOutcome>, + budget: SkillMetadataBudget, + side_effects: SkillRenderSideEffects<'_>, +) -> Option { + let mut entries = catalog + .entries + .iter() + .filter(|entry| entry.enabled && entry.prompt_visible) + .collect::>(); + if entries.is_empty() { + record_skill_render_side_effects( + side_effects, + /*total_count*/ 0, + /*included_count*/ 0, + /*omitted_count*/ 0, + /*truncated_description_chars*/ 0, + ); + return None; + } + if let Some(outcome) = host_outcome { + let host_paths = + ordered_skills_for_budget(&outcome.allowed_skills_for_implicit_invocation()) + .into_iter() + .map(|skill| { + normalized_path(&skill.path_to_skills_md.to_string_lossy()).into_owned() + }) + .collect::>(); + entries.sort_by(|left, right| { + let host_position = |entry: &SkillCatalogEntry| { + (entry.authority.kind == SkillSourceKind::Host).then(|| { + let path = normalized_path(entry.main_prompt.as_str()); + host_paths + .iter() + .position(|host_path| host_path == path.as_ref()) + .unwrap_or(host_paths.len()) + }) + }; + match (host_position(left), host_position(right)) { + (Some(left), Some(right)) => left.cmp(&right), + (Some(_), None) => std::cmp::Ordering::Less, + (None, Some(_)) => std::cmp::Ordering::Greater, + (None, None) => left + .authority + .kind + .to_string() + .cmp(&right.authority.kind.to_string()) + .then_with(|| left.authority.id.cmp(&right.authority.id)) + .then_with(|| left.id.0.cmp(&right.id.0)), + } + }); + } + + let absolute = build_available_skills_from_lines( + entries + .iter() + .map(|entry| SkillLine::from_catalog_entry(entry)) + .collect(), + entries.len(), + budget, + SkillPathAliases::default(), + ) + .and_then(|available| bound_runtime_skill_metadata_bytes(available, budget)); + let should_try_aliases = absolute.as_ref().is_none_or(|absolute| { + absolute.report.omitted_count > 0 || absolute.report.truncated_description_chars > 0 + }); + let aliased = if should_try_aliases + && entries + .iter() + .all(|entry| entry.authority.kind == SkillSourceKind::Host) + { + host_outcome + .and_then(|outcome| { + build_aliased_available_skills_from_catalog(outcome, &entries, budget) + }) + .and_then(|available| bound_runtime_skill_metadata_bytes(available, budget)) + } else { + None + }; + let selected = match (absolute, aliased) { + (Some(absolute), Some(aliased)) + if aliased_render_is_better(&aliased, &absolute, budget) => + { + aliased + } + (Some(absolute), _) => absolute, + (None, Some(aliased)) => aliased, + (None, None) => return None, + }; + + record_available_skills_side_effects(&selected, budget, side_effects); + Some(selected) +} + fn build_available_skills_from_lines( skill_lines: Vec>, total_count: usize, @@ -213,7 +314,58 @@ fn build_available_skills_from_lines( } let (skill_lines, report) = render_skill_lines_from_lines(skill_lines, total_count, budget); - let warning_message = if report.omitted_count > 0 { + let warning_message = skill_metadata_warning(&report, budget); + let available = AvailableSkills { + skill_root_lines: path_aliases.skill_root_lines, + skill_lines, + report, + warning_message, + }; + Some(available) +} + +fn bound_runtime_skill_metadata_bytes( + mut available: AvailableSkills, + budget: SkillMetadataBudget, +) -> Option { + let mut used_bytes = available + .skill_root_lines + .iter() + .map(|line| line.len().saturating_add(1)) + .sum::(); + if used_bytes > MAX_RUNTIME_SKILL_METADATA_BYTES { + return None; + } + + available.skill_lines.retain(|line| { + let line_bytes = line.len().saturating_add(1); + let Some(next_bytes) = used_bytes.checked_add(line_bytes) else { + return false; + }; + if next_bytes > MAX_RUNTIME_SKILL_METADATA_BYTES { + return false; + } + used_bytes = next_bytes; + true + }); + if available.skill_lines.is_empty() { + return None; + } + + available.report.included_count = available.skill_lines.len(); + available.report.omitted_count = available + .report + .total_count + .saturating_sub(available.report.included_count); + available.warning_message = skill_metadata_warning(&available.report, budget); + Some(available) +} + +fn skill_metadata_warning( + report: &SkillRenderReport, + budget: SkillMetadataBudget, +) -> Option { + if report.omitted_count > 0 { let skill_word = if report.omitted_count == 1 { "skill" } else { @@ -243,14 +395,7 @@ fn build_available_skills_from_lines( ) } else { None - }; - let available = AvailableSkills { - skill_root_lines: path_aliases.skill_root_lines, - skill_lines, - report, - warning_message, - }; - Some(available) + } } fn record_available_skills_side_effects( @@ -450,6 +595,7 @@ impl SkillRenderReport { struct SkillLine<'a> { name: &'a str, description: Cow<'a, str>, + locator: &'static str, path: String, } @@ -492,6 +638,29 @@ impl<'a> SkillLine<'a> { Self { name: skill.name.as_str(), description, + locator: "file", + path, + } + } + + fn from_catalog_entry(entry: &'a SkillCatalogEntry) -> Self { + Self::from_catalog_entry_with_path(entry, entry.rendered_path().to_string()) + } + + fn from_catalog_entry_with_path(entry: &'a SkillCatalogEntry, path: String) -> Self { + let locator = match &entry.authority.kind { + SkillSourceKind::Host => "file", + SkillSourceKind::Executor => "environment resource", + SkillSourceKind::Orchestrator => "orchestrator resource", + }; + let description = entry + .short_description + .as_deref() + .unwrap_or(entry.description.as_str()); + Self { + name: &entry.name, + description: truncate_default_context_skill_description(description), + locator, path, } } @@ -525,19 +694,25 @@ impl<'a> SkillLine<'a> { fn render_with_description_chars(&self, description_chars: usize) -> String { if description_chars == 0 { - format!("- {}: (file: {})", self.name, self.path) + format!("- {}: ({}: {})", self.name, self.locator, self.path) } else { let end = self.rendered_description_prefix_len(description_chars); let description = &self.description.as_ref()[..end]; - format!("- {}: {} (file: {})", self.name, description, self.path) + format!( + "- {}: {} ({}: {})", + self.name, description, self.locator, self.path + ) } } fn render_with_description(&self, description: &str) -> String { if description.is_empty() { - format!("- {}: (file: {})", self.name, self.path) + format!("- {}: ({}: {})", self.name, self.locator, self.path) } else { - format!("- {}: {} (file: {})", self.name, description, self.path) + format!( + "- {}: {} ({}: {})", + self.name, description, self.locator, self.path + ) } } } @@ -682,6 +857,57 @@ fn build_aliased_available_skills( build_available_skills_from_lines(skill_lines, skills.len(), adjusted_budget, plan.aliases) } +fn build_aliased_available_skills_from_catalog( + outcome: &SkillLoadOutcome, + entries: &[&SkillCatalogEntry], + budget: SkillMetadataBudget, +) -> Option { + let host_skills = outcome + .allowed_skills_for_implicit_invocation() + .into_iter() + .filter(|skill| { + entries.iter().any(|entry| { + entry.authority.kind == SkillSourceKind::Host + && normalized_path(entry.main_prompt.as_str()) + == normalized_path(&skill.path_to_skills_md.to_string_lossy()) + }) + }) + .collect::>(); + let plan = build_alias_plan(outcome, &host_skills, budget)?; + if plan.table_cost >= budget.limit() { + return None; + } + let adjusted_limit = budget.limit().saturating_sub(plan.table_cost); + let adjusted_budget = match budget { + SkillMetadataBudget::Tokens(_) => SkillMetadataBudget::Tokens(adjusted_limit), + SkillMetadataBudget::Characters(_) => SkillMetadataBudget::Characters(adjusted_limit), + }; + let skill_lines = entries + .iter() + .map(|entry| { + let path = host_skills + .iter() + .find(|skill| { + entry.authority.kind == SkillSourceKind::Host + && normalized_path(entry.main_prompt.as_str()) + == normalized_path(&skill.path_to_skills_md.to_string_lossy()) + }) + .map(|skill| render_skill_path_with_aliases(skill, &plan)) + .unwrap_or_else(|| entry.rendered_path().to_string()); + SkillLine::from_catalog_entry_with_path(entry, path) + }) + .collect(); + build_available_skills_from_lines(skill_lines, entries.len(), adjusted_budget, plan.aliases) +} + +fn normalized_path(path: &str) -> Cow<'_, str> { + if path.contains('\\') { + Cow::Owned(path.replace('\\', "/")) + } else { + Cow::Borrowed(path) + } +} + #[derive(Debug, Clone, Default, PartialEq, Eq)] struct SkillPathAliases { skill_root_lines: Vec, @@ -932,6 +1158,9 @@ mod tests { use std::collections::HashMap; use std::sync::Arc; + use crate::runtime::SkillAuthority; + use crate::runtime::SkillPackageId; + use crate::runtime::SkillResourceId; use codex_utils_absolute_path::test_support::PathBufExt; use codex_utils_absolute_path::test_support::test_path_buf; use pretty_assertions::assert_eq; @@ -960,6 +1189,46 @@ mod tests { skill } + #[test] + fn catalog_rendering_keeps_a_hard_byte_cap_for_multibyte_metadata() { + let description = "💡".repeat(MAX_DEFAULT_CONTEXT_SKILL_DESCRIPTION_CHARS + 1); + let entries = ["alpha", "beta"] + .into_iter() + .map(|name| { + SkillCatalogEntry::new( + SkillPackageId(name.to_string()), + SkillAuthority::new(SkillSourceKind::Executor, "executor"), + name, + description.clone(), + SkillResourceId::new(format!("{name}/SKILL.md")), + ) + }) + .collect(); + let catalog = SkillCatalog { + entries, + warnings: Vec::new(), + }; + + let rendered = build_available_skills_from_catalog( + &catalog, + /*host_outcome*/ None, + SkillMetadataBudget::Characters(usize::MAX), + SkillRenderSideEffects::None, + ) + .expect("one catalog entry should fit"); + let metadata_bytes = rendered + .skill_root_lines + .iter() + .chain(&rendered.skill_lines) + .map(|line| line.len().saturating_add(1)) + .sum::(); + + assert!(metadata_bytes <= MAX_RUNTIME_SKILL_METADATA_BYTES); + assert_eq!(rendered.report.included_count, 1); + assert_eq!(rendered.report.omitted_count, 1); + assert!(rendered.warning_message.is_some()); + } + fn expected_skill_line(skill: &SkillMetadata, description: &str) -> String { SkillLine::new(skill).render_with_description(description) } diff --git a/codex-rs/core-skills/src/runtime.rs b/codex-rs/core-skills/src/runtime.rs index bad8460fdd..c5925d7207 100644 --- a/codex-rs/core-skills/src/runtime.rs +++ b/codex-rs/core-skills/src/runtime.rs @@ -1,4 +1,5 @@ use std::any::Any; +use std::collections::HashMap; use std::fmt; use std::future::Future; use std::hash::Hash; @@ -20,21 +21,14 @@ pub enum SkillSourceKind { Executor, /// Skills owned by the orchestrator rather than an execution environment. Orchestrator, - /// A source kind for providers that do not fit an existing transport category. - Custom(String), } impl SkillSourceKind { - pub fn custom(kind: impl Into) -> Self { - Self::Custom(kind.into()) - } - fn as_str(&self) -> &str { match self { Self::Host => "host", Self::Executor => "executor", Self::Orchestrator => "orchestrator", - Self::Custom(kind) => kind, } } } @@ -189,13 +183,6 @@ pub struct SkillCatalog { } impl SkillCatalog { - pub fn extend(&mut self, other: SkillCatalog) { - for entry in other.entries { - self.push_entry(entry); - } - self.warnings.extend(other.warnings); - } - pub fn push_entry(&mut self, entry: SkillCatalogEntry) { if self .entries @@ -290,9 +277,6 @@ impl Hash for SkillSourceIdentity { /// A resource returned by [`SkillSource::list`] must be read through the same /// source instead of being converted into an ambient local path. pub trait SkillSource: Send + Sync { - /// Returns the authority category owned by this source. - fn kind(&self) -> SkillSourceKind; - /// Returns the stable identity of the runtime owner captured by this source. fn identity(&self) -> SkillSourceIdentity; @@ -306,7 +290,7 @@ pub trait SkillSource: Send + Sync { #[derive(Clone)] struct RegisteredSkillSource { label: String, - source: Arc, + source: Arc Arc + Send + Sync>, } /// Bound skill sources used to build and read one runtime catalog. @@ -321,6 +305,18 @@ impl SkillSources { } pub fn with_source(mut self, label: impl Into, source: Arc) -> Self { + self.sources.push(RegisteredSkillSource { + label: label.into(), + source: Arc::new(move || Arc::clone(&source)), + }); + self + } + + pub fn with_source_factory( + mut self, + label: impl Into, + source: Arc Arc + Send + Sync>, + ) -> Self { self.sources.push(RegisteredSkillSource { label: label.into(), source, @@ -328,36 +324,39 @@ impl SkillSources { self } - pub async fn list(&self) -> SkillCatalog { + pub fn extend(&mut self, other: Self) { + self.sources.extend(other.sources); + } + + pub(crate) async fn list_with_sources( + &self, + ) -> ( + SkillCatalog, + HashMap<(SkillAuthority, SkillPackageId), Arc>, + ) { let mut catalog = SkillCatalog::default(); + let mut sources = HashMap::new(); for source in &self.sources { - match source.source.list().await { - Ok(source_catalog) => catalog.extend(source_catalog), + let bound_source = (source.source)(); + match bound_source.list().await { + Ok(source_catalog) => { + catalog.warnings.extend(source_catalog.warnings); + for entry in source_catalog.entries { + let key = (entry.authority.clone(), entry.id.clone()); + if let std::collections::hash_map::Entry::Vacant(route) = sources.entry(key) + { + route.insert(Arc::clone(&bound_source)); + catalog.entries.push(entry); + } + } + } Err(err) => catalog.warnings.push(format!( "{} skills unavailable: {}", source.label, err.message )), } } - catalog - } - - pub async fn list_kind(&self, kind: &SkillSourceKind) -> SkillSourceResult { - let mut catalog = SkillCatalog::default(); - for source in self - .sources - .iter() - .filter(|source| source.source.kind() == *kind) - { - let source_catalog = source.source.list().await.map_err(|err| { - SkillSourceError::new(format!( - "{} skills unavailable: {}", - source.label, err.message - )) - })?; - catalog.extend(source_catalog); - } - Ok(catalog) + (catalog, sources) } } diff --git a/codex-rs/core-skills/src/runtime_selection.rs b/codex-rs/core-skills/src/runtime_selection.rs new file mode 100644 index 0000000000..40d5c70948 --- /dev/null +++ b/codex-rs/core-skills/src/runtime_selection.rs @@ -0,0 +1,180 @@ +use std::collections::HashSet; + +use codex_protocol::user_input::UserInput; + +use crate::injection::extract_tool_mentions; +use crate::runtime::SkillAuthority; +use crate::runtime::SkillCatalog; +use crate::runtime::SkillCatalogEntry; +use crate::runtime::SkillPackageId; +use crate::runtime::SkillSourceKind; + +const SKILL_PATH_PREFIX: &str = "skill://"; + +/// Selects explicit skill mentions from one authority-aware runtime catalog. +/// +/// Exact locators win. Plain names prefer executor, then orchestrator, then +/// host skills, and must be unique within the first matching authority kind. +/// Plain names that collide with another tool namespace are ignored. +/// A structured selection blocks a plain-name fallback even when its locator +/// no longer exists. +pub fn collect_runtime_skill_mentions( + inputs: &[UserInput], + catalog: &SkillCatalog, + plain_name_conflicts: &HashSet, +) -> Vec { + let mut selected = Vec::new(); + let mut seen = HashSet::new(); + let mut blocked_plain_names = HashSet::new(); + + for input in inputs { + match input { + UserInput::Skill { name, path } => { + blocked_plain_names.insert(name.clone()); + select_by_path(catalog, &path.to_string_lossy(), &mut seen, &mut selected); + } + UserInput::Mention { name, path } if path_is_skill(path) => { + blocked_plain_names.insert(name.clone()); + select_by_path(catalog, path, &mut seen, &mut selected); + } + _ => {} + } + } + + for input in inputs { + let UserInput::Text { text, .. } = input else { + continue; + }; + let mentions = extract_tool_mentions(text); + select_text_mentions( + catalog, + &mentions, + &blocked_plain_names, + plain_name_conflicts, + &mut seen, + &mut selected, + ); + } + + selected +} + +fn select_text_mentions( + catalog: &SkillCatalog, + mentions: &crate::injection::ToolMentions<'_>, + blocked_plain_names: &HashSet, + plain_name_conflicts: &HashSet, + seen: &mut HashSet, + selected: &mut Vec, +) { + let mentioned_paths = mentions + .paths() + .filter(|path| path_is_skill(path)) + .map(normalize_skill_path) + .collect::>(); + for entry in catalog.entries.iter().filter(|entry| entry.enabled) { + if entry_paths(entry) + .into_iter() + .any(|path| mentioned_paths.contains(normalize_skill_path(path))) + { + push_selected(entry, seen, selected); + } + } + + let selected_names = mentions + .plain_names() + .filter(|name| !blocked_plain_names.contains(*name)) + .filter(|name| !plain_name_conflicts.contains(&name.to_ascii_lowercase())) + .filter_map(|name| select_by_name(catalog, name)) + .map(SkillCatalogEntryKey::from) + .collect::>(); + for entry in &catalog.entries { + if selected_names.contains(&SkillCatalogEntryKey::from(entry)) { + push_selected(entry, seen, selected); + } + } +} + +fn select_by_path( + catalog: &SkillCatalog, + path: &str, + seen: &mut HashSet, + selected: &mut Vec, +) { + let path = normalize_skill_path(path); + for entry in catalog.entries.iter().filter(|entry| entry.enabled) { + if entry_paths(entry) + .into_iter() + .any(|candidate| normalize_skill_path(candidate) == path) + { + push_selected(entry, seen, selected); + } + } +} + +fn entry_paths(entry: &SkillCatalogEntry) -> [&str; 3] { + [ + entry.main_prompt.as_str(), + entry.id.0.as_str(), + entry.rendered_path(), + ] +} + +fn select_by_name<'a>(catalog: &'a SkillCatalog, name: &str) -> Option<&'a SkillCatalogEntry> { + for kind in [ + SkillSourceKind::Executor, + SkillSourceKind::Orchestrator, + SkillSourceKind::Host, + ] { + let mut matches = catalog + .entries + .iter() + .filter(|entry| entry.enabled && entry.authority.kind == kind && entry.name == name); + let first = matches.next(); + if first.is_some() { + return first.filter(|_| matches.next().is_none()); + } + } + None +} + +fn push_selected( + entry: &SkillCatalogEntry, + seen: &mut HashSet, + selected: &mut Vec, +) { + if seen.insert(SkillCatalogEntryKey::from(entry)) { + selected.push(entry.clone()); + } +} + +fn path_is_skill(path: &str) -> bool { + path.starts_with(SKILL_PATH_PREFIX) + || path + .rsplit(['/', '\\']) + .next() + .is_some_and(|file_name| file_name.eq_ignore_ascii_case("SKILL.md")) +} + +fn normalize_skill_path(path: &str) -> &str { + path.strip_prefix(SKILL_PATH_PREFIX).unwrap_or(path) +} + +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +struct SkillCatalogEntryKey { + authority: SkillAuthority, + package: SkillPackageId, +} + +impl From<&SkillCatalogEntry> for SkillCatalogEntryKey { + fn from(entry: &SkillCatalogEntry) -> Self { + Self { + authority: entry.authority.clone(), + package: entry.id.clone(), + } + } +} + +#[cfg(test)] +#[path = "runtime_selection_tests.rs"] +mod tests; diff --git a/codex-rs/core-skills/src/runtime_selection_tests.rs b/codex-rs/core-skills/src/runtime_selection_tests.rs new file mode 100644 index 0000000000..c4a147f4f0 --- /dev/null +++ b/codex-rs/core-skills/src/runtime_selection_tests.rs @@ -0,0 +1,66 @@ +use std::collections::HashSet; + +use codex_protocol::user_input::UserInput; +use pretty_assertions::assert_eq; + +use super::*; + +fn entry(name: &str, path: &str) -> SkillCatalogEntry { + SkillCatalogEntry::new( + SkillPackageId(path.to_string()), + SkillAuthority::new(SkillSourceKind::Host, "host"), + name, + format!("Use {name}."), + crate::runtime::SkillResourceId::new(path), + ) +} + +fn text_input(text: &str) -> UserInput { + UserInput::Text { + text: text.to_string(), + text_elements: Vec::new(), + } +} + +#[test] +fn text_mentions_follow_catalog_order() { + let beta = entry("beta", "/skills/beta/SKILL.md"); + let alpha = entry("alpha", "/skills/alpha/SKILL.md"); + let catalog = SkillCatalog { + entries: vec![beta.clone(), alpha.clone()], + warnings: Vec::new(), + }; + + let selected = collect_runtime_skill_mentions( + &[text_input("Use $alpha and $beta")], + &catalog, + &HashSet::new(), + ); + + assert_eq!(selected, vec![beta, alpha]); +} + +#[test] +fn connector_collision_blocks_plain_name_but_not_exact_locator() { + let calendar = entry("calendar", "skill://executor/calendar/SKILL.md"); + let catalog = SkillCatalog { + entries: vec![calendar.clone()], + warnings: Vec::new(), + }; + let conflicts = HashSet::from(["calendar".to_string()]); + + assert_eq!( + collect_runtime_skill_mentions(&[text_input("Use $calendar")], &catalog, &conflicts), + Vec::new() + ); + assert_eq!( + collect_runtime_skill_mentions( + &[text_input( + "Use [$calendar](skill://executor/calendar/SKILL.md)", + )], + &catalog, + &conflicts, + ), + vec![calendar] + ); +} diff --git a/codex-rs/core-skills/src/skill_instructions.rs b/codex-rs/core-skills/src/skill_instructions.rs index b2a002d902..dd8fad489b 100644 --- a/codex-rs/core-skills/src/skill_instructions.rs +++ b/codex-rs/core-skills/src/skill_instructions.rs @@ -1,8 +1,13 @@ use codex_context_fragments::ContextualUserFragment; use crate::injection::SkillInjection; +use crate::runtime::SkillCatalogEntry; -#[derive(Debug, Clone, PartialEq)] +const MAX_SKILL_NAME_BYTES: usize = 256; +const MAX_SKILL_PATH_BYTES: usize = 1_024; +const MAX_SKILL_BODY_BYTES: usize = 8_000; + +#[derive(Debug, Clone, PartialEq, Eq)] pub struct SkillInstructions { name: String, path: String, @@ -19,6 +24,29 @@ impl From<&SkillInjection> for SkillInstructions { } } +impl SkillInstructions { + /// Builds one hard-bounded instruction fragment from a runtime catalog entry. + pub fn from_runtime(entry: &SkillCatalogEntry, contents: &str) -> (Self, bool) { + let (contents, truncated) = truncate_utf8(contents, MAX_SKILL_BODY_BYTES); + ( + Self { + name: truncate_utf8(&entry.name, MAX_SKILL_NAME_BYTES).0, + path: truncate_utf8(entry.rendered_path(), MAX_SKILL_PATH_BYTES).0, + contents, + }, + truncated, + ) + } +} + +fn truncate_utf8(value: &str, max_bytes: usize) -> (String, bool) { + let mut end = value.len().min(max_bytes); + while !value.is_char_boundary(end) { + end = end.saturating_sub(1); + } + (value[..end].to_string(), end < value.len()) +} + impl ContextualUserFragment for SkillInstructions { fn role(&self) -> &'static str { "user" diff --git a/codex-rs/ext/skills/src/catalog.rs b/codex-rs/ext/skills/src/catalog.rs deleted file mode 100644 index 74ee02de2a..0000000000 --- a/codex-rs/ext/skills/src/catalog.rs +++ /dev/null @@ -1,23 +0,0 @@ -pub use codex_core_skills::runtime::SkillAuthority; -pub use codex_core_skills::runtime::SkillCatalog; -pub use codex_core_skills::runtime::SkillCatalogEntry; -pub use codex_core_skills::runtime::SkillPackageId; -pub use codex_core_skills::runtime::SkillReadResult; -pub use codex_core_skills::runtime::SkillResourceId; -pub use codex_core_skills::runtime::SkillSourceError as SkillProviderError; -pub use codex_core_skills::runtime::SkillSourceKind; -pub use codex_core_skills::runtime::SkillSourceResult as SkillProviderResult; - -/// Search results for a package whose files are not readable through ordinary -/// executor filesystem access. -#[derive(Clone, Debug, Default, PartialEq, Eq)] -pub struct SkillSearchResult { - pub matches: Vec, -} - -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct SkillSearchMatch { - pub resource: SkillResourceId, - pub title: String, - pub snippet: String, -} diff --git a/codex-rs/ext/skills/src/config.rs b/codex-rs/ext/skills/src/config.rs deleted file mode 100644 index 3d903cafb8..0000000000 --- a/codex-rs/ext/skills/src/config.rs +++ /dev/null @@ -1,10 +0,0 @@ -/// Host-supplied configuration used by the skills extension. -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct SkillsExtensionConfig { - /// Whether the available-skills catalog is included in model context. - pub include_instructions: bool, - /// Whether bundled skills are eligible for discovery. - pub bundled_skills_enabled: bool, - /// Whether orchestrator-owned skills are eligible for discovery. - pub orchestrator_skills_enabled: bool, -} diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index de9facd8c2..cb6f7bd3eb 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -1,53 +1,23 @@ use std::sync::Arc; -use codex_core_skills::HostSkillsSnapshot; -use codex_core_skills::injection::InjectedHostSkillPrompts; use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_extension_api::ConfigContributor; -use codex_extension_api::ContextContributor; -use codex_extension_api::ContextualUserFragment; use codex_extension_api::ExtensionData; -use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionFuture; use codex_extension_api::ExtensionRegistryBuilder; -use codex_extension_api::PromptFragment; use codex_extension_api::ThreadLifecycleContributor; use codex_extension_api::ThreadStartInput; use codex_extension_api::ToolCall; use codex_extension_api::ToolContributor; use codex_extension_api::ToolExecutor; -use codex_extension_api::TurnInputContext; -use codex_extension_api::TurnInputContributor; use codex_mcp::McpResourceClient; -use codex_protocol::capabilities::SelectedCapabilityRoot; -use codex_protocol::protocol::Event; -use codex_protocol::protocol::EventMsg; -use codex_protocol::protocol::WarningEvent; -use crate::SkillsExtensionConfig; -use crate::catalog::SkillCatalog; -use crate::catalog::SkillCatalogEntry; -use crate::catalog::SkillReadResult; -use crate::catalog::SkillSourceKind; -use crate::fragments::SkillInstructions; -use crate::provider::HostSkillProvider; -use crate::provider::SkillListQuery; -use crate::provider::SkillReadRequest; -use crate::render::MAX_SKILL_NAME_BYTES; -use crate::render::MAX_SKILL_PATH_BYTES; -use crate::render::available_skills_fragment; -use crate::render::truncate_main_prompt_contents; -use crate::render::truncate_utf8_to_bytes; -use crate::selection::collect_explicit_skill_mentions; -use crate::sources::SkillProviders; +use crate::sources::orchestrator_skill_sources; use crate::state::SkillsThreadState; -use crate::state::SkillsTurnState; use crate::tools::skill_tools; struct SkillsExtension { - providers: SkillProviders, - event_sink: Arc, - config_from_host: Arc SkillsExtensionConfig + Send + Sync>, + orchestrator_skills_enabled: Arc bool + Send + Sync>, } impl ThreadLifecycleContributor for SkillsExtension @@ -56,19 +26,21 @@ where { fn on_thread_start<'a>(&'a self, input: ThreadStartInput<'a, C>) -> ExtensionFuture<'a, ()> { Box::pin(async move { - let selected_roots = input - .thread_store - .get::>() - .map(|selected_roots| selected_roots.as_ref().clone()) - .unwrap_or_default(); let orchestrator_skills_available = !input .environments .iter() .any(|environment| environment.environment_id == LOCAL_ENVIRONMENT_ID); - input.thread_store.insert(SkillsThreadState::new( - (self.config_from_host)(input.config), - selected_roots, - orchestrator_skills_available, + let thread_state = input.thread_store.get_or_init(|| { + SkillsThreadState::new( + (self.orchestrator_skills_enabled)(input.config), + orchestrator_skills_available, + ) + }); + thread_state + .set_orchestrator_skills_enabled((self.orchestrator_skills_enabled)(input.config)); + input.thread_store.insert(orchestrator_skill_sources( + thread_state, + input.session_store.get::(), )); }) } @@ -80,67 +52,26 @@ where { fn on_config_changed( &self, - _session_store: &ExtensionData, + session_store: &ExtensionData, thread_store: &ExtensionData, _previous_config: &C, new_config: &C, ) { - let next_config = (self.config_from_host)(new_config); + let enabled = (self.orchestrator_skills_enabled)(new_config); if let Some(state) = thread_store.get::() { - state.set_config(next_config); + state.set_orchestrator_skills_enabled(enabled); } else { let orchestrator_skills_available = true; - thread_store.insert(SkillsThreadState::new( - next_config, - Vec::new(), - orchestrator_skills_available, + let thread_state = thread_store + .get_or_init(|| SkillsThreadState::new(enabled, orchestrator_skills_available)); + thread_store.insert(orchestrator_skill_sources( + thread_state, + session_store.get::(), )); } } } -impl ContextContributor for SkillsExtension -where - C: Send + Sync + 'static, -{ - fn contribute_thread_context<'a>( - &'a self, - session_store: &'a ExtensionData, - thread_store: &'a ExtensionData, - ) -> std::pin::Pin> + Send + 'a>> { - Box::pin(async move { - let Some(thread_state) = thread_store.get::() else { - return Vec::new(); - }; - let config = thread_state.config(); - if !config.include_instructions { - return Vec::new(); - } - let catalog = self - .list_skills( - SkillListQuery { - turn_id: thread_store.level_id().to_string(), - executor_roots: thread_state.selected_roots().to_vec(), - host_snapshot: None, - include_host_skills: false, - include_bundled_skills: config.bundled_skills_enabled, - include_orchestrator_skills: thread_state.orchestrator_skills_enabled(), - mcp_resources: session_store.get::(), - }, - &thread_state, - ) - .await; - for warning in &catalog.warnings { - self.emit_warning(thread_store.level_id(), warning.clone()); - } - available_skills_fragment(&catalog) - .map(|fragment| PromptFragment::developer_capability(fragment.render())) - .into_iter() - .collect() - }) - } -} - impl ToolContributor for SkillsExtension where C: Send + Sync + 'static, @@ -153,224 +84,24 @@ where let Some(thread_state) = thread_store.get::() else { return Vec::new(); }; - if !self.providers.has_orchestrator_provider() - || !thread_state.orchestrator_skills_enabled() - { + if !thread_state.orchestrator_skills_enabled() { return Vec::new(); } - skill_tools( - self.providers.clone(), - session_store.get::(), - thread_state, - ) - } -} - -impl TurnInputContributor for SkillsExtension -where - C: Send + Sync + 'static, -{ - fn contribute<'a>( - &'a self, - input: TurnInputContext, - session_store: &'a ExtensionData, - thread_store: &'a ExtensionData, - turn_store: &'a ExtensionData, - ) -> ExtensionFuture<'a, Vec>> { - Box::pin(async move { - let Some(thread_state) = thread_store.get::() else { - return Vec::new(); - }; - - let config = thread_state.config(); - let host_snapshot = turn_store.get::(); - let query = SkillListQuery { - turn_id: input.turn_id.clone(), - executor_roots: thread_state.selected_roots().to_vec(), - host_snapshot: host_snapshot.clone(), - include_host_skills: true, - include_bundled_skills: config.bundled_skills_enabled, - include_orchestrator_skills: thread_state.orchestrator_skills_enabled(), - mcp_resources: session_store.get::(), - }; - let catalog = self.list_skills(query, &thread_state).await; - for warning in &catalog.warnings { - self.emit_warning(&input.turn_id, warning.clone()); - } - - let selected_entries = collect_explicit_skill_mentions(&input.user_input, &catalog); - let mut fragments: Vec> = Vec::new(); - if config.include_instructions { - let mut turn_catalog = catalog.clone(); - turn_catalog.entries.retain(|entry| { - entry.authority.kind != SkillSourceKind::Executor - && entry.authority.kind != SkillSourceKind::Orchestrator - }); - if let Some(fragment) = available_skills_fragment(&turn_catalog) { - fragments.push(Box::new(fragment)); - } - } - - let mut warnings = catalog.warnings.clone(); - let mut main_prompts_injected = false; - let mut injected_host_skill_prompts = InjectedHostSkillPrompts::default(); - for entry in &selected_entries { - match self - .read_main_prompt(entry, host_snapshot.clone(), session_store, &thread_state) - .await - { - Ok(read_result) => { - let (contents, truncated) = - truncate_main_prompt_contents(read_result.contents.as_str()); - if truncated { - let warning = format!( - "Skill `{}` exceeded the main prompt context limit and was truncated.", - entry.name - ); - self.emit_warning(&input.turn_id, warning.clone()); - warnings.push(warning); - } - let fragment = SkillInstructions { - name: truncate_utf8_to_bytes(&entry.name, MAX_SKILL_NAME_BYTES).0, - path: truncate_utf8_to_bytes( - entry.rendered_path(), - MAX_SKILL_PATH_BYTES, - ) - .0, - contents, - }; - fragments.push(Box::new(fragment)); - main_prompts_injected = true; - if entry.authority.kind == SkillSourceKind::Host { - injected_host_skill_prompts.insert_path(entry.main_prompt.as_str()); - } - } - Err(message) => { - let warning = format!("Failed to load skill `{}`: {message}", entry.name); - self.emit_warning(&input.turn_id, warning.clone()); - warnings.push(warning); - } - } - } - - if let Some(host_snapshot) = &host_snapshot { - for entry in selected_entries - .iter() - .filter(|entry| entry.authority.kind != SkillSourceKind::Host) - { - for host_skill in host_snapshot - .outcome() - .skills - .iter() - .filter(|host_skill| host_skill.name == entry.name) - { - injected_host_skill_prompts - .insert_path(host_skill.path_to_skills_md.to_string_lossy()); - } - } - } - - turn_store.insert(SkillsTurnState { - catalog, - selected_entries, - warnings, - main_prompts_injected, - }); - if !injected_host_skill_prompts.is_empty() { - turn_store.insert(injected_host_skill_prompts); - } - - fragments - }) - } -} - -impl SkillsExtension { - #[tracing::instrument(level = "trace", skip_all)] - async fn list_skills( - &self, - mut query: SkillListQuery, - thread_state: &SkillsThreadState, - ) -> SkillCatalog { - let include_orchestrator_skills = query.include_orchestrator_skills; - let orchestrator_query = query.clone(); - let mcp_resources = orchestrator_query.mcp_resources.clone(); - query.include_orchestrator_skills = false; - - let mut catalog = self.providers.list_for_turn(query).await; - if include_orchestrator_skills { - let orchestrator_catalog = thread_state - .orchestrator_catalog_snapshot( - mcp_resources.as_deref(), - self.providers - .list_orchestrator_for_turn(orchestrator_query), - ) - .await; - catalog.extend(orchestrator_catalog); - } - catalog - } - - #[tracing::instrument(level = "trace", skip_all, fields(skill = %entry.name))] - async fn read_main_prompt( - &self, - entry: &SkillCatalogEntry, - host_snapshot: Option>, - session_store: &ExtensionData, - thread_state: &SkillsThreadState, - ) -> Result { - thread_state - .read_skill( - &self.providers, - SkillReadRequest { - authority: entry.authority.clone(), - package: entry.id.clone(), - resource: entry.main_prompt.clone(), - host_snapshot, - mcp_resources: session_store.get::(), - }, - ) - .await - .map_err(|err| err.message) - } - - fn emit_warning(&self, turn_id: &str, message: String) { - self.event_sink.emit(Event { - id: turn_id.to_string(), - msg: EventMsg::Warning(WarningEvent { message }), - }); + skill_tools(session_store.get::(), thread_state) } } pub fn install( registry: &mut ExtensionRegistryBuilder, - config_from_host: impl Fn(&C) -> SkillsExtensionConfig + Send + Sync + 'static, -) where - C: Send + Sync + 'static, -{ - install_with_providers( - registry, - SkillProviders::new().with_host_provider(Arc::new(HostSkillProvider::new())), - config_from_host, - ); -} - -pub fn install_with_providers( - registry: &mut ExtensionRegistryBuilder, - providers: SkillProviders, - config_from_host: impl Fn(&C) -> SkillsExtensionConfig + Send + Sync + 'static, + orchestrator_skills_enabled: impl Fn(&C) -> bool + Send + Sync + 'static, ) where C: Send + Sync + 'static, { let extension = Arc::new(SkillsExtension { - providers, - event_sink: registry.event_sink(), - config_from_host: Arc::new(config_from_host), + orchestrator_skills_enabled: Arc::new(orchestrator_skills_enabled), }); registry.thread_lifecycle_contributor(extension.clone()); registry.config_contributor(extension.clone()); - registry.prompt_contributor(extension.clone()); - registry.turn_input_contributor(extension.clone()); registry.tool_contributor(extension); } diff --git a/codex-rs/ext/skills/src/fragments.rs b/codex-rs/ext/skills/src/fragments.rs index 0a24b48b5a..19f5a4b6cb 100644 --- a/codex-rs/ext/skills/src/fragments.rs +++ b/codex-rs/ext/skills/src/fragments.rs @@ -1,3 +1,4 @@ +use codex_core_skills::AvailableSkills; use codex_core_skills::render_available_skills_body; use codex_extension_api::ContextualUserFragment; use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG; @@ -5,12 +6,16 @@ use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; #[derive(Clone, Debug, Eq, PartialEq)] pub(crate) struct AvailableSkillsInstructions { + skill_root_lines: Vec, skill_lines: Vec, } -impl AvailableSkillsInstructions { - pub(crate) fn from_skill_lines(skill_lines: Vec) -> Self { - Self { skill_lines } +impl From for AvailableSkillsInstructions { + fn from(available: AvailableSkills) -> Self { + Self { + skill_root_lines: available.skill_root_lines, + skill_lines: available.skill_lines, + } } } @@ -28,34 +33,6 @@ impl ContextualUserFragment for AvailableSkillsInstructions { } fn body(&self) -> String { - render_available_skills_body(&[], &self.skill_lines) - } -} - -#[derive(Clone, Debug, Eq, PartialEq)] -pub(crate) struct SkillInstructions { - pub(crate) name: String, - pub(crate) path: String, - pub(crate) contents: String, -} - -impl ContextualUserFragment for SkillInstructions { - fn role(&self) -> &'static str { - "user" - } - - fn markers(&self) -> (&'static str, &'static str) { - Self::type_markers() - } - - fn type_markers() -> (&'static str, &'static str) { - ("", "") - } - - fn body(&self) -> String { - let name = &self.name; - let path = &self.path; - let contents = &self.contents; - format!("\n{name}\n{path}\n{contents}\n") + render_available_skills_body(&self.skill_root_lines, &self.skill_lines) } } diff --git a/codex-rs/ext/skills/src/lib.rs b/codex-rs/ext/skills/src/lib.rs index 8abdf6a93e..833562e9b7 100644 --- a/codex-rs/ext/skills/src/lib.rs +++ b/codex-rs/ext/skills/src/lib.rs @@ -1,20 +1,7 @@ -pub mod catalog; -mod config; mod extension; -mod fragments; -pub mod provider; mod render; -mod selection; mod sources; mod state; mod tools; -pub use config::SkillsExtensionConfig; pub use extension::install; -pub use extension::install_with_providers; -pub use provider::ExecutorSkillProvider; -pub use provider::HostSkillProvider; -pub use provider::OrchestratorSkillProvider; -pub use provider::SkillProvider; -pub use sources::SkillProviderSource; -pub use sources::SkillProviders; diff --git a/codex-rs/ext/skills/src/provider.rs b/codex-rs/ext/skills/src/provider.rs deleted file mode 100644 index 405592e934..0000000000 --- a/codex-rs/ext/skills/src/provider.rs +++ /dev/null @@ -1,66 +0,0 @@ -use std::future::Future; -use std::pin::Pin; -use std::sync::Arc; - -mod executor; -mod host; -mod orchestrator; - -use codex_core_skills::HostSkillsSnapshot; -use codex_mcp::McpResourceClient; -use codex_protocol::capabilities::SelectedCapabilityRoot; - -use crate::catalog::SkillAuthority; -use crate::catalog::SkillCatalog; -use crate::catalog::SkillPackageId; -use crate::catalog::SkillProviderResult; -use crate::catalog::SkillReadResult; -use crate::catalog::SkillResourceId; -use crate::catalog::SkillSearchResult; - -pub use executor::ExecutorSkillProvider; -pub use host::HostSkillProvider; -pub use orchestrator::OrchestratorSkillProvider; - -#[derive(Clone, Debug)] -pub struct SkillListQuery { - pub turn_id: String, - pub executor_roots: Vec, - pub host_snapshot: Option>, - pub include_host_skills: bool, - pub include_bundled_skills: bool, - pub include_orchestrator_skills: bool, - pub mcp_resources: Option>, -} - -#[derive(Clone, Debug)] -pub struct SkillReadRequest { - pub authority: SkillAuthority, - pub package: SkillPackageId, - pub resource: SkillResourceId, - pub host_snapshot: Option>, - pub mcp_resources: Option>, -} - -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct SkillSearchRequest { - pub authority: SkillAuthority, - pub package: SkillPackageId, - pub query: String, -} - -pub type SkillProviderFuture<'a, T> = - Pin> + Send + 'a>>; - -/// Source-specific skill catalog and resource access. -/// -/// Implementations must preserve authority boundaries: a resource listed by a -/// provider must be read or searched through the same provider/authority rather -/// than converted into an ambient local path. -pub trait SkillProvider: Send + Sync { - fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog>; - - fn read(&self, request: SkillReadRequest) -> SkillProviderFuture<'_, SkillReadResult>; - - fn search(&self, request: SkillSearchRequest) -> SkillProviderFuture<'_, SkillSearchResult>; -} diff --git a/codex-rs/ext/skills/src/provider/executor.rs b/codex-rs/ext/skills/src/provider/executor.rs deleted file mode 100644 index 9938035e92..0000000000 --- a/codex-rs/ext/skills/src/provider/executor.rs +++ /dev/null @@ -1,166 +0,0 @@ -use std::sync::Arc; - -use codex_core_skills::loader::EnvironmentSkillMetadata; -use codex_core_skills::loader::load_environment_skills_from_root; -use codex_exec_server::EnvironmentManager; -use codex_protocol::capabilities::CapabilityRootLocation; -use codex_protocol::protocol::Product; -use codex_utils_path_uri::PathConvention; - -use crate::catalog::SkillAuthority; -use crate::catalog::SkillCatalog; -use crate::catalog::SkillCatalogEntry; -use crate::catalog::SkillPackageId; -use crate::catalog::SkillProviderError; -use crate::catalog::SkillReadResult; -use crate::catalog::SkillResourceId; -use crate::catalog::SkillSearchResult; -use crate::catalog::SkillSourceKind; -use crate::provider::SkillListQuery; -use crate::provider::SkillProvider; -use crate::provider::SkillProviderFuture; -use crate::provider::SkillReadRequest; -use crate::provider::SkillSearchRequest; - -/// Discovers and reads skills through the filesystem owned by an execution environment. -#[derive(Clone, Debug)] -pub struct ExecutorSkillProvider { - environment_manager: Arc, - restriction_product: Option, -} - -impl ExecutorSkillProvider { - pub fn new_with_restriction_product( - environment_manager: Arc, - restriction_product: Option, - ) -> Self { - Self { - environment_manager, - restriction_product, - } - } -} - -impl SkillProvider for ExecutorSkillProvider { - fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { - Box::pin(async move { - let mut catalog = SkillCatalog::default(); - for selected_root in query.executor_roots { - let selected_root_id = selected_root.id; - let CapabilityRootLocation::Environment { - environment_id, - path, - } = selected_root.location; - let authority = - SkillAuthority::new(SkillSourceKind::Executor, selected_root_id.clone()); - let Some(environment) = self.environment_manager.get_environment(&environment_id) - else { - catalog.warnings.push(format!( - "Selected capability root `{selected_root_id}` references unavailable environment `{environment_id}`." - )); - continue; - }; - let file_system = environment.get_filesystem(); - let outcome = load_environment_skills_from_root( - file_system.as_ref(), - &path, - self.restriction_product, - ) - .await; - catalog.warnings.extend(outcome.warnings); - for skill in outcome.skills { - catalog.push_entry(catalog_entry_from_skill( - &skill, - authority.clone(), - &selected_root_id, - &environment_id, - )); - } - } - - Ok(catalog) - }) - } - - fn read(&self, request: SkillReadRequest) -> SkillProviderFuture<'_, SkillReadResult> { - Box::pin(async move { - if request.authority.kind != SkillSourceKind::Executor { - return Err(SkillProviderError::new(format!( - "executor skill provider cannot read {} resources", - request.authority.kind - ))); - } - if request.package.0 != request.resource.as_str() { - return Err(SkillProviderError::new( - "executor skill resource does not match its package", - )); - } - let Some((environment_id, resource_path)) = request.resource.environment_path() else { - return Err(SkillProviderError::new( - "executor skill resource is not bound to an environment", - )); - }; - let Some(environment) = self.environment_manager.get_environment(environment_id) else { - return Err(SkillProviderError::new(format!( - "executor skill resource references unavailable environment `{environment_id}`" - ))); - }; - let contents = environment - .get_filesystem() - .read_file_text(resource_path, /*sandbox*/ None) - .await - .map_err(|err| { - SkillProviderError::new(format!( - "failed to read executor skill resource {}: {err}", - request.resource.as_str() - )) - })?; - - Ok(SkillReadResult { - resource: request.resource, - contents, - }) - }) - } - - fn search(&self, _request: SkillSearchRequest) -> SkillProviderFuture<'_, SkillSearchResult> { - Box::pin(async { Ok(SkillSearchResult::default()) }) - } -} - -fn catalog_entry_from_skill( - skill: &EnvironmentSkillMetadata, - authority: SkillAuthority, - selected_root_id: &str, - environment_id: &str, -) -> SkillCatalogEntry { - let skill_path = skill.path_to_skills_md.inferred_native_path_string(); - let normalized_path = match skill.path_to_skills_md.infer_path_convention() { - Some(PathConvention::Windows) => skill_path.replace('\\', "/"), - Some(PathConvention::Posix) | None => skill_path, - }; - let display_path = format!( - "skill://{selected_root_id}/{}", - normalized_path.trim_start_matches('/') - ); - let entry = SkillCatalogEntry::new( - SkillPackageId(display_path.clone()), - authority, - skill.name.clone(), - skill.description.clone(), - SkillResourceId::environment( - display_path.clone(), - environment_id, - skill.path_to_skills_md.clone(), - ), - ) - .with_short_description(skill.short_description.clone()) - .with_display_path(display_path) - .with_dependencies(skill.dependencies.clone()); - - if skill.allows_implicit_invocation() { - entry - } else { - entry.hidden_from_prompt() - } -} diff --git a/codex-rs/ext/skills/src/provider/host.rs b/codex-rs/ext/skills/src/provider/host.rs deleted file mode 100644 index 2cee694216..0000000000 --- a/codex-rs/ext/skills/src/provider/host.rs +++ /dev/null @@ -1,129 +0,0 @@ -use codex_core_skills::SkillLoadOutcome; -use codex_core_skills::SkillMetadata; - -use crate::catalog::SkillAuthority; -use crate::catalog::SkillCatalog; -use crate::catalog::SkillCatalogEntry; -use crate::catalog::SkillPackageId; -use crate::catalog::SkillProviderError; -use crate::catalog::SkillReadResult; -use crate::catalog::SkillResourceId; -use crate::catalog::SkillSearchResult; -use crate::catalog::SkillSourceKind; -use crate::provider::SkillListQuery; -use crate::provider::SkillProvider; -use crate::provider::SkillProviderFuture; -use crate::provider::SkillReadRequest; -use crate::provider::SkillSearchRequest; - -const HOST_AUTHORITY_ID: &str = "host"; - -/// Host-owned skill provider backed by an immutable service snapshot. -/// -/// Discovery and caching belong to `SkillsService`; this provider only maps a -/// snapshot into the authority-aware catalog/read contract. -#[derive(Clone, Default)] -pub struct HostSkillProvider; - -impl HostSkillProvider { - pub fn new() -> Self { - Self - } -} - -impl SkillProvider for HostSkillProvider { - fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { - Box::pin(async move { - let Some(host_snapshot) = query.host_snapshot else { - return Err(SkillProviderError::new( - "host skill provider requires a host skills snapshot", - )); - }; - - Ok(catalog_from_outcome(host_snapshot.outcome())) - }) - } - - fn read(&self, request: SkillReadRequest) -> SkillProviderFuture<'_, SkillReadResult> { - Box::pin(async move { - let Some(host_snapshot) = request.host_snapshot else { - return Err(SkillProviderError::new( - "host skill provider requires a host skills snapshot", - )); - }; - let Some(skill) = host_snapshot.outcome().skills.iter().find(|skill| { - let skill_path = skill.path_to_skills_md.to_string_lossy(); - skill_path == request.resource.as_str() - || skill_path.replace('\\', "/") == request.resource.as_str() - }) else { - return Err(SkillProviderError::new(format!( - "host skill resource is not loaded: {}", - request.resource.as_str() - ))); - }; - - let contents = host_snapshot.read_skill_text(skill).await.map_err(|err| { - SkillProviderError::new(format!( - "failed to read host skill resource {}: {err}", - request.resource.as_str() - )) - })?; - - Ok(SkillReadResult { - resource: request.resource, - contents, - }) - }) - } - - fn search(&self, _request: SkillSearchRequest) -> SkillProviderFuture<'_, SkillSearchResult> { - Box::pin(async { Ok(SkillSearchResult::default()) }) - } -} - -fn catalog_from_outcome(outcome: &SkillLoadOutcome) -> SkillCatalog { - let mut catalog = SkillCatalog { - entries: Vec::new(), - warnings: outcome - .errors - .iter() - .map(|err| { - format!( - "Failed to load skill at {}: {}", - err.path.display(), - err.message - ) - }) - .collect(), - }; - - for (skill, enabled) in outcome.skills_with_enabled() { - catalog.push_entry(catalog_entry_from_skill(skill, enabled)); - } - - catalog -} - -fn catalog_entry_from_skill(skill: &SkillMetadata, enabled: bool) -> SkillCatalogEntry { - let skill_path = skill.path_to_skills_md.to_string_lossy().into_owned(); - let display_path = skill_path.replace('\\', "/"); - let mut entry = SkillCatalogEntry::new( - SkillPackageId(skill_path.clone()), - SkillAuthority::new(SkillSourceKind::Host, HOST_AUTHORITY_ID), - skill.name.clone(), - skill.description.clone(), - SkillResourceId::new(skill_path), - ) - .with_short_description(skill.short_description.clone()) - .with_display_path(display_path) - .with_dependencies(skill.dependencies.clone()); - - if !enabled { - entry = entry.disabled(); - } - if !skill.allows_implicit_invocation() { - entry = entry.hidden_from_prompt(); - } - - entry -} diff --git a/codex-rs/ext/skills/src/provider/orchestrator.rs b/codex-rs/ext/skills/src/provider/orchestrator.rs deleted file mode 100644 index 8d97bf32a1..0000000000 --- a/codex-rs/ext/skills/src/provider/orchestrator.rs +++ /dev/null @@ -1,331 +0,0 @@ -use std::collections::HashSet; -use std::time::Duration; - -use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; -use codex_protocol::mcp::Resource; -use codex_protocol::mcp::ResourceContent; -use url::Url; - -use crate::catalog::SkillAuthority; -use crate::catalog::SkillCatalog; -use crate::catalog::SkillCatalogEntry; -use crate::catalog::SkillPackageId; -use crate::catalog::SkillProviderError; -use crate::catalog::SkillReadResult; -use crate::catalog::SkillResourceId; -use crate::catalog::SkillSearchResult; -use crate::catalog::SkillSourceKind; -use crate::provider::SkillListQuery; -use crate::provider::SkillProvider; -use crate::provider::SkillProviderFuture; -use crate::provider::SkillReadRequest; -use crate::provider::SkillSearchRequest; - -const ORCHESTRATOR_SKILL_MIME_TYPE: &str = "mcp/skill"; -const ORCHESTRATOR_SKILL_DISCOVERY_TIMEOUT: Duration = Duration::from_secs(10); -const ORCHESTRATOR_SKILL_READ_TIMEOUT: Duration = Duration::from_secs(10); -const MAX_RESOURCE_PAGES: usize = 10; -const MAX_ORCHESTRATOR_SKILLS: usize = 100; -const MAX_SKILL_NAME_CHARS: usize = 64; -const MAX_QUALIFIED_SKILL_NAME_CHARS: usize = 128; -const MAX_SKILL_PACKAGE_URI_CHARS: usize = 1_024; -const MAX_SKILL_RESOURCE_URI_CHARS: usize = 2_048; -const MAX_SKILL_RESOURCE_CONTENT_BYTES: usize = 1024 * 1024; - -/// Discovers and reads skills owned by the orchestrator. -/// -/// The provider uses session-scoped resources without exposing the transport or -/// resource server to callers that configure the skills extension. -#[derive(Clone, Debug, Default)] -pub struct OrchestratorSkillProvider; - -impl OrchestratorSkillProvider { - pub fn new() -> Self { - Self - } -} - -impl SkillProvider for OrchestratorSkillProvider { - fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { - Box::pin(async move { - let Some(client) = query.mcp_resources else { - return Ok(SkillCatalog::default()); - }; - if !client.has_server(CODEX_APPS_MCP_SERVER_NAME).await { - return Ok(SkillCatalog::default()); - } - - let discovery_deadline = - tokio::time::Instant::now() + ORCHESTRATOR_SKILL_DISCOVERY_TIMEOUT; - let mut catalog = SkillCatalog::default(); - let mut cursor = None; - let mut seen_cursors = HashSet::new(); - let mut skill_resources_seen = 0usize; - let mut skipped_resources = 0usize; - let mut truncated = false; - let mut completed_pages = 0usize; - - for _ in 0..MAX_RESOURCE_PAGES { - let page = match tokio::time::timeout_at( - discovery_deadline, - client.list_resources(CODEX_APPS_MCP_SERVER_NAME, cursor.clone()), - ) - .await - { - Ok(result) => result.map_err(|err| { - SkillProviderError::new(format!( - "failed to list orchestrator skill resources: {err:#}" - )) - }), - Err(_) => Err(SkillProviderError::new(format!( - "orchestrator skill discovery timed out after {ORCHESTRATOR_SKILL_DISCOVERY_TIMEOUT:?}" - ))), - }; - let result = match page { - Ok(result) => result, - Err(err) if completed_pages == 0 => return Err(err), - Err(err) => { - let page_word = if completed_pages == 1 { - "page" - } else { - "pages" - }; - catalog.warnings.push(format!( - "Orchestrator skill discovery stopped after {completed_pages} resource {page_word}: {}", - err.message - )); - cursor = None; - break; - } - }; - completed_pages = completed_pages.saturating_add(1); - - for resource in &result.resources { - if resource.mime_type.as_deref() != Some(ORCHESTRATOR_SKILL_MIME_TYPE) { - continue; - } - if skill_resources_seen >= MAX_ORCHESTRATOR_SKILLS { - truncated = true; - break; - } - skill_resources_seen = skill_resources_seen.saturating_add(1); - match catalog_entry_from_resource(resource) { - Some(entry) => catalog.push_entry(entry), - None => skipped_resources = skipped_resources.saturating_add(1), - } - } - - if truncated { - break; - } - let Some(next_cursor) = result.next_cursor else { - cursor = None; - break; - }; - if !seen_cursors.insert(next_cursor.clone()) { - catalog.warnings.push( - "Orchestrator skill resource pagination returned a duplicate cursor." - .to_string(), - ); - cursor = None; - break; - } - cursor = Some(next_cursor); - } - - if cursor.is_some() || truncated { - catalog.warnings.push(format!( - "Orchestrator skill discovery was truncated at {MAX_ORCHESTRATOR_SKILLS} skills or {MAX_RESOURCE_PAGES} resource pages." - )); - } - if skipped_resources > 0 { - catalog.warnings.push(format!( - "Skipped {skipped_resources} malformed orchestrator skill resources." - )); - } - - Ok(catalog) - }) - } - - fn read(&self, request: SkillReadRequest) -> SkillProviderFuture<'_, SkillReadResult> { - Box::pin(async move { - if request.authority - != SkillAuthority::new(SkillSourceKind::Orchestrator, CODEX_APPS_MCP_SERVER_NAME) - { - return Err(SkillProviderError::new(format!( - "orchestrator skill provider cannot read authority {}", - request.authority.id - ))); - } - if !resource_belongs_to_package(&request.package.0, request.resource.as_str()) { - return Err(SkillProviderError::new( - "orchestrator skill resource does not match its package", - )); - } - - let Some(client) = request.mcp_resources.as_ref() else { - return Err(SkillProviderError::new( - "session MCP resource client is not configured", - )); - }; - let result = tokio::time::timeout( - ORCHESTRATOR_SKILL_READ_TIMEOUT, - client.read_resource(CODEX_APPS_MCP_SERVER_NAME, request.resource.as_str()), - ) - .await - .map_err(|_| { - SkillProviderError::new(format!( - "orchestrator skill read timed out after {ORCHESTRATOR_SKILL_READ_TIMEOUT:?}" - )) - })? - .map_err(|err| { - SkillProviderError::new(format!( - "failed to read orchestrator skill resource {}: {err:#}", - request.resource.as_str() - )) - })?; - let contents = result - .contents - .into_iter() - .find_map(|contents| match contents { - ResourceContent::Text { uri, text, .. } if uri == request.resource.as_str() => { - Some(text) - } - ResourceContent::Text { .. } | ResourceContent::Blob { .. } => None, - }); - let Some(contents) = contents else { - return Err(SkillProviderError::new(format!( - "orchestrator skill resource {} did not return matching text contents", - request.resource.as_str() - ))); - }; - if contents.len() > MAX_SKILL_RESOURCE_CONTENT_BYTES { - return Err(SkillProviderError::new(format!( - "orchestrator skill resource {} exceeds the {MAX_SKILL_RESOURCE_CONTENT_BYTES}-byte read limit", - request.resource.as_str() - ))); - } - - Ok(SkillReadResult { - resource: request.resource, - contents, - }) - }) - } - - fn search(&self, _request: SkillSearchRequest) -> SkillProviderFuture<'_, SkillSearchResult> { - Box::pin(async { Ok(SkillSearchResult::default()) }) - } -} - -fn catalog_entry_from_resource(resource: &Resource) -> Option { - let uri = validated_skill_uri(resource.uri.as_str(), MAX_SKILL_PACKAGE_URI_CHARS)?; - let meta = resource.meta.as_ref()?.as_object()?; - let skill_name = normalized_label(meta.get("skill_name")?.as_str()?, MAX_SKILL_NAME_CHARS)?; - let name = if meta.get("source").and_then(|value| value.as_str()) == Some("user") { - skill_name - } else { - let plugin_name = - normalized_label(meta.get("plugin_name")?.as_str()?, MAX_SKILL_NAME_CHARS)?; - let qualified_name = format!("{plugin_name}:{skill_name}"); - (qualified_name.chars().count() <= MAX_QUALIFIED_SKILL_NAME_CHARS) - .then_some(qualified_name)? - }; - let description = normalized_description(resource.description.as_deref().unwrap_or_default())?; - let main_prompt = main_prompt_uri(uri); - - Some( - SkillCatalogEntry::new( - SkillPackageId(uri.to_string()), - SkillAuthority::new(SkillSourceKind::Orchestrator, CODEX_APPS_MCP_SERVER_NAME), - name, - description, - SkillResourceId::new(main_prompt), - ) - .with_display_path(uri), - ) -} - -fn validated_skill_uri(uri: &str, max_chars: usize) -> Option<&str> { - validated_skill_url(uri, max_chars).map(|_| uri) -} - -fn validated_skill_url(uri: &str, max_chars: usize) -> Option { - if uri.chars().count() > max_chars - || uri - .chars() - .any(|ch| ch.is_control() || ch.is_whitespace() || matches!(ch, '<' | '>')) - { - return None; - } - - let url = Url::parse(uri).ok()?; - let path_is_valid = url.path_segments().is_some_and(|segments| { - let segments = segments.collect::>(); - !segments.is_empty() && segments.iter().all(|segment| !segment.is_empty()) - }); - (url.scheme() == "skill" - && url.as_str() == uri - && url.host_str().is_some_and(|host| !host.is_empty()) - && url.username().is_empty() - && url.password().is_none() - && url.port().is_none() - && url.query().is_none() - && url.fragment().is_none() - && path_is_valid) - .then_some(url) -} - -fn resource_belongs_to_package(package: &str, resource: &str) -> bool { - let Some(package) = validated_skill_url(package, MAX_SKILL_PACKAGE_URI_CHARS) else { - return false; - }; - let Some(resource) = validated_skill_url(resource, MAX_SKILL_RESOURCE_URI_CHARS) else { - return false; - }; - - let Some(package_segments) = package.path_segments() else { - return false; - }; - let Some(resource_segments) = resource.path_segments() else { - return false; - }; - let package_segments = package_segments.collect::>(); - let resource_segments = resource_segments.collect::>(); - - package.scheme() == resource.scheme() - && package.host_str() == resource.host_str() - && resource_segments.len() > package_segments.len() - && resource_segments.starts_with(&package_segments) -} - -fn normalized_label(value: &str, max_chars: usize) -> Option { - let value = normalized_single_line(value, max_chars)?; - let invalid = value.is_empty() || value.chars().any(|ch| matches!(ch, '&' | '<' | '>')); - (!invalid).then_some(value) -} - -fn normalized_description(value: &str) -> Option { - let value = value.split_whitespace().collect::>().join(" "); - if value.chars().any(char::is_control) { - return None; - } - - Some( - value - .replace('&', "&") - .replace('<', "<") - .replace('>', ">"), - ) -} - -fn normalized_single_line(value: &str, max_chars: usize) -> Option { - let value = value.split_whitespace().collect::>().join(" "); - let valid = value.chars().count() <= max_chars && !value.chars().any(char::is_control); - valid.then_some(value) -} - -fn main_prompt_uri(package_uri: &str) -> String { - format!("{}/SKILL.md", package_uri.trim_end_matches('/')) -} diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index dda32b4864..7cc1680017 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -1,18 +1,15 @@ use std::borrow::Cow; +use codex_core_skills::SkillRenderSideEffects; +use codex_core_skills::build_available_skills_from_catalog; +use codex_core_skills::default_skill_metadata_budget; use codex_utils_string::take_bytes_at_char_boundary; use crate::catalog::SkillCatalog; -use crate::catalog::SkillCatalogEntry; -use crate::catalog::SkillSourceKind; use crate::fragments::AvailableSkillsInstructions; -const MAX_AVAILABLE_SKILLS_BYTES: usize = 8_000; -const MAX_MAIN_PROMPT_BYTES: usize = 8_000; const MAX_CATALOG_SKILL_DESCRIPTION_CHARS: usize = 1_024; const TRUNCATED_SKILL_DESCRIPTION_SUFFIX: &str = "..."; -pub(crate) const MAX_SKILL_NAME_BYTES: usize = 256; -pub(crate) const MAX_SKILL_PATH_BYTES: usize = 1_024; #[tracing::instrument( level = "trace", @@ -21,42 +18,15 @@ pub(crate) const MAX_SKILL_PATH_BYTES: usize = 1_024; )] pub(crate) fn available_skills_fragment( catalog: &SkillCatalog, -) -> Option { - let mut total_bytes = 0usize; - let mut omitted = 0usize; - let mut skill_lines = Vec::new(); - - for entry in catalog - .entries - .iter() - .filter(|entry| entry.enabled && entry.prompt_visible) - { - let description = entry - .short_description - .as_deref() - .unwrap_or(entry.description.as_str()); - let description = truncate_catalog_skill_description(description); - let line = render_skill_line(entry, description.as_ref()); - let next_bytes = total_bytes.saturating_add(line.len()); - if next_bytes > MAX_AVAILABLE_SKILLS_BYTES { - omitted = omitted.saturating_add(1); - continue; - } - total_bytes = next_bytes; - skill_lines.push(line); - } - - if skill_lines.is_empty() { - return None; - } - if omitted > 0 { - let skill_word = if omitted == 1 { "skill" } else { "skills" }; - skill_lines.push(format!( - "- {omitted} additional {skill_word} omitted from this bounded skills list." - )); - } - - Some(AvailableSkillsInstructions::from_skill_lines(skill_lines)) +) -> Option<(AvailableSkillsInstructions, Option)> { + let available = build_available_skills_from_catalog( + catalog, + /*host_outcome*/ None, + default_skill_metadata_budget(/*context_window*/ None), + SkillRenderSideEffects::None, + )?; + let warning = available.warning_message.clone(); + Some((available.into(), warning)) } pub(crate) fn truncate_catalog_skill_description(description: &str) -> Cow<'_, str> { @@ -79,26 +49,6 @@ pub(crate) fn truncate_catalog_skill_description(description: &str) -> Cow<'_, s Cow::Owned(truncated) } -fn render_skill_line(entry: &SkillCatalogEntry, description: &str) -> String { - let locator_kind = match &entry.authority.kind { - SkillSourceKind::Host => "file", - SkillSourceKind::Executor => "environment resource", - SkillSourceKind::Orchestrator => "orchestrator resource", - SkillSourceKind::Custom(_) => "custom resource", - }; - let name = entry.name.as_str(); - let path = entry.rendered_path(); - if description.is_empty() { - format!("- {name}: ({locator_kind}: {path})") - } else { - format!("- {name}: {description} ({locator_kind}: {path})") - } -} - -pub(crate) fn truncate_main_prompt_contents(contents: &str) -> (String, bool) { - truncate_utf8_to_bytes(contents, MAX_MAIN_PROMPT_BYTES) -} - pub(crate) fn truncate_utf8_to_bytes(contents: &str, max_bytes: usize) -> (String, bool) { let truncated = take_bytes_at_char_boundary(contents, max_bytes); (truncated.to_string(), truncated.len() < contents.len()) diff --git a/codex-rs/ext/skills/src/selection.rs b/codex-rs/ext/skills/src/selection.rs deleted file mode 100644 index a708b18d6c..0000000000 --- a/codex-rs/ext/skills/src/selection.rs +++ /dev/null @@ -1,137 +0,0 @@ -use std::collections::HashSet; - -use codex_core_skills::injection::extract_tool_mentions; -use codex_protocol::user_input::UserInput; - -use crate::catalog::SkillAuthority; -use crate::catalog::SkillCatalog; -use crate::catalog::SkillCatalogEntry; -use crate::catalog::SkillPackageId; - -const SKILL_PATH_PREFIX: &str = "skill://"; - -#[tracing::instrument( - level = "trace", - skip_all, - fields( - input_count = inputs.len(), - catalog_entry_count = catalog.entries.len() - ) -)] -pub(crate) fn collect_explicit_skill_mentions( - inputs: &[UserInput], - catalog: &SkillCatalog, -) -> Vec { - let mut selected = Vec::new(); - let mut seen = HashSet::new(); - let mut blocked_plain_names = HashSet::new(); - - for input in inputs { - match input { - UserInput::Skill { name, path } => { - blocked_plain_names.insert(name.clone()); - select_by_path(catalog, &path.to_string_lossy(), &mut seen, &mut selected); - } - UserInput::Mention { name, path } if path_is_skill(path) => { - blocked_plain_names.insert(name.clone()); - select_by_path(catalog, path, &mut seen, &mut selected); - } - UserInput::Text { .. } | UserInput::Image { .. } | UserInput::LocalImage { .. } => {} - UserInput::Mention { .. } => {} - _ => {} - } - } - - for input in inputs { - let UserInput::Text { text, .. } = input else { - continue; - }; - - let mentions = extract_tool_mentions(text); - for path in mentions.paths() { - if path_is_skill(path) { - select_by_path( - catalog, - normalize_skill_path(path), - &mut seen, - &mut selected, - ); - } - } - for name in mentions.plain_names() { - if blocked_plain_names.contains(name) { - continue; - } - if let Some(entry) = catalog - .entries - .iter() - .find(|entry| entry.enabled && entry.name == name) - { - push_selected(entry, &mut seen, &mut selected); - } - } - } - - selected -} - -fn select_by_path( - catalog: &SkillCatalog, - path: &str, - seen: &mut HashSet, - selected: &mut Vec, -) { - let normalized_path = normalize_skill_path(path); - for entry in catalog.entries.iter().filter(|entry| entry.enabled) { - if entry_matches_path(entry, normalized_path) { - push_selected(entry, seen, selected); - } - } -} - -fn push_selected( - entry: &SkillCatalogEntry, - seen: &mut HashSet, - selected: &mut Vec, -) { - let key = SkillCatalogEntryKey::from(entry); - if seen.insert(key) { - selected.push(entry.clone()); - } -} - -fn entry_matches_path(entry: &SkillCatalogEntry, path: &str) -> bool { - entry.main_prompt.as_str() == path - || entry.id.0 == path - || entry - .display_path - .as_deref() - .is_some_and(|display_path| normalize_skill_path(display_path) == path) -} - -fn path_is_skill(path: &str) -> bool { - path.starts_with(SKILL_PATH_PREFIX) - || path - .rsplit(['/', '\\']) - .next() - .is_some_and(|file_name| file_name.eq_ignore_ascii_case("SKILL.md")) -} - -fn normalize_skill_path(path: &str) -> &str { - path.strip_prefix(SKILL_PATH_PREFIX).unwrap_or(path) -} - -#[derive(Clone, Debug, PartialEq, Eq, Hash)] -struct SkillCatalogEntryKey { - authority: SkillAuthority, - package: SkillPackageId, -} - -impl From<&SkillCatalogEntry> for SkillCatalogEntryKey { - fn from(entry: &SkillCatalogEntry) -> Self { - Self { - authority: entry.authority.clone(), - package: entry.id.clone(), - } - } -} diff --git a/codex-rs/ext/skills/src/sources.rs b/codex-rs/ext/skills/src/sources.rs index 387b9f6566..cd9853e5f6 100644 --- a/codex-rs/ext/skills/src/sources.rs +++ b/codex-rs/ext/skills/src/sources.rs @@ -1,240 +1,385 @@ -use std::fmt; +use std::collections::HashSet; use std::sync::Arc; +use std::time::Duration; -use codex_core_skills::runtime::SkillReadRequest as RuntimeSkillReadRequest; +use codex_core_skills::runtime::SkillAuthority; +use codex_core_skills::runtime::SkillCatalog; +use codex_core_skills::runtime::SkillCatalogEntry; +use codex_core_skills::runtime::SkillPackageId; +use codex_core_skills::runtime::SkillReadRequest; +use codex_core_skills::runtime::SkillReadResult; +use codex_core_skills::runtime::SkillResourceId; use codex_core_skills::runtime::SkillSource; +use codex_core_skills::runtime::SkillSourceError; use codex_core_skills::runtime::SkillSourceFuture; use codex_core_skills::runtime::SkillSourceIdentity; +use codex_core_skills::runtime::SkillSourceKind; +use codex_core_skills::runtime::SkillSourceResult; use codex_core_skills::runtime::SkillSources; +use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::McpResourceClient; +use codex_protocol::mcp::Resource; +use codex_protocol::mcp::ResourceContent; +use url::Url; -use crate::catalog::SkillCatalog; -use crate::catalog::SkillProviderError; -use crate::catalog::SkillProviderResult; -use crate::catalog::SkillReadResult; -use crate::catalog::SkillSearchResult; -use crate::catalog::SkillSourceKind; -use crate::provider::SkillListQuery; -use crate::provider::SkillProvider; -use crate::provider::SkillReadRequest; -use crate::provider::SkillSearchRequest; +use crate::state::SkillsThreadState; -#[derive(Clone)] -pub struct SkillProviderSource { - kind: SkillSourceKind, - label: String, - provider: Arc, - identity: SkillSourceIdentity, -} +const ORCHESTRATOR_SKILL_MIME_TYPE: &str = "mcp/skill"; +const ORCHESTRATOR_SKILL_DISCOVERY_TIMEOUT: Duration = Duration::from_secs(10); +const ORCHESTRATOR_SKILL_READ_TIMEOUT: Duration = Duration::from_secs(10); +const MAX_RESOURCE_PAGES: usize = 10; +const MAX_ORCHESTRATOR_SKILLS: usize = 100; +const MAX_SKILL_NAME_CHARS: usize = 64; +const MAX_QUALIFIED_SKILL_NAME_CHARS: usize = 128; +const MAX_SKILL_PACKAGE_URI_CHARS: usize = 1_024; +const MAX_SKILL_RESOURCE_URI_CHARS: usize = 2_048; +const MAX_SKILL_RESOURCE_CONTENT_BYTES: usize = 1024 * 1024; -impl SkillProviderSource { - pub fn new( - kind: SkillSourceKind, - label: impl Into, - provider: Arc, - ) -> Self { - let identity = SkillSourceIdentity::from_owner(Arc::new(Arc::clone(&provider))); - Self { - kind, - label: label.into(), - provider, - identity, - } - } - - pub fn host(label: impl Into, provider: Arc) -> Self { - Self::new(SkillSourceKind::Host, label, provider) - } - - pub fn executor(label: impl Into, provider: Arc) -> Self { - Self::new(SkillSourceKind::Executor, label, provider) - } - - pub fn orchestrator(label: impl Into, provider: Arc) -> Self { - Self::new(SkillSourceKind::Orchestrator, label, provider) - } - - fn should_list(&self, query: &SkillListQuery) -> bool { - match &self.kind { - SkillSourceKind::Host => query.include_host_skills, - SkillSourceKind::Executor => !query.executor_roots.is_empty(), - SkillSourceKind::Orchestrator => query.include_orchestrator_skills, - SkillSourceKind::Custom(_) => true, - } - } - - fn owns_kind(&self, kind: &SkillSourceKind) -> bool { - &self.kind == kind - } - - fn bind(&self, query: SkillListQuery) -> Arc { - Arc::new(BoundSkillProvider { - kind: self.kind.clone(), - provider: Arc::clone(&self.provider), - identity: self.identity.clone(), - host_snapshot: query.host_snapshot.clone(), - mcp_resources: query.mcp_resources.clone(), - query: Some(query), - }) - } -} - -impl fmt::Debug for SkillProviderSource { - fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - formatter - .debug_struct("SkillProviderSource") - .field("kind", &self.kind) - .field("label", &self.label) - .finish() - } -} - -#[derive(Clone, Default, Debug)] -pub struct SkillProviders { - sources: Vec, -} - -impl SkillProviders { - pub fn new() -> Self { - Self::default() - } - - pub fn with_provider(mut self, source: SkillProviderSource) -> Self { - self.sources.push(source); - self - } - - pub fn with_host_provider(mut self, provider: Arc) -> Self { - self.sources - .push(SkillProviderSource::host("host", provider)); - self - } - - pub fn with_executor_provider(mut self, provider: Arc) -> Self { - self.sources - .push(SkillProviderSource::executor("executor", provider)); - self - } - - pub fn with_orchestrator_provider(mut self, provider: Arc) -> Self { - self.sources - .push(SkillProviderSource::orchestrator("orchestrator", provider)); - self - } - - pub(crate) fn has_orchestrator_provider(&self) -> bool { - self.sources - .iter() - .any(|source| source.kind == SkillSourceKind::Orchestrator) - } - - pub(crate) async fn list_for_turn(&self, query: SkillListQuery) -> SkillCatalog { - self.sources_for_turn(query).list().await - } - - pub(crate) async fn list_orchestrator_for_turn( - &self, - query: SkillListQuery, - ) -> SkillProviderResult { - self.sources_for_turn(query) - .list_kind(&SkillSourceKind::Orchestrator) - .await - } - - fn sources_for_turn(&self, query: SkillListQuery) -> SkillSources { - self.sources - .iter() - .filter(|source| source.should_list(&query)) - .fold(SkillSources::new(), |sources, source| { - sources.with_source(source.label.clone(), source.bind(query.clone())) - }) - } - - pub(crate) async fn read( - &self, - request: SkillReadRequest, - ) -> Result { - let mut sources = self - .sources - .iter() - .filter(|source| source.owns_kind(&request.authority.kind)); - let Some(source) = sources.next() else { - return Err(SkillProviderError::new(format!( - "{} skill provider is not configured", - request.authority.kind - ))); - }; - if sources.next().is_some() { - return Err(SkillProviderError::new(format!( - "{} skill authority is ambiguous", - request.authority.kind - ))); - } - source.provider.read(request).await - } - - pub async fn search( - &self, - request: SkillSearchRequest, - ) -> Result { - let mut last_error = None; - for source in self - .sources - .iter() - .filter(|source| source.owns_kind(&request.authority.kind)) - { - match source.provider.search(request.clone()).await { - Ok(result) => return Ok(result), - Err(err) => last_error = Some(err), - } - } - - match last_error { - Some(err) => Err(err), - None => Err(SkillProviderError::new(format!( - "{} skill provider is not configured", - request.authority.kind - ))), - } - } -} - -struct BoundSkillProvider { - kind: SkillSourceKind, - provider: Arc, - identity: SkillSourceIdentity, - host_snapshot: Option>, +pub(crate) fn orchestrator_skill_sources( + thread_state: Arc, mcp_resources: Option>, - query: Option, +) -> SkillSources { + SkillSources::new().with_source_factory( + "orchestrator", + Arc::new(move || { + orchestrator_skill_source(Arc::clone(&thread_state), mcp_resources.clone()) + }), + ) } -impl SkillSource for BoundSkillProvider { - fn kind(&self) -> SkillSourceKind { - self.kind.clone() - } +pub(crate) fn orchestrator_skill_source( + thread_state: Arc, + mcp_resources: Option>, +) -> Arc { + // Bind listing and reads to the manager generation current when this source is created. + let mcp_resources = mcp_resources.map(|client| Arc::new(client.snapshot())); + let identity = mcp_resources + .as_ref() + .map(|client| SkillSourceIdentity::from_owner(client.manager_snapshot())) + .unwrap_or_else(|| SkillSourceIdentity::from_owner(Arc::clone(&thread_state))); + Arc::new(OrchestratorSkillSource { + identity, + thread_state, + mcp_resources, + }) +} +struct OrchestratorSkillSource { + identity: SkillSourceIdentity, + thread_state: Arc, + mcp_resources: Option>, +} + +impl SkillSource for OrchestratorSkillSource { fn identity(&self) -> SkillSourceIdentity { self.identity.clone() } fn list(&self) -> SkillSourceFuture<'_, SkillCatalog> { - let Some(query) = self.query.clone() else { - return Box::pin(async { - Err(SkillProviderError::new( - "skill source was not bound for catalog listing", - )) - }); - }; - self.provider.list(query) + Box::pin(async move { + if !self.thread_state.orchestrator_skills_enabled() { + return Ok(SkillCatalog::default()); + } + Ok(self + .thread_state + .orchestrator_catalog_snapshot( + self.mcp_resources.as_deref(), + discover_orchestrator_skills(self.mcp_resources.as_deref()), + ) + .await) + }) } - fn read(&self, request: RuntimeSkillReadRequest) -> SkillSourceFuture<'_, SkillReadResult> { - self.provider.read(SkillReadRequest { - authority: request.authority, - package: request.package, - resource: request.resource, - host_snapshot: self.host_snapshot.clone(), - mcp_resources: self.mcp_resources.clone(), + fn read(&self, request: SkillReadRequest) -> SkillSourceFuture<'_, SkillReadResult> { + Box::pin(async move { + let read_request = request.clone(); + self.thread_state + .read_orchestrator_resource( + &request, + self.mcp_resources.as_deref(), + read_orchestrator_skill(self.mcp_resources.as_deref(), read_request), + ) + .await }) } } + +async fn discover_orchestrator_skills( + client: Option<&McpResourceClient>, +) -> SkillSourceResult { + let Some(client) = client else { + return Ok(SkillCatalog::default()); + }; + if !client.has_server(CODEX_APPS_MCP_SERVER_NAME).await { + return Ok(SkillCatalog::default()); + } + + let discovery_deadline = tokio::time::Instant::now() + ORCHESTRATOR_SKILL_DISCOVERY_TIMEOUT; + let mut catalog = SkillCatalog::default(); + let mut cursor = None; + let mut seen_cursors = HashSet::new(); + let mut skill_resources_seen = 0usize; + let mut skipped_resources = 0usize; + let mut truncated = false; + let mut completed_pages = 0usize; + + for _ in 0..MAX_RESOURCE_PAGES { + let page = match tokio::time::timeout_at( + discovery_deadline, + client.list_resources(CODEX_APPS_MCP_SERVER_NAME, cursor.clone()), + ) + .await + { + Ok(result) => result.map_err(|err| { + SkillSourceError::new(format!( + "failed to list orchestrator skill resources: {err:#}" + )) + }), + Err(_) => Err(SkillSourceError::new(format!( + "orchestrator skill discovery timed out after {ORCHESTRATOR_SKILL_DISCOVERY_TIMEOUT:?}" + ))), + }; + let result = match page { + Ok(result) => result, + Err(err) if completed_pages == 0 => return Err(err), + Err(err) => { + let page_word = if completed_pages == 1 { + "page" + } else { + "pages" + }; + catalog.warnings.push(format!( + "Orchestrator skill discovery stopped after {completed_pages} resource {page_word}: {}", + err.message + )); + cursor = None; + break; + } + }; + completed_pages = completed_pages.saturating_add(1); + + for resource in &result.resources { + if resource.mime_type.as_deref() != Some(ORCHESTRATOR_SKILL_MIME_TYPE) { + continue; + } + if skill_resources_seen >= MAX_ORCHESTRATOR_SKILLS { + truncated = true; + break; + } + skill_resources_seen = skill_resources_seen.saturating_add(1); + match catalog_entry_from_resource(resource) { + Some(entry) => catalog.push_entry(entry), + None => skipped_resources = skipped_resources.saturating_add(1), + } + } + + if truncated { + break; + } + let Some(next_cursor) = result.next_cursor else { + cursor = None; + break; + }; + if !seen_cursors.insert(next_cursor.clone()) { + catalog.warnings.push( + "Orchestrator skill resource pagination returned a duplicate cursor.".to_string(), + ); + cursor = None; + break; + } + cursor = Some(next_cursor); + } + + if cursor.is_some() || truncated { + catalog.warnings.push(format!( + "Orchestrator skill discovery was truncated at {MAX_ORCHESTRATOR_SKILLS} skills or {MAX_RESOURCE_PAGES} resource pages." + )); + } + if skipped_resources > 0 { + catalog.warnings.push(format!( + "Skipped {skipped_resources} malformed orchestrator skill resources." + )); + } + + Ok(catalog) +} + +async fn read_orchestrator_skill( + client: Option<&McpResourceClient>, + request: SkillReadRequest, +) -> SkillSourceResult { + if request.authority != orchestrator_authority() { + return Err(SkillSourceError::new(format!( + "orchestrator skill source cannot read authority {}", + request.authority.id + ))); + } + if !resource_belongs_to_package(&request.package.0, request.resource.as_str()) { + return Err(SkillSourceError::new( + "orchestrator skill resource does not match its package", + )); + } + + let Some(client) = client else { + return Err(SkillSourceError::new( + "session MCP resource client is not configured", + )); + }; + let result = tokio::time::timeout( + ORCHESTRATOR_SKILL_READ_TIMEOUT, + client.read_resource(CODEX_APPS_MCP_SERVER_NAME, request.resource.as_str()), + ) + .await + .map_err(|_| { + SkillSourceError::new(format!( + "orchestrator skill read timed out after {ORCHESTRATOR_SKILL_READ_TIMEOUT:?}" + )) + })? + .map_err(|err| { + SkillSourceError::new(format!( + "failed to read orchestrator skill resource {}: {err:#}", + request.resource.as_str() + )) + })?; + let contents = result + .contents + .into_iter() + .find_map(|contents| match contents { + ResourceContent::Text { uri, text, .. } if uri == request.resource.as_str() => { + Some(text) + } + ResourceContent::Text { .. } | ResourceContent::Blob { .. } => None, + }); + let Some(contents) = contents else { + return Err(SkillSourceError::new(format!( + "orchestrator skill resource {} did not return matching text contents", + request.resource.as_str() + ))); + }; + if contents.len() > MAX_SKILL_RESOURCE_CONTENT_BYTES { + return Err(SkillSourceError::new(format!( + "orchestrator skill resource {} exceeds the {MAX_SKILL_RESOURCE_CONTENT_BYTES}-byte read limit", + request.resource.as_str() + ))); + } + + Ok(SkillReadResult { + resource: request.resource, + contents, + }) +} + +fn catalog_entry_from_resource(resource: &Resource) -> Option { + let uri = validated_skill_uri(resource.uri.as_str(), MAX_SKILL_PACKAGE_URI_CHARS)?; + let meta = resource.meta.as_ref()?.as_object()?; + let skill_name = normalized_label(meta.get("skill_name")?.as_str()?, MAX_SKILL_NAME_CHARS)?; + let name = if meta.get("source").and_then(|value| value.as_str()) == Some("user") { + skill_name + } else { + let plugin_name = + normalized_label(meta.get("plugin_name")?.as_str()?, MAX_SKILL_NAME_CHARS)?; + let qualified_name = format!("{plugin_name}:{skill_name}"); + (qualified_name.chars().count() <= MAX_QUALIFIED_SKILL_NAME_CHARS) + .then_some(qualified_name)? + }; + let description = normalized_description(resource.description.as_deref().unwrap_or_default())?; + let main_prompt = main_prompt_uri(uri); + + Some( + SkillCatalogEntry::new( + SkillPackageId(uri.to_string()), + orchestrator_authority(), + name, + description, + SkillResourceId::new(main_prompt), + ) + .with_display_path(uri), + ) +} + +fn orchestrator_authority() -> SkillAuthority { + SkillAuthority::new(SkillSourceKind::Orchestrator, CODEX_APPS_MCP_SERVER_NAME) +} + +fn validated_skill_uri(uri: &str, max_chars: usize) -> Option<&str> { + validated_skill_url(uri, max_chars).map(|_| uri) +} + +fn validated_skill_url(uri: &str, max_chars: usize) -> Option { + if uri.chars().count() > max_chars + || uri + .chars() + .any(|ch| ch.is_control() || ch.is_whitespace() || matches!(ch, '<' | '>')) + { + return None; + } + + let url = Url::parse(uri).ok()?; + let path_is_valid = url.path_segments().is_some_and(|segments| { + let segments = segments.collect::>(); + !segments.is_empty() && segments.iter().all(|segment| !segment.is_empty()) + }); + (url.scheme() == "skill" + && url.as_str() == uri + && url.host_str().is_some_and(|host| !host.is_empty()) + && url.username().is_empty() + && url.password().is_none() + && url.port().is_none() + && url.query().is_none() + && url.fragment().is_none() + && path_is_valid) + .then_some(url) +} + +fn resource_belongs_to_package(package: &str, resource: &str) -> bool { + let Some(package) = validated_skill_url(package, MAX_SKILL_PACKAGE_URI_CHARS) else { + return false; + }; + let Some(resource) = validated_skill_url(resource, MAX_SKILL_RESOURCE_URI_CHARS) else { + return false; + }; + + let Some(package_segments) = package.path_segments() else { + return false; + }; + let Some(resource_segments) = resource.path_segments() else { + return false; + }; + let package_segments = package_segments.collect::>(); + let resource_segments = resource_segments.collect::>(); + + package.scheme() == resource.scheme() + && package.host_str() == resource.host_str() + && resource_segments.len() > package_segments.len() + && resource_segments.starts_with(&package_segments) +} + +fn normalized_label(value: &str, max_chars: usize) -> Option { + let value = normalized_single_line(value, max_chars)?; + let invalid = value.is_empty() || value.chars().any(|ch| matches!(ch, '&' | '<' | '>')); + (!invalid).then_some(value) +} + +fn normalized_description(value: &str) -> Option { + let value = value.split_whitespace().collect::>().join(" "); + if value.chars().any(char::is_control) { + return None; + } + + Some( + value + .replace('&', "&") + .replace('<', "<") + .replace('>', ">"), + ) +} + +fn normalized_single_line(value: &str, max_chars: usize) -> Option { + let value = value.split_whitespace().collect::>().join(" "); + let valid = value.chars().count() <= max_chars && !value.chars().any(char::is_control); + valid.then_some(value) +} + +fn main_prompt_uri(package_uri: &str) -> String { + format!("{}/SKILL.md", package_uri.trim_end_matches('/')) +} diff --git a/codex-rs/ext/skills/src/state.rs b/codex-rs/ext/skills/src/state.rs index 0b751ebdc2..307c8241ae 100644 --- a/codex-rs/ext/skills/src/state.rs +++ b/codex-rs/ext/skills/src/state.rs @@ -2,99 +2,79 @@ use std::collections::HashMap; use std::future::Future; use std::sync::Arc; use std::sync::Mutex; +use std::sync::atomic::AtomicBool; +use std::sync::atomic::Ordering; +use codex_core_skills::runtime::SkillAuthority; +use codex_core_skills::runtime::SkillCatalog; +use codex_core_skills::runtime::SkillPackageId; +use codex_core_skills::runtime::SkillReadRequest; +use codex_core_skills::runtime::SkillReadResult; +use codex_core_skills::runtime::SkillResourceId; +use codex_core_skills::runtime::SkillSourceError; +use codex_core_skills::runtime::SkillSourceResult; use codex_mcp::McpResourceClient; use codex_mcp::McpResourceClientCacheKey; -use codex_protocol::capabilities::SelectedCapabilityRoot; use tokio::sync::OnceCell; -use crate::SkillsExtensionConfig; -use crate::catalog::SkillAuthority; -use crate::catalog::SkillCatalog; -use crate::catalog::SkillCatalogEntry; -use crate::catalog::SkillPackageId; -use crate::catalog::SkillProviderError; -use crate::catalog::SkillProviderResult; -use crate::catalog::SkillReadResult; -use crate::catalog::SkillResourceId; -use crate::catalog::SkillSourceKind; -use crate::provider::SkillReadRequest; -use crate::sources::SkillProviders; - const MAX_CACHED_ORCHESTRATOR_RESOURCES: usize = 100; const MAX_CACHED_ORCHESTRATOR_CONTENT_BYTES: usize = 8 * 1024 * 1024; pub(crate) struct SkillsThreadState { - config: Mutex, - selected_roots: Vec, + orchestrator_skills_enabled: AtomicBool, orchestrator_skills_available: bool, orchestrator_cache: Mutex>>, } impl SkillsThreadState { pub(crate) fn new( - config: SkillsExtensionConfig, - selected_roots: Vec, + orchestrator_skills_enabled: bool, orchestrator_skills_available: bool, ) -> Self { Self { - config: Mutex::new(config), - selected_roots, + orchestrator_skills_enabled: AtomicBool::new(orchestrator_skills_enabled), orchestrator_skills_available, orchestrator_cache: Mutex::new(None), } } - pub(crate) fn config(&self) -> SkillsExtensionConfig { - self.config - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .clone() - } - - pub(crate) fn set_config(&self, config: SkillsExtensionConfig) { - *self - .config - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) = config; - } - - pub(crate) fn selected_roots(&self) -> &[SelectedCapabilityRoot] { - &self.selected_roots + pub(crate) fn set_orchestrator_skills_enabled(&self, enabled: bool) { + self.orchestrator_skills_enabled + .store(enabled, Ordering::Relaxed); } pub(crate) fn orchestrator_skills_enabled(&self) -> bool { - self.orchestrator_skills_available && self.config().orchestrator_skills_enabled + self.orchestrator_skills_available + && self.orchestrator_skills_enabled.load(Ordering::Relaxed) } pub(crate) async fn orchestrator_catalog_snapshot( &self, mcp_resources: Option<&McpResourceClient>, - initialize: impl Future> + Send, + initialize: impl Future> + Send, ) -> SkillCatalog { - self.orchestrator_cache(mcp_resources) - .catalog - .get_or_init(|| async { - initialize.await.unwrap_or_else(|err| SkillCatalog { - warnings: vec![err.message], - ..Default::default() - }) - }) - .await - .clone() + let cache = self.orchestrator_cache(mcp_resources); + if let Some(catalog) = cache.catalog.get() { + return catalog.clone(); + } + let catalog = initialize.await.unwrap_or_else(|err| SkillCatalog { + warnings: vec![err.message], + ..Default::default() + }); + if !catalog.entries.is_empty() && catalog.warnings.is_empty() { + let _ = cache.catalog.set(catalog.clone()); + } + catalog } - pub(crate) async fn read_skill( + pub(crate) async fn read_orchestrator_resource( &self, - providers: &SkillProviders, - request: SkillReadRequest, - ) -> SkillProviderResult { - if request.authority.kind != SkillSourceKind::Orchestrator { - return providers.read(request).await; - } - - let cache = self.orchestrator_cache(request.mcp_resources.as_deref()); - let cache_key = SkillReadCacheKey::from(&request); + request: &SkillReadRequest, + mcp_resources: Option<&McpResourceClient>, + read: impl Future> + Send, + ) -> SkillSourceResult { + let cache_key = SkillReadCacheKey::from(request); + let cache = self.orchestrator_cache(mcp_resources); if let Some(result) = cache .resources .lock() @@ -104,7 +84,7 @@ impl SkillsThreadState { return Ok(result); } - let result = providers.read(request).await?; + let result = read.await?; if result.resource != cache_key.resource { return Ok(result); } @@ -196,11 +176,3 @@ impl OrchestratorResourceCache { result } } - -#[derive(Clone, Debug, Default, PartialEq, Eq)] -pub(crate) struct SkillsTurnState { - pub(crate) catalog: SkillCatalog, - pub(crate) selected_entries: Vec, - pub(crate) warnings: Vec, - pub(crate) main_prompts_injected: bool, -} diff --git a/codex-rs/ext/skills/src/tools/list.rs b/codex-rs/ext/skills/src/tools/list.rs index e556f9783f..6433819f48 100644 --- a/codex-rs/ext/skills/src/tools/list.rs +++ b/codex-rs/ext/skills/src/tools/list.rs @@ -7,9 +7,9 @@ use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; -use crate::catalog::SkillCatalogEntry; use crate::render::truncate_catalog_skill_description; use crate::render::truncate_utf8_to_bytes; +use codex_core_skills::runtime::SkillCatalogEntry; use super::MAX_HANDLE_BYTES; use super::SkillToolAuthority; @@ -68,7 +68,8 @@ impl ToolExecutor for ListTool { Box::pin(async move { let args: ListArgs = parse_args(&call)?; let authority = args.authority.into_authority(); - let catalog = self.context.catalog(&call.turn_id, args.authority).await; + let source = self.context.source(); + let catalog = self.context.catalog(source.as_ref()).await; let response = ListResponse { skills: catalog .entries diff --git a/codex-rs/ext/skills/src/tools/mod.rs b/codex-rs/ext/skills/src/tools/mod.rs index 0d05064afb..7e9b319bff 100644 --- a/codex-rs/ext/skills/src/tools/mod.rs +++ b/codex-rs/ext/skills/src/tools/mod.rs @@ -1,5 +1,9 @@ use std::sync::Arc; +use codex_core_skills::runtime::SkillAuthority; +use codex_core_skills::runtime::SkillCatalog; +use codex_core_skills::runtime::SkillSource; +use codex_core_skills::runtime::SkillSourceKind; use codex_extension_api::FunctionCallError; use codex_extension_api::JsonToolOutput; use codex_extension_api::ResponsesApiTool; @@ -19,11 +23,7 @@ use serde::Deserialize; use serde::Serialize; use serde_json::Value; -use crate::catalog::SkillAuthority; -use crate::catalog::SkillCatalog; -use crate::catalog::SkillSourceKind; -use crate::provider::SkillListQuery; -use crate::sources::SkillProviders; +use crate::sources::orchestrator_skill_source; use crate::state::SkillsThreadState; mod list; @@ -34,13 +34,11 @@ const SKILLS_NAMESPACE: &str = "skills"; const MAX_HANDLE_BYTES: usize = 2_048; pub(crate) fn skill_tools( - providers: SkillProviders, mcp_resources: Option>, thread_state: Arc, ) -> Vec>> { let context = SkillToolContext { - providers, - mcp_resources, + mcp_resources: mcp_resources.map(|client| Arc::new(client.snapshot())), thread_state, }; vec![ @@ -53,30 +51,22 @@ pub(crate) fn skill_tools( #[derive(Clone)] struct SkillToolContext { - providers: SkillProviders, mcp_resources: Option>, thread_state: Arc, } impl SkillToolContext { - async fn catalog(&self, turn_id: &str, authority: SkillToolAuthority) -> SkillCatalog { - match authority { - SkillToolAuthority::Orchestrator => { - self.thread_state - .orchestrator_catalog_snapshot( - self.mcp_resources.as_deref(), - self.providers.list_orchestrator_for_turn(SkillListQuery { - turn_id: turn_id.to_string(), - executor_roots: Vec::new(), - host_snapshot: None, - include_host_skills: false, - include_bundled_skills: false, - include_orchestrator_skills: true, - mcp_resources: self.mcp_resources.clone(), - }), - ) - .await - } + fn source(&self) -> Arc { + orchestrator_skill_source(Arc::clone(&self.thread_state), self.mcp_resources.clone()) + } + + async fn catalog(&self, source: &dyn SkillSource) -> SkillCatalog { + match source.list().await { + Ok(catalog) => catalog, + Err(err) => SkillCatalog { + warnings: vec![err.message], + ..Default::default() + }, } } } diff --git a/codex-rs/ext/skills/src/tools/read.rs b/codex-rs/ext/skills/src/tools/read.rs index 64587cab95..ff29e61579 100644 --- a/codex-rs/ext/skills/src/tools/read.rs +++ b/codex-rs/ext/skills/src/tools/read.rs @@ -1,3 +1,6 @@ +use codex_core_skills::runtime::SkillPackageId; +use codex_core_skills::runtime::SkillReadRequest; +use codex_core_skills::runtime::SkillResourceId; use codex_extension_api::FunctionCallError; use codex_extension_api::ToolCall; use codex_extension_api::ToolExecutor; @@ -8,10 +11,6 @@ use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; -use crate::catalog::SkillPackageId; -use crate::catalog::SkillResourceId; -use crate::provider::SkillReadRequest; - use super::MAX_HANDLE_BYTES; use super::SkillToolAuthority; use super::SkillToolContext; @@ -62,7 +61,8 @@ impl ToolExecutor for ReadTool { validate_handle("package", &args.package, MAX_HANDLE_BYTES)?; validate_handle("resource", &args.resource, MAX_HANDLE_BYTES)?; - let catalog = self.context.catalog(&call.turn_id, args.authority).await; + let source = self.context.source(); + let catalog = self.context.catalog(source.as_ref()).await; let package_is_available = catalog.entries.iter().any(|entry| { entry.enabled && entry.authority == authority && entry.id.0 == args.package }); @@ -73,19 +73,12 @@ impl ToolExecutor for ReadTool { } let requested_resource = SkillResourceId::new(args.resource); - let result = self - .context - .thread_state - .read_skill( - &self.context.providers, - SkillReadRequest { - authority, - package: SkillPackageId(args.package), - resource: requested_resource.clone(), - host_snapshot: None, - mcp_resources: self.context.mcp_resources.clone(), - }, - ) + let result = source + .read(SkillReadRequest { + authority, + package: SkillPackageId(args.package), + resource: requested_resource.clone(), + }) .await .map_err(|err| { tracing::warn!( diff --git a/codex-rs/ext/skills/tests/executor_file_system_authority.rs b/codex-rs/ext/skills/tests/executor_file_system_authority.rs deleted file mode 100644 index 06bf062eac..0000000000 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ /dev/null @@ -1,312 +0,0 @@ -use std::io; -use std::sync::Arc; -use std::sync::atomic::AtomicUsize; -use std::sync::atomic::Ordering; - -use codex_core_skills::HostSkillsSnapshot; -use codex_core_skills::loader::SkillRoot; -use codex_core_skills::loader::load_skills_from_roots; -use codex_exec_server::CopyOptions; -use codex_exec_server::CreateDirectoryOptions; -use codex_exec_server::EnvironmentManager; -use codex_exec_server::ExecutorFileSystem; -use codex_exec_server::ExecutorFileSystemFuture; -use codex_exec_server::FileMetadata; -use codex_exec_server::FileSystemReadStream; -use codex_exec_server::FileSystemSandboxContext; -use codex_exec_server::ReadDirectoryEntry; -use codex_exec_server::RemoveOptions; -use codex_protocol::capabilities::CapabilityRootLocation; -use codex_protocol::capabilities::SelectedCapabilityRoot; -use codex_protocol::protocol::SkillScope; -use codex_skills_extension::ExecutorSkillProvider; -use codex_skills_extension::provider::SkillListQuery; -use codex_skills_extension::provider::SkillProvider; -use codex_utils_absolute_path::AbsolutePathBuf; -use codex_utils_path_uri::PathUri; -use pretty_assertions::assert_eq; - -const SKILL_CONTENTS: &str = - "---\nname: synthetic\ndescription: Synthetic executor skill.\n---\n\nEXECUTOR_ONLY_BODY\n"; -const PLUGIN_MANIFEST: &str = r#"{"name":"synthetic-plugin"}"#; -static NEXT_TEST_ROOT_ID: AtomicUsize = AtomicUsize::new(0); - -struct SyntheticFileSystem { - alias_root: PathUri, - canonical_root: PathUri, - has_plugin_manifest: bool, -} - -impl SyntheticFileSystem { - fn path(&self, relative_path: &str) -> io::Result { - self.canonical_root - .join(relative_path) - .map_err(io::Error::other) - } - - async fn canonicalize(&self, path: &PathUri) -> io::Result { - if path == &self.alias_root { - return Ok(self.canonical_root.clone()); - } - self.metadata(path)?; - Ok(path.clone()) - } - - async fn read_file(&self, path: &PathUri) -> io::Result> { - if path == &self.path("skill/SKILL.md")? { - Ok(SKILL_CONTENTS.as_bytes().to_vec()) - } else if self.has_plugin_manifest && path == &self.path(".claude-plugin/plugin.json")? { - Ok(PLUGIN_MANIFEST.as_bytes().to_vec()) - } else { - Err(io::Error::new(io::ErrorKind::NotFound, "not found")) - } - } - - async fn read_directory(&self, path: &PathUri) -> io::Result> { - if path == &self.canonical_root { - Ok(vec![ReadDirectoryEntry { - file_name: "skill".to_string(), - is_directory: true, - is_file: false, - }]) - } else if path == &self.path("skill")? { - Ok(vec![ReadDirectoryEntry { - file_name: "SKILL.md".to_string(), - is_directory: false, - is_file: true, - }]) - } else { - Err(io::Error::new(io::ErrorKind::NotFound, "not found")) - } - } - - fn metadata(&self, path: &PathUri) -> io::Result { - let skill_dir = self.path("skill")?; - let skill_path = self.path("skill/SKILL.md")?; - let manifest_path = self.path(".claude-plugin/plugin.json")?; - let (is_directory, is_file) = if path == &self.canonical_root || path == &skill_dir { - (true, false) - } else if path == &skill_path || self.has_plugin_manifest && path == &manifest_path { - (false, true) - } else { - return Err(io::Error::new(io::ErrorKind::NotFound, "not found")); - }; - Ok(FileMetadata { - is_directory, - is_file, - is_symlink: false, - size: 0, - created_at_ms: 0, - modified_at_ms: 0, - }) - } -} - -impl ExecutorFileSystem for SyntheticFileSystem { - fn canonicalize<'a>( - &'a self, - path: &'a PathUri, - _sandbox: Option<&'a FileSystemSandboxContext>, - ) -> ExecutorFileSystemFuture<'a, PathUri> { - Box::pin(SyntheticFileSystem::canonicalize(self, path)) - } - - fn read_file<'a>( - &'a self, - path: &'a PathUri, - _sandbox: Option<&'a FileSystemSandboxContext>, - ) -> ExecutorFileSystemFuture<'a, Vec> { - Box::pin(SyntheticFileSystem::read_file(self, path)) - } - - fn read_file_stream<'a>( - &'a self, - _path: &'a PathUri, - _sandbox: Option<&'a FileSystemSandboxContext>, - ) -> ExecutorFileSystemFuture<'a, FileSystemReadStream> { - Box::pin(async { - Err(io::Error::new( - io::ErrorKind::Unsupported, - "synthetic filesystem does not support streaming reads", - )) - }) - } - - fn write_file<'a>( - &'a self, - _path: &'a PathUri, - _contents: Vec, - _sandbox: Option<&'a FileSystemSandboxContext>, - ) -> ExecutorFileSystemFuture<'a, ()> { - Box::pin(async move { Err(io::Error::new(io::ErrorKind::Unsupported, "read only")) }) - } - - fn create_directory<'a>( - &'a self, - _path: &'a PathUri, - _options: CreateDirectoryOptions, - _sandbox: Option<&'a FileSystemSandboxContext>, - ) -> ExecutorFileSystemFuture<'a, ()> { - Box::pin(async move { Err(io::Error::new(io::ErrorKind::Unsupported, "read only")) }) - } - - fn get_metadata<'a>( - &'a self, - path: &'a PathUri, - _sandbox: Option<&'a FileSystemSandboxContext>, - ) -> ExecutorFileSystemFuture<'a, FileMetadata> { - Box::pin(async move { self.metadata(path) }) - } - - fn read_directory<'a>( - &'a self, - path: &'a PathUri, - _sandbox: Option<&'a FileSystemSandboxContext>, - ) -> ExecutorFileSystemFuture<'a, Vec> { - Box::pin(SyntheticFileSystem::read_directory(self, path)) - } - - fn remove<'a>( - &'a self, - _path: &'a PathUri, - _options: RemoveOptions, - _sandbox: Option<&'a FileSystemSandboxContext>, - ) -> ExecutorFileSystemFuture<'a, ()> { - Box::pin(async move { Err(io::Error::new(io::ErrorKind::Unsupported, "read only")) }) - } - - fn copy<'a>( - &'a self, - _source_path: &'a PathUri, - _destination_path: &'a PathUri, - _options: CopyOptions, - _sandbox: Option<&'a FileSystemSandboxContext>, - ) -> ExecutorFileSystemFuture<'a, ()> { - Box::pin(async move { Err(io::Error::new(io::ErrorKind::Unsupported, "read only")) }) - } -} - -#[tokio::test] -async fn skill_loading_and_reads_use_the_supplied_executor_file_system() { - let test_root = - std::env::temp_dir().join(format!("codex-executor-skill-fs-{}", std::process::id())); - let alias_root = AbsolutePathBuf::from_absolute_path_checked(test_root.join("alias")) - .expect("absolute path"); - let canonical_root = AbsolutePathBuf::from_absolute_path_checked(test_root.join("canonical")) - .expect("absolute path"); - assert!(!alias_root.as_path().exists()); - assert!(!canonical_root.as_path().exists()); - - let outcome = load_skills_from_roots( - [SkillRoot { - path: alias_root.clone(), - scope: SkillScope::User, - file_system: Arc::new(SyntheticFileSystem { - alias_root: PathUri::from_abs_path(&alias_root), - canonical_root: PathUri::from_abs_path(&canonical_root), - has_plugin_manifest: false, - }), - plugin_id: None, - plugin_namespace: None, - plugin_root: None, - }], - /*plugin_skill_snapshots*/ None, - ) - .await; - assert_eq!(outcome.errors, Vec::new()); - assert_eq!(outcome.skills.len(), 1); - - let skill = outcome.skills[0].clone(); - assert_eq!(skill.name, "synthetic"); - assert_eq!( - skill.path_to_skills_md, - canonical_root.join("skill/SKILL.md") - ); - let loaded = HostSkillsSnapshot::new(Arc::new(outcome)); - assert_eq!( - loaded.read_skill_text(&skill).await.expect("skill body"), - SKILL_CONTENTS - ); -} - -#[tokio::test] -async fn selected_root_id_distinguishes_identical_executor_paths() { - let root_label = if cfg!(unix) { - r"root\identity" - } else { - "root-identity" - }; - let test_root = create_local_skill_root(root_label).expect("create local skill root"); - let selected_root = test_root.to_string_lossy().into_owned(); - let selected_root = if cfg!(windows) { - selected_root.replace('\\', "/") - } else { - selected_root - }; - let provider = ExecutorSkillProvider::new_with_restriction_product( - Arc::new(EnvironmentManager::default_for_tests()), - /*restriction_product*/ None, - ); - let catalog = provider - .list(SkillListQuery { - turn_id: "turn-1".to_string(), - executor_roots: ["root-a", "root-b"] - .into_iter() - .map(|id| SelectedCapabilityRoot { - id: id.to_string(), - location: CapabilityRootLocation::Environment { - environment_id: "local".to_string(), - path: PathUri::from_host_native_path(&test_root).expect("skill root URI"), - }, - }) - .collect(), - host_snapshot: None, - include_host_skills: false, - include_bundled_skills: true, - include_orchestrator_skills: false, - mcp_resources: None, - }) - .await - .expect("list executor skills"); - - assert_eq!( - catalog - .entries - .iter() - .map(|entry| ( - entry.authority.id.clone(), - entry.display_path.clone().expect("display path"), - )) - .collect::>(), - vec![ - ( - "root-a".to_string(), - format!( - "skill://root-a/{}/skill/SKILL.md", - selected_root.trim_start_matches('/') - ), - ), - ( - "root-b".to_string(), - format!( - "skill://root-b/{}/skill/SKILL.md", - selected_root.trim_start_matches('/') - ), - ), - ] - ); - - std::fs::remove_dir_all(test_root).expect("remove skill directory"); -} - -fn create_local_skill_root(label: &str) -> io::Result { - let id = NEXT_TEST_ROOT_ID.fetch_add(1, Ordering::Relaxed); - let test_root = std::env::temp_dir().join(format!( - "codex-executor-skill-{label}-{}-{id}", - std::process::id() - )); - let skill_dir = test_root.join("skill"); - std::fs::create_dir_all(&skill_dir)?; - std::fs::write(skill_dir.join("SKILL.md"), SKILL_CONTENTS)?; - Ok(test_root) -} diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs deleted file mode 100644 index 761dc34d71..0000000000 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ /dev/null @@ -1,738 +0,0 @@ -use std::path::PathBuf; -use std::sync::Arc; -use std::sync::Mutex; -use std::sync::atomic::AtomicUsize; -use std::sync::atomic::Ordering; - -use codex_core_skills::HostSkillsSnapshot; -use codex_core_skills::SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS; -use codex_core_skills::SKILLS_INTRO_WITH_ABSOLUTE_PATHS; -use codex_core_skills::SkillLoadOutcome; -use codex_core_skills::SkillMetadata; -use codex_core_skills::injection::InjectedHostSkillPrompts; -use codex_extension_api::ConversationHistory; -use codex_extension_api::ExtensionData; -use codex_extension_api::ExtensionEventSink; -use codex_extension_api::ExtensionRegistryBuilder; -use codex_extension_api::NoopTurnItemEmitter; -use codex_extension_api::ThreadStartInput; -use codex_extension_api::ToolCall; -use codex_extension_api::ToolPayload; -use codex_extension_api::TurnInputContext; -use codex_protocol::capabilities::CapabilityRootLocation; -use codex_protocol::capabilities::SelectedCapabilityRoot; -use codex_protocol::protocol::Event; -use codex_protocol::protocol::EventMsg; -use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG; -use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; -use codex_protocol::protocol::SessionSource; -use codex_protocol::protocol::SkillScope; -use codex_protocol::protocol::TruncationPolicy; -use codex_protocol::user_input::UserInput; -use codex_skills_extension::SkillProviders; -use codex_skills_extension::SkillsExtensionConfig; -use codex_skills_extension::catalog::SkillAuthority; -use codex_skills_extension::catalog::SkillCatalog; -use codex_skills_extension::catalog::SkillCatalogEntry; -use codex_skills_extension::catalog::SkillPackageId; -use codex_skills_extension::catalog::SkillProviderError; -use codex_skills_extension::catalog::SkillReadResult; -use codex_skills_extension::catalog::SkillResourceId; -use codex_skills_extension::catalog::SkillSearchResult; -use codex_skills_extension::catalog::SkillSourceKind; -use codex_skills_extension::install; -use codex_skills_extension::install_with_providers; -use codex_skills_extension::provider::SkillListQuery; -use codex_skills_extension::provider::SkillProvider; -use codex_skills_extension::provider::SkillProviderFuture; -use codex_skills_extension::provider::SkillReadRequest; -use codex_skills_extension::provider::SkillSearchRequest; -use codex_utils_absolute_path::AbsolutePathBuf; -use codex_utils_path_uri::PathUri; -use pretty_assertions::assert_eq; - -type TestResult = Result<(), Box>; - -static NEXT_CODEX_HOME_ID: AtomicUsize = AtomicUsize::new(0); -const DEMO_SKILL_CONTENTS: &str = - "---\nname: demo\ndescription: Demo skill.\n---\n# Demo\n\nUse the demo skill.\n"; - -#[tokio::test] -async fn installed_extension_uses_host_service_snapshot() -> TestResult { - let codex_home = test_codex_home(); - let skill_path = codex_home.join("skills").join("demo").join("SKILL.md"); - std::fs::create_dir_all( - skill_path - .parent() - .ok_or("skill path should have a parent")?, - )?; - std::fs::write(&skill_path, DEMO_SKILL_CONTENTS)?; - let config = default_config(); - - let mut builder = ExtensionRegistryBuilder::new(); - install(&mut builder, skills_extension_config); - let registry = builder.build(); - let session_store = ExtensionData::new("session"); - let thread_store = ExtensionData::new("thread"); - let session_source = SessionSource::Cli; - registry.thread_lifecycle_contributors()[0] - .on_thread_start(ThreadStartInput { - config: &config, - session_source: &session_source, - persistent_thread_state_available: true, - environments: &[], - session_store: &session_store, - thread_store: &thread_store, - }) - .await; - - let skill_path = AbsolutePathBuf::try_from(skill_path)?; - let skill_path_string = skill_path.to_string_lossy().into_owned(); - let mut outcome = SkillLoadOutcome::default(); - outcome.skills.push(SkillMetadata { - name: "demo".to_string(), - description: "Demo skill.".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: skill_path, - scope: SkillScope::User, - plugin_id: None, - }); - let loaded_skills = Arc::new(outcome); - let skill_prompt_path = skill_path_string.replace('\\', "/"); - let turn_store = ExtensionData::new("turn-1"); - turn_store.insert(HostSkillsSnapshot::new(Arc::clone(&loaded_skills))); - - let fragments = registry.turn_input_contributors()[0] - .contribute( - TurnInputContext { - turn_id: "turn-1".to_string(), - user_input: vec![UserInput::Text { - text: "$demo".to_string(), - text_elements: Vec::new(), - }], - environments: Vec::new(), - }, - &session_store, - &thread_store, - &turn_store, - ) - .await; - - let expected_catalog = format!( - "{SKILLS_INSTRUCTIONS_OPEN_TAG}\n## Skills\n{SKILLS_INTRO_WITH_ABSOLUTE_PATHS}\n### Available skills\n- demo: Demo skill. (file: {skill_prompt_path})\n### How to use skills\n{SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS}\n{SKILLS_INSTRUCTIONS_CLOSE_TAG}" - ); - let expected_skill = format!( - "\ndemo\n{skill_prompt_path}\n{DEMO_SKILL_CONTENTS}\n" - ); - assert_eq!( - vec![("developer", expected_catalog), ("user", expected_skill),], - fragments - .iter() - .map(|fragment| (fragment.role(), fragment.render())) - .collect::>() - ); - let injected_host_skill_prompts = turn_store - .get::() - .ok_or("host skill prompt marker should be set")?; - assert!(injected_host_skill_prompts.contains_path(&skill_path_string)); - - std::fs::remove_dir_all(codex_home)?; - Ok(()) -} - -#[tokio::test] -async fn selected_executor_catalog_is_context_and_selected_entrypoint_is_turn_input() -> TestResult -{ - let read_requests = Arc::new(Mutex::new(Vec::new())); - let executor_provider = Arc::new(StaticSkillProvider { - catalog: SkillCatalog { - entries: vec![test_entry( - SkillSourceKind::Executor, - "env-1", - "executor/lint-fix", - "lint-fix/SKILL.md", - )], - warnings: Vec::new(), - }, - read_requests: Arc::clone(&read_requests), - list_calls: None, - fail_first_list: false, - }); - let providers = SkillProviders::new().with_executor_provider(executor_provider); - let mut builder = ExtensionRegistryBuilder::new(); - install_with_providers(&mut builder, providers, skills_extension_config); - let registry = builder.build(); - - let session_store = ExtensionData::new("session"); - let thread_store = ExtensionData::new("thread"); - thread_store.insert(vec![SelectedCapabilityRoot { - id: "lint-fix".to_string(), - location: CapabilityRootLocation::Environment { - environment_id: "env-1".to_string(), - path: PathUri::parse("file:///skills/lint-fix").expect("skill root URI"), - }, - }]); - let session_source = SessionSource::Cli; - let config = default_config(); - registry.thread_lifecycle_contributors()[0] - .on_thread_start(ThreadStartInput { - config: &config, - session_source: &session_source, - persistent_thread_state_available: true, - environments: &[], - session_store: &session_store, - thread_store: &thread_store, - }) - .await; - - let prompt_fragments = registry.context_contributors()[0] - .contribute_thread_context(&session_store, &thread_store) - .await; - assert_eq!(1, prompt_fragments.len()); - assert!( - prompt_fragments[0] - .text() - .starts_with(SKILLS_INSTRUCTIONS_OPEN_TAG) - ); - assert!(prompt_fragments[0].text().contains("lint-fix")); - assert!( - prompt_fragments[0] - .text() - .contains("(environment resource: skill://executor/lint-fix/SKILL.md)") - ); - - let turn_store = ExtensionData::new("turn-1"); - let fragments = registry.turn_input_contributors()[0] - .contribute( - TurnInputContext { - turn_id: "turn-1".to_string(), - user_input: vec![UserInput::Text { - text: "$lint-fix please".to_string(), - text_elements: Vec::new(), - }], - environments: Vec::new(), - }, - &session_store, - &thread_store, - &turn_store, - ) - .await; - - assert_eq!(1, fragments.len()); - assert_eq!("user", fragments[0].role()); - assert!(fragments[0].render().contains("lint-fix")); - assert!(fragments[0].render().contains("# Lint Fix")); - assert_eq!( - vec![( - SkillAuthority::new(SkillSourceKind::Executor, "env-1"), - SkillPackageId("executor/lint-fix".to_string()), - SkillResourceId::new("lint-fix/SKILL.md"), - )], - read_request_keys(&read_requests) - ); - let rebuilt_prompt_fragments = registry.context_contributors()[0] - .contribute_thread_context(&session_store, &thread_store) - .await; - assert_eq!(1, rebuilt_prompt_fragments.len()); - assert!(rebuilt_prompt_fragments[0].text().contains("lint-fix")); - - let next_turn_store = ExtensionData::new("turn-2"); - let next_fragments = registry.turn_input_contributors()[0] - .contribute( - TurnInputContext { - turn_id: "turn-2".to_string(), - user_input: vec![UserInput::Text { - text: "no skill this time".to_string(), - text_elements: Vec::new(), - }], - environments: Vec::new(), - }, - &session_store, - &thread_store, - &next_turn_store, - ) - .await; - - assert!(next_fragments.is_empty()); - - Ok(()) -} - -#[tokio::test] -async fn default_context_truncates_catalog_descriptions() -> TestResult { - let description = "x".repeat(1_025); - let mut entry = test_entry( - SkillSourceKind::Orchestrator, - "codex_apps", - "orchestrator/long-description", - "skill://orchestrator/long-description/SKILL.md", - ); - entry.description = description.clone(); - let providers = - SkillProviders::new().with_orchestrator_provider(Arc::new(StaticSkillProvider { - catalog: SkillCatalog { - entries: vec![entry], - warnings: Vec::new(), - }, - read_requests: Arc::new(Mutex::new(Vec::new())), - list_calls: None, - fail_first_list: false, - })); - let mut builder = ExtensionRegistryBuilder::new(); - install_with_providers(&mut builder, providers, skills_extension_config); - let registry = builder.build(); - let session_store = ExtensionData::new("session"); - let thread_store = ExtensionData::new("thread"); - let session_source = SessionSource::Cli; - let config = default_config(); - registry.thread_lifecycle_contributors()[0] - .on_thread_start(ThreadStartInput { - config: &config, - session_source: &session_source, - persistent_thread_state_available: true, - environments: &[], - session_store: &session_store, - thread_store: &thread_store, - }) - .await; - - let fragments = registry.context_contributors()[0] - .contribute_thread_context(&session_store, &thread_store) - .await; - assert_eq!(1, fragments.len()); - let rendered = fragments[0].text(); - assert!(rendered.contains(&("x".repeat(1_021) + "..."))); - assert!(!rendered.contains(&"x".repeat(1_024))); - assert!(!rendered.contains(&description)); - - Ok(()) -} - -#[tokio::test] -async fn skills_list_truncates_catalog_descriptions_in_tool_output() -> TestResult { - let description = "x".repeat(1_025); - let mut entry = test_entry( - SkillSourceKind::Orchestrator, - "codex_apps", - "orchestrator/long-description", - "skill://orchestrator/long-description/SKILL.md", - ); - entry.description = description.clone(); - let providers = - SkillProviders::new().with_orchestrator_provider(Arc::new(StaticSkillProvider { - catalog: SkillCatalog { - entries: vec![entry], - warnings: Vec::new(), - }, - read_requests: Arc::new(Mutex::new(Vec::new())), - list_calls: None, - fail_first_list: false, - })); - let mut builder = ExtensionRegistryBuilder::new(); - install_with_providers(&mut builder, providers, skills_extension_config); - let registry = builder.build(); - let session_store = ExtensionData::new("session"); - let thread_store = ExtensionData::new("thread"); - let session_source = SessionSource::Cli; - let config = default_config(); - registry.thread_lifecycle_contributors()[0] - .on_thread_start(ThreadStartInput { - config: &config, - session_source: &session_source, - persistent_thread_state_available: true, - environments: &[], - session_store: &session_store, - thread_store: &thread_store, - }) - .await; - - let tools = registry.tool_contributors()[0].tools(&session_store, &thread_store); - let list_tool = tools - .iter() - .find(|tool| tool.tool_name().name == "list") - .ok_or("skills.list tool should be registered")?; - let payload = ToolPayload::Function { - arguments: serde_json::json!({"authority": {"kind": "orchestrator"}}).to_string(), - }; - let output = list_tool - .handle(ToolCall { - turn_id: "turn-1".to_string(), - call_id: "call-1".to_string(), - tool_name: list_tool.tool_name(), - model: "gpt-test".to_string(), - truncation_policy: TruncationPolicy::Bytes(1_024), - conversation_history: ConversationHistory::default(), - turn_item_emitter: Arc::new(NoopTurnItemEmitter), - environments: Vec::new(), - payload: payload.clone(), - }) - .await?; - let response = output - .post_tool_use_response("call-1", &payload) - .ok_or("skills.list should expose structured output")?; - let rendered_description = response["skills"][0]["description"] - .as_str() - .ok_or("skills.list response should include a description")?; - - assert_eq!(rendered_description, "x".repeat(1_021) + "..."); - assert_ne!(rendered_description, description); - - Ok(()) -} - -#[tokio::test] -async fn orchestrator_catalog_snapshot_caches_failure() -> TestResult { - let list_calls = Arc::new(AtomicUsize::new(0)); - let providers = - SkillProviders::new().with_orchestrator_provider(Arc::new(StaticSkillProvider { - catalog: SkillCatalog { - entries: vec![test_entry( - SkillSourceKind::Orchestrator, - "codex_apps", - "orchestrator/first", - "skill://orchestrator/first/SKILL.md", - )], - warnings: Vec::new(), - }, - read_requests: Arc::new(Mutex::new(Vec::new())), - list_calls: Some(Arc::clone(&list_calls)), - fail_first_list: true, - })); - let (event_tx, event_rx) = std::sync::mpsc::channel(); - let mut builder = - ExtensionRegistryBuilder::with_event_sink(Arc::new(ChannelEventSink(event_tx))); - install_with_providers(&mut builder, providers, skills_extension_config); - let registry = builder.build(); - let session_store = ExtensionData::new("session"); - let thread_store = ExtensionData::new("thread"); - let session_source = SessionSource::Cli; - let config = default_config(); - registry.thread_lifecycle_contributors()[0] - .on_thread_start(ThreadStartInput { - config: &config, - session_source: &session_source, - persistent_thread_state_available: true, - environments: &[], - session_store: &session_store, - thread_store: &thread_store, - }) - .await; - - let initial_fragments = registry.context_contributors()[0] - .contribute_thread_context(&session_store, &thread_store) - .await; - assert!(initial_fragments.is_empty()); - let EventMsg::Warning(warning) = event_rx.try_recv()?.msg else { - panic!("expected warning event"); - }; - assert_eq!( - warning.message, - "orchestrator skills unavailable: temporary orchestrator failure" - ); - - for turn_id in ["turn-1", "turn-2"] { - let fragments = registry.turn_input_contributors()[0] - .contribute( - TurnInputContext { - turn_id: turn_id.to_string(), - user_input: vec![UserInput::Text { - text: "$first".to_string(), - text_elements: Vec::new(), - }], - environments: Vec::new(), - }, - &session_store, - &thread_store, - &ExtensionData::new(turn_id), - ) - .await; - assert!(fragments.is_empty()); - } - assert_eq!(1, list_calls.load(Ordering::Relaxed)); - - Ok(()) -} - -#[tokio::test] -async fn root_qualified_locator_selects_only_the_matching_executor_skill() -> TestResult { - let read_requests = Arc::new(Mutex::new(Vec::new())); - let root_a_locator = "skill://root-a/shared/lint-fix/SKILL.md"; - let root_b_locator = "skill://root-b/shared/lint-fix/SKILL.md"; - let executor_provider = Arc::new(StaticSkillProvider { - catalog: SkillCatalog { - entries: [("root-a", root_a_locator), ("root-b", root_b_locator)] - .into_iter() - .map(|(root_id, locator)| { - SkillCatalogEntry::new( - SkillPackageId(locator.to_string()), - SkillAuthority::new(SkillSourceKind::Executor, root_id), - "lint-fix", - "Fix lint errors.", - SkillResourceId::new(locator), - ) - .with_display_path(locator) - }) - .collect(), - warnings: Vec::new(), - }, - read_requests: Arc::clone(&read_requests), - list_calls: None, - fail_first_list: false, - }); - let providers = SkillProviders::new().with_executor_provider(executor_provider); - let mut builder = ExtensionRegistryBuilder::new(); - install_with_providers(&mut builder, providers, skills_extension_config); - let registry = builder.build(); - let session_store = ExtensionData::new("session"); - let thread_store = ExtensionData::new("thread"); - thread_store.insert( - [("root-a", "/skills/root-a"), ("root-b", "/skills/root-b")] - .into_iter() - .map(|(id, path)| SelectedCapabilityRoot { - id: id.to_string(), - location: CapabilityRootLocation::Environment { - environment_id: "env-1".to_string(), - path: PathUri::parse(&format!("file://{path}")).expect("skill root URI"), - }, - }) - .collect::>(), - ); - let session_source = SessionSource::Cli; - let config = default_config(); - registry.thread_lifecycle_contributors()[0] - .on_thread_start(ThreadStartInput { - config: &config, - session_source: &session_source, - persistent_thread_state_available: true, - environments: &[], - session_store: &session_store, - thread_store: &thread_store, - }) - .await; - - let fragments = registry.turn_input_contributors()[0] - .contribute( - TurnInputContext { - turn_id: "turn-1".to_string(), - user_input: vec![UserInput::Mention { - name: "lint-fix".to_string(), - path: root_b_locator.to_string(), - }], - environments: Vec::new(), - }, - &session_store, - &thread_store, - &ExtensionData::new("turn-1"), - ) - .await; - - assert_eq!(1, fragments.len()); - assert!(fragments[0].render().contains(root_b_locator)); - assert_eq!( - vec![( - SkillAuthority::new(SkillSourceKind::Executor, "root-b"), - SkillPackageId(root_b_locator.to_string()), - SkillResourceId::new(root_b_locator), - )], - read_request_keys(&read_requests) - ); - - Ok(()) -} - -#[tokio::test] -async fn prompt_hidden_skill_can_still_be_invoked() -> TestResult { - let read_requests = Arc::new(Mutex::new(Vec::new())); - let provider = Arc::new(StaticSkillProvider { - catalog: SkillCatalog { - entries: vec![ - test_entry( - SkillSourceKind::Host, - "host", - "host/visible-skill", - "visible-skill/SKILL.md", - ), - test_entry( - SkillSourceKind::Host, - "host", - "host/hidden-skill", - "hidden-skill/SKILL.md", - ) - .hidden_from_prompt(), - ], - warnings: Vec::new(), - }, - read_requests: Arc::clone(&read_requests), - list_calls: None, - fail_first_list: false, - }); - let providers = SkillProviders::new().with_host_provider(provider); - let mut builder = ExtensionRegistryBuilder::new(); - install_with_providers(&mut builder, providers, skills_extension_config); - let registry = builder.build(); - let session_store = ExtensionData::new("session"); - let thread_store = ExtensionData::new("thread"); - let session_source = SessionSource::Cli; - let config = default_config(); - registry.thread_lifecycle_contributors()[0] - .on_thread_start(ThreadStartInput { - config: &config, - session_source: &session_source, - persistent_thread_state_available: true, - environments: &[], - session_store: &session_store, - thread_store: &thread_store, - }) - .await; - - let fragments = registry.turn_input_contributors()[0] - .contribute( - TurnInputContext { - turn_id: "turn-1".to_string(), - user_input: vec![UserInput::Text { - text: "$hidden-skill".to_string(), - text_elements: Vec::new(), - }], - environments: Vec::new(), - }, - &session_store, - &thread_store, - &ExtensionData::new("turn-1"), - ) - .await; - - assert_eq!(2, fragments.len()); - assert!(fragments[0].render().contains("visible-skill")); - assert!(!fragments[0].render().contains("hidden-skill")); - assert!(fragments[1].render().contains("hidden-skill")); - assert_eq!( - vec![( - SkillAuthority::new(SkillSourceKind::Host, "host"), - SkillPackageId("host/hidden-skill".to_string()), - SkillResourceId::new("hidden-skill/SKILL.md"), - )], - read_request_keys(&read_requests) - ); - - Ok(()) -} - -#[derive(Clone)] -struct StaticSkillProvider { - catalog: SkillCatalog, - read_requests: Arc>>, - list_calls: Option>, - fail_first_list: bool, -} - -struct ChannelEventSink(std::sync::mpsc::Sender); - -impl ExtensionEventSink for ChannelEventSink { - fn emit(&self, event: Event) { - let _ = self.0.send(event); - } -} - -impl SkillProvider for StaticSkillProvider { - fn list(&self, _query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { - let list_call = self - .list_calls - .as_ref() - .map(|list_calls| list_calls.fetch_add(1, Ordering::Relaxed)); - let fail = self.fail_first_list && list_call == Some(0); - let catalog = self.catalog.clone(); - Box::pin(async move { - if fail { - Err(SkillProviderError::new("temporary orchestrator failure")) - } else { - Ok(catalog) - } - }) - } - - fn read(&self, request: SkillReadRequest) -> SkillProviderFuture<'_, SkillReadResult> { - let read_requests = Arc::clone(&self.read_requests); - Box::pin(async move { - read_requests - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .push(request.clone()); - Ok(SkillReadResult { - resource: request.resource, - contents: "# Lint Fix\n\nRun the formatter.".to_string(), - }) - }) - } - - fn search(&self, _request: SkillSearchRequest) -> SkillProviderFuture<'_, SkillSearchResult> { - Box::pin(async { Ok(SkillSearchResult::default()) }) - } -} - -fn test_entry( - kind: SkillSourceKind, - authority_id: &str, - package_id: &str, - main_prompt: &str, -) -> SkillCatalogEntry { - let name = package_id.rsplit('/').next().unwrap_or(package_id); - SkillCatalogEntry::new( - SkillPackageId(package_id.to_string()), - SkillAuthority::new(kind, authority_id), - name, - "Fix lint errors.", - SkillResourceId::new(main_prompt), - ) - .with_display_path(format!("skill://{package_id}/SKILL.md")) -} - -#[derive(Clone, Debug, Eq, PartialEq)] -struct TestConfig { - include_instructions: bool, - bundled_skills_enabled: bool, - orchestrator_skills_enabled: bool, -} - -fn default_config() -> TestConfig { - TestConfig { - include_instructions: true, - bundled_skills_enabled: true, - orchestrator_skills_enabled: true, - } -} - -fn skills_extension_config(config: &TestConfig) -> SkillsExtensionConfig { - SkillsExtensionConfig { - include_instructions: config.include_instructions, - bundled_skills_enabled: config.bundled_skills_enabled, - orchestrator_skills_enabled: config.orchestrator_skills_enabled, - } -} - -fn test_codex_home() -> PathBuf { - let id = NEXT_CODEX_HOME_ID.fetch_add(1, Ordering::Relaxed); - std::env::temp_dir().join(format!( - "codex-skills-extension-test-{}-{id}", - std::process::id(), - )) -} - -fn read_request_keys( - requests: &Arc>>, -) -> Vec<(SkillAuthority, SkillPackageId, SkillResourceId)> { - requests - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .iter() - .map(|request| { - ( - request.authority.clone(), - request.package.clone(), - request.resource.clone(), - ) - }) - .collect() -}