fix(embedding): bound OpenAI-compatible embedding response reads#96868
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 26, 2026, 4:17 AM ET / 08:17 UTC. Summary PR surface: Source -3, Tests +120. Total +117 across 2 files. Reproducibility: yes. Current main clearly uses response.json() on the successful embedding response path, and the PR body provides a loopback node:http proof that the patched path throws at the cap and cancels the socket; I did not rerun it in this read-only review. Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this focused bounded-reader PR after maintainer acceptance of the 16 MiB cap, then close the older overlapping PR as superseded. Do we have a high-confidence way to reproduce the issue? Yes. Current main clearly uses response.json() on the successful embedding response path, and the PR body provides a loopback node:http proof that the patched path throws at the cap and cancels the socket; I did not rerun it in this read-only review. Is this the best way to solve the issue? Yes. Reusing readProviderJsonResponse is the narrowest maintainable fix because it applies the existing provider JSON cap and cancellation helper instead of adding a second custom limiter; an embedding-specific budget is only preferable if maintainers expect valid responses above 16 MiB. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 9a735bea03f6. Label changesLabel justifications:
Evidence reviewedPR surface: Source -3, Tests +120. Total +117 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
Bot
added
proof: sufficient
|
Fixed the actionable Current head verification:
The remaining compact failure I saw is in @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Merge-ready for a469fb4. Local and ClawSweeper review clean, CI green, best narrow fix, follows established pattern, and the 16 MiB cap is acceptable because this OpenAI-compatible embedding path reads inline JSON vectors from /embeddings, not files or media bytes. The response shape is bounded by one data entry per input, OpenClaw's public embeddings bridge already caps request fanout/input size, and typical 3,072-dim float JSON batches stay comfortably below 16 MiB. This is a practical guardrail rather than an upstream hard limit: custom servers with extreme dimensions or very verbose float JSON can be rejected/cancelled instead of letting the runtime buffer an unbounded successful response. |
…nclaw#96868) * fix(embedding): bound OpenAI-compatible embedding response reads * test(embedding): avoid loop-condition lint in stream proof
…nclaw#96868) * fix(embedding): bound OpenAI-compatible embedding response reads * test(embedding): avoid loop-condition lint in stream proof
…nclaw#96868) * fix(embedding): bound OpenAI-compatible embedding response reads * test(embedding): avoid loop-condition lint in stream proof
…nclaw#96868) * fix(embedding): bound OpenAI-compatible embedding response reads * test(embedding): avoid loop-condition lint in stream proof (cherry picked from commit f1e4980)
…nclaw#96868) * fix(embedding): bound OpenAI-compatible embedding response reads * test(embedding): avoid loop-condition lint in stream proof
2 participants
What Problem This Solves
Success-path untrusted body reads in the OpenAI-compatible embedding provider used unbounded
response.json(), so a user-configured OpenAI-compatible embedding endpoint could stream an oversized JSON body and force OpenClaw to buffer until OOM or hang before parsing.Changes
response.json()call with sharedreadProviderJsonResponse, which applies the existing 16 MiB provider JSON cap and cancels the stream on overflow.node:httpregression test for an oversized chunked success JSON response that verifies the provider throws the shared cap error and the server sees the socket close before the full body is sent.Real behavior proof
Behavior addressed: OpenAI-compatible embedding success responses now use the shared bounded JSON reader instead of unbounded
response.json().Real environment tested: local Linux checkout, Node runtime, real loopback
node:httpserver, productioncreateOpenAICompatibleEmbeddingProviderexport, production fetch path, no mocked response object for the proof script.Exact steps:
node scripts/run-vitest.mjs src/plugins/openai-compatible-embedding-provider.test.ts pnpm tsgo:core pnpm tsgo:core:test node --import tsx scratchpad/embedding-bound-proof.mts .agents/skills/autoreview/scripts/autoreview --mode localObserved result: focused Vitest passed, both tsgo lanes exited 0, the scratchpad proof produced PASS lines for bounded throw, socket abort, small JSON happy path, and an old unbounded
response.json()negative control.What was not tested: full repository Vitest/CI matrix was not run; this patch only changes the OpenAI-compatible embedding success JSON read path and its focused provider tests.
Evidence
Label: security
AI-assisted.