From d6b796538b52a4d37c45217fbfd1965103079c8b Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:20:30 +0200 Subject: [PATCH] fix(chantier): align the branch with the tree the suite was verified on The 55 findings were merged one squashed commit at a time. Several of them landed during the first merge attempts, before a defect in the carving mechanism was found and corrected, so they carried a snapshot taken while their agent was still polishing rather than that agent's final state. The gap was real, not cosmetic: 200 lines of regression tests missing from one file, 141 from another, 40 from a third. The branch compiled and its remaining tests passed, which is exactly why this needed comparing against something rather than trusting a green run. This takes every path the audit owns from chantier/audit-2026-09-full, the single commit of the whole remediation tree on which fmt, the full test suite and clippy were run, then re-applies the constant-size chunks_exact replacement to the five files that needed it, since that branch predates the lint fix. The two files still differing from it afterwards differ only by that same lint fix, which they now carry and it did not. Verified on this branch: cargo check --workspace --all-targets, cargo test --workspace, cargo clippy --workspace --all-targets -- -D warnings, and cargo fmt --all --check. Refs #220 --- crates/rustmotion-components/src/gif.rs | 6 +- crates/rustmotion-components/src/table.rs | 4 +- crates/rustmotion-components/src/video.rs | 2 +- .../rustmotion-components/tests/audit_ws_d.rs | 40 ++++ .../rustmotion-components/tests/audit_ws_i.rs | 6 +- .../rustmotion-core/src/css/taffy_bridge.rs | 2 +- crates/rustmotion-core/src/engine/animator.rs | 2 +- .../rustmotion-core/src/engine/layout_pass.rs | 4 +- .../src/engine/renderer/assets.rs | 6 +- crates/rustmotion-core/src/expand.rs | 4 +- crates/rustmotion-core/tests/audit_ws_a.rs | 10 +- crates/rustmotion-core/tests/audit_ws_h.rs | 8 +- crates/rustmotion-core/tests/audit_ws_j.rs | 6 +- crates/rustmotion-html/tests/audit_ws_f.rs | 16 +- crates/rustmotion-studio/src/editor/audio.rs | 4 +- .../src/scenario/optimistic.rs | 4 +- .../rustmotion/src/cli/commands/geometry.rs | 175 +++++++-------- crates/rustmotion/src/encode/video/formats.rs | 11 +- crates/rustmotion/src/encode/video/tasks.rs | 17 +- crates/rustmotion/src/include.rs | 10 +- crates/rustmotion/tests/audit_ws_b.rs | 207 +++++++++++++++++- crates/rustmotion/tests/audit_ws_c.rs | 141 ++++++++++++ crates/rustmotion/tests/audit_ws_e.rs | 4 +- crates/rustmotion/tests/audit_ws_h.rs | 6 +- 24 files changed, 536 insertions(+), 159 deletions(-) diff --git a/crates/rustmotion-components/src/gif.rs b/crates/rustmotion-components/src/gif.rs index b5698604..5a0d918d 100644 --- a/crates/rustmotion-components/src/gif.rs +++ b/crates/rustmotion-components/src/gif.rs @@ -455,9 +455,9 @@ mod tests { /// own video is, so passing generous `max_w`/`max_h` here isolates the /// byte-budget check from the video-dimensions check exercised by the /// next test. 144 MiB is deliberately far short of the 65535×65535 - /// (~17 GiB) header a real crafted file could declare — large enough to - /// prove the budget check fires, small enough that running this test - /// never risks the allocation it is asserting never happens. + /// (~17 GiB) header the audit's own crafted file could declare — large + /// enough to prove the budget check fires, small enough that running + /// this test never risks the allocation it is asserting never happens. #[test] fn a_canvas_over_the_byte_budget_is_rejected_without_allocating_it() { let path = std::env::temp_dir().join(format!( diff --git a/crates/rustmotion-components/src/table.rs b/crates/rustmotion-components/src/table.rs index ab882664..06e90ff4 100644 --- a/crates/rustmotion-components/src/table.rs +++ b/crates/rustmotion-components/src/table.rs @@ -419,7 +419,9 @@ mod tests { // probe specifically for near-white text ink rather than any lit // pixel (the header/row backgrounds paint regardless of font-size). let text_ink = buf - .chunks_exact(4) + .as_chunks::<4>() + .0 + .iter() .filter(|p| p[3] > 0 && p[0] > 200 && p[1] > 200 && p[2] > 200) .count(); assert!( diff --git a/crates/rustmotion-components/src/video.rs b/crates/rustmotion-components/src/video.rs index 1500dbb5..cae7b018 100644 --- a/crates/rustmotion-components/src/video.rs +++ b/crates/rustmotion-components/src/video.rs @@ -176,7 +176,7 @@ impl Painter for Video { let img_info = ImageInfo::new( (fw as i32, fh as i32), ColorType::RGBA8888, - skia_safe::AlphaType::Premul, + skia_safe::AlphaType::Unpremul, None, ); let row_bytes = fw as usize * 4; diff --git a/crates/rustmotion-components/tests/audit_ws_d.rs b/crates/rustmotion-components/tests/audit_ws_d.rs index e884b076..7ac9dbd8 100644 --- a/crates/rustmotion-components/tests/audit_ws_d.rs +++ b/crates/rustmotion-components/tests/audit_ws_d.rs @@ -281,3 +281,43 @@ fn without_loop_video_playback_still_clamps_not_wraps() { "no loop_video: must clamp to the window's end (nearest: yellow), not wrap" ); } + +// ─── cached-frame draw path mistagged straight alpha as premultiplied ────── + +/// ffmpeg's `-pix_fmt rgba` output — what fills the video-frame cache — is +/// straight (unpremultiplied) alpha. Tagging that buffer `AlphaType::Premul` +/// makes Skia treat the RGB channels as already scaled by alpha instead of +/// scaling them itself, which brightens (here: doubles) every +/// semi-transparent pixel's channels once composited. +/// +/// A straight-alpha (200, 100, 50, 128) pixel, composited over black: +/// correctly tagged `Unpremul`, Skia premultiplies it to +/// (200×128/255, 100×128/255, 50×128/255) ≈ (100, 50, 25) before compositing +/// over black, landing there almost exactly (the `(1 - alpha) * 0` background +/// term vanishes either way). Mistagged `Premul`, Skia uses the raw channel +/// values directly as if already scaled — (200, 100, 50) — composited over +/// black with no further scaling, landing at roughly double the correct +/// result. +#[test] +fn cached_frame_straight_alpha_composites_correctly_not_doubled() { + let src = unique_src("alpha"); + let cache_key = format!("{src}:10x10"); + let straight = [200u8, 100, 50, 128]; + video_frame_cache().insert( + cache_key, + Arc::new(vec![(0.0, solid_rgba(straight, 2, 2), 2, 2)]), + ); + + let v = video(&src, ImageFit::Fill, None, None, None); + let ctx = ctx_at(0.0); + let pixels = paint_and_read(&v, &ctx, 10, 10, false); + let composited = px(&pixels, 10, 5, 5); + + let close = |actual: u8, expected: u8| (actual as i16 - expected as i16).abs() <= 4; + assert!( + close(composited[0], 100) && close(composited[1], 50) && close(composited[2], 25), + "straight-alpha (200,100,50,128) over black must composite to roughly (100,50,25), \ + got {composited:?} — a value near (200,100,50) means the buffer is still mistagged \ + as premultiplied and its channels are being used unscaled" + ); +} diff --git a/crates/rustmotion-components/tests/audit_ws_i.rs b/crates/rustmotion-components/tests/audit_ws_i.rs index 358e2c9c..46a94cbd 100644 --- a/crates/rustmotion-components/tests/audit_ws_i.rs +++ b/crates/rustmotion-components/tests/audit_ws_i.rs @@ -134,7 +134,9 @@ fn paint_card_with_child(card_json: serde_json::Value) -> PaintedScene { fn count_dominant(pixels: &[u8], dominant: usize, muted: &[usize]) -> usize { pixels - .chunks_exact(4) + .as_chunks::<4>() + .0 + .iter() .filter(|p| p[dominant] > 180 && muted.iter().all(|&m| p[m] < 80)) .count() } @@ -213,7 +215,7 @@ fn text_own_color_wins_over_cascaded_card_color_at_paint_time() { } fn any_ink_above_black_below_y(pixels: &[u8], width: i32, y_threshold: i32) -> bool { - pixels.chunks_exact(4).enumerate().any(|(i, p)| { + pixels.as_chunks::<4>().0.iter().enumerate().any(|(i, p)| { let y = i as i32 / width; y > y_threshold && (p[0] > 20 || p[1] > 20 || p[2] > 20) }) diff --git a/crates/rustmotion-core/src/css/taffy_bridge.rs b/crates/rustmotion-core/src/css/taffy_bridge.rs index e30401ce..7ee1fbce 100644 --- a/crates/rustmotion-core/src/css/taffy_bridge.rs +++ b/crates/rustmotion-core/src/css/taffy_bridge.rs @@ -246,7 +246,7 @@ pub fn to_taffy_style(css: &CssStyle, ctx: &ConversionContext) -> tf::Style { /// = padding + border`) subtracts from a leaf's `available_space` before /// handing it to the measure function — see [`crate::engine::layout_pass`], /// which uses this to bring `known_dimensions` (still border-box) into that -/// same content-box space. Percentage padding/border resolves +/// same content-box space (RM-27). Percentage padding/border resolves /// against `ctx.length.parent_size` like every other percentage in this /// module; a leaf's own known/available width is not threaded here, so a /// percentage inset on a leaf is only as accurate as that shared context. diff --git a/crates/rustmotion-core/src/engine/animator.rs b/crates/rustmotion-core/src/engine/animator.rs index be2dce5e..ba343334 100644 --- a/crates/rustmotion-core/src/engine/animator.rs +++ b/crates/rustmotion-core/src/engine/animator.rs @@ -2867,7 +2867,7 @@ mod spring_duration_tests { // at t=0.8s: spring_value_raw(0.8, 6, 120, 1) ~= 1.027616 — 2.76% // past the target, well outside any reasonable rest_threshold // (default 0.5%). An author asking this spring to "finish at 0.8s" - // got a value nowhere near rest. Reference recomputed when the + // got a value nowhere near rest. Reference recomputed for RM-09 // (issue #220): the solver's underdamped branch fed the wrong // argument to its sine term, so this captured value moved when that // was corrected. diff --git a/crates/rustmotion-core/src/engine/layout_pass.rs b/crates/rustmotion-core/src/engine/layout_pass.rs index 386c201d..e9a03c5e 100644 --- a/crates/rustmotion-core/src/engine/layout_pass.rs +++ b/crates/rustmotion-core/src/engine/layout_pass.rs @@ -72,7 +72,7 @@ impl LayoutResult { /// taffy nodes and our `BoxNode` ids + intrinsic measurers. /// /// `inset_width`/`inset_height` are this node's own resolved padding+border -///: taffy's `compute_leaf_layout` already subtracts them from +/// (RM-27): taffy's `compute_leaf_layout` already subtracts them from /// `available_space` before calling the measure function, but forwards /// `known_dimensions` — the outer border-box size — untouched, handing an /// `IntrinsicMeasure` implementor two arguments in different coordinate @@ -137,7 +137,7 @@ pub fn run_layout(root: &BoxNode, viewport: (f32, f32), ctx: &ConversionContext) /// Build one taffy node and, recursively, its subtree. /// /// `inherited_font_size` is the already-resolved (px) font-size of `node`'s -/// parent: CSS resolves `em` on every layout property against the +/// parent (RM-26): CSS resolves `em` on every layout property against the /// element's *own* computed font-size, and font-size itself inherits down /// the tree unless overridden. `to_taffy_style` and [`content_box_inset`] /// only ever see the single `ConversionContext` handed to them, so their diff --git a/crates/rustmotion-core/src/engine/renderer/assets.rs b/crates/rustmotion-core/src/engine/renderer/assets.rs index 9121f48e..97345599 100644 --- a/crates/rustmotion-core/src/engine/renderer/assets.rs +++ b/crates/rustmotion-core/src/engine/renderer/assets.rs @@ -50,7 +50,7 @@ pub fn gif_cache() -> &'static GifCacheMap { /// then never answers — or trickles one byte a minute — hangs the calling /// thread forever; ureq's 10 MB body cap bounds bytes, not time. On the icon /// path that thread can be a render worker with nobody at the keyboard to -/// notice. +/// notice (RM-41). /// /// `Config::builder()` starts from `Config::default()`, which already /// resolves a proxy from `HTTPS_PROXY`/`https_proxy`/`HTTP_PROXY`/ @@ -258,7 +258,7 @@ pub fn ffmpeg_available() -> bool { /// `rtmp://`, `concat:`, …), which turns an unfiltered `src` into an SSRF /// primitive — a scenario author can point it at /// `http://169.254.169.254/...` (the cloud metadata endpoint) or an internal -/// service, and read the exit status as a port-scan oracle. Remote +/// service, and read the exit status as a port-scan oracle (RM-42). Remote /// video was never a designed feature here — this module's own `is_remote` /// doc, a few functions below, and `rustmotion info`'s identical assumption /// both already treat every `src` as a local path — so this closes an @@ -276,7 +276,7 @@ fn reject_remote_video_src(src: &str) -> Result<()> { if looks_like_scheme { return Err(RustmotionError::Generic(format!( "video src '{src}' names a '{scheme}://' URL — rustmotion does not fetch video \ - over the network, only local file paths are accepted" + over the network, only local file paths are accepted (RM-42)" ))); } Ok(()) diff --git a/crates/rustmotion-core/src/expand.rs b/crates/rustmotion-core/src/expand.rs index ea153e98..c47860e1 100644 --- a/crates/rustmotion-core/src/expand.rs +++ b/crates/rustmotion-core/src/expand.rs @@ -157,7 +157,7 @@ use crate::variables::substitute; const MAX_EXPANSION_DEPTH: u32 = 64; /// Ceiling on the total number of nodes a single document's `for-each` -/// expansion may produce, across every level of nesting combined. +/// expansion may produce, across every level of nesting combined (RM-43). /// [`MAX_EXPANSION_DEPTH`] bounds how deep directives may nest, not how many /// nodes they produce — and nesting one `for-each` inside another's /// `template` is explicitly supported (`use_template_can_contain_a_nested_ @@ -306,7 +306,7 @@ pub fn expand_directives(value: &mut Value, file_label: &str) -> Result<()> { } /// Subtracts `n` from the shared expansion-node budget, or fails naming the -/// limit and where it was hit. See [`MAX_EXPANSION_NODES`] for why +/// limit and where it was hit (RM-43). See [`MAX_EXPANSION_NODES`] for why /// this is checked once per `for-each` directive's item count rather than /// once per final node: it is the only point in the recursion where the /// multiplicative blow-up can be caught before the work that would produce diff --git a/crates/rustmotion-core/tests/audit_ws_a.rs b/crates/rustmotion-core/tests/audit_ws_a.rs index d264fbae..7ad93749 100644 --- a/crates/rustmotion-core/tests/audit_ws_a.rs +++ b/crates/rustmotion-core/tests/audit_ws_a.rs @@ -1,5 +1,5 @@ //! Regression tests for the workstream A (animation & paint) audit findings -//! tracked in issue #220. +//! tracked in issue #220: RM-01, RM-09, RM-10, RM-37. use rustmotion_core::css::style::{ Background, BackgroundLayer, BoxShadow, Color as CssColor, CssStyle, Display, FlexDirection, @@ -72,7 +72,7 @@ fn probe(buf: &[u8], w: u32, x: usize, y: usize) -> (u8, u8, u8) { (buf[i], buf[i + 1], buf[i + 2]) } -// ---- opacity layer must not clip the node's own outset box-shadow ---- +// ---- RM-01: opacity layer must not clip the node's own outset box-shadow ---- fn card_with_shadow(opacity: Option) -> BoxNode { let css = CssStyle { @@ -134,7 +134,7 @@ fn opacity_layer_does_not_clip_own_outset_box_shadow() { ); } -// ---- underdamped spring step response must start from rest ---- +// ---- RM-09: underdamped spring step response must start from rest ---- fn spring_config(damping: f64, stiffness: f64, mass: f64) -> SpringConfig { SpringConfig { @@ -192,7 +192,7 @@ fn underdamped_spring_matches_the_analytic_closed_form() { } } -// ---- linear-gradient(180deg, ...) must put the first stop at the top ---- +// ---- RM-10: linear-gradient(180deg, ...) must put the first stop at the top ---- fn gradient_card(w: f32, h: f32, angle: f32) -> BoxNode { let css = CssStyle { @@ -243,7 +243,7 @@ fn linear_gradient_180deg_puts_the_first_stop_at_the_top() { ); } -// ---- `spring_settle_time` must not be rescanned on every `spring_value` call ---- +// ---- RM-37: `spring_settle_time` must not be rescanned on every `spring_value` call ---- #[test] fn spring_settle_time_is_memoized_not_rescanned_every_call() { diff --git a/crates/rustmotion-core/tests/audit_ws_h.rs b/crates/rustmotion-core/tests/audit_ws_h.rs index 0fd3a042..4f02d02f 100644 --- a/crates/rustmotion-core/tests/audit_ws_h.rs +++ b/crates/rustmotion-core/tests/audit_ws_h.rs @@ -1,9 +1,9 @@ //! Regression tests for the workstream H (untrusted scenario ingestion) -//! audit findings that live in `rustmotion-core`. +//! audit findings that live in `rustmotion-core`: RM-41, RM-42, RM-43. use rustmotion_core::engine::renderer::{extract_video_frame, http_agent}; -// ---- no timeout on any HTTP call — a bare `ureq::get` always uses +// ---- RM-41: no timeout on any HTTP call — a bare `ureq::get` always uses // the default, untimed agent, so a stalled host hangs a render forever ---- #[test] @@ -20,7 +20,7 @@ fn shared_http_agent_has_finite_global_and_connect_timeouts() { ); } -// ---- a scenario's `video.src` reaches `ffmpeg -i` verbatim, with no +// ---- RM-42: a scenario's `video.src` reaches `ffmpeg -i` verbatim, with no // protocol allowlist — a remote-looking src turns into an SSRF primitive ---- #[test] @@ -56,7 +56,7 @@ fn extract_video_frame_does_not_reject_a_plain_local_path() { ); } -// ---- `for-each` expansion has a depth ceiling but no node budget — +// ---- RM-43: `for-each` expansion has a depth ceiling but no node budget — // nesting is multiplicative, so a handful of small arrays nested a few // levels deep can declare a product in the millions ---- diff --git a/crates/rustmotion-core/tests/audit_ws_j.rs b/crates/rustmotion-core/tests/audit_ws_j.rs index 6b3501b9..4da80594 100644 --- a/crates/rustmotion-core/tests/audit_ws_j.rs +++ b/crates/rustmotion-core/tests/audit_ws_j.rs @@ -1,5 +1,5 @@ //! Regression tests for the workstream J (layout pass & CSS unit resolution) -//! audit findings on unit resolution and intrinsic measurement. +//! audit findings: RM-26, RM-27. use std::sync::{Arc, Mutex}; @@ -9,7 +9,7 @@ use rustmotion_core::css::units::{Length, LengthPercentage as CLP}; use rustmotion_core::engine::box_tree::{AvailableSpace, BoxNode, IntrinsicMeasure}; use rustmotion_core::engine::layout_pass::run_layout; -// ---- `em` on layout properties must resolve against the element's +// ---- RM-26: `em` on layout properties must resolve against the element's // own (inherited) font-size, not a constant 16px ---- /// A card sized by its parent's explicit `font-size` (48px) inherits that @@ -48,7 +48,7 @@ fn em_padding_resolves_against_inherited_font_size_not_a_constant_16px() { assert_eq!(content_w, 200.0 - 2.0 * 48.0); } -// ---- a leaf's `IntrinsicMeasure::measure` must receive `known` and +// ---- RM-27: a leaf's `IntrinsicMeasure::measure` must receive `known` and // `available` in the same (content-box) coordinate space ---- type RecordedCall = ((Option, Option), (AvailableSpace, AvailableSpace)); diff --git a/crates/rustmotion-html/tests/audit_ws_f.rs b/crates/rustmotion-html/tests/audit_ws_f.rs index e8ab981a..00c40637 100644 --- a/crates/rustmotion-html/tests/audit_ws_f.rs +++ b/crates/rustmotion-html/tests/audit_ws_f.rs @@ -1,17 +1,17 @@ //! Regression tests for workstream F's 4 confirmed audit findings on the //! HTML dialect transpiler: an attribute outside a hardcoded allowlist -//! vanishing with no error, a CSS shorthand transpiling to a string -//! the core length parser cannot read and silently resolving to 0px, +//! vanishing with no error (RM-03), a CSS shorthand transpiling to a string +//! the core length parser cannot read and silently resolving to 0px (RM-04), //! `