mirror of
https://github.com/farcasclaudiu/openclaw.git
synced 2026-06-29 11:02:12 +03:00
fix(nodes-tool): add exec approval flow for agent tool run action (#4726)
Merged via /review-pr -> /prepare-pr -> /merge-pr. Prepared head SHA: b8ed4f1b6e4b1363c791dad153bf224b13f87ed3 Co-authored-by: rmorse <853547+rmorse@users.noreply.github.com> Co-authored-by: steipete <58493+steipete@users.noreply.github.com> Reviewed-by: @steipete
This commit is contained in:
@@ -35,6 +35,7 @@ Docs: https://docs.openclaw.ai
|
|||||||
- Agents/Codex: allow `gpt-5.3-codex-spark` in forward-compat fallback, live model filtering, and thinking presets, and fix model-picker recognition for spark. (#14990) Thanks @L-U-C-K-Y.
|
- Agents/Codex: allow `gpt-5.3-codex-spark` in forward-compat fallback, live model filtering, and thinking presets, and fix model-picker recognition for spark. (#14990) Thanks @L-U-C-K-Y.
|
||||||
- OpenAI Codex/Auth: bridge OpenClaw OAuth profiles into `pi` `auth.json` so model discovery and models-list registry resolution can use Codex OAuth credentials. (#15184) Thanks @loiie45e.
|
- OpenAI Codex/Auth: bridge OpenClaw OAuth profiles into `pi` `auth.json` so model discovery and models-list registry resolution can use Codex OAuth credentials. (#15184) Thanks @loiie45e.
|
||||||
- Agents/Transcript policy: sanitize OpenAI/Codex tool-call ids during transcript policy normalization to prevent invalid tool-call identifiers from propagating into session history. (#15279) Thanks @divisonofficer.
|
- Agents/Transcript policy: sanitize OpenAI/Codex tool-call ids during transcript policy normalization to prevent invalid tool-call identifiers from propagating into session history. (#15279) Thanks @divisonofficer.
|
||||||
|
- Agents/Nodes: harden node exec approval decision handling in the `nodes` tool run path by failing closed on unexpected approval decisions, and add regression coverage for approval-required retry/deny/timeout flows. (#4726) Thanks @rmorse.
|
||||||
- Models/Codex: resolve configured `openai-codex/gpt-5.3-codex-spark` through forward-compat fallback during `models list`, so it is not incorrectly tagged as missing when runtime resolution succeeds. (#15174) Thanks @loiie45e.
|
- Models/Codex: resolve configured `openai-codex/gpt-5.3-codex-spark` through forward-compat fallback during `models list`, so it is not incorrectly tagged as missing when runtime resolution succeeds. (#15174) Thanks @loiie45e.
|
||||||
- macOS Voice Wake: fix a crash in trigger trimming for CJK/Unicode transcripts by matching and slicing on original-string ranges instead of transformed-string indices. (#11052) Thanks @Flash-LHR.
|
- macOS Voice Wake: fix a crash in trigger trimming for CJK/Unicode transcripts by matching and slicing on original-string ranges instead of transformed-string indices. (#11052) Thanks @Flash-LHR.
|
||||||
- Heartbeat: prevent scheduler silent-death races during runner reloads, preserve retry cooldown backoff under wake bursts, and prioritize user/action wake causes over interval/retry reasons when coalescing. (#15108) Thanks @joeykrug.
|
- Heartbeat: prevent scheduler silent-death races during runner reloads, preserve retry cooldown backoff under wake bursts, and prioritize user/action wake causes over interval/retry reasons when coalescing. (#15108) Thanks @joeykrug.
|
||||||
|
|||||||
@@ -132,4 +132,125 @@ describe("nodes run", () => {
|
|||||||
invokeTimeoutMs: 45_000,
|
invokeTimeoutMs: 45_000,
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("requests approval and retries with allow-once decision", async () => {
|
||||||
|
let invokeCalls = 0;
|
||||||
|
callGateway.mockImplementation(async ({ method, params }) => {
|
||||||
|
if (method === "node.list") {
|
||||||
|
return { nodes: [{ nodeId: "mac-1", commands: ["system.run"] }] };
|
||||||
|
}
|
||||||
|
if (method === "node.invoke") {
|
||||||
|
invokeCalls += 1;
|
||||||
|
if (invokeCalls === 1) {
|
||||||
|
throw new Error("SYSTEM_RUN_DENIED: approval required");
|
||||||
|
}
|
||||||
|
expect(params).toMatchObject({
|
||||||
|
nodeId: "mac-1",
|
||||||
|
command: "system.run",
|
||||||
|
params: {
|
||||||
|
command: ["echo", "hi"],
|
||||||
|
approved: true,
|
||||||
|
approvalDecision: "allow-once",
|
||||||
|
},
|
||||||
|
});
|
||||||
|
return { payload: { stdout: "", stderr: "", exitCode: 0, success: true } };
|
||||||
|
}
|
||||||
|
if (method === "exec.approval.request") {
|
||||||
|
expect(params).toMatchObject({
|
||||||
|
command: "echo hi",
|
||||||
|
host: "node",
|
||||||
|
timeoutMs: 120_000,
|
||||||
|
});
|
||||||
|
return { decision: "allow-once" };
|
||||||
|
}
|
||||||
|
throw new Error(`unexpected method: ${String(method)}`);
|
||||||
|
});
|
||||||
|
|
||||||
|
const tool = createOpenClawTools().find((candidate) => candidate.name === "nodes");
|
||||||
|
if (!tool) {
|
||||||
|
throw new Error("missing nodes tool");
|
||||||
|
}
|
||||||
|
|
||||||
|
await tool.execute("call1", {
|
||||||
|
action: "run",
|
||||||
|
node: "mac-1",
|
||||||
|
command: ["echo", "hi"],
|
||||||
|
});
|
||||||
|
expect(invokeCalls).toBe(2);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("fails with user denied when approval decision is deny", async () => {
|
||||||
|
callGateway.mockImplementation(async ({ method }) => {
|
||||||
|
if (method === "node.list") {
|
||||||
|
return { nodes: [{ nodeId: "mac-1", commands: ["system.run"] }] };
|
||||||
|
}
|
||||||
|
if (method === "node.invoke") {
|
||||||
|
throw new Error("SYSTEM_RUN_DENIED: approval required");
|
||||||
|
}
|
||||||
|
if (method === "exec.approval.request") {
|
||||||
|
return { decision: "deny" };
|
||||||
|
}
|
||||||
|
throw new Error(`unexpected method: ${String(method)}`);
|
||||||
|
});
|
||||||
|
|
||||||
|
const tool = createOpenClawTools().find((candidate) => candidate.name === "nodes");
|
||||||
|
if (!tool) {
|
||||||
|
throw new Error("missing nodes tool");
|
||||||
|
}
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
tool.execute("call1", {
|
||||||
|
action: "run",
|
||||||
|
node: "mac-1",
|
||||||
|
command: ["echo", "hi"],
|
||||||
|
}),
|
||||||
|
).rejects.toThrow("exec denied: user denied");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("fails closed for timeout and invalid approval decisions", async () => {
|
||||||
|
const tool = createOpenClawTools().find((candidate) => candidate.name === "nodes");
|
||||||
|
if (!tool) {
|
||||||
|
throw new Error("missing nodes tool");
|
||||||
|
}
|
||||||
|
|
||||||
|
callGateway.mockImplementation(async ({ method }) => {
|
||||||
|
if (method === "node.list") {
|
||||||
|
return { nodes: [{ nodeId: "mac-1", commands: ["system.run"] }] };
|
||||||
|
}
|
||||||
|
if (method === "node.invoke") {
|
||||||
|
throw new Error("SYSTEM_RUN_DENIED: approval required");
|
||||||
|
}
|
||||||
|
if (method === "exec.approval.request") {
|
||||||
|
return {};
|
||||||
|
}
|
||||||
|
throw new Error(`unexpected method: ${String(method)}`);
|
||||||
|
});
|
||||||
|
await expect(
|
||||||
|
tool.execute("call1", {
|
||||||
|
action: "run",
|
||||||
|
node: "mac-1",
|
||||||
|
command: ["echo", "hi"],
|
||||||
|
}),
|
||||||
|
).rejects.toThrow("exec denied: approval timed out");
|
||||||
|
|
||||||
|
callGateway.mockImplementation(async ({ method }) => {
|
||||||
|
if (method === "node.list") {
|
||||||
|
return { nodes: [{ nodeId: "mac-1", commands: ["system.run"] }] };
|
||||||
|
}
|
||||||
|
if (method === "node.invoke") {
|
||||||
|
throw new Error("SYSTEM_RUN_DENIED: approval required");
|
||||||
|
}
|
||||||
|
if (method === "exec.approval.request") {
|
||||||
|
return { decision: "allow-never" };
|
||||||
|
}
|
||||||
|
throw new Error(`unexpected method: ${String(method)}`);
|
||||||
|
});
|
||||||
|
await expect(
|
||||||
|
tool.execute("call1", {
|
||||||
|
action: "run",
|
||||||
|
node: "mac-1",
|
||||||
|
command: ["echo", "hi"],
|
||||||
|
}),
|
||||||
|
).rejects.toThrow("exec denied: invalid approval decision");
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -436,17 +436,74 @@ export function createNodesTool(options?: {
|
|||||||
typeof params.needsScreenRecording === "boolean"
|
typeof params.needsScreenRecording === "boolean"
|
||||||
? params.needsScreenRecording
|
? params.needsScreenRecording
|
||||||
: undefined;
|
: undefined;
|
||||||
const raw = await callGatewayTool<{ payload: unknown }>("node.invoke", gatewayOpts, {
|
const runParams = {
|
||||||
|
command,
|
||||||
|
cwd,
|
||||||
|
env,
|
||||||
|
timeoutMs: commandTimeoutMs,
|
||||||
|
needsScreenRecording,
|
||||||
|
agentId,
|
||||||
|
sessionKey,
|
||||||
|
};
|
||||||
|
|
||||||
|
// First attempt without approval flags.
|
||||||
|
try {
|
||||||
|
const raw = await callGatewayTool<{ payload?: unknown }>("node.invoke", gatewayOpts, {
|
||||||
|
nodeId,
|
||||||
|
command: "system.run",
|
||||||
|
params: runParams,
|
||||||
|
timeoutMs: invokeTimeoutMs,
|
||||||
|
idempotencyKey: crypto.randomUUID(),
|
||||||
|
});
|
||||||
|
return jsonResult(raw?.payload ?? {});
|
||||||
|
} catch (firstErr) {
|
||||||
|
const msg = firstErr instanceof Error ? firstErr.message : String(firstErr);
|
||||||
|
if (!msg.includes("SYSTEM_RUN_DENIED: approval required")) {
|
||||||
|
throw firstErr;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Node requires approval – create a pending approval request on
|
||||||
|
// the gateway and wait for the user to approve/deny via the UI.
|
||||||
|
const APPROVAL_TIMEOUT_MS = 120_000;
|
||||||
|
const cmdText = command.join(" ");
|
||||||
|
const approvalResult = await callGatewayTool(
|
||||||
|
"exec.approval.request",
|
||||||
|
{ ...gatewayOpts, timeoutMs: APPROVAL_TIMEOUT_MS + 5_000 },
|
||||||
|
{
|
||||||
|
command: cmdText,
|
||||||
|
cwd,
|
||||||
|
host: "node",
|
||||||
|
agentId,
|
||||||
|
sessionKey,
|
||||||
|
timeoutMs: APPROVAL_TIMEOUT_MS,
|
||||||
|
},
|
||||||
|
);
|
||||||
|
const decisionRaw =
|
||||||
|
approvalResult && typeof approvalResult === "object"
|
||||||
|
? (approvalResult as { decision?: unknown }).decision
|
||||||
|
: undefined;
|
||||||
|
const approvalDecision =
|
||||||
|
decisionRaw === "allow-once" || decisionRaw === "allow-always" ? decisionRaw : null;
|
||||||
|
|
||||||
|
if (!approvalDecision) {
|
||||||
|
if (decisionRaw === "deny") {
|
||||||
|
throw new Error("exec denied: user denied");
|
||||||
|
}
|
||||||
|
if (decisionRaw === undefined || decisionRaw === null) {
|
||||||
|
throw new Error("exec denied: approval timed out");
|
||||||
|
}
|
||||||
|
throw new Error("exec denied: invalid approval decision");
|
||||||
|
}
|
||||||
|
|
||||||
|
// Retry with the approval decision.
|
||||||
|
const raw = await callGatewayTool<{ payload?: unknown }>("node.invoke", gatewayOpts, {
|
||||||
nodeId,
|
nodeId,
|
||||||
command: "system.run",
|
command: "system.run",
|
||||||
params: {
|
params: {
|
||||||
command,
|
...runParams,
|
||||||
cwd,
|
approved: true,
|
||||||
env,
|
approvalDecision,
|
||||||
timeoutMs: commandTimeoutMs,
|
|
||||||
needsScreenRecording,
|
|
||||||
agentId,
|
|
||||||
sessionKey,
|
|
||||||
},
|
},
|
||||||
timeoutMs: invokeTimeoutMs,
|
timeoutMs: invokeTimeoutMs,
|
||||||
idempotencyKey: crypto.randomUUID(),
|
idempotencyKey: crypto.randomUUID(),
|
||||||
|
|||||||
@@ -57,6 +57,7 @@ describe("config io write", () => {
|
|||||||
defaults: {
|
defaults: {
|
||||||
cliBackends: {
|
cliBackends: {
|
||||||
codex: {
|
codex: {
|
||||||
|
command: "codex",
|
||||||
env: {
|
env: {
|
||||||
OPENAI_API_KEY: "${OPENAI_API_KEY}",
|
OPENAI_API_KEY: "${OPENAI_API_KEY}",
|
||||||
},
|
},
|
||||||
@@ -110,9 +111,14 @@ describe("config io write", () => {
|
|||||||
configPath,
|
configPath,
|
||||||
JSON.stringify(
|
JSON.stringify(
|
||||||
{
|
{
|
||||||
channels: {
|
agents: {
|
||||||
discord: {
|
defaults: {
|
||||||
allowFrom: ["${DISCORD_USER_ID}", "123"],
|
cliBackends: {
|
||||||
|
codex: {
|
||||||
|
command: "codex",
|
||||||
|
args: ["${DISCORD_USER_ID}", "123"],
|
||||||
|
},
|
||||||
|
},
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
@@ -131,23 +137,41 @@ describe("config io write", () => {
|
|||||||
expect(snapshot.valid).toBe(true);
|
expect(snapshot.valid).toBe(true);
|
||||||
|
|
||||||
const next = structuredClone(snapshot.config);
|
const next = structuredClone(snapshot.config);
|
||||||
const allowFrom = Array.isArray(next.channels?.discord?.allowFrom)
|
const codexBackend = next.agents?.defaults?.cliBackends?.codex;
|
||||||
? next.channels?.discord?.allowFrom
|
const args = Array.isArray(codexBackend?.args) ? codexBackend?.args : [];
|
||||||
: [];
|
next.agents = {
|
||||||
next.channels = {
|
...next.agents,
|
||||||
...next.channels,
|
defaults: {
|
||||||
discord: {
|
...next.agents?.defaults,
|
||||||
...next.channels?.discord,
|
cliBackends: {
|
||||||
allowFrom: [...allowFrom, "456"],
|
...next.agents?.defaults?.cliBackends,
|
||||||
|
codex: {
|
||||||
|
...codexBackend,
|
||||||
|
command: typeof codexBackend?.command === "string" ? codexBackend.command : "codex",
|
||||||
|
args: [...args, "456"],
|
||||||
|
},
|
||||||
|
},
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
await io.writeConfigFile(next);
|
await io.writeConfigFile(next);
|
||||||
|
|
||||||
const persisted = JSON.parse(await fs.readFile(configPath, "utf-8")) as {
|
const persisted = JSON.parse(await fs.readFile(configPath, "utf-8")) as {
|
||||||
channels: { discord?: { allowFrom?: string[] } };
|
agents: {
|
||||||
|
defaults: {
|
||||||
|
cliBackends: {
|
||||||
|
codex: {
|
||||||
|
args: string[];
|
||||||
|
};
|
||||||
|
};
|
||||||
|
};
|
||||||
|
};
|
||||||
};
|
};
|
||||||
expect(persisted.channels.discord?.allowFrom).toEqual(["${DISCORD_USER_ID}", "123", "456"]);
|
expect(persisted.agents.defaults.cliBackends.codex.args).toEqual([
|
||||||
|
"${DISCORD_USER_ID}",
|
||||||
|
"123",
|
||||||
|
"456",
|
||||||
|
]);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user