From 6059eb428ac61def5fd6ed8cba570cb5b1588bcb Mon Sep 17 00:00:00 2001 From: jif-oai Date: Thu, 25 Jun 2026 11:34:58 +0100 Subject: [PATCH] Make skills follow model-step state --- codex-rs/app-server/src/extensions.rs | 10 +- codex-rs/app-server/src/mcp_refresh.rs | 7 - codex-rs/app-server/src/message_processor.rs | 8 - codex-rs/codex-mcp/src/resource_client.rs | 37 +- codex-rs/core-skills/src/lib.rs | 5 + codex-rs/core-skills/src/model.rs | 4 + codex-rs/core-skills/src/runtime.rs | 58 +- codex-rs/core-skills/src/runtime_snapshot.rs | 483 +++++++++++++ .../core-skills/src/skill_instructions.rs | 34 +- .../context/available_skills_instructions.rs | 50 -- codex-rs/core/src/context/mod.rs | 2 - codex-rs/core/src/context/world_state/mod.rs | 2 + .../core/src/context/world_state/skills.rs | 138 ++++ codex-rs/core/src/lib.rs | 8 - codex-rs/core/src/mcp_skill_dependencies.rs | 36 +- codex-rs/core/src/plugins/mentions.rs | 2 - codex-rs/core/src/plugins/mod.rs | 1 - codex-rs/core/src/session/mod.rs | 46 +- codex-rs/core/src/session/step_context.rs | 5 + codex-rs/core/src/session/tests.rs | 62 +- codex-rs/core/src/session/turn.rs | 284 +++++--- codex-rs/core/src/session/world_state.rs | 5 + codex-rs/core/src/tools/spec_plan_tests.rs | 5 + codex-rs/core/tests/common/test_codex.rs | 10 +- codex-rs/core/tests/suite/remote_env.rs | 194 ++++++ codex-rs/ext/skills/src/extension.rs | 286 +------- codex-rs/ext/skills/src/fragments.rs | 38 -- codex-rs/ext/skills/src/lib.rs | 1 - codex-rs/ext/skills/src/render.rs | 24 - codex-rs/ext/skills/src/sources.rs | 90 ++- codex-rs/ext/skills/src/state.rs | 76 ++- codex-rs/ext/skills/tests/skills_extension.rs | 635 ++---------------- 32 files changed, 1387 insertions(+), 1259 deletions(-) create mode 100644 codex-rs/core-skills/src/runtime_snapshot.rs delete mode 100644 codex-rs/core/src/context/available_skills_instructions.rs create mode 100644 codex-rs/core/src/context/world_state/skills.rs delete mode 100644 codex-rs/ext/skills/src/fragments.rs diff --git a/codex-rs/app-server/src/extensions.rs b/codex-rs/app-server/src/extensions.rs index ed040aa8d9e9..a3919683cbe9 100644 --- a/codex-rs/app-server/src/extensions.rs +++ b/codex-rs/app-server/src/extensions.rs @@ -36,7 +36,6 @@ pub(crate) struct ThreadExtensionDependencies { pub(crate) thread_manager: Weak, pub(crate) goal_service: Arc, pub(crate) environment_manager: Arc, - pub(crate) executor_skill_provider: Arc, /// Process-scoped persistence backend for extensions that need stored thread history. pub(crate) thread_store: Arc, } @@ -56,7 +55,6 @@ where thread_manager, goal_service, environment_manager, - executor_skill_provider, thread_store: _thread_store, } = dependencies; let mut builder = ExtensionRegistryBuilder::::with_event_sink(event_sink); @@ -79,11 +77,9 @@ where codex_image_generation_extension::install(&mut builder, auth_manager, |config: &Config| { Some(config.codex_home.clone()) }); - let skill_providers = codex_skills_extension::SkillProviders::new() - .with_executor_provider(executor_skill_provider) - .with_orchestrator_provider(Arc::new( - codex_skills_extension::OrchestratorSkillProvider::new(), - )); + let skill_providers = codex_skills_extension::SkillProviders::new().with_orchestrator_provider( + Arc::new(codex_skills_extension::OrchestratorSkillProvider::new()), + ); codex_skills_extension::install_with_providers( &mut builder, skill_providers, diff --git a/codex-rs/app-server/src/mcp_refresh.rs b/codex-rs/app-server/src/mcp_refresh.rs index 848d201f2ea6..426d8fcc67b6 100644 --- a/codex-rs/app-server/src/mcp_refresh.rs +++ b/codex-rs/app-server/src/mcp_refresh.rs @@ -216,12 +216,6 @@ mod tests { .expect("refresh tests require state db"); let thread_store = thread_store_from_config(&good_config, Some(state_db.clone())); let environment_manager = Arc::new(EnvironmentManager::default_for_tests()); - let executor_skill_provider: Arc = Arc::new( - codex_skills_extension::ExecutorSkillProvider::new_with_restriction_product( - Arc::clone(&environment_manager), - SessionSource::Exec.restriction_product(), - ), - ); let thread_manager = Arc::new_cyclic(|thread_manager| { ThreadManager::new( &good_config, @@ -238,7 +232,6 @@ mod tests { thread_manager: thread_manager.clone(), goal_service: Arc::new(codex_goal_extension::GoalService::new()), environment_manager: Arc::clone(&environment_manager), - executor_skill_provider: Arc::clone(&executor_skill_provider), thread_store: Arc::clone(&thread_store), }, ), diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 41ecbfe5613e..3efaff5c1875 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -337,13 +337,6 @@ impl MessageProcessor { let thread_store = codex_core::thread_store_from_config(config.as_ref(), state_db.clone()); let environment_manager_for_requests = Arc::clone(&environment_manager); let environment_manager_for_extensions = Arc::clone(&environment_manager); - let restriction_product = session_source.restriction_product(); - let executor_skill_provider: Arc = Arc::new( - codex_skills_extension::ExecutorSkillProvider::new_with_restriction_product( - Arc::clone(&environment_manager_for_extensions), - restriction_product, - ), - ); let goal_service = Arc::new(GoalService::new()); let thread_manager = Arc::new_cyclic(|thread_manager| { ThreadManager::new( @@ -364,7 +357,6 @@ impl MessageProcessor { thread_manager: thread_manager.clone(), goal_service: Arc::clone(&goal_service), environment_manager: Arc::clone(&environment_manager_for_extensions), - executor_skill_provider: Arc::clone(&executor_skill_provider), thread_store: Arc::clone(&thread_store), }, ), diff --git a/codex-rs/codex-mcp/src/resource_client.rs b/codex-rs/codex-mcp/src/resource_client.rs index 4406b81a53c6..6f479c4027d8 100644 --- a/codex-rs/codex-mcp/src/resource_client.rs +++ b/codex-rs/codex-mcp/src/resource_client.rs @@ -33,7 +33,13 @@ pub struct McpResourceReadResult { /// snapshot, so calls automatically use replacements installed during startup and refresh. #[derive(Clone)] pub struct McpResourceClient { - manager: Arc>, + manager: ResourceManager, +} + +#[derive(Clone)] +enum ResourceManager { + Live(Arc>), + Snapshot(Arc), } /// Opaque identity for the manager currently used by an MCP resource client. @@ -59,19 +65,36 @@ impl std::fmt::Debug for McpResourceClient { impl McpResourceClient { /// Creates a resource client backed by the session's replaceable MCP manager. pub fn new(manager: Arc>) -> Self { - Self { manager } + Self { + manager: ResourceManager::Live(manager), + } + } + + /// Pins resource calls to the manager generation that is current now. + pub fn snapshot(&self) -> Self { + Self { + manager: ResourceManager::Snapshot(self.manager_snapshot()), + } + } + + /// Returns the exact manager generation used by this client right now. + pub fn manager_snapshot(&self) -> Arc { + match &self.manager { + ResourceManager::Live(manager) => manager.load_full(), + ResourceManager::Snapshot(manager) => Arc::clone(manager), + } } /// Returns an identity that changes whenever the published manager changes. pub fn cache_key(&self) -> McpResourceClientCacheKey { - McpResourceClientCacheKey(Arc::downgrade(&self.manager.load_full())) + McpResourceClientCacheKey(Arc::downgrade(&self.manager_snapshot())) } /// Returns whether the current manager contains the named server. /// /// This does not wait for server startup or imply that startup succeeded. pub async fn has_server(&self, server: &str) -> bool { - self.manager.load_full().contains_server(server) + self.manager_snapshot().contains_server(server) } /// Lists one resource page from the named server. @@ -83,8 +106,7 @@ impl McpResourceClient { let params = cursor.map(|cursor| PaginatedRequestParams::default().with_cursor(Some(cursor))); let result = self - .manager - .load_full() + .manager_snapshot() .list_resources(server, params) .await?; let resources = result @@ -101,8 +123,7 @@ impl McpResourceClient { /// Reads one resource from the named server. pub async fn read_resource(&self, server: &str, uri: &str) -> Result { let result = self - .manager - .load_full() + .manager_snapshot() .read_resource(server, ReadResourceRequestParams::new(uri.to_string())) .await?; let contents = result diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs index 83b2ba34de1f..3ef6335881ab 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -9,6 +9,7 @@ pub mod render; mod root_loader; pub mod runtime; mod runtime_selection; +mod runtime_snapshot; pub mod service; mod skill_instructions; pub mod system; @@ -36,6 +37,10 @@ pub use render::default_skill_metadata_budget; pub use render::render_available_skills_body; pub use root_loader::PluginSkillSnapshots; pub use runtime_selection::collect_runtime_skill_mentions; +pub use runtime_snapshot::RuntimeSkillInjection; +pub use runtime_snapshot::RuntimeSkillInjections; +pub use runtime_snapshot::SkillInjectionIdentity; +pub use runtime_snapshot::SkillsSnapshot; pub use service::SkillsLoadInput; pub use service::SkillsService; pub use skill_instructions::SkillInstructions; diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index 6afbde6cc94b..a7f17eb87735 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -148,6 +148,10 @@ impl HostSkillsSnapshot { self.outcome.as_ref() } + pub(crate) fn outcome_arc(&self) -> Arc { + Arc::clone(&self.outcome) + } + pub async fn read_skill_text(&self, skill: &SkillMetadata) -> io::Result { let fs = self .outcome diff --git a/codex-rs/core-skills/src/runtime.rs b/codex-rs/core-skills/src/runtime.rs index bad8460fddb4..a8240b2fece5 100644 --- a/codex-rs/core-skills/src/runtime.rs +++ b/codex-rs/core-skills/src/runtime.rs @@ -1,4 +1,5 @@ use std::any::Any; +use std::collections::HashMap; use std::fmt; use std::future::Future; use std::hash::Hash; @@ -306,7 +307,7 @@ pub trait SkillSource: Send + Sync { #[derive(Clone)] struct RegisteredSkillSource { label: String, - source: Arc, + source: Arc Arc + Send + Sync>, } /// Bound skill sources used to build and read one runtime catalog. @@ -321,6 +322,18 @@ impl SkillSources { } pub fn with_source(mut self, label: impl Into, source: Arc) -> Self { + self.sources.push(RegisteredSkillSource { + label: label.into(), + source: Arc::new(move || Arc::clone(&source)), + }); + self + } + + pub fn with_source_factory( + mut self, + label: impl Into, + source: Arc Arc + Send + Sync>, + ) -> Self { self.sources.push(RegisteredSkillSource { label: label.into(), source, @@ -328,28 +341,53 @@ impl SkillSources { self } + pub fn extend(&mut self, other: Self) { + self.sources.extend(other.sources); + } + pub async fn list(&self) -> SkillCatalog { + self.list_with_sources().await.0 + } + + pub(crate) async fn list_with_sources( + &self, + ) -> ( + SkillCatalog, + HashMap<(SkillAuthority, SkillPackageId), Arc>, + ) { let mut catalog = SkillCatalog::default(); + let mut sources = HashMap::new(); for source in &self.sources { - match source.source.list().await { - Ok(source_catalog) => catalog.extend(source_catalog), + let bound_source = (source.source)(); + match bound_source.list().await { + Ok(source_catalog) => { + catalog.warnings.extend(source_catalog.warnings); + for entry in source_catalog.entries { + let key = (entry.authority.clone(), entry.id.clone()); + if let std::collections::hash_map::Entry::Vacant(route) = sources.entry(key) + { + route.insert(Arc::clone(&bound_source)); + catalog.entries.push(entry); + } + } + } Err(err) => catalog.warnings.push(format!( "{} skills unavailable: {}", source.label, err.message )), } } - catalog + (catalog, sources) } pub async fn list_kind(&self, kind: &SkillSourceKind) -> SkillSourceResult { let mut catalog = SkillCatalog::default(); - for source in self - .sources - .iter() - .filter(|source| source.source.kind() == *kind) - { - let source_catalog = source.source.list().await.map_err(|err| { + for source in self.sources.iter() { + let bound_source = (source.source)(); + if bound_source.kind() != *kind { + continue; + } + let source_catalog = bound_source.list().await.map_err(|err| { SkillSourceError::new(format!( "{} skills unavailable: {}", source.label, err.message diff --git a/codex-rs/core-skills/src/runtime_snapshot.rs b/codex-rs/core-skills/src/runtime_snapshot.rs new file mode 100644 index 000000000000..e1e538a33f93 --- /dev/null +++ b/codex-rs/core-skills/src/runtime_snapshot.rs @@ -0,0 +1,483 @@ +use std::collections::HashMap; +use std::collections::HashSet; +use std::fmt; +use std::sync::Arc; + +use codex_context_fragments::ContextualUserFragment; +use codex_exec_server::ResolvedSelectedCapabilityRoot; +use codex_protocol::capabilities::CapabilityRootLocation; +use codex_protocol::protocol::Product; +use codex_protocol::user_input::UserInput; + +use crate::AvailableSkills; +use crate::HostSkillsSnapshot; +use crate::SkillInstructions; +use crate::SkillMetadata; +use crate::collect_runtime_skill_mentions; +use crate::default_skill_metadata_budget; +use crate::loader::EnvironmentSkillMetadata; +use crate::loader::load_environment_skills_from_root; +use crate::render::SkillRenderSideEffects; +use crate::render::build_available_skills_from_catalog; +use crate::runtime::SkillAuthority; +use crate::runtime::SkillCatalog; +use crate::runtime::SkillCatalogEntry; +use crate::runtime::SkillPackageId; +use crate::runtime::SkillReadRequest; +use crate::runtime::SkillReadResult; +use crate::runtime::SkillResourceId; +use crate::runtime::SkillSource; +use crate::runtime::SkillSourceError; +use crate::runtime::SkillSourceFuture; +use crate::runtime::SkillSourceIdentity; +use crate::runtime::SkillSourceKind; +use crate::runtime::SkillSources; + +const MAX_INJECTIONS_PER_STEP: usize = 8; +const MAX_INJECTION_BYTES_PER_STEP: usize = 32 * 1024; +const HOST_AUTHORITY_ID: &str = "host"; + +type EntryKey = (SkillAuthority, SkillPackageId); + +/// Identity of one injected package and the runtime owner that supplied it. +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +pub struct SkillInjectionIdentity { + source: SkillSourceIdentity, + authority: SkillAuthority, + package: SkillPackageId, +} + +#[derive(Debug)] +pub struct RuntimeSkillInjection { + pub identity: SkillInjectionIdentity, + pub entry: SkillCatalogEntry, + pub instructions: SkillInstructions, +} + +#[derive(Debug, Default)] +pub struct RuntimeSkillInjections { + pub items: Vec, + pub warnings: Vec, +} + +/// One immutable, authority-aware skill view used by a model sampling step. +#[derive(Clone)] +pub struct SkillsSnapshot { + host: HostSkillsSnapshot, + catalog: Arc, + sources: Arc>>, + available: Arc>, + warnings: Arc>, +} + +impl SkillsSnapshot { + pub fn from_host(host: HostSkillsSnapshot, context_window: Option) -> Self { + let source: Arc = Arc::new(HostSkillSource::new(host.clone())); + let catalog = host_catalog(&host); + let source_by_entry = catalog + .entries + .iter() + .map(|entry| { + ( + (entry.authority.clone(), entry.id.clone()), + Arc::clone(&source), + ) + }) + .collect(); + Self::new(host, catalog, source_by_entry, context_window) + } + + pub async fn load( + host: HostSkillsSnapshot, + executor_roots: &[ResolvedSelectedCapabilityRoot], + extra_sources: Option<&SkillSources>, + restriction_product: Option, + context_window: Option, + ) -> Self { + let mut sources = + SkillSources::new().with_source("host", Arc::new(HostSkillSource::new(host.clone()))); + for root in executor_roots { + sources = sources.with_source( + format!("executor root `{}`", root.selected_root().id), + Arc::new(ExecutorSkillSource::new(root.clone(), restriction_product)), + ); + } + if let Some(extra_sources) = extra_sources { + sources.extend(extra_sources.clone()); + } + + let (catalog, source_by_entry) = sources.list_with_sources().await; + Self::new(host, catalog, source_by_entry, context_window) + } + + fn new( + host: HostSkillsSnapshot, + catalog: SkillCatalog, + source_by_entry: HashMap>, + context_window: Option, + ) -> Self { + let available = build_available_skills_from_catalog( + &catalog, + Some(host.outcome()), + default_skill_metadata_budget(context_window), + SkillRenderSideEffects::None, + ); + let mut warnings = catalog.warnings.clone(); + if let Some(warning) = available + .as_ref() + .and_then(|available| available.warning_message.clone()) + { + warnings.push(warning); + } + Self { + host, + catalog: Arc::new(catalog), + sources: Arc::new(source_by_entry), + available: Arc::new(available), + warnings: Arc::new(warnings), + } + } + + pub fn available(&self) -> Option<&AvailableSkills> { + self.available.as_ref().as_ref() + } + + pub fn warnings(&self) -> &[String] { + self.warnings.as_ref() + } + + pub fn skill_name_counts_lower(&self) -> HashMap { + let mut counts = HashMap::new(); + for entry in self.catalog.entries.iter().filter(|entry| entry.enabled) { + *counts.entry(entry.name.to_ascii_lowercase()).or_default() += 1; + } + counts + } + + pub fn host_skill(&self, entry: &SkillCatalogEntry) -> Option<&SkillMetadata> { + if entry.authority.kind != SkillSourceKind::Host { + return None; + } + self.host + .outcome() + .skills + .iter() + .find(|skill| skill.path_to_skills_md.to_string_lossy() == entry.main_prompt.as_str()) + } + + pub async fn injections( + &self, + input: &[UserInput], + plain_name_conflicts: &HashSet, + already_injected: &HashSet, + already_injected_names: &HashSet, + ) -> RuntimeSkillInjections { + let selected = collect_runtime_skill_mentions(input, &self.catalog, plain_name_conflicts); + let mut result = RuntimeSkillInjections::default(); + let mut injected_names = already_injected_names.clone(); + let mut remaining_bytes = MAX_INJECTION_BYTES_PER_STEP; + let mut injection_limit_reached = false; + for entry in &selected { + let key = (entry.authority.clone(), entry.id.clone()); + let Some(source) = self.sources.get(&key) else { + result.warnings.push(format!( + "Failed to load skill `{}`: its runtime source is unavailable.", + entry.name + )); + continue; + }; + let identity = SkillInjectionIdentity { + source: source.identity(), + authority: entry.authority.clone(), + package: entry.id.clone(), + }; + if already_injected.contains(&identity) { + continue; + } + let replaces_previous = injected_names.contains(&entry.name); + if result.items.len() == MAX_INJECTIONS_PER_STEP { + injection_limit_reached = true; + break; + } + let request = SkillReadRequest { + authority: entry.authority.clone(), + package: entry.id.clone(), + resource: entry.main_prompt.clone(), + }; + match source.read(request).await { + Ok(read) => { + let Some((instructions, truncated)) = + SkillInstructions::from_runtime_with_total_limit( + entry, + &read.contents, + remaining_bytes, + replaces_previous, + ) + else { + result.warnings.push( + "Additional selected skills were not loaded because this step reached the 32 KB skill instruction limit." + .to_string(), + ); + break; + }; + let instruction_bytes = instructions.render().len(); + remaining_bytes = remaining_bytes.saturating_sub(instruction_bytes); + if truncated { + result.warnings.push(format!( + "Skill `{}` exceeded the step's instruction limit and was truncated.", + entry.name + )); + } + result.items.push(RuntimeSkillInjection { + identity, + entry: entry.clone(), + instructions, + }); + injected_names.insert(entry.name.clone()); + } + Err(err) => result + .warnings + .push(format!("Failed to load skill `{}`: {err}", entry.name)), + } + } + if injection_limit_reached { + result.warnings.push(format!( + "Only the first {MAX_INJECTIONS_PER_STEP} selected skills were loaded for this step." + )); + } + result + } +} + +impl fmt::Debug for SkillsSnapshot { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter + .debug_struct("SkillsSnapshot") + .field("catalog", &self.catalog) + .field("available", &self.available) + .finish_non_exhaustive() + } +} + +struct HostSkillSource { + snapshot: HostSkillsSnapshot, + identity: SkillSourceIdentity, +} + +impl HostSkillSource { + fn new(snapshot: HostSkillsSnapshot) -> Self { + Self { + identity: SkillSourceIdentity::from_owner(snapshot.outcome_arc()), + snapshot, + } + } +} + +impl SkillSource for HostSkillSource { + fn kind(&self) -> SkillSourceKind { + SkillSourceKind::Host + } + + fn identity(&self) -> SkillSourceIdentity { + self.identity.clone() + } + + fn list(&self) -> SkillSourceFuture<'_, SkillCatalog> { + Box::pin(async move { Ok(host_catalog(&self.snapshot)) }) + } + + fn read(&self, request: SkillReadRequest) -> SkillSourceFuture<'_, SkillReadResult> { + Box::pin(async move { + if request.authority != SkillAuthority::new(SkillSourceKind::Host, HOST_AUTHORITY_ID) { + return Err(SkillSourceError::new("host skill authority does not match")); + } + let Some(skill) = self.snapshot.outcome().skills.iter().find(|skill| { + skill.path_to_skills_md.to_string_lossy() == request.resource.as_str() + }) else { + return Err(SkillSourceError::new(format!( + "host skill resource is not loaded: {}", + request.resource.as_str() + ))); + }; + let contents = self.snapshot.read_skill_text(skill).await.map_err(|err| { + SkillSourceError::new(format!( + "failed to read host skill resource {}: {err}", + request.resource.as_str() + )) + })?; + Ok(SkillReadResult { + resource: request.resource, + contents, + }) + }) + } +} + +fn host_catalog(snapshot: &HostSkillsSnapshot) -> SkillCatalog { + let outcome = snapshot.outcome(); + let mut catalog = SkillCatalog { + warnings: outcome + .errors + .iter() + .map(|error| { + format!( + "Failed to load skill at {}: {}", + error.path.display(), + error.message + ) + }) + .collect(), + ..Default::default() + }; + for (skill, enabled) in outcome.skills_with_enabled() { + let path = skill.path_to_skills_md.to_string_lossy().into_owned(); + let mut entry = SkillCatalogEntry::new( + SkillPackageId(path.clone()), + SkillAuthority::new(SkillSourceKind::Host, HOST_AUTHORITY_ID), + skill.name.clone(), + skill.description.clone(), + SkillResourceId::new(path.clone()), + ) + .with_short_description(skill.short_description.clone()) + .with_display_path(path.replace('\\', "/")) + .with_dependencies(skill.dependencies.clone()); + if !enabled { + entry = entry.disabled(); + } + if !skill.allows_implicit_invocation() { + entry = entry.hidden_from_prompt(); + } + catalog.push_entry(entry); + } + catalog +} + +struct ExecutorSkillSource { + root: ResolvedSelectedCapabilityRoot, + authority: SkillAuthority, + restriction_product: Option, + identity: SkillSourceIdentity, +} + +impl ExecutorSkillSource { + fn new(root: ResolvedSelectedCapabilityRoot, restriction_product: Option) -> Self { + Self { + authority: SkillAuthority::new( + SkillSourceKind::Executor, + root.selected_root().id.clone(), + ), + identity: SkillSourceIdentity::from_owner(Arc::clone(root.environment())), + root, + restriction_product, + } + } +} + +impl SkillSource for ExecutorSkillSource { + fn kind(&self) -> SkillSourceKind { + SkillSourceKind::Executor + } + + fn identity(&self) -> SkillSourceIdentity { + self.identity.clone() + } + + fn list(&self) -> SkillSourceFuture<'_, SkillCatalog> { + Box::pin(async move { + let CapabilityRootLocation::Environment { + environment_id, + path, + } = &self.root.selected_root().location; + let outcome = load_environment_skills_from_root( + self.root.file_system().as_ref(), + path, + self.restriction_product, + ) + .await; + let mut catalog = SkillCatalog { + warnings: outcome.warnings, + ..Default::default() + }; + for skill in outcome.skills { + catalog.push_entry(executor_catalog_entry( + skill, + self.authority.clone(), + &self.root.selected_root().id, + environment_id, + )); + } + Ok(catalog) + }) + } + + fn read(&self, request: SkillReadRequest) -> SkillSourceFuture<'_, SkillReadResult> { + Box::pin(async move { + if request.authority != self.authority || request.package.0 != request.resource.as_str() + { + return Err(SkillSourceError::new( + "executor skill resource does not match its captured source", + )); + } + let CapabilityRootLocation::Environment { environment_id, .. } = + &self.root.selected_root().location; + let Some((resource_environment, path)) = request.resource.environment_path() else { + return Err(SkillSourceError::new( + "executor skill resource has no environment path", + )); + }; + if resource_environment != environment_id { + return Err(SkillSourceError::new( + "executor skill resource belongs to a different environment", + )); + } + let contents = self + .root + .file_system() + .read_file_text(path, /*sandbox*/ None) + .await + .map_err(|err| { + SkillSourceError::new(format!( + "failed to read executor skill resource {}: {err}", + request.resource.as_str() + )) + })?; + Ok(SkillReadResult { + resource: request.resource, + contents, + }) + }) + } +} + +fn executor_catalog_entry( + skill: EnvironmentSkillMetadata, + authority: SkillAuthority, + root_id: &str, + environment_id: &str, +) -> SkillCatalogEntry { + let prompt_visible = skill.allows_implicit_invocation(); + let path = skill.path_to_skills_md.inferred_native_path_string(); + let display_path = format!( + "skill://{root_id}/{}", + path.replace('\\', "/").trim_start_matches('/') + ); + let entry = SkillCatalogEntry::new( + SkillPackageId(display_path.clone()), + authority, + skill.name, + skill.description, + SkillResourceId::environment( + display_path.clone(), + environment_id, + skill.path_to_skills_md, + ), + ) + .with_short_description(skill.short_description) + .with_display_path(display_path) + .with_dependencies(skill.dependencies); + if prompt_visible { + entry + } else { + entry.hidden_from_prompt() + } +} diff --git a/codex-rs/core-skills/src/skill_instructions.rs b/codex-rs/core-skills/src/skill_instructions.rs index dd8fad489bb8..57562ce703ef 100644 --- a/codex-rs/core-skills/src/skill_instructions.rs +++ b/codex-rs/core-skills/src/skill_instructions.rs @@ -6,12 +6,14 @@ use crate::runtime::SkillCatalogEntry; const MAX_SKILL_NAME_BYTES: usize = 256; const MAX_SKILL_PATH_BYTES: usize = 1_024; const MAX_SKILL_BODY_BYTES: usize = 8_000; +const REPLACEMENT_NOTICE: &str = "These instructions replace the previously provided instructions for this skill.\n"; #[derive(Debug, Clone, PartialEq, Eq)] pub struct SkillInstructions { name: String, path: String, contents: String, + replaces_previous: bool, } impl From<&SkillInjection> for SkillInstructions { @@ -20,6 +22,7 @@ impl From<&SkillInjection> for SkillInstructions { name: skill.name.clone(), path: skill.path.clone(), contents: skill.contents.clone(), + replaces_previous: false, } } } @@ -33,10 +36,34 @@ impl SkillInstructions { name: truncate_utf8(&entry.name, MAX_SKILL_NAME_BYTES).0, path: truncate_utf8(entry.rendered_path(), MAX_SKILL_PATH_BYTES).0, contents, + replaces_previous: false, }, truncated, ) } + + pub(crate) fn from_runtime_with_total_limit( + entry: &SkillCatalogEntry, + contents: &str, + max_total_bytes: usize, + replaces_previous: bool, + ) -> Option<(Self, bool)> { + let name = truncate_utf8(&entry.name, MAX_SKILL_NAME_BYTES).0; + let path = truncate_utf8(entry.rendered_path(), MAX_SKILL_PATH_BYTES).0; + let mut instructions = Self { + name, + path, + contents: String::new(), + replaces_previous, + }; + let envelope_bytes = instructions.render().len(); + let max_body_bytes = max_total_bytes + .checked_sub(envelope_bytes)? + .min(MAX_SKILL_BODY_BYTES); + let (contents, truncated) = truncate_utf8(contents, max_body_bytes); + instructions.contents = contents; + Some((instructions, truncated)) + } } fn truncate_utf8(value: &str, max_bytes: usize) -> (String, bool) { @@ -61,8 +88,13 @@ impl ContextualUserFragment for SkillInstructions { } fn body(&self) -> String { + let replacement_notice = if self.replaces_previous { + REPLACEMENT_NOTICE + } else { + "" + }; format!( - "\n{}\n{}\n{}\n", + "\n{}\n{}\n{replacement_notice}{}\n", self.name, self.path, self.contents ) } diff --git a/codex-rs/core/src/context/available_skills_instructions.rs b/codex-rs/core/src/context/available_skills_instructions.rs deleted file mode 100644 index 4e166d436cfc..000000000000 --- a/codex-rs/core/src/context/available_skills_instructions.rs +++ /dev/null @@ -1,50 +0,0 @@ -use codex_core_skills::AvailableSkills; -use codex_core_skills::render_available_skills_body; -use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG; -use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; - -use super::ContextualUserFragment; - -/// Model-context fragment describing the skills available to Codex. -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct AvailableSkillsInstructions { - skill_root_lines: Vec, - skill_lines: Vec, -} - -impl AvailableSkillsInstructions { - /// Creates a skills context fragment from pre-rendered catalog lines. - pub fn from_skill_lines(skill_lines: Vec) -> Self { - Self { - skill_root_lines: Vec::new(), - skill_lines, - } - } -} - -impl From for AvailableSkillsInstructions { - fn from(available_skills: AvailableSkills) -> Self { - Self { - skill_root_lines: available_skills.skill_root_lines, - skill_lines: available_skills.skill_lines, - } - } -} - -impl ContextualUserFragment for AvailableSkillsInstructions { - fn role(&self) -> &'static str { - "developer" - } - - fn markers(&self) -> (&'static str, &'static str) { - Self::type_markers() - } - - fn type_markers() -> (&'static str, &'static str) { - (SKILLS_INSTRUCTIONS_OPEN_TAG, SKILLS_INSTRUCTIONS_CLOSE_TAG) - } - - fn body(&self) -> String { - render_available_skills_body(&self.skill_root_lines, &self.skill_lines) - } -} diff --git a/codex-rs/core/src/context/mod.rs b/codex-rs/core/src/context/mod.rs index da7db0b37389..ffe0fb8d084c 100644 --- a/codex-rs/core/src/context/mod.rs +++ b/codex-rs/core/src/context/mod.rs @@ -3,7 +3,6 @@ mod approved_command_prefix_saved; mod apps_instructions; mod available_plugins_instructions; -mod available_skills_instructions; mod collaboration_mode_instructions; mod contextual_user_message; mod current_time_reminder; @@ -37,7 +36,6 @@ pub(crate) mod world_state; pub(crate) use approved_command_prefix_saved::ApprovedCommandPrefixSaved; pub(crate) use apps_instructions::AppsInstructions; pub(crate) use available_plugins_instructions::AvailablePluginsInstructions; -pub use available_skills_instructions::AvailableSkillsInstructions; pub(crate) use codex_context_fragments::AdditionalContextDeveloperFragment; pub(crate) use codex_context_fragments::AdditionalContextUserFragment; pub use codex_context_fragments::ContextualUserFragment; diff --git a/codex-rs/core/src/context/world_state/mod.rs b/codex-rs/core/src/context/world_state/mod.rs index 50b9967096d7..954f2dbc4dd3 100644 --- a/codex-rs/core/src/context/world_state/mod.rs +++ b/codex-rs/core/src/context/world_state/mod.rs @@ -1,5 +1,6 @@ mod agents_md; mod environment; +mod skills; use crate::context::ContextualUserFragment; use codex_protocol::models::ContentItem; @@ -14,6 +15,7 @@ use std::fmt; pub(crate) use agents_md::AgentsMdState; pub(crate) use environment::EnvironmentsState; +pub(crate) use skills::SkillsState; trait ErasedWorldStateSection: Send + Sync { fn snapshot(&self) -> Option; diff --git a/codex-rs/core/src/context/world_state/skills.rs b/codex-rs/core/src/context/world_state/skills.rs new file mode 100644 index 000000000000..cabe683eeaab --- /dev/null +++ b/codex-rs/core/src/context/world_state/skills.rs @@ -0,0 +1,138 @@ +use codex_core_skills::SkillsSnapshot; +use codex_core_skills::render_available_skills_body; +use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG; +use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; +use serde::Deserialize; +use serde::Serialize; + +use super::PreviousSectionState; +use super::WorldStateSection; +use crate::context::ContextualUserFragment; + +const REPLACEMENT_NOTICE: &str = + "This complete skills list replaces the previously provided skills list."; +const REMOVAL_NOTICE: &str = "The previously provided skills list no longer applies."; + +#[derive(Clone, Debug, Default)] +pub(crate) struct SkillsState { + skill_root_lines: Vec, + skill_lines: Vec, +} + +#[derive(Clone, Debug, Default, Deserialize, PartialEq, Eq, Serialize)] +pub(crate) struct SkillsStateSnapshot { + skill_root_lines: Vec, + skill_lines: Vec, +} + +impl SkillsState { + pub(crate) fn new(skills: &SkillsSnapshot, enabled: bool) -> Self { + if !enabled { + return Self::default(); + } + let Some(available) = skills.available() else { + return Self::default(); + }; + Self { + skill_root_lines: available.skill_root_lines.clone(), + skill_lines: available.skill_lines.clone(), + } + } + + fn has_catalog(&self) -> bool { + !self.skill_lines.is_empty() + } +} + +impl WorldStateSection for SkillsState { + const ID: &'static str = "skills"; + type Snapshot = SkillsStateSnapshot; + + fn snapshot(&self) -> Self::Snapshot { + SkillsStateSnapshot { + skill_root_lines: self.skill_root_lines.clone(), + skill_lines: self.skill_lines.clone(), + } + } + + fn matches_legacy_fragment(role: &str, text: &str) -> bool { + let text = text.trim(); + role == "developer" + && text.starts_with(SKILLS_INSTRUCTIONS_OPEN_TAG) + && text.ends_with(SKILLS_INSTRUCTIONS_CLOSE_TAG) + } + + fn render_diff( + &self, + previous: PreviousSectionState<'_, Self::Snapshot>, + ) -> Option> { + let current = self.snapshot(); + if matches!(previous, PreviousSectionState::Known(previous) if previous == ¤t) { + return None; + } + let previous_had_catalog = match previous { + PreviousSectionState::Known(previous) => !previous.skill_lines.is_empty(), + PreviousSectionState::Unknown => true, + PreviousSectionState::Absent => false, + }; + if !self.has_catalog() && !previous_had_catalog { + return None; + } + Some(Box::new(SkillsStateFragment { + skill_root_lines: self.skill_root_lines.clone(), + skill_lines: self.skill_lines.clone(), + include_policy: !previous_had_catalog, + notice: if !self.has_catalog() { + Some(REMOVAL_NOTICE) + } else if previous_had_catalog { + Some(REPLACEMENT_NOTICE) + } else { + None + }, + })) + } +} + +struct SkillsStateFragment { + skill_root_lines: Vec, + skill_lines: Vec, + include_policy: bool, + notice: Option<&'static str>, +} + +impl ContextualUserFragment for SkillsStateFragment { + fn role(&self) -> &'static str { + "developer" + } + + fn markers(&self) -> (&'static str, &'static str) { + Self::type_markers() + } + + fn type_markers() -> (&'static str, &'static str) { + (SKILLS_INSTRUCTIONS_OPEN_TAG, SKILLS_INSTRUCTIONS_CLOSE_TAG) + } + + fn body(&self) -> String { + let catalog = (!self.skill_lines.is_empty()).then(|| { + if self.include_policy { + render_available_skills_body(&self.skill_root_lines, &self.skill_lines) + } else { + let mut lines = vec!["## Skills".to_string()]; + if !self.skill_root_lines.is_empty() { + lines.push("### Skill roots".to_string()); + lines.extend(self.skill_root_lines.iter().cloned()); + } + lines.push("### Available skills".to_string()); + lines.extend(self.skill_lines.iter().cloned()); + format!("\n{}\n", lines.join("\n")) + } + }); + match (self.notice, catalog) { + (Some(notice), Some(catalog)) => format!("\n{notice}\n{catalog}"), + (Some(notice), None) => format!("\n{notice}\n"), + (None, Some(catalog)) => catalog, + (None, None) => String::new(), + } + } +} diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index ad5f54123fa3..53f170dddf7f 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -78,7 +78,6 @@ pub(crate) mod prompt_debug; pub use prompt_debug::build_prompt_input; pub(crate) mod mentions { pub(crate) use crate::plugins::build_connector_slug_counts; - pub(crate) use crate::plugins::build_skill_name_counts; pub(crate) use crate::plugins::collect_explicit_app_ids; pub(crate) use crate::plugins::collect_explicit_plugin_mentions; pub(crate) use crate::plugins::collect_tool_mentions_from_messages; @@ -88,14 +87,7 @@ pub mod sandboxing; mod session_prefix; mod session_startup_prewarm; pub mod skills; -pub(crate) use skills::SkillInjections; -pub(crate) use skills::SkillMetadata; pub(crate) use skills::SkillsService; -pub(crate) use skills::build_available_skills; -pub(crate) use skills::build_skill_injections; -pub(crate) use skills::build_skill_name_counts; -pub(crate) use skills::collect_explicit_skill_mentions; -pub(crate) use skills::default_skill_metadata_budget; pub(crate) use skills::injection; pub(crate) use skills::maybe_emit_implicit_skill_invocation; pub(crate) use skills::skills_load_input_from_config; diff --git a/codex-rs/core/src/mcp_skill_dependencies.rs b/codex-rs/core/src/mcp_skill_dependencies.rs index 9b2b85aa2d0d..0f8eabc08b78 100644 --- a/codex-rs/core/src/mcp_skill_dependencies.rs +++ b/codex-rs/core/src/mcp_skill_dependencies.rs @@ -15,10 +15,10 @@ use codex_rmcp_client::perform_oauth_login; use tokio_util::sync::CancellationToken; use tracing::warn; -use crate::SkillMetadata; use crate::session::session::Session; use crate::session::turn_context::TurnContext; use crate::skills::model::SkillToolDependency; +use codex_core_skills::runtime::SkillCatalogEntry; use codex_mcp::ElicitationReviewerHandle; use codex_mcp::McpOAuthLoginSupport; use codex_mcp::McpPermissionPromptAutoApproveContext; @@ -35,13 +35,13 @@ pub(crate) async fn maybe_prompt_and_install_mcp_dependencies( sess: &Session, turn_context: &TurnContext, cancellation_token: &CancellationToken, - mentioned_skills: &[SkillMetadata], + mentioned_skills: &[SkillCatalogEntry], elicitation_reviewer: Option, -) { +) -> bool { let originator_value = originator().value; if !is_first_party_originator(originator_value.as_str()) { // Only support first-party clients for now. - return; + return false; } let config = turn_context.config.clone(); @@ -50,24 +50,24 @@ pub(crate) async fn maybe_prompt_and_install_mcp_dependencies( .features .enabled(codex_features::Feature::SkillMcpDependencyInstall) { - return; + return false; } let installed = sess.runtime_mcp_servers(config.as_ref()).await; let missing = collect_missing_mcp_dependencies(mentioned_skills, &installed); if missing.is_empty() { - return; + return false; } let unprompted_missing = filter_prompted_mcp_dependencies(sess, &missing).await; if unprompted_missing.is_empty() { - return; + return false; } if should_install_mcp_dependencies(sess, turn_context, &unprompted_missing, cancellation_token) .await { - maybe_install_mcp_dependencies( + return maybe_install_mcp_dependencies( sess, turn_context, config.as_ref(), @@ -76,35 +76,36 @@ pub(crate) async fn maybe_prompt_and_install_mcp_dependencies( ) .await; } + false } pub(crate) async fn maybe_install_mcp_dependencies( sess: &Session, turn_context: &TurnContext, config: &crate::config::Config, - mentioned_skills: &[SkillMetadata], + mentioned_skills: &[SkillCatalogEntry], elicitation_reviewer: Option, -) { +) -> bool { if mentioned_skills.is_empty() || !config .features .enabled(codex_features::Feature::SkillMcpDependencyInstall) { - return; + return false; } let codex_home = config.codex_home.clone(); let installed = sess.runtime_mcp_servers(config).await; let missing = collect_missing_mcp_dependencies(mentioned_skills, &installed); if missing.is_empty() { - return; + return false; } let mut servers = match load_global_mcp_servers(&codex_home).await { Ok(servers) => servers, Err(err) => { warn!("failed to load MCP servers while installing skill dependencies: {err}"); - return; + return false; } }; @@ -120,7 +121,7 @@ pub(crate) async fn maybe_install_mcp_dependencies( } if !updated { - return; + return false; } if let Err(err) = ConfigEditsBuilder::new(&codex_home) @@ -129,7 +130,7 @@ pub(crate) async fn maybe_install_mcp_dependencies( .await { warn!("failed to persist MCP dependencies for mentioned skills: {err}"); - return; + return false; } for (name, server_config) in added { @@ -197,7 +198,7 @@ pub(crate) async fn maybe_install_mcp_dependencies( } if let Err(err) = refresh_config.mcp_servers.set(configured_servers) { warn!("failed to refresh MCP dependencies for mentioned skills: {err}"); - return; + return false; } let refresh_servers = sess.runtime_mcp_servers(&refresh_config).await; sess.refresh_mcp_servers_now( @@ -208,6 +209,7 @@ pub(crate) async fn maybe_install_mcp_dependencies( elicitation_reviewer, ) .await; + true } async fn should_install_mcp_dependencies( @@ -416,7 +418,7 @@ fn mcp_dependency_to_server_config( } fn collect_missing_mcp_dependencies( - mentioned_skills: &[SkillMetadata], + mentioned_skills: &[SkillCatalogEntry], installed: &HashMap, ) -> HashMap { let mut missing = HashMap::new(); diff --git a/codex-rs/core/src/plugins/mentions.rs b/codex-rs/core/src/plugins/mentions.rs index a5e94345cb09..016066d86d8d 100644 --- a/codex-rs/core/src/plugins/mentions.rs +++ b/codex-rs/core/src/plugins/mentions.rs @@ -102,8 +102,6 @@ pub(crate) fn collect_explicit_plugin_mentions( .collect() } -pub(crate) use crate::build_skill_name_counts; - pub(crate) fn build_connector_slug_counts( connectors: &[connectors::AppInfo], ) -> HashMap { diff --git a/codex-rs/core/src/plugins/mod.rs b/codex-rs/core/src/plugins/mod.rs index 11e2c338bc39..bdd4cfb4d787 100644 --- a/codex-rs/core/src/plugins/mod.rs +++ b/codex-rs/core/src/plugins/mod.rs @@ -12,7 +12,6 @@ pub(crate) use injection::build_plugin_injections; pub(crate) use render::render_explicit_plugin_instructions; pub(crate) use mentions::build_connector_slug_counts; -pub(crate) use mentions::build_skill_name_counts; pub(crate) use mentions::collect_explicit_app_ids; pub(crate) use mentions::collect_explicit_plugin_mentions; pub(crate) use mentions::collect_tool_mentions_from_messages; diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index f86f432a6923..f266559dca49 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -15,7 +15,6 @@ use crate::agent::AgentStatus; use crate::agent::agent_status_from_event; use crate::agent::status::is_final; use crate::attestation::AttestationProvider; -use crate::build_available_skills; use crate::compact; use crate::config::ManagedFeatures; use crate::config::resolve_tool_suggest_config_from_layer_stack; @@ -23,7 +22,6 @@ use crate::connectors; use crate::context::ApprovedCommandPrefixSaved; use crate::context::AppsInstructions; use crate::context::AvailablePluginsInstructions; -use crate::context::AvailableSkillsInstructions; use crate::context::CollaborationModeInstructions; use crate::context::ContextualUserFragment; use crate::context::MultiAgentModeInstructions; @@ -33,7 +31,6 @@ use crate::context::PersonalitySpecInstructions; use crate::context::RecommendedPluginsInstructions; use crate::context::world_state::WorldState; use crate::current_time::TimeProvider; -use crate::default_skill_metadata_budget; use crate::environment_selection::TurnEnvironmentSnapshot; use crate::exec_policy::ExecPolicyManager; use crate::image_preparation::prepare_response_items; @@ -42,7 +39,6 @@ use crate::realtime_conversation::RealtimeConversationManager; use crate::session::step_context::StepContext; use crate::session::turn_context::TurnEnvironment; use crate::session_prefix::format_inter_agent_completion_message; -use crate::skills::SkillRenderSideEffects; use crate::skills_load_input_from_config; use crate::turn_metadata::TurnMetadataState; use crate::turn_timing::now_unix_timestamp_ms; @@ -303,8 +299,6 @@ pub(crate) struct PreviousTurnSettings { pub(crate) realtime_active: Option, } -#[cfg(test)] -use crate::SkillMetadata; use crate::SkillsService; use crate::exec_policy::ExecPolicyUpdateError; use crate::guardian::GuardianReviewSessionManager; @@ -315,6 +309,8 @@ use crate::session_startup_prewarm::SessionStartupPrewarmHandle; use crate::shell; #[cfg(test)] use crate::skills::SkillLoadOutcome; +#[cfg(test)] +use crate::skills::SkillMetadata; use crate::state::AutoCompactWindowIds; use crate::state::AutoCompactWindowSnapshot; use crate::state::PendingRequestPermissions; @@ -2834,10 +2830,25 @@ impl Session { .environment_manager() .resolve_selected_capability_roots(&self.services.selected_capability_roots) .await; + let extra_skill_sources = self + .services + .thread_extension_data + .get::(); + let skills = Arc::new( + codex_core_skills::SkillsSnapshot::load( + turn_context.turn_skills.snapshot.clone(), + &selected_capability_roots, + extra_skill_sources.as_deref(), + turn_context.session_source.restriction_product(), + turn_context.model_context_window(), + ) + .await, + ); Arc::new(StepContext::new( turn_context, environments, selected_capability_roots, + skills, loaded_agents_md, )) } @@ -3209,29 +3220,6 @@ impl Session { developer_sections.push(apps_instructions.render()); } } - if turn_context.config.include_skill_instructions { - let available_skills = build_available_skills( - turn_context.turn_skills.snapshot.outcome(), - default_skill_metadata_budget(turn_context.model_info.context_window), - SkillRenderSideEffects::ThreadStart { - session_telemetry: &self.services.session_telemetry, - }, - ); - if let Some(available_skills) = available_skills { - let warning_message = available_skills.warning_message.clone(); - let skills_instructions = AvailableSkillsInstructions::from(available_skills); - if let Some(warning_message) = warning_message { - self.send_event_raw(Event { - id: String::new(), - msg: EventMsg::Warning(WarningEvent { - message: warning_message, - }), - }) - .await; - } - developer_sections.push(skills_instructions.render()); - } - } let loaded_plugins = self .services .plugins_manager diff --git a/codex-rs/core/src/session/step_context.rs b/codex-rs/core/src/session/step_context.rs index e463015459d1..d13110e281c7 100644 --- a/codex-rs/core/src/session/step_context.rs +++ b/codex-rs/core/src/session/step_context.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use crate::agents_md::LoadedAgentsMd; use crate::environment_selection::TurnEnvironmentSnapshot; use crate::session::turn_context::TurnContext; +use codex_core_skills::SkillsSnapshot; use codex_exec_server::ResolvedSelectedCapabilityRoot; /// Request-scoped state that may change between model sampling requests. @@ -12,6 +13,8 @@ pub(crate) struct StepContext { pub(crate) environments: TurnEnvironmentSnapshot, /// Capability roots bound to ready environments in this exact step. pub(crate) selected_capability_roots: Vec, + /// Complete skill catalog and exact read routes captured for this step. + pub(crate) skills: Arc, /// The canonical AGENTS.md value observed with this environment snapshot. pub(crate) loaded_agents_md: Option>, } @@ -21,12 +24,14 @@ impl StepContext { turn: Arc, environments: TurnEnvironmentSnapshot, selected_capability_roots: Vec, + skills: Arc, loaded_agents_md: Option>, ) -> Self { Self { turn, environments, selected_capability_roots, + skills, loaded_agents_md, } } diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 525fbc3b99f1..ad61c1e6bd4f 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -13,6 +13,7 @@ use crate::session::step_context::StepContext; use crate::shell::default_user_shell; use crate::shell_snapshot::ShellSnapshot; use crate::skills::SkillRenderSideEffects; +use crate::skills::build_available_skills; use crate::skills::render::SkillMetadataBudget; use crate::test_support::models_manager_with_provider; use crate::tools::format_exec_output_str; @@ -187,10 +188,15 @@ use std::time::Duration as StdDuration; impl StepContext { pub(crate) fn for_test(turn: Arc) -> Arc { let environments = turn.environments.clone(); + let skills = Arc::new(codex_core_skills::SkillsSnapshot::from_host( + turn.turn_skills.snapshot.clone(), + turn.model_context_window(), + )); Arc::new(Self::new( turn, environments, Vec::new(), + skills, /*loaded_agents_md*/ None, )) } @@ -8451,62 +8457,6 @@ fn emit_thread_start_skill_metrics_records_description_truncated_chars_without_o ); } -#[tokio::test] -async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_builds() { - let (session, turn_context, rx) = make_session_and_context_with_rx().await; - let mut turn_context = Arc::into_inner(turn_context).expect("sole thread settings owner"); - let mut outcome = SkillLoadOutcome::default(); - outcome.skills = vec![ - SkillMetadata { - name: "admin-skill".to_string(), - description: "desc".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: test_path_buf("/tmp/admin-skill/SKILL.md").abs(), - scope: SkillScope::Admin, - plugin_id: None, - }, - SkillMetadata { - name: "repo-skill".to_string(), - description: "desc".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(), - scope: SkillScope::Repo, - plugin_id: None, - }, - ]; - turn_context.model_info.context_window = Some(100); - turn_context.turn_skills = TurnSkillsContext::new(HostSkillsSnapshot::new(Arc::new(outcome))); - let turn_context = Arc::new(turn_context); - - let _ = build_initial_context(&session, &turn_context).await; - let warning_event = timeout(Duration::from_secs(1), rx.recv()) - .await - .expect("warning event should arrive") - .expect("warning event should be readable"); - assert!(matches!( - warning_event.msg, - EventMsg::Warning(WarningEvent { message }) - if message == "Exceeded skills context budget of 2%. All skill descriptions were removed and 2 additional skills were not included in the model-visible skills list." - )); - - let _ = build_initial_context(&session, &turn_context).await; - let warning_event = timeout(Duration::from_secs(1), rx.recv()) - .await - .expect("warning event should arrive on repeated build") - .expect("warning event should be readable"); - assert!(matches!( - warning_event.msg, - EventMsg::Warning(WarningEvent { message }) - if message == "Exceeded skills context budget of 2%. All skill descriptions were removed and 2 additional skills were not included in the model-visible skills list." - )); -} - #[tokio::test] async fn handle_output_item_done_records_image_save_history_message() { let (session, turn_context) = make_session_and_context().await; diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 9b3588fc1e68..1ccf5fd80431 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -4,12 +4,9 @@ use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::Ordering; -use crate::SkillInjections; -use crate::build_skill_injections; use crate::client::ModelClientSession; use crate::client_common::Prompt; use crate::client_common::ResponseEvent; -use crate::collect_explicit_skill_mentions; use crate::compact::InitialContextInjection; use crate::compact::run_inline_auto_compact_task; use crate::compact::should_use_remote_compact_task; @@ -30,7 +27,6 @@ use crate::injection::tool_kind_for_path; use crate::mcp_skill_dependencies::maybe_prompt_and_install_mcp_dependencies; use crate::mcp_tool_exposure::build_mcp_tool_exposure; use crate::mentions::build_connector_slug_counts; -use crate::mentions::build_skill_name_counts; use crate::mentions::collect_explicit_app_ids; use crate::mentions::collect_explicit_plugin_mentions; use crate::mentions::collect_tool_mentions_from_messages; @@ -71,11 +67,12 @@ use codex_analytics::AppInvocation; use codex_analytics::CompactionPhase; use codex_analytics::CompactionReason; use codex_analytics::InvocationType; +use codex_analytics::SkillInvocation; use codex_analytics::TurnResolvedConfigFact; use codex_analytics::build_track_events_context; use codex_async_utils::OrCancelExt; use codex_core_plugins::RecommendedPluginCandidatesInput; -use codex_core_skills::injection::InjectedHostSkillPrompts; +use codex_core_skills::SkillInjectionIdentity; use codex_extension_api::TurnInputContext; use codex_extension_api::TurnInputEnvironment; use codex_features::Feature; @@ -171,19 +168,28 @@ pub(crate) async fn run_turn( .record_context_updates_and_set_reference_context_item(first_step_context.as_ref()) .await; - let Some((injection_items, explicitly_enabled_connectors)) = - build_skills_and_plugins(&sess, turn_context.as_ref(), &input, &cancellation_token).await - else { - return Ok(None); - }; - if run_pending_session_start_hooks(&sess, &turn_context).await { return Ok(None); } let mut can_drain_pending_input = input.is_empty(); - if run_hooks_and_record_inputs(&sess, &turn_context, &input).await { + let (accepted_input, should_stop) = + run_hooks_and_record_inputs(&sess, &turn_context, &input).await; + if should_stop { return Ok(None); } + let mut accepted_user_input = user_input_from_turn_input(&accepted_input); + + let Some((injection_items, explicitly_enabled_connectors, available_connectors)) = + build_skills_and_plugins( + &sess, + turn_context.as_ref(), + &accepted_input, + &cancellation_token, + ) + .await + else { + return Ok(None); + }; sess.merge_connector_selection(explicitly_enabled_connectors.clone()) .await; @@ -217,6 +223,7 @@ pub(crate) async fn run_turn( // 2. After auto-compact, when model/tool continuation needs to resume before any steer. let mut next_step_context = Some(first_step_context); + let mut skill_injection_state = SkillInjectionState::default(); loop { // Note that pending_input would be something like a message the user // submitted through the UI while the model was running. Though the UI @@ -227,9 +234,12 @@ pub(crate) async fn run_turn( Vec::new() }; - if run_hooks_and_record_inputs(&sess, &turn_context, &pending_input).await { + let (accepted_pending_input, should_stop) = + run_hooks_and_record_inputs(&sess, &turn_context, &pending_input).await; + if should_stop { break; } + accepted_user_input.extend(user_input_from_turn_input(&accepted_pending_input)); let window_id = sess.current_window_id().await; super::rollout_budget::maybe_record_reminder( @@ -252,15 +262,19 @@ pub(crate) async fn run_turn( ) .await?; - if turn_context - .config - .features - .enabled(Feature::DeferredExecutor) - { - world_state = sess - .record_step_world_state_if_changed(&world_state, step_context.as_ref()) - .await; - } + world_state = sess + .record_step_world_state_if_changed(&world_state, step_context.as_ref()) + .await; + let _mcp_refreshed = record_skill_injections( + &sess, + turn_context.as_ref(), + step_context.as_ref(), + &accepted_user_input, + &available_connectors, + &cancellation_token, + &mut skill_injection_state, + ) + .await; // Construct the input that we will send to the model. let sampling_request_input: Vec = async { @@ -477,9 +491,10 @@ async fn run_hooks_and_record_inputs( sess: &Arc, turn_context: &Arc, input: &[TurnInput], -) -> bool { +) -> (Vec, bool) { let mut blocked_input = false; let mut accepted_user_input = false; + let mut accepted_input = Vec::with_capacity(input.len()); for input_item in input { let hook_outcome = inspect_pending_input(sess, turn_context, input_item).await; if hook_outcome.should_stop { @@ -496,9 +511,147 @@ async fn run_hooks_and_record_inputs( hook_outcome.additional_contexts, ) .await; + accepted_input.push(input_item.clone()); } } - blocked_input && !accepted_user_input + (accepted_input, blocked_input && !accepted_user_input) +} + +fn user_input_from_turn_input(input: &[TurnInput]) -> Vec { + input + .iter() + .filter_map(|item| match item { + TurnInput::UserInput { content, .. } => Some(content.as_slice()), + TurnInput::ResponseItem(_) | TurnInput::InterAgentCommunication(_) => None, + }) + .flatten() + .cloned() + .collect() +} + +#[derive(Default)] +struct SkillInjectionState { + injected: HashSet, + injected_names: HashSet, + emitted_warnings: HashSet, +} + +async fn record_skill_injections( + sess: &Arc, + turn_context: &TurnContext, + step_context: &StepContext, + input: &[UserInput], + available_connectors: &[connectors::AppInfo], + cancellation_token: &CancellationToken, + state: &mut SkillInjectionState, +) -> bool { + if crate::guardian::is_guardian_reviewer_source(&turn_context.session_source) { + return false; + } + for warning in step_context.skills.warnings() { + emit_skill_warning_once( + sess, + turn_context, + warning.clone(), + &mut state.emitted_warnings, + ) + .await; + } + let plain_name_conflicts = build_connector_slug_counts(available_connectors) + .into_keys() + .map(|slug| slug.to_ascii_lowercase()) + .collect(); + let injections = step_context + .skills + .injections( + input, + &plain_name_conflicts, + &state.injected, + &state.injected_names, + ) + .await; + for warning in injections.warnings { + emit_skill_warning_once(sess, turn_context, warning, &mut state.emitted_warnings).await; + } + if injections.items.is_empty() { + return false; + } + let selected_entries = injections + .items + .iter() + .map(|injection| injection.entry.clone()) + .collect::>(); + let mcp_refreshed = maybe_prompt_and_install_mcp_dependencies( + sess, + turn_context, + cancellation_token, + &selected_entries, + Some(sess.mcp_elicitation_reviewer()), + ) + .await; + + let items = injections + .items + .iter() + .map(|injection| ContextualUserFragment::into(injection.instructions.clone())) + .collect::>(); + let connector_ids = collect_explicit_app_ids_from_skill_items( + &items, + available_connectors, + &step_context.skills.skill_name_counts_lower(), + ); + sess.merge_connector_selection(connector_ids).await; + + let skill_invocations = injections + .items + .iter() + .filter_map(|injection| { + step_context + .skills + .host_skill(&injection.entry) + .map(|skill| SkillInvocation { + skill_name: skill.name.clone(), + skill_scope: skill.scope, + skill_path: skill.path_to_skills_md.to_path_buf(), + plugin_id: skill.plugin_id.clone(), + invocation_type: InvocationType::Explicit, + }) + }) + .collect::>(); + sess.services + .analytics_events_client + .track_skill_invocations( + build_track_events_context( + turn_context.model_info.slug.clone(), + sess.thread_id.to_string(), + turn_context.sub_id.clone(), + turn_context.originator.clone(), + ), + skill_invocations, + ); + for injection in injections.items { + turn_context.session_telemetry.counter( + "codex.skill.injected", + /*inc*/ 1, + &[("status", "ok"), ("skill", injection.entry.name.as_str())], + ); + state.injected_names.insert(injection.entry.name.clone()); + state.injected.insert(injection.identity); + } + sess.record_conversation_items(turn_context, &items).await; + mcp_refreshed +} + +async fn emit_skill_warning_once( + sess: &Session, + turn_context: &TurnContext, + message: String, + emitted: &mut HashSet, +) { + if emitted.insert(message.clone()) { + sess.send_event(turn_context, EventMsg::Warning(WarningEvent { message })) + .await; + } } #[instrument(level = "trace", skip_all)] @@ -507,22 +660,14 @@ async fn build_skills_and_plugins( turn_context: &TurnContext, input: &[TurnInput], cancellation_token: &CancellationToken, -) -> Option<(Vec, HashSet)> { +) -> Option<(Vec, HashSet, Vec)> { // Guardian input embeds the parent transcript as untrusted evidence. Do not interpret skill or // plugin mentions from that generated prompt as requests to inject additional instructions. if crate::guardian::is_guardian_reviewer_source(&turn_context.session_source) { - return Some((Vec::new(), HashSet::new())); + return Some((Vec::new(), HashSet::new(), Vec::new())); } - let user_input = input - .iter() - .filter_map(|item| match item { - TurnInput::UserInput { content, .. } => Some(content.as_slice()), - TurnInput::ResponseItem(_) | TurnInput::InterAgentCommunication(_) => None, - }) - .flatten() - .cloned() - .collect::>(); + let user_input = user_input_from_turn_input(input); let tracking = build_track_events_context( turn_context.model_info.slug.clone(), sess.thread_id.to_string(), @@ -572,61 +717,12 @@ async fn build_skills_and_plugins( } else { Vec::new() }; - let skills_outcome = turn_context.turn_skills.snapshot.outcome(); - let connector_slug_counts = build_connector_slug_counts(&available_connectors); let extension_injection_items = build_extension_turn_input_items(sess, turn_context, &user_input, cancellation_token) .await?; - let skill_name_counts_lower = - build_skill_name_counts(&skills_outcome.skills, &skills_outcome.disabled_paths).1; - let mentioned_skills = collect_explicit_skill_mentions( - &user_input, - &skills_outcome.skills, - &skills_outcome.disabled_paths, - &connector_slug_counts, - ); - maybe_prompt_and_install_mcp_dependencies( - sess, - turn_context, - cancellation_token, - &mentioned_skills, - Some(sess.mcp_elicitation_reviewer()), - ) - .await; - - let injected_host_skill_prompts = turn_context - .extension_data - .get::(); - let SkillInjections { - items: skill_injections, - warnings: skill_warnings, - } = build_skill_injections( - &mentioned_skills, - Some(skills_outcome), - Some(&turn_context.session_telemetry), - &sess.services.analytics_events_client, - tracking.clone(), - ) - .await; - - for message in skill_warnings { - sess.send_event(turn_context, EventMsg::Warning(WarningEvent { message })) - .await; - } - - let skill_items: Vec = skill_injections - .iter() - .map(|skill| ContextualUserFragment::into(crate::context::SkillInstructions::from(skill))) - .collect(); - let skill_connector_ids = collect_explicit_app_ids_from_skill_items( - &skill_items, - &available_connectors, - &skill_name_counts_lower, - ); let plugin_items = build_plugin_injections(&mentioned_plugins, &mcp_tools, &available_connectors); - let mut explicitly_enabled_connectors = collect_explicit_app_ids(&user_input); - explicitly_enabled_connectors.extend(skill_connector_ids); + let explicitly_enabled_connectors = collect_explicit_app_ids(&user_input); let connector_names_by_id = available_connectors .iter() .map(|connector| (connector.id.as_str(), connector.name.as_str())) @@ -656,19 +752,13 @@ async fn build_skills_and_plugins( } } - let mut injection_items: Vec = match injected_host_skill_prompts { - Some(injected_host_skill_prompts) => skill_injections - .iter() - .filter(|skill| !injected_host_skill_prompts.contains_path(&skill.path)) - .map(|skill| { - ContextualUserFragment::into(crate::context::SkillInstructions::from(skill)) - }) - .collect(), - None => skill_items, - }; - injection_items.extend(plugin_items); + let mut injection_items = plugin_items; injection_items.extend(extension_injection_items); - Some((injection_items, explicitly_enabled_connectors)) + Some(( + injection_items, + explicitly_enabled_connectors, + available_connectors, + )) } #[tracing::instrument( diff --git a/codex-rs/core/src/session/world_state.rs b/codex-rs/core/src/session/world_state.rs index cedeaab25e95..b05ccb88b914 100644 --- a/codex-rs/core/src/session/world_state.rs +++ b/codex-rs/core/src/session/world_state.rs @@ -2,6 +2,7 @@ use super::session::Session; use super::step_context::StepContext; use crate::context::world_state::AgentsMdState; use crate::context::world_state::EnvironmentsState; +use crate::context::world_state::SkillsState; use crate::context::world_state::WorldState; impl Session { @@ -25,6 +26,10 @@ impl Session { let mut world_state = WorldState::default(); world_state.add_section(AgentsMdState::new(step_context.loaded_agents_md.as_deref())); + world_state.add_section(SkillsState::new( + step_context.skills.as_ref(), + turn_context.config.include_skill_instructions, + )); if turn_context.config.include_environment_context { world_state.add_section( EnvironmentsState::from_turn_context_with_environments( diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index 6fc5adcb1175..54eabec83c6d 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -685,11 +685,16 @@ async fn environment_tools_follow_the_step_context() { turn.model_info.apply_patch_tool_type = Some(ApplyPatchToolType::Freeform); let environments = turn.environments.clone(); + let skills = Arc::new(codex_core_skills::SkillsSnapshot::from_host( + turn.turn_skills.snapshot.clone(), + turn.model_context_window(), + )); turn.environments.turn_environments.clear(); let step_context = Arc::new(StepContext::new( Arc::new(turn), environments, Vec::new(), + skills, /*loaded_agents_md*/ None, )); diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index eb4fa3b39f39..f7004bfe14a5 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -26,6 +26,7 @@ use codex_core::thread_store_from_config; use codex_exec_server::CreateDirectoryOptions; use codex_exec_server::ExecutorFileSystem; use codex_exec_server::RemoveOptions; +use codex_extension_api::ExtensionDataInit; use codex_extension_api::ExtensionRegistry; use codex_extension_api::LoadUserInstructionsFuture; use codex_extension_api::UserInstructionsProvider; @@ -287,6 +288,7 @@ pub struct TestCodexBuilder { user_shell_override: Option, exec_server_url: Option, extensions: Arc>, + thread_extension_init: ExtensionDataInit, user_instructions_provider: Option>, supports_openai_form_elicitation: bool, external_time_provider: Option>, @@ -378,6 +380,11 @@ impl TestCodexBuilder { self } + pub fn with_thread_extension_init(mut self, thread_extension_init: ExtensionDataInit) -> Self { + self.thread_extension_init = thread_extension_init; + self + } + pub fn with_user_instructions_provider( mut self, provider: Arc, @@ -665,7 +672,7 @@ impl TestCodexBuilder { metrics_service_name: None, parent_trace: None, environments, - thread_extension_init: Default::default(), + thread_extension_init: std::mem::take(&mut self.thread_extension_init), supports_openai_form_elicitation: self.supports_openai_form_elicitation, }), ) @@ -1214,6 +1221,7 @@ pub fn test_codex() -> TestCodexBuilder { user_shell_override: None, exec_server_url: None, extensions: empty_extension_registry(), + thread_extension_init: ExtensionDataInit::new(), user_instructions_provider: None, supports_openai_form_elicitation: false, external_time_provider: None, diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index 5f8c50349693..f3cebf755f5b 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -11,7 +11,10 @@ use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_exec_server::REMOTE_ENVIRONMENT_ID; use codex_exec_server::RemoveOptions; +use codex_extension_api::ExtensionDataInit; use codex_features::Feature; +use codex_protocol::capabilities::CapabilityRootLocation; +use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemAccessMode; @@ -468,6 +471,54 @@ async fn serve_environment_with_agents_md( } } +async fn serve_environment_with_skill( + listener: TcpListener, + skill_path: PathUri, + skill_contents: String, + attach: tokio::sync::oneshot::Receiver<()>, + mut shutdown: tokio::sync::oneshot::Receiver<()>, +) -> usize { + let mut websocket = accept_initialized_exec_server(listener).await; + attach.await.expect("attach signal"); + send_environment_info(&mut websocket).await; + let mut reads = 0; + let skill_path_string = skill_path.to_string(); + loop { + let request = tokio::select! { + request = read_exec_server_json(&mut websocket) => request, + _ = &mut shutdown => return reads, + }; + let response = match request["method"].as_str() { + Some("fs/walk") => json!({ + "id": request["id"], + "result": { + "entries": [{ "path": skill_path, "kind": "file" }], + "errors": [], + "truncated": false, + } + }), + Some("fs/getMetadata") => json!({ + "id": request["id"], + "error": { "code": -32004, "message": "not found" } + }), + Some("fs/readFile") + if request["params"]["path"].as_str() == Some(skill_path_string.as_str()) => + { + reads += 1; + json!({ + "id": request["id"], + "result": { "dataBase64": BASE64_STANDARD.encode(&skill_contents) } + }) + } + method => panic!("unexpected exec-server request: {method:?}"), + }; + websocket + .send(Message::Text(response.to_string().into())) + .await + .expect("filesystem response"); + } +} + fn tool_names(body: &Value) -> Vec { body["tools"] .as_array() @@ -730,6 +781,149 @@ async fn deferred_executor_loads_agents_md_when_environment_becomes_ready() -> R Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn deferred_executor_replaces_a_colliding_host_skill_for_the_next_step() -> Result<()> { + const HOST_BODY: &str = "HOST_SKILL_BODY_MARKER"; + const EXECUTOR_BODY: &str = "EXECUTOR_SKILL_BODY_MARKER"; + const SKILL_REPLACEMENT_NOTICE: &str = + "These instructions replace the previously provided instructions for this skill."; + let root_path = PathUri::from_host_native_path("/remote-capability")?; + let skill_path = root_path.join("skills/deploy/SKILL.md")?; + let skill_contents = format!( + "---\nname: deploy\ndescription: Deploy through the ready executor.\n---\n\n{EXECUTOR_BODY}\n" + ); + let mut thread_extension_init = ExtensionDataInit::new(); + thread_extension_init.insert(vec![SelectedCapabilityRoot { + id: "remote-capability@1".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: REMOTE_ENVIRONMENT_ID.to_string(), + path: root_path, + }, + }]); + + let listener = TcpListener::bind("127.0.0.1:0").await?; + let server = start_mock_server().await; + let response_mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call( + "wait-1", + "wait_for_environment", + &json!({ "environment_id": REMOTE_ENVIRONMENT_ID }).to_string(), + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-2", "done"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + let mut builder = test_codex() + .with_exec_server_url(format!("ws://{}", listener.local_addr()?)) + .with_thread_extension_init(thread_extension_init) + .with_pre_build_hook(|home| { + let skill_dir = home.join("skills/deploy"); + std::fs::create_dir_all(&skill_dir).expect("create host skill directory"); + std::fs::write( + skill_dir.join("SKILL.md"), + format!( + "---\nname: deploy\ndescription: Deploy from the host.\n---\n\n{HOST_BODY}\n" + ), + ) + .expect("write host skill"); + }) + .with_config(|config| { + config.project_doc_max_bytes = 0; + assert!(config.features.enable(Feature::DeferredExecutor).is_ok()); + }); + let (attach_tx, attach_rx) = tokio::sync::oneshot::channel(); + let (shutdown_tx, shutdown_rx) = tokio::sync::oneshot::channel(); + let exec_server = tokio::spawn(serve_environment_with_skill( + listener, + skill_path, + skill_contents, + attach_rx, + shutdown_rx, + )); + let test = timeout(Duration::from_secs(5), builder.build(&server)) + .await + .context("thread startup should not wait for the remote environment")??; + + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "Use $deploy after the environment is ready".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + }) + .await?; + wait_for_response_request_count(&response_mock, /*expected_count*/ 1).await; + attach_tx.send(()).expect("attach environment"); + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + shutdown_tx.send(()).expect("stop exec server"); + assert_eq!(exec_server.await?, 2); + + let requests = response_mock.requests(); + assert_eq!(requests.len(), 2); + assert_eq!(skill_body_occurrences(&requests[0], HOST_BODY), 1); + assert_eq!(skill_body_occurrences(&requests[0], EXECUTOR_BODY), 0); + assert_eq!(skill_body_occurrences(&requests[1], HOST_BODY), 1); + assert_eq!(skill_body_occurrences(&requests[1], EXECUTOR_BODY), 1); + assert_eq!(environment_skill_occurrences(&requests[0], "deploy"), 0); + assert_eq!(environment_skill_occurrences(&requests[1], "deploy"), 1); + let first_user_context = requests[0].message_input_texts("user"); + let host_skill = first_user_context + .iter() + .find(|text| text.starts_with("") && text.contains(HOST_BODY)) + .expect("first request should contain host skill instructions"); + assert!(!host_skill.contains(SKILL_REPLACEMENT_NOTICE)); + let second_user_context = requests[1].message_input_texts("user"); + let executor_skill = second_user_context + .iter() + .find(|text| text.starts_with("") && text.contains(EXECUTOR_BODY)) + .expect("second request should contain executor skill instructions"); + assert!(executor_skill.contains(SKILL_REPLACEMENT_NOTICE)); + let second_developer_context = requests[1].message_input_texts("developer"); + let replacement_catalog = second_developer_context + .iter() + .find(|text| text.contains("This complete skills list replaces")) + .expect("second request should contain the replacement skill catalog"); + assert!(!replacement_catalog.contains("### How to use skills")); + Ok(()) +} + +fn skill_body_occurrences(request: &ResponsesRequest, contents: &str) -> usize { + request + .message_input_texts("user") + .iter() + .filter(|text| text.starts_with("") && text.contains(contents)) + .count() +} + +fn environment_skill_occurrences(request: &ResponsesRequest, name: &str) -> usize { + request + .message_input_texts("developer") + .iter() + .filter(|text| { + text.starts_with("") + && text.contains(&format!("- {name}:")) + && text.contains("(environment resource:") + }) + .count() +} + fn agents_md_occurrences(request: &ResponsesRequest, contents: &str) -> usize { request .message_input_texts("user") diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index 8673f9f6f24f..8e85180bfbb5 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -1,49 +1,24 @@ -use std::collections::HashSet; use std::sync::Arc; -use codex_core_skills::HostSkillsSnapshot; -use codex_core_skills::SkillInstructions; -use codex_core_skills::collect_runtime_skill_mentions; -use codex_core_skills::injection::InjectedHostSkillPrompts; use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_extension_api::ConfigContributor; -use codex_extension_api::ContextContributor; -use codex_extension_api::ContextualUserFragment; use codex_extension_api::ExtensionData; -use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionFuture; use codex_extension_api::ExtensionRegistryBuilder; -use codex_extension_api::PromptFragment; use codex_extension_api::ThreadLifecycleContributor; use codex_extension_api::ThreadStartInput; use codex_extension_api::ToolCall; use codex_extension_api::ToolContributor; use codex_extension_api::ToolExecutor; -use codex_extension_api::TurnInputContext; -use codex_extension_api::TurnInputContributor; use codex_mcp::McpResourceClient; -use codex_protocol::capabilities::SelectedCapabilityRoot; -use codex_protocol::protocol::Event; -use codex_protocol::protocol::EventMsg; -use codex_protocol::protocol::WarningEvent; use crate::SkillsExtensionConfig; -use crate::catalog::SkillCatalog; -use crate::catalog::SkillCatalogEntry; -use crate::catalog::SkillReadResult; -use crate::catalog::SkillSourceKind; -use crate::provider::HostSkillProvider; -use crate::provider::SkillListQuery; -use crate::provider::SkillReadRequest; -use crate::render::available_skills_fragment; use crate::sources::SkillProviders; use crate::state::SkillsThreadState; -use crate::state::SkillsTurnState; use crate::tools::skill_tools; struct SkillsExtension { providers: SkillProviders, - event_sink: Arc, config_from_host: Arc SkillsExtensionConfig + Send + Sync>, } @@ -53,20 +28,23 @@ where { fn on_thread_start<'a>(&'a self, input: ThreadStartInput<'a, C>) -> ExtensionFuture<'a, ()> { Box::pin(async move { - let selected_roots = input - .thread_store - .get::>() - .map(|selected_roots| selected_roots.as_ref().clone()) - .unwrap_or_default(); let orchestrator_skills_available = !input .environments .iter() .any(|environment| environment.environment_id == LOCAL_ENVIRONMENT_ID); - input.thread_store.insert(SkillsThreadState::new( - (self.config_from_host)(input.config), - selected_roots, - orchestrator_skills_available, - )); + let thread_state = input.thread_store.get_or_init(|| { + SkillsThreadState::new( + (self.config_from_host)(input.config), + orchestrator_skills_available, + ) + }); + thread_state.set_config((self.config_from_host)(input.config)); + input + .thread_store + .insert(self.providers.orchestrator_sources_for_thread( + thread_state, + input.session_store.get::(), + )); }) } } @@ -77,7 +55,7 @@ where { fn on_config_changed( &self, - _session_store: &ExtensionData, + session_store: &ExtensionData, thread_store: &ExtensionData, _previous_config: &C, new_config: &C, @@ -87,60 +65,16 @@ where state.set_config(next_config); } else { let orchestrator_skills_available = true; - thread_store.insert(SkillsThreadState::new( - next_config, - Vec::new(), - orchestrator_skills_available, + let thread_state = thread_store + .get_or_init(|| SkillsThreadState::new(next_config, orchestrator_skills_available)); + thread_store.insert(self.providers.orchestrator_sources_for_thread( + thread_state, + session_store.get::(), )); } } } -impl ContextContributor for SkillsExtension -where - C: Send + Sync + 'static, -{ - fn contribute_thread_context<'a>( - &'a self, - session_store: &'a ExtensionData, - thread_store: &'a ExtensionData, - ) -> std::pin::Pin> + Send + 'a>> { - Box::pin(async move { - let Some(thread_state) = thread_store.get::() else { - return Vec::new(); - }; - let config = thread_state.config(); - if !config.include_instructions { - return Vec::new(); - } - let catalog = self - .list_skills( - SkillListQuery { - turn_id: thread_store.level_id().to_string(), - executor_roots: thread_state.selected_roots().to_vec(), - host_snapshot: None, - include_host_skills: false, - include_bundled_skills: config.bundled_skills_enabled, - include_orchestrator_skills: thread_state.orchestrator_skills_enabled(), - mcp_resources: session_store.get::(), - }, - &thread_state, - ) - .await; - for warning in &catalog.warnings { - self.emit_warning(thread_store.level_id(), warning.clone()); - } - let Some((fragment, warning)) = available_skills_fragment(&catalog) else { - return Vec::new(); - }; - if let Some(warning) = warning { - self.emit_warning(thread_store.level_id(), warning); - } - vec![PromptFragment::developer_capability(fragment.render())] - }) - } -} - impl ToolContributor for SkillsExtension where C: Send + Sync + 'static, @@ -167,190 +101,13 @@ where } } -impl TurnInputContributor for SkillsExtension -where - C: Send + Sync + 'static, -{ - fn contribute<'a>( - &'a self, - input: TurnInputContext, - session_store: &'a ExtensionData, - thread_store: &'a ExtensionData, - turn_store: &'a ExtensionData, - ) -> ExtensionFuture<'a, Vec>> { - Box::pin(async move { - let Some(thread_state) = thread_store.get::() else { - return Vec::new(); - }; - - let config = thread_state.config(); - let host_snapshot = turn_store.get::(); - let query = SkillListQuery { - turn_id: input.turn_id.clone(), - executor_roots: thread_state.selected_roots().to_vec(), - host_snapshot: host_snapshot.clone(), - include_host_skills: true, - include_bundled_skills: config.bundled_skills_enabled, - include_orchestrator_skills: thread_state.orchestrator_skills_enabled(), - mcp_resources: session_store.get::(), - }; - let catalog = self.list_skills(query, &thread_state).await; - for warning in &catalog.warnings { - self.emit_warning(&input.turn_id, warning.clone()); - } - - let plain_name_conflicts = HashSet::new(); - let selected_entries = - collect_runtime_skill_mentions(&input.user_input, &catalog, &plain_name_conflicts); - let mut warnings = catalog.warnings.clone(); - let mut fragments: Vec> = Vec::new(); - if config.include_instructions { - let mut turn_catalog = catalog.clone(); - turn_catalog.entries.retain(|entry| { - entry.authority.kind != SkillSourceKind::Executor - && entry.authority.kind != SkillSourceKind::Orchestrator - }); - if let Some((fragment, warning)) = available_skills_fragment(&turn_catalog) { - if let Some(warning) = warning { - self.emit_warning(&input.turn_id, warning.clone()); - warnings.push(warning); - } - fragments.push(Box::new(fragment)); - } - } - - let mut main_prompts_injected = false; - let mut injected_host_skill_prompts = InjectedHostSkillPrompts::default(); - for entry in &selected_entries { - match self - .read_main_prompt(entry, host_snapshot.clone(), session_store, &thread_state) - .await - { - Ok(read_result) => { - let (fragment, truncated) = - SkillInstructions::from_runtime(entry, &read_result.contents); - if truncated { - let warning = format!( - "Skill `{}` exceeded the main prompt context limit and was truncated.", - entry.name - ); - self.emit_warning(&input.turn_id, warning.clone()); - warnings.push(warning); - } - fragments.push(Box::new(fragment)); - main_prompts_injected = true; - if entry.authority.kind == SkillSourceKind::Host { - injected_host_skill_prompts.insert_path(entry.main_prompt.as_str()); - } - } - Err(message) => { - let warning = format!("Failed to load skill `{}`: {message}", entry.name); - self.emit_warning(&input.turn_id, warning.clone()); - warnings.push(warning); - } - } - } - - if let Some(host_snapshot) = &host_snapshot { - for entry in selected_entries - .iter() - .filter(|entry| entry.authority.kind != SkillSourceKind::Host) - { - for host_skill in host_snapshot - .outcome() - .skills - .iter() - .filter(|host_skill| host_skill.name == entry.name) - { - injected_host_skill_prompts - .insert_path(host_skill.path_to_skills_md.to_string_lossy()); - } - } - } - - turn_store.insert(SkillsTurnState { - catalog, - selected_entries, - warnings, - main_prompts_injected, - }); - if !injected_host_skill_prompts.is_empty() { - turn_store.insert(injected_host_skill_prompts); - } - - fragments - }) - } -} - -impl SkillsExtension { - #[tracing::instrument(level = "trace", skip_all)] - async fn list_skills( - &self, - mut query: SkillListQuery, - thread_state: &SkillsThreadState, - ) -> SkillCatalog { - let include_orchestrator_skills = query.include_orchestrator_skills; - let orchestrator_query = query.clone(); - let mcp_resources = orchestrator_query.mcp_resources.clone(); - query.include_orchestrator_skills = false; - - let mut catalog = self.providers.list_for_turn(query).await; - if include_orchestrator_skills { - let orchestrator_catalog = thread_state - .orchestrator_catalog_snapshot( - mcp_resources.as_deref(), - self.providers - .list_orchestrator_for_turn(orchestrator_query), - ) - .await; - catalog.extend(orchestrator_catalog); - } - catalog - } - - #[tracing::instrument(level = "trace", skip_all, fields(skill = %entry.name))] - async fn read_main_prompt( - &self, - entry: &SkillCatalogEntry, - host_snapshot: Option>, - session_store: &ExtensionData, - thread_state: &SkillsThreadState, - ) -> Result { - thread_state - .read_skill( - &self.providers, - SkillReadRequest { - authority: entry.authority.clone(), - package: entry.id.clone(), - resource: entry.main_prompt.clone(), - host_snapshot, - mcp_resources: session_store.get::(), - }, - ) - .await - .map_err(|err| err.message) - } - - fn emit_warning(&self, turn_id: &str, message: String) { - self.event_sink.emit(Event { - id: turn_id.to_string(), - msg: EventMsg::Warning(WarningEvent { message }), - }); - } -} - pub fn install( registry: &mut ExtensionRegistryBuilder, config_from_host: impl Fn(&C) -> SkillsExtensionConfig + Send + Sync + 'static, ) where C: Send + Sync + 'static, { - install_with_providers( - registry, - SkillProviders::new().with_host_provider(Arc::new(HostSkillProvider::new())), - config_from_host, - ); + install_with_providers(registry, SkillProviders::new(), config_from_host); } pub fn install_with_providers( @@ -362,12 +119,9 @@ pub fn install_with_providers( { let extension = Arc::new(SkillsExtension { providers, - event_sink: registry.event_sink(), config_from_host: Arc::new(config_from_host), }); registry.thread_lifecycle_contributor(extension.clone()); registry.config_contributor(extension.clone()); - registry.prompt_contributor(extension.clone()); - registry.turn_input_contributor(extension.clone()); registry.tool_contributor(extension); } diff --git a/codex-rs/ext/skills/src/fragments.rs b/codex-rs/ext/skills/src/fragments.rs deleted file mode 100644 index 19f5a4b6cb87..000000000000 --- a/codex-rs/ext/skills/src/fragments.rs +++ /dev/null @@ -1,38 +0,0 @@ -use codex_core_skills::AvailableSkills; -use codex_core_skills::render_available_skills_body; -use codex_extension_api::ContextualUserFragment; -use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG; -use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; - -#[derive(Clone, Debug, Eq, PartialEq)] -pub(crate) struct AvailableSkillsInstructions { - skill_root_lines: Vec, - skill_lines: Vec, -} - -impl From for AvailableSkillsInstructions { - fn from(available: AvailableSkills) -> Self { - Self { - skill_root_lines: available.skill_root_lines, - skill_lines: available.skill_lines, - } - } -} - -impl ContextualUserFragment for AvailableSkillsInstructions { - fn role(&self) -> &'static str { - "developer" - } - - fn markers(&self) -> (&'static str, &'static str) { - Self::type_markers() - } - - fn type_markers() -> (&'static str, &'static str) { - (SKILLS_INSTRUCTIONS_OPEN_TAG, SKILLS_INSTRUCTIONS_CLOSE_TAG) - } - - fn body(&self) -> String { - render_available_skills_body(&self.skill_root_lines, &self.skill_lines) - } -} diff --git a/codex-rs/ext/skills/src/lib.rs b/codex-rs/ext/skills/src/lib.rs index 071a6c783634..9385ea3859a3 100644 --- a/codex-rs/ext/skills/src/lib.rs +++ b/codex-rs/ext/skills/src/lib.rs @@ -1,7 +1,6 @@ pub mod catalog; mod config; mod extension; -mod fragments; pub mod provider; mod render; mod sources; diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index 7cc1680017ea..ebd3c960450e 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -1,34 +1,10 @@ use std::borrow::Cow; -use codex_core_skills::SkillRenderSideEffects; -use codex_core_skills::build_available_skills_from_catalog; -use codex_core_skills::default_skill_metadata_budget; use codex_utils_string::take_bytes_at_char_boundary; -use crate::catalog::SkillCatalog; -use crate::fragments::AvailableSkillsInstructions; - const MAX_CATALOG_SKILL_DESCRIPTION_CHARS: usize = 1_024; const TRUNCATED_SKILL_DESCRIPTION_SUFFIX: &str = "..."; -#[tracing::instrument( - level = "trace", - skip_all, - fields(catalog_entry_count = catalog.entries.len()) -)] -pub(crate) fn available_skills_fragment( - catalog: &SkillCatalog, -) -> Option<(AvailableSkillsInstructions, Option)> { - let available = build_available_skills_from_catalog( - catalog, - /*host_outcome*/ None, - default_skill_metadata_budget(/*context_window*/ None), - SkillRenderSideEffects::None, - )?; - let warning = available.warning_message.clone(); - Some((available.into(), warning)) -} - pub(crate) fn truncate_catalog_skill_description(description: &str) -> Cow<'_, str> { if description .char_indices() diff --git a/codex-rs/ext/skills/src/sources.rs b/codex-rs/ext/skills/src/sources.rs index 387b9f6566ad..e3410d01ef5a 100644 --- a/codex-rs/ext/skills/src/sources.rs +++ b/codex-rs/ext/skills/src/sources.rs @@ -18,6 +18,7 @@ use crate::provider::SkillListQuery; use crate::provider::SkillProvider; use crate::provider::SkillReadRequest; use crate::provider::SkillSearchRequest; +use crate::state::SkillsThreadState; #[derive(Clone)] pub struct SkillProviderSource { @@ -128,10 +129,6 @@ impl SkillProviders { .any(|source| source.kind == SkillSourceKind::Orchestrator) } - pub(crate) async fn list_for_turn(&self, query: SkillListQuery) -> SkillCatalog { - self.sources_for_turn(query).list().await - } - pub(crate) async fn list_orchestrator_for_turn( &self, query: SkillListQuery, @@ -141,6 +138,50 @@ impl SkillProviders { .await } + pub(crate) fn orchestrator_sources_for_thread( + &self, + thread_state: Arc, + mcp_resources: Option>, + ) -> SkillSources { + self.sources + .iter() + .filter(|source| source.kind == SkillSourceKind::Orchestrator) + .fold(SkillSources::new(), |sources, provider| { + let provider = provider.clone(); + let thread_state = Arc::clone(&thread_state); + let mcp_resources = mcp_resources.clone(); + sources.with_source_factory( + "orchestrator", + Arc::new(move || { + let mcp_resources = mcp_resources + .as_ref() + .map(|client| Arc::new(client.snapshot())); + let identity = mcp_resources + .as_ref() + .map(|client| { + SkillSourceIdentity::from_owner(client.manager_snapshot()) + }) + .unwrap_or_else(|| provider.identity.clone()); + let query = SkillListQuery { + turn_id: String::new(), + executor_roots: Vec::new(), + host_snapshot: None, + include_host_skills: false, + include_bundled_skills: false, + include_orchestrator_skills: true, + mcp_resources: mcp_resources.clone(), + }; + Arc::new(CachedOrchestratorSource { + inner: provider.bind(query), + identity, + thread_state: Arc::clone(&thread_state), + mcp_resources, + }) as Arc + }), + ) + }) + } + fn sources_for_turn(&self, query: SkillListQuery) -> SkillSources { self.sources .iter() @@ -199,6 +240,47 @@ impl SkillProviders { } } +struct CachedOrchestratorSource { + inner: Arc, + identity: SkillSourceIdentity, + thread_state: Arc, + mcp_resources: Option>, +} + +impl SkillSource for CachedOrchestratorSource { + fn kind(&self) -> SkillSourceKind { + SkillSourceKind::Orchestrator + } + + fn identity(&self) -> SkillSourceIdentity { + self.identity.clone() + } + + fn list(&self) -> SkillSourceFuture<'_, SkillCatalog> { + Box::pin(async move { + if !self.thread_state.orchestrator_skills_enabled() { + return Ok(SkillCatalog::default()); + } + Ok(self + .thread_state + .orchestrator_catalog_snapshot(self.mcp_resources.as_deref(), self.inner.list()) + .await) + }) + } + + fn read(&self, request: RuntimeSkillReadRequest) -> SkillSourceFuture<'_, SkillReadResult> { + Box::pin(async move { + self.thread_state + .read_orchestrator_source( + self.inner.as_ref(), + request, + self.mcp_resources.as_deref(), + ) + .await + }) + } +} + struct BoundSkillProvider { kind: SkillSourceKind, provider: Arc, diff --git a/codex-rs/ext/skills/src/state.rs b/codex-rs/ext/skills/src/state.rs index 0b751ebdc2e2..b3e6aaf336f8 100644 --- a/codex-rs/ext/skills/src/state.rs +++ b/codex-rs/ext/skills/src/state.rs @@ -3,15 +3,15 @@ use std::future::Future; use std::sync::Arc; use std::sync::Mutex; +use codex_core_skills::runtime::SkillReadRequest as RuntimeSkillReadRequest; +use codex_core_skills::runtime::SkillSource; use codex_mcp::McpResourceClient; use codex_mcp::McpResourceClientCacheKey; -use codex_protocol::capabilities::SelectedCapabilityRoot; use tokio::sync::OnceCell; use crate::SkillsExtensionConfig; use crate::catalog::SkillAuthority; use crate::catalog::SkillCatalog; -use crate::catalog::SkillCatalogEntry; use crate::catalog::SkillPackageId; use crate::catalog::SkillProviderError; use crate::catalog::SkillProviderResult; @@ -26,20 +26,14 @@ const MAX_CACHED_ORCHESTRATOR_CONTENT_BYTES: usize = 8 * 1024 * 1024; pub(crate) struct SkillsThreadState { config: Mutex, - selected_roots: Vec, orchestrator_skills_available: bool, orchestrator_cache: Mutex>>, } impl SkillsThreadState { - pub(crate) fn new( - config: SkillsExtensionConfig, - selected_roots: Vec, - orchestrator_skills_available: bool, - ) -> Self { + pub(crate) fn new(config: SkillsExtensionConfig, orchestrator_skills_available: bool) -> Self { Self { config: Mutex::new(config), - selected_roots, orchestrator_skills_available, orchestrator_cache: Mutex::new(None), } @@ -59,10 +53,6 @@ impl SkillsThreadState { .unwrap_or_else(std::sync::PoisonError::into_inner) = config; } - pub(crate) fn selected_roots(&self) -> &[SelectedCapabilityRoot] { - &self.selected_roots - } - pub(crate) fn orchestrator_skills_enabled(&self) -> bool { self.orchestrator_skills_available && self.config().orchestrator_skills_enabled } @@ -72,16 +62,18 @@ impl SkillsThreadState { mcp_resources: Option<&McpResourceClient>, initialize: impl Future> + Send, ) -> SkillCatalog { - self.orchestrator_cache(mcp_resources) - .catalog - .get_or_init(|| async { - initialize.await.unwrap_or_else(|err| SkillCatalog { - warnings: vec![err.message], - ..Default::default() - }) - }) - .await - .clone() + let cache = self.orchestrator_cache(mcp_resources); + if let Some(catalog) = cache.catalog.get() { + return catalog.clone(); + } + let catalog = initialize.await.unwrap_or_else(|err| SkillCatalog { + warnings: vec![err.message], + ..Default::default() + }); + if !catalog.entries.is_empty() && catalog.warnings.is_empty() { + let _ = cache.catalog.set(catalog.clone()); + } + catalog } pub(crate) async fn read_skill( @@ -93,8 +85,34 @@ impl SkillsThreadState { return providers.read(request).await; } - let cache = self.orchestrator_cache(request.mcp_resources.as_deref()); let cache_key = SkillReadCacheKey::from(&request); + let mcp_resources = request.mcp_resources.clone(); + self.read_orchestrator_cached(cache_key, mcp_resources.as_deref(), providers.read(request)) + .await + } + + pub(crate) async fn read_orchestrator_source( + &self, + source: &dyn SkillSource, + request: RuntimeSkillReadRequest, + mcp_resources: Option<&McpResourceClient>, + ) -> SkillProviderResult { + let cache_key = SkillReadCacheKey { + authority: request.authority.clone(), + package: request.package.clone(), + resource: request.resource.clone(), + }; + self.read_orchestrator_cached(cache_key, mcp_resources, source.read(request)) + .await + } + + async fn read_orchestrator_cached( + &self, + cache_key: SkillReadCacheKey, + mcp_resources: Option<&McpResourceClient>, + read: impl Future> + Send, + ) -> SkillProviderResult { + let cache = self.orchestrator_cache(mcp_resources); if let Some(result) = cache .resources .lock() @@ -104,7 +122,7 @@ impl SkillsThreadState { return Ok(result); } - let result = providers.read(request).await?; + let result = read.await?; if result.resource != cache_key.resource { return Ok(result); } @@ -196,11 +214,3 @@ impl OrchestratorResourceCache { result } } - -#[derive(Clone, Debug, Default, PartialEq, Eq)] -pub(crate) struct SkillsTurnState { - pub(crate) catalog: SkillCatalog, - pub(crate) selected_entries: Vec, - pub(crate) warnings: Vec, - pub(crate) main_prompts_injected: bool, -} diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index 761dc34d71b3..872e860e2258 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -1,34 +1,17 @@ -use std::path::PathBuf; use std::sync::Arc; -use std::sync::Mutex; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; -use codex_core_skills::HostSkillsSnapshot; -use codex_core_skills::SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS; -use codex_core_skills::SKILLS_INTRO_WITH_ABSOLUTE_PATHS; -use codex_core_skills::SkillLoadOutcome; -use codex_core_skills::SkillMetadata; -use codex_core_skills::injection::InjectedHostSkillPrompts; +use codex_core_skills::runtime::SkillSources; use codex_extension_api::ConversationHistory; use codex_extension_api::ExtensionData; -use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionRegistryBuilder; use codex_extension_api::NoopTurnItemEmitter; use codex_extension_api::ThreadStartInput; use codex_extension_api::ToolCall; use codex_extension_api::ToolPayload; -use codex_extension_api::TurnInputContext; -use codex_protocol::capabilities::CapabilityRootLocation; -use codex_protocol::capabilities::SelectedCapabilityRoot; -use codex_protocol::protocol::Event; -use codex_protocol::protocol::EventMsg; -use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG; -use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; use codex_protocol::protocol::SessionSource; -use codex_protocol::protocol::SkillScope; use codex_protocol::protocol::TruncationPolicy; -use codex_protocol::user_input::UserInput; use codex_skills_extension::SkillProviders; use codex_skills_extension::SkillsExtensionConfig; use codex_skills_extension::catalog::SkillAuthority; @@ -40,314 +23,30 @@ use codex_skills_extension::catalog::SkillReadResult; use codex_skills_extension::catalog::SkillResourceId; use codex_skills_extension::catalog::SkillSearchResult; use codex_skills_extension::catalog::SkillSourceKind; -use codex_skills_extension::install; use codex_skills_extension::install_with_providers; use codex_skills_extension::provider::SkillListQuery; use codex_skills_extension::provider::SkillProvider; use codex_skills_extension::provider::SkillProviderFuture; use codex_skills_extension::provider::SkillReadRequest; use codex_skills_extension::provider::SkillSearchRequest; -use codex_utils_absolute_path::AbsolutePathBuf; -use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; type TestResult = Result<(), Box>; -static NEXT_CODEX_HOME_ID: AtomicUsize = AtomicUsize::new(0); -const DEMO_SKILL_CONTENTS: &str = - "---\nname: demo\ndescription: Demo skill.\n---\n# Demo\n\nUse the demo skill.\n"; - -#[tokio::test] -async fn installed_extension_uses_host_service_snapshot() -> TestResult { - let codex_home = test_codex_home(); - let skill_path = codex_home.join("skills").join("demo").join("SKILL.md"); - std::fs::create_dir_all( - skill_path - .parent() - .ok_or("skill path should have a parent")?, - )?; - std::fs::write(&skill_path, DEMO_SKILL_CONTENTS)?; - let config = default_config(); - - let mut builder = ExtensionRegistryBuilder::new(); - install(&mut builder, skills_extension_config); - let registry = builder.build(); - let session_store = ExtensionData::new("session"); - let thread_store = ExtensionData::new("thread"); - let session_source = SessionSource::Cli; - registry.thread_lifecycle_contributors()[0] - .on_thread_start(ThreadStartInput { - config: &config, - session_source: &session_source, - persistent_thread_state_available: true, - environments: &[], - session_store: &session_store, - thread_store: &thread_store, - }) - .await; - - let skill_path = AbsolutePathBuf::try_from(skill_path)?; - let skill_path_string = skill_path.to_string_lossy().into_owned(); - let mut outcome = SkillLoadOutcome::default(); - outcome.skills.push(SkillMetadata { - name: "demo".to_string(), - description: "Demo skill.".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: skill_path, - scope: SkillScope::User, - plugin_id: None, - }); - let loaded_skills = Arc::new(outcome); - let skill_prompt_path = skill_path_string.replace('\\', "/"); - let turn_store = ExtensionData::new("turn-1"); - turn_store.insert(HostSkillsSnapshot::new(Arc::clone(&loaded_skills))); - - let fragments = registry.turn_input_contributors()[0] - .contribute( - TurnInputContext { - turn_id: "turn-1".to_string(), - user_input: vec![UserInput::Text { - text: "$demo".to_string(), - text_elements: Vec::new(), - }], - environments: Vec::new(), - }, - &session_store, - &thread_store, - &turn_store, - ) - .await; - - let expected_catalog = format!( - "{SKILLS_INSTRUCTIONS_OPEN_TAG}\n## Skills\n{SKILLS_INTRO_WITH_ABSOLUTE_PATHS}\n### Available skills\n- demo: Demo skill. (file: {skill_prompt_path})\n### How to use skills\n{SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS}\n{SKILLS_INSTRUCTIONS_CLOSE_TAG}" - ); - let expected_skill = format!( - "\ndemo\n{skill_prompt_path}\n{DEMO_SKILL_CONTENTS}\n" - ); - assert_eq!( - vec![("developer", expected_catalog), ("user", expected_skill),], - fragments - .iter() - .map(|fragment| (fragment.role(), fragment.render())) - .collect::>() - ); - let injected_host_skill_prompts = turn_store - .get::() - .ok_or("host skill prompt marker should be set")?; - assert!(injected_host_skill_prompts.contains_path(&skill_path_string)); - - std::fs::remove_dir_all(codex_home)?; - Ok(()) -} - #[tokio::test] -async fn selected_executor_catalog_is_context_and_selected_entrypoint_is_turn_input() -> TestResult -{ - let read_requests = Arc::new(Mutex::new(Vec::new())); - let executor_provider = Arc::new(StaticSkillProvider { +async fn skills_list_truncates_catalog_descriptions_in_tool_output() -> TestResult { + let description = "x".repeat(1_025); + let mut entry = test_entry("orchestrator/long-description"); + entry.description = description.clone(); + let (registry, session_store, thread_store) = start_extension(StaticSkillProvider { catalog: SkillCatalog { - entries: vec![test_entry( - SkillSourceKind::Executor, - "env-1", - "executor/lint-fix", - "lint-fix/SKILL.md", - )], + entries: vec![entry], warnings: Vec::new(), }, - read_requests: Arc::clone(&read_requests), list_calls: None, fail_first_list: false, - }); - let providers = SkillProviders::new().with_executor_provider(executor_provider); - let mut builder = ExtensionRegistryBuilder::new(); - install_with_providers(&mut builder, providers, skills_extension_config); - let registry = builder.build(); - - let session_store = ExtensionData::new("session"); - let thread_store = ExtensionData::new("thread"); - thread_store.insert(vec![SelectedCapabilityRoot { - id: "lint-fix".to_string(), - location: CapabilityRootLocation::Environment { - environment_id: "env-1".to_string(), - path: PathUri::parse("file:///skills/lint-fix").expect("skill root URI"), - }, - }]); - let session_source = SessionSource::Cli; - let config = default_config(); - registry.thread_lifecycle_contributors()[0] - .on_thread_start(ThreadStartInput { - config: &config, - session_source: &session_source, - persistent_thread_state_available: true, - environments: &[], - session_store: &session_store, - thread_store: &thread_store, - }) - .await; - - let prompt_fragments = registry.context_contributors()[0] - .contribute_thread_context(&session_store, &thread_store) - .await; - assert_eq!(1, prompt_fragments.len()); - assert!( - prompt_fragments[0] - .text() - .starts_with(SKILLS_INSTRUCTIONS_OPEN_TAG) - ); - assert!(prompt_fragments[0].text().contains("lint-fix")); - assert!( - prompt_fragments[0] - .text() - .contains("(environment resource: skill://executor/lint-fix/SKILL.md)") - ); - - let turn_store = ExtensionData::new("turn-1"); - let fragments = registry.turn_input_contributors()[0] - .contribute( - TurnInputContext { - turn_id: "turn-1".to_string(), - user_input: vec![UserInput::Text { - text: "$lint-fix please".to_string(), - text_elements: Vec::new(), - }], - environments: Vec::new(), - }, - &session_store, - &thread_store, - &turn_store, - ) - .await; - - assert_eq!(1, fragments.len()); - assert_eq!("user", fragments[0].role()); - assert!(fragments[0].render().contains("lint-fix")); - assert!(fragments[0].render().contains("# Lint Fix")); - assert_eq!( - vec![( - SkillAuthority::new(SkillSourceKind::Executor, "env-1"), - SkillPackageId("executor/lint-fix".to_string()), - SkillResourceId::new("lint-fix/SKILL.md"), - )], - read_request_keys(&read_requests) - ); - let rebuilt_prompt_fragments = registry.context_contributors()[0] - .contribute_thread_context(&session_store, &thread_store) - .await; - assert_eq!(1, rebuilt_prompt_fragments.len()); - assert!(rebuilt_prompt_fragments[0].text().contains("lint-fix")); - - let next_turn_store = ExtensionData::new("turn-2"); - let next_fragments = registry.turn_input_contributors()[0] - .contribute( - TurnInputContext { - turn_id: "turn-2".to_string(), - user_input: vec![UserInput::Text { - text: "no skill this time".to_string(), - text_elements: Vec::new(), - }], - environments: Vec::new(), - }, - &session_store, - &thread_store, - &next_turn_store, - ) - .await; - - assert!(next_fragments.is_empty()); - - Ok(()) -} - -#[tokio::test] -async fn default_context_truncates_catalog_descriptions() -> TestResult { - let description = "x".repeat(1_025); - let mut entry = test_entry( - SkillSourceKind::Orchestrator, - "codex_apps", - "orchestrator/long-description", - "skill://orchestrator/long-description/SKILL.md", - ); - entry.description = description.clone(); - let providers = - SkillProviders::new().with_orchestrator_provider(Arc::new(StaticSkillProvider { - catalog: SkillCatalog { - entries: vec![entry], - warnings: Vec::new(), - }, - read_requests: Arc::new(Mutex::new(Vec::new())), - list_calls: None, - fail_first_list: false, - })); - let mut builder = ExtensionRegistryBuilder::new(); - install_with_providers(&mut builder, providers, skills_extension_config); - let registry = builder.build(); - let session_store = ExtensionData::new("session"); - let thread_store = ExtensionData::new("thread"); - let session_source = SessionSource::Cli; - let config = default_config(); - registry.thread_lifecycle_contributors()[0] - .on_thread_start(ThreadStartInput { - config: &config, - session_source: &session_source, - persistent_thread_state_available: true, - environments: &[], - session_store: &session_store, - thread_store: &thread_store, - }) - .await; - - let fragments = registry.context_contributors()[0] - .contribute_thread_context(&session_store, &thread_store) - .await; - assert_eq!(1, fragments.len()); - let rendered = fragments[0].text(); - assert!(rendered.contains(&("x".repeat(1_021) + "..."))); - assert!(!rendered.contains(&"x".repeat(1_024))); - assert!(!rendered.contains(&description)); - - Ok(()) -} - -#[tokio::test] -async fn skills_list_truncates_catalog_descriptions_in_tool_output() -> TestResult { - let description = "x".repeat(1_025); - let mut entry = test_entry( - SkillSourceKind::Orchestrator, - "codex_apps", - "orchestrator/long-description", - "skill://orchestrator/long-description/SKILL.md", - ); - entry.description = description.clone(); - let providers = - SkillProviders::new().with_orchestrator_provider(Arc::new(StaticSkillProvider { - catalog: SkillCatalog { - entries: vec![entry], - warnings: Vec::new(), - }, - read_requests: Arc::new(Mutex::new(Vec::new())), - list_calls: None, - fail_first_list: false, - })); - let mut builder = ExtensionRegistryBuilder::new(); - install_with_providers(&mut builder, providers, skills_extension_config); - let registry = builder.build(); - let session_store = ExtensionData::new("session"); - let thread_store = ExtensionData::new("thread"); - let session_source = SessionSource::Cli; - let config = default_config(); - registry.thread_lifecycle_contributors()[0] - .on_thread_start(ThreadStartInput { - config: &config, - session_source: &session_source, - persistent_thread_state_available: true, - environments: &[], - session_store: &session_store, - thread_store: &thread_store, - }) - .await; + }) + .await; let tools = registry.tool_contributors()[0].tools(&session_store, &thread_store); let list_tool = tools @@ -379,207 +78,52 @@ async fn skills_list_truncates_catalog_descriptions_in_tool_output() -> TestResu assert_eq!(rendered_description, "x".repeat(1_021) + "..."); assert_ne!(rendered_description, description); - Ok(()) } #[tokio::test] -async fn orchestrator_catalog_snapshot_caches_failure() -> TestResult { +async fn transient_orchestrator_failure_is_not_cached() -> TestResult { let list_calls = Arc::new(AtomicUsize::new(0)); - let providers = - SkillProviders::new().with_orchestrator_provider(Arc::new(StaticSkillProvider { - catalog: SkillCatalog { - entries: vec![test_entry( - SkillSourceKind::Orchestrator, - "codex_apps", - "orchestrator/first", - "skill://orchestrator/first/SKILL.md", - )], - warnings: Vec::new(), - }, - read_requests: Arc::new(Mutex::new(Vec::new())), - list_calls: Some(Arc::clone(&list_calls)), - fail_first_list: true, - })); - let (event_tx, event_rx) = std::sync::mpsc::channel(); - let mut builder = - ExtensionRegistryBuilder::with_event_sink(Arc::new(ChannelEventSink(event_tx))); - install_with_providers(&mut builder, providers, skills_extension_config); - let registry = builder.build(); - let session_store = ExtensionData::new("session"); - let thread_store = ExtensionData::new("thread"); - let session_source = SessionSource::Cli; - let config = default_config(); - registry.thread_lifecycle_contributors()[0] - .on_thread_start(ThreadStartInput { - config: &config, - session_source: &session_source, - persistent_thread_state_available: true, - environments: &[], - session_store: &session_store, - thread_store: &thread_store, - }) - .await; - - let initial_fragments = registry.context_contributors()[0] - .contribute_thread_context(&session_store, &thread_store) - .await; - assert!(initial_fragments.is_empty()); - let EventMsg::Warning(warning) = event_rx.try_recv()?.msg else { - panic!("expected warning event"); - }; - assert_eq!( - warning.message, - "orchestrator skills unavailable: temporary orchestrator failure" - ); - - for turn_id in ["turn-1", "turn-2"] { - let fragments = registry.turn_input_contributors()[0] - .contribute( - TurnInputContext { - turn_id: turn_id.to_string(), - user_input: vec![UserInput::Text { - text: "$first".to_string(), - text_elements: Vec::new(), - }], - environments: Vec::new(), - }, - &session_store, - &thread_store, - &ExtensionData::new(turn_id), - ) - .await; - assert!(fragments.is_empty()); - } - assert_eq!(1, list_calls.load(Ordering::Relaxed)); - - Ok(()) -} - -#[tokio::test] -async fn root_qualified_locator_selects_only_the_matching_executor_skill() -> TestResult { - let read_requests = Arc::new(Mutex::new(Vec::new())); - let root_a_locator = "skill://root-a/shared/lint-fix/SKILL.md"; - let root_b_locator = "skill://root-b/shared/lint-fix/SKILL.md"; - let executor_provider = Arc::new(StaticSkillProvider { + let (_registry, _session_store, thread_store) = start_extension(StaticSkillProvider { catalog: SkillCatalog { - entries: [("root-a", root_a_locator), ("root-b", root_b_locator)] - .into_iter() - .map(|(root_id, locator)| { - SkillCatalogEntry::new( - SkillPackageId(locator.to_string()), - SkillAuthority::new(SkillSourceKind::Executor, root_id), - "lint-fix", - "Fix lint errors.", - SkillResourceId::new(locator), - ) - .with_display_path(locator) - }) - .collect(), + entries: vec![test_entry("orchestrator/first")], warnings: Vec::new(), }, - read_requests: Arc::clone(&read_requests), - list_calls: None, - fail_first_list: false, - }); - let providers = SkillProviders::new().with_executor_provider(executor_provider); - let mut builder = ExtensionRegistryBuilder::new(); - install_with_providers(&mut builder, providers, skills_extension_config); - let registry = builder.build(); - let session_store = ExtensionData::new("session"); - let thread_store = ExtensionData::new("thread"); - thread_store.insert( - [("root-a", "/skills/root-a"), ("root-b", "/skills/root-b")] - .into_iter() - .map(|(id, path)| SelectedCapabilityRoot { - id: id.to_string(), - location: CapabilityRootLocation::Environment { - environment_id: "env-1".to_string(), - path: PathUri::parse(&format!("file://{path}")).expect("skill root URI"), - }, - }) - .collect::>(), - ); - let session_source = SessionSource::Cli; - let config = default_config(); - registry.thread_lifecycle_contributors()[0] - .on_thread_start(ThreadStartInput { - config: &config, - session_source: &session_source, - persistent_thread_state_available: true, - environments: &[], - session_store: &session_store, - thread_store: &thread_store, - }) - .await; - - let fragments = registry.turn_input_contributors()[0] - .contribute( - TurnInputContext { - turn_id: "turn-1".to_string(), - user_input: vec![UserInput::Mention { - name: "lint-fix".to_string(), - path: root_b_locator.to_string(), - }], - environments: Vec::new(), - }, - &session_store, - &thread_store, - &ExtensionData::new("turn-1"), - ) - .await; - - assert_eq!(1, fragments.len()); - assert!(fragments[0].render().contains(root_b_locator)); - assert_eq!( - vec![( - SkillAuthority::new(SkillSourceKind::Executor, "root-b"), - SkillPackageId(root_b_locator.to_string()), - SkillResourceId::new(root_b_locator), - )], - read_request_keys(&read_requests) - ); - + list_calls: Some(Arc::clone(&list_calls)), + fail_first_list: true, + }) + .await; + let sources = thread_store + .get::() + .ok_or("orchestrator skill source should be registered")?; + + let first = sources.list().await; + let second = sources.list().await; + + assert!(first.entries.is_empty()); + assert_eq!(first.warnings.len(), 1); + assert_eq!(second.entries, vec![test_entry("orchestrator/first")]); + assert_eq!(list_calls.load(Ordering::Relaxed), 2); Ok(()) } -#[tokio::test] -async fn prompt_hidden_skill_can_still_be_invoked() -> TestResult { - let read_requests = Arc::new(Mutex::new(Vec::new())); - let provider = Arc::new(StaticSkillProvider { - catalog: SkillCatalog { - entries: vec![ - test_entry( - SkillSourceKind::Host, - "host", - "host/visible-skill", - "visible-skill/SKILL.md", - ), - test_entry( - SkillSourceKind::Host, - "host", - "host/hidden-skill", - "hidden-skill/SKILL.md", - ) - .hidden_from_prompt(), - ], - warnings: Vec::new(), - }, - read_requests: Arc::clone(&read_requests), - list_calls: None, - fail_first_list: false, - }); - let providers = SkillProviders::new().with_host_provider(provider); +async fn start_extension( + provider: StaticSkillProvider, +) -> ( + codex_extension_api::ExtensionRegistry, + ExtensionData, + ExtensionData, +) { + let providers = SkillProviders::new().with_orchestrator_provider(Arc::new(provider)); let mut builder = ExtensionRegistryBuilder::new(); install_with_providers(&mut builder, providers, skills_extension_config); let registry = builder.build(); let session_store = ExtensionData::new("session"); let thread_store = ExtensionData::new("thread"); let session_source = SessionSource::Cli; - let config = default_config(); registry.thread_lifecycle_contributors()[0] .on_thread_start(ThreadStartInput { - config: &config, + config: &TestConfig, session_source: &session_source, persistent_thread_state_available: true, environments: &[], @@ -587,55 +131,16 @@ async fn prompt_hidden_skill_can_still_be_invoked() -> TestResult { thread_store: &thread_store, }) .await; - - let fragments = registry.turn_input_contributors()[0] - .contribute( - TurnInputContext { - turn_id: "turn-1".to_string(), - user_input: vec![UserInput::Text { - text: "$hidden-skill".to_string(), - text_elements: Vec::new(), - }], - environments: Vec::new(), - }, - &session_store, - &thread_store, - &ExtensionData::new("turn-1"), - ) - .await; - - assert_eq!(2, fragments.len()); - assert!(fragments[0].render().contains("visible-skill")); - assert!(!fragments[0].render().contains("hidden-skill")); - assert!(fragments[1].render().contains("hidden-skill")); - assert_eq!( - vec![( - SkillAuthority::new(SkillSourceKind::Host, "host"), - SkillPackageId("host/hidden-skill".to_string()), - SkillResourceId::new("hidden-skill/SKILL.md"), - )], - read_request_keys(&read_requests) - ); - - Ok(()) + (registry, session_store, thread_store) } #[derive(Clone)] struct StaticSkillProvider { catalog: SkillCatalog, - read_requests: Arc>>, list_calls: Option>, fail_first_list: bool, } -struct ChannelEventSink(std::sync::mpsc::Sender); - -impl ExtensionEventSink for ChannelEventSink { - fn emit(&self, event: Event) { - let _ = self.0.send(event); - } -} - impl SkillProvider for StaticSkillProvider { fn list(&self, _query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { let list_call = self @@ -654,15 +159,10 @@ impl SkillProvider for StaticSkillProvider { } fn read(&self, request: SkillReadRequest) -> SkillProviderFuture<'_, SkillReadResult> { - let read_requests = Arc::clone(&self.read_requests); Box::pin(async move { - read_requests - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .push(request.clone()); Ok(SkillReadResult { resource: request.resource, - contents: "# Lint Fix\n\nRun the formatter.".to_string(), + contents: "# Skill".to_string(), }) }) } @@ -672,67 +172,24 @@ impl SkillProvider for StaticSkillProvider { } } -fn test_entry( - kind: SkillSourceKind, - authority_id: &str, - package_id: &str, - main_prompt: &str, -) -> SkillCatalogEntry { - let name = package_id.rsplit('/').next().unwrap_or(package_id); +fn test_entry(package_id: &str) -> SkillCatalogEntry { SkillCatalogEntry::new( SkillPackageId(package_id.to_string()), - SkillAuthority::new(kind, authority_id), - name, + SkillAuthority::new(SkillSourceKind::Orchestrator, "codex_apps"), + package_id.rsplit('/').next().unwrap_or(package_id), "Fix lint errors.", - SkillResourceId::new(main_prompt), + SkillResourceId::new(format!("skill://{package_id}/SKILL.md")), ) .with_display_path(format!("skill://{package_id}/SKILL.md")) } -#[derive(Clone, Debug, Eq, PartialEq)] -struct TestConfig { - include_instructions: bool, - bundled_skills_enabled: bool, - orchestrator_skills_enabled: bool, -} +#[derive(Clone, Debug)] +struct TestConfig; -fn default_config() -> TestConfig { - TestConfig { +fn skills_extension_config(_config: &TestConfig) -> SkillsExtensionConfig { + SkillsExtensionConfig { include_instructions: true, bundled_skills_enabled: true, orchestrator_skills_enabled: true, } } - -fn skills_extension_config(config: &TestConfig) -> SkillsExtensionConfig { - SkillsExtensionConfig { - include_instructions: config.include_instructions, - bundled_skills_enabled: config.bundled_skills_enabled, - orchestrator_skills_enabled: config.orchestrator_skills_enabled, - } -} - -fn test_codex_home() -> PathBuf { - let id = NEXT_CODEX_HOME_ID.fetch_add(1, Ordering::Relaxed); - std::env::temp_dir().join(format!( - "codex-skills-extension-test-{}-{id}", - std::process::id(), - )) -} - -fn read_request_keys( - requests: &Arc>>, -) -> Vec<(SkillAuthority, SkillPackageId, SkillResourceId)> { - requests - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .iter() - .map(|request| { - ( - request.authority.clone(), - request.package.clone(), - request.resource.clone(), - ) - }) - .collect() -}