Address Copilot review on #16

Five fixes from the Copilot reviewer pass:

1. build_layout: validate loaded resource is a Theme before assigning to
   node.theme. A non-Theme resource path (e.g. a .tres StandardMaterial3D)
   now errors with INVALID_PARAMS instead of silently failing at runtime.

2. build_layout _apply_property: when Color/Vector2/Vector2i/NodePath
   coercion fails, return INVALID_PARAMS instead of silently passing the
   raw value through to node.set(). _coerce_for_type now returns
   {"ok": bool, "value": ...} so failures are explicit.

3. create_theme overwritten: captured pre-save existence flag instead of
   computing it post-save (which was always true when overwrite=true).

4. _validate_res_path: accepts a param_name (default "theme_path") so
   create_theme reports "Missing required param: path" rather than
   "theme_path". Applies the name uniformly across all three validation
   branches.

5. _set_scalar: reject null value explicitly with a clear error message.
   Previously a null payload slipped past the parser failure check and
   reached Theme.set_color(null) / equivalent.

Adds GDScript regression tests for:
- theme: null-value rejection, overwritten-flag accuracy on first vs
  second create, param-name-aware missing-path error
- build_layout: non-Theme resource rejection, uncoercible property
  rejection

Author: claude

plugin/addons/godot_ai/handlers/theme_handler.gd

@@ -25,11 +25,14 @@ func create_theme(params: Dictionary) -> Dictionary:
 	var path: String = params.get("path", "")
 	var overwrite: bool = params.get("overwrite", false)
 
-	var err := _validate_res_path(path, ".tres")
+	var err := _validate_res_path(path, ".tres", "path")
 	if err != null:
 		return err
 
-	if FileAccess.file_exists(path) and not overwrite:
+	# Capture whether the file was already there BEFORE the save so we can
+	# report `overwritten` accurately (after save the file always exists).
+	var existed_before := FileAccess.file_exists(path)
+	if existed_before and not overwrite:
 		return McpErrorCodes.make(
 			McpErrorCodes.INVALID_PARAMS,
 			"Theme already exists at %s (pass overwrite=true to replace)" % path
@@ -51,7 +54,7 @@ func create_theme(params: Dictionary) -> Dictionary:
 	return {
 		"data": {
 			"path": path,
-			"overwritten": overwrite and FileAccess.file_exists(path),
+			"overwritten": existed_before,
 			"undoable": false,
 			"reason": "File creation is persistent; delete the file manually to revert",
 		}
@@ -115,9 +118,15 @@ func _set_scalar(
 	if not "value" in params:
 		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: value")
 
-	var parsed = parser.call(params.get("value"))
-	if parsed == null and params.get("value") != null:
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Invalid %s value: %s" % [kind, params.get("value")])
+	var raw_value = params.get("value")
+	if raw_value == null:
+		return McpErrorCodes.make(
+			McpErrorCodes.INVALID_PARAMS,
+			"Invalid %s value: null (pass a concrete value; use the appropriate clear command to remove a slot)" % kind
+		)
+	var parsed = parser.call(raw_value)
+	if parsed == null:
+		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Invalid %s value: %s" % [kind, raw_value])
 
 	var had_before: bool = has_fn.call(theme, name, class_name_param)
 	var before_value = getter.call(theme, name, class_name_param) if had_before else null
@@ -331,15 +340,18 @@ func _load_theme_from_params(params: Dictionary) -> Dictionary:
 	return {"theme": theme, "path": theme_path}
 
 
-static func _validate_res_path(path: String, required_suffix: String) -> Variant:
+static func _validate_res_path(path: String, required_suffix: String, param_name: String = "theme_path") -> Variant:
 	if path.is_empty():
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: theme_path")
+		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: %s" % param_name)
 	if not path.begins_with("res://"):
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Path must start with res:// (got %s)" % path)
+		return McpErrorCodes.make(
+			McpErrorCodes.INVALID_PARAMS,
+			"%s must start with res:// (got %s)" % [param_name, path]
+		)
 	if not path.ends_with(required_suffix):
 		return McpErrorCodes.make(
 			McpErrorCodes.INVALID_PARAMS,
-			"Path must end with %s (got %s)" % [required_suffix, path]
+			"%s must end with %s (got %s)" % [param_name, required_suffix, path]
 		)
 	return null
 

plugin/addons/godot_ai/handlers/ui_handler.gd

@@ -237,10 +237,13 @@ func _build_subtree(spec: Dictionary) -> Dictionary:
 				node.free()
 				return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Theme not found: %s" % theme_path)
 			var theme_res: Resource = ResourceLoader.load(theme_path)
+			if theme_res == null or not theme_res is Theme:
+				node.free()
+				return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "theme path must point to a Theme resource: %s" % theme_path)
 			if not node is Control and not node is Window:
 				node.free()
 				return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "theme can only be set on Control / Window (got %s)" % node_type)
-			node.theme = theme_res
+			node.theme = theme_res as Theme
 
 	# Anchor preset — applied before children so children inherit sensible anchors.
 	if spec.has("anchor_preset"):
@@ -292,40 +295,59 @@ func _apply_property(node: Node, prop: String, value: Variant) -> Variant:
 			"Property '%s' not found on %s" % [prop, node.get_class()]
 		)
 
-	var coerced := _coerce_for_type(value, prop_type)
-	node.set(prop, coerced)
+	var coercion := _coerce_for_type(value, prop_type)
+	if not coercion.ok:
+		return McpErrorCodes.make(
+			McpErrorCodes.INVALID_PARAMS,
+			"Property '%s' on %s expects type %s (cannot coerce %s)" % [
+				prop, node.get_class(), type_string(prop_type), value
+			]
+		)
+	node.set(prop, coercion.value)
 	return null
 
 
-static func _coerce_for_type(value: Variant, prop_type: int) -> Variant:
+## Coerce a JSON-friendly value to the target Godot type. Returns
+## {"ok": true, "value": coerced} on success, {"ok": false} on failure.
+## For types we don't explicitly coerce, the value is returned as-is
+## (Godot will typecheck at set() time and fail loudly if it disagrees).
+static func _coerce_for_type(value: Variant, prop_type: int) -> Dictionary:
 	match prop_type:
 		TYPE_COLOR:
 			if value is Color:
-				return value
+				return {"ok": true, "value": 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))
 				if a == b:
-					return a
+					return {"ok": true, "value": a}
+				return {"ok": false}
 			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": true,
+					"value": Color(float(value.r), float(value.g), float(value.b), float(value.get("a", 1.0))),
+				}
+			return {"ok": false}
 		TYPE_VECTOR2:
 			if value is Vector2:
-				return value
+				return {"ok": true, "value": value}
 			if value is Dictionary and value.has("x") and value.has("y"):
-				return Vector2(float(value.x), float(value.y))
+				return {"ok": true, "value": 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": true, "value": Vector2(float(value[0]), float(value[1]))}
+			return {"ok": false}
 		TYPE_VECTOR2I:
 			if value is Vector2i:
-				return value
+				return {"ok": true, "value": value}
 			if value is Dictionary and value.has("x") and value.has("y"):
-				return Vector2i(int(value.x), int(value.y))
+				return {"ok": true, "value": Vector2i(int(value.x), int(value.y))}
 			if value is Array and value.size() == 2:
-				return Vector2i(int(value[0]), int(value[1]))
+				return {"ok": true, "value": Vector2i(int(value[0]), int(value[1]))}
+			return {"ok": false}
 		TYPE_NODE_PATH:
 			if value is NodePath:
-				return value
+				return {"ok": true, "value": value}
 			if value is String:
-				return NodePath(value)
-	return value
+				return {"ok": true, "value": NodePath(value)}
+			return {"ok": false}
+	return {"ok": true, "value": value}

test_project/tests/test_theme.gd

@@ -260,3 +260,40 @@ func test_theme_apply_rejects_non_control() -> void:
 	})
 	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
 	assert_contains(result.error.message, "not a Control")
+
+
+# ----- Regression: Copilot review fixes -----
+
+func test_theme_set_color_rejects_null_value() -> void:
+	_make_theme()
+	var result := _handler.set_color({
+		"theme_path": TEST_THEME_PATH,
+		"class_name": "Label",
+		"name": "font_color",
+		"value": null,
+	})
+	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
+	assert_contains(result.error.message, "null")
+
+
+func test_create_theme_overwritten_flag_tracks_pre_save_state() -> void:
+	# Fresh location — overwritten must be false even when overwrite=true.
+	if FileAccess.file_exists(TEST_THEME_PATH):
+		DirAccess.remove_absolute(TEST_THEME_PATH)
+	var result := _handler.create_theme({"path": TEST_THEME_PATH, "overwrite": true})
+	assert_has_key(result, "data")
+	assert_eq(result.data.overwritten, false, "Overwritten should be false on fresh create")
+
+	# Second call: now it should be true.
+	var result2 := _handler.create_theme({"path": TEST_THEME_PATH, "overwrite": true})
+	assert_has_key(result2, "data")
+	assert_eq(result2.data.overwritten, true, "Overwritten should be true on second create")
+
+
+func test_create_theme_missing_path_names_param_correctly() -> void:
+	# Error message should name `path`, not `theme_path`, for theme_create.
+	var result := _handler.create_theme({})
+	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
+	assert_contains(result.error.message, "path")
+	# Make sure it's NOT using the default "theme_path" label.
+	assert_true(result.error.message.find("theme_path") == -1, "Error should say 'path', not 'theme_path'")

test_project/tests/test_ui.gd

@@ -296,3 +296,26 @@ func test_build_layout_is_undoable() -> void:
 	assert_eq(scene_root.get_child_count(), before_count + 1)
 	_undo_redo.undo()
 	assert_eq(scene_root.get_child_count(), before_count, "Undo should remove the whole built tree")
+
+
+func test_build_layout_rejects_non_theme_resource() -> void:
+	# A .tres that is not a Theme — use a StandardMaterial3D saved to disk.
+	var bogus_path := "res://tests/_mcp_test_not_a_theme.tres"
+	var mat := StandardMaterial3D.new()
+	ResourceSaver.save(mat, bogus_path)
+	var result := _handler.build_layout({
+		"tree": {"type": "Panel", "theme": bogus_path}
+	})
+	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
+	assert_contains(result.error.message, "Theme resource")
+	if FileAccess.file_exists(bogus_path):
+		DirAccess.remove_absolute(bogus_path)
+
+
+func test_build_layout_rejects_uncoercible_property() -> void:
+	# Label.modulate is a Color — "not a color" must be rejected (not silently passed).
+	var result := _handler.build_layout({
+		"tree": {"type": "Label", "properties": {"modulate": "not a color!!"}}
+	})
+	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
+	assert_contains(result.error.message, "modulate")