diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index fac170bcce..4eb9ab73b5 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -17,6 +17,7 @@ use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; use codex_protocol::user_input::UserInput; use codex_skills_extension::HostSkillProvider; +use codex_skills_extension::OrchestratorSkillProvider; use codex_skills_extension::SkillProvider; use codex_skills_extension::SkillProviderSource; use codex_skills_extension::SkillProviders; @@ -44,13 +45,21 @@ use core_test_support::responses::ev_completed; use core_test_support::responses::ev_response_created; use core_test_support::responses::mount_sse_once; use core_test_support::responses::sse; +use core_test_support::skip_if_no_network; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_mcp_server; use pretty_assertions::assert_eq; +use serde_json::Value; +use serde_json::json; use tempfile::TempDir; use tokio::time::Duration; use tokio::time::Instant; use toml::toml; +use wiremock::Mock; +use wiremock::Request; +use wiremock::ResponseTemplate; +use wiremock::matchers::method; +use wiremock::matchers::path_regex; struct StaticSkillProvider { catalog: SkillCatalog, @@ -436,6 +445,192 @@ async fn rendered_catalogs_for_turns( Ok((developer_texts, client_warning_messages)) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn explicit_only_orchestrator_skill_is_hidden_but_can_be_invoked() -> Result<()> { + skip_if_no_network!(Ok(())); + + const SKILL_PACKAGE: &str = "skill://demo/explicit-only"; + const MAIN_RESOURCE: &str = "skill://demo/explicit-only/SKILL.md"; + const REFERENCED_RESOURCE: &str = "skill://demo/explicit-only/references/guide.md"; + const REFERENCED_CONTENTS: &str = "# Referenced guide"; + const READ_CALL_ID: &str = "read-explicit-only-resource"; + + let server = responses::start_mock_server().await; + let apps_server = AppsTestServer::mount(&server).await?; + let response = responses::mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + responses::ev_function_call_with_namespace( + READ_CALL_ID, + "skills", + "read", + &json!({ + "authority": { "kind": "orchestrator" }, + "package": SKILL_PACKAGE, + "resource": REFERENCED_RESOURCE, + }) + .to_string(), + ), + ev_completed("resp-1"), + ]), + sse(vec![ev_response_created("resp-2"), ev_completed("resp-2")]), + ], + ) + .await; + + Mock::given(method("POST")) + .and(path_regex("^/api/codex/ps/mcp/?$")) + .and(|request: &Request| { + serde_json::from_slice::(&request.body).is_ok_and(|body| { + matches!( + body["method"].as_str(), + Some("resources/list" | "resources/read") + ) + }) + }) + .respond_with(|request: &Request| { + let body: Value = serde_json::from_slice(&request.body) + .expect("MCP resource request should be valid JSON"); + let result = match body["method"].as_str() { + Some("resources/list") => { + let resources = [ + ("visible", Some(json!(true))), + ("explicit-only", Some(json!(false))), + ("missing-policy", None), + ("non-boolean-policy", Some(json!("false"))), + ] + .map(|(name, allow_implicit_invocation)| { + let mut metadata = json!({ + "plugin_name": "demo", + "skill_name": name, + }); + if let Some(allow_implicit_invocation) = allow_implicit_invocation { + metadata["allow_implicit_invocation"] = allow_implicit_invocation; + } + json!({ + "name": name, + "uri": format!("skill://demo/{name}"), + "mimeType": "mcp/skill", + "_meta": metadata, + }) + }); + json!({ "resources": resources }) + } + Some("resources/read") => { + let uri = body["params"]["uri"] + .as_str() + .expect("MCP resource read should include a resource URI"); + let contents = match uri { + MAIN_RESOURCE => { + format!("# Explicit-only instructions\nRead {REFERENCED_RESOURCE}.") + } + REFERENCED_RESOURCE => REFERENCED_CONTENTS.to_string(), + _ => unreachable!("unexpected MCP resource URI: {uri}"), + }; + json!({ + "contents": [{ + "uri": uri, + "mimeType": "text/markdown", + "text": contents, + }], + }) + } + method => unreachable!("unexpected MCP resource method: {method:?}"), + }; + ResponseTemplate::new(/*status*/ 200).set_body_json(json!({ + "jsonrpc": "2.0", + "id": body["id"], + "result": result, + })) + }) + .with_priority(/*p*/ 1) + .mount(&server) + .await; + + let mut extensions = ExtensionRegistryBuilder::new(); + install_with_providers( + &mut extensions, + SkillProviders::new() + .with_orchestrator_provider(Arc::new(OrchestratorSkillProvider::new())), + |config: &Config| SkillsExtensionConfig { + include_instructions: config.include_skill_instructions, + bundled_skills_enabled: false, + orchestrator_skills_enabled: true, + shadow_selection_enabled: false, + }, + ); + let mut builder = apps_enabled_builder(apps_server.chatgpt_base_url) + // Local executors disable orchestrator skill discovery. + .with_exec_server_url("none") + .with_extensions(Arc::new(extensions.build())) + .with_config(|config| { + config.include_skill_instructions = true; + config.orchestrator_skills_enabled = true; + }); + let test = builder.build_with_auto_env(&server).await?; + wait_for_mcp_server(&test.codex, CODEX_APPS_MCP_SERVER_NAME).await?; + + test.submit_turn("Use $demo:explicit-only.").await?; + + let requests = response.requests(); + assert_eq!(requests.len(), 2); + let request = &requests[0]; + let developer_messages = request.message_input_texts("developer"); + for name in ["visible", "missing-policy", "non-boolean-policy"] { + let catalog_entry = format!("- demo:{name}:"); + assert!( + developer_messages + .iter() + .any(|message| message.contains(&catalog_entry)), + "model-visible skills should include `{name}`: {developer_messages:?}" + ); + } + assert!( + developer_messages + .iter() + .all(|message| !message.contains("- demo:explicit-only:")), + "model-visible skills should omit the explicit-only skill: {developer_messages:?}" + ); + let user_messages = request.message_input_texts("user"); + let skill_instructions = user_messages + .iter() + .find(|message| { + message.contains("demo:explicit-only") + && message.contains("# Explicit-only instructions") + && message.contains(REFERENCED_RESOURCE) + }) + .expect("explicit invocation should inject the hidden skill instructions and reference"); + let resource_access = skill_instructions + .split_once("") + .and_then(|(_, remainder)| remainder.split_once("")) + .map(|(metadata, _)| metadata) + .expect("hidden orchestrator skills should include resource-access metadata"); + assert_eq!( + serde_json::from_str::(resource_access)?, + json!({ + "authority": { "kind": "orchestrator" }, + "package": SKILL_PACKAGE, + "main_resource": MAIN_RESOURCE, + }) + ); + assert_eq!( + serde_json::from_str::( + &requests[1] + .function_call_output_text(READ_CALL_ID) + .expect("skills.read should return the referenced resource"), + )?, + json!({ + "resource": REFERENCED_RESOURCE, + "contents": REFERENCED_CONTENTS, + "next_cursor": null, + }) + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn production_turn_scales_extension_catalog_from_resolved_model_window() -> Result<()> { let skill_count = 800; diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index 835e213635..335308fce0 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -40,8 +40,8 @@ use crate::catalog::SkillCatalogEntry; use crate::catalog::SkillReadResult; use crate::catalog::SkillSourceKind; use crate::fragments::AvailableSkillsInstructions; -use crate::fragments::ExecutorSkillResourceAccess; use crate::fragments::SkillInstructions; +use crate::fragments::SkillResourceAccess; use crate::provider::HostSkillProvider; use crate::provider::SkillListQuery; use crate::provider::SkillReadRequest; @@ -68,6 +68,7 @@ use crate::state::HostSkillsStepState; use crate::state::SkillsSessionState; use crate::state::SkillsThreadState; use crate::state::SkillsTurnState; +use crate::tools::SkillToolAuthority; use crate::tools::skill_tools; use crate::warnings::bounded_warnings; use crate::world_state::HostSkillsWarningEmitter; @@ -640,10 +641,11 @@ where ) .0, contents, - executor_resource_access: (!entry.prompt_visible - && entry.authority.kind == SkillSourceKind::Executor) - .then(|| ExecutorSkillResourceAccess { - authority_id: entry.authority.id.clone(), + resource_access: (!entry.prompt_visible) + .then_some(&entry.authority) + .and_then(SkillToolAuthority::from_authority) + .map(|authority| SkillResourceAccess { + authority, package: entry.id.0.clone(), main_resource: entry.main_prompt.as_str().to_string(), }), diff --git a/codex-rs/ext/skills/src/fragments.rs b/codex-rs/ext/skills/src/fragments.rs index 6251a427f2..4b84edb179 100644 --- a/codex-rs/ext/skills/src/fragments.rs +++ b/codex-rs/ext/skills/src/fragments.rs @@ -5,6 +5,7 @@ use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; use crate::catalog_prompt::SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS; use crate::catalog_prompt::SKILLS_HOW_TO_USE_WITH_ALIASES; use crate::catalog_prompt::render_available_skills_body; +use crate::tools::SkillToolAuthority; #[derive(Clone, Debug, Eq, PartialEq)] pub(crate) struct AvailableSkillsInstructions { @@ -57,12 +58,12 @@ pub(crate) struct SkillInstructions { pub(crate) name: String, pub(crate) path: String, pub(crate) contents: String, - pub(crate) executor_resource_access: Option, + pub(crate) resource_access: Option, } #[derive(Clone, Debug, Eq, PartialEq)] -pub(crate) struct ExecutorSkillResourceAccess { - pub(crate) authority_id: String, +pub(crate) struct SkillResourceAccess { + pub(crate) authority: SkillToolAuthority, pub(crate) package: String, pub(crate) main_resource: String, } @@ -85,14 +86,11 @@ impl ContextualUserFragment for SkillInstructions { let path = &self.path; let contents = &self.contents; let resource_access = self - .executor_resource_access + .resource_access .as_ref() .map(|access| { let metadata = serde_json::json!({ - "authority": { - "kind": "executor", - "id": access.authority_id, - }, + "authority": access.authority, "package": access.package, "main_resource": access.main_resource, }); diff --git a/codex-rs/ext/skills/src/provider/orchestrator.rs b/codex-rs/ext/skills/src/provider/orchestrator.rs index 17e6968350..065d9db69d 100644 --- a/codex-rs/ext/skills/src/provider/orchestrator.rs +++ b/codex-rs/ext/skills/src/provider/orchestrator.rs @@ -24,9 +24,14 @@ use crate::provider::SkillSearchRequest; const ORCHESTRATOR_SKILL_MIME_TYPE: &str = "mcp/skill"; const ORCHESTRATOR_SKILL_DISCOVERY_TIMEOUT: Duration = Duration::from_secs(10); +const ORCHESTRATOR_SKILL_DISCOVERY_DURATION_METRIC: &str = + "codex.skills.orchestrator.discovery.duration_ms"; +const ORCHESTRATOR_SKILL_DISCOVERY_RESOURCES_METRIC: &str = + "codex.skills.orchestrator.discovery.resources_total"; const ORCHESTRATOR_SKILL_READ_TIMEOUT: Duration = Duration::from_secs(10); const MAX_RESOURCE_PAGES: usize = 10; const MAX_ORCHESTRATOR_SKILLS: usize = 100; +const MAX_HIDDEN_ORCHESTRATOR_SKILLS: usize = MAX_RESOURCE_PAGES * MAX_ORCHESTRATOR_SKILLS; const MAX_SKILL_NAME_CHARS: usize = 64; const MAX_QUALIFIED_SKILL_NAME_CHARS: usize = 128; const MAX_SKILL_PACKAGE_URI_CHARS: usize = 1_024; @@ -54,15 +59,20 @@ impl SkillProvider for OrchestratorSkillProvider { return Ok(SkillCatalog::default()); } + let _discovery_timer = + codex_otel::start_global_timer(ORCHESTRATOR_SKILL_DISCOVERY_DURATION_METRIC, &[]) + .ok(); let discovery_deadline = tokio::time::Instant::now() + ORCHESTRATOR_SKILL_DISCOVERY_TIMEOUT; let mut catalog = SkillCatalog::default(); let mut cursor = None; let mut seen_cursors = HashSet::new(); - let mut skill_resources_seen = 0usize; + let mut visible_skills_seen = 0usize; + let mut hidden_skills_seen = 0usize; let mut skipped_resources = 0usize; let mut truncated = false; let mut completed_pages = 0usize; + let mut total_resources = 0usize; for _ in 0..MAX_RESOURCE_PAGES { let page = match tokio::time::timeout_at( @@ -98,25 +108,33 @@ impl SkillProvider for OrchestratorSkillProvider { } }; completed_pages = completed_pages.saturating_add(1); + total_resources = total_resources.saturating_add(result.resources.len()); for resource in &result.resources { if resource.mime_type.as_deref() != Some(ORCHESTRATOR_SKILL_MIME_TYPE) { continue; } - if skill_resources_seen >= MAX_ORCHESTRATOR_SKILLS { - truncated = true; - break; - } - skill_resources_seen = skill_resources_seen.saturating_add(1); match catalog_entry_from_resource(resource) { - Some(entry) => catalog.push_entry(entry), + Some(entry) => { + if entry.prompt_visible { + if visible_skills_seen >= MAX_ORCHESTRATOR_SKILLS { + truncated = true; + continue; + } + visible_skills_seen = visible_skills_seen.saturating_add(1); + } else { + if hidden_skills_seen >= MAX_HIDDEN_ORCHESTRATOR_SKILLS { + truncated = true; + continue; + } + hidden_skills_seen = hidden_skills_seen.saturating_add(1); + } + catalog.push_entry(entry); + } None => skipped_resources = skipped_resources.saturating_add(1), } } - if truncated { - break; - } let Some(next_cursor) = result.next_cursor else { cursor = None; break; @@ -134,7 +152,7 @@ impl SkillProvider for OrchestratorSkillProvider { if cursor.is_some() || truncated { catalog.warnings.push(format!( - "Orchestrator skill discovery was truncated at {MAX_ORCHESTRATOR_SKILLS} skills or {MAX_RESOURCE_PAGES} resource pages." + "Orchestrator skill discovery was truncated at {MAX_ORCHESTRATOR_SKILLS} visible skills, {MAX_HIDDEN_ORCHESTRATOR_SKILLS} hidden skills, or {MAX_RESOURCE_PAGES} resource pages." )); } if skipped_resources > 0 { @@ -143,6 +161,14 @@ impl SkillProvider for OrchestratorSkillProvider { )); } + if let Some(metrics) = codex_otel::global() { + let _ = metrics.histogram( + ORCHESTRATOR_SKILL_DISCOVERY_RESOURCES_METRIC, + i64::try_from(total_resources).unwrap_or(i64::MAX), + &[], + ); + } + Ok(catalog) }) } @@ -221,6 +247,10 @@ impl SkillProvider for OrchestratorSkillProvider { fn catalog_entry_from_resource(resource: &Resource) -> Option { let uri = validated_skill_uri(resource.uri.as_str(), MAX_SKILL_PACKAGE_URI_CHARS)?; let meta = resource.meta.as_ref()?.as_object()?; + let allow_implicit_invocation = meta + .get("allow_implicit_invocation") + .and_then(serde_json::Value::as_bool) + .unwrap_or(true); let skill_name = normalized_label(meta.get("skill_name")?.as_str()?, MAX_SKILL_NAME_CHARS)?; let name = if meta.get("source").and_then(|value| value.as_str()) == Some("user") { skill_name @@ -234,16 +264,20 @@ fn catalog_entry_from_resource(resource: &Resource) -> Option let description = normalized_description(resource.description.as_deref().unwrap_or_default())?; let main_prompt = main_prompt_uri(uri); - Some( - SkillCatalogEntry::new( - SkillPackageId(uri.to_string()), - SkillAuthority::new(SkillSourceKind::Orchestrator, CODEX_APPS_MCP_SERVER_NAME), - name, - description, - SkillResourceId::new(main_prompt), - ) - .with_display_path(uri), + let entry = SkillCatalogEntry::new( + SkillPackageId(uri.to_string()), + SkillAuthority::new(SkillSourceKind::Orchestrator, CODEX_APPS_MCP_SERVER_NAME), + name, + description, + SkillResourceId::new(main_prompt), ) + .with_display_path(uri); + + Some(if allow_implicit_invocation { + entry + } else { + entry.hidden_from_prompt() + }) } fn validated_skill_uri(uri: &str, max_chars: usize) -> Option<&str> { diff --git a/codex-rs/ext/skills/src/tools/mod.rs b/codex-rs/ext/skills/src/tools/mod.rs index 58a7a656ae..a754af42ca 100644 --- a/codex-rs/ext/skills/src/tools/mod.rs +++ b/codex-rs/ext/skills/src/tools/mod.rs @@ -135,7 +135,7 @@ impl SkillToolAuthoritySelector { #[derive(Clone, Debug, Deserialize, Eq, Hash, JsonSchema, PartialEq, Serialize)] #[serde(tag = "kind", rename_all = "snake_case", deny_unknown_fields)] -enum SkillToolAuthority { +pub(crate) enum SkillToolAuthority { Orchestrator, Executor { id: String }, } @@ -148,7 +148,7 @@ impl SkillToolAuthority { } } - fn from_authority(authority: &SkillAuthority) -> Option { + pub(crate) fn from_authority(authority: &SkillAuthority) -> Option { match &authority.kind { SkillSourceKind::Orchestrator if authority.id == CODEX_APPS_MCP_SERVER_NAME => { Some(Self::Orchestrator)