From 434e269691bcffd494e68190ae20c49925bf4326 Mon Sep 17 00:00:00 2001 From: Hamza Date: Wed, 28 Oct 2026 18:34:12 +0500 Subject: [PATCH 1/6] 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:?}"); +} From f11f5f3b11ac06be32c142b1d681bdb912c6bd10 Mon Sep 17 00:00:00 2001 From: Hamza Date: Thu, 29 Oct 2026 13:51:49 +0500 Subject: [PATCH 2/6] cli: a missing value is found before anything runs Review caught a consequence of the previous commit. Standing the order check aside so a missing value could be reported meant the value was reported later -- after the exports before it had already been written. `run --export first.png --scale --export second.png` wrote first.png and only then failed, so an invocation that fails could overwrite a file that was already there. `values_present` checks every option that takes a value before the order check and before any export, so the line is refused as a whole. The test writes a file at the export path first and asserts its bytes are unchanged. --- apps/designcraft-cli/src/main.rs | 21 +++++++++++++++++++++ apps/designcraft-cli/tests/it/cli.rs | 14 ++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/apps/designcraft-cli/src/main.rs b/apps/designcraft-cli/src/main.rs index 98c9b32d2..46e4a7c8c 100644 --- a/apps/designcraft-cli/src/main.rs +++ b/apps/designcraft-cli/src/main.rs @@ -379,7 +379,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" diff --git a/apps/designcraft-cli/tests/it/cli.rs b/apps/designcraft-cli/tests/it/cli.rs index 87e5b58fd..a2c909e38 100644 --- a/apps/designcraft-cli/tests/it/cli.rs +++ b/apps/designcraft-cli/tests/it/cli.rs @@ -592,6 +592,20 @@ fn the_argument_parser_refuses_what_it_cannot_mean() { 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 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()); From 4c9e5277f79c89b4ddc2ed546a847c0d2ac8d1cb Mon Sep 17 00:00:00 2001 From: Hamza Date: Mon, 2 Nov 2026 16:00:15 +0500 Subject: [PATCH 3/6] cli: subcommand help is for subcommands that exist ` --help` answered about that subcommand without first asking whether the subcommand was one, so `designcraft-cli typo --help` printed the whole usage and succeeded. `typo` on its own was an error and `typo --help` was not, which is backwards: a script checking the exit status carried straight past a misspelled command (noted in review). `is_subcommand` reads the names out of `USAGE` rather than listing them again, so one added to the usage text cannot be missing here, and `usage_for` now uses the same line parser instead of its own copy. An unknown name falls through to the dispatch and fails there, as it already did without `--help`. Measured over every name the usage lists -- run, commands, describe, script, app, mcp, perf, bench, validate-pdf, links -- each still answers on stdout and exits 0, while `typo` and `nonsense` exit 1. The test asserts both halves, so the check cannot quietly become a block on subcommand help. --- apps/designcraft-cli/src/main.rs | 27 +++++++++++++++++++++--- apps/designcraft-cli/tests/it/cli.rs | 31 ++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/apps/designcraft-cli/src/main.rs b/apps/designcraft-cli/src/main.rs index 46e4a7c8c..9d7b0306f 100644 --- a/apps/designcraft-cli/src/main.rs +++ b/apps/designcraft-cli/src/main.rs @@ -124,13 +124,27 @@ fn can_be_value(v: &str) -> bool { /// The usage lines for one subcommand, for ` --help`. Each line of [`USAGE`] names its /// subcommand, and the indented lines after it belong to it. +/// 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()) +} + 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 let Some(named) = usage_subcommand(line) { + keep = named == sub; } if keep { out.push(line); @@ -156,8 +170,15 @@ fn main() -> ExitCode { } // ` --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)); diff --git a/apps/designcraft-cli/tests/it/cli.rs b/apps/designcraft-cli/tests/it/cli.rs index a2c909e38..444ebf758 100644 --- a/apps/designcraft-cli/tests/it/cli.rs +++ b/apps/designcraft-cli/tests/it/cli.rs @@ -641,3 +641,34 @@ fn an_argument_that_is_not_utf8_is_an_error_not_a_panic() { assert!(err.contains("not valid UTF-8"), "{err:?}"); assert!(!err.contains("panicked"), "{err:?}"); } + +/// ` --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"); + } +} + +/// 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"]; From 133e9098e8abc76452f8c5b49d69ab6e3b1abbff Mon Sep 17 00:00:00 2001 From: Hamza Date: Mon, 2 Nov 2026 17:39:55 +0500 Subject: [PATCH 4/6] cli: validate-pdf's help names --standard `validate_pdf` accepts `--standard x4|x1a`, and both its own usage string and the module docs say so, but the line in the global `USAGE` left it off -- and that line is exactly what `validate-pdf --help` slices out. So the option was documented everywhere except the one place a user would look (noted in review). `subcommand_help_names_the_options_that_subcommand_takes` asserts it, checks the parser and the help agree by refusing an unknown standard, and does the same for `run --cmd` and `mcp --automation-read-root` so this is about subcommand help rather than one line. --- apps/designcraft-cli/src/main.rs | 2 +- apps/designcraft-cli/tests/it/cli.rs | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/apps/designcraft-cli/src/main.rs b/apps/designcraft-cli/src/main.rs index 9d7b0306f..b03bbec02 100644 --- a/apps/designcraft-cli/src/main.rs +++ b/apps/designcraft-cli/src/main.rs @@ -40,7 +40,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 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 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] diff --git a/apps/designcraft-cli/tests/it/cli.rs b/apps/designcraft-cli/tests/it/cli.rs index 444ebf758..172220df7 100644 --- a/apps/designcraft-cli/tests/it/cli.rs +++ b/apps/designcraft-cli/tests/it/cli.rs @@ -669,6 +669,28 @@ fn an_unknown_subcommand_is_an_error_even_with_help_after_it() { } } +/// 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"]; From 61ed22140a91b4990ab97484fcc4e08e1c35422b Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Tue, 3 Nov 2026 19:41:27 +0000 Subject: [PATCH 5/6] Apply review fixes - apps/designcraft-cli/src/main.rs --- apps/designcraft-cli/src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/designcraft-cli/src/main.rs b/apps/designcraft-cli/src/main.rs index 32faeda04..2882ef059 100644 --- a/apps/designcraft-cli/src/main.rs +++ b/apps/designcraft-cli/src/main.rs @@ -124,8 +124,6 @@ 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. /// 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 { @@ -141,6 +139,8 @@ fn usage_subcommand(line: &str) -> Option<&str> { .and_then(|rest| rest.split_whitespace().next()) } +/// 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; From 65c4db30e1be4c4275140f71271be59b247d7c2d Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Tue, 3 Nov 2026 21:48:08 +0000 Subject: [PATCH 6/6] Apply review fixes - apps/designcraft-cli/src/main.rs --- apps/designcraft-cli/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/designcraft-cli/src/main.rs b/apps/designcraft-cli/src/main.rs index 2882ef059..c106ae544 100644 --- a/apps/designcraft-cli/src/main.rs +++ b/apps/designcraft-cli/src/main.rs @@ -345,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!(