Stop MCP server processes when the app quits - #209
Open
C-K-Loan wants to merge 12 commits into
Open
Conversation
Bring the MCP engine (client/config/host) and the Extensions capability hub (Extensions/MCP/Tools/Skills) onto main's extension platform. Add mcpServers, disabledToolNames, and skills to NativSettings, render ExtensionsHubView in the Extensions tab, and add the mcp-swift-sdk package. Chat tool-source integration into the existing tool loop is the remaining seam.
Thread the MCP host into the chat view model; advertise enabled MCP tool definitions alongside the native registry; route MCP tool calls through the host in the existing execution loop; and inject the built-in tool-use skill (plus enabled user skills) into the system prompt when tools are available.
The community catalog is now Sources/Nativ/Resources/MCPCatalog.json instead of a hardcoded Swift array. A new Verify MCP Catalog workflow launches every entry over stdio, completes an MCP handshake, and requires tools/list to return at least one tool before it can merge, so any contributed server must pass CI. Docs/mcp-catalog.md documents the contribution flow.
Add Sources/Nativ/Features/Shared/NativComponents.swift with reusable, flat primitives (status tone/dot/badge, code block, tinted icon tile, hover close button) and adopt them where the MCP work had rolled its own. Chat tool cells now carry status-driven color and a Done/Failed/Cancelled badge, pretty-print argument and result JSON, and tag calls that come from an MCP server. The MCP server rows, catalog logos, and catalog close button reuse the same primitives.
git, fetch, and sqlite resolve their own dependencies at launch and broke against the newest MCP SDK (McpError renamed, low-level Server API changed). Pin them via uvx --with so the app and the verify-mcp-catalog check both launch a known-good combination. Documented the pattern in Docs/mcp-catalog.md.
A Kit is a curated bundle of MCP servers, their tools, skills, and extensions for a role (Engineering, Research, Sales, Operations). Enabling one turns all its pieces on at once and records it; each piece stays individually manageable from a detail sheet. Kits are the marquee section at the top of the hub. NativKit.swift is the code-level registry contributors extend — add a new kit, or fold a newly added MCP server/skill/extension into an existing one; a kit references catalog server ids, inline skills with stable UUIDs, and extension ids. Docs/kits.md documents authoring. Also shares a Color.nativTint helper and exposes MCPCatalogEntry.catalog so kits resolve catalog servers.
It drives main-actor state (model.settings, extension manager), and its callers are all SwiftUI views, so mark the enum @mainactor.
A server with no command can't launch, so hide such entries from the MCP list and require a command (not just a name) before Save is enabled — no empty 'New Server' rows. Change the Extensions nav icon to a connected-nodes glyph.
The unit-test target lists NativSettings.swift explicitly, and that file now holds a `skills: [NativSkill]` property, but NativSkill.swift itself was never added alongside it — so the test target fails to compile and the whole suite is skipped rather than reported as failing. Adding the one missing source file brings the 216 existing tests back.
MCP tool calls went straight from the model to the server with no approval step, while switch_model — the only other tool that reaches outside the chat — has always asked first. A server configured for filesystem or shell access could therefore write or delete without the user seeing anything until it was done. The existing consent branch was hardcoded to switch_model. It now asks ChatToolConsentRequirement what a call needs, so both tools share one gate and the approve/decline/cancel handling is written once. switch_model's behaviour is unchanged; an approved MCP call falls through to the same dispatch path it already used. The gate matches MCP tools on their name prefix rather than on whether the host can currently route them. Connections reload asynchronously and debounced, so a name that looks unroutable at gate time can be routable by the time it is dispatched — matching on the name alone closes that window and keeps an unrecognized or misconfigured server gated instead of letting it through. The prompt now describes what it is actually approving: the tool and the server it belongs to, rather than switch_model's "the server restarts briefly" wording, which was shown for every gated call.
MCPHostManager.shutdown() had no callers, so every MCP server a session started outlived the app — quitting Nativ left npx and uvx children running until they were killed by hand. Wiring it up needed two fixes. The manager was a @StateObject owned by ControlPanelView, so nothing outside the view hierarchy could reach it at termination; it now sits alongside the other app-lifetime objects on AppDelegate and is passed down the same way extensionManager already is, which also stops a rebuilt view from silently spawning a second set of servers. The second is that shutdown() handed its disconnects to a detached task. Even called from applicationWillTerminate that loses the race — the app exits before the task runs — so the children survived anyway. MCPClient now keeps its process behind a lock where a nonisolated caller can reach it, and shutdown() terminates synchronously.
The shutdown work had no tests. Mocking the transport would not have helped — the bug it fixes is that a real child process outlives the app, which a mock cannot reproduce. A fixture server completes the MCP handshake, writes its pid, and then just blocks on stdin. The tests read that pid and watch the actual OS process, so they assert the child died rather than trusting the client's own bookkeeping about what it thinks it terminated. Both levels are covered: MCPClient.terminateImmediately() directly, and MCPHostManager.shutdown() with two connected servers, which is what applicationWillTerminate calls. Restoring either to its previous form fails these — the manager test reports both servers surviving. The manager test's poll is synchronous on purpose, and that is the part worth not simplifying later: the old shutdown() handed its work to a Task created in a main-actor context, so blocking the main actor is what makes this a stand-in for termination. Awaiting instead would let that task run and the test would pass against the bug it exists to catch. The fixture echoes back whichever protocol version the client asks for rather than pinning one, so an SDK bump does not silently break it.
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #207 and #208 (combined diff until those merge, same fork-base limitation). New commits here: 93ed3d5 wires MCPHostManager.shutdown() into app termination (it existed but nothing called it, and the detached-Task version lost the race against app exit anyway — now synchronous), d4cdeba covers it with tests against a real subprocess, watching its actual pid rather than trusting the client's own bookkeeping.