Skip to content

Commit 2d7035c

Browse files
dsarnoclaude
andcommitted
Address code-review findings on the logger fix (F1/F3/F4)
F1 (merge-blocker): logger_loader.build() no longer sets script.resource_path. Setting it collided on the 2nd+ build of the same path in one process (editor_reload_plugin / self-update reload cycle), printing a red "Another resource is loaded from path ..." ERROR and leaving the script with an empty path anyway — re-introducing the exact red console text this .gdignore change set out to remove, on ALL engine versions. Matches game_helper.gd::_handle_eval, which compiles from source the same way and omits resource_path. Verified: a plugin reload now produces zero collision errors (was 1 per reload). F3: plugin.gd::_cleanup_legacy_logger_scripts() deletes the pre-2.5.8 runtime/editor_logger.gd + game_logger.gd (+ .uid) if present. A self-update from an older version leaves them orphaned (the runner only writes files in the new ZIP, never prunes), and they'd re-emit the "Could not find base class Logger" parse errors on Godot 4.4 (self-update allowed by the gate; Logger absent until 4.5). Existence-guarded, so a no-op on fresh installs and dev checkouts. F4: editor_logger.gd resolves McpLogBacktrace via `const preload(...)` instead of the bare class_name, matching game_logger.gd. Removes the cold-enable class-registry-timing risk where build() could compile before the global class table is populated -> reload() fails -> editor log capture silently never attaches. Verified: 4.6 import clean (0 parse, 0 collision), reload clean, editor logger suite 18/18, ruff + pytest green (1088). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 280e82d commit 2d7035c

3 files changed

Lines changed: 44 additions & 5 deletions

File tree

plugin/addons/godot_ai/plugin.gd

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,13 @@ func _enter_tree() -> void:
194194
print("MCP | plugin disabled in headless mode")
195195
return
196196

197+
## Self-update from a pre-loggers/ version leaves the old logger scripts
198+
## orphaned at runtime/*.gd (the runner only writes files in the new ZIP,
199+
## it doesn't prune). Those still `extends Logger` and re-emit the parse
200+
## errors on Godot < 4.5. Delete them once so upgraders match a fresh
201+
## install. No-op on fresh installs and dev checkouts (files absent).
202+
_cleanup_legacy_logger_scripts()
203+
197204
## Register port overrides before spawn so `http_port()` / `ws_port()`
198205
## return the user's configured values (if any) when `_start_server`
199206
## builds the CLI args.
@@ -513,6 +520,24 @@ func _attach_editor_logger() -> void:
513520
OS.call("add_logger", _editor_logger)
514521

515522

523+
## Remove the pre-2.5.8 logger scripts left at runtime/*.gd by a self-update
524+
## (the runner doesn't prune files dropped between versions). They `extends
525+
## Logger` and would re-emit "Could not find base class Logger" parse errors
526+
## on Godot < 4.5 even though the live copies now live in the .gdignore'd
527+
## runtime/loggers/ folder. Idempotent: existence-guarded, so it's a no-op on
528+
## fresh installs and symlinked dev checkouts.
529+
func _cleanup_legacy_logger_scripts() -> void:
530+
var legacy := [
531+
"res://addons/godot_ai/runtime/editor_logger.gd",
532+
"res://addons/godot_ai/runtime/editor_logger.gd.uid",
533+
"res://addons/godot_ai/runtime/game_logger.gd",
534+
"res://addons/godot_ai/runtime/game_logger.gd.uid",
535+
]
536+
for res_path in legacy:
537+
if FileAccess.file_exists(res_path):
538+
DirAccess.remove_absolute(ProjectSettings.globalize_path(res_path))
539+
540+
516541
func _detach_editor_logger() -> void:
517542
if _editor_logger != null and OS.has_method("remove_logger"):
518543
OS.call("remove_logger", _editor_logger)

plugin/addons/godot_ai/runtime/logger_loader.gd

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,16 @@ static func build(path: String) -> GDScript:
4141
return null
4242
var script := GDScript.new()
4343
script.source_code = source
44-
## Give it a stable resource path so error messages and any internal
45-
## path resolution behave; the path is `.gdignore`'d so this never
46-
## collides with an imported resource.
47-
script.resource_path = path
44+
## Deliberately do NOT set `script.resource_path`: this builds a fresh
45+
## anonymous GDScript every call, and a reload cycle (editor_reload_plugin,
46+
## self-update disable→enable) calls build() again for the same path. Two
47+
## live Resources sharing one non-empty resource_path trips Godot's
48+
## "Another resource is loaded from path ..." error and leaves the new
49+
## script with an empty path anyway — re-introducing red console text on
50+
## every reload, the exact thing this folder's .gdignore set out to remove.
51+
## game_helper.gd::_handle_eval compiles from source the same way and also
52+
## omits resource_path. The script still resolves its absolute preloads /
53+
## class_names fine without a path.
4854
if script.reload() != OK:
4955
return null
5056
return script

plugin/addons/godot_ai/runtime/loggers/editor_logger.gd

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,14 @@ extends Logger
3333

3434
const ADDON_PATH_MARKER := "/addons/godot_ai/"
3535

36+
## Resolve McpLogBacktrace by path, not by the `McpLogBacktrace` class_name.
37+
## This script is compiled from source at runtime by logger_loader.gd; a bare
38+
## class_name reference depends on the global class-name table being populated
39+
## at compile time, which isn't guaranteed on a cold editor enable mid-scan.
40+
## `const preload` resolves at compile time independent of the registry —
41+
## matches game_logger.gd's deliberate choice for the same reason.
42+
const _LogBacktrace := preload("res://addons/godot_ai/utils/log_backtrace.gd")
43+
3644
## McpEditorLogBuffer — untyped because this script is loaded dynamically and
3745
## McpEditorLogBuffer's class_name isn't yet registered on the parser at the
3846
## time `extends Logger` resolves. Constructor-injected so the hot path
@@ -65,7 +73,7 @@ func _log_error(
6573
var message_res_path := _extract_user_res_path(message)
6674
if not _is_user_script(file) and script_backtraces.is_empty() and message_res_path.is_empty():
6775
return
68-
var resolved := McpLogBacktrace.resolve_error(
76+
var resolved := _LogBacktrace.resolve_error(
6977
function, file, line, code, rationale, error_type, script_backtraces,
7078
)
7179
if not _is_user_script(resolved.path):

0 commit comments

Comments
 (0)