[Fix] Cap Lark/Feishu inbound media downloads#81044
Conversation
|
Codex review: needs maintainer review before merge. Summary Reproducibility: yes. Current main source shows the configured maxBytes is enforced only after Feishu SDK responses are materialized, and the PR body supplies before/after targeted test output plus live over-limit Feishu p2p runtime logs. Real behavior proof Next step before merge Security Review detailsBest possible solution: Merge the focused Feishu plugin fix after required CI and maintainer review pass, keeping saveMediaBuffer as defense-in-depth and leaving SDK-level temp-file download cancellation to a separate follow-up if the SDK exposes it. Do we have a high-confidence way to reproduce the issue? Yes. Current main source shows the configured maxBytes is enforced only after Feishu SDK responses are materialized, and the PR body supplies before/after targeted test output plus live over-limit Feishu p2p runtime logs. Is this the best way to solve the issue? Yes. Passing the existing cap into the Feishu download seam and checking each supported SDK response shape is the narrow maintainable fix without changing config, shared media-store ownership, or outbound behavior. What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 186de9daa0ed. |
|
@clawsweeper re-review |
|
@codex review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Signed-off-by: samzong <samzong.lu@gmail.com>
1a7827c to
c699b9e
Compare
|
Thanks @samzong. I landed this as a broader core media-download refactor on main instead of updating this PR branch, because the right fix ended up being shared Plugin SDK/runtime helpers rather than another Feishu-only cap. Landed commit: 53d007b What landed:
Verification:
Closing this PR because the generalized fix is now on main with your contribution credited. |
Summary
Describe the problem and fix in 2-5 bullets:
If this PR fixes a plugin beta-release blocker, title it
fix(<plugin-id>): beta blocker - <summary>and link the matchingBeta blocker: <plugin-name> - <summary>issue labeledbeta-blocker. Contributors cannot label PRs, so the title is the PR-side signal for maintainers and automation.saveMediaBufferrejected them.maxBytes, reject over-limitBuffer,ArrayBuffer,data, readable stream, async iterable, andwriteFiletemp-file responses, and inbound message handling passes the same cap into the download helper.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Real behavior proof (required for external PRs)
External contributors must show after-fix evidence from a real OpenClaw setup. Unit tests, mocks, lint, typechecks, snapshots, and CI are supplemental only. Screenshots are encouraged even for CLI, console, text, or log changes; terminal screenshots and copied live output count. Be mindful of private information like IP addresses, API keys, phone numbers, non-public endpoints, or other private details when providing evidence.
channels.feishu.mediaMaxMb = 1.pnpm openclaw gateway status --deep.pnpm openclaw channels status --deep./tmp/openclaw/openclaw-2026-05-12.logfor Feishu/media/download lines.Supplemental targeted regression proof:
Feishu message resource download failed: payload exceeds maxBytes 1048576, before the older final persistence cap errorMedia exceeds 1MB limit.Media exceeds 1MB limit. The targeted command also failed before the production fix with 5 failures: the bot download request hadmaxBytes: undefined, and the buffer-like, readable, async-iterable, andwriteFileover-limit fixtures resolved instead of rejecting.Root Cause (if applicable)
For bug fixes or regressions, explain why this happened, not just what changed. Otherwise write
N/A. If the cause is unclear, writeUnknown.downloadMessageResourceFeishudid not accept the inbound media cap, andreadFeishuResponseBufferreturned fully materialized buffers for Feishu SDK response shapes before any size check.saveMediaBufferhad the final persistence cap, which hid the fact that download/read-back had already materialized the payload.Regression Test Plan (if applicable)
For bug fixes or regressions, name the smallest reliable test coverage that should catch this. Otherwise write
N/A.extensions/feishu/src/media.test.ts,extensions/feishu/src/bot.test.ts.writeFileresponses, and inbound media download receives the configuredmaxBytesbefore persistence.User-visible / Behavior Changes
Over-limit Feishu inbound media is rejected earlier during download/read-back instead of only at final media-store persistence.
Diagram (if applicable)
For UI changes or non-trivial logic flows, include a small ASCII diagram reviewers can scan quickly. Otherwise write
N/A.Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation: N/ARepro + Verification
Environment
channels.feishu.mediaMaxMb = 1; Feishu app credentials configured locally and redacted;channels.feishu.mediaMaxMb = 1Steps
pnpm test extensions/feishu/src/media.test.ts extensions/feishu/src/bot.test.ts -- --reporter=verbose.Expected
downloadMessageResourceFeishu.Actual
pnpm check:changedpassed conflict-marker, changelog attribution, wildcard re-export, duplicate coverage, extension typecheck, and extension-test typecheck gates, then failed on an unrelated pre-existing browser lint issue:extensions/browser/src/cli/browser-cli-state.option-collisions.test.ts:41 no-control-regex.Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
git diff --check, and pre-ship review scan.readFile, and inbound configuredmediaMaxMbpropagation.AI-assisted disclosure
CHANGELOG.mdchanges are included.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
No existing bot review conversations for this new PR.
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoRisks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.writeFilepath still downloads the resource into a temp file before OpenClaw can inspect file size.saveMediaBufferas the final cap. A deeper SDK-level streaming/cancel path would be a separate change if Feishu exposes a suitable API.