mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-26 04:15:48 -06:00
fix: stop cron exec approval spam (#128031)
Co-authored-by: RoboClaw <309084314+roboclaw-bot@users.noreply.github.com> Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com>
This commit is contained in:
@@ -228,6 +228,7 @@ type HostExecApprovalParams = {
|
||||
turnSourceAccountId?: string;
|
||||
turnSourceThreadId?: string | number;
|
||||
approvalReviewerDeviceIds?: string[];
|
||||
trigger?: string;
|
||||
requireDeliveryRoute?: boolean;
|
||||
suppressDelivery?: boolean;
|
||||
};
|
||||
@@ -338,7 +339,10 @@ async function buildHostApprovalDecisionParams(
|
||||
runId: params.runId,
|
||||
toolCallId: params.toolCallId,
|
||||
requireDeliveryRoute: params.requireDeliveryRoute,
|
||||
suppressDelivery: params.suppressDelivery,
|
||||
// Cron has no live reviewer. Register for audit/fallback resolution without
|
||||
// exposing an operator-visible request that the scheduled run cannot answer.
|
||||
suppressDelivery:
|
||||
params.suppressDelivery === true || params.trigger === "cron" ? true : undefined,
|
||||
approvalReviewerDeviceIds: params.approvalReviewerDeviceIds,
|
||||
...buildExecApprovalTurnSourceContext(params),
|
||||
};
|
||||
|
||||
@@ -984,6 +984,7 @@ export async function processGatewayAllowlist(
|
||||
sessionId: params.sessionId,
|
||||
runId: params.runId,
|
||||
toolCallId: params.toolCallId,
|
||||
trigger: params.trigger,
|
||||
approvalReviewerDeviceIds: params.approvalReviewerDeviceId
|
||||
? [params.approvalReviewerDeviceId]
|
||||
: undefined,
|
||||
|
||||
@@ -218,6 +218,7 @@ export async function executeNodeHostCommand(
|
||||
workdir: prepared.cwd,
|
||||
host: "node",
|
||||
nodeId: target.nodeId,
|
||||
trigger: params.trigger,
|
||||
toolCallId: params.toolCallId,
|
||||
security: hostSecurity,
|
||||
ask: hostAsk,
|
||||
|
||||
@@ -386,6 +386,16 @@ function mockNoApprovalRouteRegistration() {
|
||||
|
||||
const requireRecord = createRequireRecord("record", "expected-label");
|
||||
|
||||
function requireExecApprovalRequestCall() {
|
||||
const requestCall = vi
|
||||
.mocked(callGatewayTool)
|
||||
.mock.calls.find(([method]) => method === "exec.approval.request");
|
||||
return {
|
||||
params: requireRecord(requestCall?.[2], "approval request params"),
|
||||
options: requireRecord(requestCall?.[3], "approval request options"),
|
||||
};
|
||||
}
|
||||
|
||||
function expectRecordFields(
|
||||
record: Record<string, unknown> | undefined,
|
||||
expected: Record<string, unknown>,
|
||||
@@ -1562,6 +1572,7 @@ describe("exec approvals", () => {
|
||||
resolveRegistration?.({ status: "accepted", id: "approval-id" });
|
||||
const result = await executePromise;
|
||||
expect(result.details.status).toBe("approval-pending");
|
||||
expect(requireExecApprovalRequestCall().params.suppressDelivery).toBeUndefined();
|
||||
expect(calls[0]).toBe("exec.approval.request");
|
||||
expect(calls).toContain("exec.approval.waitDecision");
|
||||
});
|
||||
@@ -1609,12 +1620,9 @@ describe("exec approvals", () => {
|
||||
expect(result.details.status).toBe("completed");
|
||||
expect(getResultText(result)).toContain("cron-ok");
|
||||
|
||||
const approvalRequestCall = vi
|
||||
.mocked(callGatewayTool)
|
||||
.mock.calls.find(([method]) => method === "exec.approval.request");
|
||||
expect(requireRecord(approvalRequestCall?.[3], "approval request options").expectFinal).toBe(
|
||||
false,
|
||||
);
|
||||
const approvalRequest = requireExecApprovalRequestCall();
|
||||
expect(approvalRequest.options.expectFinal).toBe(false);
|
||||
expect(approvalRequest.params.suppressDelivery).toBe(true);
|
||||
expect(
|
||||
vi
|
||||
.mocked(callGatewayTool)
|
||||
@@ -1682,6 +1690,7 @@ describe("exec approvals", () => {
|
||||
|
||||
expect(result.details.status).toBe("completed");
|
||||
expect(getResultText(result)).toContain("cron-node-ok");
|
||||
expect(requireExecApprovalRequestCall().params.suppressDelivery).toBe(true);
|
||||
const systemRun = requireRecord(systemRunInvoke, "system.run invoke");
|
||||
expect(systemRun.command).toBe("system.run");
|
||||
const params = requireRecord(systemRun.params, "system.run params");
|
||||
@@ -1713,6 +1722,54 @@ describe("exec approvals", () => {
|
||||
command: "echo cron-denied",
|
||||
}),
|
||||
).rejects.toThrow("Automation runs cannot wait for interactive exec approval");
|
||||
expect(requireExecApprovalRequestCall().params.suppressDelivery).toBe(true);
|
||||
});
|
||||
|
||||
it("denies node cron no-route approvals when askFallback is deny", async () => {
|
||||
await writeExecApprovalsConfig({
|
||||
version: 1,
|
||||
defaults: { security: "full", ask: "always", askFallback: "deny" },
|
||||
agents: {},
|
||||
});
|
||||
vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => {
|
||||
if (method === "exec.approval.request") {
|
||||
return { id: "approval-id", decision: null };
|
||||
}
|
||||
if (method === "exec.approval.waitDecision") {
|
||||
return { decision: null };
|
||||
}
|
||||
if (method === "node.invoke") {
|
||||
const invoke = requireRecord(params, "node invoke");
|
||||
if (invoke.command === "system.run.prepare") {
|
||||
return buildPreparedSystemRunPayload(params);
|
||||
}
|
||||
}
|
||||
return { ok: true };
|
||||
});
|
||||
|
||||
const tool = createExecTool({
|
||||
host: "node",
|
||||
ask: "always",
|
||||
security: "full",
|
||||
trigger: "cron",
|
||||
approvalRunningNoticeMs: 0,
|
||||
});
|
||||
|
||||
await expect(
|
||||
tool.execute("call-cron-node-denied", {
|
||||
command: "echo cron-node-denied",
|
||||
}),
|
||||
).rejects.toThrow("Automation runs cannot wait for interactive exec approval");
|
||||
expect(requireExecApprovalRequestCall().params.suppressDelivery).toBe(true);
|
||||
expect(
|
||||
vi
|
||||
.mocked(callGatewayTool)
|
||||
.mock.calls.some(
|
||||
([method, _opts, params]) =>
|
||||
method === "node.invoke" &&
|
||||
requireRecord(params, "node invoke").command === "system.run",
|
||||
),
|
||||
).toBe(false);
|
||||
});
|
||||
});
|
||||
/* oxlint-disable max-lines -- TODO: split this grandfathered oversized file. */
|
||||
|
||||
@@ -861,6 +861,57 @@ describe("handlePendingApprovalRequest", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("expires suppressed requests instead of retaining a hidden turn-source route", async () => {
|
||||
const manager = new ExecApprovalManager();
|
||||
const record = manager.create(
|
||||
{
|
||||
command: "echo cron",
|
||||
turnSourceChannel: "discord",
|
||||
turnSourceAccountId: "default",
|
||||
},
|
||||
60_000,
|
||||
"approval-suppressed-turn-source",
|
||||
);
|
||||
const decisionPromise = manager.register(record, 60_000);
|
||||
const respond = vi.fn();
|
||||
const broadcast = vi.fn();
|
||||
const deliverRequest = vi.fn(() => true);
|
||||
|
||||
await handlePendingApprovalRequest({
|
||||
manager,
|
||||
record,
|
||||
decisionPromise,
|
||||
respond,
|
||||
context: {
|
||||
broadcast,
|
||||
hasExecApprovalClients: () => true,
|
||||
} as unknown as GatewayRequestContext,
|
||||
requestEventName: "exec.approval.requested",
|
||||
requestEvent: {
|
||||
id: record.id,
|
||||
request: record.request,
|
||||
createdAtMs: record.createdAtMs,
|
||||
expiresAtMs: record.expiresAtMs,
|
||||
},
|
||||
twoPhase: true,
|
||||
suppressDelivery: true,
|
||||
deliverRequest,
|
||||
});
|
||||
|
||||
expect(broadcast).not.toHaveBeenCalled();
|
||||
expect(deliverRequest).not.toHaveBeenCalled();
|
||||
expect(hasApprovalTurnSourceRouteMock).not.toHaveBeenCalled();
|
||||
expect(manager.getSnapshot(record.id)).toMatchObject({
|
||||
resolvedBy: "no-approval-route",
|
||||
terminalReason: "no-route",
|
||||
});
|
||||
expect(respond).toHaveBeenCalledWith(
|
||||
true,
|
||||
expect.objectContaining({ id: record.id, decision: null }),
|
||||
undefined,
|
||||
);
|
||||
});
|
||||
|
||||
it("does not target no-device browser UI approvals to unrelated approval-scoped clients", async () => {
|
||||
hasApprovalTurnSourceRouteMock.mockReturnValueOnce(false);
|
||||
const manager = new ExecApprovalManager();
|
||||
|
||||
@@ -347,6 +347,7 @@ export async function handlePendingApprovalRequest<
|
||||
// A turn-source route can approve without an active approval client, so keep
|
||||
// the record alive when the originating channel/account can still receive it.
|
||||
const hasTurnSourceRoute =
|
||||
!suppressDelivery &&
|
||||
!hasApprovalClients &&
|
||||
!delivered &&
|
||||
hasApprovalTurnSourceRoute({
|
||||
|
||||
Reference in New Issue
Block a user