diff --git a/docs/gateway/bridge-protocol.md b/docs/gateway/bridge-protocol.md index 1e43ccc75..47ff09b4d 100644 --- a/docs/gateway/bridge-protocol.md +++ b/docs/gateway/bridge-protocol.md @@ -58,8 +58,8 @@ Exact allowlist is enforced in `src/gateway/server-bridge.ts`. ## Exec lifecycle events -Nodes can emit `exec.started`, `exec.finished`, or `exec.denied` events to surface -system.run activity. These are mapped to system events in the gateway. +Nodes can emit `exec.finished` or `exec.denied` events to surface system.run activity. +These are mapped to system events in the gateway. (Legacy nodes may still emit `exec.started`.) Payload fields (all optional unless noted): - `sessionKey` (required): agent session to receive the system event. diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index b96273664..517b73fbe 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -134,6 +134,10 @@ When a prompt is required, the gateway broadcasts `exec.approval.requested` to o The Control UI and macOS app resolve it via `exec.approval.resolve`, then the gateway forwards the approved request to the node host. +When approvals are required, the exec tool returns immediately with an approval id. Use that id to +correlate later system events (`Exec finished` / `Exec denied`). If no decision arrives before the +timeout, the request is treated as an approval timeout and surfaced as a denial reason. + The confirmation dialog includes: - command + args - cwd @@ -162,11 +166,13 @@ Security notes: ## System events Exec lifecycle is surfaced as system messages: -- `exec.started` -- `exec.finished` -- `exec.denied` +- `Exec running` (only if the command exceeds the running notice threshold) +- `Exec finished` +- `Exec denied` These are posted to the agent’s session after the node reports the event. +Gateway-host exec approvals emit the same lifecycle events when the command finishes (and optionally when running longer than the threshold). +Approval-gated execs reuse the approval id as the `runId` in these messages for easy correlation. ## Implications diff --git a/docs/tools/exec.md b/docs/tools/exec.md index 3a4f8297f..b5c53593c 100644 --- a/docs/tools/exec.md +++ b/docs/tools/exec.md @@ -38,6 +38,7 @@ Notes: ## Config - `tools.exec.notifyOnExit` (default: true): when true, backgrounded exec sessions enqueue a system event and request a heartbeat on exit. +- `tools.exec.approvalRunningNoticeMs` (default: 10000): emit a single “running” notice when an approval-gated exec runs longer than this (0 disables). - `tools.exec.host` (default: `sandbox`) - `tools.exec.security` (default: `deny` for sandbox, `allowlist` for gateway + node when unset) - `tools.exec.ask` (default: `on-miss`) @@ -92,6 +93,11 @@ Example: Sandboxed agents can require per-request approval before `exec` runs on the gateway or node host. See [Exec approvals](/tools/exec-approvals) for the policy, allowlist, and UI flow. +When approvals are required, the exec tool returns immediately with +`status: "approval-pending"` and an approval id. Once approved (or denied / timed out), +the Gateway emits system events (`Exec finished` / `Exec denied`). If the command is still +running after `tools.exec.approvalRunningNoticeMs`, a single `Exec running` notice is emitted. + ## Allowlist + safe bins Allowlist enforcement matches **resolved binary paths only** (no basename matches). When diff --git a/src/agents/bash-tools.exec.approval-id.test.ts b/src/agents/bash-tools.exec.approval-id.test.ts new file mode 100644 index 000000000..54f6d7150 --- /dev/null +++ b/src/agents/bash-tools.exec.approval-id.test.ts @@ -0,0 +1,71 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("./tools/gateway.js", () => ({ + callGatewayTool: vi.fn(), +})); + +vi.mock("./tools/nodes-utils.js", () => ({ + listNodes: vi.fn(async () => [ + { nodeId: "node-1", commands: ["system.run"], platform: "darwin" }, + ]), + resolveNodeIdFromList: vi.fn((nodes: Array<{ nodeId: string }>) => nodes[0]?.nodeId), +})); + +describe("exec approvals", () => { + let previousHome: string | undefined; + + beforeEach(async () => { + previousHome = process.env.HOME; + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "clawdbot-test-")); + process.env.HOME = tempDir; + }); + + afterEach(() => { + vi.resetAllMocks(); + if (previousHome === undefined) { + delete process.env.HOME; + } else { + process.env.HOME = previousHome; + } + }); + + it("reuses approval id as the node runId", async () => { + const { callGatewayTool } = await import("./tools/gateway.js"); + let invokeParams: unknown; + let resolveInvoke: (() => void) | undefined; + const invokeSeen = new Promise((resolve) => { + resolveInvoke = resolve; + }); + + vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => { + if (method === "exec.approval.request") { + return { decision: "allow-once" }; + } + if (method === "node.invoke") { + invokeParams = params; + resolveInvoke?.(); + return { ok: true }; + } + return { ok: true }; + }); + + const { createExecTool } = await import("./bash-tools.exec.js"); + const tool = createExecTool({ + host: "node", + ask: "always", + approvalRunningNoticeMs: 0, + }); + + const result = await tool.execute("call1", { command: "ls -la" }); + expect(result.details.status).toBe("approval-pending"); + const approvalId = (result.details as { approvalId: string }).approvalId; + + await invokeSeen; + + const runId = (invokeParams as { params?: { runId?: string } } | undefined)?.params?.runId; + expect(runId).toBe(approvalId); + }); +}); diff --git a/src/agents/bash-tools.exec.ts b/src/agents/bash-tools.exec.ts index b13003d8b..32655f6a9 100644 --- a/src/agents/bash-tools.exec.ts +++ b/src/agents/bash-tools.exec.ts @@ -70,6 +70,10 @@ const DEFAULT_PENDING_MAX_OUTPUT = clampNumber( const DEFAULT_PATH = process.env.PATH ?? "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"; const DEFAULT_NOTIFY_TAIL_CHARS = 400; +const DEFAULT_APPROVAL_TIMEOUT_MS = 120_000; +const DEFAULT_APPROVAL_REQUEST_TIMEOUT_MS = 130_000; +const DEFAULT_APPROVAL_RUNNING_NOTICE_MS = 10_000; +const APPROVAL_SLUG_LENGTH = 8; type PtyExitEvent = { exitCode: number; signal?: number }; type PtyListener = (event: T) => void; @@ -91,6 +95,24 @@ type PtySpawn = ( }, ) => PtyHandle; +type ExecProcessOutcome = { + status: "completed" | "failed"; + exitCode: number | null; + exitSignal: NodeJS.Signals | number | null; + durationMs: number; + aggregated: string; + timedOut: boolean; + reason?: string; +}; + +type ExecProcessHandle = { + session: ProcessSession; + startedAt: number; + pid?: number; + promise: Promise; + kill: () => void; +}; + export type ExecToolDefaults = { host?: ExecHost; security?: ExecSecurity; @@ -101,6 +123,7 @@ export type ExecToolDefaults = { agentId?: string; backgroundMs?: number; timeoutSec?: number; + approvalRunningNoticeMs?: number; sandbox?: BashSandboxConfig; elevated?: ExecElevatedDefaults; allowBackground?: boolean; @@ -182,6 +205,16 @@ export type ExecToolDetails = durationMs: number; aggregated: string; cwd?: string; + } + | { + status: "approval-pending"; + approvalId: string; + approvalSlug: string; + expiresAtMs: number; + host: ExecHost; + command: string; + cwd?: string; + nodeId?: string; }; function normalizeExecHost(value?: string | null): ExecHost | null { @@ -286,6 +319,326 @@ function maybeNotifyOnExit(session: ProcessSession, status: "completed" | "faile requestHeartbeatNow({ reason: `exec:${session.id}:exit` }); } +function createApprovalSlug(id: string) { + return id.slice(0, APPROVAL_SLUG_LENGTH); +} + +function resolveApprovalRunningNoticeMs(value?: number) { + if (typeof value !== "number" || !Number.isFinite(value)) { + return DEFAULT_APPROVAL_RUNNING_NOTICE_MS; + } + if (value <= 0) return 0; + return Math.floor(value); +} + +function emitExecSystemEvent(text: string, opts: { sessionKey?: string; contextKey?: string }) { + const sessionKey = opts.sessionKey?.trim(); + if (!sessionKey) return; + enqueueSystemEvent(text, { sessionKey, contextKey: opts.contextKey }); + requestHeartbeatNow({ reason: "exec-event" }); +} + +async function runExecProcess(opts: { + command: string; + workdir: string; + env: Record; + sandbox?: BashSandboxConfig; + containerWorkdir?: string | null; + usePty: boolean; + warnings: string[]; + maxOutput: number; + pendingMaxOutput: number; + notifyOnExit: boolean; + scopeKey?: string; + sessionKey?: string; + timeoutSec: number; + onUpdate?: (partialResult: AgentToolResult) => void; +}): Promise { + const startedAt = Date.now(); + const sessionId = createSessionSlug(); + let child: ChildProcessWithoutNullStreams | null = null; + let pty: PtyHandle | null = null; + let stdin: SessionStdin | undefined; + + if (opts.sandbox) { + child = spawn( + "docker", + buildDockerExecArgs({ + containerName: opts.sandbox.containerName, + command: opts.command, + workdir: opts.containerWorkdir ?? opts.sandbox.containerWorkdir, + env: opts.env, + tty: opts.usePty, + }), + { + cwd: opts.workdir, + env: process.env, + detached: process.platform !== "win32", + stdio: ["pipe", "pipe", "pipe"], + windowsHide: true, + }, + ) as ChildProcessWithoutNullStreams; + stdin = child.stdin; + } else if (opts.usePty) { + const ptyModule = (await import("@lydell/node-pty")) as unknown as { + spawn?: PtySpawn; + default?: { spawn?: PtySpawn }; + }; + const spawnPty = ptyModule.spawn ?? ptyModule.default?.spawn; + if (!spawnPty) { + throw new Error("PTY support is unavailable (node-pty spawn not found)."); + } + const { shell, args: shellArgs } = getShellConfig(); + pty = spawnPty(shell, [...shellArgs, opts.command], { + cwd: opts.workdir, + env: opts.env, + name: process.env.TERM ?? "xterm-256color", + cols: 120, + rows: 30, + }); + stdin = { + destroyed: false, + write: (data, cb) => { + try { + pty?.write(data); + cb?.(null); + } catch (err) { + cb?.(err as Error); + } + }, + end: () => { + try { + const eof = process.platform === "win32" ? "\x1a" : "\x04"; + pty?.write(eof); + } catch { + // ignore EOF errors + } + }, + }; + } else { + const { shell, args: shellArgs } = getShellConfig(); + child = spawn(shell, [...shellArgs, opts.command], { + cwd: opts.workdir, + env: opts.env, + detached: process.platform !== "win32", + stdio: ["pipe", "pipe", "pipe"], + windowsHide: true, + }) as ChildProcessWithoutNullStreams; + stdin = child.stdin; + } + + const session = { + id: sessionId, + command: opts.command, + scopeKey: opts.scopeKey, + sessionKey: opts.sessionKey, + notifyOnExit: opts.notifyOnExit, + exitNotified: false, + child: child ?? undefined, + stdin, + pid: child?.pid ?? pty?.pid, + startedAt, + cwd: opts.workdir, + maxOutputChars: opts.maxOutput, + pendingMaxOutputChars: opts.pendingMaxOutput, + totalOutputChars: 0, + pendingStdout: [], + pendingStderr: [], + pendingStdoutChars: 0, + pendingStderrChars: 0, + aggregated: "", + tail: "", + exited: false, + exitCode: undefined as number | null | undefined, + exitSignal: undefined as NodeJS.Signals | number | null | undefined, + truncated: false, + backgrounded: false, + } satisfies ProcessSession; + addSession(session); + + let settled = false; + let timeoutTimer: NodeJS.Timeout | null = null; + let timeoutFinalizeTimer: NodeJS.Timeout | null = null; + let timedOut = false; + const timeoutFinalizeMs = 1000; + let resolveFn: ((outcome: ExecProcessOutcome) => void) | null = null; + + const settle = (outcome: ExecProcessOutcome) => { + if (settled) return; + settled = true; + resolveFn?.(outcome); + }; + + const finalizeTimeout = () => { + if (session.exited) return; + markExited(session, null, "SIGKILL", "failed"); + maybeNotifyOnExit(session, "failed"); + const aggregated = session.aggregated.trim(); + const reason = `Command timed out after ${opts.timeoutSec} seconds`; + settle({ + status: "failed", + exitCode: null, + exitSignal: "SIGKILL", + durationMs: Date.now() - startedAt, + aggregated, + timedOut: true, + reason: aggregated ? `${aggregated}\n\n${reason}` : reason, + }); + }; + + const onTimeout = () => { + timedOut = true; + killSession(session); + if (!timeoutFinalizeTimer) { + timeoutFinalizeTimer = setTimeout(() => { + finalizeTimeout(); + }, timeoutFinalizeMs); + } + }; + + if (opts.timeoutSec > 0) { + timeoutTimer = setTimeout(() => { + onTimeout(); + }, opts.timeoutSec * 1000); + } + + const emitUpdate = () => { + if (!opts.onUpdate) return; + const tailText = session.tail || session.aggregated; + const warningText = opts.warnings.length ? `${opts.warnings.join("\n")}\n\n` : ""; + opts.onUpdate({ + content: [{ type: "text", text: warningText + (tailText || "") }], + details: { + status: "running", + sessionId, + pid: session.pid ?? undefined, + startedAt, + cwd: session.cwd, + tail: session.tail, + }, + }); + }; + + const handleStdout = (data: string) => { + const str = sanitizeBinaryOutput(data.toString()); + for (const chunk of chunkString(str)) { + appendOutput(session, "stdout", chunk); + emitUpdate(); + } + }; + + const handleStderr = (data: string) => { + const str = sanitizeBinaryOutput(data.toString()); + for (const chunk of chunkString(str)) { + appendOutput(session, "stderr", chunk); + emitUpdate(); + } + }; + + if (pty) { + const cursorResponse = buildCursorPositionResponse(); + pty.onData((data) => { + const raw = data.toString(); + const { cleaned, requests } = stripDsrRequests(raw); + if (requests > 0) { + for (let i = 0; i < requests; i += 1) { + pty.write(cursorResponse); + } + } + handleStdout(cleaned); + }); + } else if (child) { + child.stdout.on("data", handleStdout); + child.stderr.on("data", handleStderr); + } + + const promise = new Promise((resolve) => { + resolveFn = resolve; + const handleExit = (code: number | null, exitSignal: NodeJS.Signals | number | null) => { + if (timeoutTimer) clearTimeout(timeoutTimer); + if (timeoutFinalizeTimer) clearTimeout(timeoutFinalizeTimer); + const durationMs = Date.now() - startedAt; + const wasSignal = exitSignal != null; + const isSuccess = code === 0 && !wasSignal && !timedOut; + const status: "completed" | "failed" = isSuccess ? "completed" : "failed"; + markExited(session, code, exitSignal, status); + maybeNotifyOnExit(session, status); + if (!session.child && session.stdin) { + session.stdin.destroyed = true; + } + + if (settled) return; + const aggregated = session.aggregated.trim(); + if (!isSuccess) { + const reason = timedOut + ? `Command timed out after ${opts.timeoutSec} seconds` + : wasSignal && exitSignal + ? `Command aborted by signal ${exitSignal}` + : code === null + ? "Command aborted before exit code was captured" + : `Command exited with code ${code}`; + const message = aggregated ? `${aggregated}\n\n${reason}` : reason; + settle({ + status: "failed", + exitCode: code ?? null, + exitSignal: exitSignal ?? null, + durationMs, + aggregated, + timedOut, + reason: message, + }); + return; + } + settle({ + status: "completed", + exitCode: code ?? 0, + exitSignal: exitSignal ?? null, + durationMs, + aggregated, + timedOut: false, + }); + }; + + if (pty) { + pty.onExit((event) => { + const rawSignal = event.signal ?? null; + const normalizedSignal = rawSignal === 0 ? null : rawSignal; + handleExit(event.exitCode ?? null, normalizedSignal); + }); + } else if (child) { + child.once("close", (code, exitSignal) => { + handleExit(code, exitSignal); + }); + + child.once("error", (err) => { + if (timeoutTimer) clearTimeout(timeoutTimer); + if (timeoutFinalizeTimer) clearTimeout(timeoutFinalizeTimer); + markExited(session, null, null, "failed"); + maybeNotifyOnExit(session, "failed"); + const aggregated = session.aggregated.trim(); + const message = aggregated ? `${aggregated}\n\n${String(err)}` : String(err); + settle({ + status: "failed", + exitCode: null, + exitSignal: null, + durationMs: Date.now() - startedAt, + aggregated, + timedOut, + reason: message, + }); + }); + } + }); + + return { + session, + startedAt, + pid: session.pid ?? undefined, + promise, + kill: () => killSession(session), + }; +} + export function createExecTool( defaults?: ExecToolDefaults, // biome-ignore lint/suspicious/noExplicitAny: TypeBox schema type from pi-agent-core uses a different module instance. @@ -305,6 +658,7 @@ export function createExecTool( const safeBins = resolveSafeBins(defaults?.safeBins); const notifyOnExit = defaults?.notifyOnExit !== false; const notifySessionKey = defaults?.sessionKey?.trim() || undefined; + const approvalRunningNoticeMs = resolveApprovalRunningNoticeMs(defaults?.approvalRunningNoticeMs); return { name: "exec", @@ -334,8 +688,6 @@ export function createExecTool( const maxOutput = DEFAULT_MAX_OUTPUT; const pendingMaxOutput = DEFAULT_PENDING_MAX_OUTPUT; - const startedAt = Date.now(); - const sessionId = createSessionSlug(); const warnings: string[] = []; const backgroundRequested = params.background === true; const yieldRequested = typeof params.yieldMs === "number"; @@ -385,12 +737,7 @@ export function createExecTool( .join("\n"), ); } - logInfo( - `exec: elevated command (${sessionId.slice(0, 8)}) ${truncateMiddle( - params.command, - 120, - )}`, - ); + logInfo(`exec: elevated command ${truncateMiddle(params.command, 120)}`); } const configuredHost = defaults?.host ?? "sandbox"; const requestedHost = normalizeExecHost(params.host) ?? null; @@ -431,7 +778,6 @@ export function createExecTool( workdir = resolveWorkdir(rawWorkdir, warnings); } - const { shell, args: shellArgs } = getShellConfig(); const baseEnv = coerceEnv(process.env); const mergedEnv = params.env ? { ...baseEnv, ...params.env } : baseEnv; const env = sandbox @@ -500,69 +846,158 @@ export function createExecTool( applyPathPrepend(nodeEnv, defaultPathPrepend, { requireExisting: true }); } const requiresAsk = hostAsk === "always" || hostAsk === "on-miss"; - - let approvedByAsk = false; - let approvalDecision: "allow-once" | "allow-always" | null = null; - if (requiresAsk) { - const decisionResult = (await callGatewayTool( - "exec.approval.request", - { timeoutMs: 130_000 }, - { - command: params.command, + const commandText = params.command; + const invokeTimeoutMs = Math.max( + 10_000, + (typeof params.timeout === "number" ? params.timeout : defaultTimeoutSec) * 1000 + 5_000, + ); + const buildInvokeParams = ( + approvedByAsk: boolean, + approvalDecision: "allow-once" | "allow-always" | null, + runId?: string, + ) => + ({ + nodeId, + command: "system.run", + params: { + command: argv, + rawCommand: params.command, cwd: workdir, - host: "node", - security: hostSecurity, - ask: hostAsk, + env: nodeEnv, + timeoutMs: typeof params.timeout === "number" ? params.timeout * 1000 : undefined, agentId: defaults?.agentId, - resolvedPath: null, - sessionKey: defaults?.sessionKey ?? null, - timeoutMs: 120_000, + sessionKey: defaults?.sessionKey, + approved: approvedByAsk, + approvalDecision: approvalDecision ?? undefined, + runId: runId ?? undefined, }, - )) as { decision?: string } | null; - const decision = - decisionResult && typeof decisionResult === "object" - ? (decisionResult.decision ?? null) - : null; + idempotencyKey: crypto.randomUUID(), + }) satisfies Record; - if (decision === "deny") { - throw new Error("exec denied: user denied"); - } - if (!decision) { - if (askFallback === "full") { + if (requiresAsk) { + const approvalId = crypto.randomUUID(); + const approvalSlug = createApprovalSlug(approvalId); + const expiresAtMs = Date.now() + DEFAULT_APPROVAL_TIMEOUT_MS; + const contextKey = `exec:${approvalId}`; + const noticeSeconds = Math.max(1, Math.round(approvalRunningNoticeMs / 1000)); + const warningText = warnings.length ? `${warnings.join("\n")}\n\n` : ""; + + void (async () => { + let decision: string | null = null; + try { + const decisionResult = (await callGatewayTool( + "exec.approval.request", + { timeoutMs: DEFAULT_APPROVAL_REQUEST_TIMEOUT_MS }, + { + id: approvalId, + command: commandText, + cwd: workdir, + host: "node", + security: hostSecurity, + ask: hostAsk, + agentId: defaults?.agentId, + resolvedPath: null, + sessionKey: defaults?.sessionKey ?? null, + timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS, + }, + )) as { decision?: string } | null; + decision = + decisionResult && typeof decisionResult === "object" + ? (decisionResult.decision ?? null) + : null; + } catch { + emitExecSystemEvent( + `Exec denied (node=${nodeId} id=${approvalId}, approval-request-failed): ${commandText}`, + { sessionKey: notifySessionKey, contextKey }, + ); + return; + } + + let approvedByAsk = false; + let approvalDecision: "allow-once" | "allow-always" | null = null; + let deniedReason: string | null = null; + + if (decision === "deny") { + deniedReason = "user-denied"; + } else if (!decision) { + if (askFallback === "full") { + approvedByAsk = true; + approvalDecision = "allow-once"; + } else if (askFallback === "allowlist") { + // Defer allowlist enforcement to the node host. + } else { + deniedReason = "approval-timeout"; + } + } else if (decision === "allow-once") { approvedByAsk = true; approvalDecision = "allow-once"; - } else if (askFallback === "allowlist") { - // Defer allowlist enforcement to the node host. - } else { - throw new Error("exec denied: approval required (approval UI not available)"); + } else if (decision === "allow-always") { + approvedByAsk = true; + approvalDecision = "allow-always"; } - } - if (decision === "allow-once") { - approvedByAsk = true; - approvalDecision = "allow-once"; - } - if (decision === "allow-always") { - approvedByAsk = true; - approvalDecision = "allow-always"; - } + + if (deniedReason) { + emitExecSystemEvent( + `Exec denied (node=${nodeId} id=${approvalId}, ${deniedReason}): ${commandText}`, + { sessionKey: notifySessionKey, contextKey }, + ); + return; + } + + let runningTimer: NodeJS.Timeout | null = null; + if (approvalRunningNoticeMs > 0) { + runningTimer = setTimeout(() => { + emitExecSystemEvent( + `Exec running (node=${nodeId} id=${approvalId}, >${noticeSeconds}s): ${commandText}`, + { sessionKey: notifySessionKey, contextKey }, + ); + }, approvalRunningNoticeMs); + } + + try { + await callGatewayTool( + "node.invoke", + { timeoutMs: invokeTimeoutMs }, + buildInvokeParams(approvedByAsk, approvalDecision, approvalId), + ); + } catch { + emitExecSystemEvent( + `Exec denied (node=${nodeId} id=${approvalId}, invoke-failed): ${commandText}`, + { sessionKey: notifySessionKey, contextKey }, + ); + } finally { + if (runningTimer) clearTimeout(runningTimer); + } + })(); + + return { + content: [ + { + type: "text", + text: + `${warningText}Approval required (id ${approvalSlug}). ` + + "Approve to run; updates will arrive after completion.", + }, + ], + details: { + status: "approval-pending", + approvalId, + approvalSlug, + expiresAtMs, + host: "node", + command: commandText, + cwd: workdir, + nodeId, + }, + }; } - const invokeParams: Record = { - nodeId, - command: "system.run", - params: { - command: argv, - rawCommand: params.command, - cwd: workdir, - env: nodeEnv, - timeoutMs: typeof params.timeout === "number" ? params.timeout * 1000 : undefined, - agentId: defaults?.agentId, - sessionKey: defaults?.sessionKey, - approved: approvedByAsk, - approvalDecision: approvalDecision ?? undefined, - }, - idempotencyKey: crypto.randomUUID(), - }; - const raw = (await callGatewayTool("node.invoke", {}, invokeParams)) as { + + const startedAt = Date.now(); + const raw = (await callGatewayTool( + "node.invoke", + { timeoutMs: invokeTimeoutMs }, + buildInvokeParams(false, null), + )) as { payload?: { exitCode?: number; timedOut?: boolean; @@ -620,64 +1055,183 @@ export function createExecTool( hostSecurity === "allowlist" && (!analysis.ok || !allowlistSatisfied)); - let approvedByAsk = false; if (requiresAsk) { - const decisionResult = (await callGatewayTool( - "exec.approval.request", - { timeoutMs: 130_000 }, - { - command: params.command, - cwd: workdir, - host: "gateway", - security: hostSecurity, - ask: hostAsk, - agentId: defaults?.agentId, - resolvedPath: analysis.segments[0]?.resolution?.resolvedPath ?? null, - sessionKey: defaults?.sessionKey ?? null, - timeoutMs: 120_000, - }, - )) as { decision?: string } | null; - const decision = - decisionResult && typeof decisionResult === "object" - ? (decisionResult.decision ?? null) - : null; + const approvalId = crypto.randomUUID(); + const approvalSlug = createApprovalSlug(approvalId); + const expiresAtMs = Date.now() + DEFAULT_APPROVAL_TIMEOUT_MS; + const contextKey = `exec:${approvalId}`; + const resolvedPath = analysis.segments[0]?.resolution?.resolvedPath ?? null; + const noticeSeconds = Math.max(1, Math.round(approvalRunningNoticeMs / 1000)); + const commandText = params.command; + const effectiveTimeout = + typeof params.timeout === "number" ? params.timeout : defaultTimeoutSec; + const warningText = warnings.length ? `${warnings.join("\n")}\n\n` : ""; - if (decision === "deny") { - throw new Error("exec denied: user denied"); - } - if (!decision) { - if (askFallback === "full") { - approvedByAsk = true; - } else if (askFallback === "allowlist") { - if (!analysis.ok || !allowlistSatisfied) { - throw new Error("exec denied: approval required (approval UI not available)"); - } - approvedByAsk = true; - } else { - throw new Error("exec denied: approval required (approval UI not available)"); + void (async () => { + let decision: string | null = null; + try { + const decisionResult = (await callGatewayTool( + "exec.approval.request", + { timeoutMs: DEFAULT_APPROVAL_REQUEST_TIMEOUT_MS }, + { + id: approvalId, + command: commandText, + cwd: workdir, + host: "gateway", + security: hostSecurity, + ask: hostAsk, + agentId: defaults?.agentId, + resolvedPath, + sessionKey: defaults?.sessionKey ?? null, + timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS, + }, + )) as { decision?: string } | null; + decision = + decisionResult && typeof decisionResult === "object" + ? (decisionResult.decision ?? null) + : null; + } catch { + emitExecSystemEvent( + `Exec denied (gateway id=${approvalId}, approval-request-failed): ${commandText}`, + { sessionKey: notifySessionKey, contextKey }, + ); + return; } - } - if (decision === "allow-once") { - approvedByAsk = true; - } - if (decision === "allow-always") { - approvedByAsk = true; - if (hostSecurity === "allowlist") { - for (const segment of analysis.segments) { - const pattern = segment.resolution?.resolvedPath ?? ""; - if (pattern) { - addAllowlistEntry(approvals.file, defaults?.agentId, pattern); + + let approvedByAsk = false; + let deniedReason: string | null = null; + + if (decision === "deny") { + deniedReason = "user-denied"; + } else if (!decision) { + if (askFallback === "full") { + approvedByAsk = true; + } else if (askFallback === "allowlist") { + if (!analysis.ok || !allowlistSatisfied) { + deniedReason = "approval-timeout (allowlist-miss)"; + } else { + approvedByAsk = true; + } + } else { + deniedReason = "approval-timeout"; + } + } else if (decision === "allow-once") { + approvedByAsk = true; + } else if (decision === "allow-always") { + approvedByAsk = true; + if (hostSecurity === "allowlist") { + for (const segment of analysis.segments) { + const pattern = segment.resolution?.resolvedPath ?? ""; + if (pattern) { + addAllowlistEntry(approvals.file, defaults?.agentId, pattern); + } } } } - } + + if ( + hostSecurity === "allowlist" && + (!analysis.ok || !allowlistSatisfied) && + !approvedByAsk + ) { + deniedReason = deniedReason ?? "allowlist-miss"; + } + + if (deniedReason) { + emitExecSystemEvent( + `Exec denied (gateway id=${approvalId}, ${deniedReason}): ${commandText}`, + { sessionKey: notifySessionKey, contextKey }, + ); + return; + } + + if (allowlistMatches.length > 0) { + const seen = new Set(); + for (const match of allowlistMatches) { + if (seen.has(match.pattern)) continue; + seen.add(match.pattern); + recordAllowlistUse( + approvals.file, + defaults?.agentId, + match, + commandText, + resolvedPath ?? undefined, + ); + } + } + + let run: ExecProcessHandle | null = null; + try { + run = await runExecProcess({ + command: commandText, + workdir, + env, + sandbox: undefined, + containerWorkdir: null, + usePty: params.pty === true && !sandbox, + warnings, + maxOutput, + pendingMaxOutput, + notifyOnExit: false, + scopeKey: defaults?.scopeKey, + sessionKey: notifySessionKey, + timeoutSec: effectiveTimeout, + }); + } catch { + emitExecSystemEvent( + `Exec denied (gateway id=${approvalId}, spawn-failed): ${commandText}`, + { sessionKey: notifySessionKey, contextKey }, + ); + return; + } + + markBackgrounded(run.session); + + let runningTimer: NodeJS.Timeout | null = null; + if (approvalRunningNoticeMs > 0) { + runningTimer = setTimeout(() => { + emitExecSystemEvent( + `Exec running (gateway id=${approvalId}, session=${run?.session.id}, >${noticeSeconds}s): ${commandText}`, + { sessionKey: notifySessionKey, contextKey }, + ); + }, approvalRunningNoticeMs); + } + + const outcome = await run.promise; + if (runningTimer) clearTimeout(runningTimer); + const output = normalizeNotifyOutput( + tail(outcome.aggregated || "", DEFAULT_NOTIFY_TAIL_CHARS), + ); + const exitLabel = outcome.timedOut ? "timeout" : `code ${outcome.exitCode ?? "?"}`; + const summary = output + ? `Exec finished (gateway id=${approvalId}, session=${run.session.id}, ${exitLabel})\n${output}` + : `Exec finished (gateway id=${approvalId}, session=${run.session.id}, ${exitLabel})`; + emitExecSystemEvent(summary, { sessionKey: notifySessionKey, contextKey }); + })(); + + return { + content: [ + { + type: "text", + text: + `${warningText}` + + `Approval required (id ${approvalSlug}). ` + + "Approve to run; updates will arrive after completion.", + }, + ], + details: { + status: "approval-pending", + approvalId, + approvalSlug, + expiresAtMs, + host: "gateway", + command: params.command, + cwd: workdir, + }, + }; } - if ( - hostSecurity === "allowlist" && - (!analysis.ok || !allowlistSatisfied) && - !approvedByAsk - ) { + if (hostSecurity === "allowlist" && (!analysis.ok || !allowlistSatisfied)) { throw new Error("exec denied: allowlist miss"); } @@ -697,241 +1251,70 @@ export function createExecTool( } } - const usePty = params.pty === true && !sandbox; - let child: ChildProcessWithoutNullStreams | null = null; - let pty: PtyHandle | null = null; - let stdin: SessionStdin | undefined; - - if (sandbox) { - child = spawn( - "docker", - buildDockerExecArgs({ - containerName: sandbox.containerName, - command: params.command, - workdir: containerWorkdir ?? sandbox.containerWorkdir, - env, - tty: params.pty === true, - }), - { - cwd: workdir, - env: process.env, - detached: process.platform !== "win32", - stdio: ["pipe", "pipe", "pipe"], - windowsHide: true, - }, - ) as ChildProcessWithoutNullStreams; - stdin = child.stdin; - } else if (usePty) { - const ptyModule = (await import("@lydell/node-pty")) as unknown as { - spawn?: PtySpawn; - default?: { spawn?: PtySpawn }; - }; - const spawnPty = ptyModule.spawn ?? ptyModule.default?.spawn; - if (!spawnPty) { - throw new Error("PTY support is unavailable (node-pty spawn not found)."); - } - pty = spawnPty(shell, [...shellArgs, params.command], { - cwd: workdir, - env, - name: process.env.TERM ?? "xterm-256color", - cols: 120, - rows: 30, - }); - stdin = { - destroyed: false, - write: (data, cb) => { - try { - pty?.write(data); - cb?.(null); - } catch (err) { - cb?.(err as Error); - } - }, - end: () => { - try { - const eof = process.platform === "win32" ? "\x1a" : "\x04"; - pty?.write(eof); - } catch { - // ignore EOF errors - } - }, - }; - } else { - child = spawn(shell, [...shellArgs, params.command], { - cwd: workdir, - env, - detached: process.platform !== "win32", - stdio: ["pipe", "pipe", "pipe"], - windowsHide: true, - }) as ChildProcessWithoutNullStreams; - stdin = child.stdin; - } - - const session = { - id: sessionId, - command: params.command, - scopeKey: defaults?.scopeKey, - sessionKey: notifySessionKey, - notifyOnExit, - exitNotified: false, - child: child ?? undefined, - stdin, - pid: child?.pid ?? pty?.pid, - startedAt, - cwd: workdir, - maxOutputChars: maxOutput, - pendingMaxOutputChars: pendingMaxOutput, - totalOutputChars: 0, - pendingStdout: [], - pendingStderr: [], - pendingStdoutChars: 0, - pendingStderrChars: 0, - aggregated: "", - tail: "", - exited: false, - exitCode: undefined as number | null | undefined, - exitSignal: undefined as NodeJS.Signals | number | null | undefined, - truncated: false, - backgrounded: false, - }; - addSession(session); - - let settled = false; - let yielded = false; - let yieldTimer: NodeJS.Timeout | null = null; - let timeoutTimer: NodeJS.Timeout | null = null; - let timeoutFinalizeTimer: NodeJS.Timeout | null = null; - let timedOut = false; - const timeoutFinalizeMs = 1000; - let rejectFn: ((err: Error) => void) | null = null; - - const settle = (fn: () => void) => { - if (settled) return; - settled = true; - fn(); - }; - const effectiveTimeout = typeof params.timeout === "number" ? params.timeout : defaultTimeoutSec; - const finalizeTimeout = () => { - if (session.exited) return; - markExited(session, null, "SIGKILL", "failed"); - maybeNotifyOnExit(session, "failed"); - if (settled || !rejectFn) return; - const aggregated = session.aggregated.trim(); - const reason = `Command timed out after ${effectiveTimeout} seconds`; - const message = aggregated ? `${aggregated}\n\n${reason}` : reason; - settle(() => rejectFn?.(new Error(message))); - }; + const warningText = warnings.length ? `${warnings.join("\n")}\n\n` : ""; + const usePty = params.pty === true && !sandbox; + const run = await runExecProcess({ + command: params.command, + workdir, + env, + sandbox, + containerWorkdir, + usePty, + warnings, + maxOutput, + pendingMaxOutput, + notifyOnExit, + scopeKey: defaults?.scopeKey, + sessionKey: notifySessionKey, + timeoutSec: effectiveTimeout, + onUpdate, + }); + + let yielded = false; + let yieldTimer: NodeJS.Timeout | null = null; // Tool-call abort should not kill backgrounded sessions; timeouts still must. const onAbortSignal = () => { - if (yielded || session.backgrounded) return; - killSession(session); - }; - - // Timeouts always terminate, even for backgrounded sessions. - const onTimeout = () => { - timedOut = true; - killSession(session); - if (!timeoutFinalizeTimer) { - timeoutFinalizeTimer = setTimeout(() => { - finalizeTimeout(); - }, timeoutFinalizeMs); - } + if (yielded || run.session.backgrounded) return; + run.kill(); }; if (signal?.aborted) onAbortSignal(); else if (signal) { signal.addEventListener("abort", onAbortSignal, { once: true }); } - if (effectiveTimeout > 0) { - timeoutTimer = setTimeout(() => { - onTimeout(); - }, effectiveTimeout * 1000); - } - - const emitUpdate = () => { - if (!onUpdate) return; - const tailText = session.tail || session.aggregated; - const warningText = warnings.length ? `${warnings.join("\n")}\n\n` : ""; - onUpdate({ - content: [{ type: "text", text: warningText + (tailText || "") }], - details: { - status: "running", - sessionId, - pid: session.pid ?? undefined, - startedAt, - cwd: session.cwd, - tail: session.tail, - }, - }); - }; - - const handleStdout = (data: string) => { - const str = sanitizeBinaryOutput(data.toString()); - for (const chunk of chunkString(str)) { - appendOutput(session, "stdout", chunk); - emitUpdate(); - } - }; - - const handleStderr = (data: string) => { - const str = sanitizeBinaryOutput(data.toString()); - for (const chunk of chunkString(str)) { - appendOutput(session, "stderr", chunk); - emitUpdate(); - } - }; - - if (pty) { - const cursorResponse = buildCursorPositionResponse(); - pty.onData((data) => { - const raw = data.toString(); - const { cleaned, requests } = stripDsrRequests(raw); - if (requests > 0) { - for (let i = 0; i < requests; i += 1) { - pty.write(cursorResponse); - } - } - handleStdout(cleaned); - }); - } else if (child) { - child.stdout.on("data", handleStdout); - child.stderr.on("data", handleStderr); - } return new Promise>((resolve, reject) => { - rejectFn = reject; - const resolveRunning = () => { - settle(() => - resolve({ - content: [ - { - type: "text", - text: - `${warnings.length ? `${warnings.join("\n")}\n\n` : ""}` + - `Command still running (session ${sessionId}, pid ${session.pid ?? "n/a"}). ` + - "Use process (list/poll/log/write/kill/clear/remove) for follow-up.", - }, - ], - details: { - status: "running", - sessionId, - pid: session.pid ?? undefined, - startedAt, - cwd: session.cwd, - tail: session.tail, + const resolveRunning = () => + resolve({ + content: [ + { + type: "text", + text: + `${warningText}` + + `Command still running (session ${run.session.id}, pid ${ + run.session.pid ?? "n/a" + }). ` + + "Use process (list/poll/log/write/kill/clear/remove) for follow-up.", }, - }), - ); - }; + ], + details: { + status: "running", + sessionId: run.session.id, + pid: run.session.pid ?? undefined, + startedAt: run.startedAt, + cwd: run.session.cwd, + tail: run.session.tail, + }, + }); const onYieldNow = () => { if (yieldTimer) clearTimeout(yieldTimer); - if (settled) return; + if (yielded) return; yielded = true; - markBackgrounded(session); + markBackgrounded(run.session); resolveRunning(); }; @@ -940,87 +1323,43 @@ export function createExecTool( onYieldNow(); } else { yieldTimer = setTimeout(() => { - if (settled) return; + if (yielded) return; yielded = true; - markBackgrounded(session); + markBackgrounded(run.session); resolveRunning(); }, yieldWindow); } } - const handleExit = (code: number | null, exitSignal: NodeJS.Signals | number | null) => { - if (yieldTimer) clearTimeout(yieldTimer); - if (timeoutTimer) clearTimeout(timeoutTimer); - if (timeoutFinalizeTimer) clearTimeout(timeoutFinalizeTimer); - const durationMs = Date.now() - startedAt; - const wasSignal = exitSignal != null; - const isSuccess = code === 0 && !wasSignal && !signal?.aborted && !timedOut; - const status: "completed" | "failed" = isSuccess ? "completed" : "failed"; - markExited(session, code, exitSignal, status); - maybeNotifyOnExit(session, status); - if (!session.child && session.stdin) { - session.stdin.destroyed = true; - } - - if (yielded || session.backgrounded) return; - - const aggregated = session.aggregated.trim(); - if (!isSuccess) { - const reason = timedOut - ? `Command timed out after ${effectiveTimeout} seconds` - : wasSignal && exitSignal - ? `Command aborted by signal ${exitSignal}` - : code === null - ? "Command aborted before exit code was captured" - : `Command exited with code ${code}`; - const message = aggregated ? `${aggregated}\n\n${reason}` : reason; - settle(() => reject(new Error(message))); - return; - } - - settle(() => + run.promise + .then((outcome) => { + if (yieldTimer) clearTimeout(yieldTimer); + if (yielded || run.session.backgrounded) return; + if (outcome.status === "failed") { + reject(new Error(outcome.reason ?? "Command failed.")); + return; + } resolve({ content: [ { type: "text", - text: - `${warnings.length ? `${warnings.join("\n")}\n\n` : ""}` + - (aggregated || "(no output)"), + text: `${warningText}${outcome.aggregated || "(no output)"}`, }, ], details: { status: "completed", - exitCode: code ?? 0, - durationMs, - aggregated, - cwd: session.cwd, + exitCode: outcome.exitCode ?? 0, + durationMs: outcome.durationMs, + aggregated: outcome.aggregated, + cwd: run.session.cwd, }, - }), - ); - }; - - // `exit` can fire before stdio fully flushes (notably on Windows). - // `close` waits for streams to close, so aggregated output is complete. - if (pty) { - pty.onExit((event) => { - const rawSignal = event.signal ?? null; - const normalizedSignal = rawSignal === 0 ? null : rawSignal; - handleExit(event.exitCode ?? null, normalizedSignal); - }); - } else if (child) { - child.once("close", (code, exitSignal) => { - handleExit(code, exitSignal); - }); - - child.once("error", (err) => { + }); + }) + .catch((err) => { if (yieldTimer) clearTimeout(yieldTimer); - if (timeoutTimer) clearTimeout(timeoutTimer); - if (timeoutFinalizeTimer) clearTimeout(timeoutFinalizeTimer); - markExited(session, null, null, "failed"); - maybeNotifyOnExit(session, "failed"); - settle(() => reject(err)); + if (yielded || run.session.backgrounded) return; + reject(err as Error); }); - } }); }, }; diff --git a/src/agents/pi-tools.ts b/src/agents/pi-tools.ts index 677f391da..c61e3b694 100644 --- a/src/agents/pi-tools.ts +++ b/src/agents/pi-tools.ts @@ -84,6 +84,7 @@ function resolveExecConfig(cfg: ClawdbotConfig | undefined) { pathPrepend: globalExec?.pathPrepend, backgroundMs: globalExec?.backgroundMs, timeoutSec: globalExec?.timeoutSec, + approvalRunningNoticeMs: globalExec?.approvalRunningNoticeMs, cleanupMs: globalExec?.cleanupMs, notifyOnExit: globalExec?.notifyOnExit, applyPatch: globalExec?.applyPatch, @@ -219,6 +220,8 @@ export function createClawdbotCodingTools(options?: { messageProvider: options?.messageProvider, backgroundMs: options?.exec?.backgroundMs ?? execConfig.backgroundMs, timeoutSec: options?.exec?.timeoutSec ?? execConfig.timeoutSec, + approvalRunningNoticeMs: + options?.exec?.approvalRunningNoticeMs ?? execConfig.approvalRunningNoticeMs, notifyOnExit: options?.exec?.notifyOnExit ?? execConfig.notifyOnExit, sandbox: sandbox ? { diff --git a/src/config/schema.ts b/src/config/schema.ts index 324f7189b..cd22e94d9 100644 --- a/src/config/schema.ts +++ b/src/config/schema.ts @@ -161,6 +161,7 @@ const FIELD_LABELS: Record = { "tools.exec.applyPatch.enabled": "Enable apply_patch", "tools.exec.applyPatch.allowModels": "apply_patch Model Allowlist", "tools.exec.notifyOnExit": "Exec Notify On Exit", + "tools.exec.approvalRunningNoticeMs": "Exec Approval Running Notice (ms)", "tools.exec.host": "Exec Host", "tools.exec.security": "Exec Security", "tools.exec.ask": "Exec Ask", diff --git a/src/config/types.tools.ts b/src/config/types.tools.ts index 7ce292935..cb0d28f4b 100644 --- a/src/config/types.tools.ts +++ b/src/config/types.tools.ts @@ -137,6 +137,8 @@ export type ExecToolConfig = { backgroundMs?: number; /** Default timeout (seconds) before auto-killing exec commands. */ timeoutSec?: number; + /** Emit a running notice (ms) when approval-backed exec runs long (default: 10000, 0 = off). */ + approvalRunningNoticeMs?: number; /** How long to keep finished sessions in memory (ms). */ cleanupMs?: number; /** Emit a system event and heartbeat when a backgrounded exec exits. */ diff --git a/src/config/zod-schema.agent-runtime.ts b/src/config/zod-schema.agent-runtime.ts index ba6eb1db9..b01fd264b 100644 --- a/src/config/zod-schema.agent-runtime.ts +++ b/src/config/zod-schema.agent-runtime.ts @@ -260,6 +260,7 @@ export const AgentToolsSchema = z safeBins: z.array(z.string()).optional(), backgroundMs: z.number().int().positive().optional(), timeoutSec: z.number().int().positive().optional(), + approvalRunningNoticeMs: z.number().int().nonnegative().optional(), cleanupMs: z.number().int().positive().optional(), notifyOnExit: z.boolean().optional(), applyPatch: z diff --git a/src/gateway/exec-approval-manager.ts b/src/gateway/exec-approval-manager.ts index 7e2540496..5e51f2abc 100644 --- a/src/gateway/exec-approval-manager.ts +++ b/src/gateway/exec-approval-manager.ts @@ -33,11 +33,15 @@ type PendingEntry = { export class ExecApprovalManager { private pending = new Map(); - create(request: ExecApprovalRequestPayload, timeoutMs: number): ExecApprovalRecord { + create( + request: ExecApprovalRequestPayload, + timeoutMs: number, + id?: string | null, + ): ExecApprovalRecord { const now = Date.now(); - const id = randomUUID(); + const resolvedId = id && id.trim().length > 0 ? id.trim() : randomUUID(); const record: ExecApprovalRecord = { - id, + id: resolvedId, request, createdAtMs: now, expiresAtMs: now + timeoutMs, diff --git a/src/gateway/protocol/schema/exec-approvals.ts b/src/gateway/protocol/schema/exec-approvals.ts index ae2caf77c..9f1a25d38 100644 --- a/src/gateway/protocol/schema/exec-approvals.ts +++ b/src/gateway/protocol/schema/exec-approvals.ts @@ -89,6 +89,7 @@ export const ExecApprovalsNodeSetParamsSchema = Type.Object( export const ExecApprovalRequestParamsSchema = Type.Object( { + id: Type.Optional(NonEmptyString), command: NonEmptyString, cwd: Type.Optional(Type.String()), host: Type.Optional(Type.String()), diff --git a/src/gateway/server-methods/exec-approval.test.ts b/src/gateway/server-methods/exec-approval.test.ts index 1d0328532..4682e6e1a 100644 --- a/src/gateway/server-methods/exec-approval.test.ts +++ b/src/gateway/server-methods/exec-approval.test.ts @@ -60,4 +60,120 @@ describe("exec approval handlers", () => { ); expect(broadcasts.some((entry) => entry.event === "exec.approval.resolved")).toBe(true); }); + + it("accepts explicit approval ids", async () => { + const manager = new ExecApprovalManager(); + const handlers = createExecApprovalHandlers(manager); + const broadcasts: Array<{ event: string; payload: unknown }> = []; + + const respond = vi.fn(); + const context = { + broadcast: (event: string, payload: unknown) => { + broadcasts.push({ event, payload }); + }, + }; + + const requestPromise = handlers["exec.approval.request"]({ + params: { + id: "approval-123", + command: "echo ok", + cwd: "/tmp", + host: "gateway", + timeoutMs: 2000, + }, + respond, + context: context as unknown as Parameters< + (typeof handlers)["exec.approval.request"] + >[0]["context"], + client: null, + req: { id: "req-1", type: "req", method: "exec.approval.request" }, + isWebchatConnect: noop, + }); + + const requested = broadcasts.find((entry) => entry.event === "exec.approval.requested"); + const id = (requested?.payload as { id?: string })?.id ?? ""; + expect(id).toBe("approval-123"); + + const resolveRespond = vi.fn(); + await handlers["exec.approval.resolve"]({ + params: { id, decision: "allow-once" }, + respond: resolveRespond, + context: context as unknown as Parameters< + (typeof handlers)["exec.approval.resolve"] + >[0]["context"], + client: { connect: { client: { id: "cli", displayName: "CLI" } } }, + req: { id: "req-2", type: "req", method: "exec.approval.resolve" }, + isWebchatConnect: noop, + }); + + await requestPromise; + expect(respond).toHaveBeenCalledWith( + true, + expect.objectContaining({ id: "approval-123", decision: "allow-once" }), + undefined, + ); + }); + + it("rejects duplicate approval ids", async () => { + const manager = new ExecApprovalManager(); + const handlers = createExecApprovalHandlers(manager); + const respondA = vi.fn(); + const respondB = vi.fn(); + const broadcasts: Array<{ event: string; payload: unknown }> = []; + const context = { + broadcast: (event: string, payload: unknown) => { + broadcasts.push({ event, payload }); + }, + }; + + const requestPromise = handlers["exec.approval.request"]({ + params: { + id: "dup-1", + command: "echo ok", + }, + respond: respondA, + context: context as unknown as Parameters< + (typeof handlers)["exec.approval.request"] + >[0]["context"], + client: null, + req: { id: "req-1", type: "req", method: "exec.approval.request" }, + isWebchatConnect: noop, + }); + + await handlers["exec.approval.request"]({ + params: { + id: "dup-1", + command: "echo again", + }, + respond: respondB, + context: context as unknown as Parameters< + (typeof handlers)["exec.approval.request"] + >[0]["context"], + client: null, + req: { id: "req-2", type: "req", method: "exec.approval.request" }, + isWebchatConnect: noop, + }); + + expect(respondB).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ message: "approval id already pending" }), + ); + + const requested = broadcasts.find((entry) => entry.event === "exec.approval.requested"); + const id = (requested?.payload as { id?: string })?.id ?? ""; + const resolveRespond = vi.fn(); + await handlers["exec.approval.resolve"]({ + params: { id, decision: "deny" }, + respond: resolveRespond, + context: context as unknown as Parameters< + (typeof handlers)["exec.approval.resolve"] + >[0]["context"], + client: { connect: { client: { id: "cli", displayName: "CLI" } } }, + req: { id: "req-3", type: "req", method: "exec.approval.resolve" }, + isWebchatConnect: noop, + }); + + await requestPromise; + }); }); diff --git a/src/gateway/server-methods/exec-approval.ts b/src/gateway/server-methods/exec-approval.ts index 683c70a1e..8fb13cea0 100644 --- a/src/gateway/server-methods/exec-approval.ts +++ b/src/gateway/server-methods/exec-approval.ts @@ -26,6 +26,7 @@ export function createExecApprovalHandlers(manager: ExecApprovalManager): Gatewa return; } const p = params as { + id?: string; command: string; cwd?: string; host?: string; @@ -37,6 +38,15 @@ export function createExecApprovalHandlers(manager: ExecApprovalManager): Gatewa timeoutMs?: number; }; const timeoutMs = typeof p.timeoutMs === "number" ? p.timeoutMs : 120_000; + const explicitId = typeof p.id === "string" && p.id.trim().length > 0 ? p.id.trim() : null; + if (explicitId && manager.getSnapshot(explicitId)) { + respond( + false, + undefined, + errorShape(ErrorCodes.INVALID_REQUEST, "approval id already pending"), + ); + return; + } const request = { command: p.command, cwd: p.cwd ?? null, @@ -47,7 +57,7 @@ export function createExecApprovalHandlers(manager: ExecApprovalManager): Gatewa resolvedPath: p.resolvedPath ?? null, sessionKey: p.sessionKey ?? null, }; - const record = manager.create(request, timeoutMs); + const record = manager.create(request, timeoutMs, explicitId); context.broadcast( "exec.approval.requested", { diff --git a/src/node-host/runner.ts b/src/node-host/runner.ts index 09a148ca4..0d4892e35 100644 --- a/src/node-host/runner.ts +++ b/src/node-host/runner.ts @@ -57,6 +57,7 @@ type SystemRunParams = { sessionKey?: string | null; approved?: boolean | null; approvalDecision?: string | null; + runId?: string | null; }; type SystemWhichParams = { @@ -583,7 +584,7 @@ async function handleInvoke( const ask = approvals.agent.ask; const autoAllowSkills = approvals.agent.autoAllowSkills; const sessionKey = params.sessionKey?.trim() || "node"; - const runId = crypto.randomUUID(); + const runId = params.runId?.trim() || crypto.randomUUID(); const env = sanitizeEnv(params.env ?? undefined); const analysis = rawCommand ? analyzeShellCommand({ command: rawCommand, cwd: params.cwd ?? undefined, env }) @@ -803,17 +804,6 @@ async function handleInvoke( return; } - await sendNodeEvent( - client, - "exec.started", - buildExecEventPayload({ - sessionKey, - runId, - host: "node", - command: cmdText, - }), - ); - const result = await runCommand( argv, params.cwd?.trim() || undefined,