Skip to content

Commit 824c85b

Browse files
jesse-merhijesse-merhi
committed
fix(exec): use unavailable approval decisions
1 parent d51abfa commit 824c85b

12 files changed

Lines changed: 187 additions & 53 deletions

‎apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6362,7 +6362,7 @@ public struct ExecApprovalRequestParams: Codable, Sendable {
63626362
public let security: AnyCodable?
63636363
public let ask: AnyCodable?
63646364
public let warningtext: AnyCodable?
6365-
public let alloweddecisions: [String]?
6365+
public let unavailabledecisions: [String]?
63666366
public let commandspans: [[String: AnyCodable]]?
63676367
public let agentid: AnyCodable?
63686368
public let resolvedpath: AnyCodable?
@@ -6388,7 +6388,7 @@ public struct ExecApprovalRequestParams: Codable, Sendable {
63886388
security: AnyCodable?,
63896389
ask: AnyCodable?,
63906390
warningtext: AnyCodable?,
6391-
alloweddecisions: [String]?,
6391+
unavailabledecisions: [String]?,
63926392
commandspans: [[String: AnyCodable]]?,
63936393
agentid: AnyCodable? = nil,
63946394
resolvedpath: AnyCodable?,
@@ -6413,7 +6413,7 @@ public struct ExecApprovalRequestParams: Codable, Sendable {
64136413
self.security = security
64146414
self.ask = ask
64156415
self.warningtext = warningtext
6416-
self.alloweddecisions = alloweddecisions
6416+
self.unavailabledecisions = unavailabledecisions
64176417
self.commandspans = commandspans
64186418
self.agentid = agentid
64196419
self.resolvedpath = resolvedpath
@@ -6440,7 +6440,7 @@ public struct ExecApprovalRequestParams: Codable, Sendable {
64406440
case security
64416441
case ask
64426442
case warningtext = "warningText"
6443-
case alloweddecisions = "allowedDecisions"
6443+
case unavailabledecisions = "unavailableDecisions"
64446444
case commandspans = "commandSpans"
64456445
case agentid = "agentId"
64466446
case resolvedpath = "resolvedPath"

‎packages/gateway-protocol/src/exec-approvals-validators.test.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,4 +106,27 @@ describe("exec approvals protocol validators", () => {
106106
}),
107107
).toBe(false);
108108
});
109+
110+
it("accepts only optional unavailable approval decisions", () => {
111+
expect(
112+
validateExecApprovalRequestParams({
113+
command: "echo hi",
114+
unavailableDecisions: ["allow-always"],
115+
}),
116+
).toBe(true);
117+
118+
for (const unavailableDecisions of [
119+
[],
120+
["allow-always", "allow-always"],
121+
["allow-once"],
122+
["deny"],
123+
]) {
124+
expect(
125+
validateExecApprovalRequestParams({
126+
command: "echo hi",
127+
unavailableDecisions,
128+
}),
129+
).toBe(false);
130+
}
131+
});
109132
});

‎packages/gateway-protocol/src/schema/exec-approvals.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,10 +151,10 @@ export const ExecApprovalRequestParamsSchema = Type.Object(
151151
security: Type.Optional(Type.Union([Type.String(), Type.Null()])),
152152
ask: Type.Optional(Type.Union([Type.String(), Type.Null()])),
153153
warningText: Type.Optional(Type.Union([Type.String(), Type.Null()])),
154-
allowedDecisions: Type.Optional(
155-
Type.Array(Type.String({ enum: ["allow-once", "allow-always", "deny"] }), {
154+
unavailableDecisions: Type.Optional(
155+
Type.Array(Type.String({ enum: ["allow-always"] }), {
156156
minItems: 1,
157-
maxItems: 3,
157+
maxItems: 1,
158158
}),
159159
),
160160
commandSpans: Type.Optional(

‎src/agents/bash-tools.exec-approval-request.ts‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import {
1313
} from "@openclaw/normalization-core/string-coerce";
1414
import type {
1515
ExecApprovalCommandSpan,
16-
ExecApprovalDecision,
16+
ExecApprovalUnavailableDecision,
1717
ExecAsk,
1818
ExecSecurity,
1919
SystemRunApprovalPlan,
@@ -56,7 +56,7 @@ export type RequestExecApprovalDecisionParams = {
5656
ask: ExecAsk;
5757
warningText?: string;
5858
commandSpans?: ExecApprovalCommandSpan[];
59-
allowedDecisions?: readonly ExecApprovalDecision[];
59+
unavailableDecisions?: readonly ExecApprovalUnavailableDecision[];
6060
agentId?: string;
6161
resolvedPath?: string;
6262
sessionKey?: string;
@@ -89,7 +89,9 @@ function buildExecApprovalRequestToolParams(
8989
ask: params.ask,
9090
warningText: params.warningText,
9191
commandSpans: params.commandSpans,
92-
...(params.allowedDecisions ? { allowedDecisions: params.allowedDecisions } : {}),
92+
...(params.unavailableDecisions?.length
93+
? { unavailableDecisions: params.unavailableDecisions }
94+
: {}),
9395
agentId: params.agentId,
9496
resolvedPath: params.resolvedPath,
9597
sessionKey: params.sessionKey,
@@ -210,7 +212,7 @@ type HostExecApprovalParams = {
210212
ask: ExecAsk;
211213
warningText?: string;
212214
commandSpans?: ExecApprovalCommandSpan[];
213-
allowedDecisions?: readonly ExecApprovalDecision[];
215+
unavailableDecisions?: readonly ExecApprovalUnavailableDecision[];
214216
commandHighlighting?: boolean;
215217
agentId?: string;
216218
resolvedPath?: string;
@@ -319,7 +321,7 @@ async function buildHostApprovalDecisionParams(
319321
ask: params.ask,
320322
warningText: params.warningText,
321323
commandSpans,
322-
allowedDecisions: params.allowedDecisions,
324+
unavailableDecisions: params.unavailableDecisions,
323325
...buildExecApprovalRequesterContext({
324326
agentId: params.agentId,
325327
sessionKey: params.sessionKey,

‎src/agents/bash-tools.exec-host-gateway.test.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,17 @@ const resolveExecApprovalAllowedDecisionsMock = vi.hoisted(() =>
8686
: ["allow-once", "allow-always", "deny"],
8787
),
8888
);
89+
const resolveExecApprovalUnavailableDecisionsMock = vi.hoisted(() =>
90+
vi.fn(
91+
(params?: {
92+
ask?: string | null;
93+
allowAlwaysPersistence?: { kind: string } | null;
94+
}): readonly ["allow-always"] | readonly [] =>
95+
params?.ask === "always" || params?.allowAlwaysPersistence?.kind === "one-shot"
96+
? ["allow-always"]
97+
: [],
98+
),
99+
);
89100
const buildEnforcedShellCommandMock = vi.hoisted(() =>
90101
vi.fn((): { ok: boolean; reason?: string; command?: string } => ({
91102
ok: false,
@@ -144,6 +155,7 @@ vi.mock("../infra/exec-approvals.js", async (importOriginal) => ({
144155
resolveApprovalAuditTrustPath: vi.fn(() => null),
145156
resolveAllowAlwaysPatterns: vi.fn(() => []),
146157
resolveExecApprovalAllowedDecisions: resolveExecApprovalAllowedDecisionsMock,
158+
resolveExecApprovalUnavailableDecisions: resolveExecApprovalUnavailableDecisionsMock,
147159
addAllowlistEntry: vi.fn(),
148160
addDurableCommandApproval: vi.fn(),
149161
}));
@@ -291,6 +303,7 @@ describe("processGatewayAllowlist", () => {
291303
}));
292304
detectInterpreterInlineEvalArgvMock.mockReset();
293305
detectInterpreterInlineEvalArgvMock.mockReturnValue(null);
306+
resolveExecApprovalUnavailableDecisionsMock.mockClear();
294307
buildExecApprovalPendingToolResultMock.mockReturnValue({
295308
details: { status: "approval-pending" },
296309
content: [],

‎src/agents/bash-tools.exec-host-gateway.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import {
2323
recordAllowlistMatchesUse,
2424
resolveApprovalAuditTrustPath,
2525
resolveAllowAlwaysPersistenceDecision,
26+
resolveExecApprovalUnavailableDecisions,
2627
requiresExecApproval,
2728
} from "../infra/exec-approvals.js";
2829
import type { ExecAuthorizationPlan } from "../infra/exec-authorization-plan.js";
@@ -531,6 +532,14 @@ export async function processGatewayAllowlist(
531532
ask: hostAsk,
532533
allowAlwaysPersistence: effectiveAllowAlwaysPersistence,
533534
});
535+
const approvalUnavailableDecisions = resolveExecApprovalUnavailableDecisions({
536+
ask: hostAsk,
537+
allowAlwaysPersistence: effectiveAllowAlwaysPersistence,
538+
});
539+
const unavailableDecisionRequestParams =
540+
approvalUnavailableDecisions.length > 0
541+
? { unavailableDecisions: approvalUnavailableDecisions }
542+
: {};
534543
if (requiresSecurityAuditSuppressionApproval) {
535544
params.warnings.push(
536545
"Warning: security audit suppression changes require explicit approval unless exec is running in yolo mode.",
@@ -619,7 +628,7 @@ export async function processGatewayAllowlist(
619628
host: "gateway",
620629
security: hostSecurity,
621630
ask: hostAsk,
622-
allowedDecisions: approvalAllowedDecisions,
631+
...unavailableDecisionRequestParams,
623632
commandHighlighting: params.commandHighlighting,
624633
warningText: params.warnings.join("\n").trim() || undefined,
625634
...buildExecApprovalRequesterContext({

‎src/agents/bash-tools.exec-host-node.test.ts‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ type MockAllowAlwaysPersistenceInput = Parameters<
1919
type MockAllowAlwaysPersistenceDecision =
2020
import("../infra/exec-approvals.js").AllowAlwaysPersistenceDecision;
2121
type MockExecApprovalDecision = import("../infra/exec-approvals.js").ExecApprovalDecision;
22+
type MockExecApprovalUnavailableDecision =
23+
import("../infra/exec-approvals.js").ExecApprovalUnavailableDecision;
2224
type MockAllowlistSegment = {
2325
raw?: string;
2426
resolution: null;
@@ -151,6 +153,17 @@ const resolveExecApprovalAllowedDecisionsMock = vi.hoisted(() =>
151153
: ["allow-once", "allow-always", "deny"],
152154
),
153155
);
156+
const resolveExecApprovalUnavailableDecisionsMock = vi.hoisted(() =>
157+
vi.fn(
158+
(params?: {
159+
ask?: string | null;
160+
allowAlwaysPersistence?: { kind: string } | null;
161+
}): readonly MockExecApprovalUnavailableDecision[] =>
162+
params?.ask === "always" || params?.allowAlwaysPersistence?.kind === "one-shot"
163+
? ["allow-always"]
164+
: [],
165+
),
166+
);
154167
const resolveExecHostApprovalContextMock = vi.hoisted(() =>
155168
vi.fn(() => ({
156169
approvals: { allowlist: [] as ExecAllowlistEntry[], file: { version: 1, agents: {} } },
@@ -209,6 +222,7 @@ vi.mock("../infra/exec-approvals.js", () => ({
209222
resolveAllowAlwaysPersistenceDecision: resolveAllowAlwaysPersistenceDecisionMock,
210223
resolveAllowAlwaysPatternCoverage: resolveAllowAlwaysPatternCoverageMock,
211224
resolveExecApprovalAllowedDecisions: resolveExecApprovalAllowedDecisionsMock,
225+
resolveExecApprovalUnavailableDecisions: resolveExecApprovalUnavailableDecisionsMock,
212226
resolveExecApprovalsFromFile: resolveExecApprovalsFromFileMock,
213227
maxAsk: (a: ExecAsk, b: ExecAsk): ExecAsk => {
214228
const order: Record<ExecAsk, number> = { off: 0, "on-miss": 1, always: 2 };
@@ -494,6 +508,7 @@ describe("executeNodeHostCommand", () => {
494508
patterns: [{ pattern: "/trusted/bin/tool" }],
495509
});
496510
resolveExecApprovalAllowedDecisionsMock.mockClear();
511+
resolveExecApprovalUnavailableDecisionsMock.mockClear();
497512
resolveExecHostApprovalContextMock.mockReset();
498513
resolveExecHostApprovalContextMock.mockReturnValue({
499514
approvals: { allowlist: [], file: { version: 1, agents: {} } },
@@ -1749,7 +1764,7 @@ describe("executeNodeHostCommand", () => {
17491764
patterns: [{ pattern: "/trusted/bin/tool" }],
17501765
},
17511766
});
1752-
expect(requireRegisteredApprovalRequest().allowedDecisions).toEqual(["allow-once", "deny"]);
1767+
expect(requireRegisteredApprovalRequest().unavailableDecisions).toEqual(["allow-always"]);
17531768
expect(buildExecApprovalPendingToolResultMock).toHaveBeenCalledWith(
17541769
expect.objectContaining({
17551770
allowedDecisions: ["allow-once", "deny"],
@@ -1798,7 +1813,7 @@ describe("executeNodeHostCommand", () => {
17981813
patterns: [{ pattern: "/trusted/bin/tool" }],
17991814
},
18001815
});
1801-
expect(requireRegisteredApprovalRequest().allowedDecisions).toEqual(["allow-once", "deny"]);
1816+
expect(requireRegisteredApprovalRequest().unavailableDecisions).toEqual(["allow-always"]);
18021817
});
18031818

18041819
it("offers allow-always for prepared node commands with complete node coverage", async () => {
@@ -1870,11 +1885,7 @@ describe("executeNodeHostCommand", () => {
18701885
patterns: [{ pattern: "/node/bin/git" }],
18711886
},
18721887
});
1873-
expect(requireRegisteredApprovalRequest().allowedDecisions).toEqual([
1874-
"allow-once",
1875-
"allow-always",
1876-
"deny",
1877-
]);
1888+
expect(requireRegisteredApprovalRequest().unavailableDecisions).toBeUndefined();
18781889
});
18791890

18801891
it("does not use fallback-full when node auto-review cannot parse the command", async () => {
@@ -2209,7 +2220,7 @@ describe("executeNodeHostCommand", () => {
22092220
ask: "on-miss",
22102221
allowAlwaysPersistence: { kind: "one-shot", reasons: ["unplanned"] },
22112222
});
2212-
expect(requireRegisteredApprovalRequest().allowedDecisions).toEqual(["allow-once", "deny"]);
2223+
expect(requireRegisteredApprovalRequest().unavailableDecisions).toEqual(["allow-always"]);
22132224
expect(buildExecApprovalPendingToolResultMock).toHaveBeenCalledWith(
22142225
expect.objectContaining({
22152226
allowedDecisions: ["allow-once", "deny"],

‎src/agents/bash-tools.exec-host-node.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
maxAsk,
1212
requiresExecApproval,
1313
resolveExecApprovalAllowedDecisions,
14+
resolveExecApprovalUnavailableDecisions,
1415
} from "../infra/exec-approvals.js";
1516
import { defaultExecAutoReviewer, type ExecAutoReviewInput } from "../infra/exec-auto-review.js";
1617
import {
@@ -139,6 +140,12 @@ export async function executeNodeHostCommand(
139140
ask: approvalDecisionAsk,
140141
allowAlwaysPersistence,
141142
});
143+
const unavailableDecisions = resolveExecApprovalUnavailableDecisions({
144+
ask: approvalDecisionAsk,
145+
allowAlwaysPersistence,
146+
});
147+
const unavailableDecisionRequestParams =
148+
unavailableDecisions.length > 0 ? { unavailableDecisions } : {};
142149
const requiresAsk =
143150
requiresExecApproval({
144151
ask: hostAsk,
@@ -167,7 +174,7 @@ export async function executeNodeHostCommand(
167174
nodeId: target.nodeId,
168175
security: hostSecurity,
169176
ask: hostAsk,
170-
allowedDecisions,
177+
...unavailableDecisionRequestParams,
171178
commandHighlighting: params.commandHighlighting,
172179
...buildExecApprovalRequesterContext({
173180
agentId: prepared.agentId,

‎src/gateway/server-methods/exec-approval.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {
2121
import type { ExecApprovalForwarder } from "../../infra/exec-approval-forwarder.js";
2222
import {
2323
DEFAULT_EXEC_APPROVAL_TIMEOUT_MS,
24+
normalizeExecApprovalUnavailableDecisions,
2425
resolveExecApprovalRequestAllowedDecisions,
2526
type ExecApprovalRequest,
2627
type ExecApprovalResolved,
@@ -168,7 +169,7 @@ export function createExecApprovalHandlers(
168169
security?: string;
169170
ask?: string;
170171
warningText?: string | null;
171-
allowedDecisions?: string[];
172+
unavailableDecisions?: string[];
172173
commandSpans?: {
173174
startIndex: number;
174175
endIndex: number;
@@ -299,6 +300,9 @@ export function createExecApprovalHandlers(
299300
);
300301
return;
301302
}
303+
const unavailableDecisions = normalizeExecApprovalUnavailableDecisions(
304+
p.unavailableDecisions,
305+
);
302306
const request = {
303307
command: sanitizedCommandText,
304308
commandPreview:
@@ -317,9 +321,10 @@ export function createExecApprovalHandlers(
317321
warningText: warningText ? sanitizeExecApprovalWarningText(warningText) : null,
318322
commandAnalysis,
319323
commandSpans,
324+
unavailableDecisions: unavailableDecisions.length > 0 ? unavailableDecisions : undefined,
320325
allowedDecisions: resolveExecApprovalRequestAllowedDecisions({
321326
ask: p.ask ?? null,
322-
allowedDecisions: p.allowedDecisions,
327+
unavailableDecisions,
323328
}),
324329
agentId: effectiveAgentId ?? null,
325330
resolvedPath: p.resolvedPath ?? null,

0 commit comments

Comments
 (0)