openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-05 20:10:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

Move explicit skill selection into the skills crate (#37177)

Browse Source 
## What changed

- Add `ExplicitSkillLookup` to decouple explicit mention selection from the
  core skill-loading model.
- Export `collect_explicit_skill_mentions` from `codex-skills` and implement
  the lookup interface for `SkillLoadOutcome`.
- Move the selection tests into `codex-skills` while keeping the prompt-size
  boundary test with prompt injection.

GitOrigin-RevId: 21105af31c9b519c231c5ea02464da560705c293
This commit is contained in:
felixxia-oai
2026-08-05 23:23:21 +00:00
committed by copyberry
parent 6bb6e9045f
commit a3ebd19fa4
7 changed files with 268 additions and 188 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core-skills/src/injection.rs
+1 -168
View File
@@ -1,10 +1,8 @@
use std::collections::HashMap;
use std::collections::HashSet;
use std::sync::Arc;
use crate::SkillLoadOutcome;
use crate::SkillMetadata;
use crate::build_skill_name_counts;
use codex_analytics::AnalyticsEventsClient;
use codex_analytics::InvocationType;
use codex_analytics::SkillInvocation;
@@ -12,7 +10,6 @@ use codex_analytics::TrackEventsContext;
use codex_exec_server::LOCAL_FS;
use codex_otel::SessionTelemetry;
use codex_otel::sanitize_metric_tag_value;
use codex_protocol::user_input::UserInput;
pub use codex_skills::ToolMentionKind;
pub use codex_skills::ToolMentions;
pub use codex_skills::app_id_from_path;
@@ -21,7 +18,6 @@ pub use codex_skills::extract_tool_mentions_with_sigil;
pub use codex_skills::normalize_skill_path;
pub use codex_skills::plugin_config_name_from_path;
pub use codex_skills::tool_kind_for_path;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
use codex_utils_string::take_bytes_at_char_boundary;
@@ -177,169 +173,6 @@ fn emit_skill_injected_metric(
);
}
/// Collect explicitly mentioned skills from structured and text mentions.
///
/// 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 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],
loaded_skills: &SkillLoadOutcome,
connector_slug_counts: &HashMap<String, usize>,
) -> Vec<SkillMetadata> {
let skill_name_counts =
build_skill_name_counts(&loaded_skills.skills, &loaded_skills.disabled_paths).0;
let selection_context = SkillSelectionContext {
loaded_skills,
skill_name_counts: &skill_name_counts,
connector_slug_counts,
};
let mut selected: Vec<SkillMetadata> = Vec::new();
let mut seen_names: HashSet<String> = HashSet::new();
let mut seen_paths: HashSet<AbsolutePathBuf> = HashSet::new();
let mut blocked_plain_names: HashSet<String> = HashSet::new();
for input in inputs {
if let UserInput::Skill { name, path, .. } = input {
blocked_plain_names.insert(name.clone());
let Ok(path) = AbsolutePathBuf::relative_to_current_dir(path) else {
continue;
};
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;
}
seen_paths.insert(skill.path_to_skills_md.clone());
seen_names.insert(skill.name.clone());
selected.push(skill.clone());
}
}
for input in inputs {
if let UserInput::Text { text, .. } = input {
let mentioned_names = extract_tool_mentions(text);
select_skills_from_mentions(
&selection_context,
&blocked_plain_names,
&mentioned_names,
&mut seen_names,
&mut seen_paths,
&mut selected,
);
}
}
selected
}
struct SkillSelectionContext<'a> {
loaded_skills: &'a SkillLoadOutcome,
skill_name_counts: &'a HashMap<String, usize>,
connector_slug_counts: &'a HashMap<String, usize>,
}
/// Select mentioned skills while preserving the order of `skills`.
fn select_skills_from_mentions(
selection_context: &SkillSelectionContext<'_>,
blocked_plain_names: &HashSet<String>,
mentions: &ToolMentions<'_>,
seen_names: &mut HashSet<String>,
seen_paths: &mut HashSet<AbsolutePathBuf>,
selected: &mut Vec<SkillMetadata>,
) {
if mentions.is_empty() {
return;
}
let mention_skill_paths: HashSet<String> = mentions
.paths()
.filter(|path| {
!matches!(
tool_kind_for_path(path),
ToolMentionKind::App | ToolMentionKind::Mcp | ToolMentionKind::Plugin
)
})
.map(normalize_host_skill_path)
.collect();
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 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.loaded_skills.skills {
if !selection_context.loaded_skills.is_skill_enabled(skill)
|| seen_paths.contains(&skill.path_to_skills_md)
{
continue;
}
if blocked_plain_names.contains(skill.name.as_str()) {
continue;
}
if !mentions.contains_plain_name(skill.name.as_str()) {
continue;
}
let skill_count = selection_context
.skill_name_counts
.get(skill.name.as_str())
.copied()
.unwrap_or(0);
let connector_count = selection_context
.connector_slug_counts
.get(&skill.name.to_ascii_lowercase())
.copied()
.unwrap_or(0);
if skill_count != 1 || connector_count != 0 {
continue;
}
if seen_names.insert(skill.name.clone()) {
seen_paths.insert(skill.path_to_skills_md.clone());
selected.push(skill.clone());
}
}
}
#[cfg(test)]
#[path = "injection_tests.rs"]
#[path = "prompt_injection_tests.rs"]
mod tests;
codex-rs/core-skills/src/model.rs
+18
View File
@@ -131,6 +131,24 @@ impl codex_skills::ImplicitSkillLookup for SkillLoadOutcome {
}
}
impl codex_skills::ExplicitSkillLookup for SkillLoadOutcome {
fn skills(&self) -> &[SkillMetadata] {
&self.skills
}
fn disabled_paths(&self) -> &HashSet<AbsolutePathBuf> {
&self.disabled_paths
}
fn skill_discovery_path_for_path(&self, path: &AbsolutePathBuf) -> Option<&AbsolutePathBuf> {
SkillLoadOutcome::skill_discovery_path_for_path(self, path)
}
fn is_skill_enabled(&self, skill: &SkillMetadata) -> bool {
SkillLoadOutcome::is_skill_enabled(self, skill)
}
}
#[derive(Clone, Default)]
pub(crate) struct SkillFileSystemsByPath {
values: Arc<HashMap<AbsolutePathBuf, Arc<dyn ExecutorFileSystem>>>,
codex-rs/core-skills/src/prompt_injection_tests.rs
+14
View File
@@ -0,0 +1,14 @@
use pretty_assertions::assert_eq;
use super::MAX_SKILL_PROMPT_BYTES;
use super::bounded_skill_prompt_contents;
#[test]
fn skill_prompt_contents_are_bounded_at_utf8_boundaries() {
let contents = format!("{}é", "a".repeat(MAX_SKILL_PROMPT_BYTES - 1));
let (bounded, truncated) = bounded_skill_prompt_contents(&contents);
assert_eq!(bounded.len(), MAX_SKILL_PROMPT_BYTES - 1);
assert_eq!(truncated, true);
}
codex-rs/core/src/skills.rs
+1 -1
View File
@@ -22,12 +22,12 @@ pub use codex_core_skills::filter_skill_load_outcome_for_product;
pub use codex_core_skills::injection;
pub use codex_core_skills::injection::SkillInjections;
pub use codex_core_skills::injection::build_skill_injections;
pub use codex_core_skills::injection::collect_explicit_skill_mentions;
pub use codex_core_skills::loader;
pub use codex_core_skills::model;
pub use codex_core_skills::remote;
pub use codex_skills::SkillMetadata;
pub use codex_skills::SkillPolicy;
pub use codex_skills::collect_explicit_skill_mentions;
pub use codex_skills_extension::HostSkillsLoadInput;
pub use codex_skills_extension::HostSkillsService;
pub use codex_skills_extension::bundled_skills_enabled_from_stack;
codex-rs/skills/src/lib.rs
+3
View File
@@ -5,6 +5,7 @@ mod model;
mod name_counts;
mod parser;
mod policy;
mod selection;
pub use interface::SkillInterfaceAssetPolicy;
pub use interface::SkillInterfaceFile;
@@ -33,6 +34,8 @@ pub use parser::ParsedSkillFrontmatter;
pub use parser::SkillParseError;
pub use parser::parse_skill_frontmatter_metadata;
pub use policy::resolve_disabled_skill_paths;
pub use selection::ExplicitSkillLookup;
pub use selection::collect_explicit_skill_mentions;
use codex_utils_absolute_path::AbsolutePathBuf;
use include_dir::Dir;
codex-rs/skills/src/selection.rs
+205
View File
@@ -0,0 +1,205 @@
use std::collections::HashMap;
use std::collections::HashSet;
use codex_protocol::user_input::UserInput;
use codex_utils_absolute_path::AbsolutePathBuf;
use crate::SkillMetadata;
use crate::ToolMentionKind;
use crate::ToolMentions;
use crate::build_skill_name_counts;
use crate::extract_tool_mentions;
use crate::normalize_skill_path;
use crate::tool_kind_for_path;
/// Supplies ordered skills, disabled identities, and discovery paths for explicit selection.
///
/// Implementations should preserve skill discovery order and expose the logical discovery path
/// associated with each canonical skill identity when one is available.
pub trait ExplicitSkillLookup {
fn skills(&self) -> &[SkillMetadata];
fn disabled_paths(&self) -> &HashSet<AbsolutePathBuf>;
fn skill_discovery_path_for_path(&self, path: &AbsolutePathBuf) -> Option<&AbsolutePathBuf>;
fn is_skill_enabled(&self, skill: &SkillMetadata) -> bool {
!self.disabled_paths().contains(&skill.path_to_skills_md)
}
}
/// Collect explicitly mentioned skills from structured and text mentions.
///
/// 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 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],
loaded_skills: &impl ExplicitSkillLookup,
connector_slug_counts: &HashMap<String, usize>,
) -> Vec<SkillMetadata> {
let skill_name_counts =
build_skill_name_counts(loaded_skills.skills(), loaded_skills.disabled_paths()).0;
let selection_context = SkillSelectionContext {
loaded_skills,
skill_name_counts: &skill_name_counts,
connector_slug_counts,
};
let mut selected: Vec<SkillMetadata> = Vec::new();
let mut seen_names: HashSet<String> = HashSet::new();
let mut seen_paths: HashSet<AbsolutePathBuf> = HashSet::new();
let mut blocked_plain_names: HashSet<String> = HashSet::new();
for input in inputs {
if let UserInput::Skill { name, path, .. } = input {
blocked_plain_names.insert(name.clone());
let Ok(path) = AbsolutePathBuf::relative_to_current_dir(path) else {
continue;
};
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;
}
seen_paths.insert(skill.path_to_skills_md.clone());
seen_names.insert(skill.name.clone());
selected.push(skill.clone());
}
}
for input in inputs {
if let UserInput::Text { text, .. } = input {
let mentioned_names = extract_tool_mentions(text);
select_skills_from_mentions(
&selection_context,
&blocked_plain_names,
&mentioned_names,
&mut seen_names,
&mut seen_paths,
&mut selected,
);
}
}
selected
}
struct SkillSelectionContext<'a> {
loaded_skills: &'a dyn ExplicitSkillLookup,
skill_name_counts: &'a HashMap<String, usize>,
connector_slug_counts: &'a HashMap<String, usize>,
}
/// Select mentioned skills while preserving the order of `skills`.
fn select_skills_from_mentions(
selection_context: &SkillSelectionContext<'_>,
blocked_plain_names: &HashSet<String>,
mentions: &ToolMentions<'_>,
seen_names: &mut HashSet<String>,
seen_paths: &mut HashSet<AbsolutePathBuf>,
selected: &mut Vec<SkillMetadata>,
) {
if mentions.is_empty() {
return;
}
let mention_skill_paths: HashSet<String> = mentions
.paths()
.filter(|path| {
!matches!(
tool_kind_for_path(path),
ToolMentionKind::App | ToolMentionKind::Mcp | ToolMentionKind::Plugin
)
})
.map(normalize_host_skill_path)
.collect();
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 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.loaded_skills.skills() {
if !selection_context.loaded_skills.is_skill_enabled(skill)
|| seen_paths.contains(&skill.path_to_skills_md)
{
continue;
}
if blocked_plain_names.contains(skill.name.as_str()) {
continue;
}
if !mentions.contains_plain_name(skill.name.as_str()) {
continue;
}
let skill_count = selection_context
.skill_name_counts
.get(skill.name.as_str())
.copied()
.unwrap_or(0);
let connector_count = selection_context
.connector_slug_counts
.get(&skill.name.to_ascii_lowercase())
.copied()
.unwrap_or(0);
if skill_count != 1 || connector_count != 0 {
continue;
}
if seen_names.insert(skill.name.clone()) {
seen_paths.insert(skill.path_to_skills_md.clone());
selected.push(skill.clone());
}
}
}
fn normalize_host_skill_path(path: &str) -> String {
normalize_skill_path(path).replace('\\', "/")
}
#[cfg(test)]
#[path = "selection_tests.rs"]
mod tests;
codex-rs/core-skills/src/injection_tests.rs → codex-rs/skills/src/selection_tests.rs
+26 -19
View File
@@ -5,7 +5,27 @@ 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;
#[derive(Default)]
struct TestLookup {
skills: Vec<SkillMetadata>,
disabled_paths: HashSet<AbsolutePathBuf>,
skill_discovery_path_by_path: HashMap<AbsolutePathBuf, AbsolutePathBuf>,
}
impl ExplicitSkillLookup for TestLookup {
fn skills(&self) -> &[SkillMetadata] {
&self.skills
}
fn disabled_paths(&self) -> &HashSet<AbsolutePathBuf> {
&self.disabled_paths
}
fn skill_discovery_path_for_path(&self, path: &AbsolutePathBuf) -> Option<&AbsolutePathBuf> {
self.skill_discovery_path_by_path.get(path)
}
}
fn make_skill(name: &str, path: &str) -> SkillMetadata {
SkillMetadata {
@@ -22,16 +42,6 @@ fn make_skill(name: &str, path: &str) -> SkillMetadata {
}
}
#[test]
fn skill_prompt_contents_are_bounded_at_utf8_boundaries() {
let contents = format!("{}é", "a".repeat(MAX_SKILL_PROMPT_BYTES - 1));
let (bounded, truncated) = bounded_skill_prompt_contents(&contents);
assert_eq!(bounded.len(), MAX_SKILL_PROMPT_BYTES - 1);
assert_eq!(truncated, true);
}
fn linked_skill_mention(name: &str, unix_path: &str) -> String {
format!("[${name}]({})", test_path_buf(unix_path).display())
}
@@ -42,7 +52,7 @@ fn collect_mentions(
disabled_paths: &HashSet<AbsolutePathBuf>,
connector_slug_counts: &HashMap<String, usize>,
) -> Vec<SkillMetadata> {
let loaded_skills = SkillLoadOutcome {
let loaded_skills = TestLookup {
skills: skills.to_vec(),
disabled_paths: disabled_paths.clone(),
..Default::default()
@@ -50,15 +60,12 @@ fn collect_mentions(
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([(
fn skill_outcome_with_discovery_path(skill: SkillMetadata, discovery_path: &str) -> TestLookup {
TestLookup {
skill_discovery_path_by_path: HashMap::from([(
skill.path_to_skills_md.clone(),
test_path_buf(discovery_path).abs(),
)])),
)]),
skills: vec![skill],
..Default::default()
}