From 13f580ef090887bf0dfcdd366fa783ac553e273e Mon Sep 17 00:00:00 2001 From: Anthony Tafoya Date: Tue, 29 Sep 2026 01:48:11 +0000 Subject: [PATCH] Deduplicate cloud and executor skill listings before budgeting (#49127) ## Why Skills available from both cloud and executor providers consume duplicate catalog space, leaving less room for descriptions of unique skills. ## What changed - Prefer model-visible cloud skills over executor listings with the same `plugin:skill` name before allocating the metadata budget. - Preserve aliases from the full visible executor inventory so filtering duplicates does not renumber remaining packages. Continue choosing full locators when they fit the budget better. - Keep executor packages available through `skills.list` and `skills.read`, and retain executor listings when cloud skills are disabled. ## Testing Add regression tests and a model-request snapshot covering description retention, stable aliases across turns, executor availability changes, package reads, and full-locator selection after deduplication. GitOrigin-RevId: 477e4cf16b8968dbe61438dd5af1eca9cbfa78c8 --- codex-rs/core/tests/suite/scenarios.rs | 3 + .../suite/scenarios_skill_catalog_dedup.rs | 151 +++++++++++ codex-rs/core/tests/suite/skills_extension.rs | 4 +- ...catalog_dedup__dedup_before_budgeting.snap | 125 +++++++++ codex-rs/ext/skills/src/render.rs | 143 +++-------- codex-rs/ext/skills/src/render_dedup.rs | 103 ++++++++ codex-rs/ext/skills/src/render_tests.rs | 74 ++++++ .../ext/skills/src/world_state_catalogs.rs | 51 +++- codex-rs/ext/skills/tests/skills_extension.rs | 3 + .../presentation_dedup_tests.rs | 239 ++++++++++++++++++ 10 files changed, 775 insertions(+), 121 deletions(-) create mode 100644 codex-rs/core/tests/suite/scenarios_skill_catalog_dedup.rs create mode 100644 codex-rs/core/tests/suite/snapshots/all__suite__scenarios__skill_catalog_dedup__dedup_before_budgeting.snap create mode 100644 codex-rs/ext/skills/src/render_dedup.rs create mode 100644 codex-rs/ext/skills/tests/skills_extension/presentation_dedup_tests.rs diff --git a/codex-rs/core/tests/suite/scenarios.rs b/codex-rs/core/tests/suite/scenarios.rs index 43ab5ec6bd..9f2e33dbfb 100644 --- a/codex-rs/core/tests/suite/scenarios.rs +++ b/codex-rs/core/tests/suite/scenarios.rs @@ -130,6 +130,9 @@ mod mxc; #[path = "scenarios_tools_namespace_budget.rs"] mod tools_namespace_budget; +#[path = "scenarios_skill_catalog_dedup.rs"] +mod skill_catalog_dedup; + fn skills_extensions() -> Arc> { let mut extensions = ExtensionRegistryBuilder::::new(); install(&mut extensions, |config: &Config| SkillsExtensionConfig { diff --git a/codex-rs/core/tests/suite/scenarios_skill_catalog_dedup.rs b/codex-rs/core/tests/suite/scenarios_skill_catalog_dedup.rs new file mode 100644 index 0000000000..155b207161 --- /dev/null +++ b/codex-rs/core/tests/suite/scenarios_skill_catalog_dedup.rs @@ -0,0 +1,151 @@ +//! Snapshots cloud-preferred skill deduplication and stable executor aliases in model requests. + +use std::sync::Arc; + +use anyhow::Result; +use codex_core::config::Config; +use codex_extension_api::ExtensionRegistryBuilder; +use codex_features::Feature; +use codex_skills_extension::SkillProviders; +use codex_skills_extension::SkillsExtensionConfig; +use codex_skills_extension::catalog::SkillAuthority; +use codex_skills_extension::catalog::SkillCatalog; +use codex_skills_extension::catalog::SkillCatalogEntry; +use codex_skills_extension::catalog::SkillPackageId; +use codex_skills_extension::catalog::SkillResourceId; +use codex_skills_extension::catalog::SkillSourceKind; +use codex_skills_extension::install_with_providers; +use core_test_support::context_snapshot; +use core_test_support::context_snapshot::ContextSnapshotOptions; +use core_test_support::responses; +use core_test_support::skip_if_no_network; +use core_test_support::test_codex::test_codex; +use pretty_assertions::assert_eq; + +use super::super::skills_extension::CatalogSkillProvider; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn cloud_preference_preserves_executor_aliases_and_description_budget() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let roots = [ + format!("skill://executor-a/{}", "a".repeat(180)), + format!("skill://executor-b/{}", "b".repeat(180)), + ]; + let description = format!("{} UNIQUE_DESCRIPTION_END", "Skill guidance. ".repeat(60)); + let mut executor_entries = Vec::new(); + for (plugin, root) in ["demo", "other"].into_iter().zip(&roots) { + for index in 0..3 { + let package = format!("{root}/s{index}"); + executor_entries.push( + SkillCatalogEntry::new( + SkillPackageId(package.clone()), + SkillAuthority::new(SkillSourceKind::Executor, "executor"), + format!("{plugin}:s{index}"), + &description, + SkillResourceId::new(format!("{package}/SKILL.md")), + ) + .with_alias_root(root), + ); + } + } + let cloud_entries = (0..3) + .map(|index| { + SkillCatalogEntry::new( + SkillPackageId(format!("skill://cloud/s{index}")), + SkillAuthority::new(SkillSourceKind::Cloud, "cloud"), + format!("demo:s{index}"), + "Cloud skill instructions.", + SkillResourceId::new(format!("skill://cloud/s{index}/SKILL.md")), + ) + }) + .collect(); + let provider = |entries| { + Arc::new(CatalogSkillProvider { + catalog: SkillCatalog { + entries, + warnings: Vec::new(), + }, + }) + }; + let mut extensions = ExtensionRegistryBuilder::new(); + install_with_providers( + &mut extensions, + SkillProviders::new() + .with_executor_provider(provider(executor_entries)) + .with_cloud_provider(provider(cloud_entries)), + |config: &Config| SkillsExtensionConfig { + include_instructions: config.include_skill_instructions, + max_context_tokens: config.skill_max_context_tokens, + bundled_skills_enabled: false, + cloud_skill_enabled: true, + shadow_selection_enabled: false, + }, + ); + let mock = responses::mount_sse_sequence( + &server, + (0..2) + .map(|index| { + responses::sse(vec![ + responses::ev_assistant_message( + &format!("message-{index}"), + "Skills are available.", + ), + responses::ev_completed(&format!("response-{index}")), + ]) + }) + .collect(), + ) + .await; + let test = test_codex() + .with_exec_server_url("none") + .with_extensions(Arc::new(extensions.build())) + .with_model_info_override("gpt-6-astra", |model| { + model.context_window = Some(70_000); + model.max_context_window = None; + model.include_skills_usage_instructions = false; + }) + .with_config(|config| { + super::configure_scenario_catalog(config); + config.cloud_skill_enabled = true; + config + .features + .enable(Feature::ExecutorCapabilityDiscovery) + .expect("enable executor capability discovery"); + }) + .build_with_auto_env(&server) + .await?; + test.submit_turn("Which skills are available?").await?; + test.submit_turn("Are those same skills still available?") + .await?; + + let requests = mock.requests(); + assert_eq!(requests.len(), 2); + for (request_index, request) in requests.iter().enumerate() { + let body = request.message_input_texts("developer").join("\n"); + for index in 0..3 { + assert_eq!( + body.matches(&format!("- demo:s{index}:")).count(), + 1, + "request {request_index}" + ); + assert!(body.contains(&format!("(cloud package: skill://cloud/s{index})"))); + assert!(body.contains(&format!("(executor package: e1/s{index})"))); + assert!(!body.contains(&format!("(executor package: e0/s{index})"))); + } + for (index, root) in roots.iter().enumerate() { + assert!(body.contains(&format!("- `e{index}` = `{root}`"))); + } + assert_eq!(body.matches("UNIQUE_DESCRIPTION_END").count(), 3); + } + insta::assert_snapshot!( + "dedup_before_budgeting", + context_snapshot::format_request_history_snapshot( + "Cloud skills replace duplicate executor listings before budgeting; unique executor skills retain their descriptions and e1 aliases across turns.", + &requests, + &ContextSnapshotOptions::default().include_request_settings(), + ) + ); + Ok(()) +} diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index ec7e757581..5f116a99d1 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -128,8 +128,8 @@ struct StaticSkillProvider { main_prompt_contents: Option, } -struct CatalogSkillProvider { - catalog: SkillCatalog, +pub(super) struct CatalogSkillProvider { + pub(super) catalog: SkillCatalog, } struct PausedCatalogSkillProvider { diff --git a/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__skill_catalog_dedup__dedup_before_budgeting.snap b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__skill_catalog_dedup__dedup_before_budgeting.snap new file mode 100644 index 0000000000..4a1c05002d --- /dev/null +++ b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__skill_catalog_dedup__dedup_before_budgeting.snap @@ -0,0 +1,125 @@ +--- +source: core/tests/suite/scenarios_skill_catalog_dedup.rs +expression: "context_snapshot::format_request_history_snapshot(\"Cloud skills replace duplicate executor listings before budgeting; unique executor skills retain their descriptions and e1 aliases across turns.\",\n&requests, &ContextSnapshotOptions::default().include_request_settings(),)" +--- +Scenario: Cloud skills replace duplicate executor listings before budgeting; unique executor skills retain their descriptions and e1 aliases across turns. + +## Window 1 +Settings: + include: ["reasoning.encrypted_content"] + model: "gpt-6-astra" + parallel_tool_calls: false + prompt_cache_key: "" + reasoning: {"context":"all_turns","effort":"low"} + store: false + stream: true + text: {"verbosity":"low"} + tool_choice: "auto" +-- request 1 (turn) -- +00:additional_tools/developer (3; hash=c803bbbd445f67cc): + - namespace/functions; hash=48b2ce07cd26a39b + - custom/exec + - function/wait + - function/request_user_input + - function/request_user_input_async + - namespace/clock: Tools for reading and waiting on time.; hash=f402e8c5e9b5e317 + - function/sleep + - namespace/collaboration: Tools for spawning and managing sub-agents.; hash=fd994b11525da73c + - function/followup_task + - function/interrupt_agent + - function/list_agents + - function/send_message + - function/spawn_agent + - function/wait_agent +01:message/developer: + You are Codex, an agent based on GPT-6. You and the user share one workspace, and your job is to collabo...them until their intended goal is completely handled. [hash=efa50f074efa5803] + + # When to ask the user for permission + + Use your best judgement given task context for when you really need user permission, like a competent co...ork without ending the turn to clarify with the user. [hash=ca7367e658b51388] + + User authorization and preferences persist across turns. Do not request permission again when the user h... any guidelines provided in skills or external files. [hash=911f91c92c72882c] + + + + - Skill naming: If a plugin contributes skills, those skill entries are prefixed with plugin_name: in the Skills list. + - MCP naming: Plugin-provided MCP tools keep standard MCP identifiers such as mcp__server__tool; use tool provenance to tell which plugin they come from. + - Trigger rules: If the user explicitly names a plugin, prefer capabilities associated with that plugin for that turn. + - Relationship to capabilities: Plugins are not invoked directly. Use their underlying skills, MCP tools, and app tools to help solve the task. + - Relevance: Determine what a plugin can help with from explicit user mention or from the plugin-associa..., MCP tools, and apps exposed elsewhere in this turn. [hash=620459a4e7b590f0] + - Missing/blocked: If the user requests a plugin that does not have relevant callable capabilities for t..., say so briefly and continue with the best fallback. [hash=746b0469a8f6cb3e] + +02:message/developer[4]: + [01] + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `danger-full-access`...ll commands are permitted. Network access is enabled. [hash=c134061cebb94a1b] + Approval policy is currently never. Do not provide the `sandbox_permissions` for any reason, commands will be rejected. + + [02] # Collaboration Mode: Default + + You are now in Default mode. Any previous instructions for other modes (e.g. Plan mode) are no longer active. + + Your active mode changes only when new developer instructions with a different `...<...by themselves. Known mode names are Default and Plan. [hash=e75f6b12fc52d812] + + ## request_user_input availability + + Use the `request_user_input` tool only when it is listed in the available tools for this turn. + + Use the `request_user_input` tool only for optional questions where the answer would materially improve the quality of the work. + + If `request_user_input` returns no answers, continue with best judgment instead of asking again or treating the turn as blocked. + + Never use the `request_user_input` tool for permission requests or permission-related escalations. + + [03] + ## Skills + A skill is a set of instructions provided through a `SKILL.md` source. Below is the list of skills that ...locators can be expanded using the skill roots table. [hash=f00d6a4d3e192f06] + ### Skill roots + - `e0` = `skill://executor-a/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa` [hash=89039f2551ec08fb] + - `e1` = `skill://executor-b/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb...bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb` [hash=aec7776011a3143f] + Read a skill package directly with `skills.read({"package":""})` to read its `SKILL.md`; root a...ackage is not provided, use `skills.list` to find it. [hash=25bf84ea2123d647] + ### Available skills + - other:s0: Skill guidance. Skill guidance. Skill guidance. Skill guidance. Skill guidance. Skill guidan...ce. UNIQUE_DESCRIPTION_END (executor package: e1/s0) [hash=1ce5d9766079d21b] + - other:s1: Skill guidance. Skill guidance. Skill guidance. Skill guidance. Skill guidance. Skill guidan...ce. UNIQUE_DESCRIPTION_END (executor package: e1/s1) [hash=d8432891813affeb] + - other:s2: Skill guidance. Skill guidance. Skill guidance. Skill guidance. Skill guidance. Skill guidan...ce. UNIQUE_DESCRIPTION_END (executor package: e1/s2) [hash=1c05fa218dd21e8f] + + [04] + ## Skills + A skill is a set of instructions provided through a `SKILL.md` source. Below is the list of skills that ...urce` locators use their provider's access mechanism. [hash=b6782e3abdba03b2] + Read a skill package directly with `skills.read({"package":""})` to read its `SKILL.md`; root a...ackage is not provided, use `skills.list` to find it. [hash=25bf84ea2123d647] + ### Available skills + - demo:s0: Cloud skill instructions. (cloud package: skill://cloud/s0) + - demo:s1: Cloud skill instructions. (cloud package: skill://cloud/s1) + - demo:s2: Cloud skill instructions. (cloud package: skill://cloud/s2) + +03:message/developer: + You are `/root`, the primary agent in a team of agents collaborating to fulfill the user's goals. + + At the start of your turn, you are the active agent. + You can spawn sub-agents to handle subtasks, and those sub-agents can spawn their own sub-agents. + All agents in the team, including the agents that you can assign tasks to, are equally intelligent and capable, and have access to the same set of tools. + + You can use `spawn_agent` to create a new agent, `followup_task` to give an existing agent a new task an...message to a running agent without triggering a turn. [hash=7d40ff8b58977202] + `send_message` calls may be read by a human, so ensure they are legible. Always put proper spaces between words and/or numbers. + + - All agents use the same current working directory. + - As a result, edits made by one agent are immediately visible to all other agents. + + When calling `wait_agent`, prefer longer waits (minutes) to avoid busy polling. + + There are 4 available concurrency slots, meaning that up to 4 agents can be active at once, including you. + + Full-history forks (`fork_turns` omitted or `"all"`) inherit the parent model and reasoning effort and d...ne"` or a positive integer string. [hash=c7f52e90688299ce] +04:message/developer: + Any earlier instruction enabling proactive multi-agent delegation no longer applies. D...elegation, or parallel agent work. [hash=2b6e037067b4390c] +05:message/user: + + + + +06:message/user: + Which skills are available? +-- request 2 (turn) -- +07:message/assistant: + Skills are available. +08:message/user: + Are those same skills still available? diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index ede504ada0..cfd65d61ba 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -474,6 +474,10 @@ pub(crate) struct RenderedSkillCatalogs { pub(crate) host: Option, } +#[path = "render_dedup.rs"] +mod dedup; +pub(crate) use dedup::PreparedSkillCatalog; + impl AvailableSkillsRender { /// Count the metadata charged by the allocator, including aliases and omission notices. pub(crate) fn metadata_cost( @@ -522,139 +526,65 @@ pub(crate) fn render_available_skills( budget: SkillMetadataBudget, include_skills_usage_instructions: bool, ) -> Option { - let mut entries = catalog - .entries - .iter() - .filter(|entry| entry.is_model_visible()) - .collect::>(); - policy.order_entries(&mut entries); - if entries.is_empty() { - return None; - } - - let absolute = render_catalog( - entries - .iter() - .map(|entry| SkillLine::new(entry, policy)) - .collect(), - budget, - Vec::new(), - SkillPromptKind::Unaliased, - policy, - ); - let selected = if let Some(aliased) = - build_aliased_catalog(&entries, policy, budget, include_skills_usage_instructions) - && aliased_render_is_better( - &aliased, - &absolute, - budget, - include_skills_usage_instructions, - ) { - aliased - } else { - absolute - }; - - Some(AvailableSkillsRender { - prompt_kind: selected.prompt_kind, - skill_root_lines: selected.skill_root_lines, - skill_lines: selected.skill_lines, - preserve_empty_fragment: policy == SkillCatalogRenderPolicy::CoreCompatible, - report: selected.report, - }) + PreparedSkillCatalog::new(catalog, policy).render(budget, include_skills_usage_instructions) } -pub(crate) fn render_combined_available_skills( - executor_catalog: &SkillCatalog, - cloud_catalog: &SkillCatalog, - host_catalog: &SkillCatalog, +pub(crate) fn render_prepared_skill_catalogs( + executor: &PreparedSkillCatalog<'_>, + cloud: &PreparedSkillCatalog<'_>, + host: &PreparedSkillCatalog<'_>, budget: SkillMetadataBudget, include_skills_usage_instructions: bool, ) -> RenderedSkillCatalogs { - let mut executor_entries = executor_catalog - .entries - .iter() - .filter(|entry| entry.is_model_visible()) - .collect::>(); - let mut cloud_entries = cloud_catalog - .entries - .iter() - .filter(|entry| entry.is_model_visible()) - .collect::>(); - let mut host_entries = host_catalog - .entries - .iter() - .filter(|entry| entry.is_model_visible()) - .collect::>(); - SkillCatalogRenderPolicy::ExtensionCompatible.order_entries(&mut executor_entries); - SkillCatalogRenderPolicy::ExtensionCompatible.order_entries(&mut cloud_entries); - SkillCatalogRenderPolicy::CoreCompatible.order_entries(&mut host_entries); let nonempty_catalog_count = [ - !executor_entries.is_empty(), - !cloud_entries.is_empty(), - !host_entries.is_empty(), + !executor.entries.is_empty(), + !cloud.entries.is_empty(), + !host.entries.is_empty(), ] .into_iter() .filter(|nonempty| *nonempty) .count(); if nonempty_catalog_count <= 1 { return RenderedSkillCatalogs { - executor: render_available_skills( - executor_catalog, - SkillCatalogRenderPolicy::ExtensionCompatible, - budget, - include_skills_usage_instructions, - ), - cloud: render_available_skills( - cloud_catalog, - SkillCatalogRenderPolicy::ExtensionCompatible, - budget, - include_skills_usage_instructions, - ), - host: render_available_skills( - host_catalog, - SkillCatalogRenderPolicy::CoreCompatible, - budget, - include_skills_usage_instructions, - ), + executor: executor.render(budget, include_skills_usage_instructions), + cloud: cloud.render(budget, include_skills_usage_instructions), + host: host.render(budget, include_skills_usage_instructions), }; } - let extension_policy = SkillCatalogRenderPolicy::ExtensionCompatible; - let host_policy = SkillCatalogRenderPolicy::CoreCompatible; let absolute = render_combined_lines( - CatalogLines::unaliased(&executor_entries, extension_policy), - CatalogLines::unaliased(&cloud_entries, extension_policy), - CatalogLines::unaliased(&host_entries, host_policy), + executor.unaliased(), + cloud.unaliased(), + host.unaliased(), budget, ); let mut selected = absolute; let host_only_aliases = build_aliased_combined_catalog( - CatalogLines::unaliased(&executor_entries, extension_policy), - CatalogLines::unaliased(&cloud_entries, extension_policy), - CatalogLines::aliased(&host_entries, host_policy), + executor.unaliased(), + cloud.unaliased(), + host.aliased(), budget, include_skills_usage_instructions, ); let executor_only_aliases = build_aliased_combined_catalog( - CatalogLines::aliased(&executor_entries, extension_policy), - CatalogLines::unaliased(&cloud_entries, extension_policy), - CatalogLines::unaliased(&host_entries, host_policy), + executor.aliased(), + cloud.unaliased(), + host.unaliased(), budget, include_skills_usage_instructions, ); let cloud_only_aliases = build_aliased_combined_catalog( - CatalogLines::unaliased(&executor_entries, extension_policy), - CatalogLines::aliased(&cloud_entries, extension_policy), - CatalogLines::unaliased(&host_entries, host_policy), + executor.unaliased(), + cloud.aliased(), + host.unaliased(), budget, include_skills_usage_instructions, ); let all_source_aliases = build_aliased_combined_catalog( - CatalogLines::aliased(&executor_entries, extension_policy), - CatalogLines::aliased(&cloud_entries, extension_policy), - CatalogLines::aliased(&host_entries, host_policy), + executor.aliased(), + cloud.aliased(), + host.aliased(), budget, include_skills_usage_instructions, ); @@ -709,8 +639,12 @@ impl<'a> CatalogLines<'a> { } } - fn aliased(entries: &[&'a SkillCatalogEntry], policy: SkillCatalogRenderPolicy) -> Self { - let Some(plan) = build_alias_plan(entries) else { + fn aliased( + entries: &[&'a SkillCatalogEntry], + policy: SkillCatalogRenderPolicy, + alias_plan: Option<&AliasPlan>, + ) -> Self { + let Some(plan) = alias_plan.filter(|_| !entries.is_empty()) else { return Self::unaliased(entries, policy); }; @@ -725,7 +659,7 @@ impl<'a> CatalogLines<'a> { SkillLine::with_locator( entry, policy, - render_skill_locator_with_aliases(entry, &plan), + render_skill_locator_with_aliases(entry, plan), ) }) .collect(), @@ -1014,12 +948,11 @@ fn available_skills_fragment( } fn build_aliased_catalog( - entries: &[&SkillCatalogEntry], + catalog: CatalogLines<'_>, policy: SkillCatalogRenderPolicy, budget: SkillMetadataBudget, include_skills_usage_instructions: bool, ) -> Option { - let catalog = CatalogLines::aliased(entries, policy); if catalog.root_lines.is_empty() { return None; } diff --git a/codex-rs/ext/skills/src/render_dedup.rs b/codex-rs/ext/skills/src/render_dedup.rs new file mode 100644 index 0000000000..93902f3435 --- /dev/null +++ b/codex-rs/ext/skills/src/render_dedup.rs @@ -0,0 +1,103 @@ +//! Prefers cloud skills before budgeting while retaining aliases from the full visible inventory. + +use std::collections::HashSet; + +use super::AvailableSkillsRender; +use super::CatalogLines; +use super::SkillCatalogRenderPolicy; +use super::SkillMetadataBudget; +use super::aliased_render_is_better; +use super::build_alias_plan; +use super::build_aliased_catalog; +use super::render_catalog; +use crate::aliases::AliasPlan; +use crate::catalog::SkillCatalog; +use crate::catalog::SkillCatalogEntry; +use crate::catalog_prompt::SkillPromptKind; + +pub(crate) struct PreparedSkillCatalog<'a> { + pub(super) entries: Vec<&'a SkillCatalogEntry>, + alias_plan: Option, + policy: SkillCatalogRenderPolicy, +} + +impl<'a> PreparedSkillCatalog<'a> { + pub(crate) fn new(catalog: &'a SkillCatalog, policy: SkillCatalogRenderPolicy) -> Self { + let mut entries = catalog + .entries + .iter() + .filter(|entry| entry.is_model_visible()) + .collect::>(); + policy.order_entries(&mut entries); + let alias_plan = build_alias_plan(&entries); + Self { + entries, + alias_plan, + policy, + } + } + + pub(crate) fn prefer_cloud_skills(&mut self, cloud: &SkillCatalog) { + let cloud_names: HashSet<_> = cloud + .entries + .iter() + .filter(|entry| entry.is_model_visible()) + .map(|entry| entry.name.as_str()) + .filter(|name| { + name.split_once(':') + .is_some_and(|(plugin, skill)| !plugin.is_empty() && !skill.is_empty()) + }) + .collect(); + + self.entries + .retain(|entry| !cloud_names.contains(entry.name.as_str())); + } + + pub(super) fn unaliased(&self) -> CatalogLines<'a> { + CatalogLines::unaliased(&self.entries, self.policy) + } + + pub(super) fn aliased(&self) -> CatalogLines<'a> { + CatalogLines::aliased(&self.entries, self.policy, self.alias_plan.as_ref()) + } + + pub(crate) fn render( + &self, + budget: SkillMetadataBudget, + include_skills_usage_instructions: bool, + ) -> Option { + if self.entries.is_empty() { + return None; + } + let absolute = render_catalog( + self.unaliased().skills, + budget, + Vec::new(), + SkillPromptKind::Unaliased, + self.policy, + ); + let selected = if let Some(aliased) = build_aliased_catalog( + self.aliased(), + self.policy, + budget, + include_skills_usage_instructions, + ) && aliased_render_is_better( + &aliased, + &absolute, + budget, + include_skills_usage_instructions, + ) { + aliased + } else { + absolute + }; + + Some(AvailableSkillsRender { + prompt_kind: selected.prompt_kind, + skill_root_lines: selected.skill_root_lines, + skill_lines: selected.skill_lines, + preserve_empty_fragment: self.policy == SkillCatalogRenderPolicy::CoreCompatible, + report: selected.report, + }) + } +} diff --git a/codex-rs/ext/skills/src/render_tests.rs b/codex-rs/ext/skills/src/render_tests.rs index 823702a499..58aa7ae014 100644 --- a/codex-rs/ext/skills/src/render_tests.rs +++ b/codex-rs/ext/skills/src/render_tests.rs @@ -20,6 +20,25 @@ use crate::catalog_prompt::render_available_skills_body; use crate::loader::HostSkillRoot; use crate::loader::load_and_merge_host_skill_roots; +fn render_combined_available_skills( + executor_catalog: &SkillCatalog, + cloud_catalog: &SkillCatalog, + host_catalog: &SkillCatalog, + budget: SkillMetadataBudget, + include_skills_usage_instructions: bool, +) -> RenderedSkillCatalogs { + render_prepared_skill_catalogs( + &PreparedSkillCatalog::new( + executor_catalog, + SkillCatalogRenderPolicy::ExtensionCompatible, + ), + &PreparedSkillCatalog::new(cloud_catalog, SkillCatalogRenderPolicy::ExtensionCompatible), + &PreparedSkillCatalog::new(host_catalog, SkillCatalogRenderPolicy::CoreCompatible), + budget, + include_skills_usage_instructions, + ) +} + #[test] fn skill_prompt_contents_are_bounded_at_utf8_boundaries() { let contents = format!("{}é", "a".repeat(MAX_SKILL_PROMPT_BYTES - 1)); @@ -30,6 +49,61 @@ fn skill_prompt_contents_are_bounded_at_utf8_boundaries() { assert_eq!(truncated, true); } +#[test] +fn prebudget_dedup_can_choose_full_locators_over_preserved_aliases() { + let hidden_root = format!("skill://executor/{}", "long-package-root/".repeat(40)); + let mut hidden = entry( + "demo:hidden", + "Hidden duplicate.", + /*short_description*/ None, + ) + .with_alias_root(hidden_root.clone()); + hidden.authority.kind = SkillSourceKind::Executor; + hidden.id = SkillPackageId(format!("{hidden_root}/hidden")); + let mut visible = entry( + "other:visible", + "Keep this description.", + /*short_description*/ None, + ) + .with_alias_root("skill://short"); + visible.authority.kind = SkillSourceKind::Executor; + visible.id = SkillPackageId("skill://short/visible".to_string()); + let executor = SkillCatalog { + entries: vec![hidden.clone(), visible], + warnings: Vec::new(), + }; + hidden.authority.kind = SkillSourceKind::Cloud; + let cloud = SkillCatalog { + entries: vec![hidden], + warnings: Vec::new(), + }; + let mut prepared = + PreparedSkillCatalog::new(&executor, SkillCatalogRenderPolicy::ExtensionCompatible); + prepared.prefer_cloud_skills(&cloud); + let rendered = prepared + .render( + SkillMetadataBudget::Characters(300), + /*include_skills_usage_instructions*/ false, + ) + .expect("remaining executor skill should render"); + assert!(rendered.skill_root_lines.is_empty()); + assert_eq!( + rendered.skill_lines, + vec![ + "- other:visible: Keep this description. (executor package: skill://short/visible)" + .to_string(), + ] + ); + assert_eq!( + rendered.report, + SkillRenderReport { + total_count: 1, + included_count: 1, + ..Default::default() + } + ); +} + fn entry(name: &str, description: &str, short_description: Option<&str>) -> SkillCatalogEntry { entry_with_path( name, diff --git a/codex-rs/ext/skills/src/world_state_catalogs.rs b/codex-rs/ext/skills/src/world_state_catalogs.rs index fc3feccff8..d0b76d38fa 100644 --- a/codex-rs/ext/skills/src/world_state_catalogs.rs +++ b/codex-rs/ext/skills/src/world_state_catalogs.rs @@ -15,11 +15,12 @@ use crate::catalog::SkillCatalog; use crate::provider::SkillListQuery; use crate::provider::attribute_executor_plugins; use crate::render::AvailableSkillsRender; +use crate::render::PreparedSkillCatalog; use crate::render::RenderedSkillCatalogs; use crate::render::SkillCatalogRenderPolicy; use crate::render::SkillMetadataBudget; use crate::render::render_available_skills; -use crate::render::render_combined_available_skills; +use crate::render::render_prepared_skill_catalogs; use crate::render::skill_metadata_budget; use crate::render_observability::CatalogSurface; use crate::render_observability::record_catalog_render; @@ -265,6 +266,11 @@ impl<'a> CatalogContext<'a> { ) -> [RenderedCatalogContribution; 3] { let total_limit = self.metadata_budget.limit(); let default_cloud_limit = total_limit - total_limit / FILESYSTEM_BUDGET_DIVISOR; + let mut executor = PreparedSkillCatalog::new( + &catalogs.executor.catalog, + SkillCatalogRenderPolicy::ExtensionCompatible, + ); + executor.prefer_cloud_skills(&catalogs.cloud.catalog); // Identify the full visible cloud inventory before budgeting, including entries // a previous cap omitted. Executor readiness and provider warnings are not inputs. let cloud_metadata = catalogs @@ -307,14 +313,17 @@ impl<'a> CatalogContext<'a> { }); allocation.cloud_limit = allocation.cloud_limit.min(default_cloud_limit); let (mut rendered, mut filesystem_budget) = - self.render_with_cloud_limit(&catalogs, allocation.cloud_limit); + self.render_with_cloud_limit(&catalogs, &executor, allocation.cloud_limit); - if let Some(cloud_limit) = - self.rebalance_to_retain_all_skills(&catalogs, &rendered, allocation.cloud_limit) - { + if let Some(cloud_limit) = self.rebalance_to_retain_all_skills( + &catalogs, + &executor, + &rendered, + allocation.cloud_limit, + ) { allocation.cloud_limit = cloud_limit; (rendered, filesystem_budget) = - self.render_with_cloud_limit(&catalogs, allocation.cloud_limit); + self.render_with_cloud_limit(&catalogs, &executor, allocation.cloud_limit); } [ @@ -341,6 +350,7 @@ impl<'a> CatalogContext<'a> { fn rebalance_to_retain_all_skills( &self, catalogs: &CatalogContributions, + executor: &PreparedSkillCatalog<'_>, rendered: &RenderedSkillCatalogs, current_cloud_limit: usize, ) -> Option { @@ -352,10 +362,16 @@ impl<'a> CatalogContext<'a> { return None; } - let shared = render_combined_available_skills( - &catalogs.executor.catalog, - &catalogs.cloud.catalog, - &catalogs.host.catalog, + let shared = render_prepared_skill_catalogs( + executor, + &PreparedSkillCatalog::new( + &catalogs.cloud.catalog, + SkillCatalogRenderPolicy::ExtensionCompatible, + ), + &PreparedSkillCatalog::new( + &catalogs.host.catalog, + SkillCatalogRenderPolicy::CoreCompatible, + ), self.metadata_budget, self.include_usage, ); @@ -378,6 +394,7 @@ impl<'a> CatalogContext<'a> { fn render_with_cloud_limit( &self, catalogs: &CatalogContributions, + executor: &PreparedSkillCatalog<'_>, cloud_limit: usize, ) -> (RenderedSkillCatalogs, SkillMetadataBudget) { if !self.config.include_instructions { @@ -398,10 +415,16 @@ impl<'a> CatalogContext<'a> { let filesystem_budget = self .metadata_budget .with_limit(self.metadata_budget.limit().saturating_sub(cloud_cost)); - let mut rendered = render_combined_available_skills( - &catalogs.executor.catalog, - &SkillCatalog::default(), - &catalogs.host.catalog, + let mut rendered = render_prepared_skill_catalogs( + executor, + &PreparedSkillCatalog::new( + &SkillCatalog::default(), + SkillCatalogRenderPolicy::ExtensionCompatible, + ), + &PreparedSkillCatalog::new( + &catalogs.host.catalog, + SkillCatalogRenderPolicy::CoreCompatible, + ), filesystem_budget, self.include_usage, ); diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index f980b7e4a0..5d7ddc6be0 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -89,6 +89,9 @@ type TestResult = Result<(), Box>; #[path = "skills_extension/shadow_task_context_tests.rs"] mod shadow_task_context_tests; +#[path = "skills_extension/presentation_dedup_tests.rs"] +mod presentation_dedup_tests; + static NEXT_CODEX_HOME_ID: AtomicUsize = AtomicUsize::new(0); const SKILLS_INTRO_WITH_ABSOLUTE_PATHS: &str = "A skill is a set of instructions provided through a `SKILL.md` source. Below is the list of skills that can be used. Each entry includes a name, description, and source locator. `file` locators are on the host filesystem, `executor package` locators are owned by their execution environment, `cloud package` locators are opaque package identifiers, and `custom resource` locators use their provider's access mechanism."; const DEMO_SKILL_CONTENTS: &str = diff --git a/codex-rs/ext/skills/tests/skills_extension/presentation_dedup_tests.rs b/codex-rs/ext/skills/tests/skills_extension/presentation_dedup_tests.rs new file mode 100644 index 0000000000..ad078fba53 --- /dev/null +++ b/codex-rs/ext/skills/tests/skills_extension/presentation_dedup_tests.rs @@ -0,0 +1,239 @@ +//! Pre-budget deduplication preserves source packages and aliases across executor readiness changes. + +use super::*; +use codex_exec_server::EnvironmentManager; +use pretty_assertions::assert_eq; + +#[tokio::test] +async fn cloud_preference_preserves_aliases_reads_and_executor_fallback() -> TestResult { + for (cloud_enabled, all_overlap) in [(true, false), (false, false), (true, true)] { + let description = format!("{} UNIQUE_DESCRIPTION_END", "Skill guidance. ".repeat(60)); + let executor_roots = [ + format!("skill://executor-a/{}", "a".repeat(180)), + format!("skill://executor-b/{}", "b".repeat(180)), + ]; + let mut executor_entries = Vec::new(); + for (plugin, root) in ["demo", "other"].into_iter().zip(&executor_roots) { + for index in 0..3 { + let package = format!("{root}/s{index}"); + let mut entry = test_entry( + SkillSourceKind::Executor, + "skills", + &package, + &format!("{package}/SKILL.md"), + ); + entry.name = format!("{plugin}:s{index}"); + entry.description = description.clone(); + executor_entries.push(entry.with_alias_root(root)); + } + } + let mut cloud_entries = Vec::new(); + for plugin in if all_overlap { + vec!["demo", "other"] + } else { + vec!["demo"] + } { + for index in 0..3 { + let mut entry = test_entry( + SkillSourceKind::Cloud, + "codex_apps", + &format!("cloud/{plugin}/s{index}"), + &format!("cloud/{plugin}/s{index}/SKILL.md"), + ); + entry.name = format!("{plugin}:s{index}"); + cloud_entries.push(entry); + } + } + let provider = |entries| { + Arc::new(StaticSkillProvider { + catalog: SkillCatalog { + entries, + warnings: Vec::new(), + }, + read_requests: Arc::new(Mutex::new(Vec::new())), + list_calls: None, + fail_first_list: false, + }) + }; + let providers = SkillProviders::new() + .with_executor_provider(provider(executor_entries.clone())) + .with_cloud_provider(provider(cloud_entries)); + 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 config = TestConfig { + cloud_skill_enabled: cloud_enabled, + ..default_config() + }; + registry.thread_lifecycle_contributors()[0] + .on_thread_start(ThreadStartInput { + config: &config, + session_source: &SessionSource::Cli, + persistent_thread_state_available: true, + environments: &[], + mcp_resource_client: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + }) + .await; + let roots = [SelectedCapabilityRoot { + id: "skills".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "local".to_string(), + path: PathUri::parse("file:///skills")?, + }, + }]; + let resolved_roots = EnvironmentManager::default_for_tests() + .resolve_selected_capability_roots(&roots, &Default::default()) + .await; + assert_eq!(resolved_roots.len(), 1); + let mut previous = serde_json::Map::new(); + let mut initial_cloud_body = None; + let model_info = ModelInfo { + context_window: Some(70_000), + ..catalog_model_info() + }; + for (stage, ready) in [ + ("before", false), + ("connected", true), + ("disconnected", false), + ] { + start_registered_turn(®istry, &session_store, &thread_store, stage).await; + let turn_store = ExtensionData::new(stage); + let sections = registry.context_contributors()[0] + .contribute_world_state(WorldStateContributionInput { + previous_world_state: Some(&previous), + model_info: &model_info, + thread_id: codex_protocol::ThreadId::new(), + turn_id: stage, + environments: &[], + ready_selected_capability_roots: if ready { &roots } else { &[] }, + executor_capability_discovery: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + turn_store: &turn_store, + }) + .await; + let executor_body = world_state_section(§ions, "skills").snapshot()["body"] + .as_str() + .unwrap_or_default() + .to_string(); + let cloud_body = world_state_section(§ions, "cloud_skills").snapshot()["body"] + .as_str() + .unwrap_or_default() + .to_string(); + for index in 0..3 { + let listing = format!("- demo:s{index}:"); + assert_eq!(executor_body.contains(&listing), ready && !cloud_enabled); + assert_eq!(cloud_body.contains(&listing), cloud_enabled); + assert_eq!( + executor_body.contains(&format!("- other:s{index}:")), + ready && !all_overlap + ); + } + assert_eq!( + executor_body.matches("UNIQUE_DESCRIPTION_END").count(), + if ready && cloud_enabled && !all_overlap { + 3 + } else { + 0 + } + ); + if stage == "before" { + initial_cloud_body = Some(cloud_body.clone()); + } else { + assert_eq!(Some(&cloud_body), initial_cloud_body.as_ref()); + } + if !ready || all_overlap { + assert!(executor_body.is_empty()); + } else { + assert!(executor_body.contains("(executor package: e1/s0)")); + for (index, root) in executor_roots.iter().enumerate() { + assert!(executor_body.contains(&format!("- `e{index}` = `{root}`"))); + } + } + previous = sections + .iter() + .map(|section| (section.id().to_string(), section.snapshot().clone())) + .collect(); + if !ready { + continue; + } + turn_store.insert(resolved_roots.clone()); + let tools = registry.tool_contributors()[0].tools_for_step( + &session_store, + &thread_store, + &turn_store, + ); + for (name, arguments, expected_resource) in [ + ( + "list", + serde_json::json!({"authority": {"kind": "executor"}}), + None, + ), + ( + "read", + serde_json::json!({"package": "e0/s0"}), + Some(executor_entries[0].main_prompt.as_str()), + ), + ( + "read", + serde_json::json!({"package": "e1/s0"}), + Some(executor_entries[3].main_prompt.as_str()), + ), + ( + "read", + serde_json::json!({"package": executor_entries[0].id.0}), + Some(executor_entries[0].main_prompt.as_str()), + ), + ] { + let tool = tools + .iter() + .find(|tool| tool.tool_name().name == name) + .ok_or("missing tool")?; + let payload = ToolPayload::Function { + arguments: arguments.to_string(), + }; + let output = tool + .handle(ToolCall { + turn_id: stage.to_string(), + call_id: "call".to_string(), + tool_name: tool.tool_name(), + model: "test".to_string(), + codex_turn_metadata: None, + truncation_policy: TruncationPolicy::Bytes(20_000), + source: ToolCallSource::Direct, + conversation_history: ConversationHistory::default(), + turn_item_emitter: Arc::new(NoopTurnItemEmitter), + environments: Vec::new(), + payload: payload.clone(), + }) + .await?; + let response = output + .post_tool_use_response("call", &payload) + .ok_or("missing response")?; + if let Some(resource) = expected_resource { + assert_eq!(response["resource"], resource); + } else { + assert_eq!( + response["skills"] + .as_array() + .ok_or("missing skills")? + .iter() + .map(|skill| skill["package"].as_str()) + .collect::>(), + executor_entries + .iter() + .map(|entry| Some(entry.id.0.as_str())) + .collect::>() + ); + } + } + } + } + Ok(()) +}