Skip to content

fix(qwen): bound video description success response reads#96604

Merged
sallyom merged 1 commit into
openclaw:mainfrom
Alix-007:fix/bound-qwen-video
Jun 25, 2026
Merged

fix(qwen): bound video description success response reads#96604
sallyom merged 1 commit into
openclaw:mainfrom
Alix-007:fix/bound-qwen-video

Conversation

@Alix-007

@Alix-007 Alix-007 commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

On the success path, describeQwenVideo
(extensions/qwen/media-understanding-provider.ts) read the video-description
response with an unbounded await res.json(). res.json() buffers the entire
body into memory before parsing — no byte ceiling, with or without a
Content-Length header. The Qwen OpenAI-compatible endpoint is untrusted external
input: a hostile, misconfigured, or SSRF-reachable baseUrl can stream an
arbitrarily large or never-ending JSON body and force the runtime to buffer it
all, causing memory pressure or a hang on the media-understanding path. This is
the success-JSON-side companion to the #95103 / #95108 response-limit campaign
(and to the already-merged #95218 / #96027 / #96038 bounded-reader PRs).

Changes

  • Read the success body through the shared byte-bounded reader
    readProviderJsonResponse (16 MiB cap, re-exported via
    openclaw/plugin-sdk/provider-http), then JSON.parse the decoded prefix —
    the exact pattern merged in #96027 / #96038. Plugin-scoped, no new
    abstraction.
  • On overflow the reader cancels the underlying stream and throws a bounded error
    (Qwen video description failed: JSON response exceeds 16777216 bytes).
  • Malformed JSON is normalized to a stable
    Qwen video description failed: malformed JSON response (previously a raw
    SyntaxError); the OpenAI-compatible payload extraction is unchanged.

Real behavior proof

  • Behavior addressed: An untrusted Qwen video-description success response
    with no Content-Length and an oversized / never-ending body must not be
    buffered whole; the read must stop at the 16 MiB cap, cancel the stream, and
    throw a bounded error — while valid small responses still parse unchanged.
  • Real environment tested: A real node:http server (createServer) bound
    to 127.0.0.1, streaming a JSON body in 1 MiB chunks with no
    Content-Length     header (would total ~64 MiB, 4× the cap), driving the real
    exported     describeQwenVideo over a real loopback socket — through the
    real postJsonRequest / SSRF-guarded undici dispatcher and the real
    readProviderJsonResponse reader. Loopback is allowed via the provider's own
    request: { allowPrivateNetwork: true } override (the same mechanism Ollama
    uses via buildOllamaBaseUrlSsrFPolicy). Node v22.22.0. No mock fetch, no
    in-process ReadableStream fixture — the bytes cross a real socket.
  • Exact steps: node --import tsx __qwen_bound_proof.mts — start the
    streaming server → drive the real describeQwenVideo against it and assert it
    rejects with the bounded error + the server observed an early socket abort
    (stream cancelled) with bytes-on-wire near the cap → run a negative control
    (old unbounded fetch().arrayBuffer() of the same endpoint) → drive the real
    function against a second server returning a small valid payload.
  • Evidence after fix:
    • Bounded case: threw Qwen video description failed: JSON response exceeds 16777216 bytes; the server saw the socket aborted after 18,874,369 bytes
      (≈18 MiB, ≪ the ~64 MiB it would have sent), confirming the stream was
      cancelled, not drained.
    • Negative control: an unbounded read of the same endpoint pulled the full
      67,108,866 bytes       (>3.5× the bounded read), proving the cap is load-bearing
      (without it the body keeps accumulating past 16 MiB).
    • Small case: parsed into the expected text intact, no truncation.
  • What was not tested: I did not hit a live DashScope / Qwen endpoint (no key,
    would not reproduce a hostile oversized body); the proof reproduces the exact
    untrusted-body transport shape with a local streaming server over a real
    socket. The proof drives the bytes-on-wire + early-socket-close signal (the
    load-bearing signal) rather than driving the runtime to actual OOM. The proof
    script is not committed.

Evidence

Real node:http terminal proof (real loopback socket, real exported
describeQwenVideo, real SSRF dispatcher + readProviderJsonResponse):

[proof] huge server http://127.0.0.1:44351, small server http://127.0.0.1:41785, cap=16777216 bytes, would-stream≈67108864 bytes

PASS  describeQwenVideo throws bounded error on unbounded streamed JSON body :: threw=true msg="Qwen video description failed: JSON response exceeds 16777216 bytes"
PASS  stream cancelled: server saw socket abort before sending the full ~64MiB :: aborted=true bytesSent=18874369 (<67108864)
PASS  stream cancelled near the 16 MiB cap (bytesSent ≈ cap, ≪ 64 MiB) :: bytesSent=18874369 (<33554432)
PASS  negative control: unbounded read buffers the FULL body PAST the 16 MiB cap :: buffered=67108866 bytes (>16777216) — cap is load-bearing
PASS  negative control consumed far more than the bounded read :: unbounded buffered=67108866 vs bounded sent≈18874369
PASS  happy path: small valid /chat/completions JSON still parsed :: text="a calm beach at sunset" model="qwen-vl-max-latest"

[proof] ALL PASS

In-repo Vitest suite (bounded-overflow streaming regression + malformed-JSON +
the unchanged happy-path payload test) passes 3/3:

 Test Files  1 passed (1)
      Tests  3 passed (3)

Checks on the changed files:

  • node scripts/run-vitest.mjs extensions/qwen/media-understanding-provider.test.ts --run → 3/3 passed
  • tsgo (extensions test project) → clean on extensions/qwen. This revision
    also fixes the two new describeQwenVideo test calls that were missing the
    required fileName / timeoutMs fields of VideoDescriptionRequest
    (error TS2739), which would otherwise fail check-test-types.
  • oxlint extensions/qwen/media-understanding-provider.ts extensions/qwen/media-understanding-provider.test.ts → exit 0, clean

Label: security

AI-assisted.

@clawsweeper

clawsweeper Bot commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 24, 2026, 11:20 PM ET / 03:20 UTC.

Summary
The PR changes Qwen video-description success JSON parsing to use readProviderJsonResponse and adds oversized-body cancellation plus malformed-JSON regression coverage.

PR surface: Source +8, Tests +71. Total +79 across 2 files.

Reproducibility: yes. source inspection gives a high-confidence path: current main and v2026.6.10 call res.json() on the Qwen video-description success body, while the PR body provides after-fix loopback streaming-server proof for bounded cancellation and small-response parsing.

Review metrics: 1 noteworthy metric.

  • Provider success JSON cap: 1 success path changed to a 16 MiB cap. The Qwen video-description success response changes from unbounded parsing to a hard rejection boundary, which maintainers should notice before merge.

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] Maintainers should decide whether the shared 16 MiB cap is acceptable for Qwen video-description success payloads before merge.

Risk before merge

  • [P1] Existing Qwen users with a custom or unusually large valid video-description success JSON body over 16 MiB will now receive Qwen video description failed: JSON response exceeds 16777216 bytes; maintainers should intentionally accept the shared cap or request a Qwen-specific limit before merge.

Maintainer options:

  1. Accept the shared provider JSON cap (recommended)
    Treat Qwen video-description success JSON over 16 MiB as invalid provider behavior and land after normal maintainer review and checks are green.
  2. Use a Qwen-specific cap first
    If maintainers believe legitimate Qwen video-description payloads can exceed 16 MiB, set a provider-specific limit and cover that policy in the same regression tests before merge.
  3. Pause for provider evidence
    If the cap policy is uncertain, pause until maintainers can confirm expected Qwen success payload sizes or choose a different limit.

Next step before merge

  • No automated repair is needed; the remaining action is maintainer compatibility review for the intentional 16 MiB Qwen response cap and normal merge gating.

Security
Cleared: The diff narrows an unbounded external provider-response memory exposure and does not add dependencies, workflow changes, package metadata changes, permissions, secrets handling, or artifact execution paths.

Review details

Best possible solution:

Land the plugin-local bounded-reader change if maintainers accept the shared 16 MiB provider JSON boundary; otherwise adjust the Qwen limit with focused tests before merge.

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

Yes, source inspection gives a high-confidence path: current main and v2026.6.10 call res.json() on the Qwen video-description success body, while the PR body provides after-fix loopback streaming-server proof for bounded cancellation and small-response parsing.

Is this the best way to solve the issue?

Yes, this is the best code shape if maintainers accept the cap policy: reusing readProviderJsonResponse is narrower than a custom Qwen reader and keeps byte limits, cancellation, and malformed-JSON wrapping in the shared provider HTTP helper.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority provider hardening PR with limited blast radius and no active outage evidence.
  • merge-risk: 🚨 compatibility: The PR changes Qwen video-description success JSON parsing from unbounded parsing to rejecting responses over the shared 16 MiB provider cap.
  • 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 (terminal): The PR body includes terminal proof from a real loopback HTTP server driving the exported Qwen function over a socket, showing early abort near the cap, a negative control, and a small valid response check.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes terminal proof from a real loopback HTTP server driving the exported Qwen function over a socket, showing early abort near the cap, a negative control, and a small valid response check.
Evidence reviewed

PR surface:

Source +8, Tests +71. Total +79 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 9 1 +8
Tests 1 71 0 +71
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 80 1 +79

What I checked:

Likely related people:

  • steipete: GitHub path history shows steipete introduced extensions/qwen/media-understanding-provider.ts and later refactored the same Qwen media helper area. (role: original Qwen provider feature introducer; confidence: high; commits: e3ac0f43df3e, a171de283f6d, bfa48c4025a2; files: extensions/qwen/media-understanding-provider.ts, extensions/qwen/index.ts)
  • Alix-007: Beyond this PR, Alix-007 authored merged shared-provider and sibling bounded-reader PRs that this patch follows. (role: recent response-bound campaign contributor; confidence: high; commits: 2592f8a51a4e, d1c2934d0d17, 605aede38c10; files: src/agents/provider-http-errors.ts, extensions/ollama/src/provider-models.ts, extensions/exa/src/exa-web-search-provider.runtime.ts)
  • joshavant: Recent history shows joshavant changed successful provider response reads and exported related provider HTTP SDK surface used by this PR. (role: shared provider helper contributor; confidence: medium; commits: 0a14444924e3; files: src/agents/provider-http-errors.ts, src/plugin-sdk/provider-http.ts)
  • vincentkoc: Provider HTTP history shows earlier safe JSON, binary cap, and error-body reader work in the same helper module family. (role: provider HTTP hardening contributor; confidence: medium; commits: d16f79f49d24, 79691d485847, e802fb8a9fbb; files: src/agents/provider-http-errors.ts, src/plugin-sdk/provider-http.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: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 25, 2026
@Alix-007

Alix-007 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper review

@clawsweeper

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

@Alix-007
Alix-007 force-pushed the fix/bound-qwen-video branch from bd4fd98 to 81da8da Compare June 25, 2026 02:46
@Alix-007

Alix-007 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper review

@clawsweeper

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

@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: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 25, 2026
@sallyom

sallyom commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Maintainer local review: merge-ready provided green CI, reviewed as a batch:

Autoreview is clean, the 16 MiB cap is acceptable for these provider JSON/status/error/usage bodies, and the changes follow the established shared bounded-reader pattern. This is the best narrow fix for these call sites: no new SDK/API/config surface, no upgrade/backward-compatibility concern beyond the intended oversized-body failure mode, and no expected breaking behavior for normal provider responses.

@sallyom
sallyom merged commit cc124d2 into openclaw:main Jun 25, 2026
117 of 123 checks passed
This was referenced Jun 25, 2026
Merged
Merged
Merged
Merged
@github-actions github-actions Bot mentioned this pull request Jun 25, 2026
Open
@clawsweeper clawsweeper Bot mentioned this pull request Jun 25, 2026
Merged
@mushuiyu886 mushuiyu886 mentioned this pull request Jun 26, 2026
Closed
This was referenced Jun 26, 2026
Merged
Merged
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 26, 2026
@Alix-007
fix(qwen): bound video success response (openclaw#96604)
577534e
@clawsweeper clawsweeper Bot mentioned this pull request Jun 28, 2026
Merged
QiuYuang pushed a commit to QiuYuang/openclaw that referenced this pull request Jul 1, 2026
@Alix-007 @QiuYuang
fix(qwen): bound video success response (openclaw#96604)
ec5b935
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

merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Alix-007 @sallyom