fix(approval): treat null ask as missing context in unavailable-copy checks

liuhao1024 · liuhao1024 · commit 97301db93ca8 · 2026-06-27T22:55:50.000+08:00
Normalize null and undefined in the ask-aware unavailable-copy branches across reply, forwarder, and Control UI renderers so that explicit ask: null uses the policy-required fallback instead of the non-persistable message. Add focused null regression tests for reply and forwarder. Addresses Codex review finding on PR #97077.
diff --git a/src/infra/exec-approval-forwarder.test.ts b/src/infra/exec-approval-forwarder.test.ts
@@ -667,6 +667,29 @@ describe("exec approval forwarder", () => {
     expect(text).not.toContain("cannot be persisted");
   });
 
+  it("shows policy-required reason when allow-always is unavailable and ask is null", async () => {
+    vi.useFakeTimers();
+    const { deliver, forwarder } = createForwarder({ cfg: TARGETS_CFG });
+    await expect(
+      forwarder.handleRequested({
+        ...baseRequest,
+        request: {
+          ...baseRequest.request,
+          ask: null,
+          unavailableDecisions: ["allow-always"],
+        },
+      }),
+    ).resolves.toBe(true);
+    await Promise.resolve();
+    const text = getFirstDeliveryText(deliver);
+    expect(text).toContain("Reply with: /approve req-1 allow-once|deny");
+    expect(text).not.toContain("allow-once|allow-always|deny");
+    expect(text).toContain(
+      "Allow Always is unavailable because the effective policy requires approval every time.",
+    );
+    expect(text).not.toContain("cannot be persisted");
+  });
+
   it("shows non-persistable reason when allow-always is unavailable without ask=always", async () => {
     vi.useFakeTimers();
     const { deliver, forwarder } = createForwarder({ cfg: TARGETS_CFG });
diff --git a/src/infra/exec-approval-forwarder.ts b/src/infra/exec-approval-forwarder.ts
@@ -294,7 +294,7 @@ export function buildExecApprovalRequestMessage(request: ExecApprovalRequest, no
   lines.push(`Reply with: /approve ${request.id} ${decisionText}`);
   if (!allowedDecisions.includes("allow-always")) {
     lines.push(
-      request.request.ask !== undefined && request.request.ask !== "always"
+      request.request.ask != null && request.request.ask !== "always"
         ? "Allow Always is unavailable because this command cannot be persisted (e.g., shell redirection or dynamic content)."
         : "Allow Always is unavailable because the effective policy requires approval every time.",
     );
diff --git a/src/infra/exec-approval-reply.test.ts b/src/infra/exec-approval-reply.test.ts
@@ -384,6 +384,23 @@ describe("exec approval reply helpers", () => {
     expect(payload.text).not.toContain("cannot be persisted");
   });
 
+  it("shows policy-required reason when allow-always is unavailable and ask is null", () => {
+    const payload = buildExecApprovalPendingReplyPayload({
+      approvalId: "req-null-ask",
+      approvalSlug: "slug-null-ask",
+      ask: null,
+      allowedDecisions: ["allow-once", "deny"],
+      command: "echo hello",
+      host: "gateway",
+    });
+
+    expect(payload.text).not.toContain("allow-always");
+    expect(payload.text).toContain(
+      "The effective approval policy requires approval every time, so Allow Always is unavailable.",
+    );
+    expect(payload.text).not.toContain("cannot be persisted");
+  });
+
   it("shows non-persistable reason when allow-always is unavailable without ask=always", () => {
     const payload = buildExecApprovalPendingReplyPayload({
       approvalId: "req-oneshot",
diff --git a/src/infra/exec-approval-reply.ts b/src/infra/exec-approval-reply.ts
@@ -380,7 +380,7 @@ export function buildExecApprovalPendingReplyPayload(
     // without the full exec context), default to the policy-required explanation
     // rather than the non-persistable message.
     lines.push(
-      params.ask !== undefined && params.ask !== "always"
+      params.ask != null && params.ask !== "always"
         ? "Allow Always is unavailable because this command cannot be persisted (e.g., shell redirection or dynamic content)."
         : "The effective approval policy requires approval every time, so Allow Always is unavailable.",
     );
diff --git a/ui/src/ui/views/exec-approval.ts b/ui/src/ui/views/exec-approval.ts
@@ -156,7 +156,7 @@ function renderUnavailableDecisionWarning(
   if (active.kind !== "exec" || decisions.includes("allow-always")) {
     return nothing;
   }
-  const isNonPersistable = active.request.ask !== undefined && active.request.ask !== "always";
+  const isNonPersistable = active.request.ask != null && active.request.ask !== "always";
   const key = isNonPersistable
     ? "execApproval.allowAlwaysUnavailableNonPersistable"
     : "execApproval.allowAlwaysUnavailable";

Original file line number	Diff line number	Diff line change
`@@ -156,7 +156,7 @@ function renderUnavailableDecisionWarning(`
`156`	`156`	`if (active.kind !== "exec" \|\| decisions.includes("allow-always")) {`
`157`	`157`	`return nothing;`
`158`	`158`	`}`
`159`		`- const isNonPersistable = active.request.ask !== undefined && active.request.ask !== "always";`
	`159`	`+ const isNonPersistable = active.request.ask != null && active.request.ask !== "always";`
`160`	`160`	`const key = isNonPersistable`
`161`	`161`	`? "execApproval.allowAlwaysUnavailableNonPersistable"`
`162`	`162`	`: "execApproval.allowAlwaysUnavailable";`