fix: unify exec approval ids
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
71
src/agents/bash-tools.exec.approval-id.test.ts
Normal file
71
src/agents/bash-tools.exec.approval-id.test.ts
Normal file
@@ -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<void>((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);
|
||||
});
|
||||
});
|
||||
File diff suppressed because it is too large
Load Diff
@@ -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
|
||||
? {
|
||||
|
||||
@@ -161,6 +161,7 @@ const FIELD_LABELS: Record<string, string> = {
|
||||
"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",
|
||||
|
||||
@@ -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. */
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -33,11 +33,15 @@ type PendingEntry = {
|
||||
export class ExecApprovalManager {
|
||||
private pending = new Map<string, PendingEntry>();
|
||||
|
||||
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,
|
||||
|
||||
@@ -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()),
|
||||
|
||||
@@ -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;
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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",
|
||||
{
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user