diff --git a/codex-rs/app-server/tests/suite/v2/mcp_resource.rs b/codex-rs/app-server/tests/suite/v2/mcp_resource.rs index f4427207b5..2064b4bf07 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_resource.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_resource.rs @@ -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 diff --git a/codex-rs/core/tests/suite/skills.rs b/codex-rs/core/tests/suite/skills.rs index 6bec2c73ec..896ccc73ea 100644 --- a/codex-rs/core/tests/suite/skills.rs +++ b/codex-rs/core/tests/suite/skills.rs @@ -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() diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index 6004c79e24..fa92c557ae 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -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::>(); + 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(()) } diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index 92cba67ad6..7deb8446de 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -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, diff --git a/codex-rs/ext/skills/src/render_tests.rs b/codex-rs/ext/skills/src/render_tests.rs index 090224fc58..06e40f4baf 100644 --- a/codex-rs/ext/skills/src/render_tests.rs +++ b/codex-rs/ext/skills/src/render_tests.rs @@ -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]