mirror of
https://github.com/openai/codex.git
synced 2026-10-23 18:08:45 +00:00
fix(skills): make budget warning actionable
This commit is contained in:
@@ -334,7 +334,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,
|
||||
"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."
|
||||
"Skills metadata exceeded the 2% context budget. Some skills were omitted from the model-visible skills list. Omitted skills: imagegen, openai-docs, plugin-creator, skill-creator, skill-installer, and 2 more."
|
||||
);
|
||||
|
||||
timeout(
|
||||
|
||||
@@ -17,10 +17,8 @@ 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 MAX_WARNING_SKILL_NAMES: usize = 5;
|
||||
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";
|
||||
pub const SKILLS_INTRO_WITH_ABSOLUTE_PATHS: &str = "A skill is a set of local instructions to follow that is stored in a `SKILL.md` file. Below is the list of skills that can be used. Each entry includes a name, description, and file path so you can open the source for full instructions when using a specific skill.";
|
||||
pub const SKILLS_INTRO_WITH_ALIASES: &str = "A skill is a set of local instructions to follow that is stored in a `SKILL.md` file. Below is the list of skills that can be used. Each entry includes a name, description, and a short path that can be expanded into an absolute path using the skill roots table.";
|
||||
pub const SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS: &str = r###"- Discovery: The list above is the skills available in this session (name + description + file path). Skill bodies live on disk at the listed paths.
|
||||
@@ -121,6 +119,8 @@ pub struct SkillRenderReport {
|
||||
pub omitted_count: usize,
|
||||
pub truncated_description_chars: usize,
|
||||
pub truncated_description_count: usize,
|
||||
pub largest_truncated_description_names: Vec<String>,
|
||||
pub omitted_skill_names: Vec<String>,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
@@ -210,30 +210,26 @@ fn build_available_skills_from_lines(
|
||||
|
||||
let (skill_lines, report) = render_skill_lines_from_lines(skill_lines, total_count, budget);
|
||||
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
|
||||
"{} Some skills were omitted from the model-visible skills list.{}",
|
||||
budget_warning_prefix(budget),
|
||||
warning_skill_names_suffix(
|
||||
"Omitted skills",
|
||||
&report.omitted_skill_names,
|
||||
report.omitted_count
|
||||
)
|
||||
))
|
||||
} 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()
|
||||
"{} All skills are still available, but some descriptions were shortened.{}",
|
||||
budget_warning_prefix(budget),
|
||||
warning_skill_names_suffix(
|
||||
"Largest truncated descriptions",
|
||||
&report.largest_truncated_description_names,
|
||||
report.largest_truncated_description_names.len()
|
||||
)
|
||||
))
|
||||
} else {
|
||||
None
|
||||
@@ -273,17 +269,26 @@ fn record_available_skills_side_effects(
|
||||
}
|
||||
}
|
||||
|
||||
fn budget_warning_prefix(budget: SkillMetadataBudget, prefix: &str) -> String {
|
||||
fn budget_warning_prefix(budget: SkillMetadataBudget) -> &'static str {
|
||||
match budget {
|
||||
SkillMetadataBudget::Tokens(_) => prefix.replacen(
|
||||
"Exceeded skills context budget.",
|
||||
"Exceeded skills context budget of 2%.",
|
||||
1,
|
||||
),
|
||||
SkillMetadataBudget::Characters(_) => prefix.to_string(),
|
||||
SkillMetadataBudget::Tokens(_) => "Skills metadata exceeded the 2% context budget.",
|
||||
SkillMetadataBudget::Characters(_) => "Skills metadata exceeded the context budget.",
|
||||
}
|
||||
}
|
||||
|
||||
fn warning_skill_names_suffix(label: &str, names: &[String], total_count: usize) -> String {
|
||||
if names.is_empty() {
|
||||
return String::new();
|
||||
}
|
||||
|
||||
let mut names_text = names.join(", ");
|
||||
let hidden_count = total_count.saturating_sub(names.len());
|
||||
if hidden_count > 0 {
|
||||
names_text.push_str(&format!(", and {hidden_count} more"));
|
||||
}
|
||||
format!(" {label}: {names_text}.")
|
||||
}
|
||||
|
||||
fn record_skill_render_side_effects(
|
||||
side_effects: SkillRenderSideEffects<'_>,
|
||||
total_count: usize,
|
||||
@@ -340,6 +345,8 @@ fn render_skill_lines_from_lines(
|
||||
/*omitted_count*/ 0,
|
||||
/*truncated_description_chars*/ 0,
|
||||
/*truncated_description_count*/ 0,
|
||||
Vec::new(),
|
||||
Vec::new(),
|
||||
),
|
||||
);
|
||||
}
|
||||
@@ -353,8 +360,11 @@ fn render_skill_lines_from_lines(
|
||||
&skill_lines,
|
||||
budget.limit().saturating_sub(minimum_cost),
|
||||
);
|
||||
let (truncated_description_chars, truncated_description_count) =
|
||||
sum_description_truncation(&rendered);
|
||||
let (
|
||||
truncated_description_chars,
|
||||
truncated_description_count,
|
||||
largest_truncated_description_names,
|
||||
) = summarize_description_truncation(&rendered);
|
||||
let included = rendered
|
||||
.into_iter()
|
||||
.map(|rendered| rendered.line)
|
||||
@@ -368,6 +378,8 @@ fn render_skill_lines_from_lines(
|
||||
/*omitted_count*/ 0,
|
||||
truncated_description_chars,
|
||||
truncated_description_count,
|
||||
largest_truncated_description_names,
|
||||
Vec::new(),
|
||||
),
|
||||
);
|
||||
}
|
||||
@@ -383,8 +395,10 @@ fn render_minimum_skill_lines_until_budget(
|
||||
let mut included = Vec::new();
|
||||
let mut used = 0usize;
|
||||
let mut omitted_count = 0usize;
|
||||
let mut omitted_skill_names = Vec::new();
|
||||
let mut truncated_description_chars = 0usize;
|
||||
let mut truncated_description_count = 0usize;
|
||||
let mut truncated_descriptions = Vec::new();
|
||||
for line in skill_lines {
|
||||
let line_cost = line.minimum_cost(budget);
|
||||
let description_char_count = line.description_char_count();
|
||||
@@ -393,12 +407,16 @@ fn render_minimum_skill_lines_until_budget(
|
||||
included.push(line.render_minimum());
|
||||
} else {
|
||||
omitted_count = omitted_count.saturating_add(1);
|
||||
if omitted_skill_names.len() < MAX_WARNING_SKILL_NAMES {
|
||||
omitted_skill_names.push(line.name.to_string());
|
||||
}
|
||||
}
|
||||
|
||||
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);
|
||||
truncated_descriptions.push((description_char_count, line.name.to_string()));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -408,6 +426,8 @@ fn render_minimum_skill_lines_until_budget(
|
||||
omitted_count,
|
||||
truncated_description_chars,
|
||||
truncated_description_count,
|
||||
top_skill_names_by_truncated_chars(truncated_descriptions),
|
||||
omitted_skill_names,
|
||||
);
|
||||
|
||||
(included, report)
|
||||
@@ -419,6 +439,8 @@ fn skill_render_report(
|
||||
omitted_count: usize,
|
||||
truncated_description_chars: usize,
|
||||
truncated_description_count: usize,
|
||||
largest_truncated_description_names: Vec<String>,
|
||||
omitted_skill_names: Vec<String>,
|
||||
) -> SkillRenderReport {
|
||||
SkillRenderReport {
|
||||
total_count,
|
||||
@@ -426,6 +448,8 @@ fn skill_render_report(
|
||||
omitted_count,
|
||||
truncated_description_chars,
|
||||
truncated_description_count,
|
||||
largest_truncated_description_names,
|
||||
omitted_skill_names,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -449,6 +473,7 @@ struct SkillLine<'a> {
|
||||
|
||||
struct RenderedSkillLine {
|
||||
line: String,
|
||||
skill_name: String,
|
||||
truncated_chars: usize,
|
||||
}
|
||||
|
||||
@@ -458,19 +483,38 @@ struct DescriptionBudgetLine<'a> {
|
||||
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),
|
||||
)
|
||||
}
|
||||
})
|
||||
fn summarize_description_truncation(rendered: &[RenderedSkillLine]) -> (usize, usize, Vec<String>) {
|
||||
let mut truncated_description_chars = 0usize;
|
||||
let mut truncated_description_count = 0usize;
|
||||
let mut truncated_descriptions = Vec::new();
|
||||
for line in rendered {
|
||||
if line.truncated_chars == 0 {
|
||||
continue;
|
||||
}
|
||||
truncated_description_chars =
|
||||
truncated_description_chars.saturating_add(line.truncated_chars);
|
||||
truncated_description_count = truncated_description_count.saturating_add(1);
|
||||
truncated_descriptions.push((line.truncated_chars, line.skill_name.clone()));
|
||||
}
|
||||
|
||||
(
|
||||
truncated_description_chars,
|
||||
truncated_description_count,
|
||||
top_skill_names_by_truncated_chars(truncated_descriptions),
|
||||
)
|
||||
}
|
||||
|
||||
fn top_skill_names_by_truncated_chars(mut entries: Vec<(usize, String)>) -> Vec<String> {
|
||||
entries.sort_by(|(left_chars, left_name), (right_chars, right_name)| {
|
||||
right_chars
|
||||
.cmp(left_chars)
|
||||
.then_with(|| left_name.cmp(right_name))
|
||||
});
|
||||
entries
|
||||
.into_iter()
|
||||
.take(MAX_WARNING_SKILL_NAMES)
|
||||
.map(|(_chars, name)| name)
|
||||
.collect()
|
||||
}
|
||||
|
||||
impl<'a> SkillLine<'a> {
|
||||
@@ -626,6 +670,7 @@ fn render_lines_with_description_budget(
|
||||
.saturating_sub(description_chars);
|
||||
RenderedSkillLine {
|
||||
line: line.line.render_with_description_chars(description_chars),
|
||||
skill_name: line.line.name.to_string(),
|
||||
truncated_chars,
|
||||
}
|
||||
})
|
||||
@@ -1066,7 +1111,7 @@ mod tests {
|
||||
assert_eq!(
|
||||
rendered.warning_message,
|
||||
Some(
|
||||
"Warning: Exceeded skills context budget. Loaded skill descriptions were truncated by an average of 14 characters per skill."
|
||||
"Skills metadata exceeded the context budget. All skills are still available, but some descriptions were shortened. Largest truncated descriptions: alpha-skill, beta-skill."
|
||||
.to_string()
|
||||
)
|
||||
);
|
||||
@@ -1116,7 +1161,7 @@ mod tests {
|
||||
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."
|
||||
"Skills metadata exceeded the context budget. Some skills were omitted from the model-visible skills list. Omitted skills: repo-skill, user-skill."
|
||||
.to_string()
|
||||
)
|
||||
);
|
||||
@@ -1145,7 +1190,7 @@ mod tests {
|
||||
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."
|
||||
"Skills metadata exceeded the context budget. Some skills were omitted from the model-visible skills list. Omitted skills: oversized-system-skill."
|
||||
.to_string()
|
||||
)
|
||||
);
|
||||
|
||||
@@ -5219,7 +5219,7 @@ async fn build_initial_context_trims_skill_metadata_from_context_window_budget()
|
||||
assert!(
|
||||
developer_texts
|
||||
.iter()
|
||||
.all(|text| !text.contains("Exceeded skills context budget")),
|
||||
.all(|text| !text.contains("Skills metadata exceeded")),
|
||||
"expected skill budget warning to stay out of the initial context, got {developer_texts:?}"
|
||||
);
|
||||
assert!(
|
||||
@@ -5256,7 +5256,7 @@ fn emit_thread_start_skill_metrics_records_enabled_kept_and_truncated_values() {
|
||||
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."
|
||||
"Skills metadata exceeded the context budget. Some skills were omitted from the model-visible skills list. Omitted skills: repo-skill."
|
||||
.to_string()
|
||||
)
|
||||
);
|
||||
@@ -5367,7 +5367,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 == "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."
|
||||
if message == "Skills metadata exceeded the 2% context budget. Some skills were omitted from the model-visible skills list. Omitted skills: admin-skill, repo-skill."
|
||||
));
|
||||
|
||||
let _ = session.build_initial_context(&turn_context).await;
|
||||
@@ -5378,7 +5378,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 == "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."
|
||||
if message == "Skills metadata exceeded the 2% context budget. Some skills were omitted from the model-visible skills list. Omitted skills: admin-skill, repo-skill."
|
||||
));
|
||||
}
|
||||
|
||||
|
||||
@@ -191,7 +191,7 @@ async fn live_app_server_warning_notification_renders_message() {
|
||||
chat.handle_server_notification(
|
||||
ServerNotification::Warning(WarningNotification {
|
||||
thread_id: None,
|
||||
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(),
|
||||
message: "Skills metadata exceeded the 2% context budget. All skills are still available, but some descriptions were shortened. Largest truncated descriptions: imagegen, code-replay.".to_string(),
|
||||
}),
|
||||
/*replay_kind*/ None,
|
||||
);
|
||||
@@ -201,12 +201,12 @@ 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("Warning: Exceeded skills context budget of 2%."),
|
||||
normalized.contains("Skills metadata exceeded the 2% context budget."),
|
||||
"expected warning notification message, got {rendered}"
|
||||
);
|
||||
assert!(
|
||||
normalized.contains(
|
||||
"All skill descriptions were removed and 2 additional skills were not included in the model-visible skills list."
|
||||
"All skills are still available, but some descriptions were shortened. Largest truncated descriptions: imagegen, code-replay."
|
||||
),
|
||||
"expected warning guidance, got {rendered}"
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user