Automation: reject an invalid document index instead of using the active document (#4160) (#4161)

Root cause: the JSON-lines server's `index_of` read `index` with `as_u64`, so a
string, negative, fractional or boolean index became None, which means "the
active document". `doc.close {"index":"0"}` therefore closed the active
(possibly unsaved) document instead of failing. The MCP server types the param
as Option<usize> and already rejects these values.

Fix: `index_of` returns Result<Option<usize>, AutomationError>. A missing or
null index still means the active document; a non-negative integer is used;
anything else is a bad request ("`index` must be a non-negative integer").
All doc.* call sites propagate the error with `?`.

Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>

crates/automation/src/rpc.rs

@@ -60,8 +60,13 @@ fn bad(msg: impl Into<String>) -> AutomationError {
     AutomationError::BadRequest(msg.into())
 }
 
-fn index_of(p: &Value) -> Option<usize> {
-    p.get("index").and_then(Value::as_u64).map(|i| i as usize)
+/// The optional `index` param: missing or null means the active document; anything else must be a
+/// non-negative integer (never silently fall back to the active document, which `doc.close` would close).
+fn index_of(p: &Value) -> Result<Option<usize>, AutomationError> {
+    match p.get("index") {
+        None | Some(Value::Null) => Ok(None),
+        Some(v) => v.as_u64().and_then(|i| usize::try_from(i).ok()).map(Some).ok_or_else(|| bad("`index` must be a non-negative integer")),
+    }
 }
 
 fn str_of<'a>(p: &'a Value, k: &str) -> Option<&'a str> {
@@ -109,7 +114,7 @@ impl Headless {
                 let mut opts = p.as_object().cloned().unwrap_or_default();
                 opts.remove("path");
                 opts.remove("index");
-                self.place(index_of(&p), path, &Value::Object(opts))
+                self.place(index_of(&p)?, path, &Value::Object(opts))
             }
             "doc.new" => self.command_run("file.new", p),
             "doc.save" => {
@@ -123,7 +128,7 @@ impl Headless {
                     opts.encode.webp_lossless = false;
                 }
                 let path = str_of(&p, "path").map(PathBuf::from);
-                self.save(index_of(&p), path.as_deref(), str_of(&p, "format"), &opts)
+                self.save(index_of(&p)?, path.as_deref(), str_of(&p, "format"), &opts)
             }
             "doc.exportLayers" | "doc.exportDataSets" => {
                 let dir = str_of(&p, "dir").ok_or_else(|| bad(format!("{method} needs `dir`")))?;
@@ -131,16 +136,16 @@ impl Headless {
                 opts.remove("dir");
                 opts.remove("index");
                 let opts = Value::Object(opts);
-                if method == "doc.exportLayers" { self.export_layers(index_of(&p), dir, &opts) } else { self.export_data_sets(index_of(&p), dir, &opts) }
+                if method == "doc.exportLayers" { self.export_layers(index_of(&p)?, dir, &opts) } else { self.export_data_sets(index_of(&p)?, dir, &opts) }
             }
             "doc.importDataSets" => {
                 let path = str_of(&p, "path").ok_or_else(|| bad("doc.importDataSets needs `path`"))?;
                 let mut opts = p.as_object().cloned().unwrap_or_default();
                 opts.remove("path");
                 opts.remove("index");
-                self.import_data_sets(index_of(&p), path, &Value::Object(opts))
+                self.import_data_sets(index_of(&p)?, path, &Value::Object(opts))
             }
-            "doc.inspect" => self.inspect(index_of(&p)),
+            "doc.inspect" => self.inspect(index_of(&p)?),
             "doc.render" => {
                 let max = match p.get("maxSide") {
                     None => 1024,
@@ -148,7 +153,7 @@ impl Headless {
                         value.as_u64().and_then(|value| u32::try_from(value).ok()).ok_or_else(|| bad("maxSide must be an unsigned 32-bit integer"))?
                     }
                 };
-                let png = self.render_png(index_of(&p), max)?;
+                let png = self.render_png(index_of(&p)?, max)?;
                 match str_of(&p, "path") {
                     Some(path) => {
                         self.write_render(&PathBuf::from(path), &png)?;
@@ -161,10 +166,10 @@ impl Headless {
                 }
             }
             "doc.select" => {
-                let i = index_of(&p).ok_or_else(|| bad("doc.select needs `index`"))?;
+                let i = index_of(&p)?.ok_or_else(|| bad("doc.select needs `index`"))?;
                 self.select(i)
             }
-            "doc.close" => self.close(index_of(&p)),
+            "doc.close" => self.close(index_of(&p)?),
             "batch" => self.batch(&p),
             "methods" => Ok(json!(METHODS)),
             other => Err(bad(format!("unknown method `{other}` (try `methods`)"))),
@@ -830,4 +835,27 @@ mod tests {
         assert_eq!(v["error"], "authentication required");
         assert!(v.get("result").is_none());
     }
+
+    #[test]
+    fn invalid_document_index_is_rejected_not_the_active_document() {
+        let h = session();
+        for _ in 0..2 {
+            assert_eq!(respond(&h, r#"{"method":"doc.new","params":{"width":8,"height":8}}"#)["ok"], true);
+        }
+        let count = |h: &Mutex<Headless>| h.lock().unwrap().session_list()["documents"].as_array().unwrap().len();
+        for index in [json!("0"), json!(-1), json!(0.5), json!(true)] {
+            for method in ["doc.close", "doc.inspect", "doc.select"] {
+                let reply = respond(&h, &json!({"method": method, "params": {"index": index}}).to_string());
+                assert_eq!(reply["ok"], false, "{method} {index}: {reply}");
+                assert!(reply["error"].as_str().unwrap().contains("`index` must be a non-negative integer"), "{reply}");
+            }
+            assert_eq!(count(&h), 2, "index {index} closed a document");
+        }
+        // Null means "the active document", like a missing `index`.
+        assert_eq!(respond(&h, r#"{"method":"doc.inspect","params":{"index":null}}"#)["ok"], true);
+        // A valid index still closes that document.
+        let reply = respond(&h, r#"{"method":"doc.close","params":{"index":0}}"#);
+        assert_eq!(reply["ok"], true, "{reply}");
+        assert_eq!(count(&h), 1);
+    }
 }