game_eval: require an ack before treating a missing compile beacon as a parse error (#490 review)

The 3s compile-grace timer inferred a parse error purely from the ABSENCE of
mcp:eval_compiled, conflating two cases: (a) the source failed to parse
(game's _handle_eval aborts at reload() before the beacon), and (b) the game
main thread hasn't serviced the eval message yet — a long frame/load, or a
CPU-bound prior eval (now possible with concurrent evals). In case (b) a valid
eval was false-failed as EVAL_COMPILE_ERROR, and worse, _clear_pending dropped
the eventual real eval_compiled/eval_response as an unknown request_id, losing
the actual result.

Fix (the eval_ack the #490 design originally called for): the game sends
mcp:eval_ack at the top of _handle_eval, BEFORE reload(), so it survives a
parse-error abort. The editor now fires EVAL_COMPILE_ERROR only when the eval
was acked (received + reload started) but never compiled — a positive
compile-failure signal. A missing ack means "not serviced yet", so the grace
defers to the normal timeout and leaves _pending intact, so the real reply is
still delivered.

Refactored the grace lambda into _on_eval_grace(request_id) so the gate is
unit-testable. Added editor-side tests: _on_eval_ack sets the flag; grace fires
EVAL_COMPILE_ERROR when acked-but-not-compiled, defers (no false error, pending
intact) when not acked, and is a no-op when already compiled. Full GDScript
suite green (1366); all 6 live cases re-verified.

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

Author: dsarno

plugin/addons/godot_ai/debugger/mcp_debugger_plugin.gd

@@ -154,6 +154,9 @@ func _capture(message: String, data: Array, _session_id: int) -> bool:
 		"mcp:eval_error":
 			_on_eval_error(data)
 			return true
+		"mcp:eval_ack":
+			_on_eval_ack(data)
+			return true
 		"mcp:eval_compiled":
 			_on_eval_compiled(data)
 			return true
@@ -437,21 +440,12 @@ func _send_eval(
 			_log_buffer.log("[debug] !! eval timeout (%s)" % request_id)
 	timer.timeout.connect(timeout_callable)
 
-	## #490: arm the compile-grace timer. If mcp:eval_compiled hasn't set
-	## `compiled` by the time it fires, the eval source failed to parse.
+	## #490: arm the compile-grace timer. _on_eval_grace concludes a parse error
+	## only when the game acked the eval (it received the message and started
+	## reload()) but never sent mcp:eval_compiled — see there for why a missing
+	## ack must NOT be read as a compile error.
 	var grace: SceneTreeTimer = tree.create_timer(EVAL_COMPILE_GRACE_SEC)
-	var grace_callable := func() -> void:
-		var pending_entry = _pending.get(request_id)
-		if pending_entry == null or pending_entry.get("compiled", false):
-			return
-		_clear_pending(request_id)
-		var conn: McpConnection = pending_entry.connection
-		if conn == null or not is_instance_valid(conn):
-			return
-		_send_error(conn, request_id, ErrorCodes.EVAL_COMPILE_ERROR,
-			"Game eval failed to compile — likely a GDScript syntax/parse error. The parse error text is in the editor's Output/Debugger panel; it is not capturable from the running game. Check your eval code's syntax.")
-		if _log_buffer:
-			_log_buffer.log("[debug] !! eval compile error (%s)" % request_id)
+	var grace_callable := func() -> void: _on_eval_grace(request_id)
 	grace.timeout.connect(grace_callable)
 
 	_pending[request_id] = {
@@ -460,6 +454,7 @@ func _send_eval(
 		"timeout_callable": timeout_callable,
 		"grace_timer": grace,
 		"grace_callable": grace_callable,
+		"acked": false,
 		"compiled": false,
 	}
 
@@ -512,6 +507,47 @@ func _on_eval_error(data: Array) -> void:
 		_log_buffer.log("[debug] <- mcp:eval_error (%s): %s" % [request_id, message])
 
 
+## #490: the game sends this at the top of _handle_eval, BEFORE reload() (so it
+## survives a parse-error abort). It positively signals "the game received this
+## eval and started compiling it" — letting _on_eval_grace tell a real parse
+## error (acked, never compiled) apart from a message the game hasn't serviced
+## yet (never acked — main thread blocked by a long frame/load or a CPU-bound
+## prior eval).
+func _on_eval_ack(data: Array) -> void:
+	if data.is_empty():
+		return
+	var request_id: String = data[0]
+	var pending_entry = _pending.get(request_id)
+	if pending_entry == null:
+		return
+	pending_entry["acked"] = true
+	if _log_buffer:
+		_log_buffer.log("[debug] <- mcp:eval_ack (%s)" % request_id)
+
+
+## #490: compile-grace timer fired. Conclude a parse error ONLY when the game
+## acked the eval (started reload()) but never sent mcp:eval_compiled. If it
+## never acked, the game simply hasn't serviced the message yet — NOT a parse
+## error — so leave _pending intact and let the normal eval timeout handle it
+## rather than false-failing a valid eval and dropping its eventual real reply.
+func _on_eval_grace(request_id: String) -> void:
+	var pending_entry = _pending.get(request_id)
+	if pending_entry == null or pending_entry.get("compiled", false):
+		return
+	if not pending_entry.get("acked", false):
+		if _log_buffer:
+			_log_buffer.log("[debug] eval grace: no ack yet, deferring to timeout (%s)" % request_id)
+		return
+	_clear_pending(request_id)
+	var conn: McpConnection = pending_entry.connection
+	if conn == null or not is_instance_valid(conn):
+		return
+	_send_error(conn, request_id, ErrorCodes.EVAL_COMPILE_ERROR,
+		"Game eval failed to compile — likely a GDScript syntax/parse error. The parse error text is in the editor's Output/Debugger panel; it is not capturable from the running game. Check your eval code's syntax.")
+	if _log_buffer:
+		_log_buffer.log("[debug] !! eval compile error (%s)" % request_id)
+
+
 ## #490: the game sends this the instant reload() of the eval source
 ## succeeds. Flips the pending entry's `compiled` flag so the compile-grace
 ## timer won't fire a false EVAL_COMPILE_ERROR.

plugin/addons/godot_ai/runtime/game_helper.gd

@@ -593,6 +593,12 @@ func _handle_eval(data: Array) -> void:
 
 	var script: GDScript = GDScript.new()
 	script.source_code = script_source
+	## #490: ack BEFORE reload(). A parse error aborts this function at reload()
+	## without a return code in a debug build, so this is our only chance to tell
+	## the editor "received + about to compile." The editor uses that to tell a
+	## real parse error (acked, never compiled) apart from a message it simply
+	## hasn't serviced yet (never acked); see mcp_debugger_plugin._on_eval_grace.
+	EngineDebugger.send_message("mcp:eval_ack", [request_id])
 	## reload() ABORTS this function on a parse error in a debug build (it does
 	## not return a non-OK code there), so the lines below only run when the
 	## source compiled. Keep reload() INLINE — moving it behind a timer/await

test_project/tests/test_game_eval_errors.gd

@@ -344,3 +344,53 @@ func test_clear_pending_disconnects_grace_and_probe_timers() -> void:
 		"_clear_pending disconnects the compile-grace timer")
 	assert_false(probe.timeout.is_connected(probe_cb),
 		"_clear_pending disconnects the runtime probe timer")
+
+
+func test_on_eval_ack_sets_flag() -> void:
+	var plugin := McpDebuggerPlugin.new()
+	var rid := "rid-ack"
+	plugin._pending[rid] = {"connection": null, "acked": false, "compiled": false}
+	plugin._on_eval_ack([rid])
+	assert_true(plugin._pending[rid]["acked"],
+		"mcp:eval_ack flips the pending entry's acked flag")
+
+
+func test_eval_grace_fires_compile_error_when_acked_but_not_compiled() -> void:
+	## The positive compile-failure signal: the game acked (started reload) but
+	## never sent mcp:eval_compiled → the source failed to parse.
+	var plugin := McpDebuggerPlugin.new()
+	var conn := _StubConnection.new()
+	var rid := "rid-grace-compile"
+	plugin._pending[rid] = {"connection": conn, "acked": true, "compiled": false}
+	plugin._on_eval_grace(rid)
+	assert_false(plugin._pending.has(rid), "a confirmed compile error clears pending")
+	assert_eq(conn.captured.size(), 1, "one deferred reply")
+	assert_eq(conn.captured[0]["payload"]["error"]["code"], ErrorCodes.EVAL_COMPILE_ERROR,
+		"replies with EVAL_COMPILE_ERROR")
+	conn.free()
+
+
+func test_eval_grace_defers_when_not_acked() -> void:
+	## The fix: a missing ack means the game hasn't serviced the eval yet (busy
+	## main thread), NOT a parse error. Must NOT fire EVAL_COMPILE_ERROR and must
+	## leave pending intact so the eventual real reply is still delivered.
+	var plugin := McpDebuggerPlugin.new()
+	var conn := _StubConnection.new()
+	var rid := "rid-grace-noack"
+	plugin._pending[rid] = {"connection": conn, "acked": false, "compiled": false}
+	plugin._on_eval_grace(rid)
+	assert_true(plugin._pending.has(rid),
+		"an un-acked eval is left pending (deferred to the normal timeout)")
+	assert_eq(conn.captured.size(), 0, "no false EVAL_COMPILE_ERROR is sent")
+	conn.free()
+
+
+func test_eval_grace_noop_when_already_compiled() -> void:
+	var plugin := McpDebuggerPlugin.new()
+	var conn := _StubConnection.new()
+	var rid := "rid-grace-compiled"
+	plugin._pending[rid] = {"connection": conn, "acked": true, "compiled": true}
+	plugin._on_eval_grace(rid)
+	assert_true(plugin._pending.has(rid), "a compiled eval is untouched by the grace timer")
+	assert_eq(conn.captured.size(), 0, "no compile error for a compiled eval")
+	conn.free()