diff --git a/clients/web/package-lock.json b/clients/web/package-lock.json index f3903443..26ebbe66 100644 --- a/clients/web/package-lock.json +++ b/clients/web/package-lock.json @@ -18,6 +18,7 @@ "@modelcontextprotocol/sdk": "^1.29.0", "ajv": "^8.17.1", "atomically": "^2.1.1", + "chokidar": "^4.0.3", "hono": "^4.12.18", "open": "^10.2.0", "pino": "^9.14.0", @@ -3518,6 +3519,21 @@ "node": ">= 16" } }, + "node_modules/chokidar": { + "version": "4.0.3", + "resolved": "https://registry.npmjs.org/chokidar/-/chokidar-4.0.3.tgz", + "integrity": "sha512-Qgzu8kfBvo+cA4962jnP1KkS6Dop5NS6g7R5LFYJr4b8Ub94PPQXUksCw9PvXoeXPRRddRNC5C1JQUR2SMGtnA==", + "license": "MIT", + "dependencies": { + "readdirp": "^4.0.1" + }, + "engines": { + "node": ">= 14.16.0" + }, + "funding": { + "url": "https://paulmillr.com/funding/" + } + }, "node_modules/chromatic": { "version": "13.3.5", "resolved": "https://registry.npmjs.org/chromatic/-/chromatic-13.3.5.tgz", @@ -7556,6 +7572,19 @@ "react-dom": ">=16.6.0" } }, + "node_modules/readdirp": { + "version": "4.1.2", + "resolved": "https://registry.npmjs.org/readdirp/-/readdirp-4.1.2.tgz", + "integrity": "sha512-GDhwkLfywWL2s6vEjyhri+eXmfH6j1L7JE27WhqLeYzoh/A3DBaYGEj2H/HFZCn/kMfim73FXxEJTw06WtxQwg==", + "license": "MIT", + "engines": { + "node": ">= 14.18.0" + }, + "funding": { + "type": "individual", + "url": "https://paulmillr.com/funding/" + } + }, "node_modules/real-require": { "version": "0.2.0", "resolved": "https://registry.npmjs.org/real-require/-/real-require-0.2.0.tgz", diff --git a/clients/web/package.json b/clients/web/package.json index 401d4f43..7fbdc3aa 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -33,6 +33,7 @@ "@modelcontextprotocol/sdk": "^1.29.0", "ajv": "^8.17.1", "atomically": "^2.1.1", + "chokidar": "^4.0.3", "hono": "^4.12.18", "open": "^10.2.0", "pino": "^9.14.0", diff --git a/clients/web/server/server.ts b/clients/web/server/server.ts index 4692fbc7..b0b0e5d5 100644 --- a/clients/web/server/server.ts +++ b/clients/web/server/server.ts @@ -45,7 +45,7 @@ export async function startHonoServer( const rootPath = config.staticRoot ?? __dirname; - const { app: apiApp } = createRemoteApp({ + const { app: apiApp, close: closeApi } = createRemoteApp({ authToken: config.dangerouslyOmitAuth ? undefined : resolvedAuthToken, dangerouslyOmitAuth: config.dangerouslyOmitAuth, storageDir: config.storageDir, @@ -117,6 +117,7 @@ export async function startHonoServer( return { async close(): Promise { + await closeApi(); await sandboxController.close(); if ("closeAllConnections" in httpServer) { httpServer.closeAllConnections(); diff --git a/clients/web/server/vite-base-config.ts b/clients/web/server/vite-base-config.ts index c33934be..1d8a7d08 100644 --- a/clients/web/server/vite-base-config.ts +++ b/clients/web/server/vite-base-config.ts @@ -23,6 +23,11 @@ export function getViteBaseConfig() { // keeps Vite's dev-time scanner from chasing it through the plugin's // node-only import chain. "atomically", + // `chokidar` is only loaded inside `core/mcp/remote/node/server.ts` + // when the lazy mcp.json watcher starts. It transitively imports + // `readdirp` and core node fs/os modules; excluding it keeps Vite's + // dep scanner from walking into them during dev startup. + "chokidar", "cross-spawn", "which", ], diff --git a/clients/web/server/vite-hono-plugin.ts b/clients/web/server/vite-hono-plugin.ts index 3140c8d4..3c4225d4 100644 --- a/clients/web/server/vite-hono-plugin.ts +++ b/clients/web/server/vite-hono-plugin.ts @@ -53,13 +53,11 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { }); await sandboxController.start(); - const originalClose = server.close.bind(server); - server.close = async () => { - await sandboxController.close(); - return originalClose(); - }; - - const { app: honoApp, authToken: resolvedToken } = createRemoteApp({ + const { + app: honoApp, + authToken: resolvedToken, + close: closeApi, + } = createRemoteApp({ authToken: config.dangerouslyOmitAuth ? undefined : config.authToken, dangerouslyOmitAuth: config.dangerouslyOmitAuth, storageDir: config.storageDir, @@ -69,6 +67,15 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { initialConfig: webServerConfigToInitialPayload(config), }); + // Chain the API close (mcp.json watcher) and the sandbox into the + // Vite server's close so dev-server restarts release both resources. + const originalClose = server.close.bind(server); + server.close = async () => { + await closeApi(); + await sandboxController.close(); + return originalClose(); + }; + const sandboxUrl = sandboxController.getUrl(); const logBanner = () => { diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx index 0e3f9332..2c5aedc7 100644 --- a/clients/web/src/test/core/react/useServers.test.tsx +++ b/clients/web/src/test/core/react/useServers.test.tsx @@ -19,12 +19,13 @@ interface Harness { fetchFn: typeof fetch; configPath: string; tempDir: string; + closeApi: () => Promise; } function setupHarness(): Harness { const tempDir = mkdtempSync(join(tmpdir(), "inspector-useServers-")); const configPath = join(tempDir, "mcp.json"); - const { app } = createRemoteApp({ + const { app, close: closeApi } = createRemoteApp({ dangerouslyOmitAuth: true, mcpConfigPath: configPath, initialConfig: { defaultEnvironment: {} }, @@ -38,10 +39,16 @@ function setupHarness(): Harness { : new Request(input as string | URL, init); return app.fetch(req) as Promise; }; - return { fetchFn, configPath, tempDir }; + return { fetchFn, configPath, tempDir, closeApi }; } -function teardownHarness(h: Harness): void { +async function teardownHarness(h: Harness): Promise { + // Closing here releases the lazy chokidar watcher started by the SSE + // subscription useServers opens on mount. Without it the watcher would + // hang around for the lifetime of the vitest worker — harmless for the + // suite as a whole, but it would slow worker exit and could leak inotify + // watches on Linux. + await h.closeApi(); try { rmSync(h.tempDir, { recursive: true }); } catch { @@ -60,8 +67,8 @@ describe("useServers", () => { h = setupHarness(); }); - afterEach(() => { - teardownHarness(h); + afterEach(async () => { + await teardownHarness(h); }); it("starts in loading state, then loads and converts the seed config", async () => { @@ -218,6 +225,83 @@ describe("useServers", () => { expect(result.current.servers.map((s) => s.id)).toEqual(["hand"]); }); + it("auto-refreshes when the /api/servers/events SSE channel signals a change", async () => { + // Mount the hook (which also opens the SSE subscription) and let the + // initial GET settle so we're observing a steady state before mutating + // the file. + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn: h.fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + + // Give chokidar a beat to register with the now-seeded file. The lazy + // watcher inside createRemoteApp starts on the first SSE subscriber, so + // by the time the initial GET resolves the watcher is already attached. + // The pause covers any platform-specific scan-stabilization window + // before we mutate. + await new Promise((r) => setTimeout(r, 200)); + + // Simulate an external editor save. The watcher fires → SSE broadcasts + // → the hook's reader-loop calls refresh() without us touching the + // exposed refresh() API. + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { external: { type: "stdio", command: "outside" } }, + }) + "\n", + ); + + await waitFor( + () => { + expect(result.current.servers.map((s) => s.id)).toEqual(["external"]); + }, + { timeout: 3000 }, + ); + }); + + it("SSE-driven refresh does not flip loading back to true (no skeleton flicker)", async () => { + // Regression guard for the background-refresh split: a consumer + // rendering a loading spinner / skeleton must not see `loading` go + // true on every external mcp.json edit. The hook's SSE handler calls + // refreshInternal(true), which skips the setLoading toggles entirely; + // sampling a single point can miss the bug under React batching, so + // record every observed `loading` value across the SSE cycle and + // assert none of them is true. + const loadingHistory: boolean[] = []; + const { result } = renderHook(() => { + const r = useServers({ + baseUrl: "http://test.local", + fetchFn: h.fetchFn, + }); + loadingHistory.push(r.loading); + return r; + }); + + await waitFor(() => expect(result.current.loading).toBe(false)); + // Slice off the mount-phase renders; only renders after the initial + // load are subject to the no-flicker contract. + const baselineLen = loadingHistory.length; + + await new Promise((r) => setTimeout(r, 200)); + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { flicker: { type: "stdio", command: "outside" } }, + }) + "\n", + ); + + await waitFor( + () => { + expect(result.current.servers.map((s) => s.id)).toEqual(["flicker"]); + }, + { timeout: 3000 }, + ); + + const postRefreshLoadings = loadingHistory.slice(baselineLen); + expect(postRefreshLoadings.length).toBeGreaterThan(0); + expect(postRefreshLoadings.every((v) => v === false)).toBe(true); + }); + it("captures the error message on fetch network failure", async () => { const failingFetch = vi .fn() diff --git a/clients/web/src/test/integration/mcp/remote/servers-events.test.ts b/clients/web/src/test/integration/mcp/remote/servers-events.test.ts new file mode 100644 index 00000000..ed27b452 --- /dev/null +++ b/clients/web/src/test/integration/mcp/remote/servers-events.test.ts @@ -0,0 +1,383 @@ +/** + * Integration tests for GET /api/servers/events — the SSE channel that + * notifies subscribed browsers when `mcp.json` changes on disk. Covers: + * + * - external write to the file triggers a broadcast + * - external unlink triggers a broadcast + * - the backend's own POST/PUT/DELETE does NOT trigger a broadcast + * (self-write suppression via the post-write mtime capture) + * - multiple connected subscribers each receive the broadcast + * + * Tests spin up a real TCP server (so the SSE response body flows through + * @hono/node-server rather than the in-process Hono fetch) and read the + * stream with `fetch().body.getReader()`. Each harness calls `closeApi()` + * in teardown so the lazy chokidar watcher is released. + */ + +import { describe, it, expect, afterEach, beforeEach } from "vitest"; +import { + mkdtempSync, + rmSync, + writeFileSync, + unlinkSync, + existsSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { serve } from "@hono/node-server"; +import type { ServerType } from "@hono/node-server"; +import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js"; + +interface Harness { + baseUrl: string; + server: ServerType; + configPath: string; + tempDir: string; + closeApi: () => Promise; +} + +async function setup(): Promise { + const tempDir = mkdtempSync(join(tmpdir(), "inspector-servers-events-")); + const configPath = join(tempDir, "mcp.json"); + const { app, close: closeApi } = createRemoteApp({ + dangerouslyOmitAuth: true, + mcpConfigPath: configPath, + initialConfig: { defaultEnvironment: {} }, + }); + const { baseUrl, server } = await new Promise<{ + baseUrl: string; + server: ServerType; + }>((resolve, reject) => { + const s = serve( + { fetch: app.fetch, port: 0, hostname: "127.0.0.1" }, + (info) => { + const port = + info && typeof info === "object" && "port" in info + ? (info as { port: number }).port + : 0; + resolve({ baseUrl: `http://127.0.0.1:${port}`, server: s }); + }, + ); + s.on("error", reject); + }); + return { baseUrl, server, configPath, tempDir, closeApi }; +} + +async function teardown(h: Harness): Promise { + await h.closeApi(); + await new Promise((resolve) => h.server.close(() => resolve())); + try { + rmSync(h.tempDir, { recursive: true }); + } catch { + /* ignore */ + } +} + +interface EventSink { + readonly events: string[]; + /** Resolve once `events.length >= n` (or reject after `timeoutMs`). */ + waitFor(n: number, timeoutMs?: number): Promise; + close(): void; +} + +/** + * Open an SSE subscription and parse out each `data:` payload into a flat + * string array. Resolves once the response head is received so the caller can + * mutate the file knowing the subscriber is registered server-side. + */ +async function subscribe(baseUrl: string): Promise { + const controller = new AbortController(); + const res = await fetch(`${baseUrl}/api/servers/events`, { + signal: controller.signal, + }); + if (!res.ok || !res.body) { + throw new Error(`SSE subscribe failed: ${res.status}`); + } + + const events: string[] = []; + const waiters: { n: number; resolve: () => void }[] = []; + + const notifyWaiters = (): void => { + for (const w of [...waiters]) { + if (events.length >= w.n) { + waiters.splice(waiters.indexOf(w), 1); + w.resolve(); + } + } + }; + + const reader = res.body.getReader(); + const decoder = new TextDecoder(); + let buffer = ""; + + void (async () => { + try { + while (true) { + const { done, value } = await reader.read(); + if (done) return; + buffer += decoder.decode(value, { stream: true }); + let frameEnd = buffer.indexOf("\n\n"); + while (frameEnd !== -1) { + const frame = buffer.slice(0, frameEnd); + buffer = buffer.slice(frameEnd + 2); + const dataLine = frame.split("\n").find((l) => l.startsWith("data:")); + if (dataLine) { + events.push(dataLine.slice("data:".length).trim()); + notifyWaiters(); + } + frameEnd = buffer.indexOf("\n\n"); + } + } + } catch { + // Aborted by caller in close(); benign. + } + })(); + + return { + events, + async waitFor(n: number, timeoutMs = 2000): Promise { + if (events.length >= n) return; + await new Promise((resolve, reject) => { + const timer = setTimeout(() => { + reject( + new Error( + `Timed out waiting for ${n} event(s); saw ${events.length}`, + ), + ); + }, timeoutMs); + waiters.push({ + n, + resolve: () => { + clearTimeout(timer); + resolve(); + }, + }); + }); + }, + close(): void { + controller.abort(); + }, + }; +} + +describe("GET /api/servers/events", () => { + let h: Harness; + + beforeEach(async () => { + h = await setup(); + }); + + afterEach(async () => { + await teardown(h); + }); + + it("emits an event when an external editor writes the config file", async () => { + // Seed the file so chokidar starts watching an existing path. The + // subscribe() round-trip below ensures the watcher is registered before + // we mutate the file. + writeFileSync( + h.configPath, + JSON.stringify({ mcpServers: {} }, null, 2) + "\n", + ); + const sink = await subscribe(h.baseUrl); + + // Small gap so chokidar's initial-scan settles before we mutate. + await new Promise((r) => setTimeout(r, 150)); + writeFileSync( + h.configPath, + JSON.stringify( + { + mcpServers: { alpha: { type: "stdio", command: "node" } }, + }, + null, + 2, + ) + "\n", + ); + + await sink.waitFor(1); + expect(sink.events).toHaveLength(1); + expect(JSON.parse(sink.events[0]!)).toEqual({ type: "change" }); + sink.close(); + }); + + it("does NOT emit when the backend's own POST /api/servers writes the file", async () => { + writeFileSync( + h.configPath, + JSON.stringify({ mcpServers: {} }, null, 2) + "\n", + ); + const sink = await subscribe(h.baseUrl); + await new Promise((r) => setTimeout(r, 150)); + + const postRes = await fetch(`${h.baseUrl}/api/servers`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "alpha", + config: { type: "stdio", command: "node" }, + }), + }); + expect(postRes.status).toBe(200); + + // awaitWriteFinish stability is 100ms; allow generous margin so a real + // (incorrectly broadcast) event would land in `events` before we assert. + await new Promise((r) => setTimeout(r, 400)); + expect(sink.events).toEqual([]); + sink.close(); + }); + + it("does NOT emit when the backend's own PUT /api/servers/:id writes the file", async () => { + writeFileSync( + h.configPath, + JSON.stringify( + { mcpServers: { alpha: { type: "stdio", command: "old" } } }, + null, + 2, + ) + "\n", + ); + const sink = await subscribe(h.baseUrl); + await new Promise((r) => setTimeout(r, 150)); + + const putRes = await fetch(`${h.baseUrl}/api/servers/alpha`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "alpha", + config: { type: "stdio", command: "new" }, + }), + }); + expect(putRes.status).toBe(200); + + await new Promise((r) => setTimeout(r, 400)); + expect(sink.events).toEqual([]); + sink.close(); + }); + + it("does NOT emit when the backend's own DELETE /api/servers/:id writes the file", async () => { + writeFileSync( + h.configPath, + JSON.stringify( + { mcpServers: { alpha: { type: "stdio", command: "node" } } }, + null, + 2, + ) + "\n", + ); + const sink = await subscribe(h.baseUrl); + await new Promise((r) => setTimeout(r, 150)); + + const delRes = await fetch(`${h.baseUrl}/api/servers/alpha`, { + method: "DELETE", + }); + expect(delRes.status).toBe(200); + + await new Promise((r) => setTimeout(r, 400)); + expect(sink.events).toEqual([]); + sink.close(); + }); + + it("emits a broadcast when an external edit slips in between two backend writes (peer-tab visibility)", async () => { + // This is the race the watcher-handler suppression cannot catch on its + // own: external editor saves AFTER the backend has read the current + // mtime but BEFORE chokidar's awaitWriteFinish has fired, so chokidar + // coalesces the external mtime and our merged-write mtime into one + // event whose mtime matches our own write — and we'd suppress. + // `writeMcpAndTrackMtime` covers this by stat()ing on entry and + // broadcasting itself if the pre-write mtime doesn't match its tracked + // value, so a second connected subscriber learns about the change. + writeFileSync( + h.configPath, + JSON.stringify( + { mcpServers: { alpha: { type: "stdio", command: "v1" } } }, + null, + 2, + ) + "\n", + ); + // Drive the first write through the backend so lastWrittenMtimeMs is + // populated with the mtime we expect on disk going into the next write. + const firstPut = await fetch(`${h.baseUrl}/api/servers/alpha`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "alpha", + config: { type: "stdio", command: "v2" }, + }), + }); + expect(firstPut.status).toBe(200); + + // Subscribe AFTER the first write so the peer subscriber represents a + // separate tab opened mid-session. + const sink = await subscribe(h.baseUrl); + await new Promise((r) => setTimeout(r, 150)); + + // External editor writes the file directly. Need a fresh mtime distinct + // from the last backend write, so wait a beat (mtime granularity on + // older filesystems is ms-coarse). + await new Promise((r) => setTimeout(r, 20)); + writeFileSync( + h.configPath, + JSON.stringify( + { mcpServers: { alpha: { type: "stdio", command: "external" } } }, + null, + 2, + ) + "\n", + ); + + // Backend write follows quickly enough that chokidar's + // awaitWriteFinish coalesces both events. Without the pre-stat check + // the peer subscriber would never see this edit. + const secondPut = await fetch(`${h.baseUrl}/api/servers/alpha`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "alpha", + config: { type: "stdio", command: "v3-merged" }, + }), + }); + expect(secondPut.status).toBe(200); + + await sink.waitFor(1); + expect(sink.events.length).toBeGreaterThanOrEqual(1); + expect(JSON.parse(sink.events[0]!)).toEqual({ type: "change" }); + sink.close(); + }); + + it("emits when the user deletes the config file", async () => { + writeFileSync( + h.configPath, + JSON.stringify({ mcpServers: {} }, null, 2) + "\n", + ); + const sink = await subscribe(h.baseUrl); + await new Promise((r) => setTimeout(r, 150)); + + expect(existsSync(h.configPath)).toBe(true); + unlinkSync(h.configPath); + + await sink.waitFor(1); + expect(sink.events).toHaveLength(1); + sink.close(); + }); + + it("broadcasts to multiple connected subscribers", async () => { + writeFileSync( + h.configPath, + JSON.stringify({ mcpServers: {} }, null, 2) + "\n", + ); + const sink1 = await subscribe(h.baseUrl); + const sink2 = await subscribe(h.baseUrl); + await new Promise((r) => setTimeout(r, 150)); + + writeFileSync( + h.configPath, + JSON.stringify( + { mcpServers: { beta: { type: "stdio", command: "node" } } }, + null, + 2, + ) + "\n", + ); + + await Promise.all([sink1.waitFor(1), sink2.waitFor(1)]); + expect(sink1.events).toHaveLength(1); + expect(sink2.events).toHaveLength(1); + sink1.close(); + sink2.close(); + }); +}); diff --git a/clients/web/tsconfig.app.json b/clients/web/tsconfig.app.json index 1134c60e..8e54add8 100644 --- a/clients/web/tsconfig.app.json +++ b/clients/web/tsconfig.app.json @@ -44,7 +44,8 @@ "./node_modules/hono/dist/types/helper/streaming/index" ], "@hono/node-server": ["./node_modules/@hono/node-server"], - "atomically": ["./node_modules/atomically"] + "atomically": ["./node_modules/atomically"], + "chokidar": ["./node_modules/chokidar"] } }, "include": ["src", "../../core/**/*.ts"], diff --git a/clients/web/tsconfig.node.json b/clients/web/tsconfig.node.json index 33532f67..23d92428 100644 --- a/clients/web/tsconfig.node.json +++ b/clients/web/tsconfig.node.json @@ -41,7 +41,8 @@ "./node_modules/hono/dist/types/helper/streaming/index" ], "@hono/node-server": ["./node_modules/@hono/node-server"], - "atomically": ["./node_modules/atomically"] + "atomically": ["./node_modules/atomically"], + "chokidar": ["./node_modules/chokidar"] } }, "include": ["vite.config.ts", "server/**/*.ts"] diff --git a/clients/web/tsconfig.test.json b/clients/web/tsconfig.test.json index 9a4d2995..3648e3d9 100644 --- a/clients/web/tsconfig.test.json +++ b/clients/web/tsconfig.test.json @@ -19,6 +19,7 @@ ], "@hono/node-server": ["./node_modules/@hono/node-server"], "atomically": ["./node_modules/atomically"], + "chokidar": ["./node_modules/chokidar"], "express": ["./node_modules/@types/express"], "yaml": ["./node_modules/yaml"] } diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index de63d932..a9f3534b 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -52,6 +52,7 @@ const nodeModulesAliases = [ { find: /^hono\/streaming$/, replacement: path.resolve(dirname, 'node_modules/hono/dist/helper/streaming/index.js') }, { find: /^@hono\/node-server$/, replacement: path.resolve(dirname, 'node_modules/@hono/node-server') }, { find: /^atomically$/, replacement: path.resolve(dirname, 'node_modules/atomically') }, + { find: /^chokidar$/, replacement: path.resolve(dirname, 'node_modules/chokidar') }, { find: /^express$/, replacement: path.resolve(dirname, 'node_modules/express') }, { find: /^yaml$/, replacement: path.resolve(dirname, 'node_modules/yaml') }, // Pin the SDK auth subpath so test and source resolve to the exact same @@ -113,6 +114,18 @@ export default defineConfig({ ) { return; } + // Same story for `chokidar`: the mcp.json file watcher in + // `core/mcp/remote/node/server.ts` is reachable only through the dev + // backend, never from the browser bundle. Rollup still scans the + // import statement and warns when it can't resolve it. + if ( + warning.code === 'UNRESOLVED_IMPORT' && + typeof warning.message === 'string' && + warning.message.includes("'chokidar'") && + warning.message.includes('server.ts') + ) { + return; + } defaultHandler(warning); }, }, diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index cf90aba9..a90ebc5a 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -5,6 +5,7 @@ */ import { randomBytes, timingSafeEqual } from "node:crypto"; +import { stat as fsStat } from "node:fs/promises"; import type pino from "pino"; import { getDefaultStorageDir, @@ -21,6 +22,7 @@ import type { LogEvent } from "pino"; import { Hono } from "hono"; import type { Context, Env, Next } from "hono"; import { streamSSE } from "hono/streaming"; +import { watch as chokidarWatch, type FSWatcher } from "chokidar"; import { createTransportNode } from "../../node/transport.js"; import type { RemoteConnectRequest, RemoteSendRequest } from "../types.js"; import type { @@ -84,6 +86,21 @@ export interface CreateRemoteAppResult { app: Hono; /** The auth token (from options, env var, or generated). Returned so caller can embed in client. */ authToken: string; + /** + * Tear down stateful resources owned by the app (currently the lazy + * chokidar watcher behind `/api/servers/events`). Long-lived prod callers + * (the standalone server, the vite dev plugin) chain this into their own + * HTTP-server close so the watcher is released on shutdown. Tests that + * exercise SSE should call it in teardown — tests that never subscribe + * never start the watcher and can omit it without leaking. + * + * Resolves once the subscriber set is cleared and the watcher is closed. + * Individual SSE stream callbacks held inside `streamSSE` are not awaited + * here — they resolve on their own when the underlying socket aborts. + * Callers that need to be sure those have settled (e.g. the standalone + * server) should call `httpServer.closeAllConnections()` *after* this. + */ + close: () => Promise; } /** @@ -257,6 +274,145 @@ export function createRemoteApp( const storageDir = options.storageDir ?? getDefaultStorageDir(); const mcpConfigPath = options.mcpConfigPath ?? getDefaultMcpConfigPath(); + // --- /api/servers/events: file-watch fanout state ----------------------- + // + // Subscribers are SSE writers added by GET /api/servers/events and removed + // on stream abort. The chokidar watcher is created lazily on the first + // subscription and torn down again when the last one leaves, so tests (and + // headless tools) that never open the channel never spin up a real fs + // watcher. The `lastWrittenMtimeMs` field is captured after every write we + // initiate ourselves; the watcher handler stat()s on each event and + // suppresses the broadcast if the mtime matches — that's how we avoid + // refreshing every connected browser tab for our own POST/PUT/DELETE. + const serverEventSubscribers = new Set<(data: string) => void>(); + let mcpConfigWatcher: FSWatcher | null = null; + let lastWrittenMtimeMs: number | null = null; + + const broadcastServerListChange = (): void => { + const payload = JSON.stringify({ type: "change" }); + for (const send of serverEventSubscribers) { + try { + send(payload); + } catch { + // Each subscriber owns its own write loop and cleans itself up in + // onAbort; swallowing here keeps one bad stream from blocking the + // fanout to the rest. + } + } + }; + + const writeMcpAndTrackMtime = async (data: string): Promise => { + // If an external editor wrote the file between our previous write and + // this one, chokidar's `awaitWriteFinish` will coalesce both events + // into a single watcher fire whose mtime matches what we're about to + // write — and the watcher handler will then suppress the broadcast. + // Peer subscribers (e.g. a second browser tab) would never learn about + // the external edit. Detect that case here by comparing the current + // on-disk mtime against our last tracked mtime; broadcast after the + // write completes so peers re-fetch. + // + // This is a notification-of-divergence, not a preservation guarantee: + // depending on whether the external write landed before or after the + // route handler's `readMcpConfig()`, the external edit's content may + // already be inside our serialized payload (it'll round-trip) or it + // may have been read-around and the next `writeStoreFile` below will + // overwrite it. Either way peers learn there's been a change and + // re-fetch the resulting authoritative on-disk state. Preserving the + // external edit's content in the second ordering would require a + // read-modify-write retry loop, which is outside this PR's scope and + // probably not worth it for a single-user local dev tool. + // + // The originating tab's mutator triggers its own refresh on PUT/POST + // success, so this extra broadcast is intended for peers only — a + // double refresh on the originating tab is cheap (same GET, same + // payload) and acceptable. + let externalEditDetected = false; + if (lastWrittenMtimeMs !== null) { + try { + const s = await fsStat(mcpConfigPath); + if (s.mtimeMs !== lastWrittenMtimeMs) { + externalEditDetected = true; + } + } catch { + // File missing → an external delete slipped in between our writes. + // Treat as an external edit so peers learn about it. + externalEditDetected = true; + } + } + + await writeStoreFile(mcpConfigPath, data); + try { + const s = await fsStat(mcpConfigPath); + lastWrittenMtimeMs = s.mtimeMs; + } catch { + // If the stat fails the next watcher event will broadcast — that's the + // correct fallback (the only cost is a redundant client refresh). + } + + if (externalEditDetected) { + broadcastServerListChange(); + } + }; + + const handleWatcherEvent = async (event: string): Promise => { + if (event !== "add" && event !== "change" && event !== "unlink") return; + if (event !== "unlink") { + try { + const s = await fsStat(mcpConfigPath); + if ( + lastWrittenMtimeMs !== null && + s.mtimeMs === lastWrittenMtimeMs + ) { + return; + } + } catch { + // File vanished between event and stat — broadcast so subscribers + // re-fetch and the GET handler can re-seed on the next read. + } + } + broadcastServerListChange(); + }; + + const handleWatcherError = (err: unknown): void => { + const msg = err instanceof Error ? err.message : String(err); + if (fileLogger) { + fileLogger.warn({ err: msg }, "mcp.json watcher error"); + } else { + console.warn("[mcp.json watcher]", msg); + } + }; + + const ensureWatcher = (): void => { + if (mcpConfigWatcher) return; + // `awaitWriteFinish` coalesces the multi-event sequence editors produce + // when they save via temp-file + rename (the watched path briefly + // disappears then reappears). The stability threshold also covers our + // own atomically-written rename, so a single backend POST yields one + // event to inspect rather than an unlink/add pair. + mcpConfigWatcher = chokidarWatch(mcpConfigPath, { + ignoreInitial: true, + awaitWriteFinish: { stabilityThreshold: 100, pollInterval: 50 }, + }); + mcpConfigWatcher.on("all", (event) => { + void handleWatcherEvent(event); + }); + mcpConfigWatcher.on("error", handleWatcherError); + }; + + const maybeStopWatcher = async (): Promise => { + if (serverEventSubscribers.size > 0) return; + if (!mcpConfigWatcher) return; + const w = mcpConfigWatcher; + mcpConfigWatcher = null; + try { + await w.close(); + } catch { + // Closing a chokidar instance can throw on already-closed handles + // (e.g. if the underlying fs watch was unhooked by a signal). The + // resource is gone either way; nothing to do. + } + }; + // Apply origin validation middleware first (before auth) // This prevents DNS rebinding attacks by validating Origin header app.use("*", createOriginMiddleware(allowedOrigins)); @@ -831,7 +987,7 @@ export function createRemoteApp( try { const raw = await readStoreFile(mcpConfigPath); if (raw === null) { - await writeStoreFile(mcpConfigPath, serializeStore(DEFAULT_SEED_CONFIG)); + await writeMcpAndTrackMtime(serializeStore(DEFAULT_SEED_CONFIG)); return c.json(DEFAULT_SEED_CONFIG); } const parsed = parseStore(raw) as { mcpServers?: unknown } | null; @@ -886,7 +1042,7 @@ export function createRemoteApp( body.config, postSettings, ); - await writeStoreFile(mcpConfigPath, serializeStore(current)); + await writeMcpAndTrackMtime(serializeStore(current)); return c.json({ ok: true }); }); } catch (error) { @@ -994,7 +1150,7 @@ export function createRemoteApp( next.mcpServers[key] = val; } } - await writeStoreFile(mcpConfigPath, serializeStore(next)); + await writeMcpAndTrackMtime(serializeStore(next)); return c.json({ ok: true }); }); } catch (error) { @@ -1016,7 +1172,7 @@ export function createRemoteApp( return c.json({ ok: true }); } delete current.mcpServers[id]; - await writeStoreFile(mcpConfigPath, serializeStore(current)); + await writeMcpAndTrackMtime(serializeStore(current)); return c.json({ ok: true }); }); } catch (error) { @@ -1025,5 +1181,40 @@ export function createRemoteApp( } }); - return { app, authToken }; + // Server-sent events for `mcp.json` external edits. The payload is + // intentionally empty-of-meaning ({"type":"change"}) — the client only + // cares that *something* happened on disk and re-fetches GET /api/servers + // to get the authoritative state. This sidesteps any drift between an + // event payload and the canonical normalize-on-read shape. + app.get("/api/servers/events", async (c) => { + return streamSSE(c, async (stream) => { + const send = (data: string): void => { + void stream.writeSSE({ event: "change", data }); + }; + serverEventSubscribers.add(send); + ensureWatcher(); + + stream.onAbort(() => { + serverEventSubscribers.delete(send); + void maybeStopWatcher(); + stream.close(); + }); + + // Hono closes the stream the moment this callback returns, so hold the + // promise open until the client aborts. Cleanup happens in the + // onAbort handler registered above. + await new Promise((resolve) => { + stream.onAbort(() => resolve()); + }); + }); + }); + + return { + app, + authToken, + close: async () => { + serverEventSubscribers.clear(); + await maybeStopWatcher(); + }, + }; } diff --git a/core/react/useServers.ts b/core/react/useServers.ts index c4412473..a546581a 100644 --- a/core/react/useServers.ts +++ b/core/react/useServers.ts @@ -76,30 +76,98 @@ export function useServers(opts: UseServersOptions): UseServersResult { const [loading, setLoading] = useState(true); const [error, setError] = useState(undefined); - const refresh = useCallback(async (): Promise => { - setLoading(true); - setError(undefined); - try { - const res = await doFetch(`${base}/api/servers`, { - method: "GET", - headers: buildHeaders(authToken, false), - }); - if (!res.ok) { - throw new Error(await readErrorMessage(res)); + // Inner refresh that the public `refresh` and the SSE-triggered background + // refresh both share. `background` skips the loading-state toggle so + // consumers rendering a spinner or skeleton don't flash on every external + // mcp.json edit — the existing list stays on screen while the re-fetch + // resolves. `error` is still reset / set either way so a real error + // surfaces even from a background refresh. + const refreshInternal = useCallback( + async (background: boolean): Promise => { + if (!background) setLoading(true); + setError(undefined); + try { + const res = await doFetch(`${base}/api/servers`, { + method: "GET", + headers: buildHeaders(authToken, false), + }); + if (!res.ok) { + throw new Error(await readErrorMessage(res)); + } + const body = (await res.json()) as MCPConfig; + setServers(mcpConfigToServerEntries(body)); + } catch (err) { + setError(err instanceof Error ? err.message : String(err)); + } finally { + if (!background) setLoading(false); } - const body = (await res.json()) as MCPConfig; - setServers(mcpConfigToServerEntries(body)); - } catch (err) { - setError(err instanceof Error ? err.message : String(err)); - } finally { - setLoading(false); - } - }, [base, authToken, doFetch]); + }, + [base, authToken, doFetch], + ); + + const refresh = useCallback( + (): Promise => refreshInternal(false), + [refreshInternal], + ); useEffect(() => { void refresh(); }, [refresh]); + // Subscribe to `/api/servers/events` so external edits to mcp.json + // (the user editing the file by hand, or `mcp.json` written by another + // tool) propagate without a manual page refresh. We deliberately use + // fetch() + ReadableStream rather than EventSource because EventSource + // can't send the existing `x-mcp-remote-auth: Bearer …` header — the + // backend's auth contract is unchanged. The event payload itself is + // ignored: any signal triggers a re-fetch, since the GET handler is the + // sole source of truth for the list shape. + useEffect(() => { + const controller = new AbortController(); + void (async () => { + try { + const res = await doFetch(`${base}/api/servers/events`, { + method: "GET", + headers: buildHeaders(authToken, false), + signal: controller.signal, + }); + if (!res.ok || !res.body) return; + const reader = res.body.getReader(); + const decoder = new TextDecoder(); + let buffer = ""; + // SSE frames are separated by a blank line (`\n\n`). We don't parse + // the event type or data — `refreshInternal()` re-fetches the + // canonical state regardless — so we just count frames and fire a + // single background refresh per decode chunk. Two `change` + // broadcasts landing in the same chunk become one re-fetch instead + // of two concurrent ones whose setState order is unspecified. + // Cross-chunk debounce is not added: `awaitWriteFinish`'s 100ms + // stability threshold already serializes external edits at the + // source, and chained fetches against the same GET endpoint are + // idempotent enough that a rare back-to-back pair just costs one + // extra round-trip. + while (true) { + const { done, value } = await reader.read(); + if (done) break; + buffer += decoder.decode(value, { stream: true }); + let sawFrame = false; + let frameEnd = buffer.indexOf("\n\n"); + while (frameEnd !== -1) { + buffer = buffer.slice(frameEnd + 2); + sawFrame = true; + frameEnd = buffer.indexOf("\n\n"); + } + if (sawFrame) void refreshInternal(true); + } + } catch { + // AbortError on unmount, or a transient network blip. No reconnect: + // a real failure leaves the hook in last-known-good state and the + // user can hit refresh manually. The dev-tool reload story is fine. + } + })(); + return () => controller.abort(); + }, [base, authToken, doFetch, refreshInternal]); + const addServer = useCallback( async (id: string, config: MCPServerConfig): Promise => { const res = await doFetch(`${base}/api/servers`, {