diff --git a/apps/photocraft-cli/src/lib.rs b/apps/photocraft-cli/src/lib.rs index 5d66941b96..d5fc149607 100644 --- a/apps/photocraft-cli/src/lib.rs +++ b/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 ] [--automation-read-root ] [--automation-write-root ] Run the MCP server on stdio (headless engine, or bridge to a running `photocraft --control `). + 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 ] [--control-token <64-hex> | --control-token-file ] [--automation-read-root ] [--automation-write-root ] Keep one headless session open and answer JSON lines ({\"id\",\"method\",\"params\"}) on stdio, or on 127.0.0.1:. 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 --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, 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, 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()) diff --git a/apps/photocraft-cli/tests/it/cli.rs b/apps/photocraft-cli/tests/it/cli.rs index 95e46a7b45..61778d0fd3 100644 --- a/apps/photocraft-cli/tests/it/cli.rs +++ b/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"); diff --git a/book/src/automation/cli.md b/book/src/automation/cli.md index 05a7c3da27..d33a8baad4 100644 --- a/book/src/automation/cli.md +++ b/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. diff --git a/book/src/automation/mcp.md b/book/src/automation/mcp.md index 2c36b464c2..9b2e8d7c52 100644 --- a/book/src/automation/mcp.md +++ b/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 diff --git a/docs/mcp.md b/docs/mcp.md index 9a8c573ce6..f5c0b24e11 100644 --- a/docs/mcp.md +++ b/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