From b991d3854dfd6b210454ec0ca36a63cdb8210c87 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Wed, 17 Jun 2026 19:21:07 +0200 Subject: [PATCH] Move explicit skill invocation into SkillsExtension --- codex-rs/Cargo.lock | 3 +- .../tests/suite/v2/executor_skills.rs | 69 +++- codex-rs/codex-mcp/src/dependencies.rs | 161 +++++++++ codex-rs/codex-mcp/src/dependencies_tests.rs | 31 ++ codex-rs/codex-mcp/src/lib.rs | 4 + codex-rs/connectors/src/explicit.rs | 54 ++++ codex-rs/connectors/src/explicit_tests.rs | 49 +++ codex-rs/connectors/src/lib.rs | 2 + codex-rs/core-skills/Cargo.toml | 1 - codex-rs/core-skills/src/injection.rs | 123 ------- .../core-skills/src/skill_instructions.rs | 12 - codex-rs/core/src/lib.rs | 8 - codex-rs/core/src/mcp_skill_dependencies.rs | 186 +---------- codex-rs/core/src/plugins/mentions.rs | 38 +-- codex-rs/core/src/plugins/mod.rs | 3 - codex-rs/core/src/session/mod.rs | 6 +- codex-rs/core/src/session/review.rs | 1 + codex-rs/core/src/session/session.rs | 1 + codex-rs/core/src/session/tests.rs | 74 ----- codex-rs/core/src/session/turn.rs | 138 ++------ codex-rs/core/src/skills.rs | 2 - .../src/contributors/turn_input.rs | 8 + codex-rs/ext/skills/Cargo.toml | 2 + codex-rs/ext/skills/src/extension.rs | 142 ++++++-- codex-rs/ext/skills/src/selection.rs | 22 +- codex-rs/ext/skills/tests/skills_extension.rs | 305 +++++++++++++++++- 26 files changed, 846 insertions(+), 599 deletions(-) create mode 100644 codex-rs/codex-mcp/src/dependencies.rs create mode 100644 codex-rs/codex-mcp/src/dependencies_tests.rs create mode 100644 codex-rs/connectors/src/explicit.rs create mode 100644 codex-rs/connectors/src/explicit_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 010be9282d..7fb1a0dae2 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2793,7 +2793,6 @@ name = "codex-core-skills" version = "0.0.0" dependencies = [ "anyhow", - "codex-analytics", "codex-app-server-protocol", "codex-config", "codex-context-fragments", @@ -3885,6 +3884,8 @@ dependencies = [ name = "codex-skills-extension" version = "0.0.0" dependencies = [ + "codex-analytics", + "codex-connectors", "codex-core-skills", "codex-exec-server", "codex-extension-api", diff --git a/codex-rs/app-server/tests/suite/v2/executor_skills.rs b/codex-rs/app-server/tests/suite/v2/executor_skills.rs index b6e9edde2c..897768202c 100644 --- a/codex-rs/app-server/tests/suite/v2/executor_skills.rs +++ b/codex-rs/app-server/tests/suite/v2/executor_skills.rs @@ -23,13 +23,20 @@ const LOCAL_SKILL_MARKER: &str = "LOCAL_SKILL_BODY_MARKER"; #[tokio::test] async fn selected_executor_root_exposes_plugin_skill() -> Result<()> { let server = responses::start_mock_server().await; - let response_mock = responses::mount_sse_once( + let response_mock = responses::mount_sse_sequence( &server, - responses::sse(vec![ - responses::ev_response_created("resp-selected"), - responses::ev_assistant_message("msg-selected", "Done"), - responses::ev_completed("resp-selected"), - ]), + vec![ + responses::sse(vec![ + responses::ev_response_created("resp-selected"), + responses::ev_assistant_message("msg-selected", "Done"), + responses::ev_completed("resp-selected"), + ]), + responses::sse(vec![ + responses::ev_response_created("resp-host"), + responses::ev_assistant_message("msg-host", "Done"), + responses::ev_completed("resp-host"), + ]), + ], ) .await; @@ -145,5 +152,55 @@ stream_max_retries = 0 assert!(skill_fragment.contains(SKILL_MARKER)); assert!(!skill_fragment.contains(LOCAL_SKILL_MARKER)); + let request_id = app_server + .send_thread_start_request(ThreadStartParams { + model: Some("mock-model".to_string()), + ..Default::default() + }) + .await?; + let response: JSONRPCResponse = timeout( + READ_TIMEOUT, + app_server.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response(response)?; + let request_id = app_server + .send_turn_start_request(TurnStartParams { + thread_id: thread.id, + input: vec![UserInput::Text { + text: format!("Use ${SKILL_NAME}"), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + timeout( + READ_TIMEOUT, + app_server.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + timeout( + READ_TIMEOUT, + app_server.read_stream_until_notification_message("turn/completed"), + ) + .await??; + + let host_request = response_mock + .requests() + .last() + .cloned() + .expect("host skill request should be captured"); + let host_skill_fragments = host_request + .message_input_texts("user") + .into_iter() + .filter(|text| text.starts_with("")) + .collect::>(); + assert_eq!(1, host_skill_fragments.len()); + let host_skill_fragment = host_skill_fragments + .first() + .expect("host skill instructions should be model-visible"); + assert!(host_skill_fragment.contains(LOCAL_SKILL_MARKER)); + assert!(!host_skill_fragment.contains(SKILL_MARKER)); + Ok(()) } diff --git a/codex-rs/codex-mcp/src/dependencies.rs b/codex-rs/codex-mcp/src/dependencies.rs new file mode 100644 index 0000000000..f727ea5222 --- /dev/null +++ b/codex-rs/codex-mcp/src/dependencies.rs @@ -0,0 +1,161 @@ +use std::collections::HashMap; +use std::collections::HashSet; + +use codex_config::McpServerConfig; +use codex_config::McpServerTransportConfig; +use tracing::warn; + +/// One MCP server requested by a capability selected for the current turn. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct McpServerDependency { + pub source_name: String, + pub name: String, + pub transport: Option, + pub command: Option, + pub url: Option, +} + +/// Turn-scoped MCP dependencies contributed by capability owners. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct McpServerDependencies { + dependencies: Vec, +} + +impl McpServerDependencies { + pub fn push(&mut self, dependency: McpServerDependency) { + self.dependencies.push(dependency); + } + + pub fn is_empty(&self) -> bool { + self.dependencies.is_empty() + } + + pub fn missing_from( + &self, + installed: &HashMap, + ) -> HashMap { + let mut missing = HashMap::new(); + let installed_keys = installed + .iter() + .map(|(name, config)| canonical_server_key(name, config)) + .collect::>(); + let mut seen_keys = HashSet::new(); + + for dependency in &self.dependencies { + let dependency_key = match canonical_dependency_key(dependency) { + Ok(key) => key, + Err(err) => { + warn!( + "unable to auto-install MCP dependency {} for {}: {err}", + dependency.name, dependency.source_name + ); + continue; + } + }; + if installed_keys.contains(&dependency_key) || !seen_keys.insert(dependency_key.clone()) + { + continue; + } + + match dependency_to_server_config(dependency) { + Ok(config) => { + missing.insert(dependency.name.clone(), config); + } + Err(err) => warn!( + "unable to auto-install MCP dependency {dependency_key} for {}: {err}", + dependency.source_name + ), + } + } + missing + } +} + +pub fn canonical_server_key(name: &str, config: &McpServerConfig) -> String { + match &config.transport { + McpServerTransportConfig::Stdio { command, .. } => canonical_key("stdio", command, name), + McpServerTransportConfig::StreamableHttp { url, .. } => { + canonical_key("streamable_http", url, name) + } + } +} + +fn canonical_dependency_key(dependency: &McpServerDependency) -> Result { + let transport = dependency.transport.as_deref().unwrap_or("streamable_http"); + if transport.eq_ignore_ascii_case("streamable_http") { + let url = dependency + .url + .as_ref() + .ok_or_else(|| "missing url for streamable_http dependency".to_string())?; + return Ok(canonical_key("streamable_http", url, &dependency.name)); + } + if transport.eq_ignore_ascii_case("stdio") { + let command = dependency + .command + .as_ref() + .ok_or_else(|| "missing command for stdio dependency".to_string())?; + return Ok(canonical_key("stdio", command, &dependency.name)); + } + Err(format!("unsupported transport {transport}")) +} + +fn dependency_to_server_config( + dependency: &McpServerDependency, +) -> Result { + let transport = dependency.transport.as_deref().unwrap_or("streamable_http"); + let transport = if transport.eq_ignore_ascii_case("streamable_http") { + McpServerTransportConfig::StreamableHttp { + url: dependency + .url + .clone() + .ok_or_else(|| "missing url for streamable_http dependency".to_string())?, + bearer_token_env_var: None, + http_headers: None, + env_http_headers: None, + } + } else if transport.eq_ignore_ascii_case("stdio") { + McpServerTransportConfig::Stdio { + command: dependency + .command + .clone() + .ok_or_else(|| "missing command for stdio dependency".to_string())?, + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: None, + } + } else { + return Err(format!("unsupported transport {transport}")); + }; + + Ok(McpServerConfig { + transport, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: None, + oauth_resource: None, + tools: HashMap::new(), + }) +} + +fn canonical_key(transport: &str, identifier: &str, fallback: &str) -> String { + let identifier = identifier.trim(); + if identifier.is_empty() { + fallback.to_string() + } else { + format!("mcp__{transport}__{identifier}") + } +} + +#[cfg(test)] +#[path = "dependencies_tests.rs"] +mod tests; diff --git a/codex-rs/codex-mcp/src/dependencies_tests.rs b/codex-rs/codex-mcp/src/dependencies_tests.rs new file mode 100644 index 0000000000..966ed8e4d9 --- /dev/null +++ b/codex-rs/codex-mcp/src/dependencies_tests.rs @@ -0,0 +1,31 @@ +use std::collections::HashMap; + +use pretty_assertions::assert_eq; + +use super::McpServerDependencies; +use super::McpServerDependency; +use super::canonical_server_key; + +#[test] +fn missing_from_deduplicates_by_transport_identity() { + let dependency = McpServerDependency { + source_name: "demo".to_string(), + name: "docs".to_string(), + transport: Some("streamable_http".to_string()), + command: None, + url: Some("https://example.com/mcp".to_string()), + }; + let mut dependencies = McpServerDependencies::default(); + dependencies.push(dependency.clone()); + dependencies.push(dependency); + + let missing = dependencies.missing_from(&HashMap::new()); + + assert_eq!(missing.len(), 1); + assert_eq!( + missing + .get("docs") + .map(|config| canonical_server_key("docs", config)), + Some("mcp__streamable_http__https://example.com/mcp".to_string()) + ); +} diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index b56a4f9072..af798cfd6f 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -1,5 +1,8 @@ pub use connection_manager::McpConnectionManager; pub use connection_manager::tool_is_model_visible; +pub use dependencies::McpServerDependencies; +pub use dependencies::McpServerDependency; +pub use dependencies::canonical_server_key; pub use elicitation::ElicitationReviewRequest; pub use elicitation::ElicitationReviewer; pub use elicitation::ElicitationReviewerHandle; @@ -74,6 +77,7 @@ pub(crate) mod auth_elicitation; mod catalog; pub(crate) mod codex_apps; pub(crate) mod connection_manager; +mod dependencies; pub(crate) mod elicitation; pub(crate) mod mcp; mod plugin_config; diff --git a/codex-rs/connectors/src/explicit.rs b/codex-rs/connectors/src/explicit.rs new file mode 100644 index 0000000000..332eedc641 --- /dev/null +++ b/codex-rs/connectors/src/explicit.rs @@ -0,0 +1,54 @@ +use std::collections::HashMap; +use std::collections::HashSet; + +use codex_app_server_protocol::AppInfo; + +use crate::metadata::connector_mention_slug; + +/// Connector references discovered by a capability owner while preparing turn input. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct ExplicitConnectorMentions { + connector_ids: HashSet, + plain_names: HashSet, +} + +impl ExplicitConnectorMentions { + pub fn insert_connector_id(&mut self, connector_id: impl Into) { + self.connector_ids.insert(connector_id.into()); + } + + pub fn insert_plain_name(&mut self, name: impl Into) { + self.plain_names.insert(name.into().to_ascii_lowercase()); + } + + pub fn extend(&mut self, other: Self) { + self.connector_ids.extend(other.connector_ids); + self.plain_names.extend(other.plain_names); + } + + pub fn is_empty(&self) -> bool { + self.connector_ids.is_empty() && self.plain_names.is_empty() + } + + pub fn resolve(&self, connectors: &[AppInfo]) -> HashSet { + let slug_counts = connectors + .iter() + .fold(HashMap::new(), |mut counts, connector| { + *counts + .entry(connector_mention_slug(connector)) + .or_insert(0usize) += 1; + counts + }); + let mut connector_ids = self.connector_ids.clone(); + connector_ids.extend(connectors.iter().filter_map(|connector| { + let slug = connector_mention_slug(connector); + (slug_counts.get(&slug) == Some(&1) && self.plain_names.contains(&slug)) + .then(|| connector.id.clone()) + })); + connector_ids + } +} + +#[cfg(test)] +#[path = "explicit_tests.rs"] +mod tests; diff --git a/codex-rs/connectors/src/explicit_tests.rs b/codex-rs/connectors/src/explicit_tests.rs new file mode 100644 index 0000000000..cc5fd301e3 --- /dev/null +++ b/codex-rs/connectors/src/explicit_tests.rs @@ -0,0 +1,49 @@ +use codex_app_server_protocol::AppInfo; +use pretty_assertions::assert_eq; + +use super::ExplicitConnectorMentions; + +#[test] +fn resolve_keeps_ids_and_unambiguous_plain_names() { + let connectors = vec![connector("calendar-1", "Google Calendar")]; + let mut mentions = ExplicitConnectorMentions::default(); + mentions.insert_connector_id("direct"); + mentions.insert_plain_name("google-calendar"); + + assert_eq!( + mentions.resolve(&connectors), + ["calendar-1".to_string(), "direct".to_string()] + .into_iter() + .collect() + ); +} + +#[test] +fn resolve_skips_ambiguous_plain_names() { + let connectors = vec![ + connector("calendar-1", "Google Calendar"), + connector("calendar-2", "Google Calendar"), + ]; + let mut mentions = ExplicitConnectorMentions::default(); + mentions.insert_plain_name("google-calendar"); + + assert_eq!(mentions.resolve(&connectors), Default::default()); +} + +fn connector(id: &str, name: &str) -> AppInfo { + AppInfo { + id: id.to_string(), + name: name.to_string(), + description: None, + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + branding: None, + app_metadata: None, + labels: None, + install_url: None, + is_accessible: true, + is_enabled: true, + plugin_display_names: Vec::new(), + } +} diff --git a/codex-rs/connectors/src/lib.rs b/codex-rs/connectors/src/lib.rs index a75120c0a4..c478a7d5f3 100644 --- a/codex-rs/connectors/src/lib.rs +++ b/codex-rs/connectors/src/lib.rs @@ -14,6 +14,7 @@ use serde::Serialize; pub mod accessible; mod app_tool_policy; mod directory_cache; +mod explicit; pub mod filter; pub mod merge; pub mod metadata; @@ -24,6 +25,7 @@ pub use app_tool_policy::AppToolPolicyInput; pub use app_tool_policy::app_is_enabled; pub use app_tool_policy::apps_config_from_layer_stack; pub use directory_cache::ConnectorDirectoryCacheContext; +pub use explicit::ExplicitConnectorMentions; pub const CONNECTORS_CACHE_TTL: Duration = Duration::from_secs(3600); diff --git a/codex-rs/core-skills/Cargo.toml b/codex-rs/core-skills/Cargo.toml index 7748540ac1..3bd2c38e61 100644 --- a/codex-rs/core-skills/Cargo.toml +++ b/codex-rs/core-skills/Cargo.toml @@ -14,7 +14,6 @@ workspace = true [dependencies] anyhow = { workspace = true } -codex-analytics = { workspace = true } codex-app-server-protocol = { workspace = true } codex-config = { workspace = true } codex-context-fragments = { workspace = true } diff --git a/codex-rs/core-skills/src/injection.rs b/codex-rs/core-skills/src/injection.rs index e7d1fd3749..5c96d2ac89 100644 --- a/codex-rs/core-skills/src/injection.rs +++ b/codex-rs/core-skills/src/injection.rs @@ -1,135 +1,12 @@ use std::collections::HashMap; use std::collections::HashSet; -use std::sync::Arc; -use crate::SkillLoadOutcome; use crate::SkillMetadata; use crate::build_skill_name_counts; -use codex_analytics::AnalyticsEventsClient; -use codex_analytics::InvocationType; -use codex_analytics::SkillInvocation; -use codex_analytics::TrackEventsContext; -use codex_exec_server::LOCAL_FS; -use codex_otel::SessionTelemetry; use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; -use codex_utils_path_uri::PathUri; use codex_utils_plugins::mention_syntax::TOOL_MENTION_SIGIL; -#[derive(Debug, Default)] -pub struct SkillInjections { - pub items: Vec, - pub warnings: Vec, -} - -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct SkillInjection { - pub name: String, - pub path: String, - pub contents: String, -} - -/// Host skill prompts that have already been injected by an extension for this -/// turn. -/// -/// Core uses this to keep the legacy skill-injection path from sending the same -/// host `SKILL.md` body again while the skills extension is being wired in. -#[derive(Debug, Clone, Default, PartialEq, Eq)] -pub struct InjectedHostSkillPrompts { - paths: HashSet, -} - -impl InjectedHostSkillPrompts { - pub fn insert_path(&mut self, path: impl Into) { - let path = path.into(); - self.paths.insert(normalize_host_skill_path(&path)); - self.paths.insert(path); - } - - pub fn is_empty(&self) -> bool { - self.paths.is_empty() - } - - pub fn contains_path(&self, path: &str) -> bool { - self.paths.contains(path) || self.paths.contains(&normalize_host_skill_path(path)) - } -} - -pub async fn build_skill_injections( - mentioned_skills: &[SkillMetadata], - loaded_skills: Option<&SkillLoadOutcome>, - otel: Option<&SessionTelemetry>, - analytics_client: &AnalyticsEventsClient, - tracking: TrackEventsContext, -) -> SkillInjections { - if mentioned_skills.is_empty() { - return SkillInjections::default(); - } - - let mut result = SkillInjections { - items: Vec::with_capacity(mentioned_skills.len()), - warnings: Vec::new(), - }; - let mut invocations = Vec::new(); - - for skill in mentioned_skills { - let fs = loaded_skills - .and_then(|outcome| outcome.file_system_for_skill(skill)) - .unwrap_or_else(|| Arc::clone(&LOCAL_FS)); - let path = PathUri::from_abs_path(&skill.path_to_skills_md); - match fs.read_file_text(&path, /*sandbox*/ None).await { - Ok(contents) => { - emit_skill_injected_metric(otel, skill, "ok"); - invocations.push(SkillInvocation { - skill_name: skill.name.clone(), - skill_scope: skill.scope, - skill_path: skill.path_to_skills_md.to_path_buf(), - plugin_id: skill.plugin_id.clone(), - invocation_type: InvocationType::Explicit, - }); - result.items.push(SkillInjection { - name: skill.name.clone(), - path: skill.path_to_skills_md.to_string_lossy().into_owned(), - contents, - }); - } - Err(err) => { - emit_skill_injected_metric(otel, skill, "error"); - let message = format!( - "Failed to load skill {name} at {path}: {err:#}", - name = skill.name, - path = skill.path_to_skills_md.display() - ); - result.warnings.push(message); - } - } - } - - analytics_client.track_skill_invocations(tracking, invocations); - - result -} - -fn normalize_host_skill_path(path: &str) -> String { - normalize_skill_path(path).replace('\\', "/") -} - -fn emit_skill_injected_metric( - otel: Option<&SessionTelemetry>, - skill: &SkillMetadata, - status: &str, -) { - let Some(otel) = otel else { - return; - }; - - otel.counter( - "codex.skill.injected", - /*inc*/ 1, - &[("status", status), ("skill", skill.name.as_str())], - ); -} - /// Collect explicitly mentioned skills from structured and text mentions. /// /// Structured `UserInput::Skill` selections are resolved first by path against diff --git a/codex-rs/core-skills/src/skill_instructions.rs b/codex-rs/core-skills/src/skill_instructions.rs index b2a002d902..a4b84c650b 100644 --- a/codex-rs/core-skills/src/skill_instructions.rs +++ b/codex-rs/core-skills/src/skill_instructions.rs @@ -1,7 +1,5 @@ use codex_context_fragments::ContextualUserFragment; -use crate::injection::SkillInjection; - #[derive(Debug, Clone, PartialEq)] pub struct SkillInstructions { name: String, @@ -9,16 +7,6 @@ pub struct SkillInstructions { contents: String, } -impl From<&SkillInjection> for SkillInstructions { - fn from(skill: &SkillInjection) -> Self { - Self { - name: skill.name.clone(), - path: skill.path.clone(), - contents: skill.contents.clone(), - } - } -} - impl ContextualUserFragment for SkillInstructions { fn role(&self) -> &'static str { "user" diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 0ae718e045..2a3751fa79 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -75,23 +75,15 @@ pub(crate) mod prompt_debug; #[doc(hidden)] 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; } mod sandbox_tags; 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_skill_injections; -pub(crate) use skills::build_skill_name_counts; -pub(crate) use skills::collect_explicit_skill_mentions; 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 32e95b80ee..794ade40ce 100644 --- a/codex-rs/core/src/mcp_skill_dependencies.rs +++ b/codex-rs/core/src/mcp_skill_dependencies.rs @@ -1,9 +1,7 @@ use std::collections::HashMap; -use std::collections::HashSet; use codex_config::ConfigEditsBuilder; use codex_config::McpServerConfig; -use codex_config::McpServerTransportConfig; use codex_config::load_global_mcp_servers; use codex_login::default_client::is_first_party_originator; use codex_login::default_client::originator; @@ -15,13 +13,13 @@ 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_mcp::ElicitationReviewerHandle; use codex_mcp::McpOAuthLoginSupport; use codex_mcp::McpPermissionPromptAutoApproveContext; +use codex_mcp::McpServerDependencies; +use codex_mcp::canonical_server_key; use codex_mcp::mcp_permission_prompt_is_auto_approved; use codex_mcp::oauth_login_support; use codex_mcp::resolve_oauth_scopes; @@ -35,7 +33,7 @@ pub(crate) async fn maybe_prompt_and_install_mcp_dependencies( sess: &Session, turn_context: &TurnContext, cancellation_token: &CancellationToken, - mentioned_skills: &[SkillMetadata], + dependencies: &McpServerDependencies, elicitation_reviewer: Option, ) { let originator_value = originator().value; @@ -45,7 +43,7 @@ pub(crate) async fn maybe_prompt_and_install_mcp_dependencies( } let config = turn_context.config.clone(); - if mentioned_skills.is_empty() + if dependencies.is_empty() || !config .features .enabled(codex_features::Feature::SkillMcpDependencyInstall) @@ -54,7 +52,7 @@ pub(crate) async fn maybe_prompt_and_install_mcp_dependencies( } let installed = sess.runtime_mcp_servers(config.as_ref()).await; - let missing = collect_missing_mcp_dependencies(mentioned_skills, &installed); + let missing = dependencies.missing_from(&installed); if missing.is_empty() { return; } @@ -71,7 +69,7 @@ pub(crate) async fn maybe_prompt_and_install_mcp_dependencies( sess, turn_context, config.as_ref(), - mentioned_skills, + dependencies, elicitation_reviewer, ) .await; @@ -82,10 +80,10 @@ pub(crate) async fn maybe_install_mcp_dependencies( sess: &Session, turn_context: &TurnContext, config: &crate::config::Config, - mentioned_skills: &[SkillMetadata], + dependencies: &McpServerDependencies, elicitation_reviewer: Option, ) { - if mentioned_skills.is_empty() + if dependencies.is_empty() || !config .features .enabled(codex_features::Feature::SkillMcpDependencyInstall) @@ -95,7 +93,7 @@ pub(crate) async fn maybe_install_mcp_dependencies( let codex_home = config.codex_home.clone(); let installed = sess.runtime_mcp_servers(config).await; - let missing = collect_missing_mcp_dependencies(mentioned_skills, &installed); + let missing = dependencies.missing_from(&installed); if missing.is_empty() { return; } @@ -280,7 +278,7 @@ async fn should_install_mcp_dependencies( let prompted_keys = missing .iter() - .map(|(name, config)| canonical_mcp_server_key(name, config)); + .map(|(name, config)| canonical_server_key(name, config)); sess.record_mcp_dependency_prompted(prompted_keys).await; install @@ -297,7 +295,7 @@ async fn filter_prompted_mcp_dependencies( missing .iter() - .filter(|(name, config)| !prompted.contains(&canonical_mcp_server_key(name, config))) + .filter(|(name, config)| !prompted.contains(&canonical_server_key(name, config))) .map(|(name, config)| (name.clone(), config.clone())) .collect() } @@ -307,165 +305,3 @@ fn format_missing_mcp_dependencies(missing: &HashMap) - names.sort(); names.join(", ") } - -fn canonical_mcp_key(transport: &str, identifier: &str, fallback: &str) -> String { - let identifier = identifier.trim(); - if identifier.is_empty() { - fallback.to_string() - } else { - format!("mcp__{transport}__{identifier}") - } -} - -fn canonical_mcp_server_key(name: &str, config: &McpServerConfig) -> String { - match &config.transport { - McpServerTransportConfig::Stdio { command, .. } => { - canonical_mcp_key("stdio", command, name) - } - McpServerTransportConfig::StreamableHttp { url, .. } => { - canonical_mcp_key("streamable_http", url, name) - } - } -} - -fn canonical_mcp_dependency_key(dependency: &SkillToolDependency) -> Result { - let transport = dependency.transport.as_deref().unwrap_or("streamable_http"); - if transport.eq_ignore_ascii_case("streamable_http") { - let url = dependency - .url - .as_ref() - .ok_or_else(|| "missing url for streamable_http dependency".to_string())?; - return Ok(canonical_mcp_key("streamable_http", url, &dependency.value)); - } - if transport.eq_ignore_ascii_case("stdio") { - let command = dependency - .command - .as_ref() - .ok_or_else(|| "missing command for stdio dependency".to_string())?; - return Ok(canonical_mcp_key("stdio", command, &dependency.value)); - } - Err(format!("unsupported transport {transport}")) -} - -fn mcp_dependency_to_server_config( - dependency: &SkillToolDependency, -) -> Result { - let transport = dependency.transport.as_deref().unwrap_or("streamable_http"); - if transport.eq_ignore_ascii_case("streamable_http") { - let url = dependency - .url - .as_ref() - .ok_or_else(|| "missing url for streamable_http dependency".to_string())?; - return Ok(McpServerConfig { - transport: McpServerTransportConfig::StreamableHttp { - url: url.clone(), - bearer_token_env_var: None, - http_headers: None, - env_http_headers: None, - }, - environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), - enabled: true, - required: false, - supports_parallel_tool_calls: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - default_tools_approval_mode: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth: None, - oauth_resource: None, - tools: HashMap::new(), - }); - } - - if transport.eq_ignore_ascii_case("stdio") { - let command = dependency - .command - .as_ref() - .ok_or_else(|| "missing command for stdio dependency".to_string())?; - return Ok(McpServerConfig { - transport: McpServerTransportConfig::Stdio { - command: command.clone(), - args: Vec::new(), - env: None, - env_vars: Vec::new(), - cwd: None, - }, - environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), - enabled: true, - required: false, - supports_parallel_tool_calls: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - default_tools_approval_mode: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth: None, - oauth_resource: None, - tools: HashMap::new(), - }); - } - - Err(format!("unsupported transport {transport}")) -} - -fn collect_missing_mcp_dependencies( - mentioned_skills: &[SkillMetadata], - installed: &HashMap, -) -> HashMap { - let mut missing = HashMap::new(); - let installed_keys: HashSet = installed - .iter() - .map(|(name, config)| canonical_mcp_server_key(name, config)) - .collect(); - let mut seen_canonical_keys = HashSet::new(); - - for skill in mentioned_skills { - let Some(dependencies) = skill.dependencies.as_ref() else { - continue; - }; - - for tool in &dependencies.tools { - if !tool.r#type.eq_ignore_ascii_case("mcp") { - continue; - } - let dependency_key = match canonical_mcp_dependency_key(tool) { - Ok(key) => key, - Err(err) => { - let dependency = tool.value.as_str(); - let skill_name = skill.name.as_str(); - warn!( - "unable to auto-install MCP dependency {dependency} for skill {skill_name}: {err}", - ); - continue; - } - }; - if installed_keys.contains(&dependency_key) - || seen_canonical_keys.contains(&dependency_key) - { - continue; - } - - let config = match mcp_dependency_to_server_config(tool) { - Ok(config) => config, - Err(err) => { - let dependency = dependency_key.as_str(); - let skill_name = skill.name.as_str(); - warn!( - "unable to auto-install MCP dependency {dependency} for skill {skill_name}: {err}", - ); - continue; - } - }; - - missing.insert(tool.value.clone(), config); - seen_canonical_keys.insert(dependency_key); - } - } - - missing -} diff --git a/codex-rs/core/src/plugins/mentions.rs b/codex-rs/core/src/plugins/mentions.rs index a5e94345cb..57b329cd43 100644 --- a/codex-rs/core/src/plugins/mentions.rs +++ b/codex-rs/core/src/plugins/mentions.rs @@ -1,10 +1,7 @@ -use std::collections::HashMap; use std::collections::HashSet; -use codex_connectors::metadata::connector_mention_slug; use codex_protocol::user_input::UserInput; -use crate::connectors; use crate::injection::ToolMentionKind; use crate::injection::app_id_from_path; use crate::injection::extract_tool_mentions_with_sigil; @@ -15,27 +12,20 @@ use crate::mention_syntax::TOOL_MENTION_SIGIL; use super::PluginCapabilitySummary; -pub(crate) struct CollectedToolMentions { - pub(crate) plain_names: HashSet, - pub(crate) paths: HashSet, +fn collect_tool_paths_from_messages(messages: &[String]) -> HashSet { + collect_tool_paths_from_messages_with_sigil(messages, TOOL_MENTION_SIGIL) } -pub(crate) fn collect_tool_mentions_from_messages(messages: &[String]) -> CollectedToolMentions { - collect_tool_mentions_from_messages_with_sigil(messages, TOOL_MENTION_SIGIL) -} - -fn collect_tool_mentions_from_messages_with_sigil( +fn collect_tool_paths_from_messages_with_sigil( messages: &[String], sigil: char, -) -> CollectedToolMentions { - let mut plain_names = HashSet::new(); +) -> HashSet { let mut paths = HashSet::new(); for message in messages { let mentions = extract_tool_mentions_with_sigil(message, sigil); - plain_names.extend(mentions.plain_names().map(str::to_string)); paths.extend(mentions.paths().map(str::to_string)); } - CollectedToolMentions { plain_names, paths } + paths } pub(crate) fn collect_explicit_app_ids(input: &[UserInput]) -> HashSet { @@ -53,7 +43,7 @@ pub(crate) fn collect_explicit_app_ids(input: &[UserInput]) -> HashSet { UserInput::Mention { path, .. } => Some(path.clone()), _ => None, }) - .chain(collect_tool_mentions_from_messages(&messages).paths) + .chain(collect_tool_paths_from_messages(&messages)) .filter(|path| tool_kind_for_path(path.as_str()) == ToolMentionKind::App) .filter_map(|path| app_id_from_path(path.as_str()).map(str::to_string)) .collect() @@ -84,8 +74,7 @@ pub(crate) fn collect_explicit_plugin_mentions( }) .chain( // Plugin plaintext links use `@`, not the default `$` tool sigil. - collect_tool_mentions_from_messages_with_sigil(&messages, PLUGIN_TEXT_MENTION_SIGIL) - .paths, + collect_tool_paths_from_messages_with_sigil(&messages, PLUGIN_TEXT_MENTION_SIGIL), ) .filter(|path| tool_kind_for_path(path.as_str()) == ToolMentionKind::Plugin) .filter_map(|path| plugin_config_name_from_path(path.as_str()).map(str::to_string)) @@ -102,19 +91,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 { - let mut counts: HashMap = HashMap::new(); - for connector in connectors { - let slug = connector_mention_slug(connector); - *counts.entry(slug).or_insert(0) += 1; - } - counts -} - #[cfg(test)] #[path = "mentions_tests.rs"] mod tests; diff --git a/codex-rs/core/src/plugins/mod.rs b/codex-rs/core/src/plugins/mod.rs index 11e2c338bc..1a2877091e 100644 --- a/codex-rs/core/src/plugins/mod.rs +++ b/codex-rs/core/src/plugins/mod.rs @@ -11,8 +11,5 @@ pub(crate) use discoverable::list_tool_suggest_discoverable_plugins; 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 381c721878..3f109e2206 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -227,8 +227,6 @@ use self::session::SessionConfiguration; pub(crate) use self::session::SessionSettingsUpdate; #[cfg(test)] use self::turn::AssistantMessageStreamParsers; -#[cfg(test)] -use self::turn::collect_explicit_app_ids_from_skill_items; use self::turn::realtime_text_for_event; use self::turn_context::TurnContext; use self::turn_context::TurnSkillsContext; @@ -288,8 +286,6 @@ pub(crate) struct PreviousTurnSettings { pub(crate) realtime_active: Option, } -#[cfg(test)] -use crate::SkillMetadata; use crate::SkillsService; use crate::agents_md::load_project_instructions; use crate::exec_policy::ExecPolicyUpdateError; @@ -301,6 +297,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::AutoCompactWindowSnapshot; use crate::state::PendingRequestPermissions; use crate::state::SessionServices; diff --git a/codex-rs/core/src/session/review.rs b/codex-rs/core/src/session/review.rs index bbdfc3f984..32f9aa4b0d 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -99,6 +99,7 @@ pub(super) async fn spawn_review_thread( let extension_data = Arc::new(codex_extension_api::ExtensionData::new( review_turn_id.clone(), )); + extension_data.insert(session_telemetry_for_context.clone()); extension_data.insert(parent_turn_context.turn_skills.snapshot.clone()); let review_turn_context = TurnContext { diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index c4bc064a47..b84b396f29 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -959,6 +959,7 @@ impl Session { )); let session_extension_data = codex_extension_api::ExtensionData::new(session_id.to_string()); + session_extension_data.insert(analytics_events_client.clone()); session_extension_data.insert(McpResourceClient::new(Arc::clone( &mcp_connection_manager, ))); diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 8f37adf839..1cec89c89c 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -80,7 +80,6 @@ use crate::tools::handlers::ShellCommandHandler; use crate::tools::registry::ToolExecutor; use crate::tools::router::ToolCallSource; use crate::turn_diff_tracker::TurnDiffTracker; -use codex_app_server_protocol::AppInfo; use codex_app_server_protocol::McpElicitationSchema; use codex_config::config_toml::ConfigToml; use codex_config::config_toml::ProjectConfig; @@ -257,18 +256,6 @@ fn histogram_sum(resource_metrics: &ResourceMetrics, name: &str) -> u64 { } } -fn skill_message(text: &str) -> ResponseItem { - ResponseItem::Message { - id: None, - role: "user".to_string(), - content: vec![ContentItem::InputText { - text: text.to_string(), - }], - phase: None, - metadata: None, - } -} - #[tokio::test] async fn regular_turn_emits_turn_started_with_trace_id_without_waiting_for_startup_prewarm() { let _trace_test_context = install_test_tracing("codex-core-tests"); @@ -584,24 +571,6 @@ fn test_tool_runtime(session: Arc, turn_context: Arc) -> T ToolCallRuntime::new(router, session, turn_context, tracker) } -fn make_connector(id: &str, name: &str) -> AppInfo { - AppInfo { - id: id.to_string(), - name: name.to_string(), - description: None, - logo_url: None, - logo_url_dark: None, - distribution_channel: None, - branding: None, - app_metadata: None, - labels: None, - install_url: None, - is_accessible: true, - is_enabled: true, - plugin_display_names: Vec::new(), - } -} - #[test] fn assistant_message_stream_parsers_can_be_seeded_from_output_item_added_text() { let mut parsers = AssistantMessageStreamParsers::new(/*plan_mode*/ false); @@ -1538,49 +1507,6 @@ disabled_tools = [ ); } -#[test] -fn collect_explicit_app_ids_from_skill_items_includes_linked_mentions() { - let connectors = vec![make_connector("calendar", "Calendar")]; - let skill_items = vec![skill_message( - "\ndemo\n/tmp/skills/demo/SKILL.md\nuse [$calendar](app://calendar)\n", - )]; - - let connector_ids = - collect_explicit_app_ids_from_skill_items(&skill_items, &connectors, &HashMap::new()); - - assert_eq!(connector_ids, HashSet::from(["calendar".to_string()])); -} - -#[test] -fn collect_explicit_app_ids_from_skill_items_resolves_unambiguous_plain_mentions() { - let connectors = vec![make_connector("calendar", "Calendar")]; - let skill_items = vec![skill_message( - "\ndemo\n/tmp/skills/demo/SKILL.md\nuse $calendar\n", - )]; - - let connector_ids = - collect_explicit_app_ids_from_skill_items(&skill_items, &connectors, &HashMap::new()); - - assert_eq!(connector_ids, HashSet::from(["calendar".to_string()])); -} - -#[test] -fn collect_explicit_app_ids_from_skill_items_skips_plain_mentions_with_skill_conflicts() { - let connectors = vec![make_connector("calendar", "Calendar")]; - let skill_items = vec![skill_message( - "\ndemo\n/tmp/skills/demo/SKILL.md\nuse $calendar\n", - )]; - let skill_name_counts_lower = HashMap::from([("calendar".to_string(), 1)]); - - let connector_ids = collect_explicit_app_ids_from_skill_items( - &skill_items, - &connectors, - &skill_name_counts_lower, - ); - - assert_eq!(connector_ids, HashSet::::new()); -} - #[tokio::test] async fn reconstruct_history_matches_live_compactions() { 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 22987498c5..2d41c0235f 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; @@ -24,16 +21,10 @@ use crate::hook_runtime::record_pending_input; use crate::hook_runtime::run_legacy_after_agent_hook; use crate::hook_runtime::run_pending_session_start_hooks; use crate::hook_runtime::run_turn_stop_hooks; -use crate::injection::ToolMentionKind; -use crate::injection::app_id_from_path; -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; use crate::plugins::build_plugin_injections; use crate::responses_metadata::CodexResponsesMetadata; use crate::responses_metadata::CodexResponsesRequestKind; @@ -74,7 +65,6 @@ 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_extension_api::TurnInputContext; use codex_extension_api::TurnInputEnvironment; use codex_features::Feature; @@ -528,57 +518,35 @@ 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( + let reserved_plain_tool_names = available_connectors + .iter() + .map(codex_connectors::metadata::connector_mention_slug) + .collect(); + let extension_injection_items = build_extension_turn_input_items( + sess, + turn_context, &user_input, - &skills_outcome.skills, - &skills_outcome.disabled_paths, - &connector_slug_counts, - ); + reserved_plain_tool_names, + cancellation_token, + ) + .await?; + let mcp_dependencies = turn_context + .extension_data + .get::() + .unwrap_or_default(); maybe_prompt_and_install_mcp_dependencies( sess, turn_context, cancellation_token, - &mentioned_skills, + &mcp_dependencies, Some(sess.mcp_elicitation_reviewer()), ) .await; - - let injected_host_skill_prompts = turn_context + let skill_connector_ids = 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, - ); + .get::() + .map(|mentions| mentions.resolve(&available_connectors)) + .unwrap_or_default(); let plugin_items = build_plugin_injections(&mentioned_plugins, &mcp_tools, &available_connectors); let mut explicitly_enabled_connectors = collect_explicit_app_ids(&user_input); @@ -609,17 +577,7 @@ async fn build_skills_and_plugins( .track_plugin_used(tracking.clone(), plugin); } - 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)) } @@ -628,6 +586,7 @@ async fn build_extension_turn_input_items( sess: &Arc, turn_context: &TurnContext, user_input: &[UserInput], + reserved_plain_tool_names: HashSet, cancellation_token: &CancellationToken, ) -> Option> { let contributors = sess.services.extensions.turn_input_contributors().to_vec(); @@ -654,7 +613,9 @@ async fn build_extension_turn_input_items( let input = TurnInputContext { thread_id: sess.thread_id(), turn_id: turn_context.sub_id.to_string(), + model: turn_context.model_info.slug.clone(), user_input: user_input.to_vec(), + reserved_plain_tool_names, environments, }; @@ -967,57 +928,6 @@ async fn run_auto_compact( Ok(()) } -pub(super) fn collect_explicit_app_ids_from_skill_items( - skill_items: &[ResponseItem], - connectors: &[connectors::AppInfo], - skill_name_counts_lower: &HashMap, -) -> HashSet { - if skill_items.is_empty() || connectors.is_empty() { - return HashSet::new(); - } - - let skill_messages = skill_items - .iter() - .filter_map(|item| match item { - ResponseItem::Message { content, .. } => { - content.iter().find_map(|content_item| match content_item { - ContentItem::InputText { text } => Some(text.clone()), - _ => None, - }) - } - _ => None, - }) - .collect::>(); - if skill_messages.is_empty() { - return HashSet::new(); - } - - let mentions = collect_tool_mentions_from_messages(&skill_messages); - let mention_names_lower = mentions - .plain_names - .iter() - .map(|name| name.to_ascii_lowercase()) - .collect::>(); - let mut connector_ids = mentions - .paths - .iter() - .filter(|path| tool_kind_for_path(path) == ToolMentionKind::App) - .filter_map(|path| app_id_from_path(path).map(str::to_string)) - .collect::>(); - - let connector_slug_counts = build_connector_slug_counts(connectors); - for connector in connectors { - let slug = codex_connectors::metadata::connector_mention_slug(connector); - let connector_count = connector_slug_counts.get(&slug).copied().unwrap_or(0); - let skill_count = skill_name_counts_lower.get(&slug).copied().unwrap_or(0); - if connector_count == 1 && skill_count == 0 && mention_names_lower.contains(&slug) { - connector_ids.insert(connector.id.clone()); - } - } - - connector_ids -} - #[instrument(level = "trace", skip_all)] pub(crate) fn build_prompt( input: Vec, diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index 2c1ba8245a..b80cbe5394 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -22,8 +22,6 @@ pub use codex_core_skills::default_skill_metadata_budget; pub use codex_core_skills::detect_implicit_skill_invocation_for_command; pub use codex_core_skills::filter_skill_load_outcome_for_product; pub use codex_core_skills::injection; -pub use codex_core_skills::injection::SkillInjections; -pub use codex_core_skills::injection::build_skill_injections; pub use codex_core_skills::injection::collect_explicit_skill_mentions; pub use codex_core_skills::loader; pub use codex_core_skills::model; diff --git a/codex-rs/ext/extension-api/src/contributors/turn_input.rs b/codex-rs/ext/extension-api/src/contributors/turn_input.rs index 481a3a9eef..9b72ef08e2 100644 --- a/codex-rs/ext/extension-api/src/contributors/turn_input.rs +++ b/codex-rs/ext/extension-api/src/contributors/turn_input.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::path::PathBuf; use codex_protocol::ThreadId; @@ -21,8 +22,15 @@ pub struct TurnInputContext { pub thread_id: ThreadId, /// Stable host-owned turn identifier. pub turn_id: String, + /// Model slug selected for this turn. + pub model: String, /// User input submitted for this turn. pub user_input: Vec, + /// Plain tool names already claimed by host-managed capabilities. + /// + /// Contributors should not resolve these names through ambiguous plaintext + /// mentions. Explicit resource or path mentions remain valid. + pub reserved_plain_tool_names: HashSet, /// Resolved turn environments, in host priority order. pub environments: Vec, } diff --git a/codex-rs/ext/skills/Cargo.toml b/codex-rs/ext/skills/Cargo.toml index 61089be1c7..14c2663de8 100644 --- a/codex-rs/ext/skills/Cargo.toml +++ b/codex-rs/ext/skills/Cargo.toml @@ -14,6 +14,8 @@ doctest = false workspace = true [dependencies] +codex-analytics = { workspace = true } +codex-connectors = { workspace = true } codex-core-skills = { workspace = true } codex-exec-server = { workspace = true } codex-extension-api = { workspace = true } diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index 3e3cf98ea0..84b11204ef 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -1,7 +1,17 @@ +use std::collections::HashSet; use std::sync::Arc; +use codex_analytics::AnalyticsEventsClient; +use codex_analytics::InvocationType; +use codex_analytics::SkillInvocation; +use codex_analytics::build_track_events_context; +use codex_connectors::ExplicitConnectorMentions; use codex_core_skills::HostSkillsSnapshot; -use codex_core_skills::injection::InjectedHostSkillPrompts; +use codex_core_skills::SkillMetadata; +use codex_core_skills::injection::ToolMentionKind; +use codex_core_skills::injection::app_id_from_path; +use codex_core_skills::injection::extract_tool_mentions; +use codex_core_skills::injection::tool_kind_for_path; use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_extension_api::ConfigContributor; use codex_extension_api::ContextContributionContext; @@ -20,6 +30,8 @@ use codex_extension_api::ToolExecutor; use codex_extension_api::TurnInputContext; use codex_extension_api::TurnInputContributor; use codex_mcp::McpResourceClient; +use codex_mcp::McpServerDependencies; +use codex_mcp::McpServerDependency; use codex_otel::SessionTelemetry; use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_protocol::protocol::Event; @@ -112,6 +124,7 @@ where Box::pin(async move { let session_store = context.session_store; let thread_store = context.thread_store; + let turn_store = context.turn_store; let Some(thread_state) = thread_store.get::() else { return Vec::new(); }; @@ -119,11 +132,11 @@ where if !config.include_instructions { return Vec::new(); } - let host_snapshot = context.turn_store.get::(); + let host_snapshot = turn_store.get::(); let catalog = self .list_skills( SkillListQuery { - turn_id: context.turn_store.level_id().to_string(), + turn_id: turn_store.level_id().to_string(), executor_roots: thread_state.selected_roots().to_vec(), host_snapshot: host_snapshot.clone(), include_host_skills: true, @@ -206,7 +219,7 @@ where turn_id: input.turn_id.clone(), executor_roots: thread_state.selected_roots().to_vec(), host_snapshot: host_snapshot.clone(), - include_host_skills: false, + include_host_skills: true, include_bundled_skills: config.bundled_skills_enabled, include_orchestrator_skills: thread_state.orchestrator_skills_enabled(), mcp_resources: session_store.get::(), @@ -216,11 +229,43 @@ where self.emit_warning(input.thread_id, &input.turn_id, warning.clone()); } - let selected_entries = collect_explicit_skill_mentions(&input.user_input, &catalog); + let selected_entries = collect_explicit_skill_mentions( + &input.user_input, + &catalog, + &input.reserved_plain_tool_names, + ); let mut fragments: Vec> = Vec::new(); + let skill_names = catalog + .entries + .iter() + .filter(|entry| entry.enabled) + .map(|entry| entry.name.to_ascii_lowercase()) + .collect::>(); + let mut mcp_dependencies = McpServerDependencies::default(); + for entry in &selected_entries { + for dependency in entry + .dependencies + .iter() + .flat_map(|dependencies| &dependencies.tools) + .filter(|dependency| dependency.r#type.eq_ignore_ascii_case("mcp")) + { + mcp_dependencies.push(McpServerDependency { + source_name: entry.name.clone(), + name: dependency.value.clone(), + transport: dependency.transport.clone(), + command: dependency.command.clone(), + url: dependency.url.clone(), + }); + } + } + if !mcp_dependencies.is_empty() { + turn_store.insert(mcp_dependencies); + } + let mut warnings = catalog.warnings.clone(); let mut main_prompts_injected = false; - let mut injected_host_skill_prompts = InjectedHostSkillPrompts::default(); + let mut connector_mentions = ExplicitConnectorMentions::default(); + let mut skill_invocations = Vec::new(); for entry in &selected_entries { match self .read_main_prompt(entry, host_snapshot.clone(), session_store, &thread_state) @@ -229,6 +274,35 @@ where Ok(read_result) => { let (contents, truncated) = truncate_main_prompt_contents(read_result.contents.as_str()); + let tool_mentions = extract_tool_mentions(&contents); + for path in tool_mentions.paths() { + if tool_kind_for_path(path) == ToolMentionKind::App + && let Some(connector_id) = app_id_from_path(path) + { + connector_mentions.insert_connector_id(connector_id); + } + } + for name in tool_mentions.plain_names() { + if !skill_names.contains(&name.to_ascii_lowercase()) { + connector_mentions.insert_plain_name(name); + } + } + if let Some(skill) = host_skill_for_entry(host_snapshot.as_deref(), entry) { + if let Some(telemetry) = turn_store.get::() { + telemetry.counter( + "codex.skill.injected", + /*inc*/ 1, + &[("status", "ok"), ("skill", skill.name.as_str())], + ); + } + skill_invocations.push(SkillInvocation { + skill_name: skill.name.clone(), + skill_scope: skill.scope, + skill_path: skill.path_to_skills_md.to_path_buf(), + plugin_id: skill.plugin_id.clone(), + invocation_type: InvocationType::Explicit, + }); + } if truncated { let warning = format!( "Skill `{}` exceeded the main prompt context limit and was truncated.", @@ -248,33 +322,35 @@ where }; 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) => { + if let Some(skill) = host_skill_for_entry(host_snapshot.as_deref(), entry) + && let Some(telemetry) = turn_store.get::() + { + telemetry.counter( + "codex.skill.injected", + /*inc*/ 1, + &[("status", "error"), ("skill", skill.name.as_str())], + ); + } let warning = format!("Failed to load skill `{}`: {message}", entry.name); self.emit_warning(input.thread_id, &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()); - } - } + if !connector_mentions.is_empty() { + turn_store.insert(connector_mentions); + } + if let Some(analytics) = session_store.get::() { + analytics.track_skill_invocations( + build_track_events_context( + input.model, + thread_store.level_id().to_string(), + input.turn_id, + ), + skill_invocations, + ); } turn_store.insert(SkillsTurnState { @@ -283,9 +359,6 @@ where warnings, main_prompts_injected, }); - if !injected_host_skill_prompts.is_empty() { - turn_store.insert(injected_host_skill_prompts); - } fragments }) @@ -350,6 +423,19 @@ impl SkillsExtension { } } +fn host_skill_for_entry<'a>( + host_snapshot: Option<&'a HostSkillsSnapshot>, + entry: &SkillCatalogEntry, +) -> Option<&'a SkillMetadata> { + if entry.authority.kind != SkillSourceKind::Host { + return None; + } + host_snapshot?.outcome().skills.iter().find(|skill| { + let path = skill.path_to_skills_md.to_string_lossy(); + path == entry.main_prompt.as_str() || path.replace('\\', "/") == entry.main_prompt.as_str() + }) +} + pub fn install( registry: &mut ExtensionRegistryBuilder, config_from_host: impl Fn(&C) -> SkillsExtensionConfig + Send + Sync + 'static, diff --git a/codex-rs/ext/skills/src/selection.rs b/codex-rs/ext/skills/src/selection.rs index cfea2e3abd..cbc573ec78 100644 --- a/codex-rs/ext/skills/src/selection.rs +++ b/codex-rs/ext/skills/src/selection.rs @@ -7,12 +7,14 @@ use crate::catalog::SkillAuthority; use crate::catalog::SkillCatalog; use crate::catalog::SkillCatalogEntry; use crate::catalog::SkillPackageId; +use crate::catalog::SkillSourceKind; const SKILL_PATH_PREFIX: &str = "skill://"; pub(crate) fn collect_explicit_skill_mentions( inputs: &[UserInput], catalog: &SkillCatalog, + reserved_plain_tool_names: &HashSet, ) -> Vec { let mut selected = Vec::new(); let mut seen = HashSet::new(); @@ -54,13 +56,29 @@ pub(crate) fn collect_explicit_skill_mentions( if blocked_plain_names.contains(name) { continue; } - if let Some(entry) = catalog + let matching_entries = catalog .entries .iter() - .find(|entry| entry.enabled && entry.name == name) + .filter(|entry| entry.enabled && entry.name == name); + if let Some(entry) = matching_entries + .clone() + .find(|entry| entry.authority.kind != SkillSourceKind::Host) { push_selected(entry, &mut seen, &mut selected); + continue; } + + let mut host_entries = + matching_entries.filter(|entry| entry.authority.kind == SkillSourceKind::Host); + let Some(entry) = host_entries.next() else { + continue; + }; + if host_entries.next().is_some() + || reserved_plain_tool_names.contains(&name.to_ascii_lowercase()) + { + continue; + } + push_selected(entry, &mut seen, &mut selected); } } diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index b3347afa1f..fce11a1f8e 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -1,20 +1,25 @@ +use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; use std::sync::Mutex; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; +use codex_connectors::ExplicitConnectorMentions; 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::model::SkillDependencies; +use codex_core_skills::model::SkillToolDependency; use codex_extension_api::ContextContributionContext; use codex_extension_api::ExtensionData; use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionRegistryBuilder; use codex_extension_api::ThreadStartInput; use codex_extension_api::TurnInputContext; +use codex_mcp::McpServerDependencies; use codex_protocol::ThreadId; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::SelectedCapabilityRoot; @@ -52,9 +57,10 @@ 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 [$calendar](app://calendar).\n"; +const STATIC_SKILL_CONTENTS: &str = "# Lint Fix\n\nRun the formatter."; #[tokio::test] -async fn installed_extension_catalog_uses_host_service_snapshot() -> TestResult { +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( @@ -90,7 +96,16 @@ async fn installed_extension_catalog_uses_host_service_snapshot() -> TestResult description: "Demo skill.".to_string(), short_description: None, interface: None, - dependencies: None, + dependencies: Some(SkillDependencies { + tools: vec![SkillToolDependency { + r#type: "mcp".to_string(), + value: "docs".to_string(), + description: None, + transport: Some("streamable_http".to_string()), + command: None, + url: Some("https://example.com/mcp".to_string()), + }], + }), policy: None, path_to_skills_md: skill_path, scope: SkillScope::User, @@ -105,11 +120,57 @@ async fn installed_extension_catalog_uses_host_service_snapshot() -> TestResult .contribute(prompt_context(&session_store, &thread_store, &turn_store)) .await; + let fragments = registry.turn_input_contributors()[0] + .contribute( + TurnInputContext { + thread_id: ThreadId::default(), + turn_id: "turn-1".to_string(), + model: "test-model".to_string(), + user_input: vec![UserInput::Text { + text: "$demo".to_string(), + text_elements: Vec::new(), + }], + reserved_plain_tool_names: Default::default(), + 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!(1, prompt_fragments.len()); assert_eq!(expected_catalog, prompt_fragments[0].text()); + assert_eq!( + vec![("user", expected_skill)], + fragments + .iter() + .map(|fragment| (fragment.role(), fragment.render())) + .collect::>() + ); + assert_eq!( + turn_store + .get::() + .ok_or("expected connector mentions")? + .resolve(&[]), + ["calendar".to_string()].into_iter().collect() + ); + assert_eq!( + turn_store + .get::() + .ok_or("expected MCP dependencies")? + .missing_from(&HashMap::new()) + .keys() + .cloned() + .collect::>(), + vec!["docs".to_string()] + ); std::fs::remove_dir_all(codex_home)?; Ok(()) @@ -139,6 +200,7 @@ async fn selected_executor_catalog_is_context_and_selected_entrypoint_is_turn_in entries: executor_entries, warnings: Vec::new(), }, + contents: STATIC_SKILL_CONTENTS.to_string(), read_requests: Arc::clone(&read_requests), list_calls: None, fail_first_list: false, @@ -154,6 +216,7 @@ async fn selected_executor_catalog_is_context_and_selected_entrypoint_is_turn_in )], warnings: Vec::new(), }, + contents: STATIC_SKILL_CONTENTS.to_string(), read_requests: Arc::new(Mutex::new(Vec::new())), list_calls: None, fail_first_list: false, @@ -237,10 +300,12 @@ async fn selected_executor_catalog_is_context_and_selected_entrypoint_is_turn_in TurnInputContext { thread_id: ThreadId::default(), turn_id: "turn-1".to_string(), + model: "test-model".to_string(), user_input: vec![UserInput::Text { text: "$lint-fix please".to_string(), text_elements: Vec::new(), }], + reserved_plain_tool_names: Default::default(), environments: Vec::new(), }, &session_store, @@ -273,10 +338,12 @@ async fn selected_executor_catalog_is_context_and_selected_entrypoint_is_turn_in TurnInputContext { thread_id: ThreadId::default(), turn_id: "turn-2".to_string(), + model: "test-model".to_string(), user_input: vec![UserInput::Text { text: "no skill this time".to_string(), text_elements: Vec::new(), }], + reserved_plain_tool_names: Default::default(), environments: Vec::new(), }, &session_store, @@ -304,6 +371,7 @@ async fn orchestrator_catalog_snapshot_caches_failure() -> TestResult { )], warnings: Vec::new(), }, + contents: STATIC_SKILL_CONTENTS.to_string(), read_requests: Arc::new(Mutex::new(Vec::new())), list_calls: Some(Arc::clone(&list_calls)), fail_first_list: true, @@ -350,10 +418,12 @@ async fn orchestrator_catalog_snapshot_caches_failure() -> TestResult { TurnInputContext { thread_id: ThreadId::default(), turn_id: turn_id.to_string(), + model: "test-model".to_string(), user_input: vec![UserInput::Text { text: "$first".to_string(), text_elements: Vec::new(), }], + reserved_plain_tool_names: Default::default(), environments: Vec::new(), }, &session_store, @@ -390,6 +460,7 @@ async fn root_qualified_locator_selects_only_the_matching_executor_skill() -> Te .collect(), warnings: Vec::new(), }, + contents: STATIC_SKILL_CONTENTS.to_string(), read_requests: Arc::clone(&read_requests), list_calls: None, fail_first_list: false, @@ -430,10 +501,12 @@ async fn root_qualified_locator_selects_only_the_matching_executor_skill() -> Te TurnInputContext { thread_id: ThreadId::default(), turn_id: "turn-1".to_string(), + model: "test-model".to_string(), user_input: vec![UserInput::Mention { name: "lint-fix".to_string(), path: root_b_locator.to_string(), }], + reserved_plain_tool_names: Default::default(), environments: Vec::new(), }, &session_store, @@ -463,26 +536,27 @@ async fn prompt_hidden_skill_can_still_be_invoked() -> TestResult { catalog: SkillCatalog { entries: vec![ test_entry( - SkillSourceKind::Orchestrator, - "codex_apps", - "orchestrator/visible-skill", - "skill://orchestrator/visible-skill/SKILL.md", + SkillSourceKind::Host, + "host", + "host/visible-skill", + "visible-skill/SKILL.md", ), test_entry( - SkillSourceKind::Orchestrator, - "codex_apps", - "orchestrator/hidden-skill", - "skill://orchestrator/hidden-skill/SKILL.md", + SkillSourceKind::Host, + "host", + "host/hidden-skill", + "hidden-skill/SKILL.md", ) .hidden_from_prompt(), ], warnings: Vec::new(), }, + contents: STATIC_SKILL_CONTENTS.to_string(), read_requests: Arc::clone(&read_requests), list_calls: None, fail_first_list: false, }); - let providers = SkillProviders::new().with_orchestrator_provider(provider); + let providers = SkillProviders::new().with_host_provider(provider); let mut builder = ExtensionRegistryBuilder::new(); install_with_providers(&mut builder, providers, skills_extension_config); let registry = builder.build(); @@ -517,10 +591,12 @@ async fn prompt_hidden_skill_can_still_be_invoked() -> TestResult { TurnInputContext { thread_id: ThreadId::default(), turn_id: "turn-1".to_string(), + model: "test-model".to_string(), user_input: vec![UserInput::Text { text: "$hidden-skill".to_string(), text_elements: Vec::new(), }], + reserved_plain_tool_names: Default::default(), environments: Vec::new(), }, &session_store, @@ -533,9 +609,9 @@ async fn prompt_hidden_skill_can_still_be_invoked() -> TestResult { assert!(fragments[0].render().contains("hidden-skill")); assert_eq!( vec![( - SkillAuthority::new(SkillSourceKind::Orchestrator, "codex_apps"), - SkillPackageId("orchestrator/hidden-skill".to_string()), - SkillResourceId::new("skill://orchestrator/hidden-skill/SKILL.md"), + SkillAuthority::new(SkillSourceKind::Host, "host"), + SkillPackageId("host/hidden-skill".to_string()), + SkillResourceId::new("hidden-skill/SKILL.md"), )], read_request_keys(&read_requests) ); @@ -543,9 +619,207 @@ async fn prompt_hidden_skill_can_still_be_invoked() -> TestResult { Ok(()) } +#[tokio::test] +async fn host_plain_mentions_preserve_legacy_disambiguation() -> TestResult { + let read_requests = Arc::new(Mutex::new(Vec::new())); + let calendar_locator = "skill://host/calendar/SKILL.md"; + let provider = Arc::new(StaticSkillProvider { + catalog: SkillCatalog { + entries: vec![ + SkillCatalogEntry::new( + SkillPackageId("host/deploy-user".to_string()), + SkillAuthority::new(SkillSourceKind::Host, "host"), + "deploy", + "Deploy from the user scope.", + SkillResourceId::new("deploy-user/SKILL.md"), + ), + SkillCatalogEntry::new( + SkillPackageId("host/deploy-repo".to_string()), + SkillAuthority::new(SkillSourceKind::Host, "host"), + "deploy", + "Deploy from the repo scope.", + SkillResourceId::new("deploy-repo/SKILL.md"), + ), + SkillCatalogEntry::new( + SkillPackageId("host/calendar".to_string()), + SkillAuthority::new(SkillSourceKind::Host, "host"), + "calendar", + "Use the calendar skill.", + SkillResourceId::new("calendar/SKILL.md"), + ) + .with_display_path(calendar_locator), + ], + warnings: Vec::new(), + }, + contents: STATIC_SKILL_CONTENTS.to_string(), + read_requests: Arc::clone(&read_requests), + list_calls: None, + fail_first_list: false, + }); + let providers = SkillProviders::new().with_host_provider(provider); + let mut builder = ExtensionRegistryBuilder::new(); + install_with_providers(&mut builder, providers, skills_extension_config); + let registry = builder.build(); + let session_store = ExtensionData::new("session"); + let thread_store = ExtensionData::new("thread"); + let session_source = SessionSource::Cli; + let config = default_config(); + registry.thread_lifecycle_contributors()[0] + .on_thread_start(ThreadStartInput { + config: &config, + session_source: &session_source, + persistent_thread_state_available: true, + environments: &[], + session_store: &session_store, + thread_store: &thread_store, + }) + .await; + + let ambiguous_fragments = registry.turn_input_contributors()[0] + .contribute( + TurnInputContext { + thread_id: ThreadId::default(), + turn_id: "turn-ambiguous".to_string(), + model: "test-model".to_string(), + user_input: vec![UserInput::Text { + text: "$deploy".to_string(), + text_elements: Vec::new(), + }], + reserved_plain_tool_names: Default::default(), + environments: Vec::new(), + }, + &session_store, + &thread_store, + &ExtensionData::new("turn-ambiguous"), + ) + .await; + assert!(ambiguous_fragments.is_empty()); + + let connector_conflict_fragments = registry.turn_input_contributors()[0] + .contribute( + TurnInputContext { + thread_id: ThreadId::default(), + turn_id: "turn-connector-conflict".to_string(), + model: "test-model".to_string(), + user_input: vec![UserInput::Text { + text: "$calendar".to_string(), + text_elements: Vec::new(), + }], + reserved_plain_tool_names: ["calendar".to_string()].into_iter().collect(), + environments: Vec::new(), + }, + &session_store, + &thread_store, + &ExtensionData::new("turn-connector-conflict"), + ) + .await; + assert!(connector_conflict_fragments.is_empty()); + + let explicit_fragments = registry.turn_input_contributors()[0] + .contribute( + TurnInputContext { + thread_id: ThreadId::default(), + turn_id: "turn-explicit".to_string(), + model: "test-model".to_string(), + user_input: vec![UserInput::Mention { + name: "calendar".to_string(), + path: calendar_locator.to_string(), + }], + reserved_plain_tool_names: ["calendar".to_string()].into_iter().collect(), + environments: Vec::new(), + }, + &session_store, + &thread_store, + &ExtensionData::new("turn-explicit"), + ) + .await; + assert_eq!(1, explicit_fragments.len()); + assert_eq!( + vec![( + SkillAuthority::new(SkillSourceKind::Host, "host"), + SkillPackageId("host/calendar".to_string()), + SkillResourceId::new("calendar/SKILL.md"), + )], + read_request_keys(&read_requests) + ); + + Ok(()) +} + +#[tokio::test] +async fn connector_mentions_only_include_injected_skill_text() -> TestResult { + let provider = Arc::new(StaticSkillProvider { + catalog: SkillCatalog { + entries: vec![test_entry( + SkillSourceKind::Executor, + "env-1", + "executor/lint-fix", + "lint-fix/SKILL.md", + )], + warnings: Vec::new(), + }, + contents: format!("{}[$calendar](app://calendar)", "x".repeat(9_000)), + read_requests: Arc::new(Mutex::new(Vec::new())), + list_calls: None, + fail_first_list: false, + }); + let providers = SkillProviders::new().with_executor_provider(provider); + let mut builder = ExtensionRegistryBuilder::new(); + install_with_providers(&mut builder, providers, skills_extension_config); + let registry = builder.build(); + let session_store = ExtensionData::new("session"); + let thread_store = ExtensionData::new("thread"); + thread_store.insert(vec![SelectedCapabilityRoot { + id: "lint-fix".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "env-1".to_string(), + path: "/skills/lint-fix".to_string(), + }, + }]); + 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 turn_store = ExtensionData::new("turn-1"); + let fragments = registry.turn_input_contributors()[0] + .contribute( + TurnInputContext { + thread_id: ThreadId::default(), + turn_id: "turn-1".to_string(), + model: "test-model".to_string(), + user_input: vec![UserInput::Text { + text: "$lint-fix".to_string(), + text_elements: Vec::new(), + }], + reserved_plain_tool_names: Default::default(), + environments: Vec::new(), + }, + &session_store, + &thread_store, + &turn_store, + ) + .await; + + assert_eq!(1, fragments.len()); + assert!(!fragments[0].render().contains("app://calendar")); + assert!(turn_store.get::().is_none()); + + Ok(()) +} + #[derive(Clone)] struct StaticSkillProvider { catalog: SkillCatalog, + contents: String, read_requests: Arc>>, list_calls: Option>, fail_first_list: bool, @@ -577,6 +851,7 @@ impl SkillProvider for StaticSkillProvider { } fn read(&self, request: SkillReadRequest) -> SkillProviderFuture<'_, SkillReadResult> { + let contents = self.contents.clone(); let read_requests = Arc::clone(&self.read_requests); Box::pin(async move { read_requests @@ -585,7 +860,7 @@ impl SkillProvider for StaticSkillProvider { .push(request.clone()); Ok(SkillReadResult { resource: request.resource, - contents: "# Lint Fix\n\nRun the formatter.".to_string(), + contents, }) }) }