Skip to content

Add Restart Server button to dev-mode Setup section#406

Merged
dsarno merged 5 commits into
betafrom
dlight/bold-shannon-846b3e
May 7, 2026
Merged

Add Restart Server button to dev-mode Setup section#406
dsarno merged 5 commits into
betafrom
dlight/bold-shannon-846b3e

Conversation

@dsarno

@dsarno dsarno commented May 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds a dedicated Restart Server button to the dock's Setup section, visible whenever ClientConfigurator.is_dev_checkout() is true and Developer mode is on. Sits alongside the existing Start/Stop Dev Server button.
  • Click routes through new Plugin.force_restart_server_preserving_mode() which picks the managed (_lifecycle.force_restart_server) vs --reload dev (stop_dev_server + _wait_for_port_free + start_dev_server) restart path based on what's currently running, so contributors keep Python auto-reload across the kill+respawn.
  • While the click is in flight, the button shows Restarting… disabled, mirroring the existing _crash_restart_btn / _version_restart_btn feedback pattern. Tooltip explains "Killing the current server and respawning…".
  • Disabled with an explanatory tooltip when no server is running on port 8000.

Why

Same-version Python edits get adopted as compatible by server_lifecycle.gd::start_server so neither the version-drift _version_restart_btn nor the spawn-failure _crash_restart_btn surfaces — reloading the plugin or relaunching the editor just re-adopts the stale running server. The existing Start/Stop Dev Server button toggles modes ("Switch to dev mode" / "Exit dev mode" / "Start dev server"), which doesn't match the "kill whatever is running and respawn from current source" semantics contributors actually need when editing Python without bumping the version.

Implementation notes

  • _update_dev_section_buttons() does a single has_managed_server / is_dev_server_running discovery per _update_status tick and applies state to both the dev-server toggle and the new Restart button. Avoids the duplicate lsof/ps fork Copilot flagged.
  • force_restart_server_preserving_mode() waits for the port to free between stop_dev_server and start_dev_server — without this, uvicorn's --reload parent reloader can hold the port past OS.kill and the new spawn races the old shutdown.
  • CLAUDE.md: reworded the "worktree-safety" headline to ban only another session's worktree, matching the nuanced exception two paragraphs below ("use your own worktree, commit frequently").

Test plan

  • ruff check src/ tests/ — clean
  • pytest -q — 899 passed
  • godot --headless --import — no GDScript parse errors
  • New unit tests for _restart_server_btn_state (3-combination truth table)
  • New tests in test_dock.gd covering rendering, visibility gating, dispatch, enabled/disabled state, in-flight "Restarting…" state
  • Live click smoke on the worktree editor — log confirms stop_dev_server_wait_for_port_freestart_dev_server with --reload (PID changes, new server picks up source edits without a version bump)
  • CI green on all 12 checks (Python 3.11/3.13, Godot Linux/macOS/Windows, Release smoke ×3, Game capture smoke ×3, close-linked-issues)

🤖 Generated with Claude Code

@codecov

codecov Bot commented May 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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

Adds a dev-checkout-only “Restart Server” control to the dock’s Setup section to let contributors force-respawn the currently-running server (managed vs --reload dev server) without toggling dev mode, plus tests to lock in the new button state/visibility/dispatch behavior.

Changes:

  • Add GodotAiPlugin.force_restart_server_preserving_mode() to restart whichever server is currently holding the HTTP port while preserving managed vs --reload mode.
  • Render and wire a new “Restart Server” button in mcp_dock.gd (dev checkout + Developer mode), with enable/tooltip state derived from managed/dev-running detection.
  • Extend unit tests in test_dock.gd and test_dock_dev_server_btn.gd to cover visibility, dispatch, and the _restart_server_btn_state truth table.

Reviewed changes

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

Show a summary per file
File Description
test_project/tests/test_dock.gd Adds integration-style dock tests for the new Restart Server button (rendering, visibility gating, dispatch, enabled/tooltip behavior).
test_project/tests/test_dock_dev_server_btn.gd Adds unit tests for _restart_server_btn_state truth table.
plugin/addons/godot_ai/plugin.gd Adds force_restart_server_preserving_mode() to restart the active server while preserving managed vs --reload mode.
plugin/addons/godot_ai/mcp_dock.gd Adds the Restart Server button in dev-mode Setup UI plus state/update/pressed handling helpers.
CLAUDE.md Updates developer documentation around worktree/Godot launch safety guidance.

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

Comment on lines +1429 to +1432
if is_dev_server_running():
stop_dev_server()
start_dev_server()
return true

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — already addressed in c0df1b8. The dev branch of force_restart_server_preserving_mode now does stop_dev_server()_wait_for_port_free(ClientConfigurator.http_port(), 5.0)start_dev_server(), mirroring the managed _lifecycle.force_restart_server path.

Kept the wait inline rather than pushing it into stop_dev_server() so existing callers (the "Exit dev mode" path in _on_dev_server_pressed) keep their non-blocking behavior — only the explicit kill+respawn flow needs to block on port release.

Comment thread plugin/addons/godot_ai/mcp_dock.gd Outdated
Comment on lines +841 to +842
_update_dev_server_btn()
_update_dev_restart_btn()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 6ba9ff8. Added _update_dev_section_buttons() which scans (has_managed, dev_running) once and applies state to both buttons. The per-frame _update_status path now calls only the aggregator; same for the click-deferred refreshes in _on_dev_*_pressed.

The per-button _update_dev_server_btn / _update_dev_restart_btn helpers stayed for test isolation (_seed_dev_restart_btn swaps in a mock plugin and exercises just the restart button without the server-toggle button needing to exist).

dsarno and others added 5 commits May 7, 2026 08:15
Same-version Python edits get adopted as compatible (server_lifecycle.gd
adoption arm) so neither the version-drift restart nor the spawn-failure
restart surfaces. Contributors editing Python source had no UI affordance
to force a respawn — reloading the plugin or relaunching the editor just
re-adopted the stale running server.

Adds a dedicated Restart Server button next to Start/Stop Dev Server,
visible whenever is_dev_checkout() is true and Developer mode is on.
Click routes through new force_restart_server_preserving_mode() which
picks managed (existing _lifecycle.force_restart_server()) vs --reload
dev (stop_dev_server() + start_dev_server()) based on what's running,
so the user keeps Python auto-reload across the kill+respawn.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The headline "Always launch Godot from the root repo's test_project/"
contradicted the nuance two paragraphs down ("use your own worktree"
and "multi-editor is fine when you need the PR's live plugin code").
The intent was always "never point Godot at a vanish-prone worktree",
not a blanket ban on worktree launches.

Reworded to match the actual intent so the right exception (this
session's own worktree, with frequent commits) doesn't read as a
violation of the leading rule.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`_dock` is typed as `Node` in this suite, so property chains through
`_dock._dev_restart_btn.disabled` / `.tooltip_text` resolve to Variant
and `:=` can't infer. CI Linux/macOS/Windows Godot tests all failed to
load the suite as a result. Replace `:=` with explicit `: bool` /
`: String` annotations on the four locals the parser flagged plus the
nearby int local for consistency.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three actionable findings from the parallel reuse / quality / efficiency
agents:

1. Real bug — efficiency review caught a missing wait_for_port_free in
   force_restart_server_preserving_mode's dev branch. stop_dev_server()
   issues OS.kill and returns, but the listener can take >500ms to
   release the port; start_dev_server's fixed 500ms timer then races
   the old uvicorn shutdown and the --reload spawn fails to bind.
   Mirrored the managed path's wait via _wait_for_port_free.

2. Trim doc comments restating WHAT per CLAUDE.md "default to no
   comments" — kept only the WHY (the adoption-arm trap). Drops
   ~17 lines across mcp_dock.gd and plugin.gd.

3. Extract _seed_dev_restart_btn / _cleanup_dev_restart_btn helpers
   in test_dock.gd to match the file's existing _seed_server_row /
   _cleanup_server_row pattern. Cuts boilerplate across the three
   new dispatch/state tests.

Also drops a no-op bool() cast on a value already typed bool.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Copilot review on PR #406: _update_dev_server_btn and
_update_dev_restart_btn each called _plugin.has_managed_server() and
_plugin.is_dev_server_running() independently. The latter shells out
via lsof / ps in plugin.gd::is_dev_server_running, so a single
_update_status pass forked two subprocesses where one would do.

Add _update_dev_section_buttons() that scans once and applies to both
buttons. Used from _update_status (per-frame) and the click-deferred
refreshes in _on_dev_server_pressed / _on_dev_restart_pressed. The
per-button helpers stay so tests that swap in mocks via
_seed_dev_restart_btn / _seed_server_row keep their isolated entry
points.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dsarno dsarno force-pushed the dlight/bold-shannon-846b3e branch from 3493e04 to 6ba9ff8 Compare May 7, 2026 15:16
@dsarno dsarno changed the base branch from main to beta May 7, 2026 15:16
@dsarno dsarno merged commit ae3f769 into beta May 7, 2026
22 checks passed
dsarno added a commit that referenced this pull request May 7, 2026
1. Fix Restart/Start label bug. After clicking the primary button, the
   newly-spawned --reload server's LISTEN holder is uvicorn's
   `multiprocessing.spawn` worker — that PID's cmdline doesn't contain
   the godot_ai brand, so `is_dev_server_running()` returned false and
   the label stayed "Start Dev Server" instead of flipping to "Restart
   Dev Server" once the server was up. Walk up the parent chain
   (bounded depth 5) inside `_pid_cmdline_is_godot_ai` so a worker
   whose parent reloader IS branded gets recognised. Adds `_pid_parent`
   helper covering POSIX (`ps -o ppid=`) and Windows (PowerShell CIM).

2. Drop the dock's "Mode override" dropdown. The dropdown advertised
   toggling between the github-repo local plugin and the installed ZIP
   version, but it didn't actually do that — it just nudged
   `is_dev_checkout()` for the update banner, which is more usefully
   driven by the GODOT_AI_MODE env var (still supported, still tested
   in test_clients.gd). Removes ~50 lines of dead UI plumbing and the
   stale "fresh check paints over a clean slate" comment.

3. Delete the orphan live_demo test file from a debugging session that
   never landed cleanly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dsarno added a commit that referenced this pull request May 7, 2026
…parent-walk (#408)

* Show "Restarting…" feedback while dev-mode restart is in flight

Live smoke uncovered a 5s editor freeze (from _wait_for_port_free) with
no acknowledgement of the click — the user wonders if the button did
anything. Mirror the existing _crash_restart_btn / _version_restart_btn
pattern: set _server_restart_in_progress on click, dispatch via
call_deferred + a 0.15s paint window, then clear the flag after the
spawn lands. _update_dev_section_buttons checks the flag and renders
"Restarting…" + disabled while in flight.

Test path is non-tree, so _on_dev_restart_pressed runs synchronously
when is_inside_tree() is false — keeps the existing dispatch test
green without needing await scaffolding.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Consolidate dev-section to one Restart Dev Server + small ✕ stop

Per PR review feedback: two restart-flavored buttons in the same
section felt redundant, and the new Restart Server stayed disabled
when only an adopted external dev server was running because brand
detection misses orphaned multiprocessing.spawn workers.

Replaces both _dev_server_btn (3-state Switch/Exit/Start toggle) and
_dev_restart_btn (kill+respawn preserving mode) with a single primary
button + a small stop affordance:

- _dev_primary_btn — full-width "Restart Dev Server" / "Start Dev
  Server" (label adapts to whether something's running). Always enabled.
  Click → force_restart_or_start_dev_server which kills any godot-ai
  server (managed or dev), waits for the port to free, and lands in
  --reload mode. Same "Restarting…" feedback during the blocking wait.

- _dev_stop_btn — compact "✕" beside the primary. Enabled only when a
  dev server is actually running. Click → stop_dev_server (existing).
  Intentionally never targets the managed server (lifecycle owns that).

Plugin's force_restart_server_preserving_mode is renamed/retargeted as
force_restart_or_start_dev_server. The previous "preserve managed mode"
semantics drop — clicking Restart now always lands in dev mode, since
that's the only mode contributors editing Python source actually want.
The lifecycle's force_restart_server stays for the version-drift /
crash recovery paths that legitimately preserve managed mode.

One verb, one mental model. Adapted tests cover the truth table.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Make force_restart_or_start_dev_server cover orphan workers

Previous version branched on has_managed_server / is_dev_server_running,
both of which miss orphaned multiprocessing.spawn uvicorn workers — when
the parent reloader dies (or the editor is restarted while the worker
keeps holding the port), the worker's cmdline is just "spawn_main"
without godot_ai branding, so neither detection path fires. Result:
clicking Restart skipped the kill and went straight to start_dev_server,
which then failed to bind :8000.

Switch the kill arm's condition to _is_port_in_use(port). The user
clicking Restart is explicit consent to take over the port, so killing
whoever's there (regardless of brand) is the right semantics. Lifecycle
state still resets when has_managed_server() so the next adoption pass
sees a clean slate.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Three follow-ups from live PR #406 review

1. Fix Restart/Start label bug. After clicking the primary button, the
   newly-spawned --reload server's LISTEN holder is uvicorn's
   `multiprocessing.spawn` worker — that PID's cmdline doesn't contain
   the godot_ai brand, so `is_dev_server_running()` returned false and
   the label stayed "Start Dev Server" instead of flipping to "Restart
   Dev Server" once the server was up. Walk up the parent chain
   (bounded depth 5) inside `_pid_cmdline_is_godot_ai` so a worker
   whose parent reloader IS branded gets recognised. Adds `_pid_parent`
   helper covering POSIX (`ps -o ppid=`) and Windows (PowerShell CIM).

2. Drop the dock's "Mode override" dropdown. The dropdown advertised
   toggling between the github-repo local plugin and the installed ZIP
   version, but it didn't actually do that — it just nudged
   `is_dev_checkout()` for the update banner, which is more usefully
   driven by the GODOT_AI_MODE env var (still supported, still tested
   in test_clients.gd). Removes ~50 lines of dead UI plumbing and the
   stale "fresh check paints over a clean slate" comment.

3. Delete the orphan live_demo test file from a debugging session that
   never landed cleanly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

2 participants