From b77791c228b729a6ea48fae8e963f5dad0cc8ea5 Mon Sep 17 00:00:00 2001 From: xl-openai Date: Wed, 22 Apr 2026 12:33:29 -0700 Subject: [PATCH] feat: Fairly trim skill descriptions within context budget (#18925) Preserve skill name/path entries whenever possible and trim descriptions first, using round-robin character allocation so short descriptions do not waste budget. --- .../app-server/tests/suite/v2/turn_start.rs | 2 +- codex-rs/core-skills/src/render.rs | 544 ++++++++++++++++-- codex-rs/core/src/session/mod.rs | 8 +- codex-rs/core/src/session/tests.rs | 71 ++- codex-rs/otel/src/metrics/names.rs | 2 + .../tui/src/chatwidget/tests/app_server.rs | 10 +- 6 files changed, 587 insertions(+), 50 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index b2b1ac1d8..93e2e4c80 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -324,7 +324,7 @@ async fn turn_start_emits_thread_scoped_warning_notification_for_trimmed_skills( assert_eq!(warning.thread_id.as_deref(), Some(thread.id.as_str())); assert_eq!( warning.message, - "Some enabled skills were not included in the model-visible skills list for this session. Mention a skill by name or path if you need it." + "Warning: Exceeded skills context budget of 2%. All skill descriptions were removed and 7 additional skills were not included in the model-visible skills list." ); timeout( diff --git a/codex-rs/core-skills/src/render.rs b/codex-rs/core-skills/src/render.rs index 6bcd5e4e0..add2fcaf5 100644 --- a/codex-rs/core-skills/src/render.rs +++ b/codex-rs/core-skills/src/render.rs @@ -1,5 +1,6 @@ use crate::model::SkillMetadata; use codex_otel::SessionTelemetry; +use codex_otel::THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC; use codex_otel::THREAD_SKILLS_ENABLED_TOTAL_METRIC; use codex_otel::THREAD_SKILLS_KEPT_TOTAL_METRIC; use codex_otel::THREAD_SKILLS_TRUNCATED_METRIC; @@ -8,6 +9,11 @@ use codex_utils_output_truncation::approx_token_count; const DEFAULT_SKILL_METADATA_CHAR_BUDGET: usize = 8_000; const SKILL_METADATA_CONTEXT_WINDOW_PERCENT: usize = 2; +const SKILL_DESCRIPTION_TRUNCATION_WARNING_THRESHOLD_CHARS: usize = 10; +const APPROX_BYTES_PER_TOKEN: usize = 4; +pub const SKILL_DESCRIPTION_TRUNCATED_WARNING_PREFIX: &str = "Warning: Exceeded skills context budget. Loaded skill descriptions were truncated by an average of"; +pub const SKILL_DESCRIPTIONS_REMOVED_WARNING_PREFIX: &str = + "Warning: Exceeded skills context budget. All skill descriptions were removed and"; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum SkillMetadataBudget { @@ -28,6 +34,17 @@ impl SkillMetadataBudget { Self::Characters(_) => text.chars().count(), } } + + fn cost_from_counts(self, chars: usize, bytes: usize) -> usize { + match self { + Self::Tokens(_) => approx_token_count_from_bytes(bytes), + Self::Characters(_) => chars, + } + } +} + +fn approx_token_count_from_bytes(bytes: usize) -> usize { + bytes.saturating_add(APPROX_BYTES_PER_TOKEN.saturating_sub(1)) / APPROX_BYTES_PER_TOKEN } #[derive(Debug, Clone, PartialEq, Eq)] @@ -35,6 +52,8 @@ pub struct SkillRenderReport { pub total_count: usize, pub included_count: usize, pub omitted_count: usize, + pub truncated_description_chars: usize, + pub truncated_description_count: usize, } #[derive(Clone, Copy)] @@ -49,7 +68,7 @@ pub enum SkillRenderSideEffects<'a> { pub struct AvailableSkills { pub skill_lines: Vec, pub report: SkillRenderReport, - pub emit_warning: bool, + pub warning_message: Option, } pub fn default_skill_metadata_budget(context_window: Option) -> SkillMetadataBudget { @@ -75,37 +94,91 @@ pub fn build_available_skills( side_effects: SkillRenderSideEffects<'_>, ) -> Option { if skills.is_empty() { - let _ = record_skill_render_side_effects( + record_skill_render_side_effects( side_effects, /*total_count*/ 0, /*included_count*/ 0, - /*truncated*/ false, + /*omitted_count*/ 0, + /*truncated_description_chars*/ 0, ); return None; } let (skill_lines, report) = render_skill_lines(skills, budget); - let emit_warning = record_skill_render_side_effects( + let warning_message = if report.omitted_count > 0 { + let skill_word = if report.omitted_count == 1 { + "skill" + } else { + "skills" + }; + let verb = if report.omitted_count == 1 { + "was" + } else { + "were" + }; + Some(format!( + "{} {} additional {} {} not included in the model-visible skills list.", + budget_warning_prefix(budget, SKILL_DESCRIPTIONS_REMOVED_WARNING_PREFIX), + report.omitted_count, + skill_word, + verb + )) + } else if report.average_truncated_description_chars() + > SKILL_DESCRIPTION_TRUNCATION_WARNING_THRESHOLD_CHARS + { + Some(format!( + "{} {} characters per skill.", + budget_warning_prefix(budget, SKILL_DESCRIPTION_TRUNCATED_WARNING_PREFIX), + report.average_truncated_description_chars() + )) + } else { + None + }; + record_skill_render_side_effects( side_effects, report.total_count, report.included_count, - report.omitted_count > 0, + report.omitted_count, + report.truncated_description_chars, ); + if report.omitted_count > 0 || report.truncated_description_chars > 0 { + tracing::info!( + budget_limit = budget.limit(), + total_skills = report.total_count, + included_skills = report.included_count, + omitted_skills = report.omitted_count, + truncated_description_chars_per_skill = report.average_truncated_description_chars(), + truncated_skill_descriptions = report.truncated_description_count, + "truncated skill metadata to fit skills context budget" + ); + } Some(AvailableSkills { skill_lines, report, - emit_warning, + warning_message, }) } +fn budget_warning_prefix(budget: SkillMetadataBudget, prefix: &str) -> String { + match budget { + SkillMetadataBudget::Tokens(_) => prefix.replacen( + "Exceeded skills context budget.", + "Exceeded skills context budget of 2%.", + 1, + ), + SkillMetadataBudget::Characters(_) => prefix.to_string(), + } +} + fn record_skill_render_side_effects( side_effects: SkillRenderSideEffects<'_>, total_count: usize, included_count: usize, - truncated: bool, -) -> bool { + omitted_count: usize, + truncated_description_chars: usize, +) { match side_effects { - SkillRenderSideEffects::None => false, + SkillRenderSideEffects::None => {} SkillRenderSideEffects::ThreadStart { session_telemetry } => { session_telemetry.histogram( THREAD_SKILLS_ENABLED_TOTAL_METRIC, @@ -119,10 +192,14 @@ fn record_skill_render_side_effects( ); session_telemetry.histogram( THREAD_SKILLS_TRUNCATED_METRIC, - if truncated { 1 } else { 0 }, + if omitted_count > 0 { 1 } else { 0 }, + &[], + ); + session_telemetry.histogram( + THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC, + i64::try_from(truncated_description_chars).unwrap_or(i64::MAX), &[], ); - truncated } } } @@ -132,32 +209,307 @@ fn render_skill_lines( budget: SkillMetadataBudget, ) -> (Vec, SkillRenderReport) { let ordered_skills = ordered_skills_for_budget(skills); + let skill_lines = ordered_skills + .into_iter() + .map(SkillLine::new) + .collect::>(); + let full_cost = skill_lines.iter().fold(0usize, |used, line| { + used.saturating_add(line.full_cost(budget)) + }); + if full_cost <= budget.limit() { + let included = skill_lines + .iter() + .map(SkillLine::render_full) + .collect::>(); + + return ( + included, + skill_render_report( + /*total_count*/ skills.len(), + /*included_count*/ skill_lines.len(), + /*omitted_count*/ 0, + /*truncated_description_chars*/ 0, + /*truncated_description_count*/ 0, + ), + ); + } + + let minimum_cost = skill_lines.iter().fold(0usize, |used, line| { + used.saturating_add(line.minimum_cost(budget)) + }); + if minimum_cost <= budget.limit() { + let rendered = render_lines_with_description_budget( + budget, + &skill_lines, + budget.limit().saturating_sub(minimum_cost), + ); + let (truncated_description_chars, truncated_description_count) = + sum_description_truncation(&rendered); + let included = rendered + .into_iter() + .map(|rendered| rendered.line) + .collect::>(); + + return ( + included, + skill_render_report( + /*total_count*/ skills.len(), + /*included_count*/ skill_lines.len(), + /*omitted_count*/ 0, + truncated_description_chars, + truncated_description_count, + ), + ); + } + + render_minimum_skill_lines_until_budget(budget, skill_lines, skills.len()) +} + +fn render_minimum_skill_lines_until_budget( + budget: SkillMetadataBudget, + skill_lines: Vec>, + total_count: usize, +) -> (Vec, SkillRenderReport) { let mut included = Vec::new(); let mut used = 0usize; let mut omitted_count = 0usize; - - for skill in ordered_skills { - let line = render_skill_line(skill); - let line_cost = budget.cost(&format!("{line}\n")); + let mut truncated_description_chars = 0usize; + let mut truncated_description_count = 0usize; + for line in skill_lines { + let line_cost = line.minimum_cost(budget); + let description_char_count = line.description_char_count(); if used.saturating_add(line_cost) <= budget.limit() { used = used.saturating_add(line_cost); - included.push(line); - continue; + included.push(line.render_minimum()); + } else { + omitted_count = omitted_count.saturating_add(1); } - omitted_count = omitted_count.saturating_add(1); + truncated_description_chars = + truncated_description_chars.saturating_add(description_char_count); + if description_char_count > 0 { + truncated_description_count = truncated_description_count.saturating_add(1); + } } - let report = SkillRenderReport { - total_count: skills.len(), - included_count: included.len(), + let report = skill_render_report( + total_count, + included.len(), omitted_count, - }; + truncated_description_chars, + truncated_description_count, + ); (included, report) } +fn skill_render_report( + total_count: usize, + included_count: usize, + omitted_count: usize, + truncated_description_chars: usize, + truncated_description_count: usize, +) -> SkillRenderReport { + SkillRenderReport { + total_count, + included_count, + omitted_count, + truncated_description_chars, + truncated_description_count, + } +} + +impl SkillRenderReport { + fn average_truncated_description_chars(&self) -> usize { + if self.truncated_description_count == 0 { + return 0; + } + + self.truncated_description_chars + .saturating_add(self.truncated_description_count.saturating_sub(1)) + / self.truncated_description_count + } +} + +struct SkillLine<'a> { + name: &'a str, + description: &'a str, + path: String, +} + +struct RenderedSkillLine { + line: String, + truncated_chars: usize, +} + +struct DescriptionBudgetLine<'a> { + line: &'a SkillLine<'a>, + description_char_count: usize, + extra_costs: Vec, +} + +fn sum_description_truncation(rendered: &[RenderedSkillLine]) -> (usize, usize) { + rendered + .iter() + .fold((0usize, 0usize), |(chars, count), line| { + if line.truncated_chars == 0 { + (chars, count) + } else { + ( + chars.saturating_add(line.truncated_chars), + count.saturating_add(1), + ) + } + }) +} + +impl<'a> SkillLine<'a> { + fn new(skill: &'a SkillMetadata) -> Self { + Self { + name: skill.name.as_str(), + description: skill.description.as_str(), + path: skill.path_to_skills_md.to_string_lossy().replace('\\', "/"), + } + } + + fn full_cost(&self, budget: SkillMetadataBudget) -> usize { + line_cost(budget, &self.render_full()) + } + + fn minimum_cost(&self, budget: SkillMetadataBudget) -> usize { + line_cost(budget, &self.render_minimum()) + } + + fn description_char_count(&self) -> usize { + self.description.chars().count() + } + + fn render_full(&self) -> String { + self.render_with_description(self.description) + } + + fn render_minimum(&self) -> String { + self.render_with_description("") + } + + fn rendered_description_prefix_len(&self, description_chars: usize) -> usize { + self.description + .char_indices() + .nth(description_chars) + .map_or(self.description.len(), |(idx, _)| idx) + } + + fn render_with_description_chars(&self, description_chars: usize) -> String { + if description_chars == 0 { + format!("- {}: (file: {})", self.name, self.path) + } else { + let end = self.rendered_description_prefix_len(description_chars); + let description = &self.description[..end]; + format!("- {}: {} (file: {})", self.name, description, self.path) + } + } + + fn render_with_description(&self, description: &str) -> String { + if description.is_empty() { + format!("- {}: (file: {})", self.name, self.path) + } else { + format!("- {}: {} (file: {})", self.name, description, self.path) + } + } +} + +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_char_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 line_cost(budget: SkillMetadataBudget, line: &str) -> usize { + budget.cost(&format!("{line}\n")) +} + +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 one character at a time across skills. + // Short descriptions naturally drop out, so their unused share can go to + // longer descriptions instead of being stranded in a fixed per-skill quota. + loop { + let mut changed = false; + for (index, line) in budget_lines.iter().enumerate() { + if char_allocations[index] >= line.description_char_count { + continue; + } + + let current_cost = current_extra_costs[index]; + let next_chars = char_allocations[index].saturating_add(1); + let next_cost = line.extra_costs[next_chars]; + let delta = next_cost.saturating_sub(current_cost); + 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)| { + let truncated_chars = line + .description_char_count + .saturating_sub(description_chars); + RenderedSkillLine { + line: line.line.render_with_description_chars(description_chars), + truncated_chars, + } + }) + .collect() +} + fn ordered_skills_for_budget(skills: &[SkillMetadata]) -> Vec<&SkillMetadata> { let mut ordered = skills.iter().collect::>(); ordered.sort_by(|a, b| { @@ -178,13 +530,6 @@ fn prompt_scope_rank(scope: SkillScope) -> u8 { } } -fn render_skill_line(skill: &SkillMetadata) -> String { - let path_str = skill.path_to_skills_md.to_string_lossy().replace('\\', "/"); - let name = skill.name.as_str(); - let description = skill.description.as_str(); - format!("- {name}: {description} (file: {path_str})") -} - #[cfg(test)] mod tests { use super::*; @@ -205,6 +550,20 @@ mod tests { } } + fn make_skill_with_description( + name: &str, + scope: SkillScope, + description: &str, + ) -> SkillMetadata { + let mut skill = make_skill(name, scope); + skill.description = description.to_string(); + skill + } + + fn expected_skill_line(skill: &SkillMetadata, description: &str) -> String { + SkillLine::new(skill).render_with_description(description) + } + #[test] fn default_budget_uses_two_percent_of_full_context_window() { assert_eq!( @@ -230,15 +589,117 @@ mod tests { } #[test] - fn budgeted_rendering_preserves_prompt_priority() { + fn budgeted_rendering_truncates_descriptions_equally_before_omitting_skills() { + let alpha = make_skill_with_description("alpha-skill", SkillScope::Repo, "abcdef"); + let beta = make_skill_with_description("beta-skill", SkillScope::Repo, "uvwxyz"); + let minimum_cost = SkillLine::new(&alpha) + .minimum_cost(SkillMetadataBudget::Characters(usize::MAX)) + + SkillLine::new(&beta).minimum_cost(SkillMetadataBudget::Characters(usize::MAX)); + let budget = SkillMetadataBudget::Characters(minimum_cost + 6); + + let rendered = build_available_skills( + &[beta.clone(), alpha.clone()], + budget, + SkillRenderSideEffects::None, + ) + .expect("skills should render"); + + assert_eq!(rendered.report.included_count, 2); + assert_eq!(rendered.report.omitted_count, 0); + assert_eq!(rendered.report.truncated_description_chars, 8); + assert_eq!(rendered.warning_message, None); + assert_eq!( + rendered.skill_lines, + vec![ + expected_skill_line(&alpha, "ab"), + expected_skill_line(&beta, "uv"), + ] + ); + } + + #[test] + fn budgeted_rendering_does_not_warn_when_average_description_truncation_is_within_threshold() { + let alpha = make_skill_with_description("alpha-skill", SkillScope::Repo, "abcdefghij"); + let beta = make_skill_with_description("beta-skill", SkillScope::Repo, "uvwxyzabcd"); + let minimum_cost = SkillLine::new(&alpha) + .minimum_cost(SkillMetadataBudget::Characters(usize::MAX)) + + SkillLine::new(&beta).minimum_cost(SkillMetadataBudget::Characters(usize::MAX)); + let budget = SkillMetadataBudget::Characters(minimum_cost + 6); + + let rendered = build_available_skills(&[alpha, beta], budget, SkillRenderSideEffects::None) + .expect("skills should render"); + + assert_eq!(rendered.report.included_count, 2); + assert_eq!(rendered.report.omitted_count, 0); + assert_eq!(rendered.report.truncated_description_chars, 16); + assert_eq!(rendered.report.truncated_description_count, 2); + assert_eq!(rendered.warning_message, None); + } + + #[test] + fn budgeted_rendering_warns_when_average_description_truncation_exceeds_threshold() { + let alpha = + make_skill_with_description("alpha-skill", SkillScope::Repo, "abcdefghijklmnop"); + let beta = make_skill_with_description("beta-skill", SkillScope::Repo, "uvwxyzabcdefghij"); + let minimum_cost = SkillLine::new(&alpha) + .minimum_cost(SkillMetadataBudget::Characters(usize::MAX)) + + SkillLine::new(&beta).minimum_cost(SkillMetadataBudget::Characters(usize::MAX)); + let budget = SkillMetadataBudget::Characters(minimum_cost + 6); + + let rendered = build_available_skills(&[alpha, beta], budget, SkillRenderSideEffects::None) + .expect("skills should render"); + + assert_eq!(rendered.report.included_count, 2); + assert_eq!(rendered.report.omitted_count, 0); + assert_eq!(rendered.report.truncated_description_chars, 28); + assert_eq!(rendered.report.truncated_description_count, 2); + assert_eq!( + rendered.warning_message, + Some( + "Warning: Exceeded skills context budget. Loaded skill descriptions were truncated by an average of 14 characters per skill." + .to_string() + ) + ); + } + + #[test] + fn budgeted_rendering_redistributes_unused_description_budget() { + let short = make_skill_with_description("short-skill", SkillScope::Repo, "x"); + let long = make_skill_with_description("long-skill", SkillScope::Repo, "abcdefghi"); + let minimum_cost = SkillLine::new(&short) + .minimum_cost(SkillMetadataBudget::Characters(usize::MAX)) + + SkillLine::new(&long).minimum_cost(SkillMetadataBudget::Characters(usize::MAX)); + let budget = SkillMetadataBudget::Characters(minimum_cost + 11); + + let rendered = build_available_skills( + &[short.clone(), long.clone()], + budget, + SkillRenderSideEffects::None, + ) + .expect("skills should render"); + + assert_eq!(rendered.report.included_count, 2); + assert_eq!(rendered.report.omitted_count, 0); + assert_eq!(rendered.warning_message, None); + assert_eq!( + rendered.skill_lines, + vec![ + expected_skill_line(&long, "abcdefgh"), + expected_skill_line(&short, "x"), + ] + ); + } + + #[test] + fn budgeted_rendering_preserves_prompt_priority_when_minimum_lines_exceed_budget() { let system = make_skill("system-skill", SkillScope::System); let user = make_skill("user-skill", SkillScope::User); let repo = make_skill("repo-skill", SkillScope::Repo); let admin = make_skill("admin-skill", SkillScope::Admin); let system_cost = SkillMetadataBudget::Characters(usize::MAX) - .cost(&format!("{}\n", render_skill_line(&system))); + .cost(&format!("{}\n", SkillLine::new(&system).render_minimum())); let admin_cost = SkillMetadataBudget::Characters(usize::MAX) - .cost(&format!("{}\n", render_skill_line(&admin))); + .cost(&format!("{}\n", SkillLine::new(&admin).render_minimum())); let budget = SkillMetadataBudget::Characters(system_cost + admin_cost); let rendered = build_available_skills( @@ -250,10 +711,17 @@ mod tests { assert_eq!(rendered.report.included_count, 2); assert_eq!(rendered.report.omitted_count, 2); - assert!(!rendered.emit_warning); + assert_eq!( + rendered.warning_message, + Some( + "Warning: Exceeded skills context budget. All skill descriptions were removed and 2 additional skills were not included in the model-visible skills list." + .to_string() + ) + ); let rendered_text = rendered.skill_lines.join("\n"); assert!(rendered_text.contains("- system-skill:")); assert!(rendered_text.contains("- admin-skill:")); + assert!(!rendered_text.contains("desc")); assert!(!rendered_text.contains("- repo-skill:")); assert!(!rendered_text.contains("- user-skill:")); } @@ -264,7 +732,7 @@ mod tests { oversized.description = "desc ".repeat(100); let repo = make_skill("repo-skill", SkillScope::Repo); let repo_cost = SkillMetadataBudget::Characters(usize::MAX) - .cost(&format!("{}\n", render_skill_line(&repo))); + .cost(&format!("{}\n", SkillLine::new(&repo).render_full())); let budget = SkillMetadataBudget::Characters(repo_cost); let rendered = @@ -273,7 +741,13 @@ mod tests { assert_eq!(rendered.report.included_count, 1); assert_eq!(rendered.report.omitted_count, 1); - assert!(!rendered.emit_warning); + assert_eq!( + rendered.warning_message, + Some( + "Warning: 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 rendered_text = rendered.skill_lines.join("\n"); assert!(!rendered_text.contains("- oversized-system-skill:")); assert!(rendered_text.contains("- repo-skill:")); diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 6a6ed86a8..5dc1eadd6 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -373,8 +373,6 @@ pub struct Codex { pub(crate) type SessionLoopTermination = Shared>; -pub(crate) const THREAD_START_SKILLS_TRIMMED_WARNING_MESSAGE: &str = "Some enabled skills were not included in the model-visible skills list for this session. Mention a skill by name or path if you need it."; - /// Wrapper returned by [`Codex::spawn`] containing the spawned [`Codex`] and /// the unique session id. pub struct CodexSpawnOk { @@ -2501,13 +2499,13 @@ impl Session { }, ); if let Some(available_skills) = available_skills { - let emit_warning = available_skills.emit_warning; + let warning_message = available_skills.warning_message.clone(); let skills_instructions = AvailableSkillsInstructions::from(available_skills); - if emit_warning { + if let Some(warning_message) = warning_message { self.send_event_raw(Event { id: String::new(), msg: EventMsg::Warning(WarningEvent { - message: THREAD_START_SKILLS_TRIMMED_WARNING_MESSAGE.to_string(), + message: warning_message, }), }) .await; diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index bf0623bcb..b50273713 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -68,6 +68,7 @@ use codex_execpolicy::Policy; use codex_network_proxy::NetworkProxyConfig; use codex_otel::MetricsClient; use codex_otel::MetricsConfig; +use codex_otel::THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC; use codex_otel::THREAD_SKILLS_ENABLED_TOTAL_METRIC; use codex_otel::THREAD_SKILLS_KEPT_TOTAL_METRIC; use codex_otel::THREAD_SKILLS_TRUNCATED_METRIC; @@ -4817,7 +4818,7 @@ async fn build_initial_context_trims_skill_metadata_from_context_window_budget() assert!( developer_texts .iter() - .all(|text| !text.contains(THREAD_START_SKILLS_TRIMMED_WARNING_MESSAGE)), + .all(|text| !text.contains("Exceeded skills context budget")), "expected skill budget warning to stay out of the initial context, got {developer_texts:?}" ); assert!( @@ -4849,7 +4850,13 @@ fn emit_thread_start_skill_metrics_records_enabled_kept_and_truncated_values() { ) .expect("skills should render"); - assert!(rendered.emit_warning); + assert_eq!( + rendered.warning_message, + Some( + "Warning: 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 snapshot = session_telemetry .snapshot_metrics() .expect("runtime metrics snapshot"); @@ -4859,6 +4866,62 @@ fn emit_thread_start_skill_metrics_records_enabled_kept_and_truncated_values() { ); assert_eq!(histogram_sum(&snapshot, THREAD_SKILLS_KEPT_TOTAL_METRIC), 0); assert_eq!(histogram_sum(&snapshot, THREAD_SKILLS_TRUNCATED_METRIC), 1); + assert_eq!( + histogram_sum(&snapshot, THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC), + 4 + ); +} + +#[test] +fn emit_thread_start_skill_metrics_records_description_truncated_chars_without_omitted_skills() { + let session_telemetry = test_session_telemetry_without_metadata(); + let alpha = SkillMetadata { + name: "alpha-skill".to_string(), + description: "abcdef".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + path_to_skills_md: test_path_buf("/tmp/alpha-skill/SKILL.md").abs(), + scope: SkillScope::Repo, + }; + let beta = SkillMetadata { + name: "beta-skill".to_string(), + description: "uvwxyz".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + path_to_skills_md: test_path_buf("/tmp/beta-skill/SKILL.md").abs(), + scope: SkillScope::Repo, + }; + let minimum_skill_line_cost = |skill: &SkillMetadata| { + let path = skill.path_to_skills_md.to_string_lossy().replace('\\', "/"); + format!("- {}: (file: {})\n", skill.name, path) + .chars() + .count() + }; + let minimum_budget = minimum_skill_line_cost(&alpha) + minimum_skill_line_cost(&beta); + + let rendered = build_available_skills( + &[alpha, beta], + SkillMetadataBudget::Characters(minimum_budget + 6), + SkillRenderSideEffects::ThreadStart { + session_telemetry: &session_telemetry, + }, + ) + .expect("skills should render"); + + assert_eq!(rendered.report.omitted_count, 0); + assert_eq!(rendered.report.truncated_description_chars, 8); + let snapshot = session_telemetry + .snapshot_metrics() + .expect("runtime metrics snapshot"); + assert_eq!(histogram_sum(&snapshot, THREAD_SKILLS_TRUNCATED_METRIC), 0); + assert_eq!( + histogram_sum(&snapshot, THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC), + 8 + ); } #[tokio::test] @@ -4899,7 +4962,7 @@ async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_buil assert!(matches!( warning_event.msg, EventMsg::Warning(WarningEvent { message }) - if message == THREAD_START_SKILLS_TRIMMED_WARNING_MESSAGE + if message == "Warning: Exceeded skills context budget of 2%. All skill descriptions were removed and 2 additional skills were not included in the model-visible skills list." )); let _ = session.build_initial_context(&turn_context).await; @@ -4910,7 +4973,7 @@ async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_buil assert!(matches!( warning_event.msg, EventMsg::Warning(WarningEvent { message }) - if message == THREAD_START_SKILLS_TRIMMED_WARNING_MESSAGE + if message == "Warning: Exceeded skills context budget of 2%. All skill descriptions were removed and 2 additional skills were not included in the model-visible skills list." )); } diff --git a/codex-rs/otel/src/metrics/names.rs b/codex-rs/otel/src/metrics/names.rs index 2d564bac2..198663cb6 100644 --- a/codex-rs/otel/src/metrics/names.rs +++ b/codex-rs/otel/src/metrics/names.rs @@ -40,4 +40,6 @@ pub const STARTUP_PREWARM_AGE_AT_FIRST_TURN_METRIC: &str = pub const THREAD_STARTED_METRIC: &str = "codex.thread.started"; pub const THREAD_SKILLS_ENABLED_TOTAL_METRIC: &str = "codex.thread.skills.enabled_total"; pub const THREAD_SKILLS_KEPT_TOTAL_METRIC: &str = "codex.thread.skills.kept_total"; +pub const THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC: &str = + "codex.thread.skills.description_truncated_chars"; pub const THREAD_SKILLS_TRUNCATED_METRIC: &str = "codex.thread.skills.truncated"; diff --git a/codex-rs/tui/src/chatwidget/tests/app_server.rs b/codex-rs/tui/src/chatwidget/tests/app_server.rs index 702bd08ef..95ec72940 100644 --- a/codex-rs/tui/src/chatwidget/tests/app_server.rs +++ b/codex-rs/tui/src/chatwidget/tests/app_server.rs @@ -191,7 +191,7 @@ async fn live_app_server_warning_notification_renders_message() { chat.handle_server_notification( ServerNotification::Warning(WarningNotification { thread_id: None, - message: "Some enabled skills were not included in the model-visible skills list for this session. Mention a skill by name or path if you need it.".to_string(), + message: "Warning: Exceeded skills context budget of 2%. All skill descriptions were removed and 2 additional skills were not included in the model-visible skills list.".to_string(), }), /*replay_kind*/ None, ); @@ -201,13 +201,13 @@ async fn live_app_server_warning_notification_renders_message() { let rendered = lines_to_single_string(&cells[0]); let normalized = rendered.split_whitespace().collect::>().join(" "); assert!( - normalized.contains( - "Some enabled skills were not included in the model-visible skills list for this session." - ), + normalized.contains("Warning: Exceeded skills context budget of 2%."), "expected warning notification message, got {rendered}" ); assert!( - normalized.contains("Mention a skill by name or path if you need it."), + normalized.contains( + "All skill descriptions were removed and 2 additional skills were not included in the model-visible skills list." + ), "expected warning guidance, got {rendered}" ); }