diff --git a/apps/solidcraft/src/control_server.rs b/apps/solidcraft/src/control_server.rs index 4f1e8b70b7..f7635e17b3 100644 --- a/apps/solidcraft/src/control_server.rs +++ b/apps/solidcraft/src/control_server.rs @@ -475,6 +475,14 @@ fn send_events(design: &DesignWatch, lines: &Lines, stop: &AtomicBool, mut seen: } } +/// What this app's control channel does beyond the methods every version has, for a client to +/// find out before it relies on it (an older app answers `control.capabilities` with "unknown +/// method"). `param_check`: the levels `engine.execute`, `engine.batch` and `engine.script` take +/// as a request's own parameter check (#9184). +fn capabilities() -> Value { + json!({"param_check": solidcraft_engine::schema::ParamCheck::ALL.map(|l| l.name())}) +} + /// Write `reply` on the connection: through its subscription once it has one. fn answer(out: &mut TcpStream, events: Option<&Subscription>, reply: &Value) -> std::io::Result<()> { match events { @@ -535,15 +543,20 @@ fn serve_with( let id = msg.get("id").cloned().unwrap_or(Value::Null); let method = msg.get("method").and_then(Value::as_str).unwrap_or("").to_string(); let params = msg.get("params").cloned().unwrap_or(json!({})); - // A request that timed out may have finished since (#1618). `control.running` only - // looks, so it leaves that reply where it is. - let late = pending.as_ref().filter(|_| method != "control.running").and_then(|p| match p.reply.try_recv() { + // A request that timed out may have finished since (#1618). `control.running` and + // `control.capabilities` only look, so they leave that reply where it is. + let late = pending.as_ref().filter(|_| !matches!(method.as_str(), "control.running" | "control.capabilities")).and_then(|p| match p + .reply + .try_recv() + { Ok(reply) => Some(reply), Err(TryRecvError::Disconnected) => Some(json!({"ok": false, "error": "the app ended the request without a reply"})), Err(TryRecvError::Empty) => None, }); let mut r = if method == "control.running" { json!({"ok": true, "result": running.result(connection)}) + } else if method == "control.capabilities" { + json!({"ok": true, "result": capabilities()}) } else if method == "control.pending" { match (pending.take(), late) { (None, _) => json!({"ok": true, "result": {"pending": null}}), @@ -750,6 +763,9 @@ mod tests { let mut watcher = open(); let (r, rows) = running(&mut watcher, &|_| true); assert!(r["ok"] == true && rows.is_empty(), "idle: {r}"); + // What the app's control channel does beyond the methods every version has (#9184). + let r = watcher(json!({"id": 0, "method": "control.capabilities", "token": token})); + assert_eq!((&r["ok"], &r["result"]["param_check"]), (&json!(true), &json!(["off", "warn", "strict"])), "{r}"); let mut export = open(); let path = "/private/part.step"; @@ -781,6 +797,9 @@ mod tests { let own: Vec<&Value> = r["result"]["running"].as_array().unwrap().iter().filter(|row| row["self"] == true).collect(); assert_eq!(own.len(), 1, "{r}"); assert_eq!(own[0]["command"], "file.export", "{r}"); + // So may it ask what the app can do: answered at once, not refused as busy. + let r = export(json!({"id": 8, "method": "control.capabilities"})); + assert_eq!(r["result"]["param_check"], json!(["off", "warn", "strict"]), "{r}"); gate_tx.send(()).unwrap(); assert_eq!(queued.join().unwrap()["result"], "engine.batch"); diff --git a/crates/engine/src/schema/check.rs b/crates/engine/src/schema/check.rs index e9a11e3c35..373f323a8f 100644 --- a/crates/engine/src/schema/check.rs +++ b/crates/engine/src/schema/check.rs @@ -28,6 +28,9 @@ pub enum ParamCheck { } impl ParamCheck { + /// Every level, least checking first. + pub const ALL: [ParamCheck; 3] = [ParamCheck::Off, ParamCheck::Warn, ParamCheck::Strict]; + pub fn parse(s: &str) -> Option { match s.trim().to_ascii_lowercase().as_str() { "off" => Some(ParamCheck::Off), @@ -36,6 +39,15 @@ impl ParamCheck { _ => None, } } + + /// The level's word, as [`ParamCheck::parse`] reads it (the control channel's `param_check`). + pub fn name(self) -> &'static str { + match self { + ParamCheck::Off => "off", + ParamCheck::Warn => "warn", + ParamCheck::Strict => "strict", + } + } } /// The JSON Schema (2020-12) of what deserializes into `T`: a command's parameters. diff --git a/crates/engine/src/schema/tests.rs b/crates/engine/src/schema/tests.rs index 966478d3de..bb698045f9 100644 --- a/crates/engine/src/schema/tests.rs +++ b/crates/engine/src/schema/tests.rs @@ -361,6 +361,13 @@ fn the_level_survives_a_new_design() { assert_eq!(s.scratch().param_check, ParamCheck::Strict); } +#[test] +fn a_level_is_read_back_from_its_name() { + for level in ParamCheck::ALL { + assert_eq!(ParamCheck::parse(level.name()), Some(level)); + } +} + /// Whether `v` conforms to `node` of the schema `root`, for the keywords these schemas use /// apart from `pattern`: `type`, `properties`, `required`, `additionalProperties: false`, /// `items`, `minItems`, `maxItems`, `uniqueItems`, `enum`, `minLength`, `maxLength`, `minimum`, diff --git a/crates/mcp/src/backend.rs b/crates/mcp/src/backend.rs index 4bc2fcd1a3..937267ba96 100644 --- a/crates/mcp/src/backend.rs +++ b/crates/mcp/src/backend.rs @@ -35,7 +35,7 @@ pub trait Backend { None } /// How calls are checked against their commands' typed contracts (`--param-check`): by the - /// headless session, or by `Remote` before it sends them to the app. + /// headless session, or for `Remote` by the app or before the call is sent to it. fn param_check(&self) -> ParamCheck { ParamCheck::Off } @@ -112,6 +112,11 @@ pub(crate) struct Waits { const WAITS: Waits = Waits { pending: PENDING_WAIT, settle: SETTLE_WAIT, poll: POLL_EVERY }; +/// The level a control reply says its request was checked at (#9184), if it says one. +fn echoed(v: &Value) -> Option { + v.get("param_check").and_then(Value::as_str).and_then(ParamCheck::parse) +} + /// A control reply as a call's result. fn outcome(v: &Value) -> Result { if v.get("ok").and_then(Value::as_bool) == Some(true) { @@ -136,8 +141,14 @@ pub struct Remote { /// An outstanding request's end, once a later call waited for it (for `pending`). settled: Option, waits: Waits, - /// How calls are checked before they are sent (`--param-check`). + /// How calls are checked (`--param-check`): by the app, when it checks a request's own + /// level, else here before they are sent. param_check: ParamCheck, + /// Whether the connected app checks a request's `param_check` itself (#9184), asked once per + /// connection; `None` until asked. + app_checks: Option, + /// The level the app said it checked the last reply at (`param_check` beside `ok`). + echoed: Option, } /// How a tool error starts when no app listens at the bridged address. The agent reads it, and @@ -173,9 +184,18 @@ impl Remote { settled: None, waits: WAITS, param_check: ParamCheck::Off, + app_checks: None, + echoed: None, } } + /// Check calls against their commands' contracts at `level` (`--param-check`): an app that + /// checks a request's own level is sent it, and an older one gets only the calls that pass. + pub fn with_param_check(mut self, level: ParamCheck) -> Self { + self.param_check = level; + self + } + /// Shorter waits for requests still running (tests). #[cfg(test)] pub(crate) fn set_waits(&mut self, waits: Waits) { @@ -185,6 +205,8 @@ impl Remote { /// Close the connection. A request still running loses its reply with it. fn drop_conn(&mut self) { self.conn = None; + // The next connection may reach another app (restarted, another version). + self.app_checks = None; if let Some(o) = self.outstanding.take() { self.settled = Some(Settled::Lost { what: o.what }); } @@ -407,6 +429,7 @@ impl Remote { if method != "control.running" { self.settle_earlier()?; } + self.echoed = None; let what = control_pending::describe(method, ¶ms); let effect = control_pending::effect(method, ¶ms); let again = retry_busy.then(|| params.clone()); @@ -424,6 +447,7 @@ impl Remote { } .map_err(|e| self.explain(&e))?; let v: Value = serde_json::from_str(&reply).map_err(|e| format!("bad reply from the app: {e}"))?; + self.echoed = echoed(&v); // Timed out in the app, still running (#1618): wait for its result on this connection. if v.get("running").and_then(Value::as_bool) == Some(true) { self.outstanding = Some(Outstanding { what: what.clone(), effect: Some(effect), since: sent }); @@ -433,7 +457,10 @@ impl Remote { self.waits.pending.as_secs() ); return match self.wait_outstanding(self.waits.pending)? { - Waited::Done(reply) => outcome(&reply), + Waited::Done(reply) => { + self.echoed = echoed(&reply); + outcome(&reply) + } Waited::Nothing => { Err(format!("{what} timed out in the app, which kept no result for it: inspect the design before sending it again")) } @@ -464,28 +491,76 @@ fn contract_issues(id: &str, params: &Value) -> Vec { contract.map(|c| c.check(params).into_iter().map(|i| i.message).collect()).unwrap_or_default() } -/// `reply` with `issues` as its `param_warnings`, as a headless session at the warn level adds -/// them; a reply that has its own (an app that checked the call itself) keeps those. -fn with_param_warnings(mut reply: Value, issues: Vec) -> Value { +/// Give `reply` `issues` as its `param_warnings`, as a headless session at the warn level does; a +/// reply that has its own (an app that checked the call itself) keeps those. +fn add_param_warnings(reply: &mut Value, issues: Vec) { if !issues.is_empty() && let Some(o) = reply.as_object_mut() { o.entry("param_warnings").or_insert_with(|| json!(issues)); } - reply } impl Remote { - /// Check calls against their commands' contracts at `level` before sending them - /// (`--param-check`): the app's own session checks nothing. - pub fn with_param_check(mut self, level: ParamCheck) -> Self { - self.param_check = level; - self + /// Whether the app checks a request's `param_check` itself (#9184): asked once per + /// connection with `control.capabilities`, which lists the levels it takes; an older app + /// answers "unknown method". Any other answer (no app, a busy one) is asked again next time. + fn app_checks(&mut self) -> bool { + if let Some(known) = self.app_checks { + return known; + } + // Sent ahead of settling a request still running on this connection: the app answers + // `control.capabilities` at once, as `control.running`, without taking that one's reply. + let line = self.request_line("control.capabilities", json!({})); + let reply = self.roundtrip(&line).ok().and_then(|r| serde_json::from_str::(&r).ok()); + let known = match reply.as_ref().map(outcome) { + Some(Ok(c)) => Some(c["param_check"].as_array().is_some_and(|levels| levels.iter().any(|l| l == self.param_check.name()))), + Some(Err(e)) if e.starts_with("unknown method") => Some(false), + _ => None, + }; + if self.conn.is_some() { + self.app_checks = known; + } + known.unwrap_or(false) } - /// `engine.execute` checked here: `strict` refuses a call that breaks its command's contract - /// without sending it, and `warn` sends it and adds the issues to the app's reply. + /// The reply to a call sent with this bridge's level: at `strict`, one that doesn't say the + /// app checked it at that level is an error, since the call may have run unchecked; and the + /// next calls are checked here. + fn confirmed(&mut self, reply: Result) -> Result { + let unconfirmed = reply.is_ok() && self.echoed != Some(self.param_check); + if unconfirmed { + self.app_checks = Some(false); + } + match reply { + Ok(_) if unconfirmed && self.param_check == ParamCheck::Strict => { + Err("the app did not say it checked this call at --param-check strict, so it may have run unchecked: inspect the design \ + before sending it again (the next calls are checked here before they are sent)" + .into()) + } + other => other, + } + } + + /// `params` with this bridge's level as the request's `param_check`, for an app that checks it. + fn at_level(&self, mut params: Value) -> Value { + if let Some(o) = params.as_object_mut() { + o.insert("param_check".into(), json!(self.param_check.name())); + } + params + } + + /// `engine.execute` checked at this bridge's level. An app that checks a request's level is + /// sent it, and checks against its own commands (its reply must say so, see `confirmed`). + /// For an older one the check is made here: + /// `strict` refuses a call that breaks its command's contract without sending it, and `warn` + /// sends it and adds the issues to the app's reply. fn execute_checked(&mut self, params: Value) -> Result { + if self.app_checks() { + let params = self.at_level(params); + let reply = self.call_once("engine.execute", params, true); + return self.confirmed(reply); + } let id = params.get("command").and_then(Value::as_str).unwrap_or_default(); let issues = contract_issues(id, params.get("params").unwrap_or(&Value::Null)); if self.param_check == ParamCheck::Strict @@ -493,14 +568,21 @@ impl Remote { { return Err(solidcraft_engine::params::bad(id, first.as_str()).to_string()); } - let reply = self.call_once("engine.execute", params, true)?; - Ok(with_param_warnings(reply, issues)) + let mut reply = self.call_once("engine.execute", params, true)?; + add_param_warnings(&mut reply, issues); + Ok(reply) } - /// `engine.batch` checked here: `strict` refuses a batch with a step that breaks its command's - /// contract before anything is sent, as the app would refuse it unrun, and `warn` adds each - /// step's issues to its result. + /// `engine.batch` checked at this bridge's level, as `execute_checked` checks a call: by an + /// app that checks a request's level, else here, where `strict` refuses a batch with a step + /// that breaks its command's contract before anything is sent, as the app would refuse it + /// unrun, and `warn` adds each step's issues to its result. fn batch_checked(&mut self, params: Value) -> Result { + if self.app_checks() { + let params = self.at_level(params); + let reply = self.call_once("engine.batch", params, true); + return self.confirmed(reply); + } let steps = params.get("commands").and_then(Value::as_array).map(Vec::as_slice).unwrap_or_default(); if self.param_check == ParamCheck::Strict { if let Some(refusal) = solidcraft_engine::batch_preflight::contracts(steps) { @@ -518,7 +600,7 @@ impl Remote { let mut reply = self.call_once("engine.batch", params, true)?; if let Some(results) = reply.get_mut("results").and_then(Value::as_array_mut) { for (result, issues) in results.iter_mut().zip(issues) { - *result = with_param_warnings(std::mem::take(result), issues); + add_param_warnings(result, issues); } } Ok(reply) diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index a9883270cd..6c897500ec 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -2374,6 +2374,70 @@ fn a_bridged_server_checks_calls_at_its_param_check_level() { assert_eq!(r.get("param_warnings"), None, "{r}"); } +/// Bridged to an app that checks a request's own `param_check` (#9184), the server asks it once +/// per connection (`control.capabilities`) and then sends every call with its level, unchecked +/// here: the app checks against its own commands, so a key only a newer app knows is not +/// refused, and its answer is the one the agent sees. +#[test] +fn a_bridged_server_leaves_the_check_to_an_app_that_makes_it() { + use solidcraft_engine::schema::ParamCheck; + let levels = std::sync::Arc::new(std::sync::Mutex::new(Vec::::new())); + let log = levels.clone(); + let (addr, seen) = scripted_app(move |req| { + let level = req["params"]["param_check"].clone(); + match req["method"].as_str() { + Some("control.capabilities") => json!({"ok": true, "result": {"param_check": ["off", "warn", "strict"]}}), + Some("engine.batch") => { + log.lock().unwrap().push(level.clone()); + json!({"ok": true, "result": {"completed": 1, "results": [{"param_warnings": ["the app's own"]}], "undone": 0}, "param_check": level}) + } + _ => { + log.lock().unwrap().push(level.clone()); + json!({"ok": true, "result": {"created_bodies": ["Box1"], "param_warnings": ["the app's own"]}, "param_check": level}) + } + } + }); + let remote = crate::Remote::connect_with_token(&addr, Some("0123456789abcdef0123456789abcdef")).unwrap().with_param_check(ParamCheck::Strict); + let mut s = Server::new(Box::new(remote)); + let sent = |what: &str| seen.lock().unwrap().1.iter().filter(|w| w.as_str() == what).count(); + let newer = json!({"command": "solid.box", "params": {"length": 5, "width": 5, "height": 5, "a_newer_key": 1}}); + + for _ in 0..2 { + let r = tool(&mut s, "execute", newer.clone()); + assert_eq!(r["param_warnings"], json!(["the app's own"]), "{r}"); + } + assert_eq!((sent("engine.execute solid.box"), sent("control.capabilities")), (2, 1), "asked once, both sent"); + // A batch goes on a fresh connection, so the next call asks again. + let r = tool(&mut s, "batch", json!({"commands": [newer.clone()]})); + assert_eq!(r["results"][0]["param_warnings"], json!(["the app's own"]), "{r}"); + tool(&mut s, "execute", newer); + assert_eq!(sent("control.capabilities"), 2); + assert!(levels.lock().unwrap().iter().all(|l| l == "strict"), "{:?}", levels.lock().unwrap()); +} + +/// An app that says it checks `param_check` but answers a call without saying it checked it at +/// the bridge's level (#9184), with no echo or another level: at `strict` that is an error, as the +/// call may have run unchecked, and from then on the server checks the calls itself. +#[test] +fn a_bridged_server_at_strict_refuses_an_unconfirmed_reply() { + use solidcraft_engine::schema::ParamCheck; + for echo in [Value::Null, json!("off")] { + let (addr, seen) = scripted_app(move |req| match req["method"].as_str() { + Some("control.capabilities") => json!({"ok": true, "result": {"param_check": ["off", "warn", "strict"]}}), + _ => json!({"ok": true, "result": {"created_bodies": ["Box1"]}, "param_check": echo}), + }); + let remote = crate::Remote::connect_with_token(&addr, Some("0123456789abcdef0123456789abcdef")).unwrap().with_param_check(ParamCheck::Strict); + let mut s = Server::new(Box::new(remote)); + let sent = |what: &str| seen.lock().unwrap().1.iter().filter(|w| w.as_str() == what).count(); + let r = raw_tool(&mut s, "execute", json!({"command": "solid.sphere", "params": {"radius": 2}})); + assert_eq!(r["result"]["isError"], json!(true), "{r}"); + assert!(r.to_string().contains("did not say it checked this call"), "{r}"); + let r = raw_tool(&mut s, "execute", json!({"command": "solid.box", "params": {"length": 5, "width": 5, "height": 5, "operaton": "cut"}})); + assert!(r.to_string().contains("unknown key `operaton`"), "checked here now: {r}"); + assert_eq!(sent("engine.execute solid.box"), 0, "refused unsent"); + } +} + /// Plain `mcp` with no app running: the session is headless, says so on every result, and bridges /// to the app once one runs, but only while it holds no work of its own. #[test] diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 298107eb49..1232654496 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -49,6 +49,7 @@ use std::time::Duration; use serde_json::{Value, json}; use solidcraft_engine::io::vfs; use solidcraft_engine::render::{Camera, StandardView}; +use solidcraft_engine::schema::ParamCheck; use solidcraft_engine::{batch_params, timing}; use crate::SolveApp; @@ -452,17 +453,86 @@ pub fn view_png(app: &SolveApp, w: u32, h: u32) -> Result, String> { } pub fn handle(app: &mut SolveApp, ctx: &egui::Context, req: &ControlRequest) -> Outcome { - let scope = match request_scope(app, &req.params) { - Ok(Some(scope)) => scope, - Ok(None) => return dispatch(app, ctx, &req.method, &req.params), + let level = match request_param_check(&req.method, &req.params) { + Ok(level) => level, Err(e) => return err(e), }; + let scope = match request_scope(app, &req.params) { + Ok(scope) => scope, + Err(e) => return err(e), + }; + if scope.is_none() && level.is_none() { + return dispatch(app, ctx, &req.method, &req.params); + } let mut p = req.params.clone(); if let Some(o) = p.as_object_mut() { o.remove("allow_paths"); o.remove("design_folders"); + o.remove("param_check"); } - vfs::scoped(Some(scope), || dispatch(app, ctx, &req.method, &p)) + let run = |app: &mut SolveApp| match level { + Some(level) => dispatch_checked(app, ctx, &req.method, &p, level), + None => dispatch(app, ctx, &req.method, &p), + }; + match scope { + Some(scope) => vfs::scoped(Some(scope), || run(app)), + None => run(app), + } +} + +/// The request's `param_check` (#9184): how the commands of an `engine.execute`, `engine.batch` +/// or `engine.script` are checked against their contracts, whatever the app's own session level; +/// `None` when it names none (other methods ignore the field). +pub(crate) fn request_param_check(method: &str, params: &Value) -> Result, String> { + if !matches!(method, "engine.execute" | "command" | "engine.batch" | "engine.script") { + return Ok(None); + } + match params.get("param_check") { + None => Ok(None), + Some(v) => v.as_str().and_then(ParamCheck::parse).map(Some).ok_or_else(|| "`param_check` must be off, warn or strict".to_string()), + } +} + +/// `dispatch` of an `engine.execute`, `engine.batch` or `engine.script` with its commands checked +/// at `level`, echoed in the reply as `param_check`, so a client can tell an app that checked +/// from an older one that ignored the field. The session goes back to its own level afterwards. +fn dispatch_checked(app: &mut SolveApp, ctx: &egui::Context, method: &str, p: &Value, level: ParamCheck) -> Outcome { + let mut out = if switches_design(p).is_some() { + switch_checked(app, ctx, method, p, level) + } else { + let own = app.session.param_check; + app.session.param_check = level; + let out = dispatch(app, ctx, method, p); + app.session.param_check = own; + out + }; + if let Outcome::Done(Value::Object(o)) = &mut out { + o.insert("param_check".into(), json!(level.name())); + } + out +} + +/// A command that switches to another design, checked at `level` before it runs: it runs at the +/// app's own level, since the session it leaves and the one it brings in each keep theirs. +fn switch_checked(app: &mut SolveApp, ctx: &egui::Context, method: &str, p: &Value, level: ParamCheck) -> Outcome { + let id = command(p).unwrap_or_default(); + let contract = solidcraft_engine::find_command(id).and_then(|c| c.contract); + let issues: Vec = + contract.map(|c| c.check(p.get("params").unwrap_or(&Value::Null)).into_iter().map(|i| i.message).collect()).unwrap_or_default(); + if level == ParamCheck::Strict + && let Some(first) = issues.first() + { + return err(solidcraft_engine::params::bad(id, first.as_str())); + } + let mut out = dispatch(app, ctx, method, p); + if level == ParamCheck::Warn + && !issues.is_empty() + && let Outcome::Done(v) = &mut out + && let Some(o) = v.get_mut("result").and_then(Value::as_object_mut) + { + o.insert("param_warnings".into(), json!(issues)); + } + out } fn dispatch(app: &mut SolveApp, ctx: &egui::Context, method: &str, p: &Value) -> Outcome { @@ -1916,6 +1986,60 @@ mod tests { assert!(!v["result"]["failed"]["error"].as_str().is_some_and(|e| e.starts_with("unknown command")), "{v}"); } + /// A request's own `param_check` (#9184): `engine.execute`, `engine.batch` and `engine.script` + /// run at it and say so in the reply, whatever the app's session level, which is its own again + /// afterwards. A command that switches designs keeps the app's level. + #[test] + fn a_request_carries_its_own_param_check_level() { + use solidcraft_engine::schema::ParamCheck; + let mut h = crate::scenario::Harness::new(); + let before = inspect(&mut h); + let warning = "unknown key `operaton`; did you mean `operation`?"; + let misspelt = |level: &str| json!({"command": "solid.box", "params": {"length": 10, "width": 10, "height": 10, "operaton": "cut"}, "param_check": level}); + + let v = h.call("engine.execute", misspelt("strict")); + assert_eq!((&v["ok"], &v["param_check"]), (&json!(false), &json!("strict")), "{v}"); + assert!(v["error"].as_str().is_some_and(|e| e.contains(warning)), "{v}"); + assert_eq!(inspect(&mut h)["bodies"], before["bodies"], "refused unrun"); + assert_eq!(h.app.session.param_check, ParamCheck::Off, "the app's own level again"); + + let v = h.call("engine.execute", misspelt("warn")); + assert_eq!((&v["ok"], &v["param_check"], &v["result"]["param_warnings"]), (&json!(true), &json!("warn"), &json!([warning])), "{v}"); + assert_eq!(h.app.session.param_check, ParamCheck::Off); + + let steps = json!([ + {"command": "solid.sphere", "params": {"radius": 3, "center": [50, 0, 0]}}, + {"command": "solid.box", "params": {"lenght": 10, "width": 10, "height": 10}}, + ]); + let shown = inspect(&mut h)["bodies"].clone(); + let v = h.call("engine.batch", json!({"commands": steps, "param_check": "strict"})); + assert_eq!((&v["result"]["completed"], &v["result"]["failed"]["index"], &v["param_check"]), (&json!(0), &json!(1), &json!("strict")), "{v}"); + assert_eq!(inspect(&mut h)["bodies"], shown, "nothing ran"); + let step = json!({"command": "solid.box", "params": {"length": 10, "width": 10, "height": 10, "operaton": "cut"}}); + let v = h.call("engine.script", json!({"commands": [step], "param_check": "warn"})); + assert_eq!((&v["param_check"], &v["result"][0]["param_warnings"]), (&json!("warn"), &json!([warning])), "{v}"); + + // Asked nothing, told nothing; a word that is no level is refused. + let v = h.call("engine.execute", json!({"command": "inspect.measure", "params": {}})); + assert_eq!((&v["ok"], v.get("param_check")), (&json!(true), None), "{v}"); + let v = h.call("engine.execute", json!({"command": "inspect.measure", "params": {}, "param_check": "loud"})); + assert!(v["ok"] == false && v["error"].as_str().is_some_and(|e| e.contains("`param_check` must be off, warn or strict")), "{v}"); + // The level is this request's, not another design's. + let v = h.call("engine.execute", json!({"command": "view.next_tab", "params": {}, "param_check": "strict"})); + assert_eq!((&v["ok"], &v["param_check"]), (&json!(true), &json!("strict")), "{v}"); + assert_eq!(h.app.session.param_check, ParamCheck::Off); + // Its own parameters are still checked at the request's level, before it runs. + let v = h.call("engine.execute", json!({"command": "view.next_tab", "params": {"nonsense": 1}, "param_check": "strict"})); + assert!(v["ok"] == false && v["error"].as_str().is_some_and(|e| e.contains("nonsense")), "{v}"); + assert_eq!(v["param_check"], "strict", "{v}"); + let v = h.call("engine.execute", json!({"command": "view.next_tab", "params": {"nonsense": 1}, "param_check": "warn"})); + assert!(v["result"]["param_warnings"].as_array().is_some_and(|w| !w.is_empty()), "{v}"); + assert_eq!(h.app.session.param_check, ParamCheck::Off); + // Other methods ignore the field, whatever it holds. + let v = h.call("ui.inspect", json!({"param_check": "loud"})); + assert_eq!((&v["ok"], v.get("param_check")), (&json!(true), None), "{v}"); + } + #[test] fn engine_batch_is_refused_when_a_step_loads_a_file() { let mut h = crate::scenario::Harness::new(); diff --git a/crates/ui-egui/src/control_job.rs b/crates/ui-egui/src/control_job.rs index fe2c45ebaf..33829820fd 100644 --- a/crates/ui-egui/src/control_job.rs +++ b/crates/ui-egui/src/control_job.rs @@ -406,12 +406,22 @@ impl SolveApp { /// is taken care of; `Some`: it runs here (no threads). fn answer_on_a_copy(&mut self, req: ControlRequest, id: &'static str, params: Value) -> Option { let Ok(scope) = crate::control::request_scope(self, &req.params) else { return Some(req) }; + // The request's own level (#9184) checks it on the copy, and the reply says so, as + // `control::handle` does; a word that is no level is refused there. + let Ok(level) = crate::control::request_param_check(&req.method, &req.params) else { return Some(req) }; let mut copy = self.session.scratch(); + if let Some(level) = level { + copy.param_check = level; + } let rx = spawn(scope, move || { - Some(match copy.execute(id, ¶ms) { + let mut reply = match copy.execute(id, ¶ms) { Ok(v) => json!({"ok": true, "result": v}), Err(e) => json!({"ok": false, "error": e.to_string()}), - }) + }; + if let Some(level) = level { + reply["param_check"] = json!(level.name()); + } + Some(reply) }); let Some(rx) = rx else { return Some(req) }; let job = ControlJob { label: label(self, id), req, work: Work::Answer, done: rx, started: crate::now_ms(), steps: None }; diff --git a/crates/ui-egui/src/history_off_thread_tests.rs b/crates/ui-egui/src/history_off_thread_tests.rs index 8e898d2e97..702b72bce4 100644 --- a/crates/ui-egui/src/history_off_thread_tests.rs +++ b/crates/ui-egui/src/history_off_thread_tests.rs @@ -682,6 +682,43 @@ fn a_measure_reads_a_copy_off_the_window_thread_preference_or_not() { assert_eq!(h.app.session.log.len(), log, "a reading leaves no echo: {:?}", h.app.session.log); } +/// A reading on a copy keeps the request's own `param_check` (#9184), as one run here does: its +/// reply says the level, and a word that is no level is refused. +#[test] +fn a_reading_on_a_copy_keeps_the_requests_param_check() { + let (mut h, tx) = agent_harness(); + let measure = ask(&tx, "engine.execute", json!({"command": "inspect.measure", "params": {}, "param_check": "strict"})); + h.frames(1); + assert_eq!(h.app.control_answering.len(), 1, "read a copy"); + let r = reply_to(&mut h, &measure); + assert_eq!((&r["ok"], &r["param_check"]), (&json!(true), &json!("strict")), "{r}"); + let loud = ask(&tx, "engine.execute", json!({"command": "inspect.measure", "params": {}, "param_check": "loud"})); + let r = reply_to(&mut h, &loud); + assert!(r["ok"] == false && r["error"].as_str().is_some_and(|e| e.contains("`param_check` must be off, warn or strict")), "{r}"); + // A design change computed ahead off the window thread runs here at the request's level. + let features = h.app.session.doc.features.len(); + let misspelt = |level: &str| { + let mut call = second_hole(); + call["params"]["operaton"] = json!("cut"); + call["param_check"] = json!(level); + call + }; + let strict = ask(&tx, "engine.execute", misspelt("strict")); + h.frames(1); + assert!(h.app.agent_computing().is_some(), "computed ahead off the window thread"); + let r = reply_to(&mut h, &strict); + assert_eq!((&r["ok"], &r["param_check"]), (&json!(false), &json!("strict")), "{r}"); + assert_eq!(h.app.session.doc.features.len(), features, "refused unrun"); + let warn = ask(&tx, "engine.execute", misspelt("warn")); + let r = reply_to(&mut h, &warn); + assert_eq!( + (&r["param_check"], &r["result"]["param_warnings"]), + (&json!("warn"), &json!(["unknown key `operaton`; did you mean `operation`?"])), + "{r}" + ); + assert_eq!(h.app.session.param_check, solidcraft_engine::schema::ParamCheck::Off, "the app's own level again"); +} + /// A clearance or interference check of every pair of bodies reads a copy off the window thread /// too: on an assembly of 150 bodies it computed for minutes with the window not responding. The /// answers are the ones running it here gives. diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 792b7fd50b..53971e63ba 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -129,6 +129,12 @@ turn behind another. `command` is the command id of `engine.execute` (a known on paths and expressions stay with the connection that sent them. `self` marks the asking connection's own request. `{"running": []}` when the app is idle. +`control.capabilities` says what this app's control channel does beyond the methods every +version has, so a client can find out before it relies on it: `{"param_check": ["off", "warn", +"strict"]}`, the levels a request's own `param_check` takes (below). It is answered at once, like +`control.running`, also on a `busy` connection. An app older than it answers ``unknown method +`control.capabilities` ``; a feature added later is listed here too. + Any request's `params` may carry `allow_paths: ["/abs/folder", …]`: every file that request reads, writes or deletes (command paths such as `file.export`'s `path` or `doc.recovery_*`'s `dir`, `ui.render`, `ui.screenshot`) must then lie inside one of those folders after `..` and @@ -140,6 +146,20 @@ or saved a design in; each counts on the open design folder's terms. The bridged MCP server sends its folders this way on every request ([mcp.md](mcp.md)). Without `allow_paths` file paths are not limited: a client holding the token can do what the user can. +`engine.execute`, `engine.batch` and `engine.script` may carry `param_check: "off" | "warn" | +"strict"` in their `params`: the commands of that request are checked against their typed +contracts at that level, whatever the app's own (which is `off`), as a headless session's +`--param-check` checks them (#9184). `warn` runs a call that breaks its contract and adds +`param_warnings` to its result; `strict` refuses it (a batch before its first step), and the +design is left as it was. Every reply to such a request says the level it was checked at as +`param_check`, beside `ok`. A command that switches to another design (`component.edit_in_place`, +`component.finish_edit`, `view.next_tab`, `view.previous_tab`) has its parameters checked at +that level before it runs, and then runs at the app's own level, as each design keeps its own. +Any other word is refused; other methods ignore the field. An app older than this ignores the field and its reply has no +`param_check`, so a client that needs the check asks `control.capabilities` first and treats a +reply without `param_check` as unchecked. The bridged MCP server asks once per connection +([mcp.md](mcp.md)). + ## Engine | Method | Params | Result | diff --git a/docs/mcp.md b/docs/mcp.md index 69bc20962c..3bd30f84bb 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -71,11 +71,17 @@ are converted; `describe_command` shows a command's schemas) can be checked as t recipe given with `--in`, and for calls bridged to the app; the default is `off`, which changes nothing. `warn` runs the call and adds `param_warnings` to its result, such as ``unknown key `operaton`; did you mean `operation`?`` (a starting script's warnings go to stderr). `strict` -refuses the call, and the design is left as it was. Bridged, the server checks each call (and -each step of a `batch`) against its own commands' contracts before sending it: `strict` sends -nothing that breaks one, and a command the server has no contract for goes to the app unchecked. -Use the server from the same release as the app: an app newer than the server may take keys the -server's contracts don't know yet, and `strict` would refuse them. +refuses the call, and the design is left as it was. Bridged, the server asks the app once per +connection whether it checks a request's own level (`control.capabilities` and `param_check` in +[control-protocol.md](control-protocol.md)). If it does, every call and `batch` is sent with the +level and the app checks it against its own commands, so a newer app's keys are not refused. At +`strict`, a reply that doesn't say the app checked the call at that level is an error (it may +have run unchecked: inspect the design), and the server checks the next calls itself. +An older app is checked here instead, against the server's own commands' contracts, before +anything is sent: `strict` sends nothing that breaks one, and a command the server has no +contract for goes to the app unchecked. With such an app, use the server from the same release: +an app newer than the server may take keys the server's contracts don't know yet, and `strict` +would refuse them. ### Which files the agent can reach