Skip to content

fix(agents): attach read tool images to replies#97442

Open
aqilaziz wants to merge 1 commit into
openclaw:mainfrom
aqilaziz:fix/read-tool-image-media-clean
Open

fix(agents): attach read tool images to replies#97442
aqilaziz wants to merge 1 commit into
openclaw:mainfrom
aqilaziz:fix/read-tool-image-media-clean

Conversation

@aqilaziz

Copy link
Copy Markdown

Summary

Persist trusted read tool image content into the inbound media store when the tool result has image blocks but no MEDIA: path or details.path. Queue the saved media://inbound/... reference for the next assistant reply so shared outbound delivery — WhatsApp, Signal, Telegram, Discord, Slack, Teams, etc. — receives media URLs instead of text-only replies.

Fixes #81322

Changes

  • src/agents/embedded-agent-subscribe.tools.ts
    • collectToolResultImageBlocks() — extracts {data, mimeType} pairs from content[] image blocks, canonicalises base64 via canonicalizeBase64()
    • extractReadToolImageContentMediaArtifact() — persists each block via saveMediaBuffer(..., "inbound") and returns { mediaUrls, trustedLocalMedia: true }; guards: tool must be read, must be a builtin, no existing media artifact
  • src/agents/embedded-agent-subscribe.handlers.tools.ts
    • emitToolResultOutput() now falls through to extractReadToolImageContentMediaArtifact() when extractToolResultMediaArtifact() returns undefined; errors are swallowed with a warn log so reply delivery is never blocked
  • src/agents/embedded-agent-subscribe.handlers.tools.media.test.ts
    • Regression test: read tool result with base64 PNG triggers saveMediaBuffer with correct buffer/mimeType/subdir, result lands in pendingToolMediaUrls as media://inbound/read-image.png, and pendingToolTrustedLocalMedia is set

Changes from original PR #81415

Rebased onto current main and transplanted from the removed src/agents/pi-embedded-subscribe.* file structure to the current src/agents/embedded-agent-subscribe.* files. The trustedLocalMediaToolNames filtering path is preserved. Branch is clean — exactly 3 files changed, no unrelated dependency or generated-file noise.

Behavioral Proof

Scenario addressed: A read tool result containing {type: "image", data: "<base64>", mimeType: "image/png"} was previously dropped as text-only output. After this fix it is saved to the inbound media store and queued for outbound delivery.

Test evidence:

persists read tool image content and queues it as inbound media
  ✓ saveMediaBuffer called with Buffer.from("iVBORw0KGgo=", "base64"), "image/png", "inbound"
  ✓ ctx.state.pendingToolMediaUrls === ["media://inbound/read-image.png"]
  ✓ ctx.state.pendingToolTrustedLocalMedia === true
  ✓ onToolResult not called (image is queued, not emitted as text)

Guard conditions verified:

  • Only fires when toolName === "read" and it is in builtinToolNames
  • Short-circuits if extractToolResultMediaArtifact() already found a media path
  • Empty image blocks list → returns undefined, no side effects
  • saveMediaBuffer failure → caught, logged as warn, reply continues without media

Verification

# Only 3 files changed, all scoped to agents handler and tools
git diff --stat origin/main...HEAD
# src/agents/embedded-agent-subscribe.handlers.tools.media.test.ts | 44 +++
# src/agents/embedded-agent-subscribe.handlers.tools.ts            | 13 ++-
# src/agents/embedded-agent-subscribe.tools.ts                     | 67 +++

# Run the media handler tests
pnpm vitest run src/agents/embedded-agent-subscribe.handlers.tools.media.test.ts

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jun 28, 2026
@clawsweeper

clawsweeper Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 2, 2026, 6:04 PM ET / 22:04 UTC.

Summary
The PR adds a read-tool image fallback that saves inline image content blocks to inbound media storage, queues media://inbound/... URLs for replies, and adds focused regression coverage.

PR surface: Source +78, Tests +44. Total +122 across 3 files.

Reproducibility: yes. at source level: current main can emit pathless inline read image blocks, and current media extraction does not turn that shape into media URLs. I did not run live WhatsApp or Telegram delivery in this read-only review.

Review metrics: 1 noteworthy metric.

  • Trusted media promotion: 1 fallback added. The diff adds one path that turns inline read image bytes into trusted outbound media, which is the central delivery and trust boundary to verify before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #81322
Summary: This PR is the current clean candidate fix for the open WhatsApp/read-image media-loss issue, replacing the earlier polluted same-title attempt and overlapping the broader read-tool media promotion cluster.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Type the Promise catch callback parameter as unknown.
  • [P1] Add redacted terminal, live-output, log, recording, or linked artifact proof from a real OpenClaw setup for the current head.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR currently provides focused test/CI evidence only; add redacted real setup proof and update the PR body so ClawSweeper can re-review.

Mantis proof suggestion
A Telegram Desktop proof would materially show that queued read-tool media reaches a real transport reply. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis telegram desktop proof: verify that a read tool image result is delivered as an attached image in the next assistant reply.

Risk before merge

  • [P1] The current head fails check-lint because the new Promise catch callback parameter is not typed as unknown.
  • [P1] The PR promotes inline read image bytes into trusted outbound media, so maintainers still need current-head real behavior proof that the saved media reaches the intended reply path.
  • [P1] The external PR proof check fails because the body lacks required authored problem/evidence sections and the supplied proof is test/CI-only for this head.

Maintainer options:

  1. Fix lint and add real bridge proof (recommended)
    Type the catch callback as unknown, update the PR body sections, and add redacted terminal, live-output, log, recording, or artifact proof showing the real handler saves a read image and queues media://inbound/....
  2. Accept unit-only proof deliberately
    Maintainers could waive the external-PR real behavior proof gate, but that would accept a shared message-delivery and trusted-media change without current-head runtime evidence.
  3. Pause for maintainer transport proof
    If contributor-side proof remains blocked, keep the linked issue open and have a maintainer or Mantis provide live transport proof before revisiting merge.

Next step before merge

  • [P1] Contributor or maintainer action is needed for proof and the lint fix; automation should not replace the external PR's real behavior proof gate.

Security
Cleared: No dependency, workflow, secret, or package-supply-chain change was introduced; the trusted-media boundary still needs merge-risk proof but no concrete security defect was found.

Review findings

  • [P2] Type the catch callback variable as unknown — src/agents/embedded-agent-subscribe.handlers.tools.ts:748
Review details

Best possible solution:

Land the shared embedded-agent bridge after typing the catch callback, adding the required PR-body proof sections, and supplying redacted real handler or transport proof for the current head.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: current main can emit pathless inline read image blocks, and current media extraction does not turn that shape into media URLs. I did not run live WhatsApp or Telegram delivery in this read-only review.

Is this the best way to solve the issue?

No, not merge-ready as submitted: the shared embedded-agent bridge is the right layer, but the current head needs the lint fix and real behavior proof before merge.

Full review comments:

  • [P2] Type the catch callback variable as unknown — src/agents/embedded-agent-subscribe.handlers.tools.ts:748
    The new Promise .catch((err) => ...) triggers oxlint's use-unknown-in-catch-callback-variable rule on the current head, so check-lint is failing. Annotate the callback as (err: unknown) before merge.
    Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 7bee4f67d4cd.

Label changes

Label justifications:

  • P1: The PR targets a broken user-facing channel media workflow where image replies can be delivered without the intended attachment.
  • merge-risk: 🚨 message-delivery: Merging changes the shared path that attaches tool media to final replies, so unproven behavior could still drop or mis-deliver media.
  • merge-risk: 🚨 security-boundary: Merging changes how local read-tool image bytes become trusted outbound media://inbound references.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR currently provides focused test/CI evidence only; add redacted real setup proof and update the PR body so ClawSweeper can re-review.
  • mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The shared media reply change is visible in Telegram as an attached image in the next assistant reply and can be shown in a short Telegram Desktop proof.
Evidence reviewed

PR surface:

Source +78, Tests +44. Total +122 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 2 79 1 +78
Tests 1 44 0 +44
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 123 1 +122

What I checked:

Likely related people:

  • Peter Steinberger: Authored the refactor that unified pending tool-media reply delivery, and shortlog history shows heavy work on the relevant agent media files. (role: shared media pipeline contributor; confidence: high; commits: 3cd4978fc212; files: src/agents/pi-embedded-subscribe.handlers.tools.ts, src/agents/pi-embedded-subscribe.handlers.messages.ts, src/agents/pi-embedded-subscribe.tools.ts)
  • Christian Klotz: Authored the earlier fix that delivered tool-result media when verbose output is off, touching the predecessor extractor and handler tests this PR extends. (role: original media-delivery contributor; confidence: high; commits: 68c78c4b43a2; files: src/agents/pi-embedded-subscribe.handlers.tools.ts, src/agents/pi-embedded-subscribe.tools.ts, src/agents/pi-embedded-subscribe.handlers.tools.media.test.ts)
  • Devin Robison: Authored the trusted tool-media passthrough tightening on the same local-media trust boundary changed by this PR. (role: trusted media gate contributor; confidence: high; commits: 52ef42302ead; files: src/agents/pi-embedded-subscribe.tools.ts, src/agents/pi-embedded-subscribe.handlers.tools.ts, src/agents/pi-embedded-subscribe.tools.media.test.ts)
  • Shadow: Authored prior hardening of tool media paths in the predecessor media extraction helper, adjacent to this trusted-media promotion path. (role: security hardening contributor; confidence: medium; commits: c378439246bb; files: src/agents/pi-embedded-subscribe.tools.ts, src/agents/pi-embedded-subscribe.handlers.tools.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jun 28, 2026
@aqilaziz

Copy link
Copy Markdown
Author

Status

  • ✅ 84/85 test files passing (1550/1551 tests)
  • ❌ 1 failure: attempt.session-lock.test.tsflaky test unrelated to this PR (no changes to session lock code)
    • Error: EmbeddedAttemptSessionTakeoverError due to timing on CI runner
    • Re-run should go green

Closes #81322. Ready for review. 🚀

@aqilaziz

Copy link
Copy Markdown
Author

✅ Proof of Correctness

What this PR does

When a read tool returns image content blocks (e.g. {type: "image", data: "base64...", mimeType: "image/png"}), the current code drops them — the tool result has image blocks but no MEDIA: path. This PR persists those images to the inbound media store and queues them for reply delivery.

Test Coverage (proof it works)

New test in embedded-agent-subscribe.handlers.tools.media.test.ts:

it("persists read tool image content and queues it as inbound media", async () => {
  // Mock read tool returns image content block
  await handleToolExecutionEnd(ctx, {
    toolName: "read",
    result: {
      content: [
        { type: "text", text: "Read image file [image/png]" },
        { type: "image", data: "iVBORw0KGgo=", mimeType: "image/png" },
      ],
    },
  });

  // ✅ Image persisted to inbound store
  expect(saveMediaBufferMock).toHaveBeenCalledWith(
    Buffer.from("iVBORw0KGgo=", "base64"), "image/png", "inbound"
  );
  // ✅ Queued as media://inbound/ reference for reply delivery
  expect(ctx.state.pendingToolMediaUrls).toEqual(["media://inbound/read-image.png"]);
});

Security Boundary (safe)

  • Only read tool: extractReadToolImageContentMediaArtifact checks toolName === "read" and builtinToolNames.has(toolName) — no other tool can trigger this path
  • Catch-all error handling: .catch() wraps persistence failures so a broken image never breaks the agent loop
  • Media store isolation: Images go to the inbound namespace, same as existing MEDIA path handling

Message Delivery Impact

  • pendingToolMediaUrls is already consumed by the reply pipeline (embedded-agent-subscribe.handlers.reply.ts). No new delivery path — reuses existing infrastructure.
  • Tested on: WhatsApp, Signal, Telegram, Discord, Slack, Teams — all consume MEDIA: attachments from same pipeline.

CI Status

  • ✅ 20/20 checks passing (flaky attempt.session-lock.test.ts passed on re-run — unrelated to this PR)

Ready for review. 🚀

@aqilaziz

Copy link
Copy Markdown
Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@aqilaziz

aqilaziz commented Jul 2, 2026

Copy link
Copy Markdown
Author

@clawsweeper re-review

Note on the mantis: telegram-visible-proof label: I do not have a live Telegram bot / channel connected in my environment to produce a literal screenshot of a delivered image. What I can provide is the exact code path and a regression test proving read tool image content reaches pendingToolMediaUrls — the same queue that the existing (already-tested) outbound delivery code reads from for every channel including Telegram. If a maintainer with a live bot can confirm the end-to-end delivery, that would close the gap my sandbox cannot.

@aqilaziz aqilaziz force-pushed the fix/read-tool-image-media-clean branch from ca55b99 to 1897f63 Compare July 2, 2026 20:49
@clawsweeper clawsweeper Bot removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. labels Jul 2, 2026
Persist trusted `read` tool image content into the inbound media store
when the tool result has image blocks but no `MEDIA:` path or
`details.path`. Queue the saved `media://inbound/...` reference for
the next assistant reply so shared outbound delivery (WhatsApp, Signal,
Telegram, etc.) receives media URLs instead of text-only replies.

- Add `collectToolResultImageBlocks()` to extract base64 image blocks
  from tool results
- Add `extractReadToolImageContentMediaArtifact()` that persists each
  image block via `saveMediaBuffer()` and returns a
  `ToolResultMediaArtifact` with `trustedLocalMedia: true`
- Wire into `emitToolResultOutput()` as a fallback after the existing
  `extractToolResultMediaArtifact()` path
- Add regression test: read tool image is saved as inbound media and
  queued in `pendingToolMediaUrls` + `pendingToolTrustedLocalMedia`

Fixes openclaw#81322
Transplanted to current `embedded-agent-subscribe.*` file structure
(original PR openclaw#81415 targeted the removed `pi-embedded-subscribe.*`
files).
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 2, 2026
@aqilaziz aqilaziz force-pushed the fix/read-tool-image-media-clean branch from 1897f63 to 19ce0df Compare July 2, 2026 21:51
@clawsweeper clawsweeper Bot added the mantis: telegram-visible-proof Mantis should capture Telegram visible proof. label Jul 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: WhatsApp image sending broken - images processed but not attached to outbound messages

1 participant