diff --git a/codex-rs/config/src/skills_config.rs b/codex-rs/config/src/skills_config.rs new file mode 100644 index 0000000000..3dbe65a245 --- /dev/null +++ b/codex-rs/config/src/skills_config.rs @@ -0,0 +1,53 @@ +//! Skill-related configuration types shared across crates. + +use codex_utils_absolute_path::AbsolutePathBuf; +use schemars::JsonSchema; +use serde::Deserialize; +use serde::Serialize; + +const fn default_enabled() -> bool { + true +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)] +#[schemars(deny_unknown_fields)] +pub struct SkillConfig { + /// Path-based selector. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub path: Option, + /// Name-based selector. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub name: Option, + pub enabled: bool, +} + +#[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, Eq, JsonSchema)] +#[schemars(deny_unknown_fields)] +pub struct SkillsConfig { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub bundled: Option, + + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub config: Vec, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)] +#[schemars(deny_unknown_fields)] +pub struct BundledSkillsConfig { + #[serde(default = "default_enabled")] + pub enabled: bool, +} + +impl Default for BundledSkillsConfig { + fn default() -> Self { + Self { enabled: true } + } +} + +impl TryFrom for SkillsConfig { + type Error = toml::de::Error; + + fn try_from(value: toml::Value) -> Result { + SkillsConfig::deserialize(value) + } +} diff --git a/codex-rs/core-skills/Cargo.toml b/codex-rs/core-skills/Cargo.toml new file mode 100644 index 0000000000..7a7fbc3573 --- /dev/null +++ b/codex-rs/core-skills/Cargo.toml @@ -0,0 +1,34 @@ +[package] +edition.workspace = true +license.workspace = true +name = "codex-core-skills" +version.workspace = true + +[lib] +doctest = false +name = "codex_core_skills" +path = "src/lib.rs" + +[lints] +workspace = true + +[dependencies] +anyhow = { workspace = true } +codex-app-server-protocol = { workspace = true } +codex-config = { workspace = true } +codex-otel = { workspace = true } +codex-protocol = { workspace = true } +codex-skills = { workspace = true } +codex-utils-absolute-path = { workspace = true } +dirs = { workspace = true } +dunce = { workspace = true } +reqwest = { workspace = true, features = ["json", "stream"] } +serde = { workspace = true, features = ["derive"] } +serde_json = { workspace = true } +tokio = { workspace = true, features = ["fs", "macros", "rt"] } +toml = { workspace = true } +tracing = { workspace = true } + +[dev-dependencies] +pretty_assertions = { workspace = true } +tempfile = { workspace = true } diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs new file mode 100644 index 0000000000..da1e94dc07 --- /dev/null +++ b/codex-rs/core-skills/src/lib.rs @@ -0,0 +1 @@ +pub mod skills; diff --git a/codex-rs/core-skills/src/skills/config_rules.rs b/codex-rs/core-skills/src/skills/config_rules.rs new file mode 100644 index 0000000000..121443d62e --- /dev/null +++ b/codex-rs/core-skills/src/skills/config_rules.rs @@ -0,0 +1,135 @@ +use std::collections::HashSet; +use std::path::Path; +use std::path::PathBuf; + +use codex_app_server_protocol::ConfigLayerSource; +use codex_config::ConfigLayerStack; +use codex_config::ConfigLayerStackOrdering; +use codex_config::SkillConfig; +use codex_config::SkillsConfig; +use tracing::warn; + +use crate::skills::SkillMetadata; + +#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub(crate) enum SkillConfigRuleSelector { + Name(String), + Path(PathBuf), +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct SkillConfigRule { + pub selector: SkillConfigRuleSelector, + pub enabled: bool, +} + +#[derive(Debug, Clone, Default, PartialEq, Eq, Hash)] +pub(crate) struct SkillConfigRules { + pub entries: Vec, +} + +pub(crate) fn skill_config_rules_from_stack( + config_layer_stack: &ConfigLayerStack, +) -> SkillConfigRules { + let mut entries = Vec::new(); + for layer in config_layer_stack.get_layers( + ConfigLayerStackOrdering::LowestPrecedenceFirst, + /*include_disabled*/ true, + ) { + if !matches!( + layer.name, + ConfigLayerSource::User { .. } | ConfigLayerSource::SessionFlags + ) { + continue; + } + + let Some(skills_value) = layer.config.get("skills") else { + continue; + }; + let skills: SkillsConfig = match skills_value.clone().try_into() { + Ok(skills) => skills, + Err(err) => { + warn!("invalid skills config: {err}"); + continue; + } + }; + + for entry in skills.config { + let Some(selector) = skill_config_rule_selector(&entry) else { + continue; + }; + // Preserve layer order so a later name selector can override an earlier path selector + // for the same loaded skill. + entries.retain(|entry: &SkillConfigRule| entry.selector != selector); + entries.push(SkillConfigRule { + selector, + enabled: entry.enabled, + }); + } + } + + SkillConfigRules { entries } +} + +pub(crate) fn resolve_disabled_skill_paths( + skills: &[SkillMetadata], + rules: &SkillConfigRules, +) -> HashSet { + let mut disabled_paths = HashSet::new(); + + for entry in &rules.entries { + match &entry.selector { + SkillConfigRuleSelector::Path(path) => { + if entry.enabled { + disabled_paths.remove(path); + } else { + disabled_paths.insert(path.clone()); + } + } + SkillConfigRuleSelector::Name(name) => { + for path in skills + .iter() + .filter(|skill| skill.name == *name) + .map(|skill| skill.path_to_skills_md.clone()) + { + if entry.enabled { + disabled_paths.remove(&path); + } else { + disabled_paths.insert(path); + } + } + } + } + } + + disabled_paths +} + +fn skill_config_rule_selector(entry: &SkillConfig) -> Option { + match (entry.path.as_ref(), entry.name.as_deref()) { + (Some(path), None) => Some(SkillConfigRuleSelector::Path(normalize_rule_path( + path.as_path(), + ))), + (None, Some(name)) => { + let name = name.trim(); + if name.is_empty() { + warn!("ignoring empty skills.config name override"); + None + } else { + Some(SkillConfigRuleSelector::Name(name.to_string())) + } + } + (Some(_), Some(_)) => { + warn!("ignoring skills.config entry with both path and name selectors"); + None + } + (None, None) => { + warn!("ignoring skills.config entry without a path or name selector"); + None + } + } +} + +fn normalize_rule_path(path: &Path) -> PathBuf { + dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()) +} diff --git a/codex-rs/core-skills/src/skills/env_var_dependencies.rs b/codex-rs/core-skills/src/skills/env_var_dependencies.rs new file mode 100644 index 0000000000..00f5bad8cc --- /dev/null +++ b/codex-rs/core-skills/src/skills/env_var_dependencies.rs @@ -0,0 +1,162 @@ +use std::collections::HashMap; +use std::collections::HashSet; +use std::env; +use std::sync::Arc; + +use codex_protocol::request_user_input::RequestUserInputArgs; +use codex_protocol::request_user_input::RequestUserInputQuestion; +use codex_protocol::request_user_input::RequestUserInputResponse; +use tracing::warn; + +use crate::codex::Session; +use crate::codex::TurnContext; +use crate::skills::SkillMetadata; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct SkillDependencyInfo { + pub(crate) skill_name: String, + pub(crate) name: String, + pub(crate) description: Option, +} + +/// Resolve required dependency values (session cache, then env vars), +/// and prompt the UI for any missing ones. +pub(crate) async fn resolve_skill_dependencies_for_turn( + sess: &Arc, + turn_context: &Arc, + dependencies: &[SkillDependencyInfo], +) { + if dependencies.is_empty() { + return; + } + + let existing_env = sess.dependency_env().await; + let mut loaded_values = HashMap::new(); + let mut missing = Vec::new(); + let mut seen_names = HashSet::new(); + + for dependency in dependencies { + let name = dependency.name.clone(); + if !seen_names.insert(name.clone()) { + continue; + } + if existing_env.contains_key(&name) { + continue; + } + match env::var(&name) { + Ok(value) => { + loaded_values.insert(name.clone(), value); + continue; + } + Err(env::VarError::NotPresent) => {} + Err(err) => { + warn!("failed to read env var {name}: {err}"); + } + } + missing.push(dependency.clone()); + } + + if !loaded_values.is_empty() { + sess.set_dependency_env(loaded_values).await; + } + + if !missing.is_empty() { + request_skill_dependencies(sess, turn_context, &missing).await; + } +} + +pub(crate) fn collect_env_var_dependencies( + mentioned_skills: &[SkillMetadata], +) -> Vec { + let mut dependencies = Vec::new(); + for skill in mentioned_skills { + let Some(skill_dependencies) = &skill.dependencies else { + continue; + }; + for tool in &skill_dependencies.tools { + if tool.r#type != "env_var" { + continue; + } + if tool.value.is_empty() { + continue; + } + dependencies.push(SkillDependencyInfo { + skill_name: skill.name.clone(), + name: tool.value.clone(), + description: tool.description.clone(), + }); + } + } + dependencies +} + +/// Prompt via request_user_input to gather missing env vars. +pub(crate) async fn request_skill_dependencies( + sess: &Arc, + turn_context: &Arc, + dependencies: &[SkillDependencyInfo], +) { + let questions = dependencies + .iter() + .map(|dep| { + let requirement = dep.description.as_ref().map_or_else( + || format!("The skill \"{}\" requires \"{}\" to be set.", dep.skill_name, dep.name), + |description| { + format!( + "The skill \"{}\" requires \"{}\" to be set ({}).", + dep.skill_name, dep.name, description + ) + }, + ); + let question = format!( + "{requirement} This is an experimental internal feature. The value is stored in memory for this session only.", + ); + RequestUserInputQuestion { + id: dep.name.clone(), + header: "Skill requires environment variable".to_string(), + question, + is_other: false, + is_secret: true, + options: None, + } + }) + .collect::>(); + + if questions.is_empty() { + return; + } + + let args = RequestUserInputArgs { questions }; + let call_id = format!("skill-deps-{}", turn_context.sub_id); + let response = sess + .request_user_input(turn_context, call_id, args) + .await + .unwrap_or_else(|| RequestUserInputResponse { + answers: HashMap::new(), + }); + + if response.answers.is_empty() { + return; + } + + let mut values = HashMap::new(); + for (name, answer) in response.answers { + let mut user_note = None; + for entry in &answer.answers { + if let Some(note) = entry.strip_prefix("user_note: ") + && !note.trim().is_empty() + { + user_note = Some(note.trim().to_string()); + } + } + if let Some(value) = user_note { + values.insert(name, value); + } + } + + if values.is_empty() { + return; + } + + sess.set_dependency_env(values).await; +} diff --git a/codex-rs/core-skills/src/skills/injection.rs b/codex-rs/core-skills/src/skills/injection.rs new file mode 100644 index 0000000000..b83be2322c --- /dev/null +++ b/codex-rs/core-skills/src/skills/injection.rs @@ -0,0 +1,493 @@ +use std::collections::HashMap; +use std::collections::HashSet; +use std::path::PathBuf; + +use crate::analytics_client::AnalyticsEventsClient; +use crate::analytics_client::InvocationType; +use crate::analytics_client::SkillInvocation; +use crate::analytics_client::TrackEventsContext; +use crate::instructions::SkillInstructions; +use crate::mention_syntax::TOOL_MENTION_SIGIL; +use crate::mentions::build_skill_name_counts; +use crate::skills::SkillMetadata; +use codex_otel::SessionTelemetry; +use codex_protocol::models::ResponseItem; +use codex_protocol::user_input::UserInput; +use tokio::fs; + +#[derive(Debug, Default)] +pub(crate) struct SkillInjections { + pub(crate) items: Vec, + pub(crate) warnings: Vec, +} + +pub(crate) async fn build_skill_injections( + mentioned_skills: &[SkillMetadata], + 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 { + match fs::read_to_string(&skill.path_to_skills_md).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.clone(), + invocation_type: InvocationType::Explicit, + }); + result.items.push(ResponseItem::from(SkillInstructions { + 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 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(crate) 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()); + 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.as_path() == path.as_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(crate) 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(crate) fn plain_names(&self) -> impl Iterator + '_ { + self.plain_names.iter().copied() + } + + pub(crate) fn paths(&self) -> impl Iterator + '_ { + self.paths.iter().copied() + } +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum ToolMentionKind { + App, + Mcp, + Plugin, + Skill, + Other, +} + +const APP_PATH_PREFIX: &str = "app://"; +const MCP_PATH_PREFIX: &str = "mcp://"; +const PLUGIN_PATH_PREFIX: &str = "plugin://"; +const SKILL_PATH_PREFIX: &str = "skill://"; +const SKILL_FILENAME: &str = "SKILL.md"; + +pub(crate) fn tool_kind_for_path(path: &str) -> ToolMentionKind { + if path.starts_with(APP_PATH_PREFIX) { + ToolMentionKind::App + } else if path.starts_with(MCP_PATH_PREFIX) { + ToolMentionKind::Mcp + } else if path.starts_with(PLUGIN_PATH_PREFIX) { + ToolMentionKind::Plugin + } else if path.starts_with(SKILL_PATH_PREFIX) || is_skill_filename(path) { + ToolMentionKind::Skill + } else { + ToolMentionKind::Other + } +} + +fn is_skill_filename(path: &str) -> bool { + let file_name = path.rsplit(['/', '\\']).next().unwrap_or(path); + file_name.eq_ignore_ascii_case(SKILL_FILENAME) +} + +pub(crate) fn app_id_from_path(path: &str) -> Option<&str> { + path.strip_prefix(APP_PATH_PREFIX) + .filter(|value| !value.is_empty()) +} + +pub(crate) fn plugin_config_name_from_path(path: &str) -> Option<&str> { + path.strip_prefix(PLUGIN_PATH_PREFIX) + .filter(|value| !value.is_empty()) +} + +pub(crate) fn normalize_skill_path(path: &str) -> &str { + path.strip_prefix(SKILL_PATH_PREFIX).unwrap_or(path) +} + +/// Extract `$tool-name` mentions from a single text input. +/// +/// Supports explicit resource links in the form `[$tool-name](resource path)`. When a +/// resource path is present, it is captured for exact path matching while also tracking +/// the name for fallback matching. +pub(crate) fn extract_tool_mentions(text: &str) -> ToolMentions<'_> { + extract_tool_mentions_with_sigil(text, TOOL_MENTION_SIGIL) +} + +pub(crate) fn extract_tool_mentions_with_sigil(text: &str, sigil: char) -> ToolMentions<'_> { + let text_bytes = text.as_bytes(); + let mut mentioned_names: HashSet<&str> = HashSet::new(); + let mut mentioned_paths: HashSet<&str> = HashSet::new(); + let mut plain_names: HashSet<&str> = HashSet::new(); + + let mut index = 0; + while index < text_bytes.len() { + let byte = text_bytes[index]; + if byte == b'[' + && let Some((name, path, end_index)) = + parse_linked_tool_mention(text, text_bytes, index, sigil) + { + if !is_common_env_var(name) { + if !matches!( + tool_kind_for_path(path), + ToolMentionKind::App | ToolMentionKind::Mcp | ToolMentionKind::Plugin + ) { + mentioned_names.insert(name); + } + mentioned_paths.insert(path); + } + index = end_index; + continue; + } + + if byte != sigil as u8 { + index += 1; + continue; + } + + let name_start = index + 1; + let Some(first_name_byte) = text_bytes.get(name_start) else { + index += 1; + continue; + }; + if !is_mention_name_char(*first_name_byte) { + index += 1; + continue; + } + + let mut name_end = name_start + 1; + while let Some(next_byte) = text_bytes.get(name_end) + && is_mention_name_char(*next_byte) + { + name_end += 1; + } + + let name = &text[name_start..name_end]; + if !is_common_env_var(name) { + mentioned_names.insert(name); + plain_names.insert(name); + } + index = name_end; + } + + ToolMentions { + names: mentioned_names, + paths: mentioned_paths, + plain_names, + } +} + +/// Select mentioned skills while preserving the order of `skills`. +fn select_skills_from_mentions( + selection_context: &SkillSelectionContext<'_>, + 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], + start: usize, + sigil: char, +) -> Option<(&'a str, &'a str, usize)> { + let sigil_index = start + 1; + if text_bytes.get(sigil_index) != Some(&(sigil as u8)) { + return None; + } + + let name_start = sigil_index + 1; + let first_name_byte = text_bytes.get(name_start)?; + if !is_mention_name_char(*first_name_byte) { + return None; + } + + let mut name_end = name_start + 1; + while let Some(next_byte) = text_bytes.get(name_end) + && is_mention_name_char(*next_byte) + { + name_end += 1; + } + + if text_bytes.get(name_end) != Some(&b']') { + return None; + } + + let mut path_start = name_end + 1; + while let Some(next_byte) = text_bytes.get(path_start) + && next_byte.is_ascii_whitespace() + { + path_start += 1; + } + if text_bytes.get(path_start) != Some(&b'(') { + return None; + } + + let mut path_end = path_start + 1; + while let Some(next_byte) = text_bytes.get(path_end) + && *next_byte != b')' + { + path_end += 1; + } + if text_bytes.get(path_end) != Some(&b')') { + return None; + } + + let path = text[path_start + 1..path_end].trim(); + if path.is_empty() { + return None; + } + + let name = &text[name_start..name_end]; + Some((name, path, path_end + 1)) +} + +fn is_common_env_var(name: &str) -> bool { + let upper = name.to_ascii_uppercase(); + matches!( + upper.as_str(), + "PATH" + | "HOME" + | "USER" + | "SHELL" + | "PWD" + | "TMPDIR" + | "TEMP" + | "TMP" + | "LANG" + | "TERM" + | "XDG_CONFIG_HOME" + ) +} + +#[cfg(test)] +fn text_mentions_skill(text: &str, skill_name: &str) -> bool { + if skill_name.is_empty() { + return false; + } + + let text_bytes = text.as_bytes(); + let skill_bytes = skill_name.as_bytes(); + + for (index, byte) in text_bytes.iter().copied().enumerate() { + if byte != b'$' { + continue; + } + + let name_start = index + 1; + let Some(rest) = text_bytes.get(name_start..) else { + continue; + }; + if !rest.starts_with(skill_bytes) { + continue; + } + + let after_index = name_start + skill_bytes.len(); + let after = text_bytes.get(after_index).copied(); + if after.is_none_or(|b| !is_mention_name_char(b)) { + return true; + } + } + + false +} + +fn is_mention_name_char(byte: u8) -> bool { + matches!(byte, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'_' | b'-' | b':') +} + +#[cfg(test)] +#[path = "injection_tests.rs"] +mod tests; diff --git a/codex-rs/core-skills/src/skills/injection_tests.rs b/codex-rs/core-skills/src/skills/injection_tests.rs new file mode 100644 index 0000000000..8d66a0af57 --- /dev/null +++ b/codex-rs/core-skills/src/skills/injection_tests.rs @@ -0,0 +1,348 @@ +use super::*; +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, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: PathBuf::from(path), + scope: codex_protocol::protocol::SkillScope::User, + } +} + +fn set<'a>(items: &'a [&'a str]) -> HashSet<&'a str> { + items.iter().copied().collect() +} + +fn assert_mentions(text: &str, expected_names: &[&str], expected_paths: &[&str]) { + let mentions = extract_tool_mentions(text); + assert_eq!(mentions.names, set(expected_names)); + assert_eq!(mentions.paths, set(expected_paths)); +} + +fn 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"], + &["/tmp/beta"], + ); +} + +#[test] +fn extract_tool_mentions_skips_common_env_vars() { + assert_mentions("use $PATH and $alpha", &["alpha"], &[]); + assert_mentions("use [$HOME](/tmp/skill)", &[], &[]); + assert_mentions("use $XDG_CONFIG_HOME and $beta", &["beta"], &[]); +} + +#[test] +fn extract_tool_mentions_requires_link_syntax() { + assert_mentions("[beta](/tmp/beta)", &[], &[]); + assert_mentions("[$beta] /tmp/beta", &["beta"], &[]); + assert_mentions("[$beta]()", &["beta"], &[]); +} + +#[test] +fn extract_tool_mentions_trims_linked_paths_and_allows_spacing() { + assert_mentions("use [$beta] ( /tmp/beta )", &["beta"], &["/tmp/beta"]); +} + +#[test] +fn extract_tool_mentions_stops_at_non_name_chars() { + assert_mentions( + "use $alpha.skill and $beta_extra", + &["alpha", "beta_extra"], + &[], + ); +} + +#[test] +fn extract_tool_mentions_keeps_plugin_skill_namespaces() { + assert_mentions( + "use $slack:search and $alpha", + &["alpha", "slack:search"], + &[], + ); +} + +#[test] +fn collect_explicit_skill_mentions_text_respects_skill_order() { + let alpha = make_skill("alpha-skill", "/tmp/alpha"); + 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: PathBuf::from("/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: PathBuf::from("/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: PathBuf::from("/tmp/alpha"), + }, + ]; + let disabled = HashSet::from([PathBuf::from("/tmp/alpha")]); + 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 inputs = vec![UserInput::Text { + text: "use [$alpha-skill](/tmp/alpha) and [$alpha-skill](/tmp/alpha)".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![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: "use $demo-skill and [$demo-skill](/tmp/beta)".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![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: "use [$alpha-skill](/tmp/alpha)".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![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: "use [$demo-skill](/tmp/alpha)".to_string(), + text_elements: Vec::new(), + }]; + let disabled = HashSet::from([PathBuf::from("/tmp/alpha")]); + 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: "use [$demo-skill](/tmp/beta)".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![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: "use [$demo-skill](/tmp/missing)".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_skips_missing_path_without_fallback() { + let alpha = make_skill("demo-skill", "/tmp/alpha"); + let skills = vec![alpha]; + let inputs = vec![UserInput::Text { + text: "use [$demo-skill](/tmp/missing)".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()); +} diff --git a/codex-rs/core-skills/src/skills/invocation_utils.rs b/codex-rs/core-skills/src/skills/invocation_utils.rs new file mode 100644 index 0000000000..122158bda7 --- /dev/null +++ b/codex-rs/core-skills/src/skills/invocation_utils.rs @@ -0,0 +1,235 @@ +use std::collections::HashMap; +use std::path::Path; +use std::path::PathBuf; + +use crate::analytics_client::InvocationType; +use crate::analytics_client::SkillInvocation; +use crate::analytics_client::build_track_events_context; +use crate::codex::Session; +use crate::codex::TurnContext; +use crate::skills::SkillLoadOutcome; +use crate::skills::SkillMetadata; + +pub(crate) fn build_implicit_skill_path_indexes( + skills: Vec, +) -> ( + HashMap, + HashMap, +) { + let mut by_scripts_dir = HashMap::new(); + let mut by_skill_doc_path = HashMap::new(); + for skill in skills { + let skill_doc_path = normalize_path(skill.path_to_skills_md.as_path()); + by_skill_doc_path.insert(skill_doc_path, skill.clone()); + + if let Some(skill_dir) = skill.path_to_skills_md.parent() { + let scripts_dir = normalize_path(&skill_dir.join("scripts")); + by_scripts_dir.insert(scripts_dir, skill); + } + } + + (by_scripts_dir, by_skill_doc_path) +} + +fn detect_implicit_skill_invocation_for_command( + outcome: &SkillLoadOutcome, + turn_context: &TurnContext, + command: &str, + workdir: Option<&str>, +) -> Option { + let workdir = turn_context.resolve_path(workdir.map(str::to_owned)); + let workdir = normalize_path(workdir.as_path()); + let tokens = tokenize_command(command); + + if let Some(candidate) = detect_skill_script_run(outcome, tokens.as_slice(), workdir.as_path()) + { + return Some(candidate); + } + + if let Some(candidate) = detect_skill_doc_read(outcome, tokens.as_slice(), workdir.as_path()) { + return Some(candidate); + } + + None +} + +pub(crate) async fn maybe_emit_implicit_skill_invocation( + sess: &Session, + turn_context: &TurnContext, + command: &str, + workdir: Option<&str>, +) { + let Some(candidate) = detect_implicit_skill_invocation_for_command( + &turn_context.turn_skills.outcome, + turn_context, + command, + workdir, + ) else { + return; + }; + let invocation = SkillInvocation { + skill_name: candidate.name, + skill_scope: candidate.scope, + skill_path: candidate.path_to_skills_md, + invocation_type: InvocationType::Implicit, + }; + let skill_scope = match invocation.skill_scope { + codex_protocol::protocol::SkillScope::User => "user", + codex_protocol::protocol::SkillScope::Repo => "repo", + codex_protocol::protocol::SkillScope::System => "system", + codex_protocol::protocol::SkillScope::Admin => "admin", + }; + let skill_path = invocation.skill_path.to_string_lossy(); + let skill_name = invocation.skill_name.clone(); + let seen_key = format!("{skill_scope}:{skill_path}:{skill_name}"); + let inserted = { + let mut seen_skills = turn_context + .turn_skills + .implicit_invocation_seen_skills + .lock() + .await; + seen_skills.insert(seen_key) + }; + if !inserted { + return; + } + + turn_context.session_telemetry.counter( + "codex.skill.injected", + /*inc*/ 1, + &[ + ("status", "ok"), + ("skill", skill_name.as_str()), + ("invoke_type", "implicit"), + ], + ); + sess.services + .analytics_events_client + .track_skill_invocations( + build_track_events_context( + turn_context.model_info.slug.clone(), + sess.conversation_id.to_string(), + turn_context.sub_id.clone(), + ), + vec![invocation], + ); +} + +fn tokenize_command(command: &str) -> Vec { + shlex::split(command).unwrap_or_else(|| { + command + .split_whitespace() + .map(std::string::ToString::to_string) + .collect() + }) +} + +fn script_run_token(tokens: &[String]) -> Option<&str> { + const RUNNERS: [&str; 10] = [ + "python", "python3", "bash", "zsh", "sh", "node", "deno", "ruby", "perl", "pwsh", + ]; + const SCRIPT_EXTENSIONS: [&str; 7] = [".py", ".sh", ".js", ".ts", ".rb", ".pl", ".ps1"]; + + let runner_token = tokens.first()?; + let runner = command_basename(runner_token).to_ascii_lowercase(); + let runner = runner.strip_suffix(".exe").unwrap_or(&runner); + if !RUNNERS.contains(&runner) { + return None; + } + + let mut script_token: Option<&str> = None; + for token in tokens.iter().skip(1) { + if token == "--" { + continue; + } + if token.starts_with('-') { + continue; + } + script_token = Some(token.as_str()); + break; + } + let script_token = script_token?; + if SCRIPT_EXTENSIONS + .iter() + .any(|extension| script_token.to_ascii_lowercase().ends_with(extension)) + { + return Some(script_token); + } + + None +} + +fn detect_skill_script_run( + outcome: &SkillLoadOutcome, + tokens: &[String], + workdir: &Path, +) -> Option { + let script_token = script_run_token(tokens)?; + let script_path = Path::new(script_token); + let script_path = if script_path.is_absolute() { + script_path.to_path_buf() + } else { + workdir.join(script_path) + }; + let script_path = normalize_path(script_path.as_path()); + + for ancestor in script_path.ancestors() { + if let Some(candidate) = outcome.implicit_skills_by_scripts_dir.get(ancestor) { + return Some(candidate.clone()); + } + } + + None +} + +fn detect_skill_doc_read( + outcome: &SkillLoadOutcome, + tokens: &[String], + workdir: &Path, +) -> Option { + if !command_reads_file(tokens) { + return None; + } + + for token in tokens.iter().skip(1) { + if token.starts_with('-') { + continue; + } + let path = Path::new(token); + let candidate_path = if path.is_absolute() { + normalize_path(path) + } else { + normalize_path(&workdir.join(path)) + }; + if let Some(candidate) = outcome.implicit_skills_by_doc_path.get(&candidate_path) { + return Some(candidate.clone()); + } + } + + None +} + +fn command_reads_file(tokens: &[String]) -> bool { + const READERS: [&str; 8] = ["cat", "sed", "head", "tail", "less", "more", "bat", "awk"]; + let Some(program) = tokens.first() else { + return false; + }; + let program = command_basename(program).to_ascii_lowercase(); + READERS.contains(&program.as_str()) +} + +fn command_basename(command: &str) -> String { + Path::new(command) + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or(command) + .to_string() +} + +fn normalize_path(path: &Path) -> PathBuf { + std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()) +} + +#[cfg(test)] +#[path = "invocation_utils_tests.rs"] +mod tests; diff --git a/codex-rs/core-skills/src/skills/invocation_utils_tests.rs b/codex-rs/core-skills/src/skills/invocation_utils_tests.rs new file mode 100644 index 0000000000..657582b742 --- /dev/null +++ b/codex-rs/core-skills/src/skills/invocation_utils_tests.rs @@ -0,0 +1,119 @@ +use super::SkillLoadOutcome; +use super::SkillMetadata; +use super::detect_skill_doc_read; +use super::detect_skill_script_run; +use super::normalize_path; +use super::script_run_token; +use pretty_assertions::assert_eq; +use std::collections::HashMap; +use std::path::Path; +use std::path::PathBuf; +use std::sync::Arc; + +fn test_skill_metadata(skill_doc_path: PathBuf) -> SkillMetadata { + SkillMetadata { + name: "test-skill".to_string(), + description: "test".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: skill_doc_path, + scope: codex_protocol::protocol::SkillScope::User, + } +} + +#[test] +fn script_run_detection_matches_runner_plus_extension() { + let tokens = vec![ + "python3".to_string(), + "-u".to_string(), + "scripts/fetch_comments.py".to_string(), + ]; + + assert_eq!(script_run_token(&tokens).is_some(), true); +} + +#[test] +fn script_run_detection_excludes_python_c() { + let tokens = vec![ + "python3".to_string(), + "-c".to_string(), + "print(1)".to_string(), + ]; + + assert_eq!(script_run_token(&tokens).is_some(), false); +} + +#[test] +fn skill_doc_read_detection_matches_absolute_path() { + let skill_doc_path = PathBuf::from("/tmp/skill-test/SKILL.md"); + let normalized_skill_doc_path = normalize_path(skill_doc_path.as_path()); + let skill = test_skill_metadata(skill_doc_path); + let outcome = SkillLoadOutcome { + implicit_skills_by_scripts_dir: Arc::new(HashMap::new()), + implicit_skills_by_doc_path: Arc::new(HashMap::from([(normalized_skill_doc_path, skill)])), + ..Default::default() + }; + + let tokens = vec![ + "cat".to_string(), + "/tmp/skill-test/SKILL.md".to_string(), + "|".to_string(), + "head".to_string(), + ]; + let found = detect_skill_doc_read(&outcome, &tokens, Path::new("/tmp")); + + assert_eq!( + found.map(|value| value.name), + Some("test-skill".to_string()) + ); +} + +#[test] +fn skill_script_run_detection_matches_relative_path_from_skill_root() { + let skill_doc_path = PathBuf::from("/tmp/skill-test/SKILL.md"); + let scripts_dir = normalize_path(Path::new("/tmp/skill-test/scripts")); + let skill = test_skill_metadata(skill_doc_path); + let outcome = SkillLoadOutcome { + implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])), + implicit_skills_by_doc_path: Arc::new(HashMap::new()), + ..Default::default() + }; + let tokens = vec![ + "python3".to_string(), + "scripts/fetch_comments.py".to_string(), + ]; + + let found = detect_skill_script_run(&outcome, &tokens, Path::new("/tmp/skill-test")); + + assert_eq!( + found.map(|value| value.name), + Some("test-skill".to_string()) + ); +} + +#[test] +fn skill_script_run_detection_matches_absolute_path_from_any_workdir() { + let skill_doc_path = PathBuf::from("/tmp/skill-test/SKILL.md"); + let scripts_dir = normalize_path(Path::new("/tmp/skill-test/scripts")); + let skill = test_skill_metadata(skill_doc_path); + let outcome = SkillLoadOutcome { + implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])), + implicit_skills_by_doc_path: Arc::new(HashMap::new()), + ..Default::default() + }; + let tokens = vec![ + "python3".to_string(), + "/tmp/skill-test/scripts/fetch_comments.py".to_string(), + ]; + + let found = detect_skill_script_run(&outcome, &tokens, Path::new("/tmp/other")); + + assert_eq!( + found.map(|value| value.name), + Some("test-skill".to_string()) + ); +} diff --git a/codex-rs/core-skills/src/skills/loader.rs b/codex-rs/core-skills/src/skills/loader.rs new file mode 100644 index 0000000000..726efb0db4 --- /dev/null +++ b/codex-rs/core-skills/src/skills/loader.rs @@ -0,0 +1,926 @@ +use crate::config_loader::default_project_root_markers; +use crate::config_loader::merge_toml_values; +use crate::config_loader::project_root_markers_from_config; +use crate::plugins::plugin_namespace_for_skill_path; +use crate::skills::model::SkillDependencies; +use crate::skills::model::SkillError; +use crate::skills::model::SkillInterface; +use crate::skills::model::SkillLoadOutcome; +use crate::skills::model::SkillManagedNetworkOverride; +use crate::skills::model::SkillMetadata; +use crate::skills::model::SkillPolicy; +use crate::skills::model::SkillToolDependency; +use crate::skills::system::system_cache_root_dir; +use codex_app_server_protocol::ConfigLayerSource; +use codex_config::ConfigLayerStack; +use codex_config::ConfigLayerStackOrdering; +use codex_protocol::models::FileSystemPermissions; +use codex_protocol::models::MacOsSeatbeltProfileExtensions; +use codex_protocol::models::NetworkPermissions; +use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::Product; +use codex_protocol::protocol::SkillScope; +use codex_utils_absolute_path::AbsolutePathBufGuard; +use dirs::home_dir; +use dunce::canonicalize as canonicalize_path; +use serde::Deserialize; +use std::collections::HashSet; +use std::collections::VecDeque; +use std::error::Error; +use std::fmt; +use std::fs; +use std::path::Component; +use std::path::Path; +use std::path::PathBuf; +use toml::Value as TomlValue; +use tracing::error; + +#[cfg(test)] +use crate::config::Config; + +#[derive(Debug, Deserialize)] +struct SkillFrontmatter { + #[serde(default)] + name: Option, + #[serde(default)] + description: Option, + #[serde(default)] + metadata: SkillFrontmatterMetadata, +} + +#[derive(Debug, Default, Deserialize)] +struct SkillFrontmatterMetadata { + #[serde(default, rename = "short-description")] + short_description: Option, +} + +#[derive(Debug, Default, Deserialize)] +struct SkillMetadataFile { + #[serde(default)] + interface: Option, + #[serde(default)] + dependencies: Option, + #[serde(default)] + policy: Option, + #[serde(default)] + permissions: Option, +} + +#[derive(Default)] +struct LoadedSkillMetadata { + interface: Option, + dependencies: Option, + policy: Option, + permission_profile: Option, + managed_network_override: Option, +} + +#[derive(Debug, Default, Deserialize, PartialEq, Eq)] +struct SkillPermissionProfile { + #[serde(default)] + network: Option, + #[serde(default)] + file_system: Option, + #[serde(default)] + macos: Option, +} + +#[derive(Debug, Default, Deserialize, PartialEq, Eq)] +struct SkillNetworkPermissions { + #[serde(default)] + enabled: Option, + #[serde(default)] + allowed_domains: Option>, + #[serde(default)] + denied_domains: Option>, +} + +#[derive(Debug, Default, Deserialize)] +struct Interface { + display_name: Option, + short_description: Option, + icon_small: Option, + icon_large: Option, + brand_color: Option, + default_prompt: Option, +} + +#[derive(Debug, Default, Deserialize)] +struct Dependencies { + #[serde(default)] + tools: Vec, +} + +#[derive(Debug, Deserialize)] +struct Policy { + #[serde(default)] + allow_implicit_invocation: Option, + #[serde(default)] + products: Vec, +} + +#[derive(Debug, Default, Deserialize)] +struct DependencyTool { + #[serde(rename = "type")] + kind: Option, + value: Option, + description: Option, + transport: Option, + command: Option, + url: Option, +} + +const SKILLS_FILENAME: &str = "SKILL.md"; +const AGENTS_DIR_NAME: &str = ".agents"; +const SKILLS_METADATA_DIR: &str = "agents"; +const SKILLS_METADATA_FILENAME: &str = "openai.yaml"; +const SKILLS_DIR_NAME: &str = "skills"; +const MAX_NAME_LEN: usize = 64; +const MAX_DESCRIPTION_LEN: usize = 1024; +const MAX_SHORT_DESCRIPTION_LEN: usize = MAX_DESCRIPTION_LEN; +const MAX_DEFAULT_PROMPT_LEN: usize = MAX_DESCRIPTION_LEN; +const MAX_DEPENDENCY_TYPE_LEN: usize = MAX_NAME_LEN; +const MAX_DEPENDENCY_TRANSPORT_LEN: usize = MAX_NAME_LEN; +const MAX_DEPENDENCY_VALUE_LEN: usize = MAX_DESCRIPTION_LEN; +const MAX_DEPENDENCY_DESCRIPTION_LEN: usize = MAX_DESCRIPTION_LEN; +const MAX_DEPENDENCY_COMMAND_LEN: usize = MAX_DESCRIPTION_LEN; +const MAX_DEPENDENCY_URL_LEN: usize = MAX_DESCRIPTION_LEN; +// Traversal depth from the skills root. +const MAX_SCAN_DEPTH: usize = 6; +const MAX_SKILLS_DIRS_PER_ROOT: usize = 2000; + +#[derive(Debug)] +enum SkillParseError { + Read(std::io::Error), + MissingFrontmatter, + InvalidYaml(serde_yaml::Error), + MissingField(&'static str), + InvalidField { field: &'static str, reason: String }, +} + +impl fmt::Display for SkillParseError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + SkillParseError::Read(e) => write!(f, "failed to read file: {e}"), + SkillParseError::MissingFrontmatter => { + write!(f, "missing YAML frontmatter delimited by ---") + } + SkillParseError::InvalidYaml(e) => write!(f, "invalid YAML: {e}"), + SkillParseError::MissingField(field) => write!(f, "missing field `{field}`"), + SkillParseError::InvalidField { field, reason } => { + write!(f, "invalid {field}: {reason}") + } + } + } +} + +impl Error for SkillParseError {} + +pub(crate) struct SkillRoot { + pub(crate) path: PathBuf, + pub(crate) scope: SkillScope, +} + +pub(crate) fn load_skills_from_roots(roots: I) -> SkillLoadOutcome +where + I: IntoIterator, +{ + let mut outcome = SkillLoadOutcome::default(); + for root in roots { + discover_skills_under_root(&root.path, root.scope, &mut outcome); + } + + let mut seen: HashSet = HashSet::new(); + outcome + .skills + .retain(|skill| seen.insert(skill.path_to_skills_md.clone())); + + fn scope_rank(scope: SkillScope) -> u8 { + // Higher-priority scopes first (matches root scan order for dedupe). + match scope { + SkillScope::Repo => 0, + SkillScope::User => 1, + SkillScope::System => 2, + SkillScope::Admin => 3, + } + } + + outcome.skills.sort_by(|a, b| { + scope_rank(a.scope) + .cmp(&scope_rank(b.scope)) + .then_with(|| a.name.cmp(&b.name)) + .then_with(|| a.path_to_skills_md.cmp(&b.path_to_skills_md)) + }); + + outcome +} + +pub(crate) fn skill_roots( + config_layer_stack: &ConfigLayerStack, + cwd: &Path, + plugin_skill_roots: Vec, +) -> Vec { + skill_roots_with_home_dir( + config_layer_stack, + cwd, + home_dir().as_deref(), + plugin_skill_roots, + ) +} + +fn skill_roots_with_home_dir( + config_layer_stack: &ConfigLayerStack, + cwd: &Path, + home_dir: Option<&Path>, + plugin_skill_roots: Vec, +) -> Vec { + let mut roots = skill_roots_from_layer_stack_inner(config_layer_stack, home_dir); + roots.extend(plugin_skill_roots.into_iter().map(|path| SkillRoot { + path, + scope: SkillScope::User, + })); + roots.extend(repo_agents_skill_roots(config_layer_stack, cwd)); + dedupe_skill_roots_by_path(&mut roots); + roots +} + +fn skill_roots_from_layer_stack_inner( + config_layer_stack: &ConfigLayerStack, + home_dir: Option<&Path>, +) -> Vec { + let mut roots = Vec::new(); + + for layer in config_layer_stack.get_layers( + ConfigLayerStackOrdering::HighestPrecedenceFirst, + /*include_disabled*/ true, + ) { + let Some(config_folder) = layer.config_folder() else { + continue; + }; + + match &layer.name { + ConfigLayerSource::Project { .. } => { + roots.push(SkillRoot { + path: config_folder.as_path().join(SKILLS_DIR_NAME), + scope: SkillScope::Repo, + }); + } + ConfigLayerSource::User { .. } => { + // Deprecated user skills location (`$CODEX_HOME/skills`), kept for backward + // compatibility. + roots.push(SkillRoot { + path: config_folder.as_path().join(SKILLS_DIR_NAME), + scope: SkillScope::User, + }); + + // `$HOME/.agents/skills` (user-installed skills). + if let Some(home_dir) = home_dir { + roots.push(SkillRoot { + path: home_dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME), + scope: SkillScope::User, + }); + } + + // Embedded system skills are cached under `$CODEX_HOME/skills/.system` and are a + // special case (not a config layer). + roots.push(SkillRoot { + path: system_cache_root_dir(config_folder.as_path()), + scope: SkillScope::System, + }); + } + ConfigLayerSource::System { .. } => { + // The system config layer lives under `/etc/codex/` on Unix, so treat + // `/etc/codex/skills` as admin-scoped skills. + roots.push(SkillRoot { + path: config_folder.as_path().join(SKILLS_DIR_NAME), + scope: SkillScope::Admin, + }); + } + ConfigLayerSource::Mdm { .. } + | ConfigLayerSource::SessionFlags + | ConfigLayerSource::LegacyManagedConfigTomlFromFile { .. } + | ConfigLayerSource::LegacyManagedConfigTomlFromMdm => {} + } + } + + roots +} + +fn repo_agents_skill_roots(config_layer_stack: &ConfigLayerStack, cwd: &Path) -> Vec { + let project_root_markers = project_root_markers_from_stack(config_layer_stack); + let project_root = find_project_root(cwd, &project_root_markers); + let dirs = dirs_between_project_root_and_cwd(cwd, &project_root); + let mut roots = Vec::new(); + for dir in dirs { + let agents_skills = dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME); + if agents_skills.is_dir() { + roots.push(SkillRoot { + path: agents_skills, + scope: SkillScope::Repo, + }); + } + } + roots +} + +fn project_root_markers_from_stack(config_layer_stack: &ConfigLayerStack) -> Vec { + let mut merged = TomlValue::Table(toml::map::Map::new()); + for layer in config_layer_stack.get_layers( + ConfigLayerStackOrdering::LowestPrecedenceFirst, + /*include_disabled*/ false, + ) { + if matches!(layer.name, ConfigLayerSource::Project { .. }) { + continue; + } + merge_toml_values(&mut merged, &layer.config); + } + + match project_root_markers_from_config(&merged) { + Ok(Some(markers)) => markers, + Ok(None) => default_project_root_markers(), + Err(err) => { + tracing::warn!("invalid project_root_markers: {err}"); + default_project_root_markers() + } + } +} + +fn find_project_root(cwd: &Path, project_root_markers: &[String]) -> PathBuf { + if project_root_markers.is_empty() { + return cwd.to_path_buf(); + } + + for ancestor in cwd.ancestors() { + for marker in project_root_markers { + let marker_path = ancestor.join(marker); + if marker_path.exists() { + return ancestor.to_path_buf(); + } + } + } + + cwd.to_path_buf() +} + +fn dirs_between_project_root_and_cwd(cwd: &Path, project_root: &Path) -> Vec { + let mut dirs = cwd + .ancestors() + .scan(false, |done, a| { + if *done { + None + } else { + if a == project_root { + *done = true; + } + Some(a.to_path_buf()) + } + }) + .collect::>(); + dirs.reverse(); + dirs +} + +fn dedupe_skill_roots_by_path(roots: &mut Vec) { + let mut seen: HashSet = HashSet::new(); + roots.retain(|root| seen.insert(root.path.clone())); +} + +fn discover_skills_under_root(root: &Path, scope: SkillScope, outcome: &mut SkillLoadOutcome) { + let Ok(root) = canonicalize_path(root) else { + return; + }; + + if !root.is_dir() { + return; + } + + fn enqueue_dir( + queue: &mut VecDeque<(PathBuf, usize)>, + visited_dirs: &mut HashSet, + truncated_by_dir_limit: &mut bool, + path: PathBuf, + depth: usize, + ) { + if depth > MAX_SCAN_DEPTH { + return; + } + if visited_dirs.len() >= MAX_SKILLS_DIRS_PER_ROOT { + *truncated_by_dir_limit = true; + return; + } + if visited_dirs.insert(path.clone()) { + queue.push_back((path, depth)); + } + } + + // Follow symlinked directories for user, admin, and repo skills. System skills are written by Codex itself. + let follow_symlinks = matches!( + scope, + SkillScope::Repo | SkillScope::User | SkillScope::Admin + ); + + let mut visited_dirs: HashSet = HashSet::new(); + visited_dirs.insert(root.clone()); + + let mut queue: VecDeque<(PathBuf, usize)> = VecDeque::from([(root.clone(), 0)]); + let mut truncated_by_dir_limit = false; + + while let Some((dir, depth)) = queue.pop_front() { + let entries = match fs::read_dir(&dir) { + Ok(entries) => entries, + Err(e) => { + error!("failed to read skills dir {}: {e:#}", dir.display()); + continue; + } + }; + + for entry in entries.flatten() { + let path = entry.path(); + let file_name = match path.file_name().and_then(|f| f.to_str()) { + Some(name) => name, + None => continue, + }; + + if file_name.starts_with('.') { + continue; + } + + let Ok(file_type) = entry.file_type() else { + continue; + }; + + if file_type.is_symlink() { + if !follow_symlinks { + continue; + } + + // Follow the symlink to determine what it points to. + let metadata = match fs::metadata(&path) { + Ok(metadata) => metadata, + Err(e) => { + error!( + "failed to stat skills entry {} (symlink): {e:#}", + path.display() + ); + continue; + } + }; + + if metadata.is_dir() { + let Ok(resolved_dir) = canonicalize_path(&path) else { + continue; + }; + enqueue_dir( + &mut queue, + &mut visited_dirs, + &mut truncated_by_dir_limit, + resolved_dir, + depth + 1, + ); + continue; + } + + continue; + } + + if file_type.is_dir() { + let Ok(resolved_dir) = canonicalize_path(&path) else { + continue; + }; + enqueue_dir( + &mut queue, + &mut visited_dirs, + &mut truncated_by_dir_limit, + resolved_dir, + depth + 1, + ); + continue; + } + + if file_type.is_file() && file_name == SKILLS_FILENAME { + match parse_skill_file(&path, scope) { + Ok(skill) => { + outcome.skills.push(skill); + } + Err(err) => { + if scope != SkillScope::System { + outcome.errors.push(SkillError { + path, + message: err.to_string(), + }); + } + } + } + } + } + } + + if truncated_by_dir_limit { + tracing::warn!( + "skills scan truncated after {} directories (root: {})", + MAX_SKILLS_DIRS_PER_ROOT, + root.display() + ); + } +} + +fn parse_skill_file(path: &Path, scope: SkillScope) -> Result { + let contents = fs::read_to_string(path).map_err(SkillParseError::Read)?; + + let frontmatter = extract_frontmatter(&contents).ok_or(SkillParseError::MissingFrontmatter)?; + + let parsed: SkillFrontmatter = + serde_yaml::from_str(&frontmatter).map_err(SkillParseError::InvalidYaml)?; + + let base_name = parsed + .name + .as_deref() + .map(sanitize_single_line) + .filter(|value| !value.is_empty()) + .unwrap_or_else(|| default_skill_name(path)); + let name = namespaced_skill_name(path, &base_name); + let description = parsed + .description + .as_deref() + .map(sanitize_single_line) + .unwrap_or_default(); + let short_description = parsed + .metadata + .short_description + .as_deref() + .map(sanitize_single_line) + .filter(|value| !value.is_empty()); + let LoadedSkillMetadata { + interface, + dependencies, + policy, + permission_profile, + managed_network_override, + } = load_skill_metadata(path); + + validate_len(&name, MAX_NAME_LEN, "name")?; + validate_len(&description, MAX_DESCRIPTION_LEN, "description")?; + if let Some(short_description) = short_description.as_deref() { + validate_len( + short_description, + MAX_SHORT_DESCRIPTION_LEN, + "metadata.short-description", + )?; + } + + let resolved_path = canonicalize_path(path).unwrap_or_else(|_| path.to_path_buf()); + + Ok(SkillMetadata { + name, + description, + short_description, + interface, + dependencies, + policy, + permission_profile, + managed_network_override, + path_to_skills_md: resolved_path, + scope, + }) +} + +fn default_skill_name(path: &Path) -> String { + path.parent() + .and_then(Path::file_name) + .and_then(|name| name.to_str()) + .map(sanitize_single_line) + .filter(|value| !value.is_empty()) + .unwrap_or_else(|| "skill".to_string()) +} + +fn namespaced_skill_name(path: &Path, base_name: &str) -> String { + plugin_namespace_for_skill_path(path) + .map(|namespace| format!("{namespace}:{base_name}")) + .unwrap_or_else(|| base_name.to_string()) +} + +fn load_skill_metadata(skill_path: &Path) -> LoadedSkillMetadata { + // Fail open: optional metadata should not block loading SKILL.md. + let Some(skill_dir) = skill_path.parent() else { + return LoadedSkillMetadata::default(); + }; + let metadata_path = skill_dir + .join(SKILLS_METADATA_DIR) + .join(SKILLS_METADATA_FILENAME); + if !metadata_path.exists() { + return LoadedSkillMetadata::default(); + } + + let contents = match fs::read_to_string(&metadata_path) { + Ok(contents) => contents, + Err(error) => { + tracing::warn!( + "ignoring {path}: failed to read {label}: {error}", + path = metadata_path.display(), + label = SKILLS_METADATA_FILENAME + ); + return LoadedSkillMetadata::default(); + } + }; + + let parsed: SkillMetadataFile = { + let _guard = AbsolutePathBufGuard::new(skill_dir); + match serde_yaml::from_str(&contents) { + Ok(parsed) => parsed, + Err(error) => { + tracing::warn!( + "ignoring {path}: invalid {label}: {error}", + path = metadata_path.display(), + label = SKILLS_METADATA_FILENAME + ); + return LoadedSkillMetadata::default(); + } + } + }; + + let SkillMetadataFile { + interface, + dependencies, + policy, + permissions, + } = parsed; + let (permission_profile, managed_network_override) = normalize_permissions(permissions); + LoadedSkillMetadata { + interface: resolve_interface(interface, skill_dir), + dependencies: resolve_dependencies(dependencies), + policy: resolve_policy(policy), + permission_profile, + managed_network_override, + } +} + +fn normalize_permissions( + permissions: Option, +) -> ( + Option, + Option, +) { + let Some(permissions) = permissions else { + return (None, None); + }; + let managed_network_override = permissions + .network + .as_ref() + .map(|network| SkillManagedNetworkOverride { + allowed_domains: network.allowed_domains.clone(), + denied_domains: network.denied_domains.clone(), + }) + .filter(SkillManagedNetworkOverride::has_domain_overrides); + let permission_profile = PermissionProfile { + network: permissions.network.and_then(|network| { + let network = NetworkPermissions { + enabled: network.enabled, + }; + (!network.is_empty()).then_some(network) + }), + file_system: permissions + .file_system + .filter(|file_system| !file_system.is_empty()), + macos: permissions.macos, + }; + + ( + (!permission_profile.is_empty()).then_some(permission_profile), + managed_network_override, + ) +} + +fn resolve_interface(interface: Option, skill_dir: &Path) -> Option { + let interface = interface?; + let interface = SkillInterface { + display_name: resolve_str( + interface.display_name, + MAX_NAME_LEN, + "interface.display_name", + ), + short_description: resolve_str( + interface.short_description, + MAX_SHORT_DESCRIPTION_LEN, + "interface.short_description", + ), + icon_small: resolve_asset_path(skill_dir, "interface.icon_small", interface.icon_small), + icon_large: resolve_asset_path(skill_dir, "interface.icon_large", interface.icon_large), + brand_color: resolve_color_str(interface.brand_color, "interface.brand_color"), + default_prompt: resolve_str( + interface.default_prompt, + MAX_DEFAULT_PROMPT_LEN, + "interface.default_prompt", + ), + }; + let has_fields = interface.display_name.is_some() + || interface.short_description.is_some() + || interface.icon_small.is_some() + || interface.icon_large.is_some() + || interface.brand_color.is_some() + || interface.default_prompt.is_some(); + if has_fields { Some(interface) } else { None } +} + +fn resolve_dependencies(dependencies: Option) -> Option { + let dependencies = dependencies?; + let tools: Vec = dependencies + .tools + .into_iter() + .filter_map(resolve_dependency_tool) + .collect(); + if tools.is_empty() { + None + } else { + Some(SkillDependencies { tools }) + } +} + +fn resolve_policy(policy: Option) -> Option { + policy.map(|policy| SkillPolicy { + allow_implicit_invocation: policy.allow_implicit_invocation, + products: policy.products, + }) +} + +fn resolve_dependency_tool(tool: DependencyTool) -> Option { + let r#type = resolve_required_str( + tool.kind, + MAX_DEPENDENCY_TYPE_LEN, + "dependencies.tools.type", + )?; + let value = resolve_required_str( + tool.value, + MAX_DEPENDENCY_VALUE_LEN, + "dependencies.tools.value", + )?; + let description = resolve_str( + tool.description, + MAX_DEPENDENCY_DESCRIPTION_LEN, + "dependencies.tools.description", + ); + let transport = resolve_str( + tool.transport, + MAX_DEPENDENCY_TRANSPORT_LEN, + "dependencies.tools.transport", + ); + let command = resolve_str( + tool.command, + MAX_DEPENDENCY_COMMAND_LEN, + "dependencies.tools.command", + ); + let url = resolve_str(tool.url, MAX_DEPENDENCY_URL_LEN, "dependencies.tools.url"); + + Some(SkillToolDependency { + r#type, + value, + description, + transport, + command, + url, + }) +} + +fn resolve_asset_path( + skill_dir: &Path, + field: &'static str, + path: Option, +) -> Option { + // Icons must be relative paths under the skill's assets/ directory; otherwise return None. + let path = path?; + if path.as_os_str().is_empty() { + return None; + } + + let assets_dir = skill_dir.join("assets"); + if path.is_absolute() { + tracing::warn!( + "ignoring {field}: icon must be a relative assets path (not {})", + assets_dir.display() + ); + return None; + } + + let mut normalized = PathBuf::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::Normal(component) => normalized.push(component), + Component::ParentDir => { + tracing::warn!("ignoring {field}: icon path must not contain '..'"); + return None; + } + _ => { + tracing::warn!("ignoring {field}: icon path must be under assets/"); + return None; + } + } + } + + let mut components = normalized.components(); + match components.next() { + Some(Component::Normal(component)) if component == "assets" => {} + _ => { + tracing::warn!("ignoring {field}: icon path must be under assets/"); + return None; + } + } + + Some(skill_dir.join(normalized)) +} + +fn sanitize_single_line(raw: &str) -> String { + raw.split_whitespace().collect::>().join(" ") +} + +fn validate_len( + value: &str, + max_len: usize, + field_name: &'static str, +) -> Result<(), SkillParseError> { + if value.is_empty() { + return Err(SkillParseError::MissingField(field_name)); + } + if value.chars().count() > max_len { + return Err(SkillParseError::InvalidField { + field: field_name, + reason: format!("exceeds maximum length of {max_len} characters"), + }); + } + Ok(()) +} + +fn resolve_str(value: Option, max_len: usize, field: &'static str) -> Option { + let value = value?; + let value = sanitize_single_line(&value); + if value.is_empty() { + tracing::warn!("ignoring {field}: value is empty"); + return None; + } + if value.chars().count() > max_len { + tracing::warn!("ignoring {field}: exceeds maximum length of {max_len} characters"); + return None; + } + Some(value) +} + +fn resolve_required_str( + value: Option, + max_len: usize, + field: &'static str, +) -> Option { + let Some(value) = value else { + tracing::warn!("ignoring {field}: value is missing"); + return None; + }; + resolve_str(Some(value), max_len, field) +} + +fn resolve_color_str(value: Option, field: &'static str) -> Option { + let value = value?; + let value = value.trim(); + if value.is_empty() { + tracing::warn!("ignoring {field}: value is empty"); + return None; + } + let mut chars = value.chars(); + if value.len() == 7 && chars.next() == Some('#') && chars.all(|c| c.is_ascii_hexdigit()) { + Some(value.to_string()) + } else { + tracing::warn!("ignoring {field}: expected #RRGGBB, got {value}"); + None + } +} + +fn extract_frontmatter(contents: &str) -> Option { + let mut lines = contents.lines(); + if !matches!(lines.next(), Some(line) if line.trim() == "---") { + return None; + } + + let mut frontmatter_lines: Vec<&str> = Vec::new(); + let mut found_closing = false; + for line in lines.by_ref() { + if line.trim() == "---" { + found_closing = true; + break; + } + frontmatter_lines.push(line); + } + + if frontmatter_lines.is_empty() || !found_closing { + return None; + } + + Some(frontmatter_lines.join("\n")) +} +#[cfg(test)] +pub(crate) fn skill_roots_from_layer_stack( + config_layer_stack: &ConfigLayerStack, + home_dir: Option<&Path>, +) -> Vec { + skill_roots_with_home_dir(config_layer_stack, Path::new("."), home_dir, Vec::new()) +} + +#[cfg(test)] +#[path = "loader_tests.rs"] +mod tests; diff --git a/codex-rs/core-skills/src/skills/loader_tests.rs b/codex-rs/core-skills/src/skills/loader_tests.rs new file mode 100644 index 0000000000..65c6a2520f --- /dev/null +++ b/codex-rs/core-skills/src/skills/loader_tests.rs @@ -0,0 +1,2060 @@ +use super::*; +use crate::config::ConfigBuilder; +use crate::config::ConfigOverrides; +use crate::config::ConfigToml; +use crate::config::ProjectConfig; +use crate::config_loader::ConfigLayerEntry; +use crate::config_loader::ConfigRequirements; +use crate::config_loader::ConfigRequirementsToml; +use codex_config::CONFIG_TOML_FILE; +use codex_config::ConfigLayerStack; +use codex_protocol::config_types::TrustLevel; +use codex_protocol::models::FileSystemPermissions; +use codex_protocol::models::MacOsAutomationPermission; +use codex_protocol::models::MacOsContactsPermission; +use codex_protocol::models::MacOsPreferencesPermission; +use codex_protocol::models::MacOsSeatbeltProfileExtensions; +use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::Product; +use codex_protocol::protocol::SkillScope; +use codex_utils_absolute_path::AbsolutePathBuf; +use pretty_assertions::assert_eq; +use std::collections::HashMap; +use std::path::Path; +use tempfile::TempDir; +use toml::Value as TomlValue; + +const REPO_ROOT_CONFIG_DIR_NAME: &str = ".codex"; + +async fn make_config(codex_home: &TempDir) -> Config { + make_config_for_cwd(codex_home, codex_home.path().to_path_buf()).await +} + +async fn make_config_for_cwd(codex_home: &TempDir, cwd: PathBuf) -> Config { + let trust_root = cwd + .ancestors() + .find(|ancestor| ancestor.join(".git").exists()) + .map(Path::to_path_buf) + .unwrap_or_else(|| cwd.clone()); + + fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + toml::to_string(&ConfigToml { + projects: Some(HashMap::from([( + trust_root.to_string_lossy().to_string(), + ProjectConfig { + trust_level: Some(TrustLevel::Trusted), + }, + )])), + ..Default::default() + }) + .expect("serialize config"), + ) + .unwrap(); + + let harness_overrides = ConfigOverrides { + cwd: Some(cwd), + ..Default::default() + }; + + ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(harness_overrides) + .build() + .await + .expect("defaults for test should always succeed") +} + +fn load_skills_for_test(config: &Config) -> SkillLoadOutcome { + // Keep unit tests hermetic by never scanning the real `$HOME/.agents/skills`. + super::load_skills_from_roots(super::skill_roots_with_home_dir( + &config.config_layer_stack, + &config.cwd, + None, + Vec::new(), + )) +} + +fn mark_as_git_repo(dir: &Path) { + // Config/project-root discovery only checks for the presence of `.git` (file or dir), + // so we can avoid shelling out to `git init` in tests. + fs::write(dir.join(".git"), "gitdir: fake\n").unwrap(); +} + +fn normalized(path: &Path) -> PathBuf { + canonicalize_path(path).unwrap_or_else(|_| path.to_path_buf()) +} + +#[test] +fn skill_roots_from_layer_stack_maps_user_to_user_and_system_cache_and_system_to_admin() +-> anyhow::Result<()> { + let tmp = tempfile::tempdir()?; + + let system_folder = tmp.path().join("etc/codex"); + let home_folder = tmp.path().join("home"); + let user_folder = home_folder.join("codex"); + fs::create_dir_all(&system_folder)?; + fs::create_dir_all(&user_folder)?; + + // The file path doesn't need to exist; it's only used to derive the config folder. + let system_file = AbsolutePathBuf::from_absolute_path(system_folder.join("config.toml"))?; + let user_file = AbsolutePathBuf::from_absolute_path(user_folder.join("config.toml"))?; + + let layers = vec![ + ConfigLayerEntry::new( + ConfigLayerSource::System { file: system_file }, + TomlValue::Table(toml::map::Map::new()), + ), + ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + TomlValue::Table(toml::map::Map::new()), + ), + ]; + let stack = ConfigLayerStack::new( + layers, + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + )?; + + let got = skill_roots_from_layer_stack(&stack, Some(&home_folder)) + .into_iter() + .map(|root| (root.scope, root.path)) + .collect::>(); + + assert_eq!( + got, + vec![ + (SkillScope::User, user_folder.join("skills")), + ( + SkillScope::User, + home_folder.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME) + ), + ( + SkillScope::System, + user_folder.join("skills").join(".system") + ), + (SkillScope::Admin, system_folder.join("skills")), + ] + ); + + Ok(()) +} + +#[test] +fn skill_roots_from_layer_stack_includes_disabled_project_layers() -> anyhow::Result<()> { + let tmp = tempfile::tempdir()?; + + let home_folder = tmp.path().join("home"); + let user_folder = home_folder.join("codex"); + fs::create_dir_all(&user_folder)?; + + let project_root = tmp.path().join("repo"); + let dot_codex = project_root.join(".codex"); + fs::create_dir_all(&dot_codex)?; + + let user_file = AbsolutePathBuf::from_absolute_path(user_folder.join("config.toml"))?; + let project_dot_codex = AbsolutePathBuf::from_absolute_path(&dot_codex)?; + + let layers = vec![ + ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + TomlValue::Table(toml::map::Map::new()), + ), + ConfigLayerEntry::new_disabled( + ConfigLayerSource::Project { + dot_codex_folder: project_dot_codex, + }, + TomlValue::Table(toml::map::Map::new()), + "marked untrusted", + ), + ]; + let stack = ConfigLayerStack::new( + layers, + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + )?; + + let got = skill_roots_from_layer_stack(&stack, Some(&home_folder)) + .into_iter() + .map(|root| (root.scope, root.path)) + .collect::>(); + + assert_eq!( + got, + vec![ + (SkillScope::Repo, dot_codex.join("skills")), + (SkillScope::User, user_folder.join("skills")), + ( + SkillScope::User, + home_folder.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME) + ), + ( + SkillScope::System, + user_folder.join("skills").join(".system") + ), + ] + ); + + Ok(()) +} + +#[test] +fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<()> { + let tmp = tempfile::tempdir()?; + + let home_folder = tmp.path().join("home"); + let user_folder = home_folder.join("codex"); + fs::create_dir_all(&user_folder)?; + + let user_file = AbsolutePathBuf::from_absolute_path(user_folder.join("config.toml"))?; + let layers = vec![ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + TomlValue::Table(toml::map::Map::new()), + )]; + let stack = ConfigLayerStack::new( + layers, + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + )?; + + let skill_path = write_skill_at( + &home_folder.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME), + "agents-home", + "agents-home-skill", + "from home agents", + ); + + let outcome = load_skills_from_roots(skill_roots_from_layer_stack(&stack, Some(&home_folder))); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "agents-home-skill".to_string(), + description: "from home agents".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::User, + }] + ); + + Ok(()) +} + +fn write_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) -> PathBuf { + write_skill_at(&codex_home.path().join("skills"), dir, name, description) +} + +fn write_system_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) -> PathBuf { + write_skill_at( + &codex_home.path().join("skills/.system"), + dir, + name, + description, + ) +} + +fn write_skill_at(root: &Path, dir: &str, name: &str, description: &str) -> PathBuf { + let skill_dir = root.join(dir); + fs::create_dir_all(&skill_dir).unwrap(); + let indented_description = description.replace('\n', "\n "); + let content = + format!("---\nname: {name}\ndescription: |-\n {indented_description}\n---\n\n# Body\n"); + let path = skill_dir.join(SKILLS_FILENAME); + fs::write(&path, content).unwrap(); + path +} + +fn write_raw_skill_at(root: &Path, dir: &str, frontmatter: &str) -> PathBuf { + let skill_dir = root.join(dir); + fs::create_dir_all(&skill_dir).unwrap(); + let path = skill_dir.join(SKILLS_FILENAME); + let content = format!("---\n{frontmatter}\n---\n\n# Body\n"); + fs::write(&path, content).unwrap(); + path +} + +fn write_skill_metadata_at(skill_dir: &Path, contents: &str) -> PathBuf { + let path = skill_dir + .join(SKILLS_METADATA_DIR) + .join(SKILLS_METADATA_FILENAME); + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).unwrap(); + } + fs::write(&path, contents).unwrap(); + path +} + +fn write_skill_interface_at(skill_dir: &Path, contents: &str) -> PathBuf { + write_skill_metadata_at(skill_dir, contents) +} + +#[tokio::test] +async fn loads_skill_dependencies_metadata_from_yaml() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "dep-skill", "from json"); + let skill_dir = skill_path.parent().expect("skill dir"); + + write_skill_metadata_at( + skill_dir, + r#" +{ + "dependencies": { + "tools": [ + { + "type": "env_var", + "value": "GITHUB_TOKEN", + "description": "GitHub API token with repo scopes" + }, + { + "type": "mcp", + "value": "github", + "description": "GitHub MCP server", + "transport": "streamable_http", + "url": "https://example.com/mcp" + }, + { + "type": "cli", + "value": "gh", + "description": "GitHub CLI" + }, + { + "type": "mcp", + "value": "local-gh", + "description": "Local GH MCP server", + "transport": "stdio", + "command": "gh-mcp" + } + ] + } +} +"#, + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "dep-skill".to_string(), + description: "from json".to_string(), + short_description: None, + interface: None, + dependencies: Some(SkillDependencies { + tools: vec![ + SkillToolDependency { + r#type: "env_var".to_string(), + value: "GITHUB_TOKEN".to_string(), + description: Some("GitHub API token with repo scopes".to_string()), + transport: None, + command: None, + url: None, + }, + SkillToolDependency { + r#type: "mcp".to_string(), + value: "github".to_string(), + description: Some("GitHub MCP server".to_string()), + transport: Some("streamable_http".to_string()), + command: None, + url: Some("https://example.com/mcp".to_string()), + }, + SkillToolDependency { + r#type: "cli".to_string(), + value: "gh".to_string(), + description: Some("GitHub CLI".to_string()), + transport: None, + command: None, + url: None, + }, + SkillToolDependency { + r#type: "mcp".to_string(), + value: "local-gh".to_string(), + description: Some("Local GH MCP server".to_string()), + transport: Some("stdio".to_string()), + command: Some("gh-mcp".to_string()), + url: None, + }, + ], + }), + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::User, + }] + ); +} + +#[tokio::test] +async fn loads_skill_interface_metadata_from_yaml() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); + let skill_dir = skill_path.parent().expect("skill dir"); + let normalized_skill_dir = normalized(skill_dir); + + write_skill_interface_at( + skill_dir, + r##" +interface: + display_name: "UI Skill" + short_description: " short desc " + icon_small: "./assets/small-400px.png" + icon_large: "./assets/large-logo.svg" + brand_color: "#3B82F6" + default_prompt: " default prompt " +"##, + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + let user_skills: Vec = outcome + .skills + .into_iter() + .filter(|skill| skill.scope == SkillScope::User) + .collect(); + assert_eq!( + user_skills, + vec![SkillMetadata { + name: "ui-skill".to_string(), + description: "from json".to_string(), + short_description: None, + interface: Some(SkillInterface { + display_name: Some("UI Skill".to_string()), + short_description: Some("short desc".to_string()), + icon_small: Some(normalized_skill_dir.join("assets/small-400px.png")), + icon_large: Some(normalized_skill_dir.join("assets/large-logo.svg")), + brand_color: Some("#3B82F6".to_string()), + default_prompt: Some("default prompt".to_string()), + }), + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(skill_path.as_path()), + scope: SkillScope::User, + }] + ); +} + +#[tokio::test] +async fn loads_skill_policy_from_yaml() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "policy-skill", "from json"); + let skill_dir = skill_path.parent().expect("skill dir"); + + write_skill_metadata_at( + skill_dir, + r#" +policy: + allow_implicit_invocation: false +"#, + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!( + outcome.skills[0].policy, + Some(SkillPolicy { + allow_implicit_invocation: Some(false), + products: vec![], + }) + ); + assert!(outcome.allowed_skills_for_implicit_invocation().is_empty()); +} + +#[tokio::test] +async fn empty_skill_policy_defaults_to_allow_implicit_invocation() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "policy-empty", "from json"); + let skill_dir = skill_path.parent().expect("skill dir"); + + write_skill_metadata_at( + skill_dir, + r#" +policy: {} +"#, + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!( + outcome.skills[0].policy, + Some(SkillPolicy { + allow_implicit_invocation: None, + products: vec![], + }) + ); + assert_eq!( + outcome.allowed_skills_for_implicit_invocation(), + outcome.skills + ); +} + +#[tokio::test] +async fn loads_skill_policy_products_from_yaml() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "policy-products", "from yaml"); + let skill_dir = skill_path.parent().expect("skill dir"); + + write_skill_metadata_at( + skill_dir, + r#" +policy: + products: + - codex + - CHATGPT + - atlas +"#, + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!( + outcome.skills[0].policy, + Some(SkillPolicy { + allow_implicit_invocation: None, + products: vec![Product::Codex, Product::Chatgpt, Product::Atlas], + }) + ); +} + +#[tokio::test] +async fn loads_skill_permissions_from_yaml() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "permissions-skill", "from yaml"); + let skill_dir = skill_path.parent().expect("skill dir"); + fs::create_dir_all(skill_dir.join("data")).expect("create read path"); + fs::create_dir_all(skill_dir.join("output")).expect("create write path"); + + write_skill_metadata_at( + skill_dir, + r#" +permissions: + network: + enabled: true + file_system: + read: + - "./data" + write: + - "./output" +"#, + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!( + outcome.skills[0].permission_profile, + Some(PermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + file_system: Some(FileSystemPermissions { + read: Some(vec![ + AbsolutePathBuf::try_from(normalized(skill_dir.join("data").as_path())) + .expect("absolute data path"), + ]), + write: Some(vec![ + AbsolutePathBuf::try_from(normalized(skill_dir.join("output").as_path())) + .expect("absolute output path"), + ]), + }), + macos: None, + }) + ); + assert_eq!(outcome.skills[0].managed_network_override, None); +} + +#[tokio::test] +async fn empty_skill_permissions_do_not_create_profile() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "permissions-empty", "from yaml"); + let skill_dir = skill_path.parent().expect("skill dir"); + + write_skill_metadata_at( + skill_dir, + r#" +permissions: {} +"#, + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].permission_profile, None); +} + +#[test] +fn normalize_permissions_splits_managed_network_overrides() { + let (permission_profile, managed_network_override) = + normalize_permissions(Some(SkillPermissionProfile { + network: Some(SkillNetworkPermissions { + enabled: Some(true), + allowed_domains: Some(vec!["skill.example.com".to_string()]), + denied_domains: Some(vec!["blocked.skill.example.com".to_string()]), + }), + file_system: None, + macos: None, + })); + + assert_eq!( + permission_profile, + Some(PermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + file_system: None, + macos: None, + }) + ); + assert_eq!( + managed_network_override, + Some(SkillManagedNetworkOverride { + allowed_domains: Some(vec!["skill.example.com".to_string()]), + denied_domains: Some(vec!["blocked.skill.example.com".to_string()]), + }) + ); +} + +#[test] +fn normalize_permissions_preserves_network_gate_separately_from_overrides() { + let (permission_profile, managed_network_override) = + normalize_permissions(Some(SkillPermissionProfile { + network: Some(SkillNetworkPermissions { + enabled: Some(false), + allowed_domains: Some(vec!["skill.example.com".to_string()]), + denied_domains: None, + }), + file_system: None, + macos: None, + })); + + assert_eq!( + permission_profile, + Some(PermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(false), + }), + file_system: None, + macos: None, + }) + ); + assert_eq!( + managed_network_override, + Some(SkillManagedNetworkOverride { + allowed_domains: Some(vec!["skill.example.com".to_string()]), + denied_domains: None, + }) + ); +} + +#[test] +fn skill_metadata_parses_macos_permissions_yaml() { + let parsed = serde_yaml::from_str::( + r#" +permissions: + macos: + macos_preferences: "read_write" + macos_automation: + - "com.apple.Notes" + macos_launch_services: true + macos_accessibility: true + macos_calendar: true +"#, + ) + .expect("parse skill metadata"); + + assert_eq!( + parsed.permissions, + Some(SkillPermissionProfile { + network: None, + file_system: None, + macos: Some(MacOsSeatbeltProfileExtensions { + macos_preferences: MacOsPreferencesPermission::ReadWrite, + macos_automation: MacOsAutomationPermission::BundleIds(vec![ + "com.apple.Notes".to_string(), + ]), + macos_launch_services: true, + macos_accessibility: true, + macos_calendar: true, + macos_reminders: false, + macos_contacts: MacOsContactsPermission::None, + }), + }) + ); +} + +#[test] +fn skill_metadata_parses_macos_reminders_permission_yaml() { + let parsed = serde_yaml::from_str::( + r#" +permissions: + macos: + macos_reminders: true +"#, + ) + .expect("parse reminders skill metadata"); + + assert_eq!( + parsed.permissions, + Some(SkillPermissionProfile { + network: None, + file_system: None, + macos: Some(MacOsSeatbeltProfileExtensions { + macos_preferences: MacOsPreferencesPermission::ReadOnly, + macos_automation: MacOsAutomationPermission::None, + macos_launch_services: false, + macos_accessibility: false, + macos_calendar: false, + macos_reminders: true, + macos_contacts: MacOsContactsPermission::None, + }), + }) + ); +} + +#[test] +fn skill_metadata_parses_network_domain_overrides_under_permissions() { + let parsed = serde_yaml::from_str::( + r#" +permissions: + network: + enabled: true + allowed_domains: + - "skill.example.com" + denied_domains: + - "blocked.skill.example.com" +"#, + ) + .expect("parse network skill metadata"); + + assert_eq!( + parsed.permissions, + Some(SkillPermissionProfile { + network: Some(SkillNetworkPermissions { + enabled: Some(true), + allowed_domains: Some(vec!["skill.example.com".to_string()]), + denied_domains: Some(vec!["blocked.skill.example.com".to_string()]), + }), + file_system: None, + macos: None, + }) + ); +} + +#[cfg(target_os = "macos")] +#[tokio::test] +async fn loads_skill_macos_permissions_from_yaml() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "permissions-macos", "from yaml"); + let skill_dir = skill_path.parent().expect("skill dir"); + + write_skill_metadata_at( + skill_dir, + r#" +permissions: + macos: + macos_preferences: "read_write" + macos_automation: + - "com.apple.Notes" + macos_launch_services: true + macos_accessibility: true + macos_calendar: true +"#, + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!( + outcome.skills[0].permission_profile, + Some(PermissionProfile { + macos: Some(MacOsSeatbeltProfileExtensions { + macos_preferences: MacOsPreferencesPermission::ReadWrite, + macos_automation: MacOsAutomationPermission::BundleIds(vec![ + "com.apple.Notes".to_string() + ],), + macos_launch_services: true, + macos_accessibility: true, + macos_calendar: true, + macos_reminders: false, + macos_contacts: MacOsContactsPermission::None, + }), + ..Default::default() + }) + ); +} + +#[cfg(not(target_os = "macos"))] +#[tokio::test] +async fn loads_skill_macos_permissions_from_yaml_non_macos_does_not_create_profile() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "permissions-macos", "from yaml"); + let skill_dir = skill_path.parent().expect("skill dir"); + + write_skill_metadata_at( + skill_dir, + r#" +permissions: + macos: + macos_preferences: "read_write" + macos_automation: + - "com.apple.Notes" + macos_launch_services: true + macos_accessibility: true + macos_calendar: true +"#, + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!( + outcome.skills[0].permission_profile, + Some(PermissionProfile { + macos: Some(MacOsSeatbeltProfileExtensions { + macos_preferences: MacOsPreferencesPermission::ReadWrite, + macos_automation: MacOsAutomationPermission::BundleIds(vec![ + "com.apple.Notes".to_string() + ],), + macos_launch_services: true, + macos_accessibility: true, + macos_calendar: true, + macos_reminders: false, + macos_contacts: MacOsContactsPermission::None, + }), + ..Default::default() + }) + ); +} + +#[tokio::test] +async fn accepts_icon_paths_under_assets_dir() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); + let skill_dir = skill_path.parent().expect("skill dir"); + let normalized_skill_dir = normalized(skill_dir); + + write_skill_interface_at( + skill_dir, + r#" +{ + "interface": { + "display_name": "UI Skill", + "icon_small": "assets/icon.png", + "icon_large": "./assets/logo.svg" + } +} +"#, + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "ui-skill".to_string(), + description: "from json".to_string(), + short_description: None, + interface: Some(SkillInterface { + display_name: Some("UI Skill".to_string()), + short_description: None, + icon_small: Some(normalized_skill_dir.join("assets/icon.png")), + icon_large: Some(normalized_skill_dir.join("assets/logo.svg")), + brand_color: None, + default_prompt: None, + }), + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::User, + }] + ); +} + +#[tokio::test] +async fn ignores_invalid_brand_color() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); + let skill_dir = skill_path.parent().expect("skill dir"); + + write_skill_interface_at( + skill_dir, + r#" +{ + "interface": { + "brand_color": "blue" + } +} +"#, + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "ui-skill".to_string(), + description: "from json".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::User, + }] + ); +} + +#[tokio::test] +async fn ignores_default_prompt_over_max_length() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); + let skill_dir = skill_path.parent().expect("skill dir"); + let normalized_skill_dir = normalized(skill_dir); + let too_long = "x".repeat(MAX_DEFAULT_PROMPT_LEN + 1); + + write_skill_interface_at( + skill_dir, + &format!( + r##" +{{ + "interface": {{ + "display_name": "UI Skill", + "icon_small": "./assets/small-400px.png", + "default_prompt": "{too_long}" + }} +}} +"## + ), + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "ui-skill".to_string(), + description: "from json".to_string(), + short_description: None, + interface: Some(SkillInterface { + display_name: Some("UI Skill".to_string()), + short_description: None, + icon_small: Some(normalized_skill_dir.join("assets/small-400px.png")), + icon_large: None, + brand_color: None, + default_prompt: None, + }), + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::User, + }] + ); +} + +#[tokio::test] +async fn drops_interface_when_icons_are_invalid() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); + let skill_dir = skill_path.parent().expect("skill dir"); + + write_skill_interface_at( + skill_dir, + r#" +{ + "interface": { + "icon_small": "icon.png", + "icon_large": "./assets/../logo.svg" + } +} +"#, + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "ui-skill".to_string(), + description: "from json".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::User, + }] + ); +} + +#[cfg(unix)] +fn symlink_dir(target: &Path, link: &Path) { + std::os::unix::fs::symlink(target, link).unwrap(); +} + +#[cfg(unix)] +fn symlink_file(target: &Path, link: &Path) { + std::os::unix::fs::symlink(target, link).unwrap(); +} + +#[tokio::test] +#[cfg(unix)] +async fn loads_skills_via_symlinked_subdir_for_user_scope() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let shared = tempfile::tempdir().expect("tempdir"); + + let shared_skill_path = write_skill_at(shared.path(), "demo", "linked-skill", "from link"); + + fs::create_dir_all(codex_home.path().join("skills")).unwrap(); + symlink_dir(shared.path(), &codex_home.path().join("skills/shared")); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "linked-skill".to_string(), + description: "from link".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&shared_skill_path), + scope: SkillScope::User, + }] + ); +} + +#[tokio::test] +#[cfg(unix)] +async fn ignores_symlinked_skill_file_for_user_scope() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let shared = tempfile::tempdir().expect("tempdir"); + + let shared_skill_path = write_skill_at(shared.path(), "demo", "linked-file-skill", "from link"); + + let skill_dir = codex_home.path().join("skills/demo"); + fs::create_dir_all(&skill_dir).unwrap(); + symlink_file(&shared_skill_path, &skill_dir.join(SKILLS_FILENAME)); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills, Vec::new()); +} + +#[tokio::test] +#[cfg(unix)] +async fn does_not_loop_on_symlink_cycle_for_user_scope() { + let codex_home = tempfile::tempdir().expect("tempdir"); + + // Create a cycle: + // $CODEX_HOME/skills/cycle/loop -> $CODEX_HOME/skills/cycle + let cycle_dir = codex_home.path().join("skills/cycle"); + fs::create_dir_all(&cycle_dir).unwrap(); + symlink_dir(&cycle_dir, &cycle_dir.join("loop")); + + let skill_path = write_skill_at(&cycle_dir, "demo", "cycle-skill", "still loads"); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "cycle-skill".to_string(), + description: "still loads".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::User, + }] + ); +} + +#[test] +#[cfg(unix)] +fn loads_skills_via_symlinked_subdir_for_admin_scope() { + let admin_root = tempfile::tempdir().expect("tempdir"); + let shared = tempfile::tempdir().expect("tempdir"); + + let shared_skill_path = + write_skill_at(shared.path(), "demo", "admin-linked-skill", "from link"); + fs::create_dir_all(admin_root.path()).unwrap(); + symlink_dir(shared.path(), &admin_root.path().join("shared")); + + let outcome = load_skills_from_roots([SkillRoot { + path: admin_root.path().to_path_buf(), + scope: SkillScope::Admin, + }]); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "admin-linked-skill".to_string(), + description: "from link".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&shared_skill_path), + scope: SkillScope::Admin, + }] + ); +} + +#[tokio::test] +#[cfg(unix)] +async fn loads_skills_via_symlinked_subdir_for_repo_scope() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + mark_as_git_repo(repo_dir.path()); + let shared = tempfile::tempdir().expect("tempdir"); + + let linked_skill_path = write_skill_at(shared.path(), "demo", "repo-linked-skill", "from link"); + let repo_skills_root = repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME); + fs::create_dir_all(&repo_skills_root).unwrap(); + symlink_dir(shared.path(), &repo_skills_root.join("shared")); + + let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "repo-linked-skill".to_string(), + description: "from link".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&linked_skill_path), + scope: SkillScope::Repo, + }] + ); +} + +#[tokio::test] +#[cfg(unix)] +async fn system_scope_ignores_symlinked_subdir() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let shared = tempfile::tempdir().expect("tempdir"); + + write_skill_at(shared.path(), "demo", "system-linked-skill", "from link"); + + let system_root = codex_home.path().join("skills/.system"); + fs::create_dir_all(&system_root).unwrap(); + symlink_dir(shared.path(), &system_root.join("shared")); + + let outcome = load_skills_from_roots([SkillRoot { + path: system_root, + scope: SkillScope::System, + }]); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 0); +} + +#[tokio::test] +async fn respects_max_scan_depth_for_user_scope() { + let codex_home = tempfile::tempdir().expect("tempdir"); + + let within_depth_path = write_skill( + &codex_home, + "d0/d1/d2/d3/d4/d5", + "within-depth-skill", + "loads", + ); + let _too_deep_path = write_skill( + &codex_home, + "d0/d1/d2/d3/d4/d5/d6", + "too-deep-skill", + "should not load", + ); + + let skills_root = codex_home.path().join("skills"); + let outcome = load_skills_from_roots([SkillRoot { + path: skills_root, + scope: SkillScope::User, + }]); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "within-depth-skill".to_string(), + description: "loads".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&within_depth_path), + scope: SkillScope::User, + }] + ); +} + +#[tokio::test] +async fn loads_valid_skill() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill(&codex_home, "demo", "demo-skill", "does things\ncarefully"); + let cfg = make_config(&codex_home).await; + + let outcome = load_skills_for_test(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "demo-skill".to_string(), + description: "does things carefully".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::User, + }] + ); +} + +#[tokio::test] +async fn falls_back_to_directory_name_when_skill_name_is_missing() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_path = write_raw_skill_at( + &codex_home.path().join("skills"), + "directory-derived", + "description: fallback name", + ); + let cfg = make_config(&codex_home).await; + + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "directory-derived".to_string(), + description: "fallback name".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::User, + }] + ); +} + +#[tokio::test] +async fn namespaces_plugin_skills_using_plugin_name() { + let root = tempfile::tempdir().expect("tempdir"); + let plugin_root = root.path().join("plugins/sample"); + let skill_path = write_raw_skill_at( + &plugin_root.join("skills"), + "sample-search", + "description: search sample data", + ); + fs::create_dir_all(plugin_root.join(".codex-plugin")).unwrap(); + fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + r#"{"name":"sample"}"#, + ) + .unwrap(); + + let outcome = load_skills_from_roots([SkillRoot { + path: plugin_root.join("skills"), + scope: SkillScope::User, + }]); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "sample:sample-search".to_string(), + description: "search sample data".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::User, + }] + ); +} + +#[tokio::test] +async fn loads_short_description_from_metadata() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_dir = codex_home.path().join("skills/demo"); + fs::create_dir_all(&skill_dir).unwrap(); + let contents = "---\nname: demo-skill\ndescription: long description\nmetadata:\n short-description: short summary\n---\n\n# Body\n"; + let skill_path = skill_dir.join(SKILLS_FILENAME); + fs::write(&skill_path, contents).unwrap(); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "demo-skill".to_string(), + description: "long description".to_string(), + short_description: Some("short summary".to_string()), + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::User, + }] + ); +} + +#[tokio::test] +async fn enforces_short_description_length_limits() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let skill_dir = codex_home.path().join("skills/demo"); + fs::create_dir_all(&skill_dir).unwrap(); + let too_long = "x".repeat(MAX_SHORT_DESCRIPTION_LEN + 1); + let contents = format!( + "---\nname: demo-skill\ndescription: long description\nmetadata:\n short-description: {too_long}\n---\n\n# Body\n" + ); + fs::write(skill_dir.join(SKILLS_FILENAME), contents).unwrap(); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + assert_eq!(outcome.skills.len(), 0); + assert_eq!(outcome.errors.len(), 1); + assert!( + outcome.errors[0] + .message + .contains("invalid metadata.short-description"), + "expected length error, got: {:?}", + outcome.errors + ); +} + +#[tokio::test] +async fn skips_hidden_and_invalid() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let hidden_dir = codex_home.path().join("skills/.hidden"); + fs::create_dir_all(&hidden_dir).unwrap(); + fs::write( + hidden_dir.join(SKILLS_FILENAME), + "---\nname: hidden\ndescription: hidden\n---\n", + ) + .unwrap(); + + // Invalid because missing closing frontmatter. + let invalid_dir = codex_home.path().join("skills/invalid"); + fs::create_dir_all(&invalid_dir).unwrap(); + fs::write(invalid_dir.join(SKILLS_FILENAME), "---\nname: bad").unwrap(); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills_for_test(&cfg); + assert_eq!(outcome.skills.len(), 0); + assert_eq!(outcome.errors.len(), 1); + assert!( + outcome.errors[0] + .message + .contains("missing YAML frontmatter"), + "expected frontmatter error" + ); +} + +#[tokio::test] +async fn enforces_length_limits() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let max_desc = "\u{1F4A1}".repeat(MAX_DESCRIPTION_LEN); + write_skill(&codex_home, "max-len", "max-len", &max_desc); + let cfg = make_config(&codex_home).await; + + let outcome = load_skills_for_test(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + + let too_long_desc = "\u{1F4A1}".repeat(MAX_DESCRIPTION_LEN + 1); + write_skill(&codex_home, "too-long", "too-long", &too_long_desc); + let outcome = load_skills_for_test(&cfg); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.errors.len(), 1); + assert!( + outcome.errors[0].message.contains("invalid description"), + "expected length error" + ); +} + +#[tokio::test] +async fn loads_skills_from_repo_root() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + mark_as_git_repo(repo_dir.path()); + + let skills_root = repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME); + let skill_path = write_skill_at(&skills_root, "repo", "repo-skill", "from repo"); + let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; + + let outcome = load_skills_for_test(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "repo-skill".to_string(), + description: "from repo".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::Repo, + }] + ); +} + +#[tokio::test] +async fn loads_skills_from_agents_dir_without_codex_dir() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + mark_as_git_repo(repo_dir.path()); + + let skill_path = write_skill_at( + &repo_dir.path().join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME), + "agents", + "agents-skill", + "from agents", + ); + let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; + + let outcome = load_skills_for_test(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "agents-skill".to_string(), + description: "from agents".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::Repo, + }] + ); +} + +#[tokio::test] +async fn loads_skills_from_all_codex_dirs_under_project_root() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + mark_as_git_repo(repo_dir.path()); + + let nested_dir = repo_dir.path().join("nested/inner"); + fs::create_dir_all(&nested_dir).unwrap(); + + let root_skill_path = write_skill_at( + &repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "root", + "root-skill", + "from root", + ); + let nested_skill_path = write_skill_at( + &repo_dir + .path() + .join("nested") + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "nested", + "nested-skill", + "from nested", + ); + + let cfg = make_config_for_cwd(&codex_home, nested_dir).await; + + let outcome = load_skills_for_test(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![ + SkillMetadata { + name: "nested-skill".to_string(), + description: "from nested".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&nested_skill_path), + scope: SkillScope::Repo, + }, + SkillMetadata { + name: "root-skill".to_string(), + description: "from root".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&root_skill_path), + scope: SkillScope::Repo, + }, + ] + ); +} + +#[tokio::test] +async fn loads_skills_from_codex_dir_when_not_git_repo() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let work_dir = tempfile::tempdir().expect("tempdir"); + + let skill_path = write_skill_at( + &work_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "local", + "local-skill", + "from cwd", + ); + + let cfg = make_config_for_cwd(&codex_home, work_dir.path().to_path_buf()).await; + + let outcome = load_skills_for_test(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "local-skill".to_string(), + description: "from cwd".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::Repo, + }] + ); +} + +#[tokio::test] +async fn deduplicates_by_path_preferring_first_root() { + let root = tempfile::tempdir().expect("tempdir"); + + let skill_path = write_skill_at(root.path(), "dupe", "dupe-skill", "from repo"); + + let outcome = load_skills_from_roots([ + SkillRoot { + path: root.path().to_path_buf(), + scope: SkillScope::Repo, + }, + SkillRoot { + path: root.path().to_path_buf(), + scope: SkillScope::User, + }, + ]); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "dupe-skill".to_string(), + description: "from repo".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::Repo, + }] + ); +} + +#[tokio::test] +async fn keeps_duplicate_names_from_repo_and_user() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + mark_as_git_repo(repo_dir.path()); + + let user_skill_path = write_skill(&codex_home, "user", "dupe-skill", "from user"); + let repo_skill_path = write_skill_at( + &repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "repo", + "dupe-skill", + "from repo", + ); + + let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; + + let outcome = load_skills_for_test(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![ + SkillMetadata { + name: "dupe-skill".to_string(), + description: "from repo".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&repo_skill_path), + scope: SkillScope::Repo, + }, + SkillMetadata { + name: "dupe-skill".to_string(), + description: "from user".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&user_skill_path), + scope: SkillScope::User, + }, + ] + ); +} + +#[tokio::test] +async fn keeps_duplicate_names_from_nested_codex_dirs() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + mark_as_git_repo(repo_dir.path()); + + let nested_dir = repo_dir.path().join("nested/inner"); + fs::create_dir_all(&nested_dir).unwrap(); + + let root_skill_path = write_skill_at( + &repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "root", + "dupe-skill", + "from root", + ); + let nested_skill_path = write_skill_at( + &repo_dir + .path() + .join("nested") + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "nested", + "dupe-skill", + "from nested", + ); + + let cfg = make_config_for_cwd(&codex_home, nested_dir).await; + let outcome = load_skills_for_test(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + let root_path = canonicalize_path(&root_skill_path).unwrap_or_else(|_| root_skill_path.clone()); + let nested_path = + canonicalize_path(&nested_skill_path).unwrap_or_else(|_| nested_skill_path.clone()); + let (first_path, second_path, first_description, second_description) = + if root_path <= nested_path { + (root_path, nested_path, "from root", "from nested") + } else { + (nested_path, root_path, "from nested", "from root") + }; + assert_eq!( + outcome.skills, + vec![ + SkillMetadata { + name: "dupe-skill".to_string(), + description: first_description.to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: first_path, + scope: SkillScope::Repo, + }, + SkillMetadata { + name: "dupe-skill".to_string(), + description: second_description.to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: second_path, + scope: SkillScope::Repo, + }, + ] + ); +} + +#[tokio::test] +async fn repo_skills_search_does_not_escape_repo_root() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let outer_dir = tempfile::tempdir().expect("tempdir"); + let repo_dir = outer_dir.path().join("repo"); + fs::create_dir_all(&repo_dir).unwrap(); + + let _skill_path = write_skill_at( + &outer_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "outer", + "outer-skill", + "from outer", + ); + mark_as_git_repo(&repo_dir); + + let cfg = make_config_for_cwd(&codex_home, repo_dir).await; + + let outcome = load_skills_for_test(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 0); +} + +#[tokio::test] +async fn loads_skills_when_cwd_is_file_in_repo() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + mark_as_git_repo(repo_dir.path()); + + let skill_path = write_skill_at( + &repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "repo", + "repo-skill", + "from repo", + ); + let file_path = repo_dir.path().join("some-file.txt"); + fs::write(&file_path, "contents").unwrap(); + + let cfg = make_config_for_cwd(&codex_home, file_path).await; + + let outcome = load_skills_for_test(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "repo-skill".to_string(), + description: "from repo".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::Repo, + }] + ); +} + +#[tokio::test] +async fn non_git_repo_skills_search_does_not_walk_parents() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let outer_dir = tempfile::tempdir().expect("tempdir"); + let nested_dir = outer_dir.path().join("nested/inner"); + fs::create_dir_all(&nested_dir).unwrap(); + + write_skill_at( + &outer_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "outer", + "outer-skill", + "from outer", + ); + + let cfg = make_config_for_cwd(&codex_home, nested_dir).await; + + let outcome = load_skills_for_test(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 0); +} + +#[tokio::test] +async fn loads_skills_from_system_cache_when_present() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let work_dir = tempfile::tempdir().expect("tempdir"); + + let skill_path = write_system_skill(&codex_home, "system", "system-skill", "from system"); + + let cfg = make_config_for_cwd(&codex_home, work_dir.path().to_path_buf()).await; + + let outcome = load_skills_for_test(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "system-skill".to_string(), + description: "from system".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: normalized(&skill_path), + scope: SkillScope::System, + }] + ); +} + +#[tokio::test] +async fn skill_roots_include_admin_with_lowest_priority() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cfg = make_config(&codex_home).await; + + let scopes: Vec = super::skill_roots(&cfg.config_layer_stack, &cfg.cwd, Vec::new()) + .into_iter() + .map(|root| root.scope) + .collect(); + let mut expected = vec![SkillScope::User, SkillScope::System]; + if home_dir().is_some() { + expected.insert(1, SkillScope::User); + } + expected.push(SkillScope::Admin); + assert_eq!(scopes, expected); +} diff --git a/codex-rs/core-skills/src/skills/manager.rs b/codex-rs/core-skills/src/skills/manager.rs new file mode 100644 index 0000000000..6810356a2b --- /dev/null +++ b/codex-rs/core-skills/src/skills/manager.rs @@ -0,0 +1,331 @@ +use std::collections::HashMap; +use std::collections::HashSet; +use std::path::Path; +use std::path::PathBuf; +use std::sync::Arc; +use std::sync::RwLock; + +use codex_protocol::protocol::Product; +use codex_protocol::protocol::SkillScope; +use codex_utils_absolute_path::AbsolutePathBuf; +use toml::Value as TomlValue; +use tracing::info; +use tracing::warn; + +use crate::config::Config; +use crate::config_loader::CloudRequirementsLoader; +use crate::config_loader::LoaderOverrides; +use crate::config_loader::load_config_layers_state; +use crate::plugins::PluginsManager; +use crate::skills::SkillLoadOutcome; +use crate::skills::build_implicit_skill_path_indexes; +use crate::skills::config_rules::SkillConfigRules; +use crate::skills::config_rules::resolve_disabled_skill_paths; +use crate::skills::config_rules::skill_config_rules_from_stack; +use crate::skills::loader::SkillRoot; +use crate::skills::loader::load_skills_from_roots; +use crate::skills::loader::skill_roots; +use crate::skills::system::install_system_skills; +use crate::skills::system::uninstall_system_skills; +use codex_config::SkillsConfig; + +pub struct SkillsManager { + codex_home: PathBuf, + plugins_manager: Arc, + restriction_product: Option, + cache_by_cwd: RwLock>, + cache_by_config: RwLock>, +} + +impl SkillsManager { + pub fn new( + codex_home: PathBuf, + plugins_manager: Arc, + bundled_skills_enabled: bool, + ) -> Self { + Self::new_with_restriction_product( + codex_home, + plugins_manager, + bundled_skills_enabled, + Some(Product::Codex), + ) + } + + pub fn new_with_restriction_product( + codex_home: PathBuf, + plugins_manager: Arc, + bundled_skills_enabled: bool, + restriction_product: Option, + ) -> Self { + let manager = Self { + codex_home, + plugins_manager, + restriction_product, + cache_by_cwd: RwLock::new(HashMap::new()), + cache_by_config: RwLock::new(HashMap::new()), + }; + if !bundled_skills_enabled { + // The loader caches bundled skills under `skills/.system`. Clearing that directory is + // best-effort cleanup; root selection still enforces the config even if removal fails. + uninstall_system_skills(&manager.codex_home); + } else if let Err(err) = install_system_skills(&manager.codex_home) { + tracing::error!("failed to install system skills: {err}"); + } + manager + } + + /// Load skills for an already-constructed [`Config`], avoiding any additional config-layer + /// loading. + /// + /// This path uses a cache keyed by the effective skill-relevant config state rather than just + /// cwd so role-local and session-local skill overrides cannot bleed across sessions that happen + /// to share a directory. + pub fn skills_for_config(&self, config: &Config) -> SkillLoadOutcome { + let roots = self.skill_roots_for_config(config); + let skill_config_rules = skill_config_rules_from_stack(&config.config_layer_stack); + let cache_key = config_skills_cache_key(&roots, &skill_config_rules); + if let Some(outcome) = self.cached_outcome_for_config(&cache_key) { + return outcome; + } + + let outcome = self.build_skill_outcome(roots, &skill_config_rules); + let mut cache = self + .cache_by_config + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner); + cache.insert(cache_key, outcome.clone()); + outcome + } + + pub(crate) fn skill_roots_for_config(&self, config: &Config) -> Vec { + let loaded_plugins = self.plugins_manager.plugins_for_config(config); + let mut roots = skill_roots( + &config.config_layer_stack, + &config.cwd, + loaded_plugins.effective_skill_roots(), + ); + if !config.bundled_skills_enabled() { + roots.retain(|root| root.scope != SkillScope::System); + } + roots + } + + pub async fn skills_for_cwd( + &self, + cwd: &Path, + config: &Config, + force_reload: bool, + ) -> SkillLoadOutcome { + if !force_reload && let Some(outcome) = self.cached_outcome_for_cwd(cwd) { + return outcome; + } + + self.skills_for_cwd_with_extra_user_roots(cwd, config, force_reload, &[]) + .await + } + + pub async fn skills_for_cwd_with_extra_user_roots( + &self, + cwd: &Path, + config: &Config, + force_reload: bool, + extra_user_roots: &[PathBuf], + ) -> SkillLoadOutcome { + if !force_reload && let Some(outcome) = self.cached_outcome_for_cwd(cwd) { + return outcome; + } + let normalized_extra_user_roots = normalize_extra_user_roots(extra_user_roots); + + let cwd_abs = match AbsolutePathBuf::try_from(cwd) { + Ok(cwd_abs) => cwd_abs, + Err(err) => { + return SkillLoadOutcome { + errors: vec![crate::skills::model::SkillError { + path: cwd.to_path_buf(), + message: err.to_string(), + }], + ..Default::default() + }; + } + }; + + let cli_overrides: Vec<(String, TomlValue)> = Vec::new(); + let config_layer_stack = match load_config_layers_state( + &self.codex_home, + Some(cwd_abs), + &cli_overrides, + LoaderOverrides::default(), + CloudRequirementsLoader::default(), + ) + .await + { + Ok(config_layer_stack) => config_layer_stack, + Err(err) => { + return SkillLoadOutcome { + errors: vec![crate::skills::model::SkillError { + path: cwd.to_path_buf(), + message: err.to_string(), + }], + ..Default::default() + }; + } + }; + + let loaded_plugins = self + .plugins_manager + .plugins_for_config_with_force_reload(config, force_reload); + let mut roots = skill_roots( + &config_layer_stack, + cwd, + loaded_plugins.effective_skill_roots(), + ); + if !bundled_skills_enabled_from_stack(&config_layer_stack) { + roots.retain(|root| root.scope != SkillScope::System); + } + roots.extend( + normalized_extra_user_roots + .iter() + .cloned() + .map(|path| SkillRoot { + path, + scope: SkillScope::User, + }), + ); + let skill_config_rules = skill_config_rules_from_stack(&config_layer_stack); + let outcome = self.build_skill_outcome(roots, &skill_config_rules); + let mut cache = self + .cache_by_cwd + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner); + cache.insert(cwd.to_path_buf(), outcome.clone()); + outcome + } + + fn build_skill_outcome( + &self, + roots: Vec, + skill_config_rules: &SkillConfigRules, + ) -> SkillLoadOutcome { + let outcome = crate::skills::filter_skill_load_outcome_for_product( + load_skills_from_roots(roots), + self.restriction_product, + ); + let disabled_paths = resolve_disabled_skill_paths(&outcome.skills, skill_config_rules); + finalize_skill_outcome(outcome, disabled_paths) + } + + pub fn clear_cache(&self) { + let cleared_cwd = { + let mut cache = self + .cache_by_cwd + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let cleared = cache.len(); + cache.clear(); + cleared + }; + let cleared_config = { + let mut cache = self + .cache_by_config + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let cleared = cache.len(); + cache.clear(); + cleared + }; + let cleared = cleared_cwd + cleared_config; + info!("skills cache cleared ({cleared} entries)"); + } + + fn cached_outcome_for_cwd(&self, cwd: &Path) -> Option { + match self.cache_by_cwd.read() { + Ok(cache) => cache.get(cwd).cloned(), + Err(err) => err.into_inner().get(cwd).cloned(), + } + } + + fn cached_outcome_for_config( + &self, + cache_key: &ConfigSkillsCacheKey, + ) -> Option { + match self.cache_by_config.read() { + Ok(cache) => cache.get(cache_key).cloned(), + Err(err) => err.into_inner().get(cache_key).cloned(), + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +struct ConfigSkillsCacheKey { + roots: Vec<(PathBuf, u8)>, + skill_config_rules: SkillConfigRules, +} + +pub(crate) fn bundled_skills_enabled_from_stack( + config_layer_stack: &codex_config::ConfigLayerStack, +) -> bool { + let effective_config = config_layer_stack.effective_config(); + let Some(skills_value) = effective_config + .as_table() + .and_then(|table| table.get("skills")) + else { + return true; + }; + + let skills: SkillsConfig = match skills_value.clone().try_into() { + Ok(skills) => skills, + Err(err) => { + warn!("invalid skills config: {err}"); + return true; + } + }; + + skills.bundled.unwrap_or_default().enabled +} + +fn config_skills_cache_key( + roots: &[SkillRoot], + skill_config_rules: &SkillConfigRules, +) -> ConfigSkillsCacheKey { + ConfigSkillsCacheKey { + roots: roots + .iter() + .map(|root| { + let scope_rank = match root.scope { + SkillScope::Repo => 0, + SkillScope::User => 1, + SkillScope::System => 2, + SkillScope::Admin => 3, + }; + (root.path.clone(), scope_rank) + }) + .collect(), + skill_config_rules: skill_config_rules.clone(), + } +} + +fn finalize_skill_outcome( + mut outcome: SkillLoadOutcome, + disabled_paths: HashSet, +) -> SkillLoadOutcome { + outcome.disabled_paths = disabled_paths; + let (by_scripts_dir, by_doc_path) = + build_implicit_skill_path_indexes(outcome.allowed_skills_for_implicit_invocation()); + outcome.implicit_skills_by_scripts_dir = Arc::new(by_scripts_dir); + outcome.implicit_skills_by_doc_path = Arc::new(by_doc_path); + outcome +} + +fn normalize_extra_user_roots(extra_user_roots: &[PathBuf]) -> Vec { + let mut normalized: Vec = extra_user_roots + .iter() + .map(|path| dunce::canonicalize(path).unwrap_or_else(|_| path.clone())) + .collect(); + normalized.sort_unstable(); + normalized.dedup(); + normalized +} + +#[cfg(test)] +#[path = "manager_tests.rs"] +mod tests; diff --git a/codex-rs/core-skills/src/skills/manager_tests.rs b/codex-rs/core-skills/src/skills/manager_tests.rs new file mode 100644 index 0000000000..1e99ee4ffc --- /dev/null +++ b/codex-rs/core-skills/src/skills/manager_tests.rs @@ -0,0 +1,634 @@ +use super::*; +use crate::config::ConfigBuilder; +use crate::config::ConfigOverrides; +use crate::config_loader::ConfigLayerEntry; +use crate::config_loader::ConfigRequirementsToml; +use crate::plugins::PluginsManager; +use crate::skills::SkillMetadata; +use crate::skills::config_rules::resolve_disabled_skill_paths; +use crate::skills::config_rules::skill_config_rules_from_stack; +use codex_app_server_protocol::ConfigLayerSource; +use codex_config::ConfigLayerStack; +use pretty_assertions::assert_eq; +use std::fs; +use std::path::PathBuf; +use tempfile::TempDir; + +fn write_user_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) { + let skill_dir = codex_home.path().join("skills").join(dir); + fs::create_dir_all(&skill_dir).unwrap(); + let content = format!("---\nname: {name}\ndescription: {description}\n---\n\n# Body\n"); + fs::write(skill_dir.join("SKILL.md"), content).unwrap(); +} + +fn write_plugin_skill( + codex_home: &TempDir, + marketplace: &str, + plugin_name: &str, + dir: &str, + name: &str, + description: &str, +) -> PathBuf { + let plugin_root = codex_home + .path() + .join("plugins/cache") + .join(marketplace) + .join(plugin_name) + .join("local"); + let skill_dir = plugin_root.join("skills").join(dir); + fs::create_dir_all(plugin_root.join(".codex-plugin")).unwrap(); + fs::create_dir_all(&skill_dir).unwrap(); + fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + format!(r#"{{"name":"{plugin_name}"}}"#), + ) + .unwrap(); + let content = format!("---\nname: {name}\ndescription: {description}\n---\n\n# Body\n"); + let skill_path = skill_dir.join("SKILL.md"); + fs::write(&skill_path, content).unwrap(); + skill_path +} + +fn test_skill(name: &str, path: PathBuf) -> SkillMetadata { + SkillMetadata { + name: name.to_string(), + description: "test".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: path, + scope: SkillScope::User, + } +} + +#[test] +fn new_with_disabled_bundled_skills_removes_stale_cached_system_skills() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let stale_system_skill_dir = codex_home.path().join("skills/.system/stale-skill"); + fs::create_dir_all(&stale_system_skill_dir).expect("create stale system skill dir"); + fs::write(stale_system_skill_dir.join("SKILL.md"), "# stale\n") + .expect("write stale system skill"); + + let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf())); + let _skills_manager = + SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager, false); + + assert!( + !codex_home.path().join("skills/.system").exists(), + "expected disabling system skills to remove stale cached bundled skills" + ); +} + +#[tokio::test] +async fn skills_for_config_reuses_cache_for_same_effective_config() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + + let cfg = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }) + .build() + .await + .expect("defaults for test should always succeed"); + + let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf())); + let skills_manager = SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager, true); + + write_user_skill(&codex_home, "a", "skill-a", "from a"); + let outcome1 = skills_manager.skills_for_config(&cfg); + assert!( + outcome1.skills.iter().any(|s| s.name == "skill-a"), + "expected skill-a to be discovered" + ); + + // Write a new skill after the first call; the second call should reuse the config-aware cache + // entry because the effective skill config is unchanged. + write_user_skill(&codex_home, "b", "skill-b", "from b"); + let outcome2 = skills_manager.skills_for_config(&cfg); + assert_eq!(outcome2.errors, outcome1.errors); + assert_eq!(outcome2.skills, outcome1.skills); +} + +#[tokio::test] +async fn skills_for_config_disables_plugin_skills_by_name() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let skill_path = write_plugin_skill( + &codex_home, + "test", + "sample", + "sample-search", + "sample-search", + "search sample data", + ); + fs::write( + codex_home.path().join(crate::config::CONFIG_TOML_FILE), + r#"[features] +plugins = true + +[[skills.config]] +name = "sample:sample-search" +enabled = false + +[plugins."sample@test"] +enabled = true +"#, + ) + .expect("write config"); + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }) + .build() + .await + .expect("load config"); + + let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf())); + let skills_manager = SkillsManager::new( + codex_home.path().to_path_buf(), + plugins_manager, + config.bundled_skills_enabled(), + ); + + let outcome = skills_manager.skills_for_config(&config); + let skill = outcome + .skills + .iter() + .find(|skill| skill.name == "sample:sample-search") + .expect("plugin skill should load"); + let skill_path = dunce::canonicalize(skill_path).expect("skill path should canonicalize"); + + assert_eq!(skill.path_to_skills_md, skill_path); + assert!(outcome.disabled_paths.contains(&skill.path_to_skills_md)); + assert!( + !outcome + .allowed_skills_for_implicit_invocation() + .iter() + .any(|allowed_skill| allowed_skill.path_to_skills_md == skill.path_to_skills_md) + ); +} + +#[tokio::test] +async fn skills_for_cwd_reuses_cached_entry_even_when_entry_has_extra_roots() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let extra_root = tempfile::tempdir().expect("tempdir"); + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }) + .build() + .await + .expect("defaults for test should always succeed"); + + let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf())); + let skills_manager = SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager, true); + let _ = skills_manager.skills_for_config(&config); + + write_user_skill(&extra_root, "x", "extra-skill", "from extra root"); + let extra_root_path = extra_root.path().to_path_buf(); + let outcome_with_extra = skills_manager + .skills_for_cwd_with_extra_user_roots( + cwd.path(), + &config, + true, + std::slice::from_ref(&extra_root_path), + ) + .await; + assert!( + outcome_with_extra + .skills + .iter() + .any(|skill| skill.name == "extra-skill") + ); + assert!( + outcome_with_extra + .skills + .iter() + .any(|skill| skill.scope == SkillScope::System) + ); + + // The cwd-only API returns the current cached entry for this cwd, even when that entry + // was produced with extra roots. + let outcome_without_extra = skills_manager + .skills_for_cwd(cwd.path(), &config, false) + .await; + assert_eq!(outcome_without_extra.skills, outcome_with_extra.skills); + assert_eq!(outcome_without_extra.errors, outcome_with_extra.errors); +} + +#[tokio::test] +async fn skills_for_config_excludes_bundled_skills_when_disabled_in_config() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let bundled_skill_dir = codex_home.path().join("skills/.system/bundled-skill"); + fs::create_dir_all(&bundled_skill_dir).expect("create bundled skill dir"); + fs::write( + bundled_skill_dir.join("SKILL.md"), + "---\nname: bundled-skill\ndescription: from bundled root\n---\n\n# Body\n", + ) + .expect("write bundled skill"); + + fs::write( + codex_home.path().join(crate::config::CONFIG_TOML_FILE), + "[skills.bundled]\nenabled = false\n", + ) + .expect("write config"); + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }) + .build() + .await + .expect("load config"); + + let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf())); + let skills_manager = SkillsManager::new( + codex_home.path().to_path_buf(), + plugins_manager, + config.bundled_skills_enabled(), + ); + + // Recreate the cached bundled skill after startup cleanup so this assertion exercises + // root selection rather than relying on directory removal succeeding. + fs::create_dir_all(&bundled_skill_dir).expect("recreate bundled skill dir"); + fs::write( + bundled_skill_dir.join("SKILL.md"), + "---\nname: bundled-skill\ndescription: from bundled root\n---\n\n# Body\n", + ) + .expect("rewrite bundled skill"); + + let outcome = skills_manager.skills_for_config(&config); + assert!( + outcome + .skills + .iter() + .all(|skill| skill.name != "bundled-skill") + ); + assert!( + outcome + .skills + .iter() + .all(|skill| skill.scope != SkillScope::System) + ); +} + +#[tokio::test] +async fn skills_for_cwd_with_extra_roots_only_refreshes_on_force_reload() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let extra_root_a = tempfile::tempdir().expect("tempdir"); + let extra_root_b = tempfile::tempdir().expect("tempdir"); + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }) + .build() + .await + .expect("defaults for test should always succeed"); + + let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf())); + let skills_manager = SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager, true); + let _ = skills_manager.skills_for_config(&config); + + write_user_skill(&extra_root_a, "x", "extra-skill-a", "from extra root a"); + write_user_skill(&extra_root_b, "x", "extra-skill-b", "from extra root b"); + + let extra_root_a_path = extra_root_a.path().to_path_buf(); + let outcome_a = skills_manager + .skills_for_cwd_with_extra_user_roots( + cwd.path(), + &config, + true, + std::slice::from_ref(&extra_root_a_path), + ) + .await; + assert!( + outcome_a + .skills + .iter() + .any(|skill| skill.name == "extra-skill-a") + ); + assert!( + outcome_a + .skills + .iter() + .all(|skill| skill.name != "extra-skill-b") + ); + + let extra_root_b_path = extra_root_b.path().to_path_buf(); + let outcome_b = skills_manager + .skills_for_cwd_with_extra_user_roots( + cwd.path(), + &config, + false, + std::slice::from_ref(&extra_root_b_path), + ) + .await; + assert!( + outcome_b + .skills + .iter() + .any(|skill| skill.name == "extra-skill-a") + ); + assert!( + outcome_b + .skills + .iter() + .all(|skill| skill.name != "extra-skill-b") + ); + + let outcome_reloaded = skills_manager + .skills_for_cwd_with_extra_user_roots( + cwd.path(), + &config, + true, + std::slice::from_ref(&extra_root_b_path), + ) + .await; + assert!( + outcome_reloaded + .skills + .iter() + .any(|skill| skill.name == "extra-skill-b") + ); + assert!( + outcome_reloaded + .skills + .iter() + .all(|skill| skill.name != "extra-skill-a") + ); +} + +#[test] +fn normalize_extra_user_roots_is_stable_for_equivalent_inputs() { + let a = PathBuf::from("/tmp/a"); + let b = PathBuf::from("/tmp/b"); + + let first = normalize_extra_user_roots(&[a.clone(), b.clone(), a.clone()]); + let second = normalize_extra_user_roots(&[b, a]); + + assert_eq!(first, second); +} + +#[cfg_attr(windows, ignore)] +#[test] +fn disabled_paths_for_skills_allows_session_flags_to_override_user_layer() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_path = tempdir.path().join("skills").join("demo").join("SKILL.md"); + let skill = test_skill("demo-skill", skill_path.clone()); + let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml")) + .expect("user config path should be absolute"); + let user_layer = ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + toml::from_str(&format!( + r#"[[skills.config]] +path = "{}" +enabled = false +"#, + skill_path.display() + )) + .expect("user layer toml"), + ); + let session_layer = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::from_str(&format!( + r#"[[skills.config]] +path = "{}" +enabled = true +"#, + skill_path.display() + )) + .expect("session layer toml"), + ); + let stack = ConfigLayerStack::new( + vec![user_layer, session_layer], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + + let skill_config_rules = skill_config_rules_from_stack(&stack); + assert_eq!( + resolve_disabled_skill_paths(&[skill], &skill_config_rules), + HashSet::new() + ); +} + +#[cfg_attr(windows, ignore)] +#[test] +fn disabled_paths_for_skills_allows_session_flags_to_disable_user_enabled_skill() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_path = tempdir.path().join("skills").join("demo").join("SKILL.md"); + let skill = test_skill("demo-skill", skill_path.clone()); + let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml")) + .expect("user config path should be absolute"); + let user_layer = ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + toml::from_str(&format!( + r#"[[skills.config]] +path = "{}" +enabled = true +"#, + skill_path.display() + )) + .expect("user layer toml"), + ); + let session_layer = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::from_str(&format!( + r#"[[skills.config]] +path = "{}" +enabled = false +"#, + skill_path.display() + )) + .expect("session layer toml"), + ); + let stack = ConfigLayerStack::new( + vec![user_layer, session_layer], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + + let skill_config_rules = skill_config_rules_from_stack(&stack); + assert_eq!( + resolve_disabled_skill_paths(&[skill], &skill_config_rules), + HashSet::from([skill_path]) + ); +} + +#[cfg_attr(windows, ignore)] +#[test] +fn disabled_paths_for_skills_disables_matching_name_selectors() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_path = tempdir.path().join("skills").join("demo").join("SKILL.md"); + let skill = test_skill("github:yeet", skill_path.clone()); + let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml")) + .expect("user config path should be absolute"); + let user_layer = ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + toml::from_str( + r#"[[skills.config]] +name = "github:yeet" +enabled = false +"#, + ) + .expect("user layer toml"), + ); + let stack = ConfigLayerStack::new( + vec![user_layer], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + + let skill_config_rules = skill_config_rules_from_stack(&stack); + assert_eq!( + resolve_disabled_skill_paths(&[skill], &skill_config_rules), + HashSet::from([skill_path]) + ); +} + +#[cfg_attr(windows, ignore)] +#[test] +fn disabled_paths_for_skills_allows_name_selector_to_override_path_selector() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_path = tempdir.path().join("skills").join("demo").join("SKILL.md"); + let skill = test_skill("github:yeet", skill_path.clone()); + let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml")) + .expect("user config path should be absolute"); + let user_layer = ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + toml::from_str(&format!( + r#"[[skills.config]] +path = "{}" +enabled = false +"#, + skill_path.display() + )) + .expect("user layer toml"), + ); + let session_layer = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::from_str( + r#"[[skills.config]] +name = "github:yeet" +enabled = true +"#, + ) + .expect("session layer toml"), + ); + let stack = ConfigLayerStack::new( + vec![user_layer, session_layer], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + + let skill_config_rules = skill_config_rules_from_stack(&stack); + assert_eq!( + resolve_disabled_skill_paths(&[skill], &skill_config_rules), + HashSet::new() + ); +} + +#[cfg_attr(windows, ignore)] +#[tokio::test] +async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let skill_dir = codex_home.path().join("skills").join("demo"); + fs::create_dir_all(&skill_dir).expect("create skill dir"); + let skill_path = skill_dir.join("SKILL.md"); + fs::write( + &skill_path, + "---\nname: demo-skill\ndescription: demo description\n---\n\n# Body\n", + ) + .expect("write skill"); + fs::write( + codex_home.path().join(crate::config::CONFIG_TOML_FILE), + format!( + r#"[[skills.config]] +path = "{}" +enabled = false +"#, + skill_path.display() + ), + ) + .expect("write config"); + + let parent_config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }) + .build() + .await + .expect("load parent config"); + let role_path = codex_home.path().join("enable-role.toml"); + fs::write( + &role_path, + format!( + r#"[[skills.config]] +path = "{}" +enabled = true +"#, + skill_path.display() + ), + ) + .expect("write role config"); + let mut child_config = parent_config.clone(); + child_config.agent_roles.insert( + "custom".to_string(), + crate::config::AgentRoleConfig { + description: None, + config_file: Some(role_path), + nickname_candidates: None, + }, + ); + crate::agent::role::apply_role_to_config(&mut child_config, Some("custom")) + .await + .expect("custom role should apply"); + + let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf())); + let skills_manager = SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager, true); + + let parent_outcome = skills_manager + .skills_for_cwd(cwd.path(), &parent_config, true) + .await; + let parent_skill = parent_outcome + .skills + .iter() + .find(|skill| skill.name == "demo-skill") + .expect("demo skill should be discovered"); + assert_eq!(parent_outcome.is_skill_enabled(parent_skill), false); + + let child_outcome = skills_manager.skills_for_config(&child_config); + let child_skill = child_outcome + .skills + .iter() + .find(|skill| skill.name == "demo-skill") + .expect("demo skill should be discovered"); + assert_eq!(child_outcome.is_skill_enabled(child_skill), true); +} diff --git a/codex-rs/core-skills/src/skills/mod.rs b/codex-rs/core-skills/src/skills/mod.rs new file mode 100644 index 0000000000..870b398cae --- /dev/null +++ b/codex-rs/core-skills/src/skills/mod.rs @@ -0,0 +1,19 @@ +pub(crate) mod config_rules; +mod env_var_dependencies; +pub mod injection; +pub(crate) mod invocation_utils; +pub mod loader; +pub mod manager; +pub mod model; +pub mod remote; +pub mod render; +pub mod system; + +pub(crate) use invocation_utils::build_implicit_skill_path_indexes; +pub use manager::SkillsManager; +pub use model::SkillError; +pub use model::SkillLoadOutcome; +pub use model::SkillMetadata; +pub use model::SkillPolicy; +pub use model::filter_skill_load_outcome_for_product; +pub use render::render_skills_section; diff --git a/codex-rs/core-skills/src/skills/model.rs b/codex-rs/core-skills/src/skills/model.rs new file mode 100644 index 0000000000..d47904b9c7 --- /dev/null +++ b/codex-rs/core-skills/src/skills/model.rs @@ -0,0 +1,158 @@ +use std::collections::HashMap; +use std::collections::HashSet; +use std::path::PathBuf; +use std::sync::Arc; + +use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::Product; +use codex_protocol::protocol::SkillScope; +use serde::Deserialize; + +#[derive(Debug, Clone, Default, Deserialize, PartialEq, Eq)] +pub struct SkillManagedNetworkOverride { + pub allowed_domains: Option>, + pub denied_domains: Option>, +} + +impl SkillManagedNetworkOverride { + pub fn has_domain_overrides(&self) -> bool { + self.allowed_domains.is_some() || self.denied_domains.is_some() + } +} + +#[derive(Debug, Clone, PartialEq)] +pub struct SkillMetadata { + pub name: String, + pub description: String, + pub short_description: Option, + pub interface: Option, + pub dependencies: Option, + pub policy: Option, + pub permission_profile: Option, + pub managed_network_override: Option, + /// Path to the SKILLS.md file that declares this skill. + pub path_to_skills_md: PathBuf, + pub scope: SkillScope, +} + +impl SkillMetadata { + fn allow_implicit_invocation(&self) -> bool { + self.policy + .as_ref() + .and_then(|policy| policy.allow_implicit_invocation) + .unwrap_or(true) + } + + pub fn matches_product_restriction_for_product( + &self, + restriction_product: Option, + ) -> bool { + match &self.policy { + Some(policy) => { + policy.products.is_empty() + || restriction_product.is_some_and(|product| { + product.matches_product_restriction(&policy.products) + }) + } + None => true, + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct SkillPolicy { + pub allow_implicit_invocation: Option, + // TODO: Enforce product gating in Codex skill selection/injection instead of only parsing and + // storing this metadata. + pub products: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillInterface { + pub display_name: Option, + pub short_description: Option, + pub icon_small: Option, + pub icon_large: Option, + pub brand_color: Option, + pub default_prompt: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillDependencies { + pub tools: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillToolDependency { + pub r#type: String, + pub value: String, + pub description: Option, + pub transport: Option, + pub command: Option, + pub url: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillError { + pub path: PathBuf, + pub message: String, +} + +#[derive(Debug, Clone, Default)] +pub struct SkillLoadOutcome { + pub skills: Vec, + pub errors: Vec, + pub disabled_paths: HashSet, + pub(crate) implicit_skills_by_scripts_dir: Arc>, + pub(crate) implicit_skills_by_doc_path: Arc>, +} + +impl SkillLoadOutcome { + pub fn is_skill_enabled(&self, skill: &SkillMetadata) -> bool { + !self.disabled_paths.contains(&skill.path_to_skills_md) + } + + pub fn is_skill_allowed_for_implicit_invocation(&self, skill: &SkillMetadata) -> bool { + self.is_skill_enabled(skill) && skill.allow_implicit_invocation() + } + + pub fn allowed_skills_for_implicit_invocation(&self) -> Vec { + self.skills + .iter() + .filter(|skill| self.is_skill_allowed_for_implicit_invocation(skill)) + .cloned() + .collect() + } + + pub fn skills_with_enabled(&self) -> impl Iterator { + self.skills + .iter() + .map(|skill| (skill, self.is_skill_enabled(skill))) + } +} + +pub fn filter_skill_load_outcome_for_product( + mut outcome: SkillLoadOutcome, + restriction_product: Option, +) -> SkillLoadOutcome { + outcome + .skills + .retain(|skill| skill.matches_product_restriction_for_product(restriction_product)); + outcome.implicit_skills_by_scripts_dir = Arc::new( + outcome + .implicit_skills_by_scripts_dir + .iter() + .filter(|(_, skill)| skill.matches_product_restriction_for_product(restriction_product)) + .map(|(path, skill)| (path.clone(), skill.clone())) + .collect(), + ); + outcome.implicit_skills_by_doc_path = Arc::new( + outcome + .implicit_skills_by_doc_path + .iter() + .filter(|(_, skill)| skill.matches_product_restriction_for_product(restriction_product)) + .map(|(path, skill)| (path.clone(), skill.clone())) + .collect(), + ); + outcome +} diff --git a/codex-rs/core-skills/src/skills/remote.rs b/codex-rs/core-skills/src/skills/remote.rs new file mode 100644 index 0000000000..165c450635 --- /dev/null +++ b/codex-rs/core-skills/src/skills/remote.rs @@ -0,0 +1,270 @@ +use anyhow::Context; +use anyhow::Result; +use serde::Deserialize; +use std::path::Component; +use std::path::Path; +use std::path::PathBuf; +use std::time::Duration; + +use crate::auth::CodexAuth; +use crate::config::Config; +use crate::default_client::build_reqwest_client; + +const REMOTE_SKILLS_API_TIMEOUT: Duration = Duration::from_secs(30); + +// Low-level client for the remote skill API. This is intentionally kept around for +// future wiring, but it is not used yet by any active product surface. + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RemoteSkillScope { + WorkspaceShared, + AllShared, + Personal, + Example, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RemoteSkillProductSurface { + Chatgpt, + Codex, + Api, + Atlas, +} + +fn as_query_scope(scope: RemoteSkillScope) -> Option<&'static str> { + match scope { + RemoteSkillScope::WorkspaceShared => Some("workspace-shared"), + RemoteSkillScope::AllShared => Some("all-shared"), + RemoteSkillScope::Personal => Some("personal"), + RemoteSkillScope::Example => Some("example"), + } +} + +fn as_query_product_surface(product_surface: RemoteSkillProductSurface) -> &'static str { + match product_surface { + RemoteSkillProductSurface::Chatgpt => "chatgpt", + RemoteSkillProductSurface::Codex => "codex", + RemoteSkillProductSurface::Api => "api", + RemoteSkillProductSurface::Atlas => "atlas", + } +} + +fn ensure_chatgpt_auth(auth: Option<&CodexAuth>) -> Result<&CodexAuth> { + let Some(auth) = auth else { + anyhow::bail!("chatgpt authentication required for remote skill scopes"); + }; + if !auth.is_chatgpt_auth() { + anyhow::bail!( + "chatgpt authentication required for remote skill scopes; api key auth is not supported" + ); + } + Ok(auth) +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RemoteSkillSummary { + pub id: String, + pub name: String, + pub description: String, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RemoteSkillDownloadResult { + pub id: String, + pub path: PathBuf, +} + +#[derive(Debug, Deserialize)] +struct RemoteSkillsResponse { + #[serde(rename = "hazelnuts")] + skills: Vec, +} + +#[derive(Debug, Deserialize)] +struct RemoteSkill { + id: String, + name: String, + description: String, +} + +pub async fn list_remote_skills( + config: &Config, + auth: Option<&CodexAuth>, + scope: RemoteSkillScope, + product_surface: RemoteSkillProductSurface, + enabled: Option, +) -> Result> { + let base_url = config.chatgpt_base_url.trim_end_matches('/'); + let auth = ensure_chatgpt_auth(auth)?; + + let url = format!("{base_url}/hazelnuts"); + let product_surface = as_query_product_surface(product_surface); + let mut query_params = vec![("product_surface", product_surface)]; + if let Some(scope) = as_query_scope(scope) { + query_params.push(("scope", scope)); + } + if let Some(enabled) = enabled { + let enabled = if enabled { "true" } else { "false" }; + query_params.push(("enabled", enabled)); + } + + let client = build_reqwest_client(); + let mut request = client + .get(&url) + .timeout(REMOTE_SKILLS_API_TIMEOUT) + .query(&query_params); + let token = auth + .get_token() + .context("Failed to read auth token for remote skills")?; + request = request.bearer_auth(token); + if let Some(account_id) = auth.get_account_id() { + request = request.header("chatgpt-account-id", account_id); + } + let response = request + .send() + .await + .with_context(|| format!("Failed to send request to {url}"))?; + + let status = response.status(); + let body = response.text().await.unwrap_or_default(); + if !status.is_success() { + anyhow::bail!("Request failed with status {status} from {url}: {body}"); + } + + let parsed: RemoteSkillsResponse = + serde_json::from_str(&body).context("Failed to parse skills response")?; + + Ok(parsed + .skills + .into_iter() + .map(|skill| RemoteSkillSummary { + id: skill.id, + name: skill.name, + description: skill.description, + }) + .collect()) +} + +pub async fn export_remote_skill( + config: &Config, + auth: Option<&CodexAuth>, + skill_id: &str, +) -> Result { + let auth = ensure_chatgpt_auth(auth)?; + + let client = build_reqwest_client(); + let base_url = config.chatgpt_base_url.trim_end_matches('/'); + let url = format!("{base_url}/hazelnuts/{skill_id}/export"); + let mut request = client.get(&url).timeout(REMOTE_SKILLS_API_TIMEOUT); + + let token = auth + .get_token() + .context("Failed to read auth token for remote skills")?; + request = request.bearer_auth(token); + if let Some(account_id) = auth.get_account_id() { + request = request.header("chatgpt-account-id", account_id); + } + + let response = request + .send() + .await + .with_context(|| format!("Failed to send download request to {url}"))?; + + let status = response.status(); + let body = response.bytes().await.context("Failed to read download")?; + if !status.is_success() { + let body_text = String::from_utf8_lossy(&body); + anyhow::bail!("Download failed with status {status} from {url}: {body_text}"); + } + + if !is_zip_payload(&body) { + anyhow::bail!("Downloaded remote skill payload is not a zip archive"); + } + + let output_dir = config.codex_home.join("skills").join(skill_id); + tokio::fs::create_dir_all(&output_dir) + .await + .context("Failed to create downloaded skills directory")?; + + let zip_bytes = body.to_vec(); + let output_dir_clone = output_dir.clone(); + let prefix_candidates = vec![skill_id.to_string()]; + tokio::task::spawn_blocking(move || { + extract_zip_to_dir(zip_bytes, &output_dir_clone, &prefix_candidates) + }) + .await + .context("Zip extraction task failed")??; + + Ok(RemoteSkillDownloadResult { + id: skill_id.to_string(), + path: output_dir, + }) +} + +fn safe_join(base: &Path, name: &str) -> Result { + let path = Path::new(name); + for component in path.components() { + match component { + Component::Normal(_) => {} + _ => { + anyhow::bail!("Invalid file path in remote skill payload: {name}"); + } + } + } + Ok(base.join(path)) +} + +fn is_zip_payload(bytes: &[u8]) -> bool { + bytes.starts_with(b"PK\x03\x04") + || bytes.starts_with(b"PK\x05\x06") + || bytes.starts_with(b"PK\x07\x08") +} + +fn extract_zip_to_dir( + bytes: Vec, + output_dir: &Path, + prefix_candidates: &[String], +) -> Result<()> { + let cursor = std::io::Cursor::new(bytes); + let mut archive = zip::ZipArchive::new(cursor).context("Failed to open zip archive")?; + for i in 0..archive.len() { + let mut file = archive.by_index(i).context("Failed to read zip entry")?; + if file.is_dir() { + continue; + } + let raw_name = file.name().to_string(); + let normalized = normalize_zip_name(&raw_name, prefix_candidates); + let Some(normalized) = normalized else { + continue; + }; + let file_path = safe_join(output_dir, &normalized)?; + if let Some(parent) = file_path.parent() { + std::fs::create_dir_all(parent) + .with_context(|| format!("Failed to create parent dir for {normalized}"))?; + } + let mut out = std::fs::File::create(&file_path) + .with_context(|| format!("Failed to create file {normalized}"))?; + std::io::copy(&mut file, &mut out) + .with_context(|| format!("Failed to write skill file {normalized}"))?; + } + Ok(()) +} + +fn normalize_zip_name(name: &str, prefix_candidates: &[String]) -> Option { + let mut trimmed = name.trim_start_matches("./"); + for prefix in prefix_candidates { + if prefix.is_empty() { + continue; + } + let prefix = format!("{prefix}/"); + if let Some(rest) = trimmed.strip_prefix(&prefix) { + trimmed = rest; + break; + } + } + if trimmed.is_empty() { + None + } else { + Some(trimmed.to_string()) + } +} diff --git a/codex-rs/core-skills/src/skills/render.rs b/codex-rs/core-skills/src/skills/render.rs new file mode 100644 index 0000000000..797d53db21 --- /dev/null +++ b/codex-rs/core-skills/src/skills/render.rs @@ -0,0 +1,48 @@ +use crate::skills::model::SkillMetadata; +use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG; +use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; + +pub fn render_skills_section(skills: &[SkillMetadata]) -> Option { + if skills.is_empty() { + return None; + } + + let mut lines: Vec = Vec::new(); + lines.push("## Skills".to_string()); + lines.push("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 file path so you can open the source for full instructions when using a specific skill.".to_string()); + lines.push("### Available skills".to_string()); + + for skill in skills { + let path_str = skill.path_to_skills_md.to_string_lossy().replace('\\', "/"); + let name = skill.name.as_str(); + let description = skill.description.as_str(); + lines.push(format!("- {name}: {description} (file: {path_str})")); + } + + lines.push("### How to use skills".to_string()); + lines.push( + r###"- Discovery: The list above is the skills available in this session (name + description + file path). Skill bodies live on disk at the listed paths. +- 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 the path can't be read, say so briefly and continue with the best fallback. +- How to use a skill (progressive disclosure): + 1) After deciding to use a skill, open its `SKILL.md`. Read only enough to follow the workflow. + 2) When `SKILL.md` references relative paths (e.g., `scripts/foo.py`), resolve them relative to the skill directory listed above first, and only consider other paths if needed. + 3) If `SKILL.md` points to extra folders such as `references/`, load only the specific files needed for the request; don't bulk-load everything. + 4) If `scripts/` exist, prefer running or patching them instead of retyping large code blocks. + 5) If `assets/` or templates exist, reuse them instead of recreating from scratch. +- Coordination and sequencing: + - If multiple skills apply, choose the minimal set that covers the request and state the order you'll use them. + - Announce which skill(s) you're using and why (one short line). If you skip an obvious skill, say why. +- Context hygiene: + - Keep context small: summarize long sections instead of pasting them; only load extra files when needed. + - Avoid deep reference-chasing: prefer opening only files directly linked from `SKILL.md` unless you're blocked. + - When variants exist (frameworks, providers, domains), pick only the relevant reference file(s) and note that choice. +- Safety and fallback: If a skill can't be applied cleanly (missing files, unclear instructions), state the issue, pick the next-best approach, and continue."### + .to_string(), + ); + + let body = lines.join("\n"); + Some(format!( + "{SKILLS_INSTRUCTIONS_OPEN_TAG}\n{body}\n{SKILLS_INSTRUCTIONS_CLOSE_TAG}" + )) +} diff --git a/codex-rs/core-skills/src/skills/system.rs b/codex-rs/core-skills/src/skills/system.rs new file mode 100644 index 0000000000..394fe00c3b --- /dev/null +++ b/codex-rs/core-skills/src/skills/system.rs @@ -0,0 +1,9 @@ +pub(crate) use codex_skills::install_system_skills; +pub(crate) use codex_skills::system_cache_root_dir; + +use std::path::Path; + +pub(crate) fn uninstall_system_skills(codex_home: &Path) { + let system_skills_dir = system_cache_root_dir(codex_home); + let _ = std::fs::remove_dir_all(&system_skills_dir); +} diff --git a/codex-rs/plugin/Cargo.toml b/codex-rs/plugin/Cargo.toml new file mode 100644 index 0000000000..45c9a37d3f --- /dev/null +++ b/codex-rs/plugin/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition.workspace = true +license.workspace = true +name = "codex-plugin" +version.workspace = true + +[lib] +doctest = false +name = "codex_plugin" +path = "src/lib.rs" + +[lints] +workspace = true + +[dependencies] +thiserror = { workspace = true } diff --git a/codex-rs/plugin/src/lib.rs b/codex-rs/plugin/src/lib.rs new file mode 100644 index 0000000000..51e98ceba4 --- /dev/null +++ b/codex-rs/plugin/src/lib.rs @@ -0,0 +1,46 @@ +//! Shared plugin identifiers and telemetry-facing summaries. + +mod plugin_id; + +pub use plugin_id::PluginId; +pub use plugin_id::PluginIdError; +pub use plugin_id::validate_plugin_segment; + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct AppConnectorId(pub String); + +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct PluginCapabilitySummary { + pub config_name: String, + pub display_name: String, + pub description: Option, + pub has_skills: bool, + pub mcp_server_names: Vec, + pub app_connector_ids: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PluginTelemetryMetadata { + pub plugin_id: PluginId, + pub capability_summary: Option, +} + +impl PluginTelemetryMetadata { + pub fn from_plugin_id(plugin_id: &PluginId) -> Self { + Self { + plugin_id: plugin_id.clone(), + capability_summary: None, + } + } +} + +impl PluginCapabilitySummary { + pub fn telemetry_metadata(&self) -> Option { + PluginId::parse(&self.config_name) + .ok() + .map(|plugin_id| PluginTelemetryMetadata { + plugin_id, + capability_summary: Some(self.clone()), + }) + } +} diff --git a/codex-rs/plugin/src/plugin_id.rs b/codex-rs/plugin/src/plugin_id.rs new file mode 100644 index 0000000000..075116322b --- /dev/null +++ b/codex-rs/plugin/src/plugin_id.rs @@ -0,0 +1,64 @@ +//! Stable plugin identifier parsing and validation shared with the plugin cache. + +#[derive(Debug, thiserror::Error)] +pub enum PluginIdError { + #[error("{0}")] + Invalid(String), +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PluginId { + pub plugin_name: String, + pub marketplace_name: String, +} + +impl PluginId { + pub fn new(plugin_name: String, marketplace_name: String) -> Result { + validate_plugin_segment(&plugin_name, "plugin name").map_err(PluginIdError::Invalid)?; + validate_plugin_segment(&marketplace_name, "marketplace name") + .map_err(PluginIdError::Invalid)?; + Ok(Self { + plugin_name, + marketplace_name, + }) + } + + pub fn parse(plugin_key: &str) -> Result { + let Some((plugin_name, marketplace_name)) = plugin_key.rsplit_once('@') else { + return Err(PluginIdError::Invalid(format!( + "invalid plugin key `{plugin_key}`; expected @" + ))); + }; + if plugin_name.is_empty() || marketplace_name.is_empty() { + return Err(PluginIdError::Invalid(format!( + "invalid plugin key `{plugin_key}`; expected @" + ))); + } + + Self::new(plugin_name.to_string(), marketplace_name.to_string()).map_err(|err| match err { + PluginIdError::Invalid(message) => { + PluginIdError::Invalid(format!("{message} in `{plugin_key}`")) + } + }) + } + + pub fn as_key(&self) -> String { + format!("{}@{}", self.plugin_name, self.marketplace_name) + } +} + +/// Validates a single path segment used in plugin IDs and cache layout. +pub fn validate_plugin_segment(segment: &str, kind: &str) -> Result<(), String> { + if segment.is_empty() { + return Err(format!("invalid {kind}: must not be empty")); + } + if !segment + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || ch == '-' || ch == '_') + { + return Err(format!( + "invalid {kind}: only ASCII letters, digits, `_`, and `-` are allowed" + )); + } + Ok(()) +}