fix(message-tool): hydrate attachments[] on reply (and other single-attachment actions)#86724
fix(message-tool): hydrate attachments[] on reply (and other single-attachment actions)#86724omarshahine wants to merge 2 commits into
Conversation
|
Codex review: needs changes before merge. Reviewed May 26, 2026, 2:58 AM ET / 06:58 UTC. Summary PR surface: Source +53, Tests +51. Total +104 across 2 files. Reproducibility: yes. Current source shows the schema accepts Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Mantis proof suggestion Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge Security Review findings
Review detailsBest possible solution: Keep the shared hydrator approach, but choose attachment source and metadata atomically and make Do we have a high-confidence way to reproduce the issue? Yes. Current source shows the schema accepts Is this the best way to solve the issue? No. The shared hydrator is the right boundary, but this patch needs a narrower correction so source selection, metadata, sandbox normalization, and host media root collection remain consistent. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 3b023e9bdb62. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +53, Tests +51. Total +104 across 2 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
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 PR egg 🔥 Warming up: real-behavior proof passed; findings, security review, or rank-up moves are still in progress. Hatch commandComment Hatchability rules:
What is this egg doing here?
|
End-to-end repro evidence on a live Mac iMessage gatewayCaptured from a live agent session running this branch ( Patched runner:
|
End-to-end verification: patched code path delivers a real iMessage attachmentCaptured on a live Mac iMessage gateway running this branch ( DM (chat_id=3) — uses the patched
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper what happened to our re-review? |
|
🦞👀 I queued a lightweight read-only assist pass. It will post a separate answer comment and will not edit the durable ClawSweeper review comment or trigger close, merge, repair, label, or branch changes. Request: what happened to our re-review? |
|
ClawSweeper assist: the re-review appears to have run and updated the existing ClawSweeper review comment, but it did not produce a clean approval. The current durable review still says “needs changes before merge” for head SHA Evidence:
Suggested next action: ask for a full fresh correctness pass with Source: #86724 (comment) |
|
@clawsweeper review |
|
🦞👀 Command router queued. I will update this comment with the next step. Re-review progress:
|
…roupIcon/upload-file
The message-tool schema (src/agents/tools/message-tool.ts:194) advertises a
structured `attachments: [{path, mimeType, name, …}]` array as a peer of the
top-level `media|mediaUrl|path|filePath` slots. The `send` action consumed it
via `collectMessageAttachmentMediaHints` in the runner, but the single-attachment
hydration path used by `reply`, `sendAttachment`, `setGroupIcon`, and
`upload-file` (`hydrateAttachmentActionPayload` in message-action-params.ts) only
looked at top-level keys. Agents that followed the published schema and passed
`attachments: [{path: "/abs/pic.png", mimeType: "image/png", name: "pic.png"}]`
to `action: "reply"` had hydration find no hint, no buffer was loaded, and the
channel handler shipped text-only with the attachment silently dropped. No
error was raised because `extractReplyAttachment` only throws on a *bypass*
shape (top-level path with no buffer), not on absence — and `attachments[]`
left every top-level slot empty.
Fix: lift the first attachment's path / mimeType / filename into the
top-level slots inside `hydrateAttachmentActionPayload` before reading hints.
Backward-compatible (only fills empty slots; explicit top-level hints win) and
preserves all existing sandbox / localRoots / size enforcement, since the
loaded `mediaSource` still flows through `loadWebMedia` with the resolved
policy.
Bug surface: every channel's `reply` action (iMessage, Telegram, WhatsApp,
Discord, BlueBubbles, …), since they all share the same core hydration step.
|
@clawsweeper re-review Addressed the prior P1 scope finding on head Changes:
Verification:
|
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
6a2cb8c to
01e7b3c
Compare
|
@clawsweeper re-review Rebased onto current |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review Addressed the two P2 findings on head Changes:
Verification:
Known proof gap: this is still a unit/integration proof for the shared outbound runner path. No live channel proof was added for this shared-core metadata/root-expansion fix. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@steipete you were right to call this out. I fixed the shared outbound issues instead of trying to explain them away. What changed on
Why we missed it: the earlier pass was focused on the user-visible iMessage attachment symptom and the first ClawSweeper scope finding. I had run focused tests and ClawSweeper review, but I had not run the repo’s Codex autoreview closeout on the corrected branch before asking for another look. That was the process miss; the root policy documents this under Process change for this PR and future nontrivial code PRs: before pushing/landing, run |
|
Closing as superseded by #86870 (Peter Steinberger, merged 2026-05-26T13:50:33Z). His fix lands the same conceptual change with a cleaner abstraction:
End-to-end iMessage delivery via patched runner was verified live during this PR's work (DM chat 3 at 03:00:57Z + group chat 22 at 03:01:18Z, both |
Summary
Agents that followed the published
messagetool schema and passedattachments: [{ path, mimeType, name }]onaction: "reply"had the file silently dropped — only the text portion shipped. No error was raised. Same gap onsendAttachment,setGroupIcon, andupload-file.Root cause
src/agents/tools/message-tool.ts:194declaresattachments[]as a top-level peer ofmedia|mediaUrl|path|filePath|bufferetc., withpath|filePath|media|mediaUrl|fileUrl|url|mimeType|nameper-entry. No action scoping.src/infra/outbound/message-action-runner.ts:506(collectMessageAttachmentMediaHints) readsattachments[]— but it's only called frombuildSendPayloadParts, which is only reached onaction: "send"and the webchat internal-source-reply sink. Every other action path never seesattachments[].src/infra/outbound/message-action-params.ts:353(hydrateAttachmentActionPayload), used byreply/sendAttachment/setGroupIcon/upload-file, reads only top-levelmedia|mediaUrl|path|filePath|fileUrlfor hints.args.buffernever gets populated.replyhandler then sees no hydrated buffer and no top-level path. E.g.extensions/imessage/src/actions.ts:290extractReplyAttachmentreturnsnull(only throws on the bypass case — a top-level path without a buffer — not on absence), so the reply ships text-only. Silent drop.Affects every channel that supports
replywith media — iMessage, Telegram, WhatsApp, Discord, BlueBubbles, etc. — because they all share the same hydration step.Fix
Lift the first usable
attachments[]entry into the top-level hint slots insidehydrateAttachmentActionPayloadbefore reading them:path|filePath|media|mediaUrl|fileUrl|url→args.path(if no top-level hint and no buffer already)mimeType|contentType→args.contentType(canonical; hydration falls back to mimeType too)name|filename→args.filenameBackwards-compatible: only fills empty slots, explicit top-level hints win. The lifted path still flows through
loadWebMediawith the resolvedmediaPolicy, so all sandbox /mediaLocalRoots/ size enforcement is preserved. No security boundary changes.How I found it
Real-world repro: I (the lobster gateway, a personal agent on a Mac running
openclaw 2026.5.26) was asked to "send me an image of a red coffee mug on a blue table." I wrote an SVG viaapply_patch, then invokedmessage(action: "reply", attachments: [{path: ".../tmp-red-mug-blue-table.svg", mimeType: "image/svg+xml", name: "..."}]). The owner replied "Nothing was sent." I retried with a real PNG path inattachments[]— same result.imsg history --chat-id <dm> --attachmentsconfirmed the text portion landed both times but no attachment row was attached. Tracing it back fromimessageMessageActions.handleActionshowed the iMessage handler is fine (it gets called with no buffer and no path-bypass keys;extractReplyAttachmentcorrectly returnsnull). The hydration step never sawattachments[].Test plan
src/infra/outbound/message-action-runner.media.test.tsfor thereplyaction:hydrates buffer/filename/contentType from attachments[] on replyprefers an explicit top-level hint over attachments[] entriesmessage-action-runner.media.test.tspass (24 pre-existing + 2 new).src/infra/outboundsuite: pre-existing failure count drops from 119 → 107 with this patch (no new failures introduced; the lift also clears 12 pre-existing failures where the same shape was tested elsewhere).