openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-14 21:40:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

Honor explicit-only orchestrator skills (#36976)

Browse Source 
## Why

Orchestrator skills with `allow_implicit_invocation: false` were still exposed in the model-visible skill catalog.

## What changed

- Hide explicit-only orchestrator skills from the prompt catalog while keeping them available for direct invocation.
- Include orchestrator resource-access metadata with injected instructions so `skills.read` can load referenced resources.
- Track visible and hidden discovery limits separately, and record discovery duration and resource-count metrics.

## Testing

Add an integration test that verifies an explicit-only orchestrator skill stays out of the prompt catalog, can be selected explicitly, and can read a referenced resource.

GitOrigin-RevId: 9cab5a61eb27c96bec9d1346c7dcdbc97376f7dc
This commit is contained in:
rphilizaire-openai
2026-08-04 21:42:03 +00:00
committed by copyberry
parent c607da9f37
commit 1b90b1d16b
5 changed files with 264 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/tests/suite/skills_extension.rs
+195
View File
@@ -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::<Value>(&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("<name>demo:explicit-only</name>")
&& 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("<resource_access>")
.and_then(|(_, remainder)| remainder.split_once("</resource_access>"))
.map(|(metadata, _)| metadata)
.expect("hidden orchestrator skills should include resource-access metadata");
assert_eq!(
serde_json::from_str::<Value>(resource_access)?,
json!({
"authority": { "kind": "orchestrator" },
"package": SKILL_PACKAGE,
"main_resource": MAIN_RESOURCE,
})
);
assert_eq!(
serde_json::from_str::<Value>(
&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;
codex-rs/ext/skills/src/extension.rs
+7 -5
View File
@@ -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(),
}),
codex-rs/ext/skills/src/fragments.rs
+6 -8
View File
@@ -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<ExecutorSkillResourceAccess>,
pub(crate) resource_access: Option<SkillResourceAccess>,
}
#[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,
});
codex-rs/ext/skills/src/provider/orchestrator.rs
+54 -20
View File
@@ -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<SkillCatalogEntry> {
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<SkillCatalogEntry>
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> {
codex-rs/ext/skills/src/tools/mod.rs
+2 -2
View File
@@ -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<Self> {
pub(crate) fn from_authority(authority: &SkillAuthority) -> Option<Self> {
match &authority.kind {
SkillSourceKind::Orchestrator if authority.id == CODEX_APPS_MCP_SERVER_NAME => {
Some(Self::Orchestrator)