fix(go/ai): make ModelResponse.History nil-safe and stop aliasing request messages - #5883
Open
apascal07 wants to merge 2 commits into
Open
fix(go/ai): make ModelResponse.History nil-safe and stop aliasing request messages#5883apascal07 wants to merge 2 commits into
ModelResponse.History nil-safe and stop aliasing request messages#5883apascal07 wants to merge 2 commits into
Conversation
History dereferenced its receiver inside the guard meant to protect against a nil receiver, so the nil check guaranteed the panic it was written to prevent. Request could also be nil independently, since not every model plugin assigns it, which panicked on the append path. History also appended the response message directly onto Request.Messages. When that slice had spare capacity the append wrote into the caller's backing array, so the result aliased anything else pointing at it and successive calls overwrote each other. Return early on a nil receiver, treat a nil Request as no prior messages, and build the result from a clone so callers own the slice they get back.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the History() method of ModelResponse to ensure it always returns a freshly allocated slice, preventing callers from accidentally mutating the underlying request messages. It also adds comprehensive unit tests to verify this behavior. The review feedback points out a performance optimization in History() where appending to slices.Clone causes a double allocation, and suggests pre-allocating the slice instead.
apascal07
marked this pull request as ready for review
August 1, 2026 02:26
slices.Clone allocates at capacity equal to length for most sizes, so appending the response message reallocated and recopied. Size the slice once and copy into it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes three panic paths and a slice-aliasing defect in
ModelResponse.History, found while verifying #4683.The nil guard dereferenced the receiver it was checking:
if mr == nilfell through toreturn mr.Request.Messages.Requestis nil-able independently of the receiver and is nil in shipped code, since the Veo background model stashes its request inMetadata["inputRequest"]and never assigns it, soop.Output.History()panicked on any Veo operation. A response with neither field set panicked back in the guard.Separately,
append(mr.Request.Messages, mr.Message)reused spare capacity inRequest.Messageswhen it had any, writing the response message into a slot the caller still owned. The result aliased anything else pointing into that array, and two calls on one response returned slices sharing the element each had just written.Historynow returns early on a nil receiver, treats a nilRequestas no prior messages, and builds the result withslices.Clone, which also preserves the old nil return for the empty case.TestModelResponseHistorycovers seven cases: five panic or fail onmain, two pin behavior that already worked. Go 1.26.3, fullgo test ./...ingo/: 40 packages pass, 0 fail.Populating
Requeston every model path is a plugin-contract change and belongs with thecompat_oaistreaming gap in #5239.