docs(design): plan in-sandbox platform-tool MCP (Claude/Codex tools on Daytona, unified with Pi)#5234
docs(design): plan in-sandbox platform-tool MCP (Claude/Codex tools on Daytona, unified with Pi)#5234mmabrouk wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds design documentation for delivering platform tools through an in-sandbox MCP server and existing file relay, including transport selection, lifecycle behavior, security constraints, implementation slices, open questions, and status tracking. No runtime code changes are included. ChangesIn-sandbox tool MCP design
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d7eed07-2a85-4109-a6e8-25fb2352e8ee
📒 Files selected for processing (6)
docs/design/agent-workflows/projects/in-sandbox-tool-mcp/README.mddocs/design/agent-workflows/projects/in-sandbox-tool-mcp/context.mddocs/design/agent-workflows/projects/in-sandbox-tool-mcp/open-questions.mddocs/design/agent-workflows/projects/in-sandbox-tool-mcp/plan.mddocs/design/agent-workflows/projects/in-sandbox-tool-mcp/research.mddocs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md
| A2 also removes surface rather than adding it: no listener at all (not even loopback), no | ||
| port to choose, no readiness poll racing Claude's `tools/list` (the MCP handshake is | ||
| synchronous at spawn), and it works under `network: off` since it is stdio plus file I/O. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not claim restricted-network execution is supported yet.
A2 is network-independent at the transport level, but the existing run-plan.ts gate still refuses Daytona runs with restricted networking and executable tools, as documented in research.md:172-178 and this plan’s security section at Lines 228-229. Remove or defer the network-off acceptance cell unless that separate gate is intentionally changed.
Proposed documentation correction
- Network-off cell: gateway tool executes with `network` restricted +
- `best_effort` (relay is file I/O).
+ Network-off behavior remains out of scope until the existing restricted-network
+ run-plan gate is separately changed and validated.Also applies to: 282-283
| - **Crash mid-turn.** If the shim dies, in-flight `tools/call`s fail in the harness and the | ||
| model sees tool errors; the Claude SDK reports the server as failed. The runner relay loop | ||
| is unaffected (it just stops seeing requests). No runner-side supervision is needed | ||
| because the runner never owned the process. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define behavior when the shim crashes after writing a request.
If the shim exits after creating <id>.req.json but before consuming <id>.res.json, the runner can still execute a side-effectful tool while Claude reports an MCP failure. “The runner relay loop is unaffected” does not cover this lost-response case. Add a crash-after-write integration test and document the cancellation/idempotency policy before treating crash handling as complete.
|
Verdict: approve with comments. The A1-to-A2 flip is well argued and honestly costed: the warm-reuse lifecycle facts check out against the code ( 1. Factual: the orphaned-req.json risk is overstated, and the precision matters because you assign the bug to the sibling. research.md says a reused relay dir "would execute" a crashed turn's orphan on the next turn. The code already clears the relay dir on Daytona during workspace preparation ( 2. Factual: "The relay directory is derived from the durable cwd ( 3. Consistency with #5232: the shared-writer seam is described asymmetrically. Your coordination section says the sibling "swaps the internals" of 4. Consistency with #5201: slice 1 refactors the file #5201 edits, and neither plan declares the order. 5. Consistency: "fail loud for client tools on Daytona until continuation lands" is consistent with #5201's local-only scope, but the bridge between them is doubly disclaimed. Slice 4 lists "client tools through the shim (after the continuation work)" while deferring the relay pause protocol; #5201 scopes Daytona out entirely. Nobody owns: the relay park/late-response shape, a relay-backed implementation of #5201's delivery port (whose 6. Nits. Anchor drift of a couple of lines in This is an automated cross-consistency review requested by the owner, run alongside his own review of #5232 (event-driven tool relay). It checks this plan against #5201 (MCP client-tool continuation) and #5232 for contradictions, shared seams, and sequencing. |
…n Daytona, unified with Pi) Claude-Session: https://claude.ai/code/session_01Fr4A5zjs5rsufRaWgWiUNC
…MCP plan Correct the relay-dir and orphaned-request research claims, pin the waitForRelayResponse seam contract and landing order with the event-driven-tool-relay sibling, state the bearer layering rule and the ordering with mcp-client-tool-continuation, name the unowned Daytona client-tool bridge, cover crash-after-write, and clarify the network-off QA cell (best_effort opt-out). Claude-Session: https://claude.ai/code/session_01Fr4A5zjs5rsufRaWgWiUNC
mmabrouk
force-pushed
the
in-sandbox-tool-mcp
branch
from
fdeff6b to
7616d69
Compare
…inuation plan State the transport-specific limits of cancel and onClosed in the delivery port, open the transport union, add the future in-sandbox stdio mapping (PR #5234) with its missing prerequisites, pin the WP1 bearer to the HTTP transport wrapper, declare that PR #5234 slice 1 lands before WP1 and WP3, and correct the WP5 Daytona wording. Claude-Session: https://claude.ai/code/session_01Fr4A5zjs5rsufRaWgWiUNC
|
Follow-up: the review findings above are now addressed in this workspace (owner request). Addressed in this PR's branch:
Pending, deliberately not in this PR:
|
…relay modules, shrink v1 Fold the Codex xhigh review into the in-sandbox tool MCP plan. Gate A2 on a new slice 0 restart spike (session/load after a VM stop is adapter behavior, not correct by construction; orphan exit weakened to an expectation). Hand the relay-client/relay-protocol extraction to PR #5232 slice 0 and consume it here, reversing the earlier ordering. Cut the standalone transport-neutral mcp-handler slice and rewrite the unification section honestly (Pi never speaks MCP; the real sharing is the relay client and file protocol). Decide specs delivery now: a file, not an unbounded env variable. Move client tools, Codex-on-Daytona, snapshot bake, Pi-as-MCP-client, watch adoption, and mandatory replay capture to explicit follow-ups. Add the missing warm-reuse edges (session/load, sanitized-ID collision, bundle skew, partial visibility via #5232's atomic rename). Make the internal/user stdio separation structural and fix naming (tool-mcp-stdio.ts, internal-tool-mcp-handler.ts, helpers under engines/sandbox_agent/, ACP entry shapes out of mcp-bridge.ts). Claude-Session: https://claude.ai/code/session_0127AM79khCdvD2b8BG2joZL
…surement gates Fold the Codex xhigh review into the client-tool continuation plan. Rescope v1 to WP0 (expanded with cold-path baseline metrics: first-reissue match rate, argument drift, added model calls/latency/cost, wrong-replica rate, wait percentiles) plus WP1 loopback hardening (auth from headers before body parsing, timing-safe comparison, per-environment token and rotation test, result-size cap separated from auth). Defer WP2 through WP5 behind two concrete unlock gates: Claude holds the request past the 60-second idle TTL, and cold replay demonstrably harms users. Slim the delivery port to deliver -> accepted|unavailable plus dispose; cut cancel and onClosed (they modeled an HTTP response handle); correctness rests on lease expiry and teardown. Replace the standalone registry with pool-owned placement (ParkedClientTool beside ParkedApproval, awaiting_client_tool in session-pool.ts, McpHttpResultDelivery under tools/). Rename harnessToolCallId to toolCallId with its provenance as a registration invariant. Restrict any future warm mode to owner-routed deployments and define the delivery commit point. Drop the dependency on #5234's handler extraction (cut from that project's v1). The deferral is the top review question for the owner; it reverses part of a plan he LGTM'd and is flagged as such in status.md and open-questions.md. Claude-Session: https://claude.ai/code/session_0127AM79khCdvD2b8BG2joZL
|
Folded the Codex xhigh review into this workspace (commit d2832ea). The owner was asleep; per his standing simplify-aggressively instruction I adopted the cuts rather than defending scope. Everything is reversible at his review. Per finding:
Also dropped the cross-writer byte golden in favor of semantic contract tests (the reader is |
…matrix Slice 0 restart spike: session/load respawns the stdio MCP shim on the pinned adapter (claude-agent-acp 0.22.2, snapshot agenta-sandbox-pi); A2 locked, no engine fork needed. Slice 1 implemented on lane feat-in-sandbox-tool-mcp. Slice 2 matrix: all runnable cells green (Claude+local execution, Pi+Daytona execution, Claude+Daytona mechanism, client-tool and reserved-name refusals, no-tools negative); Claude+Daytona execution + warm/restart cells credit-blocked and recorded as the merge gate in status.md. Claude-Session: https://claude.ai/code/session_01Fr4A5zjs5rsufRaWgWiUNC
Harden the internal gateway-tool MCP endpoint that delivers Agenta tools to local Claude over loopback HTTP. Until now it relied only on 127.0.0.1 reachability, so any other local process could list or call tools through it. - Mint a per-server bearer token (randomBytes 32, base64url) in startInternalToolMcpServer; advertise it as the entry's Authorization header in buildToolMcpServers. - Authenticate before reading or parsing the request body; reject missing/wrong tokens with 401 and no dispatch. Timing-safe comparison, length-guarded. - Reject a JSON-RPC batch containing a client tool before executing any item. - The bearer lives ONLY in the HTTP transport path; the in-sandbox stdio shim (#5234) stays credential-free by construction (different file, different transport). It is a local loopback guard, never a provider/control-plane credential — the private callRef still never reaches the advertisement. Adds real-socket HTTP integration tests (auth, malformed/oversized body, batch client-tool rejection, close) and updates the two layering pins that asserted headers: [] to expect the loopback guard header. WP0: checked-in measurement protocol + unlock-gate scorecard for the deferred warm hold-open path (results marked PENDING LIVE RUN). No runtime behavior change from WP0. Claude-Session: https://claude.ai/code/session_0127AM79khCdvD2b8BG2joZL
…lay writer Slice 0 of the event-driven tool relay: move the in-sandbox writer (relayToolCall) out of tools/dispatch.ts into tools/relay-client.ts, and the wire protocol (suffixes, request/response types, sanitizeRelayId, request serialization, shared timing env constants) into tools/relay-protocol.ts. Both new modules are bundle-safe (node builtins only) so the Pi extension bundle and the future in-sandbox MCP shim (Agenta-AI#5234) can consume them without pulling in server-side runner code. dispatch.ts and relay.ts re-export every moved symbol, so all call sites compile unchanged. Zero behavior change; a golden test pins the byte-exact .req.json serialization as the cross-writer contract. Side effect: the extension bundle drops the server-side relay loop it previously pulled in through dispatch's constant imports. Claude-Session: https://claude.ai/code/session_01Fr4A5zjs5rsufRaWgWiUNC
1 participant
Context
A Claude Code run on Daytona with any custom tool is refused up front (
run-plan.ts:355,REMOTE_TOOLS_UNSUPPORTED_MESSAGE): the internal tool MCP server binds the runner's loopback, unreachable from inside a remote sandbox, and Claude has no in-sandbox writer for the file relay the way Pi does. So the harness we want people to use for serious agents gets zero platform tools in the sandbox we want to be the default.This PR is design only: a planning workspace at
docs/design/agent-workflows/projects/in-sandbox-tool-mcp/(README, context, research, plan, open-questions, status). No runtime code changes.Settled decisions the workspace encodes (owner, 2026-07-11)
Recommended design
Revive PR #4873 (closed, pre-rename): a small dependency-free stdio MCP server bundle, uploaded into the sandbox and advertised as an internal
McpServerStdioentry namedagenta-tools. The harness's ACP adapter spawns it inside the sandbox;tools/callwrites the same relay request file Pi's extension writes; the runner executes server-side behind the existing permission guard.This flips the earlier
claude-daytona-toolsrecommendation (A1, HTTP on the sandbox loopback) to A2 (harness-spawned stdio). Warm sandbox reuse (#5225) is the reason: a harness-spawned shim inherits the session lifecycle, so park-to-running, park-to-stopped, and tool-set changes between turns are correct by construction, while a runner-started HTTP shim would need port management, readiness polling, health checks on resume, and stale-process replacement. Theremote-tools-deliverydaemon-spawned candidate collapses into A2 (the daemon already forwards session MCP entries; no sandbox-agent change needed).Unification story: extract the relay writer (
relay-client.ts) and a transport-neutral MCP handler (mcp-handler.ts) as shared modules consumed by the Pi extension path, the local Claude HTTP server, and the new shim, with a golden test pinning byte-identical relay request files. "Pi consumes the MCP server directly" is written up as a later option with honest trade-offs (Pi has no MCP client by design; the payoff is user-MCP-on-Pi, not this feature).Sequencing: gateway/callback tools first; client tools (
request_connection) stay fail-loud on this path until the client-tool continuation work lands (the relay parks them with no response file, so the shim would hang). Cross-referenced with the concurrentevent-driven-tool-relaysibling: shared file contract, and the shim's response wait is one swappable function.What review is needed
A plan review, not a code review. The interview list is
open-questions.md; the two decisions that shape implementation most:https://claude.ai/code/session_01Fr4A5zjs5rsufRaWgWiUNC