From 2f8a44baea790a8bb2ebb2bc5f31a0cd9543c484 Mon Sep 17 00:00:00 2001 From: alexsong-oai Date: Tue, 27 Jan 2026 12:06:40 -0800 Subject: [PATCH] Remove load from SKILL.toml fallback (#10007) --- codex-rs/core/src/skills/loader.rs | 243 ++++++++++------------------- 1 file changed, 84 insertions(+), 159 deletions(-) diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index 221639085..a411dd8bc 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -52,7 +52,6 @@ struct Interface { const SKILLS_FILENAME: &str = "SKILL.md"; const SKILLS_JSON_FILENAME: &str = "SKILL.json"; -const SKILLS_TOML_FILENAME: &str = "SKILL.toml"; const SKILLS_DIR_NAME: &str = "skills"; const MAX_NAME_LEN: usize = 64; const MAX_DESCRIPTION_LEN: usize = 1024; @@ -373,92 +372,60 @@ fn parse_skill_file(path: &Path, scope: SkillScope) -> Result Option { // Fail open: optional interface metadata should not block loading SKILL.md. let skill_dir = skill_path.parent()?; - let interface_paths = [ - (skill_dir.join(SKILLS_JSON_FILENAME), InterfaceFormat::Json), - (skill_dir.join(SKILLS_TOML_FILENAME), InterfaceFormat::Toml), - ]; - - for (interface_path, format) in interface_paths { - if !interface_path.exists() { - continue; - } - - let contents = match fs::read_to_string(&interface_path) { - Ok(contents) => contents, - Err(error) => { - tracing::warn!( - "ignoring {path}: failed to read {label}: {error}", - path = interface_path.display(), - label = format.label() - ); - continue; - } - }; - let parsed: SkillMetadataFile = match format.parse(&contents) { - Ok(parsed) => parsed, - Err(error) => { - tracing::warn!( - "ignoring {path}: invalid {label}: {error}", - path = interface_path.display(), - label = format.label() - ); - continue; - } - }; - let interface = parsed.interface?; - - let interface = SkillInterface { - display_name: resolve_str( - interface.display_name, - MAX_NAME_LEN, - "interface.display_name", - ), - short_description: resolve_str( - interface.short_description, - MAX_SHORT_DESCRIPTION_LEN, - "interface.short_description", - ), - icon_small: resolve_asset_path(skill_dir, "interface.icon_small", interface.icon_small), - icon_large: resolve_asset_path(skill_dir, "interface.icon_large", interface.icon_large), - brand_color: resolve_color_str(interface.brand_color, "interface.brand_color"), - default_prompt: resolve_str( - interface.default_prompt, - MAX_DEFAULT_PROMPT_LEN, - "interface.default_prompt", - ), - }; - let has_fields = interface.display_name.is_some() - || interface.short_description.is_some() - || interface.icon_small.is_some() - || interface.icon_large.is_some() - || interface.brand_color.is_some() - || interface.default_prompt.is_some(); - return if has_fields { Some(interface) } else { None }; + let interface_path = skill_dir.join(SKILLS_JSON_FILENAME); + if !interface_path.exists() { + return None; } - None -} - -#[derive(Clone, Copy)] -enum InterfaceFormat { - Json, - Toml, -} - -impl InterfaceFormat { - fn label(self) -> &'static str { - match self { - InterfaceFormat::Json => "SKILL.json", - InterfaceFormat::Toml => "SKILL.toml", + let contents = match fs::read_to_string(&interface_path) { + Ok(contents) => contents, + Err(error) => { + tracing::warn!( + "ignoring {path}: failed to read SKILL.json: {error}", + path = interface_path.display() + ); + return None; } - } - - fn parse(self, contents: &str) -> Result { - match self { - InterfaceFormat::Json => serde_json::from_str(contents).map_err(|err| err.to_string()), - InterfaceFormat::Toml => toml::from_str(contents).map_err(|err| err.to_string()), + }; + let parsed: SkillMetadataFile = match serde_json::from_str(&contents) { + Ok(parsed) => parsed, + Err(error) => { + tracing::warn!( + "ignoring {path}: invalid JSON: {error}", + path = interface_path.display() + ); + return None; } - } + }; + let interface = parsed.interface?; + + let interface = SkillInterface { + display_name: resolve_str( + interface.display_name, + MAX_NAME_LEN, + "interface.display_name", + ), + short_description: resolve_str( + interface.short_description, + MAX_SHORT_DESCRIPTION_LEN, + "interface.short_description", + ), + icon_small: resolve_asset_path(skill_dir, "interface.icon_small", interface.icon_small), + icon_large: resolve_asset_path(skill_dir, "interface.icon_large", interface.icon_large), + brand_color: resolve_color_str(interface.brand_color, "interface.brand_color"), + default_prompt: resolve_str( + interface.default_prompt, + MAX_DEFAULT_PROMPT_LEN, + "interface.default_prompt", + ), + }; + let has_fields = interface.display_name.is_some() + || interface.short_description.is_some() + || interface.icon_small.is_some() + || interface.icon_large.is_some() + || interface.brand_color.is_some() + || interface.default_prompt.is_some(); + if has_fields { Some(interface) } else { None } } fn resolve_asset_path( @@ -789,12 +756,6 @@ mod tests { } fn write_skill_interface_at(skill_dir: &Path, contents: &str) -> PathBuf { - let path = skill_dir.join(SKILLS_TOML_FILENAME); - fs::write(&path, contents).unwrap(); - path - } - - fn write_skill_interface_json_at(skill_dir: &Path, contents: &str) -> PathBuf { let path = skill_dir.join(SKILLS_JSON_FILENAME); fs::write(&path, contents).unwrap(); path @@ -802,60 +763,12 @@ mod tests { #[tokio::test] async fn loads_skill_interface_metadata_happy_path() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from toml"); - let skill_dir = skill_path.parent().expect("skill dir"); - let normalized_skill_dir = normalized(skill_dir); - - write_skill_interface_at( - skill_dir, - r##" -[interface] -display_name = "UI Skill" -short_description = " short desc " -icon_small = "./assets/small-400px.png" -icon_large = "./assets/large-logo.svg" -brand_color = "#3B82F6" -default_prompt = " default prompt " -"##, - ); - - let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "ui-skill".to_string(), - description: "from toml".to_string(), - short_description: None, - interface: Some(SkillInterface { - display_name: Some("UI Skill".to_string()), - short_description: Some("short desc".to_string()), - icon_small: Some(normalized_skill_dir.join("assets/small-400px.png")), - icon_large: Some(normalized_skill_dir.join("assets/large-logo.svg")), - brand_color: Some("#3B82F6".to_string()), - default_prompt: Some("default prompt".to_string()), - }), - path: normalized(&skill_path), - scope: SkillScope::User, - }] - ); - } - - #[tokio::test] - async fn loads_skill_interface_metadata_from_json() { let codex_home = tempfile::tempdir().expect("tempdir"); let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); let skill_dir = skill_path.parent().expect("skill dir"); let normalized_skill_dir = normalized(skill_dir); - write_skill_interface_json_at( + write_skill_interface_at( skill_dir, r##" { @@ -902,17 +815,20 @@ default_prompt = " default prompt " #[tokio::test] async fn accepts_icon_paths_under_assets_dir() { let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from toml"); + let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); let skill_dir = skill_path.parent().expect("skill dir"); let normalized_skill_dir = normalized(skill_dir); write_skill_interface_at( skill_dir, r#" -[interface] -display_name = "UI Skill" -icon_small = "assets/icon.png" -icon_large = "./assets/logo.svg" +{ + "interface": { + "display_name": "UI Skill", + "icon_small": "assets/icon.png", + "icon_large": "./assets/logo.svg" + } +} "#, ); @@ -928,7 +844,7 @@ icon_large = "./assets/logo.svg" outcome.skills, vec![SkillMetadata { name: "ui-skill".to_string(), - description: "from toml".to_string(), + description: "from json".to_string(), short_description: None, interface: Some(SkillInterface { display_name: Some("UI Skill".to_string()), @@ -947,14 +863,17 @@ icon_large = "./assets/logo.svg" #[tokio::test] async fn ignores_invalid_brand_color() { let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from toml"); + let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); let skill_dir = skill_path.parent().expect("skill dir"); write_skill_interface_at( skill_dir, r#" -[interface] -brand_color = "blue" +{ + "interface": { + "brand_color": "blue" + } +} "#, ); @@ -970,7 +889,7 @@ brand_color = "blue" outcome.skills, vec![SkillMetadata { name: "ui-skill".to_string(), - description: "from toml".to_string(), + description: "from json".to_string(), short_description: None, interface: None, path: normalized(&skill_path), @@ -982,7 +901,7 @@ brand_color = "blue" #[tokio::test] async fn ignores_default_prompt_over_max_length() { let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from toml"); + let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); let skill_dir = skill_path.parent().expect("skill dir"); let normalized_skill_dir = normalized(skill_dir); let too_long = "x".repeat(MAX_DEFAULT_PROMPT_LEN + 1); @@ -991,10 +910,13 @@ brand_color = "blue" skill_dir, &format!( r##" -[interface] -display_name = "UI Skill" -icon_small = "./assets/small-400px.png" -default_prompt = "{too_long}" +{{ + "interface": {{ + "display_name": "UI Skill", + "icon_small": "./assets/small-400px.png", + "default_prompt": "{too_long}" + }} +}} "## ), ); @@ -1011,7 +933,7 @@ default_prompt = "{too_long}" outcome.skills, vec![SkillMetadata { name: "ui-skill".to_string(), - description: "from toml".to_string(), + description: "from json".to_string(), short_description: None, interface: Some(SkillInterface { display_name: Some("UI Skill".to_string()), @@ -1030,15 +952,18 @@ default_prompt = "{too_long}" #[tokio::test] async fn drops_interface_when_icons_are_invalid() { let codex_home = tempfile::tempdir().expect("tempdir"); - let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from toml"); + let skill_path = write_skill(&codex_home, "demo", "ui-skill", "from json"); let skill_dir = skill_path.parent().expect("skill dir"); write_skill_interface_at( skill_dir, r#" -[interface] -icon_small = "icon.png" -icon_large = "./assets/../logo.svg" +{ + "interface": { + "icon_small": "icon.png", + "icon_large": "./assets/../logo.svg" + } +} "#, ); @@ -1054,7 +979,7 @@ icon_large = "./assets/../logo.svg" outcome.skills, vec![SkillMetadata { name: "ui-skill".to_string(), - description: "from toml".to_string(), + description: "from json".to_string(), short_description: None, interface: None, path: normalized(&skill_path),