diff --git a/apps/photocraft-cli/src/args.rs b/apps/photocraft-cli/src/args.rs index 17c0e8f529..1a1aa4833e 100644 --- a/apps/photocraft-cli/src/args.rs +++ b/apps/photocraft-cli/src/args.rs @@ -14,7 +14,7 @@ use std::path::PathBuf; use clap::error::ErrorKind; use clap::parser::ValueSource; use clap::{ArgAction, ArgGroup, ArgMatches, Args, CommandFactory, FromArgMatches, Parser, Subcommand}; -use photocraft_automation::args::{AutomationArgs, ControlArgs, blank_env_is_unset, control_port, name_env_vars}; +use photocraft_automation::args::{AutomationArgs, ControlArgs, blank_env_is_unset, conflict, control_port, name_env_vars}; use photocraft_codecs::{JpegFormat, TiffCompression}; use photocraft_io::{DdsCompression, DpxBits, TgaBits}; use serde_json::Value; @@ -352,10 +352,29 @@ pub(crate) fn parse(args: &[OsString]) -> Result { { return Err(cmd.error(ErrorKind::ValueValidation, format!("invalid value '{v}' for 'PHOTOCRAFT_CONTROL_PORT': {}", ValueError::Bridge))); } + if let Some(sub) = cmd.find_subcommand_mut(name) { + // Only the bridge reads a token, so a stale exported pair doesn't stop a headless + // run. + if a.bridge.is_some() { + conflict(sub, m, "control_token", "control_token_file")?; + } + // The roots are the headless engine's: the variable switching to the bridge would + // drop them. + if a.bridge_from_env { + conflict(sub, m, "automation_read_root", "bridge")?; + conflict(sub, m, "automation_write_root", "bridge")?; + } + } check_server(cmd.find_subcommand_mut(name), m, "bridge")?; } (Command::Serve(a), Some((name, m))) => { a.port_from_env = from_env(m, "port"); + // Only the TCP server reads a token, so a stale exported pair doesn't stop stdio. + if a.port.is_some() + && let Some(sub) = cmd.find_subcommand_mut(name) + { + conflict(sub, m, "control_token", "control_token_file")?; + } check_server(cmd.find_subcommand_mut(name), m, "port")?; } _ => {} diff --git a/apps/photocraft-cli/tests/it/cli.rs b/apps/photocraft-cli/tests/it/cli.rs index e34751c4db..aad93db025 100644 --- a/apps/photocraft-cli/tests/it/cli.rs +++ b/apps/photocraft-cli/tests/it/cli.rs @@ -1265,11 +1265,11 @@ fn either_or_flags_are_usage_errors() { assert_eq!(code, 2); assert!(err.contains("--params"), "{err}"); let token = "a".repeat(64); - for sub in ["mcp", "serve"] { - let o = bin().args([sub, "--control-token", token.as_str(), "--control-token-file", "token"]).stdin(Stdio::null()).output().unwrap(); + for mode in [["mcp", "--bridge", "1"], ["serve", "--port", "0"]] { + let o = bin().args(mode).args(["--control-token", token.as_str(), "--control-token-file", "token"]).stdin(Stdio::null()).output().unwrap(); let err = String::from_utf8_lossy(&o.stderr); - assert_eq!(o.status.code(), Some(2), "{sub}: {err}"); - assert!(err.contains("--control-token-file") && err.contains("cannot be used with"), "{sub}: {err}"); + assert_eq!(o.status.code(), Some(2), "{mode:?}: {err}"); + assert!(err.contains("--control-token-file") && err.contains("cannot be used with"), "{mode:?}: {err}"); } for port in ["70000", "x"] { let o = bin().args(["serve", "--port", port]).stdin(Stdio::null()).output().unwrap(); @@ -1347,6 +1347,14 @@ fn control_port_env_switches_modes_with_a_notice_and_flags_win() { // Empty or blank counts as unset: `serve` reads stdio (at its end, so it exits at once) and // `mcp` stays headless. + // A root on the command line is the headless engine's: the variable can't switch to a bridge + // that would drop it. + for root in ["--automation-read-root", "--automation-write-root"] { + let Exited { code, stderr: err } = with_var(&["mcp", root, "."], "PHOTOCRAFT_CONTROL_PORT", &port.to_string()); + assert_eq!(code, Some(2), "{root}: {err}"); + assert!(err.contains(root) && err.contains("cannot be used with 'PHOTOCRAFT_CONTROL_PORT'") && !err.contains("bridging"), "{root}: {err}"); + } + for empty in ["", " "] { let Exited { code, stderr: err } = with_var(&["serve"], "PHOTOCRAFT_CONTROL_PORT", empty); assert!(code == Some(0) && !err.contains("PHOTOCRAFT_CONTROL_PORT"), "{empty:?}: {err}"); @@ -1372,17 +1380,30 @@ fn control_port_env_switches_modes_with_a_notice_and_flags_win() { /// `--control-token` and `--control-token-file` fall back to `PHOTOCRAFT_CONTROL_TOKEN` and /// `PHOTOCRAFT_CONTROL_TOKEN_FILE`. One from the environment and the other as a flag is the -/// either/or usage error, naming the variable. Help names the token's variable, never its value. +/// either/or usage error, naming the variable, when a token is used: a stale exported pair doesn't +/// stop headless `mcp` or stdio `serve`. Help names the token's variable, never its value. #[test] fn control_token_env_vars_are_the_flags_fallbacks() { let token = "a".repeat(64); + for mode in [["mcp", "--bridge", "1"], ["serve", "--port", "0"]] { + let Exited { code, stderr: err } = with_var(&[&mode[..], &["--control-token", token.as_str()][..]].concat(), "PHOTOCRAFT_CONTROL_TOKEN_FILE", "token"); + assert_eq!(code, Some(2), "{mode:?}: {err}"); + assert!(err.contains("PHOTOCRAFT_CONTROL_TOKEN_FILE") && err.contains("cannot be used with"), "{mode:?}: {err}"); + let Exited { code, stderr: err } = with_var(&[&mode[..], &["--control-token-file", "token"][..]].concat(), "PHOTOCRAFT_CONTROL_TOKEN", &token); + assert_eq!(code, Some(2), "{mode:?}: {err}"); + assert!(err.contains("PHOTOCRAFT_CONTROL_TOKEN") && err.contains("cannot be used with") && !err.contains(&token), "{mode:?}: {err}"); + } for sub in ["mcp", "serve"] { - let Exited { code, stderr: err } = with_var(&[sub, "--control-token", token.as_str()], "PHOTOCRAFT_CONTROL_TOKEN_FILE", "token"); - assert_eq!(code, Some(2), "{sub}: {err}"); - assert!(err.contains("PHOTOCRAFT_CONTROL_TOKEN_FILE") && err.contains("cannot be used with"), "{sub}: {err}"); - let Exited { code, stderr: err } = with_var(&[sub, "--control-token-file", "token"], "PHOTOCRAFT_CONTROL_TOKEN", &token); - assert_eq!(code, Some(2), "{sub}: {err}"); - assert!(err.contains("PHOTOCRAFT_CONTROL_TOKEN") && err.contains("cannot be used with") && !err.contains(&token), "{sub}: {err}"); + let o = bin().arg(sub).env("PHOTOCRAFT_CONTROL_TOKEN", &token).env("PHOTOCRAFT_CONTROL_TOKEN_FILE", "token").stdin(Stdio::null()).output().unwrap(); + let err = String::from_utf8_lossy(&o.stderr); + assert!(!err.contains("cannot be used with"), "{sub}: {err}"); + // stdio `serve` exits 0 at the end of its input; how headless `mcp` ends there isn't + // pinned, so it only must not be the usage error. + if sub == "serve" { + assert_eq!(o.status.code(), Some(0), "{sub}: {err}"); + } else { + assert_ne!(o.status.code(), Some(2), "{sub}: {err}"); + } let o = bin().args([sub, "--help"]).env("PHOTOCRAFT_CONTROL_TOKEN", &token).output().unwrap(); let out = String::from_utf8_lossy(&o.stdout); assert!(out.contains("[env: PHOTOCRAFT_CONTROL_TOKEN]") && !out.contains(&token), "{sub}: {out}"); diff --git a/book/src/automation/cli.md b/book/src/automation/cli.md index 8970f37b0e..488d3190a2 100644 --- a/book/src/automation/cli.md +++ b/book/src/automation/cli.md @@ -66,11 +66,10 @@ Paths supplied to the current CLI are ordinary operating-system paths. The CLI d CLI parsing checks the flags and their values before any work starts. A flag the subcommand doesn't take (a typo such as `--fromat`), a flag given twice (only `--cmd` and `--params` may repeat), a value out of range (`--quality 0`), malformed `--new` or `--params` JSON, `` together with `--new`, and `--control-token` together with `--control-token-file` are all usage errors (exit status 2), rather than being ignored or failing later. A value flag never takes the next flag as its value, so a value starting with `-` is written with `=` (`--out=-image.png`) or as `./-image.png`; negative numbers such as `--filter -1` still work. A path that is not valid Unicode reaches the command instead of being a usage error: `convert` reads and writes it, while `info`, `run` and `batch` open files through the automation layer, which still needs UTF-8 paths and fails with exit status 1 (`droplet` takes its arguments as text, so it refuses such a path as a usage error). `photocraft-cli --version` (or `-V`) prints the version; there is no `version` subcommand. `mcp` and `serve` reject positional arguments, empty values and options for the wrong mode before -starting a server. Explicit control-token flags require `mcp --bridge` or `serve --port` -(or `PHOTOCRAFT_CONTROL_PORT`, below). 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. +starting a server. Explicit control-token flags require `mcp --bridge` or `serve --port` (or +`PHOTOCRAFT_CONTROL_PORT`, below). 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. -`mcp` and `serve` fall back to environment variables: `PHOTOCRAFT_CONTROL_TOKEN` and `PHOTOCRAFT_CONTROL_TOKEN_FILE` for the token flags, and `PHOTOCRAFT_CONTROL_PORT` for `serve --port` and `mcp --bridge` (which then bridges to `127.0.0.1:`). The automation roots are flags only: the desktop app's `PHOTOCRAFT_AUTOMATION_READ_ROOT` and `PHOTOCRAFT_AUTOMATION_WRITE_ROOT` grant the CLI nothing. A flag wins over its variable, an empty variable counts as unset, and a bad value is a usage error naming the variable. When `PHOTOCRAFT_CONTROL_PORT` switches `serve` to TCP or `mcp` to bridging, the command says so on standard error. ` --help` lists each variable (unless it is set but empty), never with the token's value. A variable holds a port, so `PHOTOCRAFT_CONTROL_PORT=127.0.0.1:7878` is refused; only `--bridge` takes `host:port`. +`mcp` and `serve` fall back to environment variables: `PHOTOCRAFT_CONTROL_TOKEN` and `PHOTOCRAFT_CONTROL_TOKEN_FILE` for the token flags, and `PHOTOCRAFT_CONTROL_PORT` for `serve --port` and `mcp --bridge` (which then bridges to `127.0.0.1:`). The automation roots are flags only: the desktop app's `PHOTOCRAFT_AUTOMATION_READ_ROOT` and `PHOTOCRAFT_AUTOMATION_WRITE_ROOT` grant the CLI nothing. A flag wins over its variable, an empty variable counts as unset, and a bad value is a usage error naming the variable. When `PHOTOCRAFT_CONTROL_PORT` switches `serve` to TCP or `mcp` to bridging, the command says so on standard error. A token together with a token file is a usage error only where a token is read (`serve` on TCP, `mcp` bridging), so a stale exported pair doesn't stop stdio `serve` or headless `mcp`. An automation root given to `mcp` while the variable would switch it to bridging is a usage error too: the roots are the headless engine's, and the bridge would drop them. ` --help` lists each variable (unless it is set but empty), never with the token's value. A variable holds a port, so `PHOTOCRAFT_CONTROL_PORT=127.0.0.1:7878` is refused; only `--bridge` takes `host:port`. This checking is not a security policy layer. Engine commands remain responsible for validating their own parameters. diff --git a/crates/automation/src/args.rs b/crates/automation/src/args.rs index 9837bc1621..236ffbbf3b 100644 --- a/crates/automation/src/args.rs +++ b/crates/automation/src/args.rs @@ -4,13 +4,16 @@ //! //! Parse with [`blank_env_is_unset`] applied to the arguments, so an empty variable counts as unset, //! and pass a parse error through [`name_env_vars`], so a bad value names the variable it came from. +//! Check the token pair with [`conflict`] only where a token is used, so a stale exported pair +//! doesn't stop a launch that needs none. use std::ffi::OsString; use std::path::PathBuf; use clap::builder::NonEmptyStringValueParser; -use clap::error::{ContextKind, ContextValue}; -use clap::{Arg, Args, Command}; +use clap::error::{ContextKind, ContextValue, ErrorKind}; +use clap::parser::ValueSource; +use clap::{Arg, ArgMatches, Args, Command}; use crate::{AuthorizedWorkspace, AutomationError}; @@ -18,7 +21,7 @@ use crate::{AuthorizedWorkspace, AutomationError}; #[derive(Args)] pub struct ControlArgs { /// The control token (64 hex digits) - #[arg(long, value_name = "64-HEX", env = "PHOTOCRAFT_CONTROL_TOKEN", hide_env_values = true, value_parser = NonEmptyStringValueParser::new(), conflicts_with = "control_token_file")] + #[arg(long, value_name = "64-HEX", env = "PHOTOCRAFT_CONTROL_TOKEN", hide_env_values = true, value_parser = NonEmptyStringValueParser::new())] pub control_token: Option, /// A file holding the control token, used instead of --control-token (a server creates it /// when missing) @@ -82,6 +85,24 @@ pub fn name_env_vars(mut error: clap::Error, cmd: &Command, args: &[OsString]) - error } +/// clap's either/or usage error when `m`, parsed by `cmd`, has both `a` and `b`, each named as it was +/// given: the flag, or its variable. For a conflict that depends on the mode, such as the token pair +/// (only a server or bridge reads a token), which clap's `conflicts_with` can't express. +pub fn conflict(cmd: &mut Command, m: &ArgMatches, a: &str, b: &str) -> Result<(), clap::Error> { + if m.value_source(a).is_none() || m.value_source(b).is_none() { + return Ok(()); + } + let shown = |id: &str| { + let arg = cmd.get_arguments().find(|arg| arg.get_id() == id); + match (m.value_source(id), arg.and_then(Arg::get_env)) { + (Some(ValueSource::EnvVariable), Some(var)) => var.to_string_lossy().into_owned(), + _ => arg.map_or_else(|| id.to_owned(), |arg| arg.to_string()), + } + }; + let message = format!("the argument '{}' cannot be used with '{}'", shown(a), shown(b)); + Err(cmd.error(ErrorKind::ArgumentConflict, message)) +} + #[cfg(test)] mod tests { use super::*; diff --git a/docs/control-protocol.md b/docs/control-protocol.md index f1613743af..bea5d28b46 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -315,7 +315,7 @@ magenta alignment lines. `ui.pointer` drives the same code, so agents get identi `photocraft-automation` provides an MCP server built on the official Rust SDK (`rmcp`). It runs in one of two modes: - **Headless** (`photocraft-cli mcp`): an in-process `photocraft_engine::Session`. There is no window. -- **Bridge** (`photocraft-cli mcp --bridge 127.0.0.1:7878 --control-token-file `): every tool is forwarded to a running `photocraft --control 7878 --control-token-file ` over this protocol, so agents see and drive the live app. `--bridge 7878` is short for `--bridge 127.0.0.1:7878`. With no `--bridge`, `PHOTOCRAFT_CONTROL_PORT` selects the bridge port (`127.0.0.1:`), and the CLI says so on standard error; with neither (or the variable empty), the MCP runs headless. A value that is not a port is a usage error naming the variable. +- **Bridge** (`photocraft-cli mcp --bridge 127.0.0.1:7878 --control-token-file `): every tool is forwarded to a running `photocraft --control 7878 --control-token-file ` over this protocol, so agents see and drive the live app. `--bridge 7878` is short for `--bridge 127.0.0.1:7878`. With no `--bridge`, `PHOTOCRAFT_CONTROL_PORT` selects the bridge port (`127.0.0.1:`), and the CLI says so on standard error; with neither (or the variable empty), the MCP runs headless. A value that is not a port is a usage error naming the variable. `--automation-read-root` or `--automation-write-root` with the variable set is a usage error, since the bridge would drop the roots. The bridge keeps one authenticated TCP connection open, and drops and reconnects before a call once the connection has been idle long enough that the app (which closes idle connections after 30s) may have dropped it. A transport failure while sending a request or waiting for its reply, including a reply timeout, drops the connection and reports that the operation may have completed; inspect the document before retrying an edit. Edits are never automatically resent (#1007); a connection that could not be established is reported as "the request was not sent" instead. The MCP server opts its pure-read tools (`ui_inspect`, `doc_inspect`, `command_list`, `jobs_list`, `session_list`, `ui_screenshot`) into one automatic retry on a fresh connection, since a read applies nothing; clients driving `BridgeClient` directly opt in per call with `call_read`. The next separate call reconnects and authenticates, and reply lines whose `id` does not match the request are skipped. The client's call timeout is 55s, strictly below the server's 60s reply deadline, so the bridge's error is the one callers see; commands that can run longer should pass `"wait": false` and poll `jobs.list`. It only accepts loopback addresses, because the app only listens on loopback. Supply its bearer token with `--control-token-file`, `--control-token`, `PHOTOCRAFT_CONTROL_TOKEN_FILE`, or `PHOTOCRAFT_CONTROL_TOKEN`: