From e176922c5b02ec07f50a70996b9158f922cd1168 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Thu, 23 Jul 2026 18:13:04 +0300 Subject: [PATCH 1/6] 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 1d695d30a6..9e3b0c7b19 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1876,6 +1876,7 @@ dependencies = [ "codex-rollout", "codex-sandboxing", "codex-shell-command", + "codex-skills-extension", "codex-state", "codex-thread-store", "codex-tools", @@ -3195,6 +3196,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 7fcc4a9d46..a87ff34bde 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -223,6 +223,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 f65d07786c..48a306a6f9 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 b20ff803ac..12a3ef7933 100644 --- a/codex-rs/app-server/src/extensions.rs +++ b/codex-rs/app-server/src/extensions.rs @@ -86,6 +86,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(), From 1c1a83f7c6588ca93343d8eed2652e8643a7abf9 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Fri, 24 Jul 2026 21:26:22 +0300 Subject: [PATCH 2/6] fix(ci): enable lib tests for skills extension and unpin SDK user-content index - codex-rs/ext/skills declared `test = false` on its lib target while the new `ranking` module adds `#[cfg(test)]` unit tests, which made cargo emit a warning and `cargo shear --deny-warnings` fail. Sibling ext crates that carry in-source tests (memories, web-search, workflows, image-generation) omit the flag, so drop it here too. - The skills extension is now installed in the app-server registry, so a `` contextual fragment can precede the user text in the request payload. Assert the combined user text is present among the user content segments instead of pinning it to index 0. --- codex-rs/ext/skills/Cargo.toml | 1 - sdk/typescript/tests/run.test.ts | 5 ++++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/codex-rs/ext/skills/Cargo.toml b/codex-rs/ext/skills/Cargo.toml index b52a110c4f..c1a96568ed 100644 --- a/codex-rs/ext/skills/Cargo.toml +++ b/codex-rs/ext/skills/Cargo.toml @@ -7,7 +7,6 @@ version.workspace = true [lib] name = "codex_skills_extension" path = "src/lib.rs" -test = false doctest = false [lints] diff --git a/sdk/typescript/tests/run.test.ts b/sdk/typescript/tests/run.test.ts index 3698950e9d..f0e36b2fa0 100644 --- a/sdk/typescript/tests/run.test.ts +++ b/sdk/typescript/tests/run.test.ts @@ -609,7 +609,10 @@ describe("Codewith", () => { const payload = requests[0]; expect(payload).toBeDefined(); const lastUser = payload!.json.input.at(-1); - expect(lastUser?.content?.[0]?.text).toBe("Describe file changes\n\nFocus on impacted tests"); + // Contextual fragments (e.g. ``) can be prepended to the user + // message, so assert the combined user text is present rather than pinning its index. + const userTexts = (lastUser?.content ?? []).map((segment) => segment.text); + expect(userTexts).toContain("Describe file changes\n\nFocus on impacted tests"); } finally { cleanup(); await close(); From 8fe8907e94583e3861b5bf97b257654dae2999f0 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Fri, 24 Jul 2026 21:55:54 +0300 Subject: [PATCH 3/6] fix(sdk): assert combined user text across all input items The skills extension is now installed in the app-server registry, so a contextual fragment is appended as an additional user input item after the user message. Search every user content segment for the combined text instead of pinning it to the last item. --- sdk/typescript/tests/run.test.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sdk/typescript/tests/run.test.ts b/sdk/typescript/tests/run.test.ts index f0e36b2fa0..a2078e40ca 100644 --- a/sdk/typescript/tests/run.test.ts +++ b/sdk/typescript/tests/run.test.ts @@ -608,10 +608,12 @@ describe("Codewith", () => { const payload = requests[0]; expect(payload).toBeDefined(); - const lastUser = payload!.json.input.at(-1); - // Contextual fragments (e.g. ``) can be prepended to the user - // message, so assert the combined user text is present rather than pinning its index. - const userTexts = (lastUser?.content ?? []).map((segment) => segment.text); + // Contextual fragments (e.g. ``) are appended as additional user + // items, so assert the combined user text is present anywhere in the input instead of + // pinning it to a specific item/segment index. + const userTexts = payload!.json.input.flatMap((item) => + (item.content ?? []).map((segment) => segment.text), + ); expect(userTexts).toContain("Describe file changes\n\nFocus on impacted tests"); } finally { cleanup(); From d336d5486151e77af0a6b1cab079373e197a1188 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Fri, 24 Jul 2026 22:17:54 +0300 Subject: [PATCH 4/6] fix(skills): keep the starter list from being starved by bundled system skills The starter cap took the first `MAX_STARTER_SKILLS` entries of the scope-then-name ordering. Every session also loads the five bundled `System` skills, which sort first, so the starter list was always 100% built-ins and a workspace's own Repo/User/Admin (and plugin) skills never reached the model-visible list at all. Deal the starter slots round-robin across the scopes that are present, then render the winners in the usual deterministic order. Adds a unit test for the starvation case. Also retunes the core alias budget-pressure test: only five of its twelve skills now reach the developer message, so the context window has to be tighter for those starter entries to still exceed the 2% skills budget and exercise the alias rendering path. --- codex-rs/core-skills/src/render.rs | 90 ++++++++++++++++++++++++++--- codex-rs/core/tests/suite/client.rs | 5 +- 2 files changed, 87 insertions(+), 8 deletions(-) diff --git a/codex-rs/core-skills/src/render.rs b/codex-rs/core-skills/src/render.rs index 65303bd7b1..c4ad201831 100644 --- a/codex-rs/core-skills/src/render.rs +++ b/codex-rs/core-skills/src/render.rs @@ -1,5 +1,6 @@ use std::collections::HashMap; use std::collections::HashSet; +use std::collections::VecDeque; use std::path::Component; use std::path::Path; @@ -166,9 +167,8 @@ pub fn build_available_skills( ) -> Option { let all_skills = outcome.allowed_skills_for_implicit_invocation(); let total_count = all_skills.len(); - let skills = ordered_skills_for_budget(&all_skills) + let skills = starter_skills(&all_skills, MAX_STARTER_SKILLS) .into_iter() - .take(MAX_STARTER_SKILLS) .cloned() .collect::>(); if skills.is_empty() { @@ -904,6 +904,57 @@ fn ordered_skills_for_budget(skills: &[SkillMetadata]) -> Vec<&SkillMetadata> { ordered } +/// Pick the starter subset of skills rendered into the model-visible list. +/// +/// Every session also loads the bundled `System` skills, so taking the first +/// `limit` entries of [`ordered_skills_for_budget`] would spend the whole starter +/// budget on built-ins and starve a workspace's own Repo/User/Admin skills. Deal +/// the slots round-robin across the scopes that are actually present instead, so +/// each scope keeps a share, then emit the winners in the usual deterministic +/// order. +fn starter_skills(skills: &[SkillMetadata], limit: usize) -> Vec<&SkillMetadata> { + let ordered = ordered_skills_for_budget(skills); + if ordered.len() <= limit { + return ordered; + } + + let mut scopes: Vec = Vec::new(); + let mut buckets: Vec> = Vec::new(); + for (position, skill) in ordered.iter().enumerate() { + let scope = prompt_scope_rank(skill.scope); + match scopes.iter().position(|candidate| *candidate == scope) { + Some(index) => buckets[index].push_back(position), + None => { + scopes.push(scope); + buckets.push(VecDeque::from([position])); + } + } + } + + let mut selected = Vec::with_capacity(limit); + while selected.len() < limit { + let mut dealt = false; + for bucket in &mut buckets { + if selected.len() == limit { + break; + } + if let Some(position) = bucket.pop_front() { + selected.push(position); + dealt = true; + } + } + if !dealt { + break; + } + } + + selected.sort_unstable(); + selected + .into_iter() + .map(|position| ordered[position]) + .collect() +} + fn prompt_scope_rank(scope: SkillScope) -> u8 { match scope { SkillScope::System => 0, @@ -1253,6 +1304,32 @@ mod tests { assert!(body.contains("`skills.read`")); } + #[test] + fn starter_selection_does_not_let_bundled_system_skills_starve_other_scopes() { + // Every session loads the bundled `System` skills, and there are already + // enough of them to fill `MAX_STARTER_SKILLS` on their own. The starter + // subset must still surface the workspace's own skills. + let mut skills = (0..MAX_STARTER_SKILLS) + .map(|index| make_skill(&format!("system-{index}"), SkillScope::System)) + .collect::>(); + skills.push(make_skill("repo-only", SkillScope::Repo)); + skills.push(make_skill("user-only", SkillScope::User)); + + let selected = starter_skills(&skills, MAX_STARTER_SKILLS) + .into_iter() + .map(|skill| skill.name.as_str()) + .collect::>(); + + assert_eq!(selected.len(), MAX_STARTER_SKILLS); + assert!(selected.contains(&"repo-only"), "got {selected:?}"); + assert!(selected.contains(&"user-only"), "got {selected:?}"); + // Deterministic render order is still scope-then-name. + assert_eq!( + selected, + vec!["system-0", "system-1", "system-2", "repo-only", "user-only"] + ); + } + #[test] fn outcome_rendering_uses_aliases_when_they_allow_more_skills_to_fit() { let root = test_path_buf( @@ -1266,23 +1343,22 @@ mod tests { }) .collect::>(); let outcome = outcome_with_roots(skills.clone(), vec![root]); - let starter_skills = ordered_skills_for_budget(&skills) + let starter = starter_skills(&skills, MAX_STARTER_SKILLS) .into_iter() - .take(MAX_STARTER_SKILLS) .cloned() .collect::>(); - let absolute_minimum = starter_skills.iter().fold(0usize, |cost, skill| { + let absolute_minimum = starter.iter().fold(0usize, |cost, skill| { cost.saturating_add( SkillLine::new(skill).minimum_cost(SkillMetadataBudget::Characters(usize::MAX)), ) }); let plan = build_alias_plan( &outcome, - &starter_skills, + &starter, SkillMetadataBudget::Characters(usize::MAX), ) .expect("alias plan should build"); - let alias_minimum = starter_skills.iter().fold(plan.table_cost, |cost, skill| { + let alias_minimum = starter.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)), diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index ac25d36c98..919225ad4d 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -1581,7 +1581,10 @@ async fn skills_use_aliases_in_developer_message_under_budget_pressure() { &user_config_path, toml! { skills = { bundled = { enabled = false } } }.into(), ); - config.model_context_window = Some(12_000); + // Only `MAX_STARTER_SKILLS` of the 12 skills reach the developer message, + // so the window has to be tighter than before for those starter entries to + // still exceed the 2% skills budget and force the alias rendering. + config.model_context_window = Some(5_000); }); let codex = builder .build(&server) From 33c0a1506926d1932ebc83f7ac2adf83d0e697e1 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Sat, 25 Jul 2026 09:45:04 +0300 Subject: [PATCH 5/6] fix(skills): make the starter cap conditional and the escape hatch reachable Round-2 review fixes for the deferred-skills change. The cap now only fires when it buys something, and everything it hides is genuinely reachable. - The `MAX_STARTER_SKILLS` cap is no longer unconditional. It applies only when the whole prompt-visible catalog cannot be rendered inside the skills context budget (checked under both the absolute-path and aliased-path layouts) *and* the thread exposes a `skills.list` catalog-search tool. A workspace with a dozen skills on a 200k window renders all of them again, and the budget machinery (aliasing, description truncation, omission warnings) is back on the live path instead of being near-dead code. `build_available_skills_with_starter_cap` exposes the ceiling for embedders. - Tie the cap to the tool it depends on. `codex-core-skills` now owns the canonical `skills`/`list` tool names, the extension uses them, and core derives `SkillCatalogSearch` from the thread's actual extension tools. An embedder built with `empty_extension_registry()` can no longer be shown a truncated list it has no way to search past. - `skills.list` can widen and page: `limit` default 5 / max 50, new `offset` (max 1000), and a `next_offset` cursor in the response. The old default == max made `limit` vestigial and left matches 6..N unreachable. - De-quadratify the real hot path. `catalog_from_outcome` runs on every turn and de-duplicated with a linear scan per insert; because every host entry shares one authority, each comparison degraded into a full string compare of long common-prefix paths. Adds a 20k-entry regression test. - Surface deferral in telemetry: new `codex.thread.skills.deferred_total`, and `codex.thread.skills.truncated` now also flags deferral, so a 2105 -> 5 drop can no longer be reported alongside `truncated = 0`. - Stop duplicating the ~2.5k-char `How to use skills` preamble. The per-turn ranked fragment renders a compact `## Task-relevant skills` body and defers to the developer-message block for the shared guidance. - Improve and pin down retrieval recall: light Porter-style plural/verb-form stemming so `charts` finds a `chart` skill, plus a test documenting that synonym paraphrases still return nothing. - Restore the `skills_use_aliases_in_developer_message_under_budget_pressure` 12k context window and assert all 12 skills render; add a deterministic core-skills mirror of it. Restore follow-up-turn catalog coverage in `installed_extension_injects_available_catalog_and_selected_entrypoint`. - Update CHANGELOG. --- CHANGELOG.md | 28 ++ codex-rs/Cargo.lock | 1 + codex-rs/core-skills/src/lib.rs | 8 + codex-rs/core-skills/src/render.rs | 357 +++++++++++++++++- codex-rs/core/src/session/mod.rs | 1 + codex-rs/core/src/session/tests.rs | 3 + codex-rs/core/src/skills.rs | 19 + codex-rs/core/tests/suite/client.rs | 14 +- codex-rs/ext/skills/Cargo.toml | 1 + codex-rs/ext/skills/src/provider/host.rs | 85 ++++- codex-rs/ext/skills/src/ranking.rs | 145 ++++++- codex-rs/ext/skills/src/render.rs | 9 +- codex-rs/ext/skills/src/tools/list.rs | 55 ++- codex-rs/ext/skills/src/tools/mod.rs | 5 +- codex-rs/ext/skills/tests/skills_extension.rs | 196 +++++++++- codex-rs/otel/src/metrics/names.rs | 4 + 16 files changed, 894 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b068577b2..e9d8813fbd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,8 +42,36 @@ Known evidence gaps: ## [Unreleased] +### Added + +- Skills now scale to large catalogs. When the full skill catalog does not fit + the 2% skills context budget, the developer message renders a small starter + list instead of every skill, and the model reaches the rest through a new + `skills.list` catalog-search tool (available in the app-server and MCP-server + extension registries). The starter list is dealt round-robin across skill + scopes, so bundled system skills cannot starve a workspace's own skills. +- `skills.list` supports paging: `limit` (default 5, max 50) and `offset` + (max 1000), with a `next_offset` cursor in the response, so lower-ranked + matches are reachable without rewording the query. Ranking is lexical token + overlap with light plural/verb-form stemming, so `charts` finds a `chart` + skill; it has no synonym or embedding layer. +- New `codex.thread.skills.deferred_total` metric reports how many skills were + held back from the starter list. `codex.thread.skills.truncated` now also + flags deferral, so a large catalog can no longer report a big drop in kept + skills alongside `truncated = 0`. + ### Fixed +- The starter-list cap only applies when it is actually needed: a catalog that + fits the skills context budget still renders in full, and an embedder that + installs no `skills.list` tool always gets the complete list rather than a + truncated one it cannot search past. +- The per-turn ranked skills fragment no longer repeats the `How to use skills` + preamble already present in the developer-message skills block, saving roughly + 2.5k characters on every turn that matches a skill. +- Building the host skill catalog is no longer quadratic. It ran on every turn + and de-duplicated with a linear scan per insert over long common-prefix + filesystem paths. - Auth-profile auto-switch now resumes the interrupted turn on the new profile. When a turn fails on a rate-limited profile and no usage reset is available, the agent switches to a healthier profile and automatically re-runs the failed diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 9e3b0c7b19..f9530c18e9 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3753,6 +3753,7 @@ dependencies = [ "codex-extension-api", "codex-protocol", "codex-tools", + "codex-utils-absolute-path", "codex-utils-output-truncation", "pretty_assertions", "schemars 1.2.1", diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs index 0390302afd..f8fc21fe02 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -22,13 +22,21 @@ pub use model::SkillMetadata; pub use model::SkillPolicy; pub use model::filter_skill_load_outcome_for_product; pub use render::AvailableSkills; +pub use render::MAX_STARTER_SKILLS; pub use render::SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS; pub use render::SKILLS_HOW_TO_USE_WITH_ALIASES; pub use render::SKILLS_INTRO_WITH_ABSOLUTE_PATHS; pub use render::SKILLS_INTRO_WITH_ALIASES; +pub use render::SKILLS_LIST_TOOL_NAME; +pub use render::SKILLS_TOOL_NAMESPACE; +pub use render::SkillCatalogSearch; pub use render::SkillMetadataBudget; pub use render::SkillRenderReport; +pub use render::TASK_RELEVANT_SKILLS_HEADING; +pub use render::TASK_RELEVANT_SKILLS_INTRO; pub use render::build_available_skills; +pub use render::build_available_skills_with_starter_cap; pub use render::default_skill_metadata_budget; pub use render::render_available_skills_body; +pub use render::render_task_relevant_skills_body; pub use skill_instructions::SkillInstructions; diff --git a/codex-rs/core-skills/src/render.rs b/codex-rs/core-skills/src/render.rs index c4ad201831..c7bdf4f7b4 100644 --- a/codex-rs/core-skills/src/render.rs +++ b/codex-rs/core-skills/src/render.rs @@ -7,6 +7,7 @@ use std::path::Path; use crate::model::SkillLoadOutcome; use crate::model::SkillMetadata; use codex_otel::SessionTelemetry; +use codex_otel::THREAD_SKILLS_DEFERRED_TOTAL_METRIC; use codex_otel::THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC; use codex_otel::THREAD_SKILLS_ENABLED_TOTAL_METRIC; use codex_otel::THREAD_SKILLS_KEPT_TOTAL_METRIC; @@ -17,7 +18,13 @@ 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; +/// Upper bound on the starter list *once deferral is actually warranted*. +/// +/// This is a ceiling, not a quota: [`build_available_skills`] only falls back to +/// it when the whole prompt-visible catalog cannot be rendered inside the skills +/// context budget *and* the embedder exposes a catalog-search tool the model can +/// use to reach the rest. Small catalogs keep rendering in full. +pub 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 = "Starter skill descriptions were shortened to fit the skills context budget. The full catalog remains searchable, but some starter descriptions are shorter."; @@ -61,6 +68,35 @@ pub const SKILLS_HOW_TO_USE_WITH_ALIASES: &str = r###"- Discovery: Always consid - 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."###; +/// Namespace of the extension-provided skill tools (`skills.list`, +/// `skills.read`, ...). Shared so that core can tell whether the catalog-search +/// escape hatch referenced by the prompt text above actually exists for a +/// thread before it defers any skill. +pub const SKILLS_TOOL_NAMESPACE: &str = "skills"; +/// Name of the catalog-search tool inside [`SKILLS_TOOL_NAMESPACE`]. +pub const SKILLS_LIST_TOOL_NAME: &str = "list"; + +pub const TASK_RELEVANT_SKILLS_HEADING: &str = "## Task-relevant skills"; +pub const TASK_RELEVANT_SKILLS_INTRO: &str = "Additional catalog matches for this turn's request. The discovery, trigger, progressive-disclosure, and context-hygiene rules from the `## Skills` section above apply unchanged and are deliberately not repeated here."; + +/// Render the per-turn, ranked skill matches contributed by the skills +/// extension. +/// +/// This intentionally omits the `### How to use skills` preamble that +/// [`render_available_skills_body`] emits: on any turn whose text matches a +/// skill, both blocks land in the same request, and repeating ~2.5k characters +/// of identical how-to-use guidance defeats the point of deferring the catalog +/// in the first place. +pub fn render_task_relevant_skills_body(skill_lines: &[String]) -> String { + let mut lines: Vec = Vec::with_capacity(skill_lines.len().saturating_add(3)); + lines.push(TASK_RELEVANT_SKILLS_HEADING.to_string()); + lines.push(TASK_RELEVANT_SKILLS_INTRO.to_string()); + lines.push("### Available skills".to_string()); + lines.extend(skill_lines.iter().cloned()); + + format!("\n{}\n", lines.join("\n")) +} + pub fn render_available_skills_body(skill_root_lines: &[String], skill_lines: &[String]) -> String { let mut lines: Vec = Vec::new(); lines.push("## Skills".to_string()); @@ -143,6 +179,37 @@ pub struct AvailableSkills { pub warning_message: Option, } +/// Whether the embedder exposes a catalog-search tool (`skills.list`) that the +/// model can call to reach skills held back from the rendered starter list. +/// +/// The starter cap and the search tool are two halves of one contract: hiding +/// part of the catalog is only acceptable when the model has a way to find the +/// remainder. Embedders that build a thread with `empty_extension_registry()` +/// (or otherwise skip `codex_skills_extension::install`) pass +/// [`SkillCatalogSearch::Unavailable`] and always get the complete list, capped +/// only by the context budget itself. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SkillCatalogSearch { + /// A catalog-search tool is installed for this thread. + Available, + /// No catalog-search tool is installed; nothing may be deferred. + Unavailable, +} + +impl SkillCatalogSearch { + pub fn from_tool_available(tool_available: bool) -> Self { + if tool_available { + Self::Available + } else { + Self::Unavailable + } + } + + fn allows_deferral(self) -> bool { + matches!(self, Self::Available) + } +} + pub fn default_skill_metadata_budget(context_window: Option) -> SkillMetadataBudget { context_window .and_then(|window| usize::try_from(window).ok()) @@ -163,11 +230,32 @@ pub fn default_skill_metadata_budget(context_window: Option) -> SkillMetada pub fn build_available_skills( outcome: &SkillLoadOutcome, budget: SkillMetadataBudget, + catalog_search: SkillCatalogSearch, + side_effects: SkillRenderSideEffects<'_>, +) -> Option { + build_available_skills_with_starter_cap( + outcome, + budget, + catalog_search, + MAX_STARTER_SKILLS, + side_effects, + ) +} + +/// [`build_available_skills`] with an explicit starter ceiling, for embedders +/// and tests that need to tune how aggressively the catalog is deferred. +pub fn build_available_skills_with_starter_cap( + outcome: &SkillLoadOutcome, + budget: SkillMetadataBudget, + catalog_search: SkillCatalogSearch, + max_starter_skills: usize, side_effects: SkillRenderSideEffects<'_>, ) -> Option { let all_skills = outcome.allowed_skills_for_implicit_invocation(); let total_count = all_skills.len(); - let skills = starter_skills(&all_skills, MAX_STARTER_SKILLS) + let starter_limit = + starter_limit_for(outcome, &all_skills, budget, catalog_search, max_starter_skills); + let skills = starter_skills(&all_skills, starter_limit) .into_iter() .cloned() .collect::>(); @@ -176,6 +264,7 @@ pub fn build_available_skills( side_effects, /*total_count*/ 0, /*included_count*/ 0, + /*deferred_count*/ 0, /*omitted_count*/ 0, /*truncated_description_chars*/ 0, ); @@ -269,19 +358,24 @@ fn record_available_skills_side_effects( side_effects, available.report.total_count, available.report.included_count, + available.report.deferred_count, available.report.omitted_count, available.report.truncated_description_chars, ); - if available.report.omitted_count > 0 || available.report.truncated_description_chars > 0 { + if available.report.deferred_count > 0 + || available.report.omitted_count > 0 + || available.report.truncated_description_chars > 0 + { tracing::info!( budget_limit = budget.limit(), total_skills = available.report.total_count, included_skills = available.report.included_count, + deferred_skills = available.report.deferred_count, omitted_skills = available.report.omitted_count, truncated_description_chars_per_skill = available.report.average_truncated_description_chars(), truncated_skill_descriptions = available.report.truncated_description_count, - "truncated skill metadata to fit skills context budget" + "withheld skill metadata to fit skills context budget" ); } } @@ -301,6 +395,7 @@ fn record_skill_render_side_effects( side_effects: SkillRenderSideEffects<'_>, total_count: usize, included_count: usize, + deferred_count: usize, omitted_count: usize, truncated_description_chars: usize, ) { @@ -317,9 +412,18 @@ fn record_skill_render_side_effects( i64::try_from(included_count).unwrap_or(i64::MAX), &[], ); + // Skills held back for `skills.list` discovery are reported + // separately from budget-driven omissions, and both count as + // "the model did not see the whole catalog" so that a 2105 -> 5 + // drop cannot show up alongside `truncated = 0`. + session_telemetry.histogram( + THREAD_SKILLS_DEFERRED_TOTAL_METRIC, + i64::try_from(deferred_count).unwrap_or(i64::MAX), + &[], + ); session_telemetry.histogram( THREAD_SKILLS_TRUNCATED_METRIC, - if omitted_count > 0 { 1 } else { 0 }, + i64::from(omitted_count > 0 || deferred_count > 0), &[], ); session_telemetry.histogram( @@ -904,6 +1008,65 @@ fn ordered_skills_for_budget(skills: &[SkillMetadata]) -> Vec<&SkillMetadata> { ordered } +/// Decide how many skills the model-visible list may hold. +/// +/// Deferring part of the catalog costs recall, so it only happens when it buys +/// something. Two conditions must both hold: +/// +/// 1. the model can reach the rest through a catalog-search tool, and +/// 2. the complete catalog genuinely does not fit the skills context budget. +/// +/// Otherwise every prompt-visible skill stays in the list and the existing +/// budget machinery (aliasing, description truncation, omission warnings) does +/// the trimming, exactly as it did before deferral existed. +fn starter_limit_for( + outcome: &SkillLoadOutcome, + skills: &[SkillMetadata], + budget: SkillMetadataBudget, + catalog_search: SkillCatalogSearch, + max_starter_skills: usize, +) -> usize { + if !catalog_search.allows_deferral() + || skills.len() <= max_starter_skills + || full_catalog_fits_budget(outcome, skills, budget) + { + return skills.len(); + } + + max_starter_skills +} + +/// Whether every skill can be rendered with its full description inside the +/// budget, under either the absolute-path or the aliased-path layout. +fn full_catalog_fits_budget( + outcome: &SkillLoadOutcome, + skills: &[SkillMetadata], + budget: SkillMetadataBudget, +) -> bool { + let limit = budget.limit(); + let absolute_cost = skills.iter().fold(0usize, |used, skill| { + used.saturating_add(SkillLine::new(skill).full_cost(budget)) + }); + if absolute_cost <= limit { + return true; + } + + let Some(plan) = build_alias_plan(outcome, skills, budget) else { + return false; + }; + if plan.table_cost >= limit { + return false; + } + + let aliased_cost = skills.iter().fold(plan.table_cost, |used, skill| { + used.saturating_add( + SkillLine::with_path(skill, render_skill_path_with_aliases(skill, &plan)) + .full_cost(budget), + ) + }); + aliased_cost <= limit +} + /// Pick the starter subset of skills rendered into the model-visible list. /// /// Every session also loads the bundled `System` skills, so taking the first @@ -1257,6 +1420,7 @@ mod tests { let rendered = build_available_skills( &outcome, SkillMetadataBudget::Characters(usize::MAX), + SkillCatalogSearch::Available, SkillRenderSideEffects::None, ) .expect("skills should render"); @@ -1265,9 +1429,8 @@ 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) + fn scale_outcome(count: usize) -> SkillLoadOutcome { + let skills = (0..count) .rev() .map(|index| { let name = format!("scale-skill-{index:04}"); @@ -1277,11 +1440,19 @@ mod tests { ) }) .collect::>(); - let outcome = outcome_with_roots(skills, Vec::new()); + outcome_with_roots(skills, Vec::new()) + } + + #[test] + fn outcome_rendering_keeps_a_stable_five_skill_starter_at_large_catalog_scale() { + let outcome = scale_outcome(2_105); let rendered = build_available_skills( &outcome, - SkillMetadataBudget::Characters(usize::MAX), + // 2% of a 200k context window: the 2105-entry catalog cannot fit, + // so deferral is warranted. + default_skill_metadata_budget(Some(200_000)), + SkillCatalogSearch::Available, SkillRenderSideEffects::None, ) .expect("starter skills should render"); @@ -1304,6 +1475,171 @@ mod tests { assert!(body.contains("`skills.read`")); } + #[test] + fn starter_cap_does_not_fire_when_the_whole_catalog_fits_the_budget() { + // A dozen skills on a 200k context window fit inside the 2% budget with + // room to spare, so nothing may be hidden behind `skills.list`. + let outcome = scale_outcome(12); + + let rendered = build_available_skills( + &outcome, + default_skill_metadata_budget(Some(200_000)), + SkillCatalogSearch::Available, + SkillRenderSideEffects::None, + ) + .expect("skills should render"); + + assert_eq!(rendered.report.total_count, 12); + assert_eq!(rendered.report.included_count, 12); + assert_eq!(rendered.report.deferred_count, 0); + assert_eq!(rendered.report.omitted_count, 0); + assert_eq!(rendered.report.truncated_description_chars, 0); + } + + #[test] + fn starter_cap_does_not_fire_without_a_catalog_search_tool() { + // Same oversized catalog as the scale test, but this embedder installed + // no `skills.list`. Hiding entries would make them unreachable, so the + // budget machinery has to do the trimming instead. + let outcome = scale_outcome(2_105); + + let rendered = build_available_skills( + &outcome, + default_skill_metadata_budget(Some(200_000)), + SkillCatalogSearch::Unavailable, + SkillRenderSideEffects::None, + ) + .expect("skills should render"); + + assert_eq!(rendered.report.total_count, 2_105); + assert_eq!(rendered.report.deferred_count, 0); + assert!( + rendered.report.included_count > MAX_STARTER_SKILLS, + "expected the budget path, not the starter cap: {:?}", + rendered.report + ); + assert!( + rendered.report.omitted_count > 0, + "expected budget omissions to be reported: {:?}", + rendered.report + ); + assert!( + rendered + .warning_message + .as_deref() + .is_some_and(|message| message.contains("not included in the model-visible")), + "expected an omission warning: {:?}", + rendered.warning_message + ); + } + + #[test] + fn budget_negotiation_still_runs_on_the_starter_subset() { + // Deferral trims the catalog down to the starter cap; aliasing and + // description truncation must still apply to what is left, otherwise + // the whole budget path would be dead code. + let root = test_path_buf( + "/Users/xl/.codewith/plugins/cache/openai-curated/example/hash1234567890/skills-with-a-very-long-shared-prefix", + ) + .abs(); + let skills = (0..40) + .map(|index| { + let mut skill = skill_with_path( + &format!("shared-root-skill-{index:02}"), + &root.join(format!("skill-{index:02}/SKILL.md")), + ); + skill.description = "d".repeat(400); + skill + }) + .collect::>(); + let outcome = outcome_with_roots(skills, vec![root]); + + let rendered = build_available_skills( + &outcome, + default_skill_metadata_budget(Some(12_000)), + SkillCatalogSearch::Available, + SkillRenderSideEffects::None, + ) + .expect("skills should render"); + + assert_eq!(rendered.report.total_count, 40); + assert_eq!(rendered.report.included_count, MAX_STARTER_SKILLS); + assert_eq!(rendered.report.deferred_count, 40 - MAX_STARTER_SKILLS); + assert!( + !rendered.skill_root_lines.is_empty(), + "expected the alias plan to win under budget pressure" + ); + assert!( + rendered.report.truncated_description_chars > 0, + "expected description truncation on the starter subset: {:?}", + rendered.report + ); + } + + #[test] + fn uncapped_catalog_still_aliases_every_skill_under_budget_pressure() { + // Deterministic mirror of the `codex-core` + // `skills_use_aliases_in_developer_message_under_budget_pressure` + // integration test: 12 skills under one long shared root, a 12k context + // window (240-token budget), and no catalog-search tool. Absolute paths + // do not fit, so aliasing must kick in and carry all 12 entries. + let root = test_path_buf( + "/tmp/.tmp0a1b2c/codex-home-with-long-shared-prefix-for-skill-alias-budget-test/.tmp3d4e5f/skills", + ) + .abs(); + let skills = (0..12) + .map(|index| { + skill_with_path( + &format!("s{index:02}"), + &root.join(format!("s{index:02}/SKILL.md")), + ) + }) + .collect::>(); + let outcome = outcome_with_roots(skills, vec![root]); + let budget = default_skill_metadata_budget(Some(12_000)); + + let rendered = build_available_skills( + &outcome, + budget, + SkillCatalogSearch::Unavailable, + SkillRenderSideEffects::None, + ) + .expect("skills should render"); + + assert_eq!(rendered.report.total_count, 12); + assert_eq!(rendered.report.included_count, 12); + assert_eq!(rendered.report.deferred_count, 0); + assert_eq!(rendered.report.omitted_count, 0); + assert_eq!(rendered.skill_root_lines.len(), 1); + let rendered_text = rendered.skill_lines.join("\n"); + for index in 0..12 { + assert!( + rendered_text.contains(&format!("(file: r0/s{index:02}/SKILL.md)")), + "expected aliased entry for s{index:02} in {rendered_text}" + ); + } + } + + #[test] + fn task_relevant_body_drops_the_duplicated_how_to_use_preamble() { + let lines = vec!["- alpha-skill: desc (file: /tmp/alpha/SKILL.md)".to_string()]; + + let body = render_task_relevant_skills_body(&lines); + + assert!(body.contains(TASK_RELEVANT_SKILLS_HEADING)); + assert!(body.contains("- alpha-skill: desc (file: /tmp/alpha/SKILL.md)")); + assert!(!body.contains("### How to use skills")); + assert!(!body.contains(SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS)); + assert!(!body.contains(SKILLS_HOW_TO_USE_WITH_ALIASES)); + assert!( + body.len() + < render_available_skills_body(&[], &lines) + .len() + .saturating_sub(2_000), + "expected the compact body to save the ~2.5k-char preamble" + ); + } + #[test] fn starter_selection_does_not_let_bundled_system_skills_starve_other_scopes() { // Every session loads the bundled `System` skills, and there are already @@ -1372,6 +1708,7 @@ mod tests { let rendered = build_available_skills( &outcome, SkillMetadataBudget::Characters(alias_minimum), + SkillCatalogSearch::Available, SkillRenderSideEffects::None, ) .expect("skills should render"); diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index e9208d2a38..29f7e2aac7 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -3270,6 +3270,7 @@ impl Session { let available_skills = build_available_skills( &turn_context.turn_skills.outcome, default_skill_metadata_budget(turn_context.model_info.context_window), + crate::skills::skill_catalog_search_availability(self), SkillRenderSideEffects::ThreadStart { session_telemetry: &self.services.session_telemetry, }, diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index b00a6b688f..c694c453d6 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -10,6 +10,7 @@ use crate::context::TurnAborted; use crate::function_tool::FunctionCallError; use crate::session::handlers::inter_agent_communication; use crate::shell::default_user_shell; +use crate::skills::SkillCatalogSearch; use crate::skills::SkillRenderSideEffects; use crate::skills::render::SkillMetadataBudget; use crate::test_support::models_manager_with_provider; @@ -9126,6 +9127,7 @@ fn emit_thread_start_skill_metrics_records_enabled_kept_and_truncated_values() { let rendered = build_available_skills( &outcome, SkillMetadataBudget::Characters(1), + SkillCatalogSearch::Available, SkillRenderSideEffects::ThreadStart { session_telemetry: &session_telemetry, }, @@ -9192,6 +9194,7 @@ fn emit_thread_start_skill_metrics_records_description_truncated_chars_without_o let rendered = build_available_skills( &outcome, SkillMetadataBudget::Characters(minimum_budget + 6), + SkillCatalogSearch::Available, SkillRenderSideEffects::ThreadStart { session_telemetry: &session_telemetry, }, diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index b11f79260a..07903bedff 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -11,6 +11,7 @@ use codex_utils_plugins::PluginSkillRoot; pub use codex_core_skills::SkillError; pub use codex_core_skills::SkillLoadOutcome; pub use codex_core_skills::SkillMetadata; +pub use codex_core_skills::SkillCatalogSearch; pub use codex_core_skills::SkillPolicy; pub use codex_core_skills::SkillRenderReport; pub use codex_core_skills::SkillsLoadInput; @@ -33,6 +34,24 @@ pub use codex_core_skills::render; pub use codex_core_skills::render::SkillRenderSideEffects; pub use codex_core_skills::system; +/// Whether this thread exposes the extension-provided skills catalog-search +/// tool. +/// +/// The starter cap in `codex-core-skills` may only hide skills when the model +/// has that escape hatch. Embedders that build a thread without the skills +/// extension (`empty_extension_registry()`) therefore keep the complete +/// model-visible list. +pub(crate) fn skill_catalog_search_availability(session: &Session) -> SkillCatalogSearch { + let available = crate::tools::router::extension_tool_executors(session) + .iter() + .any(|executor| { + let name = executor.tool_name(); + name.namespace.as_deref() == Some(codex_core_skills::SKILLS_TOOL_NAMESPACE) + && name.name == codex_core_skills::SKILLS_LIST_TOOL_NAME + }); + SkillCatalogSearch::from_tool_available(available) +} + pub(crate) fn skills_load_input_from_config( config: &Config, effective_skill_roots: Vec, diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index 919225ad4d..692a862351 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -1581,10 +1581,7 @@ async fn skills_use_aliases_in_developer_message_under_budget_pressure() { &user_config_path, toml! { skills = { bundled = { enabled = false } } }.into(), ); - // Only `MAX_STARTER_SKILLS` of the 12 skills reach the developer message, - // so the window has to be tighter than before for those starter entries to - // still exceed the 2% skills budget and force the alias rendering. - config.model_context_window = Some(5_000); + config.model_context_window = Some(12_000); }); let codex = builder .build(&server) @@ -1632,6 +1629,15 @@ async fn skills_use_aliases_in_developer_message_under_budget_pressure() { ), "expected alias-specific skill instructions: {developer_messages:?}" ); + // The starter cap must not fire here: this thread has no `skills.list` + // escape hatch, so every skill stays visible and the budget machinery + // (aliasing) is what absorbs the pressure. + for index in 0..12 { + assert!( + developer_text.contains(&format!("(file: r0/s{index:02}/SKILL.md)")), + "expected skill s{index:02} in the developer message: {developer_messages:?}" + ); + } let _codex_home_guard = codex_home; let _codex_home_parent_guard = codex_home_parent; } diff --git a/codex-rs/ext/skills/Cargo.toml b/codex-rs/ext/skills/Cargo.toml index c1a96568ed..ca8ac76aad 100644 --- a/codex-rs/ext/skills/Cargo.toml +++ b/codex-rs/ext/skills/Cargo.toml @@ -24,6 +24,7 @@ serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } [dev-dependencies] +codex-utils-absolute-path = { workspace = true } codex-utils-output-truncation = { workspace = true } pretty_assertions = { workspace = true } tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } diff --git a/codex-rs/ext/skills/src/provider/host.rs b/codex-rs/ext/skills/src/provider/host.rs index 9f169ceef4..f7a5c72dce 100644 --- a/codex-rs/ext/skills/src/provider/host.rs +++ b/codex-rs/ext/skills/src/provider/host.rs @@ -102,9 +102,18 @@ fn catalog_from_outcome(outcome: &SkillLoadOutcome) -> SkillCatalog { .collect(), }; - for (skill, enabled) in outcome.skills_with_enabled() { - catalog.push_entry(catalog_entry_from_skill(skill, enabled)); - } + // This runs on every turn, for the entire host catalog. De-duplicating with + // `push_entry` would be a linear scan per insert, and because every host + // entry shares `SkillAuthority(Host, "host")` the authority guard always + // short-circuits true, so each comparison degrades into a full string + // compare of long common-prefix filesystem paths. `extend` hashes instead. + catalog.extend(SkillCatalog { + entries: outcome + .skills_with_enabled() + .map(|(skill, enabled)| catalog_entry_from_skill(skill, enabled)) + .collect(), + warnings: Vec::new(), + }); catalog } @@ -131,3 +140,73 @@ fn catalog_entry_from_skill(skill: &SkillMetadata, enabled: bool) -> SkillCatalo entry } + +#[cfg(test)] +mod tests { + use std::time::Duration; + use std::time::Instant; + + use codex_core_skills::SkillLoadOutcome; + use codex_protocol::protocol::SkillScope; + use codex_utils_absolute_path::test_support::PathBufExt; + use codex_utils_absolute_path::test_support::test_path_buf; + use pretty_assertions::assert_eq; + + use super::*; + + /// Deep enough that a linear-scan de-duplication (`n^2 / 2` full string + /// compares of long common-prefix paths) cannot finish inside the budget + /// below, while the hashed path stays in the tens of milliseconds even in a + /// debug build. + const SCALE: usize = 20_000; + const BUDGET: Duration = Duration::from_secs(20); + + fn scale_skill(index: usize) -> SkillMetadata { + SkillMetadata { + name: format!("scale-skill-{index:05}"), + description: "desc".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + path_to_skills_md: test_path_buf(&format!( + "/Users/xl/.codewith/plugins/cache/openai-curated/example/hash1234567890/skills-with-a-very-long-shared-prefix/scale-skill-{index:05}/SKILL.md" + )) + .abs(), + scope: SkillScope::User, + plugin_id: None, + } + } + + #[test] + fn catalog_from_outcome_deduplicates_by_identity() { + let skill = scale_skill(0); + let mut outcome = SkillLoadOutcome::default(); + outcome.skills = vec![skill.clone(), skill]; + + let catalog = catalog_from_outcome(&outcome); + + assert_eq!(catalog.entries.len(), 1); + } + + #[test] + fn catalog_from_outcome_stays_linear_at_catalog_scale() { + // `catalog_from_outcome` runs on the production per-turn path + // (`TurnInputContributor::contribute` -> + // `providers.list_for_turn_with_routes`), so a quadratic de-duplication + // here costs every turn, not just startup. + let mut outcome = SkillLoadOutcome::default(); + outcome.skills = (0..SCALE).map(scale_skill).collect(); + + let started = Instant::now(); + let catalog = catalog_from_outcome(&outcome); + let elapsed = started.elapsed(); + + assert_eq!(catalog.entries.len(), SCALE); + assert!( + elapsed < BUDGET, + "building a {SCALE}-entry host catalog took {elapsed:?}; expected the hashed \ + de-duplication path (under {BUDGET:?}), not a per-insert linear scan" + ); + } +} diff --git a/codex-rs/ext/skills/src/ranking.rs b/codex-rs/ext/skills/src/ranking.rs index 3cb08f1dd1..148cb84537 100644 --- a/codex-rs/ext/skills/src/ranking.rs +++ b/codex-rs/ext/skills/src/ranking.rs @@ -5,8 +5,21 @@ use codex_protocol::user_input::UserInput; use crate::catalog::SkillCatalog; use crate::catalog::SkillCatalogEntry; +/// Page size used when the model does not ask for one, and the size of the +/// ranked fragment injected into the turn. pub(crate) const DEFAULT_SKILL_MATCH_LIMIT: usize = 5; -pub(crate) const MAX_SKILL_MATCH_LIMIT: usize = 5; +/// Largest page the model may request. Deliberately larger than +/// [`DEFAULT_SKILL_MATCH_LIMIT`]: a `limit` that can only ever shrink the +/// result set is not an escape hatch. +pub(crate) const MAX_SKILL_MATCH_LIMIT: usize = 50; +/// Largest `offset` the model may page to. Together with +/// [`MAX_SKILL_MATCH_LIMIT`] this bounds how deep a single query can walk the +/// ranked catalog. +pub(crate) const MAX_SKILL_MATCH_OFFSET: usize = 1_000; + +/// A `limit` that can only shrink the page is not an escape hatch: it must be +/// able to widen the default page too. +const _: () = assert!(MAX_SKILL_MATCH_LIMIT > DEFAULT_SKILL_MATCH_LIMIT); const MAX_QUERY_CHARS: usize = 4_096; const MAX_QUERY_TERMS: usize = 128; @@ -119,12 +132,60 @@ fn lexical_terms(value: &str, max_chars: usize, max_terms: usize) -> HashSet() .split(|character: char| !character.is_alphanumeric()) + // Stop words are dropped on both sides of stemming: before, so that + // `this` is never reduced to a bare `thi`; after, so that inflections + // such as `uses` collapse onto the listed `use`. .filter(|term| term.len() >= 3 && !is_stop_word(term)) + .map(stem) + .filter(|term| !is_stop_word(term)) .take(max_terms) - .map(ToOwned::to_owned) .collect() } +/// Fold the most common English inflections onto a shared key so that +/// `charts`/`chart`, `plotting`/`plot`, and `rendered`/`render` match each +/// other. +/// +/// This is a deliberately tiny suffix stripper, not a real stemmer: it keeps +/// matching O(1) per term (both sides are normalized into the same `HashSet`) +/// and cannot introduce the quadratic prefix/fuzzy scans that a wider recall +/// fix would. It does nothing for synonyms — see the recall tests below for the +/// behaviour that is deliberately documented rather than fixed here. +fn stem(term: &str) -> String { + const MIN_STEM_LEN: usize = 3; + + // Plural forms, following Porter step 1a. + let singular = if let Some(stem) = term.strip_suffix("sses") { + format!("{stem}ss") + } else if let Some(stem) = term.strip_suffix("ies") { + format!("{stem}y") + } else if term.ends_with("ss") { + term.to_string() + } else if let Some(stem) = term.strip_suffix('s') { + stem.to_string() + } else { + term.to_string() + }; + let singular = if singular.len() >= MIN_STEM_LEN { + singular + } else { + term.to_string() + }; + + // Verb forms, following a trimmed-down Porter step 1b. No consonant + // un-doubling: `plotting` simply fails to fold onto `plot` rather than + // risking `passing` -> `pas`. + for suffix in ["ing", "ed"] { + if let Some(stem) = singular.strip_suffix(suffix) + && stem.len() >= MIN_STEM_LEN.max(4) + { + return stem.to_string(); + } + } + + singular +} + fn non_name_char(character: char) -> bool { !character.is_alphanumeric() && character != '-' && character != '_' && character != '$' } @@ -222,4 +283,84 @@ mod tests { ); assert!(rank_catalog(&catalog, "completely unmatched", 5).is_empty()); } + + #[test] + fn inflected_query_terms_still_reach_their_skill() { + let catalog = SkillCatalog { + entries: vec![ + entry("chart-builder", "Render a chart from a table"), + entry("query-runner", "Run a query against the warehouse"), + ], + warnings: Vec::new(), + }; + + assert_eq!( + rank_catalog(&catalog, "build some charts", 5) + .into_iter() + .map(|entry| entry.name.as_str()) + .collect::>(), + vec!["chart-builder"] + ); + assert_eq!( + rank_catalog(&catalog, "run my queries", 5) + .into_iter() + .map(|entry| entry.name.as_str()) + .collect::>(), + vec!["query-runner"] + ); + } + + #[test] + fn paraphrased_queries_do_not_reach_lexically_disjoint_skills() { + // Documents the real, deliberately-scoped behaviour of this ranker: it + // is lexical token overlap with light stemming. It has no synonym or + // embedding layer, so a skill whose metadata shares no token with the + // request is invisible to `skills.list` and the model has to reword. + // + // This is the reason `MAX_SKILL_MATCH_LIMIT` and paging exist: when the + // top matches are wrong, widening the page is the only recourse the + // model has. Any future recall work (substring, embeddings) should flip + // these assertions rather than delete them. + let catalog = SkillCatalog { + entries: vec![entry("dataviz", "charts, graphs, plots")], + warnings: Vec::new(), + }; + + assert!( + rank_catalog(&catalog, "help me visualize these results", 5).is_empty(), + "synonym recall is not implemented; update this test when it is" + ); + assert_eq!( + rank_catalog(&catalog, "draw me a graph", 5) + .into_iter() + .map(|entry| entry.name.as_str()) + .collect::>(), + vec!["dataviz"], + "a shared surface token is required for a match" + ); + } + + #[test] + fn ranking_can_be_walked_past_the_default_page() { + let entries = (0..40) + .map(|index| entry(&format!("blacksmith-{index:02}"), "Operate sandboxes")) + .collect::>(); + let catalog = SkillCatalog { + entries, + warnings: Vec::new(), + }; + + let first_page = rank_catalog(&catalog, "blacksmith", DEFAULT_SKILL_MATCH_LIMIT) + .into_iter() + .map(|entry| entry.name.clone()) + .collect::>(); + let wide_page = rank_catalog(&catalog, "blacksmith", MAX_SKILL_MATCH_LIMIT) + .into_iter() + .map(|entry| entry.name.clone()) + .collect::>(); + + assert_eq!(first_page.len(), DEFAULT_SKILL_MATCH_LIMIT); + assert_eq!(wide_page.len(), 40); + assert_eq!(wide_page[..first_page.len()], first_page[..]); + } } diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index e166465e10..b07f3db118 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -1,4 +1,4 @@ -use codex_core_skills::render_available_skills_body; +use codex_core_skills::render_task_relevant_skills_body; use codex_extension_api::ContextualUserFragment; use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG; use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; @@ -54,8 +54,13 @@ pub(crate) fn available_skills_fragment( return None; } + // The `## Skills` developer message already carries the shared + // `### How to use skills` preamble for this turn (both blocks are gated on + // `include_skill_instructions`), so this per-turn fragment renders only the + // ranked matches. Repeating the preamble would burn ~2.5k characters on + // every turn that lexically matches a skill. Some(AvailableSkillsFragment { - body: render_available_skills_body(&[], &skill_lines), + body: render_task_relevant_skills_body(&skill_lines), }) } diff --git a/codex-rs/ext/skills/src/tools/list.rs b/codex-rs/ext/skills/src/tools/list.rs index 5448d8ae91..a6bf5caa30 100644 --- a/codex-rs/ext/skills/src/tools/list.rs +++ b/codex-rs/ext/skills/src/tools/list.rs @@ -11,6 +11,7 @@ use serde::Serialize; use crate::ranking::DEFAULT_SKILL_MATCH_LIMIT; use crate::ranking::MAX_SKILL_MATCH_LIMIT; +use crate::ranking::MAX_SKILL_MATCH_OFFSET; use crate::ranking::rank_catalog; use super::MAX_HANDLE_BYTES; @@ -25,17 +26,24 @@ use super::serialized_len; use super::skill_function_tool; use super::skill_tool_name; -const TOOL_NAME: &str = "list"; +const TOOL_NAME: &str = codex_core_skills::SKILLS_LIST_TOOL_NAME; 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; +/// Deepest rank the tool will materialize for one call: the last reachable page +/// plus one sentinel entry used to detect that more matches exist. +const MAX_INSPECTED_MATCHES: usize = MAX_SKILL_MATCH_OFFSET + MAX_SKILL_MATCH_LIMIT + 1; #[derive(Deserialize, JsonSchema)] #[serde(deny_unknown_fields)] struct ListArgs { query: String, + /// Page size. Defaults to `DEFAULT_SKILL_MATCH_LIMIT`, maximum + /// `MAX_SKILL_MATCH_LIMIT`. limit: Option, + /// Rank of the first match to return, for paging past the first page. + /// Defaults to 0, maximum `MAX_SKILL_MATCH_OFFSET`. + offset: Option, } #[derive(Debug, Eq, JsonSchema, PartialEq, Serialize)] @@ -53,6 +61,9 @@ struct ListMatch { struct ListResponse { matches: Vec, truncated: bool, + /// Value to pass back as `offset` to continue past this page. `None` once + /// the ranked matches are exhausted. + next_offset: Option, } #[derive(Clone)] @@ -69,7 +80,9 @@ impl ToolExecutor for ListTool { 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.", + &format!( + "Search the full current skill catalog by task or capability. Returns deterministic metadata matches plus opaque authority, package, and main_resource handles for skills.read. Ranking is lexical, so if the top matches look wrong, widen `limit` (default {DEFAULT_SKILL_MATCH_LIMIT}, max {MAX_SKILL_MATCH_LIMIT}) or page deeper by passing the returned `next_offset` back as `offset` (max {MAX_SKILL_MATCH_OFFSET}) before rewording the query. Explicit-only and disabled skills are never returned." + ), ) } @@ -90,18 +103,36 @@ impl ToolExecutor for ListTool { "limit must be between 1 and {MAX_SKILL_MATCH_LIMIT}" ))); } + let offset = args.offset.unwrap_or(0); + if offset > MAX_SKILL_MATCH_OFFSET { + return Err(FunctionCallError::RespondToModel(format!( + "offset must be at most {MAX_SKILL_MATCH_OFFSET}" + ))); + } let snapshot = self.context.snapshot(&call.turn_id)?; - let ranked = rank_catalog(&snapshot.catalog, &args.query, MAX_INSPECTED_MATCHES); + // One past the requested page, so `next_offset` can distinguish "the + // page is full" from "there is genuinely nothing after it". + let inspected = offset + .saturating_add(limit) + .saturating_add(1) + .min(MAX_INSPECTED_MATCHES); + let ranked = rank_catalog(&snapshot.catalog, &args.query, inspected); + let ranked_len = ranked.len(); let mut response = ListResponse { matches: Vec::new(), - truncated: ranked.len() > limit, + truncated: false, + next_offset: None, }; - for entry in ranked { + // Ranks consumed from `offset` onwards, including entries skipped for + // unusable handles, so the caller can resume exactly where this page + // stopped. + let mut consumed = 0usize; + for entry in ranked.into_iter().skip(offset) { if response.matches.len() == limit { - response.truncated = true; break; } + consumed = consumed.saturating_add(1); if catalog_tool_handles(entry).is_none() || entry.name.is_empty() || entry.name.len() > MAX_NAME_BYTES @@ -132,11 +163,19 @@ impl ToolExecutor for ListTool { response.truncated |= description_truncated; if serialized_len(&response)? > MAX_OUTPUT_BYTES { response.matches.pop(); - response.truncated = true; + consumed = consumed.saturating_sub(1); break; } } + let scanned = offset.saturating_add(consumed); + // Either ranked matches remain past this page, or the ranking itself + // was clipped at `inspected` and there may be more beyond it. Both mean + // the model still has somewhere to go. + let has_more = scanned < ranked_len || ranked_len >= inspected; + response.truncated |= has_more; + response.next_offset = has_more.then_some(scanned); + json_output(&response) } } diff --git a/codex-rs/ext/skills/src/tools/mod.rs b/codex-rs/ext/skills/src/tools/mod.rs index 86dd87e843..1f91f35a92 100644 --- a/codex-rs/ext/skills/src/tools/mod.rs +++ b/codex-rs/ext/skills/src/tools/mod.rs @@ -29,7 +29,10 @@ mod read; mod schema; mod search; -const SKILLS_NAMESPACE: &str = "skills"; +/// Shared with `codex-core-skills` so that core can check whether this +/// catalog-search escape hatch exists before it defers any skill from the +/// model-visible starter list. +const SKILLS_NAMESPACE: &str = codex_core_skills::SKILLS_TOOL_NAMESPACE; const MAX_ARGUMENT_BYTES: usize = 16 * 1024; const MAX_HANDLE_BYTES: usize = 2_048; const MAX_OUTPUT_BYTES: usize = 32 * 1024; diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index d7c0f9919d..c5af8603d2 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -344,24 +344,180 @@ async fn installed_extension_injects_available_catalog_and_selected_entrypoint() .is_empty() ); - let next_turn_store = ExtensionData::new("turn-2"); - let next_fragments = registry.turn_input_contributors()[0] + // A follow-up turn that still matches the catalog re-injects the ranked + // list (without re-injecting the skill body, which the turn-1 entrypoint + // already covered). + let matching_turn_store = ExtensionData::new("turn-2"); + let matching_fragments = registry.turn_input_contributors()[0] .contribute( TurnInputContext { turn_id: "turn-2".to_string(), user_input: vec![UserInput::Text { - text: "no skill this time".to_string(), + text: "keep fixing the lint errors".to_string(), text_elements: Vec::new(), }], environments: Vec::new(), }, &session_store, &thread_store, - &next_turn_store, + &matching_turn_store, ) .await; - assert!(next_fragments.is_empty()); + assert_eq!(1, matching_fragments.len()); + assert_eq!("developer", matching_fragments[0].role()); + let matching_body = matching_fragments[0].render(); + assert!(matching_body.starts_with(SKILLS_INSTRUCTIONS_OPEN_TAG)); + assert!(matching_body.contains("lint-fix")); + // The `### How to use skills` preamble belongs to the developer-message + // `## Skills` block; repeating it here would duplicate ~2.5k characters on + // every matching turn. + assert!(matching_body.contains(codex_core_skills::TASK_RELEVANT_SKILLS_HEADING)); + assert!(!matching_body.contains("### How to use skills")); + assert!(!matching_body.contains(codex_core_skills::SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS)); + + // Turns whose text shares no token with any skill contribute nothing. Note + // that this also swallows short continuations ("continue", "yes") because + // the ranker is purely lexical -- see the recall tests in `ranking.rs`. + let unrelated_turn_store = ExtensionData::new("turn-3"); + let unrelated_fragments = registry.turn_input_contributors()[0] + .contribute( + TurnInputContext { + turn_id: "turn-3".to_string(), + user_input: vec![UserInput::Text { + text: "no relevant request".to_string(), + text_elements: Vec::new(), + }], + environments: Vec::new(), + }, + &session_store, + &thread_store, + &unrelated_turn_store, + ) + .await; + + assert!(unrelated_fragments.is_empty()); + + Ok(()) +} + +#[tokio::test] +async fn skills_list_pages_past_the_default_limit() -> TestResult { + // The starter list only ever shows a handful of skills, so the model must + // be able to walk the whole ranked catalog through `skills.list`. A `limit` + // that can only shrink the page, or a page with no continuation, would + // leave matches 6..N unreachable. + const TOTAL: usize = 24; + let entries = (0..TOTAL) + .map(|index| { + test_entry( + SkillSourceKind::Host, + "host", + &format!("host/lint-{index:02}"), + &format!("lint-{index:02}/SKILL.md"), + ) + }) + .collect::>(); + let provider = Arc::new(StaticSkillProvider { + catalog: SkillCatalog { + entries, + warnings: Vec::new(), + }, + read_requests: Arc::new(Mutex::new(Vec::new())), + }); + let providers = SkillProviders::new().with_host_provider(provider); + let mut builder = ExtensionRegistryBuilder::new(); + install_with_providers(&mut builder, providers); + let registry = builder.build(); + let session_store = ExtensionData::new("session"); + let thread_store = ExtensionData::new("thread"); + let session_source = SessionSource::Cli; + let config = default_config().await?; + registry.thread_lifecycle_contributors()[0] + .on_thread_start(ThreadStartInput { + config: &config, + session_source: &session_source, + persistent_thread_state_available: true, + session_store: &session_store, + thread_store: &thread_store, + }) + .await; + let turn_store = ExtensionData::new("turn-page"); + registry.turn_input_contributors()[0] + .contribute( + TurnInputContext { + turn_id: "turn-page".to_string(), + user_input: vec![UserInput::Text { + text: "fix lint errors".to_string(), + text_elements: Vec::new(), + }], + environments: Vec::new(), + }, + &session_store, + &thread_store, + &turn_store, + ) + .await; + + let list_tool = find_tool( + ®istry, + &session_store, + &thread_store, + ToolName::namespaced("skills", "list"), + ); + + // Default page: five matches, explicitly continuable. + let first = call_tool(Arc::clone(&list_tool), "turn-page", json!({ "query": "lint" })).await?; + let first_names = match_names(&first); + assert_eq!(first_names.len(), 5); + assert_eq!(first["truncated"], json!(true)); + assert_eq!(first["next_offset"], json!(5)); + + // Walk the remainder with the returned cursor; every skill is reachable. + let mut seen = first_names; + let mut offset = first["next_offset"].as_u64().expect("cursor"); + loop { + let page = call_tool( + Arc::clone(&list_tool), + "turn-page", + json!({ "query": "lint", "offset": offset }), + ) + .await?; + seen.extend(match_names(&page)); + match page["next_offset"].as_u64() { + Some(next) => { + assert!(next > offset, "cursor must advance: {page}"); + offset = next; + } + None => break, + } + } + assert_eq!(seen.len(), TOTAL); + assert_eq!( + seen.iter().collect::>().len(), + TOTAL + ); + + // `limit` can widen the page, not just narrow it. + let wide = call_tool( + Arc::clone(&list_tool), + "turn-page", + json!({ "query": "lint", "limit": 50 }), + ) + .await?; + assert_eq!(match_names(&wide).len(), TOTAL); + assert_eq!(wide["next_offset"], Value::Null); + assert_eq!(wide["truncated"], json!(false)); + + // Paging past the end is well-formed and terminates. + let past_end = call_tool( + Arc::clone(&list_tool), + "turn-page", + json!({ "query": "lint", "offset": 100 }), + ) + .await?; + assert_eq!(past_end["matches"], json!([])); + assert_eq!(past_end["next_offset"], Value::Null); Ok(()) } @@ -672,13 +828,25 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { let oversized_list_limit = call_tool( Arc::clone(&list_tool), "turn-tools", - json!({ "query": "package", "limit": 6 }), + json!({ "query": "package", "limit": 51 }), ) .await; assert_eq!( oversized_list_limit, Err(FunctionCallError::RespondToModel( - "limit must be between 1 and 5".to_string() + "limit must be between 1 and 50".to_string() + )) + ); + let oversized_list_offset = call_tool( + Arc::clone(&list_tool), + "turn-tools", + json!({ "query": "package", "offset": 1_001 }), + ) + .await; + assert_eq!( + oversized_list_offset, + Err(FunctionCallError::RespondToModel( + "offset must be at most 1000".to_string() )) ); @@ -1098,6 +1266,20 @@ fn test_codex_home() -> PathBuf { )) } +fn match_names(response: &Value) -> Vec { + response["matches"] + .as_array() + .unwrap_or_else(|| panic!("matches array in {response}")) + .iter() + .map(|entry| { + entry["name"] + .as_str() + .unwrap_or_else(|| panic!("match name in {response}")) + .to_string() + }) + .collect() +} + fn read_request_keys( requests: &Arc>>, ) -> Vec<(SkillAuthority, SkillPackageId, SkillResourceId)> { diff --git a/codex-rs/otel/src/metrics/names.rs b/codex-rs/otel/src/metrics/names.rs index c635a93820..33f1c48949 100644 --- a/codex-rs/otel/src/metrics/names.rs +++ b/codex-rs/otel/src/metrics/names.rs @@ -57,6 +57,10 @@ pub const STARTUP_PREWARM_AGE_AT_FIRST_TURN_METRIC: &str = pub const THREAD_STARTED_METRIC: &str = "codex.thread.started"; pub const THREAD_SKILLS_ENABLED_TOTAL_METRIC: &str = "codex.thread.skills.enabled_total"; pub const THREAD_SKILLS_KEPT_TOTAL_METRIC: &str = "codex.thread.skills.kept_total"; +/// Skills withheld from the model-visible starter list because the catalog did +/// not fit the skills context budget. They remain reachable through the +/// `skills.list` catalog-search tool. +pub const THREAD_SKILLS_DEFERRED_TOTAL_METRIC: &str = "codex.thread.skills.deferred_total"; pub const THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC: &str = "codex.thread.skills.description_truncated_chars"; pub const THREAD_SKILLS_TRUNCATED_METRIC: &str = "codex.thread.skills.truncated"; From 895feea62feb53e513f0fe23679841d1e3678f81 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Sat, 25 Jul 2026 11:18:32 +0300 Subject: [PATCH 6/6] fix(skills): make the whole catalog reachable and stop the prompt lying about it Round-3 review fixes on top of the deferred-skills work. - Remove the offset ceiling. `skills.list` walked at most rank 1049 (MAX_SKILL_MATCH_OFFSET 1000 + MAX_SKILL_MATCH_LIMIT 50), so on a 2000-skill catalog 47.5% of the entries were unreachable by any offset/limit combination. Ranking is now bounded by the catalog only. - Never emit a cursor the tool rejects. Following `next_offset` verbatim used to hit `offset must be at most 1000`; deep offsets are now answered with a well-formed page, `next_offset` strictly advances, and new `total_matches`/`has_more` fields separate exhaustion from a partial page. - Add an enumeration escape hatch. `query` is optional: omit it (or pass an empty string) to page the catalog alphabetically. Lexical ranking cannot reach a skill whose wording the model has to guess, so enumeration is the only complete path. - Prefer truncating descriptions over hiding skills. Deferral now fires only when the catalog cannot be listed even without descriptions; a realistic twelve-skill workspace with 1536-char descriptions rendered five and silently dropped seven. Deferral also always warns. - Carry the rules when there is no `## Skills` block. Core builds that block from the host outcome while the extension ranks the merged catalog, so a remote/executor-only catalog left the model with bare skill lines and a pointer to a section that was never written. - Make the intro conditional. A complete list no longer claims to be a subset, and `skills.list` is not advertised on threads that do not install it. - Stop diluting the description-truncation metric with the full catalog size, and assert the new deferred_total metric. - Delete the callerless O(n)-per-insert `SkillCatalog::push_entry`. --- CHANGELOG.md | 40 +- .../app-server/tests/suite/v2/turn_start.rs | 17 +- codex-rs/core-skills/src/lib.rs | 23 +- codex-rs/core-skills/src/render.rs | 435 ++++++++++++++++-- .../context/available_skills_instructions.rs | 5 +- codex-rs/core/src/session/tests.rs | 104 +++++ codex-rs/core/src/skills.rs | 2 +- codex-rs/ext/skills/src/catalog.rs | 12 - codex-rs/ext/skills/src/extension.rs | 33 +- codex-rs/ext/skills/src/provider/host.rs | 4 +- codex-rs/ext/skills/src/ranking.rs | 103 ++++- codex-rs/ext/skills/src/render.rs | 17 +- codex-rs/ext/skills/src/tools/list.rs | 101 ++-- codex-rs/ext/skills/tests/skills_extension.rs | 288 +++++++++++- 14 files changed, 1034 insertions(+), 150 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9d8813fbd..5262dc5e12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,11 +50,20 @@ Known evidence gaps: `skills.list` catalog-search tool (available in the app-server and MCP-server extension registries). The starter list is dealt round-robin across skill scopes, so bundled system skills cannot starve a workspace's own skills. -- `skills.list` supports paging: `limit` (default 5, max 50) and `offset` - (max 1000), with a `next_offset` cursor in the response, so lower-ranked - matches are reachable without rewording the query. Ranking is lexical token - overlap with light plural/verb-form stemming, so `charts` finds a `chart` - skill; it has no synonym or embedding layer. +- `skills.list` supports paging: `limit` (default 5, max 50) and an unbounded + `offset`, with a `next_offset` cursor in the response, so every ranked match + is reachable without rewording the query. The walk is bounded by the catalog + and nothing else - there is no offset ceiling, `next_offset` always strictly + advances, and any cursor the tool emits is a cursor it accepts. Responses + carry `total_matches` and `has_more` so exhaustion is distinguishable from a + partial page. Ranking is lexical token overlap with light plural/verb-form + stemming, so `charts` finds a `chart` skill; it has no synonym or embedding + layer. +- `skills.list` can enumerate. `query` is optional: omit it (or pass an empty + string) to page through the whole catalog alphabetically. Lexical ranking can + only surface skills whose metadata shares a token with the request, so without + enumeration a skill whose wording the model cannot guess was unreachable in + principle, not merely hard to find. - New `codex.thread.skills.deferred_total` metric reports how many skills were held back from the starter list. `codex.thread.skills.truncated` now also flags deferral, so a large catalog can no longer report a big drop in kept @@ -63,9 +72,24 @@ Known evidence gaps: ### Fixed - The starter-list cap only applies when it is actually needed: a catalog that - fits the skills context budget still renders in full, and an embedder that - installs no `skills.list` tool always gets the complete list rather than a - truncated one it cannot search past. + can be listed at all inside the skills context budget still renders in full + (with descriptions shortened as required) rather than hiding skills, and an + embedder that installs no `skills.list` tool always gets the complete list + rather than a truncated one it cannot search past. +- Deferring skills is never silent. Whenever the starter cap holds skills back, + the session emits a warning naming how many skills are shown, how many exist, + and that the rest stay searchable. +- The `## Skills` intro no longer claims the list is incomplete when it is + complete, and no longer points at `skills.list` on threads where that tool is + not installed. +- The per-turn ranked skills fragment carries the usage rules itself when no + `## Skills` developer block was emitted. Core builds that block from the host + skill outcome, so a catalog served entirely by remote or executor providers + used to leave the model with bare skill lines, no usage rules, and a reference + to a section that did not exist. +- The average description-truncation telemetry is no longer diluted by the full + catalog size, which rounded real per-skill truncation down to zero on large + catalogs and disarmed the truncation warning. - The per-turn ranked skills fragment no longer repeats the `How to use skills` preamble already present in the developer-message skills block, saving roughly 2.5k characters on every turn that matches a skill. diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index 203ebdda2c..a7958935bb 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -1017,14 +1017,25 @@ async fn turn_start_emits_thread_scoped_warning_notification_for_trimmed_skills( "Exceeded skills context budget of 2%. All skill descriptions were removed and "; let warning_suffix = " additional skills were not included in the model-visible skills list."; assert!( - warning.message.starts_with(warning_prefix) && warning.message.ends_with(warning_suffix), + warning.message.starts_with(warning_prefix), "unexpected warning message: {:?}", warning.message ); - let omitted_skill_count = warning + // A budget this small also trips the starter cap, so the omission sentence + // is followed by the deferral notice. Both have to be there: the user needs + // to know skills are missing *and* that `skills.list` can still find them. + let (omission, deferral) = warning .message + .split_once(&format!("{warning_suffix} ")) + .unwrap_or_else(|| panic!("unexpected warning message: {:?}", warning.message)); + assert!( + deferral.starts_with("Showing ") + && deferral.ends_with("stay searchable through `skills.list`."), + "expected a deferral notice: {:?}", + warning.message + ); + let omitted_skill_count = omission .trim_start_matches(warning_prefix) - .trim_end_matches(warning_suffix) .parse::() .expect("warning omitted skill count should be numeric"); assert!( diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs index f8fc21fe02..eee8281f6b 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -23,17 +23,32 @@ pub use model::SkillPolicy; pub use model::filter_skill_load_outcome_for_product; pub use render::AvailableSkills; pub use render::MAX_STARTER_SKILLS; -pub use render::SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS; -pub use render::SKILLS_HOW_TO_USE_WITH_ALIASES; -pub use render::SKILLS_INTRO_WITH_ABSOLUTE_PATHS; -pub use render::SKILLS_INTRO_WITH_ALIASES; +pub use render::SKILLS_DISCOVERY_CATALOG_SEARCH; +pub use render::SKILLS_DISCOVERY_COMPLETE_LIST; +pub use render::SKILLS_DISCOVERY_LEAD; +pub use render::SKILLS_DISCOVERY_PARTIAL_LIST; +pub use render::SKILLS_DISCOVERY_PATHS_WITH_ABSOLUTE_PATHS; +pub use render::SKILLS_DISCOVERY_PATHS_WITH_ALIASES; +pub use render::SKILLS_HOW_TO_USE_TAIL_WITH_ABSOLUTE_PATHS; +pub use render::SKILLS_HOW_TO_USE_TAIL_WITH_ALIASES; +pub use render::SKILLS_INTRO_COMPLETE_LIST; +pub use render::SKILLS_INTRO_LEAD; +pub use render::SKILLS_INTRO_PARTIAL_LIST; +pub use render::SKILLS_INTRO_TRAILER_WITH_ABSOLUTE_PATHS; +pub use render::SKILLS_INTRO_TRAILER_WITH_ALIASES; pub use render::SKILLS_LIST_TOOL_NAME; +pub use render::SKILLS_MISSING_WITH_CATALOG_SEARCH; +pub use render::SKILLS_MISSING_WITHOUT_CATALOG_SEARCH; pub use render::SKILLS_TOOL_NAMESPACE; +pub use render::SKILLS_TRIGGER_RULES; pub use render::SkillCatalogSearch; pub use render::SkillMetadataBudget; pub use render::SkillRenderReport; +pub use render::SkillsListCoverage; +pub use render::SkillsPreamble; pub use render::TASK_RELEVANT_SKILLS_HEADING; pub use render::TASK_RELEVANT_SKILLS_INTRO; +pub use render::TASK_RELEVANT_SKILLS_STANDALONE_INTRO; pub use render::build_available_skills; pub use render::build_available_skills_with_starter_cap; pub use render::default_skill_metadata_budget; diff --git a/codex-rs/core-skills/src/render.rs b/codex-rs/core-skills/src/render.rs index c7bdf4f7b4..0d0df892de 100644 --- a/codex-rs/core-skills/src/render.rs +++ b/codex-rs/core-skills/src/render.rs @@ -31,12 +31,46 @@ pub const SKILL_DESCRIPTION_TRUNCATED_WARNING: &str = "Starter skill description 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. 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): +/// Opening sentence of the `## Skills` intro, shared by every variant. +pub const SKILLS_INTRO_LEAD: &str = + "A skill is a set of local instructions to follow that is stored in a `SKILL.md` file."; +/// Completeness claim used when some of the catalog was held back. +pub const SKILLS_INTRO_PARTIAL_LIST: &str = + "The list below is a small starter or task-relevant subset, not the complete skills catalog."; +/// Completeness claim used when every available skill is rendered below. +/// Asserting incompleteness here when the list is in fact complete pushes the +/// model to hunt for skills that do not exist, and to reach for a +/// catalog-search tool that may not even be installed. +pub const SKILLS_INTRO_COMPLETE_LIST: &str = + "The list below is the complete set of skills available to you."; +pub const SKILLS_INTRO_TRAILER_WITH_ABSOLUTE_PATHS: &str = "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_TRAILER_WITH_ALIASES: &str = "Each entry includes a name, description, and a short path that can be expanded into an absolute path using the skill roots table."; + +/// Opening of the `- Discovery:` bullet, shared by every variant. +/// +/// Completeness and catalog-search availability are two independent axes - +/// a list can be partial with no way to search past it - so the bullet is +/// composed from parts rather than picked from a matrix of whole sentences. +pub const SKILLS_DISCOVERY_LEAD: &str = + "- Discovery: Always consider whether a skill is relevant before acting."; +pub const SKILLS_DISCOVERY_PARTIAL_LIST: &str = + "The list above is only a starter or task-relevant subset, not the complete catalog."; +pub const SKILLS_DISCOVERY_COMPLETE_LIST: &str = "The list above is every available skill."; +pub const SKILLS_DISCOVERY_PATHS_WITH_ABSOLUTE_PATHS: &str = "Each entry is a name, description, and file path, and skill bodies live on disk at those paths."; +pub const SKILLS_DISCOVERY_PATHS_WITH_ALIASES: &str = "Each entry is a name, description, and short path; skill bodies live on disk at those paths after expanding the matching alias from `### Skill roots`."; +/// Appended only when a catalog-search tool is installed *and* there is +/// something outside the rendered list for it to find. +pub const SKILLS_DISCOVERY_CATALOG_SEARCH: &str = "Use `skills.list` to search the full catalog by task, or call it with no `query` to enumerate every skill, then use `skills.read` with the returned opaque handles when needed."; +/// `- Missing/blocked:` bullet, split out because it also names `skills.list`. +pub const SKILLS_MISSING_WITH_CATALOG_SEARCH: &str = "- Missing/blocked: If a named skill isn't listed, search for it with `skills.list`. If it cannot be found or read, say so briefly and continue with the best fallback."; +pub const SKILLS_MISSING_WITHOUT_CATALOG_SEARCH: &str = "- Missing/blocked: If a named skill isn't listed, it is not available. Say so briefly and continue with the best fallback."; + +/// `- Trigger rules:` bullet, shared by every variant. +pub const SKILLS_TRIGGER_RULES: &str = "- 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."; + +/// Everything in `### How to use skills` after the completeness-dependent +/// bullets, for a list rendered with absolute paths. +pub const SKILLS_HOW_TO_USE_TAIL_WITH_ABSOLUTE_PATHS: &str = r###"- 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. 3) If `SKILL.md` points to extra folders such as `references/`, load only the specific files needed for the request; don't bulk-load everything. @@ -50,10 +84,9 @@ pub const SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS: &str = r###"- Discovery: Always - 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: 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): +/// Everything in `### How to use skills` after the completeness-dependent +/// bullets, for a list rendered with `### Skill roots` aliases. +pub const SKILLS_HOW_TO_USE_TAIL_WITH_ALIASES: &str = r###"- 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. 3) If `SKILL.md` points to extra folders such as `references/`, load only the specific files needed for the request; don't bulk-load everything. @@ -78,32 +111,137 @@ pub const SKILLS_LIST_TOOL_NAME: &str = "list"; pub const TASK_RELEVANT_SKILLS_HEADING: &str = "## Task-relevant skills"; pub const TASK_RELEVANT_SKILLS_INTRO: &str = "Additional catalog matches for this turn's request. The discovery, trigger, progressive-disclosure, and context-hygiene rules from the `## Skills` section above apply unchanged and are deliberately not repeated here."; +/// Intro used when there is no `## Skills` section to defer to, so this +/// fragment has to carry the rules itself. +pub const TASK_RELEVANT_SKILLS_STANDALONE_INTRO: &str = "Skills matching this turn's request. A skill is a set of instructions to follow, stored in a `SKILL.md` file. No separate `## Skills` section was emitted for this request, so the rules for using these skills are below."; + +/// Whether the `## Skills` developer block — and with it the shared +/// `### How to use skills` preamble — is present in the same request as the +/// per-turn task-relevant fragment. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SkillsPreamble { + /// The `## Skills` block is present and already carries the rules. + RenderedElsewhere, + /// No `## Skills` block was emitted. This happens whenever the *host* skill + /// outcome is empty but the merged catalog is not, i.e. every skill comes + /// from a remote or executor provider. Pointing at a section that does not + /// exist would leave the model with bare skill lines and no usage rules. + Missing, +} /// Render the per-turn, ranked skill matches contributed by the skills /// extension. /// -/// This intentionally omits the `### How to use skills` preamble that -/// [`render_available_skills_body`] emits: on any turn whose text matches a -/// skill, both blocks land in the same request, and repeating ~2.5k characters -/// of identical how-to-use guidance defeats the point of deferring the catalog -/// in the first place. -pub fn render_task_relevant_skills_body(skill_lines: &[String]) -> String { - let mut lines: Vec = Vec::with_capacity(skill_lines.len().saturating_add(3)); +/// With [`SkillsPreamble::RenderedElsewhere`] this intentionally omits the +/// `### How to use skills` preamble that [`render_available_skills_body`] +/// emits: on any turn whose text matches a skill, both blocks land in the same +/// request, and repeating ~2.5k characters of identical how-to-use guidance +/// defeats the point of deferring the catalog in the first place. +pub fn render_task_relevant_skills_body( + skill_lines: &[String], + preamble: SkillsPreamble, +) -> String { + let mut lines: Vec = Vec::with_capacity(skill_lines.len().saturating_add(6)); lines.push(TASK_RELEVANT_SKILLS_HEADING.to_string()); - lines.push(TASK_RELEVANT_SKILLS_INTRO.to_string()); + match preamble { + SkillsPreamble::RenderedElsewhere => lines.push(TASK_RELEVANT_SKILLS_INTRO.to_string()), + SkillsPreamble::Missing => lines.push(TASK_RELEVANT_SKILLS_STANDALONE_INTRO.to_string()), + } lines.push("### Available skills".to_string()); lines.extend(skill_lines.iter().cloned()); + if preamble == SkillsPreamble::Missing { + lines.push("### How to use skills".to_string()); + // These lines come from the merged catalog, so `skills.list` is + // installed by construction and the list is a ranked subset. + lines.push(render_discovery_bullet( + SkillsListCoverage { + complete: false, + catalog_search: SkillCatalogSearch::Available, + }, + /*aliased*/ false, + )); + lines.push(SKILLS_TRIGGER_RULES.to_string()); + lines.push(SKILLS_MISSING_WITH_CATALOG_SEARCH.to_string()); + lines.push(SKILLS_HOW_TO_USE_TAIL_WITH_ABSOLUTE_PATHS.to_string()); + } format!("\n{}\n", lines.join("\n")) } -pub fn render_available_skills_body(skill_root_lines: &[String], skill_lines: &[String]) -> String { +/// How much of the catalog the rendered `## Skills` list actually covers, and +/// whether the model has a tool to reach whatever is missing. +/// +/// The prompt used to assert incompleteness unconditionally, which is a lie +/// whenever the whole catalog fits — and it pointed at `skills.list` even on +/// threads built without the skills extension, where that tool does not exist. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct SkillsListCoverage { + /// Every prompt-visible skill is rendered in the list. + pub complete: bool, + /// Whether the catalog-search tool is installed for this thread. + pub catalog_search: SkillCatalogSearch, +} + +impl SkillsListCoverage { + /// The list is complete *and* nothing can search beyond it: the safest + /// default for embedders that render a body without a render report. + pub const COMPLETE_WITHOUT_SEARCH: Self = Self { + complete: true, + catalog_search: SkillCatalogSearch::Unavailable, + }; + + fn from_report(report: &SkillRenderReport, catalog_search: SkillCatalogSearch) -> Self { + Self { + complete: report.deferred_count == 0 && report.omitted_count == 0, + catalog_search, + } + } + + /// Only claim the list is partial when the model can do something about it. + fn advertise_catalog_search(self) -> bool { + !self.complete && self.catalog_search == SkillCatalogSearch::Available + } +} + +fn render_discovery_bullet(coverage: SkillsListCoverage, aliased: bool) -> String { + let completeness = if coverage.complete { + SKILLS_DISCOVERY_COMPLETE_LIST + } else { + SKILLS_DISCOVERY_PARTIAL_LIST + }; + let paths = if aliased { + SKILLS_DISCOVERY_PATHS_WITH_ALIASES + } else { + SKILLS_DISCOVERY_PATHS_WITH_ABSOLUTE_PATHS + }; + let mut bullet = format!("{SKILLS_DISCOVERY_LEAD} {completeness} {paths}"); + if coverage.advertise_catalog_search() { + bullet.push(' '); + bullet.push_str(SKILLS_DISCOVERY_CATALOG_SEARCH); + } + bullet +} + +pub fn render_available_skills_body( + skill_root_lines: &[String], + skill_lines: &[String], + coverage: SkillsListCoverage, +) -> String { + let aliased = !skill_root_lines.is_empty(); let mut lines: Vec = Vec::new(); lines.push("## Skills".to_string()); - if skill_root_lines.is_empty() { - lines.push(SKILLS_INTRO_WITH_ABSOLUTE_PATHS.to_string()); + let completeness = if coverage.complete { + SKILLS_INTRO_COMPLETE_LIST } else { - lines.push(SKILLS_INTRO_WITH_ALIASES.to_string()); + SKILLS_INTRO_PARTIAL_LIST + }; + let trailer = if aliased { + SKILLS_INTRO_TRAILER_WITH_ALIASES + } else { + SKILLS_INTRO_TRAILER_WITH_ABSOLUTE_PATHS + }; + lines.push(format!("{SKILLS_INTRO_LEAD} {completeness} {trailer}")); + if aliased { lines.push("### Skill roots".to_string()); lines.extend(skill_root_lines.iter().cloned()); } @@ -111,12 +249,24 @@ pub fn render_available_skills_body(skill_root_lines: &[String], skill_lines: &[ lines.extend(skill_lines.iter().cloned()); lines.push("### How to use skills".to_string()); - let how_to_use = if skill_root_lines.is_empty() { - SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS - } else { - SKILLS_HOW_TO_USE_WITH_ALIASES - }; - lines.push(how_to_use.to_string()); + lines.push(render_discovery_bullet(coverage, aliased)); + lines.push(SKILLS_TRIGGER_RULES.to_string()); + lines.push( + if coverage.catalog_search == SkillCatalogSearch::Available { + SKILLS_MISSING_WITH_CATALOG_SEARCH + } else { + SKILLS_MISSING_WITHOUT_CATALOG_SEARCH + } + .to_string(), + ); + lines.push( + if aliased { + SKILLS_HOW_TO_USE_TAIL_WITH_ALIASES + } else { + SKILLS_HOW_TO_USE_TAIL_WITH_ABSOLUTE_PATHS + } + .to_string(), + ); format!("\n{}\n", lines.join("\n")) } @@ -176,6 +326,9 @@ pub struct AvailableSkills { pub skill_root_lines: Vec, pub skill_lines: Vec, pub report: SkillRenderReport, + /// What the rendered list actually covers, so the intro can stop asserting + /// incompleteness on lists that are complete. + pub coverage: SkillsListCoverage, pub warning_message: Option, } @@ -253,8 +406,13 @@ pub fn build_available_skills_with_starter_cap( ) -> Option { let all_skills = outcome.allowed_skills_for_implicit_invocation(); let total_count = all_skills.len(); - let starter_limit = - starter_limit_for(outcome, &all_skills, budget, catalog_search, max_starter_skills); + let starter_limit = starter_limit_for( + outcome, + &all_skills, + budget, + catalog_search, + max_starter_skills, + ); let skills = starter_skills(&all_skills, starter_limit) .into_iter() .cloned() @@ -294,10 +452,42 @@ pub fn build_available_skills_with_starter_cap( selected.report.total_count = total_count; selected.report.deferred_count = total_count.saturating_sub(skills.len()); + // Deferral hides whole skills, which is strictly more surprising than a + // shortened description, so it must never be silent. `warning_message` is + // built before the starter cap is known, so fold it in here. + if selected.report.deferred_count > 0 { + selected.warning_message = Some(deferred_skills_warning( + &selected.report, + selected.warning_message.as_deref(), + )); + } + selected.coverage = SkillsListCoverage::from_report(&selected.report, catalog_search); record_available_skills_side_effects(&selected, budget, side_effects); Some(selected) } +/// Append the deferral notice to whatever the budget renderer already had to +/// say. +/// +/// Deliberately appended rather than prepended: the budget warning keeps its +/// established opening, and a reader who only skims the first sentence still +/// learns that skills are missing. +fn deferred_skills_warning(report: &SkillRenderReport, existing: Option<&str>) -> String { + let skill_word = if report.deferred_count == 1 { + "skill" + } else { + "skills" + }; + let deferral = format!( + "Showing {} of {} skills in the model-visible list; the other {} {} stay searchable through `skills.list`.", + report.included_count, report.total_count, report.deferred_count, skill_word + ); + match existing { + Some(existing) => format!("{existing} {deferral}"), + None => deferral, + } +} + fn build_available_skills_from_lines( skill_lines: Vec>, total_count: usize, @@ -340,10 +530,15 @@ fn build_available_skills_from_lines( } else { None }; + let coverage = SkillsListCoverage { + complete: report.omitted_count == 0, + catalog_search: SkillCatalogSearch::Unavailable, + }; let available = AvailableSkills { skill_root_lines: path_aliases.skill_root_lines, skill_lines, report, + coverage, warning_message, }; Some(available) @@ -548,14 +743,22 @@ fn skill_render_report( } impl SkillRenderReport { + /// Average characters of description dropped per skill that was actually + /// put through the renderer. + /// + /// Deliberately *not* divided by `total_count`: that field is rewritten to + /// the full catalog size once the starter cap is known, which would dilute + /// "100 chars trimmed from each of 5 starter skills" into 500/2105 = 0 and + /// silently disarm both the truncation warning and the metric. fn average_truncated_description_chars(&self) -> usize { - if self.total_count == 0 || self.truncated_description_chars == 0 { + let rendered_count = self.included_count.saturating_add(self.omitted_count); + if rendered_count == 0 || self.truncated_description_chars == 0 { return 0; } self.truncated_description_chars - .saturating_add(self.total_count.saturating_sub(1)) - / self.total_count + .saturating_add(rendered_count.saturating_sub(1)) + / rendered_count } } @@ -901,8 +1104,11 @@ fn aliased_metadata_overhead_cost( skill_root_lines: &[String], ) -> usize { let empty_skill_lines: &[String] = &[]; - let absolute_body = render_available_skills_body(&[], empty_skill_lines); - let aliased_body = render_available_skills_body(skill_root_lines, empty_skill_lines); + // Only the layout differs between the two calls, so the coverage wording + // cancels out of the delta as long as both sides use the same value. + let coverage = SkillsListCoverage::COMPLETE_WITHOUT_SEARCH; + let absolute_body = render_available_skills_body(&[], empty_skill_lines, coverage); + let aliased_body = render_available_skills_body(skill_root_lines, empty_skill_lines, coverage); budget .cost(&aliased_body) .saturating_sub(budget.cost(&absolute_body)) @@ -1028,7 +1234,7 @@ fn starter_limit_for( ) -> usize { if !catalog_search.allows_deferral() || skills.len() <= max_starter_skills - || full_catalog_fits_budget(outcome, skills, budget) + || whole_catalog_can_be_listed(outcome, skills, budget) { return skills.len(); } @@ -1036,16 +1242,24 @@ fn starter_limit_for( max_starter_skills } -/// Whether every skill can be rendered with its full description inside the -/// budget, under either the absolute-path or the aliased-path layout. -fn full_catalog_fits_budget( +/// Whether every skill can be *listed at all* inside the budget, i.e. with its +/// name and path and its description shortened as far as the empty string, +/// under either the absolute-path or the aliased-path layout. +/// +/// Deferral removes whole skills from the model's view; description truncation +/// only makes them terser. Truncation is therefore always preferred, and +/// deferral is reserved for catalogs so large they cannot be enumerated even in +/// that minimal form. Testing the *full* cost here made a realistic twelve-skill +/// workspace with long descriptions silently drop seven of its skills, where the +/// truncation path would have rendered all twelve. +fn whole_catalog_can_be_listed( outcome: &SkillLoadOutcome, skills: &[SkillMetadata], budget: SkillMetadataBudget, ) -> bool { let limit = budget.limit(); let absolute_cost = skills.iter().fold(0usize, |used, skill| { - used.saturating_add(SkillLine::new(skill).full_cost(budget)) + used.saturating_add(SkillLine::new(skill).minimum_cost(budget)) }); if absolute_cost <= limit { return true; @@ -1061,7 +1275,7 @@ fn full_catalog_fits_budget( let aliased_cost = skills.iter().fold(plan.table_cost, |used, skill| { used.saturating_add( SkillLine::with_path(skill, render_skill_path_with_aliases(skill, &plan)) - .full_cost(budget), + .minimum_cost(budget), ) }); aliased_cost <= limit @@ -1461,20 +1675,120 @@ mod tests { 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); + // Hiding 2100 skills must never be silent. + assert_eq!( + rendered.warning_message.as_deref(), + Some( + "Showing 5 of 2105 skills in the model-visible list; the other 2100 skills stay searchable through `skills.list`." + ) + ); 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); + let body = render_available_skills_body( + &rendered.skill_root_lines, + &rendered.skill_lines, + rendered.coverage, + ); 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 a_small_catalog_truncates_descriptions_instead_of_hiding_skills() { + // Twelve skills with realistically long descriptions on a 200k window. + // The full descriptions do not fit the 2% budget, but the skills + // themselves easily do, so all twelve must still be listed. + let outcome = outcome_with_roots( + (0..12) + .map(|index| { + let mut skill = skill_with_path( + &format!("wide-skill-{index:02}"), + &test_path_buf(&format!("/tmp/skills/wide-skill-{index:02}/SKILL.md")) + .abs(), + ); + skill.description = "d".repeat(1_536); + skill + }) + .collect::>(), + Vec::new(), + ); + + let rendered = build_available_skills( + &outcome, + default_skill_metadata_budget(Some(200_000)), + SkillCatalogSearch::Available, + SkillRenderSideEffects::None, + ) + .expect("skills should render"); + + assert_eq!(rendered.report.total_count, 12); + assert_eq!(rendered.report.included_count, 12); + assert_eq!(rendered.report.deferred_count, 0); + assert_eq!(rendered.report.omitted_count, 0); + assert!(rendered.report.truncated_description_chars > 0); + assert!( + rendered.warning_message.is_some(), + "shortening descriptions must warn" + ); + } + + #[test] + fn a_complete_list_does_not_claim_to_be_a_subset_or_advertise_a_missing_tool() { + let outcome = scale_outcome(12); + + let rendered = build_available_skills( + &outcome, + default_skill_metadata_budget(Some(200_000)), + SkillCatalogSearch::Unavailable, + SkillRenderSideEffects::None, + ) + .expect("skills should render"); + + assert_eq!(rendered.report.deferred_count, 0); + assert_eq!( + rendered.coverage, + SkillsListCoverage::COMPLETE_WITHOUT_SEARCH + ); + + let body = render_available_skills_body( + &rendered.skill_root_lines, + &rendered.skill_lines, + rendered.coverage, + ); + assert!(body.contains(SKILLS_INTRO_COMPLETE_LIST)); + assert!(body.contains(SKILLS_DISCOVERY_COMPLETE_LIST)); + assert!(!body.contains("not the complete")); + assert!( + !body.contains("skills.list"), + "must not point at a tool this thread does not have: {body}" + ); + } + + #[test] + fn a_partial_list_keeps_pointing_at_the_catalog_search_escape_hatch() { + let coverage = SkillsListCoverage { + complete: false, + catalog_search: SkillCatalogSearch::Available, + }; + + let body = render_available_skills_body( + &[], + &["- alpha: desc (file: /tmp/alpha/SKILL.md)".to_string()], + coverage, + ); + + assert!(body.contains(SKILLS_INTRO_PARTIAL_LIST)); + assert!(body.contains(SKILLS_DISCOVERY_PARTIAL_LIST)); + assert!(body.contains(SKILLS_DISCOVERY_CATALOG_SEARCH)); + assert!(body.contains(SKILLS_MISSING_WITH_CATALOG_SEARCH)); + } + #[test] fn starter_cap_does_not_fire_when_the_whole_catalog_fits_the_budget() { // A dozen skills on a 200k context window fit inside the 2% budget with @@ -1624,22 +1938,45 @@ mod tests { fn task_relevant_body_drops_the_duplicated_how_to_use_preamble() { let lines = vec!["- alpha-skill: desc (file: /tmp/alpha/SKILL.md)".to_string()]; - let body = render_task_relevant_skills_body(&lines); + let body = render_task_relevant_skills_body(&lines, SkillsPreamble::RenderedElsewhere); assert!(body.contains(TASK_RELEVANT_SKILLS_HEADING)); assert!(body.contains("- alpha-skill: desc (file: /tmp/alpha/SKILL.md)")); assert!(!body.contains("### How to use skills")); - assert!(!body.contains(SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS)); - assert!(!body.contains(SKILLS_HOW_TO_USE_WITH_ALIASES)); + assert!(!body.contains(SKILLS_HOW_TO_USE_TAIL_WITH_ABSOLUTE_PATHS)); + assert!(!body.contains(SKILLS_HOW_TO_USE_TAIL_WITH_ALIASES)); assert!( body.len() - < render_available_skills_body(&[], &lines) - .len() - .saturating_sub(2_000), + < render_available_skills_body( + &[], + &lines, + SkillsListCoverage::COMPLETE_WITHOUT_SEARCH + ) + .len() + .saturating_sub(2_000), "expected the compact body to save the ~2.5k-char preamble" ); } + #[test] + fn task_relevant_body_carries_the_rules_when_there_is_no_skills_section() { + // A purely remote/executor catalog produces no `## Skills` block, so + // this fragment is the only place the model can learn the rules - and + // it must not point at a section that was never written. + let lines = vec!["- alpha-skill: desc (file: remote/alpha)".to_string()]; + + let body = render_task_relevant_skills_body(&lines, SkillsPreamble::Missing); + + assert!(body.contains(TASK_RELEVANT_SKILLS_HEADING)); + assert!(!body.contains(TASK_RELEVANT_SKILLS_INTRO)); + assert!(body.contains(TASK_RELEVANT_SKILLS_STANDALONE_INTRO)); + assert!(body.contains("### How to use skills")); + assert!(body.contains(SKILLS_DISCOVERY_PARTIAL_LIST)); + assert!(body.contains(SKILLS_DISCOVERY_CATALOG_SEARCH)); + assert!(body.contains(SKILLS_TRIGGER_RULES)); + assert!(body.contains(SKILLS_HOW_TO_USE_TAIL_WITH_ABSOLUTE_PATHS)); + } + #[test] fn starter_selection_does_not_let_bundled_system_skills_starve_other_scopes() { // Every session loads the bundled `System` skills, and there are already diff --git a/codex-rs/core/src/context/available_skills_instructions.rs b/codex-rs/core/src/context/available_skills_instructions.rs index f7921072b2..0a7bb0a7d2 100644 --- a/codex-rs/core/src/context/available_skills_instructions.rs +++ b/codex-rs/core/src/context/available_skills_instructions.rs @@ -1,4 +1,5 @@ use codex_core_skills::AvailableSkills; +use codex_core_skills::SkillsListCoverage; use codex_core_skills::render_available_skills_body; use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG; use codex_protocol::protocol::SKILLS_INSTRUCTIONS_OPEN_TAG; @@ -9,6 +10,7 @@ use super::ContextualUserFragment; pub(crate) struct AvailableSkillsInstructions { skill_root_lines: Vec, skill_lines: Vec, + coverage: SkillsListCoverage, } impl From for AvailableSkillsInstructions { @@ -16,6 +18,7 @@ impl From for AvailableSkillsInstructions { Self { skill_root_lines: available_skills.skill_root_lines, skill_lines: available_skills.skill_lines, + coverage: available_skills.coverage, } } } @@ -34,6 +37,6 @@ impl ContextualUserFragment for AvailableSkillsInstructions { } fn body(&self) -> String { - render_available_skills_body(&self.skill_root_lines, &self.skill_lines) + render_available_skills_body(&self.skill_root_lines, &self.skill_lines, self.coverage) } } diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index c694c453d6..b56d981324 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -12,6 +12,7 @@ use crate::session::handlers::inter_agent_communication; use crate::shell::default_user_shell; use crate::skills::SkillCatalogSearch; use crate::skills::SkillRenderSideEffects; +use crate::skills::render::MAX_STARTER_SKILLS; use crate::skills::render::SkillMetadataBudget; use crate::test_support::models_manager_with_provider; use crate::tools::format_exec_output_str; @@ -97,6 +98,7 @@ use codex_execpolicy::Policy; use codex_network_proxy::NetworkProxyConfig; use codex_otel::MetricsClient; use codex_otel::MetricsConfig; +use codex_otel::THREAD_SKILLS_DEFERRED_TOTAL_METRIC; use codex_otel::THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC; use codex_otel::THREAD_SKILLS_ENABLED_TOTAL_METRIC; use codex_otel::THREAD_SKILLS_KEPT_TOTAL_METRIC; @@ -9207,12 +9209,114 @@ fn emit_thread_start_skill_metrics_records_description_truncated_chars_without_o .snapshot_metrics() .expect("runtime metrics snapshot"); assert_eq!(histogram_sum(&snapshot, THREAD_SKILLS_TRUNCATED_METRIC), 0); + assert_eq!( + histogram_sum(&snapshot, THREAD_SKILLS_DEFERRED_TOTAL_METRIC), + 0 + ); assert_eq!( histogram_sum(&snapshot, THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC), 8 ); } +#[test] +fn emit_thread_start_skill_metrics_records_skills_deferred_to_catalog_search() { + // A catalog too large to list even without descriptions is held back to the + // starter cap. That drop has to show up in the metrics: `kept_total` alone + // would report the shrink with no explanation, and `deferred_total` is the + // only signal that separates "budget cut them" from "search will find them". + let session_telemetry = test_session_telemetry_without_metadata(); + let mut outcome = SkillLoadOutcome::default(); + outcome.skills = (0..400) + .map(|index| SkillMetadata { + name: format!("repo-skill-{index:03}"), + description: "desc".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + path_to_skills_md: test_path_buf(&format!("/tmp/repo-skill-{index:03}/SKILL.md")).abs(), + scope: SkillScope::Repo, + plugin_id: None, + }) + .collect(); + + let rendered = build_available_skills( + &outcome, + SkillMetadataBudget::Characters(1_000), + SkillCatalogSearch::Available, + SkillRenderSideEffects::ThreadStart { + session_telemetry: &session_telemetry, + }, + ) + .expect("skills should render"); + + assert_eq!(rendered.report.total_count, 400); + assert_eq!(rendered.report.included_count, MAX_STARTER_SKILLS); + assert_eq!(rendered.report.deferred_count, 400 - MAX_STARTER_SKILLS); + assert!(rendered.warning_message.is_some()); + let snapshot = session_telemetry + .snapshot_metrics() + .expect("runtime metrics snapshot"); + assert_eq!( + histogram_sum(&snapshot, THREAD_SKILLS_ENABLED_TOTAL_METRIC), + 400 + ); + assert_eq!( + histogram_sum(&snapshot, THREAD_SKILLS_KEPT_TOTAL_METRIC), + u64::try_from(MAX_STARTER_SKILLS).expect("starter cap fits u64") + ); + assert_eq!( + histogram_sum(&snapshot, THREAD_SKILLS_DEFERRED_TOTAL_METRIC), + u64::try_from(400 - MAX_STARTER_SKILLS).expect("deferred count fits u64") + ); + assert_eq!(histogram_sum(&snapshot, THREAD_SKILLS_TRUNCATED_METRIC), 1); +} + +#[test] +fn description_truncation_metric_is_not_diluted_by_the_full_catalog_size() { + // `report.total_count` is rewritten to the whole catalog once the starter + // cap is known. Averaging the truncated characters over that number instead + // of over the skills actually rendered used to round 100-chars-per-starter + // down to zero and silently disarm the truncation warning. + let session_telemetry = test_session_telemetry_without_metadata(); + let mut outcome = SkillLoadOutcome::default(); + outcome.skills = (0..400) + .map(|index| SkillMetadata { + name: format!("repo-skill-{index:03}"), + description: "d".repeat(200), + short_description: None, + interface: None, + dependencies: None, + policy: None, + path_to_skills_md: test_path_buf(&format!("/tmp/repo-skill-{index:03}/SKILL.md")).abs(), + scope: SkillScope::Repo, + plugin_id: None, + }) + .collect(); + + let rendered = build_available_skills( + &outcome, + SkillMetadataBudget::Characters(1_000), + SkillCatalogSearch::Available, + SkillRenderSideEffects::ThreadStart { + session_telemetry: &session_telemetry, + }, + ) + .expect("skills should render"); + + assert_eq!(rendered.report.total_count, 400); + assert!(rendered.report.truncated_description_chars > 0); + let snapshot = session_telemetry + .snapshot_metrics() + .expect("runtime metrics snapshot"); + assert_eq!( + histogram_sum(&snapshot, THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC), + u64::try_from(rendered.report.truncated_description_chars) + .expect("truncated chars fit u64") + ); +} + #[tokio::test] async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_builds() { let (session, turn_context, rx) = make_session_and_context_with_rx().await; diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index 07903bedff..84e3311961 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -8,10 +8,10 @@ use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_plugins::PluginSkillRoot; +pub use codex_core_skills::SkillCatalogSearch; pub use codex_core_skills::SkillError; pub use codex_core_skills::SkillLoadOutcome; pub use codex_core_skills::SkillMetadata; -pub use codex_core_skills::SkillCatalogSearch; pub use codex_core_skills::SkillPolicy; pub use codex_core_skills::SkillRenderReport; pub use codex_core_skills::SkillsLoadInput; diff --git a/codex-rs/ext/skills/src/catalog.rs b/codex-rs/ext/skills/src/catalog.rs index b1c4c7c105..f9c5bc19cb 100644 --- a/codex-rs/ext/skills/src/catalog.rs +++ b/codex-rs/ext/skills/src/catalog.rs @@ -173,18 +173,6 @@ impl SkillCatalog { } self.warnings.extend(other.warnings); } - - pub fn push_entry(&mut self, entry: SkillCatalogEntry) { - if self - .entries - .iter() - .any(|existing| existing.authority == entry.authority && existing.id == entry.id) - { - return; - } - - self.entries.push(entry); - } } /// Contents returned after resolving a skill resource through its owner. diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index 6ec1e4d5fd..1a70423f9c 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use codex_core::config::Config; use codex_core_skills::HostLoadedSkills; use codex_core_skills::SkillInstructions; +use codex_core_skills::SkillsPreamble; use codex_core_skills::injection::InjectedHostSkillPrompts; use codex_core_skills::injection::SkillInjection; use codex_extension_api::ConfigContributor; @@ -153,8 +154,12 @@ impl TurnInputContributor for SkillsExtension { 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, &user_query, DEFAULT_SKILL_MATCH_LIMIT) + && let Some(fragment) = available_skills_fragment( + &catalog, + &user_query, + DEFAULT_SKILL_MATCH_LIMIT, + skills_preamble_for(host_loaded_skills.as_deref()), + ) { fragments.push(Box::new(fragment)); } @@ -237,6 +242,30 @@ impl SkillsExtension { } } +/// Whether core will have emitted a `## Skills` developer block for this turn. +/// +/// Core renders that block from the *host* skill outcome, while this extension +/// ranks over the merged catalog. A thread whose skills all come from remote or +/// executor providers therefore has an empty host outcome and no `## Skills` +/// section, and the task-relevant fragment must carry the usage rules itself +/// instead of deferring to a section that was never written. +fn skills_preamble_for(host_loaded_skills: Option<&HostLoadedSkills>) -> SkillsPreamble { + // Deliberately not `allowed_skills_for_implicit_invocation()`: that clones + // the whole catalog, and this runs on every turn. + let host_block_rendered = host_loaded_skills.is_some_and(|host| { + let outcome = host.outcome(); + outcome + .skills + .iter() + .any(|skill| outcome.is_skill_allowed_for_implicit_invocation(skill)) + }); + if host_block_rendered { + SkillsPreamble::RenderedElsewhere + } else { + SkillsPreamble::Missing + } +} + fn clear_tool_snapshot(thread_store: &ExtensionData, turn_id: &str) { if let Some(thread_state) = thread_store.get::() { thread_state.clear_tool_snapshot(turn_id); diff --git a/codex-rs/ext/skills/src/provider/host.rs b/codex-rs/ext/skills/src/provider/host.rs index f7a5c72dce..43d08a0683 100644 --- a/codex-rs/ext/skills/src/provider/host.rs +++ b/codex-rs/ext/skills/src/provider/host.rs @@ -103,8 +103,8 @@ fn catalog_from_outcome(outcome: &SkillLoadOutcome) -> SkillCatalog { }; // This runs on every turn, for the entire host catalog. De-duplicating with - // `push_entry` would be a linear scan per insert, and because every host - // entry shares `SkillAuthority(Host, "host")` the authority guard always + // a per-insert linear scan would be quadratic, and because every host entry + // shares `SkillAuthority(Host, "host")` the authority guard always // short-circuits true, so each comparison degrades into a full string // compare of long common-prefix filesystem paths. `extend` hashes instead. catalog.extend(SkillCatalog { diff --git a/codex-rs/ext/skills/src/ranking.rs b/codex-rs/ext/skills/src/ranking.rs index 148cb84537..29e66fb243 100644 --- a/codex-rs/ext/skills/src/ranking.rs +++ b/codex-rs/ext/skills/src/ranking.rs @@ -12,10 +12,6 @@ pub(crate) const DEFAULT_SKILL_MATCH_LIMIT: usize = 5; /// [`DEFAULT_SKILL_MATCH_LIMIT`]: a `limit` that can only ever shrink the /// result set is not an escape hatch. pub(crate) const MAX_SKILL_MATCH_LIMIT: usize = 50; -/// Largest `offset` the model may page to. Together with -/// [`MAX_SKILL_MATCH_LIMIT`] this bounds how deep a single query can walk the -/// ranked catalog. -pub(crate) const MAX_SKILL_MATCH_OFFSET: usize = 1_000; /// A `limit` that can only shrink the page is not an escape hatch: it must be /// able to widen the default page too. @@ -43,14 +39,34 @@ pub(crate) fn user_text_query(inputs: &[UserInput]) -> String { query } +/// The best `limit` matches for `query`, in ranked order. pub(crate) fn rank_catalog<'a>( catalog: &'a SkillCatalog, query: &str, limit: usize, +) -> Vec<&'a SkillCatalogEntry> { + if limit == 0 { + return Vec::new(); + } + + let mut ranked = rank_catalog_all(catalog, query); + ranked.truncate(limit); + ranked +} + +/// Every prompt-visible entry that matches `query`, best first. +/// +/// The result set is bounded by the catalog and nothing else. A fixed page +/// ceiling here would make the tail of a large catalog unreachable no matter +/// how the model pages, which is exactly the failure mode paging exists to +/// avoid. +pub(crate) fn rank_catalog_all<'a>( + catalog: &'a SkillCatalog, + query: &str, ) -> 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 { + if query_terms.is_empty() { return Vec::new(); } @@ -72,11 +88,33 @@ pub(crate) fn rank_catalog<'a>( .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() + ranked.into_iter().map(|(_, entry)| entry).collect() +} + +/// Every prompt-visible entry, in a stable order that does not depend on any +/// query. +/// +/// This is the enumeration escape hatch behind a `skills.list` call with no +/// `query`. Ranking is lexical, so a skill only surfaces when the request +/// happens to share a surface token with its metadata: without a way to list +/// the catalog outright, a skill whose wording the model cannot guess is +/// unreachable in principle, not merely hard to find. +pub(crate) fn enumerate_catalog(catalog: &SkillCatalog) -> Vec<&SkillCatalogEntry> { + let mut entries = catalog + .entries + .iter() + .filter(|entry| entry.is_prompt_visible()) + .collect::>(); + entries.sort_by(|left, right| { + left.name + .cmp(&right.name) + .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)) + }); + entries } fn relevance_score( @@ -132,9 +170,9 @@ fn lexical_terms(value: &str, max_chars: usize, max_terms: usize) -> HashSet() .split(|character: char| !character.is_alphanumeric()) - // Stop words are dropped on both sides of stemming: before, so that - // `this` is never reduced to a bare `thi`; after, so that inflections - // such as `uses` collapse onto the listed `use`. + // Stop words are dropped on both sides of stemming: before, so that a + // stop word is never reduced to a meaningless three-letter stem; after, + // so that inflections such as `uses` collapse onto the listed `use`. .filter(|term| term.len() >= 3 && !is_stop_word(term)) .map(stem) .filter(|term| !is_stop_word(term)) @@ -363,4 +401,43 @@ mod tests { assert_eq!(wide_page.len(), 40); assert_eq!(wide_page[..first_page.len()], first_page[..]); } + + #[test] + fn ranking_is_bounded_by_the_catalog_not_by_a_page_ceiling() { + // Every entry matches the query, so the ranked set is the whole + // catalog. A fixed offset/limit ceiling used to make everything past + // rank 1049 unreachable. + const TOTAL: usize = 2_000; + let entries = (0..TOTAL) + .map(|index| entry(&format!("skill-{index:04}"), "Operate deploy pipelines")) + .collect::>(); + let catalog = SkillCatalog { + entries, + warnings: Vec::new(), + }; + + assert_eq!(rank_catalog_all(&catalog, "deploy").len(), TOTAL); + } + + #[test] + fn enumeration_reaches_skills_no_query_term_matches() { + let catalog = SkillCatalog { + entries: vec![ + entry("kanban-groomer", "Reprioritise the backlog board"), + entry("alpha-skill", "Something else"), + entry("hidden-skill", "Something else").disabled(), + ], + warnings: Vec::new(), + }; + + assert!(rank_catalog_all(&catalog, "tidy up my tickets").is_empty()); + assert_eq!( + enumerate_catalog(&catalog) + .into_iter() + .map(|entry| entry.name.as_str()) + .collect::>(), + vec!["alpha-skill", "kanban-groomer"], + "enumeration must be complete, ordered, and still respect availability" + ); + } } diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index b07f3db118..b351e81848 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -1,3 +1,4 @@ +use codex_core_skills::SkillsPreamble; use codex_core_skills::render_task_relevant_skills_body; use codex_extension_api::ContextualUserFragment; use codex_protocol::protocol::SKILLS_INSTRUCTIONS_CLOSE_TAG; @@ -38,6 +39,7 @@ pub(crate) fn available_skills_fragment( catalog: &SkillCatalog, query: &str, limit: usize, + preamble: SkillsPreamble, ) -> Option { let skill_lines = rank_catalog(catalog, query, limit) .into_iter() @@ -54,13 +56,16 @@ pub(crate) fn available_skills_fragment( return None; } - // The `## Skills` developer message already carries the shared - // `### How to use skills` preamble for this turn (both blocks are gated on - // `include_skill_instructions`), so this per-turn fragment renders only the - // ranked matches. Repeating the preamble would burn ~2.5k characters on - // every turn that lexically matches a skill. + // When the `## Skills` developer message is present it already carries the + // shared `### How to use skills` preamble for this turn, so this per-turn + // fragment renders only the ranked matches; repeating the preamble would + // burn ~2.5k characters on every turn that lexically matches a skill. + // `include_skill_instructions` gates both blocks, but it is not the only + // gate: core builds `## Skills` from the *host* outcome while this ranks + // over the *merged* catalog, so a purely remote/executor catalog produces + // no `## Skills` block at all and the caller passes `Missing`. Some(AvailableSkillsFragment { - body: render_task_relevant_skills_body(&skill_lines), + body: render_task_relevant_skills_body(&skill_lines, preamble), }) } diff --git a/codex-rs/ext/skills/src/tools/list.rs b/codex-rs/ext/skills/src/tools/list.rs index a6bf5caa30..935bd63c38 100644 --- a/codex-rs/ext/skills/src/tools/list.rs +++ b/codex-rs/ext/skills/src/tools/list.rs @@ -11,8 +11,8 @@ use serde::Serialize; use crate::ranking::DEFAULT_SKILL_MATCH_LIMIT; use crate::ranking::MAX_SKILL_MATCH_LIMIT; -use crate::ranking::MAX_SKILL_MATCH_OFFSET; -use crate::ranking::rank_catalog; +use crate::ranking::enumerate_catalog; +use crate::ranking::rank_catalog_all; use super::MAX_HANDLE_BYTES; use super::MAX_OUTPUT_BYTES; @@ -30,19 +30,25 @@ const TOOL_NAME: &str = codex_core_skills::SKILLS_LIST_TOOL_NAME; const MAX_QUERY_BYTES: usize = 4_096; const MAX_NAME_BYTES: usize = 256; const MAX_DESCRIPTION_BYTES: usize = 1_024; -/// Deepest rank the tool will materialize for one call: the last reachable page -/// plus one sentinel entry used to detect that more matches exist. -const MAX_INSPECTED_MATCHES: usize = MAX_SKILL_MATCH_OFFSET + MAX_SKILL_MATCH_LIMIT + 1; +/// Headroom left under [`MAX_OUTPUT_BYTES`] for the response fields that are +/// only filled in after the page has been assembled (`has_more`, +/// `next_offset`). +const RESPONSE_ENVELOPE_RESERVE_BYTES: usize = 128; #[derive(Deserialize, JsonSchema)] #[serde(deny_unknown_fields)] struct ListArgs { - query: String, + /// Task or capability to rank the catalog against. Omit it (or pass an + /// empty string) to enumerate the whole catalog alphabetically instead of + /// searching. + query: Option, /// Page size. Defaults to `DEFAULT_SKILL_MATCH_LIMIT`, maximum /// `MAX_SKILL_MATCH_LIMIT`. limit: Option, - /// Rank of the first match to return, for paging past the first page. - /// Defaults to 0, maximum `MAX_SKILL_MATCH_OFFSET`. + /// Rank of the first result to return, for paging past the first page. + /// Defaults to 0. There is no ceiling: any `next_offset` this tool hands + /// back is accepted, and an offset past the end returns an empty page + /// rather than an error. offset: Option, } @@ -60,9 +66,19 @@ struct ListMatch { #[schemars(deny_unknown_fields)] struct ListResponse { matches: Vec, + /// Total number of catalog entries this call can reach, across every page. + total_matches: usize, + /// Whether anything on *this page* was shortened or skipped: a description + /// clipped to its byte cap, or an entry dropped because its handles were + /// unusable. Says nothing about whether further pages exist; that is + /// `has_more`. truncated: bool, + /// Whether this page stopped before the end of the result set. `false` + /// means the results really are exhausted. + has_more: bool, /// Value to pass back as `offset` to continue past this page. `None` once - /// the ranked matches are exhausted. + /// the results are exhausted. Always strictly greater than the `offset` + /// that produced it, and never a value this tool would reject. next_offset: Option, } @@ -81,7 +97,7 @@ impl ToolExecutor for ListTool { skill_function_tool::( TOOL_NAME, &format!( - "Search the full current skill catalog by task or capability. Returns deterministic metadata matches plus opaque authority, package, and main_resource handles for skills.read. Ranking is lexical, so if the top matches look wrong, widen `limit` (default {DEFAULT_SKILL_MATCH_LIMIT}, max {MAX_SKILL_MATCH_LIMIT}) or page deeper by passing the returned `next_offset` back as `offset` (max {MAX_SKILL_MATCH_OFFSET}) before rewording the query. Explicit-only and disabled skills are never returned." + "Search or enumerate the full current skill catalog. Pass `query` to rank by task or capability; omit `query` to list the entire catalog alphabetically, which is the reliable way to reach skills whose wording you cannot guess. Returns deterministic metadata matches plus opaque authority, package, and main_resource handles for skills.read. `total_matches` is the size of the whole result set and `has_more` says whether further pages exist. Ranking is lexical, so if the top matches look wrong, widen `limit` (default {DEFAULT_SKILL_MATCH_LIMIT}, max {MAX_SKILL_MATCH_LIMIT}), page deeper by passing the returned `next_offset` straight back as `offset` (always accepted, no ceiling), or drop `query` and enumerate. Explicit-only and disabled skills are never returned." ), ) } @@ -96,39 +112,41 @@ impl ToolExecutor for ListTool { async fn handle(&self, call: ToolCall) -> Result, FunctionCallError> { let args: ListArgs = parse_args(&call)?; - validate_query(&args.query)?; + let query = args.query.unwrap_or_default(); + validate_query(&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}" ))); } + // No offset ceiling: the model is told to feed `next_offset` straight + // back, so rejecting an offset the tool itself produced would strand it + // mid-walk. An offset past the end simply yields an empty final page. let offset = args.offset.unwrap_or(0); - if offset > MAX_SKILL_MATCH_OFFSET { - return Err(FunctionCallError::RespondToModel(format!( - "offset must be at most {MAX_SKILL_MATCH_OFFSET}" - ))); - } let snapshot = self.context.snapshot(&call.turn_id)?; - // One past the requested page, so `next_offset` can distinguish "the - // page is full" from "there is genuinely nothing after it". - let inspected = offset - .saturating_add(limit) - .saturating_add(1) - .min(MAX_INSPECTED_MATCHES); - let ranked = rank_catalog(&snapshot.catalog, &args.query, inspected); - let ranked_len = ranked.len(); + // A blank query means "enumerate", not "match nothing": lexical ranking + // cannot surface a skill whose wording the request does not share, so + // enumeration is the only path that reaches the whole catalog. + let results = if query.trim().is_empty() { + enumerate_catalog(&snapshot.catalog) + } else { + rank_catalog_all(&snapshot.catalog, &query) + }; + let total_matches = results.len(); let mut response = ListResponse { matches: Vec::new(), + total_matches, truncated: false, + has_more: false, next_offset: None, }; // Ranks consumed from `offset` onwards, including entries skipped for // unusable handles, so the caller can resume exactly where this page // stopped. let mut consumed = 0usize; - for entry in ranked.into_iter().skip(offset) { + for entry in results.into_iter().skip(offset) { if response.matches.len() == limit { break; } @@ -161,34 +179,43 @@ impl ToolExecutor for ListTool { } response.matches.push(candidate); response.truncated |= description_truncated; - if serialized_len(&response)? > MAX_OUTPUT_BYTES { + if serialized_len(&response)? + > MAX_OUTPUT_BYTES.saturating_sub(RESPONSE_ENVELOPE_RESERVE_BYTES) + { response.matches.pop(); - consumed = consumed.saturating_sub(1); + response.truncated = true; + // The cursor must strictly advance. If this entry was the only + // one on the page, rewinding past it would hand back the same + // `offset` and loop a cursor-following model forever, so leave + // it consumed and let the caller resume after it instead. + if !response.matches.is_empty() { + consumed = consumed.saturating_sub(1); + } break; } } let scanned = offset.saturating_add(consumed); - // Either ranked matches remain past this page, or the ranking itself - // was clipped at `inspected` and there may be more beyond it. Both mean - // the model still has somewhere to go. - let has_more = scanned < ranked_len || ranked_len >= inspected; - response.truncated |= has_more; + let has_more = scanned < total_matches; + response.has_more = has_more; response.next_offset = has_more.then_some(scanned); + debug_assert!( + response.next_offset.is_none_or(|next| next > offset), + "next_offset must strictly advance past the requested offset" + ); json_output(&response) } } +/// A blank query is deliberately *not* an error: it selects enumeration. Only +/// genuinely unusable input is rejected. fn validate_query(query: &str) -> Result<(), FunctionCallError> { - if !query.trim().is_empty() - && query.len() <= MAX_QUERY_BYTES - && !query.chars().any(char::is_control) - { + if 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" + "query must contain no control characters and be at most {MAX_QUERY_BYTES} bytes; omit it entirely to enumerate the whole catalog" ))) } diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index c5af8603d2..766370cb08 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -7,6 +7,8 @@ use std::sync::atomic::Ordering; use codex_core::config::Config; use codex_core::config::ConfigBuilder; use codex_core_skills::HostLoadedSkills; +use codex_core_skills::SkillLoadOutcome; +use codex_core_skills::SkillMetadata; use codex_core_skills::SkillsLoadInput; use codex_core_skills::SkillsManager; use codex_core_skills::injection::InjectedHostSkillPrompts; @@ -298,6 +300,10 @@ async fn installed_extension_injects_available_catalog_and_selected_entrypoint() .await; let turn_store = ExtensionData::new("turn-1"); + // Core renders the `## Skills` developer block from the host outcome; the + // preamble may only be dropped from the task-relevant fragment when that + // block actually exists. + turn_store.insert(host_loaded_skills("lint-fix")); let fragments = registry.turn_input_contributors()[0] .contribute( TurnInputContext { @@ -348,6 +354,7 @@ async fn installed_extension_injects_available_catalog_and_selected_entrypoint() // list (without re-injecting the skill body, which the turn-1 entrypoint // already covered). let matching_turn_store = ExtensionData::new("turn-2"); + matching_turn_store.insert(host_loaded_skills("lint-fix")); let matching_fragments = registry.turn_input_contributors()[0] .contribute( TurnInputContext { @@ -374,7 +381,8 @@ async fn installed_extension_injects_available_catalog_and_selected_entrypoint() // every matching turn. assert!(matching_body.contains(codex_core_skills::TASK_RELEVANT_SKILLS_HEADING)); assert!(!matching_body.contains("### How to use skills")); - assert!(!matching_body.contains(codex_core_skills::SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS)); + assert!(matching_body.contains(codex_core_skills::TASK_RELEVANT_SKILLS_INTRO)); + assert!(!matching_body.contains(codex_core_skills::SKILLS_HOW_TO_USE_TAIL_WITH_ABSOLUTE_PATHS)); // Turns whose text shares no token with any skill contribute nothing. Note // that this also swallows short continuations ("continue", "yes") because @@ -467,10 +475,18 @@ async fn skills_list_pages_past_the_default_limit() -> TestResult { ); // Default page: five matches, explicitly continuable. - let first = call_tool(Arc::clone(&list_tool), "turn-page", json!({ "query": "lint" })).await?; + let first = call_tool( + Arc::clone(&list_tool), + "turn-page", + json!({ "query": "lint" }), + ) + .await?; let first_names = match_names(&first); assert_eq!(first_names.len(), 5); - assert_eq!(first["truncated"], json!(true)); + // `truncated` is about this page's content, `has_more` about the walk. + assert_eq!(first["truncated"], json!(false)); + assert_eq!(first["has_more"], json!(true)); + assert_eq!(first["total_matches"], json!(TOTAL)); assert_eq!(first["next_offset"], json!(5)); // Walk the remainder with the returned cursor; every skill is reachable. @@ -507,6 +523,7 @@ async fn skills_list_pages_past_the_default_limit() -> TestResult { .await?; assert_eq!(match_names(&wide).len(), TOTAL); assert_eq!(wide["next_offset"], Value::Null); + assert_eq!(wide["has_more"], json!(false)); assert_eq!(wide["truncated"], json!(false)); // Paging past the end is well-formed and terminates. @@ -522,6 +539,232 @@ async fn skills_list_pages_past_the_default_limit() -> TestResult { Ok(()) } +#[tokio::test] +async fn skills_list_reaches_every_entry_of_a_large_catalog_and_enumerates_unguessable_ones() +-> TestResult { + // The three properties this tool has to hold at "thousands of skills" + // scale: + // 1. every rank is reachable by walking the cursor, + // 2. a cursor the tool emits is never a cursor the tool rejects, + // 3. a skill nobody can guess a query term for is still reachable. + const TOTAL: usize = 2_000; + let mut entries = (0..TOTAL) + .map(|index| { + SkillCatalogEntry::new( + SkillPackageId(format!("host/skill-{index:04}")), + SkillAuthority::new(SkillSourceKind::Host, "host"), + format!("skill-{index:04}"), + "Ship a deploy to production.", + SkillResourceId(format!("skill-{index:04}/SKILL.md")), + ) + }) + .collect::>(); + entries.push(SkillCatalogEntry::new( + SkillPackageId("host/kanban-groomer".to_string()), + SkillAuthority::new(SkillSourceKind::Host, "host"), + "kanban-groomer", + "Reprioritise the backlog board", + SkillResourceId("kanban-groomer/SKILL.md".to_string()), + )); + let provider = Arc::new(StaticSkillProvider { + catalog: SkillCatalog { + entries, + warnings: Vec::new(), + }, + read_requests: Arc::new(Mutex::new(Vec::new())), + }); + let providers = SkillProviders::new().with_host_provider(provider); + let mut builder = ExtensionRegistryBuilder::new(); + install_with_providers(&mut builder, providers); + let registry = builder.build(); + let session_store = ExtensionData::new("session"); + let thread_store = ExtensionData::new("thread"); + let session_source = SessionSource::Cli; + let config = default_config().await?; + registry.thread_lifecycle_contributors()[0] + .on_thread_start(ThreadStartInput { + config: &config, + session_source: &session_source, + persistent_thread_state_available: true, + session_store: &session_store, + thread_store: &thread_store, + }) + .await; + let turn_store = ExtensionData::new("turn-scale"); + registry.turn_input_contributors()[0] + .contribute( + TurnInputContext { + turn_id: "turn-scale".to_string(), + user_input: vec![UserInput::Text { + text: "hello".to_string(), + text_elements: Vec::new(), + }], + environments: Vec::new(), + }, + &session_store, + &thread_store, + &turn_store, + ) + .await; + let list_tool = find_tool( + ®istry, + &session_store, + &thread_store, + ToolName::namespaced("skills", "list"), + ); + + // (1)+(2): walk the whole ranked result set with the cursor the tool hands + // back, never adjusting it. + let mut seen = std::collections::HashSet::new(); + let mut offset = 0u64; + let mut calls = 0usize; + loop { + let page = call_tool( + Arc::clone(&list_tool), + "turn-scale", + json!({ "query": "deploy", "limit": 50, "offset": offset }), + ) + .await?; + calls += 1; + assert!(calls < 200, "cursor walk should terminate"); + assert_eq!(page["total_matches"], json!(TOTAL)); + seen.extend(match_names(&page)); + match page["next_offset"].as_u64() { + Some(next) => { + assert!(next > offset, "cursor must strictly advance: {page}"); + assert_eq!(page["has_more"], json!(true)); + offset = next; + } + None => { + assert_eq!(page["has_more"], json!(false)); + break; + } + } + } + assert_eq!(seen.len(), TOTAL); + assert!(seen.contains("skill-1999"), "the tail must be reachable"); + + // (3): no query term reaches the odd-one-out, but enumeration does. + for query in ["tidy up my tickets", "tickets", "skill", "the", "*"] { + let page = call_tool( + Arc::clone(&list_tool), + "turn-scale", + json!({ "query": query, "limit": 50 }), + ) + .await?; + assert!( + !match_names(&page).contains(&"kanban-groomer".to_string()), + "{query} unexpectedly matched" + ); + } + let mut enumerated = Vec::new(); + let mut offset = 0u64; + loop { + // Both spellings of "no query" must work: omitted and blank. + let arguments = if offset == 0 { + json!({ "limit": 50 }) + } else { + json!({ "query": "", "limit": 50, "offset": offset }) + }; + let page = call_tool(Arc::clone(&list_tool), "turn-scale", arguments).await?; + assert_eq!(page["total_matches"], json!(TOTAL + 1)); + enumerated.extend(match_names(&page)); + match page["next_offset"].as_u64() { + Some(next) => offset = next, + None => break, + } + } + assert_eq!(enumerated.len(), TOTAL + 1); + assert!(enumerated.contains(&"kanban-groomer".to_string())); + assert!( + enumerated.windows(2).all(|pair| pair[0] <= pair[1]), + "enumeration must be alphabetical" + ); + + // A deep offset is answered, not rejected. + let deep = call_tool( + Arc::clone(&list_tool), + "turn-scale", + json!({ "query": "deploy", "limit": 50, "offset": 1_000 }), + ) + .await?; + let deep_cursor = deep["next_offset"].as_u64().expect("cursor past rank 1000"); + let after_deep = call_tool( + Arc::clone(&list_tool), + "turn-scale", + json!({ "query": "deploy", "limit": 50, "offset": deep_cursor }), + ) + .await?; + assert_eq!(match_names(&after_deep).len(), 50); + + Ok(()) +} + +#[tokio::test] +async fn task_relevant_fragment_carries_the_rules_when_no_host_skills_block_exists() -> TestResult { + // Core builds `## Skills` from the host outcome. With an empty host outcome + // and a remote-only catalog there is no such section, so the per-turn + // fragment must not point at one - it has to carry the rules itself. + let remote_provider = Arc::new(StaticSkillProvider { + catalog: SkillCatalog { + entries: vec![test_entry( + SkillSourceKind::Remote, + "remote", + "remote/lint-fix", + "lint-fix/SKILL.md", + )], + warnings: Vec::new(), + }, + read_requests: Arc::new(Mutex::new(Vec::new())), + }); + let providers = SkillProviders::new().with_remote_provider(remote_provider); + let mut builder = ExtensionRegistryBuilder::new(); + install_with_providers(&mut builder, providers); + let registry = builder.build(); + let session_store = ExtensionData::new("session"); + let thread_store = ExtensionData::new("thread"); + let session_source = SessionSource::Cli; + let config = default_config().await?; + registry.thread_lifecycle_contributors()[0] + .on_thread_start(ThreadStartInput { + config: &config, + session_source: &session_source, + persistent_thread_state_available: true, + session_store: &session_store, + thread_store: &thread_store, + }) + .await; + + let turn_store = ExtensionData::new("turn-remote-only"); + let fragments = registry.turn_input_contributors()[0] + .contribute( + TurnInputContext { + turn_id: "turn-remote-only".to_string(), + user_input: vec![UserInput::Text { + text: "fix the lint errors".to_string(), + text_elements: Vec::new(), + }], + environments: Vec::new(), + }, + &session_store, + &thread_store, + &turn_store, + ) + .await; + + assert_eq!(1, fragments.len()); + let body = fragments[0].render(); + assert!(body.contains("lint-fix")); + assert!(!body.contains(codex_core_skills::TASK_RELEVANT_SKILLS_INTRO)); + assert!(body.contains(codex_core_skills::TASK_RELEVANT_SKILLS_STANDALONE_INTRO)); + assert!(body.contains("### How to use skills")); + assert!(body.contains(codex_core_skills::SKILLS_DISCOVERY_CATALOG_SEARCH)); + assert!(body.contains(codex_core_skills::SKILLS_TRIGGER_RULES)); + assert!(body.contains(codex_core_skills::SKILLS_HOW_TO_USE_TAIL_WITH_ABSOLUTE_PATHS)); + + Ok(()) +} + #[tokio::test] async fn deferred_skill_is_searchable_and_loadable_but_disabled_skill_is_not() -> TestResult { let read_requests = Arc::new(Mutex::new(Vec::new())); @@ -821,7 +1064,7 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { assert_eq!( oversized_list_query, Err(FunctionCallError::RespondToModel( - "query must contain non-whitespace text, contain no control characters, and be at most 4096 bytes" + "query must contain no control characters and be at most 4096 bytes; omit it entirely to enumerate the whole catalog" .to_string() )) ); @@ -837,18 +1080,17 @@ async fn model_tools_route_exact_packages_and_bound_results() -> TestResult { "limit must be between 1 and 50".to_string() )) ); - let oversized_list_offset = call_tool( + // A deep offset is answered with an empty final page, never rejected: the + // tool must accept any cursor it could itself have produced. + let deep_list_offset = call_tool( Arc::clone(&list_tool), "turn-tools", json!({ "query": "package", "offset": 1_001 }), ) - .await; - assert_eq!( - oversized_list_offset, - Err(FunctionCallError::RespondToModel( - "offset must be at most 1000".to_string() - )) - ); + .await?; + assert_eq!(deep_list_offset["matches"], json!([])); + assert_eq!(deep_list_offset["has_more"], json!(false)); + assert_eq!(deep_list_offset["next_offset"], Value::Null); let custom_output = call_tool( Arc::clone(&search_tool), @@ -1249,6 +1491,28 @@ fn test_entry( .with_display_path(format!("skill://{package_id}/SKILL.md")) } +/// A host outcome carrying one implicitly-invocable skill, i.e. the state in +/// which core does render a `## Skills` developer block. +fn host_loaded_skills(name: &str) -> HostLoadedSkills { + let path = codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path( + std::env::temp_dir().join(name).join("SKILL.md"), + ) + .unwrap_or_else(|err| panic!("temp dir should be absolute: {err}")); + let mut outcome = SkillLoadOutcome::default(); + outcome.skills.push(SkillMetadata { + name: name.to_string(), + description: "Fix lint errors.".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + path_to_skills_md: path, + scope: codex_protocol::protocol::SkillScope::Repo, + plugin_id: None, + }); + HostLoadedSkills::new(Arc::new(outcome)) +} + async fn default_config() -> std::io::Result { let codex_home = test_codex_home(); std::fs::create_dir_all(&codex_home)?;