fix(cli): check the token pair only where a token is read (#7071)

With the environment fallbacks, clap's conflicts_with on the token pair
made a stale exported PHOTOCRAFT_CONTROL_TOKEN and
PHOTOCRAFT_CONTROL_TOKEN_FILE a usage error for every mcp and serve, though
headless mcp and stdio serve read no token. As on main, the pair is now
refused only by serve on TCP and mcp bridging, still as clap's either/or
usage error naming each half as it was given.

An automation root on the command line with PHOTOCRAFT_CONTROL_PORT set
is a usage error too: the variable would switch mcp to bridging, which
drops the roots, with only a note on stderr.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

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<Cli, clap::Error> {
             {
                 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")?;
         }
         _ => {}

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}");

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, `<FILE>` 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:<port>`). 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. `<subcommand> --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:<port>`). 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. `<subcommand> --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.

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<String>,
     /// 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::*;

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 <path>`): every tool is forwarded to a running `photocraft --control 7878 --control-token-file <path>` 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:<port>`), 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 <path>`): every tool is forwarded to a running `photocraft --control 7878 --control-token-file <path>` 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:<port>`), 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`: