diff --git a/sdks/python/oss/tests/pytest/unit/agents/test_ui_messages.py b/sdks/python/oss/tests/pytest/unit/agents/test_ui_messages.py index 4594bf2b3d..ae91e04820 100644 --- a/sdks/python/oss/tests/pytest/unit/agents/test_ui_messages.py +++ b/sdks/python/oss/tests/pytest/unit/agents/test_ui_messages.py @@ -319,6 +319,44 @@ async def test_permission_interaction_becomes_approval_request(self): assert synth["toolCallId"] == "call_1" assert synth["toolName"] == "deleteFile" + async def test_parked_gate_emits_only_approval_request_no_error_output(self): + # The "park does not clobber" contract (F-024): once the runner stops replying `reject` + # on a parked gate, a turn's events are [tool_call, interaction_request(permission), + # done] with NO error tool_result. The egress must then emit exactly one + # tool-approval-request for the tool and NO tool-output-error/-denied on the same id, so + # the approval prompt is the last word on the tool part. + run = _run( + events=[ + { + "type": "tool_call", + "id": "call_5", + "name": "deleteFile", + "input": {"path": "/x"}, + }, + { + "type": "interaction_request", + "id": "perm_5", + "kind": "permission", + "payload": { + "toolCallId": "call_5", + "availableReplies": ["once", "always", "reject"], + "toolCall": {"toolCallId": "call_5", "name": "deleteFile"}, + }, + }, + {"type": "done"}, + ], + result={"output": ""}, + ) + parts = await _collect(run, session_id="s1") + approvals = [p for p in parts if p["type"] == "tool-approval-request"] + assert len(approvals) == 1 + assert approvals[0]["toolCallId"] == "call_5" + # No error/denied part clobbers the approval prompt for this tool call. + for kind in ("tool-output-error", "tool-output-denied"): + assert all( + p.get("toolCallId") != "call_5" for p in parts if p["type"] == kind + ) + async def test_permission_tool_call_id_falls_back_to_nested_tool_call(self): # No top-level toolCallId on the payload: dig it out of the nested ACP toolCall detail. run = _run( diff --git a/services/agent/package.json b/services/agent/package.json index c33fb3db40..d17d1f8cd3 100644 --- a/services/agent/package.json +++ b/services/agent/package.json @@ -28,7 +28,8 @@ "@opentelemetry/semantic-conventions": "1.28.0", "@zed-industries/claude-agent-acp": "^0.23.1", "pi-acp": "0.0.29", - "sandbox-agent": "0.4.2" + "sandbox-agent": "0.4.2", + "undici": "8.3.0" }, "devDependencies": { "@types/node": "^24.0.0", diff --git a/services/agent/pnpm-lock.yaml b/services/agent/pnpm-lock.yaml index 62bde1acb0..51ea22acd4 100644 --- a/services/agent/pnpm-lock.yaml +++ b/services/agent/pnpm-lock.yaml @@ -41,6 +41,9 @@ importers: sandbox-agent: specifier: 0.4.2 version: 0.4.2(@daytonaio/sdk@0.187.0(ws@8.21.0))(zod@4.4.3) + undici: + specifier: 8.3.0 + version: 8.3.0 devDependencies: '@types/node': specifier: ^24.0.0 diff --git a/services/agent/src/engines/sandbox_agent.ts b/services/agent/src/engines/sandbox_agent.ts index 8f45440fa2..311c650ef6 100644 --- a/services/agent/src/engines/sandbox_agent.ts +++ b/services/agent/src/engines/sandbox_agent.ts @@ -51,6 +51,7 @@ import { assertRequiredCapabilities, probeCapabilities, } from "./sandbox_agent/capabilities.ts"; +import { createAcpFetch } from "./sandbox_agent/acp-fetch.ts"; import { buildDaemonEnv, resolveDaemonBinary } from "./sandbox_agent/daemon.ts"; import { createCookieFetch, @@ -139,6 +140,7 @@ export interface SandboxAgentDeps extends BuildRunPlanDeps { resolveDaemonBinary?: typeof resolveDaemonBinary; buildSandboxProvider?: typeof buildSandboxProvider; createCookieFetch?: typeof createCookieFetch; + createAcpFetch?: typeof createAcpFetch; prepareWorkspace?: typeof prepareWorkspace; probeCapabilities?: typeof probeCapabilities; applyModel?: typeof applyModel; @@ -235,11 +237,14 @@ export async function runSandboxAgent( // Propagate caller cancellation (a client disconnect on the streaming HTTP edge) so an // in-flight run aborts instead of finishing unobserved. The `finally` still disposes. ...(signal ? { signal } : {}), - // Daytona's preview proxy authenticates with a per-sandbox cookie; carry it across - // requests so ACP calls after the first don't 401. Harmless for local. - ...(plan.isDaytona - ? { fetch: (deps.createCookieFetch ?? createCookieFetch)() } - : {}), + // Drive the ACP HTTP client through a long-timeout undici dispatcher so a parked HITL + // turn (the connection held open while a human approves a tool) is NOT reaped by + // undici's default `headersTimeout` (which would kill it with UND_ERR_HEADERS_TIMEOUT). + // Daytona additionally needs the per-sandbox auth cookie carried across requests, so it + // uses the cookie fetch — which itself layers on the same long-timeout ACP dispatcher. + fetch: plan.isDaytona + ? (deps.createCookieFetch ?? createCookieFetch)() + : (deps.createAcpFetch ?? createAcpFetch)(), }); // On Daytona, push the harness login, the extension, and AGENTS.md into the remote diff --git a/services/agent/src/engines/sandbox_agent/acp-fetch.ts b/services/agent/src/engines/sandbox_agent/acp-fetch.ts new file mode 100644 index 0000000000..8ac35fa363 --- /dev/null +++ b/services/agent/src/engines/sandbox_agent/acp-fetch.ts @@ -0,0 +1,51 @@ +import { Agent, fetch as undiciFetch } from "undici"; + +/** + * HITL parks the ACP HTTP connection open for human-timescale delays: when a tool call needs + * approval, the runner holds the in-flight `prompt` request while it waits for the human to + * click Approve/Deny, then resumes the same parked turn. Node's global `fetch` (undici) ships + * a DEFAULT `headersTimeout` (~5 min) and `bodyTimeout`; once it fires undici calls + * `failReadable()` and the ACP stream dies with `UND_ERR_HEADERS_TIMEOUT`, killing both the + * parked turn and the resume turn. A plain chat completes in seconds so it never trips this. + * + * The fix is to drive the ACP HTTP client through an undici dispatcher whose timeouts are + * disabled (0) or set to a long park window, instead of the default. We scope it to the ACP + * fetch the `sandbox-agent` SDK uses rather than touching the global dispatcher, so unrelated + * HTTP keeps its safe defaults. + */ + +/** Disabled by default (0 = no timeout). Override with a millisecond value if a bound is wanted. */ +function envTimeoutMs(name: string): number { + const raw = process.env[name]; + if (raw === undefined || raw === "") return 0; + const parsed = Number(raw); + return Number.isFinite(parsed) && parsed >= 0 ? parsed : 0; +} + +/** + * Build the long-timeout undici dispatcher used for ACP HTTP. `headersTimeout` is the one that + * reaps a parked turn (no response headers arrive while the human deliberates); `bodyTimeout` + * guards the streamed body. Both default to disabled so a park held for any human-timescale + * delay is never reaped. `keepAliveTimeout`/`keepAliveMaxTimeout` are raised so the connection + * is not pooled-closed mid-park either. + */ +export function createAcpDispatcher(): Agent { + const headersTimeout = envTimeoutMs("SANDBOX_AGENT_ACP_HEADERS_TIMEOUT_MS"); + const bodyTimeout = envTimeoutMs("SANDBOX_AGENT_ACP_BODY_TIMEOUT_MS"); + return new Agent({ + headersTimeout, + bodyTimeout, + keepAliveTimeout: 600_000, + keepAliveMaxTimeout: 600_000, + }); +} + +/** + * A `fetch` for the ACP HTTP client backed by {@link createAcpDispatcher}. We use undici's own + * `fetch` so the `dispatcher` option is honored regardless of how the global dispatcher is set. + * The `sandbox-agent` SDK accepts a custom `fetch`; we hand it this one on every path. + */ +export function createAcpFetch(dispatcher: Agent = createAcpDispatcher()): typeof fetch { + return ((input: any, init?: any) => + undiciFetch(input, { ...init, dispatcher })) as unknown as typeof fetch; +} diff --git a/services/agent/src/engines/sandbox_agent/daytona.ts b/services/agent/src/engines/sandbox_agent/daytona.ts index 5318f131bf..4495bdadbf 100644 --- a/services/agent/src/engines/sandbox_agent/daytona.ts +++ b/services/agent/src/engines/sandbox_agent/daytona.ts @@ -1,6 +1,7 @@ import { existsSync, readFileSync } from "node:fs"; import { join } from "node:path"; +import { createAcpFetch } from "./acp-fetch.ts"; import { uploadPiExtensionToSandbox, uploadSkillsToSandbox, @@ -156,8 +157,11 @@ export async function prepareDaytonaPiAssets({ * `daytona-sandbox-auth-*` cookie set on the first response; Node's fetch keeps no cookie * jar, so without this the proxy rejects later ACP requests with "Authentication * required" / 502. The sandbox-agent SDK accepts a custom fetch, so we hand it this one. + * + * It layers on {@link createAcpFetch} (the long-timeout ACP dispatcher) so a parked HITL turn + * over Daytona is not reaped by undici's default `headersTimeout` either. */ -export function createCookieFetch(): typeof fetch { +export function createCookieFetch(inner: typeof fetch = createAcpFetch()): typeof fetch { const jar = new Map>(); // host -> (name -> "name=value") return async (input: any, init?: any) => { const url = new URL(typeof input === "string" ? input : input.url); @@ -170,7 +174,7 @@ export function createCookieFetch(): typeof fetch { if (existing) merged.unshift(existing); headers.set("cookie", merged.join("; ")); } - const response = await fetch(input, { ...init, headers }); + const response = await inner(input, { ...init, headers }); const setCookies = typeof (response.headers as any).getSetCookie === "function" ? (response.headers as any).getSetCookie() diff --git a/services/agent/src/engines/sandbox_agent/permissions.ts b/services/agent/src/engines/sandbox_agent/permissions.ts index fbde0a6017..2155c1a990 100644 --- a/services/agent/src/engines/sandbox_agent/permissions.ts +++ b/services/agent/src/engines/sandbox_agent/permissions.ts @@ -38,7 +38,16 @@ export function attachPermissionResponder({ .onPermission({ id, availableReplies, raw: req }) .then((decision) => { if (!req?.id) return; - return session.respondPermission(req.id, decisionToReply(decision, availableReplies) as any); + // PARK (cross-turn HITL): send NO reply. The `interaction_request` above is the last + // word on this tool call; the harness ends the turn with the tool PENDING and the next + // turn's stored decision resolves it. Replying `reject` here would make Claude emit a + // failed tool call ("User refused permission") that clobbers the approval prompt on the + // same tool-call id (F-024) — do NOT map park onto a reply. + if (decision === "park") return; + return session.respondPermission( + req.id, + decisionToReply(decision, availableReplies) as any, + ); }) .catch(() => {}); }); diff --git a/services/agent/src/engines/sandbox_agent/run-plan.ts b/services/agent/src/engines/sandbox_agent/run-plan.ts index fa8d7d978a..8973eb2c78 100644 --- a/services/agent/src/engines/sandbox_agent/run-plan.ts +++ b/services/agent/src/engines/sandbox_agent/run-plan.ts @@ -11,6 +11,7 @@ import { resolvePromptText, } from "../../protocol.ts"; import { executableToolSpecs } from "../../tools/public-spec.ts"; +import { CODE_TOOL_UNSUPPORTED_MESSAGE } from "../../tools/code.ts"; import { USER_MCP_UNSUPPORTED_MESSAGE } from "../../tools/mcp-bridge.ts"; import { type MaterializedSkill, @@ -118,6 +119,15 @@ function hasStdioMcpServer(servers: McpServerConfig[] | undefined): boolean { ); } +/** + * True when any resolved tool is a `code` tool. Code execution was removed for security + * (F-010); the sidecar must refuse a run that carries one rather than advertise it and then + * launder a per-call rejection into a "successful" reply (F-016). + */ +function hasCodeTool(specs: ResolvedToolSpec[]): boolean { + return specs.some((spec) => spec.kind === "code"); +} + function defaultLocalCwd(): string { return mkdtempSync(join(tmpdir(), "agenta-sandbox-agent-")); } @@ -192,6 +202,16 @@ export function buildRunPlan( return { ok: false, error: LOCAL_NETWORK_UNSUPPORTED_MESSAGE }; } + // Code tools were removed (F-010 security): the sidecar no longer executes author-supplied + // snippets. `runCodeTool` throws per-call, but a per-call throw becomes a tool RESULT the + // model launders into an `ok:true` reply ("Code tools are not supported by the sidecar."), + // so a removed capability reads as a SUCCESS at the response envelope (F-016). Fail loud + // up-front instead: refuse any run that carries a `code` tool, the way stdio MCP is gated. + // Keep the wire shape; the delivery is not supported. + if (hasCodeTool(toolSpecs)) { + return { ok: false, error: CODE_TOOL_UNSUPPORTED_MESSAGE }; + } + // stdio MCP servers run as arbitrary processes on the RUNNER HOST, outside the sandbox // boundary, and the sidecar's stdio MCP implementation is disabled (parity with the removed // code execution) until its security is fixed. Refuse any run carrying one, the way code diff --git a/services/agent/src/responder.ts b/services/agent/src/responder.ts index ea2fe92e0f..a2bc44c6d0 100644 --- a/services/agent/src/responder.ts +++ b/services/agent/src/responder.ts @@ -23,8 +23,27 @@ import type { AgentRunRequest, ContentBlock } from "./protocol.ts"; export type PermissionPolicy = "auto" | "deny"; +/** + * What the responder decides for one permission gate. + * + * - `allow` / `deny` are terminal: the adapter maps them onto an ACP reply via + * `decisionToReply` and the harness runs or refuses the tool this turn. + * - `park` is NOT a harness reply. It means "a human must decide; end the turn with this + * tool PENDING". On park the adapter sends NO `respondPermission`, so the harness never + * produces a refused/failed tool call, and the `interaction_request` already emitted stays + * the last word on the tool call. The next turn carries the stored decision and resolves it + * via `allow`/`deny`. This is the cross-turn HITL "park" — see `HITLResponder`. + * + * `decisionToReply` only ever sees `allow`/`deny`; `park` is handled before it (it has no ACP + * reply). Do NOT "simplify" park back to `deny`: for Claude, replying `reject` produces a + * failed tool call ("User refused permission") whose `tool_result {isError}` overwrites the + * approval prompt on the same tool-call id (the F-024 clobber bug). + */ export type PermissionDecision = "allow" | "deny"; +/** The full set of responder outcomes, including the runner-internal `park`. */ +export type ResponderOutcome = PermissionDecision | "park"; + /** A permission gate raised by the harness, normalized from the ACP request. */ export interface PermissionRequest { /** The ACP permission id; reused as the `interaction_request` event id for reply matching. */ @@ -41,14 +60,14 @@ export interface PermissionRequest { * alongside the cross-turn responder. */ export interface Responder { - onPermission(request: PermissionRequest): Promise; + onPermission(request: PermissionRequest): Promise; } -/** Headless responder: a fixed policy, no human in the loop. */ +/** Headless responder: a fixed policy, no human in the loop. Never parks (no human surface). */ export class PolicyResponder implements Responder { constructor(private readonly policy: PermissionPolicy) {} - async onPermission(_request: PermissionRequest): Promise { + async onPermission(_request: PermissionRequest): Promise { return this.policy === "deny" ? "deny" : "allow"; } } @@ -68,12 +87,14 @@ export type ApprovalDecisions = ReadonlyMap; * It answers a permission gate three ways, in order: * 1. The user already decided (a stored `decisions` entry for this tool-call id or tool * name) -> apply it. THIS IS THE RESUME PATH: turn N parks, turn N+1 carries the reply. - * 2. No stored decision and there is a human surface (`hasHumanSurface`) -> `deny` to PARK. - * The `interaction_request` was already emitted upstream (the FE prompts), so denying - * here just declines to run the unapproved tool this turn; the turn ends safely and a - * later turn carrying the decision resolves it via branch 1. + * 2. No stored decision and there is a human surface (`hasHumanSurface`) -> `park`. The + * `interaction_request` was already emitted upstream (the FE prompts), so the turn ends + * with this tool PENDING and NO harness reply (the adapter skips `respondPermission`). + * A later turn carrying the decision resolves it via branch 1. Parking by `deny` instead + * would make Claude emit a failed tool call that clobbers the approval prompt (F-024). * 3. No stored decision and no human surface (headless `/invoke`) -> the `basePolicy` - * decision. This branch is byte-identical to `PolicyResponder`, so `/invoke` is unchanged. + * decision. This branch is byte-identical to `PolicyResponder`, so `/invoke` is unchanged + * (it never parks; there is no human to resolve a parked turn). * * Pure: every input (decisions, base policy, surface flag) is injected; no I/O. */ @@ -84,10 +105,10 @@ export class HITLResponder implements Responder { private readonly hasHumanSurface: boolean, ) {} - async onPermission(request: PermissionRequest): Promise { + async onPermission(request: PermissionRequest): Promise { const stored = this.lookup(request); if (stored) return stored; - if (this.hasHumanSurface) return "deny"; // park: do not run the unapproved tool this turn + if (this.hasHumanSurface) return "park"; // human must decide; end the turn, tool pending return this.basePolicy === "deny" ? "deny" : "allow"; // headless: PolicyResponder parity } diff --git a/services/agent/src/tools/code.ts b/services/agent/src/tools/code.ts index 14544b0cfd..b3c27f09dc 100644 --- a/services/agent/src/tools/code.ts +++ b/services/agent/src/tools/code.ts @@ -1,10 +1,15 @@ /** * Code-tool sidecar execution gate. * - * The code-tool interface still exists and code tools are still advertised to harnesses. The - * sidecar no longer executes author-supplied snippets locally, though: every delivery path - * funnels a `kind: "code"` call through this function, so throwing here makes direct Pi, - * sandbox Pi, and the ACP/MCP bridge fail consistently without changing the public wire shape. + * The code-tool interface still exists on the wire, but the sidecar no longer executes + * author-supplied snippets locally (F-010 security removal). A run that carries a `code` tool + * is refused UP FRONT in `buildRunPlan` (`run-plan.ts` `hasCodeTool` -> + * `CODE_TOOL_UNSUPPORTED_MESSAGE`) so the failure surfaces as a non-success run result rather + * than being laundered into an `ok:true` reply (F-016: a per-call throw becomes a tool RESULT + * the model echoes back as "success"). This per-call throw remains as a defense-in-depth + * backstop: every delivery path (direct Pi, sandbox Pi, the ACP/MCP bridge, the relay) funnels + * a `kind: "code"` call through here, so even if a code tool reaches execution it fails + * consistently, without changing the public wire shape. */ export type CodeRuntime = "python" | "node"; diff --git a/services/agent/tests/unit/responder.test.ts b/services/agent/tests/unit/responder.test.ts index ab38dae8a1..4a99283ac3 100644 --- a/services/agent/tests/unit/responder.test.ts +++ b/services/agent/tests/unit/responder.test.ts @@ -108,10 +108,16 @@ describe("HITLResponder", () => { ); }); - it("parks (deny) when there is a human surface and no stored decision", async () => { + it("parks when there is a human surface and no stored decision (NOT deny)", async () => { // `basePolicy` is "auto" so this proves the park overrides the policy, not the policy. + // Park must NOT be `deny`: replying `reject` to Claude clobbers the approval prompt (F-024). const responder = new HITLResponder(new Map(), "auto", true); - assert.equal(await responder.onPermission(permReq("tc-x", "edit")), "deny"); + assert.equal(await responder.onPermission(permReq("tc-x", "edit")), "park"); + + // A deny basePolicy must still PARK (the human surface wins): a human can decide, so the + // turn ends pending rather than refusing the tool with a clobbering reject. + const denyBase = new HITLResponder(new Map(), "deny", true); + assert.equal(await denyBase.onPermission(permReq("tc-w", "edit")), "park"); }); it("headless: no decision + no human surface falls back to basePolicy (PolicyResponder parity)", async () => { diff --git a/services/agent/tests/unit/sandbox-agent-acp-fetch.test.ts b/services/agent/tests/unit/sandbox-agent-acp-fetch.test.ts new file mode 100644 index 0000000000..2cf3e972c8 --- /dev/null +++ b/services/agent/tests/unit/sandbox-agent-acp-fetch.test.ts @@ -0,0 +1,72 @@ +/** + * Unit tests for the ACP HTTP fetch dispatcher. + * + * HITL parks the ACP connection open while a human approves a tool; the default undici + * `headersTimeout` would reap it (UND_ERR_HEADERS_TIMEOUT) and kill the parked + resume turns. + * These tests pin that the ACP dispatcher disables those timeouts by default and honors the + * env overrides. + * + * Run: pnpm test (or: pnpm exec vitest run tests/unit/sandbox-agent-acp-fetch.test.ts) + */ +import { afterEach, describe, it } from "vitest"; +import assert from "node:assert/strict"; + +import { + createAcpDispatcher, + createAcpFetch, +} from "../../src/engines/sandbox_agent/acp-fetch.ts"; + +const envKeys = [ + "SANDBOX_AGENT_ACP_HEADERS_TIMEOUT_MS", + "SANDBOX_AGENT_ACP_BODY_TIMEOUT_MS", +]; +const previousEnv = new Map(); +for (const key of envKeys) previousEnv.set(key, process.env[key]); + +afterEach(() => { + for (const key of envKeys) { + const value = previousEnv.get(key); + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } +}); + +/** Read the undici Agent's resolved options off its private `Symbol(options)`. */ +function agentOptions(dispatcher: object): Record { + const sym = Object.getOwnPropertySymbols(dispatcher).find( + (s) => String(s) === "Symbol(options)", + ); + assert.ok(sym, "undici Agent should expose Symbol(options)"); + return (dispatcher as Record>)[sym]; +} + +describe("createAcpDispatcher", () => { + it("disables headers/body timeouts by default so a parked HITL turn is not reaped", () => { + delete process.env.SANDBOX_AGENT_ACP_HEADERS_TIMEOUT_MS; + delete process.env.SANDBOX_AGENT_ACP_BODY_TIMEOUT_MS; + const opts = agentOptions(createAcpDispatcher()); + assert.equal(opts.headersTimeout, 0); + assert.equal(opts.bodyTimeout, 0); + }); + + it("honors a positive env override for the headers and body timeout", () => { + process.env.SANDBOX_AGENT_ACP_HEADERS_TIMEOUT_MS = "900000"; + process.env.SANDBOX_AGENT_ACP_BODY_TIMEOUT_MS = "120000"; + const opts = agentOptions(createAcpDispatcher()); + assert.equal(opts.headersTimeout, 900000); + assert.equal(opts.bodyTimeout, 120000); + }); + + it("falls back to disabled (0) for a non-numeric override", () => { + process.env.SANDBOX_AGENT_ACP_HEADERS_TIMEOUT_MS = "not-a-number"; + const opts = agentOptions(createAcpDispatcher()); + assert.equal(opts.headersTimeout, 0); + }); +}); + +describe("createAcpFetch", () => { + it("returns a fetch bound to the long-timeout ACP dispatcher", () => { + const acpFetch = createAcpFetch(); + assert.equal(typeof acpFetch, "function"); + }); +}); diff --git a/services/agent/tests/unit/sandbox-agent-daytona.test.ts b/services/agent/tests/unit/sandbox-agent-daytona.test.ts index 6c15cf4c1c..3f5d791da6 100644 --- a/services/agent/tests/unit/sandbox-agent-daytona.test.ts +++ b/services/agent/tests/unit/sandbox-agent-daytona.test.ts @@ -100,11 +100,11 @@ describe("uploadPiAuthToSandbox", () => { describe("createCookieFetch", () => { it("persists Daytona preview cookies per host", async () => { const seenCookies: Array = []; - globalThis.fetch = (async (_input: any, init?: any) => { + const innerFetch = (async (_input: any, init?: any) => { seenCookies.push(new Headers(init?.headers).get("cookie")); return new Response("ok", { headers: { "set-cookie": "session=abc; Path=/" } }); }) as typeof fetch; - const cookieFetch = createCookieFetch(); + const cookieFetch = createCookieFetch(innerFetch); await cookieFetch("https://sandbox.example.test/first"); await cookieFetch("https://sandbox.example.test/second", { diff --git a/services/agent/tests/unit/sandbox-agent-orchestration.test.ts b/services/agent/tests/unit/sandbox-agent-orchestration.test.ts index 018c574f19..4fe7fb9c4b 100644 --- a/services/agent/tests/unit/sandbox-agent-orchestration.test.ts +++ b/services/agent/tests/unit/sandbox-agent-orchestration.test.ts @@ -636,7 +636,7 @@ describe("runSandboxAgent default HITL responder wiring", () => { ]); }); - it("human surface (/messages: sessionId set) with no decision parks the tool (reject)", async () => { + it("human surface (/messages: sessionId set) with no decision PARKS the tool, no harness reply (F-024)", async () => { const { calls, deps } = depsWithDefaultResponder(); const result = await runSandboxAgent( @@ -652,11 +652,20 @@ describe("runSandboxAgent default HITL responder wiring", () => { await flushPromises(); assert.equal(result.ok, true); - // Park: decline the unapproved tool this turn (the interaction_request already prompted - // the browser); the next turn carrying the decision resolves it. - assert.deepEqual(calls.permissionReplies, [ - { id: "perm-1", reply: "reject" }, - ]); + // Park: the interaction_request IS emitted (the FE prompts the browser) ... + assert.deepEqual( + result.events + ?.filter((e) => e.type === "interaction_request") + .map((e) => ({ + type: e.type, + id: (e as any).id, + })), + [{ type: "interaction_request", id: "perm-1" }], + ); + // ... but the harness gets NO reply: a `reject` here would make Claude emit a failed tool + // call that clobbers the approval prompt on the same tool-call id (F-024). The turn ends + // with the tool pending; the next turn carrying the decision resolves it. + assert.deepEqual(calls.permissionReplies, []); }); it("human surface with a stored approval resumes the tool (always)", async () => { diff --git a/services/agent/tests/unit/sandbox-agent-permissions.test.ts b/services/agent/tests/unit/sandbox-agent-permissions.test.ts index 600b9b2881..14ebdc8e6a 100644 --- a/services/agent/tests/unit/sandbox-agent-permissions.test.ts +++ b/services/agent/tests/unit/sandbox-agent-permissions.test.ts @@ -76,6 +76,47 @@ describe("attachPermissionResponder", () => { assert.deepEqual(replies, [{ id: "perm-1", reply: "always" }]); }); + it("parks: emits the interaction_request but sends NO harness reply (F-024 regression)", async () => { + // The park outcome must never reach the harness as a reply: a `reject` would make Claude + // emit a failed tool call ("User refused permission") whose tool_result{isError} clobbers + // the approval prompt on the same tool-call id. So on park the approval-request event is + // emitted and respondPermission is NOT called — the turn ends with the tool pending. + let handler: ((req: any) => void) | undefined; + const replies: Array<{ id: string; reply: string }> = []; + const session = { + onPermissionRequest(cb: (req: any) => void) { + handler = cb; + }, + async respondPermission(id: string, reply: string) { + replies.push({ id, reply }); + }, + }; + const events: AgentEvent[] = []; + + attachPermissionResponder({ + session, + run: { emitEvent: (event) => events.push(event) }, + responder: { + async onPermission() { + return "park"; + }, + }, + }); + handler?.({ + id: "perm-park", + availableReplies: ["once", "always", "reject"], + toolCall: { toolCallId: "tool-9", name: "edit" }, + }); + await flushPromises(); + + // The approval-request event IS emitted (the FE needs it to prompt) ... + assert.equal(events.length, 1); + assert.equal(events[0].type, "interaction_request"); + assert.equal((events[0] as any).id, "perm-park"); + // ... but the harness gets NO reply (no reject to clobber the prompt with). + assert.deepEqual(replies, []); + }); + it("does not respond when the ACP request has no id", async () => { let handler: ((req: any) => void) | undefined; const replies: Array<{ id: string; reply: string }> = []; @@ -91,7 +132,11 @@ describe("attachPermissionResponder", () => { attachPermissionResponder({ session, run: { emitEvent: () => {} }, - responder: { async onPermission() { return "deny"; } }, + responder: { + async onPermission() { + return "deny"; + }, + }, }); handler?.({ availableReplies: ["reject"] }); await flushPromises(); diff --git a/services/agent/tests/unit/sandbox-agent-run-plan.test.ts b/services/agent/tests/unit/sandbox-agent-run-plan.test.ts index d9c7d28fc8..6ddabeaf00 100644 --- a/services/agent/tests/unit/sandbox-agent-run-plan.test.ts +++ b/services/agent/tests/unit/sandbox-agent-run-plan.test.ts @@ -268,6 +268,54 @@ describe("buildRunPlan", () => { assert.match(result.error, /MCP servers are not supported by the sidecar/); }); + it("errors on any run carrying a code tool (code execution removed, fail loud)", () => { + // Code tools were removed for security (F-010). The run is refused up-front so the failure + // surfaces as a non-success result (ok:false) rather than being laundered into a 200 reply + // (F-016: a per-call throw becomes a tool result the model echoes back as "success"). + let created = false; + const result = buildRunPlan( + { + harness: "pi_core", + sandbox: "local", + prompt: "compute it", + customTools: [ + { + name: "secret_math", + kind: "code", + runtime: "python", + code: "def main(x=0):\n return x * 7 + 1\n", + }, + ], + } as AgentRunRequest, + { + createLocalCwd: () => { + created = true; + return "/tmp/local-cwd"; + }, + }, + ); + + assert.equal(result.ok, false); + if (result.ok) return; + assert.match(result.error, /Code tools are not supported by the sidecar\./); + // Fails before any cwd is created (parity with the other up-front gates). + assert.equal(created, false); + }); + + it("allows a run with a non-code (callback) tool", () => { + const result = buildRunPlan( + { + harness: "pi_core", + sandbox: "local", + prompt: "do it", + customTools: [{ name: "server_tool", kind: "callback" }], + } as AgentRunRequest, + { createLocalCwd: () => "/tmp/local-cwd" }, + ); + + assert.equal(result.ok, true); + }); + it("allows a strict restricted-network Daytona run with only a remote MCP server", () => { const result = buildRunPlan( { diff --git a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentConfigControl.tsx b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentConfigControl.tsx index baae5d33f6..e53e86594a 100644 --- a/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentConfigControl.tsx +++ b/web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentConfigControl.tsx @@ -128,6 +128,9 @@ export function AgentConfigControl({ // (the picker only ever produces one); a legacy bare string is read for display. The picker is // harness-filtered: selecting a model sets BOTH the model id and its provider. const harness = typeof config.harness === "string" ? config.harness : null + // Pi (`pi_core`/`pi_agenta`) never gates tool use (`permissions: false`); a permission + // policy is meaningless for it, so the field is hidden for Pi. Only Claude honors it. + const isPiHarness = harness === "pi_core" || harness === "pi_agenta" const modelId = useMemo(() => modelIdFromConfig(config.model), [config.model]) const connection = useMemo(() => connectionFromConfig(config.model), [config.model]) const modeOptions = useMemo( @@ -678,14 +681,18 @@ export function AgentConfigControl({ disabled={disabled} /> - setField("permission_policy", v)} - withTooltip={withTooltip} - disabled={disabled} - /> + {/* Permission policy is Claude-only: Pi runs tools without prompting (no gate), + * so the field is hidden for Pi rather than offering a setting nothing honors. */} + {!isPiHarness && ( + setField("permission_policy", v)} + withTooltip={withTooltip} + disabled={disabled} + /> + )} {/* Sandbox permissions (Layer 2): the sandbox security boundary, for every harness. */}