Fix External MCP server bugs; add npx/Node.js detection and install help
Root-caused two real issues Rune hit with Obsidian/Homepage external MCP servers: 1. Toggling a server's enable switch silently wiped transportKind/env/ url/bearerToken/headers back to stdio defaults (only id/name/command/ args/isEnabled/timeout/createdAt were preserved) — almost certainly how Obsidian's config got corrupted into an empty-command stdio entry despite never being edited directly. Fixed via ExternalMCPServer.withEnabledToggled(), which flips only isEnabled. 2. npx (installed via Homebrew) was invisible to Confab because GUI apps only inherit launchd's minimal PATH, not the Terminal PATH. Tried spawning the user's login shell to ask for its real PATH — this caused two real hangs in one session (first an -ilc pipe deadlock, then a waitUntilExit()/CFRunLoop reentrancy issue even after fixing that) and was abandoned entirely in favor of LoginShellEnvironment: deterministic, subprocess-free directory probing (Homebrew, MacPorts, Volta, nvm's alias file) that can't hang by construction. Also added: - Edit capability for existing External MCP servers (previously only Add/Toggle/Delete) — the second thing Rune explicitly asked for, and the way to fix a corrupted entry like Obsidian's without deleting it. - MCPClientError.commandNotFound: a stdio server's command is checked against PATH up front in StdioMCPTransport.prepare() and fails immediately with a clear reason instead of cycling through 3 rounds of crash/restart backoff (5s/15s/30s) for a permanently-missing binary. - A "Get Node.js" button appears when this happens, opening a sheet with a copyable `brew install node`, a one-click install (via NodeInstallHelper, using the terminationHandler/readabilityHandler pattern already proven safe elsewhere in this file — deliberately not waitUntilExit()), or a nodejs.org link if Homebrew isn't present. - ExternalMCPManager.retryClient(id:) to manually retry after fixing the underlying cause. - Help book: new "Servers That Use npx" section, updated Server Status section, updated Settings blurb. 37 new/changed tests covering the toggle fix, PATH probing, the commandNotFound fast-fail path, and missing-command detection — full suite (374 tests) passes clean.
This commit is contained in:
@@ -39,6 +39,84 @@ enum MCPTransportSupport {
|
||||
}
|
||||
}
|
||||
|
||||
// MARK: - Login Shell PATH Resolution
|
||||
|
||||
/// GUI apps launched from Finder/Dock/LaunchServices inherit launchd's minimal PATH
|
||||
/// (`/usr/bin:/bin:/usr/sbin:/sbin`), not the interactive-shell PATH a Terminal session gets — so
|
||||
/// `npx`/`node` installed via nvm, Homebrew, Volta etc. are invisible to a bare `/usr/bin/env <cmd>`
|
||||
/// spawn, even though the same command works fine when the user runs it themselves in Terminal.
|
||||
///
|
||||
/// History: the obvious fix — spawn the user's real login shell and ask it for `$PATH` — caused two
|
||||
/// real hangs in one session and was abandoned. First with `-ilc` (interactive login), which
|
||||
/// deadlocked on a full pipe buffer from shell startup noise (fixed by draining concurrently).
|
||||
/// After that fix, it hung *again* with non-interactive `-lc`: a live `sample` of the stuck process
|
||||
/// showed `NSTask.waitUntilExit()` still parked inside a nested CFRunLoop even though the child
|
||||
/// shell process was already gone from the process list — a CFRunLoop/SIGCHLD-notification
|
||||
/// reentrancy issue specific to calling it from a Swift-concurrency worker thread inside this app's
|
||||
/// XCTest host process. Spawning any subprocess just to read one environment variable is exposed to
|
||||
/// this whole class of platform quirk, and a subprocess-based timeout can't protect against a hang
|
||||
/// inside the *notification mechanism itself*. Instead: probe the fixed set of directories every
|
||||
/// common Node install method actually uses, with plain filesystem checks — no subprocess, no run
|
||||
/// loop, nothing that can hang, at the cost of being a known list rather than a fully general answer.
|
||||
enum LoginShellEnvironment {
|
||||
/// Directories to add if present, in priority order. Covers Homebrew (Apple Silicon and Intel),
|
||||
/// MacPorts, Volta, and nvm's "default" alias (via `nvmDefaultNodeBinDirectory`).
|
||||
nonisolated static func candidateDirectories(home: String = NSHomeDirectory()) -> [String] {
|
||||
var dirs = [
|
||||
"/opt/homebrew/bin", "/opt/homebrew/sbin",
|
||||
"/usr/local/bin", "/usr/local/sbin",
|
||||
"/opt/local/bin", "/opt/local/sbin",
|
||||
"\(home)/.volta/bin",
|
||||
]
|
||||
if let nvmDir = nvmDefaultNodeBinDirectory(home: home) {
|
||||
dirs.append(nvmDir)
|
||||
}
|
||||
return dirs
|
||||
}
|
||||
|
||||
/// nvm has no fixed "current" symlink usable in PATH, but it does write its default version as
|
||||
/// plain text to `~/.nvm/alias/default` — reading that file (no subprocess, no shell) lets us
|
||||
/// construct the versioned bin directory nvm's own shell integration would otherwise add.
|
||||
nonisolated static func nvmDefaultNodeBinDirectory(home: String, fileManager: FileManager = .default) -> String? {
|
||||
let aliasPath = "\(home)/.nvm/alias/default"
|
||||
guard let raw = try? String(contentsOfFile: aliasPath, encoding: .utf8) else { return nil }
|
||||
let version = raw.trimmingCharacters(in: .whitespacesAndNewlines)
|
||||
guard !version.isEmpty else { return nil }
|
||||
let versionDir = version.hasPrefix("v") ? version : "v\(version)"
|
||||
return "\(home)/.nvm/versions/node/\(versionDir)/bin"
|
||||
}
|
||||
|
||||
/// Prepends every candidate directory that exists and isn't already on `basePath`. Pure,
|
||||
/// synchronous, and instant — safe to call from anywhere, including MainActor, with no risk of
|
||||
/// blocking.
|
||||
nonisolated static func augmentedPath(
|
||||
basePath: String, home: String = NSHomeDirectory(), fileManager: FileManager = .default
|
||||
) -> String {
|
||||
let existing = Set(basePath.split(separator: ":").map(String.init))
|
||||
let toAdd = candidateDirectories(home: home).filter { !existing.contains($0) && fileManager.fileExists(atPath: $0) }
|
||||
guard !toAdd.isEmpty else { return basePath }
|
||||
return (toAdd + [basePath]).joined(separator: ":")
|
||||
}
|
||||
|
||||
/// Searches `basePath` (already augmented by the caller, typically) for an executable file
|
||||
/// named `name`. Returns its full path if found. Pure filesystem lookup — no subprocess.
|
||||
nonisolated static func findExecutable(named name: String, in searchPath: String, fileManager: FileManager = .default) -> String? {
|
||||
for dir in searchPath.split(separator: ":") {
|
||||
let candidate = "\(dir)/\(name)"
|
||||
if fileManager.isExecutableFile(atPath: candidate) { return candidate }
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
/// Homebrew's install prefix, if Homebrew itself is present — used to offer a one-command
|
||||
/// "install Node.js" suggestion when a stdio server's command (typically `npx`) can't be found.
|
||||
nonisolated static func homebrewPrefix(fileManager: FileManager = .default) -> String? {
|
||||
if fileManager.fileExists(atPath: "/opt/homebrew/bin/brew") { return "/opt/homebrew" }
|
||||
if fileManager.fileExists(atPath: "/usr/local/bin/brew") { return "/usr/local" }
|
||||
return nil
|
||||
}
|
||||
}
|
||||
|
||||
// MARK: - Stdio Transport
|
||||
|
||||
/// Launches the server as a subprocess and speaks newline-delimited JSON-RPC over its
|
||||
@@ -63,18 +141,33 @@ final class StdioMCPTransport: MCPTransport {
|
||||
}
|
||||
|
||||
func prepare() async throws {
|
||||
// Inherited environment first, with PATH augmented to include common Homebrew/MacPorts/
|
||||
// Volta/nvm install locations (see LoginShellEnvironment — launchd's default PATH doesn't
|
||||
// include them, so a bare `npx` spawn fails even though it works fine in Terminal), then
|
||||
// layer the user-configured vars over it so they can override.
|
||||
var environment = ProcessInfo.processInfo.environment
|
||||
let basePath = environment["PATH"] ?? "/usr/bin:/bin:/usr/sbin:/sbin"
|
||||
let searchPath = LoginShellEnvironment.augmentedPath(basePath: basePath)
|
||||
environment["PATH"] = searchPath
|
||||
for (key, value) in server.env { environment[key] = value }
|
||||
|
||||
let proc = Process()
|
||||
if server.command.hasPrefix("/") {
|
||||
guard FileManager.default.isExecutableFile(atPath: server.command) else {
|
||||
throw MCPClientError.commandNotFound(server.command)
|
||||
}
|
||||
proc.executableURL = URL(fileURLWithPath: server.command)
|
||||
proc.arguments = server.args
|
||||
} else {
|
||||
// Check up front rather than letting `/usr/bin/env <cmd>` fail asynchronously after
|
||||
// launch: a missing command is a permanent condition — surfacing it immediately as a
|
||||
// clear error, instead of after 3 rounds of crash/restart backoff, is the whole point.
|
||||
guard LoginShellEnvironment.findExecutable(named: server.command, in: searchPath) != nil else {
|
||||
throw MCPClientError.commandNotFound(server.command)
|
||||
}
|
||||
proc.executableURL = URL(fileURLWithPath: "/usr/bin/env")
|
||||
proc.arguments = [server.command] + server.args
|
||||
}
|
||||
// Inherited environment first (so PATH etc. still resolves — e.g. `npx` needs PATH to
|
||||
// find node), then layer the user-configured vars over it so they can override.
|
||||
var environment = ProcessInfo.processInfo.environment
|
||||
for (key, value) in server.env { environment[key] = value }
|
||||
proc.environment = environment
|
||||
|
||||
let stdinPipe = Pipe()
|
||||
|
||||
Reference in New Issue
Block a user