Skip to content

Surface editor error context in log reads#520

Merged
dsarno merged 2 commits into
hi-godot:mainfrom
mq1n:log-call-improvements
Jun 11, 2026
Merged

Surface editor error context in log reads#520
dsarno merged 2 commits into
hi-godot:mainfrom
mq1n:log-call-improvements

Conversation

@mq1n

@mq1n mq1n commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Editor and game logs now preserve structured error details, read visible Debugger Errors rows through logs_read, and clear those rows from clear_logs. Compact responses remain the default; callers opt into rich metadata with include_details.

Editor and game logs now preserve structured error details, read visible Debugger Errors rows through logs_read, and clear those rows from clear_logs. Compact responses remain the default; callers opt into rich metadata with include_details.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the logging surface area so logs_read can return richer, structured error context (stack frames / resolved locations / Debugger “Errors” tab metadata) while keeping the default response compact. It also ensures editor/game buffers preserve optional details payloads and that clear_logs clears visible Debugger Errors rows.

Changes:

  • Add include_details plumbing end-to-end (Python tool → handler → plugin) to opt into rich error metadata.
  • Preserve and transport structured details for game/editor log entries (buffers + logger → debugger channel).
  • Read visible Debugger dock “Errors” tab rows into source="editor" logs and clear those rows via clear_logs.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/unit/test_runtime_handlers.py Extends stub runtime + unit coverage for include_details pass-through.
tests/integration/test_mcp_tools.py Integration test asserting Errors-tab style metadata is returned when include_details is set.
test_project/tests/test_editor.gd Adds GDScript tests for details preservation, Debugger Errors-tree ingestion/dedupe, and clearing debugger rows.
src/godot_ai/tools/editor.py Exposes include_details in the public MCP tool API and documents richer editor/game log shapes.
src/godot_ai/handlers/editor.py Passes include_details to the plugin for non-plugin sources.
plugin/addons/godot_ai/utils/log_backtrace.gd Adds structured details (error type, resolved location, full frames) to resolved errors.
plugin/addons/godot_ai/utils/game_log_buffer.gd Extends game log buffer entries to optionally carry deep-copied details.
plugin/addons/godot_ai/utils/editor_log_buffer.gd Extends editor log buffer entries to optionally carry deep-copied details.
plugin/addons/godot_ai/testing/stub_backtrace.gd Enhances stub backtrace to support multi-frame stacks for details tests.
plugin/addons/godot_ai/runtime/loggers/game_logger.gd Ships optional details alongside log batch entries.
plugin/addons/godot_ai/runtime/loggers/editor_logger.gd Persists optional details in the editor log buffer and updates resolved details when overriding resolved location.
plugin/addons/godot_ai/handlers/editor_handler.gd Implements include_details stripping/retention, reads Debugger Errors-tree rows, and clears them on clear_logs.
plugin/addons/godot_ai/debugger/mcp_debugger_plugin.gd Accepts log-batch entries with optional details (array or dict forms) and persists them to the game buffer.
docs/TOOLS.md Documents expanded logs_read(source="editor") scope and include_details=true.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +198 to +205
func _entries_for_response(entries: Array[Dictionary], include_details: bool) -> Array[Dictionary]:
var out: Array[Dictionary] = []
for entry in entries:
var copy: Dictionary = entry.duplicate(true)
if not include_details:
copy.erase("details")
out.append(copy)
return out
Comment on lines +219 to +243
func _read_debugger_error_entries() -> Array[Dictionary]:
if _debugger_plugin == null and _debugger_errors_root == null:
return []
var root: Node = _debugger_errors_root
if root == null:
root = EditorInterface.get_base_control()
if root == null:
return []
var trees: Array[Tree] = []
_collect_debugger_error_trees(root, trees)
var entries: Array[Dictionary] = []
for tree in trees:
for entry in _entries_from_debugger_error_tree(tree):
if not _has_equivalent_log_entry(entries, entry):
entries.append(entry)
return entries


static func _collect_debugger_error_trees(node: Node, out: Array[Tree]) -> void:
if node is Tree and _tree_has_debugger_errors(node as Tree):
out.append(node as Tree)
for child in node.get_children():
if child is Node:
_collect_debugger_error_trees(child as Node, out)

@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@dsarno dsarno left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eval summary

Solid, well-structured work — the reverse-engineering of Godot's debugger internals is faithful (I checked script_editor_debugger.cpp on both 4.6 and 4.3: error items really do carry _is_warning/_is_error meta and metadata(0) = [source_file, source_line] on both, so the data model holds across the supported 4.3+ range). All 15 CI checks are green and the Python suite passes locally (1142 passed). A few things to address before merge:

Blocking

  • clear_logs desyncs the Debugger Errors panel — calling tree.clear() directly skips the engine's counter/tab/signal reset, leaving the "Errors (N)" badge stale and the toolbar buttons enabled against an empty tree. It also turns a log-buffer maintenance call into a destructive wipe of the user's visible Errors panel. Details inline. Consider opt-in gating.
  • No live-editor smoke. AGENTS.md requires a live smoke for editor-UI features, and the two UI-fidelity issues here (the clear desync, and the stack-frame nesting below) are exactly the class that synthetic-tree GDScript tests can't catch.

Should fix

  • details.frames is tested against a structure that doesn't match the live tree — the fixture flattens frames as direct children, but real Godot nests them under a single <Stack Trace> header. The compact path is fine; the opt-in details.frames likely collapses to one entry live. Inline.

Copilot's two perf comments are both valid and worth acting on:

  • _entries_for_response deep-duplicates every entry even on the default include_details=false path, where the only mutation is erase("details"). Shallow-copy there; reserve the deep copy for the details branch.
  • _read_debugger_error_entries walks the entire get_base_control() subtree on every source="editor"/"all" poll — and the clear path does the same walk. Since logs_read is a polling/tail tool, caching the located Tree(s) (with free-invalidation) or narrowing the search root is the right mitigation.

Nit: plugin.gd:239 still constructs EditorHandler with 5 args, so the new debugger_errors_root param is test-injection only and production relies on the get_base_control() fallback — which is what makes both the clear path and Copilot's walk hit the full editor tree.


Generated by Claude Code

var cleared := 0
for tree in trees:
cleared += _entries_from_debugger_error_tree(tree).size()
tree.clear()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling tree.clear() directly desyncs the Debugger dock's Errors panel. Godot's own Clear button (ScriptEditorDebugger::_clear_errors_list()) does more than clear the tree:

error_tree->clear();
error_count = 0;
warning_count = 0;
emit_signal("errors_cleared");
update_tabs();           // resets the "Errors (N)" tab badge
// ... also disables the Clear/Expand/Collapse buttons

Clearing only the Tree leaves error_count/warning_count non-zero, so the "Errors (N)" tab badge stays stale, the errors_cleared signal never fires, and the toolbar buttons remain enabled against an empty tree. There's no public API to invoke _clear_errors_list() from the plugin, so reaching into the Tree like this can't fully/safely reset the panel.

Two concerns compound here:

  1. Destructive + surprising: a routine clear_logs (a log-buffer maintenance call) now wipes the user's visible Errors panel as a side effect. Consider gating the Errors-tab clear behind an explicit opt-in param rather than firing on every clear_logs.
  2. UI inconsistency: if it stays, it shouldn't bypass the engine's counter/tab reset — at present the badge and buttons are left out of sync with the now-empty tree.

This needs a live-editor smoke (per AGENTS.md) with real Debugger errors present to observe the badge/button state after a clear.


Generated by Claude Code

warning_frame.set_text(0, "<Stack Trace>")
warning_frame.set_text(1, "res://scripts/player.gd:21 @ _ready()")
warning_frame.set_metadata(0, ["res://scripts/player.gd", 21])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixture models stack frames as flat direct children of the error item (a single <Stack Trace> row). In the real ScriptEditorDebugger error tree, there is one <Stack Trace> header item with the individual frames nested underneath it as grandchildren. Since _details_from_debugger_error_item only walks direct children (item.get_first_child()child.get_next()), live details.frames will likely collapse to a single entry instead of the real per-frame list.

The compact path (level/text/path/line, sourced from the top item's own metadata(0)) is unaffected — this gap is confined to the opt-in details payload. Worth verifying against a real editor and, if confirmed, recursing into the <Stack Trace> subtree so the fixture and the extraction reflect Godot's actual nesting.


Generated by Claude Code

@dsarno

dsarno commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

@mq1n were you still planning on fixing this Pr

…ling

- clear_logs no longer wipes the Debugger Errors panel by default; the
  Errors-tab clear sits behind a new clear_debugger_errors opt-in and
  routes through the panel's own Clear button so the engine resets
  error_count/warning_count, the "Errors (N)" badge, the errors_cleared
  signal, and toolbar button states (a raw Tree.clear() left them stale).
- details.frames now captures every stack row. The real tree (verified
  against script_editor_debugger.cpp on 4.3 and 4.6) lays frames out as
  flat children with only frame 0 labeled "<Stack Trace>", so the old
  per-child label match collapsed multi-frame stacks to one entry.
  Detection is run-based, survives translated labels, and the test
  fixture now mirrors the real layout.
- _entries_for_response shallow-copies on the default compact path and
  reserves the deep copy for include_details=true.
- Debugger tree discovery scans only the cached EditorDebuggerNode
  subtree instead of recursing the whole editor UI on every poll.

Live-smoked against a real editor (Godot 4.6.3): a real push_error row
returned all 5 stack frames; the opt-in clear reset the "Errors (1)"
badge and button states via the engine's own clear path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@dsarno

dsarno commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Pushed the review fixes directly to your branch so we can land this — opt-in gating + engine-path reset for the Errors-tab clear, full multi-frame stack extraction, and the two perf items. Thanks for the contribution; happy to walk through any of it if you resurface.

@dsarno dsarno merged commit 78f9222 into hi-godot:main Jun 11, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants