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.
- `<sub> --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

@@ -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<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())
+}
+
+/// 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() {
+        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<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.
+    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<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()?;
@@ -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()?;

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: `<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 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:?}");
+}