From 4ac9d9c94fd29335d26eb695399f611e11f3d396 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Thu, 9 Jul 2026 21:29:34 +0200 Subject: [PATCH 1/3] feat(runner): park Pi approval gates over the ACP permission plane Both Pi approval gates (builtin + custom-tool) stop expressing an approval as a file-relay poll and raise it as ctx.ui.confirm carrying a JSON envelope with the real gate identity. The pi-acp bridge turns that into a real ACP session/request_permission, which the runner classifies, decides, and (under keep-alive) parks and resumes on the live session, exactly like Claude's gate. Within the approval TTL a human's answer resumes the original tool call with its original arguments; outside it, everything degrades to today's cold path. Behind AGENTA_RUNNER_PI_DIALOG_GATE (default off); flag-off is byte-identical. - pi-gate-envelope.ts: the shared, strict, version-checked envelope contract. - acp-interactions.ts: envelope detection + real-id normalization + runner-side metadata recovery + fail-closed malformed reject + gateType plumbing. - responder.ts: FIFO appendDecision + consume-1-append-1 for the custom-tool double gate. - sandbox_agent.ts / server.ts: ParkedApproval gateType union + resume guard. - agenta.ts / pi-assets.ts: the extension switch and the flag env. Slice 0 (live daemon confidence run) passed both criteria on the dev box. Claude-Session: https://claude.ai/code/session_01CSTSEXSe4DDhoXCFjZpZ5W --- services/runner/src/engines/sandbox_agent.ts | 65 +++- .../engines/sandbox_agent/acp-interactions.ts | 192 +++++++++- .../src/engines/sandbox_agent/pi-assets.ts | 10 +- .../engines/sandbox_agent/pi-gate-envelope.ts | 139 +++++++ services/runner/src/extensions/agenta.ts | 103 ++++- services/runner/src/responder.ts | 51 +++ services/runner/src/server.ts | 20 +- .../runner/tests/unit/extension-tools.test.ts | 203 +++++++++- .../tests/unit/pi-gate-envelope.test.ts | 277 ++++++++++++++ services/runner/tests/unit/responder.test.ts | 352 +++++++++++++++-- .../sandbox-agent-acp-interactions.test.ts | 362 +++++++++++++++++- .../unit/sandbox-agent-pi-assets.test.ts | 38 +- .../unit/session-keepalive-approval.test.ts | 135 ++++++- 13 files changed, 1860 insertions(+), 87 deletions(-) create mode 100644 services/runner/src/engines/sandbox_agent/pi-gate-envelope.ts create mode 100644 services/runner/tests/unit/pi-gate-envelope.test.ts diff --git a/services/runner/src/engines/sandbox_agent.ts b/services/runner/src/engines/sandbox_agent.ts index 1d54ad08ef..32332b8415 100644 --- a/services/runner/src/engines/sandbox_agent.ts +++ b/services/runner/src/engines/sandbox_agent.ts @@ -94,7 +94,10 @@ import { PendingApprovalLatch, permissionsFromRequest, } from "../permission-plan.ts"; -import { attachPermissionResponder } from "./sandbox_agent/acp-interactions.ts"; +import { + attachPermissionResponder, + type ParkedApprovalGateType, +} from "./sandbox_agent/acp-interactions.ts"; import { PAUSED, PendingApprovalPauseController, @@ -136,6 +139,14 @@ function log(message: string): void { process.stderr.write(`[sandbox-agent] ${message}\n`); } +/** Pi approval parking flag (runner side, default OFF). Only a few explicit truthy spellings. */ +function piDialogGateEnabled(): boolean { + const raw = (process.env.AGENTA_RUNNER_PI_DIALOG_GATE ?? "") + .trim() + .toLowerCase(); + return raw === "1" || raw === "true" || raw === "yes" || raw === "on"; +} + /** Extract the run credential from the OTLP export headers (initial value, constant for the run). */ function runCredential(request: AgentRunRequest): string { const headers = (request.telemetry?.exporters?.otlp?.headers ?? {}) as Record< @@ -359,15 +370,16 @@ interface CurrentTurn { } /** - * A Claude ACP permission gate that paused the turn and can be answered later on the SAME live - * session (slice 2 keep-alive). Recorded ONLY for a harness ACP permission gate (never a Pi relay - * gate, a Pi builtin gate, or a client-tool MCP pause — those cannot be answered across a turn - * boundary and stay on the cold path). Existence of this record is what makes the dispatch park a - * paused session in `awaiting_approval` instead of tearing it down. + * A permission gate that paused the turn and can be answered later on the SAME live session. + * Recorded for a Claude ACP permission gate (keep-alive slice 2) or a Pi dialog permission gate + * (Pi approval parking, which rides `ctx.ui.confirm` onto the same ACP permission plane). NOT + * recorded for a Pi file-relay gate or a client-tool MCP pause — those cannot be answered across + * a turn boundary and stay on the cold path. Existence of this record is what makes the dispatch + * park a paused session in `awaiting_approval` instead of tearing it down. */ export interface ParkedApproval { - /** Marks the pending-gate shape; the dispatch treats any other shape as a cold-fallback. */ - gateType: "claude-acp-permission"; + /** Which gate paused; the dispatch resumes only a recognized type and treats others as cold. */ + gateType: ParkedApprovalGateType; /** The ACP permission-request id, answered later via `session.respondPermission`. */ permissionId: string; /** The gated tool call's id — matched against the incoming approval envelope's toolCallId. */ @@ -591,6 +603,9 @@ export async function acquireEnvironment( otlpAuthFilePath, builtinGatingActive: plan.builtinGatingActive, builtinGrants: plan.builtinGrants, + // Pi approval parking: route both Pi gates over the parkable dialog plane. Runner-side + // flag, default off; flag-off keeps the byte-identical relay path. + dialogGateActive: piDialogGateEnabled(), // The materialized skill names (author + forced `_agenta.*`) so Pi's own agent span // records which skills loaded (F-029); local Pi self-instruments, so the runner's // sandbox-agent otel has no span to stamp here. @@ -1226,7 +1241,12 @@ export async function runTurn( const latch = new PendingApprovalLatch(); const responder = deps.responderFactory?.(request) ?? - new ApprovalResponder(permissionPlan, decisions, logger); + new ApprovalResponder(permissionPlan, decisions, logger, { + // The Pi double-gate bridge: only where the dialog gate is live AND the relay enforces + // (Pi). On any other run the relay never consumes a re-appended decision, so appending + // would leak it to a later identical call. + bridgeRelayDoubleGate: plan.isPi && piDialogGateEnabled(), + }); // Every pause seeds the durable interactions plane, whichever gate paused. const recordPendingInteraction = ( token: string, @@ -1262,7 +1282,9 @@ export async function runTurn( return; void resolveInteraction(sessionId, token, () => cred); }; - // Exactly one gate per call: the harness gate on Claude, the relay on Pi. + // The harness gate decides on Claude; the relay decides on Pi. Under the Pi dialog gate a + // custom tool is checked twice (dialog + relay execution check); the responder's + // bridgeRelayDoubleGate accounting keeps the two consuming one human decision. const relayPermissions: RelayPermissions = { enforce: plan.isPi, decide: (gate) => decide(gate, permissionPlan, decisions), @@ -1312,9 +1334,24 @@ export async function runTurn( onPausedToolCall: (id) => pause.markPausedToolCall(id), onCreateInteraction: recordPendingInteraction, onResolveInteraction: resolveInteractionToken, - // Slice 2: record the parkable Claude ACP permission gate (only in keep-alive park mode) so - // the dispatch can resume it live. Fires per pending gate (before the latch) so a parallel - // gate is counted; the single-gate resume records only the FIRST gate's answer target. + // Envelope detection is scoped to a Pi run with the dialog gate live. Flag off (or any + // Claude run) never parses: a Claude gate whose title collides with the dialog title must + // take today's path, not the fail-closed reject. + dialogGateEnabled: plan.isPi && piDialogGateEnabled(), + // Recover permission metadata for a Pi dialog gate: the envelope names the tool, the runner + // fills specPermission/readOnlyHint from the run's own resolved specs (relay parity). Pi only. + piToolSpecsByName: plan.isPi + ? new Map( + plan.toolSpecs.map((spec) => [ + spec.name, + { permission: spec.permission, readOnly: spec.readOnly }, + ]), + ) + : undefined, + // Record the parkable permission gate (only in keep-alive park mode) so the dispatch can + // resume it live. Fires per pending gate (before the latch) so a parallel gate is counted; + // the single-gate resume records only the FIRST gate's answer target. `info.gateType` names + // the plane (Claude ACP vs Pi dialog) so the resume answers on the right one. onUserApprovalGate: opts.approvalParkMode ? (info) => { env.approvalGateCount += 1; @@ -1324,7 +1361,7 @@ export async function runTurn( info.toolCallId ) { env.parkedApproval = { - gateType: "claude-acp-permission", + gateType: info.gateType, permissionId: info.permissionId, toolCallId: info.toolCallId, toolName: info.toolName, diff --git a/services/runner/src/engines/sandbox_agent/acp-interactions.ts b/services/runner/src/engines/sandbox_agent/acp-interactions.ts index 4409da5caf..25cfb2d44c 100644 --- a/services/runner/src/engines/sandbox_agent/acp-interactions.ts +++ b/services/runner/src/engines/sandbox_agent/acp-interactions.ts @@ -6,9 +6,25 @@ import { type Responder, } from "../../responder.ts"; import { + piBuiltinIdentity, PendingApprovalLatch, type GateDescriptor, } from "../../permission-plan.ts"; +import { + parsePiGateEnvelope, + type PiGateEnvelope, +} from "./pi-gate-envelope.ts"; + +/** The parkable gate types a paused turn can record (widened for the Pi dialog gate). */ +export type ParkedApprovalGateType = + "claude-acp-permission" | "pi-dialog-permission"; + +/** The permission metadata the runner recovers per tool for a Pi dialog gate (identity-only + * envelope carries no policy). Keyed by resolved tool name. */ +export interface PiToolSpecMeta { + permission?: ToolPermission; + readOnly?: boolean; +} export interface AttachPermissionResponderInput { session: any; @@ -34,11 +50,11 @@ export interface AttachPermissionResponderInput { /** Called after a stored decision was successfully forwarded to the harness. */ onResolveInteraction?: (token: string) => void; /** - * Fires for EVERY Claude ACP permission gate (harness executor) that resolves to - * pendingApproval, BEFORE the single-pause latch. Slice 2 keep-alive uses it to record the - * parked permission id / tool-call id (for a live resume via `respondPermission`) and to count - * how many gates are pending this turn (a multi-gate pause does not park). It never fires for a - * client-tool gate (`pauseClientTool`), so only Claude ACP permission gates can park. + * Fires for EVERY parkable permission gate (a Claude ACP gate or a Pi dialog gate) that + * resolves to pendingApproval, BEFORE the single-pause latch. Keep-alive uses it to record + * the parked permission id / tool-call id (for a live resume via `respondPermission`) and to + * count how many gates are pending this turn (a multi-gate pause does not park). It never + * fires for a client-tool gate (`pauseClientTool`), which stays on the cold path. */ onUserApprovalGate?: (info: { permissionId: string; @@ -46,7 +62,22 @@ export interface AttachPermissionResponderInput { toolName: string | undefined; args: unknown; interactionToken: string; + /** Which gate paused, so the park record can resume it on the right plane. */ + gateType: ParkedApprovalGateType; }) => void; + /** + * Detect Pi gate envelopes on incoming permission requests. ON only for a Pi run with the + * dialog gate active. It must stay OFF everywhere else: the pre-filter is the dialog TITLE, + * and a Claude gate whose ACP title happens to be the literal dialog title (editing a file + * named after it, a bash command equal to it) has no envelope and would be auto-rejected + * where today's path pauses or resolves it normally. + */ + dialogGateEnabled?: boolean; + /** + * Resolved tool specs by name, for a Pi dialog gate. The envelope carries identity only, so + * the runner recovers `specPermission`/`readOnlyHint` here (relay parity). Absent for Claude. + */ + piToolSpecsByName?: ReadonlyMap; } /** Wire ACP permission reverse-RPC into the runner's event stream and responder policy. */ @@ -62,6 +93,8 @@ export function attachPermissionResponder({ onCreateInteraction, onResolveInteraction, onUserApprovalGate, + dialogGateEnabled, + piToolSpecsByName, }: AttachPermissionResponderInput): void { session.onPermissionRequest((req: any) => { void handleRequest(req).catch((err) => { @@ -73,7 +106,9 @@ export function attachPermissionResponder({ // The emitted payload carries a COPY of the ACP toolCall stamped with `resolvedName` (the // gate's stable anchor). The Vercel egress prefers it over the drift-prone title/kind // display fields, so the approval part names the tool exactly as the responder keys it. - // The inbound ACP object itself is never mutated. + // This stamping never mutates the inbound ACP object. (The one deliberate inbound mutation + // is the Pi dialog gate's id/args normalization in `handlePiGate`, which must happen in + // place so every downstream read sees the envelope's real identity.) const stampResolvedName = (toolCall: any, gate: GateDescriptor): any => { if (!toolCall || typeof toolCall !== "object" || !gate.toolName) return toolCall; @@ -89,6 +124,7 @@ export function attachPermissionResponder({ req: any, id: string, gate: GateDescriptor, + gateType: ParkedApprovalGateType, ): void => { // Signal the parkable gate BEFORE the latch so a keep-alive resume can record the pending // permission id and the multi-gate detector counts every pending gate (not just the first). @@ -99,6 +135,7 @@ export function attachPermissionResponder({ toolName: gate.toolName, args: gate.args, interactionToken: interactionEventId(id, gateToolCallId), + gateType, }); if (!latch.tryAcquire()) return; const toolCallId = stringValue(req?.toolCall?.toolCallId); @@ -183,9 +220,112 @@ export function attachPermissionResponder({ } }; + // A bare reject that answers the harness WITHOUT touching the durable interactions plane (no + // row was created for a request the runner refuses before classifying it). Used for a + // malformed Pi gate envelope and an unknown builtin name: fail closed so an unapproved tool + // never runs. A request with no answerable id pauses instead (matching the base path), so the + // in-sandbox confirm dies with the teardown rather than hanging until the turn timeout. + const rejectRequest = async ( + id: string, + availableReplies: string[], + ): Promise => { + if (!id) { + onPause?.(); + return; + } + try { + await session.respondPermission( + id, + decisionToReply("deny", availableReplies) as any, + ); + } catch (err) { + log?.(`[HITL] reject failed id=${id}: ${errorMessage(err)}`); + onPause?.(); + } + }; + + /** + * A Pi gate that rode `ctx.ui.confirm`: classify from the envelope identity, not from the + * spec-less dialog strings. The tool-call id is normalized to the envelope's REAL id BEFORE + * anything reads `req.toolCall` (the descriptor, pause bookkeeping, the emitted card, and the + * park record all key on it); the emitted card's `rawInput` is set to the real args so it + * renders like a relay-gate card rather than showing the envelope JSON. + */ + const handlePiGate = async ( + req: any, + id: string, + availableReplies: string[], + envelope: PiGateEnvelope, + ): Promise => { + const toolCall = req?.toolCall; + if (toolCall && typeof toolCall === "object") { + toolCall.toolCallId = envelope.toolCallId; + toolCall.rawInput = envelope.input; + } + const gate = buildPiGateDescriptor(envelope, piToolSpecsByName); + // An unrecognized builtin name fails closed (relay parity: the relay denies unknown + // builtins outright). The envelope is sandbox-origin and untrusted; letting the raw name + // through would also put a fabricated tool name on the human's approval card. + if (!gate) { + log?.( + `[HITL] pi-gate unknown builtin ${JSON.stringify(envelope.toolName)} id=${id}; reject (fail closed)`, + ); + await rejectRequest(id, availableReplies); + return; + } + if (log) { + log( + `[HITL] pi-gate id=${id} ` + + JSON.stringify({ + gate: envelope.gate, + toolCallId: envelope.toolCallId, + toolName: gate.toolName, + executor: gate.executor, + specPermission: gate.specPermission, + readOnlyHint: gate.readOnlyHint, + }), + ); + } + const verdict = await responder.onPermission({ + id, + availableReplies, + gate, + raw: req, + }); + if (verdict.kind === "pendingApproval" || !id) { + pauseUserApproval(req, id, gate, "pi-dialog-permission"); + return; + } + await replyPermission(id, verdict.kind, availableReplies); + }; + async function handleRequest(req: any): Promise { const id = stringValue(req?.id) ?? ""; const availableReplies = stringArray(req?.availableReplies); + + // A Pi gate rides `ctx.ui.confirm` under the fixed dialog title. Detect it FIRST, before the + // spec-less classification below: without this the gate would key as `agenta-approval` with + // dialog-string args (wrong identity on cards, the decision map, and policy). Detection runs + // ONLY when the dialog gate is live for this run (`dialogGateEnabled`): the pre-filter is the + // TITLE, so with the flag off a Claude gate whose title collides with the dialog title must + // take today's path, not the fail-closed reject. With detection on, a matching title whose + // envelope does not parse fails closed (reject), never falls through — under a default-allow + // plan a fallthrough would confirm an unapproved execution. + if (dialogGateEnabled) { + const piGate = parsePiGateEnvelope(req); + if (piGate.matched) { + if (!piGate.envelope) { + log?.( + `[HITL] pi-gate malformed envelope id=${id}; reject (fail closed)`, + ); + await rejectRequest(id, availableReplies); + return; + } + await handlePiGate(req, id, availableReplies, piGate.envelope); + return; + } + } + const toolCall = req?.toolCall; const spec = resolvedSpecOf(toolCall); const gate = buildGateDescriptor(toolCall, run, serverPermissions); @@ -233,13 +373,51 @@ export function attachPermissionResponder({ raw: req, }); if (verdict.kind === "pendingApproval" || !id) { - pauseUserApproval(req, id, gate); + pauseUserApproval(req, id, gate, "claude-acp-permission"); return; } await replyPermission(id, verdict.kind, availableReplies); } } +/** + * Build the `GateDescriptor` for a Pi dialog gate from the envelope identity plus the runner's + * own resolved specs (the envelope carries identity, never policy). + * + * `pi-builtin` maps to `executor: "harness"` with the builtin's canonical rule name and + * read-only hint (matching `handlePermissionRelayRequest` in relay.ts); an UNKNOWN builtin + * name returns undefined so the caller rejects it (the relay denies unknown builtins outright, + * and the sandbox-origin envelope must not put a fabricated name on the approval card). + * `pi-custom-tool` maps to `executor: "relay"` with the spec's author permission and read-only + * hint recovered by name (matching the relay gate in relay.ts), so an author-allow tool stays + * instant-allow, an author-deny tool stays instant-deny, and a read-only builtin auto-allows — + * relay parity. + */ +export function buildPiGateDescriptor( + envelope: PiGateEnvelope, + piToolSpecsByName: ReadonlyMap | undefined, +): GateDescriptor | undefined { + if (envelope.gate === "pi-builtin") { + const identity = piBuiltinIdentity(envelope.toolName); + if (!identity) return undefined; + return { + executor: "harness", + toolName: identity.ruleName, + readOnlyHint: identity.readOnly, + args: envelope.input, + }; + } + const spec = piToolSpecsByName?.get(envelope.toolName); + return { + executor: "relay", + toolName: envelope.toolName, + specPermission: toolPermission(spec?.permission), + readOnlyHint: + typeof spec?.readOnly === "boolean" ? spec.readOnly : undefined, + args: envelope.input, + }; +} + /** * The name the runner already recorded for this tool-call id via the `session/update` * `tool_call` event. Used to key a harness gate so it matches the stored decision across a diff --git a/services/runner/src/engines/sandbox_agent/pi-assets.ts b/services/runner/src/engines/sandbox_agent/pi-assets.ts index 15b3b97f4b..bbecff39c8 100644 --- a/services/runner/src/engines/sandbox_agent/pi-assets.ts +++ b/services/runner/src/engines/sandbox_agent/pi-assets.ts @@ -46,6 +46,8 @@ export function buildPiExtensionEnv( skills?: string[]; builtinGatingActive?: boolean; builtinGrants?: string[]; + /** Route both Pi gates over the extension-UI dialog plane (Pi approval parking). */ + dialogGateActive?: boolean; } = {}, ): Record { const env: Record = {}; @@ -76,6 +78,10 @@ export function buildPiExtensionEnv( env.AGENTA_AGENT_BUILTIN_GRANTS = (opts.builtinGrants ?? []).join(","); if (opts.relayDir) env.AGENTA_AGENT_TOOLS_RELAY_DIR = opts.relayDir; } + // Pi approval parking: with the flag on both gates ride `ctx.ui.confirm` (a parkable ACP + // permission request) instead of the file relay. One flag drives both sides coherently + // because the runner installs the extension per run. + if (opts.dialogGateActive) env.AGENTA_AGENT_PI_DIALOG_GATE = "1"; if (opts.usageOutPath) env.AGENTA_AGENT_USAGE_CAPTURE_PATH = opts.usageOutPath; return env; @@ -281,7 +287,9 @@ export function prepareLocalPiAssets({ installPiExtensionLocal(process.env.PI_CODING_AGENT_DIR, log); } else { // unset here means this run has no Agenta extension (tracing + tools); warn so it's visible. - log("PI_CODING_AGENT_DIR is unset; plain local Pi run has no Agenta extension installed"); + log( + "PI_CODING_AGENT_DIR is unset; plain local Pi run has no Agenta extension installed", + ); } return undefined; } diff --git a/services/runner/src/engines/sandbox_agent/pi-gate-envelope.ts b/services/runner/src/engines/sandbox_agent/pi-gate-envelope.ts new file mode 100644 index 0000000000..eb9187276f --- /dev/null +++ b/services/runner/src/engines/sandbox_agent/pi-gate-envelope.ts @@ -0,0 +1,139 @@ +/** + * The Pi approval-gate envelope: the one sandbox-internal contract this feature adds. + * + * A Pi gate stops expressing an approval as a file-relay wait and raises it as + * `ctx.ui.confirm(PI_GATE_DIALOG_TITLE, )` from inside the sandbox. The + * `pi-acp` bridge forwards only the dialog strings (`{method, title, message}`) with a + * synthetic `pi-ui-` tool-call id, so the real gate identity (tool name, the model's + * tool-call id, the arguments) is tunneled through the `message` field as this JSON envelope. + * The runner parses it back into a real gate identity at the permission responder. + * + * This module is imported by BOTH sides — the in-sandbox extension (bundled by esbuild) and + * the runner — so the build and parse stay one source of truth. Keep it dependency-free (no + * node built-ins) so it bundles cleanly into the extension. + * + * Field roles (design-interfaces): `v`/`kind` are protocol context (version + discriminator so + * an unrelated future `confirm` can never be misread as a gate); `gate` is routing (which gate + * raised it); `toolName`/`toolCallId`/`input` are the gate identity (data). The envelope + * carries identity ONLY, never policy — the runner recovers permission metadata from the run's + * own resolved specs, because the sandbox is not trusted to state its own permissions. + */ + +/** The fixed `ctx.ui.confirm` title, used as the cheap pre-filter for a Pi gate dialog. */ +export const PI_GATE_DIALOG_TITLE = "agenta-approval"; + +/** The envelope version. A request whose title matches but whose version differs fails closed. */ +export const PI_GATE_ENVELOPE_VERSION = 1; + +/** The envelope discriminator, so a stray `confirm` from a future extension cannot misclassify. */ +export const PI_GATE_ENVELOPE_KIND = "agenta.gate"; + +/** Which Pi gate raised the dialog. Routes the runner's `GateDescriptor.executor`. */ +export type PiGateKind = "pi-builtin" | "pi-custom-tool"; + +export interface PiGateEnvelope { + v: typeof PI_GATE_ENVELOPE_VERSION; + kind: typeof PI_GATE_ENVELOPE_KIND; + gate: PiGateKind; + /** The tool name the decision map keys on (the builtin canonical name or the custom spec name). */ + toolName: string; + /** The model's real tool-call id (NOT the bridge's synthetic `pi-ui-`). */ + toolCallId: string; + /** The call arguments, verbatim, so the approval card and the stored-decision key are exact. */ + input: unknown; +} + +export interface BuildPiGateEnvelopeInput { + gate: PiGateKind; + toolName: string; + toolCallId: string; + input: unknown; +} + +/** Serialize a gate identity into the dialog `message` string (extension side). */ +export function buildPiGateEnvelope(input: BuildPiGateEnvelopeInput): string { + const envelope: PiGateEnvelope = { + v: PI_GATE_ENVELOPE_VERSION, + kind: PI_GATE_ENVELOPE_KIND, + gate: input.gate, + toolName: input.toolName, + toolCallId: input.toolCallId, + input: input.input, + }; + return JSON.stringify(envelope); +} + +/** + * The outcome of inspecting one ACP permission request for a Pi gate envelope: + * - `matched: false` — the dialog title is not ours; not a Pi gate, take today's path. + * - `matched: true, envelope: undefined` — the title IS ours but the envelope did not parse; + * the caller MUST fail closed (reject), never fall through (a fallthrough under a + * default-allow plan would confirm an unapproved execution). + * - `matched: true, envelope` — parsed; classify from the identity. + */ +export type PiGateParseResult = + { matched: false } | { matched: true; envelope?: PiGateEnvelope }; + +/** The dialog `message` string carried on an ACP permission request, or undefined. */ +function gateMessageOf(request: unknown): string | undefined { + const toolCall = (request as { toolCall?: unknown } | undefined)?.toolCall; + const rawInput = (toolCall as { rawInput?: unknown } | undefined)?.rawInput; + const message = (rawInput as { message?: unknown } | undefined)?.message; + return typeof message === "string" ? message : undefined; +} + +/** The dialog title carried on an ACP permission request, or undefined. */ +function gateTitleOf(request: unknown): string | undefined { + const toolCall = (request as { toolCall?: unknown } | undefined)?.toolCall; + const title = (toolCall as { title?: unknown } | undefined)?.title; + return typeof title === "string" ? title : undefined; +} + +/** + * Strict, version-checked parse of an ACP permission request into a Pi gate envelope. + * + * The title is the pre-filter: a request whose title is not `PI_GATE_DIALOG_TITLE` is not our + * gate (`matched: false`). A request whose title matches but whose envelope is malformed + * (unparseable JSON, wrong `kind`/`v`, missing identity) returns `matched: true` with no + * envelope so the caller fails closed. + */ +export function parsePiGateEnvelope(request: unknown): PiGateParseResult { + if (gateTitleOf(request) !== PI_GATE_DIALOG_TITLE) return { matched: false }; + + const message = gateMessageOf(request); + if (message === undefined) return { matched: true }; + + let parsed: unknown; + try { + parsed = JSON.parse(message); + } catch { + return { matched: true }; + } + const envelope = validatePiGateEnvelope(parsed); + return { matched: true, envelope }; +} + +function isPiGateKind(value: unknown): value is PiGateKind { + return value === "pi-builtin" || value === "pi-custom-tool"; +} + +/** Return the envelope only when every required identity field is present and well-typed. */ +function validatePiGateEnvelope(value: unknown): PiGateEnvelope | undefined { + if (typeof value !== "object" || value === null) return undefined; + const record = value as Record; + if (record.v !== PI_GATE_ENVELOPE_VERSION) return undefined; + if (record.kind !== PI_GATE_ENVELOPE_KIND) return undefined; + if (!isPiGateKind(record.gate)) return undefined; + if (typeof record.toolName !== "string" || !record.toolName) return undefined; + if (typeof record.toolCallId !== "string" || !record.toolCallId) + return undefined; + if (!("input" in record)) return undefined; + return { + v: PI_GATE_ENVELOPE_VERSION, + kind: PI_GATE_ENVELOPE_KIND, + gate: record.gate, + toolName: record.toolName, + toolCallId: record.toolCallId, + input: record.input, + }; +} diff --git a/services/runner/src/extensions/agenta.ts b/services/runner/src/extensions/agenta.ts index 6ed008210a..255ac20213 100644 --- a/services/runner/src/extensions/agenta.ts +++ b/services/runner/src/extensions/agenta.ts @@ -30,6 +30,7 @@ import { readFileSync, unlinkSync, writeFileSync } from "node:fs"; import { isToolCallEventType, type ExtensionAPI, + type ExtensionContext, type ToolCallEvent, type ToolCallEventResult, } from "@earendil-works/pi-coding-agent"; @@ -38,6 +39,11 @@ 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"; +import { + buildPiGateEnvelope, + PI_GATE_DIALOG_TITLE, + type PiGateKind, +} from "../engines/sandbox_agent/pi-gate-envelope.ts"; /** Read the OTLP bearer from its runner-written file once, then best-effort delete it. */ export function readOtlpAuthFile(path?: string): string | undefined { @@ -81,6 +87,40 @@ function isTruthyFlag(raw: string | undefined): boolean { return normalized === "1" || normalized === "true"; } +/** + * Raise a Pi approval gate as an extension-UI dialog carrying the JSON envelope, instead of the + * file-relay poll. The `pi-acp` bridge surfaces this as a real ACP `session/request_permission` + * the runner holds, classifies, and (under keep-alive) parks. No `opts` are passed to `confirm`, + * so Pi arms no reaper and the dialog waits indefinitely; any cancellation resolves it to `false`, + * which is a fail-closed block. If the UI plane is somehow unavailable, block (never run + * unapproved). + */ +async function piDialogAllows( + ctx: ExtensionContext | undefined, + gate: PiGateKind, + toolName: string, + toolCallId: string, + input: unknown, +): Promise<{ allowed: boolean; reason?: string }> { + const ui = ctx?.ui; + const confirm = ui?.confirm; + if (!ui || typeof confirm !== "function") { + return { allowed: false, reason: "Permission dialog is unavailable." }; + } + const message = buildPiGateEnvelope({ gate, toolName, toolCallId, input }); + try { + const confirmed = await confirm.call(ui, PI_GATE_DIALOG_TITLE, message); + return confirmed === true + ? { allowed: true } + : { allowed: false, reason: "Denied by the permission policy." }; + } catch (err) { + return { + allowed: false, + reason: err instanceof Error ? err.message : "Permission dialog failed.", + }; + } +} + function isPiBuiltinToolName(name: string): name is PiBuiltinToolName { return PI_BUILTIN_TOOL_NAME_SET.has(name); } @@ -167,6 +207,7 @@ function registerBuiltinGating( pi: ExtensionAPI, relayDir: string | undefined, builtinGrants: readonly PiBuiltinToolName[], + dialogGate: boolean, ): void { pi.on("before_agent_start", async () => { pi.setActiveTools( @@ -180,9 +221,23 @@ function registerBuiltinGating( pi.on( "tool_call", - async (event): Promise => { + async (event, ctx): Promise => { const toolName = builtinToolNameFromEvent(event); if (!toolName) return undefined; + + // Dialog plane (flag on): the gate rides `ctx.ui.confirm`, so the runner holds and can park + // it. The relay path stays behind the flag for rollback. + if (dialogGate) { + const { allowed, reason } = await piDialogAllows( + ctx, + "pi-builtin", + toolName, + event.toolCallId, + event.input, + ); + return allowed ? undefined : blockReason(reason); + } + if (!relayDir) { return blockReason( "Permission check denied because the relay directory is missing.", @@ -246,7 +301,7 @@ function parseSkillsLoaded(raw: string | undefined): string[] { } /** Register public tool metadata as Pi tools whose execution relays to the runner. */ -function registerTools(pi: ExtensionAPI): void { +function registerTools(pi: ExtensionAPI, dialogGate: boolean): void { const raw = process.env.AGENTA_AGENT_TOOLS_PUBLIC_SPECS; const relayDir = process.env.AGENTA_AGENT_TOOLS_RELAY_DIR; if (!raw || !relayDir) return; @@ -261,6 +316,10 @@ function registerTools(pi: ExtensionAPI): void { let registered = 0; for (const spec of specs) { + // The dialog gate applies to EXECUTABLE custom tools only. `client` tools are + // browser-fulfilled across a turn boundary through the relay's own pause semantics; gating + // one via the dialog would be wrong, so they keep today's path. + const gateViaDialog = dialogGate && (spec.kind ?? "callback") !== "client"; pi.registerTool({ name: spec.name, label: spec.name, @@ -269,7 +328,35 @@ function registerTools(pi: ExtensionAPI): void { promptGuidelines: promptGuidelines(spec), // Pi accepts plain JSON Schema here (non-TypeBox validation path). parameters: (specInputSchema(spec) as any) ?? EMPTY_OBJECT_SCHEMA, - async execute(toolCallId: string, params: unknown, signal?: AbortSignal) { + async execute( + toolCallId: string, + params: unknown, + signal?: AbortSignal, + _onUpdate?: unknown, + ctx?: ExtensionContext, + ) { + // Gate BEFORE the relay execution: only an allow proceeds. A deny surfaces as the tool's + // result text (mirroring the relay's own deny), so the model loop continues. + if (gateViaDialog) { + const { allowed, reason } = await piDialogAllows( + ctx, + "pi-custom-tool", + spec.name, + toolCallId, + params, + ); + if (!allowed) { + return { + content: [ + { + type: "text", + text: reason ?? "Denied by the permission policy.", + }, + ], + details: { toolName: spec.name }, + }; + } + } const text = await runResolvedTool(spec, params, { toolCallId, relayDir, @@ -301,11 +388,17 @@ const factory = (pi: ExtensionAPI): void => { const builtinGrants = normalizeBuiltinGrants( process.env.AGENTA_AGENT_BUILTIN_GRANTS, ); + // Approval parking (Option C): route both Pi gates over the extension-UI dialog plane instead + // of the file relay, so the runner can hold and park an ask. Runner-side flag + // AGENTA_RUNNER_PI_DIALOG_GATE -> sandbox AGENTA_AGENT_PI_DIALOG_GATE (buildPiExtensionEnv). + // Default off: with it off, both gates keep the byte-identical relay path. + const dialogGate = isTruthyFlag(process.env.AGENTA_AGENT_PI_DIALOG_GATE); const usageOut = process.env.AGENTA_AGENT_USAGE_CAPTURE_PATH; if (!hasTracing && !hasTools && !hasBuiltinGating && !usageOut) return; - if (hasTools) registerTools(pi); - if (hasBuiltinGating) registerBuiltinGating(pi, relayDir, builtinGrants); + if (hasTools) registerTools(pi, dialogGate); + if (hasBuiltinGating) + registerBuiltinGating(pi, relayDir, builtinGrants, dialogGate); // Tracing exports the span tree (when the OTLP target is reachable, i.e. local runs). // Usage accumulation is needed both for that export AND for the writeback the runner // uses on Daytona (where the in-sandbox process can't reach Agenta's OTLP, so the diff --git a/services/runner/src/responder.ts b/services/runner/src/responder.ts index aeae8b720e..316f9c07f5 100644 --- a/services/runner/src/responder.ts +++ b/services/runner/src/responder.ts @@ -236,6 +236,27 @@ export class ConversationDecisions implements StoredPermissionDecisions { return value; } + /** + * Re-append one decision to the FRONT of this call's queue (the Pi double-gate bridge). + * + * A Pi custom-tool dialog gate and the relay's execution check both `decide()` the SAME + * (name + canonical args) key: the dialog approves the call, then the relay watcher re-checks + * it before executing. When the dialog answered from a STORED decision it consumed one queued + * entry, so the relay's later `take` would find none and pause a second time. Re-appending the + * consumed decision lets the relay consume exactly what the human already granted + * (consume-1-append-1). It goes to the FRONT, not the back, because the relay's `decide` is the + * immediate next `take` on this call (Pi runs tools sequentially): a back-append would hand the + * relay a LATER identical call's decision. A single call empties the queue first, so front and + * back coincide there; front is correct for the 2+ identical-call case too. + */ + appendDecision(gate: GateDescriptor, decision: "allow" | "deny"): void { + const key = approvedCallKey(gate.toolName, gate.args); + if (!key) return; + const queue = this.decisionQueues.get(key); + if (queue) queue.unshift(decision); + else this.decisionQueues.set(key, [decision]); + } + /** The next FIFO client-tool output for this exact call, without consuming it. */ peekClientOutput(gate: GateDescriptor): { found: boolean; output?: unknown } { const entry = this.nextClientOutput(gate); @@ -271,16 +292,46 @@ export class ConversationDecisions implements StoredPermissionDecisions { * in execution shape: `allow` and `ask` both mean "forward to the browser and pause unless a * stored browser output is already available"; `deny` refuses the call. */ +export interface ApprovalResponderOptions { + /** + * The Pi double-gate bridge (dialog-gate runs only). ON only when the run routes Pi gates + * over the dialog plane, where a dialog-allowed custom tool still hits the relay watcher's + * own `decide()` before executing. It must stay OFF everywhere else: on Claude the relay + * never enforces (`enforce: plan.isPi`), so a relay-shaped Claude gate has no second + * consumer and an appended decision would linger and mis-resolve a LATER identical call, + * changing flag-off behavior. + */ + bridgeRelayDoubleGate?: boolean; +} + export class ApprovalResponder implements Responder { constructor( private readonly plan: PermissionPlan, private readonly decisions: ConversationDecisions, private readonly log: (msg: string) => void = () => {}, + private readonly options: ApprovalResponderOptions = {}, ) {} async onPermission(request: PermissionGateRequest): Promise { const permission = effectivePermission(request.gate, this.plan); const verdict = decide(request.gate, this.plan, this.decisions); + // Pi double-gate bridge: a pi-custom-tool dialog gate (executor "relay") whose "ask" decision + // was answered instantly from a STORED allow consumed one queued entry; the relay's own + // execution check will `decide()` the same call again and must see it. Re-append exactly the + // consumed decision. Guarded so it fires ONLY when the relay will actually consume it: + // allow only (on a deny the extension short-circuits without relaying, so an appended deny + // would linger and auto-deny a later identical call that should re-prompt); "ask" only (an + // allow/deny policy consumes nothing — decide returns before `stored.take` — and a pause + // consumed nothing); relay executor only (builtins have no relay second gate); and only + // when the dialog-gate bridge is on (see ApprovalResponderOptions). + if ( + this.options.bridgeRelayDoubleGate && + request.gate.executor === "relay" && + permission === "ask" && + verdict.kind === "allow" + ) { + this.decisions.appendDecision(request.gate, verdict.kind); + } this.log( `[HITL] gate toolName=${JSON.stringify(request.gate.toolName)} ` + `permission=${permission} outcome=${verdict.kind}`, diff --git a/services/runner/src/server.ts b/services/runner/src/server.ts index dd83a623ca..c73862c524 100644 --- a/services/runner/src/server.ts +++ b/services/runner/src/server.ts @@ -378,17 +378,17 @@ export async function runWithKeepalive( env.lastTurnToolCallIds ?? [], ); - // Whether a paused turn holds a single, parkable Claude ACP permission gate (slice 2). Only such - // a gate carries a `respondPermission`-answerable id; a Pi relay/builtin gate or a client-tool - // MCP pause never records `parkedApproval`, and more than one pending gate cannot be answered by - // the single-gate resume — both stay on the cold path, logged. + // Whether a paused turn holds a single, parkable permission gate (a Claude ACP gate or a Pi + // dialog gate). Only such a gate carries a `respondPermission`-answerable id; a Pi file-relay + // gate or a client-tool MCP pause never records `parkedApproval`, and more than one pending + // gate cannot be answered by the single-gate resume — both stay on the cold path, logged. const approvalToPark = ( env: SessionEnvironment, result: AgentRunResult, ): boolean => { if (result.stopReason !== "paused") return false; if (!env.parkedApproval) { - klog(`non-claude-gate-no-park key=${key}`); + klog(`non-parkable-gate-no-park key=${key}`); return false; } if ((env.approvalGateCount ?? 0) > 1) { @@ -612,8 +612,14 @@ export async function runWithKeepalive( : undefined; const priorFp = historyFingerprint(priorConversation(request)); let mismatch: string | undefined; - if (!parked || parked.gateType !== "claude-acp-permission") { - mismatch = "not-claude-gate"; // defensive: only a Claude ACP gate ever parks here + if ( + !parked || + (parked.gateType !== "claude-acp-permission" && + parked.gateType !== "pi-dialog-permission") + ) { + // Defensive: only a parkable gate type (Claude ACP or Pi dialog) ever parks here. Both + // resume via `respondPermission` on the live session; the daemon maps the reply by kind. + mismatch = "unrecognized-gate-type"; } else if (!decision) { mismatch = "no-matching-approval"; // fresh user text, or an approval for another id } else if (priorFp !== existing.historyFingerprint) { diff --git a/services/runner/tests/unit/extension-tools.test.ts b/services/runner/tests/unit/extension-tools.test.ts index a328100c3e..85976f3f75 100644 --- a/services/runner/tests/unit/extension-tools.test.ts +++ b/services/runner/tests/unit/extension-tools.test.ts @@ -12,7 +12,13 @@ */ import { afterEach, describe, it } from "vitest"; import assert from "node:assert/strict"; -import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + existsSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -32,8 +38,27 @@ const TOOL_ENV = [ "AGENTA_AGENT_CONTENT_CAPTURE_ENABLED", "AGENTA_AGENT_BUILTIN_GATING", "AGENTA_AGENT_BUILTIN_GRANTS", + "AGENTA_AGENT_PI_DIALOG_GATE", ]; +/** A fake extension UI context whose `confirm` records its calls and returns a scripted answer. */ +function fakeDialogCtx(answer: boolean | (() => Promise)) { + const calls: Array<{ title: string; message: string }> = []; + return { + calls, + ctx: { + mode: "rpc" as const, + hasUI: true, + ui: { + async confirm(title: string, message: string) { + calls.push({ title, message }); + return typeof answer === "function" ? await answer() : answer; + }, + }, + }, + }; +} + function fakePi(opts: { activeTools?: string[]; allTools?: string[] } = {}) { const registered: any[] = []; const handlers: Record = {}; @@ -95,15 +120,27 @@ describe("agenta extension tool registration", () => { const math = pi.registered[0]; assert.equal(math.description, "qa math", "carries the description"); assert.ok( - math.parameters && math.parameters.properties && math.parameters.properties.x, + math.parameters && + math.parameters.properties && + math.parameters.properties.x, "passes the JSON Schema through to Pi", ); - assert.equal(math.promptSnippet, "qa math", "opts the tool into Pi's Available tools prompt"); + assert.equal( + math.promptSnippet, + "qa math", + "opts the tool into Pi's Available tools prompt", + ); assert.ok( - math.promptGuidelines.some((line: string) => line.includes("required argument(s): x")), + math.promptGuidelines.some((line: string) => + line.includes("required argument(s): x"), + ), "adds prompt guidance for required arguments", ); - assert.equal(typeof math.execute, "function", "each tool has an execute() that relays"); + assert.equal( + typeof math.execute, + "function", + "each tool has an execute() that relays", + ); const noSchema = pi.registered[1]; assert.ok( @@ -236,7 +273,9 @@ describe("agenta extension tool registration", () => { it("does not register when specs are present but the relay dir is missing", () => { clearEnv(); - process.env.AGENTA_AGENT_TOOLS_PUBLIC_SPECS = JSON.stringify([{ name: "x" }]); + process.env.AGENTA_AGENT_TOOLS_PUBLIC_SPECS = JSON.stringify([ + { name: "x" }, + ]); const pi = fakePi(); factory(pi as any); assert.equal( @@ -265,3 +304,155 @@ describe("readOtlpAuthFile", () => { assert.equal(readOtlpAuthFile("/nonexistent/agenta-otlp-auth"), undefined); }); }); + +describe("agenta extension: Pi dialog gate (approval parking)", () => { + function builtinEvent(toolName: string, input: unknown) { + return { type: "tool_call", toolName, toolCallId: "tc-b", input }; + } + + it("builtin gate rides ctx.ui.confirm with the envelope; allow -> undefined", async () => { + clearEnv(); + process.env.AGENTA_AGENT_BUILTIN_GATING = "1"; + process.env.AGENTA_AGENT_TOOLS_RELAY_DIR = "/tmp/agenta-relay-unused"; + process.env.AGENTA_AGENT_PI_DIALOG_GATE = "1"; + + const pi = fakePi(); + factory(pi as any); + const hook = pi.handlers.tool_call![0]; + const { calls, ctx } = fakeDialogCtx(true); + + const result = await hook(builtinEvent("bash", { command: "ls" }), ctx); + assert.equal(result, undefined, "allow -> the builtin proceeds"); + assert.equal(calls.length, 1, "the dialog was raised (not the relay)"); + assert.equal(calls[0].title, "agenta-approval"); + const envelope = JSON.parse(calls[0].message); + assert.equal(envelope.kind, "agenta.gate"); + assert.equal(envelope.gate, "pi-builtin"); + assert.equal(envelope.toolName, "bash"); + assert.deepEqual(envelope.input, { command: "ls" }); + }); + + it("builtin gate: deny -> block, and a thrown/absent dialog fails closed (block)", async () => { + clearEnv(); + process.env.AGENTA_AGENT_BUILTIN_GATING = "1"; + process.env.AGENTA_AGENT_PI_DIALOG_GATE = "1"; + + const pi = fakePi(); + factory(pi as any); + const hook = pi.handlers.tool_call![0]; + + const denied = await hook( + builtinEvent("bash", {}), + fakeDialogCtx(false).ctx, + ); + assert.equal(denied.block, true, "deny -> block"); + + const threw = await hook( + builtinEvent("bash", {}), + fakeDialogCtx(async () => { + throw new Error("dialog transport gone"); + }).ctx, + ); + assert.equal(threw.block, true, "a thrown dialog fails closed"); + + const noUi = await hook(builtinEvent("bash", {}), { + mode: "rpc", + hasUI: false, + }); + assert.equal(noUi.block, true, "no UI plane fails closed"); + }); + + it("custom-tool gate: a deny returns the reason WITHOUT relaying (early return)", async () => { + clearEnv(); + process.env.AGENTA_AGENT_TOOLS_PUBLIC_SPECS = JSON.stringify([ + { name: "park_probe", description: "echo", kind: "callback" }, + ]); + // A relay dir that does not exist: if the deny path relayed, the poll would hang/fail. It must + // not be reached. + process.env.AGENTA_AGENT_TOOLS_RELAY_DIR = + "/tmp/agenta-relay-must-not-be-used"; + process.env.AGENTA_AGENT_PI_DIALOG_GATE = "1"; + + const pi = fakePi(); + factory(pi as any); + const tool = pi.registered[0]; + const { calls, ctx } = fakeDialogCtx(false); + + const result = await tool.execute( + "call_1", + { token: "T" }, + undefined, + undefined, + ctx, + ); + assert.equal(calls.length, 1, "the dialog was raised before the relay"); + const envelope = JSON.parse(calls[0].message); + assert.equal(envelope.gate, "pi-custom-tool"); + assert.equal(envelope.toolName, "park_probe"); + assert.deepEqual(envelope.input, { token: "T" }); + assert.ok( + result.content[0].text.toLowerCase().includes("denied"), + "a denied custom tool returns the deny reason as its result", + ); + }); + + it("custom-tool gate: a CLIENT spec is NOT dialog-gated (keeps its relay path)", async () => { + clearEnv(); + const dir = mkdtempSync(join(tmpdir(), "agenta-relay-client-")); + // Pre-seed the relay response so the client tool's relay returns immediately. + writeFileSync( + join(dir, "cclient.res.json"), + JSON.stringify({ ok: true, text: "browser-fulfilled" }), + "utf-8", + ); + process.env.AGENTA_AGENT_TOOLS_PUBLIC_SPECS = JSON.stringify([ + { name: "request_connection", description: "connect", kind: "client" }, + ]); + process.env.AGENTA_AGENT_TOOLS_RELAY_DIR = dir; + process.env.AGENTA_AGENT_PI_DIALOG_GATE = "1"; + + const pi = fakePi(); + factory(pi as any); + const tool = pi.registered[0]; + const { calls, ctx } = fakeDialogCtx(false); + + const result = await tool.execute( + "cclient", + { integration: "slack" }, + undefined, + undefined, + ctx, + ); + assert.equal(calls.length, 0, "a client tool is never dialog-gated"); + assert.equal( + result.content[0].text, + "browser-fulfilled", + "it took the relay path", + ); + rmSync(dir, { recursive: true, force: true }); + }); + + it("with the dialog flag OFF, the builtin gate keeps the relay path (no dialog raised)", async () => { + clearEnv(); + const dir = mkdtempSync(join(tmpdir(), "agenta-relay-off-")); + process.env.AGENTA_AGENT_BUILTIN_GATING = "1"; + process.env.AGENTA_AGENT_TOOLS_RELAY_DIR = dir; + // AGENTA_AGENT_PI_DIALOG_GATE intentionally unset. + + const pi = fakePi(); + factory(pi as any); + const hook = pi.handlers.tool_call![0]; + const { calls, ctx } = fakeDialogCtx(true); + + // Pre-seed the relay permission response (allow) so the relay path resolves without a runner. + writeFileSync( + join(dir, "tc-b.res.json"), + JSON.stringify({ kind: "permission", ok: true, verdict: "allow" }), + "utf-8", + ); + const result = await hook(builtinEvent("bash", { command: "ls" }), ctx); + assert.equal(calls.length, 0, "flag off: the dialog is never raised"); + assert.equal(result, undefined, "the relay allow let the builtin proceed"); + rmSync(dir, { recursive: true, force: true }); + }); +}); diff --git a/services/runner/tests/unit/pi-gate-envelope.test.ts b/services/runner/tests/unit/pi-gate-envelope.test.ts new file mode 100644 index 0000000000..6e654f775a --- /dev/null +++ b/services/runner/tests/unit/pi-gate-envelope.test.ts @@ -0,0 +1,277 @@ +/** + * Unit tests for the Pi approval-gate envelope (the one sandbox-internal contract) and the + * runner-side classification of a Pi dialog gate into a GateDescriptor. + * + * Run: pnpm test (or: pnpm exec vitest run tests/unit/pi-gate-envelope.test.ts) + */ +import { describe, it } from "vitest"; +import assert from "node:assert/strict"; + +import { + buildPiGateEnvelope, + parsePiGateEnvelope, + PI_GATE_DIALOG_TITLE, + type PiGateEnvelope, +} from "../../src/engines/sandbox_agent/pi-gate-envelope.ts"; +import { buildPiGateDescriptor } from "../../src/engines/sandbox_agent/acp-interactions.ts"; + +/** Wrap an envelope string the way the pi-acp bridge delivers it on an ACP permission request. */ +function asRequest(message: unknown, title = PI_GATE_DIALOG_TITLE) { + return { + id: "perm-1", + availableReplies: ["once", "reject"], + toolCall: { + toolCallId: "pi-ui-synthetic", + title, + rawInput: { method: "confirm", title, message }, + }, + }; +} + +describe("pi-gate-envelope build/parse round-trip", () => { + it("round-trips a custom-tool gate byte-exact, including hostile strings", () => { + const input = { + token: "TOKEN-ALLOW-a1b2", + probe: 'quotes"and\\back\\slashes and 日本語 and \n newline', + }; + const message = buildPiGateEnvelope({ + gate: "pi-custom-tool", + toolName: "park_probe", + toolCallId: "call_6JFT|fc_0d79", + input, + }); + const result = parsePiGateEnvelope(asRequest(message)); + assert.equal(result.matched, true); + assert.ok(result.matched && result.envelope); + const envelope = (result as { envelope: PiGateEnvelope }).envelope; + assert.equal(envelope.gate, "pi-custom-tool"); + assert.equal(envelope.toolName, "park_probe"); + assert.equal(envelope.toolCallId, "call_6JFT|fc_0d79"); + assert.deepEqual(envelope.input, input); + }); + + it("round-trips a builtin gate", () => { + const message = buildPiGateEnvelope({ + gate: "pi-builtin", + toolName: "bash", + toolCallId: "call_x", + input: { command: "ls" }, + }); + const result = parsePiGateEnvelope(asRequest(message)); + assert.ok(result.matched && result.envelope); + assert.equal(result.envelope!.gate, "pi-builtin"); + assert.equal(result.envelope!.toolName, "bash"); + }); +}); + +describe("parsePiGateEnvelope classification", () => { + it("a non-matching dialog title is NOT a Pi gate (takes today's path)", () => { + const message = buildPiGateEnvelope({ + gate: "pi-builtin", + toolName: "bash", + toolCallId: "call_x", + input: {}, + }); + const result = parsePiGateEnvelope(asRequest(message, "some-other-title")); + assert.deepEqual(result, { matched: false }); + }); + + it("a plain Claude ACP gate (no dialog title) is not matched", () => { + const result = parsePiGateEnvelope({ + id: "perm-1", + toolCall: { toolCallId: "tc-1", title: "commit", rawInput: { a: 1 } }, + }); + assert.deepEqual(result, { matched: false }); + }); + + it("matched title with unparseable JSON fails closed (matched, no envelope)", () => { + const result = parsePiGateEnvelope(asRequest("{not json")); + assert.equal(result.matched, true); + assert.equal((result as { envelope?: unknown }).envelope, undefined); + }); + + it("matched title with wrong kind fails closed", () => { + const bad = JSON.stringify({ + v: 1, + kind: "something.else", + gate: "pi-builtin", + toolName: "bash", + toolCallId: "c", + input: {}, + }); + const result = parsePiGateEnvelope(asRequest(bad)); + assert.equal(result.matched, true); + assert.equal((result as { envelope?: unknown }).envelope, undefined); + }); + + it("matched title with wrong version fails closed", () => { + const bad = JSON.stringify({ + v: 2, + kind: "agenta.gate", + gate: "pi-builtin", + toolName: "bash", + toolCallId: "c", + input: {}, + }); + const result = parsePiGateEnvelope(asRequest(bad)); + assert.equal(result.matched, true); + assert.equal((result as { envelope?: unknown }).envelope, undefined); + }); + + it("matched title with unknown gate kind fails closed", () => { + const bad = JSON.stringify({ + v: 1, + kind: "agenta.gate", + gate: "pi-something", + toolName: "bash", + toolCallId: "c", + input: {}, + }); + const result = parsePiGateEnvelope(asRequest(bad)); + assert.equal(result.matched, true); + assert.equal((result as { envelope?: unknown }).envelope, undefined); + }); + + it("matched title missing identity (no toolName / no toolCallId) fails closed", () => { + for (const bad of [ + { + v: 1, + kind: "agenta.gate", + gate: "pi-builtin", + toolCallId: "c", + input: {}, + }, + { + v: 1, + kind: "agenta.gate", + gate: "pi-builtin", + toolName: "bash", + input: {}, + }, + { + v: 1, + kind: "agenta.gate", + gate: "pi-builtin", + toolName: "bash", + toolCallId: "c", + }, + ]) { + const result = parsePiGateEnvelope(asRequest(JSON.stringify(bad))); + assert.equal(result.matched, true); + assert.equal((result as { envelope?: unknown }).envelope, undefined); + } + }); +}); + +describe("buildPiGateDescriptor (runner-side metadata recovery)", () => { + it("pi-builtin -> harness executor with the builtin's rule name and read-only hint", () => { + const readGate = buildPiGateDescriptor( + { + v: 1, + kind: "agenta.gate", + gate: "pi-builtin", + toolName: "read", + toolCallId: "c", + input: { path: "a" }, + }, + undefined, + ); + assert.equal(readGate!.executor, "harness"); + assert.equal(readGate!.toolName, "Read"); + assert.equal(readGate!.readOnlyHint, true); + + const bashGate = buildPiGateDescriptor( + { + v: 1, + kind: "agenta.gate", + gate: "pi-builtin", + toolName: "bash", + toolCallId: "c", + input: { command: "ls" }, + }, + undefined, + ); + assert.equal(bashGate!.toolName, "Bash"); + assert.equal(bashGate!.readOnlyHint, false); + }); + + it("pi-custom-tool -> relay executor with author permission + readOnly recovered by name", () => { + const specs = new Map([ + ["author_allow", { permission: "allow" as const, readOnly: false }], + ["author_deny", { permission: "deny" as const, readOnly: false }], + ["reader", { readOnly: true }], + ]); + const allow = buildPiGateDescriptor( + { + v: 1, + kind: "agenta.gate", + gate: "pi-custom-tool", + toolName: "author_allow", + toolCallId: "c", + input: {}, + }, + specs, + ); + assert.equal(allow!.executor, "relay"); + assert.equal(allow!.specPermission, "allow"); + + const deny = buildPiGateDescriptor( + { + v: 1, + kind: "agenta.gate", + gate: "pi-custom-tool", + toolName: "author_deny", + toolCallId: "c", + input: {}, + }, + specs, + ); + assert.equal(deny!.specPermission, "deny"); + + const reader = buildPiGateDescriptor( + { + v: 1, + kind: "agenta.gate", + gate: "pi-custom-tool", + toolName: "reader", + toolCallId: "c", + input: {}, + }, + specs, + ); + assert.equal(reader!.specPermission, undefined); + assert.equal(reader!.readOnlyHint, true); + }); + + it("the envelope input is the gate args (stored-decision key parity with the relay)", () => { + const g = buildPiGateDescriptor( + { + v: 1, + kind: "agenta.gate", + gate: "pi-custom-tool", + toolName: "t", + toolCallId: "c", + input: { a: 1, b: 2 }, + }, + new Map(), + ); + assert.deepEqual(g!.args, { a: 1, b: 2 }); + }); + + it("an unknown builtin name yields NO descriptor (the caller must reject it)", () => { + // Relay parity: the relay denies unknown builtins outright, and the sandbox-origin envelope + // must not put a fabricated name on the approval card. + const g = buildPiGateDescriptor( + { + v: 1, + kind: "agenta.gate", + gate: "pi-builtin", + toolName: "fabricated_tool", + toolCallId: "c", + input: {}, + }, + undefined, + ); + assert.equal(g, undefined); + }); +}); diff --git a/services/runner/tests/unit/responder.test.ts b/services/runner/tests/unit/responder.test.ts index 4e414bf82a..e9120b84b0 100644 --- a/services/runner/tests/unit/responder.test.ts +++ b/services/runner/tests/unit/responder.test.ts @@ -8,7 +8,11 @@ import assert from "node:assert/strict"; import { createSandboxAgentOtel } from "../../src/tracing/otel.ts"; import type { AgentEvent, AgentRunRequest } from "../../src/protocol.ts"; -import type { GateDescriptor, PermissionPlan } from "../../src/permission-plan.ts"; +import type { + GateDescriptor, + PermissionPlan, +} from "../../src/permission-plan.ts"; +import { decide } from "../../src/permission-plan.ts"; import { ApprovalResponder, ConversationDecisions, @@ -82,7 +86,10 @@ describe("approvedCallKey", () => { it("normalizes absent args to {} so a no-arg tool resumes", () => { assert.ok(approvedCallKey("edit", {})); - assert.equal(approvedCallKey("edit", undefined), approvedCallKey("edit", {})); + assert.equal( + approvedCallKey("edit", undefined), + approvedCallKey("edit", {}), + ); assert.equal(approvedCallKey("edit", null), approvedCallKey("edit", {})); assert.notEqual( approvedCallKey("edit", { path: "a" }), @@ -212,10 +219,17 @@ describe("ApprovalResponder", () => { it("allows and denies from the effective permission before stored decisions", async () => { const key = approvedCallKey("edit", { path: "a.txt" })!; const stored = new Map([[key, "allow"]]); - const deny = new ApprovalResponder(plan("deny"), new ConversationDecisions(stored)); + const deny = new ApprovalResponder( + plan("deny"), + new ConversationDecisions(stored), + ); assert.deepEqual(await permissionVerdict(deny, gate()), { kind: "deny" }); - assert.equal(stored.has(key), true, "effective deny does not consume stale allow"); + assert.equal( + stored.has(key), + true, + "effective deny does not consume stale allow", + ); const allow = new ApprovalResponder( plan("allow"), @@ -231,7 +245,9 @@ describe("ApprovalResponder", () => { new ConversationDecisions(new Map([[key, "allow"]])), ); - assert.deepEqual(await permissionVerdict(responder, gate()), { kind: "allow" }); + assert.deepEqual(await permissionVerdict(responder, gate()), { + kind: "allow", + }); assert.deepEqual(await permissionVerdict(responder, gate()), { kind: "pendingApproval", }); @@ -244,12 +260,17 @@ describe("ApprovalResponder", () => { new ConversationDecisions(new Map([[key, "deny"]])), ); - assert.deepEqual(await permissionVerdict(responder, gate()), { kind: "deny" }); + assert.deepEqual(await permissionVerdict(responder, gate()), { + kind: "deny", + }); assert.deepEqual(await permissionVerdict(responder, gate()), { kind: "pendingApproval", }); - const noStored = new ApprovalResponder(plan("ask"), new ConversationDecisions(new Map())); + const noStored = new ApprovalResponder( + plan("ask"), + new ConversationDecisions(new Map()), + ); assert.deepEqual(await permissionVerdict(noStored, gate()), { kind: "pendingApproval", }); @@ -276,7 +297,9 @@ describe("ApprovalResponder", () => { }); it("client tools peek at stored output by default", async () => { - const key = approvedCallKey("request_connection", { integration: "slack" })!; + const key = approvedCallKey("request_connection", { + integration: "slack", + })!; const output = { connected: true }; const responder = new ApprovalResponder( plan("deny"), @@ -300,7 +323,9 @@ describe("ApprovalResponder", () => { }); it("client tools consume stored output when the relay fulfills", async () => { - const key = approvedCallKey("request_connection", { integration: "slack" })!; + const key = approvedCallKey("request_connection", { + integration: "slack", + })!; const output = { connected: true }; const responder = new ApprovalResponder( plan("deny"), @@ -323,7 +348,9 @@ describe("ApprovalResponder", () => { }); it("client tools support peek then consume for the Claude two-read flow", async () => { - const key = approvedCallKey("request_connection", { integration: "slack" })!; + const key = approvedCallKey("request_connection", { + integration: "slack", + })!; const output = { connected: true }; const responder = new ApprovalResponder( plan("deny"), @@ -348,7 +375,9 @@ describe("ApprovalResponder", () => { }); it("client explicit ask consumes stored deny; stored allow still forwards to the browser", async () => { - const denyKey = approvedCallKey("request_connection", { integration: "slack" })!; + const denyKey = approvedCallKey("request_connection", { + integration: "slack", + })!; const denyResponder = new ApprovalResponder( plan("allow"), new ConversationDecisions(new Map([[denyKey, "deny"]])), @@ -359,17 +388,23 @@ describe("ApprovalResponder", () => { specPermission: "ask", args: { integration: "slack" }, }); - assert.deepEqual(await denyResponder.onClientTool({ id: "tool-1", gate: client }), { - kind: "deny", - }); + assert.deepEqual( + await denyResponder.onClientTool({ id: "tool-1", gate: client }), + { + kind: "deny", + }, + ); const allowResponder = new ApprovalResponder( plan("allow"), new ConversationDecisions(new Map([[denyKey, "allow"]])), ); - assert.deepEqual(await allowResponder.onClientTool({ id: "tool-1", gate: client }), { - kind: "pendingApproval", - }); + assert.deepEqual( + await allowResponder.onClientTool({ id: "tool-1", gate: client }), + { + kind: "pendingApproval", + }, + ); }); }); @@ -412,8 +447,13 @@ describe("extractApprovalDecisions", () => { }; const decisions = extractApprovalDecisions(request); - assert.deepEqual(decisions.get(approvedCallKey("edit", { path: "a.txt" })!), ["allow"]); - assert.deepEqual(decisions.get(approvedCallKey("bash", { cmd: "ls" })!), ["deny"]); + assert.deepEqual( + decisions.get(approvedCallKey("edit", { path: "a.txt" })!), + ["allow"], + ); + assert.deepEqual(decisions.get(approvedCallKey("bash", { cmd: "ls" })!), [ + "deny", + ]); assert.equal(decisions.has("edit"), false); assert.equal(decisions.has("tc-1"), false); }); @@ -535,7 +575,9 @@ describe("extractApprovalDecisions", () => { ], }; - const key = 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), [ @@ -549,7 +591,11 @@ describe("extractApprovalDecisions", () => { { role: "tool", content: [ - { type: "tool_result", toolCallId: "tc-9", output: "the weather is 24C" }, + { + type: "tool_result", + toolCallId: "tc-9", + output: "the weather is 24C", + }, { type: "tool_result", toolCallId: "tc-10", output: { temp: 24 } }, { type: "text", text: "hello" }, ], @@ -569,7 +615,9 @@ describe("extractApprovalDecisions", () => { }); describe("client-tool output store (separate from approvals)", () => { - const clientGate = (input: unknown = { integration: "slack" }): GateDescriptor => ({ + const clientGate = ( + input: unknown = { integration: "slack" }, + ): GateDescriptor => ({ executor: "client", toolName: "request_connection", args: input, @@ -610,7 +658,9 @@ describe("client-tool output store (separate from approvals)", () => { ], }; const outputs = extractClientToolOutputs(request); - const key = approvedCallKey("request_connection", { integration: "slack" })!; + const key = approvedCallKey("request_connection", { + integration: "slack", + })!; assert.deepEqual(outputs.get(key), [ { connected: true, account: "first" }, { connected: true, account: "second" }, @@ -619,7 +669,9 @@ describe("client-tool output store (separate from approvals)", () => { assert.equal(outputs.has(approvedCallKey("edit", { path: "a" })!), false); // ...and lives only in the approval store. const decisions = extractApprovalDecisions(request); - assert.deepEqual(decisions.get(approvedCallKey("edit", { path: "a" })!), ["allow"]); + assert.deepEqual(decisions.get(approvedCallKey("edit", { path: "a" })!), [ + "allow", + ]); }); it("resolves two identical client calls from the FIFO store, in order", async () => { @@ -656,18 +708,27 @@ describe("client-tool output store (separate from approvals)", () => { ); 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" }, - }); + 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", - }); + assert.deepEqual( + await responder.onClientTool(request1, { consume: true }), + { + kind: "pendingApproval", + }, + ); }); it("does NOT fulfill a new identical call from a PRIOR turn's output (cross-turn)", async () => { @@ -771,7 +832,7 @@ describe("client-tool output store (separate from approvals)", () => { ); }); - it("returns a client output literally \"allow\" as output, never as a permission decision", async () => { + it('returns a client output literally "allow" as output, never as a permission decision', async () => { const request: AgentRunRequest = { sessionId: "s-client", messages: [ @@ -848,3 +909,222 @@ describe("emitEvent", () => { assert.equal((ev as any).name, "weather"); }); }); + +describe("ConversationDecisions.appendDecision (Pi double-gate bridge)", () => { + const relayGate: GateDescriptor = { + executor: "relay", + toolName: "park_probe", + args: { token: "T" }, + }; + + it("re-appended decision is consumed by the next take on the same call", () => { + const decisions = new ConversationDecisions(new Map()); + decisions.appendDecision(relayGate, "allow"); + assert.equal(decisions.take(relayGate), "allow"); + assert.equal(decisions.take(relayGate), undefined, "consumed once"); + }); + + it("front-inserts so the relay's take gets THIS call's decision, not a later one", () => { + // Two identical-arg calls with DIFFERENT decisions (allow then deny), FIFO order. + const key = approvedCallKey("park_probe", { token: "T" })!; + const decisions = new ConversationDecisions( + new Map([[key, ["allow", "deny"]]]), + ); + // Call A: the dialog takes "allow" then re-appends it for the relay. + assert.equal(decisions.take(relayGate), "allow"); + decisions.appendDecision(relayGate, "allow"); + // The relay's immediate next take must see A's "allow", not B's "deny". + assert.equal(decisions.take(relayGate), "allow"); + // Call B: the dialog takes "deny", re-appends, the relay takes "deny". + assert.equal(decisions.take(relayGate), "deny"); + decisions.appendDecision(relayGate, "deny"); + assert.equal(decisions.take(relayGate), "deny"); + }); +}); + +describe("ApprovalResponder: Pi custom-tool double-gate accounting", () => { + const askPlan: PermissionPlan = { default: "ask", rules: [] }; + const bridge = { bridgeRelayDoubleGate: true }; + const relayGate: GateDescriptor = { + executor: "relay", + toolName: "park_probe", + args: { token: "T" }, + }; + + it("a stored-answered relay gate re-appends so the relay's decide still allows", async () => { + const key = approvedCallKey("park_probe", { token: "T" })!; + const decisions = new ConversationDecisions(new Map([[key, ["allow"]]])); + const responder = new ApprovalResponder( + askPlan, + decisions, + undefined, + bridge, + ); + + const verdict = await responder.onPermission({ + id: "p", + availableReplies: ["once", "reject"], + gate: relayGate, + }); + assert.equal( + verdict.kind, + "allow", + "the dialog gate allowed from the stored decision", + ); + // The relay's SECOND check on the same call still finds an allow (consume-1-append-1). + assert.equal(decide(relayGate, askPlan, decisions).kind, "allow"); + }); + + it("does NOT append with the bridge OFF (default), even for a relay ask allow", async () => { + // The Claude shape: a relay-executor gate but no relay enforcement (`enforce: plan.isPi`), + // so nothing consumes an appended decision; it would leak the allow to a LATER identical + // call — a flag-off behavior change. The default responder never appends. + const key = approvedCallKey("park_probe", { token: "T" })!; + const decisions = new ConversationDecisions(new Map([[key, ["allow"]]])); + const responder = new ApprovalResponder(askPlan, decisions); + + const verdict = await responder.onPermission({ + id: "p", + availableReplies: ["once", "reject"], + gate: relayGate, + }); + assert.equal(verdict.kind, "allow"); + // Consumed once, nothing re-appended: a later identical gate re-prompts. + assert.equal(decide(relayGate, askPlan, decisions).kind, "pendingApproval"); + }); + + it("does NOT re-append a DENY (the extension short-circuits; no relay consumer)", async () => { + // On a deny the custom tool's execute() returns the deny text WITHOUT relaying, so nothing + // consumes an appended deny; it would linger and auto-deny a later identical call that + // should re-prompt instead. + const key = approvedCallKey("park_probe", { token: "T" })!; + const decisions = new ConversationDecisions(new Map([[key, ["deny"]]])); + const responder = new ApprovalResponder( + askPlan, + decisions, + undefined, + bridge, + ); + + const verdict = await responder.onPermission({ + id: "p", + availableReplies: ["once", "reject"], + gate: relayGate, + }); + assert.equal(verdict.kind, "deny"); + assert.equal( + decide(relayGate, askPlan, decisions).kind, + "pendingApproval", + "the deny was consumed once and NOT re-appended; a later identical call re-prompts", + ); + }); + + it("does NOT append for a builtin (harness executor) — no relay second gate", async () => { + const key = approvedCallKey("Bash", { command: "ls" })!; + const decisions = new ConversationDecisions(new Map([[key, ["allow"]]])); + const responder = new ApprovalResponder( + askPlan, + decisions, + undefined, + bridge, + ); + const harnessGate: GateDescriptor = { + executor: "harness", + toolName: "Bash", + args: { command: "ls" }, + }; + + const verdict = await responder.onPermission({ + id: "p", + availableReplies: ["once", "reject"], + gate: harnessGate, + }); + assert.equal(verdict.kind, "allow"); + // Consumed once; nothing re-appended, so a second take finds nothing. + assert.equal( + decide(harnessGate, askPlan, decisions).kind, + "pendingApproval", + ); + }); + + it("does NOT append when the policy (allow) decided without consulting stored", async () => { + const allowPlan: PermissionPlan = { default: "allow", rules: [] }; + const decisions = new ConversationDecisions(new Map()); + const responder = new ApprovalResponder( + allowPlan, + decisions, + undefined, + bridge, + ); + const verdict = await responder.onPermission({ + id: "p", + availableReplies: ["once", "reject"], + gate: relayGate, + }); + assert.equal(verdict.kind, "allow"); + // A policy allow never consumed a stored decision, so none is re-appended (the relay's own + // policy-allow decide also passes without needing one). + assert.equal(decide(relayGate, allowPlan, decisions).kind, "allow"); + }); + + it("does NOT append when the gate pauses (no stored decision consumed)", async () => { + const decisions = new ConversationDecisions(new Map()); + const responder = new ApprovalResponder( + askPlan, + decisions, + undefined, + bridge, + ); + const verdict = await responder.onPermission({ + id: "p", + availableReplies: ["once", "reject"], + gate: relayGate, + }); + assert.equal(verdict.kind, "pendingApproval"); + assert.equal(decide(relayGate, askPlan, decisions).kind, "pendingApproval"); + }); + + it("warm resume: the folded {approved} envelope seeds the relay's execution check", () => { + // On a warm approval resume the FE folds the gated tool_call + the {approved} decision into the + // request. The resume turn builds ConversationDecisions from it; the relay's decide (the second + // gate after the dialog resolves via respondPermission) must find an allow for the SAME key. + const resumeRequest: AgentRunRequest = { + harness: "pi", + messages: [ + { role: "user", content: "do it" }, + { + role: "assistant", + content: [ + { + type: "tool_call", + toolCallId: "call_REAL", + toolName: "park_probe", + input: { token: "T" }, + }, + ], + }, + { + role: "user", + content: [ + { + type: "tool_result", + toolCallId: "call_REAL", + output: { approved: true }, + }, + ], + }, + ], + }; + const decisions = new ConversationDecisions( + extractApprovalDecisions(resumeRequest), + ); + // The relay gate the in-sandbox execution raises: same name + exact params (key parity). + const relayGate: GateDescriptor = { + executor: "relay", + toolName: "park_probe", + specPermission: "ask", + args: { token: "T" }, + }; + assert.equal(decide(relayGate, askPlan, decisions).kind, "allow"); + }); +}); diff --git a/services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts b/services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts index d3977cba77..4a6f923a3f 100644 --- a/services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts +++ b/services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts @@ -8,9 +8,21 @@ import assert from "node:assert/strict"; import type { AgentEvent } from "../../src/protocol.ts"; import type { ClientToolVerdict, Responder } from "../../src/responder.ts"; -import type { Verdict } from "../../src/permission-plan.ts"; +import { + ApprovalResponder, + ConversationDecisions, +} from "../../src/responder.ts"; +import type { PermissionPlan, Verdict } from "../../src/permission-plan.ts"; import { PendingApprovalLatch } from "../../src/permission-plan.ts"; -import { attachPermissionResponder } from "../../src/engines/sandbox_agent/acp-interactions.ts"; +import { + attachPermissionResponder, + type PiToolSpecMeta, +} from "../../src/engines/sandbox_agent/acp-interactions.ts"; +import { + buildPiGateEnvelope, + PI_GATE_DIALOG_TITLE, + type PiGateKind, +} from "../../src/engines/sandbox_agent/pi-gate-envelope.ts"; function flushPromises(): Promise { return new Promise((resolve) => setImmediate(resolve)); @@ -451,3 +463,349 @@ describe("attachPermissionResponder", () => { assert.equal(seen.permission?.[0].gate.serverPermission, "deny"); }); }); + +// -------------------------------------------------------------------------- // +// Pi approval parking: a gate rides ctx.ui.confirm and arrives as an ACP // +// permission request carrying the JSON envelope through rawInput.message. // +// -------------------------------------------------------------------------- // + +/** An ACP permission request the pi-acp bridge synthesizes from a `ctx.ui.confirm` dialog: a + * synthetic `pi-ui-` tool-call id and the real gate identity tunneled through the message. */ +function piGateRequest(opts: { + gate: PiGateKind; + toolName: string; + toolCallId: string; + input: unknown; + message?: string; + title?: string; +}) { + const message = + opts.message ?? + buildPiGateEnvelope({ + gate: opts.gate, + toolName: opts.toolName, + toolCallId: opts.toolCallId, + input: opts.input, + }); + return { + id: "perm-pi", + availableReplies: ["once", "reject"], + toolCall: { + toolCallId: "pi-ui-synthetic-uuid", + kind: "other", + status: "pending", + title: opts.title ?? PI_GATE_DIALOG_TITLE, + rawInput: { method: "confirm", title: PI_GATE_DIALOG_TITLE, message }, + }, + }; +} + +function permissionPlan( + defaultMode: PermissionPlan["default"], +): PermissionPlan { + return { default: defaultMode, rules: [] }; +} + +describe("attachPermissionResponder: Pi dialog gate", () => { + it("normalizes the synthetic id to the envelope's REAL id everywhere it is read", async () => { + const { session, emit } = makeSession(); + const events: AgentEvent[] = []; + const pausedToolCalls: string[] = []; + const gates: any[] = []; + + attachPermissionResponder({ + session, + run: { emitEvent: (event) => events.push(event) }, + responder: fakeResponder({ kind: "pendingApproval" }), + latch: new PendingApprovalLatch(), + dialogGateEnabled: true, + onPausedToolCall: (id) => pausedToolCalls.push(id), + onUserApprovalGate: (info) => gates.push(info), + }); + emit( + piGateRequest({ + gate: "pi-custom-tool", + toolName: "park_probe", + toolCallId: "call_REAL_123", + input: { token: "T" }, + }), + ); + await flushPromises(); + + // pause bookkeeping, the park record, and the emitted card ALL key on the real id. + assert.deepEqual(pausedToolCalls, ["call_REAL_123"]); + assert.equal(gates[0].toolCallId, "call_REAL_123"); + assert.equal(gates[0].gateType, "pi-dialog-permission"); + const payload = (events[0] as any).payload; + assert.equal(payload.toolCallId, "call_REAL_123"); + assert.equal(payload.toolCall.toolCallId, "call_REAL_123"); + // the card shows the REAL args (resolvedName + rawInput), not the envelope JSON. + assert.equal(payload.toolCall.resolvedName, "park_probe"); + assert.deepEqual(payload.toolCall.rawInput, { token: "T" }); + }); + + it("a malformed envelope under the matching title rejects (fail closed), no pause", async () => { + const replies: Array<{ id: string; reply: string }> = []; + const { session, emit } = makeSession(async (id, reply) => { + replies.push({ id, reply }); + }); + const events: AgentEvent[] = []; + const created: unknown[] = []; + let pauses = 0; + + attachPermissionResponder({ + session, + run: { emitEvent: (event) => events.push(event) }, + // A default-allow responder: if the malformed request fell through it would ALLOW. + responder: fakeResponder({ kind: "allow" }), + latch: new PendingApprovalLatch(), + dialogGateEnabled: true, + onPause: () => { + pauses += 1; + }, + onCreateInteraction: (token) => created.push(token), + }); + emit( + piGateRequest({ + gate: "pi-custom-tool", + toolName: "x", + toolCallId: "c", + input: {}, + message: "{ not valid json", + }), + ); + await flushPromises(); + + assert.deepEqual(replies, [{ id: "perm-pi", reply: "reject" }]); + assert.equal(pauses, 0, "a malformed gate never pauses"); + assert.deepEqual(events, [], "no interaction_request emitted"); + assert.deepEqual( + created, + [], + "no durable interaction created for a rejected request", + ); + }); + + it("a non-matching dialog title is untouched (takes today's spec-less path)", async () => { + const { session, emit } = makeSession(); + const seen: { permission?: any[] } = {}; + + attachPermissionResponder({ + session, + run: { emitEvent: () => {} }, + responder: fakeResponder({ kind: "pendingApproval" }, undefined, seen), + latch: new PendingApprovalLatch(), + dialogGateEnabled: true, + }); + emit( + piGateRequest({ + gate: "pi-custom-tool", + toolName: "park_probe", + toolCallId: "call_x", + input: {}, + title: "not-agenta-approval", + }), + ); + await flushPromises(); + + // Classified by the title (today's path), NOT by the envelope. + assert.equal(seen.permission?.[0].gate.toolName, "not-agenta-approval"); + }); + + it("recovers permission metadata so author-allow is instant-allow and author-deny instant-deny", async () => { + const replies: Array<{ id: string; reply: string }> = []; + const { session, emit } = makeSession(async (id, reply) => { + replies.push({ id, reply }); + }); + const piToolSpecsByName = new Map([ + ["author_allow", { permission: "allow" }], + ["author_deny", { permission: "deny" }], + ]); + const responder = new ApprovalResponder( + permissionPlan("ask"), + new ConversationDecisions(new Map()), + ); + + attachPermissionResponder({ + session, + run: { emitEvent: () => {} }, + responder, + latch: new PendingApprovalLatch(), + dialogGateEnabled: true, + piToolSpecsByName, + }); + emit( + piGateRequest({ + gate: "pi-custom-tool", + toolName: "author_allow", + toolCallId: "c1", + input: {}, + }), + ); + await flushPromises(); + assert.deepEqual(replies, [{ id: "perm-pi", reply: "once" }]); + + emit( + piGateRequest({ + gate: "pi-custom-tool", + toolName: "author_deny", + toolCallId: "c2", + input: {}, + }), + ); + await flushPromises(); + assert.deepEqual(replies[1], { id: "perm-pi", reply: "reject" }); + }); + + it("a read-only builtin auto-allows under allow_reads (no pause)", async () => { + const replies: Array<{ id: string; reply: string }> = []; + const { session, emit } = makeSession(async (id, reply) => { + replies.push({ id, reply }); + }); + let pauses = 0; + const responder = new ApprovalResponder( + permissionPlan("allow_reads"), + new ConversationDecisions(new Map()), + ); + + attachPermissionResponder({ + session, + run: { emitEvent: () => {} }, + responder, + latch: new PendingApprovalLatch(), + dialogGateEnabled: true, + onPause: () => { + pauses += 1; + }, + }); + emit( + piGateRequest({ + gate: "pi-builtin", + toolName: "read", + toolCallId: "c", + input: { path: "a" }, + }), + ); + await flushPromises(); + + assert.deepEqual(replies, [{ id: "perm-pi", reply: "once" }]); + assert.equal( + pauses, + 0, + "a read-only builtin never pauses under allow_reads", + ); + }); + + it("a write builtin under allow_reads pauses for a human", async () => { + const { session, emit } = makeSession(); + let pauses = 0; + const gates: any[] = []; + const responder = new ApprovalResponder( + permissionPlan("allow_reads"), + new ConversationDecisions(new Map()), + ); + + attachPermissionResponder({ + session, + run: { emitEvent: () => {} }, + responder, + latch: new PendingApprovalLatch(), + dialogGateEnabled: true, + onPause: () => { + pauses += 1; + }, + onUserApprovalGate: (info) => gates.push(info), + }); + emit( + piGateRequest({ + gate: "pi-builtin", + toolName: "bash", + toolCallId: "c", + input: { command: "rm -rf /" }, + }), + ); + await flushPromises(); + + assert.equal(pauses, 1); + assert.equal(gates[0].gateType, "pi-dialog-permission"); + assert.equal(gates[0].toolName, "Bash"); + }); + + it("an unknown builtin name in the envelope rejects (fail closed, relay parity)", async () => { + const replies: Array<{ id: string; reply: string }> = []; + const { session, emit } = makeSession(async (id, reply) => { + replies.push({ id, reply }); + }); + const events: AgentEvent[] = []; + let pauses = 0; + // A default-allow responder: if the fabricated name fell through it would ALLOW. + const responder = fakeResponder({ kind: "allow" }); + + attachPermissionResponder({ + session, + run: { emitEvent: (event) => events.push(event) }, + responder, + latch: new PendingApprovalLatch(), + dialogGateEnabled: true, + onPause: () => { + pauses += 1; + }, + }); + emit( + piGateRequest({ + gate: "pi-builtin", + toolName: "fabricated_tool", + toolCallId: "c", + input: {}, + }), + ); + await flushPromises(); + + assert.deepEqual(replies, [{ id: "perm-pi", reply: "reject" }]); + assert.equal(pauses, 0, "an unknown builtin never pauses"); + assert.deepEqual(events, [], "no approval card for a fabricated name"); + }); + + it("with detection OFF, a Claude gate whose title collides with the dialog title takes today's path", async () => { + // The MF-1 regression: attachPermissionResponder is shared by Claude and Pi. With the + // dialog gate off, a Claude gate titled literally "agenta-approval" (editing a file with + // that name, a bash command equal to it) has no envelope and must pause/resolve exactly as + // on the base path, never auto-reject. + const replies: Array<{ id: string; reply: string }> = []; + const { session, emit } = makeSession(async (id, reply) => { + replies.push({ id, reply }); + }); + const events: AgentEvent[] = []; + const gates: any[] = []; + let pauses = 0; + + attachPermissionResponder({ + session, + run: { emitEvent: (event) => events.push(event) }, + responder: fakeResponder({ kind: "pendingApproval" }), + latch: new PendingApprovalLatch(), + // dialogGateEnabled intentionally absent (flag off / Claude run). + onPause: () => { + pauses += 1; + }, + onUserApprovalGate: (info) => gates.push(info), + }); + emit({ + id: "perm-claude", + availableReplies: ["once", "reject"], + toolCall: { + toolCallId: "tc-claude", + title: PI_GATE_DIALOG_TITLE, + kind: "execute", + rawInput: { command: "cat agenta-approval" }, + }, + }); + await flushPromises(); + + assert.deepEqual(replies, [], "never auto-rejected"); + assert.equal(pauses, 1, "paused exactly as the base path does"); + assert.equal(gates[0].gateType, "claude-acp-permission"); + assert.equal(events.length, 1, "the approval card was emitted"); + assert.equal((events[0] as any).payload.toolCallId, "tc-claude"); + }); +}); diff --git a/services/runner/tests/unit/sandbox-agent-pi-assets.test.ts b/services/runner/tests/unit/sandbox-agent-pi-assets.test.ts index f727d7097e..19e1cea418 100644 --- a/services/runner/tests/unit/sandbox-agent-pi-assets.test.ts +++ b/services/runner/tests/unit/sandbox-agent-pi-assets.test.ts @@ -46,7 +46,8 @@ describe("buildPiExtensionEnv", () => { const request = { context: { propagation: { - traceparent: "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", + traceparent: + "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", }, }, telemetry: { @@ -67,7 +68,9 @@ describe("buildPiExtensionEnv", () => { properties: { x: { type: "string" } }, }, callRef: "server-secret-ref", - contextBindings: { "target.workflow_variant_id": "$ctx.workflow.variant.id" }, + contextBindings: { + "target.workflow_variant_id": "$ctx.workflow.variant.id", + }, timeoutMs: 120000, env: { SECRET: "do-not-expose" }, kind: "callback", @@ -161,6 +164,28 @@ describe("buildPiExtensionEnv", () => { assert.equal(env.AGENTA_AGENT_TOOLS_PUBLIC_SPECS, undefined); }); + it("sets the Pi dialog gate flag only when active (default off = byte-identical)", () => { + // Flag off (undefined): no dialog env, so the extension keeps the relay path. + assert.equal( + buildPiExtensionEnv({} as AgentRunRequest, false, {}) + .AGENTA_AGENT_PI_DIALOG_GATE, + undefined, + ); + assert.equal( + buildPiExtensionEnv({} as AgentRunRequest, false, { + dialogGateActive: false, + }).AGENTA_AGENT_PI_DIALOG_GATE, + undefined, + ); + // Flag on: exports the sandbox-side switch. + assert.equal( + buildPiExtensionEnv({} as AgentRunRequest, false, { + dialogGateActive: true, + }).AGENTA_AGENT_PI_DIALOG_GATE, + "1", + ); + }); + it("accepts snake_case tool schemas from older Python wire payloads", () => { const env = buildPiExtensionEnv( { @@ -192,7 +217,8 @@ describe("buildPiExtensionEnv", () => { const request = { context: { propagation: { - traceparent: "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", + traceparent: + "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", }, }, telemetry: { capture: { content: { enabled: true } } }, @@ -212,14 +238,16 @@ describe("buildPiExtensionEnv", () => { const request = { context: { propagation: { - traceparent: "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", + traceparent: + "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", }, }, telemetry: { capture: { content: { enabled: true } } }, } as AgentRunRequest; assert.equal( - buildPiExtensionEnv(request, true, { skills: [] }).AGENTA_AGENT_SKILLS_LOADED, + buildPiExtensionEnv(request, true, { skills: [] }) + .AGENTA_AGENT_SKILLS_LOADED, undefined, ); assert.equal( diff --git a/services/runner/tests/unit/session-keepalive-approval.test.ts b/services/runner/tests/unit/session-keepalive-approval.test.ts index fc5b1ded24..a8df3056f9 100644 --- a/services/runner/tests/unit/session-keepalive-approval.test.ts +++ b/services/runner/tests/unit/session-keepalive-approval.test.ts @@ -52,13 +52,15 @@ const auth = { // ---------------------------------------------------------------------------- // interface TurnScript { - /** The turn pauses on a single Claude ACP permission gate (records parkedApproval). */ + /** The turn pauses on a single parkable permission gate (records parkedApproval). */ approvalPause?: { permissionId: string; toolCallId: string; toolName?: string; /** How many gates pended (>1 = multi-gate; still records the first). Default 1. */ gates?: number; + /** The parked gate plane; default the Claude ACP gate. */ + gateType?: ParkedApproval["gateType"]; }; /** The turn pauses on a non-Claude gate (Pi relay / client tool): paused, no parkedApproval. */ nonClaudePause?: boolean; @@ -166,7 +168,7 @@ function makeApprovalEngine( }); promptPromise.catch(() => {}); env.parkedApproval = { - gateType: "claude-acp-permission", + gateType: script.approvalPause.gateType ?? "claude-acp-permission", permissionId: script.approvalPause.permissionId, toolCallId: script.approvalPause.toolCallId, toolName: script.approvalPause.toolName, @@ -337,6 +339,40 @@ describe("runWithKeepalive: approval park + resume", () => { ); }); + it("parks and resumes a Pi DIALOG gate exactly like the Claude gate (server guard accepts it)", async () => { + const { engine, calls } = makeApprovalEngine([ + { + approvalPause: { + permissionId: "perm-1", + toolCallId: "tc-gate", + toolName: "commit", + gateType: "pi-dialog-permission", + }, + toolCallIds: ["tc-gate"], + }, + ]); + const ctx = makeCtx(engine); + + const r1 = await runWithKeepalive(pauseTurn(), undefined, undefined, ctx); + assert.equal(r1.stopReason, "paused"); + assert.equal( + ctx.pool.get(POOL_KEY)!.state, + "awaiting_approval", + "the Pi dialog gate parked (not rejected as an unrecognized gate type)", + ); + + const r2 = await runWithKeepalive( + approveResume(true), + undefined, + undefined, + ctx, + ); + assert.equal(r2.ok, true); + assert.equal(calls.acquire, 1, "the resume did NOT re-acquire cold"); + assert.equal(calls.resumes.length, 1, "the Pi gate is answered live once"); + assert.equal(calls.resumes[0].reply, "once"); + }); + it("answers a denied gate live with reject on the resume", async () => { const { engine, calls } = makeApprovalEngine([ { @@ -390,7 +426,7 @@ describe("runWithKeepalive: approval park + resume", () => { }); describe("runWithKeepalive: never-park gate types stay cold", () => { - it("a non-Claude gate pause (Pi relay / client-tool MCP) never parks, tears down cold", async () => { + it("a non-parkable gate pause (Pi file relay / client-tool MCP) never parks, tears down cold", async () => { const cap = captureStderr(); try { const { engine, calls } = makeApprovalEngine([{ nonClaudePause: true }]); @@ -403,7 +439,7 @@ describe("runWithKeepalive: never-park gate types stay cold", () => { "no parked approval -> torn down as today", ); assert.equal(ctx.pool.size(), 0, "nothing parked"); - assert.ok(cap.lines.some((l) => l.includes("non-claude-gate-no-park"))); + assert.ok(cap.lines.some((l) => l.includes("non-parkable-gate-no-park"))); } finally { cap.restore(); } @@ -1327,4 +1363,95 @@ describe("runTurn: real approval park + respondPermission resume", () => { assert.equal(calls.sessionDestroyed, 1, "destroy tore the session down"); assert.equal(calls.sandboxDestroyed, 1); }); + + it("a Pi warm resume seeds the relay's execution check from the folded approval", async () => { + // The Pi custom-tool double gate on the WARM path: the resume answers the held dialog via + // respondPermission (never through the responder, so nothing re-appends), and the relay's + // own execution check must still pass. The resume request folds the gated tool_call plus + // the {approved: true} envelope; the resume turn's real ConversationDecisions (built by + // runTurn from that history) is what the relay consults. Capture the REAL relayPermissions + // runTurn wires into startToolRelay and assert its decide() finds the allow. + const { calls, deps } = pausableHarness(); + let relayPermissions: + { enforce: boolean; decide: (gate: any) => { kind: string } } | undefined; + deps.startToolRelay = (( + _host: unknown, + _dir: unknown, + _specs: unknown, + _callback: unknown, + permissions: any, + ) => { + relayPermissions = permissions; + return { stop: async () => {} }; + }) as any; + + const resumeRequest: AgentRunRequest = { + harness: "pi_core", + customTools: [{ name: "park_probe", permission: "ask" }] as any, + toolCallback: { + endpoint: "http://callback", + authorization: "bearer", + } as any, + messages: [ + { role: "user", content: "do X" }, + { + role: "assistant", + content: [ + { + type: "tool_call", + toolCallId: "call_REAL", + toolName: "park_probe", + input: { token: "T" }, + }, + ], + }, + { + role: "user", + content: [ + { + type: "tool_result", + toolCallId: "call_REAL", + output: { approved: true }, + }, + ], + }, + ], + }; + const acquired = await acquireEnvironment(resumeRequest, deps); + assert.equal(acquired.ok, true); + if (!acquired.ok) return; + const env = acquired.env; + + // Drive the resume branch: answer the (already-parked) gate and continue the held prompt. + const result = await runTurn(env, resumeRequest, undefined, undefined, { + approvalParkMode: true, + resume: { + permissionId: "perm-1", + reply: "once", + toolCallId: "call_REAL", + toolName: "park_probe", + args: { token: "T" }, + interactionToken: "call_REAL", + promptPromise: Promise.resolve({ stopReason: "complete" }), + }, + }); + assert.equal(result.ok, true); + assert.deepEqual( + calls.permissionReplies, + [{ id: "perm-1", reply: "once" }], + "the held dialog was answered on the live session", + ); + assert.ok(relayPermissions, "the resume turn restarted the relay"); + assert.equal(relayPermissions!.enforce, true, "Pi: the relay enforces"); + // The relay's second gate on the resumed call finds the folded approval. + const verdict = relayPermissions!.decide({ + executor: "relay", + toolName: "park_probe", + specPermission: "ask", + args: { token: "T" }, + }); + assert.equal(verdict.kind, "allow"); + + await env.destroy(); + }); }); From 6a940d5d9acdcc70e853b4e84fb83da18e21b000 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Fri, 10 Jul 2026 01:41:06 +0200 Subject: [PATCH 2/3] feat(runner): Pi approval parking is the only path; drop the flag and the relay permission plane The dialog gate is now the unconditional Pi behavior. AGENTA_RUNNER_PI_DIALOG_GATE and AGENTA_AGENT_PI_APPROVAL... flags are gone; envelope detection stays scoped to Pi runs structurally (the responder receives the resolved-specs map only on Pi runs, and its presence turns detection on), so a Claude gate whose title collides with the dialog title still takes the base path. Dead code removed with it: - extension relayPermissionCheck poll and its dispatch.ts plumbing - relay.ts permission plane: handlePermissionRelayRequest, the kind:permission record protocol, RelayPermissions and the watcher's decide enforcement - the double-gate FIFO bridge in ConversationDecisions (with the relay re-check gone, the dialog is the single enforcement point) - builtin-only runs start no relay (useToolRelay = custom tools only) Also: gateType renamed to pi-acp-permission (symmetric with claude-acp-permission); CodeRabbit fixes (a custom-tool envelope with no resolved spec fails closed like an unknown builtin; test title-sync nit); args validated before the dialog so a malformed call errors to the model instead of prompting a human. Claude-Session: https://claude.ai/code/session_01CSTSEXSe4DDhoXCFjZpZ5W --- services/runner/src/engines/sandbox_agent.ts | 86 +--- .../engines/sandbox_agent/acp-interactions.ts | 70 +-- .../src/engines/sandbox_agent/pi-assets.ts | 9 +- .../src/engines/sandbox_agent/run-plan.ts | 4 +- services/runner/src/extensions/agenta.ts | 73 +--- services/runner/src/responder.ts | 51 --- services/runner/src/server.ts | 10 +- services/runner/src/tools/dispatch.ts | 106 ----- services/runner/src/tools/relay.ts | 266 +----------- .../tests/unit/builtin-grant-list.test.ts | 62 +++ .../runner/tests/unit/extension-tools.test.ts | 30 -- .../unit/permission-record-fixture.test.ts | 150 ------- .../tests/unit/pi-gate-envelope.test.ts | 28 +- services/runner/tests/unit/responder.test.ts | 220 ---------- .../sandbox-agent-acp-interactions.test.ts | 78 +++- .../unit/sandbox-agent-orchestration.test.ts | 66 +-- .../unit/sandbox-agent-pi-assets.test.ts | 26 +- .../tests/unit/sandbox-agent-run-plan.test.ts | 43 +- .../unit/session-keepalive-approval.test.ts | 93 +--- .../tests/unit/tool-callref-bindings.test.ts | 95 +---- .../runner/tests/unit/tool-direct.test.ts | 5 - .../unit/tool-dispatch-permission.test.ts | 144 ------- .../unit/tool-relay-permission-parity.test.ts | 192 --------- .../unit/tool-relay-permission-record.test.ts | 400 ------------------ .../tests/unit/tool-relay-permission.test.ts | 318 -------------- 25 files changed, 344 insertions(+), 2281 deletions(-) create mode 100644 services/runner/tests/unit/builtin-grant-list.test.ts delete mode 100644 services/runner/tests/unit/permission-record-fixture.test.ts delete mode 100644 services/runner/tests/unit/tool-dispatch-permission.test.ts delete mode 100644 services/runner/tests/unit/tool-relay-permission-parity.test.ts delete mode 100644 services/runner/tests/unit/tool-relay-permission-record.test.ts delete mode 100644 services/runner/tests/unit/tool-relay-permission.test.ts diff --git a/services/runner/src/engines/sandbox_agent.ts b/services/runner/src/engines/sandbox_agent.ts index 32332b8415..ac9d0db8af 100644 --- a/services/runner/src/engines/sandbox_agent.ts +++ b/services/runner/src/engines/sandbox_agent.ts @@ -44,7 +44,6 @@ import { localRelayHost, sandboxRelayHost, startToolRelay, - type RelayPermissions, } from "../tools/relay.ts"; import { ApprovalResponder, @@ -90,7 +89,6 @@ import { writeOtlpAuthFile, } from "./sandbox_agent/pi-assets.ts"; import { - decide, PendingApprovalLatch, permissionsFromRequest, } from "../permission-plan.ts"; @@ -139,14 +137,6 @@ function log(message: string): void { process.stderr.write(`[sandbox-agent] ${message}\n`); } -/** Pi approval parking flag (runner side, default OFF). Only a few explicit truthy spellings. */ -function piDialogGateEnabled(): boolean { - const raw = (process.env.AGENTA_RUNNER_PI_DIALOG_GATE ?? "") - .trim() - .toLowerCase(); - return raw === "1" || raw === "true" || raw === "yes" || raw === "on"; -} - /** Extract the run credential from the OTLP export headers (initial value, constant for the run). */ function runCredential(request: AgentRunRequest): string { const headers = (request.telemetry?.exporters?.otlp?.headers ?? {}) as Record< @@ -371,11 +361,11 @@ interface CurrentTurn { /** * A permission gate that paused the turn and can be answered later on the SAME live session. - * Recorded for a Claude ACP permission gate (keep-alive slice 2) or a Pi dialog permission gate - * (Pi approval parking, which rides `ctx.ui.confirm` onto the same ACP permission plane). NOT - * recorded for a Pi file-relay gate or a client-tool MCP pause — those cannot be answered across - * a turn boundary and stay on the cold path. Existence of this record is what makes the dispatch - * park a paused session in `awaiting_approval` instead of tearing it down. + * Recorded for a Claude ACP permission gate (keep-alive slice 2) or a Pi ACP permission gate + * (Pi approval parking: the gate rides the extension's `ctx.ui.confirm` onto the same ACP + * permission plane). NOT recorded for a client-tool MCP pause — that cannot be answered across + * a turn boundary and stays on the cold path. Existence of this record is what makes the + * dispatch park a paused session in `awaiting_approval` instead of tearing it down. */ export interface ParkedApproval { /** Which gate paused; the dispatch resumes only a recognized type and treats others as cold. */ @@ -603,9 +593,6 @@ export async function acquireEnvironment( otlpAuthFilePath, builtinGatingActive: plan.builtinGatingActive, builtinGrants: plan.builtinGrants, - // Pi approval parking: route both Pi gates over the parkable dialog plane. Runner-side - // flag, default off; flag-off keeps the byte-identical relay path. - dialogGateActive: piDialogGateEnabled(), // The materialized skill names (author + forced `_agenta.*`) so Pi's own agent span // records which skills loaded (F-029); local Pi self-instruments, so the runner's // sandbox-agent otel has no span to stamp here. @@ -1154,13 +1141,13 @@ export async function runTurn( (id) => pause.isPausedToolCall(id), TOOL_NOT_EXECUTED_PAUSED, ); - // Slice 2 park mode: a parkable Claude ACP permission gate recorded `env.parkedApproval` - // BEFORE firing this pause (the onUserApprovalGate hook runs before the single-pause latch). - // Keep the live session — the gated tool runs on the resume — so skip ONLY the mcpAbort and - // the destroySession. The teardown is not lost: the dispatch either parks the session or, - // if it decides not to (multi-gate, pool full), calls `env.destroy()` which runs them. A - // non-parkable pause (flag off, Pi relay/builtin gate, client tool) never records - // `parkedApproval`, so it still tears down here exactly as today. + // Park mode: a parkable permission gate (Claude ACP or Pi ACP) recorded + // `env.parkedApproval` BEFORE firing this pause (the onUserApprovalGate hook runs before + // the single-pause latch). Keep the live session — the gated tool runs on the resume — so + // skip ONLY the mcpAbort and the destroySession. The teardown is not lost: the dispatch + // either parks the session or, if it decides not to (multi-gate, pool full), calls + // `env.destroy()` which runs them. A non-parkable pause (keep-alive off, client tool) + // never records `parkedApproval`, so it still tears down here exactly as today. if (opts.approvalParkMode && env.parkedApproval) return; // 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. @@ -1241,12 +1228,7 @@ export async function runTurn( const latch = new PendingApprovalLatch(); const responder = deps.responderFactory?.(request) ?? - new ApprovalResponder(permissionPlan, decisions, logger, { - // The Pi double-gate bridge: only where the dialog gate is live AND the relay enforces - // (Pi). On any other run the relay never consumes a re-appended decision, so appending - // would leak it to a later identical call. - bridgeRelayDoubleGate: plan.isPi && piDialogGateEnabled(), - }); + new ApprovalResponder(permissionPlan, decisions, logger); // Every pause seeds the durable interactions plane, whichever gate paused. const recordPendingInteraction = ( token: string, @@ -1282,36 +1264,6 @@ export async function runTurn( return; void resolveInteraction(sessionId, token, () => cred); }; - // The harness gate decides on Claude; the relay decides on Pi. Under the Pi dialog gate a - // custom tool is checked twice (dialog + relay execution check); the responder's - // bridgeRelayDoubleGate accounting keeps the two consuming one human decision. - const relayPermissions: RelayPermissions = { - enforce: plan.isPi, - decide: (gate) => decide(gate, permissionPlan, decisions), - onPendingApproval: ({ toolCallId, toolName, args }) => { - if (!latch.tryAcquire()) return { emitted: false }; - pause.markPausedToolCall(toolCallId); - run.emitEvent({ - type: "interaction_request", - id: toolCallId, - kind: "user_approval", - payload: { - toolCallId, - toolCall: { - toolCallId, - name: toolName, - title: toolName, - rawInput: args, - input: args, - }, - availableReplies: ["once", "reject"], - }, - }); - recordPendingInteraction(toolCallId, toolName, args); - pause.pause(); - return { emitted: true }; - }, - }; const serverPermissions = serverPermissionsFromRequest(request); // Build the per-turn permission handler WITHOUT attaching to the live session: the // session-lifetime `onPermissionRequest` (in acquireEnvironment) routes into it via @@ -1334,12 +1286,9 @@ export async function runTurn( onPausedToolCall: (id) => pause.markPausedToolCall(id), onCreateInteraction: recordPendingInteraction, onResolveInteraction: resolveInteractionToken, - // Envelope detection is scoped to a Pi run with the dialog gate live. Flag off (or any - // Claude run) never parses: a Claude gate whose title collides with the dialog title must - // take today's path, not the fail-closed reject. - dialogGateEnabled: plan.isPi && piDialogGateEnabled(), - // Recover permission metadata for a Pi dialog gate: the envelope names the tool, the runner - // fills specPermission/readOnlyHint from the run's own resolved specs (relay parity). Pi only. + // Pi runs only: presence of the specs map turns Pi gate envelope detection on AND is how + // the runner recovers specPermission/readOnlyHint (the envelope carries identity, never + // policy). Absent for Claude, so a title collision there keeps the base path. piToolSpecsByName: plan.isPi ? new Map( plan.toolSpecs.map((spec) => [ @@ -1351,7 +1300,7 @@ export async function runTurn( // Record the parkable permission gate (only in keep-alive park mode) so the dispatch can // resume it live. Fires per pending gate (before the latch) so a parallel gate is counted; // the single-gate resume records only the FIRST gate's answer target. `info.gateType` names - // the plane (Claude ACP vs Pi dialog) so the resume answers on the right one. + // the plane (Claude ACP vs Pi ACP) so the resume answers on the right one. onUserApprovalGate: opts.approvalParkMode ? (info) => { env.approvalGateCount += 1; @@ -1393,7 +1342,6 @@ export async function runTurn( plan.relayDir, plan.toolSpecs, request.toolCallback as ToolCallbackContext | undefined, - relayPermissions, request.runContext, env.clientToolRelayRef.current, ); diff --git a/services/runner/src/engines/sandbox_agent/acp-interactions.ts b/services/runner/src/engines/sandbox_agent/acp-interactions.ts index 25cfb2d44c..437e81d2d0 100644 --- a/services/runner/src/engines/sandbox_agent/acp-interactions.ts +++ b/services/runner/src/engines/sandbox_agent/acp-interactions.ts @@ -15,11 +15,11 @@ import { type PiGateEnvelope, } from "./pi-gate-envelope.ts"; -/** The parkable gate types a paused turn can record (widened for the Pi dialog gate). */ +/** The parkable gate types a paused turn can record (the Claude ACP and Pi ACP gates). */ export type ParkedApprovalGateType = - "claude-acp-permission" | "pi-dialog-permission"; + "claude-acp-permission" | "pi-acp-permission"; -/** The permission metadata the runner recovers per tool for a Pi dialog gate (identity-only +/** The permission metadata the runner recovers per tool for a Pi gate (the identity-only * envelope carries no policy). Keyed by resolved tool name. */ export interface PiToolSpecMeta { permission?: ToolPermission; @@ -50,7 +50,7 @@ export interface AttachPermissionResponderInput { /** Called after a stored decision was successfully forwarded to the harness. */ onResolveInteraction?: (token: string) => void; /** - * Fires for EVERY parkable permission gate (a Claude ACP gate or a Pi dialog gate) that + * Fires for EVERY parkable permission gate (a Claude ACP gate or a Pi ACP gate) that * resolves to pendingApproval, BEFORE the single-pause latch. Keep-alive uses it to record * the parked permission id / tool-call id (for a live resume via `respondPermission`) and to * count how many gates are pending this turn (a multi-gate pause does not park). It never @@ -66,16 +66,13 @@ export interface AttachPermissionResponderInput { gateType: ParkedApprovalGateType; }) => void; /** - * Detect Pi gate envelopes on incoming permission requests. ON only for a Pi run with the - * dialog gate active. It must stay OFF everywhere else: the pre-filter is the dialog TITLE, + * Resolved tool specs by name for the Pi gates. PRESENCE marks a Pi run and turns Pi gate + * envelope detection on; it must stay absent for Claude. The pre-filter is the dialog TITLE, * and a Claude gate whose ACP title happens to be the literal dialog title (editing a file * named after it, a bash command equal to it) has no envelope and would be auto-rejected - * where today's path pauses or resolves it normally. - */ - dialogGateEnabled?: boolean; - /** - * Resolved tool specs by name, for a Pi dialog gate. The envelope carries identity only, so - * the runner recovers `specPermission`/`readOnlyHint` here (relay parity). Absent for Claude. + * where the base path pauses or resolves it normally. The map itself is how the runner + * recovers `specPermission`/`readOnlyHint` (the envelope carries identity, never policy), so + * detection and metadata recovery are inseparable by construction. */ piToolSpecsByName?: ReadonlyMap; } @@ -93,7 +90,6 @@ export function attachPermissionResponder({ onCreateInteraction, onResolveInteraction, onUserApprovalGate, - dialogGateEnabled, piToolSpecsByName, }: AttachPermissionResponderInput): void { session.onPermissionRequest((req: any) => { @@ -107,7 +103,7 @@ export function attachPermissionResponder({ // gate's stable anchor). The Vercel egress prefers it over the drift-prone title/kind // display fields, so the approval part names the tool exactly as the responder keys it. // This stamping never mutates the inbound ACP object. (The one deliberate inbound mutation - // is the Pi dialog gate's id/args normalization in `handlePiGate`, which must happen in + // is the Pi gate's id/args normalization in `handlePiGate`, which must happen in // place so every downstream read sees the envelope's real identity.) const stampResolvedName = (toolCall: any, gate: GateDescriptor): any => { if (!toolCall || typeof toolCall !== "object" || !gate.toolName) @@ -263,12 +259,13 @@ export function attachPermissionResponder({ toolCall.rawInput = envelope.input; } const gate = buildPiGateDescriptor(envelope, piToolSpecsByName); - // An unrecognized builtin name fails closed (relay parity: the relay denies unknown - // builtins outright). The envelope is sandbox-origin and untrusted; letting the raw name - // through would also put a fabricated tool name on the human's approval card. + // An unrecognized tool name (builtin OR custom) fails closed. The envelope is + // sandbox-origin and untrusted; letting the raw name through would resolve it against the + // run's default permission and put a fabricated tool name on the human's approval card. if (!gate) { log?.( - `[HITL] pi-gate unknown builtin ${JSON.stringify(envelope.toolName)} id=${id}; reject (fail closed)`, + `[HITL] pi-gate unknown ${envelope.gate === "pi-builtin" ? "builtin" : "custom tool"} ` + + `${JSON.stringify(envelope.toolName)} id=${id}; reject (fail closed)`, ); await rejectRequest(id, availableReplies); return; @@ -293,7 +290,7 @@ export function attachPermissionResponder({ raw: req, }); if (verdict.kind === "pendingApproval" || !id) { - pauseUserApproval(req, id, gate, "pi-dialog-permission"); + pauseUserApproval(req, id, gate, "pi-acp-permission"); return; } await replyPermission(id, verdict.kind, availableReplies); @@ -306,12 +303,12 @@ export function attachPermissionResponder({ // A Pi gate rides `ctx.ui.confirm` under the fixed dialog title. Detect it FIRST, before the // spec-less classification below: without this the gate would key as `agenta-approval` with // dialog-string args (wrong identity on cards, the decision map, and policy). Detection runs - // ONLY when the dialog gate is live for this run (`dialogGateEnabled`): the pre-filter is the - // TITLE, so with the flag off a Claude gate whose title collides with the dialog title must - // take today's path, not the fail-closed reject. With detection on, a matching title whose - // envelope does not parse fails closed (reject), never falls through — under a default-allow - // plan a fallthrough would confirm an unapproved execution. - if (dialogGateEnabled) { + // ONLY on a Pi run (`piToolSpecsByName` present): the pre-filter is the TITLE, so a Claude + // gate whose title collides with the dialog title must take the base path, not the + // fail-closed reject. With detection on, a matching title whose envelope does not parse + // fails closed (reject), never falls through — under a default-allow plan a fallthrough + // would confirm an unapproved execution. + if (piToolSpecsByName) { const piGate = parsePiGateEnvelope(req); if (piGate.matched) { if (!piGate.envelope) { @@ -381,17 +378,16 @@ export function attachPermissionResponder({ } /** - * Build the `GateDescriptor` for a Pi dialog gate from the envelope identity plus the runner's - * own resolved specs (the envelope carries identity, never policy). + * Build the `GateDescriptor` for a Pi gate from the envelope identity plus the runner's own + * resolved specs (the envelope carries identity, never policy). * * `pi-builtin` maps to `executor: "harness"` with the builtin's canonical rule name and - * read-only hint (matching `handlePermissionRelayRequest` in relay.ts); an UNKNOWN builtin - * name returns undefined so the caller rejects it (the relay denies unknown builtins outright, - * and the sandbox-origin envelope must not put a fabricated name on the approval card). - * `pi-custom-tool` maps to `executor: "relay"` with the spec's author permission and read-only - * hint recovered by name (matching the relay gate in relay.ts), so an author-allow tool stays - * instant-allow, an author-deny tool stays instant-deny, and a read-only builtin auto-allows — - * relay parity. + * read-only hint from `piBuiltinIdentity`. `pi-custom-tool` maps to `executor: "relay"` with + * the spec's author permission and read-only hint recovered by name, so an author-allow tool + * stays instant-allow, an author-deny tool stays instant-deny, and a read-only builtin + * auto-allows. An UNKNOWN name (a builtin outside the canonical set, or a custom tool with no + * resolved spec) returns undefined so the caller rejects it: the sandbox-origin envelope must + * not resolve a fabricated name against the default permission or put it on the approval card. */ export function buildPiGateDescriptor( envelope: PiGateEnvelope, @@ -407,7 +403,11 @@ export function buildPiGateDescriptor( args: envelope.input, }; } - const spec = piToolSpecsByName?.get(envelope.toolName); + // A custom-tool name with no matching resolved spec fails closed too: the envelope is + // sandbox-origin, and without a spec there is no recovered policy — falling through would + // resolve a fabricated or mismatched name against the run's default permission. + if (!piToolSpecsByName?.has(envelope.toolName)) return undefined; + const spec = piToolSpecsByName.get(envelope.toolName); return { executor: "relay", toolName: envelope.toolName, diff --git a/services/runner/src/engines/sandbox_agent/pi-assets.ts b/services/runner/src/engines/sandbox_agent/pi-assets.ts index bbecff39c8..f1f40da335 100644 --- a/services/runner/src/engines/sandbox_agent/pi-assets.ts +++ b/services/runner/src/engines/sandbox_agent/pi-assets.ts @@ -46,8 +46,6 @@ export function buildPiExtensionEnv( skills?: string[]; builtinGatingActive?: boolean; builtinGrants?: string[]; - /** Route both Pi gates over the extension-UI dialog plane (Pi approval parking). */ - dialogGateActive?: boolean; } = {}, ): Record { const env: Record = {}; @@ -73,15 +71,12 @@ export function buildPiExtensionEnv( env.AGENTA_AGENT_TOOLS_PUBLIC_SPECS = JSON.stringify(specs); env.AGENTA_AGENT_TOOLS_RELAY_DIR = opts.relayDir; } + // Builtin gating needs no relay dir: the gate rides the extension's `ctx.ui.confirm` + // dialog onto the ACP permission plane (Pi approval parking), not the file relay. if (opts.builtinGatingActive) { env.AGENTA_AGENT_BUILTIN_GATING = "1"; env.AGENTA_AGENT_BUILTIN_GRANTS = (opts.builtinGrants ?? []).join(","); - if (opts.relayDir) env.AGENTA_AGENT_TOOLS_RELAY_DIR = opts.relayDir; } - // Pi approval parking: with the flag on both gates ride `ctx.ui.confirm` (a parkable ACP - // permission request) instead of the file relay. One flag drives both sides coherently - // because the runner installs the extension per run. - if (opts.dialogGateActive) env.AGENTA_AGENT_PI_DIALOG_GATE = "1"; if (opts.usageOutPath) env.AGENTA_AGENT_USAGE_CAPTURE_PATH = opts.usageOutPath; return env; diff --git a/services/runner/src/engines/sandbox_agent/run-plan.ts b/services/runner/src/engines/sandbox_agent/run-plan.ts index 352ed3df61..b7ed86b4ad 100644 --- a/services/runner/src/engines/sandbox_agent/run-plan.ts +++ b/services/runner/src/engines/sandbox_agent/run-plan.ts @@ -444,7 +444,9 @@ export function buildRunPlan( executableToolSpecs: executableToolSpecsForRun, builtinGrants, builtinGatingActive, - useToolRelay: toolSpecs.length > 0 || builtinGatingActive, + // The relay carries tool EXECUTION only (permission gates ride the extension's + // `ctx.ui.confirm` dialog onto the ACP plane), so a builtin-gating-only run needs no relay. + useToolRelay: toolSpecs.length > 0, systemPrompt, appendSystemPrompt, hasSystemPrompt: !!(systemPrompt || appendSystemPrompt), diff --git a/services/runner/src/extensions/agenta.ts b/services/runner/src/extensions/agenta.ts index 255ac20213..7446e410ce 100644 --- a/services/runner/src/extensions/agenta.ts +++ b/services/runner/src/extensions/agenta.ts @@ -38,7 +38,11 @@ import { 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"; +import { + assertRequiredArguments, + requiredFields, + specInputSchema, +} from "../tools/spec-schema.ts"; import { buildPiGateEnvelope, PI_GATE_DIALOG_TITLE, @@ -62,7 +66,7 @@ export function readOtlpAuthFile(path?: string): string | undefined { } return value || undefined; } -import { relayPermissionCheck, runResolvedTool } from "../tools/dispatch.ts"; +import { runResolvedTool } from "../tools/dispatch.ts"; function log(message: string): void { process.stderr.write(`[agenta-pi-ext] ${message}\n`); @@ -205,9 +209,7 @@ function blockReason(reason: string | undefined): ToolCallEventResult { function registerBuiltinGating( pi: ExtensionAPI, - relayDir: string | undefined, builtinGrants: readonly PiBuiltinToolName[], - dialogGate: boolean, ): void { pi.on("before_agent_start", async () => { pi.setActiveTools( @@ -224,40 +226,14 @@ function registerBuiltinGating( async (event, ctx): Promise => { const toolName = builtinToolNameFromEvent(event); if (!toolName) return undefined; - - // Dialog plane (flag on): the gate rides `ctx.ui.confirm`, so the runner holds and can park - // it. The relay path stays behind the flag for rollback. - if (dialogGate) { - const { allowed, reason } = await piDialogAllows( - ctx, - "pi-builtin", - toolName, - event.toolCallId, - event.input, - ); - return allowed ? undefined : blockReason(reason); - } - - if (!relayDir) { - return blockReason( - "Permission check denied because the relay directory is missing.", - ); - } - - try { - const response = await relayPermissionCheck( - relayDir, - toolName, - event.toolCallId, - event.input, - ); - if (response.verdict === "allow") return undefined; - return blockReason(response.reason); - } catch (err) { - return blockReason( - err instanceof Error ? err.message : "Permission check failed.", - ); - } + const { allowed, reason } = await piDialogAllows( + ctx, + "pi-builtin", + toolName, + event.toolCallId, + event.input, + ); + return allowed ? undefined : blockReason(reason); }, ); } @@ -301,7 +277,7 @@ function parseSkillsLoaded(raw: string | undefined): string[] { } /** Register public tool metadata as Pi tools whose execution relays to the runner. */ -function registerTools(pi: ExtensionAPI, dialogGate: boolean): void { +function registerTools(pi: ExtensionAPI): void { const raw = process.env.AGENTA_AGENT_TOOLS_PUBLIC_SPECS; const relayDir = process.env.AGENTA_AGENT_TOOLS_RELAY_DIR; if (!raw || !relayDir) return; @@ -318,8 +294,8 @@ function registerTools(pi: ExtensionAPI, dialogGate: boolean): void { for (const spec of specs) { // The dialog gate applies to EXECUTABLE custom tools only. `client` tools are // browser-fulfilled across a turn boundary through the relay's own pause semantics; gating - // one via the dialog would be wrong, so they keep today's path. - const gateViaDialog = dialogGate && (spec.kind ?? "callback") !== "client"; + // one via the dialog would be wrong, so they keep their path. + const gateViaDialog = (spec.kind ?? "callback") !== "client"; pi.registerTool({ name: spec.name, label: spec.name, @@ -335,8 +311,11 @@ function registerTools(pi: ExtensionAPI, dialogGate: boolean): void { _onUpdate?: unknown, ctx?: ExtensionContext, ) { + // Validate BEFORE the gate: a malformed call must error to the model, never reach a + // human as an approval prompt (and never relay as a no-op). + assertRequiredArguments(spec, params); // Gate BEFORE the relay execution: only an allow proceeds. A deny surfaces as the tool's - // result text (mirroring the relay's own deny), so the model loop continues. + // result text, so the model loop continues. if (gateViaDialog) { const { allowed, reason } = await piDialogAllows( ctx, @@ -388,17 +367,11 @@ const factory = (pi: ExtensionAPI): void => { const builtinGrants = normalizeBuiltinGrants( process.env.AGENTA_AGENT_BUILTIN_GRANTS, ); - // Approval parking (Option C): route both Pi gates over the extension-UI dialog plane instead - // of the file relay, so the runner can hold and park an ask. Runner-side flag - // AGENTA_RUNNER_PI_DIALOG_GATE -> sandbox AGENTA_AGENT_PI_DIALOG_GATE (buildPiExtensionEnv). - // Default off: with it off, both gates keep the byte-identical relay path. - const dialogGate = isTruthyFlag(process.env.AGENTA_AGENT_PI_DIALOG_GATE); const usageOut = process.env.AGENTA_AGENT_USAGE_CAPTURE_PATH; if (!hasTracing && !hasTools && !hasBuiltinGating && !usageOut) return; - if (hasTools) registerTools(pi, dialogGate); - if (hasBuiltinGating) - registerBuiltinGating(pi, relayDir, builtinGrants, dialogGate); + if (hasTools) registerTools(pi); + if (hasBuiltinGating) registerBuiltinGating(pi, builtinGrants); // Tracing exports the span tree (when the OTLP target is reachable, i.e. local runs). // Usage accumulation is needed both for that export AND for the writeback the runner // uses on Daytona (where the in-sandbox process can't reach Agenta's OTLP, so the diff --git a/services/runner/src/responder.ts b/services/runner/src/responder.ts index 316f9c07f5..aeae8b720e 100644 --- a/services/runner/src/responder.ts +++ b/services/runner/src/responder.ts @@ -236,27 +236,6 @@ export class ConversationDecisions implements StoredPermissionDecisions { return value; } - /** - * Re-append one decision to the FRONT of this call's queue (the Pi double-gate bridge). - * - * A Pi custom-tool dialog gate and the relay's execution check both `decide()` the SAME - * (name + canonical args) key: the dialog approves the call, then the relay watcher re-checks - * it before executing. When the dialog answered from a STORED decision it consumed one queued - * entry, so the relay's later `take` would find none and pause a second time. Re-appending the - * consumed decision lets the relay consume exactly what the human already granted - * (consume-1-append-1). It goes to the FRONT, not the back, because the relay's `decide` is the - * immediate next `take` on this call (Pi runs tools sequentially): a back-append would hand the - * relay a LATER identical call's decision. A single call empties the queue first, so front and - * back coincide there; front is correct for the 2+ identical-call case too. - */ - appendDecision(gate: GateDescriptor, decision: "allow" | "deny"): void { - const key = approvedCallKey(gate.toolName, gate.args); - if (!key) return; - const queue = this.decisionQueues.get(key); - if (queue) queue.unshift(decision); - else this.decisionQueues.set(key, [decision]); - } - /** The next FIFO client-tool output for this exact call, without consuming it. */ peekClientOutput(gate: GateDescriptor): { found: boolean; output?: unknown } { const entry = this.nextClientOutput(gate); @@ -292,46 +271,16 @@ export class ConversationDecisions implements StoredPermissionDecisions { * in execution shape: `allow` and `ask` both mean "forward to the browser and pause unless a * stored browser output is already available"; `deny` refuses the call. */ -export interface ApprovalResponderOptions { - /** - * The Pi double-gate bridge (dialog-gate runs only). ON only when the run routes Pi gates - * over the dialog plane, where a dialog-allowed custom tool still hits the relay watcher's - * own `decide()` before executing. It must stay OFF everywhere else: on Claude the relay - * never enforces (`enforce: plan.isPi`), so a relay-shaped Claude gate has no second - * consumer and an appended decision would linger and mis-resolve a LATER identical call, - * changing flag-off behavior. - */ - bridgeRelayDoubleGate?: boolean; -} - export class ApprovalResponder implements Responder { constructor( private readonly plan: PermissionPlan, private readonly decisions: ConversationDecisions, private readonly log: (msg: string) => void = () => {}, - private readonly options: ApprovalResponderOptions = {}, ) {} async onPermission(request: PermissionGateRequest): Promise { const permission = effectivePermission(request.gate, this.plan); const verdict = decide(request.gate, this.plan, this.decisions); - // Pi double-gate bridge: a pi-custom-tool dialog gate (executor "relay") whose "ask" decision - // was answered instantly from a STORED allow consumed one queued entry; the relay's own - // execution check will `decide()` the same call again and must see it. Re-append exactly the - // consumed decision. Guarded so it fires ONLY when the relay will actually consume it: - // allow only (on a deny the extension short-circuits without relaying, so an appended deny - // would linger and auto-deny a later identical call that should re-prompt); "ask" only (an - // allow/deny policy consumes nothing — decide returns before `stored.take` — and a pause - // consumed nothing); relay executor only (builtins have no relay second gate); and only - // when the dialog-gate bridge is on (see ApprovalResponderOptions). - if ( - this.options.bridgeRelayDoubleGate && - request.gate.executor === "relay" && - permission === "ask" && - verdict.kind === "allow" - ) { - this.decisions.appendDecision(request.gate, verdict.kind); - } this.log( `[HITL] gate toolName=${JSON.stringify(request.gate.toolName)} ` + `permission=${permission} outcome=${verdict.kind}`, diff --git a/services/runner/src/server.ts b/services/runner/src/server.ts index c73862c524..6e652a243a 100644 --- a/services/runner/src/server.ts +++ b/services/runner/src/server.ts @@ -379,9 +379,9 @@ export async function runWithKeepalive( ); // Whether a paused turn holds a single, parkable permission gate (a Claude ACP gate or a Pi - // dialog gate). Only such a gate carries a `respondPermission`-answerable id; a Pi file-relay - // gate or a client-tool MCP pause never records `parkedApproval`, and more than one pending - // gate cannot be answered by the single-gate resume — both stay on the cold path, logged. + // ACP gate). Only such a gate carries a `respondPermission`-answerable id; a client-tool MCP + // pause never records `parkedApproval`, and more than one pending gate cannot be answered by + // the single-gate resume — both stay on the cold path, logged. const approvalToPark = ( env: SessionEnvironment, result: AgentRunResult, @@ -615,9 +615,9 @@ export async function runWithKeepalive( if ( !parked || (parked.gateType !== "claude-acp-permission" && - parked.gateType !== "pi-dialog-permission") + parked.gateType !== "pi-acp-permission") ) { - // Defensive: only a parkable gate type (Claude ACP or Pi dialog) ever parks here. Both + // Defensive: only a parkable gate type (Claude ACP or Pi ACP) ever parks here. Both // resume via `respondPermission` on the live session; the daemon maps the reply by kind. mismatch = "unrecognized-gate-type"; } else if (!decision) { diff --git a/services/runner/src/tools/dispatch.ts b/services/runner/src/tools/dispatch.ts index ff41211ff4..a670aed741 100644 --- a/services/runner/src/tools/dispatch.ts +++ b/services/runner/src/tools/dispatch.ts @@ -32,16 +32,12 @@ import { callAgentaTool } from "./callback.ts"; import { runCodeTool } from "./code.ts"; import { assertRequiredArguments } from "./spec-schema.ts"; import { - RELAY_PERMISSION_PROTOCOL, RELAY_POLL_MS, RELAY_REQ_SUFFIX, RELAY_RES_SUFFIX, RELAY_TIMEOUT_MS, - parsePermissionRelayResponse, sanitizeRelayId, sleep, - type PermissionRelayRequest, - type PermissionRelayResponse, type RelayResponse, } from "./relay.ts"; @@ -110,108 +106,6 @@ export async function relayToolCall( throw new Error(`tool relay timed out for ${toolName}`); } -function oneLineReason(reason: string): string { - return reason.replace(/\s+/g, " ").trim() || "Permission check failed."; -} - -function denyPermissionRelayResponse(reason: string): PermissionRelayResponse { - return { - kind: "permission", - ok: false, - verdict: "deny", - reason: oneLineReason(reason), - }; -} - -/** - * Pi builtin permission check: write a permission request into the same relay directory the - * runner watches for tool execution, then poll for its permission response. The extension must - * fail closed because returning nothing lets Pi execute the builtin. - */ -export async function relayPermissionCheck( - dir: string, - toolName: string, - toolCallId: string, - args: unknown, -): Promise { - const id = sanitizeRelayId(toolCallId); - const reqPath = `${dir}/${id}${RELAY_REQ_SUFFIX}`; - const resPath = `${dir}/${id}${RELAY_RES_SUFFIX}`; - try { - mkdirSync(dir, { recursive: true }); - } catch { - // The runner also creates it; a race here is harmless. - } - - const req: PermissionRelayRequest = { - kind: "permission", - protocol: RELAY_PERMISSION_PROTOCOL, - toolName, - toolCallId, - args: args ?? {}, - }; - try { - writeFileSync(reqPath, JSON.stringify(req), "utf-8"); - } catch (err) { - return denyPermissionRelayResponse( - `permission relay request for ${toolName} could not be written: ${ - err instanceof Error ? err.message : String(err) - }`, - ); - } - - const cleanup = (): void => { - try { - unlinkSync(reqPath); - } catch { - /* best-effort cleanup */ - } - try { - unlinkSync(resPath); - } catch { - /* best-effort cleanup */ - } - }; - - const deadline = Date.now() + RELAY_TIMEOUT_MS; - while (Date.now() < deadline) { - if (existsSync(resPath)) { - let parsed: PermissionRelayResponse | undefined; - try { - parsed = parsePermissionRelayResponse( - JSON.parse(readFileSync(resPath, "utf-8")), - ); - } catch { - cleanup(); - return denyPermissionRelayResponse( - `permission relay response for ${toolName} was unparseable`, - ); - } - cleanup(); - if (!parsed) { - return denyPermissionRelayResponse( - `permission relay response for ${toolName} was unparseable`, - ); - } - if (!parsed.ok) { - return denyPermissionRelayResponse( - parsed.reason || `permission relay failed for ${toolName}`, - ); - } - return parsed; - } - await sleep(RELAY_POLL_MS); - } - try { - unlinkSync(reqPath); - } catch { - /* best-effort cleanup */ - } - return denyPermissionRelayResponse( - `permission relay timed out for ${toolName}`, - ); -} - /** * Execute one resolved tool and return its result text. Throws on failure; every call site * turns the throw into a tool-error result so the model loop continues rather than crashing. diff --git a/services/runner/src/tools/relay.ts b/services/runner/src/tools/relay.ts index ac0db98774..881041397c 100644 --- a/services/runner/src/tools/relay.ts +++ b/services/runner/src/tools/relay.ts @@ -38,11 +38,6 @@ import type { RunContext, ToolCallbackContext, } from "../protocol.ts"; -import { - piBuiltinIdentity, - type GateDescriptor, - type Verdict, -} from "../permission-plan.ts"; import type { ClientToolRelay } from "./client-tool-relay.ts"; import { assertRequiredArguments } from "./spec-schema.ts"; @@ -76,7 +71,6 @@ export const RELAY_POLL_MAX_MS = Number( export const RELAY_POLL_IDLE_GROW_AFTER = Number( process.env.AGENTA_AGENT_TOOLS_RELAY_IDLE_GROW_AFTER ?? 5, ); -export const RELAY_PERMISSION_PROTOCOL = 1; /** The next poll delay given the count of consecutive idle polls (no new request seen). */ export function relayPollDelayMs(idlePolls: number): number { @@ -91,14 +85,7 @@ export interface ExecuteRelayRequest { toolCallId: string; args: unknown; } -export interface PermissionRelayRequest { - kind: "permission"; - protocol: typeof RELAY_PERMISSION_PROTOCOL; - toolName: string; - toolCallId: string; - args: unknown; -} -export type RelayRequest = ExecuteRelayRequest | PermissionRelayRequest; +export type RelayRequest = ExecuteRelayRequest; export interface ExecuteRelayResponse { kind?: "execute"; @@ -107,27 +94,6 @@ export interface ExecuteRelayResponse { error?: string; } export type RelayResponse = ExecuteRelayResponse; -export type PermissionRelayVerdict = "allow" | "deny" | "pendingApproval"; -export interface PermissionRelayResponse { - kind: "permission"; - ok: boolean; - verdict: PermissionRelayVerdict; - reason?: string; -} -export type RelayRecordResponse = - ExecuteRelayResponse | PermissionRelayResponse; -export interface RelayPermissions { - /** False when the harness raises its own gates first (Claude); the relay then executes - * what reaches it. True when the relay is the only gate (Pi). */ - enforce: boolean; - decide: (gate: GateDescriptor) => Verdict; - /** Called when an ask pauses at the relay: emit the approval event and pause the turn. */ - onPendingApproval: (info: { - toolCallId: string; - toolName: string; - args: unknown; - }) => { emitted: boolean }; -} const PAUSED = Symbol("paused"); /** Make a tool-call id safe to use as a filename (and bounded). */ @@ -138,24 +104,6 @@ export function sanitizeRelayId(id: string): string { export const sleep = (ms: number): Promise => new Promise((r) => setTimeout(r, ms)); -export function parsePermissionRelayResponse( - value: unknown, -): PermissionRelayResponse | undefined { - if (!isRecord(value)) return undefined; - if (value.kind !== "permission") return undefined; - if (typeof value.ok !== "boolean") return undefined; - if (!isPermissionRelayVerdict(value.verdict)) return undefined; - if (value.reason !== undefined && typeof value.reason !== "string") { - return undefined; - } - return { - kind: "permission", - ok: value.ok, - verdict: value.verdict, - ...(value.reason === undefined ? {} : { reason: value.reason }), - }; -} - export interface RelayHost { list: (dir: string) => Promise; read: (path: string) => Promise; @@ -203,11 +151,14 @@ export function sandboxRelayHost(sandbox: any): RelayHost { }; } +// The relay carries EXECUTION only. Permission gates never ride these files: Claude raises its +// own ACP gates before a call reaches the relay, and a Pi gate rides the extension's +// `ctx.ui.confirm` dialog onto the ACP permission plane (Pi approval parking), decided and +// parked by the runner's permission responder before the extension writes an execute request. async function executeRelayedTool( spec: ResolvedToolSpec, req: ExecuteRelayRequest, callback: ToolCallbackContext | undefined, - permissions: RelayPermissions, runContext: RunContext | undefined, clientToolRelay: ClientToolRelay | undefined, ): Promise { @@ -237,33 +188,6 @@ async function executeRelayedTool( return JSON.stringify(decision.output ?? {}); } - if (permissions.enforce) { - const gate: GateDescriptor = { - executor: "relay", - toolName: spec.name, - specPermission: spec.permission, - readOnlyHint: spec.readOnly, - args: req.args, - }; - const verdict = permissions.decide(gate); - if (verdict.kind === "deny") { - if (spec.permission === "deny") { - return authoredDenyReason(spec.name); - } - return permissionPolicyDenyReason(spec.name); - } - if (verdict.kind === "pendingApproval") { - // Pi file-relay approvals are recorded here. Claude's approval card is - // harness-rendered before this point, so this relay cannot redact it. - permissions.onPendingApproval({ - toolCallId: req.toolCallId, - toolName: spec.name, - args: pendingApprovalArgs(spec, req.args), - }); - return PAUSED; - } - } - return executeAllowedRelayedTool(spec, req, callback, runContext); } @@ -318,140 +242,11 @@ async function executeAllowedRelayedTool( * the in-sandbox extension is waiting on. Returns `stop()` to end the loop and drain any * in-flight executions; call it once the prompt resolves. */ -function permissionPolicyDenyReason(toolName: string): string { - return `Tool '${toolName}' is denied by the permission policy.`; -} - -function authoredDenyReason(toolName: string): string { - return `Tool '${toolName}' is denied by policy.`; -} - -function permissionProtocolMismatchReason(): string { - return "Permission check denied because of a runner/extension version mismatch."; -} - -function logPermissionRelayError(message: string): void { - process.stderr.write(`[tool-relay] ERROR ${message}\n`); -} - -function permissionDenyResponse(reason: string): PermissionRelayResponse { - return { kind: "permission", ok: true, verdict: "deny", reason }; -} - -function isPermissionRelayVerdict( - value: unknown, -): value is PermissionRelayVerdict { - return value === "allow" || value === "deny" || value === "pendingApproval"; -} - -function isRecord(value: unknown): value is Record { - return typeof value === "object" && value !== null && !Array.isArray(value); -} - -function cloneJsonish(value: unknown): unknown { - if (Array.isArray(value)) return value.map((item) => cloneJsonish(item)); - if (!isRecord(value)) return value; - const out: Record = {}; - for (const [key, item] of Object.entries(value)) { - out[key] = cloneJsonish(item); - } - return out; -} - -function pruneEmptyAncestors(target: Record, path: string): void { - const parts = path.split("."); - const ancestors: Array<{ owner: Record; key: string }> = []; - let cursor = target; - for (const part of parts.slice(0, -1)) { - const next = cursor[part]; - if (!isRecord(next)) return; - ancestors.push({ owner: cursor, key: part }); - cursor = next; - } - for (const { owner, key } of ancestors.reverse()) { - const value = owner[key]; - if (!isRecord(value) || Object.keys(value).length > 0) return; - delete owner[key]; - } -} - -function pendingApprovalArgs( - spec: ResolvedToolSpec, - args: unknown, -): unknown { - if (!spec.callRef || !spec.contextBindings || !isRecord(args)) return args; - const displayArgs = cloneJsonish(args); - if (!isRecord(displayArgs)) return displayArgs; - for (const path of Object.keys(spec.contextBindings)) { - deepDelete(displayArgs, path); - pruneEmptyAncestors(displayArgs, path); - } - return displayArgs; -} - -async function handlePermissionRelayRequest( - req: Partial & { - kind: "permission"; - protocol?: unknown; - }, - fallbackId: string, - permissions: RelayPermissions, -): Promise { - const toolName = - typeof req.toolName === "string" ? req.toolName : ""; - if (req.protocol !== RELAY_PERMISSION_PROTOCOL) { - logPermissionRelayError( - `permission protocol mismatch for ${toolName}: got ${JSON.stringify( - req.protocol, - )}, expected ${RELAY_PERMISSION_PROTOCOL}; denying`, - ); - return permissionDenyResponse(permissionProtocolMismatchReason()); - } - - const identity = piBuiltinIdentity(toolName); - if (!identity) { - logPermissionRelayError( - `unknown builtin permission tool ${toolName}; denying`, - ); - return permissionDenyResponse(permissionPolicyDenyReason(toolName)); - } - - const gate: GateDescriptor = { - executor: "harness", - toolName: identity.ruleName, - readOnlyHint: identity.readOnly, - args: req.args, - }; - const verdict = permissions.decide(gate); - if (verdict.kind === "allow") { - return { kind: "permission", ok: true, verdict: "allow" }; - } - if (verdict.kind === "deny") { - return permissionDenyResponse(permissionPolicyDenyReason(toolName)); - } - - const pending = permissions.onPendingApproval({ - toolCallId: - typeof req.toolCallId === "string" ? req.toolCallId : fallbackId, - toolName, - args: req.args, - }); - return { - kind: "permission", - ok: true, - verdict: "pendingApproval", - reason: pending.emitted - ? `Waiting for approval of ${toolName}.` - : "Another approval is pending; retry after it resolves.", - }; -} - export function startToolRelay( host: RelayHost, relayDir: string, specs: ResolvedToolSpec[], callback: ToolCallbackContext | undefined, - permissions: RelayPermissions, runContext?: RunContext, clientToolRelay?: ClientToolRelay, ): { stop: () => Promise } { @@ -462,45 +257,26 @@ export function startToolRelay( const handle = async (reqName: string): Promise => { const id = reqName.slice(0, -RELAY_REQ_SUFFIX.length); - let res: RelayRecordResponse; - let permissionReqName: string | undefined; + let res: RelayResponse; try { const raw = await host.read(`${relayDir}/${reqName}`); const req = JSON.parse(raw) as RelayRequest; - if (req.kind === "permission") { - permissionReqName = - typeof req.toolName === "string" ? req.toolName : undefined; - res = await handlePermissionRelayRequest(req, id, permissions); - } else { - const spec = specsByName.get(req.toolName); - if (!spec) throw new Error(`unknown tool '${req.toolName}'`); - const text = await executeRelayedTool( - spec, - { ...req, toolCallId: req.toolCallId ?? id }, - callback, - permissions, - runContext, - clientToolRelay, - ); - if (text === PAUSED) return; - res = { ok: true, text }; - } + const spec = specsByName.get(req.toolName); + if (!spec) throw new Error(`unknown tool '${req.toolName}'`); + const text = await executeRelayedTool( + spec, + { ...req, toolCallId: req.toolCallId ?? id }, + callback, + runContext, + clientToolRelay, + ); + if (text === PAUSED) return; + res = { ok: true, text }; } catch (err) { - if (permissionReqName !== undefined) { - logPermissionRelayError( - `permission request for ${permissionReqName} failed: ${ - err instanceof Error ? err.message : String(err) - }; denying`, - ); - res = permissionDenyResponse( - permissionPolicyDenyReason(permissionReqName), - ); - } else { - res = { - ok: false, - error: err instanceof Error ? err.message : String(err), - }; - } + res = { + ok: false, + error: err instanceof Error ? err.message : String(err), + }; } try { await host.write( diff --git a/services/runner/tests/unit/builtin-grant-list.test.ts b/services/runner/tests/unit/builtin-grant-list.test.ts new file mode 100644 index 0000000000..730b0bd2ca --- /dev/null +++ b/services/runner/tests/unit/builtin-grant-list.test.ts @@ -0,0 +1,62 @@ +/** + * Grant-list regression pin (0e71bd0f7a): a run whose `tools` omits `bash` must not grant it, + * both at the `RunPlan` layer (`buildRunPlan`) and at the extension's active-tool-set layer + * (`replaceActiveBuiltinTools`). This bug shipped once as a silently-dropped grant list; pin it + * at both layers so it cannot recur unnoticed at either one. + * + * (The former relay permission-parity half of this file is gone with the relay permission + * plane: Pi gates ride the extension's `ctx.ui.confirm` dialog onto the ACP permission plane.) + * + * Run: pnpm test (or: pnpm exec vitest run tests/unit/builtin-grant-list.test.ts) + */ +import { describe, it } from "vitest"; +import assert from "node:assert/strict"; + +import type { AgentRunRequest } from "../../src/protocol.ts"; +import { buildRunPlan } from "../../src/engines/sandbox_agent/run-plan.ts"; +import { replaceActiveBuiltinTools } from "../../src/extensions/agenta.ts"; + +describe("grant-list regression pin (0e71bd0f7a)", () => { + it("buildRunPlan excludes bash from builtinGrants and turns gating on when `tools` omits it", () => { + const result = buildRunPlan( + { + harness: "pi_core", + messages: [{ role: "user", content: "hello" }], + tools: ["read"], + } as AgentRunRequest, + { createLocalCwd: () => "/tmp/local-cwd" }, + ); + + assert.equal(result.ok, true); + if (!result.ok) return; + assert.deepEqual(result.plan.builtinGrants, ["read"]); + assert.ok( + !result.plan.builtinGrants.includes("bash"), + "bash must not be silently re-granted", + ); + assert.equal(result.plan.builtinGatingActive, true); + }); + + it("replaceActiveBuiltinTools drops bash/edit/write and keeps read when only read is granted", () => { + const allTools = [ + { name: "read" }, + { name: "bash" }, + { name: "edit" }, + { name: "write" }, + { name: "grep" }, + { name: "find" }, + { name: "ls" }, + ]; + + const next = replaceActiveBuiltinTools( + ["read", "bash", "edit", "write"], + allTools, + ["read"], + ); + + assert.deepEqual(next, ["read"]); + assert.ok(!next.includes("bash")); + assert.ok(!next.includes("edit")); + assert.ok(!next.includes("write")); + }); +}); diff --git a/services/runner/tests/unit/extension-tools.test.ts b/services/runner/tests/unit/extension-tools.test.ts index 85976f3f75..58d5615702 100644 --- a/services/runner/tests/unit/extension-tools.test.ts +++ b/services/runner/tests/unit/extension-tools.test.ts @@ -38,7 +38,6 @@ const TOOL_ENV = [ "AGENTA_AGENT_CONTENT_CAPTURE_ENABLED", "AGENTA_AGENT_BUILTIN_GATING", "AGENTA_AGENT_BUILTIN_GRANTS", - "AGENTA_AGENT_PI_DIALOG_GATE", ]; /** A fake extension UI context whose `confirm` records its calls and returns a scripted answer. */ @@ -313,8 +312,6 @@ describe("agenta extension: Pi dialog gate (approval parking)", () => { it("builtin gate rides ctx.ui.confirm with the envelope; allow -> undefined", async () => { clearEnv(); process.env.AGENTA_AGENT_BUILTIN_GATING = "1"; - process.env.AGENTA_AGENT_TOOLS_RELAY_DIR = "/tmp/agenta-relay-unused"; - process.env.AGENTA_AGENT_PI_DIALOG_GATE = "1"; const pi = fakePi(); factory(pi as any); @@ -335,7 +332,6 @@ describe("agenta extension: Pi dialog gate (approval parking)", () => { it("builtin gate: deny -> block, and a thrown/absent dialog fails closed (block)", async () => { clearEnv(); process.env.AGENTA_AGENT_BUILTIN_GATING = "1"; - process.env.AGENTA_AGENT_PI_DIALOG_GATE = "1"; const pi = fakePi(); factory(pi as any); @@ -371,7 +367,6 @@ describe("agenta extension: Pi dialog gate (approval parking)", () => { // not be reached. process.env.AGENTA_AGENT_TOOLS_RELAY_DIR = "/tmp/agenta-relay-must-not-be-used"; - process.env.AGENTA_AGENT_PI_DIALOG_GATE = "1"; const pi = fakePi(); factory(pi as any); @@ -409,7 +404,6 @@ describe("agenta extension: Pi dialog gate (approval parking)", () => { { name: "request_connection", description: "connect", kind: "client" }, ]); process.env.AGENTA_AGENT_TOOLS_RELAY_DIR = dir; - process.env.AGENTA_AGENT_PI_DIALOG_GATE = "1"; const pi = fakePi(); factory(pi as any); @@ -431,28 +425,4 @@ describe("agenta extension: Pi dialog gate (approval parking)", () => { ); rmSync(dir, { recursive: true, force: true }); }); - - it("with the dialog flag OFF, the builtin gate keeps the relay path (no dialog raised)", async () => { - clearEnv(); - const dir = mkdtempSync(join(tmpdir(), "agenta-relay-off-")); - process.env.AGENTA_AGENT_BUILTIN_GATING = "1"; - process.env.AGENTA_AGENT_TOOLS_RELAY_DIR = dir; - // AGENTA_AGENT_PI_DIALOG_GATE intentionally unset. - - const pi = fakePi(); - factory(pi as any); - const hook = pi.handlers.tool_call![0]; - const { calls, ctx } = fakeDialogCtx(true); - - // Pre-seed the relay permission response (allow) so the relay path resolves without a runner. - writeFileSync( - join(dir, "tc-b.res.json"), - JSON.stringify({ kind: "permission", ok: true, verdict: "allow" }), - "utf-8", - ); - const result = await hook(builtinEvent("bash", { command: "ls" }), ctx); - assert.equal(calls.length, 0, "flag off: the dialog is never raised"); - assert.equal(result, undefined, "the relay allow let the builtin proceed"); - rmSync(dir, { recursive: true, force: true }); - }); }); diff --git a/services/runner/tests/unit/permission-record-fixture.test.ts b/services/runner/tests/unit/permission-record-fixture.test.ts deleted file mode 100644 index b1fa91c152..0000000000 --- a/services/runner/tests/unit/permission-record-fixture.test.ts +++ /dev/null @@ -1,150 +0,0 @@ -/** - * Phase 4 (docs/design/agent-workflows/projects/pi-builtin-gating/plan.md): "The relay record - * types are runtime files, not part of the `/run` golden wire, so no golden changes. Add a - * small fixture test for the permission record round-trip." - * - * This pins the on-disk shape of the permission relay records: the REQUEST record - * `{kind, protocol, toolName, toolCallId, args}` and the three RESPONSE verdict variants - * (allow / deny+reason / pendingApproval+reason), each written to a temp relay dir and read - * back exactly as `startToolRelay` and the extension's `relayPermissionCheck` would see them - * over the filesystem. It also pins two parser boundaries in `parsePermissionRelayResponse`: - * an unknown extra field must not break parsing (forward compatibility), and an execute-record - * shape (no `kind`) must never be accepted as a permission response (the discriminated-union - * boundary Phase 1 introduced so the two record kinds can never be cross-read). - * - * Run: pnpm test (or: pnpm exec vitest run tests/unit/permission-record-fixture.test.ts) - */ -import { afterEach, describe, it } from "vitest"; -import assert from "node:assert/strict"; -import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; - -import { - parsePermissionRelayResponse, - RELAY_PERMISSION_PROTOCOL, - type PermissionRelayResponse, -} from "../../src/tools/relay.ts"; - -const dirs: string[] = []; - -afterEach(() => { - while (dirs.length > 0) { - const dir = dirs.pop(); - if (dir) rmSync(dir, { recursive: true, force: true }); - } -}); - -function tempRelayDir(): string { - const dir = mkdtempSync(join(tmpdir(), "agenta-permission-record-fixture-")); - dirs.push(dir); - return dir; -} - -/** Write `value` as JSON to `/`, then read the bytes back off disk. Mirrors the - * relay's own file round trip (`host.write` then `host.read` in `tools/relay.ts`). */ -function roundTripJson( - dir: string, - name: string, - value: unknown, -): { raw: string; parsed: unknown } { - const path = join(dir, name); - const raw = JSON.stringify(value); - writeFileSync(path, raw, "utf-8"); - const readBack = readFileSync(path, "utf-8"); - assert.equal(readBack, raw, "the bytes on disk match what was written exactly"); - return { raw: readBack, parsed: JSON.parse(readBack) }; -} - -describe("permission relay request record fixture", () => { - it("round-trips the request record shape {kind, protocol, toolName, toolCallId, args}", () => { - const dir = tempRelayDir(); - const request = { - kind: "permission" as const, - protocol: RELAY_PERMISSION_PROTOCOL, - toolName: "bash", - toolCallId: "call-42", - args: { command: "npm test" }, - }; - - const { parsed } = roundTripJson(dir, "call-42.req.json", request); - - assert.deepEqual(parsed, request); - }); -}); - -describe("permission relay response record fixture (allow / deny / pendingApproval)", () => { - const variants: Array<{ name: string; response: PermissionRelayResponse }> = [ - { - name: "allow", - response: { kind: "permission", ok: true, verdict: "allow" }, - }, - { - name: "deny with reason", - response: { - kind: "permission", - ok: true, - verdict: "deny", - reason: "Tool 'bash' is denied by the permission policy.", - }, - }, - { - name: "pendingApproval with reason", - response: { - kind: "permission", - ok: true, - verdict: "pendingApproval", - reason: "Waiting for approval of bash.", - }, - }, - ]; - - for (const { name, response } of variants) { - it(`round-trips the ${name} response byte-stably through parsePermissionRelayResponse`, () => { - const dir = tempRelayDir(); - const { raw, parsed } = roundTripJson(dir, "call-42.res.json", response); - - assert.deepEqual(parsed, response); - - const reparsed = parsePermissionRelayResponse(parsed); - assert.deepEqual(reparsed, response); - - // Byte-stable: re-serializing the parsed record reproduces the exact bytes written to - // disk, so there is no field reordering or silent coercion across the round trip. - assert.equal(JSON.stringify(reparsed), raw); - }); - } - - it("tolerates an unknown extra field without breaking parsing", () => { - const dir = tempRelayDir(); - const withExtraField = { - kind: "permission", - ok: true, - verdict: "allow", - // A field a future protocol revision might add; today's parser must ignore it rather - // than reject the whole record (forward compatibility across a runner/extension skew). - futureField: "some-value-from-a-newer-runner", - }; - - const { parsed } = roundTripJson(dir, "call-99.res.json", withExtraField); - - assert.deepEqual(parsePermissionRelayResponse(parsed), { - kind: "permission", - ok: true, - verdict: "allow", - }); - }); - - it("does NOT parse an execute-record shape {toolName, toolCallId, args} (no kind) as a permission response", () => { - const dir = tempRelayDir(); - const executeRecord = { - toolName: "server_tool", - toolCallId: "call-1", - args: { a: 1 }, - }; - - const { parsed } = roundTripJson(dir, "call-1.req.json", executeRecord); - - assert.equal(parsePermissionRelayResponse(parsed), undefined); - }); -}); diff --git a/services/runner/tests/unit/pi-gate-envelope.test.ts b/services/runner/tests/unit/pi-gate-envelope.test.ts index 6e654f775a..9d851064db 100644 --- a/services/runner/tests/unit/pi-gate-envelope.test.ts +++ b/services/runner/tests/unit/pi-gate-envelope.test.ts @@ -243,7 +243,7 @@ describe("buildPiGateDescriptor (runner-side metadata recovery)", () => { assert.equal(reader!.readOnlyHint, true); }); - it("the envelope input is the gate args (stored-decision key parity with the relay)", () => { + it("the envelope input is the gate args (the stored-decision key)", () => { const g = buildPiGateDescriptor( { v: 1, @@ -253,15 +253,16 @@ describe("buildPiGateDescriptor (runner-side metadata recovery)", () => { toolCallId: "c", input: { a: 1, b: 2 }, }, - new Map(), + new Map([["t", {}]]), ); assert.deepEqual(g!.args, { a: 1, b: 2 }); }); - it("an unknown builtin name yields NO descriptor (the caller must reject it)", () => { - // Relay parity: the relay denies unknown builtins outright, and the sandbox-origin envelope - // must not put a fabricated name on the approval card. - const g = buildPiGateDescriptor( + it("an unknown name yields NO descriptor (the caller must reject it)", () => { + // The sandbox-origin envelope must not resolve a fabricated name against the default + // permission or put it on the approval card: a builtin outside the canonical set and a + // custom tool with no resolved spec both fail closed. + const unknownBuiltin = buildPiGateDescriptor( { v: 1, kind: "agenta.gate", @@ -272,6 +273,19 @@ describe("buildPiGateDescriptor (runner-side metadata recovery)", () => { }, undefined, ); - assert.equal(g, undefined); + assert.equal(unknownBuiltin, undefined); + + const unknownCustomTool = buildPiGateDescriptor( + { + v: 1, + kind: "agenta.gate", + gate: "pi-custom-tool", + toolName: "not_a_resolved_tool", + toolCallId: "c", + input: {}, + }, + new Map([["park_probe", {}]]), + ); + assert.equal(unknownCustomTool, undefined); }); }); diff --git a/services/runner/tests/unit/responder.test.ts b/services/runner/tests/unit/responder.test.ts index e9120b84b0..8376aa656f 100644 --- a/services/runner/tests/unit/responder.test.ts +++ b/services/runner/tests/unit/responder.test.ts @@ -12,7 +12,6 @@ import type { GateDescriptor, PermissionPlan, } from "../../src/permission-plan.ts"; -import { decide } from "../../src/permission-plan.ts"; import { ApprovalResponder, ConversationDecisions, @@ -909,222 +908,3 @@ describe("emitEvent", () => { assert.equal((ev as any).name, "weather"); }); }); - -describe("ConversationDecisions.appendDecision (Pi double-gate bridge)", () => { - const relayGate: GateDescriptor = { - executor: "relay", - toolName: "park_probe", - args: { token: "T" }, - }; - - it("re-appended decision is consumed by the next take on the same call", () => { - const decisions = new ConversationDecisions(new Map()); - decisions.appendDecision(relayGate, "allow"); - assert.equal(decisions.take(relayGate), "allow"); - assert.equal(decisions.take(relayGate), undefined, "consumed once"); - }); - - it("front-inserts so the relay's take gets THIS call's decision, not a later one", () => { - // Two identical-arg calls with DIFFERENT decisions (allow then deny), FIFO order. - const key = approvedCallKey("park_probe", { token: "T" })!; - const decisions = new ConversationDecisions( - new Map([[key, ["allow", "deny"]]]), - ); - // Call A: the dialog takes "allow" then re-appends it for the relay. - assert.equal(decisions.take(relayGate), "allow"); - decisions.appendDecision(relayGate, "allow"); - // The relay's immediate next take must see A's "allow", not B's "deny". - assert.equal(decisions.take(relayGate), "allow"); - // Call B: the dialog takes "deny", re-appends, the relay takes "deny". - assert.equal(decisions.take(relayGate), "deny"); - decisions.appendDecision(relayGate, "deny"); - assert.equal(decisions.take(relayGate), "deny"); - }); -}); - -describe("ApprovalResponder: Pi custom-tool double-gate accounting", () => { - const askPlan: PermissionPlan = { default: "ask", rules: [] }; - const bridge = { bridgeRelayDoubleGate: true }; - const relayGate: GateDescriptor = { - executor: "relay", - toolName: "park_probe", - args: { token: "T" }, - }; - - it("a stored-answered relay gate re-appends so the relay's decide still allows", async () => { - const key = approvedCallKey("park_probe", { token: "T" })!; - const decisions = new ConversationDecisions(new Map([[key, ["allow"]]])); - const responder = new ApprovalResponder( - askPlan, - decisions, - undefined, - bridge, - ); - - const verdict = await responder.onPermission({ - id: "p", - availableReplies: ["once", "reject"], - gate: relayGate, - }); - assert.equal( - verdict.kind, - "allow", - "the dialog gate allowed from the stored decision", - ); - // The relay's SECOND check on the same call still finds an allow (consume-1-append-1). - assert.equal(decide(relayGate, askPlan, decisions).kind, "allow"); - }); - - it("does NOT append with the bridge OFF (default), even for a relay ask allow", async () => { - // The Claude shape: a relay-executor gate but no relay enforcement (`enforce: plan.isPi`), - // so nothing consumes an appended decision; it would leak the allow to a LATER identical - // call — a flag-off behavior change. The default responder never appends. - const key = approvedCallKey("park_probe", { token: "T" })!; - const decisions = new ConversationDecisions(new Map([[key, ["allow"]]])); - const responder = new ApprovalResponder(askPlan, decisions); - - const verdict = await responder.onPermission({ - id: "p", - availableReplies: ["once", "reject"], - gate: relayGate, - }); - assert.equal(verdict.kind, "allow"); - // Consumed once, nothing re-appended: a later identical gate re-prompts. - assert.equal(decide(relayGate, askPlan, decisions).kind, "pendingApproval"); - }); - - it("does NOT re-append a DENY (the extension short-circuits; no relay consumer)", async () => { - // On a deny the custom tool's execute() returns the deny text WITHOUT relaying, so nothing - // consumes an appended deny; it would linger and auto-deny a later identical call that - // should re-prompt instead. - const key = approvedCallKey("park_probe", { token: "T" })!; - const decisions = new ConversationDecisions(new Map([[key, ["deny"]]])); - const responder = new ApprovalResponder( - askPlan, - decisions, - undefined, - bridge, - ); - - const verdict = await responder.onPermission({ - id: "p", - availableReplies: ["once", "reject"], - gate: relayGate, - }); - assert.equal(verdict.kind, "deny"); - assert.equal( - decide(relayGate, askPlan, decisions).kind, - "pendingApproval", - "the deny was consumed once and NOT re-appended; a later identical call re-prompts", - ); - }); - - it("does NOT append for a builtin (harness executor) — no relay second gate", async () => { - const key = approvedCallKey("Bash", { command: "ls" })!; - const decisions = new ConversationDecisions(new Map([[key, ["allow"]]])); - const responder = new ApprovalResponder( - askPlan, - decisions, - undefined, - bridge, - ); - const harnessGate: GateDescriptor = { - executor: "harness", - toolName: "Bash", - args: { command: "ls" }, - }; - - const verdict = await responder.onPermission({ - id: "p", - availableReplies: ["once", "reject"], - gate: harnessGate, - }); - assert.equal(verdict.kind, "allow"); - // Consumed once; nothing re-appended, so a second take finds nothing. - assert.equal( - decide(harnessGate, askPlan, decisions).kind, - "pendingApproval", - ); - }); - - it("does NOT append when the policy (allow) decided without consulting stored", async () => { - const allowPlan: PermissionPlan = { default: "allow", rules: [] }; - const decisions = new ConversationDecisions(new Map()); - const responder = new ApprovalResponder( - allowPlan, - decisions, - undefined, - bridge, - ); - const verdict = await responder.onPermission({ - id: "p", - availableReplies: ["once", "reject"], - gate: relayGate, - }); - assert.equal(verdict.kind, "allow"); - // A policy allow never consumed a stored decision, so none is re-appended (the relay's own - // policy-allow decide also passes without needing one). - assert.equal(decide(relayGate, allowPlan, decisions).kind, "allow"); - }); - - it("does NOT append when the gate pauses (no stored decision consumed)", async () => { - const decisions = new ConversationDecisions(new Map()); - const responder = new ApprovalResponder( - askPlan, - decisions, - undefined, - bridge, - ); - const verdict = await responder.onPermission({ - id: "p", - availableReplies: ["once", "reject"], - gate: relayGate, - }); - assert.equal(verdict.kind, "pendingApproval"); - assert.equal(decide(relayGate, askPlan, decisions).kind, "pendingApproval"); - }); - - it("warm resume: the folded {approved} envelope seeds the relay's execution check", () => { - // On a warm approval resume the FE folds the gated tool_call + the {approved} decision into the - // request. The resume turn builds ConversationDecisions from it; the relay's decide (the second - // gate after the dialog resolves via respondPermission) must find an allow for the SAME key. - const resumeRequest: AgentRunRequest = { - harness: "pi", - messages: [ - { role: "user", content: "do it" }, - { - role: "assistant", - content: [ - { - type: "tool_call", - toolCallId: "call_REAL", - toolName: "park_probe", - input: { token: "T" }, - }, - ], - }, - { - role: "user", - content: [ - { - type: "tool_result", - toolCallId: "call_REAL", - output: { approved: true }, - }, - ], - }, - ], - }; - const decisions = new ConversationDecisions( - extractApprovalDecisions(resumeRequest), - ); - // The relay gate the in-sandbox execution raises: same name + exact params (key parity). - const relayGate: GateDescriptor = { - executor: "relay", - toolName: "park_probe", - specPermission: "ask", - args: { token: "T" }, - }; - assert.equal(decide(relayGate, askPlan, decisions).kind, "allow"); - }); -}); diff --git a/services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts b/services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts index 4a6f923a3f..31d3697dfd 100644 --- a/services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts +++ b/services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts @@ -495,7 +495,11 @@ function piGateRequest(opts: { kind: "other", status: "pending", title: opts.title ?? PI_GATE_DIALOG_TITLE, - rawInput: { method: "confirm", title: PI_GATE_DIALOG_TITLE, message }, + rawInput: { + method: "confirm", + title: opts.title ?? PI_GATE_DIALOG_TITLE, + message, + }, }, }; } @@ -506,6 +510,17 @@ function permissionPlan( return { default: defaultMode, rules: [] }; } +/** The resolved-specs map that marks a Pi run and feeds metadata recovery. Every custom tool a + * test raises must be present: an unknown name fails closed by design. */ +function piSpecs( + entries: Array<[string, PiToolSpecMeta]> = [ + ["park_probe", {}], + ["x", {}], + ], +): Map { + return new Map(entries); +} + describe("attachPermissionResponder: Pi dialog gate", () => { it("normalizes the synthetic id to the envelope's REAL id everywhere it is read", async () => { const { session, emit } = makeSession(); @@ -518,7 +533,7 @@ describe("attachPermissionResponder: Pi dialog gate", () => { run: { emitEvent: (event) => events.push(event) }, responder: fakeResponder({ kind: "pendingApproval" }), latch: new PendingApprovalLatch(), - dialogGateEnabled: true, + piToolSpecsByName: piSpecs(), onPausedToolCall: (id) => pausedToolCalls.push(id), onUserApprovalGate: (info) => gates.push(info), }); @@ -535,7 +550,7 @@ describe("attachPermissionResponder: Pi dialog gate", () => { // pause bookkeeping, the park record, and the emitted card ALL key on the real id. assert.deepEqual(pausedToolCalls, ["call_REAL_123"]); assert.equal(gates[0].toolCallId, "call_REAL_123"); - assert.equal(gates[0].gateType, "pi-dialog-permission"); + assert.equal(gates[0].gateType, "pi-acp-permission"); const payload = (events[0] as any).payload; assert.equal(payload.toolCallId, "call_REAL_123"); assert.equal(payload.toolCall.toolCallId, "call_REAL_123"); @@ -559,7 +574,7 @@ describe("attachPermissionResponder: Pi dialog gate", () => { // A default-allow responder: if the malformed request fell through it would ALLOW. responder: fakeResponder({ kind: "allow" }), latch: new PendingApprovalLatch(), - dialogGateEnabled: true, + piToolSpecsByName: piSpecs(), onPause: () => { pauses += 1; }, @@ -595,7 +610,7 @@ describe("attachPermissionResponder: Pi dialog gate", () => { run: { emitEvent: () => {} }, responder: fakeResponder({ kind: "pendingApproval" }, undefined, seen), latch: new PendingApprovalLatch(), - dialogGateEnabled: true, + piToolSpecsByName: piSpecs(), }); emit( piGateRequest({ @@ -631,7 +646,6 @@ describe("attachPermissionResponder: Pi dialog gate", () => { run: { emitEvent: () => {} }, responder, latch: new PendingApprovalLatch(), - dialogGateEnabled: true, piToolSpecsByName, }); emit( @@ -673,7 +687,7 @@ describe("attachPermissionResponder: Pi dialog gate", () => { run: { emitEvent: () => {} }, responder, latch: new PendingApprovalLatch(), - dialogGateEnabled: true, + piToolSpecsByName: piSpecs(), onPause: () => { pauses += 1; }, @@ -710,7 +724,7 @@ describe("attachPermissionResponder: Pi dialog gate", () => { run: { emitEvent: () => {} }, responder, latch: new PendingApprovalLatch(), - dialogGateEnabled: true, + piToolSpecsByName: piSpecs(), onPause: () => { pauses += 1; }, @@ -727,7 +741,7 @@ describe("attachPermissionResponder: Pi dialog gate", () => { await flushPromises(); assert.equal(pauses, 1); - assert.equal(gates[0].gateType, "pi-dialog-permission"); + assert.equal(gates[0].gateType, "pi-acp-permission"); assert.equal(gates[0].toolName, "Bash"); }); @@ -746,7 +760,7 @@ describe("attachPermissionResponder: Pi dialog gate", () => { run: { emitEvent: (event) => events.push(event) }, responder, latch: new PendingApprovalLatch(), - dialogGateEnabled: true, + piToolSpecsByName: piSpecs(), onPause: () => { pauses += 1; }, @@ -766,11 +780,43 @@ describe("attachPermissionResponder: Pi dialog gate", () => { assert.deepEqual(events, [], "no approval card for a fabricated name"); }); - it("with detection OFF, a Claude gate whose title collides with the dialog title takes today's path", async () => { - // The MF-1 regression: attachPermissionResponder is shared by Claude and Pi. With the - // dialog gate off, a Claude gate titled literally "agenta-approval" (editing a file with - // that name, a bash command equal to it) has no envelope and must pause/resolve exactly as - // on the base path, never auto-reject. + it("an unknown custom-tool name (no resolved spec) rejects (fail closed)", async () => { + const replies: Array<{ id: string; reply: string }> = []; + const { session, emit } = makeSession(async (id, reply) => { + replies.push({ id, reply }); + }); + const events: AgentEvent[] = []; + let pauses = 0; + // A default-allow responder: if the fabricated name fell through it would ALLOW. + const responder = fakeResponder({ kind: "allow" }); + + attachPermissionResponder({ + session, + run: { emitEvent: (event) => events.push(event) }, + responder, + latch: new PendingApprovalLatch(), + piToolSpecsByName: piSpecs([["park_probe", {}]]), + }); + emit( + piGateRequest({ + gate: "pi-custom-tool", + toolName: "not_a_resolved_tool", + toolCallId: "c", + input: {}, + }), + ); + await flushPromises(); + + assert.deepEqual(replies, [{ id: "perm-pi", reply: "reject" }]); + assert.equal(pauses, 0, "an unresolved custom tool never pauses"); + assert.deepEqual(events, [], "no approval card for a fabricated name"); + }); + + it("a Claude run (no Pi specs) never enters envelope detection, even on a title collision", async () => { + // attachPermissionResponder is shared by Claude and Pi. On a Claude run (piToolSpecsByName + // absent), a gate titled literally "agenta-approval" (editing a file with that name, a bash + // command equal to it) has no envelope and must pause/resolve exactly as on the base path, + // never auto-reject. const replies: Array<{ id: string; reply: string }> = []; const { session, emit } = makeSession(async (id, reply) => { replies.push({ id, reply }); @@ -784,7 +830,7 @@ describe("attachPermissionResponder: Pi dialog gate", () => { run: { emitEvent: (event) => events.push(event) }, responder: fakeResponder({ kind: "pendingApproval" }), latch: new PendingApprovalLatch(), - // dialogGateEnabled intentionally absent (flag off / Claude run). + // piToolSpecsByName intentionally absent (a Claude run). onPause: () => { pauses += 1; }, diff --git a/services/runner/tests/unit/sandbox-agent-orchestration.test.ts b/services/runner/tests/unit/sandbox-agent-orchestration.test.ts index 8192f369af..9a1d718d43 100644 --- a/services/runner/tests/unit/sandbox-agent-orchestration.test.ts +++ b/services/runner/tests/unit/sandbox-agent-orchestration.test.ts @@ -14,6 +14,7 @@ import { TOOL_NOT_EXECUTED_PAUSED, } from "../../src/tracing/otel.ts"; import { PendingApprovalPauseController } from "../../src/engines/sandbox_agent/pause.ts"; +import { buildPiGateEnvelope } from "../../src/engines/sandbox_agent/pi-gate-envelope.ts"; import type { PermissionDecision } from "../../src/responder.ts"; import { runSandboxAgent, @@ -36,7 +37,7 @@ interface FakeOptions { streamUsage?: Record; output?: string; promptError?: Error; - permissionDecision?: PermissionDecision; + permissionDecision?: PermissionDecision | "pendingApproval"; emitPermission?: boolean; permissionToolCallId?: string; permissionToolName?: string; @@ -695,15 +696,11 @@ describe("runSandboxAgent orchestration", () => { [{ name: "server_tool", kind: "callback" }], undefined, ]); - const relayPermissions = calls.toolRelayArgs?.[4] as any; - assert.equal(relayPermissions.enforce, true, "Pi enforces at the relay"); - assert.equal(typeof relayPermissions.decide, "function"); - assert.equal(typeof relayPermissions.onPendingApproval, "function"); - // No runContext on the request. - assert.equal(calls.toolRelayArgs?.[5], undefined); + // The relay carries execution only (no permissions argument): no runContext here. + assert.equal(calls.toolRelayArgs?.[4], undefined); // Trailing arg is the relay callbacks object (client-tool + park handlers). assert.deepEqual( - Object.keys((calls.toolRelayArgs?.[6] ?? {}) as object).sort(), + Object.keys((calls.toolRelayArgs?.[5] ?? {}) as object).sort(), ["onClientTool", "onPause"], ); assert.equal( @@ -737,11 +734,7 @@ describe("runSandboxAgent orchestration", () => { true, "the run succeeds; gateway tools reach Claude", ); - assert.equal( - (calls.toolRelayArgs?.[4] as any)?.enforce, - false, - "Claude gates before the relay, so the relay does not re-enforce", - ); + // The relay carries execution only; Claude's own ACP gates decide before a call reaches it. const mcpServers = calls.createSessionOptions?.sessionInit?.mcpServers ?? []; assert.equal( @@ -1483,9 +1476,11 @@ describe("runSandboxAgent default ApprovalResponder wiring", () => { ); }); - it("drops teardown tool updates after a Pi relay approval pause while keeping other ids", async () => { - const relayPause = { fire: () => {} }; - const { calls, deps } = fakeHarness({ + it("drops teardown tool updates after a Pi approval pause while keeping other ids", async () => { + // The Pi ask arrives as an ACP permission request carrying the gate envelope (the dialog + // plane); the pause must suppress the GATED call's teardown updates while the sibling is + // settled deterministically. + const { deps } = fakeHarness({ promptEvents: [ { payload: { @@ -1506,7 +1501,28 @@ describe("runSandboxAgent default ApprovalResponder wiring", () => { }, }, ], - afterPromptEvents: () => relayPause.fire(), + emitPermission: true, + permissionDecision: "pendingApproval", + permissionRequests: [ + { + id: "perm-pi", + availableReplies: ["once", "reject"], + toolCall: { + toolCallId: "pi-ui-synthetic", + title: "agenta-approval", + rawInput: { + method: "confirm", + title: "agenta-approval", + message: buildPiGateEnvelope({ + gate: "pi-custom-tool", + toolName: "approval_needed", + toolCallId: "tool-1", + input: { path: "a" }, + }), + }, + }, + }, + ], postPermissionEvents: [ { payload: { @@ -1531,14 +1547,6 @@ describe("runSandboxAgent default ApprovalResponder wiring", () => { ], }); deps.createOtel = createSandboxAgentOtel as any; - relayPause.fire = () => { - const relayPermissions = calls.toolRelayArgs?.[4] as any; - relayPermissions.onPendingApproval({ - toolCallId: "tool-1", - toolName: "approval_needed", - args: { path: "a" }, - }); - }; const result = await runSandboxAgent( { @@ -1560,8 +1568,12 @@ describe("runSandboxAgent default ApprovalResponder wiring", () => { assert.deepEqual( result.events ?.filter((event) => event.type === "interaction_request") - .map((event) => ({ id: (event as any).id, kind: (event as any).kind })), - [{ id: "tool-1", kind: "user_approval" }], + .map((event) => ({ + id: (event as any).id, + kind: (event as any).kind, + toolCallId: (event as any).payload?.toolCallId, + })), + [{ id: "perm-pi", kind: "user_approval", toolCallId: "tool-1" }], ); assert.deepEqual( result.events diff --git a/services/runner/tests/unit/sandbox-agent-pi-assets.test.ts b/services/runner/tests/unit/sandbox-agent-pi-assets.test.ts index 19e1cea418..1357a397fc 100644 --- a/services/runner/tests/unit/sandbox-agent-pi-assets.test.ts +++ b/services/runner/tests/unit/sandbox-agent-pi-assets.test.ts @@ -151,7 +151,7 @@ describe("buildPiExtensionEnv", () => { assert.equal(env.AGENTA_AGENT_TOOLS_RELAY_DIR, undefined); }); - it("sets builtin gating env and relay dir without custom tools", () => { + it("sets builtin gating env WITHOUT a relay dir (the gate rides the ACP dialog plane)", () => { const env = buildPiExtensionEnv({} as AgentRunRequest, false, { relayDir: "/tmp/relay", builtinGatingActive: true, @@ -160,32 +160,10 @@ describe("buildPiExtensionEnv", () => { assert.equal(env.AGENTA_AGENT_BUILTIN_GATING, "1"); assert.equal(env.AGENTA_AGENT_BUILTIN_GRANTS, "read,write"); - assert.equal(env.AGENTA_AGENT_TOOLS_RELAY_DIR, "/tmp/relay"); + assert.equal(env.AGENTA_AGENT_TOOLS_RELAY_DIR, undefined); assert.equal(env.AGENTA_AGENT_TOOLS_PUBLIC_SPECS, undefined); }); - it("sets the Pi dialog gate flag only when active (default off = byte-identical)", () => { - // Flag off (undefined): no dialog env, so the extension keeps the relay path. - assert.equal( - buildPiExtensionEnv({} as AgentRunRequest, false, {}) - .AGENTA_AGENT_PI_DIALOG_GATE, - undefined, - ); - assert.equal( - buildPiExtensionEnv({} as AgentRunRequest, false, { - dialogGateActive: false, - }).AGENTA_AGENT_PI_DIALOG_GATE, - undefined, - ); - // Flag on: exports the sandbox-side switch. - assert.equal( - buildPiExtensionEnv({} as AgentRunRequest, false, { - dialogGateActive: true, - }).AGENTA_AGENT_PI_DIALOG_GATE, - "1", - ); - }); - it("accepts snake_case tool schemas from older Python wire payloads", () => { const env = buildPiExtensionEnv( { 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 4b137dc1d7..971305f8d9 100644 --- a/services/runner/tests/unit/sandbox-agent-run-plan.test.ts +++ b/services/runner/tests/unit/sandbox-agent-run-plan.test.ts @@ -86,7 +86,10 @@ describe("buildRunPlan", () => { // cwd: an ephemeral sibling whose leaf is the cwd basename. assert.ok(!result.plan.relayDir.startsWith(result.plan.cwd)); assert.ok(result.plan.relayDir.endsWith("/agenta/relay/local-cwd")); - assert.equal(result.plan.usageOutPath, `${result.plan.relayDir}/.agenta-usage.json`); + assert.equal( + result.plan.usageOutPath, + `${result.plan.relayDir}/.agenta-usage.json`, + ); assert.equal(result.plan.prompt, " ship it "); assert.equal(result.plan.agentsMd, "instructions"); assert.equal(result.plan.systemPrompt, "system"); @@ -137,7 +140,8 @@ describe("buildRunPlan", () => { if (!result.ok) return; assert.deepEqual(result.plan.builtinGrants, ["read", "write"]); assert.equal(result.plan.builtinGatingActive, true); - assert.equal(result.plan.useToolRelay, true); + // Builtin gating rides the ACP dialog plane, not the relay: no custom tools, no relay. + assert.equal(result.plan.useToolRelay, false); }); it("turns builtin gating on when grants include Pi-nondefault builtins", () => { @@ -218,7 +222,7 @@ describe("buildRunPlan", () => { assert.equal(omitted.plan.builtinGatingActive, false); assert.deepEqual(none.plan.builtinGrants, []); assert.equal(none.plan.builtinGatingActive, true); - assert.equal(none.plan.useToolRelay, true); + assert.equal(none.plan.useToolRelay, false); }); it("turns builtin gating on when the permission kill switch is set", () => { @@ -242,7 +246,7 @@ describe("buildRunPlan", () => { "write", ]); assert.equal(result.plan.builtinGatingActive, true); - assert.equal(result.plan.useToolRelay, true); + assert.equal(result.plan.useToolRelay, false); }); it("turns builtin gating on when an all-allow policy has a builtin rule", () => { @@ -586,8 +590,15 @@ describe("buildRunPlan", () => { assert.equal(result.ok, false); if (result.ok) return; assert.match(result.error, /non-Pi harness on a remote sandbox/); - assert.match(result.error, /docs\/design\/agent-workflows\/projects\/remote-tools-delivery\//); - assert.equal(created, false, "fails before any cwd is created (up-front gate)"); + assert.match( + result.error, + /docs\/design\/agent-workflows\/projects\/remote-tools-delivery\//, + ); + assert.equal( + created, + false, + "fails before any cwd is created (up-front gate)", + ); }); it("refuses claude x UNKNOWN remote provider x tools (fails closed, not open)", () => { @@ -692,7 +703,11 @@ describe("buildRunPlan", () => { 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)"); + 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)", () => { @@ -956,8 +971,13 @@ describe("buildRunPlan durableCwd (prefix-derived cwd)", () => { assert.equal(result.ok, true); if (!result.ok) return; - assert.equal(result.plan.cwd, "/home/sandbox/agenta/mounts/proj-1/mount-abc"); - assert.deepEqual(daytonaCwdCalls, ["/home/sandbox/agenta/mounts/proj-1/mount-abc"]); + assert.equal( + result.plan.cwd, + "/home/sandbox/agenta/mounts/proj-1/mount-abc", + ); + assert.deepEqual(daytonaCwdCalls, [ + "/home/sandbox/agenta/mounts/proj-1/mount-abc", + ]); }); it("falls back to ephemeral cwd when durableCwd is absent (non-session / sign failed)", () => { @@ -991,7 +1011,10 @@ describe("buildRunPlan durableCwd (prefix-derived cwd)", () => { function makePlan() { return buildRunPlan( - { harness: "claude", messages: [{ role: "user", content: "hi" }] } as AgentRunRequest, + { + harness: "claude", + messages: [{ role: "user", content: "hi" }], + } as AgentRunRequest, { durableCwd: localPath, createLocalCwd: (durable) => durable ?? "/tmp/fallback", diff --git a/services/runner/tests/unit/session-keepalive-approval.test.ts b/services/runner/tests/unit/session-keepalive-approval.test.ts index a8df3056f9..b24ebf2049 100644 --- a/services/runner/tests/unit/session-keepalive-approval.test.ts +++ b/services/runner/tests/unit/session-keepalive-approval.test.ts @@ -346,7 +346,7 @@ describe("runWithKeepalive: approval park + resume", () => { permissionId: "perm-1", toolCallId: "tc-gate", toolName: "commit", - gateType: "pi-dialog-permission", + gateType: "pi-acp-permission", }, toolCallIds: ["tc-gate"], }, @@ -1363,95 +1363,4 @@ describe("runTurn: real approval park + respondPermission resume", () => { assert.equal(calls.sessionDestroyed, 1, "destroy tore the session down"); assert.equal(calls.sandboxDestroyed, 1); }); - - it("a Pi warm resume seeds the relay's execution check from the folded approval", async () => { - // The Pi custom-tool double gate on the WARM path: the resume answers the held dialog via - // respondPermission (never through the responder, so nothing re-appends), and the relay's - // own execution check must still pass. The resume request folds the gated tool_call plus - // the {approved: true} envelope; the resume turn's real ConversationDecisions (built by - // runTurn from that history) is what the relay consults. Capture the REAL relayPermissions - // runTurn wires into startToolRelay and assert its decide() finds the allow. - const { calls, deps } = pausableHarness(); - let relayPermissions: - { enforce: boolean; decide: (gate: any) => { kind: string } } | undefined; - deps.startToolRelay = (( - _host: unknown, - _dir: unknown, - _specs: unknown, - _callback: unknown, - permissions: any, - ) => { - relayPermissions = permissions; - return { stop: async () => {} }; - }) as any; - - const resumeRequest: AgentRunRequest = { - harness: "pi_core", - customTools: [{ name: "park_probe", permission: "ask" }] as any, - toolCallback: { - endpoint: "http://callback", - authorization: "bearer", - } as any, - messages: [ - { role: "user", content: "do X" }, - { - role: "assistant", - content: [ - { - type: "tool_call", - toolCallId: "call_REAL", - toolName: "park_probe", - input: { token: "T" }, - }, - ], - }, - { - role: "user", - content: [ - { - type: "tool_result", - toolCallId: "call_REAL", - output: { approved: true }, - }, - ], - }, - ], - }; - const acquired = await acquireEnvironment(resumeRequest, deps); - assert.equal(acquired.ok, true); - if (!acquired.ok) return; - const env = acquired.env; - - // Drive the resume branch: answer the (already-parked) gate and continue the held prompt. - const result = await runTurn(env, resumeRequest, undefined, undefined, { - approvalParkMode: true, - resume: { - permissionId: "perm-1", - reply: "once", - toolCallId: "call_REAL", - toolName: "park_probe", - args: { token: "T" }, - interactionToken: "call_REAL", - promptPromise: Promise.resolve({ stopReason: "complete" }), - }, - }); - assert.equal(result.ok, true); - assert.deepEqual( - calls.permissionReplies, - [{ id: "perm-1", reply: "once" }], - "the held dialog was answered on the live session", - ); - assert.ok(relayPermissions, "the resume turn restarted the relay"); - assert.equal(relayPermissions!.enforce, true, "Pi: the relay enforces"); - // The relay's second gate on the resumed call finds the folded approval. - const verdict = relayPermissions!.decide({ - executor: "relay", - toolName: "park_probe", - specPermission: "ask", - args: { token: "T" }, - }); - assert.equal(verdict.kind, "allow"); - - await env.destroy(); - }); }); diff --git a/services/runner/tests/unit/tool-callref-bindings.test.ts b/services/runner/tests/unit/tool-callref-bindings.test.ts index d51b4ebaf8..bdff826da5 100644 --- a/services/runner/tests/unit/tool-callref-bindings.test.ts +++ b/services/runner/tests/unit/tool-callref-bindings.test.ts @@ -1,9 +1,9 @@ /** * Unit tests for callRef callback execution through the host relay. * - * These pin the test_run runner contract: contextBindings are applied only after the permission - * verdict and only in the callRef branch, timeoutMs reaches /tools/call, and runContext.run.kind - * is forwarded as x-agenta-run-kind. + * These pin the test_run runner contract: contextBindings are applied only in the callRef + * branch, timeoutMs reaches /tools/call, and runContext.run.kind is forwarded as + * x-agenta-run-kind. (The relay carries execution only; permission gates ride the ACP plane.) */ import { afterEach, describe, it } from "vitest"; import assert from "node:assert/strict"; @@ -21,7 +21,6 @@ import type { ResolvedToolSpec, RunContext } from "../../src/protocol.ts"; import { localRelayHost, startToolRelay, - type RelayPermissions, type RelayResponse, } from "../../src/tools/relay.ts"; @@ -36,8 +35,6 @@ interface CapturedFetch { init: RequestInit; } -type PendingInfo = { toolCallId: string; toolName: string; args: unknown }; - const realFetch = globalThis.fetch; const realTimeout = AbortSignal.timeout; @@ -55,33 +52,10 @@ function stubFetch(body = "ok"): CapturedFetch[] { return calls; } -function permissions(input: { - verdict?: "allow" | "deny" | "pendingApproval"; - seenArgs?: unknown[]; - pending?: PendingInfo[]; -} = {}): RelayPermissions { - return { - enforce: true, - decide: (gate) => { - input.seenArgs?.push(gate.args); - if (input.verdict === "deny") return { kind: "deny" }; - if (input.verdict === "pendingApproval") { - return { kind: "pendingApproval" }; - } - return { kind: "allow" }; - }, - onPendingApproval: (info) => { - input.pending?.push(info); - return { emitted: true }; - }, - }; -} - async function relayOnce(input: { spec: ResolvedToolSpec; args: unknown; runContext?: RunContext; - permissions?: RelayPermissions; expectResponse?: boolean; stopWhen?: () => boolean; }): Promise { @@ -90,14 +64,17 @@ async function relayOnce(input: { const id = "call-1"; writeFileSync( join(dir, `${id}.req.json`), - JSON.stringify({ toolName: input.spec.name, toolCallId: id, args: input.args }), + JSON.stringify({ + toolName: input.spec.name, + toolCallId: id, + args: input.args, + }), ); const relay = startToolRelay( localRelayHost(), dir, [input.spec], { endpoint: ENDPOINT, authorization: "ApiKey secret" }, - input.permissions ?? permissions(), input.runContext, ); const resPath = join(dir, `${id}.res.json`); @@ -109,7 +86,11 @@ async function relayOnce(input: { await relay.stop(); const wroteResponse = existsSync(resPath); if (input.expectResponse === false) { - assert.equal(wroteResponse, false, "the relay did not write a response file"); + assert.equal( + wroteResponse, + false, + "the relay did not write a response file", + ); return undefined; } assert.ok(wroteResponse, "the relay wrote a response file"); @@ -119,7 +100,9 @@ async function relayOnce(input: { } } -function callRefSpec(overrides: Partial = {}): ResolvedToolSpec { +function callRefSpec( + overrides: Partial = {}, +): ResolvedToolSpec { return { name: "test_run", kind: "callback", @@ -133,9 +116,8 @@ function callRefSpec(overrides: Partial = {}): ResolvedToolSpe } describe("startToolRelay callRef context bindings", () => { - it("applies bindings after the allow verdict and lets bindings override model args", async () => { + it("applies bindings in the callRef branch and lets bindings override model args", async () => { const calls = stubFetch(); - const seenArgs: unknown[] = []; const res = await relayOnce({ spec: callRefSpec(), @@ -144,17 +126,10 @@ describe("startToolRelay callRef context bindings", () => { inputs: { city: "Berlin" }, }, runContext: RUN_CONTEXT, - permissions: permissions({ seenArgs }), }); assert.equal(res?.ok, true); assert.equal(calls.length, 1); - assert.deepEqual(seenArgs, [ - { - target: { workflow_variant_id: "model-variant" }, - inputs: { city: "Berlin" }, - }, - ]); const posted = JSON.parse(calls[0].init.body as string); assert.deepEqual(posted.data.function, { name: "tools.agenta.test_run", @@ -182,47 +157,13 @@ describe("startToolRelay callRef context bindings", () => { assert.equal(calls.length, 0); }); - it("does not bind or fetch when the permission verdict denies", async () => { - const calls = stubFetch(); - - const res = await relayOnce({ - spec: callRefSpec({ permission: "deny" }), - args: { target: { workflow_variant_id: "model-variant" } }, - runContext: { run: { kind: "test" } }, - permissions: permissions({ verdict: "deny" }), - }); - - assert.equal(res?.ok, true); - assert.equal(res?.text, "Tool 'test_run' is denied by policy."); - assert.equal(calls.length, 0); - }); - - it("redacts bound args from pending display but still binds them on execution", async () => { + it("binds context values on execution", async () => { const calls = stubFetch(); - const pending: PendingInfo[] = []; const args = { target: { workflow_variant_id: "model-variant" }, inputs: { city: "Berlin" }, }; - await relayOnce({ - spec: callRefSpec(), - args, - runContext: { run: { kind: "test" } }, - permissions: permissions({ verdict: "pendingApproval", pending }), - expectResponse: false, - stopWhen: () => pending.length === 1, - }); - - assert.deepEqual(pending, [ - { - toolCallId: "call-1", - toolName: "test_run", - args: { inputs: { city: "Berlin" } }, - }, - ]); - assert.equal(calls.length, 0); - const res = await relayOnce({ spec: callRefSpec(), args, diff --git a/services/runner/tests/unit/tool-direct.test.ts b/services/runner/tests/unit/tool-direct.test.ts index 646efda9a9..3b3f4bd5e2 100644 --- a/services/runner/tests/unit/tool-direct.test.ts +++ b/services/runner/tests/unit/tool-direct.test.ts @@ -579,11 +579,6 @@ async function relayOnce( dir, [spec], callback, - { - enforce: false, - decide: () => ({ kind: "allow" }), - onPendingApproval: () => ({ emitted: false }), - }, runContext, ); const resPath = join(dir, `${id}.res.json`); diff --git a/services/runner/tests/unit/tool-dispatch-permission.test.ts b/services/runner/tests/unit/tool-dispatch-permission.test.ts deleted file mode 100644 index 4c60a447ab..0000000000 --- a/services/runner/tests/unit/tool-dispatch-permission.test.ts +++ /dev/null @@ -1,144 +0,0 @@ -/** - * Unit tests for the Pi builtin permission relay helper. - * - * Run: pnpm test (or: pnpm exec vitest run tests/unit/tool-dispatch-permission.test.ts) - */ -import { afterEach, describe, it, vi } from "vitest"; -import assert from "node:assert/strict"; -import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; - -const dirs: string[] = []; - -function tempDir(): string { - const dir = mkdtempSync(join(tmpdir(), "agenta-permission-relay-test-")); - dirs.push(dir); - return dir; -} - -afterEach(() => { - vi.unstubAllEnvs(); - for (const dir of dirs.splice(0)) { - rmSync(dir, { recursive: true, force: true }); - } -}); - -async function waitForFile(path: string): Promise { - const deadline = Date.now() + 1000; - while (Date.now() < deadline) { - if (existsSync(path)) return; - await new Promise((resolve) => setTimeout(resolve, 5)); - } - throw new Error(`timed out waiting for ${path}`); -} - -describe("relayPermissionCheck", () => { - it("round-trips a permission request and parses the response", async () => { - const { relayPermissionCheck } = await import("../../src/tools/dispatch.ts"); - const { - RELAY_PERMISSION_PROTOCOL, - RELAY_REQ_SUFFIX, - RELAY_RES_SUFFIX, - sanitizeRelayId, - } = await import("../../src/tools/relay.ts"); - const dir = tempDir(); - const toolCallId = "call/bash:1"; - const id = sanitizeRelayId(toolCallId); - const reqPath = join(dir, `${id}${RELAY_REQ_SUFFIX}`); - const resPath = join(dir, `${id}${RELAY_RES_SUFFIX}`); - - const pending = relayPermissionCheck(dir, "bash", toolCallId, { - command: "npm test", - }); - await waitForFile(reqPath); - - assert.deepEqual(JSON.parse(readFileSync(reqPath, "utf-8")), { - kind: "permission", - protocol: RELAY_PERMISSION_PROTOCOL, - toolName: "bash", - toolCallId, - args: { command: "npm test" }, - }); - - writeFileSync( - resPath, - JSON.stringify({ kind: "permission", ok: true, verdict: "allow" }), - "utf-8", - ); - - assert.deepEqual(await pending, { - kind: "permission", - ok: true, - verdict: "allow", - }); - }); - - it("fails closed on an unparseable response", async () => { - const { relayPermissionCheck } = await import("../../src/tools/dispatch.ts"); - const { RELAY_RES_SUFFIX, sanitizeRelayId } = await import( - "../../src/tools/relay.ts" - ); - const dir = tempDir(); - const toolCallId = "call-unparseable"; - writeFileSync( - join(dir, `${sanitizeRelayId(toolCallId)}${RELAY_RES_SUFFIX}`), - "not json", - "utf-8", - ); - - const response = await relayPermissionCheck(dir, "write", toolCallId, { - file_path: "x", - }); - - assert.equal(response.kind, "permission"); - assert.equal(response.ok, false); - assert.equal(response.verdict, "deny"); - assert.match(response.reason ?? "", /unparseable/); - }); - - it("fails closed when the runner responds ok:false", async () => { - const { relayPermissionCheck } = await import("../../src/tools/dispatch.ts"); - const { RELAY_RES_SUFFIX, sanitizeRelayId } = await import( - "../../src/tools/relay.ts" - ); - const dir = tempDir(); - const toolCallId = "call-failed"; - writeFileSync( - join(dir, `${sanitizeRelayId(toolCallId)}${RELAY_RES_SUFFIX}`), - JSON.stringify({ - kind: "permission", - ok: false, - verdict: "allow", - reason: "runner failed\nwhile deciding", - }), - "utf-8", - ); - - const response = await relayPermissionCheck(dir, "edit", toolCallId, {}); - - assert.deepEqual(response, { - kind: "permission", - ok: false, - verdict: "deny", - reason: "runner failed while deciding", - }); - }); - - it("fails closed on timeout", async () => { - vi.resetModules(); - vi.stubEnv("AGENTA_AGENT_TOOLS_RELAY_TIMEOUT", "10"); - vi.stubEnv("AGENTA_AGENT_TOOLS_RELAY_POLLING", "1"); - const { relayPermissionCheck } = await import("../../src/tools/dispatch.ts"); - const dir = tempDir(); - - const response = await relayPermissionCheck(dir, "read", "call-timeout", { - file_path: "README.md", - }); - - assert.equal(response.kind, "permission"); - assert.equal(response.ok, false); - assert.equal(response.verdict, "deny"); - assert.match(response.reason ?? "", /timed out/); - }); -}); diff --git a/services/runner/tests/unit/tool-relay-permission-parity.test.ts b/services/runner/tests/unit/tool-relay-permission-parity.test.ts deleted file mode 100644 index e9488643f5..0000000000 --- a/services/runner/tests/unit/tool-relay-permission-parity.test.ts +++ /dev/null @@ -1,192 +0,0 @@ -/** - * Phase 4 (docs/design/agent-workflows/projects/pi-builtin-gating/plan.md): parity and - * regression-pin tests for the relay's builtin-gating seam. - * - * Parity: a builtin `bash` pending (a `kind: "permission"` record, handled by - * `handlePermissionRelayRequest`) and a custom relay tool's `ask` pending (a `kind: "execute"` - * record, handled by `executeRelayedTool`) are two different code paths inside - * `startToolRelay`, but both route through the SAME `onPendingApproval` callback with the SAME - * `{toolCallId, toolName, args}` shape. A caller downstream of the relay (the responder / SSE - * pause plumbing) can treat every pending pause uniformly, regardless of which path produced - * it — the engine wraps both identically. - * - * Regression pin (0e71bd0f7a): a run whose `tools` omits `bash` must not grant it, both at the - * `RunPlan` layer (`buildRunPlan`) and at the extension's active-tool-set layer - * (`replaceActiveBuiltinTools`). This bug shipped once as a silently-dropped grant list; pin it - * at both layers so it cannot recur unnoticed at either one. - * - * Run: pnpm test (or: pnpm exec vitest run tests/unit/tool-relay-permission-parity.test.ts) - */ -import { describe, it } from "vitest"; -import assert from "node:assert/strict"; -import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; - -import { - localRelayHost, - RELAY_PERMISSION_PROTOCOL, - startToolRelay, - type RelayPermissions, -} from "../../src/tools/relay.ts"; -import type { AgentRunRequest, ResolvedToolSpec } from "../../src/protocol.ts"; -import { decide, type PermissionPlan } from "../../src/permission-plan.ts"; -import { ConversationDecisions } from "../../src/responder.ts"; -import { buildRunPlan } from "../../src/engines/sandbox_agent/run-plan.ts"; -import { replaceActiveBuiltinTools } from "../../src/extensions/agenta.ts"; - -type PendingInfo = { toolCallId: string; toolName: string; args: unknown }; - -function askPlan(): PermissionPlan { - return { default: "ask", rules: [] }; -} - -/** A RelayPermissions whose `decide` always resolves through an `ask` default plan, and whose - * `onPendingApproval` appends every call it receives to `pending` verbatim. */ -function collectingPermissions(pending: PendingInfo[]): RelayPermissions { - const decisions = new ConversationDecisions(new Map()); - const plan = askPlan(); - return { - enforce: true, - decide: (gate) => decide(gate, plan, decisions), - onPendingApproval: (info) => { - pending.push(info); - return { emitted: true }; - }, - }; -} - -/** Write one relay request record, start the relay, and wait until `onPendingApproval` has - * fired (or the deadline passes), then stop the relay. */ -async function relayUntilPending(input: { - record: Record; - permissions: RelayPermissions; - pending: PendingInfo[]; - specs?: ResolvedToolSpec[]; -}): Promise { - const dir = mkdtempSync(join(tmpdir(), "agenta-relay-parity-")); - try { - const id = - typeof input.record.toolCallId === "string" - ? input.record.toolCallId - : "call-1"; - writeFileSync(join(dir, `${id}.req.json`), JSON.stringify(input.record)); - const relay = startToolRelay( - localRelayHost(), - dir, - input.specs ?? [], - undefined, - input.permissions, - ); - const deadline = Date.now() + 5000; - while (Date.now() < deadline && input.pending.length === 0) { - await new Promise((resolve) => setTimeout(resolve, 20)); - } - await relay.stop(); - } finally { - rmSync(dir, { recursive: true, force: true }); - } -} - -describe("onPendingApproval parity: builtin permission record vs custom relay tool", () => { - it("delivers the same {toolCallId, toolName, args} shape from both pending paths", async () => { - const builtinPending: PendingInfo[] = []; - await relayUntilPending({ - record: { - kind: "permission", - protocol: RELAY_PERMISSION_PROTOCOL, - toolName: "bash", - toolCallId: "builtin-call-1", - args: { command: "npm test" }, - }, - permissions: collectingPermissions(builtinPending), - pending: builtinPending, - }); - - const customPending: PendingInfo[] = []; - const customSpec: ResolvedToolSpec = { - name: "send_email", - kind: "code", - runtime: "python", - code: "def main(**kw):\n return kw\n", - permission: "ask", - }; - await relayUntilPending({ - record: { - toolName: "send_email", - toolCallId: "custom-call-1", - args: { to: "a@b.com" }, - }, - permissions: collectingPermissions(customPending), - pending: customPending, - specs: [customSpec], - }); - - assert.equal(builtinPending.length, 1, "the builtin path paused exactly once"); - assert.equal(customPending.length, 1, "the custom-tool path paused exactly once"); - - const [builtin] = builtinPending; - const [custom] = customPending; - - // Same seam, same keys: neither path leaks extra fields (e.g. a `kind` discriminator) into - // the callback, and neither drops one of the three. - assert.deepEqual(Object.keys(builtin).sort(), ["args", "toolCallId", "toolName"]); - assert.deepEqual(Object.keys(custom).sort(), ["args", "toolCallId", "toolName"]); - - assert.deepEqual(builtin, { - toolCallId: "builtin-call-1", - toolName: "bash", - args: { command: "npm test" }, - }); - assert.deepEqual(custom, { - toolCallId: "custom-call-1", - toolName: "send_email", - args: { to: "a@b.com" }, - }); - }); -}); - -describe("grant-list regression pin (0e71bd0f7a)", () => { - it("buildRunPlan excludes bash from builtinGrants and turns gating on when `tools` omits it", () => { - const result = buildRunPlan( - { - harness: "pi_core", - messages: [{ role: "user", content: "hello" }], - tools: ["read"], - } as AgentRunRequest, - { createLocalCwd: () => "/tmp/local-cwd" }, - ); - - assert.equal(result.ok, true); - if (!result.ok) return; - assert.deepEqual(result.plan.builtinGrants, ["read"]); - assert.ok( - !result.plan.builtinGrants.includes("bash"), - "bash must not be silently re-granted", - ); - assert.equal(result.plan.builtinGatingActive, true); - }); - - it("replaceActiveBuiltinTools drops bash/edit/write and keeps read when only read is granted", () => { - const allTools = [ - { name: "read" }, - { name: "bash" }, - { name: "edit" }, - { name: "write" }, - { name: "grep" }, - { name: "find" }, - { name: "ls" }, - ]; - - const next = replaceActiveBuiltinTools( - ["read", "bash", "edit", "write"], - allTools, - ["read"], - ); - - assert.deepEqual(next, ["read"]); - assert.ok(!next.includes("bash")); - assert.ok(!next.includes("edit")); - assert.ok(!next.includes("write")); - }); -}); diff --git a/services/runner/tests/unit/tool-relay-permission-record.test.ts b/services/runner/tests/unit/tool-relay-permission-record.test.ts deleted file mode 100644 index 6beff38918..0000000000 --- a/services/runner/tests/unit/tool-relay-permission-record.test.ts +++ /dev/null @@ -1,400 +0,0 @@ -/** - * Unit tests for builtin permission records on the runner relay. - * - * Run: pnpm test (or: pnpm exec vitest run tests/unit/tool-relay-permission-record.test.ts) - */ -import { describe, it } from "vitest"; -import assert from "node:assert/strict"; -import { - existsSync, - mkdtempSync, - readFileSync, - rmSync, - writeFileSync, -} from "node:fs"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; - -import { - localRelayHost, - parsePermissionRelayResponse, - RELAY_PERMISSION_PROTOCOL, - startToolRelay, - type PermissionRelayResponse, - type RelayPermissions, - type RelayResponse, -} from "../../src/tools/relay.ts"; -import type { ResolvedToolSpec } from "../../src/protocol.ts"; -import { - decide, - type GateDescriptor, - type PermissionPlan, -} from "../../src/permission-plan.ts"; -import { approvedCallKey, ConversationDecisions } from "../../src/responder.ts"; - -const codeSpec: ResolvedToolSpec = { - name: "server_tool", - kind: "code", - runtime: "python", - code: "def main(**kw):\n return kw\n", -}; - -function permissionPlan( - defaultMode: PermissionPlan["default"], - rules: PermissionPlan["rules"] = [], -): PermissionPlan { - return { default: defaultMode, rules }; -} - -function permissions( - input: { - plan?: PermissionPlan; - decisions?: ConversationDecisions; - pending?: Array<{ toolCallId: string; toolName: string; args: unknown }>; - gates?: GateDescriptor[]; - pendingEmitted?: boolean; - } = {}, -): RelayPermissions { - const plan = input.plan ?? permissionPlan("allow"); - const decisions = input.decisions ?? new ConversationDecisions(new Map()); - return { - enforce: true, - decide: (gate) => { - input.gates?.push(gate); - return decide(gate, plan, decisions); - }, - onPendingApproval: (info) => { - input.pending?.push(info); - return { emitted: input.pendingEmitted ?? true }; - }, - }; -} - -function permissionRecord( - toolName: string, - args: unknown = { command: "pwd" }, -): Record { - return { - kind: "permission", - protocol: RELAY_PERMISSION_PROTOCOL, - toolName, - toolCallId: "call-1", - args, - }; -} - -async function relayRecordOnce(input: { - record: Record; - permissions: RelayPermissions; - specs?: ResolvedToolSpec[]; -}): Promise { - const dir = mkdtempSync(join(tmpdir(), "agenta-relay-permission-record-")); - try { - const id = - typeof input.record.toolCallId === "string" - ? input.record.toolCallId - : "call-1"; - writeFileSync(join(dir, `${id}.req.json`), JSON.stringify(input.record)); - const relay = startToolRelay( - localRelayHost(), - dir, - input.specs ?? [], - undefined, - input.permissions, - ); - const resPath = join(dir, `${id}.res.json`); - const deadline = Date.now() + 5000; - while (Date.now() < deadline && !existsSync(resPath)) { - await new Promise((resolve) => setTimeout(resolve, 20)); - } - await relay.stop(); - assert.ok(existsSync(resPath), "the relay wrote a response file"); - return JSON.parse(readFileSync(resPath, "utf-8")) as unknown; - } finally { - rmSync(dir, { recursive: true, force: true }); - } -} - -async function permissionRelayOnce(input: { - record: Record; - permissions: RelayPermissions; -}): Promise { - const raw = await relayRecordOnce(input); - const parsed = parsePermissionRelayResponse(raw); - assert.ok(parsed, `permission response validated: ${JSON.stringify(raw)}`); - return parsed; -} - -describe("parsePermissionRelayResponse", () => { - it("accepts permission responses and rejects execute-shaped records", () => { - assert.deepEqual( - parsePermissionRelayResponse({ - kind: "permission", - ok: true, - verdict: "allow", - }), - { kind: "permission", ok: true, verdict: "allow" }, - ); - assert.equal( - parsePermissionRelayResponse({ ok: true, text: "{}" }), - undefined, - ); - }); -}); - -describe("startToolRelay builtin permission records", () => { - it("writes an allow permission response under an all-allow policy", async () => { - const pending: Array<{ - toolCallId: string; - toolName: string; - args: unknown; - }> = []; - const gates: GateDescriptor[] = []; - - const res = await permissionRelayOnce({ - record: permissionRecord("bash", { command: "pwd" }), - permissions: permissions({ - plan: permissionPlan("allow"), - pending, - gates, - }), - }); - - assert.deepEqual(res, { kind: "permission", ok: true, verdict: "allow" }); - assert.deepEqual(pending, []); - assert.deepEqual(gates, [ - { - executor: "harness", - toolName: "Bash", - readOnlyHint: false, - args: { command: "pwd" }, - }, - ]); - }); - - it("writes a deny permission response with the relay policy wording", async () => { - const res = await permissionRelayOnce({ - record: permissionRecord("bash", { command: "rm -rf /tmp/nope" }), - permissions: permissions({ plan: permissionPlan("deny") }), - }); - - assert.deepEqual(res, { - kind: "permission", - ok: true, - verdict: "deny", - reason: "Tool 'bash' is denied by the permission policy.", - }); - }); - - it("calls onPendingApproval and writes pendingApproval verbatim", async () => { - const pending: Array<{ - toolCallId: string; - toolName: string; - args: unknown; - }> = []; - - const res = await permissionRelayOnce({ - record: permissionRecord("bash", { command: "npm test" }), - permissions: permissions({ - plan: permissionPlan("ask"), - pending, - }), - }); - - assert.deepEqual(pending, [ - { toolCallId: "call-1", toolName: "bash", args: { command: "npm test" } }, - ]); - assert.deepEqual(res, { - kind: "permission", - ok: true, - verdict: "pendingApproval", - reason: "Waiting for approval of bash.", - }); - }); - - it("writes the another-approval reason when the pending latch is held", async () => { - const pending: Array<{ - toolCallId: string; - toolName: string; - args: unknown; - }> = []; - - const res = await permissionRelayOnce({ - record: permissionRecord("write", { path: "a.txt", content: "x" }), - permissions: permissions({ - plan: permissionPlan("ask"), - pending, - pendingEmitted: false, - }), - }); - - assert.deepEqual(pending, [ - { - toolCallId: "call-1", - toolName: "write", - args: { path: "a.txt", content: "x" }, - }, - ]); - assert.deepEqual(res, { - kind: "permission", - ok: true, - verdict: "pendingApproval", - reason: "Another approval is pending; retry after it resolves.", - }); - }); - - it("allows read builtins under allow_reads and asks for write builtins", async () => { - const pending: Array<{ - toolCallId: string; - toolName: string; - args: unknown; - }> = []; - const relayPermissions = permissions({ - plan: permissionPlan("allow_reads"), - pending, - }); - - const grep = await permissionRelayOnce({ - record: permissionRecord("grep", { pattern: "TODO", path: "." }), - permissions: relayPermissions, - }); - assert.deepEqual(grep, { kind: "permission", ok: true, verdict: "allow" }); - - const write = await permissionRelayOnce({ - record: permissionRecord("write", { path: "a.txt", content: "x" }), - permissions: relayPermissions, - }); - assert.equal(write.verdict, "pendingApproval"); - assert.deepEqual(pending, [ - { - toolCallId: "call-1", - toolName: "write", - args: { path: "a.txt", content: "x" }, - }, - ]); - }); - - it("matches Bash prefix rules on the real command arg after name normalization", async () => { - const gates: GateDescriptor[] = []; - - const res = await permissionRelayOnce({ - record: permissionRecord("bash", { command: "git status" }), - permissions: permissions({ - plan: permissionPlan("deny", [ - { pattern: "Bash(git:*)", permission: "allow" }, - ]), - gates, - }), - }); - - assert.deepEqual(res, { kind: "permission", ok: true, verdict: "allow" }); - assert.deepEqual(gates[0], { - executor: "harness", - toolName: "Bash", - readOnlyHint: false, - args: { command: "git status" }, - }); - }); - - it("projects stored bash approvals by command but keeps write approvals exact", async () => { - const bashKey = approvedCallKey("bash", { command: "npm test" })!; - const writeKey = approvedCallKey("write", { - path: "a.txt", - content: "old", - })!; - const pending: Array<{ - toolCallId: string; - toolName: string; - args: unknown; - }> = []; - const relayPermissions = permissions({ - plan: permissionPlan("ask"), - decisions: new ConversationDecisions( - new Map([ - [bashKey, "allow"], - [writeKey, "allow"], - ]), - ), - pending, - }); - - const bash = await permissionRelayOnce({ - record: permissionRecord("bash", { command: "npm test", timeout: 10 }), - permissions: relayPermissions, - }); - assert.deepEqual(bash, { kind: "permission", ok: true, verdict: "allow" }); - - const write = await permissionRelayOnce({ - record: permissionRecord("write", { path: "a.txt", content: "new" }), - permissions: relayPermissions, - }); - assert.equal(write.verdict, "pendingApproval"); - assert.deepEqual(pending, [ - { - toolCallId: "call-1", - toolName: "write", - args: { path: "a.txt", content: "new" }, - }, - ]); - }); - - it("fails closed on missing or unknown permission protocol versions", async () => { - for (const record of [ - { kind: "permission", toolName: "bash", toolCallId: "call-1", args: {} }, - { - kind: "permission", - protocol: 999, - toolName: "bash", - toolCallId: "call-1", - args: {}, - }, - ]) { - const res = await permissionRelayOnce({ - record, - permissions: permissions({ plan: permissionPlan("allow") }), - }); - - assert.equal(res.kind, "permission"); - assert.equal(res.ok, true); - assert.equal(res.verdict, "deny"); - assert.match(res.reason ?? "", /runner\/extension version mismatch/); - } - }); - - it("fails closed on an unknown builtin without calling decide", async () => { - const gates: GateDescriptor[] = []; - - const res = await permissionRelayOnce({ - record: permissionRecord("cat", { path: "a.txt" }), - permissions: permissions({ plan: permissionPlan("allow"), gates }), - }); - - assert.deepEqual(gates, []); - assert.deepEqual(res, { - kind: "permission", - ok: true, - verdict: "deny", - reason: "Tool 'cat' is denied by the permission policy.", - }); - }); - - it("leaves execute records with no kind on the existing relay path", async () => { - const raw = await relayRecordOnce({ - record: { toolName: "server_tool", toolCallId: "call-1", args: { a: 1 } }, - permissions: { - enforce: false, - decide: () => ({ kind: "allow" }), - onPendingApproval: () => ({ emitted: false }), - }, - specs: [codeSpec], - }); - const res = raw as RelayResponse; - - assert.equal("kind" in (raw as Record), false); - assert.equal(res.ok, false); - assert.match( - res.error ?? "", - /Code tools are not supported by the sidecar\./, - ); - }); -}); diff --git a/services/runner/tests/unit/tool-relay-permission.test.ts b/services/runner/tests/unit/tool-relay-permission.test.ts deleted file mode 100644 index c54c81c342..0000000000 --- a/services/runner/tests/unit/tool-relay-permission.test.ts +++ /dev/null @@ -1,318 +0,0 @@ -/** - * Unit tests for runner-side relay permission enforcement. - * - * Run: pnpm test (or: pnpm exec vitest run tests/unit/tool-relay-permission.test.ts) - */ -import { describe, it } from "vitest"; -import assert from "node:assert/strict"; -import { - existsSync, - mkdtempSync, - readFileSync, - rmSync, - writeFileSync, -} from "node:fs"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; - -import { - localRelayHost, - RELAY_POLL_MAX_MS, - RELAY_POLL_MS, - relayPollDelayMs, - startToolRelay, - type ClientToolRelay, - type RelayPermissions, - type RelayResponse, -} from "../../src/tools/relay.ts"; -import type { ResolvedToolSpec } from "../../src/protocol.ts"; -import { decide, type PermissionPlan } from "../../src/permission-plan.ts"; -import { approvedCallKey, 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"], - readOnly?: boolean, -): ResolvedToolSpec => ({ - name, - kind: "code", - runtime: "python", - code: 'def main(**kw):\n return {"ran": True, "echo": kw}\n', - permission, - readOnly, -}); - -function permissionPlan( - defaultMode: PermissionPlan["default"], -): PermissionPlan { - return { default: defaultMode, rules: [] }; -} - -function permissions(input: { - enforce: boolean; - plan?: PermissionPlan; - decisions?: ConversationDecisions; - pending?: Array<{ toolCallId: string; toolName: string; args: unknown }>; -}): RelayPermissions { - const plan = input.plan ?? permissionPlan("allow"); - const decisions = input.decisions ?? new ConversationDecisions(new Map()); - return { - enforce: input.enforce, - decide: (gate) => decide(gate, plan, decisions), - onPendingApproval: (info) => { - input.pending?.push(info); - return { emitted: true }; - }, - }; -} - -async function relayOnce(input: { - spec: ResolvedToolSpec; - permissions: RelayPermissions; - args?: unknown; - id?: string; - expectResponse?: boolean; - stopWhen?: () => boolean; - clientToolRelay?: ClientToolRelay; -}): Promise { - const dir = mkdtempSync(join(tmpdir(), "agenta-relay-perm-")); - try { - const id = input.id ?? "call-1"; - writeFileSync( - join(dir, `${id}.req.json`), - JSON.stringify({ - toolName: input.spec.name, - toolCallId: id, - args: input.args ?? { a: 1 }, - }), - ); - const relay = startToolRelay( - localRelayHost(), - dir, - [input.spec], - undefined, - input.permissions, - undefined, - input.clientToolRelay, - ); - const resPath = join(dir, `${id}.res.json`); - const deadline = Date.now() + 5000; - while (Date.now() < deadline && !existsSync(resPath)) { - if (input.stopWhen?.()) break; - await new Promise((resolve) => setTimeout(resolve, 20)); - } - await relay.stop(); - const wroteResponse = existsSync(resPath); - if (input.expectResponse === false) { - assert.equal( - wroteResponse, - false, - "the relay did not write a response file", - ); - return undefined; - } - assert.ok(wroteResponse, "the relay wrote a response file"); - return JSON.parse(readFileSync(resPath, "utf-8")) as RelayResponse; - } finally { - rmSync(dir, { recursive: true, force: true }); - } -} - -function assertCodeToolExecuted(res: RelayResponse | undefined): void { - assert.equal(res?.ok, false); - assert.match( - res?.error ?? "", - /Code tools are not supported by the sidecar\./, - ); -} - -describe("startToolRelay permission enforcement", () => { - it("enforce=false executes an ask spec without pausing", async () => { - const pending: Array<{ - toolCallId: string; - toolName: string; - args: unknown; - }> = []; - const res = await relayOnce({ - spec: codeSpec("needs_approval", "ask"), - permissions: permissions({ - enforce: false, - plan: permissionPlan("ask"), - pending, - }), - }); - - assertCodeToolExecuted(res); - assert.deepEqual(pending, []); - }); - - it("enforce=true allows allowed tools and refuses authored deny distinctly", async () => { - const pending: Array<{ - toolCallId: string; - toolName: string; - args: unknown; - }> = []; - const allow = await relayOnce({ - spec: codeSpec("permitted", "allow"), - permissions: permissions({ - enforce: true, - plan: permissionPlan("ask"), - pending, - }), - }); - assertCodeToolExecuted(allow); - - const deny = await relayOnce({ - spec: codeSpec("blocked", "deny"), - permissions: permissions({ - enforce: true, - plan: permissionPlan("allow"), - pending, - }), - }); - assert.equal(deny?.ok, true); - assert.equal(deny?.text, "Tool 'blocked' is denied by policy."); - assert.deepEqual(pending, []); - }); - - it("enforce=true refuses policy deny with the permission-policy text", async () => { - const res = await relayOnce({ - spec: codeSpec("locked_down"), - permissions: permissions({ enforce: true, plan: permissionPlan("deny") }), - }); - - assert.equal(res?.ok, true); - assert.equal( - res?.text, - "Tool 'locked_down' is denied by the permission policy.", - ); - }); - - it("ask with no stored decision pauses without writing a response or executing", async () => { - const pending: Array<{ - toolCallId: string; - toolName: string; - args: unknown; - }> = []; - await relayOnce({ - spec: codeSpec("approval_needed", "ask"), - permissions: permissions({ - enforce: true, - plan: permissionPlan("allow"), - pending, - }), - expectResponse: false, - stopWhen: () => pending.length === 1, - }); - - assert.deepEqual(pending, [ - { toolCallId: "call-1", toolName: "approval_needed", args: { a: 1 } }, - ]); - }); - - it("ask with a stored allow executes once and consumes the stored decision", async () => { - const key = approvedCallKey("approval_needed", { a: 1 })!; - const pending: Array<{ - toolCallId: string; - toolName: string; - args: unknown; - }> = []; - const relayPermissions = permissions({ - enforce: true, - plan: permissionPlan("allow"), - decisions: new ConversationDecisions(new Map([[key, "allow"]])), - pending, - }); - - const first = await relayOnce({ - id: "call-1", - spec: codeSpec("approval_needed", "ask"), - permissions: relayPermissions, - }); - assertCodeToolExecuted(first); - assert.deepEqual(pending, []); - - await relayOnce({ - id: "call-2", - spec: codeSpec("approval_needed", "ask"), - permissions: relayPermissions, - expectResponse: false, - stopWhen: () => pending.length === 1, - }); - assert.deepEqual(pending, [ - { toolCallId: "call-2", toolName: "approval_needed", args: { a: 1 } }, - ]); - }); - - it("allow_reads executes read-hinted tools and pauses tools without a read hint", async () => { - const pending: Array<{ - toolCallId: string; - toolName: string; - args: unknown; - }> = []; - const relayPermissions = permissions({ - enforce: true, - plan: permissionPlan("allow_reads"), - pending, - }); - - const read = await relayOnce({ - id: "read-call", - spec: codeSpec("read_tool", undefined, true), - permissions: relayPermissions, - }); - assertCodeToolExecuted(read); - - await relayOnce({ - id: "write-call", - spec: codeSpec("write_tool"), - permissions: relayPermissions, - expectResponse: false, - stopWhen: () => pending.length === 1, - }); - assert.deepEqual(pending, [ - { toolCallId: "write-call", toolName: "write_tool", args: { a: 1 } }, - ]); - }); - - it("client tools use pendingApproval to park without writing a relay response", async () => { - const parked: string[] = []; - await relayOnce({ - spec: { name: "request_connection", kind: "client" }, - permissions: permissions({ enforce: true }), - args: { integration: "slack" }, - expectResponse: false, - stopWhen: () => parked.length === 1, - clientToolRelay: { - onClientTool: async () => "pendingApproval", - onPause: (request) => { - parked.push(request.toolCallId); - }, - }, - }); - - assert.deepEqual(parked, ["call-1"]); - }); -}); From 3606e5d5cb2c91189cae25bc0c0dc7e8f3bee4dc Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Fri, 10 Jul 2026 11:33:06 +0200 Subject: [PATCH 3/3] fix(runner): re-check relay execute records runner-side; redact context-bound args from Pi approval gates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the parking change (codex P1/P2): P1: the relay dir is sandbox-writable, so an execute record forged with bash bypassed the in-sandbox dialog gate entirely once the relay's permission plane was deleted. The relay now re-checks every record runner-side (Pi runs only): author-allow executes, author-deny never does, and an ask tool executes only by consuming a per-turn grant recorded when the dialog gate (or a parked-approval resume) resolved that exact call to allow. Grants are keyed on approvedCallKey(name, args) with a count, so a forged or replayed record for an ask tool fails closed. P2: for a callRef tool with contextBindings, the approval card and the stored-decision key carried the model's values for bound paths the runner overwrites from runContext at execution — the human approved args that never run. buildPiGateDescriptor now strips bound paths (restored redaction helper), and the grant/consume sides key on the same redacted shape. Also: validate-before-dialog extension test, pi-coding-agent execute-arity contract comment, guard wiring pinned in orchestration tests. Claude-Session: https://claude.ai/code/session_012EjviaDzVYboubuwgESDwS --- services/runner/src/engines/sandbox_agent.ts | 68 ++++- .../engines/sandbox_agent/acp-interactions.ts | 38 ++- services/runner/src/extensions/agenta.ts | 2 + services/runner/src/responder.ts | 28 ++ services/runner/src/tools/relay.ts | 79 ++++++ .../runner/tests/unit/extension-tools.test.ts | 31 ++ .../tests/unit/pi-gate-envelope.test.ts | 29 ++ .../sandbox-agent-acp-interactions.test.ts | 128 +++++++++ .../unit/sandbox-agent-orchestration.test.ts | 7 +- .../tests/unit/tool-relay-guard.test.ts | 267 ++++++++++++++++++ 10 files changed, 665 insertions(+), 12 deletions(-) create mode 100644 services/runner/tests/unit/tool-relay-guard.test.ts diff --git a/services/runner/src/engines/sandbox_agent.ts b/services/runner/src/engines/sandbox_agent.ts index ac9d0db8af..eb2aa3bc50 100644 --- a/services/runner/src/engines/sandbox_agent.ts +++ b/services/runner/src/engines/sandbox_agent.ts @@ -42,11 +42,14 @@ import { } from "../tracing/otel.ts"; import { localRelayHost, + redactContextBoundArgs, sandboxRelayHost, startToolRelay, + type RelayExecutionGuard, } from "../tools/relay.ts"; import { ApprovalResponder, + ApprovedExecutionGrants, ConversationDecisions, extractApprovalDecisions, extractClientToolOutputs, @@ -89,6 +92,7 @@ import { writeOtlpAuthFile, } from "./sandbox_agent/pi-assets.ts"; import { + decide, PendingApprovalLatch, permissionsFromRequest, } from "../permission-plan.ts"; @@ -1225,6 +1229,10 @@ export async function runTurn( storedDecisionMap, extractClientToolOutputs(request), ); + const executionGrants = new ApprovedExecutionGrants(); + // The guard's decide() must never consume this turn's stored decisions — the DIALOG is their + // consumer (it runs first). An empty store makes every `ask` route to the grant ledger. + const relayGuardDecisions = new ConversationDecisions(new Map()); const latch = new PendingApprovalLatch(); const responder = deps.responderFactory?.(request) ?? @@ -1293,10 +1301,22 @@ export async function runTurn( ? new Map( plan.toolSpecs.map((spec) => [ spec.name, - { permission: spec.permission, readOnly: spec.readOnly }, + { + permission: spec.permission, + readOnly: spec.readOnly, + // callRef tools only: bound paths are runner-filled at execution, so the + // approval card and decision keys must not carry the model's values for them. + contextBindings: spec.callRef + ? spec.contextBindings + : undefined, + }, ]), ) : undefined, + // A resolved custom-tool allow becomes an execution grant the relay guard consumes, so + // only a dialog-approved (or policy-allowed) call ever executes from the relay dir. + onPiGateAllowed: (info) => + executionGrants.grant(info.toolName, info.args), // Record the parkable permission gate (only in keep-alive park mode) so the dispatch can // resume it live. Fires per pending gate (before the latch) so a parallel gate is counted; // the single-gate resume records only the FIRST gate's answer target. `info.gateType` names @@ -1334,6 +1354,44 @@ export async function runTurn( log: logger, }); + // Pi only: the dialog gate lives in the sandbox, so the relay re-checks every execute record + // runner-side (a forged record must not run an ask/deny tool). Claude keeps today's behavior: + // its harness gates fire before a call reaches the relay, and its relay was never re-checked. + const relayGuard: RelayExecutionGuard | undefined = plan.isPi + ? (spec, req) => { + const verdict = decide( + { + executor: "relay", + toolName: spec.name, + specPermission: spec.permission, + readOnlyHint: spec.readOnly, + args: req.args, + }, + permissionPlan, + relayGuardDecisions, + ); + if (verdict.kind === "allow") return { allow: true }; + if (verdict.kind === "deny") { + return { + allow: false, + reason: `Tool '${spec.name}' is denied by the permission policy.`, + }; + } + return executionGrants.consume( + spec.name, + redactContextBoundArgs( + req.args, + spec.callRef ? spec.contextBindings : undefined, + ), + ) + ? { allow: true } + : { + allow: false, + reason: `Tool '${spec.name}' was not approved via the permission dialog.`, + }; + } + : undefined; + if (plan.useToolRelay) { turn.toolRelay = (deps.startToolRelay ?? startToolRelay)( plan.isDaytona @@ -1344,6 +1402,7 @@ export async function runTurn( request.toolCallback as ToolCallbackContext | undefined, request.runContext, env.clientToolRelayRef.current, + relayGuard, ); } @@ -1368,6 +1427,13 @@ export async function runTurn( }); promptPromise = Promise.resolve(opts.resume.promptPromise); promptPromise.catch(() => {}); + // A parked Pi dialog gate resumes on a FRESH turn whose relay and grant ledger are new; + // grant the approved call here so the extension's execute record (written right after the + // confirm resolves) passes the relay guard. Claude resumes grant too — harmlessly, no + // guard consults it. + if (opts.resume.reply === "once") { + executionGrants.grant(opts.resume.toolName, opts.resume.args); + } await env.session.respondPermission( opts.resume.permissionId, opts.resume.reply, diff --git a/services/runner/src/engines/sandbox_agent/acp-interactions.ts b/services/runner/src/engines/sandbox_agent/acp-interactions.ts index 437e81d2d0..050d0287fe 100644 --- a/services/runner/src/engines/sandbox_agent/acp-interactions.ts +++ b/services/runner/src/engines/sandbox_agent/acp-interactions.ts @@ -14,6 +14,7 @@ import { parsePiGateEnvelope, type PiGateEnvelope, } from "./pi-gate-envelope.ts"; +import { redactContextBoundArgs } from "../../tools/relay.ts"; /** The parkable gate types a paused turn can record (the Claude ACP and Pi ACP gates). */ export type ParkedApprovalGateType = @@ -24,6 +25,9 @@ export type ParkedApprovalGateType = export interface PiToolSpecMeta { permission?: ToolPermission; readOnly?: boolean; + /** Present only for callRef tools; drives approval-args redaction (bound paths are overwritten + * from runContext at execution, so the card and decision keys must not show the model's values). */ + contextBindings?: Record; } export interface AttachPermissionResponderInput { @@ -65,6 +69,10 @@ export interface AttachPermissionResponderInput { /** Which gate paused, so the park record can resume it on the right plane. */ gateType: ParkedApprovalGateType; }) => void; + /** Fires when a Pi CUSTOM-TOOL gate resolves to allow (author/policy/stored-decision). The + * runner records an execution grant so the relay guard accepts exactly this approved call; + * builtins never reach the relay, so they do not fire it. */ + onPiGateAllowed?: (info: { toolName: string; args: unknown }) => void; /** * Resolved tool specs by name for the Pi gates. PRESENCE marks a Pi run and turns Pi gate * envelope detection on; it must stay absent for Claude. The pre-filter is the dialog TITLE, @@ -90,6 +98,7 @@ export function attachPermissionResponder({ onCreateInteraction, onResolveInteraction, onUserApprovalGate, + onPiGateAllowed, piToolSpecsByName, }: AttachPermissionResponderInput): void { session.onPermissionRequest((req: any) => { @@ -103,8 +112,9 @@ export function attachPermissionResponder({ // gate's stable anchor). The Vercel egress prefers it over the drift-prone title/kind // display fields, so the approval part names the tool exactly as the responder keys it. // This stamping never mutates the inbound ACP object. (The one deliberate inbound mutation - // is the Pi gate's id/args normalization in `handlePiGate`, which must happen in - // place so every downstream read sees the envelope's real identity.) + // is the Pi gate's id/args normalization in `handlePiGate`, which must happen in place so + // every downstream read sees the envelope's real identity — with `rawInput` set to the + // gate's REDACTED args, never the model's values for context-bound paths.) const stampResolvedName = (toolCall: any, gate: GateDescriptor): any => { if (!toolCall || typeof toolCall !== "object" || !gate.toolName) return toolCall; @@ -244,8 +254,9 @@ export function attachPermissionResponder({ * A Pi gate that rode `ctx.ui.confirm`: classify from the envelope identity, not from the * spec-less dialog strings. The tool-call id is normalized to the envelope's REAL id BEFORE * anything reads `req.toolCall` (the descriptor, pause bookkeeping, the emitted card, and the - * park record all key on it); the emitted card's `rawInput` is set to the real args so it - * renders like a relay-gate card rather than showing the envelope JSON. + * park record all key on it); the emitted card's `rawInput` is set to the gate's REDACTED + * args (bound paths stripped for a contextBindings tool, verbatim otherwise) so it renders + * like a relay-gate card without showing model values the execution will overwrite. */ const handlePiGate = async ( req: any, @@ -253,11 +264,6 @@ export function attachPermissionResponder({ availableReplies: string[], envelope: PiGateEnvelope, ): Promise => { - const toolCall = req?.toolCall; - if (toolCall && typeof toolCall === "object") { - toolCall.toolCallId = envelope.toolCallId; - toolCall.rawInput = envelope.input; - } const gate = buildPiGateDescriptor(envelope, piToolSpecsByName); // An unrecognized tool name (builtin OR custom) fails closed. The envelope is // sandbox-origin and untrusted; letting the raw name through would resolve it against the @@ -270,6 +276,11 @@ export function attachPermissionResponder({ await rejectRequest(id, availableReplies); return; } + const toolCall = req?.toolCall; + if (toolCall && typeof toolCall === "object") { + toolCall.toolCallId = envelope.toolCallId; + toolCall.rawInput = gate.args; + } if (log) { log( `[HITL] pi-gate id=${id} ` + @@ -293,6 +304,11 @@ export function attachPermissionResponder({ pauseUserApproval(req, id, gate, "pi-acp-permission"); return; } + // The grant must exist BEFORE the harness reply: the extension writes the execute record + // the moment the confirm resolves, and the relay guard consumes the grant to accept it. + if (verdict.kind === "allow" && envelope.gate === "pi-custom-tool") { + onPiGateAllowed?.({ toolName: gate.toolName!, args: gate.args }); + } await replyPermission(id, verdict.kind, availableReplies); }; @@ -414,7 +430,9 @@ export function buildPiGateDescriptor( specPermission: toolPermission(spec?.permission), readOnlyHint: typeof spec?.readOnly === "boolean" ? spec.readOnly : undefined, - args: envelope.input, + // Context-bound paths are overwritten from runContext at execution; the approval card and + // the stored-decision key must not carry the model's values for them. + args: redactContextBoundArgs(envelope.input, spec?.contextBindings), }; } diff --git a/services/runner/src/extensions/agenta.ts b/services/runner/src/extensions/agenta.ts index 7446e410ce..48ce1fb1e0 100644 --- a/services/runner/src/extensions/agenta.ts +++ b/services/runner/src/extensions/agenta.ts @@ -304,6 +304,8 @@ function registerTools(pi: ExtensionAPI): void { promptGuidelines: promptGuidelines(spec), // Pi accepts plain JSON Schema here (non-TypeBox validation path). parameters: (specInputSchema(spec) as any) ?? EMPTY_OBJECT_SCHEMA, + // The positional shape (ctx 5th) is pi-coding-agent's registerTool execute contract. If + // upstream changes the arity, the gate fails closed (no ui -> block); it never fails open. async execute( toolCallId: string, params: unknown, diff --git a/services/runner/src/responder.ts b/services/runner/src/responder.ts index aeae8b720e..3647ecf818 100644 --- a/services/runner/src/responder.ts +++ b/services/runner/src/responder.ts @@ -75,6 +75,34 @@ export function approvedCallKey( return `${shape.toolName}#${hash}`; } +/** + * Per-turn ledger of approval-equivalent allows for Pi relay executions. The dialog gate (or a + * parked-approval resume) grants; the relay execution guard consumes one grant per matching + * record. Keyed by `approvedCallKey(toolName, args)` with a count, so N approvals permit exactly + * N executions and a forged or replayed record for an `ask` tool fails closed. + */ +export class ApprovedExecutionGrants { + private counts = new Map(); + + /** Record one approval-equivalent allow. No-op when the call is unkeyable (fails closed). */ + grant(toolName: string | undefined, args: unknown): void { + const key = approvedCallKey(toolName, args); + if (!key) return; + this.counts.set(key, (this.counts.get(key) ?? 0) + 1); + } + + /** Consume one grant for this exact call; false when absent, exhausted, or unkeyable. */ + consume(toolName: string | undefined, args: unknown): boolean { + const key = approvedCallKey(toolName, args); + if (!key) return false; + const count = this.counts.get(key) ?? 0; + if (count <= 0) return false; + if (count === 1) this.counts.delete(key); + else this.counts.set(key, count - 1); + return true; + } +} + /** * Order-independent, stable serialization of tool args so the same call hashes the same. * Returns `undefined` for any value that is not plain JSON so the caller can fail closed diff --git a/services/runner/src/tools/relay.ts b/services/runner/src/tools/relay.ts index 881041397c..6ae4782230 100644 --- a/services/runner/src/tools/relay.ts +++ b/services/runner/src/tools/relay.ts @@ -96,6 +96,74 @@ export interface ExecuteRelayResponse { export type RelayResponse = ExecuteRelayResponse; const PAUSED = Symbol("paused"); +/** + * Runner-side authorization for one relay execute record. The relay dir is sandbox-writable, + * so a record can be forged without ever passing the in-sandbox approval dialog; this re-check + * is the runner-side enforcement the dialog cannot provide. The deny reason becomes the tool's + * result text, so the model loop continues (same shape as a dialog deny). + */ +export type RelayExecutionGuard = ( + spec: ResolvedToolSpec, + req: ExecuteRelayRequest, +) => { allow: true } | { allow: false; reason: string }; + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function cloneJsonish(value: unknown): unknown { + if (Array.isArray(value)) return value.map((item) => cloneJsonish(item)); + if (!isRecord(value)) return value; + const out: Record = {}; + for (const [key, item] of Object.entries(value)) { + out[key] = cloneJsonish(item); + } + return out; +} + +function pruneEmptyAncestors( + target: Record, + path: string, +): void { + const parts = path.split("."); + const ancestors: Array<{ owner: Record; key: string }> = []; + let cursor = target; + for (const part of parts.slice(0, -1)) { + const next = cursor[part]; + if (!isRecord(next)) return; + ancestors.push({ owner: cursor, key: part }); + cursor = next; + } + for (const { owner, key } of ancestors.reverse()) { + const value = owner[key]; + if (!isRecord(value) || Object.keys(value).length > 0) return; + delete owner[key]; + } +} + +/** + * Strip context-bound argument paths from a tool call's args. Bound paths are overwritten from + * runContext at execution, so approval display and stored-decision keys must not include the + * model's values for them: a card would show a value that never executes, and a decision keyed + * on it would not match the same call re-keyed after redaction. Empty ancestor objects left by + * a deleted path are pruned so the redacted shape is canonical. + */ +export function redactContextBoundArgs( + args: unknown, + contextBindings: Record | undefined, +): unknown { + if (!contextBindings || Object.keys(contextBindings).length === 0) + return args; + if (!isRecord(args)) return args; + const redacted = cloneJsonish(args); + if (!isRecord(redacted)) return redacted; + for (const path of Object.keys(contextBindings)) { + deepDelete(redacted, path); + pruneEmptyAncestors(redacted, path); + } + return redacted; +} + /** 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"; @@ -161,6 +229,7 @@ async function executeRelayedTool( callback: ToolCallbackContext | undefined, runContext: RunContext | undefined, clientToolRelay: ClientToolRelay | undefined, + guard: RelayExecutionGuard | undefined, ): Promise { if (spec.kind === "client") { assertRequiredArguments(spec, req.args); @@ -188,6 +257,14 @@ async function executeRelayedTool( return JSON.stringify(decision.output ?? {}); } + // Client tools keep their own browser-fulfilled pause semantics above; everything else is + // re-checked here because the request file is sandbox-writable and proves nothing about the + // dialog gate having run. + if (guard) { + const verdict = guard(spec, req); + if (!verdict.allow) return verdict.reason; + } + return executeAllowedRelayedTool(spec, req, callback, runContext); } @@ -249,6 +326,7 @@ export function startToolRelay( callback: ToolCallbackContext | undefined, runContext?: RunContext, clientToolRelay?: ClientToolRelay, + guard?: RelayExecutionGuard, ): { stop: () => Promise } { let active = true; const seen = new Set(); @@ -269,6 +347,7 @@ export function startToolRelay( callback, runContext, clientToolRelay, + guard, ); if (text === PAUSED) return; res = { ok: true, text }; diff --git a/services/runner/tests/unit/extension-tools.test.ts b/services/runner/tests/unit/extension-tools.test.ts index 58d5615702..9662aeab1d 100644 --- a/services/runner/tests/unit/extension-tools.test.ts +++ b/services/runner/tests/unit/extension-tools.test.ts @@ -391,6 +391,37 @@ describe("agenta extension: Pi dialog gate (approval parking)", () => { ); }); + it("custom-tool gate: a malformed call errors to the model BEFORE the dialog is raised", async () => { + // Argument validation precedes the gate: a missing required argument must never reach a + // human as an approval prompt (and never relay as a no-op). + clearEnv(); + process.env.AGENTA_AGENT_TOOLS_PUBLIC_SPECS = JSON.stringify([ + { + name: "park_probe", + description: "echo", + kind: "callback", + inputSchema: { + type: "object", + properties: { token: { type: "string" } }, + required: ["token"], + }, + }, + ]); + process.env.AGENTA_AGENT_TOOLS_RELAY_DIR = + "/tmp/agenta-relay-must-not-be-used"; + + const pi = fakePi(); + factory(pi as any); + const tool = pi.registered[0]; + const { calls, ctx } = fakeDialogCtx(true); + + await assert.rejects( + () => tool.execute("call_1", {}, undefined, undefined, ctx), + /missing required argument\(s\): token/, + ); + assert.equal(calls.length, 0, "the dialog was never raised"); + }); + it("custom-tool gate: a CLIENT spec is NOT dialog-gated (keeps its relay path)", async () => { clearEnv(); const dir = mkdtempSync(join(tmpdir(), "agenta-relay-client-")); diff --git a/services/runner/tests/unit/pi-gate-envelope.test.ts b/services/runner/tests/unit/pi-gate-envelope.test.ts index 9d851064db..f76a2304ea 100644 --- a/services/runner/tests/unit/pi-gate-envelope.test.ts +++ b/services/runner/tests/unit/pi-gate-envelope.test.ts @@ -258,6 +258,35 @@ describe("buildPiGateDescriptor (runner-side metadata recovery)", () => { assert.deepEqual(g!.args, { a: 1, b: 2 }); }); + it("a spec with contextBindings redacts the bound path from the gate args", () => { + // Bound paths are runner-filled at execution; the descriptor (card + decision key) must not + // carry the model's value for them, and the emptied ancestor object is pruned. + const g = buildPiGateDescriptor( + { + v: 1, + kind: "agenta.gate", + gate: "pi-custom-tool", + toolName: "test_run", + toolCallId: "c", + input: { + target: { workflow_variant_id: "model-sent" }, + inputs: { city: "Berlin" }, + }, + }, + new Map([ + [ + "test_run", + { + contextBindings: { + "target.workflow_variant_id": "$ctx.workflow.variant.id", + }, + }, + ], + ]), + ); + assert.deepEqual(g!.args, { inputs: { city: "Berlin" } }); + }); + it("an unknown name yields NO descriptor (the caller must reject it)", () => { // The sandbox-origin envelope must not resolve a fabricated name against the default // permission or put it on the approval card: a builtin outside the canonical set and a diff --git a/services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts b/services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts index 31d3697dfd..12a8c303f0 100644 --- a/services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts +++ b/services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts @@ -812,6 +812,134 @@ describe("attachPermissionResponder: Pi dialog gate", () => { assert.deepEqual(events, [], "no approval card for a fabricated name"); }); + it("redacts context-bound argument paths from the approval card and the park record", async () => { + // Bound paths are overwritten from runContext at execution; the card must not show the + // model's values for them, and the park record (grant key) must match the redacted shape. + const { session, emit } = makeSession(); + const events: AgentEvent[] = []; + const gates: any[] = []; + + attachPermissionResponder({ + session, + run: { emitEvent: (event) => events.push(event) }, + responder: fakeResponder({ kind: "pendingApproval" }), + latch: new PendingApprovalLatch(), + piToolSpecsByName: piSpecs([ + [ + "test_run", + { + contextBindings: { + "target.workflow_variant_id": "$ctx.workflow.variant.id", + }, + }, + ], + ]), + onUserApprovalGate: (info) => gates.push(info), + }); + emit( + piGateRequest({ + gate: "pi-custom-tool", + toolName: "test_run", + toolCallId: "call_1", + input: { + target: { workflow_variant_id: "model-sent" }, + inputs: { city: "Berlin" }, + }, + }), + ); + await flushPromises(); + + const payload = (events[0] as any).payload; + // Bound path gone; its now-empty ancestor object pruned. + assert.deepEqual(payload.toolCall.rawInput, { inputs: { city: "Berlin" } }); + assert.deepEqual(gates[0].args, { inputs: { city: "Berlin" } }); + }); + + it("onPiGateAllowed fires for an allowed custom tool with the redacted args", async () => { + const { session, emit } = makeSession(); + const allowed: Array<{ toolName: string; args: unknown }> = []; + const responder = new ApprovalResponder( + permissionPlan("ask"), + new ConversationDecisions(new Map()), + ); + + attachPermissionResponder({ + session, + run: { emitEvent: () => {} }, + responder, + latch: new PendingApprovalLatch(), + piToolSpecsByName: piSpecs([ + [ + "author_allow", + { + permission: "allow", + contextBindings: { + "target.workflow_variant_id": "$ctx.workflow.variant.id", + }, + }, + ], + ]), + onPiGateAllowed: (info) => allowed.push(info), + }); + emit( + piGateRequest({ + gate: "pi-custom-tool", + toolName: "author_allow", + toolCallId: "c1", + input: { + target: { workflow_variant_id: "model-sent" }, + inputs: { city: "Berlin" }, + }, + }), + ); + await flushPromises(); + + assert.deepEqual(allowed, [ + { toolName: "author_allow", args: { inputs: { city: "Berlin" } } }, + ]); + }); + + it("onPiGateAllowed never fires on a deny or for an allowed builtin gate", async () => { + const { session, emit } = makeSession(); + const allowed: unknown[] = []; + const responder = new ApprovalResponder( + permissionPlan("allow_reads"), + new ConversationDecisions(new Map()), + ); + + attachPermissionResponder({ + session, + run: { emitEvent: () => {} }, + responder, + latch: new PendingApprovalLatch(), + piToolSpecsByName: piSpecs([["author_deny", { permission: "deny" }]]), + onPiGateAllowed: (info) => allowed.push(info), + }); + emit( + piGateRequest({ + gate: "pi-custom-tool", + toolName: "author_deny", + toolCallId: "c1", + input: {}, + }), + ); + await flushPromises(); + assert.deepEqual(allowed, [], "a denied custom tool grants nothing"); + + // A read-only builtin auto-allows under allow_reads, but builtins never reach the relay, + // so no execution grant is recorded for them. + emit( + piGateRequest({ + gate: "pi-builtin", + toolName: "read", + toolCallId: "c2", + input: { path: "a" }, + }), + ); + await flushPromises(); + assert.deepEqual(allowed, [], "an allowed builtin grants nothing"); + }); + it("a Claude run (no Pi specs) never enters envelope detection, even on a title collision", async () => { // attachPermissionResponder is shared by Claude and Pi. On a Claude run (piToolSpecsByName // absent), a gate titled literally "agenta-approval" (editing a file with that name, a bash diff --git a/services/runner/tests/unit/sandbox-agent-orchestration.test.ts b/services/runner/tests/unit/sandbox-agent-orchestration.test.ts index 9a1d718d43..e1ff7c6729 100644 --- a/services/runner/tests/unit/sandbox-agent-orchestration.test.ts +++ b/services/runner/tests/unit/sandbox-agent-orchestration.test.ts @@ -703,6 +703,9 @@ describe("runSandboxAgent orchestration", () => { Object.keys((calls.toolRelayArgs?.[5] ?? {}) as object).sort(), ["onClientTool", "onPause"], ); + // A Pi run passes the execution guard: the relay dir is sandbox-writable, so every execute + // record is re-checked runner-side (a forged record must not run an ask/deny tool). + assert.equal(typeof calls.toolRelayArgs?.[6], "function"); assert.equal( calls.toolRelayStops, 2, @@ -734,7 +737,9 @@ describe("runSandboxAgent orchestration", () => { true, "the run succeeds; gateway tools reach Claude", ); - // The relay carries execution only; Claude's own ACP gates decide before a call reaches it. + // The relay carries execution only; Claude's own ACP gates decide before a call reaches it, + // so a Claude run never gets the Pi execution guard. + assert.equal(calls.toolRelayArgs?.[6], undefined); const mcpServers = calls.createSessionOptions?.sessionInit?.mcpServers ?? []; assert.equal( diff --git a/services/runner/tests/unit/tool-relay-guard.test.ts b/services/runner/tests/unit/tool-relay-guard.test.ts new file mode 100644 index 0000000000..7f1788e156 --- /dev/null +++ b/services/runner/tests/unit/tool-relay-guard.test.ts @@ -0,0 +1,267 @@ +/** + * Unit tests for the relay execution guard (finding P1). + * + * The relay dir is sandbox-writable, so the model can forge an `.req.json` execute record + * without ever passing the in-sandbox `ctx.ui.confirm` dialog. The guard is the runner-side + * re-check: an author-allow tool executes, an author-deny tool never does, and an `ask` tool + * executes only by consuming a grant the dialog gate (or a parked-approval resume) recorded. + * The guard here is composed exactly the way `runTurn` builds it (decide + an EMPTY stored + * decision store + the grant ledger + context-binding redaction) so the test pins the composed + * behavior, not just the pieces. + * + * Run: pnpm test (or: pnpm exec vitest run tests/unit/tool-relay-guard.test.ts) + */ +import { afterEach, describe, it } from "vitest"; +import assert from "node:assert/strict"; +import { + existsSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import type { ResolvedToolSpec, RunContext } from "../../src/protocol.ts"; +import { + localRelayHost, + redactContextBoundArgs, + startToolRelay, + type RelayExecutionGuard, + type RelayResponse, +} from "../../src/tools/relay.ts"; +import { + ApprovedExecutionGrants, + ConversationDecisions, +} from "../../src/responder.ts"; +import { decide, type PermissionPlan } from "../../src/permission-plan.ts"; + +const ENDPOINT = "https://agenta.example/api/tools/call"; +const RUN_CONTEXT: RunContext = { + run: { kind: "test" }, + workflow: { variant: { id: "own-variant" } }, +}; + +interface CapturedFetch { + url: string; + init: RequestInit; +} + +const realFetch = globalThis.fetch; + +afterEach(() => { + globalThis.fetch = realFetch; +}); + +function stubFetch(body = "ok"): CapturedFetch[] { + const calls: CapturedFetch[] = []; + globalThis.fetch = (async (url: any, init: any) => { + calls.push({ url: String(url), init: init ?? {} }); + return new Response(body, { status: 200 }); + }) as typeof fetch; + return calls; +} + +/** The guard exactly as `runTurn` composes it: decide() over an EMPTY stored-decision store + * (the dialog is the stored decisions' consumer, never the guard), then the grant ledger, + * consuming under the same redaction `handlePiGate` applied when the grant was recorded. */ +function buildRelayGuard( + permissionPlan: PermissionPlan, + executionGrants: ApprovedExecutionGrants, +): RelayExecutionGuard { + const relayGuardDecisions = new ConversationDecisions(new Map()); + return (spec, req) => { + const verdict = decide( + { + executor: "relay", + toolName: spec.name, + specPermission: spec.permission, + readOnlyHint: spec.readOnly, + args: req.args, + }, + permissionPlan, + relayGuardDecisions, + ); + if (verdict.kind === "allow") return { allow: true }; + if (verdict.kind === "deny") { + return { + allow: false, + reason: `Tool '${spec.name}' is denied by the permission policy.`, + }; + } + return executionGrants.consume( + spec.name, + redactContextBoundArgs( + req.args, + spec.callRef ? spec.contextBindings : undefined, + ), + ) + ? { allow: true } + : { + allow: false, + reason: `Tool '${spec.name}' was not approved via the permission dialog.`, + }; + }; +} + +/** Write one forged execute record and run the relay over it (a record the model could write + * itself with bash — it proves nothing about the dialog having run). */ +async function relayOnce(input: { + spec: ResolvedToolSpec; + args: unknown; + guard?: RelayExecutionGuard; + runContext?: RunContext; +}): Promise { + const dir = mkdtempSync(join(tmpdir(), "agenta-relay-guard-")); + try { + const id = "call-1"; + writeFileSync( + join(dir, `${id}.req.json`), + JSON.stringify({ + toolName: input.spec.name, + toolCallId: id, + args: input.args, + }), + ); + const relay = startToolRelay( + localRelayHost(), + dir, + [input.spec], + { endpoint: ENDPOINT, authorization: "ApiKey secret" }, + input.runContext, + undefined, + input.guard, + ); + const resPath = join(dir, `${id}.res.json`); + const deadline = Date.now() + 5000; + while (Date.now() < deadline && !existsSync(resPath)) { + await new Promise((resolve) => setTimeout(resolve, 20)); + } + await relay.stop(); + assert.ok(existsSync(resPath), "the relay wrote a response file"); + return JSON.parse(readFileSync(resPath, "utf-8")) as RelayResponse; + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +function askSpec(overrides: Partial = {}): ResolvedToolSpec { + return { + name: "park_probe", + kind: "callback", + callRef: "tools.agenta.park_probe", + permission: "ask", + ...overrides, + }; +} + +const ASK_PLAN: PermissionPlan = { default: "ask", rules: [] }; + +describe("startToolRelay execution guard", () => { + it("a forged record for an `ask` tool with no grant fails closed, never fetching", async () => { + const calls = stubFetch(); + const guard = buildRelayGuard(ASK_PLAN, new ApprovedExecutionGrants()); + + const res = await relayOnce({ + spec: askSpec(), + args: { token: "T" }, + guard, + }); + + assert.equal(res.ok, true, "a guard deny is a tool RESULT, not an error"); + assert.match(res.text ?? "", /was not approved/); + assert.equal(calls.length, 0, "the forged record never executed"); + }); + + it("a granted call executes exactly once; a replayed identical record is denied", async () => { + const calls = stubFetch(); + const executionGrants = new ApprovedExecutionGrants(); + const guard = buildRelayGuard(ASK_PLAN, executionGrants); + const args = { token: "T" }; + // The dialog gate approved this exact call once (as handlePiGate records it). + executionGrants.grant("park_probe", args); + + const first = await relayOnce({ spec: askSpec(), args, guard }); + assert.equal(first.ok, true); + assert.equal(calls.length, 1, "the approved call executed"); + + const replay = await relayOnce({ spec: askSpec(), args, guard }); + assert.match(replay.text ?? "", /was not approved/); + assert.equal(calls.length, 1, "the replayed record consumed nothing"); + }); + + it("an author-deny tool is denied regardless of any record", async () => { + const calls = stubFetch(); + const guard = buildRelayGuard(ASK_PLAN, new ApprovedExecutionGrants()); + + const res = await relayOnce({ + spec: askSpec({ permission: "deny" }), + args: {}, + guard, + }); + + assert.match(res.text ?? "", /denied by the permission policy/); + assert.equal(calls.length, 0); + }); + + it("an author-allow tool executes with no grant (instant-allow parity with the dialog)", async () => { + const calls = stubFetch(); + const guard = buildRelayGuard(ASK_PLAN, new ApprovedExecutionGrants()); + + const res = await relayOnce({ + spec: askSpec({ permission: "allow" }), + args: {}, + guard, + }); + + assert.equal(res.ok, true); + assert.equal(calls.length, 1); + }); + + it("no guard at all executes unconditionally (Claude parity: gates fire before the relay)", async () => { + const calls = stubFetch(); + + const res = await relayOnce({ spec: askSpec(), args: { token: "T" } }); + + assert.equal(res.ok, true); + assert.equal(calls.length, 1); + }); + + it("a contextBindings tool: the grant is keyed on REDACTED args and matches the raw record", async () => { + const calls = stubFetch(); + const executionGrants = new ApprovedExecutionGrants(); + const guard = buildRelayGuard(ASK_PLAN, executionGrants); + const spec = askSpec({ + contextBindings: { + "target.workflow_variant_id": "$ctx.workflow.variant.id", + }, + }); + const rawArgs = { + target: { workflow_variant_id: "model-sent" }, + inputs: { city: "Berlin" }, + }; + // handlePiGate grants with the redacted shape (the bound path is runner-filled at + // execution, so neither the card nor the grant key may carry the model's value for it). + executionGrants.grant( + spec.name, + redactContextBoundArgs(rawArgs, spec.contextBindings), + ); + + const res = await relayOnce({ + spec, + args: rawArgs, + guard, + runContext: RUN_CONTEXT, + }); + + assert.equal(res.ok, true, "redaction applied on both sides -> consumed"); + assert.equal(calls.length, 1); + const posted = JSON.parse(calls[0].init.body as string); + assert.deepEqual( + posted.data.function.arguments.target, + { workflow_variant_id: "own-variant" }, + "execution still binds the runner's own context value", + ); + }); +});