From 2dda81c9bcc957922ab5384c42c6686f8d6294b9 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 7 Oct 2026 14:13:45 +0700 Subject: [PATCH 1/8] CLI: a reader that closes stdout early is not a crash (#167) The CLI printed with println!/eprintln!, which panic when the stream is gone, so `exec --list | head` (or any output larger than the pipe buffer) ended in a broken-pipe panic with exit status 101. Output now goes through say!/note!, which never panic. Once stdout fails, later output is dropped but the command still finishes its work (a `run ... --save` sequence isn't cut short); a closed pipe is quiet and exits 0, any other stdout error is reported and makes the exit status 1. The MCP server ends quietly when its client closes stdout. Test: a_closed_stdout_is_not_a_crash (info --json, exec --list, a saving run sequence and mcp with stdout closed before they write). Co-Authored-By: Claude Opus 5.5 --- apps/effectcraft-cli/src/main.rs | 133 ++++++++++++++++++++++-------- apps/effectcraft-cli/tests/cli.rs | 26 ++++++ 2 files changed, 123 insertions(+), 36 deletions(-) diff --git a/apps/effectcraft-cli/src/main.rs b/apps/effectcraft-cli/src/main.rs index 753f0626..540a5463 100644 --- a/apps/effectcraft-cli/src/main.rs +++ b/apps/effectcraft-cli/src/main.rs @@ -36,10 +36,54 @@ #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] +use std::io::Write; + use effectcraft_automation::tools::{self, Reply}; use effectcraft_automation::{Backend, McpServer}; use serde_json::{Value, json}; +/// `println!` that never panics: see [`write_line`]. +macro_rules! say { + ($($t:tt)*) => { + write_line(format_args!($($t)*)) + }; +} + +/// `eprintln!` that never panics: a diagnostic that can't be written is dropped. +macro_rules! note { + ($($t:tt)*) => {{ + let _ = writeln!(std::io::stderr(), $($t)*); + }}; +} + +/// Why stdout stopped taking output, once it has. +static STDOUT_FAILED: std::sync::OnceLock = std::sync::OnceLock::new(); + +/// Write a line to stdout. Once that fails, later output is dropped but the command still +/// finishes its work (a `run … --save` sequence isn't cut short). A reader that closes stdout +/// early (`info --json | head`) has read what it wanted; any other failure is reported and makes +/// the exit status 1 ([`stdout_broken`]). +fn write_line(text: std::fmt::Arguments) { + if STDOUT_FAILED.get().is_some() { + return; + } + let r = { + let mut out = std::io::stdout().lock(); + out.write_fmt(text).and_then(|()| out.write_all(b"\n")) + }; + if let Err(e) = r { + let _ = STDOUT_FAILED.set(e.kind()); + if e.kind() != std::io::ErrorKind::BrokenPipe { + note!("effectcraft-cli: cannot write to stdout: {e}"); + } + } +} + +/// Whether stdout failed for a reason other than its reader closing it early. +fn stdout_broken() -> bool { + STDOUT_FAILED.get().is_some_and(|k| *k != std::io::ErrorKind::BrokenPipe) +} + const USAGE: &str = "usage: effectcraft-cli [args] [--json] info project + engine summary commands [--filter TEXT] [--enabled] list engine commands @@ -173,11 +217,11 @@ fn value_arg(s: &str) -> Value { fn main() { let raw: Vec = std::env::args().skip(1).collect(); if matches!(raw.first().map(String::as_str), Some("--version" | "-V" | "version")) { - println!("effectcraft-cli {}", env!("CARGO_PKG_VERSION")); + say!("effectcraft-cli {}", env!("CARGO_PKG_VERSION")); return; } if raw.is_empty() || raw.iter().any(|a| a == "-h" || a == "--help") { - println!("{USAGE}"); + say!("{USAGE}"); std::process::exit(if raw.is_empty() { 2 } else { 0 }); } let mut args = match Args::parse(raw) { @@ -190,20 +234,21 @@ fn main() { } let cmd = args.pos.remove(0); match run(&cmd, &args, json_out) { + Ok(()) if stdout_broken() => std::process::exit(1), Ok(()) => {} Err(Failure::Usage(e)) => fail_usage(&e), Err(Failure::Error(e)) => { if json_out { - println!("{}", json!({"error": e})); + say!("{}", json!({"error": e})); } - eprintln!("effectcraft-cli {cmd}: {e}"); + note!("effectcraft-cli {cmd}: {e}"); std::process::exit(1); } } } fn fail_usage(e: &str) -> ! { - eprintln!("effectcraft-cli: {e}\n{USAGE}"); + note!("effectcraft-cli: {e}\n{USAGE}"); std::process::exit(2) } @@ -292,13 +337,13 @@ fn script_cmd(args: &Args, json_out: bool) -> Result<(), Failure> { emit(&o, true); } else { if let Some(out) = r["output"].as_str().filter(|o| !o.is_empty()) { - println!("{out}"); + say!("{out}"); } if !r["result"].is_null() && r["error"].is_null() { - println!("{}", r["result"]); + say!("{}", r["result"]); } if let Some(p) = saved { - eprintln!("saved {p}"); + note!("saved {p}"); } } if let Some(e) = r.get("error").filter(|e| !e.is_null()) { @@ -319,9 +364,9 @@ fn script_cmd(args: &Args, json_out: bool) -> Result<(), Failure> { /// Print a result: compact JSON with `--json`, else pretty JSON. fn emit(v: &Value, json_out: bool) { if json_out { - println!("{v}"); + say!("{v}"); } else { - println!("{}", serde_json::to_string_pretty(v).unwrap_or_default()); + say!("{}", serde_json::to_string_pretty(v).unwrap_or_default()); } } @@ -344,19 +389,19 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { if json_out { emit(&info, true); } else { - println!( + say!( "EffectCraft {} ({}), {} commands, {} effects", info["version"].as_str().unwrap_or(""), info["mode"].as_str().unwrap_or(""), info["commands"], info["effects"] ); - println!("project: {}", p["path"].as_str().unwrap_or("(unsaved)")); + say!("project: {}", p["path"].as_str().unwrap_or("(unsaved)")); for i in p["items"].as_array().into_iter().flatten() { - println!(" item {:>3} {:<12} {}", i["id"], i["type"].as_str().unwrap_or(""), i["name"].as_str().unwrap_or("")); + say!(" item {:>3} {:<12} {}", i["id"], i["type"].as_str().unwrap_or(""), i["name"].as_str().unwrap_or("")); } if let Some(c) = comp { - println!( + say!( "active comp {} \"{}\" {}x{} @ {} fps, {} s", c["id"], c["name"].as_str().unwrap_or(""), @@ -366,7 +411,7 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { c["duration"] ); for l in c["layers"].as_array().into_iter().flatten() { - println!(" #{:<3} id {:<4} {:<10} {}", l["index"], l["id"], l["type"].as_str().unwrap_or(""), l["name"].as_str().unwrap_or("")); + say!(" #{:<3} id {:<4} {:<10} {}", l["index"], l["id"], l["type"].as_str().unwrap_or(""), l["name"].as_str().unwrap_or("")); } } } @@ -422,7 +467,7 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { if json_out { emit(&v, true); } else { - println!("layer {} \"{}\" ({}) at {} s", v["id"], v["name"].as_str().unwrap_or(""), v["type"].as_str().unwrap_or(""), v["time"]); + say!("layer {} \"{}\" ({}) at {} s", v["id"], v["name"].as_str().unwrap_or(""), v["type"].as_str().unwrap_or(""), v["time"]); for p in v["properties"].as_array().into_iter().flatten() { let mut extra = String::new(); if let Some(k) = p.get("keys") { @@ -431,7 +476,7 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { if let Some(e) = p.get("expression").and_then(Value::as_str) { extra += &format!(" expr: {e}"); } - println!(" {:<44} {:<8} {}{extra}", p["path"].as_str().unwrap_or(""), p["type"].as_str().unwrap_or(""), p["value"]); + say!(" {:<44} {:<8} {}{extra}", p["path"].as_str().unwrap_or(""), p["type"].as_str().unwrap_or(""), p["value"]); } } } @@ -481,12 +526,16 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { if json_out { emit(&info, true); } else { - eprintln!("rendered {}x{} at {:.3}s in {ms:.1} ms -> {out}", f.width, f.height, f.time); + note!("rendered {}x{} at {:.3}s in {ms:.1} ms -> {out}", f.width, f.height, f.time); } } "mcp" => { let b = backend(args, false)?; - McpServer::new(b).serve_stdio().map_err(|e| e.to_string())?; + // A client that closes its end of stdout has ended the session. + match McpServer::new(b).serve_stdio() { + Err(e) if e.kind() != std::io::ErrorKind::BrokenPipe => return Err(e.to_string().into()), + _ => {} + } } other => return usage_err(&format!("unknown subcommand `{other}`")), } @@ -577,7 +626,7 @@ fn render(args: &Args, json_out: bool) -> Result<(), Failure> { let _ = s.execute("renderQueue.setRender", json!({"index": k + 1, "render": false})); } let r = s.execute("renderQueue.add", p).map_err(err)?; - eprintln!( + note!( "{} → {} ({}×{}, {} frames, {})", r["compName"].as_str().unwrap_or("?"), r["outputPath"].as_str().unwrap_or("?"), @@ -593,7 +642,8 @@ fn render(args: &Args, json_out: bool) -> Result<(), Failure> { std::thread::sleep(std::time::Duration::from_millis(100)); if tty && let Some(p) = s.render_progress() { let left = p.remaining.map(|r| format!(", ~{r:.1}s left")).unwrap_or_default(); - eprint!( + let _ = write!( + std::io::stderr(), " [{}/{}] frame {}/{} {:.1}s{left}\x1b[K", (p.items_done + 1).min(p.items_total), @@ -607,7 +657,7 @@ fn render(args: &Args, json_out: bool) -> Result<(), Failure> { } s.poll_render(); if tty { - eprintln!(); + note!(); } let mut results = vec![]; let mut failed = vec![]; @@ -615,7 +665,7 @@ fn render(args: &Args, json_out: bool) -> Result<(), Failure> { let name = s.project.item(it.comp).map(|i| i.name.clone()).unwrap_or_else(|| "?".into()); match &it.status { RenderStatus::Done => { - eprintln!("done: {name} → {} in {:.2}s", it.last_output.as_deref().unwrap_or("?"), it.render_time.unwrap_or(0.0)); + note!("done: {name} → {} in {:.2}s", it.last_output.as_deref().unwrap_or("?"), it.render_time.unwrap_or(0.0)); results.push(json!({"comp": name, "output": it.last_output, "seconds": it.render_time})); } RenderStatus::Failed(e) => failed.push(format!("{name}: {e}")), @@ -640,7 +690,7 @@ fn list_commands_cmd(args: &Args, json_out: bool) -> Result<(), Failure> { emit(&v, true); } else { for c in v.as_array().into_iter().flatten() { - println!( + say!( "{:<36} {:<36} {:<16} {}", c["id"].as_str().unwrap_or(""), c["label"].as_str().unwrap_or(""), @@ -708,7 +758,7 @@ fn bench_ops(args: &Args) -> Result<(), Failure> { m.extend(effectcraft_ui_egui::bench::ui_ops(|| { let mut s = effectcraft_host::session(); if let Err(e) = s.execute("file.open", json!({"path": path})) { - eprintln!("bench --ops: {e}"); + note!("bench --ops: {e}"); } s })); @@ -716,9 +766,9 @@ fn bench_ops(args: &Args) -> Result<(), Failure> { if args.flag("--json") { emit(&json!({"spec": format!("{spec:?}"), "measures": m.iter().map(perf::Measure::json).collect::>()}), true); } else { - println!("bench --ops: {} comps, {} layers in Main, {} footage items", spec.comps, spec.main_layers, spec.footage); + say!("bench --ops: {} comps, {} layers in Main, {} footage items", spec.comps, spec.main_layers, spec.footage); for x in &m { - println!(" {:<22} {:>10.2} ms {}", x.name, x.ms, x.note); + say!(" {:<22} {:>10.2} ms {}", x.name, x.ms, x.note); } } Ok(()) @@ -788,7 +838,7 @@ fn bench(s: &Session, cid: ItemId, t: Tick, opts: RenderOpts, n: usize) { } } } - eprintln!( + note!( "bench (no cache): {n} runs at scale {}: wall min {:.2} ms, median {:.2} ms; CPU (all threads) min {:.2} ms", opts.scale, min(&totals), @@ -796,9 +846,9 @@ fn bench(s: &Session, cid: ItemId, t: Tick, opts: RenderOpts, n: usize) { min(&cpus) ); for (d, name, p, c, fx) in per_layer { - eprintln!(" {:indent$}{name:<28} process {:8.2} ms composite {:8.2} ms (min)", "", min(&p), min(&c), indent = d * 2); + note!(" {:indent$}{name:<28} process {:8.2} ms composite {:8.2} ms (min)", "", min(&p), min(&c), indent = d * 2); for (id, v) in fx { - eprintln!(" {:indent$} fx {id:<30} {:8.2} ms", "", min(&v), indent = d * 2); + note!(" {:indent$} fx {id:<30} {:8.2} ms", "", min(&v), indent = d * 2); } } } @@ -819,7 +869,7 @@ fn bench_play(s: &Session, cid: ItemId, t: Tick, opts: RenderOpts, n: usize) { let mut v: Vec = runs.iter().map(|r| r.0).collect(); let mean = v.iter().sum::() / n as f64; let cpu = runs.iter().map(|r| r.1).sum::() / n as f64; - eprintln!( + note!( "bench-play ({label}): {n} frames at scale {}: wall mean {mean:.2} ms, median {:.2} ms, min {:.2} ms; CPU mean {cpu:.2} ms", opts.scale, median(&mut v), @@ -827,7 +877,7 @@ fn bench_play(s: &Session, cid: ItemId, t: Tick, opts: RenderOpts, n: usize) { ); } let st = cache.stats(); - eprintln!(" layer cache: {} hits, {} misses, {} entries, {:.1} MB", st.hits, st.misses, st.entries, st.bytes as f64 / 1e6); + note!(" layer cache: {} hits, {} misses, {} entries, {:.1} MB", st.hits, st.misses, st.entries, st.bytes as f64 / 1e6); } /// Median wall time (ms) of `n` runs of `f` after one warm-up run. @@ -1008,10 +1058,21 @@ fn bench_gpu(s: &Session, args: &Args) -> Result<(), Failure> { } }; let project = &project; - eprintln!("GPU: {} — median of {n} runs (ms/frame)", effectcraft_render::Accelerator::name(&gpu)); - eprintln!( + note!("GPU: {} — median of {n} runs (ms/frame)", effectcraft_render::Accelerator::name(&gpu)); + note!( "{:<28} {:>5} {:>10} {:>9} {:>9} {:>9} {:>9} {:>9} {:>8} {:>9} {:>9} {:>11}", - "comp", "res", "size", "cpu cold", "gpu cold", "cpu warm", "gpu warm", "gpu view", "speedup", "auto", "gpu≠cpu", "up/dn MB" + "comp", + "res", + "size", + "cpu cold", + "gpu cold", + "cpu warm", + "gpu warm", + "gpu view", + "speedup", + "auto", + "gpu≠cpu", + "up/dn MB" ); for cid in comps { let Some(comp) = project.comp(cid) else { continue }; @@ -1077,7 +1138,7 @@ fn bench_gpu(s: &Session, args: &Args) -> Result<(), Failure> { .count(); let pct = 100.0 * off as f64 / a.data.len().max(1) as f64; let speedup = cpu_warm / gpu_warm.max(1e-9); - eprintln!( + note!( "{name:<28} {label:>5} {size:>10} {cpu_cold:>9.2} {gpu_cold:>9.2} {cpu_warm:>9.2} {gpu_warm:>9.2} {view:>9.2} {speedup:>7.2}x {auto:>9.2} {pct:>8.3}% {traffic:>11}" ); } diff --git a/apps/effectcraft-cli/tests/cli.rs b/apps/effectcraft-cli/tests/cli.rs index 6ec33992..1247ee22 100644 --- a/apps/effectcraft-cli/tests/cli.rs +++ b/apps/effectcraft-cli/tests/cli.rs @@ -130,6 +130,32 @@ fn errors_are_json() { assert_eq!(out.status.code(), Some(2)); } +/// A reader that closes stdout before the CLI writes (`| head`) is not a crash: the command +/// still does its work and exits 0, without a panic (#167). +#[test] +fn a_closed_stdout_is_not_a_crash() { + let proj = tmp("closed-stdout.ecproj"); + let p = proj.to_str().unwrap(); + for args in [&["info", "--json"][..], &["exec", "--list"], &["run", "comp.new", r#"{"name":"Piped"}"#, "project.summary", "--empty", "--save-as", p]] { + let mut child = bin().args(args).stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::piped()).spawn().unwrap(); + drop(child.stdout.take()); + let out = child.wait_with_output().unwrap(); + let err = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(0), "{args:?}: {err}"); + assert!(!err.contains("panicked"), "{args:?}: {err}"); + } + assert!(std::fs::read_to_string(&proj).unwrap().contains("Piped"), "the sequence ran to the end and saved"); + // The MCP server ends quietly when its client closes stdout. + let mut child = bin().arg("mcp").stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::piped()).spawn().unwrap(); + drop(child.stdout.take()); + { + let mut stdin = child.stdin.take().unwrap(); + let _ = writeln!(stdin, "{}", json!({"jsonrpc": "2.0", "id": 1, "method": "ping"})); + } + let out = child.wait_with_output().unwrap(); + assert_eq!(out.status.code(), Some(0), "{}", String::from_utf8_lossy(&out.stderr)); +} + #[test] fn mcp_over_stdio() { let mut child = bin().args(["mcp", "--demo"]).stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::null()).spawn().unwrap(); From 50f26e6a4794e8dbdebb8aec456d0caa73a87f5f Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 7 Oct 2026 14:22:34 +0700 Subject: [PATCH 2/8] CLI: unknown options are usage errors (#168) Args::parse kept any unrecognised `--option` as a valueless flag nobody read, so `exec comp.new --bogus` ran with defaults and exit 0, and a misspelt valued option left its value behind as a positional: `--saveas z.ecproj` opened z.ecproj as the project and failed with an unrelated "cannot read" error. Options are now checked against the declared lists (VALUED and the new FLAGS) before anything runs; unknown ones are a usage error naming them (exit 2), and `--flag=value` on a flag says it takes no value. render-frame's `--transparent` (the MCP render_frame option) was one of the silently ignored options; it now keeps the frame's alpha. Tests: unknown_options_are_refused, every_option_read_is_declared (every option the code reads is declared, so none is refused), unknown_options_are_usage_errors (exit 2, nothing runs or renders, and --transparent renders alpha). Co-Authored-By: Claude Opus 5.5 --- apps/effectcraft-cli/src/main.rs | 73 +++++++++++++++++++++++++++++-- apps/effectcraft-cli/tests/cli.rs | 28 ++++++++++++ docs/agents.md | 3 +- 3 files changed, 99 insertions(+), 5 deletions(-) diff --git a/apps/effectcraft-cli/src/main.rs b/apps/effectcraft-cli/src/main.rs index 540a5463..ec3b0b50 100644 --- a/apps/effectcraft-cli/src/main.rs +++ b/apps/effectcraft-cli/src/main.rs @@ -9,7 +9,7 @@ //! effectcraft-cli props [--flat] [--time S] a layer's property tree (with paths) //! effectcraft-cli get [--time S] read a property //! effectcraft-cli set [--time S] [--expression E] -//! effectcraft-cli render-frame [--comp C] [--time S|--frame N] [--max-side PX|--scale K] [--out F.png] +//! effectcraft-cli render-frame [--comp C] [--time S|--frame N] [--max-side PX|--scale K] [--out F.png] [--transparent] //! effectcraft-cli render [--comp C] --out FILE [--format h264|hevc|av1|prores|webm|png|jpeg|tiff|exr|gif|wav|aiff] [--start S] [--end S] //! [--work-area] [--fps N] [--resolution full|half|third|quarter|K] [--quality best|draft] [--channels rgb|rgba] //! [--jpeg-quality N] [--bitrate KBPS] [--prores proxy|lt|standard|hq|4444|4444xq] [--audio auto|on|off] @@ -92,7 +92,7 @@ const USAGE: &str = "usage: effectcraft-cli [--flat] [--time S] a layer's property tree with paths get [--time S] read a property set [--time S] [--expression E] - render-frame [--comp C] [--time S | --frame N] [--max-side PX | --scale K] [--out F.png] + render-frame [--comp C] [--time S | --frame N] [--max-side PX | --scale K] [--out F.png] [--transparent] render [--comp C] --out FILE [--format F] [--start S] [--end S] [--work-area] [--fps N] [--resolution full|half|third|quarter|K] [--quality best|draft] [--channels rgb|rgba] [--jpeg-quality N] [--bitrate KBPS] [--prores PROFILE] [--audio auto|on|off] @@ -156,6 +156,25 @@ const VALUED: &[&str] = &[ "--opus-app", ]; +/// Options without a value. Any other `--option` is a usage error. +const FLAGS: &[&str] = &[ + "--json", + "--save", + "--demo", + "--empty", + "--gpu", + "--list", + "--schemas", + "--enabled", + "--flat", + "--transparent", + "--work-area", + "--queue", + "--ops", + "--small", + "--adv3d", +]; + struct Args { pos: Vec, opts: Vec<(String, Option)>, @@ -165,7 +184,7 @@ struct Args { impl Args { fn parse(raw: Vec) -> Result { - let (mut pos, mut opts) = (vec![], vec![]); + let (mut pos, mut opts, mut unknown) = (vec![], vec![], vec![]); let mut it = raw.into_iter(); while let Some(a) = it.next() { if a.starts_with("--") && a.len() > 2 { @@ -176,6 +195,10 @@ impl Args { if VALUED.contains(&k.as_str()) { let v = inline.or_else(|| it.next()).ok_or_else(|| format!("{k} needs a value"))?; opts.push((k, Some(v))); + } else if !FLAGS.contains(&k.as_str()) { + unknown.push(format!("`{k}`")); + } else if inline.is_some() { + return Err(format!("{k} takes no value")); } else { opts.push((k, None)); } @@ -183,6 +206,12 @@ impl Args { pos.push(a); } } + // Before anything runs: a misspelt option must not run the command with defaults, or + // leave its value behind as a positional (`--saveas x.ecproj` opening x.ecproj). + if !unknown.is_empty() { + let s = if unknown.len() == 1 { "" } else { "s" }; + return Err(format!("unknown option{s} {}", unknown.join(", "))); + } let mut a = Args { pos, opts, project: None }; a.project = match a.opt("--project") { Some(p) => Some(p.to_string()), @@ -519,7 +548,7 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { } let out = args.opt("--out").unwrap_or("frame.png").to_string(); let t0 = std::time::Instant::now(); - let f = b.render(comp.as_ref(), time, max_side)?; + let f = b.render_with(comp.as_ref(), time, max_side, args.flag("--transparent"))?; let ms = t0.elapsed().as_secs_f64() * 1000.0; std::fs::write(&out, &f.png).map_err(|e| format!("cannot write {out}: {e}"))?; let info = json!({"path": out, "comp": f.comp, "time": f.time, "width": f.width, "height": f.height, "ms": (ms * 10.0).round() / 10.0}); @@ -1145,3 +1174,39 @@ fn bench_gpu(s: &Session, args: &Args) -> Result<(), Failure> { } Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + fn parse(args: &[&str]) -> Result { + Args::parse(args.iter().map(|a| a.to_string()).collect()) + } + + /// Unknown options are usage errors naming them, not ignored (#168). + #[test] + fn unknown_options_are_refused() { + let err = |args: &[&str]| parse(args).err().unwrap(); + assert_eq!(err(&["exec", "comp.new", "--bogus", "--empty", "--saveas", "z.ecproj"]), "unknown options `--bogus`, `--saveas`"); + assert_eq!(err(&["render", "--bogus=1"]), "unknown option `--bogus`"); + assert_eq!(err(&["info", "--json=yes"]), "--json takes no value"); + let a = parse(&["render", "t.ecproj", "--comp=Main", "--work-area", "--out", "x.mp4", "--json"]).unwrap(); + assert_eq!((a.project.as_deref(), a.opt("--comp"), a.opt("--out")), (Some("t.ecproj"), Some("Main"), Some("x.mp4"))); + assert!(a.flag("--work-area") && a.flag("--json")); + } + + /// Every `--option` the CLI reads is declared in VALUED or FLAGS, so none is refused. + #[test] + fn every_option_read_is_declared() { + let code = include_str!("main.rs").split("#[cfg(test)]").next().unwrap(); + for rest in code.split("\"--").skip(1) { + let name: String = rest.chars().take_while(|c| c.is_ascii_alphanumeric() || *c == '-').collect(); + let opt = format!("--{name}"); + // `--help` and `--version` are handled before parsing. + if name.is_empty() || !rest[name.len()..].starts_with('"') || ["--help", "--version"].contains(&opt.as_str()) { + continue; + } + assert!(VALUED.contains(&opt.as_str()) || FLAGS.contains(&opt.as_str()), "{opt} is read but not declared"); + } + } +} diff --git a/apps/effectcraft-cli/tests/cli.rs b/apps/effectcraft-cli/tests/cli.rs index 1247ee22..6ac475c7 100644 --- a/apps/effectcraft-cli/tests/cli.rs +++ b/apps/effectcraft-cli/tests/cli.rs @@ -130,6 +130,34 @@ fn errors_are_json() { assert_eq!(out.status.code(), Some(2)); } +/// An unknown option is a usage error (exit 2) naming it, before anything runs: the command +/// doesn't run with defaults, and a misspelt option's value isn't opened as the project (#168). +#[test] +fn unknown_options_are_usage_errors() { + let png = tmp("bogus.png"); + let o = png.to_str().unwrap(); + for (args, named) in [ + (&["exec", "comp.new", "--bogus", "--empty", "--json"][..], "`--bogus`"), + (&["render-frame", "--bogusflag", "--out", o, "--json"], "`--bogusflag`"), + (&["exec", "comp.new", "--empty", "--saveas", "z.ecproj", "--json"], "`--saveas`"), + ] { + let out = bin().args(args).output().unwrap(); + let err = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(2), "{args:?}: {err}"); + assert!(err.contains(&format!("unknown option {named}")), "{args:?}: {err}"); + assert!(out.stdout.is_empty(), "{args:?}: nothing ran"); + } + assert!(!png.exists(), "render-frame didn't render"); + // `--transparent` (silently ignored before) keeps the frame's alpha. + let proj = tmp("transparent.ecproj"); + let p = proj.to_str().unwrap(); + ok_json(&["run", "comp.new", r#"{"name":"T","width":32,"height":32}"#, "layer.newSolid", r#"{"width":8,"height":8}"#, "--empty", "--save-as", p]); + for (flag, alpha) in [(None, 255), (Some("--transparent"), 0)] { + ok_json(&[&["render-frame", p, "--out", o][..], flag.as_slice()].concat()); + assert_eq!(image::open(&png).unwrap().to_rgba8().get_pixel(0, 0)[3], alpha, "{flag:?}"); + } +} + /// A reader that closes stdout before the CLI writes (`| head`) is not a crash: the command /// still does its work and exits 0, without a panic (#167). #[test] diff --git a/docs/agents.md b/docs/agents.md index eb9ebbba..50e7d981 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -430,7 +430,8 @@ that closes the dialog). File ▸ Scripts: `file.scripts.list`, `file.runScript Each invocation runs a headless engine with no window. It opens the demo project unless you pass `--project F.ecproj`, a positional `*.ecproj` or `--empty`. Add `--json` for one compact JSON document -on stdout. Errors print `{"error": ...}` and exit with status 1; usage errors exit with status 2. +on stdout. Errors print `{"error": ...}` and exit with status 1; usage errors, such as an unknown +option, exit with status 2 before anything runs. ```sh effectcraft-cli info --json From 3aaa84047173df13be9a6d5d17a46aed43f1e18c Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 7 Oct 2026 14:31:37 +0700 Subject: [PATCH 3/8] Scripting: script.run reports .jsxbin scripts as unsupported (#176) `effectcraft-cli script FILE` passes the file's text to script.run, which ran a compiled .jsxbin script as JavaScript and failed with "SyntaxError: unexpected '@'" at 1:1, while file.runScript already says .jsxbin isn't supported. script.run now refuses a script named `.jsxbin` or whose text starts with the `@JSXBIN@` header, with file.runScript's message (one shared constant). The check is in the command, so the CLI with or without --bridge, MCP run_script and the Script Console all report it. Tests: engine_entry_points (by header and by name), script_file_and_eval (the CLI exits 1 with the message and no SyntaxError). Co-Authored-By: Claude Opus 5.5 --- apps/effectcraft-cli/tests/cli.rs | 7 +++++++ crates/engine/src/commands/file_more.rs | 18 +++++++++++++++--- crates/script/src/tests.rs | 6 ++++++ 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/apps/effectcraft-cli/tests/cli.rs b/apps/effectcraft-cli/tests/cli.rs index 6ac475c7..c2827617 100644 --- a/apps/effectcraft-cli/tests/cli.rs +++ b/apps/effectcraft-cli/tests/cli.rs @@ -239,4 +239,11 @@ fn script_file_and_eval() { let (code, v) = run_json(&["script", "--eval", "\nnope()"]); assert_eq!(code, 1); assert_eq!(v["error"]["line"], json!(2)); + // A compiled .jsxbin script is reported as unsupported, not as a SyntaxError (#176). + let bin_script = tmp("compiled.jsxbin"); + std::fs::write(&bin_script, "@JSXBIN@ES@2.0@MyBbyBn0ABJAnAEjzFjBjMjFjSjUBfRBFeFjIjFjMjMjPff0DzACByB\n").unwrap(); + let out = bin().args(["script", bin_script.to_str().unwrap()]).output().unwrap(); + assert_eq!(out.status.code(), Some(1)); + let err = String::from_utf8_lossy(&out.stderr); + assert!(err.contains(".jsxbin scripts are not supported: run the .jsx source") && !err.contains("SyntaxError"), "{err}"); } diff --git a/crates/engine/src/commands/file_more.rs b/crates/engine/src/commands/file_more.rs index ac636076..5b051265 100644 --- a/crates/engine/src/commands/file_more.rs +++ b/crates/engine/src/commands/file_more.rs @@ -438,8 +438,8 @@ fn run_script(s: &mut Session, p: &Value) -> Result { && p.get("steps").is_none() && is_js_script(path) { - if path.to_ascii_lowercase().ends_with(".jsxbin") { - return Err(bad("file.runScript", "binary .jsxbin scripts are not supported: run the .jsx source")); + if is_jsxbin(path) { + return Err(bad("file.runScript", JSXBIN)); } let bytes = s.services.read_file(path).map_err(|e| EngineError::Other(format!("cannot read {path}: {e}")))?; let code = String::from_utf8_lossy(&bytes).to_string(); @@ -487,7 +487,14 @@ fn run_script(s: &mut Session, p: &Value) -> Result { /// command scripts. fn is_js_script(path: &str) -> bool { let l = path.to_ascii_lowercase(); - l.ends_with(".jsx") || l.ends_with(".js") || l.ends_with(".jsxbin") + l.ends_with(".jsx") || l.ends_with(".js") || is_jsxbin(path) +} + +/// Compiled `.jsxbin` scripts are an undocumented binary encoding, not JavaScript. +const JSXBIN: &str = "binary .jsxbin scripts are not supported: run the .jsx source"; + +fn is_jsxbin(name: &str) -> bool { + name.to_ascii_lowercase().ends_with(".jsxbin") } /// A script error becomes a toast and a command error. @@ -519,6 +526,11 @@ fn run_js(s: &mut Session, code: &str, name: &str, console: bool) -> Result Result { let code = str_p(p, "code").ok_or_else(|| bad("script.run", "missing `code` (JavaScript)"))?.to_string(); let name = str_p(p, "name").unwrap_or("script").to_string(); + // A compiled script (named `.jsxbin`, or its text starts with the format's header) would only + // fail to parse at 1:1. + if is_jsxbin(&name) || code.trim_start_matches('\u{feff}').trim_start().starts_with("@JSXBIN@") { + return Err(bad("script.run", JSXBIN)); + } run_js(s, &code, &name, super::b_p(p, "console").unwrap_or(false)) } diff --git a/crates/script/src/tests.rs b/crates/script/src/tests.rs index 8fd6362d..10deb79b 100644 --- a/crates/script/src/tests.rs +++ b/crates/script/src/tests.rs @@ -376,6 +376,12 @@ fn engine_entry_points() { // Scripts can't run scripts. let r = s.execute("script.run", json!({"code": "app.run('script.run', {code: '1'})"})).unwrap(); assert!(r["error"]["message"].as_str().unwrap().contains("scripts can't run scripts"), "{r}"); + // Compiled .jsxbin scripts (by name or by their header) are unsupported, not a SyntaxError + // at 1:1 (#176). + for p in [json!({"code": "@JSXBIN@ES@2.0@MyBbyBn0AB", "name": "compiled.bin"}), json!({"code": "1", "name": "/x/compiled.JSXBIN"})] { + let e = s.execute("script.run", p.clone()).unwrap_err().to_string(); + assert!(e.contains(".jsxbin scripts are not supported: run the .jsx source"), "{p}: {e}"); + } // File ▸ Scripts ▸ Run Script File… runs .jsx as JavaScript. let dir = std::env::temp_dir().join(format!("ec-script-file-{}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); From 842148916799eb0663d528f8a3a11dd346ae45d7 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 7 Oct 2026 14:42:10 +0700 Subject: [PATCH 4/8] Expressions: prop.get reports a syntax error that disabled the expression (#163) prop.setExpression keeps an expression with a syntax error but disables it, and says why only in its own reply. MCP set_property answers with prop.get instead, and prop.get reported `evaluated` / `expressionError` only for enabled expressions, so set_property, get_property and prop.get all showed the broken expression as if it were installed. prop.get now reports a disabled expression too: `evaluated` is the static value that renders, and `expressionError` is its syntax error. Test: expression_syntax_errors_reach_the_reply (set_property and get_property carry the SyntaxError; runtime errors and valid expressions report as before). Co-Authored-By: Claude Opus 5.5 --- crates/automation/src/tests.rs | 22 ++++++++++++++++++++++ crates/engine/src/commands/prop.rs | 10 ++++++++-- docs/agents.md | 3 ++- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/crates/automation/src/tests.rs b/crates/automation/src/tests.rs index dd236dad..8a9ba805 100644 --- a/crates/automation/src/tests.rs +++ b/crates/automation/src/tests.rs @@ -177,6 +177,28 @@ fn headless_workflow() { let _ = std::fs::remove_dir_all(dir); } +/// An expression with a syntax error is kept but disabled; set_property and get_property report +/// its error, and `evaluated` is the static value that renders (#163). +#[test] +fn expression_syntax_errors_reach_the_reply() { + let mut s = McpServer::new(Backend::headless(effectcraft_host::session())); + call_json(&mut s, "execute_command", json!({"command": "comp.new", "params": {"name": "A", "width": 64, "height": 64}})); + call_json(&mut s, "execute_command", json!({"command": "layer.newSolid", "params": {"name": "S", "color": "#ff0000", "width": 32, "height": 8}})); + let at = |expr: &str| json!({"layer": "S", "path": "transform/rotation", "expression": expr}); + call_json(&mut s, "set_property", json!({"layer": "S", "path": "transform/rotation", "value": 30})); + let set = call_json(&mut s, "set_property", at("thisIsBroken(")); + let got = call_json(&mut s, "get_property", json!({"layer": "S", "path": "transform/rotation"})); + for r in [&set, &got] { + assert_eq!((&r["expression"], &r["evaluated"]), (&json!("thisIsBroken("), &json!(30.0)), "{r}"); + assert!(r["expressionError"].as_str().unwrap().contains("SyntaxError"), "{r}"); + } + // A runtime error and a valid expression report as before. + let r = call_json(&mut s, "set_property", at("nope*2")); + assert!(r["expressionError"].as_str().unwrap().contains("nope is not defined"), "{r}"); + let r = call_json(&mut s, "set_property", at("60")); + assert_eq!((&r["evaluated"], r.get("expressionError")), (&json!(60.0), None), "{r}"); +} + #[test] fn stdio_loop() { let mut s = server(); diff --git a/crates/engine/src/commands/prop.rs b/crates/engine/src/commands/prop.rs index bff27e77..409210e8 100644 --- a/crates/engine/src/commands/prop.rs +++ b/crates/engine/src/commands/prop.rs @@ -118,8 +118,14 @@ fn get(s: &mut Session, p: &Value) -> Result { "keys": keys, "expression": pr.expr.as_ref().map(|e| e.text.clone()), }); // With an expression, `value` is the keyframed (pre-expression) value; also report what the - // expression makes of it (what renders), or why it fails. - if pr.has_expression() + // expression makes of it (what renders), or why it fails. A disabled one renders `value`; one + // disabled by its syntax error (prop.setExpression) reports that error. + if let Some(e) = pr.expr.as_ref().filter(|e| !e.enabled && !e.text.trim().is_empty()) { + out["evaluated"] = raw.to_json(); + if let Some(err) = s.expr_check.and_then(|check| check(&e.text).err()) { + out["expressionError"] = json!(err); + } + } else if pr.has_expression() && let (Some(h), Some(comp)) = (s.expr.as_deref(), s.project.comp(cid)) { let mut ctx = crate::render::EvalCtx::new(&s.project, cid, comp, t); diff --git a/docs/agents.md b/docs/agents.md index 50e7d981..376475e0 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -136,7 +136,8 @@ build real projects through these interfaces; they are worked examples of everyt * **One call, one undo step**: `batch {"steps":[{"command":"layer.newShape","params":{"kind":"ellipse","name":"Ring"}},{"command":"layer.addShapeItem","params":{"layer":"$1.layer","kind":"trim"}},{"command":"prop.addKey","params":{"layer":"$1.layer","path":"transform/opacity","time":0,"value":0}}]}` (a string param that is exactly `"$N"` or `"$N.key"` is replaced; `$2.path` is the trim's path). * **Expressions**: `set_property`/`get_property` replies carry `evaluated` (what renders) next to - `value` (the keyframed value) and `expressionError` when the expression fails. + `value` (the keyframed value) and `expressionError` when the expression fails. An expression with + a syntax error is kept but disabled, so it renders `value` and reports the syntax error. * **Keyframes like a person**: `keys.select {keys, toggle: true}` (Shift+click), `keys.selectEqual` / `selectPrevious` / `selectFollowing`, `keys.move {delta, merge}` (steps sharing a merge key are one drag: each applies to the keys as they were before it, so a key passed over survives) and From d416d9a01f1d3dac20298c962da38b3f777375e1 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 7 Oct 2026 14:48:45 +0700 Subject: [PATCH 5/8] MCP: add_effect is one undo step and leaves nothing applied when it fails (#175) add_effect ran effect.apply and then one prop.set per `values` entry as separate commands: each became its own undo step, and a bad value (an unknown parameter id) returned an error but left the effect, and any values set before it, on the layer, so a corrected retry landed at effects/#2. It now runs them as one engine.batch, named like effect.apply's own undo step ("Apply Gaussian Blur"): one undo removes the effect with its values, and a failing step rolls the whole call back. The reply is unchanged. Test: add_effect_is_one_undo_step_and_all_or_nothing. Co-Authored-By: Claude Opus 5.5 --- crates/automation/src/tests.rs | 26 ++++++++++++++++++++++++++ crates/automation/src/tools.rs | 15 +++++++++++---- docs/agents.md | 2 +- 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/crates/automation/src/tests.rs b/crates/automation/src/tests.rs index 8a9ba805..1364d920 100644 --- a/crates/automation/src/tests.rs +++ b/crates/automation/src/tests.rs @@ -416,6 +416,32 @@ fn high_level_tools_add_effect_and_report_state() { assert!(err && c[0]["text"].as_str().unwrap().contains("unknown effect"), "{c:?}"); } +/// add_effect is one undo step, and a call that fails leaves the layer and the history as they +/// were (#175). +#[test] +fn add_effect_is_one_undo_step_and_all_or_nothing() { + let mut s = server(); + call_json(&mut s, "execute_command", json!({"command": "comp.new", "params": {"name": "A", "width": 64, "height": 64}})); + call_json(&mut s, "execute_command", json!({"command": "layer.newSolid", "params": {"name": "S", "color": "#ff0000"}})); + let undo = |s: &mut McpServer| call_json(s, "get_project", json!({}))["undo"].as_array().unwrap().clone(); + let effects = |s: &mut McpServer| { + let l = call_json(s, "get_layer", json!({"layer": "S", "flat": true})); + l["properties"].as_array().unwrap().iter().filter(|p| p["path"].as_str().unwrap().starts_with("effects/")).count() + }; + let before = undo(&mut s); + let (c, err) = call(&mut s, "add_effect", json!({"layer": "S", "effect": "Gaussian Blur", "values": {"blurriness": 8, "repeatEdgePixels": true}})); + assert!(err && c[0]["text"].as_str().unwrap().contains("effects/#1/repeatEdgePixels"), "{c:?}"); + assert_eq!((undo(&mut s), effects(&mut s)), (before.clone(), 0), "nothing applied"); + // The corrected retry lands at #1, in one undo step named like effect.apply's. + let r = call_json(&mut s, "add_effect", json!({"layer": "S", "effect": "Gaussian Blur", "values": {"blurriness": 8, "repeatEdge": true}})); + assert_eq!((&r["path"], &r["set"]), (&json!("effects/#1"), &json!(["effects/#1/blurriness", "effects/#1/repeatEdge"])), "{r}"); + let after = undo(&mut s); + assert_eq!(after.len(), before.len() + 1, "{after:?}"); + assert_eq!(after.last(), Some(&json!("Apply Gaussian Blur"))); + call_json(&mut s, "undo", json!({})); + assert_eq!((undo(&mut s), effects(&mut s)), (before, 0), "one undo removes the effect and its values"); +} + #[test] fn list_fonts_lists_families_with_styles() { let mut s = server(); diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index 8a1ae2be..87315767 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -154,7 +154,8 @@ fn list_fonts(b: &mut Backend, a: &Value) -> Result { json_reply(b.exec("text.fonts", obj(&[("query", get(a, "query")), ("rescan", get(a, "rescan"))]))?) } -/// `effect.apply` on one layer, then set parameters on the new instance by param id. +/// `effect.apply` on one layer, then set parameters on the new instance by param id: one batch, +/// so one undo step, and a failing value leaves nothing applied. fn add_effect(b: &mut Backend, a: &Value) -> Result { let (layer, effect, comp) = (need(a, "layer")?, need(a, "effect")?, get(a, "comp")); let count = |t: &Value| effects_group(t).and_then(|g| g.get("children")).and_then(Value::as_array).map_or(0, Vec::len); @@ -162,16 +163,22 @@ fn add_effect(b: &mut Backend, a: &Value) -> Result { let index = count(&before) + 1; let mut p = obj(&[("effect", Some(effect)), ("comp", comp)]); p["layers"] = json!([layer]); - b.exec("effect.apply", p)?; + let mut steps = vec![json!({"command": "effect.apply", "params": p})]; let prefix = format!("effects/#{index}"); let mut set = vec![]; if let Some(vals) = get(a, "values").and_then(Value::as_object) { for (k, v) in vals { let path = format!("{prefix}/{k}"); - b.exec("prop.set", obj(&[("layer", Some(layer)), ("comp", comp), ("path", Some(&json!(path))), ("value", Some(v))]))?; + steps.push( + json!({"command": "prop.set", "params": obj(&[("layer", Some(layer)), ("comp", comp), ("path", Some(&json!(path))), ("value", Some(v))])}), + ); set.push(path); } } + // The undo step is named like effect.apply's own ("Apply Gaussian Blur"). + let name = effect.as_str().map(|e| effectcraft_engine::effects::lookup(e).map_or(e, |s| s.name)); + let label = name.map(|n| json!(format!("Apply {n}"))); + b.exec("engine.batch", obj(&[("steps", Some(&json!(steps))), ("label", label.as_ref())]))?; let tree = b.exec("layer.tree", obj(&[("layer", Some(layer)), ("comp", comp)]))?; let inst = effects_group(&tree).and_then(|g| g.get("children")).and_then(Value::as_array).and_then(|c| c.get(index - 1)).cloned(); let mut params = vec![]; @@ -590,7 +597,7 @@ static TOOLS: &[ToolDef] = &[ }, ToolDef { name: "add_effect", - description: "Apply an effect to a layer and optionally set its parameters in one undoable call. `effect` is an id (`ec.blur.gaussian`) or After Effects name (`Gaussian Blur`; see list_effects); `values` maps parameter ids to values, e.g. {\"blurriness\": 12}. Returns the instance `path` (`effects/#n`, for set_property / add_keyframe) and its parameters with their paths and current values.", + description: "Apply an effect to a layer and optionally set its parameters in one undoable call. `effect` is an id (`ec.blur.gaussian`) or After Effects name (`Gaussian Blur`; see list_effects); `values` maps parameter ids to values, e.g. {\"blurriness\": 12}. One undo step; if a value fails, nothing is applied. Returns the instance `path` (`effects/#n`, for set_property / add_keyframe) and its parameters with their paths and current values.", bridge_only: false, schema: || { schema( diff --git a/docs/agents.md b/docs/agents.md index 376475e0..b4ea4763 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -59,7 +59,7 @@ The server speaks JSON-RPC 2.0 over stdio, one message per line, and supports MC | `add_keyframe {layer, path, time+value \| keys:[...], interpolation?, comp?}` | Adds keys, then optionally applies linear/bezier/hold/easyEase. | | `list_effects {filter?}` | Effect ids, names, categories, GPU / 32-bpc support and parameters. | | `list_fonts {query?, rescan?}` | Font families text layers can use, bundled and installed, with their styles, origin and own-language name; `rescan` picks up fonts installed since launch. | -| `add_effect {layer, effect, values?, comp?}` | Apply an effect and set its parameters in one call; returns the instance path (`effects/#n`) and its parameter paths. | +| `add_effect {layer, effect, values?, comp?}` | Apply an effect and set its parameters in one call and one undo step (a failing value leaves nothing applied); returns the instance path (`effects/#n`) and its parameter paths. | | `render_frame {comp?, time?, max_side?, path?, inline?, transparent?}` | Returns a PNG image of a frame; `transparent: true` keeps the alpha (as an RGB + Alpha render writes it) instead of compositing over the comp background. | | `open_project {path \| demo \| new}` / `save_project {path?}` | Open and save files. | | `undo {steps?}` / `redo {steps?}` | History. | From 5d2083b64ccd1be66fe7e0a47055e6546576a26e Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 7 Oct 2026 15:01:56 +0700 Subject: [PATCH 6/8] Comp: comp.new / comp.settings no longer advertise an `advanced3D` key (#177) Their params docs wrote the renderer as `renderer? classic3D|advanced3D`. Without a `:`, the doc parser reads `|` as "or another key", so `advanced3D` became an accepted key in the schemas and the unknown-key check: `comp.new {"advanced3D": true}` succeeded and made a Classic 3D comp. The docs now use the value-list form the other commands use, `renderer?: classic3D|advanced3D`. The key is removed rather than implemented: it would only duplicate `renderer` (and could contradict it). A scan of every command's schema found no other key that came from a value list. Test: renderer_values_are_not_parameter_keys (not in the schema, rejected as unknown, `renderer` still works on both commands). Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/commands/comp.rs | 4 ++-- crates/engine/src/tests.rs | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/commands/comp.rs b/crates/engine/src/commands/comp.rs index 8114cb73..a3267fa1 100644 --- a/crates/engine/src/commands/comp.rs +++ b/crates/engine/src/commands/comp.rs @@ -268,7 +268,7 @@ pub fn specs() -> Vec { "New Composition...", ["Composition"], Some("Cmd+N"), - "{name?, width?, height?, frameRate?, duration? (s), startTime? (s) | startTimecode?, background? [r,g,b]|#hex, pixelAspect?, shutterAngle?, shutterPhase?, motionBlurSamples?, adaptiveSampleLimit? (16–256), preserveFrameRate?: bool, preserveResolution?: bool, renderer? classic3D|advanced3D, anchor?, open?}", + "{name?, width?, height?, frameRate?, duration? (s), startTime? (s) | startTimecode?, background? [r,g,b]|#hex, pixelAspect?, shutterAngle?, shutterPhase?, motionBlurSamples?, adaptiveSampleLimit? (16–256), preserveFrameRate?: bool, preserveResolution?: bool, renderer?: classic3D|advanced3D, anchor?, open?}", always, new_comp ), @@ -277,7 +277,7 @@ pub fn specs() -> Vec { "Composition Settings...", ["Composition"], Some("Cmd+K"), - "{comp?, name?, width?, height?, anchor? 0-8 (resize anchor, 4 = center), frameRate?, duration?, startTime? (s) | startTimecode?, background?, shutterAngle?, shutterPhase?, motionBlurSamples?, adaptiveSampleLimit? (16–256), preserveFrameRate?: bool (nested or in the render queue it shows only its own frames), preserveResolution?: bool (nested, it renders at full size), pixelAspect?, renderer? classic3D|advanced3D}", + "{comp?, name?, width?, height?, anchor? 0-8 (resize anchor, 4 = center), frameRate?, duration?, startTime? (s) | startTimecode?, background?, shutterAngle?, shutterPhase?, motionBlurSamples?, adaptiveSampleLimit? (16–256), preserveFrameRate?: bool (nested or in the render queue it shows only its own frames), preserveResolution?: bool (nested, it renders at full size), pixelAspect?, renderer?: classic3D|advanced3D}", has_comp, settings ), diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 93e67a4a..f505b2a5 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -371,6 +371,27 @@ fn comp_settings_anchor_start_timecode_renderer() { assert_eq!(c.renderer, crate::project::Renderer::Advanced3D); } +/// `advanced3D` is a value of `renderer`, not a key: comp.new / comp.settings don't advertise it +/// in their schemas and reject it as unknown instead of ignoring it (#177). +#[test] +fn renderer_values_are_not_parameter_keys() { + let mut s = Session::default(); + for (id, p) in [("comp.new", json!({"name": "A"})), ("comp.settings", json!({}))] { + let spec = crate::command_specs().iter().find(|c| c.id == id).unwrap(); + let keys = crate::commands::accepted_params(spec.params).unwrap(); + assert!(keys.contains(&"renderer".to_string()) && !keys.contains(&"advanced3D".to_string()), "{id}: {keys:?}"); + let mut bad = p.clone(); + bad["advanced3D"] = json!(true); + let e = s.execute_checked(id, bad).unwrap_err().to_string(); + assert!(e.contains("unknown parameter(s) `advanced3D`"), "{id}: {e}"); + let mut good = p; + good["renderer"] = json!("advanced3D"); + s.execute_checked(id, good).unwrap(); + assert_eq!(s.active_comp().unwrap().renderer, crate::project::Renderer::Advanced3D, "{id}"); + s.execute("comp.renderer", json!({"renderer": "classic3d"})).unwrap(); + } +} + #[test] fn comp_rejects_zero_and_negative_frame_rates() { let mut s = Session::default(); From 247187872d85e93c39b773e2b405e3f2680a0a1f Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 7 Oct 2026 15:31:31 +0700 Subject: [PATCH 7/8] Render Queue: Output To an .mp4 file keeps an HEVC or AV1 module (#179) apply_output switched the format whenever the Output To extension mapped to a different format, and `.mp4` maps to H.264 only, so renderQueue.setOutput with an .mp4 path (even the item's own default name) turned an HEVC or AV1 module into H.264 and the render wrote an H.264 stream. The extension now changes the format only when the current format doesn't write it (OutputFormat::extension): HEVC, AV1 and H.264 all write .mp4, so they stay; `.mov` on H.264 still picks ProRes and `.mp4` on ProRes still picks H.264. Test: output_to_keeps_a_format_that_writes_the_extension. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/commands/render_queue.rs | 3 ++- crates/engine/src/tests_codec_options.rs | 27 ++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/commands/render_queue.rs b/crates/engine/src/commands/render_queue.rs index a7841e7e..66fa0e2d 100644 --- a/crates/engine/src/commands/render_queue.rs +++ b/crates/engine/src/commands/render_queue.rs @@ -360,7 +360,8 @@ fn apply_output(om: &mut OutputModule, templates: &RenderTemplates, roi: Option< } if let Some(o) = str_p(p, "output").or(str_p(p, "path")) { om.output = o.to_string(); - if let Some(f) = OutputFormat::from_path(o).filter(|f| *f != om.format && !o.contains("[fileExtension]")) { + // Only an extension the format doesn't write changes it: `.mp4` is H.264's, HEVC's and AV1's. + if let Some(f) = OutputFormat::from_path(o).filter(|f| f.extension() != om.format.extension() && !o.contains("[fileExtension]")) { if p.get("format").is_some() { // An explicit format wins and the extension follows it, as choosing a format in // the Output Module renames Output To (#154: `--format hevc --out x.mp4`). diff --git a/crates/engine/src/tests_codec_options.rs b/crates/engine/src/tests_codec_options.rs index 757f7c43..80703063 100644 --- a/crates/engine/src/tests_codec_options.rs +++ b/crates/engine/src/tests_codec_options.rs @@ -83,6 +83,33 @@ fn an_explicit_format_wins_over_the_output_extension() { assert_eq!((om.format, om.output.as_str()), (OutputFormat::PngSequence, "/out/still.png")); } +/// Output To only changes the format when the format can't write the new extension: HEVC and AV1 +/// write `.mp4` too, so they stay (#179). Other extensions still pick a file type. +#[test] +fn output_to_keeps_a_format_that_writes_the_extension() { + let mut s = session(); + let cases = [ + ("hevc", "/out/x.mp4", OutputFormat::Hevc, "/out/x.mp4"), + ("av1", "/out/x.mp4", OutputFormat::Av1, "/out/x.mp4"), + ("h264", "/out/x.mov", OutputFormat::ProRes, "/out/x.mov"), + ("prores", "/out/x.mp4", OutputFormat::H264, "/out/x.mp4"), + ("h264", "/out/x.mp4", OutputFormat::H264, "/out/x.mp4"), + ]; + for (format, out, want, path) in cases { + let a = s.execute("renderQueue.add", json!({"format": format})).unwrap(); + let r = s.execute("renderQueue.setOutput", json!({"item": a["item"], "path": out})).unwrap(); + let om = &s.project.render_queue.last().unwrap().output; + assert_eq!((om.format, om.output.as_str()), (want, path), "{format} → {out}"); + if OutputFormat::from_name(format) == Some(want) { + assert_eq!(r["outputModuleSummary"], a["outputModuleSummary"], "{format} → {out}: the module is unchanged"); + } + } + // The module's own default name (HEVC resolves to `Main.mp4`) changes nothing either. + let a = s.execute("renderQueue.add", json!({"format": "hevc"})).unwrap(); + let r = s.execute("renderQueue.setOutput", json!({"item": a["item"], "path": a["outputPath"]})).unwrap(); + assert_eq!((&r["output"]["format"], &r["outputModuleSummary"]), (&json!("Hevc"), &a["outputModuleSummary"]), "{r}"); +} + /// A `comp` that names no composition says so, rather than "no active composition" (#155). #[test] fn an_unknown_comp_is_named_in_the_error() { From c1e13b1f667118f825a1814f2201eef2d376c769 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 7 Oct 2026 15:45:53 +0700 Subject: [PATCH 8/8] Export: AV1 WebM refuses an explicit RGB + Alpha request (#166) apply_output checked `channels: rgba` against the module before it applied `webmCodec`, and choosing AV1 then quietly dropped the channels to RGB. So `render --format webm --webm-codec av1 --channels rgba` (one renderQueue.add) succeeded and wrote an opaque AV1 stream. The codec options now apply before Channels, so the existing "has no alpha channel" error (how an unsupported channels request is already reported) refuses the combination; for WebM it names the codec: "AV1 WebM has no alpha channel (VP9 WebM has)". Choosing AV1 alone on an RGBA VP9 item still switches it to RGB, as changing the format does. Test: av1_webm_refuses_requested_alpha. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/commands/render_queue.rs | 10 ++++++++-- crates/engine/src/tests_codec_options.rs | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/commands/render_queue.rs b/crates/engine/src/commands/render_queue.rs index 66fa0e2d..8abc855c 100644 --- a/crates/engine/src/commands/render_queue.rs +++ b/crates/engine/src/commands/render_queue.rs @@ -219,6 +219,9 @@ fn apply_output(om: &mut OutputModule, templates: &RenderTemplates, roi: Option< let f = OutputFormat::from_name(f).ok_or_else(|| bad(cmd, FORMATS))?; om.set_format(f); } + // Before Channels: the WebM codec decides whether alpha can be written, so an explicit + // `channels: rgba` with `webmCodec: av1` is refused rather than dropped (#166). + any |= apply_codec_options(om, p, cmd)?; if let Some(c) = str_p(p, "channels") { om.channels = match c.to_ascii_lowercase().replace([' ', '+'], "").as_str() { "rgb" => Channels::Rgb, @@ -227,7 +230,11 @@ fn apply_output(om: &mut OutputModule, templates: &RenderTemplates, roi: Option< _ => return Err(bad(cmd, "channels: rgb|rgba|alpha")), }; if om.channels == Channels::Rgba && !om.supports_alpha() { - return Err(bad(cmd, format!("{} has no alpha channel", om.format.label()))); + let what = match om.format { + OutputFormat::WebM => format!("{} WebM has no alpha channel (VP9 WebM has)", om.webm_codec.label()), + f => format!("{} has no alpha channel", f.label()), + }; + return Err(bad(cmd, what)); } } if let Some(c) = str_p(p, "color").or(str_p(p, "alphaMode")) { @@ -237,7 +244,6 @@ fn apply_output(om: &mut OutputModule, templates: &RenderTemplates, roi: Option< _ => return Err(bad(cmd, "color: straight|premultiplied")), }; } - any |= apply_codec_options(om, p, cmd)?; if let Some(q) = f_p(p, "quality") { om.quality = q.clamp(1.0, 100.0) as u8; } diff --git a/crates/engine/src/tests_codec_options.rs b/crates/engine/src/tests_codec_options.rs index 80703063..8adabba5 100644 --- a/crates/engine/src/tests_codec_options.rs +++ b/crates/engine/src/tests_codec_options.rs @@ -110,6 +110,25 @@ fn output_to_keeps_a_format_that_writes_the_extension() { assert_eq!((&r["output"]["format"], &r["outputModuleSummary"]), (&json!("Hevc"), &a["outputModuleSummary"]), "{r}"); } +/// An explicit RGB + Alpha request with the AV1 WebM codec (which has no alpha) is refused, not +/// silently rendered opaque (#166). Without `channels`, AV1 WebM renders RGB as before. +#[test] +fn av1_webm_refuses_requested_alpha() { + let mut s = session(); + let n = s.project.render_queue.len(); + let e = s.execute("renderQueue.add", json!({"format": "webm", "webmCodec": "av1", "channels": "rgba"})).unwrap_err().to_string(); + assert!(e.contains("AV1 WebM has no alpha channel"), "{e}"); + assert_eq!(s.project.render_queue.len(), n, "nothing queued"); + let a = s.execute("renderQueue.add", json!({"format": "webm", "channels": "rgba"})).unwrap(); + let e = s.execute("renderQueue.setOutputModule", json!({"item": a["item"], "webmCodec": "av1", "channels": "rgba"})).unwrap_err().to_string(); + assert!(e.contains("AV1 WebM has no alpha channel"), "{e}"); + let om = &s.project.render_queue.last().unwrap().output; + assert_eq!((om.webm_codec, om.channels), (WebmVideoCodec::Vp9, Channels::Rgba), "unchanged"); + s.execute("renderQueue.add", json!({"format": "webm", "webmCodec": "av1"})).unwrap(); + let om = &s.project.render_queue.last().unwrap().output; + assert_eq!((om.webm_codec, om.channels), (WebmVideoCodec::Av1, Channels::Rgb)); +} + /// A `comp` that names no composition says so, rather than "no active composition" (#155). #[test] fn an_unknown_comp_is_named_in_the_error() {