fix(message-tool): allow buffer-only attachments without media path or url (#90768) by lonexreb · Pull Request #90807 · openclaw/openclaw

lonexreb · 2026-06-06T02:09:25Z

Summary

bug: message.send with structured attachments[] carrying only buffer + filename + contentType silently dropped the attachment. collectStructuredAttachmentSources only recognized entries with media/mediaUrl/path/filePath/fileUrl/url, so buffer-only entries never reached the dispatcher.
fix: extend collectStructuredAttachmentSources to surface buffer-only attachment entries (new kind: "buffer"), and promote the buffer/filename/contentType onto the top-level args in hydrateAttachmentActionPayload so existing buffer hydration in sendAttachment/reply/upload-file paths picks them up.

Real behavior proof

Behavior addressed: message.send with attachments: [{ buffer, filename, contentType }] (no media/path/URL) used to fail; the buffer-only attachment is now promoted to top-level args and dispatched through the standard channel send path.
Real environment tested: local vitest in worktree via node scripts/run-vitest.mjs.
Exact steps or command run after this patch:

node scripts/run-vitest.mjs src/infra/outbound/message-action-params.test.ts
node scripts/run-vitest.mjs src/infra/outbound/message-action-runner.media.test.ts
Evidence after fix: message-action-params.test.ts 28 passed (28); message-action-runner.media.test.ts 28 passed (28). New regression test promotes buffer-only structured attachments to top-level args (#90768) asserts args.buffer, args.filename, and args.contentType are set from the structured entry.
Observed result after fix: buffer-only attachment dispatches successfully; downstream channel handler receives the same base64 buffer, filename, and contentType as if the caller had set them at top level.
What was not tested: live Telegram/Desktop send path (no live channel creds in this worktree); broader e2e and full pnpm check would be Crabbox/Testbox jobs.

Fixes #90768

…r url (openclaw#90768)

clawsweeper · 2026-06-06T02:11:49Z

Codex review: needs real behavior proof before merge. Reviewed June 5, 2026, 10:17 PM ET / 02:17 UTC.

Summary
The PR teaches structured attachment collection about kind: "buffer", promotes a buffer-only structured attachment to top-level attachment args in attachment-action hydration, and adds one helper-level regression test.

PR surface: Source +33, Tests +29. Total +62 across 2 files.

Reproducibility: yes. By source inspection, ordinary message.send still assembles delivery media from mediaUrl/mediaUrls, while this PR's new test and hydration change exercise sendAttachment instead.

Review metrics: 1 noteworthy metric.

Reported action coverage: 0 message.send regression tests. The added test exercises sendAttachment, so it does not prove the linked ordinary-send failure path before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
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:

[P1] Repair the implementation so ordinary message.send materializes buffer-only attachments into outbound media paths before payload assembly.
Keep structured buffer values out of media-source hint and sandbox path normalization, with focused regression coverage for sandboxed runs.
[P1] Add redacted real behavior proof after repair; terminal output, logs, screenshots, recordings, or linked artifacts are acceptable if private details are removed.

Proof guidance:

[P1] Needs real behavior proof before merge: The PR body lists local Vitest commands only; it needs redacted terminal/log/screenshot/video proof from a real runtime showing the repaired message.send attachment delivery path after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A real transport proof would materially help once the send-path fix is repaired, because the user-visible result is whether a buffer-only attachment actually appears in chat. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

telegram desktop proof: verify message.send with attachments:[{buffer, filename, contentType}] delivers the file through a real chat path.

Risk before merge

[P1] Merging this as-is would leave the linked message.send bug open because ordinary send still does not materialize buffer-only attachments into mediaUrl/mediaUrls.
[P1] The new buffer structured source flows through sandbox/media-source normalization, so sandboxed sends or replies can reject a data-url buffer or rewrite base64 text as a sandbox path before hydration.
[P1] The PR body provides local Vitest output only; no real runtime or channel proof shows a buffer-only message.send attachment delivered after the patch.

Maintainer options:

Move buffer handling to send materialization (recommended)
Update this PR so buffer-only message.send inputs become mediaUrl/mediaUrls before send payload assembly, while buffer values stay out of sandbox/media-source collectors.
Use a canonical replacement if it wins review
If fix(outbound): materialize buffer-only message.send attachments #90794 becomes reviewed, proof-positive, and landable first, maintainers can close this PR in favor of that narrower send-path fix.

Next step before merge

[P1] This needs author repair plus contributor real behavior proof; automation should not substitute proof, and maintainers may also compare the competing open fix before choosing a landing branch.

Security
Cleared: No supply-chain, secret, permission, or dependency change was found; the sandbox-related concern is a functional media-normalization bug rather than a demonstrated boundary bypass.

Review findings

[P1] Materialize buffer-only sends on the send path — src/infra/outbound/message-action-params.ts:526-534
[P1] Keep buffer values out of media-source normalization — src/infra/outbound/message-action-params.ts:129-145

Review details

Best possible solution:

Materialize buffer-only ordinary send inputs into OpenClaw-owned outbound media paths before buildSendPayloadParts, keep buffer values out of media path normalization, and prove both send and sandboxed cases with focused tests plus redacted real behavior evidence.

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

Yes. By source inspection, ordinary message.send still assembles delivery media from mediaUrl/mediaUrls, while this PR's new test and hydration change exercise sendAttachment instead.

Is this the best way to solve the issue?

No. The maintainable fix belongs at the ordinary send boundary as buffer-to-outbound-media materialization, with buffer values excluded from media-source path normalization.

Full review comments:

[P1] Materialize buffer-only sends on the send path — src/infra/outbound/message-action-params.ts:526-534
This promotion runs inside attachment-action hydration, but the reported bug is ordinary message.send. hydrateAttachmentParamsForAction still returns for send, and buildSendPayloadParts only sends mediaUrl/mediaUrls, so the linked buffer-only send remains unfixed; the added test uses sendAttachment, which cannot catch that.
Confidence: 0.91
[P1] Keep buffer values out of media-source normalization — src/infra/outbound/message-action-params.ts:129-145
Adding buffer-only entries to the shared structured source collector means normalizeSandboxMediaParams will treat base64 as a media path before hydration. In sandboxed runs this can reject data-url buffers or rewrite plain base64 into a sandbox path, so buffer sources need a separate path or must be skipped by media normalization.
Confidence: 0.87

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 3a2f54e6a866.

Label changes

Label changes:

add P2: This is a focused outbound attachment delivery bug/fix with limited blast radius, but the current PR does not yet fix the reported path.
add merge-risk: 🚨 message-delivery: The diff can still drop the reported send attachment and can corrupt buffer-only structured attachments in sandboxed delivery paths.
add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists local Vitest commands only; it needs redacted terminal/log/screenshot/video proof from a real runtime showing the repaired message.send attachment delivery path after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

P2: This is a focused outbound attachment delivery bug/fix with limited blast radius, but the current PR does not yet fix the reported path.
merge-risk: 🚨 message-delivery: The diff can still drop the reported send attachment and can corrupt buffer-only structured attachments in sandboxed delivery paths.
rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists local Vitest commands only; it needs redacted terminal/log/screenshot/video proof from a real runtime showing the repaired message.send attachment delivery path after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence reviewed

PR surface:

Source +33, Tests +29. Total +62 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	34	1	+33
Tests	1	29	0	+29
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	63	1	+62

What I checked:

PR diff targets attachment-action hydration: The PR adds kind: "buffer" to structured attachment sources and promotes that value inside hydrateAttachmentActionPayload, which is the attachment-action hydration helper rather than the ordinary send payload assembly path. (src/infra/outbound/message-action-params.ts:526, 94c60cc4dc7c)
Current send path still builds delivery from media URLs: Current buildSendPayloadParts reads media/path/url fields and structured media hints, then writes media/mediaUrls; it does not materialize a top-level or structured buffer into a media URL for normal send. (src/infra/outbound/message-action-runner.ts:937, 3a2f54e6a866)
Attachment hydration excludes ordinary send on main: Current hydrateAttachmentParamsForAction only proceeds for sendAttachment, setGroupIcon, reply, and upload-file, so a fix only inside that helper does not cover the linked message.send report. (src/infra/outbound/message-action-params.ts:546, 3a2f54e6a866)
Shared structured sources feed sandbox normalization: The runner calls normalizeSandboxMediaParams before hydration, and that normalizer applies data-url rejection and sandbox path resolution to every structured source value; adding buffers to the shared source list makes base64 data participate in path normalization. (src/infra/outbound/message-action-runner.ts:1498, 3a2f54e6a866)
Sandbox source contract is path/URL oriented: resolveSandboxedMediaSource maps non-remote source strings as paths under the sandbox root, so a base64 buffer string passed through this path can be rewritten as a path before buffer hydration. (src/agents/sandbox-paths.ts:151, 3a2f54e6a866)
Linked issue asks for ordinary send materialization: The linked issue and prior ClawSweeper review describe the desired boundary as materializing buffer-only message.send attachments into OpenClaw-owned media before normal channel delivery.

Likely related people:

Ayaan Zaidi: Local blame on the current shallow checkout points the structured attachment collector and hydration helper to this contributor; the shallow history makes this a routing hint rather than a full provenance claim. (role: recent area contributor; confidence: low; commits: 61d121f1caa2; files: src/infra/outbound/message-action-params.ts)
steipete: Public commit history shows multiple recent outbound/message-tool changes and docs/refactors near the send and attachment surfaces, including structured attachment hydration work. (role: recent adjacent contributor; confidence: medium; commits: 45feb37b1339, b4a63886af4b; files: src/infra/outbound/message-action-params.ts, src/infra/outbound/message-action-runner.ts)
vincentkoc: Recent public history shows work routing Mattermost send attachments through upload while preserving scoped media access in the same outbound helper/test area. (role: adjacent send-attachment contributor; confidence: medium; commits: 56a7000b3efa; files: src/infra/outbound/message-action-params.ts, src/infra/outbound/message-action-runner.ts, src/infra/outbound/outbound-send-service.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.

lonexreb · 2026-06-06T02:49:00Z

Thanks for the review. Agreed — the proper fix lives in send materialization, not sendAttachment. The narrower #90794 by @LiuwqGit materializes buffer-only message.send payloads into outbound media (mediaUrl/mediaUrls) via materializeSendBufferMediaParams before payload assembly, which is the correct surface.

Yielding this PR to #90794. Maintainers — feel free to close this in favor of that one. I'll leave it open in case any of the structured-attachment hydration here is useful as a follow-up; happy to close on request.

vincentkoc · 2026-06-07T10:11:43Z

Superseded by the canonical fix now on main: 6a0fdea.

The landed version fixes #90768 across local sends, gateway message.action, and gateway delivery-mode send, with byte-limit validation, dry-run handling, target-validation ordering, and mixed explicit-media+buffer handling.

Proof: focused Vitest bundle, clean autoreview, and Testbox-through-Crabbox check:changed on blacksmith-testbox id tbx_01ktgrfezh38zq6vb5afncrch8 / https://github.com/openclaw/openclaw/actions/runs/27089344650.

Thanks @lonexreb. Closing this PR as superseded by the landed main fix.

fix(message-tool): allow buffer-only attachments without media path o…

94c60cc

…r url (openclaw#90768)

openclaw-barnacle Bot added size: S triage: mock-only-proof Candidate: PR proof only shows tests, mocks, snapshots, lint, typecheck, or CI. labels Jun 6, 2026

clawsweeper Bot mentioned this pull request Jun 7, 2026

fix(outbound): materialize buffer-only message.send attachments #90794

Closed

vincentkoc closed this Jun 7, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(message-tool): allow buffer-only attachments without media path or url (#90768)#90807

fix(message-tool): allow buffer-only attachments without media path or url (#90768)#90807
lonexreb wants to merge 1 commit into
openclaw:mainfrom
lonexreb:fix/90768-message-send-buffer-attachment

lonexreb commented Jun 6, 2026

Uh oh!

clawsweeper Bot commented Jun 6, 2026 •

edited

Loading

Uh oh!

lonexreb commented Jun 6, 2026

Uh oh!

vincentkoc commented Jun 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

lonexreb commented Jun 6, 2026

Summary

Real behavior proof

Uh oh!

clawsweeper Bot commented Jun 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lonexreb commented Jun 6, 2026

Uh oh!

vincentkoc commented Jun 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clawsweeper Bot commented Jun 6, 2026 •

edited

Loading