diff --git a/apps/lightcraft-cli/src/main.rs b/apps/lightcraft-cli/src/main.rs index f8ddf550..7c9d255e 100644 --- a/apps/lightcraft-cli/src/main.rs +++ b/apps/lightcraft-cli/src/main.rs @@ -342,7 +342,7 @@ fn mcp(args: &[String]) -> Result<(), String> { eprintln!("lightcraft-cli mcp: serving MCP on stdio ({})", backend.describe()); let mut server = Server::new(backend).with_command_tools(!compact); let stdin = std::io::stdin(); - server.serve(BufReader::new(stdin.lock()), std::io::stdout().lock()).map_err(|e| e.to_string()) + server.serve(BufReader::new(stdin), std::io::stdout()).map_err(|e| e.to_string()) } fn merge(args: &[String]) -> Result<(), String> { diff --git a/crates/mcp/src/backend.rs b/crates/mcp/src/backend.rs index 5db2aaaa..4772363b 100644 --- a/crates/mcp/src/backend.rs +++ b/crates/mcp/src/backend.rs @@ -15,8 +15,13 @@ pub trait Backend { fn has_ui(&self) -> bool; /// Short human description ("headless", "remote 127.0.0.1:7980"). fn describe(&self) -> String; + /// Progress/cancellation for the next headless batch export. Other backends ignore it. + fn set_progress(&mut self, _hook: Option) {} } +/// `(completed photos, total photos, next filename)`; false stops before the next photo. +pub type ProgressHook = Box bool + Send>; + /// A running LightCraft app, reached through its loopback control port. pub struct Remote { addr: String, diff --git a/crates/mcp/src/headless.rs b/crates/mcp/src/headless.rs index fdf13adf..974de023 100644 --- a/crates/mcp/src/headless.rs +++ b/crates/mcp/src/headless.rs @@ -8,7 +8,7 @@ use lightcraft_engine::catalog::PhotoId; use lightcraft_raster::Rgba8; use serde_json::{Value, json}; -use crate::backend::Backend; +use crate::backend::{Backend, ProgressHook}; /// File extensions recognised as photos when expanding folders. pub const PHOTO_EXTENSIONS: &[&str] = &[ @@ -20,6 +20,7 @@ pub const PHOTO_EXTENSIONS: &[&str] = &[ /// Headless backend: a [`Session`] with filesystem hooks. pub struct Headless { pub session: Session, + progress: Option, } impl Drop for Headless { @@ -37,7 +38,7 @@ impl Default for Headless { impl Headless { pub fn new(session: Session) -> Self { - Self { session } + Self { session, progress: None } } /// A headless session with the procedurally generated demo library. @@ -60,7 +61,7 @@ impl Headless { /// (see `lightcraft_engine::export::ExportOptions::from_json`, plus `ids`, `dir`, `path`). /// With `path` and no `format`, the format follows the path's extension. fn export(&mut self, p: &Value) -> Result { - use lightcraft_engine::export::{Destination, ExportFormat, ExportOptions, Resize, export_batch}; + use lightcraft_engine::export::{Destination, ExportFormat, ExportOptions, Resize, prepare_batch, run_batch}; let p = &self.session.export_params(p)?; let mut opts = ExportOptions::from_params(p).map_err(|e| e.to_string())?; if !ExportOptions::has_size_param(p) { @@ -82,10 +83,28 @@ impl Headless { }; let dir = p.get("dir").and_then(Value::as_str).unwrap_or(""); let write = &mut lightcraft_engine::export::write_file; - let files = - export_batch(&mut self.session, &ids, &opts, &Destination { dir: dir.to_string(), exact: exact.map(str::to_string) }, write, &|path| { - Path::new(path).exists() - })?; + let items = prepare_batch(&mut self.session, &ids, &opts)?; + let total = items.len(); + let mut hook = self.progress.take(); + let mut cancelled = false; + let files = run_batch( + items, + &opts, + &Destination { dir: dir.to_string(), exact: exact.map(str::to_string) }, + write, + &|path| Path::new(path).exists(), + true, + &mut |done, name| { + cancelled = hook.as_mut().is_some_and(|h| !h(done, total, name)); + !cancelled + }, + )?; + if !cancelled && let Some(hook) = hook.as_mut() { + cancelled = !hook(total, total, ""); + } + if cancelled { + return Err("cancelled".into()); + } // Single-photo exports also report path/width/height at the top level (back-compat). let mut out = files.first().cloned().unwrap_or_else(|| json!({})); out["files"] = json!(files); @@ -94,6 +113,10 @@ impl Headless { } impl Backend for Headless { + fn set_progress(&mut self, hook: Option) { + self.progress = hook; + } + fn call(&mut self, method: &str, params: Value) -> Result { let p = if params.is_null() { json!({}) } else { params }; match method { diff --git a/crates/mcp/src/lib.rs b/crates/mcp/src/lib.rs index a1fb5510..b2417fe0 100644 --- a/crates/mcp/src/lib.rs +++ b/crates/mcp/src/lib.rs @@ -21,7 +21,7 @@ mod headless; mod server; mod tools; -pub use backend::{Backend, Remote}; +pub use backend::{Backend, ProgressHook, Remote}; pub use base64::{base64_decode, base64_encode}; pub use headless::{Headless, PHOTO_EXTENSIONS, encode_image, expand_paths, write_image}; pub use server::{PROTOCOL_VERSION, Server}; diff --git a/crates/mcp/src/progress.rs b/crates/mcp/src/progress.rs new file mode 100644 index 00000000..2043dfd9 --- /dev/null +++ b/crates/mcp/src/progress.rs @@ -0,0 +1,165 @@ +//! Stdio progress and cancellation between photos. The backend keeps its session on the +//! serving thread; only stdin and stdout move to transport threads. Other calls wait in order. + +use std::collections::VecDeque; +use std::io::{BufRead, Write}; +use std::sync::atomic::Ordering; +use std::sync::mpsc::{Receiver, Sender, TryRecvError}; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant}; + +use serde_json::{Value, json}; + +use super::{Server, response}; +use crate::backend::ProgressHook; + +type Input = Option>; + +#[derive(Clone)] +pub(super) struct Wire { + inbox: Arc>>, + out: Sender, + deferred: Arc>>, +} + +fn lock(m: &Mutex) -> std::sync::MutexGuard<'_, T> { + m.lock().unwrap_or_else(std::sync::PoisonError::into_inner) +} + +impl Server { + pub(super) fn serve_wire(&mut self, input: impl BufRead + Send, output: impl Write + Send) -> std::io::Result<()> { + std::thread::scope(|scope| { + let (in_tx, in_rx) = std::sync::mpsc::channel::(); + let (out_tx, out_rx) = std::sync::mpsc::channel::(); + let wire = Wire { inbox: Arc::new(Mutex::new(in_rx)), out: out_tx, deferred: Arc::new(Mutex::new(VecDeque::new())) }; + let writer = std::thread::Builder::new().name("mcp-output".into()).spawn_scoped(scope, move || -> std::io::Result<()> { + let mut output = output; + for line in out_rx { + output.write_all(line.as_bytes())?; + output.write_all(b"\n")?; + output.flush()?; + } + Ok(()) + })?; + let reader = match std::thread::Builder::new().name("mcp-input".into()).spawn_scoped(scope, move || { + for line in input.lines() { + let failed = line.is_err(); + if in_tx.send(Some(line)).is_err() || failed { + return; + } + } + let _ = in_tx.send(None); + }) { + Ok(reader) => reader, + Err(error) => { + drop(wire); + let _ = writer.join(); + return Err(error); + } + }; + self.wire = Some(wire.clone()); + let served = (|| -> std::io::Result<()> { + loop { + let queued = lock(&wire.deferred).pop_front(); + let line = match queued { + Some(line) => line, + None => lock(&wire.inbox).recv().ok().flatten(), + }; + let Some(line) = line else { return Ok(()) }; + if let Some(reply) = self.handle_line(&line?) { + wire.out.send(reply).map_err(|_| std::io::Error::new(std::io::ErrorKind::BrokenPipe, "MCP output closed"))?; + } + } + })(); + self.wire = None; + drop(wire); + let written = writer.join().map_err(|_| std::io::Error::other("MCP output thread panicked")).and_then(|r| r); + let read = reader.join().map_err(|_| std::io::Error::other("MCP input thread panicked")); + served.and(written).and(read) + }) + } + + /// Called at photo boundaries, including completion. EOF finishes the pending export; + /// cancellation stops before the next photo. No file scan or output deletion is needed: + /// the existing writer replaces each whole file atomically. + pub(super) fn progress_hook(&self) -> Option { + let wire = self.wire.clone()?; + let (id, token) = self.current.clone()?; + let suppress = self.suppress.clone(); + let mut closed = false; + let mut last = None; + let mut reported: Option = None; + Some(Box::new(move |done, total, name| { + while !closed { + let next = lock(&wire.inbox).try_recv(); + let line = match next { + Ok(Some(Ok(line))) => line, + Ok(Some(Err(error))) => { + lock(&wire.deferred).push_back(Some(Err(error))); + closed = true; + break; + } + Ok(None) | Err(TryRecvError::Disconnected) => { + lock(&wire.deferred).push_back(None); + closed = true; + break; + } + Err(TryRecvError::Empty) => break, + }; + let msg: Value = serde_json::from_str(&line).unwrap_or(Value::Null); + match msg.get("method").and_then(Value::as_str) { + Some("notifications/cancelled") if msg.get("id").is_none() && msg["params"]["requestId"] == id => { + suppress.store(true, Ordering::Relaxed); + } + Some("ping") + if msg.get("id").is_some_and(|i| i.is_string() || i.is_number()) + && msg.get("params").is_none_or(|p| p.is_null() || p.is_object()) => + { + let _ = wire.out.send(response(msg["id"].clone(), json!({})).to_string()); + } + _ => lock(&wire.deferred).push_back(Some(Ok(line))), + } + } + if suppress.load(Ordering::Relaxed) { + return false; + } + if let Some(token) = &token + && total > 0 + && last.is_none_or(|last| done > last) + && (done == total || reported.is_none_or(|at| at.elapsed() >= Duration::from_millis(100))) + { + last = Some(done); + reported = Some(Instant::now()); + let message = if done < total { + format!("Exporting {name} ({} of {total})", done.saturating_add(1)) + } else { + format!("Exported {total} photo(s)") + }; + let notification = json!({"jsonrpc":"2.0","method":"notifications/progress", + "params":{"progressToken":token,"progress":done,"total":total,"message":message}}); + let _ = wire.out.send(notification.to_string()); + } + true + })) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn transport_lock_recovers_after_panic() { + let state = Arc::new(Mutex::new(VecDeque::new())); + let held = state.clone(); + let _ = std::thread::spawn(move || { + held.lock().unwrap().push_back(7); + let _guard = held.lock().unwrap(); + panic!("synthetic transport panic"); + }) + .join(); + assert_eq!(lock(&state).pop_front(), Some(7)); + lock(&state).push_back(8); + assert_eq!(lock(&state).pop_front(), Some(8)); + } +} diff --git a/crates/mcp/src/server.rs b/crates/mcp/src/server.rs index c3371be5..ba64ef45 100644 --- a/crates/mcp/src/server.rs +++ b/crates/mcp/src/server.rs @@ -1,11 +1,16 @@ //! JSON-RPC 2.0 framing and the MCP lifecycle / tools / resources methods. use std::io::{BufRead, Write}; +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; + +#[path = "progress.rs"] +mod progress; use serde_json::{Value, json}; use crate::backend::Backend; -use crate::tools::{call_tool, tool_definitions}; +use crate::tools::{ToolResult, call_tool, check_args, doc_inspect, tool_definitions}; /// The MCP revision we implement. pub const PROTOCOL_VERSION: &str = "2025-06-18"; @@ -22,11 +27,13 @@ const RESOURCE_NOT_FOUND: i64 = -32002; const INSTRUCTIONS: &str = "LightCraft is a non-destructive photo library and raw developer (a Lightroom alternative). \ Photos have integer ids; most editing acts on the *active* photo (tools that take `id` make that photo active first). \ Typical loop: query_photos → select_photos (or pass id) → list_controls → set_develop {values: {\"light.exposure\": 0.5}} → \ -render_photo to look at the result → export. Positions (crop rects, mask points, brush strokes) are normalized image \ -coordinates 0..1 with the origin at the top-left. Every action is a command: list_commands shows ids and parameter \ -docs; run_command runs any of them. Edits are undoable (undo / redo)."; +render_preview to look at the result → export. Positions (crop rects, mask points, brush strokes) are normalized image \ +coordinates 0..1 with the origin at the top-left. Every action is a command: command_list shows ids and parameter \ +docs; command_run {id, params} runs any of them; command_batch runs steps in order. Inspect with doc_inspect. Edits are undoable (undo / redo)."; /// Resource URIs. +pub const DOCUMENT_URI: &str = "lightcraft://document"; +pub const COMMANDS_URI: &str = "lightcraft://commands"; pub const LIBRARY_URI: &str = "lightcraft://library"; pub const PHOTOS_URI: &str = "lightcraft://photos"; pub const PHOTO_URI: &str = "lightcraft://photo/active"; @@ -38,6 +45,9 @@ pub struct Server { backend: Box, initialized: bool, command_tools: bool, + wire: Option, + current: Option<(Value, Option)>, + suppress: Arc, } fn response(id: Value, result: Value) -> Value { @@ -54,7 +64,7 @@ fn resource(uri: &str, name: &str, title: &str, description: &str) -> Value { impl Server { pub fn new(backend: Box) -> Self { - Self { backend, initialized: false, command_tools: true } + Self { backend, initialized: false, command_tools: true, wire: None, current: None, suppress: Arc::new(AtomicBool::new(false)) } } /// Whether `tools/list` includes one `cmd_*` tool per registered command (default true). @@ -75,16 +85,8 @@ impl Server { /// Serve newline-delimited JSON-RPC until `input` closes. Logs go to stderr only (stdout is /// the protocol stream). - pub fn serve(&mut self, input: impl BufRead, mut output: impl Write) -> std::io::Result<()> { - for line in input.lines() { - let line = line?; - if let Some(reply) = self.handle_line(&line) { - output.write_all(reply.as_bytes())?; - output.write_all(b"\n")?; - output.flush()?; - } - } - Ok(()) + pub fn serve(&mut self, input: impl BufRead + Send, output: impl Write + Send) -> std::io::Result<()> { + self.serve_wire(input, output) } /// Handle one line; returns the reply line (None for notifications and blank lines). @@ -131,8 +133,19 @@ impl Server { if !(params.is_object() || params.is_null()) { return Some(error(id, INVALID_PARAMS, "`params` must be an object")); } - Some(match self.request(method, ¶ms) { - Ok(r) => response(id, r), + let token = params.get("_meta").and_then(|m| m.get("progressToken")).filter(|t| t.is_string() || t.is_number()).cloned(); + self.current = Some((id.clone(), token)); + self.suppress.store(false, Ordering::Relaxed); + let result = self.request(method, ¶ms); + self.current = None; + if self.suppress.swap(false, Ordering::Relaxed) { + return None; + } + Some(match result { + Ok(mut r) => { + modern_result_fields(method, ¶ms, &mut r); + response(id, r) + } Err((code, m)) => error(id, code, m), }) } @@ -158,13 +171,33 @@ impl Server { })) } "ping" => Ok(json!({})), - "tools/list" => Ok(json!({"tools": tool_definitions(self.backend.as_mut(), self.command_tools)})), + "tools/list" => { + let command_tools = self.command_tools; + let tools = self.guarded(|b| tool_definitions(b, command_tools)).map_err(|e| (INTERNAL_ERROR, e))?; + Ok(json!({"tools": tools})) + } "tools/call" => { let name = params.get("name").and_then(Value::as_str).ok_or((INVALID_PARAMS, "missing tool `name`".to_string()))?; let args = params.get("arguments").cloned().unwrap_or(Value::Null); - Ok(call_tool(self.backend.as_mut(), name, &args).to_value()) + check_args(name, &args).map_err(|e| (INVALID_PARAMS, e))?; + let long = matches!(name, "export" | "cmd_app_export") + || (name == "command_run" && args["id"] == "app.export") + || (name == "run_command" && args["command"] == "app.export"); + let hook = if long && !self.backend.has_ui() { self.progress_hook() } else { None }; + let result = self.guarded(|b| { + b.set_progress(hook); + call_tool(b, name, &args) + }); + let clear = self.guarded(|b| b.set_progress(None)); + let result = match (result, clear) { + (Ok(result), Ok(())) => result, + (Err(error), _) | (_, Err(error)) => ToolResult::error(error), + }; + Ok(result.to_value()) } "resources/list" => Ok(json!({"resources": [ + resource(DOCUMENT_URI, "document", "Library document", "Library state and counts (same as doc_inspect)"), + resource(COMMANDS_URI, "commands", "Command catalog", "Every command (same as command_list)"), resource(LIBRARY_URI, "library", "Library state", "Current source, filter, sort, selection, active photo/mask and undo/redo labels (library.state)"), resource(PHOTOS_URI, "photos", "Photos in view", "The photos in the current view with ids, ratings, flags and metadata (catalog.query)"), resource(PHOTO_URI, "active-photo", "Active photo", "Everything about the active photo: file, metadata, develop settings, albums, history (photo.inspect)"), @@ -175,6 +208,7 @@ impl Server { "resources/read" => { let uri = params.get("uri").and_then(Value::as_str).ok_or((INVALID_PARAMS, "missing `uri`".to_string()))?; let cmd = match uri { + DOCUMENT_URI | COMMANDS_URI => "", LIBRARY_URI => "library.state", PHOTOS_URI => "catalog.query", PHOTO_URI => "photo.inspect", @@ -182,10 +216,47 @@ impl Server { CONTROLS_URI => "develop.controls", _ => return Err((RESOURCE_NOT_FOUND, format!("resource not found: {uri}"))), }; - let v = self.backend.call("engine.execute", json!({"command": cmd, "params": {}})).map_err(|e| (INTERNAL_ERROR, e))?; + let v = self + .guarded(|b| match uri { + DOCUMENT_URI => doc_inspect(b), + COMMANDS_URI => b.call("engine.commands", json!({})), + _ => b.call("engine.execute", json!({"command": cmd, "params": {}})), + }) + .and_then(|v| v) + .map_err(|e| (INTERNAL_ERROR, e))?; Ok(json!({"contents": [{"uri": uri, "mimeType": "application/json", "text": serde_json::to_string_pretty(&v).unwrap_or_default()}]})) } other => Err((METHOD_NOT_FOUND, format!("method not found: {other}"))), } } } + +impl Server { + /// Escaped backend panics become errors; subsequent requests can keep using the session. + fn guarded(&mut self, f: impl FnOnce(&mut dyn Backend) -> T) -> Result { + std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(self.backend.as_mut()))).map_err(|p| { + let message = + p.downcast_ref::<&str>().map(|s| s.to_string()).or_else(|| p.downcast_ref::().cloned()).unwrap_or_else(|| "panic".into()); + format!("internal error: {message}") + }) + } +} + +/// Per-request protocol metadata leaves initialize-based clients' result shapes unchanged. +fn modern_result_fields(method: &str, params: &Value, result: &mut Value) { + let modern = params.pointer("/_meta/io.modelcontextprotocol~1protocolVersion").and_then(Value::as_str).is_some_and(|v| v >= "2026-07-28"); + let ttl = match method { + "tools/list" | "resources/list" | "resources/templates/list" => Some(600_000), + "resources/read" => Some(0), + "tools/call" => None, + _ => return, + }; + let Some(obj) = result.as_object_mut().filter(|_| modern) else { + return; + }; + obj.insert("resultType".into(), json!("complete")); + if let Some(ttl) = ttl { + obj.insert("ttlMs".into(), json!(ttl)); + obj.insert("cacheScope".into(), json!("private")); + } +} diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index ee17e1ad..3393e504 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -139,6 +139,19 @@ fn export_refuses_unknown_params_and_bad_values() { assert!(e.contains("did you mean `quality`"), "{e}"); } +/// Every option the `export` tool advertises reaches `app.export` (they were accepted, then dropped). +#[test] +fn export_tool_forwards_its_advertised_options() { + let mut b = Headless::demo(); + let base = std::env::temp_dir().join(format!("lc-mcp-export-fwd-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + let r = call_tool(&mut b, "export", &json!({"dir": base.to_string_lossy(), "subfolder": "sub", "longEdge": 64, "format": "png"})); + assert!(!r.is_error, "{r:?}"); + let files: Vec<_> = std::fs::read_dir(base.join("sub")).unwrap().collect(); + assert_eq!(files.len(), 1, "written into the subfolder"); + let _ = std::fs::remove_dir_all(&base); +} + /// The `import` helper moves: renamed into the folder template, the source removed. #[test] fn import_tool_moves_with_a_folder_template() { @@ -215,7 +228,7 @@ fn resources() { let mut s = server(); let r = rpc(&mut s, 1, "resources/list", json!({})); let list = r["result"]["resources"].as_array().unwrap(); - assert_eq!(list.len(), 5); + assert_eq!(list.len(), 7); for (i, res) in list.iter().enumerate() { let uri = res["uri"].as_str().unwrap(); let r = rpc(&mut s, 10 + i as u64, "resources/read", json!({"uri": uri})); @@ -245,3 +258,116 @@ fn select_photos_rejects_unknown_ids_and_keeps_the_active_photo() { assert!(r.is_error, "{r:?}"); assert_eq!(b.session.active(), Some(first)); } + +#[test] +fn core_tools_have_hints_and_reject_unknown_keys() { + for compact in [false, true] { + let mut s = server().with_command_tools(!compact); + let list = rpc(&mut s, 1, "tools/list", json!({})); + let tools = list["result"]["tools"].as_array().unwrap(); + for name in ["command_list", "command_run", "command_batch", "doc_inspect", "render_preview", "list_commands", "run_command", "render_photo"] + { + assert!(tools.iter().any(|t| t["name"] == name), "missing {name}"); + } + for t in tools { + assert!(!t["title"].as_str().unwrap().is_empty()); + for hint in ["readOnlyHint", "destructiveHint", "idempotentHint", "openWorldHint"] { + assert!(t["annotations"][hint].is_boolean(), "{t}"); + } + } + for name in ["command_list", "command_run", "command_batch", "doc_inspect", "render_preview", "query_photos", "run_command"] { + let r = rpc(&mut s, 2, "tools/call", json!({"name":name,"arguments":{"misspelled":true}})); + assert_eq!(r["error"]["code"], -32602, "{r}"); + assert!(r["error"]["message"].as_str().unwrap().contains("misspelled"), "{r}"); + } + let preview = tools.iter().find(|t| t["name"] == "render_preview").unwrap(); + assert_eq!(preview["annotations"]["readOnlyHint"], true); + assert!(preview["inputSchema"]["properties"].get("path").is_none()); + let writer = tools.iter().find(|t| t["name"] == "render_photo").unwrap(); + assert_eq!(writer["annotations"]["readOnlyHint"], false); + let export = tools.iter().find(|t| t["name"] == "export").unwrap(); + for key in lightcraft_engine::export::OPTION_PARAMS.iter().chain(lightcraft_engine::export::TARGET_PARAMS) { + assert!(export["inputSchema"]["properties"].get(*key).is_some(), "missing export argument {key}"); + } + let before = rpc(&mut s, 2, "resources/read", json!({"uri":"lightcraft://library"})); + let image = rpc(&mut s, 2, "tools/call", json!({"name":"render_preview","arguments":{"max_side":64}})); + assert_eq!(image["result"]["isError"], false, "{image}"); + assert!(image["result"]["content"].as_array().unwrap().iter().any(|c| c["type"] == "image")); + assert_eq!(before, rpc(&mut s, 2, "resources/read", json!({"uri":"lightcraft://library"}))); + let batch = json!({"steps":[{"id":"catalog.stats"},{"id":"no.such"},{"id":"catalog.stats"}],"stop_on_error":false}); + let r = rpc(&mut s, 3, "tools/call", json!({"name":"command_batch","arguments":batch})); + assert_eq!(r["result"]["isError"], true); + assert_eq!(r["result"]["structuredContent"]["completed"], 2); + assert_eq!(r["result"]["structuredContent"]["failed"], 1); + let r = rpc(&mut s, 4, "tools/call", json!({"name":"command_batch","arguments":{"steps":[{"id":"no.such"},{"id":"catalog.stats"}]}})); + assert_eq!(r["result"]["structuredContent"]["completed"], 0); + assert_eq!(r["result"]["structuredContent"]["results"].as_array().unwrap().len(), 1); + } +} + +#[test] +fn core_resources_match_tools_after_parse_error() { + let mut s = server(); + let bad: Value = serde_json::from_str(&s.handle_line("{bad").unwrap()).unwrap(); + assert_eq!(bad["error"]["code"], -32700); + assert!(bad["id"].is_null()); + let list = rpc(&mut s, 1, "resources/list", json!({})); + for (uri, tool) in [("lightcraft://document", "doc_inspect"), ("lightcraft://commands", "command_list")] { + assert!(list["result"]["resources"].as_array().unwrap().iter().any(|r| r["uri"] == uri)); + let resource = rpc(&mut s, 2, "resources/read", json!({"uri":uri})); + let call = rpc(&mut s, 3, "tools/call", json!({"name":tool,"arguments":{}})); + let a: Value = serde_json::from_str(resource["result"]["contents"][0]["text"].as_str().unwrap()).unwrap(); + let b: Value = serde_json::from_str(call["result"]["content"][0]["text"].as_str().unwrap()).unwrap(); + assert_eq!(a, b); + } + let old = rpc(&mut s, 4, "resources/read", json!({"uri":"lightcraft://library"})); + assert!(old["result"]["contents"].is_array(), "{old}"); +} + +#[test] +fn escaped_tool_panic_is_an_error_and_session_survives() { + struct Faulty { + inner: Headless, + fail: bool, + } + impl Backend for Faulty { + fn call(&mut self, method: &str, params: Value) -> Result { + if method == "engine.execute" && params["command"] == "catalog.stats" && std::mem::take(&mut self.fail) { + panic!("synthetic backend panic"); + } + self.inner.call(method, params) + } + fn has_ui(&self) -> bool { + false + } + fn describe(&self) -> String { + "test".into() + } + } + let mut s = Server::new(Box::new(Faulty { inner: Headless::demo(), fail: true })); + let params = json!({"name":"run_command","arguments":{"command":"catalog.stats"}}); + let r = rpc(&mut s, 1, "tools/call", params.clone()); + assert_eq!(r["result"]["isError"], true, "{r}"); + assert!(r.to_string().contains("synthetic backend panic")); + assert_eq!(rpc(&mut s, 2, "tools/call", params)["result"]["isError"], false); + assert!(rpc(&mut s, 3, "ping", json!({}))["result"].is_object()); +} + +#[test] +fn modern_requests_receive_result_and_cache_fields() { + let mut s = server(); + let meta = json!({"io.modelcontextprotocol/protocolVersion":"2026-07-28"}); + for (method, mut params) in [ + ("tools/list", json!({})), + ("resources/list", json!({})), + ("resources/templates/list", json!({})), + ("resources/read", json!({"uri":"lightcraft://library"})), + ] { + params["_meta"] = meta.clone(); + let r = rpc(&mut s, 1, method, params); + assert_eq!(r["result"]["resultType"], "complete", "{r}"); + assert_eq!(r["result"]["cacheScope"], "private"); + assert_eq!(r["result"]["ttlMs"], if method == "resources/read" { 0 } else { 600_000 }); + } + assert!(rpc(&mut s, 2, "tools/list", json!({}))["result"].get("resultType").is_none()); +} diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 1b41d86a..4a114f18 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -66,7 +66,7 @@ fn tool(name: &str, title: &str, description: &str, properties: Value, required: "name": name, "title": title, "description": description, - "inputSchema": {"type": "object", "properties": properties, "required": required}, + "inputSchema": {"type": "object", "properties": properties, "required": required, "additionalProperties": false}, }) } @@ -78,6 +78,51 @@ fn ids_schema(what: &str) -> Value { pub fn helper_tools(has_ui: bool) -> Vec { let photo_id = json!({"type": "integer", "description": "Photo id (default: the active photo). Makes that photo active first."}); let mut v = vec![ + tool( + "command_list", + "List commands", + "Every command (engine + UI when connected) with id, label, menu, shortcut, parameter doc and whether it is enabled now. Each id is also callable as tool `cmd_` or via command_run.", + json!({ + "filter": {"type": "string", "description": "Only commands whose id or label contains this text (case-insensitive)"}, + "enabled_only": {"type": "boolean", "description": "Only commands that are enabled now"} + }), + &[], + ), + tool( + "command_run", + "Run command", + "Run any LightCraft command by id with JSON params (see command_list for ids and parameter docs), e.g. {id: \"photo.rate\", params: {rating: 4}}.", + json!({"id": {"type": "string", "description": "Command id"}, "params": {"type": "object", "description": "Command parameters", "additionalProperties": true}}), + &["id"], + ), + tool( + "command_batch", + "Run commands", + "Run several commands in order: {steps: [{id, params?}], stop_on_error?: true}. Returns {completed, failed, results: [{ok, result | error}]}.", + json!({ + "steps": {"type": "array", "items": {"type": "object", "properties": {"id": {"type": "string"}, "params": {"type": "object", "additionalProperties": true}}, "required": ["id"], "additionalProperties": false}}, + "stop_on_error": {"type": "boolean", "description": "Stop at the first failing step (default true)"} + }), + &["steps"], + ), + tool( + "doc_inspect", + "Inspect library", + "The library as JSON: current source, filter, sort, selection, active photo and undo/redo labels (library.state) plus counts (catalog.stats).", + json!({}), + &[], + ), + tool( + "render_preview", + "Render preview", + "Render a photo with its current develop settings (cropped) and return it as an image (`max_side` is the long edge; does not change selection or save a file).", + json!({ + "id": {"type": "integer", "description": "Photo id (default: the active photo); selection is unchanged"}, + "max_side": {"type": "integer", "description": "Long edge in pixels (default 1024, max 4096)"}, + "format": {"type": "string", "enum": ["png", "jpeg"], "description": "Image encoding (default png; jpeg is smaller)"}, + }), + &[], + ), tool( "list_commands", "List commands", @@ -187,6 +232,13 @@ pub fn helper_tools(has_ui: bool) -> Vec { "ids": {"type": "array", "items": {"type": "integer"}, "description": "Photos to export (default: the given/active photo)"}, "path": {"type": "string"}, "dir": {"type": "string"}, + "preset": {"type": "string", "description": "Export preset id or name"}, + "background": {"type": "boolean", "description": "Desktop background export (headless exports remain blocking)"}, + "resize": {"type": "object", "description": "Full Resize options; alternatively use the size helpers below"}, + "subfolder": {"type": "string"}, + "conflict": {"type": "string", "enum": ["unique", "overwrite", "skip"]}, + "tiffCompression": {"type": "string", "enum": ["none", "lzw", "zip"]}, + "dngCompression": {"type": "string", "enum": ["lossless", "deflate", "uncompressed"]}, "format": {"type": "string", "enum": ["jpeg", "png", "tiff", "webp", "avif", "original", "dng"], "description": "original = the file as is + an XMP sidecar with the edits; dng = raw photos as DNG with the edits embedded"}, "longEdge": {"type": "integer", "description": "Output long edge in px. When no size param is given: 3000; 0 = full size (cropped, native resolution)"}, "shortEdge": {"type": "integer", "description": "Output short edge in px"}, @@ -213,6 +265,9 @@ pub fn helper_tools(has_ui: bool) -> Vec { ]; if has_ui { v.extend([ + tool("ui_inspect", "Inspect UI", "UI state and selection (same as inspect_ui).", json!({}), &[]), + tool("ui_screenshot", "Screenshot", "Return the window as an image without saving a file.", + json!({"maxSize": {"type": "integer"}, "format": {"type": "string", "enum": ["png", "jpeg"]}}), &[]), tool( "screenshot", "Screenshot", @@ -298,12 +353,60 @@ pub fn command_tools(backend: &mut dyn Backend) -> Vec { .collect() } +/// Conservative hints: generated command tools may edit; optional output paths make a helper a writer. +fn annotations(name: &str, title: &str) -> Value { + let read_only = matches!( + name, + "command_list" + | "list_commands" + | "doc_inspect" + | "render_preview" + | "query_photos" + | "list_controls" + | "get_develop" + | "inspect_ui" + | "list_widgets" + | "ui_inspect" + | "ui_screenshot" + ); + let files = matches!(name, "export" | "render_photo" | "screenshot"); + json!({"title": title, "readOnlyHint": read_only, "destructiveHint": !read_only && !files, + "idempotentHint": read_only || matches!(name, "render_photo" | "screenshot"), "openWorldHint": false}) +} + +/// Helpers have fixed keys; generated command tools retain the registry's free-form params. +pub(crate) fn check_args(name: &str, args: &Value) -> Result<(), String> { + if args.is_null() { + return Ok(()); + } + let Some(m) = args.as_object() else { + return Err("tool arguments must be an object".into()); + }; + let defs = helper_tools(true); + let Some(def) = defs.iter().find(|t| t["name"] == name) else { + return Ok(()); + }; + let Some(props) = def["inputSchema"]["properties"].as_object() else { + return Ok(()); + }; + if let Some(bad) = m.keys().find(|k| !props.contains_key(*k)) { + let accepted: Vec<&str> = props.keys().map(String::as_str).collect(); + return Err(format!("unknown argument \"{bad}\" for {name}; expected: {}", accepted.join(", "))); + } + Ok(()) +} + /// All tools: helpers, plus the generated command tools when `with_commands`. pub fn tool_definitions(backend: &mut dyn Backend, with_commands: bool) -> Vec { let mut v = helper_tools(backend.has_ui()); if with_commands { v.extend(command_tools(backend)); } + for t in &mut v { + let name = t["name"].as_str().unwrap_or_default().to_string(); + let title = t["title"].as_str().unwrap_or(&name).to_string(); + t["annotations"] = annotations(&name, &title); + } v } @@ -445,22 +548,67 @@ pub fn call_tool(b: &mut dyn Backend, name: &str, args: &Value) -> ToolResult { return ToolResult::from(exec(b, &rest.replace('_', "."), args.clone())); } match name { - "list_commands" => { + "list_commands" | "command_list" => { let filter = s("filter").unwrap_or("").to_lowercase(); + let enabled_only = args.get("enabled_only").and_then(Value::as_bool).unwrap_or(false); ToolResult::from(b.call("engine.commands", json!({})).map(|v| { match v { - Value::Array(a) if !filter.is_empty() => Value::Array( + Value::Array(a) => Value::Array( a.into_iter() .filter(|c| { - c["id"].as_str().unwrap_or("").to_lowercase().contains(&filter) + filter.is_empty() + || c["id"].as_str().unwrap_or("").to_lowercase().contains(&filter) || c["label"].as_str().unwrap_or("").to_lowercase().contains(&filter) }) + .filter(|c| !enabled_only || c["enabled"].as_bool() != Some(false)) .collect(), ), v => v, } })) } + "command_run" => match s("id") { + Some(c) => ToolResult::from(exec(b, c, args.get("params").cloned().filter(|p| !p.is_null()).unwrap_or(json!({})))), + None => ToolResult::error("missing `id`"), + }, + "command_batch" => { + let Some(steps) = args.get("steps").and_then(Value::as_array) else { return ToolResult::error("missing `steps`") }; + let stop = args.get("stop_on_error").and_then(Value::as_bool).unwrap_or(true); + let (mut completed, mut failed, mut results) = (0, 0, Vec::new()); + for st in steps { + let Some(id) = st["id"].as_str() else { + failed += 1; + results.push(json!({"ok": false, "error": "step without `id`"})); + if stop { + break; + } + continue; + }; + match exec(b, id, st.get("params").cloned().filter(|p| !p.is_null()).unwrap_or(json!({}))) { + Ok(r) => { + completed += 1; + results.push(json!({"ok": true, "result": r})); + } + Err(e) => { + failed += 1; + results.push(json!({"ok": false, "error": e})); + if stop { + break; + } + } + } + } + let r = json!({"completed": completed, "failed": failed, "results": results}); + if failed > 0 { ToolResult { is_error: true, ..ToolResult::json(r) } } else { ToolResult::json(r) } + } + "doc_inspect" => ToolResult::from(doc_inspect(b)), + "render_preview" => { + let mut a = obj(args, &["id", "format"]); + if let Some(m) = args.get("max_side") { + a["size"] = m.clone(); + } + render_photo(b, &a) + } "run_command" => match s("command") { Some(c) => ToolResult::from(exec(b, c, args.get("params").cloned().filter(|p| !p.is_null()).unwrap_or(json!({})))), None => ToolResult::error("missing `command`"), @@ -581,12 +729,19 @@ pub fn call_tool(b: &mut dyn Backend, name: &str, args: &Value) -> ToolResult { "watermark", "colorSpace", "bitDepth", + "preset", + "background", + "resize", + "subfolder", + "conflict", + "tiffCompression", + "dngCompression", ], ), ) })), - "screenshot" => screenshot(b, args), - "inspect_ui" => ToolResult::from(b.call("ui.inspect", json!({}))), + "screenshot" | "ui_screenshot" => screenshot(b, args), + "inspect_ui" | "ui_inspect" => ToolResult::from(b.call("ui.inspect", json!({}))), "set_ui" => ToolResult::from(b.call("ui.set", args.get("state").cloned().unwrap_or(json!({})))), "list_widgets" => ToolResult::from(b.call("ui.widgets", obj(args, &["filter"]))), "click" => { @@ -606,3 +761,10 @@ pub fn call_tool(b: &mut dyn Backend, name: &str, args: &Value) -> ToolResult { other => ToolResult::error(format!("unknown tool `{other}` (see tools/list)")), } } + +/// The canonical document resource and tool share the same snapshot. +pub(crate) fn doc_inspect(b: &mut dyn Backend) -> Result { + let library = exec(b, "library.state", json!({}))?; + let stats = exec(b, "catalog.stats", json!({}))?; + Ok(json!({"library": library, "stats": stats})) +} diff --git a/crates/mcp/tests/e2e.rs b/crates/mcp/tests/e2e.rs index ea27919c..ef2e49b6 100644 --- a/crates/mcp/tests/e2e.rs +++ b/crates/mcp/tests/e2e.rs @@ -153,3 +153,115 @@ fn remote_unreachable_is_a_tool_error() { assert_eq!(r[0]["result"]["isError"], true); assert!(r[0]["result"]["content"][0]["text"].as_str().unwrap().contains("not reachable")); } + +/// Real stdio framing and real atomic exports: progress, ping during a batch, cancellation +/// between complete photos, unrelated output preservation, and a usable session afterwards. +#[test] +fn export_progress_and_cancel() { + let dir = std::env::temp_dir().join(format!("lc-mcp-progress-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let (input, mut send) = std::io::pipe().unwrap(); + let (output, write) = std::io::pipe().unwrap(); + let server = std::thread::spawn(move || Server::new(Box::new(Headless::demo())).serve(BufReader::new(input), write).unwrap()); + let (tx, rx) = std::sync::mpsc::channel(); + let reader = std::thread::spawn(move || { + for line in BufReader::new(output).lines() { + let value: Value = serde_json::from_str(&line.unwrap()).unwrap(); + if tx.send(value).is_err() { + break; + } + } + }); + let next = || rx.recv_timeout(std::time::Duration::from_secs(30)).expect("MCP reply/progress"); + writeln!(send, "{}", call(1, "query_photos", json!({"limit":8}))).unwrap(); + let query = next(); + let ids: Vec<_> = query["result"]["structuredContent"]["photos"].as_array().unwrap().iter().map(|p| p["id"].clone()).collect(); + assert_eq!(ids.len(), 8); + let mut request = call(2, "export", json!({"ids":ids,"dir":dir.join("complete"),"longEdge":1200,"format":"png"})); + request["params"]["_meta"] = json!({"progressToken":"photos","io.modelcontextprotocol/protocolVersion":"2026-07-28"}); + writeln!(send, "{request}").unwrap(); + let (mut last, mut notes, mut asked, mut answered) = (-1.0, 0, false, false); + loop { + let value = next(); + if value["method"] == "notifications/progress" { + assert_eq!(value["params"]["progressToken"], "photos"); + let progress = value["params"]["progress"].as_f64().unwrap(); + assert!(progress > last && progress <= 8.0, "{value}"); + assert_eq!(value["params"]["total"], 8); + last = progress; + notes += 1; + if !asked { + writeln!(send, "{}", json!({"jsonrpc":"2.0","id":3,"method":"ping"})).unwrap(); + asked = true; + } + } else if value["id"] == 3 { + answered = true; + } else { + assert_eq!(value["id"], 2, "{value}"); + assert_eq!(value["result"]["isError"], false, "{value}"); + assert_eq!(value["result"]["resultType"], "complete"); + break; + } + } + assert!(notes >= 2 && answered, "{notes} progress messages; ping answered {answered}"); + assert_eq!(last, 8.0); + let out = dir.join("cancelled"); + std::fs::create_dir_all(&out).unwrap(); + let decoy = out.join("photo999.png"); + std::fs::write(&decoy, b"unrelated existing output").unwrap(); + let mut request = call(4, "command_run", json!({"id":"app.export","params":{"ids":ids,"dir":out,"longEdge":1200,"format":"png"}})); + request["params"]["_meta"] = json!({"progressToken":44}); + writeln!(send, "{request}").unwrap(); + loop { + let value = next(); + assert_ne!(value["id"], 4, "must be cancellable before completion: {value}"); + assert_eq!(value["params"]["progressToken"], 44, "{value}"); + if value["params"]["progress"].as_f64().unwrap() > 0.0 { + break; + } + } + writeln!(send, "{}", json!({"jsonrpc":"2.0","method":"notifications/cancelled","params":{"requestId":4}})).unwrap(); + writeln!(send, "{}", call(5, "doc_inspect", json!({}))).unwrap(); + loop { + let value = next(); + if value["method"] == "notifications/progress" { + continue; + } + assert_eq!(value["id"], 5, "cancelled request must not reply: {value}"); + assert_eq!(value["result"]["isError"], false); + break; + } + assert_eq!(std::fs::read(&decoy).unwrap(), b"unrelated existing output"); + let photos: Vec<_> = std::fs::read_dir(&out).unwrap().map(|e| e.unwrap().path()).filter(|p| *p != decoy).collect(); + assert!(!photos.is_empty() && photos.len() < ids.len(), "only completed photos remain: {photos:?}"); + for path in &photos { + lightcraft_codecs::decode(&std::fs::read(path).unwrap(), Default::default()).expect("complete image, no partial/temp file"); + } + assert_eq!(std::fs::read_dir(dir.join("complete")).unwrap().count(), 8, "earlier export retained"); + drop(send); + server.join().unwrap(); + reader.join().unwrap(); + std::fs::remove_dir_all(dir).unwrap(); +} + +#[test] +fn export_without_token_finishes_after_eof() { + let dir = std::env::temp_dir().join(format!("lc-mcp-eof-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let mut headless = Headless::demo(); + let ids: Vec<_> = headless.session.visible_cloned().iter().take(2).map(|id| id.0).collect(); + let request = call(1, "cmd_app_export", json!({"ids":ids,"dir":dir,"longEdge":64,"format":"png"})); + // Headless sessions remain movable to a worker after installing progress support. + let replies = std::thread::spawn(move || { + let mut s = Server::new(Box::new(headless)); + session(&mut s, &[request, call(2, "doc_inspect", json!({}))]) + }) + .join() + .unwrap(); + assert_eq!(replies.len(), 2, "no token means no notifications"); + assert_eq!(replies[0]["result"]["isError"], false, "{replies:?}"); + assert_eq!(replies[1]["id"], 2, "queued request survives EOF"); + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 2); + std::fs::remove_dir_all(dir).unwrap(); +} diff --git a/docs/mcp.md b/docs/mcp.md index 7e815406..3134468e 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -71,6 +71,39 @@ Every stdio MCP client takes the same shape: a `command` plus `args`. For exampl During development you can also point the client at `cargo run --release -p lightcraft-cli -- mcp` (with `"cwd"` set to the repository), at the cost of a slower start. +## Shared core tools + +These follow the same conventions as [filmcraft #28](https://github.com/storytold/filmcraft/pull/28). +They are listed in both full and compact mode; existing documented helpers remain listed too. +There are no hidden compatibility aliases. + +| Tool | Arguments | Result | +|---|---|---| +| `command_list` | `filter?`, `enabled_only?` | Command catalog | +| `command_run` | `id`, `params?` | Command result | +| `command_batch` | `steps: [{id, params?}]`, `stop_on_error?` | Counts and per-step results; stops on error by default | +| `doc_inspect` | none | Library state and catalog counts | +| `render_preview` | `id?`, `max_side?`, `format?` | Image; neither changes selection nor saves a file | + +Each edit in a batch has its own undo step. Connect mode also lists `ui_inspect` and +`ui_screenshot` (the latter has no output-path argument). The existing `inspect_ui`, +`screenshot`, `list_commands`, `run_command` and `render_photo` keep their documented arguments. +Ports and connect mode are unchanged. + +Every listed tool has a title and all four MCP hints. Generated command tools are conservatively +marked as edits. Helpers with optional output paths are marked as writers; `export` is not +idempotent because the default conflict policy chooses a new filename on repeated calls. +Unknown helper argument keys return JSON-RPC `-32602` naming the key and accepted arguments. +Generated `cmd_*` tools and nested command params remain free-form: the registry has prose +parameter docs, not machine-readable schemas. Existing command validation (including strict +export options) is preserved. Escaped tool panics return `isError: true`, and the session keeps +serving; backend panics during resource reads or tool listing return an internal error. +The headless backend owns its session directly, without a session mutex. + +`lightcraft://document` and `lightcraft://commands` contain JSON matching `doc_inspect` and +`command_list`. Existing resources remain available. Requests declaring MCP 2026-07-28 in +per-request `_meta` receive `resultType: complete` and list/read cache hints; reads are not cached. + ## Tools ### Helpers @@ -186,3 +219,20 @@ lightcraft-cli calibrate --max 300 ~/Pictures/2026 # camera colour profiles (d (`Remote`) against a stand-in control server; also import → render → JPEG export of a real file. - `apps/lightcraft-cli/tests/cli.rs` — spawns `lightcraft-cli mcp` with real pipes; `render`; `commands`. + +## Export progress and cancellation + +Headless direct `export`, `command_run` / `run_command` with `app.export`, and `cmd_app_export` +calls report photo-count progress when `params._meta.progressToken` is a string or number. +Notifications are strictly increasing, at most ten per second plus the final total. No token +means no notifications. `ping` is answered at photo boundaries; other requests wait in order +until the export ends. EOF lets a pending export finish and preserves queued requests. + +`notifications/cancelled` with `params.requestId` stops the matching export before its next +photo, suppressing its response. Unknown/completed request ids are ignored. A photo already +being processed finishes first: completed photos remain, each written with the existing atomic +file writer, and there are no partial files to delete. Unrelated outputs and earlier exports +are untouched. A failed export is an `isError` tool result. Transport mutexes recover poisoning. + +Connect mode and exports inside `command_batch` remain synchronous and do not report MCP +progress or cancellation. Use a direct headless export call for this behavior.