From 7eb3b2e072aa3110da2a329bb49109744292a87b Mon Sep 17 00:00:00 2001 From: BFlat Date: Wed, 7 Oct 2026 14:00:36 +0700 Subject: [PATCH] Automation: non-object params rejected; headless sees finished jobs; serve stdio survives a bad line; MCP batch keeps results (#503, #504, #505, #506, #529) (#546) * Command params that are not a JSON object are an error (#506) `Session::dispatch` now rejects array, string, number and boolean params with "invalid parameters for ``: params must be a JSON object" instead of running the command with every default; `null` still means no params. The one check covers the control channel, `serve`, MCP (headless and bridge) and the CLI, and batch steps fail per step. `photocraft-cli run --new` prefixes its error with `--new`. Co-Authored-By: Claude Opus 5.5 * Headless requests apply finished background jobs first (#503) A `wait: false` job was applied to its document only by a later command or `jobs.list`, so `doc.save`, export, `doc.inspect`, `doc.render` and `session.list` (and their MCP tools) still read and wrote the document without it. `Headless::sync_jobs` now runs at the top of every JSON-lines request and before every headless MCP tool call. Co-Authored-By: Claude Opus 5.5 * `serve` stdio keeps the session after a rejected request line (#505) An over-long request line (or one that is not UTF-8) ended the stdio session, losing every open document. It now gets one error reply, the rest of that line is skipped without being dispatched, and the next request is served. `read_bounded_line` reads bytes, so a cut inside a multi-byte character is reported as too long instead of an I/O error. TCP connections still close after an oversized line. Co-Authored-By: Claude Opus 5.5 * MCP `command_batch` keeps its step results when the reply budget stops it (#504) The batch budget charged each step's plain JSON size, but MCP sends the batch reply as escaped text content, so a batch the budget had already stopped could still exceed the tool-result ceiling and come back as a bare "response exceeds" error. MCP (headless and bridge) now uses `BatchReplyBudget::escaped`, which charges the escaped size, through the new `Headless::batch_with_budget`; `serve` keeps the plain budget. Co-Authored-By: Claude Opus 5.5 * docs: the 1 MiB request-line limit does not apply to MCP stdio (#529) The book stated the 1 MiB request-line limit without naming a transport. It applies to desktop and headless TCP and to the headless JSON-lines stdio server; MCP over stdio reads its own lines and only its tool results are size-checked. Say so in the automation security pages and in the control protocol's transport limits. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- apps/photocraft-cli/src/lib.rs | 2 +- apps/photocraft-cli/tests/cli.rs | 9 +++ book/src/automation/security-model.md | 2 +- book/src/security/automation-security.md | 5 +- crates/automation/src/budgets.rs | 37 ++++++++-- crates/automation/src/headless.rs | 11 ++- crates/automation/src/rpc.rs | 91 +++++++++++++++++++----- crates/automation/src/security.rs | 47 +++++++++++- crates/automation/src/server.rs | 50 ++++++++++++- crates/engine/src/jobs.rs | 5 ++ crates/engine/src/jobs_tests.rs | 17 +++++ docs/control-protocol.md | 14 +++- 12 files changed, 258 insertions(+), 32 deletions(-) diff --git a/apps/photocraft-cli/src/lib.rs b/apps/photocraft-cli/src/lib.rs index 5f11e870..cfbb0b19 100644 --- a/apps/photocraft-cli/src/lib.rs +++ b/apps/photocraft-cli/src/lib.rs @@ -247,7 +247,7 @@ fn run_cmds(a: &Args, out: &mut dyn Write, err: &mut dyn Write) -> R { } ([], Some(new)) => { let p: Value = serde_json::from_str(new).map_err(|e| format!("bad --new JSON: {e}"))?; - h.command_run("file.new", p).map_err(|e| e.to_string())?; + h.command_run("file.new", p).map_err(|e| format!("--new: {e}"))?; } _ => return Err("run needs exactly one of or --new ".into()), } diff --git a/apps/photocraft-cli/tests/cli.rs b/apps/photocraft-cli/tests/cli.rs index b23d534a..9ce1be6c 100644 --- a/apps/photocraft-cli/tests/cli.rs +++ b/apps/photocraft-cli/tests/cli.rs @@ -178,6 +178,15 @@ fn run_errors() { assert_eq!(o.status.code(), Some(1)); let o = bin().args(["run", "--new", "{", "--cmd", "x"]).output().unwrap(); assert_eq!(o.status.code(), Some(1)); + // Params must be a JSON object, for `--new` and for each `--params`. + let o = bin().args(["run", "--new", "[3]", "--cmd", "layer.new.layer"]).output().unwrap(); + assert_eq!(o.status.code(), Some(1)); + let e = String::from_utf8_lossy(&o.stderr); + assert!(e.contains("--new") && e.contains("must be a JSON object"), "{e}"); + let o = bin().args(["run", "--new", "{}", "--cmd", "layer.new.layer", "--params", "\"x\""]).output().unwrap(); + assert_eq!(o.status.code(), Some(1)); + let e = String::from_utf8_lossy(&o.stderr); + assert!(e.contains("layer.new.layer") && e.contains("must be a JSON object"), "{e}"); } #[test] diff --git a/book/src/automation/security-model.md b/book/src/automation/security-model.md index 27c15eed..8afa1d21 100644 --- a/book/src/automation/security-model.md +++ b/book/src/automation/security-model.md @@ -11,7 +11,7 @@ Automation requests are untrusted input with potential effects beyond the active | MCP capability scopes | **Partial:** filesystem read/write capabilities only; general tool scopes are absent | | Allowed read/write roots | **Implemented:** separate launch-time roots, with absent authority failing closed | | Symlink-safe capability filesystem | **Implemented:** relative operations use held directory capabilities and reject link escapes | -| Request-byte and JSON-depth limits | **Partial:** 1 MiB request-line limit; no explicit JSON-depth policy | +| Request-byte and JSON-depth limits | **Partial:** 1 MiB request-line limit on TCP and headless JSON-lines stdio (MCP stdio input is not byte-capped); no explicit JSON-depth policy | | Batch-step limit | **Implemented:** 256 steps for headless and MCP batches | | Reply-size limit | **Implemented:** 8 MiB encoded JSON/MCP result; aggregate batch reply budget | | Headless preview budgets | **Implemented:** 2048-pixel requested edge, 67,108,864 source pixels, 5 MiB encoded PNG | diff --git a/book/src/security/automation-security.md b/book/src/security/automation-security.md index 83e14b17..71e5c271 100644 --- a/book/src/security/automation-security.md +++ b/book/src/security/automation-security.md @@ -7,7 +7,10 @@ Automation is a privilege boundary because requests can cause filesystem access, - Desktop control binds to `127.0.0.1` and uses one JSON request/reply per line. - Headless TCP refuses a successfully bound non-loopback address. - Every desktop-control and headless-TCP connection must authenticate with a 256-bit bearer token before method dispatch. -- Encoded request lines are limited to 1 MiB, active TCP connections to 16, and headless/MCP batches to 256 steps. +- Encoded request lines are limited to 1 MiB on desktop and headless TCP and on the headless + JSON-lines stdio server (`photocraft-cli serve`). MCP over stdio has no request-byte cap: the + MCP SDK reads its own lines, and only MCP tool results are size-checked. Active TCP connections + are limited to 16, and headless/MCP batches to 256 steps. - JSON-lines replies and encoded MCP tool results are limited to 8 MiB; retained batch replies have an aggregate budget and stop later steps when exhausted. The headless stdio JSON-lines transport enforces the same request/reply ceilings as TCP. diff --git a/crates/automation/src/budgets.rs b/crates/automation/src/budgets.rs index 37f6e024..6c61a185 100644 --- a/crates/automation/src/budgets.rs +++ b/crates/automation/src/budgets.rs @@ -103,19 +103,34 @@ pub fn write_reply(out: &mut impl Write, reply: &Value) -> io::Result<()> { /// many individually valid replies cannot accumulate into an enormous batch. pub struct BatchReplyBudget { remaining: usize, + /// Charge each result as it costs inside a JSON string (MCP text content). + escaped: bool, } impl Default for BatchReplyBudget { fn default() -> Self { - Self { remaining: MAX_RESPONSE_BYTES - MAX_REQUEST_BYTES - 4096 } + Self { remaining: MAX_RESPONSE_BYTES - MAX_REQUEST_BYTES - 4096, escaped: false } } } impl BatchReplyBudget { + /// For a batch reply sent as MCP text content: the encoded reply is embedded in a JSON + /// string, which escapes every quote and backslash again, so a batch the plain budget accepts + /// could exceed the tool-result ceiling and lose every step result. + pub fn escaped() -> Self { + Self { escaped: true, ..Self::default() } + } + /// Must be called before retaining the result or dispatching the next step. pub fn charge(&mut self, result: &Value) -> Result<(), AutomationError> { - let bytes = encode_with_limit(result, self.remaining.saturating_sub(1))?; - self.remaining = self.remaining.saturating_sub(bytes.len() + 1); + let limit = self.remaining.saturating_sub(1); + let bytes = encode_with_limit(result, limit)?; + // Compact JSON has no raw control characters, so only `"` and `\` grow when escaped. + let size = if self.escaped { bytes.len() + bytes.iter().filter(|&&b| b == b'"' || b == b'\\').count() } else { bytes.len() }; + if size > limit { + return Err(bad(format!("response exceeds {limit} bytes once escaped"))); + } + self.remaining = self.remaining.saturating_sub(size + 1); Ok(()) } } @@ -161,9 +176,23 @@ mod tests { #[test] fn batch_cannot_retain_more_than_the_aggregate_reply_budget() { - let mut budget = BatchReplyBudget { remaining: 10 }; + let mut budget = BatchReplyBudget { remaining: 10, escaped: false }; budget.charge(&json!("abc")).unwrap(); assert!(budget.charge(&json!("abc")).is_err()); assert_eq!(budget.remaining, 4); } + + #[test] + fn escaped_budget_charges_the_size_inside_a_json_string() { + // `"a\"b"` is 6 bytes plain and 10 once embedded in a string (`\"a\\\"b\"`). + let value = json!("a\"b"); + let escaped_len = serde_json::to_string(&Value::String(value.to_string())).unwrap().len() - 2; + assert_eq!(escaped_len, 10); + let mut budget = BatchReplyBudget { remaining: 11, escaped: true }; + budget.charge(&value).unwrap(); + assert_eq!(budget.remaining, 0); + let mut budget = BatchReplyBudget { remaining: 10, escaped: true }; + assert!(budget.charge(&value).is_err()); + assert!(BatchReplyBudget { remaining: 10, escaped: false }.charge(&value).is_ok()); + } } diff --git a/crates/automation/src/headless.rs b/crates/automation/src/headless.rs index d20f3825..0f944af4 100644 --- a/crates/automation/src/headless.rs +++ b/crates/automation/src/headless.rs @@ -49,6 +49,13 @@ impl Headless { Self { session: Session::new(), writers: HashMap::new(), filesystem: Filesystem::Workspace(workspace) } } + /// Apply background jobs that finished since the last request, so every request (save, + /// export, inspect, preview, `session.list`, commands) sees their result. Cheap when no job + /// runs. The JSON-lines server and the MCP server call it before each request. + pub fn sync_jobs(&mut self) { + self.session.poll_jobs(); + } + fn doc_index(&self, index: Option) -> Result { match index { Some(i) if i < self.session.documents().len() => Ok(i), @@ -207,8 +214,8 @@ impl Headless { if !matches!(&self.filesystem, Filesystem::TrustedLocal) { authorize_engine_command(id, ¶ms)?; } - // Background jobs that finished since the last request are applied first. - self.session.poll_jobs(); + // Background jobs that finished since the last request (or batch step) are applied first. + self.sync_jobs(); if wait { return Ok(self.session.execute(id, params)?); } diff --git a/crates/automation/src/rpc.rs b/crates/automation/src/rpc.rs index fa780f71..b69097b4 100644 --- a/crates/automation/src/rpc.rs +++ b/crates/automation/src/rpc.rs @@ -24,7 +24,8 @@ use serde_json::{Value, json}; use crate::budgets::{BatchReplyBudget, write_reply}; use crate::security::{ - ConnectionLimiter, LineRead, MAX_BATCH_STEPS, MAX_CONNECTIONS, MAX_REQUEST_BYTES, authentication_reply, configure_stream, read_bounded_line, + ConnectionLimiter, LineRead, MAX_BATCH_STEPS, MAX_CONNECTIONS, MAX_REQUEST_BYTES, authentication_reply, configure_stream, discard_rest_of_line, + read_bounded_line, }; use crate::{AutomationError, Headless}; @@ -61,6 +62,7 @@ fn str_of<'a>(p: &'a Value, k: &str) -> Option<&'a str> { impl Headless { /// Dispatch one request. Unknown methods and bad params are errors, never panics. pub fn handle(&mut self, method: &str, params: Value) -> Result { + self.sync_jobs(); let p = if params.is_null() { json!({}) } else { params }; match method { "engine.execute" => { @@ -137,6 +139,12 @@ impl Headless { /// `{method, params?}` (any [`METHODS`] entry). Stops at the first error unless /// `stopOnError` is false; the reply lists every step's result. pub fn batch(&mut self, p: &Value) -> Result { + self.batch_with_budget(p, BatchReplyBudget::default()) + } + + /// [`Headless::batch`] with a caller's reply budget (MCP charges escaped sizes, see + /// [`BatchReplyBudget::escaped`]). Steps stop once the budget runs out. + pub fn batch_with_budget(&mut self, p: &Value, mut reply_budget: BatchReplyBudget) -> Result { let steps = p.get("steps").and_then(Value::as_array).ok_or_else(|| bad("batch needs `steps`"))?; if steps.len() > MAX_BATCH_STEPS { return Err(bad(format!("batch contains {} steps; maximum is {MAX_BATCH_STEPS}", steps.len()))); @@ -144,7 +152,6 @@ impl Headless { let stop = p.get("stopOnError").and_then(Value::as_bool).unwrap_or(true); let mut results = Vec::with_capacity(steps.len()); let mut failed = 0usize; - let mut reply_budget = BatchReplyBudget::default(); for (i, s) in steps.iter().enumerate() { let params = s.get("params").cloned().unwrap_or(Value::Null); let r = if let Some(c) = str_of(s, "command") { @@ -201,25 +208,24 @@ pub fn respond(h: &Mutex, line: &str) -> Value { } } -/// Serve JSON lines from `r` to `w` until EOF. Blank lines are ignored. +/// Serve JSON lines from `r` to `w` until EOF. Blank lines are ignored. An over-long or non-UTF-8 +/// line gets an error reply and is skipped; the session (and its open documents) keeps serving. pub fn serve_lines(h: &Mutex, mut r: impl BufRead, mut w: impl Write) -> std::io::Result<()> { let mut line = String::new(); loop { - match read_bounded_line(&mut r, &mut line)? { - LineRead::Eof => return Ok(()), - LineRead::TooLong => { - write_reply(&mut w, &json!({"id": null, "ok": false, "error": format!("request exceeds {MAX_REQUEST_BYTES} bytes")}))?; - w.flush()?; - return Ok(()); - } - LineRead::Line => {} - } - if line.trim().is_empty() { - continue; - } - let reply = respond(h, &line); + let (reply, unread_rest) = match read_bounded_line(&mut r, &mut line) { + Ok(LineRead::Eof) => return Ok(()), + Ok(LineRead::TooLong) => (json!({"id": null, "ok": false, "error": format!("request exceeds {MAX_REQUEST_BYTES} bytes")}), !line.ends_with('\n')), + Err(e) if e.kind() == std::io::ErrorKind::InvalidData => (json!({"id": null, "ok": false, "error": "request is not valid UTF-8"}), false), + Err(e) => return Err(e), + Ok(LineRead::Line) if line.trim().is_empty() => continue, + Ok(LineRead::Line) => (respond(h, &line), false), + }; write_reply(&mut w, &reply)?; w.flush()?; + if unread_rest { + discard_rest_of_line(&mut r)?; + } } } @@ -446,6 +452,37 @@ mod tests { } } + /// Regression (#503): a finished `wait: false` job was only applied by a later command or + /// `jobs.list`, so save, inspect, render and `session.list` still saw the document without it. + #[test] + fn every_request_sees_a_finished_background_job() { + let dir = std::env::temp_dir().join(format!("pc-rpc-job-sync-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let (before, after) = (dir.join("before.png"), dir.join("after.png")); + let mut h = Headless::trusted_local(); + h.handle("doc.new", json!({"width": 64, "height": 48})).unwrap(); + h.handle("engine.execute", json!({"command": "filter.noise.addNoise", "params": {"amount": 50}})).unwrap(); + h.handle("doc.save", json!({"path": before.to_string_lossy()})).unwrap(); + let render = |h: &mut Headless| h.handle("doc.render", json!({"maxSide": 0})).unwrap()["base64"].clone(); + let unblurred = render(&mut h); + let revision = |h: &mut Headless| h.handle("session.list", json!({})).unwrap()["documents"][0]["revision"].clone(); + let start = revision(&mut h); + h.handle("engine.execute", json!({"command": "filter.blur.gaussianBlur", "params": {"radius": 4}, "wait": false})).unwrap(); + // Only reading requests from here on: no command and no `jobs.list`. + let t = std::time::Instant::now(); + while revision(&mut h) == start { + assert!(t.elapsed().as_secs() < 60, "session.list never saw the finished job"); + std::thread::sleep(std::time::Duration::from_millis(5)); + } + let history = h.handle("doc.inspect", json!({})).unwrap()["history"].clone(); + assert_eq!(history.as_array().unwrap().last().unwrap(), "Gaussian Blur", "{history}"); + h.handle("doc.save", json!({"path": after.to_string_lossy()})).unwrap(); + let decode = |p: &std::path::Path| photocraft_codecs::decode(&std::fs::read(p).unwrap()).unwrap().to_rgba8(); + assert_ne!(decode(&before), decode(&after), "the export holds the blur"); + assert_ne!(render(&mut h), unblurred, "the preview holds the blur"); + std::fs::remove_dir_all(dir).unwrap(); + } + #[test] fn automation_preview_rejects_oversized_full_size_and_numeric_wraparound() { let mut h = Headless::new(); @@ -472,6 +509,28 @@ mod tests { assert!(h.lock().unwrap().session.documents().is_empty()); } + /// Regression (#505): an over-long or non-UTF-8 line ended the stdio session, losing every + /// open document. Now it gets one error reply, the rest of the line is skipped (never + /// dispatched), and the next request is served. + #[test] + fn stdio_skips_a_rejected_line_and_keeps_the_session() { + let h = Mutex::new(Headless::new()); + let mut input = b"{\"id\":1,\"method\":\"doc.new\",\"params\":{\"width\":8,\"height\":8}}\n".to_vec(); + input.extend(" ".repeat(MAX_REQUEST_BYTES + 1).into_bytes()); + input.extend(b"{\"id\":9,\"method\":\"doc.close\"}\n"); + input.extend(b"\xff\xfe{\"id\":8,\"method\":\"doc.close\"}\n"); + input.extend(b"{\"id\":2,\"method\":\"session.list\"}\n"); + let mut out = Vec::new(); + serve_lines(&h, input.as_slice(), &mut out).unwrap(); + let replies: Vec = String::from_utf8(out).unwrap().lines().map(|l| serde_json::from_str(l).unwrap()).collect(); + assert_eq!(replies.len(), 4, "{replies:?}"); + assert_eq!(replies[0]["ok"], true); + assert!(replies[1]["error"].as_str().unwrap().contains("request exceeds"), "{}", replies[1]); + assert!(replies[2]["error"].as_str().unwrap().contains("not valid UTF-8"), "{}", replies[2]); + assert_eq!(replies[3]["id"], 2); + assert_eq!(replies[3]["result"]["documents"].as_array().unwrap().len(), 1, "the document stays open"); + } + #[test] fn preview_rejection_precedes_output_file_creation() { let root = std::env::temp_dir().join(format!("pc-preview-budget-{}", std::process::id())); diff --git a/crates/automation/src/security.rs b/crates/automation/src/security.rs index 463f012a..075427c0 100644 --- a/crates/automation/src/security.rs +++ b/crates/automation/src/security.rs @@ -34,19 +34,43 @@ pub enum LineRead { } /// Read one line without ever buffering more than [`MAX_REQUEST_BYTES`] plus one byte. +/// +/// `TooLong` leaves the bytes read so far in `line` (lossily decoded); the rest of that line is +/// still unread unless `line` ends with a newline (see [`discard_rest_of_line`]). A line within +/// the limit that is not UTF-8 is an `InvalidData` error, and the whole line has been consumed. pub fn read_bounded_line(reader: &mut impl BufRead, line: &mut String) -> std::io::Result { line.clear(); - let mut limited = std::io::Read::take(reader, (MAX_REQUEST_BYTES + 1) as u64); - let n = limited.read_line(line)?; + let mut bytes = Vec::new(); + let n = std::io::Read::take(reader, (MAX_REQUEST_BYTES + 1) as u64).read_until(b'\n', &mut bytes)?; if n == 0 { Ok(LineRead::Eof) } else if n > MAX_REQUEST_BYTES { + // A cut can split a multi-byte character; the length, not the text, decides. + line.push_str(&String::from_utf8_lossy(&bytes)); Ok(LineRead::TooLong) } else { + *line = String::from_utf8(bytes).map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; Ok(LineRead::Line) } } +/// Skip the rest of the current line (through its newline, or to EOF) without buffering it, so +/// a stream transport can answer an over-long request and keep reading the next one. +pub fn discard_rest_of_line(reader: &mut impl BufRead) -> std::io::Result<()> { + loop { + let buf = reader.fill_buf()?; + if buf.is_empty() { + return Ok(()); + } + if let Some(end) = buf.iter().position(|&b| b == b'\n') { + reader.consume(end + 1); + return Ok(()); + } + let n = buf.len(); + reader.consume(n); + } +} + /// Generate a 256-bit bearer token with the operating system CSPRNG. pub fn generate_token() -> Result { let mut bytes = [0u8; TOKEN_BYTES]; @@ -235,6 +259,25 @@ mod tests { let mut line = String::new(); assert_eq!(read_bounded_line(&mut reader, &mut line).unwrap(), LineRead::TooLong); assert_eq!(line.len(), MAX_REQUEST_BYTES + 1); + discard_rest_of_line(&mut reader).unwrap(); + assert_eq!(read_bounded_line(&mut reader, &mut line).unwrap(), LineRead::Eof); + } + + #[test] + fn bounded_reader_reports_a_cut_multibyte_line_as_too_long_and_bad_utf8_as_invalid_data() { + // The cut at the limit falls inside a two-byte character. + let input = format!("xx{}\nnext\n", "é".repeat(MAX_REQUEST_BYTES / 2)); + let mut reader = std::io::Cursor::new(input); + let mut line = String::new(); + assert_eq!(read_bounded_line(&mut reader, &mut line).unwrap(), LineRead::TooLong); + discard_rest_of_line(&mut reader).unwrap(); + assert_eq!(read_bounded_line(&mut reader, &mut line).unwrap(), LineRead::Line); + assert_eq!(line, "next\n"); + // Invalid UTF-8 consumes its whole line, so the next read starts on the next line. + let mut reader = std::io::Cursor::new(b"\xff\xfe\nnext\n".to_vec()); + assert_eq!(read_bounded_line(&mut reader, &mut line).unwrap_err().kind(), std::io::ErrorKind::InvalidData); + assert_eq!(read_bounded_line(&mut reader, &mut line).unwrap(), LineRead::Line); + assert_eq!(line, "next\n"); } #[test] diff --git a/crates/automation/src/server.rs b/crates/automation/src/server.rs index 2a7017d3..eed14cf0 100644 --- a/crates/automation/src/server.rs +++ b/crates/automation/src/server.rs @@ -279,6 +279,7 @@ impl PhotocraftMcp { // of the document (`Session::edit`), so the session is still // consistent: keep serving instead of failing every later call. let mut g = h.lock().unwrap_or_else(PoisonError::into_inner); + g.sync_jobs(); f(&mut g) }) .await @@ -520,7 +521,8 @@ impl PhotocraftMcp { let steps: Vec = p.steps.into_iter().map(|s| json!({"command": s.id, "params": s.params.unwrap_or_else(|| json!({})), "wait": s.wait.unwrap_or(true)})).collect(); let args = json!({"steps": steps, "stopOnError": stop}); - if let Some(r) = self.headless_op(move |h| h.batch(&args)).await { + // The reply travels as escaped JSON text, so steps are charged their escaped size. + if let Some(r) = self.headless_op(move |h| h.batch_with_budget(&args, BatchReplyBudget::escaped())).await { return to_result(r); } let Some(b) = self.bridge_client() else { @@ -528,7 +530,7 @@ impl PhotocraftMcp { }; let mut results = Vec::new(); let mut failed = 0; - let mut reply_budget = BatchReplyBudget::default(); + let mut reply_budget = BatchReplyBudget::escaped(); for s in &steps { let response = b.call("engine.execute", s.clone()).await; let was_error = response.is_err(); @@ -707,4 +709,48 @@ mod tests { let r = mcp.headless_op(|h| h.command_run("file.new", json!({"width": 8, "height": 8}))).await.unwrap(); assert!(r.is_ok(), "session still usable after a panic: {r:?}"); } + + /// Regression (#504): the batch budget counted plain JSON, but MCP sends the reply as escaped + /// text, so a batch the budget stopped could still exceed the tool-result ceiling and the + /// client lost every step result. + #[tokio::test(flavor = "multi_thread")] + async fn mcp_command_batch_stopped_by_the_budget_keeps_its_step_results() { + let mcp = PhotocraftMcp::headless(); + let step = |id: &str, params: Value| RunParams { id: id.into(), params: Some(params), wait: None }; + let mut steps: Vec = (0..MAX_BATCH_STEPS - 1).map(|_| step("command.list", json!({}))).collect(); + steps.push(step("file.new", json!({"width": 8, "height": 8}))); + let result = mcp.command_batch(Parameters(BatchParams { steps, stop_on_error: Some(false) })).await.unwrap(); + assert_ne!(result.is_error, Some(true), "{:?}", result.content.first().and_then(|c| c.as_text()).map(|t| &t.text[..200.min(t.text.len())])); + let text = &result.content.first().and_then(|c| c.as_text()).unwrap().text; + let reply: Value = serde_json::from_str(text).unwrap(); + let results = reply["results"].as_array().unwrap(); + assert!(results.last().unwrap()["error"].as_str().unwrap().contains("batch response budget exceeded"), "{}", results.last().unwrap()); + assert_eq!(reply["completed"].as_u64().unwrap() as usize, results.len() - 1); + assert_eq!(reply["failed"], 1); + let docs = mcp.headless_op(|h| Ok(h.session.documents().len())).await.unwrap().unwrap(); + assert_eq!(docs, 0, "the step after the budget ran out did not run"); + } + + /// The MCP server applies finished jobs before every tool call too (#503). + #[tokio::test(flavor = "multi_thread")] + async fn mcp_tools_see_a_finished_background_job() { + let mcp = PhotocraftMcp::headless(); + mcp.headless_op(|h| { + h.command_run("file.new", json!({"width": 64, "height": 48}))?; + h.command_run("filter.noise.addNoise", json!({"amount": 50}))?; + h.command_start("filter.blur.gaussianBlur", json!({"radius": 4}), false) + }) + .await + .unwrap() + .unwrap(); + let t = std::time::Instant::now(); + loop { + let inspected = mcp.headless_op(|h| h.inspect(None)).await.unwrap().unwrap(); + if inspected["history"].as_array().unwrap().last().unwrap() == "Gaussian Blur" { + break; + } + assert!(t.elapsed().as_secs() < 60, "doc_inspect never saw the finished job: {inspected}"); + std::thread::sleep(std::time::Duration::from_millis(5)); + } + } } diff --git a/crates/engine/src/jobs.rs b/crates/engine/src/jobs.rs index dc948a82..1ec15f01 100644 --- a/crates/engine/src/jobs.rs +++ b/crates/engine/src/jobs.rs @@ -588,6 +588,11 @@ impl Session { /// Run the command `id`: inline, or (with `background`) as a job when it is one. pub(crate) fn dispatch(&mut self, id: &str, params: Value, background: bool) -> Result { let spec = crate::commands::find(id).ok_or_else(|| EngineError::UnknownCommand(id.to_string()))?; + // Params are named fields: anything but an object (or null, meaning none) is a caller + // mistake, not "use every default". + if !(params.is_object() || params.is_null()) { + return Err(EngineError::BadParams { cmd: id.to_string(), msg: "params must be a JSON object".into() }); + } if let Err(why) = crate::smart_cmds::target_enabled(self, spec, ¶ms).unwrap_or_else(|| (spec.enabled)(self)) { return Err(EngineError::Disabled(id.to_string(), why)); } diff --git a/crates/engine/src/jobs_tests.rs b/crates/engine/src/jobs_tests.rs index 3dc9d96e..63409cd6 100644 --- a/crates/engine/src/jobs_tests.rs +++ b/crates/engine/src/jobs_tests.rs @@ -305,6 +305,23 @@ fn wait_job_blocks_until_applied() { assert!(s.wait_job(JobId(12345)).is_err()); } +#[test] +fn params_that_are_not_an_object_are_rejected_before_running() { + let mut s = session(40, 30); + let before = s.active().unwrap().history.past_len(); + for id in ["image.adjustments.invert", "filter.blur.gaussianBlur", "layer.new.layer"] { + for p in [json!([3]), json!("x"), json!(5), json!(true)] { + let e = s.execute(id, p.clone()).unwrap_err().to_string(); + assert!(e.contains(id) && e.contains("must be a JSON object"), "{id} {p}: {e}"); + assert!(s.start(id, p).is_err(), "{id}: background start must reject too"); + } + } + assert_eq!(s.active().unwrap().history.past_len(), before, "nothing ran"); + s.execute("image.adjustments.invert", json!({})).unwrap(); + s.execute("image.adjustments.invert", Value::Null).unwrap(); + assert_eq!(s.active().unwrap().history.past_len(), before + 2); +} + /// Cancel latency on a 24 MP document (6000×4000): from `cancel_job` until the worker thread /// has exited. Release only: debug builds are ~20× slower per tile. #[test] diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 1b4c9a87..1e0e4a62 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -32,7 +32,7 @@ The transport is `apps/photocraft/src/control_server.rs`, and the handlers are i ## Methods -- `engine.execute {command, params}`: run any engine or UI command by id. Engine commands run directly with their default params and never open a dialog. Use `ui.menu.invoke` for menu-click behaviour, which opens a command's dialog when no params are given +- `engine.execute {command, params}`: run any engine or UI command by id. Engine commands run directly with their default params and never open a dialog. Use `ui.menu.invoke` for menu-click behaviour, which opens a command's dialog when no params are given. `params` must be a JSON object (omit it or pass `null` for none); an array, string, number or boolean is an error naming the command, in every transport (control channel, `serve`, MCP and the CLI) - `engine.commands`: list commands with enablement - `ui.inspect`: full UI state (tool, panels, views, dialogs, windows, window size). The menu tree is not included; use `ui.menu.list`. `view` holds the View/Window/Type preferences: `screen_mode`, `extras`, `show` and `snap_to` flags, `flip_horizontal`, `arrange` (Window › Arrange layout), pixel aspect, font preview size, language options. `perf.timings.gpuInfo` holds the graphics adapter, backend, driver, the backend chosen at launch and why, the canvas renderer (`gpu`/`cpu`) and, after a device loss, `lost` (`help.systemInfo` returns the same as `info`) - `ui.set {tool?, panels?, dockTabs?, dock?, dockWidth?, maskTarget?, vectorMaskTarget?, selectionMode?, zoom?, center?, fit?, theme?, brushSize?, brushSection?, brushTab?, brushesView?}`: change UI state; any other field is an error, checked before anything changes (`theme` is `pro`, `proMedium`, `studio`, `studioLight` or `classic`; `selectionMode` is the selection tools' options-bar mode, 0 New, 1 Add, 2 Subtract, 3 Intersect; `brushSection` indexes the Brush Settings sections, `brushTab` 0 = Brush Settings, 1 = Brushes; `dock` is `{order: ["layers", …], heights: {"properties": 180}, collapsed: ["color"]}`, the right-dock groups top to bottom, their heights in points and the groups collapsed to their tab strip; `dockWidth` sets the right dock width in points, clamped to 250..520) @@ -195,7 +195,7 @@ no MCP framing, no app start-up per command. Configure its file access with the | Method | Params | |---|---| | `engine.execute` | `{command, params?, wait?}`: any engine command (`wait: false` starts a long one as a background job: `{job, pending}`) | -| `jobs.list` / `jobs.cancel` | `{}` / `{job?}`: background jobs (applying finished ones); cancel one or all | +| `jobs.list` / `jobs.cancel` | `{}` / `{job?}`: background jobs; cancel one or all. Every request (and MCP tool call) first applies the jobs that finished, so `doc.save`, `doc.inspect`, `doc.render` and `session.list` include a finished job's result without polling `jobs.list` first | | `engine.commands` | `{filter?}`: registry with params docs and enablement | | `session.list` | open documents and the active index | | `doc.open` / `doc.new` | `{path}` / `file.new` params | @@ -230,8 +230,16 @@ The desktop and headless TCP listeners currently enforce: An oversized line, excess connection, unauthenticated request, or unauthorized filesystem path is rejected before command dispatch or file effects. The headless JSON-lines stdio server also -enforces the request and reply byte ceilings. MCP tool results are checked as encoded JSON, +enforces the request and reply byte ceilings; MCP over stdio (`photocraft-cli mcp`) does not +cap request bytes, because the MCP SDK reads its own request lines (only MCP tool results are +checked). A TCP connection is closed after an oversized +line; on stdio an oversized or non-UTF-8 line gets one error reply (`id: null`), the rest of that +line is skipped without being dispatched, and the session and its open documents keep serving. MCP tool results are checked as encoded JSON, including the text/image content envelope, and the MCP bridge bounds incoming desktop replies. +A `batch` or `command_batch` stops at the first step whose result no longer fits the reply budget +(that step may have run; later ones do not). MCP charges each result at its size escaped inside +the text content, so the reply still lists `completed`, `failed` and every result so far, ending +with the budget error. Headless automation previews (`doc.render` / MCP `doc_render_preview`) allow a maximum requested edge of 2048 pixels and a source document of at most 67,108,864 pixels. `maxSide: 0` (MCP