mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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.
This commit is contained in:
committed by
GitHub
Unverified
parent
ddde50c611
commit
b77791c228
@@ -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(
|
||||
|
||||
@@ -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<String>,
|
||||
pub report: SkillRenderReport,
|
||||
pub emit_warning: bool,
|
||||
pub warning_message: Option<String>,
|
||||
}
|
||||
|
||||
pub fn default_skill_metadata_budget(context_window: Option<i64>) -> SkillMetadataBudget {
|
||||
@@ -75,37 +94,91 @@ pub fn build_available_skills(
|
||||
side_effects: SkillRenderSideEffects<'_>,
|
||||
) -> Option<AvailableSkills> {
|
||||
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<String>, SkillRenderReport) {
|
||||
let ordered_skills = ordered_skills_for_budget(skills);
|
||||
let skill_lines = ordered_skills
|
||||
.into_iter()
|
||||
.map(SkillLine::new)
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
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::<Vec<_>>();
|
||||
|
||||
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::<Vec<_>>();
|
||||
|
||||
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<SkillLine<'_>>,
|
||||
total_count: usize,
|
||||
) -> (Vec<String>, 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<usize>,
|
||||
}
|
||||
|
||||
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<RenderedSkillLine> {
|
||||
let budget_lines = skill_lines
|
||||
.iter()
|
||||
.map(|line| DescriptionBudgetLine::new(line, budget))
|
||||
.collect::<Vec<_>>();
|
||||
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::<Vec<_>>();
|
||||
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:"));
|
||||
|
||||
@@ -373,8 +373,6 @@ pub struct Codex {
|
||||
|
||||
pub(crate) type SessionLoopTermination = Shared<BoxFuture<'static, ()>>;
|
||||
|
||||
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;
|
||||
|
||||
@@ -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."
|
||||
));
|
||||
}
|
||||
|
||||
|
||||
@@ -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";
|
||||
|
||||
@@ -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::<Vec<_>>().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}"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user