From 9f84b8c266df8ed1dc14ea8096b1749154158b3f Mon Sep 17 00:00:00 2001 From: Aaron Ring Date: Mon, 26 Jan 2026 22:21:53 -0800 Subject: [PATCH] Exec approvals: improve batching text + retries --- src/auto-reply/reply/commands-approve.test.ts | 6 ++ src/auto-reply/reply/commands-approve.ts | 14 +++- src/infra/exec-approval-forwarder.test.ts | 4 ++ src/infra/exec-approval-forwarder.ts | 72 +++++++++++++------ src/infra/outbound/deliver.ts | 10 +-- 5 files changed, 77 insertions(+), 29 deletions(-) diff --git a/src/auto-reply/reply/commands-approve.test.ts b/src/auto-reply/reply/commands-approve.test.ts index 960b3df0e..21b5aa290 100644 --- a/src/auto-reply/reply/commands-approve.test.ts +++ b/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) { @@ -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"]); }); }); diff --git a/src/auto-reply/reply/commands-approve.ts b/src/auto-reply/reply/commands-approve.ts index 3da67754f..8c1ca327c 100644 --- a/src/auto-reply/reply/commands-approve.ts +++ b/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; diff --git a/src/infra/exec-approval-forwarder.test.ts b/src/infra/exec-approval-forwarder.test.ts index 422e22c48..cd3dfc5b4 100644 --- a/src/infra/exec-approval-forwarder.test.ts +++ b/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(); diff --git a/src/infra/exec-approval-forwarder.ts b/src/infra/exec-approval-forwarder.ts index b676286d2..475d8b9e4 100644 --- a/src/infra/exec-approval-forwarder.ts +++ b/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; handleResolved: (resolved: ExecApprovalResolved) => Promise; @@ -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 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, diff --git a/src/infra/outbound/deliver.ts b/src/infra/outbound/deliver.ts index 87ed12851..e957dfa87 100644 --- a/src/infra/outbound/deliver.ts +++ b/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; }