Clone copilot SessionConfig.Tools before appending per-run tools#595
Open
PratikDhanave wants to merge 1 commit into
Open
Clone copilot SessionConfig.Tools before appending per-run tools#595PratikDhanave wants to merge 1 commit into
PratikDhanave wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a concurrency/data-race risk in the Copilot provider’s session configuration building by ensuring each run owns its own SessionConfig.Tools slice before per-run tools are appended.
Changes:
- Clone
SessionConfig.ToolsincopySessionConfigto avoid aliasing provider-held config state. - Clone
SessionConfig.ToolsincopyResumeSessionConfigfor the same reason when resuming sessions. - Add internal tests validating that per-run tool appends do not share backing arrays with the provider config (or across runs).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| provider/copilotprovider/copilot.go | Clones Tools slices in session config copy helpers to prevent shared backing arrays and data races when appending per-run tools. |
| provider/copilotprovider/copilot_internal_test.go | Adds regression tests ensuring per-run tool appends do not mutate or alias the provider’s shared SessionConfig.Tools backing array. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PratikDhanave
force-pushed
the
clone-copilot-session-tools
branch
from
July 23, 2026 02:31
ee5cc14 to
72842b9
Compare
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.
PratikDhanave
force-pushed
the
clone-copilot-session-tools
branch
from
July 23, 2026 08:33
72842b9 to
dd36a56
Compare
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.
What
copySessionConfigdidclone := *source, a shallow copy that leavesclone.Toolsaliasing the shared provider-heldp.cfg.SessionConfig.Tools, andcopyResumeSessionConfigassignedTools: source.Toolsdirectly. BothsessionConfigandresumeSessionConfigthen runcfg.Tools = append(cfg.Tools, copilotTools(options)...)per run.When the user-supplied
Toolsslice has spare capacity, that append writes the per-run tools into the caller's backing array, and two concurrent runs of the same agent write into the same index — a data race and a corrupted tool list.The fix clones the slice before appending:
clone.Tools = slices.Clone(source.Tools)andTools: slices.Clone(source.Tools).slicesis already imported.Why
Each run must own its
Toolsslice so appending per-run tools never mutates the shared session config or another concurrent run. This mirrors the clone-before-append fixes already merged in #529 and #555, keeping copilot session setup consistent with the rest of the port.Testing
Added
copilot_internal_test.gocovering bothsessionConfigandresumeSessionConfig: a provider whoseSessionConfig.Toolshas spare capacity is invoked twice with a per-run tool, asserting the shared config is unchanged and that neither returned slice shares a backing array with the shared config or with each other. The tests fail before the fix and pass after.go build ./...,go vet ./provider/copilotprovider/..., andgo test ./provider/copilotprovider/...all pass.