From 50f26e6a4794e8dbdebb8aec456d0caa73a87f5f Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 7 Oct 2026 14:22:34 +0700 Subject: [PATCH] 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