From 1df68e9a5dbda28911c28e47bbdeb5ccc22df9e7 Mon Sep 17 00:00:00 2001 From: BFlat Date: Wed, 7 Oct 2026 13:21:07 +0700 Subject: [PATCH] CLI batch: recorded-action steps, --format .ext, strict --quality, refuse --out = --in (#489, #490, #491, #492) (#545) * CLI --quality refuses every value outside 1-100 (#491) --quality was parsed as a u8 and clamped, so 0 and 101-255 were silently accepted while 256 and above were rejected. Every value outside 1-100 is now a bad --quality error, checked before any file is read or written. Co-Authored-By: Claude Opus 5.5 * CLI batch: --format with a leading dot names outputs . (#490) batch built output names from the raw --format value, so --format .jpg wrote a..jpg. One leading dot is now dropped, as the save path and file.automate.batch already do. Co-Authored-By: Claude Opus 5.5 * CLI batch --actions takes recorded-action and droplet steps (#489) batch read only {"command", "params"} objects, so the [id, params] pairs a recorded action or droplet stores failed with 'missing "command"'. It now uses the engine's action parser (automate_cmds::parse_action, split out of parse_script), which takes [id, params], objects and bare ids, bare or wrapped in {"steps": ...} or a droplet; {"actions": ...} still works. Co-Authored-By: Claude Opus 5.5 * CLI batch refuses an --out folder that is the --in folder (#492) Results are saved as /., so --out naming the input folder silently replaced the originals. batch now compares the canonical folders (relative spellings, trailing separators, symbolic links) and fails before writing anything, naming the folder; the new --in-place flag opts in. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- apps/photocraft-cli/src/lib.rs | 55 ++++++----- apps/photocraft-cli/tests/cli.rs | 151 +++++++++++++++++++++++++++++ book/src/automation/cli.md | 2 +- crates/engine/src/automate_cmds.rs | 23 +++-- docs/development.md | 2 +- 5 files changed, 201 insertions(+), 32 deletions(-) diff --git a/apps/photocraft-cli/src/lib.rs b/apps/photocraft-cli/src/lib.rs index 4837e6b8..5f11e870 100644 --- a/apps/photocraft-cli/src/lib.rs +++ b/apps/photocraft-cli/src/lib.rs @@ -21,8 +21,11 @@ USAGE: photocraft-cli run ( | --new ) --cmd [--params ] [--cmd …] [--out ] [--format ] [--quality <1-100>] Open a file, run engine commands in order, save the result. Each --params applies to the preceding --cmd. Prints each command's JSON result. - photocraft-cli batch --actions --in --out [--format ] [--quality <1-100>] - Apply an action list ([{\"command\": id, \"params\": {…}}, …]) to every image in a directory. + photocraft-cli batch --actions --in --out [--format ] [--quality <1-100>] [--in-place] + Apply an action list to every image in a directory. Steps are [id, params] pairs, + {\"command\": id, \"params\": {…}} objects or bare ids, as a recorded action or droplet stores them + (a list, or wrapped in {\"actions\": …}, {\"steps\": …} or a droplet). An --out folder that is the + --in folder is refused, as the results would replace the originals; --in-place allows it. photocraft-cli droplet … [--out ] Run a droplet (File › Automate › Create Droplet) on images and folders. photocraft-cli commands [--json] [--filter ] @@ -59,7 +62,7 @@ const SUBCOMMANDS: &[Subcommand] = &[ Subcommand { name: "convert", values: &["--format", "--quality"], bare: &[], run: convert }, Subcommand { name: "info", values: &[], bare: &["--compact"], run: |a, out, _| info(a, out) }, Subcommand { name: "run", values: &["--new", "--cmd", "--params", "--out", "--format", "--quality"], bare: &[], run: run_cmds }, - Subcommand { name: "batch", values: &["--actions", "--in", "--out", "--format", "--quality"], bare: &[], run: batch }, + Subcommand { name: "batch", values: &["--actions", "--in", "--out", "--format", "--quality"], bare: &["--in-place"], run: batch }, Subcommand { name: "droplet", values: &["--out"], bare: &[], run: droplet }, Subcommand { name: "commands", values: &["--filter"], bare: &["--json"], run: |a, out, _| commands(a, out) }, Subcommand { @@ -165,8 +168,8 @@ pub fn run(args: &[String], out: &mut dyn Write, err: &mut dyn Write) -> i32 { fn export_opts(a: &Args) -> Result { let mut o = ExportOptions::default(); if let Some(q) = a.get("--quality") { - let q: u8 = q.parse().map_err(|_| format!("bad --quality `{q}`"))?; - o.encode.jpeg_quality = q.clamp(1, 100); + o.encode.jpeg_quality = + q.parse().ok().filter(|q| (1..=100).contains(q)).ok_or_else(|| format!("bad --quality `{q}`: expected a whole number from 1 to 100"))?; } Ok(o) } @@ -186,9 +189,10 @@ fn convert(a: &Args, _out: &mut dyn Write, err: &mut dyn Write) -> R { let [input, output] = a.positional.as_slice() else { return Err("convert needs ".into()); }; + let opts = export_opts(a)?; let o = files::open(Path::new(input)).map_err(|e| e.to_string())?; warn_all(err, &o.warnings); - let ws = files::save(&o.document, Path::new(output), a.get("--format"), &export_opts(a)?, None).map_err(|e| e.to_string())?; + let ws = files::save(&o.document, Path::new(output), a.get("--format"), &opts, None).map_err(|e| e.to_string())?; warn_all(err, &ws); Ok(()) } @@ -233,6 +237,7 @@ fn command_list(a: &Args) -> Result, String> { } fn run_cmds(a: &Args, out: &mut dyn Write, err: &mut dyn Write) -> R { + let opts = export_opts(a)?; let mut h = Headless::trusted_local(); match (a.positional.as_slice(), a.get("--new")) { ([file], None) => { @@ -255,29 +260,25 @@ fn run_cmds(a: &Args, out: &mut dyn Write, err: &mut dyn Write) -> R { print_json(out, &json!({"command": id, "result": r}), true)?; } if let Some(o) = a.get("--out") { - let r = h.save(None, Some(Path::new(o)), a.get("--format"), &export_opts(a)?).map_err(|e| e.to_string())?; + let r = h.save(None, Some(Path::new(o)), a.get("--format"), &opts).map_err(|e| e.to_string())?; let ws: Vec = serde_json::from_value(r["warnings"].clone()).unwrap_or_default(); warn_all(err, &ws); } Ok(()) } -/// Parse an actions file: `[{"command": id, "params": {…}}]` or -/// `{"actions": [...]}`; `"id"` is accepted for `"command"`. +/// Parse an actions file: the steps of a recorded action or a droplet (`[id, params]` pairs, +/// `{"command": id, "params": {…}}` objects or bare ids; `"id"` is accepted for `"command"`), as a +/// list or wrapped in `{"actions": […]}`, `{"steps": […]}` or a droplet (#489). pub fn parse_actions(text: &str) -> Result, String> { let v: Value = serde_json::from_str(text).map_err(|e| format!("actions JSON: {e}"))?; - let list = match &v { - Value::Array(a) => a.clone(), - Value::Object(m) => m.get("actions").and_then(Value::as_array).cloned().ok_or("actions JSON: expected an array or {\"actions\": [...]}")?, - _ => return Err("actions JSON: expected an array".into()), - }; - list.into_iter() - .enumerate() - .map(|(i, a)| { - let id = a.get("command").or_else(|| a.get("id")).and_then(Value::as_str).ok_or_else(|| format!("action {i}: missing \"command\""))?; - Ok((id.to_owned(), a.get("params").cloned().unwrap_or_else(|| json!({})))) - }) - .collect() + photocraft_engine::automate_cmds::parse_action(v.get("actions").unwrap_or(&v), "batch --actions").map_err(|e| e.to_string()) +} + +/// Whether `a` and `b` name the same existing folder, however they are spelt (relative, with a +/// trailing separator, through a symbolic link). +fn same_dir(a: &Path, b: &Path) -> bool { + matches!((std::fs::canonicalize(a), std::fs::canonicalize(b)), (Ok(a), Ok(b)) if a == b) } fn is_input(p: &Path) -> bool { @@ -292,14 +293,24 @@ fn batch(a: &Args, out: &mut dyn Write, err: &mut dyn Write) -> R { let text = std::fs::read_to_string(actions_path).map_err(|e| format!("{actions_path}: {e}"))?; let actions = parse_actions(&text)?; let opts = export_opts(a)?; + // Results are saved as `/.`, so an `--out` that is the `--in` folder would + // replace the originals (#492). + if !a.has("--in-place") && same_dir(&in_dir, &out_dir) { + return Err(format!( + "--out {} is the --in folder, so the results would replace the originals: choose another --out folder, or pass --in-place to overwrite them", + out_dir.display() + )); + } std::fs::create_dir_all(&out_dir).map_err(|e| e.to_string())?; let mut inputs: Vec = std::fs::read_dir(&in_dir).map_err(|e| format!("{}: {e}", in_dir.display()))?.flatten().map(|e| e.path()).filter(|p| is_input(p)).collect(); inputs.sort(); let (mut ok, mut failed) = (0, 0); let mut written = photocraft_engine::file_cmds::OutputClaims::default(); + // `--format .jpg` names outputs `.jpg`, as `--format jpg` does (#490). + let format = a.get("--format").map(|f| f.strip_prefix('.').unwrap_or(f)); for input in &inputs { - let ext = a.get("--format").map(str::to_owned).or_else(|| input.extension().map(|e| e.to_string_lossy().into_owned())).unwrap_or_else(|| "png".into()); + let ext = format.map(str::to_owned).or_else(|| input.extension().map(|e| e.to_string_lossy().into_owned())).unwrap_or_else(|| "png".into()); let stem = input.file_stem().map(|s| s.to_string_lossy().into_owned()).unwrap_or_default(); let target = out_dir.join(format!("{stem}.{ext}")); let target_text = target.to_string_lossy(); diff --git a/apps/photocraft-cli/tests/cli.rs b/apps/photocraft-cli/tests/cli.rs index 44b98bac..b23d534a 100644 --- a/apps/photocraft-cli/tests/cli.rs +++ b/apps/photocraft-cli/tests/cli.rs @@ -212,6 +212,157 @@ fn parse_actions_formats() { assert!(photocraft_cli::parse_actions(r#"{"actions":[{"command":"c"}]}"#).unwrap().len() == 1); assert!(photocraft_cli::parse_actions(r#"[{"params":{}}]"#).is_err()); assert!(photocraft_cli::parse_actions("7").is_err()); + // #489: the step shapes a recorded action and a droplet store, bare or wrapped. + let want = vec![("a".to_string(), json!({"x":1})), ("b".to_string(), json!({}))]; + for text in [ + r#"[["a",{"x":1}],["b"]]"#, + r#"[["a",{"x":1}],"b"]"#, + r#"{"steps":[["a",{"x":1}],["b",{}]]}"#, + r#"{"photocraftDroplet":1,"action":{"steps":[["a",{"x":1}],{"command":"b"}]}}"#, + r#"{"actions":[["a",{"x":1}],["b",{}]]}"#, + ] { + assert_eq!(photocraft_cli::parse_actions(text).unwrap(), want, "{text}"); + } + for text in ["[42]", "[[]]", "[[7,{}]]", r#"{"name":"x"}"#] { + assert!(photocraft_cli::parse_actions(text).is_err(), "{text}"); + } +} + +/// Runs `batch` on `input` with `actions` (saved as `.json`), writing to the folder `/`. +fn batch_with(d: &Path, input: &Path, name: &str, actions: &str, extra: &[&str]) -> std::process::Output { + let file = d.join(format!("{name}.json")); + std::fs::write(&file, actions).unwrap(); + bin().args(["batch", "--actions"]).arg(&file).arg("--in").arg(input).arg("--out").arg(d.join(name)).args(extra).output().unwrap() +} + +/// #489: `batch --actions` takes `[id, params]` steps and a droplet's steps, with the same results +/// as the object form; a malformed step fails before anything is written. +#[test] +fn batch_accepts_recorded_action_steps() { + let d = tmp("batch-steps"); + let input = d.join("in"); + std::fs::create_dir_all(&input).unwrap(); + write_png(&input.join("a.png"), 8, 4, 3); + write_png(&input.join("b.png"), 6, 5, 9); + let shapes = [ + ("objects", r#"[{"command":"image.adjustments.invert"}]"#), + ("pairs", r#"[["image.adjustments.invert",{}]]"#), + ("ids", r#"["image.adjustments.invert"]"#), + ("droplet", r#"{"photocraftDroplet":1,"action":{"steps":[["image.adjustments.invert",{}]]}}"#), + ]; + for (name, actions) in shapes { + let o = batch_with(&d, &input, name, actions, &[]); + assert!(o.status.success(), "{name}: {}", String::from_utf8_lossy(&o.stderr)); + assert!(String::from_utf8_lossy(&o.stdout).contains("2 succeeded, 0 failed"), "{name}"); + for f in ["a.png", "b.png"] { + assert_eq!(std::fs::read(d.join(name).join(f)).unwrap(), std::fs::read(d.join("objects").join(f)).unwrap(), "{name}/{f}"); + } + } + let o = batch_with(&d, &input, "bad", "[42]", &[]); + assert_eq!(o.status.code(), Some(1)); + assert!(!d.join("bad").exists(), "nothing written"); +} + +/// #490: a leading dot on `--format` doesn't double the dot in output names. +#[test] +fn batch_format_with_leading_dot() { + let d = tmp("batch-dot"); + let input = d.join("in"); + std::fs::create_dir_all(&input).unwrap(); + write_png(&input.join("a.png"), 8, 4, 3); + write_png(&input.join("b.png"), 6, 5, 9); + let invert = r#"[{"command":"image.adjustments.invert"}]"#; + for (name, format) in [("dot", ".jpg"), ("plain", "jpg")] { + let o = batch_with(&d, &input, name, invert, &["--format", format]); + assert!(o.status.success(), "{format}: {}", String::from_utf8_lossy(&o.stderr)); + let mut names: Vec = std::fs::read_dir(d.join(name)).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect(); + names.sort(); + assert_eq!(names, ["a.jpg", "b.jpg"], "{format}"); + assert_eq!(photocraft_codecs::detect(&std::fs::read(d.join(name).join("a.jpg")).unwrap()), Some(photocraft_codecs::Format::Jpeg)); + } +} + +/// #491: `--quality` outside 1-100 is an error for every subcommand, whatever the number's size, +/// and nothing is written. +#[test] +fn quality_outside_1_to_100_is_refused() { + let d = tmp("quality"); + let input = d.join("in"); + std::fs::create_dir_all(&input).unwrap(); + let a = input.join("a.png"); + write_png(&a, 8, 4, 3); + let invert = r#"[{"command":"image.adjustments.invert"}]"#; + for q in ["0", "101", "255", "256", "1000", "-1", "abc", "50.5", ""] { + let out = d.join("q.jpg"); + let (mut convert, mut run) = (bin(), bin()); + convert.arg("convert").arg(&a).arg(&out).args(["--quality", q]); + run.arg("run").arg(&a).args(["--cmd", "image.adjustments.invert", "--out"]).arg(&out).args(["--quality", q]); + for cmd in [&mut convert, &mut run] { + let o = cmd.output().unwrap(); + assert_eq!(o.status.code(), Some(1), "--quality {q}"); + assert!(String::from_utf8_lossy(&o.stderr).contains(&format!("bad --quality `{q}`: expected a whole number from 1 to 100")), "--quality {q}"); + assert!(!out.exists(), "--quality {q} wrote a file"); + } + let o = batch_with(&d, &input, "qb", invert, &["--format", "jpg", "--quality", q]); + assert_eq!(o.status.code(), Some(1), "batch --quality {q}"); + assert!(!d.join("qb").exists(), "batch --quality {q} wrote a folder"); + } + // The documented range still encodes, lower quality making a smaller file. + let sizes: Vec = ["1", "50", "100"] + .iter() + .map(|q| { + let out = d.join(format!("q{q}.jpg")); + ok(bin().arg("convert").arg(&a).arg(&out).args(["--quality", q])); + std::fs::read(out).unwrap().len() + }) + .collect(); + assert!(sizes[0] <= sizes[1] && sizes[1] <= sizes[2], "{sizes:?}"); +} + +/// #492: an `--out` folder that is the `--in` folder, however it is spelt, is refused before +/// anything is written, unless `--in-place` asks for it. +#[test] +fn batch_refuses_to_write_over_its_inputs() { + let d = tmp("batch-in-place"); + let input = d.join("in"); + std::fs::create_dir_all(&input).unwrap(); + write_png(&input.join("a.png"), 8, 4, 3); + let img = photocraft_codecs::Image::from_u8(8, 4, photocraft_codecs::ChannelLayout::Rgb, vec![90; 96]).unwrap(); + std::fs::write(input.join("a.tif"), photocraft_codecs::encode(&img, photocraft_codecs::Format::Tiff, &Default::default()).unwrap()).unwrap(); + let actions = d.join("actions.json"); + std::fs::write(&actions, r#"[{"command":"image.adjustments.invert"}]"#).unwrap(); + let snapshot = || { + let mut files: Vec<(PathBuf, Vec)> = + std::fs::read_dir(&input).unwrap().map(|e| e.unwrap().path()).map(|p| (p.clone(), std::fs::read(p).unwrap())).collect(); + files.sort(); + files + }; + let before = snapshot(); + let batch = |out: &Path, extra: &[&str]| { + bin().current_dir(&d).args(["batch", "--actions"]).arg(&actions).args(["--in", "in", "--out"]).arg(out).args(extra).output().unwrap() + }; + #[cfg(unix)] + std::os::unix::fs::symlink(&input, d.join("link")).unwrap(); + let link = cfg!(unix).then(|| PathBuf::from("link")); + for out in [PathBuf::from("in"), PathBuf::from("./in/"), input.clone(), input.join(".")].iter().chain(&link) { + for extra in [&[][..], &["--format", "tif"][..]] { + let o = batch(out, extra); + let err = String::from_utf8_lossy(&o.stderr); + assert_eq!(o.status.code(), Some(1), "--out {} {extra:?}: {err}", out.display()); + assert!(err.contains("is the --in folder") && err.contains("--in-place"), "{err}"); + assert_eq!(snapshot(), before, "--out {} {extra:?} changed the originals", out.display()); + } + } + // A separate folder runs as before, leaving the originals alone. + let (out, _) = ok(bin().current_dir(&d).args(["batch", "--actions"]).arg(&actions).args(["--in", "in", "--out", "out"])); + assert!(out.contains("2 succeeded, 0 failed"), "{out}"); + assert_eq!(snapshot(), before); + // `--in-place` opts in: each result replaces its original. + let o = batch(Path::new("in"), &["--in-place"]); + assert!(o.status.success(), "{}", String::from_utf8_lossy(&o.stderr)); + let after = snapshot(); + assert_eq!(after.len(), 2); + assert!(after.iter().zip(&before).all(|(a, b)| a.0 == b.0 && a.1 != b.1), "both inputs replaced"); } #[test] diff --git a/book/src/automation/cli.md b/book/src/automation/cli.md index 27d17500..819ffd26 100644 --- a/book/src/automation/cli.md +++ b/book/src/automation/cli.md @@ -21,6 +21,6 @@ photocraft-cli run input.psd \ --out output.png ``` -Paths supplied to the current CLI are ordinary operating-system paths. The CLI does not restrict them to a workspace root. Batch output creates the requested output directory, and save operations may create parent directories. Run untrusted action files only in an appropriately isolated account or environment. +Paths supplied to the current CLI are ordinary operating-system paths. The CLI does not restrict them to a workspace root. Batch output creates the requested output directory, and save operations may create parent directories. `batch` refuses an output directory that is its input directory (the results would replace the originals) unless `--in-place` is given. Run untrusted action files only in an appropriately isolated account or environment. CLI parsing reports malformed flags and JSON as errors, and a flag the subcommand doesn't take (a typo such as `--fromat`) is a usage error (exit status 2) rather than being ignored. It is not a security policy layer. Engine commands remain responsible for validating their own parameters. diff --git a/crates/engine/src/automate_cmds.rs b/crates/engine/src/automate_cmds.rs index a10a1bad..0ae4d5ee 100644 --- a/crates/engine/src/automate_cmds.rs +++ b/crates/engine/src/automate_cmds.rs @@ -50,14 +50,7 @@ pub fn parse_script(text: &str) -> Result> { let t = text.trim_start_matches('\u{feff}').trim(); if t.starts_with('[') || t.starts_with('{') { let v: Value = serde_json::from_str(t).map_err(|e| bad(cmd, format!("script JSON: {e}")))?; - let steps = match &v { - Value::Array(_) => &v, - Value::Object(o) => { - o.get("steps").or_else(|| o.get("action").and_then(|a| a.get("steps").or(Some(a)))).ok_or_else(|| bad(cmd, "script object needs \"steps\""))? - } - _ => return Err(bad(cmd, "script JSON must be an array of steps or an object")), - }; - return parse_steps(steps, cmd); + return parse_action(&v, cmd); } let mut out = Vec::new(); for (n, line) in t.lines().enumerate() { @@ -72,6 +65,20 @@ pub fn parse_script(text: &str) -> Result> { Ok(out) } +/// The steps of a JSON action: an array of steps (`[id, params]`, `{"command", "params"}` or a +/// bare id), `{"steps"|"action": …}` (a recorded action) or a droplet. `cmd` names the caller in +/// errors. +pub fn parse_action(v: &Value, cmd: &str) -> Result> { + let steps = match v { + Value::Array(_) => v, + Value::Object(o) => { + o.get("steps").or_else(|| o.get("action").and_then(|a| a.get("steps").or(Some(a)))).ok_or_else(|| bad(cmd, "an action object needs \"steps\""))? + } + _ => return Err(bad(cmd, "an action must be an array of steps or an object")), + }; + parse_steps(steps, cmd) +} + fn parse_steps(v: &Value, cmd: &str) -> Result> { let arr = v.as_array().ok_or_else(|| bad(cmd, "steps must be an array"))?; arr.iter() diff --git a/docs/development.md b/docs/development.md index ff7b2938..5865f6e8 100644 --- a/docs/development.md +++ b/docs/development.md @@ -100,7 +100,7 @@ cargo run -p photocraft-cli -- batch --actions actions.json --in photos/ --out d cargo run -p photocraft-cli -- commands --filter blur # the command registry ``` -`actions.json` is `[{"command": "", "params": {…}}, …]`, which is the same shape as a recorded action. `run` prints one JSON line per command result. +`actions.json` holds the steps of a recorded action: `[["", {…}], …]`, `[{"command": "", "params": {…}}, …]` or bare ids, as a list or wrapped in `{"actions": …}` or `{"steps": …}`; a droplet file works too. `batch` refuses an `--out` folder that is the `--in` folder, since the results would replace the originals, unless `--in-place` is given. `run` prints one JSON line per command result. ## Native format (.pcraft)