feat(runner): authenticate the internal MCP loopback endpoint (WP1) + continuation measurement protocol (WP0)#5245
Merged
Conversation
…ndbox stdio MCP shim Slice 1 of in-sandbox-tool-mcp (docs/design/agent-workflows/projects/in-sandbox-tool-mcp/). Today a Claude run on Daytona with any custom tool is refused up front (REMOTE_TOOLS_UNSUPPORTED_MESSAGE): execution via the file relay is solved, but nothing inside the sandbox advertises the tools to an MCP-client harness. This revives PR #4873 onto today's paths, consuming the relay client PR #5243 extracted. - tools/tool-mcp-stdio.ts: a dependency-free stdio MCP shim (9.6 kB bundled) the harness's ACP adapter spawns inside the sandbox. NDJSON JSON-RPC; tools/list serves the run's public specs from a specs FILE; tools/call writes a relay request through the shared relay client (atomic publication, per-tool timeoutMs) and maps failures to isError results. Fail-loud exit 1 on missing env/specs. Env contract in tools/tool-mcp-env.ts: AGENTA_AGENT_TOOLS_RELAY_DIR + AGENTA_AGENT_TOOLS_PUBLIC_SPECS_FILE (a file, not an unbounded env value copied through four exec layers). - engines/sandbox_agent/tool-mcp-assets.ts: bundle + specs upload, always-write (closes bundle-version skew in reused sandboxes), THROWS on missing bundle/failed upload on the path that requires the shim. Bundle override SANDBOX_AGENT_RELAY_MCP_BUNDLE. - engines/sandbox_agent/mcp.ts: dedicated internal-entry constructor building the typeless ACP stdio shape named agenta-tools (claude_settings.py couples permission rules to it); McpServerStdio moved here out of mcp-bridge.ts; user-declared servers may not claim the reserved name; toAcpMcpServers still cannot emit stdio; user-stdio refusal unrelaxed. - run-plan.ts: the remote-tools gate narrows - executable tools on Daytona pass (new toolMcpDir ephemeral sibling of the relay dir), client tools refuse loud with their own message, non-Daytona remote providers stay fail-closed. - engines/sandbox_agent/relay-guard.ts: the relay execution guard now guards EVERY harness (review finding): allow/deny identical everywhere; ask consumes the Pi grant ledger on Pi and passes on MCP harnesses whose own dialog gates the call, with the forged-ask residual documented. Previously Claude relay records executed unguarded. - The slice-0 restart spike locked the A2 transport: the pinned Claude ACP adapter (claude-agent-acp 0.22.2 in snapshot agenta-sandbox-pi) respawns the stdio MCP subprocess on the session/load path after a VM stop/restart (spike-restart.md in the workspace). Tests: 25+ new (shim handler + real startToolRelay round trip incl. crash-after-write, concurrency, stdout purity; assets fail-loud; three-layer session MCP pin; gate matrix; non-Pi guard semantics). Suite 1054 passed / 4 known pre-existing failures; typecheck clean for the slice; both bundles build with the forbidden-server-symbols gate. Claude-Session: https://claude.ai/code/session_01Fr4A5zjs5rsufRaWgWiUNC
…internal MCP name The engine half of the shim slice, split from the previous commit after a cross-lane hunk-dependency snag (a formatting pass had touched the parallel Daytona-secret lane's committed regions; reverted, insertions re-anchored): - sandbox_agent.ts: upload the shim bundle + specs file on the Daytona non-Pi executable-tools path (fail-loud helper, deps-injectable) and pass the assets into buildSessionMcpServers; the relay execution guard is now built for EVERY harness via buildRelayExecutionGuard (allow/deny identical everywhere; ask consumes the Pi grant ledger on Pi and passes on MCP harnesses whose own dialog gates the call). - mcp.ts: materialization-time reserved-name defense (assertNoReservedUserMcpName at the top of buildSessionMcpServers). - run-plan.ts: declaration-time refusal of a user MCP server named agenta-tools, before any resource is created, plus its gate test. Claude-Session: https://claude.ai/code/session_01Fr4A5zjs5rsufRaWgWiUNC
The three-layer test used the credentials wire field owned by the parallel Daytona-secret work, so this lane's tip failed typecheck standalone. The layering pin is about WHICH servers are delivered; auth delivery has its own tests. Also swap the 'no Bearer' assertion for 'no user MCP url leaks into the internal entry'. Claude-Session: https://claude.ai/code/session_01Fr4A5zjs5rsufRaWgWiUNC
…CP shim The internal gateway-tool channel now has two transports (local loopback HTTP; the in-sandbox stdio shim on Daytona): update the runner-to-MCP-server interface page (shim env contract, SANDBOX_AGENT_RELAY_MCP_BUNDLE, reserved server name, narrowed remote gates, the every-harness relay execution guard and its stated ask residual), the interface index rows, the harness-adapters and mcp-models coupling notes, the permission-responder guard note, the tools/ground-truth/claude-code narrative pages, and the operator env-var inventory in running-the-agent. Claude-Session: https://claude.ai/code/session_01Fr4A5zjs5rsufRaWgWiUNC
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe change secures the internal loopback MCP endpoint with per-server bearer tokens, rejects unsupported client-tool batches, expands endpoint tests, updates channel assertions, and adds a README for measuring MCP client-tool continuation experiments. ChangesMCP loopback endpoint security
Continuation experiment protocol
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant InternalMcpServer
participant ClientRelay
MCPClient->>InternalMcpServer: POST with per-server bearer token
InternalMcpServer->>InternalMcpServer: Validate authorization and batch contents
InternalMcpServer->>ClientRelay: Dispatch supported client tool call
ClientRelay-->>InternalMcpServer: Return tool result
InternalMcpServer-->>MCPClient: Return JSON-RPC response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b71b07df-51ca-4798-b5f2-c48867c2fa2d
📒 Files selected for processing (6)
docs/design/agent-workflows/projects/mcp-client-tool-continuation/experiments/README.mdservices/runner/src/tools/mcp-bridge.tsservices/runner/src/tools/tool-mcp-http.tsservices/runner/tests/unit/sandbox-agent-orchestration.test.tsservices/runner/tests/unit/session-mcp-layering.test.tsservices/runner/tests/unit/tool-bridge.test.ts
Comment on lines
+22
to
+24
| 1. Start one local runner environment with one browser-fulfilled client tool. Capture the runner, | ||
| harness, ACP, and MCP logs needed to correlate request identities. Do not enable or implement the | ||
| deferred warm path for this experiment. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Define repeated-trial criteria before making a permanent transport decision.
The protocol starts one local environment, but any transport failure is said to cut the warm path “permanently.” Require repeated independent trials and a predefined aggregation rule so a transient runner, OS, or Claude failure does not become an irreversible product decision.
Also applies to: 79-80
Comment on lines
+39
to
+47
| | Transport result | Observed value | | ||
| | --- | --- | | ||
| | Same MCP request id after more than 60 seconds | `PENDING LIVE RUN` | | ||
| | Same ACP tool-call id after more than 60 seconds | `PENDING LIVE RUN` | | ||
| | Client close, retry, or settled error after more than 60 seconds | `PENDING LIVE RUN` | | ||
| | Same ids after more than 300 seconds | `PENDING LIVE RUN` | | ||
| | Measured request lifetime ceiling and safety margin | `PENDING LIVE RUN` | | ||
| | One quiet pending socket fd delta | `PENDING LIVE RUN` | | ||
| | One quiet pending socket RSS/PSS delta | `PENDING LIVE RUN` | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Measure successful fulfillment, not only ID retention.
The transport gate requires the original MCP request to remain usable, but these rows only record whether IDs remain present or whether the connection closes/retries/errors. Add whether the delayed result was accepted and produced a continued Claude response, and keep close, retry, and settled-error outcomes distinct.
Suggested measurement rows
| Same MCP request id after more than 60 seconds | `PENDING LIVE RUN` |
| Same ACP tool-call id after more than 60 seconds | `PENDING LIVE RUN` |
+| Delayed result accepted and continued answer after more than 60 seconds | `PENDING LIVE RUN` |
| Client close, retry, or settled error after more than 60 seconds | `PENDING LIVE RUN` |
| Same ids after more than 300 seconds | `PENDING LIVE RUN` |
+| Delayed result accepted and continued answer after more than 300 seconds | `PENDING LIVE RUN` |Also applies to: 82-85
Comment on lines
+82
to
+85
| | Gate | Passing rule | Measurement | Decision | | ||
| | --- | --- | --- | --- | | ||
| | Transport | The original MCP request remains usable for at least 60 seconds, with a measured ceiling and safety margin. | `PENDING LIVE RUN` | `PENDING LIVE RUN` | | ||
| | Value | First-reissue mismatch is greater than 5%, argument-drift re-interaction is greater than 2%, or cold continuation adds user-visible p50 latency measured in seconds or model cost that users or support report. | `PENDING LIVE RUN` | `PENDING LIVE RUN` | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the value gate deterministic.
“p50 latency measured in seconds” has no threshold, and “model cost that users or support report” has no numeric criterion or evidence requirement. Different reviewers could reach opposite decisions from the same results; define explicit latency/cost thresholds and the required supporting evidence.
Member
Author
|
Reviewed with Codex (high effort). The auth logic is clean: the 401 gate runs before any body parse, the token compare is timing-safe, and a batch with a client tool runs nothing before it is rejected. The minor items are fixed in #5248: authenticate before the method check (401, not 405, for GET/DELETE) and the extra auth tests. One blocker is yours to decide: the bearer rides the harness command line. The Claude SDK puts the MCP config, including our |
This was referenced Jul 12, 2026
Closed
Merged
mmabrouk
commented
Jul 12, 2026
| // a distinct token, which protects the loopback endpoint from other local processes. | ||
| headers: [ | ||
| { | ||
| name: "Authorization", |
Member
Author
There was a problem hiding this comment.
This bearer guards the loopback endpoint from other local processes. Heads-up for review: the Claude SDK serializes this header into the child's --mcp-config argv (verified in sdk.mjs), so the token is readable from /proc/<pid>/cmdline. That is the blocker in my summary comment. The fix is to deliver the token by environment variable, pending a live check that the pinned Claude CLI expands ${VAR} in header values.
Harden the internal gateway-tool MCP endpoint that delivers Agenta tools to local Claude over loopback HTTP. Until now it relied only on 127.0.0.1 reachability, so any other local process could list or call tools through it. - Mint a per-server bearer token (randomBytes 32, base64url) in startInternalToolMcpServer; advertise it as the entry's Authorization header in buildToolMcpServers. - Authenticate before reading or parsing the request body; reject missing/wrong tokens with 401 and no dispatch. Timing-safe comparison, length-guarded. - Reject a JSON-RPC batch containing a client tool before executing any item. - The bearer lives ONLY in the HTTP transport path; the in-sandbox stdio shim (#5234) stays credential-free by construction (different file, different transport). It is a local loopback guard, never a provider/control-plane credential — the private callRef still never reaches the advertisement. Adds real-socket HTTP integration tests (auth, malformed/oversized body, batch client-tool rejection, close) and updates the two layering pins that asserted headers: [] to expect the loopback guard header. WP0: checked-in measurement protocol + unlock-gate scorecard for the deferred warm hold-open path (results marked PENDING LIVE RUN). No runtime behavior change from WP0. Claude-Session: https://claude.ai/code/session_0127AM79khCdvD2b8BG2joZL
dosubot
Bot
added
size:L
mmabrouk
added
lgtm
mmabrouk
force-pushed
the
feat-client-tool-continuation-v1
branch
from
6ab5d45 to
43ed55a
Compare
mmabrouk
added a commit
that referenced
this pull request
Jul 12, 2026
Addresses the mechanical review findings on #5243, #5244, and #5245 that carry no design decision and need no live verification. Landed as one commit on top of the stack because GitButler cannot retro-assign a new delta onto a lower applied lane; each fix is mapped to its PR below. Relay (#5243): - relay.ts: bound every relay-file removal with a timeout so a hung daemon deletion can no longer wedge pickup, stale-sweep, or shutdown; a timed-out removal is treated as failed and the loop continues. - relay-watch.ts: tie a safety-poll miss to the active watch generation so a later zero-exit of the same generation cannot erase it (the demotion counter now accumulates); validate the whole watch-window string instead of parseInt accepting partial garbage. Shim (#5244): - tool-mcp-stdio.ts: answer MCP 'ping' with an empty result; return -32700 on unparseable input and -32600 on an invalid request; document the at-least-once relay delivery contract. Protocol-version negotiation is left unchanged (that needs a live check). - tool-mcp-assets.ts: cache the shim bundle by path and write bundle + specs concurrently, removing avoidable serial Daytona round trips. - restored the direct client-only Daytona refusal test; pinned the Pi vs non-Pi ask guard wiring; updated the build:extension docs. Continuation (#5245): - tool-mcp-http.ts: authenticate before the method check so an unauthenticated GET/DELETE gets 401, not 405. - added auth tests: unauthorized GET/DELETE, wrong scheme, empty and whitespace tokens, and that neither the callback executor nor the client relay runs on an unauthorized call. Deferred (need a design decision or a live check, not in this commit): the non-Pi ask authorization model (#5244), the bearer-in-argv delivery (#5245), MCP protocol-version negotiation (#5244). Claude-Session: https://claude.ai/code/session_0127AM79khCdvD2b8BG2joZL
dosubot
Bot
added
the
size:L
dosubot
Bot
added
size:XXL
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom
The internal MCP endpoint that delivers Agenta gateway tools to local Claude (a loopback HTTP server the runner serves on
127.0.0.1) authenticated callers by nothing but reachability. Any other process on the runner host couldPOSTto it and list or call the run's tools — the same host-boundary hole#4831closed for user stdio MCP servers, reopened for the internal channel. This is the v1 slice of the client-tool continuation plan (#5201): harden the endpoint now; defer the warm hold-open path behind measurement gates.What changed
WP1 — authenticate the loopback endpoint (
tool-mcp-http.ts,mcp-bridge.ts)startInternalToolMcpServermints arandomBytes(32).toString("base64url")token per instance;buildToolMcpServersadvertises it as the entry'sAuthorization: Bearer …header. Each run/environment owns a distinct token.401beforereadBody— attacker-controlled input is never read or parsed when unauthorized. Comparison is timing-safe (crypto.timingSafeEqual), length-guarded so a mismatch never throws.client-kindtools/callis rejected up front (-32600) before any sibling item executes — a client tool crosses a turn boundary and cannot safely share a concurrent batch.Before:
headers: [], "the channel is unauthenticated on loopback." After:headers: [Authorization: Bearer <per-session>].Layering (the important invariant): the bearer lives only in the HTTP transport path. The in-sandbox stdio shim (#5234,
tool-mcp-stdio.ts) is byte-unchanged and stays credential-free by construction — different file, different transport. The bearer is a local loopback access guard, never a provider/control-plane credential; the privatecallRefstill never reaches the advertisement (pinned insession-mcp-layering.test.ts).WP0 — measurement protocol for the deferred warm path (docs only)
projects/mcp-client-tool-continuation/experiments/README.md: a checked-in, repeatable protocol for the two measurements that gate the deferred warm hold-open path (transport ceiling: does Claude keep the MCP request usable past the 60s/300s TTLs; cold-path baseline: the five user-cost metrics), plus an unlock-gate scorecard. All observed values arePENDING LIVE RUN. No runtime behavior change from WP0.Deferred (NOT in this PR)
WP2–WP5 (the continuation kernel, the hold-open path, the failure envelope, the canary) stay deferred behind the two unlock gates. The cold-replay fallback remains the one continuation mechanism. See
plan.md"Decision summary." The warm-path deferral is the owner's top open review question — it has not been ruled on.Tests
New real-socket HTTP integration tests in
tool-bridge.test.ts: per-server bearer, token rotation across instances,initialize/list/normal call, missing + wrong token → 401 with no dispatch, malformed JSON, oversized body (>1 MB), client-tool batch rejection, normal close. Updated the two layering pins (session-mcp-layering.test.ts,sandbox-agent-orchestration.test.ts) that assertedheaders: []to expect the loopback-guard header while keeping the "private callRef never leaks" assertion.pnpm run typecheckandpnpm test: all WP1/layering tests green. The 4 remaining failures are pre-existing baseline failures on this stack, unrelated to this change:code-tool.test.ts(orphaned test for removed code),sandbox-agent-qa-transcript-replay(×2),sandbox-agent-run-planhasApiKey.Open rollout gate
One live check remains (local, credit-free, deliberately not run in this autonomous pass to conserve budget): confirm the local Claude ACP adapter forwards the
Authorizationheader on each MCP request. Confidence is high without it — the user HTTP MCP path (#4834, e.g. Linear) already forwards this exactheadersfield to external MCP servers, so the internal channel uses a proven mechanism — but it should be verified before merge. If the adapter did not forward it, every gateway-tool call on local Claude would 401.Where to look first
services/runner/src/tools/tool-mcp-http.ts—hasValidAuthorization(timing-safe), the 401 gate beforereadBody, the batch preflight.services/runner/src/tools/mcp-bridge.ts— token generation seam + the advertisedAuthorizationheader.services/runner/tests/unit/tool-bridge.test.ts— the auth/batch integration tests.Stack
Base:
feat-in-sandbox-tool-mcp(#5244). This lane depends on #5244's shim work only for stack ordering; the WP1 change is confined to the HTTP path.https://claude.ai/code/session_0127AM79khCdvD2b8BG2joZL