feat(code-mode): isolate snippet execution in a subprocess with RPC bridge - #10
Conversation
…ridge User snippets now run in a fresh child process (`_worker.py`) per call instead of in the server process. The parent audits the snippet (AST allow-list) before spawning, holds the live MCP tool registry and request `ctx` (neither crosses the process boundary), and answers `ynab.read`/`ynab.write` calls over a stdio JSON-RPC bridge using length-prefixed frames. Because user code runs in a separate OS process, the execution timeout is now a hard wall clock: on expiry the parent `kill()`s the child, terminating synchronous blocking or CPU-bound code that the old in-process `asyncio.wait_for` could never interrupt. Closes mcp-ynab-fkv. - New `_sandbox.py`: leaf module of pure primitives (safe builtins, code wrap, bounded stdout, result truncation, length-prefixed framing) shared by parent and worker. Imports only stdlib + pydantic_core — never the server — so the worker boots without building the FastMCP app or demanding an API key. - New `_worker.py`: side-effect-free child, launched by file path (not `-m`) to bypass `mcp_ynab/__init__`'s server import. Builds RPC stubs, runs the snippet, captures stdout, ships a result frame. - `runner.py`: rewritten parent side — audit-then-spawn, concurrent RPC serving (no deadlock), hard-kill on timeout, fail-closed on malformed frames. - Tests: hard-kill of a non-cooperative loop (fkv guard), >64KB RPC payload round-trip (framing/deadlock guard), and a real `server.execute` -> real mcp -> real subprocess end-to-end run. Out of scope (tracked separately): RLIMIT/seccomp (mcp-ynab-fsv.1b), env scrubbing, network isolation. Refs mcp-ynab-a43
📝 WalkthroughWalkthroughThis PR moves Code Mode execution to a per-invocation subprocess: parent audits and spawns a worker, communicates over length-prefixed JSON frames for RPC, and enforces hard timeouts by killing the child; the worker runs user code with safe builtins, bounded stdout, and result truncation. ChangesSubprocess-Isolated Code Mode Execution
CI and Repo Config
Sequence DiagramsequenceDiagram
participant Parent
participant Worker
participant UserCode
participant Tools
Parent->>Worker: startup frame (code, mode, dispatch, limits)
Worker->>Worker: wrap_code + compile
Worker->>UserCode: exec in sandbox globals
UserCode->>Worker: ynab.read/ynab.write call (rpc)
Worker->>Parent: rpc request frame
Parent->>Tools: call real tool
Tools-->>Parent: tool result
Parent->>Worker: rpc response frame
UserCode->>Worker: return result
Worker->>Parent: final result frame (value, logs, truncated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.beads/config.yaml (1)
64-64: Beads config key formatting is normalized;sync.remoteshould be applied correctly.
sync.remoteat.beads/config.yamlline 64 matches Beads’ canonical dotted key format. Beads also normalizes known hyphenated aliases, so the existingsync-branchat line 45 should not be ignored at runtime (it should map tosync.branch).
- Optional: rename
sync-branch→sync.branchin the config for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.beads/config.yaml at line 64, sync.remote is already using the canonical dotted key but the hyphenated alias sync-branch may be confusing; update the config so the branch key uses the canonical form by renaming sync-branch → sync.branch and verify the existing sync.remote entry remains unchanged; ensure any code reading the config (references to sync.remote, sync.branch) still works with Beads' normalization if you choose to keep the alias, but prefer the explicit sync.branch key for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.beads/config.yaml:
- Line 64: sync.remote is already using the canonical dotted key but the
hyphenated alias sync-branch may be confusing; update the config so the branch
key uses the canonical form by renaming sync-branch → sync.branch and verify the
existing sync.remote entry remains unchanged; ensure any code reading the config
(references to sync.remote, sync.branch) still works with Beads' normalization
if you choose to keep the alias, but prefer the explicit sync.branch key for
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3b44cc1-78f0-47a4-ac2b-84a117818a62
📒 Files selected for processing (6)
.beads/config.yamlsrc/mcp_ynab/code_mode/README.mdsrc/mcp_ynab/code_mode/_sandbox.pysrc/mcp_ynab/code_mode/_worker.pysrc/mcp_ynab/code_mode/runner.pytests/test_code_mode.py
Adding/removing labels on a pull request maps to the `pull-requests`
permission scope even though the REST path is `/issues/{n}/labels`. The
workflow declared `issues: write` but only `pull-requests: read`, so
`issues.addLabels` returned 403 "Resource not accessible by integration"
on every PR that needed a release label (observed on PR #10 and the
dependabot PRs).
Grant `pull-requests: write` so the labeler can apply patch/minor/major
labels. Note: pull_request_target runs the base-branch copy of the
workflow, so this takes effect for PRs opened after it lands on main.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.beads/config.yaml:
- Line 45: Update the Beads config's sync.remote value to use a Dolt-compatible
URL scheme: replace the current git+https://... remote with
https://github.com/klauern/mcp-ynab.git so Beads does not prepend/modify the
scheme; ensure the dotted key sync.remote (and existing sync.branch) remain
unchanged and the URL is exactly the https:// form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| # Can also use BEADS_SYNC_BRANCH env var for local override. | ||
| # If not set, bd sync will require you to run 'bd config set sync.branch <branch>'. | ||
| sync-branch: "beads-sync" | ||
| sync.branch: "beads-sync" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the correct configuration key format for Beads sync branch setting: sync-branch or sync.branch?
💡 Result:
The correct configuration key for the Beads sync branch setting is sync.branch [1][2][3]. Official documentation and codebase references consistently use the dot-separated format sync.branch for setting, retrieving, and unsetting this configuration [1][2][3]. For example, you can set the sync branch using the following command [2]: bd config set sync.branch You can verify the current setting with [2][3]: bd config get sync.branch Additionally, the corresponding environment variable for this setting is BEADS_SYNC_BRANCH [1][2].
Citations:
- 1: gastownhall/beads@f73651a
- 2: https://github.com/steveyegge/beads/blob/main/docs/PROTECTED_BRANCHES.md
- 3: https://github.com/steveyegge/beads/blob/c446a2ef/examples/protected-branch/README.md
🌐 Web query:
Beads config key sync.remoteexpected URL scheme: should it begit+https://...or plainhttps://...? Also confirm Beads uses sync.remote and what values it accepts.
💡 Result:
Beads does utilize the sync.remote configuration key, which is used to define the remote repository for synchronization [1][2]. Regarding the URL scheme, you should generally provide the URL as it would be used by Dolt (the underlying engine for Beads) without extra prefixes unless required by your specific remote type [3]. Importantly, recent updates to Beads (as of April 2026) specifically avoid normalizing or altering user-provided sync.remote URLs because automatic conversion (such as adding git+ prefixes) was found to break certain Dolt remotesapi endpoints [4]. Therefore, you should provide the exact URL string compatible with your target remote (e.g., https://github.com/org/repo.git) [4][3]. Values accepted by sync.remote are any Dolt-compatible remote URLs [2][5]. Examples of supported schemes include: - HTTPS: https://github.com/org/repo.git [3] - SSH: git+ssh://git@github.com/org/repo.git [5][3] - Other Dolt-supported backends: dolthub://, s3://, gs://, az://, or file:// [1][3] You can configure this setting via the command line (e.g., bd config set sync.remote ) or by directly editing the.beads/config.yaml file [1][2][5]. Note that Beads will persist this value into your configuration to ensure it is used for synchronization [2][5].
Citations:
- 1: https://gastownhall.github.io/beads/reference/configuration
- 2: https://github.com/steveyegge/beads/blob/c446a2ef/cmd/bd/sync_remote.go
- 3: https://github.com/steveyegge/beads/blob/main/docs/SYNC_SETUP.md
- 4: gastownhall/beads@1e76efc
- 5: https://github.com/gastownhall/beads/blob/main/docs/SYNC_CONCEPTS.md
Fix sync.remote URL scheme in .beads/config.yaml (line 64)
- Line 45:
sync.branch(dotted notation) is the correct Beads config key; the change is fine. - Line 64: Beads expects a Dolt-compatible remote URL and avoids adding/rewriting prefixes—use
https://github.com/klauern/mcp-ynab.gitinstead ofgit+https://...to prevent sync failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.beads/config.yaml at line 45, Update the Beads config's sync.remote value
to use a Dolt-compatible URL scheme: replace the current git+https://... remote
with https://github.com/klauern/mcp-ynab.git so Beads does not prepend/modify
the scheme; ensure the dotted key sync.remote (and existing sync.branch) remain
unchanged and the URL is exactly the https:// form.
Summary
Closes the long-standing Code Mode runner gap: user snippets ran in the server process under a soft
asyncio.wait_fortimeout that could not interrupt synchronous blocking or CPU-bound code. They now run in a fresh child process per call with a hard, kill-enforced timeout.Closes
mcp-ynab-a43(subprocess isolation + RPC bridge) andmcp-ynab-fkv(soft-timeout bug). Remaining OS-level hardening (RLIMIT/seccomp/env-scrub/network) split into the newmcp-ynab-fsv.7.How it works
runner.py): audits the snippet (AST allow-list) before spawning, holds the live MCP tool registry and requestctx(neither crosses the process boundary), spawns the worker, and answersynab.read/ynab.writecalls over a stdio JSON-RPC bridge. Hard-kill()s the child on timeout; fails closed on malformed frames._worker.py: side-effect-free child, launched by file path (notpython -m) so it never triggersmcp_ynab/__init__'sfrom .server import mcp— the child boots without building the FastMCP app or needing a YNAB API key. Builds RPC stubs, runs the snippet, captures stdout, ships a result frame._sandbox.py: new leaf module of pure primitives (safe builtins, code wrap, bounded stdout, result truncation, length-prefixed framing) shared by both sides. Imports only stdlib + pydantic_core.Length-prefixed frames (
<len>\n<payload>+readexactly) are used instead of newline-delimited JSON so a single tool result can exceed asyncio's 64KB line-buffer limit.Tests (314 pass, +3 new)
test_run_code_hard_kills_synchronous_blocking_code— a non-cooperativewhile Trueloop is killed at the 0.2s timeout (~0.21s wall), the fkv guard.test_run_code_handles_large_rpc_payload— a ~200KB tool result round-trips without deadlock (framing guard).test_execute_end_to_end_spawns_real_subprocess— realserver.execute→ realmcpregistry → real worker in the live event loop, no mocks.All 39 pre-existing Code Mode tests pass unchanged (behavior/contract preserved:
CodeModeResultschema, stdout→logs capture, Pydantic boundary serialization, mutations gating, truncation byte counts).Out of scope (tracked in
mcp-ynab-fsv.7)RLIMIT/seccomp, child env scrubbing (currently inherits parent env), network isolation. The process boundary contains crashes/hangs/accidental blocking; it is not yet a hardened sandbox for adversarial Python.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores