fix(agent): park HITL approvals + keep ACP alive + fail loud on code tools

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(

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",

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

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;
+}

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<string, Map<string, string>>(); // 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()

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(() => {});
   });

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

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<PermissionDecision>;
+  onPermission(request: PermissionRequest): Promise<ResponderOutcome>;
 }
 
-/** 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<PermissionDecision> {
+  async onPermission(_request: PermissionRequest): Promise<ResponderOutcome> {
     return this.policy === "deny" ? "deny" : "allow";
   }
 }
@@ -68,12 +87,14 @@ export type ApprovalDecisions = ReadonlyMap<string, PermissionDecision>;
  * 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<PermissionDecision> {
+  async onPermission(request: PermissionRequest): Promise<ResponderOutcome> {
     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
   }
 

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";

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 () => {