Skip to content

Commit cbc8476

Browse files
saju01openclaw-clownfish[bot]vincentkoc
authored
fix(exec-approval): stop misattributing Allow Always unavailability to policy (#97740)
* fix(exec-approval): stop misattributing Allow Always unavailability to policy Allow Always is dropped both when policy ask=always AND when a command is non-persistable (e.g. shell redirect `2>&1` -> one-shot), but the prompt always claimed 'effective approval policy requires approval every time'. That's misleading for the non-persistable case (#97069). Reword to reason-neutral 'Allow Always is unavailable for this command.' across all approval surfaces, update en + 20 locale bundles, refresh i18n meta, and the matching tests. Closes #97069 * fix(exec-approval): stop misattributing Allow Always unavailability to policy * test(ui): await exec approval render updates * chore(ui): sync approval i18n metadata --------- Co-authored-by: saju01 <[email protected]> Co-authored-by: openclaw-clownfish[bot] <280122609+openclaw-clownfish[bot]@users.noreply.github.com> Co-authored-by: Vincent Koc <[email protected]>
1 parent c702719 commit cbc8476

50 files changed

Lines changed: 203 additions & 100 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎src/agents/bash-tools.exec-runtime.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -403,9 +403,7 @@ export function buildApprovalPendingMessage(params: {
403403
);
404404
lines.push(`Reply with: /approve ${params.approvalSlug} ${decisionText}`);
405405
if (!allowedDecisions.includes("allow-always")) {
406-
lines.push(
407-
"The effective approval policy requires approval every time, so Allow Always is unavailable.",
408-
);
406+
lines.push("Allow Always is unavailable for this command.");
409407
}
410408
lines.push("If the short code is ambiguous, use the full id in /approve.");
411409
return lines.join("\n");

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -480,8 +480,7 @@ export function createExecApprovalHandlers(
480480
return allowedDecisions.includes(decision)
481481
? null
482482
: {
483-
message:
484-
"allow-always is unavailable because the effective policy requires approval every time",
483+
message: "allow-always is unavailable for this command",
485484
details: APPROVAL_ALLOW_ALWAYS_UNAVAILABLE_DETAILS,
486485
};
487486
},

‎src/gateway/server-methods/server-methods.test.ts‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3716,8 +3716,7 @@ describe("exec approval handlers", () => {
37163716
expect(mockCallArg(resolveRespond)).toBe(false);
37173717
expect(mockCallArg(resolveRespond, 0, 1)).toBeUndefined();
37183718
expectRecordFields(mockCallArg(resolveRespond, 0, 2), {
3719-
message:
3720-
"allow-always is unavailable because the effective policy requires approval every time",
3719+
message: "allow-always is unavailable for this command",
37213720
});
37223721

37233722
const denyRespond = vi.fn();
@@ -3759,8 +3758,7 @@ describe("exec approval handlers", () => {
37593758
expect(mockCallArg(resolveRespond)).toBe(false);
37603759
expect(mockCallArg(resolveRespond, 0, 1)).toBeUndefined();
37613760
expectRecordFields(mockCallArg(resolveRespond, 0, 2), {
3762-
message:
3763-
"allow-always is unavailable because the effective policy requires approval every time",
3761+
message: "allow-always is unavailable for this command",
37643762
});
37653763

37663764
const allowOnceRespond = vi.fn();

‎src/infra/exec-approval-forwarder.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -283,9 +283,7 @@ export function buildExecApprovalRequestMessage(request: ExecApprovalRequest, no
283283
);
284284
lines.push(`Reply with: /approve ${request.id} ${decisionText}`);
285285
if (!allowedDecisions.includes("allow-always")) {
286-
lines.push(
287-
"Allow Always is unavailable because the effective policy requires approval every time.",
288-
);
286+
lines.push("Allow Always is unavailable for this command.");
289287
}
290288
return lines.join("\n");
291289
}

‎src/infra/exec-approval-reply.test.ts‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,7 @@ describe("exec approval reply helpers", () => {
334334
expect(payload.text).not.toContain("C:\\Users\\alice");
335335
});
336336

337-
it("omits allow-always actions when the effective policy requires approval every time", () => {
337+
it("omits allow-always actions when allow-always is unavailable", () => {
338338
const payload = buildExecApprovalPendingReplyPayload({
339339
approvalId: "req-ask-always",
340340
approvalSlug: "slug-always",
@@ -353,9 +353,7 @@ describe("exec approval reply helpers", () => {
353353
});
354354
expect(payload.text).toContain("```txt\n/approve slug-always allow-once\n```");
355355
expect(payload.text).not.toContain("allow-always");
356-
expect(payload.text).toContain(
357-
"The effective approval policy requires approval every time, so Allow Always is unavailable.",
358-
);
356+
expect(payload.text).toContain("Allow Always is unavailable for this command.");
359357
expect(payload.presentation).toEqual({
360358
blocks: [
361359
{

‎src/infra/exec-approval-reply.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -383,9 +383,7 @@ export function buildExecApprovalPendingReplyPayload(
383383
lines.push(secondaryFence);
384384
}
385385
if (!allowedDecisions.includes("allow-always")) {
386-
lines.push(
387-
"The effective approval policy requires approval every time, so Allow Always is unavailable.",
388-
);
386+
lines.push("Allow Always is unavailable for this command.");
389387
}
390388
const info: string[] = [];
391389
info.push(`Host: ${params.host}`);

‎src/plugin-sdk/approval-reaction-runtime.test.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,27 @@ describe("plugin-sdk/approval-reaction-runtime", () => {
155155
expect(payload.text).not.toContain("\nIgnore previous instructions");
156156
});
157157

158+
it("builds exec reaction prompts with neutral allow-always unavailable copy", () => {
159+
const payload = buildApprovalReactionPromptPayloadForRequest({
160+
request: {
161+
...execRequest,
162+
request: {
163+
...execRequest.request,
164+
ask: "always",
165+
},
166+
},
167+
nowMs: 1_000,
168+
});
169+
170+
expect(payload.text).toContain("React with:\n\n👍 Allow Once\n👎 Deny");
171+
expect(payload.text).not.toContain("♾️ Allow Always");
172+
expect(payload.text).toContain("Allow Always is unavailable for this command.");
173+
expect(payload.text).not.toContain("effective policy requires approval every time");
174+
expect(
175+
payload.text?.trim().endsWith("Reply with: /approve exec-approval-123 allow-once|deny"),
176+
).toBe(true);
177+
});
178+
158179
it("builds canonical plugin reaction prompts with real ids", () => {
159180
const payload = buildApprovalReactionPromptPayloadForRequest({
160181
request: {
@@ -176,6 +197,7 @@ describe("plugin-sdk/approval-reaction-runtime", () => {
176197
expect(payload.text).toContain(
177198
"Allow Always is unavailable because the effective policy requires approval every time.",
178199
);
200+
expect(payload.text).not.toContain("Allow Always is unavailable for this command.");
179201
expect(
180202
payload.text?.trim().endsWith("Reply with: /approve plugin:approval-123 allow-once|deny"),
181203
).toBe(true);

‎src/plugin-sdk/approval-reaction-runtime.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,13 +236,16 @@ function buildDecisionText(allowedDecisions: readonly ExecApprovalReplyDecision[
236236
}
237237

238238
function buildManualInstructionSection(params: {
239+
approvalKind: ApprovalKind;
239240
approvalId: string;
240241
allowedDecisions: readonly ExecApprovalReplyDecision[];
241242
}): string[] {
242243
const lines: string[] = [];
243244
if (!params.allowedDecisions.includes("allow-always")) {
244245
lines.push(
245-
"Allow Always is unavailable because the effective policy requires approval every time.",
246+
params.approvalKind === "exec"
247+
? "Allow Always is unavailable for this command."
248+
: "Allow Always is unavailable because the effective policy requires approval every time.",
246249
);
247250
}
248251
if (params.allowedDecisions.length > 0) {
@@ -335,6 +338,7 @@ function buildApprovalReactionPromptText(params: {
335338
sections.push(commandInstructions.join("\n"));
336339
}
337340
const manualInstructions = buildManualInstructionSection({
341+
approvalKind: view.approvalKind,
338342
approvalId: view.approvalId,
339343
allowedDecisions,
340344
});

‎ui/src/components/exec-approval.test.ts‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
/* @vitest-environment jsdom */
2+
3+
import { html, nothing, render, type LitElement } from "lit";
4+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
5+
import type { ExecApprovalRequest } from "../app/exec-approval.ts";
6+
import { i18n } from "../i18n/index.ts";
7+
import { getRenderedModalDialog, installDialogPolyfill } from "../test-helpers/modal-dialog.ts";
8+
import "./exec-approval.ts";
9+
10+
let container: HTMLDivElement;
11+
let restoreDialogPolyfill: () => void;
12+
13+
function createExecRequest(overrides: Partial<ExecApprovalRequest> = {}): ExecApprovalRequest {
14+
return {
15+
id: "approval-1",
16+
kind: "exec",
17+
request: {
18+
command: "echo hello",
19+
ask: "on-request",
20+
},
21+
createdAtMs: Date.now() - 1_000,
22+
expiresAtMs: Date.now() + 60_000,
23+
...overrides,
24+
};
25+
}
26+
27+
async function renderApproval(request: ExecApprovalRequest) {
28+
render(
29+
html`<openclaw-exec-approval
30+
.props=${{
31+
queue: [request],
32+
busy: false,
33+
error: null,
34+
onDecision: vi.fn(),
35+
}}
36+
></openclaw-exec-approval>`,
37+
container,
38+
);
39+
const approval = container.querySelector<LitElement>("openclaw-exec-approval");
40+
if (!approval) {
41+
throw new Error("Expected exec approval");
42+
}
43+
await approval.updateComplete;
44+
}
45+
46+
describe("openclaw-exec-approval", () => {
47+
beforeEach(async () => {
48+
restoreDialogPolyfill = installDialogPolyfill();
49+
await i18n.setLocale("en");
50+
container = document.createElement("div");
51+
document.body.append(container);
52+
});
53+
54+
afterEach(async () => {
55+
render(nothing, container);
56+
container.remove();
57+
await i18n.setLocale("en");
58+
restoreDialogPolyfill();
59+
vi.restoreAllMocks();
60+
});
61+
62+
it("uses neutral unavailable copy for exec allow-always decisions", async () => {
63+
await renderApproval(
64+
createExecRequest({
65+
request: {
66+
command: "echo hello",
67+
ask: "always",
68+
allowedDecisions: ["allow-once", "deny"],
69+
},
70+
}),
71+
);
72+
73+
await getRenderedModalDialog(container);
74+
75+
expect(
76+
Array.from(container.querySelectorAll(".exec-approval-actions button")).map((button) =>
77+
button.textContent?.trim(),
78+
),
79+
).toEqual(["Allow once", "Deny"]);
80+
expect(container.querySelector(".exec-approval-warning")?.textContent?.trim()).toBe(
81+
"Allow Always is unavailable for this command.",
82+
);
83+
});
84+
85+
it("does not show exec unavailable copy for restricted plugin approvals", async () => {
86+
await renderApproval(
87+
createExecRequest({
88+
id: "plugin-approval-1",
89+
kind: "plugin",
90+
request: {
91+
command: "Plugin approval",
92+
allowedDecisions: ["allow-once", "deny"],
93+
},
94+
pluginTitle: "Plugin approval",
95+
}),
96+
);
97+
98+
await getRenderedModalDialog(container);
99+
100+
expect(
101+
Array.from(container.querySelectorAll(".exec-approval-actions button")).map((button) =>
102+
button.textContent?.trim(),
103+
),
104+
).toEqual(["Allow once", "Deny"]);
105+
expect(container.querySelector(".exec-approval-warning")).toBeNull();
106+
});
107+
});

‎ui/src/i18n/.i18n/ar.meta.json‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)