fix(message-tool): hydrate structured reply attachments#86870
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5ecb1cafd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const structuredAttachmentMode = action === "send" ? "all" : "selected"; | ||
|
|
||
| await normalizeSandboxMediaParams({ | ||
| args: params, | ||
| mediaPolicy: normalizationPolicy, |
There was a problem hiding this comment.
Gate attachment normalization to attachment-capable actions
structuredAttachmentMode is set to "selected" for every non-send action, so attachments[] are now normalized and fed into media-access root expansion even when the action never consumes attachments. Since hydrateAttachmentParamsForAction only hydrates sendAttachment, setGroupIcon, reply, and upload-file, unrelated actions can now fail early (for example on data: URLs or disallowed local paths in a stray attachments field) instead of ignoring that extra input as before. Restrict structured-attachment processing to the attachment-capable actions to avoid this regression.
Useful? React with 👍 / 👎.
Summary
Fixes outbound message actions so structured
attachments[]media is visible to the same sandbox, local-root, and hydration path as top-levelmedia/pathhints.This keeps the real user-facing reply image fix from #86724, but avoids broadening media roots or copying metadata from ignored attachments:
sendcollects and sandbox-normalizes every structured attachment because core sends every entry.reply/sendAttachment/setGroupIcon/upload-fileselect one structured attachment only when no explicit top-level media source wins.attachments[]entries do not expand roots or leak filename/contentType.Verification
pnpm exec oxfmt --write src/infra/outbound/message-action-params.ts src/infra/outbound/message-action-params.test.ts src/infra/outbound/message-action-runner.ts src/infra/outbound/message-action-runner.media.test.tsgit diff --checkpnpm tsgo:core && pnpm tsgo:test:srctsxselector/hydration probe covering selected attachment, top-level precedence, plugin media precedence, all send attachment sources, and ignored attachment metadata/Users/steipete/Projects/agent-scripts/skills/autoreview/scripts/autoreview --mode local --prompt "Review this outbound message attachment fix after adding selected/all structured attachment modes and passing plugin media-source gates into hydration. Focus on attachments[] source selection, sandbox/local-root ordering, and avoiding metadata/root expansion from ignored attachments. Do not ask for broader refactors."Autoreview result: clean, no accepted/actionable findings.
Vitest note:
node scripts/run-vitest.mjs src/infra/outbound/message-action-params.test.ts src/infra/outbound/message-action-runner.media.test.tspassed before the final reviewer follow-up change. After the final change, local Vitest collection repeatedly CPU-spun in this checkout even with a fresh cache/single worker;tsgoand the direct runtime probe cover the changed selector/hydration behavior.Real behavior proof
Behavior addressed: reply and other single-attachment actions now honor
attachments[]media without bypassing existing outbound media checks.Real environment tested: local OpenClaw checkout.
Exact steps or command run after this patch:
pnpm tsgo:core && pnpm tsgo:test:src;git diff --check; directtsxselector/hydration probe; autoreview local.Evidence after fix: structured attachment sources are selected only when no top-level/plugin media source wins; send collects all structured attachment sources; hydration copies metadata only from the selected attachment.
Observed result after fix: probes and type checks passed; autoreview clean.
What was not tested: final Vitest rerun did not complete locally because collection CPU-spun in this checkout after the final follow-up patch.