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..5ec0e6cbce --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/README.md @@ -0,0 +1,45 @@ +# 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 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. 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 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 + +- [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) 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 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 new file mode 100644 index 0000000000..97089592fd --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/context.md @@ -0,0 +1,120 @@ +# 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 + +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 + 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 warm release, if it unlocks + +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. +- 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..39e680e361 --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/interface.md @@ -0,0 +1,228 @@ +# 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. + +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. +- `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. 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 + +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; + toolCallId: string; + }; + scope: { + projectId: string; + sessionId: string; + }; + routing: { + runnerInstanceId: string; + environmentId: string; + }; + tool: { + name: string; + inputDigest: string; + }; + lifecycle: { + state: PendingClientToolState; + createdAt: number; + expiresAt: number; + }; +} +``` + +**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. + +`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 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 +interface ClientToolResultDelivery { + deliver(output: unknown): Promise< + | { kind: "accepted" } + | { kind: "unavailable"; reason: string } + >; + + dispose(reason: string): Promise; +} +``` + +- `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 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` | `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. | + +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 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: + +```ts +interface ClientToolResumeInput { + projectId: string; + sessionId: string; + toolCallId: string; + output: unknown; +} +``` + +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 + +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 +{ + type: "http", + name: "agenta-tools", + url, + headers: [{ name: "authorization", value: `Bearer ${token}` }], +} +``` + +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, 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. `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 +[../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: + +| Local implementation | Future gateway implementation | +| --- | --- | +| 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. | +| 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. 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..4328df38cd --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/open-questions.md @@ -0,0 +1,53 @@ +# Open questions + +## 1. TOP review question: accept the warm-path deferral? + +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: 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 A. If cold drift is rare, a second continuation mechanism is +complexity without users to serve. + +## 2. Are the proposed value-gate thresholds right? + +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: confirm or adjust the thresholds when reviewing question 1. + +## 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. +- 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 new file mode 100644 index 0000000000..216f400b6c --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/plan.md @@ -0,0 +1,177 @@ +# Plan + +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. + +## Decision summary + +- **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. And no design can justify a second continuation mechanism until the +first one's failure rate is measured. WP0 now answers both. + +### Work: transport ceiling (unchanged) + +- 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. + +### Work: cold-path baseline (new) + +From production traces where available, otherwise from a realistic QA batch of client-tool +turns: + +- 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. + +### Exit + +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 (`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; each environment owns a distinct + server and token. +- Advertise the token in the MCP server's standard `authorization` header. +- 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. +- 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 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 + +- 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 new file mode 100644 index 0000000000..b330806773 --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/qa.md @@ -0,0 +1,141 @@ +# 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 +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..a8daff08eb --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/research.md @@ -0,0 +1,169 @@ +# 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 #5234 +([../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 +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 + +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..bb25acf269 --- /dev/null +++ b/docs/design/agent-workflows/projects/mcp-client-tool-continuation/status.md @@ -0,0 +1,85 @@ +# Status + +Last updated: 2026-07-11 (late) + +## 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`. + +**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 + 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. +- 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 + +- 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 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 | 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 (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.