From 64e519145c4888cec731cc06241368294142755b Mon Sep 17 00:00:00 2001 From: PratikDhanave Date: Thu, 23 Jul 2026 07:46:08 +0530 Subject: [PATCH] Clone copilot SessionConfig.Tools before appending per-run tools copySessionConfig shallow-copied the source struct, so the clone's Tools field aliased the shared provider-held SessionConfig.Tools backing array, and copyResumeSessionConfig assigned it directly. sessionConfig and resumeSessionConfig then append per-run tools in place, so a caller slice with spare capacity is written through, and two concurrent runs of the same agent race on the same backing array. Clone Tools before appending so each run gets its own slice. --- provider/copilotprovider/copilot.go | 3 +- provider/copilotprovider/copilot_test.go | 96 ++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/provider/copilotprovider/copilot.go b/provider/copilotprovider/copilot.go index adb1daa3..30637b7a 100644 --- a/provider/copilotprovider/copilot.go +++ b/provider/copilotprovider/copilot.go @@ -252,6 +252,7 @@ func copySessionConfig(source *copilot.SessionConfig) copilot.SessionConfig { return copilot.SessionConfig{Streaming: copilot.Bool(true)} } clone := *source + clone.Tools = slices.Clone(source.Tools) clone.Streaming = copyBoolDefaultTrue(source.Streaming) return clone } @@ -263,7 +264,7 @@ func copyResumeSessionConfig(source *copilot.SessionConfig) copilot.ResumeSessio return copilot.ResumeSessionConfig{ Model: source.Model, ReasoningEffort: source.ReasoningEffort, - Tools: source.Tools, + Tools: slices.Clone(source.Tools), SystemMessage: source.SystemMessage, AvailableTools: source.AvailableTools, ExcludedTools: source.ExcludedTools, diff --git a/provider/copilotprovider/copilot_test.go b/provider/copilotprovider/copilot_test.go index ee27ca1e..63181fc5 100644 --- a/provider/copilotprovider/copilot_test.go +++ b/provider/copilotprovider/copilot_test.go @@ -215,6 +215,40 @@ func TestCopyResumeSessionConfig_WithStreamingNull_DefaultsToTrue(t *testing.T) assertEqual(t, runtime.lastResumeRequest(t)["streaming"], true, "streaming") } +func TestCreateSession_WithPerRunTools_DoesNotMutateSharedSessionConfigTools(t *testing.T) { + runtime := newFakeRuntime(t, idleEvent()) + shared := sharedToolsSessionConfig() + agent := copilotprovider.NewAgent(runtime.client(), copilotprovider.AgentConfig{SessionConfig: shared}) + + for range 2 { + if _, err := runText(t, agent, "hello", agentpkg.WithTool(perRunTool(t))); err != nil { + t.Fatalf("RunText: %v", err) + } + } + + assertSharedToolsUnchanged(t, shared) + assertRequestToolNames(t, runtime.lastCreateRequest(t), []string{"preconfigured", "PerRun"}) +} + +func TestResumeSession_WithPerRunTools_DoesNotMutateSharedSessionConfigTools(t *testing.T) { + runtime := newFakeRuntime(t, idleEvent()) + shared := sharedToolsSessionConfig() + agent := copilotprovider.NewAgent(runtime.client(), copilotprovider.AgentConfig{SessionConfig: shared}) + session, err := agent.CreateSession(context.Background(), agentpkg.WithServiceID("existing-session")) + if err != nil { + t.Fatalf("CreateSession: %v", err) + } + + for range 2 { + if _, err := runText(t, agent, "hello", agentpkg.WithSession(session), agentpkg.WithTool(perRunTool(t))); err != nil { + t.Fatalf("RunText: %v", err) + } + } + + assertSharedToolsUnchanged(t, shared) + assertRequestToolNames(t, runtime.lastResumeRequest(t), []string{"preconfigured", "PerRun"}) +} + func TestConvertToAgentResponseUpdate_AssistantMessageEventWhenStreaming_DoesNotEmitTextContent(t *testing.T) { runtime := newFakeRuntime(t, sessionEvent("assistant.message", map[string]any{"messageId": "msg-456", "content": "Some streamed content that was already delivered via delta events"}), @@ -607,6 +641,68 @@ func dataContent(t *testing.T, name, value string) *message.DataContent { } } +// sharedToolsSessionConfig returns a config whose Tools slice has spare capacity, +// mirroring a caller that preconfigured tools ahead of time. The spare capacity is +// what let the old shallow copy alias the shared backing array when a per-run tool +// was appended. +func sharedToolsSessionConfig() *copilot.SessionConfig { + tools := make([]copilot.Tool, 1, 4) + tools[0] = copilot.Tool{Name: "preconfigured"} + return &copilot.SessionConfig{Model: "gpt-4o", Tools: tools} +} + +func perRunTool(t *testing.T) tool.Tool { + t.Helper() + return functool.MustNew(functool.Config{Name: "PerRun", Description: "per-run tool"}, func(context.Context, struct{}) (string, error) { + return "ok", nil + }) +} + +// assertSharedToolsUnchanged verifies the provider never wrote a per-run tool +// through the caller's shared Tools slice, including its spare capacity beyond len. +func assertSharedToolsUnchanged(t *testing.T, config *copilot.SessionConfig) { + t.Helper() + if len(config.Tools) != 1 { + t.Fatalf("shared Tools mutated: len = %d, want 1", len(config.Tools)) + } + if got := config.Tools[0].Name; got != "preconfigured" { + t.Fatalf("shared Tools[0] corrupted: %q", got) + } + for i, tl := range config.Tools[:cap(config.Tools)] { + if i == 0 { + continue + } + if tl.Name != "" { + t.Fatalf("shared Tools backing array mutated at index %d: %q", i, tl.Name) + } + } +} + +func assertRequestToolNames(t *testing.T, request map[string]any, want []string) { + t.Helper() + raw, ok := request["tools"].([]any) + if !ok { + t.Fatalf("tools = %#v, want slice", request["tools"]) + } + got := make([]string, 0, len(raw)) + for _, item := range raw { + entry, ok := item.(map[string]any) + if !ok { + t.Fatalf("tool entry = %#v, want object", item) + } + name, _ := entry["name"].(string) + got = append(got, name) + } + if len(got) != len(want) { + t.Fatalf("tool names = %#v, want %#v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("tool names = %#v, want %#v", got, want) + } + } +} + func richSessionConfig() *copilot.SessionConfig { return &copilot.SessionConfig{ Model: "gpt-4o",