Skip to content

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

Closed
LiuwqGit wants to merge 4 commits into
openclaw:mainfrom
LiuwqGit:fix/issue-90768-send-buffer-materialize
Closed

fix(outbound): materialize buffer-only message.send attachments#90794
LiuwqGit wants to merge 4 commits into
openclaw:mainfrom
LiuwqGit:fix/issue-90768-send-buffer-materialize

Conversation

@LiuwqGit

@LiuwqGit LiuwqGit commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Summary

What problem does this PR solve?

  • message.send could fail when callers supplied attachment bytes only via buffer + filename + contentType, because the durable send path expected media/mediaUrl paths and never materialized buffer-only payloads.

Why does this matter now?

  • Issue #90768: in-memory artifact delivery workflows hit a dead end unless callers manually staged a local file first (impact:message-loss).

What is the intended outcome?

  • Buffer-only send params are staged into the OpenClaw outbound media store and exposed as media/mediaUrl/mediaUrls before normal channel delivery, preserving the existing URL/path contract, default outbound media byte cap, and dry-run no-write semantics.

What is intentionally out of scope?

  • No per-channel buffer contracts, LM Studio/plugin changes, or sendAttachment behavior changes; explicit media/mediaUrl/path sources are left untouched.

What does success look like?

  • message.send with only buffer + metadata delivers through the core outbound path with a staged outbound media path; oversized buffer-only payloads are rejected before staging; dry-run previews do not write outbound media files.

What should reviewers focus on?

  • resolveSendBufferMaxBytes + materializeSendBufferMediaParams in message-action-params.ts (dryRun gate + MEDIA_MAX_BYTES)
  • resolveOutboundAttachmentFromBuffer in media/outbound-attachment.ts
  • Regression tests in message-action-params.test.ts, message-action-runner.media.test.ts, and message-action-buffer-send.runtime-proof.test.ts

Linked context

Which issue does this close?

Closes #90768

Which issues, PRs, or discussions are related?

Was this requested by a maintainer or owner?

  • Community report; ClawSweeper identified source-repro path and acceptance tests.

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: Buffer-only message.send attachments materialize to outbound media paths before channel dispatch; dry-run previews skip outbound file writes; payloads above the default media byte cap are rejected before staging/dispatch.
  • Real environment tested: Local checkout on branch fix/issue-90768-send-buffer-materialize, Vitest runtime proof on Windows host (E:/Projects/skills/work/openclaw) using real outbound media store I/O (no executeSendAction mocks in L3 proof test).
  • Exact steps or command run after this patch:
    • node scripts/run-vitest.mjs src/infra/outbound/message-action-buffer-send.runtime-proof.test.ts --reporter=verbose
    • node scripts/run-vitest.mjs src/infra/outbound/message-action-params.test.ts src/infra/outbound/message-action-runner.media.test.ts -t "buffer-only|dry run|oversized|materializ"
    • pnpm check:test-types
  • Evidence after fix:
{
  "l3_proof": {
    "environment": "local vitest runtime (no outbound-send mocks)",
    "dry_run": {
      "outbound_file_count_before": 0,
      "outbound_file_count_after_hydrate": 0,
      "outbound_file_count_after_runMessageAction": 0,
      "args_media": null,
      "sendResult_mediaUrl": null
    },
    "real_send": {
      "staged_path": "<redacted>/.openclaw/media/outbound/artifact---<uuid>.txt",
      "staged_contents": "artifact bytes",
      "sendResult_mediaUrl": "<redacted>/.openclaw/media/outbound/artifact---<uuid>.txt",
      "sendResult_delivery": { "messageId": "msg-runtime-proof" }
    }
  },
  "vitest": {
    "filtered": "9 passed (params + runner media + dry-run regressions)",
    "runtime_proof": "1 passed (message-action-buffer-send.runtime-proof.test.ts)"
  },
  "check:test-types": "exit 0"
}
  • Observed result after fix: Dry-run buffer-only hydrateAttachmentParamsForAction / runMessageAction leave outbound media file count unchanged and do not set args.media/sendResult.mediaUrl. Real send stages artifact bytes to a readable outbound path and passes the same bytes through runMessageAction → core delivery.
  • What was not tested: Live Telegram/Slack channel delivery on reporter's self-hosted runtime.
  • Proof limitations: L3 proof uses workspace test plugin + real filesystem staging/delivery path; not an end-to-end external channel screenshot/recording.
  • Before evidence (optional): hydrateAttachmentParamsForAction skipped send; buffer-only sends lacked mediaUrl. Initial draft used unbounded maxBytes and wrote outbound files during dry-run previews.

Tests and validation

Which commands did you run?

  • node scripts/run-vitest.mjs src/infra/outbound/message-action-buffer-send.runtime-proof.test.ts → 1 passed (L3 proof JSON logged)
  • node scripts/run-vitest.mjs src/infra/outbound/message-action-params.test.ts src/infra/outbound/message-action-runner.media.test.ts -t "buffer-only|dry run|oversized|materializ" → 9 passed
  • pnpm check:test-types → exit 0

What regression coverage was added or updated?

  • materializeSendBufferMediaParams skips outbound writes when dryRun is true; still validates byte cap and normalizes metadata.
  • Dry-run + real-send buffer-only coverage in params, runner media, and runtime-proof tests.
  • Oversized buffer-only rejection at materialization and before channel dispatch.

What failed before this fix, if known?

  • Buffer-only message.send never converted buffer into a deliverable mediaUrl; dry-run previews could write outbound media files.

If no test was added, why not?

  • N/A — regression + runtime proof tests added.

Risk checklist

Did user-visible behavior change? (Yes/No)

Yes.

Did config, environment, or migration behavior change? (Yes/No)

No.

Did security, auth, secrets, network, or tool execution behavior change? (Yes/No)

No.

What is the highest-risk area?

  • Staging buffer-only sends into the outbound media store when no explicit media path/URL was provided.

How is that risk mitigated?

  • Skips materialization when any explicit media source exists; enforces MEDIA_MAX_BYTES; preserves dry-run no-write behavior consistent with existing attachment hydration.

Current review state

What is the next action?

  • Maintainer review; ClawSweeper re-review after dry-run + L3 proof updates.

What is still waiting on author, maintainer, CI, or external proof?

  • CI on fork PR.

Which bot or reviewer comments were addressed?

  • ClawSweeper P1: MEDIA_MAX_BYTES cap via resolveSendBufferMaxBytes.
  • ClawSweeper P1/P2: dry-run no-write regression fixed (dryRun gate on send buffer materialization).
  • ClawSweeper P1: added L3 runtime proof with real outbound media store I/O and runMessageAction dry-run vs real-send comparison.

@openclaw-barnacle openclaw-barnacle Bot added size: M proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 6, 2026
@clawsweeper

clawsweeper Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 7, 2026, 4:26 AM ET / 08:26 UTC.

Summary
The branch materializes top-level buffer-only message.send payloads into outbound media paths before channel dispatch and adds focused regression/runtime proof tests.

PR surface: Source +139, Tests +433. Total +572 across 7 files.

Reproducibility: yes. from source: current main advertises top-level buffer, but send only builds payload media from media/path/URL fields and rejects a send with no text or media.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

  • [P2] Run required CI/Testbox checks for the fork PR before merge.

Risk before merge

  • [P1] The PR fixes the advertised top-level buffer send shape, but structured attachments[].buffer remains a separate shape that is not currently advertised by the main message tool schema and is being pursued by another open PR.

Maintainer options:

  1. Decide the mitigation before merge
    Land the core outbound materialization path after maintainer review and required checks, and track structured attachments[].buffer separately if maintainers want that additional contract.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • Keep this PR open for maintainer review and required checks; no ClawSweeper repair is needed from the reviewed diff.

Security
Cleared: The diff adds no dependencies, workflows, scripts, auth, or secret handling, and it stages user-provided bytes through the existing capped outbound media store.

Review details

Best possible solution:

Land the core outbound materialization path after maintainer review and required checks, and track structured attachments[].buffer separately if maintainers want that additional contract.

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

Yes from source: current main advertises top-level buffer, but send only builds payload media from media/path/URL fields and rejects a send with no text or media.

Is this the best way to solve the issue?

Yes: materializing the buffer into the existing outbound media store before channel dispatch preserves the current channel media path/URL contract instead of adding per-channel buffer handling.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix runtime proof with real filesystem outbound media staging, dry-run no-write evidence, observed staged bytes, and focused command output.

Label justifications:

  • P2: The PR addresses a normal-priority message delivery bug where buffer-backed attachments cannot be sent unless callers first stage a file.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): The PR body includes after-fix runtime proof with real filesystem outbound media staging, dry-run no-write evidence, observed staged bytes, and focused command output.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix runtime proof with real filesystem outbound media staging, dry-run no-write evidence, observed staged bytes, and focused command output.
Evidence reviewed

PR surface:

Source +139, Tests +433. Total +572 across 7 files.

View PR surface stats
Area Files Added Removed Net
Source 2 139 0 +139
Tests 5 435 2 +433
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 7 574 2 +572

Acceptance criteria:

  • [P1] node scripts/run-vitest.mjs src/infra/outbound/message-action-buffer-send.runtime-proof.test.ts.
  • [P1] node scripts/run-vitest.mjs src/infra/outbound/message-action-params.test.ts src/infra/outbound/message-action-runner.media.test.ts -t "buffer-only|dry run|oversized|materializ".
  • [P1] pnpm check:test-types.

What I checked:

Likely related people:

  • vincentkoc: Recent merged history touched outbound send attachment routing and the message-action/media test area that this PR modifies. (role: recent outbound media contributor; confidence: medium; commits: 56a7000b3efa; files: src/infra/outbound/message-action-params.ts, src/infra/outbound/message-action-runner.media.test.ts)
  • steipete: Multiple recent commits in the outbound runner and media helper history are adjacent to the send delivery and media staging path. (role: recent outbound runner/media contributor; confidence: medium; commits: b4a63886af4b, 1955f42bfeb2, 3bb02d333865; files: src/infra/outbound/message-action-runner.ts, src/media/outbound-attachment.ts)
  • omarshahine: Adjacent reply-attachment wiring history is relevant because the PR deliberately avoids changing reply/sendAttachment hydration behavior while adding send-specific staging. (role: adjacent reply attachment contributor; confidence: low; commits: 200eb62ef4ce; files: src/infra/outbound/message-action-params.ts, src/infra/outbound/message-action-runner.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: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 6, 2026
@LiuwqGit

LiuwqGit commented Jun 6, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review — addressed prior P1/P2 feedback in 127de6f:

  1. Byte cap: resolveSendBufferMaxBytes now uses resolveAttachmentMaxBytes(...) ?? MEDIA_MAX_BYTES (removed unbounded fallback).
  2. L2 staging proof: Vitest asserts staged path + fs.readFile content + executeSendAction mediaUrl/mediaUrls wiring.
  3. Oversized regression: new tests reject MEDIA_MAX_BYTES + 1 buffer-only sends at materialization and before executeSendAction.

Validation: 6 passed (filtered vitest) + pnpm check:test-types exit 0.

@clawsweeper

clawsweeper Bot commented Jun 6, 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.

Re-review progress:

@LiuwqGit

LiuwqGit commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review — addressed dry-run + L3 proof blockers in 8e88fe7:

  1. dryRun no-write: materializeSendBufferMediaParams now gates outbound file staging on !dryRun; dry-run still validates byte cap + metadata.
  2. L3 runtime proof: message-action-buffer-send.runtime-proof.test.ts uses real outbound media store I/O (no executeSendAction mocks) — dry-run leaves outbound file count unchanged; real runMessageAction send stages artifact bytes to readable path.

Validation: L3 proof 1 passed + filtered vitest 9 passed + pnpm check:test-types exit 0.

@clawsweeper

clawsweeper Bot commented Jun 7, 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.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 7, 2026
@LiuwqGit
LiuwqGit force-pushed the fix/issue-90768-send-buffer-materialize branch from 8e88fe7 to 898319b Compare June 7, 2026 02:34
@openclaw-barnacle openclaw-barnacle Bot added the extensions: memory-core Extension: memory-core label Jun 7, 2026
@LiuwqGit

LiuwqGit commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

CI status note (898319b)

PR-scoped checks are green

  • check-test-types / check-prod-types
  • Real behavior proof
  • Outbound vitest regressions + L3 runtime proof (message-action-buffer-send.runtime-proof.test.ts)

Remaining failures are merge-with-main upstream blockers, not regressions in the #90768 outbound buffer-send diff:

Failed job Root cause (from logs)
check-lint oxlint debt on main after merge: core (1), extensions (8), scripts (7) — none in message-action-params* / outbound-attachment*
check-additional-extension-bundled lint:extensions:bundled (same extensions oxlint set)
check-additional-runtime-topology-architecture check:architecture → Madge 1 import cycle: src/plugins/api-lifecycle.tssrc/plugins/registry-lifecycle.ts
build-artifacts cascade from the above

This branch was rebased onto latest main and includes a small memory-core doctor test typing fix (OpenClawConfig) only to unblock check:test-types after the rebase.

Happy to keep this PR focused on #90768; if maintainers prefer, I can split upstream lint/architecture fixes into a separate PR or wait for main to clear those gates.

LiuwqGit and others added 4 commits June 7, 2026 16:01
Stage buffer + filename/contentType into the outbound media store and
set mediaUrl before core send delivery. Closes openclaw#90768.

Co-authored-by: Cursor <[email protected]>
ClawSweeper re-review: replace unbounded maxBytes fallback with
resolveSendBufferMaxBytes (resolveAttachmentMaxBytes ?? MEDIA_MAX_BYTES)
and add oversized buffer-only send rejection tests at materialization
and runner layers.

Co-authored-by: Cursor <[email protected]>
Gate send buffer materialization on non-dry-run paths, keep byte-cap
validation during previews, and add L3 runtime proof with real outbound
media store I/O plus dry-run vs real-send runMessageAction coverage.

Co-authored-by: Cursor <[email protected]>
Unblocks check:test-types after rebasing onto latest main.

Co-authored-by: Cursor <[email protected]>
@LiuwqGit
LiuwqGit force-pushed the fix/issue-90768-send-buffer-materialize branch from 898319b to 138a477 Compare June 7, 2026 08:18
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 7, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 7, 2026
vincentkoc added a commit that referenced this pull request Jun 7, 2026
Fixes #90768

Incorporates the send-buffer materialization shape proposed in #90794 by @LiuwqGit, with maintainer fixes for dry-run, gateway delivery, byte-cap, target-validation, and downstream plugin dispatch paths.
@vincentkoc

Copy link
Copy Markdown
Member

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

This kept the core shape from this PR: buffer-only message.send input is converted into outbound media before delivery. I landed the canonical version with additional coverage/fixes for dry-run previews, gateway message.action, gateway delivery-mode send, size validation before decode, target-validation ordering, and mixed explicit-media+buffer inputs.

Proof on the landed commit:

Thanks @LiuwqGit. Closing this PR as superseded by the credited main commit.

@vincentkoc vincentkoc closed this Jun 7, 2026
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 8, 2026
Fixes openclaw#90768

Incorporates the send-buffer materialization shape proposed in openclaw#90794 by @LiuwqGit, with maintainer fixes for dry-run, gateway delivery, byte-cap, target-validation, and downstream plugin dispatch paths.
wangmiao0668000666 pushed a commit to wangmiao0668000666/openclaw that referenced this pull request Jun 9, 2026
Fixes openclaw#90768

Incorporates the send-buffer materialization shape proposed in openclaw#90794 by @LiuwqGit, with maintainer fixes for dry-run, gateway delivery, byte-cap, target-validation, and downstream plugin dispatch paths.
badgerbees pushed a commit to badgerbees/openclaw that referenced this pull request Jul 8, 2026
Fixes openclaw#90768

Incorporates the send-buffer materialization shape proposed in openclaw#90794 by @LiuwqGit, with maintainer fixes for dry-run, gateway delivery, byte-cap, target-validation, and downstream plugin dispatch paths.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

extensions: memory-core Extension: memory-core P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: L status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: message.send can fail for buffer-only attachments without an explicit media path or URL

2 participants