diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b068577b2..5262dc5e12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,8 +42,60 @@ 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 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 + skills alongside `truncated = 0`. + ### Fixed +- The starter-list cap only applies when it is actually needed: a catalog that + 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. +- 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 1d695d30a6..f9530c18e9 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", @@ -3751,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/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/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 0390302afd..eee8281f6b 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -22,13 +22,36 @@ pub use model::SkillMetadata; pub use model::SkillPolicy; pub use model::filter_skill_load_outcome_for_product; pub use render::AvailableSkills; -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::MAX_STARTER_SKILLS; +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; 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 a31f80b670..0d0df892de 100644 --- a/codex-rs/core-skills/src/render.rs +++ b/codex-rs/core-skills/src/render.rs @@ -1,11 +1,13 @@ use std::collections::HashMap; use std::collections::HashSet; +use std::collections::VecDeque; use std::path::Component; 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; @@ -16,18 +18,59 @@ 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; +/// 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 = "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. -- 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. @@ -41,10 +84,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. -- 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. @@ -59,13 +101,147 @@ pub const SKILLS_HOW_TO_USE_WITH_ALIASES: &str = r###"- Discovery: The list abov - 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 fn render_available_skills_body(skill_root_lines: &[String], skill_lines: &[String]) -> String { +/// 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."; +/// 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. +/// +/// 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()); + 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")) +} + +/// 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()); } @@ -73,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")) } @@ -119,6 +307,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, @@ -137,9 +326,43 @@ 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, } +/// 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()) @@ -160,14 +383,46 @@ 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 { - let skills = outcome.allowed_skills_for_implicit_invocation(); + 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 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::>(); if skills.is_empty() { record_skill_render_side_effects( side_effects, /*total_count*/ 0, /*included_count*/ 0, + /*deferred_count*/ 0, /*omitted_count*/ 0, /*truncated_description_chars*/ 0, ); @@ -182,7 +437,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,10 +450,44 @@ pub fn build_available_skills( absolute }; + 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, @@ -241,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) @@ -259,19 +553,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" ); } } @@ -291,6 +590,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, ) { @@ -307,9 +607,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( @@ -426,6 +735,7 @@ fn skill_render_report( SkillRenderReport { total_count, included_count, + deferred_count: 0, omitted_count, truncated_description_chars, truncated_description_count, @@ -433,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 } } @@ -786,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)) @@ -893,6 +1214,124 @@ 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 + || whole_catalog_can_be_listed(outcome, skills, budget) + { + return skills.len(); + } + + max_starter_skills +} + +/// 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).minimum_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)) + .minimum_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 +/// `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, @@ -1073,7 +1512,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() ) ); @@ -1195,6 +1634,7 @@ mod tests { let rendered = build_available_skills( &outcome, SkillMetadataBudget::Characters(usize::MAX), + SkillCatalogSearch::Available, SkillRenderSideEffects::None, ) .expect("skills should render"); @@ -1203,6 +1643,366 @@ mod tests { assert_eq!(rendered.report.included_count, 2); } + fn scale_outcome(count: usize) -> SkillLoadOutcome { + let skills = (0..count) + .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::>(); + 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, + // 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"); + + 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); + // 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, + 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 + // 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, 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_TAIL_WITH_ABSOLUTE_PATHS)); + assert!(!body.contains(SKILLS_HOW_TO_USE_TAIL_WITH_ALIASES)); + assert!( + body.len() + < 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 + // 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( @@ -1216,18 +2016,22 @@ mod tests { }) .collect::>(); let outcome = outcome_with_roots(skills.clone(), vec![root]); - let absolute_minimum = skills.iter().fold(0usize, |cost, skill| { + let starter = starter_skills(&skills, MAX_STARTER_SKILLS) + .into_iter() + .cloned() + .collect::>(); + 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, - &skills, + &starter, SkillMetadataBudget::Characters(usize::MAX), ) .expect("alias plan should build"); - let alias_minimum = 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)), @@ -1241,11 +2045,17 @@ mod tests { let rendered = build_available_skills( &outcome, SkillMetadataBudget::Characters(alias_minimum), + SkillCatalogSearch::Available, SkillRenderSideEffects::None, ) .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 +2072,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/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/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..b56d981324 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -10,7 +10,9 @@ 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::MAX_STARTER_SKILLS; use crate::skills::render::SkillMetadataBudget; use crate::test_support::models_manager_with_provider; use crate::tools::format_exec_output_str; @@ -96,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; @@ -9126,6 +9129,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 +9196,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, }, @@ -9204,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 b11f79260a..84e3311961 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -8,6 +8,7 @@ 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; @@ -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 ac25d36c98..692a862351 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -1629,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 b52a110c4f..ca8ac76aad 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] @@ -25,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/catalog.rs b/codex-rs/ext/skills/src/catalog.rs index 2fd633bf2c..f9c5bc19cb 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,22 +161,17 @@ pub struct SkillCatalog { impl SkillCatalog { pub fn extend(&mut self, other: SkillCatalog) { - for entry in other.entries { - self.push_entry(entry); - } - self.warnings.extend(other.warnings); - } - - pub fn push_entry(&mut self, entry: SkillCatalogEntry) { - if self + let mut existing = self .entries .iter() - .any(|existing| existing.authority == entry.authority && existing.id == entry.id) - { - return; + .map(|entry| (entry.authority.clone(), entry.id.clone())) + .collect::>(); + for entry in other.entries { + if existing.insert((entry.authority.clone(), entry.id.clone())) { + self.entries.push(entry); + } } - - 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..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; @@ -32,6 +33,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 +152,14 @@ 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, + skills_preamble_for(host_loaded_skills.as_deref()), + ) { fragments.push(Box::new(fragment)); } @@ -233,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/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/provider/host.rs b/codex-rs/ext/skills/src/provider/host.rs index 9f169ceef4..43d08a0683 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 + // 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 { + 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 new file mode 100644 index 0000000000..29e66fb243 --- /dev/null +++ b/codex-rs/ext/skills/src/ranking.rs @@ -0,0 +1,443 @@ +use std::collections::HashSet; + +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; +/// 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; + +/// 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; +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 +} + +/// 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() { + 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().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( + 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()) + // 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)) + .take(max_terms) + .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 != '$' +} + +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()); + } + + #[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[..]); + } + + #[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 41c14c82ac..b351e81848 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -1,12 +1,16 @@ -use codex_core_skills::render_available_skills_body; +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; 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,57 +35,61 @@ 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, + preamble: SkillsPreamble, +) -> 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." - )); - } + // 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_available_skills_body(&[], &skill_lines), + body: render_task_relevant_skills_body(&skill_lines, preamble), }) } fn render_skill_line(entry: &crate::catalog::SkillCatalogEntry, description: &str) -> 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..935bd63c38 --- /dev/null +++ b/codex-rs/ext/skills/src/tools/list.rs @@ -0,0 +1,221 @@ +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::enumerate_catalog; +use crate::ranking::rank_catalog_all; + +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 = 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; +/// 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 { + /// 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 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, +} + +#[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, + /// 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 results are exhausted. Always strictly greater than the `offset` + /// that produced it, and never a value this tool would reject. + next_offset: Option, +} + +#[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, + &format!( + "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." + ), + ) + } + + 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)?; + 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); + + let snapshot = self.context.snapshot(&call.turn_id)?; + // 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 results.into_iter().skip(offset) { + if response.matches.len() == limit { + break; + } + consumed = consumed.saturating_add(1); + 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.saturating_sub(RESPONSE_ENVELOPE_RESERVE_BYTES) + { + response.matches.pop(); + 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); + 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.len() <= MAX_QUERY_BYTES && !query.chars().any(char::is_control) { + return Ok(()); + } + + Err(FunctionCallError::RespondToModel(format!( + "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/src/tools/mod.rs b/codex-rs/ext/skills/src/tools/mod.rs index dd7f19ca05..1f91f35a92 100644 --- a/codex-rs/ext/skills/src/tools/mod.rs +++ b/codex-rs/ext/skills/src/tools/mod.rs @@ -24,11 +24,15 @@ use crate::catalog::SkillSourceKind; use crate::state::SkillsThreadState; use crate::state::SkillsToolSnapshot; +mod list; 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; @@ -38,6 +42,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..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 { @@ -344,26 +350,417 @@ 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"); + matching_turn_store.insert(host_loaded_skills("lint-fix")); + 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_eq!(1, next_fragments.len()); - assert_eq!("developer", next_fragments[0].role()); - assert!(next_fragments[0].render().contains("lint-fix")); + 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::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 + // 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); + // `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. + 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["has_more"], json!(false)); + 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(()) +} + +#[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(()) } @@ -442,17 +839,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 +855,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 +971,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 +992,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 +1007,93 @@ 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 no control characters and be at most 4096 bytes; omit it entirely to enumerate the whole catalog" + .to_string() + )) + ); + let oversized_list_limit = call_tool( + Arc::clone(&list_tool), + "turn-tools", + json!({ "query": "package", "limit": 51 }), + ) + .await; + assert_eq!( + oversized_list_limit, + Err(FunctionCallError::RespondToModel( + "limit must be between 1 and 50".to_string() + )) + ); + // 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!(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(&tools[0]), + Arc::clone(&search_tool), "turn-tools", json!({ "authority": { @@ -621,7 +1119,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 +1136,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 +1153,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 +1166,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 +1183,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 +1200,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 +1239,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 +1274,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 +1298,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" }, @@ -993,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)?; @@ -1010,6 +1530,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/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(), 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"; diff --git a/sdk/typescript/tests/run.test.ts b/sdk/typescript/tests/run.test.ts index 3698950e9d..a2078e40ca 100644 --- a/sdk/typescript/tests/run.test.ts +++ b/sdk/typescript/tests/run.test.ts @@ -608,8 +608,13 @@ 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. ``) 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(); await close();