From 6278742c418a2b3d3d29fd025dfdc13489a27987 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Wed, 22 Jul 2026 11:03:14 +0000 Subject: [PATCH] Preserve skill catalog entries under metadata pressure (#34732) ## Why Long skill descriptions can consume the catalog's metadata budget before later skills are listed, hiding otherwise usable skills from the model. ## What changed - Reserve space for every skill's name and locator when those minimum lines fit. - Distribute the remaining token or character budget across descriptions in round-robin order. - Fall back to omitting entries only when the minimum catalog cannot fit. ## Testing Added extension and production-turn coverage that verifies moderate budget pressure keeps every catalog entry, shortens descriptions evenly, and avoids an omission marker. GitOrigin-RevId: 41971e4b47a86863b3839707b5cffcd8d83b888c --- codex-rs/core/tests/suite/skills_extension.rs | 83 ++++++ codex-rs/ext/skills/src/render.rs | 248 +++++++++++++++--- codex-rs/ext/skills/src/render_tests.rs | 14 +- codex-rs/ext/skills/tests/skills_extension.rs | 82 ++++++ 4 files changed, 384 insertions(+), 43 deletions(-) diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index 9b6585cb0085..2560d9414b43 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -28,6 +28,7 @@ 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::test_codex::test_codex; +use pretty_assertions::assert_eq; struct StaticSkillProvider { catalog: SkillCatalog, @@ -149,3 +150,85 @@ async fn production_turn_scales_extension_catalog_from_resolved_model_window() - Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn production_turn_fairly_shortens_extension_catalog_descriptions() -> Result<()> { + let server = responses::start_mock_server().await; + let response = mount_sse_once( + &server, + sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]), + ) + .await; + let source_kind = SkillSourceKind::Custom("test".to_string()); + let description = "x".repeat(1_025); + let catalog = SkillCatalog { + entries: (0..10) + .map(|index| { + let name = format!("skill-{index:02}"); + SkillCatalogEntry::new( + SkillPackageId(format!("test/{name}")), + SkillAuthority::new(source_kind.clone(), "test"), + name.clone(), + description.clone(), + SkillResourceId::new(format!("{name}/SKILL.md")), + ) + .with_display_path(format!("skill://test/{name}/SKILL.md")) + }) + .collect(), + warnings: Vec::new(), + }; + let mut extensions = ExtensionRegistryBuilder::::new(); + install_with_providers( + &mut extensions, + SkillProviders::new().with_provider(SkillProviderSource::new( + source_kind, + "test", + Arc::new(StaticSkillProvider { catalog }), + )), + |config: &Config| SkillsExtensionConfig { + include_instructions: config.include_skill_instructions, + bundled_skills_enabled: false, + orchestrator_skills_enabled: false, + shadow_selection_enabled: false, + }, + ); + let mut builder = test_codex() + .with_extensions(Arc::new(extensions.build())) + .with_model_info_override("gpt-5.5", |model_info| { + model_info.context_window = Some(100_000); + model_info.max_context_window = None; + }) + .with_config(|config| { + config.include_skill_instructions = true; + }); + let test = builder.build_with_auto_env(&server).await?; + + test.submit_turn("Inspect the available skills.").await?; + let developer_texts = response.single_request().message_input_texts("developer"); + let catalog_text = developer_texts + .iter() + .find(|text| text.contains("skill://test/")) + .unwrap_or_else(|| { + panic!( + "production request should include the extension skill catalog, got {developer_texts:?}" + ) + }); + let description_lengths = catalog_text + .lines() + .filter_map(|line| { + line.strip_prefix("- skill-") + .and_then(|line| line.split_once(": ")) + .and_then(|(_, line)| line.split_once(" (custom resource:")) + .map(|(description, _)| description.chars().count()) + }) + .collect::>(); + assert_eq!(10, description_lengths.len()); + assert!( + description_lengths + .iter() + .all(|length| *length > 0 && *length < 1_024) + ); + assert!(!catalog_text.contains("additional skills omitted")); + + Ok(()) +} diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index 7bcbf9a7cd40..04fa77d9bcef 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -14,6 +14,7 @@ const SKILL_METADATA_CONTEXT_WINDOW_PERCENT: usize = 2; const MAX_MAIN_PROMPT_BYTES: usize = 8_000; const MAX_CATALOG_SKILL_DESCRIPTION_CHARS: usize = 1_024; const TRUNCATED_SKILL_DESCRIPTION_SUFFIX: &str = "..."; +const APPROX_BYTES_PER_TOKEN: usize = 4; pub(crate) const MAX_SKILL_NAME_BYTES: usize = 256; pub(crate) const MAX_SKILL_PATH_BYTES: usize = 1_024; @@ -73,6 +74,208 @@ fn metadata_line_cost(budget: SkillMetadataBudget, line: &str) -> usize { } } +impl SkillMetadataBudget { + fn limit(self) -> usize { + match self { + Self::Tokens(limit) | Self::Characters(limit) => limit, + } + } + + fn cost_from_counts(self, chars: usize, bytes: usize) -> usize { + match self { + Self::Tokens(_) => { + bytes.saturating_add(APPROX_BYTES_PER_TOKEN.saturating_sub(1)) + / APPROX_BYTES_PER_TOKEN + } + Self::Characters(_) => chars, + } + } +} + +struct SkillLine<'a> { + name: &'a str, + description: Cow<'a, str>, + locator: String, + locator_kind: &'static str, +} + +impl<'a> SkillLine<'a> { + fn new(entry: &'a SkillCatalogEntry, policy: SkillCatalogRenderPolicy) -> Self { + let description = policy.description(entry); + Self { + name: entry.name.as_str(), + description: truncate_catalog_skill_description(description), + locator: entry.rendered_path().to_string(), + locator_kind: match &entry.authority.kind { + SkillSourceKind::Host => "file", + SkillSourceKind::Executor => "environment resource", + SkillSourceKind::Orchestrator => "orchestrator resource", + SkillSourceKind::Custom(_) => "custom resource", + }, + } + } + + fn full_cost(&self, budget: SkillMetadataBudget) -> usize { + metadata_line_cost(budget, &self.render_full()) + } + + fn minimum_cost(&self, budget: SkillMetadataBudget) -> usize { + metadata_line_cost(budget, &self.render_minimum()) + } + + fn render_full(&self) -> String { + self.render_with_description(self.description.as_ref()) + } + + fn render_minimum(&self) -> String { + self.render_with_description("") + } + + fn render_with_description_chars(&self, description_chars: usize) -> String { + let end = self + .description + .char_indices() + .nth(description_chars) + .map_or(self.description.len(), |(index, _)| index); + self.render_with_description(&self.description[..end]) + } + + fn render_with_description(&self, description: &str) -> String { + let name = self.name; + let locator = self.locator.as_str(); + let locator_kind = self.locator_kind; + if description.is_empty() { + format!("- {name}: ({locator_kind}: {locator})") + } else { + format!("- {name}: {description} ({locator_kind}: {locator})") + } + } +} + +struct DescriptionBudgetLine<'a> { + line: &'a SkillLine<'a>, + description_char_count: usize, + extra_costs: Vec, +} + +impl<'a> DescriptionBudgetLine<'a> { + fn new(line: &'a SkillLine<'a>, budget: SkillMetadataBudget) -> Self { + let minimum_line = line.render_minimum(); + let minimum_chars = minimum_line.chars().count().saturating_add(1); + let minimum_bytes = minimum_line.len().saturating_add(1); + let minimum_cost = budget.cost_from_counts(minimum_chars, minimum_bytes); + + let description_char_count = line.description.chars().count(); + let mut extra_costs = Vec::with_capacity(description_char_count.saturating_add(1)); + extra_costs.push(0); + + let mut prefix_chars = 0usize; + let mut prefix_bytes = 0usize; + for ch in line.description.chars() { + prefix_chars = prefix_chars.saturating_add(1); + prefix_bytes = prefix_bytes.saturating_add(ch.len_utf8()); + let rendered_chars = minimum_chars.saturating_add(prefix_chars).saturating_add(1); + let rendered_bytes = minimum_bytes.saturating_add(prefix_bytes).saturating_add(1); + let cost = budget + .cost_from_counts(rendered_chars, rendered_bytes) + .saturating_sub(minimum_cost); + extra_costs.push(cost); + } + + Self { + line, + description_char_count, + extra_costs, + } + } +} + +fn render_skill_lines( + skill_lines: Vec>, + budget: SkillMetadataBudget, +) -> (Vec, usize) { + let full_cost = skill_lines.iter().fold(0usize, |used, line| { + used.saturating_add(line.full_cost(budget)) + }); + if full_cost <= budget.limit() { + return (skill_lines.iter().map(SkillLine::render_full).collect(), 0); + } + + let minimum_cost = skill_lines.iter().fold(0usize, |used, line| { + used.saturating_add(line.minimum_cost(budget)) + }); + if minimum_cost <= budget.limit() { + return ( + render_lines_with_description_budget( + budget, + &skill_lines, + budget.limit().saturating_sub(minimum_cost), + ), + 0, + ); + } + + let mut included = Vec::new(); + let mut used = 0usize; + let mut omitted = 0usize; + for line in skill_lines { + let rendered = line.render_full(); + let next_used = used.saturating_add(metadata_line_cost(budget, &rendered)); + if next_used <= budget.limit() { + used = next_used; + included.push(rendered); + } else { + omitted = omitted.saturating_add(1); + } + } + (included, omitted) +} + +fn render_lines_with_description_budget( + budget: SkillMetadataBudget, + skill_lines: &[SkillLine<'_>], + limit: usize, +) -> Vec { + let budget_lines = skill_lines + .iter() + .map(|line| DescriptionBudgetLine::new(line, budget)) + .collect::>(); + let mut char_allocations = vec![0usize; budget_lines.len()]; + let mut current_extra_costs = vec![0usize; budget_lines.len()]; + let mut remaining = limit; + + // Distribute description space round-robin so no skill monopolizes the + // remaining budget. + loop { + let mut changed = false; + for (index, line) in budget_lines.iter().enumerate() { + if char_allocations[index] >= line.description_char_count { + continue; + } + + let next_chars = char_allocations[index].saturating_add(1); + let next_cost = line.extra_costs[next_chars]; + let delta = next_cost.saturating_sub(current_extra_costs[index]); + if delta <= remaining { + char_allocations[index] = next_chars; + current_extra_costs[index] = next_cost; + remaining = remaining.saturating_sub(delta); + changed = true; + } + } + + if !changed { + break; + } + } + + budget_lines + .iter() + .zip(char_allocations) + .map(|(line, description_chars)| line.line.render_with_description_chars(description_chars)) + .collect() +} + #[tracing::instrument( level = "trace", skip_all, @@ -84,34 +287,21 @@ pub(crate) fn available_skills_fragment( policy: SkillCatalogRenderPolicy, budget: SkillMetadataBudget, ) -> Option { - let budget_limit = match budget { - SkillMetadataBudget::Tokens(limit) | SkillMetadataBudget::Characters(limit) => limit, - }; - let mut total_cost = 0usize; - let mut omitted = 0usize; - let mut skill_lines = Vec::new(); - - for entry in catalog + let skill_lines = catalog .entries .iter() .filter(|entry| entry.enabled && entry.prompt_visible) - { - let description = policy.description(entry); - let description = truncate_catalog_skill_description(description); - let line = render_skill_line(entry, description.as_ref()); - let next_cost = total_cost.saturating_add(metadata_line_cost(budget, &line)); - if next_cost > budget_limit { - omitted = omitted.saturating_add(1); - continue; - } - total_cost = next_cost; - skill_lines.push(line); - } + .map(|entry| SkillLine::new(entry, policy)) + .collect(); + let (mut skill_lines, mut omitted) = render_skill_lines(skill_lines, budget); + let mut total_cost = skill_lines.iter().fold(0usize, |used, line| { + used.saturating_add(metadata_line_cost(budget, line)) + }); if omitted > 0 { loop { let marker = omission_marker(omitted); - if total_cost.saturating_add(metadata_line_cost(budget, &marker)) <= budget_limit { + if total_cost.saturating_add(metadata_line_cost(budget, &marker)) <= budget.limit() { skill_lines.push(marker); break; } @@ -154,22 +344,6 @@ pub(crate) fn truncate_catalog_skill_description(description: &str) -> Cow<'_, s Cow::Owned(truncated) } -fn render_skill_line(entry: &SkillCatalogEntry, description: &str) -> String { - let locator_kind = match &entry.authority.kind { - SkillSourceKind::Host => "file", - SkillSourceKind::Executor => "environment resource", - SkillSourceKind::Orchestrator => "orchestrator resource", - SkillSourceKind::Custom(_) => "custom resource", - }; - let name = entry.name.as_str(); - let path = entry.rendered_path(); - if description.is_empty() { - format!("- {name}: ({locator_kind}: {path})") - } else { - format!("- {name}: {description} ({locator_kind}: {path})") - } -} - pub(crate) fn truncate_main_prompt_contents(contents: &str) -> (String, bool) { truncate_utf8_to_bytes(contents, MAX_MAIN_PROMPT_BYTES) } diff --git a/codex-rs/ext/skills/src/render_tests.rs b/codex-rs/ext/skills/src/render_tests.rs index 50e2750b11b8..ee94e19950b3 100644 --- a/codex-rs/ext/skills/src/render_tests.rs +++ b/codex-rs/ext/skills/src/render_tests.rs @@ -149,13 +149,15 @@ fn character_fallback_counts_multibyte_metadata_by_characters() { } #[test] -fn catalog_emits_omission_marker_when_every_skill_exceeds_budget() { +fn catalog_emits_omission_marker_when_every_minimum_skill_line_exceeds_budget() { + let oversized = entry( + "oversized", + &"x".repeat(MAX_CATALOG_SKILL_DESCRIPTION_CHARS), + /*short_description*/ None, + ) + .with_display_path(format!("skill://{}", "x".repeat(512))); let catalog = SkillCatalog { - entries: vec![entry( - "oversized", - &"x".repeat(MAX_CATALOG_SKILL_DESCRIPTION_CHARS), - /*short_description*/ None, - )], + entries: vec![oversized], warnings: Vec::new(), }; diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index ec34d8050ecb..b121b3b9b6d3 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -398,6 +398,88 @@ async fn default_context_truncates_catalog_descriptions() -> TestResult { Ok(()) } +#[tokio::test] +async fn moderate_budget_pressure_keeps_every_catalog_entry() -> TestResult { + let description = "x".repeat(1_025); + let entries = (0..10) + .map(|index| { + let package_id = format!("orchestrator/skill-{index:02}"); + let mut entry = test_entry( + SkillSourceKind::Orchestrator, + "codex_apps", + &package_id, + &format!("skill://{package_id}/SKILL.md"), + ); + entry.description = description.clone(); + entry + }) + .collect(); + let providers = + SkillProviders::new().with_orchestrator_provider(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 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 fragments = registry.context_contributors()[0] + .contribute_thread_context(&session_store, &thread_store, &ExtensionData::new("step")) + .await; + assert_eq!(1, fragments.len()); + let rendered = fragments[0].text(); + let description_lengths = (0..10) + .map(|index| { + let package_id = format!("orchestrator/skill-{index:02}"); + let line_prefix = format!("- skill-{index:02}: "); + let line_suffix = format!(" (orchestrator resource: skill://{package_id}/SKILL.md)"); + rendered + .lines() + .find_map(|line| { + line.strip_prefix(&line_prefix) + .and_then(|line| line.strip_suffix(&line_suffix)) + }) + .unwrap_or_else(|| panic!("rendered catalog should include skill-{index:02}")) + .chars() + .count() + }) + .collect::>(); + let shortest_description = *description_lengths + .iter() + .min() + .expect("catalog should include descriptions"); + let longest_description = *description_lengths + .iter() + .max() + .expect("catalog should include descriptions"); + assert!(shortest_description > 0); + assert!(longest_description < 1_024); + assert!(longest_description.abs_diff(shortest_description) <= 1); + assert!(!rendered.contains("additional skills omitted from this bounded skills list")); + assert!(!rendered.contains(&"x".repeat(1_021))); + + Ok(()) +} + #[tokio::test] async fn skills_list_truncates_catalog_descriptions_in_tool_output() -> TestResult { let description = "x".repeat(1_025);