From 2c92af09cfc95d4190ffe8f86298ba65eebcf7d4 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Thu, 23 Jul 2026 19:38:53 +0000 Subject: [PATCH] Warn when skill catalogs exceed their context budget (#34997) ## Why Skill catalog rendering can shorten descriptions or omit enabled skills to fit the model context budget. Surface these reductions so users know when the model-visible catalog has lost detail. ## What changed - Emit a warning with the omitted skill count whenever entries do not fit. - Warn when description shortening averages more than 100 characters per skill. - Deduplicate executor catalog warnings across repeated world-state builds in a turn. ## Testing - Cover the description-shortening threshold and omission warning text. - Verify warnings through extension and production-turn catalogs, including per-turn deduplication. GitOrigin-RevId: bd7fc3482e5dfd8c79072f772a2f54aa502478d9 --- codex-rs/core/tests/suite/skills_extension.rs | 37 +++++++++- codex-rs/ext/skills/src/extension.rs | 74 ++++++++++++++++--- codex-rs/ext/skills/src/render.rs | 49 ++++++++++-- codex-rs/ext/skills/src/render_tests.rs | 65 ++++++++++++++++ codex-rs/ext/skills/src/state.rs | 13 ++++ codex-rs/ext/skills/src/world_state.rs | 38 +++------- codex-rs/ext/skills/tests/skills_extension.rs | 56 +++++++++++++- 7 files changed, 283 insertions(+), 49 deletions(-) diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index c4cd98e66bcc..2d37b2579a56 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -2,7 +2,10 @@ use std::sync::Arc; use anyhow::Result; use codex_core::config::Config; +use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionRegistryBuilder; +use codex_protocol::protocol::Event; +use codex_protocol::protocol::EventMsg; use codex_skills_extension::SkillProvider; use codex_skills_extension::SkillProviderSource; use codex_skills_extension::SkillProviders; @@ -34,6 +37,14 @@ struct StaticSkillProvider { catalog: SkillCatalog, } +struct ChannelEventSink(std::sync::mpsc::Sender); + +impl ExtensionEventSink for ChannelEventSink { + fn emit(&self, event: Event) { + let _ = self.0.send(event); + } +} + impl SkillProvider for StaticSkillProvider { fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { // Keep thread context empty so the catalog is exercised through the @@ -88,7 +99,10 @@ async fn production_turn_scales_extension_catalog_from_resolved_model_window() - .collect(), warnings: Vec::new(), }; - let mut extensions = ExtensionRegistryBuilder::::new(); + let (event_tx, event_rx) = std::sync::mpsc::channel(); + let mut extensions = ExtensionRegistryBuilder::::with_event_sink(Arc::new( + ChannelEventSink(event_tx), + )); install_with_providers( &mut extensions, SkillProviders::new().with_provider(SkillProviderSource::new( @@ -139,12 +153,22 @@ async fn production_turn_scales_extension_catalog_from_resolved_model_window() - .iter() .filter(|line| line.starts_with("- skill-")) .count(); + let EventMsg::Warning(warning) = event_rx.try_recv()?.msg else { + panic!("expected catalog budget warning"); + }; + let omitted_count = 400 - included_count; assert!(catalog_text.contains("additional skills omitted")); assert!(!catalog_text.contains( "A description long enough to keep the catalog under sustained budget pressure." )); assert!(metadata_cost <= expected_budget); + assert_eq!( + warning.message, + format!( + "Exceeded skills context budget. All skill descriptions were removed and {omitted_count} additional skills were not included in the model-visible skills list." + ) + ); included_counts.push(included_count); } @@ -180,7 +204,9 @@ async fn production_turn_fairly_shortens_extension_catalog_descriptions() -> Res .collect(), warnings: Vec::new(), }; - let mut extensions = ExtensionRegistryBuilder::::new(); + let (event_tx, event_rx) = std::sync::mpsc::channel(); + let mut extensions = + ExtensionRegistryBuilder::::with_event_sink(Arc::new(ChannelEventSink(event_tx))); install_with_providers( &mut extensions, SkillProviders::new().with_provider(SkillProviderSource::new( @@ -232,6 +258,13 @@ async fn production_turn_fairly_shortens_extension_catalog_descriptions() -> Res .all(|length| *length > 0 && *length < 1_024) ); assert!(!catalog_text.contains("additional skills omitted")); + let EventMsg::Warning(warning) = event_rx.try_recv()?.msg else { + panic!("expected catalog budget warning"); + }; + assert_eq!( + warning.message, + "Skill descriptions were shortened to fit the skills context budget. Codex can still see every skill, but some descriptions are shorter. Disable unused skills or plugins to leave more room for the rest." + ); Ok(()) } diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index ad75177d5f1c..c4700fc66d58 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -36,6 +36,7 @@ use crate::catalog::SkillCatalog; use crate::catalog::SkillCatalogEntry; use crate::catalog::SkillReadResult; use crate::catalog::SkillSourceKind; +use crate::fragments::AvailableSkillsInstructions; use crate::fragments::SkillInstructions; use crate::provider::HostSkillProvider; use crate::provider::SkillListQuery; @@ -43,13 +44,15 @@ use crate::provider::SkillReadRequest; use crate::render::MAX_SKILL_NAME_BYTES; use crate::render::MAX_SKILL_PATH_BYTES; use crate::render::SkillCatalogRenderPolicy; -use crate::render::available_skills_fragment; +use crate::render::SkillMetadataBudget; use crate::render::capped_skill_metadata_budget; +use crate::render::render_available_skills; use crate::render::truncate_main_prompt_contents; use crate::render::truncate_utf8_to_bytes; use crate::selection::collect_explicit_skill_mentions; use crate::shadow_selection_experiment::ShadowSelectionExperiment; use crate::sources::SkillProviders; +use crate::state::EmittedCatalogBudgetWarnings; use crate::state::ExecutorSkillsStepState; use crate::state::SkillsSessionState; use crate::state::SkillsThreadState; @@ -65,6 +68,29 @@ struct SkillsExtension { shadow_selection: Arc, } +#[derive(Default)] +struct RenderedCatalog { + fragment: Option, + warning_message: Option, +} + +fn render_catalog( + catalog: &SkillCatalog, + include_skills_usage_instructions: bool, + policy: SkillCatalogRenderPolicy, + budget: SkillMetadataBudget, +) -> RenderedCatalog { + let Some(rendered) = render_available_skills(catalog, policy, budget) else { + return RenderedCatalog::default(); + }; + let warning_message = rendered.report.warning_message(); + let fragment = rendered.into_fragment(include_skills_usage_instructions); + RenderedCatalog { + fragment, + warning_message, + } +} + impl ThreadLifecycleContributor for SkillsExtension where C: Send + Sync + 'static, @@ -150,15 +176,20 @@ where let include_usage = thread_store .get::() .is_some_and(|model_info| model_info.include_skills_usage_instructions); - available_skills_fragment( + let rendered = render_catalog( &catalog, include_usage, SkillCatalogRenderPolicy::ExtensionCompatible, capped_skill_metadata_budget(/*context_window*/ None), - ) - .map(|fragment| PromptFragment::developer_capability(fragment.render())) - .into_iter() - .collect() + ); + if let Some(message) = rendered.warning_message { + self.emit_warning(thread_store.level_id(), message); + } + rendered + .fragment + .map(|fragment| PromptFragment::developer_capability(fragment.render())) + .into_iter() + .collect() }) } @@ -200,11 +231,28 @@ where .as_deref() .and_then(ModelInfo::resolved_context_window); let metadata_budget = capped_skill_metadata_budget(context_window); + let rendered = if config.include_instructions { + render_catalog( + &catalog, + include_usage, + SkillCatalogRenderPolicy::ExtensionCompatible, + metadata_budget, + ) + } else { + RenderedCatalog::default() + }; + if let Some(message) = rendered.warning_message + && input + .turn_store + .get_or_init(EmittedCatalogBudgetWarnings::default) + .insert(&message) + { + self.emit_warning(input.turn_id, message); + } + let executor_body = rendered.fragment.map(|fragment| fragment.body()); let mut sections = vec![executor_skills_world_state_section( - &catalog, + executor_body, config.include_instructions, - include_usage, - metadata_budget, )]; if let Some(host_snapshot) = input.turn_store.get::() && self.providers.has_host_provider() @@ -352,12 +400,16 @@ where .as_deref() .and_then(ModelInfo::resolved_context_window); let metadata_budget = capped_skill_metadata_budget(context_window); - if let Some(fragment) = available_skills_fragment( + let rendered = render_catalog( &turn_catalog, include_usage, SkillCatalogRenderPolicy::ExtensionCompatible, metadata_budget, - ) { + ); + if let Some(message) = rendered.warning_message { + self.emit_warning(&input.turn_id, message); + } + if let Some(fragment) = rendered.fragment { fragments.push(Box::new(fragment)); } } diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index 18e92a312292..b25fc0aca169 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -15,7 +15,11 @@ 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 SKILL_DESCRIPTION_TRUNCATION_WARNING_THRESHOLD_CHARS: usize = 100; const APPROX_BYTES_PER_TOKEN: usize = 4; +const SKILL_DESCRIPTION_TRUNCATED_WARNING: &str = "Skill descriptions were shortened to fit the skills context budget. Codex can still see every skill, but some descriptions are shorter. Disable unused skills or plugins to leave more room for the rest."; +const SKILL_DESCRIPTIONS_REMOVED_WARNING_PREFIX: &str = + "Exceeded skills context budget. All skill descriptions were removed and"; pub(crate) const MAX_SKILL_NAME_BYTES: usize = 256; pub(crate) const MAX_SKILL_PATH_BYTES: usize = 1_024; @@ -87,6 +91,41 @@ pub(crate) struct SkillRenderReport { pub(crate) truncated_description_count: usize, } +impl SkillRenderReport { + pub(crate) fn warning_message(&self) -> Option { + if self.omitted_count > 0 { + let skill_word = if self.omitted_count == 1 { + "skill" + } else { + "skills" + }; + let verb = if self.omitted_count == 1 { + "was" + } else { + "were" + }; + return Some(format!( + "{} {} additional {} {} not included in the model-visible skills list.", + SKILL_DESCRIPTIONS_REMOVED_WARNING_PREFIX, self.omitted_count, skill_word, verb + )); + } + + (self.average_truncated_description_chars() + > SKILL_DESCRIPTION_TRUNCATION_WARNING_THRESHOLD_CHARS) + .then(|| SKILL_DESCRIPTION_TRUNCATED_WARNING.to_string()) + } + + fn average_truncated_description_chars(&self) -> usize { + if self.total_count == 0 || self.truncated_description_chars == 0 { + return 0; + } + + self.truncated_description_chars + .saturating_add(self.total_count.saturating_sub(1)) + / self.total_count + } +} + pub(crate) fn capped_skill_metadata_budget(context_window: Option) -> SkillMetadataBudget { context_window .and_then(|window| usize::try_from(window).ok()) @@ -385,13 +424,6 @@ fn sum_description_truncation(rendered: &[RenderedSkillLine]) -> (usize, usize) pub(crate) struct AvailableSkillsRender { skill_lines: Vec, - #[cfg_attr( - not(test), - expect( - dead_code, - reason = "consumed by alias selection and render side effects in follow-ups" - ) - )] pub(crate) report: SkillRenderReport, } @@ -477,7 +509,8 @@ pub(crate) fn render_available_skills( }) } -pub(crate) fn available_skills_fragment( +#[cfg(test)] +fn available_skills_fragment( catalog: &SkillCatalog, include_skills_usage_instructions: bool, policy: SkillCatalogRenderPolicy, diff --git a/codex-rs/ext/skills/src/render_tests.rs b/codex-rs/ext/skills/src/render_tests.rs index 8c139834b52d..9dffad3456a1 100644 --- a/codex-rs/ext/skills/src/render_tests.rs +++ b/codex-rs/ext/skills/src/render_tests.rs @@ -310,6 +310,13 @@ fn catalog_emits_omission_marker_when_every_minimum_skill_line_exceeds_budget() truncated_description_chars: MAX_CATALOG_SKILL_DESCRIPTION_CHARS, truncated_description_count: 1, }; + assert_eq!( + expected_report.warning_message(), + Some( + "Exceeded skills context budget. All skill descriptions were removed and 1 additional skill was not included in the model-visible skills list." + .to_string() + ) + ); let core_render = render_available_skills( &catalog, SkillCatalogRenderPolicy::CoreCompatible, @@ -375,3 +382,61 @@ fn catalog_preserves_report_when_no_fragment_fits_budget() { .is_none() ); } + +#[test] +fn substantial_description_shortening_emits_warning() { + let catalog = SkillCatalog { + entries: vec![ + entry( + "long-skill", + &"a".repeat(250), + /*short_description*/ None, + ), + entry("empty-skill", "", /*short_description*/ None), + ], + warnings: Vec::new(), + }; + let skill_lines = catalog + .entries + .iter() + .map(|entry| SkillLine::new(entry, SkillCatalogRenderPolicy::ExtensionCompatible)) + .collect::>(); + let minimum_cost = skill_lines.iter().fold(0usize, |used, line| { + used.saturating_add(line.minimum_cost(SkillMetadataBudget::Characters(usize::MAX))) + }); + let render = render_available_skills( + &catalog, + SkillCatalogRenderPolicy::ExtensionCompatible, + SkillMetadataBudget::Characters(minimum_cost + 49), + ) + .expect("catalog should render"); + + assert_eq!( + render.report.warning_message(), + Some( + "Skill descriptions were shortened to fit the skills context budget. Codex can still see every skill, but some descriptions are shorter. Disable unused skills or plugins to leave more room for the rest." + .to_string() + ) + ); +} + +#[test] +fn substantial_description_shortening_warning_starts_above_threshold() { + let report_at_threshold = SkillRenderReport { + total_count: 2, + included_count: 2, + omitted_count: 0, + truncated_description_chars: 200, + truncated_description_count: 2, + }; + assert_eq!(report_at_threshold.warning_message(), None); + + let report_above_threshold = SkillRenderReport { + truncated_description_chars: 201, + ..report_at_threshold + }; + assert_eq!( + report_above_threshold.warning_message(), + Some(SKILL_DESCRIPTION_TRUNCATED_WARNING.to_string()) + ); +} diff --git a/codex-rs/ext/skills/src/state.rs b/codex-rs/ext/skills/src/state.rs index 5c026321d9ce..8e39ad5395fb 100644 --- a/codex-rs/ext/skills/src/state.rs +++ b/codex-rs/ext/skills/src/state.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::collections::HashSet; use std::future::Future; use std::sync::Arc; use std::sync::Mutex; @@ -337,6 +338,18 @@ impl OrchestratorResourceCache { } } +#[derive(Default)] +pub(crate) struct EmittedCatalogBudgetWarnings(Mutex>); + +impl EmittedCatalogBudgetWarnings { + pub(crate) fn insert(&self, warning: &str) -> bool { + self.0 + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .insert(warning.to_string()) + } +} + #[derive(Clone, Debug, Default, PartialEq, Eq)] pub(crate) struct SkillsTurnState { pub(crate) catalog: SkillCatalog, diff --git a/codex-rs/ext/skills/src/world_state.rs b/codex-rs/ext/skills/src/world_state.rs index e4fb09ec0b40..274f52719810 100644 --- a/codex-rs/ext/skills/src/world_state.rs +++ b/codex-rs/ext/skills/src/world_state.rs @@ -9,11 +9,8 @@ use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG; use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; use serde_json::json; -use crate::catalog::SkillCatalog; use crate::fragments::AvailableSkillsInstructions; -use crate::render::SkillCatalogRenderPolicy; use crate::render::SkillMetadataBudget; -use crate::render::available_skills_fragment; pub(crate) const SKILLS_WORLD_STATE_ID: &str = "skills"; pub(crate) const HOST_SKILLS_WORLD_STATE_ID: &str = "host_skills"; @@ -25,22 +22,9 @@ const NO_HOST_SKILLS_BODY: &str = const HIDDEN_HOST_SKILLS_BODY: &str = "\n## Host skills update\nHost skills are not listed automatically. Explicit skill mentions can still be resolved when available.\n"; pub(crate) fn executor_skills_world_state_section( - catalog: &SkillCatalog, + body: Option, include_instructions: bool, - include_skills_usage_instructions: bool, - metadata_budget: SkillMetadataBudget, ) -> WorldStateSectionContribution { - let body = if include_instructions { - available_skills_fragment( - catalog, - include_skills_usage_instructions, - SkillCatalogRenderPolicy::ExtensionCompatible, - metadata_budget, - ) - .map(|fragment| fragment.body()) - } else { - None - }; let snapshot = json!({ "body": body, "includeInstructions": include_instructions, @@ -63,16 +47,18 @@ pub(crate) fn executor_skills_world_state_section( } let body = match body.as_deref() { - Some(body) => body, - None if previous_is_absent => return None, - None if !include_instructions => HIDDEN_EXECUTOR_SKILLS_BODY, - None => NO_EXECUTOR_SKILLS_BODY, + Some(body) => Some(body), + None if previous_is_absent => None, + None if !include_instructions => Some(HIDDEN_EXECUTOR_SKILLS_BODY), + None => Some(NO_EXECUTOR_SKILLS_BODY), }; - Some(RenderedWorldStateFragment::new( - "developer", - (SKILLS_INSTRUCTIONS_OPEN_TAG, SKILLS_INSTRUCTIONS_CLOSE_TAG), - body, - )) + body.map(|body| { + RenderedWorldStateFragment::new( + "developer", + (SKILLS_INSTRUCTIONS_OPEN_TAG, SKILLS_INSTRUCTIONS_CLOSE_TAG), + body, + ) + }) }) .with_legacy_matcher(|role, text| { role == "developer" diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index 05f7c4df875e..189e4bf046dc 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -503,7 +503,9 @@ async fn extreme_budget_pressure_removes_descriptions_before_omitting_entries() list_calls: None, fail_first_list: false, })); - let mut builder = ExtensionRegistryBuilder::new(); + let (event_tx, event_rx) = std::sync::mpsc::channel(); + let mut builder = + ExtensionRegistryBuilder::with_event_sink(Arc::new(ChannelEventSink(event_tx))); install_with_providers(&mut builder, providers, skills_extension_config); let registry = builder.build(); let session_store = ExtensionData::new("session"); @@ -537,6 +539,19 @@ async fn extreme_budget_pressure_removes_descriptions_before_omitting_entries() assert!(!rendered.contains("- skill-199:")); assert!(!rendered.contains("description-")); assert!(rendered.contains("additional skills omitted from this bounded skills list")); + let omitted_count = 200 - included_count; + let event = event_rx.try_recv()?; + assert_eq!("thread", event.id); + let EventMsg::Warning(warning) = event.msg else { + panic!("expected catalog budget warning"); + }; + assert_eq!( + warning.message, + format!( + "Exceeded skills context budget. All skill descriptions were removed and {omitted_count} additional skills were not included in the model-visible skills list." + ) + ); + assert!(event_rx.try_recv().is_err()); Ok(()) } @@ -833,7 +848,9 @@ async fn model_context_window_scales_executor_catalog_but_not_thread_catalog() - list_calls: None, fail_first_list: false, })); - let mut builder = ExtensionRegistryBuilder::new(); + let (event_tx, event_rx) = std::sync::mpsc::channel(); + let mut builder = + ExtensionRegistryBuilder::with_event_sink(Arc::new(ChannelEventSink(event_tx))); install_with_providers(&mut builder, providers, skills_extension_config); let registry = builder.build(); let session_store = ExtensionData::new("session"); @@ -886,11 +903,46 @@ async fn model_context_window_scales_executor_catalog_but_not_thread_catalog() - turn_store: &turn_store, }) .await; + // Core rebuilds world state before each sampling step. + let _repeated_sections = registry.context_contributors()[0] + .contribute_world_state(WorldStateContributionInput { + thread_id: codex_protocol::ThreadId::new(), + turn_id: "turn-1", + environments: &[], + ready_selected_capability_roots: &selected_roots, + executor_capability_discovery: None, + session_store: &session_store, + thread_store: &thread_store, + turn_store: &turn_store, + }) + .await; + let event = event_rx.try_recv()?; + assert_eq!("turn-1", event.id); + let EventMsg::Warning(warning) = event.msg else { + panic!("expected catalog budget warning"); + }; + assert!( + warning + .message + .starts_with("Exceeded skills context budget.") + ); + assert!( + warning + .message + .ends_with("additional skills were not included in the model-visible skills list.") + ); + let snapshot = sections[0].snapshot().clone(); let fragment = sections[0] .render_diff(PreviousWorldStateSection::Absent) .ok_or("bounded executor catalog should render")?; assert!(fragment.body().contains("additional skills omitted")); assert!(!fragment.body().contains("skill-39")); + assert!( + sections[0] + .render_diff(PreviousWorldStateSection::Known(&snapshot)) + .is_none() + ); + assert!(event_rx.try_recv().is_err()); Ok(()) }