Address retroactive review of PRs #17 and #21

Nine follow-ups from a retroactive code review of the animation_* tool
family (#17) and the cyberpunk HUD friction fixes (#21). All 388 Python
+ 345 GDScript tests pass; smoke-tested live against the editor.

1. node_create scene_path now uses PackedScene.GEN_EDIT_STATE_INSTANCE
   so the editor treats the result as a real scene instance (foldout icon,
   .tscn stores a reference, not an exploded subtree). Removes the
   _set_owner_recursive call that was running outside the undo action and
   also broke the instance link by forcing descendant ownership.

2. Animation track undo no longer caches the baseline track_count at do
   time - it resolves the index via anim.find_track(path, type) at undo
   time. Robust to any history interleaving. New test stresses this path.

3. _coerce_value_for_track now errors with INVALID_PARAMS when the target
   node exists but the property doesnt - previously the raw value passed
   through and produced garbage keyframes at playback. Matches the
   handlers own docstring.

4. add_method_track validates keyframe args is an Array and method is a
   non-empty string. Previously a typed-assign crash or silent garbage.

5. animation_delete now uses _resolve_animation to locate the owning
   library, matching the read-side symmetry with animation_get /
   animation_play. Delete from non-default libraries works end-to-end.

6. theme_set_stylebox_flat flattened from 27 parameters to a nested-dict
   API: border / corners / margins / shadow each take {all, sides} with
   side-specific keys overriding all. Unknown keys fail loudly. Breaking
   change - tool was only merged in #21 and has no external clients yet.

7. project_stop replaces the fixed 150ms sleep with a bounded poll on
   session.readiness. Reaches truth as soon as the existing
   readiness_changed event arrives (~17ms) instead of blanket-sleeping
   150ms, with a 1s timeout for a hung play process.

8. ui_build_layout pseudo-property tests now read back via
   get_theme_*_override (not the fallback get_theme_* getters), per the
   CLAUDE.md "assert on the stored Variant" rule. Added missing
   theme_override_styles/ test.

9. signal_connect distinguishes declared-but-uninstantiated autoloads
   from genuinely missing nodes, with a clear error pointing users at
   @onready/runtime-connect as the workaround. Previous "not found"
   error masked the real reason most runtime-only autoloads cant be
   connected at edit time.

Also updates CLAUDE.md with reference patterns for find_track-at-undo,
GEN_EDIT_STATE_INSTANCE, and get_theme_*_override readback tests.

Deferred to a follow-up PR: plugin dispatcher coroutine support +
readiness Option C (plugin-side await). That refactor touches the hot
path every command flows through and deserves its own bisect-friendly
landing.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: dsarno

CLAUDE.md

@@ -185,6 +185,35 @@ JSON dicts like `{"r":1,"g":0,"b":0,"a":1}` only become `Color` / `Vector2` / `V
 
 GDScript tests that just assert `track_count == 1` will pass even when coercion is broken. **Always read back via `track_get_key_value(idx, k)` and assert `value is Color` / `value is Vector3` / etc.** `test_animation.gd` `test_add_property_track_coerces_vector3_dict` is the reference pattern. The same rule applies to any future handler that takes JSON values intended to land as typed Variants in the scene.
 
+Same principle for theme override pseudo-properties on Controls: use `get_theme_color_override`, `get_theme_constant_override`, `get_theme_font_size_override`, `get_theme_stylebox_override` in tests — **not** the fallback `get_theme_color` getters — so a broken override silently resolving via the theme fallback can't mask a bug. `test_ui.gd` `test_build_layout_theme_override_*` are the reference pattern.
+
+### Auto-generated indices: look up at undo time, not do time
+
+When a write tool mutates a resource whose index is assigned by Godot (`Animation.add_track` returns an int index, same for track keys, `MultiMesh.instance_count`, etc.), do **not** capture that index at do time and reuse it in the undo callable. Any other mutation landing between the do and the undo makes the index stale — the undo will then remove the wrong element (or error).
+
+Instead, undo via a helper that resolves the index at undo time via a stable lookup:
+
+```gdscript
+_undo_redo.add_undo_method(self, "_undo_remove_track_by_path", anim, track_path, Animation.TYPE_VALUE)
+
+func _undo_remove_track_by_path(anim: Animation, path: String, type: int) -> void:
+    var idx := anim.find_track(NodePath(path), type)
+    if idx >= 0:
+        anim.remove_track(idx)
+```
+
+See `animation_handler.gd::_undo_remove_track_by_path` for the reference pattern. Cover with a test that interleaves a second mutation between the do and undo of the first (`test_animation.gd::test_add_property_track_undo_survives_interleaving`).
+
+### Scene instancing: use GEN_EDIT_STATE_INSTANCE
+
+When a tool instantiates a PackedScene into the edited scene, pass `PackedScene.GEN_EDIT_STATE_INSTANCE` to `instantiate()`:
+
+```gdscript
+new_node = packed_scene.instantiate(PackedScene.GEN_EDIT_STATE_INSTANCE)
+```
+
+This makes Godot treat the result as a real scene instance: the root shows the foldout icon, the `.tscn` stores a reference to the sub-scene rather than an exploded subtree, and the instance can be swapped or toggled editable via the usual editor UI. Don't manually set descendant owners to your scene_root — descendants of a scene instance stay owned by their sub-scene; overriding that breaks the instance link. See `node_handler.gd::create_node`.
+
 ## Test coverage
 
 100% code coverage for core features, always. Every tool, handler, and protocol path must have both:

plugin/addons/godot_ai/handlers/animation_handler.gd

@@ -156,26 +156,32 @@ func delete_animation(params: Dictionary) -> Dictionary:
 		return resolved
 	var player: AnimationPlayer = resolved.player
 
-	if not player.has_animation(anim_name):
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS,
-			"Animation '%s' not found on player at %s" % [anim_name, player_path])
-
-	var library: AnimationLibrary = resolved.library
-	if library == null:
-		return McpErrorCodes.make(McpErrorCodes.INTERNAL_ERROR, "No default library found")
-
-	var old_anim: Animation = library.get_animation(anim_name)
+	# Use _resolve_animation so we can delete from ANY library, not just the
+	# default. Mirrors the read-side symmetry with animation_get / animation_play
+	# which already search all libraries via _resolve_animation.
+	var anim_resolved := _resolve_animation(player, anim_name)
+	if anim_resolved.has("error"):
+		return anim_resolved
+	var old_anim: Animation = anim_resolved.animation
+	var library: AnimationLibrary = anim_resolved.library
+	# Clip key within the owning library — strips the "libname/" prefix if the
+	# caller passed a qualified name.
+	var clip_key: String = anim_name
+	var slash := anim_name.find("/")
+	if slash >= 0:
+		clip_key = anim_name.substr(slash + 1)
 
 	_undo_redo.create_action("MCP: Delete animation %s" % anim_name)
-	_undo_redo.add_do_method(library, "remove_animation", anim_name)
-	_undo_redo.add_undo_method(library, "add_animation", anim_name, old_anim)
+	_undo_redo.add_do_method(library, "remove_animation", clip_key)
+	_undo_redo.add_undo_method(library, "add_animation", clip_key, old_anim)
 	_undo_redo.add_do_reference(old_anim)  # prevent GC so undo→redo works
 	_undo_redo.commit_action()
 
 	return {
 		"data": {
 			"player_path": player_path,
 			"animation_name": anim_name,
+			"library_key": anim_resolved.get("library_key", ""),
 			"undoable": true,
 		}
 	}
@@ -237,11 +243,12 @@ func add_property_track(params: Dictionary) -> Dictionary:
 			"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, coerced_keyframes)
-	_undo_redo.add_undo_method(anim, "remove_track", baseline)
+	# Undo locates the track by (path, type) at undo time rather than caching
+	# an index captured at do time. Cached indices go stale if any other track
+	# mutation lands between do and undo (Godot editor, another MCP call, etc.)
+	_undo_redo.add_undo_method(self, "_undo_remove_track_by_path", anim, track_path, Animation.TYPE_VALUE)
 	_undo_redo.commit_action()
 
 	return {
@@ -251,7 +258,6 @@ func add_property_track(params: Dictionary) -> Dictionary:
 			"track_path": track_path,
 			"interpolation": interp_str,
 			"keyframe_count": keyframes.size(),
-			"track_index": baseline,
 			"undoable": true,
 		}
 	}
@@ -306,6 +312,12 @@ func add_method_track(params: Dictionary) -> Dictionary:
 			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Each keyframe must have a 'time' field")
 		if not "method" in kf:
 			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Each keyframe must have a 'method' field")
+		var method_field = kf.get("method")
+		if typeof(method_field) != TYPE_STRING or (method_field as String).is_empty():
+			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "'method' must be a non-empty string")
+		if kf.has("args") and typeof(kf.get("args")) != TYPE_ARRAY:
+			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS,
+				"'args' must be an array if provided (got %s)" % type_string(typeof(kf.get("args"))))
 
 	var resolved := _resolve_player(player_path)
 	if resolved.has("error"):
@@ -317,11 +329,10 @@ func add_method_track(params: Dictionary) -> Dictionary:
 		return anim_resolved
 	var anim: Animation = anim_resolved.animation
 
-	var baseline := anim.get_track_count()
-
 	_undo_redo.create_action("MCP: Add method track %s to %s" % [target_path, anim_name])
 	_undo_redo.add_do_method(self, "_do_add_method_track", anim, target_path, keyframes)
-	_undo_redo.add_undo_method(anim, "remove_track", baseline)
+	# Undo locates the track by (path, type) at undo time — see add_property_track.
+	_undo_redo.add_undo_method(self, "_undo_remove_track_by_path", anim, target_path, Animation.TYPE_METHOD)
 	_undo_redo.commit_action()
 
 	return {
@@ -330,12 +341,21 @@ func add_method_track(params: Dictionary) -> Dictionary:
 			"animation_name": anim_name,
 			"target_node_path": target_path,
 			"keyframe_count": keyframes.size(),
-			"track_index": baseline,
 			"undoable": true,
 		}
 	}
 
 
+## Remove a track identified by (path, type) at undo time. Robust to
+## history interleaving: if another track was added since the do, the
+## find_track call still resolves to the correct index. Returns silently
+## if the track is no longer present (e.g. a prior undo already removed it).
+func _undo_remove_track_by_path(anim: Animation, track_path: String, track_type: int) -> void:
+	var idx := anim.find_track(NodePath(track_path), track_type)
+	if idx >= 0:
+		anim.remove_track(idx)
+
+
 func _do_add_method_track(anim: Animation, target_path: String, keyframes: Array) -> void:
 	var idx := anim.add_track(Animation.TYPE_METHOD)
 	anim.track_set_path(idx, NodePath(target_path))
@@ -862,15 +882,18 @@ static func _coerce_value_for_track(value: Variant, track_path: String, player:
 
 	var target: Node = root_node.get_node_or_null(node_part)
 	if target == null:
+		# Target node isn't in the scene yet — authoring-time path. Pass through.
 		return {"ok": value}
 
 	for p in target.get_property_list():
 		if p.name == prop_part:
 			return _coerce_for_type(value, p.get("type", TYPE_NIL), prop_part)
 
-	# Property not found on current target — pass through. The caller may
-	# plan to retarget the AnimationPlayer (set root_node) before playback.
-	return {"ok": value}
+	# Target exists but the property doesn't. Reject loudly — silently storing
+	# the raw value here produces garbage keyframes at playback time.
+	return {"error":
+		"Property '%s' not found on target '%s' (class %s)" %
+		[prop_part, node_part, target.get_class()]}
 
 
 ## Coerce a single value to the given Godot variant type. Returns

plugin/addons/godot_ai/handlers/node_handler.gd

@@ -31,14 +31,18 @@ func create_node(params: Dictionary) -> Dictionary:
 
 	if not scene_path.is_empty():
 		# Scene instancing path — load and instantiate a PackedScene.
+		# GEN_EDIT_STATE_INSTANCE makes the editor treat the result as a real
+		# scene instance (foldout icon, the .tscn stores a reference instead of
+		# an exploded subtree). Descendants remain owned by their sub-scene;
+		# setting their owner to our scene_root would break the instance link.
 		if not scene_path.begins_with("res://"):
 			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "scene_path must start with res://")
 		if not ResourceLoader.exists(scene_path):
 			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Scene not found: %s" % scene_path)
 		var packed_scene = ResourceLoader.load(scene_path)
 		if packed_scene == null or not packed_scene is PackedScene:
 			return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Resource at %s is not a PackedScene" % scene_path)
-		new_node = packed_scene.instantiate()
+		new_node = packed_scene.instantiate(PackedScene.GEN_EDIT_STATE_INSTANCE)
 		if new_node == null:
 			return McpErrorCodes.make(McpErrorCodes.INTERNAL_ERROR, "Failed to instantiate scene: %s" % scene_path)
 	else:
@@ -63,10 +67,6 @@ func create_node(params: Dictionary) -> Dictionary:
 	_undo_redo.add_undo_method(parent, "remove_child", new_node)
 	_undo_redo.commit_action()
 
-	# For instanced scenes, set owner on descendants so they serialize properly.
-	if not scene_path.is_empty():
-		_set_owner_recursive(new_node, scene_root)
-
 	var response := {
 		"name": new_node.name,
 		"type": new_node.get_class(),

plugin/addons/godot_ai/handlers/signal_handler.gd

@@ -117,17 +117,15 @@ func _resolve_signal_params(params: Dictionary) -> Dictionary:
 	if scene_root == null:
 		return McpErrorCodes.make(McpErrorCodes.EDITOR_NOT_READY, "No scene open")
 
-	var source := ScenePath.resolve(params.path, scene_root)
-	if source == null:
-		source = _resolve_autoload(params.path)
-	if source == null:
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Source node not found: %s (not in scene tree or autoloads)" % params.path)
-
-	var target := ScenePath.resolve(params.target, scene_root)
-	if target == null:
-		target = _resolve_autoload(params.target)
-	if target == null:
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Target node not found: %s (not in scene tree or autoloads)" % params.target)
+	var source_result := _resolve_node_or_autoload(params.path, scene_root, "Source")
+	if source_result.has("error"):
+		return source_result
+	var source: Node = source_result.node
+
+	var target_result := _resolve_node_or_autoload(params.target, scene_root, "Target")
+	if target_result.has("error"):
+		return target_result
+	var target: Node = target_result.node
 
 	return {
 		"source": source,
@@ -138,16 +136,34 @@ func _resolve_signal_params(params: Dictionary) -> Dictionary:
 	}
 
 
-## Attempt to resolve a path as an autoload singleton.
-## Uses direct ProjectSettings lookup (O(1)) instead of scanning all properties.
-func _resolve_autoload(path: String) -> Node:
+## Resolve a path to a Node, with three distinct outcomes:
+##   1. Found in the edited scene tree → returns {node}
+##   2. Declared as an autoload AND instantiated at edit time → returns {node}
+##   3. Declared as an autoload but NOT instantiated at edit time → returns
+##      INVALID_PARAMS with guidance. Most autoloads are runtime-only, so a
+##      silent "not found" hides the real reason the connection can't be made.
+##   4. Not in scene and not a declared autoload → returns INVALID_PARAMS.
+func _resolve_node_or_autoload(path: String, scene_root: Node, role: String) -> Dictionary:
+	var node := ScenePath.resolve(path, scene_root)
+	if node != null:
+		return {"node": node}
+
 	var name := path.trim_prefix("/")
-	if not ProjectSettings.has_setting("autoload/" + name):
-		return null
-	var tree := Engine.get_main_loop()
-	if tree is SceneTree:
-		return (tree as SceneTree).root.get_node_or_null(name)
-	return null
+	if ProjectSettings.has_setting("autoload/" + name):
+		# Autoload is declared — see if the editor has it instanced.
+		var tree := Engine.get_main_loop()
+		if tree is SceneTree:
+			var live := (tree as SceneTree).root.get_node_or_null(name)
+			if live != null:
+				return {"node": live}
+		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS,
+			"%s '%s' is a declared autoload but isn't instantiated in the editor. " % [role, name] +
+			"Most autoloads are runtime-only; edit-time signal connection isn't supported for them. " +
+			"Connect it from a script attached to the scene using @onready + connect(), " +
+			"or enable editor-instancing for this autoload in Project Settings > Autoload.")
+
+	return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS,
+		"%s node not found: %s (not in scene tree or autoloads)" % [role, path])
 
 
 func _signal_response(source: Node, signal_name: String, target: Node, method: String, scene_root: Node) -> Dictionary: