Merge PR #8035: CLI: reject ignored or conflicting mcp/serve launch options

apps/photocraft-cli/src/lib.rs

@@ -52,15 +52,20 @@ USAGE:
   photocraft-cli mcp [--bridge <127.0.0.1:port>] [--control-token <64-hex> | --control-token-file <path>]
       [--automation-read-root <dir>] [--automation-write-root <dir>]
       Run the MCP server on stdio (headless engine, or bridge to a running `photocraft --control <port>`).
+      Headless: each root grants that direction of file access; an omitted root denies it.
+      Prefer absolute root paths in MCP client configurations.
+      Bridge: set roots on the desktop app; CLI root flags are rejected. Explicit token flags
+      require --bridge; credentials can also use PHOTOCRAFT_CONTROL_TOKEN or PHOTOCRAFT_CONTROL_TOKEN_FILE.
   photocraft-cli serve [--port <port>] [--control-token <64-hex> | --control-token-file <path>]
       [--automation-read-root <dir>] [--automation-write-root <dir>]
       Keep one headless session open and answer JSON lines ({\"id\",\"method\",\"params\"}) on stdio,
       or on 127.0.0.1:<port>. Methods: engine.execute, jobs.list/cancel, engine.commands,
       doc.open/new/place/save/exportLayers/exportDataSets/importDataSets/inspect/render/select/close, session.list, batch, methods
       (docs/control-protocol.md#headless-server).
+      Explicit token flags require --port. Stdio needs no control token.
 
   photocraft-cli <subcommand> --help (or -h) prints this text. A flag the subcommand doesn't take is
-  an error.
+  an error. mcp and serve take no positional arguments; their options cannot be repeated or empty.
 ";
 
 struct Args {
@@ -155,7 +160,12 @@ fn parse(sub: &Subcommand, args: &[String]) -> Result<Option<Args>, 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).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()) {
@@ -167,9 +177,43 @@ fn parse(sub: &Subcommand, args: &[String]) -> Result<Option<Args>, String> {
         }
         i += 1;
     }
+    validate_server_args(sub.name, &a)?;
     Ok(Some(a))
 }
 
+/// Refuse launch options that would otherwise be silently ignored. In particular, bridge
+/// roots belong to the desktop process: accepting them here promises authority we cannot set.
+fn validate_server_args(sub: &str, a: &Args) -> R {
+    if !matches!(sub, "mcp" | "serve") {
+        return Ok(());
+    }
+    if !a.positional.is_empty() {
+        return Err(format!("{sub} does not take positional arguments"));
+    }
+    let mut seen = std::collections::HashSet::new();
+    for (flag, value) in &a.flags {
+        if !seen.insert(flag) {
+            return Err(format!("{flag} may only be specified once"));
+        }
+        if value.as_deref().is_none_or(str::is_empty) {
+            return Err(format!("{flag} needs a non-empty value"));
+        }
+    }
+    if a.has("--control-token") && a.has("--control-token-file") {
+        return Err("use either --control-token or --control-token-file, not both".into());
+    }
+    let transport = if sub == "mcp" { "--bridge" } else { "--port" };
+    for flag in ["--control-token", "--control-token-file"] {
+        if a.has(flag) && !a.has(transport) {
+            return Err(format!("{flag} requires {transport}"));
+        }
+    }
+    if sub == "mcp" && a.has("--bridge") && (a.has("--automation-read-root") || a.has("--automation-write-root")) {
+        return Err("configure automation roots on the desktop app; --automation-read-root and --automation-write-root require headless MCP mode".into());
+    }
+    Ok(())
+}
+
 impl Args {
     fn get(&self, k: &str) -> Option<&str> {
         self.flags.iter().rev().find(|(f, _)| f == k).and_then(|(_, v)| v.as_deref())

apps/photocraft-cli/tests/it/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}");
@@ -105,6 +127,81 @@ fn unknown_flags_are_usage_errors() {
     assert!(!out.trim().contains('\n'), "compact JSON is one line");
 }
 
+fn server_usage_error(args: &[&str], expected: &str) {
+    let o = bin().args(args).env_remove("PHOTOCRAFT_CONTROL_TOKEN").env_remove("PHOTOCRAFT_CONTROL_TOKEN_FILE").stdin(Stdio::null()).output().unwrap();
+    let err = String::from_utf8_lossy(&o.stderr);
+    assert_eq!(o.status.code(), Some(2), "{args:?}: {err}");
+    assert!(o.stdout.is_empty(), "usage errors must leave protocol stdout empty: {args:?}");
+    assert!(err.contains(expected), "{args:?}: expected {expected:?} in {err}");
+}
+
+#[test]
+fn server_modes_reject_ignored_or_conflicting_options() {
+    for sub in ["mcp", "serve"] {
+        server_usage_error(&[sub, "stray.psd"], "does not take positional arguments");
+        for flag in ["--control-token", "--control-token-file"] {
+            server_usage_error(&[sub, flag, "unused"], if sub == "mcp" { "requires --bridge" } else { "requires --port" });
+        }
+        for flag in ["--automation-read-root", "--automation-write-root"] {
+            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"));
+        }
+        let transport = if sub == "mcp" { "--bridge" } else { "--port" };
+        for flag in [transport, "--control-token", "--control-token-file"] {
+            server_usage_error(&[sub, flag, "one", &format!("{flag}=two")], "may only be specified once");
+            server_usage_error(&[sub, flag, ""], "needs a non-empty value");
+        }
+        server_usage_error(
+            &[sub, transport, "7878", "--control-token", "unused", "--control-token-file", "unused"],
+            "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 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");
+    }
+    std::fs::remove_dir_all(d).unwrap();
+}
+
 #[test]
 fn convert_png_pcraft_png_is_lossless() {
     let d = tmp("convert");

book/src/automation/cli.md

@@ -64,3 +64,8 @@ layers because that would lose their group's blending semantics.
 Paths supplied to the current CLI are ordinary operating-system paths. The CLI does not restrict them to a workspace root. Batch output creates the requested output directory, and save operations may create parent directories. `batch` refuses an output directory that is its input directory (the results would replace the originals) unless `--in-place` is given. Run untrusted action files only in an appropriately isolated account or environment.
 
 CLI parsing reports malformed flags and JSON as errors, and a flag the subcommand doesn't take (a typo such as `--fromat`) is a usage error (exit status 2) rather than being ignored. It is not a security policy layer. Engine commands remain responsible for validating their own parameters.
+
+`mcp` and `serve` reject positional arguments, repeated options, missing or empty values and
+options for the wrong mode before starting a server. Explicit control-token flags require
+`mcp --bridge` or `serve --port`. Configure automation roots on the desktop process when using
+bridge mode; the CLI rejects root flags there. See the [MCP launch guide](mcp.md) for examples.

book/src/automation/mcp.md

@@ -20,6 +20,15 @@ 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
+except `--help`/`-h` 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

docs/mcp.md

@@ -57,6 +57,18 @@ Paths are relative to the corresponding root, or absolute paths beneath it; with
 filesystem authority is absent. Bridge mode uses `--bridge` and `--control-token-file` as
 described in [development.md](development.md). UI tools require a running desktop app.
 
+Use absolute root paths in MCP client configurations, since a relative root is resolved from
+the client's launch directory. In bridge mode, configure roots on the **desktop app**; 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`.
+
+MCP and `serve` launch options (all but `--help`/`-h`) take one non-empty value each and cannot
+be repeated. Missing values, stray positional arguments and options for the wrong mode are
+usage errors (exit 2), reported on stderr before the server starts; stdout stays available for
+protocol messages. For `serve`, explicit credential flags require `--port`; stdio needs no
+control token.
+
 ## Tools and resources
 
 - `command_list`: discover engine ids, parameter docs and current enabled state; accepts