From f731d91125ab721e2eea547b2562df3710596852 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Fri, 10 Jul 2026 14:50:23 +0200 Subject: [PATCH 1/3] docs(design): plan exact MCP client-tool continuation --- .../mcp-client-tool-continuation/README.md | 44 ++++ .../mcp-client-tool-continuation/context.md | 119 +++++++++ .../mcp-client-tool-continuation/interface.md | 193 +++++++++++++++ .../open-questions.md | 50 ++++ .../mcp-client-tool-continuation/plan.md | 233 ++++++++++++++++++ .../mcp-client-tool-continuation/qa.md | 135 ++++++++++ .../mcp-client-tool-continuation/research.md | 164 ++++++++++++ .../mcp-client-tool-continuation/status.md | 51 ++++ 8 files changed, 989 insertions(+) create mode 100644 docs/design/agent-workflows/projects/mcp-client-tool-continuation/README.md create mode 100644 docs/design/agent-workflows/projects/mcp-client-tool-continuation/context.md create mode 100644 docs/design/agent-workflows/projects/mcp-client-tool-continuation/interface.md create mode 100644 docs/design/agent-workflows/projects/mcp-client-tool-continuation/open-questions.md create mode 100644 docs/design/agent-workflows/projects/mcp-client-tool-continuation/plan.md create mode 100644 docs/design/agent-workflows/projects/mcp-client-tool-continuation/qa.md create mode 100644 docs/design/agent-workflows/projects/mcp-client-tool-continuation/research.md create mode 100644 docs/design/agent-workflows/projects/mcp-client-tool-continuation/status.md diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/README.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/README.md new file mode 100644 index 0000000000..6613ebfc61 --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/README.md @@ -0,0 +1,44 @@ +# Exact client-tool continuation + +Claude currently ends an Agenta client-tool turn by closing the internal MCP request without a +result. When the browser returns the result, the runner starts or loads a session and Claude asks +for the tool again. That path is safe, but it cannot preserve the original call id or arguments. + +This project adds a faster exact path for local Claude sessions. The runner keeps the original MCP +request and harness prompt open, parks the session, and writes the browser result to that same +JSON-RPC request. Cold replay remains the fallback for expiry, restart, disconnect, pool pressure, +wrong-replica routing, and unsupported environments. + +The first implementation does not build an MCP gateway. It defines a transport-neutral +continuation interface so a future gateway can own authentication, routing, and durable pending +operations without changing the session state machine. + +## Status + +Design only. Implementation has not started. + +The proposed rollout is local Claude only. Pi approval parking is already implemented through the +ACP permission plane. Non-Pi Daytona runs cannot receive Agenta tools through the current internal +MCP endpoint because the endpoint binds to the runner's loopback interface. + +## Documents + +- [context.md](context.md) explains the current behavior, goals, scope, and user-visible result. +- [research.md](research.md) records the code findings and the effect of PRs #5153, #5185, and + #5197. +- [interface.md](interface.md) defines the gateway-neutral pending-operation contract and state + machine. +- [plan.md](plan.md) splits the work into progressive work packages and rollout gates. +- [qa.md](qa.md) defines unit, integration, live, security, and resource verification. +- [open-questions.md](open-questions.md) lists the decisions that still need review. +- [status.md](status.md) is the live source of truth for progress and dependencies. + +## Proposed sequence + +1. Measure Claude's MCP request timeout. +2. Authenticate and test the existing internal loopback MCP endpoint. +3. Add the transport-neutral continuation registry with no behavior change. +4. Add the local Claude hold-open and resume path behind a runner kill switch. +5. Close race, timeout, shutdown, and resource-limit cases before enabling it. +6. Canary locally and keep Daytona and cross-replica exact routing on the cold fallback. + diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/context.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/context.md new file mode 100644 index 0000000000..8435b61b2c --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/context.md @@ -0,0 +1,119 @@ +# Context + +## The two tool paths called MCP + +User-configured MCP servers and Agenta client tools are different paths. + +- A user MCP server is supplied by the user. Claude calls that server directly. +- An Agenta tool is resolved by the platform. For local Claude, the runner exposes those tools + through a small internal MCP HTTP server in `tool-mcp-http.ts`. +- A client tool is an Agenta tool whose result comes from the browser. `request_connection` is one + example. The runner must pause while the user completes the browser interaction. + +This project changes only the third path. It does not change user MCP servers, tool resolution, Pi +tool delivery, or the public `/run` request shape. + +## Current client-tool flow + +The current local Claude flow is: + +1. Claude sends `tools/call` to the runner's loopback MCP server. +2. The runner validates the arguments, correlates the call with Claude's ACP tool-call id, emits a + `client_tool` interaction, and pauses the turn. +3. `tool-mcp-http.ts` returns the internal `MCP_PAUSED` sentinel. The HTTP listener calls + `res.destroy()` without writing a JSON-RPC result. +4. The runner tears down the prompt or session unless another parkable gate owns it. +5. The browser adds a `tool_result` to a new `/run` request. +6. The cold path indexes that result by tool name and canonical arguments. Claude reissues the + client tool, and the runner returns the stored browser output. + +The cold path is fail-safe. If the model changes the arguments, the stored result does not match +and the user sees the interaction again. The cost is that the original MCP request is gone, the +new call has a new id, and the model has made another decision. + +## Desired warm flow + +The exact path changes steps 3 through 6: + +1. The runner leaves the original JSON-RPC response open. +2. The session pool parks the live harness in `awaiting_client_tool` with the original prompt + promise and a transport-neutral pending-operation handle. +3. The browser result arrives in a new `/run` request and matches the original ACP tool-call id. +4. The runner atomically claims the pending operation and writes the result to the held JSON-RPC + response. +5. Claude settles the original tool call and continues the original prompt. The new `/run` owns + streaming and tracing for the continuation. + +There is no new model request between the browser result and continuation of the original prompt. + +## How PR 5197 changes the fallback + +PR #5197 adds native harness session continuity, durable harness-session ids, reconnectable Daytona +sandboxes, and `session/load`. Those changes improve the path after a live park is lost. They do +not answer an MCP request that was already destroyed. + +After PR #5197: + +- A live pending operation is still the only exact path. +- If the live operation is gone, `session/load` restores structured harness history when eligible. +- Claude settles the interrupted pending call and reissues it with a new id. The stored browser + output answers that new call through the existing cold decision store. +- Plain cold replay remains the final fallback when native session load is unavailable or stale. + +The pending socket must not be stored in `session_states.data`. A Node `ServerResponse` belongs to +one process and cannot survive a process restart. PR #5197's durable state is context recovery, not +a pending-operation ledger. + +## Goals + +- Continue the original local Claude MCP call after a browser result arrives. +- Preserve the original harness tool-call id, tool name, arguments, and prompt. +- Keep cold replay as a correct fallback for every failure and unsupported case. +- Authenticate the internal MCP endpoint before increasing its lifetime. +- Bound pending sessions, sockets, output size, and wait time. +- Make duplicate, late, expired, and disconnected results deterministic. +- Expose metrics that distinguish exact continuation from cold fallback. +- Define an interface that an in-memory runner implementation and a future MCP gateway can both + implement. + +## Non-goals + +- Building or selecting a production MCP gateway. +- Making the runner-loopback MCP endpoint reachable from Daytona. +- Adding cross-replica forwarding or changing load-balancer routing. +- Guaranteeing end-to-end exactly-once delivery across a runner crash. +- Changing the public `/run` wire shape in the first implementation. +- Replacing the existing cold name-and-arguments matching path. +- Supporting MCP batches that contain a client tool. +- Supporting more than one pending client tool per session in the first release. +- Changing Pi. Pi approval gates already use the ACP permission plane. + +## Invariants + +1. A browser result can complete only a pending operation in the same project and session and with + the same harness tool-call id. +2. One pending operation has at most one successful claimant and one terminal outcome. +3. No tool arguments, browser outputs, bearer tokens, or credentials appear in logs or metrics. +4. A transport failure never discards the inbound browser result before cold fallback can read it. +5. Expiry, shutdown, client disconnect, pool eviction, and duplicate completion close the held + response and reclaim the session. +6. Unsupported cases use today's cold path. They do not fail the run only because exact + continuation is unavailable. +7. The default remains off until timeout, security, race, and resource tests pass. + +## Scope of the first release + +The first release requires all of the following: + +- Claude or another non-Pi harness using the internal MCP channel. +- A local sandbox, because the internal MCP endpoint is runner-loopback only. +- Session keepalive enabled. +- The client-tool continuation kill switch enabled. +- A single JSON-RPC call, not a batch. +- No other pending client tool in the same session. +- The resume request reaches the runner replica that owns the live operation. + +If any condition is false, the runner closes the original request as it does today and uses cold +replay. A multi-replica deployment can therefore remain correct without new routing, although its +exact-continuation hit rate may be lower. + diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/interface.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/interface.md new file mode 100644 index 0000000000..b8de996b73 --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/interface.md @@ -0,0 +1,193 @@ +# Gateway-neutral continuation interface + +## Design rule + +The contract describes a pending client-tool operation. It does not describe Node HTTP objects or +the runner's current pool implementation. + +Each field is grouped by semantic role: + +- `identity` identifies the operation and browser interaction. +- `scope` identifies the tenant boundary that may complete it. +- `routing` says where the live owner can be reached. +- `protocol` carries MCP-specific request context. +- `tool` describes the call without storing raw input. +- `lifecycle` controls expiry and terminal state. + +Credentials stay under the MCP connection that they authenticate. They do not appear in operation +metadata. + +## Operation shape + +The following TypeScript is a design contract, not a required final spelling: + +```ts +type PendingClientToolState = + | "pending" + | "delivering" + | "delivered" + | "cancelled" + | "expired"; + +interface PendingClientToolOperation { + identity: { + operationId: string; + interactionId: string; + harnessToolCallId: string; + }; + scope: { + projectId: string; + sessionId: string; + }; + routing: { + runnerInstanceId: string; + environmentId: string; + }; + protocol: { + transport: "mcp_streamable_http"; + requestId: string | number; + }; + tool: { + name: string; + inputDigest: string; + }; + lifecycle: { + state: PendingClientToolState; + createdAt: number; + expiresAt: number; + }; +} +``` + +`inputDigest` detects internal mismatches without retaining or logging raw arguments. It is not an +authorization decision and is not the live browser-result key. + +## Field classification + +| Field | Role | Owner | Lifetime | Reason | +| --- | --- | --- | --- | --- | +| `operationId` | Identity | Continuation registry | One operation | Stable internal idempotency key. | +| `interactionId` | Protocol context | Interaction producer | One operation | Connects to the durable interaction row and UI event. | +| `harnessToolCallId` | Protocol context | Harness correlation index | One harness call | Exact browser-result key for the live path. | +| `projectId`, `sessionId` | Scope | Platform | Session | Prevents cross-project and cross-session completion. | +| `runnerInstanceId` | Routing | Runner deployment | Process lifetime | Identifies the process that owns the live handle. | +| `environmentId` | Routing | Session engine | Live environment lifetime | Prevents a stale operation from completing a replacement session. | +| `transport` | Protocol context | Delivery adapter | One operation | Selects the adapter without exposing its implementation. | +| `requestId` | Protocol context | MCP client | One JSON-RPC request | Required in the JSON-RPC response. | +| `name`, `inputDigest` | Data description | Tool call | One operation | Supports validation and safe diagnosis without raw input. | +| `state`, timestamps | Lifecycle policy | Registry | One operation | Bounds ownership, retries, and cleanup. | + +## Delivery port + +The HTTP implementation can close over a Node `ServerResponse`, but only the adapter can see it: + +```ts +type DeliveryResult = "written" | "transport_closed"; + +interface ClientToolResultDelivery { + deliver(output: unknown): Promise; + cancel(reason: string): Promise; + onClosed(listener: () => void): () => void; +} +``` + +The in-memory registry stores the interface, not the response object. A later gateway adapter can +implement the same port with a gateway result endpoint or message queue. + +## Registry contract + +```ts +interface ClientToolContinuationRegistry { + register( + operation: PendingClientToolOperation, + delivery: ClientToolResultDelivery, + ): "registered" | "duplicate" | "capacity_exceeded"; + + claim(input: { + projectId: string; + sessionId: string; + harnessToolCallId: string; + environmentId: string; + }): + | { kind: "claimed"; operation: PendingClientToolOperation; delivery: ClientToolResultDelivery } + | { kind: "missing" | "duplicate" | "expired" | "wrong_owner" }; + + markDelivered(operationId: string): void; + cancel(operationId: string, reason: string): Promise; + cancelEnvironment(environmentId: string, reason: string): Promise; + size(): number; +} +``` + +`claim` is atomic. It changes `pending` to `delivering`. Only the claimant receives the delivery +port. A second caller observes `duplicate` or a terminal state and cannot write a second response. + +## State machine + +| From | Event | To | Required action | +| --- | --- | --- | --- | +| None | Valid single client-tool call | `pending` | Register before emitting pause. | +| `pending` | Exact browser result arrives | `delivering` | One atomic claimant receives the port. | +| `delivering` | MCP response write succeeds | `delivered` | Continue the original prompt and resolve the interaction. | +| `pending` or `delivering` | Socket closes | `cancelled` | Evict the live session and keep cold fallback available. | +| `pending` or `delivering` | Session teardown or shutdown | `cancelled` | Close the response and destroy the environment. | +| `pending` | TTL expires | `expired` | Close the response and destroy the environment. | +| Any terminal state | Late or duplicate result | Unchanged | Do not deliver again; use or retain cold fallback. | + +The local implementation provides at-most-once completion of a live handle. It cannot provide +end-to-end exactly-once delivery across a process crash. The browser-side action has already +completed before the result reaches the registry, and the cold path can return that stored output +without repeating the browser action. + +## Exact resume input + +The first implementation can derive the live resume input from the existing `/run` messages: + +```ts +interface ClientToolResumeInput { + projectId: string; + sessionId: string; + harnessToolCallId: string; + output: unknown; +} +``` + +The extractor accepts exactly one current-turn `tool_result` for the parked +`harnessToolCallId`. Missing or duplicate results do not claim the operation. The existing cold +store still indexes the same result by name and arguments if live delivery cannot proceed. + +## Loopback authentication + +Each internal MCP server receives a random per-environment bearer token. The ACP MCP connection +places it under the standard `authorization` header: + +```ts +{ + type: "http", + name: "agenta-tools", + url, + headers: [{ name: "authorization", value: `Bearer ${token}` }], +} +``` + +The server validates the token before `initialize`, `tools/list`, or `tools/call`. Agenta keeps the +token only for the live environment, never places it in operation metadata, and never logs it. The +harness receives it as connection credentials and may retain its own session configuration, so the +token has no value after the environment closes its dedicated server. + +## Future gateway mapping + +A real MCP gateway can implement the same roles with different ownership: + +| Local implementation | Future gateway implementation | +| --- | --- | +| In-memory registry | Durable pending-operation store with compare-and-set claim. | +| `runnerInstanceId` lookup | Gateway route to the owning runner or session worker. | +| Per-environment loopback bearer | Authenticated harness-to-gateway credential. | +| `McpHttpResultDelivery` | Gateway response stream or result-delivery endpoint. | +| Runner TTL timer | Gateway lease and retention policy. | +| Process-local metrics | Fleet-wide audit and quota metrics. | + +The gateway will still need authenticated actor context, encrypted result retention, revocation, +and acknowledgement semantics. Those concerns are not hidden inside `metadata` and are not +pretended to exist in the local implementation. diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/open-questions.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/open-questions.md new file mode 100644 index 0000000000..d5a1e91c31 --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/open-questions.md @@ -0,0 +1,50 @@ +# Open questions + +## 1. What timeout is enough to ship? + +The exact value must come from WP0. + +- Option A: require the held MCP request to exceed the five-minute approval TTL. +- Option B: require it to exceed the 60-second idle TTL, cap the live wait below the measured + ceiling, and use cold fallback for longer waits. +- Option C: ship any measurable hold, even below 60 seconds. + +Recommendation: Option B. It provides a useful fast path without claiming that the runner controls +Claude's client timeout. If the request does not survive 60 seconds, stop the implementation and +keep the current cold path. + +## 2. Should loopback authentication be part of this project? + +- Option A: add a per-environment bearer in WP1 before hold-open. +- Option B: leave loopback unauthenticated because the risk predates this project. + +Recommendation: Option A. The endpoint can execute Agenta tools, and hold-open extends its useful +lifetime. Authentication is a prerequisite, not unrelated cleanup. + +## 3. Should the first release add cross-replica routing? + +- Option A: route a browser result to the runner that owns the live operation. +- Option B: keep ownership process-local and use cold fallback on the wrong replica. + +Recommendation: Option B. Cross-replica routing belongs to the future gateway or a broader session +routing design. The neutral contract records the owner so that work can be added without changing +the operation model. + +## 4. Should the first release support multiple pending client tools? + +- Option A: allow multiple operations per session and support client-tool JSON-RPC batches. +- Option B: support one pending operation and reject client-tool batches before execution. + +Recommendation: Option B. The current pause latch and session resume path are single-gate. Multiple +pending calls require a separate interaction and ordering design. + +## 5. When is a live result considered delivered? + +- Option A: when the runner writes and flushes the JSON-RPC response. +- Option B: only after the runner also observes the matching harness tool-call completion update. + +Recommendation: implement Option A as the transport terminal state and record the harness update +as a separate continuation acknowledgement metric. If the harness does not continue, destroy the +session and leave the browser result available to cold fallback. A future gateway can add durable +acknowledgement without changing the delivery port. + diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/plan.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/plan.md new file mode 100644 index 0000000000..83aba7aa84 --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/plan.md @@ -0,0 +1,233 @@ +# Plan + +## Decision summary + +Build an exact local continuation path as a cache above the existing cold fallback. + +- Use the original ACP tool-call id for live browser-result correlation. +- Add a dedicated `awaiting_client_tool` pool state and resume verb. +- Keep raw HTTP state inside an MCP delivery adapter. +- Authenticate the current loopback endpoint before holding requests open. +- Support one pending client tool per local Claude session and reject client-tool batches. +- Add a runner-only kill switch. Do not add a frontend flag. +- Start the lifecycle-changing work only after PR #5197 merges and the branch rebases. +- Leave Daytona exact delivery, cross-replica routing, and a real MCP gateway out of scope. + +## Dependency order + +```text +WP0 timeout measurement + -> WP1 loopback hardening + -> WP2 neutral continuation kernel + -> WP3 local hold-open path + -> WP4 failure and resource envelope + -> WP5 canary and rollout +``` + +WP0 through WP2 can be prepared independently. WP3 edits the same environment, park, teardown, +and continuity code as PR #5197, so it starts after that PR lands. + +## WP0: measure the transport ceiling + +### Purpose + +The runner controls the server side of the internal MCP request. Claude controls the client-side +timeout. No design can promise a five-minute park until the real client is measured. + +### Work + +- Add a repeatable local Claude experiment that exposes one client tool and intentionally delays + its result. +- Record the pinned Claude harness, ACP adapter, MCP SDK, Node, and runner versions. +- Test a hold longer than the 60-second idle TTL first. +- If that succeeds, test beyond the 300-second approval TTL. +- Record whether Claude keeps the same MCP request id and ACP tool-call id, closes the connection, + retries, or settles the call with an error. +- Measure one quiet pending socket's file-descriptor and memory delta separately from the already + measured live Claude process tree. + +### Exit gate + +The first release proceeds only if the request remains usable beyond the 60-second idle TTL. If it +does not, keep the current cold path and stop the hold-open implementation. If it covers 60 seconds +but not five minutes, cap the live wait below the measured limit with a safety margin and fall back +cold for longer waits. + +### Deliverable + +A checked-in experiment protocol and report. No production behavior change. + +## WP1: harden the existing internal MCP endpoint + +### Purpose + +The endpoint already dispatches Agenta tools and currently relies only on loopback reachability. +Hold-open increases its lifetime, so authentication and direct transport tests come first. + +### Work + +- Generate a random bearer token per session environment. +- Advertise the token in the MCP server's standard `authorization` header. +- Reject missing or wrong tokens before parsing or dispatching a tool call. +- Keep the existing one-megabyte request-body cap and add an explicit result-size cap. +- Reject a JSON-RPC batch containing a client tool before executing any item in that batch. +- Keep non-client batches unchanged unless a test finds a protocol violation. +- Add focused HTTP integration tests for initialize, list, normal call, unauthorized call, + malformed JSON, oversized body, client-tool batch rejection, and close. +- Run one live local Claude test to confirm the ACP adapter forwards MCP headers. + +### Exit gate + +An unauthenticated sibling process cannot list or call tools. Valid local Claude behavior remains +unchanged. The MCP server integration suite owns its real socket lifecycle. + +### Rollback + +Revert this package independently. It does not depend on hold-open behavior. + +## WP2: add the neutral continuation kernel + +### Purpose + +Introduce the identity, lifecycle, and ownership rules before any HTTP request is held. + +### Work + +- Add the operation, registry, and delivery-port interfaces from [interface.md](interface.md). +- Implement an in-memory registry with atomic claim and terminal transitions. +- Enforce one pending client tool per environment and a global cap no greater than the session + pool cap. +- Add `environmentId` and `runnerInstanceId` routing identity without exposing them on the public + `/run` wire. +- Add expiry and environment-wide cancellation. +- Add counters and durations with ids, inputs, outputs, and credentials excluded. +- Add unit tests for registration, duplicate registration, capacity, claim races, late completion, + expiry, transport close, and environment cancellation. +- Add the runner-only `AGENTA_RUNNER_CLIENT_TOOL_CONTINUATION` kill switch, default off. + +### Exit gate + +The registry has full deterministic unit coverage. No request is held and current client-tool +behavior is byte-for-byte unchanged while the kill switch is off. + +## WP3: wire the local Claude hold-open path + +### Purpose + +Use the neutral kernel to resume the original MCP call and harness prompt. + +### Work + +#### Register and park + +- In the single-message `tools/call` path, create an `McpHttpResultDelivery` before pausing. +- Extend the client-tool relay result so the transport receives the correlated ACP tool-call id + and interaction id. +- Register the operation before emitting the pause. If registration fails, use today's + `MCP_PAUSED` destroy-and-cold behavior. +- Record `parkedClientTool` on the environment with the neutral operation and original prompt + promise. +- Teach the pause callback to preserve the MCP server and session only when a valid + `parkedClientTool` exists and the kill switch is on. +- Add `awaiting_client_tool` to the pool with the approval TTL capped by WP0's measured transport + ceiling. + +#### Claim and resume + +- Add an exact current-turn extractor for one `tool_result` with the parked ACP tool-call id. +- In `server.ts`, validate project, session, history fingerprint, mount expiry, operation owner, + and exact tool-call id. Do not compare newly minted per-turn credentials with the parked + environment, matching the existing approval-resume rule. +- Atomically check out the `awaiting_client_tool` session. A losing request cannot access the + delivery port. +- Add a `resumeClientTool` form to `runTurn`. It installs the new turn's stream and trace sink, + seeds the existing tool-call span, writes the real JSON-RPC result, and awaits the original + prompt promise. +- Resolve the interaction only after delivery succeeds and the continuation is accepted. +- Re-park a normally completed continuation as idle. A new gate can use its own state. + +#### Preserve fallback + +- Do not consume or remove the browser output from the inbound request before delivery succeeds. +- On a missing handle, wrong owner, closed transport, delivery failure, or lost checkout race, + destroy the stale live environment and run the existing cold path with the original request. +- Do not retry cold after continuation output has already streamed to the caller, matching the + current no-duplicate-stream rule. + +### Exit gate + +A real HTTP integration test proves that the original JSON-RPC request stays open, receives the +browser output once, and returns the same request id. A runner integration test proves that the +original prompt continues without a second model-issued client-tool call. + +### Rollback + +Disable `AGENTA_RUNNER_CLIENT_TOOL_CONTINUATION`. The code returns to the existing socket destroy +and cold replay path without a deployment rollback. + +## WP4: complete the failure and resource envelope + +### Purpose + +Default-off happy-path code is not ready to enable until every long-lived-resource exit is tested. + +### Work + +- Cancel and evict when the MCP client closes the held request. +- Cancel and evict on approval TTL, mount expiry, pool eviction, supersede, environment destroy, + runner shutdown, and failed original prompt. +- Reject duplicate browser results, duplicate `/run` requests, a result for another tool-call id, + and a second pending client tool in the same session. +- Keep one terminal result for each operation and make every cleanup method idempotent. +- Add global pending-operation and held-socket gauges. Alert before the configured pool cap. +- Confirm that pending sockets cannot exceed parked sessions and that both return to baseline after + expiry and shutdown. +- Add structured path metrics: `live`, `cold`, `unsupported`, `expired`, `wrong_replica`, + `transport_closed`, and `delivery_failed`. +- Add a bounded load test at the configured pool maximum and one over-cap request. +- Add process shutdown and restart tests that prove browser output remains usable by the cold path. + +### Exit gate + +- No cross-session or cross-project completion in tests. +- No duplicate completion in race tests. +- No held responses or registry entries after expiry, disconnect, shutdown, or destroy. +- The over-cap case falls back cold without exceeding the configured cap. +- Logs and metrics contain no arguments, outputs, bearer tokens, or credentials. + +## WP5: canary and rollout + +### Stage 1: development + +- Enable the kill switch on a single local runner replica. +- Run the live matrix in [qa.md](qa.md), including waits above 60 seconds and cold fallback after + forced expiry. +- Compare model-call traces between the exact and cold paths. The exact path must not contain a + second client-tool decision. + +### Stage 2: limited environment + +- Enable only where local Claude sessions and session keepalive are already enabled. +- Keep Daytona and other unsupported harnesses on the cold path. +- Watch pending count, completion path, wait duration, socket close, expiry, wrong-replica, pool + eviction, and process file descriptors. + +### Stage 3: default decision + +Decide whether to turn the runner flag on by default only after the canary establishes the real +timeout and fallback rates. Keep the kill switch after default-on so an MCP client upgrade can be +disabled without a frontend or API deployment. + +## Deployment and compatibility notes + +- PR #5197 must merge before WP3 starts. Rebase and re-check `sandbox_agent.ts`, `server.ts`, + `session-pool.ts`, continuity invalidation, and `shouldPark` before editing. +- PR #5197's Daytona auto-stop default is five minutes, the same as the approval TTL. This project + does not rely on that equality because the exact path is local only. +- A wrong replica cannot access the live handle. It uses cold fallback. A future gateway can route + the completion to the owner through the neutral registry contract. +- A future actor or principal id belongs in authenticated request context when the platform makes + it available. Do not infer one from a tool name, browser payload, or untrusted metadata. +- No new public wire field is required for the first release because the existing browser + `tool_result.toolCallId` is the exact live key. + diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/qa.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/qa.md new file mode 100644 index 0000000000..180ecbffdf --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/qa.md @@ -0,0 +1,135 @@ +# Verification plan + +## Test layers + +This project needs real transport tests in addition to engine unit tests. A fake response object +cannot prove socket lifetime, disconnect behavior, or JSON-RPC framing. + +| Layer | Purpose | Required before | +| --- | --- | --- | +| Pure unit | Registry transitions, exact matching, pool state, limits | WP2 merge | +| MCP HTTP integration | Authentication, framing, hold, write, close, batch behavior | WP1 and WP3 merge | +| Runner integration | Park, resume, race, expiry, shutdown, cold fallback | WP3 and WP4 merge | +| Live local Claude | Client timeout and original-prompt continuation | WP0 and rollout | +| Resource test | File descriptors, pending caps, cleanup | Enablement | +| Security test | Unauthorized process, token leakage, cross-scope result | Enablement | + +## Unit matrix + +### Registry + +- Register one operation. +- Reject a duplicate operation id. +- Reject a second pending operation for the same environment. +- Enforce the global cap. +- Allow exactly one claimant from two simultaneous claims. +- Reject wrong project, session, tool-call id, environment, and runner owner. +- Ignore duplicate and late completion after a terminal state. +- Expire and cancel idempotently. +- Cancel every operation owned by a destroyed environment. + +### Result extraction + +- Match exactly one current-turn result by ACP tool-call id. +- Reject no result, a different id, and two results with the same id. +- Do not use a prior turn's result. +- Preserve the same result for cold name-and-arguments fallback when live delivery fails. +- Keep approval envelopes separate from client-tool outputs. + +### Session pool + +- Park and check out `awaiting_client_tool` only through its dedicated method. +- Refuse idle and approval checkout for that state. +- Expire with a client-tool-specific reason. +- Do not evict another session after a checkout race. +- Destroy the environment exactly once on supersede, expiry, close, and shutdown. + +## MCP HTTP integration matrix + +Start `startInternalToolMcpServer` on a real ephemeral port and use a real HTTP client. + +- Missing and wrong bearer tokens return unauthorized before tool listing or dispatch. +- The correct bearer can initialize, list tools, and call a normal tool. +- A client-tool call remains open without headers or a result until delivery. +- Delivery writes one JSON-RPC response with the original MCP request id and browser output. +- A second delivery does not write another response. +- Client disconnect invokes cancellation and removes the operation. +- Environment cancellation closes every active response. +- Server close drains held responses within the shutdown bound. +- A client-tool batch is rejected before any batch item runs. +- Oversized request and output bodies fail with bounded errors and release resources. + +The likely focused command is: + +```bash +cd services/runner +pnpm exec vitest run tests/unit/tool-mcp-http.test.ts tests/unit/client-tool-continuation.test.ts +``` + +The implementation PR must use the actual final test file names in its PR description. + +## Runner integration matrix + +| Scenario | Expected exact behavior | Required fallback | +| --- | --- | --- | +| Valid result on owning replica | Same MCP request and prompt continue once | None | +| Keepalive disabled | No live registration | Existing cold replay | +| Feature flag disabled | Current socket destroy | Existing cold replay | +| Local pool full | No held resource beyond cap | Existing cold replay | +| Wrong replica receives result | No access to owner handle | Existing cold replay | +| TTL expires before result | Handle closes and environment is destroyed | `session/load` or cold replay | +| Runner restarts | In-memory handle is gone | PR #5197 continuity, then cold store | +| MCP client disconnects | Cancel and evict | Existing cold replay | +| Mount credentials expire | Evict before delivery | Existing cold replay | +| Original prompt rejects | Cancel and evict | Cold only if nothing streamed | +| Duplicate result requests | One live claimant | Loser cannot redeliver | +| Second client tool arrives | Refuse second park | Cold path | +| Client-tool batch arrives | Reject before any item executes | Model can retry singly | + +## Live local Claude protocol + +1. Pin and record the runner image, Claude CLI, ACP adapter, and MCP SDK versions. +2. Start a local Claude run with session keepalive and one browser-fulfilled client tool. +3. Trigger the client tool and wait longer than 60 seconds before fulfilling it. +4. Confirm the same MCP request id and ACP tool-call id receive the result. +5. Confirm the original prompt continues and no new model-issued client-tool call appears. +6. Repeat beyond 300 seconds if the first hold succeeds. +7. Repeat with forced expiry, runner restart, and client disconnect. Confirm each uses cold + fallback and does not lose the browser output. +8. Repeat with the flag off. Confirm behavior matches the current baseline. + +Use the agent-workflows QA procedure for the live run and save a redacted transcript as regression +evidence. A stable successful run should become an agent replay regression test when the runner +response can be recorded without a live model. + +## Security checks + +- A sibling process in the same namespace cannot initialize, list, or call the internal MCP server + without the per-environment bearer. +- A token for environment A cannot call environment B's server. +- A result for project or session A cannot claim an operation from project or session B. +- Logs, traces, metrics, thrown errors, and snapshots do not contain the bearer, raw arguments, or + browser output. +- The result-size cap applies before serialization can cause unbounded memory growth. +- Unknown, late, and duplicate results fail closed and remain eligible for cold handling. + +## Resource checks + +- Park up to the configured session pool maximum. +- Record process file descriptors, registry size, held response count, RSS, and PSS. +- Submit one over-cap request and confirm it does not add a held response. +- Expire all operations and confirm descriptors and registry gauges return to baseline. +- Repeat with shutdown and client disconnect. +- Compare the socket delta separately from the live Claude process cost. Do not attribute the + existing harness memory footprint to the continuation registry. + +## Acceptance bar + +- Exact continuation never crosses project, session, environment, or tool-call identity. +- One live operation completes at most once. +- Every failure path reclaims its socket, registry entry, and session. +- Browser output remains available to the cold fallback until live delivery succeeds. +- The flag-off path matches current behavior. +- Daytona remains explicitly unsupported for exact continuation and fails or falls back as it does + before this project. + diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/research.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/research.md new file mode 100644 index 0000000000..08653c46eb --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/research.md @@ -0,0 +1,164 @@ +# Research + +## Repository snapshot + +Research was completed on 2026-07-10 with the GitButler workspace based on `big-agents` commit +`cb63991eaa7d757c98d7c02a54382403fbe348ff` and `behind: 0`. + +- PR #5185, Pi approval parking, is merged. Pi now parks both approval gate types on the ACP + permission plane. +- PR #5153, the broader session keepalive design, remains a draft design PR. Its client-tool + follow-up recommends holding the MCP socket open, subject to measuring Claude's client timeout. +- PR #5197 was reviewed at head `343d7146935a8eb3ed41a203cd9a3db6ee954eef`. It is open against + `big-agents` and adds harness continuity and sandbox lifecycle support. + +The session keepalive design files are currently supplied by PR #5153 rather than `big-agents`. +This workspace repeats the required context so this design PR remains self-contained. + +## Current runner findings + +### The internal MCP server aborts a client-tool response + +`services/runner/src/tools/tool-mcp-http.ts` starts a stateless Streamable HTTP server on +`127.0.0.1`. Its client-tool branch: + +- validates required arguments; +- creates a runner-local call id; +- calls the shared `ClientToolRelay`; +- returns `MCP_PAUSED` when browser input is needed; and +- destroys the response socket when the listener sees that sentinel. + +The server also supports JSON-RPC batches. One paused client tool currently aborts the whole batch, +including responses for unrelated calls in the same HTTP request. + +There is no focused integration test that starts this server, sends a real HTTP MCP request, holds +it, and observes teardown. Most coverage reaches the behavior through engine fakes. + +### The internal endpoint has no application authentication + +`services/runner/src/tools/mcp-bridge.ts` advertises the endpoint with an empty `headers` list. The +server relies on loopback reachability. The same endpoint can dispatch non-client Agenta tools via +`runResolvedTool`. + +Loopback limits network reachability, but it is not an authorization boundary between processes in +the same host or network namespace. Extending the server and socket lifetime increases the window +in which a sibling process could call it. A per-environment bearer token is therefore a prerequisite +for hold-open behavior, even though the missing authentication predates this project. + +The bearer is defense in depth, not host isolation. A process that can inspect another process's +memory or IPC may still recover it. Strong workload identity and process isolation belong to the +deployment boundary or a future gateway. + +### Exact ACP correlation already exists + +`services/runner/src/engines/sandbox_agent/client-tools.ts` uses a +`ToolCallCorrelationIndex` for Claude. It maps the internal MCP call to the real ACP tool-call id +before emitting the browser interaction. Pi already has an exact relay id. + +The pending-operation design should reuse that correlated ACP id. It should not use the internal +random relay id or the cold `approvedCallKey(name, args)` as the live identity. + +### Cold result storage is separate and consume-once + +`services/runner/src/responder.ts` builds a FIFO client-tool output store from inbound +`tool_result` blocks. The store is keyed by tool name plus canonical arguments because a cold model +reissue has a new id. + +That store remains the fallback. The live path needs a new exact extractor keyed by the original +ACP tool-call id. It must read, not consume, the inbound result until the held MCP response has been +written or the request has fallen back to cold. + +### The keepalive pool needs a distinct state + +`services/runner/src/engines/sandbox_agent/session-pool.ts` currently has `busy`, `idle`, +`awaiting_approval`, and `destroyed`. `server.ts` checks out `awaiting_approval` only when it finds +an approval envelope for the parked ACP tool-call id. + +A client-tool result is not an approval decision and does not call `respondPermission`. Reusing +`awaiting_approval` would mix different resume verbs and validation rules. The pool needs +`awaiting_client_tool`, with a dedicated checkout method and expiry label. + +### The harness prompt can remain pending + +`runTurn` already retains the original prompt promise for a parked ACP approval. A pause controller +returns the turn to the HTTP caller while the prompt stays unresolved. The next `/run` installs a +new streaming and tracing sink, answers the held gate, then awaits the original prompt. + +Client-tool continuation can reuse that pattern. The difference is the resume verb: write the +browser output to the held MCP response instead of calling `respondPermission`. + +The pause callback currently aborts the environment's MCP controller for a non-approval pause. A +parkable client-tool record must exist before the pause fires so the callback can preserve the MCP +server and session only for that case. + +### The internal MCP channel is local only + +`services/runner/src/engines/sandbox_agent/mcp.ts` skips the internal MCP channel on Daytona. +`127.0.0.1` inside the sandbox is not the runner's loopback interface. Non-Pi Daytona runs with +Agenta tools are rejected before session creation because no delivery path exists. + +PR #5197 reconnects and resumes Daytona sandboxes, but it does not change this transport boundary. +Exact client-tool continuation on Daytona therefore requires a reachable authenticated endpoint, +such as a future MCP gateway. That work is outside this project. + +## Effect of PR 5197 + +PR #5197 provides useful lower-level lifecycle behavior: + +- per-harness session ids and a staleness guard; +- native `session/load` when the harness supports it; +- durable transcript mounts without persisting login credentials; +- reconnectable Daytona sandbox ids and hot, warm, cold, dead, and new lifecycle rungs; +- local replica ownership checks; +- continuity invalidation after paused, failed, or aborted turns; +- Compose passthrough for the Daytona lifecycle timers; +- a production-code pin that loaded sessions receive only the new user message; +- process-group cleanup; and +- a shared `shouldPark` policy that avoids parking failed or paused ordinary turns. + +This project should integrate with those rules after PR #5197 merges. In particular, a held +client-tool pause is a deliberate exception to the ordinary "paused turns do not park" rule. The +exception must be explicit and identity-bound. + +PR #5197 does not provide: + +- a held MCP response; +- a pending client-tool registry; +- exact browser-result correlation for a live MCP request; +- authentication for the internal loopback endpoint; +- routing to the replica that owns a pending operation; or +- durable result delivery and acknowledgement. + +Its `session_states.data` synchronization is a best-effort read, merge, and write path. This design +must not treat it as an atomic operation ledger. The live handle stays process-local. A future +gateway can supply durable operation storage behind the neutral interface. + +## Old risks and risks introduced by hold-open + +| Risk | Origin | Treatment in this plan | +| --- | --- | --- | +| Unauthenticated loopback MCP endpoint | Existing | Fix before hold-open in WP1. | +| Direct tool dispatch from the internal endpoint | Existing | Require the per-environment bearer before list or call. | +| Cold name-and-arguments matching | Existing | Retain only as fallback; use exact tool-call id when live. | +| Batch pause aborts all batch responses | Existing | Reject client-tool batches before any batch item executes. | +| No direct MCP HTTP lifecycle test | Existing | Add in WP1 and extend in WP3 and WP4. | +| Per-session memory cost and pool pressure | Existing keepalive | Reuse the pool cap and never exceed one pending client tool per session. | +| Multi-replica warm state is process-local | Existing keepalive | Wrong replica falls back cold; no forwarding in scope. | +| Claude MCP client timeout is unknown | Existing latent limit | Measure in WP0 before choosing a live TTL. | +| Duplicate or racing browser results | New | Atomic claim and terminal state machine in WP2 and WP4. | +| Long-lived sockets and file descriptors | New | Global cap, TTL, disconnect cleanup, shutdown drain, and load test. | +| Output lost between claim and delivery | New | Keep inbound request readable until delivery succeeds; otherwise evict and replay cold. | +| Raw Node response leaks into generic session state | New design risk | Hide it inside the MCP delivery adapter. | + +## Design consequences + +1. Authentication and real HTTP integration tests come before new hold behavior. +2. The live identity is project, session, and original ACP tool-call id. +3. The internal JSON-RPC request id stays as protocol context. It is required to write the correct + response but is not the browser correlation key. +4. The session pool stores a neutral operation descriptor and delivery port, not a + `ServerResponse`. +5. The first release rejects client-tool batches and multiple pending calls. +6. The exact path is an optimization with a strict cold fallback, not a new source of durable + truth. +7. Daytona and cross-replica exact continuation remain future gateway responsibilities. diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/status.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/status.md new file mode 100644 index 0000000000..67015aa7d4 --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/status.md @@ -0,0 +1,51 @@ +# Status + +Last updated: 2026-07-10 + +## Current phase + +Design review. No implementation code has been changed. + +Draft design PR: [#5201](https://github.com/Agenta-AI/agenta/pull/5201), labelled +`needs-review`. + +## Completed + +- Read the Codex onboarding, plan-feature, interface-design, GitButler, documentation, and PR + writing guidance. +- Reviewed the current internal MCP server, client-tool relay, responder, session pool, keepalive + dispatch, and MCP delivery boundary. +- Confirmed PR #5185 is merged and Pi approval parking is a separate completed path. +- Confirmed PR #5153 remains the draft home of the original hold-open recommendation. +- Refreshed PR #5197 at head `343d7146935a8eb3ed41a203cd9a3db6ee954eef` and incorporated its + continuity invalidation, sandbox lifecycle, native session load, and ownership work. +- Classified existing risks separately from risks introduced by holding sockets open. +- Defined a gateway-neutral pending-operation and delivery-port contract. +- Split the implementation into six progressive work packages with rollback gates. + +## Decisions proposed + +- Ship local Claude first. Keep Daytona exact continuation out of scope. +- Require the measured client timeout to exceed 60 seconds. +- Add loopback authentication before hold-open. +- Use a separate runner kill switch, default off. +- Support one pending client tool per session. +- Use exact ACP tool-call identity for live completion and retain name-and-arguments matching only + for cold fallback. +- Treat PR #5197 as the improved cold and lifecycle layer, not as a pending-operation store. + +## Dependencies + +| Dependency | State | Effect | +| --- | --- | --- | +| Session keepalive pool and approval parking | Implemented on `big-agents` | Supplies live session parking and resume structure. | +| Pi approval parking, PR #5185 | Merged | No client-tool work required for Pi. | +| Session keepalive design, PR #5153 | Draft | Contains the original client-tool hold-open recommendation and experiments. | +| Session continuity, PR #5197 | Open, next to merge | WP3 waits for merge because it changes the same lifecycle files. | +| Real MCP gateway | Out of scope | The interface preserves a future adapter boundary. | + +## Next step after approval + +Run WP0 and post the measured timeout report before authorizing hold-open implementation. WP1 can +then harden the current MCP endpoint independently. WP3 starts only after PR #5197 merges and the +implementation branch is rebased on the resulting `big-agents` head. From 85ae348cb95bcf97f30eae3b7632f6c7a4045d2d Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 11 Jul 2026 23:00:20 +0200 Subject: [PATCH 2/3] docs(design): fold cross-consistency review into the client-tool continuation plan State the transport-specific limits of cancel and onClosed in the delivery port, open the transport union, add the future in-sandbox stdio mapping (PR #5234) with its missing prerequisites, pin the WP1 bearer to the HTTP transport wrapper, declare that PR #5234 slice 1 lands before WP1 and WP3, and correct the WP5 Daytona wording. Claude-Session: https://claude.ai/code/session_01Fr4A5zjs5rsufRaWgWiUNC --- .../mcp-client-tool-continuation/interface.md | 43 ++++++++++++++++++- .../mcp-client-tool-continuation/plan.md | 16 ++++++- .../mcp-client-tool-continuation/research.md | 11 +++-- .../mcp-client-tool-continuation/status.md | 12 +++++- 4 files changed, 76 insertions(+), 6 deletions(-) diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/interface.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/interface.md index b8de996b73..6f963036e9 100644 --- a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/interface.md +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/interface.md @@ -72,7 +72,7 @@ authorization decision and is not the live browser-result key. | `projectId`, `sessionId` | Scope | Platform | Session | Prevents cross-project and cross-session completion. | | `runnerInstanceId` | Routing | Runner deployment | Process lifetime | Identifies the process that owns the live handle. | | `environmentId` | Routing | Session engine | Live environment lifetime | Prevents a stale operation from completing a replacement session. | -| `transport` | Protocol context | Delivery adapter | One operation | Selects the adapter without exposing its implementation. | +| `transport` | Protocol context | Delivery adapter | One operation | Selects the adapter without exposing its implementation. The union is open: it holds only `"mcp_streamable_http"` today and grows a value per new adapter (for example `"mcp_stdio_relay"` for the in-sandbox shim, see below). | | `requestId` | Protocol context | MCP client | One JSON-RPC request | Required in the JSON-RPC response. | | `name`, `inputDigest` | Data description | Tool call | One operation | Supports validation and safe diagnosis without raw input. | | `state`, timestamps | Lifecycle policy | Registry | One operation | Bounds ownership, retries, and cleanup. | @@ -94,6 +94,21 @@ interface ClientToolResultDelivery { The in-memory registry stores the interface, not the response object. A later gateway adapter can implement the same port with a gateway result endpoint or message queue. +Two of the port's semantics are transport-specific, and the kernel must not assume the HTTP +meanings: + +- `cancel(reason)` means "release the handle and stop expecting delivery". Only the HTTP + adapter can promise that the client saw an abort without a result (`res.destroy()`). A stdio + or relay-backed adapter has no per-request abort: its `cancel` settles the call with a + JSON-RPC error, or does nothing and lets the shim-side wait time out. The kernel treats a + cancelled operation as terminal either way; what the MCP client observed is an adapter + property. +- `onClosed` is best-effort per transport. The HTTP adapter observes the socket close. A + relay-backed adapter running inside a sandbox has no close signal the runner can observe, so + `onClosed` may never fire and cancellation degrades to TTL expiry and environment teardown. + Registry cleanup must never depend on `onClosed` alone; the TTL, teardown, and shutdown + paths in WP4 are the guaranteed exits. + ## Registry contract ```ts @@ -139,6 +154,10 @@ end-to-end exactly-once delivery across a process crash. The browser-side action completed before the result reaches the registry, and the cold path can return that stored output without repeating the browser action. +The "socket closes" row is transport-specific. It exists only where the adapter can observe a +close (the HTTP adapter). An adapter without a close signal (a relay-backed one) skips that row, +and its operations leave `pending` only through delivery, TTL expiry, teardown, or shutdown. + ## Exact resume input The first implementation can derive the live resume input from the existing `/run` messages: @@ -175,6 +194,28 @@ token only for the live environment, never places it in operation metadata, and harness receives it as connection credentials and may retain its own session configuration, so the token has no value after the environment closes its dedicated server. +## Future in-sandbox stdio mapping + +The in-sandbox tool MCP project +([../in-sandbox-tool-mcp/](../in-sandbox-tool-mcp/README.md), PR #5234) commits Daytona +delivery to a harness-spawned stdio shim that writes relay request files. If exact +continuation ever extends to that path, the kernel stays unchanged and a relay-backed adapter +implements the port as follows: + +| Port method | Relay-backed implementation | +| --- | --- | +| `deliver(output)` | Write a late `.res.json` that the shim's response wait picks up and answers on the original JSON-RPC id. | +| `cancel(reason)` | Write an error response, or write nothing and let the shim-side wait time out. No abort-without-result exists. | +| `onClosed` | Never fires. TTL and teardown are the only exits. | + +Three prerequisites do not exist yet and belong to a separate bridge design, not to this +project: the relay response protocol must gain a park shape (today a paused client tool writes +no response file and the shim's wait times out at 60 seconds), the shim's per-call wait must +learn to outlive that timeout for parked calls, and the held browser wait must survive or +refuse the five-minute Daytona auto-stop that kills the shim on park-to-stopped. The +recommendation to open one owned workspace for that bridge is recorded in +[../mcp-delivery-architecture/orchestration.md](../mcp-delivery-architecture/orchestration.md). + ## Future gateway mapping A real MCP gateway can implement the same roles with different ownership: diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/plan.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/plan.md index 83aba7aa84..f38d9f3a71 100644 --- a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/plan.md +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/plan.md @@ -69,6 +69,10 @@ Hold-open increases its lifetime, so authentication and direct transport tests c - Generate a random bearer token per session environment. - Advertise the token in the MCP server's standard `authorization` header. - Reject missing or wrong tokens before parsing or dispatching a tool call. +- Place the token validation in the HTTP transport wrapper in `tool-mcp-http.ts`, never in + the transport-neutral message handler that PR #5234 extracts as `tools/mcp-handler.ts`. + The in-sandbox stdio shim shares that handler and must stay credential-free; only the + listener-owning HTTP transport has anything to authenticate. - Keep the existing one-megabyte request-body cap and add an explicit result-size cap. - Reject a JSON-RPC batch containing a client tool before executing any item in that batch. - Keep non-client batches unchanged unless a test finds a protocol violation. @@ -208,7 +212,9 @@ Default-off happy-path code is not ready to enable until every long-lived-resour ### Stage 2: limited environment - Enable only where local Claude sessions and session keepalive are already enabled. -- Keep Daytona and other unsupported harnesses on the cold path. +- Keep Daytona and other unsupported harnesses on today's behavior: cold replay where a + delivery path exists, and the up-front refusal for Daytona client tools (which PR #5234 + narrows but keeps for client tools). - Watch pending count, completion path, wait duration, socket close, expiry, wrong-replica, pool eviction, and process file descriptors. @@ -222,6 +228,14 @@ disabled without a frontend or API deployment. - PR #5197 must merge before WP3 starts. Rebase and re-check `sandbox_agent.ts`, `server.ts`, `session-pool.ts`, continuity invalidation, and `shouldPark` before editing. +- PR #5234 ([../in-sandbox-tool-mcp/](../in-sandbox-tool-mcp/README.md)) refactors the same + files WP1 and WP3 edit: its slice 1 extracts the transport-neutral message handler from + `tool-mcp-http.ts` into `tools/mcp-handler.ts` (with an optional client-tool pause hook) + and the relay writer from `dispatch.ts` into `tools/relay-client.ts`. Land that slice + first. WP1's batch rejection then goes into the shared handler, WP1's bearer stays in the + HTTP transport wrapper (see WP1), and WP3's register-before-pause logic plugs into the + handler's pause hook. The combined landing order across the three tool projects is in + [../mcp-delivery-architecture/orchestration.md](../mcp-delivery-architecture/orchestration.md). - PR #5197's Daytona auto-stop default is five minutes, the same as the approval TTL. This project does not rely on that equality because the exact path is local only. - A wrong replica cannot access the live handle. It uses cold fallback. A future gateway can route diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/research.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/research.md index 08653c46eb..0db728dcef 100644 --- a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/research.md +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/research.md @@ -95,11 +95,16 @@ server and session only for that case. `services/runner/src/engines/sandbox_agent/mcp.ts` skips the internal MCP channel on Daytona. `127.0.0.1` inside the sandbox is not the runner's loopback interface. Non-Pi Daytona runs with -Agenta tools are rejected before session creation because no delivery path exists. +Agenta tools are rejected before session creation because no delivery path exists. PR #5234 +([../in-sandbox-tool-mcp/](../in-sandbox-tool-mcp/README.md)) narrows that refusal: after its +slice 2, gateway tools reach MCP-client harnesses on Daytona through an in-sandbox stdio shim +and the file relay, and the up-front refusal remains only for client tools and for non-Daytona +remote providers. PR #5197 reconnects and resumes Daytona sandboxes, but it does not change this transport boundary. -Exact client-tool continuation on Daytona therefore requires a reachable authenticated endpoint, -such as a future MCP gateway. That work is outside this project. +Exact client-tool continuation on Daytona therefore requires either a future MCP gateway or a +relay-backed delivery adapter built on PR #5234's shim (the unowned bridge described in +[interface.md](interface.md)). Both are outside this project. ## Effect of PR 5197 diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/status.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/status.md index 67015aa7d4..2e691919ba 100644 --- a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/status.md +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/status.md @@ -1,6 +1,6 @@ # Status -Last updated: 2026-07-10 +Last updated: 2026-07-11 ## Current phase @@ -22,6 +22,15 @@ Draft design PR: [#5201](https://github.com/Agenta-AI/agenta/pull/5201), labelle - Classified existing risks separately from risks introduced by holding sockets open. - Defined a gateway-neutral pending-operation and delivery-port contract. - Split the implementation into six progressive work packages with rollback gates. +- 2026-07-11: folded in the cross-consistency review round (requested by the owner alongside + his own review of the event-driven-tool-relay PR). interface.md now states the + transport-specific limits of `cancel` and `onClosed`, opens the `transport` union, and adds + the future in-sandbox stdio mapping (PR #5234) with its three missing prerequisites. plan.md + now places the WP1 bearer in the HTTP transport wrapper (never in the shared + `mcp-handler.ts` that PR #5234 extracts), declares that PR #5234 slice 1 lands before WP1 + and WP3, and corrects the WP5 Daytona wording (refusal for client tools, cold elsewhere). + research.md notes how PR #5234 narrows the Daytona refusal. Combined landing order: + `../mcp-delivery-architecture/orchestration.md`. ## Decisions proposed @@ -42,6 +51,7 @@ Draft design PR: [#5201](https://github.com/Agenta-AI/agenta/pull/5201), labelle | Pi approval parking, PR #5185 | Merged | No client-tool work required for Pi. | | Session keepalive design, PR #5153 | Draft | Contains the original client-tool hold-open recommendation and experiments. | | Session continuity, PR #5197 | Open, next to merge | WP3 waits for merge because it changes the same lifecycle files. | +| In-sandbox tool MCP, PR #5234 | Open, design review | Its slice 1 extracts `mcp-handler.ts` and `relay-client.ts` from the files WP1 and WP3 edit; land it before WP1 and WP3. | | Real MCP gateway | Out of scope | The interface preserves a future adapter boundary. | ## Next step after approval From 3fdac7c86a9b86b3c5e48b20be9689e9f561fb24 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 11 Jul 2026 23:19:19 +0200 Subject: [PATCH 3/3] docs(design): adopt the Codex review - defer the warm path behind measurement gates Fold the Codex xhigh review into the client-tool continuation plan. Rescope v1 to WP0 (expanded with cold-path baseline metrics: first-reissue match rate, argument drift, added model calls/latency/cost, wrong-replica rate, wait percentiles) plus WP1 loopback hardening (auth from headers before body parsing, timing-safe comparison, per-environment token and rotation test, result-size cap separated from auth). Defer WP2 through WP5 behind two concrete unlock gates: Claude holds the request past the 60-second idle TTL, and cold replay demonstrably harms users. Slim the delivery port to deliver -> accepted|unavailable plus dispose; cut cancel and onClosed (they modeled an HTTP response handle); correctness rests on lease expiry and teardown. Replace the standalone registry with pool-owned placement (ParkedClientTool beside ParkedApproval, awaiting_client_tool in session-pool.ts, McpHttpResultDelivery under tools/). Rename harnessToolCallId to toolCallId with its provenance as a registration invariant. Restrict any future warm mode to owner-routed deployments and define the delivery commit point. Drop the dependency on #5234's handler extraction (cut from that project's v1). The deferral is the top review question for the owner; it reverses part of a plan he LGTM'd and is flagged as such in status.md and open-questions.md. Claude-Session: https://claude.ai/code/session_0127AM79khCdvD2b8BG2joZL --- .../mcp-client-tool-continuation/README.md | 47 +-- .../mcp-client-tool-continuation/context.md | 7 +- .../mcp-client-tool-continuation/interface.md | 222 ++++++----- .../open-questions.md | 79 ++-- .../mcp-client-tool-continuation/plan.md | 350 +++++++----------- .../mcp-client-tool-continuation/qa.md | 6 + .../mcp-client-tool-continuation/research.md | 8 +- .../mcp-client-tool-continuation/status.md | 74 ++-- 8 files changed, 376 insertions(+), 417 deletions(-) diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/README.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/README.md index 6613ebfc61..5ec0e6cbce 100644 --- a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/README.md +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/README.md @@ -4,21 +4,21 @@ Claude currently ends an Agenta client-tool turn by closing the internal MCP req result. When the browser returns the result, the runner starts or loads a session and Claude asks for the tool again. That path is safe, but it cannot preserve the original call id or arguments. -This project adds a faster exact path for local Claude sessions. The runner keeps the original MCP -request and harness prompt open, parks the session, and writes the browser result to that same -JSON-RPC request. Cold replay remains the fallback for expiry, restart, disconnect, pool pressure, -wrong-replica routing, and unsupported environments. - -The first implementation does not build an MCP gateway. It defines a transport-neutral -continuation interface so a future gateway can own authentication, routing, and durable pending -operations without changing the session state machine. +This project studied a faster exact path for local Claude sessions: keep the original MCP +request and harness prompt open, park the session, and write the browser result to that same +JSON-RPC request. After a Codex review on 2026-07-11, that warm path is **deferred behind two +measurement gates**: Claude must demonstrably hold the request open for a useful interval, and +cold replay must demonstrably harm users. What ships now is the measurement (WP0) and the +independent hardening of the loopback MCP endpoint (WP1). The design for the warm path stays +in this workspace as a revised note so the build can start quickly if the gates pass. ## Status -Design only. Implementation has not started. +Design only. Implementation has not started. The warm-path deferral is the top review +question for the owner; see [status.md](status.md) and [open-questions.md](open-questions.md). -The proposed rollout is local Claude only. Pi approval parking is already implemented through the -ACP permission plane. Non-Pi Daytona runs cannot receive Agenta tools through the current internal +The scope is local Claude only. Pi approval parking is already implemented through the ACP +permission plane. Non-Pi Daytona runs cannot receive Agenta tools through the current internal MCP endpoint because the endpoint binds to the runner's loopback interface. ## Documents @@ -26,19 +26,20 @@ MCP endpoint because the endpoint binds to the runner's loopback interface. - [context.md](context.md) explains the current behavior, goals, scope, and user-visible result. - [research.md](research.md) records the code findings and the effect of PRs #5153, #5185, and #5197. -- [interface.md](interface.md) defines the gateway-neutral pending-operation contract and state - machine. -- [plan.md](plan.md) splits the work into progressive work packages and rollout gates. -- [qa.md](qa.md) defines unit, integration, live, security, and resource verification. -- [open-questions.md](open-questions.md) lists the decisions that still need review. +- [interface.md](interface.md) is the design note for the deferred warm path: the + pending-operation contract, the slimmed delivery port, and the pool-owned placement. +- [plan.md](plan.md) defines what ships now (WP0 expanded, WP1), the unlock gates, and the + revised shape of the deferred warm path. +- [qa.md](qa.md) defines the verification layers; the layers beyond WP0 and WP1 apply only if + the warm path unlocks. +- [open-questions.md](open-questions.md) lists the decisions that still need review, led by + the deferral itself. - [status.md](status.md) is the live source of truth for progress and dependencies. ## Proposed sequence -1. Measure Claude's MCP request timeout. -2. Authenticate and test the existing internal loopback MCP endpoint. -3. Add the transport-neutral continuation registry with no behavior change. -4. Add the local Claude hold-open and resume path behind a runner kill switch. -5. Close race, timeout, shutdown, and resource-limit cases before enabling it. -6. Canary locally and keep Daytona and cross-replica exact routing on the cold fallback. - +1. Measure Claude's MCP request timeout and the cold path's real user cost (WP0). +2. Authenticate and test the existing internal loopback MCP endpoint (WP1), independently. +3. Score the measurements against the unlock gates in [plan.md](plan.md). +4. Only if both gates pass, and only in owner-routed deployments: build the smallest + pool-owned hold-open path per the revised design note, after PR #5197 merges. diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/context.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/context.md index 8435b61b2c..97089592fd 100644 --- a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/context.md +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/context.md @@ -33,7 +33,8 @@ new call has a new id, and the model has made another decision. ## Desired warm flow -The exact path changes steps 3 through 6: +This flow is the deferred goal, not the first deliverable; [plan.md](plan.md) gates it on +two measurements. The exact path changes steps 3 through 6: 1. The runner leaves the original JSON-RPC response open. 2. The session pool parks the live harness in `awaiting_client_tool` with the original prompt @@ -101,9 +102,9 @@ a pending-operation ledger. continuation is unavailable. 7. The default remains off until timeout, security, race, and resource tests pass. -## Scope of the first release +## Scope of the first warm release, if it unlocks -The first release requires all of the following: +A warm release requires all of the following: - Claude or another non-Pi harness using the internal MCP channel. - A local sandbox, because the internal MCP endpoint is runner-loopback only. diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/interface.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/interface.md index 6f963036e9..39e680e361 100644 --- a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/interface.md +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/interface.md @@ -1,21 +1,27 @@ # Gateway-neutral continuation interface +Status: design note for the deferred warm path (see [plan.md](plan.md)). Nothing here is +scheduled for implementation until the unlock gates pass. Revised 2026-07-11 after the +Codex review: the delivery port shrank to delivery plus disposal, transport liveness became +an optional adapter-owned signal, the standalone registry was replaced by pool-owned +placement, and `harnessToolCallId` was renamed. + ## Design rule -The contract describes a pending client-tool operation. It does not describe Node HTTP objects or -the runner's current pool implementation. +The contract describes a pending client-tool operation. It does not describe Node HTTP +objects or the runner's current pool implementation. Each field is grouped by semantic role: - `identity` identifies the operation and browser interaction. - `scope` identifies the tenant boundary that may complete it. - `routing` says where the live owner can be reached. -- `protocol` carries MCP-specific request context. - `tool` describes the call without storing raw input. - `lifecycle` controls expiry and terminal state. -Credentials stay under the MCP connection that they authenticate. They do not appear in operation -metadata. +Credentials stay under the MCP connection that they authenticate. They do not appear in +operation metadata. Adapter-specific protocol context (the JSON-RPC `requestId`, the +transport kind) lives with the adapter that needs it, not in the neutral operation. ## Operation shape @@ -33,7 +39,7 @@ interface PendingClientToolOperation { identity: { operationId: string; interactionId: string; - harnessToolCallId: string; + toolCallId: string; }; scope: { projectId: string; @@ -43,10 +49,6 @@ interface PendingClientToolOperation { runnerInstanceId: string; environmentId: string; }; - protocol: { - transport: "mcp_streamable_http"; - requestId: string | number; - }; tool: { name: string; inputDigest: string; @@ -59,126 +61,115 @@ interface PendingClientToolOperation { } ``` -`inputDigest` detects internal mismatches without retaining or logging raw arguments. It is not an -authorization decision and is not the live browser-result key. +**Provenance of `toolCallId`, stated as an invariant:** it is the harness-correlated ACP +tool-call id, the exact key a browser result carries back. Current correlation is +best-effort and falls back to the MCP-generated random id when matching fails; that +fallback id must never enter this field. Warm registration is allowed only when the +correlation index returned a real harness tool-call id; otherwise the runner destroys the +MCP response and uses cold replay. The registration gate is what proves the provenance; the +earlier name `harnessToolCallId` asserted more than the code guaranteed. -## Field classification - -| Field | Role | Owner | Lifetime | Reason | -| --- | --- | --- | --- | --- | -| `operationId` | Identity | Continuation registry | One operation | Stable internal idempotency key. | -| `interactionId` | Protocol context | Interaction producer | One operation | Connects to the durable interaction row and UI event. | -| `harnessToolCallId` | Protocol context | Harness correlation index | One harness call | Exact browser-result key for the live path. | -| `projectId`, `sessionId` | Scope | Platform | Session | Prevents cross-project and cross-session completion. | -| `runnerInstanceId` | Routing | Runner deployment | Process lifetime | Identifies the process that owns the live handle. | -| `environmentId` | Routing | Session engine | Live environment lifetime | Prevents a stale operation from completing a replacement session. | -| `transport` | Protocol context | Delivery adapter | One operation | Selects the adapter without exposing its implementation. The union is open: it holds only `"mcp_streamable_http"` today and grows a value per new adapter (for example `"mcp_stdio_relay"` for the in-sandbox shim, see below). | -| `requestId` | Protocol context | MCP client | One JSON-RPC request | Required in the JSON-RPC response. | -| `name`, `inputDigest` | Data description | Tool call | One operation | Supports validation and safe diagnosis without raw input. | -| `state`, timestamps | Lifecycle policy | Registry | One operation | Bounds ownership, retries, and cleanup. | +`inputDigest` detects internal mismatches without retaining or logging raw arguments. It is +not an authorization decision and is not the live browser-result key. ## Delivery port -The HTTP implementation can close over a Node `ServerResponse`, but only the adapter can see it: +The port carries exactly two capabilities: deliver a result, release adapter-owned +resources. The HTTP implementation can close over a Node `ServerResponse`, but only the +adapter can see it: ```ts -type DeliveryResult = "written" | "transport_closed"; - interface ClientToolResultDelivery { - deliver(output: unknown): Promise; - cancel(reason: string): Promise; - onClosed(listener: () => void): () => void; -} -``` - -The in-memory registry stores the interface, not the response object. A later gateway adapter can -implement the same port with a gateway result endpoint or message queue. - -Two of the port's semantics are transport-specific, and the kernel must not assume the HTTP -meanings: + deliver(output: unknown): Promise< + | { kind: "accepted" } + | { kind: "unavailable"; reason: string } + >; -- `cancel(reason)` means "release the handle and stop expecting delivery". Only the HTTP - adapter can promise that the client saw an abort without a result (`res.destroy()`). A stdio - or relay-backed adapter has no per-request abort: its `cancel` settles the call with a - JSON-RPC error, or does nothing and lets the shim-side wait time out. The kernel treats a - cancelled operation as terminal either way; what the MCP client observed is an adapter - property. -- `onClosed` is best-effort per transport. The HTTP adapter observes the socket close. A - relay-backed adapter running inside a sandbox has no close signal the runner can observe, so - `onClosed` may never fire and cancellation degrades to TTL expiry and environment teardown. - Registry cleanup must never depend on `onClosed` alone; the TTL, teardown, and shutdown - paths in WP4 are the guaranteed exits. - -## Registry contract - -```ts -interface ClientToolContinuationRegistry { - register( - operation: PendingClientToolOperation, - delivery: ClientToolResultDelivery, - ): "registered" | "duplicate" | "capacity_exceeded"; - - claim(input: { - projectId: string; - sessionId: string; - harnessToolCallId: string; - environmentId: string; - }): - | { kind: "claimed"; operation: PendingClientToolOperation; delivery: ClientToolResultDelivery } - | { kind: "missing" | "duplicate" | "expired" | "wrong_owner" }; - - markDelivered(operationId: string): void; - cancel(operationId: string, reason: string): Promise; - cancelEnvironment(environmentId: string, reason: string): Promise; - size(): number; + dispose(reason: string): Promise; } ``` -`claim` is atomic. It changes `pending` to `delivering`. Only the claimant receives the delivery -port. A second caller observes `duplicate` or a terminal state and cannot write a second response. +- `deliver` attempts to hand the browser output to the waiting client. `accepted` means the + transport took the response; `unavailable` means it cannot (closed socket, dead handle) + and the caller falls back cold with the preserved result. +- `dispose` means only "release adapter-owned resources". It makes no claim about what the + remote MCP client observed. For a future relay-backed adapter, whether abandoning a call + writes a JSON-RPC error or writes nothing is a protocol decision that adapter documents; + it is not hidden inside a generic `cancel`. + +The earlier port also required `cancel(reason)` and `onClosed(listener)`. Both modeled an +HTTP response handle and were cut: a relay-backed adapter has no per-request abort and no +close signal, and a port is not neutral because its awkward methods "may do nothing". + +**Transport liveness is optional adapter input, not part of the port.** The HTTP adapter +may own a closed signal (the socket close event) and use it to trigger early cleanup. +Correctness must never depend on it: lease expiry and environment teardown are the +guaranteed exits for every adapter. + +## Placement instead of a registry + +A standalone `ClientToolContinuationRegistry` is premature. The session pool already owns +capacity, TTL, atomic checkout, and environment teardown, and the first release would allow +exactly one pending client tool per environment. If the warm path is built: + +- `ParkedClientTool` sits beside `ParkedApproval` in the sandbox-agent session model, + holding the neutral operation, the delivery port, and the original prompt promise. +- `awaiting_client_tool` and its dedicated checkout live in `session-pool.ts`. Checkout is + the atomic claim: a losing request never sees the delivery port. +- The claim behaves as a lease: a checkout that fails to deliver must finalize (deliver, + dispose, or re-park) within a bounded time, finalization is idempotent, and expiry + applies to a claimed operation the same as to a pending one. +- The exact current-turn result extraction sits beside the existing responder extractors. +- `McpHttpResultDelivery` sits under `tools/` as transport code. + +A durable pending-operation registry appears only at a future gateway boundary, where a +second implementation exists to constrain the abstraction. ## State machine | From | Event | To | Required action | | --- | --- | --- | --- | -| None | Valid single client-tool call | `pending` | Register before emitting pause. | +| None | Valid single client-tool call with proven correlation | `pending` | Register before emitting the pause; on failure, commit the cold-pause form. | | `pending` | Exact browser result arrives | `delivering` | One atomic claimant receives the port. | -| `delivering` | MCP response write succeeds | `delivered` | Continue the original prompt and resolve the interaction. | -| `pending` or `delivering` | Socket closes | `cancelled` | Evict the live session and keep cold fallback available. | -| `pending` or `delivering` | Session teardown or shutdown | `cancelled` | Close the response and destroy the environment. | -| `pending` | TTL expires | `expired` | Close the response and destroy the environment. | +| `delivering` | `deliver` returns `accepted` | `delivered` | Continue the original prompt and resolve the interaction. | +| `delivering` | `deliver` returns `unavailable`, or the claimant fails to finalize within the lease | `cancelled` | Dispose, evict the live session, fall back cold with the preserved result. | +| `pending` or `delivering` | Session teardown or shutdown | `cancelled` | Dispose and destroy the environment. | +| `pending` | TTL expires | `expired` | Dispose and destroy the environment. | +| `pending` | Adapter-owned closed signal fires (optional) | `cancelled` | Early cleanup only; expiry and teardown remain the guaranteed exits. | | Any terminal state | Late or duplicate result | Unchanged | Do not deliver again; use or retain cold fallback. | -The local implementation provides at-most-once completion of a live handle. It cannot provide -end-to-end exactly-once delivery across a process crash. The browser-side action has already -completed before the result reaches the registry, and the cold path can return that stored output -without repeating the browser action. +Terminal entries are retained for the length of the session turn so a late duplicate reads +as `duplicate` rather than `missing`; after that, cleanup may drop them. -The "socket closes" row is transport-specific. It exists only where the adapter can observe a -close (the HTTP adapter). An adapter without a close signal (a relay-backed one) skips that row, -and its operations leave `pending` only through delivery, TTL expiry, teardown, or shutdown. +The local implementation provides at-most-once completion of a live handle. It cannot +provide end-to-end exactly-once delivery across a process crash. The browser-side action +has already completed before the result reaches the runner, and the cold path can return +that stored output without repeating the browser action. ## Exact resume input -The first implementation can derive the live resume input from the existing `/run` messages: +The first implementation can derive the live resume input from the existing `/run` +messages: ```ts interface ClientToolResumeInput { projectId: string; sessionId: string; - harnessToolCallId: string; + toolCallId: string; output: unknown; } ``` -The extractor accepts exactly one current-turn `tool_result` for the parked -`harnessToolCallId`. Missing or duplicate results do not claim the operation. The existing cold -store still indexes the same result by name and arguments if live delivery cannot proceed. +The extractor accepts exactly one current-turn `tool_result` for the parked `toolCallId`. +Missing or duplicate results do not claim the operation. The existing cold store still +indexes the same result by name and arguments if live delivery cannot proceed. ## Loopback authentication -Each internal MCP server receives a random per-environment bearer token. The ACP MCP connection -places it under the standard `authorization` header: +This section backs WP1, which ships now regardless of the warm path. + +Each internal MCP server receives a random per-environment bearer token. The ACP MCP +connection places it under the standard `authorization` header: ```ts { @@ -189,31 +180,34 @@ places it under the standard `authorization` header: } ``` -The server validates the token before `initialize`, `tools/list`, or `tools/call`. Agenta keeps the -token only for the live environment, never places it in operation metadata, and never logs it. The -harness receives it as connection credentials and may retain its own session configuration, so the -token has no value after the environment closes its dedicated server. +The server validates the token, from the request headers and with a timing-safe +comparison, before `initialize`, `tools/list`, or `tools/call`. Agenta keeps the token only +for the live environment, never places it in operation metadata, and never logs it. The +harness receives it as connection credentials and may retain its own session +configuration, so the token has no value after the environment closes its dedicated +server. ## Future in-sandbox stdio mapping The in-sandbox tool MCP project ([../in-sandbox-tool-mcp/](../in-sandbox-tool-mcp/README.md), PR #5234) commits Daytona delivery to a harness-spawned stdio shim that writes relay request files. If exact -continuation ever extends to that path, the kernel stays unchanged and a relay-backed adapter -implements the port as follows: +continuation ever extends to that path, a relay-backed adapter implements the port as +follows: | Port method | Relay-backed implementation | | --- | --- | -| `deliver(output)` | Write a late `.res.json` that the shim's response wait picks up and answers on the original JSON-RPC id. | -| `cancel(reason)` | Write an error response, or write nothing and let the shim-side wait time out. No abort-without-result exists. | -| `onClosed` | Never fires. TTL and teardown are the only exits. | +| `deliver(output)` | Write a late `.res.json` that the shim's response wait picks up and answers on the original JSON-RPC id. `unavailable` when the relay dir or shim is gone. | +| `dispose(reason)` | Adapter decision, documented there: write an error response, or write nothing and let the shim-side wait time out. | +| Closed signal | None. TTL and teardown are the only exits, which the slimmed port already assumes. | Three prerequisites do not exist yet and belong to a separate bridge design, not to this -project: the relay response protocol must gain a park shape (today a paused client tool writes -no response file and the shim's wait times out at 60 seconds), the shim's per-call wait must -learn to outlive that timeout for parked calls, and the held browser wait must survive or -refuse the five-minute Daytona auto-stop that kills the shim on park-to-stopped. The -recommendation to open one owned workspace for that bridge is recorded in +project: the relay response protocol must gain a park shape (today a paused client tool +writes no response file and the shim's wait times out at 60 seconds), the shim's per-call +wait must learn to outlive that timeout for parked calls, and the held browser wait must +survive or refuse the five-minute Daytona auto-stop that kills the shim on +park-to-stopped. The recommendation to open one owned workspace for that bridge is recorded +in [../mcp-delivery-architecture/orchestration.md](../mcp-delivery-architecture/orchestration.md). ## Future gateway mapping @@ -222,13 +216,13 @@ A real MCP gateway can implement the same roles with different ownership: | Local implementation | Future gateway implementation | | --- | --- | -| In-memory registry | Durable pending-operation store with compare-and-set claim. | +| Pool-owned `ParkedClientTool` | Durable pending-operation store with compare-and-set claim. | | `runnerInstanceId` lookup | Gateway route to the owning runner or session worker. | | Per-environment loopback bearer | Authenticated harness-to-gateway credential. | | `McpHttpResultDelivery` | Gateway response stream or result-delivery endpoint. | -| Runner TTL timer | Gateway lease and retention policy. | +| Pool TTL timer | Gateway lease and retention policy. | | Process-local metrics | Fleet-wide audit and quota metrics. | -The gateway will still need authenticated actor context, encrypted result retention, revocation, -and acknowledgement semantics. Those concerns are not hidden inside `metadata` and are not -pretended to exist in the local implementation. +The gateway will still need authenticated actor context, encrypted result retention, +revocation, and acknowledgement semantics. Those concerns are not hidden inside `metadata` +and are not pretended to exist in the local implementation. diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/open-questions.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/open-questions.md index d5a1e91c31..4328df38cd 100644 --- a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/open-questions.md +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/open-questions.md @@ -1,50 +1,53 @@ # Open questions -## 1. What timeout is enough to ship? +## 1. TOP review question: accept the warm-path deferral? -The exact value must come from WP0. +This is the headline change since the owner's review. He LGTM'd the fuller plan (WP0 +through WP5, with the hold-open implementation gated on the timeout measurement). A Codex +xhigh review on 2026-07-11 recommended not building the warm path yet: the plan measured +transport feasibility but not user-visible value, and the cold path is working code with +bounded failure behavior. The deferral was adopted while the owner slept, under his +standing simplify-aggressively instruction, and it is reversible by restoring the prior +plan revision from git history. -- Option A: require the held MCP request to exceed the five-minute approval TTL. -- Option B: require it to exceed the 60-second idle TTL, cap the live wait below the measured - ceiling, and use cold fallback for longer waits. -- Option C: ship any measurable hold, even below 60 seconds. +- Option A: keep the deferral. v1 = WP0 expanded (timeout plus cold-path baseline) and WP1 + (loopback hardening). WP2 through WP5 unlock only if the two gates in + [plan.md](plan.md) pass. +- Option B: restore the previous scope and authorize the hold-open build after WP0's + timeout measurement alone, as the pre-review plan had it. -Recommendation: Option B. It provides a useful fast path without claiming that the runner controls -Claude's client timeout. If the request does not survive 60 seconds, stop the implementation and -keep the current cold path. +Recommendation: Option A. If cold drift is rare, a second continuation mechanism is +complexity without users to serve. -## 2. Should loopback authentication be part of this project? +## 2. Are the proposed value-gate thresholds right? -- Option A: add a per-environment bearer in WP1 before hold-open. -- Option B: leave loopback unauthenticated because the risk predates this project. +The plan proposes: build the warm path only if the first cold reissue mismatches in more +than 5 percent of client-tool turns, or argument drift repeats a browser interaction in +more than 2 percent, or the cold continuation adds user-visible latency or reported cost. +These numbers are proposals to make the gate concrete, not measurements. -Recommendation: Option A. The endpoint can execute Agenta tools, and hold-open extends its useful -lifetime. Authentication is a prerequisite, not unrelated cleanup. +Recommendation: confirm or adjust the thresholds when reviewing question 1. -## 3. Should the first release add cross-replica routing? +## 3. Should the first warm release add cross-replica routing? - Option A: route a browser result to the runner that owns the live operation. - Option B: keep ownership process-local and use cold fallback on the wrong replica. - -Recommendation: Option B. Cross-replica routing belongs to the future gateway or a broader session -routing design. The neutral contract records the owner so that work can be added without changing -the operation model. - -## 4. Should the first release support multiple pending client tools? - -- Option A: allow multiple operations per session and support client-tool JSON-RPC batches. -- Option B: support one pending operation and reject client-tool batches before execution. - -Recommendation: Option B. The current pause latch and session resume path are single-gate. Multiple -pending calls require a separate interaction and ordering design. - -## 5. When is a live result considered delivered? - -- Option A: when the runner writes and flushes the JSON-RPC response. -- Option B: only after the runner also observes the matching harness tool-call completion update. - -Recommendation: implement Option A as the transport terminal state and record the harness update -as a separate continuation acknowledgement metric. If the harness does not continue, destroy the -session and leave the browser result available to cold fallback. A future gateway can add durable -acknowledgement without changing the delivery port. - +- Option C (adopted in the revised plan): stricter than B, enable warm mode only in + owner-routed deployments (single replica, verified affinity, or an owner-routing token). + Elsewhere warm continuation is unsupported and the runner goes cold immediately. + +Recommendation: Option C. A wrong-replica cold start can race a live owner that still holds +the original prompt and parked session; a lower hit rate understates that concurrency risk. + +## Settled + +- **Loopback authentication is part of this project** and ships now as WP1, independent of + the warm-path decision. The endpoint dispatches Agenta tools; authentication is a + prerequisite, not unrelated cleanup. +- **Timeout bar** (old question 1): folded into the transport gate. The held request must + survive the 60-second idle TTL or the warm path is cut, not deferred. +- **Multiple pending client tools and client-tool batches** (old question 4): deferred with + the warm path; the batch rejection itself ships in WP1 as protocol hardening. +- **Delivery commit point** (old question 5): defined in the plan. Delivery `accepted` is + the commit point: before it, the inbound result stays readable for cold fallback; after + it, no cold continuation may start even if the original prompt later fails. diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/plan.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/plan.md index f38d9f3a71..216f400b6c 100644 --- a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/plan.md +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/plan.md @@ -1,80 +1,111 @@ # Plan -## Decision summary - -Build an exact local continuation path as a cache above the existing cold fallback. - -- Use the original ACP tool-call id for live browser-result correlation. -- Add a dedicated `awaiting_client_tool` pool state and resume verb. -- Keep raw HTTP state inside an MCP delivery adapter. -- Authenticate the current loopback endpoint before holding requests open. -- Support one pending client tool per local Claude session and reject client-tool batches. -- Add a runner-only kill switch. Do not add a frontend flag. -- Start the lifecycle-changing work only after PR #5197 merges and the branch rebases. -- Leave Daytona exact delivery, cross-replica routing, and a real MCP gateway out of scope. - -## Dependency order +Revised 2026-07-11 after a Codex xhigh review. The verdict: do not build the warm hold-open +path yet. The plan measured transport feasibility but not user-visible value; it proposed a +registry, a new pool state, a new resume verb, long-lived sockets, and extensive race +handling without establishing how often cold replay actually harms users. This revision +adopts that rescope. The owner LGTM'd the fuller plan before the review; see +[status.md](status.md) and [open-questions.md](open-questions.md) for the deferral +provenance and the review question it raises. -```text -WP0 timeout measurement - -> WP1 loopback hardening - -> WP2 neutral continuation kernel - -> WP3 local hold-open path - -> WP4 failure and resource envelope - -> WP5 canary and rollout -``` - -WP0 through WP2 can be prepared independently. WP3 edits the same environment, park, teardown, -and continuity code as PR #5197, so it starts after that PR lands. +## Decision summary -## WP0: measure the transport ceiling +- **Ship now: WP0 (expanded) and WP1.** Measure both the transport ceiling and the cold + path's real user cost, and harden the loopback endpoint. Neither changes client-tool + behavior. +- **Defer: WP2 through WP5** (the continuation kernel, the hold-open path, the failure + envelope, the canary). They unlock only if two measurements pass; the gates are below. +- Keep [interface.md](interface.md) as a revised design note for the deferred path, not an + implementation contract. +- Keep the cold path as the one continuation mechanism until the gates pass. It is working + code with bounded failure behavior; its weakness (name-and-arguments matching instead of + exact identity) must be shown to be materially visible before a second mechanism exists. + +## Unlock gates for the warm path + +Both must pass; either failing keeps the deferral: + +1. **Transport gate.** WP0 shows Claude keeps the original MCP request usable beyond the + 60-second idle TTL, with a measured ceiling and a safety margin. If the request does not + survive 60 seconds, cut the warm path permanently, not just defer it. +2. **Value gate.** The cold-path baseline shows material user harm. Proposed thresholds, + for the owner to confirm or adjust: the first cold reissue fails to match the stored + browser result in more than 5 percent of client-tool turns, or argument drift causes a + repeated browser interaction in more than 2 percent, or the cold continuation adds + user-visible latency (seconds at p50, not milliseconds) or model cost that support or + users actually report. If cold drift is rare, defer even when the timeout gate passes. + +A third condition applies at build time, not as a measurement: the warm mode may only be +enabled in owner-routed deployments (a single runner replica, verified session affinity +covering the browser resume, or an owner-routing token that actually reaches the owner). +Without owner routing, warm continuation stays unsupported and the runner chooses cold +immediately; the concurrency risk of a wrong-replica cold start racing a live owner is +worse than a lower hit rate. See "Wrong-replica commit point" below. + +## WP0: measure the transport ceiling and the cold-path baseline ### Purpose -The runner controls the server side of the internal MCP request. Claude controls the client-side -timeout. No design can promise a five-minute park until the real client is measured. +The runner controls the server side of the internal MCP request; Claude controls the +client-side timeout. And no design can justify a second continuation mechanism until the +first one's failure rate is measured. WP0 now answers both. -### Work +### Work: transport ceiling (unchanged) -- Add a repeatable local Claude experiment that exposes one client tool and intentionally delays - its result. +- Add a repeatable local Claude experiment that exposes one client tool and intentionally + delays its result. - Record the pinned Claude harness, ACP adapter, MCP SDK, Node, and runner versions. -- Test a hold longer than the 60-second idle TTL first. -- If that succeeds, test beyond the 300-second approval TTL. -- Record whether Claude keeps the same MCP request id and ACP tool-call id, closes the connection, - retries, or settles the call with an error. -- Measure one quiet pending socket's file-descriptor and memory delta separately from the already - measured live Claude process tree. +- Test a hold longer than the 60-second idle TTL first; if that succeeds, test beyond the + 300-second approval TTL. +- Record whether Claude keeps the same MCP request id and ACP tool-call id, closes the + connection, retries, or settles the call with an error. +- Measure one quiet pending socket's file-descriptor and memory delta separately from the + already measured live Claude process tree. -### Exit gate +### Work: cold-path baseline (new) + +From production traces where available, otherwise from a realistic QA batch of client-tool +turns: -The first release proceeds only if the request remains usable beyond the 60-second idle TTL. If it -does not, keep the current cold path and stop the hold-open implementation. If it covers 60 seconds -but not five minutes, cap the live wait below the measured limit with a safety margin and fall back -cold for longer waits. +- The percentage of browser results matched successfully on the first cold reissue. +- The percentage where Claude changes arguments and causes another browser interaction. +- The added model calls, latency, and token cost of the cold continuation turn. +- The percentage of resumptions that reach a different runner replica than the one that + parked. +- Typical and percentile user wait times between the browser result and the continued + answer. -### Deliverable +### Exit -A checked-in experiment protocol and report. No production behavior change. +A checked-in experiment protocol and report covering both measurements, scored against the +unlock gates. No production behavior change. ## WP1: harden the existing internal MCP endpoint ### Purpose -The endpoint already dispatches Agenta tools and currently relies only on loopback reachability. -Hold-open increases its lifetime, so authentication and direct transport tests come first. +The endpoint already dispatches Agenta tools and currently relies only on loopback +reachability (`mcp-bridge.ts` advertises no headers). This is justified independently of +the warm path and lands regardless of the gate outcomes. ### Work -- Generate a random bearer token per session environment. +- Generate a random bearer token per session environment; each environment owns a distinct + server and token. - Advertise the token in the MCP server's standard `authorization` header. -- Reject missing or wrong tokens before parsing or dispatching a tool call. -- Place the token validation in the HTTP transport wrapper in `tool-mcp-http.ts`, never in - the transport-neutral message handler that PR #5234 extracts as `tools/mcp-handler.ts`. - The in-sandbox stdio shim shares that handler and must stay credential-free; only the - listener-owning HTTP transport has anything to authenticate. -- Keep the existing one-megabyte request-body cap and add an explicit result-size cap. +- Authenticate from the request headers before reading or parsing the body, and reject + missing or wrong tokens before any dispatch. +- Make the token comparison timing-safe. +- Test token rotation after environment replacement. +- Keep the existing one-megabyte request-body cap. Treat the result-size cap as a separate + concern from authentication, and state which stage it bounds (the incoming `/run` output, + the serialization, or the MCP response body) when implementing it. - Reject a JSON-RPC batch containing a client tool before executing any item in that batch. + Batch semantics are protocol behavior, so the rejection lives in the server's message + handling in `tool-mcp-http.ts`; if PR #5234 later extracts a minimal shared MCP + dispatcher while building its stdio shim, the rejection moves with it. (The earlier + ordering, where PR #5234's handler extraction landed before WP1, is gone: that + extraction was cut from #5234's v1.) - Keep non-client batches unchanged unless a test finds a protocol violation. - Add focused HTTP integration tests for initialize, list, normal call, unauthorized call, malformed JSON, oversized body, client-tool batch rejection, and close. @@ -82,166 +113,65 @@ Hold-open increases its lifetime, so authentication and direct transport tests c ### Exit gate -An unauthenticated sibling process cannot list or call tools. Valid local Claude behavior remains -unchanged. The MCP server integration suite owns its real socket lifecycle. - -### Rollback - -Revert this package independently. It does not depend on hold-open behavior. - -## WP2: add the neutral continuation kernel - -### Purpose - -Introduce the identity, lifecycle, and ownership rules before any HTTP request is held. - -### Work - -- Add the operation, registry, and delivery-port interfaces from [interface.md](interface.md). -- Implement an in-memory registry with atomic claim and terminal transitions. -- Enforce one pending client tool per environment and a global cap no greater than the session - pool cap. -- Add `environmentId` and `runnerInstanceId` routing identity without exposing them on the public - `/run` wire. -- Add expiry and environment-wide cancellation. -- Add counters and durations with ids, inputs, outputs, and credentials excluded. -- Add unit tests for registration, duplicate registration, capacity, claim races, late completion, - expiry, transport close, and environment cancellation. -- Add the runner-only `AGENTA_RUNNER_CLIENT_TOOL_CONTINUATION` kill switch, default off. - -### Exit gate - -The registry has full deterministic unit coverage. No request is held and current client-tool -behavior is byte-for-byte unchanged while the kill switch is off. - -## WP3: wire the local Claude hold-open path - -### Purpose - -Use the neutral kernel to resume the original MCP call and harness prompt. - -### Work - -#### Register and park - -- In the single-message `tools/call` path, create an `McpHttpResultDelivery` before pausing. -- Extend the client-tool relay result so the transport receives the correlated ACP tool-call id - and interaction id. -- Register the operation before emitting the pause. If registration fails, use today's - `MCP_PAUSED` destroy-and-cold behavior. -- Record `parkedClientTool` on the environment with the neutral operation and original prompt - promise. -- Teach the pause callback to preserve the MCP server and session only when a valid - `parkedClientTool` exists and the kill switch is on. -- Add `awaiting_client_tool` to the pool with the approval TTL capped by WP0's measured transport - ceiling. - -#### Claim and resume - -- Add an exact current-turn extractor for one `tool_result` with the parked ACP tool-call id. -- In `server.ts`, validate project, session, history fingerprint, mount expiry, operation owner, - and exact tool-call id. Do not compare newly minted per-turn credentials with the parked - environment, matching the existing approval-resume rule. -- Atomically check out the `awaiting_client_tool` session. A losing request cannot access the - delivery port. -- Add a `resumeClientTool` form to `runTurn`. It installs the new turn's stream and trace sink, - seeds the existing tool-call span, writes the real JSON-RPC result, and awaits the original - prompt promise. -- Resolve the interaction only after delivery succeeds and the continuation is accepted. -- Re-park a normally completed continuation as idle. A new gate can use its own state. - -#### Preserve fallback - -- Do not consume or remove the browser output from the inbound request before delivery succeeds. -- On a missing handle, wrong owner, closed transport, delivery failure, or lost checkout race, - destroy the stale live environment and run the existing cold path with the original request. -- Do not retry cold after continuation output has already streamed to the caller, matching the - current no-duplicate-stream rule. - -### Exit gate - -A real HTTP integration test proves that the original JSON-RPC request stays open, receives the -browser output once, and returns the same request id. A runner integration test proves that the -original prompt continues without a second model-issued client-tool call. +An unauthenticated sibling process cannot list or call tools. Valid local Claude behavior +remains unchanged. The MCP server integration suite owns its real socket lifecycle. ### Rollback -Disable `AGENTA_RUNNER_CLIENT_TOOL_CONTINUATION`. The code returns to the existing socket destroy -and cold replay path without a deployment rollback. - -## WP4: complete the failure and resource envelope - -### Purpose - -Default-off happy-path code is not ready to enable until every long-lived-resource exit is tested. - -### Work - -- Cancel and evict when the MCP client closes the held request. -- Cancel and evict on approval TTL, mount expiry, pool eviction, supersede, environment destroy, - runner shutdown, and failed original prompt. -- Reject duplicate browser results, duplicate `/run` requests, a result for another tool-call id, - and a second pending client tool in the same session. -- Keep one terminal result for each operation and make every cleanup method idempotent. -- Add global pending-operation and held-socket gauges. Alert before the configured pool cap. -- Confirm that pending sockets cannot exceed parked sessions and that both return to baseline after - expiry and shutdown. -- Add structured path metrics: `live`, `cold`, `unsupported`, `expired`, `wrong_replica`, - `transport_closed`, and `delivery_failed`. -- Add a bounded load test at the configured pool maximum and one over-cap request. -- Add process shutdown and restart tests that prove browser output remains usable by the cold path. - -### Exit gate - -- No cross-session or cross-project completion in tests. -- No duplicate completion in race tests. -- No held responses or registry entries after expiry, disconnect, shutdown, or destroy. -- The over-cap case falls back cold without exceeding the configured cap. -- Logs and metrics contain no arguments, outputs, bearer tokens, or credentials. - -## WP5: canary and rollout - -### Stage 1: development - -- Enable the kill switch on a single local runner replica. -- Run the live matrix in [qa.md](qa.md), including waits above 60 seconds and cold fallback after - forced expiry. -- Compare model-call traces between the exact and cold paths. The exact path must not contain a - second client-tool decision. - -### Stage 2: limited environment - -- Enable only where local Claude sessions and session keepalive are already enabled. -- Keep Daytona and other unsupported harnesses on today's behavior: cold replay where a - delivery path exists, and the up-front refusal for Daytona client tools (which PR #5234 - narrows but keeps for client tools). -- Watch pending count, completion path, wait duration, socket close, expiry, wrong-replica, pool - eviction, and process file descriptors. - -### Stage 3: default decision - -Decide whether to turn the runner flag on by default only after the canary establishes the real -timeout and fallback rates. Keep the kill switch after default-on so an MCP client upgrade can be -disabled without a frontend or API deployment. +Revert this package independently. It does not depend on any warm-path decision. + +## Deferred: the warm hold-open path (old WP2 through WP5) + +The full work packages from the earlier revision stay in this file's history and are not +scheduled. If the unlock gates pass, the build follows the revised shape below, which also +folds in the review's structural findings: + +- **No standalone continuation registry.** The session pool already owns capacity, TTL, + atomic checkout, and teardown, and the first release allows one pending client tool per + environment. Build instead: a neutral `ParkedClientTool` record beside `ParkedApproval` + in the sandbox-agent session model; an `awaiting_client_tool` state and a dedicated + checkout in `session-pool.ts`; an exact current-turn result extractor beside the existing + responder extractors; and `McpHttpResultDelivery` under `tools/` as transport code. A + durable registry appears only at a future gateway boundary, when a second owner exists to + constrain the abstraction. +- **The slimmed delivery port** in [interface.md](interface.md): `deliver` plus `dispose`, + no `cancel`, no `onClosed`. Correctness rests on lease expiry and environment teardown, + never on a transport close signal. +- **Warm registration requires proven correlation.** Register only when the correlation + index returned a real harness tool-call id; otherwise destroy the MCP response and use + cold replay. The best-effort fallback id is acceptable for cold replay only. +- **Register before the interaction is emitted.** The current code correlates, marks the + paused call, emits the browser interaction, and creates the durable interaction inside + `buildClientToolRelay` before the HTTP layer learns anything. Refactor to a + prepare/commit sequence: prepare the pending call and return the correlated identity + without emitting; register the delivery and park ownership; then emit the interaction + and pause; on registration failure, commit the cold-pause form explicitly. +- **Owner-routed deployments only**, per the third condition above. + +### Wrong-replica commit point + +The fallback design must fix one commit point precisely: + +- **Before delivery is accepted:** the inbound browser result stays readable so cold replay + can use it. A resume that reaches a non-owner replica goes cold with the preserved + result. +- **After delivery is accepted:** never start a cold continuation because the original + prompt later fails. The MCP result may already have been consumed by the harness, and a + second continuation would double the side effects. + +A resume on replica B while replica A owns the live handle cannot cancel or invalidate A; +restricting warm mode to owner-routed deployments is what prevents the overlap, not the +fallback logic. ## Deployment and compatibility notes -- PR #5197 must merge before WP3 starts. Rebase and re-check `sandbox_agent.ts`, `server.ts`, - `session-pool.ts`, continuity invalidation, and `shouldPark` before editing. -- PR #5234 ([../in-sandbox-tool-mcp/](../in-sandbox-tool-mcp/README.md)) refactors the same - files WP1 and WP3 edit: its slice 1 extracts the transport-neutral message handler from - `tool-mcp-http.ts` into `tools/mcp-handler.ts` (with an optional client-tool pause hook) - and the relay writer from `dispatch.ts` into `tools/relay-client.ts`. Land that slice - first. WP1's batch rejection then goes into the shared handler, WP1's bearer stays in the - HTTP transport wrapper (see WP1), and WP3's register-before-pause logic plugs into the - handler's pause hook. The combined landing order across the three tool projects is in - [../mcp-delivery-architecture/orchestration.md](../mcp-delivery-architecture/orchestration.md). -- PR #5197's Daytona auto-stop default is five minutes, the same as the approval TTL. This project - does not rely on that equality because the exact path is local only. -- A wrong replica cannot access the live handle. It uses cold fallback. A future gateway can route - the completion to the owner through the neutral registry contract. -- A future actor or principal id belongs in authenticated request context when the platform makes - it available. Do not infer one from a tool name, browser payload, or untrusted metadata. -- No new public wire field is required for the first release because the existing browser - `tool_result.toolCallId` is the exact live key. - +- WP0 and WP1 have no dependency on PR #5197 or PR #5234 and can land now. +- If the warm path is later authorized, it starts only after PR #5197 merges (it edits the + same environment, park, teardown, and continuity code); rebase and re-check + `sandbox_agent.ts`, `server.ts`, `session-pool.ts`, and `shouldPark` first. +- PR #5234 no longer extracts a shared MCP handler in its v1, so nothing here waits on it. + Its project narrows the Daytona refusal for executable tools; client tools on Daytona + keep the up-front refusal until the separate bridge workspace exists (see + [../mcp-delivery-architecture/orchestration.md](../mcp-delivery-architecture/orchestration.md)). +- No new public wire field is required. The existing browser `tool_result.toolCallId` is + the exact live key if the warm path is ever built. diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/qa.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/qa.md index 180ecbffdf..b330806773 100644 --- a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/qa.md +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/qa.md @@ -1,5 +1,11 @@ # Verification plan +Scope note (2026-07-11): the warm hold-open path is deferred behind the unlock gates in +[plan.md](plan.md). The WP0 and WP1 layers below apply now. Every layer tied to WP2 through +WP5 (registry, park, resume, hold, canary) applies only if the gates pass, and should then +be re-read against the revised design note in [interface.md](interface.md) (pool-owned +placement, slimmed delivery port, no standalone registry). + ## Test layers This project needs real transport tests in addition to engine unit tests. A fake response object diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/research.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/research.md index 0db728dcef..a8daff08eb 100644 --- a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/research.md +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/research.md @@ -96,10 +96,10 @@ server and session only for that case. `services/runner/src/engines/sandbox_agent/mcp.ts` skips the internal MCP channel on Daytona. `127.0.0.1` inside the sandbox is not the runner's loopback interface. Non-Pi Daytona runs with Agenta tools are rejected before session creation because no delivery path exists. PR #5234 -([../in-sandbox-tool-mcp/](../in-sandbox-tool-mcp/README.md)) narrows that refusal: after its -slice 2, gateway tools reach MCP-client harnesses on Daytona through an in-sandbox stdio shim -and the file relay, and the up-front refusal remains only for client tools and for non-Daytona -remote providers. +([../in-sandbox-tool-mcp/](../in-sandbox-tool-mcp/README.md)) narrows that refusal: once its +shim ships, gateway tools reach MCP-client harnesses on Daytona through an in-sandbox stdio +shim and the file relay, and the up-front refusal remains only for client tools and for +non-Daytona remote providers. PR #5197 reconnects and resumes Daytona sandboxes, but it does not change this transport boundary. Exact client-tool continuation on Daytona therefore requires either a future MCP gateway or a diff --git a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/status.md b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/status.md index 2e691919ba..bb25acf269 100644 --- a/docs/design/agent-workflows/projects/mcp-client-tool-continuation/status.md +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/status.md @@ -1,6 +1,6 @@ # Status -Last updated: 2026-07-11 +Last updated: 2026-07-11 (late) ## Current phase @@ -9,6 +9,12 @@ Design review. No implementation code has been changed. Draft design PR: [#5201](https://github.com/Agenta-AI/agenta/pull/5201), labelled `needs-review`. +**Headline for the owner:** the warm hold-open path (old WP2 through WP5) is now DEFERRED +behind two measurement gates, on the recommendation of a Codex xhigh review folded in while +the owner slept. He had LGTM'd the fuller plan; this is a deliberate conservative rescope +under his standing simplify-aggressively instruction, and it is reversible. It is the top +item in [open-questions.md](open-questions.md). + ## Completed - Read the Codex onboarding, plan-feature, interface-design, GitButler, documentation, and PR @@ -21,41 +27,59 @@ Draft design PR: [#5201](https://github.com/Agenta-AI/agenta/pull/5201), labelle continuity invalidation, sandbox lifecycle, native session load, and ownership work. - Classified existing risks separately from risks introduced by holding sockets open. - Defined a gateway-neutral pending-operation and delivery-port contract. -- Split the implementation into six progressive work packages with rollback gates. -- 2026-07-11: folded in the cross-consistency review round (requested by the owner alongside - his own review of the event-driven-tool-relay PR). interface.md now states the - transport-specific limits of `cancel` and `onClosed`, opens the `transport` union, and adds - the future in-sandbox stdio mapping (PR #5234) with its three missing prerequisites. plan.md - now places the WP1 bearer in the HTTP transport wrapper (never in the shared - `mcp-handler.ts` that PR #5234 extracts), declares that PR #5234 slice 1 lands before WP1 - and WP3, and corrects the WP5 Daytona wording (refusal for client tools, cold elsewhere). - research.md notes how PR #5234 narrows the Daytona refusal. Combined landing order: - `../mcp-delivery-architecture/orchestration.md`. +- 2026-07-11: folded in the cross-consistency review round (transport-specific limits of the + old port methods, the open transport union, the future in-sandbox stdio mapping and its + three missing prerequisites, the WP1 bearer placement, the WP5 Daytona wording). +- 2026-07-11 (late): folded in the Codex xhigh review of this workspace. Changes: + - **Rescoped v1 to measure and harden.** WP0 expanded with cold-path baseline metrics + (first-reissue match rate, argument-drift rate, added model calls/latency/token cost, + wrong-replica resumption rate, user wait percentiles). WP1 gained the review's + hardening details (auth from headers before body parsing, timing-safe comparison, + per-environment token, rotation test, result-size cap separated from auth). WP2 + through WP5 are deferred behind explicit unlock gates stated in [plan.md](plan.md). + - **Slimmed the delivery port** in [interface.md](interface.md): `deliver` returning + `accepted | unavailable(reason)` plus `dispose(reason)`; `cancel` and `onClosed` cut + (they modeled an HTTP response handle); transport liveness is an optional adapter-owned + closed signal; correctness rests on lease expiry and environment teardown. + - **Replaced the standalone registry with pool-owned placement**: `ParkedClientTool` + beside `ParkedApproval`, `awaiting_client_tool` plus checkout in `session-pool.ts`, + exact extraction beside the responder extractors, `McpHttpResultDelivery` under + `tools/`; a durable registry only at a future gateway boundary. + - **Renamed `harnessToolCallId` to `toolCallId`** with its provenance documented as an + invariant: warm registration requires a proven harness-correlated id; the best-effort + fallback id is cold-only. + - **Fixed the wrong-replica posture**: any future warm mode is restricted to owner-routed + deployments, and the delivery commit point is defined (before `accepted`: preserve the + inbound result for cold; after: never start a cold continuation). + - **Dropped the dependency on PR #5234's handler extraction** (cut from that project's + v1); WP1's batch rejection lands in `tool-mcp-http.ts` and moves only if a shared + dispatcher is extracted later. ## Decisions proposed -- Ship local Claude first. Keep Daytona exact continuation out of scope. -- Require the measured client timeout to exceed 60 seconds. -- Add loopback authentication before hold-open. -- Use a separate runner kill switch, default off. -- Support one pending client tool per session. -- Use exact ACP tool-call identity for live completion and retain name-and-arguments matching only - for cold fallback. +- Defer the warm path behind the transport gate and the value gate; ship WP0 (expanded) and + WP1 now. +- Require the measured client timeout to exceed 60 seconds, or cut (not defer) the warm + path. +- Add loopback authentication before any hold-open work, and land it regardless. +- Restrict any future warm mode to owner-routed deployments. +- Use exact ACP tool-call identity for live completion and retain name-and-arguments + matching only for cold fallback. - Treat PR #5197 as the improved cold and lifecycle layer, not as a pending-operation store. ## Dependencies | Dependency | State | Effect | | --- | --- | --- | -| Session keepalive pool and approval parking | Implemented on `big-agents` | Supplies live session parking and resume structure. | +| Session keepalive pool and approval parking | Implemented on `big-agents` | Supplies live session parking and resume structure for the deferred warm path. | | Pi approval parking, PR #5185 | Merged | No client-tool work required for Pi. | | Session keepalive design, PR #5153 | Draft | Contains the original client-tool hold-open recommendation and experiments. | -| Session continuity, PR #5197 | Open, next to merge | WP3 waits for merge because it changes the same lifecycle files. | -| In-sandbox tool MCP, PR #5234 | Open, design review | Its slice 1 extracts `mcp-handler.ts` and `relay-client.ts` from the files WP1 and WP3 edit; land it before WP1 and WP3. | -| Real MCP gateway | Out of scope | The interface preserves a future adapter boundary. | +| Session continuity, PR #5197 | Open, next to merge | Gates only the deferred warm path, not WP0 or WP1. | +| In-sandbox tool MCP, PR #5234 | Open, design review | No longer a code dependency; its v1 dropped the shared-handler extraction. | +| Real MCP gateway | Out of scope | The interface note preserves a future adapter boundary. | ## Next step after approval -Run WP0 and post the measured timeout report before authorizing hold-open implementation. WP1 can -then harden the current MCP endpoint independently. WP3 starts only after PR #5197 merges and the -implementation branch is rebased on the resulting `big-agents` head. +Run WP0 (both measurements) and post the report scored against the unlock gates. WP1 can +proceed in parallel as independent hardening. The warm path starts only if both gates pass, +and then only after PR #5197 merges and the implementation branch is rebased.