diff --git a/Cargo.lock b/Cargo.lock index 325403a..ea2d879 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4946,6 +4946,7 @@ dependencies = [ "rayon", "rustmotion", "rustmotion-studio", + "schemars", "serde", "serde_json", ] diff --git a/crates/rustmotion-cli/Cargo.toml b/crates/rustmotion-cli/Cargo.toml index 7473ff6..5375a1d 100644 --- a/crates/rustmotion-cli/Cargo.toml +++ b/crates/rustmotion-cli/Cargo.toml @@ -19,6 +19,7 @@ clap_complete = "4.6.0" ratatui = "0.29" crossterm = "0.28" notify = "7" +schemars = "0.8" serde = { version = "1", features = ["derive"] } serde_json = "1" dirs = "6.0.0" diff --git a/crates/rustmotion-cli/src/commands/mod.rs b/crates/rustmotion-cli/src/commands/mod.rs index 23083a1..3b5fc6f 100644 --- a/crates/rustmotion-cli/src/commands/mod.rs +++ b/crates/rustmotion-cli/src/commands/mod.rs @@ -4,6 +4,7 @@ mod render; mod schema; mod still; mod validate; +mod validate_attrs; mod validate_schema; pub mod validation; diff --git a/crates/rustmotion-cli/src/commands/validate.rs b/crates/rustmotion-cli/src/commands/validate.rs index ca3b9ec..ac722e5 100644 --- a/crates/rustmotion-cli/src/commands/validate.rs +++ b/crates/rustmotion-cli/src/commands/validate.rs @@ -9,6 +9,7 @@ pub fn cmd_validate( report: Option<&Path>, fix: bool, strict_anim: bool, + strict_attrs: bool, lenient: bool, ) -> Result<()> { let loaded = match validation::load(ValidationSource::File(input)) { @@ -20,6 +21,9 @@ pub fn cmd_validate( }; let mut report_out = validation::run_checks(&loaded, strict_anim); + if strict_attrs { + report_out.promote_attr_warnings(); + } if let Some(report_path) = report { write_report(report_path, &report_out)?; @@ -47,6 +51,9 @@ pub fn cmd_validate( // the on-disk state. let reloaded = validation::load(ValidationSource::File(input))?; report_out = validation::run_checks(&reloaded, strict_anim); + if strict_attrs { + report_out.promote_attr_warnings(); + } } } @@ -86,6 +93,7 @@ fn write_report(path: &Path, report: &ValidationReport) -> Result<()> { "geometry_violations": report.geom_violations, "unresolved_vars": report.unresolved_vars, "warnings": report.warnings, + "attr_warnings": report.attr_warnings, }); let pretty = serde_json::to_string_pretty(&json) .map_err(|e| RustmotionError::Generic(format!("serialize report: {}", e)))?; diff --git a/crates/rustmotion-cli/src/commands/validate_attrs.rs b/crates/rustmotion-cli/src/commands/validate_attrs.rs new file mode 100644 index 0000000..e179974 --- /dev/null +++ b/crates/rustmotion-cli/src/commands/validate_attrs.rs @@ -0,0 +1,308 @@ +//! Unknown-attribute detection for LLM-authored scenarios. +//! +//! Component structs deliberately do not use `deny_unknown_fields` (they rely +//! on `#[serde(flatten)]` for timing, which is incompatible with it), so a +//! typo'd attribute like `` silently disappears at +//! typed load. This module rebuilds the set of known top-level properties per +//! component from the schemars JSON Schema of `Component` and reports any +//! unknown key as a warning (promoted to an error by `--strict-attrs`). +//! +//! It also surfaces typed-deserialization failures (e.g. an unknown CSS +//! property, rejected by `CssStyle`'s `deny_unknown_fields`, or a missing +//! required field) as blocking schema errors — today those children are +//! silently dropped at render time. + +use std::collections::{BTreeMap, BTreeSet}; +use std::sync::OnceLock; + +use rustmotion::components::{ChildComponent, Component}; +use rustmotion::schema::ResolvedScenario; + +/// Keys accepted on any component object but absent from the per-variant +/// schema properties: +/// - `position`, `x`, `y`, `z-index`: `ChildComponent` wrapper fields +/// (flattened around the component itself). +/// - `animation`: top-level `animation` is ignored by the engine and already +/// reported by the dedicated misplaced-animation warning — flagging it here +/// too would double-report. +const WRAPPER_KEYS: &[&str] = &["position", "x", "y", "z-index", "animation"]; + +/// Serde enum aliases that schemars does not know about: alias tag → schema tag. +const TAG_ALIASES: &[(&str, &str)] = &[("container", "div"), ("progress_bar", "progress")]; + +/// Lazily-built map: component tag ("counter") → set of known top-level +/// property names, extracted from the schemars `oneOf` variants. Flattened +/// `TimingConfig` fields (`start_at`, `end_at`) are included by schemars in +/// each variant's properties, so no extra allowlist is needed for them. +fn known_props() -> &'static BTreeMap> { + static CACHE: OnceLock>> = OnceLock::new(); + CACHE.get_or_init(|| { + let schema = serde_json::to_value(schemars::schema_for!(Component)) + .expect("Component schema serializes"); + let mut map = BTreeMap::new(); + if let Some(one_of) = schema["oneOf"].as_array() { + for variant in one_of { + let Some(tag) = variant["properties"]["type"]["enum"][0].as_str() else { + continue; + }; + let props: BTreeSet = variant["properties"] + .as_object() + .map(|o| o.keys().cloned().collect()) + .unwrap_or_default(); + map.insert(tag.to_string(), props); + } + } + for (alias, target) in TAG_ALIASES { + if let Some(props) = map.get(*target).cloned() { + map.insert(alias.to_string(), props); + } + } + map + }) +} + +/// Check every component in the scenario. Returns `(errors, warnings)`: +/// - errors: children that fail typed deserialization (would be silently +/// dropped at render time) — always blocking; +/// - warnings: unknown top-level attributes (silently ignored at load) — +/// advisory unless `--strict-attrs`. +pub fn check_component_attrs(scenario: &ResolvedScenario) -> (Vec, Vec) { + let mut errors = Vec::new(); + let mut warnings = Vec::new(); + for (vi, view) in scenario.views.iter().enumerate() { + for (si, scene) in view.scenes.iter().enumerate() { + for (ci, child) in scene.children.iter().enumerate() { + let path = format!("views[{vi}].scenes[{si}].children[{ci}]"); + // Typed parse at scene level covers nested children + // transitively (containers parse their subtree). + if let Err(e) = serde_json::from_value::(child.clone()) { + let kind = child.get("type").and_then(|t| t.as_str()).unwrap_or("?"); + errors.push(format!( + "{path} (type={kind}): invalid component — would be silently dropped at render: {}", + truncate(&e.to_string(), 220) + )); + } + walk_component(child, &path, &mut warnings); + } + } + } + (errors, warnings) +} + +/// Recursively check one component object (and its `children`, when the +/// component type supports children) for unknown top-level attributes. +fn walk_component(value: &serde_json::Value, path: &str, warnings: &mut Vec) { + let Some(obj) = value.as_object() else { + return; + }; + let Some(tag) = obj.get("type").and_then(|t| t.as_str()) else { + return; // missing/invalid type — already covered by the typed-parse error + }; + let Some(known) = known_props().get(tag) else { + return; // unknown component tag — already covered by the typed-parse error + }; + + for key in obj.keys() { + if known.contains(key) || WRAPPER_KEYS.contains(&key.as_str()) { + continue; + } + warnings.push(format!( + "{path}: unknown attribute '{key}' on '{tag}' — it is silently ignored ({})", + suggest(key, known) + )); + } + + if known.contains("children") { + if let Some(children) = obj.get("children").and_then(|c| c.as_array()) { + for (i, child) in children.iter().enumerate() { + walk_component(child, &format!("{path}.children[{i}]"), warnings); + } + } + } +} + +/// Build the "(did you mean …? known: …)" suffix: known keys sorted by edit +/// distance to the unknown one, closest first, capped at 8. +fn suggest(unknown: &str, known: &BTreeSet) -> String { + let mut ranked: Vec<(usize, &str)> = known + .iter() + .map(|k| (levenshtein(unknown, k), k.as_str())) + .collect(); + ranked.sort(); + + let best = ranked.first().filter(|(d, _)| *d <= 2).map(|(_, k)| *k); + let listed: Vec<&str> = ranked.iter().take(8).map(|(_, k)| *k).collect(); + let ellipsis = if ranked.len() > 8 { ", …" } else { "" }; + match best { + Some(k) => format!( + "did you mean '{k}'? known: {}{}", + listed.join(", "), + ellipsis + ), + None => format!("known: {}{}", listed.join(", "), ellipsis), + } +} + +/// Classic dynamic-programming Levenshtein distance (small strings only). +fn levenshtein(a: &str, b: &str) -> usize { + let a: Vec = a.chars().collect(); + let b: Vec = b.chars().collect(); + let mut prev: Vec = (0..=b.len()).collect(); + for (i, ca) in a.iter().enumerate() { + let mut cur = vec![i + 1]; + for (j, cb) in b.iter().enumerate() { + let cost = usize::from(ca != cb); + cur.push((prev[j] + cost).min(prev[j + 1] + 1).min(cur[j] + 1)); + } + prev = cur; + } + prev[b.len()] +} + +/// Cap long serde error messages (CssStyle's field list is ~150 entries). +fn truncate(msg: &str, max: usize) -> String { + if msg.len() <= max { + msg.to_string() + } else { + let mut cut = max; + while !msg.is_char_boundary(cut) { + cut -= 1; + } + format!("{}…", &msg[..cut]) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::commands::validation::{load, run_checks, ValidationSource}; + + fn resolved(children: serde_json::Value) -> ResolvedScenario { + let json = serde_json::json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ "duration": 2.0, "children": children }] + }) + .to_string(); + load(ValidationSource::Inline(&json)) + .expect("scenario loads") + .scenario + } + + #[test] + fn unknown_attribute_warns_with_exact_name() { + let s = resolved(serde_json::json!([ + { "type": "counter", "from": 0, "to": 100, "typo-attr": "x" } + ])); + let (errors, warnings) = check_component_attrs(&s); + assert!(errors.is_empty(), "unexpected errors: {errors:?}"); + assert_eq!(warnings.len(), 1, "expected one warning: {warnings:?}"); + assert!(warnings[0].contains("'typo-attr'"), "got: {}", warnings[0]); + assert!(warnings[0].contains("'counter'"), "got: {}", warnings[0]); + assert!( + warnings[0].contains("views[0].scenes[0].children[0]"), + "got: {}", + warnings[0] + ); + // Lists known attributes so the author can self-correct. + assert!(warnings[0].contains("known:"), "got: {}", warnings[0]); + } + + #[test] + fn strict_attrs_promotes_warnings_to_errors() { + let json = serde_json::json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ "duration": 2.0, "children": [ + { "type": "counter", "from": 0, "to": 100, "typo-attr": "x" } + ]}] + }) + .to_string(); + let loaded = load(ValidationSource::Inline(&json)).unwrap(); + let mut report = run_checks(&loaded, false); + assert!(!report.attr_warnings.is_empty(), "expected attr warnings"); + assert!(!report.is_blocking(false), "warnings must not block"); + + report.promote_attr_warnings(); + assert!(report.attr_warnings.is_empty()); + assert!( + report.schema_errors.iter().any(|e| e.contains("typo-attr")), + "promoted error missing: {:?}", + report.schema_errors + ); + assert!(report.is_blocking(false), "strict attrs must block"); + } + + #[test] + fn flattened_and_wrapper_fields_are_not_flagged() { + // start_at/end_at come from the flattened TimingConfig; position/x/y/ + // z-index from the ChildComponent wrapper. None may warn. + let s = resolved(serde_json::json!([ + { + "type": "text", "content": "hi", + "start_at": 0.5, "end_at": 1.5, + "position": "absolute", "x": 10, "y": 20, "z-index": 2 + } + ])); + let (errors, warnings) = check_component_attrs(&s); + assert!(errors.is_empty(), "unexpected errors: {errors:?}"); + assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}"); + } + + #[test] + fn clean_scenario_has_zero_warnings() { + let s = resolved(serde_json::json!([ + { "type": "text", "content": "hi", "style": { "font-size": 42 } }, + { "type": "counter", "from": 0, "to": 100, "suffix": "%" }, + { "type": "card", "children": [ + { "type": "text", "content": "nested" } + ]} + ])); + let (errors, warnings) = check_component_attrs(&s); + assert!(errors.is_empty(), "unexpected errors: {errors:?}"); + assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}"); + } + + #[test] + fn nested_child_unknown_attribute_gets_nested_path() { + let s = resolved(serde_json::json!([ + { "type": "card", "children": [ + { "type": "text", "content": "hi", "contnet": "typo" } + ]} + ])); + let (_, warnings) = check_component_attrs(&s); + assert_eq!(warnings.len(), 1, "expected one warning: {warnings:?}"); + assert!( + warnings[0].contains("views[0].scenes[0].children[0].children[0]"), + "got: {}", + warnings[0] + ); + assert!( + warnings[0].contains("did you mean 'content'?"), + "close match should be suggested: {}", + warnings[0] + ); + } + + #[test] + fn alias_tags_use_target_schema() { + // "container" is a serde alias of "div"; "progress_bar" of "progress". + // Their unknown attributes must be checked against the target schema. + let s = resolved(serde_json::json!([ + { "type": "container", "typo": "x", "children": [] } + ])); + let (_, warnings) = check_component_attrs(&s); + assert_eq!(warnings.len(), 1, "expected one warning: {warnings:?}"); + assert!(warnings[0].contains("'typo'"), "got: {}", warnings[0]); + } + + #[test] + fn invalid_component_is_a_blocking_error() { + // Missing required field `to`: today the child is silently dropped at + // render; validation must surface it as an error. + let s = resolved(serde_json::json!([ + { "type": "counter", "from": 0 } + ])); + let (errors, _) = check_component_attrs(&s); + assert_eq!(errors.len(), 1, "expected one error: {errors:?}"); + assert!(errors[0].contains("counter"), "got: {}", errors[0]); + } +} diff --git a/crates/rustmotion-cli/src/commands/validation.rs b/crates/rustmotion-cli/src/commands/validation.rs index ffffa86..51bfc72 100644 --- a/crates/rustmotion-cli/src/commands/validation.rs +++ b/crates/rustmotion-cli/src/commands/validation.rs @@ -42,6 +42,9 @@ pub struct ValidationReport { pub unresolved_vars: Vec, /// Non-blocking advisory messages (do not prevent rendering). pub warnings: Vec, + /// Unknown component attributes (silently ignored at load). Advisory by + /// default; promoted to blocking errors by `--strict-attrs`. + pub attr_warnings: Vec, } impl ValidationReport { @@ -73,6 +76,12 @@ impl ValidationReport { unresolved_vars: self.unresolved_vars.len(), } } + + /// `--strict-attrs`: turn unknown-attribute warnings into blocking schema + /// errors. + pub fn promote_attr_warnings(&mut self) { + self.schema_errors.append(&mut self.attr_warnings); + } } /// Load + parse + apply variable defaults + resolve includes. Returns the @@ -124,13 +133,17 @@ pub fn run_checks(loaded: &LoadedScenario, strict_anim: bool) -> ValidationRepor if strict_anim { geom_violations.extend(validate_geometry_animated(&loaded.scenario)); } - let (schema_errors, mut warnings) = validate_scenario(&loaded.scenario); + let (mut schema_errors, mut warnings) = validate_scenario(&loaded.scenario); warnings.extend(warn_misplaced_animation(&loaded.raw)); + let (attr_errors, attr_warnings) = + super::validate_attrs::check_component_attrs(&loaded.scenario); + schema_errors.extend(attr_errors); ValidationReport { schema_errors, geom_violations, unresolved_vars: variables::find_unresolved(&loaded.raw), warnings, + attr_warnings, } } @@ -224,6 +237,9 @@ pub fn print_report(report: &ValidationReport, source_label: &str) { for w in &report.warnings { eprintln!("Warning: {}", w); } + for w in &report.attr_warnings { + eprintln!("Warning: {}", w); + } for name in &report.unresolved_vars { eprintln!( "Warning: unresolved variable reference '${}' in '{}'", @@ -243,6 +259,29 @@ pub fn print_report(report: &ValidationReport, source_label: &str) { } } +#[cfg(test)] +mod html_css_error_tests { + use super::*; + + #[test] + fn unknown_css_property_from_html_is_a_readable_validate_error() { + // CssStyle is deny_unknown_fields: a typo'd CSS property must surface + // as a validation error naming the property, not silently drop the + // child at render time. + let html = r##"

Hi

"##; + let value = rustmotion::loader::html_to_scenario_json(html).expect("transpiles"); + let json = serde_json::to_string(&value).unwrap(); + let loaded = load(ValidationSource::Inline(&json)).expect("loads"); + let report = run_checks(&loaded, false); + assert!( + report.schema_errors.iter().any(|e| e.contains("font-siez")), + "expected a schema error naming the unknown CSS property: {:?}", + report.schema_errors + ); + assert!(report.is_blocking(false), "must block rendering"); + } +} + #[cfg(test)] mod misplaced_animation_tests { use super::*; diff --git a/crates/rustmotion-cli/src/lib.rs b/crates/rustmotion-cli/src/lib.rs index 5cd471c..899a2b7 100644 --- a/crates/rustmotion-cli/src/lib.rs +++ b/crates/rustmotion-cli/src/lib.rs @@ -137,6 +137,10 @@ enum Commands { #[arg(long)] strict_anim: bool, + /// Treat unknown component attributes as errors instead of warnings. + #[arg(long)] + strict_attrs: bool, + /// Treat geometry violations as warnings instead of errors. #[arg(long)] lenient: bool, @@ -319,8 +323,16 @@ pub fn run() -> Result<()> { report, fix, strict_anim, + strict_attrs, + lenient, + } => commands::cmd_validate( + &file, + report.as_deref(), + fix, + strict_anim, + strict_attrs, lenient, - } => commands::cmd_validate(&file, report.as_deref(), fix, strict_anim, lenient), + ), Commands::Schema { output } => commands::cmd_schema(output.as_deref()), Commands::Info { file } => commands::cmd_info(&file), Commands::Skills { action } => match action {