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