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 `<id>`: 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * `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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
BFlat
GitHub
parent
8f0f95959d
commit
7eb3b2e072
|
|
@ -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 <file> or --new <json>".into()),
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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]
|
||||
|
|
|
|||
|
|
@ -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 |
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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());
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<usize>) -> Result<usize, AutomationError> {
|
||||
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)?);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<Value, AutomationError> {
|
||||
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<Value, AutomationError> {
|
||||
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<Value, AutomationError> {
|
||||
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<Headless>, 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<Headless>, 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<Value> = 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()));
|
||||
|
|
|
|||
|
|
@ -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<LineRead> {
|
||||
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<String, AutomationError> {
|
||||
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]
|
||||
|
|
|
|||
|
|
@ -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<Value> =
|
||||
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<RunParams> = (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));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<Started> {
|
||||
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));
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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]
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue