Clone anthropic MessageNewParams slices to avoid mutating caller state#529
Open
PratikDhanave wants to merge 3 commits into
Open
Clone anthropic MessageNewParams slices to avoid mutating caller state#529PratikDhanave wants to merge 3 commits into
PratikDhanave wants to merge 3 commits into
Conversation
buildMessageParams took the caller-provided MessageNewParams option by struct copy (params = p) and then appended system instructions and messages to params.System / params.Messages. Because the struct copy shares the caller's slice backing arrays, appending within spare capacity silently overwrote the caller's data, and a reused params value would accumulate duplicated system blocks across runs. Clone the two mutated slice fields after adopting the option, mirroring what the gemini provider already does for the same reason. Adds a black-box test asserting the caller's System backing array is untouched.
There was a problem hiding this comment.
Pull request overview
This PR prevents the Anthropic provider from accidentally mutating caller-supplied anthropic.MessageNewParams slice backing arrays when buildMessageParams appends system instructions and messages, aligning behavior with the existing Gemini provider approach. It also adds a regression test to ensure caller slice state is preserved.
Changes:
- Clone
MessageNewParams.SystemandMessageNewParams.Messagesimmediately after adopting theMessageNewParamsoption to avoid aliasing the caller’s slice backing arrays. - Add a regression test that validates building a request does not mutate a caller-provided
Systemslice with spare capacity. - Minor formatting/gofmt adjustments in the Anthropic provider test helper/config test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| provider/anthropicprovider/agent.go | Clones System and Messages slices after adopting MessageNewParams to avoid mutating caller state when appending. |
| provider/anthropicprovider/agent_test.go | Adds a regression test ensuring caller-provided System slice backing array is not mutated; includes minor formatting changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+472
to
+476
| // Caller-supplied System slice with spare capacity. | ||
| system := make([]anthropic.TextBlockParam, 1, 4) | ||
| system[0] = anthropic.TextBlockParam{Text: "s0"} | ||
| opt := anthropicprovider.MessageNewParams(anthropic.MessageNewParams{System: system}) | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Done — added TestBuildMessageParams_DoesNotMutateCallerMessagesSlice, a red-green regression test covering a caller-supplied Messages slice with spare capacity.
Address review feedback: the fix clones both System and Messages, but the test only covered System. Add a regression test asserting a caller-supplied Messages slice with spare capacity is not mutated.
# Conflicts: # provider/anthropicprovider/agent_test.go
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
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.
Problem
buildMessageParamsadopts the caller-providedMessageNewParamsoption by struct copy:It then appends to
params.System(system instructions) andparams.Messages. A struct copy shares the slice backing arrays with the caller's value, so:MessageNewParamsacross runs accumulates duplicated system blocks.The gemini provider already clones for exactly this reason (
provider/geminiprovider/agent.go), so this also aligns the two providers.Fix
Clone the two mutated slice fields right after adopting the option:
Test
TestBuildMessageParams_DoesNotMutateCallerSystemSlice(black-box, inagent_test.go) passes aSystemslice with spare capacity and asserts the caller's backing array is untouched after a run. It fails onmain(spare slot = "added") and passes with the fix.