Skip to content

fix(openai): bound JSON/text response reads to prevent OOM#96323

Closed
lsr911 wants to merge 3 commits into
openclaw:mainfrom
lsr911:fix/openai-bounded-json-response
Closed

fix(openai): bound JSON/text response reads to prevent OOM#96323
lsr911 wants to merge 3 commits into
openclaw:mainfrom
lsr911:fix/openai-bounded-json-response

Conversation

@lsr911

@lsr911 lsr911 commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Bound OpenAI provider HTTP response reads using shared provider helpers
to prevent unbuffered JSON from untrusted upstream responses.

Changes

File Change
extensions/openai/embedding-batch.ts readProviderJsonResponse<OpenAiBatchStatus> (shared 16 MiB default)
extensions/openai/openai-chatgpt-oauth-flow.runtime.ts readResponseWithLimit (16 MiB, matching shared provider cap) in postTokenForm
extensions/openai/realtime-provider-shared.ts readProviderJsonResponse (shared 16 MiB default)

All caps now align with the shared 16 MiB provider JSON default — no custom undersized caps.

Evidence

Real HTTP proof: bounded reader cancels oversized stream

node --import tsx test/_proof_bounded_json.mts
PASS  oversized body: throws bounded cap error
PASS  stream cancelled at cap: bytesSent=1048576
PASS  happy path: small JSON parsed correctly
PASS  negative control: unbounded read buffers PAST cap
[proof] 4 PASS, 0 FAIL

Changed-path: Batch status tests

pnpm exec vitest run extensions/openai/embedding-batch.test.ts
 Tests  4 passed (4)

Changed-path: OAuth flow tests

pnpm exec vitest run extensions/openai/openai-chatgpt-oauth-flow.runtime.test.ts

AI-assisted.

@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 27, 2026, 1:53 AM ET / 05:53 UTC.

Summary
The PR changes OpenAI Batch status, ChatGPT OAuth token, and Realtime client-secret success-response reads from raw response buffering/parsing to bounded shared response helpers.

PR surface: Source +10. Total +10 across 3 files.

Reproducibility: Source-level yes, live changed-path no. Current main reaches raw response.json() and arrayBuffer() in the targeted OpenAI paths, and the bounded reader contract is clear from source, but this review did not establish a live provider repro.

Review metrics: 1 noteworthy metric.

  • Provider success-body caps: 2 shared JSON caps added, 1 OAuth raw-body cap added. These are new success-path rejection points where current main accepts unbounded bodies, so proof and cap policy matter before merge.

Stored data model
Persistent data-model change detected: vector/embedding metadata: extensions/openai/embedding-batch.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until stronger real behavior proof is added.

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

Rank-up moves:

  • [P1] Add redacted terminal/log/live output for normal and oversized Batch status, OAuth exchange, OAuth refresh, and Realtime client-secret responses; redact IPs, API keys, phone numbers, non-public endpoints, and other private details.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR body shows terminal bounded-reader proof, but not redacted after-fix real output for the OAuth exchange/refresh and Realtime client-secret changed paths; update the PR body with redacted proof so ClawSweeper can re-review, or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] Merging this changes oversized successful OpenAI Batch, OAuth, and Realtime responses from accepted/unbounded buffering to runtime errors; that mitigates OOM risk but is compatibility- and availability-sensitive for any legitimate large response.
  • [P1] The PR body has generic terminal proof and mocked changed-path tests, but not redacted after-fix real output for normal and oversized OAuth exchange, OAuth refresh, and Realtime client-secret paths.

Maintainer options:

  1. Prove the changed OpenAI paths (recommended)
    Ask the contributor to add redacted terminal/log/live output for normal and oversized Batch status, OAuth exchange, OAuth refresh, and Realtime client-secret responses before merge.
  2. Accept the shared cap deliberately
    Maintainers can accept the 16 MiB shared cap from source-level evidence alone, but they should explicitly own the risk that a legitimate larger success body now fails.
  3. Pause until a narrower owner path exists
    If maintainers do not want external contributor proof to set cap policy for these provider paths, pause this PR and route the hardening through an owner-led provider response sweep.

Next step before merge

  • [P1] Human review remains because the blocker is contributor real behavior proof and maintainer acceptance of compatibility-sensitive success-response caps, not a narrow automated code repair.

Security
Cleared: The diff adds no dependency, workflow, permission, credential, package, or supply-chain surface; the security-relevant availability hardening is tracked as merge risk.

Review details

Best possible solution:

Land after redacted changed-path proof covers normal and oversized Batch status, OAuth exchange/refresh, and Realtime client-secret responses using the shared 16 MiB cap.

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

Source-level yes, live changed-path no. Current main reaches raw response.json() and arrayBuffer() in the targeted OpenAI paths, and the bounded reader contract is clear from source, but this review did not establish a live provider repro.

Is this the best way to solve the issue?

Yes on helper boundary, not yet on merge readiness. Reusing the shared bounded readers with the shared 16 MiB cap is the narrow maintainable fix; the remaining gap is changed-path real behavior proof.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.

Label justifications:

  • P2: This is focused OpenAI provider availability hardening with limited blast radius, but proof and compatibility questions remain before merge.
  • merge-risk: 🚨 compatibility: The PR can reject provider success responses that current main accepts because it adds finite response-size caps to previously unbounded OpenAI paths.
  • merge-risk: 🚨 availability: The change mitigates OOM risk but can also turn large legitimate OpenAI responses into runtime failures if the cap behavior is unproven for real paths.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body shows terminal bounded-reader proof, but not redacted after-fix real output for the OAuth exchange/refresh and Realtime client-secret changed paths; update the PR body with redacted proof so ClawSweeper can re-review, or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +10. Total +10 across 3 files.

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

What I checked:

Likely related people:

  • steipete: Recent history on the touched OpenAI OAuth, Realtime, and embedding-batch files includes OpenAI provider identity, realtime secret expiry, and embedding batch poll work. (role: recent OpenAI provider contributor; confidence: medium; commits: 4c33aaa86c16, dca53afd5301, 33c44626d211; files: extensions/openai/openai-chatgpt-oauth-flow.runtime.ts, extensions/openai/realtime-provider-shared.ts, extensions/openai/embedding-batch.ts)
  • Alix-007: Authored the merged shared provider JSON response cap helper that this PR applies to the OpenAI paths. (role: shared bounded JSON helper contributor; confidence: high; commits: 2592f8a51a4e; files: src/agents/provider-http-errors.ts, src/agents/provider-http-errors.test.ts)
  • joshavant: Recent shared provider HTTP history includes successful provider response read hardening adjacent to this cap-policy decision. (role: recent shared provider response contributor; confidence: medium; commits: 0a14444924e3; files: src/agents/provider-http-errors.ts, src/plugin-sdk/provider-http.ts)
  • vincentkoc: Recent history includes OpenAI embedding batch error-body bounds and provider HTTP reader cleanup near the affected surface. (role: recent OpenAI/provider HTTP hardening contributor; confidence: medium; commits: 5df5aa164052; files: extensions/openai/embedding-batch.ts, src/agents/provider-http-errors.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. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 24, 2026
@openclaw-barnacle openclaw-barnacle Bot added size: M triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed size: XS labels Jun 25, 2026
@lsr911

lsr911 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Closing: multiple CI failures (lint, real-behavior-proof, extension-bundled).

@lsr911 lsr911 closed this Jun 25, 2026
@openclaw-barnacle openclaw-barnacle Bot added triage: blank-template Candidate: PR template appears mostly untouched. 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 26, 2026
@lsr911

lsr911 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper could not start a re-review for this item.

Reason: re-review requires an open issue or PR.

@lsr911 lsr911 reopened this Jun 26, 2026
@lsr911

lsr911 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

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

@lsr911

lsr911 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@lsr911
lsr911 force-pushed the fix/openai-bounded-json-response branch from a37a51e to 5e72528 Compare June 26, 2026 12:09
@lsr911

lsr911 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

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

@lsr911

lsr911 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@lsr911
lsr911 force-pushed the fix/openai-bounded-json-response branch from 5e72528 to d28af0c Compare June 27, 2026 00:34
@lsr911

lsr911 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

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

@lsr911

lsr911 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

1 similar comment
@lsr911

lsr911 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@lsr911 @claude
fix(openai): bound JSON response reads via shared provider helpers
d2b2cff 
- embedding-batch: use readProviderJsonResponse (shared 16 MiB default cap)
  for batch status JSON; file content left unbounded per review
- oauth-flow: cap token endpoint response body at first read via
  readResponseWithLimit (must use raw reader to re-wrap in Response)
- realtime-provider: use readProviderJsonResponse (shared default cap)
  for client secret JSON

Co-Authored-By: Claude <[email protected]>
Signed-off-by: lsr911 <[email protected]>
@lsr911
lsr911 force-pushed the fix/openai-bounded-json-response branch from d28af0c to d2b2cff Compare June 27, 2026 00:57
@lsr911

lsr911 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

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

@lsr911

lsr911 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

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

@lsr911

lsr911 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 27, 2026
@lsr911

lsr911 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

1 similar comment
@lsr911

lsr911 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@lsr911

lsr911 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

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

@lsr911

lsr911 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jun 27, 2026
This was referenced Jun 28, 2026
Merged
Merged
Closed
Closed
@lsr911

lsr911 commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Closed: merge conflicts with upstream. Superseded by cleaner single-module PRs covering the same modules.

@lsr911 lsr911 closed this Jun 30, 2026
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

extensions: openai merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: blank-template Candidate: PR template appears mostly untouched.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lsr911