diff --git a/CHANGELOG.md b/CHANGELOG.md index e94d2e06c3..0be2be34b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ Docs: https://docs.openclaw.ai - iOS/Talk: harden mobile talk config handling by ignoring redacted/env-placeholder API keys, support secure local keychain override, improve accessibility motion/contrast behavior in status UI, and tighten ATS to local-network allowance. (#18163) Thanks @mbelinky. - iOS/Location: restore the significant location monitor implementation (service hooks + protocol surface + ATS key alignment) after merge drift so iOS builds compile again. (#18260) Thanks @ngutman. - Telegram: keep DM-topic replies and draft previews in the originating private-chat topic by preserving positive `message_thread_id` values for DM threads. (#18586) Thanks @sebslight. +- Discord: prevent duplicate media delivery when the model uses the `message send` tool with media, by skipping media extraction from messaging tool results since the tool already sent the message directly. (#18270) - Telegram: keep draft-stream preview replies attached to the user message for `replyToMode: "all"` in groups and DMs, preserving threaded reply context from preview through finalization. (#17880) Thanks @yinghaosang. - Telegram: prevent streaming final replies from being overwritten by later final/error payloads, and suppress fallback tool-error warnings when a recovered assistant answer already exists after tool calls. (#17883) Thanks @Marvae and @obviyus. - Telegram: disable block streaming when `channels.telegram.streamMode` is `off`, preventing newline/content-block replies from splitting into multiple messages. (#17679) Thanks @saivarunk. diff --git a/src/agents/pi-embedded-subscribe.handlers.tools.test.ts b/src/agents/pi-embedded-subscribe.handlers.tools.test.ts index 30e5b7881a..8fbd298347 100644 --- a/src/agents/pi-embedded-subscribe.handlers.tools.test.ts +++ b/src/agents/pi-embedded-subscribe.handlers.tools.test.ts @@ -1,13 +1,26 @@ +import type { AgentEvent } from "@mariozechner/pi-agent-core"; import { describe, expect, it, vi } from "vitest"; +import type { MessagingToolSend } from "./pi-embedded-messaging.js"; +import type { + ToolCallSummary, + ToolHandlerContext, +} from "./pi-embedded-subscribe.handlers.types.js"; import { handleToolExecutionEnd, handleToolExecutionStart, } from "./pi-embedded-subscribe.handlers.tools.js"; -function createTestContext() { +type ToolExecutionStartEvent = Extract; +type ToolExecutionEndEvent = Extract; + +function createTestContext(): { + ctx: ToolHandlerContext; + warn: ReturnType; + onBlockReplyFlush: ReturnType; +} { const onBlockReplyFlush = vi.fn(); const warn = vi.fn(); - const ctx = { + const ctx: ToolHandlerContext = { params: { runId: "run-test", onBlockReplyFlush, @@ -21,20 +34,24 @@ function createTestContext() { warn, }, state: { - toolMetaById: new Map(), + toolMetaById: new Map(), + toolMetas: [], toolSummaryById: new Set(), - pendingMessagingTargets: new Map(), + pendingMessagingTargets: new Map(), pendingMessagingTexts: new Map(), + pendingMessagingMediaUrls: new Map(), messagingToolSentTexts: [], messagingToolSentTextsNormalized: [], + messagingToolSentMediaUrls: [], messagingToolSentTargets: [], successfulCronAdds: 0, - toolMetas: [], }, shouldEmitToolResult: () => false, + shouldEmitToolOutput: () => false, emitToolSummary: vi.fn(), + emitToolOutput: vi.fn(), trimMessagingToolSent: vi.fn(), - } as const; + }; return { ctx, warn, onBlockReplyFlush }; } @@ -43,15 +60,14 @@ describe("handleToolExecutionStart read path checks", () => { it("does not warn when read tool uses file_path alias", async () => { const { ctx, warn, onBlockReplyFlush } = createTestContext(); - await handleToolExecutionStart( - ctx as never, - { - type: "tool_execution_start", - toolName: "read", - toolCallId: "tool-1", - args: { file_path: "/tmp/example.txt" }, - } as never, - ); + const evt: ToolExecutionStartEvent = { + type: "tool_execution_start", + toolName: "read", + toolCallId: "tool-1", + args: { file_path: "/tmp/example.txt" }, + }; + + await handleToolExecutionStart(ctx, evt); expect(onBlockReplyFlush).toHaveBeenCalledTimes(1); expect(warn).not.toHaveBeenCalled(); @@ -60,15 +76,14 @@ describe("handleToolExecutionStart read path checks", () => { it("warns when read tool has neither path nor file_path", async () => { const { ctx, warn } = createTestContext(); - await handleToolExecutionStart( - ctx as never, - { - type: "tool_execution_start", - toolName: "read", - toolCallId: "tool-2", - args: {}, - } as never, - ); + const evt: ToolExecutionStartEvent = { + type: "tool_execution_start", + toolName: "read", + toolCallId: "tool-2", + args: {}, + }; + + await handleToolExecutionStart(ctx, evt); expect(warn).toHaveBeenCalledTimes(1); expect(String(warn.mock.calls[0]?.[0] ?? "")).toContain("read tool called without path"); @@ -128,3 +143,127 @@ describe("handleToolExecutionEnd cron.add commitment tracking", () => { expect(ctx.state.successfulCronAdds).toBe(0); }); }); + +describe("messaging tool media URL tracking", () => { + it("tracks mediaUrl arg from messaging tool as pending", async () => { + const { ctx } = createTestContext(); + + const evt: ToolExecutionStartEvent = { + type: "tool_execution_start", + toolName: "message", + toolCallId: "tool-m1", + args: { action: "send", to: "channel:123", content: "hi", mediaUrl: "file:///img.jpg" }, + }; + + await handleToolExecutionStart(ctx, evt); + + expect(ctx.state.pendingMessagingMediaUrls.get("tool-m1")).toBe("file:///img.jpg"); + }); + + it("commits pending media URL on tool success", async () => { + const { ctx } = createTestContext(); + + // Simulate start + const startEvt: ToolExecutionStartEvent = { + type: "tool_execution_start", + toolName: "message", + toolCallId: "tool-m2", + args: { action: "send", to: "channel:123", content: "hi", mediaUrl: "file:///img.jpg" }, + }; + + await handleToolExecutionStart(ctx, startEvt); + + // Simulate successful end + const endEvt: ToolExecutionEndEvent = { + type: "tool_execution_end", + toolName: "message", + toolCallId: "tool-m2", + isError: false, + result: { ok: true }, + }; + + await handleToolExecutionEnd(ctx, endEvt); + + expect(ctx.state.messagingToolSentMediaUrls).toContain("file:///img.jpg"); + expect(ctx.state.pendingMessagingMediaUrls.has("tool-m2")).toBe(false); + }); + + it("trims messagingToolSentMediaUrls to 200 on commit (FIFO)", async () => { + const { ctx } = createTestContext(); + + // Replace mock with a real trim that replicates production cap logic. + const MAX = 200; + ctx.trimMessagingToolSent = () => { + if (ctx.state.messagingToolSentTexts.length > MAX) { + const overflow = ctx.state.messagingToolSentTexts.length - MAX; + ctx.state.messagingToolSentTexts.splice(0, overflow); + ctx.state.messagingToolSentTextsNormalized.splice(0, overflow); + } + if (ctx.state.messagingToolSentTargets.length > MAX) { + const overflow = ctx.state.messagingToolSentTargets.length - MAX; + ctx.state.messagingToolSentTargets.splice(0, overflow); + } + if (ctx.state.messagingToolSentMediaUrls.length > MAX) { + const overflow = ctx.state.messagingToolSentMediaUrls.length - MAX; + ctx.state.messagingToolSentMediaUrls.splice(0, overflow); + } + }; + + // Pre-fill with 200 URLs (url-0 .. url-199) + for (let i = 0; i < 200; i++) { + ctx.state.messagingToolSentMediaUrls.push(`file:///img-${i}.jpg`); + } + expect(ctx.state.messagingToolSentMediaUrls).toHaveLength(200); + + // Commit one more via start → end + const startEvt: ToolExecutionStartEvent = { + type: "tool_execution_start", + toolName: "message", + toolCallId: "tool-cap", + args: { action: "send", to: "channel:123", content: "hi", mediaUrl: "file:///img-new.jpg" }, + }; + await handleToolExecutionStart(ctx, startEvt); + + const endEvt: ToolExecutionEndEvent = { + type: "tool_execution_end", + toolName: "message", + toolCallId: "tool-cap", + isError: false, + result: { ok: true }, + }; + await handleToolExecutionEnd(ctx, endEvt); + + // Should be capped at 200, oldest removed, newest appended. + expect(ctx.state.messagingToolSentMediaUrls).toHaveLength(200); + expect(ctx.state.messagingToolSentMediaUrls[0]).toBe("file:///img-1.jpg"); + expect(ctx.state.messagingToolSentMediaUrls[199]).toBe("file:///img-new.jpg"); + expect(ctx.state.messagingToolSentMediaUrls).not.toContain("file:///img-0.jpg"); + }); + + it("discards pending media URL on tool error", async () => { + const { ctx } = createTestContext(); + + const startEvt: ToolExecutionStartEvent = { + type: "tool_execution_start", + toolName: "message", + toolCallId: "tool-m3", + args: { action: "send", to: "channel:123", content: "hi", mediaUrl: "file:///img.jpg" }, + }; + + await handleToolExecutionStart(ctx, startEvt); + + const endEvt: ToolExecutionEndEvent = { + type: "tool_execution_end", + toolName: "message", + toolCallId: "tool-m3", + isError: true, + result: "Error: failed", + }; + + await handleToolExecutionEnd(ctx, endEvt); + + expect(ctx.state.messagingToolSentMediaUrls).toHaveLength(0); + expect(ctx.state.pendingMessagingMediaUrls.has("tool-m3")).toBe(false); +>>>>>>> 018297172 (test(media-dedup): add missing coverage for Discord media dedup wiring) + }); +}); diff --git a/src/auto-reply/reply/agent-runner-payloads.test.ts b/src/auto-reply/reply/agent-runner-payloads.test.ts new file mode 100644 index 0000000000..a823896958 --- /dev/null +++ b/src/auto-reply/reply/agent-runner-payloads.test.ts @@ -0,0 +1,46 @@ +import { describe, expect, it } from "vitest"; +import { buildReplyPayloads } from "./agent-runner-payloads.js"; + +const baseParams = { + isHeartbeat: false, + didLogHeartbeatStrip: false, + blockStreamingEnabled: false, + blockReplyPipeline: null, + replyToMode: "off" as const, +}; + +describe("buildReplyPayloads media filter integration", () => { + it("strips media URL from payload when in messagingToolSentMediaUrls", () => { + const { replyPayloads } = buildReplyPayloads({ + ...baseParams, + payloads: [{ text: "hello", mediaUrl: "file:///tmp/photo.jpg" }], + messagingToolSentMediaUrls: ["file:///tmp/photo.jpg"], + }); + + expect(replyPayloads).toHaveLength(1); + expect(replyPayloads[0].mediaUrl).toBeUndefined(); + }); + + it("preserves media URL when not in messagingToolSentMediaUrls", () => { + const { replyPayloads } = buildReplyPayloads({ + ...baseParams, + payloads: [{ text: "hello", mediaUrl: "file:///tmp/photo.jpg" }], + messagingToolSentMediaUrls: ["file:///tmp/other.jpg"], + }); + + expect(replyPayloads).toHaveLength(1); + expect(replyPayloads[0].mediaUrl).toBe("file:///tmp/photo.jpg"); + }); + + it("applies media filter after text filter", () => { + const { replyPayloads } = buildReplyPayloads({ + ...baseParams, + payloads: [{ text: "hello world!", mediaUrl: "file:///tmp/photo.jpg" }], + messagingToolSentTexts: ["hello world!"], + messagingToolSentMediaUrls: ["file:///tmp/photo.jpg"], + }); + + // Text filter removes the payload entirely (text matched), so nothing remains. + expect(replyPayloads).toHaveLength(0); + }); +}); diff --git a/src/auto-reply/reply/followup-runner.test.ts b/src/auto-reply/reply/followup-runner.test.ts index 85a9d35c3d..ebdf83ad09 100644 --- a/src/auto-reply/reply/followup-runner.test.ts +++ b/src/auto-reply/reply/followup-runner.test.ts @@ -257,6 +257,47 @@ describe("createFollowupRunner messaging tool dedupe", () => { expect(onBlockReply).not.toHaveBeenCalled(); }); + it("drops media URL from payload when messaging tool already sent it", async () => { + const onBlockReply = vi.fn(async () => {}); + runEmbeddedPiAgentMock.mockResolvedValueOnce({ + payloads: [{ mediaUrl: "/tmp/img.png" }], + messagingToolSentMediaUrls: ["/tmp/img.png"], + meta: {}, + }); + + const runner = createFollowupRunner({ + opts: { onBlockReply }, + typing: createMockTypingController(), + typingMode: "instant", + defaultModel: "anthropic/claude-opus-4-5", + }); + + await runner(baseQueuedRun()); + + // Media stripped → payload becomes non-renderable → not delivered. + expect(onBlockReply).not.toHaveBeenCalled(); + }); + + it("delivers media payload when not a duplicate", async () => { + const onBlockReply = vi.fn(async () => {}); + runEmbeddedPiAgentMock.mockResolvedValueOnce({ + payloads: [{ mediaUrl: "/tmp/img.png" }], + messagingToolSentMediaUrls: ["/tmp/other.png"], + meta: {}, + }); + + const runner = createFollowupRunner({ + opts: { onBlockReply }, + typing: createMockTypingController(), + typingMode: "instant", + defaultModel: "anthropic/claude-opus-4-5", + }); + + await runner(baseQueuedRun()); + + expect(onBlockReply).toHaveBeenCalledTimes(1); + }); + it("persists usage even when replies are suppressed", async () => { const storePath = path.join( await fs.mkdtemp(path.join(tmpdir(), "openclaw-followup-usage-")),