From 4f533c721be32070ab162edfc3e15ffe8adb40d7 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 14 May 2026 08:24:32 -0700 Subject: [PATCH] fix(mcp): skip stdin EOF handlers when MCP_STDIO=1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenClaw's bundle-mcp gateway and similar wrappers pipe the JSON-RPC handshake on stdin then close their stdin half. Pre-fix, both stdin 'end' and 'close' listeners (server.ts:65-66 and serve.ts:204-206) treated this as a permanent disconnect and shut the server down before the first tool call arrived. Guard both sites with `process.env.MCP_STDIO !== '1'`. Signal handlers (SIGTERM/SIGINT/SIGHUP), transport.onclose, and the parent-process watchdog still cover legitimate shutdown paths. The serve.ts site threads the env read through an injectable `mcpStdio?: boolean` on ServeOptions so tests stay isolated (no process.env mutation per scripts/check-test-isolation.sh R1). Tests: 3 new cases in test/serve-stdio-lifecycle.test.ts pin the guard's invariants — mcpStdio=true must NOT trigger shutdown on stdin EOF, signals must still drive shutdown with mcpStdio=true, and mcpStdio=false (default) preserves existing CLI behavior. 25/25 pass. Origin: PR #870. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/commands/serve.ts | 17 ++++++++- src/mcp/server.ts | 11 +++++- test/serve-stdio-lifecycle.test.ts | 61 ++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 3 deletions(-) diff --git a/src/commands/serve.ts b/src/commands/serve.ts index 1b1651a82..af70480ca 100644 --- a/src/commands/serve.ts +++ b/src/commands/serve.ts @@ -56,6 +56,13 @@ export interface ServeOptions { // tick fell through to the cached `process.ppid` and the watchdog // never fired, while still claiming to be installed. probeWatchdog?: () => boolean; + // v0.34.0 (#870): test seam for the MCP_STDIO=1 piped-stdin guard. + // When true, runServe skips the stdin 'end'/'close' shutdown hooks + // because the wrapping gateway (OpenClaw bundle-mcp, others) pipes the + // JSON-RPC handshake and closes stdin immediately. Signal handlers and + // transport.onclose still cover legitimate shutdown. + // Defaults to `process.env.MCP_STDIO === '1'` when omitted. + mcpStdio?: boolean; } export async function runServe( @@ -201,7 +208,15 @@ function installStdioLifecycle( // Skip when stdin is a TTY: interactive `gbrain serve` use shouldn't // terminate just because the user hasn't typed anything. Signal / // watchdog paths still cover that case if needed. - if (!deps.stdin.isTTY) { + // v0.34.0 (#870): when MCP_STDIO=1, the wrapping gateway pipes the + // JSON-RPC handshake then closes its stdin half. Treating that as a + // permanent disconnect kills the server before the first tool call. + // Signal handlers (SIGTERM/SIGINT/SIGHUP), transport.onclose, and the + // parent-process watchdog below still cover legitimate shutdown paths. + // `mcpStdio` is the injectable form; default reads the env once at + // install time so tests stay isolated (no process.env mutation). + const mcpStdioMode = opts.mcpStdio ?? (process.env.MCP_STDIO === '1'); + if (!deps.stdin.isTTY && !mcpStdioMode) { deps.stdin.once('end', () => beginShutdown('stdin-end')); deps.stdin.once('close', () => beginShutdown('stdin-close')); } diff --git a/src/mcp/server.ts b/src/mcp/server.ts index e268cbbd4..b600f2d0c 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -62,8 +62,15 @@ export async function startMcpServer(engine: BrainEngine) { .catch(() => {}) .finally(() => process.exit(code)); }; - process.stdin.on('end', () => shutdown('stdin end')); - process.stdin.on('close', () => shutdown('stdin close')); + // v0.34.0 (#870): when MCP_STDIO=1, the wrapping gateway (OpenClaw's + // bundle-mcp layer, others) often pipes the JSON-RPC handshake then + // closes its stdin half. Treating that as a permanent disconnect kills + // the server before the first tool call arrives. Signal handlers and + // transport.onclose still cover the legitimate shutdown paths. + if (process.env.MCP_STDIO !== '1') { + process.stdin.on('end', () => shutdown('stdin end')); + process.stdin.on('close', () => shutdown('stdin close')); + } // @ts-ignore — SDK exposes onclose on transport transport.onclose = () => shutdown('transport close'); process.on('SIGTERM', () => shutdown('SIGTERM')); diff --git a/test/serve-stdio-lifecycle.test.ts b/test/serve-stdio-lifecycle.test.ts index e548b0134..ff284031b 100644 --- a/test/serve-stdio-lifecycle.test.ts +++ b/test/serve-stdio-lifecycle.test.ts @@ -81,6 +81,7 @@ function makeHarness(opts: { isTTY?: boolean; initialParentPid?: number; probeWatchdog?: boolean; + mcpStdio?: boolean; } = {}): Harness { const engine = new StubEngine(); const stdin = new EventEmitter() as EventEmitter & { isTTY?: boolean }; @@ -120,6 +121,7 @@ function makeHarness(opts: { setInterval: timers.setInterval, clearInterval: timers.clearInterval, probeWatchdog: () => probeWatchdogResult, + mcpStdio: opts.mcpStdio, }; return { @@ -439,4 +441,63 @@ describe('runServe stdio lifecycle', () => { expect(code).toBe(0); expect(h.logs.some(l => l.includes('cleanup error: synthetic disconnect failure'))).toBe(true); }); + + // v0.34.0 (#870): OpenClaw gateway / bundle-mcp wrappers pipe the + // JSON-RPC handshake on stdin then close their stdin half. Without + // MCP_STDIO=1 the server treats that as a permanent disconnect and + // exits before handling tools/call. The guard skips the stdin 'end' / + // 'close' hooks when MCP_STDIO=1; signals and parent watchdog still + // cover legitimate shutdown. + describe('MCP_STDIO=1 piped-stdin guard (#870)', () => { + test('stdin end with mcpStdio=true does NOT trigger shutdown', async () => { + const h = makeHarness({ mcpStdio: true }); + await startInBackground(h.engine, [], h.opts); + + // Without the guard this would shutdown; with the guard it must not. + h.stdin.emit('end'); + + // Give the event loop a microtask turn to catch any erroneous shutdown + // path. We assert NO exit was registered. + await new Promise((r) => setTimeout(r, 10)); + expect(h.engine.disconnectCalls).toBe(0); + + // Then trigger SIGTERM to drive the test to completion; signal handlers + // remain active even with mcpStdio=true (codex would catch if they didn't). + h.signals.emit('SIGTERM'); + const code = await h.exited; + expect(code).toBe(0); + expect(h.engine.disconnectCalls).toBe(1); + expect(h.logs.some(l => l.includes('graceful exit (SIGTERM)'))).toBe(true); + }); + + test('stdin close with mcpStdio=true does NOT trigger shutdown', async () => { + const h = makeHarness({ mcpStdio: true }); + await startInBackground(h.engine, [], h.opts); + + h.stdin.emit('close'); + + await new Promise((r) => setTimeout(r, 10)); + expect(h.engine.disconnectCalls).toBe(0); + + h.signals.emit('SIGINT'); + const code = await h.exited; + expect(code).toBe(0); + expect(h.engine.disconnectCalls).toBe(1); + }); + + test('mcpStdio=false (default) preserves stdin EOF shutdown', async () => { + // Regression guard: the guard must not over-trigger. With the env + // unset, stdin EOF must still drive shutdown so existing CLI usage + // (gbrain serve under launchd, claude-desktop's stdio MCP) is + // unchanged. + const h = makeHarness({ mcpStdio: false }); + await startInBackground(h.engine, [], h.opts); + + h.stdin.emit('end'); + const code = await h.exited; + expect(code).toBe(0); + expect(h.engine.disconnectCalls).toBe(1); + expect(h.logs.some(l => l.includes('graceful exit (stdin-end)'))).toBe(true); + }); + }); });