From b5d6318b1fb82d135a4228f08e01f5a67f39273f Mon Sep 17 00:00:00 2001 From: Locez Date: Thu, 17 Sep 2026 00:35:19 +0800 Subject: [PATCH] fix(runtime): parse skill frontmatter as YAML Skill frontmatter was scanned line by line, so a `metadata:` map with indented keys failed with `invalid frontmatter line` and the whole skill was dropped from the catalog. Real skill files carry structured metadata, so that silently hid them from the model. - Read frontmatter with serde_norway and keep only the typed `name` and `description` fields; unknown top-level keys are ignored on purpose. - Accept plain multi-line scalars, `|`/`>` blocks, quoted values, trailing comments, CRLF line endings, and a leading byte-order mark. - Normalize name and description in `SkillMetadata::new`, so the stable prefix and the CLI completion list share one single-line invariant instead of depending on the renderer. - Type loader failures as `FrontmatterError` and `SkillLoadWarningReason` instead of `String` messages. - Move frontmatter parsing into `skill/frontmatter.rs` and share `text::collapse_whitespace` with memory canonicalization. Verified with cargo fmt --all --check, cargo clippy --all-targets --all-features -- -D warnings, and cargo test --all. --- Cargo.lock | 20 + Cargo.toml | 1 + crates/merry-cli/src/coding/tests/skills.rs | 2 +- crates/merry-runtime/Cargo.toml | 1 + .../merry-runtime/src/context/projection.rs | 7 +- crates/merry-runtime/src/lib.rs | 6 +- crates/merry-runtime/src/memory.rs | 9 +- crates/merry-runtime/src/skill.rs | 372 ++++++++++++------ crates/merry-runtime/src/skill/frontmatter.rs | 259 ++++++++++++ crates/merry-runtime/src/text.rs | 24 ++ 10 files changed, 559 insertions(+), 142 deletions(-) create mode 100644 crates/merry-runtime/src/skill/frontmatter.rs create mode 100644 crates/merry-runtime/src/text.rs diff --git a/Cargo.lock b/Cargo.lock index f609181a..4ae3c21b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1775,6 +1775,7 @@ dependencies = [ "schemars", "serde", "serde_json", + "serde_norway", "sha2", "tempfile", "thiserror 2.0.20", @@ -3056,6 +3057,19 @@ dependencies = [ "zmij", ] +[[package]] +name = "serde_norway" +version = "0.9.42" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e408f29489b5fd500fab51ff1484fc859bb655f32c671f307dcd733b72e8168c" +dependencies = [ + "indexmap", + "itoa", + "ryu", + "serde", + "unsafe-libyaml-norway", +] + [[package]] name = "serde_path_to_error" version = "0.1.20" @@ -3852,6 +3866,12 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b4ac048d71ede7ee76d585517add45da530660ef4390e49b098733c6e897f254" +[[package]] +name = "unsafe-libyaml-norway" +version = "0.2.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b39abd59bf32521c7f2301b52d05a6a2c975b6003521cbd0c6dc1582f0a22104" + [[package]] name = "untrusted" version = "0.9.0" diff --git a/Cargo.toml b/Cargo.toml index bddea23e..2e9d245b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -70,6 +70,7 @@ syn = { version = "2", features = ["full"] } schemars = { version = "1.0", features = ["derive"] } serde = { version = "1.0", features = ["derive", "rc"] } serde_json = "1.0" +serde_norway = "0.9" sha2 = "0.10" syntect = "5" thiserror = "2.0" diff --git a/crates/merry-cli/src/coding/tests/skills.rs b/crates/merry-cli/src/coding/tests/skills.rs index 7cbfc00f..4d953dfa 100644 --- a/crates/merry-cli/src/coding/tests/skills.rs +++ b/crates/merry-cli/src/coding/tests/skills.rs @@ -21,7 +21,7 @@ async fn projects_skill_metadata_without_body() { std::fs::create_dir_all(skill_root.join("demo")).expect("mkdir skill"); std::fs::write( skill_root.join("demo/SKILL.md"), - "---\nname: demo-skill\ndescription: Use for demo tasks.\n---\n# Demo\nbody sentinel\n", + "---\nname: demo-skill\ndescription: Use for demo tasks.\nmetadata:\n cli_version: \">=1.2.3\"\n requires:\n bins:\n - demo-cli\n---\n# Demo\nbody sentinel\n", ) .expect("write skill"); diff --git a/crates/merry-runtime/Cargo.toml b/crates/merry-runtime/Cargo.toml index 4c7593b0..1a344d6d 100644 --- a/crates/merry-runtime/Cargo.toml +++ b/crates/merry-runtime/Cargo.toml @@ -15,6 +15,7 @@ merry-tools-macros = { path = "../merry-tools-macros", version = "0.1.0" } schemars.workspace = true serde.workspace = true serde_json.workspace = true +serde_norway.workspace = true sha2.workspace = true thiserror.workspace = true tokio.workspace = true diff --git a/crates/merry-runtime/src/context/projection.rs b/crates/merry-runtime/src/context/projection.rs index 783829e2..64207ea9 100644 --- a/crates/merry-runtime/src/context/projection.rs +++ b/crates/merry-runtime/src/context/projection.rs @@ -8,6 +8,7 @@ use crate::{ ActivatedMemory, MemoryActivationProvenance, MemoryActivationReason, MemoryActivationScore, MemoryEvidence, MemoryId, MemoryScope, }, + text, token_estimate::estimate_text_tokens, }; use merry_core::EvidenceRef; @@ -33,11 +34,7 @@ fn format_memory_scopes(scopes: &[MemoryScope]) -> String { } fn canonicalize_memory_reason_text(value: &str) -> String { - value - .split_whitespace() - .collect::>() - .join(" ") - .to_lowercase() + text::collapse_whitespace(value).to_lowercase() } /// Compiles allowlisted structured runtime state into a deterministic context snapshot. diff --git a/crates/merry-runtime/src/lib.rs b/crates/merry-runtime/src/lib.rs index 6f2d9f73..9ca4bc55 100644 --- a/crates/merry-runtime/src/lib.rs +++ b/crates/merry-runtime/src/lib.rs @@ -59,6 +59,7 @@ mod session_store; mod skill; mod step; mod subagent; +mod text; mod token_estimate; mod tool; mod tool_admission; @@ -155,7 +156,10 @@ pub use session_projection::SessionTranscriptItem; pub use session_store::{ FileSessionStore, PlanPersistenceLocation, SessionReservation, SessionStoreError, }; -pub use skill::{SkillCatalog, SkillError, SkillLoadWarning, SkillMetadata}; +pub use skill::{ + FrontmatterError, SkillCatalog, SkillError, SkillLoadWarning, SkillLoadWarningReason, + SkillMetadata, +}; pub use step::StepContext; pub use subagent::{ CancelSubagentsInput, ChildRuntimeFactory, ChildRuntimeInput, ChildWorkspaceScope, diff --git a/crates/merry-runtime/src/memory.rs b/crates/merry-runtime/src/memory.rs index 985ea4bb..cb8b6a1e 100644 --- a/crates/merry-runtime/src/memory.rs +++ b/crates/merry-runtime/src/memory.rs @@ -7,6 +7,7 @@ // Staged internal activation types are compiled before every call path is wired. #![cfg_attr(not(test), allow(dead_code))] +use crate::text; use merry_core::EvidenceRef; use std::{cmp::Ordering, fmt}; use thiserror::Error; @@ -608,15 +609,11 @@ fn validate_non_blank(field: &'static str, value: &str) -> Result<(), MemoryErro } fn canonicalize_match_text(value: &str) -> String { - value - .split_whitespace() - .collect::>() - .join(" ") - .to_lowercase() + text::collapse_whitespace(value).to_lowercase() } fn canonicalize_label_text(value: &str) -> String { - value.split_whitespace().collect::>().join(" ") + text::collapse_whitespace(value) } fn validate_reason(reason: &MemoryActivationReason) -> Result<(), MemoryError> { diff --git a/crates/merry-runtime/src/skill.rs b/crates/merry-runtime/src/skill.rs index 5a987f19..caa771a7 100644 --- a/crates/merry-runtime/src/skill.rs +++ b/crates/merry-runtime/src/skill.rs @@ -2,16 +2,24 @@ //! //! Skills are discovered from `SKILL.md` files, but only frontmatter metadata //! enters the cacheable stable prefix. Full skill bodies remain available -//! through normal workspace file reads. +//! through normal workspace file reads. Frontmatter is parsed as YAML by the +//! `frontmatter` submodule, which reads only `name` and `description`. use std::{ collections::{BTreeMap, btree_map::Entry}, - fs, io, + fmt, fs, path::{Path, PathBuf}, }; use thiserror::Error; +use crate::text; + +#[path = "skill/frontmatter.rs"] +mod frontmatter; + +pub use frontmatter::FrontmatterError; + const SKILLS_INTRO: &str = "A skill is a set of local instructions stored in a `SKILL.md` file. The list below is metadata for discovery only; skill bodies stay on disk until needed."; const SKILLS_HOW_TO_USE: &str = r#"- If the user explicitly names a skill, including with a `$skill-name` token, use it for that turn. - If the task clearly matches a skill description, read that skill's `SKILL.md` before relying on it. @@ -29,8 +37,8 @@ pub enum SkillError { /// Field name. field: &'static str, }, - /// A required field had unsupported control characters. - #[error("{field} must not contain control characters")] + /// A required single-line field contained control characters or line breaks. + #[error("{field} must be single-line text without control characters")] ControlCharacters { /// Field name. field: &'static str, @@ -49,12 +57,6 @@ pub enum SkillError { /// Duplicate skill path. path: String, }, - /// Configured skill root does not exist. - #[error("skill root does not exist: {root}")] - RootNotFound { - /// Configured root. - root: String, - }, /// Configured skill root is not a directory. #[error("skill root is not a directory: {root}")] RootNotDirectory { @@ -82,6 +84,11 @@ pub struct SkillMetadata { impl SkillMetadata { /// Creates validated skill metadata. + /// + /// Normalization happens here so every consumer sees the same single-line + /// values: `name` is trimmed and `description` has its whitespace + /// collapsed. Fields that stay blank or contain control characters are + /// rejected. pub fn new( name: impl Into, description: impl Into, @@ -89,12 +96,13 @@ impl SkillMetadata { root: PathBuf, ) -> Result { let name = name.into(); - validate_text("skill name", &name)?; - let description = description.into(); - validate_text("skill description", &description)?; + let name = name.trim(); + validate_single_line("skill name", name)?; + let description = text::collapse_whitespace(&description.into()); + validate_single_line("skill description", &description)?; validate_skill_path(&skill_md_path)?; Ok(Self { - name, + name: name.to_owned(), description, skill_md_path, root, @@ -183,7 +191,9 @@ impl SkillCatalog { } Entry::Occupied(_) => warnings.push(SkillLoadWarning::new( skill.skill_md_path.clone(), - format!("duplicate skill name `{}` was skipped", skill.name()), + SkillLoadWarningReason::DuplicateName { + name: skill.name().to_owned(), + }, )), } } @@ -251,17 +261,14 @@ impl SkillCatalog { #[derive(Debug, Clone, PartialEq, Eq)] pub struct SkillLoadWarning { path: PathBuf, - message: String, + reason: SkillLoadWarningReason, } impl SkillLoadWarning { /// Creates a skill load warning. #[must_use] - pub fn new(path: PathBuf, message: impl Into) -> Self { - Self { - path, - message: message.into(), - } + pub fn new(path: PathBuf, reason: SkillLoadWarningReason) -> Self { + Self { path, reason } } /// Path that produced the warning. @@ -270,21 +277,47 @@ impl SkillLoadWarning { &self.path } - /// Human-readable warning detail. + /// Typed reason the skill was left out of the catalog. #[must_use] - pub fn message(&self) -> &str { - &self.message + pub fn reason(&self) -> &SkillLoadWarningReason { + &self.reason + } +} + +impl fmt::Display for SkillLoadWarning { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(formatter, "{}: {}", self.path.display(), self.reason) } } -fn validate_text(field: &'static str, value: &str) -> Result<(), SkillError> { +/// Reason a discovered `SKILL.md` file was left out of the catalog. +#[derive(Debug, Clone, PartialEq, Eq, Error)] +pub enum SkillLoadWarningReason { + /// The file could not be read from disk. + #[error("failed to read file: {message}")] + Read { + /// IO error detail. + message: String, + }, + /// The frontmatter was missing or invalid. + #[error(transparent)] + Frontmatter(#[from] FrontmatterError), + /// The frontmatter parsed, but the resulting metadata was rejected. + #[error(transparent)] + InvalidMetadata(#[from] SkillError), + /// Another skill in the same catalog already used this name. + #[error("duplicate skill name `{name}` was skipped")] + DuplicateName { + /// Duplicate skill name. + name: String, + }, +} + +fn validate_single_line(field: &'static str, value: &str) -> Result<(), SkillError> { if value.trim().is_empty() { return Err(SkillError::Blank { field }); } - if value - .chars() - .any(|character| character.is_control() && character != '\n' && character != '\t') - { + if value.chars().any(char::is_control) { return Err(SkillError::ControlCharacters { field }); } Ok(()) @@ -322,12 +355,21 @@ fn scan_root( } let mut scanned_dirs = 0usize; - scan_dir(root, root, 0, &mut scanned_dirs, skills, warnings) + scan_dir( + root, + root, + Path::new(""), + 0, + &mut scanned_dirs, + skills, + warnings, + ) } fn scan_dir( root: &Path, dir: &Path, + relative_dir: &Path, depth: usize, scanned_dirs: &mut usize, skills: &mut Vec, @@ -340,9 +382,10 @@ fn scan_dir( let skill_md = dir.join(SKILLS_FILENAME); if skill_md.is_file() { - match parse_skill_file(root, &skill_md) { + let relative_skill_md = relative_dir.join(SKILLS_FILENAME); + match parse_skill_file(&skill_md, &relative_skill_md, root) { Ok(metadata) => skills.push(metadata), - Err(message) => warnings.push(SkillLoadWarning::new(skill_md, message)), + Err(reason) => warnings.push(SkillLoadWarning::new(skill_md, reason)), } } @@ -361,14 +404,15 @@ fn scan_dir( message: source.to_string(), })?; if file_type.is_dir() { - child_dirs.push(entry.path()); + child_dirs.push((entry.path(), relative_dir.join(entry.file_name()))); } } child_dirs.sort(); - for child_dir in child_dirs { + for (child_dir, child_relative_dir) in child_dirs { scan_dir( root, &child_dir, + &child_relative_dir, depth.saturating_add(1), scanned_dirs, skills, @@ -379,77 +423,26 @@ fn scan_dir( Ok(()) } -fn parse_skill_file(root: &Path, skill_md: &Path) -> Result { - let text = fs::read_to_string(skill_md).map_err(read_error)?; - let frontmatter = frontmatter_block(&text)?; - let fields = parse_frontmatter_fields(frontmatter)?; - let name = fields - .name - .ok_or_else(|| "missing field `name`".to_owned())?; - let description = fields - .description - .ok_or_else(|| "missing field `description`".to_owned())?; - let relative_path = skill_md - .strip_prefix(root) - .map_err(|_| "skill path is outside configured root".to_owned())? - .to_path_buf(); - SkillMetadata::new(name, description, relative_path, root.to_path_buf()) - .map_err(|error| error.to_string()) -} - -fn read_error(error: io::Error) -> String { - format!("failed to read file: {error}") -} - -fn frontmatter_block(text: &str) -> Result<&str, String> { - let Some(rest) = text.strip_prefix("---\n") else { - return Err("missing frontmatter delimited by ---".to_owned()); - }; - let Some((frontmatter, _body)) = rest.split_once("\n---") else { - return Err("missing closing frontmatter delimiter".to_owned()); - }; - Ok(frontmatter) -} - -#[derive(Default)] -struct FrontmatterFields { - name: Option, - description: Option, -} - -fn parse_frontmatter_fields(frontmatter: &str) -> Result { - let mut fields = FrontmatterFields::default(); - for line in frontmatter.lines() { - let trimmed = line.trim(); - if trimmed.is_empty() { - continue; - } - if line != trimmed { - return Err(format!("invalid frontmatter line: {line}")); - } - let Some((key, value)) = trimmed.split_once(':') else { - return Err(format!("invalid frontmatter line: {trimmed}")); - }; - let value = value.trim(); - match key.trim() { - "name" => fields.name = Some(unquote_frontmatter_value(value).to_owned()), - "description" => fields.description = Some(unquote_frontmatter_value(value).to_owned()), - _ => {} - } - } - Ok(fields) -} - -fn unquote_frontmatter_value(value: &str) -> &str { - value - .strip_prefix('"') - .and_then(|rest| rest.strip_suffix('"')) - .or_else(|| { - value - .strip_prefix('\'') - .and_then(|rest| rest.strip_suffix('\'')) - }) - .unwrap_or(value) +/// Reads one skill file and turns its frontmatter into validated metadata. +/// +/// `relative_skill_md` is built by the directory walk, so metadata never has to +/// re-derive a root-relative path from the absolute scan path. +fn parse_skill_file( + skill_md: &Path, + relative_skill_md: &Path, + root: &Path, +) -> Result { + let text = fs::read_to_string(skill_md).map_err(|source| SkillLoadWarningReason::Read { + message: source.to_string(), + })?; + let fields = frontmatter::parse(&text)?; + SkillMetadata::new( + fields.name(), + fields.description(), + relative_skill_md.to_path_buf(), + root.to_path_buf(), + ) + .map_err(SkillLoadWarningReason::InvalidMetadata) } #[cfg(test)] @@ -535,6 +528,45 @@ mod tests { .expect_err("control characters should be rejected"); assert!(control.to_string().contains("skill name")); } + + #[test] + fn normalizes_description_to_one_line() { + let skill = metadata( + "sample-tool", + "Use when the description\n spans several lines.", + "skills/sample-tool/SKILL.md", + ); + assert_eq!( + skill.description(), + "Use when the description spans several lines." + ); + + let catalog = SkillCatalog::from_metadata(vec![skill]).expect("valid catalog"); + let rendered = catalog + .to_stable_prefix_message_text() + .expect("catalog should render"); + let entry = rendered + .lines() + .find(|line| line.contains("sample-tool")) + .expect("catalog should list the skill"); + assert_eq!( + entry, + "- sample-tool: Use when the description spans several lines. (file: skills/sample-tool/SKILL.md)" + ); + } + + #[test] + fn rejects_multi_line_names() { + let error = SkillMetadata::new( + "sample\ntool", + "Valid description.", + PathBuf::from("skills/sample/SKILL.md"), + PathBuf::from("/workspace"), + ) + .expect_err("line breaks in a name should be rejected"); + + assert!(error.to_string().contains("skill name"), "{error}"); + } } #[cfg(test)] @@ -547,48 +579,81 @@ mod loader_tests { } #[test] - fn loads_skill_metadata_from_roots() { + fn loads_skill_frontmatter_from_roots() { let temp = tempfile::tempdir().expect("tempdir"); let root = temp.path().join("skills"); write( &root.join("frontend/SKILL.md"), - r#"--- -name: frontend-design -description: Use when building polished frontend UI. ---- - -# Frontend Design - -full skill body sentinel -"#, + "---\nname: frontend-design\ndescription: Use when building polished frontend UI.\n---\n\n# Frontend Design\n\nfull skill body sentinel\n", ); - - let catalog = SkillCatalog::load_from_roots([root.clone()]).expect("loads catalog"); - assert_eq!(catalog.skills().len(), 1); - assert_eq!(catalog.skills()[0].name(), "frontend-design"); - assert_eq!( - catalog.skills()[0].description(), - "Use when building polished frontend UI." + write( + &root.join("sample-tool/SKILL.md"), + "---\nname: sample-tool\ndescription: Use when the task needs\n the sample tool.\nmetadata:\n cli_version: \">=1.2.3\"\n requires:\n bins:\n - sample-cli\n---\n\n# Sample Tool Skill\n", + ); + write( + &root.join("windows/SKILL.md"), + "\u{feff}---\r\nname: windows\r\ndescription: Windows-authored skill.\r\n---\r\n# Windows\r\n", ); + + let catalog = SkillCatalog::load_from_roots([root]).expect("loads catalog"); + assert!(catalog.warnings().is_empty(), "{:?}", catalog.warnings()); + let skills = catalog + .skills() + .iter() + .map(|skill| { + ( + skill.name(), + skill.description(), + skill.skill_md_path().to_path_buf(), + ) + }) + .collect::>(); assert_eq!( - catalog.skills()[0].skill_md_path(), - Path::new("frontend/SKILL.md") + skills, + vec![ + ( + "frontend-design", + "Use when building polished frontend UI.", + PathBuf::from("frontend/SKILL.md"), + ), + ( + "sample-tool", + "Use when the task needs the sample tool.", + PathBuf::from("sample-tool/SKILL.md"), + ), + ( + "windows", + "Windows-authored skill.", + PathBuf::from("windows/SKILL.md"), + ), + ] ); - assert!(catalog.warnings().is_empty()); + + let rendered = catalog + .to_stable_prefix_message_text() + .expect("catalog should render"); + assert!(rendered.contains("frontend/SKILL.md")); + assert!(!rendered.contains("# Frontend Design")); + assert!(!rendered.contains("full skill body sentinel")); } #[test] - fn skips_invalid_skill_frontmatter_with_warning() { + fn skips_invalid_skill_files_with_typed_warnings() { let temp = tempfile::tempdir().expect("tempdir"); let root = temp.path().join("skills"); write( - &root.join("valid/SKILL.md"), - "---\nname: valid\n description: bad indentation\n---\n", + &root.join("bad-name/SKILL.md"), + "---\nname: |\n bad\n name\ndescription: Use when invalid.\n---\n", ); write( &root.join("missing-description/SKILL.md"), "---\nname: missing-description\n---\n", ); + write( + &root.join("nested-description/SKILL.md"), + "---\nname: nested-description\ndescription:\n nested: value\n---\n", + ); + write(&root.join("no-frontmatter/SKILL.md"), "# Body only\n"); write( &root.join("ok/SKILL.md"), "---\nname: ok\ndescription: Valid skill.\n---\n# OK\n", @@ -597,7 +662,56 @@ full skill body sentinel let catalog = SkillCatalog::load_from_roots([root]).expect("load should not fail"); assert_eq!(catalog.skills().len(), 1); assert_eq!(catalog.skills()[0].name(), "ok"); - assert_eq!(catalog.warnings().len(), 2); + + let warnings = catalog.warnings(); + assert_eq!(warnings.len(), 4); + assert!(warnings[0].path().ends_with("bad-name/SKILL.md")); + assert!(matches!( + warnings[0].reason(), + SkillLoadWarningReason::InvalidMetadata(SkillError::ControlCharacters { + field: "skill name" + }) + )); + assert!(matches!( + warnings[1].reason(), + SkillLoadWarningReason::Frontmatter(FrontmatterError::MissingField { + field: "description" + }) + )); + assert!(matches!( + warnings[2].reason(), + SkillLoadWarningReason::Frontmatter(FrontmatterError::Invalid { .. }) + )); + assert!(matches!( + warnings[3].reason(), + SkillLoadWarningReason::Frontmatter(FrontmatterError::MissingDelimiter) + )); + } + + #[test] + fn warns_about_duplicate_skill_names() { + let temp = tempfile::tempdir().expect("tempdir"); + let first_root = temp.path().join("first"); + let second_root = temp.path().join("second"); + write( + &first_root.join("sample/SKILL.md"), + "---\nname: sample-tool\ndescription: First copy.\n---\n", + ); + write( + &second_root.join("sample/SKILL.md"), + "---\nname: Sample-Tool\ndescription: Second copy.\n---\n", + ); + + let catalog = + SkillCatalog::load_from_roots([first_root, second_root]).expect("loads catalog"); + + assert_eq!(catalog.skills().len(), 1); + assert_eq!(catalog.skills()[0].description(), "First copy."); + assert_eq!(catalog.warnings().len(), 1); + assert!(matches!( + catalog.warnings()[0].reason(), + SkillLoadWarningReason::DuplicateName { name } if name == "Sample-Tool" + )); } #[test] diff --git a/crates/merry-runtime/src/skill/frontmatter.rs b/crates/merry-runtime/src/skill/frontmatter.rs new file mode 100644 index 00000000..0de51b3a --- /dev/null +++ b/crates/merry-runtime/src/skill/frontmatter.rs @@ -0,0 +1,259 @@ +//! `SKILL.md` frontmatter extraction and YAML parsing. +//! +//! Skill frontmatter is the YAML document between the leading `---` +//! delimiters. Merry reads only `name` and `description` and intentionally +//! ignores every other top-level key, so third-party skill files that carry +//! structured `metadata`, tool policy, or license sections still load. +//! +//! Parsing the block as YAML instead of scanning it line by line is what makes +//! block scalars (`|`, `>`), quoted values, trailing comments, plain +//! multi-line scalars, CRLF line endings, and a leading byte-order mark behave +//! the way their authors expect. + +use serde::Deserialize; +use thiserror::Error; + +/// Frontmatter values Merry requires from one `SKILL.md` file. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillFrontmatter { + name: String, + description: String, +} + +impl SkillFrontmatter { + /// Skill name as written in the frontmatter. + #[must_use] + pub fn name(&self) -> &str { + &self.name + } + + /// Skill description as written in the frontmatter. + #[must_use] + pub fn description(&self) -> &str { + &self.description + } +} + +/// Errors raised while extracting and parsing `SKILL.md` frontmatter. +#[derive(Debug, Clone, PartialEq, Eq, Error)] +pub enum FrontmatterError { + /// The document does not open with a `---` line. + #[error("missing frontmatter delimited by ---")] + MissingDelimiter, + /// The opening `---` line has no closing `---` line. + #[error("missing closing frontmatter delimiter")] + MissingClosingDelimiter, + /// A required field is absent or null. + #[error("missing frontmatter field `{field}`")] + MissingField { + /// Missing field name. + field: &'static str, + }, + /// The frontmatter is not valid YAML for the fields Merry reads. + #[error("invalid frontmatter: {message}")] + Invalid { + /// Deserialization detail. + message: String, + }, +} + +/// Frontmatter as deserialized from YAML. +/// +/// Unknown keys are ignored on purpose: real skill files carry `metadata`, +/// `license`, or tool policy sections that Merry does not model, and an +/// unsupported key must not hide the whole skill from the catalog. +#[derive(Debug, Deserialize)] +struct FrontmatterDocument { + #[serde(default)] + name: Option, + #[serde(default)] + description: Option, +} + +/// Extracts `name` and `description` from one `SKILL.md` document. +/// +/// The complete file is accepted and only its frontmatter block is parsed. +/// Failures are reported as [`FrontmatterError`] so the caller can skip this +/// skill with a typed warning instead of rejecting the configured root. +pub fn parse(text: &str) -> Result { + let block = frontmatter_block(text)?; + let document = + serde_norway::from_str::>(block).map_err(|error| { + FrontmatterError::Invalid { + message: error.to_string(), + } + })?; + let Some(document) = document else { + return Err(FrontmatterError::MissingField { field: "name" }); + }; + Ok(SkillFrontmatter { + name: document + .name + .ok_or(FrontmatterError::MissingField { field: "name" })?, + description: document.description.ok_or(FrontmatterError::MissingField { + field: "description", + })?, + }) +} + +/// Returns the YAML text between the opening and closing `---` lines. +/// +/// Delimiter lines are matched textually, which stays consistent with YAML +/// document boundaries because block scalar content is always indented. +fn frontmatter_block(text: &str) -> Result<&str, FrontmatterError> { + // Editors on Windows may save a byte-order mark before the delimiter. + let text = text.strip_prefix('\u{feff}').unwrap_or(text); + let mut offset = 0usize; + let mut block_start = None; + for line in text.split_inclusive('\n') { + if line.trim_end() == "---" { + match block_start { + None => block_start = Some(offset + line.len()), + Some(start) => return Ok(&text[start..offset]), + } + } else if block_start.is_none() { + return Err(FrontmatterError::MissingDelimiter); + } + offset += line.len(); + } + match block_start { + None => Err(FrontmatterError::MissingDelimiter), + Some(_) => Err(FrontmatterError::MissingClosingDelimiter), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn parse_document(text: &str) -> SkillFrontmatter { + parse(text).expect("frontmatter should parse") + } + + #[test] + fn reads_name_and_description_and_ignores_structured_metadata() { + let skill = parse_document( + "---\nname: sample-tool\ndescription: Use when the task needs the sample tool.\nmetadata:\n cli_version: \">=1.2.3\"\n category: product\n requires:\n bins:\n - sample-cli\n---\n\n# Sample Tool Skill\n", + ); + + assert_eq!(skill.name(), "sample-tool"); + assert_eq!( + skill.description(), + "Use when the task needs the sample tool." + ); + } + + #[test] + fn folds_plain_multi_line_scalars() { + let skill = parse_document( + "---\nname: sample-tool\ndescription: Use when the task needs\n the sample tool.\n---\n# Body\n", + ); + + assert_eq!( + skill.description(), + "Use when the task needs the sample tool." + ); + } + + #[test] + fn parses_block_scalars_and_comments() { + let folded = parse_document( + "---\nname: folded\ndescription: >-\n Use when the task\n needs the sample tool.\n---\n", + ); + assert_eq!( + folded.description(), + "Use when the task needs the sample tool." + ); + + let literal = parse_document( + "---\nname: literal\ndescription: |\n First line.\n Second line.\n---\n", + ); + assert_eq!(literal.description(), "First line.\nSecond line.\n"); + + let commented = parse_document( + "---\n# generated skill\nname: commented\ndescription: Use when parsing comments. # trailing note\nlicense: MIT\n---\n", + ); + assert_eq!(commented.name(), "commented"); + assert_eq!(commented.description(), "Use when parsing comments."); + } + + #[test] + fn parses_quoted_values_and_crlf_and_byte_order_mark() { + let quoted = parse_document( + "---\nname: \"quoted\"\ndescription: 'Use when quoted: values matter.'\n---\n", + ); + assert_eq!(quoted.name(), "quoted"); + assert_eq!(quoted.description(), "Use when quoted: values matter."); + + let crlf = parse_document( + "---\r\nname: windows\r\ndescription: Windows-authored skill.\r\n---\r\n# Body\r\n", + ); + assert_eq!(crlf.description(), "Windows-authored skill."); + + let bom = parse_document( + "\u{feff}---\nname: bom\ndescription: BOM-authored skill.\n---\n# Body\n", + ); + assert_eq!(bom.name(), "bom"); + assert_eq!(bom.description(), "BOM-authored skill."); + } + + #[test] + fn reports_missing_delimiters() { + assert_eq!( + parse("# Body only\n"), + Err(FrontmatterError::MissingDelimiter) + ); + assert_eq!( + parse("---\nname: sample-tool\n"), + Err(FrontmatterError::MissingClosingDelimiter) + ); + } + + #[test] + fn reports_missing_and_null_fields() { + assert_eq!( + parse("---\nname: sample-tool\n---\n"), + Err(FrontmatterError::MissingField { + field: "description" + }) + ); + assert_eq!( + parse("---\ndescription: Use when needed.\n---\n"), + Err(FrontmatterError::MissingField { field: "name" }) + ); + assert_eq!( + parse("---\nname:\ndescription: Use when needed.\n---\n"), + Err(FrontmatterError::MissingField { field: "name" }) + ); + assert_eq!( + parse("---\n---\n"), + Err(FrontmatterError::MissingField { field: "name" }) + ); + } + + #[test] + fn reports_non_scalar_and_invalid_frontmatter() { + let non_scalar = parse("---\nname: sample-tool\ndescription:\n nested: value\n---\n") + .expect_err("nested description should be rejected"); + assert!( + matches!(non_scalar, FrontmatterError::Invalid { .. }), + "{non_scalar:?}" + ); + + let invalid = parse("---\nname: sample-tool\ndescription: [unclosed\n---\n") + .expect_err("invalid YAML should be rejected"); + assert!( + matches!(invalid, FrontmatterError::Invalid { .. }), + "{invalid:?}" + ); + } + + #[test] + fn stringifies_plain_scalar_field_values() { + // YAML resolves an unquoted number to a scalar; reading it as text keeps + // frontmatter written with `name: 123` loadable instead of rejected. + let skill = parse_document("---\nname: 123\ndescription: Use when needed.\n---\n"); + + assert_eq!(skill.name(), "123"); + } +} diff --git a/crates/merry-runtime/src/text.rs b/crates/merry-runtime/src/text.rs new file mode 100644 index 00000000..bcc1fc32 --- /dev/null +++ b/crates/merry-runtime/src/text.rs @@ -0,0 +1,24 @@ +//! Small text-normalization helpers shared by runtime records. + +/// Collapses every whitespace run into one space and trims both ends. +/// +/// Skill descriptions, memory labels, and memory reasoning text are stored and +/// compared as single-line values, so they share one normalization rule instead +/// of each module reimplementing the same `split_whitespace` conversion. +pub(crate) fn collapse_whitespace(value: &str) -> String { + value.split_whitespace().collect::>().join(" ") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn collapses_line_breaks_and_padding() { + assert_eq!( + collapse_whitespace(" Use when\n\t the task needs the tool. \n"), + "Use when the task needs the tool." + ); + assert_eq!(collapse_whitespace("\n \t\n"), ""); + } +}