From 9af7b2416f9728f9a98835292120d6992d56b12b Mon Sep 17 00:00:00 2001 From: Khawar Zamman Wani Date: Wed, 21 Oct 2026 01:19:01 +0530 Subject: [PATCH] CLI: help wins over a missing value; widen launch-validation tests `--help` or `-h` where a flag's value should be prints usage rather than a "needs a value" error, in every subcommand. Tests cover the missing-value rule outside mcp/serve and the `--flag=--value` form, and the stdin test checks each launch line that used to create a token file. The book's MCP page documents the launch rules. Co-Authored-By: Claude Opus 5.5 --- apps/photocraft-cli/src/lib.rs | 7 ++- apps/photocraft-cli/tests/cli.rs | 84 ++++++++++++++++++++++---------- book/src/automation/mcp.md | 8 +++ 3 files changed, 71 insertions(+), 28 deletions(-) diff --git a/apps/photocraft-cli/src/lib.rs b/apps/photocraft-cli/src/lib.rs index 20eea19d51..51aac5299b 100644 --- a/apps/photocraft-cli/src/lib.rs +++ b/apps/photocraft-cli/src/lib.rs @@ -129,7 +129,12 @@ fn parse(sub: &Subcommand, args: &[String]) -> Result, String> { } a.flags.push((k.to_owned(), Some(v.to_owned()))); } else if sub.values.contains(&s.as_str()) { - let v = args.get(i + 1).filter(|v| !v.starts_with("--") && v.as_str() != "-h").ok_or_else(|| format!("{s} needs a value"))?; + // A flag where the value should be is a missing value, not the value; help still wins. + let v = args.get(i + 1); + if v.is_some_and(|v| v == "--help" || v == "-h") { + return Ok(None); + } + let v = v.filter(|v| !v.starts_with("--")).ok_or_else(|| format!("{s} needs a value"))?; a.flags.push((s.clone(), Some(v.clone()))); i += 1; } else if sub.bare.contains(&s.as_str()) { diff --git a/apps/photocraft-cli/tests/cli.rs b/apps/photocraft-cli/tests/cli.rs index 78ac2177e0..e472e02530 100644 --- a/apps/photocraft-cli/tests/cli.rs +++ b/apps/photocraft-cli/tests/cli.rs @@ -56,6 +56,12 @@ fn every_subcommand_answers_help() { assert!(out.contains("USAGE") && out.contains("photocraft-cli batch"), "{sub} {help}: {out}"); } } + // Help where a flag's value should be is still help, not a missing value or a file named `-h`. + for args in [&["run", "x.png", "--out", "--help"][..], &["commands", "--filter", "-h"], &["mcp", "--bridge", "-h"], &["serve", "--port", "--help"]] { + let o = bin().current_dir(&d).args(args).stdin(Stdio::null()).output().unwrap(); + assert_eq!(o.status.code(), Some(0), "{args:?}: {}", String::from_utf8_lossy(&o.stderr)); + assert!(String::from_utf8_lossy(&o.stdout).contains("USAGE"), "{args:?}"); + } assert_eq!(std::fs::read_dir(&d).unwrap().count(), 0, "help wrote nothing"); } @@ -87,6 +93,22 @@ fn unknown_flags_are_usage_errors() { let o = bin().args(["commands", "--json=yes"]).output().unwrap(); assert_eq!(o.status.code(), Some(2)); assert!(String::from_utf8_lossy(&o.stderr).contains("--json takes no value")); + // A flag can't be a value: a missing value is reported, not filled with the next flag. + let o = bin() + .args(["batch", "--actions"]) + .arg(&actions) + .arg("--in") + .arg(&input) + .arg("--out") + .arg(&out_dir) + .args(["--format", "--quality", "80"]) + .output() + .unwrap(); + assert_eq!(o.status.code(), Some(2)); + assert!(String::from_utf8_lossy(&o.stderr).contains("--format needs a value")); + assert!(!out_dir.exists(), "nothing written"); + // `--flag=value` still passes a value that starts with `--`. + ok(bin().args(["commands", "--filter=--json"])); // Every flag still works in both forms. let (out, _) = ok(bin().args(["commands", "--json", "--filter=gaussian"])); assert!(out.contains("filter.blur.gaussianBlur"), "{out}"); @@ -124,7 +146,6 @@ fn server_modes_reject_ignored_or_conflicting_options() { server_usage_error(&[sub, flag, ".", flag, "."], "may only be specified once"); server_usage_error(&[sub, &format!("{flag}=")], "needs a non-empty value"); server_usage_error(&[sub, flag, "--automation-write-root", "."], &format!("{flag} needs a value")); - server_usage_error(&["mcp", "--bridge", "127.0.0.1:7878", flag, "."], "configure automation roots on the desktop app"); } let transport = if sub == "mcp" { "--bridge" } else { "--port" }; for flag in [transport, "--control-token", "--control-token-file"] { @@ -136,39 +157,48 @@ fn server_modes_reject_ignored_or_conflicting_options() { "use either --control-token or --control-token-file", ); } + for flag in ["--automation-read-root", "--automation-write-root"] { + server_usage_error(&["mcp", "--bridge", "127.0.0.1:7878", flag, "."], "configure automation roots on the desktop app"); + } } +/// Each launch line would, before validation, have started a TCP server and written the token file. #[test] fn server_validation_does_not_consume_stdio_or_create_files() { let d = tmp("server-validation"); let token = d.join("must-not-be-created.token"); - let mut child = bin() - .args(["serve", "--port", "0", "--control-token-file"]) - .arg(&token) - .arg("stray") - .env_remove("PHOTOCRAFT_CONTROL_TOKEN") - .env_remove("PHOTOCRAFT_CONTROL_TOKEN_FILE") - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - .unwrap(); - // Keep stdin open: invalid launch arguments must fail before starting a server. - let input = child.stdin.take().unwrap(); - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); - while child.try_wait().unwrap().is_none() && std::time::Instant::now() < deadline { - std::thread::sleep(std::time::Duration::from_millis(10)); + let other = d.join("other.token"); + let token_flag = format!("--control-token-file={}", token.display()); + let other_flag = format!("--control-token-file={}", other.display()); + for tail in [&["stray"][..], &["--automation-read-root", ".", "--automation-read-root", "."], &["--port", "0"], &[&other_flag]] { + let mut child = bin() + .current_dir(&d) + .args(["serve", "--port", "0", &token_flag]) + .args(tail) + .env_remove("PHOTOCRAFT_CONTROL_TOKEN") + .env_remove("PHOTOCRAFT_CONTROL_TOKEN_FILE") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + // Keep stdin open: invalid launch arguments must fail before starting a server. + let input = child.stdin.take().unwrap(); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while child.try_wait().unwrap().is_none() && std::time::Instant::now() < deadline { + std::thread::sleep(std::time::Duration::from_millis(10)); + } + if child.try_wait().unwrap().is_none() { + child.kill().unwrap(); + child.wait().unwrap(); + panic!("{tail:?}: invalid configuration started a server"); + } + drop(input); + let o = child.wait_with_output().unwrap(); + assert_eq!(o.status.code(), Some(2), "{tail:?}: {}", String::from_utf8_lossy(&o.stderr)); + assert!(o.stdout.is_empty(), "{tail:?}"); + assert!(!token.exists() && !other.exists(), "{tail:?}: validation must happen before creating credentials"); } - if child.try_wait().unwrap().is_none() { - child.kill().unwrap(); - child.wait().unwrap(); - panic!("invalid configuration started a server"); - } - drop(input); - let o = child.wait_with_output().unwrap(); - assert_eq!(o.status.code(), Some(2), "{}", String::from_utf8_lossy(&o.stderr)); - assert!(o.stdout.is_empty()); - assert!(!token.exists(), "validation must happen before creating credentials"); std::fs::remove_dir_all(d).unwrap(); } diff --git a/book/src/automation/mcp.md b/book/src/automation/mcp.md index 2c36b464c2..9f4b5e5ace 100644 --- a/book/src/automation/mcp.md +++ b/book/src/automation/mcp.md @@ -20,6 +20,14 @@ Bridge to a running desktop application with the same private token file used by photocraft-cli mcp --bridge 127.0.0.1:7878 --control-token-file /private/path/control.token ``` +Use absolute root paths in MCP client configurations, since a relative root is resolved from the +client's launch directory. In bridge mode the roots are configured on the desktop app, and root +flags on `photocraft-cli mcp --bridge` are rejected. Explicit `--control-token` and +`--control-token-file` flags require `--bridge` and are mutually exclusive; bridge credentials can +also come from `PHOTOCRAFT_CONTROL_TOKEN` or `PHOTOCRAFT_CONTROL_TOKEN_FILE`. Each launch option +takes one non-empty value and cannot be repeated. Stray positional arguments, missing values and +options for the wrong mode are usage errors (exit 2) reported on stderr before the server starts. + Tools cover session/document operations, command discovery and execution, batching, and—when bridged—UI inspection and control. Tool schemas improve correctness but are not authorization boundaries. ## From an installed release