Skip to content

fix(discovery): persist and prefer canonical backend URL#3350

Open
naicud wants to merge 1 commit into
BloopAI:mainfrom
naicud:fix/3349-mcp-discovery-canonical-backend
Open

fix(discovery): persist and prefer canonical backend URL#3350
naicud wants to merge 1 commit into
BloopAI:mainfrom
naicud:fix/3349-mcp-discovery-canonical-backend

Conversation

@naicud

@naicud naicud commented Apr 11, 2026

Copy link
Copy Markdown

fix(discovery): persist and prefer canonical backend URL

What

  • add an optional backend_url field to PortInfo and keep legacy raw-port / JSON discovery parsing intact
  • write the canonical backend URL into vibe-kanban.port during server startup
  • update MCP base URL resolution to prefer VIBE_BACKEND_URL, then explicit env host/port, then the discovered canonical backend URL before falling back to legacy host+port reconstruction
  • add unit coverage for discovery parsing and base-URL precedence

Why

Issue #3349 showed MCP could recover a stale or incomplete discovery record and rebuild http://127.0.0.1:<port> even when the healthy desktop backend was serving on http://localhost:<port> / ::1.

Persisting the backend's canonical runtime URL lets MCP reuse the same endpoint the server actually bound, avoiding loopback-family mismatches while preserving existing overrides and legacy discovery compatibility.

How

  • extend crates/utils/src/port_file.rs with PortInfo::backend_url, a read_port_info helper, and compatibility tests for legacy formats
  • write discovery metadata through write_port_file_with_proxy_and_backend_url(...) from crates/server/src/startup.rs
  • refactor resolve_base_url in crates/mcp/src/bin/vibe_kanban_mcp.rs into source-precedence logic that prefers the canonical discovered URL when present

Testing

  • cargo test -p utils --lib
  • cargo test -p mcp --bin vibe-kanban-mcp
  • cargo test -p server --lib --no-run

Related


Note

Medium Risk
Changes backend discovery/URL resolution between server and MCP; mistakes could break local connectivity if the port file or precedence logic is wrong, but scope is limited and covered by new unit tests.

Overview
Persists a canonical backend_url alongside ports in the temp discovery file (PortInfo), while keeping backwards-compatible parsing for legacy raw-port and older JSON formats.

On server startup, writes this canonical URL into the discovery record, and updates MCP base-URL resolution to prefer VIBE_BACKEND_URL, then explicit env host/port, then the discovered canonical URL before falling back to reconstructing http://<host>:<port>. Adds unit tests covering discovery parsing and URL precedence.

Reviewed by Cursor Bugbot for commit c9e7bab. Bugbot is set up for automated code reviews on this repo. Configure here.

- add optional backend_url support to discovery metadata while keeping legacy parsing
- write the canonical localhost backend URL during server startup
- prefer the canonical discovery URL before legacy host+port reconstruction in MCP

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c9e7bab. Configure here.

None => Some(read_port_info("vibe-kanban").await?),
};

resolve_base_url_from_sources(log_prefix, backend_url, host, explicit_port, port_info)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Eager evaluation breaks VIBE_BACKEND_URL short-circuit precedence

High Severity

resolve_base_url eagerly reads the port file and parses port env vars (both with ? propagation) before resolve_base_url_from_sources gets to check VIBE_BACKEND_URL. If VIBE_BACKEND_URL is set but the port file doesn't exist, or a port env var contains a non-numeric value, the function returns an error instead of using the highest-precedence override. The old code short-circuited on VIBE_BACKEND_URL before touching any other source, so this is a regression.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c9e7bab. Configure here.

@Juanlucasbg

Copy link
Copy Markdown

Aggressive review summary — PR #3350

221-line plumbing fix across crates/utils/src/port_file.rs, crates/server/src/startup.rs, and crates/mcp/src/bin/vibe_kanban_mcp.rs. Replaces the port-only on-disk discovery file with a PortInfo struct that captures the canonical backend URL (port + host + presumably scheme). MCP binary now prefers that canonical URL when not overridden by env vars. Verdict: clean — recommend merge.

Why the change is correct

The pre-PR discovery flow was: server writes port_file containing just an integer; MCP reads it and constructs http://${HOST_or_default}:${port}. The MCP-side host construction defaults to 127.0.0.1, which broke any deployment where the server bound to a non-default host (Docker, remote SSH, custom binding). The PR makes the server write a richer PortInfo so the MCP can use exactly the URL the server itself is listening on.

The refactor of resolve_base_url is well-shaped: precedence is VIBE_BACKEND_URL env > explicit PORT/BACKEND_PORT env > port_info from the discovery file. Each branch logs which source it picked, which is exactly the right operator-debugging surface.

Findings

  • Structural — PASS: The split into resolve_base_url (env reads) and resolve_base_url_from_sources (pure logic) makes the latter unit-testable. The Option<u16> / Option<PortInfo> shape correctly distinguishes "not set" from "set to default".
  • Adversarial — LOW:
    • File schema migration: If a stale port_file from an older version exists (pre-PR JSON shape), the new read_port_info deserializer needs to either (a) handle the legacy shape or (b) fail with a clear "stale port file, please restart server" message. Worth verifying — not visible in this excerpt.
    • Race: Server writes port_info, MCP reads. If MCP starts before server has finished writing, the read fails. Existing read_port_file likely already handled retries; ensure read_port_info preserves that behavior.
  • Security — LOW: The discovery file is presumably world-readable in temp dir. Holding the canonical URL there isn't sensitive (it's a localhost endpoint), but if a future deployment puts a non-localhost canonical URL in there, the file's permissions become more important. Consider making it 0600.
  • Conventions — PASS: tracing-info logging on every branch.

NITs

  • Test coverage: The pure resolve_base_url_from_sources function is begging for unit tests covering the four branches (backend_url set, explicit_port set, port_info available, all three missing). One test per branch.
  • Backward-compat: If the new PortInfo JSON shape isn't backward-compatible, document the breaking change in PR description so users running an old MCP against a new server don't silently fall back.

Verdict

Approve.

— Reviewed by automated single-pass review (discovery plumbing; full 4-tool battery skipped — refactor follows clean-input-output split with logged precedence).

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.

MCP backend discovery can latch onto a stale port file and wrong loopback host, missing a healthy desktop backend on localhost/::1

2 participants