Redesign per Copilot review: sticky bypass after watchdog + tests in tree

Two issues from Copilot's review on PR #381 + the second CI failure:

(A) Cross-scan signal race [Copilot]: scan #1 watchdog'd, scan #2 then
    arms a fresh `filesystem_changed` listener. A delayed emission
    from scan #1 fires on whichever listener is currently connected
    to the shared signal — Godot can't tag emissions with their
    source scan — so scan #2's listener falsely settles before its
    actual filesystem scan completed, re-enabling the plugin against
    a potentially incomplete on-disk install. Generation-counter +
    Callable.bind doesn't help because a single emission still fires
    every connected listener (CONNECT_ONE_SHOT only auto-disconnects
    after firing).

(B) Second CI failure: `Timer.start()` requires the Timer to be
    inside a SceneTree. The previous fix removed `add_child(runner)`
    from tests but didn't parent the runner anywhere, so when
    `_arm_scan_watchdog` called `add_child(timer)` and `timer.start()`
    inside the runner, the timer wasn't in any tree.

Fix:

- Add a sticky `_scan_timed_out` flag set by `_on_scan_watchdog_timeout`.
- `_start_filesystem_scan` checks the flag at the top and bypasses the
  connect + `fs.scan()` path entirely, falling straight through to
  `call_deferred(deferred_step)`. With no listener armed, a delayed
  emission from the timed-out scan has nothing to satisfy. Godot's
  normal background scan catches up after the plugin re-enables.
- Tests now parent the runner via `(Engine.get_main_loop() as
  SceneTree).root.add_child(runner)` so `Timer.start()` works.
  Cleanup via a `_free_runner` helper (remove_child + free).
- New regression test `test_subsequent_scan_after_watchdog_bypasses_
  listener_arm` explicitly drives scan #1 → watchdog → scan #2 and
  asserts scan #2 does NOT set `_waiting_for_scan = true` (i.e. no
  listener armed for a delayed scan-#1 emission to satisfy).
- Existing watchdog test `test_watchdog_no_op_when_signal_already_settled`
  also asserts `_scan_timed_out` stays false on the late-Timer-after-
  successful-signal path — guards against the watchdog poisoning
  subsequent scans when no actual deadlock happened.

Author: claude

plugin/addons/godot_ai/update_reload_runner.gd

@@ -45,6 +45,13 @@ var _scan_next_step := ""
 ## defensive pattern for state that survives `fs.scan()` during update.
 const SCAN_WATCHDOG_SECS := 30.0
 var _scan_watchdog_timer = null
+## Sticky flag set by `_on_scan_watchdog_timeout`. Subsequent
+## `_start_filesystem_scan` calls in the same update bypass connect+scan
+## so a delayed `filesystem_changed` emission from the timed-out scan
+## can't fire on a freshly-armed listener for the next scan and falsely
+## settle it before that scan actually completed. See PR #381 review for
+## the cross-scan race this guards against.
+var _scan_timed_out := false
 ## Keep Array fields untyped: this runner survives fs.scan() during update,
 ## and typed Variant storage is part of the hot-reload crash class.
 var _new_file_paths = []
@@ -131,6 +138,21 @@ func _start_filesystem_scan(next_step: String = "_enable_new_plugin") -> void:
 		call_deferred(deferred_step)
 		return
 
+	## Bypass: a previous scan in this update already watchdog'd, so the
+	## editor's filesystem is unresponsive. Re-arming a `filesystem_changed`
+	## listener now would race with a delayed emission from the timed-out
+	## scan: that single emission would fire whichever listener is currently
+	## connected to the shared signal, falsely settling this scan before it
+	## actually completed. Skip the wait; Godot's normal background scan
+	## catches up after the plugin re-enables. See PR #381 review.
+	if _scan_timed_out:
+		push_warning(
+			"MCP | skipping filesystem_changed wait after previous timeout (next_step=%s)"
+			% deferred_step
+		)
+		call_deferred(deferred_step)
+		return
+
 	_waiting_for_scan = true
 	_scan_next_step = deferred_step
 	if not fs.filesystem_changed.is_connected(_on_filesystem_changed):
@@ -156,15 +178,22 @@ func _stop_scan_watchdog() -> void:
 func _on_scan_watchdog_timeout() -> void:
 	## Signal didn't fire within SCAN_WATCHDOG_SECS — most likely the
 	## filesystem scan is blocked behind a slow disk / NFS / AV scanner
-	## still reading the just-extracted addon files. Drop the listener so
-	## a delayed signal can't double-call `_finish_scan_wait`, then proceed.
-	## `_finish_scan_wait` is idempotent on `_waiting_for_scan == false`.
+	## still reading the just-extracted addon files.
+	## Set the sticky `_scan_timed_out` flag so any subsequent
+	## `_start_filesystem_scan` in this update skips its connect+scan
+	## (otherwise a delayed emission from this scan would falsely settle
+	## the next scan's listener — see PR #381 review).
+	## Disconnect the current listener too, so this scan's listener can't
+	## double-call `_finish_scan_wait` if the signal arrives quickly after
+	## the timeout fires. `_finish_scan_wait` is idempotent on
+	## `_waiting_for_scan == false`.
 	if not _waiting_for_scan:
 		return
 	push_warning(
 		"MCP | filesystem_changed didn't fire within %ds; proceeding without scan confirmation"
 		% int(SCAN_WATCHDOG_SECS)
 	)
+	_scan_timed_out = true
 	var fs := EditorInterface.get_resource_filesystem()
 	if fs != null and fs.filesystem_changed.is_connected(_on_filesystem_changed):
 		fs.filesystem_changed.disconnect(_on_filesystem_changed)

test_project/tests/test_update_reload_runner.gd

@@ -483,6 +483,29 @@ func test_install_zip_paths_rolls_back_when_mid_loop_write_fails() -> void:
 # ----- _arm_scan_watchdog / _on_scan_watchdog_timeout (audit-v2 #9) -----
 
 
+func _scene_root() -> Node:
+	# `Timer.start()` requires the timer (and thus its parent) to be
+	# inside a SceneTree. `McpTestSuite` extends `RefCounted` so it can't
+	# act as a parent itself — parent the runner under the editor's
+	# SceneTree root for the duration of the test, then remove + free.
+	var tree := Engine.get_main_loop() as SceneTree
+	return tree.root if tree != null else null
+
+
+func _new_runner_in_tree():
+	var runner = _new_runner()
+	var root := _scene_root()
+	assert_true(root != null, "test setup: SceneTree root must exist")
+	root.add_child(runner)
+	return runner
+
+
+func _free_runner(runner) -> void:
+	if runner.get_parent() != null:
+		runner.get_parent().remove_child(runner)
+	runner.free()
+
+
 func _arm_scan_state(runner) -> void:
 	# Mirror what `_start_filesystem_scan` would do, minus the actual
 	# `EditorInterface.get_resource_filesystem().scan()` call. We can't
@@ -498,11 +521,7 @@ func test_watchdog_timeout_proceeds_when_signal_never_fires() -> void:
 	## `_waiting_for_scan = true` forever. The watchdog must clear that
 	## flag and dispatch `_scan_next_step` so the rest of the update
 	## sequence can finish.
-	## Test fires `_on_scan_watchdog_timeout` directly — the runner does
-	## not need to be in a SceneTree because we don't depend on the Timer
-	## actually counting down. (`McpTestSuite` extends `RefCounted`, so
-	## `add_child(runner)` is not available here.)
-	var runner = _new_runner()
+	var runner = _new_runner_in_tree()
 	_arm_scan_state(runner)
 	assert_true(runner._waiting_for_scan, "scan wait armed by precondition")
 	assert_true(runner._scan_watchdog_timer != null, "watchdog timer node exists once armed")
@@ -511,33 +530,39 @@ func test_watchdog_timeout_proceeds_when_signal_never_fires() -> void:
 
 	assert_false(runner._waiting_for_scan, "watchdog cleared the wait flag")
 	assert_eq(runner._scan_next_step, "", "next-step token consumed by _finish_scan_wait")
-	runner.free()
+	assert_true(runner._scan_timed_out, "watchdog must set the sticky bypass flag")
+	_free_runner(runner)
 
 
 func test_watchdog_no_op_when_signal_already_settled() -> void:
 	## Race: signal fires, `_finish_scan_wait` cleans up, then the watchdog
 	## Timer fires anyway (Godot's Timer can have queued timeout). Calling
 	## `_on_scan_watchdog_timeout` after a settled scan must be a no-op —
-	## otherwise it would double-dispatch `_scan_next_step`.
-	var runner = _new_runner()
+	## otherwise it would double-dispatch `_scan_next_step` AND it must
+	## NOT poison subsequent scans by setting `_scan_timed_out = true`.
+	var runner = _new_runner_in_tree()
 	_arm_scan_state(runner)
 
 	# Simulate the happy path: signal arrived, _finish_scan_wait ran.
 	runner._finish_scan_wait()
 	assert_false(runner._waiting_for_scan, "wait already cleared by signal handler")
 
-	# Now the late watchdog timeout fires. It must not crash, must not
-	# re-dispatch, must not flip _waiting_for_scan back to true.
+	# Now the late watchdog timeout fires. Must not flip _waiting_for_scan
+	# back to true, must not set _scan_timed_out (the scan succeeded).
 	runner._on_scan_watchdog_timeout()
 	assert_false(runner._waiting_for_scan, "watchdog stays no-op after settled wait")
-	runner.free()
+	assert_false(
+		runner._scan_timed_out,
+		"watchdog no-op must not poison _scan_timed_out — the scan actually succeeded",
+	)
+	_free_runner(runner)
 
 
 func test_finish_scan_wait_stops_armed_watchdog() -> void:
 	## Happy path: filesystem_changed signal arrives; `_finish_scan_wait`
 	## must stop the still-running Timer so it doesn't fire later and
 	## attempt a second cleanup. Verify by inspecting the Timer state.
-	var runner = _new_runner()
+	var runner = _new_runner_in_tree()
 	_arm_scan_state(runner)
 	assert_false(runner._scan_watchdog_timer.is_stopped(), "timer running after arm")
 
@@ -547,15 +572,15 @@ func test_finish_scan_wait_stops_armed_watchdog() -> void:
 		runner._scan_watchdog_timer.is_stopped(),
 		"finish_scan_wait must stop the watchdog so it can't fire later",
 	)
-	runner.free()
+	_free_runner(runner)
 
 
 func test_watchdog_timer_reused_across_arms() -> void:
 	## `_arm_scan_watchdog` lazy-creates the Timer on first use and reuses
 	## it on subsequent arms. The runner makes two filesystem scans during
 	## a single update (new files, then existing files), so the second arm
 	## must not leak a second Timer child.
-	var runner = _new_runner()
+	var runner = _new_runner_in_tree()
 	_arm_scan_state(runner)
 	var first_timer = runner._scan_watchdog_timer
 	runner._finish_scan_wait()
@@ -565,4 +590,44 @@ func test_watchdog_timer_reused_across_arms() -> void:
 
 	assert_true(first_timer == second_timer, "timer reused, not recreated")
 	runner._finish_scan_wait()
-	runner.free()
+	_free_runner(runner)
+
+
+func test_subsequent_scan_after_watchdog_bypasses_listener_arm() -> void:
+	## Cross-scan race regression (PR #381 review): if scan #1 watchdog'd,
+	## scan #2 must NOT arm a fresh `filesystem_changed` listener.
+	##
+	## Why: a delayed `filesystem_changed` emission from scan #1 fires on
+	## any listener currently connected to the shared signal — Godot can't
+	## tag emissions with their source scan. So if scan #2 has armed a
+	## fresh listener by the time scan #1's emission finally arrives, that
+	## listener fires and falsely settles scan #2 before its actual
+	## filesystem scan completed — re-enabling the plugin against a
+	## potentially-incomplete on-disk install.
+	##
+	## Fix: the watchdog sets the sticky `_scan_timed_out` flag, and
+	## `_start_filesystem_scan` checks it at the top and bypasses the
+	## connect+scan path entirely. The pending plugin re-enable still
+	## happens via `call_deferred(next_step)` — Godot's normal background
+	## scan catches up after the plugin re-enables.
+	var runner = _new_runner_in_tree()
+
+	# Scan #1: arm + watchdog timeout.
+	_arm_scan_state(runner)
+	runner._on_scan_watchdog_timeout()
+	assert_true(runner._scan_timed_out, "watchdog set the sticky flag")
+	assert_false(runner._waiting_for_scan, "watchdog cleared the wait")
+
+	# Scan #2 attempts to start. Pre-fix, this re-armed the listener. Now,
+	# the bypass path must short-circuit before _waiting_for_scan flips to
+	# true. We pass a benign deferred step (`set_process`) so the
+	# `call_deferred` in the bypass branch doesn't re-enable the plugin
+	# in the test environment.
+	runner._start_filesystem_scan("set_process")
+	assert_false(
+		runner._waiting_for_scan,
+		"post-watchdog _start_filesystem_scan must NOT arm a new listener — "
+		+ "no listener means no false-settle from a delayed scan-#1 emission",
+	)
+
+	_free_runner(runner)