Skip to content
Merged
9 changes: 2 additions & 7 deletions codex-rs/core-skills/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -712,13 +712,8 @@ async fn parse_skill_file(

validate_len(&base_name, MAX_NAME_LEN, "name")?;
validate_len(&name, MAX_QUALIFIED_NAME_LEN, "qualified name")?;
validate_len(&description, MAX_DESCRIPTION_LEN, "description")?;
if let Some(short_description) = short_description.as_deref() {
validate_len(
short_description,
MAX_SHORT_DESCRIPTION_LEN,
"metadata.short-description",
)?;
if description.is_empty() {
return Err(SkillParseError::MissingField("description"));
}

let resolved_path = canonicalize_for_skill_identity(fs, path).await;
Expand Down
28 changes: 16 additions & 12 deletions codex-rs/core-skills/src/loader_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1548,7 +1548,7 @@ async fn preserves_block_scalar_body_while_repairing_other_fields() {
}

#[tokio::test]
async fn enforces_short_description_length_limits() {
async fn preserves_overlong_short_descriptions() {
let codex_home = tempfile::tempdir().expect("tempdir");
let skill_dir = codex_home.path().join("skills/demo");
fs::create_dir_all(&skill_dir).unwrap();
Expand All @@ -1560,15 +1560,13 @@ async fn enforces_short_description_length_limits() {

let cfg = make_config(&codex_home).await;
let outcome = load_skills_for_test(&cfg).await;
assert_eq!(outcome.skills.len(), 0);
assert_eq!(outcome.errors.len(), 1);
assert!(
outcome.errors[0]
.message
.contains("invalid metadata.short-description"),
"expected length error, got: {:?}",
outcome.errors.is_empty(),
"unexpected errors: {:?}",
outcome.errors
);
assert_eq!(outcome.skills.len(), 1);
assert_eq!(outcome.skills[0].short_description, Some(too_long));
}

#[tokio::test]
Expand Down Expand Up @@ -1600,7 +1598,7 @@ async fn skips_hidden_and_invalid() {
}

#[tokio::test]
async fn enforces_length_limits() {
async fn preserves_overlong_descriptions() {
let codex_home = tempfile::tempdir().expect("tempdir");
let max_desc = "\u{1F4A1}".repeat(MAX_DESCRIPTION_LEN);
write_skill(&codex_home, "max-len", "max-len", &max_desc);
Expand All @@ -1617,12 +1615,18 @@ async fn enforces_length_limits() {
let too_long_desc = "\u{1F4A1}".repeat(MAX_DESCRIPTION_LEN + 1);
write_skill(&codex_home, "too-long", "too-long", &too_long_desc);
let outcome = load_skills_for_test(&cfg).await;
assert_eq!(outcome.skills.len(), 1);
assert_eq!(outcome.errors.len(), 1);
assert!(
outcome.errors[0].message.contains("invalid description"),
"expected length error"
outcome.errors.is_empty(),
"unexpected errors: {:?}",
outcome.errors
);
assert_eq!(outcome.skills.len(), 2);
let too_long_skill = outcome
.skills
.iter()
.find(|skill| skill.name == "too-long")
.expect("too-long skill");
assert_eq!(too_long_skill.description, too_long_desc);
}

#[tokio::test]
Expand Down
54 changes: 50 additions & 4 deletions codex-rs/core-skills/src/render.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::borrow::Cow;
use std::collections::HashMap;
use std::collections::HashSet;
use std::path::Component;
Expand All @@ -16,6 +17,8 @@ use codex_utils_output_truncation::approx_token_count;

const DEFAULT_SKILL_METADATA_CHAR_BUDGET: usize = 8_000;
const SKILL_METADATA_CONTEXT_WINDOW_PERCENT: usize = 2;
const MAX_DEFAULT_CONTEXT_SKILL_DESCRIPTION_CHARS: usize = 1_024;
const TRUNCATED_SKILL_DESCRIPTION_SUFFIX: &str = "...";
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. Codex can still see every skill, but some descriptions are shorter. Disable unused skills or plugins to leave more room for the rest.";
Expand Down Expand Up @@ -446,7 +449,7 @@ impl SkillRenderReport {

struct SkillLine<'a> {
name: &'a str,
description: &'a str,
description: Cow<'a, str>,
path: String,
}

Expand Down Expand Up @@ -485,9 +488,10 @@ impl<'a> SkillLine<'a> {
}

fn with_path(skill: &'a SkillMetadata, path: String) -> Self {
let description = truncate_default_context_skill_description(skill.description.as_str());
Self {
name: skill.name.as_str(),
description: skill.description.as_str(),
description,
path,
}
}
Expand All @@ -505,7 +509,7 @@ impl<'a> SkillLine<'a> {
}

fn render_full(&self) -> String {
self.render_with_description(self.description)
self.render_with_description(self.description.as_ref())
}

fn render_minimum(&self) -> String {
Expand All @@ -524,7 +528,7 @@ impl<'a> SkillLine<'a> {
format!("- {}: (file: {})", self.name, self.path)
} else {
let end = self.rendered_description_prefix_len(description_chars);
let description = &self.description[..end];
let description = &self.description.as_ref()[..end];
format!("- {}: {} (file: {})", self.name, description, self.path)
}
}
Expand All @@ -538,6 +542,26 @@ impl<'a> SkillLine<'a> {
}
}

fn truncate_default_context_skill_description(description: &str) -> Cow<'_, str> {
if description
.char_indices()
.nth(MAX_DEFAULT_CONTEXT_SKILL_DESCRIPTION_CHARS)
.is_none()
{
return Cow::Borrowed(description);
}

let prefix_chars = MAX_DEFAULT_CONTEXT_SKILL_DESCRIPTION_CHARS
.saturating_sub(TRUNCATED_SKILL_DESCRIPTION_SUFFIX.chars().count());
let prefix_end = description
.char_indices()
.nth(prefix_chars)
.map_or(description.len(), |(index, _)| index);
let mut truncated = description[..prefix_end].to_string();
truncated.push_str(TRUNCATED_SKILL_DESCRIPTION_SUFFIX);
Cow::Owned(truncated)
}

impl<'a> DescriptionBudgetLine<'a> {
fn new(line: &'a SkillLine<'a>, budget: SkillMetadataBudget) -> Self {
let minimum_line = line.render_minimum();
Expand Down Expand Up @@ -1030,6 +1054,28 @@ mod tests {
);
}

#[test]
fn default_context_caps_descriptions_without_mutating_metadata() {
let description = "\u{1F4A1}".repeat(MAX_DEFAULT_CONTEXT_SKILL_DESCRIPTION_CHARS + 1);
let skill = make_skill_with_description("long-skill", SkillScope::Repo, &description);
let expected_description = "\u{1F4A1}".repeat(
MAX_DEFAULT_CONTEXT_SKILL_DESCRIPTION_CHARS
- TRUNCATED_SKILL_DESCRIPTION_SUFFIX.chars().count(),
) + TRUNCATED_SKILL_DESCRIPTION_SUFFIX;

let rendered = build_available_skills_from_metadata(
std::slice::from_ref(&skill),
SkillMetadataBudget::Characters(usize::MAX),
)
.expect("skill should render");

assert_eq!(skill.description, description);
assert_eq!(
rendered.skill_lines,
vec![expected_skill_line(&skill, &expected_description)]
);
}

#[test]
fn budgeted_rendering_truncates_descriptions_equally_before_omitting_skills() {
let alpha = make_skill_with_description("alpha-skill", SkillScope::Repo, "abcdef");
Expand Down
12 changes: 8 additions & 4 deletions codex-rs/ext/skills/src/provider/orchestrator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ const MAX_RESOURCE_PAGES: usize = 10;
const MAX_ORCHESTRATOR_SKILLS: usize = 100;
const MAX_SKILL_NAME_CHARS: usize = 64;
const MAX_QUALIFIED_SKILL_NAME_CHARS: usize = 128;
const MAX_SKILL_DESCRIPTION_CHARS: usize = 1_024;
const MAX_SKILL_PACKAGE_URI_CHARS: usize = 1_024;
const MAX_SKILL_RESOURCE_URI_CHARS: usize = 2_048;
const MAX_SKILL_RESOURCE_CONTENT_BYTES: usize = 1024 * 1024;
Expand Down Expand Up @@ -308,12 +307,17 @@ fn normalized_label(value: &str, max_chars: usize) -> Option<String> {
}

fn normalized_description(value: &str) -> Option<String> {
normalized_single_line(value, MAX_SKILL_DESCRIPTION_CHARS).map(|value| {
let value = value.split_whitespace().collect::<Vec<_>>().join(" ");
Comment thread
charlesgong-openai marked this conversation as resolved.
if value.chars().any(char::is_control) {
return None;
}

Some(
value
.replace('&', "&amp;")
.replace('<', "&lt;")
.replace('>', "&gt;")
})
.replace('>', "&gt;"),
)
}

fn normalized_single_line(value: &str, max_chars: usize) -> Option<String> {
Expand Down
27 changes: 26 additions & 1 deletion codex-rs/ext/skills/src/render.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::borrow::Cow;

use codex_utils_string::take_bytes_at_char_boundary;

use crate::catalog::SkillCatalog;
Expand All @@ -7,6 +9,8 @@ use crate::fragments::AvailableSkillsInstructions;

const MAX_AVAILABLE_SKILLS_BYTES: usize = 8_000;
const MAX_MAIN_PROMPT_BYTES: usize = 8_000;
const MAX_CATALOG_SKILL_DESCRIPTION_CHARS: usize = 1_024;
const TRUNCATED_SKILL_DESCRIPTION_SUFFIX: &str = "...";
pub(crate) const MAX_SKILL_NAME_BYTES: usize = 256;
pub(crate) const MAX_SKILL_PATH_BYTES: usize = 1_024;

Expand All @@ -26,7 +30,8 @@ pub(crate) fn available_skills_fragment(
.short_description
.as_deref()
.unwrap_or(entry.description.as_str());
let line = render_skill_line(entry, description);
let description = truncate_catalog_skill_description(description);
let line = render_skill_line(entry, description.as_ref());
let next_bytes = total_bytes.saturating_add(line.len());
if next_bytes > MAX_AVAILABLE_SKILLS_BYTES {
omitted = omitted.saturating_add(1);
Expand All @@ -49,6 +54,26 @@ pub(crate) fn available_skills_fragment(
Some(AvailableSkillsInstructions::from_skill_lines(skill_lines))
}

pub(crate) fn truncate_catalog_skill_description(description: &str) -> Cow<'_, str> {
if description
.char_indices()
.nth(MAX_CATALOG_SKILL_DESCRIPTION_CHARS)
.is_none()
{
return Cow::Borrowed(description);
}

let prefix_chars = MAX_CATALOG_SKILL_DESCRIPTION_CHARS
.saturating_sub(TRUNCATED_SKILL_DESCRIPTION_SUFFIX.chars().count());
let prefix_end = description
.char_indices()
.nth(prefix_chars)
.map_or(description.len(), |(index, _)| index);
let mut truncated = description[..prefix_end].to_string();
truncated.push_str(TRUNCATED_SKILL_DESCRIPTION_SUFFIX);
Cow::Owned(truncated)
}

fn render_skill_line(entry: &SkillCatalogEntry, description: &str) -> String {
let locator_kind = match &entry.authority.kind {
SkillSourceKind::Host => "file",
Expand Down
3 changes: 2 additions & 1 deletion codex-rs/ext/skills/src/tools/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use serde::Deserialize;
use serde::Serialize;

use crate::catalog::SkillCatalogEntry;
use crate::render::truncate_catalog_skill_description;
use crate::render::truncate_utf8_to_bytes;

use super::MAX_HANDLE_BYTES;
Expand Down Expand Up @@ -95,7 +96,7 @@ fn listed_skill(entry: SkillCatalogEntry) -> Option<ListedSkill> {
authority,
package: entry.id.0,
name: entry.name,
description: entry.description,
description: truncate_catalog_skill_description(&entry.description).into_owned(),
main_resource: entry.main_prompt.as_str().to_string(),
})
}
Expand Down
Loading
Loading