diff --git a/docs/design/agent-workflows/documentation/tools.md b/docs/design/agent-workflows/documentation/tools.md index eee9fe5245..be1fbfb28f 100644 --- a/docs/design/agent-workflows/documentation/tools.md +++ b/docs/design/agent-workflows/documentation/tools.md @@ -204,9 +204,11 @@ natively. Today that splits cleanly into two paths. (`services/runner/src/tools/tool-mcp-stdio.ts`, bundled by `build:extension`) plus a public-specs JSON file into an ephemeral in-VM dir and advertises them as a stdio entry the harness spawns; the shim's `tools/call` writes relay request files that the runner-side - relay loop executes server-side. The shim carries executable (gateway/callback) tools only; - client tools on a non-Pi remote run are refused up front, and a non-Daytona remote provider - fails closed (see "Client tools" below). The full transport and env contract lives in the + relay loop executes server-side. The shim carries BOTH executable (gateway/callback) AND + `client` tools: an executable call relays inline, and a `client` call parks — the relay writes + a benign "paused" answer so the shim ends its `tools/call` cleanly while the runner ends the + turn (see "Client tools" below). A non-Daytona remote provider still fails closed. The full + transport and env contract lives in the [runner-to-MCP interface page](../interfaces/cross-service/runner-to-mcp-server.md). Both paths funnel execution through one function, `runResolvedTool` in @@ -356,19 +358,23 @@ The pause itself is shared by both delivery paths through one seam - **Pi** calls the tool through its extension; the runner's file relay pauses it (writes no response file) and the seam emits the interaction. -- **Claude** calls the tool over the internal `agenta-tools` MCP server, and the runner pauses it - inside the `tools/call` handler: it emits NO JSON-RPC result and aborts that in-flight request, - so Claude cannot settle the call before the turn ends `paused`. The browser result resumes it - next turn (the MCP handler returns the stored output if the model re-calls). This pause is - local-only: the Daytona stdio shim has no pause path (the relay loop parks a client call and - writes no response file, so the shim would hang until the relay timeout and teach the model - the tool is broken), so a non-Pi remote run carrying a client tool is refused up front - (`REMOTE_CLIENT_TOOLS_UNSUPPORTED_MESSAGE`), never delivered silently. Executable tools DO - deliver on Claude+Daytona through the shim; a remote provider that is not Daytona still - refuses ANY custom tool (`REMOTE_TOOLS_UNSUPPORTED_MESSAGE`) until in-sandbox delivery is - proven there. (The ACP permission gate in `acp-interactions.ts` keeps its own - `kind: "client"` pause branch as a live fallback for a harness that raises a permission gate - carrying a resolved client spec.) +- **Claude on the LOCAL sandbox** calls the tool over the internal `agenta-tools` loopback MCP + server, and the runner pauses it inside the `tools/call` handler: it emits NO JSON-RPC result + and aborts that in-flight request, so Claude cannot settle the call before the turn ends + `paused`. The browser result resumes it next turn (the MCP handler returns the stored output if + the model re-calls). +- **Claude on DAYTONA** calls the tool over the in-sandbox stdio shim. The shim blocks on a relay + answer file, so the runner cannot simply drop the response the way the local path aborts its + request. Instead the relay loop writes a benign "paused" answer (`{ ok: true, paused: true }`, + gated by the run plan's `writePausedAnswer` flag), and the shim returns a non-error wait result + that names the tool and tells the model not to retry. Claude's turn ends cleanly while the + runner ends the turn on the shared pause seam; the browser result returns on the cold-replay + resume turn, exactly as on the local and Pi paths. This closes the Claude+Daytona gap (#5256) + and the residual mixed-set silent drop (#4984). A remote provider that is NOT Daytona still + refuses ANY custom tool (`REMOTE_TOOLS_UNSUPPORTED_MESSAGE`) until in-sandbox delivery is proven + there. (The ACP permission gate in `acp-interactions.ts` keeps its own `kind: "client"` pause + branch as a live fallback for a harness that raises a permission gate carrying a resolved client + spec.) A client tool's `render` hint can be `{ kind: "connect" }` (e.g. `request_connection`), the typed member of `RenderHint` that asks the frontend to draw the connect widget. diff --git a/docs/design/agent-workflows/interfaces/README.md b/docs/design/agent-workflows/interfaces/README.md index 832f0c217b..1720de6399 100644 --- a/docs/design/agent-workflows/interfaces/README.md +++ b/docs/design/agent-workflows/interfaces/README.md @@ -47,7 +47,7 @@ page. `Status` is read from each page's prose: **stable** (wired and unlikely to | [Agent config schema](public-edge/agent-config-schema.md) | public | `agent/schemas.py`, `sdk/utils/types.py`, `agents/dtos.py` (`HARNESS_IDENTITIES`) | stable | `unit/agents/test_dtos_agent_config.py`, `unit/agents/test_harness_identity.py` | | [`/run`](cross-service/service-to-agent-runner.md) | cross-service (the spine) | `protocol.ts`, `utils/wire.py`, `utils/ts_runner.py`, `server.ts`/`cli.ts` | stable (pinned by golden) | `unit/agents/test_wire_contract.py` + `golden/`, `services/agent/tests/unit/wire-contract.test.ts` | | [Runner to harness](cross-service/runner-to-harness.md) | cross-service (ACP) | `engines/sandbox_agent.ts` + `sandbox_agent/{run-plan,capabilities,permissions}.ts` | evolving | `services/agent/tests/unit/sandbox-agent-*.test.ts` | -| [Runner to MCP server](cross-service/runner-to-mcp-server.md) | cross-service | `agents/mcp/`, `engines/sandbox_agent/{mcp,tool-mcp-assets,relay-guard}.ts`, `tools/{mcp-bridge,tool-mcp-http,tool-mcp-stdio,tool-mcp-env,relay,relay-client,relay-protocol,relay-watch}.ts` | evolving (internal channel delivered locally over loopback HTTP and on Daytona via the in-sandbox stdio shim; user stdio disabled; remote client tools deferred) | `services/runner/tests/unit/{mcp-servers,session-mcp-layering,tool-mcp-assets,tool-mcp-stdio,tool-relay-guard}.test.ts` | +| [Runner to MCP server](cross-service/runner-to-mcp-server.md) | cross-service | `agents/mcp/`, `engines/sandbox_agent/{mcp,tool-mcp-assets,relay-guard}.ts`, `tools/{mcp-bridge,tool-mcp-http,tool-mcp-stdio,tool-mcp-env,relay,relay-client,relay-protocol,relay-watch}.ts` | evolving (internal channel delivered locally over loopback HTTP and on Daytona via the in-sandbox stdio shim, `client` tools included — a client call parks via a paused relay answer; user stdio disabled) | `services/runner/tests/unit/{mcp-servers,session-mcp-layering,tool-mcp-assets,tool-mcp-stdio,tool-relay-guard}.test.ts` | | [Runner to tool callback](cross-service/runner-to-tool-callback.md) | cross-service | `tools/{callback,dispatch,direct}.ts`, `apis/fastapi/tools/router.py` (`/tools/call`, `/tools/discover`, `_call_reserved_agenta_tool`), `core/tools/{discovery,service,platform_handlers}.py`, `agent/tools/resolver.py` | evolving (the `call` descriptor is wired and platform ops emit it; the legacy `tools.agenta.find_capabilities` route is deleted; reserved refs now dispatch server handlers, resolution flag-gated off until the runner half lands) | `services/agent/tests/unit/{code-tool,extension-tools}.test.ts`, `api unit/tools/{test_workflow_tool_call,test_discovery,test_platform_handlers}.py`, `unit/agents/platform/test_op_catalog.py` | | [Service and runner trace export](cross-service/service-and-runner-trace-export.md) | cross-service | `agent/tracing.py`, `tracing/otel.ts`, `extensions/agenta.ts` | stable | `services/agent/tests/unit/` | | [Service to vault and tool providers](cross-service/service-to-vault-and-tool-providers.md) | cross-service (external) | `agent/app.py`, `platform/{resolve,connections}.py`, `agents/capabilities.py`, `tools/router.py` | stable | `unit/agents/connections/`, `unit/agents/platform/`, `unit/agents/tools/` | diff --git a/docs/design/agent-workflows/interfaces/cross-service/runner-to-mcp-server.md b/docs/design/agent-workflows/interfaces/cross-service/runner-to-mcp-server.md index fea8089c4f..1dc9d10adc 100644 --- a/docs/design/agent-workflows/interfaces/cross-service/runner-to-mcp-server.md +++ b/docs/design/agent-workflows/interfaces/cross-service/runner-to-mcp-server.md @@ -30,7 +30,7 @@ and `buildSessionMcpServers` repeats the check at session materialization (`assertNoReservedUserMcpName`) as defense in depth. **The internal channel, local transport (HTTP on loopback).** For a non-Pi harness with -executable tool specs on the LOCAL sandbox, `buildToolMcpServers` starts a tiny MCP +tool specs (executable and `client` alike) on the LOCAL sandbox, `buildToolMcpServers` starts a tiny MCP server on `127.0.0.1:` and returns one ACP `type: "http"` entry (`{name: "agenta-tools", url, headers: []}`). The server speaks JSON-RPC 2.0 over Streamable-HTTP (stateless JSON mode) and answers three methods: @@ -57,9 +57,9 @@ closed for user stdio MCP. The run end closes it (releases the port). **The internal channel, Daytona transport (in-sandbox stdio shim).** The loopback URL is a runner-host address; on Daytona the harness runs IN the sandbox, where `127.0.0.1` is the -sandbox's own loopback, not the runner's, so the HTTP transport is unusable. Instead, for a -non-Pi harness with executable tool specs, the engine uploads two files into an ephemeral -in-VM dir (`/home/sandbox/agenta/tool-mcp/`, a sibling of the relay dir, keyed the same +sandbox's own loopback, not the runner's, so the HTTP transport is unusable. Instead, for any +non-Pi harness with tool specs (executable and `client` alike), the engine uploads two files +into an ephemeral in-VM dir (`/home/sandbox/agenta/tool-mcp/`, a sibling of the relay dir, keyed the same way — never inside the relay dir, which the relay loop sweeps, and never on the durable geesefs cwd): @@ -67,9 +67,11 @@ geesefs cwd): `pnpm run build:extension` alongside the Pi extension. `SANDBOX_AGENT_RELAY_MCP_BUNDLE` overrides the bundle path on the runner (trusted deployment configuration, never run or request configuration; tests point it at a fixture). -- `tool-mcp-specs.json` — the run's `AdvertisedToolSpec` array (public fields only). A file, - not an env value, because the env is copied through four exec layers and tool JSON Schemas - are unbounded. +- `tool-mcp-specs.json` — the run's `AdvertisedToolSpec` array (public fields only). `client` + specs ride this file too now, so the model sees browser-fulfilled tools and can call them; a + paused `tools/call` returns a benign non-error wait result via the `{ ok: true, paused: true }` + relay answer (below). A file, not an env value, because the env is copied through four exec + layers and tool JSON Schemas are unbounded. `buildInternalToolMcpEntry` then advertises one ACP stdio entry in `sessionInit.mcpServers`: `{name: "agenta-tools", command: "node", args: [bundlePath], env}` with NO `type` field — the @@ -93,15 +95,20 @@ server-side. The shim runs under the sandbox's own confinement, not on the runne does not reopen the #4831 user-stdio hole (that hole is a runner-host concern, and this entry is synthesized by the runner, never user-declared). -Two refusals remain on the remote path, both loud in `run-plan.ts` (never a silent tool drop): -a remote provider that is not Daytona fails closed with `REMOTE_TOOLS_UNSUPPORTED_MESSAGE` -(the shim's upload + spawn path is proven for Daytona only, and a new provider must not -silently re-open the F1 zero-tools drop), and any `client` (browser-fulfilled) tool on a -non-Pi remote run refuses with `REMOTE_CLIENT_TOOLS_UNSUPPORTED_MESSAGE` (the relay loop parks -a client call and writes no response file, so through the shim it would hang until the relay -timeout; the loopback channel's pause-by-abort has no stdio equivalent yet). Executable -(gateway/callback) tools proceed. A user http MCP server (a remote URL the harness dials -directly) is NOT loopback-bound and stays delivered on Daytona unchanged. +One refusal remains on the remote path, loud in `run-plan.ts` (never a silent tool drop): a +remote provider that is not Daytona fails closed with `REMOTE_TOOLS_UNSUPPORTED_MESSAGE` (the +shim's upload + spawn path is proven for Daytona only, and a new provider must not silently +re-open the F1 zero-tools drop). On Daytona both executable (gateway/callback) AND `client` +(browser-fulfilled) tools now proceed. A client call PARKS through the shim rather than being +refused: the runner-side relay loop (`startToolRelay`) writes a benign paused answer +(`{ ok: true, paused: true }`) into the relay response file, gated by the run-plan +`writePausedAnswer` capability flag (true for a non-Pi harness, false for Pi). The shim maps +that answer to a benign, NON-error `tools/call` result whose text names the tool and tells the +model not to retry, so Claude's turn ends cleanly on the shared client-tool pause seam instead +of waiting out the per-tool relay timeout and emitting a late error frame. The browser result +returns on the cold-replay resume turn, exactly as on the local and Pi paths. A user http MCP +server (a remote URL the harness dials directly) is NOT loopback-bound and stays delivered on +Daytona unchanged. **The file relay.** A resolved tool may need to run privately rather than inside the harness process. The relay moves the call across that boundary: the child publishes a `.req.json` @@ -167,10 +174,10 @@ stdio does not exist in the run contract. Pi refuses external user MCP servers w on Daytona via `buildInternalToolMcpEntry`; the reserved-name check `assertNoReservedUserMcpName`; threads `clientToolRelay` + abort signal; `validateUserMcpUrl` SSRF guard). -- `services/runner/src/engines/sandbox_agent/run-plan.ts`: the remote gates - (`REMOTE_TOOLS_UNSUPPORTED_MESSAGE` for non-Daytona remotes, - `REMOTE_CLIENT_TOOLS_UNSUPPORTED_MESSAGE` for client tools on a non-Pi remote run, the - reserved-name refusal) and the `toolMcpDir` placement invariants. +- `services/runner/src/engines/sandbox_agent/run-plan.ts`: the remote gate + (`REMOTE_TOOLS_UNSUPPORTED_MESSAGE` for non-Daytona remotes, the reserved-name refusal), the + `writePausedAnswer` capability flag (true for non-Pi so a client call parks rather than hangs; + false for Pi), and the `toolMcpDir` placement invariants. - `services/runner/src/engines/sandbox_agent/tool-mcp-assets.ts`: the shim bundle location (`SANDBOX_AGENT_RELAY_MCP_BUNDLE` override) and the per-run upload (`uploadToolMcpAssets`, fail-loud `TOOL_MCP_UNAVAILABLE_MESSAGE`). @@ -185,15 +192,19 @@ stdio does not exist in the run contract. Pi refuses external user MCP servers w - `services/runner/src/tools/tool-mcp-http.ts`: the internal loopback HTTP MCP server (the `client` pause: no JSON-RPC result + abort-the-request). - `services/runner/src/tools/tool-mcp-stdio.ts`: the in-sandbox stdio shim (newline-delimited - JSON-RPC; `tools/call` writes relay request files through the shared relay client). + JSON-RPC; `tools/call` writes relay request files through the shared relay client, and maps a + `paused` relay answer to a benign, non-error wait result so a client call ends the turn cleanly). - `services/runner/src/tools/tool-mcp-env.ts`: the shim's env-name contract, a dependency-free module the server-side entry builder shares without importing the bundle entrypoint. - `services/runner/src/tools/spec-schema.ts`: the shared `specInputSchema` accessor + arg validation. - `services/runner/src/tools/relay.ts`: the runner-side relay loop and hosts - (delete-on-pickup; idle-poll backoff in fallback mode). + (delete-on-pickup; idle-poll backoff in fallback mode; `startToolRelay` writes the + `{ ok: true, paused: true }` paused answer for a parked client call when `writePausedAnswer` + is set). - `services/runner/src/tools/relay-client.ts` and `relay-protocol.ts`: the bundle-safe - in-sandbox writer and wire protocol (atomic publication; the hop-1 response watch). + in-sandbox writer and wire protocol (atomic publication; the hop-1 response watch; the + optional `paused?: true` field on `ExecuteRelayResponse` for a parked client call). - `services/runner/src/tools/relay-watch.ts`: the hop-2 wake sources (local `fs.watch`; the flagged Daytona watch exec). @@ -209,15 +220,25 @@ stdio does not exist in the run contract. Pi refuses external user MCP servers w the MCP client the installed Claude harness uses; re-verify it if that version moves. The Daytona stdio shim answers the same three methods over newline-delimited JSON-RPC; it copies the `specInputSchema` fallback locally so its bundle's import surface stays exactly - relay-client + relay-protocol + types. -- **The client-tool pause is no-result-before-finish.** A paused `tools/call` must never write a - JSON-RPC result (a result lets the harness settle and clobber the pending widget); the handler - aborts its own request and the engine fires an `AbortSignal` on pause/teardown. The stdio shim - has NO pause path, which is exactly why client tools are refused on the remote non-Pi path. -- **The remote-tools gates.** A non-Pi run on a non-Daytona remote provider carrying ANY custom - tool is refused in `run-plan.ts` (fail closed until in-sandbox delivery is proven there); a - non-Pi remote run carrying a `client` tool is refused separately. Executable tools proceed on - Daytona via the shim. Do not widen either gate without a proven delivery path. + relay-client + relay-protocol + types. On the shim a client `tools/call` parks instead of + aborting: it returns the benign non-error wait result mapped from the `{ ok: true, paused: true }` + relay answer. +- **The client-tool pause seam has two transports, one outcome.** Both end the turn on the + shared client-tool pause seam; they differ only in how the paused `tools/call` gets there. On + the loopback HTTP channel a paused call must never write a JSON-RPC result (a result lets the + harness settle and clobber the pending widget); the handler aborts its own request and the + engine fires an `AbortSignal` on pause/teardown. Over stdio there is no socket to abort, so the + runner instead writes a benign `{ ok: true, paused: true }` relay answer (gated by the run-plan + `writePausedAnswer` flag) and the shim returns a benign, non-error wait result — the turn still + ends on the pause seam, and client tools now deliver on Daytona. The pause models a CLOSED set + of outcomes — answered / error / paused-cold (the current Daytona behavior) — with a + `paused-hold` outcome RESERVED for a future warm-path native hold (keeping the shim's call open + inside a live turn, like an ACP approval); no warm behavior is built yet. +- **The remote-tools gate.** A non-Pi run on a non-Daytona remote provider carrying ANY custom + tool is refused in `run-plan.ts` (fail closed until in-sandbox delivery is proven there). On + Daytona both executable AND `client` tools proceed via the shim: executable tools run + server-side, and a client call parks through the `writePausedAnswer` relay answer. Do not widen + the gate to another remote provider without a proven delivery path. - **The reserved server name.** `agenta-tools` must stay in lockstep with the Python adapter's rendered `mcp__agenta-tools__` rules, and a user server may never claim it (checked at declaration time and again at materialization). diff --git a/docs/design/agent-workflows/projects/client-tools-daytona/PLAN.md b/docs/design/agent-workflows/projects/client-tools-daytona/PLAN.md new file mode 100644 index 0000000000..359eb01e33 --- /dev/null +++ b/docs/design/agent-workflows/projects/client-tools-daytona/PLAN.md @@ -0,0 +1,360 @@ +# Plan: client tools on Claude in remote (Daytona) sandboxes + +This plan makes one combination work that does not work today: an agent on the **Claude** +harness, running in a **Daytona** remote sandbox, calling a **client tool** such as +`request_connection`. The tool already works on Pi in any sandbox and on Claude in the local +sandbox. The plan reuses the exact delivery path Pi uses on Daytona, so it adds no new network +route between the runner and the sandbox. It links #5256 (the feature request) and #4984 (the +silent-drop bug whose last remaining case this closes). + +Read the glossary in [README.md](README.md) first. Every domain term below is defined there. + +--- + +## 1. What happens today + +A user builds an agent that uses a client tool and runs it on Claude in a Daytona sandbox. Two +outcomes are possible, both wrong for the user: + +- **The tool set is only client tools.** The run is refused up front with a message that says + client tools are not deliverable on Daytona (the `DAYTONA_CLIENT_ONLY_TOOLS_UNSUPPORTED_MESSAGE` + refusal, `services/runner/src/engines/sandbox_agent/run-plan.ts:84-90`, `:436-441`). This + refusal shipped in PR #5366 as an interim fix so the run fails loudly instead of silently. +- **The tool set mixes client and executable tools.** The run proceeds. The executable tools + are delivered and work. The client tools are silently dropped: the model never sees them in + its tool list, so it can never call them. This is the residual case of #4984 that #5366 did + not close. + +On Pi (any sandbox) and on Claude in the local sandbox, the same client tool works: the model +calls it, the model's turn pauses, the frontend renders the browser widget, the user completes +it, and the agent resumes with the browser result. + +## 2. Why it happens + +Three independent mechanisms block the Claude-plus-Daytona path. Each is verified against the +current code on `origin/main`. + +**Block A: the shim never advertises client tools.** On Daytona the runner uploads a specs +file that the shim serves over `tools/list`. The upload site builds that file from the +executable subset only. `services/runner/src/engines/sandbox_agent/environment.ts:691-700` +gates the upload on `plan.executableToolSpecs.length > 0` and uploads +`advertisedToolSpecs(plan.executableToolSpecs)`. The advertisement gate repeats the filter: +`services/runner/src/engines/sandbox_agent/mcp.ts:333` builds the shim entry only when +`executableToolSpecs(toolSpecs).length > 0`. So the client specs never reach the sandbox, and +Claude never sees a client tool it could call. + +**Block B: the shim's `tools/call` has no "pause" outcome.** When Claude does call a tool, the +shim resolves it to exactly one of two results: a text result, or an error result +(`services/runner/src/tools/tool-mcp-stdio.ts:210-254`). It writes a relay request and blocks +on the runner's answer, bounded only by the per-tool timeout. There is no way for it to say +"pause this turn and wait for the browser." + +**Block C: the relay writes no answer when a client tool parks.** The runner-side relay loop +already knows how to park a client tool: `executeRelayedTool` sees `spec.kind === "client"`, +asks the shared client-tool seam, and on a pending decision returns the `PAUSED` sentinel +(`services/runner/src/tools/relay.ts:326-345`). But when the loop gets `PAUSED` it simply +returns without writing an answer file (`services/runner/src/tools/relay.ts:564`, +`if (text === PAUSED) return;`). For Pi that is fine, because Pi pauses through its own +extension. For the Claude shim it is fatal: the shim is still blocking on an answer file that +will never appear, so it waits out the full per-tool timeout and then returns an error to +Claude. That both surfaces a tool error to the model and risks a late timeout frame arriving +after the runner has already torn the turn down. + +**What already works and needs no new plumbing.** The runner-side pause seam is already wired +into the Daytona path. `run-turn.ts:398-412` arms the relay loop with the sandbox filesystem +host (`sandboxRelayHost`) on Daytona and passes the shared client-tool seam +(`env.clientToolRelayRef.current`, built by `buildClientToolRelay` at `run-turn.ts:373`) into +it, for every harness including Claude. So once the shim advertises a client tool (Block A) and +the relay answers the park (Block C), the runner already parks the turn, emits the browser +widget, and ends the turn exactly as it does for Pi on Daytona. This is the key reason the +change is small. + +## 3. The approach Mahmoud approved + +Reuse the Pi-on-Daytona pattern over the file relay that already exists. Add no second network +path between the runner and the sandbox. Concretely: let the shim advertise client tools, teach +the relay and the shim one new "paused" answer so the shim can end its `tools/call` cleanly +while the runner ends the turn, and let the existing cold-replay resume return the browser +result on the next turn. Then delete the interim #5366 refusal, which the working path +replaces. + +The rest of this document elaborates that direction into five steps. It does not reopen it. + +## 4. The five steps + +Each step lists the files it changes, the contract it alters, and why. A "contract" here means +a shape or a rule that another part of the system depends on: a wire field, a function's +return type, or a documented behavior. + +### Step 1: advertise client tools to the shim + +Stop filtering client specs out of the uploaded specs file and out of the advertisement gate. + +- `services/runner/src/engines/sandbox_agent/environment.ts:691-700`: change the upload gate + from `plan.executableToolSpecs.length > 0` to `plan.toolSpecs.length > 0`, and upload + `advertisedToolSpecs(plan.toolSpecs)` instead of `advertisedToolSpecs(plan.executableToolSpecs)`. + This is the true source of Block A. Note for the implementer: the design brief attributed the + stripping to `run-plan.ts` and `mcp.ts`; the file that actually assembles the uploaded specs + is `environment.ts`, and it must be part of this step (see the research note in §8). +- `services/runner/src/engines/sandbox_agent/mcp.ts:333`: change the advertisement gate from + `internalToolMcp && executableToolSpecs(toolSpecs).length > 0` to + `internalToolMcp && toolSpecs.length > 0`, so a run whose tools are all client tools still + advertises the shim. +- `services/runner/src/engines/sandbox_agent/mcp.ts:341-348`: correct the honest log so it + counts every advertised tool, not only the executable ones. The current wording ("N gateway + tool(s) advertised") would undercount once client tools ride the same channel. + +The shim itself (`tool-mcp-stdio.ts`) needs no change to advertise: its `tools/list` already +maps whatever specs it is given (`tool-mcp-stdio.ts:193-208`). Once the client specs are in the +uploaded file, Claude sees them. + +**Contract altered**: the uploaded specs file (`AdvertisedToolSpec[]`) now includes client-kind +tools on the Daytona shim path. The advertised shape stays exactly the same three public fields +(name, description, input schema); only the population set widens. No credential or private +field is added (the shim still reads only public metadata). + +### Step 2: confirm the runner-side pause covers the Daytona relay + +The pause seam is already wired for Daytona (see §2, "What already works"). This step is +primarily verification, plus one correlation detail to check live. + +- No new wiring is expected. `run-turn.ts:398-412` already passes the client-tool seam into the + Daytona relay loop, and `executeRelayedTool` (`relay.ts:326-345`) already parks a client tool + and returns `PAUSED`. The `clientToolRelay` value passed to `buildSessionMcpServers` + (`environment.ts:887`) is consumed only on the local branch (`mcp.ts:327-332`) and ignored on + Daytona; that is correct, because the Daytona pause happens in the relay loop, not in the MCP + server object. Leave it. +- The one detail to verify live: correlation of the parked widget to Claude's real tool-call + id. The shim mints its own request id (`randomUUID()`, `tool-mcp-stdio.ts:229`) and does not + know Claude's ACP tool-call id, so the seam correlates by tool name plus arguments + (`toolCallIndex.lookup(toolName, input)`, `client-tools.ts:242-244`), exactly as the local + Claude MCP path does. Confirm in the live Daytona test (§5) that the widget attaches to the + right Claude tool bubble. Only if the live test shows a correlation miss does this step add + code; the plan does not assume it will. + +**Contract altered**: none expected. If the live test forces a correlation fix, it would refine +how the seam resolves the tool-call id, not add a wire field. + +### Step 3: teach the relay and the shim a "paused" answer + +This is the one genuinely new piece of code. It removes Block B and Block C together. + +- **The relay answer shape** (`services/runner/src/tools/relay-protocol.ts:29-35`): add an + optional `paused?: true` field to `ExecuteRelayResponse`. This is a new, backward-compatible + variant of the answer the runner writes into `.res.json`. An answer is now one of: a + success (`ok: true, text`), a failure (`ok: false, error`), or a pause + (`ok: true, paused: true`). +- **The relay loop** (`services/runner/src/tools/relay.ts:564`): instead of returning without + writing an answer on `PAUSED`, write a paused answer file for the Claude shim path: + `writeResponse(id, { ok: true, paused: true })`. This must be conditional on the non-Pi + Daytona shim path only. Pi also parks client tools through this same loop and must keep its + current behavior (no answer file; Pi pauses through its extension). Thread a flag into + `startToolRelay` (for example `writePausedAnswer: !plan.isPi`, set from the run plan at the + `run-turn.ts:398-412` call site) and gate the paused write on it. Do not write a paused answer + on the Pi path. +- **The relay client** (`services/runner/src/tools/relay-client.ts:260-269`): `relayToolCall` + currently returns `res.text` on success and throws on failure. Teach it to recognize + `res.paused` and return a distinct paused result to its caller (a small sentinel, not a + thrown error), so the shim can tell a pause apart from a normal result. +- **The shim's `tools/call`** (`services/runner/src/tools/tool-mcp-stdio.ts:226-236`): when the + relay client reports a pause, return a benign, non-error tool result to Claude, for example + `{ content: [{ type: "text", text: "Waiting for the user to complete this action." }] }`, and + crucially not `isError: true`. Claude receives a clean result, its turn ends, and meanwhile + the runner has already ended the turn (the seam called `pause.pause()` at + `client-tools.ts:266`). The real browser result arrives on the resume turn (Step 4). + +**The ordering risk and its mitigation.** The known risk is that the pause and the turn +teardown race under Daytona filesystem latency. Today the failure mode is exactly that race: +the shim waits out a long timeout and then emits a late error frame after teardown. The paused +answer removes the race by construction. The runner writes the paused answer promptly when the +tool parks, not after a timeout, so the shim resolves its `tools/call` immediately with a benign +result and never emits a late timeout frame. This is the same "explicit early signal instead of +silent no-write" fix the local path already relies on. + +**Relay cleanup.** Issue #5256 asks that a paused call cannot emit a late timeout answer. The +paused answer file plus the shim's immediate benign return achieve that. In addition, ensure the +paused `.req.json` and `.res.json` pair does not linger to be re-read on the resume +turn's replay. The relay already sweeps stale files at turn start (`relay.ts` ready and sweep, +around `:454`); confirm the paused pair is swept or ignored so a resume cannot pick up stale +paused bytes for a fresh call. + +**Contract altered**: `ExecuteRelayResponse` gains `paused?: true` (a new answer variant on the +relay files). `relayToolCall`'s return type gains a paused case. The shim's `tools/call` gains a +third outcome (benign wait result) alongside success and error. All three are additive and +carry no secret. + +### Step 4: verify cold-replay resume on Daytona + +No code is expected here. The resume path already exists; Block A meant it was never reached for +a client tool. Once Steps 1 through 3 land, verify it live. + +The client-tool resume runs through cold replay: the runner reads the browser output back from +replayed history (`extractClientToolOutputs`, `responder.ts:391-409`) and the transcript builder +emits a client resume frame (`transcript.ts:190-246`). On the resume turn the shim runs again, +Claude re-calls the client tool, and this time the seam finds the stored browser output and +returns it as a fulfilled result (`client-tools.ts:238`), so `executeRelayedTool` returns the +real output string (`relay.ts:349`) and the shim returns a normal (non-paused) tool result. +The paused path from Step 3 only fires on the first, still-pending call. + +Verify the three continuation cases #5256 names: a normal pause and resume, a resume after the +control link reconnects, and a resume after the sandbox restarts. Also verify repeated and +multiple client-tool calls in one run do not consume the wrong stored result (the seam consumes +one stored output per tool-call key in first-in-first-out order, `responder.ts:256-263`). + +**Contract altered**: none. This step confirms an existing contract now reaches a case it could +not before. + +### Step 5: remove the #5366 client-only refusal and replace its tests + +The interim refusal is now wrong: the path it refused works. + +- `services/runner/src/engines/sandbox_agent/run-plan.ts`: delete + `DAYTONA_CLIENT_ONLY_TOOLS_UNSUPPORTED_MESSAGE` (`:84-90`) and its use (`:436-441`). Keep the + separate `REMOTE_TOOLS_UNSUPPORTED_MESSAGE` refusal for a non-Daytona remote provider + (`:433-434`): that path is still unproven and must still fail closed. +- Tests: `tests/unit/sandbox-agent-run-plan.test.ts`, `tests/unit/session-mcp-layering.test.ts`, + and `tests/unit/tool-bridge.test.ts` reference the client-only refusal or the mixed-set drop. + Remove the assertions that the refusal fires and that a mixed set drops its client tools. + Replace them with assertions of the working path: a client-only run on Daytona builds the shim + and advertises the client tool; a mixed run advertises both kinds; a client-tool call parks + and the relay writes a paused answer. + +**Contract altered**: the run plan no longer refuses a client-only Daytona run. The refusal +message constant is deleted. A caller or test that asserted the refusal must change. + +## 5. Test plan + +Three layers. Unit tests pin the new logic. A live Daytona script proves the end-to-end path. +A new release-gate journey guards it against regression before every agent-workflows release. + +### Unit tests (`services/runner`, vitest) + +- **Advertisement (Step 1)**: given a client-only tool set on Daytona, `buildSessionMcpServers` + builds the shim entry and the uploaded specs file contains the client tool; given a mixed set, + the file contains both kinds. Cover the corrected log wording. +- **Paused relay answer (Step 3)**: when `executeRelayedTool` returns `PAUSED` on the non-Pi + Daytona path, the loop writes `{ ok: true, paused: true }`; on the Pi path it writes no answer + file (unchanged). `relayToolCall` maps a paused answer to its paused result, not a throw. The + shim's `tools/call` maps a paused result to a benign, non-error content result. +- **Refusal removal (Step 5)**: the run plan no longer returns the client-only message on + Daytona; it still returns `REMOTE_TOOLS_UNSUPPORTED_MESSAGE` for a non-Daytona remote. +- **Wire contract**: `ExecuteRelayResponse` gains `paused?: true`. The relay files are internal + to the runner (not part of the `/run` wire contract mirrored in `wire.py`), so no golden + fixture in `sdks/python` changes. Confirm this while editing `relay-protocol.ts`. + +### Live Daytona QA script (the manual proof) + +Extend the QA matrix runner +(`docs/design/agent-workflows/projects/qa/scripts/run_matrix.py`) with a client-tool cell on +the Claude-plus-Daytona combination. Outline: + +1. Mint an ephemeral account and key (the existing accounts fixture). +2. Invoke on the Claude harness, Daytona sandbox, with a single client tool declared (a + `request_connection`-shaped spec) and a prompt that provokes the call. +3. Assert the streamed frames show a `client_tool` interaction request (the browser widget), + not a tool error and not a silent zero-tool run. +4. Post the browser output back as the interaction answer. +5. Invoke the resume on the same session; assert the run continues and the tool result carries + the browser output. +6. Repeat for the reconnect and sandbox-restart cases from #5256. + +Run it against the live dev stack, from the three-variable environment the QA scripts already +use. Record the result as a QA finding under `docs/design/agent-workflows/projects/qa/`. + +### New release-gate journey (the regression guard) + +Add a `client` journey to the portable release gate +(`.agents/skills/agent-release-gate/resources/qa_product.py`, the `JOURNEYS` map at `:911-920`) +and bind it to the Claude-plus-Daytona cell **C2** (`:134-143`), with the Claude-plus-local cell +**C1** (`:125-133`) as a parity control. The journey drives the product endpoint the playground +drives and asserts on the frame stream and the real side effect, never on the model's prose, so +it runs against any deployment. Assertions, modeled on the existing `j4_approve` flow: + +- Turn one: the wire emits a `client_tool` interaction for the declared client tool. The run + does not error and does not return zero tools. +- The gate answers the interaction with a browser output. +- Turn two (resume): the wire shows the tool result carrying that browser output, and the run + completes. + +Passing on C2 is the release gate for this feature. Passing on C1 proves the change did not +regress the local Claude path. + +## 6. Rollout and rollback + +The whole change ships behind the Daytona path for the Claude harness and touches no other +combination. Each step is revertable on its own: + +- **Step 1** is revertable: restore the `executableToolSpecs` filter at the upload and + advertisement gates. Client tools stop being advertised again. +- **Step 2** adds no code (or a small correlation refinement), so there is nothing to roll back + beyond that refinement. +- **Step 3** is the load-bearing change and is revertable: remove the `paused` field, restore + `if (text === PAUSED) return;`, and revert the shim's benign-result branch. Behavior returns + to Block B and Block C. +- **Step 4** adds no code. +- **Step 5** is revertable: restore the `DAYTONA_CLIENT_ONLY_TOOLS_UNSUPPORTED_MESSAGE` refusal. + +If the live Daytona test in Step 4 fails and cannot be made green quickly, the safe partial +state is Steps 1 through 3 landed but Step 5 held back, so the interim refusal still guards the +client-only case while the mixed-set drop is fixed. The cleanest full rollback is to revert all +five steps together, which returns to today's #5366 behavior exactly. + +There is no data migration, no schema change outside the runner, and no change to the `/run` +wire contract, so a rollback is a code revert with no cleanup. + +## 7. Non-goals + +- **No redesign of the relay's turn boundary.** The relay loop is a fire-and-forget poll with no + general turn-boundary model. Making a relay-executed executable tool with an `ask` permission + park from inside the loop is a separate open issue (the "S5.2" gap, + `../research/.../open-issues.md:132-169`). This plan uses the client-tool park path that + already exists in the loop; it does not give the loop a general turn-boundary model. +- **No new network path.** The change rides the existing file relay and the existing ACP link. + It does not add a runner-to-sandbox socket, a callback URL, or a second control channel. +- **No gateway or code-tool parking.** Executable (gateway and callback) tools keep running + server-side and returning inline. This plan does not change how they execute or add a pause + for them. +- **No concurrent-approval change.** Multiple client-tool widgets already park together in one + turn; this plan preserves that and does not touch the one-approval-per-turn latch (see §8). +- **No change to Pi.** Pi's client-tool path on any sandbox is untouched; the paused answer is + gated to the non-Pi Daytona shim path. + +## 8. How this composes with two in-flight efforts + +All three efforts touch pause and resume. They must not collide. + +- **The concurrent human-in-the-loop work** (`../hitl-fix/`, and Question 2 of the research) + concerns approval gates, which pause through a different plane: the ACP permission reverse + call, gated by a one-approval-per-turn latch (`permission-plan.ts:173-185`). Client tools do + not use that latch; each client tool parks its own widget through the un-latched seam + (`client-tools.ts:178-181`, `:245-264`). This plan changes only how a client-tool park is + delivered on Daytona, not how many can park or how approvals serialize. It therefore does not + touch the latch, the parked-approval record, or the keep-alive multi-gate refusal. The two + efforts are independent and can land in either order. +- **The in-flight deny-frame PR** changes how a denied interaction projects to the wire. This + plan's paused path emits a benign wait result, not a deny, and the client-tool deny already + has its own return (`relay.ts:346-347`, "was denied"). The two do not overlap in code. The one + place to check on integration: that a client tool denied on Daytona (user cancels the widget) + still projects the deny frame the same way the local path does, once both changes are in. Flag + this as a merge-time check, not a design conflict. + +## 9. Open questions for review + +1. **The paused answer's gating flag.** The plan gates the paused-answer write on `!plan.isPi`. + Is there any Daytona non-Pi harness other than Claude on the horizon where "not Pi" would be + too broad? If a third harness appears, the gate may need to key on "uses the shim" rather than + "not Pi." Naming the flag `writePausedAnswer` (set from the run plan) keeps that future change + local. +2. **Correlation on Daytona (Step 2).** The plan assumes name-plus-arguments correlation works on + Daytona exactly as it does on local Claude. If two client-tool calls in one turn carry + identical arguments, the lookup could attach a widget to the wrong bubble. The seam already + consumes stored outputs first-in-first-out per key; confirm the live test exercises the + identical-args case, and decide whether the shim should pass a correlating hint if it does not. +3. **The benign wait text.** The shim returns a short "waiting for the user" text result to + Claude on a pause. Should that text be fixed, or should it echo the tool name and render hint + so a model that reasons over it stays coherent? This is a small wording decision with a + possible effect on model behavior; worth a view before implementation. +4. **Relay cleanup timing.** The plan relies on the existing stale-file sweep to clear the paused + req/res pair before the resume turn. Confirm the sweep runs early enough on the resume turn + that a paused pair from the previous turn cannot satisfy a fresh call's wait with stale bytes, + or add an explicit delete when the paused answer is written. diff --git a/docs/design/agent-workflows/projects/client-tools-daytona/README.md b/docs/design/agent-workflows/projects/client-tools-daytona/README.md new file mode 100644 index 0000000000..19cff8ac35 --- /dev/null +++ b/docs/design/agent-workflows/projects/client-tools-daytona/README.md @@ -0,0 +1,69 @@ +# Client tools on Claude in remote (Daytona) sandboxes + +An agent that runs on the Claude harness inside a remote Daytona sandbox cannot call a +browser-fulfilled tool such as `request_connection`. The same tool already works on Pi (any +sandbox) and on Claude in the local sandbox. This workspace plans the change that closes the +gap. It reuses the delivery path Pi already uses on Daytona, so it adds no new network route +between the runner and the sandbox. + +This is a design and planning workspace. It changes no code. It links the feature request +(#5256) and the silent-drop bug whose remaining case it closes (#4984). + +## Read in this order + +1. [PLAN.md](PLAN.md): the whole plan. What happens today, why, the five implementation + steps with their file-level changes and altered contracts, the test plan, rollout and + rollback, non-goals, how it composes with two other in-flight efforts, and open questions. + +The plan is a single file on purpose: the change is small and the steps are tightly coupled, +so one linear read serves the implementer better than a split workspace. + +## Glossary + +These terms recur throughout the plan. Each is defined once here so later text can lean on +the shared meaning. + +- **Runner**: the Node service (`services/runner/`) that drives one agent turn. It receives a + JSON `/run` request and returns a structured result. It decides how to run the agent; the + Python agent service decides what to run. +- **Harness**: the coding-agent program the runner drives. Two exist: **Pi** (bundled, MIT) + and **Claude** (Claude Code over a protocol link). They differ in how they discover and + call tools. +- **Sandbox**: the isolated compute where the harness runs. **Local** means the same host as + the runner. **Daytona** means a separate remote virtual machine. +- **MCP** (Model Context Protocol): the standard way a harness lists and calls tools. +- **ACP** (Agent Client Protocol): the control link between the runner and the harness + process running inside the sandbox. It carries the prompt, streamed events, and permission + requests. +- **Executable tool**: a gateway or callback tool the runner runs itself, server-side, behind + its own permission check. Its credentials never enter the sandbox. +- **Client tool**: a tool fulfilled by the user's browser, not by the runner. `request_connection` + is the canonical example. The model calls it, the browser does the work, the browser's result + becomes the tool result. +- **In-sandbox stdio MCP shim** ("the shim"): a small Node program the runner uploads into a + Daytona sandbox. The Claude harness launches it and speaks MCP to it over standard input and + output. The shim forwards each tool call to the runner and returns the runner's answer. +- **File relay** ("the relay"): how the shim and the runner exchange a tool call across the + sandbox boundary. The shim writes a request file `.req.json` into a shared directory; the + runner writes the answer file `.res.json`; the shim reads it back. The runner also runs a + loop that watches that directory and executes each request. +- **Park and resume**: pause the model's current turn to wait for a human or the browser, then + continue the same agent session later with the awaited result. +- **Cold replay**: the resume mechanism that re-sends the whole stored conversation to reach + the paused point again, rather than holding the live session in memory. Client-tool resume + uses cold replay today. + +## Related work + +- [../daytona-gate-delivery/](../daytona-gate-delivery/): the Claude permission-gate parity + fix on Daytona; the ACP permission plane this plan does not touch. +- [../mcp-client-tool-continuation/](../mcp-client-tool-continuation/): earlier experiments on + client-tool continuation. +- [../remote-tools-delivery/](../remote-tools-delivery/) and + [../gateway-tool-mcp/](../gateway-tool-mcp/): executable-tool delivery to Claude on Daytona + (#5244), the path this plan extends to client tools. +- [../hitl-fix/](../hitl-fix/): the concurrent human-in-the-loop work this plan must not + regress (see PLAN.md, "How this composes"). +- The research this plan builds on: + [../../scratch/research-client-tools-and-concurrent-hitl.md](../../scratch/research-client-tools-and-concurrent-hitl.md), + Question 1. diff --git a/services/runner/src/engines/sandbox_agent/client-tools.ts b/services/runner/src/engines/sandbox_agent/client-tools.ts index 46964fcff2..ee244aef59 100644 --- a/services/runner/src/engines/sandbox_agent/client-tools.ts +++ b/services/runner/src/engines/sandbox_agent/client-tools.ts @@ -24,6 +24,45 @@ import type { type EmitRun = { emitEvent: (event: AgentEvent) => void }; +/** + * Lifecycle policy for a parked browser-fulfilled client tool: what happens to both the turn and + * the in-sandbox shim's blocking `tools/call` when it parks. Closed set: + * - "pi-native": Pi parks through its own extension; the relay writes no answer file. + * - "cold-acknowledge": the non-Pi shim (Claude on Daytona) blocks on a relay answer file, so the + * relay writes a benign paused answer to end the `tools/call`. The only + * disposition that writes an answer. + * - "warm-hold": RESERVED, not built — keep the `tools/call` open inside the live turn (the + * way an ACP approval holds on a warm session). See #5384. + */ +export type ClientToolPauseDisposition = + | "pi-native" + | "cold-acknowledge" + | "warm-hold"; + +/** + * Whether the relay loop writes the benign paused answer for this disposition — the single derived + * switch the relay consumes. Exhaustive on purpose: a new disposition (the warm hold) forces a + * decision here rather than silently falling through to the cold behavior. + */ +export function relayWritesPausedAnswer( + disposition: ClientToolPauseDisposition, +): boolean { + switch (disposition) { + case "cold-acknowledge": + return true; + case "pi-native": + // Pi parks through its extension; the shim answer file is a non-Pi concept. + return false; + case "warm-hold": + // The warm hold keeps the call open inside the live turn, so it writes no cold answer. + return false; + default: { + const unreachable: never = disposition; + return unreachable; + } + } +} + /** * Correlates an MCP `tools/call` (which carries only name + arguments) to the real ACP * tool-call id Claude surfaced on the event stream, so the paused `client_tool` interaction diff --git a/services/runner/src/engines/sandbox_agent/environment-setup.ts b/services/runner/src/engines/sandbox_agent/environment-setup.ts index d8ca015109..e27d25fbb7 100644 --- a/services/runner/src/engines/sandbox_agent/environment-setup.ts +++ b/services/runner/src/engines/sandbox_agent/environment-setup.ts @@ -214,12 +214,14 @@ export async function prepareEnvironmentSetup( `piPublicTools=${piExtEnv.AGENTA_AGENT_TOOLS_PUBLIC_SPECS ? "yes" : "no"}`, ); if (!plan.isPi && plan.isDaytona) { - const omittedClientTools = plan.toolSpecs + const clientTools = plan.toolSpecs .filter((spec) => spec.kind === "client") .map((spec) => spec.name); - if (omittedClientTools.length > 0) { + if (clientTools.length > 0) { + // Client tools ride the Daytona stdio shim alongside executable tools: the model sees them, + // calls them, and the call parks (the relay writes a benign paused answer). See mcp.ts. logger( - `omitting client tools from Daytona stdio MCP shim: ${omittedClientTools.join(", ")}`, + `advertising client tools on the Daytona stdio MCP shim: ${clientTools.join(", ")}`, ); } } diff --git a/services/runner/src/engines/sandbox_agent/environment.ts b/services/runner/src/engines/sandbox_agent/environment.ts index f66a87d2ba..bf31bf8d1d 100644 --- a/services/runner/src/engines/sandbox_agent/environment.ts +++ b/services/runner/src/engines/sandbox_agent/environment.ts @@ -709,13 +709,15 @@ export async function acquireEnvironment( if (plan.isPi && plan.builtinGatingActive && !daytonaExtensionInstalled) { throw new Error(PI_PERMISSION_EXTENSION_UNAVAILABLE_MESSAGE); } - if (!plan.isPi && plan.executableToolSpecs.length > 0) { + if (!plan.isPi && plan.toolSpecs.length > 0) { + // Advertise the FULL tool set to the shim, client tools included: a parked client tool + // resolves through the relay's paused answer (see startToolRelay / tool-mcp-stdio.ts). internalToolMcp = await ( deps.uploadToolMcpAssets ?? uploadToolMcpAssets )( environment.sandbox, plan.toolMcpDir, - advertisedToolSpecs(plan.executableToolSpecs), + advertisedToolSpecs(plan.toolSpecs), logger, ); } diff --git a/services/runner/src/engines/sandbox_agent/mcp.ts b/services/runner/src/engines/sandbox_agent/mcp.ts index 1c2b716254..dc4af26917 100644 --- a/services/runner/src/engines/sandbox_agent/mcp.ts +++ b/services/runner/src/engines/sandbox_agent/mcp.ts @@ -7,7 +7,6 @@ import { buildToolMcpServers, } from "../../tools/mcp-bridge.ts"; import type { ClientToolRelay } from "../../tools/client-tool-relay.ts"; -import { executableToolSpecs } from "../../tools/public-spec.ts"; // The shim's env contract, from the dependency-free names module — never from the shim's // bundle entrypoint (`tool-mcp-stdio.ts`), which server code must not import. import { @@ -267,17 +266,20 @@ export interface SessionMcpServers { * * TWO INDEPENDENT LAYERS — do not merge their gates (the #4831 regression this fixed conflated * them into one switch; project gateway-tool-mcp): - * 1. INTERNAL gateway-tool channel: the runner-synthesized advertisement of the run's resolved - * gateway/callback tools. Carries only public metadata; execution relays server-side. Two + * 1. INTERNAL tool channel: the runner-synthesized advertisement of the run's resolved tools — + * gateway/callback tools AND browser-fulfilled `client` tools (the model must SEE a client + * tool to call it; the call then parks). Carries only public metadata; execution relays + * server-side (a `client` call parks rather than executing). Two * transports by where the harness runs: LOCAL keeps the loopback HTTP MCP server * (`buildToolMcpServers`); on DAYTONA the harness runs IN the sandbox (where `127.0.0.1` * is the sandbox's loopback, not the runner's), so the channel is the uploaded in-sandbox * stdio MCP shim instead (`buildInternalToolMcpEntry`, fed by `uploadToolMcpAssets` — its - * `tools/call` writes relay request files the runner-side loop executes). Daytona WITHOUT - * uploaded shim assets means no delivery path; `run-plan.ts` refuses undeliverable - * combinations (non-Daytona remotes, client tools on Daytona) before a session is ever - * built, and the log below states honestly whether an advertisement happened — it must - * never claim a delivery that isn't happening (the F1 false log). + * `tools/call` writes relay request files the runner-side loop executes; a `client` tool + * parks and the loop writes a benign paused answer). Daytona WITHOUT uploaded shim assets + * means no delivery path; `run-plan.ts` refuses the one still-undeliverable combination (a + * non-Daytona remote provider) before a session is ever built, and the log below states + * honestly whether an advertisement happened — it must never claim a delivery that isn't + * happening (the F1 false log). * 2. USER MCP capability (`toAcpMcpServers`): the user's own declared servers — stdio DISABLED, * http delivered (#4834). UNCHANGED on every sandbox: a user http MCP is a REMOTE url the * harness dials directly (not a runner loopback), so it stays reachable from a Daytona sandbox. @@ -320,9 +322,10 @@ export async function buildSessionMcpServers({ // as the internal TYPELESS stdio entry instead — the in-sandbox harness launches the // uploaded `tool-mcp-stdio.js`, which writes relay request files the runner-side relay // loop executes (the loop already polls the sandbox FS; see `engines/sandbox_agent.ts`). - // Only executable specs ride this channel (client tools have no pause path through the - // shim; `run-plan.ts` refuses them on this path). Without uploaded assets there is no - // delivery path and the honest no-channel log below fires. + // BOTH executable and client specs ride this channel: an executable call relays inline, + // a client call parks and the relay writes a benign paused answer so the shim ends its + // `tools/call` cleanly while the runner ends the turn (see `startToolRelay`). Without + // uploaded assets there is no delivery path and the honest no-channel log below fires. let internal: SessionMcpServers; if (!isDaytona) { internal = await buildToolMcpServers(toolSpecs, relayDir, { @@ -330,7 +333,7 @@ export async function buildSessionMcpServers({ signal, log, }); - } else if (internalToolMcp && executableToolSpecs(toolSpecs).length > 0) { + } else if (internalToolMcp && toolSpecs.length > 0) { internal = { servers: [buildInternalToolMcpEntry(internalToolMcp, relayDir)], close: async () => {}, @@ -339,9 +342,11 @@ export async function buildSessionMcpServers({ internal = { servers: [], close: async () => {} }; } if (isDaytona && toolSpecs.length > 0) { + // Count every advertised tool, executable and client alike: both kinds now ride the shim + // (executable tools relay inline; a client tool parks and resolves via the paused answer). log( internal.servers.length > 0 - ? `daytona: ${executableToolSpecs(toolSpecs).length} gateway tool(s) advertised via ` + + ? `daytona: ${toolSpecs.length} tool(s) advertised via ` + `the in-sandbox stdio MCP shim (the loopback MCP URL is unreachable from the sandbox)` : `daytona: ${toolSpecs.length} tool(s) NOT advertised (no in-sandbox tool MCP assets ` + `for this harness — run-plan should have refused this run)`, diff --git a/services/runner/src/engines/sandbox_agent/run-plan.ts b/services/runner/src/engines/sandbox_agent/run-plan.ts index f619376787..75cacac464 100644 --- a/services/runner/src/engines/sandbox_agent/run-plan.ts +++ b/services/runner/src/engines/sandbox_agent/run-plan.ts @@ -27,6 +27,7 @@ import { resolveSkillDirs as defaultResolveSkillDirs, } from "../skills.ts"; import { assert } from "./capabilities.ts"; +import type { ClientToolPauseDisposition } from "./client-tools.ts"; import { buildTurnText } from "./transcript.ts"; import { KNOWN_SANDBOX_PROVIDER_IDS, @@ -70,25 +71,6 @@ export const REMOTE_TOOLS_UNSUPPORTED_MESSAGE = "sandbox, use the Pi harness, or remove the tools. Tracked in " + "docs/design/agent-workflows/projects/in-sandbox-tool-mcp/."; -/** - * A non-Pi harness (Claude) on Daytona receives tools through the in-sandbox stdio MCP shim, - * which advertises only EXECUTABLE (gateway/callback) tools. Client tools are intentionally - * omitted: the shim's blocking relay call cannot pause for a browser round-trip yet. So a run - * whose ENTIRE tool set is client-kind has nothing deliverable to advertise on Daytona — it - * would otherwise proceed, drop every tool silently, and return `ok:true` (the F1 zero-tools - * drop). `run-plan.ts` is the documented refusal point for this (see `mcp.ts`); refuse it up - * front, exactly as the non-Daytona remote case is refused. A MIX of client + executable tools - * is fine (the executable ones are delivered; the client ones are dropped), so this fires only - * when NO executable tool remains. - */ -export const DAYTONA_CLIENT_ONLY_TOOLS_UNSUPPORTED_MESSAGE = - "Client tools are not deliverable to a non-Pi harness on Daytona: the in-sandbox stdio MCP " + - "shim advertises only executable (gateway/callback) tools, and a client tool cannot pause " + - "for a browser round-trip through its blocking relay yet. This run's tool set is entirely " + - "client-kind, so nothing would be advertised and the run would silently get zero tools. Add " + - "an executable tool, use the Pi harness, run on the local sandbox, or remove the tools. " + - "Tracked in docs/design/agent-workflows/projects/in-sandbox-tool-mcp/."; - /** * `runtime_provided` (subscription) auth means the harness authenticates from explicitly prepared * local runtime state (a mounted Pi/Claude login). That state lives only in the runner container @@ -153,6 +135,13 @@ export interface RunPlan { /** True when Pi builtin grants or permissions need extension enforcement. */ builtinGatingActive: boolean; useToolRelay: boolean; + /** + * How a parked client tool disposes of the turn and the in-sandbox shim's blocking call (closed + * set: `ClientToolPauseDisposition`). Kept on the plan so a future harness, or the reserved warm + * hold, is a local change rather than an `!isPi` test scattered across call sites. Today: Pi → + * "pi-native", the non-Pi shim (Claude on Daytona) → "cold-acknowledge". + */ + clientToolPauseDisposition: ClientToolPauseDisposition; systemPrompt?: string; appendSystemPrompt?: string; hasSystemPrompt: boolean; @@ -422,23 +411,11 @@ export function buildRunPlan( return { ok: false, error: RESERVED_MCP_SERVER_NAME_MESSAGE }; } - // Non-Pi + remote + tools: executable (gateway/callback) tools are DELIVERABLE on Daytona - // via the in-sandbox stdio MCP shim (uploaded per run, advertised as the internal typeless - // stdio entry, calls relayed to the runner through the file relay). A remote provider that is - // not Daytona fails CLOSED because the shim's upload + spawn path is proven for Daytona only. - // Client tools are intentionally omitted from the Daytona shim's uploaded public specs: its - // blocking relay call cannot pause for a browser round-trip yet. Local Claude and Pi retain - // their existing client-tool delivery paths. - if (!isPi && isRemoteSandbox && toolSpecs.length > 0) { - if (!isDaytona) { - return { ok: false, error: REMOTE_TOOLS_UNSUPPORTED_MESSAGE }; - } - // On Daytona the shim advertises only executable tools; client tools are omitted. If the run - // carries tools but NONE are executable, nothing would be delivered — refuse instead of - // silently advertising an empty tool set (the F1 zero-tools drop mcp.ts's log warns about). - if (executableToolSpecsForRun.length === 0) { - return { ok: false, error: DAYTONA_CLIENT_ONLY_TOOLS_UNSUPPORTED_MESSAGE }; - } + // Non-Pi + remote + tools: executable and client tools are both deliverable on Daytona via the + // in-sandbox stdio MCP shim. A non-Daytona remote provider fails CLOSED because the shim's + // upload + spawn path is proven for Daytona only. + if (!isPi && isRemoteSandbox && !isDaytona && toolSpecs.length > 0) { + return { ok: false, error: REMOTE_TOOLS_UNSUPPORTED_MESSAGE }; } // Layer 2: even on Daytona, code/gateway tools run on the RUNNER HOST via the relay, not @@ -548,6 +525,9 @@ export function buildRunPlan( // The relay carries tool EXECUTION only (permission gates ride the extension's // `ctx.ui.confirm` dialog onto the ACP plane), so a builtin-gating-only run needs no relay. useToolRelay: toolSpecs.length > 0, + // Pi parks through its own extension (no answer file); the non-Pi shim blocks on an answer + // file, so a parked client tool is acknowledged with a benign paused answer. + clientToolPauseDisposition: isPi ? "pi-native" : "cold-acknowledge", systemPrompt, appendSystemPrompt, hasSystemPrompt: !!(systemPrompt || appendSystemPrompt), diff --git a/services/runner/src/engines/sandbox_agent/run-turn.ts b/services/runner/src/engines/sandbox_agent/run-turn.ts index e044c8d31d..a05dfd8761 100644 --- a/services/runner/src/engines/sandbox_agent/run-turn.ts +++ b/services/runner/src/engines/sandbox_agent/run-turn.ts @@ -34,7 +34,10 @@ import { TOOL_NOT_EXECUTED_PAUSED, } from "../../tracing/otel.ts"; import { attachPermissionResponder } from "./acp-interactions.ts"; -import { buildClientToolRelay } from "./client-tools.ts"; +import { + buildClientToolRelay, + relayWritesPausedAnswer, +} from "./client-tools.ts"; import { invalidateContinuity } from "./environment.ts"; import { conciseError } from "./errors.ts"; import { @@ -408,7 +411,14 @@ export async function runTurn( request.runContext, env.clientToolRelayRef.current, relayGuard, - { log: logger }, + { + log: logger, + // Derived from the run plan's client-tool pause disposition (the closed set lives at + // the client-tool boundary; the relay only needs the boolean). + writePausedAnswer: relayWritesPausedAnswer( + plan.clientToolPauseDisposition, + ), + }, ); // Ordering invariant: the relay's stale-file sweep must complete before the // resume's respondPermission or the fresh prompt below can cause a legitimate diff --git a/services/runner/src/engines/sandbox_agent/tool-mcp-assets.ts b/services/runner/src/engines/sandbox_agent/tool-mcp-assets.ts index 59ddd86a06..c8c7e78a6d 100644 --- a/services/runner/src/engines/sandbox_agent/tool-mcp-assets.ts +++ b/services/runner/src/engines/sandbox_agent/tool-mcp-assets.ts @@ -14,11 +14,10 @@ * Schemas are unbounded. This module owns the bundle location and the upload; `mcp.ts` stays * pure (it builds the session entry, it does not touch files). * - * FAIL LOUD: the caller only invokes this on the path that REQUIRES the shim (Daytona + - * non-Pi + executable tools) — without it the tools silently never surface (the F-042 / - * silent-tool-drop bug). So a missing bundle or a failed upload THROWS the named message and - * the engine turns it into `{ok: false, error}` rather than proceeding with an empty tool - * channel. + * FAIL LOUD: the caller only invokes this on the path that REQUIRES the shim (Daytona + non-Pi + + * tools, executable and client alike) — without it the tools silently never surface (the F-042 / + * silent-tool-drop bug). So a missing bundle or a failed upload THROWS the named message and the + * engine turns it into `{ok: false, error}` rather than proceeding with an empty tool channel. */ import { existsSync, readFileSync } from "node:fs"; import { join } from "node:path"; diff --git a/services/runner/src/tools/dispatch.ts b/services/runner/src/tools/dispatch.ts index 273753cfc7..f81dfa0e2c 100644 --- a/services/runner/src/tools/dispatch.ts +++ b/services/runner/src/tools/dispatch.ts @@ -22,7 +22,7 @@ import type { ResolvedToolSpec } from "../protocol.ts"; import { callAgentaTool } from "./callback.ts"; import { CODE_TOOL_UNSUPPORTED_MESSAGE } from "./code.ts"; -import { relayToolCall } from "./relay-client.ts"; +import { relayToolCall, RELAY_PAUSED } from "./relay-client.ts"; import { assertRequiredArguments } from "./spec-schema.ts"; // Compatibility re-export: the writer moved to `relay-client.ts`; importers that still @@ -43,6 +43,22 @@ export interface RunResolvedToolOpts { signal?: AbortSignal; } +/** + * This dispatch path (the Pi extension and the local loopback MCP) never opts into the cold-pause + * protocol — `writePausedAnswer` is off for Pi and the local MCP parks in-process — so a + * `RELAY_PAUSED` reaching here is a contract violation. Fail loud rather than laundering the + * sentinel into a string; only the in-sandbox shim handles a pause. + */ +function assertNotPaused( + name: string, + result: string | typeof RELAY_PAUSED, +): string { + if (result === RELAY_PAUSED) { + throw new Error(`unexpected paused relay answer for tool '${name}'`); + } + return result; +} + /** * Execute one resolved tool and return its result text. Throws on failure; every call site * turns the throw into a tool-error result so the model loop continues rather than crashing. @@ -67,13 +83,16 @@ export async function runResolvedTool( } if (spec.kind === "client") { if (opts.relayDir) { - return relayToolCall( - opts.relayDir, + return assertNotPaused( spec.name, - opts.toolCallId, - params, - spec.timeoutMs, - opts.signal, + await relayToolCall( + opts.relayDir, + spec.name, + opts.toolCallId, + params, + spec.timeoutMs, + opts.signal, + ), ); } throw new Error( @@ -82,13 +101,16 @@ export async function runResolvedTool( } // callback (default): route back to Agenta's /tools/call (directly or via the Daytona relay). if (opts.relayDir) { - return relayToolCall( - opts.relayDir, + return assertNotPaused( spec.name, - opts.toolCallId, - params, - spec.timeoutMs, - opts.signal, + await relayToolCall( + opts.relayDir, + spec.name, + opts.toolCallId, + params, + spec.timeoutMs, + opts.signal, + ), ); } return callAgentaTool( diff --git a/services/runner/src/tools/relay-client.ts b/services/runner/src/tools/relay-client.ts index 5a2bc35f9b..da3e4a84b7 100644 --- a/services/runner/src/tools/relay-client.ts +++ b/services/runner/src/tools/relay-client.ts @@ -227,6 +227,13 @@ export async function waitForRelayResponse( * Daytona tool call: the in-sandbox process can't reach Agenta, so write the request to a * file the runner watches and poll for the response it writes back (see tools/relay.ts). */ +/** + * Sentinel returned by `relayToolCall` when a browser-fulfilled client tool parked. A distinct + * value (not a throw, not an empty string) so the shim can tell a pause apart from a normal result + * or a failure and return a benign wait result to the harness (see tool-mcp-stdio.ts). + */ +export const RELAY_PAUSED = Symbol("relay-paused"); + export async function relayToolCall( dir: string, toolName: string, @@ -234,7 +241,7 @@ export async function relayToolCall( params: unknown, timeoutMs?: number, signal?: AbortSignal, -): Promise { +): Promise { const { reqPath, resPath } = publishRelayRequest(dir, { toolName, toolCallId, @@ -255,6 +262,8 @@ export async function relayToolCall( } throw err; } + // Cleanup runs for every answer variant, pause included; the turn-start stale sweep is only the + // backstop for a torn-down shim that never reached this line. try { unlinkSync(reqPath); } catch { @@ -265,6 +274,9 @@ export async function relayToolCall( } catch { /* best-effort cleanup */ } + // Check the pause before the ok/text path: a paused answer is `ok: true` with no text, and the + // shim must not read it as an empty success. + if (res.paused) return RELAY_PAUSED; if (res.ok) return res.text ?? ""; throw new Error(res.error || `tool relay failed for ${toolName}`); } diff --git a/services/runner/src/tools/relay-protocol.ts b/services/runner/src/tools/relay-protocol.ts index 9924447673..f3c30003b8 100644 --- a/services/runner/src/tools/relay-protocol.ts +++ b/services/runner/src/tools/relay-protocol.ts @@ -31,6 +31,15 @@ export interface ExecuteRelayResponse { ok: boolean; text?: string; error?: string; + /** + * The "paused" answer variant: a client tool parked, so the turn ends and the browser result + * returns on the cold-replay resume. An answer is one of three — success (`ok: true, text`), + * failure (`ok: false, error`), or pause (`ok: true, paused: true`). Additive but NOT blindly + * backward-compatible: a reader that ignores `paused` misreads a pause as an empty-text success, + * so the shim must check `paused` before `ok` (see relay-client.ts). Written only on the non-Pi + * in-sandbox shim path (`writePausedAnswer`). + */ + paused?: true; } export type RelayResponse = ExecuteRelayResponse; diff --git a/services/runner/src/tools/relay.ts b/services/runner/src/tools/relay.ts index 684fa0c191..be81097b91 100644 --- a/services/runner/src/tools/relay.ts +++ b/services/runner/src/tools/relay.ts @@ -513,10 +513,13 @@ export function startToolRelay( runContext?: RunContext, clientToolRelay?: ClientToolRelay, guard?: RelayExecutionGuard, - opts?: { log?: (msg: string) => void }, + opts?: { log?: (msg: string) => void; writePausedAnswer?: boolean }, ): { ready: Promise; stop: () => Promise } { let active = true; const log = opts?.log ?? (() => {}); + // Single switch between the two pause outcomes; defaults OFF so an un-flagged caller keeps Pi's + // write-no-answer behavior. + const writePausedAnswer = opts?.writePausedAnswer ?? false; // Telemetry gate: without a log sink there is nowhere for pickup_ms to go, so the // stat (a daemon round-trip on Daytona) is skipped entirely. const telemetry = opts?.log !== undefined; @@ -561,8 +564,15 @@ export function startToolRelay( clientToolRelay, guard, ); - if (text === PAUSED) return; - res = { ok: true, text }; + if (text === PAUSED) { + // A client tool parked. Pi writes no answer; the non-Pi shim gets a benign paused answer so + // it ends its blocking `tools/call` at once instead of waiting out the per-tool timeout and + // emitting a late error frame (see ClientToolPauseDisposition). + if (!writePausedAnswer) return; + res = { ok: true, paused: true }; + } else { + res = { ok: true, text }; + } } catch (err) { res = { ok: false, diff --git a/services/runner/src/tools/tool-mcp-stdio.ts b/services/runner/src/tools/tool-mcp-stdio.ts index 308557f32c..2193de4c3d 100644 --- a/services/runner/src/tools/tool-mcp-stdio.ts +++ b/services/runner/src/tools/tool-mcp-stdio.ts @@ -49,7 +49,7 @@ import { fileURLToPath } from "node:url"; import type { AdvertisedToolSpec } from "./public-spec.ts"; // The file-relay CLIENT only (never dispatch.ts): the bundle carries file-relay code alone — // no direct callback (`/tools/call` POST) or code executor belongs in a sandbox child. -import { relayToolCall } from "./relay-client.ts"; +import { relayToolCall, RELAY_PAUSED } from "./relay-client.ts"; // The two required env names — the whole input contract of the shim. They live in the // dependency-free `tool-mcp-env.ts` so server code (`engines/sandbox_agent/mcp.ts`) shares // them without importing this bundle entrypoint; re-exported here for the shim's consumers. @@ -230,6 +230,27 @@ export async function handleToolMcpMessage( params?.arguments, spec.timeoutMs, ); + if (text === RELAY_PAUSED) { + // A client tool parked; the runner has already ended the turn. Return a benign, NON-error + // result so the harness's turn ends cleanly instead of waiting out the per-tool timeout and + // emitting a late error frame. The text tells the model not to retry while the pause + // settles. The reserved warm-hold outcome would hold this call open instead (see #5384). + return { + jsonrpc: "2.0", + id, + result: { + content: [ + { + type: "text", + text: + `The user has been asked to complete "${name}" in their browser. ` + + `This is being handled outside this tool call. Do not call "${name}" again ` + + `or retry — the result will be delivered automatically once the user responds.`, + }, + ], + }, + }; + } return { jsonrpc: "2.0", id, diff --git a/services/runner/tests/unit/client-tools.test.ts b/services/runner/tests/unit/client-tools.test.ts index 8773601704..7902c47d88 100644 --- a/services/runner/tests/unit/client-tools.test.ts +++ b/services/runner/tests/unit/client-tools.test.ts @@ -23,8 +23,19 @@ import { buildClientToolRelay, createToolCallCorrelationIndex, emitClientToolInteraction, + relayWritesPausedAnswer, } from "../../src/engines/sandbox_agent/client-tools.ts"; +describe("relayWritesPausedAnswer (client-tool pause disposition)", () => { + it("only the cold-acknowledge disposition writes the paused answer", () => { + // The single derived switch the relay consumes. The exhaustive mapping is what keeps the + // reserved warm hold from silently being treated as cold. + assert.equal(relayWritesPausedAnswer("cold-acknowledge"), true); + assert.equal(relayWritesPausedAnswer("pi-native"), false); + assert.equal(relayWritesPausedAnswer("warm-hold"), false); + }); +}); + function responderReturning(verdict: ClientToolVerdict): Responder { return { async onPermission() { diff --git a/services/runner/tests/unit/relay-client.test.ts b/services/runner/tests/unit/relay-client.test.ts index 834940f547..3a62b4995f 100644 --- a/services/runner/tests/unit/relay-client.test.ts +++ b/services/runner/tests/unit/relay-client.test.ts @@ -24,6 +24,7 @@ import { join } from "node:path"; import { createRelayDirWatch, publishRelayRequest, + RELAY_PAUSED, relayToolCall, waitForRelayResponse, } from "../../src/tools/relay-client.ts"; @@ -116,6 +117,30 @@ describe("relayToolCall (writer round-trip)", () => { } }); + it("maps a paused answer to the RELAY_PAUSED sentinel and cleans up both files", async () => { + const dir = tempDir(); + try { + const reqPath = join(dir, `call-pause${RELAY_REQ_SUFFIX}`); + const resPath = join(dir, `call-pause${RELAY_RES_SUFFIX}`); + // Play the runner: write the paused answer variant. + setTimeout(() => { + writeFileSync(resPath, JSON.stringify({ ok: true, paused: true })); + }, 50); + const out = await relayToolCall(dir, "request_connection", "call-pause", { + integration: "slack", + }); + assert.equal( + out, + RELAY_PAUSED, + "a paused answer returns the sentinel, not an empty string and not a throw", + ); + assert.ok(!existsSync(reqPath), "request file was cleaned up on pause"); + assert.ok(!existsSync(resPath), "response file was cleaned up on pause"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it("rejects with the response error message on ok:false", async () => { const dir = tempDir(); try { diff --git a/services/runner/tests/unit/relay-loop.test.ts b/services/runner/tests/unit/relay-loop.test.ts index d748ba818e..71d8c161d2 100644 --- a/services/runner/tests/unit/relay-loop.test.ts +++ b/services/runner/tests/unit/relay-loop.test.ts @@ -40,6 +40,8 @@ import { sweepStaleRelayFiles, type RelayHost, } from "../../src/tools/relay.ts"; +import type { ClientToolRelay } from "../../src/tools/client-tool-relay.ts"; +import type { ResolvedToolSpec } from "../../src/protocol.ts"; import { RELAY_SAFETY_POLL_MS, daytonaRelayActivitySource, @@ -812,3 +814,83 @@ describe("startToolRelay stage=relay_pickup telemetry", () => { await relay.stop(); }); }); + +describe("startToolRelay client-tool pause (the paused-cold answer)", () => { + const clientSpec: ResolvedToolSpec = { + name: "request_connection", + kind: "client", + description: "browser-fulfilled", + }; + + /** A client-tool seam that always parks: the browser has not answered yet. */ + function parkingRelay(onParked?: () => void): ClientToolRelay { + return { + onClientTool: async () => "pendingApproval", + onPause: () => onParked?.(), + }; + } + + function putClientRequest(files: Map, id: string): void { + files.set( + `${DIR}/${id}.req.json`, + JSON.stringify({ + toolName: "request_connection", + toolCallId: id, + args: { integration: "slack" }, + }), + ); + } + + it("non-Pi shim path (writePausedAnswer) writes { ok: true, paused: true } when a client tool parks", async () => { + const { host, files } = fakeHost(); + const relay = startToolRelay( + host, + DIR, + [clientSpec], + undefined, + undefined, + parkingRelay(), + undefined, + { writePausedAnswer: true }, + ); + + putClientRequest(files, "call-1"); + await until(() => files.has(`${DIR}/call-1.res.json`), "the paused answer"); + const res = JSON.parse(files.get(`${DIR}/call-1.res.json`) ?? "{}"); + assert.deepEqual( + res, + { ok: true, paused: true }, + "the shim path gets a benign paused answer, not an error or an empty success", + ); + + await relay.stop(); + }); + + it("Pi path (no writePausedAnswer) writes NO answer file when a client tool parks", async () => { + const { host, files } = fakeHost(); + let parked = false; + const relay = startToolRelay( + host, + DIR, + [clientSpec], + undefined, + undefined, + parkingRelay(() => { + parked = true; + }), + undefined, + {}, // writePausedAnswer defaults OFF -> Pi's native pause, no answer file + ); + + putClientRequest(files, "call-1"); + await until(() => parked, "the client tool parked"); + // Settle long enough that a (wrong) answer write would have landed. + await sleep(RELAY_POLL_MS + 50); + assert.ok( + !files.has(`${DIR}/call-1.res.json`), + "Pi parks through its own extension; the loop writes no answer file", + ); + + await relay.stop(); + }); +}); diff --git a/services/runner/tests/unit/sandbox-agent-run-plan.test.ts b/services/runner/tests/unit/sandbox-agent-run-plan.test.ts index c4597c424c..0581fd260b 100644 --- a/services/runner/tests/unit/sandbox-agent-run-plan.test.ts +++ b/services/runner/tests/unit/sandbox-agent-run-plan.test.ts @@ -676,7 +676,7 @@ describe("buildRunPlan", () => { assert.equal(result.ok, true); }); - it("allows claude x daytona x mixed tools and keeps only executable shim specs", () => { + it("allows claude x daytona x mixed tools and advertises both kinds on the shim", () => { const result = buildRunPlan( { harness: "claude", @@ -702,14 +702,17 @@ describe("buildRunPlan", () => { result.plan.executableToolSpecs.map((tool) => tool.name), ["server_tool"], ); + assert.equal( + result.plan.clientToolPauseDisposition, + "cold-acknowledge", + ); }); - it("refuses claude x daytona x client-ONLY tools (nothing deliverable to advertise)", () => { - // The shim advertises only executable tools; client tools are omitted. A run whose entire - // tool set is client-kind has nothing to deliver on Daytona, so it must fail CLOSED up - // front rather than proceed, drop every tool, and return ok:true (the F1 zero-tools drop - // that mcp.ts's "run-plan should have refused this run" log points at). - let created = false; + it("allows claude x daytona x client-ONLY tools (the shim advertises them and the relay parks)", () => { + // The shim now advertises client tools too. A client-only run on Daytona builds a normal + // plan: the client tool rides toolSpecs, and the pause disposition is "cold-acknowledge" so + // a parked client tool gets a benign paused answer (the runner ends the turn; the browser + // result returns on the cold-replay resume). This replaces the interim #5366 refusal. const result = buildRunPlan( { harness: "claude", @@ -718,26 +721,24 @@ describe("buildRunPlan", () => { customTools: [{ name: "request_connection", kind: "client" }], } as AgentRunRequest, { - createDaytonaCwd: () => { - created = true; - return "/home/sandbox/agenta-fixed"; - }, + createDaytonaCwd: () => "/home/sandbox/agenta-fixed", }, ); - assert.equal(result.ok, false); - if (result.ok) return; - assert.match(result.error, /Client tools are not deliverable/); - assert.match(result.error, /entirely\s+client-kind/); - assert.match( - result.error, - /docs\/design\/agent-workflows\/projects\/in-sandbox-tool-mcp\//, + assert.equal(result.ok, true); + if (!result.ok) return; + assert.deepEqual( + result.plan.toolSpecs.map((tool) => tool.name), + ["request_connection"], ); + // No executable tool remains, but the run is no longer refused. + assert.deepEqual(result.plan.executableToolSpecs, []); assert.equal( - created, - false, - "fails before any cwd is created (up-front gate)", + result.plan.clientToolPauseDisposition, + "cold-acknowledge", + "non-Pi shim path acknowledges a parked client tool with a paused answer", ); + assert.equal(result.plan.useToolRelay, true); }); it("allows pi x daytona x client-only tools (Pi's extension + file relay deliver them)", () => { @@ -752,6 +753,9 @@ describe("buildRunPlan", () => { ); assert.equal(result.ok, true); + if (!result.ok) return; + // Pi parks through its own extension, so its disposition is "pi-native" (no paused answer). + assert.equal(result.plan.clientToolPauseDisposition, "pi-native"); }); it("still refuses claude x daytona x executable tools under strict restricted network", () => { diff --git a/services/runner/tests/unit/session-mcp-layering.test.ts b/services/runner/tests/unit/session-mcp-layering.test.ts index 211092e4a0..ab7180d730 100644 --- a/services/runner/tests/unit/session-mcp-layering.test.ts +++ b/services/runner/tests/unit/session-mcp-layering.test.ts @@ -350,7 +350,11 @@ describe("buildSessionMcpServers layering (do-not-merge regression guard)", () = assert.deepEqual(servers, [], "Pi never uses the stdio shim"); }); - it("(Daytona, non-Pi, internalToolMcp, only client tools) -> no internal entry (nothing executable)", async () => { + it("(Daytona, non-Pi, internalToolMcp, only client tools) -> advertises the shim (client tools ride it too)", async () => { + // The shim now advertises client tools as well as executable ones. A client-only run on + // Daytona must still build the internal stdio channel so Claude sees the client tool and can + // call it; the parked call resolves through the relay's paused answer. This is the direct + // guard against the old client-only silent drop. const clientTool: ResolvedToolSpec = { name: "request_connection", kind: "client", @@ -368,10 +372,10 @@ describe("buildSessionMcpServers layering (do-not-merge regression guard)", () = specsPath: "/home/sandbox/x/tool-mcp-specs.json", }, }); - assert.deepEqual( - servers, - [], - "a client-only spec list advertises no stdio channel", + const stdio = servers.find((s) => s.name === "agenta-tools"); + assert.ok( + stdio, + "a client-only spec list still advertises the internal stdio channel", ); }); diff --git a/services/runner/tests/unit/tool-mcp-stdio.test.ts b/services/runner/tests/unit/tool-mcp-stdio.test.ts index 07f55843db..cb93f847e5 100644 --- a/services/runner/tests/unit/tool-mcp-stdio.test.ts +++ b/services/runner/tests/unit/tool-mcp-stdio.test.ts @@ -284,6 +284,38 @@ describe("tool-mcp-stdio handler", () => { assert.equal(res.result.isError, true); assert.match(res.result.content[0].text, /upstream 500/); }); + + it("tools/call maps a paused relay answer to a benign, NON-error wait result", async () => { + const dir = tempDir("agenta-tool-mcp-"); + const pending = handleToolMcpMessage( + { + jsonrpc: "2.0", + id: 6, + method: "tools/call", + params: { name: "get_weather", arguments: {} }, + }, + specs, + dir, + ); + await waitFor(() => + readdirSync(dir).some((f) => f.endsWith(RELAY_REQ_SUFFIX)), + ); + const reqName = readdirSync(dir).find((f) => f.endsWith(RELAY_REQ_SUFFIX))!; + const id = reqName.slice(0, -RELAY_REQ_SUFFIX.length); + writeFileSync( + join(dir, `${id}${RELAY_RES_SUFFIX}`), + JSON.stringify({ ok: true, paused: true }), + "utf-8", + ); + const res: any = await pending; + // A pause is NOT an error: the harness's turn ends cleanly instead of waiting out the + // per-tool timeout and emitting a late error frame. + assert.notEqual(res.result.isError, true); + // The fixed template names the tool and tells the model not to retry. + assert.match(res.result.content[0].text, /get_weather/); + assert.match(res.result.content[0].text, /[Dd]o not call/); + assert.match(res.result.content[0].text, /retry/); + }); }); describe("shim -> real relay loop round trip (semantic writer/reader contract)", () => {