From fe8500c0a00eaddda49b68e6ade818a93b58dfb5 Mon Sep 17 00:00:00 2001 From: jif Date: Fri, 24 Jul 2026 14:43:14 +0000 Subject: [PATCH] Enable resource reads for explicit executor skills (#35198) ## Why Executor skills that disallow implicit invocation are omitted from `skills.list`, so an explicitly selected skill did not provide the authority and package identifiers needed to read its referenced resources. ## What changed - Add `resource_access` metadata to injected instructions for explicit-only executor skills. - Teach the `skills.read` description to accept that metadata as well as values returned by `skills.list`. - Keep discoverable executor skill instructions unchanged. ## Testing Add an app-server integration scenario that selects an explicit-only executor skill, confirms it remains absent from `skills.list`, and reads a referenced file through `skills.read`. GitOrigin-RevId: 97d3b93d1618c013363788452303c8a31e32cc56 --- .../tests/suite/v2/executor_skills.rs | 197 ++++++++++++------ codex-rs/ext/skills/src/extension.rs | 8 + codex-rs/ext/skills/src/fragments.rs | 25 ++- codex-rs/ext/skills/src/tools/read.rs | 2 +- 4 files changed, 166 insertions(+), 66 deletions(-) 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 2af106f385..9f67833674 100644 --- a/codex-rs/app-server/tests/suite/v2/executor_skills.rs +++ b/codex-rs/app-server/tests/suite/v2/executor_skills.rs @@ -28,8 +28,23 @@ const SKILL_MARKER: &str = "EXECUTOR_SKILL_BODY_MARKER"; const LOCAL_SKILL_MARKER: &str = "LOCAL_SKILL_BODY_MARKER"; const REFERENCE_MARKER: &str = "EXECUTOR_SKILL_REFERENCE_MARKER"; +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum ExecutorSkillScenario { + VisibleWithBudgetWarning, + ExplicitOnly, +} + #[tokio::test] async fn selected_executor_root_exposes_plugin_skill_and_forwards_budget_warning() -> Result<()> { + exercise_executor_skill(ExecutorSkillScenario::VisibleWithBudgetWarning).await +} + +#[tokio::test] +async fn explicit_executor_skill_can_read_referenced_file() -> Result<()> { + exercise_executor_skill(ExecutorSkillScenario::ExplicitOnly).await +} + +async fn exercise_executor_skill(scenario: ExecutorSkillScenario) -> Result<()> { let server = responses::start_mock_server().await; let codex_home = TempDir::new()?; std::fs::write( @@ -71,9 +86,10 @@ stream_max_retries = 0 let plugin_dir = auto_env.selection().cwd.join("plugin")?; let manifest_dir = plugin_dir.join(".codex-plugin")?; let skill_dir = plugin_dir.join("skills/deploy")?; + let agents_dir = skill_dir.join("agents")?; let reference_dir = skill_dir.join("references")?; let file_system = auto_env.environment().get_filesystem(); - for directory in [&manifest_dir, &reference_dir] { + for directory in [&manifest_dir, &agents_dir, &reference_dir] { file_system .create_directory( directory, @@ -84,8 +100,14 @@ stream_max_retries = 0 } let manifest_path = manifest_dir.join("plugin.json")?; let skill_path = skill_dir.join("SKILL.md")?; + let openai_yaml_path = agents_dir.join("openai.yaml")?; let reference_path = reference_dir.join("details.md")?; - let reference_contents = format!("{REFERENCE_MARKER}\n{}", "x".repeat(600 * 1024)); + let reference_size = match scenario { + ExecutorSkillScenario::VisibleWithBudgetWarning => 600 * 1024, + ExecutorSkillScenario::ExplicitOnly => 40 * 1024, + }; + let allow_implicit_invocation = scenario == ExecutorSkillScenario::VisibleWithBudgetWarning; + let reference_contents = format!("{REFERENCE_MARKER}\n{}", "x".repeat(reference_size)); tokio::try_join!( file_system.write_file( &manifest_path, @@ -100,43 +122,53 @@ stream_max_retries = 0 .into_bytes(), /*sandbox*/ None, ), + file_system.write_file( + &openai_yaml_path, + format!( + "policy:\n allow_implicit_invocation: {allow_implicit_invocation}\n" + ) + .into_bytes(), + /*sandbox*/ None, + ), file_system.write_file( &reference_path, reference_contents.into_bytes(), /*sandbox*/ None, ), )?; - futures::stream::iter(0..200) - .map(|index| { - let file_system = file_system.clone(); - let plugin_dir = plugin_dir.clone(); - async move { - let relative = format!("skills/skill-{index:03}"); - let skill_dir = plugin_dir.join(&relative)?; - file_system - .create_directory( - &skill_dir, - CreateDirectoryOptions { recursive: true }, - /*sandbox*/ None, - ) - .await?; - file_system - .write_file( - &skill_dir.join("SKILL.md")?, - format!( - "---\nname: skill-{index:03}\ndescription: {}\n---\n", - "x".repeat(1_025) + if scenario == ExecutorSkillScenario::VisibleWithBudgetWarning { + futures::stream::iter(0..200) + .map(|index| { + let file_system = file_system.clone(); + let plugin_dir = plugin_dir.clone(); + async move { + let relative = format!("skills/skill-{index:03}"); + let skill_dir = plugin_dir.join(&relative)?; + file_system + .create_directory( + &skill_dir, + CreateDirectoryOptions { recursive: true }, + /*sandbox*/ None, ) - .into_bytes(), - /*sandbox*/ None, - ) - .await?; - Ok::<(), anyhow::Error>(()) - } - }) - .buffer_unordered(16) - .try_collect::>() - .await?; + .await?; + file_system + .write_file( + &skill_dir.join("SKILL.md")?, + format!( + "---\nname: skill-{index:03}\ndescription: {}\n---\n", + "x".repeat(1_025) + ) + .into_bytes(), + /*sandbox*/ None, + ) + .await?; + Ok::<(), anyhow::Error>(()) + } + }) + .buffer_unordered(16) + .try_collect::>() + .await?; + } let authority_id = "demo-plugin@1"; let locator = |path: &PathUri| { @@ -231,24 +263,26 @@ stream_max_retries = 0 app_server.read_stream_until_response_message(RequestId::Integer(request_id)), ) .await??; - let warning = timeout(READ_TIMEOUT, async { - loop { - let warning: WarningNotification = app_server.read_notification("warning").await?; - if warning + if scenario == ExecutorSkillScenario::VisibleWithBudgetWarning { + let warning = timeout(READ_TIMEOUT, async { + loop { + let warning: WarningNotification = app_server.read_notification("warning").await?; + if warning + .message + .starts_with("Exceeded skills context budget.") + { + return Ok::(warning); + } + } + }) + .await??; + assert_eq!(warning.thread_id, Some(thread_id)); + assert!( + warning .message .starts_with("Exceeded skills context budget.") - { - return Ok::(warning); - } - } - }) - .await??; - assert_eq!(warning.thread_id, Some(thread_id)); - assert!( - warning - .message - .starts_with("Exceeded skills context budget.") - ); + ); + } timeout( READ_TIMEOUT, app_server.read_stream_until_notification_message("turn/completed"), @@ -275,26 +309,54 @@ stream_max_retries = 0 assert!(skill_fragment.contains(&format!("{SKILL_NAME}"))); assert!(skill_fragment.contains(SKILL_MARKER)); assert!(!skill_fragment.contains(LOCAL_SKILL_MARKER)); + match scenario { + ExecutorSkillScenario::VisibleWithBudgetWarning => { + assert!(!skill_fragment.contains("")); + } + ExecutorSkillScenario::ExplicitOnly => { + let resource_access = skill_fragment + .split_once("") + .and_then(|(_, rest)| rest.split_once("")) + .map(|(metadata, _)| serde_json::from_str::(metadata)) + .transpose()? + .expect("explicit executor skill should include resource access metadata"); + assert_eq!( + resource_access, + json!({ + "authority": {"kind": "executor", "id": authority_id}, + "package": package, + "main_resource": main_resource, + }) + ); + } + } let list_output = serde_json::from_str::( &requests[1] .function_call_output_text("list") .expect("skills.list output"), )?; - let deploy_skill = list_output["skills"] - .as_array() - .and_then(|skills| skills.iter().find(|skill| skill["name"] == SKILL_NAME)) - .expect("skills.list should include the selected executor skill"); - assert_eq!( - deploy_skill, - &json!({ - "authority": {"kind": "executor", "id": authority_id}, - "package": package, - "name": SKILL_NAME, - "description": "Deploy through the executor.", - "main_resource": main_resource, - }) - ); - assert!(list_output["next_cursor"].is_string()); + match scenario { + ExecutorSkillScenario::VisibleWithBudgetWarning => { + let deploy_skill = list_output["skills"] + .as_array() + .and_then(|skills| skills.iter().find(|skill| skill["name"] == SKILL_NAME)) + .expect("skills.list should include the selected executor skill"); + assert_eq!( + deploy_skill, + &json!({ + "authority": {"kind": "executor", "id": authority_id}, + "package": package, + "name": SKILL_NAME, + "description": "Deploy through the executor.", + "main_resource": main_resource, + }) + ); + assert!(list_output["next_cursor"].is_string()); + } + ExecutorSkillScenario::ExplicitOnly => { + assert_eq!(list_output["skills"], json!([])); + } + } assert!( requests[2] .function_call_output_text("main") @@ -311,7 +373,14 @@ stream_max_retries = 0 .as_str() .is_some_and(|contents| contents.contains(REFERENCE_MARKER)) ); - assert!(reference_output["next_cursor"].is_string()); + match scenario { + ExecutorSkillScenario::VisibleWithBudgetWarning => { + assert!(reference_output["next_cursor"].is_string()); + } + ExecutorSkillScenario::ExplicitOnly => { + assert!(reference_output["next_cursor"].is_null()); + } + } Ok(()) } diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index 36c23f52e4..6c58b36c7e 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -36,6 +36,7 @@ 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::provider::HostSkillProvider; use crate::provider::SkillListQuery; @@ -464,6 +465,13 @@ where ) .0, contents, + executor_resource_access: (!entry.prompt_visible + && entry.authority.kind == SkillSourceKind::Executor) + .then(|| ExecutorSkillResourceAccess { + authority_id: entry.authority.id.clone(), + package: entry.id.0.clone(), + main_resource: entry.main_prompt.as_str().to_string(), + }), }; fragments.push(Box::new(fragment)); main_prompts_injected = true; diff --git a/codex-rs/ext/skills/src/fragments.rs b/codex-rs/ext/skills/src/fragments.rs index 21e9366982..041fb5b3b7 100644 --- a/codex-rs/ext/skills/src/fragments.rs +++ b/codex-rs/ext/skills/src/fragments.rs @@ -77,6 +77,14 @@ pub(crate) struct SkillInstructions { pub(crate) name: String, pub(crate) path: String, pub(crate) contents: String, + pub(crate) executor_resource_access: Option, +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) struct ExecutorSkillResourceAccess { + pub(crate) authority_id: String, + pub(crate) package: String, + pub(crate) main_resource: String, } impl ContextualUserFragment for SkillInstructions { @@ -96,6 +104,21 @@ impl ContextualUserFragment for SkillInstructions { let name = &self.name; let path = &self.path; let contents = &self.contents; - format!("\n{name}\n{path}\n{contents}\n") + let resource_access = self + .executor_resource_access + .as_ref() + .map(|access| { + let metadata = serde_json::json!({ + "authority": { + "kind": "executor", + "id": access.authority_id, + }, + "package": access.package, + "main_resource": access.main_resource, + }); + format!("\n{metadata}") + }) + .unwrap_or_default(); + format!("\n{name}\n{path}{resource_access}\n{contents}\n") } } diff --git a/codex-rs/ext/skills/src/tools/read.rs b/codex-rs/ext/skills/src/tools/read.rs index 13e2f0899a..b0c71cb74d 100644 --- a/codex-rs/ext/skills/src/tools/read.rs +++ b/codex-rs/ext/skills/src/tools/read.rs @@ -56,7 +56,7 @@ impl ToolExecutor for ReadTool { fn spec(&self) -> ToolSpec { skill_function_tool::( TOOL_NAME, - "Read one page from a skill resource. Pass the exact authority and package from skills.list, plus its main_resource or a referenced resource beneath that package. Pass next_cursor back as cursor to continue.", + "Read one page from a skill resource. Pass the exact authority and package from skills.list or an explicitly selected skill's resource_access metadata, plus its main_resource or a referenced resource beneath that package. Pass next_cursor back as cursor to continue.", ) }