mirror of
https://github.com/openai/codex.git
synced 2026-10-21 16:38:24 +00:00
Preserve discovery paths for symlinked skills (#37144)
## Why Symlinked skills have a canonical `SKILL.md` path that can differ from the path under the configured skill root. A catalog entry that advertises the discovery path must still select the canonical skill. ## What changed - Track each loaded skill's discovery path alongside its canonical identity. - Render discovery paths in the skill catalog and accept either form for structured selections and linked skill mentions. - Preserve enabled-state checks, deduplication, and product filtering when resolving discovery paths. ## Testing Added loader, catalog, mention-resolution, and end-to-end turn coverage for skills discovered through directory symlinks. GitOrigin-RevId: bae539b766cc3782c7c121a4276c337699cbb509
This commit is contained in:
@@ -174,24 +174,23 @@ fn emit_skill_injected_metric(
|
||||
///
|
||||
/// Structured `UserInput::Skill` selections are resolved first by path against
|
||||
/// enabled skills. Text inputs are then scanned to extract `$skill-name` tokens, and we
|
||||
/// iterate `skills` in their existing order to preserve prior ordering semantics.
|
||||
/// Explicit links are resolved by path and plain names are only used when the match
|
||||
/// is unambiguous.
|
||||
/// iterate loaded skills in their existing order to preserve prior ordering semantics.
|
||||
/// Explicit paths match either a skill's canonical identity or its logical discovery
|
||||
/// path, and plain names are only used when the match is unambiguous.
|
||||
///
|
||||
/// Complexity: `O(T + (N_s + N_t) * S)` time, `O(S + M)` space, where:
|
||||
/// `S` = number of skills, `T` = total text length, `N_s` = number of structured skill inputs,
|
||||
/// `N_t` = number of text inputs, `M` = max mentions parsed from a single text input.
|
||||
pub fn collect_explicit_skill_mentions(
|
||||
inputs: &[UserInput],
|
||||
skills: &[SkillMetadata],
|
||||
disabled_paths: &HashSet<AbsolutePathBuf>,
|
||||
loaded_skills: &SkillLoadOutcome,
|
||||
connector_slug_counts: &HashMap<String, usize>,
|
||||
) -> Vec<SkillMetadata> {
|
||||
let skill_name_counts = build_skill_name_counts(skills, disabled_paths).0;
|
||||
let skill_name_counts =
|
||||
build_skill_name_counts(&loaded_skills.skills, &loaded_skills.disabled_paths).0;
|
||||
|
||||
let selection_context = SkillSelectionContext {
|
||||
skills,
|
||||
disabled_paths,
|
||||
loaded_skills,
|
||||
skill_name_counts: &skill_name_counts,
|
||||
connector_slug_counts,
|
||||
};
|
||||
@@ -207,19 +206,25 @@ pub fn collect_explicit_skill_mentions(
|
||||
continue;
|
||||
};
|
||||
|
||||
if selection_context.disabled_paths.contains(&path) || seen_paths.contains(&path) {
|
||||
let Some(skill) = selection_context.loaded_skills.skills.iter().find(|skill| {
|
||||
skill.path_to_skills_md == path
|
||||
|| selection_context
|
||||
.loaded_skills
|
||||
.skill_discovery_path_for_path(&skill.path_to_skills_md)
|
||||
.is_some_and(|discovery_path| discovery_path == &path)
|
||||
}) else {
|
||||
continue;
|
||||
};
|
||||
|
||||
if !selection_context.loaded_skills.is_skill_enabled(skill)
|
||||
|| seen_paths.contains(&skill.path_to_skills_md)
|
||||
{
|
||||
continue;
|
||||
}
|
||||
|
||||
if let Some(skill) = selection_context
|
||||
.skills
|
||||
.iter()
|
||||
.find(|skill| skill.path_to_skills_md == path)
|
||||
{
|
||||
seen_paths.insert(skill.path_to_skills_md.clone());
|
||||
seen_names.insert(skill.name.clone());
|
||||
selected.push(skill.clone());
|
||||
}
|
||||
seen_paths.insert(skill.path_to_skills_md.clone());
|
||||
seen_names.insert(skill.name.clone());
|
||||
selected.push(skill.clone());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -241,8 +246,7 @@ pub fn collect_explicit_skill_mentions(
|
||||
}
|
||||
|
||||
struct SkillSelectionContext<'a> {
|
||||
skills: &'a [SkillMetadata],
|
||||
disabled_paths: &'a HashSet<AbsolutePathBuf>,
|
||||
loaded_skills: &'a SkillLoadOutcome,
|
||||
skill_name_counts: &'a HashMap<String, usize>,
|
||||
connector_slug_counts: &'a HashMap<String, usize>,
|
||||
}
|
||||
@@ -400,7 +404,7 @@ fn select_skills_from_mentions(
|
||||
return;
|
||||
}
|
||||
|
||||
let mention_skill_paths: HashSet<&str> = mentions
|
||||
let mention_skill_paths: HashSet<String> = mentions
|
||||
.paths()
|
||||
.filter(|path| {
|
||||
!matches!(
|
||||
@@ -408,30 +412,34 @@ fn select_skills_from_mentions(
|
||||
ToolMentionKind::App | ToolMentionKind::Mcp | ToolMentionKind::Plugin
|
||||
)
|
||||
})
|
||||
.map(normalize_skill_path)
|
||||
.map(normalize_host_skill_path)
|
||||
.collect();
|
||||
|
||||
for skill in selection_context.skills {
|
||||
if selection_context
|
||||
.disabled_paths
|
||||
.contains(&skill.path_to_skills_md)
|
||||
for skill in &selection_context.loaded_skills.skills {
|
||||
if !selection_context.loaded_skills.is_skill_enabled(skill)
|
||||
|| seen_paths.contains(&skill.path_to_skills_md)
|
||||
{
|
||||
continue;
|
||||
}
|
||||
|
||||
let path_str = skill.path_to_skills_md.to_string_lossy();
|
||||
if mention_skill_paths.contains(path_str.as_ref()) {
|
||||
let canonical_path = normalize_host_skill_path(&skill.path_to_skills_md.to_string_lossy());
|
||||
let matches_discovery_path = selection_context
|
||||
.loaded_skills
|
||||
.skill_discovery_path_for_path(&skill.path_to_skills_md)
|
||||
.is_some_and(|discovery_path| {
|
||||
mention_skill_paths.contains(&normalize_host_skill_path(
|
||||
&discovery_path.to_string_lossy(),
|
||||
))
|
||||
});
|
||||
if mention_skill_paths.contains(&canonical_path) || matches_discovery_path {
|
||||
seen_paths.insert(skill.path_to_skills_md.clone());
|
||||
seen_names.insert(skill.name.clone());
|
||||
selected.push(skill.clone());
|
||||
}
|
||||
}
|
||||
|
||||
for skill in selection_context.skills {
|
||||
if selection_context
|
||||
.disabled_paths
|
||||
.contains(&skill.path_to_skills_md)
|
||||
for skill in &selection_context.loaded_skills.skills {
|
||||
if !selection_context.loaded_skills.is_skill_enabled(skill)
|
||||
|| seen_paths.contains(&skill.path_to_skills_md)
|
||||
{
|
||||
continue;
|
||||
|
||||
@@ -5,6 +5,7 @@ use codex_utils_absolute_path::test_support::test_path_buf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
use std::sync::Arc;
|
||||
|
||||
fn make_skill(name: &str, path: &str) -> SkillMetadata {
|
||||
SkillMetadata {
|
||||
@@ -51,7 +52,26 @@ fn collect_mentions(
|
||||
disabled_paths: &HashSet<AbsolutePathBuf>,
|
||||
connector_slug_counts: &HashMap<String, usize>,
|
||||
) -> Vec<SkillMetadata> {
|
||||
collect_explicit_skill_mentions(inputs, skills, disabled_paths, connector_slug_counts)
|
||||
let loaded_skills = SkillLoadOutcome {
|
||||
skills: skills.to_vec(),
|
||||
disabled_paths: disabled_paths.clone(),
|
||||
..Default::default()
|
||||
};
|
||||
collect_explicit_skill_mentions(inputs, &loaded_skills, connector_slug_counts)
|
||||
}
|
||||
|
||||
fn skill_outcome_with_discovery_path(
|
||||
skill: SkillMetadata,
|
||||
discovery_path: &str,
|
||||
) -> SkillLoadOutcome {
|
||||
SkillLoadOutcome {
|
||||
skill_discovery_path_by_path: Arc::new(HashMap::from([(
|
||||
skill.path_to_skills_md.clone(),
|
||||
test_path_buf(discovery_path).abs(),
|
||||
)])),
|
||||
skills: vec![skill],
|
||||
..Default::default()
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -180,6 +200,61 @@ fn collect_explicit_skill_mentions_prioritizes_structured_inputs() {
|
||||
assert_eq!(selected, vec![beta, alpha]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collect_explicit_skill_mentions_accepts_structured_discovery_path() {
|
||||
let skill = make_skill("linked-skill", "/tmp/shared/linked-skill/SKILL.md");
|
||||
let loaded_skills = skill_outcome_with_discovery_path(
|
||||
skill.clone(),
|
||||
"/tmp/project/.agents/skills/linked-skill/SKILL.md",
|
||||
);
|
||||
let inputs = vec![UserInput::Skill {
|
||||
name: "linked-skill".to_string(),
|
||||
path: test_path_buf("/tmp/project/.agents/skills/linked-skill/SKILL.md"),
|
||||
}];
|
||||
|
||||
let selected = collect_explicit_skill_mentions(&inputs, &loaded_skills, &HashMap::new());
|
||||
|
||||
assert_eq!(selected, vec![skill]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collect_explicit_skill_mentions_accepts_linked_discovery_path() {
|
||||
let skill = make_skill("linked-skill", "/tmp/shared/linked-skill/SKILL.md");
|
||||
let loaded_skills = skill_outcome_with_discovery_path(
|
||||
skill.clone(),
|
||||
"/tmp/project/.agents/skills/linked-skill/SKILL.md",
|
||||
);
|
||||
let inputs = vec![UserInput::Text {
|
||||
text: linked_skill_mention(
|
||||
"linked-skill",
|
||||
"/tmp/project/.agents/skills/linked-skill/SKILL.md",
|
||||
),
|
||||
text_elements: Vec::new(),
|
||||
}];
|
||||
|
||||
let selected = collect_explicit_skill_mentions(&inputs, &loaded_skills, &HashMap::new());
|
||||
|
||||
assert_eq!(selected, vec![skill]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collect_explicit_skill_mentions_rejects_disabled_discovery_path() {
|
||||
let skill = make_skill("linked-skill", "/tmp/shared/linked-skill/SKILL.md");
|
||||
let mut loaded_skills = skill_outcome_with_discovery_path(
|
||||
skill.clone(),
|
||||
"/tmp/project/.agents/skills/linked-skill/SKILL.md",
|
||||
);
|
||||
loaded_skills.disabled_paths.insert(skill.path_to_skills_md);
|
||||
let inputs = vec![UserInput::Skill {
|
||||
name: "linked-skill".to_string(),
|
||||
path: test_path_buf("/tmp/project/.agents/skills/linked-skill/SKILL.md"),
|
||||
}];
|
||||
|
||||
let selected = collect_explicit_skill_mentions(&inputs, &loaded_skills, &HashMap::new());
|
||||
|
||||
assert_eq!(selected, Vec::new());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collect_explicit_skill_mentions_skips_invalid_structured_and_blocks_plain_fallback() {
|
||||
let alpha = make_skill("alpha-skill", "/tmp/alpha");
|
||||
|
||||
@@ -39,6 +39,7 @@ use futures::FutureExt;
|
||||
use futures::StreamExt;
|
||||
use namespace::SkillNamespaceResolver;
|
||||
use serde::Deserialize;
|
||||
use std::collections::HashMap;
|
||||
use std::error::Error;
|
||||
use std::fmt;
|
||||
use std::io;
|
||||
@@ -165,6 +166,7 @@ pub(crate) struct SkillRootSnapshot {
|
||||
pub(crate) root: AbsolutePathBuf,
|
||||
pub(crate) is_agent_plugin: bool,
|
||||
pub(crate) skills: Vec<SkillMetadata>,
|
||||
pub(crate) skill_discovery_path_by_path: Arc<HashMap<AbsolutePathBuf, AbsolutePathBuf>>,
|
||||
pub(crate) errors: Vec<SkillError>,
|
||||
pub(crate) file_system: Arc<dyn ExecutorFileSystem>,
|
||||
}
|
||||
@@ -178,6 +180,7 @@ pub(crate) async fn load_skill_root(root: SkillRoot) -> SkillRootSnapshot {
|
||||
root: canonical_root,
|
||||
is_agent_plugin: root.discovery_mode == SkillDiscoveryMode::DirectChildren,
|
||||
skills: outcome.skills,
|
||||
skill_discovery_path_by_path: outcome.skill_discovery_path_by_path,
|
||||
errors: outcome.errors,
|
||||
file_system: root.file_system,
|
||||
}
|
||||
@@ -324,6 +327,11 @@ async fn load_skills_under_root(
|
||||
.map(|skill| {
|
||||
let plugin_root = plugin_root.as_ref();
|
||||
async move {
|
||||
let discovery_path = skill
|
||||
.skill
|
||||
.path
|
||||
.to_abs_path()
|
||||
.unwrap_or_else(|_| skill.path.clone());
|
||||
let result = parse_skill_file(
|
||||
fs,
|
||||
&skill.skill,
|
||||
@@ -335,14 +343,14 @@ async fn load_skills_under_root(
|
||||
)
|
||||
.await
|
||||
.map_err(|err| err.to_string());
|
||||
(skill.path, skill.path_uri, result)
|
||||
(skill.path, skill.path_uri, discovery_path, result)
|
||||
}
|
||||
})
|
||||
.buffered(MAX_CONCURRENT_SKILL_LOADS)
|
||||
.collect::<Vec<_>>()
|
||||
.boxed();
|
||||
let (namespace_resolver, skill_results) = tokio::join!(namespace_resolver, skill_results);
|
||||
for (path, path_uri, result) in skill_results {
|
||||
for (path, path_uri, discovery_path, result) in skill_results {
|
||||
let result = result.and_then(|mut skill| {
|
||||
skill.name = namespace_resolver
|
||||
.for_skill(&root_uri, &path_uri)
|
||||
@@ -352,7 +360,11 @@ async fn load_skills_under_root(
|
||||
Ok(skill)
|
||||
});
|
||||
match result {
|
||||
Ok(skill) => outcome.skills.push(skill),
|
||||
Ok(skill) => {
|
||||
Arc::make_mut(&mut outcome.skill_discovery_path_by_path)
|
||||
.insert(skill.path_to_skills_md.clone(), discovery_path);
|
||||
outcome.skills.push(skill);
|
||||
}
|
||||
Err(err) if skill_root.scope != SkillScope::System => {
|
||||
outcome.errors.push(SkillError { path, message: err })
|
||||
}
|
||||
|
||||
@@ -623,6 +623,21 @@ async fn loads_skills_via_symlinked_subdir_for_user_scope() {
|
||||
remote_plugin_id: None,
|
||||
}]
|
||||
);
|
||||
let canonical_skill_path = normalized(&shared_skill_path);
|
||||
let discovery_path = outcome
|
||||
.skill_root_for_path(&canonical_skill_path)
|
||||
.expect("symlinked skill should retain its discovery root")
|
||||
.join("shared/demo/SKILL.md");
|
||||
assert_eq!(
|
||||
outcome.skill_discovery_path_for_path(&canonical_skill_path),
|
||||
Some(&discovery_path)
|
||||
);
|
||||
let filtered_outcome =
|
||||
crate::filter_skill_load_outcome_for_product(outcome, Some(Product::Codex));
|
||||
assert_eq!(
|
||||
filtered_outcome.skill_discovery_path_for_path(&canonical_skill_path),
|
||||
Some(&discovery_path)
|
||||
);
|
||||
}
|
||||
|
||||
// Directory symlinks on Windows can require Developer Mode or administrator privileges.
|
||||
@@ -795,6 +810,15 @@ async fn loads_skills_via_symlinked_subdir_for_repo_scope() {
|
||||
remote_plugin_id: None,
|
||||
}]
|
||||
);
|
||||
let canonical_skill_path = normalized(&linked_skill_path);
|
||||
let discovery_path = outcome
|
||||
.skill_root_for_path(&canonical_skill_path)
|
||||
.expect("repo skill should retain its discovery root")
|
||||
.join("shared/demo/SKILL.md");
|
||||
assert_eq!(
|
||||
outcome.skill_discovery_path_for_path(&canonical_skill_path),
|
||||
Some(&discovery_path)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -1880,6 +1904,41 @@ async fn deduplicates_by_path_preferring_first_root() {
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn deduplicated_symlinked_skill_preserves_first_discovery_path() {
|
||||
let source_root = tempfile::tempdir().expect("source tempdir");
|
||||
let first_root = tempfile::tempdir().expect("first tempdir");
|
||||
let second_root = tempfile::tempdir().expect("second tempdir");
|
||||
let skill_path = write_skill_at(source_root.path(), "demo", "demo", "shared skill");
|
||||
symlink_dir(
|
||||
&source_root.path().join("demo"),
|
||||
&first_root.path().join("first-link"),
|
||||
);
|
||||
symlink_dir(
|
||||
&source_root.path().join("demo"),
|
||||
&second_root.path().join("second-link"),
|
||||
);
|
||||
|
||||
let outcome = load_skills_for_test([
|
||||
local_skill_root(first_root.path(), SkillScope::Repo),
|
||||
local_skill_root(second_root.path(), SkillScope::User),
|
||||
])
|
||||
.await;
|
||||
let canonical_skill_path = normalized(&skill_path);
|
||||
let expected_root = normalized(first_root.path());
|
||||
let expected_discovery_path = expected_root.join("first-link/SKILL.md");
|
||||
|
||||
assert_eq!(outcome.skills.len(), 1);
|
||||
assert_eq!(
|
||||
(
|
||||
outcome.skill_root_for_path(&canonical_skill_path),
|
||||
outcome.skill_discovery_path_for_path(&canonical_skill_path),
|
||||
),
|
||||
(Some(&expected_root), Some(&expected_discovery_path))
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn keeps_duplicate_names_from_repo_and_user() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
|
||||
@@ -28,6 +28,7 @@ pub struct SkillLoadOutcome {
|
||||
pub disabled_paths: HashSet<AbsolutePathBuf>,
|
||||
pub(crate) skill_roots: Vec<AbsolutePathBuf>,
|
||||
pub(crate) skill_root_by_path: Arc<HashMap<AbsolutePathBuf, AbsolutePathBuf>>,
|
||||
pub(crate) skill_discovery_path_by_path: Arc<HashMap<AbsolutePathBuf, AbsolutePathBuf>>,
|
||||
pub(crate) agent_plugin_skill_paths: HashSet<AbsolutePathBuf>,
|
||||
pub(crate) file_systems_by_skill_path: SkillFileSystemsByPath,
|
||||
pub(crate) implicit_skills_by_scripts_dir: Arc<HashMap<AbsolutePathBuf, SkillMetadata>>,
|
||||
@@ -81,6 +82,14 @@ impl SkillLoadOutcome {
|
||||
self.skill_root_by_path.get(path)
|
||||
}
|
||||
|
||||
/// Returns the logical path used to discover a canonical skill path.
|
||||
pub fn skill_discovery_path_for_path(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
) -> Option<&AbsolutePathBuf> {
|
||||
self.skill_discovery_path_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()
|
||||
@@ -179,6 +188,14 @@ pub fn filter_skill_load_outcome_for_product(
|
||||
.map(|(path, root)| (path.clone(), root.clone()))
|
||||
.collect(),
|
||||
);
|
||||
outcome.skill_discovery_path_by_path = Arc::new(
|
||||
outcome
|
||||
.skill_discovery_path_by_path
|
||||
.iter()
|
||||
.filter(|(path, _)| retained_paths.contains(*path))
|
||||
.map(|(path, discovery_path)| (path.clone(), discovery_path.clone()))
|
||||
.collect(),
|
||||
);
|
||||
outcome
|
||||
.agent_plugin_skill_paths
|
||||
.retain(|path| retained_paths.contains(path));
|
||||
|
||||
@@ -145,6 +145,7 @@ fn merge_skill_root_snapshots(snapshots: Vec<SkillRootSnapshot>) -> SkillLoadOut
|
||||
let mut outcome = SkillLoadOutcome::default();
|
||||
let mut skill_roots = Vec::new();
|
||||
let mut skill_root_by_path = HashMap::new();
|
||||
let mut skill_discovery_path_by_path = HashMap::new();
|
||||
let mut file_systems_by_skill_path = HashMap::new();
|
||||
|
||||
for snapshot in snapshots {
|
||||
@@ -152,6 +153,7 @@ fn merge_skill_root_snapshots(snapshots: Vec<SkillRootSnapshot>) -> SkillLoadOut
|
||||
root,
|
||||
is_agent_plugin,
|
||||
skills,
|
||||
skill_discovery_path_by_path: discovery_paths,
|
||||
errors,
|
||||
file_system,
|
||||
} = snapshot;
|
||||
@@ -162,6 +164,9 @@ fn merge_skill_root_snapshots(snapshots: Vec<SkillRootSnapshot>) -> SkillLoadOut
|
||||
let path = skill.path_to_skills_md.clone();
|
||||
if !skill_root_by_path.contains_key(&path) {
|
||||
skill_root_by_path.insert(path.clone(), root.clone());
|
||||
if let Some(discovery_path) = discovery_paths.get(&path) {
|
||||
skill_discovery_path_by_path.insert(path.clone(), discovery_path.clone());
|
||||
}
|
||||
file_systems_by_skill_path.insert(path.clone(), Arc::clone(&file_system));
|
||||
if is_agent_plugin {
|
||||
outcome.agent_plugin_skill_paths.insert(path);
|
||||
@@ -182,6 +187,7 @@ fn merge_skill_root_snapshots(snapshots: Vec<SkillRootSnapshot>) -> SkillLoadOut
|
||||
.map(|skill| skill.path_to_skills_md.clone())
|
||||
.collect::<HashSet<_>>();
|
||||
skill_root_by_path.retain(|path, _| retained_skill_paths.contains(path));
|
||||
skill_discovery_path_by_path.retain(|path, _| retained_skill_paths.contains(path));
|
||||
let used_roots = skill_root_by_path.values().cloned().collect::<HashSet<_>>();
|
||||
skill_roots.retain(|root| used_roots.contains(root));
|
||||
file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(path));
|
||||
@@ -190,6 +196,7 @@ fn merge_skill_root_snapshots(snapshots: Vec<SkillRootSnapshot>) -> SkillLoadOut
|
||||
.retain(|path| retained_skill_paths.contains(path));
|
||||
outcome.skill_roots = skill_roots;
|
||||
outcome.skill_root_by_path = Arc::new(skill_root_by_path);
|
||||
outcome.skill_discovery_path_by_path = Arc::new(skill_discovery_path_by_path);
|
||||
outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path);
|
||||
|
||||
outcome.skills.sort_by(|a, b| {
|
||||
|
||||
@@ -691,12 +691,8 @@ async fn required_mcp_servers_for_input(
|
||||
};
|
||||
let skills_snapshot = turn_context.skills_snapshot();
|
||||
let skills_outcome = skills_snapshot.outcome();
|
||||
let mentioned_skills = collect_explicit_skill_mentions(
|
||||
user_input,
|
||||
&skills_outcome.skills,
|
||||
&skills_outcome.disabled_paths,
|
||||
&connector_slug_counts,
|
||||
);
|
||||
let mentioned_skills =
|
||||
collect_explicit_skill_mentions(user_input, skills_outcome, &connector_slug_counts);
|
||||
for skill in mentioned_skills {
|
||||
if let Some(dependencies) = skill.dependencies {
|
||||
required_servers.extend(
|
||||
@@ -771,12 +767,8 @@ async fn build_skills_and_plugins(
|
||||
.await?;
|
||||
let skill_name_counts_lower =
|
||||
build_skill_name_counts(&skills_outcome.skills, &skills_outcome.disabled_paths).1;
|
||||
let mentioned_skills = collect_explicit_skill_mentions(
|
||||
user_input,
|
||||
&skills_outcome.skills,
|
||||
&skills_outcome.disabled_paths,
|
||||
&connector_slug_counts,
|
||||
);
|
||||
let mentioned_skills =
|
||||
collect_explicit_skill_mentions(user_input, skills_outcome, &connector_slug_counts);
|
||||
maybe_prompt_and_install_mcp_dependencies(
|
||||
sess,
|
||||
turn_context,
|
||||
|
||||
@@ -3,14 +3,19 @@
|
||||
|
||||
use anyhow::Result;
|
||||
use codex_core::TurnInput;
|
||||
use codex_core::config::Config;
|
||||
use codex_exec_server::CreateDirectoryOptions;
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_extension_api::ExtensionRegistryBuilder;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_skills_extension::SkillsExtensionConfig;
|
||||
use codex_skills_extension::install;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use core_test_support::create_directory_symlink;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
use core_test_support::responses::ev_completed;
|
||||
use core_test_support::responses::ev_response_created;
|
||||
@@ -18,6 +23,7 @@ use core_test_support::responses::mount_sse_once;
|
||||
use core_test_support::responses::sse;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use core_test_support::skip_if_remote;
|
||||
use core_test_support::skip_if_target_windows;
|
||||
use core_test_support::test_codex::local_selections;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
@@ -135,6 +141,106 @@ async fn user_turn_includes_skill_instructions() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn user_turn_selects_symlinked_skill_by_advertised_discovery_path() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_remote!(
|
||||
Ok(()),
|
||||
"remote filesystems do not expose directory symlink creation"
|
||||
);
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let skill_body = "instructions from the canonical linked skill";
|
||||
let mut extensions = ExtensionRegistryBuilder::<Config>::new();
|
||||
install(&mut extensions, |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_workspace_setup(move |cwd, _fs| async move {
|
||||
let source_skill_dir = cwd.join("shared-skills/linked-demo");
|
||||
let discovery_root = cwd.join(".agents/skills");
|
||||
std::fs::create_dir_all(source_skill_dir.as_path())?;
|
||||
std::fs::create_dir_all(discovery_root.as_path())?;
|
||||
std::fs::write(
|
||||
source_skill_dir.join("SKILL.md"),
|
||||
format!(
|
||||
"---\nname: linked-demo\ndescription: Linked demo skill\n---\n\n{skill_body}\n"
|
||||
),
|
||||
)?;
|
||||
create_directory_symlink(
|
||||
source_skill_dir.as_path(),
|
||||
discovery_root.join("linked-demo").as_path(),
|
||||
);
|
||||
Ok(())
|
||||
});
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
let discovery_root = test.config.cwd.join(".agents/skills").canonicalize()?;
|
||||
let discovery_path = discovery_root.join("linked-demo/SKILL.md");
|
||||
let canonical_path = discovery_path.canonicalize()?;
|
||||
let discovery_path_display = discovery_path.display();
|
||||
let canonical_path_display = canonical_path.display();
|
||||
let mock = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("linked-skill-response"),
|
||||
ev_assistant_message("linked-skill-message", "done"),
|
||||
ev_completed("linked-skill-response"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
test.codex
|
||||
.try_start_turn_if_idle(vec"),
|
||||
text_elements: Vec::new(),
|
||||
},
|
||||
UserInput::Skill {
|
||||
name: "linked-demo".to_string(),
|
||||
path: discovery_path.to_path_buf(),
|
||||
},
|
||||
],
|
||||
client_id: Some("linked-skill-user-message".to_string()),
|
||||
}])
|
||||
.await
|
||||
.map_err(|error| {
|
||||
anyhow::anyhow!("linked skill input was rejected: {:?}", error.reason())
|
||||
})?;
|
||||
|
||||
core_test_support::wait_for_event(test.codex.as_ref(), |event| {
|
||||
matches!(event, codex_protocol::protocol::EventMsg::TurnComplete(_))
|
||||
})
|
||||
.await;
|
||||
|
||||
let request = mock.single_request();
|
||||
let developer_texts = request.message_input_texts("developer");
|
||||
let advertised_path = format!("(file: {discovery_path_display})");
|
||||
assert!(
|
||||
developer_texts
|
||||
.iter()
|
||||
.any(|text| text.contains(&advertised_path)),
|
||||
"expected symlink discovery path in the skill catalog, got {developer_texts:?}"
|
||||
);
|
||||
|
||||
let user_texts = request.message_input_texts("user");
|
||||
let canonical_identity = format!("<path>{canonical_path_display}</path>");
|
||||
assert!(
|
||||
user_texts.iter().any(|text| {
|
||||
text.contains("<skill>\n<name>linked-demo</name>")
|
||||
&& text.contains(&canonical_identity)
|
||||
&& text.contains(skill_body)
|
||||
}),
|
||||
"expected canonical skill instructions selected by discovery path, got {user_texts:?}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn idle_user_turn_includes_skill_instructions_in_the_first_request() -> Result<()> {
|
||||
skip_if_target_windows!(Ok(()), "requires native cross-OS skill paths");
|
||||
|
||||
@@ -106,6 +106,11 @@ fn catalog_from_outcome(outcome: &SkillLoadOutcome) -> SkillCatalog {
|
||||
|
||||
for (skill, enabled) in outcome.skills_with_enabled() {
|
||||
let mut entry = catalog_entry_from_skill(skill, enabled);
|
||||
if let Some(discovery_path) =
|
||||
outcome.skill_discovery_path_for_path(&skill.path_to_skills_md)
|
||||
{
|
||||
entry = entry.with_display_path(discovery_path.to_string_lossy().replace('\\', "/"));
|
||||
}
|
||||
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()) {
|
||||
|
||||
@@ -63,3 +63,53 @@ async fn host_catalog_entries_carry_their_render_metadata() -> Result<(), Box<dy
|
||||
std::fs::remove_dir_all(root.as_path())?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn host_catalog_preserves_symlinked_skill_discovery_paths()
|
||||
-> Result<(), Box<dyn std::error::Error>> {
|
||||
let root = tempfile::tempdir()?;
|
||||
let source = tempfile::tempdir()?;
|
||||
let source_skill_dir = source.path().join("linked-skill");
|
||||
std::fs::create_dir_all(&source_skill_dir)?;
|
||||
std::fs::write(
|
||||
source_skill_dir.join("SKILL.md"),
|
||||
"---\nname: linked-skill\ndescription: Linked skill.\n---\n# Linked skill\n",
|
||||
)?;
|
||||
std::os::unix::fs::symlink(&source_skill_dir, root.path().join("linked-skill"))?;
|
||||
|
||||
let root = AbsolutePathBuf::try_from(std::fs::canonicalize(root.path())?)?;
|
||||
let outcome = load_skills_from_roots(
|
||||
[SkillRoot {
|
||||
path: root.clone(),
|
||||
scope: SkillScope::User,
|
||||
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(1)),
|
||||
)
|
||||
.await;
|
||||
let catalog = catalog_from_outcome(&outcome);
|
||||
let canonical_path = std::fs::canonicalize(source_skill_dir.join("SKILL.md"))?;
|
||||
let discovery_path = root.join("linked-skill/SKILL.md");
|
||||
|
||||
assert_eq!(catalog.entries.len(), 1);
|
||||
assert_eq!(
|
||||
(
|
||||
catalog.entries[0].main_prompt.as_str(),
|
||||
catalog.entries[0].display_path.as_deref(),
|
||||
catalog.entries[0].display_path_root(),
|
||||
),
|
||||
(
|
||||
canonical_path.to_string_lossy().as_ref(),
|
||||
Some(discovery_path.to_string_lossy().as_ref()),
|
||||
Some(root.to_string_lossy().as_ref()),
|
||||
)
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -1615,6 +1615,16 @@ async fn host_catalog_compacts_shared_paths_under_budget_pressure() -> TestResul
|
||||
),
|
||||
)?;
|
||||
}
|
||||
#[cfg(unix)]
|
||||
{
|
||||
let source_skill_dir = test_root.join("shared-skills/linked-skill");
|
||||
std::fs::create_dir_all(&source_skill_dir)?;
|
||||
std::fs::write(
|
||||
source_skill_dir.join("SKILL.md"),
|
||||
"---\nname: linked-skill\ndescription: Linked skill.\n---\n# Linked skill\n",
|
||||
)?;
|
||||
std::os::unix::fs::symlink(&source_skill_dir, root.join("linked-skill"))?;
|
||||
}
|
||||
let root = AbsolutePathBuf::try_from(std::fs::canonicalize(root)?)?;
|
||||
let rendered_root = root.to_string_lossy().replace('\\', "/");
|
||||
let outcome = load_skills_from_roots(
|
||||
@@ -1632,7 +1642,7 @@ async fn host_catalog_compacts_shared_paths_under_budget_pressure() -> TestResul
|
||||
)
|
||||
.await;
|
||||
assert_eq!(outcome.errors, Vec::new());
|
||||
assert_eq!(outcome.skills.len(), 12);
|
||||
assert_eq!(outcome.skills.len(), 12 + usize::from(cfg!(unix)));
|
||||
|
||||
let mut builder = ExtensionRegistryBuilder::new();
|
||||
install(&mut builder, skills_extension_config);
|
||||
@@ -1687,6 +1697,8 @@ async fn host_catalog_compacts_shared_paths_under_budget_pressure() -> TestResul
|
||||
);
|
||||
assert!(catalog.contains("(file: r0/skill-00/SKILL.md)"));
|
||||
assert!(catalog.contains("(file: r0/skill-11/SKILL.md)"));
|
||||
#[cfg(unix)]
|
||||
assert!(catalog.contains("(file: r0/linked-skill/SKILL.md)"));
|
||||
assert!(!catalog.contains("additional skills omitted"));
|
||||
|
||||
std::fs::remove_dir_all(test_root)?;
|
||||
|
||||
Reference in New Issue
Block a user