diff --git a/src/agents/openclaw-tools.camera.test.ts b/src/agents/openclaw-tools.camera.test.ts index 70692506fea..2edbb9b451d 100644 --- a/src/agents/openclaw-tools.camera.test.ts +++ b/src/agents/openclaw-tools.camera.test.ts @@ -17,7 +17,6 @@ vi.mock("../media/image-ops.js", () => ({ let createOpenClawTools: typeof import("./openclaw-tools.js").createOpenClawTools; const NODE_ID = "mac-1"; -const BASE_RUN_INPUT = { action: "run", node: NODE_ID, command: ["echo", "hi"] } as const; const JPG_PAYLOAD = { format: "jpg", base64: "aGVsbG8=", @@ -138,43 +137,6 @@ function setupNodeInvokeMock(params: { }); } -function createSystemRunPreparePayload(cwd: string | null) { - return { - payload: { - plan: { - argv: ["echo", "hi"], - cwd, - commandText: "echo hi", - agentId: null, - sessionKey: null, - }, - }, - }; -} - -function setupSystemRunGateway(params: { - onRunInvoke: (invokeParams: unknown) => GatewayMockResult | Promise; - onApprovalRequest?: (approvalParams: unknown) => GatewayMockResult | Promise; - prepareCwd?: string | null; -}) { - callGateway.mockImplementation(async ({ method, params: gatewayParams }: GatewayCall) => { - if (method === "node.list") { - return mockNodeList({ commands: ["system.run"] }); - } - if (method === "node.invoke") { - const command = (gatewayParams as { command?: string } | undefined)?.command; - if (command === "system.run.prepare") { - return createSystemRunPreparePayload(params.prepareCwd ?? null); - } - return await params.onRunInvoke(gatewayParams); - } - if (method === "exec.approval.request" && params.onApprovalRequest) { - return await params.onApprovalRequest(gatewayParams); - } - return unexpectedGatewayMethod(method); - }); -} - function setupPhotosLatestMock(params?: { remoteIp?: string }) { setupNodeInvokeMock({ ...(params?.remoteIp ? { remoteIp: params.remoteIp } : {}), @@ -604,119 +566,6 @@ describe("nodes device_status and device_info", () => { }); }); -describe("nodes run", () => { - it("passes invoke and command timeouts", async () => { - setupSystemRunGateway({ - prepareCwd: "/tmp", - onRunInvoke: (invokeParams) => { - expect(invokeParams).toMatchObject({ - nodeId: NODE_ID, - command: "system.run", - timeoutMs: 45_000, - params: { - command: ["echo", "hi"], - cwd: "/tmp", - env: { FOO: "bar" }, - timeoutMs: 12_000, - }, - }); - return { - payload: { stdout: "", stderr: "", exitCode: 0, success: true }, - }; - }, - }); - - await executeNodes({ - ...BASE_RUN_INPUT, - cwd: "/tmp", - env: ["FOO=bar"], - commandTimeoutMs: 12_000, - invokeTimeoutMs: 45_000, - }); - }); - - it("requests approval and retries with allow-once decision", async () => { - let invokeCalls = 0; - let approvalId: string | null = null; - setupSystemRunGateway({ - onRunInvoke: (invokeParams) => { - invokeCalls += 1; - if (invokeCalls === 1) { - throw new Error("SYSTEM_RUN_DENIED: approval required"); - } - expect(invokeParams).toMatchObject({ - nodeId: NODE_ID, - command: "system.run", - params: { - command: ["echo", "hi"], - runId: approvalId, - approved: true, - approvalDecision: "allow-once", - }, - }); - return { payload: { stdout: "", stderr: "", exitCode: 0, success: true } }; - }, - onApprovalRequest: (approvalParams) => { - expect(approvalParams).toMatchObject({ - id: expect.any(String), - systemRunPlan: expect.objectContaining({ - argv: ["echo", "hi"], - commandText: "echo hi", - }), - nodeId: NODE_ID, - host: "node", - timeoutMs: 120_000, - }); - approvalId = - typeof (approvalParams as { id?: unknown } | undefined)?.id === "string" - ? ((approvalParams as { id: string }).id ?? null) - : null; - return { decision: "allow-once" }; - }, - }); - - await executeNodes(BASE_RUN_INPUT); - expect(invokeCalls).toBe(2); - }); - - it("fails with user denied when approval decision is deny", async () => { - setupSystemRunGateway({ - onRunInvoke: () => { - throw new Error("SYSTEM_RUN_DENIED: approval required"); - }, - onApprovalRequest: () => { - return { decision: "deny" }; - }, - }); - - await expect(executeNodes(BASE_RUN_INPUT)).rejects.toThrow("exec denied: user denied"); - }); - - it("fails closed for timeout and invalid approval decisions", async () => { - setupSystemRunGateway({ - onRunInvoke: () => { - throw new Error("SYSTEM_RUN_DENIED: approval required"); - }, - onApprovalRequest: () => { - return {}; - }, - }); - await expect(executeNodes(BASE_RUN_INPUT)).rejects.toThrow("exec denied: approval timed out"); - - setupSystemRunGateway({ - onRunInvoke: () => { - throw new Error("SYSTEM_RUN_DENIED: approval required"); - }, - onApprovalRequest: () => { - return { decision: "allow-never" }; - }, - }); - await expect(executeNodes(BASE_RUN_INPUT)).rejects.toThrow( - "exec denied: invalid approval decision", - ); - }); -}); - describe("nodes invoke", () => { it("allows metadata-only camera.list via generic invoke", async () => { setupNodeInvokeMock({ diff --git a/src/agents/tools/nodes-tool.test.ts b/src/agents/tools/nodes-tool.test.ts index eafa74aba7f..f7176bf6273 100644 --- a/src/agents/tools/nodes-tool.test.ts +++ b/src/agents/tools/nodes-tool.test.ts @@ -8,8 +8,6 @@ const gatewayMocks = vi.hoisted(() => ({ const nodeUtilsMocks = vi.hoisted(() => ({ resolveNodeId: vi.fn(async () => "node-1"), resolveNode: vi.fn(async () => ({ nodeId: "node-1", remoteIp: "127.0.0.1" })), - listNodes: vi.fn(async () => [] as Array<{ nodeId: string; commands?: string[] }>), - resolveNodeIdFromList: vi.fn(() => "node-1"), })); const nodesCameraMocks = vi.hoisted(() => ({ @@ -48,8 +46,6 @@ vi.mock("./gateway.js", () => ({ vi.mock("./nodes-utils.js", () => ({ resolveNodeId: nodeUtilsMocks.resolveNodeId, resolveNode: nodeUtilsMocks.resolveNode, - listNodes: nodeUtilsMocks.listNodes, - resolveNodeIdFromList: nodeUtilsMocks.resolveNodeIdFromList, })); vi.mock("../../cli/nodes-camera.js", () => ({ @@ -77,8 +73,6 @@ async function loadFreshNodesToolModuleForTest() { vi.doMock("./nodes-utils.js", () => ({ resolveNodeId: nodeUtilsMocks.resolveNodeId, resolveNode: nodeUtilsMocks.resolveNode, - listNodes: nodeUtilsMocks.listNodes, - resolveNodeIdFromList: nodeUtilsMocks.resolveNodeIdFromList, })); vi.doMock("../../cli/nodes-camera.js", () => ({ cameraTempPath: nodesCameraMocks.cameraTempPath, @@ -148,50 +142,15 @@ describe("createNodesTool screen_record duration guardrails", () => { ); }); - it("omits rawCommand when preparing wrapped argv execution", async () => { - nodeUtilsMocks.listNodes.mockResolvedValue([ - { - nodeId: "node-1", - commands: ["system.run"], - }, - ]); - gatewayMocks.callGatewayTool.mockImplementation(async (_method, _opts, payload) => { - if (payload?.command === "system.run.prepare") { - return { - payload: { - plan: { - argv: ["bash", "-lc", "echo hi"], - cwd: null, - commandText: 'bash -lc "echo hi"', - commandPreview: "echo hi", - agentId: null, - sessionKey: null, - }, - }, - }; - } - if (payload?.command === "system.run") { - return { payload: { ok: true } }; - } - throw new Error(`unexpected command: ${String(payload?.command)}`); - }); + it("rejects the removed run action", async () => { const tool = createNodesTool(); - await tool.execute("call-1", { - action: "run", - node: "macbook", - command: ["bash", "-lc", "echo hi"], - }); - - const prepareCall = gatewayMocks.callGatewayTool.mock.calls.find( - (call) => call[2]?.command === "system.run.prepare", - )?.[2]; - expect(prepareCall).toBeTruthy(); - expect(prepareCall?.params).toMatchObject({ - command: ["bash", "-lc", "echo hi"], - agentId: "main", - }); - expect(prepareCall?.params).not.toHaveProperty("rawCommand"); + await expect( + tool.execute("call-1", { + action: "run", + node: "macbook", + }), + ).rejects.toThrow("Unknown action: run"); }); it("returns camera snaps via details.media.mediaUrls", async () => { gatewayMocks.callGatewayTool.mockResolvedValue({ payload: { ok: true } }); @@ -385,4 +344,16 @@ describe("createNodesTool screen_record duration guardrails", () => { { scopes: ["operator.write"] }, ); }); + + it("blocks invokeCommand system.run so exec stays the only shell path", async () => { + const tool = createNodesTool(); + + await expect( + tool.execute("call-1", { + action: "invoke", + node: "macbook", + invokeCommand: "system.run", + }), + ).rejects.toThrow('invokeCommand "system.run" is reserved for shell execution'); + }); }); diff --git a/src/cli/argv.test.ts b/src/cli/argv.test.ts index 002105b768d..a7ec2b529c4 100644 --- a/src/cli/argv.test.ts +++ b/src/cli/argv.test.ts @@ -125,7 +125,7 @@ describe("argv helpers", () => { }, { name: "help after -- terminator", - argv: ["node", "openclaw", "nodes", "run", "--", "git", "--help"], + argv: ["node", "openclaw", "nodes", "invoke", "--", "device.status", "--help"], expected: false, }, { diff --git a/src/cli/nodes-cli.coverage.test.ts b/src/cli/nodes-cli.coverage.test.ts index 99a21580100..3cd9fee440b 100644 --- a/src/cli/nodes-cli.coverage.test.ts +++ b/src/cli/nodes-cli.coverage.test.ts @@ -1,7 +1,5 @@ import { Command } from "commander"; import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; -import type { ExecApprovalsFile } from "../infra/exec-approvals.js"; -import { buildSystemRunPreparePayload } from "../test-utils/system-run-prepare-payload.js"; import { createCliRuntimeCapture } from "./test-runtime-capture.js"; type NodeInvokeCall = { @@ -15,17 +13,6 @@ type NodeInvokeCall = { }; let lastNodeInvokeCall: NodeInvokeCall | null = null; -let lastApprovalRequestCall: { params?: Record } | null = null; -let localExecApprovalsFile: ExecApprovalsFile = { version: 1, agents: {} }; -let nodeExecApprovalsFile: ExecApprovalsFile = { - version: 1, - defaults: { - security: "allowlist", - ask: "on-miss", - askFallback: "deny", - }, - agents: {}, -}; const callGateway = vi.fn(async (opts: NodeInvokeCall) => { if (opts.method === "node.list") { @@ -44,16 +31,6 @@ const callGateway = vi.fn(async (opts: NodeInvokeCall) => { } if (opts.method === "node.invoke") { lastNodeInvokeCall = opts; - const command = opts.params?.command; - if (command === "system.run.prepare") { - const params = (opts.params?.params ?? {}) as { - command?: unknown[]; - rawCommand?: unknown; - cwd?: unknown; - agentId?: unknown; - }; - return buildSystemRunPreparePayload(params); - } return { payload: { stdout: "", @@ -64,24 +41,12 @@ const callGateway = vi.fn(async (opts: NodeInvokeCall) => { }, }; } - if (opts.method === "exec.approvals.node.get") { - return { - path: "/tmp/exec-approvals.json", - exists: true, - hash: "hash", - file: nodeExecApprovalsFile, - }; - } - if (opts.method === "exec.approval.request") { - lastApprovalRequestCall = opts as { params?: Record }; - return { decision: "allow-once" }; - } return { ok: true }; }); const randomIdempotencyKey = vi.fn(() => "rk_test"); -const { defaultRuntime, resetRuntimeCapture } = createCliRuntimeCapture(); +const { runtimeErrors, defaultRuntime, resetRuntimeCapture } = createCliRuntimeCapture(); vi.mock("../gateway/call.js", () => ({ callGateway: (opts: unknown) => callGateway(opts as NodeInvokeCall), @@ -93,20 +58,6 @@ vi.mock("../runtime.js", async (importOriginal) => ({ defaultRuntime, })); -vi.mock("../config/config.js", () => ({ - loadConfig: () => ({}), -})); - -vi.mock("../infra/exec-approvals.js", async () => { - const actual = await vi.importActual( - "../infra/exec-approvals.js", - ); - return { - ...actual, - loadExecApprovals: () => localExecApprovalsFile, - }; -}); - describe("nodes-cli coverage", () => { let registerNodesCli: (program: Command) => void; let sharedProgram: Command; @@ -119,8 +70,6 @@ describe("nodes-cli coverage", () => { return last; }; - const getApprovalRequestCall = () => lastApprovalRequestCall; - const runNodesCommand = async (args: string[]) => { await sharedProgram.parseAsync(args, { from: "user" }); return getNodeInvokeCall(); @@ -138,128 +87,23 @@ describe("nodes-cli coverage", () => { callGateway.mockClear(); randomIdempotencyKey.mockClear(); lastNodeInvokeCall = null; - lastApprovalRequestCall = null; - localExecApprovalsFile = { version: 1, agents: {} }; - nodeExecApprovalsFile = { - version: 1, - defaults: { - security: "allowlist", - ask: "on-miss", - askFallback: "deny", - }, - agents: {}, - }; }); - it("invokes system.run with parsed params", async () => { - const invoke = await runNodesCommand([ - "nodes", - "run", - "--node", - "mac-1", - "--cwd", - "/tmp", - "--env", - "FOO=bar", - "--command-timeout", - "1200", - "--needs-screen-recording", - "--invoke-timeout", - "5000", - "echo", - "hi", - ]); - - expect(invoke).toBeTruthy(); - expect(invoke?.params?.idempotencyKey).toBe("rk_test"); - expect(invoke?.params?.command).toBe("system.run"); - expect(invoke?.params?.params).toEqual({ - command: ["echo", "hi"], - rawCommand: "echo hi", - cwd: "/tmp", - env: { FOO: "bar" }, - timeoutMs: 1200, - needsScreenRecording: true, - agentId: "main", - approved: true, - approvalDecision: "allow-once", - runId: expect.any(String), - }); - expect(invoke?.params?.timeoutMs).toBe(5000); - const approval = getApprovalRequestCall(); - expect(approval?.params?.["systemRunPlan"]).toEqual({ - argv: ["echo", "hi"], - cwd: "/tmp", - commandText: "echo hi", - commandPreview: null, - agentId: "main", - sessionKey: null, + it("does not register the removed run wrapper", async () => { + await expect( + sharedProgram.parseAsync(["nodes", "run", "--node", "mac-1"], { from: "user" }), + ).rejects.toMatchObject({ + code: "commander.unknownCommand", }); }); - it("invokes system.run with raw command", async () => { - const invoke = await runNodesCommand([ - "nodes", - "run", - "--agent", - "main", - "--node", - "mac-1", - "--raw", - "echo hi", - ]); - - expect(invoke).toBeTruthy(); - expect(invoke?.params?.idempotencyKey).toBe("rk_test"); - expect(invoke?.params?.command).toBe("system.run"); - expect(invoke?.params?.params).toMatchObject({ - command: ["/bin/sh", "-lc", "echo hi"], - rawCommand: '/bin/sh -lc "echo hi"', - agentId: "main", - approved: true, - approvalDecision: "allow-once", - runId: expect.any(String), - }); - const approval = getApprovalRequestCall(); - expect(approval?.params?.["systemRunPlan"]).toEqual({ - argv: ["/bin/sh", "-lc", "echo hi"], - cwd: null, - commandText: '/bin/sh -lc "echo hi"', - commandPreview: "echo hi", - agentId: "main", - sessionKey: null, - }); - }); - - it("inherits ask=off from local exec approvals when tools.exec.ask is unset", async () => { - localExecApprovalsFile = { - version: 1, - defaults: { - security: "allowlist", - ask: "off", - askFallback: "deny", - }, - agents: {}, - }; - nodeExecApprovalsFile = { - version: 1, - defaults: { - security: "allowlist", - askFallback: "deny", - }, - agents: {}, - }; - - const invoke = await runNodesCommand(["nodes", "run", "--node", "mac-1", "echo", "hi"]); - - expect(invoke).toBeTruthy(); - expect(invoke?.params?.command).toBe("system.run"); - expect(invoke?.params?.params).toMatchObject({ - command: ["echo", "hi"], - approved: false, - }); - expect(invoke?.params?.params).not.toHaveProperty("approvalDecision"); - expect(getApprovalRequestCall()).toBeNull(); + it("blocks system.run on nodes invoke", async () => { + await expect( + sharedProgram.parseAsync(["nodes", "invoke", "--node", "mac-1", "--command", "system.run"], { + from: "user", + }), + ).rejects.toThrow("__exit__:1"); + expect(runtimeErrors.at(-1)).toContain('command "system.run" is reserved for shell execution'); }); it("invokes system.notify with provided fields", async () => { diff --git a/src/cli/nodes-cli/register.invoke.nodes-run-approval-timeout.test.ts b/src/cli/nodes-cli/register.invoke.approval-transport-timeout.test.ts similarity index 82% rename from src/cli/nodes-cli/register.invoke.nodes-run-approval-timeout.test.ts rename to src/cli/nodes-cli/register.invoke.approval-transport-timeout.test.ts index f297f72c16b..0eab090a5de 100644 --- a/src/cli/nodes-cli/register.invoke.nodes-run-approval-timeout.test.ts +++ b/src/cli/nodes-cli/register.invoke.approval-transport-timeout.test.ts @@ -1,19 +1,18 @@ import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { DEFAULT_EXEC_APPROVAL_TIMEOUT_MS } from "../../infra/exec-approvals.js"; -import { parseTimeoutMs } from "../nodes-run.js"; +import { parseTimeoutMs } from "../parse-timeout.js"; /** * Regression test for #12098: - * `openclaw nodes run` times out after 35s because the CLI transport timeout - * (35s default) is shorter than the exec approval timeout (120s). The - * exec.approval.request call must use a transport timeout at least as long - * as the approval timeout so the gateway has enough time to collect the - * user's decision. + * `exec.approval.request` times out after 35s when the CLI transport timeout + * is shorter than the exec approval timeout (120s). The transport timeout + * must be at least as long as the approval timeout so the gateway has enough + * time to collect the user's decision. * * The root cause: callGatewayCli reads opts.timeout for the transport timeout. - * Before the fix, nodes run called callGatewayCli("exec.approval.request", opts, ...) - * without overriding opts.timeout, so the 35s CLI default raced against the - * 120s approval wait on the gateway side. The CLI always lost. + * Before the fix, node exec flows called callGatewayCli("exec.approval.request", + * opts, ...) without overriding opts.timeout, so the 35s CLI default raced + * against the 120s approval wait on the gateway side. The CLI always lost. * * The fix: override the transport timeout for exec.approval.request to be at * least approvalTimeoutMs + 10_000. @@ -32,7 +31,7 @@ vi.mock("../progress.js", () => ({ withProgress: (_opts: unknown, fn: () => unknown) => fn(), })); -describe("nodes run: approval transport timeout (#12098)", () => { +describe("exec approval transport timeout (#12098)", () => { let callGatewayCli: typeof import("./rpc.js").callGatewayCli; beforeAll(async () => { @@ -100,7 +99,7 @@ describe("nodes run: approval transport timeout (#12098)", () => { // parseTimeoutMs returns undefined for garbage input, ?? 0 ensures // Math.max picks the approval floor instead of producing NaN const transportTimeoutMs = Math.max(parseTimeoutMs("foo") ?? 0, approvalTimeoutMs + 10_000); - expect(transportTimeoutMs).toBe(130_000); + expect(transportTimeoutMs).toBe(approvalTimeoutMs + 10_000); await callGatewayCli( "exec.approval.request", @@ -110,6 +109,6 @@ describe("nodes run: approval transport timeout (#12098)", () => { ); const callOpts = callGatewaySpy.mock.calls[0][0]; - expect(callOpts.timeoutMs).toBe(130_000); + expect(callOpts.timeoutMs).toBe(approvalTimeoutMs + 10_000); }); });