From e4b2f4a53e37df370828fa50cb3ff20122562686 Mon Sep 17 00:00:00 2001 From: wong2kim Date: Mon, 31 Aug 2026 00:16:54 +0900 Subject: [PATCH 1/5] feat(mcp): add a persistent Node REPL surface for agents wmux gave MCP callers no structured runtime. `terminal_send` types keys into a PTY, so there is no return value and no error channel, only screen scraping. `browser_evaluate` keeps page-scope globals but loses them on navigation and cannot reach the filesystem, the network, or require(). Add `repl_run` / `repl_reset` / `repl_sessions`: a persistent Node runtime whose variables, required modules, and open handles survive between calls. Each session is a child Node process, not an in-process vm. REPL code is code the caller wrote seconds ago and has never run, so it calls process.exit(), it throws from native callbacks, it blows the heap; the broker hosts every agent's MCP connection in one process, and an in-process runtime would turn one agent's typo into every agent's outage. The eval protocol rides the child's IPC channel, which leaves its stdout and stderr free to carry the user code's own output verbatim, with no framing to escape and no way for a printed line to forge a protocol message. Evaluation uses vm.runInThisContext rather than a fresh vm context: a fresh context has no setTimeout, no fetch and no TextEncoder, so it would need a hand-curated global list that goes stale every Node release. Running in the child's real context gives the whole standard library, and top-level let/const still persist because a Script's lexical declarations live in the context's global lexical scope. Only require() is installed explicitly, bound to the session cwd. Timeouts are two layers because one is not enough. The vm watchdog stops a synchronous runaway while keeping session state; a parent hard deadline SIGKILLs the child for the cases the watchdog cannot see (a promise that never settles, a blocked native call), and that state loss is always reported rather than papered over. Output is bounded head-and-tail per eval with an exact elided byte count, so a runaway logger cannot grow the broker's heap. Session scope is the caller's MCP connection, held in ConnectionScope beside the PlaywrightEngine for the same reason: a process-global map would hand one agent another agent's live state. Children also self-exit when their IPC channel closes, which is the only reaping signal that survives the parent being killed outright. The authority ceiling is unchanged: a caller holding `terminal_send` already drives an arbitrary shell as the user in its own pane. The child env is narrower than that shell's, starting from the gated-automation filter and additionally dropping CLAUDE* / ANTHROPIC* / AI_AGENT, which would otherwise stop a nested agent from persisting its transcript or move work onto metered auth. Withheld credential names are reported so a failing script is diagnosable in one step. --- docs/api/inventory.md | 10 + scripts/mcp-protocol-baseline.json | 5 +- src/mcp/broker.ts | 6 + src/mcp/connectionScope.ts | 8 + src/mcp/entry.ts | 5 + src/mcp/index.ts | 9 + src/mcp/repl/ReplSession.ts | 407 ++++++++++++++++++ .../__tests__/replRegistry.runtime.test.ts | 121 ++++++ .../__tests__/replSession.runtime.test.ts | 260 +++++++++++ src/mcp/repl/__tests__/replTools.test.ts | 133 ++++++ src/mcp/repl/__tests__/truncate.test.ts | 63 +++ src/mcp/repl/replRegistry.ts | 129 ++++++ src/mcp/repl/replRunnerSource.ts | 201 +++++++++ src/mcp/repl/tools.ts | 238 ++++++++++ src/mcp/repl/truncate.ts | 179 ++++++++ 15 files changed, 1773 insertions(+), 1 deletion(-) create mode 100644 src/mcp/repl/ReplSession.ts create mode 100644 src/mcp/repl/__tests__/replRegistry.runtime.test.ts create mode 100644 src/mcp/repl/__tests__/replSession.runtime.test.ts create mode 100644 src/mcp/repl/__tests__/replTools.test.ts create mode 100644 src/mcp/repl/__tests__/truncate.test.ts create mode 100644 src/mcp/repl/replRegistry.ts create mode 100644 src/mcp/repl/replRunnerSource.ts create mode 100644 src/mcp/repl/tools.ts create mode 100644 src/mcp/repl/truncate.ts diff --git a/docs/api/inventory.md b/docs/api/inventory.md index eae1a0d27..8354f7ecf 100644 --- a/docs/api/inventory.md +++ b/docs/api/inventory.md @@ -180,6 +180,16 @@ The wmux MCP server (hosted in-process, named-pipe transport to the daemon) expo | `wmux_search_panes` | `pane.search` | | | `send_message` | inter-workspace messaging — send a message to another workspace. Backed by the same handler as `a2a_task_send` (which is registered as a literal alias). NOT `input.send` semantics. | | +### REPL surface (experimental) + +Backed by **no RPC method**: the sessions are child processes of the MCP server itself, so nothing crosses the substrate boundary and there is nothing for the daemon to authorize. Scope is the caller's MCP connection — a session is not shared between panes or workspaces and does not survive a wmux restart. `full` profile only; deliberately absent from the commander surface. + +| MCP tool | Backs RPC method | Description | +|---|---|---| +| `repl_run` | *(none — in-process child)* | Evaluate JavaScript in a persistent Node runtime; variables, required modules, and open handles survive between calls. | +| `repl_reset` | *(none — in-process child)* | Kill a session and its state; the next `repl_run` starts a fresh runtime. | +| `repl_sessions` | *(none — in-process child)* | List the sessions this connection holds (cwd, pid, age, busy). | + ### A2A surface (stable) | MCP tool | Backs RPC method | Description | diff --git a/scripts/mcp-protocol-baseline.json b/scripts/mcp-protocol-baseline.json index da0d85097..4b8ea05b2 100644 --- a/scripts/mcp-protocol-baseline.json +++ b/scripts/mcp-protocol-baseline.json @@ -94,7 +94,10 @@ "surface_new", "surface_close", "pane_stash", - "pane_unstash" + "pane_unstash", + "repl_run", + "repl_reset", + "repl_sessions" ] }, "commander": { diff --git a/src/mcp/broker.ts b/src/mcp/broker.ts index 93f1f7259..cee8d8eaf 100644 --- a/src/mcp/broker.ts +++ b/src/mcp/broker.ts @@ -33,6 +33,7 @@ import { type ConnectionScope, } from './connectionScope'; import type { PlaywrightEngine } from './playwright/PlaywrightEngine'; +import { disposeReplRegistry } from './repl/replRegistry'; interface ShimHandshake { wmuxShim: number; @@ -129,6 +130,11 @@ async function hostConnection(socket: net.Socket, handshake: ShimHandshake): Pro if (engine) { void engine.disconnect().catch(() => { /* best-effort */ }); } + // Same reasoning for this caller's REPL children: they are per-connection + // and hold live state, so they die with the connection. The children also + // self-exit when their IPC channel closes, which is what covers the case + // this handler cannot — the broker being killed outright. + disposeReplRegistry(); // Close the per-connection McpServer too — without this, repeated shim // reconnects accumulate server instances in the broker process. void server.close().catch(() => { /* best-effort */ }); diff --git a/src/mcp/connectionScope.ts b/src/mcp/connectionScope.ts index 1086ad712..3f6c83599 100644 --- a/src/mcp/connectionScope.ts +++ b/src/mcp/connectionScope.ts @@ -64,6 +64,14 @@ export interface ConnectionScope { * an import cycle (snapshotCache imports this module); it owns the cast. */ snapshotCache?: unknown; + /** + * Per-connection REPL session registry, for the same reason as `playwright`: + * a REPL session is a live runtime holding the caller's variables and open + * handles, so a process-global map would hand one agent another agent's + * state. Typed as unknown to avoid an import cycle (replRegistry imports this + * module); it owns the cast. + */ + repl?: unknown; } const storage = new AsyncLocalStorage(); diff --git a/src/mcp/entry.ts b/src/mcp/entry.ts index ceb1f52d1..e691c987c 100644 --- a/src/mcp/entry.ts +++ b/src/mcp/entry.ts @@ -14,6 +14,7 @@ import { COMMANDER_MODE_ARG } from '../shared/commanderSurface'; import { clearClientIdentity } from './wmux-client'; import { PlaywrightEngine } from './playwright/PlaywrightEngine'; import { createWmuxServer } from './index'; +import { disposeReplRegistry } from './repl/replRegistry'; async function main(): Promise { const server = createWmuxServer({ @@ -38,11 +39,15 @@ async function main(): Promise { // child. Diagnostics must stay on stderr, including during shutdown. console.error('[wmux-mcp] Transport closed, disconnecting Playwright'); clearClientIdentity(); + // REPL children hold live state and are ours alone; reap them with the + // connection rather than leaving them to the disconnect watchdog. + disposeReplRegistry(); await PlaywrightEngine.getInstance().disconnect(); }; // Graceful shutdown const shutdown = async () => { + disposeReplRegistry(); await PlaywrightEngine.getInstance().disconnect(); process.exit(0); }; diff --git a/src/mcp/index.ts b/src/mcp/index.ts index d9fd783e4..3784841b7 100644 --- a/src/mcp/index.ts +++ b/src/mcp/index.ts @@ -24,6 +24,7 @@ import { registerExtractionTools } from './playwright/tools/extraction'; import { registerChannelTools } from './channels'; import { registerFanOutTools } from './fanout'; import { registerPaneLifecycleTools } from './paneLifecycle'; +import { registerReplTools } from './repl/tools'; import { getWmuxMcpServerInstructions, resolveMcpServerVersion } from './serverMetadata'; import type { RegisterWmuxToolsOptions } from './toolCatalog'; @@ -1590,6 +1591,14 @@ registerPaneLifecycleTools( MCP_CATALOG_OPTIONS, ); +// === Agent REPL tools === +// A persistent Node runtime per session, hosted as a child of THIS process and +// scoped to this connection. It takes no RPC and needs no workspace identity: +// nothing here touches the substrate, so there is nothing for the daemon to +// authorize. The authority ceiling is unchanged — a caller holding +// `terminal_send` already drives an arbitrary shell in its own pane as the user. +registerReplTools(server, MCP_CATALOG_OPTIONS); + // Hook the MCP initialize handshake so wmux substrate learns the declared // plugin identity (clientInfo.name + version). Fire `mcp.identify` once so // the trust DB picks up first-contact metadata — record-only, no diff --git a/src/mcp/repl/ReplSession.ts b/src/mcp/repl/ReplSession.ts new file mode 100644 index 000000000..1b5efb817 --- /dev/null +++ b/src/mcp/repl/ReplSession.ts @@ -0,0 +1,407 @@ +/** + * One persistent Node runtime, owned by one MCP connection. + * + * Why a child process and not `node:vm` in this process: REPL code is code the + * caller wrote seconds ago and has never run. It calls `process.exit()`, it + * blows the heap, it throws out of a native callback. In the broker topology + * this process hosts EVERY agent's MCP connection, so an in-process runtime + * would turn one agent's typo into every agent's outage. A child process makes + * the blast radius exactly one session. + * + * Lifecycle, and every way a session ends: + * + * spawn ──► starting ──ready──► idle ◄────────────┐ + * │ │ │ + * │ run() │ eval completes + * │ ▼ │ + * │ busy ─────────────┘ + * │ │ + * └────────┬────────┴──── hard deadline (SIGKILL) + * │ ├──── child exited on its own + * │ ├──── idle timer + * ▼ └──── reset() / dispose() + * dead ── state is gone; the registry respawns clean + * + * `dead` is always reported, never papered over: an agent that believes a + * variable survived when it did not writes code against a fiction. + */ +import { spawn, type ChildProcess } from 'child_process'; +import * as fs from 'fs'; +import * as path from 'path'; +import { buildGatedAutomationEnv, withheldCredentialNames } from '../../shared/envFilter'; +import { buildRunnerBootstrap, SCRIPT_TIMEOUT_MARKER, ALREADY_DECLARED_MARKER } from './replRunnerSource'; +import { OutputBuffer, truncateText, type TruncatedText } from './truncate'; + +/** Per-eval retention for each of stdout and stderr. */ +export const OUTPUT_CAP_BYTES = 64 * 1024; +/** Retention for the inspected return value. */ +export const RESULT_CAP_BYTES = 16 * 1024; +/** + * How long past the caller's timeout the child gets before SIGKILL. The vm + * watchdog inside the child stops synchronous runaways on its own; this grace + * only has to cover the message hop back, so it is short on purpose. + */ +export const HARD_KILL_GRACE_MS = 500; +/** Quiet window the pipes must show before an eval's output is considered complete. */ +const DRAIN_QUIET_MS = 20; +/** Ceiling on draining, so a still-chattering background timer cannot stall the tool. */ +const DRAIN_MAX_MS = 250; +/** How long the child gets to report `ready` before the spawn is called failed. */ +const READY_TIMEOUT_MS = 10_000; + +export type ReplSessionState = 'starting' | 'idle' | 'busy' | 'dead'; + +export interface ReplEvalOutcome { + readonly ok: boolean; + /** util.inspect of the completed value, truncated. Absent on failure. */ + readonly result?: TruncatedText; + /** Stack or message. Absent on success. */ + readonly error?: string; + readonly stdout: TruncatedText; + readonly stderr: TruncatedText; + readonly elapsedMs: number; + /** + * Set when this eval also destroyed the session (hard deadline, child exit). + * The caller MUST surface it: session state did not survive. + */ + readonly fatal?: string; + /** True when the vm watchdog stopped a synchronous runaway; state survived. */ + readonly timedOut?: boolean; + /** Set when a `let`/`const` collided with a still-live binding. */ + readonly remedy?: string; +} + +interface RunnerMessage { + readonly id?: number; + readonly ready?: boolean; + readonly ok?: boolean; + readonly result?: string; + readonly error?: string; +} + +/** Node binary plus whether Electron needs telling to behave as one. */ +function resolveNodeBinary(): { command: string; electronRunAsNode: boolean } { + const base = path.basename(process.execPath).toLowerCase(); + const isPlainNode = base === 'node' || base === 'node.exe'; + return { command: process.execPath, electronRunAsNode: !isPlainNode }; +} + +/** + * The environment a REPL child inherits. + * + * Starts from the same gated-automation filter every agent pane spawn uses, so + * the REPL is never a wider hole than the shell the caller already drives with + * `terminal_send`: wmux/Electron internals and credential-shaped names are + * dropped. Then it drops `CLAUDE*` / `ANTHROPIC*` / `AI_AGENT` on top, for the + * reason `scrubBrainSpawnEnv` documents — wmux is routinely launched from + * inside a Claude Code session, and passing that session's markers into a child + * makes any nested agent silently stop persisting its transcript, while an + * ambient API key would quietly move work onto metered auth. + */ +export function buildReplChildEnv( + base: NodeJS.ProcessEnv = process.env, +): { env: Record; withheldCredentials: string[] } { + const env = buildGatedAutomationEnv(base); + for (const key of Object.keys(env)) { + const upper = key.toUpperCase(); + if (upper.startsWith('CLAUDE') || upper.startsWith('ANTHROPIC') || upper === 'AI_AGENT') { + delete env[key]; + } + } + return { env, withheldCredentials: withheldCredentialNames(base) }; +} + +function delay(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +export interface ReplSessionOptions { + readonly name: string; + readonly cwd: string; + /** Idle lifetime before the child is reaped. */ + readonly idleMs: number; +} + +export class ReplSession { + readonly name: string; + readonly cwd: string; + readonly createdAt = Date.now(); + readonly withheldCredentials: string[]; + + private child: ChildProcess | null = null; + private state: ReplSessionState = 'starting'; + private deathReason: string | null = null; + private nextEvalId = 1; + private evalCount = 0; + private lastUsedAt = Date.now(); + private lastChunkAt = 0; + private out = new OutputBuffer(OUTPUT_CAP_BYTES); + private err = new OutputBuffer(OUTPUT_CAP_BYTES); + private pending: ((message: RunnerMessage) => void) | null = null; + private readonly ready: Promise; + private idleTimer: NodeJS.Timeout | null = null; + private readonly idleMs: number; + + constructor(options: ReplSessionOptions) { + this.name = options.name; + this.cwd = options.cwd; + this.idleMs = options.idleMs; + + // Validated before spawn so a bad cwd reads as a bad cwd, not as an opaque + // ENOENT from a process that never started. + let stat: fs.Stats; + try { + stat = fs.statSync(options.cwd); + } catch { + throw new Error(`cwd does not exist: ${options.cwd}`); + } + if (!stat.isDirectory()) { + throw new Error(`cwd is not a directory: ${options.cwd}`); + } + + const { command, electronRunAsNode } = resolveNodeBinary(); + const { env, withheldCredentials } = buildReplChildEnv(); + this.withheldCredentials = withheldCredentials; + // Re-added AFTER the filter (which strips it as an internal). Setting it + // here rather than leaving it in place keeps the filter honest: nothing + // reaches the child that was not deliberately put back. + if (electronRunAsNode) env.ELECTRON_RUN_AS_NODE = '1'; + + const child = spawn(command, ['-e', buildRunnerBootstrap()], { + cwd: options.cwd, + env, + stdio: ['ignore', 'pipe', 'pipe', 'ipc'], + windowsHide: true, + }); + this.child = child; + + child.stdout?.on('data', (chunk: Buffer) => { + this.lastChunkAt = Date.now(); + this.out.append(chunk); + }); + child.stderr?.on('data', (chunk: Buffer) => { + this.lastChunkAt = Date.now(); + this.err.append(chunk); + }); + child.on('error', (error) => this.markDead(`failed to spawn the REPL runtime: ${error.message}`)); + child.on('exit', (code, signal) => { + this.markDead( + this.deathReason ?? + `the REPL process exited on its own (code ${String(code)}, signal ${String(signal)})`, + ); + }); + + this.ready = new Promise((resolve, reject) => { + const timer = setTimeout(() => { + this.destroy('the REPL runtime did not come up within 10s'); + reject(new Error('the REPL runtime did not come up within 10s')); + }, READY_TIMEOUT_MS); + timer.unref?.(); + const onMessage = (raw: unknown) => { + const message = raw as RunnerMessage; + if (message?.ready) { + clearTimeout(timer); + child.off('message', onMessage); + if (this.state === 'starting') this.state = 'idle'; + resolve(); + } + }; + child.on('message', onMessage); + child.once('exit', () => { + clearTimeout(timer); + reject(new Error(this.deathReason ?? 'the REPL runtime exited during startup')); + }); + }); + // A rejection is delivered through run(); nothing else awaits this promise, + // and an unobserved rejection would take the whole MCP server down. + this.ready.catch(() => { /* surfaced by run() */ }); + + child.on('message', (raw: unknown) => { + const message = raw as RunnerMessage; + if (message?.id === undefined) return; + const settle = this.pending; + this.pending = null; + settle?.(message); + }); + + this.armIdleTimer(); + } + + get status(): ReplSessionState { + return this.state; + } + + get pid(): number | undefined { + return this.child?.pid; + } + + get evals(): number { + return this.evalCount; + } + + get lastUsed(): number { + return this.lastUsedAt; + } + + get dead(): boolean { + return this.state === 'dead'; + } + + get busy(): boolean { + return this.state === 'busy'; + } + + /** Why the session died, for the message the caller reads. */ + get diedBecause(): string | null { + return this.deathReason; + } + + private armIdleTimer(): void { + if (this.idleTimer) clearTimeout(this.idleTimer); + this.idleTimer = setTimeout(() => { + this.destroy(`idle for ${Math.round(this.idleMs / 60000)} minutes`); + }, this.idleMs); + // Never hold the MCP server open just to wait out an idle REPL. + this.idleTimer.unref?.(); + } + + private markDead(reason: string): void { + if (this.state === 'dead') return; + this.state = 'dead'; + this.deathReason = reason; + if (this.idleTimer) { + clearTimeout(this.idleTimer); + this.idleTimer = null; + } + const settle = this.pending; + this.pending = null; + settle?.({ id: -1, ok: false, error: reason }); + } + + /** Kill the child and mark the session dead. Idempotent. */ + destroy(reason: string): void { + if (this.state !== 'dead') this.deathReason = reason; + const child = this.child; + this.child = null; + this.markDead(reason); + if (child && child.exitCode === null && child.signalCode === null) { + try { + child.kill('SIGKILL'); + } catch { + /* already gone */ + } + } + } + + /** + * Wait for the pipes to go quiet. The IPC result and the stdout bytes travel + * on different channels, so the completion message can land before the last + * `console.log` has been read — returning immediately would drop the tail of + * the output the caller is asking for. + */ + private async drain(): Promise { + const start = Date.now(); + await delay(DRAIN_QUIET_MS); + while (Date.now() - start < DRAIN_MAX_MS) { + if (Date.now() - this.lastChunkAt >= DRAIN_QUIET_MS) return; + await delay(5); + } + } + + private takeOutput(): { stdout: TruncatedText; stderr: TruncatedText } { + const stdout = this.out.render(); + const stderr = this.err.render(); + this.out = new OutputBuffer(OUTPUT_CAP_BYTES); + this.err = new OutputBuffer(OUTPUT_CAP_BYTES); + return { stdout, stderr }; + } + + async run(code: string, timeoutMs: number): Promise { + const started = Date.now(); + await this.ready; + if (this.state === 'dead') { + throw new Error(this.deathReason ?? 'the REPL session is gone'); + } + if (this.state === 'busy') { + throw new Error('this REPL session is already running code'); + } + + this.state = 'busy'; + this.evalCount += 1; + this.lastUsedAt = Date.now(); + const id = this.nextEvalId++; + + const message = await new Promise((resolve) => { + this.pending = resolve; + // Layer two of the timeout. The child's vm watchdog cannot see a promise + // that never settles or a blocked native call, so the only reliable stop + // is killing the process — which is why this costs the session's state + // and the vm watchdog (which does not) is tried first. + const hard = setTimeout(() => { + this.destroy( + `hard timeout: the code did not finish within ${timeoutMs}ms and did not stop when asked, ` + + 'so the REPL process was killed and all session state was lost', + ); + }, timeoutMs + HARD_KILL_GRACE_MS); + hard.unref?.(); + const settle = this.pending; + this.pending = (msg) => { + clearTimeout(hard); + settle?.(msg); + }; + try { + this.child?.send({ id, code, timeoutMs }); + } catch (error) { + clearTimeout(hard); + this.destroy(`the REPL process could not be reached: ${String(error)}`); + } + }); + + await this.drain(); + const { stdout, stderr } = this.takeOutput(); + const elapsedMs = Date.now() - started; + + // Read through the getter: the assignment above narrows `this.state` to + // 'busy' for the checker, but the eval could have killed the session while + // we were awaiting it. + if (this.dead) { + return { + ok: false, + error: this.deathReason ?? 'the REPL session ended', + fatal: this.deathReason ?? 'the REPL session ended', + stdout, + stderr, + elapsedMs, + }; + } + + this.state = 'idle'; + this.lastUsedAt = Date.now(); + this.armIdleTimer(); + + if (message.ok) { + return { + ok: true, + result: truncateText(message.result ?? 'undefined', RESULT_CAP_BYTES), + stdout, + stderr, + elapsedMs, + }; + } + + const error = message.error ?? 'unknown REPL error'; + return { + ok: false, + error, + stdout, + stderr, + elapsedMs, + timedOut: error.includes(SCRIPT_TIMEOUT_MARKER) || undefined, + remedy: error.includes(ALREADY_DECLARED_MARKER) + ? 'That name is already bound in this session. `let`/`const` cannot be re-declared ' + + 'against a live binding — assign without a keyword (`x = ...`) to update it, or call ' + + 'repl_reset to start from a clean runtime.' + : undefined, + }; + } +} diff --git a/src/mcp/repl/__tests__/replRegistry.runtime.test.ts b/src/mcp/repl/__tests__/replRegistry.runtime.test.ts new file mode 100644 index 000000000..5cc0f70c1 --- /dev/null +++ b/src/mcp/repl/__tests__/replRegistry.runtime.test.ts @@ -0,0 +1,121 @@ +/** + * Registry behavior against real children: the cap that protects the shared + * broker, the respawn-after-death contract, and connection-scoped isolation. + */ +import * as os from 'os'; +import { afterEach, describe, expect, it } from 'vitest'; +import { runInConnectionScope, createConnectionScope } from '../../connectionScope'; +import { + MAX_SESSIONS_PER_CONNECTION, + ReplRegistry, + disposeReplRegistry, + getReplRegistry, +} from '../replRegistry'; + +const registries: ReplRegistry[] = []; + +function makeRegistry(): ReplRegistry { + const registry = new ReplRegistry(); + registries.push(registry); + return registry; +} + +afterEach(() => { + while (registries.length > 0) registries.pop()?.disposeAll(); +}); + +describe('ReplRegistry', () => { + it('reuses a live session and reports when it had to start a new one', async () => { + const registry = makeRegistry(); + const first = registry.acquire('default', os.tmpdir()); + expect(first.created).toBe(true); + await first.session.run('let shared = 5;', 5000); + + const second = registry.acquire('default', os.tmpdir()); + expect(second.created).toBe(false); + expect(second.session).toBe(first.session); + const outcome = await second.session.run('shared', 5000); + expect(outcome.result?.text).toBe('5'); + }, 20_000); + + it('respawns after a death and names the reason the old state is gone', async () => { + const registry = makeRegistry(); + const first = registry.acquire('default', os.tmpdir()); + await first.session.run('let gone = 1;', 5000); + await first.session.run('process.exit(0)', 5000); + expect(first.session.dead).toBe(true); + + const second = registry.acquire('default', os.tmpdir()); + expect(second.created).toBe(true); + expect(second.previousDeath).toContain('exited on its own'); + const outcome = await second.session.run('typeof gone', 5000); + expect(outcome.result?.text).toContain('undefined'); + }, 20_000); + + it('refuses to exceed the per-connection session cap', () => { + const registry = makeRegistry(); + for (let i = 0; i < MAX_SESSIONS_PER_CONNECTION; i++) { + registry.acquire(`s${i}`, os.tmpdir()); + } + expect(() => registry.acquire('one-too-many', os.tmpdir())).toThrow( + /already holds 4 REPL sessions/, + ); + // Freeing one makes room again. + expect(registry.reset('s0')).toBe(true); + expect(() => registry.acquire('one-too-many', os.tmpdir())).not.toThrow(); + }, 20_000); + + it('kills the child on reset and starts clean afterwards', async () => { + const registry = makeRegistry(); + const { session } = registry.acquire('default', os.tmpdir()); + await session.run('let wiped = "old";', 5000); + const pid = session.pid; + + expect(registry.reset('default')).toBe(true); + expect(registry.reset('default')).toBe(false); + await new Promise((r) => setTimeout(r, 200)); + expect(() => process.kill(pid as number, 0)).toThrow(); + + const fresh = registry.acquire('default', os.tmpdir()); + expect(fresh.created).toBe(true); + const outcome = await fresh.session.run('typeof wiped', 5000); + expect(outcome.result?.text).toContain('undefined'); + }, 20_000); + + it('lists only live sessions', async () => { + const registry = makeRegistry(); + registry.acquire('a', os.tmpdir()); + const { session } = registry.acquire('b', os.tmpdir()); + expect(registry.list().map((s) => s.name).sort()).toEqual(['a', 'b']); + await session.run('process.exit(0)', 5000); + expect(registry.list().map((s) => s.name)).toEqual(['a']); + }, 20_000); +}); + +describe('connection scoping', () => { + it('gives each connection its own sessions and disposes only its own', async () => { + const scopeA = createConnectionScope(); + const scopeB = createConnectionScope(); + + const runIn = (scope: ReturnType, fn: () => T): T => + runInConnectionScope(scope, fn); + + const a = runIn(scopeA, () => getReplRegistry().acquire('default', os.tmpdir())); + const b = runIn(scopeB, () => getReplRegistry().acquire('default', os.tmpdir())); + expect(a.session).not.toBe(b.session); + + await a.session.run('let onlyA = 1;', 5000); + const bView = await b.session.run('typeof onlyA', 5000); + expect(bView.result?.text).toContain('undefined'); + + // Tearing down A's connection must not touch B's live runtime. + runIn(scopeA, () => disposeReplRegistry()); + expect(a.session.dead).toBe(true); + expect(b.session.dead).toBe(false); + const stillLive = await b.session.run('1 + 1', 5000); + expect(stillLive.result?.text).toBe('2'); + + runIn(scopeB, () => disposeReplRegistry()); + expect(b.session.dead).toBe(true); + }, 20_000); +}); diff --git a/src/mcp/repl/__tests__/replSession.runtime.test.ts b/src/mcp/repl/__tests__/replSession.runtime.test.ts new file mode 100644 index 000000000..147ff4b48 --- /dev/null +++ b/src/mcp/repl/__tests__/replSession.runtime.test.ts @@ -0,0 +1,260 @@ +/** + * Real-child tests for the REPL runtime. + * + * These spawn actual Node processes, so they live in the `*.runtime.test.ts` + * suite (serialised, `vitest.runtime.config.ts`). Everything load-bearing about + * this feature — that state survives, that a runaway stops, that a killed + * parent does not orphan children — is only true if it is true of a real + * process, so mocking the child would test nothing worth testing. + */ +import * as os from 'os'; +import { afterEach, describe, expect, it } from 'vitest'; +import { ReplSession, buildReplChildEnv } from '../ReplSession'; + +const live: ReplSession[] = []; + +function makeSession(cwd = os.tmpdir(), idleMs = 60_000): ReplSession { + const session = new ReplSession({ name: 'test', cwd, idleMs }); + live.push(session); + return session; +} + +function isAlive(pid: number | undefined): boolean { + if (pid === undefined) return false; + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +afterEach(() => { + while (live.length > 0) live.pop()?.destroy('test cleanup'); +}); + +describe('ReplSession state persistence', () => { + it('keeps let/const/var bindings and required modules across calls', async () => { + const session = makeSession(); + + const first = await session.run('let counter = 41; var legacy = "kept"; const tag = "t";', 5000); + expect(first.ok).toBe(true); + + const second = await session.run('counter += 1; [counter, legacy, tag]', 5000); + expect(second.ok).toBe(true); + expect(second.result?.text).toContain('42'); + expect(second.result?.text).toContain('kept'); + + // A module required in one call is still bound in the next. + await session.run('globalThis.osmod = require("os");', 5000); + const third = await session.run('typeof osmod.platform', 5000); + expect(third.result?.text).toContain('function'); + }); + + it('captures the code\'s own stdout and stderr separately from the result', async () => { + const session = makeSession(); + const outcome = await session.run( + 'console.log("to stdout"); console.error("to stderr"); 7 * 6', + 5000, + ); + expect(outcome.ok).toBe(true); + expect(outcome.stdout.text).toContain('to stdout'); + expect(outcome.stderr.text).toContain('to stderr'); + expect(outcome.result?.text).toBe('42'); + }); + + it('supports top-level await and reports the resolved value', async () => { + const session = makeSession(); + const outcome = await session.run( + 'const delayed = await new Promise((r) => setTimeout(() => r("resolved"), 20)); delayed', + 5000, + ); + expect(outcome.ok).toBe(true); + expect(outcome.result?.text).toContain('resolved'); + }); + + it('returns the trailing expression of a multi-statement awaiting snippet', async () => { + const session = makeSession(); + const outcome = await session.run( + 'const a = await Promise.resolve(1);\nconst b = a + 41;\nb', + 5000, + ); + expect(outcome.ok).toBe(true); + expect(outcome.result?.text).toBe('42'); + }); + + it('does not persist declarations made inside an awaiting snippet, as documented', async () => { + const session = makeSession(); + await session.run('const ephemeral = await Promise.resolve(1); ephemeral', 5000); + const after = await session.run('typeof ephemeral', 5000); + expect(after.result?.text).toContain('undefined'); + + // The documented workaround: assign to a global instead of declaring. + await session.run('globalThis.kept = await Promise.resolve("yes"); kept', 5000); + const kept = await session.run('kept', 5000); + expect(kept.result?.text).toContain('yes'); + }); + + it('awaits a returned promise instead of reporting it as pending', async () => { + const session = makeSession(); + const outcome = await session.run('Promise.resolve({ ok: 1 })', 5000); + expect(outcome.ok).toBe(true); + expect(outcome.result?.text).toContain('ok: 1'); + expect(outcome.result?.text).not.toContain('pending'); + }); + + it('reports a thrown error with its stack and keeps the session alive', async () => { + const session = makeSession(); + await session.run('let survivor = "alive";', 5000); + const failed = await session.run('throw new Error("boom")', 5000); + expect(failed.ok).toBe(false); + expect(failed.error).toContain('boom'); + expect(failed.fatal).toBeUndefined(); + + const after = await session.run('survivor', 5000); + expect(after.ok).toBe(true); + expect(after.result?.text).toContain('alive'); + }); + + it('explains a let re-declaration instead of leaving a bare V8 error', async () => { + const session = makeSession(); + await session.run('let dup = 1;', 5000); + const again = await session.run('let dup = 2;', 5000); + expect(again.ok).toBe(false); + expect(again.remedy).toContain('already bound'); + expect(again.fatal).toBeUndefined(); + }); +}); + +describe('ReplSession timeouts', () => { + it('stops a synchronous runaway without losing session state', async () => { + const session = makeSession(); + await session.run('let keepme = "still here";', 5000); + + const outcome = await session.run('while (true) {}', 300); + expect(outcome.ok).toBe(false); + expect(outcome.timedOut).toBe(true); + // The vm watchdog stops the script; the process is untouched, so state lives. + expect(outcome.fatal).toBeUndefined(); + expect(session.dead).toBe(false); + + const after = await session.run('keepme', 5000); + expect(after.result?.text).toContain('still here'); + }, 20_000); + + it('hard-kills the child when a promise never settles, and says state was lost', async () => { + const session = makeSession(); + const pid = session.pid; + await session.run('let doomed = 1;', 5000); + + const outcome = await session.run('new Promise(() => {})', 300); + expect(outcome.ok).toBe(false); + expect(outcome.fatal).toContain('killed'); + expect(session.dead).toBe(true); + await new Promise((r) => setTimeout(r, 200)); + expect(isAlive(pid)).toBe(false); + }, 20_000); + + it('reports the child exiting on its own rather than hanging', async () => { + const session = makeSession(); + const outcome = await session.run('process.exit(3)', 5000); + expect(outcome.ok).toBe(false); + expect(outcome.fatal).toContain('exited on its own'); + expect(session.dead).toBe(true); + }, 20_000); + + it('survives an uncaught exception thrown from a background timer', async () => { + const session = makeSession(); + // Fire well after this run's output drain, so the throw lands between evals + // and the next run is the one that must surface it. + await session.run('setTimeout(() => { throw new Error("late boom"); }, 250);', 5000); + await new Promise((r) => setTimeout(r, 500)); + const after = await session.run('"still running"', 5000); + expect(session.dead).toBe(false); + expect(after.ok).toBe(true); + // The background failure is not swallowed — it surfaces on the next run. + expect(after.stderr.text).toContain('late boom'); + }, 20_000); +}); + +describe('ReplSession output discipline', () => { + it('truncates a flood of stdout while counting every byte', async () => { + const session = makeSession(); + const outcome = await session.run( + 'for (let i = 0; i < 20000; i++) console.log("x".repeat(80)); "done"', + 15_000, + ); + expect(outcome.ok).toBe(true); + expect(outcome.stdout.truncated).toBe(true); + expect(outcome.stdout.totalBytes).toBeGreaterThan(1_000_000); + expect(Buffer.byteLength(outcome.stdout.text)).toBeLessThan(70 * 1024); + expect(outcome.stdout.text).toContain('bytes elided'); + }, 30_000); + + it('bounds a huge return value', async () => { + const session = makeSession(); + const outcome = await session.run('"y".repeat(200000)', 10_000); + expect(outcome.ok).toBe(true); + expect(Buffer.byteLength(outcome.result?.text ?? '')).toBeLessThan(20 * 1024); + }, 20_000); +}); + +describe('ReplSession isolation', () => { + it('withholds nested-agent and credential env from the child', async () => { + const session = makeSession(); + const outcome = await session.run( + 'JSON.stringify(Object.keys(process.env).filter((k) => ' + + '/^(CLAUDE|ANTHROPIC)/i.test(k) || k === "AI_AGENT" || /_TOKEN$|_KEY$/i.test(k)))', + 5000, + ); + expect(outcome.ok).toBe(true); + expect(outcome.result?.text).toContain('[]'); + }); + + it('drops CLAUDE*/ANTHROPIC*/AI_AGENT from the built env', () => { + const { env } = buildReplChildEnv({ + PATH: '/usr/bin', + CLAUDE_CODE_CHILD_SESSION: 'x', + CLAUDECODE: '1', + ANTHROPIC_API_KEY: 'secret', + AI_AGENT: 'claude', + GITHUB_TOKEN: 'tok', + HOME: '/home/u', + }); + expect(env.PATH).toBe('/usr/bin'); + expect(env.HOME).toBe('/home/u'); + expect(env.CLAUDE_CODE_CHILD_SESSION).toBeUndefined(); + expect(env.CLAUDECODE).toBeUndefined(); + expect(env.ANTHROPIC_API_KEY).toBeUndefined(); + expect(env.AI_AGENT).toBeUndefined(); + expect(env.GITHUB_TOKEN).toBeUndefined(); + }); + + it('reports which credential names were withheld', () => { + const { withheldCredentials } = buildReplChildEnv({ PATH: '/b', GITHUB_TOKEN: 'x' }); + expect(withheldCredentials).toContain('GITHUB_TOKEN'); + }); + + it('runs in the cwd it was given and rejects one that does not exist', async () => { + const session = makeSession(os.tmpdir()); + const outcome = await session.run('process.cwd()', 5000); + expect(outcome.ok).toBe(true); + // macOS reports /var/folders/... as /private/var/folders/...; compare the tail. + expect(outcome.result?.text).toContain(os.tmpdir().replace(/^\/private/, '')); + + expect(() => makeSession('/definitely/not/a/real/path')).toThrow(/cwd does not exist/); + }); + + it('exits when its parent IPC channel closes, so a crashed parent orphans nothing', async () => { + const session = makeSession(); + await session.run('1', 5000); + const pid = session.pid; + expect(isAlive(pid)).toBe(true); + + // Close the channel WITHOUT killing the child, which is what a SIGKILLed + // parent looks like from the child's side. + (session as unknown as { child: { disconnect(): void } }).child.disconnect(); + await new Promise((r) => setTimeout(r, 500)); + expect(isAlive(pid)).toBe(false); + }, 20_000); +}); diff --git a/src/mcp/repl/__tests__/replTools.test.ts b/src/mcp/repl/__tests__/replTools.test.ts new file mode 100644 index 000000000..9b8329c37 --- /dev/null +++ b/src/mcp/repl/__tests__/replTools.test.ts @@ -0,0 +1,133 @@ +import { describe, expect, it } from 'vitest'; +import { + expectCommanderCatalogLockstep, + expectFrozenCatalog, +} from '../../__tests__/catalogAssertions'; +import { + DEFAULT_TIMEOUT_MS, + MAX_TIMEOUT_MS, + MIN_TIMEOUT_MS, + clampTimeout, + createReplToolCatalog, + formatOutcome, +} from '../tools'; +import { DEFAULT_SESSION_NAME, isValidSessionName } from '../replRegistry'; +import { truncateText } from '../truncate'; + +describe('repl tool catalog', () => { + const catalog = createReplToolCatalog(); + + it('registers exactly the three MVP tools', () => { + expect(catalog.map((spec) => spec.name)).toEqual(['repl_run', 'repl_reset', 'repl_sessions']); + }); + + it('is frozen and stays out of the commander surface', () => { + expectFrozenCatalog(catalog); + expectCommanderCatalogLockstep(catalog); + for (const spec of catalog) { + expect(spec.profiles).toEqual(['full']); + } + }); + + it('tells the caller the runtime is unsandboxed and connection-scoped', () => { + const run = catalog.find((spec) => spec.name === 'repl_run'); + // Both are load-bearing honesty: an agent that thinks this is a jail, or + // that state outlives wmux, will write code against a fiction. + expect(run?.description).toContain('NO sandbox'); + expect(run?.description).toContain('MCP connection'); + }); +}); + +describe('clampTimeout', () => { + it('defaults when unset or not a number', () => { + expect(clampTimeout(undefined)).toBe(DEFAULT_TIMEOUT_MS); + expect(clampTimeout(Number.NaN)).toBe(DEFAULT_TIMEOUT_MS); + expect(clampTimeout(Number.POSITIVE_INFINITY)).toBe(DEFAULT_TIMEOUT_MS); + }); + + it('clamps rather than rejecting out-of-range requests', () => { + expect(clampTimeout(1)).toBe(MIN_TIMEOUT_MS); + expect(clampTimeout(-5000)).toBe(MIN_TIMEOUT_MS); + expect(clampTimeout(10 * MAX_TIMEOUT_MS)).toBe(MAX_TIMEOUT_MS); + expect(clampTimeout(1234.9)).toBe(1234); + }); +}); + +describe('session names', () => { + it('accepts the boring shapes and rejects everything else', () => { + expect(isValidSessionName(DEFAULT_SESSION_NAME)).toBe(true); + expect(isValidSessionName('build-2.worker_1')).toBe(true); + expect(isValidSessionName('')).toBe(false); + expect(isValidSessionName('has space')).toBe(false); + expect(isValidSessionName('../escape')).toBe(false); + expect(isValidSessionName('a'.repeat(65))).toBe(false); + expect(isValidSessionName('a'.repeat(64))).toBe(true); + }); +}); + +describe('formatOutcome', () => { + const empty = truncateText('', 1024); + + it('renders result, stdout, and stderr in labelled blocks', () => { + const text = formatOutcome( + 'default', + { + ok: true, + result: truncateText('42', 1024), + stdout: truncateText('printed\n', 1024), + stderr: truncateText('warned\n', 1024), + elapsedMs: 12, + }, + [], + ); + expect(text).toContain('session default · ok · 12ms'); + expect(text).toContain('--- stdout ---\nprinted'); + expect(text).toContain('--- stderr ---\nwarned'); + expect(text).toContain('--- result ---\n42'); + }); + + it('surfaces the fatal reason so lost state is never silent', () => { + const text = formatOutcome( + 'default', + { + ok: false, + error: 'killed', + fatal: 'hard timeout: session state was lost', + stdout: empty, + stderr: empty, + elapsedMs: 500, + }, + [], + ); + expect(text).toContain('note: hard timeout: session state was lost'); + expect(text).toContain('--- error ---'); + }); + + it('says state survived when the vm watchdog stopped the run', () => { + const text = formatOutcome( + 'default', + { ok: false, error: 'Script execution timed out', stdout: empty, stderr: empty, elapsedMs: 300, timedOut: true }, + [], + ); + expect(text).toContain('Session state survived'); + }); + + it('reports truncation with the true byte total', () => { + const flood = truncateText('z'.repeat(5000), 400); + const text = formatOutcome( + 'default', + { ok: true, result: truncateText('1', 1024), stdout: flood, stderr: empty, elapsedMs: 5 }, + [], + ); + expect(text).toContain('stdout truncated: 5000 bytes total'); + }); + + it('passes through registry notes such as a fresh runtime', () => { + const text = formatOutcome( + 'build', + { ok: true, result: truncateText('1', 1024), stdout: empty, stderr: empty, elapsedMs: 1 }, + ['started a new runtime in /tmp'], + ); + expect(text).toContain('note: started a new runtime in /tmp'); + }); +}); diff --git a/src/mcp/repl/__tests__/truncate.test.ts b/src/mcp/repl/__tests__/truncate.test.ts new file mode 100644 index 000000000..fdef8ad9a --- /dev/null +++ b/src/mcp/repl/__tests__/truncate.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it } from 'vitest'; +import { OutputBuffer, truncateText } from '../truncate'; + +describe('truncateText', () => { + it('leaves text under the cap untouched', () => { + const result = truncateText('hello world', 1024); + expect(result.text).toBe('hello world'); + expect(result.truncated).toBe(false); + expect(result.elidedBytes).toBe(0); + expect(result.totalBytes).toBe(11); + }); + + it('keeps head and tail and reports the elided byte count', () => { + const input = `${'a'.repeat(500)}${'b'.repeat(500)}${'c'.repeat(500)}`; + const result = truncateText(input, 400); + expect(result.truncated).toBe(true); + expect(result.totalBytes).toBe(1500); + // Head is 75% of the cap, tail the remainder; the middle is what goes. + expect(result.text.startsWith('a'.repeat(300))).toBe(true); + expect(result.text.endsWith('c'.repeat(100))).toBe(true); + expect(result.elidedBytes).toBe(1100); + expect(result.text).toContain('1100 bytes elided'); + }); + + it('never cuts a multi-byte codepoint in half', () => { + const input = '한'.repeat(400); // 3 bytes each + const result = truncateText(input, 100); + expect(result.truncated).toBe(true); + expect(result.text).not.toContain('�'); + }); +}); + +describe('OutputBuffer', () => { + it('renders everything when the total stays under the cap', () => { + const buf = new OutputBuffer(1024); + buf.append(Buffer.from('one ')); + buf.append(Buffer.from('two')); + const result = buf.render(); + expect(result.text).toBe('one two'); + expect(result.truncated).toBe(false); + expect(result.totalBytes).toBe(7); + }); + + it('counts every byte while retaining only head and tail', () => { + const buf = new OutputBuffer(1000); + for (let i = 0; i < 1000; i++) buf.append(Buffer.from('0123456789')); + const result = buf.render(); + expect(result.totalBytes).toBe(10_000); + expect(result.truncated).toBe(true); + // Retention is bounded by the cap plus the elision marker, no matter how + // much arrived — this is the property that protects the shared broker. + expect(Buffer.byteLength(result.text)).toBeLessThan(1100); + expect(result.elidedBytes).toBe(9000); + }); + + it('keeps the most recent bytes in the tail', () => { + const buf = new OutputBuffer(400); + buf.append(Buffer.from('S'.repeat(1000))); + buf.append(Buffer.from('END')); + const result = buf.render(); + expect(result.text.endsWith('END')).toBe(true); + }); +}); diff --git a/src/mcp/repl/replRegistry.ts b/src/mcp/repl/replRegistry.ts new file mode 100644 index 000000000..45679a14b --- /dev/null +++ b/src/mcp/repl/replRegistry.ts @@ -0,0 +1,129 @@ +/** + * Per-connection REPL session registry. + * + * Scope is the CALLER'S MCP CONNECTION, held in ConnectionScope exactly the way + * the PlaywrightEngine is. The broker hosts N server instances in one process, + * so a process-global map would let one agent read and clobber another agent's + * live runtime — variables, open handles, and all. Scoping it with the rest of + * the per-connection state makes that unrepresentable rather than merely + * avoided. The single-child stdio entry establishes no scope, so it falls back + * to a module global, which in that topology is already one-agent-per-process. + */ +import { getConnectionScope } from '../connectionScope'; +import { ReplSession } from './ReplSession'; + +/** Concurrent sessions one connection may hold. */ +export const MAX_SESSIONS_PER_CONNECTION = 4; +/** + * Idle lifetime. Deliberately shorter than a working session feels: in the + * broker every live child is ~40 MB of resident memory multiplied by every + * connected agent, and an abandoned REPL is indistinguishable from a busy one + * until it is reaped. + */ +export const IDLE_TIMEOUT_MS = 15 * 60 * 1000; + +/** Session names are used in messages and as map keys; keep them boring. */ +const SESSION_NAME_RE = /^[A-Za-z0-9._-]{1,64}$/; + +export const DEFAULT_SESSION_NAME = 'default'; + +export function isValidSessionName(name: string): boolean { + return SESSION_NAME_RE.test(name); +} + +export class ReplRegistry { + private readonly sessions = new Map(); + + /** Live sessions, oldest first. Dead ones are swept as they are noticed. */ + list(): ReplSession[] { + this.sweep(); + return [...this.sessions.values()]; + } + + get(name: string): ReplSession | undefined { + this.sweep(); + return this.sessions.get(name); + } + + /** + * The live session for `name`, spawning one if there is none or if the + * previous one died. Returns whether a new runtime was created so the caller + * can tell the agent its state is gone rather than letting it assume. + */ + acquire(name: string, cwd: string): { session: ReplSession; created: boolean; previousDeath?: string } { + // Read the named slot BEFORE sweeping. The sweep drops dead sessions, and + // dropping this one first would throw away the reason its state vanished — + // which is the one thing the caller most needs to be told. + const existing = this.sessions.get(name); + if (existing && !existing.dead) return { session: existing, created: false }; + + const previousDeath = existing?.diedBecause ?? undefined; + if (existing) this.sessions.delete(name); + this.sweep(); + + if (this.sessions.size >= MAX_SESSIONS_PER_CONNECTION) { + throw new Error( + `this connection already holds ${MAX_SESSIONS_PER_CONNECTION} REPL sessions ` + + `(${[...this.sessions.keys()].join(', ')}). Call repl_reset on one before starting another.`, + ); + } + + const session = new ReplSession({ name, cwd, idleMs: IDLE_TIMEOUT_MS }); + this.sessions.set(name, session); + return { session, created: true, previousDeath }; + } + + /** Kill and forget one session. Returns false when there was nothing to reset. */ + reset(name: string): boolean { + const session = this.sessions.get(name); + if (!session) return false; + session.destroy('reset by repl_reset'); + this.sessions.delete(name); + return true; + } + + /** Kill every session. Called when the connection goes away. */ + disposeAll(): void { + for (const session of this.sessions.values()) { + session.destroy('the MCP connection closed'); + } + this.sessions.clear(); + } + + /** Forget sessions whose child is already gone (idle reap, crash, kill). */ + private sweep(): void { + for (const [name, session] of this.sessions) { + if (session.dead) this.sessions.delete(name); + } + } +} + +/** Single-child (stdio entry) fallback — no connection scope exists there. */ +let processRegistry: ReplRegistry | null = null; + +/** The calling connection's registry, created on first use. */ +export function getReplRegistry(): ReplRegistry { + const scope = getConnectionScope(); + if (scope) { + if (!scope.repl) scope.repl = new ReplRegistry(); + return scope.repl as ReplRegistry; + } + if (!processRegistry) processRegistry = new ReplRegistry(); + return processRegistry; +} + +/** + * Tear down whichever registry belongs to the caller. Safe to call when none + * was ever created — a connection that never touched the REPL has nothing to + * dispose, and creating one just to destroy it would spawn nothing anyway. + */ +export function disposeReplRegistry(): void { + const scope = getConnectionScope(); + if (scope) { + (scope.repl as ReplRegistry | undefined)?.disposeAll(); + scope.repl = undefined; + return; + } + processRegistry?.disposeAll(); + processRegistry = null; +} diff --git a/src/mcp/repl/replRunnerSource.ts b/src/mcp/repl/replRunnerSource.ts new file mode 100644 index 000000000..764ec4860 --- /dev/null +++ b/src/mcp/repl/replRunnerSource.ts @@ -0,0 +1,201 @@ +/** + * The program that runs INSIDE each REPL child process. + * + * It is a string, not a module, on purpose: the MCP server ships as a single + * esbuild bundle (`dist/mcp-bundle/index.js`), so a second entry point would + * need its own build step, its own copy into the packaged app, and its own + * "where is my file when relocated" resolution. An embedded string has none of + * that and cannot drift from the parent that speaks its protocol. + * + * Topology and why each piece is where it is: + * + * MCP server (broker or stdio child) + * │ ┌── ipc (fd 3) ───────────────┐ the eval protocol: {id,code,timeoutMs} + * │ │ │ out, {id,ok,result|error} back + * └──┤ node -e │ + * │ │ + * └── stdout / stderr (pipes) ──┘ the USER CODE's own output + * + * Splitting the protocol onto the IPC channel is what lets stdout stay verbatim + * user output. A single-stream design would have to frame and escape every + * console.log, and any user code writing a frame-shaped line could forge a + * protocol message. + * + * Evaluation uses `vm.runInThisContext`, NOT `vm.runInContext` on a fresh + * sandbox. A fresh sandbox has no `setTimeout`, no `fetch`, no `TextEncoder` — + * measured, not assumed — so it would need a hand-curated global list that goes + * stale with every Node release and fails as `ReferenceError: fetch is not + * defined`. Running in the child's real context gives the whole standard + * library for free, and top-level `let`/`const` still persist between calls + * because a Script's top-level lexical declarations live in the CONTEXT's + * global lexical scope, which outlives the individual script. `require` is the + * one thing missing (it is module-scoped, not global), so it is installed + * explicitly. + */ + +/** + * Sentinel the parent matches to detect a top-level-await snippet, so the retry + * fires on V8's actual message rather than on a guess about the user's code. + */ +export const TOP_LEVEL_AWAIT_MARKER = 'await is only valid in'; + +/** Sentinel for a `let`/`const` re-declared against a still-live session. */ +export const ALREADY_DECLARED_MARKER = 'has already been declared'; + +/** V8's message when `vm`'s synchronous watchdog fires. */ +export const SCRIPT_TIMEOUT_MARKER = 'Script execution timed out'; + +/** + * Child program source. Kept dependency-free and small enough to travel as a + * command-line argument on every platform (Windows caps a command line at + * 32767 characters; this is well under 8 KB even base64-encoded). + */ +export const REPL_RUNNER_SOURCE = String.raw` +'use strict'; +const vm = require('vm'); +const util = require('util'); +const path = require('path'); +const { createRequire } = require('module'); + +// Capture everything the runner needs BEFORE user code runs. User code shares +// this global context and may reassign process, console, or require; binding +// early means a script that clobbers a global breaks only itself. +const send = process.send.bind(process); +const stderrWrite = process.stderr.write.bind(process.stderr); + +// require() is module-scoped, so a script evaluated in the global context does +// not see it. Bind one to the session cwd so require('./local') resolves the +// way it would in a file sitting there. +globalThis.require = createRequire(path.join(process.cwd(), '[wmux-repl]')); +globalThis.__wmuxRepl = { version: 1 }; + +// The parent's IPC channel closing is the ONLY reaping signal that survives the +// parent being SIGKILLed (a crashed broker cannot run cleanup). Without this a +// broker crash would orphan every REPL child on the machine, forever. +process.on('disconnect', () => process.exit(0)); + +// A stray timer from an earlier eval throwing must not take the session down +// with it — that would silently destroy state the caller believes it still has. +// Report to stderr instead, where it surfaces in the next run's output. +process.on('uncaughtException', (err) => { + try { stderrWrite('[repl] uncaught exception in background code: ' + (err && err.stack || err) + '\n'); } catch (_) { /* stderr gone */ } +}); +process.on('unhandledRejection', (reason) => { + try { stderrWrite('[repl] unhandled rejection in background code: ' + (reason && reason.stack || reason) + '\n'); } catch (_) { /* stderr gone */ } +}); + +function describe(value) { + return util.inspect(value, { + depth: 3, + maxArrayLength: 200, + maxStringLength: 8192, + breakLength: 100, + getters: false, + }); +} + +// Wrap a top-level-await snippet so it can run as a Script. +// +// The wrapper is an async IIFE, and an IIFE with a BLOCK body has no completion +// value: in "await f(); result" the wrapper evaluates result and throws it +// away, and that is exactly the value the caller asked for. Node's own REPL +// solves this with a full parse and rewrite; we have no parser and want no +// dependency, so we use the shape agents actually write: the last line is the +// expression they want back. Turn that line into a return, then COMPILE the +// result and fall back to the plain wrapper if the rewrite did not produce +// valid syntax. The +// compile check is what makes the heuristic safe — a last line that was really +// the tail of a multi-line expression, or a declaration, simply does not +// survive it, and the caller gets the un-rewritten behavior instead of a +// mangled program. +function wrapAsync(code) { + const trimmed = code.replace(/\s+$/, '').replace(/;+$/, ''); + // Candidate split points, scanned from the end: statement separators are + // newlines and semicolons, and a one-liner like "const x = await f(); x" only + // has the latter. Bounded so a long script cannot turn this into a parse + // storm; 40 trailing statements is far past anything a REPL call contains. + const splits = []; + for (let i = trimmed.length - 1; i >= 0 && splits.length < 40; i--) { + const ch = trimmed[i]; + if (ch === '\n' || ch === ';') splits.push(i); + } + for (let s = 0; s < splits.length; s++) { + const at = splits[s]; + const tail = trimmed.slice(at + 1).trim(); + if (tail === '') continue; + const head = trimmed.slice(0, at + 1); + const rewritten = '(async () => {\n' + head + '\nreturn (' + tail + ');\n})()'; + try { + new vm.Script(rewritten); + return rewritten; + } catch (_) { /* that split did not yield valid syntax; try an earlier one */ } + } + // No split needed or none worked: the whole snippet may itself be one + // expression, which the wrapper can return directly. + const whole = '(async () => {\nreturn (' + trimmed + ');\n})()'; + try { + new vm.Script(whole); + return whole; + } catch (_) { /* statements, not an expression */ } + return '(async () => {\n' + code + '\n})()'; +} + +function fail(id, error) { + send({ id: id, ok: false, error: String(error && error.stack || error) }); +} + +process.on('message', (msg) => { + if (!msg || typeof msg.code !== 'string') return; + const id = msg.id; + // The vm timeout is a watchdog on SYNCHRONOUS execution only. It is the layer + // that stops a runaway loop WITHOUT losing session state; the parent's hard + // deadline is the separate layer that handles a promise that never settles. + const options = { timeout: msg.timeoutMs, displayErrors: true, filename: 'wmux-repl' }; + let value; + try { + value = vm.runInThisContext(msg.code, options); + } catch (err) { + const text = String(err && err.message || err); + if (err instanceof SyntaxError && text.indexOf('await is only valid in') !== -1) { + // Top-level await: V8 will not parse it as a Script, so re-run wrapped. + // Declarations inside the wrapper are function-scoped and do NOT persist; + // the tool description tells callers to assign to a global instead. + try { + value = vm.runInThisContext(wrapAsync(msg.code), options); + } catch (retryErr) { + fail(id, retryErr); + return; + } + } else { + fail(id, err); + return; + } + } + // A returned promise is awaited so the caller sees the resolved value rather + // than "Promise { }" — the whole point of a top-level-await REPL. + if (value && typeof value.then === 'function') { + Promise.resolve(value).then( + (resolved) => send({ id: id, ok: true, result: describe(resolved) }), + (err) => fail(id, err), + ); + return; + } + send({ id: id, ok: true, result: describe(value) }); +}); + +send({ ready: true }); +`; + +/** + * The `node -e` argument that boots the runner. + * + * Base64 rather than the raw source because the source travels as a single + * command-line argument through three different escaping regimes (POSIX exec, + * Windows CreateProcess, and Node's own argument quoting). Base64 is pure + * ASCII with no quotes, newlines, or backslashes, so none of those regimes has + * anything to mangle. + */ +export function buildRunnerBootstrap(): string { + const encoded = Buffer.from(REPL_RUNNER_SOURCE, 'utf8').toString('base64'); + return `eval(Buffer.from("${encoded}","base64").toString("utf8"))`; +} diff --git a/src/mcp/repl/tools.ts b/src/mcp/repl/tools.ts new file mode 100644 index 000000000..82639ac04 --- /dev/null +++ b/src/mcp/repl/tools.ts @@ -0,0 +1,238 @@ +/** + * The `repl_*` MCP tools. + * + * Registered through the typed catalog (`defineWmuxTool`/`registerWmuxTools`) + * like the browser wait domain, so the specs stay frozen and profile selection + * stays immutable at launch. + * + * Profile: `full` only. The commander surface is deliberately the brain's + * narrow hands (no browser, no pane teardown); a general-purpose runtime does + * not belong there. + */ +import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; +import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; +import { z } from 'zod'; +import { + defineWmuxTool, + registerWmuxTools, + type RegisterWmuxToolsOptions, + type WmuxToolSpec, +} from '../toolCatalog'; +import { + DEFAULT_SESSION_NAME, + IDLE_TIMEOUT_MS, + MAX_SESSIONS_PER_CONNECTION, + getReplRegistry, + isValidSessionName, +} from './replRegistry'; +import type { ReplEvalOutcome } from './ReplSession'; + +export const DEFAULT_TIMEOUT_MS = 30_000; +export const MIN_TIMEOUT_MS = 100; +export const MAX_TIMEOUT_MS = 300_000; + +/** Clamp rather than reject: a caller asking for 10 minutes wants the ceiling. */ +export function clampTimeout(requested: number | undefined): number { + if (requested === undefined || !Number.isFinite(requested)) return DEFAULT_TIMEOUT_MS; + return Math.min(MAX_TIMEOUT_MS, Math.max(MIN_TIMEOUT_MS, Math.floor(requested))); +} + +function text(body: string, isError = false): CallToolResult { + return { content: [{ type: 'text' as const, text: body }], isError: isError || undefined }; +} + +function formatDuration(ms: number): string { + const seconds = Math.round(ms / 1000); + if (seconds < 60) return `${seconds}s`; + return `${Math.floor(seconds / 60)}m${seconds % 60}s`; +} + +/** Render one eval into the block layout the agent reads. */ +export function formatOutcome( + sessionName: string, + outcome: ReplEvalOutcome, + notes: readonly string[], +): string { + const lines: string[] = []; + lines.push( + `session ${sessionName} · ${outcome.ok ? 'ok' : 'error'} · ${outcome.elapsedMs}ms`, + ); + for (const note of notes) lines.push(`note: ${note}`); + if (outcome.fatal) lines.push(`note: ${outcome.fatal}`); + if (outcome.timedOut) { + lines.push('note: the code was stopped by the timeout. Session state survived.'); + } + if (outcome.remedy) lines.push(`note: ${outcome.remedy}`); + + if (outcome.stdout.text) { + lines.push('', '--- stdout ---', outcome.stdout.text.replace(/\n$/, '')); + if (outcome.stdout.truncated) { + lines.push(`(stdout truncated: ${outcome.stdout.totalBytes} bytes total)`); + } + } + if (outcome.stderr.text) { + lines.push('', '--- stderr ---', outcome.stderr.text.replace(/\n$/, '')); + if (outcome.stderr.truncated) { + lines.push(`(stderr truncated: ${outcome.stderr.totalBytes} bytes total)`); + } + } + if (outcome.ok && outcome.result) { + lines.push('', '--- result ---', outcome.result.text); + if (outcome.result.truncated) { + lines.push(`(result truncated: ${outcome.result.totalBytes} bytes total)`); + } + } + if (!outcome.ok && outcome.error) { + lines.push('', '--- error ---', outcome.error); + } + return lines.join('\n'); +} + +// Descriptions are deliberately tight: every byte here rides in tools/list on +// every session, and the protocol probe enforces a total budget for that view. +// Each sentence that survived earns its place — the persistence contract, the +// two await caveats, and the two facts an agent would otherwise get wrong (no +// sandbox, no lifetime past the connection). +const REPL_RUN_DESCRIPTION = + 'Run JavaScript in a persistent Node runtime and get the return value back. ' + + 'State survives between calls: variables (including top-level let/const), required ' + + 'modules, and open handles are still there next call. Top-level await works, but ' + + 'declarations inside an awaiting snippet do not persist — assign to a global ' + + '(x = await f()) to keep one. A name already bound by let/const cannot be re-declared; ' + + 'assign without a keyword. Full fs/net/require access, NO sandbox. Lives only as long ' + + 'as your MCP connection: no wmux restart, no sharing with other panes or workspaces.'; + +const REPL_RESET_DESCRIPTION = + 'Throw away a REPL session and its state; the next repl_run starts a fresh runtime. ' + + 'Use when a session is wedged or holds a stale module.'; + +const REPL_SESSIONS_DESCRIPTION = + 'List this connection\'s REPL sessions: cwd, pid, age, and whether one is running code.'; + +export function createReplToolCatalog(): readonly WmuxToolSpec[] { + const replRun = defineWmuxTool({ + name: 'repl_run', + description: REPL_RUN_DESCRIPTION, + inputSchema: { + code: z.string().describe('JavaScript to evaluate. The last expression is the return value.'), + session: z + .string() + .optional() + .describe(`Session name; defaults to "${DEFAULT_SESSION_NAME}". Letters, digits, . _ - only.`), + timeout: z + .number() + .optional() + .describe( + `Milliseconds before the run is stopped; default ${DEFAULT_TIMEOUT_MS}, max ${MAX_TIMEOUT_MS}.`, + ), + cwd: z + .string() + .optional() + .describe( + "Working directory, applied only when the session is created. Defaults to the MCP server's " + + "own cwd, which is not necessarily your pane's — pass it explicitly to be sure.", + ), + }, + profiles: ['full'], + invoke: async ({ code, session, timeout, cwd }) => { + const name = session ?? DEFAULT_SESSION_NAME; + if (!isValidSessionName(name)) { + return text(`Invalid session name "${name}". Use 1-64 of: letters, digits, dot, underscore, hyphen.`, true); + } + const registry = getReplRegistry(); + const notes: string[] = []; + let acquired; + try { + acquired = registry.acquire(name, cwd ?? process.cwd()); + } catch (error) { + return text(String(error instanceof Error ? error.message : error), true); + } + if (acquired.created) { + if (acquired.previousDeath) { + notes.push(`the previous "${name}" runtime is gone (${acquired.previousDeath}); this is a fresh one with no state`); + } + notes.push(`started a new runtime in ${acquired.session.cwd}`); + if (acquired.session.withheldCredentials.length > 0) { + notes.push( + `credential env vars are withheld from the REPL: ${acquired.session.withheldCredentials.join(', ')}`, + ); + } + } else if (cwd && cwd !== acquired.session.cwd) { + notes.push( + `cwd was ignored — session "${name}" is already running in ${acquired.session.cwd}. ` + + 'Call repl_reset first, or use a different session name.', + ); + } + + try { + const outcome = await acquired.session.run(code, clampTimeout(timeout)); + return text(formatOutcome(name, outcome, notes), !outcome.ok); + } catch (error) { + return text( + `session ${name}: ${String(error instanceof Error ? error.message : error)}`, + true, + ); + } + }, + }); + + const replReset = defineWmuxTool({ + name: 'repl_reset', + description: REPL_RESET_DESCRIPTION, + inputSchema: { + session: z + .string() + .optional() + .describe(`Session name; defaults to "${DEFAULT_SESSION_NAME}".`), + }, + profiles: ['full'], + invoke: ({ session }) => { + const name = session ?? DEFAULT_SESSION_NAME; + if (!isValidSessionName(name)) { + return text(`Invalid session name "${name}".`, true); + } + const existed = getReplRegistry().reset(name); + return text( + existed + ? `Killed REPL session "${name}". The next repl_run starts a fresh runtime.` + : `No REPL session "${name}" was running. The next repl_run starts a fresh runtime.`, + ); + }, + }); + + const replSessions = defineWmuxTool({ + name: 'repl_sessions', + description: REPL_SESSIONS_DESCRIPTION, + inputSchema: {}, + profiles: ['full'], + invoke: () => { + const sessions = getReplRegistry().list(); + const header = + `REPL sessions are scoped to this MCP connection, capped at ${MAX_SESSIONS_PER_CONNECTION}, ` + + `and reaped after ${Math.round(IDLE_TIMEOUT_MS / 60000)} minutes idle.`; + if (sessions.length === 0) { + return text(`No REPL sessions running.\n${header}`); + } + const now = Date.now(); + const rows = sessions.map((s) => + [ + s.name, + `pid ${String(s.pid ?? '?')}`, + s.busy ? 'running code' : 'idle', + `${s.evals} run(s)`, + `up ${formatDuration(now - s.createdAt)}`, + `idle ${formatDuration(now - s.lastUsed)}`, + s.cwd, + ].join(' · '), + ); + return text([...rows, '', header].join('\n')); + }, + }); + + return Object.freeze([replRun, replReset, replSessions]); +} + +/** Register the REPL catalog through the wire-neutral current-SDK adapter. */ +export function registerReplTools(server: McpServer, options: RegisterWmuxToolsOptions): void { + registerWmuxTools(server, createReplToolCatalog(), options); +} diff --git a/src/mcp/repl/truncate.ts b/src/mcp/repl/truncate.ts new file mode 100644 index 000000000..105b95a15 --- /dev/null +++ b/src/mcp/repl/truncate.ts @@ -0,0 +1,179 @@ +/** + * Bounded output capture for the agent REPL. + * + * A REPL runs code the caller wrote seconds ago, so `while (true) console.log(x)` + * is a routine mistake rather than an exotic one. The MCP server that holds these + * buffers is the shared broker hosting EVERY agent's connection, so an unbounded + * accumulator is a cross-agent memory fault, not a local one. + * + * OutputBuffer therefore retains a bounded head and a bounded tail and forgets + * the middle, while still counting every byte that ever arrived: + * + * ┌──────── head (75% of cap) ────────┬─ elided ─┬─ tail (25%) ─┐ + * │ the first output, where the run │ counted │ the last │ + * │ announces what it is doing │ only │ output, where│ + * │ │ │ it failed │ + * └────────────────────────────────────┴──────────┴──────────────┘ + * + * Both ends matter: the head says what the script started doing, the tail + * carries the error it died on. Keeping only the head would hide every failure + * that happens after a chatty loop. + */ + +/** Result of rendering a bounded buffer or string back to text. */ +export interface TruncatedText { + /** The retained text, with an elision marker in place of the dropped middle. */ + readonly text: string; + /** True when anything was dropped. */ + readonly truncated: boolean; + /** Every byte that ever arrived, including the dropped ones. */ + readonly totalBytes: number; + /** Bytes dropped from the middle. */ + readonly elidedBytes: number; +} + +/** + * Walk back off a UTF-8 continuation byte so a cut never lands mid-codepoint. + * Without this a truncated buffer renders a U+FFFD at the seam, which reads as + * corruption in the tool output rather than as a deliberate cut. + */ +function backOffToCodepointBoundary(buf: Buffer, index: number): number { + let i = Math.min(index, buf.length); + // At most 3 continuation bytes can precede a lead byte in valid UTF-8. + for (let steps = 0; steps < 4 && i > 0; steps++) { + if ((buf[i] & 0xc0) !== 0x80) break; + i--; + } + return i; +} + +/** + * Walk FORWARD off a UTF-8 continuation byte. The tail of a truncated buffer + * starts at an arbitrary offset, so its first bytes may be the back half of a + * codepoint whose lead byte was dropped; those bytes are unrenderable and get + * skipped rather than turned into U+FFFD. + */ +function skipToCodepointBoundary(buf: Buffer, index: number): number { + let i = Math.max(0, index); + for (let steps = 0; steps < 4 && i < buf.length; steps++) { + if ((buf[i] & 0xc0) !== 0x80) break; + i++; + } + return i; +} + +function elisionMarker(bytes: number): string { + return `\n… ${bytes} bytes elided …\n`; +} + +/** + * Truncate an already-complete string to `capBytes`, keeping head and tail. + * Used for the inspected return value, which arrives in one piece. + */ +export function truncateText(input: string, capBytes: number): TruncatedText { + const buf = Buffer.from(input, 'utf8'); + if (buf.length <= capBytes) { + return { text: input, truncated: false, totalBytes: buf.length, elidedBytes: 0 }; + } + const headCap = Math.floor(capBytes * 0.75); + const tailCap = capBytes - headCap; + const headEnd = backOffToCodepointBoundary(buf, headCap); + const tailStart = skipToCodepointBoundary(buf, buf.length - tailCap); + const elidedBytes = tailStart - headEnd; + return { + text: + buf.subarray(0, headEnd).toString('utf8') + + elisionMarker(elidedBytes) + + buf.subarray(tailStart).toString('utf8'), + truncated: true, + totalBytes: buf.length, + elidedBytes, + }; +} + +/** + * Streaming accumulator with a hard retention cap. Bytes past the cap are + * counted and dropped, never buffered, so a runaway logger cannot grow the + * broker's heap no matter how long it runs. + */ +export class OutputBuffer { + private readonly headCap: number; + private readonly tailCap: number; + private readonly head: Buffer[] = []; + private headBytes = 0; + private readonly tail: Buffer[] = []; + private tailBytes = 0; + private total = 0; + + constructor(capBytes: number) { + this.headCap = Math.floor(capBytes * 0.75); + this.tailCap = Math.max(1, capBytes - this.headCap); + } + + append(chunk: Buffer): void { + this.total += chunk.length; + let rest = chunk; + if (this.headBytes < this.headCap) { + const take = Math.min(this.headCap - this.headBytes, rest.length); + this.head.push(rest.subarray(0, take)); + this.headBytes += take; + rest = rest.subarray(take); + } + if (rest.length === 0) return; + this.tail.push(rest); + this.tailBytes += rest.length; + // Drop from the FRONT of the tail so the ring always holds the most recent + // bytes. A single oversized chunk is sliced rather than kept whole. + while (this.tailBytes > this.tailCap) { + const front = this.tail[0]; + const excess = this.tailBytes - this.tailCap; + if (front.length <= excess) { + this.tail.shift(); + this.tailBytes -= front.length; + } else { + this.tail[0] = front.subarray(excess); + this.tailBytes -= excess; + } + } + } + + /** Bytes that ever arrived, including dropped ones. */ + get totalBytes(): number { + return this.total; + } + + render(): TruncatedText { + const headBuf = Buffer.concat(this.head, this.headBytes); + if (this.tailBytes === 0) { + return { + text: headBuf.toString('utf8'), + truncated: false, + totalBytes: this.total, + elidedBytes: 0, + }; + } + const tailBuf = Buffer.concat(this.tail, this.tailBytes); + const elidedBytes = this.total - this.headBytes - this.tailBytes; + if (elidedBytes === 0) { + // Everything fit; the split between head and tail is an implementation + // detail the caller must not see as a truncation. + return { + text: Buffer.concat([headBuf, tailBuf]).toString('utf8'), + truncated: false, + totalBytes: this.total, + elidedBytes: 0, + }; + } + const headEnd = backOffToCodepointBoundary(headBuf, headBuf.length); + const tailStart = skipToCodepointBoundary(tailBuf, 0); + return { + text: + headBuf.subarray(0, headEnd).toString('utf8') + + elisionMarker(elidedBytes + (headBuf.length - headEnd) + tailStart) + + tailBuf.subarray(tailStart).toString('utf8'), + truncated: true, + totalBytes: this.total, + elidedBytes, + }; + } +} From 1d666a6a22f152e6e11258e49532ec495c8366bc Mon Sep 17 00:00:00 2001 From: wong2kim Date: Mon, 31 Aug 2026 00:18:48 +0900 Subject: [PATCH 2/5] fix(mcp): cut runner frames off REPL error stacks Every REPL error ended in six frames of vm and IPC plumbing that describe how the runner is built and nothing about what the caller wrote. They were identical on every failure and, on an agent surface, paid for in context every time. Keep the message and the frames above the first internal one. A vm timeout, whose stack is entirely internal, now reports just its message, which was always the whole story. --- .../__tests__/replSession.runtime.test.ts | 6 ++++++ src/mcp/repl/replRunnerSource.ts | 19 ++++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/mcp/repl/__tests__/replSession.runtime.test.ts b/src/mcp/repl/__tests__/replSession.runtime.test.ts index 147ff4b48..33a281101 100644 --- a/src/mcp/repl/__tests__/replSession.runtime.test.ts +++ b/src/mcp/repl/__tests__/replSession.runtime.test.ts @@ -110,6 +110,10 @@ describe('ReplSession state persistence', () => { expect(failed.ok).toBe(false); expect(failed.error).toContain('boom'); expect(failed.fatal).toBeUndefined(); + // The runner's own vm/IPC frames are cut: they are identical on every error + // and would cost the agent context on every failure. + expect(failed.error).not.toContain('runInThisContext'); + expect(failed.error).not.toContain('node:internal/child_process'); const after = await session.run('survivor', 5000); expect(after.ok).toBe(true); @@ -134,6 +138,8 @@ describe('ReplSession timeouts', () => { const outcome = await session.run('while (true) {}', 300); expect(outcome.ok).toBe(false); expect(outcome.timedOut).toBe(true); + // Entirely-internal stack trimmed down to the message that is the story. + expect(outcome.error).toBe('Error: Script execution timed out after 300ms'); // The vm watchdog stops the script; the process is untouched, so state lives. expect(outcome.fatal).toBeUndefined(); expect(session.dead).toBe(false); diff --git a/src/mcp/repl/replRunnerSource.ts b/src/mcp/repl/replRunnerSource.ts index 764ec4860..9822d37ad 100644 --- a/src/mcp/repl/replRunnerSource.ts +++ b/src/mcp/repl/replRunnerSource.ts @@ -140,8 +140,25 @@ function wrapAsync(code) { return '(async () => {\n' + code + '\n})()'; } +// Cut the runner's own frames off a stack. +// +// Every error otherwise ends in six frames of vm/IPC plumbing that describe how +// this file is built and nothing about what the caller wrote. They are pure +// noise, they are identical on every error, and in an agent surface they are +// paid for in context on every failure. Keep the message and the frames above +// the first internal one; when the error is entirely internal (a vm timeout) +// that leaves just the message, which is the whole story anyway. +const INTERNAL_FRAME = /^\s+at (Script\.runInThisContext|Object\.runInThisContext|process\.eval|process\.emit|emit \(node:internal|process\.processTicksAndRejections)/; + +function trimStack(error) { + const raw = String(error && error.stack || error); + const lines = raw.split('\n'); + const cut = lines.findIndex((line) => INTERNAL_FRAME.test(line)); + return cut === -1 ? raw : lines.slice(0, cut).join('\n').replace(/\s+$/, ''); +} + function fail(id, error) { - send({ id: id, ok: false, error: String(error && error.stack || error) }); + send({ id: id, ok: false, error: trimStack(error) }); } process.on('message', (msg) => { From 1c9b88b292561c51a65c69955a3d870de50f586b Mon Sep 17 00:00:00 2001 From: wong2kim Date: Mon, 31 Aug 2026 00:38:08 +0900 Subject: [PATCH 3/5] fix(mcp): harden the REPL surface against the review panel's findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A two-model panel (Claude, GLM) reviewed the diff. Codex was unavailable. Five findings had cross-model agreement; the rest were verified individually. Result integrity: - Match the in-flight eval id before settling a reply. User code shares the child's globals and therefore its `process.send`, so a script could post its own completion, take the answer, AND clear the hard-deadline timer while it spun forever in the shared broker. A late reply from an abandoned eval is dropped by the same check. - Classify timeouts and re-declarations in the runner and ship the verdict as a field, instead of matching substrings against the error text in the parent. That text is whatever the caller's code threw, so a script could make the tool announce a watchdog stop that never happened. The watchdog is identified by V8's exact message plus the absence of any frame inside the evaluated code. Silently-different programs: - The top-level-await rewrite turned the last statement into a return, guarded only by "does it still compile". Compiling is not meaning the same thing: no semicolon is inserted before a newline followed by `[`, `(` or an operator, so `const a = await f()\n[0]` is ONE expression that split into two compilable halves and answered `[ 0 ]`. A trailing `function`/`class` declaration likewise became an expression, changing the return value and trapping the definition in the wrapper. Both shapes are now skipped. Host bounds and isolation: - Add a process-wide ceiling on live children. The per-connection cap of four is not a host bound in a broker that serves N agents at once. - Fail closed when a REPL call arrives with no connection scope in broker mode. The old silent fallback to a process-global registry would have handed one agent another agent's live session. Disposal stays tolerant, since throwing on the teardown path would abandon the rest of a connection's cleanup. - Cap the inspected value inside the child. inspect bounds strings and arrays but not an object's key count, so a large `Object.fromEntries` crossed IPC in full before the parent's cap saw it, landing in the one heap that must not hold it. Also answer, rather than hang, when inspection itself throws. Output accounting: - Report output that arrived before an eval started as `background`. Blending it in let an agent read an earlier run's timer output as its own code's doing. - Reset the drain clock per eval; carried over from the previous eval it was always already stale, making the quiet test vacuously true. - Copy retained bytes out of the caller's chunks. A retained subarray pins its whole pooled block, so a chatty writer held tens of megabytes behind a 64 KB cap — defeating the guarantee the buffer exists to make. - Fix the head-seam codepoint trim, which read one byte past the end and so never fired, rendering U+FFFD at exactly the seam it was meant to protect. - Raise the hard-kill grace to 2s: if a loaded broker delays the watchdog's reply, a stop that should have kept the session escalates into a kill that destroys it. - Report a session's real state, so one still starting is no longer listed idle. --- src/mcp/broker.ts | 5 +- src/mcp/repl/ReplSession.ts | 59 +++++++++-- .../__tests__/replRegistry.runtime.test.ts | 48 +++++++++ .../__tests__/replSession.runtime.test.ts | 87 ++++++++++++++- src/mcp/repl/__tests__/truncate.test.ts | 20 ++++ src/mcp/repl/replRegistry.ts | 73 ++++++++++++- src/mcp/repl/replRunnerSource.ts | 100 +++++++++++++----- src/mcp/repl/tools.ts | 11 +- src/mcp/repl/truncate.ts | 39 ++++++- 9 files changed, 401 insertions(+), 41 deletions(-) diff --git a/src/mcp/broker.ts b/src/mcp/broker.ts index cee8d8eaf..56efec995 100644 --- a/src/mcp/broker.ts +++ b/src/mcp/broker.ts @@ -33,7 +33,7 @@ import { type ConnectionScope, } from './connectionScope'; import type { PlaywrightEngine } from './playwright/PlaywrightEngine'; -import { disposeReplRegistry } from './repl/replRegistry'; +import { disposeReplRegistry, setReplBrokerMode } from './repl/replRegistry'; interface ShimHandshake { wmuxShim: number; @@ -159,6 +159,9 @@ async function hostConnection(socket: net.Socket, handshake: ShimHandshake): Pro } function main(): void { + // This process hosts many agents at once, so a REPL call that arrives without + // a connection scope must fail rather than fall back to a shared registry. + setReplBrokerMode(); const expectedToken = readAuthToken(); if (!expectedToken) { console.error('[wmux-mcp-broker] auth token not found; refusing to serve. Is wmux running?'); diff --git a/src/mcp/repl/ReplSession.ts b/src/mcp/repl/ReplSession.ts index 1b5efb817..3a75828ba 100644 --- a/src/mcp/repl/ReplSession.ts +++ b/src/mcp/repl/ReplSession.ts @@ -29,7 +29,7 @@ import { spawn, type ChildProcess } from 'child_process'; import * as fs from 'fs'; import * as path from 'path'; import { buildGatedAutomationEnv, withheldCredentialNames } from '../../shared/envFilter'; -import { buildRunnerBootstrap, SCRIPT_TIMEOUT_MARKER, ALREADY_DECLARED_MARKER } from './replRunnerSource'; +import { buildRunnerBootstrap } from './replRunnerSource'; import { OutputBuffer, truncateText, type TruncatedText } from './truncate'; /** Per-eval retention for each of stdout and stderr. */ @@ -37,11 +37,15 @@ export const OUTPUT_CAP_BYTES = 64 * 1024; /** Retention for the inspected return value. */ export const RESULT_CAP_BYTES = 16 * 1024; /** - * How long past the caller's timeout the child gets before SIGKILL. The vm - * watchdog inside the child stops synchronous runaways on its own; this grace - * only has to cover the message hop back, so it is short on purpose. + * How long past the caller's timeout the child gets before SIGKILL. + * + * The vm watchdog inside the child stops synchronous runaways on its own, so + * this grace only has to cover the watchdog's message hop back. It is generous + * anyway: if a loaded broker delays that hop past the grace, a stop that should + * have KEPT the session escalates into a kill that destroys it, and losing a + * session's state is far worse than waiting an extra second for a genuine hang. */ -export const HARD_KILL_GRACE_MS = 500; +export const HARD_KILL_GRACE_MS = 2_000; /** Quiet window the pipes must show before an eval's output is considered complete. */ const DRAIN_QUIET_MS = 20; /** Ceiling on draining, so a still-chattering background timer cannot stall the tool. */ @@ -69,6 +73,12 @@ export interface ReplEvalOutcome { readonly timedOut?: boolean; /** Set when a `let`/`const` collided with a still-live binding. */ readonly remedy?: string; + /** + * Output that arrived BEFORE this eval started — a timer or handle left + * running by an earlier one. Reported separately so the agent never reads + * another eval's output as its own code's doing. + */ + readonly background?: string; } interface RunnerMessage { @@ -77,6 +87,8 @@ interface RunnerMessage { readonly ok?: boolean; readonly result?: string; readonly error?: string; + /** Runner's own classification. Never re-derived from `error` text. */ + readonly kind?: 'timeout' | 'redeclare'; } /** Node binary plus whether Electron needs telling to behave as one. */ @@ -138,6 +150,8 @@ export class ReplSession { private out = new OutputBuffer(OUTPUT_CAP_BYTES); private err = new OutputBuffer(OUTPUT_CAP_BYTES); private pending: ((message: RunnerMessage) => void) | null = null; + /** Id of the eval in flight; only a reply carrying it may settle. */ + private pendingId: number | null = null; private readonly ready: Promise; private idleTimer: NodeJS.Timeout | null = null; private readonly idleMs: number; @@ -218,7 +232,14 @@ export class ReplSession { child.on('message', (raw: unknown) => { const message = raw as RunnerMessage; - if (message?.id === undefined) return; + // Match the id of the eval actually in flight. User code shares this + // process's global scope and therefore its `process.send`, so without the + // check a script could post its own completion, collect the answer early, + // AND keep the hard-deadline timer cleared while it spins forever in the + // shared broker. A late reply from an abandoned earlier eval is dropped by + // the same test. + if (message?.id === undefined || message.id !== this.pendingId) return; + this.pendingId = null; const settle = this.pending; this.pending = null; settle?.(message); @@ -275,7 +296,8 @@ export class ReplSession { } const settle = this.pending; this.pending = null; - settle?.({ id: -1, ok: false, error: reason }); + this.pendingId = null; + settle?.({ ok: false, error: reason }); } /** Kill the child and mark the session dead. Idempotent. */ @@ -331,8 +353,19 @@ export class ReplSession { this.lastUsedAt = Date.now(); const id = this.nextEvalId++; + // Anything buffered before this call came from a timer or handle left + // running by an EARLIER eval. Take it now so it is reported as background + // rather than blended into this eval's output, where the agent would read + // it as its own code's doing. + const background = this.takeOutput(); + // Reset the drain clock so quiet is measured from THIS eval. Left at the + // previous eval's value it is always already stale, which makes the quiet + // test vacuously true and defeats the drain entirely. + this.lastChunkAt = Date.now(); + const message = await new Promise((resolve) => { this.pending = resolve; + this.pendingId = id; // Layer two of the timeout. The child's vm watchdog cannot see a promise // that never settles or a blocked native call, so the only reliable stop // is killing the process — which is why this costs the session's state @@ -360,6 +393,8 @@ export class ReplSession { await this.drain(); const { stdout, stderr } = this.takeOutput(); const elapsedMs = Date.now() - started; + const backgroundText = + [background.stdout.text, background.stderr.text].filter(Boolean).join('') || undefined; // Read through the getter: the assignment above narrows `this.state` to // 'busy' for the checker, but the eval could have killed the session while @@ -371,6 +406,7 @@ export class ReplSession { fatal: this.deathReason ?? 'the REPL session ended', stdout, stderr, + background: backgroundText, elapsedMs, }; } @@ -385,6 +421,7 @@ export class ReplSession { result: truncateText(message.result ?? 'undefined', RESULT_CAP_BYTES), stdout, stderr, + background: backgroundText, elapsedMs, }; } @@ -395,9 +432,13 @@ export class ReplSession { error, stdout, stderr, + background: backgroundText, elapsedMs, - timedOut: error.includes(SCRIPT_TIMEOUT_MARKER) || undefined, - remedy: error.includes(ALREADY_DECLARED_MARKER) + // Trust the runner's classification, never a substring of `error`: that + // text is whatever the caller's own code threw, so sniffing it lets a + // script make the tool announce a watchdog stop that never happened. + timedOut: message.kind === 'timeout' || undefined, + remedy: message.kind === 'redeclare' ? 'That name is already bound in this session. `let`/`const` cannot be re-declared ' + 'against a live binding — assign without a keyword (`x = ...`) to update it, or call ' + 'repl_reset to start from a clean runtime.' diff --git a/src/mcp/repl/__tests__/replRegistry.runtime.test.ts b/src/mcp/repl/__tests__/replRegistry.runtime.test.ts index 5cc0f70c1..28def7dff 100644 --- a/src/mcp/repl/__tests__/replRegistry.runtime.test.ts +++ b/src/mcp/repl/__tests__/replRegistry.runtime.test.ts @@ -7,9 +7,11 @@ import { afterEach, describe, expect, it } from 'vitest'; import { runInConnectionScope, createConnectionScope } from '../../connectionScope'; import { MAX_SESSIONS_PER_CONNECTION, + MAX_SESSIONS_PER_PROCESS, ReplRegistry, disposeReplRegistry, getReplRegistry, + setReplBrokerMode, } from '../replRegistry'; const registries: ReplRegistry[] = []; @@ -92,6 +94,34 @@ describe('ReplRegistry', () => { }, 20_000); }); +describe('host-wide bound', () => { + it('refuses past the process ceiling even across separate connections', () => { + // The per-connection cap is not a host bound: the broker hosts N agents, so + // N x 4 children would land on one machine without this. + const owned: ReplRegistry[] = []; + let created = 0; + try { + for (let r = 0; r < 8; r++) { + const registry = makeRegistry(); + owned.push(registry); + for (let i = 0; i < MAX_SESSIONS_PER_CONNECTION; i++) { + try { + registry.acquire(`s${i}`, os.tmpdir()); + created++; + } catch (error) { + expect(String(error)).toMatch(/host-wide limit|already holds/); + expect(created).toBe(MAX_SESSIONS_PER_PROCESS); + return; + } + } + } + throw new Error(`expected the process ceiling to bite; created ${created}`); + } finally { + for (const registry of owned) registry.disposeAll(); + } + }, 60_000); +}); + describe('connection scoping', () => { it('gives each connection its own sessions and disposes only its own', async () => { const scopeA = createConnectionScope(); @@ -118,4 +148,22 @@ describe('connection scoping', () => { runIn(scopeB, () => disposeReplRegistry()); expect(b.session.dead).toBe(true); }, 20_000); + + it('refuses to serve a scopeless call once broker mode is declared', () => { + // Run LAST: broker mode is process-wide and one-way, matching the real + // broker, where it is set at startup and never cleared. + setReplBrokerMode(); + // Outside any runInConnectionScope there is no way to tell whose session + // this would be. Falling back to a shared registry would hand one agent + // another's live runtime, so this must fail loudly instead. + expect(() => getReplRegistry()).toThrow(/cannot be attributed/); + + // Inside a scope it still works, and disposal stays tolerant so teardown + // is never abandoned half-done. + const scope = createConnectionScope(); + runInConnectionScope(scope, () => { + expect(getReplRegistry()).toBeInstanceOf(ReplRegistry); + }); + expect(() => disposeReplRegistry()).not.toThrow(); + }); }); diff --git a/src/mcp/repl/__tests__/replSession.runtime.test.ts b/src/mcp/repl/__tests__/replSession.runtime.test.ts index 33a281101..cb430855c 100644 --- a/src/mcp/repl/__tests__/replSession.runtime.test.ts +++ b/src/mcp/repl/__tests__/replSession.runtime.test.ts @@ -178,8 +178,91 @@ describe('ReplSession timeouts', () => { const after = await session.run('"still running"', 5000); expect(session.dead).toBe(false); expect(after.ok).toBe(true); - // The background failure is not swallowed — it surfaces on the next run. - expect(after.stderr.text).toContain('late boom'); + // Not swallowed, and not misattributed either: it surfaces on the next run + // as background, since it belongs to the earlier eval, not this one. + expect(after.background).toContain('late boom'); + expect(after.stderr.text).not.toContain('late boom'); + }, 20_000); +}); + +describe('ReplSession result integrity (review panel findings)', () => { + it('ignores an IPC message the user code forges, so the timeout still binds', async () => { + const session = makeSession(); + // User code shares the child's globals, so it can call process.send. If the + // parent accepted that, a script could report success, take the answer, and + // keep burning CPU in the shared broker with the hard timer cleared. + const outcome = await session.run( + 'process.send({ id: 999, ok: true, result: "\'forged\'" }); 1 + 1', + 5000, + ); + expect(outcome.ok).toBe(true); + expect(outcome.result?.text).toBe('2'); + }, 20_000); + + it('does not let a thrown message impersonate the vm watchdog', async () => { + const session = makeSession(); + const outcome = await session.run('throw new Error("Script execution timed out after 1ms")', 5000); + expect(outcome.ok).toBe(false); + // Classification comes from the runner, not from matching the error text. + expect(outcome.timedOut).toBeUndefined(); + }, 20_000); + + it('bounds a value with a huge number of keys inside the child', async () => { + const session = makeSession(); + const outcome = await session.run( + 'Object.fromEntries(Array.from({ length: 300000 }, (_, i) => ["k" + i, i]))', + 20_000, + ); + expect(outcome.ok).toBe(true); + expect(outcome.result?.text).toContain('truncated in the REPL process'); + }, 30_000); + + it('still answers for a value with hostile inspection traps', async () => { + const session = makeSession(); + const outcome = await session.run( + 'new Proxy({}, { ownKeys() { throw new Error("no introspection"); } })', + 5000, + ); + // The reply must arrive and the session must live: a swallowed inspect + // failure would present as a hang and cost the caller its whole session. + expect(outcome.ok).toBe(true); + expect(outcome.result?.text).toBeTruthy(); + expect(session.dead).toBe(false); + }, 20_000); + + it('does not split an expression that only looks like two statements', async () => { + const session = makeSession(); + // There is no ASI before "[", so this is ONE expression: + // `const a = await Promise.resolve([1,2,3])[0]`, which leaves a undefined. + // A naive newline split would make the tail `[0]` the returned value and + // answer `[ 0 ]` — a different program, silently. + const outcome = await session.run('const a = await Promise.resolve([1, 2, 3])\n[0]', 5000); + expect(outcome.ok).toBe(true); + expect(outcome.result?.text).not.toContain('[ 0 ]'); + expect(outcome.result?.text).toBe('undefined'); + }, 20_000); + + it('does not rewrite a trailing declaration into a return value', async () => { + const session = makeSession(); + const outcome = await session.run( + 'await Promise.resolve(1);\nfunction helper() { return 5; }', + 5000, + ); + expect(outcome.ok).toBe(true); + // Returning the function object would be a different program than written. + expect(outcome.result?.text).toContain('undefined'); + }, 20_000); + + it('labels output left over from an earlier run as background', async () => { + const session = makeSession(); + await session.run('setTimeout(() => console.log("from the past"), 250);', 5000); + await new Promise((r) => setTimeout(r, 500)); + const outcome = await session.run('console.log("mine"); 1', 5000); + expect(outcome.background).toContain('from the past'); + expect(outcome.stdout.text).toContain('mine'); + // The two must not be blended: that is how an agent misreads another run's + // output as its own code's doing. + expect(outcome.stdout.text).not.toContain('from the past'); }, 20_000); }); diff --git a/src/mcp/repl/__tests__/truncate.test.ts b/src/mcp/repl/__tests__/truncate.test.ts index fdef8ad9a..a20ad5232 100644 --- a/src/mcp/repl/__tests__/truncate.test.ts +++ b/src/mcp/repl/__tests__/truncate.test.ts @@ -53,6 +53,26 @@ describe('OutputBuffer', () => { expect(result.elidedBytes).toBe(9000); }); + it('never renders a replacement char at the head seam', () => { + // Three-byte codepoints guarantee the head cap lands mid-sequence. + const buf = new OutputBuffer(1000); + for (let i = 0; i < 200; i++) buf.append(Buffer.from('한글테스트')); + const result = buf.render(); + expect(result.truncated).toBe(true); + expect(result.text).not.toContain('�'); + }); + + it('does not retain the caller\'s chunk objects in the tail', () => { + // A retained subarray pins its whole pooled allocation; the ring must copy. + const buf = new OutputBuffer(400); + const chunk = Buffer.alloc(64 * 1024, 0x61); + buf.append(chunk); + buf.append(Buffer.from('tail')); + const before = buf.render().text; + chunk.fill(0x62); // mutating the original must not change what we kept + expect(buf.render().text).toBe(before); + }); + it('keeps the most recent bytes in the tail', () => { const buf = new OutputBuffer(400); buf.append(Buffer.from('S'.repeat(1000))); diff --git a/src/mcp/repl/replRegistry.ts b/src/mcp/repl/replRegistry.ts index 45679a14b..00dcae2dc 100644 --- a/src/mcp/repl/replRegistry.ts +++ b/src/mcp/repl/replRegistry.ts @@ -21,6 +21,16 @@ export const MAX_SESSIONS_PER_CONNECTION = 4; * until it is reaped. */ export const IDLE_TIMEOUT_MS = 15 * 60 * 1000; +/** + * Ceiling on live children across the WHOLE process. + * + * The per-connection cap alone is not a limit in the broker: it hosts N agents + * at once, so four sessions each multiplies out, and every child is tens of + * megabytes of resident Node. Ten agents reaching their personal cap would put + * forty runtimes on one machine long before the idle reaper noticed. This is + * the bound that is actually about the host. + */ +export const MAX_SESSIONS_PER_PROCESS = 16; /** Session names are used in messages and as map keys; keep them boring. */ const SESSION_NAME_RE = /^[A-Za-z0-9._-]{1,64}$/; @@ -31,9 +41,33 @@ export function isValidSessionName(name: string): boolean { return SESSION_NAME_RE.test(name); } +/** + * Live children across every registry in this process. Registries are + * per-connection by design and so cannot see each other; the host-wide bound + * has to live outside them. + */ +const liveRegistries = new Set(); + +function processLiveSessions(): number { + let total = 0; + for (const registry of liveRegistries) total += registry.liveCount; + return total; +} + export class ReplRegistry { private readonly sessions = new Map(); + constructor() { + liveRegistries.add(this); + } + + /** Sessions this registry currently holds whose child is still alive. */ + get liveCount(): number { + let count = 0; + for (const session of this.sessions.values()) if (!session.dead) count++; + return count; + } + /** Live sessions, oldest first. Dead ones are swept as they are noticed. */ list(): ReplSession[] { this.sweep(); @@ -67,6 +101,12 @@ export class ReplRegistry { `(${[...this.sessions.keys()].join(', ')}). Call repl_reset on one before starting another.`, ); } + if (processLiveSessions() >= MAX_SESSIONS_PER_PROCESS) { + throw new Error( + `this wmux MCP server is already running ${MAX_SESSIONS_PER_PROCESS} REPL runtimes across all ` + + 'connected agents, which is its host-wide limit. Call repl_reset on a session you are done with.', + ); + } const session = new ReplSession({ name, cwd, idleMs: IDLE_TIMEOUT_MS }); this.sessions.set(name, session); @@ -88,6 +128,7 @@ export class ReplRegistry { session.destroy('the MCP connection closed'); } this.sessions.clear(); + liveRegistries.delete(this); } /** Forget sessions whose child is already gone (idle reap, crash, kill). */ @@ -101,13 +142,40 @@ export class ReplRegistry { /** Single-child (stdio entry) fallback — no connection scope exists there. */ let processRegistry: ReplRegistry | null = null; -/** The calling connection's registry, created on first use. */ +/** + * True once this process is hosting multiple connections. Set by the broker at + * startup; the single-child stdio entry never sets it. + */ +let brokerMode = false; + +/** Declare that this process hosts more than one agent's connection. */ +export function setReplBrokerMode(): void { + brokerMode = true; +} + +/** + * The calling connection's registry, created on first use. + * + * The module-global fallback is correct for the stdio entry, where the process + * already belongs to exactly one agent. In the broker it would be a disaster: + * any call that lost its AsyncLocalStorage context would land on a registry + * SHARED with every other connection, handing one agent another agent's live + * `default` session — its variables, its open handles, its half-finished work. + * A silent fallback makes that failure look like success, so in broker mode the + * missing scope is an error instead. + */ export function getReplRegistry(): ReplRegistry { const scope = getConnectionScope(); if (scope) { if (!scope.repl) scope.repl = new ReplRegistry(); return scope.repl as ReplRegistry; } + if (brokerMode) { + throw new Error( + 'internal: no MCP connection scope is active, so this REPL call cannot be attributed ' + + 'to a caller. Refusing rather than risking another agent\'s session.', + ); + } if (!processRegistry) processRegistry = new ReplRegistry(); return processRegistry; } @@ -119,6 +187,9 @@ export function getReplRegistry(): ReplRegistry { */ export function disposeReplRegistry(): void { const scope = getConnectionScope(); + // Deliberately tolerant where getReplRegistry is strict: this runs on the + // teardown path, and throwing there would abandon the rest of a connection's + // cleanup to protect against a risk that only exists when CREATING sessions. if (scope) { (scope.repl as ReplRegistry | undefined)?.disposeAll(); scope.repl = undefined; diff --git a/src/mcp/repl/replRunnerSource.ts b/src/mcp/repl/replRunnerSource.ts index 9822d37ad..02c7cf001 100644 --- a/src/mcp/repl/replRunnerSource.ts +++ b/src/mcp/repl/replRunnerSource.ts @@ -33,18 +33,6 @@ * explicitly. */ -/** - * Sentinel the parent matches to detect a top-level-await snippet, so the retry - * fires on V8's actual message rather than on a guess about the user's code. - */ -export const TOP_LEVEL_AWAIT_MARKER = 'await is only valid in'; - -/** Sentinel for a `let`/`const` re-declared against a still-live session. */ -export const ALREADY_DECLARED_MARKER = 'has already been declared'; - -/** V8's message when `vm`'s synchronous watchdog fires. */ -export const SCRIPT_TIMEOUT_MARKER = 'Script execution timed out'; - /** * Child program source. Kept dependency-free and small enough to travel as a * command-line argument on every platform (Windows caps a command line at @@ -84,14 +72,35 @@ process.on('unhandledRejection', (reason) => { try { stderrWrite('[repl] unhandled rejection in background code: ' + (reason && reason.stack || reason) + '\n'); } catch (_) { /* stderr gone */ } }); +// inspect's maxStringLength and maxArrayLength bound individual strings and +// arrays, but NOT the number of keys on a plain object, so one +// Object.fromEntries over a million entries renders hundreds of megabytes. The +// parent truncates too, but only AFTER that string has crossed IPC and landed +// in the shared broker's heap, which is precisely the process that must not be +// asked to hold it. Cap here, at the only place the big string can be avoided. +const CHILD_RESULT_CAP = 64 * 1024; + function describe(value) { - return util.inspect(value, { - depth: 3, - maxArrayLength: 200, - maxStringLength: 8192, - breakLength: 100, - getters: false, - }); + let rendered; + try { + rendered = util.inspect(value, { + depth: 3, + maxArrayLength: 200, + maxStringLength: 8192, + breakLength: 100, + getters: false, + }); + } catch (err) { + // A Proxy with a throwing trap makes inspect itself throw. Without this the + // reply never goes out and the parent can only end the session on its hard + // deadline, reporting a hang for what is really an unprintable value. + return ''; + } + if (rendered.length > CHILD_RESULT_CAP) { + return rendered.slice(0, CHILD_RESULT_CAP) + + '\n… value truncated in the REPL process (' + rendered.length + ' chars rendered) …'; + } + return rendered; } // Wrap a top-level-await snippet so it can run as a Script. @@ -123,6 +132,26 @@ function wrapAsync(code) { const at = splits[s]; const tail = trimmed.slice(at + 1).trim(); if (tail === '') continue; + // Automatic semicolon insertion trap: a newline before one of these does + // NOT end a statement, so "const a = await f()\n[0].join()" is ONE + // expression. Splitting there yields two halves that each compile fine and + // together mean something else entirely, which is the worst failure mode + // available here - a silently different program and a wrong answer. Only + // an explicit semicolon can precede such a tail. + // 96 is a backtick (a tagged-template continuation); it cannot appear in + // the literal below without ending this runner's own template. + if ( + trimmed[at] !== ';' && + ('[(+-*/,.?:=<>&|'.indexOf(tail[0]) !== -1 || tail.charCodeAt(0) === 96) + ) continue; + // A trailing declaration or control statement must never be rewritten. Some + // of them WOULD compile inside "return (...)" - a function or class + // declaration becomes an expression - and the result is a wrong return + // value plus a definition trapped in the wrapper scope instead of the + // session. Compiling is not the same as meaning the same thing. + if (/^(function|class|async|let|const|var|return|if|for|while|do|switch|try|throw|import|export)\b/.test(tail)) { + continue; + } const head = trimmed.slice(0, at + 1); const rewritten = '(async () => {\n' + head + '\nreturn (' + tail + ');\n})()'; try { @@ -157,8 +186,31 @@ function trimStack(error) { return cut === -1 ? raw : lines.slice(0, cut).join('\n').replace(/\s+$/, ''); } -function fail(id, error) { - send({ id: id, ok: false, error: trimStack(error) }); +// Classify HERE, where the real Error object is, and ship the verdict as a +// field. The parent must not re-derive it by matching substrings against the +// error text: that text can be anything the caller's own code throws, so a +// script that throws "Script execution timed out" would make the tool report a +// watchdog stop that never happened. +function classify(error, timeoutMs) { + const message = String(error && error.message || error); + // V8's watchdog error carries this EXACT message and, because it is raised by + // the engine rather than by the script, no frame inside the evaluated code. + // A script that throws the same text still has its own wmux-repl frame, which + // is what keeps it from impersonating the watchdog. + if ( + message === 'Script execution timed out after ' + timeoutMs + 'ms' && + String(error && error.stack || '').indexOf('wmux-repl') === -1 + ) { + return 'timeout'; + } + if (error instanceof SyntaxError && message.indexOf('has already been declared') !== -1) { + return 'redeclare'; + } + return undefined; +} + +function fail(id, error, timeoutMs) { + send({ id: id, ok: false, error: trimStack(error), kind: classify(error, timeoutMs) }); } process.on('message', (msg) => { @@ -180,11 +232,11 @@ process.on('message', (msg) => { try { value = vm.runInThisContext(wrapAsync(msg.code), options); } catch (retryErr) { - fail(id, retryErr); + fail(id, retryErr, msg.timeoutMs); return; } } else { - fail(id, err); + fail(id, err, msg.timeoutMs); return; } } @@ -193,7 +245,7 @@ process.on('message', (msg) => { if (value && typeof value.then === 'function') { Promise.resolve(value).then( (resolved) => send({ id: id, ok: true, result: describe(resolved) }), - (err) => fail(id, err), + (err) => fail(id, err, msg.timeoutMs), ); return; } diff --git a/src/mcp/repl/tools.ts b/src/mcp/repl/tools.ts index 82639ac04..2db5b2ab1 100644 --- a/src/mcp/repl/tools.ts +++ b/src/mcp/repl/tools.ts @@ -64,6 +64,13 @@ export function formatOutcome( } if (outcome.remedy) lines.push(`note: ${outcome.remedy}`); + if (outcome.background) { + lines.push( + '', + '--- background output (from an earlier run still going) ---', + outcome.background.replace(/\n$/, ''), + ); + } if (outcome.stdout.text) { lines.push('', '--- stdout ---', outcome.stdout.text.replace(/\n$/, '')); if (outcome.stdout.truncated) { @@ -218,7 +225,9 @@ export function createReplToolCatalog(): readonly WmuxToolSpec[] { [ s.name, `pid ${String(s.pid ?? '?')}`, - s.busy ? 'running code' : 'idle', + // Report the real state: a session still coming up is not idle, and + // saying so sends the agent looking for a runtime that is not ready. + s.status, `${s.evals} run(s)`, `up ${formatDuration(now - s.createdAt)}`, `idle ${formatDuration(now - s.lastUsed)}`, diff --git a/src/mcp/repl/truncate.ts b/src/mcp/repl/truncate.ts index 105b95a15..16b1aa436 100644 --- a/src/mcp/repl/truncate.ts +++ b/src/mcp/repl/truncate.ts @@ -62,6 +62,32 @@ function skipToCodepointBoundary(buf: Buffer, index: number): number { return i; } +/** + * Length of `buf` with any trailing INCOMPLETE UTF-8 sequence removed. + * + * Distinct from walking back off a continuation byte at a known index: here the + * buffer already ends wherever the cap fell, so the question is whether its + * last lead byte got all the continuation bytes it needs. Asking + * `backOffToCodepointBoundary(buf, buf.length)` cannot answer that — it reads + * one byte past the end, which is undefined and never looks like a + * continuation, so it always reports the buffer as already clean. + */ +function trimIncompleteTrailingSequence(buf: Buffer): number { + const len = buf.length; + for (let back = 1; back <= 4 && back <= len; back++) { + const byte = buf[len - back]; + if ((byte & 0xc0) === 0x80) continue; // continuation; keep scanning back + let needed: number; + if ((byte & 0x80) === 0) needed = 1; + else if ((byte & 0xe0) === 0xc0) needed = 2; + else if ((byte & 0xf0) === 0xe0) needed = 3; + else if ((byte & 0xf8) === 0xf0) needed = 4; + else return len; // not valid UTF-8 at all; leave the bytes alone + return back >= needed ? len : len - back; + } + return len; +} + function elisionMarker(bytes: number): string { return `\n… ${bytes} bytes elided …\n`; } @@ -110,17 +136,24 @@ export class OutputBuffer { this.tailCap = Math.max(1, capBytes - this.headCap); } + /** + * Retained bytes are always COPIED out of the caller's chunk, never held as a + * subarray of it. Node hands out pipe chunks from a shared pool and a + * retained slice pins its whole pool block, so a writer producing many small + * chunks would hold tens of megabytes behind a 64 KB cap — defeating the one + * guarantee this class exists to make. The copies are bounded by the cap. + */ append(chunk: Buffer): void { this.total += chunk.length; let rest = chunk; if (this.headBytes < this.headCap) { const take = Math.min(this.headCap - this.headBytes, rest.length); - this.head.push(rest.subarray(0, take)); + this.head.push(Buffer.from(rest.subarray(0, take))); this.headBytes += take; rest = rest.subarray(take); } if (rest.length === 0) return; - this.tail.push(rest); + this.tail.push(Buffer.from(rest)); this.tailBytes += rest.length; // Drop from the FRONT of the tail so the ring always holds the most recent // bytes. A single oversized chunk is sliced rather than kept whole. @@ -164,7 +197,7 @@ export class OutputBuffer { elidedBytes: 0, }; } - const headEnd = backOffToCodepointBoundary(headBuf, headBuf.length); + const headEnd = trimIncompleteTrailingSequence(headBuf); const tailStart = skipToCodepointBoundary(tailBuf, 0); return { text: From 34dfd0c0dbd5b8786522fcd34b4cc0cbd68fc7d1 Mon Sep 17 00:00:00 2001 From: wong2kim Date: Mon, 31 Aug 2026 00:40:10 +0900 Subject: [PATCH 4/5] docs: add the changelog fragment for #1125 --- changelog.d/1125.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 changelog.d/1125.md diff --git a/changelog.d/1125.md b/changelog.d/1125.md new file mode 100644 index 000000000..f66c4480a --- /dev/null +++ b/changelog.d/1125.md @@ -0,0 +1,14 @@ +### Added + +- **Agents get a real REPL.** Three new MCP tools — `repl_run`, `repl_reset`, + `repl_sessions` — give an agent a persistent Node runtime where variables, + required modules, and open handles survive between calls, so it can build up + context instead of re-deriving it every time. Top-level `await` works, return + values come back inspected, and stdout, stderr, and the result arrive as + separate labelled blocks. Until now the only ways to run code were + `terminal_send`, which types keys at a shell and leaves you scraping the + screen for the answer, and `browser_evaluate`, whose page globals vanish on + navigation and can never touch a file or the network. Each session is its own + child process, so a runaway loop or a `process.exit()` costs you that session + and nothing else; runs have a timeout, output is capped with both ends kept, + and sessions live as long as your MCP connection. (#1125) From 4d4e12334434bdd8db2e077b9099b9339a6db001 Mon Sep 17 00:00:00 2001 From: wong2kim Date: Mon, 31 Aug 2026 00:52:14 +0900 Subject: [PATCH 5/5] fix(mcp): make the REPL cwd test portable and reclaim tools/list budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cwd test compared a path against the INSPECTED result, which is a JS string literal: on Windows every separator comes back escaped (C:\\Users\\...), so the substring test failed on CI even though the child was in the right directory. Compare inside the child and return a boolean instead, with realpath on both sides so macOS reporting /var/... as /private/var/... stays absorbed. The not-a-directory case now uses a path under the temp dir rather than a POSIX absolute that means nothing on Windows. Trim the REPL descriptions too. Rebasing onto #1124 left the full tools/list view at 79973 bytes against its 80000 budget — 27 bytes, which any later description edit would blow. The `let` re-declaration rule moves out of the always-on description: the session already reports it as a remedy at the moment it bites, which reaches the caller when it matters instead of costing context on every session. Back to 204 bytes of headroom. --- scripts/mcp-protocol-baseline.json | 2 +- .../__tests__/replSession.runtime.test.ts | 20 ++++++++++++--- src/mcp/repl/tools.ts | 25 ++++++++++--------- 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/scripts/mcp-protocol-baseline.json b/scripts/mcp-protocol-baseline.json index 4b8ea05b2..391fd15a1 100644 --- a/scripts/mcp-protocol-baseline.json +++ b/scripts/mcp-protocol-baseline.json @@ -3,7 +3,7 @@ "profiles": { "full": { "maxListBytes": 80000, - "wireResultSha256": "9bb6d64e7a5902d7ab85b0012c17994bbacc6fa1b07243e674f4c5af8e9d1520", + "wireResultSha256": "eb6cdfcae7a3bac1a2efdd9970ebaec56c7a9ded13764c9efbe5b373f65b471b", "instructionSha256": "f18849bb1ea62bcf5a1e46a73d633c787b4e08e763cc04b01c4e557df8f833eb", "toolNames": [ "browser_open", diff --git a/src/mcp/repl/__tests__/replSession.runtime.test.ts b/src/mcp/repl/__tests__/replSession.runtime.test.ts index cb430855c..1da83b6de 100644 --- a/src/mcp/repl/__tests__/replSession.runtime.test.ts +++ b/src/mcp/repl/__tests__/replSession.runtime.test.ts @@ -7,7 +7,9 @@ * parent does not orphan children — is only true if it is true of a real * process, so mocking the child would test nothing worth testing. */ +import * as fs from 'fs'; import * as os from 'os'; +import * as path from 'path'; import { afterEach, describe, expect, it } from 'vitest'; import { ReplSession, buildReplChildEnv } from '../ReplSession'; @@ -326,12 +328,22 @@ describe('ReplSession isolation', () => { it('runs in the cwd it was given and rejects one that does not exist', async () => { const session = makeSession(os.tmpdir()); - const outcome = await session.run('process.cwd()', 5000); + // Compare INSIDE the child and return a boolean. Matching the path against + // the inspected result instead would compare against a JS string literal: + // on Windows every separator comes back escaped (C:\\Users\\...), so a + // substring test for the real path fails even when the cwd is correct. + // realpath on both sides absorbs macOS reporting /var/... as /private/var/.... + const expectedCwd = fs.realpathSync(os.tmpdir()); + const outcome = await session.run( + `require("fs").realpathSync(process.cwd()) === ${JSON.stringify(expectedCwd)}`, + 5000, + ); expect(outcome.ok).toBe(true); - // macOS reports /var/folders/... as /private/var/folders/...; compare the tail. - expect(outcome.result?.text).toContain(os.tmpdir().replace(/^\/private/, '')); + expect(outcome.result?.text).toBe('true'); - expect(() => makeSession('/definitely/not/a/real/path')).toThrow(/cwd does not exist/); + expect(() => makeSession(path.join(os.tmpdir(), 'definitely-not-a-real-wmux-path'))).toThrow( + /cwd does not exist/, + ); }); it('exits when its parent IPC channel closes, so a crashed parent orphans nothing', async () => { diff --git a/src/mcp/repl/tools.ts b/src/mcp/repl/tools.ts index 2db5b2ab1..1444204d6 100644 --- a/src/mcp/repl/tools.ts +++ b/src/mcp/repl/tools.ts @@ -96,25 +96,26 @@ export function formatOutcome( } // Descriptions are deliberately tight: every byte here rides in tools/list on -// every session, and the protocol probe enforces a total budget for that view. -// Each sentence that survived earns its place — the persistence contract, the -// two await caveats, and the two facts an agent would otherwise get wrong (no -// sandbox, no lifetime past the connection). +// every session, and the protocol probe enforces a total budget for that view +// that the whole tool surface shares. Each sentence that survived earns its +// place — the persistence contract, the await caveat, and the two facts an +// agent would otherwise get wrong (no sandbox, no lifetime past the +// connection). The `let` re-declaration rule is deliberately NOT here: the +// session reports it as a remedy at the moment it bites, which reaches the +// caller when it matters instead of costing context on every session. const REPL_RUN_DESCRIPTION = 'Run JavaScript in a persistent Node runtime and get the return value back. ' + 'State survives between calls: variables (including top-level let/const), required ' + 'modules, and open handles are still there next call. Top-level await works, but ' + 'declarations inside an awaiting snippet do not persist — assign to a global ' + - '(x = await f()) to keep one. A name already bound by let/const cannot be re-declared; ' + - 'assign without a keyword. Full fs/net/require access, NO sandbox. Lives only as long ' + - 'as your MCP connection: no wmux restart, no sharing with other panes or workspaces.'; + '(x = await f()). Full fs/net/require access, NO sandbox. Lives only as long as your ' + + 'MCP connection: no wmux restart, no sharing with other panes or workspaces.'; const REPL_RESET_DESCRIPTION = - 'Throw away a REPL session and its state; the next repl_run starts a fresh runtime. ' + - 'Use when a session is wedged or holds a stale module.'; + 'Throw away a REPL session and its state; the next repl_run starts a fresh runtime.'; const REPL_SESSIONS_DESCRIPTION = - 'List this connection\'s REPL sessions: cwd, pid, age, and whether one is running code.'; + 'List this connection\'s REPL sessions: cwd, pid, age, and current state.'; export function createReplToolCatalog(): readonly WmuxToolSpec[] { const replRun = defineWmuxTool({ @@ -136,8 +137,8 @@ export function createReplToolCatalog(): readonly WmuxToolSpec[] { .string() .optional() .describe( - "Working directory, applied only when the session is created. Defaults to the MCP server's " + - "own cwd, which is not necessarily your pane's — pass it explicitly to be sure.", + "Working directory, honoured only when the session is created. Defaults to the MCP " + + "server's cwd, which is not necessarily your pane's — pass it explicitly.", ), }, profiles: ['full'],