fix(microsoft-foundry): bound connection test error reads#97812
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 29, 2026, 11:10 AM ET / 15:10 UTC. Summary PR surface: Source +9, Tests +73. Total +82 across 2 files. Reproducibility: yes. Current main source shows Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsBest possible solution: Merge this bounded-reader fix after ordinary maintainer approval, preserving the existing 200-character diagnostic while capping the body read. Do we have a high-confidence way to reproduce the issue? Yes. Current main source shows Is this the best way to solve the issue? Yes. Reusing the existing AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 825d9a66623a. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +9, Tests +73. Total +82 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
rating: 🧂 unranked krab
|
Addressed the P1 "mock-only proof" blocker: updated the PR body with real-HTTP behavior proof outside Vitest mocks. The exact swapped-in reader With the fix each branch reads exactly 8192 bytes, the 200-char note slice stays intact, and the server flushes only ~0.22 MiB before the stream is canceled; the pre-fix @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
proof: sufficient
…7812) Co-authored-by: Pick-cat <[email protected]>
…7812) Co-authored-by: Pick-cat <[email protected]>
…7812) Co-authored-by: Pick-cat <[email protected]>
…7812) Co-authored-by: Pick-cat <[email protected]>
…7812) Co-authored-by: Pick-cat <[email protected]> (cherry picked from commit eb5fb2a)
…7812) Co-authored-by: Pick-cat <[email protected]> (cherry picked from commit eb5fb2a)
2 participants
What Problem This Solves
testFoundryConnectionreads the connection-test error body with an unboundedawait res.text()on both failure branches (400and any other non-OK status) during Microsoft Foundry onboarding. The endpoint URL is operator-supplied; a host that returns a large body with noContent-Length(wrong endpoint, error page, broken proxy, or hostile host) makes onboarding buffer the entire payload into memory just to display a 200-char snippet — a memory-pressure vector on the setup path.Affected surface:
extensions/microsoft-foundry/onboard.ts— the400and!res.okbranches oftestFoundryConnection.Why This Change Was Made
Both branches only ever display
body.slice(0, 200)in the prompter note, but they read the whole body first. This routes both reads through the existing shared bounded readerreadResponseTextLimited(fromopenclaw/plugin-sdk/provider-http) with an 8 KiB cap (FOUNDRY_CONNECTION_TEST_ERROR_BODY_LIMIT_BYTES) — far above the 200-char display need, so the note is unchanged while the read can no longer grow without bound. The reader cancels the upstream stream once the cap is hit. The existing.catch(() => "")is preserved, so the note degrades exactly as before on a read failure.No new abstraction — reuses the existing plugin-SDK helper. Plugin-local change, two call sites with the identical pattern.
User Impact
Operators running the Foundry connection test still see the same diagnostic note (status + first 200 chars of the error body), but a misbehaving or hostile endpoint can no longer drive onboarding to buffer an arbitrarily large error body into memory.
Evidence
1. Real-behavior proof with a negative control (real HTTP, outside Vitest mocks). The exact swapped-in reader —
readResponseTextLimited, imported from the same SDK barrel the fix uses (openclaw/plugin-sdk/provider-http) — is driven against a realnode:httpserver that streams a 25 MiB error body in chunks with noContent-Length, for both changed branches (400and non-OK503), measuring bytes actually flushed on the wire before the socket closes. The negative control runs the pre-fixawait res.text()against the same server. All endpoints are127.0.0.1; no secrets.Without the fix the read returns the full 25.03 MiB body; with the fix each branch reads exactly 8192 bytes, the 200-char note slice is still intact, and the server flushes only 0.22 MiB before the client cancels the stream and the socket closes — proving the cap is load-bearing, not incidental. The residual ~0.22 MiB is in-flight TCP/socket buffering, not buffered by the reader.
2. Committed regression test (
extensions/microsoft-foundry/onboard.connection.test.ts) drives the realtestFoundryConnectionthrough the non-OK branch with a streamed oversized error body, assertsres.text()is never called, asserts the stream is canceled, and asserts the prompter note still renders the bounded snippet.3. Lint on the changed file:
What was NOT tested: the full
testFoundryConnectionwrapper was not driven end-to-end against a live Azure endpoint, because it first acquires a token via theazCLI (getAccessTokenResult→execAz) and itsfetchWithSsrFGuardcall uses the default (private-network-denying) SSRF policy, so localhost cannot be substituted live. The committed Vitest test exercises that wrapper with the token/fetch boundary stubbed; the bounded read itself — the only line this PR changes — is proven live over a real socket above, including the negative control. I did not drive the process to an actual OOM.AI-assisted: implementation, tests, and the proof harness were drafted with agent assistance; all commands above were run locally.