From 89399250750c0af1bb2023b6a770883496400a84 Mon Sep 17 00:00:00 2001 From: Isaac Levine Date: Mon, 12 Oct 2026 10:57:59 -0400 Subject: [PATCH] CLI: add --automation-read-root and --automation-write-root to mcp --- apps/designcraft-cli/src/automation_args.rs | 90 +++++++++++++++ apps/designcraft-cli/src/main.rs | 39 +++++-- apps/designcraft-cli/tests/cli.rs | 121 ++++++++++++++++++++ 3 files changed, 243 insertions(+), 7 deletions(-) create mode 100644 apps/designcraft-cli/src/automation_args.rs diff --git a/apps/designcraft-cli/src/automation_args.rs b/apps/designcraft-cli/src/automation_args.rs new file mode 100644 index 000000000..52ef34f20 --- /dev/null +++ b/apps/designcraft-cli/src/automation_args.rs @@ -0,0 +1,90 @@ +//! Parse `--automation-read-root` / `--automation-write-root` before the server starts. A malformed +//! flag must not silently remove a root and leave the server unconfined. + +use std::path::PathBuf; + +pub struct AutomationArgs { + pub read_root: Option, + pub write_root: Option, + read_given: bool, + write_given: bool, +} + +impl AutomationArgs { + pub fn new(read_root: Option, write_root: Option) -> Self { + Self { read_root, write_root, read_given: false, write_given: false } + } + + /// Returns whether this argument is a root flag. A second occurrence of a flag is an error, + /// including when the two spellings (`--flag DIR`, `--flag=DIR`) are mixed. + pub fn consume(&mut self, arg: &str, args: &mut impl Iterator) -> Result { + let (flag, inline) = arg.split_once('=').map_or((arg, None), |(flag, value)| (flag, Some(value))); + let (root, given) = match flag { + "--automation-read-root" => (&mut self.read_root, &mut self.read_given), + "--automation-write-root" => (&mut self.write_root, &mut self.write_given), + _ => return Ok(false), + }; + if *given { + return Err(format!("{flag} given twice")); + } + let value = match inline { + Some(value) => value.to_owned(), + None => { + let value = args.next().ok_or_else(|| format!("{flag} needs a folder"))?; + if value.starts_with('-') { + return Err(format!("{flag} needs a folder; use {flag}= for a folder starting with '-'")); + } + value + } + }; + if value.is_empty() { + return Err(format!("{flag} needs a non-empty folder")); + } + *root = Some(value.into()); + *given = true; + Ok(true) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn roots_are_taken_once_and_other_arguments_are_left() { + let mut roots = AutomationArgs::new(None, None); + let mut args = ["a folder".to_owned(), "doc.designcraft".to_owned()].into_iter(); + assert!(roots.consume("--automation-read-root", &mut args).unwrap()); + assert!(roots.consume("--automation-write-root=out", &mut args).unwrap()); + assert!(!roots.consume("--sample", &mut args).unwrap()); + assert_eq!(roots.read_root, Some("a folder".into())); + assert_eq!(roots.write_root, Some("out".into())); + assert_eq!(args.next().as_deref(), Some("doc.designcraft")); + assert!(roots.consume("--automation-read-root=other", &mut args).is_err()); + assert_eq!(roots.read_root, Some("a folder".into())); + } + + #[test] + fn invalid_operands_leave_roots_unchanged() { + for flag in ["--automation-read-root", "--automation-write-root"] { + for operand in [None, Some(""), Some("--version"), Some("--connect"), Some("-folder")] { + let mut roots = AutomationArgs::new(Some("read".into()), Some("write".into())); + let mut args = operand.map(str::to_owned).into_iter(); + assert!(roots.consume(flag, &mut args).is_err(), "{flag} {operand:?}"); + assert_eq!(roots.read_root, Some("read".into())); + assert_eq!(roots.write_root, Some("write".into())); + } + let mut roots = AutomationArgs::new(None, None); + assert!(roots.consume(&format!("{flag}="), &mut std::iter::empty()).is_err()); + } + } + + #[test] + fn explicit_dash_folder_and_relative_dash_folder_are_valid() { + let mut roots = AutomationArgs::new(None, None); + assert!(roots.consume("--automation-read-root=-folder", &mut std::iter::empty()).unwrap()); + assert!(roots.consume("--automation-write-root", &mut ["./-folder".to_owned()].into_iter()).unwrap()); + assert_eq!(roots.read_root, Some("-folder".into())); + assert_eq!(roots.write_root, Some("./-folder".into())); + } +} diff --git a/apps/designcraft-cli/src/main.rs b/apps/designcraft-cli/src/main.rs index c3808f24b..80ee7915f 100644 --- a/apps/designcraft-cli/src/main.rs +++ b/apps/designcraft-cli/src/main.rs @@ -9,7 +9,8 @@ //! # run a command script (crates/engine/src/script.rs): `$N.path` references //! designcraft-cli app [--port PORT] COMMAND [JSON] # run a command in the running app (designcraft --control PORT) //! designcraft-cli app [--port PORT] --method METHOD [JSON] # any control-channel method (ui.screenshot, ui.render, …) -//! designcraft-cli mcp [--connect PORT] [--sample] # MCP server over stdio (docs/mcp.md) +//! designcraft-cli mcp [--connect PORT] [--sample] [--automation-read-root DIR] [--automation-write-root DIR] +//! # MCP server over stdio (docs/mcp.md); the roots confine the files it reads and writes //! designcraft-cli perf [--pages N] [--frames N] [--chars N] [--images N] [--runs N] [--strict] # budgets on a synthetic stress document //! designcraft-cli bench FILE [--runs N] # the same measurements on one document //! designcraft-cli validate-pdf FILE.pdf # run the built-in PDF/X-4 conformance checks @@ -30,13 +31,14 @@ macro_rules! outln { }}; } +mod automation_args; mod perf; use designcraft_engine::Session; use serde_json::{Value, json}; /// The usage block. Shared by `--help` (stdout, success) and an unknown command (stderr, failure). -const USAGE: &str = "usage: designcraft-cli run [--in FILE | --sample] [--cmd ID[=JSON]]... [--page N] [--scale S] [--pdf-options JSON] [--export OUT] [--all-pages DIR]\n (--page, --scale and --pdf-options apply to the exports that follow them)\n designcraft-cli commands [FILTER]\n designcraft-cli describe COMMAND\n designcraft-cli script [FILE|-] [--in FILE | --sample] [--connect PORT] [--save OUT] [--export OUT] [--keep-going]\n designcraft-cli app [--port PORT] COMMAND [JSON] | --method METHOD [JSON]\n designcraft-cli mcp [--connect PORT] [--sample]\n designcraft-cli perf [--pages N] [--runs N] [--strict]\n designcraft-cli bench FILE [--runs N]\n designcraft-cli links\n designcraft-cli --version"; +const USAGE: &str = "usage: designcraft-cli run [--in FILE | --sample] [--cmd ID[=JSON]]... [--page N] [--scale S] [--pdf-options JSON] [--export OUT] [--all-pages DIR]\n (--page, --scale and --pdf-options apply to the exports that follow them)\n designcraft-cli commands [FILTER]\n designcraft-cli describe COMMAND\n designcraft-cli script [FILE|-] [--in FILE | --sample] [--connect PORT] [--save OUT] [--export OUT] [--keep-going]\n designcraft-cli app [--port PORT] COMMAND [JSON] | --method METHOD [JSON]\n designcraft-cli mcp [--connect PORT] [--sample] [--automation-read-root DIR] [--automation-write-root DIR]\n the two roots confine every file the agent's commands read (open, placed images, data sources, libraries) and\n write (save, export, package) to those folders, links followed. A root left out grants none of its access.\n They run the in-process server, so they conflict with --connect (a running app is not confined by them).\n designcraft-cli perf [--pages N] [--runs N] [--strict]\n designcraft-cli bench FILE [--runs N]\n designcraft-cli links\n designcraft-cli --version"; /// The links line under the usage block. fn usage_footer() -> String { @@ -129,16 +131,31 @@ fn validate_pdf(path: Option<&str>) -> Result<(), String> { /// `mcp` (headless, in-process engine) or `mcp --connect PORT|HOST:PORT` (drive a running app /// started with `designcraft --control PORT`). JSON-RPC on stdin/stdout; logs on stderr. fn mcp(args: &[String]) -> Result<(), String> { + use designcraft_engine::file_access::{self, AutomationRoots}; use designcraft_mcp::{Backend, Headless, Remote, Server, control_addr}; let mut connect: Option = None; let mut sample = false; - let mut it = args.iter(); + let mut roots_args = automation_args::AutomationArgs::new(None, None); + let mut it = args.iter().cloned(); while let Some(a) = it.next() { - match a.as_str() { - "--connect" => connect = Some(it.next().cloned().ok_or("--connect needs a port or host:port")?), - "--sample" => sample = true, - other => return Err(format!("unknown mcp option `{other}` (usage: designcraft-cli mcp [--connect PORT] [--sample])")), + if roots_args.consume(&a, &mut it)? { + continue; } + match a.as_str() { + "--connect" => connect = Some(it.next().ok_or("--connect needs a port or host:port")?), + "--sample" => sample = true, + other => { + return Err(format!( + "unknown mcp option `{other}` (usage: designcraft-cli mcp [--connect PORT] [--sample] [--automation-read-root DIR] [--automation-write-root DIR])" + )); + } + } + } + let roots = AutomationRoots::new(roots_args.read_root.as_deref(), roots_args.write_root.as_deref())?; + if roots.is_some() && connect.is_some() { + return Err("--automation-read-root and --automation-write-root confine the in-process server; a running app is not confined by them, \ +so drop --connect (or use --connect without the roots)" + .into()); } let backend: Box = match connect { Some(c) => { @@ -150,9 +167,17 @@ fn mcp(args: &[String]) -> Result<(), String> { } None => { let mut h = Headless::with_document(); + // `--sample` is the operator's own choice, made before the roots come into force. if sample { h.session.execute("file.newSample", &json!({})).map_err(|e| e.to_string())?; } + if let Some(roots) = roots { + // Every thread of this process, not only the server's: nothing reaches past the roots. + file_access::confine_process(roots.clone()); + let show = |root: Option<&std::path::Path>| root.map_or_else(|| "none".to_string(), |r| r.display().to_string()); + eprintln!("designcraft-cli: files confined: read root {}, write root {}", show(roots.read_root()), show(roots.write_root())); + h = h.with_automation_roots(Some(roots)); + } Box::new(h) } }; diff --git a/apps/designcraft-cli/tests/cli.rs b/apps/designcraft-cli/tests/cli.rs index c2804be94..fd96b89bc 100644 --- a/apps/designcraft-cli/tests/cli.rs +++ b/apps/designcraft-cli/tests/cli.rs @@ -154,3 +154,124 @@ fn raster_export_preserves_an_existing_reader_and_private_symlink_target() { assert_eq!(std::fs::metadata(&target).unwrap().permissions().mode() & 0o777, 0o600); assert!(std::fs::symlink_metadata(alias).unwrap().file_type().is_symlink()); } + +/// Send `calls` (tool name, arguments) to `designcraft-cli mcp `; returns whether it exited +/// cleanly, its stderr, and the stdout lines after `initialize`, one per call. +fn mcp_session(args: &[&str], calls: &[(&str, serde_json::Value)]) -> (bool, String, Vec) { + let mut child = cli().arg("mcp").args(args).stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::piped()).spawn().unwrap(); + { + let stdin = child.stdin.as_mut().unwrap(); + writeln!(stdin, r#"{{"jsonrpc":"2.0","id":1,"method":"initialize","params":{{"protocolVersion":"2025-06-18"}}}}"#).unwrap(); + for (i, (name, arguments)) in calls.iter().enumerate() { + let call = serde_json::json!({"jsonrpc": "2.0", "id": i + 2, "method": "tools/call", "params": {"name": name, "arguments": arguments}}); + writeln!(stdin, "{call}").unwrap(); + } + } + let out = child.wait_with_output().unwrap(); + let stdout = String::from_utf8(out.stdout).unwrap(); + let replies = stdout.lines().skip(1).map(|l| serde_json::from_str(l).unwrap()).collect(); + (out.status.success(), String::from_utf8_lossy(&out.stderr).into_owned(), replies) +} + +fn text(reply: &serde_json::Value) -> &str { + reply["result"]["content"][0]["text"].as_str().unwrap() +} + +/// A `.designcraft` file with one page, made with the CLI itself. +fn make_document(path: &std::path::Path) { + let out = cli().args(["run", "--cmd", "file.new={}", "--cmd"]).arg(format!("file.save={}", serde_json::json!({"path": path}))).output().unwrap(); + assert!(out.status.success(), "{}", String::from_utf8_lossy(&out.stderr)); + assert!(path.exists()); +} + +#[test] +fn mcp_automation_roots_confine_the_server() { + let base = std::env::temp_dir().join(format!("dc-cli-roots-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + let (work, elsewhere) = (base.join("work"), base.join("elsewhere")); + std::fs::create_dir_all(&work).unwrap(); + std::fs::create_dir_all(&elsewhere).unwrap(); + make_document(&work.join("in.designcraft")); + make_document(&elsewhere.join("secret.designcraft")); + let work_s = work.to_str().unwrap(); + let secret = elsewhere.join("secret.designcraft").to_str().unwrap().to_string(); + + // Both spellings of the flags; inside works, outside is refused with a readable message. + let read_flag = format!("--automation-read-root={work_s}"); + let calls = [ + ("open_document", serde_json::json!({"path": work.join("in.designcraft").to_str().unwrap()})), + ("open_document", serde_json::json!({"path": secret})), + ("save_document", serde_json::json!({"path": work.join("saved.designcraft").to_str().unwrap()})), + ("save_document", serde_json::json!({"path": elsewhere.join("saved.designcraft").to_str().unwrap()})), + ("export_png", serde_json::json!({"path": elsewhere.join("page.png").to_str().unwrap()})), + ("render_page", serde_json::json!({"path": elsewhere.join("render.png").to_str().unwrap()})), + ]; + let (success, stderr, replies) = mcp_session(&[&read_flag, "--automation-write-root", work_s], &calls); + assert!(success, "{stderr}"); + assert!(stderr.contains("files confined"), "{stderr}"); + assert_eq!(replies[0]["result"]["isError"], false, "{}", replies[0]); + assert_eq!(replies[1]["result"]["isError"], true); + assert!(text(&replies[1]).contains("outside the read root"), "{}", text(&replies[1])); + assert_eq!(replies[2]["result"]["isError"], false, "{}", replies[2]); + assert!(work.join("saved.designcraft").exists()); + for reply in &replies[3..] { + assert_eq!(reply["result"]["isError"], true, "{reply}"); + assert!(text(reply).contains("outside the write root"), "{}", text(reply)); + } + for name in ["saved.designcraft", "page.png", "render.png"] { + assert!(!elsewhere.join(name).exists(), "{name} was written outside the write root"); + } + + // A root left out grants none of its access. + let save = [("new_document", serde_json::json!({})), calls[2].clone()]; + let (_, _, replies) = mcp_session(&[&read_flag], &save); + assert_eq!(replies[1]["result"]["isError"], true); + assert!(text(&replies[1]).contains("write authority is absent"), "{}", text(&replies[1])); + let (_, _, replies) = mcp_session(&["--automation-write-root", work_s], &calls[..1]); + assert_eq!(replies[0]["result"]["isError"], true); + assert!(text(&replies[0]).contains("read authority is absent"), "{}", text(&replies[0])); + + // No flags: nothing is confined. + let (success, _, replies) = mcp_session(&[], &calls[1..2]); + assert!(success); + assert_eq!(replies[0]["result"]["isError"], false, "{}", replies[0]); + let _ = std::fs::remove_dir_all(base); +} + +#[test] +fn mcp_automation_root_flags_are_checked_up_front() { + let base = std::env::temp_dir().join(format!("dc-cli-roots-args-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + std::fs::create_dir_all(&base).unwrap(); + let dir = base.to_str().unwrap(); + let missing = base.join("missing"); + let fails = |args: &[&str], needle: &str| { + let mut full = vec!["mcp"]; + full.extend_from_slice(args); + let o = cli().args(&full).output().unwrap(); + assert!(!o.status.success(), "{args:?}"); + let err = String::from_utf8_lossy(&o.stderr).into_owned(); + assert!(err.contains(needle), "{args:?}: {err}"); + }; + // The flags confine the in-process server, so they cannot be combined with --connect. + fails(&["--connect", "7979", "--automation-read-root", dir], "--connect"); + fails(&["--automation-write-root", dir, "--connect", "7979"], "--connect"); + fails(&["--automation-read-root", dir, "--automation-read-root", dir], "given twice"); + fails(&["--automation-write-root", dir, "--automation-write-root", dir], "given twice"); + fails(&["--automation-read-root", dir, "--automation-read-root=x"], "given twice"); + fails(&["--automation-read-root"], "needs a folder"); + fails(&["--automation-write-root", "--sample"], "needs a folder"); + fails(&["--automation-read-root", missing.to_str().unwrap()], "cannot open the automation read root"); + fails(&["--automation-write-root="], "needs a non-empty folder"); + // A file is not a folder. + std::fs::write(base.join("f.txt"), "x").unwrap(); + fails(&["--automation-write-root", base.join("f.txt").to_str().unwrap()], "cannot open the automation write root"); + let _ = std::fs::remove_dir_all(base); +} + +#[test] +fn usage_lists_the_automation_root_flags() { + let out = cli().arg("--help").output().unwrap(); + let usage = String::from_utf8_lossy(&out.stdout); + assert!(usage.contains("--automation-read-root DIR") && usage.contains("--automation-write-root DIR"), "{usage}"); +}