From 8584d77f385586c6e595766589d434d5a43152b0 Mon Sep 17 00:00:00 2001 From: Egor Merkushev Date: Thu, 2 Jul 2026 17:17:01 +0300 Subject: [PATCH] Release 0.4.5 broker stabilization --- CHANGELOG.md | 19 ++++ README.md | 3 +- .../Documentation.docc/XcodeMCPWrapper.md | 3 +- docs/broker-mode.md | 49 +++++++++ pyproject.toml | 2 +- server.json | 4 +- src/mcpbridge_wrapper/__main__.py | 22 ++++- src/mcpbridge_wrapper/broker/daemon.py | 99 +++++++++++++------ src/mcpbridge_wrapper/broker/proxy.py | 42 +++++++- src/mcpbridge_wrapper/broker/transport.py | 43 +++++++- src/mcpbridge_wrapper/broker/types.py | 5 + src/mcpbridge_wrapper/doctor.py | 26 +++++ tests/unit/test_broker_daemon.py | 92 ++++++++++++++++- tests/unit/test_broker_proxy.py | 72 +++++++++++++- tests/unit/test_broker_stubs.py | 8 ++ tests/unit/test_broker_transport.py | 49 +++++++++ tests/unit/test_doctor.py | 8 ++ 17 files changed, 494 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 86db1dd7..86422577 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,24 @@ All notable changes to the mcpbridge-wrapper project will be documented in this The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.4.5] - 2026-07-02 + +### Added + +- Added broker host identity metadata in `~/.mcpbridge_wrapper/broker.host.json` and surfaced the recorded host executable in `--broker-status` and `--doctor`. +- Added `MCPBRIDGE_WRAPPER_BROKER_HOST_CMD` so broker auto-spawn can be pinned to a stable launcher path while clients continue using `--broker`. + +### Changed + +- Broker proxies now reuse the live per-user singleton daemon by default even when their package version differs, avoiding daemon churn during `uvx` cache or environment changes. Restart the daemon explicitly when upgrading the broker host. +- Updated broker-mode documentation to clarify `uvx --broker` singleton behavior and the tradeoff between convenient auto-spawn and a dedicated stable host. + +### Fixed + +- Prevented repeated downstream `initialize` and `notifications/initialized` messages from being forwarded to the single upstream Xcode MCP session, which could make `xcrun mcpbridge` close the connection under multiple real MCP clients. +- Increased broker and proxy stream limits so large Xcode tool catalogs can pass through the singleton broker without line-length read failures. +- Ensured reconnect replaces the previous upstream `xcrun mcpbridge` child before launching a new one, preventing stale upstream processes and repeated Xcode permission prompts. + ## [0.4.4] - 2026-03-10 ### Added @@ -123,6 +141,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - ✅ Support for all 20 Xcode MCP tools - ✅ Configuration examples for Cursor, Claude Code, and Codex CLI +[0.4.5]: https://github.com/SoundBlaster/XcodeMCPWrapper/releases/tag/v0.4.5 [0.4.4]: https://github.com/SoundBlaster/XcodeMCPWrapper/releases/tag/v0.4.4 [0.4.3]: https://github.com/SoundBlaster/XcodeMCPWrapper/releases/tag/v0.4.3 [0.4.2]: https://github.com/SoundBlaster/XcodeMCPWrapper/releases/tag/v0.4.2 diff --git a/README.md b/README.md index 48a86788..6df40d75 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,7 @@ -[![Version](https://img.shields.io/badge/version-0.4.4-blue.svg)](https://github.com/SoundBlaster/XcodeMCPWrapper/releases/tag/v0.4.4) +[![Version](https://img.shields.io/badge/version-0.4.5-blue.svg)](https://github.com/SoundBlaster/XcodeMCPWrapper/releases/tag/v0.4.5) [![Python 3.9+](https://img.shields.io/badge/python-3.9+-blue.svg)](https://www.python.org/downloads/) [![License: MIT](https://img.shields.io/badge/License-MIT-yellow.svg)](https://opensource.org/licenses/MIT) @@ -621,6 +621,7 @@ See [Web UI Setup Guide](docs/webui-setup.md) for detailed configuration. ## Known Issues - **Broker cold-start — Xcode approval timing race (0 tools with green dot):** When the broker daemon starts a new `xcrun mcpbridge` process (on first launch or after a daemon restart), Xcode shows a per-process "Allow Connection?" dialog. If your MCP client sends `tools/list` *before* Xcode grants approval, it receives an empty list and **caches it permanently** — showing 0 tools with a green connected indicator and no error message. Each unique binary path (direct wrapper vs broker daemon) triggers a *separate* dialog. After approval the permission persists — no re-approval is needed on subsequent sessions. **Workaround:** watch for the Xcode dialog immediately after enabling broker mode; after clicking Allow, reload the MCP connection in your client (disable → re-enable in settings). See [Troubleshooting: 0 tools after first broker connection](docs/troubleshooting.md#mcp-client-shows-0-tools-green-dot-after-first-broker-connection) for client-specific recovery steps and the diagnostic command. +- **Singleton broker host behavior:** `--broker` reuses one live daemon per local user, including `uvx --from mcpbridge-wrapper mcpbridge-wrapper --broker` clients. The first client that auto-spawns the daemon determines the Python host identity Xcode sees; for the most stable permission identity, start a dedicated broker host from a fixed path or set `MCPBRIDGE_WRAPPER_BROKER_HOST_CMD` before auto-spawn. - **BUG-T5 → FU-P13-T7 (P0):** Empty-content tool results can still violate strict `structuredContent` expectations in strict MCP clients. - **BUG-T6 → FU-P13-T8 (P0):** Web UI port collisions can happen when multiple MCP sessions start with the same `--web-ui-port` (for example `8080`), producing `address already in use`. - **BUG-T7 → FU-P13-T9 (P0):** `resources/list` and `resources/templates/list` probing may return non-standard error shapes in some client paths. diff --git a/Sources/XcodeMCPWrapper/Documentation.docc/XcodeMCPWrapper.md b/Sources/XcodeMCPWrapper/Documentation.docc/XcodeMCPWrapper.md index 336ad074..6e53d15a 100644 --- a/Sources/XcodeMCPWrapper/Documentation.docc/XcodeMCPWrapper.md +++ b/Sources/XcodeMCPWrapper/Documentation.docc/XcodeMCPWrapper.md @@ -8,7 +8,7 @@ Model Context Protocol (MCP). [https://github.com/SoundBlaster/XcodeMCPWrapper](https://github.com/SoundBlaster/XcodeMCPWrapper) -[![Version](https://img.shields.io/badge/version-0.4.4-blue.svg)](https://github.com/SoundBlaster/XcodeMCPWrapper/releases/tag/v0.4.4) +[![Version](https://img.shields.io/badge/version-0.4.5-blue.svg)](https://github.com/SoundBlaster/XcodeMCPWrapper/releases/tag/v0.4.5) ## Overview @@ -462,6 +462,7 @@ Important for multi-agent setups: ## Known Issues - **Broker cold-start — Xcode approval timing race (0 tools with green dot):** When the broker daemon starts a new `xcrun mcpbridge` process (on first launch or after a daemon restart), Xcode shows a per-process "Allow Connection?" dialog. If your MCP client sends `tools/list` *before* Xcode grants approval, it receives an empty list and **caches it permanently** — showing 0 tools with a green connected indicator and no error message. Each unique binary path (direct wrapper vs broker daemon) triggers a *separate* dialog. After approval the permission persists — no re-approval is needed on subsequent sessions. **Workaround:** watch for the Xcode dialog immediately after enabling broker mode; after clicking Allow, reload the MCP connection in your client (disable → re-enable in settings). See for client-specific recovery steps and the diagnostic command. +- **Singleton broker host behavior:** `--broker` reuses one live daemon per local user, including `uvx --from mcpbridge-wrapper mcpbridge-wrapper --broker` clients. The first client that auto-spawns the daemon determines the Python host identity Xcode sees; for the most stable permission identity, start a dedicated broker host from a fixed path or set `MCPBRIDGE_WRAPPER_BROKER_HOST_CMD` before auto-spawn. - **BUG-T5 → FU-P13-T7 (P0):** Empty-content tool results can still violate strict `structuredContent` expectations in strict MCP clients. - **BUG-T6 → FU-P13-T8 (P0):** Web UI port collisions can happen when multiple MCP sessions start with the same `--web-ui-port` (for example `8080`), producing `address already in use`. - **BUG-T7 → FU-P13-T9 (P0):** `resources/list` and `resources/templates/list` probing may return non-standard error shapes in some client paths. diff --git a/docs/broker-mode.md b/docs/broker-mode.md index 4f6833d8..b8017636 100644 --- a/docs/broker-mode.md +++ b/docs/broker-mode.md @@ -118,6 +118,55 @@ mcpbridge-wrapper --broker-stop Use broker mode when you want lower process churn across repeated MCP client restarts. +## Singleton broker host behavior + +Broker mode keeps one long-lived daemon per local macOS user. The daemon owns the +single upstream `xcrun mcpbridge` session and every `--broker` client connects to +that daemon through `~/.mcpbridge_wrapper/broker.sock`. This includes `uvx` +clients such as: + +```json +{ + "mcpServers": { + "xcode-tools": { + "command": "uvx", + "args": ["--from", "mcpbridge-wrapper", "mcpbridge-wrapper", "--broker"] + } + } +} +``` + +If a short-lived proxy has a different package version than the live daemon, the +proxy reuses that daemon and prints a warning instead of stopping and replacing it +automatically. Restart the daemon explicitly when you choose to upgrade the host: + +```bash +mcpbridge-wrapper --broker-stop +mcpbridge-wrapper --broker-daemon --web-ui +``` + +With `uvx`, the first client that needs to auto-spawn the daemon also determines +the daemon host identity that Xcode sees. That identity is usually a Python +executable from the `uvx` runtime/cache. For the most stable Xcode permission +identity, start a dedicated broker host from a fixed path, or pin auto-spawn to a +stable launcher with `MCPBRIDGE_WRAPPER_BROKER_HOST_CMD`. The configured command +is used only when `--broker` needs to auto-spawn a daemon; existing daemons are +reused. + +```bash +export MCPBRIDGE_WRAPPER_BROKER_HOST_CMD="/Users/egor/bin/mcpbridge-wrapper-host" +uvx --from 'mcpbridge-wrapper[webui]' mcpbridge-wrapper --broker --web-ui +``` + +The daemon writes host identity metadata to: + +```text +~/.mcpbridge_wrapper/broker.host.json +``` + +Use `mcpbridge-wrapper --broker-status` or `mcpbridge-wrapper --doctor` to inspect +the recorded host executable. + --- ## Multi-agent topology and Web UI ownership diff --git a/pyproject.toml b/pyproject.toml index a908df9d..6e4a43ec 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "mcpbridge-wrapper" -version = "0.4.4" +version = "0.4.5" description = "Python wrapper for xcrun mcpbridge that adds structuredContent to MCP responses" readme = "README.md" requires-python = ">=3.9" diff --git a/server.json b/server.json index 6a6d987d..9c664693 100644 --- a/server.json +++ b/server.json @@ -3,7 +3,7 @@ "name": "io.github.SoundBlaster/xcode-mcpbridge-wrapper", "title": "Xcode: MCP Bridge Wrapper for Cursor", "description": "Python wrapper that makes Xcode's MCP bridge compatible with strict MCP clients like Cursor", - "version": "0.4.4", + "version": "0.4.5", "repository": { "url": "https://github.com/SoundBlaster/XcodeMCPWrapper", "source": "github" @@ -13,7 +13,7 @@ { "registryType": "pypi", "identifier": "mcpbridge-wrapper", - "version": "0.4.4", + "version": "0.4.5", "transport": { "type": "stdio" }, diff --git a/src/mcpbridge_wrapper/__main__.py b/src/mcpbridge_wrapper/__main__.py index 4f01f0de..f4f1c64e 100644 --- a/src/mcpbridge_wrapper/__main__.py +++ b/src/mcpbridge_wrapper/__main__.py @@ -1,6 +1,7 @@ """Entry point for mcpbridge-wrapper.""" import contextlib +import json import os import signal import subprocess @@ -956,6 +957,7 @@ def main() -> int: print(f"PID file: {broker_config.pid_file}") print(f"Socket: {broker_config.socket_path}") print(f"Version file: {broker_config.version_file}") + print(f"Host file: {broker_config.host_file}") pid: Optional[int] = None if broker_config.pid_file.exists(): @@ -981,6 +983,12 @@ def main() -> int: print(f"WARNING: version mismatch! proxy={__version__}, daemon={daemon_version}") else: print("Daemon version: (unknown)") + if broker_config.host_file.exists(): + with contextlib.suppress(OSError, json.JSONDecodeError): + host_payload = json.loads(broker_config.host_file.read_text()) + host_executable = host_payload.get("executable") + if host_executable: + print(f"Daemon host: {host_executable}") return 0 # --broker-stop: stop running broker daemon and exit @@ -996,7 +1004,12 @@ def main() -> int: pid_val = int(pid_file.read_text().strip()) except (ValueError, OSError): print("Corrupt PID file; cleaning up.", file=sys.stderr) - for p in (pid_file, broker_config.socket_path, broker_config.version_file): + for p in ( + pid_file, + broker_config.socket_path, + broker_config.version_file, + broker_config.host_file, + ): p.unlink(missing_ok=True) return 0 @@ -1039,7 +1052,12 @@ def main() -> int: ) return 1 - for p in (pid_file, broker_config.socket_path, broker_config.version_file): + for p in ( + pid_file, + broker_config.socket_path, + broker_config.version_file, + broker_config.host_file, + ): p.unlink(missing_ok=True) print("Broker stopped and files cleaned up.") return 0 diff --git a/src/mcpbridge_wrapper/broker/daemon.py b/src/mcpbridge_wrapper/broker/daemon.py index 1a623dca..12087356 100644 --- a/src/mcpbridge_wrapper/broker/daemon.py +++ b/src/mcpbridge_wrapper/broker/daemon.py @@ -45,6 +45,8 @@ _BROKER_TOOLS_ID = -1 _TOOLS_PROBE_RETRY_BASE_DELAY_SECONDS = 0.25 _TOOLS_PROBE_RETRY_MAX_DELAY_SECONDS = 2.0 +_TOOLS_PROBE_INITIAL_DELAY_SECONDS = 0.1 +_UPSTREAM_STREAM_LIMIT_BYTES = 16 * 1024 * 1024 class BrokerDaemon: @@ -90,6 +92,8 @@ def __init__( self._shutdown_lock = threading.Lock() # Set after the broker's own initialize probe succeeds; cleared on reconnect. self._upstream_initialized: asyncio.Event = asyncio.Event() + # Cached initialize response from upstream; served to each downstream client. + self._initialize_response_cache: str | None = None # Cached tools/list result (JSON string); None until first successful probe. self._tools_list_cache: str | None = None # Normalized fingerprint of the last non-empty cached tools/list result. @@ -212,6 +216,19 @@ async def start(self) -> None: self._config.version_file.write_text(__version__) logger.debug("Version file written: %s", self._config.version_file) + self._config.host_file.write_text( + json.dumps( + { + "pid": os.getpid(), + "executable": sys.executable, + "argv": sys.argv, + "version": __version__, + }, + sort_keys=True, + ) + ) + logger.debug("Host identity file written: %s", self._config.host_file) + # Background reader self._stop_event.clear() self._stopped_event.clear() @@ -269,20 +286,7 @@ async def stop(self) -> None: ) try: - # Terminate upstream - if self._upstream is not None and self._upstream.returncode is None: - with contextlib.suppress(Exception): - if self._upstream.stdin is not None: - self._upstream.stdin.close() - try: - await asyncio.wait_for( - self._upstream.wait(), - timeout=self._config.graceful_shutdown_timeout, - ) - except asyncio.TimeoutError: - logger.warning("Upstream did not exit cleanly; killing.") - self._upstream.kill() - await self._upstream.wait() + await self._terminate_upstream(terminate=False) finally: # Always mark shutdown complete so run_forever/stop waiters unblock. self._cleanup_files() @@ -473,21 +477,7 @@ async def _rollback_startup(self) -> None: await self._read_task self._read_task = None - # Terminate the upstream subprocess - if self._upstream is not None and self._upstream.returncode is None: - with contextlib.suppress(Exception): - self._upstream.terminate() - try: - await asyncio.wait_for( - self._upstream.wait(), - timeout=self._config.graceful_shutdown_timeout, - ) - except asyncio.TimeoutError: - with contextlib.suppress(Exception): - self._upstream.kill() - with contextlib.suppress(Exception): - await self._upstream.wait() - self._upstream = None + await self._terminate_upstream() # Remove stale files and mark daemon as stopped self._cleanup_files() @@ -506,12 +496,14 @@ def _check_and_clear_stale_lock(self) -> None: pid_file = self._config.pid_file sock_file = self._config.socket_path ver_file = self._config.version_file + host_file = self._config.host_file if not pid_file.exists(): # No lock file — clear any orphaned socket/version and proceed if sock_file.exists(): sock_file.unlink(missing_ok=True) ver_file.unlink(missing_ok=True) + host_file.unlink(missing_ok=True) return raw = pid_file.read_text().strip() @@ -522,6 +514,7 @@ def _check_and_clear_stale_lock(self) -> None: pid_file.unlink(missing_ok=True) sock_file.unlink(missing_ok=True) ver_file.unlink(missing_ok=True) + host_file.unlink(missing_ok=True) return try: @@ -537,6 +530,7 @@ def _check_and_clear_stale_lock(self) -> None: pid_file.unlink(missing_ok=True) sock_file.unlink(missing_ok=True) ver_file.unlink(missing_ok=True) + host_file.unlink(missing_ok=True) except PermissionError as err: # Process exists but owned by another user — treat as running raise RuntimeError( @@ -550,9 +544,43 @@ async def _launch_upstream(self) -> None: stdin=PIPE, stdout=PIPE, stderr=sys.stderr, + limit=_UPSTREAM_STREAM_LIMIT_BYTES, ) logger.debug("Upstream launched (PID %d)", self._upstream.pid) + async def _terminate_upstream(self, *, terminate: bool = True) -> None: + """Close the current upstream subprocess and clear its process handle.""" + upstream = self._upstream + if upstream is None: + return + + if upstream.returncode is not None: + self._upstream = None + return + + with contextlib.suppress(Exception): + if upstream.stdin is not None: + upstream.stdin.close() + + if terminate: + with contextlib.suppress(Exception): + upstream.terminate() + + try: + await asyncio.wait_for( + upstream.wait(), + timeout=self._config.graceful_shutdown_timeout, + ) + except asyncio.TimeoutError: + logger.warning("Upstream did not exit cleanly; killing.") + with contextlib.suppress(Exception): + upstream.kill() + with contextlib.suppress(Exception): + await upstream.wait() + finally: + if self._upstream is upstream: + self._upstream = None + async def _read_upstream_loop(self) -> None: """Read JSON-RPC lines from upstream stdout indefinitely. @@ -592,6 +620,8 @@ async def _read_upstream_loop(self) -> None: if raw_id == _BROKER_INIT_ID: # Broker's own initialize probe response received. + if isinstance(msg, dict) and "result" in msg: + self._initialize_response_cache = line self._upstream_initialized.set() logger.info("Upstream initialize probe acknowledged; upstream is ready.") upstream = self._upstream @@ -614,7 +644,7 @@ async def _read_upstream_loop(self) -> None: upstream = self._upstream if upstream is not None and upstream.stdin is not None: self._reset_tools_probe_retry_backoff() - self._schedule_tools_list_probe() + self._schedule_tools_list_probe(delay=_TOOLS_PROBE_INITIAL_DELAY_SECONDS) continue if raw_id == _BROKER_TOOLS_ID: @@ -673,9 +703,11 @@ async def _reconnect(self) -> None: self._state = BrokerState.RECONNECTING # Invalidate readiness gate and cache so clients wait for the new upstream. self._upstream_initialized.clear() + self._initialize_response_cache = None self._tools_list_cache = None self._tools_catalog_ready.clear() self._cancel_tools_probe_retry() + await self._terminate_upstream() cap = self._config.reconnect_backoff_cap while not self._stop_event.is_set(): @@ -710,7 +742,12 @@ async def _reconnect(self) -> None: def _cleanup_files(self) -> None: """Remove PID file, socket file, and version file.""" - for path in (self._config.pid_file, self._config.socket_path, self._config.version_file): + for path in ( + self._config.pid_file, + self._config.socket_path, + self._config.version_file, + self._config.host_file, + ): try: path.unlink(missing_ok=True) logger.debug("Removed %s", path) diff --git a/src/mcpbridge_wrapper/broker/proxy.py b/src/mcpbridge_wrapper/broker/proxy.py index 9ce013c8..e007cd3c 100644 --- a/src/mcpbridge_wrapper/broker/proxy.py +++ b/src/mcpbridge_wrapper/broker/proxy.py @@ -20,6 +20,7 @@ import json import logging import os +import shlex import signal import socket import subprocess @@ -31,6 +32,8 @@ logger = logging.getLogger(__name__) +_PROXY_STREAM_LIMIT_BYTES = 16 * 1024 * 1024 + class BrokerProxy: """Forwards stdio ↔ Unix socket for a single MCP client. @@ -63,6 +66,7 @@ def __init__( connect_timeout: float = 10.0, spawn_args: list[str] | None = None, web_ui_port: int | None = None, + restart_on_version_mismatch: bool = False, stdin: asyncio.StreamReader | None = None, stdout: asyncio.StreamWriter | None = None, ) -> None: @@ -79,6 +83,7 @@ def __init__( self._new_broker_spawned: bool = False self._stdin = stdin self._stdout = stdout + self._restart_on_version_mismatch = restart_on_version_mismatch # ------------------------------------------------------------------ # Public API @@ -288,9 +293,23 @@ def _cleanup_broker_files(self) -> None: self._config.pid_file, self._config.socket_path, self._config.version_file, + self._config.host_file, ): path.unlink(missing_ok=True) + def _build_spawn_command(self, spawn_args: list[str]) -> list[str]: + """Return the command used to spawn the singleton broker host. + + ``MCPBRIDGE_WRAPPER_BROKER_HOST_CMD`` lets operators pin daemon + ownership to a stable launcher path, which is important for Xcode's + per-process permission identity. The configured command receives the + normalized broker daemon arguments appended to it. + """ + host_cmd = os.environ.get("MCPBRIDGE_WRAPPER_BROKER_HOST_CMD") + if host_cmd: + return [*shlex.split(host_cmd), *spawn_args] + return [sys.executable, "-m", "mcpbridge_wrapper", *spawn_args] + async def _spawn_broker_if_needed(self) -> None: """Spawn the broker daemon if not already running. @@ -337,9 +356,18 @@ async def _spawn_broker_if_needed(self) -> None: else: # Daemon is alive — check for version mismatch. if self._check_version_mismatch(): - logger.info("Stopping stale broker (version mismatch)…") - await loop.run_in_executor(None, self._stop_stale_daemon) - # Fall through to spawn a new daemon. + if self._restart_on_version_mismatch: + logger.info("Stopping stale broker (version mismatch)…") + await loop.run_in_executor(None, self._stop_stale_daemon) + # Fall through to spawn a new daemon. + else: + print( + "Warning: broker version differs from this proxy; " + "reusing the running singleton broker. Restart it " + "explicitly with --broker-stop when you want to upgrade.", + file=sys.stderr, + ) + return else: logger.debug("Broker already running (PID %d); skipping spawn.", pid) return @@ -364,6 +392,7 @@ async def _spawn_broker_if_needed(self) -> None: socket_path.unlink(missing_ok=True) pid_file.unlink(missing_ok=True) self._config.version_file.unlink(missing_ok=True) + self._config.host_file.unlink(missing_ok=True) # Fall through to spawn. logger.info("Spawning broker daemon…") @@ -375,7 +404,7 @@ async def _spawn_broker_if_needed(self) -> None: self._new_broker_spawned = True subprocess.Popen( - [sys.executable, "-m", "mcpbridge_wrapper", *spawn_args], + self._build_spawn_command(spawn_args), start_new_session=True, stdin=subprocess.DEVNULL, stdout=subprocess.DEVNULL, @@ -406,7 +435,10 @@ async def _connect_with_timeout( while loop.time() < deadline: try: - reader, writer = await asyncio.open_unix_connection(socket_path) + reader, writer = await asyncio.open_unix_connection( + socket_path, + limit=_PROXY_STREAM_LIMIT_BYTES, + ) logger.debug("Connected to broker at %s", socket_path) return reader, writer except (FileNotFoundError, ConnectionRefusedError) as exc: diff --git a/src/mcpbridge_wrapper/broker/transport.py b/src/mcpbridge_wrapper/broker/transport.py index 1761853e..d4935392 100644 --- a/src/mcpbridge_wrapper/broker/transport.py +++ b/src/mcpbridge_wrapper/broker/transport.py @@ -460,6 +460,44 @@ async def _process_client_line(self, session: ClientSession, line: str) -> None: raw_id = msg.get("id") is_notification = raw_id is None + if method_name == "notifications/initialized" and is_notification: + session.initialized = True + if session.pending_tools_list_changed: + session.pending_tools_list_changed = False + await self._write_to_session(session, _TOOLS_LIST_CHANGED_NOTIFICATION) + return + + if method_name == "initialize" and not is_notification: + if not self._daemon.upstream_initialized.is_set(): + try: + await asyncio.wait_for( + self._daemon.upstream_initialized.wait(), + timeout=float(self._config.queue_ttl), + ) + except asyncio.TimeoutError: + await self._send_error( + session, + raw_id, + -32001, + "Broker upstream not ready — request TTL exceeded", + ) + return + + cached_initialize = self._daemon._initialize_response_cache + if cached_initialize is None: + await self._send_error( + session, + raw_id, + -32001, + "Broker initialize response not ready", + ) + return + + response_msg = json.loads(cached_initialize) + response_msg["id"] = raw_id + await self._write_to_session(session, json.dumps(response_msg, separators=(",", ":"))) + return + broker_id: int | None = None local_alias: int | None = None @@ -589,11 +627,6 @@ async def _process_client_line(self, session: ClientSession, line: str) -> None: session.session_id, remapped_line, ) - if method_name == "notifications/initialized": - session.initialized = True - if session.pending_tools_list_changed: - session.pending_tools_list_changed = False - await self._write_to_session(session, _TOOLS_LIST_CHANGED_NOTIFICATION) except Exception as exc: logger.warning( "Failed to write to upstream from session %d: %s", diff --git a/src/mcpbridge_wrapper/broker/types.py b/src/mcpbridge_wrapper/broker/types.py index 101e4df2..0c519b46 100644 --- a/src/mcpbridge_wrapper/broker/types.py +++ b/src/mcpbridge_wrapper/broker/types.py @@ -49,6 +49,11 @@ def version_file(self) -> Path: """Path to the version stamp file, derived from pid_file's directory.""" return self.pid_file.parent / "broker.version" + @property + def host_file(self) -> Path: + """Path to broker host identity metadata, derived from pid_file's directory.""" + return self.pid_file.parent / "broker.host.json" + @classmethod def default(cls) -> BrokerConfig: """Return config with default paths under ~/.mcpbridge_wrapper/.""" diff --git a/src/mcpbridge_wrapper/doctor.py b/src/mcpbridge_wrapper/doctor.py index f385665d..ff3d27f1 100644 --- a/src/mcpbridge_wrapper/doctor.py +++ b/src/mcpbridge_wrapper/doctor.py @@ -3,6 +3,7 @@ from __future__ import annotations import contextlib +import json import os import shutil import subprocess @@ -29,6 +30,9 @@ class LocalBrokerDiagnostics: version_file_present: bool version: str | None version_mismatch: bool + host_file: str + host_file_present: bool + host_executable: str | None @dataclass @@ -127,6 +131,21 @@ def _read_local_version(version_file: str) -> str | None: return version or None +def _read_host_executable(host_file: str) -> str | None: + """Read broker host executable metadata when present.""" + if not os.path.exists(host_file): + return None + + try: + with open(host_file, encoding="utf-8") as handle: + payload = json.load(handle) + except (OSError, ValueError): + return None + + executable = payload.get("executable") + return executable if isinstance(executable, str) and executable else None + + def _find_listener_pids_for_port(port: int | None) -> list[int]: """Return listener PIDs bound to a TCP port, or an empty list.""" if port is None: @@ -177,6 +196,7 @@ def collect_local_broker_diagnostics(runtime: TUIRuntimeConfig) -> LocalBrokerDi pid_file = str(runtime.pid_file) pid, pid_running = _read_local_pid(pid_file) version = _read_local_version(str(runtime.version_file)) + host_file = runtime.version_file.parent / "broker.host.json" return LocalBrokerDiagnostics( pid_file=pid_file, @@ -189,6 +209,9 @@ def collect_local_broker_diagnostics(runtime: TUIRuntimeConfig) -> LocalBrokerDi version_file_present=runtime.version_file.exists(), version=version, version_mismatch=bool(version and version != __version__), + host_file=str(host_file), + host_file_present=host_file.exists(), + host_executable=_read_host_executable(str(host_file)), ) @@ -253,6 +276,9 @@ def classify_doctor_report( "Version File: " + f"{local.version_file} ({'present' if local.version_file_present else 'missing'})", f"Recorded Daemon Version: {local.version or 'n/a'}", + "Host File: " + + f"{local.host_file} ({'present' if local.host_file_present else 'missing'})", + f"Host Executable: {local.host_executable or 'n/a'}", ] if local.version_mismatch: local_state_lines.append(f"Version Mismatch: yes (package {__version__})") diff --git a/tests/unit/test_broker_daemon.py b/tests/unit/test_broker_daemon.py index 9aa0bb2e..ef861196 100644 --- a/tests/unit/test_broker_daemon.py +++ b/tests/unit/test_broker_daemon.py @@ -164,6 +164,21 @@ async def test_start_creates_data_dir(self, tmp_path: Path) -> None: if daemon._read_task and not daemon._read_task.done(): daemon._read_task.cancel() + @pytest.mark.asyncio + async def test_launch_upstream_uses_large_stream_limit(self, tmp_path: Path) -> None: + """Xcode 27 beta can emit MCP JSON lines larger than asyncio's default limit.""" + cfg = _make_config(tmp_path) + daemon = BrokerDaemon(cfg) + proc = _make_mock_process() + + with patch( + "mcpbridge_wrapper.broker.daemon.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc), + ) as mock_create: + await daemon._launch_upstream() + + assert mock_create.await_args.kwargs["limit"] >= 16 * 1024 * 1024 + # --------------------------------------------------------------------------- # start() — duplicate instance prevention @@ -452,6 +467,56 @@ async def _factory(*a, **kw): # type: ignore[no-untyped-def] if daemon._read_task and not daemon._read_task.done(): daemon._read_task.cancel() + @pytest.mark.asyncio + async def test_reconnect_terminates_previous_upstream_before_relaunch( + self, + tmp_path: Path, + ) -> None: + """EOF reconnect must not leave the previous upstream process alive.""" + cfg = BrokerConfig( + socket_path=tmp_path / "broker.sock", + pid_file=tmp_path / "broker.pid", + upstream_cmd=["true"], + reconnect_backoff_cap=0, + queue_ttl=5, + graceful_shutdown_timeout=1, + ) + daemon = BrokerDaemon(cfg) + + first_proc = _make_mock_process() + first_proc.terminate = MagicMock() + first_proc.stdout.readline = AsyncMock(return_value=b"") + + second_proc = _make_mock_process() + + async def _block(*a, **kw) -> bytes: # type: ignore[no-untyped-def] + await daemon._stop_event.wait() + return b"" + + second_proc.stdout.readline = _block + call_n = 0 + + async def _factory(*a, **kw): # type: ignore[no-untyped-def] + nonlocal call_n + call_n += 1 + return first_proc if call_n == 1 else second_proc + + with patch( + "mcpbridge_wrapper.broker.daemon.asyncio.create_subprocess_exec", + new=_factory, + ): + await daemon.start() + await asyncio.sleep(0.2) + + first_proc.stdin.close.assert_called() + first_proc.terminate.assert_called_once() + first_proc.wait.assert_awaited() + assert call_n >= 2 + + daemon._stop_event.set() + if daemon._read_task and not daemon._read_task.done(): + daemon._read_task.cancel() + # --------------------------------------------------------------------------- # Client disconnects do NOT affect upstream @@ -1027,9 +1092,30 @@ async def test_start_writes_version_file(self, tmp_path: Path) -> None: if daemon._read_task and not daemon._read_task.done(): daemon._read_task.cancel() + @pytest.mark.asyncio + async def test_start_writes_host_file(self, tmp_path: Path) -> None: + """start() writes broker host identity metadata alongside the PID file.""" + cfg = _make_config(tmp_path) + daemon = BrokerDaemon(cfg) + proc = _make_mock_process() + + with patch( + "mcpbridge_wrapper.broker.daemon.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc), + ): + await daemon.start() + + assert cfg.host_file.exists() + host_text = cfg.host_file.read_text() + assert "executable" in host_text + + daemon._stop_event.set() + if daemon._read_task and not daemon._read_task.done(): + daemon._read_task.cancel() + @pytest.mark.asyncio async def test_stop_removes_version_file(self, tmp_path: Path) -> None: - """stop() removes the version file.""" + """stop() removes broker state files.""" cfg = _make_config(tmp_path) daemon = BrokerDaemon(cfg) proc = _make_mock_process() @@ -1040,9 +1126,11 @@ async def test_stop_removes_version_file(self, tmp_path: Path) -> None: ): await daemon.start() assert cfg.version_file.exists() + assert cfg.host_file.exists() await daemon.stop() assert not cfg.version_file.exists() + assert not cfg.host_file.exists() @pytest.mark.asyncio async def test_status_includes_version(self, tmp_path: Path) -> None: @@ -1059,6 +1147,7 @@ def test_stale_lock_cleanup_removes_version_file(self, tmp_path: Path) -> None: cfg.pid_file.write_text("99999999") cfg.socket_path.write_text("leftover") cfg.version_file.write_text("0.1.0") + cfg.host_file.write_text("{}") daemon = BrokerDaemon(cfg) with patch( @@ -1068,6 +1157,7 @@ def test_stale_lock_cleanup_removes_version_file(self, tmp_path: Path) -> None: daemon._check_and_clear_stale_lock() assert not cfg.version_file.exists() + assert not cfg.host_file.exists() # --------------------------------------------------------------------------- diff --git a/tests/unit/test_broker_proxy.py b/tests/unit/test_broker_proxy.py index bb689afd..b5f7f9da 100644 --- a/tests/unit/test_broker_proxy.py +++ b/tests/unit/test_broker_proxy.py @@ -348,6 +348,35 @@ def _fake_exists(path_obj: Path) -> bool: assert cmd[:3] == [sys.executable, "-m", "mcpbridge_wrapper"] assert cmd[3:] == ["--broker-daemon", "--web-ui", "--web-ui-port", "9090"] + @pytest.mark.asyncio + async def test_spawn_uses_configured_broker_host_command(self, tmp_path: Path) -> None: + """A stable broker host command can own daemon startup instead of sys.executable.""" + cfg = _make_config(tmp_path) + proxy = BrokerProxy(cfg, auto_spawn=True, connect_timeout=1.0) + + real_exists = Path.exists + socket_checks = {"count": 0} + + def _fake_exists(path_obj: Path) -> bool: + if path_obj == cfg.pid_file: + return False + if path_obj == cfg.socket_path: + socket_checks["count"] += 1 + return socket_checks["count"] >= 2 + return real_exists(path_obj) + + with patch.dict( + os.environ, + {"MCPBRIDGE_WRAPPER_BROKER_HOST_CMD": "/opt/mcpbridge-wrapper/bin/host --fixed"}, + ), patch.object(Path, "exists", _fake_exists), patch("subprocess.Popen") as mock_popen: + await proxy._spawn_broker_if_needed() + + assert mock_popen.call_args.args[0] == [ + "/opt/mcpbridge-wrapper/bin/host", + "--fixed", + "--broker-daemon", + ] + # --------------------------------------------------------------------------- # Stale socket recovery (P2-T2) @@ -998,8 +1027,24 @@ async def test_connect_with_timeout_returns_streams_on_success(self, tmp_path: P assert writer is expected_writer @pytest.mark.asyncio - async def test_version_mismatch_triggers_restart(self, tmp_path: Path) -> None: - """When version mismatches, old daemon is stopped and new one spawned.""" + async def test_connect_with_timeout_uses_large_stream_limit(self, tmp_path: Path) -> None: + """Broker responses can exceed asyncio's default 64 KiB reader limit.""" + cfg = _make_config(tmp_path) + proxy = BrokerProxy(cfg, connect_timeout=0.2) + expected_reader = asyncio.StreamReader() + expected_writer = MagicMock() + + with patch( + "mcpbridge_wrapper.broker.proxy.asyncio.open_unix_connection", + AsyncMock(return_value=(expected_reader, expected_writer)), + ) as mock_connect: + await proxy._connect_with_timeout() + + assert mock_connect.await_args.kwargs["limit"] >= 16 * 1024 * 1024 + + @pytest.mark.asyncio + async def test_version_mismatch_reuses_singleton_by_default(self, tmp_path: Path) -> None: + """A live broker is reused by default even when its version differs.""" cfg = _make_config(tmp_path) # Write a live PID (our own) and a mismatched version cfg.pid_file.write_text(str(os.getpid())) @@ -1007,11 +1052,32 @@ async def test_version_mismatch_triggers_restart(self, tmp_path: Path) -> None: proxy = BrokerProxy(cfg, auto_spawn=True, connect_timeout=0.3) + with patch.object(proxy, "_pid_belongs_to_broker", return_value=True), patch.object( + proxy, "_stop_stale_daemon" + ) as mock_stop, patch("subprocess.Popen") as mock_popen: + await proxy._spawn_broker_if_needed() + + mock_stop.assert_not_called() + mock_popen.assert_not_called() + + @pytest.mark.asyncio + async def test_version_mismatch_can_still_restart_when_requested(self, tmp_path: Path) -> None: + """The old restart-on-mismatch behavior remains available as an explicit option.""" + cfg = _make_config(tmp_path) + cfg.pid_file.write_text(str(os.getpid())) + cfg.version_file.write_text("0.0.0-old") + + proxy = BrokerProxy( + cfg, + auto_spawn=True, + connect_timeout=0.3, + restart_on_version_mismatch=True, + ) + stop_called = [] def fake_stop() -> None: stop_called.append(True) - # Clean up files so spawn proceeds cfg.pid_file.unlink(missing_ok=True) cfg.socket_path.unlink(missing_ok=True) cfg.version_file.unlink(missing_ok=True) diff --git a/tests/unit/test_broker_stubs.py b/tests/unit/test_broker_stubs.py index 06e28de8..787756ce 100644 --- a/tests/unit/test_broker_stubs.py +++ b/tests/unit/test_broker_stubs.py @@ -101,6 +101,14 @@ def test_version_file_custom_path(self) -> None: ) assert cfg.version_file == Path("/tmp/custom/broker.version") + def test_host_file_custom_path(self) -> None: + cfg = BrokerConfig( + socket_path=Path("/tmp/test.sock"), + pid_file=Path("/tmp/custom/test.pid"), + upstream_cmd=["my-bridge"], + ) + assert cfg.host_file == Path("/tmp/custom/broker.host.json") + # --------------------------------------------------------------------------- # ClientSession diff --git a/tests/unit/test_broker_transport.py b/tests/unit/test_broker_transport.py index a5212e2b..6dc5d761 100644 --- a/tests/unit/test_broker_transport.py +++ b/tests/unit/test_broker_transport.py @@ -79,6 +79,17 @@ def _make_daemon_mock(state: BrokerState = BrokerState.READY) -> MagicMock: tools_ready_event.set() daemon.tools_catalog_ready = tools_ready_event # No cached tools/list by default. + daemon._initialize_response_cache = json.dumps( + { + "jsonrpc": "2.0", + "id": 0, + "result": { + "protocolVersion": "2024-11-05", + "capabilities": {}, + "serverInfo": {"name": "xcode", "version": "1"}, + }, + } + ) daemon._tools_list_cache = None return daemon @@ -327,6 +338,44 @@ async def test_non_integer_broker_id_dropped(self, tmp_path: Any) -> None: class TestProcessClientLine: + @pytest.mark.asyncio + async def test_initialize_served_locally_from_cache(self, tmp_path: Any) -> None: + server = _make_server(tmp_path) + session = _make_session(1) + server._sessions[1] = session + + request = json.dumps( + { + "jsonrpc": "2.0", + "id": "init-1", + "method": "initialize", + "params": { + "protocolVersion": "2024-11-05", + "capabilities": {}, + "clientInfo": {"name": "test", "version": "1"}, + }, + } + ) + await server._process_client_line(session, request) + + server._daemon._upstream.stdin.write.assert_not_called() + raw = session.writer.write.call_args.args[0].decode().strip() + response = json.loads(raw) + assert response["id"] == "init-1" + assert "result" in response + + @pytest.mark.asyncio + async def test_initialized_notification_not_forwarded_to_upstream(self, tmp_path: Any) -> None: + server = _make_server(tmp_path) + session = _make_session(1) + server._sessions[1] = session + + request = json.dumps({"jsonrpc": "2.0", "method": "notifications/initialized"}) + await server._process_client_line(session, request) + + assert session.initialized is True + server._daemon._upstream.stdin.write.assert_not_called() + @pytest.mark.asyncio async def test_integer_id_is_remapped(self, tmp_path: Any) -> None: server = _make_server(tmp_path) diff --git a/tests/unit/test_doctor.py b/tests/unit/test_doctor.py index 433afd12..2fc737cb 100644 --- a/tests/unit/test_doctor.py +++ b/tests/unit/test_doctor.py @@ -44,6 +44,8 @@ def _local( version_file_present: bool = True, version: str | None = "0.4.1", version_mismatch: bool = False, + host_file_present: bool = True, + host_executable: str | None = "/usr/local/bin/mcpbridge-wrapper", ) -> LocalBrokerDiagnostics: return LocalBrokerDiagnostics( pid_file="/tmp/broker.pid", @@ -56,6 +58,9 @@ def _local( version_file_present=version_file_present, version=version, version_mismatch=version_mismatch, + host_file="/tmp/broker.host.json", + host_file_present=host_file_present, + host_executable=host_executable, ) @@ -167,6 +172,8 @@ def test_collect_local_broker_diagnostics_reads_runtime_files(self, tmp_path: Pa socket_path.write_text("sock") version_file = tmp_path / "broker.version" version_file.write_text("0.4.1") + host_file = tmp_path / "broker.host.json" + host_file.write_text('{"executable": "/usr/local/bin/mcpbridge-wrapper"}') runtime = TUIRuntimeConfig( base_url="http://127.0.0.1:8080", auth_header=None, @@ -183,6 +190,7 @@ def test_collect_local_broker_diagnostics_reads_runtime_files(self, tmp_path: Pa assert diagnostics.pid_running is True assert diagnostics.socket_present is True assert diagnostics.version == "0.4.1" + assert diagnostics.host_executable == "/usr/local/bin/mcpbridge-wrapper" def test_collect_dashboard_diagnostics_collects_successful_probes(self) -> None: runtime = _runtime()