mirror of
https://github.com/openai/codex.git
synced 2026-10-22 17:58:46 +00:00
Reduce skill catalog prompts with path aliases (#41011)
## Why Repeated skill locator roots can make catalog prompts larger even when the catalog fits within its metadata budget. ## What changed Evaluate aliased catalogs regardless of budget pressure and select them when they preserve skill inclusion and description content while reducing prompt size. Apply the same selection to combined host, executor, and orchestrator catalogs. ## Testing Add coverage for alias selection in full host-only catalogs and update skill, symlink, and orchestrator resource tests to expect shortened locators and their root mappings. GitOrigin-RevId: ccedd3f3f6c7efaa1e5177a2d8c3d81d59239f52
This commit is contained in:
@@ -266,7 +266,12 @@ async fn orchestrator_skill_can_read_referenced_resource_without_an_executor() -
|
||||
|
||||
let developer_messages = first_request.message_input_texts("developer");
|
||||
let catalog_line =
|
||||
format!("- {SKILL_NAME}: {SKILL_DESCRIPTION} (orchestrator package: {SKILL_RESOURCE_URI})");
|
||||
format!("- {SKILL_NAME}: {SKILL_DESCRIPTION} (orchestrator package: o0/deploy)");
|
||||
assert!(
|
||||
developer_messages
|
||||
.iter()
|
||||
.any(|text| text.contains("- `o0` = `skill://plugin_demo`"))
|
||||
);
|
||||
assert_eq!(
|
||||
1,
|
||||
developer_messages
|
||||
|
||||
@@ -269,10 +269,16 @@ async fn user_turn_selects_symlinked_skill_by_advertised_discovery_path() -> Res
|
||||
|
||||
let request = mock.single_request();
|
||||
let developer_texts = request.message_input_texts("developer");
|
||||
let advertised_path = format!(
|
||||
"(file: {})",
|
||||
discovery_path.to_string_lossy().replace('\\', "/")
|
||||
);
|
||||
let discovery_root_display = discovery_root.to_string_lossy().replace('\\', "/");
|
||||
let root_suffix = format!(" = `{discovery_root_display}`");
|
||||
let discovery_root_alias = developer_texts
|
||||
.iter()
|
||||
.flat_map(|text| text.lines())
|
||||
.find(|line| line.ends_with(&root_suffix))
|
||||
.and_then(|line| line.strip_prefix("- `"))
|
||||
.and_then(|line| line.split_once("` = ").map(|(alias, _)| alias))
|
||||
.expect("skill catalog should alias the advertised discovery root");
|
||||
let advertised_path = format!("(file: {discovery_root_alias}/linked-demo/SKILL.md)");
|
||||
assert!(
|
||||
developer_texts
|
||||
.iter()
|
||||
|
||||
@@ -2250,12 +2250,12 @@ async fn production_turn_uses_configured_skill_catalog_token_budget() -> Result<
|
||||
async fn production_turn_keeps_full_host_only_catalog_when_it_fits() -> Result<()> {
|
||||
let (developer_texts, _) =
|
||||
rendered_catalogs(&HOST_CATALOG, &[], FULL_CATALOG_CONTEXT_WINDOW).await?;
|
||||
let host_lines = developer_texts
|
||||
.iter()
|
||||
.flat_map(|text| skill_lines(text, "host"))
|
||||
.collect::<Vec<_>>();
|
||||
let host_catalog = catalog_text(&developer_texts, "host");
|
||||
let host_lines = skill_lines(host_catalog, "host");
|
||||
|
||||
assert_full_descriptions(&host_lines, &HOST_CATALOG);
|
||||
assert!(host_catalog.contains("### Skill roots"));
|
||||
assert!(host_catalog.contains("(file: r0/host-alpha/SKILL.md)"));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -515,22 +515,18 @@ pub(crate) fn render_available_skills(
|
||||
SkillPromptKind::Unaliased,
|
||||
policy,
|
||||
);
|
||||
let selected =
|
||||
if absolute.report.omitted_count == 0 && absolute.report.truncated_description_chars == 0 {
|
||||
absolute
|
||||
} else if let Some(aliased) =
|
||||
build_aliased_catalog(&entries, policy, budget, include_skills_usage_instructions)
|
||||
&& aliased_render_is_better(
|
||||
&aliased,
|
||||
&absolute,
|
||||
budget,
|
||||
include_skills_usage_instructions,
|
||||
)
|
||||
{
|
||||
aliased
|
||||
} else {
|
||||
absolute
|
||||
};
|
||||
let selected = if let Some(aliased) =
|
||||
build_aliased_catalog(&entries, policy, budget, include_skills_usage_instructions)
|
||||
&& aliased_render_is_better(
|
||||
&aliased,
|
||||
&absolute,
|
||||
budget,
|
||||
include_skills_usage_instructions,
|
||||
) {
|
||||
aliased
|
||||
} else {
|
||||
absolute
|
||||
};
|
||||
|
||||
Some(AvailableSkillsRender {
|
||||
prompt_kind: selected.prompt_kind,
|
||||
@@ -607,53 +603,51 @@ pub(crate) fn render_combined_available_skills(
|
||||
);
|
||||
|
||||
let mut selected = absolute;
|
||||
if !combined_catalog_fully_rendered(&selected) {
|
||||
let host_only_aliases = build_aliased_combined_catalog(
|
||||
CatalogLines::unaliased(&executor_entries, extension_policy),
|
||||
CatalogLines::unaliased(&orchestrator_entries, extension_policy),
|
||||
CatalogLines::aliased(&host_entries, host_policy),
|
||||
budget,
|
||||
include_skills_usage_instructions,
|
||||
);
|
||||
let executor_only_aliases = build_aliased_combined_catalog(
|
||||
CatalogLines::aliased(&executor_entries, extension_policy),
|
||||
CatalogLines::unaliased(&orchestrator_entries, extension_policy),
|
||||
CatalogLines::unaliased(&host_entries, host_policy),
|
||||
budget,
|
||||
include_skills_usage_instructions,
|
||||
);
|
||||
let orchestrator_only_aliases = build_aliased_combined_catalog(
|
||||
CatalogLines::unaliased(&executor_entries, extension_policy),
|
||||
CatalogLines::aliased(&orchestrator_entries, extension_policy),
|
||||
CatalogLines::unaliased(&host_entries, host_policy),
|
||||
budget,
|
||||
include_skills_usage_instructions,
|
||||
);
|
||||
let all_source_aliases = build_aliased_combined_catalog(
|
||||
CatalogLines::aliased(&executor_entries, extension_policy),
|
||||
CatalogLines::aliased(&orchestrator_entries, extension_policy),
|
||||
CatalogLines::aliased(&host_entries, host_policy),
|
||||
budget,
|
||||
include_skills_usage_instructions,
|
||||
);
|
||||
let host_only_aliases = build_aliased_combined_catalog(
|
||||
CatalogLines::unaliased(&executor_entries, extension_policy),
|
||||
CatalogLines::unaliased(&orchestrator_entries, extension_policy),
|
||||
CatalogLines::aliased(&host_entries, host_policy),
|
||||
budget,
|
||||
include_skills_usage_instructions,
|
||||
);
|
||||
let executor_only_aliases = build_aliased_combined_catalog(
|
||||
CatalogLines::aliased(&executor_entries, extension_policy),
|
||||
CatalogLines::unaliased(&orchestrator_entries, extension_policy),
|
||||
CatalogLines::unaliased(&host_entries, host_policy),
|
||||
budget,
|
||||
include_skills_usage_instructions,
|
||||
);
|
||||
let orchestrator_only_aliases = build_aliased_combined_catalog(
|
||||
CatalogLines::unaliased(&executor_entries, extension_policy),
|
||||
CatalogLines::aliased(&orchestrator_entries, extension_policy),
|
||||
CatalogLines::unaliased(&host_entries, host_policy),
|
||||
budget,
|
||||
include_skills_usage_instructions,
|
||||
);
|
||||
let all_source_aliases = build_aliased_combined_catalog(
|
||||
CatalogLines::aliased(&executor_entries, extension_policy),
|
||||
CatalogLines::aliased(&orchestrator_entries, extension_policy),
|
||||
CatalogLines::aliased(&host_entries, host_policy),
|
||||
budget,
|
||||
include_skills_usage_instructions,
|
||||
);
|
||||
|
||||
for candidate in [
|
||||
host_only_aliases,
|
||||
executor_only_aliases,
|
||||
orchestrator_only_aliases,
|
||||
all_source_aliases,
|
||||
]
|
||||
.into_iter()
|
||||
.flatten()
|
||||
{
|
||||
if combined_render_is_better(
|
||||
&candidate,
|
||||
&selected,
|
||||
budget,
|
||||
include_skills_usage_instructions,
|
||||
) {
|
||||
selected = candidate;
|
||||
}
|
||||
for candidate in [
|
||||
host_only_aliases,
|
||||
executor_only_aliases,
|
||||
orchestrator_only_aliases,
|
||||
all_source_aliases,
|
||||
]
|
||||
.into_iter()
|
||||
.flatten()
|
||||
{
|
||||
if combined_render_is_better(
|
||||
&candidate,
|
||||
&selected,
|
||||
budget,
|
||||
include_skills_usage_instructions,
|
||||
) {
|
||||
selected = candidate;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -842,14 +836,6 @@ fn build_aliased_combined_catalog(
|
||||
))
|
||||
}
|
||||
|
||||
fn combined_catalog_fully_rendered(rendered: &CombinedAvailableSkillsRender) -> bool {
|
||||
[&rendered.executor, &rendered.orchestrator, &rendered.host]
|
||||
.into_iter()
|
||||
.all(|catalog| {
|
||||
catalog.report.omitted_count == 0 && catalog.report.truncated_description_chars == 0
|
||||
})
|
||||
}
|
||||
|
||||
fn combined_render_is_better(
|
||||
candidate: &CombinedAvailableSkillsRender,
|
||||
current: &CombinedAvailableSkillsRender,
|
||||
|
||||
@@ -220,6 +220,20 @@ fn catalog_budget_uses_context_percentage_or_character_fallback() {
|
||||
#[test]
|
||||
fn host_only_prompts_preserve_existing_behavior_with_and_without_aliases() {
|
||||
let root = "/Users/test/.codex/plugins/cache/openai-curated/host-plugin/1.0.0/skills-with-a-long-shared-root";
|
||||
let unaliased_catalog = SkillCatalog {
|
||||
entries: [
|
||||
("alpha", "Alpha skill."),
|
||||
("beta", "Beta skill."),
|
||||
("gamma", "Gamma skill."),
|
||||
]
|
||||
.into_iter()
|
||||
.map(|(name, description)| {
|
||||
entry(name, description, /*short_description*/ None)
|
||||
.with_display_path(format!("{root}/{name}/SKILL.md"))
|
||||
})
|
||||
.collect(),
|
||||
warnings: Vec::new(),
|
||||
};
|
||||
let catalog = SkillCatalog {
|
||||
entries: [
|
||||
("alpha", "Alpha skill."),
|
||||
@@ -237,7 +251,7 @@ fn host_only_prompts_preserve_existing_behavior_with_and_without_aliases() {
|
||||
};
|
||||
|
||||
let unaliased = available_skills_fragment(
|
||||
&catalog,
|
||||
&unaliased_catalog,
|
||||
/*include_skills_usage_instructions*/ true,
|
||||
SkillCatalogRenderPolicy::CoreCompatible,
|
||||
SkillMetadataBudget::Characters(usize::MAX),
|
||||
@@ -310,7 +324,7 @@ fn host_only_prompts_preserve_existing_behavior_with_and_without_aliases() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn path_aliases_are_not_used_without_budget_pressure() {
|
||||
fn path_aliases_are_used_without_budget_pressure_when_they_reduce_prompt_size() {
|
||||
let root = "/Users/test/.codex/plugins/cache/openai-curated/example/hash/skills";
|
||||
let catalog = SkillCatalog {
|
||||
entries: vec![
|
||||
@@ -332,12 +346,8 @@ fn path_aliases_are_not_used_without_budget_pressure() {
|
||||
)
|
||||
.expect("catalog should render");
|
||||
|
||||
assert!(!fragment.body().contains("### Skill roots"));
|
||||
assert!(
|
||||
fragment
|
||||
.body()
|
||||
.contains(&format!("(file: {root}/alpha/SKILL.md)"))
|
||||
);
|
||||
assert!(fragment.body().contains("### Skill roots"));
|
||||
assert!(fragment.body().contains("(file: r0/alpha/SKILL.md)"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user