Give agents a persistent Node REPL instead of typing into a terminal - #1125
Merged
Conversation
|
Warning Review limit reachedNext included review available in 10 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openwong2kim
added a commit
that referenced
this pull request
Aug 30, 2026
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.
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.
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.
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.
openwong2kim
force-pushed
the
feat/agent-repl
branch
from
August 30, 2026 15:53
46b6c2a to
4d4e123
Compare
Owner
Author
[wmux-hermes] Triage SummaryThis PR adds persistent per-connection Node REPL tools (repl_run / repl_reset / repl_sessions) backed by child processes scoped in ConnectionScope like the PlaywrightEngine. Review focus: the no-new-authority claim (terminal_send as the ceiling), child lifecycle and dual-timeout handling, and the tools/list budget now at 79866/80000 bytes. P2 — a large new agent-facing surface that merits prompt, careful review. |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gap
wmux gives MCP callers no structured runtime. The two things that look like one aren't:
terminal_sendtypes keys into a PTY. No return value, no error channel, no types — you scrape the screen and hope the prompt came back.browser_evaluatekeeps page-scope globals, but they die on navigation, and it can't reach the filesystem, the network, orrequire().So an agent doing real work re-derives its context on every call: re-read the file, re-parse the JSON, re-connect to the thing. A persistent REPL is the single highest-use agent surface in comparable tools; wmux had zero of it.
What this adds
Three tools —
repl_run,repl_reset,repl_sessions— backed by a persistent Node runtime whose variables, required modules, and open handles survive between calls.Top-level
awaitworks. Return values come back inspected. stdout, stderr, and the result are separate labelled blocks.Design notes
A child process, not
node:vmin-process. REPL code is code the caller wrote seconds ago and has never run: it callsprocess.exit(), it throws from native callbacks, it blows the heap. The MCP broker hosts every agent's connection in one process, so an in-process runtime would turn one agent's typo into every agent's outage. A child makes the blast radius exactly one session.The eval protocol rides the child's IPC channel, which leaves stdout/stderr free to carry the user code's own output verbatim — nothing to frame, nothing to escape, and no way for a printed line to forge a protocol message.
vm.runInThisContext, not a freshvmcontext. A fresh context has nosetTimeout, nofetch, noTextEncoder(measured, not assumed), so it would need a hand-curated global list that goes stale every Node release. The child's real context gives the whole standard library, and top-levellet/conststill persist because a Script's lexical declarations live in the context's global lexical scope. Onlyrequireis installed explicitly, bound to the session cwd.Two timeout layers, because one isn't enough. The vm watchdog stops a synchronous runaway while keeping session state. A parent hard deadline SIGKILLs the child for what the watchdog cannot see — a promise that never settles, a blocked native call — and that state loss is always reported, never papered over.
Bounded output. stdout/stderr are retained head-and-tail per eval with an exact elided byte count. A 2 MB flood comes back as a 65 KB tool payload with both ends intact.
Lifecycle, stated plainly
Sessions are scoped to the caller's MCP connection, held in
ConnectionScopebeside thePlaywrightEnginefor the same reason: a process-global map would hand one agent another agent's live state. They die on reset, idle (15 min), hard timeout, connection close, or wmux restart. None of that is survivable, and the tool descriptions say so — an agent that believes a variable survived when it didn't writes code against a fiction.Security posture
No new authority. A caller holding
terminal_sendalready drives an arbitrary shell as the user in its own pane. This changes ergonomics, not the trust boundary. It takes no RPC and needs no workspace identity — nothing crosses the substrate, so there's nothing for the daemon to authorize.The child's env is narrower than that shell's: the gated-automation filter (wmux/Electron internals, credential-shaped names), plus
CLAUDE*/ANTHROPIC*/AI_AGENTdropped on top — the markers that make a nested agent silently stop persisting its transcript, or quietly move work onto metered auth. Withheld credential names (not values) are reported so a failing script is diagnosable in one step.No filesystem or network sandbox is claimed, and the tool description says that too.
Workspace boundaries do not reach into the REPL. Because this runs arbitrary JavaScript with full filesystem and network access, the workspace scoping that #922's verified lane applies to workspace-addressed tools has no purchase here: nothing stops an approved caller from using
repl_runto read another workspace's files. That is not a contradiction of #922, whose contract is to keep an approved tool inside its approved scope rather than to defend against same-user code — but the boundary is worth stating plainly rather than leaving to be discovered.Review
A multi-model panel reviewed the diff (Claude + GLM; Codex was unavailable —
402 deactivated_workspace). Five findings had cross-model agreement. Everything accepted is fixed and covered by a test. The ones worth naming:process.send, so a script could report success, collect the answer, and clear the hard-deadline timer while spinning forever in the shared broker. Replies now must carry the in-flight eval id.return, guarded only by "does it still compile" — but no semicolon is inserted before a newline followed by[, soconst a = await f()\n[0]is one expression that split into two compilable halves and answered[ 0 ].Verification
tsc --noEmit,npm run linton the new files, fullnpm test(two failures are the known symlinked-node_modulesworktree artifacts:generateNoticesandatlasCoherence, both unrelated).npm run build:mcp && npm run probe:mcpgreen; protocol baseline updated. Note the full tools/list is now 79796 / 80000 bytes — 204 to spare. Rebasing onto fix(browser): stable snapshot refs, stale-ref rejection, and the documented extract_text default #1124 had left only 27, so the REPL descriptions were trimmed rather than the shared budget raised; the next tool added will still need that budget revisited.dist/handlers.Not in scope
Multi-language runtimes (Python, Ruby), daemon-hosted sessions shared across panes or workspaces, persistence across a wmux restart, remote execution, a UI panel, streaming partial output, and any filesystem/network sandbox.