From 15e35cf60572d31841592208b97ff99a53c53a45 Mon Sep 17 00:00:00 2001 From: storybored379 Date: Fri, 9 Oct 2026 06:54:14 -0700 Subject: [PATCH] MCP: shared conventions and cancellable Render Video (#1668) The MCP server follows the crafts' shared conventions (FilmCraft #28): core tools incl. render_preview, titles and annotation hints, strict argument keys (-32602), live photocraft://document and photocraft://commands resources, and -32700 recovery from malformed JSON lines. Headless Render Video writes through the granted write root, reports progress, stays ping-responsive and can be cancelled, removing only the files it created. docs/mcp.md describes it. Co-Authored-By: Claude Opus 5.5 --- book/src/automation/mcp.md | 6 + crates/automation/src/headless.rs | 32 +++++ crates/automation/src/server.rs | 40 +++++- crates/automation/src/server/conventions.rs | 86 ++++++++++++ crates/automation/src/server/progress.rs | 43 ++++++ crates/automation/src/server/transport.rs | 61 ++++++++ crates/automation/src/workspace.rs | 83 +++++++++++ crates/automation/tests/mcp.rs | 60 +++++++- crates/automation/tests/mcp_wire.rs | 147 ++++++++++++++++++++ crates/engine/src/lib.rs | 1 + crates/engine/src/video_cmds.rs | 38 ++++- crates/engine/src/video_cmds/tests.rs | 29 ++++ docs/mcp.md | 64 +++++++++ 13 files changed, 677 insertions(+), 13 deletions(-) create mode 100644 crates/automation/src/server/conventions.rs create mode 100644 crates/automation/src/server/progress.rs create mode 100644 crates/automation/src/server/transport.rs create mode 100644 crates/automation/tests/mcp_wire.rs create mode 100644 docs/mcp.md diff --git a/book/src/automation/mcp.md b/book/src/automation/mcp.md index 830208fc..c091f775 100644 --- a/book/src/automation/mcp.md +++ b/book/src/automation/mcp.md @@ -30,3 +30,9 @@ The current server does not issue scoped sessions or distinguish trusted tools f Filesystem capabilities are implemented. General tool and method capabilities remain [proposed](../security/automation-security.md). + +The five shared core tools are `command_list`, `command_run`, `command_batch`, `doc_inspect` +and `render_preview`. Existing documented tools, including `doc_render_preview`, remain +listed. `photocraft://document` and `photocraft://commands` expose live JSON. Direct headless +Render Video supports progress and cancellation using the granted write root; see +[`docs/mcp.md`](../../../docs/mcp.md) for arguments, cleanup and transport behavior. diff --git a/crates/automation/src/headless.rs b/crates/automation/src/headless.rs index 4c663086..b0612661 100644 --- a/crates/automation/src/headless.rs +++ b/crates/automation/src/headless.rs @@ -239,6 +239,9 @@ impl Headless { pub fn command_start(&mut self, id: &str, params: Value, wait: bool) -> Result { let params = if params.is_null() { json!({}) } else { params }; if !matches!(&self.filesystem, Filesystem::TrustedLocal) { + if id == "file.export.renderVideo" { + return self.render_video(params, |_, _| true); + } authorize_engine_command(id, ¶ms)?; } // Background jobs that finished since the last request (or batch step) are applied first. @@ -252,3 +255,32 @@ impl Headless { }) } } + +impl Headless { + /// A direct automation Render Video uses only the launch-time write capability. Nested + /// engine actions still reject this filesystem command until they can carry capabilities. + pub(crate) fn render_video(&mut self, params: Value, progress: impl FnMut(usize, usize) -> bool) -> Result { + self.sync_jobs(); + let Filesystem::Workspace(workspace) = &self.filesystem else { + return Err(AutomationError::BadRequest("automation filesystem access is not granted: write authority is absent".into())); + }; + let dir = params.get("dir").and_then(Value::as_str).ok_or_else(|| AutomationError::BadRequest("renderVideo needs `dir`".into()))?; + let mut outputs = workspace.export_sequence(dir)?; + let result = photocraft_engine::render_video_with( + &self.session, + ¶ms, + |name, bytes| outputs.write(name, bytes).map_err(|e| photocraft_engine::EngineError::Other(e.to_string())), + progress, + ); + match result { + Ok(value) => { + outputs.commit(); + Ok(value) + } + Err(error) => { + outputs.cleanup()?; + Err(error.into()) + } + } + } +} diff --git a/crates/automation/src/server.rs b/crates/automation/src/server.rs index 61c439d0..7184b990 100644 --- a/crates/automation/src/server.rs +++ b/crates/automation/src/server.rs @@ -8,7 +8,7 @@ use base64::Engine as _; use rmcp::handler::server::router::tool::ToolRouter; use rmcp::handler::server::wrapper::Parameters; use rmcp::model::{CallToolResult, ContentBlock as Content}; -use rmcp::{ErrorData as McpError, ServerHandler, ServiceExt, tool, tool_handler, tool_router}; +use rmcp::{ErrorData as McpError, ServerHandler, ServiceExt, tool, tool_router}; use schemars::JsonSchema; use serde::Deserialize; use serde_json::{Value, json}; @@ -38,6 +38,7 @@ pub struct PhotocraftMcp { // --------------------------------------------------------------------------- #[derive(Debug, Default, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct DocIndex { /// Document index from `session_list` (default: the active document). #[serde(default)] @@ -45,12 +46,14 @@ pub struct DocIndex { } #[derive(Debug, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct OpenParams { /// Forward-slash relative path beneath the configured automation read root. pub path: String, } #[derive(Debug, Default, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct NewParams { #[serde(default)] pub width: Option, @@ -70,6 +73,7 @@ pub struct NewParams { } #[derive(Debug, Default, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct SaveParams { /// Forward-slash relative target beneath the configured automation write root. /// The extension selects the format. Omit to write back to the document's own file, which @@ -91,6 +95,7 @@ pub struct SaveParams { } #[derive(Debug, Default, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct PreviewParams { /// Headless mode only: document index (default: the active document). /// Bridge previews capture the app window and reject `index`. @@ -112,11 +117,13 @@ pub struct ScreenshotParams { } #[derive(Debug, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct SelectParams { pub index: usize, } #[derive(Debug, Default, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct ListParams { /// Only commands whose id or label contains this text (case-insensitive). #[serde(default)] @@ -127,6 +134,7 @@ pub struct ListParams { } #[derive(Debug, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct RunParams { /// Command id, e.g. `layer.new.layer`, `filter.blur.gaussianBlur`. pub id: String, @@ -141,6 +149,7 @@ pub struct RunParams { } #[derive(Debug, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct JobCancelParams { /// The job id from `command_run` / `jobs_list` (omit to cancel every running job). #[serde(default)] @@ -148,6 +157,7 @@ pub struct JobCancelParams { } #[derive(Debug, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct BatchParams { /// Commands to run in order: `[{"id": "layer.new.layer", "params": {"name": "Ink"}}, …]`. A /// step with `"wait": false` starts a long command as a background job (its result is @@ -176,12 +186,14 @@ pub struct PointerParams { } #[derive(Debug, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct MenuParams { /// Menu item / command id. pub id: String, } #[derive(Debug, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct UiSetParams { /// Fields for the control method `ui.set`: tool, panels, dock, dockTabs, dockWidth, colorPanel /// ({background: bool} picks which swatch the Color panel edits), maskTarget, @@ -194,6 +206,7 @@ pub struct UiSetParams { } #[derive(Debug, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct ControlParams { /// Any control-protocol method, e.g. `ui.dialog.open`. pub method: String, @@ -276,9 +289,7 @@ impl PhotocraftMcp { /// Serve MCP over stdin/stdout until the client disconnects. pub async fn serve_stdio(self) -> Result<(), AutomationError> { - let running = self.serve(rmcp::transport::stdio()).await.map_err(|e| AutomationError::Other(format!("MCP init: {e}")))?; - running.waiting().await.map_err(|e| AutomationError::Other(e.to_string()))?; - Ok(()) + self.serve_io(tokio::io::stdin(), tokio::io::stdout()).await } /// Run `f` on the headless session on a blocking thread. @@ -438,7 +449,16 @@ impl PhotocraftMcp { #[tool(description = "Document state as JSON: layer tree (top to bottom), history, selection, active layer.")] async fn doc_inspect(&self, Parameters(p): Parameters) -> Result { - if let Some(r) = self.headless_op(move |h| h.inspect(p.index)).await { + if let Some(r) = self + .headless_op(move |h| { + if p.index.is_none() && h.session.active_index().is_none() { + Ok(json!({"document": null, "session": h.session_list()})) + } else { + h.inspect(p.index) + } + }) + .await + { return to_result(r); } let Some(b) = self.bridge_client() else { @@ -466,6 +486,11 @@ impl PhotocraftMcp { self.screenshot(b, Some(max)).await } + #[tool(description = "Render a bounded flattened PNG preview (bridge mode: app window screenshot).")] + async fn render_preview(&self, p: Parameters) -> Result { + self.doc_render_preview(p).await + } + #[tool(description = "Make the document at `index` active.")] async fn doc_select(&self, Parameters(p): Parameters) -> Result { match self.headless_op(move |h| h.select(p.index)).await { @@ -711,8 +736,9 @@ impl PhotocraftMcp { } } -#[tool_handler(router = self.tool_router, name = "photocraft", instructions = "Photocraft image editor. Every edit is an engine command: call `command_list` to discover ids and parameter docs, then `command_run` (or `command_batch` for several at once). Use `doc_open`/`doc_new` first, `doc_inspect` for the layer tree, `doc_render_preview` to see the result, and `doc_save` (.pcraft is lossless native; .psd/.png/.jpg/.tif… export). In bridge mode the `ui_*` tools drive the live app (inspect, screenshot, pointer, menus).")] -impl ServerHandler for PhotocraftMcp {} +mod conventions; +mod progress; +mod transport; /// Used by the render helper in tests and the CLI. pub fn render_document_png(doc: &photocraft_doc::Document, max_side: u32) -> Result, AutomationError> { diff --git a/crates/automation/src/server/conventions.rs b/crates/automation/src/server/conventions.rs new file mode 100644 index 00000000..93a78ac7 --- /dev/null +++ b/crates/automation/src/server/conventions.rs @@ -0,0 +1,86 @@ +//! Catalog metadata, strict tool envelopes and live resources. +use super::*; +use rmcp::RoleServer; +use rmcp::model::*; +use rmcp::service::RequestContext; + +const DOCUMENT: &str = "photocraft://document"; +const COMMANDS: &str = "photocraft://commands"; +const INSTRUCTIONS: &str = "PhotoCraft image editor. Start with doc_new/doc_open, discover ids and parameter docs with command_list, edit with command_run or command_batch (one history step per command). Inspect with doc_inspect and render_preview; doc_save writes native .pcraft or exports by extension. File paths are relative to the read/write roots granted at launch. ui_inspect/ui_screenshot and other ui_* tools require bridge mode. Read photocraft://document and photocraft://commands for live JSON state."; + +fn annotated(mut tool: Tool) -> Tool { + let words = tool.name.replace('_', " "); + let mut chars = words.chars(); + let title = chars.next().map(|c| c.to_uppercase().collect::() + chars.as_str()).unwrap_or_default(); + let read = matches!( + tool.name.as_ref(), + "command_list" | "doc_inspect" | "session_list" | "render_preview" | "doc_render_preview" | "ui_inspect" | "ui_screenshot" + ); + let additive = matches!(tool.name.as_ref(), "doc_new" | "doc_open"); + tool.title = Some(title.clone()); + tool.annotations = Some(ToolAnnotations::from_raw(Some(title), Some(read), Some(!read && !additive), Some(read), Some(false))); + Arc::make_mut(&mut tool.input_schema).insert("additionalProperties".into(), json!(false)); + tool +} + +impl ServerHandler for PhotocraftMcp { + fn get_info(&self) -> ServerConfig { + ServerConfig::new(ServerCapabilities::builder().enable_tools().enable_resources().build()) + .with_server_info(Implementation::new("photocraft", env!("CARGO_PKG_VERSION"))) + .with_instructions(INSTRUCTIONS) + } + + async fn list_tools(&self, _: Option, _: RequestContext) -> Result { + let tools: Vec<_> = self.tool_router.list_all().into_iter().map(annotated).collect(); + serde_json::from_value(json!({"resultType":"complete", "ttlMs":600000, "cacheScope":"private", "tools":tools})) + .map_err(|e| McpError::internal_error(e.to_string(), None)) + } + + async fn call_tool(&self, request: CallToolRequestParams, context: RequestContext) -> Result { + if let Some(tool) = self.tool_router.get(&request.name) { + let properties = tool.input_schema.get("properties").and_then(Value::as_object); + if let Some(args) = &request.arguments + && let Some(key) = args.keys().find(|k| properties.is_none_or(|p| !p.contains_key(*k))) + { + return Err(McpError::invalid_params(format!("unknown argument `{key}` for {}", request.name), None)); + } + } + if request.name == "command_batch" { + let args = Value::Object(request.arguments.clone().unwrap_or_default()); + let _: BatchParams = serde_json::from_value(args).map_err(|e| McpError::invalid_params(e.to_string(), None))?; + } + if request.name == "command_run" && matches!(&*self.backend, Backend::Headless(_)) { + let args = Value::Object(request.arguments.clone().unwrap_or_default()); + let params: RunParams = serde_json::from_value(args).map_err(|e| McpError::invalid_params(e.to_string(), None))?; + if params.id == "file.export.renderVideo" { + return Ok(self.render_video_progress(params.params.unwrap_or_else(|| json!({})), context).await.into()); + } + } + let tcc = rmcp::handler::server::tool::ToolCallContext::new(self, request, context); + self.tool_router.call(tcc).await + } + + async fn list_resources(&self, _: Option, _: RequestContext) -> Result { + serde_json::from_value(json!({"resultType":"complete", "ttlMs":600000, "cacheScope":"private", "resources":[ + {"uri":DOCUMENT, "name":"document", "title":"Active document", "mimeType":"application/json", "description":"Live doc_inspect state, or an empty session."}, + {"uri":COMMANDS, "name":"commands", "title":"Command catalog", "mimeType":"application/json", "description":"Live command_list including enabled state."} + ]})).map_err(|e| McpError::internal_error(e.to_string(), None)) + } + + async fn read_resource(&self, request: ReadResourceRequestParams, _: RequestContext) -> Result { + let result = match request.uri.as_str() { + DOCUMENT => self.doc_inspect(Parameters(DocIndex::default())).await?, + COMMANDS => self.command_list(Parameters(ListParams::default())).await?, + uri => return Err(McpError::resource_not_found(format!("unknown resource `{uri}`"), None)), + }; + let text = result.content.iter().filter_map(|c| c.as_text()).map(|t| t.text.as_str()).collect::>().join("\n"); + if result.is_error == Some(true) { + return Err(McpError::internal_error(text, None)); + } + let result: ReadResourceResult = serde_json::from_value(json!({"resultType":"complete", "ttlMs":0, "cacheScope":"private", "contents":[ + {"uri":request.uri, "mimeType":"application/json", "text":text} + ]})) + .map_err(|e| McpError::internal_error(e.to_string(), None))?; + Ok(result.into()) + } +} diff --git a/crates/automation/src/server/progress.rs b/crates/automation/src/server/progress.rs new file mode 100644 index 00000000..41827430 --- /dev/null +++ b/crates/automation/src/server/progress.rs @@ -0,0 +1,43 @@ +//! Render Video runs on the existing blocking worker; rmcp remains available for ping/cancel. +use super::*; +use rmcp::RoleServer; +use rmcp::model::ProgressNotificationParam; +use rmcp::service::RequestContext; +use std::time::{Duration, Instant}; + +impl PhotocraftMcp { + pub(super) async fn render_video_progress(&self, params: Value, context: RequestContext) -> CallToolResult { + let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel(); + // rmcp may drop the request future on cancellation. The blocking worker owns the + // token too, so it still stops and cleans up without needing that future to be polled. + let cancel = context.ct.clone(); + let job = self.headless_op(move |h| { + h.render_video(params, move |done, total| { + let _ = tx.send((done, total)); + !cancel.is_cancelled() + }) + }); + tokio::pin!(job); + let token = context.meta.get_progress_token(); + let (mut last, mut sent_at): (Option, Option) = (None, None); + let result = loop { + tokio::select! { + biased; + Some((done, total)) = rx.recv() => { + let due = done == total || sent_at.is_none_or(|t| t.elapsed() >= Duration::from_millis(100)); + if let Some(token) = token.clone().filter(|_| due && last.is_none_or(|n| done > n) && !context.ct.is_cancelled()) { + last = Some(done); + sent_at = Some(Instant::now()); + let _ = context.peer.notify_progress(ProgressNotificationParam::new(token, done as f64).with_total(total as f64)).await; + } + } + result = &mut job => break result, + } + }; + match result { + Some(Ok(value)) => ok_json(&value), + Some(Err(error)) => fail(error), + None => no_backend(), + } + } +} diff --git a/crates/automation/src/server/transport.rs b/crates/automation/src/server/transport.rs new file mode 100644 index 00000000..7cececd5 --- /dev/null +++ b/crates/automation/src/server/transport.rs @@ -0,0 +1,61 @@ +//! Recover malformed JSON lines before passing valid messages to rmcp. +use super::*; +use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; + +impl PhotocraftMcp { + /// Serve a JSON-lines stream. A malformed line receives -32700/id:null; the next line + /// is still handled. A single writer serializes parse errors and SDK responses. + pub async fn serve_io( + self, + input: impl tokio::io::AsyncRead + Unpin + Send + 'static, + output: impl tokio::io::AsyncWrite + Unpin + Send + 'static, + ) -> Result<(), AutomationError> { + let (mut to_service, service_in) = tokio::io::duplex(1 << 20); + let (service_out, from_service) = tokio::io::duplex(1 << 20); + let (errors, mut error_rx) = tokio::sync::mpsc::channel::(16); + let reader = tokio::spawn(async move { + let mut lines = BufReader::new(input).lines(); + while let Some(line) = lines.next_line().await? { + if line.trim().is_empty() { + continue; + } + match serde_json::from_str::(&line) { + Ok(_) => to_service.write_all(format!("{line}\n").as_bytes()).await?, + Err(e) => { + let reply = json!({"jsonrpc":"2.0", "id":null, "error":{"code":-32700, "message":format!("parse error: {e}")}}); + if errors.send(reply.to_string()).await.is_err() { + break; + } + } + } + } + to_service.shutdown().await + }); + let writer = tokio::spawn(async move { + let mut output = output; + let mut replies = BufReader::new(from_service).lines(); + loop { + let line = tokio::select! { + biased; + Some(error) = error_rx.recv() => error, + reply = replies.next_line() => match reply? { Some(line) => line, None => break }, + }; + output.write_all(format!("{line}\n").as_bytes()).await?; + output.flush().await?; + } + Ok::<_, std::io::Error>(()) + }); + let result = match self.serve((service_in, service_out)).await { + Ok(running) => running.waiting().await.map(|_| ()).map_err(|e| AutomationError::Other(e.to_string())), + Err(e) => Err(AutomationError::Other(format!("MCP init: {e}"))), + }; + reader.abort(); + match reader.await { + Ok(r) => r.map_err(|e| AutomationError::Io(e.to_string()))?, + Err(e) if !e.is_cancelled() => return Err(AutomationError::Other(e.to_string())), + Err(_) => {} + } + writer.await.map_err(|e| AutomationError::Other(e.to_string()))?.map_err(|e| AutomationError::Io(e.to_string()))?; + result + } +} diff --git a/crates/automation/src/workspace.rs b/crates/automation/src/workspace.rs index 3d4cd0bf..6d160bd6 100644 --- a/crates/automation/src/workspace.rs +++ b/crates/automation/src/workspace.rs @@ -282,6 +282,62 @@ fn command_error(id: &str) -> AutomationError { AutomationError::BadRequest(format!("automation command `{id}` uses ambient filesystem paths and is disabled; use capability-scoped document methods")) } +/// An export owns only files it created through this held directory capability. Refuse +/// collisions (including symlinks) so cancellation cannot delete or replace an earlier export. +pub(crate) struct ExportSequence { + dir: Dir, + created: Vec, +} + +impl AuthorizedWorkspace { + pub(crate) fn export_sequence(&self, path: &str) -> Result { + let relative = relative_path(path)?; + let root = self.write.as_ref().ok_or_else(|| AutomationError::BadRequest(format!("{DENIED}: write authority is absent")))?; + root.dir.create_dir_all(&relative).map_err(|e| file_error("mkdir", path, e))?; + let dir = root.dir.open_dir(&relative).map_err(|e| file_error("open export directory", path, e))?; + Ok(ExportSequence { dir, created: Vec::new() }) + } +} + +impl ExportSequence { + pub(crate) fn write(&mut self, name: &str, bytes: &[u8]) -> Result<(), AutomationError> { + let relative = relative_path(name)?; + if relative.components().count() != 1 { + return Err(path_error(name, "export filename must be a single component")); + } + let mut options = OpenOptions::new(); + options.write(true).create_new(true); + let mut file = self.dir.open_with(&relative, &options).map_err(|e| file_error("create export", name, e))?; + // Record immediately after creation, before any fallible write, including partial writes. + self.created.push(name.to_owned()); + file.write_all(bytes).and_then(|()| file.sync_all()).map_err(|e| file_error("write export", name, e)) + } + + pub(crate) fn commit(&mut self) { + self.created.clear(); + } + + pub(crate) fn cleanup(&mut self) -> Result<(), AutomationError> { + let mut failed = Vec::new(); + self.created.retain(|name| match self.dir.remove_file(name) { + Ok(()) => false, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => false, + Err(e) => { + failed.push(format!("{name}: {e}")); + true + } + }); + if failed.is_empty() { Ok(()) } else { Err(AutomationError::Io(format!("export cleanup failed: {}", failed.join("; ")))) } + } +} + +impl Drop for ExportSequence { + fn drop(&mut self) { + // Also cover an escaped rendering panic; normal errors report cleanup failures above. + let _ = self.cleanup(); + } +} + #[cfg(test)] mod tests { use super::*; @@ -297,6 +353,33 @@ mod tests { (inside, outside, workspace) } + #[test] + fn export_cleanup_owns_only_created_files_even_during_unwind() { + let (inside, _, workspace) = roots("export-cleanup"); + std::fs::create_dir(inside.join("frames")).unwrap(); + std::fs::write(inside.join("frames/clip_0009.png"), b"old").unwrap(); + let panic = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let mut export = workspace.export_sequence("frames").unwrap(); + export.write("clip_0000.png", b"new").unwrap(); + assert!(export.write("clip_0009.png", b"overwrite").is_err()); + assert!(export.write("../escape.png", b"escape").is_err()); + panic!("rendering panic"); + })); + assert!(panic.is_err()); + assert_eq!(std::fs::read_dir(inside.join("frames")).unwrap().count(), 1); + assert_eq!(std::fs::read(inside.join("frames/clip_0009.png")).unwrap(), b"old"); + assert!(AuthorizedWorkspace::default().export_sequence("frames").is_err()); + } + + #[cfg(unix)] + #[test] + fn export_directory_symlinks_cannot_escape_the_granted_root() { + let (inside, outside, workspace) = roots("export-symlink"); + std::os::unix::fs::symlink(&outside, inside.join("escape")).unwrap(); + assert!(workspace.export_sequence("escape").is_err()); + assert_eq!(std::fs::read_dir(outside).unwrap().count(), 0); + } + #[test] fn valid_read_and_nonexistent_output_write_stay_in_root() { let (inside, _, workspace) = roots("valid"); diff --git a/crates/automation/tests/mcp.rs b/crates/automation/tests/mcp.rs index 198134ba..46ab35d7 100644 --- a/crates/automation/tests/mcp.rs +++ b/crates/automation/tests/mcp.rs @@ -239,7 +239,7 @@ async fn errors_are_tool_errors_not_crashes() { let r = call(&client, "command_run", json!({"id": "no.such.command"})).await; assert_eq!(r.is_error, Some(true)); assert!(text(&r).contains("unknown command")); - let r = call(&client, "doc_inspect", json!({})).await; + let r = call(&client, "doc_inspect", json!({"index": 0})).await; assert_eq!(r.is_error, Some(true)); let r = call(&client, "doc_open", json!({"path": "/definitely/missing.png"})).await; assert_eq!(r.is_error, Some(true)); @@ -894,3 +894,61 @@ async fn command_batch_rejects_too_many_steps() { assert!(text(&r).contains("maximum is 256"), "{}", text(&r)); client.cancel().await.unwrap(); } + +#[tokio::test(flavor = "multi_thread")] +async fn convention_tools_have_titles_hints_and_strict_schemas() { + let client = connect(PhotocraftMcp::headless()).await; + let tools = client.list_all_tools().await.unwrap(); + assert!(tools.iter().any(|t| t.name == "render_preview")); + // The existing documented spelling remains listed. + assert!(tools.iter().any(|t| t.name == "doc_render_preview")); + for tool in tools { + let value = serde_json::to_value(&tool).unwrap(); + assert!(value["title"].as_str().is_some_and(|s| !s.is_empty()), "{value}"); + for hint in ["readOnlyHint", "destructiveHint", "idempotentHint", "openWorldHint"] { + assert!(value["annotations"][hint].is_boolean(), "{value}"); + } + assert_eq!(value["inputSchema"]["additionalProperties"], false, "{value}"); + } + client.cancel().await.unwrap(); +} + +#[tokio::test(flavor = "multi_thread")] +async fn convention_unknown_arguments_name_the_key_without_side_effects() { + let client = connect(PhotocraftMcp::headless()).await; + for (name, args) in [ + ("doc_new", json!({"widht": 8})), + ("session_list", json!({"surprise": true})), + ("command_run", json!({"id":"file.new", "surprise":true})), + ("command_batch", json!({"steps":[{"id":"file.new", "surprise":true}]})), + ] { + let key = if name == "doc_new" { "widht" } else { "surprise" }; + let result = client.call_tool(CallToolRequestParams::new(name).with_arguments(args.as_object().unwrap().clone())).await; + let error = result.expect_err("unknown key must fail before execution").to_string(); + assert!(error.contains("-32602") && error.contains(key), "{error}"); + } + assert_eq!(json_of(&call(&client, "session_list", json!({})).await)["documents"], json!([])); + client.cancel().await.unwrap(); +} + +#[tokio::test(flavor = "multi_thread")] +async fn convention_resources_are_live_json() { + let client = connect(PhotocraftMcp::headless()).await; + let resources = client.list_all_resources().await.unwrap(); + for uri in ["photocraft://document", "photocraft://commands"] { + assert!(resources.iter().any(|r| r.uri == uri)); + } + let read = async |uri: &str| { + let r = client.read_resource(rmcp::model::ReadResourceRequestParams::new(uri)).await.unwrap(); + let v = serde_json::to_value(r).unwrap(); + serde_json::from_str::(v["contents"][0]["text"].as_str().unwrap()).unwrap() + }; + assert!(read("photocraft://document").await["document"].is_null()); + json_of(&call(&client, "doc_new", json!({"width":8,"height":6})).await); + assert_eq!(read("photocraft://document").await["width"], 8); + assert_eq!(read("photocraft://commands").await, json_of(&call(&client, "command_list", json!({})).await)); + assert!(client.read_resource(rmcp::model::ReadResourceRequestParams::new("photocraft://missing")).await.is_err()); + let preview = call(&client, "render_preview", json!({"max_side":4})).await; + assert!(preview.content.iter().any(|c| c.as_image().is_some())); + client.cancel().await.unwrap(); +} diff --git a/crates/automation/tests/mcp_wire.rs b/crates/automation/tests/mcp_wire.rs new file mode 100644 index 00000000..a1cfcc15 --- /dev/null +++ b/crates/automation/tests/mcp_wire.rs @@ -0,0 +1,147 @@ +//! Exercise the actual line transport, including recovery and modern result fields. +use photocraft_automation::PhotocraftMcp; +use serde_json::{Value, json}; +use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; + +#[tokio::test(flavor = "multi_thread")] +async fn malformed_line_recovers_and_modern_lists_are_complete() { + let (mut input, server_in) = tokio::io::duplex(1 << 20); + let (server_out, output) = tokio::io::duplex(1 << 20); + let server = tokio::spawn(PhotocraftMcp::headless().serve_io(server_in, server_out)); + let mut lines = BufReader::new(output).lines(); + let init = json!({"jsonrpc":"2.0", "id":1, "method":"initialize", "params":{"protocolVersion":"2025-06-18", "capabilities":{}, "clientInfo":{"name":"test", "version":"1"}}}); + input.write_all(format!("{init}\n").as_bytes()).await.unwrap(); + lines.next_line().await.unwrap().unwrap(); + input.write_all(b"{\"jsonrpc\":\"2.0\",\"method\":\"notifications/initialized\"}\nBROKEN\n").await.unwrap(); + let error: Value = serde_json::from_str(&lines.next_line().await.unwrap().unwrap()).unwrap(); + assert_eq!(error["id"], Value::Null); + assert_eq!(error["error"]["code"], -32700); + let meta = json!({"io.modelcontextprotocol/protocolVersion":"2026-07-28", "io.modelcontextprotocol/clientInfo":{"name":"test", "version":"1"}, "io.modelcontextprotocol/clientCapabilities":{}}); + for (id, method, extra) in [(2, "tools/list", json!({})), (3, "resources/list", json!({})), (4, "resources/read", json!({"uri":"photocraft://document"}))] { + let mut params = extra; + params["_meta"] = meta.clone(); + input.write_all(format!("{}\n", json!({"jsonrpc":"2.0", "id":id, "method":method, "params":params})).as_bytes()).await.unwrap(); + let reply: Value = serde_json::from_str(&lines.next_line().await.unwrap().unwrap()).unwrap(); + assert_eq!(reply["id"], id); + assert_eq!(reply["result"]["resultType"], "complete", "{reply}"); + assert!(reply["result"]["ttlMs"].is_number()); + assert_eq!(reply["result"]["cacheScope"], "private"); + } + drop(input); + tokio::time::timeout(std::time::Duration::from_secs(5), server).await.unwrap().unwrap().unwrap(); +} + +#[tokio::test(flavor = "multi_thread")] +async fn video_export_progress_ping_cancel_and_scoped_cleanup() { + use photocraft_automation::AuthorizedWorkspace; + use std::time::Duration; + let root = std::env::temp_dir().join(format!("pc-conventions-video-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + let workspace = AuthorizedWorkspace::new(None, Some(&root)).unwrap(); + let (mut input, server_in) = tokio::io::duplex(1 << 20); + let (server_out, output) = tokio::io::duplex(1 << 20); + let server = tokio::spawn(PhotocraftMcp::headless_with_workspace(workspace).serve_io(server_in, server_out)); + let mut lines = BufReader::new(output).lines(); + macro_rules! send { + ($value:expr) => { + input.write_all(format!("{}\n", $value).as_bytes()).await.unwrap() + }; + } + macro_rules! next { + () => { + serde_json::from_str::(&tokio::time::timeout(Duration::from_secs(30), lines.next_line()).await.unwrap().unwrap().unwrap()).unwrap() + }; + } + let call = |id: u64, name: &str, args: Value| json!({"jsonrpc":"2.0", "id":id, "method":"tools/call", "params":{"name":name, "arguments":args}}); + send!( + json!({"jsonrpc":"2.0", "id":1, "method":"initialize", "params":{"protocolVersion":"2025-06-18", "capabilities":{}, "clientInfo":{"name":"test", "version":"1"}}}) + ); + next!(); + send!(json!({"jsonrpc":"2.0", "method":"notifications/initialized"})); + send!(call(2, "doc_new", json!({"width":64,"height":48,"name":"clip"}))); + assert_eq!(next!()["result"]["isError"], false); + send!(call(3, "command_run", json!({"id":"timeline.create", "params":{"duration":6,"fps":6}}))); + assert_eq!(next!()["result"]["isError"], false); + let render = |id, dir, format, token: Option| { + let mut value = call(id, "command_run", json!({"id":"file.export.renderVideo", "params":{"dir":dir,"format":format}})); + if let Some(token) = token { + value["params"]["_meta"] = json!({"progressToken":token}); + } + value + }; + for (id, dir, format, token) in [(4, "complete", "png", Some(json!("frames"))), (5, "quiet", "png", None), (6, "gif", "gif", Some(json!(77)))] { + send!(render(id, dir, format, token.clone())); + let mut progress = Vec::new(); + loop { + let reply = next!(); + if reply["id"] == id { + assert_eq!(reply["result"]["isError"], false, "{reply}"); + break; + } + assert!(token.is_some(), "no token: {reply}"); + assert_eq!(reply["params"]["progressToken"], token.clone().unwrap()); + assert_eq!(reply["params"]["total"].as_f64(), Some(6.0)); + progress.push(reply["params"]["progress"].as_f64().unwrap()); + } + if token.is_some() { + assert_eq!(progress.first(), Some(&0.0)); + assert_eq!(progress.last(), Some(&6.0)); + assert!(progress.windows(2).all(|w| w[0] < w[1])); + } + } + assert_eq!(std::fs::read_dir(root.join("complete")).unwrap().count(), 6); + assert!(photocraft_codecs::decode(&std::fs::read(root.join("gif/clip.gif")).unwrap()).is_ok()); + // A collision must preserve the earlier file and remove only frames from this failed run. + std::fs::create_dir(root.join("collision")).unwrap(); + std::fs::write(root.join("collision/clip_0001.png"), b"earlier export").unwrap(); + send!(render(7, "collision", "png", None)); + assert_eq!(next!()["result"]["isError"], true); + assert_eq!(std::fs::read_dir(root.join("collision")).unwrap().count(), 1); + assert_eq!(std::fs::read(root.join("collision/clip_0001.png")).unwrap(), b"earlier export"); + send!(render(8, "../outside", "png", None)); + assert_eq!(next!()["result"]["isError"], true); + send!(call(9, "doc_new", json!({"width":1600,"height":1200,"name":"clip"}))); + assert_eq!(next!()["result"]["isError"], false); + send!(call(10, "command_run", json!({"id":"timeline.create", "params":{"duration":100,"fps":30}}))); + assert_eq!(next!()["result"]["isError"], false); + std::fs::create_dir(root.join("cancel")).unwrap(); + std::fs::write(root.join("cancel/clip_9999.png"), b"unrelated").unwrap(); + send!(render(11, "cancel", "png", Some(json!(11)))); + let mut pinged = false; + loop { + let note = next!(); + assert!(note["id"].is_null(), "export finished early: {note}"); + if !pinged { + send!(json!({"jsonrpc":"2.0", "id":12, "method":"ping"})); + loop { + let reply = next!(); + assert_ne!(reply["id"], 11); + if reply["id"] == 12 { + break; + } + } + pinged = true; + } + if note["params"]["progress"].as_f64().unwrap_or(0.0) >= 1.0 { + break; + } + } + assert!(std::fs::read_dir(root.join("cancel")).unwrap().count() > 1); + send!(json!({"jsonrpc":"2.0", "method":"notifications/cancelled", "params":{"requestId":11}})); + // Inspect waits for the same session lock, so cleanup must have finished before its reply. + send!(call(13, "doc_inspect", json!({}))); + loop { + let reply = next!(); + assert_ne!(reply["id"], 11, "cancelled requests have no response: {reply}"); + if reply["id"] == 13 { + assert_eq!(reply["result"]["isError"], false); + break; + } + } + assert_eq!(std::fs::read_dir(root.join("cancel")).unwrap().count(), 1); + assert_eq!(std::fs::read(root.join("cancel/clip_9999.png")).unwrap(), b"unrelated"); + drop(input); + tokio::time::timeout(Duration::from_secs(5), server).await.unwrap().unwrap().unwrap(); + std::fs::remove_dir_all(root).unwrap(); +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 0401199d..6b39c779 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -96,6 +96,7 @@ pub mod type_styles_cmds; mod variables_cmds; pub mod vector_cmds; mod video_cmds; +pub use video_cmds::render_video_with; pub mod vp_cmds; pub mod warp_cmds; pub mod web_cmds; diff --git a/crates/engine/src/video_cmds.rs b/crates/engine/src/video_cmds.rs index 1a6c92be..250ab861 100644 --- a/crates/engine/src/video_cmds.rs +++ b/crates/engine/src/video_cmds.rs @@ -320,8 +320,20 @@ fn frames_to_layers(s: &mut Session, p: &Value) -> Result { /// sequence (`{dir}/{stem}_NNNN.{format}`). Clean-room; no proprietary codec. fn render_video(s: &mut Session, p: &Value) -> Result { let dir = p.get("dir").and_then(Value::as_str).ok_or_else(|| EngineError::BadParams { cmd: "file.export.renderVideo".into(), msg: "need `dir`".into() })?; - let format = p.get("format").and_then(Value::as_str).unwrap_or("png"); std::fs::create_dir_all(dir).map_err(|e| EngineError::Other(format!("mkdir `{dir}`: {e}")))?; + render_video_with(s, p, |name, bytes| crate::file_cmds::write_file(&format!("{dir}/{name}"), bytes), |_, _| true) +} + +/// Render through a caller-owned writer. The writer receives one filename, never an ambient +/// directory. Returning false from progress stops at a frame boundary; the caller owns cleanup. +pub fn render_video_with( + s: &Session, + p: &Value, + mut write: impl FnMut(&str, &[u8]) -> Result<()>, + mut progress: impl FnMut(usize, usize) -> bool, +) -> Result { + let dir = p.get("dir").and_then(Value::as_str).unwrap_or(""); + let format = p.get("format").and_then(Value::as_str).unwrap_or("png"); let st = s.active().ok_or(EngineError::NoDocument)?; let mut base = (*st.doc).clone(); store(&mut base); @@ -332,6 +344,11 @@ fn render_video(s: &mut Session, p: &Value) -> Result { let bounds = base.bounds(); let (w, h) = (bounds.width() as usize, bounds.height() as usize); + let cancelled = || EngineError::Other("render cancelled".into()); + if !progress(0, frames) { + return Err(cancelled()); + } + // Animated GIF: one file, every frame quantised to its own palette. if format.eq_ignore_ascii_case("gif") { use photocraft_algo::quantize::{self, Dither, Forced, PaletteKind}; @@ -349,10 +366,18 @@ fn render_video(s: &mut Session, p: &Value) -> Result { let pal = quantize::build_palette(&px, PaletteKind::Adaptive, 256, Forced::None).map_err(EngineError::Other)?; let idx = quantize::quantize(&mut px, w, &pal, Dither::None, 1.0, None); gframes.push(photocraft_codecs::web::GifFrame { indices: idx, palette: pal, transparent: None, delay_cs: delay }); + // The final report follows encoding/writing, so total means the output exists. + if !progress((f + 1).min(frames.saturating_sub(1)), frames) { + return Err(cancelled()); + } } let gif = photocraft_codecs::web::encode_gif_animated(w as u32, h as u32, &gframes, true).map_err(|e| EngineError::Other(e.to_string()))?; - let path = format!("{dir}/{stem}.gif"); - crate::file_cmds::write_file(&path, &gif)?; + let name = format!("{stem}.gif"); + write(&name, &gif)?; + if !progress(frames, frames) { + return Err(cancelled()); + } + let path = format!("{dir}/{name}"); return Ok(json!({"frames": frames, "file": path})); } @@ -365,10 +390,13 @@ fn render_video(s: &mut Session, p: &Value) -> Result { t.current = f; } sync(&mut doc); - let path = format!("{dir}/{stem}_{f:04}.{format}"); + let path = format!("{stem}_{f:04}.{format}"); let bytes = photocraft_io::export(&doc, format, &opts).map(|r| r.bytes).map_err(|e| EngineError::Other(format!("render frame {f}: {e}")))?; - crate::file_cmds::write_file(&path, &bytes)?; + write(&path, &bytes)?; files.push(path); + if !progress(f + 1, frames) { + return Err(cancelled()); + } } Ok(json!({"frames": files.len(), "dir": dir})) } diff --git a/crates/engine/src/video_cmds/tests.rs b/crates/engine/src/video_cmds/tests.rs index 7c597915..190aa306 100644 --- a/crates/engine/src/video_cmds/tests.rs +++ b/crates/engine/src/video_cmds/tests.rs @@ -174,3 +174,32 @@ fn edits_to_a_frame_survive_scrubbing_and_render() { let q = flat.pixel(2, 2); assert!(q[2] > 0.99 && q[0] < 0.01, "rendered frame 1 is blue: {q:?}"); } + +#[test] +fn render_writer_progress_and_cancellation_leave_session_unchanged() { + let mut s = session(); + s.execute("timeline.create", json!({"duration": 4, "fps": 4})).unwrap(); + let before = s.active().unwrap().doc.clone(); + for format in ["png", "gif"] { + let mut names = Vec::new(); + let mut progress = Vec::new(); + let r = render_video_with( + &s, + &json!({"format":format}), + |name, bytes| { + assert!(!bytes.is_empty()); + names.push(name.to_owned()); + Ok(()) + }, + |done, total| { + progress.push((done, total)); + done < 2 + }, + ); + assert!(r.unwrap_err().to_string().contains("cancelled")); + assert!(names.len() < 4); + assert_eq!(progress.first(), Some(&(0, 4))); + assert_eq!(progress.last(), Some(&(2, 4))); + assert_eq!(s.active().unwrap().doc.timeline, before.timeline); + } +} diff --git a/docs/mcp.md b/docs/mcp.md new file mode 100644 index 00000000..523aa7a8 --- /dev/null +++ b/docs/mcp.md @@ -0,0 +1,64 @@ +# MCP conventions + +PhotoCraft follows the same conventions as FilmCraft #28. Start a headless session: + +```sh +photocraft-cli mcp --automation-read-root /work/project --automation-write-root /work/project +``` + +Paths are relative to the corresponding root; without a root that 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. + +## Tools and resources + +- `command_list`: discover engine ids, parameter docs and current enabled state; accepts + `filter` and `enabled_only`. +- `command_run`: `id`, optional `params` and `wait`. Command parameters remain engine-defined. +- `command_batch`: ordered `steps`, optional `stop_on_error`; one history step per command, + not an atomic transaction. Existing per-step results and reply budgets are preserved. +- `doc_inspect`: layer tree, history and selection; an empty session returns `document:null`. +- `render_preview`: bounded PNG content; `index` and `max_side` (default 1024, maximum 2048). +- `ui_inspect` and `ui_screenshot`: live app state/window in bridge mode. + +Existing document/session/job/UI tools remain listed, including the documented +`doc_render_preview` spelling. There are no hidden legacy aliases. Every tool has a title +and all four annotation hints. Generic command/control tools conservatively advertise writes; +file saves may replace existing targets. Hints do not grant permission. + +Unknown tool arguments return JSON-RPC `-32602`, naming the key, before execution. Batch +step envelopes are strict too. Nested engine `params` retain their command-specific handling. +Tool execution failures return `isError:true`. A worker panic becomes a tool error and the +existing poisoned-session recovery keeps serving. Invalid JSON lines receive `-32700` with +`id:null`; the next line is still processed. + +`photocraft://document` and `photocraft://commands` return live JSON matching `doc_inspect` +and `command_list`. Resource reads are uncached; tool/resource catalogs are private and cached +for ten minutes. List/read responses include the MCP 2026-07-28 result/cache fields. + +## Progress and cancellation + +A direct headless `command_run` of `file.export.renderVideo` reports frame counts when +`_meta.progressToken` is present (a string or number). Notifications increase strictly, +are throttled to at most ten per second except completion, and include `total`. No token +means no progress notifications. Ping remains responsive while rendering. + +```json +{"jsonrpc":"2.0","id":20,"method":"tools/call","params":{"name":"command_run","arguments":{"id":"file.export.renderVideo","params":{"dir":"frames","format":"png"}},"_meta":{"progressToken":"export-20"}}} +{"jsonrpc":"2.0","method":"notifications/cancelled","params":{"requestId":20}} +``` + +Create a document and timeline first (`doc_new`, then `timeline.create`). Export uses the +configured write root and creates `dir` beneath it. Filenames must be single components; +absolute paths, traversal and escaping symlinks are rejected. Existing target filenames +are refused, including symlinks, so a cancelled or failed job cannot replace an earlier export. +Only files actually created by this job are removed on cancellation, error or an escaped panic; +unrelated files and the destination directory remain. PNG sequences and animated GIF share +the engine renderer. The document and playhead are unchanged. + +Cancellation suppresses the request response and stops at a frame boundary (the current +frame/encoding may finish first). Unknown request ids are ignored. Render Video stays a +synchronous command even with `wait:false`; other job-capable commands keep their existing +`jobs_list`/`jobs_cancel` behavior. Batch exports do not emit MCP progress or support per-step +MCP cancellation. Nested engine actions and bridge Render Video remain denied by the existing +ambient-filesystem policy; the new scoped writer is available for direct headless commands.