From 01837e7e3236dd48fb31cc73b6d2196bfdde39f5 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 11 Jul 2026 22:28:18 +0200 Subject: [PATCH 1/4] docs(design): plan in-sandbox platform-tool MCP (Claude/Codex tools on Daytona, unified with Pi) Claude-Session: https://claude.ai/code/session_01Fr4A5zjs5rsufRaWgWiUNC --- .../projects/in-sandbox-tool-mcp/README.md | 69 ++++ .../projects/in-sandbox-tool-mcp/context.md | 105 +++++++ .../in-sandbox-tool-mcp/open-questions.md | 53 ++++ .../projects/in-sandbox-tool-mcp/plan.md | 297 ++++++++++++++++++ .../projects/in-sandbox-tool-mcp/research.md | 178 +++++++++++ .../projects/in-sandbox-tool-mcp/status.md | 45 +++ 6 files changed, 747 insertions(+) create mode 100644 docs/design/agent-workflows/projects/in-sandbox-tool-mcp/README.md create mode 100644 docs/design/agent-workflows/projects/in-sandbox-tool-mcp/context.md create mode 100644 docs/design/agent-workflows/projects/in-sandbox-tool-mcp/open-questions.md create mode 100644 docs/design/agent-workflows/projects/in-sandbox-tool-mcp/plan.md create mode 100644 docs/design/agent-workflows/projects/in-sandbox-tool-mcp/research.md create mode 100644 docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/README.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/README.md new file mode 100644 index 0000000000..48b9bc6fbb --- /dev/null +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/README.md @@ -0,0 +1,69 @@ +# In-sandbox platform-tool MCP + +Deliver Agenta gateway and platform tools to MCP-client harnesses (Claude Code today, Codex +next) running in a Daytona sandbox, by running our own small MCP server inside the sandbox. +Its `tools/call` handler writes to the file relay the runner already polls and executes +server-side. Today that combination gets zero tools and the run is refused up front +(`services/runner/src/engines/sandbox_agent/run-plan.ts:355`, +`REMOTE_TOOLS_UNSUPPORTED_MESSAGE`). + +The design goal the owner cares about most is unification with Pi: one gateway-tool code +path serving both the Pi extension and the new MCP server, pinned by a golden test so the +relay protocol cannot fork per harness. + +## Glossary + +- **Runner**: the Node sidecar (`services/runner/`) that executes agent runs. It holds the + run's credentials and executes gateway and platform tools server-side. +- **Harness**: the coding agent that runs inside the sandbox (Pi, Claude Code, later Codex). +- **Sandbox**: the isolated environment the harness runs in. Local (same machine as the + runner) or Daytona (a remote cloud VM the runner reaches through a daemon API). +- **Daemon**: the sandbox-agent process inside the sandbox that the runner drives over a + signed URL. It creates harness sessions and reads/writes sandbox files for the runner. +- **File relay**: the file-based tool-call channel. An in-sandbox writer creates + `.req.json` in a relay directory; the runner polls the directory, executes the call + with runner-held credentials, and writes `.res.json` back. +- **Gateway tools**: backend-resolved tools (Composio actions, workflow-as-tool, platform + operations) executed through Agenta's `/tools/call` with server-side credentials. +- **Client tools**: browser-fulfilled tools (for example `request_connection`). A call + pauses the turn and a human answers it in the playground. +- **Public spec**: the credential-free advertisement shape of a resolved tool (name, + description, input schema, kind). Private fields (`callRef`, code, scoped env, callback + auth) never leave runner memory. +- **The shim**: this project's deliverable. A small, dependency-free MCP server process + inside the sandbox that advertises the public specs and forwards each `tools/call` to + the file relay. +- **Warm reuse / park**: since PR #5225 a Daytona sandbox survives across turns. It stays + running for an idle window (park-to-running), then stops without deletion + (park-to-stopped) and restarts on the next turn. + +## Files and reading order + +1. [context.md](context.md): why the work exists, the settled owner decisions (including + the user-MCP HTTP-only policy), goals, non-goals, and neighboring projects. +2. [research.md](research.md): the verified current state with file and line anchors, what + PR #4873 built and why it went stale, and the warm-reuse lifecycle facts the design + must survive. +3. [plan.md](plan.md): the recommended design (transport choice, unification path, + lifecycle, security), the implementation slices, and the test plan. +4. [open-questions.md](open-questions.md): the decisions the owner still needs to make. +5. [status.md](status.md): progress and provenance. Source of truth for state. + +## Prior art this builds on (not duplicated here) + +- [../claude-daytona-tools/](../claude-daytona-tools/README.md): the full option analysis. + Option A (in-sandbox MCP front-end over the relay) recommended; Option B (tunnel the + runner's MCP) rejected. +- [../remote-tools-delivery/specs.md](../remote-tools-delivery/specs.md): independent + reconfirmation; recommends the in-sandbox relay client. +- [../mcp-delivery-architecture/](../mcp-delivery-architecture/README.md): the umbrella. + `gateway-mcp-location.md` records the 2026-07-11 owner decision that rejects the + API-hosted gateway and commits to the in-sandbox front-end. +- [../gateway-tool-mcp/](../gateway-tool-mcp/README.md): the three-MCP-layer distinction + (user stdio / internal gateway channel / user HTTP) that this plan keeps separable. +- PR #4873 (closed, unmerged): a working implementation of the stdio variant, pre-rename. + Mined in [research.md](research.md). + +## Status + +DESIGN ONLY. No runtime code changes in this PR. See [status.md](status.md). diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/context.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/context.md new file mode 100644 index 0000000000..20a295785d --- /dev/null +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/context.md @@ -0,0 +1,105 @@ +# Context + +## What the user sees today + +A Claude Code run on Daytona that carries any custom tool fails immediately with +`REMOTE_TOOLS_UNSUPPORTED_MESSAGE` (`services/runner/src/engines/sandbox_agent/run-plan.ts:355`). +The same run works on the local sandbox, and the same tools work on Daytona with the Pi +harness. The refusal is deliberate (PR #5047 replaced an earlier silent zero-tools success), +but it leaves a hole in the product: the harness we want customers to use for serious agents +(Claude, soon Codex) cannot use platform tools in the sandbox environment we want to be the +default (Daytona, now fast thanks to warm reuse). + +The cause is an advertisement gap, not an execution gap. Tool execution on Daytona is solved +and harness-agnostic: the runner polls a relay directory in the sandbox filesystem, executes +each request with runner-held credentials, and writes the response back. Pi works because its +bundled extension runs inside the sandbox and writes those request files. Claude takes tools +only over MCP, and the only MCP server we run today binds to the runner's own loopback, which +is unreachable from inside a remote sandbox. Nothing inside the sandbox speaks MCP for us. + +## Settled decisions (owner, 2026-07-11) + +These are decided. This workspace encodes them; it does not reopen them. + +1. **The sandbox talks only to the runner.** An API-hosted tool gateway (a platform MCP + endpoint the sandbox dials directly) was analyzed and rejected in + [../mcp-delivery-architecture/gateway-mcp-location.md](../mcp-delivery-architecture/gateway-mcp-location.md). + Warm sandboxes are the priority, and the committed model is: gateway-tool logic stays in + the runner, credentials never enter the sandbox, and delivery to MCP-client harnesses goes + through an in-sandbox front-end feeding the file relay. +2. **User-declared MCP servers: HTTP transport only, permanently.** We will never host or + run arbitrary user stdio/npx MCP servers, on the runner host or in the sandbox. A user who + wants a local MCP server runs it themselves and gives us the URL and host. This supersedes + the L1 direction ("run user stdio MCP in the sandbox") in + [../mcp-delivery-architecture/directions.md](../mcp-delivery-architecture/directions.md). + Authentication for user HTTP MCP servers: an API key in a request header, which the + existing mechanism already supports (named secrets become headers, + `services/runner/src/engines/sandbox_agent/mcp.ts:119`). OAuth for user MCP servers is a + later feature, named as future work and out of scope here. +3. **Platform tools (our own) are delivered by our own MCP server running inside the + sandbox**, feeding the existing file relay. This project designs that server. +4. **The design goal that matters most is unification with Pi.** Today Pi gets tools through + its bundled in-sandbox extension (`registerTool`, writing relay request files) and local + Claude gets them through a runner-loopback HTTP MCP server. The owner wants one + gateway-tool logic serving both, in closely shared code, so the relay protocol and the + execution semantics cannot drift per harness. + +## The three MCP layers, kept separable + +The word "MCP" names three different things in this codebase +(established in [../gateway-tool-mcp/README.md](../gateway-tool-mcp/README.md); conflating +them caused the #4831 regression). This plan touches only the second. + +| Layer | Declared by | Status | This project | +| --- | --- | --- | --- | +| User stdio/npx MCP servers | The user (`transport: "stdio"`) | Disabled, now permanently (decision 2) | Untouched. The mechanism that spawns our shim must not relax this gate. | +| Internal gateway-tool channel | Nobody; synthesized by the runner from the run's resolved tools | Local only (runner-loopback HTTP) | Extended into the sandbox. This is the whole project. | +| User HTTP MCP servers | The user (`transport: "http"` + URL) | Built, SSRF-guarded, behind `AGENTA_AGENT_MCPS_ENABLED` (default off) | Untouched. API-key-in-header auth is the current answer; OAuth is future work. | + +## Goals + +1. A Claude run on Daytona with gateway/callback tools succeeds, and the tools actually + execute (the current refusal stops firing for that combination). +2. The delivery is harness-agnostic on the sandbox side: any MCP-client harness (Codex next) + gets the same tools with no per-harness work beyond its ACP adapter. +3. One shared implementation of "turn a tool call into a relay request" serves the Pi + extension, the local Claude channel, and the new in-sandbox server, pinned by a golden + test on the relay request file bytes. +4. The lifecycle survives warm sandbox reuse (PR #5225): a parked-and-resumed sandbox, a + stopped-and-restarted sandbox, and a tool-set change between turns must all behave + correctly. +5. No security invariant weakens: public specs and the relay directory are the only things + that enter the sandbox; no credential ever does; no new network surface opens. + +## Non-goals + +- Client tools (browser round-trip, `request_connection`) on Claude+Daytona in the first + slice. Their pause semantics depend on the client-tool continuation work + ([../agent-client-tool-cleanup/](../agent-client-tool-cleanup/), + [../mcp-client-tool-continuation/](../mcp-client-tool-continuation/)). Gateway/callback + tools land first; the plan states the sequencing. +- Changing the relay file protocol or its polling mechanics. The sibling project + [../event-driven-tool-relay/](../event-driven-tool-relay/README.md) owns relay latency; + this plan only keeps the shim compatible with it. +- Re-enabling user stdio MCP in any form (decision 2). +- OAuth for user HTTP MCP servers (future work). +- Any new remote sandbox provider. The fail-closed gate for non-Daytona remote providers + stays until delivery is proven per provider. +- Changing how local Claude gets tools. The runner-loopback HTTP channel works and keeps its + client-tool pause behavior; it stays. + +## Neighboring projects + +- [../claude-daytona-tools/](../claude-daytona-tools/README.md) and + [../remote-tools-delivery/specs.md](../remote-tools-delivery/specs.md): the two prior + designs this plan reconciles into one implementation path. +- [../mcp-delivery-architecture/](../mcp-delivery-architecture/README.md): the umbrella and + the decision record. +- [../event-driven-tool-relay/](../event-driven-tool-relay/README.md): concurrent sibling. + It replaces relay polling with filesystem-event wakeups. The shim is a second writer of + the same relay files, so the two projects share the file contract; neither implements the + other's scope. +- [../session-keepalive/](../session-keepalive/) and the warm-Daytona work (PR #5225): the + reuse lifecycle this design must survive. +- [../agent-client-tool-cleanup/](../agent-client-tool-cleanup/): the park-and-resume + redesign client tools depend on. diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/open-questions.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/open-questions.md new file mode 100644 index 0000000000..f03e23bab0 --- /dev/null +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/open-questions.md @@ -0,0 +1,53 @@ +# Open questions + +Decisions the owner needs to make or confirm. Each states the recommendation so a yes is +enough. + +1. **Transport flip: approve A2 (harness-spawned stdio) over A1 (HTTP loopback)?** + `claude-daytona-tools/design.md` recommended A1 with A2 as fallback, before warm sandbox + reuse existed. The plan flips that: A2's lifecycle is tied to the harness session, which + makes every reuse case (park-to-running, park-to-stopped, tool-set change) correct by + construction, and #4873 already implemented it. A1 stays the documented fallback if an + ACP adapter refuses stdio entries. Recommendation: approve the flip. + +2. **Client tools on Claude+Daytona: keep failing loud until the continuation work lands?** + Slice 1 delivers executable tools only; a run carrying a client tool + (`request_connection`) on that path still refuses with a narrowed message. The + alternatives (drop the spec silently, or advertise it and return a synthetic error) + both mislead. Recommendation: fail loud, sequence client tools with + `agent-client-tool-cleanup` / `mcp-client-tool-continuation`. + +3. **Is Codex-on-Daytona part of this feature's acceptance, or a follow-up?** The shim is + harness-agnostic by design, but the Codex ACP adapter's handling of stdio MCP entries is + unverified. Recommendation: Claude is the acceptance gate; Codex is a verification task + in slice 4, done when the Codex harness itself is in scope. + +4. **U2 timing: when do we explore Pi consuming the in-sandbox MCP server directly?** It + requires building an MCP client into the Pi extension (pi-acp drops `mcpServers`; that + is by design), adds a hop to the one path that works everywhere, and its real payoff is + user MCP on Pi. Recommendation: decide after Codex lands; U1's shared modules already + deliver the "one gateway-tool logic" goal at the module level. + +5. **Env contract: confirm reusing `AGENTA_AGENT_TOOLS_PUBLIC_SPECS` / + `AGENTA_AGENT_TOOLS_RELAY_DIR` for the shim's per-server env.** One public-spec contract + for every in-sandbox consumer, correcting #4873's parallel names. If the spec JSON ever + outgrows env limits (very large tool sets), the fallback is writing the specs to a file + next to the bundle and passing the path. Recommendation: reuse the names; note the file + fallback, do not build it yet. + +6. **Gate posture for future remote providers: confirm fail-closed stays.** After slice 2 + the refusal still fires for any non-Daytona remote provider (the in-flight E2B work + would need its own proven delivery before the gate opens for it). Recommendation: + confirm. + +7. **Snapshot bake timing.** Per-run upload ships first (about 5 kB, negligible). Bake into + `build_snapshot.py` once the path is hot, behind a skip flag mirroring the Pi install + flag. Recommendation: follow-up, not in the first PR. + +8. **User HTTP MCP, API-key-now: is the existing mechanism the answer?** Named secrets + already become request headers on the user's HTTP entry + (`services/runner/src/engines/sandbox_agent/mcp.ts:119`), SSRF-guarded, behind + `AGENTA_AGENT_MCPS_ENABLED` (default off). The decision "API key in a header for now" + appears to be already built; the only open item is when to flip the flag default, which + is the separate S2 work (#4912), not this project. OAuth for user MCP stays named future + work. Recommendation: confirm this reading so the policy in context.md is complete. diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/plan.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/plan.md new file mode 100644 index 0000000000..b4f849856e --- /dev/null +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/plan.md @@ -0,0 +1,297 @@ +# Plan + +## The design in one paragraph + +Ship a small, dependency-free MCP server bundle into the Daytona sandbox and advertise it to +the harness as an internal stdio MCP entry named `agenta-tools`. The harness's own ACP +adapter spawns it inside the sandbox at session creation; its `tools/list` serves the run's +public specs, and its `tools/call` writes a relay request file through the same relay-writer +module the Pi extension already uses. The runner's existing relay loop executes every call +server-side, behind the existing permission guard, with credentials that never enter the +sandbox. The handler and the relay writer become shared modules with a golden test pinning +the request file bytes, so Pi, local Claude, and the in-sandbox server are three transports +over one implementation. This revives PR #4873 onto today's code rather than building new. + +## Transport: reconciling A1, A2, and daemon-spawned into one choice + +Three candidates existed across the prior designs: + +- **A1, HTTP on the sandbox loopback** (recommended by `claude-daytona-tools/design.md`): + the runner uploads the bundle, starts it with `runProcess` on a fixed port, waits for + readiness, and advertises `http://127.0.0.1:/mcp`. +- **A2, stdio spawned by the harness** (implemented by PR #4873): the runner uploads the + bundle and advertises an ACP `McpServerStdio` entry `{name, command, args, env}`; the + harness's ACP adapter spawns it inside the sandbox and speaks newline-delimited JSON-RPC + over its stdin/stdout. +- **Daemon-spawned** (recommended by `remote-tools-delivery/specs.md` as candidate b1): + teach the sandbox-agent daemon a concept of "an extra in-sandbox MCP server attached to a + session". + +The daemon-spawned candidate collapses into A2: #4873 demonstrated that no daemon change is +needed, because the daemon already forwards `sessionInit.mcpServers` verbatim and the Claude +ACP adapter already spawns a stdio entry inside the sandbox. The daemon concept +`remote-tools-delivery` asked for exists; it is the harness adapter's own MCP-server list. + +Between A1 and A2, the decision is **A2**, flipping the earlier `claude-daytona-tools` +recommendation. Two facts changed since that document: + +1. **Warm sandbox reuse (PR #5225) makes a runner-managed long-lived process the wrong + shape.** An A1 shim must be health-checked when a parked sandbox resumes, restarted after + park-to-stopped (the VM stop killed it), and found and replaced when a tool-set change + forces a cold session inside a reused sandbox (a stale process on the fixed port serving + last turn's specs). Every one of those is a new failure mode with a live-QA cell. An A2 + shim inherits the session lifecycle instead: the adapter spawns it with the session's env + at session creation, so a new session always means a fresh shim with fresh specs, a + stopped VM cannot leave a stale one (the process died with the VM and respawns with the + session), and an orphan whose parent died exits on its own when stdin closes. +2. **A2 is already implemented and unit-tested** (#4873), including the ACP entry mapping + that was the main unknown when A1 was recommended ("stdio as fallback if port/readiness + is fiddly"). The port and readiness management A1 requires is exactly the part with no + existing code. + +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. + +Costs of A2, stated honestly: + +- **Stdio optics.** User stdio MCP is disabled, permanently. This entry is stdio too, but it + is synthesized by the runner from resolved tools, never user-declared, and it runs inside + the sandbox, not on the runner host. The implementation must keep the layers structurally + separate: the internal entry is built after the user-stdio gate has already run + (`run-plan.ts:341`), never flows through `toAcpMcpServers`, and a test pins that a + user-declared stdio server is still refused on the exact path that ships the shim. +- **Dependence on the ACP adapter's typeless-entry-to-stdio mapping.** Verified against the + adapter pin at #4873 time; re-verify against the current pin, and against Codex's ACP + adapter when that harness lands. If an adapter ever refuses stdio entries, A1 is the + documented fallback: the same bundle grows an HTTP mode (`tool-mcp-http.ts` relocated), + the runner starts it via `runProcess`, and the lifecycle work above becomes real. Nothing + in the shared modules is transport-specific, so the fallback swaps the outer loop only. + +## What runs where + +``` +runner (holds credentials, executes) sandbox (holds nothing secret) +------------------------------------ ------------------------------ +resolve tools -> public specs ----------------------> shim env (specs + relay dir) +upload shim bundle (daemon FS API) -----------------> /home/sandbox/.agenta/tool-mcp.js +advertise McpServerStdio "agenta-tools" -----------> harness ACP adapter spawns shim +startToolRelay polls relay dir <--------------------- shim writes .req.json +executeRelayedTool (guard, credentials) ------------> writes .res.json + shim reads res, answers tools/call +``` + +Local runs are unchanged: Claude on the local sandbox keeps the runner-loopback HTTP server +(which also carries the client-tool pause), and Pi keeps its extension everywhere. + +## Interfaces, by semantic role + +No `/run` wire change and no new protocol field. Every new element classified: + +- **Input (what to serve):** the public tool specs, carried to the shim as + `AGENTA_AGENT_TOOLS_PUBLIC_SPECS` in the stdio entry's per-server `env`. This is the exact + variable and shape the Pi extension reads (`pi-assets.ts:71`); #4873's parallel names are + dropped so the public-spec contract has one source of truth. +- **Routing (where calls go):** `AGENTA_AGENT_TOOLS_RELAY_DIR`, same reuse. +- **Protocol context:** the ACP `McpServerStdio` entry `{name, command, args, env}`. `name` + is `agenta-tools`, a stable identity coupled to the rendered permission rules + (`claude_settings.py:60`); `command`/`args` are `node` plus the in-sandbox bundle path. +- **Config (runner-side, operator-owned):** `SANDBOX_AGENT_RELAY_MCP_BUNDLE`, the bundle + location override (test and packaging use), defaulting to the esbuild output next to the + Pi extension bundle. Later, a snapshot-bake skip flag mirroring + `AGENTA_AGENT_SANDBOX_PI_INSTALLED`. +- **Credentials:** none, anywhere in this design. The shim has no credential field because + execution stays where the credentials are. The entry's `headers` concept does not exist + for stdio; nothing rides `env` except the two public variables above. +- **The relay file protocol:** unchanged, and now golden-pinned as the stable contract + between any in-sandbox writer and the runner loop. + +## Unification with Pi + +The owner's priority. Today the "turn a model's tool call into a relay request" logic exists +in two call sites that both route through one writer (`runResolvedTool` -> +`relayToolCall`), plus an MCP message handler that exists once (`tool-mcp-http.ts:101`) and +was duplicated by #4873. The unification target is: one writer, one handler, three thin +transports. + +**U1 (do now): shared modules.** Extract two pieces: + +1. `tools/relay-client.ts`: the relay writer (`relayToolCall` plus its wait loop), moved + out of `dispatch.ts` as #4873 did, with `dispatch.ts` re-exporting so existing call sites + (the Pi extension, local Claude) are unchanged. It must bundle with zero non-relay code + and honor the per-tool `timeoutMs` and an abort signal. +2. `tools/mcp-handler.ts`: the transport-neutral MCP message handler (initialize, + tools/list with the shared schema accessor, tools/call, notifications, errors), factored + from `tool-mcp-http.ts`, parameterized by an "execute" function and an optional + client-tool pause hook. The HTTP server keeps its socket-abort pause; the stdio shim + passes no pause hook in slice 1 (client tools are not delivered there yet). + +The golden test pins the request file: the Pi extension path and the shim path, given the +same call, produce byte-identical `.req.json` content. That single test is what keeps "one +gateway-tool logic" true over time. + +**U3 (do now, nearly free): one build pipeline.** The shim bundles in the same esbuild step +as the Pi extension, into `dist/`, baked into the runner image the same way. Same packaging, +same upload helper pattern, same snapshot-bake story later. + +**U2 (explore later, not now): Pi consumes the in-sandbox MCP server directly.** The idea: +the Pi extension stops reading specs from env and instead dials the shim, lists its tools, +and registers a forwarding `registerTool` for each; eventually the extension is a generic +MCP client and user HTTP MCP could reach Pi through the same code. Honest trade-offs: + +- Pi has no MCP client by design (pi-acp drops `mcpServers`), so this is new client code in + the extension, not configuration. +- It adds a process and a hop to the one path that currently works on every backend, for no + functional gain today: the extension already shares the writer and (after U1) would share + nothing further by speaking MCP, because MCP is the part Pi does not need. +- Its real payoff is a different feature: user MCP on Pi (the open F-009 question) and + retiring the env-var spec channel. Both are worth a decision when Codex lands and the + MCP-client population grows, not before. + +Recommendation: land U1+U3 now; write U2 up as a follow-up decision for the owner (see +[open-questions.md](open-questions.md)). After U1, "one gateway-tool logic" is concretely +true at the module level: both harness families execute tool calls through the same handler +semantics and the same writer bytes, verified by the golden. + +## Lifecycle + +- **Startup and readiness.** No race exists: the ACP adapter spawns the shim and completes + the MCP handshake synchronously before the session prompt runs. A shim that fails to start + (missing bundle, bad node) surfaces as an MCP server failure in the harness, and the + fail-loud upload helper already refuses the run earlier if the bundle cannot be delivered. +- **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. +- **Teardown, ephemeral delete.** The shim dies with the sandbox. Nothing to do. +- **Warm reuse, park-to-running.** The harness session stays alive, so the shim stays alive + with it, still serving the specs that session was created with. Correct by construction: + the keep-alive fingerprint includes `customTools` (`session-pool.ts:170`), so a live + session is only continued when the tool set is unchanged. +- **Warm reuse, park-to-stopped.** The VM stop kills the harness and the shim. The next turn + restarts the sandbox and builds or loads a session; the adapter respawns the shim from the + session's MCP config. The bundle file survives on the sandbox filesystem; the upload + helper can skip an unchanged existing file as an optimization. +- **Tool-set change between turns.** The fingerprint mismatch forces a cold session in the + reused sandbox. The old session is destroyed (`destroySession`, + `engines/sandbox_agent.ts:829-832`), which ends the old shim (stdin closes, the readline + loop ends, the process exits). The new session spawns a fresh shim with the new specs in + its env. No fixed port means no collision window. +- **Spec freshness invariant, stated once:** the shim's spec list is immutable per process, + and a shim process never outlives the session that spawned it. Everything above is that + invariant applied to each reuse case, and the live-QA matrix checks each case. + +## Client tools: sequenced after, not in, the first slice + +Client tools are advertised-and-paused on the local HTTP channel today (`MCP_PAUSED` +aborts the in-flight request so no result settles). Through the in-sandbox shim the shape is +different: the relay loop parks the call and writes no response file (`relay.ts:249-252`), +so the shim's wait would hang until the relay timeout and return an error to the model, +which is exactly the park-must-emit-no-result problem the client-tool continuation work +exists to solve (a parked call must produce no tool result, and the resumed turn must settle +the original call). + +Sequencing, matching the prior designs: **slice 1 delivers executable (gateway/callback) +tools only.** A run that carries a client tool on the Claude+Daytona path keeps failing loud +with a narrowed, honest message (options considered: silently dropping client specs repeats +the F-032 silent-drop bug; advertising them and returning a synthetic error teaches the +model the tool is broken). When the continuation work lands, the shim inherits the pause +semantics by adding the pause hook to the shared handler, and the relay response protocol +gains whatever the continuation design chooses; that is deliberately not designed here. + +## Gate change + +`REMOTE_TOOLS_UNSUPPORTED_MESSAGE` narrows instead of disappearing. After slice 2 the refusal +fires only when: the harness is MCP-capable but the remote provider is not Daytona (fail +closed per provider until proven), or the run carries a client tool (until the continuation +work). The message text updates to say what is and is not supported and to point at this +workspace. The capability gate (`assertRequiredCapabilities`) is unchanged: Claude truthfully +advertises `mcpTools`, and now the advertisement is true on Daytona too. + +## Security + +The invariant, restated: **the sandbox sees public specs and a relay directory, nothing +else; every credentialed action executes runner-side; the shim opens no network surface.** + +- No credential enters the sandbox: the shim env carries specs and a path. Private spec + fields never leave runner memory (unchanged, `public-spec.ts`). +- No listener: stdio only. The loopback-only rule of the HTTP variant becomes "no socket at + all". +- The user-stdio disable is not relaxed. The internal entry is synthesized downstream of the + user gates, shares no constant and no code path with `toAcpMcpServers`' stdio branch, and + a layering test pins: user stdio still refused, user HTTP still delivered, internal entry + present, on the same Daytona run. +- The relay directory remains an in-sandbox capability: any sandbox process can write a + request file. That is the accepted, pre-existing Pi posture, and the runner-side + permission guard (`relay.ts:105`) re-checks every executable call, so a forged file cannot + run a denied tool. The shim adds a second writer, not a second trust level. +- Strict-network runs with executable tools stay refused (`run-plan.ts:368`): execution + still happens on the runner, outside the sandbox egress boundary. Unchanged. + +## Coordination with event-driven-tool-relay + +The sibling project replaces relay polling with filesystem-event wakeups. Two contact +points, agreed on paper: + +1. **The file contract is shared and golden-pinned.** Request and response names, bytes, and + delete-after-read semantics do not change in either project. The golden test in slice 1 + is the enforcement. +2. **The shim's response wait is one small function.** `relay-client.ts` isolates "wait for + `.res.json`" behind a single function with a timeout and an abort signal, currently + implemented as the existing poll. The sibling swaps its internals for `fs.watch` without + touching the handler or the writer. Nothing else in this project depends on how the wait + is implemented. + +## Packaging + +Per-run upload first: `writeFsFile` the bundle (about 5 kB) via the fail-loud helper, +mirroring the Pi extension upload. Cold-start cost is one small FS write, negligible next to +the session create it rides along with, and the file persists across warm reuse. Snapshot +bake is a follow-up optimization in `build_snapshot.py` (same pattern as the pinned `pi` +install) with an env flag to skip the upload, worth doing once the path is hot; it changes +no licensing posture because the shim is Agenta code. + +## Slices + +**Slice 1: shared modules, no behavior change.** +Extract `tools/relay-client.ts` (writer + wait, timeout + signal) and `tools/mcp-handler.ts` +(transport-neutral handler) from `dispatch.ts`/`tool-mcp-http.ts`; re-export so all call +sites compile unchanged. Add the golden test pinning the request file bytes across the Pi +path and the handler path. All existing runner tests stay green. +Acceptance: zero behavior diff; golden committed. + +**Slice 2: the shim, Daytona delivery, gate narrowing.** +Revive #4873 onto today's paths: the stdio entry bundle (thin loop over the shared handler + +writer), the esbuild step, the fail-loud upload helper, the `mcp.ts` Daytona non-Pi branch +building the internal `McpServerStdio` entry, engine wiring next to +`prepareDaytonaPiAssets`, and the narrowed run-plan gate (executable tools pass on Daytona +for MCP-capable harnesses; client tools and non-Daytona remotes still refuse loud). Unit +tests: handler over stdio framing, upload fail-loud, layering (user stdio refused / user +HTTP delivered / internal entry present), gate matrix. +Acceptance: runner tests + typecheck green; a fake-daemon integration test drives +shim -> relay dir -> `startToolRelay(localRelayHost())` -> mocked callback and asserts the +round trip. + +**Slice 3: live QA and the replay pin.** +The matrix cell that has never been green: Claude + Daytona + gateway tool (github via the +`pi-agents` project, which holds live Composio connections), asserting the tool executes and +the result reaches the answer. Negatives: Claude+Daytona with no tools still runs (no shim +uploaded); Pi+Daytona unchanged; Claude+local unchanged. Warm-reuse cells: second turn +within the idle window (live session, same shim), second turn after park-to-stopped +(restart, respawn), tool-set change between turns (cold session in the reused sandbox, +fresh shim). Network-off cell: gateway tool executes with `network` restricted + +`best_effort` (relay is file I/O). Capture one green run and pin it with the +agent-replay-test recipe so the path regression-tests without a live LLM. Sandbox hygiene: +cheap model, verify the park/delete reaps everything. +Acceptance: matrix recorded in this workspace; replay test committed. + +**Slice 4 (follow-ups, each its own decision):** client tools through the shim (after the +continuation work), snapshot bake, Codex ACP adapter verification, the U2 exploration +(Pi as an MCP client), and adopting the sibling's watch-based wait. + +## Effort + +Slices 1+2 are roughly two focused days (most code exists in #4873); slice 3 is one day +dominated by live QA. Risk is low: no wire change, no new network surface, local paths +untouched, and the one novel dependency (the adapter's stdio mapping) was already proven +once and is re-verified before merge. diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/research.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/research.md new file mode 100644 index 0000000000..854f84b931 --- /dev/null +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/research.md @@ -0,0 +1,178 @@ +# Research + +Verified against the working tree on 2026-07-11 (`gitbutler/workspace`, runner at +`services/runner/`). Every claim carries a file and line anchor. The full option analysis +lives in [../claude-daytona-tools/](../claude-daytona-tools/README.md) and +[../remote-tools-delivery/specs.md](../remote-tools-delivery/specs.md); this file records +the current state, what changed since those were written, and the PR #4873 autopsy. + +## The refusal today + +`buildRunPlan` refuses any run where the harness is not Pi, the sandbox is remote, and the +run carries any custom tool (`run-plan.ts:355`), with `REMOTE_TOOLS_UNSUPPORTED_MESSAGE` +(`run-plan.ts:64`). The gate keys on "not local" rather than "is Daytona" so a future remote +provider fails closed too (`run-plan.ts:281`). It counts client tools as well as executable +ones (`run-plan.ts:351-354`). Downstream, `buildSessionMcpServers` skips the internal channel +on Daytona and logs that run-plan should have refused the run (`engines/sandbox_agent/mcp.ts:245-264`). + +## One execution path, two front-ends + +Execution is solved and shared. `startToolRelay` (`tools/relay.ts:322`) polls the relay +directory through a pluggable host: local filesystem (`relay.ts:182`) or the Daytona daemon +FS API (`sandboxRelayHost`, `relay.ts:197`). Each request is executed against the private +spec in runner memory (`executeRelayedTool`, `relay.ts:226`): client tools pause through the +shared client-tool relay and write no response file (`relay.ts:249-252`), everything else is +re-checked by the runner-side permission guard (`RelayExecutionGuard`, `relay.ts:105`, +wired by the engine at `engines/sandbox_agent.ts:1762`) and then dispatched. The file +contract: request `{toolName, toolCallId, args}` written by `relayToolCall` +(`tools/dispatch.ts:78-82`), response `{ok, text?, error?}` (`relay.ts:90-96`), suffixes +`.req.json`/`.res.json` (`relay.ts:51-52`), writer deletes both files after reading the +response (`dispatch.ts:92-100`). + +Front-end 1, Pi (works everywhere): the bundled extension (`extensions/agenta.ts:280` +`registerTools`) reads `AGENTA_AGENT_TOOLS_PUBLIC_SPECS` and `AGENTA_AGENT_TOOLS_RELAY_DIR` +from env (set by `buildPiExtensionEnv`, `engines/sandbox_agent/pi-assets.ts:71-72`) and +registers each public spec as a Pi tool whose `execute` calls +`runResolvedTool(spec, params, {toolCallId, relayDir, signal})` (`extensions/agenta.ts:341`). +With `relayDir` set, `runResolvedTool` routes to `relayToolCall` (`dispatch.ts:149-158`), +the shared relay writer. The extension is esbuild-bundled (`pi-assets.ts:25`, +`EXTENSION_BUNDLE`) and uploaded per run (`uploadPiExtensionToSandbox`, `pi-assets.ts:178`). + +Front-end 2, Claude local only: `startInternalToolMcpServer` (`tools/tool-mcp-http.ts:271`) +is a dependency-free JSON-RPC-over-HTTP MCP server (only `node:http`/`node:crypto`), bound +to `127.0.0.1` (`tool-mcp-http.ts:50`) on an OS-assigned port (`:399`). `tools/list` serves +every public spec including client tools (`:126-149`, reading schemas through the shared +`specInputSchema` accessor that handles both `inputSchema` and `input_schema`); `tools/call` +routes non-client tools to the same `runResolvedTool` relay write (`:210-213`) and pauses a +client tool by aborting the in-flight HTTP request with no body (the `MCP_PAUSED` sentinel, +`:62`, `:190-192`). `buildToolMcpServers` (`tools/mcp-bridge.ts:97`) wraps it in a +`type: "http"` ACP entry named `agenta-tools` (`mcp-bridge.ts:119-127`). On Daytona that URL +would be the sandbox's own loopback, so the channel is skipped +(`engines/sandbox_agent/mcp.ts:245-251`). + +The missing piece is exactly one thing: an in-sandbox process that speaks MCP and writes +relay request files. Pi has one; MCP-client harnesses have none. + +## Contracts the design must not break + +- **Server name `agenta-tools`.** The Python Claude adapter renders per-tool permission + rules as `mcp__agenta-tools__` into `.claude/settings.json` + (`sdks/python/agenta/sdk/agents/adapters/claude_settings.py:60`, `:175`). The in-sandbox + server must keep the name or every rendered allow/deny rule silently stops matching. +- **The relay request file bytes.** Both writers must emit byte-identical + `{"toolName":...,"toolCallId":...,"args":...}` JSON so one relay loop serves both. Today + there is exactly one writer implementation (`relayToolCall`, `dispatch.ts:62`); the design + keeps it that way and pins it with a golden test. +- **The public-spec env contract.** `AGENTA_AGENT_TOOLS_PUBLIC_SPECS` (a JSON array of + `AdvertisedToolSpec`: name, description, inputSchema, kind, render, timeoutMs, + `tools/public-spec.ts:12-19`) and `AGENTA_AGENT_TOOLS_RELAY_DIR`. One contract, one pair + of names. +- **No wire change.** `/run` (`protocol.ts`) and its Python mirror stay untouched; the shim + is runner-internal delivery. + +## Warm sandbox reuse (PR #5225) and what it means for a shim + +Facts, verified: + +- After a clean turn a Daytona sandbox parks instead of being deleted: `pauseSandbox` at + `engines/sandbox_agent.ts:838-843`; only a failed park falls through to delete (`:851`). + Park-to-running keeps every process alive; park-to-stopped stops the VM, which kills every + process, and the next turn restarts the same instance. +- The keep-alive config fingerprint includes `customTools` and `mcpServers` + (`engines/sandbox_agent/session-pool.ts:154`, `:170-171`). A live session continues only + when the tool set is unchanged; a changed tool set forces a cold session build, possibly + inside the same reused sandbox. +- The relay directory is derived from the durable cwd (`run-plan.ts:388-391`), so it is the + same path across turns of one conversation. +- The internal MCP server's specs are fixed at session creation + (`sessionInit.mcpServers`, `engines/sandbox_agent.ts:1275`); the engine closes it in + `destroy` (`:826`). + +Implications: a long-lived runner-started shim process (the A1 shape) must be found and +killed or replaced on cold rebuild in a reused sandbox (stale specs on a fixed port), must be +restarted after park-to-stopped, and must be health-checked on park-to-running resume. A +harness-spawned shim (the A2 shape) inherits the session lifecycle instead: the harness +spawns it with the session's env at session creation and it dies with the session. This is +the strongest new fact since the prior designs were written, and it flips the transport +recommendation (see [plan.md](plan.md)). + +One pre-existing residue risk, shared with Pi: a relay directory reused across turns can hold +an orphaned `.req.json` from a crashed turn, and the next turn's fresh relay loop (whose +seen-set is per turn, `relay.ts:332`) would execute it. This is not new with the shim; it is +flagged to the [../event-driven-tool-relay/](../event-driven-tool-relay/README.md) sibling, +which owns relay mechanics. + +## Daemon and ACP facts the transport choice depends on + +- The daemon forwards `sessionInit.mcpServers` verbatim into the in-sandbox `newSession`; + the Claude ACP adapter (`@zed-industries/claude-agent-acp`) maps a typeless + `{name, command, args, env}` entry to a Claude SDK `{type: "stdio", ...}` MCP server and + the Claude Agent SDK launches it inside the sandbox over newline-delimited JSON-RPC. + Verified by PR #4873 against the then-bundled adapter; must be re-verified against the + current pin during implementation. +- pi-acp accepts `mcpServers` in session init but drops it: Pi has no MCP client by design. + Its extension `registerTool` is the bridge. Any "Pi consumes the MCP server directly" idea + therefore requires new client code inside the Pi extension, not configuration. +- The only host-to-sandbox primitives are the daemon FS API and `runProcess` + (`relay.ts:197-220`, `daytona.ts:50-75`). There is no runner-to-sandbox port forward and + no sandbox-to-runner network path. The file relay is the only back-channel, and it works + with sandbox networking fully disabled. +- The Daytona snapshot ships node (the Claude CLI is node, + `services/runner/sandbox-images/daytona/build_snapshot.py:96`), so a node shim needs no + new runtime. The snapshot build script is where a bake would go (same pattern as the + pinned `pi` install, `build_snapshot.py:94`). + +## PR #4873 autopsy: what it built, what to reuse, why it went stale + +PR #4873 ("deliver Claude gateway tools on Daytona via an in-sandbox stdio MCP relay shim", +opened 2026-06-26, closed unmerged 2026-07-05) implemented the stdio variant end to end: + +- `src/tools/relay-mcp-stdio.ts`: a 229-line stdio MCP server. Newline-delimited JSON-RPC + loop, `initialize`/`tools/list`/`tools/call` handler mirroring `tool-mcp-http.ts`, + `tools/call` writing through the relay client, stderr-only logging, fail-loud exit when + the relay dir env is missing, concurrent in-flight calls with atomic line writes. +- `src/tools/relay-client.ts`: `relayToolCall` factored out of `dispatch.ts` so the bundle + carries only file-relay code, no callback executor. Bundle: 5.3 kB, zero network calls. +- `src/engines/sandbox_agent/relay-shim.ts`: bundle path resolution + (`SANDBOX_AGENT_RELAY_MCP_BUNDLE` override) plus a fail-loud upload helper that throws a + named error when the bundle is missing or the upload fails. +- `mcp.ts`: the Daytona non-Pi branch builds an ACP `McpServerStdio` entry instead of + skipping; `sandbox_agent.ts` uploads the shim on that path; the esbuild script bundles the + shim next to the Pi extension. +- 16 unit tests: the handler, the stdio session entry, the fail-loud upload, and the + no-shim/Pi/client-only branches. + +Why it never merged, and what a straight rebase would miss: + +1. **The repo moved under it.** Every path is pre-rename (`services/agent/`, now + `services/runner/`), and the engine was refactored around it. +2. **It was never live-verified.** The PR's own gate was "do not merge until a real + Claude+Daytona+callback run is green," and Daytona credentials/credit blocked that at the + time. The urgency then dropped when the fail-loud gate (#5047) shipped separately. +3. **The internal channel evolved past its handler.** Since #4873: client tools are + advertised and paused on the channel (`MCP_PAUSED`, `tool-mcp-http.ts:62`), the + snake-case `input_schema` accessor fixed empty schemas for platform-catalog tools + (`tool-mcp-http.ts:140-146`), the runner-side relay permission guard landed + (`relay.ts:105`), and idle-backoff changed relay polling (`relay.ts:68-80`). #4873's + shim duplicated the handler instead of sharing it, so none of those fixes reach it. +4. **Small divergences to correct on revival:** it introduced parallel env var names + (`AGENTA_TOOL_PUBLIC_SPECS`/`AGENTA_TOOL_RELAY_DIR`) instead of reusing the Pi + extension's `AGENTA_AGENT_TOOLS_*` pair, and its `relayToolCall` call dropped the + per-tool `timeoutMs` budget that the public spec now carries. +5. **Warm reuse did not exist yet.** #4873 assumed a single-run ephemeral sandbox. The + lifecycle analysis in [plan.md](plan.md) is new. + +The verdict: the architecture and most of the code are sound and reviewed (Codex xhigh at +the time); the revival re-homes it onto today's paths, shares the handler instead of copying +it, fixes the env names and timeout, and adds the reuse lifecycle and live QA that were +missing. + +## Constraints carried over (summary; full list in prior art) + +From `claude-daytona-tools/research.md`, still true and re-verified: no credential may cross +into the sandbox (public specs only, `tool-mcp-http.ts:131-135`); the user stdio MCP disable +is about processes on the runner host (`run-plan.ts:337-343`, `mcp.ts:148-155`) and must not +be relaxed by whatever spawns the shim; the SSRF guard shapes why sandbox-reachable runner +URLs are the wrong direction (`mcp.ts:62-96`); strict-network runs with executable tools are +refused on Daytona because execution happens runner-side (`run-plan.ts:368-379`), and this +project does not change that. diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md new file mode 100644 index 0000000000..e21fe04335 --- /dev/null +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md @@ -0,0 +1,45 @@ +# Status + +**State: DESIGN ONLY, awaiting owner review.** No runtime code changed. The PR carrying this +workspace is the review surface; the owner will interview on it. + +## Done + +- 2026-07-11: workspace created. Prior art read and reconciled (`claude-daytona-tools`, + `remote-tools-delivery`, `mcp-delivery-architecture` including the 2026-07-11 decision in + `gateway-mcp-location.md`, `gateway-tool-mcp`). PR #4873 mined; its stdio implementation + is the revival base. Current code re-verified against the working tree (anchors in + [research.md](research.md)), including the warm-reuse lifecycle from PR #5225 and the + client-tool pause semantics on the local channel. +- Owner decisions of 2026-07-11 encoded in [context.md](context.md): runner-only sandbox + communication (API gateway rejected), user MCP HTTP-only permanently with API-key-header + auth now and OAuth as future work, platform tools via an in-sandbox MCP server, and + unification with Pi as the primary design goal. +- Recommendation written in [plan.md](plan.md): A2 (harness-spawned stdio shim) as the one + transport, shared handler + relay-writer modules with a golden byte-contract test as the + unification path, slices 1-4, and the live-QA matrix including the warm-reuse cells. + +## Next + +1. Owner review of this workspace (interview against + [open-questions.md](open-questions.md), especially the A1-to-A2 flip and the client-tool + sequencing). +2. On approval: implement slice 1 (shared modules + golden), then slice 2 (#4873 revival), + then slice 3 (live QA + replay pin). Coordinate lane usage on the agent board; check the + `event-driven-tool-relay` sibling's state before touching `relay-client.ts` internals. + +## Blockers + +None for the design. Implementation slice 3 needs Daytona credit and the `pi-agents` +project's live Composio connections (the same live-QA prerequisites recorded in +`claude-daytona-tools/design.md`). + +## Decision log + +- 2026-07-11: transport recommendation flipped from A1 (HTTP loopback, + `claude-daytona-tools`) to A2 (harness-spawned stdio, PR #4873), driven by the warm-reuse + lifecycle (PR #5225) and the existence of tested A2 code. Recorded in + [plan.md](plan.md); awaiting owner confirmation (open question 1). +- 2026-07-11: the daemon-spawned variant from `remote-tools-delivery` recognized as the + same mechanism as A2 (the daemon already forwards session MCP entries; the adapter + spawns them), so no daemon change is requested from the sandbox-agent package. From 7616d6971ec15ef0766f31306bff0f8bfdf60aee Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 11 Jul 2026 22:59:26 +0200 Subject: [PATCH 2/4] docs(design): fold cross-consistency review into the in-sandbox tool 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 --- .../projects/in-sandbox-tool-mcp/plan.md | 61 ++++++++++++++++--- .../projects/in-sandbox-tool-mcp/research.md | 26 +++++--- .../projects/in-sandbox-tool-mcp/status.md | 10 +++ 3 files changed, 79 insertions(+), 18 deletions(-) diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/plan.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/plan.md index b4f849856e..850832d998 100644 --- a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/plan.md +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/plan.md @@ -119,13 +119,26 @@ transports. 1. `tools/relay-client.ts`: the relay writer (`relayToolCall` plus its wait loop), moved out of `dispatch.ts` as #4873 did, with `dispatch.ts` re-exporting so existing call sites (the Pi extension, local Claude) are unchanged. It must bundle with zero non-relay code - and honor the per-tool `timeoutMs` and an abort signal. + and honor the per-tool `timeoutMs` and an abort signal. The response wait is one exported + function with the contract `waitForRelayResponse(resPath, { timeoutMs, signal })` (final + name at implementation time), implemented today as the existing 300 ms poll. That + function is the seam the `event-driven-tool-relay` sibling swaps for `fs.watch`; see the + coordination section below for the landing order. 2. `tools/mcp-handler.ts`: the transport-neutral MCP message handler (initialize, tools/list with the shared schema accessor, tools/call, notifications, errors), factored from `tool-mcp-http.ts`, parameterized by an "execute" function and an optional client-tool pause hook. The HTTP server keeps its socket-abort pause; the stdio shim passes no pause hook in slice 1 (client tools are not delivered there yet). + Layering rule, stated once: `mcp-handler.ts` stays credential-free. The per-environment + bearer that `mcp-client-tool-continuation` WP1 adds + ([../mcp-client-tool-continuation/plan.md](../mcp-client-tool-continuation/plan.md)) + lives in the HTTP transport wrapper in `tool-mcp-http.ts`, never in the shared handler, + so the stdio shim (a harness-spawned child process with no listener) inherits no auth + requirement. Ordering with that project: this slice 1 lands first, and its WP1 (auth, + client-tool batch rejection) and WP3 (register-before-pause hold-open) then build on the + extracted handler; the optional pause hook here is WP3's insertion point. + The golden test pins the request file: the Pi extension path and the shim path, given the same call, produce byte-identical `.req.json` content. That single test is what keeps "one gateway-tool logic" true over time. @@ -162,7 +175,14 @@ semantics and the same writer bytes, verified by the golden. - **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. + because the runner never owned the process. One case deserves its own statement: if the + shim dies after writing `.req.json` but before reading the response, the runner still + executes the call (side effects happen) and writes a `.res.json` nobody consumes, while + the model sees an MCP failure. The relay is at-least-once from the executor's point of + view, and this property is shared with Pi's writer today (an aborted or timed-out wait + after the request file is written behaves the same). The orphaned response file is inert + (the runner loop lists only `.req.json`) and the workspace preparation clears it on the + next cold build. Slice 2's integration tests must cover crash-after-write. - **Teardown, ephemeral delete.** The shim dies with the sandbox. Nothing to do. - **Warm reuse, park-to-running.** The harness session stays alive, so the shim stays alive with it, still serving the specs that session was created with. Correct by construction: @@ -199,6 +219,16 @@ model the tool is broken). When the continuation work lands, the shim inherits t semantics by adding the pause hook to the shared handler, and the relay response protocol gains whatever the continuation design chooses; that is deliberately not designed here. +Ownership, stated plainly so the gap cannot hide: neither this project nor +[../mcp-client-tool-continuation/](../mcp-client-tool-continuation/README.md) designs the +Daytona client-tool bridge. That bridge needs a relay park protocol (a paused call must +produce no result until the browser answers, but the shim's wait times out at 60 s today), +a stdio analogue for abort-without-result (`res.destroy()` does not exist for a spawned +child), and an answer to the race between a held browser wait and the five-minute Daytona +auto-stop that kills the shim on park-to-stopped. The recommendation to open one owned +workspace for it lives in +[../mcp-delivery-architecture/orchestration.md](../mcp-delivery-architecture/orchestration.md). + ## Gate change `REMOTE_TOOLS_UNSUPPORTED_MESSAGE` narrows instead of disappearing. After slice 2 the refusal @@ -230,17 +260,26 @@ else; every credentialed action executes runner-side; the shim opens no network ## Coordination with event-driven-tool-relay -The sibling project replaces relay polling with filesystem-event wakeups. Two contact -points, agreed on paper: +The sibling project replaces relay polling with filesystem-event wakeups. Three contact +points: 1. **The file contract is shared and golden-pinned.** Request and response names, bytes, and delete-after-read semantics do not change in either project. The golden test in slice 1 is the enforcement. -2. **The shim's response wait is one small function.** `relay-client.ts` isolates "wait for - `.res.json`" behind a single function with a timeout and an abort signal, currently - implemented as the existing poll. The sibling swaps its internals for `fs.watch` without - touching the handler or the writer. Nothing else in this project depends on how the wait - is implemented. +2. **The shim's response wait is one small function, and this project creates it.** + `relay-client.ts` isolates "wait for `.res.json`" behind + `waitForRelayResponse(resPath, { timeoutMs, signal })`, currently implemented as the + existing poll. The sibling swaps its internals for `fs.watch` without touching the + handler or the writer. Landing order: this project's slice 1 lands first, so the + sibling's in-sandbox watch goes into that function rather than into `dispatch.ts`. The + sibling's plan currently names `relayToolCall` in `dispatch.ts` as its seam and does not + name `relay-client.ts`; that pending alignment is recorded in + [../mcp-delivery-architecture/orchestration.md](../mcp-delivery-architecture/orchestration.md). + Nothing else in this project depends on how the wait is implemented. +3. **The orphaned-request residue across warm-continued turns** (see + [research.md](research.md)) needs an owner. The sibling owns relay mechanics but its + current plan disclaims this property; the ownership decision is tracked in the same + orchestration file. ## Packaging @@ -280,7 +319,9 @@ uploaded); Pi+Daytona unchanged; Claude+local unchanged. Warm-reuse cells: secon within the idle window (live session, same shim), second turn after park-to-stopped (restart, respawn), tool-set change between turns (cold session in the reused sandbox, fresh shim). Network-off cell: gateway tool executes with `network` restricted + -`best_effort` (relay is file I/O). Capture one green run and pin it with the +`best_effort` (relay is file I/O; `best_effort` is required because the strict-network +refusal for executable tools at `run-plan.ts:368` stays, unchanged by this project, and +only an explicit `best_effort` opts out of it). Capture one green run and pin it with the agent-replay-test recipe so the path regression-tests without a live LLM. Sandbox hygiene: cheap model, verify the park/delete reaps everything. Acceptance: matrix recorded in this workspace; replay test committed. diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/research.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/research.md index 854f84b931..b9e54a19dc 100644 --- a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/research.md +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/research.md @@ -17,7 +17,7 @@ on Daytona and logs that run-plan should have refused the run (`engines/sandbox_ ## One execution path, two front-ends -Execution is solved and shared. `startToolRelay` (`tools/relay.ts:322`) polls the relay +Execution is solved and shared. `startToolRelay` (`tools/relay.ts:324`) polls the relay directory through a pluggable host: local filesystem (`relay.ts:182`) or the Daytona daemon FS API (`sandboxRelayHost`, `relay.ts:197`). Each request is executed against the private spec in runner memory (`executeRelayedTool`, `relay.ts:226`): client tools pause through the @@ -82,8 +82,12 @@ Facts, verified: (`engines/sandbox_agent/session-pool.ts:154`, `:170-171`). A live session continues only when the tool set is unchanged; a changed tool set forces a cold session build, possibly inside the same reused sandbox. -- The relay directory is derived from the durable cwd (`run-plan.ts:388-391`), so it is the - same path across turns of one conversation. +- The relay directory is deliberately kept OFF the durable cwd: an ephemeral base + (`/home/sandbox/agenta/relay` on Daytona, `$TMPDIR/agenta/relay` locally) keyed by + `basename(cwd)` (`run-plan.ts:384-391`), so relay I/O never rides the geesefs mount (a + flaky mount surfaces as ENOTCONN on relay files; the comment there says exactly this). + Because the key comes from the durable cwd's basename, the path is stable across turns of + one conversation. - The internal MCP server's specs are fixed at session creation (`sessionInit.mcpServers`, `engines/sandbox_agent.ts:1275`); the engine closes it in `destroy` (`:826`). @@ -96,11 +100,17 @@ spawns it with the session's env at session creation and it dies with the sessio the strongest new fact since the prior designs were written, and it flips the transport recommendation (see [plan.md](plan.md)). -One pre-existing residue risk, shared with Pi: a relay directory reused across turns can hold -an orphaned `.req.json` from a crashed turn, and the next turn's fresh relay loop (whose -seen-set is per turn, `relay.ts:332`) would execute it. This is not new with the shim; it is -flagged to the [../event-driven-tool-relay/](../event-driven-tool-relay/README.md) sibling, -which owns relay mechanics. +One pre-existing residue risk, shared with Pi, stated precisely: workspace preparation +already clears the relay dir on Daytona before each environment build (`rm -rf` at +`workspace.ts:60-66`, with a comment giving exactly this rationale), so a cold build cannot +replay a crashed turn's orphaned `.req.json`. The residual window is the warm-continued +turn: a checked-out keep-alive session skips `prepareWorkspace`, and each turn starts a +fresh relay loop with a fresh per-turn seen-set (`relay.ts:334`), so an orphan from a +crashed turn inside one live environment can still be re-executed on the next turn. This is +not new with the shim. The [../event-driven-tool-relay/](../event-driven-tool-relay/README.md) +sibling owns relay mechanics, but its current plan explicitly does not change this property; +the ownership question is recorded in +[../mcp-delivery-architecture/orchestration.md](../mcp-delivery-architecture/orchestration.md). ## Daemon and ACP facts the transport choice depends on diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md index e21fe04335..b5bf965288 100644 --- a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md @@ -18,6 +18,16 @@ workspace is the review surface; the owner will interview on it. - Recommendation written in [plan.md](plan.md): A2 (harness-spawned stdio shim) as the one transport, shared handler + relay-writer modules with a golden byte-contract test as the unification path, slices 1-4, and the live-QA matrix including the warm-reuse cells. +- 2026-07-11: cross-consistency review round (requested by the owner alongside his own + review of the event-driven-tool-relay PR) folded in. Corrections: the relay dir is an + ephemeral sibling keyed by `basename(cwd)`, not derived from the durable cwd; the + orphaned-request risk narrowed to warm-continued turns (`workspace.ts:60-66` already + clears cold builds). Additions: the `waitForRelayResponse` seam contract and landing + order with the sibling, the bearer layering rule and ordering with + `mcp-client-tool-continuation`, the unowned Daytona client-tool bridge cross-reference, + the crash-after-write at-least-once note, and the `best_effort` clarification on the + network-off QA cell. Combined landing order: + [../mcp-delivery-architecture/orchestration.md](../mcp-delivery-architecture/orchestration.md). ## Next From d2832ea2c6413b9f46c266b3c0f9ec63a8a9a668 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 11 Jul 2026 23:18:27 +0200 Subject: [PATCH 3/4] docs(design): adopt the Codex review - conditional A2, consume #5232 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 --- .../projects/in-sandbox-tool-mcp/README.md | 5 +- .../projects/in-sandbox-tool-mcp/context.md | 4 +- .../in-sandbox-tool-mcp/open-questions.md | 71 ++- .../projects/in-sandbox-tool-mcp/plan.md | 453 +++++++++--------- .../projects/in-sandbox-tool-mcp/research.md | 29 +- .../projects/in-sandbox-tool-mcp/status.md | 43 +- 6 files changed, 322 insertions(+), 283 deletions(-) diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/README.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/README.md index 48b9bc6fbb..5fb04ec1de 100644 --- a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/README.md +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/README.md @@ -8,8 +8,9 @@ server-side. Today that combination gets zero tools and the run is refused up fr `REMOTE_TOOLS_UNSUPPORTED_MESSAGE`). The design goal the owner cares about most is unification with Pi: one gateway-tool code -path serving both the Pi extension and the new MCP server, pinned by a golden test so the -relay protocol cannot fork per harness. +path serving both the Pi extension and the new MCP server. That shared path is the relay +client and file protocol that PR #5232 (event-driven-tool-relay) extracts as its slice 0; +this project consumes those modules, so the relay protocol cannot fork per harness. ## Glossary diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/context.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/context.md index 20a295785d..717cb7e26e 100644 --- a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/context.md +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/context.md @@ -63,8 +63,8 @@ them caused the #4831 regression). This plan touches only the second. 2. The delivery is harness-agnostic on the sandbox side: any MCP-client harness (Codex next) gets the same tools with no per-harness work beyond its ACP adapter. 3. One shared implementation of "turn a tool call into a relay request" serves the Pi - extension, the local Claude channel, and the new in-sandbox server, pinned by a golden - test on the relay request file bytes. + extension, the local Claude channel, and the new in-sandbox server: the relay client + that PR #5232 extracts and pins with contract tests, consumed here. 4. The lifecycle survives warm sandbox reuse (PR #5225): a parked-and-resumed sandbox, a stopped-and-restarted sandbox, and a tool-set change between turns must all behave correctly. diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/open-questions.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/open-questions.md index f03e23bab0..21bcd58ac6 100644 --- a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/open-questions.md +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/open-questions.md @@ -1,53 +1,48 @@ # Open questions Decisions the owner needs to make or confirm. Each states the recommendation so a yes is -enough. - -1. **Transport flip: approve A2 (harness-spawned stdio) over A1 (HTTP loopback)?** - `claude-daytona-tools/design.md` recommended A1 with A2 as fallback, before warm sandbox - reuse existed. The plan flips that: A2's lifecycle is tied to the harness session, which - makes every reuse case (park-to-running, park-to-stopped, tool-set change) correct by - construction, and #4873 already implemented it. A1 stays the documented fallback if an - ACP adapter refuses stdio entries. Recommendation: approve the flip. - -2. **Client tools on Claude+Daytona: keep failing loud until the continuation work lands?** - Slice 1 delivers executable tools only; a run carrying a client tool +enough. Items 1 and 5 changed on 2026-07-11 when the Codex review was folded in; see +[status.md](status.md) for that provenance. + +1. **Transport: approve A2 (harness-spawned stdio) conditionally, gated on the slice 0 + restart spike?** The Codex review rejected the earlier "correct by construction" claim: + whether the pinned Claude ACP adapter respawns the shim on the `session/load` path after + a VM stop is external adapter behavior that must be proven. The plan now makes that + proof slice 0 and locks A2 only after it passes. Documented fallbacks: force cold + `createSession` for sessions containing the shim if restoration fails; A1 (HTTP + loopback) as last resort. Recommendation: approve the conditional flip. + +2. **Client tools on Claude+Daytona: keep failing loud until the bridge work lands?** + This project delivers executable tools only; a run carrying a client tool (`request_connection`) on that path still refuses with a narrowed message. The alternatives (drop the spec silently, or advertise it and return a synthetic error) - both mislead. Recommendation: fail loud, sequence client tools with - `agent-client-tool-cleanup` / `mcp-client-tool-continuation`. - -3. **Is Codex-on-Daytona part of this feature's acceptance, or a follow-up?** The shim is - harness-agnostic by design, but the Codex ACP adapter's handling of stdio MCP entries is - unverified. Recommendation: Claude is the acceptance gate; Codex is a verification task - in slice 4, done when the Codex harness itself is in scope. - -4. **U2 timing: when do we explore Pi consuming the in-sandbox MCP server directly?** It - requires building an MCP client into the Pi extension (pi-acp drops `mcpServers`; that - is by design), adds a hop to the one path that works everywhere, and its real payoff is - user MCP on Pi. Recommendation: decide after Codex lands; U1's shared modules already - deliver the "one gateway-tool logic" goal at the module level. - -5. **Env contract: confirm reusing `AGENTA_AGENT_TOOLS_PUBLIC_SPECS` / - `AGENTA_AGENT_TOOLS_RELAY_DIR` for the shim's per-server env.** One public-spec contract - for every in-sandbox consumer, correcting #4873's parallel names. If the spec JSON ever - outgrows env limits (very large tool sets), the fallback is writing the specs to a file - next to the bundle and passing the path. Recommendation: reuse the names; note the file - fallback, do not build it yet. - -6. **Gate posture for future remote providers: confirm fail-closed stays.** After slice 2 + both mislead. Recommendation: fail loud, leave client tools to the future + Daytona client-tool bridge workspace. + +3. **Specs delivery: confirm the file, not the env variable.** Decided in the plan per the + Codex review: `AGENTA_AGENT_TOOLS_PUBLIC_SPECS` is unbounded and would be copied through + four exec-environment layers under A2, so the shim reads the specs from a file uploaded + next to the bundle, with the path in its per-server env. Pi keeps its env variable; the + spec content shape stays shared. The earlier "note a fallback, build it later" position + is out. Recommendation: confirm the file. + +4. **Gate posture for future remote providers: confirm fail-closed stays.** After slice 1 the refusal still fires for any non-Daytona remote provider (the in-flight E2B work would need its own proven delivery before the gate opens for it). Recommendation: confirm. -7. **Snapshot bake timing.** Per-run upload ships first (about 5 kB, negligible). Bake into - `build_snapshot.py` once the path is hot, behind a skip flag mirroring the Pi install - flag. Recommendation: follow-up, not in the first PR. - -8. **User HTTP MCP, API-key-now: is the existing mechanism the answer?** Named secrets +5. **User HTTP MCP, API-key-now: is the existing mechanism the answer?** Named secrets already become request headers on the user's HTTP entry (`services/runner/src/engines/sandbox_agent/mcp.ts:119`), SSRF-guarded, behind `AGENTA_AGENT_MCPS_ENABLED` (default off). The decision "API key in a header for now" appears to be already built; the only open item is when to flip the flag default, which is the separate S2 work (#4912), not this project. OAuth for user MCP stays named future work. Recommendation: confirm this reading so the policy in context.md is complete. + +## Settled by the 2026-07-11 review fold (no longer questions) + +- **Codex-on-Daytona** is a follow-up verification task, cut from v1. +- **Pi consuming the shim directly** (the old U2) is a follow-up decision, cut from v1. +- **Snapshot bake** is a follow-up with its own decision; per-run upload ships first. +- **Relay module ownership**: PR #5232 owns `relay-client.ts` / `relay-protocol.ts` + extraction as its slice 0; this project consumes them. diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/plan.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/plan.md index 850832d998..324454d96c 100644 --- a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/plan.md +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/plan.md @@ -1,18 +1,24 @@ # Plan +Revised 2026-07-11 after a Codex xhigh review. The verdict: approve A2 conditionally, reject +the earlier scope. This revision adopts the cuts. The minimum viable feature is now: prove +the restart path (slice 0), consume the relay modules that PR #5232 owns, add a small stdio +entrypoint, deliver executable tools on Claude+Daytona, and prove the warm lifecycle live. +Everything else moved to explicit follow-ups. + ## The design in one paragraph Ship a small, dependency-free MCP server bundle into the Daytona sandbox and advertise it to the harness as an internal stdio MCP entry named `agenta-tools`. The harness's own ACP adapter spawns it inside the sandbox at session creation; its `tools/list` serves the run's -public specs, and its `tools/call` writes a relay request file through the same relay-writer -module the Pi extension already uses. The runner's existing relay loop executes every call +public specs, and its `tools/call` writes a relay request file through the shared relay +client that PR #5232 extracts. The runner's existing relay loop executes every call server-side, behind the existing permission guard, with credentials that never enter the -sandbox. The handler and the relay writer become shared modules with a golden test pinning -the request file bytes, so Pi, local Claude, and the in-sandbox server are three transports -over one implementation. This revives PR #4873 onto today's code rather than building new. +sandbox. This revives PR #4873 onto today's code rather than building new. One condition +gates the transport choice: a pre-implementation spike must prove that the pinned Claude ACP +adapter respawns the shim on the stop-and-restart path. -## Transport: reconciling A1, A2, and daemon-spawned into one choice +## Transport: A2, conditionally Three candidates existed across the prior designs: @@ -29,51 +35,41 @@ Three candidates existed across the prior designs: The daemon-spawned candidate collapses into A2: #4873 demonstrated that no daemon change is needed, because the daemon already forwards `sessionInit.mcpServers` verbatim and the Claude -ACP adapter already spawns a stdio entry inside the sandbox. The daemon concept -`remote-tools-delivery` asked for exists; it is the harness adapter's own MCP-server list. - -Between A1 and A2, the decision is **A2**, flipping the earlier `claude-daytona-tools` -recommendation. Two facts changed since that document: - -1. **Warm sandbox reuse (PR #5225) makes a runner-managed long-lived process the wrong - shape.** An A1 shim must be health-checked when a parked sandbox resumes, restarted after - park-to-stopped (the VM stop killed it), and found and replaced when a tool-set change - forces a cold session inside a reused sandbox (a stale process on the fixed port serving - last turn's specs). Every one of those is a new failure mode with a live-QA cell. An A2 - shim inherits the session lifecycle instead: the adapter spawns it with the session's env - at session creation, so a new session always means a fresh shim with fresh specs, a - stopped VM cannot leave a stale one (the process died with the VM and respawns with the - session), and an orphan whose parent died exits on its own when stdin closes. -2. **A2 is already implemented and unit-tested** (#4873), including the ACP entry mapping - that was the main unknown when A1 was recommended ("stdio as fallback if port/readiness - is fiddly"). The port and readiness management A1 requires is exactly the part with no - existing code. - -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. - -Costs of A2, stated honestly: - -- **Stdio optics.** User stdio MCP is disabled, permanently. This entry is stdio too, but it - is synthesized by the runner from resolved tools, never user-declared, and it runs inside - the sandbox, not on the runner host. The implementation must keep the layers structurally - separate: the internal entry is built after the user-stdio gate has already run - (`run-plan.ts:341`), never flows through `toAcpMcpServers`, and a test pins that a - user-declared stdio server is still refused on the exact path that ships the shim. -- **Dependence on the ACP adapter's typeless-entry-to-stdio mapping.** Verified against the - adapter pin at #4873 time; re-verify against the current pin, and against Codex's ACP - adapter when that harness lands. If an adapter ever refuses stdio entries, A1 is the - documented fallback: the same bundle grows an HTTP mode (`tool-mcp-http.ts` relocated), - the runner starts it via `runProcess`, and the lifecycle work above becomes real. Nothing - in the shared modules is transport-specific, so the fallback swaps the outer loop only. +ACP adapter already spawns a stdio entry inside the sandbox. + +Between A1 and A2, the recommendation stays **A2**, but for a narrower reason than earlier +drafts claimed. A2's real advantage is mechanism count: no listener, no port allocation, no +readiness endpoint, no PID bookkeeping, and no runner-side process supervision. A2 is also +already implemented and unit-tested (#4873), including the ACP entry mapping that was the +main unknown when A1 was recommended. It works under `network: off` since it is stdio plus +file I/O. + +**What earlier drafts overstated, corrected here:** + +- **Restart is not "correct by construction".** The restart path after park-to-stopped can + seed persisted `sessionInit.mcpServers` and call `resumeSession()` / `session/load` + (`engines/sandbox_agent.ts:1267`) rather than create a fresh ACP session. Whether the + Claude ACP adapter recreates dead MCP subprocesses on that path is external adapter + behavior. It must be proven, not inferred. Slice 0 is that proof, and A2 is locked only + after it passes. +- **Orphan exit is an expectation, not a guarantee.** The runner itself warns that an ACP + subprocess can reparent to PID 1 unless graceful session cancellation occurs + (`engines/sandbox_agent.ts:827`). "Stdin closes, so the shim exits" describes the normal + path; the live QA must watch for reparented survivors. + +**Documented fallbacks, in order:** if the spike shows that `session/load` silently loses +MCP servers, force a cold `createSession` for any session that contains the internal shim +(the sandbox is still reused; only the harness session rebuilds). If the adapter cannot +spawn stdio entries at all, A1 is the last resort: the same bundle grows an HTTP mode, the +runner starts it via `runProcess`, and the lifecycle work A1 requires becomes real. Do not +build transport-neutral machinery for A1 now. ## What runs where ``` runner (holds credentials, executes) sandbox (holds nothing secret) ------------------------------------ ------------------------------ -resolve tools -> public specs ----------------------> shim env (specs + relay dir) +resolve tools -> public specs (file) ---------------> specs file beside the bundle upload shim bundle (daemon FS API) -----------------> /home/sandbox/.agenta/tool-mcp.js advertise McpServerStdio "agenta-tools" -----------> harness ACP adapter spawns shim startToolRelay polls relay dir <--------------------- shim writes .req.json @@ -88,83 +84,62 @@ Local runs are unchanged: Claude on the local sandbox keeps the runner-loopback No `/run` wire change and no new protocol field. Every new element classified: -- **Input (what to serve):** the public tool specs, carried to the shim as - `AGENTA_AGENT_TOOLS_PUBLIC_SPECS` in the stdio entry's per-server `env`. This is the exact - variable and shape the Pi extension reads (`pi-assets.ts:71`); #4873's parallel names are - dropped so the public-spec contract has one source of truth. -- **Routing (where calls go):** `AGENTA_AGENT_TOOLS_RELAY_DIR`, same reuse. +- **Input (what to serve):** the public tool specs, delivered as a file uploaded next to + the bundle, with the path in the stdio entry's per-server `env`. Decided now, not + deferred: `AGENTA_AGENT_TOOLS_PUBLIC_SPECS` can carry many large JSON Schemas, and under + A2 the env is copied through four layers (runner session config, daemon/ACP protocol + state, adapter spawn environment, child process environment). An unbounded input through + an exec environment is not acceptable, and a size limit would trade a working case for a + loud failure. The relay directory is already a path capability and public specs are not + credentials, so a file is the robust shape. Pi keeps its env variable; the spec content + contract (the `AdvertisedToolSpec` array) stays one shape for both consumers. +- **Routing (where calls go):** `AGENTA_AGENT_TOOLS_RELAY_DIR`, reused unchanged. - **Protocol context:** the ACP `McpServerStdio` entry `{name, command, args, env}`. `name` is `agenta-tools`, a stable identity coupled to the rendered permission rules (`claude_settings.py:60`); `command`/`args` are `node` plus the in-sandbox bundle path. + The `McpServerStdio` shape moves out of `tools/mcp-bridge.ts` (which is the local HTTP + channel) into `engines/sandbox_agent/mcp.ts` or a small ACP MCP types module beside it. - **Config (runner-side, operator-owned):** `SANDBOX_AGENT_RELAY_MCP_BUNDLE`, the bundle location override (test and packaging use), defaulting to the esbuild output next to the - Pi extension bundle. Later, a snapshot-bake skip flag mirroring - `AGENTA_AGENT_SANDBOX_PI_INSTALLED`. + Pi extension bundle. The override selects code, so it is trusted deployment + configuration, never run or request configuration. - **Credentials:** none, anywhere in this design. The shim has no credential field because - execution stays where the credentials are. The entry's `headers` concept does not exist - for stdio; nothing rides `env` except the two public variables above. -- **The relay file protocol:** unchanged, and now golden-pinned as the stable contract - between any in-sandbox writer and the runner loop. - -## Unification with Pi - -The owner's priority. Today the "turn a model's tool call into a relay request" logic exists -in two call sites that both route through one writer (`runResolvedTool` -> -`relayToolCall`), plus an MCP message handler that exists once (`tool-mcp-http.ts:101`) and -was duplicated by #4873. The unification target is: one writer, one handler, three thin -transports. - -**U1 (do now): shared modules.** Extract two pieces: - -1. `tools/relay-client.ts`: the relay writer (`relayToolCall` plus its wait loop), moved - out of `dispatch.ts` as #4873 did, with `dispatch.ts` re-exporting so existing call sites - (the Pi extension, local Claude) are unchanged. It must bundle with zero non-relay code - and honor the per-tool `timeoutMs` and an abort signal. The response wait is one exported - function with the contract `waitForRelayResponse(resPath, { timeoutMs, signal })` (final - name at implementation time), implemented today as the existing 300 ms poll. That - function is the seam the `event-driven-tool-relay` sibling swaps for `fs.watch`; see the - coordination section below for the landing order. -2. `tools/mcp-handler.ts`: the transport-neutral MCP message handler (initialize, - tools/list with the shared schema accessor, tools/call, notifications, errors), factored - from `tool-mcp-http.ts`, parameterized by an "execute" function and an optional - client-tool pause hook. The HTTP server keeps its socket-abort pause; the stdio shim - passes no pause hook in slice 1 (client tools are not delivered there yet). - - Layering rule, stated once: `mcp-handler.ts` stays credential-free. The per-environment - bearer that `mcp-client-tool-continuation` WP1 adds - ([../mcp-client-tool-continuation/plan.md](../mcp-client-tool-continuation/plan.md)) - lives in the HTTP transport wrapper in `tool-mcp-http.ts`, never in the shared handler, - so the stdio shim (a harness-spawned child process with no listener) inherits no auth - requirement. Ordering with that project: this slice 1 lands first, and its WP1 (auth, - client-tool batch rejection) and WP3 (register-before-pause hold-open) then build on the - extracted handler; the optional pause hook here is WP3's insertion point. - -The golden test pins the request file: the Pi extension path and the shim path, given the -same call, produce byte-identical `.req.json` content. That single test is what keeps "one -gateway-tool logic" true over time. - -**U3 (do now, nearly free): one build pipeline.** The shim bundles in the same esbuild step -as the Pi extension, into `dist/`, baked into the runner image the same way. Same packaging, -same upload helper pattern, same snapshot-bake story later. - -**U2 (explore later, not now): Pi consumes the in-sandbox MCP server directly.** The idea: -the Pi extension stops reading specs from env and instead dials the shim, lists its tools, -and registers a forwarding `registerTool` for each; eventually the extension is a generic -MCP client and user HTTP MCP could reach Pi through the same code. Honest trade-offs: - -- Pi has no MCP client by design (pi-acp drops `mcpServers`), so this is new client code in - the extension, not configuration. -- It adds a process and a hop to the one path that currently works on every backend, for no - functional gain today: the extension already shares the writer and (after U1) would share - nothing further by speaking MCP, because MCP is the part Pi does not need. -- Its real payoff is a different feature: user MCP on Pi (the open F-009 question) and - retiring the env-var spec channel. Both are worth a decision when Codex lands and the - MCP-client population grows, not before. - -Recommendation: land U1+U3 now; write U2 up as a follow-up decision for the owner (see -[open-questions.md](open-questions.md)). After U1, "one gateway-tool logic" is concretely -true at the module level: both harness families execute tool calls through the same handler -semantics and the same writer bytes, verified by the golden. + execution stays where the credentials are. +- **The relay file protocol:** unchanged, owned by PR #5232 together with the relay client + and its contract tests. + +## Unification with Pi, stated honestly + +The owner's priority is one gateway-tool logic. The earlier draft oversold what an MCP +handler buys here, so this section now separates the real unification from the local one. + +**The real unification is the relay client and the relay file protocol.** Pi never speaks +MCP: pi-acp drops `mcpServers`, and the Pi extension registers tools directly and calls +`runResolvedTool` (`extensions/agenta.ts:280`). What Pi, local Claude, and the shim +genuinely share is the code that turns a tool call into a relay request file and waits for +the response. That code is `tools/relay-client.ts` and `tools/relay-protocol.ts`, and +**PR #5232 (event-driven-tool-relay) owns their extraction as its slice 0**. That PR is an +explicit prerequisite of this project. This project consumes those modules and adds only +shim-specific tests. Earlier drafts had this project extracting the modules and landing +first; that ordering is reversed and the conflict is closed. + +**An MCP handler is not Pi unification.** A shared MCP message dispatcher would unify +exactly two transports: the local HTTP server and the stdio shim. That is useful but local. +The plan therefore cuts the standalone "transport-neutral handler, zero behavior change" +slice. Instead, while implementing the stdio shim, extract the smallest reusable MCP method +dispatcher only if it clearly reduces duplication against `tool-mcp-http.ts`; otherwise +temporarily duplicate the small protocol switch with focused parity tests and extract after +both transports stabilize. If a module is extracted, name it for what it is: +`internal-tool-mcp-handler.ts`, not a generic `mcp-handler.ts`, in a repository that also +has user HTTP MCP and deliberately disabled user stdio MCP. Do not pre-design a client-tool +pause hook for a protocol that is not designed yet; that insertion point gets added when +the continuation or bridge work needs it. + +**One build pipeline (kept, nearly free).** The shim bundles in the same esbuild step as +the Pi extension, into `dist/`, baked into the runner image the same way. + +**Pi consuming the shim directly (the old U2) is cut from this project** and recorded as a +follow-up decision; see the follow-ups list in the slices section. ## Lifecycle @@ -179,78 +154,89 @@ semantics and the same writer bytes, verified by the golden. shim dies after writing `.req.json` but before reading the response, the runner still executes the call (side effects happen) and writes a `.res.json` nobody consumes, while the model sees an MCP failure. The relay is at-least-once from the executor's point of - view, and this property is shared with Pi's writer today (an aborted or timed-out wait - after the request file is written behaves the same). The orphaned response file is inert - (the runner loop lists only `.req.json`) and the workspace preparation clears it on the - next cold build. Slice 2's integration tests must cover crash-after-write. + view, and this property is shared with Pi's writer today. Slice 1's integration tests + must cover crash-after-write. - **Teardown, ephemeral delete.** The shim dies with the sandbox. Nothing to do. - **Warm reuse, park-to-running.** The harness session stays alive, so the shim stays alive - with it, still serving the specs that session was created with. Correct by construction: - the keep-alive fingerprint includes `customTools` (`session-pool.ts:170`), so a live - session is only continued when the tool set is unchanged. + with it, still serving the specs that session was created with. The keep-alive fingerprint + includes `customTools` (`session-pool.ts:170`), so a live session is only continued when + the tool set is unchanged. - **Warm reuse, park-to-stopped.** The VM stop kills the harness and the shim. The next turn - restarts the sandbox and builds or loads a session; the adapter respawns the shim from the - session's MCP config. The bundle file survives on the sandbox filesystem; the upload - helper can skip an unchanged existing file as an optimization. + restarts the sandbox and either builds a fresh session or loads the old one. The fresh + build respawns the shim by construction. The `session/load` path is the open risk that + slice 0 proves: the adapter may seed the persisted MCP config without respawning the + subprocess, or may restore it, or may fail the load. Until the spike answers this, no + claim is made. The documented fallback if restoration fails is a forced cold + `createSession` for sessions containing the shim. - **Tool-set change between turns.** The fingerprint mismatch forces a cold session in the reused sandbox. The old session is destroyed (`destroySession`, - `engines/sandbox_agent.ts:829-832`), which ends the old shim (stdin closes, the readline - loop ends, the process exits). The new session spawns a fresh shim with the new specs in - its env. No fixed port means no collision window. -- **Spec freshness invariant, stated once:** the shim's spec list is immutable per process, - and a shim process never outlives the session that spawned it. Everything above is that - invariant applied to each reuse case, and the live-QA matrix checks each case. - -## Client tools: sequenced after, not in, the first slice + `engines/sandbox_agent.ts:829-832`), which normally ends the old shim (stdin closes, the + readline loop ends, the process exits); the reparenting caveat above applies and the live + QA checks for survivors. The new session spawns a fresh shim with the new specs. No fixed + port means no collision window. +- **Warm-reuse edge cases the tests must cover, beyond the above:** + - `session/load` after VM stop (the slice 0 spike, then a live QA cell). + - Sanitized tool-call-ID collision: distinct raw IDs can sanitize to the same relay + filename; two concurrent calls must not share a file. + - Bundle-version skew in a reused sandbox: a sandbox that survived a runner deploy holds + an old bundle file; the upload helper must overwrite rather than skip when content + differs (hash or size check, not existence check). + - Partial request visibility: the writer writes directly to the final `.req.json` path + while the runner polls. PR #5232's plan amends the protocol with write-to-temp plus + atomic rename; this project inherits that amendment through the shared relay client and + must not work around it. + +## Client tools: sequenced after, not in, this project Client tools are advertised-and-paused on the local HTTP channel today (`MCP_PAUSED` aborts the in-flight request so no result settles). Through the in-sandbox shim the shape is different: the relay loop parks the call and writes no response file (`relay.ts:249-252`), -so the shim's wait would hang until the relay timeout and return an error to the model, -which is exactly the park-must-emit-no-result problem the client-tool continuation work -exists to solve (a parked call must produce no tool result, and the resumed turn must settle -the original call). - -Sequencing, matching the prior designs: **slice 1 delivers executable (gateway/callback) -tools only.** A run that carries a client tool on the Claude+Daytona path keeps failing loud -with a narrowed, honest message (options considered: silently dropping client specs repeats -the F-032 silent-drop bug; advertising them and returning a synthetic error teaches the -model the tool is broken). When the continuation work lands, the shim inherits the pause -semantics by adding the pause hook to the shared handler, and the relay response protocol -gains whatever the continuation design chooses; that is deliberately not designed here. +so the shim's wait would hang until the relay timeout and return an error to the model. + +This project delivers executable (gateway/callback) tools only. A run that carries a client +tool on the Claude+Daytona path keeps failing loud with a narrowed, honest message (options +considered: silently dropping client specs repeats the F-032 silent-drop bug; advertising +them and returning a synthetic error teaches the model the tool is broken). Ownership, stated plainly so the gap cannot hide: neither this project nor [../mcp-client-tool-continuation/](../mcp-client-tool-continuation/README.md) designs the -Daytona client-tool bridge. That bridge needs a relay park protocol (a paused call must -produce no result until the browser answers, but the shim's wait times out at 60 s today), -a stdio analogue for abort-without-result (`res.destroy()` does not exist for a spawned -child), and an answer to the race between a held browser wait and the five-minute Daytona -auto-stop that kills the shim on park-to-stopped. The recommendation to open one owned -workspace for it lives in +Daytona client-tool bridge. That bridge needs a relay park protocol, a stdio analogue for +abort-without-result, and an answer to the Daytona auto-stop race. The recommendation to +open one owned workspace for it lives in [../mcp-delivery-architecture/orchestration.md](../mcp-delivery-architecture/orchestration.md). ## Gate change -`REMOTE_TOOLS_UNSUPPORTED_MESSAGE` narrows instead of disappearing. After slice 2 the refusal +`REMOTE_TOOLS_UNSUPPORTED_MESSAGE` narrows instead of disappearing. After slice 1 the refusal fires only when: the harness is MCP-capable but the remote provider is not Daytona (fail -closed per provider until proven), or the run carries a client tool (until the continuation -work). The message text updates to say what is and is not supported and to point at this -workspace. The capability gate (`assertRequiredCapabilities`) is unchanged: Claude truthfully -advertises `mcpTools`, and now the advertisement is true on Daytona too. +closed per provider until proven), or the run carries a client tool (until the bridge work). +The message text updates to say what is and is not supported and to point at this workspace. +The capability gate (`assertRequiredCapabilities`) is unchanged: Claude truthfully advertises +`mcpTools`, and now the advertisement is true on Daytona too. ## Security The invariant, restated: **the sandbox sees public specs and a relay directory, nothing else; every credentialed action executes runner-side; the shim opens no network surface.** -- No credential enters the sandbox: the shim env carries specs and a path. Private spec - fields never leave runner memory (unchanged, `public-spec.ts`). +- No credential enters the sandbox: the shim env carries a specs-file path and the relay + dir. Private spec fields never leave runner memory (unchanged, `public-spec.ts`). - No listener: stdio only. The loopback-only rule of the HTTP variant becomes "no socket at all". -- The user-stdio disable is not relaxed. The internal entry is synthesized downstream of the - user gates, shares no constant and no code path with `toAcpMcpServers`' stdio branch, and - a layering test pins: user stdio still refused, user HTTP still delivered, internal entry - present, on the same Daytona run. +- The user-stdio disable is not relaxed, and the separation is structural, not test-only: + - The internal entry gets its own constructor and type, separate from any user MCP entry + type. Its `command`, `args`, and `env` are built entirely from runner constants and + operator configuration; no user-supplied `command`, `args`, `env`, or `transport` field + can flow into it. + - The internal entry is synthesized after the user-stdio refusal has already run + (`run-plan.ts:341`) and never flows through `toAcpMcpServers`. `toAcpMcpServers` stays + incapable of returning stdio; this project must not generalize it. + - The reserved server name `agenta-tools` is rejected for user-declared MCP servers at + validation time. The Python adapter already ignores user MCP permissions with that name + (`claude_settings.py:119`); the runner adds the matching refusal on the declaration + itself. + - A layering test pins: user stdio still refused, user HTTP still delivered, internal + entry present, on the same Daytona run. - The relay directory remains an in-sandbox capability: any sandbox process can write a request file. That is the accepted, pre-existing Pi posture, and the runner-side permission guard (`relay.ts:105`) re-checks every executable call, so a forged file cannot @@ -258,81 +244,102 @@ else; every credentialed action executes runner-side; the shim opens no network - Strict-network runs with executable tools stay refused (`run-plan.ts:368`): execution still happens on the runner, outside the sandbox egress boundary. Unchanged. -## Coordination with event-driven-tool-relay - -The sibling project replaces relay polling with filesystem-event wakeups. Three contact -points: - -1. **The file contract is shared and golden-pinned.** Request and response names, bytes, and - delete-after-read semantics do not change in either project. The golden test in slice 1 - is the enforcement. -2. **The shim's response wait is one small function, and this project creates it.** - `relay-client.ts` isolates "wait for `.res.json`" behind - `waitForRelayResponse(resPath, { timeoutMs, signal })`, currently implemented as the - existing poll. The sibling swaps its internals for `fs.watch` without touching the - handler or the writer. Landing order: this project's slice 1 lands first, so the - sibling's in-sandbox watch goes into that function rather than into `dispatch.ts`. The - sibling's plan currently names `relayToolCall` in `dispatch.ts` as its seam and does not - name `relay-client.ts`; that pending alignment is recorded in +## Coordination with event-driven-tool-relay (PR #5232) + +The sibling owns the relay mechanics this project rides on. Contact points: + +1. **PR #5232 slice 0 is this project's prerequisite.** It extracts `tools/relay-client.ts` + and `tools/relay-protocol.ts` and lands their contract tests. This project consumes the + modules; it does not create, move, or re-test them beyond shim-specific integration. +2. **The response-wait seam lives in the extracted client.** The sibling swaps its internals + for `fs.watch`; nothing in this project depends on how the wait is implemented, and + adopting the watch inside the shim is a follow-up, not v1. +3. **The relay file protocol, including the atomic-rename amendment and the + orphaned-request residue across warm-continued turns, is owned there.** This project's + tests exercise the protocol; they do not redefine it. The residue ownership decision is + tracked in [../mcp-delivery-architecture/orchestration.md](../mcp-delivery-architecture/orchestration.md). - Nothing else in this project depends on how the wait is implemented. -3. **The orphaned-request residue across warm-continued turns** (see - [research.md](research.md)) needs an owner. The sibling owns relay mechanics but its - current plan disclaims this property; the ownership decision is tracked in the same - orchestration file. + +## Naming and placement + +- Stdio entrypoint: `tools/tool-mcp-stdio.ts` (bundled to the sandbox). +- If a shared dispatcher is extracted: `tools/internal-tool-mcp-handler.ts`. +- Upload and bundle-path helpers: under `engines/sandbox_agent/`, analogous to + `pi-assets.ts`. +- ACP MCP entry shapes (`McpServerStdio`): move out of `tools/mcp-bridge.ts` into + `engines/sandbox_agent/mcp.ts` or a small types module beside it; `mcp-bridge.ts` is the + local HTTP channel and should not export ACP entry types. +- Server name: `agenta-tools`, unchanged; `claude_settings.py` couples the rendered + permission rules to it. ## Packaging -Per-run upload first: `writeFsFile` the bundle (about 5 kB) via the fail-loud helper, -mirroring the Pi extension upload. Cold-start cost is one small FS write, negligible next to -the session create it rides along with, and the file persists across warm reuse. Snapshot -bake is a follow-up optimization in `build_snapshot.py` (same pattern as the pinned `pi` -install) with an env flag to skip the upload, worth doing once the path is hot; it changes -no licensing posture because the shim is Agenta code. +Per-run upload: `writeFsFile` the bundle (about 5 kB) plus the specs file via the fail-loud +helper, mirroring the Pi extension upload. Cold-start cost is two small FS writes, +negligible next to the session create they ride along with. Five kilobytes does not justify +snapshot lifecycle and version-skew machinery, so snapshot bake stays a follow-up with its +own decision. ## Slices -**Slice 1: shared modules, no behavior change.** -Extract `tools/relay-client.ts` (writer + wait, timeout + signal) and `tools/mcp-handler.ts` -(transport-neutral handler) from `dispatch.ts`/`tool-mcp-http.ts`; re-export so all call -sites compile unchanged. Add the golden test pinning the request file bytes across the Pi -path and the handler path. All existing runner tests stay green. -Acceptance: zero behavior diff; golden committed. - -**Slice 2: the shim, Daytona delivery, gate narrowing.** -Revive #4873 onto today's paths: the stdio entry bundle (thin loop over the shared handler + -writer), the esbuild step, the fail-loud upload helper, the `mcp.ts` Daytona non-Pi branch -building the internal `McpServerStdio` entry, engine wiring next to -`prepareDaytonaPiAssets`, and the narrowed run-plan gate (executable tools pass on Daytona -for MCP-capable harnesses; client tools and non-Daytona remotes still refuse loud). Unit -tests: handler over stdio framing, upload fail-loud, layering (user stdio refused / user -HTTP delivered / internal entry present), gate matrix. -Acceptance: runner tests + typecheck green; a fake-daemon integration test drives +**Slice 0: the restart spike (gates A2).** +Against the exact pinned Claude ACP adapter: create a session with the internal stdio MCP, +stop and restart the VM, exercise the real `resumeSession` / `session/load` path, and verify +that `tools/list` succeeds and a new shim PID exists. Define what happens when MCP +restoration fails: does `session/load` fail and fall back to `session/new`, or does it +return a session with zero tools? Record the answer in this workspace. Only then lock A2. +If restoration silently loses MCP servers, adopt the documented fallback (force cold +`createSession` for sessions containing the shim); A1 is the last resort. +Acceptance: a written spike report with the adapter pin, the observed behavior, and the +decision. + +**Prerequisite (external): PR #5232 slice 0** lands `tools/relay-client.ts` and +`tools/relay-protocol.ts` with their contract tests. + +**Slice 1: the shim, Daytona delivery, gate narrowing.** +Revive #4873 onto today's paths: `tool-mcp-stdio.ts` (a thin stdio loop over the consumed +relay client), the esbuild step, the fail-loud upload helper for bundle and specs file, the +`engines/sandbox_agent/mcp.ts` Daytona non-Pi branch building the internal `McpServerStdio` +entry via its dedicated constructor, engine wiring next to `prepareDaytonaPiAssets`, and the +narrowed run-plan gate (executable tools pass on Daytona for MCP-capable harnesses; client +tools and non-Daytona remotes still refuse loud). Tests are semantic contract tests, not +byte goldens: the shim's request is accepted by a real `startToolRelay` loop; IDs are +sanitized, bounded, and collision-free; timeout and abort clean up; concurrent calls get +distinct files and distinct responses; stdout carries only complete JSON-RPC lines and +logging stays on stderr; the layering test (user stdio refused / user HTTP delivered / +internal entry present); the gate matrix; upload fail-loud. +Acceptance: runner tests and typecheck green; a fake-daemon integration test drives shim -> relay dir -> `startToolRelay(localRelayHost())` -> mocked callback and asserts the round trip. -**Slice 3: live QA and the replay pin.** +**Slice 2: live acceptance before merge.** The matrix cell that has never been green: Claude + Daytona + gateway tool (github via the `pi-agents` project, which holds live Composio connections), asserting the tool executes and the result reaches the answer. Negatives: Claude+Daytona with no tools still runs (no shim uploaded); Pi+Daytona unchanged; Claude+local unchanged. Warm-reuse cells: second turn within the idle window (live session, same shim), second turn after park-to-stopped -(restart, respawn), tool-set change between turns (cold session in the reused sandbox, -fresh shim). Network-off cell: gateway tool executes with `network` restricted + -`best_effort` (relay is file I/O; `best_effort` is required because the strict-network -refusal for executable tools at `run-plan.ts:368` stays, unchanged by this project, and -only an explicit `best_effort` opts out of it). Capture one green run and pin it with the -agent-replay-test recipe so the path regression-tests without a live LLM. Sandbox hygiene: -cheap model, verify the park/delete reaps everything. -Acceptance: matrix recorded in this workspace; replay test committed. - -**Slice 4 (follow-ups, each its own decision):** client tools through the shim (after the -continuation work), snapshot bake, Codex ACP adapter verification, the U2 exploration -(Pi as an MCP client), and adopting the sibling's watch-based wait. +(restart, respawn, per the slice 0 finding), tool-set change between turns (cold session in +the reused sandbox, fresh shim, no reparented survivor). Network-off cell: gateway tool +executes with `network` restricted + `best_effort` (relay is file I/O; `best_effort` is +required because the strict-network refusal for executable tools at `run-plan.ts:368` +stays, unchanged by this project). Sandbox hygiene: cheap model, verify the park/delete +reaps everything. +Acceptance: matrix recorded in this workspace. + +**Follow-ups, cut from v1, each its own decision:** + +- Client tools through the shim (needs the Daytona client-tool bridge workspace). +- Codex-on-Daytona verification (when the Codex harness is in scope). +- Snapshot bake and its skip flag. +- Pi as an MCP client (the old U2 exploration). +- Adopting the sibling's watch-based response wait inside the shim. +- Replay capture: a recorded `/run` response does not prove Daytona process spawning or + restart behavior; add a replay test later only if it protects a deterministic + runner/service contract worth pinning. ## Effort -Slices 1+2 are roughly two focused days (most code exists in #4873); slice 3 is one day -dominated by live QA. Risk is low: no wire change, no new network surface, local paths -untouched, and the one novel dependency (the adapter's stdio mapping) was already proven -once and is re-verified before merge. +Slice 0 is half a day of live spike work. Slice 1 is roughly one focused day once PR #5232 +slice 0 has landed (most code exists in #4873); slice 2 is one day dominated by live QA. +Risk concentrates in the one novel dependency: the adapter's stdio mapping and its restart +behavior, which slice 0 proves before any implementation is committed. diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/research.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/research.md index b9e54a19dc..b222689cd3 100644 --- a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/research.md +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/research.md @@ -59,10 +59,12 @@ relay request files. Pi has one; MCP-client harnesses have none. rules as `mcp__agenta-tools__` into `.claude/settings.json` (`sdks/python/agenta/sdk/agents/adapters/claude_settings.py:60`, `:175`). The in-sandbox server must keep the name or every rendered allow/deny rule silently stops matching. -- **The relay request file bytes.** Both writers must emit byte-identical - `{"toolName":...,"toolCallId":...,"args":...}` JSON so one relay loop serves both. Today - there is exactly one writer implementation (`relayToolCall`, `dispatch.ts:62`); the design - keeps it that way and pins it with a golden test. +- **The relay request record.** Both front-ends must emit the same + `{"toolName":...,"toolCallId":...,"args":...}` record so one relay loop serves both. + Today there is exactly one writer implementation (`relayToolCall`, `dispatch.ts:62`); the + design keeps it that way by consuming the relay client PR #5232 extracts. The reader uses + `JSON.parse` (`relay.ts:342`), so property order and whitespace are not protocol + semantics; the contract tests pin the record shape, not bytes. - **The public-spec env contract.** `AGENTA_AGENT_TOOLS_PUBLIC_SPECS` (a JSON array of `AdvertisedToolSpec`: name, description, inputSchema, kind, render, timeoutMs, `tools/public-spec.ts:12-19`) and `AGENTA_AGENT_TOOLS_RELAY_DIR`. One contract, one pair @@ -96,9 +98,15 @@ Implications: a long-lived runner-started shim process (the A1 shape) must be fo killed or replaced on cold rebuild in a reused sandbox (stale specs on a fixed port), must be restarted after park-to-stopped, and must be health-checked on park-to-running resume. A harness-spawned shim (the A2 shape) inherits the session lifecycle instead: the harness -spawns it with the session's env at session creation and it dies with the session. This is -the strongest new fact since the prior designs were written, and it flips the transport -recommendation (see [plan.md](plan.md)). +spawns it with the session's env at session creation and normally dies with the session. +Two caveats bound that claim. First, the restart path after park-to-stopped can seed +persisted `sessionInit.mcpServers` and call `resumeSession()` / `session/load` +(`engines/sandbox_agent.ts:1267`) instead of creating a fresh session; whether the Claude +ACP adapter respawns dead MCP subprocesses there is adapter behavior the plan's slice 0 +spike must prove. Second, the runner itself warns that an ACP subprocess can reparent to +PID 1 without graceful session cancellation (`engines/sandbox_agent.ts:827`), so +exit-on-stdin-close is an expectation, not a guarantee. With those caveats, this lifecycle +fact still flips the transport recommendation (see [plan.md](plan.md)). One pre-existing residue risk, shared with Pi, stated precisely: workspace preparation already clears the relay dir on Daytona before each environment build (`rm -rf` at @@ -173,9 +181,10 @@ Why it never merged, and what a straight rebase would miss: lifecycle analysis in [plan.md](plan.md) is new. The verdict: the architecture and most of the code are sound and reviewed (Codex xhigh at -the time); the revival re-homes it onto today's paths, shares the handler instead of copying -it, fixes the env names and timeout, and adds the reuse lifecycle and live QA that were -missing. +the time); the revival re-homes it onto today's paths, consumes the relay client PR #5232 +extracts instead of carrying its own copy, replaces the parallel env names with a specs +file plus the shared relay-dir variable, restores the per-tool timeout, and adds the reuse +lifecycle and live QA that were missing. ## Constraints carried over (summary; full list in prior art) diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md index b5bf965288..876c799c3a 100644 --- a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md @@ -28,21 +28,43 @@ workspace is the review surface; the owner will interview on it. the crash-after-write at-least-once note, and the `best_effort` clarification on the network-off QA cell. Combined landing order: [../mcp-delivery-architecture/orchestration.md](../mcp-delivery-architecture/orchestration.md). +- 2026-07-11 (late): Codex xhigh review of this workspace folded in. The owner was asleep; + per his standing simplify-aggressively instruction the cuts were adopted rather than + argued, and every adoption is reversible at his review. Verdict: approve A2 + conditionally, reject the earlier scope. Changes: a new slice 0 restart spike gates A2 + (the "correct by construction" claim was wrong; `session/load` may seed persisted + `mcpServers` without respawning subprocesses, and the orphan-exit claim is weakened to an + expectation); relay-module extraction moved out of this project entirely (PR #5232 owns + `relay-client.ts`/`relay-protocol.ts` as its slice 0 and is now an explicit prerequisite, + reversing the ordering the earlier consistency pass wrote); the standalone + transport-neutral `mcp-handler.ts` slice is cut and the unification section rewritten + honestly (Pi never speaks MCP; the real sharing is the relay client and file protocol); + specs move from an unbounded env variable to a file, decided now; v1 cuts moved to + explicit follow-ups (client tools, Codex-on-Daytona, snapshot bake, U2, watch adoption, + mandatory replay capture); warm-reuse edges added (session/load after VM stop, + sanitized-ID collision, bundle-version skew, partial request visibility via #5232's + atomic-rename amendment); security separation made structural (dedicated internal entry + constructor/type, reserved-name rejection for user config, `toAcpMcpServers` never + generalized to stdio); naming fixed (`tool-mcp-stdio.ts`, `internal-tool-mcp-handler.ts` + if a handler exists, upload helpers under `engines/sandbox_agent/`, ACP entry shapes out + of `mcp-bridge.ts`). ## Next 1. Owner review of this workspace (interview against - [open-questions.md](open-questions.md), especially the A1-to-A2 flip and the client-tool - sequencing). -2. On approval: implement slice 1 (shared modules + golden), then slice 2 (#4873 revival), - then slice 3 (live QA + replay pin). Coordinate lane usage on the agent board; check the - `event-driven-tool-relay` sibling's state before touching `relay-client.ts` internals. + [open-questions.md](open-questions.md), especially the conditional A2 approval and the + specs-file decision). +2. On approval: run slice 0 (the restart spike) first. Implementation waits for PR #5232 + slice 0 (relay module extraction); then slice 1 (#4873 revival over the consumed + modules), then slice 2 (live acceptance). Coordinate lane usage on the agent board. ## Blockers -None for the design. Implementation slice 3 needs Daytona credit and the `pi-agents` -project's live Composio connections (the same live-QA prerequisites recorded in -`claude-daytona-tools/design.md`). +- Implementation waits on PR #5232 slice 0 (relay module extraction), now an explicit + prerequisite. +- Slice 0 and slice 2 need Daytona credit; slice 2 also needs the `pi-agents` project's + live Composio connections (the same live-QA prerequisites recorded in + `claude-daytona-tools/design.md`). ## Decision log @@ -50,6 +72,11 @@ project's live Composio connections (the same live-QA prerequisites recorded in `claude-daytona-tools`) to A2 (harness-spawned stdio, PR #4873), driven by the warm-reuse lifecycle (PR #5225) and the existence of tested A2 code. Recorded in [plan.md](plan.md); awaiting owner confirmation (open question 1). +- 2026-07-11 (late, Codex review fold): A2 approval made conditional on the slice 0 restart + spike; relay-module ownership handed to PR #5232; the standalone shared-handler slice cut; + specs delivery decided as a file; v1 reduced to spike + shim + live acceptance with + everything else an explicit follow-up. Adopted while the owner slept, flagged for his + review, reversible. - 2026-07-11: the daemon-spawned variant from `remote-tools-delivery` recognized as the same mechanism as A2 (the daemon already forwards session MCP entries; the adapter spawns them), so no daemon change is requested from the sandbox-agent package. From 34407b71cfdf1a0359d0df32e92601a9dc701e84 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sun, 12 Jul 2026 11:30:34 +0200 Subject: [PATCH 4/4] docs(design): in-sandbox tool MCP - spike verdict, slice results, QA 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 --- .../projects/in-sandbox-tool-mcp/qa-slice2.md | 127 ++++++ .../in-sandbox-tool-mcp/spike-restart.md | 124 ++++++ .../spike/spike-adapter-pin.ts | 94 ++++ .../in-sandbox-tool-mcp/spike/spike-mcp.js | 111 +++++ .../spike/spike-restart-driver.ts | 412 ++++++++++++++++++ .../projects/in-sandbox-tool-mcp/status.md | 148 ++++--- 6 files changed, 948 insertions(+), 68 deletions(-) create mode 100644 docs/design/agent-workflows/projects/in-sandbox-tool-mcp/qa-slice2.md create mode 100644 docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike-restart.md create mode 100644 docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-adapter-pin.ts create mode 100644 docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-mcp.js create mode 100644 docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-restart-driver.ts diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/qa-slice2.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/qa-slice2.md new file mode 100644 index 0000000000..865d792bef --- /dev/null +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/qa-slice2.md @@ -0,0 +1,127 @@ +# QA slice 2: live acceptance of the in-sandbox platform-tool MCP feature + +Run 2026-07-12 against the `agenta-claude-sub-sidecar` runner (`POST /run` on +`127.0.0.1:8790`, verified via `docker port`), which holds the correct Daytona config +(`DAYTONA_SNAPSHOT=agenta-sandbox-pi`, target `eu`) and the Claude subscription login for +local Claude runs. The known credit constraint applies: every Anthropic API key on this box +is out of credit, so Claude+Daytona cells are MECHANISM cells (delivery proven, model turn +blocked upstream); Claude local rides the subscription, Pi rides its own uploaded Codex +login. + +Request/response captures: `/tmp/qa-slice2/cell*.json` + `cell*.response.json`. Echo hits: +`/tmp/qa-slice2/echo-hits.jsonl`. + +## Test rig + +- **Gateway tool**: `get_weather`, a `callback`-kind `ResolvedToolSpec` with + `callRef: "qa.get_weather"`, `permission: "allow"`, routed via + `toolCallback.endpoint = http://172.19.0.1:8977/tools/call` — an echo server on the host + (docker bridge gateway IP; `host.docker.internal` does not resolve in this container; + reachability from inside the sidecar verified with a probe fetch before the runs). It + returns the unguessable token `QA-ECHO-TOKEN-c4f7e2a91b` inside + `{call: {data: {content}}}` and logs every hit, so a reply containing the token proves + runner-side end-to-end execution. +- **Requests**: raw `AgentRunRequest` bodies POSTed to `/run` (no saved templates — avoids + the pre-existing `runner.interactions` template validation bug). Shapes mined from + `services/runner/src/protocol.ts` and `docs/design/agent-workflows/projects/qa/`. +- **Cell 3/5 model credential**: the `ANTHROPIC_API_KEY` from `.env.ee.dev`, delivered as + `modelConnection` (`provider: anthropic`, `credentialMode: env`, one + `binding: {kind: environment, name: ANTHROPIC_API_KEY}` credential with + `usage: local_use`, `endpoint.baseUrl: https://api.anthropic.com`). No credential file and + no OAuth token ever entered a sandbox. + +## Matrix + +| # | Cell | Expectation | Observed | Verdict | Evidence | +|---|------|-------------|----------|---------|----------| +| 1 | Claude + local + gateway tool (regression) | ok:true, reply carries the token, LOCAL loopback HTTP MCP channel (not the shim), echo hit once | HTTP 200, `ok:true`, output exactly `QA-ECHO-TOKEN-c4f7e2a91b`, 7.3 s. Log: `internal tool MCP server on http://127.0.0.1:34657/mcp serving 1 tool(s)`; HITL gate `mcp__agenta-tools__get_weather` outcome=allow; `relay_pickup id=c7ec049a... wake=activity`. Echo hit #1: `qa.get_weather` args `{city: Paris}` | **PASS** | `cell1-*.response.json`; sidecar log ~04:21:02Z | +| 2 | Pi + Daytona + gateway tool (regression) | ok:true + token; Pi extension delivery + relay execution | HTTP 200, `ok:true`, output exactly the token, 15.4 s, model `openai-codex/gpt-5.4-mini` on sandbox `daytona/f12c3c93-8ca5-...`. Log: `pi-gate {"gate":"pi-custom-tool","toolName":"get_weather","executor":"relay","specPermission":"allow"}` outcome=allow; `relay_pickup id=call_8WrIujya..._fc_... pickup_ms=1674 wake=poll`. Echo hit #2 with the OpenAI-style call id | **PASS** | `cell2-*.response.json`; relay + pi-gate log lines | +| 3 | Claude + Daytona + gateway tool, cold (MECHANISM) | not refused by the gate; shim delivered + advertised + relay armed; model turn fails on credit | NOT refused. 6.9 s run: `resolved model=haiku provider=anthropic ... credentialKeys=[ANTHROPIC_API_KEY]`; sandbox `daytona/368c7146-...` created; **`daytona: 1 gateway tool(s) advertised via the in-sandbox stdio MCP shim (the loopback MCP URL is unreachable from the sandbox)`** (the new `buildSessionMcpServers` log — fires only when the uploaded shim assets built the internal stdio entry); `create_session ms=1223` succeeded, meaning the ACP session was created WITH the `agenta-tools` mcpServers entry and the adapter's eager spawn+initialize of stdio MCP servers (slice-0 spike finding) did not fail; then `ok:false` `claude: the model provider account has insufficient credit (check the project's Anthropic key)` — a model/credit error AFTER delivery, not a gate refusal (a gate refusal is the `REMOTE_TOOLS_UNSUPPORTED` text in ~30 ms with no sandbox; see cell 4's timing for what refusals look like) | **MECHANISM PASS** | `cell3-*.response.json`; sandbox `368c7146` log block | +| 4 | Claude + Daytona + client tool (refusal) | ok:false with the new client-tools message BEFORE any sandbox | `ok:false` in **29 ms** with byte-exact `REMOTE_CLIENT_TOOLS_UNSUPPORTED_MESSAGE` ("Client tools are not supported for a non-Pi harness on a remote sandbox: ... Tracked in docs/design/agent-workflows/projects/in-sandbox-tool-mcp/."). Zero `sandbox_start.*daytona` log lines in the window — no sandbox created | **PASS** | `cell4-*.response.json` | +| 5 | Claude + Daytona + NO tools (negative) | no shim upload/advertisement; session proceeds; credit failure | `tools=0 executableTools=0`; NO `tool-mcp`/`advertised`/shim lines for its sandbox `daytona/b0eecdce-...`; `create_session ms=1369` succeeded; then the same credit error (5.1 s). Shim path activates only with executable tools | **PASS** | `cell5-*.response.json`; sandbox `b0eecdce` log block | +| 6 | Reserved name `agenta-tools` refusal | ok:false with the reserved-name message | `ok:false` in 28 ms with byte-exact `RESERVED_MCP_SERVER_NAME_MESSAGE` ("MCP server name 'agenta-tools' is reserved for Agenta's internal gateway-tool channel (permission rules are rendered against it); rename the MCP server.") | **PASS** | `cell6-*.response.json` | +| 7 | Warm/restart cells (live session, park-to-stopped restart, tool-set change) | — | Not attempted: a warm second turn needs a successful first turn, and no funded Anthropic key exists tonight | **BLOCKED** | The slice-0 spike ([spike-restart.md](spike-restart.md)) proved the restart respawn against the real `resumeSession`/`session/load` path: same agentSessionId, new shim pid, full `initialize`+`tools/list` re-handshake | + +Notes on cell 3 evidence granularity: + +- The shim **upload** has no success log by design (`tool-mcp-assets.ts` logs only + failures and THROWS `TOOL_MCP_UNAVAILABLE_MESSAGE` on a missing bundle or failed write). + The absence of that error plus the advertisement line — which is only reachable when + `internalToolMcp` assets exist — is the upload proof. +- The **relay loop start** has no log line either; `startToolRelay` runs unconditionally + before the prompt whenever `toolSpecs.length > 0` + (`engines/sandbox_agent.ts:1840-1860`, `useToolRelay` from `run-plan.ts:620`) and its + stale-sweep `ready` is awaited. The Daytona relay execution path itself was live-proven + in the same session by cell 2 (`relay_pickup ... wake=poll`). + +## Sidecar restart notes + +- **The brief's assumption about the container CMD was wrong.** `docker inspect` shows the + running sidecar's CMD is `mkdir -p /home/agent/.pi/agent && cp -a /pi-agent-ro/. ... && + exec node_modules/.bin/tsx src/server.ts` — it does NOT run + `node scripts/build-extension.mjs`, so a restart alone rebuilds nothing. (The repo's + `docker/Dockerfile.dev` CMD does run the build; this container was started with an + override.) Its image also predates the feature: `/app/scripts/build-extension.mjs` was + the old Pi-extension-only version and `/app/dist/tools/` did not exist. +- Fix applied: rebuilt both bundles on the host (`node scripts/build-extension.mjs` in + `services/runner` → `dist/extensions/agenta.js` + `dist/tools/tool-mcp-stdio.js`, + 9.6 kB), then `docker cp` of `scripts/build-extension.mjs` and `dist/tools/` into the + container, then `docker restart`. Restart was also required for the server code itself: + the old tsx process (started 02:13Z) predated the slice-1 src mtimes (03:54–04:07Z); + `src/` is bind-mounted, so the restart loaded current code. `dist/extensions` is + bind-mounted from the host, so Pi got the fresh extension automatically. +- Verified after restart: `/app/dist/tools/tool-mcp-stdio.js` present, sha256 + `99bc0ad1...` identical host/container; `/health` OK. +- Cost: the restart tore down 1 parked keepalive session belonging to another agent + (`destroyAll count=1` — a Claude local session parked on a Terminal ask-gate). + Coordination-board notes were posted before and after the restart. + +## Daytona teardown + +Four sandboxes were created by these runs, all on the sidecar's account +(`agenta-sandbox-pi` snapshot, target `eu`): + +| Sandbox | Cell | Final state | +|---|---|---| +| `a1d2afda-80eb-44c9-85c2-28f8165091a8` | 2 (first attempt, model not settable) | 404 gone | +| `f12c3c93-8ca5-4f30-a3c3-d781cdeb5c4e` | 2 (pass) | 404 gone | +| `368c7146-4c7d-48bf-8fca-78571b2f1f60` | 3 (mechanism) | 404 gone | +| `b0eecdce-1713-4230-8755-d415235b7d94` | 5 (no tools) | 404 gone | + +All four were already deleted by the runner's own ephemeral teardown (these were +sessionless runs, so they are deleted — not parked — at run end); each id verified 404 by +direct GET, and the paginated account list reports **0 sandboxes** (checked twice, the +second time ~1 h later). Nothing was left to reap manually. + +## Anomalies and side findings + +1. **Sidecar CMD deviation** (above): future QA against this container must `docker cp` + built bundles in; a restart alone does not rebuild. The real images + (`docker/Dockerfile.dev:55`, `docker/Dockerfile.gh:65`) run `pnpm run build:extension` + at image build, so deployed runners will carry `dist/tools/tool-mcp-stdio.js` baked. +2. **Pi's sidecar login is Codex-subscription-only.** `openai/gpt-4o-mini` (the QA + default) is not settable; the run failed loud with `ModelNotSettableError` listing only + `openai-codex/*` ids (the F-007 fail-loud behavior working as designed). Cell 2 used + `openai-codex/gpt-5.4-mini`. +3. **Local Claude gateway calls also ride the file relay** behind the loopback HTTP MCP + channel (`relay_pickup ... wake=activity` on cell 1). Expected wiring, noted because the + relay lines alone do not distinguish local from Daytona — the channel line + (`internal tool MCP server on http://127.0.0.1:...` vs `advertised via the in-sandbox + stdio MCP shim`) is the discriminator. +4. **Concurrent sidecar use during QA**: another agent ran Claude local sessions + (`tools=4` lines, a `gh pr list` approval park) throughout; all evidence above is keyed + by sandbox/session id, not by time window alone. +5. Pre-existing, unrelated: OTel trace export 401 spam in the sidecar logs + (`OTLPExporterError: Unauthorized`). + +## Verdict + +Every testable cell is green: both regressions (Claude+local, Pi+Daytona) fully execute +the gateway tool end-to-end through their unchanged channels, the Claude+Daytona mechanism +cell proves the new shim delivery chain (gate pass → upload → typeless stdio advertisement +→ session created with the entry) with the failure confined upstream of the feature (model +credit), and all three refusal/negative behaviors (client tool, reserved name, no-tools +no-shim) match the slice-1 contract byte-for-byte. The remaining live gap is the funded +model turn on Claude+Daytona (the model actually calling the tool through the shim) plus +the warm-reuse cells — one funded Anthropic key unlocks all of them; the lifecycle risk +they cover is already pinned by the slice-0 spike. diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike-restart.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike-restart.md new file mode 100644 index 0000000000..8334efbfb5 --- /dev/null +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike-restart.md @@ -0,0 +1,124 @@ +# Spike: does session/load respawn the in-sandbox stdio MCP shim after a Daytona restart? + +Slice 0 of the in-sandbox platform-tool MCP project. Run 2026-07-12 against the live +Daytona EU target with the exact pinned stack. Scripts and raw method: `spike/spike-mcp.js` +(the mini stdio MCP server), `spike/spike-restart-driver.ts` (the full create, park, +restart, resume cycle), `spike/spike-adapter-pin.ts` (version probes). + +## Verdict: YES, session/load respawns the shim + +The patched `resumeSession` reached ACP `session/load` (`loadedFromContinuity=true`, same +`agentSessionId` before and after the VM stop/start), and the Claude ACP adapter respawned +the stdio MCP subprocess as part of handling the load: a fresh process appeared with a new +pid, received `initialize`, `notifications/initialized`, and `tools/list`, and stayed +alive. No special handling is needed in the engine. + +**Implementation consequence: A2 is locked.** The typeless `{name, command, args, env}` +entry in `sessionInit.mcpServers` works on both `session/new` and `session/load`. The +engine's existing resume path (`engines/sandbox_agent.ts`, the `persist.updateSession` + +`sandbox.resumeSession(localSessionId)` block) needs no fork for sessions that carry the +internal shim entry. + +## The pinned stack (as observed inside the sandbox) + +| Component | Version | How observed | +|---|---|---| +| Daytona snapshot | `agenta-sandbox-pi` (target `eu`) | `SANDBOX_AGENT_DAYTONA_SNAPSHOT` in the ee dev env file | +| In-sandbox daemon | `sandbox-agent 0.5.0-rc.2` (`/usr/local/bin/sandbox-agent`) | `sandbox-agent --version` | +| Claude ACP adapter | `@zed-industries/claude-agent-acp 0.22.2` | `agent_processes/claude/node_modules/@zed-industries/claude-agent-acp/package.json` | +| Claude Agent SDK | `@anthropic-ai/claude-agent-sdk 0.2.76` | same node_modules tree | +| ACP SDK | `@agentclientprotocol/sdk 0.16.1` | same node_modules tree | +| In-sandbox Node | v22.22.1 | `node --version` | +| Runner client lib | `sandbox-agent@0.4.2` + `patches/sandbox-agent@0.4.2.patch` | `services/runner/package.json` `patchedDependencies` | + +The adapter lives at +`/home/sandbox/.local/share/sandbox-agent/bin/agent_processes/claude/node_modules/.bin/claude-agent-acp` +and runs as a long-lived process per live connection; it spawns a `claude` (Agent SDK) +child per session. + +## Observed behavior, step by step + +Sandbox `daytona/f686d918-1f49-4285-b89f-13e912641500`, local session id +`spike-restart:claude`, cwd `/home/sandbox/agenta/spike-cwd`, MCP entry +`{name: "agenta-tools", command: "node", args: ["/home/sandbox/agenta/spike/spike-mcp.js"], env: []}` +(no `type` field). + +1. **createSession (session/new)** returned `agentSessionId=bf7709ea-0556-4760-b20b-13e9d47f6a24`. + Within ~1.2s the shim log showed spawn #1, driven to readiness immediately (not lazily + at first tool use): + + ``` + spawned pid=145 at=2026-07-12T03:21:23.112Z + pid=145 method=initialize + pid=145 method=notifications/initialized + pid=145 method=tools/list + ``` + + A `/proc` scan confirmed pid 145 (`node /home/sandbox/agenta/spike/spike-mcp.js`) live, + next to the adapter (pid 112 `claude-agent-acp`) and its SDK child (pid 129 `claude`). + +2. **Park** (engine order: `destroySession` then `pauseSandbox`) stopped the VM; Daytona + reported `state=stopped` ~7s later. + +3. **Reconnect** (`SandboxAgent.start({..., sandboxId})`) restarted the VM and the daemon. + Before resume: the shim log still held only the pid-145 lines (disk survived the stop), + and no shim process was running. So nothing respawns at daemon restart alone. + +4. **Resume** (fresh persist seeded with the prior `agentSessionId`, then + `resumeSession("spike-restart:claude")`): + + ``` + resumeSession ok agentSessionId=bf7709ea-... prior=bf7709ea-... loadedFromContinuity=true + ``` + + ~1s later the log showed spawn #2 with a NEW pid, fully re-initialized: + + ``` + spawned pid=147 at=2026-07-12T03:21:36.399Z + pid=147 method=initialize + pid=147 method=notifications/initialized + pid=147 method=tools/list + ``` + + `/proc` confirmed pid 147 live, with a fresh adapter (pid 114) and `claude` child + (pid 131). The respawn is tied to `session/load` itself, not to any prompt: no turn ran + between reconnect and the spawn line. + +## Limitations and side findings + +- **No model-visible tool call was exercised.** The only Anthropic key available + (`ANTHROPIC_API_KEY` in the ee dev env) has no credit: both one-turn prompts failed with + `Internal error: Credit balance is too low` before any tokens ran. The respawn question + is adapter lifecycle behavior, not model behavior, and the log evidence (initialize + + tools/list received by the new pid on the load path, twice symmetric with session/new) + answers it. A funded key would let a follow-up prove the last inch (the model calling + `spike_echo` after restart). +- **MCP servers spawn eagerly.** The adapter launches and initializes stdio MCP servers at + session creation AND at session load, before any prompt. The shim will therefore be + running (and its tools listable) as soon as the session exists. +- **Stale `sandboxId` after park creates a fresh sandbox silently.** `pauseSandbox()` + clears the handle's provider refs, so reading `sandbox.sandboxId` after park yields + `undefined`, and `SandboxAgent.start` with `sandboxId: undefined` quietly creates a new + VM (spike iteration 2 hit this). The engine is safe because it persists the pointer via + `writeSandboxPointer` before parking, but any new code must capture the id pre-park. +- **`destroySession` before park does not break resume.** The engine's park order + (session/cancel then stop) leaves the Claude session loadable; `session/load` restored + the same `agentSessionId`. +- **Snapshot env naming.** The runner code reads `DAYTONA_SNAPSHOT`, but in the ee dev env + file that name points at the API code-evaluator snapshot (`daytona-small`, no daemon). + The sandbox-agent snapshot is under `SANDBOX_AGENT_DAYTONA_SNAPSHOT=agenta-sandbox-pi`. + The deployed ee-dev runner container currently shows `DAYTONA_SNAPSHOT=daytona-small` + with `SANDBOX_AGENT_PROVIDER=local`, so Daytona runs from that container would pick the + daemon-less snapshot if enabled; worth a config sweep, tracked outside this spike. + +## Teardown verification + +All sandboxes created across every driver iteration were deleted and verified gone by +direct `get` (404) and a full account list: + +- `95389f3f-6844-49e5-b4c2-f7af60807a94` (iteration 2, phase A): gone +- `ca1a5af0-4547-4501-8616-461501513ed7` (iteration 2, accidental fresh create): gone +- `f686d918-1f49-4285-b89f-13e912641500` (iteration 3, the evidence run): gone +- `1720bab9-f9c4-4d5a-a5df-59525fe980ba` (adapter pin probe): gone + +Account sandbox list after the spike: **0 sandboxes**. diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-adapter-pin.ts b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-adapter-pin.ts new file mode 100644 index 0000000000..480d4c08d5 --- /dev/null +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-adapter-pin.ts @@ -0,0 +1,94 @@ +/** + * One-off probe: record the Claude ACP adapter + Claude Code versions baked into the + * runner's Daytona snapshot (agenta-sandbox-pi). Creates a sandbox, probes, deletes it. + * + * Run from services/runner: + * pnpm exec tsx ../../docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-adapter-pin.ts + */ +import { readFileSync } from "node:fs"; +import { pathToFileURL } from "node:url"; + +const REPO = "/home/mahmoud/code/agenta"; +const RUNNER = `${REPO}/services/runner`; +const ENV_LOCAL = `${REPO}/hosting/docker-compose/ee/.env.ee.dev.local`; + +function parseEnvFile(path: string): Record { + const out: Record = {}; + for (const raw of readFileSync(path, "utf-8").split("\n")) { + const line = raw.trim(); + if (!line || line.startsWith("#")) continue; + const eq = line.indexOf("="); + if (eq <= 0) continue; + out[line.slice(0, eq).trim()] = line.slice(eq + 1).trim(); + } + return out; +} + +async function main() { + const local = parseEnvFile(ENV_LOCAL); + for (const key of ["DAYTONA_API_KEY", "DAYTONA_API_URL", "DAYTONA_TARGET", "DAYTONA_SNAPSHOT"]) { + const value = local[`SANDBOX_AGENT_${key}`] || local[key]; + if (!value) throw new Error(`missing ${key}`); + process.env[key] = value; + } + process.env.AGENTA_AGENT_SANDBOX_PI_INSTALLED = "false"; + console.log(`snapshot=${process.env.DAYTONA_SNAPSHOT} target=${process.env.DAYTONA_TARGET}`); + + const { SandboxAgent, InMemorySessionPersistDriver } = await import( + pathToFileURL(`${RUNNER}/node_modules/sandbox-agent/dist/index.js`).href + ); + const { buildSandboxProvider } = await import( + pathToFileURL(`${RUNNER}/src/engines/sandbox_agent/provider.ts`).href + ); + const { createCookieFetch } = await import( + pathToFileURL(`${RUNNER}/src/engines/sandbox_agent/daytona.ts`).href + ); + + let sandbox: any; + try { + sandbox = await SandboxAgent.start({ + sandbox: buildSandboxProvider("daytona", {}, undefined, {}, {}, undefined), + persist: new InMemorySessionPersistDriver(), + fetch: createCookieFetch(), + }); + console.log(`sandboxId=${sandbox.sandboxId}`); + // Creating a claude session materializes the adapter under agent_processes/claude. + await sandbox.createSession({ id: "pin:claude", agent: "claude", cwd: "/home/sandbox" }); + const probes = [ + "sandbox-agent --version", + "AGENT_DIR=/home/sandbox/.local/share/sandbox-agent/bin/agent_processes/claude; " + + "ls $AGENT_DIR $AGENT_DIR/node_modules 2>&1 | head -30", + "AGENT_DIR=/home/sandbox/.local/share/sandbox-agent/bin/agent_processes/claude; " + + "for f in $AGENT_DIR/package.json $AGENT_DIR/node_modules/*/package.json " + + "$AGENT_DIR/node_modules/@*/*/package.json; do " + + '[ -f "$f" ] && echo "== $f" && grep -m1 \'"name"\' "$f" && grep -m1 \'"version"\' "$f"; ' + + "done 2>/dev/null | head -60", + "node --version; claude --version 2>&1 || " + + "/home/sandbox/.local/share/sandbox-agent/bin/agent_processes/claude/node_modules/.bin/claude --version 2>&1", + ]; + for (const script of probes) { + const res = await sandbox.runProcess({ + command: "sh", + args: ["-lc", script], + timeoutMs: 30_000, + }); + console.log(`$ ${script.slice(0, 80)}\n${res?.stdout ?? ""}${res?.stderr ?? ""}`); + } + } finally { + try { + await sandbox?.destroySandbox(); + console.log("destroySandbox ok"); + } catch (err) { + console.log(`destroySandbox failed: ${(err as Error).message}`); + } + await sandbox?.dispose?.().catch(() => {}); + } +} + +main().then( + () => process.exit(0), + (err) => { + console.error(String(err?.stack ?? err)); + process.exit(1); + }, +); diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-mcp.js b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-mcp.js new file mode 100644 index 0000000000..7adbd0ec51 --- /dev/null +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-mcp.js @@ -0,0 +1,111 @@ +#!/usr/bin/env node +/** + * Minimal stdio MCP server for the in-sandbox platform-tool spike. + * + * Speaks newline-delimited JSON-RPC on stdin/stdout (the MCP stdio transport). + * Exposes ONE tool, `spike_echo`. On every spawn it appends a `spawned pid=...` + * line to LOG_PATH, and it logs each JSON-RPC method it receives, so the spike + * driver can prove (from outside the process) whether the Claude ACP adapter + * respawned it after a Daytona stop/start cycle. + * + * stdout carries ONLY JSON-RPC. All diagnostics go to the log file and stderr. + */ +"use strict"; + +const fs = require("node:fs"); +const path = require("node:path"); +const readline = require("node:readline"); + +const LOG_PATH = process.env.SPIKE_MCP_LOG || "/home/sandbox/agenta/spike-mcp.log"; + +function logLine(text) { + try { + fs.mkdirSync(path.dirname(LOG_PATH), { recursive: true }); + fs.appendFileSync(LOG_PATH, `${text}\n`); + } catch (err) { + process.stderr.write(`spike-mcp log write failed: ${err && err.message}\n`); + } +} + +logLine(`spawned pid=${process.pid} at=${new Date().toISOString()}`); + +function reply(id, result) { + process.stdout.write(`${JSON.stringify({ jsonrpc: "2.0", id, result })}\n`); +} + +function replyError(id, code, message) { + process.stdout.write( + `${JSON.stringify({ jsonrpc: "2.0", id, error: { code, message } })}\n`, + ); +} + +const TOOLS = [ + { + name: "spike_echo", + description: + "Echo the given text back, tagged with the MCP server's process id. " + + "Use whenever asked to call spike_echo.", + inputSchema: { + type: "object", + properties: { text: { type: "string", description: "Text to echo back." } }, + required: ["text"], + }, + }, +]; + +const rl = readline.createInterface({ input: process.stdin, terminal: false }); + +rl.on("line", (line) => { + const trimmed = line.trim(); + if (!trimmed) return; + let message; + try { + message = JSON.parse(trimmed); + } catch { + logLine(`pid=${process.pid} unparseable line (${trimmed.length} bytes)`); + return; + } + const { id, method, params } = message; + logLine(`pid=${process.pid} method=${method ?? "(response)"} at=${new Date().toISOString()}`); + if (method === undefined) return; // a response to something we sent (we send nothing) + + if (method === "initialize") { + reply(id, { + protocolVersion: (params && params.protocolVersion) || "2024-11-05", + capabilities: { tools: {} }, + serverInfo: { name: "agenta-spike-mcp", version: "0.0.1" }, + }); + return; + } + if (method === "notifications/initialized" || method.startsWith("notifications/")) { + return; // notifications get no response + } + if (method === "ping") { + reply(id, {}); + return; + } + if (method === "tools/list") { + reply(id, { tools: TOOLS }); + return; + } + if (method === "tools/call") { + const toolName = params && params.name; + if (toolName !== "spike_echo") { + replyError(id, -32602, `unknown tool: ${toolName}`); + return; + } + const text = (params.arguments && params.arguments.text) || ""; + logLine(`pid=${process.pid} tools/call spike_echo text=${JSON.stringify(text)}`); + reply(id, { + content: [{ type: "text", text: `spike_echo pid=${process.pid} echoed: ${text}` }], + isError: false, + }); + return; + } + if (id !== undefined) replyError(id, -32601, `method not implemented: ${method}`); +}); + +rl.on("close", () => { + logLine(`pid=${process.pid} stdin closed at=${new Date().toISOString()}`); + process.exit(0); +}); diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-restart-driver.ts b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-restart-driver.ts new file mode 100644 index 0000000000..7144c993de --- /dev/null +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-restart-driver.ts @@ -0,0 +1,412 @@ +/** + * Slice-0 spike driver: does the pinned Claude ACP adapter RESPAWN a stdio MCP + * subprocess on the `session/load` (resume) path after a Daytona VM stop/restart? + * + * Run from services/runner: + * pnpm exec tsx ../../docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike/spike-restart-driver.ts + * pnpm exec tsx .../spike-restart-driver.ts --teardown # recovery only + * + * Mirrors the engine (services/runner/src/engines/sandbox_agent.ts) exactly: + * - provider via buildSandboxProvider("daytona", ...) with the harness key in the + * `secrets` slot (the engine's plan.modelEnvironment -> Daytona create envVars), + * - SandboxAgent.start({sandbox, persist, fetch: createCookieFetch()}), + * - createSession with a TYPELESS stdio mcpServers entry {name, command, args, env}, + * - park = destroySession + sandbox.pauseSandbox(), + * - resume = fresh persist seeded via persist.updateSession({... agentSessionId}) + * then sandbox.resumeSession(localSessionId) (the patched session/load path). + * + * NEVER prints secret values. Env is read from the ee dev env files by NAME only. + */ +import { readFileSync, writeFileSync, rmSync, existsSync } from "node:fs"; +import { createRequire } from "node:module"; +import { pathToFileURL } from "node:url"; + +const REPO = "/home/mahmoud/code/agenta"; +const RUNNER = `${REPO}/services/runner`; +const ENV_LOCAL = `${REPO}/hosting/docker-compose/ee/.env.ee.dev.local`; +const ENV_DEV = `${REPO}/hosting/docker-compose/ee/.env.ee.dev`; +const SPIKE_DIR = `${REPO}/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/spike`; +const STATE_FILE = "/tmp/spike-restart-state.json"; + +const SANDBOX_MCP_PATH = "/home/sandbox/agenta/spike/spike-mcp.js"; +const SANDBOX_LOG_PATH = "/home/sandbox/agenta/spike-mcp.log"; +const CWD = "/home/sandbox/agenta/spike-cwd"; +const LOCAL_SESSION_ID = "spike-restart:claude"; +const MCP_ENTRY = { + name: "agenta-tools", + command: "node", + args: [SANDBOX_MCP_PATH], + env: [] as Array<{ name: string; value: string }>, +}; + +// The snapshot has no pgrep/ps; scan /proc cmdlines instead. +const PROC_SCAN = + 'found=0; for p in /proc/[0-9]*; do c=$(tr "\\0" " " < "$p/cmdline" 2>/dev/null); ' + + 'case "$c" in *spike-mcp*) echo "pid=${p#/proc/} cmd=$c"; found=1;; esac; done; ' + + '[ "$found" -eq 1 ] || echo NO_PROCESS'; +const PROC_SCAN_CLAUDE = + 'for p in /proc/[0-9]*; do c=$(tr "\\0" " " < "$p/cmdline" 2>/dev/null); ' + + 'case "$c" in *claude*|*acp*) echo "pid=${p#/proc/} cmd=$c";; esac; done; true'; + +function step(name: string) { + console.log(`\n===== ${new Date().toISOString()} ${name} =====`); +} + +/** Parse KEY=VALUE lines; last occurrence wins; strips surrounding quotes. */ +function parseEnvFile(path: string): Record { + const out: Record = {}; + for (const raw of readFileSync(path, "utf-8").split("\n")) { + const line = raw.trim(); + if (!line || line.startsWith("#")) continue; + const eq = line.indexOf("="); + if (eq <= 0) continue; + const key = line.slice(0, eq).trim(); + let value = line.slice(eq + 1).trim(); + if ( + (value.startsWith('"') && value.endsWith('"')) || + (value.startsWith("'") && value.endsWith("'")) + ) { + value = value.slice(1, -1); + } + out[key] = value; + } + return out; +} + +// sandbox-agent ships ESM-only "import" exports, so require.resolve cannot see them; +// import the runner's installed (pnpm-patched) copies by file path instead. +const runnerRequire = createRequire(`${RUNNER}/package.json`); +async function importFromRunner(spec: string): Promise { + const byPath: Record = { + "sandbox-agent": `${RUNNER}/node_modules/sandbox-agent/dist/index.js`, + }; + const resolved = byPath[spec] ?? runnerRequire.resolve(spec); + return import(pathToFileURL(resolved).href); +} + +function sliceText(value: unknown, max = 500): string { + const text = typeof value === "string" ? value : JSON.stringify(value); + return text && text.length > max ? `${text.slice(0, max)}...[truncated]` : String(text); +} + +async function main() { + // ---- env (names only, never printed) ------------------------------------------------- + const local = parseEnvFile(ENV_LOCAL); + const dev = parseEnvFile(ENV_DEV); + // The runner's Daytona config: SANDBOX_AGENT_DAYTONA_* names the sandbox-agent snapshot + // (agenta-sandbox-pi, daemon + Claude adapter baked); plain DAYTONA_SNAPSHOT in this env + // file is the API code-evaluator snapshot (daytona-small, no daemon). The runner code + // reads process.env.DAYTONA_*, so map the SANDBOX_AGENT_ values onto those names. + for (const key of ["DAYTONA_API_KEY", "DAYTONA_API_URL", "DAYTONA_TARGET", "DAYTONA_SNAPSHOT"]) { + const value = local[`SANDBOX_AGENT_${key}`] || local[key]; + if (!value) throw new Error(`missing ${key} in .env.ee.dev.local`); + process.env[key] = value; + } + const anthropicKey = dev.ANTHROPIC_API_KEY || ""; + console.log( + `env loaded: snapshot=${process.env.DAYTONA_SNAPSHOT} target=${process.env.DAYTONA_TARGET} ` + + `anthropic_key=${anthropicKey ? "present" : "ABSENT"}`, + ); + // Mirror the dev compose: the snapshot bakes what it needs; skip the Pi CLI install path. + process.env.AGENTA_AGENT_SANDBOX_PI_INSTALLED = "false"; + + // ---- modules (resolved from services/runner so the pnpm-patched pin is used) --------- + const { SandboxAgent, InMemorySessionPersistDriver } = await importFromRunner("sandbox-agent"); + const { Daytona } = await importFromRunner("@daytonaio/sdk"); + const { buildSandboxProvider } = await import( + pathToFileURL(`${RUNNER}/src/engines/sandbox_agent/provider.ts`).href + ); + const { createCookieFetch } = await import( + pathToFileURL(`${RUNNER}/src/engines/sandbox_agent/daytona.ts`).href + ); + + const daytonaClient = new Daytona(); + + // ---- teardown-only recovery mode ------------------------------------------------------ + const teardownArg = process.argv.indexOf("--teardown"); + if (teardownArg !== -1) { + const rawId = process.argv[teardownArg + 1]; + if (!rawId) throw new Error("--teardown needs a raw sandbox id"); + await deleteAndVerify(daytonaClient, rawId); + return; + } + + // The harness key rides the engine's own mechanism: the `secrets` argument = + // plan.modelEnvironment -> buildDaytonaCreate -> Daytona create envVars. + const secrets = anthropicKey ? { ANTHROPIC_API_KEY: anthropicKey } : {}; + const mcpServerSource = readFileSync(`${SPIKE_DIR}/spike-mcp.js`, "utf-8"); + + let sandboxA: any; + let sandboxB: any; + let rawSandboxId = ""; + try { + // =================== PHASE A: create, verify spawn #1 ================================ + step("A1 startSandboxAgent (mode=create)"); + const persistA = new InMemorySessionPersistDriver(); + sandboxA = await SandboxAgent.start({ + sandbox: buildSandboxProvider("daytona", {}, undefined, {}, secrets, undefined), + persist: persistA, + fetch: createCookieFetch(), + }); + // Capture the prefixed id NOW: pauseSandbox() clears the handle's provider refs, so + // sandboxA.sandboxId is undefined after park (this is also what the engine persists + // via writeSandboxPointer before parking). + const prefixedSandboxId = String(sandboxA.sandboxId); + console.log(`sandboxId=${prefixedSandboxId}`); + rawSandboxId = prefixedSandboxId.replace(/^daytona\//, ""); + writeFileSync(STATE_FILE, JSON.stringify({ rawSandboxId }, null, 2)); + + step("A2 adapter/daemon pin probes"); + try { + const agents = await sandboxA.listAgents(); + const summary = (agents?.agents ?? agents ?? []).map?.((a: any) => ({ + name: a.name ?? a.id, + version: a.version, + installed: a.installed, + command: a.command, + })); + console.log(`agents: ${sliceText(summary, 1500)}`); + } catch (err) { + console.log(`listAgents failed: ${(err as Error).message}`); + } + await probe(sandboxA, "sandbox-agent --version 2>&1; which sandbox-agent 2>&1"); + await probe( + sandboxA, + "for f in $(find /usr/local/lib /usr/lib /opt /home/sandbox -maxdepth 6 -name package.json " + + "\\( -path '*claude*' -o -path '*acp*' \\) -not -path '*/node_modules/*/node_modules/*' " + + "2>/dev/null | head -8); do echo \"== $f\"; head -c 400 \"$f\"; echo; done", + ); + + step("A3 upload spike MCP server + cwd"); + await sandboxA.mkdirFs({ path: "/home/sandbox/agenta/spike" }); + await sandboxA.mkdirFs({ path: CWD }); + await sandboxA.writeFsFile({ path: SANDBOX_MCP_PATH }, mcpServerSource); + console.log(`uploaded ${SANDBOX_MCP_PATH}`); + + step("A4 createSession (agent=claude, typeless stdio mcp entry)"); + const sessionA = await sandboxA.createSession({ + id: LOCAL_SESSION_ID, + agent: "claude", + cwd: CWD, + sessionInit: { cwd: CWD, mcpServers: [MCP_ENTRY] }, + }); + const agentSessionIdA = sessionA.agentSessionId; + console.log(`localSessionId=${sessionA.id} agentSessionId=${agentSessionIdA}`); + writeFileSync(STATE_FILE, JSON.stringify({ rawSandboxId, agentSessionIdA }, null, 2)); + + step("A5 verify spawn #1 (log + pgrep)"); + await readSpikeLog(sandboxA, "after createSession"); + await probe(sandboxA, PROC_SCAN); + await probe(sandboxA, PROC_SCAN_CLAUDE); + await probe( + sandboxA, + "find / -xdev -maxdepth 8 -name package.json -path \"*claude*\" 2>/dev/null | head -5", + ); + + // One short cheap prompt turn to prove end-to-end tool advertisement. + if (anthropicKey) { + step("A6 prompt turn #1 (spike_echo hello-phase1)"); + await runOneTurn(sessionA, "hello-phase1"); + await readSpikeLog(sandboxA, "after turn #1"); + await probe(sandboxA, PROC_SCAN); + } + + // =================== PHASE B: park (stop) the VM ===================================== + step("B1 park: destroySession + pauseSandbox (engine order)"); + await sandboxA.destroySession(sessionA.id).catch((err: Error) => { + console.log(`destroySession failed (continuing): ${err.message}`); + }); + await sandboxA.pauseSandbox(); + await sandboxA.dispose().catch(() => {}); + console.log("pauseSandbox returned"); + + step("B2 wait for state=stopped"); + for (let i = 0; i < 60; i++) { + const sb = await daytonaClient.get(rawSandboxId); + const state = String(sb.state ?? "unknown").toLowerCase(); + console.log(`state=${state}`); + if (state === "stopped") break; + await new Promise((r) => setTimeout(r, 2000)); + if (i === 59) throw new Error("sandbox never reached stopped"); + } + + // =================== PHASE C: restart + REAL resume path ============================= + step("C1 startSandboxAgent (mode=reconnect, by sandboxId)"); + const persistB = new InMemorySessionPersistDriver(); + sandboxB = await SandboxAgent.start({ + sandbox: buildSandboxProvider("daytona", {}, undefined, {}, secrets, undefined), + persist: persistB, + fetch: createCookieFetch(), + sandboxId: prefixedSandboxId, + }); + console.log(`reconnected sandboxId=${sandboxB.sandboxId}`); + if (String(sandboxB.sandboxId) !== prefixedSandboxId) { + throw new Error( + `reconnect returned a DIFFERENT sandbox (${sandboxB.sandboxId} != ${prefixedSandboxId})`, + ); + } + + step("C2 note restart marker + pre-resume log state"); + await probe(sandboxB, `date -Is; ${PROC_SCAN}`); + await readSpikeLog(sandboxB, "pre-resume (should have no fresh spawn yet)"); + + step("C3 seed persist + resumeSession (patched session/load path)"); + await persistB.updateSession({ + id: LOCAL_SESSION_ID, + agent: "claude", + agentSessionId: agentSessionIdA, + lastConnectionId: "", + createdAt: Date.now(), + sessionInit: { cwd: CWD, mcpServers: [MCP_ENTRY] }, + }); + let sessionB: any; + let loadedFromContinuity = false; + try { + sessionB = await sandboxB.resumeSession(LOCAL_SESSION_ID); + loadedFromContinuity = sessionB.agentSessionId === agentSessionIdA; + console.log( + `resumeSession ok agentSessionId=${sessionB.agentSessionId} ` + + `prior=${agentSessionIdA} loadedFromContinuity=${loadedFromContinuity}`, + ); + } catch (err) { + console.log(`resumeSession FAILED: ${(err as Error).message}`); + } + + step("C4 verify respawn (log + pgrep)"); + await new Promise((r) => setTimeout(r, 3000)); + await readSpikeLog(sandboxB, "post-resume"); + await probe(sandboxB, PROC_SCAN); + await probe(sandboxB, PROC_SCAN_CLAUDE); + + if (anthropicKey && sessionB) { + step("C5 prompt turn #2 (spike_echo hello-phase2)"); + await runOneTurn(sessionB, "hello-phase2"); + await readSpikeLog(sandboxB, "after turn #2"); + await probe(sandboxB, PROC_SCAN); + } + } finally { + // =================== TEARDOWN (mandatory) ============================================= + step("TEARDOWN"); + const live = sandboxB ?? sandboxA; + try { + if (live) { + await live.destroySandbox(); + console.log("destroySandbox ok"); + } + } catch (err) { + console.log(`destroySandbox failed: ${(err as Error).message}`); + } + try { + await live?.dispose?.(); + } catch {} + if (rawSandboxId) await deleteAndVerify(daytonaClient, rawSandboxId); + if (existsSync(STATE_FILE)) rmSync(STATE_FILE); + } +} + +async function probe(sandbox: any, script: string): Promise { + try { + const res = await sandbox.runProcess({ + command: "sh", + args: ["-lc", script], + timeoutMs: 30_000, + }); + console.log( + `$ ${script.slice(0, 90)}\nexit=${res?.exitCode} stdout=${sliceText(res?.stdout, 1500)}` + + (res?.stderr ? ` stderr=${sliceText(res?.stderr, 300)}` : ""), + ); + } catch (err) { + console.log(`probe failed (${script.slice(0, 60)}): ${(err as Error).message}`); + } +} + +async function readSpikeLog(sandbox: any, label: string): Promise { + try { + const bytes = await sandbox.readFsFile({ path: SANDBOX_LOG_PATH }); + const text = Buffer.from(bytes).toString("utf-8"); + console.log(`--- spike-mcp.log (${label}) ---\n${text}--- end log ---`); + } catch (err) { + console.log(`spike-mcp.log unreadable (${label}): ${(err as Error).message}`); + } +} + +async function runOneTurn(session: any, marker: string): Promise { + const offPerm = session.onPermissionRequest((req: any) => { + const reply = (req.availableReplies ?? []).find((r: string) => r !== "reject") ?? "once"; + console.log( + `permission request tool=${sliceText(req.toolCall?.title ?? req.toolCall, 120)} -> ${reply}`, + ); + session.respondPermission(req.id, reply).catch((err: Error) => { + console.log(`respondPermission failed: ${err.message}`); + }); + }); + const offEvent = session.onEvent((event: any) => { + const payload = event?.payload ?? event; + const kind = payload?.sessionUpdate ?? payload?.update?.sessionUpdate ?? event?.type ?? "?"; + console.log(` event ${sliceText(kind, 60)}: ${sliceText(payload, 220)}`); + }); + try { + // Cheapest available model; non-strict like the engine's applyModel (rejection tolerated). + for (const model of ["claude-haiku-4-5", "claude-3-5-haiku-latest", "haiku"]) { + try { + await session.setModel(model); + console.log(`model set to ${model}`); + break; + } catch {} + } + const response = await Promise.race([ + session.prompt([ + { + type: "text", + text: + `Call the spike_echo tool (from the agenta-tools MCP server) with text '${marker}' ` + + `and reply with only the tool's output.`, + }, + ]), + new Promise((_, reject) => + setTimeout(() => reject(new Error("prompt timed out after 180s")), 180_000), + ), + ]); + console.log(`prompt done: ${sliceText(response, 300)}`); + } catch (err) { + console.log(`prompt turn failed: ${(err as Error).message}`); + } finally { + offPerm?.(); + offEvent?.(); + } +} + +async function deleteAndVerify(daytonaClient: any, rawId: string): Promise { + try { + const sb = await daytonaClient.get(rawId); + await sb.delete(); + console.log(`daytona delete issued for ${rawId}`); + } catch (err) { + console.log(`daytona get/delete (${rawId}): ${(err as Error).message}`); + } + // Verify absence. + for (let i = 0; i < 15; i++) { + try { + const sb = await daytonaClient.get(rawId); + const state = String(sb.state ?? "?").toLowerCase(); + console.log(`still present state=${state}`); + if (state === "destroyed") break; + await new Promise((r) => setTimeout(r, 2000)); + } catch (err) { + console.log(`verified gone: get(${rawId}) -> ${(err as Error).message}`); + return; + } + } + console.log("WARNING: sandbox may still exist; re-run with --teardown"); +} + +main().then( + () => { + console.log("\nspike driver finished"); + process.exit(0); + }, + (err) => { + console.error(`\nspike driver failed: ${err?.stack ?? err}`); + process.exit(1); + }, +); diff --git a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md index 876c799c3a..bb2ca3e650 100644 --- a/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md +++ b/docs/design/agent-workflows/projects/in-sandbox-tool-mcp/status.md @@ -1,82 +1,94 @@ # Status -**State: DESIGN ONLY, awaiting owner review.** No runtime code changed. The PR carrying this -workspace is the review surface; the owner will interview on it. +**State: IMPLEMENTED, draft PR up for review.** Slices 0-1 landed on lane +`feat-in-sandbox-tool-mcp` (stacked on `feat-event-driven-tool-relay`, PR #5243, the +relay-module prerequisite). Slice 2 ran: every runnable live cell is green; the cells that +need a real Claude model turn on Daytona are credit-blocked and recorded as the explicit +merge gate below. Evidence: [spike-restart.md](spike-restart.md) (slice 0) and +[qa-slice2.md](qa-slice2.md) (slice 2). + +## Merge gate (do not merge until) + +The Claude+Daytona EXECUTION cells (a real gateway tool called by the model, plus the +warm-live-reuse and stopped-VM-restart turns) are blocked on Daytona/Anthropic credit. +The delivery mechanism is proven live (see qa-slice2 cell 3: no gate refusal, assets +uploaded, `agenta-tools` stdio entry advertised, session created, adapter spawned the +shim) and the restart respawn is proven by the slice-0 spike, but "the tool executes and +the result reaches the answer" has not run end to end on Daytona. Re-run qa-slice2 cells +3 and 7 once credit is restored, then flip the PR to ready. ## Done -- 2026-07-11: workspace created. Prior art read and reconciled (`claude-daytona-tools`, - `remote-tools-delivery`, `mcp-delivery-architecture` including the 2026-07-11 decision in - `gateway-mcp-location.md`, `gateway-tool-mcp`). PR #4873 mined; its stdio implementation - is the revival base. Current code re-verified against the working tree (anchors in - [research.md](research.md)), including the warm-reuse lifecycle from PR #5225 and the - client-tool pause semantics on the local channel. -- Owner decisions of 2026-07-11 encoded in [context.md](context.md): runner-only sandbox - communication (API gateway rejected), user MCP HTTP-only permanently with API-key-header - auth now and OAuth as future work, platform tools via an in-sandbox MCP server, and - unification with Pi as the primary design goal. -- Recommendation written in [plan.md](plan.md): A2 (harness-spawned stdio shim) as the one - transport, shared handler + relay-writer modules with a golden byte-contract test as the - unification path, slices 1-4, and the live-QA matrix including the warm-reuse cells. -- 2026-07-11: cross-consistency review round (requested by the owner alongside his own - review of the event-driven-tool-relay PR) folded in. Corrections: the relay dir is an - ephemeral sibling keyed by `basename(cwd)`, not derived from the durable cwd; the - orphaned-request risk narrowed to warm-continued turns (`workspace.ts:60-66` already - clears cold builds). Additions: the `waitForRelayResponse` seam contract and landing - order with the sibling, the bearer layering rule and ordering with - `mcp-client-tool-continuation`, the unowned Daytona client-tool bridge cross-reference, - the crash-after-write at-least-once note, and the `best_effort` clarification on the - network-off QA cell. Combined landing order: - [../mcp-delivery-architecture/orchestration.md](../mcp-delivery-architecture/orchestration.md). -- 2026-07-11 (late): Codex xhigh review of this workspace folded in. The owner was asleep; - per his standing simplify-aggressively instruction the cuts were adopted rather than - argued, and every adoption is reversible at his review. Verdict: approve A2 - conditionally, reject the earlier scope. Changes: a new slice 0 restart spike gates A2 - (the "correct by construction" claim was wrong; `session/load` may seed persisted - `mcpServers` without respawning subprocesses, and the orphan-exit claim is weakened to an - expectation); relay-module extraction moved out of this project entirely (PR #5232 owns - `relay-client.ts`/`relay-protocol.ts` as its slice 0 and is now an explicit prerequisite, - reversing the ordering the earlier consistency pass wrote); the standalone - transport-neutral `mcp-handler.ts` slice is cut and the unification section rewritten - honestly (Pi never speaks MCP; the real sharing is the relay client and file protocol); - specs move from an unbounded env variable to a file, decided now; v1 cuts moved to - explicit follow-ups (client tools, Codex-on-Daytona, snapshot bake, U2, watch adoption, - mandatory replay capture); warm-reuse edges added (session/load after VM stop, - sanitized-ID collision, bundle-version skew, partial request visibility via #5232's - atomic-rename amendment); security separation made structural (dedicated internal entry - constructor/type, reserved-name rejection for user config, `toAcpMcpServers` never - generalized to stdio); naming fixed (`tool-mcp-stdio.ts`, `internal-tool-mcp-handler.ts` - if a handler exists, upload helpers under `engines/sandbox_agent/`, ACP entry shapes out - of `mcp-bridge.ts`). +- 2026-07-11: workspace created; prior art reconciled; PR #4873 mined as the revival base; + owner decisions encoded in [context.md](context.md); A2 recommendation written. +- 2026-07-11 (late): Codex xhigh review folded in — conditional A2 gated on a slice-0 + restart spike, relay modules handed to PR #5232/#5243, specs moved to a file, v1 cut to + spike + shim + live acceptance. Details in the decision log. +- 2026-07-12 (overnight run): **slice 0 executed live.** Verdict: `session/load` respawns + the stdio MCP subprocess on the pinned adapter (`@zed-industries/claude-agent-acp 0.22.2`, + snapshot `agenta-sandbox-pi`, in-sandbox `sandbox-agent 0.5.0-rc.2`): new pid, fresh + `initialize` + `tools/list`, `loadedFromContinuity=true`. **A2 locked, no engine fork + needed.** Evidence + caveats (Pi login upload is Pi-only; `pauseSandbox()` clears + `sandbox.sandboxId`) in [spike-restart.md](spike-restart.md). +- 2026-07-12: **slice 1 implemented and committed** (4 commits on + `feat-in-sandbox-tool-mcp`): `tools/tool-mcp-stdio.ts` (9.6 kB bundle, NDJSON JSON-RPC, + consumes the shared relay client, specs from a FILE via + `AGENTA_AGENT_TOOLS_PUBLIC_SPECS_FILE`), `engines/sandbox_agent/tool-mcp-assets.ts` + (fail-loud always-write upload, `SANDBOX_AGENT_RELAY_MCP_BUNDLE` override), + the dedicated typeless `agenta-tools` entry constructor in `mcp.ts` (McpServerStdio moved + out of mcp-bridge; `toAcpMcpServers` still cannot emit stdio), the narrowed run-plan gate + (executable tools on Daytona pass; client tools and non-Daytona remotes refuse loud; the + reserved name is refused at declaration + materialization), and — from the review round — + the relay execution guard now built for EVERY harness (deny enforced everywhere; ask + consumes the Pi grant ledger on Pi, passes on MCP harnesses whose own dialog gates the + call; forged-ask residual documented as a follow-up). Lane tip verified standalone: + typecheck clean, 994/994 unit tests green. +- 2026-07-12: **slice 2 ran** ([qa-slice2.md](qa-slice2.md)): Claude+local+gateway PASS + (loopback channel, echo token end to end); Pi+Daytona+gateway PASS (file relay); + Claude+Daytona+gateway cold MECHANISM PASS (no refusal, shim uploaded + advertised, + session created; model turn blocked upstream by credit); Claude+Daytona+client-tool + refusal PASS (29 ms, zero sandboxes); Claude+Daytona no-tools PASS (no shim upload); + reserved-name rejection PASS. Warm/restart cells BLOCKED on credit (spike covers the + respawn mechanism). Teardown verified twice: 0 sandboxes left. +- 2026-07-12: living docs synced in the same PR (runner-to-mcp-server interface page, + interface index, harness-adapters, mcp-models, permission-responder, tools.md, + ground-truth.md, claude-code.md, running-the-agent.md). -## Next +## Known caveats / deferred (each its own decision, see plan.md follow-ups) -1. Owner review of this workspace (interview against - [open-questions.md](open-questions.md), especially the conditional A2 approval and the - specs-file decision). -2. On approval: run slice 0 (the restart spike) first. Implementation waits for PR #5232 - slice 0 (relay module extraction); then slice 1 (#4873 revival over the consumed - modules), then slice 2 (live acceptance). Coordinate lane usage on the agent board. +- Client tools through the shim (Daytona client-tool bridge workspace owns it). +- Ask-grant parity for MCP harnesses in the relay guard (a forged relay file can trigger + an ask-tool without a dialog on the MCP path; deny is enforced runner-side everywhere). +- Shim-side abort: the shim passes no AbortSignal into `relayToolCall`; a harness-cancelled + `tools/call` runs to completion/timeout (relay timeout residue is #5243-owned). +- Codex-on-Daytona, snapshot bake, Pi-as-MCP-client, watch adoption in the shim, replay + capture: unchanged follow-ups. +- Dev-stack operational note: the running dev sidecar containers were built from an image + whose CMD does not rebuild bundles on restart; loading a new shim build needs a + `docker cp` of `scripts/build-extension.mjs` (or the dist file) before the restart. + Deployed images bake the bundle at build time and are unaffected. Also, the deployed + main runner carries `DAYTONA_SNAPSHOT=daytona-small` (the daemon-less code-evaluator + snapshot) because the env file's API-scoped value leaks into the runner service; the + sub-sidecar has the correct `agenta-sandbox-pi`. Worth an env-file sweep. +- QA note: the sidecar's Codex-subscription Pi login cannot set `openai/gpt-4o-mini` + (fail-loud ModelNotSettableError); Pi QA cells use `openai-codex/gpt-5.4-mini`. -## Blockers +## Next -- Implementation waits on PR #5232 slice 0 (relay module extraction), now an explicit - prerequisite. -- Slice 0 and slice 2 need Daytona credit; slice 2 also needs the `pi-agents` project's - live Composio connections (the same live-QA prerequisites recorded in - `claude-daytona-tools/design.md`). +1. Owner review of the draft PR (interview against [open-questions.md](open-questions.md); + the conditional A2 approval is now backed by the spike). +2. Restore Daytona/Anthropic credit, re-run qa-slice2 cells 3 + 7 (execution, warm reuse, + stopped-VM restart, tool-set change, network-off), record them, flip the PR to ready. ## Decision log -- 2026-07-11: transport recommendation flipped from A1 (HTTP loopback, - `claude-daytona-tools`) to A2 (harness-spawned stdio, PR #4873), driven by the warm-reuse - lifecycle (PR #5225) and the existence of tested A2 code. Recorded in - [plan.md](plan.md); awaiting owner confirmation (open question 1). +- 2026-07-11: transport recommendation flipped from A1 (HTTP loopback) to A2 + (harness-spawned stdio, PR #4873), driven by the warm-reuse lifecycle (PR #5225). - 2026-07-11 (late, Codex review fold): A2 approval made conditional on the slice 0 restart spike; relay-module ownership handed to PR #5232; the standalone shared-handler slice cut; - specs delivery decided as a file; v1 reduced to spike + shim + live acceptance with - everything else an explicit follow-up. Adopted while the owner slept, flagged for his - review, reversible. -- 2026-07-11: the daemon-spawned variant from `remote-tools-delivery` recognized as the - same mechanism as A2 (the daemon already forwards session MCP entries; the adapter - spawns them), so no daemon change is requested from the sandbox-agent package. + specs delivery decided as a file; v1 reduced to spike + shim + live acceptance. +- 2026-07-11: the daemon-spawned variant recognized as the same mechanism as A2. +- 2026-07-12: slice-0 spike PASSED; A2 locked unconditionally (no cold-createSession + fallback needed). Relay guard extended to every harness after the implementation review + found the plan's forged-file invariant unimplemented on the new path; ask-grant parity + for MCP harnesses recorded as a follow-up rather than blocking v1.