fix(anthropic): bound streaming 200 success-body SSE reads via shared plugin-sdk guard#96632
Conversation
|
Codex review: found issues before merge. Reviewed June 25, 2026, 12:13 AM ET / 04:13 UTC. Summary PR surface: Source +138, Tests +76, Other +415. Total +629 across 5 files. Reproducibility: yes. Source inspection shows current main appends delimiter-free Anthropic SSE success chunks into unbounded buffers in both parser paths, and the PR body includes after-fix local-server proof with overflow and negative-control cases. Review metrics: 2 noteworthy metrics.
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 findings
Review detailsBest possible solution: Land the internal Anthropic SSE guard after maintainers accept or adjust the 16 MiB fail-closed boundary and the helper/repro cleanup is addressed; leave SDK promotion to a separate plugin-owned follow-up. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main appends delimiter-free Anthropic SSE success chunks into unbounded buffers in both parser paths, and the PR body includes after-fix local-server proof with overflow and negative-control cases. Is this the best way to solve the issue? Yes, with small cleanup. Keeping the guard internal is the narrow owner-boundary fix for the two core Anthropic paths, while the exact 16 MiB fail-closed boundary needs maintainer acceptance or tuning. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e7f2b125f628. Label changesLabel justifications:
Evidence reviewedPR surface: Source +138, Tests +76, Other +415. Total +629 across 5 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
|
539054c to
395cbdf
Compare
|
@clawsweeper re-review Following your review verdict (🦐 gold shrimp) and the three maintainer options you surfaced, I picked Option 2 — Keep The Guard Internal For Now: What changed in r2:
Closes the P1 blockers from your previous review:
Should also resolve the three CI failures ( Re-verification (all green locally):
When does the helper get promoted to a public SDK subpath? Only when an external plugin (planned: |
|
🦞🧹 I asked ClawSweeper to review this item again. |
395cbdf to
ec7cb9f
Compare
|
Additional fix in r2 (force-pushed as
Re-verification after the additional lint fix:
CI for |
… internal guard ClawSweeper review follow-up: revert stream-reader-with-limit from a new public plugin-SDK subpath to an internal helper under src/agents/. The new subpath required maintainer API-boundary acceptance plus regenerated SDK metadata; deferring that to the extension follow-up PRs (extensions/google + extensions/ollama) keeps this PR a clean bounded-refactor with no public SDK surface change. The helper now lives at src/agents/streaming-byte-guard.ts, mirroring the existing readResponseWithLimit / readProviderJsonResponse family in src/agents/provider-http-errors.ts. Both Anthropic SSE parsers (the new src/agents/anthropic-transport-stream.ts parser and the legacy src/llm/providers/anthropic.ts iterateSseMessages) consume the helper via relative imports, no plugin-sdk boundary involved. Removal summary: - DELETE src/plugin-sdk/stream-reader-with-limit.ts - CREATE src/agents/streaming-byte-guard.ts (rename with comment update) - UPDATE src/agents/anthropic-transport-stream.ts (relative import) - UPDATE src/llm/providers/anthropic.ts (relative import) - UPDATE scripts/repro/issue-anthropic-sse-bound-read.mjs (relative path) - REMOVE entry from scripts/lib/plugin-sdk-entrypoints.json (line 314) - REMOVE ./plugin-sdk/stream-reader-with-limit export from package.json - REMOVE row from docs/plugins/sdk-subpaths.md - REMOVE two now-unused @ts-expect-error directives in anthropic-transport-stream.test.ts Re-verification: - vitest: 115/115 (src/agents/anthropic-transport-stream.test.ts + src/llm/providers/anthropic.test.ts) - repro script: 5/5 PASS (real node:http server, 64 MiB content-length-less body) - tsgo: clean - oxlint: clean - plugin-sdk:check-exports: synced - docs/.generated/plugin-sdk-api-baseline.sha256: unchanged (SDK surface restored) Closes ClawSweeper P1 blockers from review on this PR: new public SDK subpath requires maintainer acceptance (deferred), SDK metadata drift (resolved by removal).
ec7cb9f to
73693b5
Compare
…red internal guard Same OOM family as PR openclaw#96632 (Anthropic SSE success-body bound), applied to the two remaining core streaming-body surfaces that read response.body.getReader() without an accumulation cap: - src/llm/providers/openai-chatgpt-responses.ts (parseSSE, ChatGPT Responses) - src/agents/runtime/proxy.ts (streamProxy, internal proxy route) Both surfaces accumulate decoder-decoded bytes into a string buffer while waiting for the next frame delimiter, with no upper bound. A hostile or malfunctioning endpoint (ChatGPT Responses, custom proxy backend) can return a Content-Length-less SSE body that streams forever, exhausting process memory on every /v1/messages or proxied-stream call. Fix: wrap the existing response.body.getReader() in the shared src/agents/streaming-byte-guard.ts createSseByteGuard helper (introduced in openclaw#96632, now reused). 16 MiB cap, matching readProviderJsonResponse. The existing chunk-by-chunk parsing loop, frame delimiter handling, and signal abort checks are unchanged. For proxy.ts the abort handler also routes through guard.cancel() so the bounded cancel propagates immediately on signal abort (with a fallback to reader.cancel() for the brief window before the guard is constructed). Tests: - src/llm/providers/openai-chatgpt-responses.test.ts: new describe block drives parseSSEForTest (renamed from parseSSE for direct testing) against a hostile 1 MiB-per-pull streaming body, asserts canonical overflow message and bounded pullCount (17-20 pulls), plus a happy-path end-to-end parse. - src/agents/runtime/proxy.test.ts: new integration test stubs fetch with a hostile ReadableStream body, asserts the stream ends with the bounded error and pullCount is bounded. Real-environment proof: scripts/repro/issue-openai-proxy-sse-bound-read.mjs runs against a real node:http server bound to 127.0.0.1 that streams a Content-Length-less 64 MiB body chunk-by-chunk. Five PASS lines: 1. bounded rejection: bytesSent ~19 MiB (<< 64 MiB), server.aborted=true 2. cap-trace: bounded reader cancelled at ~16 MiB 3. negative control: small body fully drained, no cap 4. happy path: small valid OpenAI SSE drained end-to-end 5. legacy-path parity: parseSSEForTest overflow surfaces canonical error Stacked on fix/anthropic-stream-bound-read (openclaw#96632); no new public SDK subpath, no SDK metadata drift, no plugin boundary touched. Stacked-on: fix/anthropic-stream-bound-read
|
Closing in favor of the Alix-007-style XS split. Why: This PR's diff (L size, 5 files, 639 LoC) is bigger than the per-surface pattern Alix-007 uses for the bounded-read family (XS, 2 files, 50–135 LoC — see #95218, #95417, #96027, #96035, #96038, #96042, #95244, #96607). ClawSweeper's gold-shrimp verdict reflects patch-quality concerns about diff size, not proof quality. What happens to this work:
Diff size analysis:
Following the per-surface split will resolve both the ClawSweeper patch-quality flag and the staging-order concern. |
…(provider path) Apply createSseByteGuard to src/llm/providers/anthropic.ts (legacy provider iterateSseMessages) so the Anthropic Messages provider SSE parser cannot be exhausted by a hostile or malfunctioning Anthropic-compatible endpoint that streams an unbounded SSE body. The 16 MiB cap matches the non-streaming readProviderJsonResponse cap. What changed: - src/llm/providers/anthropic.ts: wrap body.getReader() in createSseByteGuard before the existing iterateSseMessages loop. The function is also re-exported as iterateSseMessagesForTest for direct test access. - Inline test added to anthropic.test.ts: hostile 1 MiB pull stream, asserts canonical overflow message, cancel(reason) was an Error instance, pullCount bounded to 17-20. Reuses the createSseByteGuard helper from src/agents/streaming-byte-guard.ts (introduced in openclaw#96701 for the anthropic-transport-stream path; same helper, same 16 MiB cap, same overflow-error shape). This PR is the second of the planned per-surface rescue series for the previously closed PR openclaw#96632, after openclaw#96701. No SDK surface change. No repro script committed (proof in this body). The companion error-body path is already bounded (readAnthropicMessages ErrorBodySnippet, 8 KiB + 10s idle timeout, openclaw#95108); this PR closes the legacy-provider success-body gap.
…(provider path) Apply createSseByteGuard to src/llm/providers/anthropic.ts (legacy provider iterateSseMessages) so the Anthropic Messages provider SSE parser cannot be exhausted by a hostile or malfunctioning Anthropic-compatible endpoint that streams an unbounded SSE body. The 16 MiB cap matches the non-streaming readProviderJsonResponse cap. What changed: - src/llm/providers/anthropic.ts: wrap body.getReader() in createSseByteGuard before the existing iterateSseMessages loop. The function is also re-exported as iterateSseMessagesForTest for direct test access. - Inline test added to anthropic.test.ts: hostile 1 MiB pull stream, asserts canonical overflow message, cancel(reason) was an Error instance, pullCount bounded to 17-20. Reuses the createSseByteGuard helper from src/agents/streaming-byte-guard.ts (introduced in openclaw#96701 for the anthropic-transport-stream path; same helper, same 16 MiB cap, same overflow-error shape). This PR is the second of the planned per-surface rescue series for the previously closed PR openclaw#96632, after openclaw#96701. No SDK surface change. No repro script committed (proof in this body). The companion error-body path is already bounded (readAnthropicMessages ErrorBodySnippet, 8 KiB + 10s idle timeout, openclaw#95108); this PR closes the legacy-provider success-body gap.
What Problem This Solves
src/agents/anthropic-transport-stream.tsandsrc/llm/providers/anthropic.tsaccumulate an unbounded SSE byte stream into a string buffer while waiting for\n\nframe delimiters. The companion error-body path (readAnthropicMessagesErrorBodySnippet, 8 KiB cap + 10s idle timeout) was bounded by #95108, but the symmetric 200 success-body path was left open. A hostile or malfunctioning Anthropic-compatible endpoint (including an MITM-able proxy, a cloud-hosted mirror likeAnthropic-Vertex, or a hijacked self-hosted provider) can return aContent-Length-less SSE body that streams forever, exhausting process memory on a code path that runs for every/v1/messagescall.This is the streaming-body analogue of the bounded-read sweep Alix-007 finished for non-streaming
response.json()/response.text()(#95218,#95223,#96027,#96035,#96038,#96042): same OOM family, same helper-driven design, same 16 MiB cap, but on the chunk-by-chunk SSE path that Alix-007's sweep did not cover.ClawSweeper review follow-up (r2)
The first review (🦐 gold shrimp) flagged the helper as a new public plugin-SDK subpath needing maintainer API-boundary acceptance plus regenerated SDK metadata. Following Maintainer Option 2 ("Keep The Guard Internal For Now"), the helper now lives at
src/agents/streaming-byte-guard.tsas a core-internal helper mirroring the existingreadResponseWithLimit/readProviderJsonResponsefamily insrc/agents/provider-http-errors.ts. Both Anthropic SSE parsers consume it via relative imports. The public SDK subpath, thepackage.jsonexport, thescripts/lib/plugin-sdk-entrypoints.jsonentry, and the docs row are all removed. SDK API baseline hash (docs/.generated/plugin-sdk-api-baseline.sha256) is unchanged from upstreammain. When an extension (e.g.extensions/google,extensions/ollama) genuinely needs the helper, it will be promoted back through the SDK seam in its own follow-up PR.Changes
src/agents/streaming-byte-guard.ts(new, ~108 lines) —createSseByteGuardhelper. Internal core helper (NOT a plugin-SDK subpath). Wraps aReadableStreamDefaultReader<Uint8Array>, tracks accumulated bytes across the caller's existing chunk loop, and cancels the underlying reader + throws a canonical overflow error oncemaxBytesis exceeded. Exposesread() / cancel() / totalBytes() / overflowed()so the same guard drives both production and AbortSignal-handling code paths.src/agents/anthropic-transport-stream.ts:691—parseAnthropicSseBodyForTestnow wires the guard withANTHROPIC_MESSAGES_SUCCESS_BODY_MAX_BYTES = 16 * 1024 * 1024. The existingreadAnthropicSseChunktype was widened to accept any{ read, cancel }so the same helper drives the guarded reader.src/llm/providers/anthropic.ts:355—iterateSseMessages(the older provider's SSE parser) gets the same bound, with its own constant to keep the two call sites independently readable.src/agents/anthropic-transport-stream.test.ts— two new tests:bounds streamed Anthropic success responses without content-lengthdrivesparseAnthropicSseBodyForTestagainst a hostile 1 MiB-per-pull streaming body, asserts the canonical overflow message (Anthropic Messages success body exceeded 16777216 bytes), confirmscancel(reason)was invoked, and verifiespullCountis bounded well below the unconstrained ceiling.parses normal Anthropic success SSE responses end-to-endensures small valid SSE frames still parse through the bounded reader unchanged.Real behavior proof
node:httpserver bound to127.0.0.1returning aContent-Length-less 64 MiB body (4× the cap) chunk-by-chunk with backpressure, plus a small (8 MiB) negative-control body, plus a happy-path body. Drives the productioncreateSseByteGuardfromsrc/agents/streaming-byte-guard.tsagainst both Anthropic SSE parsers. Run withpnpm exec tsx scripts/repro/issue-anthropic-sse-bound-read.mjs.node scripts/run-vitest.mjs anthropic-transport-stream anthropic.test --run— 115/115 passed (2 files, includes 2 new tests)pnpm exec tsx scripts/repro/issue-anthropic-sse-bound-read.mjs— 5/5 PASSnode scripts/run-tsgo.mjs— cleannode scripts/run-oxlint.mjs— cleannode scripts/sync-plugin-sdk-exports.mjs --check—plugin-sdk exports synced.node scripts/generate-plugin-sdk-api-baseline.ts --check— exit 0 (baseline hash unchanged from upstreammain)createSseByteGuardcancelled the upstreamReadableStreamDefaultReaderafter ~17-19 MiB (≪ 64 MiB). Server-sidebytesSentobserved at 17,825,792 — bounded, not drained.cancel(reason)received anErrorinstance as the cancel reason. Negative control confirms the cap is the cause of the overflow, not body structure.Out of scope (separate PRs)
parseAnthropicSseBodypattern recurs in 5 other streaming code paths (openai-chatgpt-responses.ts,runtime/proxy.ts,extensions/google/transport-stream.ts,extensions/ollama/src/stream.ts, plus a 6th in the WhatsApp stalled-read family) — each will get its own PR. The core PRs (openai-chatgpt-responses.ts+runtime/proxy.ts) will reuse the same internal helper via relative import. The extension PRs (extensions/google+extensions/ollama) will promote the helper back to a public plugin-SDK subpath with the full SDK metadata synchronization, since extensions cannot importsrc/agents/**per the boundary rule insrc/plugin-sdk/CLAUDE.md.Risk
readProviderJsonResponsefor non-streaming Anthropic success bodies. The SSE parser's behavior for normal-sized responses is unchanged — only the unbounded case now throws a clear overflow error instead of buffering without limit.createSseByteGuard.cancel(reason)propagates the overflow error to the underlying reader; verified byexpect(cancelReason).toBeInstanceOf(Error)in the test.main. The follow-up extension PRs will own the SDK promotion when it becomes necessary.