mirror of
https://github.com/openai/codex.git
synced 2026-10-21 07:42:01 +00:00
Preserve discovery order for host skill path aliases (#36327)
## Why Host skill path aliases should follow the core skill loader's root discovery order, independent of how catalog entries are sorted for rendering. ## What changed - Propagate each host skill root's discovery position into its catalog entries. - Assign `r0`, `r1`, and subsequent aliases from that position while leaving entries without an explicit root order last. ## Testing Added coverage for alias ordering in extension rendering and across consecutive production turns. GitOrigin-RevId: 9085a3843c91bf2ce78a378cf5ce4926e9c38eb8
This commit is contained in:
@@ -61,6 +61,11 @@ impl SkillLoadOutcome {
|
||||
self.skill_root_by_path.get(path)
|
||||
}
|
||||
|
||||
/// Returns loaded skill roots in discovery order.
|
||||
pub fn skill_roots_in_discovery_order(&self) -> impl Iterator<Item = &AbsolutePathBuf> {
|
||||
self.skill_roots.iter()
|
||||
}
|
||||
|
||||
pub(crate) fn file_system_for_skill(
|
||||
&self,
|
||||
skill: &SkillMetadata,
|
||||
|
||||
@@ -35,6 +35,7 @@ use codex_skills_extension::provider::SkillListQuery;
|
||||
use codex_skills_extension::provider::SkillProviderFuture;
|
||||
use codex_skills_extension::provider::SkillReadRequest;
|
||||
use codex_skills_extension::provider::SkillSearchRequest;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_string::approx_token_count;
|
||||
use core_test_support::apps_test_server::AppsTestServer;
|
||||
use core_test_support::apps_test_server::apps_enabled_builder;
|
||||
@@ -602,6 +603,111 @@ async fn production_turn_keeps_full_host_only_catalog_when_it_fits() -> Result<(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn production_turn_preserves_host_alias_root_order_across_turns() -> Result<()> {
|
||||
let server = responses::start_mock_server().await;
|
||||
let response = responses::mount_sse_sequence(
|
||||
&server,
|
||||
["resp-1", "resp-2"]
|
||||
.into_iter()
|
||||
.map(|response_id| {
|
||||
sse(vec![
|
||||
ev_response_created(response_id),
|
||||
ev_completed(response_id),
|
||||
])
|
||||
})
|
||||
.collect(),
|
||||
)
|
||||
.await;
|
||||
let temp_parent = TempDir::new()?;
|
||||
let long_parent = temp_parent
|
||||
.path()
|
||||
.join("codex-home-with-long-shared-prefix-for-production-alias-order-test");
|
||||
std::fs::create_dir_all(&long_parent)?;
|
||||
let codex_home = Arc::new(TempDir::new_in(&long_parent)?);
|
||||
let first_root_path = long_parent.join("first-discovered-skills-root-with-long-shared-prefix");
|
||||
let second_root_path =
|
||||
long_parent.join("second-discovered-skills-root-with-long-shared-prefix");
|
||||
for (root, prefix) in [
|
||||
(&first_root_path, "z-first"),
|
||||
(&second_root_path, "a-second"),
|
||||
] {
|
||||
for index in 0..6 {
|
||||
let name = format!("{prefix}-{index:02}");
|
||||
let skill_dir = root.join(&name);
|
||||
std::fs::create_dir_all(&skill_dir)?;
|
||||
std::fs::write(
|
||||
skill_dir.join("SKILL.md"),
|
||||
format!("---\nname: {name}\ndescription: d\n---\n\n# body\n"),
|
||||
)?;
|
||||
}
|
||||
}
|
||||
let first_root = AbsolutePathBuf::try_from(std::fs::canonicalize(&first_root_path)?)?;
|
||||
let second_root = AbsolutePathBuf::try_from(std::fs::canonicalize(&second_root_path)?)?;
|
||||
let (extensions, _) =
|
||||
catalog_extensions(SkillCatalog::default(), /*include_host_provider*/ true);
|
||||
let mut builder = test_codex()
|
||||
.with_home(Arc::clone(&codex_home))
|
||||
.with_extensions(extensions)
|
||||
.with_model_info_override("gpt-5.5", |model_info| {
|
||||
model_info.context_window = Some(SHORTENING_CONTEXT_WINDOW);
|
||||
model_info.max_context_window = None;
|
||||
})
|
||||
.with_config(configure_catalog_test);
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
test.thread_manager
|
||||
.skills_service()
|
||||
.set_extra_roots(vec![first_root.clone(), second_root.clone()]);
|
||||
|
||||
test.submit_turn("Inspect the available skills.").await?;
|
||||
test.submit_turn("Inspect the available skills again.")
|
||||
.await?;
|
||||
|
||||
let expected_alias_lines = vec![
|
||||
format!(
|
||||
"- `r0` = `{}`",
|
||||
first_root.to_string_lossy().replace('\\', "/")
|
||||
),
|
||||
format!(
|
||||
"- `r1` = `{}`",
|
||||
second_root.to_string_lossy().replace('\\', "/")
|
||||
),
|
||||
];
|
||||
let expected_skill_lines = vec![
|
||||
"- a-second-00: d (file: r1/a-second-00/SKILL.md)".to_string(),
|
||||
"- z-first-00: d (file: r0/z-first-00/SKILL.md)".to_string(),
|
||||
];
|
||||
let actual = response
|
||||
.requests()
|
||||
.iter()
|
||||
.map(|request| {
|
||||
let developer_text = request.message_input_texts("developer").join("\n");
|
||||
let alias_lines = developer_text
|
||||
.lines()
|
||||
.filter(|line| line.starts_with("- `r"))
|
||||
.map(str::to_string)
|
||||
.collect::<Vec<_>>();
|
||||
let skill_lines = developer_text
|
||||
.lines()
|
||||
.filter(|line| {
|
||||
line.starts_with("- a-second-00:") || line.starts_with("- z-first-00:")
|
||||
})
|
||||
.map(str::to_string)
|
||||
.collect::<Vec<_>>();
|
||||
(alias_lines, skill_lines)
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
assert_eq!(
|
||||
actual,
|
||||
vec![
|
||||
(expected_alias_lines.clone(), expected_skill_lines.clone()),
|
||||
(expected_alias_lines, expected_skill_lines),
|
||||
]
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn production_turn_uses_provider_host_catalog_and_core_snapshot_injection() -> Result<()> {
|
||||
let server = responses::start_mock_server().await;
|
||||
|
||||
@@ -179,6 +179,7 @@ pub struct SkillCatalogEntry {
|
||||
pub main_prompt: SkillResourceId,
|
||||
pub display_path: Option<String>,
|
||||
display_path_root: Option<String>,
|
||||
display_path_root_order: Option<usize>,
|
||||
prompt_scope: Option<SkillScope>,
|
||||
pub dependencies: Option<SkillDependencies>,
|
||||
pub enabled: bool,
|
||||
@@ -202,6 +203,7 @@ impl SkillCatalogEntry {
|
||||
main_prompt,
|
||||
display_path: None,
|
||||
display_path_root: None,
|
||||
display_path_root_order: None,
|
||||
prompt_scope: None,
|
||||
dependencies: None,
|
||||
enabled: true,
|
||||
@@ -225,6 +227,11 @@ impl SkillCatalogEntry {
|
||||
self
|
||||
}
|
||||
|
||||
pub(crate) fn with_display_path_root_order(mut self, display_path_root_order: usize) -> Self {
|
||||
self.display_path_root_order = Some(display_path_root_order);
|
||||
self
|
||||
}
|
||||
|
||||
pub(crate) fn with_prompt_scope(mut self, prompt_scope: SkillScope) -> Self {
|
||||
self.prompt_scope = Some(prompt_scope);
|
||||
self
|
||||
@@ -259,6 +266,10 @@ impl SkillCatalogEntry {
|
||||
self.display_path_root.as_deref()
|
||||
}
|
||||
|
||||
pub(crate) fn display_path_root_order(&self) -> Option<usize> {
|
||||
self.display_path_root_order
|
||||
}
|
||||
|
||||
pub(crate) fn prompt_scope(&self) -> Option<SkillScope> {
|
||||
self.prompt_scope
|
||||
}
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
use std::collections::HashMap;
|
||||
|
||||
use codex_core_skills::SkillLoadOutcome;
|
||||
use codex_skills::SkillMetadata;
|
||||
|
||||
@@ -82,6 +84,11 @@ impl SkillProvider for HostSkillProvider {
|
||||
}
|
||||
|
||||
fn catalog_from_outcome(outcome: &SkillLoadOutcome) -> SkillCatalog {
|
||||
let root_order_by_path = outcome
|
||||
.skill_roots_in_discovery_order()
|
||||
.enumerate()
|
||||
.map(|(index, root)| (root.as_path(), index))
|
||||
.collect::<HashMap<_, _>>();
|
||||
let mut catalog = SkillCatalog {
|
||||
entries: Vec::new(),
|
||||
warnings: outcome
|
||||
@@ -101,6 +108,9 @@ fn catalog_from_outcome(outcome: &SkillLoadOutcome) -> SkillCatalog {
|
||||
let mut entry = catalog_entry_from_skill(skill, enabled);
|
||||
if let Some(root) = outcome.skill_root_for_path(&skill.path_to_skills_md) {
|
||||
entry = entry.with_display_path_root(root.to_string_lossy().replace('\\', "/"));
|
||||
if let Some(root_order) = root_order_by_path.get(root.as_path()) {
|
||||
entry = entry.with_display_path_root_order(*root_order);
|
||||
}
|
||||
}
|
||||
catalog.push_entry(entry);
|
||||
}
|
||||
|
||||
@@ -892,7 +892,10 @@ fn build_alias_plan(
|
||||
let mut alias_root_by_display_root = HashMap::new();
|
||||
let mut alias_roots = Vec::new();
|
||||
let mut seen = HashSet::new();
|
||||
for entry in entries {
|
||||
let mut alias_ordered_entries = entries.to_vec();
|
||||
alias_ordered_entries
|
||||
.sort_by_key(|entry| entry.display_path_root_order().unwrap_or(usize::MAX));
|
||||
for entry in alias_ordered_entries {
|
||||
if entry.authority.kind != SkillSourceKind::Host {
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -2,10 +2,22 @@ use super::*;
|
||||
use crate::catalog::SkillAuthority;
|
||||
use crate::catalog::SkillPackageId;
|
||||
use crate::catalog::SkillResourceId;
|
||||
use crate::provider::HostSkillProvider;
|
||||
use crate::provider::SkillListQuery;
|
||||
use crate::provider::SkillProvider;
|
||||
use codex_core_skills::HostSkillsSnapshot;
|
||||
use codex_core_skills::loader::SkillRoot;
|
||||
use codex_core_skills::loader::load_skills_from_roots;
|
||||
use codex_core_skills::render_available_skills_body;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_extension_api::ContextualUserFragment;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::sync::Arc;
|
||||
use std::time::SystemTime;
|
||||
use std::time::UNIX_EPOCH;
|
||||
use tokio::sync::Semaphore;
|
||||
|
||||
fn entry(name: &str, description: &str, short_description: Option<&str>) -> SkillCatalogEntry {
|
||||
entry_with_path(
|
||||
@@ -266,6 +278,89 @@ fn path_aliases_retain_every_skill_under_budget_pressure() {
|
||||
assert!(!body.contains("additional skills omitted"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn host_alias_roots_follow_core_discovery_order() -> Result<(), Box<dyn std::error::Error>> {
|
||||
let unique = SystemTime::now().duration_since(UNIX_EPOCH)?.as_nanos();
|
||||
let parent = std::env::temp_dir().join(format!(
|
||||
"codex-skills-extension-alias-order-{}-{unique}",
|
||||
std::process::id()
|
||||
));
|
||||
let user_root_path = parent.join("user-root");
|
||||
let system_root_path = parent.join("system-root");
|
||||
for (root, name) in [
|
||||
(&user_root_path, "user-skill"),
|
||||
(&system_root_path, "system-skill"),
|
||||
] {
|
||||
let skill_dir = root.join(name);
|
||||
std::fs::create_dir_all(&skill_dir)?;
|
||||
std::fs::write(
|
||||
skill_dir.join("SKILL.md"),
|
||||
format!("---\nname: {name}\ndescription: {name}\n---\n"),
|
||||
)?;
|
||||
}
|
||||
let user_root = AbsolutePathBuf::try_from(std::fs::canonicalize(&user_root_path)?)?;
|
||||
let system_root = AbsolutePathBuf::try_from(std::fs::canonicalize(&system_root_path)?)?;
|
||||
let outcome = load_skills_from_roots(
|
||||
[
|
||||
SkillRoot {
|
||||
path: user_root.clone(),
|
||||
scope: SkillScope::User,
|
||||
file_system: Arc::clone(&LOCAL_FS),
|
||||
plugin_identity: None,
|
||||
plugin_namespace: None,
|
||||
plugin_root: None,
|
||||
discovery_mode: Default::default(),
|
||||
},
|
||||
SkillRoot {
|
||||
path: system_root.clone(),
|
||||
scope: SkillScope::System,
|
||||
file_system: Arc::clone(&LOCAL_FS),
|
||||
plugin_identity: None,
|
||||
plugin_namespace: None,
|
||||
plugin_root: None,
|
||||
discovery_mode: Default::default(),
|
||||
},
|
||||
],
|
||||
/*plugin_skill_snapshots*/ None,
|
||||
Arc::new(Semaphore::new(2)),
|
||||
)
|
||||
.await;
|
||||
let catalog = HostSkillProvider::new()
|
||||
.list(SkillListQuery {
|
||||
turn_id: "turn-1".to_string(),
|
||||
executor_roots: Vec::new(),
|
||||
resolved_executor_roots: Vec::new(),
|
||||
host_snapshot: Some(Arc::new(HostSkillsSnapshot::new(Arc::new(outcome)))),
|
||||
include_host_skills: true,
|
||||
include_bundled_skills: false,
|
||||
include_orchestrator_skills: false,
|
||||
mcp_resources: None,
|
||||
executor_capability_discovery: None,
|
||||
})
|
||||
.await?;
|
||||
let mut entries = catalog.entries.iter().collect::<Vec<_>>();
|
||||
SkillCatalogRenderPolicy::CoreCompatible.order_entries(&mut entries);
|
||||
let actual = build_alias_plan(&entries, SkillMetadataBudget::Characters(usize::MAX))
|
||||
.expect("alias plan should build")
|
||||
.skill_root_lines;
|
||||
std::fs::remove_dir_all(parent)?;
|
||||
|
||||
assert_eq!(
|
||||
actual,
|
||||
vec![
|
||||
format!(
|
||||
"- `r0` = `{}`",
|
||||
user_root.to_string_lossy().replace('\\', "/")
|
||||
),
|
||||
format!(
|
||||
"- `r1` = `{}`",
|
||||
system_root.to_string_lossy().replace('\\', "/")
|
||||
),
|
||||
]
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mixed_catalogs_keep_absolute_authority_aware_rendering_under_budget_pressure() {
|
||||
let root = "/Users/test/.codex/plugins/cache/openai-curated/example/hash1234567890/skills-with-a-very-long-shared-prefix";
|
||||
|
||||
Reference in New Issue
Block a user