Skip to content

Commit 0e9b673

Browse files
jincheng-xydtjincheng-xydt
committed
fix #97069: distinguish one-shot command from policy requirement in approval messages
resolveExecApprovalAllowedDecisions() removes allow-always from allowed decisions when the command is not persistable (e.g., shell redirection), but the user-facing message always said 'the effective policy requires approval every time.' This misled users whose actual policy is ask=on-miss into thinking their policy was misconfigured. Fix: check params.ask (or request.request.ask) — if ask is 'always', show the policy message. Otherwise, show a one-shot message explaining that the command cannot be persisted. Fixes #97069
1 parent 4ecb45b commit 0e9b673

4 files changed

Lines changed: 49 additions & 8 deletions

File tree

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -645,6 +645,25 @@ describe("exec approval forwarder", () => {
645645
expect(text).toContain("Allow Always is unavailable");
646646
});
647647

648+
it("shows one-shot message when command cannot be persisted (#97069)", async () => {
649+
vi.useFakeTimers();
650+
const { deliver, forwarder } = createForwarder({ cfg: TARGETS_CFG });
651+
await expect(
652+
forwarder.handleRequested({
653+
...baseRequest,
654+
request: {
655+
...baseRequest.request,
656+
ask: "on-miss",
657+
unavailableDecisions: ["allow-always"],
658+
},
659+
}),
660+
).resolves.toBe(true);
661+
await Promise.resolve();
662+
const text = getFirstDeliveryText(deliver);
663+
expect(text).toContain("Allow Always is unavailable because this command is not eligible for persistent reuse");
664+
expect(text).not.toContain("effective policy requires approval every time");
665+
});
666+
648667
it.each([
649668
{
650669
command: "bash safe\u200B.sh",

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -289,12 +289,16 @@ export function buildExecApprovalRequestMessage(request: ExecApprovalRequest, no
289289
lines.push(
290290
allowedDecisions.includes("allow-always")
291291
? "Background mode note: non-interactive runs cannot wait for chat approvals; use pre-approved policy (allow-always or ask=off)."
292-
: "Background mode note: non-interactive runs cannot wait for chat approvals; the effective policy still requires per-run approval unless ask=off.",
292+
: request.request.ask === "always"
293+
? "Background mode note: non-interactive runs cannot wait for chat approvals; the effective policy still requires per-run approval unless ask=off."
294+
: "Background mode note: non-interactive runs cannot wait for chat approvals; this command cannot be pre-approved for persistent reuse.",
293295
);
294296
lines.push(`Reply with: /approve ${request.id} ${decisionText}`);
295297
if (!allowedDecisions.includes("allow-always")) {
296298
lines.push(
297-
"Allow Always is unavailable because the effective policy requires approval every time.",
299+
request.request.ask === "always"
300+
? "Allow Always is unavailable because the effective policy requires approval every time."
301+
: "Allow Always is unavailable because this command is not eligible for persistent reuse (e.g., shell redirection).",
298302
);
299303
}
300304
return lines.join("\n");

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

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -316,7 +316,7 @@ describe("exec approval reply helpers", () => {
316316
expect(payload.text).not.toContain("C:\\Users\\alice");
317317
});
318318

319-
it("omits allow-always actions when the effective policy requires approval every time", () => {
319+
it("omits allow-always and shows policy message when ask=always (#97069)", () => {
320320
const payload = buildExecApprovalPendingReplyPayload({
321321
approvalId: "req-ask-always",
322322
approvalSlug: "slug-always",
@@ -338,6 +338,22 @@ describe("exec approval reply helpers", () => {
338338
expect(payload.text).toContain(
339339
"The effective approval policy requires approval every time, so Allow Always is unavailable.",
340340
);
341+
});
342+
343+
it("omits allow-always and shows one-shot message when command is not persistable (#97069)", () => {
344+
const payload = buildExecApprovalPendingReplyPayload({
345+
approvalId: "req-one-shot",
346+
approvalSlug: "slug-one-shot",
347+
ask: "on-miss",
348+
allowedDecisions: ["allow-once", "deny"],
349+
command: "openclaw --version 2>&1",
350+
host: "gateway",
351+
});
352+
353+
expect(payload.text).not.toContain("allow-always");
354+
expect(payload.text).toContain(
355+
"This command is not eligible for Allow Always (e.g., shell redirection cannot be persisted for reuse).",
356+
);
341357
expect(payload.presentation).toEqual({
342358
blocks: [
343359
{
@@ -347,18 +363,18 @@ describe("exec approval reply helpers", () => {
347363
label: "Allow Once",
348364
action: {
349365
type: "command",
350-
command: "/approve req-ask-always allow-once",
366+
command: "/approve req-one-shot allow-once",
351367
},
352-
value: "/approve req-ask-always allow-once",
368+
value: "/approve req-one-shot allow-once",
353369
style: "success",
354370
},
355371
{
356372
label: "Deny",
357373
action: {
358374
type: "command",
359-
command: "/approve req-ask-always deny",
375+
command: "/approve req-one-shot deny",
360376
},
361-
value: "/approve req-ask-always deny",
377+
value: "/approve req-one-shot deny",
362378
style: "danger",
363379
},
364380
],

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -377,7 +377,9 @@ export function buildExecApprovalPendingReplyPayload(
377377
}
378378
if (!allowedDecisions.includes("allow-always")) {
379379
lines.push(
380-
"The effective approval policy requires approval every time, so Allow Always is unavailable.",
380+
params.ask === "always"
381+
? "The effective approval policy requires approval every time, so Allow Always is unavailable."
382+
: "This command is not eligible for Allow Always (e.g., shell redirection cannot be persisted for reuse).",
381383
);
382384
}
383385
const info: string[] = [];

0 commit comments

Comments
 (0)