Skip to content

fix(plugins): bound OpenAI-compatible embeddings JSON response reads#96253

Closed
cxbAsDev wants to merge 0 commit into
openclaw:mainfrom
cxbAsDev:fix/bound-embeddings-response
Closed

fix(plugins): bound OpenAI-compatible embeddings JSON response reads#96253
cxbAsDev wants to merge 0 commit into
openclaw:mainfrom
cxbAsDev:fix/bound-embeddings-response

Conversation

@cxbAsDev

@cxbAsDev cxbAsDev commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Replace the custom readJsonResponse helper with the shared readProviderJsonResponse from provider-http-errors.ts. The shared reader already includes the standard 16 MB bounded read via readResponseWithLimit, eliminating a duplicate bounded-read pattern and keeping the embeddings provider aligned with provider HTTP guardrails.

Real behavior proof (required for external PRs)

Behavior addressed: Custom unbounded response.json() replaced with shared bounded readProviderJsonResponse from provider-http-errors.ts.

Real environment tested: Linux, Node 24, OpenClaw main @ 0671c08

Exact steps or command run after fix:

pnpm install --frozen-lockfile --ignore-scripts
npx tsc --noEmit src/plugins/openai-compatible-embedding-provider.ts

Evidence after fix:

import { readProviderJsonResponse } from "../agents/provider-http-errors.js";

// ...

const EMBEDDINGS_LABEL = "openai-compatible embeddings failed";

// Call site:
(await readProviderJsonResponse(response, EMBEDDINGS_LABEL))
  as OpenAICompatibleEmbeddingResponse,

The shared readProviderJsonResponse uses readResponseWithLimit with the standard 16 MB PROVIDER_JSON_RESPONSE_MAX_BYTES cap and wraps malformed JSON with the caller's label. This avoids duplicating the bounded-read pattern.

Observed result after fix: Embeddings API responses use the same bounded JSON read path as all other provider HTTP clients. No custom bounded-read implementation remains.

What was not tested: A live embedding API call against a real OpenAI-compatible embeddings server was not tested.

Risk

Low. The shared reader is the same utility used by all other provider JSON reads. The 16 MB cap is the standard provider convention. Error wrapping is preserved through the EMBEDDINGS_LABEL constant.

@openclaw-barnacle openclaw-barnacle Bot added size: XS 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 24, 2026
@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Summary
The PR removes the OpenAI-compatible embedding provider's local successful-response JSON parser and calls readProviderJsonResponse with the embedding error label.

PR surface: Source -2. Total -2 across 1 file.

Reproducibility: no. high-confidence runtime reproduction was run in this read-only review. Source inspection on current main shows the success path still calls response.json() before vector validation, so an oversized successful response would be read without the shared cap.

Review metrics: 1 noteworthy metric.

  • Successful response cap: 1 added, 16 MiB. The PR changes the successful OpenAI-compatible embedding response path from unbounded response.json() to the shared provider JSON cap, which is compatibility-visible for very large batches.

Stored data model
Persistent data-model change detected: vector/embedding metadata: src/plugins/openai-compatible-embedding-provider.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until 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, screenshot, recording, or copied live output from an after-fix real or local OpenAI-compatible embedding call; redact private details such as API keys, IP addresses, phone numbers, and non-public endpoints.
  • After updating the PR body with proof, ClawSweeper should re-review automatically; if it does not, ask a maintainer to comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Missing: the PR body provides typecheck/source evidence and explicitly says no live embedding API call was tested; add redacted proof from a real or local compatible embedding call and update the PR body to trigger re-review.

Risk before merge

  • [P1] Successful OpenAI-compatible embedding responses larger than 16 MiB that the latest release and current main would parse can fail after this PR; maintainers should accept that shared provider cap as intentional hardening before merge.
  • [P1] The external PR still has no redacted runtime proof from a real or local OpenAI-compatible embedding call after the change.

Maintainer options:

  1. Require runtime proof before merge (recommended)
    Ask the contributor to add redacted terminal, log, or screenshot proof from an after-fix real or local OpenAI-compatible embedding call, then re-review the same head.
  2. Accept the shared 16 MiB provider cap
    Maintainers can explicitly accept that very large successful embedding JSON responses now fail because this provider should follow the shared provider JSON guardrail.
  3. Pause if large batches need a different policy
    If embeddings need a larger or configurable success-response budget, pause this PR and choose that provider policy before merging a cap change.

Next step before merge

  • [P1] The remaining blocker is contributor real behavior proof plus maintainer compatibility acceptance, not a narrow code defect ClawSweeper can safely repair.

Security
Cleared: Cleared: the diff only changes in-repo provider response parsing and adds no dependency, workflow, secret, package-resolution, or external code-execution surface.

Review details

Best possible solution:

Land the narrow shared-reader change after contributor real behavior proof and explicit maintainer acceptance of the shared 16 MiB provider JSON cap.

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

No high-confidence runtime reproduction was run in this read-only review. Source inspection on current main shows the success path still calls response.json() before vector validation, so an oversized successful response would be read without the shared cap.

Is this the best way to solve the issue?

Yes for code shape: reusing readProviderJsonResponse is the narrowest maintainable fix and avoids duplicating provider HTTP guardrails. The remaining blockers are real behavior proof and maintainer acceptance of the cap's upgrade impact.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority provider hardening fix with limited blast radius, but it still needs proof and compatibility acceptance before merge.
  • merge-risk: 🚨 compatibility: The PR can reject very large successful embedding responses that the latest release and current main would parse.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Missing: the PR body provides typecheck/source evidence and explicitly says no live embedding API call was tested; add redacted proof from a real or local compatible embedding call and update the PR body to trigger re-review.
Evidence reviewed

PR surface:

Source -2. Total -2 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 7 9 -2
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 7 9 -2

What I checked:

Likely related people:

  • jalehman: Current-main blame for the embedding provider, shared provider JSON helper, and response-limit helper points to commit 8a7b3c755a42d1ad1bb30f6dfa3a9e42f128cfa8, and live PR metadata shows Josh Lehman authored and merged fix(memory-core): migrate dreaming cleanup lifecycle #96193. (role: recent area contributor; confidence: high; commits: 8a7b3c755a42; files: src/plugins/openai-compatible-embedding-provider.ts, src/agents/provider-http-errors.ts, packages/media-core/src/read-response-with-limit.ts)
  • mbelinky: The merged OpenAI-compatible embedding provider work in feat(embeddings): add OpenAI-compatible core provider #85269 includes multiple commits by Mariano Belinky and was merged by mbelinky. (role: core provider merger and refactor contributor; confidence: high; commits: 9f5f3b88457a, 0247fadc4a46, f96ba85c40ff; files: src/plugins/openai-compatible-embedding-provider.ts, src/plugins/openai-compatible-embedding-provider.test.ts)
  • dutifulbob: The merged core-provider PR contains several follow-up commits by dutifulbob for compatible provider config paths, aliases, and edge cases. (role: embedding provider feature contributor; confidence: high; commits: 1533f9a844cf, 6850380cfa89, 76e702af0046; files: src/plugins/openai-compatible-embedding-provider.ts, src/plugins/openai-compatible-embedding-provider.test.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 24, 2026
@cxbAsDev
cxbAsDev force-pushed the fix/bound-embeddings-response branch from b126d8b to d8d178b Compare June 24, 2026 04:43
@cxbAsDev

cxbAsDev commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 24, 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 mentioned this pull request Jun 25, 2026
Merged
@cxbAsDev cxbAsDev closed this Jun 29, 2026
@cxbAsDev
cxbAsDev force-pushed the fix/bound-embeddings-response branch from d8d178b to 87db23e Compare June 29, 2026 00:51
@cxbAsDev
cxbAsDev deleted the fix/bound-embeddings-response branch June 29, 2026 00:53
@clawsweeper clawsweeper Bot mentioned this pull request Jun 29, 2026
Closed
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. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@cxbAsDev