editor_screenshot: stop returning INTERNAL_ERROR on 2D/empty viewports (#456)

* editor_screenshot: stop returning INTERNAL_ERROR on 2D/empty viewports

Telemetry showed editor_screenshot returning INTERNAL_ERROR on 63
unique installs / 152 hits in 24h, persisting across every plugin
version 2.5.0 -> 2.5.6. The per-uuid distribution is broad and shallow
(25 uuids hit once, 32 hit 2-4 times, only 1 looped 11x), so LLMs try
the default source once, get an unhelpful "Failed to capture image
from viewport" with no signal that the failure is caller-side, and
give up.

Two changes ship together:

1. Pre-check the edited scene root before pulling the 3D viewport
   texture. When source="viewport" and the root is Node2D, Control,
   plain Node, or null (no scene open), return EDITOR_NOT_READY with
   `error.data = {editor_state: "viewport_not_3d", scene_root_type,
   hint}` carrying an actionable hint (open a 3D scene, switch to
   source="cinematic" if a Camera3D exists, or call
   scene_get_hierarchy first). The 3D viewport's texture is empty for
   2D-rooted scenes, and the previous empty-image guard returned
   INTERNAL_ERROR — same shape we use for genuine server bugs, with
   no caller-actionable signal.

2. Reclassify the three remaining empty-image returns that AREN'T
   caller-side debugger-bridge wiring failures:
   - "Failed to capture image from viewport" (main path)
   - "Framed viewport rendered an empty image" (view_target path)
   - "Coverage sweep rendered no images"
   - "Cinematic render produced an empty image"
   These are precondition failures (headless mode, viewport not drawn
   yet, scene-camera misframed) and now return EDITOR_NOT_READY +
   `error.data = {editor_state: "viewport_empty", source, hint}`. The
   two debugger-bridge wiring errors for source="game" stay
   INTERNAL_ERROR — those are genuine server-side failures.

Tool docstring updated to surface the 2D/no-scene constraint and
Camera3D requirement up front.

Tests: 6 new GDScript test cases for viewport_screenshot_precheck
covering Node3D root (passes), Node2D / Control / plain Node / null
roots (all return the structured EDITOR_NOT_READY), and Node3D
subclass (passes). 2 new Python handler tests verify the
viewport_not_3d and viewport_empty error.data shapes propagate
through GodotCommandError to the MCP client.

* editor_screenshot: address review feedback on #456

- Fix #XXX placeholder -> #456 in test header.
- Drop redundant `hint` key from `error.data` in both viewport_not_3d
  and viewport_empty error helpers. The hint copy already lives in
  `error.message`, and GodotCommandError's string form appends every
  `data` key -- keeping the hint in both surfaced the same paragraph
  twice to the LLM. Tests now assert on `error.message` for the hint
  content.
- Make `_empty_image_error` static for consistency with the other
  error-builder helpers; none reference self.

Author: dsarno

plugin/addons/godot_ai/handlers/editor_handler.gd

@@ -279,6 +279,16 @@ func take_screenshot(params: Dictionary) -> Dictionary:
 			viewport = EditorInterface.get_editor_viewport_3d()
 			if viewport == null:
 				return ErrorCodes.make(ErrorCodes.EDITOR_NOT_READY, "No 3D viewport available")
+			## The 3D viewport's texture is empty when the edited scene
+			## has no Node3D content (2D-only scene, or no scene open),
+			## and the empty-image guard further down used to surface
+			## that as INTERNAL_ERROR — leaving callers with no signal
+			## that the failure was caller-side. Reject up front with a
+			## structured hint so the LLM can pick a sensible next step
+			## (open a 3D scene, switch to source="cinematic", etc.).
+			var precheck := viewport_screenshot_precheck(EditorInterface.get_edited_scene_root())
+			if precheck.has("error"):
+				return precheck
 		"game":
 			if not EditorInterface.is_playing_scene():
 				return ErrorCodes.make(ErrorCodes.INVALID_PARAMS, "Game is not running — use source='viewport' or start the project first")
@@ -385,7 +395,10 @@ func take_screenshot(params: Dictionary) -> Dictionary:
 			## Consistent with single-shot path: error if no frames rendered
 			## (e.g. headless mode where force_draw produces no output).
 			if images.is_empty():
-				return ErrorCodes.make(ErrorCodes.INTERNAL_ERROR, "Coverage sweep rendered no images")
+				return _empty_image_error(
+					"viewport",
+					"Coverage sweep rendered no images. The 3D viewport produced no output across any of the preset angles — typically because the editor is in headless mode (force_draw has no rendered output) or the 3D viewport has not drawn a frame yet."
+				)
 
 			var aabb_center := combined_aabb.get_center()
 			var aabb_size := combined_aabb.size
@@ -423,7 +436,10 @@ func take_screenshot(params: Dictionary) -> Dictionary:
 		RenderingServer.camera_set_transform(cam_rid, saved_xform)
 
 		if image == null or image.is_empty():
-			return ErrorCodes.make(ErrorCodes.INTERNAL_ERROR, "Framed viewport rendered an empty image")
+			return _empty_image_error(
+				"viewport",
+				"Framed viewport rendered an empty image after repositioning the camera onto the view_target. The 3D viewport produced no output — typically headless mode or the 3D viewport has not drawn a frame yet."
+			)
 
 		var result := _finalize_image(image, "viewport", max_resolution)
 		result.data["view_target"] = view_target
@@ -445,7 +461,10 @@ func take_screenshot(params: Dictionary) -> Dictionary:
 	var image: Image = viewport.get_texture().get_image()
 
 	if image == null or image.is_empty():
-		return ErrorCodes.make(ErrorCodes.INTERNAL_ERROR, "Failed to capture image from %s" % source)
+		return _empty_image_error(
+			source,
+			"Captured an empty image from %s. The 3D viewport produced no output — typically headless mode or the 3D viewport has not drawn a frame yet." % source
+		)
 
 	return _finalize_image(image, source, max_resolution)
 
@@ -507,13 +526,81 @@ func _take_cinematic_screenshot(max_resolution: int) -> Dictionary:
 	sub_vp.queue_free()
 
 	if image == null or image.is_empty():
-		return ErrorCodes.make(ErrorCodes.INTERNAL_ERROR, "Cinematic render produced an empty image")
+		return _empty_image_error(
+			"cinematic",
+			"Cinematic render produced an empty image. The SubViewport returned no texture — typically headless mode (force_draw has no rendered output) or the scene's Camera3D is positioned so nothing visible is in frame."
+		)
 
 	var result := _finalize_image(image, "cinematic", max_resolution)
 	result.data["camera_path"] = McpScenePath.from_node(scene_camera, scene_root)
 	return result
 
 
+## Reject a `source="viewport"` screenshot before we ever pull the
+## texture if the edited scene has no Node3D content. The 3D viewport
+## returns an empty (or stale) image in that case; surfacing it as
+## INTERNAL_ERROR ("Failed to capture image from viewport") gave LLM
+## callers no signal that the right move is to switch source or open a
+## 3D scene. 152 hits / 63 uuids in 24h across plugin versions 2.5.0 ->
+## 2.5.6 traced back to this. Returns `{}` on success.
+##
+## Caller passes `EditorInterface.get_edited_scene_root()`; the static
+## form lets tests exercise the branches with a synthetic scene root
+## without driving the editor.
+static func viewport_screenshot_precheck(scene_root: Node) -> Dictionary:
+	if scene_root == null:
+		return _make_viewport_not_3d_error(
+			"",
+			"The editor 3D viewport is empty because no scene is open. Open a scene with `scene_open` first."
+		)
+	if scene_root is Node3D:
+		return {}
+	## Anything that isn't a Node3D — Node2D, Control, plain Node — leaves
+	## the 3D viewport with nothing to render. Report the real root type
+	## so the caller can tell why the default source failed.
+	var root_type := scene_root.get_class()
+	var hint: String
+	if scene_root is Node2D or scene_root is Control:
+		hint = (
+			"The 3D viewport is empty because the current scene is 2D (%s root). "
+			+ "Options: (a) open a 3D scene, "
+			+ "(b) use source=\"cinematic\" if a Camera3D exists in the scene, "
+			+ "(c) call scene_get_hierarchy first to inspect what's available."
+		) % root_type
+	else:
+		hint = (
+			"The 3D viewport is empty because the current scene root (%s) has no Node3D content. "
+			+ "Options: (a) open a scene whose root is a Node3D, "
+			+ "(b) use source=\"cinematic\" if a Camera3D exists in the scene, "
+			+ "(c) call scene_get_hierarchy first to inspect what's available."
+		) % root_type
+	return _make_viewport_not_3d_error(root_type, hint)
+
+
+static func _make_viewport_not_3d_error(scene_root_type: String, hint: String) -> Dictionary:
+	## `hint` becomes `error.message`; not duplicated into `data` because
+	## `GodotCommandError`'s string form already appends every `data` key
+	## as a suffix on the agent-visible error.
+	var err := ErrorCodes.make(ErrorCodes.EDITOR_NOT_READY, hint)
+	err["error"]["data"] = {
+		"editor_state": "viewport_not_3d",
+		"scene_root_type": scene_root_type,
+	}
+	return err
+
+
+## Reached only when the precheck passed but the texture still came
+## back empty — headless rendering, a freshly opened editor whose 3D
+## viewport hasn't drawn a frame, or a SubViewport that lost its target.
+static func _empty_image_error(source: String, hint: String) -> Dictionary:
+	var err := ErrorCodes.make(ErrorCodes.EDITOR_NOT_READY, hint)
+	err["error"]["data"] = {
+		"editor_state": "viewport_empty",
+		"source": source,
+	}
+	return err
+
+
 ## Return the Camera3D that would be active if the scene were running.
 ## Preference: a descendant with `current=true`, else the first Camera3D
 ## found in a depth-first walk.

src/godot_ai/tools/editor.py

@@ -129,10 +129,19 @@ async def editor_screenshot(
     ):
         """Capture a screenshot of the Godot editor viewport or running game.
 
+        Picking a source: the default ``"viewport"`` only works for a 3D-rooted
+        scene — the editor's 3D viewport is what gets captured. A 2D scene
+        (Node2D/Control root) or an editor with no scene open returns
+        ``EDITOR_NOT_READY`` carrying ``error.data = {editor_state:
+        "viewport_not_3d", scene_root_type, hint}``; switch to ``"cinematic"``
+        if the scene has a Camera3D, or open a 3D scene first.
+
         Sources:
-        - "viewport" (default): editor 3D viewport.
+        - "viewport" (default): editor 3D viewport. Requires a 3D-rooted scene
+          currently being edited; see above for the 2D / no-scene error shape.
         - "cinematic": render edited scene through its current Camera3D
-          (no editor gizmos). INVALID_PARAMS if no current Camera3D.
+          (no editor gizmos). Requires a Camera3D in the scene; NODE_NOT_FOUND
+          if none is marked ``current``.
         - "game": running game's framebuffer (only when project is running).
 
         ``include_image=True`` (default) returns an MCP ImageContent block.

test_project/tests/test_editor.gd

@@ -128,6 +128,71 @@ func test_screenshot_game_not_playing() -> void:
 	assert_is_error(result)
 
 
+# ----- viewport_screenshot_precheck (#456: stop returning INTERNAL_ERROR on 2D) -----
+
+func test_viewport_precheck_passes_for_node3d_root() -> void:
+	## Happy path: scene_root is Node3D, precheck returns empty dict so
+	## take_screenshot falls through to its normal capture path.
+	var root := Node3D.new()
+	var result := EditorHandler.viewport_screenshot_precheck(root)
+	root.free()
+	assert_eq(result, {}, "Node3D root should return {} (no error)")
+
+
+func test_viewport_precheck_rejects_node2d_root() -> void:
+	var root := Node2D.new()
+	var result := EditorHandler.viewport_screenshot_precheck(root)
+	root.free()
+	assert_is_error(result, ErrorCodes.EDITOR_NOT_READY)
+	assert_has_key(result.error, "data")
+	assert_eq(result.error.data.editor_state, "viewport_not_3d")
+	assert_eq(result.error.data.scene_root_type, "Node2D")
+	## The message must mention the 2D nature, cinematic alternative, and
+	## scene_get_hierarchy so the LLM can actually act on it.
+	assert_contains(result.error.message, "Node2D")
+	assert_contains(result.error.message, "cinematic")
+	assert_contains(result.error.message, "scene_get_hierarchy")
+
+
+func test_viewport_precheck_rejects_control_root() -> void:
+	var root := Control.new()
+	var result := EditorHandler.viewport_screenshot_precheck(root)
+	root.free()
+	assert_is_error(result, ErrorCodes.EDITOR_NOT_READY)
+	assert_eq(result.error.data.editor_state, "viewport_not_3d")
+	assert_eq(result.error.data.scene_root_type, "Control")
+
+
+func test_viewport_precheck_rejects_plain_node_root() -> void:
+	## A scene rooted at a plain Node has no Node3D content and no 2D
+	## either — still no 3D viewport content, so still rejected, but the
+	## hint phrasing is the generic non-3D form rather than the 2D one.
+	var root := Node.new()
+	var result := EditorHandler.viewport_screenshot_precheck(root)
+	root.free()
+	assert_is_error(result, ErrorCodes.EDITOR_NOT_READY)
+	assert_eq(result.error.data.editor_state, "viewport_not_3d")
+	assert_eq(result.error.data.scene_root_type, "Node")
+	assert_contains(result.error.message, "no Node3D content")
+
+
+func test_viewport_precheck_rejects_null_scene() -> void:
+	var result := EditorHandler.viewport_screenshot_precheck(null)
+	assert_is_error(result, ErrorCodes.EDITOR_NOT_READY)
+	assert_eq(result.error.data.editor_state, "viewport_not_3d")
+	assert_eq(result.error.data.scene_root_type, "")
+	assert_contains(result.error.message, "no scene is open")
+
+
+func test_viewport_precheck_passes_for_node3d_subclass() -> void:
+	## Camera3D extends Node3D — a scene rooted at any Node3D subclass
+	## should pass the precheck (the 3D viewport will render it).
+	var root := Camera3D.new()
+	var result := EditorHandler.viewport_screenshot_precheck(root)
+	root.free()
+	assert_eq(result, {}, "Node3D subclass root should pass")
+
+
 func test_debugger_plugin_capture_prefix() -> void:
 	var plugin := McpDebuggerPlugin.new()
 	assert_true(plugin._has_capture("mcp"), "Should accept 'mcp' prefix")

tests/unit/test_runtime_handlers.py

@@ -3369,6 +3369,71 @@ async def test_editor_screenshot_handler_cinematic_surfaces_camera_path():
     assert result["camera_path"] == "/Main/Camera3D"
 
 
+async def test_editor_screenshot_handler_relays_viewport_not_3d_error():
+    """When the plugin rejects a viewport screenshot on a 2D scene with the
+    new structured EDITOR_NOT_READY + data.editor_state=viewport_not_3d, the
+    handler must surface the data dict on the raised GodotCommandError so
+    LLM callers see the hint and can switch source or open a 3D scene.
+    Fixes the 152-hit / 63-uuid INTERNAL_ERROR cluster on editor_screenshot.
+    """
+    from godot_ai.godot_client.client import GodotCommandError
+
+    class ViewportNot3DClient:
+        async def send(self, command, params=None, session_id=None, timeout=5.0):
+            raise GodotCommandError(
+                code="EDITOR_NOT_READY",
+                message=(
+                    "The 3D viewport is empty because the current scene is 2D "
+                    "(Node2D root). Options: (a) open a 3D scene, "
+                    "(b) use source=\"cinematic\" if a Camera3D exists in the scene, "
+                    "(c) call scene_get_hierarchy first to inspect what's available."
+                ),
+                data={
+                    "editor_state": "viewport_not_3d",
+                    "scene_root_type": "Node2D",
+                },
+            )
+
+    runtime = DirectRuntime(registry=SessionRegistry(), client=ViewportNot3DClient())
+    with pytest.raises(GodotCommandError) as excinfo:
+        await editor_handlers.editor_screenshot(runtime, include_image=False)
+    err = excinfo.value
+    assert err.code == "EDITOR_NOT_READY"
+    assert err.data["editor_state"] == "viewport_not_3d"
+    assert err.data["scene_root_type"] == "Node2D"
+    ## Hint copy lives in `message` (not duplicated into `data`); the LLM
+    ## still sees it via str(err) since GodotCommandError formats the
+    ## message + data suffix.
+    assert "scene_get_hierarchy" in err.message
+
+
+async def test_editor_screenshot_handler_relays_viewport_empty_error():
+    """The fallback empty-image guards (post-precheck) now return
+    EDITOR_NOT_READY + data.editor_state=viewport_empty instead of
+    INTERNAL_ERROR. Verify the data dict propagates through the handler.
+    """
+    from godot_ai.godot_client.client import GodotCommandError
+
+    class ViewportEmptyClient:
+        async def send(self, command, params=None, session_id=None, timeout=5.0):
+            raise GodotCommandError(
+                code="EDITOR_NOT_READY",
+                message="Captured an empty image from viewport.",
+                data={
+                    "editor_state": "viewport_empty",
+                    "source": "viewport",
+                },
+            )
+
+    runtime = DirectRuntime(registry=SessionRegistry(), client=ViewportEmptyClient())
+    with pytest.raises(GodotCommandError) as excinfo:
+        await editor_handlers.editor_screenshot(runtime, include_image=False)
+    err = excinfo.value
+    assert err.code == "EDITOR_NOT_READY"
+    assert err.data["editor_state"] == "viewport_empty"
+    assert err.data["source"] == "viewport"
+
+
 # ---------------------------------------------------------------------------
 # Performance monitor handler tests
 # ---------------------------------------------------------------------------