From 3a1bda0e4d050e939ce1c2d5ecc91dea0f74aa13 Mon Sep 17 00:00:00 2001 From: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Date: Wed, 11 Nov 2026 20:23:58 +0000 Subject: [PATCH] fix(acp): only approved execute requests let the agent open client terminals --- .../Adapters/AcpAdapterV2.test.ts | 33 ++++++++++++++++--- packages/provider-acp/src/server/adapter.ts | 7 ++-- .../src/server/clientPolicy.test.ts | 15 +++++---- .../provider-acp/src/server/clientPolicy.ts | 10 +++--- 4 files changed, 45 insertions(+), 20 deletions(-) diff --git a/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts b/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts index d8380ae8df441..6818729f45411 100644 --- a/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts +++ b/apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts @@ -4532,7 +4532,25 @@ describe("AcpAdapterV2", () => { kind: "other", granted: false, }, - ] as const)("$label", ({ kind, granted }) => + { + label: "grants an explicit execute approval", + kind: "execute", + requestKind: "execute", + granted: true, + }, + { + label: "does not grant execution to an approved mode switch", + kind: "execute", + requestKind: "switch_mode", + granted: false, + }, + { + label: "does not grant execution to an approved explicit other tool", + kind: "other", + requestKind: "other", + granted: false, + }, + ] as const)("$label", ({ kind, granted, ...testCase }) => Effect.gen(function* () { const childProcessSpawner = yield* ChildProcessSpawner.ChildProcessSpawner; const path = yield* Path.Path; @@ -4619,7 +4637,10 @@ describe("AcpAdapterV2", () => { const permissionFiber = yield* requestPermission( { sessionId: "mock-session-1", - toolCall: { toolCallId: "tracked-tool-call" }, + toolCall: { + toolCallId: "tracked-tool-call", + ...("requestKind" in testCase ? { kind: testCase.requestKind } : {}), + }, options: [{ optionId: "allow-once", name: "Allow once", kind: "allow_once" }], }, { requestId: "id-only-permission-request", method: "session/request_permission" }, @@ -4661,8 +4682,12 @@ describe("AcpAdapterV2", () => { { requestId: "id-only-permission-terminal", method: "terminal/create" }, ).pipe(Effect.exit); assert.deepEqual( - { prompt: approval.prompt, terminalCreated: Exit.isSuccess(terminalExit) }, - { prompt: "git status", terminalCreated: granted }, + { + prompt: approval.prompt, + requestKind: approval.requestKind, + terminalCreated: Exit.isSuccess(terminalExit), + }, + { prompt: "git status", requestKind: "command", terminalCreated: granted }, ); yield* runtime.interruptTurn({ providerThread, providerTurnId }); diff --git a/packages/provider-acp/src/server/adapter.ts b/packages/provider-acp/src/server/adapter.ts index cf213927a8394..163652914ac0b 100644 --- a/packages/provider-acp/src/server/adapter.ts +++ b/packages/provider-acp/src/server/adapter.ts @@ -6648,12 +6648,9 @@ export const makeAcpAdapterV2 = Effect.fn("makeAcpAdapterV2")(function* ( !offeredOptions.some((option) => option.decision === "acceptForSession") ? "accept" : answered; - if ( - parsedPermission.kind !== "unknown" && - (decision === "accept" || decision === "acceptForSession") - ) { + if (decision === "accept" || decision === "acceptForSession") { clientPolicyGrants.recordApproval({ - kind: providerRequestKind(parsedPermission.kind), + toolKind: parsedPermission.kind, scope: decision === "acceptForSession" ? "session" : "turn", turnKey: String(context.providerTurnId), }); diff --git a/packages/provider-acp/src/server/clientPolicy.test.ts b/packages/provider-acp/src/server/clientPolicy.test.ts index 630ebc979e753..d093b3c85200d 100644 --- a/packages/provider-acp/src/server/clientPolicy.test.ts +++ b/packages/provider-acp/src/server/clientPolicy.test.ts @@ -319,12 +319,13 @@ describe("client terminal disposition", () => { }); describe("makeAcpClientPolicyGrants", () => { - it("grants terminals only from commands, never from an approved read or edit", () => { + it("grants terminals only from execute approvals, never from any other approved kind", () => { const grants = makeAcpClientPolicyGrants(); - grants.recordApproval({ kind: "file-read", scope: "turn", turnKey: "turn-1" }); - grants.recordApproval({ kind: "file-change", scope: "session", turnKey: "turn-1" }); + for (const toolKind of ["read", "edit", "switch_mode", "other", "unknown", "browse"]) { + grants.recordApproval({ toolKind, scope: "session", turnKey: "turn-1" }); + } assert.isFalse(grants.allowsExecute("turn-1")); - grants.recordApproval({ kind: "command", scope: "turn", turnKey: "turn-1" }); + grants.recordApproval({ toolKind: "execute", scope: "turn", turnKey: "turn-1" }); assert.isTrue(grants.allowsExecute("turn-1")); assert.isFalse(grants.allowsExecute("turn-2")); assert.isFalse(grants.allowsExecute(null)); @@ -332,15 +333,15 @@ describe("makeAcpClientPolicyGrants", () => { it("keeps accept-for-session command grants across turns", () => { const grants = makeAcpClientPolicyGrants(); - grants.recordApproval({ kind: "command", scope: "session", turnKey: "turn-1" }); + grants.recordApproval({ toolKind: "execute", scope: "session", turnKey: "turn-1" }); assert.isTrue(grants.allowsExecute("turn-9")); assert.isTrue(grants.allowsExecute(null)); }); it("drops a turn's command grant when a later turn approves one", () => { const grants = makeAcpClientPolicyGrants(); - grants.recordApproval({ kind: "command", scope: "turn", turnKey: "turn-1" }); - grants.recordApproval({ kind: "command", scope: "turn", turnKey: "turn-2" }); + grants.recordApproval({ toolKind: "execute", scope: "turn", turnKey: "turn-1" }); + grants.recordApproval({ toolKind: "execute", scope: "turn", turnKey: "turn-2" }); assert.isFalse(grants.allowsExecute("turn-1")); assert.isTrue(grants.allowsExecute("turn-2")); }); diff --git a/packages/provider-acp/src/server/clientPolicy.ts b/packages/provider-acp/src/server/clientPolicy.ts index 346936f193966..2add9197de2eb 100644 --- a/packages/provider-acp/src/server/clientPolicy.ts +++ b/packages/provider-acp/src/server/clientPolicy.ts @@ -1,5 +1,5 @@ // @effect-diagnostics nodeBuiltinImport:off -import type { ProviderRequestKind, RuntimeMode } from "@t3tools/contracts"; +import type { RuntimeMode } from "@t3tools/contracts"; import type * as EffectAcpSchema from "effect-acp/compat"; import * as NodeFS from "node:fs"; import * as NodePath from "node:path"; @@ -222,7 +222,8 @@ export function acpClientExecuteDisposition( } export interface AcpApprovalGrantInput { - readonly kind: ProviderRequestKind; + /** The approved request's ACP tool kind, after ID-only requests take their tracked call's kind. */ + readonly toolKind: string; readonly scope: "session" | "turn"; readonly turnKey: string; } @@ -232,13 +233,14 @@ export interface AcpClientPolicyGrants { readonly allowsExecute: (turnKey: string | null) => boolean; } -/** ACP cannot link commands to terminals; command approvals grant execution for the turn or accepted session. */ +/** ACP cannot link commands to terminals; approved execute requests grant execution for the turn or accepted session. */ export function makeAcpClientPolicyGrants(): AcpClientPolicyGrants { let sessionExecute = false; let turnExecute: string | null = null; return { recordApproval: (input) => { - if (input.kind !== "command") return; + // Other kinds also show as command cards, but approving them must not open terminals. + if (input.toolKind !== "execute") return; if (input.scope === "session") { sessionExecute = true; } else {