From 434e269691bcffd494e68190ae20c49925bf4326 Mon Sep 17 00:00:00 2001 From: Hamza Date: Wed, 28 Oct 2026 18:34:12 +0500 Subject: [PATCH] cli: the three argument bugs photocraft#7071 lists are live here too - An option took the next token whatever it was, so `--export --scale` wrote a file called `--scale` in the working directory. A token that cannot be a value is refused, and where the value is a path the message offers the shell's own way out (`./-name`). A negative number can still be a value, and `-inf`/`-nan` still reach the check that reports them. - An argument that is not UTF-8 panicked inside `std::env::args`. It is a usage error now, named, since such a path cannot reach the engine anyway. - ` --help` was read as an unknown option by every parser but `script`'s. It answers from the usage block, on stdout, and nothing else happens. Rebased onto the post-split main, where the fix had to grow one part. Upstream has since added a check that an export option sits before its `--export`, and it consumed the following token whatever it was, so `--scale --export out.png` was reported as the *order* of the options -- advice to move an option that is already where it belongs -- when what is wrong is that `--scale` has no value. That check now skips a value only where the next token can be one, and stands aside entirely when a value is missing so the value error speaks. Its own test, `run_export_options_come_before_their_export`, still passes. The CLI's tests moved to tests/it/cli.rs, so the test additions went there. --- apps/designcraft-cli/src/main.rs | 102 +++++++++++++++++++++++---- apps/designcraft-cli/tests/it/cli.rs | 71 +++++++++++++++++++ 2 files changed, 158 insertions(+), 15 deletions(-) diff --git a/apps/designcraft-cli/src/main.rs b/apps/designcraft-cli/src/main.rs index 9ce15674c..98c9b32d2 100644 --- a/apps/designcraft-cli/src/main.rs +++ b/apps/designcraft-cli/src/main.rs @@ -96,8 +96,73 @@ fn stdout_failed(e: std::io::Error) -> ! { std::process::exit(1); } +/// Options whose value is a path, and so can be written `./-name` when it starts with `-`, the +/// shell's own way out. None of them has an `--opt=value` form (the automation roots, parsed in +/// [`automation_args`], do and say so themselves). +const PATH_OPTIONS: [&str; 4] = ["--in", "--export", "--all-pages", "--save"]; + +/// An option's value, refusing the next token when it is another option: `--export --scale` used +/// to write a file called `--scale`. +fn value(a: &str, next: Option<&String>) -> Result { + match next { + None => Err(format!("{a} needs a value")), + Some(v) if !can_be_value(v) => { + // The way out is worth offering only where the value could be such a name. + let out = if PATH_OPTIONS.contains(&a) { format!("; if you did mean that name, write `./{v}`") } else { String::new() }; + Err(format!("{a} needs a value and `{v}` is another option{out}")) + } + Some(v) => Ok(v.clone()), + } +} + +/// Whether a token can be an option's value. A negative number can; anything else that starts +/// with `-` is another option. `-inf` and `-nan` parse as numbers too, so a bad `--scale` still +/// reaches the check that reports it rather than being called an option. +fn can_be_value(v: &str) -> bool { + !(v.starts_with('-') && v.len() > 1 && v.parse::().is_err()) +} + +/// The usage lines for one subcommand, for ` --help`. Each line of [`USAGE`] names its +/// subcommand, and the indented lines after it belong to it. +fn usage_for(sub: &str) -> String { + let mut out = Vec::new(); + let mut keep = false; + for line in USAGE.lines() { + let trimmed = line.trim_start(); + if let Some(rest) = trimmed.strip_prefix("usage: designcraft-cli ").or_else(|| trimmed.strip_prefix("designcraft-cli ")) { + keep = rest.split_whitespace().next() == Some(sub); + } + if keep { + out.push(line); + } + } + if out.is_empty() { USAGE.to_string() } else { out.join("\n") } +} + fn main() -> ExitCode { - let args: Vec = std::env::args().skip(1).collect(); + // `std::env::args` panics on an argument that is not UTF-8 ("called `Result::unwrap()` on an + // `Err` value"), which a Latin-1 file name on Linux or macOS is. Such a name cannot be passed + // through to the engine, whose paths are `str`, so it is a usage error here rather than a + // crash (photocraft#1108). + let mut args: Vec = Vec::new(); + for arg in std::env::args_os().skip(1) { + match arg.into_string() { + Ok(a) => args.push(a), + Err(bad) => { + eprintln!("designcraft-cli: `{}` is not valid UTF-8, so it cannot be used as an argument", bad.to_string_lossy()); + return ExitCode::FAILURE; + } + } + } + // ` --help` asks about that subcommand, which every parser but `script`'s read as an + // unknown option (photocraft#423). It goes to stdout and succeeds, as `--help` does. + if let Some(sub) = args.first().map(String::as_str) + && !matches!(sub, "--help" | "-h" | "help" | "script") + && args[1..].iter().any(|a| a == "--help" || a == "-h") + { + outln!("{}", usage_for(sub)); + return ExitCode::SUCCESS; + } match args.first().map(String::as_str) { Some("--version" | "-V" | "version") => { outln!("designcraft-cli {}", env!("CARGO_PKG_VERSION")); @@ -252,22 +317,29 @@ so drop --connect (or use --connect without the roots)" /// They apply to the exports that follow them, so one after the last export would do nothing. fn option_after_last_export(args: &[String]) -> Option<&str> { let mut unused = None; - let mut it = args.iter(); + let mut it = args.iter().peekable(); + // A value is skipped only where the next token can be one. Taking it whatever it is read + // `--scale --export out.png` as --scale applied to no export, so a missing value was + // reported as the order of the options -- and "put it before its --export" is advice for a + // list that already does that (photocraft#7071). while let Some(a) = it.next() { + let is_opt = matches!(a.as_str(), "--page" | "--scale" | "--pdf-options" | "--export" | "--all-pages" | "--in" | "--cmd"); match a.as_str() { - "--page" | "--scale" | "--pdf-options" => { - unused = unused.or(Some(a.as_str())); - it.next(); - } - "--export" | "--all-pages" => { - unused = None; - it.next(); - } - "--in" | "--cmd" => { - it.next(); - } + "--page" | "--scale" | "--pdf-options" => unused = unused.or(Some(a.as_str())), + "--export" | "--all-pages" => unused = None, _ => {} } + if is_opt { + match it.peek() { + Some(v) if can_be_value(v.as_str()) => { + it.next(); + } + // The option has no value. That is what is wrong with the line, so this check + // stands aside and lets `value()` say so: reporting the order instead advised + // moving an option that is already where it belongs (photocraft#7071). + _ => return None, + } + } } unused } @@ -328,7 +400,7 @@ fn run(args: &[String]) -> Result<(), String> { // Earlier --cmd results, for `$N.path` references. let mut results: Vec = Vec::new(); while let Some(a) = it.next() { - let mut val = || it.next().cloned().ok_or_else(|| format!("{a} needs a value")); + let mut val = || value(a, it.next()); match a.as_str() { "--in" => { let p = val()?; @@ -470,7 +542,7 @@ fn script(args: &[String]) -> Result<(), String> { let mut keep_going = false; let mut it = args.iter(); while let Some(a) = it.next() { - let mut val = || it.next().cloned().ok_or_else(|| format!("{a} needs a value")); + let mut val = || value(a, it.next()); match a.as_str() { "--in" => { let p = val()?; diff --git a/apps/designcraft-cli/tests/it/cli.rs b/apps/designcraft-cli/tests/it/cli.rs index 861cb514f..87e5b58fd 100644 --- a/apps/designcraft-cli/tests/it/cli.rs +++ b/apps/designcraft-cli/tests/it/cli.rs @@ -556,3 +556,74 @@ fn usage_lists_the_automation_root_flags() { let usage = String::from_utf8_lossy(&out.stdout); assert!(usage.contains("--automation-read-root DIR") && usage.contains("--automation-write-root DIR"), "{usage}"); } + +/// The argument bugs photocraft#7071 lists, measured here: every one of them was live in this +/// parser too. Each case names the photocraft issue it comes from. +#[test] +fn the_argument_parser_refuses_what_it_cannot_mean() { + let dir = std::env::temp_dir().join(format!("dc-cli-args-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + // photocraft#423: ` --help` was read as an unknown option. It asks about the + // subcommand, goes to stdout and succeeds. + for sub in ["run", "commands", "describe", "app", "mcp", "perf", "bench", "validate-pdf", "links"] { + let out = cli().args([sub, "--help"]).output().unwrap(); + assert!(out.status.success(), "{sub} --help: {}", String::from_utf8_lossy(&out.stderr)); + let text = String::from_utf8_lossy(&out.stdout); + assert!(text.contains(&format!("designcraft-cli {sub}")), "{sub} --help names it: {text:?}"); + assert!(!text.contains("unknown option"), "{sub} --help: {text:?}"); + } + // `script --help` keeps its own longer help, which it had already. + let out = cli().args(["script", "--help"]).output().unwrap(); + assert!(out.status.success() && String::from_utf8_lossy(&out.stdout).contains("Script text, one step per line")); + + // photocraft#6944: an option took the next token whatever it was, so `--export --scale` + // wrote a file called `--scale` in the working directory. + let out = cli().current_dir(&dir).args(["run", "--sample", "--export", "--scale"]).output().unwrap(); + assert!(!out.status.success(), "{}", String::from_utf8_lossy(&out.stdout)); + let err = String::from_utf8_lossy(&out.stderr); + assert!(err.contains("another option") && err.contains("./--scale"), "{err:?}"); + assert!(!dir.join("--scale").exists(), "nothing was written"); + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 0, "and no other file either"); + + // A name that really does start with `-` is still reachable, the way the shell reaches one. + let out = cli().current_dir(&dir).args(["run", "--sample", "--export", "./-dash.png"]).output().unwrap(); + assert!(out.status.success(), "{}", String::from_utf8_lossy(&out.stderr)); + assert!(dir.join("-dash.png").is_file(), "{:?}", std::fs::read_dir(&dir).unwrap().flatten().map(|e| e.file_name()).collect::>()); + + // A missing value is still a missing value, with the message it had. + let out = cli().args(["run", "--sample", "--export"]).output().unwrap(); + assert!(!out.status.success()); + assert!(String::from_utf8_lossy(&out.stderr).contains("--export needs a value")); + + // And a missing value in front of an export is still a missing value: the order of the + // options is beside the point until `--scale` has one, and "put it before its --export" was + // advice for a list that already does that. + let out = cli().current_dir(&dir).args(["run", "--sample", "--scale", "--export", "out.png"]).output().unwrap(); + let err = String::from_utf8_lossy(&out.stderr); + assert!(!out.status.success(), "{err:?}"); + assert!(err.contains("--scale needs a value"), "{err:?}"); + assert!(!err.contains("has no --export"), "{err:?}"); + // `./--export` is a file name, which is no way out of a missing `--scale`. + assert!(!err.contains("./"), "{err:?}"); + assert!(!dir.join("out.png").exists(), "and nothing is exported"); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// photocraft#1108: an argument that is not UTF-8 panicked in `std::env::args` ("called +/// `Result::unwrap()` on an `Err` value"). Such a name cannot reach the engine, whose paths are +/// `str`, so it is a usage error. +#[cfg(unix)] +#[test] +fn an_argument_that_is_not_utf8_is_an_error_not_a_panic() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let out = cli().arg("info").arg(OsStr::from_bytes(b"/tmp/caf\xe9.designcraft")).output().unwrap(); + let err = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(1), "an error, not a panic (101): {err:?}"); + assert!(err.contains("not valid UTF-8"), "{err:?}"); + assert!(!err.contains("panicked"), "{err:?}"); +}