Suggest a free port inline + docs link for foreign port conflicts#608
Conversation
When a non-godot-ai process holds the HTTP port, the dock lands in terminal INCOMPATIBLE and tells the user to change the port — but leaves them to hunt for a free one and to figure out the per-client reconfigure themselves. Cheap-version improvements from #607: - The foreign-occupant crash body (INCOMPATIBLE && !can_recover_incompatible) now names a concrete free port via ClientConfigurator.suggest_free_port, which already routes through the Windows winnat reservation table so the suggested port won't itself fail with WinError 10013. - A "How to change the port" docs button surfaces in the crash panel for that case only, linking to docs/port-conflicts.md, which carries the per-client reconfigure steps that don't fit inline. No behavior change for PORT_EXCLUDED / FOREIGN_PORT or the recoverable (older godot-ai) case, which keeps Restart Server as its primary action. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WwE7zCBJ6qvjk7wyw3X5aU
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a port-conflict guide, updates the crash panel for unrecoverable incompatible servers with a free-port hint and docs link, and adds dock tests for incompatible-body and button visibility behavior. ChangesForeign port conflict UX
Sequence Diagram(s)sequenceDiagram
participant User
participant McpDock
participant Docs as docs/port-conflicts.md
participant EditorSettings
participant MCPClients
McpDock-->>User: show free-port hint and docs button
User->>Docs: open port-conflict guide
User->>EditorSettings: set godot_ai/http_port
User->>MCPClients: update client URLs/configs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/addons/godot_ai/mcp_dock.gd`:
- Line 46: The PORT_CONFLICT_DOCS constant in mcp_dock.gd is hard-linked to
blob/main, so the docs button can point to the wrong guide for installed
releases. Update the docs-opening logic around PORT_CONFLICT_DOCS and the
related button handler to use a versioned URL derived from the installed plugin
version, or resolve and open the packaged local markdown instead, so the
referenced instructions match the shipped build.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6402f2fa-d991-4254-a07c-681d80420328
📒 Files selected for processing (3)
docs/port-conflicts.mdplugin/addons/godot_ai/mcp_dock.gdtest_project/tests/test_dock.gd
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Addresses review feedback: the crash-panel "How to change the port" button linked to blob/main, so a shipped release would open the tip-of-main guide rather than the one that shipped with that build — drifting away from the build's actual UI as the doc evolves. Resolve the docs URL against the installed plugin version at click time (releases are tagged `v<version>`). The button only exists in builds that ship docs/port-conflicts.md, so the versioned ref always resolves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WwE7zCBJ6qvjk7wyw3X5aU
|
@coderabbitai Good catch on the On the earlier "Linked Issues" warning about Windows reservation-awareness: that's already satisfied — Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
And agreed on the Windows point: if ✏️ Learnings added
|
|
This looks good as the cheap/manual-path fix from #607, especially now that the docs link is pinned to the installed version. One coordination note: the current hint/docs are HTTP-only:
But the existing incompatible-server message tells users to change both HTTP and WS ports, and the server really does need the pair to stay valid. So I think the manual guidance should name both settings, e.g. |
Good catch fixing.... |
Addresses review feedback: the crash-body hint and docs only named godot_ai/http_port, but this branch (INCOMPATIBLE && !can_recover_incompatible) also fires for an incompatible godot-ai server we can't prove we own — which commonly holds both the HTTP and WS ports. Suggesting only a free HTTP port would then leave the new server unable to bind WS, and it contradicted the embedded "change both HTTP and WS ports" message. The hint now suggests a free value for each port (both reservation-aware) and names godot_ai/http_port and godot_ai/ws_port; only the HTTP port reaches client configs, so the wording keeps that distinction. docs/port-conflicts.md and the tests track the change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WwE7zCBJ6qvjk7wyw3X5aU
|
@Clubhouse1661 Good catch — fixed in c10feed. You're right that this branch isn't purely HTTP-only: it also fires for an incompatible godot-ai server we can't get strong proof on, which commonly holds both 8000 and 9500, so suggesting only a free HTTP port would leave the new server unable to bind WS (and it contradicted the embedded "change both HTTP and WS ports" message). The hint now reads e.g.:
Both suggestions go through the reservation-aware Generated by Claude Code |
… (#610) * Add foreign-occupant smoke for non-recoverable INCOMPATIBLE path (#609) The recoverable port-conflict path has live cross-OS coverage via `ci-stale-server-smoke`: it plants a godot-ai server simulator on port 8000, then asserts the plugin kills it and respawns. The mirror-image foreign-occupant path — a genuinely non-godot-ai process holds the port, so the plugin gets no ownership proof and must land in terminal INCOMPATIBLE (can_recover_incompatible == false) and make NO kill attempt — had no end-to-end guard, only mocked GDScript unit tests. Close that gap: - `ci-stale-server-smoke` gains `--mode {stale,foreign}` (default stale, so the existing CI invocation is unchanged). Foreign mode plants a plain listener that 404s on /godot-ai/status, launches Godot headless, and asserts the plugin observably: logs `MCP | proof: (none)`, logs a suggested-free-port diagnosis naming a concrete alternate port, never logs `MCP | strong proof:` or `MCP | killed pids`, and leaves the planted listener's PID alive. Shared harness helpers (find_godot/discover_user_dir/wait_for_log_lines/termination/diagnostics) are reused across both modes. - New `foreign-server-smoke` CI job on the same OS matrix as stale-server-smoke. It's a sibling job rather than a second step so the stale run's respawned port-8000 server can't bleed into the scenario. - `server_lifecycle.gd` now logs the suggested free port whenever it lands in non-recoverable INCOMPATIBLE (the single `_set_incompatible_server` chokepoint). This makes the upstream classification observable from the editor log for CI; the dock crash body (#608) renders the same hint but only to the UI, not stdout. Reservation-aware on Windows via `ClientConfigurator.suggest_free_port`. Verified locally: both smoke modes PASS against a headless 4.6 editor, and the full live unit suite is green (1506/1525, 0 failed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0153n16KG5VL4NDMUjzX1tWa * Harden terminate_proc against process-exit races in smoke cleanup terminate_proc() runs from a finally block, but poll() and the subsequent terminate()/send_signal()/kill() are not atomic: if the child exits in between, the signal call raises ProcessLookupError and turns an otherwise passing smoke into a failure. Wrap the signalling in exception handling for the already-exited case and reap after kill() so the child can't linger as a zombie. SIGTERM/Windows behavior is unchanged. Addresses a CodeRabbit review finding on #610. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0153n16KG5VL4NDMUjzX1tWa --------- Co-authored-by: Claude <noreply@anthropic.com>
3 participants
Closes #607.
Summary
Implements the cheap version from #607. When the HTTP port (default
8000) is held by a genuinely foreign (non-godot-ai) process, the dock lands in terminalINCOMPATIBLEand tells the user what to do but leaves them with the chore — hunt for a free port, then hand-edit each MCP client config. This surfaces a concrete free port inline and adds a docs link carrying the reconfigure steps.Changes
Inline copy names a concrete free port.
_crash_body_for_stateinmcp_dock.gdnow appends a suggestion for theINCOMPATIBLE && !can_recover_incompatiblecase, e.g.:The suggestion comes from the existing
ClientConfigurator.suggest_free_port(...), which already routes throughWindowsPortReservation.suggest_non_excluded_port, so the suggested port won't itself fail withWinError 10013on Windows. String/seam change only.Docs link in the crash panel. A new "How to change the port" button surfaces in the crash panel only for the foreign-occupant case, opening
docs/port-conflicts.md— a short guide covering "change the port + reconfigure your clients" (the per-client reconfigure steps that don't fit inline).New doc:
docs/port-conflicts.md.Scope / no behavior change
INCOMPATIBLE && !can_recover_incompatible(genuinely foreign occupant). Whencan_recover_incompatibleis true (a stale/older godot-ai we can prove we own), Restart Server stays the primary action and the docs link/free-port hint are suppressed — reclaiming8000beats fleeing to a random port.PORT_EXCLUDED/FOREIGN_PORTstates or theirport_picker_panel.Acceptance (cheap version)
INCOMPATIBLEcrash body names a concrete free port (reservation-aware on Windows)PORT_EXCLUDED/FOREIGN_PORT/ recoverable-godot-ai (can_recover_incompatible) casesTesting
script/ci-check-gdscript— clean (no parse errors).script/ci-godot-testsagainst a live server + headless editor: 1505/1524 passed, 0 failed (gap is pre-existing platform skips).test_dock.gdcoverage: foreign-occupant body names a free port + points atgodot_ai/http_port; recoverable body keeps Restart Server copy and omits the free-port hint; docs button visibility is gated to the foreign case.🤖 Generated with Claude Code
https://claude.ai/code/session_01WwE7zCBJ6qvjk7wyw3X5aU
Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests