diff --git a/docs/refactor/send-refactor.md b/docs/refactor/send-refactor.md index c914a4f69..42196ed5f 100644 --- a/docs/refactor/send-refactor.md +++ b/docs/refactor/send-refactor.md @@ -1,7 +1,7 @@ # send-refactor scratchpad - [x] Commit + push current outbound refactor changes -- [ ] Step 1: centralize outbound target validation +- [x] Step 1: centralize outbound target validation - [ ] Step 2: normalize payloads + single delivery call - [ ] Step 3: unify outbound JSON/result formatting - [ ] Cleanup: delete scratchpad, final lint + tests, commit + push diff --git a/src/commands/agent.ts b/src/commands/agent.ts index 3d0e6ab68..61d96efb3 100644 --- a/src/commands/agent.ts +++ b/src/commands/agent.ts @@ -44,7 +44,10 @@ import { emitAgentEvent, registerAgentRunContext, } from "../infra/agent-events.js"; -import { deliverOutboundPayloads } from "../infra/outbound/deliver.js"; +import { + deliverOutboundPayloads, + normalizeOutboundPayloads, +} from "../infra/outbound/deliver.js"; import { resolveOutboundTarget } from "../infra/outbound/targets.js"; import { defaultRuntime, type RuntimeEnv } from "../runtime.js"; import { resolveSendPolicy } from "../sessions/send-policy.js"; @@ -561,51 +564,46 @@ export async function agentCommand( return { payloads: [], meta: result.meta }; } - for (const payload of payloads) { - const mediaList = - payload.mediaUrls ?? (payload.mediaUrl ? [payload.mediaUrl] : []); - - if (!opts.json) { - const lines: string[] = []; - if (payload.text) lines.push(payload.text.trimEnd()); - for (const url of mediaList) lines.push(`MEDIA:${url}`); - runtime.log(lines.join("\n")); + const deliveryPayloads = normalizeOutboundPayloads(payloads); + const logPayload = (payload: { text: string; mediaUrls: string[] }) => { + if (opts.json) return; + const lines: string[] = []; + if (payload.text) lines.push(payload.text.trimEnd()); + for (const url of payload.mediaUrls) lines.push(`MEDIA:${url}`); + runtime.log(lines.join("\n")); + }; + if (!deliver) { + for (const payload of deliveryPayloads) { + logPayload(payload); } - - if (!deliver) continue; - - const text = payload.text ?? ""; - const media = mediaList; - if (!text && media.length === 0) continue; - - if ( - deliveryProvider === "whatsapp" || + } + if ( + deliver && + (deliveryProvider === "whatsapp" || deliveryProvider === "telegram" || deliveryProvider === "discord" || deliveryProvider === "slack" || deliveryProvider === "signal" || - deliveryProvider === "imessage" - ) { - if (!deliveryTarget) continue; - try { - await deliverOutboundPayloads({ - cfg, - provider: deliveryProvider, - to: deliveryTarget, - payloads: [payload], - deps: { - sendWhatsApp: deps.sendMessageWhatsApp, - sendTelegram: deps.sendMessageTelegram, - sendDiscord: deps.sendMessageDiscord, - sendSlack: deps.sendMessageSlack, - sendSignal: deps.sendMessageSignal, - sendIMessage: deps.sendMessageIMessage, - }, - }); - } catch (err) { - if (!bestEffortDeliver) throw err; - logDeliveryError(err); - } + deliveryProvider === "imessage") + ) { + if (deliveryTarget) { + await deliverOutboundPayloads({ + cfg, + provider: deliveryProvider, + to: deliveryTarget, + payloads: deliveryPayloads, + bestEffort: bestEffortDeliver, + onError: (err) => logDeliveryError(err), + onPayload: logPayload, + deps: { + sendWhatsApp: deps.sendMessageWhatsApp, + sendTelegram: deps.sendMessageTelegram, + sendDiscord: deps.sendMessageDiscord, + sendSlack: deps.sendMessageSlack, + sendSignal: deps.sendMessageSignal, + sendIMessage: deps.sendMessageIMessage, + }, + }); } } diff --git a/src/infra/outbound/deliver.test.ts b/src/infra/outbound/deliver.test.ts index 85b429dc2..31b24a7a7 100644 --- a/src/infra/outbound/deliver.test.ts +++ b/src/infra/outbound/deliver.test.ts @@ -1,7 +1,10 @@ import { describe, expect, it, vi } from "vitest"; import type { ClawdbotConfig } from "../../config/config.js"; -import { deliverOutboundPayloads } from "./deliver.js"; +import { + deliverOutboundPayloads, + normalizeOutboundPayloads, +} from "./deliver.js"; describe("deliverOutboundPayloads", () => { it("chunks telegram markdown and passes config token", async () => { @@ -86,9 +89,7 @@ describe("deliverOutboundPayloads", () => { }); it("uses iMessage media maxBytes from agent fallback", async () => { - const sendIMessage = vi - .fn() - .mockResolvedValue({ messageId: "i1" }); + const sendIMessage = vi.fn().mockResolvedValue({ messageId: "i1" }); const cfg: ClawdbotConfig = { agent: { mediaMaxMb: 3 } }; await deliverOutboundPayloads({ @@ -105,4 +106,41 @@ describe("deliverOutboundPayloads", () => { expect.objectContaining({ maxBytes: 3 * 1024 * 1024 }), ); }); + + it("normalizes payloads and drops empty entries", () => { + const normalized = normalizeOutboundPayloads([ + { text: "hi" }, + { mediaUrl: "https://x.test/a.jpg" }, + { text: " ", mediaUrls: [] }, + ]); + expect(normalized).toEqual([ + { text: "hi", mediaUrls: [] }, + { text: "", mediaUrls: ["https://x.test/a.jpg"] }, + ]); + }); + + it("continues on errors when bestEffort is enabled", async () => { + const sendWhatsApp = vi + .fn() + .mockRejectedValueOnce(new Error("fail")) + .mockResolvedValueOnce({ messageId: "w2", toJid: "jid" }); + const onError = vi.fn(); + const cfg: ClawdbotConfig = {}; + + const results = await deliverOutboundPayloads({ + cfg, + provider: "whatsapp", + to: "+1555", + payloads: [{ text: "a" }, { text: "b" }], + deps: { sendWhatsApp }, + bestEffort: true, + onError, + }); + + expect(sendWhatsApp).toHaveBeenCalledTimes(2); + expect(onError).toHaveBeenCalledTimes(1); + expect(results).toEqual([ + { provider: "whatsapp", messageId: "w2", toJid: "jid" }, + ]); + }); }); diff --git a/src/infra/outbound/deliver.ts b/src/infra/outbound/deliver.ts index 60195fd3a..1bac9a71e 100644 --- a/src/infra/outbound/deliver.ts +++ b/src/infra/outbound/deliver.ts @@ -33,6 +33,11 @@ export type OutboundDeliveryResult = | { provider: "signal"; messageId: string; timestamp?: number } | { provider: "imessage"; messageId: string }; +export type NormalizedOutboundPayload = { + text: string; + mediaUrls: string[]; +}; + type Chunker = (text: string, limit: number) => string[]; function resolveChunker(provider: OutboundProvider): Chunker | null { @@ -55,8 +60,15 @@ function resolveIMessageMaxBytes(cfg: ClawdbotConfig): number | undefined { return undefined; } -function normalizeMediaUrls(payload: ReplyPayload): string[] { - return payload.mediaUrls ?? (payload.mediaUrl ? [payload.mediaUrl] : []); +export function normalizeOutboundPayloads( + payloads: ReplyPayload[], +): NormalizedOutboundPayload[] { + return payloads + .map((payload) => ({ + text: payload.text ?? "", + mediaUrls: payload.mediaUrls ?? (payload.mediaUrl ? [payload.mediaUrl] : []), + })) + .filter((payload) => payload.text || payload.mediaUrls.length > 0); } export async function deliverOutboundPayloads(params: { @@ -65,6 +77,9 @@ export async function deliverOutboundPayloads(params: { to: string; payloads: ReplyPayload[]; deps?: OutboundSendDeps; + bestEffort?: boolean; + onError?: (err: unknown, payload: NormalizedOutboundPayload) => void; + onPayload?: (payload: NormalizedOutboundPayload) => void; }): Promise { const { cfg, provider, to, payloads } = params; const deps = { @@ -179,21 +194,24 @@ export async function deliverOutboundPayloads(params: { results.push({ provider: "discord", ...res }); }; - for (const payload of payloads) { - const text = payload.text ?? ""; - const mediaUrls = normalizeMediaUrls(payload); - if (!text && mediaUrls.length === 0) continue; + const normalizedPayloads = normalizeOutboundPayloads(payloads); + for (const payload of normalizedPayloads) { + try { + params.onPayload?.(payload); + if (payload.mediaUrls.length === 0) { + await sendTextChunks(payload.text); + continue; + } - if (mediaUrls.length === 0) { - await sendTextChunks(text); - continue; - } - - let first = true; - for (const url of mediaUrls) { - const caption = first ? text : ""; - first = false; - await sendMedia(caption, url); + let first = true; + for (const url of payload.mediaUrls) { + const caption = first ? payload.text : ""; + first = false; + await sendMedia(caption, url); + } + } catch (err) { + if (!params.bestEffort) throw err; + params.onError?.(err, payload); } } return results;