Skip to content

fix(openai): bound video create-submit response reads#96905

Merged
vincentkoc merged 5 commits into
openclaw:mainfrom
Alix-007:fix/bound-openai-video
Jun 29, 2026
Merged

fix(openai): bound video create-submit response reads#96905
vincentkoc merged 5 commits into
openclaw:mainfrom
Alix-007:fix/bound-openai-video

Conversation

@Alix-007

Copy link
Copy Markdown
Contributor

What Problem This Solves

The OpenAI video create-submit success path read the control JSON with an unbounded await response.json(), so a hostile, misconfigured, or SSRF-reachable endpoint could stream a no-Content-Length body indefinitely and force OpenClaw to buffer the whole payload before parsing, causing OOM pressure or a hang on the video generation path.

The poll path already goes through the bounded pollProviderOperationJson helper and the download path already uses readResponseWithLimit, so the create-submit JSON read at extensions/openai/video-generation-provider.ts was the one remaining unbounded success-path read. This is a defense-in-depth fix and a sibling-consistency cleanup: in practice the OpenAI submit response is a small control JSON ({ id, model, status, ... }), but the read was not bounded the way the rest of this provider and the already-merged byteplus/google/qwen video bounds are.

Changes

  • Replace the create-submit (await response.json()) as OpenAIVideoResponse with the shared readProviderJsonResponse helper from openclaw/plugin-sdk/provider-http, reusing the existing 16 MiB provider JSON cap; no new abstraction.
  • Preserve the existing assertOkOrThrowHttpError behavior, the submitted.id / status extraction, the poll and download paths, and the release() cleanup.
  • Keep the shared provider-http test mock's readProviderJsonResponse reader real (via importActual) so the streaming size cap is exercised under test instead of stubbed, and update the focused OpenAI video tests to stream their submit/poll JSON through real Response bodies.

Real behavior proof

  • Behavior addressed: An untrusted OpenAI video create-submit JSON body must not be buffered whole on the success path. An oversized no-Content-Length body must stop at the 16 MiB cap, cancel the stream, and throw a bounded ...exceeds 16777216 bytes error. Small valid JSON must continue to parse and the full poll + download path must keep working.
  • Real environment tested: A real node:http server (createServer) bound to 127.0.0.1, streaming JSON in 64 KiB chunks with no Content-Length header and a would-stream size of about 64 MiB. The script drove the real exported buildOpenAIVideoGenerationProvider().generateVideo() over a real loopback socket through the real postJsonRequest, guarded fetch path, and readProviderJsonResponse, using the provider's request-level allowPrivateNetwork opt-in for the loopback target. Node v22.22.0.
  • Exact steps: node --import tsx <scratchpad>/proof.mts — start the streaming server → call the real provider create-submit against the streaming route and assert a bounded throw plus a server-side socket abort near the cap → run a negative control using the old unbounded fetch().arrayBuffer() against the same body → call the real provider against a small valid submit JSON and drive the full poll + download path.
  • Observed result: the create-submit read threw OpenAI video generation failed: JSON response exceeds 16777216 bytes and the server aborted after 17,825,799 bytes (just past the cap, far below the ~64 MiB it would have streamed). The negative control buffered the full 67,108,871 bytes, proving the cap is load-bearing. The small happy path parsed the submit JSON and returned the generated video through the real poll + download path.
  • What was not tested: I did not hit the live OpenAI Sora endpoint; a live endpoint would not reproduce a hostile oversized body. This OpenAI submit response is normally a small control JSON, so this is a defense-in-depth bound rather than a fix for an observed live OOM. The real socket proof covers the transport, the cap, and the cancellation behavior; the provider-wiring is also covered by the focused in-repo Vitest regressions using the same shared reader and error text. The proof script is not committed.

Evidence

Real node:http terminal proof (real loopback socket, real exported OpenAI provider, real bounded reader):

[proof] real node:http server on http://127.0.0.1:42387/v1, cap=16777216 bytes, would-stream≈67108864 bytes on overflow route

PASS  create-submit JSON: overflow throws bounded error :: threw=true msg="OpenAI video generation failed: JSON response exceeds 16777216 bytes"
PASS  create-submit JSON: stream cancelled near 16MiB :: aborted=true bytesSent=17825799 (<33554432)
PASS  negative control: OLD unbounded read buffers PAST the 16MiB cap :: buffered=67108871 bytes (> 16777216)
PASS  negative control consumed far more than the bounded read :: unbounded buffered=67108871 vs bounded submit sent≈17825799
PASS  happy path: small submit JSON parses and full poll+download works :: err=undefined bytes=15 status=completed

[proof] 5 PASS / 0 FAIL
[proof] ALL PASS

Focused Vitest:

node scripts/test-projects.mjs extensions/openai/video-generation-provider.test.ts

Test Files  1 passed (1)
Tests  14 passed (14)

Type checks:

node scripts/run-tsgo.mjs -p tsconfig.extensions.json ...                       # extensions source: 0 errors
node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.extensions.test.json ...    # extensions tests: 0 errors
node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json ...          # core tests (shared mock): 0 errors

Lint and format:

oxlint extensions/openai/video-generation-provider.ts extensions/openai/video-generation-provider.test.ts src/plugin-sdk/test-helpers/provider-http-mocks.ts   # 0 problems
oxfmt --write ...   # formatted
git diff --check     # clean

Note: the repo autoreview step could not run because its review LLM gateway returned 503 Service Unavailable during this session; the checks above were run locally instead.

Label: security

AI-assisted.

Replace the unbounded `await response.json()` on the OpenAI video
create-submit success path with the shared byte-bounded
`readProviderJsonResponse` (16 MiB cap), matching the already-merged
byteplus/google/qwen video bounds. The poll path already uses
`pollProviderOperationJson` and the download path already uses
`readResponseWithLimit`; this closes the remaining unbounded success-path
read so a hostile or buggy endpoint cannot stream an unbounded body and
force OpenClaw to buffer it before parsing.

Keep the shared provider-http test mock's JSON reader real so the
streaming size cap is exercised under test.
@clawsweeper

clawsweeper Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 28, 2026, 10:46 AM ET / 14:46 UTC.

Summary
The branch replaces the OpenAI video create-submit success-path JSON parse with the shared bounded provider JSON reader and updates OpenAI video tests to use Response-backed JSON fixtures.

PR surface: Source +4, Tests -10. Total -6 across 2 files.

Reproducibility: yes. at source/proof level: current main reaches raw response.json() on the successful OpenAI video submit response, and the PR body includes loopback terminal proof showing bounded versus unbounded behavior. I did not rerun that proof in this read-only review.

Review metrics: 1 noteworthy metric.

  • Successful submit JSON reads: 1 changed from unbounded to 16 MiB capped. This is the compatibility-sensitive behavior maintainers must notice because oversized successful submit metadata now fails early.

Root-cause cluster
Relationship: canonical
Canonical: #96905
Summary: This PR is the canonical open OpenAI video submit bounded-read candidate; older same-surface PRs were closed unmerged in favor of this branch, while broader provider-response PRs cover adjacent paths.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

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:

  • none.

Risk before merge

  • [P1] Merging intentionally changes successful OpenAI video submit JSON bodies over 16 MiB from potentially buffered success to a provider error; maintainers need to accept that cap for custom relays and unexpected provider metadata.
  • [P1] Exact-head CI still had three in-progress checks when inspected, so landing should wait for the normal required checks to finish.

Maintainer options:

  1. Accept the shared cap (recommended)
    Land this branch as the canonical same-surface fix if maintainers accept that successful OpenAI video submit metadata over 16 MiB should fail early.
  2. Tune the cap before merge
    If OpenAI-compatible relays may legitimately return larger submit metadata, adjust the cap or add a narrow documented exception before landing.
  3. Pause for a coordinated sweep
    If maintainers want one owner-led cap policy across the remaining provider response reads, pause this PR and handle the broader sweep separately.

Next step before merge

  • [P2] Human maintainer acceptance is needed for the compatibility-sensitive 16 MiB cap; no automated code repair is identified.

Security
Cleared: The diff narrows an unbounded provider response-read exposure and adds no dependencies, workflows, permissions, secret handling, package metadata, or other supply-chain surface.

Review details

Best possible solution:

Land this branch as the canonical OpenAI video submit bounded-read fix once maintainers accept the shared 16 MiB cap and exact-head required checks finish.

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

Yes at source/proof level: current main reaches raw response.json() on the successful OpenAI video submit response, and the PR body includes loopback terminal proof showing bounded versus unbounded behavior. I did not rerun that proof in this read-only review.

Is this the best way to solve the issue?

Yes, with cap acceptance: reusing the existing shared readProviderJsonResponse helper is the narrow owner-boundary fix, and sibling poll/download paths already use bounded helpers. A local custom cap would duplicate provider HTTP behavior and leave tests easier to drift.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a focused OpenAI video provider hardening fix with limited blast radius and normal maintainer-review urgency.
  • merge-risk: 🚨 compatibility: The PR deliberately changes oversized successful OpenAI video submit JSON responses from accepted buffering to a 16 MiB provider error.
  • 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 OpenAI provider through bounded overflow, an unbounded negative control, and a small happy path.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes terminal proof from a real loopback HTTP server driving the exported OpenAI provider through bounded overflow, an unbounded negative control, and a small happy path.
Evidence reviewed

PR surface:

Source +4, Tests -10. Total -6 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 5 1 +4
Tests 1 53 63 -10
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 58 64 -6

What I checked:

  • Repository policy read: Root AGENTS.md and scoped extensions and Plugin SDK policies were read fully; provider boundary and compatibility guidance apply because this PR changes a bundled OpenAI provider and uses a plugin SDK helper. (AGENTS.md:1, 9241b9701d9c)
  • Current main behavior: Current main still reads the successful OpenAI video submit response with raw response.json() after the HTTP status check, so the central unbounded success-path read remains present before this PR. (extensions/openai/video-generation-provider.ts:427, 9241b9701d9c)
  • PR head behavior: The PR head changes the submit response parse site to readProviderJsonResponse(response, "OpenAI video generation failed"), preserving the surrounding video id extraction and cleanup path. (extensions/openai/video-generation-provider.ts:428, 503810a6a01d)
  • Bounded helper contract: readProviderJsonResponse defaults to the 16 MiB provider JSON cap and delegates to readResponseWithLimit, which cancels the stream and throws on overflow. (src/agents/provider-http-errors.ts:312, 9241b9701d9c)
  • Sibling path evidence: The OpenAI video poll path already uses pollProviderOperationJson, which reads poll responses through readProviderJsonObjectResponse, while the download path uses readResponseWithLimit. (src/media-understanding/shared.ts:178, 9241b9701d9c)
  • Test evidence in PR head: The PR head changes OpenAI video test fixtures to Response.json bodies so the mocked readProviderJsonResponse path sees a standard Response body instead of a stubbed json-only object. (extensions/openai/video-generation-provider.test.ts:107, 503810a6a01d)

Likely related people:

  • steipete: The OpenAI video provider path appears to originate in the video provider support work that added this file and submit flow. (role: feature-history contributor; confidence: medium; commits: 932194b7d58c; files: extensions/openai/video-generation-provider.ts)
  • shakkernerd: Recent history shows OpenAI video request policy, guarded cleanup, retry, and test-helper work on the affected provider path. (role: recent OpenAI video contributor; confidence: high; commits: 31b51455942c, ed7d99aa0edb, efbf9f3d4657; files: extensions/openai/video-generation-provider.ts, src/plugin-sdk/test-helpers/provider-http-mocks.ts)
  • vincentkoc: History shows adjacent provider/media response-size hardening, including generated-video download caps and provider HTTP response cleanup. (role: recent bounded-response contributor; confidence: high; commits: 9e002c12ac7a, 79691d485847, e802fb8a9fbb; files: extensions/openai/video-generation-provider.ts, src/agents/provider-http-errors.ts)
  • Alix-007: Prior merged history added the bounded readProviderJsonResponse helper now being applied here, and the same author has adjacent bounded-response work in this family. (role: shared helper contributor; confidence: high; commits: 2592f8a51a4e; 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 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. P2 Normal backlog priority with limited blast radius. labels Jun 26, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. 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. labels Jun 26, 2026
@Alix-007

Copy link
Copy Markdown
Contributor Author

Addressed the author-side lint finding from the ClawSweeper review: removed the unused streamedRawResponse helper from the OpenAI video test. No production behavior changed.\n\nVerified current head:\n- node scripts/run-vitest.mjs extensions/openai/video-generation-provider.test.ts -- --run\n- git diff --check\n\nThe remaining open point is the canonical-branch choice versus #96786, which needs maintainer direction.\n\n@clawsweeper re-review

@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. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 26, 2026
@Alix-007

Copy link
Copy Markdown
Contributor Author

Merged latest upstream main into this branch to refresh CI after the unrelated compact-shard timeouts. No production behavior changed in this follow-up.

Verified current head locally:

  • node scripts/run-vitest.mjs extensions/openai/video-generation-provider.test.ts -- --run (14 tests passed)
  • git diff --check

The remaining non-code decision is still the canonical-branch choice versus #96786; this branch is up to date from the author side.

@clawsweeper re-review

@clawsweeper

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

@Alix-007

Copy link
Copy Markdown
Contributor Author

Updated this branch against latest upstream main to resolve the post-merge drift in the video bounded-read series. The provider HTTP test helper conflict was resolved by keeping the current main helper, which still exercises bounded streamed JSON behavior.

Verified current head locally:

  • node scripts/run-vitest.mjs extensions/openai/video-generation-provider.test.ts -- --run
  • git diff --check

@clawsweeper re-review

@clawsweeper

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

@vincentkoc vincentkoc self-assigned this Jun 29, 2026
@vincentkoc
vincentkoc merged commit f0e2f7b into openclaw:main Jun 29, 2026
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

extensions: openai 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

Development

Successfully merging this pull request may close these issues.

3 participants