Eliminate benign Logger parse errors on Godot < 4.5 (#475 follow-up)#483
Conversation
editor_logger.gd and game_logger.gd `extends Logger` (a 4.5+ class). The
editor filesystem scan parses every .gd under the project, so on 4.3/4.4
each printed a `Parse Error: Could not find base class "Logger"` (+ a
"Failed to load script" follow-up) on every editor startup. Functionally
harmless — the scripts are only instanced behind a
`ClassDB.class_exists("Logger")` gate — but real red error text we
shouldn't ship.
Fix: move both files into `runtime/loggers/`, which carries a `.gdignore`
so Godot's editor scan skips the folder entirely (no parse, no error, on
any engine). A new plain-RefCounted `runtime/logger_loader.gd` (parses on
every version) compiles them from on-disk source via `FileAccess` +
`GDScript.new()` at runtime — `FileAccess` is unaffected by `.gdignore`,
which only governs the resource importer. plugin.gd and game_helper.gd
call `LoggerLoader.build(...)` past their existing
`ClassDB.class_exists("Logger")` gate, so the `extends Logger` parse only
ever happens on 4.5+ where it resolves.
Also drops the now-moot `ALLOW_LOGGER_PARSE_ERRORS` filter from
ci-check-gdscript + ci.yml: with the errors gone, the check is strict on
every engine again (a real Logger error now fails CI on any version).
Verified:
- 4.3: `godot --import` + strict `ci-check-gdscript` -> ZERO parse errors
(the two Logger lines are gone).
- 4.6: same strict check passes; editor log-capture suite 18/18 green
(12 editor_logger + 6 game_logger) — the runtime source-build loader
produces a working Logger subclass; full editor suite 91 pass / 0 fail.
- test_editor.gd logger tests switched from load(<path>) to
LoggerLoader.build(<path>) since load() can't resolve a .gdignore'd
path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The .gdignore move relocated game_logger.gd to runtime/loggers/, but test_game_logger_no_class_name_lookup.py still read it from the old runtime/ path -> FileNotFoundError on the Python CI rows. Point it at the new path. (My miss: this is a Python test that reads the GDScript file by path, so the GDScript-only move broke it; I should have run pytest before pushing.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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>
…GGER_PARSE_ERRORS (F5) The docs still described the pre-fix state: extends Logger scripts at runtime/*.gd that parse-error on <4.5, suppressed by a now-removed ALLOW_LOGGER_PARSE_ERRORS ci-check-gdscript filter. This change moved them into the .gdignore-d runtime/loggers/ folder (built at runtime by logger_loader.gd) and made ci-check-gdscript strict on all versions. - CLAUDE.md: rewrite the Logger bullet (.gdignore + loader + the must-not-set-resource_path note); strict ci-check on all versions. - skill.md / testing-strategy.md: drop the ALLOW_LOGGER_PARSE_ERRORS sentences; note the .gdignore mechanism. - plugin-architecture.md: file tree now shows runtime/loggers/ + logger_loader.gd. Docs only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR eliminates noisy-but-benign extends Logger parse errors on Godot < 4.5 by preventing the editor filesystem scan from parsing the Logger subclasses, while still allowing them to be instantiated at runtime on Godot 4.5+.
Changes:
- Move
editor_logger.gd/game_logger.gdunder a.gdignore’druntime/loggers/folder to avoid editor-scan parsing on older Godot versions. - Add
runtime/logger_loader.gdto compile the ignored logger scripts from on-disk source viaFileAccess+GDScript.new()(only after the existingClassDB.class_exists("Logger")gate). - Remove the CI allowlist for Logger parse errors, restoring strict
ci-check-gdscriptbehavior on all engine versions and updating docs/tests accordingly.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_game_logger_no_class_name_lookup.py | Updates the on-disk path used by the unit test to the new runtime/loggers/ location. |
| test_project/tests/test_editor.gd | Switches logger tests from load(path) to LoggerLoader.build(path) for .gdignore’d scripts. |
| script/ci-check-gdscript | Removes the Logger-specific filter and makes parse-error detection strict again on all versions. |
| plugin/addons/godot_ai/runtime/loggers/game_logger.gd | Relocates game logger and updates header comments for the new .gdignore + loader model. |
| plugin/addons/godot_ai/runtime/loggers/editor_logger.gd | Relocates editor logger; adds path-based preload for log backtrace resolution. |
| plugin/addons/godot_ai/runtime/loggers/.gdignore | Ensures the runtime/loggers/ folder is skipped by the editor filesystem scan. |
| plugin/addons/godot_ai/runtime/logger_loader.gd | New runtime compiler for .gdignore’d logger scripts. |
| plugin/addons/godot_ai/runtime/logger_loader.gd.uid | UID for the new loader script. |
| plugin/addons/godot_ai/runtime/game_helper.gd | Uses LoggerLoader.build(...) to attach the game logger at runtime. |
| plugin/addons/godot_ai/plugin.gd | Uses LoggerLoader.build(...) for the editor logger; adds one-time cleanup for legacy logger scripts after self-update. |
| docs/testing-strategy.md | Documents that GDScript validation is strict again on all versions. |
| docs/plugin-architecture.md | Updates the runtime layout documentation to include logger_loader.gd and runtime/loggers/. |
| CLAUDE.md | Updates version-compat guidance to reflect .gdignore + runtime compilation for Logger scripts. |
| .github/workflows/ci.yml | Removes ALLOW_LOGGER_PARSE_ERRORS env wiring for the 4.3 canary row. |
| .claude/skills/godot-ai/skill.md | Updates CI guidance, but still contains an outdated later bullet that needs alignment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| PLUGIN_ROOT = Path(__file__).resolve().parents[2] / "plugin" / "addons" / "godot_ai" | ||
| GAME_LOGGER = PLUGIN_ROOT / "runtime" / "game_logger.gd" | ||
| GAME_LOGGER = PLUGIN_ROOT / "runtime" / "loggers" / "game_logger.gd" |
There was a problem hiding this comment.
Fixed in af1ef8e — docstring example now points at runtime/loggers/game_logger.gd.
| Test guardrails: the runner flags tests with 0 assertions as failures (catches silent `return` before asserting). Always use `assert_true(false, "reason")` before early `return` in test methods. Test discovery is resilient — a broken `.gd` file doesn't kill discovery of the rest. | ||
|
|
||
| Version-gated skips: for a test that depends on 4.4+-only behavior, call `if skip_on_godot_lt("4.4", "reason"): return` at the top (`McpTestSuite.skip_on_godot_lt` returns `bool`). CI runs a Godot 4.3 Linux canary (`Godot tests / Linux (Godot 4.3)`, pinned to `4.3.0`) in addition to the three 4.6.2 OS rows; the canary sets `ALLOW_LOGGER_PARSE_ERRORS=1` (suppresses the benign `extends Logger` parse errors in `ci-check-gdscript`) and `SKIP_POSTCHURN_TEST_RUN=1` (the reload smoke's post-churn `test_run` outruns the 30s curl timeout on slow 4.3 GDScript exec). | ||
| Version-gated skips: for a test that depends on 4.4+-only behavior, call `if skip_on_godot_lt("4.4", "reason"): return` at the top (`McpTestSuite.skip_on_godot_lt` returns `bool`). CI runs a Godot 4.3 Linux canary (`Godot tests / Linux (Godot 4.3)`, pinned to `4.3.0`) in addition to the three 4.6.2 OS rows; the canary sets `SKIP_POSTCHURN_TEST_RUN=1` (the reload smoke's post-churn `test_run` outruns the 30s curl timeout on slow 4.3 GDScript exec). `ci-check-gdscript` is strict on all versions — the `extends Logger` scripts live in the `.gdignore`'d `runtime/loggers/` folder (built at runtime by `logger_loader.gd`), so 4.3 has zero parse errors with no allowlist. |
There was a problem hiding this comment.
Fixed in af1ef8e — rewrote the GDScript-conventions extends-Logger bullet to describe the .gdignore + logger_loader runtime-build (no parse error on any version) and dropped the old runtime/*.gd paths + ALLOW_LOGGER_PARSE_ERRORS.
Two doc-consistency findings: - test_game_logger_no_class_name_lookup.py: the module docstring example still showed the old res://addons/godot_ai/runtime/game_logger.gd path in a sample error trace. Point it at runtime/loggers/. - skill.md: the GDScript-conventions extends-Logger bullet still claimed the scripts emit a benign parse error during the filesystem scan and named the old runtime/*.gd paths. Rewrote it to describe the .gdignore + logger_loader runtime-build mechanism (no parse error on any version) and the must-not-set-resource_path constraint. Docs/comments only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
runtime/editor_logger.gdandruntime/game_logger.gdextends Logger(a Godot 4.5+ class). Godot's editor filesystem scan parses every.gdunder the project, so on 4.3/4.4 each file printed aParse Error: Could not find base class "Logger"(+ a "Failed to load script" follow-up) on every editor startup. Functionally harmless (the scripts are only instanced behind aClassDB.class_exists("Logger")gate), but real red error text we shouldn't ship to 4.3 users.Fix: move both files into
runtime/loggers/, which carries a.gdignoreso Godot's editor scan skips the folder entirely — no parse, no error, on any engine. A new plain-RefCountedruntime/logger_loader.gd(parses on every version) compiles them from on-disk source viaFileAccess+GDScript.new()at runtime (FileAccessis unaffected by.gdignore, which only governs the resource importer).plugin.gdandgame_helper.gdcallLoggerLoader.build(...)past their existingClassDB.class_exists("Logger")gate, so theextends Loggerparse only ever happens on 4.5+ where it resolves.Also drops the now-moot
ALLOW_LOGGER_PARSE_ERRORSfilter (added in #478) fromci-check-gdscript+ci.yml: with the errors gone, the GDScript check is strict on every engine again — a real Logger error now fails CI on any version, including the 4.3 canary.Verification (both engines)
godot --headless --import+ strictci-check-gdscript→ ZERO parse errors (the two Logger lines are gone). This is the whole point.editor_logger+ 6game_logger) — the runtime source-build loader yields a workingLoggersubclass with functioning virtuals + static helpers; fulleditorsuite 91 pass / 0 fail.test_editor.gdlogger tests switched fromload(<path>)toLoggerLoader.build(<path>)(a.gdignore'd path can't beload()-ed).Notes
.gdignore'd files are excluded from export packs, soLoggerLoader.build()returns null there (file absent) and log capture is simply unavailable — which is correct, since there's no editor/MCP attached to an exported game. The gate + null-check handle it gracefully.< 4.4self-update gate). Both should land before v2.5.8 so 4.3 users get a fully clean experience: plugin loads, no error lines, and the Update button routes to a manual-download page instead of crashing.Test plan