From 61b3108be7c06d057f963aea8dbae90d335d9737 Mon Sep 17 00:00:00 2001
From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com>
Date: Sat, 24 Oct 2026 17:06:43 +0200
Subject: [PATCH] Reject invalid index in animation remove, move, timing and
 options

Found while testing crates/engine/src/cmd/animation.rs:232-246 (remove); same pattern in each_target :265 and move_anim :250

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
---
 crates/engine/src/cmd/animation.rs | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/crates/engine/src/cmd/animation.rs b/crates/engine/src/cmd/animation.rs
index 1a97a3b3..d9194df7 100644
--- a/crates/engine/src/cmd/animation.rs
+++ b/crates/engine/src/cmd/animation.rs
@@ -230,7 +230,7 @@ fn add(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 fn remove(s: &mut Session, p: &Value) -> Result<Value> {
-    let index = usize_param(p, "index");
+    let index = opt_usize_param(p, "index", "animation.remove")?;
     let ids = if index.is_none() { targets(s, p)? } else { vec![] };
     s.edit(|doc, sel| {
         let sl = Arc::make_mut(doc.slides.get_mut(sel.slide).ok_or_else(|| bad("animation.remove", "no slide"))?);
@@ -247,7 +247,7 @@ fn remove(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 fn move_anim(s: &mut Session, p: &Value) -> Result<Value> {
-    let i = usize_param(p, "index").ok_or_else(|| bad("animation.move", "missing `index`"))?;
+    let i = opt_usize_param(p, "index", "animation.move")?.ok_or_else(|| bad("animation.move", "missing `index`"))?;
     let to = usize_param(p, "to").ok_or_else(|| bad("animation.move", "missing `to`"))?;
     s.edit(|doc, sel| {
         let sl = Arc::make_mut(doc.slides.get_mut(sel.slide).ok_or_else(|| bad("animation.move", "no slide"))?);
@@ -263,7 +263,7 @@ fn move_anim(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 fn each_target(s: &mut Session, p: &Value, f: impl Fn(&mut Animation)) -> Result<Value> {
-    let index = usize_param(p, "index");
+    let index = opt_usize_param(p, "index", "animation")?;
     let ids = if index.is_none() { targets(s, p)? } else { vec![] };
     s.edit(|doc, sel| {
         let sl = Arc::make_mut(doc.slides.get_mut(sel.slide).ok_or_else(|| bad("animation", "no slide"))?);
@@ -567,4 +567,25 @@ mod tests {
         assert_eq!(triggers(&s), vec![None], "the trigger was inside the deleted group");
         assert_eq!(s.doc().unwrap().current_slide().unwrap().animations.len(), 1);
     }
+
+    #[test]
+    fn invalid_index_is_an_error_not_the_selection() {
+        let mut s = Session::with_new();
+        let r = s.execute("shape.insert", &json!({"preset": "rect", "rect": [10.0, 10.0, 100.0, 100.0]})).unwrap();
+        let id = r.get("id").unwrap().as_u64().unwrap();
+        s.execute("animation.add", &json!({"effect": "fade", "class": "entrance", "ids": [id]})).unwrap();
+        s.select(|_, sel| {
+            sel.shapes = vec![deckcraft_model::ShapeId(id as u32)];
+        })
+        .unwrap();
+        for bad_index in [json!(-1), json!(1.5), json!("x")] {
+            for cmd in ["animation.remove", "animation.timing", "animation.options"] {
+                let r = s.execute(cmd, &json!({"index": bad_index, "duration": 100}));
+                assert!(r.is_err(), "{cmd} accepted index {bad_index}: {r:?}");
+            }
+            let r = s.execute("animation.move", &json!({"index": bad_index, "to": 0}));
+            assert!(r.is_err(), "animation.move accepted index {bad_index}: {r:?}");
+            assert_eq!(s.execute("animation.get", &json!({})).unwrap().as_array().unwrap().len(), 1);
+        }
+    }
 }