Agent chat gate: curve and occurrence ids named path are not files; broken tool arguments are not run (#1499)

Co-Authored-By: Rec0iL <[email protected]>
Co-Authored-By: Claude Sonnet 5.5 <[email protected]>

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<Value, String> {
+    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::<Value>(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::<Value>::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())));

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<String> {
+/// 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<String> {
     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::<Vec<_>>().join(", ")),
-        _ => None,
-    })
+    let shown = |s: &str| s.chars().take(200).collect::<String>();
+    let by_nature = is_file_verb(id);
+    let first = |v: &Value, any: bool| -> Option<String> {
+        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(_)));

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