Skip to content

Clone copilot SessionConfig.Tools before appending per-run tools - #595

Open
PratikDhanave wants to merge 1 commit into
microsoft:mainfrom
PratikDhanaveFork:clone-copilot-session-tools
Open

Clone copilot SessionConfig.Tools before appending per-run tools#595
PratikDhanave wants to merge 1 commit into
microsoft:mainfrom
PratikDhanaveFork:clone-copilot-session-tools

Conversation

@PratikDhanave

Copy link
Copy Markdown
Contributor

What

copySessionConfig did clone := *source, a shallow copy that leaves clone.Tools aliasing the shared provider-held p.cfg.SessionConfig.Tools, and copyResumeSessionConfig assigned Tools: source.Tools directly. Both sessionConfig and resumeSessionConfig then run cfg.Tools = append(cfg.Tools, copilotTools(options)...) per run.

When the user-supplied Tools slice 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) and Tools: slices.Clone(source.Tools). slices is already imported.

Why

Each run must own its Tools slice 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.go covering both sessionConfig and resumeSessionConfig: a provider whose SessionConfig.Tools has 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/..., and go test ./provider/copilotprovider/... all pass.

Copilot AI review requested due to automatic review settings July 23, 2026 02:16
@PratikDhanave
PratikDhanave requested a review from a team as a code owner July 23, 2026 02:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.Tools in copySessionConfig to avoid aliasing provider-held config state.
  • Clone SessionConfig.Tools in copyResumeSessionConfig for 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
PratikDhanave force-pushed the clone-copilot-session-tools branch 3 times, most recently from dd36a56 to c32f8e8 Compare July 23, 2026 15:44
@github-actions

This comment has been minimized.

@github-actions github-actions Bot added the parity-approved Go API consistency review found no parity issues label Jul 23, 2026
@PratikDhanave
PratikDhanave force-pushed the clone-copilot-session-tools branch from c32f8e8 to b009dec Compare July 24, 2026 01:42
@github-actions

This comment has been minimized.

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
PratikDhanave force-pushed the clone-copilot-session-tools branch from b009dec to 64e5191 Compare July 24, 2026 09:37
@github-actions

Copy link
Copy Markdown
Contributor

Parity Review: No Cross-Repo Consistency Issues

This PR fixes a data race in two unexported helper functions — copySessionConfig and copyResumeSessionConfig — inside provider/copilotprovider. The changes (slices.Clone before appending per-run tools) are purely internal correctness fixes with no effect on any exported Go API or observable user-facing behavior.

Scope verdict: Out of scope for parity review. No exported identifiers were added, removed, or changed. No user-visible defaults, option shapes, or runtime behaviors were altered.

Label status: No public-api-change label is warranted. The existing parity-approved label correctly reflects this PR.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

Generated by Go API Consistency Review Agent · 20.4 AIC · ⌖ 4.86 AIC · ⊞ 5.9K ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parity-approved Go API consistency review found no parity issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants