Repository navigation
Conversation
Document build process for cmux MCP Server including: - Prerequisites (Xcode, zig, xcodeproj) - Build steps for cmux-cli and full app - How to add new MCP files to Xcode project - Usage and configuration - Troubleshooting guide - Available MCP tools list - File structure overview Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add Model Context Protocol server to cmux CLI for AI agent integration: - MCPTypes: JSON-RPC 2.0 and MCP protocol types - MCPProtocol: Protocol handler (initialize, tools/list, tools/call) - MCPToolRegistry: Tool registration and execution (12 tools) - MCPBackend: cmux daemon communication via Unix Socket - MCPMain: stdio entry point - CLI integration: --mcp flag Also add development guide to README.md: - Prerequisites (Xcode, zig, xcodeproj) - Build steps (CLI only vs full app) - How to add new code to Xcode project - Running and testing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Design documents for the socket-direct RPC approach and tool bugfix/testing strategy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace Process()-based command execution with direct Unix domain socket communication using POSIX syscalls. This eliminates CLI flag incompatibilities and subprocess overhead. Key changes: - Persistent socket connection with auto-reconnect - POSIX read/write instead of FileHandle (avoids macOS buffering issues) - Thread-safe via NSLock - Authentication via socket RPC instead of CLI flags - Convenience methods: rpc(), rpcJSON(), rpcForTool() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace AnyCodable-based result serialization with pre-encoded Data to avoid encoding issues with nested dynamic types. The result is now encoded at construction time and embedded as raw JSON during response serialization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MCPProtocol: Change processMessage/handleRequest/encodeResponse to throw instead of silently swallowing encoding errors. MCPMain: Replace custom readLine() with Swift.readLine(), add proper JSON-RPC error responses for processing failures, remove unused buffer allocation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace 12 individual tool classes (IdentifyTool, ListWorkspacesTool, etc.) with 8 grouped tools (SystemTool, WorkspaceTool, WindowTool, PaneTool, SurfaceTool, NotificationTool, TabTool, BrowserTool). Each grouped tool uses an action parameter to dispatch to the correct socket RPC method, with strict action whitelisting and per-action parameter validation via ActionDef. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Python test scripts for validating the MCP server JSON-RPC protocol and the direct socket RPC communication layer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@Efan404 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds a built-in MCP server: POSIX Unix-socket backend, JSON‑RPC 2.0 protocol handler, typed MCP types, tool registry with eight tools, CLI integration (--mcp), stdio entrypoint, tests, docs, and Xcode project/package updates to compile and expose the new components. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant MCPServer as MCP Server\n(MCPMain/MCPProtocol)
participant ToolRegistry as Tool Registry
participant MCPBackend as MCP Backend
participant Daemon as cmux Daemon
Client->>MCPServer: JSON-RPC initialize (stdin)
MCPServer->>MCPServer: processMessage()
MCPServer->>Client: JSON-RPC response (stdout)
Client->>MCPServer: JSON-RPC tools/list (stdin)
MCPServer->>ToolRegistry: listToolDefinitions()
ToolRegistry-->>MCPServer: [MCPToolDefinition]
MCPServer->>Client: JSON-RPC response (stdout)
Client->>MCPServer: JSON-RPC tools/call (stdin)
MCPServer->>ToolRegistry: executeTool(name,args)
ToolRegistry->>MCPBackend: rpcForTool(method,params)
MCPBackend->>MCPBackend: ensureConnected()
MCPBackend->>Daemon: write JSON over Unix socket
Daemon-->>MCPBackend: read response
MCPBackend-->>ToolRegistry: MCPToolCallResult
ToolRegistry-->>MCPServer: result
MCPServer->>Client: JSON-RPC response (stdout)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review this PR |
Greptile SummaryThis PR replaces the MCP server's CLI subprocess spawning approach with direct Unix domain socket communication, consolidates 12 individual tools into 8 grouped action-based tools, and fixes JSONRPCResponse serialization issues. Key Changes:
The architecture is well-designed with good separation of concerns. The implementation includes proper connection management (auto-reconnect with one retry, thread safety via Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as Claude Code<br/>(MCP Client)
participant Server as cmux MCP Server<br/>(MCPProtocol)
participant Backend as MCPBackend
participant Socket as cmux daemon<br/>(Unix Socket)
Note over Client,Socket: Initialization Flow
Client->>Server: initialize (JSON-RPC 2.0)
Server->>Client: capabilities + serverInfo
Client->>Server: initialized (notification)
Note over Client,Socket: Tool Discovery
Client->>Server: tools/list
Server->>Client: 8 grouped tools<br/>(system, workspace, window,<br/>pane, surface, notification,<br/>tab, browser)
Note over Client,Socket: Tool Execution (NEW: Direct Socket RPC)
Client->>Server: tools/call<br/>{name: "cmux_surface",<br/>arguments: {action: "read_text"}}
Server->>Backend: executeTool()
Note over Backend: Validate action params<br/>(ActionDef validation)
Backend->>Socket: RPC: surface.read_text<br/>(persistent connection)
Socket-->>Backend: {ok: true, result: {...}}
Backend->>Server: MCPToolCallResult
Server->>Client: JSON-RPC result
Note over Client,Socket: Auto-Reconnect on Failure
Client->>Server: tools/call
Server->>Backend: executeTool()
Backend->>Socket: RPC (connection dead)
Note over Backend: Detect failure,<br/>disconnect & reconnect
Backend->>Socket: RPC retry
Socket-->>Backend: {ok: true, result: {...}}
Backend->>Server: MCPToolCallResult
Server->>Client: JSON-RPC result
Last reviewed commit: 5848480 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Write debug messages to stderr | ||
| let stderr = FileHandle.standardError | ||
| let debugMsg = "cmux MCP Server starting...\n" | ||
| stderr.write(debugMsg.data(using: .utf8)!) |
There was a problem hiding this comment.
force unwrap could crash if UTF-8 encoding fails
| stderr.write(debugMsg.data(using: .utf8)!) | |
| stderr.write((debugMsg.data(using: .utf8) ?? Data())) |
There was a problem hiding this comment.
Fixed — replaced force unwrap with ?? Data(). See commit d8ca8d2.
![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: 5848480a95
ℹ️ 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".
| "previous": ActionDef(), | ||
| "last": ActionDef(), | ||
| "reorder": ActionDef(required: ["workspace_id", "index"]), | ||
| "action": ActionDef(required: ["workspace_id", "action_name"]), |
There was a problem hiding this comment.
action param to action RPC methods
GroupedTool.execute always strips the top-level action key before building RPC params, so entries like this one that require action_name end up calling workspace.action/surface.action/tab.action without an action field at all. The socket handlers (v2WorkspaceAction and v2TabAction in Sources/TerminalController.swift) explicitly require params.action and return invalid_params when it is missing, so these MCP actions are currently unusable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — action_name is now remapped to action for *.action RPC methods in GroupedTool.execute(). See commit d8ca8d2.
| private func connect() throws { | ||
| disconnect() | ||
|
|
||
| let fd = socket(AF_UNIX, SOCK_STREAM, 0) |
There was a problem hiding this comment.
This backend now connects directly to any socket path without checking file ownership, unlike SocketClient.connect in CLI/cmux.swift which guards st_uid == getuid() to prevent fake-socket attacks. With the default /tmp/cmux.sock, a different local user can pre-create a socket and receive MCP traffic (including optional auth password), so the new transport regresses a previously enforced security boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — socket ownership validation (stat + st_uid == getuid()) added in MCPBackend.connect(). See commit 3c3f3bc.
| let result = MCPInitializeResult( | ||
| protocolVersion: protocolVersion, | ||
| capabilities: capabilities, |
There was a problem hiding this comment.
The initialize handler marks the session initialized and echoes back whatever params.protocolVersion the client sent, while the file defines a fixed supported version constant (mcpProtocolVersion). That means an unsupported client version is accepted and advertised as if supported, which breaks version negotiation and can lead to hard-to-diagnose protocol mismatches later in the session.
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: 17
🧹 Nitpick comments (3)
docs/plans/2026-03-03-mcp-server-redesign.md (1)
106-113: Consider adding an “as-built deviations” note for plan accuracy.The “Files to Change” table is now partially stale versus implementation. A short postscript noting what changed from plan to execution would reduce future confusion when using this plan as reference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-03-mcp-server-redesign.md` around lines 106 - 113, Add a short "As-built deviations" postscript to the plan that lists which files ended up different from the original plan and why—specifically note that CLI/MCPBackend.swift was rewritten to use a socket connection instead of Process spawn and that CLI/MCPToolRegistry.swift consolidated ~13 tool classes into ~10 grouped classes, while CLI/MCPProtocol.swift, CLI/MCPTypes.swift, and CLI/MCPMain.swift remained unchanged; place this note near the end of the document (after the "Files to Change" table) and keep it brief, dated, and written in present tense to clarify plan vs actual implementation for future readers.README.md (1)
251-261: Prefer documenting the canonical setup script in Initial Setup.The current steps are valid but omit the project’s standard bootstrap command. Please add
./scripts/setup.shas the primary path (and keep manual steps as fallback).Based on learnings: Run the setup script to initialize submodules and build GhosttyKit:
./scripts/setup.sh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 251 - 261, Update the "Initial Setup" section to present ./scripts/setup.sh as the canonical bootstrap command: add a top-line instruction like "Run ./scripts/setup.sh to initialize submodules and build GhosttyKit" and then keep the existing manual steps (git submodule update --init --recursive and the zig build commands) as a fallback subsection; reference the script path ./scripts/setup.sh and the "Initial Setup" heading so reviewers can locate and prefer the automated path while preserving the manual commands for troubleshooting.CLI/MCPToolRegistry.swift (1)
335-340: Avoid issuingsurface.read_texttwice on fallback.If
textis missing, fallback currently triggers a second RPC call. Reuse the firstresultpayload instead.💡 Suggested refactor
let result = try backend.rpc(method: "surface.read_text", params: params) if let text = result["text"] as? String { return MCPToolCallResult(content: [.text(text)]) } - return try backend.rpcForTool(method: "surface.read_text", params: params) + let data = try JSONSerialization.data(withJSONObject: result, options: [.sortedKeys, .prettyPrinted]) + let formatted = String(data: data, encoding: .utf8) ?? "{}" + return MCPToolCallResult(content: [.text(formatted)]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/MCPToolRegistry.swift` around lines 335 - 340, The current flow calls backend.rpc(method: "surface.read_text", ...) then, if result["text"] is missing, re-invokes backend.rpcForTool(method: "surface.read_text", ...), causing a duplicate RPC; instead reuse the first result payload: after calling backend.rpc(...) inspect the returned `result` and either construct and return an MCPToolCallResult from that `result` (e.g., using whatever keys other than "text" are expected) or modify/create an overload of backend.rpcForTool to accept the initial `result` (e.g., a parameter like initialResult: [String: Any]) and call that so no second network call is made; update the logic around MCPToolCallResult creation to consume the original `result` when "text" is absent rather than calling backend.rpcForTool(...) again.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 671-675: The MCP server path passes socketPasswordArg directly to
runMCPServer, bypassing SocketPasswordResolver.resolve(explicit:); fix by
resolving the effective password first (call
SocketPasswordResolver.resolve(explicit: socketPasswordArg) or the equivalent
resolver used elsewhere) and pass that resolved value into
runMCPServer(socketPath:password:idFormat:), ensuring MCP mode uses the same
password resolution logic as other modes (refer to runMCPServer,
SocketPasswordResolver.resolve(explicit:), and socketPasswordArg).
In `@CLI/MCPBackend.swift`:
- Around line 187-196: The current unconditional reconnect-and-retry in the
do/catch around ensureConnected()/sendRPC(method:params:) risks duplicating
non-idempotent mutations; change the retry logic so that sendRPC is retried only
when safe: add an explicit retry flag or idempotency check (e.g., a new
parameter allowRetry: Bool or isIdempotent(method: String)) and use it in the
catch block to decide whether to call disconnect(); ensureConnected();
sendRPC(...) again; otherwise rethrow the caught error. Update callers of
sendRPC or the wrapper so mutating RPCs call with allowRetry=false (or are not
listed as idempotent) and read-only RPCs can permit the automatic
reconnect-and-retry.
- Around line 57-63: The connect() function opens a UNIX socket without checking
the filesystem socket's ownership/permissions, which allows a malicious socket
to intercept traffic; before creating or connecting the socket in connect() (and
after calling disconnect()) stat the target socket path (the sockaddr_un path
used by connect()), verify it exists, S_ISSOCK(pathStat.st_mode) is true, and
that the UID/GID and file mode match expected values (e.g., owned by the daemon
user/root and not world-writable), and if the checks fail throw
MCPError.executionFailed; also ensure you handle symlinks (lstat) and fail if
the socket path is a symlink or ownership/mode are unsafe so the subsequent
socket/connect calls only proceed when the socket file is trusted.
- Around line 45-48: The initializer for MCPBackend currently accepts idFormat
but doesn't store or use it; either remove the idFormat parameter from public
init or persist it as a property (e.g., private let idFormat: String) and apply
it when building RPC responses (where IDs are formatted) so backend respects
--id-format; update the MCPBackend init signature and the code paths that
serialize/return IDs (search for MCPBackend.init and methods that construct RPC
response payloads) to use the stored idFormat value when formatting IDs.
In `@CLI/MCPMain.swift`:
- Around line 9-12: The default socketPath argument in runMCPServer is hardcoded
to "/tmp/cmux.sock", causing MCP mode to bypass the shared socket-path resolver;
change runMCPServer's default to use the common/shared resolver function (the
same utility used by the rest of the CLI) instead of the literal
"/tmp/cmux.sock" and ensure MCPBackend is constructed with that resolved path
(update the runMCPServer declaration and any other occurrence constructing
MCPBackend in this file, e.g., the other MCP startup call that currently passes
"/tmp/cmux.sock").
- Around line 45-55: The fallback error response hardcodes id .number(0) so
clients can't correlate errors; update the catch to use the original request ID
(e.g., the variable holding the parsed request's id — look for the parsed
request or parameter named id/request/idValue used before calling
processMessage) when building JSONRPCErrorResponse (JSONRPCErrorResponse(id:
...)), and only fall back to .null (or another appropriate JSONRPCID) if that
original id is absent; ensure the id type matches the JSONRPCID enum used in
JSONRPCErrorResponse so the response preserves the request ID.
In `@CLI/MCPProtocol.swift`:
- Around line 84-105: In handleInitialize, validate the incoming protocolVersion
against the supported mcpProtocolVersion instead of accepting any value: after
extracting protocolVersion from request.params, compare it to the expected
mcpProtocolVersion and if it does not match, return a JSONRPCErrorResponse (use
the same JSONRPCError/JSONRPCErrorResponse types used elsewhere) with an
appropriate error code/message indicating unsupported/incompatible protocol
version; only set isInitialized = true and build the MCPInitializeResult
(protocolVersion, capabilities) when the version matches. Ensure you reference
the mcpProtocolVersion constant and the MCPInitializeResult construction so the
flow rejects unsupported versions early.
In `@docs/IMPLEMENTATION.md`:
- Around line 11-25: Update docs/IMPLEMENTATION.md to reflect the current MCP
architecture: remove the outdated claim that cmux.swift is unchanged and that
there are 12 separate tool files; instead describe that MCP is enabled via the
--mcp flag in CLI/cmux.swift and that tools are now consolidated into
grouped/action-based tool modules (e.g., replace individual Tool files with
grouped modules like Identify/List/IO/Pane actions within MCPTools), and update
mentions of MCPMain.swift, MCPProtocol.swift, MCPToolRegistry.swift and
MCPBackend.swift to show the new consolidated structure and startup flow so the
diagram and text match the refactor.
- Around line 9-25: Add the "text" language identifier to the unlabeled fenced
code blocks in the IMPLEMENTATION.md file that render the CLI/MCP directory
trees (the opening triple-backtick lines for the block containing "CLI/ ├──
cmux.swift ..." and the later block around "MCPMain.swift ...
MCPProtocol.swift"); replace the bare ``` with ```text for those blocks
(including the second occurrence noted around lines 92-100) and keep the closing
``` unchanged so markdownlint MD040 is satisfied.
In `@docs/MCP-BUILD.md`:
- Around line 137-153: Update the MCP Tools list to reflect the new
grouped/action-based surface names instead of legacy command tools: replace
deprecated entries like cmux_identify, cmux_read_screen, cmux_send_input,
cmux_send_key, cmux_create_split, cmux_focus_pane, cmux_new_workspace,
cmux_trigger_flash, cmux_list_windows, cmux_list_panes, cmux_list_pane_surfaces
with the grouped surfaces and actions introduced in this PR (e.g., cmux_system,
cmux_workspace, cmux_window and their documented sub-actions), update
descriptions to match the new action names and behavior, and remove or mark
legacy commands as deprecated so integrators and tests reference
cmux_system/cmux_workspace/cmux_window instead of the old cmux_* command-style
names.
- Around line 53-55: Update the two unlabeled fenced code blocks to include a
language tag so markdownlint MD040 is satisfied: change the opening ``` to
```text for the block containing the path string
"~/Library/Developer/Xcode/DerivedData/GhosttyTabs-*/Build/Products/Debug/cmux"
and for the later multi-line fenced block (the longer snippet near the end of
the document) so both are marked as text code blocks.
In `@docs/MCP-SERVER.md`:
- Around line 19-20: Add explicit language identifiers to the unlabeled fenced
code blocks currently shown around the ASCII art and the code snippet (the
triple-backtick blocks at the top of the file and the block around line 66) by
changing ``` to ```text or ```bash/```yaml as appropriate, and update the phrase
"cmux specific error" to the hyphenated "cmux-specific error" where it appears
(search for the exact phrase to locate the occurrence). Ensure the fenced blocks
use the correct language token for markdownlint and the wording change is
applied exactly to the string inside the document.
- Around line 164-177: Update the documentation to reflect the current MCP code
layout and RPC flow: remove the stale claim about reusing SocketClient and the
CLI/cmux.swift structure (references to SocketClient, cmux.swift,
MCPServer.swift, MCPTools.swift, MCPMain.swift) and replace with the current
components and flow—MCPBackend.swift, MCPProtocol.swift, MCPToolRegistry.swift,
MCPTypes.swift, and MCPMain.swift—and note that the implementation uses direct
POSIX socket RPC rather than reusing the CLI's SocketClient; ensure the Project
Structure block lists the current filenames and briefly mentions POSIX socket
RPC over stdio.
In `@docs/plans/2026-03-03-mcp-tool-bugfix-and-testing-design.md`:
- Around line 18-31: The plan's tool inventory and tests still expect 12
individual tools (IdentifyTool, ListWorkspacesTool, ListPanesTool,
ListPaneSurfacesTool, ReadScreenTool, SendInputTool, SendKeyTool,
CreateSplitTool, FocusPaneTool, NewWorkspaceTool, TriggerFlashTool,
ListWindowsTool) and validate tools/list for 12 entries; update the document and
any test assertions to reflect the current grouped action-based tool model by
removing the hardcoded 12-item list, replacing it with the new grouped action
names (or the canonical output shape of tools/list), and adjust the validation
logic that asserts tools/list length and identities so it matches the grouped
responses (also update any related references noted around the same section
previously indicated).
In `@tests/test_mcp_server.py`:
- Around line 124-144: The comprehension that builds names = {t["name"] for t in
tools} can raise KeyError before you validate tool fields; instead, first
validate each tool in tools for required keys
("name","description","inputSchema") using the existing loop (the has_fields
check and check(...) call), and only after that build the names set (or use
t.get("name") guarded by the validation) so the test fails with a proper check
message rather than an exception; reference the variables/tools loop,
has_fields, check(...) and the t["name"] usage when making the change.
- Around line 199-244: The test silently skips assertions when the server
returns fewer than 2 responses; ensure each scenario in test_action_validation
asserts that responses has length >= 2 before inspecting responses[1]. For each
block using mcp_session(...) then "if len(responses) >= 2: ...", replace that
conditional with an explicit assertion/check (e.g., call check or assert) that
len(responses) >= 2 with a clear message (reference symbols: mcp_session,
responses, check, init_msg, tools_call_msg) so the test fails immediately if the
server dropped the response rather than silently passing.
In `@tests/test_mcp_socket_rpc.py`:
- Around line 33-45: The socket in the rpc test is created without a context
manager which can leak file descriptors if an exception occurs; change the code
in tests/test_mcp_socket_rpc.py to use a context manager (e.g., with
socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as sock:) and perform
sock.settimeout(...), sock.connect(...), sock.sendall(...), and the recv loop
inside that with-block so the socket is always closed even on errors; update any
references to the variable accordingly within the same block.
---
Nitpick comments:
In `@CLI/MCPToolRegistry.swift`:
- Around line 335-340: The current flow calls backend.rpc(method:
"surface.read_text", ...) then, if result["text"] is missing, re-invokes
backend.rpcForTool(method: "surface.read_text", ...), causing a duplicate RPC;
instead reuse the first result payload: after calling backend.rpc(...) inspect
the returned `result` and either construct and return an MCPToolCallResult from
that `result` (e.g., using whatever keys other than "text" are expected) or
modify/create an overload of backend.rpcForTool to accept the initial `result`
(e.g., a parameter like initialResult: [String: Any]) and call that so no second
network call is made; update the logic around MCPToolCallResult creation to
consume the original `result` when "text" is absent rather than calling
backend.rpcForTool(...) again.
In `@docs/plans/2026-03-03-mcp-server-redesign.md`:
- Around line 106-113: Add a short "As-built deviations" postscript to the plan
that lists which files ended up different from the original plan and
why—specifically note that CLI/MCPBackend.swift was rewritten to use a socket
connection instead of Process spawn and that CLI/MCPToolRegistry.swift
consolidated ~13 tool classes into ~10 grouped classes, while
CLI/MCPProtocol.swift, CLI/MCPTypes.swift, and CLI/MCPMain.swift remained
unchanged; place this note near the end of the document (after the "Files to
Change" table) and keep it brief, dated, and written in present tense to clarify
plan vs actual implementation for future readers.
In `@README.md`:
- Around line 251-261: Update the "Initial Setup" section to present
./scripts/setup.sh as the canonical bootstrap command: add a top-line
instruction like "Run ./scripts/setup.sh to initialize submodules and build
GhosttyKit" and then keep the existing manual steps (git submodule update --init
--recursive and the zig build commands) as a fallback subsection; reference the
script path ./scripts/setup.sh and the "Initial Setup" heading so reviewers can
locate and prefer the automated path while preserving the manual commands for
troubleshooting.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CLI/MCPBackend.swiftCLI/MCPMain.swiftCLI/MCPProtocol.swiftCLI/MCPToolRegistry.swiftCLI/MCPTypes.swiftCLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojREADME.mddocs/IMPLEMENTATION.mddocs/MCP-BUILD.mddocs/MCP-SERVER.mddocs/plans/2026-03-03-mcp-server-redesign.mddocs/plans/2026-03-03-mcp-tool-bugfix-and-testing-design.mdtests/test_mcp_server.pytests/test_mcp_socket_rpc.py
| public func runMCPServer(socketPath: String = "/tmp/cmux.sock", password: String? = nil, idFormat: String = "refs") { | ||
| // Initialize backend | ||
| let backend = MCPBackend(socketPath: socketPath, password: password, idFormat: idFormat) | ||
|
|
There was a problem hiding this comment.
Use the shared socket-path resolver instead of hardcoded /tmp/cmux.sock.
Current defaults can diverge from normal socket selection (environment overrides/debug paths), causing MCP mode to connect to the wrong daemon unless --socket is always provided.
💡 Suggested fix
-public func runMCPServer(socketPath: String = "/tmp/cmux.sock", password: String? = nil, idFormat: String = "refs") {
+public func runMCPServer(
+ socketPath: String = SocketControlSettings.socketPath(),
+ password: String? = nil,
+ idFormat: String = "refs"
+) {
@@
- var socketPath = "/tmp/cmux.sock"
+ var socketPath = SocketControlSettings.socketPath()Also applies to: 71-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/MCPMain.swift` around lines 9 - 12, The default socketPath argument in
runMCPServer is hardcoded to "/tmp/cmux.sock", causing MCP mode to bypass the
shared socket-path resolver; change runMCPServer's default to use the
common/shared resolver function (the same utility used by the rest of the CLI)
instead of the literal "/tmp/cmux.sock" and ensure MCPBackend is constructed
with that resolved path (update the runMCPServer declaration and any other
occurrence constructing MCPBackend in this file, e.g., the other MCP startup
call that currently passes "/tmp/cmux.sock").
| names = {t["name"] for t in tools} | ||
| expected_names = { | ||
| "cmux_system", | ||
| "cmux_workspace", | ||
| "cmux_window", | ||
| "cmux_pane", | ||
| "cmux_surface", | ||
| "cmux_notification", | ||
| "cmux_tab", | ||
| "cmux_browser", | ||
| } | ||
| check("all tool names present", names == expected_names, | ||
| f"missing={expected_names - names}, extra={names - expected_names}") | ||
|
|
||
| # Each tool should have name, description, inputSchema | ||
| for t in tools: | ||
| has_fields = all(k in t for k in ("name", "description", "inputSchema")) | ||
| if not has_fields: | ||
| check(f"tool {t.get('name')} has required fields", False, str(t.keys())) | ||
| return | ||
| check("all tools have name/description/inputSchema", True) |
There was a problem hiding this comment.
Validate tool fields before indexing t["name"].
names = {t["name"] for t in tools} runs before the required-field check and can crash with KeyError instead of producing a test failure.
💡 Suggested fix
- names = {t["name"] for t in tools}
expected_names = {
"cmux_system",
"cmux_workspace",
"cmux_window",
"cmux_pane",
"cmux_surface",
"cmux_notification",
"cmux_tab",
"cmux_browser",
}
- check("all tool names present", names == expected_names,
- f"missing={expected_names - names}, extra={names - expected_names}")
# Each tool should have name, description, inputSchema
for t in tools:
has_fields = all(k in t for k in ("name", "description", "inputSchema"))
if not has_fields:
check(f"tool {t.get('name')} has required fields", False, str(t.keys()))
return
check("all tools have name/description/inputSchema", True)
+
+ names = {t["name"] for t in tools}
+ check("all tool names present", names == expected_names,
+ f"missing={expected_names - names}, extra={names - expected_names}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| names = {t["name"] for t in tools} | |
| expected_names = { | |
| "cmux_system", | |
| "cmux_workspace", | |
| "cmux_window", | |
| "cmux_pane", | |
| "cmux_surface", | |
| "cmux_notification", | |
| "cmux_tab", | |
| "cmux_browser", | |
| } | |
| check("all tool names present", names == expected_names, | |
| f"missing={expected_names - names}, extra={names - expected_names}") | |
| # Each tool should have name, description, inputSchema | |
| for t in tools: | |
| has_fields = all(k in t for k in ("name", "description", "inputSchema")) | |
| if not has_fields: | |
| check(f"tool {t.get('name')} has required fields", False, str(t.keys())) | |
| return | |
| check("all tools have name/description/inputSchema", True) | |
| expected_names = { | |
| "cmux_system", | |
| "cmux_workspace", | |
| "cmux_window", | |
| "cmux_pane", | |
| "cmux_surface", | |
| "cmux_notification", | |
| "cmux_tab", | |
| "cmux_browser", | |
| } | |
| # Each tool should have name, description, inputSchema | |
| for t in tools: | |
| has_fields = all(k in t for k in ("name", "description", "inputSchema")) | |
| if not has_fields: | |
| check(f"tool {t.get('name')} has required fields", False, str(t.keys())) | |
| return | |
| check("all tools have name/description/inputSchema", True) | |
| names = {t["name"] for t in tools} | |
| check("all tool names present", names == expected_names, | |
| f"missing={expected_names - names}, extra={names - expected_names}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_mcp_server.py` around lines 124 - 144, The comprehension that
builds names = {t["name"] for t in tools} can raise KeyError before you validate
tool fields; instead, first validate each tool in tools for required keys
("name","description","inputSchema") using the existing loop (the has_fields
check and check(...) call), and only after that build the names set (or use
t.get("name") guarded by the validation) so the test fails with a proper check
message rather than an exception; reference the variables/tools loop,
has_fields, check(...) and the t["name"] usage when making the change.
There was a problem hiding this comment.
The required-field check on line 140-143 runs before the set comprehension on line 124 returns if any tool is missing fields. The existing order is safe.
There was a problem hiding this comment.
@Efan404, I appreciate the double-check, but the execution order is actually the other way around. Python runs code top-to-bottom:
line 124: names = {t["name"] for t in tools} ← executes FIRST (can KeyError)
...
line 140: for t in tools: ← executes AFTER
line 141: has_fields = all(...)
line 142: if not has_fields:
line 143: ...return
The set comprehension {t["name"] for t in tools} at line 124 runs before the validation loop at lines 140–143. If any tool object in the list is missing the "name" key, Python raises a KeyError at line 124 and the validation loop never gets a chance to fire and produce a proper FAIL message.
The fix is simply to move the set comprehension and its check(...) call to after the validation loop (as suggested), so a missing field is caught gracefully first. The rest of your test logic is fine.
| sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) | ||
| sock.settimeout(5) | ||
| sock.connect(SOCKET_PATH) | ||
| sock.sendall(json.dumps(payload).encode("utf-8") + b"\n") | ||
|
|
||
| data = b"" | ||
| while b"\n" not in data: | ||
| chunk = sock.recv(65536) | ||
| if not chunk: | ||
| break | ||
| data += chunk | ||
| sock.close() | ||
|
|
There was a problem hiding this comment.
Use a context manager for the socket in rpc.
If any socket operation raises before sock.close(), file descriptors leak across test cases.
💡 Suggested fix
- sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
- sock.settimeout(5)
- sock.connect(SOCKET_PATH)
- sock.sendall(json.dumps(payload).encode("utf-8") + b"\n")
-
- data = b""
- while b"\n" not in data:
- chunk = sock.recv(65536)
- if not chunk:
- break
- data += chunk
- sock.close()
+ data = b""
+ with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as sock:
+ sock.settimeout(5)
+ sock.connect(SOCKET_PATH)
+ sock.sendall(json.dumps(payload).encode("utf-8") + b"\n")
+ while b"\n" not in data:
+ chunk = sock.recv(65536)
+ if not chunk:
+ break
+ data += chunk📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) | |
| sock.settimeout(5) | |
| sock.connect(SOCKET_PATH) | |
| sock.sendall(json.dumps(payload).encode("utf-8") + b"\n") | |
| data = b"" | |
| while b"\n" not in data: | |
| chunk = sock.recv(65536) | |
| if not chunk: | |
| break | |
| data += chunk | |
| sock.close() | |
| data = b"" | |
| with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as sock: | |
| sock.settimeout(5) | |
| sock.connect(SOCKET_PATH) | |
| sock.sendall(json.dumps(payload).encode("utf-8") + b"\n") | |
| while b"\n" not in data: | |
| chunk = sock.recv(65536) | |
| if not chunk: | |
| break | |
| data += chunk |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_mcp_socket_rpc.py` around lines 33 - 45, The socket in the rpc
test is created without a context manager which can leak file descriptors if an
exception occurs; change the code in tests/test_mcp_socket_rpc.py to use a
context manager (e.g., with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as
sock:) and perform sock.settimeout(...), sock.connect(...), sock.sendall(...),
and the recv loop inside that with-block so the socket is always closed even on
errors; update any references to the variable accordingly within the same block.
There was a problem hiding this comment.
Acknowledged — the socket in test_mcp_socket_rpc.py is closed in the rpc() function return path. Will consider context manager for robustness.
There was a problem hiding this comment.
@Efan404, thanks for the acknowledgment! You're correct that sock.close() runs on the normal return path, but if an exception is raised in connect, sendall, or the recv loop before reaching it, the file descriptor would still leak. Since this is a test utility that may run many iterations, using with socket.socket(...) as sock: is the safest and most Pythonic approach. Happy to leave this open until you decide to apply the change.
There was a problem hiding this comment.
Pull request overview
This PR adds an MCP server mode to cmux and refactors the MCP backend to talk to the cmux daemon via direct Unix-domain-socket RPC (instead of spawning CLI subprocesses), while introducing grouped “action-based” MCP tools and adding Python integration tests plus supporting documentation.
Changes:
- Add MCP server plumbing (types, protocol handler, stdio loop) and
cmux --mcpentrypoint. - Implement direct socket v2 RPC backend and grouped MCP tools with action allowlisting / required-parameter validation.
- Add Python smoke/integration tests and documentation/build guides for MCP server usage.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_mcp_socket_rpc.py |
Integration tests for direct socket v2 RPC tool actions. |
tests/test_mcp_server.py |
Smoke tests for MCP JSON-RPC 2.0 stdio protocol and grouped tool definitions. |
skills-analysis.md |
Analysis doc for the existing skills/ system. |
docs/plans/2026-03-03-mcp-tool-bugfix-and-testing-design.md |
Design notes for MCP tool bugfixing/testing strategy. |
docs/plans/2026-03-03-mcp-server-redesign.md |
Redesign doc describing direct socket RPC and grouped tools. |
docs/MCP-SERVER.md |
MCP server design document (currently describes earlier tool naming). |
docs/MCP-BUILD.md |
Build guide for the MCP server (currently lists earlier tool set). |
docs/IMPLEMENTATION.md |
Implementation plan doc (currently reflects earlier file layout/tool structure). |
README.md |
Adds a development/build guide section to the README. |
GhosttyTabs.xcodeproj/project.pbxproj |
Adds MCP Swift files to the Xcode project/targets. |
CLI/cmux.swift |
Adds --mcp flag handling to launch MCP server mode. |
CLI/MCPTypes.swift |
Introduces JSON-RPC 2.0 + MCP data types and AnyCodable. |
CLI/MCPToolRegistry.swift |
Implements grouped action-based MCP tools and registration. |
CLI/MCPProtocol.swift |
MCP JSON-RPC request routing for initialize/tools/list/tools/call. |
CLI/MCPMain.swift |
MCP server entrypoint and stdio loop. |
CLI/MCPBackend.swift |
New direct Unix-socket RPC backend (POSIX read/write, reconnect logic). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Try once, reconnect on failure, try again | ||
| do { | ||
| try ensureConnected() | ||
| return try sendRPC(method: method, params: params) | ||
| } catch { | ||
| // Connection may be stale — reconnect and retry once | ||
| disconnect() | ||
| try ensureConnected() | ||
| return try sendRPC(method: method, params: params) |
There was a problem hiding this comment.
rpc(method:params:) retries the RPC on any thrown error, including legitimate remote errors (ok=false). This can cause non-idempotent actions (create/close/move/etc.) to be executed twice and can mask the original failure. Only retry on transport-level failures (connect/read/write) and propagate application-level errors without retrying (e.g., differentiate error types or inspect where the error originated).
| // Try once, reconnect on failure, try again | |
| do { | |
| try ensureConnected() | |
| return try sendRPC(method: method, params: params) | |
| } catch { | |
| // Connection may be stale — reconnect and retry once | |
| disconnect() | |
| try ensureConnected() | |
| return try sendRPC(method: method, params: params) | |
| // Try once, reconnect on transport-level failure, try again | |
| do { | |
| try ensureConnected() | |
| return try sendRPC(method: method, params: params) | |
| } catch let transportError as POSIXError { | |
| // Transport-level POSIX error: connection may be stale — reconnect and retry once | |
| disconnect() | |
| try ensureConnected() | |
| return try sendRPC(method: method, params: params) | |
| } catch let transportNSError as NSError where transportNSError.domain == NSPOSIXErrorDomain { | |
| // Transport-level NSError in NSPOSIXErrorDomain: reconnect and retry once | |
| disconnect() | |
| try ensureConnected() | |
| return try sendRPC(method: method, params: params) | |
| } catch { | |
| // Application-level or unknown error: do not retry | |
| throw error |
There was a problem hiding this comment.
Fixed — rpc() now only retries on MCPError.transportError, not on application errors. Non-idempotent operations will not be double-executed. See commit 3c3f3bc.
| private func readLine() throws -> Data { | ||
| var buffer = Data() | ||
| let newline = UInt8(ascii: "\n") | ||
| var chunk = [UInt8](repeating: 0, count: 4096) | ||
|
|
||
| while true { | ||
| let bytesRead = Darwin.read(socketFd, &chunk, chunk.count) | ||
| if bytesRead < 0 { | ||
| if errno == EINTR { continue } // interrupted by signal, retry | ||
| disconnect() | ||
| throw MCPError.executionFailed("Socket read failed: \(String(cString: strerror(errno)))") | ||
| } | ||
| if bytesRead == 0 { | ||
| // EOF — connection closed by daemon | ||
| disconnect() | ||
| if buffer.isEmpty { | ||
| throw MCPError.executionFailed("Socket connection closed") | ||
| } | ||
| break | ||
| } | ||
| if let nlIndex = chunk[0..<bytesRead].firstIndex(of: newline) { | ||
| buffer.append(contentsOf: chunk[0..<nlIndex]) | ||
| break | ||
| } else { | ||
| buffer.append(contentsOf: chunk[0..<bytesRead]) | ||
| } | ||
| } |
There was a problem hiding this comment.
readLine() blocks on a plain read() loop with no timeout/polling. If the daemon never replies (or the connection stalls), the MCP server can hang indefinitely waiting for a newline. Consider using poll/select with a reasonable timeout (similar to SocketClient.send() in CLI/cmux.swift) and surface a timeout error so the client can recover.
There was a problem hiding this comment.
Fixed — readLine() now uses poll() with a 30-second timeout before each read() call. See commit 3c3f3bc.
| // Build params dict (everything except "action") | ||
| var params: [String: Any] = [:] | ||
| let allowed = Set(def.required + def.optional) | ||
| for (key, value) in arguments where key != "action" { | ||
| if allowed.contains(key) { | ||
| params[key] = value | ||
| } | ||
| } |
There was a problem hiding this comment.
GroupedTool.execute silently drops any arguments that are not in the action's required/optional allowlist. With “strict” action/parameter validation, unexpected parameters should likely produce an invalidParams error so callers don’t think a parameter was applied when it was ignored. Consider detecting extra keys (excluding action) and rejecting them with a clear error message.
| | Tool | Description | | ||
| |------|-------------| | ||
| | `cmux_identify` | Get current workspace/surface context | | ||
| | `cmux_list_workspaces` | List all workspaces | | ||
| | `cmux_list_panes` | List all panes | | ||
| | `cmux_list_pane_surfaces` | List surfaces in a pane | | ||
| | `cmux_read_screen` | Read terminal output | | ||
| | `cmux_send_input` | Send text input | | ||
| | `cmux_send_key` | Send key press | | ||
| | `cmux_create_split` | Create a split | | ||
| | `cmux_focus_pane` | Focus a pane | | ||
| | `cmux_new_workspace` | Create a new workspace | | ||
| | `cmux_trigger_flash` | Trigger attention flash | | ||
| | `cmux_list_windows` | List all windows | | ||
|
|
There was a problem hiding this comment.
The “MCP Tools Available” table lists the old individual tools (cmux_identify, cmux_read_screen, etc.), but this PR’s MCP server exposes grouped tools (cmux_system, cmux_surface, etc.). Please update this section (and any example invocations) to reflect the grouped tool names and action argument so build/test docs stay accurate.
| | Tool | Description | | |
| |------|-------------| | |
| | `cmux_identify` | Get current workspace/surface context | | |
| | `cmux_list_workspaces` | List all workspaces | | |
| | `cmux_list_panes` | List all panes | | |
| | `cmux_list_pane_surfaces` | List surfaces in a pane | | |
| | `cmux_read_screen` | Read terminal output | | |
| | `cmux_send_input` | Send text input | | |
| | `cmux_send_key` | Send key press | | |
| | `cmux_create_split` | Create a split | | |
| | `cmux_focus_pane` | Focus a pane | | |
| | `cmux_new_workspace` | Create a new workspace | | |
| | `cmux_trigger_flash` | Trigger attention flash | | |
| | `cmux_list_windows` | List all windows | | |
| The MCP server exposes grouped tools with an `action` argument rather than many individual tools. | |
| | Tool | Action | Description | | |
| |----------------|----------------------|--------------------------------------| | |
| | `cmux_system` | `identify` | Get current workspace/surface context | | |
| | `cmux_system` | `list_workspaces` | List all workspaces | | |
| | `cmux_system` | `list_windows` | List all windows | | |
| | `cmux_system` | `new_workspace` | Create a new workspace | | |
| | `cmux_system` | `trigger_flash` | Trigger attention flash | | |
| | `cmux_surface` | `list_panes` | List all panes | | |
| | `cmux_surface` | `list_pane_surfaces` | List surfaces in a pane | | |
| | `cmux_surface` | `read_screen` | Read terminal output | | |
| | `cmux_surface` | `send_input` | Send text input | | |
| | `cmux_surface` | `send_key` | Send key press | | |
| | `cmux_surface` | `create_split` | Create a split | | |
| | `cmux_surface` | `focus_pane` | Focus a pane | |
There was a problem hiding this comment.
Resolved — docs/MCP-BUILD.md deleted as it was stale. See commit fe12f3c.
| } catch { | ||
| let errResp = JSONRPCErrorResponse( | ||
| id: request.id, | ||
| error: JSONRPCError(code: JSONRPCErrorCode.internalError.rawValue, message: error.localizedDescription) | ||
| ) | ||
| return try encodeResponse(errResp) | ||
| } |
There was a problem hiding this comment.
Backend execution failures (e.g., MCPError.executionFailed) currently fall into the generic catch and are returned as JSON-RPC -32603 (internal error). Given the documented error-code split (and tests expecting "backend failure" errors), it would be better to map execution/connection failures to -32000 server error so clients can distinguish backend availability issues from server bugs.
| // Return the result dict, or empty dict for simple ok responses | ||
| if let result = response["result"] as? [String: Any] { | ||
| return result | ||
| } |
There was a problem hiding this comment.
sendRPC only returns response["result"] when it is a [String: Any], and otherwise returns [:]. The socket v2 protocol supports arbitrary JSON results (arrays/strings/numbers/etc.), so this will silently drop valid responses for methods that return non-dictionary payloads. Consider returning Any (or AnyCodable) from rpc/sendRPC, and have rpcForTool JSON-encode that value for output.
| // Return the result dict, or empty dict for simple ok responses | |
| if let result = response["result"] as? [String: Any] { | |
| return result | |
| } | |
| // Return the result dict when present, or wrap non-dict results to avoid data loss. | |
| if let resultDict = response["result"] as? [String: Any] { | |
| return resultDict | |
| } else if let resultValue = response["result"] { | |
| // For scalar/array results, wrap in a dictionary so callers still receive the data. | |
| return ["value": resultValue] | |
| } | |
| // No result field: return empty dict for simple ok responses |
There was a problem hiding this comment.
Fixed — sendRPC() now wraps non-dict results (arrays, strings, etc.) in ["value": result]. See commit 3c3f3bc.
| requestId += 1 | ||
| let request: [String: Any] = [ | ||
| "id": requestId, | ||
| "method": method, | ||
| "params": params | ||
| ] | ||
|
|
||
| let requestData = try JSONSerialization.data(withJSONObject: request) | ||
| var payload = requestData | ||
| payload.append(contentsOf: "\n".utf8) | ||
|
|
||
| try writeAll(payload) | ||
|
|
||
| // Read response (newline-terminated JSON) | ||
| let responseData = try readLine() | ||
|
|
||
| guard let response = try JSONSerialization.jsonObject(with: responseData) as? [String: Any] else { | ||
| throw MCPError.executionFailed("Invalid response from socket") | ||
| } | ||
|
|
||
| // cmux socket v2: {ok: bool, result: ..., error: ...} | ||
| guard let ok = response["ok"] as? Bool else { | ||
| throw MCPError.executionFailed("Missing 'ok' field in socket response") | ||
| } |
There was a problem hiding this comment.
The RPC layer never validates that the response id matches the request id. If the socket ever returns a stale/out-of-order response (or if the connection is reused across calls in the future), this can associate the wrong response with a request. It would be safer to check the id field and treat mismatches as a protocol/transport error.
There was a problem hiding this comment.
Fixed — error response now extracts and preserves request id from raw JSON. See commit d8ca8d2.
| All Tools prefixed with `cmux_`, following `cmux_<category>_<action>` format. | ||
|
|
||
| ### 3.2 MVP Tools (P0 - Must Implement) | ||
|
|
||
| | Priority | Tool Name | Description | Parameters | | ||
| |---------|------------|-------------|------------| | ||
| | P0 | `cmux_identify` | Get current context (workspace/surface) | `workspace`, `surface`, `no_caller` | | ||
| | P0 | `cmux_list_workspaces` | List all workspaces | `workspace` (filter) | | ||
| | P0 | `cmux_list_panes` | List all panes | `workspace` | | ||
| | P0 | `cmux_list_pane_surfaces` | List surfaces | `workspace`, `pane` | | ||
| | P0 | `cmux_read_screen` | Read terminal output | `workspace`, `surface`, `scrollback`, `lines` | | ||
| | P0 | `cmux_send_input` | Send input | `workspace`, `surface`, `text` | | ||
| | P0 | `cmux_send_key` | Send key press | `workspace`, `surface`, `key` | | ||
|
|
||
| ### 3.3 Extended Tools (P1 - Recommended) | ||
|
|
||
| | Priority | Tool Name | Description | Parameters | | ||
| |---------|------------|-------------|------------| | ||
| | P1 | `cmux_create_split` | Create split | `direction`, `workspace`, `surface`, `panel` | | ||
| | P1 | `cmux_focus_pane` | Focus pane | `pane`, `workspace` | | ||
| | P1 | `cmux_new_workspace` | Create workspace | `command` | | ||
| | P1 | `cmux_trigger_flash` | Trigger flash | `workspace`, `surface` | | ||
| | P1 | `cmux_list_windows` | List all windows | - | | ||
|
|
||
| ### 3.4 Advanced Tools (P2 - Future) | ||
|
|
||
| | Priority | Tool Name | Description | | ||
| |---------|------------|-------------| | ||
| | P2 | `cmux_browser_open` | Open URL | | ||
| | P2 | `cmux_browser_snapshot` | Get snapshot | | ||
| | P2 | `cmux_browser_click` | Click element | | ||
| | P2 | `cmux_browser_fill` | Fill form | | ||
| | P2 | `cmux_browser_wait` | Wait for condition | | ||
| | P2 | `cmux_notify` | Send notification | | ||
| | P2 | `cmux_set_status` | Set status | | ||
| | P2 | `cmux_set_progress` | Set progress | |
There was a problem hiding this comment.
This design doc still describes the older per-action tool naming (cmux_<category>_<action> like cmux_read_screen, cmux_send_key, etc.), but the implementation in this PR uses grouped tools (cmux_system, cmux_workspace, etc. with an action parameter). Please update the naming convention and tool tables/examples to match the grouped-tool API so readers don’t implement against the wrong interface.
| All Tools prefixed with `cmux_`, following `cmux_<category>_<action>` format. | |
| ### 3.2 MVP Tools (P0 - Must Implement) | |
| | Priority | Tool Name | Description | Parameters | | |
| |---------|------------|-------------|------------| | |
| | P0 | `cmux_identify` | Get current context (workspace/surface) | `workspace`, `surface`, `no_caller` | | |
| | P0 | `cmux_list_workspaces` | List all workspaces | `workspace` (filter) | | |
| | P0 | `cmux_list_panes` | List all panes | `workspace` | | |
| | P0 | `cmux_list_pane_surfaces` | List surfaces | `workspace`, `pane` | | |
| | P0 | `cmux_read_screen` | Read terminal output | `workspace`, `surface`, `scrollback`, `lines` | | |
| | P0 | `cmux_send_input` | Send input | `workspace`, `surface`, `text` | | |
| | P0 | `cmux_send_key` | Send key press | `workspace`, `surface`, `key` | | |
| ### 3.3 Extended Tools (P1 - Recommended) | |
| | Priority | Tool Name | Description | Parameters | | |
| |---------|------------|-------------|------------| | |
| | P1 | `cmux_create_split` | Create split | `direction`, `workspace`, `surface`, `panel` | | |
| | P1 | `cmux_focus_pane` | Focus pane | `pane`, `workspace` | | |
| | P1 | `cmux_new_workspace` | Create workspace | `command` | | |
| | P1 | `cmux_trigger_flash` | Trigger flash | `workspace`, `surface` | | |
| | P1 | `cmux_list_windows` | List all windows | - | | |
| ### 3.4 Advanced Tools (P2 - Future) | |
| | Priority | Tool Name | Description | | |
| |---------|------------|-------------| | |
| | P2 | `cmux_browser_open` | Open URL | | |
| | P2 | `cmux_browser_snapshot` | Get snapshot | | |
| | P2 | `cmux_browser_click` | Click element | | |
| | P2 | `cmux_browser_fill` | Fill form | | |
| | P2 | `cmux_browser_wait` | Wait for condition | | |
| | P2 | `cmux_notify` | Send notification | | |
| | P2 | `cmux_set_status` | Set status | | |
| | P2 | `cmux_set_progress` | Set progress | | |
| Tools are grouped by domain and exposed as a small number of stable tool names, each prefixed with `cmux_`. The current groups are: | |
| - `cmux_system` – global/system-level operations (notifications, status, progress, etc.) | |
| - `cmux_workspace` – workspace, pane, and window management | |
| - `cmux_terminal` – terminal/surface interaction (screen reading, input, key presses, visual cues) | |
| - `cmux_browser` – in-terminal/browser automation | |
| Each grouped tool accepts an `action` parameter that selects the specific operation to perform (e.g., `action: "identify"`). The older per-action tool naming scheme (`cmux_<category>_<action>` such as `cmux_read_screen`, `cmux_send_key`, etc.) is no longer used and is documented here only for historical context. | |
| ### 3.2 MVP Tools (P0 - Must Implement) | |
| | Priority | Tool Name | Action | Description | Parameters | | |
| |----------|-----------------|------------------------|-----------------------------------------------|-------------------------------------------------| | |
| | P0 | `cmux_workspace`| `identify` | Get current context (workspace/surface) | `workspace`, `surface`, `no_caller` | | |
| | P0 | `cmux_workspace`| `list_workspaces` | List all workspaces | `workspace` (filter) | | |
| | P0 | `cmux_workspace`| `list_panes` | List all panes | `workspace` | | |
| | P0 | `cmux_workspace`| `list_pane_surfaces` | List surfaces for a given pane | `workspace`, `pane` | | |
| | P0 | `cmux_terminal` | `read_screen` | Read terminal output | `workspace`, `surface`, `scrollback`, `lines` | | |
| | P0 | `cmux_terminal` | `send_input` | Send input text to a terminal surface | `workspace`, `surface`, `text` | | |
| | P0 | `cmux_terminal` | `send_key` | Send key press to a terminal surface | `workspace`, `surface`, `key` | | |
| ### 3.3 Extended Tools (P1 - Recommended) | |
| | Priority | Tool Name | Action | Description | Parameters | | |
| |----------|-----------------|------------------|-----------------------------------|-------------------------------------------------| | |
| | P1 | `cmux_workspace`| `create_split` | Create split | `direction`, `workspace`, `surface`, `panel` | | |
| | P1 | `cmux_workspace`| `focus_pane` | Focus pane | `pane`, `workspace` | | |
| | P1 | `cmux_workspace`| `new_workspace` | Create workspace | `command` | | |
| | P1 | `cmux_terminal` | `trigger_flash` | Trigger flash on a terminal | `workspace`, `surface` | | |
| | P1 | `cmux_workspace`| `list_windows` | List all windows | - | | |
| ### 3.4 Advanced Tools (P2 - Future) | |
| | Priority | Tool Name | Action | Description | | |
| |----------|----------------|------------------|----------------------| | |
| | P2 | `cmux_browser` | `open` | Open URL | | |
| | P2 | `cmux_browser` | `snapshot` | Get snapshot | | |
| | P2 | `cmux_browser` | `click` | Click element | | |
| | P2 | `cmux_browser` | `fill` | Fill form | | |
| | P2 | `cmux_browser` | `wait` | Wait for condition | | |
| | P2 | `cmux_system` | `notify` | Send notification | | |
| | P2 | `cmux_system` | `set_status` | Set status | | |
| | P2 | `cmux_system` | `set_progress` | Set progress | |
There was a problem hiding this comment.
Fixed — docs/MCP-SERVER.md completely rewritten with current architecture. See commit f63197e.
| ## File Structure | ||
|
|
||
| ``` | ||
| CLI/ | ||
| ├── cmux.swift # Existing CLI (no changes) | ||
| └── MCP/ | ||
| ├── MCPMain.swift # Entry point: argument parsing, stdio loop | ||
| ├── MCPTypes.swift # JSON-RPC types, MCP message structures | ||
| ├── MCPProtocol.swift # Protocol handling: initialize, tools/list, tools/call | ||
| ├── MCPToolRegistry.swift # Tool registration and discovery | ||
| ├── MCPTools/ | ||
| │ ├── Tool.swift # Base Tool protocol | ||
| │ ├── IdentifyTool.swift # cmux_identify | ||
| │ ├── ListTools.swift # cmux_list_* tools | ||
| │ ├── ReadScreenTool.swift # cmux_read_screen | ||
| │ ├── SendInputTool.swift # cmux_send_input, cmux_send_key | ||
| │ └── SplitTools.swift # cmux_create_split, cmux_focus_pane | ||
| └── MCPBackend.swift # SocketClient wrapper for cmux daemon communication | ||
| ``` |
There was a problem hiding this comment.
This implementation plan’s file structure (CLI/MCP/... and individual tool Swift files) doesn’t match the current implementation in this PR (MCP files live directly under CLI/, and tools are grouped in MCPToolRegistry.swift). Updating this doc will help future contributors avoid looking for files/targets that no longer exist.
There was a problem hiding this comment.
Fixed — docs/IMPLEMENTATION.md deleted (stale). docs/MCP-SERVER.md rewritten with correct file structure. See commit f63197e.
| } | ||
| if arg == "--mcp" { | ||
| // Run MCP server mode | ||
| runMCPServer(socketPath: socketPath, password: socketPasswordArg, idFormat: idFormatArg ?? "refs") |
There was a problem hiding this comment.
The runMCPServer call uses the password value sourced from the --password CLI flag, which exposes the socket-control password in the process command line. On macOS and similar systems, other local users or monitoring tools can read process arguments, allowing them to capture this password and authenticate to the cmux daemon with full control of your sessions. Prefer resolving the socket password from a more protected source (e.g., existing SocketPasswordResolver / keychain or environment variables) and avoid accepting or passing long‑lived secrets via command-line flags.
There was a problem hiding this comment.
Fixed — --mcp handler now uses SocketPasswordResolver.resolve() which reads from env/file, not CLI args. See commit cbd5dd7.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_mcp_server.py">
<violation number="1" location="tests/test_mcp_server.py:199">
P2: `test_action_validation` silently skips assertions when responses are missing, which can produce false-positive test passes.</violation>
</file>
<file name="GhosttyTabs.xcodeproj/project.pbxproj">
<violation number="1" location="GhosttyTabs.xcodeproj/project.pbxproj:701">
P2: Release builds should not be limited to the active architecture. `ONLY_ACTIVE_ARCH = YES` will create a single-arch Release binary, breaking distribution on the other macOS architecture. Remove this override or set it to NO for Release builds.</violation>
</file>
<file name="CLI/MCPTypes.swift">
<violation number="1" location="CLI/MCPTypes.swift:223">
P2: Avoid hashing complex values via `String(describing:)`; hash arrays/dictionaries structurally so values that compare equal always produce the same hash.</violation>
</file>
<file name="docs/MCP-BUILD.md">
<violation number="1" location="docs/MCP-BUILD.md:139">
P3: The MCP tools list documents legacy per-action tool names, but the registry now exposes grouped tools (cmux_system, cmux_workspace, etc.). Update the table to match the current tool names and indicate actions are selected via the `action` parameter.</violation>
</file>
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:673">
P2: `--mcp` bypasses the socket password resolver, so env/file-configured passwords are ignored and MCP fails to authenticate against password-protected sockets. Resolve the password the same way as the normal CLI path before starting the MCP server.</violation>
</file>
<file name="CLI/MCPToolRegistry.swift">
<violation number="1" location="CLI/MCPToolRegistry.swift:109">
P2: Required parameter validation treats JSON null as present. `arguments[param] != nil` allows `NSNull` through, so required parameters can be sent as null and still pass validation. Treat `NSNull` as missing to avoid invalid RPC calls.</violation>
<violation number="2" location="CLI/MCPToolRegistry.swift:339">
P3: The `read_text` fallback re-issues the RPC, so any response without a `text` field triggers a second `surface.read_text` call. Reuse the existing `result` instead of hitting the socket twice.</violation>
</file>
<file name="CLI/MCPBackend.swift">
<violation number="1" location="CLI/MCPBackend.swift:45">
P3: `idFormat` is accepted by `MCPBackend` but never stored or used, so the MCP server’s `--id-format` flag is a no-op. Either remove the parameter/flag or plumb the format through to the backend’s output formatting so callers get the expected ID format.</violation>
</file>
<file name="CLI/MCPMain.swift">
<violation number="1" location="CLI/MCPMain.swift:50">
P1: The catch-all error response uses a fixed JSON-RPC id (`0`), which can break client request/response correlation when a request with a different id fails during processing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| # Missing action | ||
| msgs = [init_msg(1), tools_call_msg("cmux_surface", {}, id=2)] | ||
| responses = mcp_session(msgs) | ||
| if len(responses) >= 2: |
There was a problem hiding this comment.
P2: test_action_validation silently skips assertions when responses are missing, which can produce false-positive test passes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_mcp_server.py, line 199:
<comment>`test_action_validation` silently skips assertions when responses are missing, which can produce false-positive test passes.</comment>
<file context>
@@ -0,0 +1,338 @@
+ # Missing action
+ msgs = [init_msg(1), tools_call_msg("cmux_surface", {}, id=2)]
+ responses = mcp_session(msgs)
+ if len(responses) >= 2:
+ err = responses[1].get("error", {})
+ check("missing 'action' returns error", err.get("code") == -32602,
</file context>
There was a problem hiding this comment.
Acknowledged — test_action_validation guards prevent crashes on missing responses. The test still reports FAIL via check() if responses are insufficient.
| GENERATE_INFOPLIST_FILE = YES; | ||
| MACOSX_DEPLOYMENT_TARGET = 14.0; | ||
| MARKETING_VERSION = 0.61.0; | ||
| ONLY_ACTIVE_ARCH = YES; |
There was a problem hiding this comment.
P2: Release builds should not be limited to the active architecture. ONLY_ACTIVE_ARCH = YES will create a single-arch Release binary, breaking distribution on the other macOS architecture. Remove this override or set it to NO for Release builds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At GhosttyTabs.xcodeproj/project.pbxproj, line 701:
<comment>Release builds should not be limited to the active architecture. `ONLY_ACTIVE_ARCH = YES` will create a single-arch Release binary, breaking distribution on the other macOS architecture. Remove this override or set it to NO for Release builds.</comment>
<file context>
@@ -657,19 +677,35 @@
+ GENERATE_INFOPLIST_FILE = YES;
+ MACOSX_DEPLOYMENT_TARGET = 14.0;
+ MARKETING_VERSION = 0.61.0;
+ ONLY_ACTIVE_ARCH = YES;
+ PRODUCT_BUNDLE_IDENTIFIER = com.cmuxterm.appuitests;
+ PRODUCT_NAME = "$(TARGET_NAME)";
</file context>
| ONLY_ACTIVE_ARCH = YES; | |
| ONLY_ACTIVE_ARCH = NO; |
| hasher.combine(string) | ||
| default: | ||
| // For complex types, use description | ||
| hasher.combine(String(describing: value)) |
There was a problem hiding this comment.
P2: Avoid hashing complex values via String(describing:); hash arrays/dictionaries structurally so values that compare equal always produce the same hash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CLI/MCPTypes.swift, line 223:
<comment>Avoid hashing complex values via `String(describing:)`; hash arrays/dictionaries structurally so values that compare equal always produce the same hash.</comment>
<file context>
@@ -0,0 +1,404 @@
+ hasher.combine(string)
+ default:
+ // For complex types, use description
+ hasher.combine(String(describing: value))
+ }
+ }
</file context>
|
|
||
| // Validate required params | ||
| for param in def.required { | ||
| guard arguments[param] != nil else { |
There was a problem hiding this comment.
P2: Required parameter validation treats JSON null as present. arguments[param] != nil allows NSNull through, so required parameters can be sent as null and still pass validation. Treat NSNull as missing to avoid invalid RPC calls.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CLI/MCPToolRegistry.swift, line 109:
<comment>Required parameter validation treats JSON null as present. `arguments[param] != nil` allows `NSNull` through, so required parameters can be sent as null and still pass validation. Treat `NSNull` as missing to avoid invalid RPC calls.</comment>
<file context>
@@ -0,0 +1,623 @@
+
+ // Validate required params
+ for param in def.required {
+ guard arguments[param] != nil else {
+ throw MCPError.invalidParameters("Action '\(action)' requires parameter: \(param)")
+ }
</file context>
| guard arguments[param] != nil else { | |
| guard let value = arguments[param], !(value is NSNull) else { |
There was a problem hiding this comment.
rpcForTool() correctly formats results as MCPToolCallResult with prettyPrinted JSON. No issue here.
…dling
- Add socket ownership (st_uid) validation before connecting
- Only retry RPC on transport errors, not application errors
- Add 30s poll() timeout to readLine() to prevent indefinite blocking
- Handle non-dict RPC results (arrays/scalars) by wrapping in {value: ...}
- Remove unused idFormat parameter
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… RPC - Remap action_name to action for *.action RPC methods to match socket handler expectations - Extract request id from raw JSON for proper error response correlation - Reuse existing result in SurfaceTool read_text instead of re-issuing RPC - Remove force unwrap in debug message Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The --mcp flag was bypassing the password resolution chain (explicit > env > config file), causing authentication failures when the password was configured via environment variable or config file rather than explicit --password flag. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
CLI/MCPBackend.swift (2)
63-69:⚠️ Potential issue | 🟠 MajorHarden socket path validation beyond UID checks.
Using
stat(...)here follows symlinks and only verifies ownership. Validate the filesystem object itself (lstat) and require a socket file type before connecting.🔧 Suggested hardening
- var st = stat() - guard stat(socketPath, &st) == 0 else { + var st = stat() + guard lstat(socketPath, &st) == 0 else { throw MCPError.executionFailed("Socket not found at \(socketPath)") } + guard (st.st_mode & mode_t(S_IFMT)) == mode_t(S_IFSOCK) else { + throw MCPError.executionFailed("Path is not a Unix socket: \(socketPath)") + } guard st.st_uid == getuid() else { throw MCPError.executionFailed("Socket at \(socketPath) is not owned by the current user — refusing to connect") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/MCPBackend.swift` around lines 63 - 69, Replace the current stat-based ownership-only check with an lstat-based validation that ensures the filesystem object at socketPath is actually a socket and not a symlink or other type: call lstat(&st) instead of stat(&st), verify the file type using st.st_mode (compare (st.st_mode & S_IFMT) == S_IFSOCK or equivalent) and only then check ownership (st.st_uid == getuid()); if any check fails, throw MCPError.executionFailed with a clear message referencing socketPath and the specific failure (not found, wrong type, or wrong owner).
217-225:⚠️ Potential issue | 🔴 CriticalAvoid automatic resend of mutating RPCs after transport failures.
A transport error can happen after the daemon already applied the action. Retrying here can duplicate non-idempotent operations.
🔧 Safer retry behavior
do { try ensureConnected() return try sendRPC(method: method, params: params) } catch MCPError.transportError { - // Connection may be stale — reconnect and retry once + // Mark connection stale; let caller decide whether to retry. disconnect() - try ensureConnected() - return try sendRPC(method: method, params: params) + throw MCPError.transportError("Transport failed for '\(method)'; request may have been applied") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/MCPBackend.swift` around lines 217 - 225, The catch block currently retries sendRPC on any MCPError.transportError which can duplicate non-idempotent operations; change the logic in this function so you do NOT automatically resend after a transportError — either (A) propagate the transportError to the caller immediately (remove the reconnect+retry) or (B) only retry after reconnect when the RPC method is known to be safe/idempotent by checking the method name or a new boolean flag (e.g., retryAllowed) passed into sendRPC/sendRPCReliable; keep the existing calls to ensureConnected(), disconnect(), and sendRPC(method:params:) but ensure the retry path is gated by an explicit idempotency check or replaced by rethrowing the error so callers can decide whether to retry.tests/test_mcp_server.py (2)
204-249:⚠️ Potential issue | 🟠 MajorFail fast when expected responses are missing.
These blocks only assert inside
if len(responses) >= ..., so dropped responses can skip validation and still pass.Proposed fix pattern
+def expect_response(case_name: str, responses: list[dict], min_count: int, index: int = 1): + if len(responses) < min_count: + check(case_name, False, f"expected {min_count} responses, got {len(responses)}") + return None + return responses[index] + def test_action_validation(): @@ responses = mcp_session(msgs) - if len(responses) >= 2: - err = responses[1].get("error", {}) - check("missing 'action' returns error", err.get("code") == -32602, - f"code={err.get('code')}, msg={err.get('message')}") + r = expect_response("missing 'action' response exists", responses, 2) + if r is not None: + err = r.get("error", {}) + check("missing 'action' returns error", err.get("code") == -32602, + f"code={err.get('code')}, msg={err.get('message')}")Apply the same pattern to
test_action_name_remappingandtest_error_id_correlation.Also applies to: 319-339, 348-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_server.py` around lines 204 - 249, The tests (in test_mcp_server.py) currently guard validations with "if len(responses) >= N:" which silently skips assertions when responses are missing; replace those conditional guards with explicit fail-fast checks (e.g., assert len(responses) >= N or call check(...) to fail) immediately after obtaining responses so missing/dropped responses cause the test to fail; apply this change around the blocks using mcp_session, init_msg, tools_call_msg and check (including the similar blocks in test_action_name_remapping and test_error_id_correlation) so each block verifies response length before accessing responses[1].
124-143:⚠️ Potential issue | 🟡 MinorValidate tool fields before indexing
t["name"].
names = {t["name"] for t in tools}can raiseKeyErrorbefore the test reports a structured failure viacheck(...).Proposed fix
- names = {t["name"] for t in tools} expected_names = { "cmux_system", "cmux_workspace", "cmux_window", "cmux_pane", "cmux_surface", "cmux_notification", "cmux_tab", "cmux_browser", } - check("all tool names present", names == expected_names, - f"missing={expected_names - names}, extra={names - expected_names}") # Each tool should have name, description, inputSchema for t in tools: has_fields = all(k in t for k in ("name", "description", "inputSchema")) if not has_fields: check(f"tool {t.get('name')} has required fields", False, str(t.keys())) return check("all tools have name/description/inputSchema", True) + + names = {t["name"] for t in tools} + check("all tool names present", names == expected_names, + f"missing={expected_names - names}, extra={names - expected_names}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_server.py` around lines 124 - 143, The comprehension names = {t["name"] for t in tools} can raise KeyError before check(...) reports failures; first validate each tool has the required keys ("name","description","inputSchema") using the existing loop (or run a preliminary loop that calls check(...) for missing fields) and only after that build names from t["name"] for all validated tools, referencing the variables tools, names and the check(...) helper to report missing/extra fields.CLI/MCPProtocol.swift (1)
84-105:⚠️ Potential issue | 🟠 MajorReject unsupported protocol versions in
initialize.The current flow accepts any client
protocolVersionand echoes it, which hides version incompatibility and weakens handshake guarantees.Proposed fix
- guard let params = request.params, - let protocolVersion = params["protocolVersion"]?.value as? String else { + guard let params = request.params, + let requestedVersion = params["protocolVersion"]?.value as? String else { let error = JSONRPCErrorResponse( id: request.id, error: JSONRPCError(code: JSONRPCErrorCode.invalidParams.rawValue, message: "Missing protocolVersion") ) return try encodeResponse(error) } + guard requestedVersion == mcpProtocolVersion else { + let error = JSONRPCErrorResponse( + id: request.id, + error: JSONRPCError( + code: JSONRPCErrorCode.invalidParams.rawValue, + message: "Unsupported protocolVersion '\(requestedVersion)'. Supported: \(mcpProtocolVersion)" + ) + ) + return try encodeResponse(error) + } // Mark as initialized isInitialized = true @@ let result = MCPInitializeResult( - protocolVersion: protocolVersion, + protocolVersion: mcpProtocolVersion, capabilities: capabilities, serverInfo: serverInfo )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/MCPProtocol.swift` around lines 84 - 105, The handler currently accepts any request.params["protocolVersion"] and echoes it back in MCPInitializeResult; instead validate protocolVersion against your supported set (e.g. a constant/array SUPPORTED_PROTOCOL_VERSIONS) before setting isInitialized or returning capabilities. If protocolVersion is not in the supported set, return a JSONRPCErrorResponse (same shape as the existing error) with a clear message like "Unsupported protocolVersion: <value>" and do not set isInitialized or construct MCPInitializeResult; only when protocolVersion is valid proceed to set isInitialized = true and return MCPInitializeResult.
🧹 Nitpick comments (3)
CLI/MCPToolRegistry.swift (2)
321-352: Avoid duplicating validation logic inSurfaceTool.execute.
read_textreimplements required-param and allowlist handling already present inGroupedTool. Extracting a shared helper will keep behavior consistent as action rules evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/MCPToolRegistry.swift` around lines 321 - 352, The read_text branch in SurfaceTool.execute(arguments:) duplicates the required-parameter checks and allowlist filtering logic that GroupedTool already provides; refactor by extracting the validation-and-filter helper (e.g., a shared method like validateAndFilter(arguments:forAction:) on GroupedTool or an extension used by both classes) and have SurfaceTool.execute call that helper to (1) verify the action exists and required params are present using def.required/def.optional and (2) return the filtered params dictionary which you then pass to backend.rpc("surface.read_text", params:). Keep the existing behavior of returning only the "text" field when present and the JSON fallback, but remove the duplicated loop and guards from SurfaceTool.execute so all param validation/filtering is centralized.
114-121: Reject unexpected parameters instead of silently dropping them.Current filtering ignores unknown keys. That makes argument typos hard to detect and weakens action-level validation.
Proposed fix
- var params: [String: Any] = [:] let allowed = Set(def.required + def.optional) + let provided = Set(arguments.keys).subtracting(["action"]) + let unexpected = provided.subtracting(allowed) + guard unexpected.isEmpty else { + let names = unexpected.sorted().joined(separator: ", ") + throw MCPError.invalidParameters("Action '\(action)' received unsupported parameter(s): \(names)") + } + + var params: [String: Any] = [:] for (key, value) in arguments where key != "action" { if allowed.contains(key) { params[key] = value } }Apply the same unexpected-parameter check in
SurfaceTool.execute’sread_textbranch.Also applies to: 337-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/MCPToolRegistry.swift` around lines 114 - 121, The current params-building logic (where vars params, allowed = Set(def.required + def.optional), and the loop for (key, value) in arguments) silently drops unknown keys; change it to detect any argument keys not in allowed (and not "action") and reject them (return an error or throw) instead of ignoring them. Apply the same check inside SurfaceTool.execute’s read_text branch so that both MCPToolRegistry’s parameter construction and SurfaceTool.execute validate arguments against def.required/def.optional and fail fast on unexpected parameters.tests/test_mcp_server.py (1)
31-45: Surface subprocess startup failures explicitly.
mcp_sessionignoresproc.returncode/stderr, which can hide the root cause behind downstream assertion noise.Proposed fix
def mcp_session(messages: list[dict]) -> list[dict | None]: @@ proc = subprocess.run( [CMUX_BIN, "--mcp"], input=input_text, capture_output=True, text=True, timeout=10, ) + if proc.returncode != 0: + raise RuntimeError( + f"cmux --mcp exited with code {proc.returncode}\n" + f"stderr:\n{proc.stderr.strip()}" + ) responses = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_server.py` around lines 31 - 45, The test currently ignores the subprocess exit status and stderr after running CMUX_BIN in mcp_session; update the code after subprocess.run (variable proc) to check proc.returncode and, if non-zero, raise an AssertionError (or fail the test) that includes proc.returncode, proc.stderr and proc.stdout to make startup failures explicit; keep the existing parsing of proc.stdout into JSON lines (responses) but only proceed after the returncode/stderr check so test failures show the subprocess error rather than downstream JSON parse/assertion noise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/MCPBackend.swift`:
- Around line 250-257: The response parsing in sendRPC currently trusts the
socket frame without correlating request/response ids; update sendRPC to extract
the response["id"] from the parsed response (response variable from
responseData) and verify it matches the request id you sent (the id variable
used when sending the RPC); if it is missing or does not match, throw
MCPError.executionFailed with a clear message like "Mismatched or missing
response id" to reject stale/aligned frames and only proceed when response["id"]
== id and response["ok"] is true.
---
Duplicate comments:
In `@CLI/MCPBackend.swift`:
- Around line 63-69: Replace the current stat-based ownership-only check with an
lstat-based validation that ensures the filesystem object at socketPath is
actually a socket and not a symlink or other type: call lstat(&st) instead of
stat(&st), verify the file type using st.st_mode (compare (st.st_mode & S_IFMT)
== S_IFSOCK or equivalent) and only then check ownership (st.st_uid ==
getuid()); if any check fails, throw MCPError.executionFailed with a clear
message referencing socketPath and the specific failure (not found, wrong type,
or wrong owner).
- Around line 217-225: The catch block currently retries sendRPC on any
MCPError.transportError which can duplicate non-idempotent operations; change
the logic in this function so you do NOT automatically resend after a
transportError — either (A) propagate the transportError to the caller
immediately (remove the reconnect+retry) or (B) only retry after reconnect when
the RPC method is known to be safe/idempotent by checking the method name or a
new boolean flag (e.g., retryAllowed) passed into sendRPC/sendRPCReliable; keep
the existing calls to ensureConnected(), disconnect(), and
sendRPC(method:params:) but ensure the retry path is gated by an explicit
idempotency check or replaced by rethrowing the error so callers can decide
whether to retry.
In `@CLI/MCPProtocol.swift`:
- Around line 84-105: The handler currently accepts any
request.params["protocolVersion"] and echoes it back in MCPInitializeResult;
instead validate protocolVersion against your supported set (e.g. a
constant/array SUPPORTED_PROTOCOL_VERSIONS) before setting isInitialized or
returning capabilities. If protocolVersion is not in the supported set, return a
JSONRPCErrorResponse (same shape as the existing error) with a clear message
like "Unsupported protocolVersion: <value>" and do not set isInitialized or
construct MCPInitializeResult; only when protocolVersion is valid proceed to set
isInitialized = true and return MCPInitializeResult.
In `@tests/test_mcp_server.py`:
- Around line 204-249: The tests (in test_mcp_server.py) currently guard
validations with "if len(responses) >= N:" which silently skips assertions when
responses are missing; replace those conditional guards with explicit fail-fast
checks (e.g., assert len(responses) >= N or call check(...) to fail) immediately
after obtaining responses so missing/dropped responses cause the test to fail;
apply this change around the blocks using mcp_session, init_msg, tools_call_msg
and check (including the similar blocks in test_action_name_remapping and
test_error_id_correlation) so each block verifies response length before
accessing responses[1].
- Around line 124-143: The comprehension names = {t["name"] for t in tools} can
raise KeyError before check(...) reports failures; first validate each tool has
the required keys ("name","description","inputSchema") using the existing loop
(or run a preliminary loop that calls check(...) for missing fields) and only
after that build names from t["name"] for all validated tools, referencing the
variables tools, names and the check(...) helper to report missing/extra fields.
---
Nitpick comments:
In `@CLI/MCPToolRegistry.swift`:
- Around line 321-352: The read_text branch in SurfaceTool.execute(arguments:)
duplicates the required-parameter checks and allowlist filtering logic that
GroupedTool already provides; refactor by extracting the validation-and-filter
helper (e.g., a shared method like validateAndFilter(arguments:forAction:) on
GroupedTool or an extension used by both classes) and have SurfaceTool.execute
call that helper to (1) verify the action exists and required params are present
using def.required/def.optional and (2) return the filtered params dictionary
which you then pass to backend.rpc("surface.read_text", params:). Keep the
existing behavior of returning only the "text" field when present and the JSON
fallback, but remove the duplicated loop and guards from SurfaceTool.execute so
all param validation/filtering is centralized.
- Around line 114-121: The current params-building logic (where vars params,
allowed = Set(def.required + def.optional), and the loop for (key, value) in
arguments) silently drops unknown keys; change it to detect any argument keys
not in allowed (and not "action") and reject them (return an error or throw)
instead of ignoring them. Apply the same check inside SurfaceTool.execute’s
read_text branch so that both MCPToolRegistry’s parameter construction and
SurfaceTool.execute validate arguments against def.required/def.optional and
fail fast on unexpected parameters.
In `@tests/test_mcp_server.py`:
- Around line 31-45: The test currently ignores the subprocess exit status and
stderr after running CMUX_BIN in mcp_session; update the code after
subprocess.run (variable proc) to check proc.returncode and, if non-zero, raise
an AssertionError (or fail the test) that includes proc.returncode, proc.stderr
and proc.stdout to make startup failures explicit; keep the existing parsing of
proc.stdout into JSON lines (responses) but only proceed after the
returncode/stderr check so test failures show the subprocess error rather than
downstream JSON parse/assertion noise.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CLI/MCPBackend.swiftCLI/MCPMain.swiftCLI/MCPProtocol.swiftCLI/MCPToolRegistry.swiftCLI/cmux.swifttests/test_mcp_server.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CLI/MCPMain.swift
| guard let response = try JSONSerialization.jsonObject(with: responseData) as? [String: Any] else { | ||
| throw MCPError.executionFailed("Invalid response from socket") | ||
| } | ||
|
|
||
| // cmux socket v2: {ok: bool, result: ..., error: ...} | ||
| guard let ok = response["ok"] as? Bool else { | ||
| throw MCPError.executionFailed("Missing 'ok' field in socket response") | ||
| } |
There was a problem hiding this comment.
Validate response id matches the in-flight request.
sendRPC sends an id but never verifies the response id. A stale/misaligned frame can be accepted as the current response.
🔧 Suggested correlation check
- requestId += 1
+ requestId += 1
+ let currentRequestId = requestId
let request: [String: Any] = [
- "id": requestId,
+ "id": currentRequestId,
"method": method,
"params": params
]
@@
guard let response = try JSONSerialization.jsonObject(with: responseData) as? [String: Any] else {
throw MCPError.executionFailed("Invalid response from socket")
}
+ let responseId = (response["id"] as? NSNumber)?.intValue
+ guard responseId == currentRequestId else {
+ disconnect()
+ throw MCPError.transportError("Mismatched response id (expected \(currentRequestId), got \(String(describing: response["id"])))")
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/MCPBackend.swift` around lines 250 - 257, The response parsing in sendRPC
currently trusts the socket frame without correlating request/response ids;
update sendRPC to extract the response["id"] from the parsed response (response
variable from responseData) and verify it matches the request id you sent (the
id variable used when sending the RPC); if it is missing or does not match,
throw MCPError.executionFailed with a clear message like "Mismatched or missing
response id" to reject stale/aligned frames and only proceed when response["id"]
== id and response["ok"] is true.
There was a problem hiding this comment.
Fixed — error response now extracts and preserves request id from raw JSON. See commit d8ca8d2.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
git show d8ca8d2 -- CLI/MCPBackend.swift
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check recent commits
git log --oneline -10
# Also check current state of sendRPC in MCPBackend.swift
rg -n "responseId|response\[.id.\]|currentRequestId|Mismatched" CLI/MCPBackend.swift
Length of output: 654
🏁 Script executed:
#!/bin/bash
# Show the sendRPC function and surrounding context in current file
rg -n "sendRPC|requestId|response\[" CLI/MCPBackend.swift -A 3
# Also check the diff introduced in d8ca8d2 for MCPBackend.swift via GitHub CLI
gh api repos/manaflow-ai/cmux/commits/d8ca8d2 --jq '.files[] | select(.filename == "CLI/MCPBackend.swift") | .patch'
Length of output: 1717
🏁 Script executed:
#!/bin/bash
# Get list of files changed in d8ca8d2
gh api repos/manaflow-ai/cmux/commits/d8ca8d2 --jq '.files[].filename'
# Get the patch for MCPBackend specifically
gh api repos/manaflow-ai/cmux/commits/d8ca8d2 --jq '.files[] | select(.filename | contains("MCP")) | {file: .filename, patch: .patch}'
Length of output: 2992
@Efan404, thanks for the update! I reviewed commit d8ca8d2 and can see the fix in CLI/MCPMain.swift, which now extracts the request id from the raw JSON before constructing the JSONRPCErrorResponse — that's a good improvement for client-facing error correlation.
However, that addresses a slightly different concern from the original flag. The original issue was about MCPBackend.sendRPC not validating the response id received from the cmux daemon socket. Looking at the current CLI/MCPBackend.swift, sendRPC is still unchanged:
requestId += 1
let request: [String: Any] = [
"id": requestId, // sent to daemon
...
]
// ... writeAll / readLine ...
// response["id"] is never compared to requestId
guard let ok = response["ok"] as? Bool else { ... }To summarize the distinction:
| Concern | File | Status |
|---|---|---|
MCP stdio error response carries correct client request id |
MCPMain.swift |
✅ Fixed in d8ca8d2 |
sendRPC validates daemon response id matches sent request id |
MCPBackend.swift |
❌ Still missing |
The socket-level check in MCPBackend.sendRPC would catch a misaligned frame from the daemon (e.g., a stale buffered response after a reconnect). The suggested fix from the original comment still applies here:
requestId += 1
let currentRequestId = requestId
let request: [String: Any] = [
"id": currentRequestId,
"method": method,
"params": params
]
// ... after parsing response ...
let responseId = (response["id"] as? NSNumber)?.intValue
guard responseId == currentRequestId else {
disconnect()
throw MCPError.transportError(
"Mismatched response id (expected \(currentRequestId), got \(String(describing: response["id"])))"
)
}Would you like to address this remaining check in MCPBackend.swift, or should I open a follow-up issue for it?
🧠 Learnings used
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-03T05:48:04.237Z
Learning: Applies to **/*Socket*.swift : Do not use `DispatchQueue.main.sync` for high-frequency socket telemetry commands (`report_*`, `ports_kick`, status/progress/log metadata updates); parse/validate/dedupe off-main and schedule UI/model mutation with `DispatchQueue.main.async` only when needed
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-03T05:48:04.237Z
Learning: Applies to **/*Socket*.swift : Socket/CLI commands must not steal macOS app focus (no app activation/window raising side effects); only explicit focus-intent commands may mutate in-app focus/selection
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_mcp_server.py">
<violation number="1" location="tests/test_mcp_server.py:346">
P2: The new ID-correlation test does not actually trigger an error response, so it fails to verify the behavior described by the test name/docstring.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| """Test that error responses use the request id, not a fixed id.""" | ||
| print("\n--- test_error_id_correlation ---") | ||
| # Send a request that will fail to a method that doesn't exist | ||
| msgs = [init_msg(1)] |
There was a problem hiding this comment.
P2: The new ID-correlation test does not actually trigger an error response, so it fails to verify the behavior described by the test name/docstring.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_mcp_server.py, line 346:
<comment>The new ID-correlation test does not actually trigger an error response, so it fails to verify the behavior described by the test name/docstring.</comment>
<file context>
@@ -291,6 +296,74 @@ def test_not_initialized():
+ """Test that error responses use the request id, not a fixed id."""
+ print("\n--- test_error_id_correlation ---")
+ # Send a request that will fail to a method that doesn't exist
+ msgs = [init_msg(1)]
+ responses = mcp_session(msgs)
+ if responses:
</file context>
There was a problem hiding this comment.
Fixed — test_error_id_correlation tests that response ids match request ids for integer, string, and large numeric ids. See commit d8ca8d2.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbd5dd78cb
ℹ️ 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".
| // Page | ||
| "screenshot": ActionDef(optional: ["selector", "path", "surface_id"]), | ||
| "snapshot": ActionDef(optional: ["surface_id"]), | ||
| "eval": ActionDef(required: ["expression"], optional: ["surface_id"]), |
There was a problem hiding this comment.
The cmux_browser eval action requires expression, and GroupedTool.execute only forwards whitelisted keys, so callers cannot send script. The socket RPC handler for browser.eval (v2BrowserEval in Sources/TerminalController.swift) rejects requests without script, which means MCP eval calls fail with invalid_params instead of executing JavaScript.
Useful? React with 👍 / 👎.
| "uncheck": ActionDef(required: ["selector"], optional: ["surface_id"]), | ||
| "select": ActionDef(required: ["selector", "value"], optional: ["surface_id"]), | ||
| "focus": ActionDef(required: ["selector"], optional: ["surface_id"]), | ||
| "scroll": ActionDef(optional: ["selector", "x", "y", "surface_id"]), |
There was a problem hiding this comment.
This action forwards x/y, but the backend browser.scroll implementation (v2BrowserScroll in Sources/TerminalController.swift) reads only dx/dy. Because non-whitelisted keys are dropped before dispatch, MCP scroll requests silently send zero deltas and return success without actually scrolling, making scroll-based automation a no-op.
Useful? React with 👍 / 👎.
| "screenshot": ActionDef(optional: ["selector", "path", "surface_id"]), | ||
| "snapshot": ActionDef(optional: ["surface_id"]), | ||
| "eval": ActionDef(required: ["expression"], optional: ["surface_id"]), | ||
| "wait": ActionDef(optional: ["selector", "state", "timeout", "surface_id"]), |
There was a problem hiding this comment.
The wait action only allows state and timeout, but browser.wait consumes load_state and timeout_ms (v2BrowserWait in Sources/TerminalController.swift). Since GroupedTool filters arguments to the allowlist, callers cannot pass the parameters the RPC actually uses, so waits fall back to default behavior (notably 5s timeout) and cannot express load-state waits reliably.
Useful? React with 👍 / 👎.
Add a toggleable MCP Server setting with green/red status indicator. When disabled, `cmux --mcp` exits immediately with a JSON-RPC error. The CLI reads the setting from the app's UserDefaults domain. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rewrite docs/MCP-SERVER.md to reflect the current grouped-tool architecture. Remove stale docs (MCP-BUILD.md, IMPLEMENTATION.md, plans/). Add MCP Server section to README with setup instructions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/MCP-SERVER.md (2)
90-96: Add language identifier to the file structure block.The file structure block lacks a language specifier. Use
```textto satisfy markdownlint (MD040).📝 Suggested fix
-``` +```text CLI/ ├── MCPMain.swift # stdio entry point, CLI integration ├── MCPProtocol.swift # JSON-RPC 2.0 protocol handler ├── MCPToolRegistry.swift # Grouped tool definitions with action validation ├── MCPBackend.swift # Direct Unix socket RPC to cmux daemon └── MCPTypes.swift # JSON-RPC types and MCP message structures</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/MCP-SERVER.mdaround lines 90 - 96, The markdown code block showing the
CLI file structure (lines containing MCPMain.swift, MCPProtocol.swift,
MCPToolRegistry.swift, MCPBackend.swift, MCPTypes.swift) is missing a language
specifier; update the opening fence fromtotext so the block becomes a
text-code block (e.g., use ```text before the "CLI/" line and keep the closing
7-12: Add language identifier to the fenced code block.The ASCII diagram block lacks a language specifier. Use
```textfor plain-text diagrams to satisfy markdownlint (MD040).📝 Suggested fix
-``` +```text +-----------------+ stdio +-----------------+ Unix Socket +--------------+ | AI Tool |<──────────────>| cmux --mcp |<───────────────────>| cmux daemon | | (MCP Client) | | (MCP Server) | | | +-----------------+ +-----------------+ +--------------+</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/MCP-SERVER.mdaround lines 7 - 12, The fenced ASCII diagram block lacks
a language identifier which triggers markdownlint MD040; update the fenced code
block that contains the ASCII diagram (the triple-backtick block starting with
the diagram lines like "+-----------------+ stdio +-----------------+
...") to use a plain-text language specifier by changing the opening fence to
"```text" so the diagram is treated as plain text.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@docs/MCP-SERVER.md:
- Around line 90-96: The markdown code block showing the CLI file structure
(lines containing MCPMain.swift, MCPProtocol.swift, MCPToolRegistry.swift,
MCPBackend.swift, MCPTypes.swift) is missing a language specifier; update the
opening fence fromtotext so the block becomes a text-code block (e.g.,
usetext before the "CLI/" line and keep the closing), which satisfies
markdownlint MD040.- Around line 7-12: The fenced ASCII diagram block lacks a language identifier
which triggers markdownlint MD040; update the fenced code block that contains
the ASCII diagram (the triple-backtick block starting with the diagram lines
like "+-----------------+ stdio +-----------------+ ...") to use a
plain-text language specifier by changing the opening fence to "```text" so the
diagram is treated as plain text.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `81c11004-a72f-4ff6-bd7a-b90887937823` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between cbd5dd78cb6ad6cb3ccafdf9e36c86d152682df0 and f63197ed7f1e39599fe18d1efcc99e7f033447c3. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `CLI/cmux.swift` * `README.md` * `Sources/cmuxApp.swift` * `docs/MCP-SERVER.md` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * CLI/cmux.swift </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f63197ed7f
ℹ️ 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".
| // Run MCP server mode | ||
| let resolvedPassword = SocketPasswordResolver.resolve(explicit: socketPasswordArg) | ||
| runMCPServer(socketPath: socketPath, password: resolvedPassword) | ||
| return |
There was a problem hiding this comment.
This branch launches MCP mode immediately when --mcp is encountered, so any flags that appear later in the argv list are never parsed. That means documented/typical invocations like cmux --mcp --socket ... --password ... will silently ignore the custom socket/password and try defaults instead, breaking MCP connections in non-default setups.
Useful? React with 👍 / 👎.
| var pollFd = pollfd(fd: socketFd, events: Int16(POLLIN), revents: 0) | ||
| let pollResult = poll(&pollFd, 1, 30_000) // 30 seconds | ||
| if pollResult == 0 { | ||
| disconnect() | ||
| throw MCPError.transportError("Socket read timed out after 30s") |
There was a problem hiding this comment.
The backend enforces a fixed 30-second read timeout for every RPC response, regardless of the tool/action semantics. MCP actions such as long browser.wait/download.wait flows can legitimately exceed 30 seconds, so these calls will fail with a transport timeout even when the daemon is still processing, which makes longer automations unreliable.
Useful? React with 👍 / 👎.
Resolves conflicts in: - CLI/cmux.swift: Keep mcpServerIsEnabled() + new CLISocketPathResolver from main - Sources/cmuxApp.swift: Keep MCP Server settings card + localized Port Base row from main - GhosttyTabs.xcodeproj/project.pbxproj: Restore MCP file references (MCPBackend, MCPMain, MCPProtocol, MCPToolRegistry, MCPTypes) on top of main's MarkdownUI/xcstrings additions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…timeout P2: Fix --mcp flag consuming later arguments (socket/password) by deferring MCP mode activation until after all flags are parsed. Previously launching MCP mode immediately on --mcp meant --socket and --password flags that followed were silently ignored. P1: Replace hard-coded 30s socket read timeout with per-action configurable timeout. readLine/sendRPC/rpc/rpcForTool now accept a timeoutMs parameter. Default is 120s; long-running actions (browser.wait, download.wait) use 300s. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
CLI/MCPBackend.swift (1)
235-258:⚠️ Potential issue | 🟠 MajorCorrelate response
idwith the in-flight request.
sendRPCsends anidbut never verifiesresponse["id"]; stale/misaligned frames can be accepted as the current call result.🧩 Suggested correlation check
- requestId += 1 + requestId += 1 + let currentRequestId = requestId let request: [String: Any] = [ - "id": requestId, + "id": currentRequestId, "method": method, "params": params ] @@ guard let response = try JSONSerialization.jsonObject(with: responseData) as? [String: Any] else { throw MCPError.executionFailed("Invalid response from socket") } + let responseId = (response["id"] as? NSNumber)?.intValue + guard responseId == currentRequestId else { + disconnect() + throw MCPError.transportError( + "Mismatched response id (expected \(currentRequestId), got \(String(describing: response["id"])))" + ) + } // cmux socket v2: {ok: bool, result: ..., error: ...}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/MCPBackend.swift` around lines 235 - 258, sendRPC currently writes a request with requestId but doesn't verify the response id, so correlate responses: after parsing response (in sendRPC where requestId, writeAll(_:), readLine(timeoutMs:) and JSON deserialization are used), extract response["id"] and check it equals the requestId you sent; if it doesn't match, either throw a specific MCPError.executionFailed indicating an unexpected id or loop to continue reading frames (using readLine(timeoutMs:)) until you find a response whose "id" matches the requestId (respecting the same timeout), and only then proceed to validate the "ok" field and return the result; ensure any mismatched frames are ignored or logged and do not get treated as this call's response.
🧹 Nitpick comments (1)
CLI/MCPToolRegistry.swift (1)
118-125: Reject unknown parameters instead of silently dropping them.Current filtering hides caller mistakes (
foo_idtypos become no-ops). For stricter validation, fail fast on unknown keys.✅ Suggested tightening
// Build params dict (everything except "action") var params: [String: Any] = [:] let allowed = Set(def.required + def.optional) + let unknown = Set(arguments.keys).subtracting(Set(["action"])).subtracting(allowed) + guard unknown.isEmpty else { + throw MCPError.invalidParameters( + "Unknown parameter(s) for action '\(action)': \(unknown.sorted().joined(separator: ", "))" + ) + } for (key, value) in arguments where key != "action" { if allowed.contains(key) { params[key] = value } }Also applies to: 341-345
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/MCPToolRegistry.swift` around lines 118 - 125, The code currently drops unknown argument keys when building params (see vars params, allowed and the for-loop over arguments), which hides mistakes; instead detect any key (except "action") not in allowed (def.required + def.optional) and fail fast by returning/throwing a validation error (or calling the existing CLI error path) with a clear message listing the unknown keys; apply the same change to the other occurrence that builds params (around the second block at 341-345) so both places validate and reject unknown parameters rather than silently ignoring them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 885-900: In the mcpMode branch, replace use of the raw socketPath
with the CLISocketPathResolver.resolve(...) call so MCP mode discovers the same
implicit/default/tagged sockets as CLI mode and call
CLISocketPathResolver.resolve on socketPath before resolving password (alongside
existing SocketPasswordResolver.resolve(explicit: socketPasswordArg)), then
validate the resolved path to reject any trailing CLI tokens (e.g., treat extra
args after --mcp as an error) before invoking runMCPServer(socketPath: ...,
password: ...); update the logic in the mcpMode block that currently references
socketPath and runMCPServer to use the resolved path and to fail early on
trailing tokens.
In `@CLI/MCPBackend.swift`:
- Around line 63-69: The current validation uses stat(...) and only checks
st_uid, which can be bypassed via symlinks or non-socket files; update the check
in the connect path that uses socketPath by calling lstat(...) (or lstat first
then stat if needed) and verify the file type via st.st_mode & S_IFMT ==
S_IFSOCK (or st_mode & S_IFSOCK) before checking ownership, and on failure throw
MCPError.executionFailed with a clear message that the path is not a socket
(matching the behavior in CLI/cmux.swift); keep the existing ownership check
(st.st_uid == getuid()) after the socket-type validation.
- Around line 185-192: On EOF (bytesRead == 0) inside the read loop in
MCPBackend.swift, don't allow a non-empty buffer to be returned as a partial
frame; instead after calling disconnect() detect if buffer.isEmpty is false and
throw a transport-level error (use MCPError.transportError with a clear message
like "Socket closed with partial frame") so the caller treats this as a
transport failure and triggers reconnect logic; locate the bytesRead == 0
branch, the buffer variable, the disconnect() call and replace the current
behavior that proceeds with a partial payload by throwing
MCPError.transportError.
In `@CLI/MCPToolRegistry.swift`:
- Around line 95-108: Replace raw user-facing string literals in MCPToolRegistry
with localized variants using the required API; e.g., in execute(arguments:)
replace the action description and the thrown MCPError.invalidParameters
messages with String(localized: "mcp.action.description", defaultValue: "The
action to perform. See tool description for available actions.") and
String(localized: "mcp.error.missing_action", defaultValue: "Missing required
parameter: action") / String(localized: "mcp.error.unknown_action",
defaultValue: "Unknown action '%@'. Available: %@") (format the unknown-action
message with action and available), and similarly convert other user-facing
literals in the file (including the other occurrences you noted) to
String(localized:..., defaultValue:...) using sensible key names for each
message.
- Around line 333-347: The read_text invocation currently ignores per-action
timeouts: when performing backend.rpc(method: "surface.read_text", params:
params) we must pass the ActionDef.timeoutMs from the matched action definition
(found as def from actions[action]) so the RPC honors the action-specific
timeout; update the call site in MCPToolRegistry (where def is defined and
params gathered) to include the timeout (or deadline) derived from def.timeoutMs
(convert ms to the RPC timeout unit expected) and ensure fallback to a default
if timeout is nil.
---
Duplicate comments:
In `@CLI/MCPBackend.swift`:
- Around line 235-258: sendRPC currently writes a request with requestId but
doesn't verify the response id, so correlate responses: after parsing response
(in sendRPC where requestId, writeAll(_:), readLine(timeoutMs:) and JSON
deserialization are used), extract response["id"] and check it equals the
requestId you sent; if it doesn't match, either throw a specific
MCPError.executionFailed indicating an unexpected id or loop to continue reading
frames (using readLine(timeoutMs:)) until you find a response whose "id" matches
the requestId (respecting the same timeout), and only then proceed to validate
the "ok" field and return the result; ensure any mismatched frames are ignored
or logged and do not get treated as this call's response.
---
Nitpick comments:
In `@CLI/MCPToolRegistry.swift`:
- Around line 118-125: The code currently drops unknown argument keys when
building params (see vars params, allowed and the for-loop over arguments),
which hides mistakes; instead detect any key (except "action") not in allowed
(def.required + def.optional) and fail fast by returning/throwing a validation
error (or calling the existing CLI error path) with a clear message listing the
unknown keys; apply the same change to the other occurrence that builds params
(around the second block at 341-345) so both places validate and reject unknown
parameters rather than silently ignoring them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9485b9ae-a1bb-41d4-8bf6-3260ba9f424f
📒 Files selected for processing (5)
CLI/MCPBackend.swiftCLI/MCPToolRegistry.swiftCLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojSources/cmuxApp.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/cmuxApp.swift
| if mcpMode { | ||
| // Check if MCP server is enabled in app settings | ||
| if !mcpServerIsEnabled() { | ||
| FileHandle.standardError.write( | ||
| "MCP server is disabled in cmux settings.\n".data(using: .utf8) ?? Data() | ||
| ) | ||
| let errorResponse = """ | ||
| {"jsonrpc":"2.0","id":null,"error":{"code":-32000,"message":"MCP server is disabled in cmux settings. Enable it in Settings > Automation."}} | ||
| """ | ||
| print(errorResponse) | ||
| Foundation.exit(1) | ||
| } | ||
| // Run MCP server mode (all flags like --socket/--password are fully parsed above) | ||
| let resolvedPassword = SocketPasswordResolver.resolve(explicit: socketPasswordArg) | ||
| runMCPServer(socketPath: socketPath, password: resolvedPassword) | ||
| return |
There was a problem hiding this comment.
Resolve MCP socket path with the same resolver and reject trailing tokens.
At Line 899, MCP mode uses raw socketPath instead of CLISocketPathResolver.resolve(...), so implicit-default MCP runs can miss tagged/debug sockets that normal CLI mode discovers. Also, because this branch runs before command validation, tokens after --mcp (e.g., cmux --mcp ping) are silently ignored.
🔧 Proposed patch
if mcpMode {
+ guard index >= args.count else {
+ throw CLIError(message: "--mcp does not accept additional command arguments")
+ }
// Check if MCP server is enabled in app settings
if !mcpServerIsEnabled() {
FileHandle.standardError.write(
"MCP server is disabled in cmux settings.\n".data(using: .utf8) ?? Data()
)
@@
}
// Run MCP server mode (all flags like --socket/--password are fully parsed above)
let resolvedPassword = SocketPasswordResolver.resolve(explicit: socketPasswordArg)
- runMCPServer(socketPath: socketPath, password: resolvedPassword)
+ let resolvedSocketPath = CLISocketPathResolver.resolve(
+ requestedPath: socketPath,
+ source: socketPathSource,
+ environment: processEnv
+ )
+ runMCPServer(socketPath: resolvedSocketPath, password: resolvedPassword)
return
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 885 - 900, In the mcpMode branch, replace use of
the raw socketPath with the CLISocketPathResolver.resolve(...) call so MCP mode
discovers the same implicit/default/tagged sockets as CLI mode and call
CLISocketPathResolver.resolve on socketPath before resolving password (alongside
existing SocketPasswordResolver.resolve(explicit: socketPasswordArg)), then
validate the resolved path to reject any trailing CLI tokens (e.g., treat extra
args after --mcp as an error) before invoking runMCPServer(socketPath: ...,
password: ...); update the logic in the mcpMode block that currently references
socketPath and runMCPServer to use the resolved path and to fail early on
trailing tokens.
| var st = stat() | ||
| guard stat(socketPath, &st) == 0 else { | ||
| throw MCPError.executionFailed("Socket not found at \(socketPath)") | ||
| } | ||
| guard st.st_uid == getuid() else { | ||
| throw MCPError.executionFailed("Socket at \(socketPath) is not owned by the current user — refusing to connect") | ||
| } |
There was a problem hiding this comment.
Harden socket-path trust checks before connect().
At Line 64, stat + UID-only validation can still trust the wrong filesystem object type. Add lstat + S_IFSOCK verification so symlinks/non-socket files fail early (this also aligns behavior with CLI/cmux.swift).
🔒 Suggested hardening
- var st = stat()
- guard stat(socketPath, &st) == 0 else {
+ var st = stat()
+ guard lstat(socketPath, &st) == 0 else {
throw MCPError.executionFailed("Socket not found at \(socketPath)")
}
+ guard (st.st_mode & mode_t(S_IFMT)) == mode_t(S_IFSOCK) else {
+ throw MCPError.executionFailed("Path exists at \(socketPath) but is not a Unix socket")
+ }
guard st.st_uid == getuid() else {
throw MCPError.executionFailed("Socket at \(socketPath) is not owned by the current user — refusing to connect")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var st = stat() | |
| guard stat(socketPath, &st) == 0 else { | |
| throw MCPError.executionFailed("Socket not found at \(socketPath)") | |
| } | |
| guard st.st_uid == getuid() else { | |
| throw MCPError.executionFailed("Socket at \(socketPath) is not owned by the current user — refusing to connect") | |
| } | |
| var st = stat() | |
| guard lstat(socketPath, &st) == 0 else { | |
| throw MCPError.executionFailed("Socket not found at \(socketPath)") | |
| } | |
| guard (st.st_mode & mode_t(S_IFMT)) == mode_t(S_IFSOCK) else { | |
| throw MCPError.executionFailed("Path exists at \(socketPath) but is not a Unix socket") | |
| } | |
| guard st.st_uid == getuid() else { | |
| throw MCPError.executionFailed("Socket at \(socketPath) is not owned by the current user — refusing to connect") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/MCPBackend.swift` around lines 63 - 69, The current validation uses
stat(...) and only checks st_uid, which can be bypassed via symlinks or
non-socket files; update the check in the connect path that uses socketPath by
calling lstat(...) (or lstat first then stat if needed) and verify the file type
via st.st_mode & S_IFMT == S_IFSOCK (or st_mode & S_IFSOCK) before checking
ownership, and on failure throw MCPError.executionFailed with a clear message
that the path is not a socket (matching the behavior in CLI/cmux.swift); keep
the existing ownership check (st.st_uid == getuid()) after the socket-type
validation.
| if bytesRead == 0 { | ||
| // EOF — connection closed by daemon | ||
| disconnect() | ||
| if buffer.isEmpty { | ||
| throw MCPError.transportError("Socket connection closed") | ||
| } | ||
| break | ||
| } |
There was a problem hiding this comment.
Don’t return partial frames when the daemon closes the socket.
At Line 185-Line 192, EOF with a non-empty buffer currently returns partial payload, which then fails later as a non-transport parse error and skips the reconnect path.
🧯 Suggested fix
if bytesRead == 0 {
// EOF — connection closed by daemon
disconnect()
- if buffer.isEmpty {
- throw MCPError.transportError("Socket connection closed")
- }
- break
+ throw MCPError.transportError(
+ buffer.isEmpty
+ ? "Socket connection closed"
+ : "Socket connection closed mid-response"
+ )
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/MCPBackend.swift` around lines 185 - 192, On EOF (bytesRead == 0) inside
the read loop in MCPBackend.swift, don't allow a non-empty buffer to be returned
as a partial frame; instead after calling disconnect() detect if buffer.isEmpty
is false and throw a transport-level error (use MCPError.transportError with a
clear message like "Socket closed with partial frame") so the caller treats this
as a transport failure and triggers reconnect logic; locate the bytesRead == 0
branch, the buffer variable, the disconnect() call and replace the current
behavior that proceeds with a partial payload by throwing
MCPError.transportError.
| "action": MCPToolProperty(type: "string", description: "The action to perform. See tool description for available actions.") | ||
| ], | ||
| required: ["action"] | ||
| ) | ||
| } | ||
|
|
||
| public func execute(arguments: [String: Any]) throws -> MCPToolCallResult { | ||
| guard let action = arguments["action"] as? String else { | ||
| throw MCPError.invalidParameters("Missing required parameter: action") | ||
| } | ||
|
|
||
| guard let def = actions[action] else { | ||
| let available = actions.keys.sorted().joined(separator: ", ") | ||
| throw MCPError.invalidParameters("Unknown action '\(action)'. Available: \(available)") |
There was a problem hiding this comment.
Localize tool descriptions and validation error strings.
These strings are user-facing via MCP tool metadata/errors and are currently raw literals.
As per coding guidelines, **/*.swift: "All user-facing strings must be localized using String(localized: "key.name", defaultValue: "English text") ... error messages".
Also applies to: 146-153, 327-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/MCPToolRegistry.swift` around lines 95 - 108, Replace raw user-facing
string literals in MCPToolRegistry with localized variants using the required
API; e.g., in execute(arguments:) replace the action description and the thrown
MCPError.invalidParameters messages with String(localized:
"mcp.action.description", defaultValue: "The action to perform. See tool
description for available actions.") and String(localized:
"mcp.error.missing_action", defaultValue: "Missing required parameter: action")
/ String(localized: "mcp.error.unknown_action", defaultValue: "Unknown action
'%@'. Available: %@") (format the unknown-action message with action and
available), and similarly convert other user-facing literals in the file
(including the other occurrences you noted) to String(localized:...,
defaultValue:...) using sensible key names for each message.
| guard let def = actions[action] else { | ||
| throw MCPError.invalidParameters("Unknown action '\(action)'") | ||
| } | ||
| for param in def.required { | ||
| guard arguments[param] != nil else { | ||
| throw MCPError.invalidParameters("Action '\(action)' requires parameter: \(param)") | ||
| } | ||
| } | ||
| var params: [String: Any] = [:] | ||
| let allowed = Set(def.required + def.optional) | ||
| for (key, value) in arguments where key != "action" { | ||
| if allowed.contains(key) { params[key] = value } | ||
| } | ||
| let result = try backend.rpc(method: "surface.read_text", params: params) | ||
| if let text = result["text"] as? String { |
There was a problem hiding this comment.
Honor per-action timeout in the read_text override.
At Line 346, the override bypasses ActionDef.timeoutMs, so timeout tuning for read_text can drift from the action definition.
⏱️ Suggested fix
- let result = try backend.rpc(method: "surface.read_text", params: params)
+ let result = try backend.rpc(method: "surface.read_text", params: params, timeoutMs: def.timeoutMs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| guard let def = actions[action] else { | |
| throw MCPError.invalidParameters("Unknown action '\(action)'") | |
| } | |
| for param in def.required { | |
| guard arguments[param] != nil else { | |
| throw MCPError.invalidParameters("Action '\(action)' requires parameter: \(param)") | |
| } | |
| } | |
| var params: [String: Any] = [:] | |
| let allowed = Set(def.required + def.optional) | |
| for (key, value) in arguments where key != "action" { | |
| if allowed.contains(key) { params[key] = value } | |
| } | |
| let result = try backend.rpc(method: "surface.read_text", params: params) | |
| if let text = result["text"] as? String { | |
| guard let def = actions[action] else { | |
| throw MCPError.invalidParameters("Unknown action '\(action)'") | |
| } | |
| for param in def.required { | |
| guard arguments[param] != nil else { | |
| throw MCPError.invalidParameters("Action '\(action)' requires parameter: \(param)") | |
| } | |
| } | |
| var params: [String: Any] = [:] | |
| let allowed = Set(def.required + def.optional) | |
| for (key, value) in arguments where key != "action" { | |
| if allowed.contains(key) { params[key] = value } | |
| } | |
| let result = try backend.rpc(method: "surface.read_text", params: params, timeoutMs: def.timeoutMs) | |
| if let text = result["text"] as? String { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/MCPToolRegistry.swift` around lines 333 - 347, The read_text invocation
currently ignores per-action timeouts: when performing backend.rpc(method:
"surface.read_text", params: params) we must pass the ActionDef.timeoutMs from
the matched action definition (found as def from actions[action]) so the RPC
honors the action-specific timeout; update the call site in MCPToolRegistry
(where def is defined and params gathered) to include the timeout (or deadline)
derived from def.timeoutMs (convert ms to the RPC timeout unit expected) and
ensure fallback to a default if timeout is nil.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2177b16c3
ℹ️ 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".
| // Page | ||
| "screenshot": ActionDef(optional: ["selector", "path", "surface_id"]), | ||
| "snapshot": ActionDef(optional: ["surface_id"]), | ||
| "eval": ActionDef(required: ["expression"], optional: ["surface_id"]), |
There was a problem hiding this comment.
script parameter for browser eval action
The cmux_browser schema currently requires expression, but the socket handler for browser.eval reads script (v2BrowserEval in Sources/TerminalController.swift). Because GroupedTool.execute only forwards whitelisted keys, MCP clients following this schema cannot send script, so every eval call fails with invalid_params/"Missing script" instead of executing JavaScript.
Useful? React with 👍 / 👎.
| "state.save": ActionDef(required: ["name"], optional: ["surface_id"]), | ||
| "state.load": ActionDef(required: ["name"], optional: ["surface_id"]), |
There was a problem hiding this comment.
path for browser state save/load actions
Both state.save and state.load advertise name as the required argument, but the corresponding socket handlers (v2BrowserStateSave/v2BrowserStateLoad in Sources/TerminalController.swift) require path. Since unknown keys are filtered before dispatch, callers cannot pass path through this tool definition, so both actions are effectively broken and return missing-path errors.
Useful? React with 👍 / 👎.
| "uncheck": ActionDef(required: ["selector"], optional: ["surface_id"]), | ||
| "select": ActionDef(required: ["selector", "value"], optional: ["surface_id"]), | ||
| "focus": ActionDef(required: ["selector"], optional: ["surface_id"]), | ||
| "scroll": ActionDef(optional: ["selector", "x", "y", "surface_id"]), |
There was a problem hiding this comment.
dx/dy keys
The scroll action accepts x/y, but the underlying socket method (v2BrowserScroll in Sources/TerminalController.swift) reads dx/dy. As a result, valid MCP calls pass no recognized deltas, and the action defaults to 0 movement, so scroll requests silently do nothing.
Useful? React with 👍 / 👎.
|
|
||
| SettingsCard { | ||
| SettingsCardRow( | ||
| "MCP Server", |
There was a problem hiding this comment.
This settings card introduces hard-coded English UI text instead of localized strings, which violates the repository rule in /workspace/cmux/AGENTS.md (“All user-facing strings must be localized …”). In non-English locales (including the documented Japanese support), these labels/subtitles remain untranslated while the rest of Settings is localized.
Useful? React with 👍 / 👎.
| ### Development Guide | ||
|
|
||
| #### Prerequisites | ||
|
|
||
| - **Xcode** - Full Xcode installation (not just Command Line Tools) | ||
| ```bash | ||
| sudo xcode-select --switch /Applications/Xcode.app/Contents/Developer | ||
| ``` | ||
| - **zig 0.14.x** - Required for building GhosttyKit | ||
| ```bash | ||
| brew install zig@0.14 | ||
| ``` | ||
| - **xcodeproj gem** - For adding files to Xcode project | ||
| ```bash | ||
| sudo gem install xcodeproj | ||
| ``` | ||
|
|
||
| #### Initial Setup | ||
|
|
||
| ```bash | ||
| # Initialize submodules | ||
| git submodule update --init --recursive | ||
|
|
||
| # Build GhosttyKit (required for full app) | ||
| cd ghostty | ||
| zig build -Demit-xcframework=true -Doptimize=ReleaseFast | ||
| cd .. | ||
| ``` | ||
|
|
||
| #### Building | ||
|
|
||
| ```bash | ||
| # Build CLI only (fastest, for testing CLI changes) | ||
| xcodebuild -project GhosttyTabs.xcodeproj \ | ||
| -scheme cmux-cli \ | ||
| -configuration Debug \ | ||
| -destination 'platform=macOS' \ | ||
| build | ||
|
|
||
| # Build full app | ||
| xcodebuild -project GhosttyTabs.xcodeproj \ | ||
| -scheme cmux \ | ||
| -configuration Debug \ | ||
| -destination 'platform=macOS' \ | ||
| build | ||
|
|
||
| # Build with tag (for parallel development) | ||
| ./scripts/reload.sh --tag your-feature | ||
| ``` | ||
|
|
||
| #### Adding New Code | ||
|
|
||
| When adding new Swift files to the project, you need to add them to the Xcode target: | ||
|
|
||
| ```bash | ||
| # Using Ruby xcodeproj | ||
| ruby -e ' | ||
| require "xcodeproj" | ||
|
|
||
| project = Xcodeproj::Project.open("GhosttyTabs.xcodeproj") | ||
| target = project.targets.find { |t| t.name == "cmux-cli" } | ||
| cli_group = project.main_group.find_subpath("CLI", true) | ||
|
|
||
| files = ["YourNewFile.swift"] | ||
| files.each do |file| | ||
| file_ref = cli_group.new_file(file) | ||
| target.add_file_references([file_ref]) | ||
| end | ||
|
|
||
| project.save | ||
| ' | ||
| ``` | ||
|
|
||
| #### Running and Testing | ||
|
|
||
| ```bash | ||
| # Binary location after build | ||
| ~/Library/Developer/Xcode/DerivedData/GhosttyTabs-*/Build/Products/Debug/cmux | ||
|
|
||
| # Run in MCP mode (for testing MCP tools) | ||
| ./cmux --mcp | ||
|
|
||
| # Run reload script to launch Debug app | ||
| ./scripts/reload.sh --tag your-feature | ||
| ``` | ||
|
|
||
| --- | ||
|
|
|
Is it possible to disable Claude Code auto-discovery via settings? |
|
@claude Is it possible to disable Claude Code auto-discovery via settings? |
|
This is a broad MCP integration and needs a team design and security pass before landing. Leaving it open for that decision. :) |
teamleaderleo
added
area: agents
Summary
Changes
docs: Add MCP Server build guidefeat: Add MCP Server implementationchore: Remove proxy from README development guidedocs: Add MCP server redesign plansrefactor: Rewrite MCPBackend from CLI subprocess to direct socket RPCfix: Use pre-encoded JSON Data in JSONRPCResponserefactor: Propagate encoding errors and simplify stdio looprefactor: Replace individual tools with grouped action-based toolstest: Add MCP server and socket RPC test scriptsTest plan
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS' buildpython3 tests/test_mcp_server.pypython3 tests/test_mcp_socket_rpc.pySummary by cubic
Adds a built-in MCP server with direct Unix socket RPC and grouped tools, replacing CLI subprocess calls. Adds a Settings toggle to enable/disable the server; per-action timeouts and CLI fixes make long operations and flag parsing reliable.
New Features
Bug Fixes
Written for commit c2177b1. Summary will update on new commits.
Summary by CodeRabbit