Skip to content

fix(video-generation): bound DashScope JSON response reads#96248

Closed
cxbAsDev wants to merge 2 commits into
openclaw:mainfrom
cxbAsDev:fix/bound-dashscope-video-response
Closed

fix(video-generation): bound DashScope JSON response reads#96248
cxbAsDev wants to merge 2 commits into
openclaw:mainfrom
cxbAsDev:fix/bound-dashscope-video-response

Conversation

@cxbAsDev

@cxbAsDev cxbAsDev commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Replace raw response.json() with readResponseWithLimit in DashScope video generation submit and poll paths to prevent unbounded buffering.

Real behavior proof (required for external PRs)

Behavior addressed: Two unbounded response.json() calls replaced with readResponseWithLimit (16 MB cap).

Real environment tested: Linux, Node 24, OpenClaw main @ 0671c08

Exact steps or command run after fix:

node --import tsx proof-dashscope-video.mts

Evidence after fix:

cap: 16777216 bytes (16 MiB)
chunks pulled: 17
error: DashScope video-generation response exceeds 16777216 bytes
PASS: reader stopped at cap

The harness creates a streaming JSON body. The reader stops at 17 chunks (16 MB cap), cancels the stream, and throws. Before the fix, unbounded response.json() would have buffered the entire body.

Observed result after fix: Both submit and poll response reads bounded at 16 MB.

What was not tested: Live DashScope API endpoint was not tested.

Risk

Low. readResponseWithLimit is already imported in the same file for video downloads.

@cxbAsDev @claude
fix(video-generation): bound DashScope JSON response reads
aac17b5 
Replace raw response.json() with readResponseWithLimit to prevent
unbounded buffering of DashScope video generation API responses
(submit and poll paths), matching the existing provider response
read pattern.

A malicious or misconfigured DashScope-compatible endpoint could
stream an unbounded JSON body, forcing the runtime to buffer it all
before parsing.

Co-Authored-By: Claude <[email protected]>
@openclaw-barnacle openclaw-barnacle Bot added size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jun 24, 2026
@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed June 24, 2026, 1:57 AM ET / 05:57 UTC.

Summary
The PR replaces two DashScope video-generation submit and poll response.json() reads with 16 MiB readResponseWithLimit parsing.

PR surface: Source +17. Total +17 across 1 file.

Reproducibility: yes. source inspection shows current main and v2026.6.10 use raw response.json() for DashScope submit and poll success bodies, and the PR body includes terminal output from an oversized streaming-body harness. I did not run tests because this is a read-only review.

Review metrics: 1 noteworthy metric.

  • Bounded DashScope JSON readers: 2 changed. Both unbounded DashScope video-generation success-body reads are covered, so validation should cover submit and poll paths together.

Stored data model
Persistent data-model change detected: serialized state: src/video-generation/dashscope-compatible.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🐚 platinum hermit
Patch quality: 🦪 silver shellfish
Result: blocked by patch quality or review findings.

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

Rank-up moves:

  • Update the shared DashScope video test helper and direct Qwen test mocks to return real JSON Response bodies.
  • [P2] Run focused Qwen and Alibaba video provider tests after the mock repair.

Risk before merge

  • [P1] Existing Qwen/Alibaba DashScope provider tests and any harnesses using json-only response doubles will fail until those mocks return real Response bodies or arrayBuffer-compatible objects.

Maintainer options:

  1. Decide the mitigation before merge
    Keep the bounded-read implementation, but merge only after the DashScope test helpers and direct provider test mocks return real JSON Response bodies and focused provider tests pass.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] The remaining blocker is a concrete mechanical test-contract repair, not product or ownership judgment.

Security
Cleared: The diff narrows an oversized provider-response memory risk and does not add dependencies, scripts, permissions, secret handling, or new code execution paths.

Review findings

  • [P2] Return real Response bodies from DashScope mocks — src/video-generation/dashscope-compatible.ts:275
Review details

Best possible solution:

Keep the bounded-read implementation, but merge only after the DashScope test helpers and direct provider test mocks return real JSON Response bodies and focused provider tests pass.

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

Yes, source inspection shows current main and v2026.6.10 use raw response.json() for DashScope submit and poll success bodies, and the PR body includes terminal output from an oversized streaming-body harness. I did not run tests because this is a read-only review.

Is this the best way to solve the issue?

Yes for the runtime layer: using the existing bounded response reader is the narrow maintainable fix. The PR is incomplete until adjacent DashScope provider mocks are updated to satisfy that response contract.

Full review comments:

  • [P2] Return real Response bodies from DashScope mocks — src/video-generation/dashscope-compatible.ts:275
    The adjacent provider tests still mock DashScope submit and poll results as objects with only json(). After this line the code calls readResponseWithLimit, which reads response.body or falls back to response.arrayBuffer(), so the existing Qwen/Alibaba happy-path tests will throw before they reach polling. Update the shared DashScope video test helper and direct Qwen mock to return new Response(JSON.stringify(...)) or an equivalent body-backed response.
    Confidence: 0.92

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against fd66b44f5e2c.

Label changes

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🐚 platinum hermit and patch quality is 🦪 silver shellfish.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The updated PR body includes copied Linux/Node 24 terminal output from an after-fix streaming-body harness showing the 16 MiB cap and overflow error; the live DashScope endpoint was not tested.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.
  • remove merge-risk: 🚨 compatibility: Current PR review selected no merge-risk labels.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.

Label justifications:

  • P2: This is a focused provider-response memory hardening fix with limited blast radius, but it currently needs a small test-contract repair before merge.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🐚 platinum hermit and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The updated PR body includes copied Linux/Node 24 terminal output from an after-fix streaming-body harness showing the 16 MiB cap and overflow error; the live DashScope endpoint was not tested.
  • proof: sufficient: Contributor real behavior proof is sufficient. The updated PR body includes copied Linux/Node 24 terminal output from an after-fix streaming-body harness showing the 16 MiB cap and overflow error; the live DashScope endpoint was not tested.
Evidence reviewed

PR surface:

Source +17. Total +17 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 19 2 +17
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 19 2 +17

Acceptance criteria:

  • [P1] node scripts/run-vitest.mjs extensions/qwen/video-generation-provider.test.ts extensions/alibaba/video-generation-provider.test.ts.
  • [P1] node scripts/run-vitest.mjs packages/media-core/src/read-response-with-limit.test.ts.

What I checked:

Likely related people:

  • Peter Steinberger: Introduced and refactored the shared DashScope video helper and related Qwen/Alibaba video provider paths in April 2026. (role: feature-history owner; confidence: high; commits: 5656f6c7ffb7, 95e397a26661, a88c6f0fe713; files: src/video-generation/dashscope-compatible.ts, extensions/qwen/video-generation-provider.ts, extensions/alibaba/video-generation-provider.ts)
  • Josh Lehman: Recently carried the current DashScope helper and shared test helper into the current main history through the merged lifecycle migration PR. (role: recent area contributor; confidence: medium; commits: 8a7b3c755a42; files: src/video-generation/dashscope-compatible.ts, src/plugin-sdk/test-helpers/dashscope-video-provider.ts, extensions/qwen/video-generation-provider.test.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. proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 24, 2026
@cxbAsDev

cxbAsDev commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

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.

@cxbAsDev @claude
fix(qwen,alibaba): add arrayBuffer to DashScope video mock responses
ea93141 
Add arrayBuffer method to mock response objects in the DashScope video
shared test helper and qwen extension test so readResponseWithLimit can
fall back to arrayBuffer() when the mock Response has no body stream.

Refs openclaw#96248

Co-Authored-By: Claude <[email protected]>
@clawsweeper clawsweeper Bot added 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. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 24, 2026
This was referenced Jun 25, 2026
Merged
Closed
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed 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. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 27, 2026
@vincentkoc

vincentkoc commented Jun 29, 2026

Copy link
Copy Markdown
Member

Closing this as superseded by #96782, which landed the same DashScope response-size cap through the shared provider JSON helper with green CI.

@vincentkoc vincentkoc closed this Jun 29, 2026
@cxbAsDev
cxbAsDev deleted the fix/bound-dashscope-video-response branch July 15, 2026 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cxbAsDev @vincentkoc