From e4427991bf304ad6f91239903a5bb1ab2c6dfb48 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 8 Aug 2026 13:32:49 +0200 Subject: [PATCH 01/15] style(fmt): apply rustfmt across the workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cargo fmt --all --check` is the first CI job and it has been failing on main: five sites across counter.rs, animator.rs and transition.rs were merged unformatted, so every pull request opened since inherits a red build regardless of its own contents. No behavioural change — rustfmt output only. --- crates/rustmotion-components/src/counter.rs | 5 ++- crates/rustmotion-core/src/engine/animator.rs | 39 +++++++++++++++++-- .../rustmotion-core/src/engine/transition.rs | 34 +++++++++++++--- 3 files changed, 67 insertions(+), 11 deletions(-) diff --git a/crates/rustmotion-components/src/counter.rs b/crates/rustmotion-components/src/counter.rs index 4bc85dd..d9511e4 100644 --- a/crates/rustmotion-components/src/counter.rs +++ b/crates/rustmotion-components/src/counter.rs @@ -295,7 +295,10 @@ mod tests { suffix: None, easing: EasingType::default(), duration, - timing: TimingConfig { start_at, end_at: None }, + timing: TimingConfig { + start_at, + end_at: None, + }, style: CssStyle::default(), timeline: Vec::new(), stagger: None, diff --git a/crates/rustmotion-core/src/engine/animator.rs b/crates/rustmotion-core/src/engine/animator.rs index ecba7cf..a23148e 100644 --- a/crates/rustmotion-core/src/engine/animator.rs +++ b/crates/rustmotion-core/src/engine/animator.rs @@ -1393,10 +1393,41 @@ fn expand_preset_inner(preset: &AnimationPreset, config: &PresetConfig) -> Vec 128, "outgoing plane faded too far: {left_red}"); - assert!(right_blue > 128, "incoming plane still too faint: {right_blue}"); + assert!( + right_blue > 128, + "incoming plane still too faint: {right_blue}" + ); } fn solid(width: u32, height: u32, r: u8, g: u8, b: u8, a: u8) -> Vec { From 2cd338ae9effb791227de4b3b18dff367e94830f Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 8 Aug 2026 13:38:10 +0200 Subject: [PATCH 02/15] fix(encode): order ffmpeg inputs ahead of output options (#143) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(encode): order ffmpeg inputs ahead of output options FFmpeg parses argv positionally: an option applies to the next `-i` that follows it. The audio input was emitted after the codec block, so `-c:v`, `-crf`, `-profile:v` and `-pix_fmt` were read as *input* options for audio.raw and ffmpeg refused to start with "Option profile:v cannot be applied to input url". Every scenario carrying an audio track — or an embedded video with a soundtrack — failed to encode, on all four codecs, through the default path. Move the audio input next to the video input and keep `-c:a`/`-b:a` with the output description. The argv assembly moves into `ffmpeg_args`, a pure function, so the ordering invariant is unit-testable without an ffmpeg binary on the machine. A broken pipe here almost always means ffmpeg already died on its own arguments, so `FfmpegWrite` now carries the tail of ffmpeg's stderr. It used to be printed only outside `--quiet`, which left the diagnosis of this very bug reading "Failed to write to FFmpeg pipe: Broken pipe". * style(encode): apply rustfmt to the ffmpeg argv builder --- crates/rustmotion-core/src/error.rs | 10 +- crates/rustmotion/src/encode/video/ffmpeg.rs | 348 +++++++++++++------ 2 files changed, 241 insertions(+), 117 deletions(-) diff --git a/crates/rustmotion-core/src/error.rs b/crates/rustmotion-core/src/error.rs index 55e4a3f..02da951 100644 --- a/crates/rustmotion-core/src/error.rs +++ b/crates/rustmotion-core/src/error.rs @@ -161,8 +161,14 @@ pub enum RustmotionError { #[error("Failed to open FFmpeg stdin pipe")] FfmpegPipe, - #[error("Failed to write to FFmpeg pipe: {reason}")] - FfmpegWrite { reason: String }, + // A broken pipe here nearly always means ffmpeg already died on its own + // arguments, so the useful diagnostic is ffmpeg's stderr rather than our + // write error. Carry it in the error so it survives `--quiet`. + #[error("Failed to write to FFmpeg pipe: {reason}{}", .stderr.as_ref().map(|s| format!("\nffmpeg reported:\n{}", s)).unwrap_or_default())] + FfmpegWrite { + reason: String, + stderr: Option, + }, #[error("Failed to wait for FFmpeg: {reason}")] FfmpegWait { reason: String }, diff --git a/crates/rustmotion/src/encode/video/ffmpeg.rs b/crates/rustmotion/src/encode/video/ffmpeg.rs index af6fa2e..0884f76 100644 --- a/crates/rustmotion/src/encode/video/ffmpeg.rs +++ b/crates/rustmotion/src/encode/video/ffmpeg.rs @@ -10,6 +10,108 @@ use crate::schema::ResolvedScenario as Scenario; use super::tasks::{build_frame_tasks, render_frame_task}; use super::EncodeProgress; +/// Assemble FFmpeg's argument vector. +/// +/// The order is load-bearing. FFmpeg parses argv positionally: an option applies +/// to the *next* `-i` that follows it, or to the output when no input follows. So +/// the whole input section — including the audio input and its `-f s16le -ar -ac` +/// — has to be emitted before the first output option. Emitting the codec block +/// between the two inputs makes ffmpeg reject `-profile:v` as an input option for +/// audio.raw and refuse to start, which silently broke every scenario carrying an +/// audio track. +/// +/// Kept separate from the spawn so the ordering invariant is unit-testable without +/// an ffmpeg binary on the machine. +#[allow(clippy::too_many_arguments)] +fn ffmpeg_args( + width: u32, + height: u32, + fps: u32, + codec: &str, + crf_val: u8, + transparent: bool, + audio_input: Option<&str>, + output_path: &str, +) -> Vec { + let size = format!("{}x{}", width, height); + let framerate = fps.to_string(); + let crf = crf_val.to_string(); + let mut args: Vec = Vec::new(); + fn push(xs: &[&str], out: &mut Vec) { + out.extend(xs.iter().map(|s| (*s).to_string())) + } + + // ---- inputs ------------------------------------------------------------ + push(&["-y", "-loglevel", "error"], &mut args); + push(&["-f", "rawvideo", "-pixel_format", "rgba"], &mut args); + push(&["-video_size", &size], &mut args); + push(&["-framerate", &framerate], &mut args); + push(&["-i", "pipe:0"], &mut args); + + if let Some(path) = audio_input { + push( + &["-f", "s16le", "-ar", "44100", "-ac", "2", "-i", path], + &mut args, + ); + } + + // ---- output options ---------------------------------------------------- + let alpha_fmt = |with: &'static str, without: &'static str| { + if transparent { + with + } else { + without + } + }; + match codec { + "h265" | "hevc" => { + push( + &["-c:v", "libx265", "-crf", &crf, "-preset", "medium"], + &mut args, + ); + push(&["-pix_fmt", alpha_fmt("yuva420p", "yuv420p")], &mut args); + } + "vp9" => { + push( + &["-c:v", "libvpx-vp9", "-crf", &crf, "-b:v", "0"], + &mut args, + ); + push(&["-pix_fmt", alpha_fmt("yuva420p", "yuv420p")], &mut args); + } + "prores" => { + push(&["-c:v", "prores_ks", "-profile:v", "4"], &mut args); + push( + &["-pix_fmt", alpha_fmt("yuva444p10le", "yuv422p10le")], + &mut args, + ); + } + _ => { + push( + &[ + "-c:v", + "libx264", + "-crf", + &crf, + "-preset", + "medium", + "-profile:v", + "high10", + "-pix_fmt", + "yuv420p10le", + ], + &mut args, + ); + } + } + + if audio_input.is_some() { + push(&["-c:a", "aac", "-b:a", "128k"], &mut args); + } + + args.push(output_path.to_string()); + args +} + /// Encode using FFmpeg subprocess (for h265, vp9, prores, webm, mov, transparency) pub fn encode_with_ffmpeg( scenario: &Scenario, @@ -61,106 +163,38 @@ pub fn encode_with_ffmpeg( None }; + // Materialise the mixed PCM before the command is assembled: the audio input + // has to be declared next to the video input, ahead of every output option. + let audio_input: Option = match (&pcm_data, &audio_tmp_dir) { + (Some(pcm), Some(tmp_dir)) => { + let audio_path = tmp_dir.join("audio.raw"); + std::fs::write(&audio_path, pcm)?; + Some( + audio_path + .to_str() + .ok_or_else(|| RustmotionError::NonUtf8Path { + path: audio_path.to_string_lossy().into_owned(), + })? + .to_owned(), + ) + } + _ => None, + }; + // Build FFmpeg command let crf_val = crf.unwrap_or(23); let mut cmd = std::process::Command::new("ffmpeg"); - cmd.args([ - "-y", - "-loglevel", - "error", - "-f", - "rawvideo", - "-pixel_format", - "rgba", - "-video_size", - &format!("{}x{}", width, height), - "-framerate", - &fps.to_string(), - "-i", - "pipe:0", - ]); - - match codec { - "h265" | "hevc" => { - cmd.args([ - "-c:v", - "libx265", - "-crf", - &crf_val.to_string(), - "-preset", - "medium", - ]); - if transparent { - cmd.args(["-pix_fmt", "yuva420p"]); - } else { - cmd.args(["-pix_fmt", "yuv420p"]); - } - } - "vp9" => { - cmd.args([ - "-c:v", - "libvpx-vp9", - "-crf", - &crf_val.to_string(), - "-b:v", - "0", - ]); - if transparent { - cmd.args(["-pix_fmt", "yuva420p"]); - } else { - cmd.args(["-pix_fmt", "yuv420p"]); - } - } - "prores" => { - cmd.args(["-c:v", "prores_ks", "-profile:v", "4"]); - if transparent { - cmd.args(["-pix_fmt", "yuva444p10le"]); - } else { - cmd.args(["-pix_fmt", "yuv422p10le"]); - } - } - _ => { - cmd.args([ - "-c:v", - "libx264", - "-crf", - &crf_val.to_string(), - "-preset", - "medium", - "-profile:v", - "high10", - "-pix_fmt", - "yuv420p10le", - ]); - } - } - - if let Some(ref pcm) = pcm_data { - let audio_path = audio_tmp_dir.as_ref().unwrap().join("audio.raw"); - std::fs::write(&audio_path, pcm)?; - let audio_path_str = audio_path - .to_str() - .ok_or_else(|| RustmotionError::NonUtf8Path { - path: audio_path.to_string_lossy().into_owned(), - })?; - cmd.args([ - "-f", - "s16le", - "-ar", - "44100", - "-ac", - "2", - "-i", - audio_path_str, - "-c:a", - "aac", - "-b:a", - "128k", - ]); - } - - cmd.arg(output_path); + cmd.args(ffmpeg_args( + width, + height, + fps, + codec, + crf_val, + transparent, + audio_input.as_deref(), + output_path, + )); cmd.stdin(std::process::Stdio::piped()); cmd.stdout(std::process::Stdio::null()); // Always capture stderr so failures surface a useful diagnostic. We tee to @@ -205,6 +239,7 @@ pub fn encode_with_ffmpeg( if let Err(e) = stdin.write_all(&rgba) { pipe_error = Some(RustmotionError::FfmpegWrite { reason: e.to_string(), + stderr: None, // filled in below, once stderr is drained }); break; } @@ -239,7 +274,18 @@ pub fn encode_with_ffmpeg( let _ = std::fs::remove_dir_all(tmp_dir); } - if let Some(e) = pipe_error { + // ffmpeg's actual complaint sits in the last few lines of stderr. Build the + // summary once: every failure path needs it, and `--quiet` must not be the + // difference between a diagnosable error and "Broken pipe". + let stderr_summary = stderr_text + .as_ref() + .map(|s| { + let lines: Vec<&str> = s.lines().rev().take(8).collect(); + lines.into_iter().rev().collect::>().join("\n") + }) + .filter(|s| !s.trim().is_empty()); + + let tee_stderr = || { if !quiet { if let Some(ref text) = stderr_text { if !text.trim().is_empty() { @@ -247,27 +293,23 @@ pub fn encode_with_ffmpeg( } } } - return Err(e); + }; + + if let Some(e) = pipe_error { + tee_stderr(); + // A broken pipe means ffmpeg is already gone — its own error says why, + // ours only says we could not keep writing. Carry both. + return Err(match e { + RustmotionError::FfmpegWrite { reason, .. } => RustmotionError::FfmpegWrite { + reason, + stderr: stderr_summary, + }, + other => other, + }); } if !status.success() { - // Extract the last few lines of stderr — ffmpeg's actual error message - // typically appears in the final 5-10 lines. - let stderr_summary = stderr_text - .as_ref() - .map(|s| { - let lines: Vec<&str> = s.lines().rev().take(8).collect(); - lines.into_iter().rev().collect::>().join("\n") - }) - .filter(|s| !s.trim().is_empty()); - - if !quiet { - if let Some(ref text) = stderr_text { - if !text.trim().is_empty() { - eprintln!("{}", text); - } - } - } + tee_stderr(); return Err(RustmotionError::FfmpegFailed { stderr: stderr_summary, }); @@ -275,3 +317,79 @@ pub fn encode_with_ffmpeg( Ok(()) } + +#[cfg(test)] +mod tests { + use super::ffmpeg_args; + + /// Every option that describes the *output* has to sit after the last `-i`. + /// Put one before it and ffmpeg attaches it to the following input instead, + /// then aborts with "Option ... cannot be applied to input url". + const OUTPUT_OPTS: [&str; 6] = ["-c:v", "-crf", "-preset", "-profile:v", "-c:a", "-b:a"]; + + fn input_positions(args: &[String]) -> Vec { + args.iter() + .enumerate() + .filter(|(_, s)| s.as_str() == "-i") + .map(|(i, _)| i) + .collect() + } + + #[test] + fn the_audio_input_is_declared_before_every_output_option() { + for codec in ["h264", "h265", "vp9", "prores"] { + let args = ffmpeg_args(320, 240, 30, codec, 23, false, Some("/tmp/a.raw"), "o.mp4"); + let inputs = input_positions(&args); + assert_eq!( + inputs.len(), + 2, + "{codec}: expected a video and an audio input" + ); + + // The audio input keeps its own format options immediately ahead of it. + let audio_i = inputs[1]; + assert_eq!(args[audio_i - 1], "2", "{codec}: -ac lost before audio -i"); + assert_eq!(args[audio_i + 1], "/tmp/a.raw"); + + for opt in OUTPUT_OPTS { + if let Some(pos) = args.iter().position(|s| s == opt) { + assert!( + pos > audio_i, + "{codec}: {opt} is emitted at {pos}, before the audio input at {audio_i} — \ + ffmpeg would read it as an option of audio.raw and refuse to start" + ); + } + } + assert_eq!( + args.last().unwrap(), + "o.mp4", + "{codec}: output must be last" + ); + } + } + + #[test] + fn a_silent_scenario_declares_a_single_input_and_no_audio_codec() { + let args = ffmpeg_args(320, 240, 30, "h264", 23, false, None, "o.mp4"); + assert_eq!(input_positions(&args).len(), 1); + assert!(!args.iter().any(|s| s == "-c:a" || s == "-b:a")); + assert_eq!(args.last().unwrap(), "o.mp4"); + } + + #[test] + fn transparency_selects_an_alpha_pixel_format() { + for (codec, opaque, alpha) in [ + ("h265", "yuv420p", "yuva420p"), + ("vp9", "yuv420p", "yuva420p"), + ("prores", "yuv422p10le", "yuva444p10le"), + ] { + let pix = |t: bool| { + let a = ffmpeg_args(320, 240, 30, codec, 23, t, None, "o.mov"); + let i = a.iter().position(|s| s == "-pix_fmt").unwrap(); + a[i + 1].clone() + }; + assert_eq!(pix(false), opaque, "{codec} opaque"); + assert_eq!(pix(true), alpha, "{codec} transparent"); + } + } +} From e6277d01e2221eb54a7c81f8350d9f6ceb081a08 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 8 Aug 2026 14:03:01 +0200 Subject: [PATCH 03/15] fix(components): stop five painters aborting on schema-valid input (#144) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each of these renders a scenario that `rustmotion validate` accepts, and each one killed the encode mid-frame. A painter that panics takes the whole render with it, so the guard belongs in the painter rather than upstream. - shape: skia asserts `pos.len() == colors.len()` inside the gradient shader, so a `stops` list of a different length than `colors` aborted the process. Drop stops we cannot honour and let skia space the colours. - table: `"row_colors": []` deserializes to Some(vec![]), not None, so the default palette was never substituted and the modulo guard still indexed an empty slice. - tag_cloud: same shape — `palette()` handed back the caller's empty vec and the painter took `index % len()` on it. - dot_map: `dot_spacing: 0` makes the division +inf, and `inf as u32` saturates to u32::MAX, scheduling ~1.8e19 iterations. The geometry pass rejects 0.01 but not 0, so the floor has to live in the painter. Also caps the grid per axis so a large box cannot schedule unbounded work. - codeblock diff: `col`, `delete` and `insert` all counted bytes while the reveal interpolates a fraction of that total, so mid-animation offsets landed inside a multi-byte glyph and `replace_range` aborted. Switch the accounting to characters and convert to byte offsets only when slicing. This also fixes the animation itself: a CJK glyph used to take three reveal steps instead of one. The new integration test drives all five through the real pipeline — serde, box_builder, run_layout, paint_tree — so a fix that guarded the painter but left the component undeserialisable would still fail. dot_map paints on a worker with a deadline, because a runaway loop would otherwise wedge CI instead of reporting. --- .../src/codeblock/diff.rs | 50 +++-- crates/rustmotion-components/src/dot_map.rs | 17 +- crates/rustmotion-components/src/shape.rs | 10 +- crates/rustmotion-components/src/table.rs | 9 +- crates/rustmotion-components/src/tag_cloud.rs | 9 +- .../tests/degenerate_inputs.rs | 185 ++++++++++++++++++ 6 files changed, 255 insertions(+), 25 deletions(-) create mode 100644 crates/rustmotion-components/tests/degenerate_inputs.rs diff --git a/crates/rustmotion-components/src/codeblock/diff.rs b/crates/rustmotion-components/src/codeblock/diff.rs index 5726c80..864d614 100644 --- a/crates/rustmotion-components/src/codeblock/diff.rs +++ b/crates/rustmotion-components/src/codeblock/diff.rs @@ -444,7 +444,7 @@ pub(super) fn compute_word_diff(old_line: &str, new_line: &str) -> Vec { pending_delete.push_str(change.value()); @@ -456,7 +456,7 @@ pub(super) fn compute_word_diff(old_line: &str, new_line: &str) -> Vec Vec usize { + s.char_indices() + .nth(char_idx) + .map(|(i, _)| i) + .unwrap_or(s.len()) +} + pub(super) fn draw_cursor_edited_line( canvas: &Canvas, font: &Font, @@ -498,7 +511,10 @@ pub(super) fn draw_cursor_edited_line( return; } - let total_work: usize = edits.iter().map(|e| e.delete.len() + e.insert.len()).sum(); + let total_work: usize = edits + .iter() + .map(|e| e.delete.chars().count() + e.insert.chars().count()) + .sum(); if total_work == 0 { let highlighted = highlight_code(new_line, language, theme); if let Some(line) = highlighted.first() { @@ -515,32 +531,33 @@ pub(super) fn draw_cursor_edited_line( for edit in edits { let adjusted_col = (edit.col as i64 + offset_adjust).max(0) as usize; - let delete_len = edit.delete.len(); - let insert_len = edit.insert.len(); + let delete_len = edit.delete.chars().count(); + let insert_len = edit.insert.chars().count(); let edit_work = delete_len + insert_len; if work_done + edit_work <= chars_progress { - let end = (adjusted_col + delete_len).min(current_line.len()); - let start = adjusted_col.min(current_line.len()); - current_line.replace_range(start..end, &edit.insert); + let start = byte_at(¤t_line, adjusted_col); + let end = byte_at(¤t_line, adjusted_col + delete_len); + current_line.replace_range(start..end.max(start), &edit.insert); offset_adjust += insert_len as i64 - delete_len as i64; work_done += edit_work; } else { let remaining_progress = chars_progress - work_done; if remaining_progress < delete_len { let chars_deleted = remaining_progress; - let del_start = (adjusted_col + delete_len - chars_deleted).min(current_line.len()); - let del_end = (adjusted_col + delete_len).min(current_line.len()); + let first_deleted = adjusted_col + delete_len - chars_deleted; + let del_start = byte_at(¤t_line, first_deleted); + let del_end = byte_at(¤t_line, adjusted_col + delete_len); if del_start < del_end { current_line.replace_range(del_start..del_end, ""); } - cursor_col = Some(del_start.min(current_line.len())); + cursor_col = Some(first_deleted); } else { let chars_inserted = remaining_progress - delete_len; - let start = adjusted_col.min(current_line.len()); - let end = (adjusted_col + delete_len).min(current_line.len()); - let partial_insert = &edit.insert[..chars_inserted.min(edit.insert.len())]; - current_line.replace_range(start..end, partial_insert); + let start = byte_at(¤t_line, adjusted_col); + let end = byte_at(¤t_line, adjusted_col + delete_len); + let partial_insert = &edit.insert[..byte_at(&edit.insert, chars_inserted)]; + current_line.replace_range(start..end.max(start), partial_insert); cursor_col = Some(adjusted_col + chars_inserted); } break; @@ -562,7 +579,8 @@ pub(super) fn draw_cursor_edited_line( }; if should_show { - let prefix = ¤t_line[..col.min(current_line.len())]; + // `col` counts characters, like every other column here. + let prefix = ¤t_line[..byte_at(¤t_line, col)]; let (prefix_width, _) = font.measure_str(prefix, None); let cursor_x = x + prefix_width; let mut cursor_paint = paint_from_hex(cursor_color); diff --git a/crates/rustmotion-components/src/dot_map.rs b/crates/rustmotion-components/src/dot_map.rs index ae69d1f..8fa9438 100644 --- a/crates/rustmotion-components/src/dot_map.rs +++ b/crates/rustmotion-components/src/dot_map.rs @@ -154,12 +154,23 @@ impl DotMap { world_paint.set_style(PaintStyle::Fill); world_paint.set_anti_alias(true); - let spacing = self.dot_spacing; + // A spacing at or below zero makes the division +inf, and `inf as u32` + // saturates to u32::MAX in Rust — the nested loop below would then be + // scheduled for ~1.8e19 iterations and never return. The geometry pass + // rejects 0.01 but not 0, so the floor has to live here. + let spacing = if self.dot_spacing.is_finite() && self.dot_spacing >= 1.0 { + self.dot_spacing + } else { + 1.0 + }; let radius = self.dot_radius; let margin = spacing; - let cols = ((w - margin * 2.0) / spacing) as u32; - let rows = ((h - margin * 2.0) / spacing) as u32; + // Belt and braces: even a legal spacing on a very large box should not + // be able to schedule an unbounded amount of work. + const MAX_DOTS_PER_AXIS: u32 = 4096; + let cols = (((w - margin * 2.0) / spacing) as u32).min(MAX_DOTS_PER_AXIS); + let rows = (((h - margin * 2.0) / spacing) as u32).min(MAX_DOTS_PER_AXIS); for row in 0..rows { for col in 0..cols { diff --git a/crates/rustmotion-components/src/shape.rs b/crates/rustmotion-components/src/shape.rs index 5e58a8d..df6ffc1 100644 --- a/crates/rustmotion-components/src/shape.rs +++ b/crates/rustmotion-components/src/shape.rs @@ -61,7 +61,15 @@ impl Painter for Shape { .iter() .map(|c| color4f_from_hex(c)) .collect(); - let stops: Option> = gradient.stops.clone(); + // skia asserts `pos.len() == colors.len()` inside the gradient + // shader — a mismatch aborts the process instead of erroring. + // Nothing upstream enforces the pairing, so drop stops we + // cannot honour and let skia distribute the colours evenly. + let stops: Option> = gradient + .stops + .as_ref() + .filter(|s| s.len() == colors.len()) + .cloned(); let mut paint = Paint::default(); paint.set_anti_alias(true); diff --git a/crates/rustmotion-components/src/table.rs b/crates/rustmotion-components/src/table.rs index 806eb99..0c0bfab 100644 --- a/crates/rustmotion-components/src/table.rs +++ b/crates/rustmotion-components/src/table.rs @@ -146,7 +146,14 @@ impl Table { let text_color = self.style.color_str_or("#FFFFFF"); let header_text_color = self.header_text_color.as_deref().unwrap_or("#FFFFFF"); let default_row_colors = vec!["#1F2937".to_string(), "#111827".to_string()]; - let row_colors = self.row_colors.as_ref().unwrap_or(&default_row_colors); + // `"row_colors": []` deserializes to Some(vec![]), not None — a generator + // writes it to mean "no striping". Row painting indexes this slice, so an + // empty one has to fall back rather than reach the painter. + let row_colors = self + .row_colors + .as_ref() + .filter(|c| !c.is_empty()) + .unwrap_or(&default_row_colors); // Resolve fonts before the optional clip below so an early return on // font failure keeps canvas save/restore balanced. diff --git a/crates/rustmotion-components/src/tag_cloud.rs b/crates/rustmotion-components/src/tag_cloud.rs index eedad5a..93fdf21 100644 --- a/crates/rustmotion-components/src/tag_cloud.rs +++ b/crates/rustmotion-components/src/tag_cloud.rs @@ -79,11 +79,12 @@ impl TagCloud { 1.0 - (1.0 - p).powi(3) } + /// Never empty: callers index into it with `%`, and `"colors": []` — which a + /// generator emits to mean "no custom palette" — otherwise divides by zero. fn palette(&self) -> Vec<&str> { - if let Some(colors) = &self.colors { - colors.iter().map(|s| s.as_str()).collect() - } else { - DEFAULT_PALETTE.to_vec() + match &self.colors { + Some(colors) if !colors.is_empty() => colors.iter().map(|s| s.as_str()).collect(), + _ => DEFAULT_PALETTE.to_vec(), } } diff --git a/crates/rustmotion-components/tests/degenerate_inputs.rs b/crates/rustmotion-components/tests/degenerate_inputs.rs new file mode 100644 index 0000000..8b609f3 --- /dev/null +++ b/crates/rustmotion-components/tests/degenerate_inputs.rs @@ -0,0 +1,185 @@ +//! A painter must never panic — nor hang — on input the schema accepts. +//! +//! `rustmotion validate` is the documented gate before delivery, and it answers +//! "Valid scenario" for every component below. Each one was observed aborting or +//! wedging the renderer mid-frame, which kills the whole encode: the scenarios +//! here are the reproductions, verbatim, kept as a regression floor. +//! +//! Every case routes through the real pipeline — serde, box_builder, run_layout, +//! paint_tree — so a fix that only guards the painter while leaving the component +//! undeserialisable would still fail. + +use std::sync::mpsc; +use std::time::Duration; + +use rustmotion_components::box_builder::{build_scene_with_anim, BuildAnimationCtx}; +use rustmotion_components::legacy_dispatch::LegacyPaintDispatcher; +use rustmotion_components::{ChildComponent, Component, PositionMode}; +use rustmotion_core::css::taffy_bridge::ConversionContext; +use rustmotion_core::engine::layout_pass::run_layout; +use rustmotion_core::engine::paint_pass::{paint_tree, PaintFrame}; + +const W: u32 = 400; +const H: u32 = 300; + +/// Deserialize one component and paint it at `time`. Panics propagate — that is +/// the point of the test. +fn paint(json: serde_json::Value, time: f64) { + let component: Component = serde_json::from_value(json).expect("component is schema-valid"); + let children = vec![ChildComponent { + component, + position: Some(PositionMode::Absolute { x: 0.0, y: 0.0 }), + x: None, + y: None, + z_index: None, + bleed: false, + }]; + + let mut surface = + skia_safe::surfaces::raster_n32_premul((W as i32, H as i32)).expect("raster surface"); + let canvas = surface.canvas(); + canvas.clear(skia_safe::Color4f::new(0.0, 0.0, 0.0, 0.0)); + + let built = build_scene_with_anim( + &children, + (W as f32, H as f32), + BuildAnimationCtx { + time, + scene_duration: 2.0, + fps: 30, + }, + ); + let layout = run_layout( + &built.root, + (W as f32, H as f32), + &ConversionContext::default(), + ); + let dispatcher = LegacyPaintDispatcher::for_scene(&built); + let frame = PaintFrame { + time, + frame_index: (time * 30.0) as u32, + fps: 30, + video_width: W, + video_height: H, + scene_duration: 2.0, + camera: None, + }; + paint_tree(canvas, &built.root, &layout, &frame, &dispatcher); +} + +/// Paint on a worker so a runaway loop fails the test instead of wedging the +/// suite. `dot_spacing: 0` used to spin for billions of iterations; a plain +/// `paint()` call would hang CI rather than report. +fn paint_within(json: serde_json::Value, time: f64, budget: Duration, what: &str) { + let (tx, rx) = mpsc::channel(); + let worker = std::thread::spawn(move || { + paint(json, time); + let _ = tx.send(()); + }); + match rx.recv_timeout(budget) { + Ok(()) => worker.join().expect(what), + Err(_) => panic!("{what}: still painting after {budget:?} — runaway loop"), + } +} + +#[test] +fn shape_survives_a_gradient_whose_stops_do_not_match_its_colors() { + // skia asserts pos.len() == colors.len() inside the gradient shader, so a + // mismatched `stops` aborted the process rather than returning an error. + for gradient_type in ["linear", "radial"] { + paint( + serde_json::json!({ + "type": "shape", + "shape": "rect", + "style": { "width": 200, "height": 100 }, + "fill": { + "type": gradient_type, + "colors": ["#FF0000", "#0000FF"], + "stops": [0.0, 0.5, 1.0] + } + }), + 0.5, + ); + } +} + +#[test] +fn table_survives_an_empty_row_colors_list() { + // `row_colors: []` deserializes to Some(vec![]), so the default palette was + // never substituted and the modulo guard still indexed an empty slice. + paint( + serde_json::json!({ + "type": "table", + "headers": ["A", "B"], + "rows": [["1", "2"], ["3", "4"]], + "row_colors": [], + "style": { "width": 300, "height": 200 } + }), + 0.5, + ); +} + +#[test] +fn tag_cloud_survives_an_empty_colors_list() { + // palette() returned the caller's empty vec, and the painter took + // `index % palette.len()` on it. + paint( + serde_json::json!({ + "type": "tag_cloud", + "tags": [{ "text": "rust", "weight": 3 }, { "text": "skia", "weight": 1 }], + "colors": [], + "style": { "width": 300, "height": 200 } + }), + 0.5, + ); +} + +#[test] +fn dot_map_terminates_on_a_zero_dot_spacing() { + // (w - 0) / 0 is +inf, and `inf as u32` saturates to u32::MAX in Rust, so the + // nested loop was scheduled for ~1.8e19 iterations. The geometry pass catches + // dot_spacing: 0.01 but not 0. + paint_within( + serde_json::json!({ + "type": "dot_map", + "points": [{ "lat": 48.8, "lng": 2.3 }], + "dot_spacing": 0, + "style": { "width": 300, "height": 150 } + }), + 0.5, + Duration::from_secs(10), + "dot_map with dot_spacing: 0", + ); +} + +#[test] +fn codeblock_diff_survives_multibyte_text() { + // The edit script counts bytes while the reveal interpolates a fraction of + // that count, so mid-animation offsets landed inside a multi-byte character + // and `replace_range` aborted with "not a char boundary". + for (from, to) in [ + ("let a = 1;", "let café = «héllo→»;"), + ("let x = 1;", "let y = \"éàü\";"), + ("a", "日本語のテキスト"), + ("ok", "🎬 clap"), + ] { + // Sweep the reveal: the panic only fires on the frames where progress + // lands part-way through a glyph. + for step in 0..=20 { + paint( + serde_json::json!({ + "type": "codeblock", + "code": from, + "language": "rust", + "diff": true, + "states": [ + { "code": from, "at": 0.0 }, + { "code": to, "at": 0.4, "cursor": { "enabled": true } } + ], + "style": { "width": 380, "height": 120 } + }), + f64::from(step) * 0.1, + ); + } + } +} From 69ba2abd98e7464c70cfce68c720d5787f9e60d4 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 8 Aug 2026 14:15:51 +0200 Subject: [PATCH 04/15] fix(cli): stop the helper commands destroying files they do not own (#145) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `skills install` wrote CLAUDE.md wholesale and `skills uninstall` deleted it, treating a file the user authors as rustmotion's property: a project with its own build notes lost them on install and lost the file on uninstall. rustmotion now claims a delimited block and never touches anything outside it — install merges the block in place, uninstall removes only the block and deletes the file solely when that block was all it held. `--fix` serialises `LoadedScenario::raw`, which is the document *after* variable substitution and include resolution — not the document on disk. For a plain JSON scenario the two coincide; for anything templated the write silently replaces the source with its own expansion. That single cause produced three separate defects: an HTML input came back as JSON, a `config` block and every `$var` disappeared (making `--var` a no-op on the rewritten file), and `include` got inlined so path-based patches landed on nodes the source never contained. One rule closes all three: only write back a source `--fix` can reproduce. Anything else is refused with a message naming the file and what to do instead. The check reads the bytes on disk rather than the loaded tree, because by then substitution has already erased the markers that make the write unfaithful. --- crates/rustmotion-cli/src/claude_md.rs | 128 +++++++++++++++++ .../rustmotion-cli/src/commands/validate.rs | 135 ++++++++++++++++++ crates/rustmotion-cli/src/lib.rs | 1 + crates/rustmotion-cli/src/skills.rs | 27 +++- 4 files changed, 285 insertions(+), 6 deletions(-) create mode 100644 crates/rustmotion-cli/src/claude_md.rs diff --git a/crates/rustmotion-cli/src/claude_md.rs b/crates/rustmotion-cli/src/claude_md.rs new file mode 100644 index 0000000..00a8da6 --- /dev/null +++ b/crates/rustmotion-cli/src/claude_md.rs @@ -0,0 +1,128 @@ +//! Merging rustmotion's guidance into a project `CLAUDE.md` without owning the file. +//! +//! `skills install` used to write `CLAUDE.md` wholesale and `skills uninstall` used +//! to delete it. Both treated a file the user authors as rustmotion's property: a +//! project with its own build notes lost them on install, and lost the file itself +//! on uninstall. +//! +//! Instead, rustmotion claims a delimited block and never touches anything outside +//! it. The markers are HTML comments so they stay invisible in rendered Markdown. + +pub const START: &str = ""; +pub const END: &str = ""; + +/// Byte range of the rustmotion block, markers included. +fn block_span(text: &str) -> Option<(usize, usize)> { + let start = text.find(START)?; + let end = text[start..].find(END)? + start + END.len(); + Some((start, end)) +} + +/// The document to write on install. +/// +/// - no file yet: the block alone; +/// - a file without a block: the block appended, existing content untouched; +/// - a file with a block: only the block replaced, in place. +pub fn merge(existing: Option<&str>, body: &str) -> String { + let block = format!("{START}\n{}\n{END}\n", body.trim_end()); + let Some(existing) = existing else { + return block; + }; + match block_span(existing) { + Some((start, end)) => { + let mut out = String::with_capacity(existing.len() + block.len()); + out.push_str(&existing[..start]); + out.push_str(block.trim_end()); + out.push_str(&existing[end..]); + out + } + None if existing.trim().is_empty() => block, + None => { + let mut out = existing.to_string(); + if !out.ends_with('\n') { + out.push('\n'); + } + out.push('\n'); + out.push_str(&block); + out + } + } +} + +/// The document to write on uninstall, or `None` when nothing rustmotion owns is +/// left and the file should be removed. +/// +/// A file the user also wrote in survives with its own content; a file that only +/// ever held our block is reported as removable. +pub fn strip(existing: &str) -> Option { + let Some((start, end)) = block_span(existing) else { + // No block: the file is entirely the user's. Never remove it. + return Some(existing.to_string()); + }; + let mut out = String::with_capacity(existing.len()); + out.push_str(&existing[..start]); + out.push_str(&existing[end..]); + if out.trim().is_empty() { + None + } else { + Some(format!("{}\n", out.trim_end())) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const BODY: &str = "# rustmotion\nGuidance."; + + #[test] + fn a_project_claude_md_survives_install_and_uninstall() { + let user = "# My Project\nBuild with `make`. Never delete this.\n"; + + let installed = merge(Some(user), BODY); + assert!( + installed.contains("Never delete this."), + "install destroyed the user's content: {installed}" + ); + assert!(installed.contains(BODY)); + + let uninstalled = strip(&installed).expect("a user-authored file is never removed"); + assert!(uninstalled.contains("Never delete this.")); + assert!( + !uninstalled.contains("Guidance."), + "uninstall left our block behind: {uninstalled}" + ); + assert!(!uninstalled.contains(START)); + } + + #[test] + fn reinstalling_replaces_the_block_instead_of_stacking_copies() { + let once = merge(None, BODY); + let twice = merge(Some(&once), "# rustmotion\nNewer guidance."); + assert_eq!(twice.matches(START).count(), 1, "block duplicated: {twice}"); + assert!(twice.contains("Newer guidance.")); + assert!(!twice.contains("Guidance.")); + } + + #[test] + fn a_file_we_alone_created_is_reported_as_removable() { + let ours = merge(None, BODY); + assert!(strip(&ours).is_none()); + } + + #[test] + fn a_file_without_our_block_is_returned_untouched() { + let user = "# Theirs\nnothing of ours here\n"; + assert_eq!(strip(user).as_deref(), Some(user)); + } + + #[test] + fn content_around_the_block_is_preserved_on_both_sides() { + let doc = format!("before\n\n{START}\nold\n{END}\n\nafter\n"); + let merged = merge(Some(&doc), BODY); + assert!(merged.starts_with("before")); + assert!(merged.trim_end().ends_with("after")); + assert!(merged.contains("Guidance.")); + assert!(!merged.contains("\nold\n")); + } +} diff --git a/crates/rustmotion-cli/src/commands/validate.rs b/crates/rustmotion-cli/src/commands/validate.rs index a5f65f6..a0aff12 100644 --- a/crates/rustmotion-cli/src/commands/validate.rs +++ b/crates/rustmotion-cli/src/commands/validate.rs @@ -27,6 +27,67 @@ fn announced_duration(scenario: &ResolvedScenario) -> f64 { rustmotion::encode::build_frame_tasks(scenario).len() as f64 / fps } +/// Why `--fix` must not write over this input. +/// +/// `--fix` serialises `LoadedScenario::raw`, which is the document *after* +/// variable substitution and `include` resolution — not the document on disk. For +/// a plain JSON scenario the two coincide and writing back is faithful. For +/// anything templated they do not, and the write silently replaces the source +/// with its own expansion: the `config` block and every `$var` disappear, includes +/// get inlined into the parent, and an HTML input is replaced by JSON outright. +/// +/// One rule covers all three: only write back a source `--fix` can reproduce. +#[derive(Debug, PartialEq, Eq)] +enum FixRefusal { + HtmlSource, + Templated, + UsesInclude, +} + +impl FixRefusal { + fn explain(&self, path: &Path) -> String { + let p = path.display(); + match self { + Self::HtmlSource => format!( + "--fix cannot rewrite {p}: it is an HTML source, and the fixer only knows how to \ + emit JSON — applying it would replace your markup with the transpiled scenario. \ + Apply the fix to the HTML by hand, or transpile first and fix the JSON." + ), + Self::Templated => format!( + "--fix cannot rewrite {p}: it declares `config` or uses `$variables`, and the \ + fixer would write back the substituted scenario — dropping the template and \ + making `--var` a silent no-op. Fix the template by hand." + ), + Self::UsesInclude => format!( + "--fix cannot rewrite {p}: it uses `include`, and the fixer would write back the \ + resolved tree — inlining the included files into the parent and patching by a \ + path that no longer means the same node. Fix the included file directly." + ), + } + } +} + +/// `None` when `--fix` may write over `input`. +fn refuse_fix(input: &Path, raw_source: &str) -> Option { + if rustmotion::loader::is_html_path(input) { + return Some(FixRefusal::HtmlSource); + } + // Inspect the bytes on disk, not the loaded tree: by then substitution has + // already erased the very markers that make the write unfaithful. + let source: serde_json::Value = match serde_json::from_str(raw_source) { + Ok(v) => v, + // Unparseable source is not something we should be overwriting either. + Err(_) => return Some(FixRefusal::Templated), + }; + if source.get("config").is_some() || raw_source.contains("$") { + return Some(FixRefusal::Templated); + } + if raw_source.contains("\"include\"") { + return Some(FixRefusal::UsesInclude); + } + None +} + pub fn cmd_validate( input: &PathBuf, report: Option<&Path>, @@ -57,6 +118,10 @@ pub fn cmd_validate( let mut applied_fixes = 0usize; if fix && !report_out.geom_violations.is_empty() { + let raw_source = std::fs::read_to_string(input).unwrap_or_default(); + if let Some(refusal) = refuse_fix(input, &raw_source) { + return Err(RustmotionError::Generic(refusal.explain(input))); + } let mut json_value = loaded.raw.clone(); applied_fixes = apply_fixes(&mut json_value, &report_out.geom_violations); if applied_fixes > 0 { @@ -431,4 +496,74 @@ mod tests { "expected 3.0s with no transitions, got {duration}" ); } + + /// `--fix` writes back the *resolved* tree. Anything the resolution erased is + /// erased on disk too, so these three inputs must be refused rather than + /// silently rewritten. + mod fix_refusals { + use super::super::{refuse_fix, FixRefusal}; + use std::path::Path; + + const PLAIN: &str = r#"{"video":{"width":320,"height":240,"fps":30}, + "scenes":[{"duration":1.0,"children":[]}]}"#; + + #[test] + fn a_plain_json_scenario_is_writable() { + assert_eq!(refuse_fix(Path::new("s.json"), PLAIN), None); + } + + #[test] + fn an_html_source_is_refused() { + // Writing here replaces the author's markup with transpiled JSON. + assert_eq!( + refuse_fix(Path::new("s.html"), ""), + Some(FixRefusal::HtmlSource) + ); + } + + #[test] + fn a_templated_scenario_is_refused() { + // The write would bake in the substitution and make --var a no-op. + let with_config = r#"{"config":{"title":"hi"},"video":{"width":320,"height":240, + "fps":30},"scenes":[{"duration":1.0,"children":[]}]}"#; + assert_eq!( + refuse_fix(Path::new("s.json"), with_config), + Some(FixRefusal::Templated) + ); + + let with_var = r#"{"video":{"width":320,"height":240,"fps":30}, + "scenes":[{"duration":1.0,"children":[ + {"type":"text","content":"$title"}]}]}"#; + assert_eq!( + refuse_fix(Path::new("s.json"), with_var), + Some(FixRefusal::Templated) + ); + } + + #[test] + fn a_scenario_using_include_is_refused() { + // The resolved tree inlines the include, so a path-based patch lands on + // a node the source file does not contain. + let with_include = r#"{"video":{"width":320,"height":240,"fps":30}, + "scenes":[{"include":"part.json"}]}"#; + assert_eq!( + refuse_fix(Path::new("s.json"), with_include), + Some(FixRefusal::UsesInclude) + ); + } + + #[test] + fn every_refusal_names_the_file_and_says_what_to_do_instead() { + let p = Path::new("scenes/hero.json"); + for r in [ + FixRefusal::HtmlSource, + FixRefusal::Templated, + FixRefusal::UsesInclude, + ] { + let msg = r.explain(p); + assert!(msg.contains("scenes/hero.json"), "{msg}"); + assert!(msg.contains("by hand") || msg.contains("directly"), "{msg}"); + } + } + } } diff --git a/crates/rustmotion-cli/src/lib.rs b/crates/rustmotion-cli/src/lib.rs index 013f489..be15b12 100644 --- a/crates/rustmotion-cli/src/lib.rs +++ b/crates/rustmotion-cli/src/lib.rs @@ -1,3 +1,4 @@ +mod claude_md; mod commands; mod skills; pub mod tui; diff --git a/crates/rustmotion-cli/src/skills.rs b/crates/rustmotion-cli/src/skills.rs index 33c1306..1267d67 100644 --- a/crates/rustmotion-cli/src/skills.rs +++ b/crates/rustmotion-cli/src/skills.rs @@ -192,10 +192,13 @@ pub fn install(global: bool) -> Result<()> { } } - // Write CLAUDE.md only in local mode + // Write CLAUDE.md only in local mode. The project may already own this file — + // merge into a delimited block rather than claiming the whole document. if !global { let claude_path = root.join("CLAUDE.md"); - if write_if_changed(&claude_path, CLAUDE_MD)? { + let existing = std::fs::read_to_string(&claude_path).ok(); + let merged = crate::claude_md::merge(existing.as_deref(), CLAUDE_MD); + if write_if_changed(&claude_path, &merged)? { written += 1; } else { skipped += 1; @@ -299,12 +302,24 @@ pub fn uninstall(global: bool) -> Result<()> { std::fs::remove_dir_all(&skills_dir)?; let mut removed = 1; - // Remove CLAUDE.md only in local mode + // Remove only what we put there. A CLAUDE.md carrying the project's own + // instructions keeps them; the file is deleted only when our block was all it + // ever held. if !global { let claude_path = root.join("CLAUDE.md"); - if claude_path.exists() { - std::fs::remove_file(&claude_path)?; - removed += 1; + if let Ok(existing) = std::fs::read_to_string(&claude_path) { + match crate::claude_md::strip(&existing) { + Some(remaining) => { + if remaining != existing { + std::fs::write(&claude_path, remaining)?; + removed += 1; + } + } + None => { + std::fs::remove_file(&claude_path)?; + removed += 1; + } + } } } From 025af5ae2ec9cf03e0017d0c46de217cf4ad111b Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 8 Aug 2026 14:57:07 +0200 Subject: [PATCH 05/15] fix(cli): make batch validate like render and report what actually happened (#146) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `batch` produces N videos in one shot, and it was the one path that told the truth about none of them. With `--jobs > 1` a worker panic was swallowed: `let _ = h.join()` was true to its comment ("panics are surfaced as failures below") in name only — nothing below inspected the join result, and the closure's bookkeeping only runs on the return path a panic skips. A batch where every render panicked printed "0/N succeeded" and exited 0. Panics are now recorded as failures, with a final guard should a row ever go neither counted nor reported. Preflight ran a parse-only dry run, so `batch` skipped the schema and geometry pass `render` applies — the viewport-overflow gate CLAUDE.md makes mandatory was bypassed by the very mode that renders in bulk. It now runs `validation::run_checks` per row, with render's no-flag defaults, and fails the batch before a single frame is drawn. `--name-template` interpolates values straight from the data file, so a row carrying `../escaped` or `/tmp/absolute` wrote outside `--output-dir`. Names are now rejected in preflight when they contain a parent-dir, root or prefix component. The check is lexical rather than canonicalising: a canonicalize-and-compare fails with ENOENT on a subdirectory that does not exist yet, which would have broken the legitimate `{lang}/{id}.mp4` form. --- crates/rustmotion-cli/src/commands/batch.rs | 506 +++++++++++++++++++- 1 file changed, 496 insertions(+), 10 deletions(-) diff --git a/crates/rustmotion-cli/src/commands/batch.rs b/crates/rustmotion-cli/src/commands/batch.rs index 5bf6d48..f1f358c 100644 --- a/crates/rustmotion-cli/src/commands/batch.rs +++ b/crates/rustmotion-cli/src/commands/batch.rs @@ -1,22 +1,29 @@ //! Batch rendering: render one video per line of a JSONL data file. //! //! Design invariants: -//! - All input is validated (JSONL parse + name template + variable checks) BEFORE -//! any render starts. A corrupt line does not waste render time. +//! - All input is validated (JSONL parse + name template + variable checks + +//! the same schema/geometry pass `render` runs, see `validation::run_checks`) +//! BEFORE any render starts. A corrupt or overflowing line does not waste +//! render time, and does not silently produce a bad video either. //! - Each data line is a JSON object; its fields become variable overrides. //! - `{field}` in the name template is replaced by the field's JSON-rendered value; -//! `{index}` by the 0-based line number. +//! `{index}` by the 0-based line number. The resolved name may create +//! subdirectories under `--output-dir` (e.g. `{lang}/{id}.mp4`) but may not +//! escape it: a `..` component or an absolute path is rejected in preflight. //! - Unknown variables (when the template has a `config` block) produce an actionable //! error listing declared variables — same as the single-file path. -//! - Exit code is non-zero if any render failed; partial success is reported. +//! - Exit code is non-zero if any render failed *or panicked* (`--jobs > 1` +//! dispatches renders across worker threads; a panic in one is caught and +//! counted as a failure, never silently dropped); partial success is reported. use rustmotion::error::{Result, RustmotionError}; use rustmotion::loader::load_input_with_vars; use std::collections::HashMap; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use std::sync::{Arc, Mutex}; use crate::commands::render::cmd_render; +use crate::commands::validation::{self, ValidationSource}; /// One row parsed from the JSONL data file. struct BatchRow { @@ -72,6 +79,38 @@ pub(crate) fn resolve_name_template( Ok(out) } +/// Reject a resolved output name that would escape `--output-dir` once joined +/// onto it. `Path::join` replaces the base entirely when the joined path is +/// absolute, and a `..` component walks back out of it regardless — either +/// way `output_dir.join(name)` can land anywhere on disk. `name` comes from +/// `{field}` substitution of the (untrusted) JSONL data file, so this check +/// runs in preflight, before any render starts, same as every other +/// preflight check in this module. +/// +/// A plain relative path — including one that creates subdirectories, e.g. +/// `"en/abc.mp4"` — remains legal: only `ParentDir` (`..`), `RootDir` +/// (a leading `/`), and `Prefix` (a Windows drive/UNC root) are rejected. +fn reject_escaping_name(name: &str) -> std::result::Result<(), String> { + for component in Path::new(name).components() { + match component { + Component::ParentDir => { + return Err(format!( + "resolved output name '{name}' contains a '..' component, \ + which would escape --output-dir" + )) + } + Component::RootDir | Component::Prefix(_) => { + return Err(format!( + "resolved output name '{name}' is an absolute path, \ + which would escape --output-dir" + )) + } + Component::CurDir | Component::Normal(_) => {} + } + } + Ok(()) +} + /// Parse the JSONL file, resolve output names, and validate overrides against /// the scenario template (dry-run load). Returns ordered rows or an error if /// anything is invalid. No rendering happens here. @@ -121,12 +160,35 @@ fn preflight( continue; } }; + if let Err(msg) = reject_escaping_name(&name) { + preflight_errors.push(format!("line {}: {}", index + 1, msg)); + continue; + } let output_path = output_dir.join(&name); - // Validate overrides against the template (dry-run: load then discard) - let path_buf = template_path.to_path_buf(); - if let Err(e) = load_input_with_vars(&path_buf, Some(&overrides)) { - preflight_errors.push(format!("line {}: {}", index + 1, e)); + // Validate overrides against the template: parse, resolve variables, + // then run the same schema + geometry pass `render` runs before it + // renders a single-file scenario (`validation::run_checks`, wired + // with the same defaults `render` uses with no flags: blocking + // geometry violations, no animated-frame sampling). A batch row is + // exactly the situation the module doc warns about — N videos + // produced in one shot — so it must not be the one path that skips + // the "unwrappable_text_overflow" / viewport-overflow gate CLAUDE.md + // requires. The loaded scenario itself is discarded here: `render_row` + // reloads it below, right before the actual render. + let loaded = match validation::load_with_vars( + ValidationSource::File(template_path), + Some(&overrides), + ) { + Ok(l) => l, + Err(e) => { + preflight_errors.push(format!("line {}: {}", index + 1, e)); + continue; + } + }; + let report = validation::run_checks(&loaded, false); + if report.is_blocking(false) { + preflight_errors.push(format!("line {}: {}", index + 1, report.to_error())); continue; } @@ -274,7 +336,23 @@ pub fn cmd_batch( .collect(); for h in handles { - let _ = h.join(); // panics in threads are surfaced as failures below + // `h.join()` returns `Err` when the worker thread panicked instead + // of returning normally (e.g. a codec assertion on an odd frame + // dimension). That row was neither counted in `success_arc` nor + // pushed to `failures` by the closure above — it *only* runs its + // bookkeeping on the `Ok`/`Err` return path, which a panic skips + // entirely. Left unhandled, `_ = h.join()` was true to its + // comment ("panics ... are surfaced as failures below") in name + // only: nothing below ever inspected the join result, so a batch + // where every worker panicked reported "0/N succeeded" and still + // returned `Ok(())` — exit code 0 for a batch that rendered + // nothing. Record it as a failure explicitly instead. + if let Err(payload) = h.join() { + failures.lock().unwrap().push(format!( + "worker thread panicked: {}", + panic_message(&payload) + )); + } } success_count = *success_arc.lock().unwrap(); } @@ -282,6 +360,19 @@ pub fn cmd_batch( let failure_list = failures.lock().unwrap().clone(); let fail_count = failure_list.len(); + // Defense in depth: every row must end up counted as either a success or + // a failure. This should already hold given the panic handling above, + // but if it doesn't — a future refactor drops a bookkeeping update, a + // panic happens somewhere neither counter is touched — fail loudly + // rather than silently report a batch as complete when it wasn't. + if success_count + fail_count != total { + return Err(RustmotionError::Generic(format!( + "batch accounting mismatch: {success_count} succeeded + {fail_count} failed \ + != {total} total item(s) — {} item(s) neither succeeded nor were reported as failed", + total.saturating_sub(success_count + fail_count) + ))); + } + if !quiet { eprintln!("Batch complete: {}/{} succeeded.", success_count, total); } @@ -297,6 +388,23 @@ pub fn cmd_batch( Ok(()) } +/// Extract a human-readable message from a caught thread panic payload. +/// `std::thread::Result`'s `Err` variant is `Box`; panics +/// raised via `panic!("...")` / `.unwrap()` / `.expect(...)` box either a +/// `&'static str` or a `String`, which covers the vast majority of real +/// panics (including the openh264 `assert_eq!` this fix was written for). +/// Anything else (a custom payload via `std::panic::panic_any`) still +/// produces a readable, if generic, message instead of losing the row. +fn panic_message(payload: &(dyn std::any::Any + Send)) -> String { + if let Some(s) = payload.downcast_ref::<&str>() { + (*s).to_string() + } else if let Some(s) = payload.downcast_ref::() { + s.clone() + } else { + "".to_string() + } +} + fn render_row( template_path: &Path, row: &BatchRow, @@ -382,6 +490,44 @@ mod name_template_tests { } } +#[cfg(test)] +mod escaping_name_tests { + use super::*; + + #[test] + fn plain_relative_name_is_allowed() { + assert!(reject_escaping_name("abc.mp4").is_ok()); + } + + /// `{lang}/{id}.mp4`-style names are a documented, legitimate feature + /// (`field_placeholder_resolved` above): rejecting every non-`Normal` + /// path component would also reject this, which is why only + /// `ParentDir`/`RootDir`/`Prefix` are rejected, not every multi-segment + /// name. + #[test] + fn relative_subdirectory_name_is_allowed() { + assert!(reject_escaping_name("en/abc.mp4").is_ok()); + } + + #[test] + fn parent_dir_component_is_rejected() { + let err = reject_escaping_name("../../escaped.mp4").unwrap_err(); + assert!( + err.contains(".."), + "error must call out the '..' component: {err}" + ); + } + + #[test] + fn absolute_unix_path_is_rejected() { + let err = reject_escaping_name("/tmp/scratch/pwned/absolute.mp4").unwrap_err(); + assert!( + err.contains("absolute"), + "error must call out the absolute path: {err}" + ); + } +} + #[cfg(test)] mod batch_integration_tests { use super::*; @@ -511,4 +657,344 @@ mod batch_integration_tests { let _ = std::fs::remove_file(&data); let _ = std::fs::remove_dir_all(&out_dir); } + + /// Constat #2/#4: a `{field}` value from the (untrusted) JSONL data file + /// containing `..` must not be able to walk `output_dir.join(name)` back + /// out of `--output-dir`. Reproduces the brief's scenario: `--output-dir + /// /sub/dir`, `--name-template '{name}.png'`, data field + /// `"../../escaped"` — before the fix this created + /// `/escaped.png/frame_00000.png`, outside `/sub/dir`. + #[test] + fn batch_rejects_name_that_escapes_output_dir_via_parent_dir() { + let template = write_json(&minimal_template("name"), "tmpl_escape_dotdot"); + let data = write_jsonl( + &[serde_json::json!({"name": "../../escaped"})], + "escape_dotdot", + ); + let base = std::env::temp_dir().join(format!("rm_batch_escdd_{}", std::process::id())); + let out_dir = base.join("sub").join("dir"); + std::fs::create_dir_all(&out_dir).unwrap(); + + let err = cmd_batch( + &template, + &data, + &out_dir, + "{name}.png", + None, + None, + Some("png-seq".to_string()), + false, + 1, + true, + ) + .expect_err("a name containing '..' must be rejected in preflight"); + + assert!( + err.to_string().contains(".."), + "error must call out the escaping component: {err}" + ); + + // `/sub/dir/../../escaped.png` lexically resolves to + // `/escaped.png` — it must never have been created. + let escaped_target = base.join("escaped.png"); + assert!( + !escaped_target.exists(), + "traversal target must not have been created: {}", + escaped_target.display() + ); + + let _ = std::fs::remove_file(&template); + let _ = std::fs::remove_file(&data); + let _ = std::fs::remove_dir_all(&base); + } + + /// Constat #2/#4, absolute-path variant: `Path::join` with an absolute + /// component discards the base entirely, so a `{field}` value that is + /// itself an absolute path writes straight to that path, ignoring + /// `--output-dir` completely. + #[test] + fn batch_rejects_name_that_is_an_absolute_path() { + let template = write_json(&minimal_template("name"), "tmpl_escape_abs"); + let escape_target = + std::env::temp_dir().join(format!("rm_batch_abs_escape_target_{}", std::process::id())); + let name_value = escape_target.to_string_lossy().to_string(); + let data = write_jsonl(&[serde_json::json!({"name": name_value})], "escape_abs"); + let out_dir = + std::env::temp_dir().join(format!("rm_batch_out_escabs_{}", std::process::id())); + std::fs::create_dir_all(&out_dir).unwrap(); + + let target_with_ext = PathBuf::from(format!("{}.png", escape_target.display())); + let _ = std::fs::remove_dir_all(&target_with_ext); + + let err = cmd_batch( + &template, + &data, + &out_dir, + "{name}.png", + None, + None, + Some("png-seq".to_string()), + false, + 1, + true, + ) + .expect_err("an absolute output name must be rejected in preflight"); + + assert!( + err.to_string().to_lowercase().contains("absolute"), + "error must call out the absolute path: {err}" + ); + assert!( + !target_with_ext.exists(), + "absolute-path target must not have been created: {}", + target_with_ext.display() + ); + + let _ = std::fs::remove_file(&template); + let _ = std::fs::remove_file(&data); + let _ = std::fs::remove_dir_all(&out_dir); + } + + /// The traversal fix must not regress the documented `{field}/{field}` + /// nested-subdirectory feature (`field_placeholder_resolved` unit test): + /// only `..`/absolute components are rejected, plain relative + /// subdirectories still get created under `--output-dir`. + #[test] + fn batch_relative_subdirectory_name_still_creates_nested_output() { + let template = write_json(&minimal_template("lang"), "tmpl_subdir"); + let data = write_jsonl(&[serde_json::json!({"lang": "en"})], "subdir"); + let out_dir = + std::env::temp_dir().join(format!("rm_batch_out_subdir_{}", std::process::id())); + std::fs::create_dir_all(&out_dir).unwrap(); + + cmd_batch( + &template, + &data, + &out_dir, + "{lang}/{index}.png", + None, + None, + Some("png-seq".to_string()), + false, + 1, + true, + ) + .expect("a relative subdirectory name must still be allowed"); + + let subdir = out_dir.join("en").join("0.png"); + assert!( + subdir.exists() && subdir.is_dir(), + "expected nested output dir en/0.png to exist" + ); + + let _ = std::fs::remove_file(&template); + let _ = std::fs::remove_file(&data); + let _ = std::fs::remove_dir_all(&out_dir); + } + + /// Constat #3: `batch` must run the same schema + geometry validation + /// `render` runs before it renders a single-file scenario — CLAUDE.md's + /// "schema + geometry, les deux doivent passer" rule applies just as + /// much to a batch row. Reproduces the brief's scenario: a 320×180 + /// template with 96px `white-space: nowrap` text and a long `$title` + /// override overflows the viewport; `render` blocks on it, so `batch` + /// must too, before any render starts. + #[test] + fn batch_preflight_rejects_a_geometry_overflow() { + let template = write_json( + &serde_json::json!({ + "config": { "title": { "type": "string", "default": "x" } }, + "video": { "width": 320, "height": 180, "fps": 1 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "text", + "content": "$title", + "style": { "font-size": 96, "white-space": "nowrap" } + }] + }] + }), + "tmpl_overflow", + ); + let data = write_jsonl( + &[serde_json::json!({"title": "A ridiculously long overflowing headline"})], + "overflow", + ); + let out_dir = + std::env::temp_dir().join(format!("rm_batch_out_overflow_{}", std::process::id())); + std::fs::create_dir_all(&out_dir).unwrap(); + + let err = cmd_batch( + &template, + &data, + &out_dir, + "{index}.png", + None, + None, + Some("png-seq".to_string()), + false, + 1, + true, + ) + .expect_err("a scenario that overflows the viewport must fail preflight, like `render`"); + + assert!( + err.to_string().contains("geometry violation"), + "error must surface the geometry violation, matching `render`'s message: {err}" + ); + assert!( + !out_dir.join("0.png").exists(), + "preflight failure must mean no render started" + ); + + let _ = std::fs::remove_file(&template); + let _ = std::fs::remove_file(&data); + let _ = std::fs::remove_dir_all(&out_dir); + } + + /// Guard that temporarily restricts `PATH` to a minimal, `ffmpeg`-free + /// set of directories, so `render`'s `Command::new("ffmpeg").arg( + /// "-version")` availability probe fails and the built-in openh264 + /// fallback encoder runs instead — the codepath the audit's own repro + /// deliberately forced with `env -i PATH=/usr/bin:/bin ...` to make the + /// `YUVBuffer` even-dimension assertion panic reachable without a real + /// `ffmpeg` dependency in CI. Restores the original `PATH` on drop. + struct NoFfmpegPathGuard { + original: Option, + _permit: std::sync::MutexGuard<'static, ()>, + } + + /// Serializes every test in this file that mutates `PATH` — required + /// because `std::env::set_var`/`remove_var` are `unsafe`: the standard + /// library only guarantees soundness when nothing else in the process + /// reads or writes the environment concurrently. This is the only test + /// file in `rustmotion-cli` whose tests spawn a real (non `png-seq` / + /// `gif` / `raw`) video encode — every other test in this crate's test + /// binary never reads `PATH` — so this lock only needs to protect + /// against concurrent runs of tests within this file. + static PATH_MUTATION_LOCK: Mutex<()> = Mutex::new(()); + + impl NoFfmpegPathGuard { + fn install() -> Self { + let permit = PATH_MUTATION_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let original = std::env::var_os("PATH"); + // SAFETY: `_permit` holds `PATH_MUTATION_LOCK` for this guard's + // entire lifetime (released on `Drop`, after `PATH` is restored + // below), and per the lock's doc comment no other thread in this + // binary reads/writes `PATH` while it is held. + unsafe { std::env::set_var("PATH", "/usr/bin:/bin") }; + Self { + original, + _permit: permit, + } + } + } + + impl Drop for NoFfmpegPathGuard { + fn drop(&mut self) { + // SAFETY: see `install`. + unsafe { + match &self.original { + Some(v) => std::env::set_var("PATH", v), + None => std::env::remove_var("PATH"), + } + } + } + } + + /// Constat #1: with `--jobs > 1`, a worker thread that panics (instead of + /// returning `Err`) was invisible to `cmd_batch` — `let _ = h.join()` + /// discarded the panic, so neither `success_arc` nor `failures` was ever + /// incremented for that row. A batch where *every* worker panicked + /// reported "Batch complete: 0/N succeeded." and still returned `Ok(())` + /// — exit code 0 for a batch that rendered nothing. + /// + /// The brief's own reproduction (odd frame dimensions, e.g. 321×241, + /// forcing an `openh264` `assert_eq!(width % 2, 0, ...)` panic) is no + /// longer reachable *through `batch`* once constat #3 is fixed: schema + /// validation (`video.width and video.height must be even`, wired in by + /// the constat #3 fix above) now rejects that scenario in preflight, + /// before any worker thread is even spawned — a nice side effect, but it + /// means this test needs a different panic that survives preflight. + /// `create_encoder` (crates/rustmotion/src/encode/video/h264.rs) computes + /// `let pixels = width * height;` in plain (checked) `u32` arithmetic + /// before ever touching a frame buffer; 70000×70000 is even, positive, + /// and passes every schema/geometry check, but `70000 * 70000 = + /// 4_900_000_000` overflows `u32::MAX` (4_294_967_295) and panics with + /// "attempt to multiply with overflow" — deterministically, and before + /// any expensive Skia canvas allocation happens for the (nonexistent) + /// video frame. Still requires the openh264 fallback (`create_encoder` + /// is only reached when `ffmpeg` is unavailable), hence + /// `NoFfmpegPathGuard`. + #[test] + fn batch_parallel_worker_panic_is_reported_as_failure_not_silent_success() { + let template = write_json( + &serde_json::json!({ + "video": { "width": 70000, "height": 70000, "fps": 1 }, + "scenes": [{ "duration": 1.0, "children": [] }] + }), + "tmpl_overflow_dims", + ); + let data = write_jsonl( + &[serde_json::json!({}), serde_json::json!({})], + "overflow_dims", + ); + let out_dir = + std::env::temp_dir().join(format!("rm_batch_out_overflowdims_{}", std::process::id())); + std::fs::create_dir_all(&out_dir).unwrap(); + + let _no_ffmpeg = NoFfmpegPathGuard::install(); + + // Run `cmd_batch` on a worker so a hang (e.g. a deadlock introduced + // by a broken fix) fails the test instead of wedging the whole + // suite, mirroring `paint_within` in + // crates/rustmotion-components/tests/degenerate_inputs.rs. + let (tx, rx) = std::sync::mpsc::channel(); + let t_template = template.clone(); + let t_data = data.clone(); + let t_out_dir = out_dir.clone(); + let worker = std::thread::spawn(move || { + let result = cmd_batch( + &t_template, + &t_data, + &t_out_dir, + "{index}.mp4", + None, + None, + None, // default format: the mp4/openh264 path under test + false, + 2, // jobs: exercises the parallel path constat #1 is about + true, + ); + let _ = tx.send(result); + }); + + let result = match rx.recv_timeout(std::time::Duration::from_secs(30)) { + Ok(r) => { + worker + .join() + .expect("outer worker thread must not itself panic"); + r + } + Err(_) => { + panic!("cmd_batch did not return within 30s — possible deadlock in the join/accounting fix") + } + }; + + assert!( + result.is_err(), + "a batch where every worker panicked must not report success" + ); + let msg = result.unwrap_err().to_string(); + assert!( + msg.to_lowercase().contains("panic"), + "the failure must surface the panic, not just an opaque render failure: {msg}" + ); + + let _ = std::fs::remove_file(&template); + let _ = std::fs::remove_file(&data); + let _ = std::fs::remove_dir_all(&out_dir); + } } From c73d9a9dd87d950f925044857febe3f9abf87cbe Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 8 Aug 2026 15:02:14 +0200 Subject: [PATCH 06/15] fix(html): turn the dialect's silent losses into named errors (#149) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The HTML dialect degraded four classes of input without a word, and `rustmotion validate` answered "Valid scenario" for all of them. An author who writes a construct the transpiler cannot honour has to be told. `"#); + assert!( + matches!(e, crate::HtmlError::StyleElementUnsupported), + "expected StyleElementUnsupported, got: {e:?}" + ); + } + + #[test] + fn script_element_is_skipped_not_painted() { + let v = map_first(r#"

real

"#); + let children = v["children"].as_array().expect("children array"); + assert_eq!( + children.len(), + 1, + "script content must not become a component: {v}" + ); + assert_eq!(children[0]["content"], json!("real")); + } + + #[test] + fn title_and_noscript_and_template_elements_are_skipped_not_painted() { + let v = map_first( + r#"
tt

real

"#, + ); + let children = v["children"].as_array().expect("children array"); + assert_eq!( + children.len(), + 1, + "title/noscript/template content must not become a component: {v}" + ); + assert_eq!(children[0]["content"], json!("real")); + } + + #[test] + fn tag_kind_head_is_ignored() { + // content never survives as a distinct DOM node when authored + // inline (html5ever drops the wrapper per HTML5 "in body" parsing + // rules and lets its text bleed into the parent), so this can only be + // exercised at the `tag_kind` unit level, not through the full + // element_to_value/html_to_scenario_value pipeline. + assert!(matches!(tag_kind("head"), TagKind::Ignored)); + } + + // --- unsupported native elements (constat 3) --- + + #[test] + fn img_element_is_refused_with_rm_image_suggestion() { + let e = map_first_err(r#""#); + match e { + crate::HtmlError::UnsupportedNativeElement { tag, suggestion } => { + assert_eq!(tag, "img"); + assert_eq!(suggestion, "rm-image"); + } + other => panic!("expected UnsupportedNativeElement, got: {other:?}"), + } + } + + #[test] + fn video_element_is_refused_with_rm_video_suggestion() { + let e = map_first_err(r#""#); + match e { + crate::HtmlError::UnsupportedNativeElement { tag, suggestion } => { + assert_eq!(tag, "video"); + assert_eq!(suggestion, "rm-video"); + } + other => panic!("expected UnsupportedNativeElement, got: {other:?}"), + } + } + + #[test] + fn svg_element_is_refused_with_rm_svg_suggestion() { + let e = map_first_err(r#""#); + match e { + crate::HtmlError::UnsupportedNativeElement { tag, suggestion } => { + assert_eq!(tag, "svg"); + assert_eq!(suggestion, "rm-svg"); + } + other => panic!("expected UnsupportedNativeElement, got: {other:?}"), + } + } + + // --- boolean attributes on custom elements (constat 4) --- + + #[test] + fn custom_element_bool_attribute_true_and_false() { + let v = map_first(r#""#); + assert_eq!(v["auto_scroll"], json!(false)); + assert_eq!(v["diff"], json!(true)); + } + + #[test] + fn custom_element_bare_attribute_becomes_true() { + let v = map_first(r#""#); + assert_eq!( + v["diff"], + json!(true), + "bare boolean attribute must become true, not be dropped: {v}" + ); + } } diff --git a/crates/rustmotion-html/src/lib.rs b/crates/rustmotion-html/src/lib.rs index dcfd7eb..092bd1d 100644 --- a/crates/rustmotion-html/src/lib.rs +++ b/crates/rustmotion-html/src/lib.rs @@ -69,6 +69,34 @@ pub enum HtmlError { /// Emitted when `` sets both `path`/`src` and `source` — they are mutually exclusive. #[error(": 'path'/'src' and 'source' are mutually exclusive")] FontPathAndSourceConflict { family: String }, + /// Emitted for `

Styled by class

"##; + let err = html_to_scenario_value(html).expect_err("

Hi

"##; + let err = + html_to_scenario_value(html).expect_err("root-level