Merge pull request #1478 from Hamza0101/fix/cli-argument-handling

cli: the three argument bugs photocraft#7071 lists are live here too

apps/designcraft-cli/src/main.rs

@@ -42,7 +42,7 @@ use designcraft_engine::Session;
 use serde_json::{Value, json};
 
 /// The usage block. Shared by `--help` (stdout, success) and an unknown command (stderr, failure).
-const USAGE: &str = "usage: designcraft-cli run [--in FILE | --sample] [--cmd ID[=JSON]]... [--page N] [--scale S] [--pdf-options JSON] [--export OUT] [--all-pages DIR]\n         (--page, --scale and --pdf-options apply to the exports that follow them)\n       designcraft-cli commands [FILTER]\n       designcraft-cli describe COMMAND\n       designcraft-cli script [FILE|-] [--in FILE | --sample] [--connect PORT] [--save OUT] [--export OUT] [--keep-going]\n       designcraft-cli app [--port PORT] COMMAND [JSON] | --method METHOD [JSON]\n       designcraft-cli mcp [--connect PORT] [--sample] [--automation-read-root DIR]... [--automation-write-root DIR]...\n         the roots confine every file the agent's commands read (open, placed images, data sources, libraries) and\n         write (save, export, package) to those folders, links followed. Each may be given more than once; a kind\n         left out grants none of its access.\n         They run the in-process server, so they conflict with --connect (a running app is not confined by them).\n       designcraft-cli perf [--pages N] [--runs N] [--strict]\n       designcraft-cli bench FILE [--runs N]\n       designcraft-cli validate-pdf FILE.pdf  # built-in PDF/X-4 checks (including visible unembedded fonts)\n       designcraft-cli thumbnail FILE.designcraft OUT.png [SIZE]  # the saved first-page preview, at most SIZE px\n       designcraft-cli links\n       designcraft-cli --version";
+const USAGE: &str = "usage: designcraft-cli run [--in FILE | --sample] [--cmd ID[=JSON]]... [--page N] [--scale S] [--pdf-options JSON] [--export OUT] [--all-pages DIR]\n         (--page, --scale and --pdf-options apply to the exports that follow them)\n       designcraft-cli commands [FILTER]\n       designcraft-cli describe COMMAND\n       designcraft-cli script [FILE|-] [--in FILE | --sample] [--connect PORT] [--save OUT] [--export OUT] [--keep-going]\n       designcraft-cli app [--port PORT] COMMAND [JSON] | --method METHOD [JSON]\n       designcraft-cli mcp [--connect PORT] [--sample] [--automation-read-root DIR]... [--automation-write-root DIR]...\n         the roots confine every file the agent's commands read (open, placed images, data sources, libraries) and\n         write (save, export, package) to those folders, links followed. Each may be given more than once; a kind\n         left out grants none of its access.\n         They run the in-process server, so they conflict with --connect (a running app is not confined by them).\n       designcraft-cli perf [--pages N] [--runs N] [--strict]\n       designcraft-cli bench FILE [--runs N]\n       designcraft-cli validate-pdf FILE.pdf [--standard x4|x1a]  # built-in PDF/X checks (including visible unembedded fonts; the standard defaults to the file's own)\n       designcraft-cli thumbnail FILE.designcraft OUT.png [SIZE]  # the saved first-page preview, at most SIZE px\n       designcraft-cli links\n       designcraft-cli --version";
 
 /// `script --help`: the script syntax, otherwise only in `describe script.run` and docs/agents.md.
 const SCRIPT_HELP: &str = r#"usage: designcraft-cli script [FILE|-] [--in FILE | --sample] [--connect PORT] [--save OUT] [--export OUT] [--keep-going]
@@ -98,8 +98,94 @@ 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<String, String> {
+    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::<f64>().is_err())
+}
+
+/// Whether `sub` is a subcommand [`USAGE`] lists. Read from the usage text rather than written
+/// out again, so a subcommand added there cannot be missing here.
+fn is_subcommand(sub: &str) -> bool {
+    !sub.starts_with('-') && USAGE.lines().any(|line| usage_subcommand(line) == Some(sub))
+}
+
+/// The subcommand a `USAGE` line introduces, if it introduces one.
+fn usage_subcommand(line: &str) -> Option<&str> {
+    let trimmed = line.trim_start();
+    trimmed
+        .strip_prefix("usage: designcraft-cli ")
+        .or_else(|| trimmed.strip_prefix("designcraft-cli "))
+        .and_then(|rest| rest.split_whitespace().next())
+}
+
+/// The usage lines for one subcommand, for `<sub> --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() {
+        if let Some(named) = usage_subcommand(line) {
+            keep = named == sub;
+        }
+        if keep {
+            out.push(line);
+        }
+    }
+    if out.is_empty() { USAGE.to_string() } else { out.join("\n") }
+}
+
 fn main() -> ExitCode {
-    let args: Vec<String> = 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<String> = 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;
+            }
+        }
+    }
+    // `<sub> --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.
+    //
+    // Only for a subcommand that exists. Without that, `designcraft-cli typo --help` printed the
+    // whole usage and *succeeded*, so a misspelled command was an error on its own and not an
+    // error with `--help` after it -- and a script checking the exit status would carry on
+    // (noted in review). An unknown one now falls through to the dispatch, which fails as it
+    // does for `typo` alone.
+    if let Some(sub) = args.first().map(String::as_str)
+        && !matches!(sub, "--help" | "-h" | "help" | "script")
+        && is_subcommand(sub)
+        && 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"));
@@ -259,7 +345,7 @@ fn mcp(args: &[String]) -> Result<(), String> {
             continue;
         }
         match a.as_str() {
-            "--connect" => connect = Some(it.next().ok_or("--connect needs a port or host:port")?),
+            "--connect" => connect = Some(value(&a, it.next().as_ref())?),
             "--sample" => sample = true,
             other => {
                 return Err(format!(
@@ -309,22 +395,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
 }
@@ -364,7 +457,28 @@ impl Export {
     }
 }
 
+/// Every option that takes a value has one, checked before anything runs. Found later, a missing
+/// value is reported only after the exports before it have been written -- and `run --export
+/// first.png --scale --export second.png` would then have overwritten an existing `first.png` on
+/// an invocation that fails (noted in review).
+fn values_present(args: &[String]) -> Result<(), String> {
+    let mut i = 0;
+    while let Some(a) = args.get(i) {
+        i += 1;
+        if !matches!(a.as_str(), "--in" | "--cmd" | "--export" | "--all-pages" | "--page" | "--scale" | "--pdf-options") {
+            continue;
+        }
+        value(a, args.get(i))?;
+        // Step over the value, so one that looks like an option is not checked as one.
+        i += 1;
+    }
+    Ok(())
+}
+
 fn run(args: &[String]) -> Result<(), String> {
+    // Before the order check and before any export: a value error is about the line as written,
+    // and reporting it must not cost a file that was already there.
+    values_present(args)?;
     if let Some(opt) = option_after_last_export(args) {
         return Err(format!(
             "{opt} has no --export or --all-pages after it; options apply to the exports that follow them, so put it before its --export"
@@ -385,7 +499,7 @@ fn run(args: &[String]) -> Result<(), String> {
     // Earlier --cmd results, for `$N.path` references.
     let mut results: Vec<Value> = 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()?;
@@ -527,7 +641,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()?;

apps/designcraft-cli/tests/it/cli.rs

@@ -692,3 +692,141 @@ fn thumbnail_replaces_another_existing_file() {
     assert_eq!(w.max(h), 128, "{w}×{h}");
     let _ = std::fs::remove_dir_all(&dir);
 }
+
+/// 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: `<sub> --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::<Vec<_>>());
+
+    // A missing value is found before anything runs, so an export earlier on the line does not
+    // happen: reported later, `--export first.png --scale --export second.png` wrote first.png
+    // and only then failed, overwriting a file that was already there (noted in review).
+    let first = dir.join("first.png");
+    std::fs::write(&first, b"older file").unwrap();
+    let out =
+        cli().current_dir(&dir).args(["run", "--sample", "--export", first.to_str().unwrap(), "--scale", "--export", "second.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_eq!(std::fs::read(&first).unwrap(), b"older file", "the file that was already there is untouched");
+    assert!(!dir.join("second.png").exists(), "and nothing else is written");
+    std::fs::remove_file(&first).unwrap();
+
+    // 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:?}");
+}
+
+/// `<sub> --help` answers about that subcommand, which is right where the subcommand exists. For
+/// one that does not, it printed the whole usage and **succeeded**, so `typo` on its own was an
+/// error while `typo --help` was not, and a script checking the exit status carried on past a
+/// misspelled command (noted in review).
+#[test]
+fn an_unknown_subcommand_is_an_error_even_with_help_after_it() {
+    for sub in ["typo", "nonsense", "valdiate-pdf", "runn"] {
+        for help in ["--help", "-h"] {
+            let out = cli().arg(sub).arg(help).output().unwrap();
+            assert_eq!(out.status.code(), Some(1), "{sub} {help} must fail");
+            assert!(out.stdout.is_empty(), "{sub} {help} must not answer on stdout: {:?}", String::from_utf8_lossy(&out.stdout));
+            let err = String::from_utf8_lossy(&out.stderr);
+            assert!(err.contains("usage: designcraft-cli"), "{sub} {help}: {err:?}");
+        }
+    }
+
+    // Every subcommand the usage lists still answers on stdout and succeeds, so this is not a
+    // block on subcommand help. The list is read from the usage text, as the code's own check is.
+    let subs: Vec<&str> = USAGE_SUBCOMMANDS.iter().copied().filter(|s| !s.starts_with('-')).collect();
+    assert!(subs.len() >= 8, "the usage lists {} subcommands", subs.len());
+    for sub in subs {
+        let out = cli().arg(sub).arg("--help").output().unwrap();
+        assert_eq!(out.status.code(), Some(0), "{sub} --help must succeed: {:?}", String::from_utf8_lossy(&out.stderr));
+        assert!(!out.stdout.is_empty(), "{sub} --help must answer on stdout");
+    }
+}
+
+/// A subcommand's help has to name the options that subcommand accepts. `validate-pdf` takes
+/// `--standard x4|x1a` -- its own usage string and the module docs both say so -- but the line in
+/// the global `USAGE`, which is what `validate-pdf --help` slices out, left it off, so the option
+/// was hidden from the only place a user would look (noted in review).
+#[test]
+fn subcommand_help_names_the_options_that_subcommand_takes() {
+    let help = |sub: &str| {
+        let out = cli().arg(sub).arg("--help").output().unwrap();
+        assert_eq!(out.status.code(), Some(0), "{sub} --help: {:?}", String::from_utf8_lossy(&out.stderr));
+        String::from_utf8_lossy(&out.stdout).into_owned()
+    };
+    let v = help("validate-pdf");
+    assert!(v.contains("--standard"), "validate-pdf --help hides --standard: {v:?}");
+    // The option is refused for a value it cannot mean, so the help and the parser agree on it.
+    let out = cli().arg("validate-pdf").arg("x.pdf").arg("--standard").arg("x9").output().unwrap();
+    assert_eq!(out.status.code(), Some(1), "an unknown standard is refused");
+
+    // A couple of others, so this is about subcommand help in general and not one line.
+    assert!(help("run").contains("--cmd"), "run --help hides --cmd");
+    assert!(help("mcp").contains("--automation-read-root"), "mcp --help hides its roots");
+}
+
+/// The subcommands the binary's usage text lists. Kept here rather than imported because the
+/// binary is not a library; the assertion above on the count is what notices if it drifts.
+const USAGE_SUBCOMMANDS: &[&str] = &["run", "commands", "describe", "script", "app", "mcp", "perf", "bench", "validate-pdf", "links"];