feat: add cmux MCP server for terminal control - #3
Conversation
📝 WalkthroughWalkthroughAdds a local MCP server stack (Node.js HTTP + stdio server and a Python stdio adapter), new docs and README entries, an npm script and dependencies, and tooling that routes MCP tool calls to the existing cmux v2 Unix socket or cmux CLI. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as MCP Client
participant NodeServer as Node MCP Server (HTTP)
participant CMUXCLI as cmux CLI
participant CMUXSocket as v2 Unix Socket
Agent->>NodeServer: POST /mcp (initialize)
NodeServer->>NodeServer: create session transport
NodeServer-->>Agent: initialize response
Agent->>NodeServer: POST /mcp (tools/call: cmux_tree)
NodeServer->>CMUXCLI: run `cmux tree` (args)
CMUXCLI->>CMUXSocket: query TerminalController/TabManager
CMUXSocket-->>CMUXCLI: structured tree
CMUXCLI-->>NodeServer: JSON output
NodeServer-->>Agent: tool result (wrapped)
sequenceDiagram
participant Agent as MCP Client
participant PyServer as Python MCP Server (stdio)
participant CMUXSocket as v2 Unix Socket
Agent->>PyServer: JSON-RPC initialize
PyServer->>PyServer: load config / socket path
PyServer-->>Agent: initialize response
Agent->>PyServer: JSON-RPC tools/call (tool, args)
PyServer->>CMUXSocket: connect & send cmux request
CMUXSocket-->>PyServer: response
PyServer-->>Agent: JSON-RPC tool result (content & structuredContent)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds a local MCP (Model Context Protocol) server for cmux, enabling external AI agents to programmatically control cmux terminals, workspaces, windows, panes, and surfaces. The server acts as a thin adapter over cmux's existing CLI and v2 Unix socket API, avoiding a second control plane.
Changes:
- Add
scripts/cmux_mcp_server.py: A Python MCP server that communicates directly with cmux's v2 Unix socket API using stdio transport, exposing 8 focused tools. - Add
scripts/cmux-mcp-server.mjs: A Node.js MCP server that wraps the cmux CLI, supporting both stdio and HTTP transports with 6 higher-level tools covering identify, tree, list, control, terminal, and browser operations. - Update
package.json/package-lock.jsonwith MCP SDK and Zod dependencies, plus a newcmux:mcpnpm script. - Add
docs/cmux-mcp-server.mdwith design rationale and tool taxonomy, and updateREADME.mdwith usage instructions.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/cmux_mcp_server.py |
Python stdio MCP server bridging to the v2 socket API via tests_v2/cmux.py |
scripts/cmux-mcp-server.mjs |
JS MCP server wrapping the cmux CLI, supporting stdio and HTTP transports |
package.json |
Adds MCP SDK and Zod dependencies, and the cmux:mcp run script |
package-lock.json |
Lock file for new dependencies; contains worktree artifact in name field |
docs/cmux-mcp-server.md |
Design document with architecture rationale; contains a broken local filesystem path |
README.md |
MCP server usage documentation; documents only the JS server, not the Python one |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client = cmux(self.socket_path) if self.socket_path else cmux() | ||
| client.connect() | ||
| return client | ||
|
|
There was a problem hiding this comment.
The _client() helper calls client.connect() explicitly and then returns the client object to be used as a context manager with with self._client() as client:. The context manager's __enter__ calls connect() a second time (which is a no-op due to the guard in cmux.connect()), but the issue is that if connect() raises a cmuxError inside _client(), the returned object is never entered and the exception propagates correctly. However, because the client is already connected before entering the context manager, any exception raised during with setup is handled in the caller's except block, not through __exit__. This is functionally correct but semantically inconsistent — either connect() should be removed from _client() (let the with statement handle it), or _client() should return a not-yet-connected cmux object for the context manager to connect. Currently the connect call in _client() is redundant and misleading.
| client = cmux(self.socket_path) if self.socket_path else cmux() | |
| client.connect() | |
| return client | |
| return cmux(self.socket_path) if self.socket_path else cmux() |
| REPO_ROOT = Path(__file__).resolve().parents[1] | ||
| TESTS_V2_DIR = REPO_ROOT / "tests_v2" | ||
| if str(TESTS_V2_DIR) not in sys.path: | ||
| sys.path.insert(0, str(TESTS_V2_DIR)) | ||
|
|
||
| from cmux import cmux, cmuxError # noqa: E402 | ||
|
|
||
|
|
There was a problem hiding this comment.
The Python MCP server imports cmux from tests_v2/cmux.py by manipulating sys.path at startup (lines 15–20). This means the production MCP server has a compile-time dependency on the test helper library in tests_v2/. If the tests_v2 directory is absent (e.g., in a packaged or deployed scenario, or if the test directory is reorganized), the server will fail to import. A more robust approach would be to move the cmux client library to a shared location (e.g., scripts/ or a proper package), or document this dependency explicitly so that users know the tests_v2 directory must be present.
| REPO_ROOT = Path(__file__).resolve().parents[1] | |
| TESTS_V2_DIR = REPO_ROOT / "tests_v2" | |
| if str(TESTS_V2_DIR) not in sys.path: | |
| sys.path.insert(0, str(TESTS_V2_DIR)) | |
| from cmux import cmux, cmuxError # noqa: E402 | |
| try: | |
| from cmux import cmux, cmuxError # type: ignore[import] | |
| except ImportError as exc: | |
| raise RuntimeError( | |
| "Failed to import 'cmux'. Ensure the cmux client library is installed and " | |
| "available on PYTHONPATH (for example by installing the cmux package or " | |
| "including the directory that contains cmux.py)." | |
| ) from exc |
| "name": "cmux_socket_call", | ||
| "description": "Call any existing cmux v2 socket method directly.", | ||
| "inputSchema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "method": {"type": "string"}, | ||
| "params": {"type": "object"}, | ||
| }, | ||
| "required": ["method"], | ||
| "additionalProperties": False, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The cmux_socket_call tool (lines 257–268 and 375–384) is an unrestricted pass-through that allows any caller to invoke arbitrary v2 socket methods by name. While intentional as an "escape hatch" for the MCP adapter, it exposes the full cmux socket API surface (including potentially destructive methods like workspace.close, surface.close, or close-window) without any method allowlist or audit logging. The tool description should warn callers about the risks, and the documentation should note that this tool effectively grants full cmux control to any MCP client that can reach the server. At minimum, consider logging the method name when cmux_socket_call is invoked.
| httpServer.listen(config.httpPort, config.httpHost, resolve); | ||
| }); | ||
|
|
||
| console.error(`cmux-mcp listening on http://${config.httpHost}:${config.httpPort}/mcp`); |
There was a problem hiding this comment.
The cmux_socket_call tool in the JS server (exposed through buildControlCommand and buildListCommand) provides an equivalent unrestricted pass-through to the cmux CLI. Additionally, the cmux_control tool's entity/action combinations such as workspace.close and surface.close can perform destructive operations. For a local stdio server this is an acceptable risk, but if the HTTP transport is used (line 808), the server binds to 127.0.0.1:8765 by default and any local process can send requests to it without authentication. There are no rate limits applied at this layer (only express-rate-limit from the MCP SDK). Consider documenting this risk when running the HTTP transport.
| console.error(`cmux-mcp listening on http://${config.httpHost}:${config.httpPort}/mcp`); | |
| console.error(`cmux-mcp listening on http://${config.httpHost}:${config.httpPort}/mcp`); | |
| console.error( | |
| 'WARNING: The HTTP transport exposes cmux control tools (including potentially destructive operations) ' + | |
| 'over an unauthenticated local HTTP interface. Any local process can send requests to this endpoint, ' + | |
| 'and no additional rate limiting is applied here beyond what is provided by the MCP SDK.' | |
| ); | |
| console.error( | |
| 'Ensure you only bind cmux-mcp to trusted interfaces (for example, 127.0.0.1) and add your own ' + | |
| 'network or authentication controls if you run this HTTP server in a less trusted environment.' | |
| ); |
| #!/usr/bin/env python3 | ||
| """Minimal MCP adapter for cmux's existing v2 Unix socket API.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import argparse | ||
| import json | ||
| import os | ||
| import sys | ||
| import traceback | ||
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
|
|
||
| REPO_ROOT = Path(__file__).resolve().parents[1] | ||
| TESTS_V2_DIR = REPO_ROOT / "tests_v2" | ||
| if str(TESTS_V2_DIR) not in sys.path: | ||
| sys.path.insert(0, str(TESTS_V2_DIR)) | ||
|
|
||
| from cmux import cmux, cmuxError # noqa: E402 | ||
|
|
||
|
|
||
| SERVER_NAME = "cmux-mcp" | ||
| SERVER_VERSION = "0.1.0" | ||
|
|
||
|
|
||
| def _json_default(value: Any) -> Any: | ||
| if isinstance(value, Path): | ||
| return str(value) | ||
| raise TypeError(f"Object of type {type(value).__name__} is not JSON serializable") | ||
|
|
||
|
|
||
| class McpProtocolError(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class CmuxMcpServer: | ||
| def __init__(self, socket_path: str | None = None) -> None: | ||
| self.socket_path = socket_path | ||
| self._protocol_version = "2024-11-05" | ||
|
|
||
| def serve(self) -> int: | ||
| while True: | ||
| message = self._read_message() | ||
| if message is None: | ||
| return 0 | ||
| self._handle_message(message) | ||
|
|
||
| def _read_message(self) -> dict[str, Any] | None: | ||
| headers: dict[str, str] = {} | ||
|
|
||
| while True: | ||
| line = sys.stdin.buffer.readline() | ||
| if not line: | ||
| if headers: | ||
| raise McpProtocolError("Unexpected EOF while reading MCP headers") | ||
| return None | ||
| if line in (b"\r\n", b"\n"): | ||
| break | ||
| decoded = line.decode("utf-8").strip() | ||
| if ":" not in decoded: | ||
| raise McpProtocolError(f"Invalid header line: {decoded!r}") | ||
| key, value = decoded.split(":", 1) | ||
| headers[key.strip().lower()] = value.strip() | ||
|
|
||
| content_length = headers.get("content-length") | ||
| if content_length is None: | ||
| raise McpProtocolError("Missing Content-Length header") | ||
|
|
||
| body = sys.stdin.buffer.read(int(content_length)) | ||
| if len(body) != int(content_length): | ||
| raise McpProtocolError("Unexpected EOF while reading MCP body") | ||
|
|
||
| payload = json.loads(body.decode("utf-8")) | ||
| if not isinstance(payload, dict): | ||
| raise McpProtocolError("Expected JSON object payload") | ||
| return payload | ||
|
|
||
| def _write_message(self, payload: dict[str, Any]) -> None: | ||
| encoded = json.dumps(payload, separators=(",", ":"), default=_json_default).encode("utf-8") | ||
| sys.stdout.buffer.write(f"Content-Length: {len(encoded)}\r\n\r\n".encode("ascii")) | ||
| sys.stdout.buffer.write(encoded) | ||
| sys.stdout.buffer.flush() | ||
|
|
||
| def _write_response(self, request_id: Any, result: dict[str, Any]) -> None: | ||
| self._write_message({ | ||
| "jsonrpc": "2.0", | ||
| "id": request_id, | ||
| "result": result, | ||
| }) | ||
|
|
||
| def _write_error(self, request_id: Any, code: int, message: str, data: Any = None) -> None: | ||
| error: dict[str, Any] = {"code": code, "message": message} | ||
| if data is not None: | ||
| error["data"] = data | ||
| self._write_message({ | ||
| "jsonrpc": "2.0", | ||
| "id": request_id, | ||
| "error": error, | ||
| }) | ||
|
|
||
| def _handle_message(self, message: dict[str, Any]) -> None: | ||
| method = message.get("method") | ||
| request_id = message.get("id") | ||
| params = message.get("params") or {} | ||
|
|
||
| try: | ||
| if method == "initialize": | ||
| requested_version = params.get("protocolVersion") | ||
| if isinstance(requested_version, str) and requested_version: | ||
| self._protocol_version = requested_version | ||
| self._write_response(request_id, self._initialize_result()) | ||
| return | ||
|
|
||
| if method == "notifications/initialized": | ||
| return | ||
|
|
||
| if method == "ping": | ||
| self._write_response(request_id, {}) | ||
| return | ||
|
|
||
| if method == "tools/list": | ||
| self._write_response(request_id, {"tools": self._tools()}) | ||
| return | ||
|
|
||
| if method == "tools/call": | ||
| name = params.get("name") | ||
| arguments = params.get("arguments") or {} | ||
| result = self._call_tool(name, arguments) | ||
| self._write_response(request_id, result) | ||
| return | ||
|
|
||
| if request_id is not None: | ||
| self._write_error(request_id, -32601, f"Method not found: {method}") | ||
| except cmuxError as exc: | ||
| if request_id is not None: | ||
| self._write_error(request_id, -32001, str(exc)) | ||
| except McpProtocolError: | ||
| raise | ||
| except Exception as exc: # pragma: no cover - defensive scaffold | ||
| if request_id is not None: | ||
| self._write_error( | ||
| request_id, | ||
| -32603, | ||
| f"Internal error: {exc}", | ||
| {"traceback": traceback.format_exc(limit=8)}, | ||
| ) | ||
|
|
||
| def _initialize_result(self) -> dict[str, Any]: | ||
| return { | ||
| "protocolVersion": self._protocol_version, | ||
| "serverInfo": { | ||
| "name": SERVER_NAME, | ||
| "version": SERVER_VERSION, | ||
| }, | ||
| "capabilities": { | ||
| "tools": {}, | ||
| }, | ||
| } | ||
|
|
||
| def _tools(self) -> list[dict[str, Any]]: | ||
| return [ | ||
| { | ||
| "name": "cmux_socket_discover", | ||
| "description": "List candidate local cmux Unix sockets under /tmp.", | ||
| "inputSchema": { | ||
| "type": "object", | ||
| "properties": {}, | ||
| "additionalProperties": False, | ||
| }, | ||
| }, | ||
| { | ||
| "name": "cmux_system_tree", | ||
| "description": "Return the active cmux hierarchy: windows, workspaces, panes, and surfaces.", | ||
| "inputSchema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "workspace_id": {"type": "string"}, | ||
| "all_windows": {"type": "boolean"}, | ||
| }, | ||
| "additionalProperties": False, | ||
| }, | ||
| }, | ||
| { | ||
| "name": "cmux_list_workspaces", | ||
| "description": "List workspaces for the current socket or a specific window.", | ||
| "inputSchema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "window_id": {"type": "string"}, | ||
| }, | ||
| "additionalProperties": False, | ||
| }, | ||
| }, | ||
| { | ||
| "name": "cmux_create_workspace", | ||
| "description": "Create a workspace, optionally in a target window, and optionally rename it.", | ||
| "inputSchema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "window_id": {"type": "string"}, | ||
| "title": {"type": "string"}, | ||
| }, | ||
| "additionalProperties": False, | ||
| }, | ||
| }, | ||
| { | ||
| "name": "cmux_select_workspace", | ||
| "description": "Select a workspace by UUID or cmux workspace ref.", | ||
| "inputSchema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "workspace_id": {"type": "string"}, | ||
| }, | ||
| "required": ["workspace_id"], | ||
| "additionalProperties": False, | ||
| }, | ||
| }, | ||
| { | ||
| "name": "cmux_list_surfaces", | ||
| "description": "List surfaces in the current workspace or a specific workspace.", | ||
| "inputSchema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "workspace_id": {"type": "string"}, | ||
| }, | ||
| "additionalProperties": False, | ||
| }, | ||
| }, | ||
| { | ||
| "name": "cmux_send_text", | ||
| "description": "Send text to the focused surface or a specific surface.", | ||
| "inputSchema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "surface_id": {"type": "string"}, | ||
| "text": {"type": "string"}, | ||
| }, | ||
| "required": ["text"], | ||
| "additionalProperties": False, | ||
| }, | ||
| }, | ||
| { | ||
| "name": "cmux_read_text", | ||
| "description": "Read visible terminal text from the focused surface or a specific surface.", | ||
| "inputSchema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "workspace_id": {"type": "string"}, | ||
| "surface_id": {"type": "string"}, | ||
| "scrollback": {"type": "boolean"}, | ||
| }, | ||
| "additionalProperties": False, | ||
| }, | ||
| }, | ||
| { | ||
| "name": "cmux_socket_call", | ||
| "description": "Call any existing cmux v2 socket method directly.", | ||
| "inputSchema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "method": {"type": "string"}, | ||
| "params": {"type": "object"}, | ||
| }, | ||
| "required": ["method"], | ||
| "additionalProperties": False, | ||
| }, | ||
| }, | ||
| ] | ||
|
|
||
| def _call_tool(self, name: str, arguments: dict[str, Any]) -> dict[str, Any]: | ||
| handlers = { | ||
| "cmux_socket_discover": self._tool_socket_discover, | ||
| "cmux_system_tree": self._tool_system_tree, | ||
| "cmux_list_workspaces": self._tool_list_workspaces, | ||
| "cmux_create_workspace": self._tool_create_workspace, | ||
| "cmux_select_workspace": self._tool_select_workspace, | ||
| "cmux_list_surfaces": self._tool_list_surfaces, | ||
| "cmux_send_text": self._tool_send_text, | ||
| "cmux_read_text": self._tool_read_text, | ||
| "cmux_socket_call": self._tool_socket_call, | ||
| } | ||
| handler = handlers.get(name) | ||
| if handler is None: | ||
| raise cmuxError(f"Unknown tool: {name}") | ||
| payload = handler(arguments) | ||
| return self._tool_result(payload) | ||
|
|
||
| def _client(self) -> cmux: | ||
| client = cmux(self.socket_path) if self.socket_path else cmux() | ||
| client.connect() | ||
| return client | ||
|
|
||
| def _tool_socket_discover(self, _: dict[str, Any]) -> dict[str, Any]: | ||
| sockets = [] | ||
| for path in sorted(Path("/tmp").glob("cmux*.sock")): | ||
| sockets.append({ | ||
| "path": str(path), | ||
| "exists": path.exists(), | ||
| "is_socket": path.is_socket(), | ||
| }) | ||
| return { | ||
| "default_socket_path": self.socket_path or cmux.DEFAULT_SOCKET_PATH, | ||
| "sockets": sockets, | ||
| } | ||
|
|
||
| def _tool_system_tree(self, arguments: dict[str, Any]) -> dict[str, Any]: | ||
| params: dict[str, Any] = {} | ||
| if "workspace_id" in arguments: | ||
| params["workspace_id"] = arguments["workspace_id"] | ||
| if "all_windows" in arguments: | ||
| params["all_windows"] = bool(arguments["all_windows"]) | ||
| with self._client() as client: | ||
| result = client._call("system.tree", params) | ||
| return dict(result or {}) | ||
|
|
||
| def _tool_list_workspaces(self, arguments: dict[str, Any]) -> dict[str, Any]: | ||
| params: dict[str, Any] = {} | ||
| if "window_id" in arguments: | ||
| params["window_id"] = arguments["window_id"] | ||
| with self._client() as client: | ||
| result = client._call("workspace.list", params) | ||
| return dict(result or {}) | ||
|
|
||
| def _tool_create_workspace(self, arguments: dict[str, Any]) -> dict[str, Any]: | ||
| params: dict[str, Any] = {} | ||
| if "window_id" in arguments: | ||
| params["window_id"] = arguments["window_id"] | ||
| with self._client() as client: | ||
| create_result = dict(client._call("workspace.create", params) or {}) | ||
| workspace_id = create_result.get("workspace_id") | ||
| title = (arguments.get("title") or "").strip() | ||
| if workspace_id and title: | ||
| client._call("workspace.rename", { | ||
| "workspace_id": workspace_id, | ||
| "title": title, | ||
| }) | ||
| create_result["title"] = title | ||
| return create_result | ||
|
|
||
| def _tool_select_workspace(self, arguments: dict[str, Any]) -> dict[str, Any]: | ||
| workspace_id = str(arguments["workspace_id"]).strip() | ||
| with self._client() as client: | ||
| result = client._call("workspace.select", {"workspace_id": workspace_id}) | ||
| return dict(result or {}) | ||
|
|
||
| def _tool_list_surfaces(self, arguments: dict[str, Any]) -> dict[str, Any]: | ||
| params: dict[str, Any] = {} | ||
| if "workspace_id" in arguments: | ||
| params["workspace_id"] = arguments["workspace_id"] | ||
| with self._client() as client: | ||
| result = client._call("surface.list", params) | ||
| return dict(result or {}) | ||
|
|
||
| def _tool_send_text(self, arguments: dict[str, Any]) -> dict[str, Any]: | ||
| params = {"text": str(arguments["text"])} | ||
| if "surface_id" in arguments: | ||
| params["surface_id"] = str(arguments["surface_id"]).strip() | ||
| with self._client() as client: | ||
| result = client._call("surface.send_text", params) | ||
| return dict(result or {}) | ||
|
|
||
| def _tool_read_text(self, arguments: dict[str, Any]) -> dict[str, Any]: | ||
| params: dict[str, Any] = {} | ||
| if "workspace_id" in arguments: | ||
| params["workspace_id"] = arguments["workspace_id"] | ||
| if "surface_id" in arguments: | ||
| params["surface_id"] = arguments["surface_id"] | ||
| if "scrollback" in arguments: | ||
| params["scrollback"] = bool(arguments["scrollback"]) | ||
| with self._client() as client: | ||
| result = client._call("surface.read_text", params) | ||
| return dict(result or {}) | ||
|
|
||
| def _tool_socket_call(self, arguments: dict[str, Any]) -> dict[str, Any]: | ||
| method = str(arguments["method"]).strip() | ||
| params = arguments.get("params") or {} | ||
| if not isinstance(params, dict): | ||
| raise cmuxError("cmux_socket_call.params must be an object") | ||
| with self._client() as client: | ||
| result = client._call(method, params) | ||
| if isinstance(result, dict): | ||
| return result | ||
| return {"value": result} | ||
|
|
||
| def _tool_result(self, payload: dict[str, Any]) -> dict[str, Any]: | ||
| pretty = json.dumps(payload, indent=2, sort_keys=True, default=_json_default) | ||
| return { | ||
| "content": [ | ||
| { | ||
| "type": "text", | ||
| "text": pretty, | ||
| } | ||
| ], | ||
| "structuredContent": payload, | ||
| } | ||
|
|
||
|
|
||
| def parse_args() -> argparse.Namespace: | ||
| parser = argparse.ArgumentParser(description="cmux MCP stdio adapter") | ||
| parser.add_argument( | ||
| "--socket", | ||
| default=os.environ.get("CMUX_SOCKET_PATH"), | ||
| help="Override the cmux Unix socket path.", | ||
| ) | ||
| return parser.parse_args() | ||
|
|
||
|
|
||
| def main() -> int: | ||
| args = parse_args() | ||
| server = CmuxMcpServer(socket_path=args.socket) | ||
| return server.serve() | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| raise SystemExit(main()) |
There was a problem hiding this comment.
The test_coverage for the new MCP server scripts is missing. scripts/release_asset_guard.js has a companion test file scripts/release_asset_guard.test.js, establishing a convention that scripts in this directory have unit tests. Neither scripts/cmux_mcp_server.py nor scripts/cmux-mcp-server.mjs have any tests. In particular, the protocol parsing logic in _read_message (especially header parsing and content-length handling), the buildListCommand/buildControlCommand/buildTerminalCommand/buildBrowserCommand dispatch functions, and the parseCliArgs function in the JS server could all be unit-tested without a running cmux instance.
|
|
||
| body = sys.stdin.buffer.read(int(content_length)) | ||
| if len(body) != int(content_length): |
There was a problem hiding this comment.
The _read_message method passes content_length directly to int() on line 70 without validating that it's a valid non-negative integer first. A malformed Content-Length header (e.g., "abc", "-1", or a float like "1.5") will raise an unhandled ValueError from serve(), crashing the server process with an unhandled exception instead of a controlled exit. The ValueError is not a McpProtocolError, so it won't be caught by the re-raise path in _handle_message either. The int() call should be wrapped in a try/except and converted to a McpProtocolError, and the value should be validated to be positive before use.
| body = sys.stdin.buffer.read(int(content_length)) | |
| if len(body) != int(content_length): | |
| try: | |
| content_length_int = int(content_length) | |
| except ValueError: | |
| raise McpProtocolError(f"Invalid Content-Length header: {content_length!r}") | |
| if content_length_int < 0: | |
| raise McpProtocolError(f"Invalid Content-Length header (negative): {content_length!r}") | |
| body = sys.stdin.buffer.read(content_length_int) | |
| if len(body) != content_length_int: |
|
|
||
| This is justified because cmux already exposes most of the primitives an MCP host would need: | ||
|
|
||
| 1. `TerminalController` already implements a JSON v2 socket protocol with stable methods for `window.*`, `workspace.*`, `pane.*`, `surface.*`, and `browser.*` in [TerminalController.swift](/tmp/cmux-mcp-worktree/Sources/TerminalController.swift). |
There was a problem hiding this comment.
The documentation references a hardcoded local filesystem path /tmp/cmux-mcp-worktree/Sources/TerminalController.swift which is a temporary worktree path and won't be valid for any reader of this documentation. This link will always be broken. It should be replaced with a repository-relative path such as Sources/TerminalController.swift.
| 1. `TerminalController` already implements a JSON v2 socket protocol with stable methods for `window.*`, `workspace.*`, `pane.*`, `surface.*`, and `browser.*` in [TerminalController.swift](/tmp/cmux-mcp-worktree/Sources/TerminalController.swift). | |
| 1. `TerminalController` already implements a JSON v2 socket protocol with stable methods for `window.*`, `workspace.*`, `pane.*`, `surface.*`, and `browser.*` in [TerminalController.swift](Sources/TerminalController.swift). |
| default=os.environ.get("CMUX_SOCKET_PATH"), | ||
| help="Override the cmux Unix socket path.", |
There was a problem hiding this comment.
The --socket argument in parse_args() only reads CMUX_SOCKET_PATH from the environment as its default, but tests_v2/cmux.py's _default_socket_path() also supports the CMUX_SOCKET environment variable for backwards compatibility. When no --socket argument is provided and CMUX_SOCKET_PATH is unset but CMUX_SOCKET is set, socket_path will be None, and _client() will instantiate cmux() without a path, allowing tests_v2/cmux.py to pick up CMUX_SOCKET correctly. So in practice this is not a bug, but it is an inconsistency in the documented interface: the README only documents CMUX_MCP_SOCKET_PATH for the JS server, while the Python server documents CMUX_SOCKET_PATH. Clarifying this in the documentation and help text would improve usability.
| default=os.environ.get("CMUX_SOCKET_PATH"), | |
| help="Override the cmux Unix socket path.", | |
| default=os.environ.get("CMUX_SOCKET_PATH") or os.environ.get("CMUX_SOCKET"), | |
| help=( | |
| "Override the cmux Unix socket path. " | |
| "Defaults to CMUX_SOCKET_PATH or CMUX_SOCKET if set." | |
| ), |
| ## MCP server | ||
|
|
||
| cmux now includes an initial local MCP server for external agents that want to steer cmux programmatically instead of shelling out directly. | ||
|
|
||
| What it wraps today: | ||
| - `system.identify` and `tree` for discovery | ||
| - window, workspace, pane, and surface control through the existing `cmux` CLI | ||
| - terminal interaction via `read-screen`, `send`, and `send-key` | ||
| - a first-pass browser wrapper for common `cmux browser ...` actions | ||
|
|
||
| The server intentionally reuses the existing CLI and socket surface instead of introducing a second control plane, so MCP clients get the same handle model (`window:N`, `workspace:N`, `pane:N`, `surface:N`) and routing behavior that cmux already documents. | ||
|
|
||
| Install the Node dependencies once: | ||
|
|
||
| ```bash | ||
| npm install | ||
| ``` | ||
|
|
||
| Run over stdio: | ||
|
|
||
| ```bash | ||
| npm run cmux:mcp | ||
| ``` | ||
|
|
||
| Run over HTTP: | ||
|
|
||
| ```bash | ||
| npm run cmux:mcp -- --transport http --host 127.0.0.1 --port 8765 | ||
| ``` | ||
|
|
||
| Useful environment variables: | ||
|
|
||
| ```bash | ||
| export CMUX_MCP_CMUX_BIN=/path/to/cmux | ||
| export CMUX_MCP_SOCKET_PATH=/tmp/cmux.sock | ||
| export CMUX_MCP_SOCKET_PASSWORD=... | ||
| export CMUX_MCP_ID_FORMAT=refs | ||
| ``` | ||
|
|
||
| Available MCP tools: | ||
| - `cmux_identify` | ||
| - `cmux_tree` | ||
| - `cmux_list` | ||
| - `cmux_control` | ||
| - `cmux_terminal` | ||
| - `cmux_browser` | ||
|
|
||
| Term mapping for MCP clients: | ||
| - `window` is a native macOS cmux window | ||
| - `workspace` is the sidebar tab-like container | ||
| - `pane` is a split region inside a workspace | ||
| - `surface` is a tab inside a pane, usually the most stable automation target |
There was a problem hiding this comment.
The PR description mentions the Python server (python scripts/cmux_mcp_server.py) as a runnable implementation, but the README (lines 110–161) only documents the JavaScript MCP server. The Python server has a different tool set (8 tools using the v2 socket API directly) compared to the JavaScript server (6 tools using the cmux CLI). Neither the README nor the docs explain when to use which server, or that both exist. This creates confusion for users. The README should either document the Python server as an alternative, or explain the relationship/difference between the two implementations.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9641bb9e9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function compactArgs(values) { | ||
| return values.filter((value) => value !== undefined && value !== null && value !== ''); | ||
| } |
There was a problem hiding this comment.
The compactArgs helper drops only empty values, so when callers pass alternating ['--flag', optionalValue] pairs, missing values leave the flag behind and shift subsequent arguments. For example, cmux_control workspace create builds ['--cwd', input.cwd, '--command', input.commandText]; with defaults this becomes ['--cwd', '--command'], and the CLI parser (parseOption in CLI/cmux.swift) treats --command as the cwd value, causing malformed requests or failures. The same pattern affects other commands that rely on optional flag/value pairs, so tools can fail even on valid minimal inputs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cmux-mcp-server.md`:
- Line 11: The markdown link to TerminalController.swift uses an absolute local
path (/tmp/...) which is invalid for others; update the link to a repo-relative
path (e.g., Sources/TerminalController.swift or the correct relative location in
the repository) so the reference resolves for all readers and CI, and ensure the
link target matches the actual file name and directory where
TerminalController.swift is defined (TerminalController.swift, and any mention
of JSON v2 socket protocol methods like window.*, workspace.*, pane.*,
surface.*, browser.*).
In `@scripts/cmux_mcp_server.py`:
- Around line 329-339: The create+rename flow in cmux_create_workspace is not
atomic: if client._call("workspace.create", params) succeeds but
client._call("workspace.rename", ...) fails you end up with a created workspace
and a reported error, causing duplicate creates on retry; fix by passing the
title as part of the initial workspace.create call (set params["title"] = title
when title is non-empty) and remove the separate workspace.rename call, or
alternatively catch exceptions from client._call("workspace.rename") and return
create_result with the created workspace_id/title to surface partial success;
update the logic around client._call("workspace.create", params), create_result,
workspace_id, title, and workspace.rename accordingly.
- Around line 66-72: Validate and parse the Content-Length header before calling
sys.stdin.buffer.read: replace the direct use of headers.get("content-length")
and read(int(content_length)) with code in which you retrieve content_length,
attempt to parse it to an integer in a try/except, raise McpProtocolError for
non-integers, check it is >= 0 (and optionally enforce a reasonable upper bound)
and only then call sys.stdin.buffer.read(parsed_length); ensure IO size
mismatches still raise McpProtocolError when the returned body length !=
parsed_length.
- Around line 108-112: The code currently sets self._protocol_version to any
non-empty requested_version in the "initialize" branch, which can falsely
advertise support for unsupported MCP versions; change the logic in the
initialize handling so you only accept and assign requested_version if it
exactly matches the server's supported version(s) (e.g., compare against a
single constant like SUPPORTED_PROTOCOL_VERSION or a whitelist) and otherwise
leave self._protocol_version unchanged (or return an appropriate
error/negotiation response) before calling _write_response(request_id,
self._initialize_result()).
In `@scripts/cmux-mcp-server.mjs`:
- Around line 135-156: runCmux's execFileAsync call can hang and also currently
exposes raw argv (including sensitive flags like --socket-password) in the
thrown error; fix by passing a sensible timeout option to execFileAsync (e.g.,
timeout in ms) so the subprocess is killed on stall and update the catch error
path in runCmux to redact sensitive argv values before interpolating into the
thrown Error (replace socket-password value and user-supplied terminal input
with a placeholder like "[REDACTED]" or redact entire argv when any sensitive
flag is present), while preserving the existing trimmed stdout/stderr suffix
assembly (use the same error.stdout/error.stderr extraction) so the thrown error
shows the redacted command and the collected output but never the raw sensitive
arguments.
- Around line 808-865: The server currently binds to whatever config.httpHost
provides in runHttpServer and can expose sensitive controls; before
creating/starting the HTTP server (in runHttpServer / where httpServer.listen is
called), validate that config.httpHost is a loopback address (e.g., '127.0.0.1',
'::1', or 'localhost') or that an explicit opt-in flag is set (e.g.,
process.env.CMUX_MCP_ALLOW_REMOTE === '1' or a new config.allowRemoteMcp
boolean); if the host is non-loopback and no explicit opt-in is present, refuse
to bind (throw or return error) or override to bind to loopback only and log a
clear warning. Update runHttpServer, the httpServer.listen call, and any config
loading to honor the new allowRemoteMcp guard and include explanatory logging
when remote binding is allowed.
- Around line 272-277: The switch cases for 'next', 'previous', and 'last' are
emitting top-level window verbs ('next-window', 'previous-window',
'last-window') instead of workspace-scoped actions; update the three branches
(case 'next', case 'previous', case 'last') so they emit the correct
workspace-scoped contract (e.g., set command to 'next' / 'previous' / 'last' and
include entity: 'workspace' on the returned action or set options.entity =
'workspace'), and if your workspace-scoped commands are not implemented yet,
remove or return no-op/null for these cases so they aren't advertised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d12da44-7ed6-4ce3-91a1-145529c9170a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
README.mddocs/cmux-mcp-server.mdpackage.jsonscripts/cmux-mcp-server.mjsscripts/cmux_mcp_server.py
| with self._client() as client: | ||
| create_result = dict(client._call("workspace.create", params) or {}) | ||
| workspace_id = create_result.get("workspace_id") | ||
| title = (arguments.get("title") or "").strip() | ||
| if workspace_id and title: | ||
| client._call("workspace.rename", { | ||
| "workspace_id": workspace_id, | ||
| "title": title, | ||
| }) | ||
| create_result["title"] = title | ||
| return create_result |
There was a problem hiding this comment.
cmux_create_workspace is not failure-atomic.
If workspace.create succeeds and workspace.rename fails, the tool returns an error even though the workspace already exists. A retrying MCP client will create duplicates. Either set the title as part of the initial backend call, or surface partial success with the created workspace_id instead of failing the whole tool.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/cmux_mcp_server.py` around lines 329 - 339, The create+rename flow in
cmux_create_workspace is not atomic: if client._call("workspace.create", params)
succeeds but client._call("workspace.rename", ...) fails you end up with a
created workspace and a reported error, causing duplicate creates on retry; fix
by passing the title as part of the initial workspace.create call (set
params["title"] = title when title is non-empty) and remove the separate
workspace.rename call, or alternatively catch exceptions from
client._call("workspace.rename") and return create_result with the created
workspace_id/title to surface partial success; update the logic around
client._call("workspace.create", params), create_result, workspace_id, title,
and workspace.rename accordingly.
|
@CodeRabbit-ai @greptileai @BugBot @copilot Summary (Review Flow):
PR Details:
Instructions: Tasks:
🎯 REQUIRED OUTPUT FORMAT (MANDATORY): Your response MUST follow this exact structure for clarity: ## ✅ AUTOMATION COMPLETE — [STATUS: READY FOR MERGE / NEEDS WORK / BLOCKED]
**Verified at commit `9641bb9e` (9641bb9e9b52e9ffdb37ac67bcba5ad871fbe612)**
---
## 📊 Current Status: **[PERFECT/ISSUES FOUND]** [✅/⚠️/❌]
| Category | Status |
|----------|--------|
| **CI Checks** | [✅ X/Y PASSING / ❌ FAILING] |
| **Merge Status** | [✅ CLEAN / ❌ CONFLICTS] |
| **PR State** | [✅ MERGEABLE / ❌ BLOCKED] |
| **Comments Addressed** | [✅ ALL DONE / ⚠️ X UNRESOLVED] |
| **Code Quality** | [✅ GOOD / ⚠️ ISSUES] |
---
## 🎯 Comment Resolution Summary
[List each outstanding comment and whether it was addressed - check for code fixes OR consolidated response summary]
---
## 🚀 **FINAL VERDICT: [APPROVE / REQUEST CHANGES / BLOCKED]**
[Brief explanation of decision]
---
🐰 [Sign-off emoji and message]IMPORTANT: Do NOT include raw script execution output or analysis chain details in your response. Only provide the clean formatted summary above. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 62407
✅ AUTOMATION COMPLETE — STATUS: NEEDS WORKVerified at commit 📊 Current Status: ISSUES FOUND
|
| Category | Status |
|---|---|
| CI Checks | |
| Merge Status | ✅ CLEAN — no conflicts |
| PR State | |
| Comments Addressed | ❌ 0 / 18 RESOLVED — no fixes in code and no consolidated response summary found |
| Code Quality | ❌ ISSUES — multiple Major/High severity bugs; docstring coverage 0% (threshold 80%); PR description incomplete |
🎯 Comment Resolution Summary
No comments have been addressed in code, and no [AI responder] Consolidated Comment Response Summary comment was found. All 18 inline review comments remain unresolved:
🔴 High / Major Severity (must fix)
| # | File | Issue | Reviewer | Status |
|---|---|---|---|---|
| 1 | scripts/cmux-mcp-server.mjs L59-62 |
compactArgs silently corrupts CLI args — orphaned flags consume the next flag as their value when optional values are omitted (e.g. --workspace --surface surf:1) |
Cursor Bugbot + CodeRabbit | ❌ Unresolved |
| 2 | scripts/cmux-mcp-server.mjs L135-156 |
runCmux() can stall indefinitely — no timeout on execFileAsync; raw argv (including --socket-password and user input) leaked in thrown errors |
CodeRabbit | ❌ Unresolved |
| 3 | scripts/cmux-mcp-server.mjs L272-277 |
Workspace navigation dispatches wrong CLI verbs — next/previous/last cases emit next-window/previous-window/last-window instead of workspace-scoped commands |
CodeRabbit | ❌ Unresolved |
| 4 | scripts/cmux-mcp-server.mjs L808-865 |
HTTP transport allows non-loopback binds with no auth — --host/CMUX_MCP_HTTP_HOST can publish keystroke injection and screen-read tools on the network |
CodeRabbit | ❌ Unresolved |
| 5 | scripts/cmux_mcp_server.py L66-72 |
Content-Length not validated before read() — non-integer or negative value crashes server with unhandled ValueError |
CodeRabbit + Copilot | ❌ Unresolved |
| 6 | scripts/cmux_mcp_server.py L108-112 |
Server echoes arbitrary client protocolVersion — falsely advertises support for any version string |
CodeRabbit + Cursor Bugbot | ❌ Unresolved |
| 7 | scripts/cmux_mcp_server.py L329-339 |
cmux_create_workspace is not failure-atomic — if workspace.create succeeds but workspace.rename fails, retrying MCP clients create duplicates |
CodeRabbit | ❌ Unresolved |
🟡 Medium / Minor Severity
| # | File | Issue | Reviewer | Status |
|---|---|---|---|---|
| 8 | scripts/cmux_mcp_server.py |
Redundant connect() call in _client() — connect is called before the object is used as a context manager, which is semantically inconsistent |
Copilot | ❌ Unresolved |
| 9 | scripts/cmux_mcp_server.py |
Production server imports from tests_v2/cmux.py — will break if tests_v2/ is absent in deployed environments |
Copilot | ❌ Unresolved |
| 10 | scripts/cmux_mcp_server.py |
cmux_socket_call is an unrestricted API pass-through — no allowlist, no audit logging; grants full cmux control to any MCP client |
Copilot | ❌ Unresolved |
| 11 | scripts/cmux-mcp-server.mjs |
HTTP transport security not documented — unauthenticated local HTTP exposes destructive ops with no warning | Copilot | ❌ Unresolved |
| 12 | scripts/cmux_mcp_server.py |
--socket env var inconsistency — only reads CMUX_SOCKET_PATH; CMUX_SOCKET fallback not reflected in help text or docs |
Copilot | ❌ Unresolved |
| 13 | README.md |
Python MCP server not documented — README only covers the JS server; Python server has a different tool set and no usage docs | Copilot | ❌ Unresolved |
| 14 | docs/cmux-mcp-server.md L11 |
Broken /tmp/ local filesystem path — link to TerminalController.swift references author's local worktree, invalid for all other readers |
CodeRabbit + Copilot | ❌ Unresolved |
| 15 | scripts/cmux_mcp_server.py |
Missing unit tests — no test file for Python or JS MCP server; existing scripts in scripts/ have companion test files |
Copilot | ❌ Unresolved |
⚠️ Pre-merge Check Failures
| # | Check | Status |
|---|---|---|
| 16 | Docstring coverage | ❌ 0.00% — threshold is 80.00% |
| 17 | PR description | ❌ Incomplete — missing Testing section, Demo Video section, and Checklist |
🚀 FINAL VERDICT: REQUEST CHANGES
This PR introduces two substantial new server scripts with several high-severity bugs that have not been addressed: the compactArgs CLI corruption bug affects nearly all tool invocations, the HTTP transport can be bound to non-loopback addresses with no authentication guard, runCmux() can stall indefinitely and leaks sensitive flags in error messages, and the Python server's protocol version handshake is non-compliant. None of the 18 review comments have been acknowledged in code or via a consolidated response summary. The PR description and docstring coverage pre-merge checks are also failing.
Recommended next steps:
- Fix the 7 Major/High items (rows 1–7 above) before merge
- Address or explicitly acknowledge the remaining items (rows 8–15)
- Add a Testing section and complete the PR description checklist
- Add docstrings to meet the 80% threshold (or adjust the threshold if that is intentional)
🐰 Reviewed thoroughly — the rabbit found the bugs before they hopped into production! 🔍
|
[AI responder] Copilot Response - All Comments AddressedCoverage: 19/19 comments addressed (0 fixed, 8 deferred, 6 acknowledged, 1 not done, 4 duplicate refs) CRITICAL — Pending Fix (edit permissions required)
BLOCKING — Pending Fix or Deferred
IMPORTANT
STYLE / DOCUMENTATION
Priority Fix Queue (for next run or manual follow-up)
|
|
@codex fix comments |
|
To use Codex here, create an environment for this repo. |
|
@CodeRabbit all good? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6094
✅ AUTOMATION COMPLETE — STATUS: NEEDS WORKVerified at commit 📊 Current Status: ISSUES FOUND ❌
🎯 What Happened Since Last Review
🔴 Still Outstanding (High/Major — must fix before merge)
🚀 FINAL VERDICT: REQUEST CHANGESThe PR is unchanged. To move forward, the 7 high/major items above need to be fixed in actual code commits. If you'd like, I can open a GitHub issue to track these items, or you can set up a Codex environment and re-invoke 🐰 Almost there — just need those code fixes to land before the rabbit gives the green light! 🔍 |
- compactArgs: omit flag-value pairs when value is empty to avoid orphaned flags corrupting CLI args (fixes Cursor/CodeRabbit P1) - runCmux: redact --socket-password in error messages, add 30s timeout - HTTP transport: guard against non-loopback binds unless CMUX_MCP_ALLOW_REMOTE=1 - Python: validate Content-Length before int(), reject negative/invalid - Python: reject unsupported protocolVersion in initialize - docs: fix TerminalController.swift link to repo-relative path Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/cmux_mcp_server.py (1)
345-355:⚠️ Potential issue | 🟠 Major
cmux_create_workspacestill isn't failure-atomic.If
workspace.createsucceeds andworkspace.renamefails, this returns an error after the workspace already exists, so client retries can create duplicates. Foldtitleinto the initialworkspace.createcall if the backend supports it, or return the createdworkspace_idas partial success instead of failing the whole tool.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cmux_mcp_server.py` around lines 345 - 355, The workspace creation flow is not atomic: if client._call("workspace.create", params) succeeds but client._call("workspace.rename", {...}) fails you currently return an error and lose the created workspace; update cmux_create_workspace to include the title in the initial client._call("workspace.create", params) when title is present (i.e., add the "title" key to params before calling client._call("workspace.create")), falling back only if the backend truly does not accept it; if a rename still fails after create, do not raise a hard error—return the partial success by ensuring create_result includes workspace_id and a non-fatal error field or status (e.g., set create_result["partial_error"] or create_result["status"]="partial") so callers can detect and handle the created workspace instead of retrying.
🧹 Nitpick comments (1)
scripts/cmux_mcp_server.py (1)
15-20: Move the runtimecmuxclient out oftests_v2.This server now depends on a test-only directory and a repo-relative
sys.pathmutation, so it breaks as soon as the script is installed, vendored, or run outside this checkout layout. Promotecmux.pyto a normal runtime module, or colocate a supported client next to this server instead of importing fromtests_v2.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cmux_mcp_server.py` around lines 15 - 20, The server currently imports the runtime client from tests_v2 using a repo-relative sys.path hack (see REPO_ROOT, TESTS_V2_DIR, and the sys.path.insert block) and from cmux import cmux, cmuxError; remove the sys.path mutation and the REPO_ROOT/TESTS_V2_DIR logic, then relocate or promote the client implementation (cmux.py) into the runtime package (or add a small supported client module colocated next to scripts/cmux_mcp_server.py), update the import in cmux_mcp_server.py to a normal package or local import (e.g., from <package_or_local_module> import cmux, cmuxError), and ensure the new module is included in packaging so the server no longer depends on tests_v2.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/cmux_mcp_server.py`:
- Around line 117-146: For the request-handling branches (methods "initialize",
"ping", "tools/list", "tools/call") add a guard that verifies request_id is
present (not None) before performing any action or sending a response; if
request_id is missing, do not execute the request logic (do not call
_initialize_result, _write_response, or _call_tool) and simply return to treat
it as a notification. Update the conditional flow in the relevant blocks inside
the try (around method == "initialize", "ping", "tools/list", and "tools/call")
so that the check for request_id happens first and prevents side effects
(especially the call to _call_tool(name, arguments)) when request_id is absent.
---
Duplicate comments:
In `@scripts/cmux_mcp_server.py`:
- Around line 345-355: The workspace creation flow is not atomic: if
client._call("workspace.create", params) succeeds but
client._call("workspace.rename", {...}) fails you currently return an error and
lose the created workspace; update cmux_create_workspace to include the title in
the initial client._call("workspace.create", params) when title is present
(i.e., add the "title" key to params before calling
client._call("workspace.create")), falling back only if the backend truly does
not accept it; if a rename still fails after create, do not raise a hard
error—return the partial success by ensuring create_result includes workspace_id
and a non-fatal error field or status (e.g., set create_result["partial_error"]
or create_result["status"]="partial") so callers can detect and handle the
created workspace instead of retrying.
---
Nitpick comments:
In `@scripts/cmux_mcp_server.py`:
- Around line 15-20: The server currently imports the runtime client from
tests_v2 using a repo-relative sys.path hack (see REPO_ROOT, TESTS_V2_DIR, and
the sys.path.insert block) and from cmux import cmux, cmuxError; remove the
sys.path mutation and the REPO_ROOT/TESTS_V2_DIR logic, then relocate or promote
the client implementation (cmux.py) into the runtime package (or add a small
supported client module colocated next to scripts/cmux_mcp_server.py), update
the import in cmux_mcp_server.py to a normal package or local import (e.g., from
<package_or_local_module> import cmux, cmuxError), and ensure the new module is
included in packaging so the server no longer depends on tests_v2.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 118ff571-29e1-4680-896b-64b104e83814
📒 Files selected for processing (3)
docs/cmux-mcp-server.mdscripts/cmux-mcp-server.mjsscripts/cmux_mcp_server.py
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/cmux-mcp-server.mjs
|
@CodeRabbit-ai @greptileai @BugBot @copilot Summary (Review Flow):
PR Details:
Instructions: Tasks:
🎯 REQUIRED OUTPUT FORMAT (MANDATORY): Your response MUST follow this exact structure for clarity: ## ✅ AUTOMATION COMPLETE — [STATUS: READY FOR MERGE / NEEDS WORK / BLOCKED]
**Verified at commit `4ed1a2f8` (4ed1a2f8b55dffaf03220a598bcc83f4d2a0f4b6)**
---
## 📊 Current Status: **[PERFECT/ISSUES FOUND]** [✅/⚠️/❌]
| Category | Status |
|----------|--------|
| **CI Checks** | [✅ X/Y PASSING / ❌ FAILING] |
| **Merge Status** | [✅ CLEAN / ❌ CONFLICTS] |
| **PR State** | [✅ MERGEABLE / ❌ BLOCKED] |
| **Comments Addressed** | [✅ ALL DONE / ⚠️ X UNRESOLVED] |
| **Code Quality** | [✅ GOOD / ⚠️ ISSUES] |
---
## 🎯 Comment Resolution Summary
[List each outstanding comment and whether it was addressed - check for code fixes OR consolidated response summary]
---
## 🚀 **FINAL VERDICT: [APPROVE / REQUEST CHANGES / BLOCKED]**
[Brief explanation of decision]
---
🐰 [Sign-off emoji and message]IMPORTANT: Do NOT include raw script execution output or analysis chain details in your response. Only provide the clean formatted summary above. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 18212 🏁 Script executed: Length of output: 18154
✅ AUTOMATION COMPLETE — STATUS: NEEDS WORKVerified at commit 📊 Current Status: ISSUES FOUND
|
| Category | Status |
|---|---|
| CI Checks | |
| Merge Status | ✅ CLEAN — no conflicts |
| PR State | |
| Comments Addressed | |
| Code Quality | ❌ 3 High/Major bugs remain (2 new, 1 carried forward) |
🎯 Comment Resolution Summary
✅ Fixed in code (commit 4ed1a2f8)
| # | Issue | Verification |
|---|---|---|
| 1 | compactArgs orphaned flags |
✅ Now pair-based: skips both flag and value when value is empty/null |
| 2 | runCmux indefinite stall |
✅ timeout: 30_000 added to execFileAsync options |
| 3 | runCmux leaks --socket-password in errors |
✅ redactArgv(argv) applied in the catch block |
| 4 | HTTP non-loopback binding | ✅ loopbackHosts guard + CMUX_MCP_ALLOW_REMOTE=1 opt-in implemented |
| 5 | Python Content-Length crash |
✅ try/except ValueError + negative-value check added |
| 6 | Python echoes arbitrary protocolVersion |
✅ Returns -32602 on version mismatch |
🔴 Still Unresolved / New Bugs Found
| # | Severity | File | Issue | Status |
|---|---|---|---|---|
| 1 | 🔴 High (NEW) | scripts/cmux-mcp-server.mjs L113–124 |
--socket-password leaked in every successful response — formatStructured() includes the raw result.argv array in structuredContent on success. redactArgv was only applied in the catch block (L186), so every successful tool call exposes the plaintext socket password to the MCP client. |
❌ Not fixed |
| 2 | 🔴 High (carried) | scripts/cmux-mcp-server.mjs L304–310 |
Workspace next/previous/last still dispatch window CLI verbs — code still emits next-window, previous-window, last-window for workspace-entity navigation. |
❌ Not fixed |
| 3 | 🟠 Major (NEW) | scripts/cmux_mcp_server.py L117–146 |
Missing request_id guard on request methods — initialize, ping, tools/list, and tools/call execute side effects and write responses even when request_id is None. Per JSON-RPC 2.0 only notifications should omit an ID; request methods must guard on request_id presence before execution. |
❌ Not fixed |
| 4 | 🟠 Major (deferred) | scripts/cmux_mcp_server.py L340–354 |
cmux_create_workspace not failure-atomic — create + rename remain two separate calls; rename failure can leave duplicate workspaces on retry. |
🟡 Medium / Minor (unchanged from previous review)
| # | Issue | Status |
|---|---|---|
| 5 | Redundant connect() in _client() |
|
| 6 | Production server imports from tests_v2/cmux.py via sys.path |
|
| 7 | cmux_socket_call unrestricted passthrough (both servers) |
|
| 8 | --socket env var inconsistency |
|
| 9 | Python MCP server missing from README | |
| 10 | Hardcoded /tmp/ path in docs/cmux-mcp-server.md |
|
| 11 | No unit tests for either MCP server script |
🚀 FINAL VERDICT: REQUEST CHANGES
Good progress — 6 of the original 7 high/major items were addressed in code. However, two new High/Major bugs were identified in this commit cycle: --socket-password is still leaked in every successful tool response (only the error path was redacted), and workspace next/previous/last still dispatch wrong CLI verbs. A third new Major issue was flagged by automated review: JSON-RPC request_id guards are missing for request-type methods in the Python server. These three items need to be fixed before merge.
Recommended next steps:
- Apply
redactArgv()to the success path informatStructured()(or stripargvfrom the structured response entirely) - Fix workspace navigation verbs for
next/previous/last(or remove them if workspace-scoped CLI commands don't exist) - Add
request_id is not Noneguards before executing and responding toinitialize,ping,tools/list, andtools/callin the Python server
🐰 Almost there — three more fixes and this rabbit will be ready to stamp it! 🔍
- formatStructured: redact argv in success path to avoid leaking --socket-password in every tool response (fixes Cursor/CodeRabbit High) - workspace next/previous/last: remove wrong window CLI dispatch; throw clear error instead (workspace cycle not implemented) - Python: add request_id guard for initialize, ping, tools/list, tools/call per JSON-RPC 2.0 (fixes CodeRabbit Major) Made-with: Cursor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because the spend limit has been reached. To enable Bugbot Autofix, raise your spend limit in the Cursor dashboard.
| requireField(input.direction, 'direction'), | ||
| ]), | ||
| options: common, | ||
| }; |
There was a problem hiding this comment.
Surface split passes direction as positional, inconsistent with pane
Medium Severity
In buildControlCommand, the surface→split action passes direction as a bare positional argument after --surface, while the pane→create action passes it as '--direction', value. If the new-split CLI subcommand expects --direction as a named flag (consistent with new-pane), the split command will fail because the direction string is passed as an unrecognized positional argument instead of a flag-value pair.
Additional Locations (1)
There was a problem hiding this comment.
🧹 Nitpick comments (4)
scripts/cmux-mcp-server.mjs (2)
60-79: Design note:compactArgsrequires boolean flags at array end.The function correctly handles flag-value pairs but assumes standalone boolean flags (like
--scrollback) appear at the end of the array. If a boolean flag is followed by another flag, both would be incorrectly grouped as a flag-value pair. Current call sites follow this constraint, but consider adding a brief inline comment at call sites or validating that the next element isn't a flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cmux-mcp-server.mjs` around lines 60 - 79, compactArgs assumes a flag (v) followed by any non-empty next is its value, which misparses when the next is actually another flag; update compactArgs to treat the next element as a value only if it exists, is non-empty, and is not itself a flag (e.g., typeof next === 'string' && !next.startsWith('-')), so that standalone boolean flags followed by other flags are preserved; locate the logic in function compactArgs (variables v and next) and add this additional check, leaving other behavior unchanged.
891-902: Minor: Redundant::1check.Line 896 redundantly checks
bindHost === '::1'when'::1'is already inloopbackHostsandtoLowerCase()doesn't change it. The security guard is correct and effective, but the duplicate check can be removed.🧹 Proposed simplification
const loopbackHosts = new Set(['127.0.0.1', '::1', 'localhost']); const allowRemote = process.env.CMUX_MCP_ALLOW_REMOTE === '1'; const bindHost = config.httpHost; -const isLoopback = - loopbackHosts.has(String(bindHost ?? '').toLowerCase()) || - bindHost === '::1'; +const isLoopback = loopbackHosts.has(String(bindHost ?? '').toLowerCase());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cmux-mcp-server.mjs` around lines 891 - 902, The isLoopback check redundantly tests bindHost === '::1' even though loopbackHosts already contains '::1'; update the condition to remove the duplicate check and rely solely on loopbackHosts.has(String(bindHost ?? '').toLowerCase()) (keeping loopbackHosts, allowRemote, bindHost, and the subsequent error throw unchanged) so the guard remains correct but simpler.scripts/cmux_mcp_server.py (2)
399-408: Acknowledge:cmux_socket_callis an intentional escape hatch.This tool exposes arbitrary cmux v2 socket methods with no server-side validation. The params dict check (line 402-403) is a good defensive measure. Since the Python server only supports stdio transport (no HTTP), the remote exposure risk is lower than the JS server, but consider documenting this tool's security implications in the tool description.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cmux_mcp_server.py` around lines 399 - 408, The _tool_socket_call (cmux_socket_call) function currently allows arbitrary cmux v2 socket methods with only a params type check; keep the existing params validation in _tool_socket_call and explicitly document the security implications by updating the tool description/docstring (referencing cmux_socket_call and _tool_socket_call) to note it is an escape hatch, that only stdio transport is supported, and warn about the lack of server-side validation and recommended usage; optionally mention recommended future mitigations (method allowlist or stricter param schemas) so reviewers know this was considered.
15-20: Acknowledge:sys.pathmanipulation is fragile for packaging.The dynamic path insertion to import from
tests_v2works for development but will break if the package is installed via pip or bundled. This was previously noted as deferred; consider adding a tracking comment or TODO.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cmux_mcp_server.py` around lines 15 - 20, Add a short TODO comment above the dynamic path manipulation that references REPO_ROOT and TESTS_V2_DIR and notes that inserting into sys.path (the sys.path.insert(0, str(TESTS_V2_DIR)) block) is fragile for packaging and should be replaced with proper package imports or resource-based loading when this module is packaged/installed; leave the existing logic intact but add the tracking comment so future maintainers see this is deferred technical debt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/cmux_mcp_server.py`:
- Around line 399-408: The _tool_socket_call (cmux_socket_call) function
currently allows arbitrary cmux v2 socket methods with only a params type check;
keep the existing params validation in _tool_socket_call and explicitly document
the security implications by updating the tool description/docstring
(referencing cmux_socket_call and _tool_socket_call) to note it is an escape
hatch, that only stdio transport is supported, and warn about the lack of
server-side validation and recommended usage; optionally mention recommended
future mitigations (method allowlist or stricter param schemas) so reviewers
know this was considered.
- Around line 15-20: Add a short TODO comment above the dynamic path
manipulation that references REPO_ROOT and TESTS_V2_DIR and notes that inserting
into sys.path (the sys.path.insert(0, str(TESTS_V2_DIR)) block) is fragile for
packaging and should be replaced with proper package imports or resource-based
loading when this module is packaged/installed; leave the existing logic intact
but add the tracking comment so future maintainers see this is deferred
technical debt.
In `@scripts/cmux-mcp-server.mjs`:
- Around line 60-79: compactArgs assumes a flag (v) followed by any non-empty
next is its value, which misparses when the next is actually another flag;
update compactArgs to treat the next element as a value only if it exists, is
non-empty, and is not itself a flag (e.g., typeof next === 'string' &&
!next.startsWith('-')), so that standalone boolean flags followed by other flags
are preserved; locate the logic in function compactArgs (variables v and next)
and add this additional check, leaving other behavior unchanged.
- Around line 891-902: The isLoopback check redundantly tests bindHost === '::1'
even though loopbackHosts already contains '::1'; update the condition to remove
the duplicate check and rely solely on loopbackHosts.has(String(bindHost ??
'').toLowerCase()) (keeping loopbackHosts, allowRemote, bindHost, and the
subsequent error throw unchanged) so the guard remains correct but simpler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a26a7695-5721-4a50-92aa-e7c30797112b
📒 Files selected for processing (2)
scripts/cmux-mcp-server.mjsscripts/cmux_mcp_server.py
|
@CodeRabbit-ai @greptileai @BugBot @copilot Summary (Review Flow):
PR Details:
Instructions: Tasks:
🎯 REQUIRED OUTPUT FORMAT (MANDATORY): Your response MUST follow this exact structure for clarity: ## ✅ AUTOMATION COMPLETE — [STATUS: READY FOR MERGE / NEEDS WORK / BLOCKED]
**Verified at commit `a034df74` (a034df74464c9d32556e837fadec4fe47d9ee998)**
---
## 📊 Current Status: **[PERFECT/ISSUES FOUND]** [✅/⚠️/❌]
| Category | Status |
|----------|--------|
| **CI Checks** | [✅ X/Y PASSING / ❌ FAILING] |
| **Merge Status** | [✅ CLEAN / ❌ CONFLICTS] |
| **PR State** | [✅ MERGEABLE / ❌ BLOCKED] |
| **Comments Addressed** | [✅ ALL DONE / ⚠️ X UNRESOLVED] |
| **Code Quality** | [✅ GOOD / ⚠️ ISSUES] |
---
## 🎯 Comment Resolution Summary
[List each outstanding comment and whether it was addressed - check for code fixes OR consolidated response summary]
---
## 🚀 **FINAL VERDICT: [APPROVE / REQUEST CHANGES / BLOCKED]**
[Brief explanation of decision]
---
🐰 [Sign-off emoji and message]IMPORTANT: Do NOT include raw script execution output or analysis chain details in your response. Only provide the clean formatted summary above. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 15323
✅ AUTOMATION COMPLETE — STATUS: READY FOR MERGEVerified at commit 📊 Current Status: ISSUES FOUND (minor only)
|
| Category | Status |
|---|---|
| CI Checks | |
| Merge Status | ✅ CLEAN — no conflicts |
| PR State | ✅ MERGEABLE — no blocking items |
| Comments Addressed | ✅ All High/Major fixed; |
| Code Quality | ✅ All High/Major bugs resolved; minor items remain |
🎯 Comment Resolution Summary
✅ Fixed Across Commits (4ed1a2f8 + a034df74)
| # | Issue | Verification |
|---|---|---|
| 1 | compactArgs orphaned flags |
✅ Pair-based skipping implemented |
| 2 | runCmux indefinite stall |
✅ timeout: 30_000 confirmed present |
| 3 | runCmux error-path leaks --socket-password |
✅ redactArgv(argv) in catch block |
| 4 | NEW formatStructured success-path leaks --socket-password |
✅ redactArgv(result.argv ?? []) now applied in formatStructured() |
| 5 | HTTP non-loopback binding with no auth | ✅ loopbackHosts guard + CMUX_MCP_ALLOW_REMOTE=1 opt-in confirmed |
| 6 | Python Content-Length crash on bad headers |
✅ try/except ValueError + negative-value check confirmed |
| 7 | Python server echoes arbitrary protocolVersion |
✅ Returns -32602 on version mismatch confirmed |
| 8 | NEW Workspace next/previous/last dispatch wrong CLI verbs |
✅ Now throws clear Error — workspace cycle not implemented |
| 9 | NEW Python request_id guards missing on request methods |
✅ All four methods (initialize, ping, tools/list, tools/call) guard on request_id is None |
🟡 Medium / Minor — Deferred or Acknowledged (non-blocking)
| # | File | Issue | Status |
|---|---|---|---|
| 1 | scripts/cmux_mcp_server.py L340–354 |
cmux_create_workspace not failure-atomic |
|
| 2 | scripts/cmux_mcp_server.py |
Redundant connect() in _client() |
|
| 3 | scripts/cmux_mcp_server.py L15–20 |
Production imports from tests_v2/cmux.py via sys.path |
|
| 4 | Both servers | cmux_socket_call unrestricted passthrough |
|
| 5 | scripts/cmux_mcp_server.py L428 |
--socket env var inconsistency |
|
| 6 | README.md L161 |
Python MCP server missing from README | |
| 7 | docs/cmux-mcp-server.md |
Hardcoded /tmp/ local path |
|
| 8 | Both servers | No unit tests | |
| 9 | scripts/cmux-mcp-server.mjs L387 |
NEW Surface split passes direction as positional arg, inconsistent with pane split |
🚀 FINAL VERDICT: APPROVE
All 9 High/Major bugs identified across the review cycle have been confirmed fixed in code at commit a034df74. The three items called out in the previous review — success-path --socket-password leakage, wrong workspace navigation CLI dispatch, and missing Python request_id guards — are all verifiably resolved. The remaining 9 items are Medium/Minor severity, explicitly deferred or acknowledged as architectural concerns, and are appropriate for follow-up issues rather than blocking this PR. CI is clean (CodeRabbit pass; Cursor Bugbot is skipping due to spend limit, not a failure). The branch is conflict-free and mergeable.
One follow-up recommended: Open a tracking issue for the 9 deferred Medium/Minor items (especially the tests_v2 import fragility and missing unit tests) to ensure they are addressed post-merge.
🐰 All the critical bugs have been squashed — this rabbit is stamping it approved! 🎉
[approve]
|
@CodeRabbit all good? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1711
The PR is in the same state as my last review at commit
The PR is approved and ready to merge. The 9 remaining Medium/Minor items (e.g., 🐰 Green light — hop to it! 🎉 [approve] |
… polish - #4 (toggle for agents launched mid-session): the Mac now adopts a detected agent the instant its terminal title becomes the agent's (AgentChatTranscriptService observes .ghosttyDidSetTitle and calls TerminalController.adoptDetectedAgentSessions), so the session registers and pushes to the phone live, not only on next open. - #1 (scroll-to-bottom no longer dismisses keyboard): the dismiss tap excludes the button's frame (ChatScrollButtonFramePreferenceKey + excludedRegion). - #3 (smooth scroll): the button does a single animated proxy.scrollTo to the bottom anchor instead of stacked non-animated jumps. - #5 (toggle eases in): the session-list update is wrapped in withAnimation. - #2 (cramped grouping): intraGroupSpacing 2 -> 5 so a code block isn't flush against the next message bubble. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Changes
scripts/cmux_mcp_server.py(Python MCP server)scripts/cmux-mcp-server.mjs(JavaScript MCP server)Testing
node scripts/cmux-mcp-server.mjsorpython scripts/cmux_mcp_server.pyKnown Limitations
Copilot Review Tracking
Last run: 2026-03-08 | Total comments: 19 | New this run: 19 | Carried forward: 0
runCmuxerror at mjs:154-156 joins raw argv including--socket-passwordvalue into the error message, leaking credentials to HTTP logs and MCP clients. Fix: redact password value before argv.join. Deferred — edit permissions not granted this run; ~3-line fix ready for next run.httpServer.listen(config.httpPort, config.httpHost)at mjs:861 accepts any host including non-loopback, exposing terminal control/screen-read endpoints. Fix: validate host is loopback or require CMUX_MCP_ALLOW_REMOTE=1 opt-in. Deferred — edit permissions not granted; ~4-line fix ready.compactArgsat mjs:60-62 filters undefined values individually from interleaved flag-value pairs, leaving orphaned flags when optional middle values are absent. Mixed usage (standalone flags like --scrollback at line 458) makes a simple pair-based fix unsafe; safe fix requires refactoring 10+ call sites. Recommend follow-up issue.int(content_length)at py:70 crashes with ValueError on non-integer headers and reads until EOF on -1. Fix: wrap in try/except ValueError, validate >= 0. Exact 8-line fix prepared; deferred — edit permissions not granted.Note
Medium Risk
Introduces a new automation/control surface (including optional HTTP transport) that can read screens and send input; while loopback binding is guarded, misuse or misconfiguration could impact local security and reliability.
Overview
Adds an initial local MCP server layer so external agents can control cmux programmatically, exposing discovery (
cmux_identify,cmux_tree,cmux_list), UI steering (cmux_controlfor windows/workspaces/panes/surfaces), terminal I/O (cmux_terminal), and common browser actions (cmux_browser) by delegating to the existingcmuxCLI.Introduces a Node implementation (
scripts/cmux-mcp-server.mjs) with stdio and optional HTTP transport (loopback-only unlessCMUX_MCP_ALLOW_REMOTE=1), plus a minimal Python stdio adapter (scripts/cmux_mcp_server.py) over the v2 socket API; updatespackage.json/lockfile for MCP dependencies and documents usage and environment variables inREADME.md(with additional design notes indocs/cmux-mcp-server.md).Written by Cursor Bugbot for commit a034df7. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Documentation