Skip to content

Commit a95a272

Browse files
dsarnoclaude
andauthored
[Audit v2 #354] Surface FAILED_MIXED self-update state in editor_state + dock (#382)
* [Audit v2 #354] Surface FAILED_MIXED self-update state in editor_state + dock Closes audit-v2 finding #10 (#354): the runner correctly refuses to re-enable the plugin when its rollback can't restore the previous addons/godot_ai/ contents (`InstallStatus.FAILED_MIXED`), but the half-installed state was previously invisible to both agents and the dock — users saw "plugin won't start," re-ran the update, and compounded the mismatch. Adds a structured "addon dir is in MIXED state, here are the .update_backup files" diagnostic surfaced in two places: - `editor_state` MCP tool: `mixed_state` field (omitted when the addons tree is clean) carrying `addon_dir`, `backup_files`, `backup_count`, `truncated`, `message`. An MCP agent can read and report this. - Dock: a red/amber banner above the update banner listing the leftover *.update_backup paths plus a Re-scan button that re-checks the addon dir after a manual restore. Both surfaces consume the same scanner (`utils/update_mixed_state.gd`), which walks `res://addons/godot_ai/` for `*.update_backup` files, caps the result list at 200 entries (with `truncated: true` so the agent knows when output is windowed), and returns an empty Dictionary when the tree is clean. The scanner doesn't depend on a separate marker file — `.update_backup` files on disk ARE the evidence, which makes the surface resilient to runner crashes that happen before any marker write. Tests: - `tests/unit/test_editor_state_mixed_state.py`: pins the Python passthrough (clean omits the field; non-clean surfaces every key; truncated flag survives). - `test_project/tests/test_update_mixed_state.gd`: pins the scanner contract (clean dir, top-level + nested matches, sorted output, non-backup files ignored, missing dir doesn't crash, MAX cap and `truncated` flag). * Address Copilot + simplify review on PR #382 Hot path: `editor_state` is one of the always-loaded core MCP tools and agents poll it constantly. The recursive `DirAccess` walk added in the previous commit ran on every poll. Add a 5-second TTL cache keyed on the production `ADDON_DIR`; tests passing scratch dirs still see fresh scans (cache only tracks the production path), and the dock's Re-scan button passes `force=true` so a manual fix is reflected immediately. Walk determinism: when truncation kicks in, two scans of the same mixed tree could return different 200-file slices because Godot's `list_dir` order isn't guaranteed stable. Sort entries within each directory and push subdirs reverse-sorted so DFS pops them in ascending order. Result is deterministic across calls. Message accuracy: the scanner can't distinguish FAILED_MIXED from "successful install whose `_finalize_install_success` cleanup hit a permission error" — both leave `.update_backup` files behind. Soften the operator copy to acknowledge both cases; the recovery action (delete the files) is the same. Dock test coverage: extract `_apply_mixed_state_banner_diagnostic` as a render seam so `test_dock.gd` can exercise the banner against synthetic diagnostics without polluting `addons/godot_ai/` with backup files. Adds four new dock tests pinning visibility, re-rendering, dismissal, and truncation hint. Reuse: alias `BACKUP_SUFFIX` from `UpdateReloadRunner.INSTALL_BACKUP_SUFFIX` so the producer and the scanner can never drift on the suffix value. Cleanup: drop the redundant `DirAccess.dir_exists_absolute` pre-check (let `DirAccess.open(...)` returning null be the single failure path), drop unnecessary `String(...)` coercions on values that are already typed strings, and add `clear_cache()` for tests that flip state between calls. * Fix Godot CI: RichTextLabel.text empty after add_text() The previous commit's `_mixed_state_label` was a `RichTextLabel`. Its `.text` property reflects the BBCode source, not the rendered content appended via `add_text(...)` — so the dock test reading `_mixed_state_label.text` got `""` even though the banner was rendering correctly. CI's three Godot test jobs (Linux/macOS/Windows) failed on `test_mixed_state_banner_renders_synthetic_diagnostic` and `test_mixed_state_banner_renders_truncated_hint`. Switch the message label to a plain `Label` (matches the existing `_drift_label` pattern — one-shot single-line message, no scroll, no selection beyond the file list). The file-list widget stays a `RichTextLabel` because it needs the scroll region for long lists; the test reads it via `get_parsed_text()` which returns the rendered string content (not the BBCode source). The previous commit's `String(...)` cast on the assignment is preserved as a defensive type pin against `Dictionary.get()` returning Variant — no functional change there, just kept as documented. --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 21307b1 commit a95a272

8 files changed

Lines changed: 724 additions & 12 deletions

File tree

plugin/addons/godot_ai/handlers/editor_handler.gd

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ extends RefCounted
33

44
## Handles editor state, selection, log, screenshot, and performance commands.
55

6+
const UpdateMixedState := preload("res://addons/godot_ai/utils/update_mixed_state.gd")
7+
68
var _log_buffer: McpLogBuffer
79
var _connection: McpConnection
810
var _debugger_plugin: McpDebuggerPlugin
@@ -20,19 +22,26 @@ func _init(log_buffer: McpLogBuffer, connection: McpConnection = null, debugger_
2022

2123
func get_editor_state(_params: Dictionary) -> Dictionary:
2224
var scene_root := EditorInterface.get_edited_scene_root()
23-
return {
24-
"data": {
25-
"godot_version": Engine.get_version_info().get("string", "unknown"),
26-
"project_name": ProjectSettings.get_setting("application/config/name", ""),
27-
"current_scene": scene_root.scene_file_path if scene_root else "",
28-
"is_playing": EditorInterface.is_playing_scene(),
29-
"readiness": McpConnection.get_readiness(),
30-
## True once the game subprocess autoload has beaconed mcp:hello;
31-
## false between Play→Stop cycles. Lets capture-source=game callers
32-
## poll for a real ready signal instead of guessing with sleep().
33-
"game_capture_ready": _debugger_plugin != null and _debugger_plugin.is_game_capture_ready(),
34-
}
25+
var data := {
26+
"godot_version": Engine.get_version_info().get("string", "unknown"),
27+
"project_name": ProjectSettings.get_setting("application/config/name", ""),
28+
"current_scene": scene_root.scene_file_path if scene_root else "",
29+
"is_playing": EditorInterface.is_playing_scene(),
30+
"readiness": McpConnection.get_readiness(),
31+
## True once the game subprocess autoload has beaconed mcp:hello;
32+
## false between Play→Stop cycles. Lets capture-source=game callers
33+
## poll for a real ready signal instead of guessing with sleep().
34+
"game_capture_ready": _debugger_plugin != null and _debugger_plugin.is_game_capture_ready(),
3535
}
36+
## Half-installed addon tree from a failed self-update rollback. When
37+
## non-empty, the agent / dock paint the operator-facing recovery copy
38+
## from `update_mixed_state.gd::diagnose`. Field omitted when the
39+
## addons tree is clean so editor_state's normal payload stays small.
40+
## See issue #354 / audit-v2 #10.
41+
var mixed_state := UpdateMixedState.diagnose()
42+
if not mixed_state.is_empty():
43+
data["mixed_state"] = mixed_state
44+
return {"data": data}
3645

3746

3847
func get_selection(_params: Dictionary) -> Dictionary:

plugin/addons/godot_ai/mcp_dock.gd

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ extends VBoxContainer
77
const ServerStateScript := preload("res://addons/godot_ai/utils/mcp_server_state.gd")
88
const ClientRefreshStateScript := preload("res://addons/godot_ai/utils/mcp_client_refresh_state.gd")
99
const UpdateManagerScript := preload("res://addons/godot_ai/utils/update_manager.gd")
10+
const UpdateMixedStateScript := preload("res://addons/godot_ai/utils/update_mixed_state.gd")
1011
const Client := preload("res://addons/godot_ai/clients/_base.gd")
1112
const ClientConfigurator := preload("res://addons/godot_ai/client_configurator.gd")
1213
const ClientRegistry := preload("res://addons/godot_ai/clients/_registry.gd")
@@ -190,6 +191,16 @@ var _update_banner: VBoxContainer
190191
var _update_label: Label
191192
var _update_btn: Button
192193

194+
# Mixed-state banner — surfaces when `addons/godot_ai/` contains
195+
# `*.update_backup` files left by a self-update whose rollback failed
196+
# (`UpdateReloadRunner.InstallStatus.FAILED_MIXED`). Without this banner
197+
# the user sees "plugin won't start" with no actionable context, re-runs
198+
# the update, and compounds the mismatch (issue #354 / audit-v2 #10).
199+
var _mixed_state_banner: VBoxContainer
200+
var _mixed_state_label: Label
201+
var _mixed_state_files: RichTextLabel
202+
var _mixed_state_rescan_btn: Button
203+
193204

194205
func setup(connection: McpConnection, log_buffer: McpLogBuffer, plugin: EditorPlugin) -> void:
195206
_connection = connection
@@ -429,6 +440,9 @@ func _build_ui() -> void:
429440
_crash_panel.add_child(HSeparator.new())
430441
add_child(_crash_panel)
431442

443+
_build_mixed_state_banner()
444+
_refresh_mixed_state_banner()
445+
432446
# --- Update banner (top of dock, hidden until check finds a newer version) ---
433447
_update_banner = VBoxContainer.new()
434448
_update_banner.add_theme_constant_override("separation", 4)
@@ -901,6 +915,79 @@ static func _crash_body_for_state(state: int, server_status: Dictionary = {}) ->
901915
return ""
902916

903917

918+
## Build the mixed-state banner. Hidden until `_refresh_mixed_state_banner`
919+
## confirms `*.update_backup` files exist in the addons tree. Mirrors the
920+
## issue #354 fix shape: structured, agent-readable diagnostic that survives
921+
## a normal editor restart so the user can act on it instead of re-running
922+
## the update.
923+
func _build_mixed_state_banner() -> void:
924+
_mixed_state_banner = VBoxContainer.new()
925+
_mixed_state_banner.add_theme_constant_override("separation", 4)
926+
_mixed_state_banner.visible = false
927+
928+
_mixed_state_label = Label.new()
929+
_mixed_state_label.autowrap_mode = TextServer.AUTOWRAP_WORD_SMART
930+
_mixed_state_label.size_flags_horizontal = Control.SIZE_EXPAND_FILL
931+
_mixed_state_label.add_theme_color_override("font_color", Color.RED)
932+
_mixed_state_banner.add_child(_mixed_state_label)
933+
934+
_mixed_state_files = RichTextLabel.new()
935+
_mixed_state_files.bbcode_enabled = false
936+
_mixed_state_files.fit_content = true
937+
_mixed_state_files.autowrap_mode = TextServer.AUTOWRAP_OFF
938+
_mixed_state_files.selection_enabled = true
939+
_mixed_state_files.scroll_active = true
940+
_mixed_state_files.custom_minimum_size = Vector2(0, 90)
941+
_mixed_state_files.add_theme_color_override("default_color", COLOR_AMBER)
942+
_mixed_state_banner.add_child(_mixed_state_files)
943+
944+
_mixed_state_rescan_btn = Button.new()
945+
_mixed_state_rescan_btn.text = "Re-scan"
946+
_mixed_state_rescan_btn.tooltip_text = (
947+
"Scan addons/godot_ai/ for *.update_backup files again."
948+
+ " Click after restoring the addon manually to dismiss this banner."
949+
)
950+
_mixed_state_rescan_btn.pressed.connect(func(): _refresh_mixed_state_banner(true))
951+
_mixed_state_banner.add_child(_mixed_state_rescan_btn)
952+
953+
_mixed_state_banner.add_child(HSeparator.new())
954+
add_child(_mixed_state_banner)
955+
956+
957+
func _refresh_mixed_state_banner(force: bool = false) -> void:
958+
## Re-scan button passes `force=true` to bypass the scanner's TTL
959+
## cache so a manual fix is reflected immediately.
960+
_apply_mixed_state_banner_diagnostic(UpdateMixedStateScript.diagnose(
961+
UpdateMixedStateScript.ADDON_DIR, force
962+
))
963+
964+
965+
## Render seam exposed for testing — the GDScript test suite drives this
966+
## directly with synthetic diagnostics so dock banner contracts can be
967+
## pinned without polluting the real `addons/godot_ai/` tree with backup
968+
## files. Callers from production go through `_refresh_mixed_state_banner`.
969+
func _apply_mixed_state_banner_diagnostic(diag: Dictionary) -> void:
970+
if _mixed_state_banner == null:
971+
return
972+
if diag.is_empty():
973+
_mixed_state_banner.visible = false
974+
return
975+
_mixed_state_banner.visible = true
976+
## `Dictionary.get(...)` returns Variant; Label.text is typed String.
977+
## Explicit cast keeps the type contract honest and dodges some Godot
978+
## 4.x point-release quirks around Variant→typed-property assignment.
979+
_mixed_state_label.text = String(diag.get("message", ""))
980+
_mixed_state_files.clear()
981+
for path in diag.get("backup_files", []):
982+
_mixed_state_files.add_text(String(path))
983+
_mixed_state_files.newline()
984+
if bool(diag.get("truncated", false)):
985+
_mixed_state_files.add_text(
986+
"… (list truncated at %d entries)" % UpdateMixedStateScript.MAX_BACKUP_RESULTS
987+
)
988+
_mixed_state_files.newline()
989+
990+
904991
func _build_port_picker_section() -> void:
905992
_port_picker_section = VBoxContainer.new()
906993
_port_picker_section.add_theme_constant_override("separation", 4)
Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
@tool
2+
extends RefCounted
3+
4+
## Scanner that detects whether `addons/godot_ai/` is in a half-installed
5+
## state left behind by a self-update whose rollback couldn't restore the
6+
## previous addon contents (`UpdateReloadRunner.InstallStatus.FAILED_MIXED`).
7+
##
8+
## Without this surface the user sees "plugin won't start" with no actionable
9+
## context, re-runs the update, and compounds the mismatch (issue #354 /
10+
## audit-v2 #10). The dock paints a banner from `diagnose()` and
11+
## `editor_handler.gd::get_editor_state` includes the same Dictionary so an
12+
## MCP agent can see and report the state.
13+
14+
const UpdateReloadRunner := preload("res://addons/godot_ai/update_reload_runner.gd")
15+
16+
const ADDON_DIR := "res://addons/godot_ai/"
17+
## Single source of truth for the suffix lives on the producer
18+
## (`UpdateReloadRunner._install_zip_file`); aliasing here so the scanner
19+
## can never drift from what the runner actually writes.
20+
const BACKUP_SUFFIX := UpdateReloadRunner.INSTALL_BACKUP_SUFFIX
21+
## Cap so a runaway addons tree (someone parented the wrong dir, an old
22+
## crashed install left thousands of artifacts) can't blow the
23+
## `editor_state` payload size or freeze the editor on first paint.
24+
const MAX_BACKUP_RESULTS := 200
25+
## TTL for the `diagnose()` cache. `editor_state` is one of the highest-
26+
## traffic MCP tools (agents poll it constantly) and a recursive
27+
## `DirAccess` walk on every call would put I/O on the 4ms `_process()`
28+
## budget. Mixed-state is rare and persistent across editor restarts, so
29+
## a few seconds of staleness is acceptable; the dock's Re-scan button
30+
## bypasses the cache via `force=true` for immediate feedback.
31+
const CACHE_TTL_MSEC := 5000
32+
33+
static var _cache_value: Dictionary = {}
34+
static var _cache_timestamp_msec: int = -1
35+
36+
37+
## Walk `dir` recursively and return every `res://`-relative path that ends
38+
## in `.update_backup`, sorted ascending. Truncates at `MAX_BACKUP_RESULTS`
39+
## — the truncation flag is exposed via `diagnose()`.
40+
##
41+
## Walk order is deterministic: entries within each directory are sorted
42+
## alphabetically, subdirs pushed reverse-sorted so DFS pops them in
43+
## ascending order. Without this two scans of the same mixed tree could
44+
## return different 200-file slices when truncation kicks in (Godot's
45+
## `list_dir` order isn't guaranteed stable across filesystems).
46+
static func find_backups(dir: String = ADDON_DIR) -> Array:
47+
var results: Array = []
48+
var stack: Array = [dir]
49+
while not stack.is_empty():
50+
if results.size() >= MAX_BACKUP_RESULTS:
51+
break
52+
var current: String = stack.pop_back()
53+
var d := DirAccess.open(current)
54+
## Missing dir, permission error, or unreadable junction — skip
55+
## silently. A missing addons dir is the bare-clone case; mid-walk
56+
## errors stay quiet so a single permission glitch can't block the
57+
## diagnostic the rest of the scan would have produced.
58+
if d == null:
59+
continue
60+
var entries: Array = []
61+
d.list_dir_begin()
62+
while true:
63+
var entry := d.get_next()
64+
if entry.is_empty():
65+
break
66+
if entry == "." or entry == "..":
67+
continue
68+
entries.append({"name": entry, "is_dir": d.current_is_dir()})
69+
d.list_dir_end()
70+
entries.sort_custom(func(a, b): return a["name"] < b["name"])
71+
## Push subdirs reverse-sorted so the next outer iteration pops
72+
## them in ascending order — see method docstring for why this
73+
## determinism matters for the truncated case.
74+
for i in range(entries.size() - 1, -1, -1):
75+
var entry: Dictionary = entries[i]
76+
if entry["is_dir"]:
77+
stack.append(current.path_join(entry["name"]))
78+
for entry in entries:
79+
if entry["is_dir"]:
80+
continue
81+
if not String(entry["name"]).ends_with(BACKUP_SUFFIX):
82+
continue
83+
results.append(current.path_join(entry["name"]))
84+
if results.size() >= MAX_BACKUP_RESULTS:
85+
break
86+
results.sort()
87+
return results
88+
89+
90+
## Build the structured diagnostic Dictionary surfaced via `editor_state`
91+
## and the dock banner. Empty when the addons tree is clean — callers
92+
## gate banner visibility / response field on `is_empty()`.
93+
##
94+
## Cached for `CACHE_TTL_MSEC` when scanning the default `ADDON_DIR` so
95+
## per-`editor_state` polls don't re-walk the addons tree every frame.
96+
## Tests passing a custom `dir` always see a fresh scan (cache only
97+
## tracks the production path). `force=true` bypasses the cache — used
98+
## by the dock's Re-scan button so a manual fix is reflected immediately.
99+
static func diagnose(dir: String = ADDON_DIR, force: bool = false) -> Dictionary:
100+
var use_cache := dir == ADDON_DIR and not force
101+
if use_cache and _cache_timestamp_msec >= 0:
102+
if Time.get_ticks_msec() - _cache_timestamp_msec < CACHE_TTL_MSEC:
103+
return _cache_value.duplicate(true)
104+
105+
var backups := find_backups(dir)
106+
var result: Dictionary = {}
107+
if not backups.is_empty():
108+
## Most commonly produced by `_rollback_paths_written` returning
109+
## FAILED_MIXED, but `_finalize_install_success` removes backups on
110+
## a best-effort basis so a successful install can also leave them
111+
## behind if the cleanup `remove_absolute` hit a permission error.
112+
## The recovery action — delete the *.update_backup files — is the
113+
## same in both cases, so the message acknowledges both
114+
## possibilities rather than asserting the alarming one.
115+
result = {
116+
"addon_dir": dir,
117+
"backup_files": backups,
118+
"backup_count": backups.size(),
119+
"truncated": backups.size() >= MAX_BACKUP_RESULTS,
120+
"message": (
121+
"Found .update_backup files in addons/godot_ai/. This usually"
122+
+ " means a self-update rollback couldn't restore the previous"
123+
+ " addon contents (FAILED_MIXED) — the plugin may load a mix"
124+
+ " of old and new files. Restore the addon from your VCS or a"
125+
+ " fresh release ZIP, then delete the listed *.update_backup"
126+
+ " files. If the plugin runs without issues these are likely"
127+
+ " stale from a successful install and safe to delete."
128+
),
129+
}
130+
if use_cache:
131+
_cache_value = result.duplicate(true)
132+
_cache_timestamp_msec = Time.get_ticks_msec()
133+
return result
134+
135+
136+
## Reset the `diagnose()` cache. Tests that flip the addons-tree state
137+
## between calls use this to avoid TTL-bound flakiness; the dock's
138+
## Re-scan button uses `force=true` instead.
139+
static func clear_cache() -> void:
140+
_cache_value = {}
141+
_cache_timestamp_msec = -1
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
uid://dd5rti52vgs71

0 commit comments

Comments
 (0)