From 1276eb659a660c8d20d5e88c93ad23ebde58d256 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Thu, 23 Jul 2026 18:13:04 +0300 Subject: [PATCH] feat: Deferred skills + skill search: load only a few top skills at start, update the system prompt to look up skills (scale t --- codex-rs/Cargo.lock | 2 + codex-rs/Cargo.toml | 1 + codex-rs/app-server/Cargo.toml | 1 + codex-rs/app-server/src/extensions.rs | 1 + codex-rs/core-skills/src/render.rs | 95 ++++++-- codex-rs/ext/skills/src/catalog.rs | 10 +- codex-rs/ext/skills/src/extension.rs | 6 +- codex-rs/ext/skills/src/lib.rs | 1 + codex-rs/ext/skills/src/ranking.rs | 225 ++++++++++++++++++ codex-rs/ext/skills/src/render.rs | 64 +++-- codex-rs/ext/skills/src/sources.rs | 37 +-- codex-rs/ext/skills/src/tools/list.rs | 155 ++++++++++++ codex-rs/ext/skills/src/tools/mod.rs | 4 + codex-rs/ext/skills/tests/skills_extension.rs | 130 ++++++++-- codex-rs/mcp-server/Cargo.toml | 1 + codex-rs/mcp-server/src/message_processor.rs | 6 +- 16 files changed, 636 insertions(+), 103 deletions(-) create mode 100644 codex-rs/ext/skills/src/ranking.rs create mode 100644 codex-rs/ext/skills/src/tools/list.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 76a74c4411..bdbb420ff1 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1854,6 +1854,7 @@ dependencies = [ "codex-rollout", "codex-sandboxing", "codex-shell-command", + "codex-skills-extension", "codex-state", "codex-thread-store", "codex-tools", @@ -3166,6 +3167,7 @@ dependencies = [ "codex-login", "codex-protocol", "codex-shell-command", + "codex-skills-extension", "codex-utils-absolute-path", "codex-utils-cli", "codex-utils-json-to-toml", diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index a9175c38c2..a411f0d15f 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -221,6 +221,7 @@ codex-secrets = { path = "secrets" } codex-shell-command = { path = "shell-command" } codex-shell-escalation = { path = "shell-escalation" } codex-skills = { path = "skills" } +codex-skills-extension = { path = "ext/skills" } codex-state = { path = "state" } codex-stdio-to-uds = { path = "stdio-to-uds" } codex-terminal-detection = { path = "terminal-detection" } diff --git a/codex-rs/app-server/Cargo.toml b/codex-rs/app-server/Cargo.toml index aa42c6ef18..3db1acc1bb 100644 --- a/codex-rs/app-server/Cargo.toml +++ b/codex-rs/app-server/Cargo.toml @@ -51,6 +51,7 @@ codex-hooks = { workspace = true } codex-otel = { workspace = true } codex-plugin = { workspace = true } codex-shell-command = { workspace = true } +codex-skills-extension = { workspace = true } codex-utils-cli = { workspace = true } codex-utils-pty = { workspace = true } codex-backend-client = { workspace = true } diff --git a/codex-rs/app-server/src/extensions.rs b/codex-rs/app-server/src/extensions.rs index d5bde84551..3161c8ab09 100644 --- a/codex-rs/app-server/src/extensions.rs +++ b/codex-rs/app-server/src/extensions.rs @@ -85,6 +85,7 @@ where } codex_guardian::install(&mut builder, guardian_agent_spawner); codex_memories_extension::install(&mut builder, codex_otel::global()); + codex_skills_extension::install(&mut builder); codex_web_search_extension::install(&mut builder, auth_manager.clone()); codex_image_generation_extension::install(&mut builder, auth_manager); codex_workflows_extension::install(&mut builder, workflow_state_db, |config: &Config| { diff --git a/codex-rs/core-skills/src/render.rs b/codex-rs/core-skills/src/render.rs index a31f80b670..65303bd7b1 100644 --- a/codex-rs/core-skills/src/render.rs +++ b/codex-rs/core-skills/src/render.rs @@ -16,17 +16,18 @@ use codex_utils_output_truncation::approx_token_count; const DEFAULT_SKILL_METADATA_CHAR_BUDGET: usize = 8_000; const SKILL_METADATA_CONTEXT_WINDOW_PERCENT: usize = 2; +const MAX_STARTER_SKILLS: usize = 5; const SKILL_DESCRIPTION_TRUNCATION_WARNING_THRESHOLD_CHARS: usize = 100; const APPROX_BYTES_PER_TOKEN: usize = 4; -pub const SKILL_DESCRIPTION_TRUNCATED_WARNING: &str = "Skill descriptions were shortened to fit the skills context budget. Codewith can still see every skill, but some descriptions are shorter. Disable unused skills or plugins to leave more room for the rest."; -pub const SKILL_DESCRIPTION_TRUNCATED_WARNING_WITH_PERCENT: &str = "Skill descriptions were shortened to fit the 2% skills context budget. Codewith can still see every skill, but some descriptions are shorter. Disable unused skills or plugins to leave more room for the rest."; +pub const SKILL_DESCRIPTION_TRUNCATED_WARNING: &str = "Starter skill descriptions were shortened to fit the skills context budget. The full catalog remains searchable, but some starter descriptions are shorter."; +pub const SKILL_DESCRIPTION_TRUNCATED_WARNING_WITH_PERCENT: &str = "Starter skill descriptions were shortened to fit the 2% skills context budget. The full catalog remains searchable, but some starter descriptions are shorter."; pub const SKILL_DESCRIPTIONS_REMOVED_WARNING_PREFIX: &str = "Exceeded skills context budget. All skill descriptions were removed and"; -pub const SKILLS_INTRO_WITH_ABSOLUTE_PATHS: &str = "A skill is a set of local instructions to follow that is stored in a `SKILL.md` file. Below is the list of skills that can be used. Each entry includes a name, description, and file path so you can open the source for full instructions when using a specific skill."; -pub const SKILLS_INTRO_WITH_ALIASES: &str = "A skill is a set of local instructions to follow that is stored in a `SKILL.md` file. Below is the list of skills that can be used. Each entry includes a name, description, and a short path that can be expanded into an absolute path using the skill roots table."; -pub const SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS: &str = r###"- Discovery: The list above is the skills available in this session (name + description + file path). Skill bodies live on disk at the listed paths. -- Trigger rules: If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a skill's description shown above, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned. -- Missing/blocked: If a named skill isn't in the list or the path can't be read, say so briefly and continue with the best fallback. +pub const SKILLS_INTRO_WITH_ABSOLUTE_PATHS: &str = "A skill is a set of local instructions to follow that is stored in a `SKILL.md` file. The list below is a small starter or task-relevant subset, not the complete skills catalog. Each entry includes a name, description, and file path so you can open the source for full instructions when using a specific skill."; +pub const SKILLS_INTRO_WITH_ALIASES: &str = "A skill is a set of local instructions to follow that is stored in a `SKILL.md` file. The list below is a small starter or task-relevant subset, not the complete skills catalog. Each entry includes a name, description, and a short path that can be expanded into an absolute path using the skill roots table."; +pub const SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS: &str = r###"- Discovery: Always consider whether a skill is relevant before acting. The list above is only a starter or task-relevant subset (name + description + file path); skill bodies live on disk at the listed paths. When the available tools include `skills.list`, use it to search the full catalog by task, then use `skills.read` with the returned opaque handles when needed. +- Trigger rules: Explicit skill mentions win. If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a listed or discovered skill's description, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned. +- Missing/blocked: If a named skill isn't listed, search with `skills.list` when available. If it cannot be found or read, say so briefly and continue with the best fallback. - How to use a skill (progressive disclosure): 1) After deciding to use a skill, open its `SKILL.md`. Read only enough to follow the workflow. 2) When `SKILL.md` references relative paths (e.g., `scripts/foo.py`), resolve them relative to the skill directory listed above first, and only consider other paths if needed. @@ -41,9 +42,9 @@ pub const SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS: &str = r###"- Discovery: The li - Avoid deep reference-chasing: prefer opening only files directly linked from `SKILL.md` unless you're blocked. - When variants exist (frameworks, providers, domains), pick only the relevant reference file(s) and note that choice. - Safety and fallback: If a skill can't be applied cleanly (missing files, unclear instructions), state the issue, pick the next-best approach, and continue."###; -pub const SKILLS_HOW_TO_USE_WITH_ALIASES: &str = r###"- Discovery: The list above is the skills available in this session (name + description + short path). Skill bodies live on disk at the listed paths after expanding the matching alias from `### Skill roots`. -- Trigger rules: If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a skill's description shown above, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned. -- Missing/blocked: If a named skill isn't in the list or the path can't be read, say so briefly and continue with the best fallback. +pub const SKILLS_HOW_TO_USE_WITH_ALIASES: &str = r###"- Discovery: Always consider whether a skill is relevant before acting. The list above is only a starter or task-relevant subset (name + description + short path); skill bodies live on disk at the listed paths after expanding the matching alias from `### Skill roots`. When the available tools include `skills.list`, use it to search the full catalog by task, then use `skills.read` with the returned opaque handles when needed. +- Trigger rules: Explicit skill mentions win. If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a listed or discovered skill's description, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned. +- Missing/blocked: If a named skill isn't listed, search with `skills.list` when available. If it cannot be found or read, say so briefly and continue with the best fallback. - How to use a skill (progressive disclosure): 1) After deciding to use a skill, expand the listed short `path` with the matching alias from `### Skill roots`, then open its `SKILL.md`. Read only enough to follow the workflow. 2) When `SKILL.md` references relative paths (e.g., `scripts/foo.py`), resolve them relative to the directory containing that expanded `SKILL.md` first, and only consider other paths if needed. @@ -119,6 +120,7 @@ fn approx_token_count_from_bytes(bytes: usize) -> usize { pub struct SkillRenderReport { pub total_count: usize, pub included_count: usize, + pub deferred_count: usize, pub omitted_count: usize, pub truncated_description_chars: usize, pub truncated_description_count: usize, @@ -162,7 +164,13 @@ pub fn build_available_skills( budget: SkillMetadataBudget, side_effects: SkillRenderSideEffects<'_>, ) -> Option { - let skills = outcome.allowed_skills_for_implicit_invocation(); + let all_skills = outcome.allowed_skills_for_implicit_invocation(); + let total_count = all_skills.len(); + let skills = ordered_skills_for_budget(&all_skills) + .into_iter() + .take(MAX_STARTER_SKILLS) + .cloned() + .collect::>(); if skills.is_empty() { record_skill_render_side_effects( side_effects, @@ -182,7 +190,7 @@ pub fn build_available_skills( SkillPathAliases::default(), )?; - let selected = + let mut selected = if absolute.report.omitted_count == 0 && absolute.report.truncated_description_chars == 0 { absolute } else if let Some(aliased) = build_aliased_available_skills(outcome, &skills, budget) { @@ -195,6 +203,8 @@ pub fn build_available_skills( absolute }; + selected.report.total_count = total_count; + selected.report.deferred_count = total_count.saturating_sub(skills.len()); record_available_skills_side_effects(&selected, budget, side_effects); Some(selected) } @@ -426,6 +436,7 @@ fn skill_render_report( SkillRenderReport { total_count, included_count, + deferred_count: 0, omitted_count, truncated_description_chars, truncated_description_count, @@ -1073,7 +1084,7 @@ mod tests { assert_eq!( rendered.warning_message, Some( - "Skill descriptions were shortened to fit the skills context budget. Codewith can still see every skill, but some descriptions are shorter. Disable unused skills or plugins to leave more room for the rest." + "Starter skill descriptions were shortened to fit the skills context budget. The full catalog remains searchable, but some starter descriptions are shorter." .to_string() ) ); @@ -1203,6 +1214,45 @@ mod tests { assert_eq!(rendered.report.included_count, 2); } + #[test] + fn outcome_rendering_keeps_a_stable_five_skill_starter_at_large_catalog_scale() { + let skills = (0..2_105) + .rev() + .map(|index| { + let name = format!("scale-skill-{index:04}"); + skill_with_path( + &name, + &test_path_buf(&format!("/tmp/skills/{name}/SKILL.md")).abs(), + ) + }) + .collect::>(); + let outcome = outcome_with_roots(skills, Vec::new()); + + let rendered = build_available_skills( + &outcome, + SkillMetadataBudget::Characters(usize::MAX), + SkillRenderSideEffects::None, + ) + .expect("starter skills should render"); + + assert_eq!(rendered.report.total_count, 2_105); + assert_eq!(rendered.report.included_count, MAX_STARTER_SKILLS); + assert_eq!(rendered.report.deferred_count, 2_100); + assert_eq!(rendered.report.omitted_count, 0); + assert_eq!(rendered.warning_message, None); + let rendered_text = rendered.skill_lines.join("\n"); + for index in 0..MAX_STARTER_SKILLS { + assert!(rendered_text.contains(&format!("scale-skill-{index:04}"))); + } + assert!(!rendered_text.contains("scale-skill-0005")); + + let body = render_available_skills_body(&rendered.skill_root_lines, &rendered.skill_lines); + assert!(body.contains("not the complete skills catalog")); + assert!(body.contains("Always consider whether a skill is relevant before acting")); + assert!(body.contains("`skills.list`")); + assert!(body.contains("`skills.read`")); + } + #[test] fn outcome_rendering_uses_aliases_when_they_allow_more_skills_to_fit() { let root = test_path_buf( @@ -1216,18 +1266,23 @@ mod tests { }) .collect::>(); let outcome = outcome_with_roots(skills.clone(), vec![root]); - let absolute_minimum = skills.iter().fold(0usize, |cost, skill| { + let starter_skills = ordered_skills_for_budget(&skills) + .into_iter() + .take(MAX_STARTER_SKILLS) + .cloned() + .collect::>(); + let absolute_minimum = starter_skills.iter().fold(0usize, |cost, skill| { cost.saturating_add( SkillLine::new(skill).minimum_cost(SkillMetadataBudget::Characters(usize::MAX)), ) }); let plan = build_alias_plan( &outcome, - &skills, + &starter_skills, SkillMetadataBudget::Characters(usize::MAX), ) .expect("alias plan should build"); - let alias_minimum = skills.iter().fold(plan.table_cost, |cost, skill| { + let alias_minimum = starter_skills.iter().fold(plan.table_cost, |cost, skill| { cost.saturating_add( SkillLine::with_path(skill, render_skill_path_with_aliases(skill, &plan)) .minimum_cost(SkillMetadataBudget::Characters(usize::MAX)), @@ -1245,7 +1300,12 @@ mod tests { ) .expect("skills should render"); - assert_eq!(rendered.report.included_count, skills.len()); + assert_eq!(rendered.report.total_count, skills.len()); + assert_eq!(rendered.report.included_count, MAX_STARTER_SKILLS); + assert_eq!( + rendered.report.deferred_count, + skills.len() - MAX_STARTER_SKILLS + ); assert_eq!(rendered.report.omitted_count, 0); assert_eq!( rendered.skill_root_lines, @@ -1262,6 +1322,7 @@ mod tests { let rendered_text = rendered.skill_lines.join("\n"); assert!(rendered_text.contains("r0/skill-0/SKILL.md")); assert!(rendered_text.contains("r0/skill-11/SKILL.md")); + assert!(!rendered_text.contains("r0/skill-3/SKILL.md")); } #[test] diff --git a/codex-rs/ext/skills/src/catalog.rs b/codex-rs/ext/skills/src/catalog.rs index 2fd633bf2c..b1c4c7c105 100644 --- a/codex-rs/ext/skills/src/catalog.rs +++ b/codex-rs/ext/skills/src/catalog.rs @@ -1,4 +1,5 @@ use codex_core_skills::model::SkillDependencies; +use std::collections::HashSet; /// Source authority that owns a skill package and must be used to read it. #[derive(Clone, Debug, PartialEq, Eq, Hash)] @@ -160,8 +161,15 @@ pub struct SkillCatalog { impl SkillCatalog { pub fn extend(&mut self, other: SkillCatalog) { + let mut existing = self + .entries + .iter() + .map(|entry| (entry.authority.clone(), entry.id.clone())) + .collect::>(); for entry in other.entries { - self.push_entry(entry); + if existing.insert((entry.authority.clone(), entry.id.clone())) { + self.entries.push(entry); + } } self.warnings.extend(other.warnings); } diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index 154f57273c..6ec1e4d5fd 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -32,6 +32,8 @@ use crate::catalog::SkillSourceKind; use crate::provider::HostSkillProvider; use crate::provider::SkillListQuery; use crate::provider::SkillReadRequest; +use crate::ranking::DEFAULT_SKILL_MATCH_LIMIT; +use crate::ranking::user_text_query; use crate::render::available_skills_fragment; use crate::render::truncate_main_prompt_contents; use crate::selection::collect_explicit_skill_mentions; @@ -149,8 +151,10 @@ impl TurnInputContributor for SkillsExtension { let selected_entries = collect_explicit_skill_mentions(&input.user_input, &catalog); let mut fragments: Vec> = Vec::new(); + let user_query = user_text_query(&input.user_input); if config.include_instructions - && let Some(fragment) = available_skills_fragment(&catalog) + && let Some(fragment) = + available_skills_fragment(&catalog, &user_query, DEFAULT_SKILL_MATCH_LIMIT) { fragments.push(Box::new(fragment)); } diff --git a/codex-rs/ext/skills/src/lib.rs b/codex-rs/ext/skills/src/lib.rs index 404793f9ce..52a5ddeafb 100644 --- a/codex-rs/ext/skills/src/lib.rs +++ b/codex-rs/ext/skills/src/lib.rs @@ -1,6 +1,7 @@ pub mod catalog; mod extension; pub mod provider; +mod ranking; mod render; mod selection; mod sources; diff --git a/codex-rs/ext/skills/src/ranking.rs b/codex-rs/ext/skills/src/ranking.rs new file mode 100644 index 0000000000..3cb08f1dd1 --- /dev/null +++ b/codex-rs/ext/skills/src/ranking.rs @@ -0,0 +1,225 @@ +use std::collections::HashSet; + +use codex_protocol::user_input::UserInput; + +use crate::catalog::SkillCatalog; +use crate::catalog::SkillCatalogEntry; + +pub(crate) const DEFAULT_SKILL_MATCH_LIMIT: usize = 5; +pub(crate) const MAX_SKILL_MATCH_LIMIT: usize = 5; + +const MAX_QUERY_CHARS: usize = 4_096; +const MAX_QUERY_TERMS: usize = 128; +const MAX_METADATA_CHARS: usize = 4_096; + +pub(crate) fn user_text_query(inputs: &[UserInput]) -> String { + let mut query = String::new(); + for text in inputs.iter().filter_map(|input| match input { + UserInput::Text { text, .. } => Some(text.as_str()), + _ => None, + }) { + if !query.is_empty() { + query.push(' '); + } + let remaining = MAX_QUERY_CHARS.saturating_sub(query.chars().count()); + if remaining == 0 { + break; + } + query.extend(text.chars().take(remaining)); + } + query +} + +pub(crate) fn rank_catalog<'a>( + catalog: &'a SkillCatalog, + query: &str, + limit: usize, +) -> Vec<&'a SkillCatalogEntry> { + let query_lower = query.to_lowercase(); + let query_terms = lexical_terms(query, MAX_QUERY_CHARS, MAX_QUERY_TERMS); + if query_terms.is_empty() || limit == 0 { + return Vec::new(); + } + + let mut ranked = catalog + .entries + .iter() + .filter(|entry| entry.is_prompt_visible()) + .filter_map(|entry| { + relevance_score(entry, &query_lower, &query_terms).map(|score| (score, entry)) + }) + .collect::>(); + ranked.sort_by(|(left_score, left), (right_score, right)| { + right_score + .cmp(left_score) + .then_with(|| { + source_kind_key(&left.authority.kind).cmp(&source_kind_key(&right.authority.kind)) + }) + .then_with(|| left.authority.id.cmp(&right.authority.id)) + .then_with(|| left.id.0.cmp(&right.id.0)) + .then_with(|| left.name.cmp(&right.name)) + }); + ranked + .into_iter() + .take(limit) + .map(|(_, entry)| entry) + .collect() +} + +fn relevance_score( + entry: &SkillCatalogEntry, + query_lower: &str, + query_terms: &HashSet, +) -> Option { + let name_lower = entry.name.to_lowercase(); + let name_terms = lexical_terms(&entry.name, MAX_METADATA_CHARS, usize::MAX); + let description_terms = lexical_terms( + entry + .short_description + .as_deref() + .unwrap_or(entry.description.as_str()), + MAX_METADATA_CHARS, + usize::MAX, + ); + + let explicit_name_token = format!("${name_lower}"); + let explicit_name = query_lower + .split_whitespace() + .any(|part| part.trim_matches(non_name_char) == explicit_name_token) + || query_lower.trim() == name_lower; + let name_phrase = !name_lower.is_empty() && query_lower.contains(&name_lower); + let name_matches = query_terms.intersection(&name_terms).count(); + let description_matches = query_terms.intersection(&description_terms).count(); + if !explicit_name && !name_phrase && name_matches == 0 && description_matches == 0 { + return None; + } + + Some( + usize::from(explicit_name) + .saturating_mul(10_000) + .saturating_add(usize::from(name_phrase).saturating_mul(1_000)) + .saturating_add(name_matches.saturating_mul(100)) + .saturating_add(description_matches.saturating_mul(10)), + ) +} + +fn source_kind_key(kind: &crate::catalog::SkillSourceKind) -> (u8, &str) { + match kind { + crate::catalog::SkillSourceKind::Host => (0, ""), + crate::catalog::SkillSourceKind::Executor => (1, ""), + crate::catalog::SkillSourceKind::Remote => (2, ""), + crate::catalog::SkillSourceKind::Custom(kind) => (3, kind), + } +} + +fn lexical_terms(value: &str, max_chars: usize, max_terms: usize) -> HashSet { + value + .chars() + .take(max_chars) + .flat_map(char::to_lowercase) + .collect::() + .split(|character: char| !character.is_alphanumeric()) + .filter(|term| term.len() >= 3 && !is_stop_word(term)) + .take(max_terms) + .map(ToOwned::to_owned) + .collect() +} + +fn non_name_char(character: char) -> bool { + !character.is_alphanumeric() && character != '-' && character != '_' && character != '$' +} + +fn is_stop_word(term: &str) -> bool { + matches!( + term, + "and" + | "are" + | "but" + | "can" + | "for" + | "from" + | "has" + | "have" + | "how" + | "into" + | "not" + | "please" + | "skill" + | "skills" + | "that" + | "the" + | "this" + | "use" + | "using" + | "was" + | "what" + | "when" + | "where" + | "which" + | "with" + | "you" + ) +} + +#[cfg(test)] +mod tests { + use pretty_assertions::assert_eq; + + use super::*; + use crate::catalog::SkillAuthority; + use crate::catalog::SkillPackageId; + use crate::catalog::SkillResourceId; + use crate::catalog::SkillSourceKind; + + fn entry(name: &str, description: &str) -> SkillCatalogEntry { + SkillCatalogEntry::new( + SkillPackageId(format!("package-{name}")), + SkillAuthority::new(SkillSourceKind::Host, "host"), + name, + description, + SkillResourceId(format!("{name}/SKILL.md")), + ) + } + + #[test] + fn exact_name_precedes_lexical_matches_and_ties_are_stable() { + let catalog = SkillCatalog { + entries: vec![ + entry("zeta-review", "Review Rust changes"), + entry("rust-review", "Review changes"), + entry("alpha-review", "Review Rust changes"), + ], + warnings: Vec::new(), + }; + + let ranked = rank_catalog(&catalog, "please use rust-review for Rust review", 5) + .into_iter() + .map(|entry| entry.name.as_str()) + .collect::>(); + + assert_eq!(ranked, vec!["rust-review", "alpha-review", "zeta-review"]); + } + + #[test] + fn hidden_entries_and_zero_matches_are_excluded_at_scale() { + let mut entries = (0..2_100) + .map(|index| entry(&format!("skill-{index:04}"), "unrelated metadata")) + .collect::>(); + entries.push(entry("target-skill", "Operate Blacksmith sandboxes")); + entries.push(entry("manual-target", "Operate Blacksmith sandboxes").deferred()); + entries.push(entry("disabled-target", "Operate Blacksmith sandboxes").disabled()); + let catalog = SkillCatalog { + entries, + warnings: Vec::new(), + }; + + assert_eq!( + rank_catalog(&catalog, "Blacksmith sandbox", 5) + .into_iter() + .map(|entry| entry.name.as_str()) + .collect::>(), + vec!["target-skill"] + ); + assert!(rank_catalog(&catalog, "completely unmatched", 5).is_empty()); + } +} diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index 41c14c82ac..e166465e10 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -4,9 +4,12 @@ use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG; use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; use crate::catalog::SkillCatalog; +use crate::ranking::rank_catalog; -const MAX_AVAILABLE_SKILLS_CHARS: usize = 8_000; const MAX_MAIN_PROMPT_CHARS: usize = 40_000; +const MAX_SKILL_NAME_CHARS: usize = 256; +const MAX_SKILL_DESCRIPTION_CHARS: usize = 1_024; +const MAX_SKILL_PATH_CHARS: usize = 2_048; #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct AvailableSkillsFragment { @@ -31,39 +34,25 @@ impl ContextualUserFragment for AvailableSkillsFragment { } } -pub(crate) fn available_skills_fragment(catalog: &SkillCatalog) -> Option { - let mut total_chars = 0usize; - let mut omitted = 0usize; - let mut skill_lines = Vec::new(); - - for entry in catalog - .entries - .iter() - .filter(|entry| entry.is_prompt_visible()) - { - let description = entry - .short_description - .as_deref() - .unwrap_or(entry.description.as_str()); - let line = render_skill_line(entry, description); - let next_chars = total_chars.saturating_add(line.chars().count()); - if next_chars > MAX_AVAILABLE_SKILLS_CHARS { - omitted = omitted.saturating_add(1); - continue; - } - total_chars = next_chars; - skill_lines.push(line); - } +pub(crate) fn available_skills_fragment( + catalog: &SkillCatalog, + query: &str, + limit: usize, +) -> Option { + let skill_lines = rank_catalog(catalog, query, limit) + .into_iter() + .map(|entry| { + let description = entry + .short_description + .as_deref() + .unwrap_or(entry.description.as_str()); + render_skill_line(entry, description) + }) + .collect::>(); if skill_lines.is_empty() { return None; } - if omitted > 0 { - let skill_word = if omitted == 1 { "skill" } else { "skills" }; - skill_lines.push(format!( - "- {omitted} additional {skill_word} omitted from this bounded skills list." - )); - } Some(AvailableSkillsFragment { body: render_available_skills_body(&[], &skill_lines), @@ -71,17 +60,26 @@ pub(crate) fn available_skills_fragment(catalog: &SkillCatalog) -> Option String { - let file = format!("file: {}", entry.rendered_path()); + let name = bounded_chars(&entry.name, MAX_SKILL_NAME_CHARS); + let description = bounded_chars(description, MAX_SKILL_DESCRIPTION_CHARS); + let file = format!( + "file: {}", + bounded_chars(entry.rendered_path(), MAX_SKILL_PATH_CHARS) + ); let handles = crate::tools::catalog_tool_handles(entry).map_or(file.clone(), |tool_handles| { format!("{file}; {tool_handles}") }); if description.is_empty() { - format!("- {}: ({handles})", entry.name) + format!("- {name}: ({handles})") } else { - format!("- {}: {description} ({handles})", entry.name) + format!("- {name}: {description} ({handles})") } } +fn bounded_chars(value: &str, max_chars: usize) -> String { + value.chars().take(max_chars).collect() +} + pub(crate) fn truncate_main_prompt_contents(contents: &str) -> (String, bool) { let mut chars = 0usize; for (index, _) in contents.char_indices() { diff --git a/codex-rs/ext/skills/src/sources.rs b/codex-rs/ext/skills/src/sources.rs index 753557b8ca..8544f4f004 100644 --- a/codex-rs/ext/skills/src/sources.rs +++ b/codex-rs/ext/skills/src/sources.rs @@ -1,3 +1,5 @@ +use std::collections::HashMap; +use std::collections::HashSet; use std::fmt; use std::sync::Arc; @@ -76,7 +78,7 @@ pub struct SkillProviders { #[derive(Clone, Default)] pub(crate) struct SkillProviderRoutes { - routes: Vec, + routes: HashMap<(SkillAuthority, SkillPackageId), Arc>, } impl fmt::Debug for SkillProviderRoutes { @@ -88,13 +90,6 @@ impl fmt::Debug for SkillProviderRoutes { } } -#[derive(Clone)] -struct SkillProviderRoute { - authority: SkillAuthority, - package: SkillPackageId, - provider: Arc, -} - impl SkillProviderRoutes { fn push( &mut self, @@ -102,18 +97,7 @@ impl SkillProviderRoutes { package: SkillPackageId, provider: Arc, ) { - if self - .routes - .iter() - .any(|route| route.authority == authority && route.package == package) - { - return; - } - self.routes.push(SkillProviderRoute { - authority, - package, - provider, - }); + self.routes.entry((authority, package)).or_insert(provider); } fn provider( @@ -122,9 +106,8 @@ impl SkillProviderRoutes { package: &SkillPackageId, ) -> Option> { self.routes - .iter() - .find(|route| &route.authority == authority && &route.package == package) - .map(|route| Arc::clone(&route.provider)) + .get(&(authority.clone(), package.clone())) + .map(Arc::clone) } pub(crate) async fn read( @@ -186,6 +169,7 @@ impl SkillProviders { ) -> (SkillCatalog, SkillProviderRoutes) { let mut catalog = SkillCatalog::default(); let mut routes = SkillProviderRoutes::default(); + let mut seen = HashSet::new(); for source in self .sources @@ -195,16 +179,13 @@ impl SkillProviders { match source.provider.list(query.clone()).await { Ok(source_catalog) => { for entry in source_catalog.entries { - let entry_is_new = !catalog.entries.iter().any(|existing| { - existing.authority == entry.authority && existing.id == entry.id - }); - if entry_is_new { + if seen.insert((entry.authority.clone(), entry.id.clone())) { routes.push( entry.authority.clone(), entry.id.clone(), Arc::clone(&source.provider), ); - catalog.push_entry(entry); + catalog.entries.push(entry); } } catalog.warnings.extend(source_catalog.warnings); diff --git a/codex-rs/ext/skills/src/tools/list.rs b/codex-rs/ext/skills/src/tools/list.rs new file mode 100644 index 0000000000..5448d8ae91 --- /dev/null +++ b/codex-rs/ext/skills/src/tools/list.rs @@ -0,0 +1,155 @@ +use codex_extension_api::FunctionCallError; +use codex_extension_api::ToolCall; +use codex_extension_api::ToolExecutor; +use codex_extension_api::ToolName; +use codex_extension_api::ToolOutput; +use codex_extension_api::ToolSpec; +use codex_tools::ToolExposure; +use schemars::JsonSchema; +use serde::Deserialize; +use serde::Serialize; + +use crate::ranking::DEFAULT_SKILL_MATCH_LIMIT; +use crate::ranking::MAX_SKILL_MATCH_LIMIT; +use crate::ranking::rank_catalog; + +use super::MAX_HANDLE_BYTES; +use super::MAX_OUTPUT_BYTES; +use super::SkillToolAuthority; +use super::SkillToolContext; +use super::bounded_text; +use super::catalog_tool_handles; +use super::json_output; +use super::parse_args; +use super::serialized_len; +use super::skill_function_tool; +use super::skill_tool_name; + +const TOOL_NAME: &str = "list"; +const MAX_QUERY_BYTES: usize = 4_096; +const MAX_NAME_BYTES: usize = 256; +const MAX_DESCRIPTION_BYTES: usize = 1_024; +const MAX_INSPECTED_MATCHES: usize = 100; + +#[derive(Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] +struct ListArgs { + query: String, + limit: Option, +} + +#[derive(Debug, Eq, JsonSchema, PartialEq, Serialize)] +#[schemars(deny_unknown_fields)] +struct ListMatch { + name: String, + description: String, + authority: SkillToolAuthority, + package: String, + main_resource: String, +} + +#[derive(Debug, Eq, JsonSchema, PartialEq, Serialize)] +#[schemars(deny_unknown_fields)] +struct ListResponse { + matches: Vec, + truncated: bool, +} + +#[derive(Clone)] +pub(super) struct ListTool { + pub(super) context: SkillToolContext, +} + +#[async_trait::async_trait] +impl ToolExecutor for ListTool { + fn tool_name(&self) -> ToolName { + skill_tool_name(TOOL_NAME) + } + + fn spec(&self) -> ToolSpec { + skill_function_tool::( + TOOL_NAME, + "Search the full current skill catalog by task or capability. Returns at most five deterministic metadata matches and opaque authority, package, and main_resource handles for skills.read. Explicit-only and disabled skills are never returned.", + ) + } + + fn exposure(&self) -> ToolExposure { + ToolExposure::DirectModelOnly + } + + fn supports_parallel_tool_calls(&self) -> bool { + true + } + + async fn handle(&self, call: ToolCall) -> Result, FunctionCallError> { + let args: ListArgs = parse_args(&call)?; + validate_query(&args.query)?; + let limit = args.limit.unwrap_or(DEFAULT_SKILL_MATCH_LIMIT); + if !(1..=MAX_SKILL_MATCH_LIMIT).contains(&limit) { + return Err(FunctionCallError::RespondToModel(format!( + "limit must be between 1 and {MAX_SKILL_MATCH_LIMIT}" + ))); + } + + let snapshot = self.context.snapshot(&call.turn_id)?; + let ranked = rank_catalog(&snapshot.catalog, &args.query, MAX_INSPECTED_MATCHES); + let mut response = ListResponse { + matches: Vec::new(), + truncated: ranked.len() > limit, + }; + for entry in ranked { + if response.matches.len() == limit { + response.truncated = true; + break; + } + if catalog_tool_handles(entry).is_none() + || entry.name.is_empty() + || entry.name.len() > MAX_NAME_BYTES + { + response.truncated = true; + continue; + } + let description = entry + .short_description + .as_deref() + .unwrap_or(entry.description.as_str()); + let (description, description_truncated) = + bounded_text(description, MAX_DESCRIPTION_BYTES); + let candidate = ListMatch { + name: entry.name.clone(), + description, + authority: SkillToolAuthority::from_authority(&entry.authority), + package: entry.id.0.clone(), + main_resource: entry.main_prompt.0.clone(), + }; + if candidate.package.len() > MAX_HANDLE_BYTES + || candidate.main_resource.len() > MAX_HANDLE_BYTES + { + response.truncated = true; + continue; + } + response.matches.push(candidate); + response.truncated |= description_truncated; + if serialized_len(&response)? > MAX_OUTPUT_BYTES { + response.matches.pop(); + response.truncated = true; + break; + } + } + + json_output(&response) + } +} + +fn validate_query(query: &str) -> Result<(), FunctionCallError> { + if !query.trim().is_empty() + && query.len() <= MAX_QUERY_BYTES + && !query.chars().any(char::is_control) + { + return Ok(()); + } + + Err(FunctionCallError::RespondToModel(format!( + "query must contain non-whitespace text, contain no control characters, and be at most {MAX_QUERY_BYTES} bytes" + ))) +} diff --git a/codex-rs/ext/skills/src/tools/mod.rs b/codex-rs/ext/skills/src/tools/mod.rs index dd7f19ca05..86dd87e843 100644 --- a/codex-rs/ext/skills/src/tools/mod.rs +++ b/codex-rs/ext/skills/src/tools/mod.rs @@ -24,6 +24,7 @@ use crate::catalog::SkillSourceKind; use crate::state::SkillsThreadState; use crate::state::SkillsToolSnapshot; +mod list; mod read; mod schema; mod search; @@ -38,6 +39,9 @@ pub(crate) fn skill_tools( ) -> Vec>> { let context = SkillToolContext { thread_state }; vec![ + Arc::new(list::ListTool { + context: context.clone(), + }), Arc::new(search::SearchTool { context: context.clone(), }), diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index 2f9f76ef6d..d7c0f9919d 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -361,9 +361,7 @@ async fn installed_extension_injects_available_catalog_and_selected_entrypoint() ) .await; - assert_eq!(1, next_fragments.len()); - assert_eq!("developer", next_fragments[0].role()); - assert!(next_fragments[0].render().contains("lint-fix")); + assert!(next_fragments.is_empty()); Ok(()) } @@ -442,17 +440,13 @@ async fn deferred_skill_is_searchable_and_loadable_but_disabled_skill_is_not() - ) .await; - assert_eq!(2, fragments.len()); - let catalog_fragment = fragments[0].render(); - assert!(catalog_fragment.contains("visible-skill")); - assert!(!catalog_fragment.contains("deferred-skill")); - assert!(!catalog_fragment.contains("disabled-skill")); + assert_eq!(1, fragments.len()); assert!( - fragments[1] + fragments[0] .render() .contains("deferred-skill") ); - assert!(!fragments[1].render().contains("disabled-skill")); + assert!(!fragments[0].render().contains("disabled-skill")); assert_eq!( vec![( SkillAuthority::new(SkillSourceKind::Host, "host"), @@ -462,6 +456,25 @@ async fn deferred_skill_is_searchable_and_loadable_but_disabled_skill_is_not() - read_request_keys(&read_requests) ); + let list_tool = find_tool( + ®istry, + &session_store, + &thread_store, + ToolName::namespaced("skills", "list"), + ); + let list_output = call_tool( + list_tool, + "turn-1", + json!({ "query": "lint errors", "limit": 5 }), + ) + .await?; + assert_eq!(list_output["matches"][0]["name"], "visible-skill"); + assert!( + list_output["matches"] + .as_array() + .is_some_and(|matches| matches.len() == 1) + ); + Ok(()) } @@ -559,7 +572,7 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { TurnInputContext { turn_id: "turn-tools".to_string(), user_input: vec![UserInput::Text { - text: "inspect the package".to_string(), + text: "inspect the custom package".to_string(), text_elements: Vec::new(), }], environments: Vec::new(), @@ -580,6 +593,7 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { .map(|tool| tool.tool_name()) .collect::>(), vec![ + ToolName::namespaced("skills", "list"), ToolName::namespaced("skills", "search"), ToolName::namespaced("skills", "read"), ] @@ -594,8 +608,82 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { assert_eq!(spec.tools.len(), 1); } + let list_tool = find_tool( + ®istry, + &session_store, + &thread_store, + ToolName::namespaced("skills", "list"), + ); + let search_tool = find_tool( + ®istry, + &session_store, + &thread_store, + ToolName::namespaced("skills", "search"), + ); + let read_tool = find_tool( + ®istry, + &session_store, + &thread_store, + ToolName::namespaced("skills", "read"), + ); + let catalog_output = call_tool( + Arc::clone(&list_tool), + "turn-tools", + json!({ "query": "custom package", "limit": 5 }), + ) + .await?; + assert_eq!(catalog_output["matches"][0]["name"], "custom-package"); + assert_eq!( + catalog_output["matches"][0]["authority"], + json!({ + "kind": { "type": "custom", "value": "host" }, + "id": "custom-catalog" + }) + ); + assert_eq!(catalog_output["matches"][0]["package"], "custom-package"); + assert_eq!( + catalog_output["matches"][0]["main_resource"], + "custom/SKILL.md" + ); + let default_list_output = call_tool( + Arc::clone(&list_tool), + "turn-tools", + json!({ "query": "package" }), + ) + .await?; + assert!( + default_list_output["matches"] + .as_array() + .is_some_and(|matches| matches.len() <= 5) + ); + let oversized_list_query = call_tool( + Arc::clone(&list_tool), + "turn-tools", + json!({ "query": "x".repeat(4_097) }), + ) + .await; + assert_eq!( + oversized_list_query, + Err(FunctionCallError::RespondToModel( + "query must contain non-whitespace text, contain no control characters, and be at most 4096 bytes" + .to_string() + )) + ); + let oversized_list_limit = call_tool( + Arc::clone(&list_tool), + "turn-tools", + json!({ "query": "package", "limit": 6 }), + ) + .await; + assert_eq!( + oversized_list_limit, + Err(FunctionCallError::RespondToModel( + "limit must be between 1 and 5".to_string() + )) + ); + let custom_output = call_tool( - Arc::clone(&tools[0]), + Arc::clone(&search_tool), "turn-tools", json!({ "authority": { @@ -621,7 +709,7 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { ); let provider_search_error = call_tool( - Arc::clone(&tools[0]), + Arc::clone(&search_tool), "turn-tools", json!({ "authority": { "kind": { "type": "remote" }, "id": "catalog-b" }, @@ -638,7 +726,7 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { ); let provider_read_error = call_tool( - Arc::clone(&tools[1]), + Arc::clone(&read_tool), "turn-tools", json!({ "authority": { "kind": { "type": "remote" }, "id": "catalog-b" }, @@ -655,7 +743,7 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { ); let invalid_flood = call_tool( - Arc::clone(&tools[0]), + Arc::clone(&search_tool), "turn-tools", json!({ "authority": { "kind": { "type": "remote" }, "id": "invalid-catalog" }, @@ -668,7 +756,7 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { assert_eq!(invalid_flood["truncated"], true); let oversized_arguments = call_tool( - Arc::clone(&tools[0]), + Arc::clone(&search_tool), "turn-tools", json!({ "authority": { "kind": { "type": "remote" }, "id": "catalog-b" }, @@ -685,7 +773,7 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { ); let wrong_resource = call_tool( - Arc::clone(&tools[1]), + Arc::clone(&read_tool), "turn-tools", json!({ "authority": { "kind": { "type": "remote" }, "id": "catalog-b" }, @@ -702,7 +790,7 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { ); let search_output = call_tool( - Arc::clone(&tools[0]), + Arc::clone(&search_tool), "turn-tools", json!({ "authority": { "kind": { "type": "remote" }, "id": "catalog-b" }, @@ -741,7 +829,7 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { ); let read_output = call_tool( - Arc::clone(&tools[1]), + Arc::clone(&read_tool), "turn-tools", json!({ "authority": { "kind": { "type": "remote" }, "id": "catalog-b" }, @@ -776,7 +864,7 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { ); let unavailable = call_tool( - Arc::clone(&tools[0]), + Arc::clone(&search_tool), "turn-tools", json!({ "authority": { "kind": { "type": "remote" }, "id": "catalog-a" }, @@ -800,7 +888,7 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { }) .await; let stale_turn = call_tool( - Arc::clone(&tools[0]), + Arc::clone(&search_tool), "turn-tools", json!({ "authority": { "kind": { "type": "remote" }, "id": "catalog-b" }, diff --git a/codex-rs/mcp-server/Cargo.toml b/codex-rs/mcp-server/Cargo.toml index 29b2d6c7d7..6a9dfc3194 100644 --- a/codex-rs/mcp-server/Cargo.toml +++ b/codex-rs/mcp-server/Cargo.toml @@ -25,6 +25,7 @@ codex-exec-server = { workspace = true } codex-extension-api = { workspace = true } codex-login = { workspace = true } codex-protocol = { workspace = true } +codex-skills-extension = { workspace = true } codex-utils-cli = { workspace = true } codex-utils-json-to-toml = { workspace = true } rmcp = { workspace = true } diff --git a/codex-rs/mcp-server/src/message_processor.rs b/codex-rs/mcp-server/src/message_processor.rs index 284c64a520..acaca4a49d 100644 --- a/codex-rs/mcp-server/src/message_processor.rs +++ b/codex-rs/mcp-server/src/message_processor.rs @@ -6,7 +6,7 @@ use codex_core::StateDbHandle; use codex_core::ThreadManager; use codex_core::config::Config; use codex_exec_server::EnvironmentManager; -use codex_extension_api::empty_extension_registry; +use codex_extension_api::ExtensionRegistryBuilder; use codex_login::AuthManager; use codex_login::default_client::USER_AGENT_SUFFIX; use codex_login::default_client::get_codex_user_agent; @@ -62,12 +62,14 @@ impl MessageProcessor { /*enable_codex_api_key_env*/ false, ) .await; + let mut extension_builder = ExtensionRegistryBuilder::::new(); + codex_skills_extension::install(&mut extension_builder); let thread_manager = Arc::new(ThreadManager::new( config.as_ref(), auth_manager, SessionSource::Mcp, environment_manager, - empty_extension_registry(), + Arc::new(extension_builder.build()), /*analytics_events_client*/ None, codex_core::thread_store_from_config(config.as_ref(), state_db.clone()), state_db.clone(),