diff --git a/ROADMAP.md b/ROADMAP.md index 37dd2a42..98670f15 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2,7 +2,7 @@ **Stage: alpha** · next: beta, ~20 points and ~120–210 h away -> **Last reviewed:** 2026-10-14 · **Last updated:** 2026-10-14 · **Change:** major (community PR wave 2026-10-14: ~60 landed items in the progress log, among them the Font, Paragraph, Photo Album, theme colour and font and Date and Time dialogs, presenter and show controls, the SmartArt Text Pane and PPTX corpus slices; breadth 84% after `cargo xtask parity`, ready stays ~55%, stage stays alpha; duplicated rows and headers folded; previous: minor, hours to ~95% per audience added) · **Target:** Microsoft PowerPoint for Mac 16.113 (Microsoft 365) +> **Last reviewed:** 2026-10-14 · **Last updated:** 2026-10-14 · **Change:** trivial (M13.1: connected MCP validates bounded reply frames, #718; parity estimates unchanged) · **Target:** Microsoft PowerPoint for Mac 16.113 (Microsoft 365) DeckCraft aims at full PowerPoint parity, and to be better: faster, open (a documented zip+JSON format plus PPTX), scriptable by agents (CLI, JSON control channel, MCP), and available everywhere @@ -132,6 +132,7 @@ app's JSON control channel (`deckcraft --control PORT`). | Date | What landed | |---|---| | 2026-10-15 | PDF export and rendering: `anchorCtr` centres the drawn text block — right-aligned text no longer ran past the box edge (#736) | +| 2026-10-14 | M13.1: connected MCP rejects unrelated, malformed, incomplete and oversized replies without replay; buffered unsolicited bytes force a fresh connection before sending (#718) | | 2026-10-13 | Eyedropper: Shape Fill, Shape Outline, Font Color and every other colour menu end with an Eyedropper that takes the colour drawn at a point of the slide (hover shows it, a click applies it, Esc cancels); `slide.colorAt` gives the same from scripts and MCP (#367) | | 2026-10-13 | Slide show: Honeycomb's hexagons cover the whole slide again — the bottom quarter stayed black until the endpoint (#420) | | 2026-10-13 | Copy and Cut from the ribbon, the slide's right-click menu and the menus put the text on the system clipboard, as Ctrl+C and Ctrl+X do (#431) | @@ -222,6 +223,7 @@ app's JSON control channel (`deckcraft --control PORT`). | Date | Change | Summary | |---|---|---| +| 2026-10-14 | trivial | M13.1: remote MCP checks reply IDs, envelopes and bounded newline framing (#718) | | 2026-10-14 | minor | Community PR wave folded: headline breadth 84% over 191 features (P0 92%, P1 82%, P2 55%) and the feature-area breadths from `cargo xtask parity`; one row per area; Upcoming item 2 merged; per-PR count notes dropped from the progress log. Ready stays ~55%, stage alpha | | 2026-10-13 | trivial | Progress log: Eyedropper in the colour menus, slide.colorAt (#367) | | 2026-10-13 | trivial | Progress log: video seek frame-loss race and regression coverage | diff --git a/crates/mcp/src/backend.rs b/crates/mcp/src/backend.rs index cc43b4ee..fb809c09 100644 --- a/crates/mcp/src/backend.rs +++ b/crates/mcp/src/backend.rs @@ -1,11 +1,15 @@ //! Where MCP tool calls end up: a control-channel method call. -use std::io::{BufRead, BufReader, Write}; +use std::io::{BufRead, BufReader, Read, Write}; use std::net::{TcpStream, ToSocketAddrs}; use std::time::Duration; use serde_json::{Value, json}; +// Includes the newline, matching the control server's request budget. In particular a peer +// must not be able to grow a reply indefinitely by withholding its newline. +const MAX_REPLY_BYTES: u64 = 16 * 1024 * 1024; + /// Something that answers control-channel methods (`engine.execute`, `document.inspect`, /// `ui.pointer`, `ui.render`, …). See `deckcraft_ui_egui::control` for the full list. pub trait Backend { @@ -58,7 +62,7 @@ impl Remote { /// Write one request line. A failure here means the app never got a complete line, so retrying is safe. /// A connection the app has already closed (it restarted since the last call) is replaced before writing. fn send(&mut self, line: &str) -> std::io::Result<()> { - if self.conn.as_ref().is_none_or(|(_, writer)| is_closed(writer)) { + if self.conn.as_ref().is_none_or(|(reader, writer)| !reader.buffer().is_empty() || is_closed(writer)) { self.reconnect()?; } let Some((_, writer)) = self.conn.as_mut() else { @@ -69,15 +73,34 @@ impl Remote { } /// Read the reply to a request that was already sent. - fn receive(&mut self) -> std::io::Result { + fn receive(&mut self, id: u64) -> std::io::Result> { let Some((reader, _)) = self.conn.as_mut() else { return Err(std::io::Error::new(std::io::ErrorKind::NotConnected, "not connected")); }; - let mut reply = String::new(); - if reader.read_line(&mut reply)? == 0 { - return Err(std::io::Error::new(std::io::ErrorKind::UnexpectedEof, "control channel closed")); + let mut reply = Vec::new(); + reader.take(MAX_REPLY_BYTES + 1).read_until(b'\n', &mut reply)?; + if reply.len() as u64 > MAX_REPLY_BYTES { + return Err(std::io::Error::new(std::io::ErrorKind::InvalidData, "control reply exceeds 16 MiB")); + } + if reply.last() != Some(&b'\n') { + return Err(std::io::Error::new(std::io::ErrorKind::UnexpectedEof, "control reply has no terminating newline")); + } + let invalid = |message| std::io::Error::new(std::io::ErrorKind::InvalidData, message); + let v: Value = serde_json::from_slice(&reply).map_err(|e| invalid(format!("bad reply from app: {e}")))?; + if v.get("id").and_then(Value::as_u64) != Some(id) { + return Err(invalid("control reply has an unexpected request id".to_string())); + } + match v.get("ok").and_then(Value::as_bool) { + Some(true) if v.get("error").is_none() => { + v.get("result").cloned().map(Ok).ok_or_else(|| invalid("successful control reply has no result".to_string())) + } + Some(false) if v.get("result").is_none() => v + .get("error") + .and_then(Value::as_str) + .map(|error| Err(error.to_string())) + .ok_or_else(|| invalid("failed control reply has no error string".to_string())), + _ => Err(invalid("control reply has an invalid envelope".to_string())), } - Ok(reply) } } @@ -107,19 +130,13 @@ impl Backend for Remote { format!("DeckCraft app at {} is not reachable: {e}", self.addr) })?; } - let reply = self.receive().map_err(|e| { + self.receive(id).map_err(|e| { self.conn = None; format!( "lost the reply from the DeckCraft app at {} ({e}); the command may or may not have run, check the document before retrying", self.addr ) - })?; - let v: Value = serde_json::from_str(reply.trim()).map_err(|e| format!("bad reply from app: {e}"))?; - if v.get("ok").and_then(Value::as_bool) == Some(true) { - Ok(v.get("result").cloned().unwrap_or(Value::Null)) - } else { - Err(v.get("error").and_then(Value::as_str).unwrap_or("unknown error").to_string()) - } + })? } fn has_ui(&self) -> bool { @@ -138,6 +155,138 @@ mod tests { use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; + fn reply_once(reply: Vec) -> (Remote, std::thread::JoinHandle<()>) { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind"); + let addr = listener.local_addr().expect("addr").to_string(); + let peer = std::thread::spawn(move || { + let (mut stream, _) = listener.accept().expect("accept"); + stream.set_read_timeout(Some(Duration::from_secs(5))).expect("timeout"); + stream.set_write_timeout(Some(Duration::from_secs(5))).expect("write timeout"); + let mut line = String::new(); + BufReader::new(&stream).read_line(&mut line).expect("request"); + assert_eq!(serde_json::from_str::(&line).expect("JSON")["id"], 1); + // An oversized reply may be rejected before the peer finishes writing it. + let _ = stream.write_all(&reply); + }); + (Remote::connect(&addr).expect("connect"), peer) + } + + #[test] + fn wrong_reply_id_is_not_command_success() { + let (mut remote, peer) = reply_once(b"{\"id\":999,\"ok\":true,\"result\":\"unrelated\"}\n".to_vec()); + let result = remote.call("engine.execute", json!({"command":"slide.new"})); + peer.join().expect("peer"); + let error = result.expect_err("an unrelated reply cannot confirm a mutation"); + assert!(error.contains("may or may not have run"), "{error}"); + assert!(remote.conn.is_none(), "discard the invalid connection without replaying"); + } + + #[test] + fn matching_success_and_error_replies_keep_their_meaning() { + for (reply, expected) in [ + ("{\"id\":1,\"ok\":true,\"result\":null}\r\n", Ok(Value::Null)), + ("{\"id\":1,\"ok\":true,\"result\":{\"slides\":2}}\n", Ok(json!({"slides":2}))), + ("{\"id\":1,\"ok\":false,\"error\":\"disabled\"}\n", Err("disabled".into())), + ] { + let (mut remote, peer) = reply_once(reply.as_bytes().to_vec()); + assert_eq!(remote.call("engine.execute", json!({})), expected); + assert!(remote.conn.is_some(), "valid responses do not invalidate the connection"); + peer.join().expect("peer"); + } + } + + #[test] + fn malformed_reply_envelopes_leave_the_outcome_unknown() { + for reply in [ + "not JSON\n", + "[]\n", + "{\"ok\":true,\"result\":null}\n", + "{\"id\":\"1\",\"ok\":true,\"result\":null}\n", + "{\"id\":1,\"ok\":1,\"result\":null}\n", + "{\"id\":1,\"ok\":true}\n", + "{\"id\":1,\"ok\":false}\n", + "{\"id\":1,\"ok\":false,\"error\":42}\n", + "{\"id\":1,\"ok\":true,\"result\":null,\"error\":\"failed\"}\n", + "{\"id\":1,\"ok\":false,\"result\":null,\"error\":\"failed\"}\n", + ] { + let (mut remote, peer) = reply_once(reply.as_bytes().to_vec()); + let result = remote.call("engine.execute", json!({})); + peer.join().expect("peer"); + let error = result.expect_err(reply); + assert!(error.contains("may or may not have run"), "{reply}: {error}"); + assert!(remote.conn.is_none(), "{reply}: discard the invalid connection"); + } + } + + #[test] + fn eof_without_a_newline_is_not_a_complete_reply() { + let (mut remote, peer) = reply_once(b"{\"id\":1,\"ok\":true,\"result\":null}".to_vec()); + let result = remote.call("engine.execute", json!({})); + peer.join().expect("peer"); + assert!(result.is_err(), "a truncated frame cannot confirm a mutation"); + assert!(remote.conn.is_none()); + } + + #[test] + fn oversized_reply_is_rejected() { + let reply = format!("{{\"id\":1,\"ok\":true,\"result\":\"{}\"}}\n", "x".repeat(16 * 1024 * 1024)); + let (mut remote, peer) = reply_once(reply.into_bytes()); + let result = remote.call("engine.execute", json!({})); + let invalidated = remote.conn.is_none(); + // Close the socket before joining: the bounded reader need not drain the peer's reply. + remote.conn = None; + peer.join().expect("peer"); + assert!(result.is_err(), "a reply is limited to 16 MiB"); + let error = result.expect_err("checked above"); + assert!(error.contains("may or may not have run"), "{error}"); + assert!(invalidated, "discard the oversized reply's connection"); + } + + #[test] + fn reply_at_the_size_limit_is_accepted() { + let prefix = "{\"id\":1,\"ok\":true,\"result\":\""; + let suffix = "\"}\n"; + let payload_len = 16 * 1024 * 1024 - prefix.len() - suffix.len(); + let reply = format!("{prefix}{}{suffix}", "x".repeat(payload_len)); + let (mut remote, peer) = reply_once(reply.into_bytes()); + let result = remote.call("document.inspect", json!({})).expect("reply at the limit"); + assert_eq!(result.as_str().map(str::len), Some(payload_len)); + peer.join().expect("peer"); + } + + #[test] + fn buffered_unsolicited_reply_is_discarded_before_sending() { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind"); + let addr = listener.local_addr().expect("addr").to_string(); + let peer = std::thread::spawn(move || { + let (mut first, _) = listener.accept().expect("first connection"); + first.set_read_timeout(Some(Duration::from_secs(5))).expect("timeout"); + let mut line = String::new(); + BufReader::new(&first).read_line(&mut line).expect("initial request"); + // Even a stale frame whose ID happens to match must not answer a new request. + first + .write_all(b"{\"id\":1,\"ok\":true,\"result\":\"first\"}\n{\"id\":2,\"ok\":true,\"result\":\"stale\"}\n") + .expect("two replies together"); + line.clear(); + let sent_on_stale = BufReader::new(&first).read_line(&mut line).expect("first request") != 0; + if !sent_on_stale { + let (mut fresh, _) = listener.accept().expect("fresh connection"); + fresh.set_read_timeout(Some(Duration::from_secs(5))).expect("timeout"); + BufReader::new(&fresh).read_line(&mut line).expect("fresh request"); + fresh.write_all(b"{\"id\":2,\"ok\":false,\"error\":\"rejected\"}\n").expect("actual reply"); + } + sent_on_stale + }); + let mut remote = Remote::connect(&addr).expect("connect"); + assert_eq!(remote.call("document.inspect", json!({})), Ok(json!("first"))); + // Force the stale frame into BufReader, where a socket-only peek cannot see it. + assert!(!remote.conn.as_mut().expect("connection").0.fill_buf().expect("buffer").is_empty()); + let result = remote.call("engine.execute", json!({"command":"slide.new"})); + let sent_on_stale = peer.join().expect("peer"); + assert_eq!(result, Err("rejected".into()), "only the fresh peer's response counts"); + assert!(!sent_on_stale, "replace a connection with buffered unsolicited bytes before sending"); + } + #[test] fn lost_reply_is_not_resent() { let listener = TcpListener::bind("127.0.0.1:0").expect("bind"); diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 0bd20302..e715d1c7 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -3,6 +3,12 @@ Start the app with `--control `. The server listens on `127.0.0.1` only and speaks JSON lines: one request object per line, one reply per line. +The connected MCP client requires the reply's numeric `id` to match its request, `ok` to be a +boolean, and exactly one of `result` (success) or a string `error` (failure). It caps each reply at +16 MiB including its terminating newline, accepts CRLF, and rejects EOF inside a frame. Invalid +replies invalidate the connection and leave the command's outcome unknown; the client does not +replay it. Unsolicited bytes already buffered between calls force a reconnect before sending. + The server bounds what a client can hold: 16 connections at once (more get `{"ok":false,"error":"busy: …"}` and are closed), 16 MiB per request line (longer closes the connection), 64 queued requests (more are answered `busy`), 10 minutes idle and 30 seconds per blocked reply write before the socket is closed, and diff --git a/docs/gaps.md b/docs/gaps.md index 4adfda40..2cba9043 100644 --- a/docs/gaps.md +++ b/docs/gaps.md @@ -1,6 +1,6 @@ # Where DeckCraft falls short of PowerPoint -> **Last reviewed:** 2026-10-14 · **Last updated:** 2026-10-14 · **Change:** major (community PR wave 2026-10-14: soak tests, PPTX corpus, comment threads (#70), embedded fonts, SmartArt Text Pane, Animation Pane timeline, Hunspell spelling, thesaurus, AutoCorrect, 14 new dialogs, pen pressure and trackpad gestures; duplicated entries and headers folded; previous: minor, 3-D bevel, camera and face lighting rendered) · **Target:** Microsoft PowerPoint for Mac 16.113 (Microsoft 365) +> **Last reviewed:** 2026-10-14 · **Last updated:** 2026-10-14 · **Change:** trivial (connected MCP reply validation, #718; existing gaps and estimates unchanged) · **Target:** Microsoft PowerPoint for Mac 16.113 (Microsoft 365) Every known shortfall, one entry each, ranked by how much it stops someone who presents with PowerPoint from switching. This is the work list: pick from the top. Numbers are summarized in @@ -355,6 +355,7 @@ Detail in [localization-parity.md](localization-parity.md). | Date | Change | Summary | |---|---|---| +| 2026-10-14 | trivial | Fixed #718: remote MCP no longer accepts unrelated or buffered stale success; loopback regressions cover invalid envelopes, incomplete and oversized frames | | 2026-10-14 | minor | Community PR wave folded: one entry each for the soak tests (engine and UI), the PPTX corpus (seven owned corpus files), edit points / text warp / 3-D, proofing (Hunspell spelling, thesaurus, AutoCorrect), the smaller missing rows, dialogs (41 vs ~60; Print and Insert Object left) and pen/trackpad input; ranked rows 3, 4 and 10 de-duplicated (comment threads #70 and the pane timeline are done); the fixed command-palette entry (#79) removed | | 2026-10-13 | minor | EMF/WMF rectangle clip regions (`INTERSECTCLIPRECT`, `EXCLUDECLIPRECT`, `EXTSELECTCLIPRGN`) limit later drawing. EMF+ records remain | | 2026-10-13 | minor | Embedded fonts are read, written and drawn (TrueType, OpenType, Embedded OpenType). The gap is now MicroType Express subsets, which are kept but not drawn | diff --git a/docs/mcp.md b/docs/mcp.md index b2f879b3..a65fcda3 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -5,6 +5,18 @@ headless engine (no window); with `--connect PORT` it drives a running DeckCraft `--control PORT`, so an agent and a person can work on the same deck. `--sample` opens the sample deck. +Connected mode accepts only newline-terminated replies of at most 16 MiB (including the newline), +with the current request's numeric `id`, a boolean `ok`, and either `result` on success or a string +`error` on failure. CRLF is accepted. Invalid replies close the connection and report that the +command may or may not have run; they never cause a replay. Check the document before retrying a +mutation. Before sending another request, unsolicited bytes in the reader's buffer or on the +socket cause a fresh connection to be opened. Large image replies must fit this budget; render +to a file or use a smaller scale when necessary. + +Regression coverage for #718 lives in `crates/mcp/src/backend.rs`: synthetic loopback peers test +reply correlation, envelopes, framing, the size cap and buffered unsolicited bytes, alongside the +lost-reply and reconnect tests. + ```json { "mcpServers": { diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 9843d75e..2cd18abf 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -228,7 +228,7 @@ Open P0 items: Contextual tabs (Shape Format, Picture Format, Table Design/Layou | Undo & clipboard | Unlimited undo/redo | P0 | D | | Undo & clipboard | Cut, copy, paste shapes and slides | P0 | D | | Undo & clipboard | Paste special (keep formatting, picture, text only) | P1 | D | -| Automation (✱) | Every command scriptable (CLI, control channel, MCP) | P0 | D | +| Automation (✱) | Every command scriptable (CLI, control channel, MCP; remote reply IDs, envelopes and framing validated, #718) | P0 | D | | Automation (✱) | Headless rendering and conversion | P0 | D | | Platforms (✱) | macOS, Windows, Linux builds | P0 | P | | Platforms (✱) | Web (WASM) build | P1 | D | diff --git a/docs/roadmap.md b/docs/roadmap.md index 8294137d..7dc0021c 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -1,6 +1,6 @@ # DeckCraft roadmap: milestones and what's next -> **Last reviewed:** 2026-10-14 · **Last updated:** 2026-10-14 · **Change:** major (community PR wave 2026-10-14: PPTX corpus, presenter and show controls, review navigation, starter decks, Grid Options and New Window, WordArt gallery and handles, pen pressure; beta items 9 and 10 narrowed; duplicated rows and headers folded; previous: minor, core-workflow alpha gate added) · **Target:** Microsoft PowerPoint for Mac 16.113 (Microsoft 365) +> **Last reviewed:** 2026-10-14 · **Last updated:** 2026-10-14 · **Change:** trivial (M13 connected MCP reply validation, #718; beta priorities unchanged) · **Target:** Microsoft PowerPoint for Mac 16.113 (Microsoft 365) Forward-looking: where DeckCraft is going and in what order. The one-page summary is [ROADMAP.md](../ROADMAP.md); every shortfall is in [gaps.md](gaps.md); numbers are in @@ -115,6 +115,7 @@ plan above. | Date | Change | Summary | |---|---|---| +| 2026-10-14 | trivial | M13 reliability: remote MCP validates IDs and envelopes, caps replies and rejects incomplete frames; #718 | | 2026-10-14 | minor | Community PR wave folded: one Review gate row, one Current focus item 2, one beta row each for items 9 and 10 (timeline and timer controls are in), one after-beta paragraph, one M9 and M10 row, one corpus row in the 2026-10-07 history | | 2026-10-13 | trivial | Beta item 3: embedded fonts are in, except MicroType Express subsets | | 2026-10-13 | minor | M9: the WordArt Transform gallery applies a text warp; adjust handles stay open |