Repository navigation
fix(client): preserve Buffer replies in the client-side cache - #3539
suryakant-source wants to merge 4 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc5e8f7d7f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (original instanceof Buffer) { | ||
| const view = clone as Uint8Array; | ||
| return Buffer.from(view.buffer, view.byteOffset, view.length); |
There was a problem hiding this comment.
When the same Buffer instance appears in multiple positions in a reply, this branch runs before the seen lookup and creates a distinct Buffer wrapper on every visit. The wrappers share an ArrayBuffer, but a === b becomes false; if the shared buffer is both a Map key and referenced elsewhere, lookups through that reference can consequently fail. structuredClone normally preserves this alias identity, so the reconstructed buffer needs to be stored in the memo as well.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit cc5e8f7. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd4d28e512
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (let i = 0; i < original.length; i++) { | ||
| clonedArray[i] = reapplyReplyBuffers(clonedArray[i], original[i], seen); |
There was a problem hiding this comment.
When a cacheable reply contains a sparse array—concretely, JSON.GET is cacheable and its public JSON reviver creates a hole when it returns undefined for an array element—this loop visits the absent index and assigns undefined into the clone. structuredClone preserves the hole, so enabling client-side caching changes checks such as 1 in result from false to true on both the fill and hit paths; only assign indices that are actually present on the original array.
Useful? React with 👍 / 👎.
| if (!isPlainObject(original)) { | ||
| // `String` objects (`VerbatimString`), errors, dates and custom mapped classes keep | ||
| // their `structuredClone` result: their indexed/own properties are not writable | ||
| // (a `String` object throws on assignment) and cannot hold buffers in practice | ||
| return clone; |
There was a problem hiding this comment.
When a cacheable transform returns a class instance with a Buffer own property—for example, JSON.GET's exposed reviver can return a wrapper class—this branch returns the raw structured clone, where that property is still a Uint8Array. Unlike the String object this guard was added for, structuredClone turns an ordinary custom class into a plain object with writable fields, so the blanket exclusion leaves the binary-reply bug unfixed for supported arbitrary transforms; walk those cloned own fields or restrict the exemption to clone-special built-ins.
Useful? React with 👍 / 👎.
Summary
Every read served through the client-side cache passes through
structuredClone, which clones aBufferas a plainUint8Array. With aBLOB_STRING -> Buffertype mapping (the setup the client-side caching docs recommend for binary replies), cached commands returnUint8Arrayinstead ofBuffer:Buffer.isBuffer()is false,toString()gives comma-separated byte values, and the static types disagree with the runtime value. This happens on the very first read too, because the reply is cloned before being stored.The clone and the original reply have identical structure, so the fix walks them in tandem and turns the cloned values back into
Buffers exactly where the original reply had them. The memoized walk keeps shared references (and cycles) intact, andstructuredClonesemantics are unchanged for everything else (arrays, maps, sets, plain objects, errors, dates).Fixes #3536.
Test plan
cache.spec.ts:Bufferpreserved on miss and hit with independent copies;Buffers nested in arrays, maps, sets and plain objects;structuredClonesemantics kept for the rest of the reply.tsc --buildand eslint are clean.Note
Medium Risk
Touches every cached reply return path in the client, but the change is a targeted clone fix with broad unit coverage and no auth or persistence impact.
Overview
Client-side cache reads no longer turn
Bufferreplies into plainUint8Arrayvalues.handleCachenow returns replies throughcloneReplyinstead of barestructuredClone, soBLOB_STRING -> Buffertype mapping stays correct on cache misses and hits.cloneReplystill deep-clones withstructuredClone, thenreapplyReplyBufferswalks the original and clone together to restoreBufferinstances in arrays, maps, sets, and plain objects while keeping shared references and cycles. Non-plain types such asVerbatimStringkeepstructuredClonebehavior without unsafe property writes.New server-free Reply cloning tests cover top-level and nested buffers, mutation isolation, verbatim strings, and unchanged clone semantics elsewhere. Fixes #3536.
Reviewed by Cursor Bugbot for commit dd4d28e. Bugbot is set up for automated code reviews on this repo. Configure here.