fix(agent-runner): share media-path normalizer to prevent duplicate WhatsApp media reply (#68056)#68111
Conversation
Greptile SummaryFixes a duplicate WhatsApp media send bug (#68056) by sharing a single The fix is purely additive: Confidence Score: 5/5Safe to merge — the change is additive and backward-compatible, with a correct regression test covering the root cause. The fix is minimal and correct: one optional parameter with a No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/auto-reply/reply/agent-runner.media-paths.test.ts
Line: 178-242
Comment:
**Test proves no second construction, but not cache-sharing behavior**
The regression test asserts `createReplyMediaPathNormalizerRuntimeMock` is never called, which correctly proves the root-cause fix (no second independent cache is created). However, because `runEmbeddedPiAgentMock` returns `payloads: []`, the shared normalizer is never actually invoked, so no end-to-end guarantee that one cache-hit prevents a duplicate `resolveOutboundAttachmentFromUrl` call.
Consider returning a media payload (e.g. `{ text: "MEDIA:./out/chart.png" }`) and asserting `resolveOutboundAttachmentFromUrlMock` is called exactly once, which would directly verify the duplicate-send behaviour is gone rather than only proving the structural fix.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "test(agent-runner): regression — createR..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("does not create a second normalizer inside runAgentTurnWithFallback when onBlockReply is provided", async () => { | ||
| // Regression test for openclaw/openclaw#68056. | ||
| // Before the fix, runAgentTurnWithFallback always called createReplyMediaPathNormalizer | ||
| // from reply-media-paths.runtime.js to build its own normalizer instance — separate from | ||
| // the one agent-runner.ts created and passed to buildReplyPayloads. Two separate | ||
| // persistedMediaBySource caches meant the same source could be persisted twice (two UUID | ||
| // outbound files, two WhatsApp sends). | ||
| // | ||
| // After the fix, agent-runner.ts passes its normalizer into runAgentTurnWithFallback, so | ||
| // the .runtime import path is never called from inside that function. | ||
| runEmbeddedPiAgentMock.mockResolvedValue({ | ||
| payloads: [], | ||
| meta: { | ||
| agentMeta: { | ||
| sessionId: "session", | ||
| provider: "anthropic", | ||
| model: "claude", | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| await runReplyAgent({ | ||
| commandBody: "generate chart", | ||
| followupRun: createMockFollowupRun({ | ||
| prompt: "generate chart", | ||
| run: { | ||
| agentId: "main", | ||
| agentDir: "/tmp/agent", | ||
| messageProvider: "whatsapp", | ||
| workspaceDir: "/tmp/workspace", | ||
| }, | ||
| }) as unknown as FollowupRun, | ||
| queueKey: "main", | ||
| resolvedQueue: { mode: "interrupt" } as QueueSettings, | ||
| shouldSteer: false, | ||
| shouldFollowup: false, | ||
| isActive: false, | ||
| isStreaming: false, | ||
| typing: createMockTypingController(), | ||
| sessionCtx: { | ||
| Provider: "whatsapp", | ||
| Surface: "whatsapp", | ||
| To: "chat-1", | ||
| OriginatingTo: "chat-1", | ||
| AccountId: "default", | ||
| MessageSid: "msg-1", | ||
| } as unknown as TemplateContext, | ||
| defaultModel: "anthropic/claude", | ||
| resolvedVerboseLevel: "off", | ||
| isNewSession: false, | ||
| blockStreamingEnabled: false, | ||
| resolvedBlockStreamingBreak: "message_end", | ||
| shouldInjectGroupIntro: false, | ||
| typingMode: "instant", | ||
| opts: { | ||
| onBlockReply: vi.fn(), | ||
| }, | ||
| }); | ||
|
|
||
| // The .runtime import is only used by agent-runner-execution.ts. After the fix, | ||
| // runAgentTurnWithFallback receives the normalizer from the caller and never | ||
| // calls createReplyMediaPathNormalizer itself. | ||
| expect(createReplyMediaPathNormalizerRuntimeMock).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The regression test asserts createReplyMediaPathNormalizerRuntimeMock is never called, which correctly proves the root-cause fix (no second independent cache is created). However, because runEmbeddedPiAgentMock returns payloads: [], the shared normalizer is never actually invoked, so no end-to-end guarantee that one cache-hit prevents a duplicate resolveOutboundAttachmentFromUrl call.
Consider returning a media payload (e.g. { text: "MEDIA:./out/chart.png" }) and asserting resolveOutboundAttachmentFromUrlMock is called exactly once, which would directly verify the duplicate-send behaviour is gone rather than only proving the structural fix.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/agent-runner.media-paths.test.ts
Line: 178-242
Comment:
**Test proves no second construction, but not cache-sharing behavior**
The regression test asserts `createReplyMediaPathNormalizerRuntimeMock` is never called, which correctly proves the root-cause fix (no second independent cache is created). However, because `runEmbeddedPiAgentMock` returns `payloads: []`, the shared normalizer is never actually invoked, so no end-to-end guarantee that one cache-hit prevents a duplicate `resolveOutboundAttachmentFromUrl` call.
Consider returning a media payload (e.g. `{ text: "MEDIA:./out/chart.png" }`) and asserting `resolveOutboundAttachmentFromUrlMock` is called exactly once, which would directly verify the duplicate-send behaviour is gone rather than only proving the structural fix.
How can I resolve this? If you propose a fix, please make it concise.
511e12d to
fc5d913
Compare
…allback to prevent duplicate outbound media
…me not called when normalizer injected
fc5d913 to
b7b1d1a
Compare
|
Landed via rebase onto
Landed commit: Thanks @ayeshakhalid192007-dev! |
2 participants
Summary
Fixes #68056 — a single WhatsApp media reply is sent twice when
blockStreamingis disabled.Root Cause
createReplyMediaPathNormalizerwas called in two separate places, each producing an independent closure with its ownpersistedMediaBySourcecache:agent-runner-execution.ts— insiderunAgentTurnWithFallback, used by the block-reply delivery handleragent-runner.ts— at the call site, used bybuildReplyPayloadsWhen block streaming is off but
onBlockReplyis provided,reply-delivery.tssends media immediately viasendDirectBlockReply(by design — media cannot be reconstructed from final text). Both delivery paths then calledresolveOutboundAttachmentFromUrlfor the same source, each cache-missing and producing a separate UUID outbound file and a separate WhatsApp send.This matches the reporter's instrumentation finding: "The two call sites create separate normalizer instances whose
persistedMediaBySourcecaches are not shared."Changes
agent-runner-execution.ts—runAgentTurnWithFallbacknow accepts an optionalnormalizeMediaPathsparam. When supplied it is used directly; otherwise a local normalizer is created (preserves backward-compat for any callers that do not supply one).agent-runner.ts— passesnormalizeReplyMediaPaths(already created at the call site) intorunAgentTurnWithFallback, so both the block-reply delivery handler andbuildReplyPayloadsshare one closure and onepersistedMediaBySourcecache.Why This Is Safe
Purely additive — an optional parameter with a
??fallback. All callers that do not passnormalizeMediaPathsbehave identically to before. The only behavioural change is that the two delivery paths now share the same normalizer cache, which is the intended behaviour.Testing
Added regression test in
agent-runner.media-paths.test.ts: mocks the.runtimeimport path used exclusively byagent-runner-execution.tsand asserts thatcreateReplyMediaPathNormalizerfrom that path is never called after the fix (because the injected normalizer fromagent-runner.tsis used instead).Local CI results:
auto-reply-replytest suite: 1073/1073 passed ✓pnpm test:bundled: 103/103 passed ✓pnpm check/pnpm build/pnpm test:contracts: pre-existing failures present identically onmain— not introduced by this PRFixes #68056