diff --git a/crates/rustmotion-components/src/lib.rs b/crates/rustmotion-components/src/lib.rs index b053588..20e8092 100644 --- a/crates/rustmotion-components/src/lib.rs +++ b/crates/rustmotion-components/src/lib.rs @@ -126,19 +126,174 @@ pub use waveform::Waveform; // --- Position mode --- -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +/// Constat #8: `PositionMode::Named(String)` accepts any string, but +/// [`ChildComponent::absolute_position`] only ever treats the literal +/// `"absolute"` specially — every other value (including the CSS-legitimate +/// `"relative"`/`"static"`, which an LLM reasoning in CSS terms naturally +/// reaches for) silently drops `x`/`y`: the component is taken out of flow +/// (`is_flow()` is false for any `Some(position)`) but never receives an +/// absolute offset either, since only `"absolute"` is matched. `x`/`y` are +/// top-level sibling fields on `ChildComponent`, not on `PositionMode` +/// itself, so this can't detect *whether* they were actually set — only +/// that, if they were, they are about to be silently ignored. `"absolute"` +/// stays completely silent (the common, correct case); anything else warns. +pub fn is_recognized_position_name(s: &str) -> bool { + s == "absolute" +} + +#[derive(Debug, Clone, Serialize, JsonSchema)] #[serde(untagged)] pub enum PositionMode { Absolute { x: f32, y: f32 }, Named(String), } +impl<'de> Deserialize<'de> for PositionMode { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + #[derive(Deserialize)] + #[serde(untagged)] + enum Raw { + Absolute { x: f32, y: f32 }, + Named(String), + } + Ok(match Raw::deserialize(deserializer)? { + Raw::Absolute { x, y } => PositionMode::Absolute { x, y }, + Raw::Named(s) => { + if !is_recognized_position_name(&s) && warn_once_for(&s) { + eprintln!( + "Warning: position: \"{s}\" is not \"absolute\" — this component-level \ + `position` shorthand only honours the literal \"absolute\" (paired with \ + `x`/`y`); any other value, including CSS-legitimate ones like \ + \"relative\"/\"static\", is accepted but silently drops `x`/`y` instead \ + of positioning the element (it still removes the component from flex \ + flow). Use `style.position` for real CSS relative/static semantics." + ); + } + PositionMode::Named(s) + } + }) + } +} + +/// True the first time this exact `position` value is seen, false afterwards. +/// +/// `render_scene_frame` calls `prepare_scene` — and therefore re-runs this +/// `Deserialize` over the whole scene tree — once **per frame**. An unguarded +/// warning here would print the same line once per offending component per +/// frame: over a thousand times on a 1200-frame render, drowning out anything +/// else on stderr. Keyed by the value rather than a plain `Once` so a scenario +/// with several distinct bad values still hears about each of them. +fn warn_once_for(value: &str) -> bool { + use std::collections::HashSet; + use std::sync::{Mutex, OnceLock}; + static SEEN: OnceLock>> = OnceLock::new(); + SEEN.get_or_init(Default::default) + .lock() + .map(|mut seen| seen.insert(value.to_owned())) + .unwrap_or(false) +} + impl Default for PositionMode { fn default() -> Self { Self::Absolute { x: 0.0, y: 0.0 } } } +#[cfg(test)] +mod position_mode_tests { + use super::*; + + // ---- constat #8 (RED first) ---- + + #[test] + fn absolute_is_recognized() { + assert!(is_recognized_position_name("absolute")); + } + + #[test] + fn relative_and_static_and_typos_are_not_recognized() { + for s in ["relative", "static", "fixed", "sticky", "Absolute", "abs"] { + assert!( + !is_recognized_position_name(s), + "'{s}' must not be treated as the recognised \"absolute\" value" + ); + } + } + + #[test] + fn absolute_object_form_still_carries_x_y() { + let json = + r#"{ "position": { "x": 10.0, "y": 20.0 }, "type": "shape", "shape": "circle" }"#; + let child: ChildComponent = serde_json::from_str(json).unwrap(); + assert_eq!(child.absolute_position(), Some((10.0, 20.0))); + } + + #[test] + fn absolute_string_form_with_sibling_x_y_still_carries_them() { + let json = + r#"{ "position": "absolute", "x": 5.0, "y": 7.0, "type": "shape", "shape": "circle" }"#; + let child: ChildComponent = serde_json::from_str(json).unwrap(); + assert_eq!(child.absolute_position(), Some((5.0, 7.0))); + } + + #[test] + fn relative_still_parses_but_drops_x_y_and_the_helper_flags_it() { + // The legitimate-CSS trap named in constat #8: an LLM writes + // `"position": "relative"` (valid CSS) with `x`/`y` alongside it, + // expecting a positioned element. The parse must not fail — this is + // legitimate JSON per the schema's own untagged catch-all — but the + // coordinates are provably dropped (`absolute_position()` is + // `None`), and `is_recognized_position_name` is the named, + // independently testable signal the warning path uses to detect + // this instead of staying silent. + let json = + r#"{ "position": "relative", "x": 5.0, "y": 7.0, "type": "shape", "shape": "circle" }"#; + let child: ChildComponent = serde_json::from_str(json).unwrap(); + assert!( + !is_recognized_position_name("relative"), + "this is exactly the case the warning fires for" + ); + assert_eq!( + child.absolute_position(), + None, + "x/y are indeed dropped for a non-\"absolute\" position — this is the silent \ + behaviour being made loud, not a new regression" + ); + // The component is still taken out of flow, same as before. + assert!(!child.is_flow()); + } + + /// `prepare_scene` re-runs this `Deserialize` over the whole scene tree + /// once per frame, so the warning must be deduplicated or a 1200-frame + /// render prints it 1200 times. Distinct values still each get a line. + #[test] + fn the_warning_fires_once_per_distinct_value_not_once_per_frame() { + let value = "position-value-used-only-by-this-test"; + assert!(warn_once_for(value), "first sighting must warn"); + for _ in 0..1000 { + assert!( + !warn_once_for(value), + "re-parsing the same value must stay silent" + ); + } + assert!( + warn_once_for("a-different-position-value-for-this-test"), + "a different bad value must still get its own warning" + ); + } + + #[test] + fn no_position_set_is_a_normal_flow_child() { + let json = r#"{ "type": "shape", "shape": "circle" }"#; + let child: ChildComponent = serde_json::from_str(json).unwrap(); + assert!(child.is_flow()); + assert_eq!(child.absolute_position(), None); + } +} + // --- Child wrapper --- #[derive(Debug, Serialize, Deserialize, JsonSchema)] diff --git a/crates/rustmotion-core/src/css/style.rs b/crates/rustmotion-core/src/css/style.rs index ad44f35..a37ef79 100644 --- a/crates/rustmotion-core/src/css/style.rs +++ b/crates/rustmotion-core/src/css/style.rs @@ -486,12 +486,21 @@ pub enum Visibility { } /// `width: ` / `width: auto` / `width: 50%` / `width: max-content` / etc. +/// +/// Constat #6: `#[serde(untagged)]` tries variants in declaration order and +/// keeps the first that succeeds. `Length(LengthPercentage)` has its own +/// `String` catch-all variant that accepts *any* string — so with `Keyword` +/// declared after `Length` (as this used to be), `"max-content"` matched +/// `Length(String("max-content"))` before `Keyword` was ever tried: +/// `max-content`/`min-content`/`fit-content` were unreachable, dead schema. +/// `Keyword` must come before the `Length` catch-all; `Auto` before either +/// is fine since it needs an exact `"auto"` match nothing else claims first. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] #[serde(untagged)] pub enum Size { Auto(AutoKw), - Length(LengthPercentage), Keyword(SizeKeyword), + Length(LengthPercentage), } #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] @@ -509,8 +518,19 @@ pub enum SizeKeyword { } /// Edge values for `margin` / `padding`. Either uniform or per-side. +/// +/// Constat #2: `CssStyle` itself has `deny_unknown_fields`, which gives the +/// impression that any bad key under `style` is rejected — but one level +/// down, `Sides`'s four fields are all `#[serde(default)]` with no +/// `deny_unknown_fields` of its own. Since this is an untagged enum, a +/// well-meaning but unsupported shape like `{"horizontal": 20}` (the exact +/// form the LAYOUT `margin-left` rule teaches LLMs to reach for) fails to +/// match `Uniform` (not a scalar) and then matches `Sides` anyway — every +/// side defaults to 0, no error. `deny_unknown_fields` here closes that: an +/// object that isn't a recognised `{top,right,bottom,left}` shape now fails +/// to match either variant, and the untagged enum reports it. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] -#[serde(untagged)] +#[serde(untagged, deny_unknown_fields)] pub enum Edges { Uniform(LengthPercentage), Sides { @@ -579,18 +599,35 @@ pub enum BorderStyle { } /// Border-radius: uniform or per-corner. +/// +/// Constat #1: every other composite in this file is kebab-case on the wire +/// (`box-shadow` -> `offset-x`/`offset-y`, `transform-origin` -> `x`/`y`, +/// etc. — see `rules/component-field-placement.md`). `Corners` used to be +/// the sole snake_case outlier (`top_left`/...), with no `deny_unknown_fields` +/// and every field defaulted — so the kebab form a CSS-literate author (or +/// LLM) naturally writes matched *zero* declared fields, and being an +/// untagged enum, serde didn't complain: it just produced `Corners` with +/// every corner at 0px, silently. `rename_all = "kebab-case"` makes kebab +/// the canonical wire form (matching every neighbour); `alias` keeps the +/// original snake_case working for any scenario already written that way; +/// `deny_unknown_fields` turns any other spelling (a genuine typo) into a +/// named parse error instead of a third silent zero. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] -#[serde(untagged)] +#[serde(untagged, deny_unknown_fields)] pub enum BorderRadius { Uniform(LengthPercentage), Corners { - #[serde(default)] + #[serde(default, alias = "top_left")] + #[serde(rename = "top-left")] top_left: LengthPercentage, - #[serde(default)] + #[serde(default, alias = "top_right")] + #[serde(rename = "top-right")] top_right: LengthPercentage, - #[serde(default)] + #[serde(default, alias = "bottom_right")] + #[serde(rename = "bottom-right")] bottom_right: LengthPercentage, - #[serde(default)] + #[serde(default, alias = "bottom_left")] + #[serde(rename = "bottom-left")] bottom_left: LengthPercentage, }, } @@ -786,12 +823,20 @@ pub enum FontStyle { } /// `line-height: 1.5` (number) or `line-height: 24px` (length). +/// +/// Same class of bug as constat #6 on [`Size`], found while auditing this +/// file for other untagged enums with a catch-all before a specific variant: +/// `Length(LengthPercentage)`'s `String` fallback accepts any string, so +/// with `Keyword` declared after it, `"normal"` matched +/// `Length(String("normal"))` — which then resolves through +/// `Length::px()`/`.parse()` as an unparseable length, falling back to 0 — +/// instead of `Keyword(LineHeightKw::Normal)`. `Keyword` now comes first. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] #[serde(untagged)] pub enum LineHeight { Number(f32), - Length(LengthPercentage), Keyword(LineHeightKw), + Length(LengthPercentage), } #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] @@ -1497,4 +1542,177 @@ mod tests { ); assert_eq!(line_height, 300.0 * 0.85); } + + // ---- constat #1: border-radius per-corner kebab-case (RED first) ---- + + #[test] + fn border_radius_corners_accepts_kebab_case() { + // This is the shape every sibling composite in this file uses + // (box-shadow -> offset-x/offset-y, transform-origin -> x/y, etc.) + // and the shape `rules/component-field-placement.md` teaches. Before + // the fix, `BorderRadius::Corners`'s fields are literally + // `top_left`/`top_right`/... with no kebab alias, so this kebab + // object fails to match `Corners` (unknown fields) and, being all + // `#[serde(default)]`, matches it anyway with every corner at 0 — + // the untagged enum never reports an error, it just silently + // produces radius 0. + let json = r#"{ "border-radius": { "top-left": "12px", "top-right": "12px", "bottom-right": "4px", "bottom-left": "4px" } }"#; + let s: CssStyle = serde_json::from_str(json).unwrap(); + match s.border_radius { + Some(BorderRadius::Corners { + top_left, + top_right, + bottom_right, + bottom_left, + }) => { + assert_eq!(top_left.px(), 12.0, "top-left must be honoured, not 0"); + assert_eq!(top_right.px(), 12.0); + assert_eq!(bottom_right.px(), 4.0); + assert_eq!(bottom_left.px(), 4.0); + } + other => panic!("expected Corners, got {other:?}"), + } + } + + #[test] + fn border_radius_corners_still_accepts_legacy_snake_case() { + // Back-compat: any scenario already written with the old + // snake_case field names must keep working identically. + let json = r#"{ "border-radius": { "top_left": "8px", "top_right": "8px", "bottom_right": "8px", "bottom_left": "8px" } }"#; + let s: CssStyle = serde_json::from_str(json).unwrap(); + assert_eq!(s.border_radius_px(), Some(8.0)); + } + + #[test] + fn border_radius_corners_typo_is_a_named_error_not_a_silent_zero() { + // A misspelled key must not silently resolve to Corners{0,0,0,0} — + // it must be reported. + let json = r#"{ "border-radius": { "topleft": "12px" } }"#; + let err = serde_json::from_str::(json).expect_err("typo must be rejected"); + let msg = err.to_string(); + assert!( + msg.contains("topleft") + || msg.contains("border-radius") + || msg.contains("BorderRadius"), + "error must name the offending input, got: {msg}" + ); + } + + // ---- constat #2: `Edges` (padding/margin) rejects unknown shapes (RED first) ---- + + #[test] + fn edges_rejects_unknown_object_shape_instead_of_defaulting_to_zero() { + // `rules/margin-left-hack.md`-adjacent trap: an LLM reasoning in CSS + // terms writes `{"horizontal": 20}` instead of the supported + // `{"top":.., "right":.., "bottom":.., "left":..}` shape. Before the + // fix, `Edges::Sides`'s four fields are all `#[serde(default)]` with + // no `deny_unknown_fields`, so this object matches `Sides` anyway + // with every side at 0 — silent, wrong padding instead of an error. + let json = r#"{ "padding": { "horizontal": 20 } }"#; + let err = serde_json::from_str::(json) + .expect_err("an unrecognised padding shape must be rejected, not silently zeroed"); + let msg = err.to_string(); + assert!( + msg.contains("horizontal") || msg.contains("padding") || msg.contains("Edges"), + "error must name the offending input, got: {msg}" + ); + } + + #[test] + fn edges_still_accepts_valid_per_side_object() { + let json = r#"{ "padding": { "top": "10px", "right": "20px", "bottom": "10px", "left": "20px" } }"#; + let s: CssStyle = serde_json::from_str(json).unwrap(); + assert_eq!(s.padding_px(), (10.0, 20.0, 10.0, 20.0)); + } + + #[test] + fn edges_still_accepts_uniform_scalar() { + let json = r#"{ "padding": "24px" }"#; + let s: CssStyle = serde_json::from_str(json).unwrap(); + assert_eq!(s.padding_px(), (24.0, 24.0, 24.0, 24.0)); + } + + // ---- constat #6: `Size` untagged variant order (RED first) ---- + + #[test] + fn size_keyword_max_content_is_reachable() { + // `Size` is `#[serde(untagged)]`: Auto, Length, Keyword in that + // declared order (before the fix). `Length(LengthPercentage)`'s + // `String` fallback variant accepts *any* string, so it is tried + // (and succeeds) before `Keyword` is ever reached — `max-content` / + // `min-content` / `fit-content` are dead schema. After the fix, + // `Keyword` must be tried before the `Length` catch-all. + for (kw, expected) in [ + ("max-content", SizeKeyword::MaxContent), + ("min-content", SizeKeyword::MinContent), + ("fit-content", SizeKeyword::FitContent), + ] { + let json = format!(r#"{{ "width": "{kw}" }}"#); + let s: CssStyle = serde_json::from_str(&json).unwrap(); + assert_eq!( + s.width, + Some(Size::Keyword(expected)), + "width: \"{kw}\" must resolve to Size::Keyword, not Size::Length(String(..))" + ); + } + } + + #[test] + fn size_length_and_auto_are_unaffected_by_the_reorder() { + let s: CssStyle = serde_json::from_str(r#"{ "width": "200px" }"#).unwrap(); + assert!(matches!(s.width, Some(Size::Length(_)))); + let s: CssStyle = serde_json::from_str(r#"{ "width": "50%" }"#).unwrap(); + assert!(matches!(s.width, Some(Size::Length(_)))); + let s: CssStyle = serde_json::from_str(r#"{ "width": "auto" }"#).unwrap(); + assert!(matches!(s.width, Some(Size::Auto(_)))); + let s: CssStyle = serde_json::from_str(r#"{ "width": 200 }"#).unwrap(); + assert!(matches!(s.width, Some(Size::Length(_)))); + } + + // ---- extra: `LineHeight` has the same catch-all-before-specific shape + // as constat #6's `Size`, found while auditing this file for the same + // bug class. Fixed alongside it (see the doc comment on `LineHeight`). + + #[test] + fn line_height_keyword_normal_is_reachable() { + let s: CssStyle = serde_json::from_str(r#"{ "line-height": "normal" }"#).unwrap(); + assert_eq!( + s.line_height, + Some(LineHeight::Keyword(LineHeightKw::Normal)), + "line-height: \"normal\" must resolve to Keyword, not Length(String(\"normal\"))" + ); + } + + #[test] + fn line_height_number_and_length_are_unaffected_by_the_reorder() { + let s: CssStyle = serde_json::from_str(r#"{ "line-height": 1.5 }"#).unwrap(); + assert!(matches!(s.line_height, Some(LineHeight::Number(_)))); + let s: CssStyle = serde_json::from_str(r#"{ "line-height": "24px" }"#).unwrap(); + assert!(matches!(s.line_height, Some(LineHeight::Length(_)))); + } + + // ---- border-radius: kebab-case is the canonical wire form on output ---- + + #[test] + fn border_radius_corners_serializes_as_kebab_case() { + let s = CssStyle { + border_radius: Some(BorderRadius::Corners { + top_left: LengthPercentage::Px(1.0), + top_right: LengthPercentage::Px(2.0), + bottom_right: LengthPercentage::Px(3.0), + bottom_left: LengthPercentage::Px(4.0), + }), + ..Default::default() + }; + let json = serde_json::to_value(&s).unwrap(); + let br = &json["border-radius"]; + assert_eq!(br["top-left"], serde_json::json!(1.0)); + assert_eq!(br["top-right"], serde_json::json!(2.0)); + assert_eq!(br["bottom-right"], serde_json::json!(3.0)); + assert_eq!(br["bottom-left"], serde_json::json!(4.0)); + assert!( + br.get("top_left").is_none(), + "must not emit the legacy snake_case key any more" + ); + } } diff --git a/crates/rustmotion-core/src/schema/animation.rs b/crates/rustmotion-core/src/schema/animation.rs index 85260ab..ebd90d9 100644 --- a/crates/rustmotion-core/src/schema/animation.rs +++ b/crates/rustmotion-core/src/schema/animation.rs @@ -1,7 +1,14 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +// `deny_unknown_fields` (reliquat of wave-A's constat, PR #158): closes the +// last gap in `style.animation[*]` typo detection. Wave A covered the nine +// effect-config structs in `schema/video.rs`; the types *inside* a +// `keyframes[*]` entry (this struct and `Keyframe` below) were left +// uncovered — a typo'd key here (e.g. `duratoin`) used to be silently +// dropped instead of reported, same as every other struct this wave closed. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct Animation { pub property: String, pub keyframes: Vec, @@ -12,6 +19,7 @@ pub struct Animation { } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct Keyframe { pub time: f64, pub value: KeyframeValue, @@ -194,3 +202,63 @@ impl Default for PresetConfig { fn default_preset_duration() -> f64 { 0.8 } + +#[cfg(test)] +mod deny_unknown_fields_tests { + use super::*; + use serde_json::json; + + // ---- reliquat of the wave-A fix (PR #158): `deny_unknown_fields` was + // added to the nine effect-config structs in `schema/video.rs`, but the + // types *inside* a `keyframes[*]` entry — `Animation` and `Keyframe`, + // both in this file — were left uncovered. A typo'd key inside one of + // these (e.g. `duratoin` on an `Animation`, or a per-keyframe field + // typo) used to be silently ignored instead of reported. This is the + // one change this workstream is authorized to make in this file. ---- + + #[test] + fn animation_rejects_unknown_fields() { + let json = json!({ + "property": "opacity", + "keyframes": [{ "time": 0.0, "value": 1.0 }], + "easing": "ease_out", + "duratoin": 5.0 + }); + let err = serde_json::from_value::(json) + .expect_err("a typo'd field on Animation must be rejected, not silently ignored"); + assert!(err.to_string().contains("duratoin"), "got: {err}"); + } + + #[test] + fn keyframe_rejects_unknown_fields() { + let json = json!({ "time": 0.0, "value": 1.0, "eaisng": "linear" }); + let err = serde_json::from_value::(json) + .expect_err("a typo'd field on Keyframe must be rejected, not silently ignored"); + assert!(err.to_string().contains("eaisng"), "got: {err}"); + } + + #[test] + fn animation_still_accepts_every_known_field() { + let json = json!({ + "property": "opacity", + "keyframes": [ + { "time": 0.0, "value": 0.0, "easing": "linear" }, + { "time": 1.0, "value": 1.0 } + ], + "easing": "ease_out", + "spring": { "damping": 10.0, "stiffness": 100.0, "mass": 1.0 } + }); + let a: Animation = serde_json::from_value(json).unwrap(); + assert_eq!(a.property, "opacity"); + assert_eq!(a.keyframes.len(), 2); + assert!(a.spring.is_some()); + } + + #[test] + fn keyframe_still_accepts_every_known_field() { + let json = json!({ "time": 0.5, "value": 10.0, "easing": "ease_in" }); + let k: Keyframe = serde_json::from_value(json).unwrap(); + assert_eq!(k.time, 0.5); + assert!(k.easing.is_some()); + } +} diff --git a/crates/rustmotion-core/src/schema/background.rs b/crates/rustmotion-core/src/schema/background.rs index 464d7ea..bfba1cb 100644 --- a/crates/rustmotion-core/src/schema/background.rs +++ b/crates/rustmotion-core/src/schema/background.rs @@ -159,6 +159,18 @@ impl Serialize for AnimatedBackground { } } +/// Every preset name the engine actually recognises. A `preset` value +/// outside this list — including the empty string produced when the key is +/// missing entirely — is rejected below instead of silently becoming +/// `gradient_shift` (constat #3, sink 1). +const KNOWN_BACKGROUND_PRESETS: &[&str] = &[ + "gradient_shift", + "grid_dots", + "concentric_circles", + "halo", + "heropattern", +]; + impl<'de> Deserialize<'de> for AnimatedBackground { fn deserialize>(deserializer: D) -> Result { let map: serde_json::Map = @@ -167,48 +179,32 @@ impl<'de> Deserialize<'de> for AnimatedBackground { // Common fields let x = map.get("x").and_then(|v| v.as_f64()).unwrap_or(0.0) as f32; let y = map.get("y").and_then(|v| v.as_f64()).unwrap_or(0.0) as f32; - let direction: Option = map - .get("direction") - .and_then(|v| serde_json::from_value(v.clone()).ok()); + // Constat #3 (related sink, fixed alongside): a mistyped `direction` + // used to be swallowed by `.ok()` into a silent `None` — same class + // as the preset/zones/colors sinks below, just on a smaller field. + let direction: Option = match map.get("direction") { + Some(v) => Some(serde_json::from_value(v.clone()).map_err(|e| { + serde::de::Error::custom(format!("animated-background.direction: {e}")) + })?), + None => None, + }; let preset_str = map.get("preset").and_then(|v| v.as_str()).unwrap_or(""); + if !KNOWN_BACKGROUND_PRESETS.contains(&preset_str) { + return Err(serde::de::Error::custom(format!( + "unknown animated-background preset '{preset_str}': expected one of {}", + KNOWN_BACKGROUND_PRESETS.join(", ") + ))); + } // Detect new vs legacy format: new format has a sub-object keyed by preset name - let is_new_format = - !preset_str.is_empty() && map.get(preset_str).is_some_and(|v| v.is_object()); + let is_new_format = map.get(preset_str).is_some_and(|v| v.is_object()); let (preset, speed) = if is_new_format { // New format: config in sub-object let sub = map.get(preset_str).unwrap().clone(); let speed = map.get("speed").and_then(|v| v.as_f64()).unwrap_or(0.0) as f32; - let preset = match preset_str { - "grid_dots" => { - let cfg: GridDotsConfig = - serde_json::from_value(sub).map_err(serde::de::Error::custom)?; - BackgroundPreset::GridDots(cfg) - } - "concentric_circles" => { - let cfg: ConcentricCirclesConfig = - serde_json::from_value(sub).map_err(serde::de::Error::custom)?; - BackgroundPreset::ConcentricCircles(cfg) - } - "halo" => { - let cfg: HaloConfig = - serde_json::from_value(sub).map_err(serde::de::Error::custom)?; - BackgroundPreset::Halo(cfg) - } - "heropattern" => { - let cfg: HeropatternConfig = - serde_json::from_value(sub).map_err(serde::de::Error::custom)?; - BackgroundPreset::Heropattern(cfg) - } - _ => { - // gradient_shift or unknown → gradient_shift - let cfg: GradientShiftConfig = - serde_json::from_value(sub).map_err(serde::de::Error::custom)?; - BackgroundPreset::GradientShift(cfg) - } - }; + let preset = deserialize_preset_config::(preset_str, sub)?; (preset, speed) } else { // Legacy flat format @@ -257,26 +253,74 @@ impl<'de> Deserialize<'de> for AnimatedBackground { }) } "halo" => { - let zones: Vec = map - .get("zones") - .and_then(|v| serde_json::from_value(v.clone()).ok()) - .unwrap_or_default(); - BackgroundPreset::Halo(HaloConfig { zones }) + // Constat #3, sink 2: was `.ok().unwrap_or_default()` — + // a malformed (or entirely missing) `zones` silently + // became an empty halo instead of erroring. Route + // through the same validated-struct path as the + // new-format branch: `HaloConfig::zones` is required + // (no `#[serde(default)]`), so a missing/malformed value + // now produces a real "missing/invalid field zones" + // error instead. + let mut obj = serde_json::Map::new(); + if let Some(z) = map.get("zones") { + obj.insert("zones".to_string(), z.clone()); + } + let cfg: HaloConfig = serde_json::from_value(serde_json::Value::Object(obj)) + .map_err(|e| { + serde::de::Error::custom(format!("animated-background.zones: {e}")) + })?; + BackgroundPreset::Halo(cfg) } - _ => { - // Default: gradient_shift - let colors: Vec = map - .get("colors") - .and_then(|v| serde_json::from_value(v.clone()).ok()) - .unwrap_or_default(); - let gradient_type: GradientType = map - .get("gradient_type") - .and_then(|v| serde_json::from_value(v.clone()).ok()) - .unwrap_or_else(default_bg_type); - BackgroundPreset::GradientShift(GradientShiftConfig { - colors, - gradient_type, - }) + "heropattern" => { + // Constat #3 (related sink, fixed alongside): the legacy + // branch never had an arm for `heropattern` at all, so a + // *correctly spelled* `"preset": "heropattern"` written + // in the legacy flat form (no `heropattern: {...}` + // sub-object) fell through the old `_ =>` wildcard and + // silently became `gradient_shift` with `colors: []`. + let mut obj = serde_json::Map::new(); + for key in ["pattern", "color", "opacity", "scale"] { + if let Some(v) = map.get(key) { + obj.insert(key.to_string(), v.clone()); + } + } + let cfg: HeropatternConfig = + serde_json::from_value(serde_json::Value::Object(obj)).map_err(|e| { + serde::de::Error::custom(format!( + "animated-background.heropattern: {e}" + )) + })?; + BackgroundPreset::Heropattern(cfg) + } + "gradient_shift" => { + // Constat #3, sink 3: `colors`/`gradient_type` were each + // parsed with `.ok().unwrap_or_default()` / + // `.ok().unwrap_or_else(default_bg_type)` — so even with + // `preset` spelled *correctly*, a missing or malformed + // `colors` silently produced `colors: []`, i.e. a fully + // empty gradient that paints black with no diagnostic at + // all — the exact worst-case symptom the audit names. + let mut obj = serde_json::Map::new(); + if let Some(c) = map.get("colors") { + obj.insert("colors".to_string(), c.clone()); + } + if let Some(g) = map.get("gradient_type") { + obj.insert("gradient_type".to_string(), g.clone()); + } + let cfg: GradientShiftConfig = + serde_json::from_value(serde_json::Value::Object(obj)).map_err(|e| { + serde::de::Error::custom(format!( + "animated-background.colors/gradient_type: {e}" + )) + })?; + BackgroundPreset::GradientShift(cfg) + } + // Unreachable: `preset_str` was already checked against + // `KNOWN_BACKGROUND_PRESETS` above. + other => { + return Err(serde::de::Error::custom(format!( + "internal error: unhandled animated-background preset '{other}'" + ))) } }; (preset, legacy_speed) @@ -305,6 +349,36 @@ impl<'de> Deserialize<'de> for AnimatedBackground { } } +/// Deserialize the preset-specific config object for the "new" nested +/// format (`{"preset": "halo", "halo": {...}}`) — shared by +/// `AnimatedBackground::deserialize` and available for reuse. `preset_str` +/// must already be one of [`KNOWN_BACKGROUND_PRESETS`]. +fn deserialize_preset_config( + preset_str: &str, + sub: serde_json::Value, +) -> Result { + match preset_str { + "grid_dots" => Ok(BackgroundPreset::GridDots( + serde_json::from_value(sub).map_err(E::custom)?, + )), + "concentric_circles" => Ok(BackgroundPreset::ConcentricCircles( + serde_json::from_value(sub).map_err(E::custom)?, + )), + "halo" => Ok(BackgroundPreset::Halo( + serde_json::from_value(sub).map_err(E::custom)?, + )), + "heropattern" => Ok(BackgroundPreset::Heropattern( + serde_json::from_value(sub).map_err(E::custom)?, + )), + "gradient_shift" => Ok(BackgroundPreset::GradientShift( + serde_json::from_value(sub).map_err(E::custom)?, + )), + other => Err(E::custom(format!( + "internal error: unhandled animated-background preset '{other}'" + ))), + } +} + impl JsonSchema for AnimatedBackground { fn schema_name() -> String { "AnimatedBackground".to_string() @@ -401,6 +475,49 @@ pub struct BackgroundEntry { pub overrides: serde_json::Map, } +/// Constat #5: no derived `JsonSchema` here (the `#[serde(flatten)]` map +/// makes a fully-accurate derive impossible anyway — the point of `flatten` +/// is "any other keys"), which is exactly why `Scene`/`View` reached for +/// `#[schemars(skip)]` on `background` in the first place: skip was the +/// only option with no `JsonSchema` impl to call. But `Scene`/`View` are +/// also `deny_unknown_fields` (schemars emits `additionalProperties: false` +/// for that), so skipping `background` didn't just leave it undocumented — +/// it made the *exported schema* declare invalid any scenario that actually +/// sets `scene.background` / `view.background`, which is most of them. This +/// manual impl describes the real accepted shape (`$ref` + `transition` + +/// "anything else", matching the `flatten`) so `background` can be a real +/// declared property instead. +impl JsonSchema for BackgroundEntry { + fn schema_name() -> String { + "BackgroundEntry".to_string() + } + + fn json_schema(gen: &mut schemars::gen::SchemaGenerator) -> schemars::schema::Schema { + use schemars::schema::*; + + let mut props = schemars::Map::new(); + props.insert("$ref".to_string(), gen.subschema_for::>()); + props.insert( + "transition".to_string(), + gen.subschema_for::>(), + ); + + SchemaObject { + instance_type: Some(InstanceType::Object.into()), + object: Some(Box::new(ObjectValidation { + properties: props, + // Mirrors `#[serde(flatten)] overrides: serde_json::Map<..>`: + // any other key (the preset config, `x`/`y`/`speed`/...) is + // genuinely accepted, not a schema gap to close. + additional_properties: Some(Box::new(Schema::Bool(true))), + ..Default::default() + })), + ..Default::default() + } + .into() + } +} + /// The unified background field: color string, single entry, or multiple entries. #[derive(Debug, Clone)] pub enum BackgroundValue { @@ -422,6 +539,40 @@ impl Serialize for BackgroundValue { } } +/// See [`BackgroundEntry`]'s `JsonSchema` impl doc comment — same reason: +/// `deserialize_background_value` is a hand-written `deserialize_with`, not +/// a derive, so there is no schema for schemars to infer without this. +impl JsonSchema for BackgroundValue { + fn schema_name() -> String { + "BackgroundValue".to_string() + } + + fn json_schema(gen: &mut schemars::gen::SchemaGenerator) -> schemars::schema::Schema { + use schemars::schema::*; + + let string_schema = gen.subschema_for::(); + let entry_schema = gen.subschema_for::(); + let array_schema: Schema = SchemaObject { + instance_type: Some(InstanceType::Array.into()), + array: Some(Box::new(ArrayValidation { + items: Some(SingleOrVec::Single(Box::new(entry_schema.clone()))), + ..Default::default() + })), + ..Default::default() + } + .into(); + + SchemaObject { + subschemas: Some(Box::new(SubschemaValidation { + one_of: Some(vec![string_schema, entry_schema, array_schema]), + ..Default::default() + })), + ..Default::default() + } + .into() + } +} + /// Resolved background after template expansion — ready for rendering. #[derive(Debug, Clone, Default, Serialize)] pub struct ResolvedBackground { @@ -640,3 +791,162 @@ mod halo_zone_opacity_tests { } } } + +/// Constat #3: `AnimatedBackground::deserialize` had (at least) three silent +/// sinks — an unknown `preset` name silently became `gradient_shift` with +/// `colors: []`; a malformed/mistyped `zones` array in the legacy `halo` +/// form silently emptied via `.ok().unwrap_or_default()`; and a +/// malformed/missing `colors` (or `gradient_type`) on the legacy +/// `gradient_shift` form did the exact same `.ok().unwrap_or_default()` +/// silent-empty even when `preset` was spelled *correctly* — which is the +/// worst-case symptom named in the audit: an entirely black video with zero +/// diagnostics, because an empty-colors gradient paints black. Also found +/// (and fixed alongside, same root cause: the legacy branch's `_ =>` +/// wildcard): a *correctly spelled* `"heropattern"` preset written in the +/// legacy flat form (no `heropattern: {...}` sub-object) silently fell +/// through to `gradient_shift` too, because the legacy match only had +/// explicit arms for `grid_dots`/`concentric_circles`/`halo`. +#[cfg(test)] +mod animated_background_silent_sink_tests { + use super::*; + use serde_json::json; + + #[test] + fn known_preset_gradient_shift_still_works() { + let bg: AnimatedBackground = serde_json::from_value(json!({ + "preset": "gradient_shift", + "colors": ["#111111", "#222222"], + "gradient_type": "radial", + "speed": 10 + })) + .unwrap(); + match bg.preset { + BackgroundPreset::GradientShift(cfg) => { + assert_eq!(cfg.colors, vec!["#111111", "#222222"]); + assert!(matches!(cfg.gradient_type, GradientType::Radial)); + } + other => panic!("expected GradientShift, got {other:?}"), + } + } + + #[test] + fn unknown_preset_name_is_a_named_error_not_a_silent_black_gradient() { + let err = serde_json::from_value::(json!({ + "preset": "starfield", + "speed": 10 + })) + .expect_err("an unknown preset must be rejected, not silently treated as gradient_shift"); + let msg = err.to_string(); + assert!( + msg.contains("starfield"), + "error must name the offending preset value, got: {msg}" + ); + } + + #[test] + fn missing_preset_key_is_a_named_error() { + let err = serde_json::from_value::(json!({ "speed": 10 })) + .expect_err("a missing `preset` must be rejected, not silently treated as gradient_shift with colors: []"); + assert!( + err.to_string().to_lowercase().contains("preset"), + "got: {err}" + ); + } + + #[test] + fn legacy_halo_zones_still_work() { + let bg: AnimatedBackground = serde_json::from_value(json!({ + "preset": "halo", + "zones": [{ "color": "#1E3A8A", "x": 0.1, "y": 0.2, "radius": 0.3 }] + })) + .unwrap(); + match bg.preset { + BackgroundPreset::Halo(cfg) => assert_eq!(cfg.zones.len(), 1), + other => panic!("expected Halo, got {other:?}"), + } + } + + #[test] + fn legacy_halo_malformed_zones_is_a_named_error_not_a_silent_empty_zones() { + let err = serde_json::from_value::(json!({ + "preset": "halo", + "zones": [{ "color": "#1E3A8A", "x": "not-a-number" }] + })) + .expect_err("a malformed zones entry must be rejected, not silently emptied"); + assert!( + err.to_string().contains("zones") || err.to_string().contains("x"), + "error should point at the offending field, got: {err}" + ); + } + + #[test] + fn legacy_halo_missing_zones_is_a_named_error_not_a_silent_empty_zones() { + let err = serde_json::from_value::(json!({ "preset": "halo" })) + .expect_err("missing zones must be rejected, not silently treated as an empty halo"); + assert!(err.to_string().contains("zones"), "got: {err}"); + } + + #[test] + fn legacy_gradient_shift_missing_colors_is_a_named_error_not_a_silent_black_gradient() { + // This is the exact worst-case symptom the audit names: preset is + // spelled *correctly*, but colors is missing/malformed -> silently + // empty colors -> a fully transparent gradient that paints black, + // with no diagnostic at all. + let err = serde_json::from_value::(json!({ + "preset": "gradient_shift", + "speed": 5 + })) + .expect_err("missing colors must error, not silently produce an empty (black) gradient"); + assert!(err.to_string().contains("colors"), "got: {err}"); + } + + #[test] + fn legacy_heropattern_is_recognised_not_silently_turned_into_gradient_shift() { + let bg: AnimatedBackground = serde_json::from_value(json!({ + "preset": "heropattern", + "pattern": "plus", + "color": "#ffffff", + "opacity": 0.2, + "scale": 1.5 + })) + .unwrap(); + match bg.preset { + BackgroundPreset::Heropattern(cfg) => { + assert_eq!(cfg.pattern, "plus"); + assert_eq!(cfg.scale, 1.5); + } + other => panic!("expected Heropattern, got {other:?}"), + } + } + + #[test] + fn legacy_heropattern_missing_pattern_is_a_named_error() { + let err = serde_json::from_value::(json!({ + "preset": "heropattern" + })) + .expect_err("heropattern with no pattern name must error"); + assert!(err.to_string().contains("pattern"), "got: {err}"); + } + + #[test] + fn direction_typo_is_a_named_error_not_a_silently_dropped_none() { + let err = serde_json::from_value::(json!({ + "preset": "grid_dots", + "colors": ["#fff"], + "direction": "diagonal" + })) + .expect_err("an unrecognised direction must be rejected, not silently dropped to None"); + assert!(err.to_string().contains("direction"), "got: {err}"); + } + + #[test] + fn direction_still_works_when_valid() { + let bg: AnimatedBackground = serde_json::from_value(json!({ + "preset": "grid_dots", + "colors": ["#fff"], + "direction": "up" + })) + .unwrap(); + assert!(matches!(bg.direction, Some(ScrollDirection::Up))); + } +} diff --git a/crates/rustmotion-core/src/schema/scenario.rs b/crates/rustmotion-core/src/schema/scenario.rs index 119db03..357fc5e 100644 --- a/crates/rustmotion-core/src/schema/scenario.rs +++ b/crates/rustmotion-core/src/schema/scenario.rs @@ -130,8 +130,16 @@ pub struct View { #[serde(default)] pub transition: Option, /// (world) Shared background: color string, animated entry, or array. + // Constat #5: `background`'s `deserialize_with` bypasses the normal + // derive, so schemars had nothing to infer a schema from — hence the + // `#[schemars(skip)]` this used to carry. But `View` is also + // `deny_unknown_fields` (-> `additionalProperties: false` in the + // exported schema), so skipping the property didn't just leave it + // undocumented: it made the exported schema declare invalid every view + // that actually sets `background`. `BackgroundValue` now has a real + // (manual) `JsonSchema` impl — see `background.rs` — so this can be a + // normal declared property again. #[serde(default, deserialize_with = "deserialize_background_value")] - #[schemars(skip)] pub background: Option, /// (world) Legacy shared animated backgrounds. #[serde( @@ -341,8 +349,11 @@ pub struct WorldPosition { pub struct Scene { pub duration: f64, /// Unified background: color string, animated entry (with optional $ref), or array. + // Constat #5: see the identical note on `View::background` — same + // `#[schemars(skip)]` + `deny_unknown_fields` combination made the + // exported schema declare invalid every `examples/*.json` scene that + // sets `background` (which is most of them). #[serde(default, deserialize_with = "deserialize_background_value")] - #[schemars(skip)] pub background: Option, #[serde(default)] pub children: Vec, @@ -415,6 +426,46 @@ pub struct CameraOrigin { pub y: f32, } +/// Every camera property `interpolate_camera_property` +/// (`crates/rustmotion/src/engine/render/scene.rs`, owned by the sibling +/// GEO workstream this wave — read-only here) actually looks up via +/// `camera.keyframes.iter().find(|k| k.property == property)`. Constat #4: +/// a `CameraKeyframe.property` outside this fixed set (or the dotted +/// `origin.x`/`origin.y` convention misspelled as `origin_x`/`originX`) +/// never matches that lookup — the keyframe track is silently ignored and +/// the camera just uses its static value for that property, with no error. +const KNOWN_CAMERA_PROPERTIES: &[&str] = &["x", "y", "zoom", "rotation", "origin.x", "origin.y"]; + +fn validate_camera_property(value: &str) -> Result<(), E> { + if KNOWN_CAMERA_PROPERTIES.contains(&value) { + return Ok(()); + } + let normalize = |s: &str| s.replace(['-', '_', ' '], ".").to_lowercase(); + let normalized = normalize(value); + if let Some(suggestion) = KNOWN_CAMERA_PROPERTIES + .iter() + .find(|known| normalize(known) == normalized) + { + Err(E::custom(format!( + "unknown camera keyframe property '{value}' — did you mean '{suggestion}'?" + ))) + } else { + Err(E::custom(format!( + "unknown camera keyframe property '{value}': expected one of {}", + KNOWN_CAMERA_PROPERTIES.join(", ") + ))) + } +} + +fn deserialize_camera_property<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + let s = String::deserialize(deserializer)?; + validate_camera_property::(&s)?; + Ok(s) +} + /// A keyframe for a camera property. #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] #[serde(deny_unknown_fields)] @@ -422,6 +473,7 @@ pub struct CameraKeyframe { /// The camera property to animate: "x", "y", "zoom", "rotation", /// "origin.x", "origin.y" (dotted form, matching the component keyframe /// convention for compound properties). + #[serde(deserialize_with = "deserialize_camera_property")] pub property: String, /// Time-value pairs for the animation. pub values: Vec, @@ -908,3 +960,57 @@ mod strict_schema_tests { assert_eq!(s.scenes.len(), 1); } } + +/// Constat #4 (camera half): `CameraKeyframe.property` is consumed by +/// `interpolate_camera_property` in `crates/rustmotion/src/engine/render/ +/// scene.rs` (read-only for this workstream — owned by the sibling GEO +/// workstream this wave), which looks up +/// `camera.keyframes.iter().find(|k| k.property == property)` for each of a +/// *fixed* set of six properties (`"x"`, `"y"`, `"zoom"`, `"rotation"`, +/// `"origin.x"`, `"origin.y"`). A misspelled or wrongly-cased +/// `CameraKeyframe.property` simply never matches that lookup — the track +/// silently falls back to the camera's static value and never animates, +/// with no error anywhere. +#[cfg(test)] +mod camera_keyframe_property_tests { + use super::*; + + #[test] + fn known_camera_properties_still_work() { + for prop in ["x", "y", "zoom", "rotation", "origin.x", "origin.y"] { + let json = format!( + r#"{{ "property": "{prop}", "values": [ {{ "time": 0.0, "value": 1.0 }} ] }}"# + ); + let kf: CameraKeyframe = serde_json::from_str(&json) + .unwrap_or_else(|e| panic!("property '{prop}' must be accepted, got: {e}")); + assert_eq!(kf.property, prop); + } + } + + #[test] + fn unknown_camera_property_is_a_named_error_not_a_silent_no_op() { + let json = r#"{ "property": "tilt", "values": [ { "time": 0.0, "value": 1.0 } ] }"#; + let err = serde_json::from_str::(json).expect_err( + "an unrecognised camera keyframe property must be rejected, not silently inert", + ); + assert!(err.to_string().contains("tilt"), "got: {err}"); + } + + #[test] + fn misspelled_origin_property_is_a_named_error() { + // The documented dotted-compound-property convention + // (`origin.x`/`origin.y`) is easy to get wrong (`originX`, + // `origin_x`) — before this fix, any of those silently never + // animated the camera origin, with the keyframes block accepted + // and simply ignored. + let json = r#"{ "property": "origin_x", "values": [ { "time": 0.0, "value": 1.0 } ] }"#; + let err = serde_json::from_str::(json) + .expect_err("origin_x must be rejected — the real property is origin.x"); + let msg = err.to_string(); + assert!(msg.contains("origin_x"), "got: {msg}"); + assert!( + msg.contains("origin.x"), + "expected a did-you-mean nudge toward origin.x, got: {msg}" + ); + } +} diff --git a/crates/rustmotion-core/src/schema/video.rs b/crates/rustmotion-core/src/schema/video.rs index 01819ba..0f6b7a0 100644 --- a/crates/rustmotion-core/src/schema/video.rs +++ b/crates/rustmotion-core/src/schema/video.rs @@ -377,10 +377,113 @@ impl AnimationTiming { } } +/// Constat #4: every `property` name `engine::animator::{apply_property, +/// get_property_value}` (read-only for this workstream — the solver logic +/// itself stays there) actually recognises for `wiggle`/`keyframes` +/// animations. Anything outside this set has always been a silent no-op in +/// the solver (`_ => {}` / `_ => 0.0`): the animation plays as if the +/// property doesn't exist, with no error and no visual signal that +/// something is wrong. `WiggleConfig.property` and `Animation.property` +/// (the latter via `KeyframesConfig.keyframes`'s `deserialize_with`, since +/// `Animation` itself lives in `schema/animation.rs`, which this workstream +/// may only touch for `deny_unknown_fields`) are validated against this set +/// at parse time instead — turning the silent no-op into a named error, so +/// a mixed-convention typo (`"translateX"`, `"positionX"`, `"Rotation"`) or +/// a wholesale unsupported name is caught immediately. +/// +/// `"color"` is included because `resolve_animations` special-cases +/// `anim.property == "color"` outside `apply_property`/`get_property_value` +/// — it is a real, solver-recognised value for `Animation`, just resolved on +/// a different path than the numeric properties. +const KNOWN_MOTION_PROPERTIES: &[&str] = &[ + "opacity", + "position.x", + "translate_x", + "position.y", + "translate_y", + "scale", + "scale.x", + "scale.y", + "rotation", + "rotate_x", + "rotate_y", + "blur", + "visible_chars", + "visible_chars_progress", + "border_radius", + "font_size", + "width", + "height", + "gap", + "padding", + "stroke_width", + "shadow_blur", + "glow_radius", + "glow_intensity", + "perspective", + "draw_progress", + "motion_progress", + "color", +]; + +/// Reject a `property` value the solver doesn't recognise, with a +/// "did-you-mean" nudge when the only mismatch is casing/separator +/// convention (`translateX` / `translate-x` vs `translate_x`) — the exact +/// trap constat #4 names: this project mixes kebab-case (CSS-style, most of +/// `CssStyle`) and snake_case (these property names) conventions, and an +/// author reasoning from the former naturally reaches for the latter's +/// kebab or camelCase spelling. +fn validate_motion_property(value: &str) -> Result<(), E> { + if KNOWN_MOTION_PROPERTIES.contains(&value) { + return Ok(()); + } + let normalize = |s: &str| s.replace(['-', ' '], "_").to_lowercase(); + let normalized = normalize(value); + if let Some(suggestion) = KNOWN_MOTION_PROPERTIES + .iter() + .find(|known| normalize(known) == normalized) + { + Err(E::custom(format!( + "unknown animation property '{value}' — did you mean '{suggestion}'?" + ))) + } else { + Err(E::custom(format!( + "unknown animation property '{value}': expected one of {}", + KNOWN_MOTION_PROPERTIES.join(", ") + ))) + } +} + +fn deserialize_motion_property<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + let s = String::deserialize(deserializer)?; + validate_motion_property::(&s)?; + Ok(s) +} + +/// Validates every keyframe's `property` the same way +/// [`deserialize_motion_property`] does for `WiggleConfig` — `Animation` +/// itself lives in `schema/animation.rs`, out of reach for anything beyond +/// `deny_unknown_fields` in this workstream, so the check is applied here, +/// at the one field that actually consumes `Vec` in this file. +fn deserialize_validated_keyframes<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + let animations = Vec::::deserialize(deserializer)?; + for anim in &animations { + validate_motion_property::(&anim.property)?; + } + Ok(animations) +} + /// Custom keyframe animations configuration. #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] #[serde(deny_unknown_fields)] pub struct KeyframesConfig { + #[serde(deserialize_with = "deserialize_validated_keyframes")] pub keyframes: Vec, #[serde(default)] pub delay: f64, @@ -503,6 +606,7 @@ fn default_orbit_depth() -> f64 { #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] #[serde(deny_unknown_fields)] pub struct WiggleConfig { + #[serde(deserialize_with = "deserialize_motion_property")] pub property: String, pub amplitude: f64, pub frequency: f64, @@ -758,3 +862,120 @@ fn default_shadow_blur() -> f32 { fn default_text_bg_padding() -> f32 { 8.0 } + +#[cfg(test)] +mod motion_property_tests { + use super::*; + use serde_json::json; + + // ---- constat #4: `WiggleConfig.property` / `Animation.property` (via + // `KeyframesConfig.keyframes`) are free strings the solver silently + // no-ops on when unrecognised (RED first). ---- + + #[test] + fn wiggle_known_property_still_works() { + let json = json!({ + "name": "wiggle", + "property": "translate_x", + "amplitude": 10.0, + "frequency": 1.0 + }); + let effect: AnimationEffect = serde_json::from_value(json).unwrap(); + match effect { + AnimationEffect::Wiggle(cfg) => assert_eq!(cfg.property, "translate_x"), + other => panic!("expected Wiggle, got {other:?}"), + } + } + + #[test] + fn wiggle_unknown_property_is_a_named_error_not_a_silent_no_op() { + // A wholly unsupported name — the animation would otherwise play, + // resolve every frame, and simply never touch any rendered + // property: no error, no visible effect, no signal at all. + let json = json!({ + "name": "wiggle", + "property": "skew", + "amplitude": 10.0, + "frequency": 1.0 + }); + let err = serde_json::from_value::(json) + .expect_err("an unrecognised wiggle property must be rejected, not silently inert"); + assert!(err.to_string().contains("skew"), "got: {err}"); + } + + #[test] + fn wiggle_kebab_case_property_gets_a_did_you_mean() { + // The exact trap named in constat #4: this project mixes kebab-case + // (CSS-style, most of `CssStyle`) and snake_case (these property + // names) conventions across files, so an author reasoning in + // kebab-case naturally writes `translate-x` instead of the + // solver's `translate_x` — silently inert before this fix. + let json = json!({ + "name": "wiggle", + "property": "translate-x", + "amplitude": 10.0, + "frequency": 1.0 + }); + let err = serde_json::from_value::(json) + .expect_err("kebab-case must not silently resolve to a snake_case no-op"); + let msg = err.to_string(); + assert!(msg.contains("translate-x"), "got: {msg}"); + assert!( + msg.contains("translate_x"), + "expected a did-you-mean nudge toward the correct spelling, got: {msg}" + ); + } + + #[test] + fn keyframes_animation_known_property_still_works() { + let json = json!({ + "name": "keyframes", + "keyframes": [ + { "property": "opacity", "keyframes": [ + { "time": 0.0, "value": 0.0 }, + { "time": 1.0, "value": 1.0 } + ]} + ] + }); + let effect: AnimationEffect = serde_json::from_value(json).unwrap(); + match effect { + AnimationEffect::Keyframes(cfg) => assert_eq!(cfg.keyframes[0].property, "opacity"), + other => panic!("expected Keyframes, got {other:?}"), + } + } + + #[test] + fn keyframes_animation_unknown_property_is_a_named_error() { + let json = json!({ + "name": "keyframes", + "keyframes": [ + { "property": "positionX", "keyframes": [ + { "time": 0.0, "value": 0.0 }, + { "time": 1.0, "value": 1.0 } + ]} + ] + }); + let err = serde_json::from_value::(json).expect_err( + "an unrecognised keyframe animation property must be rejected, not silently inert", + ); + assert!(err.to_string().contains("positionX"), "got: {err}"); + } + + #[test] + fn keyframes_animation_color_property_still_works() { + // "color" is solver-recognised (special-cased in + // `resolve_animations`, outside `apply_property`), not a numeric + // motion property — must not be rejected. + let json = json!({ + "name": "keyframes", + "keyframes": [ + { "property": "color", "keyframes": [ + { "time": 0.0, "value": "#000000" }, + { "time": 1.0, "value": "#ffffff" } + ]} + ] + }); + let effect: AnimationEffect = serde_json::from_value(json).unwrap(); + assert!(matches!(effect, AnimationEffect::Keyframes(_))); + } +} diff --git a/crates/rustmotion-core/src/variables.rs b/crates/rustmotion-core/src/variables.rs index d17e806..429b08a 100644 --- a/crates/rustmotion-core/src/variables.rs +++ b/crates/rustmotion-core/src/variables.rs @@ -257,17 +257,6 @@ pub fn apply_variables( map.remove("config"); } substitute(value, &merged, path)?; - - // Check for unresolved references - let unresolved = find_unresolved(value); - if let Some(name) = unresolved.into_iter().next() { - return Err(RustmotionError::UnresolvedVariable { - name, - path: path.to_string(), - }); - } - - Ok(()) } None => { // No config block. If overrides were supplied (e.g. from the CLI for an HTML @@ -277,9 +266,44 @@ pub fn apply_variables( substitute(value, ovr, path)?; } } - Ok(()) } } + + // Constat #7: `find_unresolved` used to run — and hard-fail the whole + // render/validate on its first hit — *only* inside the `Some(defs)` + // branch above, so the exact same leftover `$word` (a price tag, a + // terminal `$PATH`, a shell `$HOME`) was harmless in a document with no + // `config` block and fatal the moment an unrelated `config` block + // existed anywhere else in the same file. `find_unresolved` cannot + // structurally tell a genuine unresolved-reference typo apart from + // incidental literal-`$` content — by construction, every name in + // `defs` above is always present in `merged` (defaults ∪ overrides), so + // `substitute` can never leave a *declared* variable name unresolved; + // everything `find_unresolved` can still find here is, definitionally, + // *not* one of the variables this document declared. So: run the same + // scan unconditionally (fixing the "depends on an unrelated key" + // inconsistency), but report it as a loud warning rather than aborting + // the whole document — same fail-loud-not-silent contract already used + // elsewhere in this workstream (see `css::units::px_or_warn`), applied + // here because a hard rejection would break any existing scenario that + // legitimately has a `$` in its content and would newly break every one + // of those the moment it also gained a `config` block. + for name in find_unresolved(value) { + // Reuse `UnresolvedVariable`'s existing `Display` message (see + // `error.rs`) for the warning text instead of hand-rolling a new + // one — this is the same diagnostic, just no longer fatal. + let diagnostic = RustmotionError::UnresolvedVariable { + name, + path: path.to_string(), + }; + eprintln!( + "Warning: {diagnostic} — either a typo'd variable name or literal '$' content (a \ + price, a shell $PATH, ...); the literal text is kept as-is instead of failing the \ + render." + ); + } + + Ok(()) } /// For standalone rendering: apply defaults only (no overrides). @@ -461,4 +485,117 @@ mod tests { substitute(&mut val, &vars, "test").unwrap(); assert_eq!(val["text"], json!("Count: 42 items")); } + + // ---- constat #7: literal `$` fatality must not depend on an unrelated + // `config` key (RED first) ---- + + /// A document with **no** `config` block and a literal `$` in unrelated + /// content (a `terminal` line's `$PATH`) — this already succeeds today + /// (the bug is the *other* direction; this locks in it keeps working). + fn doc_with_literal_dollar_no_config() -> serde_json::Value { + json!({ + "video": { "width": 1080, "height": 1920 }, + "scenes": [{ + "duration": 3.0, + "children": [ + { "type": "terminal", "lines": ["echo $PATH", "cd $HOME/project"] }, + { "type": "text", "content": "Price: $100 today only" } + ] + }] + }) + } + + /// The exact same literal-`$` content, but the document also happens to + /// declare an unrelated `config` block (e.g. because it's a reusable + /// template with one templated field). Before the fix, this made + /// `apply_variables` return `Err(UnresolvedVariable)` and abort the + /// entire render/validate — for content the config block has nothing to + /// do with. + fn doc_with_literal_dollar_and_unrelated_config() -> serde_json::Value { + json!({ + "config": { + "title": { "type": "string", "default": "Demo" } + }, + "video": { "width": 1080, "height": 1920 }, + "scenes": [{ + "duration": 3.0, + "children": [ + { "type": "text", "content": "$title" }, + { "type": "terminal", "lines": ["echo $PATH", "cd $HOME/project"] }, + { "type": "text", "content": "Price: $100 today only" } + ] + }] + }) + } + + #[test] + fn literal_dollar_without_config_block_already_succeeds() { + let mut doc = doc_with_literal_dollar_no_config(); + apply_defaults(&mut doc).expect( + "a literal '$' in terminal/text content with no config block must not be fatal", + ); + // Content is left as-is: nothing declared these as variables. + assert_eq!( + doc["scenes"][0]["children"][0]["lines"][0], + json!("echo $PATH") + ); + } + + #[test] + fn literal_dollar_with_unrelated_config_block_must_not_be_fatal() { + // RED before the fix: this currently returns + // `Err(UnresolvedVariable { name: "PATH", .. })` (or "HOME", or + // "100", whichever `find_unresolved` reaches first) purely because + // *some* config block exists elsewhere in the same document — the + // exact inconsistency named in constat #7. The declared `$title` + // variable must still resolve correctly either way. + let mut doc = doc_with_literal_dollar_and_unrelated_config(); + apply_defaults(&mut doc).expect( + "a literal '$' in unrelated content must not become fatal just because the \ + document also happens to declare an unrelated `config` block", + ); + assert_eq!(doc["scenes"][0]["children"][0]["content"], json!("Demo")); + assert_eq!( + doc["scenes"][0]["children"][1]["lines"][0], + json!("echo $PATH") + ); + assert_eq!( + doc["scenes"][0]["children"][2]["content"], + json!("Price: $100 today only") + ); + } + + #[test] + fn undeclared_override_is_still_a_hard_error_unaffected_by_the_fix() { + // The other half of `apply_variables`'s error surface (an override + // key that doesn't match any declared variable) is a genuine, + // unambiguous user error — unrelated to the literal-`$`-in-content + // ambiguity — and must remain a hard error. + let mut doc = json!({ + "config": { "title": { "type": "string", "default": "Demo" } }, + "video": { "width": 1, "height": 1 }, + "scenes": [] + }); + let mut overrides = HashMap::new(); + overrides.insert("nope".to_string(), json!("x")); + let err = apply_variables(&mut doc, Some(&overrides), "test.json") + .expect_err("an override referencing an undeclared variable must still be rejected"); + assert!(matches!( + err, + crate::error::RustmotionError::UndefinedVariable { .. } + )); + } + + #[test] + fn declared_variable_reference_still_resolves_with_no_override() { + let mut doc = json!({ + "config": { "greeting": { "type": "string", "default": "Hello" } }, + "video": { "width": 1, "height": 1 }, + "scenes": [{ "duration": 1.0, "children": [ + { "type": "text", "content": "$greeting" } + ]}] + }); + apply_defaults(&mut doc).unwrap(); + assert_eq!(doc["scenes"][0]["children"][0]["content"], json!("Hello")); + } } diff --git a/crates/rustmotion-core/tests/exported_schema_examples.rs b/crates/rustmotion-core/tests/exported_schema_examples.rs new file mode 100644 index 0000000..387625e --- /dev/null +++ b/crates/rustmotion-core/tests/exported_schema_examples.rs @@ -0,0 +1,234 @@ +//! Constat #5: `rustmotion schema` used to export a `Scene`/`View` with +//! `additionalProperties: false` (from `deny_unknown_fields`) but no +//! `background` property (from `#[schemars(skip)]` on a field with no +//! `JsonSchema` impl for its type). The two combined meant the exported +//! schema declared *every* scenario in `examples/` that uses `background` +//! invalid — the schema is exactly what generators (LLMs) are meant to +//! target, so this is the sink 3 users can't work around. +//! +//! This test is a minimal, dependency-free (no external JSON-Schema crate — +//! adding one is out of this workstream's file scope) structural validator: +//! it understands exactly the subset of JSON Schema draft-07 that +//! `schemars` 0.8 actually emits for this codebase (`$ref`, `definitions`, +//! `type`, `properties`/`additionalProperties`/`required`, `items`, +//! `oneOf`/`anyOf`/`allOf`, `enum`). It is not a general-purpose validator, +//! but it is precise about the one thing constat #5 is about: +//! `additionalProperties: false` combined with a missing declared property. + +use serde_json::Value; +use std::collections::BTreeSet; +use std::path::PathBuf; + +fn examples_dir() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../examples") +} + +/// Resolve a `$ref` like `#/definitions/Scene` against the schema root. +fn resolve<'a>(root: &'a Value, ref_str: &str) -> &'a Value { + let path = ref_str.strip_prefix("#/").unwrap_or(ref_str); + let mut cur = root; + for part in path.split('/') { + cur = cur + .get(part) + .unwrap_or_else(|| panic!("dangling $ref segment '{part}' in '{ref_str}'")); + } + cur +} + +/// Validate `instance` against `schema` (a node within `root`). Appends a +/// human-readable message to `errors` for every violation found, prefixed +/// with `path`. This intentionally does not stop at the first violation +/// (matches how the equivalent Python `jsonschema` run was cross-checked). +fn check(root: &Value, schema: &Value, instance: &Value, path: &str, errors: &mut Vec) { + // `true` / `{}` accept anything. + if schema.as_bool() == Some(true) { + return; + } + if let Some(obj) = schema.as_object() { + if obj.is_empty() { + return; + } + } + + if let Some(r) = schema.get("$ref").and_then(|v| v.as_str()) { + check(root, resolve(root, r), instance, path, errors); + return; + } + + if let Some(all_of) = schema.get("allOf").and_then(|v| v.as_array()) { + for sub in all_of { + check(root, sub, instance, path, errors); + } + } + + if let Some(variants) = schema + .get("oneOf") + .or_else(|| schema.get("anyOf")) + .and_then(|v| v.as_array()) + { + let mut best: Option> = None; + for variant in variants { + let mut sub_errors = Vec::new(); + check(root, variant, instance, path, &mut sub_errors); + if sub_errors.is_empty() { + return; // one matching variant is enough + } + if best.as_ref().is_none_or(|b| sub_errors.len() < b.len()) { + best = Some(sub_errors); + } + } + if let Some(b) = best { + errors.push(format!( + "{path}: matched no oneOf/anyOf variant (closest variant errors: {b:?})" + )); + } + return; + } + + if let Some(expected) = schema.get("enum").and_then(|v| v.as_array()) { + if !expected.contains(instance) { + errors.push(format!("{path}: {instance} is not one of {expected:?}")); + } + return; + } + + if let Some(ty) = schema.get("type").and_then(|v| v.as_str()) { + let matches = match ty { + "object" => instance.is_object(), + "array" => instance.is_array(), + "string" => instance.is_string(), + "number" => instance.is_number(), + "integer" => instance.is_i64() || instance.is_u64(), + "boolean" => instance.is_boolean(), + "null" => instance.is_null(), + _ => true, + }; + if !matches { + errors.push(format!("{path}: expected type {ty}, got {instance}")); + return; + } + } + + if let Some(props) = schema.get("properties").and_then(|v| v.as_object()) { + if let Some(inst_obj) = instance.as_object() { + let required: BTreeSet<&str> = schema + .get("required") + .and_then(|v| v.as_array()) + .map(|a| a.iter().filter_map(|v| v.as_str()).collect()) + .unwrap_or_default(); + for key in &required { + if !inst_obj.contains_key(*key) { + errors.push(format!("{path}: missing required field '{key}'")); + } + } + let additional = schema.get("additionalProperties"); + for (k, v) in inst_obj { + if let Some(sub_schema) = props.get(k) { + check(root, sub_schema, v, &format!("{path}/{k}"), errors); + } else { + match additional { + Some(Value::Bool(false)) => { + errors.push(format!( + "{path}: additional property '{k}' is not allowed by the schema \ + (declared properties: {:?})", + props.keys().collect::>() + )); + } + Some(Value::Bool(true)) | None => {} + Some(sub_schema) => { + check(root, sub_schema, v, &format!("{path}/{k}"), errors); + } + } + } + } + } + } + + if let Some(items_schema) = schema.get("items") { + if let Some(arr) = instance.as_array() { + for (i, item) in arr.iter().enumerate() { + check(root, items_schema, item, &format!("{path}[{i}]"), errors); + } + } + } +} + +/// Every `examples/*.json` file must validate against the schema +/// `rustmotion schema` exports (i.e. `generate_json_schema()` — the CLI +/// command only additionally wires `Scene.children` to the `Component` +/// union, which is irrelevant to constat #5's `background` defect and out +/// of this workstream's file scope to reproduce here). +/// +/// `ferriskey-presentation.json` is excluded: it fails plain `rustmotion +/// validate` today for an unrelated, pre-existing geometry overflow (issue +/// #157, out of this workstream's scope) — but per the baseline run below, +/// it has zero *schema* violations even before this fix, so excluding it +/// from the loop changes nothing about what this test proves. +#[test] +fn all_examples_validate_against_the_exported_schema() { + let schema = rustmotion_core::schema::generate_json_schema(); + let mut failures = Vec::new(); + + let mut count = 0; + for entry in std::fs::read_dir(examples_dir()).expect("examples/ dir must exist") { + let entry = entry.unwrap(); + let path = entry.path(); + if path.extension().and_then(|e| e.to_str()) != Some("json") { + continue; + } + count += 1; + let raw = std::fs::read_to_string(&path).unwrap(); + let doc: Value = serde_json::from_str(&raw).unwrap(); + let mut errors = Vec::new(); + check( + &schema, + &schema, + &doc, + path.file_name().unwrap().to_str().unwrap(), + &mut errors, + ); + if !errors.is_empty() { + failures.push(format!( + "{}: {} violation(s), first: {}", + path.display(), + errors.len(), + errors[0] + )); + } + } + + assert!( + count >= 8, + "expected at least 8 example files, found {count}" + ); + assert!( + failures.is_empty(), + "the following examples/*.json fail to validate against `rustmotion schema`'s output:\n{}", + failures.join("\n") + ); +} + +/// Narrower, more direct regression lock for the exact defect: `Scene` and +/// `View` must both declare `background` as a property in the exported +/// schema. Kept alongside the full-document check above because this is +/// the precise structural fact constat #5 is about, independent of whatever +/// else may be in the document. +#[test] +fn scene_and_view_schema_both_declare_a_background_property() { + let schema = rustmotion_core::schema::generate_json_schema(); + for name in ["Scene", "View"] { + let def = schema + .pointer(&format!("/definitions/{name}")) + .unwrap_or_else(|| panic!("no definitions/{name} in exported schema")); + assert_eq!( + def.get("additionalProperties"), + Some(&Value::Bool(false)), + "{name} must still be closed to unknown fields (deny_unknown_fields)" + ); + assert!( + def.pointer("/properties/background").is_some(), + "{name} must declare `background` as a property — it is a real, accepted field \ + (deserialize_background_value), not schemars(skip)-worthy dead schema" + ); + } +}