Add consent gating + process lifecycle to mcp-hub - #206
Closed
C-K-Loan wants to merge 4 commits into
Closed
Conversation
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.
Open
Collaborator
|
sweet, could u make this a stacked pr, easier to handle conflicts |
Contributor
Author
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.
Adds the two things flagged on #200: a consent gate for MCP tool calls (fail-closed, including when a server can't be identified) and wiring
MCPHostManager.shutdown()into app termination, since it existed but nothing called it — both covered with tests against real subprocesses. Also fixes the test target, which wasn't compiling (missing NativSkill.swift), so the existing suite was being silently skipped. Nothing else touched.