refactor(animation): error on typed-coercion failure; relax play/stop gating

Three follow-ups from PR review:

1. `_coerce_value_for_track` now returns `{ok|error}`. Callers
   pre-coerce keyframe values and surface failures as INVALID_PARAMS.
   Previously, passing an unparseable Color string silently stored the
   raw text as the keyframe, producing garbage at play time. The
   authoring-time fallbacks are preserved: if the target node isn't in
   the scene yet, or the target exists but the property isn't present
   (retargeting case — user may set AnimationPlayer.root_node later),
   the raw value passes through unchanged. Errors only fire when the
   property resolves to a known type and the value can't be parsed.

2. `animation_play` / `animation_stop` no longer call `require_writable`.
   They're transient preview ops ("Runtime playback state — not saved
   with scene") and belong on the read side alongside `animation_list`
   and `animation_get`.

3. `animation_create_simple` now delegates per-track insertion to the
   shared `_do_add_property_track` helper — the inline track-building
   block is gone. Coercion logic now has a single caller path.

Tests: two Python unit tests lock in play/stop-as-read-only; one GDScript
test exercises the new error path (unparseable color on a Sprite2D's
modulate). Four existing tests updated to use Vector3 dicts (`{x,y,z}`)
instead of Vector2 dicts for `.:position` on the Node3D scene root —
they passed before only because the old coercer silently fell through.

Live smoke test: all 312 GDScript tests pass, all 375 Python tests pass,
ruff clean. Exercised coerce → play → stop → unparseable-color rejection
→ create_simple(color strings) → get roundtrip against the live editor.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: dsarno

plugin/addons/godot_ai/handlers/animation_handler.gd

@@ -177,19 +177,29 @@ func add_property_track(params: Dictionary) -> Dictionary:
 		return anim_resolved
 	var anim: Animation = anim_resolved.animation
 
-	# Validate keyframe structure before mutating.
+	# Validate + pre-coerce keyframes before mutating. Coercion errors
+	# surface as INVALID_PARAMS rather than silently inserting garbage keys.
+	var coerced_keyframes: Array = []
 	for kf in keyframes:
 		if typeof(kf) != TYPE_DICTIONARY:
 			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Each keyframe must be a dictionary")
 		if not "time" in kf:
 			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Each keyframe must have a 'time' field")
 		if not "value" in kf:
 			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Each keyframe must have a 'value' field")
+		var coerce_result := _coerce_value_for_track(kf.get("value"), track_path, player)
+		if coerce_result.has("error"):
+			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, coerce_result.error)
+		coerced_keyframes.append({
+			"time": kf.get("time"),
+			"value": coerce_result.ok,
+			"transition": kf.get("transition", "linear"),
+		})
 
 	var baseline := anim.get_track_count()
 
 	_undo_redo.create_action("MCP: Add property track %s to %s" % [track_path, anim_name])
-	_undo_redo.add_do_method(self, "_do_add_property_track", anim, track_path, interp_str, keyframes, player)
+	_undo_redo.add_do_method(self, "_do_add_property_track", anim, track_path, interp_str, coerced_keyframes)
 	_undo_redo.add_undo_method(anim, "remove_track", baseline)
 	_undo_redo.commit_action()
 
@@ -206,22 +216,23 @@ func add_property_track(params: Dictionary) -> Dictionary:
 	}
 
 
+## Insert a pre-coerced track into the animation. Callers must coerce
+## values against the target property before calling this (see
+## _coerce_value_for_track) — this method runs inside the undo do-method
+## path where error propagation isn't possible.
 func _do_add_property_track(
 	anim: Animation,
 	track_path: String,
 	interp_str: String,
 	keyframes: Array,
-	player: AnimationPlayer,
 ) -> void:
 	var idx := anim.add_track(Animation.TYPE_VALUE)
 	anim.track_set_path(idx, NodePath(track_path))
 	anim.track_set_interpolation_type(idx, _INTERP_MODES.get(interp_str, Animation.INTERPOLATION_LINEAR))
 	for kf in keyframes:
 		var t: float = float(kf.get("time", 0.0))
-		var raw_value = kf.get("value")
 		var trans: float = _parse_transition(kf.get("transition", "linear"))
-		var coerced = _coerce_value_for_track(raw_value, track_path, player)
-		anim.track_insert_key(idx, t, coerced, trans)
+		anim.track_insert_key(idx, t, kf.get("value"), trans)
 
 
 # ============================================================================
@@ -554,30 +565,37 @@ func create_simple(params: Dictionary) -> Dictionary:
 		if computed_length <= 0.0:
 			computed_length = 1.0
 
-	# Build the animation fully in memory before touching the undo stack.
-	var anim := Animation.new()
-	anim.length = computed_length
-	anim.loop_mode = _LOOP_MODES[loop_mode_str]
-
+	# Pre-coerce all tween values before touching the anim — coercion errors
+	# surface as INVALID_PARAMS, not silent garbage keyframes.
+	var per_track_keyframes: Array = []
 	for spec in tweens:
 		var target: String = str(spec.get("target", ""))
 		var property: String = str(spec.get("property", ""))
-		var from_val = spec.get("from")
-		var to_val = spec.get("to")
+		var track_path: String = target + ":" + property
 		var duration: float = float(spec.get("duration", 1.0))
 		var delay: float = float(spec.get("delay", 0.0))
 		var trans_str = spec.get("transition", "linear")
-		var trans: float = _parse_transition(trans_str)
+		var from_result := _coerce_value_for_track(spec.get("from"), track_path, player)
+		if from_result.has("error"):
+			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "tween '%s': %s" % [track_path, from_result.error])
+		var to_result := _coerce_value_for_track(spec.get("to"), track_path, player)
+		if to_result.has("error"):
+			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "tween '%s': %s" % [track_path, to_result.error])
+		per_track_keyframes.append({
+			"track_path": track_path,
+			"keyframes": [
+				{"time": delay, "value": from_result.ok, "transition": trans_str},
+				{"time": delay + duration, "value": to_result.ok, "transition": trans_str},
+			],
+		})
 
-		var track_path: String = target + ":" + property
-		var idx := anim.add_track(Animation.TYPE_VALUE)
-		anim.track_set_path(idx, NodePath(track_path))
-		anim.track_set_interpolation_type(idx, Animation.INTERPOLATION_LINEAR)
+	# Build the animation fully in memory before touching the undo stack.
+	var anim := Animation.new()
+	anim.length = computed_length
+	anim.loop_mode = _LOOP_MODES[loop_mode_str]
 
-		var coerced_from = _coerce_value_for_track(from_val, track_path, player)
-		var coerced_to = _coerce_value_for_track(to_val, track_path, player)
-		anim.track_insert_key(idx, delay, coerced_from, trans)
-		anim.track_insert_key(idx, delay + duration, coerced_to, trans)
+	for entry in per_track_keyframes:
+		_do_add_property_track(anim, entry.track_path, "linear", entry.keyframes)
 
 	# One atomic undo action.
 	_undo_redo.create_action("MCP: Create animation %s (%d tracks)" % [anim_name, anim.get_track_count()])
@@ -675,18 +693,19 @@ func _resolve_animation(player: AnimationPlayer, anim_name: String) -> Dictionar
 # ============================================================================
 
 ## Coerce a JSON value to match the expected Godot type for the given
-## track_path. Resolves the target node and property type via reflection.
-## Falls back to raw value if the target cannot be found at author time.
-static func _coerce_value_for_track(value: Variant, track_path: String, player: AnimationPlayer) -> Variant:
-	# Parse track_path "NodeName:property" or ".:property"
+## track_path. Returns {"ok": value} or {"error": msg}.
+## Passes the raw value through when the target node isn't in the scene
+## yet (authoring-time path). Errors when the target exists but the
+## property doesn't, or when parsing a typed value (Color/Vector2/Vector3)
+## clearly fails — better to reject than silently store garbage.
+static func _coerce_value_for_track(value: Variant, track_path: String, player: AnimationPlayer) -> Dictionary:
 	var colon := track_path.rfind(":")
 	if colon < 0:
-		return value
+		return {"ok": value}
 
 	var node_part := track_path.substr(0, colon)
 	var prop_part := track_path.substr(colon + 1)
 
-	# Resolve node relative to AnimationPlayer's root (defaults to parent).
 	var root_node: Node = null
 	if player.is_inside_tree():
 		var rn := player.root_node
@@ -695,56 +714,62 @@ static func _coerce_value_for_track(value: Variant, track_path: String, player:
 		if root_node == null:
 			root_node = player.get_parent()
 	if root_node == null:
-		return value
+		return {"ok": value}
 
 	var target: Node = root_node.get_node_or_null(node_part)
 	if target == null:
-		return value  # Target not in scene yet; pass through raw value.
+		return {"ok": value}
 
-	# Inspect target property type.
 	for p in target.get_property_list():
 		if p.name == prop_part:
-			return _coerce_for_type(value, p.get("type", TYPE_NIL))
+			return _coerce_for_type(value, p.get("type", TYPE_NIL), prop_part)
 
-	return value
+	# Property not found on current target — pass through. The caller may
+	# plan to retarget the AnimationPlayer (set root_node) before playback.
+	return {"ok": value}
 
 
-## Coerce a single value to the given Godot variant type.
-static func _coerce_for_type(value: Variant, prop_type: int) -> Variant:
+## Coerce a single value to the given Godot variant type. Returns
+## {"ok": coerced} or {"error": msg}. Unknown types pass through.
+static func _coerce_for_type(value: Variant, prop_type: int, prop_name: String) -> Dictionary:
 	match prop_type:
 		TYPE_COLOR:
 			if value is Color:
-				return value
+				return {"ok": value}
 			if value is String:
-				var a := Color.from_string(value, Color(0, 0, 0, 0))
-				var b := Color.from_string(value, Color(1, 1, 1, 1))
+				var s := value as String
+				var a := Color.from_string(s, Color(0, 0, 0, 0))
+				var b := Color.from_string(s, Color(1, 1, 1, 1))
 				if a == b:
-					return a
-				return value  # Unparseable — pass through, fail at play time.
+					return {"ok": a}
+				return {"error": "Cannot parse '%s' as Color for property '%s'" % [s, prop_name]}
 			if value is Dictionary and value.has("r") and value.has("g") and value.has("b"):
-				return Color(float(value.r), float(value.g), float(value.b), float(value.get("a", 1.0)))
+				return {"ok": Color(float(value.r), float(value.g), float(value.b), float(value.get("a", 1.0)))}
+			return {"error": "Cannot coerce value to Color for property '%s' (expected string, {r,g,b}, or Color)" % prop_name}
 		TYPE_VECTOR2:
 			if value is Vector2:
-				return value
+				return {"ok": value}
 			if value is Dictionary and value.has("x") and value.has("y"):
-				return Vector2(float(value.x), float(value.y))
+				return {"ok": Vector2(float(value.x), float(value.y))}
 			if value is Array and value.size() >= 2:
-				return Vector2(float(value[0]), float(value[1]))
+				return {"ok": Vector2(float(value[0]), float(value[1]))}
+			return {"error": "Cannot coerce value to Vector2 for property '%s' (expected {x,y}, [x,y], or Vector2)" % prop_name}
 		TYPE_VECTOR3:
 			if value is Vector3:
-				return value
+				return {"ok": value}
 			if value is Dictionary and value.has("x") and value.has("y") and value.has("z"):
-				return Vector3(float(value.x), float(value.y), float(value.z))
+				return {"ok": Vector3(float(value.x), float(value.y), float(value.z))}
+			return {"error": "Cannot coerce value to Vector3 for property '%s' (expected {x,y,z} or Vector3)" % prop_name}
 		TYPE_FLOAT:
 			if value is int or value is float:
-				return float(value)
+				return {"ok": float(value)}
 		TYPE_INT:
 			if value is float or value is int:
-				return int(value)
+				return {"ok": int(value)}
 		TYPE_BOOL:
-			if value is int or value is float:
-				return bool(value)
-	return value
+			if value is int or value is float or value is bool:
+				return {"ok": bool(value)}
+	return {"ok": value}
 
 
 # ============================================================================

src/godot_ai/handlers/animation.py

@@ -96,7 +96,6 @@ async def animation_play(
     player_path: str,
     animation_name: str = "",
 ) -> dict:
-    require_writable(runtime)
     return await runtime.send_command(
         "animation_play",
         {"player_path": player_path, "animation_name": animation_name},
@@ -107,7 +106,6 @@ async def animation_stop(
     runtime: Runtime,
     player_path: str,
 ) -> dict:
-    require_writable(runtime)
     return await runtime.send_command(
         "animation_stop",
         {"player_path": player_path},

test_project/tests/test_animation.gd

@@ -255,8 +255,8 @@ func test_add_property_track_transition_named() -> void:
 		"animation_name": "anim",
 		"track_path": ".:position",
 		"keyframes": [
-			{"time": 0.0, "value": {"x": 0.0, "y": 0.0}, "transition": "ease_out"},
-			{"time": 1.0, "value": {"x": 100.0, "y": 0.0}, "transition": "ease_out"},
+			{"time": 0.0, "value": {"x": 0.0, "y": 0.0, "z": 0.0}, "transition": "ease_out"},
+			{"time": 1.0, "value": {"x": 100.0, "y": 0.0, "z": 0.0}, "transition": "ease_out"},
 		],
 	})
 	assert_has_key(result, "data")
@@ -323,6 +323,36 @@ func test_create_simple_coerces_vector3() -> void:
 	_remove_node(player_path)
 
 
+func test_add_property_track_rejects_unparseable_color() -> void:
+	# When the target property exists and has a known type (here, Color on
+	# Sprite2D.modulate), an unparseable string value should fail at author
+	# time rather than silently ending up as raw text in the keyframe.
+	var scene_root := EditorInterface.get_edited_scene_root()
+	var sprite := Sprite2D.new()
+	sprite.name = "ColorSprite"
+	scene_root.add_child(sprite)
+	sprite.owner = scene_root
+
+	var player_path := _add_player("TestBadColor")
+	if player_path.is_empty():
+		sprite.get_parent().remove_child(sprite)
+		sprite.queue_free()
+		return
+	_handler.create_animation({"player_path": player_path, "name": "anim", "length": 1.0})
+	var result := _handler.add_property_track({
+		"player_path": player_path,
+		"animation_name": "anim",
+		"track_path": "ColorSprite:modulate",
+		"keyframes": [
+			{"time": 0.0, "value": "not_a_color"},
+		],
+	})
+	assert_is_error(result, "", "expected INVALID_PARAMS for unparseable color string")
+	_remove_node(player_path)
+	sprite.get_parent().remove_child(sprite)
+	sprite.queue_free()
+
+
 func test_add_property_track_transition_raw_float() -> void:
 	var player_path := _add_player("TestTransFloat")
 	if player_path.is_empty():
@@ -333,8 +363,8 @@ func test_add_property_track_transition_raw_float() -> void:
 		"animation_name": "anim",
 		"track_path": ".:position",
 		"keyframes": [
-			{"time": 0.0, "value": {"x": 0.0, "y": 0.0}, "transition": 3.0},
-			{"time": 1.0, "value": {"x": 100.0, "y": 0.0}, "transition": 3.0},
+			{"time": 0.0, "value": {"x": 0.0, "y": 0.0, "z": 0.0}, "transition": 3.0},
+			{"time": 1.0, "value": {"x": 100.0, "y": 0.0, "z": 0.0}, "transition": 3.0},
 		],
 	})
 	assert_has_key(result, "data")
@@ -535,8 +565,8 @@ func test_create_simple_auto_length() -> void:
 			{
 				"target": ".",
 				"property": "position",
-				"from": {"x": -400.0, "y": 0.0},
-				"to": {"x": 0.0, "y": 0.0},
+				"from": {"x": -400.0, "y": 0.0, "z": 0.0},
+				"to": {"x": 0.0, "y": 0.0, "z": 0.0},
 				"duration": 0.4,
 				"delay": 0.1,
 			}
@@ -582,7 +612,7 @@ func test_create_simple_multiple_tweens() -> void:
 		"name": "combo",
 		"tweens": [
 			{"target": ".", "property": "modulate", "from": {"r":1,"g":1,"b":1,"a":0}, "to": {"r":1,"g":1,"b":1,"a":1}, "duration": 0.5},
-			{"target": ".", "property": "position", "from": {"x": -200.0, "y": 0.0}, "to": {"x": 0.0, "y": 0.0}, "duration": 0.3, "delay": 0.1},
+			{"target": ".", "property": "position", "from": {"x": -200.0, "y": 0.0, "z": 0.0}, "to": {"x": 0.0, "y": 0.0, "z": 0.0}, "duration": 0.3, "delay": 0.1},
 		],
 	})
 	assert_has_key(result, "data")

tests/unit/test_runtime_handlers.py

@@ -2604,6 +2604,42 @@ async def test_animation_list_does_not_require_writable():
     assert result["count"] == 2
 
 
+async def test_animation_play_does_not_require_writable():
+    """animation_play is a preview op — it must not call require_writable."""
+    from godot_ai.sessions.registry import Session
+
+    client = StubClient()
+    session = Session(
+        session_id="s1",
+        godot_version="4.4",
+        project_path="/tmp/p",
+        plugin_version="0.1",
+        readiness="playing",
+    )
+    registry = SessionRegistry()
+    registry.register(session)
+    runtime = DirectRuntime(registry=registry, client=client)
+    await animation_handlers.animation_play(runtime, player_path="/Main/AP", animation_name="idle")
+
+
+async def test_animation_stop_does_not_require_writable():
+    """animation_stop is a preview op — it must not call require_writable."""
+    from godot_ai.sessions.registry import Session
+
+    client = StubClient()
+    session = Session(
+        session_id="s1",
+        godot_version="4.4",
+        project_path="/tmp/p",
+        plugin_version="0.1",
+        readiness="playing",
+    )
+    registry = SessionRegistry()
+    registry.register(session)
+    runtime = DirectRuntime(registry=registry, client=client)
+    await animation_handlers.animation_stop(runtime, player_path="/Main/AP")
+
+
 async def test_animation_player_create_requires_writable():
     """Write tools must raise EDITOR_NOT_READY when editor is importing."""
     from godot_ai.godot_client.client import GodotCommandError