storytold/photocraft
2
0
Fork 0
mirror of https://github.com/storytold/photocraft.git synced 2026-11-02 18:13:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

CLI: help wins over a missing value; widen launch-validation tests

Browse Source 
`--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 <noreply@anthropic.com>
This commit is contained in:
Khawar Zamman Wani
2026-10-21 01:19:01 +05:30
parent ed9bf013c0
commit 9af7b2416f
3 changed files with 71 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/photocraft-cli/src/lib.rs
+6 -1
View File
@@ -129,7 +129,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).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()) {
apps/photocraft-cli/tests/cli.rs
+57 -27
View File
@@ -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();
}
book/src/automation/mcp.md
+8
View File
@@ -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