From 5dfc2f5719273e5feaf594f77b8a561235d42e12 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 4 Jul 2026 19:22:31 +0200 Subject: [PATCH 1/8] feat(runner): add the {kind:"connect"} RenderHint for client connect widgets TS-only RenderHint member a client tool (request_connection) stamps so the frontend renders the OAuth/API-key connect dialog when the call pauses. wire.py does not pin RenderHint, so no wire change. Claude-Session: https://claude.ai/code/session_01HhBEUFbXETNjYdcz71AGrT --- services/runner/src/protocol.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/services/runner/src/protocol.ts b/services/runner/src/protocol.ts index 4dbe579ab5..3d189ba756 100644 --- a/services/runner/src/protocol.ts +++ b/services/runner/src/protocol.ts @@ -293,7 +293,12 @@ export interface HarnessCapabilities { export type RenderHint = | { kind: "component"; component: string } | { kind: "source"; runtime: "react" | "html"; source: string | string[] } - | { kind: "spec"; schema: string }; + | { kind: "spec"; schema: string } + // `connect` requests the built-in connect widget: a `client` tool (e.g. `request_connection`) + // stamps it so the frontend renders the OAuth/API-key connect dialog when the tool pauses. No + // payload — the widget is fully described by the paused call's tool name + input. `wire.py` does + // not pin RenderHint (render rides as an opaque dict), so this member is TS-only. + | { kind: "connect" }; export type AgentEvent = | { type: "message"; text: string } From 6383b90ab65097162f213ccb946857791bd65a5e Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 4 Jul 2026 19:25:07 +0200 Subject: [PATCH 2/8] refactor(runner): single spec-schema module + fix empty-schema advertisement Collapse the byte-identical camel/snake input_schema accessor + required-field walk (dispatch.ts, relay.ts, extensions/agenta.ts) into tools/spec-schema.ts. Rename PublicToolSpec -> AdvertisedToolSpec (advertisedToolSpecs) to say what it is: the advertisement shape, client tools included. Fix: the internal MCP channel's tools/list read only camelCase s.inputSchema, so every snake-case platform-catalog tool (request_connection, commit_revision) advertised an EMPTY schema to Claude. It now reads through specInputSchema. Claude-Session: https://claude.ai/code/session_01HhBEUFbXETNjYdcz71AGrT --- .../src/engines/sandbox_agent/pi-assets.ts | 4 +- services/runner/src/extensions/agenta.ts | 23 +-- services/runner/src/tools/dispatch.ts | 59 +------- services/runner/src/tools/public-spec.ts | 22 +-- services/runner/src/tools/relay.ts | 59 +------- services/runner/src/tools/spec-schema.ts | 89 ++++++++++++ services/runner/src/tools/tool-mcp-http.ts | 8 +- .../runner/tests/unit/spec-schema.test.ts | 134 ++++++++++++++++++ .../runner/tests/unit/tool-bridge.test.ts | 35 +++++ 9 files changed, 282 insertions(+), 151 deletions(-) create mode 100644 services/runner/src/tools/spec-schema.ts create mode 100644 services/runner/tests/unit/spec-schema.test.ts diff --git a/services/runner/src/engines/sandbox_agent/pi-assets.ts b/services/runner/src/engines/sandbox_agent/pi-assets.ts index 282aca706e..dc0563156c 100644 --- a/services/runner/src/engines/sandbox_agent/pi-assets.ts +++ b/services/runner/src/engines/sandbox_agent/pi-assets.ts @@ -13,7 +13,7 @@ import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import type { AgentRunRequest, ResolvedToolSpec } from "../../protocol.ts"; -import { publicToolSpecs } from "../../tools/public-spec.ts"; +import { advertisedToolSpecs } from "../../tools/public-spec.ts"; import type { MaterializedSkill } from "../skills.ts"; import { PKG_ROOT } from "./daemon.ts"; import type { RunPlan } from "./run-plan.ts"; @@ -51,7 +51,7 @@ export function buildPiExtensionEnv( if (telemetry && opts.skills && opts.skills.length > 0) env.AGENTA_AGENT_SKILLS_LOADED = JSON.stringify(opts.skills); - const specs = publicToolSpecs( + const specs = advertisedToolSpecs( (request.customTools as ResolvedToolSpec[]) ?? [], ); if (specs.length && opts.relayDir) { diff --git a/services/runner/src/extensions/agenta.ts b/services/runner/src/extensions/agenta.ts index b04e7361e1..d077bf543f 100644 --- a/services/runner/src/extensions/agenta.ts +++ b/services/runner/src/extensions/agenta.ts @@ -30,6 +30,7 @@ import type { ExtensionAPI } from "@earendil-works/pi-coding-agent"; import { createAgentaOtel } from "../tracing/otel.ts"; import type { ResolvedToolSpec } from "../protocol.ts"; import { EMPTY_OBJECT_SCHEMA } from "../tools/callback.ts"; +import { requiredFields, specInputSchema } from "../tools/spec-schema.ts"; /** Pull the Authorization value out of an OTEL_EXPORTER_OTLP_HEADERS key=value list. */ function authorizationFromOtlpHeaders(raw?: string): string | undefined { @@ -49,28 +50,6 @@ function log(message: string): void { process.stderr.write(`[agenta-pi-ext] ${message}\n`); } -function objectSchema(schema: unknown): Record | undefined { - return schema && typeof schema === "object" && !Array.isArray(schema) - ? (schema as Record) - : undefined; -} - -function requiredFields(schema: unknown): string[] { - const object = objectSchema(schema); - const required = object?.required; - return Array.isArray(required) - ? required.filter((field): field is string => typeof field === "string") - : []; -} - -function specInputSchema(spec: ResolvedToolSpec): Record | null | undefined { - return ( - spec.inputSchema ?? - (spec as ResolvedToolSpec & { input_schema?: Record | null }) - .input_schema - ); -} - function promptSnippet(spec: ResolvedToolSpec): string { return spec.description ?? `Call ${spec.name}`; } diff --git a/services/runner/src/tools/dispatch.ts b/services/runner/src/tools/dispatch.ts index 97303f81c3..56355e003a 100644 --- a/services/runner/src/tools/dispatch.ts +++ b/services/runner/src/tools/dispatch.ts @@ -24,6 +24,7 @@ import { existsSync, mkdirSync, readFileSync, unlinkSync, writeFileSync } from " import type { ResolvedToolSpec } from "../protocol.ts"; import { callAgentaTool } from "./callback.ts"; import { runCodeTool } from "./code.ts"; +import { assertRequiredArguments } from "./spec-schema.ts"; import { RELAY_POLL_MS, RELAY_REQ_SUFFIX, @@ -48,64 +49,6 @@ export interface RunResolvedToolOpts { signal?: AbortSignal; } -function objectSchema(schema: unknown): Record | undefined { - return schema && typeof schema === "object" && !Array.isArray(schema) - ? (schema as Record) - : undefined; -} - -function requiredFields(schema: unknown): string[] { - const object = objectSchema(schema); - const required = object?.required; - return Array.isArray(required) - ? required.filter((field): field is string => typeof field === "string") - : []; -} - -function specInputSchema(spec: ResolvedToolSpec): Record | null | undefined { - return ( - spec.inputSchema ?? - (spec as ResolvedToolSpec & { input_schema?: Record | null }) - .input_schema - ); -} - -function missingRequiredFields( - schema: unknown, - value: unknown, - path: string[] = [], -): string[] { - const object = objectSchema(schema); - if (!object) return []; - - const missing: string[] = []; - const required = requiredFields(object); - const record = objectSchema(value); - for (const field of required) { - if (!record || record[field] === undefined || record[field] === null) { - missing.push([...path, field].join(".")); - } - } - - const properties = objectSchema(object.properties); - if (!properties || !record) return missing; - for (const [field, childSchema] of Object.entries(properties)) { - if (record[field] !== undefined && record[field] !== null) { - missing.push(...missingRequiredFields(childSchema, record[field], [...path, field])); - } - } - return missing; -} - -function assertRequiredArguments(spec: ResolvedToolSpec, params: unknown): void { - const missing = missingRequiredFields(specInputSchema(spec), params); - if (missing.length === 0) return; - throw new Error( - `Tool '${spec.name}' missing required argument(s): ${missing.join(", ")}. ` + - "Retry the tool call with those argument fields populated.", - ); -} - /** * Daytona tool call: the in-sandbox process can't reach Agenta, so write the request to a * file the runner watches and poll for the response it writes back (see tools/relay.ts). diff --git a/services/runner/src/tools/public-spec.ts b/services/runner/src/tools/public-spec.ts index b8c38b1313..372edb9040 100644 --- a/services/runner/src/tools/public-spec.ts +++ b/services/runner/src/tools/public-spec.ts @@ -6,8 +6,9 @@ * shape so the model can choose a tool; every execution is relayed back to the runner. */ import type { ResolvedToolSpec } from "../protocol.ts"; +import { specInputSchema } from "./spec-schema.ts"; -export interface PublicToolSpec { +export interface AdvertisedToolSpec { name: string; description?: string; inputSchema?: Record | null; @@ -20,21 +21,22 @@ export function executableToolSpecs(specs: ResolvedToolSpec[]): ResolvedToolSpec return specs.filter((spec) => (spec.kind ?? "callback") !== "client"); } -export function publicToolSpec(spec: ResolvedToolSpec): PublicToolSpec { - const inputSchema = - spec.inputSchema ?? - (spec as ResolvedToolSpec & { input_schema?: Record | null }) - .input_schema; - const out: PublicToolSpec = { +export function advertisedToolSpec(spec: ResolvedToolSpec): AdvertisedToolSpec { + const out: AdvertisedToolSpec = { name: spec.name, description: spec.description, - inputSchema, + inputSchema: specInputSchema(spec), }; if (spec.kind) out.kind = spec.kind; if (spec.render) out.render = spec.render; return out; } -export function publicToolSpecs(specs: ResolvedToolSpec[]): PublicToolSpec[] { - return specs.map(publicToolSpec); +/** + * The advertisement shape for EVERY advertisable spec — including `client` tools, which the + * model must SEE (e.g. `request_connection`) even though the browser, not the runner, fulfils + * them. (Contrast `executableToolSpecs`, which is the gatekeeper for the execute paths.) + */ +export function advertisedToolSpecs(specs: ResolvedToolSpec[]): AdvertisedToolSpec[] { + return specs.map(advertisedToolSpec); } diff --git a/services/runner/src/tools/relay.ts b/services/runner/src/tools/relay.ts index 7947a3bd81..611400e8e5 100644 --- a/services/runner/src/tools/relay.ts +++ b/services/runner/src/tools/relay.ts @@ -33,6 +33,7 @@ import type { } from "../protocol.ts"; import type { GateDescriptor, Verdict } from "../permission-plan.ts"; import type { ClientToolOutcome } from "../responder.ts"; +import { assertRequiredArguments } from "./spec-schema.ts"; export const RELAY_REQ_SUFFIX = ".req.json"; export const RELAY_RES_SUFFIX = ".res.json"; @@ -79,64 +80,6 @@ export interface ClientToolRelay { } const PAUSED = Symbol("paused"); -function objectSchema(schema: unknown): Record | undefined { - return schema && typeof schema === "object" && !Array.isArray(schema) - ? (schema as Record) - : undefined; -} - -function requiredFields(schema: unknown): string[] { - const object = objectSchema(schema); - const required = object?.required; - return Array.isArray(required) - ? required.filter((field): field is string => typeof field === "string") - : []; -} - -function specInputSchema(spec: ResolvedToolSpec): Record | null | undefined { - return ( - spec.inputSchema ?? - (spec as ResolvedToolSpec & { input_schema?: Record | null }) - .input_schema - ); -} - -function missingRequiredFields( - schema: unknown, - value: unknown, - path: string[] = [], -): string[] { - const object = objectSchema(schema); - if (!object) return []; - - const missing: string[] = []; - const required = requiredFields(object); - const record = objectSchema(value); - for (const field of required) { - if (!record || record[field] === undefined || record[field] === null) { - missing.push([...path, field].join(".")); - } - } - - const properties = objectSchema(object.properties); - if (!properties || !record) return missing; - for (const [field, childSchema] of Object.entries(properties)) { - if (record[field] !== undefined && record[field] !== null) { - missing.push(...missingRequiredFields(childSchema, record[field], [...path, field])); - } - } - return missing; -} - -function assertRequiredArguments(spec: ResolvedToolSpec, params: unknown): void { - const missing = missingRequiredFields(specInputSchema(spec), params); - if (missing.length === 0) return; - throw new Error( - `Tool '${spec.name}' missing required argument(s): ${missing.join(", ")}. ` + - "Retry the tool call with those argument fields populated.", - ); -} - /** Make a tool-call id safe to use as a filename (and bounded). */ export function sanitizeRelayId(id: string): string { return id.replace(/[^A-Za-z0-9_-]/g, "_").slice(0, 120) || "tool"; diff --git a/services/runner/src/tools/spec-schema.ts b/services/runner/src/tools/spec-schema.ts new file mode 100644 index 0000000000..1ca00edbd5 --- /dev/null +++ b/services/runner/src/tools/spec-schema.ts @@ -0,0 +1,89 @@ +/** + * Shared JSON-Schema helpers for resolved tool specs. + * + * The platform tool catalog emits snake_case `input_schema` (`static_catalog.py`, + * `op_catalog.py`), while the wire type `ResolvedToolSpec.inputSchema` is camelCase + * (`protocol.ts`). `customTools` arrives unnormalized, so every place that reads a spec's + * schema or validates a call's arguments used to re-implement the same camel/snake accessor and + * the same required-field walk. They lived as byte-identical copies in `dispatch.ts`, `relay.ts`, + * and `extensions/agenta.ts`. This module owns them ONCE so a fix (e.g. how a required field is + * detected, or that snake_case `input_schema` must be read) is a one-line edit, not several. + * + * `specInputSchema` is the single accessor for a spec's input schema — use it instead of reading + * `spec.inputSchema` directly, or a snake_case `input_schema` spec advertises an EMPTY schema to + * the model (the live bug that hit every platform-catalog tool over the Claude MCP channel). + */ +import type { ResolvedToolSpec } from "../protocol.ts"; + +/** A value usable as a JSON-Schema object node (`{type:"object", ...}`), or undefined. */ +export function objectSchema(schema: unknown): Record | undefined { + return schema && typeof schema === "object" && !Array.isArray(schema) + ? (schema as Record) + : undefined; +} + +/** The `required` field names declared on a JSON-Schema object node. */ +export function requiredFields(schema: unknown): string[] { + const object = objectSchema(schema); + const required = object?.required; + return Array.isArray(required) + ? required.filter((field): field is string => typeof field === "string") + : []; +} + +/** + * A spec's input schema, reading camelCase `inputSchema` first and falling back to snake_case + * `input_schema` (the un-normalized platform-catalog shape). THE single accessor — never read + * `spec.inputSchema` directly. + */ +export function specInputSchema( + spec: ResolvedToolSpec, +): Record | null | undefined { + return ( + spec.inputSchema ?? + (spec as ResolvedToolSpec & { input_schema?: Record | null }) + .input_schema + ); +} + +/** Dotted paths of every required field the value is missing, walking nested objects. */ +export function missingRequiredFields( + schema: unknown, + value: unknown, + path: string[] = [], +): string[] { + const object = objectSchema(schema); + if (!object) return []; + + const missing: string[] = []; + const required = requiredFields(object); + const record = objectSchema(value); + for (const field of required) { + if (!record || record[field] === undefined || record[field] === null) { + missing.push([...path, field].join(".")); + } + } + + const properties = objectSchema(object.properties); + if (!properties || !record) return missing; + for (const [field, childSchema] of Object.entries(properties)) { + if (record[field] !== undefined && record[field] !== null) { + missing.push(...missingRequiredFields(childSchema, record[field], [...path, field])); + } + } + return missing; +} + +/** + * Throw a model-actionable error if a tool call is missing any required argument. Every call site + * turns the throw into a tool-error result so the model retries with the fields populated, rather + * than the harness silently dispatching an under-specified call. + */ +export function assertRequiredArguments(spec: ResolvedToolSpec, params: unknown): void { + const missing = missingRequiredFields(specInputSchema(spec), params); + if (missing.length === 0) return; + throw new Error( + `Tool '${spec.name}' missing required argument(s): ${missing.join(", ")}. ` + + "Retry the tool call with those argument fields populated.", + ); +} diff --git a/services/runner/src/tools/tool-mcp-http.ts b/services/runner/src/tools/tool-mcp-http.ts index 215ef92e58..10d0ac3f81 100644 --- a/services/runner/src/tools/tool-mcp-http.ts +++ b/services/runner/src/tools/tool-mcp-http.ts @@ -40,6 +40,7 @@ import type { AddressInfo } from "node:net"; import type { ResolvedToolSpec } from "../protocol.ts"; import { EMPTY_OBJECT_SCHEMA } from "./callback.ts"; import { runResolvedTool } from "./dispatch.ts"; +import { specInputSchema } from "./spec-schema.ts"; type Log = (message: string) => void; @@ -98,8 +99,13 @@ async function handle( .map((s) => ({ name: s.name, description: s.description ?? s.name, + // Read via the shared accessor (camelCase `inputSchema` OR snake-case + // `input_schema`). Reading `s.inputSchema` alone advertised an EMPTY schema for every + // snake-case platform-catalog tool (`request_connection`, `commit_revision`), so + // Claude received no argument schema — a live bug, not just a client-tool one. inputSchema: - (s.inputSchema as Record) ?? EMPTY_OBJECT_SCHEMA, + (specInputSchema(s) as Record) ?? + EMPTY_OBJECT_SCHEMA, })), }, }; diff --git a/services/runner/tests/unit/spec-schema.test.ts b/services/runner/tests/unit/spec-schema.test.ts new file mode 100644 index 0000000000..f09cfbae0b --- /dev/null +++ b/services/runner/tests/unit/spec-schema.test.ts @@ -0,0 +1,134 @@ +/** + * Unit tests for the shared spec-schema helpers (tools/spec-schema.ts). + * + * These back the single source of truth for reading a tool's input schema (camelCase + * `inputSchema` OR snake-case `input_schema`) and validating required arguments, after the + * byte-identical copies in dispatch.ts / relay.ts / extensions/agenta.ts were collapsed here. + * + * Run: pnpm test (or: pnpm exec vitest run tests/unit/spec-schema.test.ts) + */ +import { describe, it } from "vitest"; +import assert from "node:assert/strict"; + +import { + assertRequiredArguments, + missingRequiredFields, + requiredFields, + specInputSchema, +} from "../../src/tools/spec-schema.ts"; +import type { ResolvedToolSpec } from "../../src/protocol.ts"; + +describe("specInputSchema", () => { + it("reads camelCase inputSchema", () => { + const spec = { + name: "t", + inputSchema: { type: "object", properties: { a: { type: "string" } } }, + } as ResolvedToolSpec; + assert.deepEqual(specInputSchema(spec), { + type: "object", + properties: { a: { type: "string" } }, + }); + }); + + it("falls back to snake-case input_schema (un-normalized platform-catalog shape)", () => { + const spec = { + name: "request_connection", + kind: "client", + input_schema: { + type: "object", + required: ["integration"], + properties: { integration: { type: "string" } }, + }, + } as unknown as ResolvedToolSpec; + assert.deepEqual(specInputSchema(spec), { + type: "object", + required: ["integration"], + properties: { integration: { type: "string" } }, + }); + }); + + it("prefers camelCase when both are present", () => { + const spec = { + name: "t", + inputSchema: { type: "object", title: "camel" }, + input_schema: { type: "object", title: "snake" }, + } as unknown as ResolvedToolSpec; + assert.equal((specInputSchema(spec) as Record).title, "camel"); + }); +}); + +describe("requiredFields", () => { + it("returns declared required field names, ignoring non-string entries", () => { + assert.deepEqual( + requiredFields({ type: "object", required: ["a", 1, "b", null] }), + ["a", "b"], + ); + }); + + it("returns [] when required is absent or the schema is not an object node", () => { + assert.deepEqual(requiredFields({ type: "object" }), []); + assert.deepEqual(requiredFields(undefined), []); + assert.deepEqual(requiredFields([1, 2]), []); + }); +}); + +describe("missingRequiredFields", () => { + it("flags a missing top-level required field", () => { + const schema = { type: "object", required: ["a", "b"], properties: {} }; + assert.deepEqual(missingRequiredFields(schema, { a: 1 }), ["b"]); + }); + + it("treats null/undefined values as missing", () => { + const schema = { type: "object", required: ["a"] }; + assert.deepEqual(missingRequiredFields(schema, { a: null }), ["a"]); + assert.deepEqual(missingRequiredFields(schema, {}), ["a"]); + }); + + it("walks nested objects and reports dotted paths", () => { + const schema = { + type: "object", + required: ["outer"], + properties: { + outer: { + type: "object", + required: ["inner"], + properties: { inner: { type: "string" } }, + }, + }, + }; + // outer present but its required `inner` is missing -> dotted path. + assert.deepEqual(missingRequiredFields(schema, { outer: {} }), [ + "outer.inner", + ]); + // outer itself missing -> only the outer path (no descent). + assert.deepEqual(missingRequiredFields(schema, {}), ["outer"]); + // fully populated -> nothing missing. + assert.deepEqual( + missingRequiredFields(schema, { outer: { inner: "x" } }), + [], + ); + }); +}); + +describe("assertRequiredArguments", () => { + it("throws a model-actionable error naming the missing fields", () => { + const spec = { + name: "request_connection", + input_schema: { type: "object", required: ["integration"] }, + } as unknown as ResolvedToolSpec; + assert.throws( + () => assertRequiredArguments(spec, {}), + /missing required argument\(s\): integration/, + ); + }); + + it("does not throw when all required args are present (snake-case schema)", () => { + const spec = { + name: "request_connection", + input_schema: { type: "object", required: ["integration"] }, + } as unknown as ResolvedToolSpec; + assert.doesNotThrow(() => + assertRequiredArguments(spec, { integration: "slack" }), + ); + }); +}); diff --git a/services/runner/tests/unit/tool-bridge.test.ts b/services/runner/tests/unit/tool-bridge.test.ts index b970b2c399..8338fd16ec 100644 --- a/services/runner/tests/unit/tool-bridge.test.ts +++ b/services/runner/tests/unit/tool-bridge.test.ts @@ -194,6 +194,41 @@ describe("buildToolMcpServers (internal gateway-tool channel)", () => { ); }); + it("advertises a snake-case input_schema as a NON-empty schema (empty-schema regression)", async () => { + // Platform-catalog tools carry snake-case `input_schema`. The advertisement used to read + // only camelCase `s.inputSchema`, so Claude got EMPTY_OBJECT_SCHEMA and no argument schema. + const specs = [ + { + name: "commit_revision", + kind: "callback", + callRef: "platform.commit_revision", + input_schema: { + type: "object", + required: ["workflow_revision"], + properties: { workflow_revision: { type: "object" } }, + }, + }, + ] as unknown as ResolvedToolSpec[]; + const { servers } = await build(specs, relayDir); + const list = await rpc(servers[0].url, { + jsonrpc: "2.0", + id: 9, + method: "tools/list", + }); + const tool = list.result.tools[0]; + assert.equal(tool.name, "commit_revision"); + assert.deepEqual(tool.inputSchema, { + type: "object", + required: ["workflow_revision"], + properties: { workflow_revision: { type: "object" } }, + }); + assert.notDeepEqual( + tool.inputSchema, + { type: "object", properties: {} }, + "must not advertise the empty fallback schema", + ); + }); + it("routes tools/call through the relay dir (server-side execution)", async () => { const dir = mkdtempSync(join(tmpdir(), "agenta-tool-relay-")); try { From e69310fdd2b2787f2ca36770be094a3de942c257 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 4 Jul 2026 19:30:30 +0200 Subject: [PATCH 3/8] feat(runner): shared client-tool seam + FIFO client-output store New engines/sandbox_agent/client-tools.ts owns the client-tool pause once for both delivery paths: buildClientToolRelay (gate descriptor -> responder verdict -> latch/markPausedToolCall/emit/recordPendingInteraction), the single client_tool interaction payload (emitClientToolInteraction), and the ACP tool-call correlation index Claude's MCP delivery will use. The engine's inline onClientTool at startToolRelay now consumes the seam (behavior-preserving for Pi). responder.ts splits the replayed-conversation store: extractApprovalDecisions keeps ONLY {approved} envelopes; new extractClientToolOutputs collects raw browser outputs as a FIFO list per name+args key. Fixes two live bugs: a client output literally "allow"/"deny" collided with a permission decision in the shared map, and two identical client calls overwrote each other's stored output. Peek-vs-take semantics preserved (ACP gate peeks, relay consumes). Claude-Session: https://claude.ai/code/session_01HhBEUFbXETNjYdcz71AGrT --- services/runner/src/engines/sandbox_agent.ts | 73 ++--- .../src/engines/sandbox_agent/client-tools.ts | 225 +++++++++++++++ services/runner/src/responder.ts | 158 ++++++++--- .../runner/tests/unit/client-tools.test.ts | 256 ++++++++++++++++++ services/runner/tests/unit/responder.test.ts | 162 ++++++++++- 5 files changed, 772 insertions(+), 102 deletions(-) create mode 100644 services/runner/src/engines/sandbox_agent/client-tools.ts create mode 100644 services/runner/tests/unit/client-tools.test.ts diff --git a/services/runner/src/engines/sandbox_agent.ts b/services/runner/src/engines/sandbox_agent.ts index 580075e6b0..aac0f6f8f0 100644 --- a/services/runner/src/engines/sandbox_agent.ts +++ b/services/runner/src/engines/sandbox_agent.ts @@ -40,8 +40,10 @@ import { ApprovalResponder, ConversationDecisions, extractApprovalDecisions, + extractClientToolOutputs, type Responder, } from "../responder.ts"; +import { buildClientToolRelay } from "./sandbox_agent/client-tools.ts"; import { type AgentRunRequest, type AgentRunResult, @@ -73,7 +75,6 @@ import { decide, PendingApprovalLatch, permissionsFromRequest, - type GateDescriptor, } from "../permission-plan.ts"; import { attachPermissionResponder } from "./sandbox_agent/acp-interactions.ts"; import { @@ -692,7 +693,10 @@ export async function runSandboxAgent( `[HITL] resume state: decisions=${JSON.stringify([...storedDecisionMap.keys()])}`, ); } - const decisions = new ConversationDecisions(storedDecisionMap); + const decisions = new ConversationDecisions( + storedDecisionMap, + extractClientToolOutputs(request), + ); const latch = new PendingApprovalLatch(); const responder = deps.responderFactory?.(request) ?? @@ -773,6 +777,17 @@ export async function runSandboxAgent( }, }); + // The ONE client-tool seam both delivery paths share: the Pi file relay (below) consumes it + // directly. Built here because it needs the responder, the otel run, and the pause plumbing. + const clientToolRelay = buildClientToolRelay({ + responder, + run, + latch, + pause, + recordPendingInteraction, + log: logger, + }); + if (plan.useToolRelay) { toolRelay = (deps.startToolRelay ?? startToolRelay)( plan.isDaytona @@ -783,59 +798,7 @@ export async function runSandboxAgent( request.toolCallback as ToolCallbackContext | undefined, relayPermissions, request.runContext, - { - onClientTool: async ({ id, toolCallId, toolName, input, spec }) => { - const gate: GateDescriptor = { - executor: "client", - toolName: spec.name, - specPermission: spec.permission, - readOnlyHint: spec.readOnly, - args: input, - }; - const verdict = await responder.onClientTool( - { - id, - toolCallId, - gate, - raw: { spec }, - }, - { consume: true }, - ); - if (process.env.AGENTA_RUNNER_DEBUG_TOOLS) { - logger( - `[client-tool] ${toolName} id=${toolCallId} kind=${spec.kind} ` + - `decision=${JSON.stringify(verdict).slice(0, 200)}`, - ); - } - if (verdict.kind === "deny") return "deny"; - if (verdict.kind === "fulfilled") return { output: verdict.output }; - if (latch.tryAcquire()) { - pause.markPausedToolCall(toolCallId); - run.emitEvent({ - type: "interaction_request", - id, - kind: "client_tool", - payload: { - toolCallId, - toolName, - input, - render: spec.render, - toolCall: { - id: toolCallId, - toolCallId, - name: toolName, - rawInput: input, - input, - kind: spec.kind, - }, - }, - }); - recordPendingInteraction(id, toolName, input, "client_tool"); - } - return "pendingApproval"; - }, - onPause: () => pause.pause(), - }, + clientToolRelay, ); } diff --git a/services/runner/src/engines/sandbox_agent/client-tools.ts b/services/runner/src/engines/sandbox_agent/client-tools.ts new file mode 100644 index 0000000000..0229d4c99d --- /dev/null +++ b/services/runner/src/engines/sandbox_agent/client-tools.ts @@ -0,0 +1,225 @@ +/** + * The shared client-tool seam. + * + * A `client` tool (e.g. `request_connection`) is browser-fulfilled across a turn boundary: the + * model calls it, the runner emits a `client_tool` interaction_request so the frontend renders a + * widget, the turn ends `paused`, and the next turn resumes with the browser's result. Two + * delivery channels reach this same pause: + * - Pi loads tools through its bundled extension and pauses via the runner's file relay + * (`tools/relay.ts` -> `startToolRelay`). + * - Claude takes tools over the internal loopback MCP server and pauses inside its + * `tools/call` handler (`tools/tool-mcp-http.ts`). + * + * Both consume the `ClientToolRelay` built here, so the pause decision (the responder's verdict + * ladder), the single `client_tool` payload shape, the pause-latch bookkeeping, and the + * ACP-tool-call correlation live in ONE place instead of being re-derived per path. + */ +import type { AgentEvent, RenderHint } from "../../protocol.ts"; +import type { GateDescriptor } from "../../permission-plan.ts"; +import { approvedCallKey, type Responder } from "../../responder.ts"; +import type { + ClientToolRelay, + ClientToolRelayRequest, +} from "../../tools/relay.ts"; + +type EmitRun = { emitEvent: (event: AgentEvent) => void }; + +/** + * Correlates an MCP `tools/call` (which carries only name + arguments) to the real ACP + * tool-call id Claude surfaced on the event stream, so the paused `client_tool` interaction + * attaches to Claude's actual tool-call bubble (and `markPausedToolCall` suppresses that + * bubble's late teardown frames, the F-024 lineage). Populated from `session.onEvent` + * `tool_call` updates; the MCP-minted id / name+args is the cold-replay fallback when the + * stream had no matching call. Best-effort and first-write-wins (a later identical call never + * re-homes an id). + */ +export interface ToolCallCorrelationIndex { + record(update: unknown): void; + lookup(toolName: string | undefined, input: unknown): string | undefined; +} + +export function createToolCallCorrelationIndex(): ToolCallCorrelationIndex { + const byArgsKey = new Map(); + const byName = new Map(); + return { + record(update) { + const u = update as + | { + sessionUpdate?: unknown; + toolCallId?: unknown; + title?: unknown; + kind?: unknown; + rawInput?: unknown; + } + | undefined; + if (!u || u.sessionUpdate !== "tool_call") return; + const toolCallId = + typeof u.toolCallId === "string" && u.toolCallId ? u.toolCallId : undefined; + if (!toolCallId) return; + const name = + typeof u.title === "string" && u.title + ? u.title + : typeof u.kind === "string" && u.kind + ? u.kind + : undefined; + const argsKey = approvedCallKey(name, u.rawInput); + if (argsKey && !byArgsKey.has(argsKey)) byArgsKey.set(argsKey, toolCallId); + if (name && !byName.has(name)) byName.set(name, toolCallId); + }, + lookup(toolName, input) { + const argsKey = approvedCallKey(toolName, input); + if (argsKey) { + const hit = byArgsKey.get(argsKey); + if (hit) return hit; + } + if (toolName) { + const hit = byName.get(toolName); + if (hit) return hit; + } + return undefined; + }, + }; +} + +export interface ClientToolInteractionParams { + /** The interaction id (the FE matches a reply by it). */ + id: string; + /** The runner/relay-minted tool-call id; overridden by the correlated ACP id when one exists. */ + toolCallId?: string; + toolName?: string; + input?: unknown; + render?: RenderHint; +} + +/** + * THE single definition of the `interaction_request kind=client_tool` payload. Emits both the + * top-level fields and a synthesized `toolCall` sub-object the Vercel egress reads (it tolerates + * either), and substitutes the correlated ACP tool-call id when the index has one. + */ +export function emitClientToolInteraction( + run: EmitRun, + params: ClientToolInteractionParams, + toolCallIndex?: ToolCallCorrelationIndex, +): void { + const correlatedId = + toolCallIndex?.lookup(params.toolName, params.input) ?? params.toolCallId; + run.emitEvent({ + type: "interaction_request", + id: params.id, + kind: "client_tool", + payload: { + toolCallId: correlatedId, + toolName: params.toolName, + input: params.input, + render: params.render, + toolCall: { + id: correlatedId, + toolCallId: correlatedId, + name: params.toolName, + rawInput: params.input, + input: params.input, + kind: "client", + }, + }, + }); +} + +/** The one-pause-per-turn latch surface the seam needs (see `PendingApprovalLatch`). */ +interface LatchLike { + tryAcquire(): boolean; +} + +/** The pause-controller surface the seam needs (see `PendingApprovalPauseController`). */ +interface PauseLike { + markPausedToolCall(toolCallId: string): void; + pause(): void; +} + +export interface BuildClientToolRelayInput { + responder: Responder; + run: EmitRun; + /** One pause per turn: only the first pending gate emits its interaction and pauses. */ + latch: LatchLike; + /** The turn-ender: `pause()` cancels the prompt; `markPausedToolCall` suppresses late frames. */ + pause: PauseLike; + /** Seeds the durable interactions plane for the pending call (fire-and-forget). */ + recordPendingInteraction: ( + token: string, + toolName: string | undefined, + toolArgs: unknown, + kind: "user_approval" | "client_tool", + ) => void; + /** Claude only: maps the call to its real ACP tool-call id. Omit for Pi (relay id is exact). */ + toolCallIndex?: ToolCallCorrelationIndex; + log?: (message: string) => void; +} + +/** + * Build the `ClientToolRelay` both delivery paths use. `onClientTool` asks the responder + * (consuming a stored browser output when one exists) and, on `pendingApproval`, emits the + * `client_tool` interaction under the latch; `onPause` ends the turn. The consumer (relay loop + * or MCP handler) calls `onClientTool` then, on a `pendingApproval` outcome, `onPause` — exactly + * the previous inline engine behavior, so Pi is unchanged. + */ +export function buildClientToolRelay({ + responder, + run, + latch, + pause, + recordPendingInteraction, + toolCallIndex, + log = () => {}, +}: BuildClientToolRelayInput): ClientToolRelay { + return { + onClientTool: async (request: ClientToolRelayRequest) => { + const gate: GateDescriptor = { + executor: "client", + toolName: request.spec.name, + specPermission: request.spec.permission, + readOnlyHint: request.spec.readOnly, + args: request.input, + }; + const verdict = await responder.onClientTool( + { + id: request.id, + toolCallId: request.toolCallId, + gate, + raw: { spec: request.spec }, + }, + { consume: true }, + ); + if (process.env.AGENTA_RUNNER_DEBUG_TOOLS) { + log( + `[client-tool] ${request.toolName} id=${request.toolCallId} kind=${request.spec.kind} ` + + `decision=${JSON.stringify(verdict).slice(0, 200)}`, + ); + } + if (verdict.kind === "deny") return "deny"; + if (verdict.kind === "fulfilled") return { output: verdict.output }; + // Pending: pause the browser-fulfilled call. Correlate to the real ACP tool-call id when + // an index is wired (Claude MCP) so the widget attaches to Claude's tool bubble and its + // late teardown frames are suppressed; Pi's relay-minted id is already exact. + const correlatedId = + toolCallIndex?.lookup(request.toolName, request.input) ?? + request.toolCallId; + if (latch.tryAcquire()) { + pause.markPausedToolCall(correlatedId); + emitClientToolInteraction(run, { + id: request.id, + toolCallId: correlatedId, + toolName: request.toolName, + input: request.input, + render: request.spec.render, + }); + recordPendingInteraction( + request.id, + request.toolName, + request.input, + "client_tool", + ); + } + return "pendingApproval"; + }, + onPause: () => pause.pause(), + }; +} diff --git a/services/runner/src/responder.ts b/services/runner/src/responder.ts index 01103ff487..c26c85c472 100644 --- a/services/runner/src/responder.ts +++ b/services/runner/src/responder.ts @@ -108,15 +108,36 @@ function canonicalJson(value: unknown): string { } /** - * Consume-once store of approvals/denials carried in the replayed conversation. + * Client-tool browser outputs the user already produced on a prior turn, keyed by + * `approvedCallKey(name, args)` (the cold-replay anchor), with a FIFO LIST per key. This store + * is SEPARATE from the approval-decision map for two reasons Codex flagged: + * + * 1. No allow/deny coercion. A permission reply is `{approved}` -> `"allow"`/`"deny"`; a client + * output is the raw browser result. Sharing one map meant a client output whose value was + * literally the string `"allow"`/`"deny"` collided with a permission decision. Here the value + * is stored verbatim and `onClientTool` never interprets it as a permission decision. + * 2. Duplicate calls. A single `Map.set` per key let two identical name+args calls overwrite + * each other. A FIFO list lets each identical call consume the next stored output in order. + */ +export type ClientToolOutputs = ReadonlyMap; + +/** + * Consume-once store of approvals/denials and client-tool outputs carried in the replayed + * conversation. * * Client-tool outputs are consume-once per fulfillment: the ACP gate only peeks to prove an * output exists, and the relay consumes when it actually serves that output to the tool child. - * Two identical client-tool calls in one conversation still share the stored output key, matching - * the pre-redesign behavior. + * Two identical client-tool calls in one conversation each resolve from the next stored output + * under the shared key (FIFO), so neither overwrites the other. */ export class ConversationDecisions implements StoredPermissionDecisions { - constructor(private readonly byKey: Map) {} + /** Per-key FIFO cursor: how many outputs under a key this conversation already consumed. */ + private readonly clientOutputCursor = new Map(); + + constructor( + private readonly byKey: Map, + private readonly clientOutputs: ClientToolOutputs = new Map(), + ) {} /** allow|deny for this exact call (name + canonical args), consumed on first take. */ take(gate: GateDescriptor): "allow" | "deny" | undefined { @@ -128,22 +149,31 @@ export class ConversationDecisions implements StoredPermissionDecisions { return value; } - /** A client-tool fulfillment output for this exact call, without consuming it. */ + /** The next FIFO client-tool output for this exact call, without consuming it. */ peekClientOutput(gate: GateDescriptor): { found: boolean; output?: unknown } { - const key = approvedCallKey(gate.toolName, gate.args); - if (!key || !this.byKey.has(key)) return { found: false }; - const value = this.byKey.get(key); - if (isPermissionDecision(value)) return { found: false }; - return { found: true, output: value }; + const entry = this.nextClientOutput(gate); + return entry ? { found: true, output: entry.output } : { found: false }; } - /** A client-tool fulfillment output for this exact call, consumed on first take. */ + /** The next FIFO client-tool output for this exact call, consumed on take. */ takeClientOutput(gate: GateDescriptor): { found: boolean; output?: unknown } { + const entry = this.nextClientOutput(gate); + if (!entry) return { found: false }; + this.clientOutputCursor.set(entry.key, entry.consumed + 1); + return { found: true, output: entry.output }; + } + + /** The next unconsumed output under this call's key, or undefined when exhausted/absent. */ + private nextClientOutput( + gate: GateDescriptor, + ): { key: string; consumed: number; output: unknown } | undefined { const key = approvedCallKey(gate.toolName, gate.args); - const output = this.peekClientOutput(gate); - if (!output.found || !key) return { found: false }; - this.byKey.delete(key); - return output; + if (!key) return undefined; + const list = this.clientOutputs.get(key); + if (!list || list.length === 0) return undefined; + const consumed = this.clientOutputCursor.get(key) ?? 0; + if (consumed >= list.length) return undefined; + return { key, consumed, output: list[consumed] }; } } @@ -204,40 +234,91 @@ export class ApprovalResponder implements Responder { * cold-replay anchor. The name/args are recovered from the matching `tool_call` block (same * `toolCallId`) the egress folds into the transcript. An unbindable approval envelope is * dropped; the gate re-raises and re-prompts, never guessed. + * + * ONLY approval-envelope (`{approved}`) results land here; a client-tool's raw browser output + * goes to the separate `extractClientToolOutputs` store (so a client output literally + * `"allow"`/`"deny"` can never be mis-read as a permission decision). */ export function extractApprovalDecisions( request: AgentRunRequest, ): Map { const decisions = new Map(); - const callShapeById = new Map(); + const callShapeById = buildCallShapeIndex(request); + for (const block of toolResultBlocks(request)) { + const decision = approvalDecisionOf(block); + if (decision === undefined) continue; + const argsKey = coldReplayKey(block, callShapeById); + if (argsKey) decisions.set(argsKey, decision); + } + return decisions; +} + +/** + * Build the client-tool output store from the inbound history: every NON-approval `tool_result` + * is a browser-fulfilled client-tool output. Keyed by the cold-replay anchor + * `approvedCallKey(name, args)`, with a FIFO LIST per key so two identical calls each resolve + * from the next stored output instead of one overwriting the other. The value is the raw + * output, never coerced. + * + * (A normal callback/code tool result also lands here, but is harmless: `onClientTool` only + * fires for `kind: "client"` tools, and a resolved callback tool is not re-called as a client + * pause.) + */ +export function extractClientToolOutputs( + request: AgentRunRequest, +): Map { + const outputs = new Map(); + const callShapeById = buildCallShapeIndex(request); + for (const block of toolResultBlocks(request)) { + if (approvalDecisionOf(block) !== undefined) continue; // an approval, not a client output + const argsKey = coldReplayKey(block, callShapeById); + if (!argsKey) continue; + const list = outputs.get(argsKey) ?? []; + list.push(block.output); + outputs.set(argsKey, list); + } + return outputs; +} + +/** Recover each tool call's name + args keyed by its id, so a reply that carries only the id + * (e.g. an `{approved}` envelope) can be bound to the cold-replay name+args anchor. */ +function buildCallShapeIndex( + request: AgentRunRequest, +): Map { + const index = new Map(); for (const message of request.messages ?? []) { const content = message?.content; if (!Array.isArray(content)) continue; for (const block of content) { if (block?.type === "tool_call" && block.toolCallId) { - callShapeById.set(block.toolCallId, { - name: block.toolName, - input: block.input, - }); + index.set(block.toolCallId, { name: block.toolName, input: block.input }); } } } + return index; +} + +/** Every `tool_result` content block across the run's message history. */ +function* toolResultBlocks(request: AgentRunRequest): Generator { for (const message of request.messages ?? []) { const content = message?.content; if (!Array.isArray(content)) continue; for (const block of content) { - const result = approvedCallResultOf(block); - if (!result.found) continue; - const shape = block.toolCallId - ? callShapeById.get(block.toolCallId) - : undefined; - const name = block.toolName ?? shape?.name; - const input = block.input ?? shape?.input; - const argsKey = approvedCallKey(name, input); - if (argsKey) decisions.set(argsKey, result.output); + if (block?.type === "tool_result") yield block; } } - return decisions; +} + +/** The cold-replay name+args key for a tool_result, recovering name/args from the correlated + * tool_call block when the result block itself carries only an id. Never a bare name or an id. */ +function coldReplayKey( + block: ContentBlock, + callShapeById: Map, +): string | undefined { + const shape = block.toolCallId ? callShapeById.get(block.toolCallId) : undefined; + const name = block.toolName ?? shape?.name; + const input = block.input ?? shape?.input; + return approvedCallKey(name, input); } function isPermissionDecision(value: unknown): value is PermissionDecision { @@ -245,11 +326,13 @@ function isPermissionDecision(value: unknown): value is PermissionDecision { } /** - * A paused call reply. Permission responses use `{ approved: boolean }`; client tools carry - * their real structured `output`. + * An approval reply uses an `{ approved: boolean }` envelope (the Vercel adapter's + * `_approval_response_blocks` shape), unique to a permission response. Returns + * `"allow"`/`"deny"` for one, or `undefined` for any other tool_result (a real browser/client + * output). */ -function approvedCallResultOf(block: ContentBlock): { found: boolean; output?: unknown } { - if (!block || block.type !== "tool_result") return { found: false }; +function approvalDecisionOf(block: ContentBlock): PermissionDecision | undefined { + if (!block || block.type !== "tool_result") return undefined; const output = block.output; if ( output && @@ -257,12 +340,9 @@ function approvedCallResultOf(block: ContentBlock): { found: boolean; output?: u !Array.isArray(output) && typeof (output as { approved?: unknown }).approved === "boolean" ) { - return { - found: true, - output: (output as { approved: boolean }).approved ? "allow" : "deny", - }; + return (output as { approved: boolean }).approved ? "allow" : "deny"; } - return { found: true, output }; + return undefined; } diff --git a/services/runner/tests/unit/client-tools.test.ts b/services/runner/tests/unit/client-tools.test.ts new file mode 100644 index 0000000000..87fac7b1c0 --- /dev/null +++ b/services/runner/tests/unit/client-tools.test.ts @@ -0,0 +1,256 @@ +/** + * Unit tests for the shared client-tool seam (engines/sandbox_agent/client-tools.ts): + * the ACP tool-call correlation index, the single client_tool interaction payload, and + * buildClientToolRelay (emits + latches + marks the paused call on pendingApproval, returns + * the responder's verdict as a relay outcome, and delegates onPause to the pause controller). + * + * Run: pnpm test (or: pnpm exec vitest run tests/unit/client-tools.test.ts) + */ +import { describe, it } from "vitest"; +import assert from "node:assert/strict"; + +import type { AgentEvent } from "../../src/protocol.ts"; +import type { ClientToolVerdict, Responder } from "../../src/responder.ts"; +import type { ClientToolRelayRequest } from "../../src/tools/relay.ts"; +import { PendingApprovalLatch } from "../../src/permission-plan.ts"; +import { + buildClientToolRelay, + createToolCallCorrelationIndex, + emitClientToolInteraction, +} from "../../src/engines/sandbox_agent/client-tools.ts"; + +function responderReturning(verdict: ClientToolVerdict): Responder { + return { + async onPermission() { + return { kind: "deny" } as const; + }, + async onClientTool() { + return verdict; + }, + }; +} + +/** A seam harness: fake pause controller + latch + captured events/interactions. */ +function seam(verdict: ClientToolVerdict, opts: { index?: boolean } = {}) { + const events: AgentEvent[] = []; + const pausedToolCalls: string[] = []; + const recorded: Array<{ token: string; toolName?: string; kind: string }> = []; + let pauses = 0; + const index = opts.index ? createToolCallCorrelationIndex() : undefined; + const relay = buildClientToolRelay({ + responder: responderReturning(verdict), + run: { emitEvent: (e) => events.push(e) }, + latch: new PendingApprovalLatch(), + pause: { + markPausedToolCall: (id) => pausedToolCalls.push(id), + pause: () => { + pauses += 1; + }, + }, + recordPendingInteraction: (token, toolName, _args, kind) => { + recorded.push({ token, toolName, kind }); + }, + toolCallIndex: index, + }); + return { relay, events, pausedToolCalls, recorded, index, pauses: () => pauses }; +} + +const req: ClientToolRelayRequest = { + id: "i-1", + toolCallId: "tc-1", + toolName: "request_connection", + input: { integration: "slack" }, + spec: { name: "request_connection", kind: "client", render: { kind: "connect" } }, +}; + +describe("createToolCallCorrelationIndex", () => { + it("maps a live ACP tool_call (name + args) to its real toolCallId", () => { + const index = createToolCallCorrelationIndex(); + index.record({ + sessionUpdate: "tool_call", + toolCallId: "acp-real-1", + title: "request_connection", + rawInput: { integration: "slack" }, + }); + assert.equal( + index.lookup("request_connection", { integration: "slack" }), + "acp-real-1", + "name+args resolves the real id", + ); + // Bare-name fallback when the args differ but the name matched a recorded call. + assert.equal( + index.lookup("request_connection", { integration: "github" }), + "acp-real-1", + "name fallback when args differ", + ); + }); + + it("ignores non-tool_call updates and is first-write-wins", () => { + const index = createToolCallCorrelationIndex(); + index.record({ sessionUpdate: "agent_message_chunk", text: "hi" }); + assert.equal(index.lookup("x", {}), undefined, "no record -> no id"); + index.record({ sessionUpdate: "tool_call", toolCallId: "id-1", title: "t" }); + index.record({ sessionUpdate: "tool_call", toolCallId: "id-2", title: "t" }); + assert.equal(index.lookup("t", {}), "id-1", "first write wins"); + }); +}); + +describe("emitClientToolInteraction", () => { + function collect() { + const events: AgentEvent[] = []; + return { run: { emitEvent: (e: AgentEvent) => events.push(e) }, events }; + } + + it("emits a client_tool interaction with the top-level + toolCall payload the egress reads", () => { + const { run, events } = collect(); + emitClientToolInteraction(run, { + id: "i-1", + toolCallId: "minted-1", + toolName: "request_connection", + input: { integration: "slack" }, + render: { kind: "connect" }, + }); + assert.equal(events.length, 1); + const ev = events[0] as any; + assert.equal(ev.type, "interaction_request"); + assert.equal(ev.kind, "client_tool"); + assert.equal(ev.id, "i-1"); + assert.equal(ev.payload.toolCallId, "minted-1"); + assert.equal(ev.payload.toolName, "request_connection"); + assert.deepEqual(ev.payload.input, { integration: "slack" }); + assert.deepEqual(ev.payload.render, { kind: "connect" }); + // The synthesized toolCall sub-object (the egress reads either shape). + assert.equal(ev.payload.toolCall.id, "minted-1"); + assert.equal(ev.payload.toolCall.name, "request_connection"); + assert.deepEqual(ev.payload.toolCall.rawInput, { integration: "slack" }); + assert.equal(ev.payload.toolCall.kind, "client"); + }); + + it("substitutes the correlated ACP id when the index has one", () => { + const { run, events } = collect(); + const index = createToolCallCorrelationIndex(); + index.record({ + sessionUpdate: "tool_call", + toolCallId: "acp-real", + title: "request_connection", + rawInput: { integration: "slack" }, + }); + emitClientToolInteraction( + run, + { + id: "i-1", + toolCallId: "minted-fallback", + toolName: "request_connection", + input: { integration: "slack" }, + }, + index, + ); + const ev = events[0] as any; + assert.equal(ev.payload.toolCallId, "acp-real", "correlated id wins over the minted one"); + assert.equal(ev.payload.toolCall.id, "acp-real"); + }); + + it("falls back to the minted id when the index has no match", () => { + const { run, events } = collect(); + const index = createToolCallCorrelationIndex(); // empty + emitClientToolInteraction( + run, + { id: "i-1", toolCallId: "minted", toolName: "request_connection", input: {} }, + index, + ); + assert.equal((events[0] as any).payload.toolCallId, "minted"); + }); +}); + +describe("buildClientToolRelay", () => { + it("on pendingApproval: emits the interaction, marks the paused call, records it, returns pendingApproval", async () => { + const s = seam({ kind: "pendingApproval" }); + const outcome = await s.relay.onClientTool(req); + assert.equal(outcome, "pendingApproval"); + assert.equal(s.events.length, 1, "the interaction is emitted"); + const ev = s.events[0] as any; + assert.equal(ev.kind, "client_tool"); + assert.equal(ev.id, "i-1"); + assert.deepEqual(ev.payload.render, { kind: "connect" }); + assert.deepEqual(s.pausedToolCalls, ["tc-1"], "the tool call is marked paused"); + assert.deepEqual(s.recorded, [ + { token: "i-1", toolName: "request_connection", kind: "client_tool" }, + ]); + // onPause is the consumer's responsibility to call after a pendingApproval outcome + // (relay loop / MCP handler); it delegates to the pause controller (the turn-ender). + assert.equal(s.pauses(), 0); + s.relay.onPause?.(req); + assert.equal(s.pauses(), 1); + }); + + it("substitutes the correlated ACP id for the paused call and the payload (Claude)", async () => { + const s = seam({ kind: "pendingApproval" }, { index: true }); + s.index!.record({ + sessionUpdate: "tool_call", + toolCallId: "acp-real", + title: "request_connection", + rawInput: { integration: "slack" }, + }); + await s.relay.onClientTool(req); + assert.deepEqual( + s.pausedToolCalls, + ["acp-real"], + "the CORRELATED id is marked paused (suppresses Claude's late frames)", + ); + assert.equal((s.events[0] as any).payload.toolCallId, "acp-real"); + assert.equal((s.events[0] as any).payload.toolCall.id, "acp-real"); + }); + + it("latch already held: no second interaction, still pendingApproval", async () => { + const s = seam({ kind: "pendingApproval" }); + assert.equal(await s.relay.onClientTool(req), "pendingApproval"); + assert.equal(await s.relay.onClientTool({ ...req, id: "i-2" }), "pendingApproval"); + assert.equal(s.events.length, 1, "only the first pending gate emits"); + assert.equal(s.recorded.length, 1); + }); + + it("does NOT emit when the responder fulfills (resume) or denies", async () => { + const fulfilled = seam({ kind: "fulfilled", output: { connected: true } }); + assert.deepEqual(await fulfilled.relay.onClientTool(req), { + output: { connected: true }, + }); + assert.equal(fulfilled.events.length, 0, "no interaction emitted on a resolved call"); + assert.deepEqual(fulfilled.pausedToolCalls, []); + + const denied = seam({ kind: "deny" }); + assert.equal(await denied.relay.onClientTool(req), "deny"); + assert.equal(denied.events.length, 0, "no interaction emitted on deny"); + }); + + it("consumes the stored output (consume: true) and passes the client gate descriptor", async () => { + const seen: Array<{ gate: unknown; opts: unknown }> = []; + const relay = buildClientToolRelay({ + responder: { + async onPermission() { + return { kind: "deny" } as const; + }, + async onClientTool(request, opts) { + seen.push({ gate: request.gate, opts }); + return { kind: "deny" } as const; + }, + }, + run: { emitEvent: () => {} }, + latch: new PendingApprovalLatch(), + pause: { markPausedToolCall: () => {}, pause: () => {} }, + recordPendingInteraction: () => {}, + }); + await relay.onClientTool(req); + assert.deepEqual(seen, [ + { + gate: { + executor: "client", + toolName: "request_connection", + specPermission: undefined, + readOnlyHint: undefined, + args: { integration: "slack" }, + }, + opts: { consume: true }, + }, + ]); + }); +}); diff --git a/services/runner/tests/unit/responder.test.ts b/services/runner/tests/unit/responder.test.ts index 4de37559a1..435ae9663d 100644 --- a/services/runner/tests/unit/responder.test.ts +++ b/services/runner/tests/unit/responder.test.ts @@ -15,6 +15,7 @@ import { approvedCallKey, decisionToReply, extractApprovalDecisions, + extractClientToolOutputs, type PermissionDecision, } from "../../src/responder.ts"; @@ -170,7 +171,7 @@ describe("ApprovalResponder", () => { const output = { connected: true }; const responder = new ApprovalResponder( plan("deny"), - new ConversationDecisions(new Map([[key, output]])), + new ConversationDecisions(new Map(), new Map([[key, [output]]])), ); const client = gate({ executor: "client", @@ -194,7 +195,7 @@ describe("ApprovalResponder", () => { const output = { connected: true }; const responder = new ApprovalResponder( plan("deny"), - new ConversationDecisions(new Map([[key, output]])), + new ConversationDecisions(new Map(), new Map([[key, [output]]])), ); const client = gate({ executor: "client", @@ -217,7 +218,7 @@ describe("ApprovalResponder", () => { const output = { connected: true }; const responder = new ApprovalResponder( plan("deny"), - new ConversationDecisions(new Map([[key, output]])), + new ConversationDecisions(new Map(), new Map([[key, [output]]])), ); const client = gate({ executor: "client", @@ -308,7 +309,7 @@ describe("extractApprovalDecisions", () => { assert.equal(decisions.has("tc-1"), false); }); - it("stores correlated client-tool outputs under the same call key", () => { + it("routes a correlated client-tool output to the CLIENT store, not the approval store", () => { const request: AgentRunRequest = { messages: [ { @@ -335,11 +336,12 @@ describe("extractApprovalDecisions", () => { ], }; - const decisions = extractApprovalDecisions(request); - assert.deepEqual( - decisions.get(approvedCallKey("request_connection", { integration: "slack" })!), + const key = approvedCallKey("request_connection", { integration: "slack" })!; + // A raw browser output is NOT an approval decision; it lives only in the client store. + assert.equal(extractApprovalDecisions(request).has(key), false); + assert.deepEqual(extractClientToolOutputs(request).get(key), [ { connected: true }, - ); + ]); }); it("ignores ordinary tool results that cannot be bound to a call shape", () => { @@ -367,6 +369,150 @@ describe("extractApprovalDecisions", () => { }); }); +describe("client-tool output store (separate from approvals)", () => { + const clientGate = (input: unknown = { integration: "slack" }): GateDescriptor => ({ + executor: "client", + toolName: "request_connection", + args: input, + }); + + it("extractClientToolOutputs stores raw outputs (not approvals) as a FIFO list per key", () => { + const request: AgentRunRequest = { + sessionId: "s-client", + messages: [ + { + role: "tool", + content: [ + // Two identical request_connection calls -> same name+args key -> a FIFO list of 2. + { + type: "tool_result", + toolCallId: "c-1", + toolName: "request_connection", + input: { integration: "slack" }, + output: { connected: true, account: "first" }, + }, + { + type: "tool_result", + toolCallId: "c-2", + toolName: "request_connection", + input: { integration: "slack" }, + output: { connected: true, account: "second" }, + }, + // An approval envelope must NOT land in the client-output store. + { + type: "tool_result", + toolCallId: "c-3", + toolName: "edit", + input: { path: "a" }, + output: { approved: true }, + }, + ], + }, + ], + }; + const outputs = extractClientToolOutputs(request); + const key = approvedCallKey("request_connection", { integration: "slack" })!; + assert.deepEqual(outputs.get(key), [ + { connected: true, account: "first" }, + { connected: true, account: "second" }, + ]); + // The approval-envelope result is absent from the client-output store... + assert.equal(outputs.has(approvedCallKey("edit", { path: "a" })!), false); + // ...and lives only in the approval store. + const decisions = extractApprovalDecisions(request); + assert.equal(decisions.get(approvedCallKey("edit", { path: "a" })!), "allow"); + }); + + it("resolves two identical client calls from the FIFO store, in order", async () => { + const request: AgentRunRequest = { + sessionId: "s-client", + messages: [ + { + role: "tool", + content: [ + { + type: "tool_result", + toolCallId: "c-1", + toolName: "request_connection", + input: { integration: "slack" }, + output: { account: "first" }, + }, + { + type: "tool_result", + toolCallId: "c-2", + toolName: "request_connection", + input: { integration: "slack" }, + output: { account: "second" }, + }, + ], + }, + ], + }; + const responder = new ApprovalResponder( + plan("ask"), + new ConversationDecisions( + extractApprovalDecisions(request), + extractClientToolOutputs(request), + ), + ); + const request1 = { id: "i-1", toolCallId: "live-1", gate: clientGate() }; + // First call consumes the first output; the second identical call consumes the second. + assert.deepEqual(await responder.onClientTool(request1, { consume: true }), { + kind: "fulfilled", + output: { account: "first" }, + }); + assert.deepEqual(await responder.onClientTool(request1, { consume: true }), { + kind: "fulfilled", + output: { account: "second" }, + }); + // A third identical call has no stored output left -> forward to the browser (pause). + assert.deepEqual(await responder.onClientTool(request1, { consume: true }), { + kind: "pendingApproval", + }); + }); + + it("returns a client output literally \"allow\" as output, never as a permission decision", async () => { + const request: AgentRunRequest = { + sessionId: "s-client", + messages: [ + { + role: "tool", + content: [ + { + type: "tool_result", + toolCallId: "c-1", + toolName: "confirm", + input: { q: "ok?" }, + // The raw browser output happens to be the string "allow" — under the old shared + // store this collided with a permission decision and was skipped. + output: "allow", + }, + ], + }, + ], + }; + const responder = new ApprovalResponder( + plan("ask"), + new ConversationDecisions( + extractApprovalDecisions(request), + extractClientToolOutputs(request), + ), + ); + assert.deepEqual( + await responder.onClientTool( + { + id: "i-1", + toolCallId: "live", + gate: { executor: "client", toolName: "confirm", args: { q: "ok?" } }, + }, + { consume: true }, + ), + { kind: "fulfilled", output: "allow" }, + "the client output is returned verbatim, not interpreted as a permission allow", + ); + }); +}); + describe("emitEvent", () => { it("streaming path: flushes to the live sink and the batch log", () => { const emitted: AgentEvent[] = []; From 7e9b2ebb4b25282bcf19de30ea274421509c20f8 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 4 Jul 2026 19:34:53 +0200 Subject: [PATCH 4/8] feat(runner): deliver client tools to Claude over the internal MCP channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude never saw client tools (request_connection): the internal agenta-tools channel filtered them out of tools/list, so the model could not call them and the browser widget never rendered — a silent drop, Pi-only feature. Now: - mcp-bridge/tool-mcp-http advertise client tools when a clientToolRelay is wired (local Claude) and PAUSE a client tools/call through the shared seam: the handler returns the MCP_PARKED sentinel, the listener destroys the socket with no JSON-RPC body (a result would let Claude settle the call and clobber the pending widget), and the turn ends paused. Required args are validated first so an under-specified call is a normal MCP tool error (model retries). - engine: deferred clientToolRelay ref (the MCP server is built before the responder exists), ACP tool-call correlation index recorded from the event stream (the paused widget attaches to Claude's real tool bubble and its late frames are suppressed), and an mcpAbort AbortController fired from the pause controller's destroy path + the finally so no in-flight tools/call settles after the turn ends. - run-plan (#5047 gate tightened, as its comment assigned to #4985): a non-Pi remote-sandbox run now refuses on ANY custom tool (toolSpecs), client kind included — client tools ride the MCP channel now, so on a remote sandbox they are exactly as undeliverable as gateway tools. Flipped the claude x daytona x client-only test to assert refusal; research.md §4 updated to match. Claude-Session: https://claude.ai/code/session_01HhBEUFbXETNjYdcz71AGrT --- .../remote-tools-delivery/research.md | 11 +- services/runner/src/engines/sandbox_agent.ts | 63 ++++++- .../runner/src/engines/sandbox_agent/mcp.ts | 17 +- .../src/engines/sandbox_agent/run-plan.ts | 16 +- services/runner/src/tools/mcp-bridge.ts | 38 +++- services/runner/src/tools/tool-mcp-http.ts | 165 +++++++++++++++--- .../tests/unit/sandbox-agent-run-plan.test.ts | 33 +++- .../runner/tests/unit/tool-bridge.test.ts | 146 +++++++++++++++- 8 files changed, 432 insertions(+), 57 deletions(-) diff --git a/docs/design/agent-workflows/projects/remote-tools-delivery/research.md b/docs/design/agent-workflows/projects/remote-tools-delivery/research.md index b8a1e17ece..677e36cb98 100644 --- a/docs/design/agent-workflows/projects/remote-tools-delivery/research.md +++ b/docs/design/agent-workflows/projects/remote-tools-delivery/research.md @@ -87,15 +87,16 @@ has an equivalent. ## 4. The interim fix implemented alongside these docs `engines/sandbox_agent/run-plan.ts` `buildRunPlan` now refuses, before any cwd or sandbox is -created, any run where `!isPi && isDaytona && executableToolSpecsForRun.length > 0` — +created, any run where `!isPi && isRemoteSandbox && toolSpecs.length > 0` — `REMOTE_TOOLS_UNSUPPORTED_MESSAGE`. This mirrors the existing not-implemented gates in the same file (`CODE_TOOL_UNSUPPORTED_MESSAGE`, `USER_MCP_UNSUPPORTED_MESSAGE`, `PI_USER_MCP_UNSUPPORTED_MESSAGE`, `FILESYSTEM_UNSUPPORTED_MESSAGE`, `LOCAL_NETWORK_UNSUPPORTED_MESSAGE`): fail loud with a single named message instead of silently -dropping a declared capability. `executableToolSpecsForRun` (already computed earlier in the -function via `executableToolSpecs(toolSpecs)`) excludes `client`-kind tools, which are -browser-fulfilled and were never advertised over the internal channel in the first place -(`tool-mcp-http.ts` `tools/list` filters them out too), so a client-only tool run is unaffected. +dropping a declared capability. The gate counts ALL custom tools, `client` kind included: since +the #4985 recut, client tools ride the same internal MCP channel on local Claude (advertised in +`tools/list`, paused in `tools/call`), so on a remote sandbox they are exactly as undeliverable +as gateway tools — the model would never see them. (The original #5047 gate exempted client +tools because, pre-#4985, they were never routed through the channel at all.) The `mcp.ts` "delivered via the file relay" log is now conditioned on `isPi` so it can never again claim a delivery that isn't happening, as defense-in-depth against a future gate bypass (the run-plan gate should make the branch it guards dead code, but the log no longer trusts that). diff --git a/services/runner/src/engines/sandbox_agent.ts b/services/runner/src/engines/sandbox_agent.ts index aac0f6f8f0..57f28eaaf9 100644 --- a/services/runner/src/engines/sandbox_agent.ts +++ b/services/runner/src/engines/sandbox_agent.ts @@ -41,9 +41,14 @@ import { ConversationDecisions, extractApprovalDecisions, extractClientToolOutputs, + type ClientToolOutcome, type Responder, } from "../responder.ts"; -import { buildClientToolRelay } from "./sandbox_agent/client-tools.ts"; +import type { ClientToolRelay } from "../tools/relay.ts"; +import { + buildClientToolRelay, + createToolCallCorrelationIndex, +} from "./sandbox_agent/client-tools.ts"; import { type AgentRunRequest, type AgentRunResult, @@ -416,6 +421,10 @@ export async function runSandboxAgent( // Internal gateway-tool MCP server closer (set when an internal channel is built for a non-Pi // harness with executable tools; a no-op otherwise). Released in the `finally`. let closeToolMcp: (() => Promise) | undefined; + // Aborts any in-flight loopback `tools/call` (a paused Claude client tool) on pause/teardown, + // so its handler is torn down deterministically and cannot write a result after the turn ends. + // Fired by the pause controller's destroy path and, as a backstop, by the `finally`. + const mcpAbort = new AbortController(); // Durable cwd: set to the host mountpoint once a session-owned local run geesefs-mounts its // store prefix, so the `finally` can unmount it. Undefined for non-session/remote/unmounted runs. let mountedCwd: string | undefined; @@ -617,6 +626,23 @@ export async function runSandboxAgent( log: logger, }); + // Correlate a Claude MCP `tools/call` (name + args only) to the real ACP tool-call id the + // event stream surfaces, so a paused `client_tool` widget attaches to Claude's tool bubble. + const toolCallIndex = createToolCallCorrelationIndex(); + // The shared client-tool relay is only built AFTER the session/model resolve (it needs the + // responder + otel run + pause plumbing). But the internal MCP server is built HERE (its URL + // is handed to createSession) and pauses client tools through that relay. A `tools/call` can + // only arrive during `session.prompt()` — long after the relay is wired — so the server + // captures a DEFERRED reference that resolves to the real relay before any call lands. + let clientToolRelay: ClientToolRelay | undefined; + const deferredClientToolRelay: ClientToolRelay = { + onClientTool: (req) => + clientToolRelay + ? clientToolRelay.onClientTool(req) + : Promise.resolve("deny" as ClientToolOutcome), + onPause: (req) => clientToolRelay?.onPause?.(req), + }; + const sessionMcp = await buildSessionMcpServers({ isPi: plan.isPi, capabilities, @@ -627,6 +653,11 @@ export async function runSandboxAgent( toolSpecs: plan.toolSpecs, userMcpServers: request.mcpServers, relayDir: plan.relayDir, + // Local Claude only: lets the internal channel advertise + pause `client` tools. The + // deferred ref resolves before any `tools/call` arrives. buildSessionMcpServers ignores it + // for Pi / Daytona (no internal channel there). + clientToolRelay: deferredClientToolRelay, + signal: mcpAbort.signal, log: logger, }); // Close the internal gateway-tool MCP server (if one started) when the run ends. @@ -674,16 +705,24 @@ export async function runSandboxAgent( ], }); - const pause = new PendingApprovalPauseController(() => - sandbox.destroySession?.(session.id), - ); + const pause = new PendingApprovalPauseController(() => { + // Abort any in-flight loopback `tools/call` (a paused Claude client tool) BEFORE the + // session teardown, so its handler cannot write a result after the turn ends. + mcpAbort.abort(); + return sandbox.destroySession?.(session.id); + }); session.onEvent((event: any) => { remountLocalCwdAfterRuntimeEnotconn(event); const payload = event?.payload; const update = payload?.params?.update ?? payload?.update; - if (update && !shouldSuppressPausedToolCallUpdate(update, pause)) { - run.handleUpdate(update); + if (update) { + // Record live ACP tool_call ids so a paused client_tool can correlate to Claude's + // bubble (recorded even for suppressed frames; the index is first-write-wins). + toolCallIndex.record(update); + if (!shouldSuppressPausedToolCallUpdate(update, pause)) { + run.handleUpdate(update); + } } }); const permissionPlan = permissionsFromRequest(request); @@ -777,14 +816,18 @@ export async function runSandboxAgent( }, }); - // The ONE client-tool seam both delivery paths share: the Pi file relay (below) consumes it - // directly. Built here because it needs the responder, the otel run, and the pause plumbing. - const clientToolRelay = buildClientToolRelay({ + // Resolve the ONE client-tool seam both delivery paths share: the Pi file relay (below) + // consumes it directly, and the Claude internal MCP server reaches it through the deferred + // ref captured above. Built here because it needs the responder, the otel run, and the pause + // plumbing. The correlation index is wired for Claude only — Pi's relay toolCallId is + // already exact, so it pauses with no index (behavior-preserving). + clientToolRelay = buildClientToolRelay({ responder, run, latch, pause, recordPendingInteraction, + toolCallIndex: plan.isPi ? undefined : toolCallIndex, log: logger, }); @@ -907,6 +950,8 @@ export async function runSandboxAgent( await runtimeRemount?.catch(() => {}); if (sandbox) inFlightSandboxes.delete(sandbox); await toolRelay?.stop().catch(() => {}); + // Teardown backstop: destroy any in-flight loopback `tools/call` before closing the server. + mcpAbort.abort(); await closeToolMcp?.().catch(() => {}); await sandbox?.destroySandbox().catch(() => {}); await sandbox?.dispose().catch(() => {}); diff --git a/services/runner/src/engines/sandbox_agent/mcp.ts b/services/runner/src/engines/sandbox_agent/mcp.ts index b5b623b9ab..5f2e8c1c06 100644 --- a/services/runner/src/engines/sandbox_agent/mcp.ts +++ b/services/runner/src/engines/sandbox_agent/mcp.ts @@ -8,6 +8,7 @@ import { USER_MCP_UNSUPPORTED_MESSAGE, type McpServerStdio, } from "../../tools/mcp-bridge.ts"; +import type { ClientToolRelay } from "../../tools/relay.ts"; type Log = (message: string) => void; @@ -171,6 +172,14 @@ export interface BuildSessionMcpServersInput { toolSpecs: ResolvedToolSpec[]; userMcpServers?: McpServerConfig[]; relayDir: string; + /** + * The shared client-tool relay. When set (local Claude), the internal channel advertises + * `client` tools and pauses a `tools/call` for one. Omit for Pi (which uses the file relay); + * on Daytona the channel is skipped entirely. + */ + clientToolRelay?: ClientToolRelay; + /** Engine pause/teardown abort signal, threaded to the internal MCP server. */ + signal?: AbortSignal; log?: Log; } @@ -214,6 +223,8 @@ export async function buildSessionMcpServers({ toolSpecs, userMcpServers, relayDir, + clientToolRelay, + signal, log = () => {}, }: BuildSessionMcpServersInput): Promise { const userMcpCount = userMcpServers?.length ?? 0; @@ -232,7 +243,11 @@ export async function buildSessionMcpServers({ // sandbox where the harness runs. On Daytona, skip the loopback HTTP advertisement. const internal = isDaytona ? { servers: [], close: async () => {} } - : await buildToolMcpServers(toolSpecs, relayDir, log); + : await buildToolMcpServers(toolSpecs, relayDir, { + clientToolRelay, + signal, + log, + }); // Only Pi has a sandbox-side file-relay writer (its bundled extension), and Pi never reaches // this point (the `isPi` early-return above), so no harness that gets here has ANY delivery // path on Daytona. `run-plan.ts` (`REMOTE_TOOLS_UNSUPPORTED_MESSAGE`) refuses that combination diff --git a/services/runner/src/engines/sandbox_agent/run-plan.ts b/services/runner/src/engines/sandbox_agent/run-plan.ts index 0a2d953d12..9097bc9740 100644 --- a/services/runner/src/engines/sandbox_agent/run-plan.ts +++ b/services/runner/src/engines/sandbox_agent/run-plan.ts @@ -270,16 +270,16 @@ export function buildRunPlan( } // F1 (audit finding, silent-tool-drop): a non-Pi harness on a remote sandbox has NO working - // delivery path for gateway/custom tools. The internal tool-MCP is loopback-only (unreachable - // from inside the sandbox), and the file-relay fallback has a sandbox-side writer only inside - // Pi's bundled extension. Before this gate the run proceeded, silently dropped every tool, and + // delivery path for ANY custom tool. The internal tool-MCP is loopback-only (unreachable from + // inside the sandbox), and the file-relay fallback has a sandbox-side writer only inside Pi's + // bundled extension. Before this gate the run proceeded, silently dropped every tool, and // still returned `ok:true`. Refuse up front, the way the other not-implemented gates above do, // and fail CLOSED for any non-local provider (see `isRemoteSandbox`) so a new remote provider - // cannot silently re-open F1. `executableToolSpecsForRun` excludes `client` tools - // (browser-fulfilled, never routed through this channel), matching what the internal MCP - // channel actually advertises; #4985 moves client tools onto that channel and owns tightening - // this exemption when it lands. - if (!isPi && isRemoteSandbox && executableToolSpecsForRun.length > 0) { + // cannot silently re-open F1. The gate counts ALL tools (`toolSpecs`), `client` kind included: + // client tools now ride the same internal MCP channel on local Claude (advertised + paused in + // `tools/call`), so on a remote sandbox they are exactly as undeliverable as gateway tools — + // the model would never see or be able to call them. + if (!isPi && isRemoteSandbox && toolSpecs.length > 0) { return { ok: false, error: REMOTE_TOOLS_UNSUPPORTED_MESSAGE }; } diff --git a/services/runner/src/tools/mcp-bridge.ts b/services/runner/src/tools/mcp-bridge.ts index d4338b0e0c..1c1372ead6 100644 --- a/services/runner/src/tools/mcp-bridge.ts +++ b/services/runner/src/tools/mcp-bridge.ts @@ -22,6 +22,7 @@ */ import type { ResolvedToolSpec } from "../protocol.ts"; import type { McpServerHttp } from "../engines/sandbox_agent/mcp.ts"; +import type { ClientToolRelay } from "./relay.ts"; import { startInternalToolMcpServer } from "./tool-mcp-http.ts"; export type { ResolvedToolSpec, ToolCallbackContext } from "../protocol.ts"; @@ -73,10 +74,21 @@ export interface ToolMcpServersResult { const NO_OP_CLOSE = async (): Promise => {}; +/** Options for the internal channel: the client-tool relay and the engine pause/teardown signal. */ +export interface BuildToolMcpServersOptions { + /** When set (local Claude), `client` tools are advertised and paused in `tools/call`. */ + clientToolRelay?: ClientToolRelay; + /** Engine abort signal; destroys an in-flight `tools/call` on pause/teardown. */ + signal?: AbortSignal; + log?: Log; +} + /** - * Build the INTERNAL gateway-tool MCP channel: start a loopback HTTP MCP server advertising the - * run's executable tools and return a `type: "http"` server entry pointing at it. An empty / - * all-`client` spec list is a no-op (`{ servers: [], close }`), so the no-tools path is untouched. + * Build the INTERNAL tool MCP channel: start a loopback HTTP MCP server advertising the run's + * tools and return a `type: "http"` server entry pointing at it. An empty spec list is a no-op + * (`{ servers: [], close }`). `client` tools are included ONLY when a `clientToolRelay` is wired + * (local Claude), where the server's `tools/call` pauses them; without one they are dropped here + * (no pause path), so an all-`client` list with no relay stays a no-op as before. * * The returned `close()` MUST be called when the run ends (the engine does this in its `finally`) * to release the bound port. The channel carries no secret: the HTTP entry has empty `headers`, @@ -85,15 +97,23 @@ const NO_OP_CLOSE = async (): Promise => {}; export async function buildToolMcpServers( specs: ResolvedToolSpec[], relayDir: string, - log: Log = () => {}, + options: BuildToolMcpServersOptions = {}, ): Promise { + const { clientToolRelay, signal, log = () => {} } = options; if (!specs || specs.length === 0) return { servers: [], close: NO_OP_CLOSE }; - // `client` tools are browser-fulfilled and never go through this channel; only an executable - // (`code`/`callback`) spec needs delivering to the harness. - const executable = specs.filter((s) => (s.kind ?? "callback") !== "client"); - if (executable.length === 0) return { servers: [], close: NO_OP_CLOSE }; + // Without a relay, a `client` tool has no pause path over this channel, so drop it and deliver + // only executable (`code`/`callback`) specs. With a relay (local Claude), keep client tools — + // the server advertises them and pauses the call. + const deliverable = clientToolRelay + ? specs + : specs.filter((s) => (s.kind ?? "callback") !== "client"); + if (deliverable.length === 0) return { servers: [], close: NO_OP_CLOSE }; - const server = await startInternalToolMcpServer(executable, relayDir, log); + const server = await startInternalToolMcpServer(deliverable, relayDir, { + clientToolRelay, + signal, + log, + }); return { servers: [ { diff --git a/services/runner/src/tools/tool-mcp-http.ts b/services/runner/src/tools/tool-mcp-http.ts index 10d0ac3f81..a041fe46d6 100644 --- a/services/runner/src/tools/tool-mcp-http.ts +++ b/services/runner/src/tools/tool-mcp-http.ts @@ -40,7 +40,8 @@ import type { AddressInfo } from "node:net"; import type { ResolvedToolSpec } from "../protocol.ts"; import { EMPTY_OBJECT_SCHEMA } from "./callback.ts"; import { runResolvedTool } from "./dispatch.ts"; -import { specInputSchema } from "./spec-schema.ts"; +import type { ClientToolRelay } from "./relay.ts"; +import { assertRequiredArguments, specInputSchema } from "./spec-schema.ts"; type Log = (message: string) => void; @@ -50,6 +51,25 @@ const HOST = "127.0.0.1"; /** Bound the request body so a malformed/oversized POST cannot exhaust runner memory. */ const MAX_BODY_BYTES = 1_000_000; +/** + * A paused client tool. The handler returns this sentinel INSTEAD of a JSON-RPC response so the + * request listener emits NO body and deterministically aborts the in-flight HTTP request: a + * normal MCP result would let the harness (Claude) settle the call and clobber the pending + * connect widget before the paused turn is observed. The seam already emitted the `client_tool` + * interaction (`onClientTool`) and the handler then ends the turn (`onPause` -> the engine's + * pause controller), so the turn ends `paused`. + */ +const MCP_PARKED = Symbol("mcp-parked"); + +/** Options for the internal MCP server: the client-tool relay and an engine abort signal. */ +export interface InternalToolMcpServerOptions { + /** When set, a `client` tool call is paused through this relay instead of relayed/executed. */ + clientToolRelay?: ClientToolRelay; + /** Fired by the engine on pause/teardown; destroys any in-flight request so none settles late. */ + signal?: AbortSignal; + log?: Log; +} + export interface InternalToolMcpServer { /** The loopback URL to advertise to the harness as a `type: "http"` MCP server. */ url: string; @@ -57,18 +77,32 @@ export interface InternalToolMcpServer { close: () => Promise; } +/** An MCP tool-error result (`isError`) so the model can recover, not a crash. */ +function mcpToolError(id: unknown, err: unknown): unknown { + return { + jsonrpc: "2.0", + id, + result: { + content: [{ type: "text", text: err instanceof Error ? err.message : String(err) }], + isError: true, + }, + }; +} + /** - * Handle one MCP JSON-RPC message. Returns the JSON-RPC response object, or `undefined` for a - * notification (no `id`). Mirrors the pre-#4831 stdio bridge handler, but takes the specs and - * relay dir in-process rather than from env, and dispatches `tools/call` to `runResolvedTool`. + * Handle one MCP JSON-RPC message. Returns the JSON-RPC response object, `undefined` for a + * notification (no `id`), or the `MCP_PARKED` sentinel for a paused client tool (the listener + * then aborts the request with no body). Takes the specs and relay dir in-process rather than + * from env, and dispatches a non-`client` `tools/call` to `runResolvedTool`. */ async function handle( message: any, specByName: Map, specs: ResolvedToolSpec[], relayDir: string, + clientToolRelay: ClientToolRelay | undefined, log: Log, -): Promise { +): Promise { const { id, method, params } = message ?? {}; // Notifications (no id, e.g. notifications/initialized) need no response. @@ -91,11 +125,12 @@ async function handle( jsonrpc: "2.0", id, result: { - // `client` tools are browser-fulfilled, so this channel never advertises them. Only - // public metadata (name/description/inputSchema) crosses to the harness — never the - // callRef, code, scoped env, or callback auth, which stay in runner memory. + // Advertise EVERY spec, including `client` tools (e.g. request_connection): the model + // must SEE them to call them; the runner pauses the call in `tools/call` below rather + // than executing it. (`buildToolMcpServers` already dropped `client` specs when no relay + // is wired.) Only public metadata (name/description/inputSchema) crosses to the harness + // — never the callRef, code, scoped env, or callback auth, which stay in runner memory. tools: specs - .filter((s) => (s.kind ?? "callback") !== "client") .map((s) => ({ name: s.name, description: s.description ?? s.name, @@ -121,6 +156,50 @@ async function handle( error: { code: -32602, message: `unknown tool: ${name}` }, }; } + + // `client` tools are browser-fulfilled across a turn boundary: pause them through the shared + // relay instead of executing. Validate required args FIRST so an under-specified call returns + // a normal MCP tool error and the model retries (same guard the Pi path has), rather than + // pausing a half-specified call. + if ((spec.kind ?? "callback") === "client") { + try { + assertRequiredArguments(spec, params?.arguments); + } catch (err) { + return mcpToolError(id, err); + } + if (!clientToolRelay) { + return mcpToolError( + id, + new Error(`client tool '${spec.name}' cannot be delivered on this run`), + ); + } + const callId = randomUUID(); + const request = { + id: callId, + toolCallId: callId, + toolName: spec.name, + input: params?.arguments, + spec, + }; + const decision = await clientToolRelay.onClientTool(request); + if (decision === "pendingApproval") { + clientToolRelay.onPause?.(request); + // No JSON-RPC result: the request listener aborts this in-flight request (see MCP_PARKED). + return MCP_PARKED; + } + if (decision === "deny") { + return mcpToolError(id, new Error(`Client tool '${spec.name}' was denied.`)); + } + // Resume: the browser already fulfilled the call; return its structured output as content. + return { + jsonrpc: "2.0", + id, + result: { + content: [{ type: "text", text: JSON.stringify(decision.output ?? {}) }], + }, + }; + } + try { // The channel holds only public metadata; execution relays to the runner via the relay // dir, where the private spec + callback auth are applied server-side. A unique id per @@ -181,17 +260,32 @@ function readBody(req: IncomingMessage): Promise { /** * Start the internal gateway-tool MCP server on loopback. Returns the URL to advertise and a - * `close()`. The caller decides whether to start it (only when there are executable specs); this - * function does not filter — it serves whatever specs it is given. + * `close()`. The caller decides whether to start it; this function does not filter — it serves + * whatever specs it is given. A `client` spec is paused through `options.clientToolRelay`; + * `options.signal` (the engine's pause/teardown abort) destroys any in-flight request so a + * paused call never settles late. */ export function startInternalToolMcpServer( specs: ResolvedToolSpec[], relayDir: string, - log: Log = () => {}, + options: InternalToolMcpServerOptions = {}, ): Promise { + const { clientToolRelay, signal, log = () => {} } = options; const specByName = new Map(specs.map((s) => [s.name, s])); + // Track in-flight responses so the engine abort signal can destroy them deterministically (a + // paused client tool destroys its OWN response below; the signal is the backstop for any other + // request still open when the turn ends). + const active = new Set(); + + /** Abort a paused request: destroy the socket with no body written, so nothing settles late. */ + const abortParked = (res: ServerResponse): void => { + active.delete(res); + res.destroy(); + }; 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") { @@ -219,22 +313,40 @@ export function startInternalToolMcpServer( // A batch is an array; handle each and answer with an array of the responses that have // an id (notifications produce none). if (Array.isArray(parsed)) { - const responses = ( - await Promise.all( - parsed.map((m) => handle(m, specByName, specs, relayDir, log)), - ) - ).filter((r) => r !== undefined); - if (responses.length === 0) { + const responses = await Promise.all( + parsed.map((m) => + handle(m, specByName, specs, relayDir, clientToolRelay, log), + ), + ); + // A paused client tool in the batch aborts the whole request (no result for any). + if (responses.some((r) => r === MCP_PARKED)) { + abortParked(res); + return; + } + const out = responses.filter((r) => r !== undefined); + if (out.length === 0) { res.writeHead(202); res.end(); return; } res.writeHead(200, { "content-type": "application/json" }); - res.end(JSON.stringify(responses)); + res.end(JSON.stringify(out)); return; } - const response = await handle(parsed, specByName, specs, relayDir, log); + const response = await handle( + parsed, + specByName, + specs, + relayDir, + clientToolRelay, + log, + ); + if (response === MCP_PARKED) { + // Paused client tool: emit NO JSON-RPC result, abort the in-flight request. + abortParked(res); + return; + } if (response === undefined) { // Notification: no body. 202 Accepted is the streamable-HTTP convention. res.writeHead(202); @@ -263,6 +375,18 @@ export function startInternalToolMcpServer( const server: Server = createServer(requestListener); + // Belt and suspenders: on pause/teardown the engine fires this signal; destroy every in-flight + // request so a handler that has not yet returned cannot write a result after the turn ended. + const onAbort = (): void => { + for (const res of [...active]) res.destroy(); + active.clear(); + server.closeAllConnections?.(); + }; + if (signal) { + if (signal.aborted) onAbort(); + else signal.addEventListener("abort", onAbort, { once: true }); + } + return new Promise((resolve, reject) => { server.on("error", reject); // Port 0 -> the OS assigns an ephemeral free port, read back from address(). @@ -279,6 +403,7 @@ export function startInternalToolMcpServer( url, close: () => new Promise((done) => { + signal?.removeEventListener("abort", onAbort); server.close(() => done()); // Drop keep-alive sockets so close() resolves promptly even if a client lingers. server.closeAllConnections?.(); 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 229f33c69d..b13e330cc9 100644 --- a/services/runner/tests/unit/sandbox-agent-run-plan.test.ts +++ b/services/runner/tests/unit/sandbox-agent-run-plan.test.ts @@ -506,10 +506,13 @@ describe("buildRunPlan", () => { assert.equal(result.ok, true); }); - it("allows claude x daytona x client-only tools (browser-fulfilled, not routed through the channel)", () => { - // `client` tools are excluded from `executableToolSpecsForRun` — they are fulfilled by the - // browser across a turn boundary and were never advertised over the internal tool-MCP - // channel (`tool-mcp-http.ts` `tools/list` filters them out too), so they carry no F1 gap. + it("refuses claude x daytona x client-only tools (they ride the MCP channel now)", () => { + // Client tools are delivered to Claude over the same internal loopback MCP channel as + // gateway tools (advertised in tools/list, paused in tools/call). On a remote sandbox that + // channel is unreachable, so a client tool is exactly as undeliverable as a gateway tool: + // the model would never see it. The old exemption (client tools "not routed through the + // channel") is gone; the gate now counts ALL custom tools. + let created = false; const result = buildRunPlan( { harness: "claude", @@ -517,6 +520,28 @@ describe("buildRunPlan", () => { 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, /non-Pi harness on a remote sandbox/); + assert.equal(created, false, "fails before any cwd is created (up-front gate)"); + }); + + it("allows pi x daytona x client-only tools (Pi's extension + file relay deliver them)", () => { + const result = buildRunPlan( + { + harness: "pi_agenta", + sandbox: "daytona", + messages: [{ role: "user", content: "hello" }], + customTools: [{ name: "request_connection", kind: "client" }], + } as AgentRunRequest, { createDaytonaCwd: () => "/home/sandbox/agenta-fixed" }, ); diff --git a/services/runner/tests/unit/tool-bridge.test.ts b/services/runner/tests/unit/tool-bridge.test.ts index 8338fd16ec..b20f46bae8 100644 --- a/services/runner/tests/unit/tool-bridge.test.ts +++ b/services/runner/tests/unit/tool-bridge.test.ts @@ -30,7 +30,11 @@ import { buildToolMcpServers, type ToolMcpServersResult, } from "../../src/tools/mcp-bridge.ts"; -import { RELAY_REQ_SUFFIX, RELAY_RES_SUFFIX } from "../../src/tools/relay.ts"; +import { + RELAY_REQ_SUFFIX, + RELAY_RES_SUFFIX, + type ClientToolRelay, +} from "../../src/tools/relay.ts"; import type { ResolvedToolSpec } from "../../src/protocol.ts"; const relayDir = "/tmp/agenta-tools"; @@ -312,4 +316,144 @@ describe("buildToolMcpServers (internal gateway-tool channel)", () => { assert.equal(out, undefined, "notification -> 202, no body"); }); }); + + describe("client tools (Claude delivery)", () => { + const clientSpec: ResolvedToolSpec = { + name: "request_connection", + kind: "client", + input_schema: { + type: "object", + required: ["integration"], + properties: { integration: { type: "string" } }, + }, + } as unknown as ResolvedToolSpec; + + it("advertises client tools in tools/list when a relay is wired", async () => { + const relay: ClientToolRelay = { + onClientTool: async () => "pendingApproval", + }; + const { servers } = await build( + [{ name: "search", kind: "callback", callRef: "x" }, clientSpec], + relayDir, + { clientToolRelay: relay }, + ); + assert.equal(servers.length, 1, "the server starts even with a client tool present"); + const list = await rpc(servers[0].url, { + jsonrpc: "2.0", + id: 1, + method: "tools/list", + }); + const names = list.result.tools.map((t: any) => t.name).sort(); + assert.deepEqual(names, ["request_connection", "search"]); + }); + + it("pauses: NO tool result, the request is aborted, and onPause fires exactly once", async () => { + // The acceptance unit: a paused client tool must produce NO JSON-RPC result for its + // tools/call (a result would let Claude settle and clobber the pending widget). The handler + // returns the paused sentinel and the listener destroys the socket, so the client's request + // is aborted with no body — and onPause is called once (the turn-ender). + let pauseCount = 0; + let onClientToolCalls = 0; + const relay: ClientToolRelay = { + onClientTool: async () => { + onClientToolCalls += 1; + return "pendingApproval"; + }, + onPause: () => { + pauseCount += 1; + }, + }; + const { servers } = await build([clientSpec], relayDir, { + clientToolRelay: relay, + }); + await assert.rejects( + async () => { + const res = await fetch(servers[0].url, { + method: "POST", + headers: { + "content-type": "application/json", + accept: "application/json, text/event-stream", + }, + body: JSON.stringify({ + jsonrpc: "2.0", + id: 7, + method: "tools/call", + params: { + name: "request_connection", + arguments: { integration: "slack" }, + }, + }), + }); + // No body is ever written for a paused call; reading it must fail (socket destroyed). + await res.text(); + }, + "the paused tools/call is aborted with no JSON-RPC result", + ); + assert.equal(onClientToolCalls, 1, "the relay was consulted once"); + assert.equal(pauseCount, 1, "onPause fired exactly once"); + }); + + it("validates required args in the client branch (a normal MCP error, not a pause)", async () => { + let pauseCount = 0; + const relay: ClientToolRelay = { + onClientTool: async () => "pendingApproval", + onPause: () => { + pauseCount += 1; + }, + }; + const { servers } = await build([clientSpec], relayDir, { + clientToolRelay: relay, + }); + const out = await rpc(servers[0].url, { + jsonrpc: "2.0", + id: 8, + method: "tools/call", + params: { name: "request_connection", arguments: {} }, // missing `integration` + }); + assert.equal(out.result.isError, true, "an under-specified call is a tool error"); + assert.match(out.result.content[0].text, /missing required argument\(s\): integration/); + assert.equal(pauseCount, 0, "an under-specified call never pauses"); + }); + + it("resumes: returns the browser's structured output as MCP content", async () => { + const relay: ClientToolRelay = { + onClientTool: async () => ({ output: { connected: true, account: "a" } }), + }; + const { servers } = await build([clientSpec], relayDir, { + clientToolRelay: relay, + }); + const out = await rpc(servers[0].url, { + jsonrpc: "2.0", + id: 9, + method: "tools/call", + params: { + name: "request_connection", + arguments: { integration: "slack" }, + }, + }); + assert.equal(out.result.isError, undefined, "a resolved client tool is not an error"); + assert.equal( + out.result.content[0].text, + JSON.stringify({ connected: true, account: "a" }), + ); + }); + + it("denies: a normal MCP tool error the model can recover from", async () => { + const relay: ClientToolRelay = { onClientTool: async () => "deny" }; + const { servers } = await build([clientSpec], relayDir, { + clientToolRelay: relay, + }); + const out = await rpc(servers[0].url, { + jsonrpc: "2.0", + id: 10, + method: "tools/call", + params: { + name: "request_connection", + arguments: { integration: "slack" }, + }, + }); + assert.equal(out.result.isError, true); + assert.match(out.result.content[0].text, /was denied/); + }); + }); }); From fc94a9d1b1f12cdeded4bb93f341090cc96b4ffb Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 4 Jul 2026 19:35:47 +0200 Subject: [PATCH 5/8] perf(runner): idle backoff on the tool-relay poll loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The relay loop polled host.list(relayDir) every 300 ms for the whole turn — on Daytona a remote ls exec ~3x/s, now also for client-only runs that spend the turn waiting on a browser-fulfilled pause with no other tool traffic. After 5 consecutive idle polls the delay grows geometrically to a 1.5 s cap (AGENTA_AGENT_TOOLS_RELAY_POLLING_MAX / _IDLE_GROW_AFTER) and resets on any new request file, so a real tool call is still picked up promptly. Claude-Session: https://claude.ai/code/session_01HhBEUFbXETNjYdcz71AGrT --- services/runner/src/tools/relay.ts | 31 ++++++++++++++++++- .../tests/unit/tool-relay-permission.test.ts | 17 ++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/services/runner/src/tools/relay.ts b/services/runner/src/tools/relay.ts index 611400e8e5..44b197cde8 100644 --- a/services/runner/src/tools/relay.ts +++ b/services/runner/src/tools/relay.ts @@ -43,6 +43,28 @@ export const RELAY_POLL_MS = Number( export const RELAY_TIMEOUT_MS = Number( process.env.AGENTA_AGENT_TOOLS_RELAY_TIMEOUT ?? 60000, ); +/** + * Idle-backoff cap for the runner relay poll. The loop polls `host.list(relayDir)` every + * `RELAY_POLL_MS` (300 ms) for the whole turn — on Daytona that `list` is a remote `ls` exec + * (~3×/s), now also for client-only runs that wait on a browser-fulfilled pause and produce no + * other tool traffic. After `RELAY_POLL_IDLE_GROW_AFTER` consecutive idle polls the delay grows + * geometrically up to this cap, so a quiet turn settles to ~1.5 s polls; the moment a request + * file appears the delay resets to `RELAY_POLL_MS`, so a real tool call is still picked up + * promptly. + */ +export const RELAY_POLL_MAX_MS = Number( + process.env.AGENTA_AGENT_TOOLS_RELAY_POLLING_MAX ?? 1500, +); +export const RELAY_POLL_IDLE_GROW_AFTER = Number( + process.env.AGENTA_AGENT_TOOLS_RELAY_IDLE_GROW_AFTER ?? 5, +); + +/** The next poll delay given the count of consecutive idle polls (no new request seen). */ +export function relayPollDelayMs(idlePolls: number): number { + if (idlePolls < RELAY_POLL_IDLE_GROW_AFTER) return RELAY_POLL_MS; + const factor = 2 ** (idlePolls - RELAY_POLL_IDLE_GROW_AFTER + 1); + return Math.min(RELAY_POLL_MS * factor, RELAY_POLL_MAX_MS); +} export interface RelayRequest { toolName: string; @@ -281,18 +303,25 @@ export function startToolRelay( }; const loop = (async () => { + // Idle-poll backoff: a quiet turn (e.g. waiting on a browser-fulfilled client-tool pause) + // grows the delay up to RELAY_POLL_MAX_MS instead of polling at 300 ms forever; any new + // request resets it. This cuts the remote `ls` rate on Daytona without delaying a real call. + let idlePolls = 0; while (active) { + let sawNew = false; try { const names = await host.list(relayDir); for (const name of names) { if (!name.endsWith(RELAY_REQ_SUFFIX) || seen.has(name)) continue; seen.add(name); + sawNew = true; inflight.push(handle(name)); } } catch { // Transient (dir not created yet, or a poll raced sandbox teardown): retry. } - await sleep(RELAY_POLL_MS); + idlePolls = sawNew ? 0 : idlePolls + 1; + await sleep(relayPollDelayMs(idlePolls)); } await Promise.allSettled(inflight); })(); diff --git a/services/runner/tests/unit/tool-relay-permission.test.ts b/services/runner/tests/unit/tool-relay-permission.test.ts index c920a0c253..a264ae2109 100644 --- a/services/runner/tests/unit/tool-relay-permission.test.ts +++ b/services/runner/tests/unit/tool-relay-permission.test.ts @@ -11,6 +11,9 @@ import { join } from "node:path"; import { localRelayHost, + RELAY_POLL_MAX_MS, + RELAY_POLL_MS, + relayPollDelayMs, startToolRelay, type ClientToolRelay, type RelayPermissions, @@ -23,6 +26,20 @@ import { ConversationDecisions, } from "../../src/responder.ts"; +describe("relayPollDelayMs (idle backoff)", () => { + it("polls at the base rate while busy, then backs off geometrically up to the cap", () => { + // No idle polls -> base rate. + assert.equal(relayPollDelayMs(0), RELAY_POLL_MS); + assert.equal(relayPollDelayMs(4), RELAY_POLL_MS, "still base before the grow threshold"); + // After the threshold the delay grows but never exceeds the cap. + assert.ok(relayPollDelayMs(5) > RELAY_POLL_MS, "grows once idle"); + assert.ok(relayPollDelayMs(5) <= RELAY_POLL_MAX_MS); + assert.equal(relayPollDelayMs(100), RELAY_POLL_MAX_MS, "saturates at the cap"); + // Monotonic non-decreasing. + assert.ok(relayPollDelayMs(6) >= relayPollDelayMs(5)); + }); +}); + const codeSpec = ( name: string, permission?: ResolvedToolSpec["permission"], From a512ae098d0d295f6b59244af11ec7cb2e18c5b5 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 4 Jul 2026 19:37:56 +0200 Subject: [PATCH 6/8] =?UTF-8?q?docs(agent-workflows):=20client=20tools=20r?= =?UTF-8?q?ide=20the=20Claude=20MCP=20channel=20=E2=80=94=20sync=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sync the five docs that described the old behavior (client tools filtered out of the internal channel's tools/list): ground-truth + tools.md now describe the shared pause seam and both delivery paths; runner-to-mcp-server.md documents the tools/list advertisement (specInputSchema), the tools/call pause (no-result + abort), the tightened REMOTE_TOOLS gate, and the new owned-by files; the two interface pages update the client-kind comment. Claude-Session: https://claude.ai/code/session_01HhBEUFbXETNjYdcz71AGrT --- .../documentation/ground-truth.md | 7 ++ .../agent-workflows/documentation/tools.md | 39 +++++++--- .../cross-service/runner-to-mcp-server.md | 76 +++++++++++++------ .../in-service/tool-models-and-resolution.md | 3 +- .../public-edge/agent-config-schema.md | 2 +- 5 files changed, 92 insertions(+), 35 deletions(-) diff --git a/docs/design/agent-workflows/documentation/ground-truth.md b/docs/design/agent-workflows/documentation/ground-truth.md index 74d497bc76..a4e9bc59c0 100644 --- a/docs/design/agent-workflows/documentation/ground-truth.md +++ b/docs/design/agent-workflows/documentation/ground-truth.md @@ -54,6 +54,13 @@ this page and the referenced code as the source of truth. channel, served over a loopback HTTP MCP endpoint the runner stands up (no runner-host child process). User-declared MCP resolution is feature-gated (`AGENTA_AGENT_ENABLE_MCP`, off by default). +- `client` tools (browser-fulfilled, e.g. `request_connection`) are delivered to Claude too on + the local path: advertised over the same internal MCP channel and PAUSED in the `tools/call` + handler (no JSON-RPC result + abort the request), then resumed from the browser result next + turn — the same cross-turn pause Pi gets via the file relay, through one shared seam + (`services/runner/src/engines/sandbox_agent/client-tools.ts`). On a remote sandbox the + loopback channel is unreachable, so a non-Pi run carrying ANY custom tool — client kind + included — is refused up front (`REMOTE_TOOLS_UNSUPPORTED_MESSAGE`), never dropped silently. ## Not Implemented diff --git a/docs/design/agent-workflows/documentation/tools.md b/docs/design/agent-workflows/documentation/tools.md index 4704879249..d5350b432b 100644 --- a/docs/design/agent-workflows/documentation/tools.md +++ b/docs/design/agent-workflows/documentation/tools.md @@ -197,8 +197,10 @@ natively. Today that splits cleanly into two paths. directory. It never receives the `call_ref`, the code, the scoped secrets, or the callback auth. When the model calls a tool, the bridge relays the request back to the runner, and the runner runs the private spec from memory. This `agenta-tools` server is a tool DELIVERY - vehicle, not a user MCP server: it carries gateway and code tools, and it exists only on the - Claude path. + vehicle, not a user MCP server: it carries gateway and code tools AND `client` tools (which it + pauses in `tools/call` rather than executing — see "Client tools" below), and it exists only on + the local Claude path (it is skipped on a remote sandbox, where its loopback URL is + unreachable). Both paths funnel execution through one function, `runResolvedTool` in `services/agent/src/tools/dispatch.ts`. It is the single place that branches on `kind`, so how @@ -296,13 +298,32 @@ not, until a provisioning story exists. ### Client tools: the browser fulfils them across a turn boundary Execution happens in the browser, not in the runner at all. A client tool is never run -in-sandbox; `runResolvedTool` throws if one is ever dispatched there, and the MCP bridge filters -client tools out of its advertised list. Instead, when the harness calls a client tool, the -runner emits an `interaction_request` event of kind `client_tool`. The `/messages` egress -projects it to a browser component, the browser runs it, and the result returns in the next -`/messages` turn, matched back by id. This is the cross-turn human-in-the-loop path, the same -mechanism approvals use. A client tool is the right type whenever only the user's environment -can answer: their location, a file on their machine, a confirmation only they can give. +in-sandbox; `runResolvedTool` throws if one is ever dispatched there. The model still SEES the +tool and calls it; the runner then PAUSES the call and emits an `interaction_request` event of +kind `client_tool`. The `/messages` egress projects it to a browser component, the browser runs +it, and the result returns in the next `/messages` turn, matched back by tool name + args. This +is the cross-turn human-in-the-loop path, the same mechanism approvals use. A client tool is the +right type whenever only the user's environment can answer: their location, a file on their +machine, a confirmation only they can give. + +The pause itself is shared by both delivery paths through one seam +(`services/runner/src/engines/sandbox_agent/client-tools.ts`, `buildClientToolRelay` + +`emitClientToolInteraction`): + +- **Pi** calls the tool through its extension; the runner's file relay pauses it (writes no + response file) and the seam emits the interaction. +- **Claude** calls the tool over the internal `agenta-tools` MCP server, and the runner pauses it + inside the `tools/call` handler: it emits NO JSON-RPC result and aborts that in-flight request, + so Claude cannot settle the call before the turn ends `paused`. The browser result resumes it + next turn (the MCP handler returns the stored output if the model re-calls). This is + local-only: on a remote sandbox the loopback MCP channel is unreachable, so a non-Pi run + carrying ANY custom tool — client kind included — is rejected up front + (`REMOTE_TOOLS_UNSUPPORTED_MESSAGE`), never delivered silently. (The ACP permission gate in + `acp-interactions.ts` keeps its own `kind: "client"` pause branch as a live fallback for a + harness that raises a permission gate carrying a resolved client spec.) + +A client tool's `render` hint can be `{ kind: "connect" }` (e.g. `request_connection`), the typed +member of `RenderHint` that asks the frontend to draw the connect widget. ### Built-in tools: the harness runs them natively diff --git a/docs/design/agent-workflows/interfaces/cross-service/runner-to-mcp-server.md b/docs/design/agent-workflows/interfaces/cross-service/runner-to-mcp-server.md index 938142e33e..a1744bd9c6 100644 --- a/docs/design/agent-workflows/interfaces/cross-service/runner-to-mcp-server.md +++ b/docs/design/agent-workflows/interfaces/cross-service/runner-to-mcp-server.md @@ -32,24 +32,35 @@ server on `127.0.0.1:` and returns one ACP `type: "http"` entry (stateless JSON mode) and answers three methods: - `initialize`: returns protocol version and `capabilities.tools`. -- `tools/list`: returns the resolved tool specs as MCP tools. Client-kind tools are filtered - out here, because the browser fulfills those. -- `tools/call`: runs the named tool through `runResolvedTool(..., { relayDir })` (the same relay - the Pi path uses) and returns `content`, or an error. +- `tools/list`: returns the resolved tool specs as MCP tools, reading each tool's input schema + through the shared `specInputSchema` accessor (camelCase `inputSchema` OR snake-case + `input_schema` — reading `inputSchema` alone advertised an EMPTY schema for every + platform-catalog tool). `client` tools ARE advertised here (when a `clientToolRelay` is wired, + i.e. local Claude): the model must see them to call them; the runner pauses the call in + `tools/call`. +- `tools/call`: for an executable (`code`/`callback`) tool, runs it through + `runResolvedTool(..., { relayDir })` (the same relay the Pi path uses) and returns `content`, + or an error. For a `client` tool it validates required args, then pauses through the shared + client-tool seam: on `pendingApproval` it emits NO JSON-RPC result and the request listener + aborts the in-flight request (socket destroyed, no body) so the harness cannot settle the call + before the turn ends `paused`; an engine `AbortSignal` cancels any other in-flight request on + pause/teardown. On resume it returns the browser's stored output. It carries NO credential: the entry has empty `headers`, the server holds only public metadata + the relay dir, and it is bound to loopback. It launches no child process — it is served by the already-running runner — so it does not reintroduce the runner-host execution hole that #4831 closed for user stdio MCP. The run end closes it (releases the port). -**On Daytona the internal channel is NOT advertised — the file relay delivers the tools.** The -loopback URL is a runner-host address; on Daytona the harness runs IN the sandbox, where -`127.0.0.1` is the sandbox's own loopback, not the runner's, so the URL is unreachable. -`buildSessionMcpServers` therefore skips the internal channel when `isDaytona` is true and the -already-running file relay (below) delivers the gateway tools instead — the runner's relay loop -polls the sandbox filesystem. This honors the design decision "HTTP advertisement for local, file -relay for Daytona." A user http MCP server (a remote URL the harness dials directly) is NOT -loopback-bound and stays delivered on Daytona unchanged. +**On Daytona the internal channel is NOT advertised — only Pi gets tools there.** The loopback +URL is a runner-host address; on Daytona the harness runs IN the sandbox, where `127.0.0.1` is +the sandbox's own loopback, not the runner's, so the URL is unreachable. `buildSessionMcpServers` +skips the internal channel when `isDaytona` is true; only Pi's in-sandbox extension consumes the +file relay there. A non-Pi (MCP-delivered) harness has no in-sandbox tool reader, so a non-Pi +remote-sandbox run carrying ANY custom tool (gateway/callback OR client) is refused up front in +`run-plan.ts` with `REMOTE_TOOLS_UNSUPPORTED_MESSAGE` — fail loud, not a silent empty delivery +(the capability gate keys on `mcpTools`, which Claude reports `true`). The gate keys on "not +local", so an unknown remote provider fails closed too. A user http MCP server (a remote URL the +harness dials directly) is NOT loopback-bound and stays delivered on Daytona unchanged. **The file relay.** A resolved tool may need to run privately rather than inside the harness process. The relay moves the call across that boundary: the child writes a `.req.json` @@ -83,24 +94,41 @@ allowlist, and permission. Two transports, opposite states: ## Owned by - `sdks/python/agenta/sdk/agents/mcp/`: the Python models and resolver. -- `services/agent/src/engines/sandbox_agent/mcp.ts`: builds the session's MCP servers (the two - layers; the `isDaytona` guard on the internal channel; `validateUserMcpUrl` SSRF guard). -- `services/agent/src/tools/mcp-bridge.ts`: the internal gateway-tool channel builder; the - `USER_MCP_UNSUPPORTED_MESSAGE` and `PI_USER_MCP_UNSUPPORTED_MESSAGE` refusal constants. -- `services/agent/src/tools/tool-mcp-http.ts`: the internal loopback HTTP MCP server. -- `services/agent/src/tools/mcp-server.ts`: the removed stdio JSON-RPC server (refusing stub). -- `services/agent/src/tools/relay.ts`: the file relay loop and hosts. +- `services/runner/src/engines/sandbox_agent/mcp.ts`: builds the session's MCP servers (the two + layers; the `isDaytona` skip on the internal channel; threads `clientToolRelay` + abort signal; + `validateUserMcpUrl` SSRF guard). +- `services/runner/src/engines/sandbox_agent/run-plan.ts`: the `REMOTE_TOOLS_UNSUPPORTED_MESSAGE` + gate (a non-Pi remote-sandbox run carrying ANY custom tool fails up front). +- `services/runner/src/engines/sandbox_agent/client-tools.ts`: the shared client-tool seam + (`buildClientToolRelay`, `emitClientToolInteraction`, the ACP tool-call correlation index). +- `services/runner/src/tools/mcp-bridge.ts`: the internal channel builder (advertises `client` + tools when a relay is wired); the `USER_MCP_UNSUPPORTED_MESSAGE` / + `PI_USER_MCP_UNSUPPORTED_MESSAGE` refusal constants. +- `services/runner/src/tools/tool-mcp-http.ts`: the internal loopback HTTP MCP server (the + `client` pause: no JSON-RPC result + abort-the-request). +- `services/runner/src/tools/spec-schema.ts`: the shared `specInputSchema` accessor + arg + validation. +- `services/runner/src/tools/mcp-server.ts`: the removed stdio JSON-RPC server (refusing stub). +- `services/runner/src/tools/relay.ts`: the file relay loop and hosts (idle-poll backoff). ## Watch for when changing - **The gate.** MCP delivery depends on harness type and the `mcpTools` capability, not on a single env flag. Changing either changes which tools reach the harness. - **The MCP server config shape.** It is part of the `/run` contract and the wire serializer. -- **The internal channel's MCP methods.** `initialize`, `tools/list`, `tools/call`, and the - client-tool filter, served over loopback HTTP. The framing (stateless JSON Streamable-HTTP) is - pinned to the MCP client the installed Claude harness uses; re-verify it if that version moves. -- **The relay.** Polling interval, timeout, and the local-versus-Daytona host. A slow tool - must fail cleanly. +- **The internal channel's MCP methods.** `initialize`, `tools/list` (now advertises `client` + tools and reads schemas via `specInputSchema`), and `tools/call` (the `client` pause: emit NO + result + abort the request, so the paused widget is the last word before the turn ends). + Served over loopback HTTP; the framing (stateless JSON Streamable-HTTP) is pinned to the MCP + client the installed Claude harness uses; re-verify it if that version moves. +- **The client-tool pause is no-result-before-finish.** A paused `tools/call` must never write a + JSON-RPC result (a result lets the harness settle and clobber the pending widget); the handler + aborts its own request and the engine fires an `AbortSignal` on pause/teardown. +- **The remote-tools gate.** A non-Pi remote-sandbox run carrying ANY custom tool (client kind + included) is refused in `run-plan.ts`. Swap it for a real in-sandbox delivery path when one + exists; do not widen it. +- **The relay.** Polling interval, idle backoff, timeout, and the local-versus-Daytona host. A + slow tool must fail cleanly. - **HTTP MCP delivery.** `toAcpMcpServers` routes the resolved secret from `env` into a request header and builds the ACP `type: "http"` entry. Changing the env-to-header mapping or the ACP variant shape changes which auth reaches the remote server. diff --git a/docs/design/agent-workflows/interfaces/in-service/tool-models-and-resolution.md b/docs/design/agent-workflows/interfaces/in-service/tool-models-and-resolution.md index df7403f80a..c3d33f2445 100644 --- a/docs/design/agent-workflows/interfaces/in-service/tool-models-and-resolution.md +++ b/docs/design/agent-workflows/interfaces/in-service/tool-models-and-resolution.md @@ -63,7 +63,8 @@ config (not markers); `resolve_tools` owns the tool-specific mapping. // code: sandboxed code with its named secrets injected into env { "kind": "code", "name": "...", "runtime": "python", "code": "...", "env": { "API_KEY": "..." } } -// client: browser-fulfilled; filtered out of the runner's MCP tools/list +// client: browser-fulfilled; advertised to the model (incl. over the local Claude MCP channel), +// then PAUSED on call — never executed in the runner { "kind": "client", "name": "..." } ``` diff --git a/docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md b/docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md index dea53d8155..85f9702e88 100644 --- a/docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md +++ b/docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md @@ -133,7 +133,7 @@ Either form is valid: "input_schema": {}, "secrets": ["API_KEY"], "permission": null, "render": null } -// client: fulfilled by the browser; filtered out of the runner's MCP tools/list +// client: fulfilled by the browser; advertised to the model, then paused on call (not executed) { "type": "client", "name": "pick_file", "description": "...", "input_schema": {}, "permission": null, "render": null } From 618764edae67d253f43d3a1d7b55d31181aa7df1 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 4 Jul 2026 20:15:51 +0200 Subject: [PATCH 7/8] fix(runner): review fixes for the Claude client-tool channel Correctness fixes from the two review rounds (Codex xhigh + internal) on the client-tools recut: - Correlation index: normalize the ACP title's mcp____ prefix on record() so a bare-name lookup() hits (suppression + widget attachment were inert live: Claude titles internal-MCP tools mcp__agenta-tools__). - Consume-on-match: a lookup() consumes its matched id (per-key FIFO, one shared entry per call), so a duplicate identical call correlates to its OWN ACP id instead of re-homing the first, already-settled one. - Drop the ACP kind fallback in record(): kind is a category, not a name. - Single correlation owner: buildClientToolRelay resolves the id once; emitClientToolInteraction no longer takes an index. - Rename MCP_PARKED -> MCP_PAUSED (pause/pendingApproval vocabulary). - Move the ClientToolRelay contract to a pure type module (tools/client-tool-relay.ts); relay.ts keeps a compat re-export for now (its own hunks are entangled with a co-session's claimed relay.ts edits). - Comment honesty: the seam owns the RELAY/MCP emit while acp-interactions owns the ACP-gate emit; the MCP abort destroys sockets, not executions (signal-threading into dispatch is a known follow-up). - New tests: prefixed-title lookup, FIFO consumption, duplicate same-id POST after a pause is also aborted and never answered. Claude-Session: https://claude.ai/code/session_01HhBEUFbXETNjYdcz71AGrT --- .../src/engines/sandbox_agent/client-tools.ts | 104 +++++++++----- .../runner/src/engines/sandbox_agent/mcp.ts | 2 +- .../runner/src/tools/client-tool-relay.ts | 30 ++++ services/runner/src/tools/mcp-bridge.ts | 2 +- services/runner/src/tools/tool-mcp-http.ts | 30 ++-- .../runner/tests/unit/client-tools.test.ts | 128 +++++++++++++----- .../runner/tests/unit/tool-bridge.test.ts | 47 ++++++- 7 files changed, 257 insertions(+), 86 deletions(-) create mode 100644 services/runner/src/tools/client-tool-relay.ts diff --git a/services/runner/src/engines/sandbox_agent/client-tools.ts b/services/runner/src/engines/sandbox_agent/client-tools.ts index 0229d4c99d..6338990d16 100644 --- a/services/runner/src/engines/sandbox_agent/client-tools.ts +++ b/services/runner/src/engines/sandbox_agent/client-tools.ts @@ -20,7 +20,7 @@ import { approvedCallKey, type Responder } from "../../responder.ts"; import type { ClientToolRelay, ClientToolRelayRequest, -} from "../../tools/relay.ts"; +} from "../../tools/client-tool-relay.ts"; type EmitRun = { emitEvent: (event: AgentEvent) => void }; @@ -29,18 +29,53 @@ type EmitRun = { emitEvent: (event: AgentEvent) => void }; * tool-call id Claude surfaced on the event stream, so the paused `client_tool` interaction * attaches to Claude's actual tool-call bubble (and `markPausedToolCall` suppresses that * bubble's late teardown frames, the F-024 lineage). Populated from `session.onEvent` - * `tool_call` updates; the MCP-minted id / name+args is the cold-replay fallback when the - * stream had no matching call. Best-effort and first-write-wins (a later identical call never - * re-homes an id). + * `tool_call` updates. Claude's ACP adapter titles an internal-MCP tool + * `mcp__agenta-tools__` while `lookup()` is called with the bare spec name, so `record()` + * strips that prefix and indexes under the bare name. A `lookup()` match CONSUMES the id + * (per-key FIFO): a duplicate identical call correlates to its OWN recorded id, never re-homing + * a first, already-settled call's id. Best-effort; the MCP-minted id / name+args is the + * cold-replay fallback when the stream had no matching call. */ export interface ToolCallCorrelationIndex { record(update: unknown): void; lookup(toolName: string | undefined, input: unknown): string | undefined; } +/** + * Strip the harness's MCP tool prefix (`mcp____`) so an ACP title indexes under the + * bare spec name `lookup()` receives. The lazy match ends the prefix at the FIRST `__` after + * the server name, so a TOOL name that itself contains `__` survives intact (our server name, + * `agenta-tools`, contains no `__`; a server name that did would truncate ambiguously). + */ +function bareToolName(title: string): string { + return title.replace(/^mcp__.+?__/, ""); +} + export function createToolCallCorrelationIndex(): ToolCallCorrelationIndex { - const byArgsKey = new Map(); - const byName = new Map(); + // One entry object is shared by both maps, so consuming a match via either key retires it + // everywhere at once (no stale id left behind in the other queue). + type Entry = { id: string; consumed: boolean }; + const byArgsKey = new Map(); + const byName = new Map(); + const recordedIds = new Set(); + const push = (map: Map, key: string, entry: Entry): void => { + const queue = map.get(key); + if (queue) queue.push(entry); + else map.set(key, [entry]); + }; + const take = ( + map: Map, + key: string | undefined, + ): string | undefined => { + if (!key) return undefined; + for (const entry of map.get(key) ?? []) { + if (!entry.consumed) { + entry.consumed = true; + return entry.id; + } + } + return undefined; + }; return { record(update) { const u = update as @@ -48,35 +83,30 @@ export function createToolCallCorrelationIndex(): ToolCallCorrelationIndex { sessionUpdate?: unknown; toolCallId?: unknown; title?: unknown; - kind?: unknown; rawInput?: unknown; } | undefined; if (!u || u.sessionUpdate !== "tool_call") return; const toolCallId = typeof u.toolCallId === "string" && u.toolCallId ? u.toolCallId : undefined; - if (!toolCallId) return; + // Each ACP call is recorded once (a re-sent frame for the same id must not enqueue twice). + if (!toolCallId || recordedIds.has(toolCallId)) return; + // The name comes from the ACP `title` only. ACP `kind` is a CATEGORY (read/fetch/execute/ + // other), not a name — indexing under it could mis-correlate unrelated calls. const name = - typeof u.title === "string" && u.title - ? u.title - : typeof u.kind === "string" && u.kind - ? u.kind - : undefined; + typeof u.title === "string" && u.title ? bareToolName(u.title) : undefined; + if (!name) return; + recordedIds.add(toolCallId); + const entry: Entry = { id: toolCallId, consumed: false }; const argsKey = approvedCallKey(name, u.rawInput); - if (argsKey && !byArgsKey.has(argsKey)) byArgsKey.set(argsKey, toolCallId); - if (name && !byName.has(name)) byName.set(name, toolCallId); + if (argsKey) push(byArgsKey, argsKey, entry); + push(byName, name, entry); }, lookup(toolName, input) { - const argsKey = approvedCallKey(toolName, input); - if (argsKey) { - const hit = byArgsKey.get(argsKey); - if (hit) return hit; - } - if (toolName) { - const hit = byName.get(toolName); - if (hit) return hit; - } - return undefined; + return ( + take(byArgsKey, approvedCallKey(toolName, input)) ?? + take(byName, toolName ? toolName : undefined) + ); }, }; } @@ -84,7 +114,8 @@ export function createToolCallCorrelationIndex(): ToolCallCorrelationIndex { export interface ClientToolInteractionParams { /** The interaction id (the FE matches a reply by it). */ id: string; - /** The runner/relay-minted tool-call id; overridden by the correlated ACP id when one exists. */ + /** The tool-call id to attach to — already correlated by the caller (`buildClientToolRelay` + * resolves the real ACP id when an index is wired; otherwise the channel-minted id). */ toolCallId?: string; toolName?: string; input?: unknown; @@ -92,29 +123,28 @@ export interface ClientToolInteractionParams { } /** - * THE single definition of the `interaction_request kind=client_tool` payload. Emits both the - * top-level fields and a synthesized `toolCall` sub-object the Vercel egress reads (it tolerates - * either), and substitutes the correlated ACP tool-call id when the index has one. + * The `interaction_request kind=client_tool` emit for the RELAY/MCP delivery paths (Pi file + * relay + Claude internal MCP), owned by this seam. The ACP-gate path has its OWN emit site + * with a richer ACP-native payload (`acp-interactions.ts` `pauseClientTool`, which forwards the + * harness's toolCall object) — two sites, one per gate. This one emits both the top-level + * fields and a synthesized `toolCall` sub-object the Vercel egress reads (it tolerates either). */ export function emitClientToolInteraction( run: EmitRun, params: ClientToolInteractionParams, - toolCallIndex?: ToolCallCorrelationIndex, ): void { - const correlatedId = - toolCallIndex?.lookup(params.toolName, params.input) ?? params.toolCallId; run.emitEvent({ type: "interaction_request", id: params.id, kind: "client_tool", payload: { - toolCallId: correlatedId, + toolCallId: params.toolCallId, toolName: params.toolName, input: params.input, render: params.render, toolCall: { - id: correlatedId, - toolCallId: correlatedId, + id: params.toolCallId, + toolCallId: params.toolCallId, name: params.toolName, rawInput: params.input, input: params.input, @@ -149,7 +179,9 @@ export interface BuildClientToolRelayInput { toolArgs: unknown, kind: "user_approval" | "client_tool", ) => void; - /** Claude only: maps the call to its real ACP tool-call id. Omit for Pi (relay id is exact). */ + /** Non-Pi harness (Claude): maps the call to its real ACP tool-call id. Omit for Pi (the + * relay-minted id is already exact). The relay resolves the id ONCE per pending call and + * hands the result to `markPausedToolCall` and the emit — the single correlation owner. */ toolCallIndex?: ToolCallCorrelationIndex; log?: (message: string) => void; } diff --git a/services/runner/src/engines/sandbox_agent/mcp.ts b/services/runner/src/engines/sandbox_agent/mcp.ts index 5f2e8c1c06..837648d360 100644 --- a/services/runner/src/engines/sandbox_agent/mcp.ts +++ b/services/runner/src/engines/sandbox_agent/mcp.ts @@ -8,7 +8,7 @@ import { USER_MCP_UNSUPPORTED_MESSAGE, type McpServerStdio, } from "../../tools/mcp-bridge.ts"; -import type { ClientToolRelay } from "../../tools/relay.ts"; +import type { ClientToolRelay } from "../../tools/client-tool-relay.ts"; type Log = (message: string) => void; diff --git a/services/runner/src/tools/client-tool-relay.ts b/services/runner/src/tools/client-tool-relay.ts new file mode 100644 index 0000000000..8d949c6d25 --- /dev/null +++ b/services/runner/src/tools/client-tool-relay.ts @@ -0,0 +1,30 @@ +/** + * The client-tool relay contract — the seam a delivery channel pauses a browser-fulfilled + * `client` tool through. Pure types only (no runtime code): the implementation is built by + * `engines/sandbox_agent/client-tools.ts` (`buildClientToolRelay`), and it is consumed by the + * Pi file relay (`tools/relay.ts` `startToolRelay`) and the Claude internal loopback MCP + * server (`tools/tool-mcp-http.ts`). + */ +import type { ResolvedToolSpec } from "../protocol.ts"; +import type { ClientToolOutcome } from "../responder.ts"; + +/** One client tool call as the delivery channel saw it: public name + args + resolved spec. */ +export interface ClientToolRelayRequest { + /** The interaction id (the FE matches a reply by it). */ + id: string; + /** The channel-minted tool-call id (relay file id on Pi, a fresh UUID on the MCP channel). */ + toolCallId: string; + toolName: string; + input: unknown; + spec: ResolvedToolSpec; +} + +/** + * The relay itself. The consumer calls `onClientTool` for each `client` tool call and then, on + * a `pendingApproval` outcome, `onPause` to end the turn (the two-step shape mirrors the + * previous inline engine behavior, so Pi is unchanged). + */ +export interface ClientToolRelay { + onClientTool: (request: ClientToolRelayRequest) => Promise; + onPause?: (request: ClientToolRelayRequest) => void; +} diff --git a/services/runner/src/tools/mcp-bridge.ts b/services/runner/src/tools/mcp-bridge.ts index 1c1372ead6..59646f9ce8 100644 --- a/services/runner/src/tools/mcp-bridge.ts +++ b/services/runner/src/tools/mcp-bridge.ts @@ -22,7 +22,7 @@ */ import type { ResolvedToolSpec } from "../protocol.ts"; import type { McpServerHttp } from "../engines/sandbox_agent/mcp.ts"; -import type { ClientToolRelay } from "./relay.ts"; +import type { ClientToolRelay } from "./client-tool-relay.ts"; import { startInternalToolMcpServer } from "./tool-mcp-http.ts"; export type { ResolvedToolSpec, ToolCallbackContext } from "../protocol.ts"; diff --git a/services/runner/src/tools/tool-mcp-http.ts b/services/runner/src/tools/tool-mcp-http.ts index a041fe46d6..16e6b961ea 100644 --- a/services/runner/src/tools/tool-mcp-http.ts +++ b/services/runner/src/tools/tool-mcp-http.ts @@ -40,7 +40,7 @@ import type { AddressInfo } from "node:net"; import type { ResolvedToolSpec } from "../protocol.ts"; import { EMPTY_OBJECT_SCHEMA } from "./callback.ts"; import { runResolvedTool } from "./dispatch.ts"; -import type { ClientToolRelay } from "./relay.ts"; +import type { ClientToolRelay } from "./client-tool-relay.ts"; import { assertRequiredArguments, specInputSchema } from "./spec-schema.ts"; type Log = (message: string) => void; @@ -59,13 +59,16 @@ const MAX_BODY_BYTES = 1_000_000; * interaction (`onClientTool`) and the handler then ends the turn (`onPause` -> the engine's * pause controller), so the turn ends `paused`. */ -const MCP_PARKED = Symbol("mcp-parked"); +const MCP_PAUSED = Symbol("mcp-paused"); /** Options for the internal MCP server: the client-tool relay and an engine abort signal. */ export interface InternalToolMcpServerOptions { /** When set, a `client` tool call is paused through this relay instead of relayed/executed. */ clientToolRelay?: ClientToolRelay; - /** Fired by the engine on pause/teardown; destroys any in-flight request so none settles late. */ + /** Fired by the engine on pause/teardown; destroys any in-flight request SOCKET so no + * response settles late. It does NOT cancel the execution: a `runResolvedTool` dispatch + * already running keeps running server-side to completion (its result is just never + * written). Threading this signal into dispatch is a known follow-up. */ signal?: AbortSignal; log?: Log; } @@ -91,7 +94,7 @@ function mcpToolError(id: unknown, err: unknown): unknown { /** * Handle one MCP JSON-RPC message. Returns the JSON-RPC response object, `undefined` for a - * notification (no `id`), or the `MCP_PARKED` sentinel for a paused client tool (the listener + * notification (no `id`), or the `MCP_PAUSED` sentinel for a paused client tool (the listener * then aborts the request with no body). Takes the specs and relay dir in-process rather than * from env, and dispatches a non-`client` `tools/call` to `runResolvedTool`. */ @@ -102,7 +105,7 @@ async function handle( relayDir: string, clientToolRelay: ClientToolRelay | undefined, log: Log, -): Promise { +): Promise { const { id, method, params } = message ?? {}; // Notifications (no id, e.g. notifications/initialized) need no response. @@ -184,8 +187,8 @@ async function handle( const decision = await clientToolRelay.onClientTool(request); if (decision === "pendingApproval") { clientToolRelay.onPause?.(request); - // No JSON-RPC result: the request listener aborts this in-flight request (see MCP_PARKED). - return MCP_PARKED; + // No JSON-RPC result: the request listener aborts this in-flight request (see MCP_PAUSED). + return MCP_PAUSED; } if (decision === "deny") { return mcpToolError(id, new Error(`Client tool '${spec.name}' was denied.`)); @@ -278,7 +281,7 @@ export function startInternalToolMcpServer( const active = new Set(); /** Abort a paused request: destroy the socket with no body written, so nothing settles late. */ - const abortParked = (res: ServerResponse): void => { + const abortPaused = (res: ServerResponse): void => { active.delete(res); res.destroy(); }; @@ -319,8 +322,8 @@ export function startInternalToolMcpServer( ), ); // A paused client tool in the batch aborts the whole request (no result for any). - if (responses.some((r) => r === MCP_PARKED)) { - abortParked(res); + if (responses.some((r) => r === MCP_PAUSED)) { + abortPaused(res); return; } const out = responses.filter((r) => r !== undefined); @@ -342,9 +345,9 @@ export function startInternalToolMcpServer( clientToolRelay, log, ); - if (response === MCP_PARKED) { + if (response === MCP_PAUSED) { // Paused client tool: emit NO JSON-RPC result, abort the in-flight request. - abortParked(res); + abortPaused(res); return; } if (response === undefined) { @@ -377,6 +380,9 @@ export function startInternalToolMcpServer( // Belt and suspenders: on pause/teardown the engine fires this signal; destroy every in-flight // request so a handler that has not yet returned cannot write a result after the turn ended. + // SOCKETS only: a `runResolvedTool` execution already dispatched keeps running server-side — + // this abort suppresses its response, it does not stop it (signal-threading into dispatch is a + // known follow-up). const onAbort = (): void => { for (const res of [...active]) res.destroy(); active.clear(); diff --git a/services/runner/tests/unit/client-tools.test.ts b/services/runner/tests/unit/client-tools.test.ts index 87fac7b1c0..90d94ca356 100644 --- a/services/runner/tests/unit/client-tools.test.ts +++ b/services/runner/tests/unit/client-tools.test.ts @@ -11,7 +11,7 @@ import assert from "node:assert/strict"; import type { AgentEvent } from "../../src/protocol.ts"; import type { ClientToolVerdict, Responder } from "../../src/responder.ts"; -import type { ClientToolRelayRequest } from "../../src/tools/relay.ts"; +import type { ClientToolRelayRequest } from "../../src/tools/client-tool-relay.ts"; import { PendingApprovalLatch } from "../../src/permission-plan.ts"; import { buildClientToolRelay, @@ -77,7 +77,49 @@ describe("createToolCallCorrelationIndex", () => { "acp-real-1", "name+args resolves the real id", ); - // Bare-name fallback when the args differ but the name matched a recorded call. + }); + + it("normalizes the mcp____ title prefix so a bare-name lookup hits (Claude ACP)", () => { + // Claude's ACP adapter titles an internal-MCP tool `mcp__agenta-tools__`, but + // lookup() is called with the bare spec name — without normalization every lookup missed + // and the minted UUID fallback always won (making suppression + widget attachment inert). + const index = createToolCallCorrelationIndex(); + index.record({ + sessionUpdate: "tool_call", + toolCallId: "acp-real-1", + title: "mcp__agenta-tools__request_connection", + rawInput: { integration: "slack" }, + }); + assert.equal( + index.lookup("request_connection", { integration: "slack" }), + "acp-real-1", + "the prefixed title indexes under the bare name (args match)", + ); + }); + + it("prefix normalization survives a tool name that itself contains __", () => { + const index = createToolCallCorrelationIndex(); + index.record({ + sessionUpdate: "tool_call", + toolCallId: "acp-real-1", + title: "mcp__agenta-tools__my__tool", + rawInput: {}, + }); + assert.equal( + index.lookup("my__tool", {}), + "acp-real-1", + "the lazy prefix strip ends at the FIRST __ after the server name", + ); + }); + + it("falls back to the bare name when the args differ", () => { + const index = createToolCallCorrelationIndex(); + index.record({ + sessionUpdate: "tool_call", + toolCallId: "acp-real-1", + title: "request_connection", + rawInput: { integration: "slack" }, + }); assert.equal( index.lookup("request_connection", { integration: "github" }), "acp-real-1", @@ -85,13 +127,49 @@ describe("createToolCallCorrelationIndex", () => { ); }); - it("ignores non-tool_call updates and is first-write-wins", () => { + it("consumes a matched id: two identical calls correlate to id-1 then id-2, then miss", () => { + // First-write-wins would correlate a duplicate identical call to the FIRST call's ACP id + // (already settled), mis-marking suppression. A match consumes its id instead (per-key FIFO, + // symmetric with the client-output FIFO). + const index = createToolCallCorrelationIndex(); + index.record({ sessionUpdate: "tool_call", toolCallId: "id-1", title: "t", rawInput: {} }); + index.record({ sessionUpdate: "tool_call", toolCallId: "id-2", title: "t", rawInput: {} }); + assert.equal(index.lookup("t", {}), "id-1", "first lookup takes the first id"); + assert.equal(index.lookup("t", {}), "id-2", "second lookup takes the second id"); + assert.equal(index.lookup("t", {}), undefined, "both consumed -> miss"); + }); + + it("a consume via the args key also retires the id from the name queue", () => { + const index = createToolCallCorrelationIndex(); + index.record({ + sessionUpdate: "tool_call", + toolCallId: "id-1", + title: "t", + rawInput: { a: 1 }, + }); + assert.equal(index.lookup("t", { a: 1 }), "id-1", "consumed via name+args"); + assert.equal( + index.lookup("t", { b: 2 }), + undefined, + "the name fallback must not resurrect a consumed id", + ); + }); + + it("ignores non-tool_call updates and re-sent frames for the same id", () => { const index = createToolCallCorrelationIndex(); index.record({ sessionUpdate: "agent_message_chunk", text: "hi" }); assert.equal(index.lookup("x", {}), undefined, "no record -> no id"); index.record({ sessionUpdate: "tool_call", toolCallId: "id-1", title: "t" }); - index.record({ sessionUpdate: "tool_call", toolCallId: "id-2", title: "t" }); - assert.equal(index.lookup("t", {}), "id-1", "first write wins"); + index.record({ sessionUpdate: "tool_call", toolCallId: "id-1", title: "t" }); + assert.equal(index.lookup("t", {}), "id-1"); + assert.equal(index.lookup("t", {}), undefined, "a re-sent frame does not enqueue twice"); + }); + + it("does NOT index under the ACP kind (a category, not a name)", () => { + // `kind` is read/fetch/execute/other; indexing under it mis-correlated unrelated calls. + const index = createToolCallCorrelationIndex(); + index.record({ sessionUpdate: "tool_call", toolCallId: "id-1", kind: "execute" }); + assert.equal(index.lookup("execute", {}), undefined, "kind-only frames are not indexed"); }); }); @@ -126,39 +204,19 @@ describe("emitClientToolInteraction", () => { assert.equal(ev.payload.toolCall.kind, "client"); }); - it("substitutes the correlated ACP id when the index has one", () => { + it("emits exactly the id it is given (correlation is the relay's job, not the emitter's)", () => { + // buildClientToolRelay resolves the correlated ACP id ONCE and passes it in; the emitter + // has no index path of its own (the relay-level test below covers the substitution). const { run, events } = collect(); - const index = createToolCallCorrelationIndex(); - index.record({ - sessionUpdate: "tool_call", - toolCallId: "acp-real", - title: "request_connection", - rawInput: { integration: "slack" }, + emitClientToolInteraction(run, { + id: "i-1", + toolCallId: "already-correlated", + toolName: "request_connection", + input: {}, }); - emitClientToolInteraction( - run, - { - id: "i-1", - toolCallId: "minted-fallback", - toolName: "request_connection", - input: { integration: "slack" }, - }, - index, - ); const ev = events[0] as any; - assert.equal(ev.payload.toolCallId, "acp-real", "correlated id wins over the minted one"); - assert.equal(ev.payload.toolCall.id, "acp-real"); - }); - - it("falls back to the minted id when the index has no match", () => { - const { run, events } = collect(); - const index = createToolCallCorrelationIndex(); // empty - emitClientToolInteraction( - run, - { id: "i-1", toolCallId: "minted", toolName: "request_connection", input: {} }, - index, - ); - assert.equal((events[0] as any).payload.toolCallId, "minted"); + assert.equal(ev.payload.toolCallId, "already-correlated"); + assert.equal(ev.payload.toolCall.id, "already-correlated"); }); }); diff --git a/services/runner/tests/unit/tool-bridge.test.ts b/services/runner/tests/unit/tool-bridge.test.ts index b20f46bae8..81317516d4 100644 --- a/services/runner/tests/unit/tool-bridge.test.ts +++ b/services/runner/tests/unit/tool-bridge.test.ts @@ -33,8 +33,8 @@ import { import { RELAY_REQ_SUFFIX, RELAY_RES_SUFFIX, - type ClientToolRelay, } from "../../src/tools/relay.ts"; +import type { ClientToolRelay } from "../../src/tools/client-tool-relay.ts"; import type { ResolvedToolSpec } from "../../src/protocol.ts"; const relayDir = "/tmp/agenta-tools"; @@ -393,6 +393,51 @@ describe("buildToolMcpServers (internal gateway-tool channel)", () => { assert.equal(pauseCount, 1, "onPause fired exactly once"); }); + it("a duplicate POST with the same JSON-RPC id after a pause is also aborted, never answered", async () => { + // Pins the no-retry assumption at the HANDLER level: if the MCP client ever re-sent a + // destroyed tools/call (same JSON-RPC id), the duplicate must get the same no-body abort — + // each POST independently consults the relay, and nothing is answered from cached state, + // so a duplicate can never double-consume a stored browser output. + let onClientToolCalls = 0; + let outputsServed = 0; + const relay: ClientToolRelay = { + onClientTool: async () => { + onClientToolCalls += 1; + // The paused turn has no stored output; every ask pauses. If the handler ever served + // a result for the duplicate anyway, outputsServed would flag it below. + return "pendingApproval"; + }, + onPause: () => {}, + }; + const { servers } = await build([clientSpec], relayDir, { + clientToolRelay: relay, + }); + const post = async (): Promise => { + const res = await fetch(servers[0].url, { + method: "POST", + headers: { + "content-type": "application/json", + accept: "application/json, text/event-stream", + }, + body: JSON.stringify({ + jsonrpc: "2.0", + id: 7, // the SAME JSON-RPC id both times (a retry, not a new call) + method: "tools/call", + params: { + name: "request_connection", + arguments: { integration: "slack" }, + }, + }), + }); + outputsServed += 1; // only reachable if a body was actually answered + await res.text(); + }; + await assert.rejects(post, "the first paused tools/call is aborted"); + await assert.rejects(post, "the duplicate (same id) is aborted too"); + assert.equal(onClientToolCalls, 2, "each POST consults the relay independently"); + assert.equal(outputsServed, 0, "neither request was ever answered with a result"); + }); + it("validates required args in the client branch (a normal MCP error, not a pause)", async () => { let pauseCount = 0; const relay: ClientToolRelay = { From 51f0e3f2a3c9d0cc314dbb7082ff4d785d123a02 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Sat, 4 Jul 2026 20:18:11 +0200 Subject: [PATCH 8/8] fix(sdk): render Claude permission rules for client tools (allow unless denied) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _rules_from_tool_specs skipped kind == "client" from the days when client tools never crossed the agenta-tools channel. On this branch they DO: the runner advertises them over the internal MCP server and pauses tools/call for the browser. Without a rule, Claude's own permission gate fires first and the call falls to the ACP path, bypassing the new pause path. Client tools now render mcp__agenta-tools__ rules: deny when the effective permission is deny, otherwise allow — including for explicit "ask" and for unset. The runner-side seam is the authoritative gate for a client tool: pausing for the browser IS the ask flow, so a Claude-side ask would only duplicate that gate in a worse place. Executable (callback/code) behavior is unchanged. Tests: allow-by-default, allow-on-ask, deny-on-deny, plain-dict client renders its rule; non-client assertions untouched. Claude-Session: https://claude.ai/code/session_01HhBEUFbXETNjYdcz71AGrT --- .../sdk/agents/adapters/claude_settings.py | 44 ++++++++++++----- .../agents/adapters/test_claude_settings.py | 49 ++++++++++++++++--- 2 files changed, 74 insertions(+), 19 deletions(-) diff --git a/sdks/python/agenta/sdk/agents/adapters/claude_settings.py b/sdks/python/agenta/sdk/agents/adapters/claude_settings.py index 4290bab9ac..b57cf80615 100644 --- a/sdks/python/agenta/sdk/agents/adapters/claude_settings.py +++ b/sdks/python/agenta/sdk/agents/adapters/claude_settings.py @@ -26,8 +26,10 @@ that honors ``allow``. Emitting an ``allow`` rule here is the only way an ``allow`` tool actually runs on Claude instead of always parking. ``ask``/unset emits no allow rule (the gate stays raised -> HITL park preserved); ``deny`` emits a deny rule (which also closes a local-Claude execution -gap). ``client`` tools are browser-fulfilled, never delivered over this channel, so they are -excluded. The runner policy supplies the default permission when a tool has no explicit value. +gap). ``client`` tools are browser-fulfilled but ARE delivered over this same channel (the runner +advertises them on ``agenta-tools`` and pauses the ``tools/call``), so they get a rule too — +allow unless denied; see :func:`_rules_from_tool_specs`. The runner policy supplies the default +permission when a tool has no explicit value. """ from __future__ import annotations @@ -131,18 +133,28 @@ def _rules_from_mcp_permissions(mcp_servers: Any) -> Dict[str, List[str]]: def _rules_from_tool_specs( tool_specs: Any, permission_default: PermissionMode ) -> Dict[str, List[str]]: - """Derive per-tool Claude rules from each resolved EXECUTABLE tool's Layer-3 ``permission`` (F-046). + """Derive per-tool Claude rules from each resolved tool's Layer-3 ``permission`` (F-046). Mirrors :func:`_rules_from_mcp_permissions`, but per-tool against the fixed internal server name - ``agenta-tools``: a callback/code tool is delivered to Claude as a tool of that MCP server, so + ``agenta-tools``: a resolved tool is delivered to Claude as a tool of that MCP server, so its rule is ``mcp__agenta-tools__``. The standalone :func:`~agenta.sdk.agents.tools.models.effective_permission` ladder (explicit permission, - else read-only under ``allow_reads``, else the runner mode) routes it to the matching list. Unset tools - only render a rule when the runner mode needs an explicit Claude allow/deny rule. ``client`` - tools are browser-fulfilled and never delivered over this channel, so they are excluded (this - mirrors the runner's ``mcp-bridge`` filter). Accepts a list - of :class:`~agenta.sdk.agents.tools.models.ToolSpec` or plain dicts (coerced to a spec so the - same permission ladder applies). + else read-only under ``allow_reads``, else the runner mode) routes an EXECUTABLE + (callback/code) tool to the matching list. Unset executable tools only render a rule when the + runner mode needs an explicit Claude allow/deny rule. + + ``client`` tools (browser-fulfilled, e.g. ``request_connection``) ride this SAME channel: + the runner advertises them on ``agenta-tools`` and pauses their ``tools/call`` for the + browser. Their rule is **deny when the effective permission is deny, otherwise allow** — + including for an explicit ``ask`` and for unset. The runner-side pause seam is the + authoritative gate for a client tool: pausing for the browser IS the ask flow, so a + Claude-side ask rule would only duplicate that gate in a worse place (Claude's own prompt + fires before the runner ever sees the call, bypassing the pause path). Without an allow rule + the same thing happens: Claude's permission gate fires first and the call falls to the ACP + path instead of pausing over MCP. + + Accepts a list of :class:`~agenta.sdk.agents.tools.models.ToolSpec` or plain dicts (coerced + to a spec so the same permission ladder applies). """ # Lazy import: ``tools.models`` does not import this adapter, but keeping the import local # avoids loading the tool models when the claude adapter is used without resolved tools. @@ -157,14 +169,20 @@ def _rules_from_tool_specs( except Exception: # A malformed/nameless spec contributes nothing (mirrors the MCP helper's name guard). continue - if spec.kind == "client": - continue permission = effective_permission( spec.permission, spec.read_only, permission_default ) + rule = f"mcp__{INTERNAL_TOOL_MCP_SERVER}__{spec.name}" + if spec.kind == "client": + # Deny stays deny; everything else (allow, explicit ask, unset) renders allow: the + # runner pause seam is the authoritative ask for a client tool (see the docstring). + if permission == "deny": + deny.append(rule) + else: + allow.append(rule) + continue if spec.permission is None and permission == "ask": continue - rule = f"mcp__{INTERNAL_TOOL_MCP_SERVER}__{spec.name}" if permission == "allow": allow.append(rule) elif permission == "ask": diff --git a/sdks/python/oss/tests/pytest/unit/agents/adapters/test_claude_settings.py b/sdks/python/oss/tests/pytest/unit/agents/adapters/test_claude_settings.py index 3490d13392..378954209a 100644 --- a/sdks/python/oss/tests/pytest/unit/agents/adapters/test_claude_settings.py +++ b/sdks/python/oss/tests/pytest/unit/agents/adapters/test_claude_settings.py @@ -313,11 +313,48 @@ def test_deny_tool_renders_deny_rule(): assert "allow" not in perms -def test_client_tool_excluded(): - # `client` tools are browser-fulfilled, never delivered over the `agenta-tools` channel, so - # they contribute no rule even with an explicit `allow`. +def test_client_tool_allow_renders_allow_rule(): + # `client` tools now ride the same `agenta-tools` channel (the runner advertises them and + # pauses their tools/call for the browser), so an explicit `allow` renders an allow rule — + # without it Claude's own gate fires first and the pause path is bypassed. spec = ClientToolSpec(name="ui_pick", description="d", permission="allow") - assert build_claude_settings_files(None, None, None, [spec]) == [] + perms = _settings(build_claude_settings_files(None, None, None, [spec]))[ + "permissions" + ] + assert perms["allow"] == [_rule("ui_pick")] + assert "ask" not in perms + assert "deny" not in perms + + +def test_client_tool_unset_renders_allow_rule(): + # Unset -> allow too: the runner-side pause seam is the authoritative gate (pausing for the + # browser IS the ask flow), so the Claude gate must stand down by default. + spec = ClientToolSpec(name="request_connection", description="d") + perms = _settings(build_claude_settings_files(None, None, None, [spec]))[ + "permissions" + ] + assert perms["allow"] == [_rule("request_connection")] + + +def test_client_tool_ask_renders_allow_rule(): + # An explicit `ask` ALSO renders allow (not an ask rule): a Claude-side ask would duplicate + # the runner's pause gate in a worse place — the pause is the ask for a client tool. + spec = ClientToolSpec(name="ui_pick", description="d", permission="ask") + perms = _settings(build_claude_settings_files(None, None, None, [spec]))[ + "permissions" + ] + assert perms["allow"] == [_rule("ui_pick")] + assert "ask" not in perms + + +def test_client_tool_deny_renders_deny_rule(): + # Deny stays deny — the one verdict Claude should enforce before the runner is reached. + spec = ClientToolSpec(name="ui_pick", description="d", permission="deny") + perms = _settings(build_claude_settings_files(None, None, None, [spec]))[ + "permissions" + ] + assert perms["deny"] == [_rule("ui_pick")] + assert "allow" not in perms def test_tool_rules_merge_with_author_and_mcp(): @@ -347,7 +384,7 @@ def test_tool_rules_merge_with_author_and_mcp(): def test_tool_rules_accept_plain_dicts(): # The builder coerces plain wire dicts so the same permission ladder applies; a `client` dict - # is excluded. + # renders its allow-by-default rule alongside the executable tool's derived allow. perms = _settings( build_claude_settings_files( None, @@ -365,4 +402,4 @@ def test_tool_rules_accept_plain_dicts(): ], ) )["permissions"] - assert perms["allow"] == [_rule("get_user")] + assert perms["allow"] == [_rule("get_user"), _rule("ui_pick")]