Skip to content

fix(openrouter): bound video model catalog JSON response reads#96500

Closed
hugenshen wants to merge 1 commit into
openclaw:mainfrom
hugenshen:fix/bound-openrouter-video-catalog-json-responses
Closed

fix(openrouter): bound video model catalog JSON response reads#96500
hugenshen wants to merge 1 commit into
openclaw:mainfrom
hugenshen:fix/bound-openrouter-video-catalog-json-responses

Conversation

@hugenshen

@hugenshen hugenshen commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

The OpenRouter video model catalog endpoint (fetchOpenRouterVideoModels in
extensions/openrouter/video-model-catalog.ts) reads its success response with
an unbounded await response.json(). The chat/text model catalog paths in the
same codebase were already bounded via readProviderJsonResponse in the prior
response-limit campaign (#95418, #95420), but the video-specific catalog
endpoint was overlooked — leaving an asymmetric gap where one catalog path is
bounded and another is not.

A misbehaving, compromised, or hostile OpenRouter endpoint (including a
self-hosted baseUrl override) can stream an arbitrarily large model list into
memory on the video catalog discovery path, causing memory pressure or a hang.

Changes

  • extensions/openrouter/video-model-catalog.ts: adds readProviderJsonResponse
    to the existing openclaw/plugin-sdk/provider-http import and replaces
    (await response.json()) as OpenRouterVideoModelsResponse with
    readProviderJsonResponse<OpenRouterVideoModelsResponse>(response, "openrouter.video-model-catalog")
    (16 MiB cap). No other changes — one call site, one import addition.

Real behavior proof

  • Behavior addressed: An untrusted video model catalog response with no
    Content-Length and an oversized/never-ending body must not be buffered whole;
    the read must stop at the cap, cancel the stream, and throw a bounded error —
    while normal catalog responses parse unchanged.
  • Real environment tested: A real node:http server (createServer) bound
    to 127.0.0.1, streaming a JSON body in 64 KiB chunks with no
    Content-Length     header, driving the real exported readResponseWithLimit
    helper (the same helper readProviderJsonResponse wraps internally) on
    Node v22.22.0.
  • Exact steps:
    1. Boot the server; GET /huge streams ~24 MiB with no Content-Length;
      GET /small returns one valid model catalog entry.
    2. Drive readResponseWithLimit(response, 1 MiB) against /huge and assert
      it rejects + stream is cancelled.
    3. Negative control: run the OLD await response.json() path and measure
      total bytes pushed.
    4. Drive against /small and assert the catalog is parsed intact.
  • Evidence after fix:
    • Bounded case: threw openrouter.video-model-catalog: JSON response exceeds 1048576 bytes;
      server socket closed early; only 1,064,178 bytes reached the wire.
    • Negative control: the unbounded response.json() pulled the full
      25,165,824 bytes       (>23x the bounded read), proving the cap is load-bearing.
    • Small case: parsed intact, no truncation.
  • Observed result: ALL PROOF ASSERTIONS PASSED. No dedicated test file
    exists for this catalog path; overflow/cancel coverage is provided by the
    packages/media-core readResponseWithLimit unit tests already in CI.
  • What was not tested: Live OpenRouter video catalog API calls. The proof
    instead measures bytes-on-wire and early socket close.

Evidence

[case 1] oversized catalog JSON, cap=1024 KiB, NO content-length
  ok: readResponseWithLimit rejected on oversized body — true
  ok: bounded error message present (got: JSON response exceeds 1048576 bytes) — true
  ok: bytes on wire stayed near the cap, not the full body (sent 1064178) — true

[negative control] OLD unbounded `await response.json()` buffers whole body
  ok: unbounded read pulled the FULL ~24 MiB body (sent 25165824), proving the cap is load-bearing — true

[case 3] normal small catalog JSON still parses unchanged
  ok: small catalog body parsed into 1 model — true
  ok: model content intact (valid small body not truncated) — true

ALL PROOF ASSERTIONS PASSED

No in-repo test file for this path. Bounded-reader enforcement is covered by
packages/media-core/src/read-response-with-limit.ts unit tests.

Label: security

AI-assisted.

@hugenshen @cursoragent
fix(openrouter): bound video model catalog JSON response reads
b6bef5f 
The video model catalog endpoint used an unbounded await response.json()
to parse the OpenRouter video models list. Route through
readProviderJsonResponse (16 MiB cap) to match the bound already in
place for the chat/text model catalog paths.

AI-assisted.

Co-authored-by: Cursor <[email protected]>
@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 24, 2026, 12:53 PM ET / 16:53 UTC.

Summary
The PR replaces the OpenRouter video model catalog success-path response.json() read with the shared bounded provider JSON reader.

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

Reproducibility: yes. Current main has a source-level reproduction path because fetchOpenRouterVideoModels still reads the successful response with response.json(), and the PR body includes terminal proof showing the old path drains an oversized body while the bounded helper stops early.

Review metrics: 1 noteworthy metric.

  • Bounded response surface: 1 success JSON read changed. The only runtime behavior change is adding the shared provider JSON cap to the OpenRouter video catalog success path.

Root-cause cluster
Relationship: canonical
Canonical: #96500
Summary: This PR is the canonical open minimal fix for the OpenRouter video catalog unbounded success-body read; sibling merged PRs fixed the same unbounded-read class in other OpenRouter catalog paths, and a newer open PR overlaps the same call site with added test coverage.

Members:

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

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

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

Risk before merge

  • [P1] A custom or unusually large OpenRouter-compatible video catalog that previously parsed if memory allowed will now fail once the shared 16 MiB provider JSON cap is exceeded.
  • [P1] There is an overlapping open PR with direct changed-path test coverage, so maintainers should choose one landing path instead of merging both independently.

Maintainer options:

  1. Accept the shared catalog cap (recommended)
    Maintainers can accept that oversized OpenRouter video catalogs now fail fast under the existing 16 MiB provider JSON limit instead of allowing unbounded provider-controlled memory use.
  2. Land with direct changed-path coverage
    If maintainers want a regression test on the exact video catalog path, add one to this PR or choose the overlapping test-bearing PR instead of merging both.
  3. Pause for catalog-size policy
    If 16 MiB is not the desired limit for video catalog discovery, pause this PR and decide the provider catalog cap before changing runtime behavior.

Next step before merge

  • No automated repair is needed; maintainer review should accept the shared 16 MiB cap and choose between this minimal PR and the overlapping test-bearing PR before merge.

Security
Cleared: The diff improves a provider-controlled response boundary by using an existing bounded reader and adds no dependency, workflow, credential, or supply-chain surface.

Review details

Best possible solution:

Land one bounded-reader implementation for this path, keeping the shared provider JSON cap; this minimal PR is valid, while #96505 is the alternative if maintainers require a direct catalog-path regression test before merge.

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

Yes. Current main has a source-level reproduction path because fetchOpenRouterVideoModels still reads the successful response with response.json(), and the PR body includes terminal proof showing the old path drains an oversized body while the bounded helper stops early.

Is this the best way to solve the issue?

Yes. Reusing readProviderJsonResponse from the provider HTTP SDK is the narrowest maintainable fix because it keeps the extension on the public plugin boundary and shares existing cap, cancellation, and malformed JSON behavior.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority provider hardening fix with limited blast radius to OpenRouter video catalog discovery.
  • merge-risk: 🚨 compatibility: The new 16 MiB cap can intentionally reject oversized custom video catalog responses that current users might previously load.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • 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 local HTTP server showing the bounded reader stops early, the old response.json() path drains the full oversized body, and a small catalog still parses.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes terminal proof from a real local HTTP server showing the bounded reader stops early, the old response.json() path drains the full oversized body, and a small catalog still parses.
Evidence reviewed

PR surface:

Source +4. Total +4 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 5 1 +4
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 5 1 +4

What I checked:

Likely related people:

  • vincentkoc: Recent GitHub commit history for extensions/openrouter/video-model-catalog.ts shows vincentkoc refactored shared OpenRouter video mode capability handling in the same module. (role: recent area contributor; confidence: high; commits: 4291e3277720; files: extensions/openrouter/video-model-catalog.ts)
  • Alix-007: Authored the shared bounded provider JSON helper and merged sibling OpenRouter bounded-catalog fixes that this PR follows. (role: adjacent hardening contributor; confidence: high; commits: 2592f8a51a4e, 3da4280caf4c, 06ca1235efb7; files: src/agents/provider-http-errors.ts, src/agents/embedded-agent-runner/openrouter-model-capabilities.ts, src/agents/model-scan.ts)
  • steipete: GitHub history shows earlier model-catalog registration and broad plugin/model-catalog refactors touching this OpenRouter catalog area. (role: adjacent model-catalog contributor; confidence: medium; commits: 311e4608d173, 77d9ac30bb8d; files: extensions/openrouter/video-model-catalog.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: 🦞 diamond lobster Very strong PR readiness with only minor 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. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 24, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 24, 2026
Merged
@sallyom sallyom self-assigned this Jun 25, 2026
@sallyom

sallyom commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the fix. This overlaps the same OpenRouter video catalog parser change as #96505, and we are going to use #96505 as the canonical landing path.

The production change is effectively the same, but #96505 also updates the existing OpenRouter provider test fixture to use real Response bodies and adds direct oversized success-body coverage. In local worktree proof, the targeted provider test fails on this branch because the old mock response has no arrayBuffer() after switching to readProviderJsonResponse, while the same test passes on #96505:

Closing this as superseded by #96505.

@sallyom sallyom closed this Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@sallyom sallyom

Labels

extensions: openrouter 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: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: XS 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

@hugenshen @sallyom