mirror of
https://github.com/bherbruck/solidcraft.git
synced 2026-11-03 02:25:55 +00:00
* claim: #9184 * Control channel: a request's own param_check level, applied and confirmed by the app (#9184) `engine.execute`, `engine.batch` and `engine.script` take an optional `param_check: "off"|"warn"|"strict"` in their params. The app runs the request at that level through the same `run_checked` path and batch preflight a headless session uses, puts its own level back, and every reply says the level it ran at as `param_check` beside `ok`. A command that switches designs keeps the app's level; any other word is refused. Readings and exports answered on a copy off the window thread keep the level too. `control.capabilities`, answered at once on the control-server thread, lists what the channel does beyond the methods every version has: `{"param_check": ["off", "warn", "strict"]}`. An older app answers "unknown method". The MCP bridge asks it once per connection. When the app lists the level, every call and batch goes with it and the app checks against its own commands; otherwise the bridge checks before sending, as #9073 does. At strict, a reply that doesn't say the app checked the call is an error, and the next calls are checked by the bridge. Also: `add_param_warnings` takes the reply by `&mut`, and `with_param_check` sits beside `Remote::new`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Control channel: a design-switching command is checked at the request's level Review fixes for #9184. `component.edit_in_place`, `component.finish_edit`, `view.next_tab` and `view.previous_tab` ran at the app's own level and echoed `off`, so a misspelt call went through at strict. They now have their parameters checked at the request's level before they run (and still run at the app's own level, as each design keeps its own), and the reply echoes the requested level. The MCP bridge at strict now requires the echo to match its level, not just to be there. `param_check` is read only by `engine.execute`, `engine.batch` and `engine.script`; other methods ignore it. The off-thread test now shows the design change computing off the window thread before it is refused. 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:
@@ -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");
|
||||
|
||||
@@ -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<ParamCheck> {
|
||||
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.
|
||||
|
||||
@@ -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`,
|
||||
|
||||
+102
-20
@@ -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<ParamCheck> {
|
||||
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<Value, String> {
|
||||
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<Settled>,
|
||||
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<bool>,
|
||||
/// The level the app said it checked the last reply at (`param_check` beside `ok`).
|
||||
echoed: Option<ParamCheck>,
|
||||
}
|
||||
|
||||
/// 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<String> {
|
||||
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<String>) -> 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<String>) {
|
||||
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::<Value>(&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<Value, String>) -> Result<Value, String> {
|
||||
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<Value, String> {
|
||||
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<Value, String> {
|
||||
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)
|
||||
|
||||
@@ -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::<Value>::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]
|
||||
|
||||
@@ -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<Vec<u8>, 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<Option<ParamCheck>, 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<String> =
|
||||
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();
|
||||
|
||||
@@ -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<ControlRequest> {
|
||||
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 };
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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 |
|
||||
|
||||
+11
-5
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user