From 8dc438044879e3a27c726d2bc5248eb030dda211 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 24 Mar 2026 20:59:22 -0700 Subject: [PATCH] progress --- codex-rs/Cargo.lock | 33 + codex-rs/Cargo.toml | 4 + codex-rs/app-server/src/config_api.rs | 10 +- codex-rs/config/Cargo.toml | 1 + codex-rs/config/src/lib.rs | 4 + codex-rs/core/Cargo.toml | 2 + codex-rs/core/src/analytics_client.rs | 61 +- codex-rs/core/src/codex_tests.rs | 8 +- codex-rs/core/src/config/types.rs | 37 +- codex-rs/core/src/lib.rs | 2 +- codex-rs/core/src/plugins/manager.rs | 46 +- codex-rs/core/src/plugins/marketplace.rs | 4 +- codex-rs/core/src/plugins/mod.rs | 9 +- codex-rs/core/src/plugins/store.rs | 65 +- codex-rs/core/src/plugins/store_tests.rs | 1 + codex-rs/core/src/skills/config_rules.rs | 135 -- .../core/src/skills/env_var_dependencies.rs | 162 -- codex-rs/core/src/skills/injection.rs | 493 ---- codex-rs/core/src/skills/injection_tests.rs | 348 --- codex-rs/core/src/skills/invocation_utils.rs | 235 -- .../core/src/skills/invocation_utils_tests.rs | 119 - codex-rs/core/src/skills/loader.rs | 926 -------- codex-rs/core/src/skills/loader_tests.rs | 2060 ----------------- codex-rs/core/src/skills/manager.rs | 331 --- codex-rs/core/src/skills/manager_tests.rs | 634 ----- codex-rs/core/src/skills/mod.rs | 25 - codex-rs/core/src/skills/model.rs | 158 -- codex-rs/core/src/skills/remote.rs | 270 --- codex-rs/core/src/skills/render.rs | 48 - codex-rs/core/src/skills/system.rs | 9 - 30 files changed, 113 insertions(+), 6127 deletions(-) delete mode 100644 codex-rs/core/src/skills/config_rules.rs delete mode 100644 codex-rs/core/src/skills/env_var_dependencies.rs delete mode 100644 codex-rs/core/src/skills/injection.rs delete mode 100644 codex-rs/core/src/skills/injection_tests.rs delete mode 100644 codex-rs/core/src/skills/invocation_utils.rs delete mode 100644 codex-rs/core/src/skills/invocation_utils_tests.rs delete mode 100644 codex-rs/core/src/skills/loader.rs delete mode 100644 codex-rs/core/src/skills/loader_tests.rs delete mode 100644 codex-rs/core/src/skills/manager.rs delete mode 100644 codex-rs/core/src/skills/manager_tests.rs delete mode 100644 codex-rs/core/src/skills/mod.rs delete mode 100644 codex-rs/core/src/skills/model.rs delete mode 100644 codex-rs/core/src/skills/remote.rs delete mode 100644 codex-rs/core/src/skills/render.rs delete mode 100644 codex-rs/core/src/skills/system.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 9ecf9b1de4..189ae1983a 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1830,6 +1830,7 @@ dependencies = [ "futures", "multimap", "pretty_assertions", + "schemars 0.8.22", "serde", "serde_json", "serde_path_to_error", @@ -1878,6 +1879,7 @@ dependencies = [ "codex-code-mode", "codex-config", "codex-connectors", + "codex-core-skills", "codex-exec-server", "codex-execpolicy", "codex-features", @@ -1886,6 +1888,7 @@ dependencies = [ "codex-login", "codex-network-proxy", "codex-otel", + "codex-plugin", "codex-protocol", "codex-rmcp-client", "codex-rollout", @@ -1971,6 +1974,29 @@ dependencies = [ "zstd", ] +[[package]] +name = "codex-core-skills" +version = "0.0.0" +dependencies = [ + "anyhow", + "codex-app-server-protocol", + "codex-config", + "codex-otel", + "codex-protocol", + "codex-skills", + "codex-utils-absolute-path", + "dirs", + "dunce", + "pretty_assertions", + "reqwest", + "serde", + "serde_json", + "tempfile", + "tokio", + "toml 0.9.11+spec-1.1.0", + "tracing", +] + [[package]] name = "codex-debug-client" version = "0.0.0" @@ -2383,6 +2409,13 @@ dependencies = [ "zip", ] +[[package]] +name = "codex-plugin" +version = "0.0.0" +dependencies = [ + "thiserror 2.0.18", +] + [[package]] name = "codex-process-hardening" version = "0.0.0" diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 5854c1c46d..14d48cc210 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -24,6 +24,7 @@ members = [ "shell-escalation", "skills", "core", + "core-skills", "hooks", "secrets", "exec", @@ -77,6 +78,7 @@ members = [ "codex-experimental-api-macros", "test-macros", "package-manager", + "plugin", "artifacts", ] resolver = "2" @@ -113,6 +115,7 @@ codex-cloud-requirements = { path = "cloud-requirements" } codex-connectors = { path = "connectors" } codex-config = { path = "config" } codex-core = { path = "core" } +codex-core-skills = { path = "core-skills" } codex-exec = { path = "exec" } codex-exec-server = { path = "exec-server" } codex-execpolicy = { path = "execpolicy" } @@ -130,6 +133,7 @@ codex-mcp-server = { path = "mcp-server" } codex-network-proxy = { path = "network-proxy" } codex-ollama = { path = "ollama" } codex-otel = { path = "otel" } +codex-plugin = { path = "plugin" } codex-process-hardening = { path = "process-hardening" } codex-protocol = { path = "protocol" } codex-rollout = { path = "rollout" } diff --git a/codex-rs/app-server/src/config_api.rs b/codex-rs/app-server/src/config_api.rs index bc82e9152e..09cc55341f 100644 --- a/codex-rs/app-server/src/config_api.rs +++ b/codex-rs/app-server/src/config_api.rs @@ -411,10 +411,12 @@ mod tests { Arc::new(RwLock::new(CloudRequirementsLoader::default())), reloader.clone(), AnalyticsEventsClient::new( - analytics_config, - codex_core::test_support::auth_manager_from_auth( - codex_core::CodexAuth::from_api_key("test"), - ), + Arc::clone(&analytics_config.auth_manager), + analytics_config + .chatgpt_base_url + .trim_end_matches('/') + .to_string(), + analytics_config.analytics_enabled, ), ); diff --git a/codex-rs/config/Cargo.toml b/codex-rs/config/Cargo.toml index 02446ed6f3..f1bd2dba16 100644 --- a/codex-rs/config/Cargo.toml +++ b/codex-rs/config/Cargo.toml @@ -14,6 +14,7 @@ codex-protocol = { workspace = true } codex-utils-absolute-path = { workspace = true } futures = { workspace = true, features = ["alloc", "std"] } multimap = { workspace = true } +schemars = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } serde_path_to_error = { workspace = true } diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 995a5b8db7..3c0f66beb6 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -6,6 +6,7 @@ mod fingerprint; mod merge; mod overrides; mod requirements_exec_policy; +mod skills_config; mod state; pub const CONFIG_TOML_FILE: &str = "config.toml"; @@ -52,6 +53,9 @@ pub use requirements_exec_policy::RequirementsExecPolicyParseError; pub use requirements_exec_policy::RequirementsExecPolicyPatternTokenToml; pub use requirements_exec_policy::RequirementsExecPolicyPrefixRuleToml; pub use requirements_exec_policy::RequirementsExecPolicyToml; +pub use skills_config::BundledSkillsConfig; +pub use skills_config::SkillConfig; +pub use skills_config::SkillsConfig; pub use state::ConfigLayerEntry; pub use state::ConfigLayerStack; pub use state::ConfigLayerStackOrdering; diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index a3868e4358..1ff094b9e2 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -34,6 +34,7 @@ codex-async-utils = { workspace = true } codex-code-mode = { workspace = true } codex-connectors = { workspace = true } codex-config = { workspace = true } +codex-core-skills = { workspace = true } codex-exec-server = { workspace = true } codex-features = { workspace = true } codex-login = { workspace = true } @@ -45,6 +46,7 @@ codex-hooks = { workspace = true } codex-network-proxy = { workspace = true } codex-otel = { workspace = true } codex-artifacts = { workspace = true } +codex-plugin = { workspace = true } codex-protocol = { workspace = true } codex-rollout = { workspace = true } codex-rmcp-client = { workspace = true } diff --git a/codex-rs/core/src/analytics_client.rs b/codex-rs/core/src/analytics_client.rs index 922f16de58..cf4781a5c0 100644 --- a/codex-rs/core/src/analytics_client.rs +++ b/codex-rs/core/src/analytics_client.rs @@ -1,5 +1,4 @@ use crate::AuthManager; -use crate::config::Config; use crate::default_client::create_client; use crate::plugins::PluginTelemetryMetadata; use codex_git_utils::collect_git_info; @@ -66,7 +65,8 @@ pub(crate) struct AnalyticsEventsQueue { #[derive(Clone)] pub struct AnalyticsEventsClient { queue: AnalyticsEventsQueue, - config: Arc, + analytics_enabled: Option, + base_url: String, } impl AnalyticsEventsQueue { @@ -147,10 +147,15 @@ impl AnalyticsEventsQueue { } impl AnalyticsEventsClient { - pub fn new(config: Arc, auth_manager: Arc) -> Self { + pub fn new( + auth_manager: Arc, + base_url: String, + analytics_enabled: Option, + ) -> Self { Self { queue: AnalyticsEventsQueue::new(Arc::clone(&auth_manager)), - config, + analytics_enabled, + base_url, } } @@ -161,7 +166,7 @@ impl AnalyticsEventsClient { ) { track_skill_invocations( &self.queue, - Arc::clone(&self.config), + self.analytics_enabled, Some(tracking), invocations, ); @@ -174,7 +179,7 @@ impl AnalyticsEventsClient { ) { track_app_mentioned( &self.queue, - Arc::clone(&self.config), + self.analytics_enabled, Some(tracking), mentions, ); @@ -246,31 +251,31 @@ enum TrackEventsJob { } struct TrackSkillInvocationsJob { - config: Arc, + analytics_enabled: Option, tracking: TrackEventsContext, invocations: Vec, } struct TrackAppMentionedJob { - config: Arc, + analytics_enabled: Option, tracking: TrackEventsContext, mentions: Vec, } struct TrackAppUsedJob { - config: Arc, + analytics_enabled: Option, tracking: TrackEventsContext, app: AppInvocation, } struct TrackPluginUsedJob { - config: Arc, + analytics_enabled: Option, tracking: TrackEventsContext, plugin: PluginTelemetryMetadata, } struct TrackPluginManagementJob { - config: Arc, + analytics_enabled: Option, plugin: PluginTelemetryMetadata, } @@ -379,11 +384,11 @@ struct CodexPluginUsedEventRequest { pub(crate) fn track_skill_invocations( queue: &AnalyticsEventsQueue, - config: Arc, + analytics_enabled: Option, tracking: Option, invocations: Vec, ) { - if config.analytics_enabled == Some(false) { + if analytics_enabled == Some(false) { return; } let Some(tracking) = tracking else { @@ -393,7 +398,7 @@ pub(crate) fn track_skill_invocations( return; } let job = TrackEventsJob::SkillInvocations(TrackSkillInvocationsJob { - config, + analytics_enabled, tracking, invocations, }); @@ -402,11 +407,11 @@ pub(crate) fn track_skill_invocations( pub(crate) fn track_app_mentioned( queue: &AnalyticsEventsQueue, - config: Arc, + analytics_enabled: Option, tracking: Option, mentions: Vec, ) { - if config.analytics_enabled == Some(false) { + if analytics_enabled == Some(false) { return; } let Some(tracking) = tracking else { @@ -416,7 +421,7 @@ pub(crate) fn track_app_mentioned( return; } let job = TrackEventsJob::AppMentioned(TrackAppMentionedJob { - config, + analytics_enabled, tracking, mentions, }); @@ -439,7 +444,7 @@ pub(crate) fn track_app_used( return; } let job = TrackEventsJob::AppUsed(TrackAppUsedJob { - config, + analytics_enabled, tracking, app, }); @@ -462,7 +467,7 @@ pub(crate) fn track_plugin_used( return; } let job = TrackEventsJob::PluginUsed(TrackPluginUsedJob { - config, + analytics_enabled, tracking, plugin, }); @@ -478,7 +483,10 @@ fn track_plugin_management( if config.analytics_enabled == Some(false) { return; } - let job = TrackPluginManagementJob { config, plugin }; + let job = TrackPluginManagementJob { + analytics_enabled, + plugin, + }; let job = match event_type { PluginManagementEventType::Installed => TrackEventsJob::PluginInstalled(job), PluginManagementEventType::Uninstalled => TrackEventsJob::PluginUninstalled(job), @@ -490,7 +498,7 @@ fn track_plugin_management( async fn send_track_skill_invocations(auth_manager: &AuthManager, job: TrackSkillInvocationsJob) { let TrackSkillInvocationsJob { - config, + analytics_enabled, tracking, invocations, } = job; @@ -538,7 +546,7 @@ async fn send_track_skill_invocations(auth_manager: &AuthManager, job: TrackSkil async fn send_track_app_mentioned(auth_manager: &AuthManager, job: TrackAppMentionedJob) { let TrackAppMentionedJob { - config, + analytics_enabled, tracking, mentions, } = job; @@ -558,7 +566,7 @@ async fn send_track_app_mentioned(auth_manager: &AuthManager, job: TrackAppMenti async fn send_track_app_used(auth_manager: &AuthManager, job: TrackAppUsedJob) { let TrackAppUsedJob { - config, + analytics_enabled, tracking, app, } = job; @@ -573,7 +581,7 @@ async fn send_track_app_used(auth_manager: &AuthManager, job: TrackAppUsedJob) { async fn send_track_plugin_used(auth_manager: &AuthManager, job: TrackPluginUsedJob) { let TrackPluginUsedJob { - config, + analytics_enabled, tracking, plugin, } = job; @@ -606,7 +614,10 @@ async fn send_track_plugin_management_event( job: TrackPluginManagementJob, event_type: &'static str, ) { - let TrackPluginManagementJob { config, plugin } = job; + let TrackPluginManagementJob { + analytics_enabled, + plugin, + } = job; let event_params = codex_plugin_metadata(plugin); let event = CodexPluginEventRequest { event_type, diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 0ba0a1beb0..c4c3d50ac9 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -2637,7 +2637,9 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { shell_zsh_path: None, main_execve_wrapper_exe: config.main_execve_wrapper_exe.clone(), analytics_events_client: AnalyticsEventsClient::new( - Arc::clone(&config), + Arc::new( + crate::analytics_client::AnalyticsEventsSettings::from_config(config.as_ref()), + ), Arc::clone(&auth_manager), ), hooks: Hooks::new(HooksConfig { @@ -3472,7 +3474,9 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx( shell_zsh_path: None, main_execve_wrapper_exe: config.main_execve_wrapper_exe.clone(), analytics_events_client: AnalyticsEventsClient::new( - Arc::clone(&config), + Arc::new( + crate::analytics_client::AnalyticsEventsSettings::from_config(config.as_ref()), + ), Arc::clone(&auth_manager), ), hooks: Hooks::new(HooksConfig { diff --git a/codex-rs/core/src/config/types.rs b/codex-rs/core/src/config/types.rs index 7a94700ce9..c9f83482aa 100644 --- a/codex-rs/core/src/config/types.rs +++ b/codex-rs/core/src/config/types.rs @@ -801,17 +801,9 @@ impl Notice { pub(crate) const TABLE_KEY: &'static str = "notice"; } -#[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, -} +pub use codex_config::BundledSkillsConfig; +pub use codex_config::SkillConfig; +pub use codex_config::SkillsConfig; #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)] #[schemars(deny_unknown_fields)] @@ -820,29 +812,6 @@ pub struct PluginConfig { 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 } - } -} - #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema)] #[schemars(deny_unknown_fields)] pub struct SandboxWorkspaceWrite { diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 71f4328aed..036e9cfb55 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -127,7 +127,7 @@ pub mod seatbelt; mod session_rollout_init_error; pub mod shell; pub mod shell_snapshot; -pub mod skills; +pub use codex_core_skills::skills; pub mod spawn; pub mod state_db_bridge; pub use codex_rollout::state_db; diff --git a/codex-rs/core/src/plugins/manager.rs b/codex-rs/core/src/plugins/manager.rs index d2d2e1b1c2..8ea4e6fabc 100644 --- a/codex-rs/core/src/plugins/manager.rs +++ b/codex-rs/core/src/plugins/manager.rs @@ -20,8 +20,6 @@ use super::remote::fetch_remote_featured_plugin_ids; use super::remote::fetch_remote_plugin_status; use super::remote::uninstall_remote_plugin; use super::startup_sync::start_startup_remote_plugin_sync_once; -use super::store::PluginId; -use super::store::PluginIdError; use super::store::PluginInstallResult as StorePluginInstallResult; use super::store::PluginStore; use super::store::PluginStoreError; @@ -46,6 +44,11 @@ use crate::skills::loader::load_skills_from_roots; use codex_app_server_protocol::ConfigValueWriteParams; use codex_app_server_protocol::MergeStrategy; use codex_features::Feature; +use codex_plugin::AppConnectorId; +use codex_plugin::PluginCapabilitySummary; +use codex_plugin::PluginId; +use codex_plugin::PluginIdError; +use codex_plugin::PluginTelemetryMetadata; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -114,9 +117,6 @@ fn featured_plugin_ids_cache_key( } } -#[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub struct AppConnectorId(pub String); - #[derive(Debug, Clone, PartialEq, Eq)] pub struct PluginInstallRequest { pub plugin_name: String, @@ -206,31 +206,6 @@ impl LoadedPlugin { } } -#[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 { fn from_plugin(plugin: &LoadedPlugin) -> Option { if !plugin.is_active() { @@ -257,15 +232,6 @@ impl PluginCapabilitySummary { || !summary.app_connector_ids.is_empty()) .then_some(summary) } - - pub fn telemetry_metadata(&self) -> Option { - PluginId::parse(&self.config_name) - .ok() - .map(|plugin_id| PluginTelemetryMetadata { - plugin_id, - capability_summary: Some(self.clone()), - }) - } } impl From for PluginCapabilitySummary { @@ -1174,7 +1140,7 @@ impl PluginsManager { } }) .collect::>(); - configured_curated_plugin_ids.sort_unstable_by_key(super::store::PluginId::as_key); + configured_curated_plugin_ids.sort_unstable_by_key(PluginId::as_key); self.start_curated_repo_sync(configured_curated_plugin_ids); start_startup_remote_plugin_sync_once( Arc::clone(self), diff --git a/codex-rs/core/src/plugins/marketplace.rs b/codex-rs/core/src/plugins/marketplace.rs index 5563f0c3aa..ee775262aa 100644 --- a/codex-rs/core/src/plugins/marketplace.rs +++ b/codex-rs/core/src/plugins/marketplace.rs @@ -1,10 +1,10 @@ use super::PluginManifestInterface; use super::load_plugin_manifest; -use super::store::PluginId; -use super::store::PluginIdError; use codex_app_server_protocol::PluginAuthPolicy; use codex_app_server_protocol::PluginInstallPolicy; use codex_git_utils::get_git_repo_root; +use codex_plugin::PluginId; +use codex_plugin::PluginIdError; use codex_protocol::protocol::Product; use codex_utils_absolute_path::AbsolutePathBuf; use dirs::home_dir; diff --git a/codex-rs/core/src/plugins/mod.rs b/codex-rs/core/src/plugins/mod.rs index 34403f4d1f..cc3e1626b6 100644 --- a/codex-rs/core/src/plugins/mod.rs +++ b/codex-rs/core/src/plugins/mod.rs @@ -11,15 +11,18 @@ mod store; pub(crate) mod test_support; mod toggles; +pub use codex_plugin::AppConnectorId; +pub use codex_plugin::PluginCapabilitySummary; +pub use codex_plugin::PluginId; +pub use codex_plugin::PluginIdError; +pub use codex_plugin::PluginTelemetryMetadata; pub(crate) use discoverable::list_tool_suggest_discoverable_plugins; pub(crate) use injection::build_plugin_injections; -pub use manager::AppConnectorId; pub use manager::ConfiguredMarketplace; pub use manager::ConfiguredMarketplaceListOutcome; pub use manager::ConfiguredMarketplacePlugin; pub use manager::LoadedPlugin; pub use manager::OPENAI_CURATED_MARKETPLACE_NAME; -pub use manager::PluginCapabilitySummary; pub use manager::PluginDetail; pub use manager::PluginInstallError; pub use manager::PluginInstallOutcome; @@ -28,7 +31,6 @@ pub use manager::PluginLoadOutcome; pub use manager::PluginReadOutcome; pub use manager::PluginReadRequest; pub use manager::PluginRemoteSyncError; -pub use manager::PluginTelemetryMetadata; pub use manager::PluginUninstallError; pub use manager::PluginsManager; pub use manager::RemotePluginSyncResult; @@ -53,5 +55,4 @@ pub(crate) use render::render_plugins_section; pub(crate) use startup_sync::curated_plugins_repo_path; pub(crate) use startup_sync::read_curated_plugins_sha; pub(crate) use startup_sync::sync_openai_plugins_repo; -pub use store::PluginId; pub use toggles::collect_plugin_enabled_candidates; diff --git a/codex-rs/core/src/plugins/store.rs b/codex-rs/core/src/plugins/store.rs index 97b0ccefe1..a222fb7f70 100644 --- a/codex-rs/core/src/plugins/store.rs +++ b/codex-rs/core/src/plugins/store.rs @@ -1,5 +1,8 @@ use super::load_plugin_manifest; use super::manifest::PLUGIN_MANIFEST_PATH; +use codex_plugin::PluginId; +use codex_plugin::PluginIdError; +use codex_plugin::validate_plugin_segment; use codex_utils_absolute_path::AbsolutePathBuf; use std::fs; use std::io; @@ -9,53 +12,6 @@ use std::path::PathBuf; pub(crate) const DEFAULT_PLUGIN_VERSION: &str = "local"; pub(crate) const PLUGINS_CACHE_DIR: &str = "plugins/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) - } -} - #[derive(Debug, Clone, PartialEq, Eq)] pub struct PluginInstallResult { pub plugin_id: PluginId, @@ -221,21 +177,6 @@ fn plugin_name_for_source(source_path: &Path) -> Result 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(()) -} - fn remove_existing_target(path: &Path) -> Result<(), PluginStoreError> { if !path.exists() { return Ok(()); diff --git a/codex-rs/core/src/plugins/store_tests.rs b/codex-rs/core/src/plugins/store_tests.rs index 61c3460255..e75e71c568 100644 --- a/codex-rs/core/src/plugins/store_tests.rs +++ b/codex-rs/core/src/plugins/store_tests.rs @@ -1,4 +1,5 @@ use super::*; +use codex_plugin::PluginId; use pretty_assertions::assert_eq; use tempfile::tempdir; diff --git a/codex-rs/core/src/skills/config_rules.rs b/codex-rs/core/src/skills/config_rules.rs deleted file mode 100644 index fa1be878bb..0000000000 --- a/codex-rs/core/src/skills/config_rules.rs +++ /dev/null @@ -1,135 +0,0 @@ -use std::collections::HashSet; -use std::path::Path; -use std::path::PathBuf; - -use codex_app_server_protocol::ConfigLayerSource; -use tracing::warn; - -use crate::config::types::SkillConfig; -use crate::config::types::SkillsConfig; -use crate::config_loader::ConfigLayerStack; -use crate::config_loader::ConfigLayerStackOrdering; -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/src/skills/env_var_dependencies.rs b/codex-rs/core/src/skills/env_var_dependencies.rs deleted file mode 100644 index 00f5bad8cc..0000000000 --- a/codex-rs/core/src/skills/env_var_dependencies.rs +++ /dev/null @@ -1,162 +0,0 @@ -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/src/skills/injection.rs b/codex-rs/core/src/skills/injection.rs deleted file mode 100644 index b83be2322c..0000000000 --- a/codex-rs/core/src/skills/injection.rs +++ /dev/null @@ -1,493 +0,0 @@ -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/src/skills/injection_tests.rs b/codex-rs/core/src/skills/injection_tests.rs deleted file mode 100644 index 8d66a0af57..0000000000 --- a/codex-rs/core/src/skills/injection_tests.rs +++ /dev/null @@ -1,348 +0,0 @@ -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/src/skills/invocation_utils.rs b/codex-rs/core/src/skills/invocation_utils.rs deleted file mode 100644 index 122158bda7..0000000000 --- a/codex-rs/core/src/skills/invocation_utils.rs +++ /dev/null @@ -1,235 +0,0 @@ -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/src/skills/invocation_utils_tests.rs b/codex-rs/core/src/skills/invocation_utils_tests.rs deleted file mode 100644 index 657582b742..0000000000 --- a/codex-rs/core/src/skills/invocation_utils_tests.rs +++ /dev/null @@ -1,119 +0,0 @@ -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/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs deleted file mode 100644 index 5672bdb0a3..0000000000 --- a/codex-rs/core/src/skills/loader.rs +++ /dev/null @@ -1,926 +0,0 @@ -use crate::config_loader::ConfigLayerStack; -use crate::config_loader::ConfigLayerStackOrdering; -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_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/src/skills/loader_tests.rs b/codex-rs/core/src/skills/loader_tests.rs deleted file mode 100644 index 7f758735cb..0000000000 --- a/codex-rs/core/src/skills/loader_tests.rs +++ /dev/null @@ -1,2060 +0,0 @@ -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::ConfigLayerStack; -use crate::config_loader::ConfigRequirements; -use crate::config_loader::ConfigRequirementsToml; -use codex_config::CONFIG_TOML_FILE; -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/src/skills/manager.rs b/codex-rs/core/src/skills/manager.rs deleted file mode 100644 index 446580b6c7..0000000000 --- a/codex-rs/core/src/skills/manager.rs +++ /dev/null @@ -1,331 +0,0 @@ -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::types::SkillsConfig; -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; - -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: &crate::config_loader::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/src/skills/manager_tests.rs b/codex-rs/core/src/skills/manager_tests.rs deleted file mode 100644 index d9e5859f4e..0000000000 --- a/codex-rs/core/src/skills/manager_tests.rs +++ /dev/null @@ -1,634 +0,0 @@ -use super::*; -use crate::config::ConfigBuilder; -use crate::config::ConfigOverrides; -use crate::config_loader::ConfigLayerEntry; -use crate::config_loader::ConfigLayerStack; -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 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/src/skills/mod.rs b/codex-rs/core/src/skills/mod.rs deleted file mode 100644 index ad76cd9da0..0000000000 --- a/codex-rs/core/src/skills/mod.rs +++ /dev/null @@ -1,25 +0,0 @@ -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 env_var_dependencies::collect_env_var_dependencies; -pub(crate) use env_var_dependencies::resolve_skill_dependencies_for_turn; -pub(crate) use injection::SkillInjections; -pub(crate) use injection::build_skill_injections; -pub(crate) use injection::collect_explicit_skill_mentions; -pub(crate) use invocation_utils::build_implicit_skill_path_indexes; -pub(crate) use invocation_utils::maybe_emit_implicit_skill_invocation; -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/src/skills/model.rs b/codex-rs/core/src/skills/model.rs deleted file mode 100644 index d47904b9c7..0000000000 --- a/codex-rs/core/src/skills/model.rs +++ /dev/null @@ -1,158 +0,0 @@ -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/src/skills/remote.rs b/codex-rs/core/src/skills/remote.rs deleted file mode 100644 index 165c450635..0000000000 --- a/codex-rs/core/src/skills/remote.rs +++ /dev/null @@ -1,270 +0,0 @@ -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/src/skills/render.rs b/codex-rs/core/src/skills/render.rs deleted file mode 100644 index 797d53db21..0000000000 --- a/codex-rs/core/src/skills/render.rs +++ /dev/null @@ -1,48 +0,0 @@ -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/src/skills/system.rs b/codex-rs/core/src/skills/system.rs deleted file mode 100644 index 394fe00c3b..0000000000 --- a/codex-rs/core/src/skills/system.rs +++ /dev/null @@ -1,9 +0,0 @@ -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); -}