From 27803c29b08daeecffd714aa930bb4760af8b65e Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sun, 12 Jul 2026 12:43:20 +0200 Subject: [PATCH 1/2] fix(runner): safe hardening from the stack review pass 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 --- services/runner/AGENTS.md | 3 +- services/runner/README.md | 8 +-- .../engines/sandbox_agent/tool-mcp-assets.ts | 20 +++++-- services/runner/src/tools/relay-watch.ts | 13 ++++- services/runner/src/tools/relay.ts | 57 ++++++++++-------- services/runner/src/tools/tool-mcp-http.ts | 18 +++--- services/runner/src/tools/tool-mcp-stdio.ts | 22 +++++++ .../runner/tests/unit/relay-watch.test.ts | 36 ++++++++++++ .../unit/sandbox-agent-orchestration.test.ts | 49 ++++++++++++++++ .../tests/unit/sandbox-agent-run-plan.test.ts | 24 ++++++++ .../runner/tests/unit/tool-bridge.test.ts | 58 +++++++++++++++++++ .../runner/tests/unit/tool-mcp-assets.test.ts | 41 +++++++++++++ .../runner/tests/unit/tool-mcp-stdio.test.ts | 33 +++++++++-- 13 files changed, 330 insertions(+), 52 deletions(-) diff --git a/services/runner/AGENTS.md b/services/runner/AGENTS.md index 539ff1716b..de7e148347 100644 --- a/services/runner/AGENTS.md +++ b/services/runner/AGENTS.md @@ -13,7 +13,8 @@ Not part of the `web/` pnpm workspace. It has its OWN `pnpm-lock.yaml`, builds i image, and pins Node 24 / pnpm 10.30 / ESM (`"type": "module"`). It runs through `tsx` (no app compile step); the only build is `pnpm run build:extension` (esbuild-bundles the Pi extension into `dist/`). Keep it standalone so the sidecar image stays decoupled from the web -dependency graph. +dependency graph. `pnpm run build:extension` bundles both the Pi extension and the in-sandbox +tool MCP shim into `dist/`. ## Commands diff --git a/services/runner/README.md b/services/runner/README.md index 7cfb87703d..6092101d24 100644 --- a/services/runner/README.md +++ b/services/runner/README.md @@ -99,10 +99,10 @@ servers. See `docs/design/agent-workflows/projects/sidecar-trust-and-sandbox-enf ## The extension bundle -`scripts/build-extension.mjs` esbuild-bundles `src/extensions/agenta.ts` into one -self-contained `dist/extensions/agenta.js` that Pi can load anywhere (host, the sidecar, a -Daytona snapshot). The dev image bakes it; rebuild after editing the extension or the -tracer: +`scripts/build-extension.mjs` esbuild-bundles the Pi extension into +`dist/extensions/agenta.js` and the in-sandbox tool MCP shim into +`dist/tools/tool-mcp-stdio.js`. The dev image bakes both bundles; rebuild after editing the +extension, tracer, or shim: ```bash pnpm run build:extension diff --git a/services/runner/src/engines/sandbox_agent/tool-mcp-assets.ts b/services/runner/src/engines/sandbox_agent/tool-mcp-assets.ts index d1947c68a9..59ddd86a06 100644 --- a/services/runner/src/engines/sandbox_agent/tool-mcp-assets.ts +++ b/services/runner/src/engines/sandbox_agent/tool-mcp-assets.ts @@ -28,6 +28,16 @@ import { PKG_ROOT } from "./daemon.ts"; type Log = (message: string) => void; +const bundleContentsByPath = new Map(); + +function toolMcpBundleContents(path: string): string { + const cached = bundleContentsByPath.get(path); + if (cached !== undefined) return cached; + const contents = readFileSync(path, "utf-8"); + bundleContentsByPath.set(path, contents); + return contents; +} + /** * The bundled shim path. Built by `pnpm run build:extension` alongside the Pi extension. * Resolved lazily (a function, not a module-level const) so `SANDBOX_AGENT_RELAY_MCP_BUNDLE` @@ -77,11 +87,11 @@ export async function uploadToolMcpAssets( const specsPath = `${destDir}/tool-mcp-specs.json`; try { await sandbox.mkdirFs({ path: destDir }); - await sandbox.writeFsFile( - { path: bundlePath }, - readFileSync(bundle, "utf-8"), - ); - await sandbox.writeFsFile({ path: specsPath }, JSON.stringify(specs)); + const bundleContents = toolMcpBundleContents(bundle); + await Promise.all([ + sandbox.writeFsFile({ path: bundlePath }, bundleContents), + sandbox.writeFsFile({ path: specsPath }, JSON.stringify(specs)), + ]); return { bundlePath, specsPath }; } catch (err) { log(`tool MCP shim upload failed: ${(err as Error).message}`); diff --git a/services/runner/src/tools/relay-watch.ts b/services/runner/src/tools/relay-watch.ts index 94a43d967a..9f68a5f0f1 100644 --- a/services/runner/src/tools/relay-watch.ts +++ b/services/runner/src/tools/relay-watch.ts @@ -74,8 +74,8 @@ export function resolveRemoteWatchWindowMs( const raw = process.env.AGENTA_AGENT_TOOLS_RELAY_REMOTE_WATCH_WINDOW_MS; if (raw === undefined || raw === "") return RELAY_REMOTE_WATCH_WINDOW_DEFAULT_MS; - const parsed = Number.parseInt(raw, 10); - if (!Number.isFinite(parsed)) { + const parsed = Number(raw); + if (!Number.isInteger(parsed)) { log( `[relay] unparseable AGENTA_AGENT_TOOLS_RELAY_REMOTE_WATCH_WINDOW_MS '${raw}'; using default ${RELAY_REMOTE_WATCH_WINDOW_DEFAULT_MS}`, ); @@ -277,6 +277,7 @@ export function daytonaRelayActivitySource( // entirely (no wake, no counter reset, no in-flight mutation). let armGeneration = 0; let liveGeneration = 0; // 0 = no live window + let missedGeneration = 0; // active window already charged for a safety-poll miss let outerBoundTimer: ReturnType | undefined; let deferredArmTimer: ReturnType | undefined; let waiter: @@ -406,6 +407,7 @@ export function daytonaRelayActivitySource( outerBoundTimer = undefined; if (closed || gen !== liveGeneration) return; liveGeneration = 0; + if (missedGeneration === gen) missedGeneration = 0; countFailure("window outer bound expired (exec never settled)"); }, jitteredWindow + graceMs + outerBoundMarginMs, @@ -415,6 +417,8 @@ export function daytonaRelayActivitySource( if (gen !== liveGeneration) return; // abandoned by the outer bound: dead gen liveGeneration = 0; clearOuterBound(); + const generationHadMiss = missedGeneration === gen; + if (generationHadMiss) missedGeneration = 0; if (closed) return; // abandoned window (see close()); its wake is meaningless now // Completion classification (fix 2 of the slice-3 review). A nullish result // is a broken daemon path, not a wake: an insta-resolving runProcess that @@ -430,7 +434,9 @@ export function daytonaRelayActivitySource( return; } if (result.exitCode === 0) { - consecutiveFailures = 0; + // A safety poll already charged this generation as a miss. Its ordinary zero exit + // must not erase that failure from the consecutive demotion counter. + if (!generationHadMiss) consecutiveFailures = 0; } else { // Nonzero, null (signal-killed / OOM), or MISSING exit is still a wake (the // list pass is harmless) but counts as a failure so a script that keeps @@ -460,6 +466,7 @@ export function daytonaRelayActivitySource( noteMiss: () => { if (closed || demoted) return; // The safety poll found a request while a window claimed healthy: the watch lied. + if (liveGeneration !== 0) missedGeneration = liveGeneration; countFailure("safety poll found a request the watch missed"); }, wait: ({ timeoutMs, signal }) => { diff --git a/services/runner/src/tools/relay.ts b/services/runner/src/tools/relay.ts index 72f1e951aa..3218ed9662 100644 --- a/services/runner/src/tools/relay.ts +++ b/services/runner/src/tools/relay.ts @@ -418,6 +418,29 @@ function isRelayFileName(name: string): boolean { ); } +/** Bound one daemon removal so relay startup, polling, and shutdown cannot wedge on it. */ +const RELAY_REMOVE_TIMEOUT_MS = 10_000; + +async function removeRelayFile(host: RelayHost, path: string): Promise { + let timer: ReturnType | undefined; + const timeout = new Promise((resolve) => { + timer = setTimeout(() => resolve(false), RELAY_REMOVE_TIMEOUT_MS); + }); + try { + return await Promise.race([ + Promise.resolve(host.remove(path)).then( + () => true, + () => false, + ), + timeout, + ]); + } catch { + return false; + } finally { + clearTimeout(timer); + } +} + /** * Clear pre-turn relay residue from a reused relay dir (a warm-continued turn skips the * cold build's rm -rf, so a crashed prior turn can leave files behind). Every relay @@ -457,7 +480,7 @@ export async function sweepStaleRelayFiles( // is only re-armed watch noise once the loop's seen set has it; a leftover // response is inert for fresh toolCallIds). await Promise.allSettled( - stale.map((name) => host.remove(`${relayDir}/${name}`)), + stale.map((name) => removeRelayFile(host, `${relayDir}/${name}`)), ); log( `[relay] cleared ${stale.length} stale relay file(s) predating the turn`, @@ -588,18 +611,9 @@ export function startToolRelay( // publication, consistent with the stale-sweep decision (a restarted turn must // not re-execute stale requests; the writer times out and surfaces a tool error // instead). A failed removal is recorded for retry on later list passes. - let removeDone: Promise; - try { - removeDone = host.remove(reqPath).then( - () => undefined, - () => { - removeFailed.add(reqName); - }, - ); - } catch { - removeFailed.add(reqName); - removeDone = Promise.resolve(); - } + const removeDone = removeRelayFile(host, reqPath).then((removed) => { + if (!removed) removeFailed.add(reqName); + }); // The execute phase starts NOW (as soon as the read returned) and runs in the // background; the pickup itself only awaits stat + remove. inflight.push(execute(id, raw)); @@ -667,18 +681,11 @@ export function startToolRelay( removeFailed.delete(name); continue; } - try { - pickups.push( - host.remove(`${relayDir}/${name}`).then( - () => { - removeFailed.delete(name); - }, - () => undefined, - ), - ); - } catch { - // Still failing synchronously; keep it in the retry set. - } + pickups.push( + removeRelayFile(host, `${relayDir}/${name}`).then((removed) => { + if (removed) removeFailed.delete(name); + }), + ); } for (const name of names) { if (!name.endsWith(RELAY_REQ_SUFFIX) || seen.has(name)) continue; diff --git a/services/runner/src/tools/tool-mcp-http.ts b/services/runner/src/tools/tool-mcp-http.ts index 91afdbef8e..5f2b06a24a 100644 --- a/services/runner/src/tools/tool-mcp-http.ts +++ b/services/runner/src/tools/tool-mcp-http.ts @@ -311,15 +311,7 @@ export function startInternalToolMcpServer( const requestListener = (req: IncomingMessage, res: ServerResponse): void => { active.add(res); res.on("close", () => active.delete(res)); - // The MCP Streamable-HTTP client opens a GET SSE stream and sends a DELETE on close; this - // stateless server offers neither, so it returns 405 (the client tolerates 405 for both). - if (req.method !== "POST") { - res.writeHead(405, { "content-type": "application/json", allow: "POST" }); - res.end(JSON.stringify({ error: "method not allowed" })); - return; - } - - // Gate the POST path before reading or parsing attacker-controlled input. + // Authenticate every MCP transport verb before dispatching or revealing method support. if (!hasValidAuthorization(req.headers.authorization, authorizationToken)) { res.writeHead(401, { "content-type": "application/json" }); res.end( @@ -332,6 +324,14 @@ export function startInternalToolMcpServer( return; } + // The MCP Streamable-HTTP client opens a GET SSE stream and sends a DELETE on close; this + // stateless server offers neither, so it returns 405 (the client tolerates 405 for both). + if (req.method !== "POST") { + res.writeHead(405, { "content-type": "application/json", allow: "POST" }); + res.end(JSON.stringify({ error: "method not allowed" })); + return; + } + readBody(req) .then(async (raw) => { let parsed: any; diff --git a/services/runner/src/tools/tool-mcp-stdio.ts b/services/runner/src/tools/tool-mcp-stdio.ts index e39fef3be6..308557f32c 100644 --- a/services/runner/src/tools/tool-mcp-stdio.ts +++ b/services/runner/src/tools/tool-mcp-stdio.ts @@ -148,6 +148,9 @@ export function loadShimConfig( } /** + * Delivery is at least once: after pickup, the runner still executes if this writer dies, and + * a writer retry publishes a new request because the relay does not deduplicate retries. + * * Handle one MCP JSON-RPC message. Returns the response object, or `undefined` for a * notification (no `id`). Mirrors `tools/tool-mcp-http.ts` `handle` (initialize / tools-list / * tools-call shapes), but `tools/call` writes a relay request file (`relayToolCall`) rather @@ -160,6 +163,14 @@ export async function handleToolMcpMessage( ): Promise { const { id, method, params } = message ?? {}; + if (typeof method !== "string") { + return { + jsonrpc: "2.0", + id: id ?? null, + error: { code: -32600, message: "invalid request" }, + }; + } + // Notifications (no id, e.g. notifications/initialized) need no response. if (id === undefined || id === null) return undefined; @@ -175,6 +186,10 @@ export async function handleToolMcpMessage( }; } + if (method === "ping") { + return { jsonrpc: "2.0", id, result: {} }; + } + if (method === "tools/list") { return { jsonrpc: "2.0", @@ -271,6 +286,13 @@ export async function runToolMcpStdio( message = JSON.parse(trimmed); } catch (err) { log(`parse error: ${(err as Error).message}`); + output.write( + `${JSON.stringify({ + jsonrpc: "2.0", + id: null, + error: { code: -32700, message: "parse error" }, + })}\n`, + ); continue; } const pending = handleToolMcpMessage(message, specs, relayDir).then( diff --git a/services/runner/tests/unit/relay-watch.test.ts b/services/runner/tests/unit/relay-watch.test.ts index c0cd53be48..fe9e044ab4 100644 --- a/services/runner/tests/unit/relay-watch.test.ts +++ b/services/runner/tests/unit/relay-watch.test.ts @@ -373,6 +373,31 @@ describe("daytonaRelayActivitySource invariants", () => { source.close(); }); + it("a zero exit from the same missed generation does not reset the demotion counter", async () => { + const { sandbox, calls } = fakeSandbox(); + const source = daytonaRelayActivitySource(sandbox, "/relay", { + windowMs: 60_000, + backoffBaseMs: 1, + backoffCapMs: 2, + }); + + for (let generation = 0; generation < 3; generation += 1) { + const wait = source.wait({ timeoutMs: 5_000 }); + assert.equal(calls.length, generation + 1); + source.noteMiss?.(); + calls[generation].resolve({ exitCode: 0, timedOut: false }); + assert.equal(await wait, "activity"); + await sleep(10); + } + + assert.equal( + source.isHealthy(), + false, + "three missed generations demote even though each later exits zero", + ); + source.close(); + }); + it("outer bound: a never-settling exec counts as a failure, re-arms inside the SAME wait, demotes after three, and late settles change nothing", async () => { const settlers: Array<{ resolve: (r: { exitCode?: number | null; timedOut?: boolean }) => void; @@ -541,6 +566,17 @@ describe("remote watch window config", () => { assert.match(logs[0], /unparseable/); }); + it("partial numeric garbage -> default with one warning", () => { + const logs: string[] = []; + process.env[WINDOW_ENV] = "30000junk"; + assert.equal( + resolveRemoteWatchWindowMs((m) => logs.push(m)), + RELAY_REMOTE_WATCH_WINDOW_DEFAULT_MS, + ); + assert.equal(logs.length, 1); + assert.match(logs[0], /unparseable/); + }); + it("1000 -> clamped to the 5000 floor with one warning", () => { const logs: string[] = []; process.env[WINDOW_ENV] = "1000"; diff --git a/services/runner/tests/unit/sandbox-agent-orchestration.test.ts b/services/runner/tests/unit/sandbox-agent-orchestration.test.ts index e0b68a87a7..2302bed442 100644 --- a/services/runner/tests/unit/sandbox-agent-orchestration.test.ts +++ b/services/runner/tests/unit/sandbox-agent-orchestration.test.ts @@ -720,6 +720,55 @@ describe("runSandboxAgent orchestration", () => { ); }); + it("wires Pi ask grants into the captured relay guard while non-Pi ask passes", async () => { + const request = { + messages: [{ role: "user", content: "use the tool" }], + permissions: { default: "ask" }, + customTools: [ + { name: "server_tool", kind: "callback", permission: "ask" }, + ], + } as AgentRunRequest; + const relayRequest = { + toolName: "server_tool", + toolCallId: "forged-call", + args: {}, + }; + + const pi = fakeHarness(); + const piResult = await runSandboxAgent( + { ...request, harness: "pi_core" }, + undefined, + undefined, + pi.deps, + ); + assert.equal(piResult.ok, true); + const piGuard = pi.calls.toolRelayArgs?.[6] as Function; + assert.deepEqual( + piGuard(request.customTools?.[0], relayRequest), + { + allow: false, + reason: + "Tool 'server_tool' was not approved via the permission dialog.", + }, + "Pi ask without a grant fails closed", + ); + + const claude = fakeHarness(); + const claudeResult = await runSandboxAgent( + { ...request, harness: "claude" }, + undefined, + undefined, + claude.deps, + ); + assert.equal(claudeResult.ok, true); + const claudeGuard = claude.calls.toolRelayArgs?.[6] as Function; + assert.deepEqual( + claudeGuard(request.customTools?.[0], relayRequest), + { allow: true }, + "non-Pi ask remains gated by the harness and passes the relay guard", + ); + }); + it("delivers a non-Pi run's gateway tools over the internal HTTP MCP channel", async () => { // Claude takes tools only over MCP. The INTERNAL gateway-tool channel (distinct from the // disabled USER stdio MCP path) is restored over a loopback HTTP MCP server the runner diff --git a/services/runner/tests/unit/sandbox-agent-run-plan.test.ts b/services/runner/tests/unit/sandbox-agent-run-plan.test.ts index 054ca04cdb..daf3c90dae 100644 --- a/services/runner/tests/unit/sandbox-agent-run-plan.test.ts +++ b/services/runner/tests/unit/sandbox-agent-run-plan.test.ts @@ -757,6 +757,30 @@ describe("buildRunPlan", () => { ); }); + it("refuses claude x daytona x client-only tools directly", () => { + let created = false; + const result = buildRunPlan( + { + harness: "claude", + sandbox: "daytona", + messages: [{ role: "user", content: "hello" }], + customTools: [{ name: "request_connection", kind: "client" }], + } as AgentRunRequest, + { + createDaytonaCwd: () => { + created = true; + return "/home/sandbox/agenta-fixed"; + }, + }, + ); + + assert.equal(result.ok, false); + if (result.ok) return; + assert.match(result.error, /Client tools are not supported/); + assert.match(result.error, /browser round-trip/); + assert.equal(created, false, "fails before any cwd is created"); + }); + it("allows pi x daytona x client-only tools (Pi's extension + file relay deliver them)", () => { const result = buildRunPlan( { diff --git a/services/runner/tests/unit/tool-bridge.test.ts b/services/runner/tests/unit/tool-bridge.test.ts index 991b67ab72..c629737c15 100644 --- a/services/runner/tests/unit/tool-bridge.test.ts +++ b/services/runner/tests/unit/tool-bridge.test.ts @@ -185,6 +185,64 @@ describe("buildToolMcpServers (internal gateway-tool channel)", () => { }); describe("the served MCP endpoint", () => { + it("authenticates every method and token shape before either executor can run", async () => { + const dir = mkdtempSync(join(tmpdir(), "agenta-tool-auth-")); + let clientDispatchCount = 0; + const clientToolRelay: ClientToolRelay = { + onClientTool: async () => { + clientDispatchCount += 1; + return "deny"; + }, + }; + try { + const { servers } = await build( + [ + { name: "search", kind: "callback", callRef: "composio.search" }, + { name: "confirm", kind: "client" }, + ], + dir, + { clientToolRelay }, + ); + const url = servers[0].url; + const token = authorizationFor(url).slice("Bearer ".length); + + for (const method of ["GET", "DELETE"]) { + const response = await fetch(url, { method }); + assert.equal(response.status, 401, `unauthenticated ${method}`); + assert.equal(((await response.json()) as any).error.code, -32001); + } + + const request = JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { name: "search", arguments: { q: "hi" } }, + }); + for (const authorization of [ + "Basic " + token, + "Bearer", + "Bearer " + token, + "Bearer\t" + token, + ]) { + const response = await fetch(url, { + method: "POST", + headers: { + "content-type": "application/json", + authorization, + }, + body: request, + }); + assert.equal(response.status, 401, authorization); + assert.equal(((await response.json()) as any).error.code, -32001); + } + + assert.deepEqual(readdirSync(dir), [], "the callback executor never publishes a request"); + assert.equal(clientDispatchCount, 0, "the client relay is never invoked"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it("answers initialize / tools/list (public spec only, client filtered)", async () => { const specs: ResolvedToolSpec[] = [ { diff --git a/services/runner/tests/unit/tool-mcp-assets.test.ts b/services/runner/tests/unit/tool-mcp-assets.test.ts index a2c483904f..d875c3a0ad 100644 --- a/services/runner/tests/unit/tool-mcp-assets.test.ts +++ b/services/runner/tests/unit/tool-mcp-assets.test.ts @@ -98,6 +98,47 @@ describe("uploadToolMcpAssets", () => { assert.deepEqual(written, specs); }); + it("starts both remote writes together after mkdir and caches the bundle by path", async () => { + const sourceBundle = fixtureBundle(); + process.env.SANDBOX_AGENT_RELAY_MCP_BUNDLE = sourceBundle; + const started: string[] = []; + const releases: Array<() => void> = []; + const sandbox = { + mkdirFs: async () => { + started.push("mkdir"); + }, + writeFsFile: ({ path }: { path: string }, body: string) => { + started.push(`${path}:${body}`); + return new Promise((resolve) => releases.push(resolve)); + }, + }; + + const first = uploadToolMcpAssets(sandbox, "/first", specs); + await Promise.resolve(); + assert.equal(started[0], "mkdir"); + assert.equal( + started.filter((entry) => entry.startsWith("/first/")).length, + 2, + "both writes start before either remote write settles", + ); + releases.splice(0).forEach((release) => release()); + await first; + + writeFileSync(sourceBundle, "// changed after first acquire", "utf-8"); + const second = uploadToolMcpAssets(sandbox, "/second", specs); + await Promise.resolve(); + const secondBundleWrite = started.find((entry) => + entry.startsWith("/second/tool-mcp-stdio.js:"), + ); + assert.equal( + secondBundleWrite, + "/second/tool-mcp-stdio.js:// shim bundle", + "the static bundle is read once per resolved path", + ); + releases.splice(0).forEach((release) => release()); + await second; + }); + it("throws (fail loud) when the bundle is missing — never silently drops tools", async () => { process.env.SANDBOX_AGENT_RELAY_MCP_BUNDLE = join( tmpdir(), diff --git a/services/runner/tests/unit/tool-mcp-stdio.test.ts b/services/runner/tests/unit/tool-mcp-stdio.test.ts index 4134afb2bb..07f55843db 100644 --- a/services/runner/tests/unit/tool-mcp-stdio.test.ts +++ b/services/runner/tests/unit/tool-mcp-stdio.test.ts @@ -186,6 +186,28 @@ describe("tool-mcp-stdio handler", () => { assert.equal(res, undefined); }); + it("ping -> an empty successful result", async () => { + const res: any = await handleToolMcpMessage( + { jsonrpc: "2.0", id: 7, method: "ping" }, + specs, + tempDir("agenta-tool-mcp-"), + ); + assert.deepEqual(res, { jsonrpc: "2.0", id: 7, result: {} }); + }); + + it("a structurally invalid message -> -32600", async () => { + const res: any = await handleToolMcpMessage( + { jsonrpc: "2.0", id: 8, params: {} }, + specs, + tempDir("agenta-tool-mcp-"), + ); + assert.deepEqual(res, { + jsonrpc: "2.0", + id: 8, + error: { code: -32600, message: "invalid request" }, + }); + }); + it("tools/list advertises only public fields and honors the snake-case input_schema", async () => { const res: any = await handleToolMcpMessage( { jsonrpc: "2.0", id: 2, method: "tools/list" }, @@ -453,7 +475,7 @@ describe("stdio loop", () => { `${JSON.stringify({ jsonrpc: "2.0", method: "notifications/initialized" })}\n`, ); input.write("\n"); // blank line: ignored - input.write("{not json}\n"); // parse error: logged to stderr, nothing on stdout + input.write("{not json}\n"); // parse error: logged and answered with id:null input.write( `${JSON.stringify({ jsonrpc: "2.0", id: 2, method: "tools/list" })}\n`, ); @@ -476,14 +498,15 @@ describe("stdio loop", () => { ); return JSON.parse(chunk); }); - // 3 responses (initialize, tools/list, unknown-tool error); the notification, the blank - // line, and the parse error produced nothing on stdout. + // 4 responses (initialize, parse error, tools/list, unknown-tool error); the notification + // and blank line produce nothing on stdout. assert.deepEqual( responses.map((r) => r.id), - [1, 2, 3], + [1, null, 2, 3], ); for (const response of responses) assert.equal(response.jsonrpc, "2.0"); - assert.equal(responses[2].error.code, -32602); + assert.equal(responses[1].error.code, -32700); + assert.equal(responses[3].error.code, -32602); }); it("keeps reading while a tools/call is in flight (interleaved responses stay valid)", async () => { From fff45dc032e6affc44f53f82043d785b41c0af30 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sun, 12 Jul 2026 20:46:57 +0200 Subject: [PATCH 2/2] test(runner): cover omitted Daytona client tools --- .../tests/unit/sandbox-agent-run-plan.test.ts | 48 ++++++------------- 1 file changed, 15 insertions(+), 33 deletions(-) diff --git a/services/runner/tests/unit/sandbox-agent-run-plan.test.ts b/services/runner/tests/unit/sandbox-agent-run-plan.test.ts index daf3c90dae..cb2bb516ef 100644 --- a/services/runner/tests/unit/sandbox-agent-run-plan.test.ts +++ b/services/runner/tests/unit/sandbox-agent-run-plan.test.ts @@ -717,13 +717,7 @@ describe("buildRunPlan", () => { assert.equal(result.ok, true); }); - it("refuses claude x daytona x client tools (no pause path through the shim yet)", () => { - // A client tool is browser-fulfilled across a turn boundary: on local Claude the - // loopback channel pauses the call, but the in-sandbox stdio shim cannot — the relay - // loop parks a client call and writes no response file, so the shim would hang until - // the relay timeout and read as a broken tool. Refuse loud (never silently drop), - // even when executable tools ride along. - let created = false; + it("allows claude x daytona x mixed tools and keeps only executable shim specs", () => { const result = buildRunPlan( { harness: "claude", @@ -735,30 +729,23 @@ describe("buildRunPlan", () => { ], } as AgentRunRequest, { - createDaytonaCwd: () => { - created = true; - return "/home/sandbox/agenta-fixed"; - }, + createDaytonaCwd: () => "/home/sandbox/agenta-fixed", }, ); - assert.equal(result.ok, false); - if (result.ok) return; - assert.match(result.error, /Client tools are not supported/); - assert.match(result.error, /browser round-trip/); - assert.match( - result.error, - /docs\/design\/agent-workflows\/projects\/in-sandbox-tool-mcp\//, + assert.equal(result.ok, true); + if (!result.ok) return; + assert.deepEqual( + result.plan.toolSpecs.map((tool) => tool.name), + ["server_tool", "request_connection"], ); - assert.equal( - created, - false, - "fails before any cwd is created (up-front gate)", + assert.deepEqual( + result.plan.executableToolSpecs.map((tool) => tool.name), + ["server_tool"], ); }); - it("refuses claude x daytona x client-only tools directly", () => { - let created = false; + it("allows claude x daytona x client-only tools with no shim specs", () => { const result = buildRunPlan( { harness: "claude", @@ -767,18 +754,13 @@ describe("buildRunPlan", () => { customTools: [{ name: "request_connection", kind: "client" }], } as AgentRunRequest, { - createDaytonaCwd: () => { - created = true; - return "/home/sandbox/agenta-fixed"; - }, + createDaytonaCwd: () => "/home/sandbox/agenta-fixed", }, ); - assert.equal(result.ok, false); - if (result.ok) return; - assert.match(result.error, /Client tools are not supported/); - assert.match(result.error, /browser round-trip/); - assert.equal(created, false, "fails before any cwd is created"); + assert.equal(result.ok, true); + if (!result.ok) return; + assert.deepEqual(result.plan.executableToolSpecs, []); }); it("allows pi x daytona x client-only tools (Pi's extension + file relay deliver them)", () => {