Exec approvals: improve batching text + retries

src/auto-reply/reply/commands-approve.test.ts

@@ -14,6 +14,7 @@ vi.mock("../../gateway/call.js", () => ({
 vi.mock("../../infra/exec-approval-forwarder.js", () => ({
   getBatchApprovalIds: vi.fn(),
   deleteBatch: vi.fn(),
+  updateBatchApprovalIds: vi.fn(),
 }));
 
 function buildParams(commandBody: string, cfg: ClawdbotConfig, ctxOverrides?: Partial<MsgContext>) {
@@ -153,6 +154,7 @@ describe("/approve-batch command", () => {
     expect(result.reply?.text).toContain("3 commands approved");
     expect(mockCallGateway).toHaveBeenCalledTimes(3);
     expect(approvalForwarder.deleteBatch).toHaveBeenCalledWith("batch-123");
+    expect(approvalForwarder.updateBatchApprovalIds).not.toHaveBeenCalled();
   });
 
   it("denies all commands in batch", async () => {
@@ -170,6 +172,8 @@ describe("/approve-batch command", () => {
     expect(result.shouldContinue).toBe(false);
     expect(result.reply?.text).toContain("2 commands approved");
     expect(mockCallGateway).toHaveBeenCalledTimes(2);
+    expect(approvalForwarder.deleteBatch).toHaveBeenCalledWith("batch-456");
+    expect(approvalForwarder.updateBatchApprovalIds).not.toHaveBeenCalled();
   });
 
   it("reports partial failures", async () => {
@@ -190,5 +194,7 @@ describe("/approve-batch command", () => {
     expect(result.shouldContinue).toBe(false);
     expect(result.reply?.text).toContain("2 succeeded");
     expect(result.reply?.text).toContain("1 failed");
+    expect(approvalForwarder.deleteBatch).not.toHaveBeenCalled();
+    expect(approvalForwarder.updateBatchApprovalIds).toHaveBeenCalledWith("batch-789", ["id2"]);
   });
 });

src/auto-reply/reply/commands-approve.ts

@@ -1,7 +1,11 @@
 import { callGateway } from "../../gateway/call.js";
 import { GATEWAY_CLIENT_MODES, GATEWAY_CLIENT_NAMES } from "../../utils/message-channel.js";
 import { logVerbose } from "../../globals.js";
-import { getBatchApprovalIds, deleteBatch } from "../../infra/exec-approval-forwarder.js";
+import {
+  getBatchApprovalIds,
+  deleteBatch,
+  updateBatchApprovalIds,
+} from "../../infra/exec-approval-forwarder.js";
 import type { CommandHandler } from "./commands-types.js";
 
 const COMMAND = "/approve";
@@ -187,8 +191,12 @@ export const handleApproveBatchCommand: CommandHandler = async (params, allowTex
     }
   }
 
-  // Clean up the batch
-  deleteBatch(parsed.batchId);
+  const failedIds = results.filter((r) => !r.success).map((r) => r.id);
+  if (failedIds.length === 0) {
+    deleteBatch(parsed.batchId);
+  } else {
+    updateBatchApprovalIds(parsed.batchId, failedIds);
+  }
 
   const succeeded = results.filter((r) => r.success).length;
   const failed = results.filter((r) => !r.success).length;

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

@@ -34,6 +34,8 @@ describe("exec approval forwarder", () => {
     });
 
     await forwarder.handleRequested(baseRequest);
+    expect(deliver).toHaveBeenCalledTimes(0);
+    await vi.advanceTimersByTimeAsync(1500);
     expect(deliver).toHaveBeenCalledTimes(1);
 
     await forwarder.handleResolved({
@@ -69,6 +71,8 @@ describe("exec approval forwarder", () => {
     });
 
     await forwarder.handleRequested(baseRequest);
+    expect(deliver).toHaveBeenCalledTimes(0);
+    await vi.advanceTimersByTimeAsync(1500);
     expect(deliver).toHaveBeenCalledTimes(1);
 
     await vi.runAllTimersAsync();

src/infra/exec-approval-forwarder.ts

@@ -83,10 +83,10 @@ function cleanupExpiredBatches(nowMs: number): void {
 }
 
 export function getBatchApprovalIds(batchId: string): string[] | null {
-  const entry = batchRegistry.get(batchId);
-  if (!entry) return null;
   // Cleanup expired batches opportunistically
   cleanupExpiredBatches(Date.now());
+  const entry = batchRegistry.get(batchId);
+  if (!entry) return null;
   return entry.approvalIds;
 }
 
@@ -94,6 +94,18 @@ export function deleteBatch(batchId: string): void {
   batchRegistry.delete(batchId);
 }
 
+export function updateBatchApprovalIds(batchId: string, approvalIds: string[]): void {
+  if (approvalIds.length === 0) {
+    batchRegistry.delete(batchId);
+    return;
+  }
+  // Cleanup expired batches opportunistically
+  cleanupExpiredBatches(Date.now());
+  const entry = batchRegistry.get(batchId);
+  if (!entry) return;
+  entry.approvalIds = approvalIds;
+}
+
 export type ExecApprovalForwarder = {
   handleRequested: (request: ExecApprovalRequest) => Promise<void>;
   handleResolved: (resolved: ExecApprovalResolved) => Promise<void>;
@@ -203,7 +215,10 @@ function buildBatchRequestMessage(
   nowMs: number,
 ): ApprovalMessage {
   const count = requests.length;
-  const lines: string[] = [`🔒 Exec approval required (${count} command${count > 1 ? "s" : ""})`];
+  const lines: string[] = [
+    `🔒 Exec approval required (${count} command${count > 1 ? "s" : ""})`,
+    `Batch ID: ${batchId}`,
+  ];
   lines.push("");
 
   // List each command with a number
@@ -211,7 +226,7 @@ function buildBatchRequestMessage(
     const cmd = req.request.command;
     // Truncate long commands for readability
     const displayCmd = cmd.length > 60 ? cmd.slice(0, 57) + "..." : cmd;
-    lines.push(`${idx + 1}. ${displayCmd}`);
+    lines.push(`${idx + 1}. ${displayCmd} (${req.id})`);
   });
 
   lines.push("");
@@ -228,6 +243,8 @@ function buildBatchRequestMessage(
   const earliestExpiry = Math.min(...requests.map((r) => r.expiresAtMs));
   const expiresIn = Math.max(0, Math.round((earliestExpiry - nowMs) / 1000));
   lines.push(`Expires in: ${expiresIn}s`);
+  lines.push(`Approve: /approve-batch ${batchId} allow-once|deny`);
+  lines.push("Or: /approve <id> allow-once|allow-always|deny");
 
   const buttons = [
     [
@@ -357,41 +374,52 @@ export function createExecApprovalForwarder(
 
     const cfg = getConfig();
     const { requests, targets } = batch;
+    const liveRequests = requests.filter((req) => pending.has(req.id));
 
-    if (requests.length === 0 || targets.length === 0) return;
+    if (liveRequests.length === 0 || targets.length === 0) return;
 
-    // Generate batch ID and register it
-    const batchId = generateBatchId();
-    const approvalIds = requests.map((r) => r.id);
-    batchRegistry.set(batchId, {
-      approvalIds,
-      sessionKey,
-      createdAtMs: nowMs(),
-    });
+    let batchId: string | null = null;
+    if (liveRequests.length > 1) {
+      // Generate batch ID and register it
+      batchId = generateBatchId();
+      const approvalIds = liveRequests.map((r) => r.id);
+      // Cleanup any expired batches before inserting
+      cleanupExpiredBatches(nowMs());
+      batchRegistry.set(batchId, {
+        approvalIds,
+        sessionKey,
+        createdAtMs: nowMs(),
+      });
 
-    // Update pending entries with batch ID
-    for (const req of requests) {
-      const entry = pending.get(req.id);
-      if (entry) entry.batchId = batchId;
+      // Update pending entries with batch ID
+      for (const req of liveRequests) {
+        const entry = pending.get(req.id);
+        if (entry) entry.batchId = batchId;
+      }
     }
 
-    log.info(`exec approvals: flushing batch ${batchId} with ${requests.length} requests`);
+    log.info(
+      `exec approvals: flushing ${
+        liveRequests.length > 1 ? `batch ${batchId}` : "single request"
+      } with ${liveRequests.length} request${liveRequests.length > 1 ? "s" : ""}`,
+    );
 
     // Send single or batch message depending on count
-    if (requests.length === 1) {
+    if (liveRequests.length === 1) {
       // Single request - use original format with all buttons
-      const message = buildRequestMessage(requests[0], nowMs());
+      const message = buildRequestMessage(liveRequests[0], nowMs());
       await deliverToTargets({
         cfg,
         targets,
         text: message.text,
         buttons: message.buttons,
         deliver,
-        shouldSend: () => pending.has(requests[0].id),
+        shouldSend: () => pending.has(liveRequests[0].id),
       });
     } else {
       // Multiple requests - use batch format
-      const message = buildBatchRequestMessage(requests, batchId, nowMs());
+      if (!batchId) return;
+      const message = buildBatchRequestMessage(liveRequests, batchId, nowMs());
       await deliverToTargets({
         cfg,
         targets,

src/infra/outbound/deliver.ts

@@ -17,6 +17,7 @@ import { sendMessageSignal } from "../../signal/send.js";
 import type { sendMessageSlack } from "../../slack/send.js";
 import type { sendMessageTelegram } from "../../telegram/send.js";
 import type { sendMessageWhatsApp } from "../../web/outbound.js";
+import { createSubsystemLogger } from "../../logging/subsystem.js";
 import {
   appendAssistantMessageToSessionTranscript,
   resolveMirroredTranscriptText,
@@ -28,6 +29,8 @@ import type { OutboundChannel } from "./targets.js";
 export type { NormalizedOutboundPayload } from "./payloads.js";
 export { normalizeOutboundPayloads } from "./payloads.js";
 
+const log = createSubsystemLogger("gateway/outbound");
+
 type SendMatrixMessage = (
   to: string,
   text: string,
@@ -321,12 +324,11 @@ export async function deliverOutboundPayloads(params: {
     try {
       throwIfAborted(abortSignal);
       params.onPayload?.(payloadSummary);
-      // Debug: check sendPayload path
-      console.log(
-        `[outbound] channel=${channel} hasSendPayload=${!!handler.sendPayload} hasChannelData=${!!payload.channelData} channelData=${JSON.stringify(payload.channelData)}`,
+      log.debug(
+        `outbound: channel=${channel} hasSendPayload=${!!handler.sendPayload} hasChannelData=${!!payload.channelData}`,
       );
       if (handler.sendPayload && payload.channelData) {
-        console.log(`[outbound] using sendPayload for ${channel}`);
+        log.debug(`outbound: using sendPayload for ${channel}`);
         results.push(await handler.sendPayload(payload));
         continue;
       }