fix(github-copilot): bound model discovery and embeddings JSON response#96499
Conversation
…se reads The GitHub Copilot embeddings plugin already bounds its error response bodies via readResponseTextLimited, but the success JSON reads for both model discovery and the embeddings call used unbounded response.json(). Route both through readProviderJsonResponse (16 MiB cap). Update isCopilotSetupError to recognise the new error label prefix so auto-selection still falls through on malformed discovery responses. Update tests to use proper Response objects and the new error messages. AI-assisted. Co-authored-by: Cursor <[email protected]>
|
Codex review: needs maintainer review before merge. Reviewed June 25, 2026, 2:38 PM ET / 18:38 UTC. Summary PR surface: Source -3, Tests -1. Total -4 across 2 files. Reproducibility: yes. Current main still calls Review metrics: 1 noteworthy metric.
Stored data model 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 the focused bounded-reader change after CI and maintainer acceptance of the 16 MiB discovery cap and 64 MiB embeddings cap. Do we have a high-confidence way to reproduce the issue? Yes. Current main still calls Is this the best way to solve the issue? Yes. After the follow-up commit, the PR uses the shared bounded JSON helper, keeps model discovery on the generic provider cap, and preserves the 64 MiB memory embedding response budget for AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 66e2fcc6f83e. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source -3, Tests -1. Total -4 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
Signed-off-by: sallyom <[email protected]>
|
Maintainer edit pushed: The existing bounded-reader proof remains sufficient for review: it verifies that the shared reader enforces the configured byte budget and cancels the stream on overflow. This edit changes only the embeddings Focused proof after the edit: |
|
Maintainer local review: merge-ready once required CI is green. Local autoreview is clean on the updated branch, focused Copilot embeddings tests passed, and the maintainer cap edit aligns embeddings success JSON with the sibling 64 MiB remote memory embedding response budget while keeping model discovery at 16 MiB. No remaining compatibility/API/config concerns from my review. |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
…se (openclaw#96499) * fix(github-copilot): bound model discovery and embeddings JSON response reads The GitHub Copilot embeddings plugin already bounds its error response bodies via readResponseTextLimited, but the success JSON reads for both model discovery and the embeddings call used unbounded response.json(). Route both through readProviderJsonResponse (16 MiB cap). Update isCopilotSetupError to recognise the new error label prefix so auto-selection still falls through on malformed discovery responses. Update tests to use proper Response objects and the new error messages. AI-assisted. Co-authored-by: Cursor <[email protected]> * fix(github-copilot): use memory embedding response cap Signed-off-by: sallyom <[email protected]> --------- Signed-off-by: sallyom <[email protected]> Co-authored-by: Cursor <[email protected]> Co-authored-by: sallyom <[email protected]>
…se (openclaw#96499) * fix(github-copilot): bound model discovery and embeddings JSON response reads The GitHub Copilot embeddings plugin already bounds its error response bodies via readResponseTextLimited, but the success JSON reads for both model discovery and the embeddings call used unbounded response.json(). Route both through readProviderJsonResponse (16 MiB cap). Update isCopilotSetupError to recognise the new error label prefix so auto-selection still falls through on malformed discovery responses. Update tests to use proper Response objects and the new error messages. AI-assisted. Co-authored-by: Cursor <[email protected]> * fix(github-copilot): use memory embedding response cap Signed-off-by: sallyom <[email protected]> --------- Signed-off-by: sallyom <[email protected]> Co-authored-by: Cursor <[email protected]> Co-authored-by: sallyom <[email protected]>
2 participants
What Problem This Solves
The GitHub Copilot embeddings plugin already bounds its error response
bodies via
readResponseTextLimited(8 KiB cap), but the success JSON readsfor both model discovery and the embeddings call used an unbounded
await response.json(). This asymmetry means a misbehaving or hostile CopilotAPI endpoint can stream an arbitrarily large success body into memory before
parsing, causing memory pressure on the model discovery and embeddings hot path —
while the same file already demonstrates awareness that response bodies need to
be capped.
The error body is bounded on both paths; this PR closes the symmetric gap on
the success-JSON side, matching the pattern already applied to other provider
paths in the
#95103/#95108/#95218response-limit campaign.Changes
discoverEmbeddingModels: replaces thetry { payload = await response.json() } catchblock with
readProviderJsonResponse(response, "github-copilot.model-discovery")(16 MiB cap) from
openclaw/plugin-sdk/provider-http.embed(embeddings call): replaces the identical pattern withreadProviderJsonResponse(response, "github-copilot.embeddings").isCopilotSetupError: adds the"github-copilot.model-discovery"label prefixso auto-selection still falls through on malformed discovery responses with the
new error format.
embeddings.test.ts:mockDiscoveryResponseto return realResponseobjects (required bythe streaming bounded reader; plain
{ json: async () => ... }objects lack.body).a throwing
.json()stub.shouldContinueAutoSelectionassertion to match the new errormessage prefix
"github-copilot.model-discovery: malformed JSON response".Real behavior proof
Content-Lengthand an oversized/never-ending body must not be buffered whole;the read must stop at the cap, cancel the stream, and throw a bounded error —
while valid small responses still parse unchanged.
node:httpserver (createServer) boundto
127.0.0.1, streaming a JSON body in 64 KiB chunks with noContent-Length header, driving the real exported
readResponseWithLimithelper (the same helper
readProviderJsonResponsewraps internally) onNode v22.22.0.
GET /hugestreams an unbounded JSON object (~24 MiB) withno
Content-Length;GET /smallreturns one valid response.readResponseWithLimit(response, 1 MiB)against/hugeand assertit rejects + the server socket is closed early.
await response.json()against thesame
/hugebody and measure how many bytes the server pushed.readResponseWithLimit(response, 1 MiB)against/smalland assertthe result is parsed intact.
JSON response exceeds 1048576 bytes; server socketclosed early (stream cancelled); only 1,064,178 bytes reached the
wire — near the 1 MiB cap, not the 24 MiB body.
response.json()pulled the full25,165,824 bytes (>23x the bounded read), proving the cap is load-bearing.
ALL PROOF ASSERTIONS PASSED. Plus the in-repo Vitestsuite passes 1/1 file, 12/12 tests — including the existing bounds-error
body tests and the updated malformed-JSON fixture.
oxlintis clean onchanged files.
The proof instead measures bytes-on-wire and early socket close, which is
the load-bearing signal.
Evidence
Label: security
AI-assisted.