From 3d914c9f158828487a52d68111f8a9e3d3905732 Mon Sep 17 00:00:00 2001 From: Rec0iL Date: Fri, 16 Oct 2026 01:01:09 +0200 Subject: [PATCH] Agent chat gate: curve and occurrence ids named path are not files; broken tool arguments are not run (#1499) Co-Authored-By: Rec0iL Co-Authored-By: Claude Sonnet 5.5 --- crates/ui-egui/src/agent_chat/api_client.rs | 74 ++++++++++- crates/ui-egui/src/agent_chat/gate.rs | 134 ++++++++++++++++++-- docs/agent-chat.md | 6 + 3 files changed, 200 insertions(+), 14 deletions(-) diff --git a/crates/ui-egui/src/agent_chat/api_client.rs b/crates/ui-egui/src/agent_chat/api_client.rs index 49a89974..878270f1 100644 --- a/crates/ui-egui/src/agent_chat/api_client.rs +++ b/crates/ui-egui/src/agent_chat/api_client.rs @@ -225,10 +225,8 @@ impl Worker { for (i, c) in calls.iter().enumerate().take(32) { let id = c.get("id").and_then(Value::as_str).map_or_else(|| format!("call-{i}"), str::to_string); let name = c.pointer("/function/name").and_then(Value::as_str).unwrap_or("").to_string(); - let args: Value = c - .pointer("/function/arguments") - .and_then(|a| if let Some(s) = a.as_str() { serde_json::from_str(s).ok() } else { Some(a.clone()) }) - .unwrap_or(json!({})); + let parsed = tool_arguments(c.pointer("/function/arguments")); + let args = parsed.clone().unwrap_or_else(|_| json!({})); let title = tool_title(&name, &args); let tool_id = format!("{id}-{}", self.next); self.emit(Event::Tool { @@ -240,6 +238,9 @@ impl Worker { }); let (text, failed) = if self.cancel.load(Ordering::Relaxed) { ("cancelled by the user".to_string(), true) + } else if let Err(e) = &parsed { + // A call whose arguments do not parse is answered, never run with `{}`. + (format!("Not run: {e}"), true) } else if HIDDEN_TOOLS.contains(&name.as_str()) || !self.tools.iter().any(|t| t.pointer("/function/name").and_then(Value::as_str) == Some(name.as_str())) { @@ -307,6 +308,25 @@ pub fn http_error(status: u16, body: &Value) -> String { ) } +/// A tool call's arguments as an object: the JSON text a model sends (or an object, for servers +/// that send one), or why they cannot be used. Missing or empty arguments are an empty object. +fn tool_arguments(raw: Option<&Value>) -> Result { + let text = match raw { + None | Some(Value::Null) => return Ok(json!({})), + Some(v @ Value::Object(_)) => return Ok(v.clone()), + Some(Value::String(s)) => s.trim(), + Some(_) => return Err("the tool arguments must be a JSON object".into()), + }; + if text.is_empty() { + return Ok(json!({})); + } + match serde_json::from_str::(text) { + Ok(v @ Value::Object(_)) => Ok(v), + Ok(_) => Err("the tool arguments must be a JSON object".into()), + Err(e) => Err(format!("the tool arguments were not valid JSON ({e}); send the call again with a JSON object")), + } +} + /// The message for what the user typed (and the view, as a data URL). fn user_message(parts: &[PromptPart]) -> Value { let images = parts.iter().any(|p| matches!(p, PromptPart::Image { .. })); @@ -474,6 +494,52 @@ mod tests { b.shutdown(true); } + #[test] + fn tool_arguments_are_an_object_or_an_error() { + assert_eq!(tool_arguments(None), Ok(json!({}))); + assert_eq!(tool_arguments(Some(&json!(""))), Ok(json!({}))); + assert_eq!(tool_arguments(Some(&json!("{\"a\": 1}"))), Ok(json!({"a": 1}))); + assert_eq!(tool_arguments(Some(&json!({"a": 1}))), Ok(json!({"a": 1}))); + for bad in [json!("{command: solid.box"), json!("[1]"), json!("7"), json!(7), json!([1])] { + assert!(tool_arguments(Some(&bad)).is_err(), "{bad}"); + } + } + + /// A call with arguments that are not JSON is answered with the reason, not run with `{}`. + #[test] + fn a_call_with_broken_arguments_is_not_run() { + let answers = std::sync::Mutex::new(vec![ + json!({"choices": [{"message": {"role": "assistant", "content": "Sorry."}}]}), + json!({"choices": [{"message": {"role": "assistant", "content": "", "tool_calls": [{"id": "c1", "type": "function", "function": {"name": "batch", "arguments": "{commands: [oops"}}]}}]}), + ]); + let seen = Arc::new(std::sync::Mutex::new(Vec::::new())); + let log = seen.clone(); + let http: HttpFn = Arc::new(move |r: &HttpRequest| { + log.lock().unwrap().push(serde_json::from_slice(r.body.as_deref().unwrap_or(b"null")).unwrap()); + Ok((200, answers.lock().unwrap().pop().unwrap_or(json!({})).to_string().into_bytes())) + }); + let cfg = ApiConfig { name: "Test".into(), base_url: "http://localhost:1/v1".into(), model: "m".into(), key: None, http }; + let ctx = StartContext { cwd: std::env::temp_dir(), bridge: None, tools: None, wake: Box::new(|| {}) }; + let mut b = ApiBackend::start(cfg, ctx).unwrap(); + b.prompt(&[PromptPart::Text("go".into())]); + let mut events = Vec::new(); + let start = std::time::Instant::now(); + while !events.iter().any(|e| matches!(e, Event::TurnEnded { .. })) && start.elapsed() < Duration::from_secs(10) { + events.extend(b.poll()); + std::thread::sleep(Duration::from_millis(10)); + } + let failed = events.iter().find_map(|e| match e { + Event::Tool { status: Some(ToolStatus::Failed), output: Some(o), .. } => Some(o.clone()), + _ => None, + }); + assert!(failed.is_some_and(|o| o.contains("Not run") && o.contains("not valid JSON")), "{events:?}"); + // The model was told, in the next request. + let sent = seen.lock().unwrap(); + let last = sent[1]["messages"].as_array().unwrap().last().unwrap().clone(); + assert!(last["content"].as_str().unwrap_or("").contains("not valid JSON"), "{last}"); + b.shutdown(true); + } + #[test] fn keys_never_go_over_plain_http_to_another_computer() { let http: HttpFn = Arc::new(|_: &HttpRequest| Ok((200, b"{}".to_vec()))); diff --git a/crates/ui-egui/src/agent_chat/gate.rs b/crates/ui-egui/src/agent_chat/gate.rs index 6317808e..2eb4141c 100644 --- a/crates/ui-egui/src/agent_chat/gate.rs +++ b/crates/ui-egui/src/agent_chat/gate.rs @@ -27,8 +27,36 @@ pub enum Verdict { /// itself) is the agent's. const METHODS: &[&str] = &["engine.commands", "document.inspect", "engine.execute", "engine.batch", "ui.view", "ui.render", "ui.screenshot"]; -/// Parameter names that hold a file or folder. -const PATH_KEYS: &[&str] = &["path", "paths", "file", "files", "dir", "folder", "out", "output", "filename", "source", "target_path"]; +/// Parameter names that hold a file or folder and nothing else. +const FILE_KEYS: &[&str] = &["file", "files", "dir", "folder", "out", "output", "filename", "target_path"]; + +/// Parameter names that are a file in some commands and geometry in others (`path`: a sweep's or +/// pattern's curve ids, an occurrence's ids; `source`: a feature or body). They name a file only +/// when the value reads like one, or the command is by its nature about files. +const AMBIGUOUS_KEYS: &[&str] = &["path", "paths", "source"]; + +/// File extensions that make a bare word a file name. +const FILE_EXTENSIONS: &[&str] = &[ + "solidcraft", + "json", + "csv", + "step", + "stp", + "iges", + "igs", + "stl", + "3mf", + "obj", + "dxf", + "svg", + "png", + "jpg", + "jpeg", + "urdf", + "txt", + "py", + "toml", +]; /// Commands that only read (not counted as changes). fn reads_only(id: &str) -> bool { @@ -39,13 +67,45 @@ fn reads_only(id: &str) -> bool { || matches!(id, "edit.checkpoint" | "render.status" | "render.backends" | "render.settings.get" | "file.slicers") } -fn names_a_file(params: &Value) -> Option { +/// Commands that read or write a file by their nature: any string they are given in a path +/// parameter is a file, whatever it looks like. +fn is_file_verb(id: &str) -> bool { + id.starts_with("library.") || ["export", "import", "save", "open", "insert", "load", "write", "recover"].iter().any(|v| id.contains(v)) +} + +/// Whether `s` reads like a file or folder name: a path separator, `~`, a leading dot, or a known +/// extension. Curve ids (`c1`), edge names (`edge-3`) and the like do not. +fn looks_like_file(s: &str) -> bool { + if s.is_empty() || s.len() > 4096 { + return false; + } + if s.contains('/') || s.contains('\\') || s.starts_with('~') || s.starts_with('.') { + return true; + } + s.rsplit_once('.').is_some_and(|(stem, ext)| !stem.is_empty() && FILE_EXTENSIONS.contains(&ext.to_ascii_lowercase().as_str())) +} + +/// The first file or folder a request's parameters name, for the question and the decision. +/// `id`: the command (empty for methods that are not commands). +fn names_a_file(id: &str, params: &Value) -> Option { let o = params.as_object()?; - PATH_KEYS.iter().find_map(|k| match o.get(*k) { - Some(Value::String(s)) if !s.is_empty() => Some(s.chars().take(200).collect()), - Some(Value::Array(a)) if !a.is_empty() => Some(a.iter().filter_map(Value::as_str).take(3).collect::>().join(", ")), - _ => None, - }) + let shown = |s: &str| s.chars().take(200).collect::(); + let by_nature = is_file_verb(id); + let first = |v: &Value, any: bool| -> Option { + match v { + Value::String(s) if any && !s.is_empty() => Some(shown(s)), + Value::String(s) if looks_like_file(s) => Some(shown(s)), + Value::Array(a) => { + let names: Vec<&str> = a.iter().filter_map(Value::as_str).filter(|s| (any && !s.is_empty()) || looks_like_file(s)).take(3).collect(); + (!names.is_empty()).then(|| names.join(", ")) + } + _ => None, + } + }; + FILE_KEYS + .iter() + .find_map(|k| o.get(*k).and_then(|v| first(v, true))) + .or_else(|| AMBIGUOUS_KEYS.iter().find_map(|k| o.get(*k).and_then(|v| first(v, by_nature)))) } /// One command as the agent would run it. `undo_left`: steps this turn may still undo without @@ -60,7 +120,7 @@ pub fn command(id: &str, params: &Value, undo_left: usize) -> Verdict { } // Files, and commands that replace or reload the whole design. let file_command = id.starts_with("file.") || id.starts_with("doc.") || matches!(id, "render.export" | "render.start"); - if let Some(p) = names_a_file(params) { + if let Some(p) = names_a_file(id, params) { return Verdict::Ask(format!("{id}: {p}")); } if file_command && !reads_only(id) { @@ -111,7 +171,7 @@ pub fn request(method: &str, params: &Value, undo_left: usize) -> Verdict { } } // A picture saved to a file names it. - "ui.screenshot" | "ui.render" => match names_a_file(params) { + "ui.screenshot" | "ui.render" => match names_a_file("ui.render.export", params) { Some(_) if is_scratch(params) => Verdict::Run { change: false }, Some(p) => Verdict::Ask(format!("{method}: {p}")), None => Verdict::Run { change: false }, @@ -189,6 +249,60 @@ mod tests { assert!(matches!(request("engine.batch", &batch, 0), Verdict::Ask(a) if a.contains("/etc/x"))); } + /// `path`, `source` and friends are also geometry (curve ids, edges, occurrence ids): those + /// are not files and must not stop a sweep or a pattern with a question. + #[test] + fn geometry_named_path_is_not_a_file() { + assert_eq!(exec("solid.sweep", json!({"profile": "S1", "path_sketch": "S2", "path": ["c1", "c2"]})), Verdict::Run { change: true }); + assert_eq!( + exec("solid.pattern.path", json!({"features": ["F1"], "path_sketch": "S2", "path": ["c1"], "count": 4})), + Verdict::Run { change: true } + ); + assert_eq!(exec("construct.plane.along_path", json!({"path": "edge-3", "t": 0.5})), Verdict::Run { change: true }); + assert_eq!(exec("component.move", json!({"component": 2, "path": [4, 7], "translate": [1, 0, 0]})), Verdict::Run { change: true }); + assert_eq!(exec("occurrence.move", json!({"occurrence": 3, "path": [1, 2]})), Verdict::Run { change: true }); + let batch = json!({"commands": [ + {"command": "solid.box", "params": {}}, + {"command": "solid.sweep", "params": {"path": ["c1", "c2"], "path_sketch": "S"}}, + ]}); + assert_eq!(request("engine.batch", &batch, 0), Verdict::Run { change: true }); + } + + /// What does name a file still asks, with or without a separator or an extension, and in + /// the commands that export, import or save by their nature. + #[test] + fn real_file_names_still_ask() { + for (id, p) in [ + ("sketch.export_dxf", json!({"path": "/tmp/s.dxf"})), + ("sketch.export_dxf", json!({"path": "s"})), + ("parameters.export", json!({"path": "params"})), + ("parameters.import", json!({"path": "p.csv"})), + ("motion.export", json!({"study": "a", "path": "out.csv"})), + ("canvas.insert", json!({"file": "pic"})), + ("library.apply", json!({"path": "lib"})), + ("solid.box", json!({"filename": "x"})), + ("solid.box", json!({"path": "../x.step"})), + ("solid.box", json!({"paths": ["a/b.step"]})), + ("solid.box", json!({"dir": "out"})), + ] { + assert!(matches!(exec(id, p.clone()), Verdict::Ask(_)), "{id} {p}"); + } + } + + /// Every registered `file.*` and `doc.*` command asks, so a new one cannot slip through by + /// name (the read-only ones, `reads_only`, are the listed exceptions). + #[test] + fn every_registered_file_and_doc_command_asks() { + let mut n = 0; + for spec in solidcraft_engine::command_specs() { + if (spec.id.starts_with("file.") || spec.id.starts_with("doc.")) && !reads_only(spec.id) { + n += 1; + assert!(matches!(exec(spec.id, json!({})), Verdict::Ask(_)), "{}", spec.id); + } + } + assert!(n >= 8, "found {n}"); + } + #[test] fn crash_reports_and_app_methods_are_never_the_agents() { assert!(matches!(exec("diagnostics.crash_reports", json!({})), Verdict::Refuse(_))); diff --git a/docs/agent-chat.md b/docs/agent-chat.md index 185ba1a0..756060c1 100644 --- a/docs/agent-chat.md +++ b/docs/agent-chat.md @@ -136,6 +136,12 @@ using one of SolidCraft's tools, SolidCraft answers for you, since its own gate and notes it in the chat as *Auto-allowed*. With **Allow everything**, or after **Always allow**, the agent's other questions are answered the same way and noted too. +What counts as "a command given a file name": a parameter that holds a file or folder (`file`, +`dir`, `output`, …), or `path` and `source` when the value reads like a file name (a separator, `~`, +a leading dot or a known extension), or any value of those in a command that exports, imports, +saves, opens or inserts. A sweep's or pattern's `path` (curve ids) and an occurrence's `path` +(ids) are geometry and run without a question. + ## What it can't do - **Files wait for you** unless you chose **Allow everything**. **Cancel** or **Stop** declines